Patchwork [2/3] of/promtree: switch to building the DT using of_attach_node

login
register
mail settings
Submitter Andres Salomon
Date March 10, 2011, 12:16 a.m.
Message ID <1299716167-9656-3-git-send-email-dilinger@queued.net>
Download mbox | patch
Permalink /patch/86166/
State Awaiting Upstream
Delegated to: David Miller
Headers show

Comments

Andres Salomon - March 10, 2011, 12:16 a.m.
Use common functions (of_attach_node) to build the device tree.  This
allows us to drop a bit of code, and provides a more understandable
tree building.  It does change the order of the allnodes/allnext list;
the old tree looked something like this:

allnodes --> root (/)
             allnext ------> np1 (/pci)
                             allnext --> np2 (/pci/foo)
                                         allnext=NULL

Pretend that was a tree.  The new tree looks something like this:

allnodes     root (/)
      |      allnext=NULL    np1 (/pci)
      |        ^------------ allnext     np2 (/pci/foo)
      |                        ^-------- allnext
      |_____________________________________^

That shouldn't actually matter for anything, but it's worth noting in case
anyone notices odd behavior changes.  Plus, it's a good excuse to make
beautiful art.

Also note that this changes what gets passed to the of_pdt_build_more()
hook.  The only user of this hook is sparc's leon_kernel.c, which ends
up using it to call prom_amba_init.  However, prom_amba_init isn't
actually set in the kernel, so I can't tell whether this actually breaks
behavior or not.

Signed-off-by: Andres Salomon <dilinger@queued.net>
---
 drivers/of/pdt.c |   39 ++++++++++++---------------------------
 1 files changed, 12 insertions(+), 27 deletions(-)
Grant Likely - March 12, 2011, 9 a.m.
On Wed, Mar 09, 2011 at 04:16:06PM -0800, Andres Salomon wrote:
> Use common functions (of_attach_node) to build the device tree.  This
> allows us to drop a bit of code, and provides a more understandable
> tree building.  It does change the order of the allnodes/allnext list;
> the old tree looked something like this:
> 
> allnodes --> root (/)
>              allnext ------> np1 (/pci)
>                              allnext --> np2 (/pci/foo)
>                                          allnext=NULL
> 
> Pretend that was a tree.  The new tree looks something like this:
> 
> allnodes     root (/)
>       |      allnext=NULL    np1 (/pci)
>       |        ^------------ allnext     np2 (/pci/foo)
>       |                        ^-------- allnext
>       |_____________________________________^
> 
> That shouldn't actually matter for anything, but it's worth noting in case
> anyone notices odd behavior changes.  Plus, it's a good excuse to make
> beautiful art.

Hmmm, I hadn't thought of that detail.  This will very likely cause
breakage in subtle ways.  Though we *shouldn't* depend on the order of
the device nodes in the tree, I know for certain that there are some
platform which will end up renumbering devices if the order is in
reverse (uarts in particular) in a way that breaks booting.  Ugh.

Now, this might not be a problem at all for SPARC, and this patch only
affects OLPC and SPARC at the moment.  If David ack's it, then I'll
pick it up.  However, it will require some more thought before
unflatten_device_tree() can be converted to use it.

g.

