device tree variations

device tree variations

[Mike Ditto]

I'm building a kernel that can run on a handful of hardware
configurations, all using OF-unaware U-Boot.  I know how to make a
static device tree (dts file) that works on one of these hardware
variations, and how to add nodes and modify properties in the platform
init code.  But I don't see a nice way to deal with nodes that should be
present on only some hardware configurations.

I could have the dts file contain only the common elements, and add all
the variable elements in the startup code.  But that means the bulk of
the device tree will be expressed as relatively ugly C source instead of
the much more readable and maintainable dts notation.  I would much
rather have the dts file contain the union of all platforms and have the
platform init code delete the nodes that are not applicable, but I don't
see an API to do those deletions.

I suppose I could instead compile N different dts files and have the
platform init code pick the appropriate dtb blob to pass to fdt_init().

Any suggestions for the best way to do this?

					-=] Mike [=-

Re: device tree variations

[David Gibson]

On Thu, Oct 16, 2008 at 05:52:54PM -0700, Mike Ditto wrote:
> I'm building a kernel that can run on a handful of hardware
> configurations, all using OF-unaware U-Boot.  I know how to make a
> static device tree (dts file) that works on one of these hardware
> variations, and how to add nodes and modify properties in the platform
> init code.  But I don't see a nice way to deal with nodes that should be
> present on only some hardware configurations.
> 
> I could have the dts file contain only the common elements, and add all
> the variable elements in the startup code.  But that means the bulk of
> the device tree will be expressed as relatively ugly C source instead of
> the much more readable and maintainable dts notation.  I would much
> rather have the dts file contain the union of all platforms and have the
> platform init code delete the nodes that are not applicable, but I don't
> see an API to do those deletions.
> 
> I suppose I could instead compile N different dts files and have the
> platform init code pick the appropriate dtb blob to pass to
> fdt_init().

Deleting the irrelevant parts or picking a device tree to pass to
fdt_init() are both reasonable solutions.  libfdt which is included in
the bootwrapper has functions for removing unwanted nodes: either
fdt_nop_node() or fdt_del_node() will suffice.  There isn't currently
a dt_ops hook to call though to those functions though.  You could
either add one, or (knowing that your platform always has a flat dt)
bypass the dt_ops hooks and call libfdt directly.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: device tree variations

[Mike Ditto]

David Gibson wrote:
> Deleting the irrelevant parts or picking a device tree to pass to
> fdt_init() are both reasonable solutions.  libfdt which is included in
> the bootwrapper has functions for removing unwanted nodes: either
> fdt_nop_node() or fdt_del_node() will suffice.  There isn't currently
> a dt_ops hook to call though to those functions though.  You could
> either add one, or (knowing that your platform always has a flat dt)
> bypass the dt_ops hooks and call libfdt directly.

Thanks.  The fdt_del_node approach works pretty nicely.  I added a
dt_ops hook since fdt is static in libfdt-wrapper.c.

At first I tried fdt_nop_node, fearing that find_node_by_prop_value()
and fdt_del_node() would interact badly when deleting in mid-traversal,
but it turns out that fdt_nop_node() upsets find_node_by_prop_value()
anyway.  Plus, the kernel prints harmless but strange messages when
there are NOPs in the tree.  So I use fdt_del_node() and rescan from
the top each time I delete a node and it works fine.

	/* Find all product-dependent nodes and delete inapplicable ones. */
	dev = NULL;
	while ((dev = find_node_by_prop_value(dev,
					      "product-dependent",
					      "", 0)) != NULL) {
		u32 mask;
		int len;

		len = getprop(dev, "productmask", &mask, sizeof mask);
		if (len == sizeof mask) {
			if ((mask & (1 << product_id)) == 0) {
				del_node(dev);
				/* Have to restart from the beginning. */
				dev = NULL;
			}
		}
	}

I had to fix the memcmp bug I mentioned in an earlier posting to allow
searching for a boolean (empty) property.

If anyone cares, below is a trivial patch to expose the del_node()
operation via dt_ops.

Thanks again,
					-=] Mike [=-


Index: arch/powerpc/boot/ops.h
===================================================================
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 ops.h
--- arch/powerpc/boot/ops.h	11 Oct 2008 02:51:35 -0000	1.1.1.1
+++ arch/powerpc/boot/ops.h	18 Oct 2008 02:06:45 -0000
@@ -40,6 +40,7 @@
 			const int buflen);
 	int	(*setprop)(const void *phandle, const char *name,
 			const void *buf, const int buflen);
