[U-Boot] [PATCH 3/6] fdtdec: Implement fdtdec_set_phandle()

[Simon Glass]

Hi Thierry,

On Mon, 11 Mar 2019 at 18:04, Thierry Reding <thierry.reding at gmail.com> wrote:
>
> On Sun, Mar 10, 2019 at 03:51:40PM -0600, Simon Glass wrote:
> > Hi Thierry,
> >
> > On Fri, 8 Mar 2019 at 13:11, Thierry Reding <thierry.reding at gmail.com> wrote:
> > >
> > > From: Thierry Reding <treding at nvidia.com>
> > >
> > > This function can be used to set a phandle for a given node.
> > >
> > > Signed-off-by: Thierry Reding <treding at nvidia.com>
> > > ---
> > >  include/fdtdec.h | 11 +++++++++++
> > >  lib/fdtdec.c     | 16 ++++++++++++++++
> > >  2 files changed, 27 insertions(+)
> >
> > This seems OK, although I think it should have a test.
>
> I'll add something to test this to the test_fdtdec command. I'm not sure
> how much sense it makes to test this individually, because the test is
> fairly elaborate (needs to create one node to reference and another to
> reference it from), so perhaps I can just add a complete test that will
> at the same time validate that the reserved-memory and carveout stuff
> works?
>
> > But what about livetree? I think it would make more sense to add a
> > high-level API which can deal with livetree/flattree.
>
> I'm not sure it really makes sense to add this kind of information to a
> livetree. The only use-case for this that I have is to convey
> information about carveouts to the kernel, so by this information is
> added to a DTB that is loaded from external storage along with the
> kernel and then modified right before the DTB is passed to the kernel on
> boot.
>
> The only case where I think such functionality would be useful in a live
> tree is if we construct the live tree in U-Boot, update it and then pass
> it to the kernel upon boot. That's not something that works today, and I
> can't test any of that, so I'm not sure it makes much sense to implement
> it now.
>
> > > diff --git a/include/fdtdec.h b/include/fdtdec.h
> > > index 5eb3c0c237a9..997103a87cdf 100644
> > > --- a/include/fdtdec.h
> > > +++ b/include/fdtdec.h
> > > @@ -968,6 +968,17 @@ int fdtdec_setup_memory_banksize(void);
> > >   */
> > >  int fdtdec_get_max_phandle(const void *blob, uint32_t *maxp);
> > >
> > > +/**
> > > + * fdtdec_set_phandle() - sets the phandle of a given node
> > > + *
> > > + * @param blob         FDT blob
> > > + * @param node         offset in the FDT blob of the node whose phandle is to
> > > + *                     be set
> > > + * @param phandle      phandle to set for the given node
> > > + * @return 0 on success or a negative error code on failure
> > > + */
> > > +int fdtdec_set_phandle(void *blob, int node, uint32_t phandle);
> > > +
> > >  /**
> > >   * Set up the device tree ready for use
> > >   */
> > > diff --git a/lib/fdtdec.c b/lib/fdtdec.c
> > > index f2af947c106e..9195a05d1129 100644
> > > --- a/lib/fdtdec.c
> > > +++ b/lib/fdtdec.c
> > > @@ -1271,6 +1271,22 @@ int fdtdec_get_max_phandle(const void *blob, uint32_t *maxp)
> > >         return 0;
> > >  }
> > >
> > > +int fdtdec_set_phandle(void *blob, int node, uint32_t phandle)
> > > +{
> > > +       fdt32_t value = cpu_to_fdt32(phandle);
> > > +       int err;
> > > +
> > > +       err = fdt_setprop(blob, node, "linux,phandle", &value, sizeof(value));
> > > +       if (err < 0)
> > > +               return err;
> >
> > Why set both properties?
>
> I was trying to mimic the output of DTC, but looking again it seems like
> it doesn't even produce linux,phandle properties. Doing some research it
> was changed to only produce "phandle" properties in v1.4.5 and the
> commit message says:
>
>         commit 0016f8c2aa32423f680ec6e94a00f1095b81b5fc
>         Author: Rob Herring <robh at kernel.org>
>         Date:   Wed Jul 12 17:20:30 2017 -0500
>
>             dtc: change default phandles to ePAPR style instead of both
>
>             Currently, both legacy (linux,phandle) and ePAPR (phandle) properties
>             are inserted into dtbs by default. The newer ePAPR style has been
>             supported in dtc and Linux kernel for 7 years. That should be a long
>             enough transition period. We can save a little space by not putting both
>             into the dtb.
>
>             Signed-off-by: Rob Herring <robh at kernel.org>
>             Signed-off-by: David Gibson <david at gibson.dropbear.id.au>
>
> So perhaps we no longer need to generate this from U-Boot either. I'll
> remove the linux,phandle code.
>

OK this all sounds good to me.

Regards,
Simon