| Message ID | 20180729090724.8436-2-hacks@slashdirt.org |
|---|---|
| State | Changes Requested |
| Delegated to: | John Crispin |
| Headers | show |
| Series | [OpenWrt-Devel,v2,1/4] ramips: fix RBM33G name | expand |
On 29/07/18 11:07, Thibaut VARÈNE wrote: > This patch improves 5684d087418d176cfdef4e045e1950ca7ba3b09f by correcting > the partition scheme for the "RouterBoot" section of the flash. > > This section is subdivided in several segments, as they are on ar71xx > RB devices, albeit with different offsets and sizes. The naming convention > from ar71xx has been preserved. The preferred 'fixed-partitions' DTS > node syntax is used, with nesting support as introduced in 2a598bbaa3. > > 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. as discussed on IRC we concluded, that this should be done with a mtd splitter. John > > The resulting partition scheme looks like this: > [ 10.114241] m25p80 spi0.0: w25x40 (512 Kbytes) > [ 10.118708] 1 fixed-partitions partitions found on MTD device spi0.0 > [ 10.125049] Creating 1 MTD partitions on "spi0.0": > [ 10.129824] 0x000000000000-0x000000040000 : "RouterBoot" > [ 10.136215] 5 fixed-partitions partitions found on MTD device RouterBoot > [ 10.142894] Creating 5 MTD partitions on "RouterBoot": > [ 10.148032] 0x000000000000-0x00000000f000 : "routerboot" > [ 10.154336] 0x00000000f000-0x000000010000 : "hard_config" > [ 10.160665] 0x000000010000-0x00000001f000 : "routerboot2" > [ 10.167046] 0x000000020000-0x000000021000 : "soft_config" > [ 10.173461] 0x000000030000-0x000000031000 : "bios" > [ 10.190191] m25p80 spi0.1: w25q128 (16384 Kbytes) > [ 10.194950] 2 fixed-partitions partitions found on MTD device spi0.1 > [ 10.201271] Creating 2 MTD partitions on "spi0.1": > [ 10.206071] 0x000000000000-0x000000040000 : "RouterBootFake" > [ 10.212746] 0x000000040000-0x000001000000 : "firmware" > [ 10.307216] 2 minor-fw partitions found on MTD device firmware > [ 10.313044] 0x000000040000-0x000000220000 : "kernel" > [ 10.319002] 0x000000220000-0x000001000000 : "rootfs" > [ 10.324906] mtd: device 9 (rootfs) set to be root filesystem > [ 10.330678] 1 squashfs-split partitions found on MTD device rootfs > [ 10.336886] 0x000000b40000-0x000001000000 : "rootfs_data" > > Leave a note in DTS to explain how the original author selected the SPI speed. > > Tested-by: Tobias Schramm <tobleminer@gmail.com> > Signed-off-by: Thibaut VARÈNE <hacks@slashdirt.org> > --- > target/linux/ramips/dts/RBM33G.dts | 69 +++++++++++++++++++++++++++++--------- > 1 file changed, 54 insertions(+), 15 deletions(-) > > diff --git a/target/linux/ramips/dts/RBM33G.dts b/target/linux/ramips/dts/RBM33G.dts > index 612dc106ed..a02d03818f 100644 > --- a/target/linux/ramips/dts/RBM33G.dts > +++ b/target/linux/ramips/dts/RBM33G.dts > @@ -104,18 +104,44 @@ > reg = <0>; > spi-max-frequency = <3125000>; > > - partition@0 { > - label = "routerboot"; > - reg = <0x0 0xf000>; > - read-only; > + partitions { > + compatible = "fixed-partitions"; > + #address-cells = <1>; > + #size-cells = <1>; > + > + partition@0 { > + label = "RouterBoot"; > + reg = <0x0 0x40000>; > + read-only; > + compatible = "fixed-partitions"; > + #address-cells = <1>; > + #size-cells = <1>; > + > + routerboot@0 { > + reg = <0x0 0xf000>; > + read-only; > + }; > + > + hard_config: hard_config@f000 { > + reg = <0xf000 0x1000>; > + read-only; > + }; > + > + routerboot2@10000 { > + reg = <0x10000 0xf000>; > + read-only; > + }; > + > + soft_config@20000 { > + reg = <0x20000 0x1000>; > + }; > + > + bios@30000 { > + reg = <0x30000 0x1000>; > + read-only; > + }; > + }; > }; > - > - factory: partition@f000 { > - label = "factory"; > - reg = <0xf000 0x71000>; > - read-only; > - }; > - > }; > > w25q128@0 { > @@ -123,17 +149,30 @@ > #size-cells = <1>; > compatible = "jedec,spi-nor"; > reg = <1>; > + // XXX empiric value to obtain actual 10MHz SCK at the chip > spi-max-frequency = <3125000>; > > - partition@40000 { > - label = "firmware"; > - reg = <0x040000 0xFC0000>; > + partitions { > + compatible = "fixed-partitions"; > + #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; > + }; > + > + firmware@40000 { > + reg = <0x040000 0xFC0000>; > + }; > }; > }; > }; > > ðernet { > - mtd-mac-address = <&factory 0x0010>; > + mtd-mac-address = <&hard_config 0x0010>; > mtd-mac-address-increment = <1>; > }; >
> Le 30 juil. 2018 à 08:40, John Crispin <john@phrozen.org> a écrit : > > > > On 29/07/18 11:07, Thibaut VARÈNE wrote: >> This patch improves 5684d087418d176cfdef4e045e1950ca7ba3b09f by correcting >> the partition scheme for the "RouterBoot" section of the flash. >> >> This section is subdivided in several segments, as they are on ar71xx >> RB devices, albeit with different offsets and sizes. The naming convention >> from ar71xx has been preserved. The preferred 'fixed-partitions' DTS >> node syntax is used, with nesting support as introduced in 2a598bbaa3. >> >> 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. > > as discussed on IRC we concluded, that this should be done with a mtd splitter. > John I’m sorry, we didn’t conclude anything, I believe you demanded it be done so. Unfortunately, after a more careful investigation, I am now convinced this is not the right course of action. Here’s why, in my very humble opinion and limited understanding: - this splitter will be quite intrusive in generic code (currently « mtdsplit » only works with « firmware » and « rootfs » named partitions) - the bootloaders (routerboot and routerboot2) cannot be programmatically splitted: they are raw machine code without a signature - all ramips routerboard machines share the exact same partition scheme which is different from all the ar71xx routerboard machines (which also all share the same scheme) - if I read the OEM source code correctly, it’s likely their ARM-based boards have yet another partition scheme Consequently, a purported splitter will be invasive and full of hardcoded numbers, making its upstream acceptance very unlikely (I anticipate the argument « this should all be done in DTS, especially now that DTS supports nested partitions »). Now, I would like to hope that a correct partition scheme that uses all the OpenWRT-accepted features and follows guidelines would be more likely to be accepted in the source than the currently broken, incorrect and incomplete existing one. Best regards, T.
On 2018-07-30 04:09 AM, Thibaut wrote: > > >> Le 30 juil. 2018 à 08:40, John Crispin <john@phrozen.org> a écrit : >> >> >> >> On 29/07/18 11:07, Thibaut VARÈNE wrote: >>> This patch improves 5684d087418d176cfdef4e045e1950ca7ba3b09f by correcting >>> the partition scheme for the "RouterBoot" section of the flash. >>> >>> This section is subdivided in several segments, as they are on ar71xx >>> RB devices, albeit with different offsets and sizes. The naming convention >>> from ar71xx has been preserved. The preferred 'fixed-partitions' DTS >>> node syntax is used, with nesting support as introduced in 2a598bbaa3. >>> >>> 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. >> >> as discussed on IRC we concluded, that this should be done with a mtd splitter. >> John > > I’m sorry, we didn’t conclude anything, I believe you demanded it be done so. > > Unfortunately, after a more careful investigation, I am now convinced this is not the right course of action. Here’s why, in my very humble opinion and limited understanding: > > - this splitter will be quite intrusive in generic code (currently « mtdsplit » only works with « firmware » and « rootfs » named partitions) > - the bootloaders (routerboot and routerboot2) cannot be programmatically splitted: they are raw machine code without a signature > - all ramips routerboard machines share the exact same partition scheme which is different from all the ar71xx routerboard machines (which also all share the same scheme) > - if I read the OEM source code correctly, it’s likely their ARM-based boards have yet another partition scheme > > Consequently, a purported splitter will be invasive and full of hardcoded numbers, making its upstream acceptance very unlikely (I anticipate the argument « this should all be done in DTS, especially now that DTS supports nested partitions »). > > Now, I would like to hope that a correct partition scheme that uses all the OpenWRT-accepted features and follows guidelines would be more likely to be accepted in the source than the currently broken, incorrect and incomplete existing one. > I'm not sure but I think https://github.com/openwrt/openwrt/commit/2a598bbaa3f75b7051c2453a6ccf706191cf2153 (kernel: backport mtd support for subpartitions in DT) might be of use here...
diff --git a/target/linux/ramips/dts/RBM33G.dts b/target/linux/ramips/dts/RBM33G.dts index 612dc106ed..a02d03818f 100644 --- a/target/linux/ramips/dts/RBM33G.dts +++ b/target/linux/ramips/dts/RBM33G.dts @@ -104,18 +104,44 @@ reg = <0>; spi-max-frequency = <3125000>; - partition@0 { - label = "routerboot"; - reg = <0x0 0xf000>; - read-only; + partitions { + compatible = "fixed-partitions"; + #address-cells = <1>; + #size-cells = <1>; + + partition@0 { + label = "RouterBoot"; + reg = <0x0 0x40000>; + read-only; + compatible = "fixed-partitions"; + #address-cells = <1>; + #size-cells = <1>; + + routerboot@0 { + reg = <0x0 0xf000>; + read-only; + }; + + hard_config: hard_config@f000 { + reg = <0xf000 0x1000>; + read-only; + }; + + routerboot2@10000 { + reg = <0x10000 0xf000>; + read-only; + }; + + soft_config@20000 { + reg = <0x20000 0x1000>; + }; + + bios@30000 { + reg = <0x30000 0x1000>; + read-only; + }; + }; }; - - factory: partition@f000 { - label = "factory"; - reg = <0xf000 0x71000>; - read-only; - }; - }; w25q128@0 { @@ -123,17 +149,30 @@ #size-cells = <1>; compatible = "jedec,spi-nor"; reg = <1>; + // XXX empiric value to obtain actual 10MHz SCK at the chip spi-max-frequency = <3125000>; - partition@40000 { - label = "firmware"; - reg = <0x040000 0xFC0000>; + partitions { + compatible = "fixed-partitions"; + #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; + }; + + firmware@40000 { + reg = <0x040000 0xFC0000>; + }; }; }; }; ðernet { - mtd-mac-address = <&factory 0x0010>; + mtd-mac-address = <&hard_config 0x0010>; mtd-mac-address-increment = <1>; };