Message ID | 1431773604-11788-5-git-send-email-noltari@gmail.com |
---|---|
State | Rejected |
Delegated to: | Rafał Miłecki |
Headers | show |
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
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.
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. >
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
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.
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.
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 --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", + }; +
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