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

[Andres Salomon <dilinger-pFFUokh25LWsTnJN9+BGXg@public.gmane.org>]

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
> >