diff mbox series

[v2] dm: core: Move "/chosen" and "/firmware" node scan

Message ID 20200213184800.13968-1-patrick.delaunay@st.com
State Superseded
Headers show
Series [v2] dm: core: Move "/chosen" and "/firmware" node scan | expand

Commit Message

Patrick Delaunay Feb. 13, 2020, 6:48 p.m. UTC
Use the new function dm_scan_fdt_ofnode_path() to scan all the nodes
which aren't devices themselves but may contain some:
- "/chosen"
- "/clocks"
- "/firmware"

The patch removes the strcmp call in recursive function dm_scan_fdt_live()
and also corrects a conflict with the 2 applied patches in
the commit 1712ca21924b ("dm: core: Scan /firmware node by default")
and in the commit 747558d01457 ("dm: fdt: scan for devices under
/firmware too"): the subnodes of "/firmware" (optee for example)
are bound 2 times.

For example the dm tree command result on STM32MP1 is:

STM32MP> dm tree
 Class     Index  Probed  Driver                Name
 -----------------------------------------------------------
 root          0  [ + ]   root_driver           root_driver
 firmware      0  [   ]   psci                  |-- psci
 sysreset      0  [   ]   psci-sysreset         |   `-- psci-sysreset
 simple_bus    0  [ + ]   generic_simple_bus    |-- soc
...
 tee           0  [ + ]   optee                 |-- optee
...
 tee           1  [   ]   optee                 `-- optee

Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com>
---

Changes in v2:
- update commit message (Serie-cc => Series-cc)

 drivers/core/root.c | 52 +++++++++++++++------------------------------
 1 file changed, 17 insertions(+), 35 deletions(-)

Comments

Michal Simek Feb. 14, 2020, 8:46 a.m. UTC | #1
?t 13. 2. 2020 v 19:48 odes?latel Patrick Delaunay
<patrick.delaunay at st.com> napsal:
>
> Use the new function dm_scan_fdt_ofnode_path() to scan all the nodes
> which aren't devices themselves but may contain some:
> - "/chosen"
> - "/clocks"
> - "/firmware"
>
> The patch removes the strcmp call in recursive function dm_scan_fdt_live()
> and also corrects a conflict with the 2 applied patches in
> the commit 1712ca21924b ("dm: core: Scan /firmware node by default")
> and in the commit 747558d01457 ("dm: fdt: scan for devices under
> /firmware too"): the subnodes of "/firmware" (optee for example)
> are bound 2 times.
>
> For example the dm tree command result on STM32MP1 is:
>
> STM32MP> dm tree
>  Class     Index  Probed  Driver                Name
>  -----------------------------------------------------------
>  root          0  [ + ]   root_driver           root_driver
>  firmware      0  [   ]   psci                  |-- psci
>  sysreset      0  [   ]   psci-sysreset         |   `-- psci-sysreset
>  simple_bus    0  [ + ]   generic_simple_bus    |-- soc
> ...
>  tee           0  [ + ]   optee                 |-- optee
> ...
>  tee           1  [   ]   optee                 `-- optee
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com>
> ---
>
> Changes in v2:
> - update commit message (Serie-cc => Series-cc)
>
>  drivers/core/root.c | 52 +++++++++++++++------------------------------
>  1 file changed, 17 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/core/root.c b/drivers/core/root.c
> index e85643819e..14df16c280 100644
> --- a/drivers/core/root.c
> +++ b/drivers/core/root.c
> @@ -203,15 +203,6 @@ static int dm_scan_fdt_live(struct udevice *parent,
>         int ret = 0, err;
>
>         for (np = node_parent->child; np; np = np->sibling) {
> -               /* "chosen" node isn't a device itself but may contain some: */
> -               if (!strcmp(np->name, "chosen")) {
> -                       pr_debug("parsing subnodes of \"chosen\"\n");
> -
> -                       err = dm_scan_fdt_live(parent, np, pre_reloc_only);
> -                       if (err && !ret)
> -                               ret = err;
> -                       continue;
> -               }
>
>                 if (!of_device_is_available(np)) {
>                         pr_debug("   - ignoring disabled device\n");
> @@ -256,21 +247,6 @@ static int dm_scan_fdt_node(struct udevice *parent, const void *blob,
>              offset = fdt_next_subnode(blob, offset)) {
>                 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);
> -                       if (err && !ret)
> -                               ret = err;
> -                       continue;
> -               }
> -
>                 if (!fdtdec_get_is_enabled(blob, offset)) {
>                         pr_debug("   - ignoring disabled device\n");
>                         continue;
> @@ -315,7 +291,8 @@ int dm_scan_fdt(const void *blob, bool pre_reloc_only)
>         return dm_scan_fdt_node(gd->dm_root, blob, 0, pre_reloc_only);
>  }
>
> -static int dm_scan_fdt_ofnode_path(const char *path, bool pre_reloc_only)
> +static int dm_scan_fdt_ofnode_path(const void *blob, const char *path,
> +                                  bool pre_reloc_only)
>  {
>         ofnode node;
>
> @@ -327,13 +304,18 @@ static int dm_scan_fdt_ofnode_path(const char *path, bool pre_reloc_only)
>         if (of_live_active())
>                 return dm_scan_fdt_live(gd->dm_root, node.np, pre_reloc_only);
>  #endif
> -       return dm_scan_fdt_node(gd->dm_root, gd->fdt_blob, node.of_offset,
> +       return dm_scan_fdt_node(gd->dm_root, blob, node.of_offset,
>                                 pre_reloc_only);
>  }
>
>  int dm_extended_scan_fdt(const void *blob, bool pre_reloc_only)
>  {
> -       int ret;
> +       int ret, i;
> +       const char * const nodes[] = {
> +               "/chosen",
> +               "/clocks",
> +               "/firmware"
> +       };
>
>         ret = dm_scan_fdt(blob, pre_reloc_only);
>         if (ret) {
> @@ -341,16 +323,16 @@ int dm_extended_scan_fdt(const void *blob, bool pre_reloc_only)
>                 return ret;
>         }
>
> -       ret = dm_scan_fdt_ofnode_path("/clocks", pre_reloc_only);
> -       if (ret) {
> -               debug("scan for /clocks failed: %d\n", ret);
> -               return ret;
> +       /* Some nodes aren't devices themselves but may contain some */
> +       for (i = 0; i < ARRAY_SIZE(nodes); i++) {
> +               ret = dm_scan_fdt_ofnode_path(blob, nodes[i], pre_reloc_only);
> +               if (ret) {
> +                       debug("dm_scan_fdt() scan for %s failed: %d\n",
> +                             nodes[i], ret);
> +                       return ret;
> +               }
>         }
>
> -       ret = dm_scan_fdt_ofnode_path("/firmware", pre_reloc_only);
> -       if (ret)
> -               debug("scan for /firmware failed: %d\n", ret);
> -
>         return ret;
>  }
>  #endif
> --
> 2.17.1
>

It looks good to me. I have reported here.
https://lists.denx.de/pipermail/u-boot/2020-January/395427.html

Tested-by: Michal Simek <michal.simek at xilinx.com>

Thanks,
Michal
Simon Glass Feb. 17, 2020, 3:55 a.m. UTC | #2
Hi Patrick,

On Thu, 13 Feb 2020 at 11:48, Patrick Delaunay <patrick.delaunay at st.com> wrote:
>
> Use the new function dm_scan_fdt_ofnode_path() to scan all the nodes
> which aren't devices themselves but may contain some:
> - "/chosen"
> - "/clocks"
> - "/firmware"
>
> The patch removes the strcmp call in recursive function dm_scan_fdt_live()
> and also corrects a conflict with the 2 applied patches in
> the commit 1712ca21924b ("dm: core: Scan /firmware node by default")
> and in the commit 747558d01457 ("dm: fdt: scan for devices under
> /firmware too"): the subnodes of "/firmware" (optee for example)
> are bound 2 times.
>
> For example the dm tree command result on STM32MP1 is:
>
> STM32MP> dm tree
>  Class     Index  Probed  Driver                Name
>  -----------------------------------------------------------
>  root          0  [ + ]   root_driver           root_driver
>  firmware      0  [   ]   psci                  |-- psci
>  sysreset      0  [   ]   psci-sysreset         |   `-- psci-sysreset
>  simple_bus    0  [ + ]   generic_simple_bus    |-- soc
> ...
>  tee           0  [ + ]   optee                 |-- optee
> ...
>  tee           1  [   ]   optee                 `-- optee
>
> Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com>
> ---
>
> Changes in v2:
> - update commit message (Serie-cc => Series-cc)
>
>  drivers/core/root.c | 52 +++++++++++++++------------------------------
>  1 file changed, 17 insertions(+), 35 deletions(-)


This looks good to me, but please can you address the test failure
(make qcheck)?

Regards,
Simon
Patrick Delaunay Feb. 18, 2020, 1:21 p.m. UTC | #3
Hi,

> From: Simon Glass <sjg at chromium.org>
> Sent: lundi 17 f?vrier 2020 04:56
> 
> Hi Patrick,
> 
> On Thu, 13 Feb 2020 at 11:48, Patrick Delaunay <patrick.delaunay at st.com>
> wrote:
> >
> > Use the new function dm_scan_fdt_ofnode_path() to scan all the nodes
> > which aren't devices themselves but may contain some:
> > - "/chosen"
> > - "/clocks"
> > - "/firmware"
> >
> > The patch removes the strcmp call in recursive function
> > dm_scan_fdt_live() and also corrects a conflict with the 2 applied
> > patches in the commit 1712ca21924b ("dm: core: Scan /firmware node by
> > default") and in the commit 747558d01457 ("dm: fdt: scan for devices
> > under /firmware too"): the subnodes of "/firmware" (optee for example)
> > are bound 2 times.
> >
> > For example the dm tree command result on STM32MP1 is:
> >
> > STM32MP> dm tree
> >  Class     Index  Probed  Driver                Name
> >  -----------------------------------------------------------
> >  root          0  [ + ]   root_driver           root_driver
> >  firmware      0  [   ]   psci                  |-- psci
> >  sysreset      0  [   ]   psci-sysreset         |   `-- psci-sysreset
> >  simple_bus    0  [ + ]   generic_simple_bus    |-- soc
> > ...
> >  tee           0  [ + ]   optee                 |-- optee
> > ...
> >  tee           1  [   ]   optee                 `-- optee
> >
> > Signed-off-by: Patrick Delaunay <patrick.delaunay at st.com>
> > ---
> >
> > Changes in v2:
> > - update commit message (Serie-cc => Series-cc)
> >
> >  drivers/core/root.c | 52
> > +++++++++++++++------------------------------
> >  1 file changed, 17 insertions(+), 35 deletions(-)
> 
> 
> This looks good to me, but please can you address the test failure (make
> qcheck)?

I forget to execute it, sorry.

Work in progress....

 
> Regards,
> Simon

Patrick
diff mbox series

Patch

diff --git a/drivers/core/root.c b/drivers/core/root.c
index e85643819e..14df16c280 100644
--- a/drivers/core/root.c
+++ b/drivers/core/root.c
@@ -203,15 +203,6 @@  static int dm_scan_fdt_live(struct udevice *parent,
 	int ret = 0, err;
 
 	for (np = node_parent->child; np; np = np->sibling) {
-		/* "chosen" node isn't a device itself but may contain some: */
-		if (!strcmp(np->name, "chosen")) {
-			pr_debug("parsing subnodes of \"chosen\"\n");
-
-			err = dm_scan_fdt_live(parent, np, pre_reloc_only);
-			if (err && !ret)
-				ret = err;
-			continue;
-		}
 
 		if (!of_device_is_available(np)) {
 			pr_debug("   - ignoring disabled device\n");
@@ -256,21 +247,6 @@  static int dm_scan_fdt_node(struct udevice *parent, const void *blob,
 	     offset = fdt_next_subnode(blob, offset)) {
 		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);
-			if (err && !ret)
-				ret = err;
-			continue;
-		}
-
 		if (!fdtdec_get_is_enabled(blob, offset)) {
 			pr_debug("   - ignoring disabled device\n");
 			continue;
@@ -315,7 +291,8 @@  int dm_scan_fdt(const void *blob, bool pre_reloc_only)
 	return dm_scan_fdt_node(gd->dm_root, blob, 0, pre_reloc_only);
 }
 
-static int dm_scan_fdt_ofnode_path(const char *path, bool pre_reloc_only)
+static int dm_scan_fdt_ofnode_path(const void *blob, const char *path,
+				   bool pre_reloc_only)
 {
 	ofnode node;
 
@@ -327,13 +304,18 @@  static int dm_scan_fdt_ofnode_path(const char *path, bool pre_reloc_only)
 	if (of_live_active())
 		return dm_scan_fdt_live(gd->dm_root, node.np, pre_reloc_only);
 #endif
-	return dm_scan_fdt_node(gd->dm_root, gd->fdt_blob, node.of_offset,
+	return dm_scan_fdt_node(gd->dm_root, blob, node.of_offset,
 				pre_reloc_only);
 }
 
 int dm_extended_scan_fdt(const void *blob, bool pre_reloc_only)
 {
-	int ret;
+	int ret, i;
+	const char * const nodes[] = {
+		"/chosen",
+		"/clocks",
+		"/firmware"
+	};
 
 	ret = dm_scan_fdt(blob, pre_reloc_only);
 	if (ret) {
@@ -341,16 +323,16 @@  int dm_extended_scan_fdt(const void *blob, bool pre_reloc_only)
 		return ret;
 	}
 
-	ret = dm_scan_fdt_ofnode_path("/clocks", pre_reloc_only);
-	if (ret) {
-		debug("scan for /clocks failed: %d\n", ret);
-		return ret;
+	/* Some nodes aren't devices themselves but may contain some */
+	for (i = 0; i < ARRAY_SIZE(nodes); i++) {
+		ret = dm_scan_fdt_ofnode_path(blob, nodes[i], pre_reloc_only);
+		if (ret) {
+			debug("dm_scan_fdt() scan for %s failed: %d\n",
+			      nodes[i], ret);
+			return ret;
+		}
 	}
 
-	ret = dm_scan_fdt_ofnode_path("/firmware", pre_reloc_only);
-	if (ret)
-		debug("scan for /firmware failed: %d\n", ret);
-
 	return ret;
 }
 #endif