[PATCH 4/4] MTD: include ar7part in the list of partitions parsers

[PATCH 4/4] MTD: include ar7part in the list of partitions parsers

[Florian Fainelli]

This patch modifies the physmap-flash driver to include
the ar7part partition parser in the list of parsers to
use when a physmap-flash driver is registered. This is
required for AR7 to create partitions correctly.

Signed-off-by: Florian Fainelli <florian@openwrt.org>
---
diff --git a/drivers/mtd/maps/physmap.c b/drivers/mtd/maps/physmap.c
index d9603f7..0e65ee7 100644
--- a/drivers/mtd/maps/physmap.c
+++ b/drivers/mtd/maps/physmap.c
@@ -79,7 +79,7 @@ static const char *rom_probe_types[] = {
 					"map_rom",
 					NULL };
 #ifdef CONFIG_MTD_PARTITIONS
-static const char *part_probe_types[] = { "cmdlinepart", "RedBoot", NULL };
+static const char *part_probe_types[] = { "cmdlinepart", "RedBoot", "ar7part", NULL };
 #endif
 
 static int physmap_flash_probe(struct platform_device *dev)

Re: [PATCH 4/4] MTD: include ar7part in the list of partitions parsers

[David Woodhouse]

On Sun, 2010-01-03 at 21:17 +0100, Florian Fainelli wrote:
> This patch modifies the physmap-flash driver to include
> the ar7part partition parser in the list of parsers to
> use when a physmap-flash driver is registered. This is
> required for AR7 to create partitions correctly.

Hrm, perhaps we'd do better to allow the probe types to be specified in
the platform physmap_flash_data?

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

Re: [PATCH 4/4] MTD: include ar7part in the list of partitions parsers

[Alexander Clouter]

In gmane.linux.drivers.mtd David Woodhouse <dwmw2@infradead.org> wrote:
>
> On Sun, 2010-01-03 at 21:17 +0100, Florian Fainelli wrote:
>> This patch modifies the physmap-flash driver to include
>> the ar7part partition parser in the list of parsers to
>> use when a physmap-flash driver is registered. This is
>> required for AR7 to create partitions correctly.
> 
> Hrm, perhaps we'd do better to allow the probe types to be specified in
> the platform physmap_flash_data?
> 
I am not completely convinced the ar7part approach is the best way as 
you are:
 1) not actually reading a partition table and instead using a bunch of 
	'magic' values to try to work out the partition layout
 2) it bears no resemble to what is seen by the ADAM2 bootloader
 3) regardless of whether you would actually want to, the user is unable 
	to amend the partition table and have Linux automagically set 
	the table apprioately

I chatted with Florian a while back about this but we never continued 
with it (Real Life[tm] seemed to get in the way) but I had cobbled the 
following together:
----
diff --git a/arch/mips/ar7/platform.c b/arch/mips/ar7/platform.c
index e2278c0..df33fea 100644
--- a/arch/mips/ar7/platform.c
+++ b/arch/mips/ar7/platform.c
@@ -24,6 +24,7 @@
 #include <linux/dma-mapping.h>
 #include <linux/platform_device.h>
 #include <linux/mtd/physmap.h>
+#include <linux/mtd/partitions.h>
 #include <linux/serial.h>
 #include <linux/serial_8250.h>
 #include <linux/ioport.h>
@@ -467,6 +468,60 @@ static void cpmac_get_mac(int instance, unsigned char *dev_addr)
 			char2hex(mac[i * 3 + 1]);
 }
 
