* Re: [PATCH] mtd: physmap_of: Add multiple regions and concatenation support
[not found] <1238752534-7718-1-git-send-email-sr@denx.de>
@ 2009-04-03 14:04 ` Grant Likely
2009-04-06 7:43 ` Stefan Roese
0 siblings, 1 reply; 2+ messages in thread
From: Grant Likely @ 2009-04-03 14:04 UTC (permalink / raw)
To: Stefan Roese; +Cc: linuxppc-dev, devicetree-discuss, linux-mtd
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.
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH] mtd: physmap_of: Add multiple regions and concatenation support
2009-04-03 14:04 ` [PATCH] mtd: physmap_of: Add multiple regions and concatenation support Grant Likely
@ 2009-04-06 7:43 ` Stefan Roese
0 siblings, 0 replies; 2+ messages in thread
From: Stefan Roese @ 2009-04-06 7:43 UTC (permalink / raw)
To: Grant Likely; +Cc: linuxppc-dev, devicetree-discuss, linux-mtd
On Friday 03 April 2009, Grant Likely wrote:
> > 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.
OK, will do.
> > 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?
Because I cloned it from physmap.c.
> 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.
Good idea. Will update. Thanks.
> > 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.
OK, I'll change it in the next patch version.
Thanks.
Best regards,
Stefan
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2009-04-06 7:43 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1238752534-7718-1-git-send-email-sr@denx.de>
2009-04-03 14:04 ` [PATCH] mtd: physmap_of: Add multiple regions and concatenation support Grant Likely
2009-04-06 7:43 ` Stefan Roese
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox