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

David Gibson david at gibson.dropbear.id.au
Thu Jun 26 23:20:46 EDT 2008


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



More information about the linux-mtd mailing list