diff mbox series

[OpenWrt-Devel,2/2] ramips: fix RBM11G name and partitioning

Message ID 20180719171233.43763-3-hacks@slashdirt.org
State Changes Requested
Delegated to: John Crispin
Headers show
Series ramips: fix RBMxxG name and partitionning | expand

Commit Message

Thibaut July 19, 2018, 5:12 p.m. UTC 
This patch improves faf64056ddd46992a75b1e277d94541c7251035c by setting
the correct 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, with an overlapping "RouterBoot" top level
partition added for clarity due to the many holes.

The resulting partition scheme looks like this:
[    2.477826] Creating 7 MTD partitions on "spi0.0":
[    2.482604] 0x000000000000-0x000000040000 : "RouterBoot"
[    2.488948] 0x000000000000-0x00000000f000 : "routerboot"
[    2.495289] 0x00000000f000-0x000000010000 : "hard_config"
[    2.501596] 0x000000010000-0x00000001f000 : "routerboot2"
[    2.507966] 0x000000020000-0x000000021000 : "soft_config"
[    2.514307] 0x000000030000-0x000000031000 : "bios"
[    2.520108] 0x000000040000-0x000001000000 : "firmware"

The device name is corrected to match the hardware-stored (in hard_config)
device name.

Leave a note in DTS to mention this device supports hardware crypto.
Leave a note in DTS to explain how the original author selected the SPI speed.

Note: more work is required to get rbcfg working on this device due to
endianness.

Tested-by: Tobias Schramm <tobleminer@gmail.com>
Signed-off-by: Thibaut VARÈNE <hacks@slashdirt.org>
---
 target/linux/ramips/dts/RBM11G.dts | 62 +++++++++++++++++++++++++++-----------
 1 file changed, 45 insertions(+), 17 deletions(-)

Comments

Mathias Kresin July 19, 2018, 5:46 p.m. UTC | #1 
2018-07-19 19:12 GMT+02:00 Thibaut VARÈNE <hacks@slashdirt.org>:
> This patch improves faf64056ddd46992a75b1e277d94541c7251035c by setting
> the correct 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, with an overlapping "RouterBoot" top level
> partition added for clarity due to the many holes.
>
> The resulting partition scheme looks like this:
> [    2.477826] Creating 7 MTD partitions on "spi0.0":
> [    2.482604] 0x000000000000-0x000000040000 : "RouterBoot"
> [    2.488948] 0x000000000000-0x00000000f000 : "routerboot"
> [    2.495289] 0x00000000f000-0x000000010000 : "hard_config"
> [    2.501596] 0x000000010000-0x00000001f000 : "routerboot2"
> [    2.507966] 0x000000020000-0x000000021000 : "soft_config"
> [    2.514307] 0x000000030000-0x000000031000 : "bios"
> [    2.520108] 0x000000040000-0x000001000000 : "firmware"
>
> The device name is corrected to match the hardware-stored (in hard_config)
> device name.
>
> Leave a note in DTS to mention this device supports hardware crypto.
> Leave a note in DTS to explain how the original author selected the SPI speed.
>
> Note: more work is required to get rbcfg working on this device due to
> endianness.
>
> Tested-by: Tobias Schramm <tobleminer@gmail.com>
> Signed-off-by: Thibaut VARÈNE <hacks@slashdirt.org>

FYI, I already NAK'ed the very same patch on github after a way to
short conversation on IRC follow by some more words on github.

I neither see the need to add notes for not yet working nodes
[personal preference] to the device tree source file, nor the need to
to create the overlapping partitions "RouterBoot" +
routerboot/hard_config/routerboot2/... [personal preference].

To get the dt compiler accepting the overlapping partitions without a
warning, a style was chosen completely different from all other dts
files in the target [maintenance reason]. Furthermore, nodes sharing
the same reg are usually (always?) expressed as child nodes in the
devicetree similar to [technical reason]:

partitions {
    compatible = "fixed-partitions";

    partition@0 {
        reg = <0 0x3000>;

        subpartition@0 {
            reg = <0 0x1000>;
        };

        subpartition@1000 {
            reg = <0x1000 0x2000>;
        };
    };

    partition@3000 {
        reg = <0x3000 0x10000>;
    };
};

To my knowledge, the above isn't possible with fixed-partitions. Which
either means fixed-partitions misses a feature or someone tries to use
it in a way not intended.

This time I'll leave it up to someone else to make a call. I tried my
best to turn it into something that I'm fine to accept and failed.

