Missing support for ECC_SOFT_BCH in fsl-elbc-nand

[Tomas Hlavacek]

Hi!

On Friday, March 27, 2015 3:13:09 PM CEST, Martin Strbačka wrote:
> in our product we have Freescale P2020 SoC together with Micron
> MT29F2G08ABAEAWP NAND. Lately we discovered that the internal driver
> (fsl-elbc-nand) supports only 1-bit HW ECC.

Actually we use the NAND pretty intensively with JFFS2 and we have seen 
some uncorrectable multiple bitflips with the current HW ECC mode (it means 
more than one bit flip in 512B subpage, if I got that right). So we would 
like to change to software BCH ECC since we have powerful enough CPU.

I would like to discuss two major questions before I submit a patch for 
this:

1) How to disable HW ECC?

What I have done is this:

diff --git a/drivers/mtd/nand/fsl_elbc_nand.c 
b/drivers/mtd/nand/fsl_elbc_nand.c
index 04b22fd..e5818ba 100644
--- a/drivers/mtd/nand/fsl_elbc_nand.c
+++ b/drivers/mtd/nand/fsl_elbc_nand.c

@@ -676,6 +691,7 @@ static int fsl_elbc_chip_init_tail(struct mtd_info 
*mtd)
        } else if (mtd->writesize == 2048) {
                priv->page_size = 1;
                setbits32(&lbc->bank[priv->bank].or, OR_FCM_PGS);
+#ifndef ELBC_ECC_SW_BCH
                /* adjust ecc setup if needed */
                if ((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
                    BR_DECC_CHK_GEN) {
@@ -684,6 +700,7 @@ static int fsl_elbc_chip_init_tail(struct mtd_info 
*mtd)
                                           &fsl_elbc_oob_lp_eccm1 :
                                           &fsl_elbc_oob_lp_eccm0;
                }
+#endif
        } else {
                dev_err(priv->dev,
                        "fsl_elbc_init: page size %d is not supported\n",
@@ -774,11 +791,18 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd 
*priv)
 
        chip->ecc.read_page = fsl_elbc_read_page;
        chip->ecc.write_page = fsl_elbc_write_page;
-       chip->ecc.write_subpage = fsl_elbc_write_subpage;
 
+#ifdef ELBC_ECC_SW_BCH
+       
out_be32(&lbc->bank[priv->bank].br,(in_be32(&lbc->bank[priv->bank].br) & 
(~BR_DECC)));
+       chip->ecc.mode = NAND_ECC_SOFT_BCH;
+       chip->ecc.size = 512;
+       chip->ecc.bytes = 13;
+       chip->ecc.strength = 8;
+#else
        /* If CS Base Register selects full hardware ECC then use it */
        if ((in_be32(&lbc->bank[priv->bank].br) & BR_DECC) ==
            BR_DECC_CHK_GEN) {
+               chip->ecc.write_subpage = fsl_elbc_write_subpage;
                chip->ecc.mode = NAND_ECC_HW;
                /* put in small page settings and adjust later if needed */
                chip->ecc.layout = (priv->fmr & FMR_ECCM) ?
@@ -790,6 +814,7 @@ static int fsl_elbc_chip_init(struct fsl_elbc_mtd 
*priv)
                /* otherwise fall back to default software ECC */
                chip->ecc.mode = NAND_ECC_SOFT;
        }
+#endif
 
        return 0;
 }


It works but I am particularly not sure of line

out_be32(&lbc->bank[priv->bank].br,(in_be32(&lbc->bank[priv->bank].br) & 
(~BR_DECC)));

Does it make sense? I have been told that on our board there is some 
unconnected pin that causes that (in_be32(&lbc->bank[priv->bank].br) & 
BR_DECC) == BR_DECC_CHK_GEN is true but I still have to disable HW ECC.

Does it make sense to add Kconf switch "force SW ECC" which would define 
my ELBC_ECC_SW_BCH?

I would like to point out that the shuffling chip->ecc.write_subpage = 
fsl_elbc_write_subpage; line is needed for subpages to work properly with 
SW ECC because the write_subpage pointer in elbc_fcm_ctrl is left intact 
when SW ECC is being initialized and then the ECC does not work for 
subpages. Not setting it causes that default write_subpage function is 
used.


2) We need to implement RNDOUT operation for SW ECC / ECC_BCH. My attempt 
follows:

@@ -335,6 +335,21 @@ static void fsl_elbc_cmdfunc(struct mtd_info *mtd, 
unsigned int command,
                fsl_elbc_run_command(mtd);
                return;
 
+       /* !!! Experimental */
+       case NAND_CMD_RNDOUT:
+               dev_vdbg(priv->dev,
+                       "fsl_elbc_cmdfunc: NAND_CMD_RNDOUT, "
+                       "column: 0x%x.\n", column);
+
+               if ((column < 512) || (priv->page_size && (column < 2048))) 
{
+                       elbc_fcm_ctrl->index = column;
+                       return;
+               } else {
+                       column -= priv->page_size ? 2048 : 512;
+                       page_addr = elbc_fcm_ctrl->page;
+                       /* and fall-through to READOOB */
+               }
+
        /* READOOB reads only the OOB because no ECC is performed. */
        case NAND_CMD_READOOB:
                dev_vdbg(priv->dev,

Maybe it is too complicated (?) and it would be sufficient to set 
elbc_fcm_ctrl->index = column both for IB and OOB data? It seems that both 
ways work for me.

Cheers,
Tomas