* [PATCH] mtd: rawnand: stm32_fmc2: fix broken ECC
@ 2020-10-29 16:38 Christophe Kerello
2020-10-30 8:19 ` Miquel Raynal
0 siblings, 1 reply; 4+ messages in thread
From: Christophe Kerello @ 2020-10-29 16:38 UTC (permalink / raw)
To: miquel.raynal, richard, vigneshr
Cc: Christophe Kerello, linux-mtd, linux-kernel, linux-stm32
Since commit d7157ff49a5b ("mtd: rawnand: Use the ECC framework user
input parsing bits"), ECC are broken in FMC2 driver in case of
nand-ecc-step-size and nand-ecc-strength are not set in the device tree.
The default user configuration set in FMC2 driver is lost when
rawnand_dt_init function is called. To avoid to lose the default user
configuration, it is needed to move it in the new user_conf structure.
Signed-off-by: Christophe Kerello <christophe.kerello@st.com>
Fixes: d7157ff49a5b ("mtd: rawnand: Use the ECC framework user input parsing bits")
---
drivers/mtd/nand/raw/stm32_fmc2_nand.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/nand/raw/stm32_fmc2_nand.c b/drivers/mtd/nand/raw/stm32_fmc2_nand.c
index b31a581..dc86ac9 100644
--- a/drivers/mtd/nand/raw/stm32_fmc2_nand.c
+++ b/drivers/mtd/nand/raw/stm32_fmc2_nand.c
@@ -1846,6 +1846,7 @@ static int stm32_fmc2_nfc_probe(struct platform_device *pdev)
struct resource *res;
struct mtd_info *mtd;
struct nand_chip *chip;
+ struct nand_device *nanddev;
struct resource cres;
int chip_cs, mem_region, ret, irq;
int start_region = 0;
@@ -1952,10 +1953,11 @@ static int stm32_fmc2_nfc_probe(struct platform_device *pdev)
chip->options |= NAND_BUSWIDTH_AUTO | NAND_NO_SUBPAGE_WRITE |
NAND_USES_DMA;
- /* Default ECC settings */
+ /* Default ECC user settings */
chip->ecc.engine_type = NAND_ECC_ENGINE_TYPE_ON_HOST;
- chip->ecc.size = FMC2_ECC_STEP_SIZE;
- chip->ecc.strength = FMC2_ECC_BCH8;
+ nanddev = mtd_to_nanddev(mtd);
+ nanddev->ecc.user_conf.step_size = FMC2_ECC_STEP_SIZE;
+ nanddev->ecc.user_conf.strength = FMC2_ECC_BCH8;
/* Scan to find existence of the device */
ret = nand_scan(chip, nand->ncs);
--
1.9.1
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH] mtd: rawnand: stm32_fmc2: fix broken ECC 2020-10-29 16:38 [PATCH] mtd: rawnand: stm32_fmc2: fix broken ECC Christophe Kerello @ 2020-10-30 8:19 ` Miquel Raynal 2020-10-30 8:31 ` Christophe Kerello 0 siblings, 1 reply; 4+ messages in thread From: Miquel Raynal @ 2020-10-30 8:19 UTC (permalink / raw) To: Christophe Kerello Cc: richard, linux-stm32, linux-mtd, vigneshr, linux-kernel Hi Christophe, Christophe Kerello <christophe.kerello@st.com> wrote on Thu, 29 Oct 2020 17:38:12 +0100: > Since commit d7157ff49a5b ("mtd: rawnand: Use the ECC framework user > input parsing bits"), ECC are broken in FMC2 driver in case of > nand-ecc-step-size and nand-ecc-strength are not set in the device tree. > The default user configuration set in FMC2 driver is lost when > rawnand_dt_init function is called. To avoid to lose the default user > configuration, it is needed to move it in the new user_conf structure. > > Signed-off-by: Christophe Kerello <christophe.kerello@st.com> > Fixes: d7157ff49a5b ("mtd: rawnand: Use the ECC framework user input parsing bits") > --- > drivers/mtd/nand/raw/stm32_fmc2_nand.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/mtd/nand/raw/stm32_fmc2_nand.c b/drivers/mtd/nand/raw/stm32_fmc2_nand.c > index b31a581..dc86ac9 100644 > --- a/drivers/mtd/nand/raw/stm32_fmc2_nand.c > +++ b/drivers/mtd/nand/raw/stm32_fmc2_nand.c > @@ -1846,6 +1846,7 @@ static int stm32_fmc2_nfc_probe(struct platform_device *pdev) > struct resource *res; > struct mtd_info *mtd; > struct nand_chip *chip; > + struct nand_device *nanddev; > struct resource cres; > int chip_cs, mem_region, ret, irq; > int start_region = 0; > @@ -1952,10 +1953,11 @@ static int stm32_fmc2_nfc_probe(struct platform_device *pdev) > chip->options |= NAND_BUSWIDTH_AUTO | NAND_NO_SUBPAGE_WRITE | > NAND_USES_DMA; > > - /* Default ECC settings */ > + /* Default ECC user settings */ > chip->ecc.engine_type = NAND_ECC_ENGINE_TYPE_ON_HOST; > - chip->ecc.size = FMC2_ECC_STEP_SIZE; > - chip->ecc.strength = FMC2_ECC_BCH8; > + nanddev = mtd_to_nanddev(mtd); > + nanddev->ecc.user_conf.step_size = FMC2_ECC_STEP_SIZE; > + nanddev->ecc.user_conf.strength = FMC2_ECC_BCH8; > > /* Scan to find existence of the device */ > ret = nand_scan(chip, nand->ncs); Sorry for breaking the driver with this change, but now I think we should have all ECC related bits in ->attach() instead of ->probe(). The ->attach() hook is called during the nand_scan() operation and at this point the chip's requirements/layout are known (not before). I know that certain controllers don't really care about that, here your simply hardcode these two fields and you don't need to know anything about the chip's properties. But as a bid to harmonize all drivers with the target of a generic ECC engine in mind, I think it's now time to move these three lines (chip->ecc.* = ...) at the top of ->attach(). Also, these fields should have been populated by the core so perhaps the best approach is to check if the user requirements are synced with the controller's capabilities and error out otherwise? We plan to send a fixes PR for -rc2, if the v2 arrives today I'll integrate it. Thanks, Miquèl ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] mtd: rawnand: stm32_fmc2: fix broken ECC 2020-10-30 8:19 ` Miquel Raynal @ 2020-10-30 8:31 ` Christophe Kerello 2020-10-30 8:36 ` Miquel Raynal 0 siblings, 1 reply; 4+ messages in thread From: Christophe Kerello @ 2020-10-30 8:31 UTC (permalink / raw) To: Miquel Raynal; +Cc: richard, linux-stm32, linux-mtd, vigneshr, linux-kernel Hi Miquel, On 10/30/20 9:19 AM, Miquel Raynal wrote: > Hi Christophe, > > Christophe Kerello <christophe.kerello@st.com> wrote on Thu, 29 Oct > 2020 17:38:12 +0100: > >> Since commit d7157ff49a5b ("mtd: rawnand: Use the ECC framework user >> input parsing bits"), ECC are broken in FMC2 driver in case of >> nand-ecc-step-size and nand-ecc-strength are not set in the device tree. >> The default user configuration set in FMC2 driver is lost when >> rawnand_dt_init function is called. To avoid to lose the default user >> configuration, it is needed to move it in the new user_conf structure. >> >> Signed-off-by: Christophe Kerello <christophe.kerello@st.com> >> Fixes: d7157ff49a5b ("mtd: rawnand: Use the ECC framework user input parsing bits") >> --- >> drivers/mtd/nand/raw/stm32_fmc2_nand.c | 8 +++++--- >> 1 file changed, 5 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/mtd/nand/raw/stm32_fmc2_nand.c b/drivers/mtd/nand/raw/stm32_fmc2_nand.c >> index b31a581..dc86ac9 100644 >> --- a/drivers/mtd/nand/raw/stm32_fmc2_nand.c >> +++ b/drivers/mtd/nand/raw/stm32_fmc2_nand.c >> @@ -1846,6 +1846,7 @@ static int stm32_fmc2_nfc_probe(struct platform_device *pdev) >> struct resource *res; >> struct mtd_info *mtd; >> struct nand_chip *chip; >> + struct nand_device *nanddev; >> struct resource cres; >> int chip_cs, mem_region, ret, irq; >> int start_region = 0; >> @@ -1952,10 +1953,11 @@ static int stm32_fmc2_nfc_probe(struct platform_device *pdev) >> chip->options |= NAND_BUSWIDTH_AUTO | NAND_NO_SUBPAGE_WRITE | >> NAND_USES_DMA; >> >> - /* Default ECC settings */ >> + /* Default ECC user settings */ >> chip->ecc.engine_type = NAND_ECC_ENGINE_TYPE_ON_HOST; >> - chip->ecc.size = FMC2_ECC_STEP_SIZE; >> - chip->ecc.strength = FMC2_ECC_BCH8; >> + nanddev = mtd_to_nanddev(mtd); >> + nanddev->ecc.user_conf.step_size = FMC2_ECC_STEP_SIZE; >> + nanddev->ecc.user_conf.strength = FMC2_ECC_BCH8; >> >> /* Scan to find existence of the device */ >> ret = nand_scan(chip, nand->ncs); > > Sorry for breaking the driver with this change, but now I think we > should have all ECC related bits in ->attach() instead of ->probe(). > The ->attach() hook is called during the nand_scan() operation and at > this point the chip's requirements/layout are known (not before). I > know that certain controllers don't really care about that, here your > simply hardcode these two fields and you don't need to know anything > about the chip's properties. But as a bid to harmonize all drivers with > the target of a generic ECC engine in mind, I think it's now time to > move these three lines (chip->ecc.* = ...) at the top of ->attach(). > Also, these fields should have been populated by the core so perhaps > the best approach is to check if the user requirements are synced with > the controller's capabilities and error out otherwise? > > We plan to send a fixes PR for -rc2, if the v2 arrives today I'll > integrate it. Ok. Issue is that the controller is initialized when stm32_fmc2_nfc_select_chip is called. This function will be called before the ->attach() hook, when the first command will be sent to the NAND device (reset command). So, moving the default ECC initialization needs probably more modifications in the driver. I will try to send a v2 today. Regards, Christophe Kerello. > > Thanks, > Miquèl > ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] mtd: rawnand: stm32_fmc2: fix broken ECC 2020-10-30 8:31 ` Christophe Kerello @ 2020-10-30 8:36 ` Miquel Raynal 0 siblings, 0 replies; 4+ messages in thread From: Miquel Raynal @ 2020-10-30 8:36 UTC (permalink / raw) To: Christophe Kerello Cc: richard, linux-stm32, linux-mtd, vigneshr, linux-kernel Hi Christophe, Christophe Kerello <christophe.kerello@st.com> wrote on Fri, 30 Oct 2020 09:31:25 +0100: > Hi Miquel, > > On 10/30/20 9:19 AM, Miquel Raynal wrote: > > Hi Christophe, > > > > Christophe Kerello <christophe.kerello@st.com> wrote on Thu, 29 Oct > > 2020 17:38:12 +0100: > > > >> Since commit d7157ff49a5b ("mtd: rawnand: Use the ECC framework user > >> input parsing bits"), ECC are broken in FMC2 driver in case of > >> nand-ecc-step-size and nand-ecc-strength are not set in the device tree. > >> The default user configuration set in FMC2 driver is lost when > >> rawnand_dt_init function is called. To avoid to lose the default user > >> configuration, it is needed to move it in the new user_conf structure. > >> > >> Signed-off-by: Christophe Kerello <christophe.kerello@st.com> > >> Fixes: d7157ff49a5b ("mtd: rawnand: Use the ECC framework user input parsing bits") > >> --- > >> drivers/mtd/nand/raw/stm32_fmc2_nand.c | 8 +++++--- > >> 1 file changed, 5 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/mtd/nand/raw/stm32_fmc2_nand.c b/drivers/mtd/nand/raw/stm32_fmc2_nand.c > >> index b31a581..dc86ac9 100644 > >> --- a/drivers/mtd/nand/raw/stm32_fmc2_nand.c > >> +++ b/drivers/mtd/nand/raw/stm32_fmc2_nand.c > >> @@ -1846,6 +1846,7 @@ static int stm32_fmc2_nfc_probe(struct platform_device *pdev) > >> struct resource *res; > >> struct mtd_info *mtd; > >> struct nand_chip *chip; > >> + struct nand_device *nanddev; > >> struct resource cres; > >> int chip_cs, mem_region, ret, irq; > >> int start_region = 0; > >> @@ -1952,10 +1953,11 @@ static int stm32_fmc2_nfc_probe(struct platform_device *pdev) > >> chip->options |= NAND_BUSWIDTH_AUTO | NAND_NO_SUBPAGE_WRITE | > >> NAND_USES_DMA; > >> >> - /* Default ECC settings */ > >> + /* Default ECC user settings */ > >> chip->ecc.engine_type = NAND_ECC_ENGINE_TYPE_ON_HOST; > >> - chip->ecc.size = FMC2_ECC_STEP_SIZE; > >> - chip->ecc.strength = FMC2_ECC_BCH8; > >> + nanddev = mtd_to_nanddev(mtd); > >> + nanddev->ecc.user_conf.step_size = FMC2_ECC_STEP_SIZE; > >> + nanddev->ecc.user_conf.strength = FMC2_ECC_BCH8; > >> >> /* Scan to find existence of the device */ > >> ret = nand_scan(chip, nand->ncs); > > > > Sorry for breaking the driver with this change, but now I think we > > should have all ECC related bits in ->attach() instead of ->probe(). > > The ->attach() hook is called during the nand_scan() operation and at > > this point the chip's requirements/layout are known (not before). I > > know that certain controllers don't really care about that, here your > > simply hardcode these two fields and you don't need to know anything > > about the chip's properties. But as a bid to harmonize all drivers with > > the target of a generic ECC engine in mind, I think it's now time to > > move these three lines (chip->ecc.* = ...) at the top of ->attach(). > > Also, these fields should have been populated by the core so perhaps > > the best approach is to check if the user requirements are synced with > > the controller's capabilities and error out otherwise? > > > > We plan to send a fixes PR for -rc2, if the v2 arrives today I'll > > integrate it. > > Ok. Issue is that the controller is initialized when stm32_fmc2_nfc_select_chip is called. This function will be called before the ->attach() hook, when the first command will be sent to the NAND device (reset command). So, moving the default ECC initialization > needs probably more modifications in the driver. > I will try to send a v2 today. The ECC engine is not supposed to be used before nand_scan() and its default state should be disabled. Maybe this driver needs an update about that, but then -hopefully- it will be pretty straightforward. No hurry though, if the fix is not ready we'll wait an additional week (it will be in next as soon as it is ready anyway). Thanks, Miquèl ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/ ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-10-30 8:37 UTC | newest] Thread overview: 4+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2020-10-29 16:38 [PATCH] mtd: rawnand: stm32_fmc2: fix broken ECC Christophe Kerello 2020-10-30 8:19 ` Miquel Raynal 2020-10-30 8:31 ` Christophe Kerello 2020-10-30 8:36 ` Miquel Raynal
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).