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

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

[Linus Walleij]

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.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/mtd/nand/raw/nand_base.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 9eba360d55f3..872b58ec5f23 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -4487,6 +4487,7 @@ EXPORT_SYMBOL(nand_detect);
 static int nand_dt_init(struct mtd_info *mtd, struct nand_chip *chip, ofnode node)
 {
 	int ret, ecc_mode = -1, ecc_strength, ecc_step;
+	int ecc_algo = NAND_ECC_UNKNOWN;
 	const char *str;
 
 	ret = ofnode_read_s32_default(node, "nand-bus-width", -1);
@@ -4512,10 +4513,13 @@ static int nand_dt_init(struct mtd_info *mtd, struct nand_chip *chip, ofnode nod
 			ecc_mode = NAND_ECC_SOFT_BCH;
 	}
 
-	if (ecc_mode == NAND_ECC_SOFT) {
-		str = ofnode_read_string(node, "nand-ecc-algo");
-		if (str && !strcmp(str, "bch"))
+	str = ofnode_read_string(node, "nand-ecc-algo");
+	if (str && !strcmp(str, "bch")) {
+		ecc_algo = NAND_ECC_BCH;
+		if (ecc_mode == NAND_ECC_SOFT)
 			ecc_mode = NAND_ECC_SOFT_BCH;
+	} else if (!strcmp(str, "hamming")) {
+		ecc_algo = NAND_ECC_HAMMING;
 	}
 
 	ecc_strength = ofnode_read_s32_default(node,
@@ -4529,6 +4533,9 @@ static int nand_dt_init(struct mtd_info *mtd, struct nand_chip *chip, ofnode nod
 		return -EINVAL;
 	}
 
+	if (ecc_algo >= 0)
+		chip->ecc.algo = ecc_algo;
+
 	if (ecc_mode >= 0)
 		chip->ecc.mode = ecc_mode;
 
-- 
2.39.0

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

[William Zhang]

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

Hi Linus,

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?

> 
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>   drivers/mtd/nand/raw/nand_base.c | 13 ++++++++++---
>   1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 9eba360d55f3..872b58ec5f23 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -4487,6 +4487,7 @@ EXPORT_SYMBOL(nand_detect);
>   static int nand_dt_init(struct mtd_info *mtd, struct nand_chip *chip, ofnode node)
>   {
>   	int ret, ecc_mode = -1, ecc_strength, ecc_step;
> +	int ecc_algo = NAND_ECC_UNKNOWN;
>   	const char *str;
>   
>   	ret = ofnode_read_s32_default(node, "nand-bus-width", -1);
> @@ -4512,10 +4513,13 @@ static int nand_dt_init(struct mtd_info *mtd, struct nand_chip *chip, ofnode nod
>   			ecc_mode = NAND_ECC_SOFT_BCH;
>   	}
>   
> -	if (ecc_mode == NAND_ECC_SOFT) {
> -		str = ofnode_read_string(node, "nand-ecc-algo");
> -		if (str && !strcmp(str, "bch"))
> +	str = ofnode_read_string(node, "nand-ecc-algo");
> +	if (str && !strcmp(str, "bch")) {
> +		ecc_algo = NAND_ECC_BCH;
> +		if (ecc_mode == NAND_ECC_SOFT)
>   			ecc_mode = NAND_ECC_SOFT_BCH;
> +	} else if (!strcmp(str, "hamming")) {
> +		ecc_algo = NAND_ECC_HAMMING;
>   	}
>   
>   	ecc_strength = ofnode_read_s32_default(node,
> @@ -4529,6 +4533,9 @@ static int nand_dt_init(struct mtd_info *mtd, struct nand_chip *chip, ofnode nod
>   		return -EINVAL;
>   	}
>   
> +	if (ecc_algo >= 0)
> +		chip->ecc.algo = ecc_algo;
> +
>   	if (ecc_mode >= 0)
>   		chip->ecc.mode = ecc_mode;
>   
> 

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

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

[Rafał Miłecki]

On 26.01.2023 02:14, William Zhang 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.

Please check
arch/arm/boot/dts/bcm5301x-nand-cs0-bch1.dtsi
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/bcm5301x-nand-cs0-bch1.dtsi

It's included by
arch/arm/boot/dts/bcm47094-dlink-dir-885l.dts
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/boot/dts/bcm47094-dlink-dir-885l.dts

I can confirm D-Link decided to use BCH-1 for some of its devices. I
didn't expect it neither and that required fixing up brcmnand driver in
Linux actually.


> 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?

My original proposal for brcmnand looked like this:
[PATCH 3/3] mtd: brcmnand: respect ECC algorithm set by NAND subsystem
https://lore.kernel.org/lkml/1461324197-1333-3-git-send-email-zajec5@gmail.com/

It was reworked by Brian to a more backward compatible solution that got
accepted:
[PATCH] mtd: brcmnand: respect ECC algorithm set by the NAND subsystem
https://lore.kernel.org/lkml/20160426055355.GA25981@localhost/
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=666b65683dad9aa90efaa4aad24ef3710101e3aa

Re: [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@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

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

[William Zhang]

[-- 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 --]

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

[Linus Walleij]

On Sun, Jan 22, 2023 at 12:43 AM Linus Walleij <linus.walleij@linaro.org> 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.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

It's been 1 1/2 month, could we apply this patch?

Yours,
Linus Walleij

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

[Tom Rini]

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

On Wed, Mar 08, 2023 at 12:05:37AM +0100, Linus Walleij wrote:
> On Sun, Jan 22, 2023 at 12:43 AM Linus Walleij <linus.walleij@linaro.org> 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.
> >
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> 
> It's been 1 1/2 month, could we apply this patch?

Can we get an ack/review from someone that chimed in earlier in the
thread with comments please?

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

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

[Michael Nazzareno Trimarchi]

Hi

On Wed, Mar 8, 2023 at 5:55 PM Tom Rini <trini@konsulko.com> wrote:

> On Wed, Mar 08, 2023 at 12:05:37AM +0100, Linus Walleij wrote:
> > On Sun, Jan 22, 2023 at 12:43 AM Linus Walleij <linus.walleij@linaro.org>
> 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.
> > >
> > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> >
> > It's been 1 1/2 month, could we apply this patch?
>
> Can we get an ack/review from someone that chimed in earlier in the
> thread with comments please?
>
>
I was waiting from William Zhang  but I will review and queue the patch

Michael

-- 
> Tom
>

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

[Michael Nazzareno Trimarchi]

Hi Linus


On Sun, Jan 22, 2023 at 12:44 AM Linus Walleij <linus.walleij@linaro.org>
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.
>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  drivers/mtd/nand/raw/nand_base.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/nand_base.c
> b/drivers/mtd/nand/raw/nand_base.c
> index 9eba360d55f3..872b58ec5f23 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -4487,6 +4487,7 @@ EXPORT_SYMBOL(nand_detect);
>  static int nand_dt_init(struct mtd_info *mtd, struct nand_chip *chip,
> ofnode node)
>  {
>         int ret, ecc_mode = -1, ecc_strength, ecc_step;
> +       int ecc_algo = NAND_ECC_UNKNOWN;
>         const char *str;
>
>         ret = ofnode_read_s32_default(node, "nand-bus-width", -1);
> @@ -4512,10 +4513,13 @@ static int nand_dt_init(struct mtd_info *mtd,
> struct nand_chip *chip, ofnode nod
>                         ecc_mode = NAND_ECC_SOFT_BCH;
>         }
>
> -       if (ecc_mode == NAND_ECC_SOFT) {
> -               str = ofnode_read_string(node, "nand-ecc-algo");
> -               if (str && !strcmp(str, "bch"))
> +       str = ofnode_read_string(node, "nand-ecc-algo");
> +       if (str && !strcmp(str, "bch")) {
> +               ecc_algo = NAND_ECC_BCH;
> +               if (ecc_mode == NAND_ECC_SOFT)
>                         ecc_mode = NAND_ECC_SOFT_BCH;
> +       } else if (!strcmp(str, "hamming")) {
> +               ecc_algo = NAND_ECC_HAMMING;
>         }
>
>         ecc_strength = ofnode_read_s32_default(node,
> @@ -4529,6 +4533,9 @@ static int nand_dt_init(struct mtd_info *mtd, struct
> nand_chip *chip, ofnode nod
>                 return -EINVAL;
>         }
>
> +       if (ecc_algo >= 0)
> +               chip->ecc.algo = ecc_algo;
> +
>

I don't see any path where the algo can be < 0

Michael


>         if (ecc_mode >= 0)
>                 chip->ecc.mode = ecc_mode;
>
> --
> 2.39.0
>
>