| Message ID | 20181107053943.4307-12-alistair@popple.id.au |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Cleanup old code | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | master/apply_patch Successfully applied |
| snowpatch_ozlabs/build-multiarch | success | Test build-multiarch on branch master |
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@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.
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@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.
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; 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; @@ -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; 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; +} + 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);
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@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(-)