Mathias
Thibaut July 19, 2018, 5:54 p.m. UTC | #2 
> On 19 Jul 2018, at 19:46, Mathias Kresin <dev@kresin.me> wrote:
> 
> 
> To get the dt compiler accepting the overlapping partitions without a
> warning, a style was chosen completely different from all other dts
> files in the target [maintenance reason]. Furthermore, nodes sharing
> the same reg are usually (always?) expressed as child nodes in the
> devicetree similar to [technical reason]:
> 
> partitions {
>    compatible = "fixed-partitions";
> 
>    partition@0 {
>        reg = <0 0x3000>;
> 
>        subpartition@0 {
>            reg = <0 0x1000>;
>        };
> 
>        subpartition@1000 {
>            reg = <0x1000 0x2000>;
>        };
>    };
> 
>    partition@3000 {
>        reg = <0x3000 0x10000>;
>    };
> };
> 
> To my knowledge, the above isn't possible with fixed-partitions.

I don’t see why. AIUI DTS syntax is respected, all nodes have a unique name and all nodes refer to their top boundary as the unit address.

> Which
> either means fixed-partitions misses a feature or someone tries to use
> it in a way not intended.

The above is in line with the canonical way to define partitions in DTS, as documented in Documentation/devicetree/bindings/mtd/partition.txt

This method is apparently used in all bcm targets, as well as ath79, ipq and lantiq. I’d suggest that _not_ using this method is a bug, and not the correct way to go, as evidenced by the aforementioned documentation. Quoting:

	For backwards compatibility partitions as direct subnodes of the flash device are
	supported. This use is discouraged.

T.
Thomas Richard via openwrt-devel July 20, 2018, 5:57 p.m. UTC | #3 
The sender domain has a DMARC Reject/Quarantine policy which disallows
sending mailing list messages using the original "From" header.

To mitigate this problem, the original message has been wrapped
automatically by the mailing list software.
On Thu, Jul 19, 2018 at 7:12 PM Thibaut VARÈNE <hacks@slashdirt.org> wrote:
>
> This patch improves faf64056ddd46992a75b1e277d94541c7251035c by setting
> the correct 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, with an overlapping "RouterBoot" top level
> partition added for clarity due to the many holes.
>
> The resulting partition scheme looks like this:
> [    2.477826] Creating 7 MTD partitions on "spi0.0":
> [    2.482604] 0x000000000000-0x000000040000 : "RouterBoot"
> [    2.488948] 0x000000000000-0x00000000f000 : "routerboot"
> [    2.495289] 0x00000000f000-0x000000010000 : "hard_config"
> [    2.501596] 0x000000010000-0x00000001f000 : "routerboot2"
> [    2.507966] 0x000000020000-0x000000021000 : "soft_config"
> [    2.514307] 0x000000030000-0x000000031000 : "bios"
> [    2.520108] 0x000000040000-0x000001000000 : "firmware"
>
> The device name is corrected to match the hardware-stored (in hard_config)
> device name.
>
> Leave a note in DTS to mention this device supports hardware crypto.
I find this bit of information strange since it has nothing to do with
the other changes. also: what kind of hardware crypto does it support?

