Re: [PATCH] mtd: nand: omap: Fix BCH bit correction

[Sascha Hauer <s.hauer@pengutronix.de>]

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