[PATCH] Support NAND partitions >4GiB with Open Firmware
Mitch Bradley
wmb at firmworks.com
Thu Jun 26 23:28:42 EDT 2008
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 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.
>
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)
>>
>
>
More information about the linux-mtd
mailing list