Hi, On Mon, Jun 5, 2023 at 1:19 PM Rafał Miłecki <zajec5@xxxxxxxxx> 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@xxxxxxxxx> (via mail) > > Signed-off-by: Christian Lamparter <chunkeey@xxxxxxxxx> > > > > 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