+	int (*del_node)(const void *phandle);
 	void *(*get_parent)(const void *phandle);
 	/* The node must not already exist. */
 	void *(*create_node)(const void *parent, const char *name);
@@ -124,6 +125,11 @@
 		return dt_ops.setprop(devp, name, buf, strlen(buf) + 1);

 	return -1;
+}
+
+static inline int del_node(const void *devp)
+{
+	return dt_ops.del_node ? dt_ops.del_node(devp) : -1;
 }

 static inline void *get_parent(const char *devp)
Index: arch/powerpc/boot/libfdt-wrapper.c
===================================================================
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 libfdt-wrapper.c
--- arch/powerpc/boot/libfdt-wrapper.c	11 Oct 2008 02:51:35 -0000	1.1.1.1
+++ arch/powerpc/boot/libfdt-wrapper.c	17 Oct 2008 22:08:44 -0000
@@ -105,6 +105,11 @@
 	return check_err(rc);
 }

+static int fdt_wrapper_del_node(const void *devp)
+{
+	return fdt_del_node(fdt, devp_offset(devp));
+}
+
 static void *fdt_wrapper_get_parent(const void *devp)
 {
 	return offset_devp(fdt_parent_offset(fdt, devp_offset(devp)));
@@ -173,6 +178,7 @@
 	dt_ops.create_node = fdt_wrapper_create_node;
 	dt_ops.find_node_by_prop_value = fdt_wrapper_find_node_by_prop_value;
 	dt_ops.find_node_by_compatible = fdt_wrapper_find_node_by_compatible;
+	dt_ops.del_node = fdt_wrapper_del_node;
 	dt_ops.get_path = fdt_wrapper_get_path;
 	dt_ops.finalize = fdt_wrapper_finalize;

Re: device tree variations

[David Gibson]

On Fri, Oct 17, 2008 at 07:14:05PM -0700, Mike Ditto wrote:
> David Gibson wrote:
> > Deleting the irrelevant parts or picking a device tree to pass to
> > fdt_init() are both reasonable solutions.  libfdt which is included in
> > the bootwrapper has functions for removing unwanted nodes: either
> > fdt_nop_node() or fdt_del_node() will suffice.  There isn't currently
> > a dt_ops hook to call though to those functions though.  You could
> > either add one, or (knowing that your platform always has a flat dt)
> > bypass the dt_ops hooks and call libfdt directly.
> 
> Thanks.  The fdt_del_node approach works pretty nicely.  I added a
> dt_ops hook since fdt is static in libfdt-wrapper.c.
> 
> At first I tried fdt_nop_node, fearing that find_node_by_prop_value()
> and fdt_del_node() would interact badly when deleting in
> mid-traversal,

Ah, yes, it would.

> but it turns out that fdt_nop_node() upsets find_node_by_prop_value()
> anyway.

Oh... it shouldn't, I think that's a bug.  I'll try to look into that
and fix it.

>  Plus, the kernel prints harmless but strange messages when
> there are NOPs in the tree.

We should probably fix that too.

>  So I use fdt_del_node() and rescan from
> the top each time I delete a node and it works fine.

Kind of ugly, but ok.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: device tree variations

[Benjamin Herrenschmidt]

> Thanks.  The fdt_del_node approach works pretty nicely.  I added a
> dt_ops hook since fdt is static in libfdt-wrapper.c.

 .../...

David says your patch is ok, However it's not in the right form. Could
you repost it please with a proper changeset comment and signed-off-by:
line ?

Thanks !

Cheers,
Ben.

[PATCH] powerpc: Add del_node function to allow early boot code to prune inapplicable devices.

[Mike Ditto]

Reposting in proper format...

Some platforms have variants that can share most of a flat device tree but need
a few devices selectively pruned at boot time.  This adds del_node() to ops.h
to allow access to the existing fdt_del_node().

Signed-off-by: Mike Ditto <mditto@consentry.com>
---

Index: arch/powerpc/boot/ops.h
===================================================================
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 ops.h
--- arch/powerpc/boot/ops.h	11 Oct 2008 02:51:35 -0000	1.1.1.1
+++ arch/powerpc/boot/ops.h	18 Oct 2008 02:06:45 -0000
@@ -40,6 +40,7 @@
 			const int buflen);
 	int	(*setprop)(const void *phandle, const char *name,
 			const void *buf, const int buflen);
