diff mbox

[OpenWrt-Devel,4/6] bcm53xx: bcm47xxpart: add support for parsing device tree partitions

Message ID 1431773604-11788-5-git-send-email-noltari@gmail.com
State Rejected
Delegated to: Rafał Miłecki
Headers show

Commit Message

Álvaro Fernández Rojas May 16, 2015, 10:53 a.m. UTC 
Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
---
 ...-bcm47xxpart-parse-device-tree-partitions.patch | 156 +++++++++++++++++++++
 1 file changed, 156 insertions(+)
 create mode 100644 target/linux/bcm53xx/patches-3.18/401-mtd-bcm47xxpart-parse-device-tree-partitions.patch

Comments

Hauke Mehrtens May 16, 2015, 11:11 a.m. UTC | #1 
On 05/16/2015 12:53 PM, Álvaro Fernández Rojas wrote:
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
> ---
>  ...-bcm47xxpart-parse-device-tree-partitions.patch | 156 +++++++++++++++++++++
>  1 file changed, 156 insertions(+)
>  create mode 100644 target/linux/bcm53xx/patches-3.18/401-mtd-bcm47xxpart-parse-device-tree-partitions.patch
> 
What is the difference to just using the existing device tree  partition
parser: drivers/mtd/ofpart.c ?

Hauke
Rafał Miłecki May 16, 2015, 11:16 a.m. UTC | #2 
On 16 May 2015 at 13:11, Hauke Mehrtens <hauke@hauke-m.de> wrote:
> On 05/16/2015 12:53 PM, Álvaro Fernández Rojas wrote:
>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>> ---
>>  ...-bcm47xxpart-parse-device-tree-partitions.patch | 156 +++++++++++++++++++++
>>  1 file changed, 156 insertions(+)
>>  create mode 100644 target/linux/bcm53xx/patches-3.18/401-mtd-bcm47xxpart-parse-device-tree-partitions.patch
>>
> What is the difference to just using the existing device tree  partition
> parser: drivers/mtd/ofpart.c ?

Ack for the question, when submitting patches more complex that "add
LEDs", please *describe* them / the changes.
Álvaro Fernández Rojas May 16, 2015, 11:53 a.m. UTC | #3 
This patch splits firmware partition based on TRX header, while ofpart won't be able to split firmware partition into kernel (linux) + rootfs.
It's very similar to bcm63xx code, where bcmtag contains the information needed to split linux partition into kernel + rootfs.

El 16/05/2015 a las 13:16, Rafał Miłecki escribió:
> On 16 May 2015 at 13:11, Hauke Mehrtens <hauke@hauke-m.de> wrote:
>> On 05/16/2015 12:53 PM, Álvaro Fernández Rojas wrote:
>>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>>> ---
>>>  ...-bcm47xxpart-parse-device-tree-partitions.patch | 156 +++++++++++++++++++++
>>>  1 file changed, 156 insertions(+)
>>>  create mode 100644 target/linux/bcm53xx/patches-3.18/401-mtd-bcm47xxpart-parse-device-tree-partitions.patch
>>>
>> What is the difference to just using the existing device tree  partition
>> parser: drivers/mtd/ofpart.c ?
> 
> Ack for the question, when submitting patches more complex that "add
> LEDs", please *describe* them / the changes.
>
Bjørn Mork May 16, 2015, 11:54 a.m. UTC | #4 
Hauke Mehrtens <hauke@hauke-m.de> writes:

> On 05/16/2015 12:53 PM, Álvaro Fernández Rojas wrote:
>> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>
>> ---
>>  ...-bcm47xxpart-parse-device-tree-partitions.patch | 156 +++++++++++++++++++++
>>  1 file changed, 156 insertions(+)
>>  create mode 100644 target/linux/bcm53xx/patches-3.18/401-mtd-bcm47xxpart-parse-device-tree-partitions.patch
>> 
> What is the difference to just using the existing device tree  partition
> parser: drivers/mtd/ofpart.c ?

I wondered about the same...

