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

[PATCH v5] 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..0eddae8 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 > ecc_strength)
+		erase_threshold = 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.7.10.4

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

[Huang Shijie]

On Tue, Dec 17, 2013 at 02:45:42PM +0100, Elie De Brauwer wrote:
>  
> +	/* Set the tolerance for bitflips when reading erased blocks. */
> +	erase_threshold = gf_len / 2;
> +	if (erase_threshold > ecc_strength)
> +		erase_threshold = ecc_strength;
> +
I was about to give you my ACK, but i find you used a wrong ecc strength
here.  The "ecc_strength" is just half of the real ECC strength used by
the BCH. Please read this line in the function:
268	ecc_strength  = bch_geo->ecc_strength >> 1;

Could you please send a new version patch ?


> +	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;
>  }
>  
> +/*
> + * 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;
> +}

Since a new version patch is inevitable, i want to give more comment
about this function.


Does the following code run faster then above?
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;
	int tmp;

	for (i = 0; i < geo->ecc_chunk_size; i++) {
		tmp = hweight8(~data[base + i]);

		if (tmp) {
			data[base + i] = 0xff;
			flip_bits += tmp;
		}
	}

	return flip_bits;
}

I am not sure this code is faster then your code, i do not have time to
do a test to compare the two functions.

If you think your function is better, just ignore my code, it is okay to
me.

I really very appreciate at your work!

thanks
Huang Shijie

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

[Elie De Brauwer]

On Wed, Dec 18, 2013 at 6:21 AM, Huang Shijie <shijie8@gmail.com> wrote:
> On Tue, Dec 17, 2013 at 02:45:42PM +0100, Elie De Brauwer wrote:
>>
>> +     /* Set the tolerance for bitflips when reading erased blocks. */
>> +     erase_threshold = gf_len / 2;
>> +     if (erase_threshold > ecc_strength)
>> +             erase_threshold = ecc_strength;
>> +
> I was about to give you my ACK, but i find you used a wrong ecc strength
> here.  The "ecc_strength" is just half of the real ECC strength used by
> the BCH. Please read this line in the function:
> 268     ecc_strength  = bch_geo->ecc_strength >> 1;
>
> Could you please send a new version patch ?
>
>
>> +     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;
>>  }
>>
>> +/*
>> + * 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;
>> +}
>
> Since a new version patch is inevitable, i want to give more comment
> about this function.
>
>
> Does the following code run faster then above?
> 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;
>         int tmp;
>
>         for (i = 0; i < geo->ecc_chunk_size; i++) {
>                 tmp = hweight8(~data[base + i]);
>
>                 if (tmp) {
>                         data[base + i] = 0xff;
>                         flip_bits += tmp;
>                 }
>         }
>
>         return flip_bits;
> }
>
> I am not sure this code is faster then your code, i do not have time to
> do a test to compare the two functions.
>
> If you think your function is better, just ignore my code, it is okay to
> me.
>
> I really very appreciate at your work!
>


Personally I would think the version you suggest would be more optimal and if
I wouldn't have stolen my version from the omap2 driver it would probably looked
more like the one you suggested. So I wrote up a little benchmark application
(available at: https://github.com/amo-ej1/Code-snippets/blob/master/C_C%2B%2B/bitflips_bench/bitflips_bench.c
)

But the output actually surprises me a bit. on my laptop your
suggestion is about 30% faster.

edb@lapelidb:~/today$ gcc bitflips_bench.c  && ./a.out
omap  : 37103 usec
speedy: 28141 usec
edb@lapelidb:~/today$ gcc -O2 ./bitflips_bench.c
edb@lapelidb:~/today$ ./a.out
omap  : 9530 usec
speedy: 6563 usec

However when testing it on my i.mx283 target  (used gcc 4.8.1) there
hardly isn't any difference
(first one without optimization, the second one with -O2).

root@(none):~# /tmp/a.out
omap  : 616839 usec
speedy: 634944 usec
root@(none):~# /tmp/a.out
omap  : 188134 usec
speedy: 188049 usec

(On 4k chunks, when testing it on 512 byte chunks the omap one
performs even better).

So I'm more tempted to stay at the original version. I will provide a
new version of the patch however with the ecc_strenght modified.

gr
E.
-- 
Elie De Brauwer