+	int (*del_node)(const void *phandle);
 	void *(*get_parent)(const void *phandle);
 	/* The node must not already exist. */
 	void *(*create_node)(const void *parent, const char *name);
@@ -124,6 +125,11 @@
 		return dt_ops.setprop(devp, name, buf, strlen(buf) + 1);

 	return -1;
+}
+
+static inline int del_node(const void *devp)
+{
+	return dt_ops.del_node ? dt_ops.del_node(devp) : -1;
 }

 static inline void *get_parent(const char *devp)
Index: arch/powerpc/boot/libfdt-wrapper.c
===================================================================
retrieving revision 1.1.1.1
diff -u -r1.1.1.1 libfdt-wrapper.c
--- arch/powerpc/boot/libfdt-wrapper.c	11 Oct 2008 02:51:35 -0000	1.1.1.1
+++ arch/powerpc/boot/libfdt-wrapper.c	17 Oct 2008 22:08:44 -0000
@@ -105,6 +105,11 @@
 	return check_err(rc);
 }

+static int fdt_wrapper_del_node(const void *devp)
+{
+	return fdt_del_node(fdt, devp_offset(devp));
+}
+
 static void *fdt_wrapper_get_parent(const void *devp)
 {
 	return offset_devp(fdt_parent_offset(fdt, devp_offset(devp)));
@@ -173,6 +178,7 @@
 	dt_ops.create_node = fdt_wrapper_create_node;
 	dt_ops.find_node_by_prop_value = fdt_wrapper_find_node_by_prop_value;
 	dt_ops.find_node_by_compatible = fdt_wrapper_find_node_by_compatible;
+	dt_ops.del_node = fdt_wrapper_del_node;
 	dt_ops.get_path = fdt_wrapper_get_path;
 	dt_ops.finalize = fdt_wrapper_finalize;

Re: [PATCH] powerpc: Add del_node function to allow early boot code to prune inapplicable devices.

[Grant Likely]

On Tue, Oct 21, 2008 at 02:32:29PM -0700, Mike Ditto wrote:
> Reposting in proper format...
> 
> Some platforms have variants that can share most of a flat device tree but need
> a few devices selectively pruned at boot time.  This adds del_node() to ops.h
> to allow access to the existing fdt_del_node().
> 
> Signed-off-by: Mike Ditto <mditto@consentry.com>

Looks good to me.

Acked-by: Grant Likely <grant.likely@secretlab.ca>

> ---
> 
> Index: arch/powerpc/boot/ops.h
> ===================================================================
> retrieving revision 1.1.1.1
> diff -u -r1.1.1.1 ops.h
> --- arch/powerpc/boot/ops.h	11 Oct 2008 02:51:35 -0000	1.1.1.1
> +++ arch/powerpc/boot/ops.h	18 Oct 2008 02:06:45 -0000
> @@ -40,6 +40,7 @@
>  			const int buflen);
>  	int	(*setprop)(const void *phandle, const char *name,
>  			const void *buf, const int buflen);
> +	int (*del_node)(const void *phandle);
>  	void *(*get_parent)(const void *phandle);
>  	/* The node must not already exist. */
>  	void *(*create_node)(const void *parent, const char *name);
> @@ -124,6 +125,11 @@
>  		return dt_ops.setprop(devp, name, buf, strlen(buf) + 1);
> 
>  	return -1;
> +}
> +
> +static inline int del_node(const void *devp)
> +{
> +	return dt_ops.del_node ? dt_ops.del_node(devp) : -1;
>  }
> 
>  static inline void *get_parent(const char *devp)
> Index: arch/powerpc/boot/libfdt-wrapper.c
> ===================================================================
> retrieving revision 1.1.1.1
> diff -u -r1.1.1.1 libfdt-wrapper.c
> --- arch/powerpc/boot/libfdt-wrapper.c	11 Oct 2008 02:51:35 -0000	1.1.1.1
> +++ arch/powerpc/boot/libfdt-wrapper.c	17 Oct 2008 22:08:44 -0000
> @@ -105,6 +105,11 @@
>  	return check_err(rc);
>  }
> 
> +static int fdt_wrapper_del_node(const void *devp)
> +{
> +	return fdt_del_node(fdt, devp_offset(devp));
> +}
> +
>  static void *fdt_wrapper_get_parent(const void *devp)
>  {
>  	return offset_devp(fdt_parent_offset(fdt, devp_offset(devp)));
> @@ -173,6 +178,7 @@
>  	dt_ops.create_node = fdt_wrapper_create_node;
>  	dt_ops.find_node_by_prop_value = fdt_wrapper_find_node_by_prop_value;
>  	dt_ops.find_node_by_compatible = fdt_wrapper_find_node_by_compatible;
> +	dt_ops.del_node = fdt_wrapper_del_node;
>  	dt_ops.get_path = fdt_wrapper_get_path;
>  	dt_ops.finalize = fdt_wrapper_finalize;
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] powerpc: Add del_node function to allow early boot code to prune inapplicable devices.

