[PATCH] mtd: rawnand: nand_base: Handle algorithm selection

[Linus Walleij]

Hi William,

so this is the patch that actually solved my bug in the end :)

On Thu, Jan 26, 2023 at 2:14 AM William Zhang
<william.zhang at broadcom.com> wrote:

> On 01/21/2023 03:43 PM, Linus Walleij wrote:
> > For BRCMNAND with 1-bit BCH ECC (BCH-1) such as used on the
> > D-Link DIR-885L and DIR-890L routers, we need to explicitly
> > select the ECC like this in the device tree:
> >
> >    nand-ecc-algo = "bch";
> >    nand-ecc-strength = <1>;
> >    nand-ecc-step-size = <512>;
> >
> > This is handled by the Linux kernel but U-Boot core does
> > not respect this. Fix it up by parsing the algorithm and
> > preserve the behaviour using this property to select
> > software BCH as far as possible.
>
> For 1 bit HW ECC, the BRCMNAND driver only uses HAMMING ECC.  The
> brcmnand_setup_dev function should take care of it with just these two
> properties in the device tress without any code changes:
>      nand-ecc-strength = <1>;
>      nand-ecc-step-size = <512>;
> unless these D-Link device has always been using software BCH-1 and
> wants to continue to use software BCH-1.
>
> BTW,  I didn't see this change from master branch of linux nand base
> driver. The "nand-ecc-algo" is only used by the ecc engine code(ecc.c)
> but this code is not in the u-boot obviously. Were you porting this from
> a different version of linux nand driver?

Rafał has provided the answer already: the D-Link DIR-885L and DIR-890L
did choose to use BCH-1 ECC. The brcmnand controller does support it
in hardware too, if configured correctly.

The way the device tree properties work is that:

    nand-ecc-strength = <1>;
    nand-ecc-step-size = <512>;

will indeed result in 1-bit Hamming just like you say while:

    nand-ecc-algo = "bch";
    nand-ecc-strength = <1>;
    nand-ecc-step-size = <512>;

will explicitly hammer it down to BCH-1. Currently the D-Link devices
are the two only devices I know that does this in the entire world, but
one of them happens to be on my desktop and I think Rafal has the
other one so we need this.

It does not use software ECC, this is just a (maybe non-standard)
way of using the hw ECC in the brcmnand controller.

In brcmnand.c we reach this:

       if (chip->ecc.algo == NAND_ECC_UNKNOWN) {
                if (chip->ecc.strength == 1 && chip->ecc.size == 512)
                        /* Default to Hamming for 1-bit ECC, if unspecified */
                        chip->ecc.algo = NAND_ECC_HAMMING;
                else
                        /* Otherwise, BCH */
                        chip->ecc.algo = NAND_ECC_BCH;
        }

        if (chip->ecc.algo == NAND_ECC_HAMMING && (chip->ecc.strength != 1 ||
                                                   chip->ecc.size != 512)) {
                dev_err(ctrl->dev, "invalid Hamming params: %d bits
per %d bytes\n",
                        chip->ecc.strength, chip->ecc.size);
                return -EINVAL;
        }

Since we now have ecc.algo == NAND_ECC_BCH none of these branches
will be taken and we will not default to hamming.

Next:

        switch (chip->ecc.size) {
        case 512:
                if (chip->ecc.algo == NAND_ECC_HAMMING)
                        cfg->ecc_level = 15;
                else
                        cfg->ecc_level = chip->ecc.strength;
                cfg->sector_size_1k = 0;
                break;

Here cfg->ecc_level will be set to 1 since algo is NAND_ECC_BCH.

And this is what these D-Link devices are using.

I understand that from a Broadcom perspective this may look like
a bit of abusive and unintended way of using the hardware, but
D-Link use it and have burnt this specific usecase into the ROM of
a few million routers so...

Yours,
Linus Walleij