[PATCH v6] gpmi-nand bitflips in erased regions.

[PATCH v6] gpmi-nand bitflips in erased regions.

[Elie De Brauwer]

Changes in v6: Correction in obtaining the ecc_strength. 

Elie De Brauwer (1):
  mtd: gpmi: Deal with bitflips in erased regions regions

 drivers/mtd/nand/gpmi-nand/bch-regs.h  |  2 ++
 drivers/mtd/nand/gpmi-nand/gpmi-lib.c  | 18 ++++++++++++++
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 44 +++++++++++++++++++++++++++++++---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h |  1 +
 4 files changed, 62 insertions(+), 3 deletions(-)

-- 
1.8.5.1

[PATCH v6] mtd: gpmi: Deal with bitflips in erased regions regions

[Elie De Brauwer]

The BCH block typically used with a GPMI block on an i.MX28/i.MX6 is only
able to correct bitflips on data actually streamed through the block.
When erasing a block the data does not stream through the BCH block
and therefore no ECC data is written to the NAND chip. This causes
gpmi_ecc_read_page to return failure as soon as a single non-1-bit is
found in an erased page. Typically causing problems at higher levels
(ubifs corrupted empty space warnings). This problem was also observed
when using SLC NAND devices.

This patch configures the BCH block to mark a block as 'erased' if
not too much bitflips are found. Next HW_BCH_STATUS0:ALLONES
is used to check if the data read were all ones, indicating a read of a
properly erased chunk was performed. If this was not the case a slow path
is entered where bitflips are counted and corrected in software,
allowing the upper layers to take proper actions.

Signed-off-by: Elie De Brauwer <eliedebrauwer@gmail.com>
Acked-by: Peter Korsgaard <peter@korsgaard.com>
---
 drivers/mtd/nand/gpmi-nand/bch-regs.h  |  2 ++
 drivers/mtd/nand/gpmi-nand/gpmi-lib.c  | 18 ++++++++++++++
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 44 +++++++++++++++++++++++++++++++---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.h |  1 +
 4 files changed, 62 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/bch-regs.h b/drivers/mtd/nand/gpmi-nand/bch-regs.h
index 588f537..a30502f 100644
--- a/drivers/mtd/nand/gpmi-nand/bch-regs.h
+++ b/drivers/mtd/nand/gpmi-nand/bch-regs.h
@@ -30,7 +30,9 @@
 #define BM_BCH_CTRL_COMPLETE_IRQ		(1 << 0)
 
 #define HW_BCH_STATUS0				0x00000010
+#define BM_BCH_STATUS0_ALLONES_MASK		(1 << 4)
 #define HW_BCH_MODE				0x00000020
+#define BM_BCH_MODE_ERASE_THRESHOLD_MASK	0xff
 #define HW_BCH_ENCODEPTR			0x00000030
 #define HW_BCH_DATAPTR				0x00000040
 #define HW_BCH_METAPTR				0x00000050
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
index aaced29..24f295f 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
@@ -244,6 +244,7 @@ int bch_set_geometry(struct gpmi_nand_data *this)
 	unsigned int ecc_strength;
 	unsigned int page_size;
 	unsigned int gf_len;
+	unsigned int erase_threshold;
 	int ret;
 
 	if (common_nfc_set_geometry(this))
@@ -286,6 +287,14 @@ int bch_set_geometry(struct gpmi_nand_data *this)
 			| BF_BCH_FLASH0LAYOUT1_DATAN_SIZE(block_size, this),
 			r->bch_regs + HW_BCH_FLASH0LAYOUT1);
 
+	/* Set the tolerance for bitflips when reading erased blocks. */
+	erase_threshold = gf_len / 2;
+	if (erase_threshold > bch_geo->ecc_strength)
+		erase_threshold = bch_geo->ecc_strength;
+
+	writel(erase_threshold & BM_BCH_MODE_ERASE_THRESHOLD_MASK,
+		r->bch_regs + HW_BCH_MODE);
+
 	/* Set *all* chip selects to use layout 0. */
 	writel(0, r->bch_regs + HW_BCH_LAYOUTSELECT);
 
@@ -1094,6 +1103,15 @@ int gpmi_is_ready(struct gpmi_nand_data *this, unsigned chip)
 	return reg & mask;
 }
 
