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

[Florian Fainelli <florian@openwrt.org>]

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