Patchwork [V2] mtd: bcm47part driver for BCM47XX chipsets

login
register
mail settings
Submitter Rafał Miłecki
Date Aug. 25, 2012, 6:50 p.m.
Message ID <1345920652-11728-1-git-send-email-zajec5@gmail.com>
Download mbox | patch
Permalink /patch/179983/
State New
Headers show

Comments

Rafał Miłecki - Aug. 25, 2012, 6:50 p.m.
This driver provides parser detecting partitions on BCM47XX flash
memories. It has many differences in comparision to older BCM63XX, like:
1) Different CFE with no more trivial MAGICs
2) More partitions types (board_data, ML, POT)
3) Supporting more than 1 flash on a device
which resulted in decision of writing new parser.

It uses generic mtd interface and was successfully tested with Netgear
WNDR4500 router which has 2 flash memories: serial one and NAND one.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
V2: 1) Add support for more partitinos (ML and POT)
    2) Optimize: don't scan whole flash (like up to 128 MiB)
    3) Optimize: don't scan TRX partitions after detecting header
---
 drivers/mtd/Kconfig     |    4 +
 drivers/mtd/Makefile    |    1 +
 drivers/mtd/bcm47part.c |  193 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 198 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mtd/bcm47part.c
Kevin Cernekee - Aug. 26, 2012, 12:57 a.m.
On Sat, Aug 25, 2012 at 11:50 AM, Rafał Miłecki <zajec5@gmail.com> wrote:
> +config MTD_BCM47_PARTS
> +       tristate "BCM47XX partitioning support"
> +       depends on BCM47XX

You may want to change "bcm47" to "bcm47xx", for consistency with the
other bcm47xx/bcm63xx references found throughout the tree.

> +       u8 *buf;
[...]
> +       buf = kzalloc(sizeof(*buf) * BCM47PART_BYTES_TO_READ, GFP_KERNEL);

sizeof(u8) probably isn't needed.

> +               /* CFE has small NVRAM at 0x400 */
> +               fourcc = (u32 *)&buf[0x400];
> +               if (*fourcc == NVRAM_HEADER) {
> +                       parts[curr_part].name = "boot";
> +                       parts[curr_part].mask_flags = MTD_WRITEABLE;
> +                       parts[curr_part].offset = offset;
> +                       curr_part++;
> +               }
> +
> +               /* Standard NVRAM */
> +               fourcc = (u32 *)&buf[0x000];
> +               if (*fourcc == NVRAM_HEADER) {
> +                       parts[curr_part].name = "nvram";
> +                       parts[curr_part].mask_flags = MTD_WRITEABLE;
> +                       parts[curr_part].offset = offset;
> +                       curr_part++;
> +               }
> +
> +               /* board_data starts with board_id which differs across boards,
> +                * but we can use 'MPFR' (hopefully) magic at 0x100 */
> +               fourcc = (u32 *)&buf[0x100];
> +               if (*fourcc == 0x5246504D) { /* MPFR */
> +                       parts[curr_part].name = "board_data";
> +                       parts[curr_part].mask_flags = MTD_WRITEABLE;
> +                       parts[curr_part].offset = offset;
> +                       curr_part++;
> +               }
> +
> +               /* POT(TOP) */
> +               fourcc = (u32 *)&buf[0x000];
> +               fourcc2 = (u32 *)&buf[0x004];
> +               if (*fourcc == 0x54544f50 &&             /* POTT */
> +                   (*fourcc2 & 0xFFFF) == 0x504f) {     /* OP */
> +                       parts[curr_part].name = "POT";
> +                       parts[curr_part].mask_flags = MTD_WRITEABLE;
> +                       parts[curr_part].offset = offset;
> +                       curr_part++;
> +               }
> +
> +               /* ML */
> +               fourcc = (u32 *)&buf[0x010];
> +               fourcc2 = (u32 *)&buf[0x014];
> +               if (*fourcc == 0x39685a42 && *fourcc2 == 0x26594131) {

I would suggest using symbolic names for the constants, even if it's
just something like ML_MAGIC1.

> +                       parts[curr_part].name = "ML";
> +                       parts[curr_part].mask_flags = MTD_WRITEABLE;
> +                       parts[curr_part].offset = offset;
> +                       curr_part++;
> +               }

Are all of these cases mutually exclusive, given that they use the same offset?

It would be nice to reduce the amount of duplicated code if possible.
Maybe something as simple as:

char *name = NULL;
if (some_condition)
    name = "POT";
else if (some_other_condition)
    name = "ML";
[...]

if (name) {
    parts[curr_part].name = name;
    parts[curr_part].mask_flags = MTD_WRITEABLE;
    parts[curr_part].offset = offset;
    curr_part++;
}


Can the contents of buf[] be represented in a struct?
Rafał Miłecki - Aug. 26, 2012, 8:49 a.m.
Thanks for comments!

2012/8/26 Kevin Cernekee <cernekee@gmail.com>:
> On Sat, Aug 25, 2012 at 11:50 AM, Rafał Miłecki <zajec5@gmail.com> wrote:
>> +config MTD_BCM47_PARTS
>> +       tristate "BCM47XX partitioning support"
>> +       depends on BCM47XX
>
> You may want to change "bcm47" to "bcm47xx", for consistency with the
> other bcm47xx/bcm63xx references found throughout the tree.

Personally I prefer that "bcm47" over "bcm47xx". 2 minor advantages for me:
1) It's shorted but still unique
2) We have chars after digits in it's name, it's easier to read than
"xxpart" or "xxnflash"

David: AFAIK you're the maintainer of mtd? Could you share your opinion?

I won't argue if you guys prefer "bcm47xx" :)


>> +       u8 *buf;
> [...]
>> +       buf = kzalloc(sizeof(*buf) * BCM47PART_BYTES_TO_READ, GFP_KERNEL);

> sizeof(u8) probably isn't needed.

I hoped it will make reading the code easier. Multiplying should be
optimized by compiler. But can drop this if you think it's not worth
it.


>> +               /* CFE has small NVRAM at 0x400 */
>> +               fourcc = (u32 *)&buf[0x400];
>> +               if (*fourcc == NVRAM_HEADER) {
>> +                       parts[curr_part].name = "boot";
>> +                       parts[curr_part].mask_flags = MTD_WRITEABLE;
>> +                       parts[curr_part].offset = offset;
>> +                       curr_part++;
>> +               }
>> +
>> +               /* Standard NVRAM */
>> +               fourcc = (u32 *)&buf[0x000];
>> +               if (*fourcc == NVRAM_HEADER) {
>> +                       parts[curr_part].name = "nvram";
>> +                       parts[curr_part].mask_flags = MTD_WRITEABLE;
>> +                       parts[curr_part].offset = offset;
>> +                       curr_part++;
>> +               }
>> +
>> +               /* board_data starts with board_id which differs across boards,
>> +                * but we can use 'MPFR' (hopefully) magic at 0x100 */
>> +               fourcc = (u32 *)&buf[0x100];
>> +               if (*fourcc == 0x5246504D) { /* MPFR */
>> +                       parts[curr_part].name = "board_data";
>> +                       parts[curr_part].mask_flags = MTD_WRITEABLE;
>> +                       parts[curr_part].offset = offset;
>> +                       curr_part++;
>> +               }
>> +
>> +               /* POT(TOP) */
>> +               fourcc = (u32 *)&buf[0x000];
>> +               fourcc2 = (u32 *)&buf[0x004];
>> +               if (*fourcc == 0x54544f50 &&             /* POTT */
>> +                   (*fourcc2 & 0xFFFF) == 0x504f) {     /* OP */
>> +                       parts[curr_part].name = "POT";
>> +                       parts[curr_part].mask_flags = MTD_WRITEABLE;
>> +                       parts[curr_part].offset = offset;
>> +                       curr_part++;
>> +               }
>> +
>> +               /* ML */
>> +               fourcc = (u32 *)&buf[0x010];
>> +               fourcc2 = (u32 *)&buf[0x014];
>> +               if (*fourcc == 0x39685a42 && *fourcc2 == 0x26594131) {
>
> I would suggest using symbolic names for the constants, even if it's
> just something like ML_MAGIC1.

Agree.


>> +                       parts[curr_part].name = "ML";
>> +                       parts[curr_part].mask_flags = MTD_WRITEABLE;
>> +                       parts[curr_part].offset = offset;
>> +                       curr_part++;
>> +               }
>
> Are all of these cases mutually exclusive, given that they use the same offset?

Yes.


> It would be nice to reduce the amount of duplicated code if possible.
> Maybe something as simple as:
>
> char *name = NULL;
> if (some_condition)
>     name = "POT";
> else if (some_other_condition)
>     name = "ML";
> [...]
>
> if (name) {
>     parts[curr_part].name = name;
>     parts[curr_part].mask_flags = MTD_WRITEABLE;
>     parts[curr_part].offset = offset;
>     curr_part++;
> }

Ouch, you just reminded me I've forgotten to drop mask_flags in few
places. Masking MTD_WRITEABLE is needed for only ~half of our cases.
So if we follow your way now, we have to pass two arguments to the
generic partition handler: "name" and "mask_flags". But maybe I can
use some additional function for that? It should de-duplicate some
code and should work even for code inside "trx" parser.


> Can the contents of buf[] be represented in a struct?

Theoretically yes, but I think it doesn't make much sense. It's for
detecting type of partition, so it wouldn't have nice struct. It
usually would have only one valid field per type. So it would be
mostly like
struct partitions_ids {
    union {
        u32 possible_nvram_fourcc;
        u32 possible_pot_fourcc;
        u32 possible_trx_fourcc;
    },
    u32 possible_pot_fourcc2;
    u32 pad[2]
    u32 possible_ml_fourcc;
    u32 possible_ml_fourcc2;
}

I really don't like idea of such a struct.

Patch

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index 27143e0..0cd4096 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -148,6 +148,10 @@  config MTD_BCM63XX_PARTS
 	  This provides partions parsing for BCM63xx devices with CFE
 	  bootloaders.
 
+config MTD_BCM47_PARTS
+	tristate "BCM47XX partitioning support"
+	depends on BCM47XX
+
 comment "User Modules And Translation Layers"
 
 config MTD_CHAR
diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
index f901354..dac90e6 100644
--- a/drivers/mtd/Makefile
+++ b/drivers/mtd/Makefile
@@ -12,6 +12,7 @@  obj-$(CONFIG_MTD_CMDLINE_PARTS) += cmdlinepart.o
 obj-$(CONFIG_MTD_AFS_PARTS)	+= afs.o
 obj-$(CONFIG_MTD_AR7_PARTS)	+= ar7part.o
 obj-$(CONFIG_MTD_BCM63XX_PARTS)	+= bcm63xxpart.o
+obj-$(CONFIG_MTD_BCM47_PARTS)	+= bcm47part.o
 
 # 'Users' - code which presents functionality to userspace.
 obj-$(CONFIG_MTD_CHAR)		+= mtdchar.o
