[v2,01/15] dm: fdt: scan for devices under /firmware too

Message ID 20180823104334.16083-2-jens.wiklander@linaro.org
State Superseded
Headers show
Series
  • AVB using OP-TEE
Related show

Commit Message

Jens Wiklander Aug. 23, 2018, 10:43 a.m.
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

Comments

Simon Glass Aug. 30, 2018, 12:28 a.m. | #1
On 23 August 2018 at 04:43, Jens Wiklander <jens.wiklander@linaro.org> 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(-)

Reviewed-by: Simon Glass <sjg@chromium.org>
Michal Simek Jan. 7, 2020, 8:49 a.m. | #2
Hi Jens and Simon,

čt 23. 8. 2018 v 12:43 odesílatel Jens Wiklander
<jens.wiklander at linaro.org> napsal:
>
> Just as /chosen may contain devices /firmware may contain devices, scan
> for devices under /firmware too.
>
> Signed-off-by: Jens Wiklander <jens.wiklander at 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);
> @@ -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);
>                 }
>         }
>
> --
> 2.17.1
>

I have debug issue which I see on ZynqMP that firmware nodes are
listed in dm tree twice.
I was caused by this patch.

 firmware      1  [ + ]   zynqmp-firmware       |-- zynqmp-firmware
 firmware      2  [ + ]   zynqmp-power          |   `-- zynqmp-power
....
 firmware      3  [   ]   zynqmp-firmware       `-- zynqmp-firmware
 firmware      4  [   ]   zynqmp-power              `-- zynqmp-power

On ZynqMP firmware node needs to be populated early that's why
u-boot,dm-pre-reloc flag is used.
That's why I am curious how to fix this. Revert this patch or add
flags to Jens case or do more checking to avoid creating duplicated
entries of nodes present in firmware node.

Thanks,
Michal
Jens Wiklander Jan. 8, 2020, 9:44 a.m. | #3
Hi Michal,

On Tue, Jan 7, 2020 at 9:49 AM Michal Simek <monstr at monstr.eu> wrote:
>
> Hi Jens and Simon,
>
> čt 23. 8. 2018 v 12:43 odesílatel Jens Wiklander
> <jens.wiklander at linaro.org> napsal:
> >
> > Just as /chosen may contain devices /firmware may contain devices, scan
> > for devices under /firmware too.
> >
> > Signed-off-by: Jens Wiklander <jens.wiklander at 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);
> > @@ -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);
> >                 }
> >         }
> >
> > --
> > 2.17.1
> >
>
> I have debug issue which I see on ZynqMP that firmware nodes are
> listed in dm tree twice.
> I was caused by this patch.
>
>  firmware      1  [ + ]   zynqmp-firmware       |-- zynqmp-firmware
>  firmware      2  [ + ]   zynqmp-power          |   `-- zynqmp-power
> ....
>  firmware      3  [   ]   zynqmp-firmware       `-- zynqmp-firmware
>  firmware      4  [   ]   zynqmp-power              `-- zynqmp-power
>
> On ZynqMP firmware node needs to be populated early that's why
> u-boot,dm-pre-reloc flag is used.
> That's why I am curious how to fix this. Revert this patch or add
> flags to Jens case or do more checking to avoid creating duplicated
> entries of nodes present in firmware node.

Reverting may solve the ZynqMP problem, but it will cause new problems
instead so I'd prefer fixing it properly.
I'm not sure if this is best fixed by more flags or by extra checking
when creating the nodes.

Thanks,
Jens

>
> Thanks,
> Michal
>
> --
> Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
> w: www.monstr.eu p: +42-0-721842854
> Maintainer of Linux kernel - Xilinx Microblaze
> Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP ARM64 SoCs
> U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs

Patch

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);
 		}
 	}