+/* Returns 1 if the last transaction consisted only out of ones. */
+int gpmi_all_ones(struct gpmi_nand_data *this)
+{
+	struct resources *r = &this->resources;
+	uint32_t reg = readl(r->bch_regs + HW_BCH_STATUS0);
+
+	return reg & BM_BCH_STATUS0_ALLONES_MASK;
+}
+
 static inline void set_dma_type(struct gpmi_nand_data *this,
 					enum dma_ops_type type)
 {
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index dabbc14..81488a0 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1012,6 +1012,30 @@ static void block_mark_swapping(struct gpmi_nand_data *this,
 	p[1] = (p[1] & mask) | (from_oob >> (8 - bit));
 }
 
+/*
+ * Count the number of 0 bits in a supposed to be
+ * erased region and correct them. Return the number
+ * of bitflips or zero when the region was correct.
+ */
+static unsigned int erased_sector_bitflips(unsigned char *data,
+					unsigned int chunk,
+					struct bch_geometry *geo)
+{
+	unsigned int flip_bits = 0;
+	int i;
+	int base = geo->ecc_chunk_size * chunk;
+
+	/* Count bitflips */
+	for (i = 0; i < geo->ecc_chunk_size; i++)
+		flip_bits += hweight8(~data[base + i]);
+
+	/* Correct bitflips by 0xFF'ing this chunk. */
+	if (flip_bits)
+		memset(&data[base], 0xFF, geo->ecc_chunk_size);
+
+	return flip_bits;
+}
+
 static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 				uint8_t *buf, int oob_required, int page)
 {
@@ -1023,6 +1047,7 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 	dma_addr_t    auxiliary_phys;
 	unsigned int  i;
 	unsigned char *status;
+	unsigned int  flips;
 	unsigned int  max_bitflips = 0;
 	int           ret;
 
@@ -1057,15 +1082,28 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 	status = auxiliary_virt + nfc_geo->auxiliary_status_offset;
 
 	for (i = 0; i < nfc_geo->ecc_chunk_count; i++, status++) {
-		if ((*status == STATUS_GOOD) || (*status == STATUS_ERASED))
+		if (*status == STATUS_GOOD)
 			continue;
 
 		if (*status == STATUS_UNCORRECTABLE) {
 			mtd->ecc_stats.failed++;
 			continue;
 		}
-		mtd->ecc_stats.corrected += *status;
-		max_bitflips = max_t(unsigned int, max_bitflips, *status);
+
+		if (*status == STATUS_ERASED) {
+			if (gpmi_all_ones(this))
+				break;
+			else
+				/* Erased block with bitflips. */
+				flips = erased_sector_bitflips(payload_virt, i,
+							       nfc_geo);
+		} else {
+			/* BCH block corrected some errors for us. */
+			flips = *status;
+		}
+
+		mtd->ecc_stats.corrected += flips;
+		max_bitflips = max_t(unsigned int, max_bitflips, flips);
 	}
 
 	if (oob_required) {
diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
index a7685e3..98db09e 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.h
@@ -268,6 +268,7 @@ extern void gpmi_clear_bch(struct gpmi_nand_data *);
 extern void gpmi_dump_info(struct gpmi_nand_data *);
 extern int bch_set_geometry(struct gpmi_nand_data *);
 extern int gpmi_is_ready(struct gpmi_nand_data *, unsigned chip);
+extern int gpmi_all_ones(struct gpmi_nand_data *);
 extern int gpmi_send_command(struct gpmi_nand_data *);
 extern void gpmi_begin(struct gpmi_nand_data *);
 extern void gpmi_end(struct gpmi_nand_data *);
-- 
1.8.5.1

Re: [PATCH v6] mtd: gpmi: Deal with bitflips in erased regions regions

[Huang Shijie]

On Wed, Dec 18, 2013 at 08:07:12PM +0100, Elie De Brauwer wrote:
> The BCH block typically used with a GPMI block on an i.MX28/i.MX6 is only
> able to correct bitflips on data actually streamed through the block.
> When erasing a block the data does not stream through the BCH block
> and therefore no ECC data is written to the NAND chip. This causes
> gpmi_ecc_read_page to return failure as soon as a single non-1-bit is
> found in an erased page. Typically causing problems at higher levels
> (ubifs corrupted empty space warnings). This problem was also observed
> when using SLC NAND devices.
> 
> This patch configures the BCH block to mark a block as 'erased' if
> not too much bitflips are found. Next HW_BCH_STATUS0:ALLONES
> is used to check if the data read were all ones, indicating a read of a
> properly erased chunk was performed. If this was not the case a slow path
> is entered where bitflips are counted and corrected in software,
> allowing the upper layers to take proper actions.
> 
> Signed-off-by: Elie De Brauwer <eliedebrauwer@gmail.com>
> Acked-by: Peter Korsgaard <peter@korsgaard.com>

thanks a lot!

Acked-by: Huang Shijie <b32955@freescale.com>

Re: [PATCH v6] mtd: gpmi: Deal with bitflips in erased regions regions

[Elie De Brauwer]

On Thu, Dec 19, 2013 at 6:10 AM, Huang Shijie <b32955@freescale.com> wrote:
> On Wed, Dec 18, 2013 at 08:07:12PM +0100, Elie De Brauwer wrote:
>> The BCH block typically used with a GPMI block on an i.MX28/i.MX6 is only
>> able to correct bitflips on data actually streamed through the block.
>> When erasing a block the data does not stream through the BCH block
>> and therefore no ECC data is written to the NAND chip. This causes
>> gpmi_ecc_read_page to return failure as soon as a single non-1-bit is
>> found in an erased page. Typically causing problems at higher levels
>> (ubifs corrupted empty space warnings). This problem was also observed
>> when using SLC NAND devices.
>>
>> This patch configures the BCH block to mark a block as 'erased' if
>> not too much bitflips are found. Next HW_BCH_STATUS0:ALLONES
>> is used to check if the data read were all ones, indicating a read of a
>> properly erased chunk was performed. If this was not the case a slow path
>> is entered where bitflips are counted and corrected in software,
>> allowing the upper layers to take proper actions.
>>
>> Signed-off-by: Elie De Brauwer <eliedebrauwer@gmail.com>
>> Acked-by: Peter Korsgaard <peter@korsgaard.com>
>
> thanks a lot!
>
> Acked-by: Huang Shijie <b32955@freescale.com>
>

Hello Huang, all,

Huang, you suggested earlier after checking with your hw guys to make
use of the ALLONES bit
in the HW_BCH0_STATUS0 register, this looked like a nice solution
since this could introduce a fast
path. However the documentation did not mention how the ALLONES bit is
implemented, including whether
or not it takes the erase threshold into account. Obviously V6 of my
patch will only function if ALLONES function if _all bits are
physically one_ and fail to function if allones something like the AND
of all statusses of the chunk.

I've been given a board with another bitflip (jay). It propgates as
the following error:
"UBIFS error (pid 36): ubifs_recover_master_node: failed to recover master node"

And if I look at the data I see the LEB consists out of 55 valid
master nodes, but two pages after the last valid master node there is
a bitflip which causes in ubifs/recovery.c get_master_node() to
trigger:
                if (!is_empty(buf, len))
                        goto out_err;

When dumping the data read I get to see a bitflip:
[    4.094719] c8a32e30: ff ff ff fd ff ff ff ff ff ff ff ff ff ff ff
ff  ................

This leads me to believe that the ALLONES bit is actually not behaving
the way I think it was. Implying that it actually is taking the
ERASE_THRESHOLD into account. (I verified this by dumping the status
register which was 0xff10 and counting the bitflips which actually
showed allones to be set while a bitflip was found) Making the allones
bit more or less useless in our case. Hence I suggest to revert the
use of the allones bit to the, (somewhat slower when reading erased
blocks) solution which always checks the erased blocks in
gpmi_ecc_read_page():

if (*status == STATUS_ERASED) {
   /* Erased block with bitflips. */
   flips = erased_sector_bitflips(payload_virt, i, nfc_geo);
} else {
   /* BCH block corrected some errors for us. */
   flips = *status;
}

What do you think and/or could you verify with your hardware guys ?

Thanks
E.
-- 
Elie De Brauwer

Re: [PATCH v6] mtd: gpmi: Deal with bitflips in erased regions regions

[Huang Shijie]

On Fri, Jan 03, 2014 at 10:43:41AM +0100, Elie De Brauwer wrote:
> On Thu, Dec 19, 2013 at 6:10 AM, Huang Shijie <b32955@freescale.com> wrote:
> > On Wed, Dec 18, 2013 at 08:07:12PM +0100, Elie De Brauwer wrote:
> >> The BCH block typically used with a GPMI block on an i.MX28/i.MX6 is only
> >> able to correct bitflips on data actually streamed through the block.
> >> When erasing a block the data does not stream through the BCH block
> >> and therefore no ECC data is written to the NAND chip. This causes
> >> gpmi_ecc_read_page to return failure as soon as a single non-1-bit is
> >> found in an erased page. Typically causing problems at higher levels
> >> (ubifs corrupted empty space warnings). This problem was also observed
> >> when using SLC NAND devices.
> >>
> >> This patch configures the BCH block to mark a block as 'erased' if
> >> not too much bitflips are found. Next HW_BCH_STATUS0:ALLONES
> >> is used to check if the data read were all ones, indicating a read of a
> >> properly erased chunk was performed. If this was not the case a slow path
> >> is entered where bitflips are counted and corrected in software,
> >> allowing the upper layers to take proper actions.
> >>
> >> Signed-off-by: Elie De Brauwer <eliedebrauwer@gmail.com>
> >> Acked-by: Peter Korsgaard <peter@korsgaard.com>
> >
> > thanks a lot!
> >
> > Acked-by: Huang Shijie <b32955@freescale.com>
> >
> 
> Hello Huang, all,
> 
> Huang, you suggested earlier after checking with your hw guys to make
> use of the ALLONES bit
> in the HW_BCH0_STATUS0 register, this looked like a nice solution
> since this could introduce a fast
> path. However the documentation did not mention how the ALLONES bit is
> implemented, including whether
> or not it takes the erase threshold into account. Obviously V6 of my
> patch will only function if ALLONES function if _all bits are
> physically one_ and fail to function if allones something like the AND
> of all statusses of the chunk.
> 
> I've been given a board with another bitflip (jay). It propgates as
> the following error:
> "UBIFS error (pid 36): ubifs_recover_master_node: failed to recover master node"
> 
> And if I look at the data I see the LEB consists out of 55 valid
> master nodes, but two pages after the last valid master node there is
> a bitflip which causes in ubifs/recovery.c get_master_node() to
> trigger:
>                 if (!is_empty(buf, len))
>                         goto out_err;
> 
> When dumping the data read I get to see a bitflip:
> [    4.094719] c8a32e30: ff ff ff fd ff ff ff ff ff ff ff ff ff ff ff
> ff  ................
> 
> This leads me to believe that the ALLONES bit is actually not behaving
> the way I think it was. Implying that it actually is taking the
> ERASE_THRESHOLD into account. (I verified this by dumping the status
> register which was 0xff10 and counting the bitflips which actually
> showed allones to be set while a bitflip was found) Making the allones

I confirmed with the hardware guy just now.
the hardware guy ate his word!

If we do not apply this patch, does it mean the UBIFS can not work?


thanks
Huang Shijie
> bit more or less useless in our case. Hence I suggest to revert the
> use of the allones bit to the, (somewhat slower when reading erased
> blocks) solution which always checks the erased blocks in
> gpmi_ecc_read_page():
> 
> if (*status == STATUS_ERASED) {
>    /* Erased block with bitflips. */
>    flips = erased_sector_bitflips(payload_virt, i, nfc_geo);
> } else {
>    /* BCH block corrected some errors for us. */
>    flips = *status;
> }
> 
> What do you think and/or could you verify with your hardware guys ?
> 
> Thanks
> E.
> -- 
> Elie De Brauwer
> 
> 

Re: [PATCH v6] mtd: gpmi: Deal with bitflips in erased regions regions

[Elie De Brauwer]

On Fri, Jan 3, 2014 at 10:44 AM, Huang Shijie <b32955@freescale.com> wrote:
> On Fri, Jan 03, 2014 at 10:43:41AM +0100, Elie De Brauwer wrote:
>> On Thu, Dec 19, 2013 at 6:10 AM, Huang Shijie <b32955@freescale.com> wrote:
>> > On Wed, Dec 18, 2013 at 08:07:12PM +0100, Elie De Brauwer wrote:
>> >> The BCH block typically used with a GPMI block on an i.MX28/i.MX6 is only
>> >> able to correct bitflips on data actually streamed through the block.
>> >> When erasing a block the data does not stream through the BCH block
>> >> and therefore no ECC data is written to the NAND chip. This causes
>> >> gpmi_ecc_read_page to return failure as soon as a single non-1-bit is
>> >> found in an erased page. Typically causing problems at higher levels
>> >> (ubifs corrupted empty space warnings). This problem was also observed
>> >> when using SLC NAND devices.
>> >>
>> >> This patch configures the BCH block to mark a block as 'erased' if
>> >> not too much bitflips are found. Next HW_BCH_STATUS0:ALLONES
>> >> is used to check if the data read were all ones, indicating a read of a
>> >> properly erased chunk was performed. If this was not the case a slow path
>> >> is entered where bitflips are counted and corrected in software,
>> >> allowing the upper layers to take proper actions.
>> >>
>> >> Signed-off-by: Elie De Brauwer <eliedebrauwer@gmail.com>
>> >> Acked-by: Peter Korsgaard <peter@korsgaard.com>
>> >
>> > thanks a lot!
>> >
>> > Acked-by: Huang Shijie <b32955@freescale.com>
>> >
>>
>> Hello Huang, all,
>>
>> Huang, you suggested earlier after checking with your hw guys to make
>> use of the ALLONES bit
>> in the HW_BCH0_STATUS0 register, this looked like a nice solution
>> since this could introduce a fast
>> path. However the documentation did not mention how the ALLONES bit is
>> implemented, including whether
>> or not it takes the erase threshold into account. Obviously V6 of my
>> patch will only function if ALLONES function if _all bits are
>> physically one_ and fail to function if allones something like the AND
>> of all statusses of the chunk.
>>
>> I've been given a board with another bitflip (jay). It propgates as
>> the following error:
>> "UBIFS error (pid 36): ubifs_recover_master_node: failed to recover master node"
>>
>> And if I look at the data I see the LEB consists out of 55 valid
>> master nodes, but two pages after the last valid master node there is
>> a bitflip which causes in ubifs/recovery.c get_master_node() to
>> trigger:
>>                 if (!is_empty(buf, len))
>>                         goto out_err;
>>
>> When dumping the data read I get to see a bitflip:
>> [    4.094719] c8a32e30: ff ff ff fd ff ff ff ff ff ff ff ff ff ff ff
>> ff  ................
>>
>> This leads me to believe that the ALLONES bit is actually not behaving
>> the way I think it was. Implying that it actually is taking the
>> ERASE_THRESHOLD into account. (I verified this by dumping the status
>> register which was 0xff10 and counting the bitflips which actually
>> showed allones to be set while a bitflip was found) Making the allones
>
> I confirmed with the hardware guy just now.
> the hardware guy ate his word!
>
> If we do not apply this patch, does it mean the UBIFS can not work?
>
>

Thanks for doublechecking ! I'll provide a new version of this patch
in the weekend.

If this patch does not get applied you will get bugreports related to
"corrupt empty space"
issues when using UBIFS, for the simple reason that ubifs only claims
to be tolerant to
corruption due to powercuts, and ubifs relies on the mtd layer that
bitflips are detected
and corrected.  In my situation I have already seen two distinct units
(on a 20 unit population
over a timespan of about two months) refuse to boot because of this
issue, which were
both solved by this patch. So ubifs might seem to work, but people
using it will likely bump
into nasty field issues.

my 2 cents
E.
-- 
Elie De Brauwer

Re: [PATCH v6] mtd: gpmi: Deal with bitflips in erased regions regions

[Huang Shijie]

On Fri, Jan 03, 2014 at 11:34:54AM +0100, Elie De Brauwer wrote:
> >
> > I confirmed with the hardware guy just now.
> > the hardware guy ate his word!
> >
> > If we do not apply this patch, does it mean the UBIFS can not work?
> >
> >
> 
> Thanks for doublechecking ! I'll provide a new version of this patch
> in the weekend.
> 
> If this patch does not get applied you will get bugreports related to
> "corrupt empty space"
> issues when using UBIFS, for the simple reason that ubifs only claims
> to be tolerant to
> corruption due to powercuts, and ubifs relies on the mtd layer that
> bitflips are detected
> and corrected.  In my situation I have already seen two distinct units
> (on a 20 unit population
> over a timespan of about two months) refuse to boot because of this
> issue, which were
> both solved by this patch. So ubifs might seem to work, but people
> using it will likely bump
> into nasty field issues.
I will discuss with our manager next week, and i hope the hardware guy
can fix it in the future.

thanks
Huang Shijie