diff --git a/drivers/mtd/bcm47part.c b/drivers/mtd/bcm47part.c
new file mode 100644
index 0000000..29c6828
--- /dev/null
+++ b/drivers/mtd/bcm47part.c
@@ -0,0 +1,193 @@ 
+/*
+ * BCM47XX MTD partitioning
+ *
+ * Copyright (C) 2012 Rafał Miłecki <zajec5@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+#include <asm/mach-bcm47xx/nvram.h>
+
+/* 10 parts were found on sflash on Netgear WNDR4500 */
+#define BCM47PART_MAX_PARTS		12
+
+/* Amount of bytes we read when analyzing each block of flash memory.
+ * Set it big enough to allow detecting partition and reading important data. */
+#define BCM47PART_BYTES_TO_READ		0x401
+
+#define TRX_MAGIC			0x30524448
+
+struct trx_header {
+	u32 magic;
+	u32 length;
+	u32 crc32;
+	u16 flags;
+	u16 version;
+	u32 offset[3];
+} __packed;
+
+static int bcm47part_parse(struct mtd_info *master,
+			   struct mtd_partition **pparts,
+			   struct mtd_part_parser_data *data)
+{
+	struct mtd_partition *parts;
+	u8 i, curr_part = 0;
+	u8 *buf;
+	u32 *fourcc, *fourcc2;
+	size_t bytes_read;
+	u32 offset;
+	u32 blocksize = 0x10000;
+	struct trx_header *trx;
+
+	/* Alloc */
+	parts = kzalloc(sizeof(struct mtd_partition) * BCM47PART_MAX_PARTS,
+			GFP_KERNEL);
+	buf = kzalloc(sizeof(*buf) * BCM47PART_BYTES_TO_READ, GFP_KERNEL);
+
+	/* Parse block by block looking for magics */
+	for (offset = 0; offset <= master->size - blocksize;
+	     offset += blocksize) {
+		/* Nothing more in higher memory */
+		if (offset >= 0x2000000)
+			break;
+
+		/* Read beginning of the block */
+		if (mtd_read(master, offset, BCM47PART_BYTES_TO_READ,
+		    &bytes_read, buf) < 0) {
+			pr_err("mtd_read error while parsing (offset: 0x%X)!\n",
+			       offset);
+			continue;
+		}
+
+		/* CFE has small NVRAM at 0x400 */
+		fourcc = (u32 *)&buf[0x400];
+		if (*fourcc == NVRAM_HEADER) {
+			parts[curr_part].name = "boot";
+			parts[curr_part].mask_flags = MTD_WRITEABLE;
+			parts[curr_part].offset = offset;
+			curr_part++;
+		}
+
+		/* Standard NVRAM */
+		fourcc = (u32 *)&buf[0x000];
+		if (*fourcc == NVRAM_HEADER) {
+			parts[curr_part].name = "nvram";
+			parts[curr_part].mask_flags = MTD_WRITEABLE;
+			parts[curr_part].offset = offset;
+			curr_part++;
+		}
+
+		/* board_data starts with board_id which differs across boards,
+		 * but we can use 'MPFR' (hopefully) magic at 0x100 */
+		fourcc = (u32 *)&buf[0x100];
+		if (*fourcc == 0x5246504D) { /* MPFR */
+			parts[curr_part].name = "board_data";
+			parts[curr_part].mask_flags = MTD_WRITEABLE;
+			parts[curr_part].offset = offset;
+			curr_part++;
+		}
+
+		/* POT(TOP) */
+		fourcc = (u32 *)&buf[0x000];
+		fourcc2 = (u32 *)&buf[0x004];
+		if (*fourcc == 0x54544f50 &&		 /* POTT */
+		    (*fourcc2 & 0xFFFF) == 0x504f) {	 /* OP */
+			parts[curr_part].name = "POT";
+			parts[curr_part].mask_flags = MTD_WRITEABLE;
+			parts[curr_part].offset = offset;
+			curr_part++;
+		}
+
+		/* ML */
+		fourcc = (u32 *)&buf[0x010];
+		fourcc2 = (u32 *)&buf[0x014];
+		if (*fourcc == 0x39685a42 && *fourcc2 == 0x26594131) {
+			parts[curr_part].name = "ML";
+			parts[curr_part].mask_flags = MTD_WRITEABLE;
+			parts[curr_part].offset = offset;
+			curr_part++;
+		}
+
+		/* TRX */
+		fourcc = (u32 *)&buf[0x000];
+		if (*fourcc == TRX_MAGIC) {
+			trx = (struct trx_header *)buf;
+
+			i = 0;
+			/* We have LZMA loader if offset[2] points to sth */
+			if (trx->offset[2]) {
+				/* TODO: should we add LZMA loader partition? */
+				i++;
+			}
+
+			parts[curr_part].name = "linux";
+			parts[curr_part].mask_flags = MTD_WRITEABLE;
+			parts[curr_part].offset = offset + trx->offset[i];
+			curr_part++;
+			i++;
+
+			parts[curr_part].name = "rootfs";
+			parts[curr_part].mask_flags = MTD_WRITEABLE;
+			parts[curr_part].offset = offset + trx->offset[i];
+			/* Pure rootfs size is known and can be calculated as:
+			 * trx->length - trx->offset[i]. We don't fill it as
+			 * we want to have jffs2 (overlay) in the same mtd. */
+			curr_part++;
+			i++;
+
+			/* We have whole TRX scanned, skip to the next part. Use
+			 * roundown (not roundup), as the loop will increase
+			 * offset in next step. */
+			offset = rounddown(offset + trx->length, blocksize);
+		}
+
+		if (curr_part == BCM47PART_MAX_PARTS) {
+			pr_warn("Reached maximum number of partitions, scanning stopped!\n");
+			break;
+		}
+	}
+
+	/* We can't read sizes of some (most of) partitions. Assume that
+	 * these partitions end at the beginning of the one they are
+	 * followed by. */
+	for (i = 0; i < curr_part - 1; i++) {
+		if (parts[i].size == 0)
+			parts[i].size = parts[i + 1].offset - parts[i].offset;
+	}
+	if (curr_part > 0 && parts[curr_part - 1].size == 0)
+		parts[curr_part - 1].size =
+				master->size - parts[curr_part - 1].offset;
+
+	*pparts = parts;
+	return curr_part;
+};
+
+static struct mtd_part_parser bcm47part_mtd_parser = {
+	.owner = THIS_MODULE,
+	.parse_fn = bcm47part_parse,
+	.name = "bcm47part",
+};
+
+static int __init bcm47part_init(void)
+{
+	return register_mtd_parser(&bcm47part_mtd_parser);
+}
+
+static void __exit bcm47part_exit(void)
+{
+	deregister_mtd_parser(&bcm47part_mtd_parser);
+}
+
+module_init(bcm47part_init);
+module_exit(bcm47part_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("MTD partitioning for BCM47XX flash memories");