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

[William Zhang]

Hi Linus and Rafał,

On 01/26/2023 12:59 AM, Linus Walleij wrote:

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.zh...@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

Okay this makes sense now. Thanks for the back porting!

smime.p7s
Description: S/MIME Cryptographic Signature