Looking at the patch, it seems this solves an issue I am having with the
Linksys EA6700 (another bcm53xx) as well: We want some way to run the
TRX parser on the "firmware" partition(s), while describing the rest of
the layout in the device tree.  The probing strategy of the bcm47xxpart
fails with all the "devinfo" and "nvram" data spread all over, in
addition to the trailing vendor data partition ("asus" on the RT-AC87U
and "brcmnand" on the EA6700).  This could probably be fixed by careful
tuning of the parser.  But I believe that makes it unnecessary complex,
risking regressions on all the routers where the current parser works
fine.

Using a combination of device tree and a TRX parser looks like a much
better alternative.

Note that the Linksys routers have *two* TRX partitions due to the
Linksys dual-image failsafe implementation.  Ideally the parser should
take this into account when naming the "linux" and "rootfs" partitions.

Just to illustrate.  This is the EA6700 stock firmware partition layout: 

Creating 6 MTD partitions on "nflash":
0x000000000000-0x000000080000 : "boot"
0x000000080000-0x000000200000 : "nvram"
0x000000200000-0x000001f00000 : "linux"
0x0000003d3608-0x000001f00000 : "rootfs"
0x000001f00000-0x000003c00000 : "linux2"
0x00000213642c-0x000003c00000 : "rootfs2"
Creating 1 MTD partitions on "brcmnand":
0x000003c00000-0x000008000000 : "brcmnand"


This is the current bcm47xxpart parser result on the same router:

[    3.099341] 9 bcm47xxpart partitions found on MTD device bcm_nand
[    3.105408] Creating 9 MTD partitions on "bcm_nand":
[    3.110365] 0x000000000000-0x000000080000 : "boot"
[    3.116753] 0x000000080000-0x000000180000 : "nvram"
[    3.124461] 0x000000180000-0x000000200000 : "nvram"
[    3.131199] 0x000000200000-0x00000020001c : "firmware"
[    3.137390] 0x00000020001c-0x0000003d3608 : "linux"
[    3.159331] 0x0000003d3608-0x000001f00000 : "rootfs"
[    3.247077] 0x000000f40000-0x000001f00000 : "rootfs_data"
[    3.283079] 0x000001f00000-0x000008000000 : "firmware"
[    3.473070] 0x000001f0001c-0x00000213642c : "linux"
[    3.495495] 0x00000213642c-0x000008000000 : "rootfs"



Note that the stock firmware isn't entirely agreeing with the CFE
partition layout either.  In particular, it ignores the 512KB "devinfo"
partition starting at 180000 (which the bcm47xxpart parser more
correctly has identified as another "nvram" partition):

nflash0              AMD NAND flash size 131072KB
nflash0.boot         AMD NAND flash offset 00000000 size 512KB
nflash0.nvram        AMD NAND flash offset 00080000 size 1024KB
nflash0.devinfo      AMD NAND flash offset 00180000 size 512KB
nflash0.trx          AMD NAND flash offset 00200000 size 1KB
nflash0.os           AMD NAND flash offset 0020001C size 29696KB
nflash0.trx2         AMD NAND flash offset 01F00000 size 1KB
nflash0.os2          AMD NAND flash offset 01F0001C size 29696KB
nflash1.boot         AMD NAND flash offset 00000000 size 512KB
nflash1.nvram        AMD NAND flash offset 00080000 size 1024KB
nflash1.devinfo      AMD NAND flash offset 00180000 size 512KB
nflash1.trx          AMD NAND flash offset 00200000 size 29696KB
nflash1.trx2         AMD NAND flash offset 01F00000 size 29696KB
nflash1.brcmnand     AMD NAND flash offset 03C00000 size 69632KB


In any case, I believe this is best described in device tree.  We just
need some method to parse the internals of the TRX partitions.  



Bjørn
Rafał Miłecki May 16, 2015, 1:28 p.m. UTC | #5 
On 16 May 2015 at 13:53, Álvaro Fernández Rojas <noltari@gmail.com> wrote:
> This patch splits firmware partition based on TRX header, while ofpart won't be able to split firmware partition into kernel (linux) + rootfs.
> It's very similar to bcm63xx code, where bcmtag contains the information needed to split linux partition into kernel + rootfs.

