| Message ID | 869354dba00f01d4e6bde897a44180ad1658389c.1685801691.git.chunkeey@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/3] ARM: MR26: MR32: remove bogus nand-ecc-algo property | expand |
On 3.06.2023 16:16, Christian Lamparter wrote: > | bcm53015-meraki-mr26.dtb: nand-controller@18028000: > | nand@0:nand-ecc-algo:0: 'hw' is not one of ['hamming', 'bch', 'rs'] > | From schema: Documentation/[...]/nand-controller.yaml > | bcm53016-meraki-mr32.dtb: nand-controller@18028000: > | nand@0:nand-ecc-algo:0: 'hw' is not one of ['hamming', 'bch', 'rs'] > | From schema: Documentation/[...]/nand-controller.yaml > > original ECC values for these old Merakis are sadly not > provided by the vendor. It looks like Meraki just stuck > with what Broadcom's SDK was doing... which left it up > to their proprietary nand driver. > > It's clear at least that they used the hardware's ecc > engine, so update the device-tree file accordingly to > specify the nand-controller as the ecc-engine. I believe that initial state can be "setup" at hardware level. I believe Broadcom's bootloader and their SDK driver just reads current ECC setup (which goes down to the hardware level). Years ago I proposed change for brcmnand to do the same (which was apparently a bad idea): [PATCH] mtd: brcmnand: set initial ECC params based on info from HW https://lists.infradead.org/pipermail/linux-mtd/2016-February/065314.html That said I think it still should be possible to determine what algo is used and specify that in DT. > this patch also removes the partition index numbers > from the MR32's partition node-names and does some > whitespace removal in order to fit the comment about > the partition oddities into the 100 characters per > limit. > > Fixes: 935327a73553 ("ARM: dts: BCM5301X: Add DT for Meraki MR26") > Fixes: ec88a9c344d9 ("ARM: BCM5301X: Add DT for Meraki MR32") > Reported-by: Rafał Miłecki <zajec5@gmail.com> (via mail) > Signed-off-by: Christian Lamparter <chunkeey@gmail.com> > > mr32 > --- > arch/arm/boot/dts/bcm53015-meraki-mr26.dts | 68 +++++++++-------- > arch/arm/boot/dts/bcm53016-meraki-mr32.dts | 88 ++++++++++++---------- > 2 files changed, 86 insertions(+), 70 deletions(-) > > diff --git a/arch/arm/boot/dts/bcm53015-meraki-mr26.dts b/arch/arm/boot/dts/bcm53015-meraki-mr26.dts > index a2eee9a1e5a7..9ea4ffc1bb71 100644 > --- a/arch/arm/boot/dts/bcm53015-meraki-mr26.dts > +++ b/arch/arm/boot/dts/bcm53015-meraki-mr26.dts > @@ -9,7 +9,6 @@ > /dts-v1/; > > #include "bcm4708.dtsi" > -#include "bcm5301x-nand-cs0-bch8.dtsi" > #include <dt-bindings/leds/common.h> > > / { > @@ -73,41 +72,50 @@ &gmac3 { > status = "disabled"; > }; > > -&nandcs { > - nand-ecc-algo = "hw"; > +&nand_controller { > + nand@0 { > + compatible = "brcm,nandcs"; > + reg = <0>; > + #address-cells = <1>; > + #size-cells = <1>; > > - partitions { > - compatible = "fixed-partitions"; > - #address-cells = <0x1>; > - #size-cells = <0x1>; > + nand-ecc-engine = <&nand_controller>; > + nand-ecc-strength = <8>; > + nand-ecc-step-size = <512>; If we really can't determine ECC algo maybe we could still have sth like bcm5301x-nand-cs0-FOO.dtsi to match other ECC setup? That way you probably also shouldn't need &nand_controller here. > > - partition@0 { > - label = "u-boot"; > - reg = <0x0 0x200000>; > - read-only; > - }; > + partitions { > + compatible = "fixed-partitions"; > + #address-cells = <0x1>; > + #size-cells = <0x1>; > > - partition@200000 { > - label = "u-boot-env"; > - reg = <0x200000 0x200000>; > - /* empty */ > - }; > + partition@0 { > + label = "u-boot"; > + reg = <0x0 0x200000>; > + read-only; > + }; > > - partition@400000 { > - label = "u-boot-backup"; > - reg = <0x400000 0x200000>; > - /* empty */ > - }; > + partition@200000 { > + label = "u-boot-env"; > + reg = <0x200000 0x200000>; > + /* empty */ > + }; > > - partition@600000 { > - label = "u-boot-env-backup"; > - reg = <0x600000 0x200000>; > - /* empty */ > - }; > + partition@400000 { > + label = "u-boot-backup"; > + reg = <0x400000 0x200000>; > + /* empty */ > + }; > > - partition@800000 { > - label = "ubi"; > - reg = <0x800000 0x7780000>; > + partition@600000 { > + label = "u-boot-env-backup"; > + reg = <0x600000 0x200000>; > + /* empty */ > + }; > + > + partition@800000 { > + label = "ubi"; > + reg = <0x800000 0x7780000>; > + }; > }; > }; > }; > diff --git a/arch/arm/boot/dts/bcm53016-meraki-mr32.dts b/arch/arm/boot/dts/bcm53016-meraki-mr32.dts > index b6a066f949ad..bca39b30ace8 100644 > --- a/arch/arm/boot/dts/bcm53016-meraki-mr32.dts > +++ b/arch/arm/boot/dts/bcm53016-meraki-mr32.dts > @@ -9,7 +9,6 @@ > /dts-v1/; > > #include "bcm4708.dtsi" > -#include "bcm5301x-nand-cs0-bch8.dtsi" > #include <dt-bindings/leds/common.h> > > / { > @@ -124,49 +123,58 @@ &pwm { > pinctrl-0 = <&pinmux_pwm>; > }; > > -&nandcs { > - nand-ecc-algo = "hw"; > - > - partitions { > - /* > - * The partition autodetection does not work for this device. > - * It will only detect the "nvram" partition with an incorrect size. > - * [ 1.721667] 1 bcm47xxpart partitions found on MTD device brcmnand.0 > - * [ 1.727962] Creating 1 MTD partitions on "brcmnand.0": > - * [ 1.733117] 0x000000400000-0x000008000000 : "nvram" > - */ > - > - compatible = "fixed-partitions"; > - #address-cells = <0x1>; > - #size-cells = <0x1>; > - > - partition0@0 { > - label = "u-boot"; > - reg = <0x0 0x100000>; > - read-only; > - }; > +&nand_controller { > + nand@0 { > + compatible = "brcm,nandcs"; > + reg = <0>; > + #address-cells = <1>; > + #size-cells = <1>; > > - partition1@100000 { > - label = "bootkernel1"; > - reg = <0x100000 0x300000>; > - read-only; > - }; > + nand-ecc-engine = <&nand_controller>; > + nand-ecc-strength = <8>; > + nand-ecc-step-size = <512>; > + > + partitions { > + /* > + * The partition autodetection does not work for this device. > + * It will only detect the "nvram" partition with an incorrect size. > + * [ 1.721667] 1 bcm47xxpart partitions found on MTD device brcmnand.0 > + * [ 1.727962] Creating 1 MTD partitions on "brcmnand.0": > + * [ 1.733117] 0x000000400000-0x000008000000 : "nvram" > + */ > + > + compatible = "fixed-partitions"; > + #address-cells = <0x1>; > + #size-cells = <0x1>; > + > + partition@0 { > + label = "u-boot"; > + reg = <0x0 0x100000>; > + read-only; > + }; > > - partition2@400000 { > - label = "nvram"; > - reg = <0x400000 0x100000>; > - read-only; > - }; > + partition@100000 { > + label = "bootkernel1"; > + reg = <0x100000 0x300000>; > + read-only; > + }; > > - partition3@500000 { > - label = "bootkernel2"; > - reg = <0x500000 0x300000>; > - read-only; > - }; > + partition@400000 { > + label = "nvram"; > + reg = <0x400000 0x100000>; > + read-only; > + }; > > - partition4@800000 { > - label = "ubi"; > - reg = <0x800000 0x7780000>; > + partition@500000 { > + label = "bootkernel2"; > + reg = <0x500000 0x300000>; > + read-only; > + }; > + > + partition@800000 { > + label = "ubi"; > + reg = <0x800000 0x7780000>; > + }; > }; > }; > };
Hi, On Mon, Jun 5, 2023 at 1:19 PM Rafał Miłecki <zajec5@gmail.com> wrote: > On 3.06.2023 16:16, Christian Lamparter wrote: > > | bcm53015-meraki-mr26.dtb: nand-controller@18028000: > > | nand@0:nand-ecc-algo:0: 'hw' is not one of ['hamming', 'bch', 'rs'] > > | From schema: Documentation/[...]/nand-controller.yaml > > | bcm53016-meraki-mr32.dtb: nand-controller@18028000: > > | nand@0:nand-ecc-algo:0: 'hw' is not one of ['hamming', 'bch', 'rs'] > > | From schema: Documentation/[...]/nand-controller.yaml > > > > original ECC values for these old Merakis are sadly not > > provided by the vendor. It looks like Meraki just stuck > > with what Broadcom's SDK was doing... which left it up > > to their proprietary nand driver. > > > > It's clear at least that they used the hardware's ecc > > engine, so update the device-tree file accordingly to > > specify the nand-controller as the ecc-engine. > > I believe that initial state can be "setup" at hardware level. I believe > Broadcom's bootloader and their SDK driver just reads current ECC setup > (which goes down to the hardware level). > > Years ago I proposed change for brcmnand to do the same (which was > apparently a bad idea): > [PATCH] mtd: brcmnand: set initial ECC params based on info from HW > https://lists.infradead.org/pipermail/linux-mtd/2016-February/065314.html > > That said I think it still should be possible to determine what algo is > used and specify that in DT. I just remembered something possibly very important. See, the MR26 and MR32 use both one big UBI partition to store the kernel (blob), rootfs (squashfs), "storage" (ubifs), caldata/nvmem in individual UBI volumes. Now, this one "weird" thing with ubi is: | Q: Why UBI does not use OOB area of NAND flashes? | A: Because many flashes (e.g., NOR) do not have OOB and UBI was designed to be generic. Also, modern | MLC NAND flashes use whole OOB area for the ECC checksum, so there is no room for application data. | But of course, things could be optimized for SLC NAND flashes if UBI used the space available in the OOB area. | This is not implemented, but one could probably do this. <http://www.linux-mtd.infradead.org/faq/ubi.html#L_why_no_oob> so the comment about "It's clear at least that they used the hardware's ecc..." could be very wrong, because they could just get away with not using any of the NAND-Chips or SOCs ECC features. The reason why nand-ecc-step-size and nand-ecc-strength are there in the both MR32+MR26 DTS is because it tells UBI about the supposed layout of the flash (Note: This does not necessarily have to exactly match the NAND-Chips real layout, since you can force it through ubi-parameters too.). UBI then uses this information to store two headers (EC and VID) at specific locations based on the supplied information. This is also in the FAQ under these topics. <http://www.linux-mtd.infradead.org/doc/ubi.html#L_ubi_headers> <http://www.linux-mtd.infradead.org/faq/ubi.html#L_find_min_io_size> <http://www.linux-mtd.infradead.org/faq/ubi.html#L_force_no_subpage> > > this patch also removes the partition index numbers > > from the MR32's partition node-names and does some > > whitespace removal in order to fit the comment about > > the partition oddities into the 100 characters per > > limit. > > > > Fixes: 935327a73553 ("ARM: dts: BCM5301X: Add DT for Meraki MR26") > > Fixes: ec88a9c344d9 ("ARM: BCM5301X: Add DT for Meraki MR32") > > Reported-by: Rafał Miłecki <zajec5@gmail.com> (via mail) > > Signed-off-by: Christian Lamparter <chunkeey@gmail.com> > > > > mr32 > > --- > > arch/arm/boot/dts/bcm53015-meraki-mr26.dts | 68 +++++++++-------- > > arch/arm/boot/dts/bcm53016-meraki-mr32.dts | 88 ++++++++++++---------- > > 2 files changed, 86 insertions(+), 70 deletions(-) > > > > diff --git a/arch/arm/boot/dts/bcm53015-meraki-mr26.dts b/arch/arm/boot/dts/bcm53015-meraki-mr26.dts > > index a2eee9a1e5a7..9ea4ffc1bb71 100644 > > --- a/arch/arm/boot/dts/bcm53015-meraki-mr26.dts > > +++ b/arch/arm/boot/dts/bcm53015-meraki-mr26.dts > > @@ -9,7 +9,6 @@ > > /dts-v1/; > > > > #include "bcm4708.dtsi" > > -#include "bcm5301x-nand-cs0-bch8.dtsi" > > #include <dt-bindings/leds/common.h> > > > > / { > > @@ -73,41 +72,50 @@ &gmac3 { > > status = "disabled"; > > }; > > > > -&nandcs { > > - nand-ecc-algo = "hw"; > > +&nand_controller { > > + nand@0 { > > + compatible = "brcm,nandcs"; > > + reg = <0>; > > + #address-cells = <1>; > > + #size-cells = <1>; > > > > - partitions { > > - compatible = "fixed-partitions"; > > - #address-cells = <0x1>; > > - #size-cells = <0x1>; > > + nand-ecc-engine = <&nand_controller>; > > + nand-ecc-strength = <8>; > > + nand-ecc-step-size = <512>; > > If we really can't determine ECC algo maybe we could still have sth like > bcm5301x-nand-cs0-FOO.dtsi > to match other ECC setup? > > That way you probably also shouldn't need &nand_controller here. > yes, this might be the way. Regards, Christian
diff --git a/arch/arm/boot/dts/bcm53015-meraki-mr26.dts b/arch/arm/boot/dts/bcm53015-meraki-mr26.dts index a2eee9a1e5a7..9ea4ffc1bb71 100644 --- a/arch/arm/boot/dts/bcm53015-meraki-mr26.dts +++ b/arch/arm/boot/dts/bcm53015-meraki-mr26.dts @@ -9,7 +9,6 @@ /dts-v1/; #include "bcm4708.dtsi" -#include "bcm5301x-nand-cs0-bch8.dtsi" #include <dt-bindings/leds/common.h> / { @@ -73,41 +72,50 @@ &gmac3 { status = "disabled"; }; -&nandcs { - nand-ecc-algo = "hw"; +&nand_controller { + nand@0 { + compatible = "brcm,nandcs"; + reg = <0>; + #address-cells = <1>; + #size-cells = <1>; - partitions { - compatible = "fixed-partitions"; - #address-cells = <0x1>; - #size-cells = <0x1>; + nand-ecc-engine = <&nand_controller>; + nand-ecc-strength = <8>; + nand-ecc-step-size = <512>; - partition@0 { - label = "u-boot"; - reg = <0x0 0x200000>; - read-only; - }; + partitions { + compatible = "fixed-partitions"; + #address-cells = <0x1>; + #size-cells = <0x1>; - partition@200000 { - label = "u-boot-env"; - reg = <0x200000 0x200000>; - /* empty */ - }; + partition@0 { + label = "u-boot"; + reg = <0x0 0x200000>; + read-only; + }; - partition@400000 { - label = "u-boot-backup"; - reg = <0x400000 0x200000>; - /* empty */ - }; + partition@200000 { + label = "u-boot-env"; + reg = <0x200000 0x200000>; + /* empty */ + }; - partition@600000 { - label = "u-boot-env-backup"; - reg = <0x600000 0x200000>; - /* empty */ - }; + partition@400000 { + label = "u-boot-backup"; + reg = <0x400000 0x200000>; + /* empty */ + }; - partition@800000 { - label = "ubi"; - reg = <0x800000 0x7780000>; + partition@600000 { + label = "u-boot-env-backup"; + reg = <0x600000 0x200000>; + /* empty */ + }; + + partition@800000 { + label = "ubi"; + reg = <0x800000 0x7780000>; + }; }; }; }; diff --git a/arch/arm/boot/dts/bcm53016-meraki-mr32.dts b/arch/arm/boot/dts/bcm53016-meraki-mr32.dts index b6a066f949ad..bca39b30ace8 100644 --- a/arch/arm/boot/dts/bcm53016-meraki-mr32.dts +++ b/arch/arm/boot/dts/bcm53016-meraki-mr32.dts @@ -9,7 +9,6 @@ /dts-v1/; #include "bcm4708.dtsi" -#include "bcm5301x-nand-cs0-bch8.dtsi" #include <dt-bindings/leds/common.h> / { @@ -124,49 +123,58 @@ &pwm { pinctrl-0 = <&pinmux_pwm>; }; -&nandcs { - nand-ecc-algo = "hw"; - - partitions { - /* - * The partition autodetection does not work for this device. - * It will only detect the "nvram" partition with an incorrect size. - * [ 1.721667] 1 bcm47xxpart partitions found on MTD device brcmnand.0 - * [ 1.727962] Creating 1 MTD partitions on "brcmnand.0": - * [ 1.733117] 0x000000400000-0x000008000000 : "nvram" - */ - - compatible = "fixed-partitions"; - #address-cells = <0x1>; - #size-cells = <0x1>; - - partition0@0 { - label = "u-boot"; - reg = <0x0 0x100000>; - read-only; - }; +&nand_controller { + nand@0 { + compatible = "brcm,nandcs"; + reg = <0>; + #address-cells = <1>; + #size-cells = <1>; - partition1@100000 { - label = "bootkernel1"; - reg = <0x100000 0x300000>; - read-only; - }; + nand-ecc-engine = <&nand_controller>; + nand-ecc-strength = <8>; + nand-ecc-step-size = <512>; + + partitions { + /* + * The partition autodetection does not work for this device. + * It will only detect the "nvram" partition with an incorrect size. + * [ 1.721667] 1 bcm47xxpart partitions found on MTD device brcmnand.0 + * [ 1.727962] Creating 1 MTD partitions on "brcmnand.0": + * [ 1.733117] 0x000000400000-0x000008000000 : "nvram" + */ + + compatible = "fixed-partitions"; + #address-cells = <0x1>; + #size-cells = <0x1>; + + partition@0 { + label = "u-boot"; + reg = <0x0 0x100000>; + read-only; + }; - partition2@400000 { - label = "nvram"; - reg = <0x400000 0x100000>; - read-only; - }; + partition@100000 { + label = "bootkernel1"; + reg = <0x100000 0x300000>; + read-only; + }; - partition3@500000 { - label = "bootkernel2"; - reg = <0x500000 0x300000>; - read-only; - }; + partition@400000 { + label = "nvram"; + reg = <0x400000 0x100000>; + read-only; + }; - partition4@800000 { - label = "ubi"; - reg = <0x800000 0x7780000>; + partition@500000 { + label = "bootkernel2"; + reg = <0x500000 0x300000>; + read-only; + }; + + partition@800000 { + label = "ubi"; + reg = <0x800000 0x7780000>; + }; }; }; };
| bcm53015-meraki-mr26.dtb: nand-controller@18028000: | nand@0:nand-ecc-algo:0: 'hw' is not one of ['hamming', 'bch', 'rs'] | From schema: Documentation/[...]/nand-controller.yaml | bcm53016-meraki-mr32.dtb: nand-controller@18028000: | nand@0:nand-ecc-algo:0: 'hw' is not one of ['hamming', 'bch', 'rs'] | From schema: Documentation/[...]/nand-controller.yaml original ECC values for these old Merakis are sadly not provided by the vendor. It looks like Meraki just stuck with what Broadcom's SDK was doing... which left it up to their proprietary nand driver. It's clear at least that they used the hardware's ecc engine, so update the device-tree file accordingly to specify the nand-controller as the ecc-engine. this patch also removes the partition index numbers from the MR32's partition node-names and does some whitespace removal in order to fit the comment about the partition oddities into the 100 characters per limit. Fixes: 935327a73553 ("ARM: dts: BCM5301X: Add DT for Meraki MR26") Fixes: ec88a9c344d9 ("ARM: BCM5301X: Add DT for Meraki MR32") Reported-by: Rafał Miłecki <zajec5@gmail.com> (via mail) Signed-off-by: Christian Lamparter <chunkeey@gmail.com> mr32 --- arch/arm/boot/dts/bcm53015-meraki-mr26.dts | 68 +++++++++-------- arch/arm/boot/dts/bcm53016-meraki-mr32.dts | 88 ++++++++++++---------- 2 files changed, 86 insertions(+), 70 deletions(-)