Missing support for ECC_SOFT_BCH in fsl-elbc-nand

Missing support for ECC_SOFT_BCH in fsl-elbc-nand

[Martin Strbačka]

Hello,

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. So we would like to switch
to ECC_SOFT_BCH (or on-die ECC as I saw some patches in this list
recently). Do you know if anybody works on this or are there any patches
already?

I tried some hacks already because I found that ECC_SOFT is a fallback
option if HW ECC is not available. But it seems that there are few bits
missing. At least an implemetation of NAND_CMD_RNDOUT. Or am I missing
something?

Best Regards,
Martin Strbacka

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

[Richard Weinberger]

On Fri, Mar 27, 2015 at 3:13 PM, Martin Strbačka <martin.strbacka@nic.cz> wrote:
> Hello,
>
> 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. So we would like to switch
> to ECC_SOFT_BCH (or on-die ECC as I saw some patches in this list
> recently). Do you know if anybody works on this or are there any patches
> already?

Works on what?
I work (obviously) on on-die ECC support.

-- 
Thanks,
//richard

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

[Martin Strbačka]

On 27.3.2015 17:02, Richard Weinberger wrote:
> On Fri, Mar 27, 2015 at 3:13 PM, Martin Strbačka <martin.strbacka@nic.cz> wrote:
>> Hello,
>>
>> 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. So we would like to switch
>> to ECC_SOFT_BCH (or on-die ECC as I saw some patches in this list
>> recently). Do you know if anybody works on this or are there any patches
>> already?
> 
> Works on what?
> I work (obviously) on on-die ECC support.

I mean working on support ECC_SOFT_BCH or on-die ECC in fsl-elbc-nand. I
believe that fsl-elbc-nand needs some modifications to support those ECC
modes. Or am I wrong?

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

[Richard Weinberger]

Hi!

Am 30.03.2015 um 10:35 schrieb Martin Strbačka:
> On 27.3.2015 17:02, Richard Weinberger wrote:
>> On Fri, Mar 27, 2015 at 3:13 PM, Martin Strbačka <martin.strbacka@nic.cz> wrote:
>>> Hello,
>>>
>>> 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. So we would like to switch
>>> to ECC_SOFT_BCH (or on-die ECC as I saw some patches in this list
>>> recently). Do you know if anybody works on this or are there any patches
>>> already?
>>
>> Works on what?
>> I work (obviously) on on-die ECC support.
> 
> I mean working on support ECC_SOFT_BCH or on-die ECC in fsl-elbc-nand. I
> believe that fsl-elbc-nand needs some modifications to support those ECC
> modes. Or am I wrong?

Ah. My on-die ECC patches should work with any NFC. If you look
at the diffstat you'll notice that it does not touch any NAND driver.
Unless fsl-elbc-nand doesn't do strange things it should just work.
Please give it a try, feedback is very welcome! :)

I've developed it on an AT91 based system.

Thanks,
//richard

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

[Scott Wood]

On Mon, 2015-03-30 at 16:20 +0200, Richard Weinberger wrote:
> Hi!
> 
> Am 30.03.2015 um 10:35 schrieb Martin Strbačka:
> > On 27.3.2015 17:02, Richard Weinberger wrote:
> >> On Fri, Mar 27, 2015 at 3:13 PM, Martin Strbačka <martin.strbacka@nic.cz> wrote:
> >>> Hello,
> >>>
> >>> 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. So we would like to switch
> >>> to ECC_SOFT_BCH (or on-die ECC as I saw some patches in this list
> >>> recently). Do you know if anybody works on this or are there any patches
> >>> already?
> >>
> >> Works on what?
> >> I work (obviously) on on-die ECC support.
> > 
> > I mean working on support ECC_SOFT_BCH or on-die ECC in fsl-elbc-nand. I
> > believe that fsl-elbc-nand needs some modifications to support those ECC
> > modes. Or am I wrong?
> 
> Ah. My on-die ECC patches should work with any NFC. If you look
> at the diffstat you'll notice that it does not touch any NAND driver.
> Unless fsl-elbc-nand doesn't do strange things it should just work.
> Please give it a try, feedback is very welcome! :)

eLBC does "strange things" as its programming model is higher-level than
what nand_base.c was designed for.

-Scott

Re: 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

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

[Scott Wood]

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

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

[Tomas Hlavacek]

Hi!

On Monday, April 13, 2015 9:12:33 PM CEST, Scott Wood wrote:
> On Mon, 2015-04-13 at 00:30 +0200, Tomas Hlavacek wrote:
>> 1) How to disable HW ECC?
>> 
>> What I have done is this:
>> 
>> diff --git a/drivers/mtd/nand/fsl_elbc_nand.c  ...
>
> 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.

How? Could you please point me to some starting point what to read in order 
to implement it?

>
>>         } 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) ...
>
> Instead change U-Boot to never set that bit in the first place.

OK, got it.

>> 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); ...
>
> I think you should just set index = column.

OK, thanks.

Tomas

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

[Scott Wood]

On Mon, 2015-04-13 at 23:56 +0200, Tomas Hlavacek wrote:
> Hi!
> 
> On Monday, April 13, 2015 9:12:33 PM CEST, Scott Wood wrote:
> > On Mon, 2015-04-13 at 00:30 +0200, Tomas Hlavacek wrote:
> >> 1) How to disable HW ECC?
> >> 
> >> What I have done is this:
> >> 
> >> diff --git a/drivers/mtd/nand/fsl_elbc_nand.c  ...
> >
> > 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.
> 
> How? Could you please point me to some starting point what to read in order 
> to implement it?

The information should be in the chip's identification bytes, either via
ONFI or a manufacturer-specific format.  Or, if that's too complicated,
at least indicate the type of ECC to be used in the device tree.

-Scott