[PATCH v1 1/3] ARM: MR26: MR32: remove bogus nand-ecc-algo property

[Christian Lamparter]

Hi,

On Mon, Jun 5, 2023 at 1:19 PM Rafał Miłecki <zajec5 at gmail.com> wrote:
> On 3.06.2023 16:16, Christian Lamparter wrote:
> > | bcm53015-meraki-mr26.dtb: nand-controller at 18028000:
> > |   nand at 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 at 18028000:
> > |   nand at 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 at gmail.com> (via mail)
> > Signed-off-by: Christian Lamparter <chunkeey at 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 at 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