| Message ID | 1251976558-13463-1-git-send-email-ext-mika.2.korhonen@nokia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, 2009-09-03 at 14:15 +0300, Mika Korhonen wrote: > Add module parameter "parts" to omap2-onenand driver. Parameter format is > the same as for cmdlinepart except mtd-id must not be specified - it > gets prepended by the driver, i.e.: parts=<partdef>[,<partdef>]* > > This allows one to repartition the OneNAND chip and is useful for flashing > applications that do the partitioning from scratch or want to backup and > update the partitioning. > > Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com> > --- > drivers/mtd/cmdlinepart.c | 35 +++++++++++++++++++++++++++++------ > drivers/mtd/onenand/omap2.c | 29 +++++++++++++++++++++++++++++ > 2 files changed, 58 insertions(+), 6 deletions(-) > > diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c > index 1479da6..77fa7b7 100644 > --- a/drivers/mtd/cmdlinepart.c > +++ b/drivers/mtd/cmdlinepart.c > @@ -5,7 +5,7 @@ > * > * The format for the command line is as follows: > * > - * mtdparts=<mtddef>[;<mtddef] > + * mtdparts=<mtddef>[;<mtddef>] > * <mtddef> := <mtd-id>:<partdef>[,<partdef>] > * where <mtd-id> is the name from the "cat /proc/mtd" command > * <partdef> := <size>[@offset][<name>][ro][lk] > @@ -54,7 +54,7 @@ struct cmdline_mtd_partition { > /* mtdpart_setup() parses into here */ > static struct cmdline_mtd_partition *partitions; > > -/* the command line passed to mtdpart_setupd() */ > +/* the command line passed to mtdpart_setup() */ > static char *cmdline; > static int cmdline_parsed = 0; > > @@ -219,9 +219,8 @@ static int mtdpart_setup_real(char *s) > { > cmdline_parsed = 1; > > - for( ; s != NULL; ) > - { > - struct cmdline_mtd_partition *this_mtd; > + for ( ; s != NULL; ) { > + struct cmdline_mtd_partition *this_mtd, *mtd, *mtd_prev; > struct mtd_partition *parts; > int mtd_id_len; > int num_parts; > @@ -270,6 +269,27 @@ static int mtdpart_setup_real(char *s) > this_mtd->mtd_id = (char*)(this_mtd + 1); > strlcpy(this_mtd->mtd_id, mtd_id, mtd_id_len + 1); > > + /* remove existing ones with the same id */ > + mtd_prev = NULL; > + for (mtd = partitions; mtd;) { > + if (strcmp(this_mtd->mtd_id, mtd->mtd_id) == 0) { > + printk(KERN_INFO "old entry for mtd id %s " > + "exists, removing\n", mtd->mtd_id); > + if (mtd_prev) { > + mtd_prev->next = mtd->next; > + kfree(mtd); > + mtd = mtd_prev->next; > + } else { > + partitions = mtd->next; > + kfree(mtd); > + mtd = partitions; > + } > + } else { > + mtd_prev = mtd; > + mtd = mtd->next; > + } > + } > + > /* link into chain */ > this_mtd->next = partitions; > partitions = this_mtd; > @@ -354,12 +374,15 @@ static int parse_cmdline_partitions(struct mtd_info *master, > * > * This function needs to be visible for bootloaders. > */ > -static int mtdpart_setup(char *s) > +int mtdpart_setup(char *s) > { > cmdline = s; > + cmdline_parsed = 0; > return 1; > } This global flag is not nice. Not sure what would be a better way, but may be you have an idea how to do this nicer? > +EXPORT_SYMBOL(mtdpart_setup); > + > __setup("mtdparts=", mtdpart_setup); > > static struct mtd_part_parser cmdline_parser = { > diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c > index 0108ed4..b216a92 100644 > --- a/drivers/mtd/onenand/omap2.c > +++ b/drivers/mtd/onenand/omap2.c > @@ -45,6 +45,7 @@ > #include <mach/board.h> > > #define DRIVER_NAME "omap2-onenand" > +#define DRIVER_NAME_SIZE sizeof(DRIVER_NAME) No need to '#define' this, this is really an unnecessary complication for readability. > > #define ONENAND_IO_SIZE SZ_128K > #define ONENAND_BUFRAM_SIZE (1024 * 5) > @@ -64,6 +65,17 @@ struct omap2_onenand { > int (*setup)(void __iomem *base, int freq); > }; > > + > +#ifdef CONFIG_MTD_CMDLINE_PARTS > +extern int mtdpart_setup(char *); > + > +static const char *part_probes[] = { "cmdlinepart", NULL }; > +static char parts[256] = DRIVER_NAME ":"; > + > +module_param_string(parts, parts + DRIVER_NAME_SIZE, 256 - DRIVER_NAME_SIZE, 0); > +#endif Please, do not make the module parameter to be compiled conditionally. If you add this parameter, I think it should be there all the time. Just use 'select' in Kconfig and make sure the parser is always there. > + > + > static void omap2_onenand_dma_cb(int lch, u16 ch_status, void *data) > { > struct omap2_onenand *c = data; > @@ -709,6 +721,23 @@ static int __devinit omap2_onenand_probe(struct platform_device *pdev) > } > > #ifdef CONFIG_MTD_PARTITIONS > +#ifdef CONFIG_MTD_CMDLINE_PARTS > + printk(KERN_INFO "parts=%s\n", parts); > + /* check module parameter */ > + if (parts[DRIVER_NAME_SIZE] != '\0') { > + /* check parts string */ > + if (strchr(parts, ';') || strchr(parts + DRIVER_NAME_SIZE, ':')) { > + printk(KERN_ERR "onenand_probe: invalid partition parameter\n"); > + } else { > + mtdpart_setup(parts); > + } Unnecessary { } Please, use %s and __func__ instead of hard-coding function names. > + } > + r = parse_mtd_partitions(&c->mtd, part_probes, &c->parts, 0); > + if (r > 0) { > + /* module param or kernel command line arg */ > + r = add_mtd_partitions(&c->mtd, c->parts, r); > + } else > +#endif > if (pdata->parts != NULL) > r = add_mtd_partitions(&c->mtd, pdata->parts, > pdata->nr_parts); This #ifdef injecting is bad, but it should go away if you do what I suggest above.
On Wed, 2009-10-28 at 13:50 +0200, Artem Bityutskiy wrote: > On Thu, 2009-09-03 at 14:15 +0300, Mika Korhonen wrote: > > Add module parameter "parts" to omap2-onenand driver. Parameter format is > > the same as for cmdlinepart except mtd-id must not be specified - it > > gets prepended by the driver, i.e.: parts=<partdef>[,<partdef>]* > > > > This allows one to repartition the OneNAND chip and is useful for flashing > > applications that do the partitioning from scratch or want to backup and > > update the partitioning. > > > > Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com> checkpatch.pl is BTW unhappy.
On Wed, Oct 28, 2009 at 5:20 PM, Artem Bityutskiy <dedekind1@gmail.com> wrote: > On Thu, 2009-09-03 at 14:15 +0300, Mika Korhonen wrote: >> Add module parameter "parts" to omap2-onenand driver. Parameter format is >> the same as for cmdlinepart except mtd-id must not be specified - it >> gets prepended by the driver, i.e.: parts=<partdef>[,<partdef>]* >> >> This allows one to repartition the OneNAND chip and is useful for flashing >> applications that do the partitioning from scratch or want to backup and >> update the partitioning. >> >> Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com> >> --- >> drivers/mtd/cmdlinepart.c | 35 +++++++++++++++++++++++++++++------ >> drivers/mtd/onenand/omap2.c | 29 +++++++++++++++++++++++++++++ >> 2 files changed, 58 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c >> index 1479da6..77fa7b7 100644 >> --- a/drivers/mtd/cmdlinepart.c >> +++ b/drivers/mtd/cmdlinepart.c >> @@ -5,7 +5,7 @@ >> * >> * The format for the command line is as follows: >> * >> - * mtdparts=<mtddef>[;<mtddef] >> + * mtdparts=<mtddef>[;<mtddef>] >> * <mtddef> := <mtd-id>:<partdef>[,<partdef>] >> * where <mtd-id> is the name from the "cat /proc/mtd" command >> * <partdef> := <size>[@offset][<name>][ro][lk] >> @@ -54,7 +54,7 @@ struct cmdline_mtd_partition { >> /* mtdpart_setup() parses into here */ >> static struct cmdline_mtd_partition *partitions; >> >> -/* the command line passed to mtdpart_setupd() */ >> +/* the command line passed to mtdpart_setup() */ >> static char *cmdline; >> static int cmdline_parsed = 0; >> >> @@ -219,9 +219,8 @@ static int mtdpart_setup_real(char *s) >> { >> cmdline_parsed = 1; >> >> - for( ; s != NULL; ) >> - { >> - struct cmdline_mtd_partition *this_mtd; >> + for ( ; s != NULL; ) { >> + struct cmdline_mtd_partition *this_mtd, *mtd, *mtd_prev; >> struct mtd_partition *parts; >> int mtd_id_len; >> int num_parts; >> @@ -270,6 +269,27 @@ static int mtdpart_setup_real(char *s) >> this_mtd->mtd_id = (char*)(this_mtd + 1); >> strlcpy(this_mtd->mtd_id, mtd_id, mtd_id_len + 1); >> >> + /* remove existing ones with the same id */ >> + mtd_prev = NULL; >> + for (mtd = partitions; mtd;) { >> + if (strcmp(this_mtd->mtd_id, mtd->mtd_id) == 0) { >> + printk(KERN_INFO "old entry for mtd id %s " >> + "exists, removing\n", mtd->mtd_id); >> + if (mtd_prev) { >> + mtd_prev->next = mtd->next; >> + kfree(mtd); >> + mtd = mtd_prev->next; >> + } else { >> + partitions = mtd->next; >> + kfree(mtd); >> + mtd = partitions; >> + } >> + } else { >> + mtd_prev = mtd; >> + mtd = mtd->next; >> + } >> + } >> + >> /* link into chain */ >> this_mtd->next = partitions; >> partitions = this_mtd; >> @@ -354,12 +374,15 @@ static int parse_cmdline_partitions(struct mtd_info *master, >> * >> * This function needs to be visible for bootloaders. >> */ >> -static int mtdpart_setup(char *s) >> +int mtdpart_setup(char *s) >> { >> cmdline = s; >> + cmdline_parsed = 0; >> return 1; >> } > > This global flag is not nice. Not sure what would be a better way, but > may be you have an idea how to do this nicer? > >> +EXPORT_SYMBOL(mtdpart_setup); >> + >> __setup("mtdparts=", mtdpart_setup); >> >> static struct mtd_part_parser cmdline_parser = { >> diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c >> index 0108ed4..b216a92 100644 >> --- a/drivers/mtd/onenand/omap2.c >> +++ b/drivers/mtd/onenand/omap2.c >> @@ -45,6 +45,7 @@ >> #include <mach/board.h> >> >> #define DRIVER_NAME "omap2-onenand" >> +#define DRIVER_NAME_SIZE sizeof(DRIVER_NAME) > > No need to '#define' this, this is really an unnecessary complication > for readability. > >> >> #define ONENAND_IO_SIZE SZ_128K >> #define ONENAND_BUFRAM_SIZE (1024 * 5) >> @@ -64,6 +65,17 @@ struct omap2_onenand { >> int (*setup)(void __iomem *base, int freq); >> }; >> >> + >> +#ifdef CONFIG_MTD_CMDLINE_PARTS >> +extern int mtdpart_setup(char *); >> + >> +static const char *part_probes[] = { "cmdlinepart", NULL }; >> +static char parts[256] = DRIVER_NAME ":"; >> + >> +module_param_string(parts, parts + DRIVER_NAME_SIZE, 256 - DRIVER_NAME_SIZE, 0); >> +#endif > > Please, do not make the module parameter to be compiled conditionally. > If you add this parameter, I think it should be there all the time. Just > use 'select' in Kconfig and make sure the parser is always there. > >> + >> + >> static void omap2_onenand_dma_cb(int lch, u16 ch_status, void *data) >> { >> struct omap2_onenand *c = data; >> @@ -709,6 +721,23 @@ static int __devinit omap2_onenand_probe(struct platform_device *pdev) >> } >> >> #ifdef CONFIG_MTD_PARTITIONS >> +#ifdef CONFIG_MTD_CMDLINE_PARTS >> + printk(KERN_INFO "parts=%s\n", parts); >> + /* check module parameter */ >> + if (parts[DRIVER_NAME_SIZE] != '\0') { >> + /* check parts string */ >> + if (strchr(parts, ';') || strchr(parts + DRIVER_NAME_SIZE, ':')) { >> + printk(KERN_ERR "onenand_probe: invalid partition parameter\n"); >> + } else { >> + mtdpart_setup(parts); >> + } > > Unnecessary { } > Please, use %s and __func__ instead of hard-coding function names. > >> + } >> + r = parse_mtd_partitions(&c->mtd, part_probes, &c->parts, 0); >> + if (r > 0) { >> + /* module param or kernel command line arg */ >> + r = add_mtd_partitions(&c->mtd, c->parts, r); >> + } else here too... Unnecessary { } >> +#endif >> if (pdata->parts != NULL) >> r = add_mtd_partitions(&c->mtd, pdata->parts, >> pdata->nr_parts); > > This #ifdef injecting is bad, but it should go away if you do what I > suggest above. >
On Thu, Sep 3, 2009 at 4:45 PM, Mika Korhonen <ext-mika.2.korhonen@nokia.com> wrote: > Add module parameter "parts" to omap2-onenand driver. Parameter format is > the same as for cmdlinepart except mtd-id must not be specified - it > gets prepended by the driver, i.e.: parts=<partdef>[,<partdef>]* > > This allows one to repartition the OneNAND chip and is useful for flashing > applications that do the partitioning from scratch or want to backup and > update the partitioning. > > Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com> > --- > drivers/mtd/cmdlinepart.c | 35 +++++++++++++++++++++++++++++------ > drivers/mtd/onenand/omap2.c | 29 +++++++++++++++++++++++++++++ > 2 files changed, 58 insertions(+), 6 deletions(-) > > diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c > index 1479da6..77fa7b7 100644 > --- a/drivers/mtd/cmdlinepart.c > +++ b/drivers/mtd/cmdlinepart.c > @@ -5,7 +5,7 @@ > * > * The format for the command line is as follows: > * > - * mtdparts=<mtddef>[;<mtddef] > + * mtdparts=<mtddef>[;<mtddef>] > * <mtddef> := <mtd-id>:<partdef>[,<partdef>] > * where <mtd-id> is the name from the "cat /proc/mtd" command > * <partdef> := <size>[@offset][<name>][ro][lk] > @@ -54,7 +54,7 @@ struct cmdline_mtd_partition { > /* mtdpart_setup() parses into here */ > static struct cmdline_mtd_partition *partitions; > > -/* the command line passed to mtdpart_setupd() */ > +/* the command line passed to mtdpart_setup() */ > static char *cmdline; > static int cmdline_parsed = 0; > > @@ -219,9 +219,8 @@ static int mtdpart_setup_real(char *s) > { > cmdline_parsed = 1; > > - for( ; s != NULL; ) > - { > - struct cmdline_mtd_partition *this_mtd; > + for ( ; s != NULL; ) { > + struct cmdline_mtd_partition *this_mtd, *mtd, *mtd_prev; > struct mtd_partition *parts; > int mtd_id_len; > int num_parts; > @@ -270,6 +269,27 @@ static int mtdpart_setup_real(char *s) > this_mtd->mtd_id = (char*)(this_mtd + 1); > strlcpy(this_mtd->mtd_id, mtd_id, mtd_id_len + 1); > > + /* remove existing ones with the same id */ > + mtd_prev = NULL; > + for (mtd = partitions; mtd;) { Space instead of tab. > + if (strcmp(this_mtd->mtd_id, mtd->mtd_id) == 0) { Hmm... I guess you won't get your device id matched here. Since the string you are passing from omap onenand driver is something like this: omap2-onenand:..... while mtd registers device ids in different format, something like: 'omap2-onenand.0'
On Thu, 2009-09-03 at 14:15 +0300, Mika Korhonen wrote: > Add module parameter "parts" to omap2-onenand driver. Parameter format is > the same as for cmdlinepart except mtd-id must not be specified - it > gets prepended by the driver, i.e.: parts=<partdef>[,<partdef>]* > > This allows one to repartition the OneNAND chip and is useful for flashing > applications that do the partitioning from scratch or want to backup and > update the partitioning. > > Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com> > --- > drivers/mtd/cmdlinepart.c | 35 +++++++++++++++++++++++++++++------ > drivers/mtd/onenand/omap2.c | 29 +++++++++++++++++++++++++++++ > 2 files changed, 58 insertions(+), 6 deletions(-) This should not be onenand module parameters actually. This re-partitioning should be done via an mtd device ioctl instead. Could you try to introduce a new mtd ioctl? I know the partitioning in mtd is ugly, so you may hit some challenges. E.g., all these special cases like #ifdef CONFIG_MTD_PARTITIONS /* Deregister partitions */ del_mtd_partitions (mtd); #endif /* Deregister the device */ del_mtd_device (mtd); make no sense and should die. We should always have partitioning support instead. So the mtdpart module should also die and partitioning support should become part of mtdcore.
Artem Bityutskiy wrote: > On Thu, 2009-09-03 at 14:15 +0300, Mika Korhonen wrote: > >> Add module parameter "parts" to omap2-onenand driver. Parameter format is >> the same as for cmdlinepart except mtd-id must not be specified - it >> gets prepended by the driver, i.e.: parts=<partdef>[,<partdef>]* >> >> This allows one to repartition the OneNAND chip and is useful for flashing >> applications that do the partitioning from scratch or want to backup and >> update the partitioning. >> >> Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com> >> --- >> drivers/mtd/cmdlinepart.c | 35 +++++++++++++++++++++++++++++------ >> drivers/mtd/onenand/omap2.c | 29 +++++++++++++++++++++++++++++ >> 2 files changed, 58 insertions(+), 6 deletions(-) >> > > This should not be onenand module parameters actually. This > re-partitioning should be done via an mtd device ioctl instead. > > Could you try to introduce a new mtd ioctl? > > I know the partitioning in mtd is ugly, so you may hit some challenges. > E.g., all these special cases like > > #ifdef CONFIG_MTD_PARTITIONS > /* Deregister partitions */ > del_mtd_partitions (mtd); > #endif > /* Deregister the device */ > del_mtd_device (mtd); > > make no sense and should die. We should always have partitioning support > instead. So the mtdpart module should also die and partitioning support > should become part of mtdcore. > > I agree, actually my first intention to was to make it more generic but the framework indeed would have needed non-minor rework, so I took the easy route to get started. Mika
ext Vimal Singh wrote: > On Thu, Sep 3, 2009 at 4:45 PM, Mika Korhonen > <ext-mika.2.korhonen@nokia.com> wrote: > >> Add module parameter "parts" to omap2-onenand driver. Parameter format is >> the same as for cmdlinepart except mtd-id must not be specified - it >> gets prepended by the driver, i.e.: parts=<partdef>[,<partdef>]* >> >> This allows one to repartition the OneNAND chip and is useful for flashing >> applications that do the partitioning from scratch or want to backup and >> update the partitioning. >> >> Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com> >> --- >> drivers/mtd/cmdlinepart.c | 35 +++++++++++++++++++++++++++++------ >> drivers/mtd/onenand/omap2.c | 29 +++++++++++++++++++++++++++++ >> 2 files changed, 58 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c >> index 1479da6..77fa7b7 100644 >> --- a/drivers/mtd/cmdlinepart.c >> +++ b/drivers/mtd/cmdlinepart.c >> @@ -5,7 +5,7 @@ >> * >> * The format for the command line is as follows: >> * >> - * mtdparts=<mtddef>[;<mtddef] >> + * mtdparts=<mtddef>[;<mtddef>] >> * <mtddef> := <mtd-id>:<partdef>[,<partdef>] >> * where <mtd-id> is the name from the "cat /proc/mtd" command >> * <partdef> := <size>[@offset][<name>][ro][lk] >> @@ -54,7 +54,7 @@ struct cmdline_mtd_partition { >> /* mtdpart_setup() parses into here */ >> static struct cmdline_mtd_partition *partitions; >> >> -/* the command line passed to mtdpart_setupd() */ >> +/* the command line passed to mtdpart_setup() */ >> static char *cmdline; >> static int cmdline_parsed = 0; >> >> @@ -219,9 +219,8 @@ static int mtdpart_setup_real(char *s) >> { >> cmdline_parsed = 1; >> >> - for( ; s != NULL; ) >> - { >> - struct cmdline_mtd_partition *this_mtd; >> + for ( ; s != NULL; ) { >> + struct cmdline_mtd_partition *this_mtd, *mtd, *mtd_prev; >> struct mtd_partition *parts; >> int mtd_id_len; >> int num_parts; >> @@ -270,6 +269,27 @@ static int mtdpart_setup_real(char *s) >> this_mtd->mtd_id = (char*)(this_mtd + 1); >> strlcpy(this_mtd->mtd_id, mtd_id, mtd_id_len + 1); >> >> + /* remove existing ones with the same id */ >> + mtd_prev = NULL; >> + for (mtd = partitions; mtd;) { >> > Space instead of tab. > > >> + if (strcmp(this_mtd->mtd_id, mtd->mtd_id) == 0) { >> > Hmm... I guess you won't get your device id matched here. Since the > string you are passing from omap onenand driver is something like > this: > omap2-onenand:..... > > while mtd registers device ids in different format, something like: > 'omap2-onenand.0' > > It's been tested, and it matches. In parse_cmdline_partitions() mtd_id is set from mtd_info.name of the chip. Could be that cmdlinepart.c is outdated, though.
On Tue, Nov 10, 2009 at 11:48 AM, Mika Korhonen <ext-mika.2.korhonen@nokia.com> wrote: > ext Vimal Singh wrote: >> >> On Thu, Sep 3, 2009 at 4:45 PM, Mika Korhonen >> <ext-mika.2.korhonen@nokia.com> wrote: >> >>> >>> Add module parameter "parts" to omap2-onenand driver. Parameter format is >>> the same as for cmdlinepart except mtd-id must not be specified - it >>> gets prepended by the driver, i.e.: parts=<partdef>[,<partdef>]* >>> >>> This allows one to repartition the OneNAND chip and is useful for >>> flashing >>> applications that do the partitioning from scratch or want to backup and >>> update the partitioning. >>> >>> Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com> >>> --- >>> drivers/mtd/cmdlinepart.c | 35 +++++++++++++++++++++++++++++------ >>> drivers/mtd/onenand/omap2.c | 29 +++++++++++++++++++++++++++++ >>> 2 files changed, 58 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c >>> index 1479da6..77fa7b7 100644 >>> --- a/drivers/mtd/cmdlinepart.c >>> +++ b/drivers/mtd/cmdlinepart.c >>> @@ -5,7 +5,7 @@ >>> * >>> * The format for the command line is as follows: >>> * >>> - * mtdparts=<mtddef>[;<mtddef] >>> + * mtdparts=<mtddef>[;<mtddef>] >>> * <mtddef> := <mtd-id>:<partdef>[,<partdef>] >>> * where <mtd-id> is the name from the "cat /proc/mtd" >>> command >>> * <partdef> := <size>[@offset][<name>][ro][lk] >>> @@ -54,7 +54,7 @@ struct cmdline_mtd_partition { >>> /* mtdpart_setup() parses into here */ >>> static struct cmdline_mtd_partition *partitions; >>> >>> -/* the command line passed to mtdpart_setupd() */ >>> +/* the command line passed to mtdpart_setup() */ >>> static char *cmdline; >>> static int cmdline_parsed = 0; >>> >>> @@ -219,9 +219,8 @@ static int mtdpart_setup_real(char *s) >>> { >>> cmdline_parsed = 1; >>> >>> - for( ; s != NULL; ) >>> - { >>> - struct cmdline_mtd_partition *this_mtd; >>> + for ( ; s != NULL; ) { >>> + struct cmdline_mtd_partition *this_mtd, *mtd, *mtd_prev; >>> struct mtd_partition *parts; >>> int mtd_id_len; >>> int num_parts; >>> @@ -270,6 +269,27 @@ static int mtdpart_setup_real(char *s) >>> this_mtd->mtd_id = (char*)(this_mtd + 1); >>> strlcpy(this_mtd->mtd_id, mtd_id, mtd_id_len + 1); >>> >>> + /* remove existing ones with the same id */ >>> + mtd_prev = NULL; >>> + for (mtd = partitions; mtd;) { >>> >> >> Space instead of tab. >> >> >>> >>> + if (strcmp(this_mtd->mtd_id, mtd->mtd_id) == 0) { >>> >> >> Hmm... I guess you won't get your device id matched here. Since the >> string you are passing from omap onenand driver is something like >> this: >> omap2-onenand:..... >> >> while mtd registers device ids in different format, something like: >> 'omap2-onenand.0' >> >> > > It's been tested, and it matches. In parse_cmdline_partitions() mtd_id is > set from mtd_info.name of the chip. Could be that cmdlinepart.c is outdated, > though. I am still not convinced. I use to get below prints when I boot up (this is for NAND and should be same for OneNAND too): ... omap2-nand driver initializing NAND device: Manufacturer ID: 0x2c, Chip ID: 0xbc (Micron NAND 512MiB 1,8V 16-bit) Creating 7 MTD partitions on "omap2-nand.0": ... This print comes from 'add_mtd_partitions' (drivers/mtd/mtdpart.c): printk(KERN_NOTICE "Creating %d MTD partitions on \"%s\":\n", nbparts, master->name); And I use to give cmdline like this: "mtdparts=omap2-nand.0:512k@0(p1),1280k@512k(p2),768k@1792k(p3),5m@2560k(p4),123392k@7680k(p5)" and this works for me.
ext Vimal Singh wrote: > On Tue, Nov 10, 2009 at 11:48 AM, Mika Korhonen > <ext-mika.2.korhonen@nokia.com> wrote: > >> ext Vimal Singh wrote: >> >>> On Thu, Sep 3, 2009 at 4:45 PM, Mika Korhonen >>> <ext-mika.2.korhonen@nokia.com> wrote: >>> >>> >>>> Add module parameter "parts" to omap2-onenand driver. Parameter format is >>>> the same as for cmdlinepart except mtd-id must not be specified - it >>>> gets prepended by the driver, i.e.: parts=<partdef>[,<partdef>]* >>>> >>>> This allows one to repartition the OneNAND chip and is useful for >>>> flashing >>>> applications that do the partitioning from scratch or want to backup and >>>> update the partitioning. >>>> >>>> Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com> >>>> --- >>>> drivers/mtd/cmdlinepart.c | 35 +++++++++++++++++++++++++++++------ >>>> drivers/mtd/onenand/omap2.c | 29 +++++++++++++++++++++++++++++ >>>> 2 files changed, 58 insertions(+), 6 deletions(-) >>>> >>>> diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c >>>> index 1479da6..77fa7b7 100644 >>>> --- a/drivers/mtd/cmdlinepart.c >>>> +++ b/drivers/mtd/cmdlinepart.c >>>> @@ -5,7 +5,7 @@ >>>> * >>>> * The format for the command line is as follows: >>>> * >>>> - * mtdparts=<mtddef>[;<mtddef] >>>> + * mtdparts=<mtddef>[;<mtddef>] >>>> * <mtddef> := <mtd-id>:<partdef>[,<partdef>] >>>> * where <mtd-id> is the name from the "cat /proc/mtd" >>>> command >>>> * <partdef> := <size>[@offset][<name>][ro][lk] >>>> @@ -54,7 +54,7 @@ struct cmdline_mtd_partition { >>>> /* mtdpart_setup() parses into here */ >>>> static struct cmdline_mtd_partition *partitions; >>>> >>>> -/* the command line passed to mtdpart_setupd() */ >>>> +/* the command line passed to mtdpart_setup() */ >>>> static char *cmdline; >>>> static int cmdline_parsed = 0; >>>> >>>> @@ -219,9 +219,8 @@ static int mtdpart_setup_real(char *s) >>>> { >>>> cmdline_parsed = 1; >>>> >>>> - for( ; s != NULL; ) >>>> - { >>>> - struct cmdline_mtd_partition *this_mtd; >>>> + for ( ; s != NULL; ) { >>>> + struct cmdline_mtd_partition *this_mtd, *mtd, *mtd_prev; >>>> struct mtd_partition *parts; >>>> int mtd_id_len; >>>> int num_parts; >>>> @@ -270,6 +269,27 @@ static int mtdpart_setup_real(char *s) >>>> this_mtd->mtd_id = (char*)(this_mtd + 1); >>>> strlcpy(this_mtd->mtd_id, mtd_id, mtd_id_len + 1); >>>> >>>> + /* remove existing ones with the same id */ >>>> + mtd_prev = NULL; >>>> + for (mtd = partitions; mtd;) { >>>> >>>> >>> Space instead of tab. >>> >>> >>> >>>> + if (strcmp(this_mtd->mtd_id, mtd->mtd_id) == 0) { >>>> >>>> >>> Hmm... I guess you won't get your device id matched here. Since the >>> string you are passing from omap onenand driver is something like >>> this: >>> omap2-onenand:..... >>> >>> while mtd registers device ids in different format, something like: >>> 'omap2-onenand.0' >>> >>> >>> >> It's been tested, and it matches. In parse_cmdline_partitions() mtd_id is >> set from mtd_info.name of the chip. Could be that cmdlinepart.c is outdated, >> though. >> > > I am still not convinced. I use to get below prints when I boot up > (this is for NAND and should be same for OneNAND too): > ... > omap2-nand driver initializing > NAND device: Manufacturer ID: 0x2c, Chip ID: 0xbc (Micron NAND 512MiB > 1,8V 16-bit) > Creating 7 MTD partitions on "omap2-nand.0": > ... > This print comes from 'add_mtd_partitions' (drivers/mtd/mtdpart.c): > printk(KERN_NOTICE "Creating %d MTD partitions on \"%s\":\n", nbparts, > master->name); > > And I use to give cmdline like this: > "mtdparts=omap2-nand.0:512k@0(p1),1280k@512k(p2),768k@1792k(p3),5m@2560k(p4),123392k@7680k(p5)" > > and this works for me. > > It does not seem to be the same for OneNAND: Creating 5 MTD partitions on "omap2-onenand": 0x00000000-0x00020000 : "p1" 0x00020000-0x00080000 : "p2" ... Another question is, if it should be. br, Mika
Korhonen Mika.2 (EXT-Ardites/Oulu) wrote: > Artem Bityutskiy wrote: >> On Thu, 2009-09-03 at 14:15 +0300, Mika Korhonen wrote: >> >>> Add module parameter "parts" to omap2-onenand driver. Parameter format is >>> the same as for cmdlinepart except mtd-id must not be specified - it >>> gets prepended by the driver, i.e.: parts=<partdef>[,<partdef>]* >>> >>> This allows one to repartition the OneNAND chip and is useful for flashing >>> applications that do the partitioning from scratch or want to backup and >>> update the partitioning. >>> >>> Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com> >>> --- >>> drivers/mtd/cmdlinepart.c | 35 +++++++++++++++++++++++++++++------ >>> drivers/mtd/onenand/omap2.c | 29 +++++++++++++++++++++++++++++ >>> 2 files changed, 58 insertions(+), 6 deletions(-) >>> >> This should not be onenand module parameters actually. This >> re-partitioning should be done via an mtd device ioctl instead. >> >> Could you try to introduce a new mtd ioctl? >> >> I know the partitioning in mtd is ugly, so you may hit some challenges. >> E.g., all these special cases like >> >> #ifdef CONFIG_MTD_PARTITIONS >> /* Deregister partitions */ >> del_mtd_partitions (mtd); >> #endif >> /* Deregister the device */ >> del_mtd_device (mtd); >> >> make no sense and should die. We should always have partitioning support >> instead. So the mtdpart module should also die and partitioning support >> should become part of mtdcore. >> >> > I agree, actually my first intention to was to make it more generic but > the framework indeed would have needed non-minor rework, so I took the > easy route to get started. Could we just have the cmdlinepart change for now and make re-partitioning a separate issue?
diff --git a/drivers/mtd/cmdlinepart.c b/drivers/mtd/cmdlinepart.c index 1479da6..77fa7b7 100644 --- a/drivers/mtd/cmdlinepart.c +++ b/drivers/mtd/cmdlinepart.c @@ -5,7 +5,7 @@ * * The format for the command line is as follows: * - * mtdparts=<mtddef>[;<mtddef] + * mtdparts=<mtddef>[;<mtddef>] * <mtddef> := <mtd-id>:<partdef>[,<partdef>] * where <mtd-id> is the name from the "cat /proc/mtd" command * <partdef> := <size>[@offset][<name>][ro][lk] @@ -54,7 +54,7 @@ struct cmdline_mtd_partition { /* mtdpart_setup() parses into here */ static struct cmdline_mtd_partition *partitions; -/* the command line passed to mtdpart_setupd() */ +/* the command line passed to mtdpart_setup() */ static char *cmdline; static int cmdline_parsed = 0; @@ -219,9 +219,8 @@ static int mtdpart_setup_real(char *s) { cmdline_parsed = 1; - for( ; s != NULL; ) - { - struct cmdline_mtd_partition *this_mtd; + for ( ; s != NULL; ) { + struct cmdline_mtd_partition *this_mtd, *mtd, *mtd_prev; struct mtd_partition *parts; int mtd_id_len; int num_parts; @@ -270,6 +269,27 @@ static int mtdpart_setup_real(char *s) this_mtd->mtd_id = (char*)(this_mtd + 1); strlcpy(this_mtd->mtd_id, mtd_id, mtd_id_len + 1); + /* remove existing ones with the same id */ + mtd_prev = NULL; + for (mtd = partitions; mtd;) { + if (strcmp(this_mtd->mtd_id, mtd->mtd_id) == 0) { + printk(KERN_INFO "old entry for mtd id %s " + "exists, removing\n", mtd->mtd_id); + if (mtd_prev) { + mtd_prev->next = mtd->next; + kfree(mtd); + mtd = mtd_prev->next; + } else { + partitions = mtd->next; + kfree(mtd); + mtd = partitions; + } + } else { + mtd_prev = mtd; + mtd = mtd->next; + } + } + /* link into chain */ this_mtd->next = partitions; partitions = this_mtd; @@ -354,12 +374,15 @@ static int parse_cmdline_partitions(struct mtd_info *master, * * This function needs to be visible for bootloaders. */ -static int mtdpart_setup(char *s) +int mtdpart_setup(char *s) { cmdline = s; + cmdline_parsed = 0; return 1; } +EXPORT_SYMBOL(mtdpart_setup); + __setup("mtdparts=", mtdpart_setup); static struct mtd_part_parser cmdline_parser = { diff --git a/drivers/mtd/onenand/omap2.c b/drivers/mtd/onenand/omap2.c index 0108ed4..b216a92 100644 --- a/drivers/mtd/onenand/omap2.c +++ b/drivers/mtd/onenand/omap2.c @@ -45,6 +45,7 @@ #include <mach/board.h> #define DRIVER_NAME "omap2-onenand" +#define DRIVER_NAME_SIZE sizeof(DRIVER_NAME) #define ONENAND_IO_SIZE SZ_128K #define ONENAND_BUFRAM_SIZE (1024 * 5) @@ -64,6 +65,17 @@ struct omap2_onenand { int (*setup)(void __iomem *base, int freq); }; + +#ifdef CONFIG_MTD_CMDLINE_PARTS +extern int mtdpart_setup(char *); + +static const char *part_probes[] = { "cmdlinepart", NULL }; +static char parts[256] = DRIVER_NAME ":"; + +module_param_string(parts, parts + DRIVER_NAME_SIZE, 256 - DRIVER_NAME_SIZE, 0); +#endif + + static void omap2_onenand_dma_cb(int lch, u16 ch_status, void *data) { struct omap2_onenand *c = data; @@ -709,6 +721,23 @@ static int __devinit omap2_onenand_probe(struct platform_device *pdev) } #ifdef CONFIG_MTD_PARTITIONS +#ifdef CONFIG_MTD_CMDLINE_PARTS + printk(KERN_INFO "parts=%s\n", parts); + /* check module parameter */ + if (parts[DRIVER_NAME_SIZE] != '\0') { + /* check parts string */ + if (strchr(parts, ';') || strchr(parts + DRIVER_NAME_SIZE, ':')) { + printk(KERN_ERR "onenand_probe: invalid partition parameter\n"); + } else { + mtdpart_setup(parts); + } + } + r = parse_mtd_partitions(&c->mtd, part_probes, &c->parts, 0); + if (r > 0) { + /* module param or kernel command line arg */ + r = add_mtd_partitions(&c->mtd, c->parts, r); + } else +#endif if (pdata->parts != NULL) r = add_mtd_partitions(&c->mtd, pdata->parts, pdata->nr_parts);
Add module parameter "parts" to omap2-onenand driver. Parameter format is the same as for cmdlinepart except mtd-id must not be specified - it gets prepended by the driver, i.e.: parts=<partdef>[,<partdef>]* This allows one to repartition the OneNAND chip and is useful for flashing applications that do the partitioning from scratch or want to backup and update the partitioning. Signed-off-by: Mika Korhonen <ext-mika.2.korhonen@nokia.com> --- drivers/mtd/cmdlinepart.c | 35 +++++++++++++++++++++++++++++------ drivers/mtd/onenand/omap2.c | 29 +++++++++++++++++++++++++++++ 2 files changed, 58 insertions(+), 6 deletions(-)