Let me think about this, but I guess it's not the way we want to
handle it. We probably should use TRX MTD splitter.
Rafał Miłecki May 18, 2015, 3:05 p.m. UTC | #6 
On 16 May 2015 at 15:28, Rafał Miłecki <zajec5@gmail.com> wrote:
> On 16 May 2015 at 13:53, Álvaro Fernández Rojas <noltari@gmail.com> wrote:
>> This patch splits firmware partition based on TRX header, while ofpart won't be able to split firmware partition into kernel (linux) + rootfs.
>> It's very similar to bcm63xx code, where bcmtag contains the information needed to split linux partition into kernel + rootfs.
>
> Let me think about this, but I guess it's not the way we want to
> handle it. We probably should use TRX MTD splitter.

OK, let me share my thoughts. There is a chain of problems ;) but we
can handle it with a bit of time.

It seems the main reason for you hacking bcm47xxpart is its support
for creating "firmware" partition subpartitions. This is not a way to
go. We should allow any driver (bcm47xxpart, ofpart) to just create
"firmware" and then have separated splitter handling "firmware"
partition.

There already is a driver for that, see MTD_SPLIT_TRX_FW.
Unfortunately, we can't enable it for bcm53xx as it will conflict with
bcm47xxpart.

So we should drop support for TRX parsing from bcm47xxpart and then
enable MTD_SPLIT_TRX_FW. Unfortunately bcm47xxpart is already
upstreamed (and I want to use upstream version). There is no reason to
drop TRX parting from bcm47xpart mainline, since MTD_SPLIT_TRX_FW
exists in OpenWrt only.

So we should add MTD_SPLIT_TRX_FW to the mainline kernel.
Unfortunately this also isn't possible, because mainline kernel
doesn't support mtdsplit.

So there is a good news. I've just sent patch to linux-mtd ML adding
support for splitting parsers:
https://patchwork.ozlabs.org/patch/473363/

Once we got my patch accepted we can work on every step from the above
chain one by one.
Rafał Miłecki May 18, 2015, 3:07 p.m. UTC | #7 
On 16 May 2015 at 12:53, Álvaro Fernández Rojas <noltari@gmail.com> wrote:
> Signed-off-by: Álvaro Fernández Rojas <noltari@gmail.com>

After discussion in this topic, I'll give a nack to this patch. We
should use different approach and we should probably focus on mainline
Broadcom NAND driver.
diff mbox

Patch