[David Gibson]

On Tue, Oct 21, 2008 at 02:32:29PM -0700, Mike Ditto wrote:
> Reposting in proper format...
> 
> Some platforms have variants that can share most of a flat device tree but need
> a few devices selectively pruned at boot time.  This adds del_node() to ops.h
> to allow access to the existing fdt_del_node().
> 
> Signed-off-by: Mike Ditto <mditto@consentry.com>
Acked-by: David Gibson <david@gibson.dropbear.id.au>

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH] powerpc: Add del_node function to allow early boot code to prune inapplicable devices.

[Benjamin Herrenschmidt]

On Tue, 2008-10-21 at 17:33 -0600, Grant Likely wrote:
> On Tue, Oct 21, 2008 at 02:32:29PM -0700, Mike Ditto wrote:
> > Reposting in proper format...
> > 
> > Some platforms have variants that can share most of a flat device tree but need
> > a few devices selectively pruned at boot time.  This adds del_node() to ops.h
> > to allow access to the existing fdt_del_node().
> > 
> > Signed-off-by: Mike Ditto <mditto@consentry.com>
> 
> Looks good to me.
> 
> Acked-by: Grant Likely <grant.likely@secretlab.ca>

little nit: The patch doesn't quite have the right form.

Patches to the kernel are expected to apply with -p1, so
for example, the patch line contains:

arch/powerpc/boot/ops.h

It should be

something/arch/powerpc/boot/ops.h

I've fixed it up manually so no need to re-submit, but you'll know next
time :-)

Cheers,
Ben.

Re: [PATCH] powerpc: Add del_node function to allow early boot code to prune inapplicable devices.

[Mike Ditto]

Benjamin Herrenschmidt wrote:
> I've fixed it up manually so no need to re-submit, but you'll know next
> time :-)

Thanks!
					-=] Mike [=-