| Message ID | 20180813155347.13844-2-jens.wiklander@linaro.org |
|---|---|
| State | Superseded |
| 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! -- Tom
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. -- Tom
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. -- Tom
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(-) -- 2.17.1