Re: [PATCH] mtd: physmap_of: Add multiple regions and concatenation support

[Grant Likely <grant.likely@secretlab.ca>]

On Fri, Apr 3, 2009 at 3:55 AM, Stefan Roese <sr@denx.de> wrote:
> This patch adds support to handle multiple non-identical chips in one
> flash device tree node. It also adds concat support to physmap_of. This
> makes it possible to support e.g. the Intel P30 48F4400 chips which
> internally consists of 2 non-identical NOR chips on one die. Additionally
> partitions now can span over multiple chips.
>
> To describe such a chip's, multiple "reg" tuples are now supported in one
> flash device tree node. Here an dts example:
>
>        flash@f0000000,0 {
>                #address-cells = <1>;
>                #size-cells = <1>;
>                compatible = "cfi-flash";
>                reg = <0 0x00000000 0x02000000
>                       0 0x02000000 0x02000000>;
>                bank-width = <2>;
>                partition@0 {
>                        label = "test-part1";
>                        reg = <0 0x04000000>;
>                };
>        };

Binding looks good to me.  Add a variant of this blurb to
Documentation/powerpc/booting-without-of.txt.  For extra credit,
factor out the MTD stuff and move it to
Documentation/powerpc/dts-bindings/.  Remember to cc: the
devicetree-discuss@ozlabs.org list when you post the binding
documentation.

> Signed-off-by: Stefan Roese <sr@denx.de>
> CC: Grant Likely <grant.likely@secretlab.ca>
> ---
>  drivers/mtd/maps/physmap_of.c |  174 ++++++++++++++++++++++++++++-------------
>  1 files changed, 120 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/mtd/maps/physmap_of.c b/drivers/mtd/maps/physmap_of.c
> index 5fcfec0..c1c2d08 100644
> --- a/drivers/mtd/maps/physmap_of.c
> +++ b/drivers/mtd/maps/physmap_of.c
> @@ -20,13 +20,17 @@
>  #include <linux/mtd/mtd.h>
>  #include <linux/mtd/map.h>
>  #include <linux/mtd/partitions.h>
> +#include <linux/mtd/concat.h>
>  #include <linux/of.h>
>  #include <linux/of_platform.h>
>
> +#define MAX_RESOURCES          4
> +

Why is this static?  Instead you could define:

struct of_flash_list {
        struct mtd_info *mtd;
        struct map_info map;
        struct resource *res;
};

struct of_flash {
        struct mtd_info         *cmtd;
#ifdef CONFIG_MTD_PARTITIONS
        struct mtd_partition    *parts;
#endif
        int list_size; /* number of elements in of_flash_list */
        struct of_flash_list     list[0];
};

Using a zero length array at the end of the structure allows you to do
this after counting the number of reg tuples:

f = kzalloc(sizeof(struct of_flash) + sizeof(struct of_flash_list)*num_chips);

That eliminates a needless hard limit to the number of flash chips.

>  struct of_flash {
> -       struct mtd_info         *mtd;
> -       struct map_info         map;
> -       struct resource         *res;
> +       struct mtd_info         *mtd[MAX_RESOURCES];
> +       struct mtd_info         *cmtd;
> +       struct map_info         map[MAX_RESOURCES];
> +       struct resource         *res[MAX_RESOURCES];
>  #ifdef CONFIG_MTD_PARTITIONS
>        struct mtd_partition    *parts;
>  #endif
> @@ -88,28 +92,40 @@ static int parse_obsolete_partitions(struct of_device *dev,
>  static int of_flash_remove(struct of_device *dev)
>  {
>        struct of_flash *info;
> +       int i;
>
>        info = dev_get_drvdata(&dev->dev);
>        if (!info)
>                return 0;
>        dev_set_drvdata(&dev->dev, NULL);
>
> -       if (info->mtd) {
> +#ifdef CONFIG_MTD_CONCAT
> +       if (info->cmtd != info->mtd[0]) {
> +               del_mtd_device(info->cmtd);
> +               mtd_concat_destroy(info->cmtd);
> +       }
> +#endif
> +
> +       if (info->cmtd) {
>                if (OF_FLASH_PARTS(info)) {
> -                       del_mtd_partitions(info->mtd);
> +                       del_mtd_partitions(info->cmtd);
>                        kfree(OF_FLASH_PARTS(info));
>                } else {
> -                       del_mtd_device(info->mtd);
> +                       del_mtd_device(info->cmtd);
>                }
> -               map_destroy(info->mtd);
>        }
>
> -       if (info->map.virt)
> -               iounmap(info->map.virt);
> +       for (i = 0; i < MAX_RESOURCES; i++) {
> +               if (info->mtd[i])
> +                       map_destroy(info->mtd[i]);
> +
> +               if (info->map[i].virt)
> +                       iounmap(info->map[i].virt);
>
> -       if (info->res) {
> -               release_resource(info->res);
> -               kfree(info->res);
> +               if (info->res[i]) {
> +                       release_resource(info->res[i]);
> +                       kfree(info->res[i]);
> +               }
>        }
>
>        return 0;
> @@ -164,15 +180,25 @@ static int __devinit of_flash_probe(struct of_device *dev,
>        const char *probe_type = match->data;
>        const u32 *width;
>        int err;
> -
> -       err = -ENXIO;
> -       if (of_address_to_resource(dp, 0, &res)) {
> -               dev_err(&dev->dev, "Can't get IO address from device tree\n");
> +       int i;
> +       int count;
> +       const u32 *p;
> +       int devices_found = 0;
> +
> +       /*
> +        * Get number of "reg" tuples. Scan for MTD devices on area's
> +        * described by each "reg" region. This makes it possible (including
> +        * the concat support) to support the Intel P30 48F4400 chips which
> +        * consists internally of 2 non-identical NOR chips on one die.
> +        */
> +       p = of_get_property(dp, "reg", &count);
> +       if (count % 12 != 0) {

This doesn't work.  You cannot know the size of each reg tuple until
#address-cells/#size-cells is parsed for the parent node.  It won't
always be 12.  Use of_n_addr_cells() + of_n_size_cells() to determine
size of each tuple.

Other than that, I think it looks good, but I'll look again when you repost.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.