+static void __init detect_partitions(void)
+{
+	unsigned int i, start, end;
+	int ret;
+	char mtdenv[5], *buf;
+
+	for (physmap_flash_data.nr_parts = 0;
+			physmap_flash_data.nr_parts < 10;
+			physmap_flash_data.nr_parts++) {
+		sprintf(mtdenv, "mtd%d", physmap_flash_data.nr_parts);
+		if (!prom_getenv(mtdenv))
+			break;
+	}
+
+	if (physmap_flash_data.nr_parts == 0) {
+		printk(KERN_INFO "No partitions found for physmap MTD\n");
+		return;
+	}
+	if (physmap_flash_data.nr_parts == 10)
+		printk(KERN_INFO "Reached nr_parts limit for physmap MTD\n");
+
+	physmap_flash_data.parts = kzalloc(
+			sizeof(struct mtd_partition)*physmap_flash_data.nr_parts,
+			GFP_KERNEL);
+	if (!physmap_flash_data.parts) {
+		printk(KERN_ERR "Unable to alloc physmap MTD parts struct\n");
+		return;
+	}
+
+	for (i = 0; i < physmap_flash_data.nr_parts; i++) {
+		sprintf(mtdenv, "mtd%d", i);
+		buf = prom_getenv(mtdenv);
+
+		ret = sscanf(buf, "%x,%x", &start, &end);
+		if (ret != 2 || start < 0x90000000 || start > 0x90400000
+				|| end < 0x90000000 || end > 0x90400000
+				|| start > end) {
+			printk(KERN_WARNING "broken partition table for physmap\n");
+			kfree(physmap_flash_data.parts);
+			physmap_flash_data.parts = NULL;
+			physmap_flash_data.nr_parts = 0;
+			return;
+		}
+
+		start -= 0x90000000;
+		end -= 0x90000000;
+
+		physmap_flash_data.parts[i].name	= NULL;
+		physmap_flash_data.parts[i].size	= end - start;
+		physmap_flash_data.parts[i].offset	= start;
+		physmap_flash_data.parts[i].mask_flags	= MTD_WRITEABLE;
+	}
+}
+
 static void __init detect_leds(void)
 {
 	char *prid, *usb_prod;
@@ -536,6 +591,7 @@ static int __init ar7_register_devices(void)
 			return res;
 	}
 #endif /* CONFIG_SERIAL_8250 */
+	detect_partitions();
 	res = platform_device_register(&physmap_flash);
 	if (res)
 		return res;
----

It simply pulls apart the 'PROM' (aka ADAM2) config and uses that to 
build the partition table.

Work's For Me[tm].

Just my thoughts.

Cheers

-- 
Alexander Clouter
.sigmonster says: BOFH excuse #12:
                  dry joints on cable plug

Re: [PATCH 4/4] MTD: include ar7part in the list of partitions parsers

[Florian Fainelli]

Hi Alexander,

