| Message ID | 20190308201140.2383-2-thierry.reding@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | [U-Boot,1/6] fdtdec: Add cpu_to_fdt_{addr,size}() macros | expand |
Hi Thierry, On Fri, 8 Mar 2019 at 13:11, Thierry Reding <thierry.reding@gmail.com> wrote: > > From: Thierry Reding <treding@nvidia.com> > > This function allows looking up the highest phandle value stored in a > device tree, which is useful to determine the next best phandle value > for new nodes. > > Signed-off-by: Thierry Reding <treding@nvidia.com> > --- > include/fdtdec.h | 12 ++++++++++++ > lib/fdtdec.c | 28 ++++++++++++++++++++++++++++ > 2 files changed, 40 insertions(+) Can we use fdt_get_max_phandle() instead? If not, could you please add a bit more detail to the commit message as we might consider changing the upstream function. Regards, Simon
On Sun, Mar 10, 2019 at 03:51:31PM -0600, Simon Glass wrote: > Hi Thierry, > > On Fri, 8 Mar 2019 at 13:11, Thierry Reding <thierry.reding@gmail.com> wrote: > > > > From: Thierry Reding <treding@nvidia.com> > > > > This function allows looking up the highest phandle value stored in a > > device tree, which is useful to determine the next best phandle value > > for new nodes. > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > --- > > include/fdtdec.h | 12 ++++++++++++ > > lib/fdtdec.c | 28 ++++++++++++++++++++++++++++ > > 2 files changed, 40 insertions(+) > > Can we use fdt_get_max_phandle() instead? If not, could you please add > a bit more detail to the commit message as we might consider changing > the upstream function. fdt_get_max_phandle() has a slightly awkward signature. It returns the phandle via the return value, which means that it can distinguish between error conditions and also uses 0 and -1 to signal no phandle found and error conditions, respectively. Using the function requires weird looking code like this: uint32_t phandle; phandle = fdt_get_max_phandle(fdt); if (phandle == 0 || phandle == (uint32_t)-1) /* process error */; So the goal here was to add an alternative that would provide a more standard interface where a regular error could be returned via the return value and the phandle would be stored in an output parameter on success. I specifically didn't want to update the upstream function because it was introduced about 2.5 years ago and will probably be used by some number of users. I'm not sure if there's a stable API policy for libfdt, but I would expect a lot of users to be annoyed if we just changed the signature of fdt_get_max_phandle(). That said, perhaps another alternative would be to implement this as a different function. If you look at the subsequent patches, the goal is to generate a new phandle for newly added nodes, so perhaps something like this could work: int fdtdec_generate_phandle(const void *fdt, uint32_t *phandle); That would be slightly more in line with the higher level of fdtdec compared to libfdt. Or perhaps you'd prefer fdt_generate_phandle() in libfdt? Thierry
Hi Thierry, On Mon, 11 Mar 2019 at 17:27, Thierry Reding <thierry.reding@gmail.com> wrote: > > On Sun, Mar 10, 2019 at 03:51:31PM -0600, Simon Glass wrote: > > Hi Thierry, > > > > On Fri, 8 Mar 2019 at 13:11, Thierry Reding <thierry.reding@gmail.com> wrote: > > > > > > From: Thierry Reding <treding@nvidia.com> > > > > > > This function allows looking up the highest phandle value stored in a > > > device tree, which is useful to determine the next best phandle value > > > for new nodes. > > > > > > Signed-off-by: Thierry Reding <treding@nvidia.com> > > > --- > > > include/fdtdec.h | 12 ++++++++++++ > > > lib/fdtdec.c | 28 ++++++++++++++++++++++++++++ > > > 2 files changed, 40 insertions(+) > > > > Can we use fdt_get_max_phandle() instead? If not, could you please add > > a bit more detail to the commit message as we might consider changing > > the upstream function. > > fdt_get_max_phandle() has a slightly awkward signature. It returns the > phandle via the return value, which means that it can distinguish > between error conditions and also uses 0 and -1 to signal no phandle > found and error conditions, respectively. Using the function requires > weird looking code like this: > > uint32_t phandle; > > phandle = fdt_get_max_phandle(fdt); > if (phandle == 0 || phandle == (uint32_t)-1) > /* process error */; > > So the goal here was to add an alternative that would provide a more > standard interface where a regular error could be returned via the > return value and the phandle would be stored in an output parameter on > success. > > I specifically didn't want to update the upstream function because it > was introduced about 2.5 years ago and will probably be used by some > number of users. I'm not sure if there's a stable API policy for libfdt, > but I would expect a lot of users to be annoyed if we just changed the > signature of fdt_get_max_phandle(). > > That said, perhaps another alternative would be to implement this as a > different function. If you look at the subsequent patches, the goal is > to generate a new phandle for newly added nodes, so perhaps something > like this could work: > > int fdtdec_generate_phandle(const void *fdt, uint32_t *phandle); That seems like a good idea. > > That would be slightly more in line with the higher level of fdtdec > compared to libfdt. > > Or perhaps you'd prefer fdt_generate_phandle() in libfdt? Well yes, then David weighs in and we avoid a blind alley which won't pass muster upstream. If you do send an upstream patch, make sure to add tests. Regards, Simon
diff --git a/include/fdtdec.h b/include/fdtdec.h index a965c33157c9..5eb3c0c237a9 100644 --- a/include/fdtdec.h +++ b/include/fdtdec.h @@ -956,6 +956,18 @@ int fdtdec_setup_mem_size_base(void); */ int fdtdec_setup_memory_banksize(void); +/** + * fdtdec_get_max_phandle() - return the highest phandle in an FDT blob + * + * Returns the highest phandle in the given FDT blob. The result of this can + * be used to generate a new phandle by incrementing by one. + * + * @param blob FDT blob + * @param maxp return location for the highest phandle in the FDT blob + * @return 0 on success or a negative error code on failure + */ +int fdtdec_get_max_phandle(const void *blob, uint32_t *maxp); + /** * Set up the device tree ready for use */ diff --git a/lib/fdtdec.c b/lib/fdtdec.c index 09a7e133a539..f2af947c106e 100644 --- a/lib/fdtdec.c +++ b/lib/fdtdec.c @@ -1243,6 +1243,34 @@ __weak void *board_fdt_blob_setup(void) } #endif +int fdtdec_get_max_phandle(const void *blob, uint32_t *maxp) +{ + uint32_t max = 0; + int offset = -1; + + while (true) { + uint32_t phandle; + + offset = fdt_next_node(blob, offset, NULL); + if (offset < 0) { + if (offset == -FDT_ERR_NOTFOUND) + break; + + return offset; + } + + phandle = fdt_get_phandle(blob, offset); + + if (phandle > max) + max = phandle; + } + + if (maxp) + *maxp = max; + + return 0; +} + int fdtdec_setup(void) { #if CONFIG_IS_ENABLED(OF_CONTROL)