| Message ID | 20180804134426.42093-2-hacks@slashdirt.org |
|---|---|
| State | Accepted |
| Delegated to: | Rafał Miłecki |
| Headers | show |
| Series | ramips: fix RBMxxG partitioning | expand |
On 04.08.2018 15:44, Thibaut VARÈNE wrote: > This patch improves 5684d087418d176cfdef4e045e1950ca7ba3b09f by correcting > the partition scheme for the "RouterBoot" section of the flash. > > The partition scheme initially submitted is incorrect and does not reflect > the actual flash structure. > > The "RouterBoot" section (name matching OEM) is subdivided in several > static segments, as they are on ar71xx RB devices albeit with different > offsets and sizes. > The naming convention from ar71xx has been preserved, except for the > bootloaders which are named "bootloader1" and "bootloader2" to avoid > confusion with the master "RouterBoot" partition. > The preferred 'fixed-partitions' DTS node syntax is used, with nesting > support as introduced in 2a598bbaa3. > "partition" is used for node names, with associated "label" to match > policy set by 6dd94c2781. > > The OEM source code also define a "RouterBootFake" partition at the > beginning of the secondary flash chip: to avoid trouble if OEM ever makes > use of that space, it is also defined here. My review & comments: 1) Adding "RouterBoot" partition This seems correct as from what I read: a) It matches how vendor describes / names that flash region b) It allow to use "rbcfg" tool which expects that partition c) It allows following OEM dump procedure 2) Nesting partitions in DTS It's definitely correct as it follows what was agreed by upstream mtd developers and DT guys (including Rob) after years of discussing that over and over. For details see d2ad00eb7879 ("dt-bindings: mtd: explicitly document nesting partitions descriptions"): https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d2ad00eb78792b396a6d012f15d6297a1701b8bc 3) Using "fixed-partitions" for subpartitions Sounds good since these subpartitions (bootloaders, bios, config) are likely to be always located as the same regions. If we ever get a dynamic parser developed and for some reason it will be more reliable than hardocded offsets & sizes, we can always switch DTS to specify that parser supported format. 4) Subpartitions of "RouterBoot" partition Look sane, "read-only;" present where expected. 5) "RouterBootFake" partition I don't see why it should be defined. If it is empty and it looks like just a reserved region, we should simply *not* create any partition for it. Maybe vendor was using some hacky MTD method for creating "firmware" partition and it needed to cover region before that with some /fake/ partition? Anyway it seems to me that one should *not* be added. I believe I reviewed every part of this patch and at this point only one change doesn't seem to be needed. Anything else I missed?
On Fri, 17 Aug 2018 at 21:56, Rafał Miłecki <rafal@milecki.pl> wrote: > 3) Using "fixed-partitions" for subpartitions > Sounds good since these subpartitions (bootloaders, bios, config) are > likely to be always located as the same regions. > If we ever get a dynamic parser developed and for some reason it will be > more reliable than hardocded offsets & sizes, we can always switch DTS > to specify that parser supported format. I found some comment pointing ar71xx and its routerboot.c and mach-rbspi.c as a reference for dynamic partition parsing. It has following advantages: 1) It dynamically finds beginning of hard coding partition 2) It dynamically finds beginning of soft coding partition and not much more. It assumes: 1) that bootloader starts at 0x0 2) there is nothing between bootloader & hard config 3) size of hard config partition 4) there is bios right after hard config partition 5) size of bios partition 6) (...) [I got tired by listing that] After all it seems routerboot.c + mach-rbspi.c also assume pretty fixed flash layout and I don't see it much much better over "fixed-partitions". Again, if we ever find "fixed-partitions" inaccurate it's something that can be changed.
diff --git a/target/linux/ramips/dts/RBM33G.dts b/target/linux/ramips/dts/RBM33G.dts index 65560ab821..db4d1599d7 100644 --- a/target/linux/ramips/dts/RBM33G.dts +++ b/target/linux/ramips/dts/RBM33G.dts @@ -106,15 +106,41 @@ #size-cells = <1>; partition@0 { - label = "routerboot"; - reg = <0x0 0xf000>; - read-only; - }; - - factory: partition@f000 { - label = "factory"; - reg = <0xf000 0x71000>; + label = "RouterBoot"; + reg = <0x0 0x40000>; read-only; + compatible = "fixed-partitions"; + #address-cells = <1>; + #size-cells = <1>; + + partition@0 { + label = "bootloader1"; + reg = <0x0 0xf000>; + read-only; + }; + + hard_config: partition@f000 { + label = "hard_config"; + reg = <0xf000 0x1000>; + read-only; + }; + + partition@10000 { + label = "bootloader2"; + reg = <0x10000 0xf000>; + read-only; + }; + + partition@20000 { + label = "soft_config"; + reg = <0x20000 0x1000>; + }; + + partition@30000 { + label = "bios"; + reg = <0x30000 0x1000>; + read-only; + }; }; }; }; @@ -122,6 +148,7 @@ w25q128@1 { compatible = "jedec,spi-nor"; reg = <1>; + // XXX empiric value to obtain actual 10MHz SCK at the chip spi-max-frequency = <3125000>; partitions { @@ -129,6 +156,13 @@ #address-cells = <1>; #size-cells = <1>; + // this contains no data but is named so in OEM source + partition@0 { + label = "RouterBootFake"; + reg = <0x0 0x40000>; + read-only; + }; + partition@40000 { label = "firmware"; reg = <0x040000 0xFC0000>; @@ -138,7 +172,7 @@ }; ðernet { - mtd-mac-address = <&factory 0x0010>; + mtd-mac-address = <&hard_config 0x0010>; mtd-mac-address-increment = <1>; };