On Sunday 03 January 2010 22:31:46 Alexander Clouter wrote:
> In gmane.linux.drivers.mtd David Woodhouse <dwmw2@infradead.org> wrote:
> > On Sun, 2010-01-03 at 21:17 +0100, Florian Fainelli wrote:
> >> This patch modifies the physmap-flash driver to include
> >> the ar7part partition parser in the list of parsers to
> >> use when a physmap-flash driver is registered. This is
> >> required for AR7 to create partitions correctly.
> >
> > Hrm, perhaps we'd do better to allow the probe types to be specified in
> > the platform physmap_flash_data?
> 
> I am not completely convinced the ar7part approach is the best way as
> you are:
>  1) not actually reading a partition table and instead using a bunch of
> 	'magic' values to try to work out the partition layout
>  2) it bears no resemble to what is seen by the ADAM2 bootloader
>  3) regardless of whether you would actually want to, the user is unable
> 	to amend the partition table and have Linux automagically set
> 	the table apprioately
> 
> I chatted with Florian a while back about this but we never continued
> with it (Real Life[tm] seemed to get in the way) but I had cobbled the
> following together:
> ----
> diff --git a/arch/mips/ar7/platform.c b/arch/mips/ar7/platform.c
> index e2278c0..df33fea 100644
> --- a/arch/mips/ar7/platform.c
> +++ b/arch/mips/ar7/platform.c
> @@ -24,6 +24,7 @@
>  #include <linux/dma-mapping.h>
>  #include <linux/platform_device.h>
>  #include <linux/mtd/physmap.h>
> +#include <linux/mtd/partitions.h>
>  #include <linux/serial.h>
>  #include <linux/serial_8250.h>
>  #include <linux/ioport.h>
> @@ -467,6 +468,60 @@ static void cpmac_get_mac(int instance, unsigned char
>  *dev_addr) char2hex(mac[i * 3 + 1]);
>  }
> 
> +static void __init detect_partitions(void)
> +{
> +	unsigned int i, start, end;
> +	int ret;
> +	char mtdenv[5], *buf;
> +
> +	for (physmap_flash_data.nr_parts = 0;
> +			physmap_flash_data.nr_parts < 10;
> +			physmap_flash_data.nr_parts++) {
> +		sprintf(mtdenv, "mtd%d", physmap_flash_data.nr_parts);
> +		if (!prom_getenv(mtdenv))
> +			break;
> +	}
> +
> +	if (physmap_flash_data.nr_parts == 0) {
> +		printk(KERN_INFO "No partitions found for physmap MTD\n");
> +		return;
> +	}
> +	if (physmap_flash_data.nr_parts == 10)
> +		printk(KERN_INFO "Reached nr_parts limit for physmap MTD\n");
> +
> +	physmap_flash_data.parts = kzalloc(
> +			sizeof(struct mtd_partition)*physmap_flash_data.nr_parts,
> +			GFP_KERNEL);
> +	if (!physmap_flash_data.parts) {
> +		printk(KERN_ERR "Unable to alloc physmap MTD parts struct\n");
> +		return;
> +	}
> +
> +	for (i = 0; i < physmap_flash_data.nr_parts; i++) {
> +		sprintf(mtdenv, "mtd%d", i);
> +		buf = prom_getenv(mtdenv);
> +
> +		ret = sscanf(buf, "%x,%x", &start, &end);
> +		if (ret != 2 || start < 0x90000000 || start > 0x90400000
> +				|| end < 0x90000000 || end > 0x90400000
> +				|| start > end) {
> +			printk(KERN_WARNING "broken partition table for physmap\n");
> +			kfree(physmap_flash_data.parts);
> +			physmap_flash_data.parts = NULL;
> +			physmap_flash_data.nr_parts = 0;
> +			return;
> +		}
> +
> +		start -= 0x90000000;
> +		end -= 0x90000000;
> +
> +		physmap_flash_data.parts[i].name	= NULL;
> +		physmap_flash_data.parts[i].size	= end - start;
> +		physmap_flash_data.parts[i].offset	= start;
> +		physmap_flash_data.parts[i].mask_flags	= MTD_WRITEABLE;
> +	}
> +}
> +
>  static void __init detect_leds(void)
>  {
>  	char *prid, *usb_prod;
> @@ -536,6 +591,7 @@ static int __init ar7_register_devices(void)
>  			return res;
>  	}
>  #endif /* CONFIG_SERIAL_8250 */
> +	detect_partitions();
>  	res = platform_device_register(&physmap_flash);
>  	if (res)
>  		return res;
> ----
> 
> It simply pulls apart the 'PROM' (aka ADAM2) config and uses that to
> build the partition table.

This is indeed simple but if I recall right the rationale behind ar7part was 
to create a sane partition layout no matter if the bootloader was ADAM2 or 
PSPBoot and the root filesystem type. JFFS2 and squashfs do not have the same 
erase-block size alignment constraints, ar7part deals with that too.

When we last chatted about that I did not recall exactly those details.
-- 
Regards, Florian

Re: [PATCH 4/4] MTD: include ar7part in the list of partitions parsers

[Alexander Clouter]

