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

[Elie De Brauwer]

On Mon, Dec 16, 2013 at 5:30 AM, Huang Shijie <b32955 at freescale.com> wrote:
> On Sun, Dec 15, 2013 at 07:44:21PM +0100, Elie De Brauwer wrote:
>> The BCH block typically used with a i.MX28 and GPMI block 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 patch configures the BCH block to mark a block as 'erased' if
>> no more than ecc_strength bitflips are found. Next HW_BCH_STATUS0:ALLONES
>> is used to check if the data read were all ones. 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 at gmail.com>
>> Acked-by: Peter Korsgaard <peter at korsgaard.com>
>> ---
>>  drivers/mtd/nand/gpmi-nand/bch-regs.h  |    2 ++
>>  drivers/mtd/nand/gpmi-nand/gpmi-lib.c  |   17 +++++++++++++
>>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c |   43 +++++++++++++++++++++++++++++---
>>  drivers/mtd/nand/gpmi-nand/gpmi-nand.h |    1 +
>>  4 files changed, 60 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..4551a38 100644
>> --- a/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
>> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-lib.c
>> @@ -286,6 +286,13 @@ 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
>> +      * equal to the ecc_strength.
>> +      */
> Please also add the following comment :
>    "We even detect the bitflips for SLC nand."
>
>> +     writel(bch_geo->ecc_strength & BM_BCH_MODE_ERASE_THRESHOLD_MASK,
>> +             r->bch_regs + HW_BCH_MODE);
>> +
>
> After discuss with the IC owner, we could set the bch_geo->gf_len here.
>
> The bch_geo->gf_len is 13 or 14 now.
>
> For the sake of safe, I suggest to set (bch_geo->gf_len / 2) for the ERASE_THRESHOLD.
>
> I think this value is enough for us.
>
>
>>       /* Set *all* chip selects to use layout 0. */
>>       writel(0, r->bch_regs + HW_BCH_LAYOUTSELECT);
>>
>> @@ -1094,6 +1101,16 @@ 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_allones(struct gpmi_nand_data *this)
>> +{
>> +     struct resources *r = &this->resources;
>> +     uint32_t reg = readl(r->gpmi_regs + HW_BCH_STATUS0);
> please add a empty line here.
>> +     if (reg & BM_BCH_STATUS0_ALLONES_MASK)
>> +             return 1;
>> +     return 0;
> We can simplify the code to:
>         return reg & BM_BCH_STATUS0_ALLONES_MASK;
>

I was doing some stress testing on this piece of code today, but I'm
afraid the ALLONES solution apparently does not work. If I look at the
BCH_STATUS0 register while doing some heavy flash access, the
(entire) register remains zero at all times, while I would at least expect the
ALLONES_MASK to change when I'm reading erased sectors. Any
suggestions on this ? Since if we can't rely on the functioning of this bit
it would be better to stick to the first version of the patch (with the gf_len
threshold in place).

my 2 cents
E.



-- 
Elie De Brauwer