| Message ID | 20230121234338.2242486-1-linus.walleij@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mtd: rawnand: nand_base: Handle algorithm selection | expand |
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; > >
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
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
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!
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
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?
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
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 > >
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;
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(-)