Missing support for ECC_SOFT_BCH in fsl-elbc-nand

DISCUSSION Martin Strbačka 5 participants 9 messages Mar 27, 2015 – Apr 14, 2015

Subsystems (2)

Memory technology devices (MTD) NAND flash

lore

Share & tools

lore

Martin Strbačka

author

2015-03-27 14:13 UTC #0 lore raw

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

Richard Weinberger

2015-03-27 16:02 UTC #1 lore raw

Quoted 8 lines · 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?

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

Signature · 2 lines

Thanks,
//richard

Martin Strbačka

author

2015-03-30 08:36 UTC #2 lore raw

Quoted 10 lines · Richard Weinberger

Quoted 8 lines · 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?

Quoted 2 lines

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?

Richard Weinberger

maintainer

2015-03-30 14:21 UTC #3 lore raw

Hi!

Am 30.03.2015 um 10:35 schrieb Martin Strbačka:

Quoted 11 lines · Richard Weinberger

Quoted 10 lines · Martin Strbačka
Quoted 8 lines
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?

Quoted 6 lines

Quoted 2 lines
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

Scott Wood

2015-04-10 02:25 UTC #4 lore raw

Quoted 17 lines · Richard Weinberger

Hi!
Am 30.03.2015 um 10:35 schrieb Martin Strbačka:
Quoted 14 lines · Richard Weinberger
Quoted 12 lines · Martin Strbačka
Quoted 8 lines
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.

Quoted 8 lines

Quoted 3 lines
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

Tomas Hlavacek

2015-04-12 22:30 UTC #5 lore raw

In reply to Martin Strbačka

Hi!

Quoted 3 lines · Martin Strbačka

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:

drivers/mtd/nand/fsl_elbc_nand.c +0 -0

index 04b22fd..e5818ba 100644

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

Scott Wood

2015-04-13 19:13 UTC #6 lore raw

Quoted 11 lines · Tomas Hlavacek

Hi!
Quoted 3 lines · Martin Strbačka
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.

Quoted 23 lines

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:

drivers/mtd/nand/fsl_elbc_nand.c +1 -0

index 04b22fd..e5818ba 100644
@@ -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) {

drivers/mtd/nand/fsl_elbc_nand.c static int fsl_elbc_chip_init_tail(struct mtd_info · line 704

Quoted 5 lines

drivers/mtd/nand/fsl_elbc_nand.c +1 -0

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

Quoted 12 lines

drivers/mtd/nand/fsl_elbc_nand.c +0 -0

        } else {
                dev_err(priv->dev,

"fsl_elbc_init: page size %d is not supported\n",

drivers/mtd/nand/fsl_elbc_nand.c +2 -1

@@ -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
+

Quoted 2 lines

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.

Quoted 10 lines

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.

Quoted 6 lines

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.

Quoted 28 lines

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

drivers/mtd/nand/fsl_elbc_nand.c +15 -0

@@ -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,

Quoted 3 lines

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

Tomas Hlavacek

2015-04-13 21:57 UTC #7 lore raw

Hi!

Quoted 5 lines · Scott Wood

Quoted 4 lines · Tomas Hlavacek
1) How to disable HW ECC?
What I have done is this:

Quoted 8 lines

...
Quoted 1 line
... renamed from drivers/mtd/nand/fsl_elbc_nand.c +0 -0
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?

Quoted 8 lines

... static int fsl_elbc_chip_init(struct fsl_elbc_mtd · line 791
Quoted 6 lines
        } else {
                dev_err(priv->dev,
                        "fsl_elbc_init: page size %d is not supported\n",
... renamed from drivers/mtd/nand/fsl_elbc_nand.c +0 -0
@@ -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.

Quoted 7 lines

... static void fsl_elbc_cmdfunc(struct mtd_info *mtd, · line 336
Quoted 5 lines
2) We need to implement RNDOUT operation for SW ECC / ECC_BCH. My attempt follows:
... renamed from drivers/mtd/nand/fsl_elbc_nand.c +0 -0
@@ -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

Scott Wood

2015-04-14 00:59 UTC #8 lore raw

Quoted 13 lines · Tomas Hlavacek

Hi!
Quoted 10 lines · Scott Wood
...
Quoted 5 lines · Tomas Hlavacek
1) How to disable HW ECC?
What I have done is this:
... renamed from drivers/mtd/nand/fsl_elbc_nand.c +0 -0
You shouldn't be setting BR_DECC if you don't want to use HW ECC, so this ifdef doesn't accomplish anything.

Quoted 6 lines

Quoted 3 lines
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