[PATCH] MTD OneNAND OMAP2/3: allow giving partition layout as module parameter

[PATCH] MTD OneNAND OMAP2/3: allow giving partition layout as module parameter

[Mika Korhonen]

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;
 }
 
+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);
-- 
1.6.0.4

Re: [PATCH] MTD OneNAND OMAP2/3: allow giving partition layout as module parameter

[Artem Bityutskiy]

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.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH] MTD OneNAND OMAP2/3: allow giving partition layout as module parameter

[Artem Bityutskiy]

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.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH] MTD OneNAND OMAP2/3: allow giving partition layout as module parameter

[Vimal Singh]

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

-- 
Regards,
Vimal Singh

Re: [PATCH] MTD OneNAND OMAP2/3: allow giving partition layout as module parameter

[Vimal Singh]

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'

-- 
Regards,
Vimal Singh

[snip]

> +
> +#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);
> --
> 1.6.0.4
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/
>

Re: [PATCH] MTD OneNAND OMAP2/3: allow giving partition layout as module parameter

[Mika Korhonen]

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.

-- 
Mika

Re: [PATCH] MTD OneNAND OMAP2/3: allow giving partition layout as module parameter

[Vimal Singh]

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.

-- 
Regards,
Vimal Singh

Re: [PATCH] MTD OneNAND OMAP2/3: allow giving partition layout as module parameter

[Mika Korhonen]

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

Re: [PATCH] MTD OneNAND OMAP2/3: allow giving partition layout as module parameter

[Artem Bityutskiy]

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.

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [PATCH] MTD OneNAND OMAP2/3: allow giving partition layout as module parameter

[Mika Korhonen]

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

Re: [PATCH] MTD OneNAND OMAP2/3: allow giving partition layout as module parameter

[Adrian Hunter]

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?