[PATCH] Support NAND partitions >4GiB with Open Firmware

[PATCH] Support NAND partitions >4GiB with Open Firmware

[Mitch Bradley]

This patch modifes ofpart.c so the total size of NAND FLASH
and the size of an individual partition can exceed 4GiB.  It does so
by decoding the "reg" property based on the values of "#address-cells"
and "#size-cells" in the parent node, thus allowing the base address
and size to be 64-bit numbers if necessary.  It handles any combination
of #address-cells = 1 or 2 and #size-cells = 1 or 2, handles the case
where the parent node doesn't have #address-cells / #size-cells properties,
and handles the case where the value of #address-cells is incorrectly
inherited from farther up the tree than the direct parent.

This patch does not solve the problem that the MTD subsystem
itself is limited to 2 GiB NAND sizes, but it is a step in that direction.
The final assignment of the 64-bit partition offset and size values is
truncated (by the C type conversion rules) to the actual size of the
struct mtd_partition offset and size fields (which are currently u_int32's).
At some point in the future, when those fields become larger, the code
should "just work".

The patch should apply to either 2.6.25 or 2.6.26rc.  It has been tested
on the OLPC variant of 2.6.25 , with additional patches to other
OLPC-specific files that aren't necessary for other architectures
that have more mature support for Open Firmware.  The OLPC
patch set, plus a set of test cases that verify correct operation for
various #address-cells / #size-cells combinations, can be found at
http://dev.laptop.org/~wmb/ofpart-olpc.tgz

Signed-off-by: Mitch Bradley <wmb@firmworks.com>

diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
index f86e069..b83b26c 100644
--- a/drivers/mtd/ofpart.c
+++ b/drivers/mtd/ofpart.c
@@ -7,6 +7,10 @@
  * Revised to handle newer style flash binding by:
  *   Copyright (C) 2007 David Gibson, IBM Corporation.
  *
+ * Revised to handle multi-cell addresses and size in reg properties,
+ * paving the way for NAND FLASH devices > 4GiB by:
+ *   Mitch Bradley <wmb@laptop.org>
+ *
  * This program is free software; you can redistribute  it and/or modify it
  * under  the terms of  the GNU General  Public License as published by the
  * Free Software Foundation;  either version 2 of the  License, or (at your
@@ -15,0 +19,0 @@

 #include <linux/module.h>
 #include <linux/init.h>
+#include <linux/device.h>
 #include <linux/of.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>

+u_int32_t decode_cell(const u_int8_t *prop)
+{
+       return ((prop[0] << 24) + (prop[1] << 16) + (prop[2] << 8) + 
prop[3]);
+}
+
 int __devinit of_mtd_parse_partitions(struct device *dev,
                                       struct mtd_info *mtd,
                                       struct device_node *node,
@@ -44,5 +54,7 @@ int __devinit of_mtd_parse_partitions(struct device *dev,
        pp = NULL;
        i = 0;
        while ((pp = of_get_next_child(node, pp))) {
-               const u32 *reg;
+               u_int64_t xoffset, xsize;
+               const u_int32_t *propval;
+               u_int32_t addrcells = 0, sizecells = 0;
                int len;

-               reg = of_get_property(pp, "reg", &len);
-               if (!reg || (len != 2 * sizeof(u32))) {
+               /*
+                * Determine the layout of a "reg" entry based on the parent
+                * node's properties, if it hasn't been done already.
+                */
+
+               if (addrcells == 0)
+                       addrcells = of_n_addr_cells(pp);
+               if (sizecells == 0)
+                       sizecells = of_n_size_cells(pp);
+
+               propval = of_get_property(pp, "reg", &len);
+
+               /*
+                * Handle the possibility of a broken device tree that
+                * doesn't define #address-cells and #size-cells properly.
+                 * In a standard device tree, if the address portion of
+                 * "reg" is one cell, the direct parent should have a
+                 * #address-cells property with value 1.
+                */
+               if (propval && (len == 2 * sizeof(u32)))
+                       addrcells = sizecells = 1;
+
+               /* Error checks */
+
+               if (addrcells < 1 || addrcells > 2) {
+                       of_node_put(pp);
+                       dev_err(dev, "Invalid #address_cells %d on %s\n",
+                               addrcells, node->full_name);
+                       kfree(*pparts);
+                       *pparts = NULL;
+                       return -EINVAL;
+               }
+
+               if (sizecells < 1 || sizecells > 2) {
+                       of_node_put(pp);
+                       dev_err(dev, "Invalid #size-cells %d on %s\n",
+                               sizecells, node->full_name);
+                       kfree(*pparts);
+                       *pparts = NULL;
+                       return -EINVAL;
+               }
+
+               if (!propval || (len != (addrcells+sizecells) * 
sizeof(u32))) {
                        of_node_put(pp);
                        dev_err(dev, "Invalid 'reg' on %s\n", 
node->full_name);
                        kfree(*pparts);
                        *pparts = NULL;
                        return -EINVAL;
                }
-               (*pparts)[i].offset = reg[0];
-               (*pparts)[i].size = reg[1];
+
+               /* Accumulate the base address and size into 64-bit 
numbers */
+               xoffset = (u_int64_t)ntohl(propval[0]);
+               if (addrcells > 1) {
+                       xoffset <<= 32;
+                       xoffset += ntohl(propval[1]);
+               }
+               xsize = (u_int64_t)ntohl(propval[addrcells]);
+               if (sizecells > 1) {
+                       xsize <<= 32;
+                       xsize += ntohl(propval[addrcells+1]);
+               }
+
+               (*pparts)[i].offset = xoffset;
+               (*pparts)[i].size = xsize;

                partname = of_get_property(pp, "label", &len);
                if (!partname)

Re: [PATCH] Support NAND partitions >4GiB with Open Firmware

[Mitch Bradley]

A revised version of the patch, addressing some points that Segher
identified, will be issued soon.

So if you want to review the patch as submitted, please be aware that
some stylistic things have already been fixed (u64 instead of u_int64_t
etc, use of of_read_number(), removal of fallback code for broken
device trees).  The info in Documentation/powerpc/booting-without-of.txt
will also be amended.

The overall scheme - using #address-cells and #size-cells to permit 64-bit
offset and sizes - remains unchanged.

Re: [PATCH] Support NAND partitions >4GiB with Open Firmware

[David Gibson]

On Thu, Jun 26, 2008 at 01:50:40PM -1000, Mitch Bradley wrote:
> This patch modifes ofpart.c so the total size of NAND FLASH
> and the size of an individual partition can exceed 4GiB.  It does so
> by decoding the "reg" property based on the values of "#address-cells"
> and "#size-cells" in the parent node, thus allowing the base address
> and size to be 64-bit numbers if necessary.  It handles any combination
> of #address-cells = 1 or 2 and #size-cells = 1 or 2, handles the case
> where the parent node doesn't have #address-cells / #size-cells properties,
> and handles the case where the value of #address-cells is incorrectly
> inherited from farther up the tree than the direct parent.
>
> This patch does not solve the problem that the MTD subsystem
> itself is limited to 2 GiB NAND sizes, but it is a step in that direction.
> The final assignment of the 64-bit partition offset and size values is
> truncated (by the C type conversion rules) to the actual size of the
> struct mtd_partition offset and size fields (which are currently u_int32's).
> At some point in the future, when those fields become larger, the code
> should "just work".
>
> The patch should apply to either 2.6.25 or 2.6.26rc.  It has been tested
> on the OLPC variant of 2.6.25 , with additional patches to other
> OLPC-specific files that aren't necessary for other architectures
> that have more mature support for Open Firmware.  The OLPC
> patch set, plus a set of test cases that verify correct operation for
> various #address-cells / #size-cells combinations, can be found at
> http://dev.laptop.org/~wmb/ofpart-olpc.tgz

I like the idea - but there are some uglies in the implementation.

> Signed-off-by: Mitch Bradley <wmb@firmworks.com>
>
> diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
> index f86e069..b83b26c 100644
> --- a/drivers/mtd/ofpart.c
> +++ b/drivers/mtd/ofpart.c
> @@ -7,6 +7,10 @@
>  * Revised to handle newer style flash binding by:
>  *   Copyright (C) 2007 David Gibson, IBM Corporation.
>  *
> + * Revised to handle multi-cell addresses and size in reg properties,
> + * paving the way for NAND FLASH devices > 4GiB by:
> + *   Mitch Bradley <wmb@laptop.org>
> + *
>  * This program is free software; you can redistribute  it and/or modify it
>  * under  the terms of  the GNU General  Public License as published by the
>  * Free Software Foundation;  either version 2 of the  License, or (at your
> @@ -15,0 +19,0 @@
>
> #include <linux/module.h>
> #include <linux/init.h>
> +#include <linux/device.h>
> #include <linux/of.h>
> #include <linux/mtd/mtd.h>
> #include <linux/mtd/partitions.h>
>
> +u_int32_t decode_cell(const u_int8_t *prop)
> +{
> +       return ((prop[0] << 24) + (prop[1] << 16) + (prop[2] << 8) +  
> prop[3]);
> +}

You don't appear to actually use this new function.

> int __devinit of_mtd_parse_partitions(struct device *dev,
>                                       struct mtd_info *mtd,
>                                       struct device_node *node,
> @@ -44,5 +54,7 @@ int __devinit of_mtd_parse_partitions(struct device *dev,
>        pp = NULL;
>        i = 0;
>        while ((pp = of_get_next_child(node, pp))) {
> -               const u32 *reg;
> +               u_int64_t xoffset, xsize;

The names u64 and u32 are preferred for kernel internal things.
Certainly not u_int64_t which isn't even the C99 standard name.

> +               const u_int32_t *propval;
> +               u_int32_t addrcells = 0, sizecells = 0;
>                int len;
>
> -               reg = of_get_property(pp, "reg", &len);
> -               if (!reg || (len != 2 * sizeof(u32))) {
> +               /*
> +                * Determine the layout of a "reg" entry based on the parent
> +                * node's properties, if it hasn't been done already.
> +                */
> +
> +               if (addrcells == 0)

Redundant 'if'; you've just initialized this variable to zero.

> +                       addrcells = of_n_addr_cells(pp);
> +               if (sizecells == 0)
> +                       sizecells = of_n_size_cells(pp);
> +
> +               propval = of_get_property(pp, "reg", &len);
> +
> +               /*
> +                * Handle the possibility of a broken device tree that
> +                * doesn't define #address-cells and #size-cells properly.
> +                 * In a standard device tree, if the address portion of
> +                 * "reg" is one cell, the direct parent should have a
> +                 * #address-cells property with value 1.
> +                */
> +               if (propval && (len == 2 * sizeof(u32)))
> +                       addrcells = sizecells = 1;
> +
> +               /* Error checks */
> +
> +               if (addrcells < 1 || addrcells > 2) {
> +                       of_node_put(pp);
> +                       dev_err(dev, "Invalid #address_cells %d on %s\n",
> +                               addrcells, node->full_name);
> +                       kfree(*pparts);
> +                       *pparts = NULL;
> +                       return -EINVAL;
> +               }
> +
> +               if (sizecells < 1 || sizecells > 2) {
> +                       of_node_put(pp);
> +                       dev_err(dev, "Invalid #size-cells %d on %s\n",
> +                               sizecells, node->full_name);
> +                       kfree(*pparts);
> +                       *pparts = NULL;
> +                       return -EINVAL;
> +               }
> +
> +               if (!propval || (len != (addrcells+sizecells) *  
> sizeof(u32))) {
>                        of_node_put(pp);
>                        dev_err(dev, "Invalid 'reg' on %s\n",  
> node->full_name);
>                        kfree(*pparts);
>                        *pparts = NULL;
>                        return -EINVAL;
>                }
> -               (*pparts)[i].offset = reg[0];
> -               (*pparts)[i].size = reg[1];
> +
> +               /* Accumulate the base address and size into 64-bit  
> numbers */
> +               xoffset = (u_int64_t)ntohl(propval[0]);

You shouldn't use the ntohl() names outside of networking code.
Instead use be32_to_cpu() and the like.

> +               if (addrcells > 1) {
> +                       xoffset <<= 32;
> +                       xoffset += ntohl(propval[1]);
> +               }
> +               xsize = (u_int64_t)ntohl(propval[addrcells]);
> +               if (sizecells > 1) {
> +                       xsize <<= 32;
> +                       xsize += ntohl(propval[addrcells+1]);
> +               }
> +
> +               (*pparts)[i].offset = xoffset;
> +               (*pparts)[i].size = xsize;
>
>                partname = of_get_property(pp, "label", &len);
>                if (!partname)

-- 
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] Support NAND partitions >4GiB with Open Firmware

[Mitch Bradley]

David Gibson wrote:
> On Thu, Jun 26, 2008 at 01:50:40PM -1000, Mitch Bradley wrote:
>   
>> This patch modifes ofpart.c so the total size of NAND FLASH
>> and the size of an individual partition can exceed 4GiB.  It does so
>> by decoding the "reg" property based on the values of "#address-cells"
>> and "#size-cells" in the parent node, thus allowing the base address
>> and size to be 64-bit numbers if necessary.  It handles any combination
>> of #address-cells = 1 or 2 and #size-cells = 1 or 2, handles the case
>> where the parent node doesn't have #address-cells / #size-cells properties,
>> and handles the case where the value of #address-cells is incorrectly
>> inherited from farther up the tree than the direct parent.
>>
>> This patch does not solve the problem that the MTD subsystem
>> itself is limited to 2 GiB NAND sizes, but it is a step in that direction.
>> The final assignment of the 64-bit partition offset and size values is
>> truncated (by the C type conversion rules) to the actual size of the
>> struct mtd_partition offset and size fields (which are currently u_int32's).
>> At some point in the future, when those fields become larger, the code
>> should "just work".
>>
>> The patch should apply to either 2.6.25 or 2.6.26rc.  It has been tested
>> on the OLPC variant of 2.6.25 , with additional patches to other
>> OLPC-specific files that aren't necessary for other architectures
>> that have more mature support for Open Firmware.  The OLPC
>> patch set, plus a set of test cases that verify correct operation for
>> various #address-cells / #size-cells combinations, can be found at
>> http://dev.laptop.org/~wmb/ofpart-olpc.tgz
>>     
>
> I like the idea - but there are some uglies in the implementation.
>
>   
>> Signed-off-by: Mitch Bradley <wmb@firmworks.com>
>>
>> diff --git a/drivers/mtd/ofpart.c b/drivers/mtd/ofpart.c
>> index f86e069..b83b26c 100644
>> --- a/drivers/mtd/ofpart.c
>> +++ b/drivers/mtd/ofpart.c
>> @@ -7,6 +7,10 @@
>>  * Revised to handle newer style flash binding by:
>>  *   Copyright (C) 2007 David Gibson, IBM Corporation.
>>  *
>> + * Revised to handle multi-cell addresses and size in reg properties,
>> + * paving the way for NAND FLASH devices > 4GiB by:
>> + *   Mitch Bradley <wmb@laptop.org>
>> + *
>>  * This program is free software; you can redistribute  it and/or modify it
>>  * under  the terms of  the GNU General  Public License as published by the
>>  * Free Software Foundation;  either version 2 of the  License, or (at your
>> @@ -15,0 +19,0 @@
>>
>> #include <linux/module.h>
>> #include <linux/init.h>
>> +#include <linux/device.h>
>> #include <linux/of.h>
>> #include <linux/mtd/mtd.h>
>> #include <linux/mtd/partitions.h>
>>
>> +u_int32_t decode_cell(const u_int8_t *prop)
>> +{
>> +       return ((prop[0] << 24) + (prop[1] << 16) + (prop[2] << 8) +  
>> prop[3]);
>> +}
>>     
>
> You don't appear to actually use this new function.
>   

It's already gone in the new version (the announcement of which may have 
been delayed by the mailing list of which I'm not a subscriber).
>   
>> int __devinit of_mtd_parse_partitions(struct device *dev,
>>                                       struct mtd_info *mtd,
>>                                       struct device_node *node,
>> @@ -44,5 +54,7 @@ int __devinit of_mtd_parse_partitions(struct device *dev,
>>        pp = NULL;
>>        i = 0;
>>        while ((pp = of_get_next_child(node, pp))) {
>> -               const u32 *reg;
>> +               u_int64_t xoffset, xsize;
>>     
>
> The names u64 and u32 are preferred for kernel internal things.
> Certainly not u_int64_t which isn't even the C99 standard name.
>   

Already fixed.

>   
>> +               const u_int32_t *propval;
>> +               u_int32_t addrcells = 0, sizecells = 0;
>>                int len;
>>
>> -               reg = of_get_property(pp, "reg", &len);
>> -               if (!reg || (len != 2 * sizeof(u32))) {
>> +               /*
>> +                * Determine the layout of a "reg" entry based on the parent
>> +                * node's properties, if it hasn't been done already.
>> +                */
>> +
>> +               if (addrcells == 0)
>>     
>
> Redundant 'if'; you've just initialized this variable to zero.
>   

The intention is that the body of the "if" should only be executed
once during the loop, since the parent node is the same for all children.

>   
>> +                       addrcells = of_n_addr_cells(pp);
>> +               if (sizecells == 0)
>> +                       sizecells = of_n_size_cells(pp);
>> +
>> +               propval = of_get_property(pp, "reg", &len);
>> +
>> +               /*
>> +                * Handle the possibility of a broken device tree that
>> +                * doesn't define #address-cells and #size-cells properly.
>> +                 * In a standard device tree, if the address portion of
>> +                 * "reg" is one cell, the direct parent should have a
>> +                 * #address-cells property with value 1.
>> +                */
>> +               if (propval && (len == 2 * sizeof(u32)))
>> +                       addrcells = sizecells = 1;
>> +
>> +               /* Error checks */
>> +
>> +               if (addrcells < 1 || addrcells > 2) {
>> +                       of_node_put(pp);
>> +                       dev_err(dev, "Invalid #address_cells %d on %s\n",
>> +                               addrcells, node->full_name);
>> +                       kfree(*pparts);
>> +                       *pparts = NULL;
>> +                       return -EINVAL;
>> +               }
>> +
>> +               if (sizecells < 1 || sizecells > 2) {
>> +                       of_node_put(pp);
>> +                       dev_err(dev, "Invalid #size-cells %d on %s\n",
>> +                               sizecells, node->full_name);
>> +                       kfree(*pparts);
>> +                       *pparts = NULL;
>> +                       return -EINVAL;
>> +               }
>> +
>> +               if (!propval || (len != (addrcells+sizecells) *  
>> sizeof(u32))) {
>>                        of_node_put(pp);
>>                        dev_err(dev, "Invalid 'reg' on %s\n",  
>> node->full_name);
>>                        kfree(*pparts);
>>                        *pparts = NULL;
>>                        return -EINVAL;
>>                }
>> -               (*pparts)[i].offset = reg[0];
>> -               (*pparts)[i].size = reg[1];
>> +
>> +               /* Accumulate the base address and size into 64-bit  
>> numbers */
>> +               xoffset = (u_int64_t)ntohl(propval[0]);
>>     
>
> You shouldn't use the ntohl() names outside of networking code.
> Instead use be32_to_cpu() and the like.
>   

That whole code block has been replaced by a call to of_read_number()

>   
>> +               if (addrcells > 1) {
>> +                       xoffset <<= 32;
>> +                       xoffset += ntohl(propval[1]);
>> +               }
>> +               xsize = (u_int64_t)ntohl(propval[addrcells]);
>> +               if (sizecells > 1) {
>> +                       xsize <<= 32;
>> +                       xsize += ntohl(propval[addrcells+1]);
>> +               }
>> +
>> +               (*pparts)[i].offset = xoffset;
>> +               (*pparts)[i].size = xsize;
>>
>>                partname = of_get_property(pp, "label", &len);
>>                if (!partname)
>>     
>
>   

Re: [PATCH] Support NAND partitions >4GiB with Open Firmware

[David Gibson]

On Thu, Jun 26, 2008 at 05:28:42PM -1000, Mitch Bradley wrote:
> David Gibson wrote:
>> On Thu, Jun 26, 2008 at 01:50:40PM -1000, Mitch Bradley wrote:
[snip]
>>> +               const u_int32_t *propval;
>>> +               u_int32_t addrcells = 0, sizecells = 0;
>>>                int len;
>>>
>>> -               reg = of_get_property(pp, "reg", &len);
>>> -               if (!reg || (len != 2 * sizeof(u32))) {
>>> +               /*
>>> +                * Determine the layout of a "reg" entry based on the parent
>>> +                * node's properties, if it hasn't been done already.
>>> +                */
>>> +
>>> +               if (addrcells == 0)
>>>     
>>
>> Redundant 'if'; you've just initialized this variable to zero.
>
> The intention is that the body of the "if" should only be executed
> once during the loop, since the parent node is the same for all
> children.

But the initialization is within the loop body as well, so this won't
do it.  Just factor the code getting addr and size cells right out of
the loop, instead.

-- 
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] Support NAND partitions >4GiB with Open Firmware

[Mitch Bradley]

David Gibson wrote:
> On Thu, Jun 26, 2008 at 05:28:42PM -1000, Mitch Bradley wrote:
>   
>> David Gibson wrote:
>>     
>>> On Thu, Jun 26, 2008 at 01:50:40PM -1000, Mitch Bradley wrote:
>>>       
> [snip]
>   
>>>> +               const u_int32_t *propval;
>>>> +               u_int32_t addrcells = 0, sizecells = 0;
>>>>                int len;
>>>>
>>>> -               reg = of_get_property(pp, "reg", &len);
>>>> -               if (!reg || (len != 2 * sizeof(u32))) {
>>>> +               /*
>>>> +                * Determine the layout of a "reg" entry based on the parent
>>>> +                * node's properties, if it hasn't been done already.
>>>> +                */
>>>> +
>>>> +               if (addrcells == 0)
>>>>     
>>>>         
>>> Redundant 'if'; you've just initialized this variable to zero.
>>>       
>> The intention is that the body of the "if" should only be executed
>> once during the loop, since the parent node is the same for all
>> children.
>>     
>
> But the initialization is within the loop body as well, so this won't
> do it.  Just factor the code getting addr and size cells right out of
> the loop, instead.
>
>   

Hmmm.  Perhaps it's better to move the declaration of the variables out of
the loop instead.

Moving the of_n_*_cells() calls outside the loop requires redundant calls
to of_get_child() and of_node_put(), because of_n_*_cells() implicitly
reach up to the parent node.  That is almost certainly more expensive 
than the "if".

Re: [PATCH] Support NAND partitions >4GiB with Open Firmware

[David Gibson]

On Thu, Jun 26, 2008 at 05:48:01PM -1000, Mitch Bradley wrote:
>
>
> David Gibson wrote:
>> On Thu, Jun 26, 2008 at 05:28:42PM -1000, Mitch Bradley wrote:
>>   
>>> David Gibson wrote:
>>>     
>>>> On Thu, Jun 26, 2008 at 01:50:40PM -1000, Mitch Bradley wrote:
>>>>       
>> [snip]
>>   
>>>>> +               const u_int32_t *propval;
>>>>> +               u_int32_t addrcells = 0, sizecells = 0;
>>>>>                int len;
>>>>>
>>>>> -               reg = of_get_property(pp, "reg", &len);
>>>>> -               if (!reg || (len != 2 * sizeof(u32))) {
>>>>> +               /*
>>>>> +                * Determine the layout of a "reg" entry based on the parent
>>>>> +                * node's properties, if it hasn't been done already.
>>>>> +                */
>>>>> +
>>>>> +               if (addrcells == 0)
>>>>>             
>>>> Redundant 'if'; you've just initialized this variable to zero.
>>>>       
>>> The intention is that the body of the "if" should only be executed
>>> once during the loop, since the parent node is the same for all
>>> children.
>>>     
>>
>> But the initialization is within the loop body as well, so this won't
>> do it.  Just factor the code getting addr and size cells right out of
>> the loop, instead.
>>
>>   
>
> Hmmm.  Perhaps it's better to move the declaration of the variables out of
> the loop instead.
>
> Moving the of_n_*_cells() calls outside the loop requires redundant calls
> to of_get_child() and of_node_put(), because of_n_*_cells() implicitly
> reach up to the parent node.  That is almost certainly more expensive  
> than the "if".

This is not exactly a hot path; clarity and (source) code size are
more important than expense.  But going down to the child then back up
is ugly too.  Maybe you should just directly pull #address-cells and
#size-cells from the parent node.  In fact, of_n_*_cells() are wrong
by the OF spec, since they assume the values are inherited, which is
not how it's supposed to work.

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