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

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.zhang@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!