diff --git a/target/linux/bcm53xx/patches-3.18/401-mtd-bcm47xxpart-parse-device-tree-partitions.patch b/target/linux/bcm53xx/patches-3.18/401-mtd-bcm47xxpart-parse-device-tree-partitions.patch
new file mode 100644
index 0000000..272b0ef
--- /dev/null
+++ b/target/linux/bcm53xx/patches-3.18/401-mtd-bcm47xxpart-parse-device-tree-partitions.patch
@@ -0,0 +1,156 @@ 
+--- a/drivers/mtd/bcm47xxpart.c
++++ b/drivers/mtd/bcm47xxpart.c
+@@ -14,6 +14,8 @@
+ #include <linux/slab.h>
+ #include <linux/mtd/mtd.h>
+ #include <linux/mtd/partitions.h>
++#include <linux/of.h>
++#include <linux/vmalloc.h>
+ 
+ #include <uapi/linux/magic.h>
+ 
+@@ -330,9 +332,143 @@ static int bcm47xxpart_parse(struct mtd_
+ 	return curr_part;
+ };
+ 
++static bool node_has_compatible(struct device_node *pp)
++{
++	return of_get_property(pp, "compatible", NULL);
++}
++
++static int parse_trxtag(struct mtd_info *master, struct mtd_partition *pparts,
++			int next_part, size_t offset, size_t size)
++{
++	struct trx_header *trx;
++	int ret, kernel_part, rootfs_part, kernel_offset, rootfs_offset;
++	size_t retlen;
++
++	/* Allocate memory for trx header */
++	trx = vmalloc(sizeof(*trx));
++	if (!trx)
++		return -ENOMEM;
++
++	/* Get the tag */
++	ret = mtd_read(master, offset, sizeof(*trx), &retlen,
++		       (void *)trx);
++
++	if (retlen != sizeof(*trx)) {
++		vfree(trx);
++		return 0;
++	}
++
++	kernel_part = next_part;
++	rootfs_part = next_part + 1;
++
++	/* We have LZMA loader if offset[2] points to sth */
++	if (trx->offset[2]) {
++		kernel_offset = 1;
++		rootfs_offset = 2;
++	}
++	else {
++		kernel_offset = 0;
++		rootfs_offset = 1;
++	}
++
++	pparts[kernel_part].name = "linux";
++	pparts[kernel_part].offset = offset + trx->offset[kernel_offset];
++	pparts[kernel_part].size = trx->offset[rootfs_offset] - trx->offset[kernel_offset];
++
++	pparts[rootfs_part].name = bcm47xxpart_trx_data_part_name(master, offset + trx->offset[rootfs_offset]);
++	pparts[rootfs_part].offset = offset + trx->offset[rootfs_offset];
++	pparts[rootfs_part].size = size - trx->offset[rootfs_offset];
++
++	vfree(trx);
++
++	return 2;
++}
++
++static int bcm47xxpart_parse_of(struct mtd_info *master,
++					   struct mtd_partition **pparts,
++					   struct mtd_part_parser_data *data)
++{
++	struct device_node *dp = data->of_node;
++	struct device_node *pp;
++	int i, nr_parts = 0;
++	const char *partname;
++	int len;
++
++	for_each_child_of_node(dp, pp) {
++		if (node_has_compatible(pp))
++			continue;
++
++		if (!of_get_property(pp, "reg", &len))
++			continue;
++
++		partname = of_get_property(pp, "label", &len);
++		if (!partname)
++			partname = of_get_property(pp, "name", &len);
++
++		if (!strcmp(partname, "firmware")) {
++			nr_parts += 2;
++		}
++
++		nr_parts++;
++	}
++
++	*pparts = kzalloc(nr_parts * sizeof(**pparts), GFP_KERNEL);
++	if (!*pparts)
++		return -ENOMEM;
++
++	i = 0;
++	for_each_child_of_node(dp, pp) {
++		const __be32 *reg;
++		int a_cells, s_cells;
++		size_t size, offset;
++
++		if (node_has_compatible(pp))
++			continue;
++
++		reg = of_get_property(pp, "reg", &len);
++		if (!reg)
++			continue;
++
++		a_cells = of_n_addr_cells(pp);
++		s_cells = of_n_size_cells(pp);
++		offset = of_read_number(reg, a_cells);
++		size = of_read_number(reg + a_cells, s_cells);
++		partname = of_get_property(pp, "label", &len);
++		if (!partname)
++			partname = of_get_property(pp, "name", &len);
++
++		if (!strcmp(partname, "firmware"))
++			i += parse_trxtag(master, *pparts, i, offset, size);
++
++		if (of_get_property(pp, "read-only", &len))
++			(*pparts)[i].mask_flags |= MTD_WRITEABLE;
++
++		if (of_get_property(pp, "lock", &len))
++			(*pparts)[i].mask_flags |= MTD_POWERUP_LOCK;
++
++		(*pparts)[i].offset = offset;
++		(*pparts)[i].size = size;
++		(*pparts)[i].name = partname;
++
++		i++;
++	}
++
++	return i;
++}
++
++static int bcm47xx_parse_partitions(struct mtd_info *master,
++				    struct mtd_partition **pparts,
++				    struct mtd_part_parser_data *data)
++{
++	if (data && data->of_node)
++		return bcm47xxpart_parse_of(master, pparts, data);
++	else
++		return bcm47xxpart_parse(master, pparts, data);
++}
++
+ static struct mtd_part_parser bcm47xxpart_mtd_parser = {
+ 	.owner = THIS_MODULE,
+-	.parse_fn = bcm47xxpart_parse,
++	.parse_fn = bcm47xx_parse_partitions,
+ 	.name = "bcm47xxpart",
+ };
+