[RFC 2/3] OLPC: add missing elements to device tree

[RFC 2/3] OLPC: add missing elements to device tree

[Daniel Drake]

In response to new device tree code in the kernel, OLPC will start
using it for probing of certain devices. However, some firmware fixes
are needed to put the devicetree into a usable state.

Retain compatibility with old firmware by fixing up the device tree
at boot-time if it does not contain the new nodes/properties that
we need for probing.

Signed-off-by: Daniel Drake <dsd-2X9k7bc8m7Mdnm+yROfE0A@public.gmane.org>
---
 arch/x86/platform/olpc/olpc_dt.c |   87 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 87 insertions(+), 0 deletions(-)

diff --git a/arch/x86/platform/olpc/olpc_dt.c b/arch/x86/platform/olpc/olpc_dt.c
index 09cbede..58bba78 100644
--- a/arch/x86/platform/olpc/olpc_dt.c
+++ b/arch/x86/platform/olpc/olpc_dt.c
@@ -22,6 +22,7 @@
 #include <linux/of_platform.h>
 #include <linux/of_pdt.h>
 #include <asm/olpc_ofw.h>
+#include <asm/olpc.h>
 
 static phandle __init olpc_dt_getsibling(phandle node)
 {
@@ -164,6 +165,91 @@ static struct of_pdt_ops prom_olpc_ops __initdata = {
 	.pkg2path = olpc_dt_pkg2path,
 };
 
+static char __init *prom_alloc_string(const char *str, int *len)
+{
+	int _len = strlen(str);
+	char *output = prom_early_alloc(_len + 1);
+
+	strcpy(output, str);
+	if (len)
+		*len = _len;
+	return output;
+}
+
+static void __init prom_alloc_property(struct device_node *node,
+	const char *name, const char *value)
+{
+	struct property *p = prom_early_alloc(sizeof(struct property));
+
+	p->name = prom_alloc_string(name, NULL);
+	p->value = prom_alloc_string(value, &p->length);
+	prom_add_property(node, p);
+}
+
+/* Add dcon device as child of display */
+static void __init add_dcon_node(struct device_node *display)
+{
+	struct device_node *node = prom_early_alloc(sizeof(struct device_node));
+	unsigned long flags;
+
+	kref_init(&node->kref);
+	node->name = prom_alloc_string("dcon", NULL);
+	node->full_name = (char *) node->name;
+	node->parent = display;
+
+	write_lock_irqsave(&devtree_lock, flags);
+	node->sibling = node->parent->child;
+	node->allnext = allnodes;
+	node->parent->child = node;
+	allnodes = node;
+	write_unlock_irqrestore(&devtree_lock, flags);
+
+	prom_alloc_property(node, "compatible", "olpc,xo1-dcon");
+}
+
+static void __init olpc_dt_fixup(void)
+{
+	struct device_node *node;
+
+	/*
+	 * Use battery's compatible property to determine if we're running a
+	 * new-enough firmware. If we have this property, no fixups are needed.
+	 */
+	node = of_find_node_by_name(NULL, "battery");
+	if (node && of_get_property(node, "compatible", NULL)) {
+		of_node_put(node);
+		return;
+	}
+
+	/*
+	 * Add "compatible" property to battery, it was missing from earlier
+	 * firmware releases.
+	 */
+	prom_alloc_property(node, "compatible", "olpc,xo1-battery");
+	of_node_put(node);
+
+	/*
+	 * Mark XO-1 RTC as compatible with olpc,xo1-rtc - this was not done in
+	 * earlier firmware releases.
+	 */
+	node = of_find_node_by_name(NULL, "rtc");
+	if (olpc_platform_info.boardrev < olpc_board_pre(0xd0) && node) {
+		struct property *p = of_get_property(node, "compatible", NULL);
+		prom_remove_property(node, p);
+		prom_alloc_property(node, "compatible", "olpc,xo1-rtc");
+	}
+	of_node_put(node);
+
+	/*
+	 * Add "dcon" device node, it was missing from earlier firmware
+	 * releases.
+	 */
+	node = of_find_node_by_name(NULL, "display");
+	if (node)
+		add_dcon_node(node);
+	of_node_put(node);
+}
+
 void __init olpc_dt_build_devicetree(void)
 {
 	phandle root;
@@ -177,6 +263,7 @@ void __init olpc_dt_build_devicetree(void)
 		return;
 	}
 	of_pdt_build_devicetree(root, &prom_olpc_ops);
+	olpc_dt_fixup();
 
 	pr_info("PROM DT: Built device tree with %u bytes of memory.\n",
 			prom_early_allocated);
-- 
1.7.4

Re: [RFC 2/3] OLPC: add missing elements to device tree

[Grant Likely]

On Fri, Mar 04, 2011 at 05:12:34PM +0000, Daniel Drake wrote:
> In response to new device tree code in the kernel, OLPC will start
> using it for probing of certain devices. However, some firmware fixes
> are needed to put the devicetree into a usable state.
> 
> Retain compatibility with old firmware by fixing up the device tree
> at boot-time if it does not contain the new nodes/properties that
> we need for probing.
> 
> Signed-off-by: Daniel Drake <dsd-2X9k7bc8m7Mdnm+yROfE0A@public.gmane.org>

Hi Daniel,

The concept looks good. here are a few comments on the
implementation.

g.

> ---
>  arch/x86/platform/olpc/olpc_dt.c |   87 ++++++++++++++++++++++++++++++++++++++
>  1 files changed, 87 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/x86/platform/olpc/olpc_dt.c b/arch/x86/platform/olpc/olpc_dt.c
> index 09cbede..58bba78 100644
> --- a/arch/x86/platform/olpc/olpc_dt.c
> +++ b/arch/x86/platform/olpc/olpc_dt.c
> @@ -22,6 +22,7 @@
>  #include <linux/of_platform.h>
>  #include <linux/of_pdt.h>
>  #include <asm/olpc_ofw.h>
> +#include <asm/olpc.h>
>  
>  static phandle __init olpc_dt_getsibling(phandle node)
>  {
> @@ -164,6 +165,91 @@ static struct of_pdt_ops prom_olpc_ops __initdata = {
>  	.pkg2path = olpc_dt_pkg2path,
>  };
>  
> +static char __init *prom_alloc_string(const char *str, int *len)
> +{
> +	int _len = strlen(str);
> +	char *output = prom_early_alloc(_len + 1);
> +
> +	strcpy(output, str);
> +	if (len)
> +		*len = _len;
> +	return output;
> +}
> +
> +static void __init prom_alloc_property(struct device_node *node,
> +	const char *name, const char *value)
> +{
> +	struct property *p = prom_early_alloc(sizeof(struct property));

How early is this running?  If the allocator is set up by now, then
this should use kzalloc()...

> +
> +	p->name = prom_alloc_string(name, NULL);
> +	p->value = prom_alloc_string(value, &p->length);

... which would also mean that kstrdup()/kmemdup() can be used here
directly.

> +	prom_add_property(node, p);
> +}

This would be useful in common code.  prom_create_property() might
be a better name.  It should also accept a length argument for the
size of the data.

> +
> +/* Add dcon device as child of display */
> +static void __init add_dcon_node(struct device_node *display)
> +{
> +	struct device_node *node = prom_early_alloc(sizeof(struct device_node));
> +	unsigned long flags;
> +
> +	kref_init(&node->kref);
> +	node->name = prom_alloc_string("dcon", NULL);
> +	node->full_name = (char *) node->name;
> +	node->parent = display;
> +
> +	write_lock_irqsave(&devtree_lock, flags);
> +	node->sibling = node->parent->child;
> +	node->allnext = allnodes;
> +	node->parent->child = node;
> +	allnodes = node;
> +	write_unlock_irqrestore(&devtree_lock, flags);
> +

This should be using the existing of_attach_node() function to link
the node into the tree (in fact I know you've seen it because the
above 6 lines are identical).  :-)  Also, there already exists a function
"new_node" in arch/powerpc/platforms/iseries/vio.c that would be
useful to move into drivers/of/base.c

> +	prom_alloc_property(node, "compatible", "olpc,xo1-dcon");
> +}
> +
> +static void __init olpc_dt_fixup(void)
> +{
> +	struct device_node *node;
> +
> +	/*
> +	 * Use battery's compatible property to determine if we're running a
> +	 * new-enough firmware. If we have this property, no fixups are needed.
> +	 */
> +	node = of_find_node_by_name(NULL, "battery");
> +	if (node && of_get_property(node, "compatible", NULL)) {
> +		of_node_put(node);
> +		return;
> +	}
> +
> +	/*
> +	 * Add "compatible" property to battery, it was missing from earlier
> +	 * firmware releases.
> +	 */
> +	prom_alloc_property(node, "compatible", "olpc,xo1-battery");
> +	of_node_put(node);
> +
> +	/*
> +	 * Mark XO-1 RTC as compatible with olpc,xo1-rtc - this was not done in
> +	 * earlier firmware releases.
> +	 */
> +	node = of_find_node_by_name(NULL, "rtc");
> +	if (olpc_platform_info.boardrev < olpc_board_pre(0xd0) && node) {
> +		struct property *p = of_get_property(node, "compatible", NULL);
> +		prom_remove_property(node, p);

What if p is NULL?  prom_remove_property() looks like it won't break
if it is, but that looks to be only by luck.  I think this can be more
defensive.

> +		prom_alloc_property(node, "compatible", "olpc,xo1-rtc");
> +	}
> +	of_node_put(node);
> +
> +	/*
> +	 * Add "dcon" device node, it was missing from earlier firmware
> +	 * releases.
> +	 */
> +	node = of_find_node_by_name(NULL, "display");
> +	if (node)
> +		add_dcon_node(node);
> +	of_node_put(node);
> +}
> +
>  void __init olpc_dt_build_devicetree(void)
>  {
>  	phandle root;
> @@ -177,6 +263,7 @@ void __init olpc_dt_build_devicetree(void)
>  		return;
>  	}
>  	of_pdt_build_devicetree(root, &prom_olpc_ops);
> +	olpc_dt_fixup();
>  
>  	pr_info("PROM DT: Built device tree with %u bytes of memory.\n",
>  			prom_early_allocated);
> -- 
> 1.7.4
> 

Re: [RFC 2/3] OLPC: add missing elements to device tree

[Andres Salomon]

On Fri, 4 Mar 2011 21:50:42 -0700
Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:

> On Fri, Mar 04, 2011 at 05:12:34PM +0000, Daniel Drake wrote:
> > In response to new device tree code in the kernel, OLPC will start
> > using it for probing of certain devices. However, some firmware
> > fixes are needed to put the devicetree into a usable state.
> > 
> > Retain compatibility with old firmware by fixing up the device tree
> > at boot-time if it does not contain the new nodes/properties that
> > we need for probing.
> > 
> > Signed-off-by: Daniel Drake <dsd-2X9k7bc8m7Mdnm+yROfE0A@public.gmane.org>
> 
> Hi Daniel,
> 
> The concept looks good. here are a few comments on the
> implementation.
> 
> g.
> 
> > ---
> >  arch/x86/platform/olpc/olpc_dt.c |   87
> > ++++++++++++++++++++++++++++++++++++++ 1 files changed, 87
> > insertions(+), 0 deletions(-)
> > 
> > diff --git a/arch/x86/platform/olpc/olpc_dt.c
> > b/arch/x86/platform/olpc/olpc_dt.c index 09cbede..58bba78 100644
> > --- a/arch/x86/platform/olpc/olpc_dt.c
> > +++ b/arch/x86/platform/olpc/olpc_dt.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/of_platform.h>
> >  #include <linux/of_pdt.h>
> >  #include <asm/olpc_ofw.h>
> > +#include <asm/olpc.h>
> >  
> >  static phandle __init olpc_dt_getsibling(phandle node)
> >  {
> > @@ -164,6 +165,91 @@ static struct of_pdt_ops prom_olpc_ops
> > __initdata = { .pkg2path = olpc_dt_pkg2path,
> >  };
> >  
> > +static char __init *prom_alloc_string(const char *str, int *len)
> > +{
> > +	int _len = strlen(str);
> > +	char *output = prom_early_alloc(_len + 1);
> > +
> > +	strcpy(output, str);
> > +	if (len)
> > +		*len = _len;
> > +	return output;
> > +}
> > +
> > +static void __init prom_alloc_property(struct device_node *node,
> > +	const char *name, const char *value)
> > +{
> > +	struct property *p = prom_early_alloc(sizeof(struct
> > property));
> 
> How early is this running?  If the allocator is set up by now, then
> this should use kzalloc()...

It's running at the same time the device tree is created, so no, the
allocator isn't available yet.

This isn't strictly a requirement, but it would be good to get
firmware hacks out of the way as early as possible, and it might as
well be done at the same time that the DT is created.

> 
> > +
> > +	p->name = prom_alloc_string(name, NULL);
> > +	p->value = prom_alloc_string(value, &p->length);
> 
> ... which would also mean that kstrdup()/kmemdup() can be used here
> directly.
> 
> > +	prom_add_property(node, p);
> > +}
> 
> This would be useful in common code.  prom_create_property() might
> be a better name.  It should also accept a length argument for the
> size of the data.
> 
> > +
> > +/* Add dcon device as child of display */
> > +static void __init add_dcon_node(struct device_node *display)
> > +{
> > +	struct device_node *node = prom_early_alloc(sizeof(struct
> > device_node));
> > +	unsigned long flags;
> > +
> > +	kref_init(&node->kref);
> > +	node->name = prom_alloc_string("dcon", NULL);
> > +	node->full_name = (char *) node->name;
> > +	node->parent = display;
> > +
> > +	write_lock_irqsave(&devtree_lock, flags);
> > +	node->sibling = node->parent->child;
> > +	node->allnext = allnodes;
> > +	node->parent->child = node;
> > +	allnodes = node;
> > +	write_unlock_irqrestore(&devtree_lock, flags);
> > +
> 
> This should be using the existing of_attach_node() function to link
> the node into the tree (in fact I know you've seen it because the
> above 6 lines are identical).  :-)  Also, there already exists a
> function "new_node" in  arch/powerpc/platforms/iseries/vio.c that
> would be useful to move into drivers/of/base.c

Is there any particular reason why OF_DYNAMIC needs to depend upon
PPC_OF?

> 
> > +	prom_alloc_property(node, "compatible", "olpc,xo1-dcon");
> > +}
> > +
> > +static void __init olpc_dt_fixup(void)
> > +{
> > +	struct device_node *node;
> > +
> > +	/*
> > +	 * Use battery's compatible property to determine if we're
> > running a
> > +	 * new-enough firmware. If we have this property, no
> > fixups are needed.
> > +	 */
> > +	node = of_find_node_by_name(NULL, "battery");
> > +	if (node && of_get_property(node, "compatible", NULL)) {
> > +		of_node_put(node);
> > +		return;
> > +	}
> > +
> > +	/*
> > +	 * Add "compatible" property to battery, it was missing
> > from earlier
> > +	 * firmware releases.
> > +	 */
> > +	prom_alloc_property(node, "compatible",
> > "olpc,xo1-battery");
> > +	of_node_put(node);
> > +
> > +	/*
> > +	 * Mark XO-1 RTC as compatible with olpc,xo1-rtc - this
> > was not done in
> > +	 * earlier firmware releases.
> > +	 */
> > +	node = of_find_node_by_name(NULL, "rtc");
> > +	if (olpc_platform_info.boardrev < olpc_board_pre(0xd0) &&
> > node) {
> > +		struct property *p = of_get_property(node,
> > "compatible", NULL);
> > +		prom_remove_property(node, p);
> 
> What if p is NULL?  prom_remove_property() looks like it won't break
> if it is, but that looks to be only by luck.  I think this can be more
> defensive.
> 
> > +		prom_alloc_property(node, "compatible",
> > "olpc,xo1-rtc");
> > +	}
> > +	of_node_put(node);
> > +
> > +	/*
> > +	 * Add "dcon" device node, it was missing from earlier
> > firmware
> > +	 * releases.
> > +	 */
> > +	node = of_find_node_by_name(NULL, "display");
> > +	if (node)
> > +		add_dcon_node(node);
> > +	of_node_put(node);
> > +}
> > +
> >  void __init olpc_dt_build_devicetree(void)
> >  {
> >  	phandle root;
> > @@ -177,6 +263,7 @@ void __init olpc_dt_build_devicetree(void)
> >  		return;
> >  	}
> >  	of_pdt_build_devicetree(root, &prom_olpc_ops);
> > +	olpc_dt_fixup();
> >  
> >  	pr_info("PROM DT: Built device tree with %u bytes of
> > memory.\n", prom_early_allocated);
> > -- 
> > 1.7.4
> > 

Re: [RFC 2/3] OLPC: add missing elements to device tree

[Grant Likely]

On Sat, Mar 05, 2011 at 09:23:47AM -0800, Andres Salomon wrote:
> On Fri, 4 Mar 2011 21:50:42 -0700
> Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> 
> > > +static void __init prom_alloc_property(struct device_node *node,
> > > +	const char *name, const char *value)
> > > +{
> > > +	struct property *p = prom_early_alloc(sizeof(struct
> > > property));
> > 
> > How early is this running?  If the allocator is set up by now, then
> > this should use kzalloc()...
> 
> It's running at the same time the device tree is created, so no, the
> allocator isn't available yet.
> 
> This isn't strictly a requirement, but it would be good to get
> firmware hacks out of the way as early as possible, and it might as
> well be done at the same time that the DT is created.

Yes, you definitely want to get firmware fiddling done with as soon as
possible.  Certainly before anything starts relying on the device tree
data.

Another option (assuming OFW is still alive) is to as it to create the
missing properties before extracting the tree.  This is what we're
currently doing in PowerPC.

> 
> > 
> > > +
> > > +	p->name = prom_alloc_string(name, NULL);
> > > +	p->value = prom_alloc_string(value, &p->length);
> > 
> > ... which would also mean that kstrdup()/kmemdup() can be used here
> > directly.
> > 
> > > +	prom_add_property(node, p);
> > > +}
> > 
> > This would be useful in common code.  prom_create_property() might
> > be a better name.  It should also accept a length argument for the
> > size of the data.
> > 
> > > +
> > > +/* Add dcon device as child of display */
> > > +static void __init add_dcon_node(struct device_node *display)
> > > +{
> > > +	struct device_node *node = prom_early_alloc(sizeof(struct
> > > device_node));
> > > +	unsigned long flags;
> > > +
> > > +	kref_init(&node->kref);
> > > +	node->name = prom_alloc_string("dcon", NULL);
> > > +	node->full_name = (char *) node->name;
> > > +	node->parent = display;
> > > +
> > > +	write_lock_irqsave(&devtree_lock, flags);
> > > +	node->sibling = node->parent->child;
> > > +	node->allnext = allnodes;
> > > +	node->parent->child = node;
> > > +	allnodes = node;
> > > +	write_unlock_irqrestore(&devtree_lock, flags);
> > > +
> > 
> > This should be using the existing of_attach_node() function to link
> > the node into the tree (in fact I know you've seen it because the
> > above 6 lines are identical).  :-)  Also, there already exists a
> > function "new_node" in  arch/powerpc/platforms/iseries/vio.c that
> > would be useful to move into drivers/of/base.c
> 
> Is there any particular reason why OF_DYNAMIC needs to depend upon
> PPC_OF?

Only historical.  Those functions can probably be brought out from
under the OF_DYNAMIC protection...  Bonus points for making pdt.c and
fdt.c use it for linking nodes to the tree so that all methods are
consistent.  :-)

It would also be useful to make the property linking code common for
fdt.c and pdt.c

g.

Re: [RFC 2/3] OLPC: add missing elements to device tree

[Andres Salomon]

On Sat, 5 Mar 2011 17:19:43 -0700
Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:

> On Sat, Mar 05, 2011 at 09:23:47AM -0800, Andres Salomon wrote:
> > On Fri, 4 Mar 2011 21:50:42 -0700
> > Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> > 
> > > > +static void __init prom_alloc_property(struct device_node
> > > > *node,
> > > > +	const char *name, const char *value)
> > > > +{
> > > > +	struct property *p = prom_early_alloc(sizeof(struct
> > > > property));
> > > 
> > > How early is this running?  If the allocator is set up by now,
> > > then this should use kzalloc()...
> > 
> > It's running at the same time the device tree is created, so no, the
> > allocator isn't available yet.
> > 
> > This isn't strictly a requirement, but it would be good to get
> > firmware hacks out of the way as early as possible, and it might as
> > well be done at the same time that the DT is created.
> 
> Yes, you definitely want to get firmware fiddling done with as soon as
> possible.  Certainly before anything starts relying on the device tree
> data.
> 
> Another option (assuming OFW is still alive) is to as it to create the
> missing properties before extracting the tree.  This is what we're
> currently doing in PowerPC.

OFW is indeed still alive.  I don't have an opinion on that, I'll leave
it up to dsd to decide (once he's no longer busy with other stuff).




> 
> > 
> > > 
> > > > +
> > > > +	p->name = prom_alloc_string(name, NULL);
> > > > +	p->value = prom_alloc_string(value, &p->length);
> > > 
> > > ... which would also mean that kstrdup()/kmemdup() can be used
> > > here directly.
> > > 
> > > > +	prom_add_property(node, p);
> > > > +}
> > > 
> > > This would be useful in common code.  prom_create_property() might
> > > be a better name.  It should also accept a length argument for the
> > > size of the data.
> > > 
> > > > +
> > > > +/* Add dcon device as child of display */
> > > > +static void __init add_dcon_node(struct device_node *display)
> > > > +{
> > > > +	struct device_node *node =
> > > > prom_early_alloc(sizeof(struct device_node));
> > > > +	unsigned long flags;
> > > > +
> > > > +	kref_init(&node->kref);
> > > > +	node->name = prom_alloc_string("dcon", NULL);
> > > > +	node->full_name = (char *) node->name;
> > > > +	node->parent = display;
> > > > +
> > > > +	write_lock_irqsave(&devtree_lock, flags);
> > > > +	node->sibling = node->parent->child;
> > > > +	node->allnext = allnodes;
> > > > +	node->parent->child = node;
> > > > +	allnodes = node;
> > > > +	write_unlock_irqrestore(&devtree_lock, flags);
> > > > +
> > > 
> > > This should be using the existing of_attach_node() function to
> > > link the node into the tree (in fact I know you've seen it
> > > because the above 6 lines are identical).  :-)  Also, there
> > > already exists a function "new_node" in
> > > arch/powerpc/platforms/iseries/vio.c that would be useful to move
> > > into drivers/of/base.c
> > 
> > Is there any particular reason why OF_DYNAMIC needs to depend upon
> > PPC_OF?
> 
> Only historical.  Those functions can probably be brought out from
> under the OF_DYNAMIC protection...  Bonus points for making pdt.c and
> fdt.c use it for linking nodes to the tree so that all methods are
> consistent.  :-)

Done.  I've done absolutely no testing of the flattree changes.

> 
> It would also be useful to make the property linking code common for
> fdt.c and pdt.c

I can look at that and the new_node stuff later.