> Leave a note in DTS to explain how the original author selected the SPI speed.
>
> Note: more work is required to get rbcfg working on this device due to
> endianness.
>
> Tested-by: Tobias Schramm <tobleminer@gmail.com>
> Signed-off-by: Thibaut VARÈNE <hacks@slashdirt.org>
> ---
>  target/linux/ramips/dts/RBM11G.dts | 62 +++++++++++++++++++++++++++-----------
>  1 file changed, 45 insertions(+), 17 deletions(-)
>
> diff --git a/target/linux/ramips/dts/RBM11G.dts b/target/linux/ramips/dts/RBM11G.dts
> index f312093a22..079b4fc146 100644
> --- a/target/linux/ramips/dts/RBM11G.dts
> +++ b/target/linux/ramips/dts/RBM11G.dts
> @@ -7,7 +7,7 @@
>
>  / {
>         compatible = "mikrotik,rbm11g", "mediatek,mt7621-soc";
> -       model = "MikroTik RBM11G";
> +       model = "MikroTik RouterBOARD M11G";
why do you need to change the model when updating the partitions? the
commit message doesn't explain this

>
>         aliases {
>                 led-status = &led_usr;
> @@ -90,29 +90,54 @@
>                 #size-cells = <1>;
>                 compatible = "jedec,spi-nor";
>                 reg = <0>;
> +               // XXX empiric value to obtain actual 10MHz SCK at the chip
>                 spi-max-frequency = <3125000>;
>
> -               partition@0 {
> -                       label = "routerboot";
> -                       reg = <0x000000 0x00F000>;
> -                       read-only;
> -               };
> -
> -               factory: partition@f000 {
> -                       label = "factory";
> -                       reg = <0x00F000 0x031000>;
> -                       read-only;
> -               };
> -
> -               partition@40000 {
> -                       label = "firmware";
> -                       reg = <0x040000 0xFC0000>;
> +               partitions {
> +                       compatible = "fixed-partitions";
> +                       #address-cells = <1>;
> +                       #size-cells = <1>;
> +
> +                       partition@0 {
> +                               label = "RouterBoot";
> +                               reg = <0x0 0x40000>;
> +                               read-only;
> +                       };
> +
> +                       routerboot@0 {
> +                               reg = <0x0 0xf000>;
> +                               read-only;
> +                       };
isn't the recommended node name "partition" nowadays?
both partitions above would then become "partition@0" -> whether the
second node overwrites the first one (since both node names and
addresses/offsets are identical) or both are added to the resulting
.dtb depends on the dtc (device tree compiler) version, so I highly
recommend *NOT* doing this

> +                       hard_config: hard_config@f000 {
> +                               reg = <0xf000 0x1000>;
> +                               read-only;
> +                       };
> +
> +                       routerboot2@10000 {
> +                               reg = <0x10000 0xf000>;
> +                               read-only;
> +                       };
> +
> +                       soft_config@20000 {
> +                               reg = <0x20000 0x1000>;
> +                       };
> +
> +                       // valid data only extends up to 0x4f but make it 0x1000 to match ar71xx
> +                       bios@30000 {
> +                               reg = <0x30000 0x1000>;
> +                               read-only;
> +                       };
> +
> +                       firmware@40000 {
> +                               reg = <0x040000 0xFC0000>;
> +                       };
>                 };
>         };
>  };
>
>  &ethernet {
> -       mtd-mac-address = <&factory 0x0010>;
> +       mtd-mac-address = <&hard_config 0x0010>;
if you keep only the big RouterBoot partition you can simply use that
here (you just have to add 0xf000 to the offset)


Regards
Martin
Thibaut July 20, 2018, 7:10 p.m. UTC | #4 
> Le 20 juil. 2018 à 19:56, Martin Blumenstingl <martin.blumenstingl@googlemail.com> a écrit :
> 
>> On Thu, Jul 19, 2018 at 7:12 PM Thibaut VARÈNE <hacks@slashdirt.org> wrote:
>> 
>> This patch improves faf64056ddd46992a75b1e277d94541c7251035c by setting
>> the correct 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, with an overlapping "RouterBoot" top level
>> partition added for clarity due to the many holes.
>> 
>> The resulting partition scheme looks like this:
>> [    2.477826] Creating 7 MTD partitions on "spi0.0":
>> [    2.482604] 0x000000000000-0x000000040000 : "RouterBoot"
>> [    2.488948] 0x000000000000-0x00000000f000 : "routerboot"
>> [    2.495289] 0x00000000f000-0x000000010000 : "hard_config"
>> [    2.501596] 0x000000010000-0x00000001f000 : "routerboot2"
>> [    2.507966] 0x000000020000-0x000000021000 : "soft_config"
>> [    2.514307] 0x000000030000-0x000000031000 : "bios"
>> [    2.520108] 0x000000040000-0x000001000000 : "firmware"
>> 
>> The device name is corrected to match the hardware-stored (in hard_config)
>> device name.
>> 
>> Leave a note in DTS to mention this device supports hardware crypto.
> I find this bit of information strange since it has nothing to do with
> the other changes. also: what kind of hardware crypto does it support?

Documentation note as there’s ongoing work to support these devices: easier to grep than to check individual devices specs. If all mt7621s have this onboard device then indeed the note is unnecessary.

>> Leave a note in DTS to explain how the original author selected the SPI speed.
>> 
>> Note: more work is required to get rbcfg working on this device due to
>> endianness.
>> 
>> Tested-by: Tobias Schramm <tobleminer@gmail.com>
>> Signed-off-by: Thibaut VARÈNE <hacks@slashdirt.org>
>> ---
>> target/linux/ramips/dts/RBM11G.dts | 62 +++++++++++++++++++++++++++-----------
>> 1 file changed, 45 insertions(+), 17 deletions(-)
>> 
>> diff --git a/target/linux/ramips/dts/RBM11G.dts b/target/linux/ramips/dts/RBM11G.dts
>> index f312093a22..079b4fc146 100644
>> --- a/target/linux/ramips/dts/RBM11G.dts
>> +++ b/target/linux/ramips/dts/RBM11G.dts
>> @@ -7,7 +7,7 @@
>> 
>> / {
>>        compatible = "mikrotik,rbm11g", "mediatek,mt7621-soc";
>> -       model = "MikroTik RBM11G";
>> +       model = "MikroTik RouterBOARD M11G";
> why do you need to change the model when updating the partitions? the
> commit message doesn't explain this

This started as a cleanup patch which I didn’t expect to be this controversial so I didn’t split out this one liner.

>>        aliases {
>>                led-status = &led_usr;
>> @@ -90,29 +90,54 @@
>>                #size-cells = <1>;
>>                compatible = "jedec,spi-nor";
>>                reg = <0>;
>> +               // XXX empiric value to obtain actual 10MHz SCK at the chip
>>                spi-max-frequency = <3125000>;
>> 
>> -               partition@0 {
>> -                       label = "routerboot";
>> -                       reg = <0x000000 0x00F000>;
>> -                       read-only;
>> -               };
>> -
>> -               factory: partition@f000 {
>> -                       label = "factory";
>> -                       reg = <0x00F000 0x031000>;
>> -                       read-only;
>> -               };
>> -
>> -               partition@40000 {
>> -                       label = "firmware";
>> -                       reg = <0x040000 0xFC0000>;
>> +               partitions {
>> +                       compatible = "fixed-partitions";
>> +                       #address-cells = <1>;
>> +                       #size-cells = <1>;
>> +
>> +                       partition@0 {
>> +                               label = "RouterBoot";
>> +                               reg = <0x0 0x40000>;
>> +                               read-only;
>> +                       };
>> +
>> +                       routerboot@0 {
>> +                               reg = <0x0 0xf000>;
>> +                               read-only;
>> +                       };
> isn't the recommended node name "partition" nowadays?

Not according to kernel documentation.

> both partitions above would then become "partition@0" -> whether the
> second node overwrites the first one (since both node names and
> addresses/offsets are identical) or both are added to the resulting
> .dtb depends on the dtc (device tree compiler) version, so I highly
> recommend *NOT* doing this
> 
>> +                       hard_config: hard_config@f000 {
>> +                               reg = <0xf000 0x1000>;
>> +                               read-only;
>> +                       };
>> +
>> +                       routerboot2@10000 {
>> +                               reg = <0x10000 0xf000>;
>> +                               read-only;
>> +                       };
>> +
>> +                       soft_config@20000 {
>> +                               reg = <0x20000 0x1000>;
>> +                       };
>> +
>> +                       // valid data only extends up to 0x4f but make it 0x1000 to match ar71xx
>> +                       bios@30000 {
>> +                               reg = <0x30000 0x1000>;
>> +                               read-only;
>> +                       };
>> +
>> +                       firmware@40000 {
>> +                               reg = <0x040000 0xFC0000>;
>> +                       };
>>                };
>>        };
>> };
>> 
>> &ethernet {
>> -       mtd-mac-address = <&factory 0x0010>;
>> +       mtd-mac-address = <&hard_config 0x0010>;
> if you keep only the big RouterBoot partition you can simply use that
> here (you just have to add 0xf000 to the offset)

I explained in the cover letter why both the top level and the subpartitions are useful. Please refer to that.

Best,
T.
diff mbox series

Patch

diff --git a/target/linux/ramips/dts/RBM11G.dts b/target/linux/ramips/dts/RBM11G.dts
index f312093a22..079b4fc146 100644
--- a/target/linux/ramips/dts/RBM11G.dts
+++ b/target/linux/ramips/dts/RBM11G.dts
@@ -7,7 +7,7 @@ 
 
 / {
 	compatible = "mikrotik,rbm11g", "mediatek,mt7621-soc";
-	model = "MikroTik RBM11G";
+	model = "MikroTik RouterBOARD M11G";
 
 	aliases {
 		led-status = &led_usr;
@@ -90,29 +90,54 @@ 
 		#size-cells = <1>;
 		compatible = "jedec,spi-nor";
 		reg = <0>;
+		// XXX empiric value to obtain actual 10MHz SCK at the chip
 		spi-max-frequency = <3125000>;
 
-		partition@0 {
-			label = "routerboot";
-			reg = <0x000000 0x00F000>;
-			read-only;
-		};
-
-		factory: partition@f000 {
-			label = "factory";
-			reg = <0x00F000 0x031000>;
-			read-only;
-		};
-
-		partition@40000 {
-			label = "firmware";
-			reg = <0x040000 0xFC0000>;
+		partitions {
+			compatible = "fixed-partitions";
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			partition@0 {
+				label = "RouterBoot";
+				reg = <0x0 0x40000>;
+				read-only;
+			};
+
+			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>;
+			};
+
+			// valid data only extends up to 0x4f but make it 0x1000 to match ar71xx
+			bios@30000 {
+				reg = <0x30000 0x1000>;
+				read-only;
+			};
+
+			firmware@40000 {
+				reg = <0x040000 0xFC0000>;
+			};
 		};
 	};
 };
 
 &ethernet {
-	mtd-mac-address = <&factory 0x0010>;
+	mtd-mac-address = <&hard_config 0x0010>;
 	mtd-mac-address-increment = <1>;
 };
 
@@ -133,3 +158,6 @@ 
 &pcie {
 	status = "okay";
 };
+
+// XXX this device has hardware crypto
+//&crypto {};