Re: Missing support for ECC_SOFT_BCH in fsl-elbc-nand

[Scott Wood <scottwood@freescale.com>]

On Mon, 2015-04-13 at 00:30 +0200, Tomas Hlavacek wrote:
> 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.

Yes, the hardware ECC on eLBC is only suitable if 1 bit per 512 bytes
meets the NAND chip's specifications.

> 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

You shouldn't be setting BR_DECC if you don't want to use HW ECC, so
this ifdef doesn't accomplish anything.

Also, while a compile-time hack may be suitable for your own use, to get
a mergable patch it should be a runtime decision.  You should be able to
read at runtime the level of ECC that the flash requires.

>         } 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)));

Instead change U-Boot to never set that bit in the first place.

> 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.

>From this I assume you're booting from NAND, but even so U-Boot will
overwrite that register with its own desired value.

> 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.

There is no default write_subpage function for swecc.

> 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.

I think you should just set index = column.  The OOB has already been
read if we previously read a full page -- and RNDOUT is also used in
other contexts such as ONFI identification.

-Scott