Support NAND partitions >4GiB with Open Firmware

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 <[email protected]>

drivers/mtd/ofpart.c +4 -0

index f86e069..b83b26c 100644
@@ -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 <[email protected]>
+ *
 * 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>

drivers/mtd/ofpart.c +3 -0

+u_int32_t decode_cell(const u_int8_t *prop)
+{
+       return ((prop[0] << 24) + (prop[1] << 16) + (prop[2] << 8) +  

prop[3]);
+}

drivers/mtd/ofpart.c +37 -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;
               }

drivers/mtd/ofpart.c +2 -2

-               (*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]);

drivers/mtd/ofpart.c +12 -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)

Quoted 18 lines · 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".

Quoted 8 lines

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.

Quoted 26 lines

Signed-off-by Mitch Bradley <[email protected]>

drivers/mtd/ofpart.c +4 -0

index f86e069..b83b26c 100644
@@ -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 <[email protected]>
+ *
 * 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>

Quoted 6 lines

drivers/mtd/ofpart.c +3 -0

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

drivers/mtd/ofpart.c int __devinit of_mtd_parse_partitions(struct device *dev, · line 58

Quoted 11 lines

int __devinit of_mtd_parse_partitions(struct device *dev,
struct mtd_info *mtd, struct device_node *node,

drivers/mtd/ofpart.c +1 -1

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

Quoted 6 lines

drivers/mtd/ofpart.c +2 -0

+               const u_int32_t *propval;
+               u_int32_t addrcells = 0, sizecells = 0;

               int len;

drivers/mtd/ofpart.c

Quoted 8 lines

drivers/mtd/ofpart.c +6 -2

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

Quoted 48 lines

drivers/mtd/ofpart.c +37 -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;
               }

drivers/mtd/ofpart.c +0 -2

-               (*pparts)[i].offset = reg[0];
-               (*pparts)[i].size = reg[1];

Quoted 5 lines

drivers/mtd/ofpart.c +2 -0

+
+               /* 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.

Quoted 17 lines

drivers/mtd/ofpart.c +12 -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)

Quoted 1 line · David Gibson

On Thu, Jun 26, 2008 at 01:50:40PM -1000, Mitch Bradley wrote:

Quoted 14 lines

Quoted 13 lines

(quoted diff) +8 -2

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

Quoted 1 line

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.

Quoted 4 lines · Mitch Bradley

Quoted 2 lines · David Gibson

On Thu, Jun 26, 2008 at 01:50:40PM -1000, Mitch Bradley wrote:

[snip]

Quoted 16 lines

Quoted 16 lines

Quoted 4 lines

(quoted diff) +2 -0

+               const u_int32_t *propval;
+               u_int32_t addrcells = 0, sizecells = 0;

Review

Quoted 10 lines

(quoted diff) +6 -2

               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.

Quoted 4 lines

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.

Quoted 23 lines · David Gibson

Quoted 4 lines · Mitch Bradley

Quoted 2 lines · David Gibson

On Thu, Jun 26, 2008 at 01:50:40PM -1000, Mitch Bradley wrote:

[snip]

Quoted 15 lines

Quoted 15 lines

Review

Quoted 13 lines

(quoted diff) +8 -2

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

Quoted 10 lines

Quoted 4 lines

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