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

[Daniel Schultz <d.schultz@phytec.de>]

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