| Message ID | 20180813155347.13844-2-jens.wiklander@linaro.org |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | AVB using OP-TEE | expand |
On Mon, Aug 13, 2018 at 05:53:38PM +0200, Jens Wiklander wrote: > Just as /chosen may contain devices /firmware may contain devices, scan > for devices under /firmware too. > > Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> > --- > drivers/core/root.c | 15 ++++++++++----- > 1 file changed, 10 insertions(+), 5 deletions(-) > > diff --git a/drivers/core/root.c b/drivers/core/root.c > index 72bcc7d7f2a3..0dca507e1187 100644 > --- a/drivers/core/root.c > +++ b/drivers/core/root.c > @@ -265,9 +265,15 @@ static int dm_scan_fdt_node(struct udevice *parent, const void *blob, > for (offset = fdt_first_subnode(blob, offset); > offset > 0; > offset = fdt_next_subnode(blob, offset)) { > - /* "chosen" node isn't a device itself but may contain some: */ > - if (!strcmp(fdt_get_name(blob, offset, NULL), "chosen")) { > - pr_debug("parsing subnodes of \"chosen\"\n"); > + const char *node_name = fdt_get_name(blob, offset, NULL); > + > + /* > + * The "chosen" and "firmware" nodes aren't devices > + * themselves but may contain some: > + */ > + if (!strcmp(node_name, "chosen") || > + !strcmp(node_name, "firmware")) { > + pr_debug("parsing subnodes of \"%s\"\n", node_name); > > err = dm_scan_fdt_node(parent, blob, offset, > pre_reloc_only); So, the /firmware node seems special. Have you talked with the devicetree folks to get it listed in the spec? That would seem rather valuable for something used by many parties. Thanks!
On 15.8.2018 16:17, Tom Rini wrote: > On Mon, Aug 13, 2018 at 05:53:38PM +0200, Jens Wiklander wrote: > >> Just as /chosen may contain devices /firmware may contain devices, scan >> for devices under /firmware too. >> >> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> >> --- >> drivers/core/root.c | 15 ++++++++++----- >> 1 file changed, 10 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/core/root.c b/drivers/core/root.c >> index 72bcc7d7f2a3..0dca507e1187 100644 >> --- a/drivers/core/root.c >> +++ b/drivers/core/root.c >> @@ -265,9 +265,15 @@ static int dm_scan_fdt_node(struct udevice *parent, const void *blob, >> for (offset = fdt_first_subnode(blob, offset); >> offset > 0; >> offset = fdt_next_subnode(blob, offset)) { >> - /* "chosen" node isn't a device itself but may contain some: */ >> - if (!strcmp(fdt_get_name(blob, offset, NULL), "chosen")) { >> - pr_debug("parsing subnodes of \"chosen\"\n"); >> + const char *node_name = fdt_get_name(blob, offset, NULL); >> + >> + /* >> + * The "chosen" and "firmware" nodes aren't devices >> + * themselves but may contain some: >> + */ >> + if (!strcmp(node_name, "chosen") || >> + !strcmp(node_name, "firmware")) { >> + pr_debug("parsing subnodes of \"%s\"\n", node_name); >> >> err = dm_scan_fdt_node(parent, blob, offset, >> pre_reloc_only); > > So, the /firmware node seems special. Have you talked with the > devicetree folks to get it listed in the spec? That would seem rather > valuable for something used by many parties. Thanks! > some days ago we have sent a patch for this too. https://lists.denx.de/pipermail/u-boot/2018-August/338049.html It is using different way but it should do the same thing. Thanks, Michal
On Wed, Aug 15, 2018 at 04:30:15PM +0200, Michal Simek wrote: > On 15.8.2018 16:17, Tom Rini wrote: > > On Mon, Aug 13, 2018 at 05:53:38PM +0200, Jens Wiklander wrote: > > > >> Just as /chosen may contain devices /firmware may contain devices, scan > >> for devices under /firmware too. > >> > >> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> > >> --- > >> drivers/core/root.c | 15 ++++++++++----- > >> 1 file changed, 10 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/core/root.c b/drivers/core/root.c > >> index 72bcc7d7f2a3..0dca507e1187 100644 > >> --- a/drivers/core/root.c > >> +++ b/drivers/core/root.c > >> @@ -265,9 +265,15 @@ static int dm_scan_fdt_node(struct udevice *parent, const void *blob, > >> for (offset = fdt_first_subnode(blob, offset); > >> offset > 0; > >> offset = fdt_next_subnode(blob, offset)) { > >> - /* "chosen" node isn't a device itself but may contain some: */ > >> - if (!strcmp(fdt_get_name(blob, offset, NULL), "chosen")) { > >> - pr_debug("parsing subnodes of \"chosen\"\n"); > >> + const char *node_name = fdt_get_name(blob, offset, NULL); > >> + > >> + /* > >> + * The "chosen" and "firmware" nodes aren't devices > >> + * themselves but may contain some: > >> + */ > >> + if (!strcmp(node_name, "chosen") || > >> + !strcmp(node_name, "firmware")) { > >> + pr_debug("parsing subnodes of \"%s\"\n", node_name); > >> > >> err = dm_scan_fdt_node(parent, blob, offset, > >> pre_reloc_only); > > > > So, the /firmware node seems special. Have you talked with the > > devicetree folks to get it listed in the spec? That would seem rather > > valuable for something used by many parties. Thanks! > > > > some days ago we have sent a patch for this too. > https://lists.denx.de/pipermail/u-boot/2018-August/338049.html > > It is using different way but it should do the same thing. OK, so it sounds like in terms of code clean-ups, we need something like what you reference and then some further clean-ups on top of that perhaps for other places to call dm_scan_fdt_ofnode_path() for special cases. And in terms of formalized specification bits, I do think /firmware should perhaps get kicked up to the spec itself so that it's very clear to all consumers.
Hi Rob, On 15.8.2018 16:34, Tom Rini wrote: > On Wed, Aug 15, 2018 at 04:30:15PM +0200, Michal Simek wrote: >> On 15.8.2018 16:17, Tom Rini wrote: >>> On Mon, Aug 13, 2018 at 05:53:38PM +0200, Jens Wiklander wrote: >>> >>>> Just as /chosen may contain devices /firmware may contain devices, scan >>>> for devices under /firmware too. >>>> >>>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> >>>> --- >>>> drivers/core/root.c | 15 ++++++++++----- >>>> 1 file changed, 10 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/drivers/core/root.c b/drivers/core/root.c >>>> index 72bcc7d7f2a3..0dca507e1187 100644 >>>> --- a/drivers/core/root.c >>>> +++ b/drivers/core/root.c >>>> @@ -265,9 +265,15 @@ static int dm_scan_fdt_node(struct udevice *parent, const void *blob, >>>> for (offset = fdt_first_subnode(blob, offset); >>>> offset > 0; >>>> offset = fdt_next_subnode(blob, offset)) { >>>> - /* "chosen" node isn't a device itself but may contain some: */ >>>> - if (!strcmp(fdt_get_name(blob, offset, NULL), "chosen")) { >>>> - pr_debug("parsing subnodes of \"chosen\"\n"); >>>> + const char *node_name = fdt_get_name(blob, offset, NULL); >>>> + >>>> + /* >>>> + * The "chosen" and "firmware" nodes aren't devices >>>> + * themselves but may contain some: >>>> + */ >>>> + if (!strcmp(node_name, "chosen") || >>>> + !strcmp(node_name, "firmware")) { >>>> + pr_debug("parsing subnodes of \"%s\"\n", node_name); >>>> >>>> err = dm_scan_fdt_node(parent, blob, offset, >>>> pre_reloc_only); >>> >>> So, the /firmware node seems special. Have you talked with the >>> devicetree folks to get it listed in the spec? That would seem rather >>> valuable for something used by many parties. Thanks! >>> >> >> some days ago we have sent a patch for this too. >> https://lists.denx.de/pipermail/u-boot/2018-August/338049.html >> >> It is using different way but it should do the same thing. > > OK, so it sounds like in terms of code clean-ups, we need something like > what you reference and then some further clean-ups on top of that > perhaps for other places to call dm_scan_fdt_ofnode_path() for special > cases. And in terms of formalized specification bits, I do think > /firmware should perhaps get kicked up to the spec itself so that it's > very clear to all consumers. I was also checking latest devicetree spec and there is no record about /firmware node and how it is supposed to be used. Thanks, Michal
On Wed, Aug 15, 2018 at 8:51 AM Michal Simek <michal.simek@xilinx.com> wrote: > > Hi Rob, > > On 15.8.2018 16:34, Tom Rini wrote: > > On Wed, Aug 15, 2018 at 04:30:15PM +0200, Michal Simek wrote: > >> On 15.8.2018 16:17, Tom Rini wrote: > >>> On Mon, Aug 13, 2018 at 05:53:38PM +0200, Jens Wiklander wrote: > >>> > >>>> Just as /chosen may contain devices /firmware may contain devices, scan > >>>> for devices under /firmware too. > >>>> > >>>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> > >>>> --- > >>>> drivers/core/root.c | 15 ++++++++++----- > >>>> 1 file changed, 10 insertions(+), 5 deletions(-) > >>>> > >>>> diff --git a/drivers/core/root.c b/drivers/core/root.c > >>>> index 72bcc7d7f2a3..0dca507e1187 100644 > >>>> --- a/drivers/core/root.c > >>>> +++ b/drivers/core/root.c > >>>> @@ -265,9 +265,15 @@ static int dm_scan_fdt_node(struct udevice *parent, const void *blob, > >>>> for (offset = fdt_first_subnode(blob, offset); > >>>> offset > 0; > >>>> offset = fdt_next_subnode(blob, offset)) { > >>>> - /* "chosen" node isn't a device itself but may contain some: */ > >>>> - if (!strcmp(fdt_get_name(blob, offset, NULL), "chosen")) { > >>>> - pr_debug("parsing subnodes of \"chosen\"\n"); > >>>> + const char *node_name = fdt_get_name(blob, offset, NULL); > >>>> + > >>>> + /* > >>>> + * The "chosen" and "firmware" nodes aren't devices > >>>> + * themselves but may contain some: > >>>> + */ > >>>> + if (!strcmp(node_name, "chosen") || > >>>> + !strcmp(node_name, "firmware")) { > >>>> + pr_debug("parsing subnodes of \"%s\"\n", node_name); > >>>> > >>>> err = dm_scan_fdt_node(parent, blob, offset, > >>>> pre_reloc_only); > >>> > >>> So, the /firmware node seems special. Have you talked with the > >>> devicetree folks to get it listed in the spec? That would seem rather > >>> valuable for something used by many parties. Thanks! > >>> > >> > >> some days ago we have sent a patch for this too. > >> https://lists.denx.de/pipermail/u-boot/2018-August/338049.html > >> > >> It is using different way but it should do the same thing. > > > > OK, so it sounds like in terms of code clean-ups, we need something like > > what you reference and then some further clean-ups on top of that > > perhaps for other places to call dm_scan_fdt_ofnode_path() for special > > cases. And in terms of formalized specification bits, I do think > > /firmware should perhaps get kicked up to the spec itself so that it's > > very clear to all consumers. > > I was also checking latest devicetree spec and there is no record about > /firmware node and how it is supposed to be used. Patches welcome. :) It's really only a container to define non-discoverable firmware interfaces. Typically accessed thru a secure call (for ARM) or a mailbox. It is primarily just convention rather than a strict requirement. We have to support firmware nodes at the top-level too anyways. Rob
On Wed, Aug 15, 2018 at 09:31:30AM -0600, Rob Herring wrote: > On Wed, Aug 15, 2018 at 8:51 AM Michal Simek <michal.simek@xilinx.com> wrote: > > > > Hi Rob, > > > > On 15.8.2018 16:34, Tom Rini wrote: > > > On Wed, Aug 15, 2018 at 04:30:15PM +0200, Michal Simek wrote: > > >> On 15.8.2018 16:17, Tom Rini wrote: > > >>> On Mon, Aug 13, 2018 at 05:53:38PM +0200, Jens Wiklander wrote: > > >>> > > >>>> Just as /chosen may contain devices /firmware may contain devices, scan > > >>>> for devices under /firmware too. > > >>>> > > >>>> Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> > > >>>> --- > > >>>> drivers/core/root.c | 15 ++++++++++----- > > >>>> 1 file changed, 10 insertions(+), 5 deletions(-) > > >>>> > > >>>> diff --git a/drivers/core/root.c b/drivers/core/root.c > > >>>> index 72bcc7d7f2a3..0dca507e1187 100644 > > >>>> --- a/drivers/core/root.c > > >>>> +++ b/drivers/core/root.c > > >>>> @@ -265,9 +265,15 @@ static int dm_scan_fdt_node(struct udevice *parent, const void *blob, > > >>>> for (offset = fdt_first_subnode(blob, offset); > > >>>> offset > 0; > > >>>> offset = fdt_next_subnode(blob, offset)) { > > >>>> - /* "chosen" node isn't a device itself but may contain some: */ > > >>>> - if (!strcmp(fdt_get_name(blob, offset, NULL), "chosen")) { > > >>>> - pr_debug("parsing subnodes of \"chosen\"\n"); > > >>>> + const char *node_name = fdt_get_name(blob, offset, NULL); > > >>>> + > > >>>> + /* > > >>>> + * The "chosen" and "firmware" nodes aren't devices > > >>>> + * themselves but may contain some: > > >>>> + */ > > >>>> + if (!strcmp(node_name, "chosen") || > > >>>> + !strcmp(node_name, "firmware")) { > > >>>> + pr_debug("parsing subnodes of \"%s\"\n", node_name); > > >>>> > > >>>> err = dm_scan_fdt_node(parent, blob, offset, > > >>>> pre_reloc_only); > > >>> > > >>> So, the /firmware node seems special. Have you talked with the > > >>> devicetree folks to get it listed in the spec? That would seem rather > > >>> valuable for something used by many parties. Thanks! > > >>> > > >> > > >> some days ago we have sent a patch for this too. > > >> https://lists.denx.de/pipermail/u-boot/2018-August/338049.html > > >> > > >> It is using different way but it should do the same thing. > > > > > > OK, so it sounds like in terms of code clean-ups, we need something like > > > what you reference and then some further clean-ups on top of that > > > perhaps for other places to call dm_scan_fdt_ofnode_path() for special > > > cases. And in terms of formalized specification bits, I do think > > > /firmware should perhaps get kicked up to the spec itself so that it's > > > very clear to all consumers. > > > > I was also checking latest devicetree spec and there is no record about > > /firmware node and how it is supposed to be used. > > Patches welcome. :) > > It's really only a container to define non-discoverable firmware > interfaces. Typically accessed thru a secure call (for ARM) or a > mailbox. It is primarily just convention rather than a strict > requirement. We have to support firmware nodes at the top-level too > anyways. Right. To be clear, I'm suggesting that someone (I was thinking Jens) pop over to the devicetree-spec list and ask about adding _something_ there as "firmware" isn't even on the list of generic names and may even warrant something in section 3 under base device node types.
diff --git a/drivers/core/root.c b/drivers/core/root.c index 72bcc7d7f2a3..0dca507e1187 100644 --- a/drivers/core/root.c +++ b/drivers/core/root.c @@ -265,9 +265,15 @@ static int dm_scan_fdt_node(struct udevice *parent, const void *blob, for (offset = fdt_first_subnode(blob, offset); offset > 0; offset = fdt_next_subnode(blob, offset)) { - /* "chosen" node isn't a device itself but may contain some: */ - if (!strcmp(fdt_get_name(blob, offset, NULL), "chosen")) { - pr_debug("parsing subnodes of \"chosen\"\n"); + const char *node_name = fdt_get_name(blob, offset, NULL); + + /* + * The "chosen" and "firmware" nodes aren't devices + * themselves but may contain some: + */ + if (!strcmp(node_name, "chosen") || + !strcmp(node_name, "firmware")) { + pr_debug("parsing subnodes of \"%s\"\n", node_name); err = dm_scan_fdt_node(parent, blob, offset, pre_reloc_only); @@ -286,8 +292,7 @@ static int dm_scan_fdt_node(struct udevice *parent, const void *blob, err = lists_bind_fdt(parent, offset_to_ofnode(offset), NULL); if (err && !ret) { ret = err; - debug("%s: ret=%d\n", fdt_get_name(blob, offset, NULL), - ret); + debug("%s: ret=%d\n", node_name, ret); } }
Just as /chosen may contain devices /firmware may contain devices, scan for devices under /firmware too. Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org> --- drivers/core/root.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)