> 
> Also note that this changes what gets passed to the of_pdt_build_more()
> hook.  The only user of this hook is sparc's leon_kernel.c, which ends
> up using it to call prom_amba_init.  However, prom_amba_init isn't
> actually set in the kernel, so I can't tell whether this actually breaks
> behavior or not.
> 
> Signed-off-by: Andres Salomon <dilinger@queued.net>
> ---
>  drivers/of/pdt.c |   39 ++++++++++++---------------------------
>  1 files changed, 12 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
> index 4d87b5d..5933127 100644
> --- a/drivers/of/pdt.c
> +++ b/drivers/of/pdt.c
> @@ -193,56 +193,41 @@ static struct device_node * __init of_pdt_create_node(phandle node,
>  	return dp;
>  }
>  
> -static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
> -						   phandle node,
> -						   struct device_node ***nextp)
> +static void __init of_pdt_build_tree(struct device_node *parent, phandle node)
>  {
> -	struct device_node *ret = NULL, *prev_sibling = NULL;
>  	struct device_node *dp;
>  
> -	while (1) {
> +	while (node) {
>  		dp = of_pdt_create_node(node, parent);
>  		if (!dp)
>  			break;
>  
> -		if (prev_sibling)
> -			prev_sibling->sibling = dp;
> -
> -		if (!ret)
> -			ret = dp;
> -		prev_sibling = dp;
> -
> -		*(*nextp) = dp;
> -		*nextp = &dp->allnext;
> -
>  		dp->full_name = of_pdt_build_full_name(dp);
> +		of_attach_node(dp);
>  
> -		dp->child = of_pdt_build_tree(dp,
> -				of_pdt_prom_ops->getchild(node), nextp);
> +		/* process child nodes prior to siblings */
> +		of_pdt_build_tree(dp, of_pdt_prom_ops->getchild(node));
>  
>  		if (of_pdt_build_more)
> -			of_pdt_build_more(dp, nextp);
> +			of_pdt_build_more(dp, NULL);
>  
>  		node = of_pdt_prom_ops->getsibling(node);
>  	}
> -
> -	return ret;
>  }
>  
>  void __init of_pdt_build_devicetree(phandle root_node, struct of_pdt_ops *ops)
>  {
> -	struct device_node **nextp;
> +	struct device_node *dp;
>  
>  	BUG_ON(!ops);
>  	of_pdt_prom_ops = ops;
>  
> -	allnodes = of_pdt_create_node(root_node, NULL);
> +	dp = of_pdt_create_node(root_node, NULL);
>  #if defined(CONFIG_SPARC)
> -	allnodes->path_component_name = "";
> +	dp->path_component_name = "";
>  #endif
> -	allnodes->full_name = "/";
> +	dp->full_name = "/";
>  
> -	nextp = &allnodes->allnext;
> -	allnodes->child = of_pdt_build_tree(allnodes,
> -			of_pdt_prom_ops->getchild(allnodes->phandle), &nextp);
> +	of_attach_node(dp);
> +	of_pdt_build_tree(dp, of_pdt_prom_ops->getchild(dp->phandle));
>  }
> -- 
> 1.7.2.3
> 
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/of/pdt.c b/drivers/of/pdt.c
index 4d87b5d..5933127 100644
--- a/drivers/of/pdt.c
+++ b/drivers/of/pdt.c
@@ -193,56 +193,41 @@  static struct device_node * __init of_pdt_create_node(phandle node,
 	return dp;
 }
 
-static struct device_node * __init of_pdt_build_tree(struct device_node *parent,
-						   phandle node,
-						   struct device_node ***nextp)
+static void __init of_pdt_build_tree(struct device_node *parent, phandle node)
 {
-	struct device_node *ret = NULL, *prev_sibling = NULL;
 	struct device_node *dp;
 
-	while (1) {
+	while (node) {
 		dp = of_pdt_create_node(node, parent);
 		if (!dp)
 			break;
 
-		if (prev_sibling)
-			prev_sibling->sibling = dp;
-
-		if (!ret)
-			ret = dp;
-		prev_sibling = dp;
-
-		*(*nextp) = dp;
-		*nextp = &dp->allnext;
-
 		dp->full_name = of_pdt_build_full_name(dp);
+		of_attach_node(dp);
 
-		dp->child = of_pdt_build_tree(dp,
-				of_pdt_prom_ops->getchild(node), nextp);
+		/* process child nodes prior to siblings */
+		of_pdt_build_tree(dp, of_pdt_prom_ops->getchild(node));
 
 		if (of_pdt_build_more)
-			of_pdt_build_more(dp, nextp);
+			of_pdt_build_more(dp, NULL);
 
 		node = of_pdt_prom_ops->getsibling(node);
 	}
-
-	return ret;
 }
 
 void __init of_pdt_build_devicetree(phandle root_node, struct of_pdt_ops *ops)
 {
-	struct device_node **nextp;
+	struct device_node *dp;
 
 	BUG_ON(!ops);
 	of_pdt_prom_ops = ops;
 
-	allnodes = of_pdt_create_node(root_node, NULL);
+	dp = of_pdt_create_node(root_node, NULL);
 #if defined(CONFIG_SPARC)
-	allnodes->path_component_name = "";
+	dp->path_component_name = "";
 #endif
-	allnodes->full_name = "/";
+	dp->full_name = "/";
 
-	nextp = &allnodes->allnext;
-	allnodes->child = of_pdt_build_tree(allnodes,
-			of_pdt_prom_ops->getchild(allnodes->phandle), &nextp);
+	of_attach_node(dp);
+	of_pdt_build_tree(dp, of_pdt_prom_ops->getchild(dp->phandle));
 }