| Message ID | 20190308201140.2383-3-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 can be used to set a phandle for a given node. > > Signed-off-by: Thierry Reding <treding@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. But what about livetree? I think it would make more sense to add a high-level API which can deal with livetree/flattree. > > 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? > + > + err = fdt_setprop(blob, node, "phandle", &value, sizeof(value)); > + if (err < 0) > + return err; > + > + return 0; > +} > + > int fdtdec_setup(void) > { > #if CONFIG_IS_ENABLED(OF_CONTROL) > -- > 2.20.1 > Regards, SImon
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@gmail.com> wrote: > > > > From: Thierry Reding <treding@nvidia.com> > > > > This function can be used to set a phandle for a given node. > > > > Signed-off-by: Thierry Reding <treding@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@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@kernel.org> Signed-off-by: David Gibson <david@gibson.dropbear.id.au> So perhaps we no longer need to generate this from U-Boot either. I'll remove the linux,phandle code. Thierry
Hi Thierry, On Mon, 11 Mar 2019 at 18:04, Thierry Reding <thierry.reding@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@gmail.com> wrote: > > > > > > From: Thierry Reding <treding@nvidia.com> > > > > > > This function can be used to set a phandle for a given node. > > > > > > Signed-off-by: Thierry Reding <treding@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@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@kernel.org> > Signed-off-by: David Gibson <david@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
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; + + err = fdt_setprop(blob, node, "phandle", &value, sizeof(value)); + if (err < 0) + return err; + + return 0; +} + int fdtdec_setup(void) { #if CONFIG_IS_ENABLED(OF_CONTROL)