[Pdbg] [PATCH v2 11/16] libpdbg: Rework chip-id functions

[Alistair Popple]

On Wednesday, 7 November 2018 5:36:46 PM AEDT Amitay Isaacs wrote:
> On Wed, 2018-11-07 at 16:39 +1100, Alistair Popple wrote:
> > Rename the chip-id functions to be consistent with other libpdbg
> > function names and move them in with the rest of the general libpdbg
> > code.
> > 
> > Signed-off-by: Alistair Popple <alistair at popple.id.au>
> > ---
> > 
> >  libpdbg/device.c  | 19 -------------------
> >  libpdbg/device.h  |  4 ----
> >  libpdbg/host.c    |  2 +-
> >  libpdbg/htm.c     |  4 ++--
> >  libpdbg/libpdbg.c | 15 +++++++++++++++
> >  libpdbg/libpdbg.h |  4 ++++
> >  6 files changed, 22 insertions(+), 26 deletions(-)
> > 
> > diff --git a/libpdbg/device.c b/libpdbg/device.c
> > index 226cf12..a7212a6 100644
> > --- a/libpdbg/device.c
> > +++ b/libpdbg/device.c
> > @@ -675,25 +675,6 @@ u64 dt_get_address(const struct pdbg_target
> > *node, unsigned int index,
> > 
> >  	return dt_get_number(p->prop + pos, na);
> >  
> >  }
> > 
> > -static u32 __dt_get_chip_id(const struct pdbg_target *node)
> > -{
> > -	const struct dt_property *prop;
> > -
> > -	for (; node; node = node->parent) {
> > -		prop = dt_find_property(node, "chip-id");
> > -		if (prop)
> > -			return dt_property_get_cell(prop, 0);
> > -	}
> > -	return 0xffffffff;
> > -}
> > -
> > -u32 dt_get_chip_id(const struct pdbg_target *node)
> > -{
> > -	u32 id = __dt_get_chip_id(node);
> > -	assert(id != 0xffffffff);
> > -	return id;
> > -}
> > -
> > 
> >  void pdbg_targets_init(void *fdt)
> >  {
> >  
> >  	dt_root = dt_new_node("", NULL, 0);
> > 
> > diff --git a/libpdbg/device.h b/libpdbg/device.h
> > index f487443..a050a23 100644
> > --- a/libpdbg/device.h
> > +++ b/libpdbg/device.h
> > @@ -44,10 +44,6 @@ const void *dt_prop_get(const struct pdbg_target
> > *node, const char *prop);
> > 
> >  const void *dt_prop_get_def(const struct pdbg_target *node, const
> > 
> > char *prop,
> > 
> >  			    void *def);
> > 
> > -/* Find an chip-id property in this node; if not found, walk up the
> > parent
> > - * nodes. Returns -1 if no chip-id property exists. */
> > -u32 dt_get_chip_id(const struct pdbg_target *node);
> > -
> > 
> >  /* Address accessors ("reg" properties parsing). No translation,
> >  
> >   * only support "simple" address forms (1 or 2 cells). Asserts
> >   * if address doesn't exist
> > 
> > diff --git a/libpdbg/host.c b/libpdbg/host.c
> > index eb627be..15e28a0 100644
> > --- a/libpdbg/host.c
> > +++ b/libpdbg/host.c
> > @@ -91,7 +91,7 @@ static int host_pib_probe(struct pdbg_target
> > *target)
> > 
> >  	if (!fd)
> >  	
> >  		return -1;
> > 
> > -	chip_id = dt_get_chip_id(target);
> > +	chip_id = pdbg_target_chip_id(target);
> > 
> >  	if (chip_id == -1)
> >  	
> >  		goto out;
> 
> chip_id cannot be -1 unless it is set in device-tree as -1.
> pdbg_target_chip_id() does not return -1 as error condition.

Good catch. Will remove the checks ... not much we can do if we don't know the 
chip-id and in any case pdbg_target_chip_id() will complain loudly (ie. 
assert(0)) if it couldn't find one.
 
> > diff --git a/libpdbg/htm.c b/libpdbg/htm.c
> > index f9013e5..1ea2c3a 100644
> > --- a/libpdbg/htm.c
> > +++ b/libpdbg/htm.c
> > @@ -551,7 +551,7 @@ static char *get_debugfs_file(struct htm *htm,
> > const char *file)
> > 
> >  	uint32_t chip_id;
> >  	char *filename;
> > 
> > -	chip_id = dt_get_chip_id(&htm->target);
> > +	chip_id = pdbg_target_chip_id(&htm->target);
> > 
> >  	if (chip_id == -1) {
> >  	
> >  		PR_ERROR("Couldn't find a chip id\n");
> >  		return NULL;
> 
> Same problem here.
> 
> > @@ -990,7 +990,7 @@ static int do_htm_dump(struct htm *htm, char
> > *filename)
> > 
> >  		return -1;
> >  	
> >  	}
> > 
> > -	chip_id = dt_get_chip_id(&htm->target);
> > +	chip_id = pdbg_target_chip_id(&htm->target);
> > 
> >  	if (chip_id == -1)
> >  	
> >  		return -1;
> 
> And again.
> 
> > diff --git a/libpdbg/libpdbg.c b/libpdbg/libpdbg.c
> > index f638fd2..07947cb 100644
> > --- a/libpdbg/libpdbg.c
> > +++ b/libpdbg/libpdbg.c
> > @@ -150,6 +150,21 @@ int pdbg_target_u32_property(struct pdbg_target
> > *target, const char *name, uint3
> > 
> >          return 0;
> >  
> >  }
> > 
> > +uint32_t pdbg_target_chip_id(struct pdbg_target *target)
> > +{
> > +        uint32_t id;
> > +
> > +	while (pdbg_target_u32_property(target, "chip-id", &id)) {
> > +		target = target->parent;
> > +
> > +		/* If we hit this we've reached the top of the tree
> > +		 * and haven't found chip-id */
> > +		assert(target);
> > +	}
> > +
> > +	return id;
> > +}
> > +
> 
> Where would you need chip-id of the some random grand*-parent as the
> chip-id for the target?  Is chip-id supposed to refer to a physical
> chip or something?  Am I missing some implicit assumption here?

I don't think so. It is supposed to refer to a physical chiplet which has a 
standard well defined translation although we're still figuring out all the 
details. Arguably we shouldn't define a special function to read it but it's 
currently used in a few places and I wanted to limit the scope of the clean-
ups to code reorganisation with no behavioural/functional changes (other than 
perhaps renaming a few functions).

- Alistair

> >  void pdbg_progress_tick(uint64_t cur, uint64_t end)
> >  {
> >  
> >  	if (progress_tick)
> > 
> > diff --git a/libpdbg/libpdbg.h b/libpdbg/libpdbg.h
> > index 8d8f0a3..4a21671 100644
> > --- a/libpdbg/libpdbg.h
> > +++ b/libpdbg/libpdbg.h
> > @@ -82,6 +82,10 @@ uint64_t pdbg_get_address(struct pdbg_target
> > *target, uint64_t *size);
> > 
> >  #define pdbg_get_target_property(target, name, size) \
> >  
> >  	pdbg_target_property(target, name, size)
> > 
> > +/* Find an chip-id property in this node; if not found, walk up the
> > parent
> > + * nodes. Returns -1 if no chip-id property exists. */
> > +uint32_t pdbg_target_chip_id(struct pdbg_target *node);
> > +
> > 
> >  /* Misc. */
> >  void pdbg_targets_init(void *fdt);
> >  void pdbg_target_probe_all(struct pdbg_target *parent);
> 
> Amitay.