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

[Amitay Isaacs]

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.

> 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?

>  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);
> -- 
> 2.11.0
> 

Amitay.
-- 

The man with imagination is never alone.