In gmane.linux.ports.mips.general Florian Fainelli <florian@openwrt.org> wrote:
>
>> [snipped]
>> 
>> It simply pulls apart the 'PROM' (aka ADAM2) config and uses that to
>> build the partition table.
> 
> This is indeed simple but if I recall right the rationale behind ar7part was 
> to create a sane partition layout no matter if the bootloader was ADAM2 or 
> PSPBoot and the root filesystem type. JFFS2 and squashfs do not have the same 
> erase-block size alignment constraints, ar7part deals with that too.
> 
I am not convinced that is a good idea, one main reason is ar7part.c has 
gotten terribly wrong the partition table ('loader' is too small[1] and 
'rootfs' overlaps with the 'linux' partition').

This is what I have from ADAM2's perspective:
----
Adam2_AR7WRD > printenv
[snipped]
mtd0                  0x900e0000,0x903f0000 <-- rootfs
mtd1                  0x90020000,0x900e0000 <-- kernel
mtd2                  0x90000000,0x90020000 <-- adam2 bootloader
mtd3                  0x903f0000,0x90400000 <-- configuration
mtd4                  0x90020000,0x903f0000 <-- kernel + rootfs
----

Linux spits out:
----
physmap platform flash device: 00800000 at 10000000
physmap-flash.0: Found 1 x16 devices at 0x0 in 16-bit bank
 Amd/Fujitsu Extended Query Table at 0x0040
physmap-flash.0: Swapping erase regions for broken CFI table.
number of CFI chips: 1
cmdlinepart partition parsing not available
RedBoot partition parsing not available
4 ar7part partitions found on MTD device physmap-flash.0
Creating 4 MTD partitions on "physmap-flash.0":
0x000000000000-0x000000010000 : "loader"
0x0000003f0000-0x000000400000 : "config"
0x000000020000-0x0000003f0000 : "linux"
0x0000000d0000-0x0000003f0000 : "rootfs"
----

My patch munched on whatever prom_getenv() returned, which from what I 
can tell looking at arch/mips/ar7/prom.c will work for both PSPBoot and 
ADAM2?

Are there some strange mtd environment variables I am yet to see out in 
the wild or does my patch simply not work for PSPBoot primed kit?  If 
not can you give me some 'spiel' to play around with?

Cheers

[1] okay, ADAM2 weighs in at less than 64kiB however it is not outside 
	the realm of possibility someone will port u-boot to AR7 which 
	would benefit from the full 128kiB of space?

-- 
Alexander Clouter
.sigmonster says: A man who turns green has eschewed protein.

Re: [PATCH 4/4] MTD: include ar7part in the list of partitions parsers

[Florian Fainelli]

Hi David,

Le dimanche 3 janvier 2010 21:56:17, David Woodhouse a écrit :
> On Sun, 2010-01-03 at 21:17 +0100, Florian Fainelli wrote:
> > This patch modifies the physmap-flash driver to include
> > the ar7part partition parser in the list of parsers to
> > use when a physmap-flash driver is registered. This is
> > required for AR7 to create partitions correctly.
> 
> Hrm, perhaps we'd do better to allow the probe types to be specified in
> the platform physmap_flash_data?

I guess so, will cook a patch which does that. Thanks!
-- 
Best regards, Florian Fainelli
Email: florian@openwrt.org
Web: http://openwrt.org
IRC: [florian] on irc.freenode.net
-------------------------------

Re: [PATCH 4/4] MTD: include ar7part in the list of partitions parsers

[Artem Bityutskiy]

On Sun, 2010-01-03 at 21:17 +0100, Florian Fainelli wrote:
> This patch modifies the physmap-flash driver to include
> the ar7part partition parser in the list of parsers to
> use when a physmap-flash driver is registered. This is
> required for AR7 to create partitions correctly.
> 
> Signed-off-by: Florian Fainelli <florian@openwrt.org>

Taken this to my l2-mtd-2.6/dunno tree.

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

Re: [PATCH 4/4] MTD: include ar7part in the list of partitions parsers

[Artem Bityutskiy]

On Sun, 2010-01-10 at 11:11 +0200, Artem Bityutskiy wrote:
> On Sun, 2010-01-03 at 21:17 +0100, Florian Fainelli wrote:
> > This patch modifies the physmap-flash driver to include
> > the ar7part partition parser in the list of parsers to
> > use when a physmap-flash driver is registered. This is
> > required for AR7 to create partitions correctly.
> > 
> > Signed-off-by: Florian Fainelli <florian@openwrt.org>
> 
> Taken this to my l2-mtd-2.6/dunno tree.

And removed after looking at the discussion :-)

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