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

[William Zhang <william.zhang@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 4027 bytes --]

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!

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4212 bytes --]