* [PATCH] mtd: nand: omap: Fix BCH bit correction @ 2017-06-06 16:10 Daniel Schultz 2017-06-07 6:45 ` Sascha Hauer 0 siblings, 1 reply; 9+ messages in thread From: Daniel Schultz @ 2017-06-06 16:10 UTC (permalink / raw) To: barebox After commit dec7b4d2bf9 was applied our barebox only corrected the first 512 Bytes of NAND pages. This patch separates between Hamming and BCH when finding out the eccsteps, because BCH always works with 2kB pages. Before this patch: barebox@Phytec phyCORE AM335x:/ nand_bitflip -r -n 5 /dev/nand0.barebox nand0.barebox: Flipping bit 5 @ 1796 nand0.barebox: Flipping bit 6 @ 1258 nand0.barebox: Flipping bit 5 @ 1062 nand0.barebox: Flipping bit 2 @ 1399 nand0.barebox: Flipping bit 6 @ 1243 No bitflips found on block 0, offset 0x00000000 barebox@Phytec phyCORE AM335x:/ nand_bitflip -r -n 5 /dev/nand0.barebox nand0.barebox: Flipping bit 2 @ 872 nand0.barebox: Flipping bit 4 @ 252 nand0.barebox: Flipping bit 3 @ 568 nand0.barebox: Flipping bit 2 @ 247 nand0.barebox: Flipping bit 5 @ 401 page at block 0, offset 0x00000000 has 3 bitflips After this patch: barebox@Phytec phyCORE AM335x:/ nand_bitflip -r -n 5 /dev/nand0.barebox nand0.barebox: Flipping bit 2 @ 1962 nand0.barebox: Flipping bit 0 @ 1563 nand0.barebox: Flipping bit 0 @ 1808 nand0.barebox: Flipping bit 6 @ 1460 nand0.barebox: Flipping bit 7 @ 2034 page at block 0, offset 0x00000000 has 5 bitflips barebox@Phytec phyCORE AM335x:/ nand_bitflip -r -n 5 /dev/nand0.barebox nand0.barebox: Flipping bit 1 @ 1352 nand0.barebox: Flipping bit 7 @ 1542 nand0.barebox: Flipping bit 2 @ 1021 nand0.barebox: Flipping bit 7 @ 691 nand0.barebox: Flipping bit 6 @ 1196 page at block 0, offset 0x00000000 has 10 bitflips, needs cleanup Signed-off-by: Daniel Schultz <d.schultz@phytec.de> --- drivers/mtd/nand/nand_omap_gpmc.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/drivers/mtd/nand/nand_omap_gpmc.c b/drivers/mtd/nand/nand_omap_gpmc.c index 05c8486..61220da 100644 --- a/drivers/mtd/nand/nand_omap_gpmc.c +++ b/drivers/mtd/nand/nand_omap_gpmc.c @@ -302,10 +302,17 @@ static int omap_correct_bch(struct mtd_info *mtd, uint8_t *dat, unsigned int err_loc[8]; int bitflip_count; int bch_max_err; + int eccsteps; - int eccsteps = (nand->ecc.mode == NAND_ECC_HW) && - (nand->ecc.size == 2048) ? 4 : 1; int eccsize = oinfo->nand.ecc.bytes; + if (oinfo->ecc_mode == OMAP_ECC_HAMMING_CODE_HW_ROMCODE) + if ((nand->ecc.mode == NAND_ECC_HW) && + (nand->ecc.size == 2048)) + eccsteps = 4; + else + eccsteps = 1; + else + eccsteps = oinfo->nand.ecc.steps; switch (oinfo->ecc_mode) { case OMAP_ECC_BCH8_CODE_HW: -- 1.9.1 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mtd: nand: omap: Fix BCH bit correction 2017-06-06 16:10 [PATCH] mtd: nand: omap: Fix BCH bit correction Daniel Schultz @ 2017-06-07 6:45 ` Sascha Hauer 2017-06-07 6:49 ` Sascha Hauer 0 siblings, 1 reply; 9+ messages in thread From: Sascha Hauer @ 2017-06-07 6:45 UTC (permalink / raw) To: Daniel Schultz; +Cc: barebox, Matt Reimer +Cc Matt Reimer <mreimer@sdgsystems.com> On Tue, Jun 06, 2017 at 06:10:25PM +0200, Daniel Schultz wrote: > After commit dec7b4d2bf9 was applied our barebox only corrected the > first 512 Bytes of NAND pages. > > This patch separates between Hamming and BCH when finding out the > eccsteps, because BCH always works with 2kB pages. > > Before this patch: > > barebox@Phytec phyCORE AM335x:/ nand_bitflip -r -n 5 /dev/nand0.barebox > nand0.barebox: Flipping bit 5 @ 1796 > nand0.barebox: Flipping bit 6 @ 1258 > nand0.barebox: Flipping bit 5 @ 1062 > nand0.barebox: Flipping bit 2 @ 1399 > nand0.barebox: Flipping bit 6 @ 1243 > No bitflips found on block 0, offset 0x00000000 > barebox@Phytec phyCORE AM335x:/ nand_bitflip -r -n 5 /dev/nand0.barebox > nand0.barebox: Flipping bit 2 @ 872 > nand0.barebox: Flipping bit 4 @ 252 > nand0.barebox: Flipping bit 3 @ 568 > nand0.barebox: Flipping bit 2 @ 247 > nand0.barebox: Flipping bit 5 @ 401 > page at block 0, offset 0x00000000 has 3 bitflips > > After this patch: > > barebox@Phytec phyCORE AM335x:/ nand_bitflip -r -n 5 /dev/nand0.barebox > nand0.barebox: Flipping bit 2 @ 1962 > nand0.barebox: Flipping bit 0 @ 1563 > nand0.barebox: Flipping bit 0 @ 1808 > nand0.barebox: Flipping bit 6 @ 1460 > nand0.barebox: Flipping bit 7 @ 2034 > page at block 0, offset 0x00000000 has 5 bitflips > barebox@Phytec phyCORE AM335x:/ nand_bitflip -r -n 5 /dev/nand0.barebox > nand0.barebox: Flipping bit 1 @ 1352 > nand0.barebox: Flipping bit 7 @ 1542 > nand0.barebox: Flipping bit 2 @ 1021 > nand0.barebox: Flipping bit 7 @ 691 > nand0.barebox: Flipping bit 6 @ 1196 > page at block 0, offset 0x00000000 has 10 bitflips, needs cleanup > > Signed-off-by: Daniel Schultz <d.schultz@phytec.de> > --- > drivers/mtd/nand/nand_omap_gpmc.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/mtd/nand/nand_omap_gpmc.c b/drivers/mtd/nand/nand_omap_gpmc.c > index 05c8486..61220da 100644 > --- a/drivers/mtd/nand/nand_omap_gpmc.c > +++ b/drivers/mtd/nand/nand_omap_gpmc.c > @@ -302,10 +302,17 @@ static int omap_correct_bch(struct mtd_info *mtd, uint8_t *dat, > unsigned int err_loc[8]; > int bitflip_count; > int bch_max_err; > + int eccsteps; > > - int eccsteps = (nand->ecc.mode == NAND_ECC_HW) && > - (nand->ecc.size == 2048) ? 4 : 1; > int eccsize = oinfo->nand.ecc.bytes; > + if (oinfo->ecc_mode == OMAP_ECC_HAMMING_CODE_HW_ROMCODE) This is wrong. When in Hamming ECC mode you shouldn't get into this function. The test should always fail. > + if ((nand->ecc.mode == NAND_ECC_HW) && > + (nand->ecc.size == 2048)) > + eccsteps = 4; > + else > + eccsteps = 1; The question is why ecc.size is set to the wrong value in the first place: case OMAP_ECC_BCH8_CODE_HW: ... oinfo->nand.ecc.size = 512 * 4; This seems to be wrong. The BCH controller works in 512 Byte chunks, so ecc.size should be 512. This would make the special cases in omap_correct_bch() unnecessary. In dec7b4d2bf9 Matt said: | The fix is to pull over a bit of code from the kernel's | omap_correct_data() that sets eccsteps = 4 when the page size is 2048 | bytes and hardware ECC is being used. In fact, this piece is in the kernel code: /* Ex NAND_ECC_HW12_2048 */ if ((info->nand.ecc.mode == NAND_ECC_HW) && (info->nand.ecc.size == 2048)) blockCnt = 4; else blockCnt = 1; I just suspect this is never used, because ecc.size is correctly set to 512 in all cases. Then ecc.steps results in 4 for 2k page sizes and the framework correctly iterates over the ecc steps. Please give the attached test a try. It's completely untested. Sascha -------------------------------------8<--------------------------------- From 34bbcd911513e9031786a3c0f12e83ee9e904b42 Mon Sep 17 00:00:00 2001 From: Sascha Hauer <s.hauer@pengutronix.de> Date: Wed, 7 Jun 2017 08:42:06 +0200 Subject: [PATCH] mtd: nand_omap_gpmc: Fix ecc size The ECC size for BCH correction is always 512 byte. Correct the ecc.size for the OMAP_ECC_BCH8_CODE_HW mode from 2048 to 512. This change will let the framework iterate over the 4 ecc steps and we no longer need special cases in omap_correct_bch(). Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- drivers/mtd/nand/nand_omap_gpmc.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/drivers/mtd/nand/nand_omap_gpmc.c b/drivers/mtd/nand/nand_omap_gpmc.c index e18ce6358a..a35ed1f093 100644 --- a/drivers/mtd/nand/nand_omap_gpmc.c +++ b/drivers/mtd/nand/nand_omap_gpmc.c @@ -303,8 +303,7 @@ static int omap_correct_bch(struct mtd_info *mtd, uint8_t *dat, int bitflip_count; int bch_max_err; - int eccsteps = (nand->ecc.mode == NAND_ECC_HW) && - (nand->ecc.size == 2048) ? 4 : 1; + int eccsteps = nand->ecc.steps; int eccsize = oinfo->nand.ecc.bytes; switch (oinfo->ecc_mode) { @@ -765,8 +764,8 @@ static int omap_gpmc_eccmode(struct gpmc_nand_info *oinfo, offset - omap_oobinfo.eccbytes; break; case OMAP_ECC_BCH8_CODE_HW: - oinfo->nand.ecc.bytes = 13 * 4; - oinfo->nand.ecc.size = 512 * 4; + oinfo->nand.ecc.bytes = 13; + oinfo->nand.ecc.size = 512; oinfo->nand.ecc.strength = BCH8_MAX_ERROR; omap_oobinfo.oobfree->offset = offset; omap_oobinfo.oobfree->length = minfo->oobsize - -- 2.11.0 -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mtd: nand: omap: Fix BCH bit correction 2017-06-07 6:45 ` Sascha Hauer @ 2017-06-07 6:49 ` Sascha Hauer 2017-06-07 6:53 ` Sascha Hauer 0 siblings, 1 reply; 9+ messages in thread From: Sascha Hauer @ 2017-06-07 6:49 UTC (permalink / raw) To: Daniel Schultz; +Cc: barebox, Matt Reimer On Wed, Jun 07, 2017 at 08:45:08AM +0200, Sascha Hauer wrote: > +Cc Matt Reimer <mreimer@sdgsystems.com> > > On Tue, Jun 06, 2017 at 06:10:25PM +0200, Daniel Schultz wrote: > > After commit dec7b4d2bf9 was applied our barebox only corrected the > > first 512 Bytes of NAND pages. > > > > This patch separates between Hamming and BCH when finding out the > > eccsteps, because BCH always works with 2kB pages. > > > > Before this patch: > > > > barebox@Phytec phyCORE AM335x:/ nand_bitflip -r -n 5 /dev/nand0.barebox > > nand0.barebox: Flipping bit 5 @ 1796 > > nand0.barebox: Flipping bit 6 @ 1258 > > nand0.barebox: Flipping bit 5 @ 1062 > > nand0.barebox: Flipping bit 2 @ 1399 > > nand0.barebox: Flipping bit 6 @ 1243 > > No bitflips found on block 0, offset 0x00000000 > > barebox@Phytec phyCORE AM335x:/ nand_bitflip -r -n 5 /dev/nand0.barebox > > nand0.barebox: Flipping bit 2 @ 872 > > nand0.barebox: Flipping bit 4 @ 252 > > nand0.barebox: Flipping bit 3 @ 568 > > nand0.barebox: Flipping bit 2 @ 247 > > nand0.barebox: Flipping bit 5 @ 401 > > page at block 0, offset 0x00000000 has 3 bitflips > > > > After this patch: > > > > barebox@Phytec phyCORE AM335x:/ nand_bitflip -r -n 5 /dev/nand0.barebox > > nand0.barebox: Flipping bit 2 @ 1962 > > nand0.barebox: Flipping bit 0 @ 1563 > > nand0.barebox: Flipping bit 0 @ 1808 > > nand0.barebox: Flipping bit 6 @ 1460 > > nand0.barebox: Flipping bit 7 @ 2034 > > page at block 0, offset 0x00000000 has 5 bitflips > > barebox@Phytec phyCORE AM335x:/ nand_bitflip -r -n 5 /dev/nand0.barebox > > nand0.barebox: Flipping bit 1 @ 1352 > > nand0.barebox: Flipping bit 7 @ 1542 > > nand0.barebox: Flipping bit 2 @ 1021 > > nand0.barebox: Flipping bit 7 @ 691 > > nand0.barebox: Flipping bit 6 @ 1196 > > page at block 0, offset 0x00000000 has 10 bitflips, needs cleanup > > > > Signed-off-by: Daniel Schultz <d.schultz@phytec.de> > > --- > > drivers/mtd/nand/nand_omap_gpmc.c | 11 +++++++++-- > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/mtd/nand/nand_omap_gpmc.c b/drivers/mtd/nand/nand_omap_gpmc.c > > index 05c8486..61220da 100644 > > --- a/drivers/mtd/nand/nand_omap_gpmc.c > > +++ b/drivers/mtd/nand/nand_omap_gpmc.c > > @@ -302,10 +302,17 @@ static int omap_correct_bch(struct mtd_info *mtd, uint8_t *dat, > > unsigned int err_loc[8]; > > int bitflip_count; > > int bch_max_err; > > + int eccsteps; > > > > - int eccsteps = (nand->ecc.mode == NAND_ECC_HW) && > > - (nand->ecc.size == 2048) ? 4 : 1; > > int eccsize = oinfo->nand.ecc.bytes; > > + if (oinfo->ecc_mode == OMAP_ECC_HAMMING_CODE_HW_ROMCODE) > > This is wrong. When in Hamming ECC mode you shouldn't get into this > function. The test should always fail. > > > + if ((nand->ecc.mode == NAND_ECC_HW) && > > + (nand->ecc.size == 2048)) > > + eccsteps = 4; > > + else > > + eccsteps = 1; > > The question is why ecc.size is set to the wrong value in the first > place: > > case OMAP_ECC_BCH8_CODE_HW: > ... > oinfo->nand.ecc.size = 512 * 4; > > This seems to be wrong. The BCH controller works in 512 Byte chunks, so > ecc.size should be 512. This would make the special cases in > omap_correct_bch() unnecessary. > > In dec7b4d2bf9 Matt said: > > | The fix is to pull over a bit of code from the kernel's > | omap_correct_data() that sets eccsteps = 4 when the page size is 2048 > | bytes and hardware ECC is being used. > > In fact, this piece is in the kernel code: > > /* Ex NAND_ECC_HW12_2048 */ > if ((info->nand.ecc.mode == NAND_ECC_HW) && > (info->nand.ecc.size == 2048)) > blockCnt = 4; > else > blockCnt = 1; > > I just suspect this is never used, because ecc.size is correctly set to 512 in > all cases. Then ecc.steps results in 4 for 2k page sizes and the framework correctly > iterates over the ecc steps. > > Please give the attached test a try. It's completely untested. And can not work. Additionally eccsteps must be set to 1 in omap_correct_bch(). This effectively makes the loop in this function unnecessary which can then removed. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mtd: nand: omap: Fix BCH bit correction 2017-06-07 6:49 ` Sascha Hauer @ 2017-06-07 6:53 ` Sascha Hauer 2017-06-09 8:17 ` Daniel Schultz 0 siblings, 1 reply; 9+ messages in thread From: Sascha Hauer @ 2017-06-07 6:53 UTC (permalink / raw) To: Daniel Schultz; +Cc: barebox, Matt Reimer On Wed, Jun 07, 2017 at 08:49:09AM +0200, Sascha Hauer wrote: > On Wed, Jun 07, 2017 at 08:45:08AM +0200, Sascha Hauer wrote: > > +Cc Matt Reimer <mreimer@sdgsystems.com> > > > > On Tue, Jun 06, 2017 at 06:10:25PM +0200, Daniel Schultz wrote: > > > After commit dec7b4d2bf9 was applied our barebox only corrected the > > > first 512 Bytes of NAND pages. > > > > > > This patch separates between Hamming and BCH when finding out the > > > eccsteps, because BCH always works with 2kB pages. > > > > > > Before this patch: > > > > > > barebox@Phytec phyCORE AM335x:/ nand_bitflip -r -n 5 /dev/nand0.barebox > > > nand0.barebox: Flipping bit 5 @ 1796 > > > nand0.barebox: Flipping bit 6 @ 1258 > > > nand0.barebox: Flipping bit 5 @ 1062 > > > nand0.barebox: Flipping bit 2 @ 1399 > > > nand0.barebox: Flipping bit 6 @ 1243 > > > No bitflips found on block 0, offset 0x00000000 > > > barebox@Phytec phyCORE AM335x:/ nand_bitflip -r -n 5 /dev/nand0.barebox > > > nand0.barebox: Flipping bit 2 @ 872 > > > nand0.barebox: Flipping bit 4 @ 252 > > > nand0.barebox: Flipping bit 3 @ 568 > > > nand0.barebox: Flipping bit 2 @ 247 > > > nand0.barebox: Flipping bit 5 @ 401 > > > page at block 0, offset 0x00000000 has 3 bitflips > > > > > > After this patch: > > > > > > barebox@Phytec phyCORE AM335x:/ nand_bitflip -r -n 5 /dev/nand0.barebox > > > nand0.barebox: Flipping bit 2 @ 1962 > > > nand0.barebox: Flipping bit 0 @ 1563 > > > nand0.barebox: Flipping bit 0 @ 1808 > > > nand0.barebox: Flipping bit 6 @ 1460 > > > nand0.barebox: Flipping bit 7 @ 2034 > > > page at block 0, offset 0x00000000 has 5 bitflips > > > barebox@Phytec phyCORE AM335x:/ nand_bitflip -r -n 5 /dev/nand0.barebox > > > nand0.barebox: Flipping bit 1 @ 1352 > > > nand0.barebox: Flipping bit 7 @ 1542 > > > nand0.barebox: Flipping bit 2 @ 1021 > > > nand0.barebox: Flipping bit 7 @ 691 > > > nand0.barebox: Flipping bit 6 @ 1196 > > > page at block 0, offset 0x00000000 has 10 bitflips, needs cleanup > > > > > > Signed-off-by: Daniel Schultz <d.schultz@phytec.de> > > > --- > > > drivers/mtd/nand/nand_omap_gpmc.c | 11 +++++++++-- > > > 1 file changed, 9 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/mtd/nand/nand_omap_gpmc.c b/drivers/mtd/nand/nand_omap_gpmc.c > > > index 05c8486..61220da 100644 > > > --- a/drivers/mtd/nand/nand_omap_gpmc.c > > > +++ b/drivers/mtd/nand/nand_omap_gpmc.c > > > @@ -302,10 +302,17 @@ static int omap_correct_bch(struct mtd_info *mtd, uint8_t *dat, > > > unsigned int err_loc[8]; > > > int bitflip_count; > > > int bch_max_err; > > > + int eccsteps; > > > > > > - int eccsteps = (nand->ecc.mode == NAND_ECC_HW) && > > > - (nand->ecc.size == 2048) ? 4 : 1; > > > int eccsize = oinfo->nand.ecc.bytes; > > > + if (oinfo->ecc_mode == OMAP_ECC_HAMMING_CODE_HW_ROMCODE) > > > > This is wrong. When in Hamming ECC mode you shouldn't get into this > > function. The test should always fail. > > > > > + if ((nand->ecc.mode == NAND_ECC_HW) && > > > + (nand->ecc.size == 2048)) > > > + eccsteps = 4; > > > + else > > > + eccsteps = 1; > > > > The question is why ecc.size is set to the wrong value in the first > > place: > > > > case OMAP_ECC_BCH8_CODE_HW: > > ... > > oinfo->nand.ecc.size = 512 * 4; > > > > This seems to be wrong. The BCH controller works in 512 Byte chunks, so > > ecc.size should be 512. This would make the special cases in > > omap_correct_bch() unnecessary. > > > > In dec7b4d2bf9 Matt said: > > > > | The fix is to pull over a bit of code from the kernel's > > | omap_correct_data() that sets eccsteps = 4 when the page size is 2048 > > | bytes and hardware ECC is being used. > > > > In fact, this piece is in the kernel code: > > > > /* Ex NAND_ECC_HW12_2048 */ > > if ((info->nand.ecc.mode == NAND_ECC_HW) && > > (info->nand.ecc.size == 2048)) > > blockCnt = 4; > > else > > blockCnt = 1; > > > > I just suspect this is never used, because ecc.size is correctly set to 512 in > > all cases. Then ecc.steps results in 4 for 2k page sizes and the framework correctly > > iterates over the ecc steps. > > > > Please give the attached test a try. It's completely untested. > > And can not work. Additionally eccsteps must be set to 1 in > omap_correct_bch(). This effectively makes the loop in this function > unnecessary which can then removed. Which then means omap_gpmc_read_page_bch_rom_mode() has to iterate over ecc.steps itself, just like the other read_page implementations in the framework do. So long, enough of self-replying for now ;) Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mtd: nand: omap: Fix BCH bit correction 2017-06-07 6:53 ` Sascha Hauer @ 2017-06-09 8:17 ` Daniel Schultz 2017-06-09 9:08 ` Sascha Hauer 0 siblings, 1 reply; 9+ messages in thread From: Daniel Schultz @ 2017-06-09 8:17 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox, Matt Reimer Hi Sascha, Am 07.06.2017 um 08:53 schrieb Sascha Hauer: > On Wed, Jun 07, 2017 at 08:49:09AM +0200, Sascha Hauer wrote: >> On Wed, Jun 07, 2017 at 08:45:08AM +0200, Sascha Hauer wrote: >>> +Cc Matt Reimer <mreimer@sdgsystems.com> >>> >>> On Tue, Jun 06, 2017 at 06:10:25PM +0200, Daniel Schultz wrote: >>>> After commit dec7b4d2bf9 was applied our barebox only corrected the >>>> first 512 Bytes of NAND pages. >>>> >>>> This patch separates between Hamming and BCH when finding out the >>>> eccsteps, because BCH always works with 2kB pages. >>>> >>>> Before this patch: >>>> >>>> barebox@Phytec phyCORE AM335x:/ nand_bitflip -r -n 5 /dev/nand0.barebox >>>> nand0.barebox: Flipping bit 5 @ 1796 >>>> nand0.barebox: Flipping bit 6 @ 1258 >>>> nand0.barebox: Flipping bit 5 @ 1062 >>>> nand0.barebox: Flipping bit 2 @ 1399 >>>> nand0.barebox: Flipping bit 6 @ 1243 >>>> No bitflips found on block 0, offset 0x00000000 >>>> barebox@Phytec phyCORE AM335x:/ nand_bitflip -r -n 5 /dev/nand0.barebox >>>> nand0.barebox: Flipping bit 2 @ 872 >>>> nand0.barebox: Flipping bit 4 @ 252 >>>> nand0.barebox: Flipping bit 3 @ 568 >>>> nand0.barebox: Flipping bit 2 @ 247 >>>> nand0.barebox: Flipping bit 5 @ 401 >>>> page at block 0, offset 0x00000000 has 3 bitflips >>>> >>>> After this patch: >>>> >>>> barebox@Phytec phyCORE AM335x:/ nand_bitflip -r -n 5 /dev/nand0.barebox >>>> nand0.barebox: Flipping bit 2 @ 1962 >>>> nand0.barebox: Flipping bit 0 @ 1563 >>>> nand0.barebox: Flipping bit 0 @ 1808 >>>> nand0.barebox: Flipping bit 6 @ 1460 >>>> nand0.barebox: Flipping bit 7 @ 2034 >>>> page at block 0, offset 0x00000000 has 5 bitflips >>>> barebox@Phytec phyCORE AM335x:/ nand_bitflip -r -n 5 /dev/nand0.barebox >>>> nand0.barebox: Flipping bit 1 @ 1352 >>>> nand0.barebox: Flipping bit 7 @ 1542 >>>> nand0.barebox: Flipping bit 2 @ 1021 >>>> nand0.barebox: Flipping bit 7 @ 691 >>>> nand0.barebox: Flipping bit 6 @ 1196 >>>> page at block 0, offset 0x00000000 has 10 bitflips, needs cleanup >>>> >>>> Signed-off-by: Daniel Schultz <d.schultz@phytec.de> >>>> --- >>>> drivers/mtd/nand/nand_omap_gpmc.c | 11 +++++++++-- >>>> 1 file changed, 9 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/mtd/nand/nand_omap_gpmc.c b/drivers/mtd/nand/nand_omap_gpmc.c >>>> index 05c8486..61220da 100644 >>>> --- a/drivers/mtd/nand/nand_omap_gpmc.c >>>> +++ b/drivers/mtd/nand/nand_omap_gpmc.c >>>> @@ -302,10 +302,17 @@ static int omap_correct_bch(struct mtd_info *mtd, uint8_t *dat, >>>> unsigned int err_loc[8]; >>>> int bitflip_count; >>>> int bch_max_err; >>>> + int eccsteps; >>>> >>>> - int eccsteps = (nand->ecc.mode == NAND_ECC_HW) && >>>> - (nand->ecc.size == 2048) ? 4 : 1; >>>> int eccsize = oinfo->nand.ecc.bytes; >>>> + if (oinfo->ecc_mode == OMAP_ECC_HAMMING_CODE_HW_ROMCODE) >>> >>> This is wrong. When in Hamming ECC mode you shouldn't get into this >>> function. The test should always fail. That's why I added this test. I don't know why this change was made [1] >>> >>>> + if ((nand->ecc.mode == NAND_ECC_HW) && >>>> + (nand->ecc.size == 2048)) >>>> + eccsteps = 4; >>>> + else >>>> + eccsteps = 1; >>> >>> The question is why ecc.size is set to the wrong value in the first >>> place: >>> >>> case OMAP_ECC_BCH8_CODE_HW: >>> ... >>> oinfo->nand.ecc.size = 512 * 4; >>> >>> This seems to be wrong. The BCH controller works in 512 Byte chunks, so >>> ecc.size should be 512. This would make the special cases in >>> omap_correct_bch() unnecessary. Only OMAP_ECC_BCH8_CODE_HW_ROMCODE can call omap_gpmc_read_page_bch_rom_mode(). So, this should be no problem, but this multiplying is not in the kernel. Maybe this can affect older systems (OMAP_ECC_BCH8_CODE_HW is only used by old phycards). >>> >>> In dec7b4d2bf9 Matt said: >>> >>> | The fix is to pull over a bit of code from the kernel's >>> | omap_correct_data() that sets eccsteps = 4 when the page size is 2048 >>> | bytes and hardware ECC is being used. >>> >>> In fact, this piece is in the kernel code: >>> >>> /* Ex NAND_ECC_HW12_2048 */ >>> if ((info->nand.ecc.mode == NAND_ECC_HW) && >>> (info->nand.ecc.size == 2048)) >>> blockCnt = 4; >>> else >>> blockCnt = 1; [1] since this is from the Hamming logic and not BCH. This is a snippet from the linux-ti kernel: case OMAP_ECC_HAM1_CODE_HW: pr_info("nand: using OMAP_ECC_HAM1_CODE_HW\n"); nand_chip->ecc.mode = NAND_ECC_HW; nand_chip->ecc.bytes = 3; nand_chip->ecc.size = 512; nand_chip->ecc.strength = 1; nand_chip->ecc.calculate = omap_calculate_ecc; nand_chip->ecc.hwctl = omap_enable_hwecc; nand_chip->ecc.correct = omap_correct_data; /* define ECC layout */ ecclayout->eccbytes = nand_chip->ecc.bytes ... case OMAP_ECC_BCH8_CODE_HW: nand_chip->ecc.mode = NAND_ECC_HW; nand_chip->ecc.size = 512; nand_chip->ecc.bytes = 13 + 1; nand_chip->ecc.strength = 8; nand_chip->ecc.hwctl = omap_enable_hwecc_bch; nand_chip->ecc.correct = omap_elm_correct_data; nand_chip->ecc.calculate = omap_calculate_ecc_bch; nand_chip->ecc.read_page = omap_read_page_bch; nand_chip->ecc.write_page = omap_write_page_bch; >>> >>> I just suspect this is never used, because ecc.size is correctly set to 512 in >>> all cases. Then ecc.steps results in 4 for 2k page sizes and the framework correctly >>> iterates over the ecc steps. >>> >>> Please give the attached test a try. It's completely untested. >> >> And can not work. Additionally eccsteps must be set to 1 in >> omap_correct_bch(). This effectively makes the loop in this function >> unnecessary which can then removed. > > Which then means omap_gpmc_read_page_bch_rom_mode() has to iterate over > ecc.steps itself, just like the other read_page implementations in the > framework do. > So, the previous assignment of eccsteps was fine? > So long, enough of self-replying for now ;) > > Sascha > -- Mit freundlichen Grüßen, With best regards, Daniel Schultz _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mtd: nand: omap: Fix BCH bit correction 2017-06-09 8:17 ` Daniel Schultz @ 2017-06-09 9:08 ` Sascha Hauer 2017-06-09 13:28 ` Daniel Schultz 0 siblings, 1 reply; 9+ messages in thread From: Sascha Hauer @ 2017-06-09 9:08 UTC (permalink / raw) To: Daniel Schultz; +Cc: barebox, Matt Reimer On Fri, Jun 09, 2017 at 10:17:55AM +0200, Daniel Schultz wrote: > Hi Sascha, > > > > > > > And can not work. Additionally eccsteps must be set to 1 in > > > omap_correct_bch(). This effectively makes the loop in this function > > > unnecessary which can then removed. > > > > Which then means omap_gpmc_read_page_bch_rom_mode() has to iterate over > > ecc.steps itself, just like the other read_page implementations in the > > framework do. > > > So, the previous assignment of eccsteps was fine? I just sent an updated patch(-series). Could you give it a try? Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mtd: nand: omap: Fix BCH bit correction 2017-06-09 9:08 ` Sascha Hauer @ 2017-06-09 13:28 ` Daniel Schultz 2017-06-12 13:41 ` Sascha Hauer 0 siblings, 1 reply; 9+ messages in thread From: Daniel Schultz @ 2017-06-09 13:28 UTC (permalink / raw) To: barebox, Sascha Hauer Hi, Am 09.06.2017 um 11:08 schrieb Sascha Hauer: > On Fri, Jun 09, 2017 at 10:17:55AM +0200, Daniel Schultz wrote: >> Hi Sascha, >> >>>> >>>> And can not work. Additionally eccsteps must be set to 1 in >>>> omap_correct_bch(). This effectively makes the loop in this function >>>> unnecessary which can then removed. >>> >>> Which then means omap_gpmc_read_page_bch_rom_mode() has to iterate over >>> ecc.steps itself, just like the other read_page implementations in the >>> framework do. >>> >> So, the previous assignment of eccsteps was fine? > > I just sent an updated patch(-series). Could you give it a try? > It works, but the current version only changes the local copy of the pointer. As a result of that it will only check the first 512 Bytes. I appended a double pointer workaround for this problem :) omap_correct_data() also calls omap_correct_bch(). Does Barebox correct NAND partitions? I have never seen this. Maybe we need here also a loop. From 2b104598933b00cd33a85333ce72a49de7230507 Mon Sep 17 00:00:00 2001 From: Daniel Schultz <d.schultz@phytec.de> Date: Fri, 9 Jun 2017 15:15:30 +0200 Subject: [PATCH] Add double pointer to current OMAP NAND ECC patch stack Signed-off-by: Daniel Schultz <d.schultz@phytec.de> --- drivers/mtd/nand/nand_omap_gpmc.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/drivers/mtd/nand/nand_omap_gpmc.c b/drivers/mtd/nand/nand_omap_gpmc.c index 334014a..7608545 100644 --- a/drivers/mtd/nand/nand_omap_gpmc.c +++ b/drivers/mtd/nand/nand_omap_gpmc.c @@ -292,8 +292,8 @@ static int omap_calculate_ecc(struct mtd_info *mtd, const uint8_t *dat, return __omap_calculate_ecc(mtd, dat, ecc_code, 0); } -static int omap_correct_bch(struct mtd_info *mtd, uint8_t *dat, - uint8_t *read_ecc, uint8_t *calc_ecc) +static int omap_correct_bch(struct mtd_info *mtd, uint8_t **dat, + uint8_t **read_ecc, uint8_t **calc_ecc) { struct nand_chip *nand = (struct nand_chip *)(mtd->priv); struct gpmc_nand_info *oinfo = (struct gpmc_nand_info *)(nand->priv); @@ -328,14 +328,14 @@ static int omap_correct_bch(struct mtd_info *mtd, uint8_t *dat, /* check for any ecc error */ for (j = 0; (j < actual_eccsize) && (eccflag == 0); j++) { - if (calc_ecc[j] != 0) { + if ((*calc_ecc)[j] != 0) { eccflag = 1; break; } } if (eccflag == 1) { - if (memcmp(calc_ecc, erased_ecc_vec, actual_eccsize) == 0) { + if (memcmp(*calc_ecc, erased_ecc_vec, actual_eccsize) == 0) { /* * calc_ecc[] matches pattern for ECC * (all 0xff) so this is definitely @@ -343,7 +343,7 @@ static int omap_correct_bch(struct mtd_info *mtd, uint8_t *dat, */ } else { bitflip_count = nand_check_erased_ecc_chunk( - dat, oinfo->nand.ecc.size, read_ecc, + *dat, oinfo->nand.ecc.size, *read_ecc, eccsize, NULL, 0, bch_max_err); if (bitflip_count < 0) is_error_reported = true; @@ -352,22 +352,22 @@ static int omap_correct_bch(struct mtd_info *mtd, uint8_t *dat, if (is_error_reported) { bitflip_count = omap_gpmc_decode_bch(1, - calc_ecc, err_loc); + *calc_ecc, err_loc); if (bitflip_count < 0) return bitflip_count; for (j = 0; j < bitflip_count; j++) { if (err_loc[j] < 4096) - dat[err_loc[j] >> 3] ^= + (*dat)[err_loc[j] >> 3] ^= 1 << (err_loc[j] & 7); /* else, not interested to correct ecc */ } } totalcount += bitflip_count; - calc_ecc = calc_ecc + actual_eccsize; - read_ecc = read_ecc + eccsize; - dat += 512; + *calc_ecc += actual_eccsize; + *read_ecc += eccsize; + *dat += 512; return totalcount; } @@ -449,7 +449,7 @@ static int omap_correct_data(struct mtd_info *mtd, uint8_t *dat, * this time with oob data. */ __omap_calculate_ecc(mtd, dat, calc_ecc, 0); - return omap_correct_bch(mtd, dat, read_ecc, calc_ecc); + return omap_correct_bch(mtd, &dat, &read_ecc, &calc_ecc); default: return -EINVAL; } @@ -705,7 +705,7 @@ static int omap_gpmc_read_page_bch_rom_mode(struct mtd_info *mtd, __omap_calculate_ecc(mtd, buf, ecc_calc, 1); for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) { - stat = omap_correct_bch(mtd, buf, ecc_code, ecc_calc); + stat = omap_correct_bch(mtd, &buf, &ecc_code, &ecc_calc); if (stat < 0) { mtd->ecc_stats.failed++; } else { -- 1.9.1 _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mtd: nand: omap: Fix BCH bit correction 2017-06-09 13:28 ` Daniel Schultz @ 2017-06-12 13:41 ` Sascha Hauer 2017-06-13 11:50 ` Daniel Schultz 0 siblings, 1 reply; 9+ messages in thread From: Sascha Hauer @ 2017-06-12 13:41 UTC (permalink / raw) To: Daniel Schultz; +Cc: barebox Hi Daniel, On Fri, Jun 09, 2017 at 03:28:59PM +0200, Daniel Schultz wrote: > Hi, > > Am 09.06.2017 um 11:08 schrieb Sascha Hauer: > > On Fri, Jun 09, 2017 at 10:17:55AM +0200, Daniel Schultz wrote: > > > Hi Sascha, > > > > > > > > > > > > > And can not work. Additionally eccsteps must be set to 1 in > > > > > omap_correct_bch(). This effectively makes the loop in this function > > > > > unnecessary which can then removed. > > > > > > > > Which then means omap_gpmc_read_page_bch_rom_mode() has to iterate over > > > > ecc.steps itself, just like the other read_page implementations in the > > > > framework do. > > > > > > > So, the previous assignment of eccsteps was fine? > > > > I just sent an updated patch(-series). Could you give it a try? > > > It works, but the current version only changes the local copy of the > pointer. As a result of that it will only check the first 512 Bytes. > I appended a double pointer workaround for this problem :) The pointer should be incremented by the caller, not by omap_correct_bch(). > > > omap_correct_data() also calls omap_correct_bch(). Does Barebox correct NAND > partitions? I have never seen this. Maybe we need here also a loop. The core already loops around eccsteps when calling ecc.correct. I digged a bit further and this is what I can come up with. Anyway, I am getting less and less confident that the patch can work. Please give it a try and see if it works. If it doesn't and we can't find out what's wrong I tend to take your original patch, although I still think this is the wrong solution. Sascha --------------------8<------------------------------ From 2c65a009dbcf2136e037f009b50306aa080e2920 Mon Sep 17 00:00:00 2001 From: Sascha Hauer <s.hauer@pengutronix.de> Date: Fri, 9 Jun 2017 10:45:21 +0200 Subject: [PATCH] mtd: nand_omap_gpmc: Fix ecc size The ECC size for BCH correction is always 512 byte. Correct the ecc.size for the OMAP_ECC_BCH8_CODE_HW mode from 2048 to 512. This change will let the framework iterate over the 4 ecc steps and we no longer need special cases in omap_correct_bch(). Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> --- drivers/mtd/nand/nand_omap_gpmc.c | 130 +++++++++++++++++--------------------- 1 file changed, 57 insertions(+), 73 deletions(-) diff --git a/drivers/mtd/nand/nand_omap_gpmc.c b/drivers/mtd/nand/nand_omap_gpmc.c index e18ce6358a..2e130bfd9a 100644 --- a/drivers/mtd/nand/nand_omap_gpmc.c +++ b/drivers/mtd/nand/nand_omap_gpmc.c @@ -297,85 +297,59 @@ static int omap_correct_bch(struct mtd_info *mtd, uint8_t *dat, { struct nand_chip *nand = (struct nand_chip *)(mtd->priv); struct gpmc_nand_info *oinfo = (struct gpmc_nand_info *)(nand->priv); - int i, j, eccflag, totalcount, actual_eccsize; + int j, eccflag; const uint8_t *erased_ecc_vec; unsigned int err_loc[8]; int bitflip_count; int bch_max_err; - - int eccsteps = (nand->ecc.mode == NAND_ECC_HW) && - (nand->ecc.size == 2048) ? 4 : 1; int eccsize = oinfo->nand.ecc.bytes; + bool is_error_reported = false; - switch (oinfo->ecc_mode) { - case OMAP_ECC_BCH8_CODE_HW: - eccsize /= eccsteps; - actual_eccsize = eccsize; - erased_ecc_vec = bch8_vector; - bch_max_err = BCH8_MAX_ERROR; - break; - case OMAP_ECC_BCH8_CODE_HW_ROMCODE: - actual_eccsize = eccsize - 1; - erased_ecc_vec = bch8_vector; - bch_max_err = BCH8_MAX_ERROR; - break; - default: - dev_err(oinfo->pdev, "invalid driver configuration\n"); - return -EINVAL; - } - - totalcount = 0; + erased_ecc_vec = bch8_vector; + bch_max_err = BCH8_MAX_ERROR; - for (i = 0; i < eccsteps; i++) { - bool is_error_reported = false; - bitflip_count = 0; - eccflag = 0; + bitflip_count = 0; + eccflag = 0; - /* check for any ecc error */ - for (j = 0; (j < actual_eccsize) && (eccflag == 0); j++) { - if (calc_ecc[j] != 0) { - eccflag = 1; - break; - } - } - - if (eccflag == 1) { - if (memcmp(calc_ecc, erased_ecc_vec, actual_eccsize) == 0) { - /* - * calc_ecc[] matches pattern for ECC - * (all 0xff) so this is definitely - * an erased-page - */ - } else { - bitflip_count = nand_check_erased_ecc_chunk( - dat, oinfo->nand.ecc.size, read_ecc, - eccsize, NULL, 0, bch_max_err); - if (bitflip_count < 0) - is_error_reported = true; - } + /* check for any ecc error */ + for (j = 0; (j < eccsize) && (eccflag == 0); j++) { + if (calc_ecc[j] != 0) { + eccflag = 1; + break; } + } - if (is_error_reported) { - bitflip_count = omap_gpmc_decode_bch(1, - calc_ecc, err_loc); + if (eccflag == 1) { + if (memcmp(calc_ecc, erased_ecc_vec, eccsize) == 0) { + /* + * calc_ecc[] matches pattern for ECC + * (all 0xff) so this is definitely + * an erased-page + */ + } else { + bitflip_count = nand_check_erased_ecc_chunk( + dat, oinfo->nand.ecc.size, read_ecc, + eccsize, NULL, 0, bch_max_err); if (bitflip_count < 0) - return bitflip_count; - - for (j = 0; j < bitflip_count; j++) { - if (err_loc[j] < 4096) - dat[err_loc[j] >> 3] ^= - 1 << (err_loc[j] & 7); - /* else, not interested to correct ecc */ - } + is_error_reported = true; } + } - totalcount += bitflip_count; - calc_ecc = calc_ecc + actual_eccsize; - read_ecc = read_ecc + eccsize; - dat += 512; + if (is_error_reported) { + bitflip_count = omap_gpmc_decode_bch(1, + calc_ecc, err_loc); + if (bitflip_count < 0) + return bitflip_count; + + for (j = 0; j < bitflip_count; j++) { + if (err_loc[j] < 4096) + dat[err_loc[j] >> 3] ^= + 1 << (err_loc[j] & 7); + /* else, not interested to correct ecc */ + } } - return totalcount; + return bitflip_count; } static int omap_correct_hamming(struct mtd_info *mtd, uint8_t *dat, @@ -667,6 +641,10 @@ static int omap_gpmc_read_page_bch_rom_mode(struct mtd_info *mtd, uint8_t *ecc_code = chip->buffers->ecccode; uint32_t *eccpos = chip->ecc.layout->eccpos; int stat, i; + unsigned int max_bitflips = 0; + int eccsize = chip->ecc.size; + int eccbytes = chip->ecc.bytes; + int eccsteps = chip->ecc.steps; writel(GPMC_ECC_SIZE_CONFIG_ECCSIZE1(0) | GPMC_ECC_SIZE_CONFIG_ECCSIZE0(64), @@ -706,13 +684,19 @@ static int omap_gpmc_read_page_bch_rom_mode(struct mtd_info *mtd, __omap_calculate_ecc(mtd, buf, ecc_calc, 1); - stat = omap_correct_bch(mtd, buf, ecc_code, ecc_calc); - if (stat < 0) - mtd->ecc_stats.failed++; - else - mtd->ecc_stats.corrected += stat; + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) { + stat = omap_correct_bch(mtd, p, ecc_code, ecc_calc); + ecc_code += eccsize + 1; + ecc_calc += eccsize; + if (stat < 0) { + mtd->ecc_stats.failed++; + } else { + mtd->ecc_stats.corrected += stat; + max_bitflips = max_t(unsigned int, max_bitflips, stat); + } + } - return 0; + return max_bitflips; } static int omap_gpmc_eccmode(struct gpmc_nand_info *oinfo, @@ -765,8 +749,8 @@ static int omap_gpmc_eccmode(struct gpmc_nand_info *oinfo, offset - omap_oobinfo.eccbytes; break; case OMAP_ECC_BCH8_CODE_HW: - oinfo->nand.ecc.bytes = 13 * 4; - oinfo->nand.ecc.size = 512 * 4; + oinfo->nand.ecc.bytes = 13; + oinfo->nand.ecc.size = 512; oinfo->nand.ecc.strength = BCH8_MAX_ERROR; omap_oobinfo.oobfree->offset = offset; omap_oobinfo.oobfree->length = minfo->oobsize - @@ -776,7 +760,7 @@ static int omap_gpmc_eccmode(struct gpmc_nand_info *oinfo, omap_oobinfo.eccpos[i] = i + offset; break; case OMAP_ECC_BCH8_CODE_HW_ROMCODE: - oinfo->nand.ecc.bytes = 13 + 1; + oinfo->nand.ecc.bytes = 13; oinfo->nand.ecc.size = 512; oinfo->nand.ecc.strength = BCH8_MAX_ERROR; nand->ecc.read_page = omap_gpmc_read_page_bch_rom_mode; -- 2.11.0 -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] mtd: nand: omap: Fix BCH bit correction 2017-06-12 13:41 ` Sascha Hauer @ 2017-06-13 11:50 ` Daniel Schultz 0 siblings, 0 replies; 9+ messages in thread From: Daniel Schultz @ 2017-06-13 11:50 UTC (permalink / raw) To: Sascha Hauer; +Cc: barebox Hi Sascha, Am 12.06.2017 um 15:41 schrieb Sascha Hauer: > Hi Daniel, > > On Fri, Jun 09, 2017 at 03:28:59PM +0200, Daniel Schultz wrote: >> Hi, >> >> Am 09.06.2017 um 11:08 schrieb Sascha Hauer: >>> On Fri, Jun 09, 2017 at 10:17:55AM +0200, Daniel Schultz wrote: >>>> Hi Sascha, >>>> >>>>>> >>>>>> And can not work. Additionally eccsteps must be set to 1 in >>>>>> omap_correct_bch(). This effectively makes the loop in this function >>>>>> unnecessary which can then removed. >>>>> >>>>> Which then means omap_gpmc_read_page_bch_rom_mode() has to iterate over >>>>> ecc.steps itself, just like the other read_page implementations in the >>>>> framework do. >>>>> >>>> So, the previous assignment of eccsteps was fine? >>> >>> I just sent an updated patch(-series). Could you give it a try? >>> >> It works, but the current version only changes the local copy of the >> pointer. As a result of that it will only check the first 512 Bytes. >> I appended a double pointer workaround for this problem :) > > The pointer should be incremented by the caller, not by > omap_correct_bch(). > >> >> >> omap_correct_data() also calls omap_correct_bch(). Does Barebox correct NAND >> partitions? I have never seen this. Maybe we need here also a loop. > > The core already loops around eccsteps when calling ecc.correct. > > I digged a bit further and this is what I can come up with. Anyway, I am > getting less and less confident that the patch can work. > > Please give it a try and see if it works. If it doesn't and we can't > find out what's wrong I tend to take your original patch, although I > still think this is the wrong solution. > With these changes it works, but I only testes with nand_bitflip. Idk if there're better ways to test NAND ECC. However, after 6 bitflips on one subpage it will successfully boot in the backup partition, with less than 6 it will boot the bitflipped-partition. Daniel diff --git a/drivers/mtd/nand/nand_omap_gpmc.c b/drivers/mtd/nand/nand_omap_gpmc.c index 2e130bf..9006e2e 100644 --- a/drivers/mtd/nand/nand_omap_gpmc.c +++ b/drivers/mtd/nand/nand_omap_gpmc.c @@ -684,10 +684,10 @@ static int omap_gpmc_read_page_bch_rom_mode(struct mtd_info *mtd, __omap_calculate_ecc(mtd, buf, ecc_calc, 1); - for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) { - stat = omap_correct_bch(mtd, p, ecc_code, ecc_calc); - ecc_code += eccsize + 1; - ecc_calc += eccsize; + for (i = 0; eccsteps; eccsteps--, i += eccbytes, buf += eccsize) { + stat = omap_correct_bch(mtd, buf, ecc_code, ecc_calc); + ecc_code += eccbytes + 1; + ecc_calc += eccbytes; if (stat < 0) { mtd->ecc_stats.failed++; } else { > Sascha > > --------------------8<------------------------------ > > From 2c65a009dbcf2136e037f009b50306aa080e2920 Mon Sep 17 00:00:00 2001 > From: Sascha Hauer <s.hauer@pengutronix.de> > Date: Fri, 9 Jun 2017 10:45:21 +0200 > Subject: [PATCH] mtd: nand_omap_gpmc: Fix ecc size > > The ECC size for BCH correction is always 512 byte. Correct the ecc.size > for the OMAP_ECC_BCH8_CODE_HW mode from 2048 to 512. This change will > let the framework iterate over the 4 ecc steps and we no longer need > special cases in omap_correct_bch(). > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de> > --- > drivers/mtd/nand/nand_omap_gpmc.c | 130 +++++++++++++++++--------------------- > 1 file changed, 57 insertions(+), 73 deletions(-) > > diff --git a/drivers/mtd/nand/nand_omap_gpmc.c b/drivers/mtd/nand/nand_omap_gpmc.c > index e18ce6358a..2e130bfd9a 100644 > --- a/drivers/mtd/nand/nand_omap_gpmc.c > +++ b/drivers/mtd/nand/nand_omap_gpmc.c > @@ -297,85 +297,59 @@ static int omap_correct_bch(struct mtd_info *mtd, uint8_t *dat, > { > struct nand_chip *nand = (struct nand_chip *)(mtd->priv); > struct gpmc_nand_info *oinfo = (struct gpmc_nand_info *)(nand->priv); > - int i, j, eccflag, totalcount, actual_eccsize; > + int j, eccflag; > const uint8_t *erased_ecc_vec; > unsigned int err_loc[8]; > int bitflip_count; > int bch_max_err; > - > - int eccsteps = (nand->ecc.mode == NAND_ECC_HW) && > - (nand->ecc.size == 2048) ? 4 : 1; > int eccsize = oinfo->nand.ecc.bytes; > + bool is_error_reported = false; > > - switch (oinfo->ecc_mode) { > - case OMAP_ECC_BCH8_CODE_HW: > - eccsize /= eccsteps; > - actual_eccsize = eccsize; > - erased_ecc_vec = bch8_vector; > - bch_max_err = BCH8_MAX_ERROR; > - break; > - case OMAP_ECC_BCH8_CODE_HW_ROMCODE: > - actual_eccsize = eccsize - 1; > - erased_ecc_vec = bch8_vector; > - bch_max_err = BCH8_MAX_ERROR; > - break; > - default: > - dev_err(oinfo->pdev, "invalid driver configuration\n"); > - return -EINVAL; > - } > - > - totalcount = 0; > + erased_ecc_vec = bch8_vector; > + bch_max_err = BCH8_MAX_ERROR; > > - for (i = 0; i < eccsteps; i++) { > - bool is_error_reported = false; > - bitflip_count = 0; > - eccflag = 0; > + bitflip_count = 0; > + eccflag = 0; > > - /* check for any ecc error */ > - for (j = 0; (j < actual_eccsize) && (eccflag == 0); j++) { > - if (calc_ecc[j] != 0) { > - eccflag = 1; > - break; > - } > - } > - > - if (eccflag == 1) { > - if (memcmp(calc_ecc, erased_ecc_vec, actual_eccsize) == 0) { > - /* > - * calc_ecc[] matches pattern for ECC > - * (all 0xff) so this is definitely > - * an erased-page > - */ > - } else { > - bitflip_count = nand_check_erased_ecc_chunk( > - dat, oinfo->nand.ecc.size, read_ecc, > - eccsize, NULL, 0, bch_max_err); > - if (bitflip_count < 0) > - is_error_reported = true; > - } > + /* check for any ecc error */ > + for (j = 0; (j < eccsize) && (eccflag == 0); j++) { > + if (calc_ecc[j] != 0) { > + eccflag = 1; > + break; > } > + } > > - if (is_error_reported) { > - bitflip_count = omap_gpmc_decode_bch(1, > - calc_ecc, err_loc); > + if (eccflag == 1) { > + if (memcmp(calc_ecc, erased_ecc_vec, eccsize) == 0) { > + /* > + * calc_ecc[] matches pattern for ECC > + * (all 0xff) so this is definitely > + * an erased-page > + */ > + } else { > + bitflip_count = nand_check_erased_ecc_chunk( > + dat, oinfo->nand.ecc.size, read_ecc, > + eccsize, NULL, 0, bch_max_err); > if (bitflip_count < 0) > - return bitflip_count; > - > - for (j = 0; j < bitflip_count; j++) { > - if (err_loc[j] < 4096) > - dat[err_loc[j] >> 3] ^= > - 1 << (err_loc[j] & 7); > - /* else, not interested to correct ecc */ > - } > + is_error_reported = true; > } > + } > > - totalcount += bitflip_count; > - calc_ecc = calc_ecc + actual_eccsize; > - read_ecc = read_ecc + eccsize; > - dat += 512; > + if (is_error_reported) { > + bitflip_count = omap_gpmc_decode_bch(1, > + calc_ecc, err_loc); > + if (bitflip_count < 0) > + return bitflip_count; > + > + for (j = 0; j < bitflip_count; j++) { > + if (err_loc[j] < 4096) > + dat[err_loc[j] >> 3] ^= > + 1 << (err_loc[j] & 7); > + /* else, not interested to correct ecc */ > + } > } > > - return totalcount; > + return bitflip_count; > } > > static int omap_correct_hamming(struct mtd_info *mtd, uint8_t *dat, > @@ -667,6 +641,10 @@ static int omap_gpmc_read_page_bch_rom_mode(struct mtd_info *mtd, > uint8_t *ecc_code = chip->buffers->ecccode; > uint32_t *eccpos = chip->ecc.layout->eccpos; > int stat, i; > + unsigned int max_bitflips = 0; > + int eccsize = chip->ecc.size; > + int eccbytes = chip->ecc.bytes; > + int eccsteps = chip->ecc.steps; > > writel(GPMC_ECC_SIZE_CONFIG_ECCSIZE1(0) | > GPMC_ECC_SIZE_CONFIG_ECCSIZE0(64), > @@ -706,13 +684,19 @@ static int omap_gpmc_read_page_bch_rom_mode(struct mtd_info *mtd, > > __omap_calculate_ecc(mtd, buf, ecc_calc, 1); > > - stat = omap_correct_bch(mtd, buf, ecc_code, ecc_calc); > - if (stat < 0) > - mtd->ecc_stats.failed++; > - else > - mtd->ecc_stats.corrected += stat; > + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) { > + stat = omap_correct_bch(mtd, p, ecc_code, ecc_calc); > + ecc_code += eccsize + 1; > + ecc_calc += eccsize; > + if (stat < 0) { > + mtd->ecc_stats.failed++; > + } else { > + mtd->ecc_stats.corrected += stat; > + max_bitflips = max_t(unsigned int, max_bitflips, stat); > + } > + } > > - return 0; > + return max_bitflips; > } > > static int omap_gpmc_eccmode(struct gpmc_nand_info *oinfo, > @@ -765,8 +749,8 @@ static int omap_gpmc_eccmode(struct gpmc_nand_info *oinfo, > offset - omap_oobinfo.eccbytes; > break; > case OMAP_ECC_BCH8_CODE_HW: > - oinfo->nand.ecc.bytes = 13 * 4; > - oinfo->nand.ecc.size = 512 * 4; > + oinfo->nand.ecc.bytes = 13; > + oinfo->nand.ecc.size = 512; > oinfo->nand.ecc.strength = BCH8_MAX_ERROR; > omap_oobinfo.oobfree->offset = offset; > omap_oobinfo.oobfree->length = minfo->oobsize - > @@ -776,7 +760,7 @@ static int omap_gpmc_eccmode(struct gpmc_nand_info *oinfo, > omap_oobinfo.eccpos[i] = i + offset; > break; > case OMAP_ECC_BCH8_CODE_HW_ROMCODE: > - oinfo->nand.ecc.bytes = 13 + 1; > + oinfo->nand.ecc.bytes = 13; > oinfo->nand.ecc.size = 512; > oinfo->nand.ecc.strength = BCH8_MAX_ERROR; > nand->ecc.read_page = omap_gpmc_read_page_bch_rom_mode; > -- Mit freundlichen Grüßen, With best regards, Daniel Schultz _______________________________________________ barebox mailing list barebox@lists.infradead.org http://lists.infradead.org/mailman/listinfo/barebox ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-06-13 11:50 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-06-06 16:10 [PATCH] mtd: nand: omap: Fix BCH bit correction Daniel Schultz 2017-06-07 6:45 ` Sascha Hauer 2017-06-07 6:49 ` Sascha Hauer 2017-06-07 6:53 ` Sascha Hauer 2017-06-09 8:17 ` Daniel Schultz 2017-06-09 9:08 ` Sascha Hauer 2017-06-09 13:28 ` Daniel Schultz 2017-06-12 13:41 ` Sascha Hauer 2017-06-13 11:50 ` Daniel Schultz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox