[PATCH] mtd: rawnand: nand_base: Handle algorithm selection
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;
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...
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...
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.c...
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/?i...
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 bitsper %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
participants (5)
-
Linus Walleij -
Michael Nazzareno Trimarchi -
Rafał Miłecki -
Tom Rini -
William Zhang