[PATCH] mtd: nand: omap: Fix BCH bit correction
Daniel Schultz
d.schultz at phytec.de
Tue Jun 13 04:50:13 PDT 2017
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 at 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 at 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
More information about the barebox
mailing list