[U-Boot] [PATCH] mtd: atmel_nand: according to pmecc version to perform 0xff page correction

[Andreas Bießmann]

Hi Bo, Josh,

On 01/16/2015 07:50 AM, Josh Wu wrote:
> Hi, Bo
> 
> On 1/16/2015 1:27 PM, Bo Shen wrote:
>> Hi Josh,
>>
>> On 01/16/2015 11:54 AM, Josh Wu wrote:
>>> As the PMECC hardware has different version. In SAMA5D4 chip, the
>>> PMECC ip
>>> can generate 0xff pmecc ECC value for all 0xff sector.
>>>
>>> According to this, add PMECC version check, if it's SAMA5D4 then we
>>> always
>>> let PMECC hardware to correct it.
>>>
>>> Signed-off-by: Josh Wu <josh.wu at atmel.com>
>>
>> except the nitpick.
>>
>> Acked-by: Bo Shen <voice.shen at atmel.com>

Acked-by: Andreas Bießmann <andreas.devel at googlemail.com>

> 
> Thanks for the quick review.
> 
>>
>>>
>>> ---
>>>
>>>   drivers/mtd/nand/atmel_nand.c     |  9 +++++++++
>>>   drivers/mtd/nand/atmel_nand_ecc.h | 20 ++++++++++++++++++++
>>>   2 files changed, 29 insertions(+)
>>>
>>> diff --git a/drivers/mtd/nand/atmel_nand.c
>>> b/drivers/mtd/nand/atmel_nand.c
>>> index 620b6e8..b16e3aa 100644
>>> --- a/drivers/mtd/nand/atmel_nand.c
>>> +++ b/drivers/mtd/nand/atmel_nand.c
>>> @@ -44,6 +44,7 @@ struct atmel_nand_host {
>>>       u8        pmecc_corr_cap;
>>>       u16        pmecc_sector_size;
>>>       u32        pmecc_index_table_offset;
>>> +    u32        pmecc_version;
>>>
>>>       int        pmecc_bytes_per_sector;
>>>       int        pmecc_sector_number;
>>> @@ -486,6 +487,10 @@ static int pmecc_correction(struct mtd_info
>>> *mtd, u32 pmecc_stat, uint8_t *buf,
>>>       int i, err_nbr, eccbytes;
>>>       uint8_t *buf_pos;
>>>
>>> +    /* SAMA5D4 PMECC IP can correct errors for all 0xff page */
>>> +    if (host->pmecc_version >= PMECC_VERSION_SAMA5D4)
>>
>> I think we can hard coded here, then we can drop the definition in
>> header file.
> I don't like hard coded magic number in personly.

me too!

> 
>>
>>> +        goto normal_check;
>>> +
>>>       eccbytes = nand_chip->ecc.bytes;
>>>       for (i = 0; i < eccbytes; i++)
>>>           if (ecc[i] != 0xff)
>>> @@ -961,6 +966,10 @@ static int atmel_pmecc_nand_init_params(struct
>>> nand_chip *nand,
>>>       nand->ecc.write_page = atmel_nand_pmecc_write_page;
>>>       nand->ecc.strength = cap;
>>>
>>> +    /* Check the PMECC ip version */
>>> +    host->pmecc_version = pmecc_readl(host->pmerrloc, version);
>>> +    dev_dbg(host->dev, "PMECC IP version is: %x\n",
>>> host->pmecc_version);
>>> +
>>>       atmel_pmecc_core_init(mtd);
>>>
>>>       return 0;
>>> diff --git a/drivers/mtd/nand/atmel_nand_ecc.h
>>> b/drivers/mtd/nand/atmel_nand_ecc.h
>>> index eac860d..b2d2682 100644
>>> --- a/drivers/mtd/nand/atmel_nand_ecc.h
>>> +++ b/drivers/mtd/nand/atmel_nand_ecc.h
>>> @@ -123,6 +123,20 @@ struct pmecc_errloc_regs {
>>>       u32 sigma[25];    /* 0x28-0x88 Error Location Sigma Registers */
>>>       u32 el[24];    /* 0x8C-0xE8 Error Location Registers */
>>>       u32 reserved1[5];    /* 0xEC-0xFC Reserved */
>>> +
>>> +    /*
>>> +     * 0x100-0x1F8:
>>> +     *   Reserved for AT91SAM9X5, AT91SAM9N12.
>>> +     *   HSMC registers for SAMA5D3, SAMA5D4.
>>> +     */
>>
>> I think no need to add this.
> actually, I would like to keep those comments.
> As in the datasheet, sama5d3 and sama5d4's PMECC ERRLOC only have the
> register range: 0x0~0xFF.
> and the range 0x100-0x1F8 is for the HSMC.
> 
> So people will be confused if they find HSMC definitions in the
> PMECC_ERRLOC structure.
> I think list this comment would be cleaner.

This is reasonable, please let the comment in.

> 
>>
>>> +    u32 reserved2[63];
>>> +
>>> +    /*
>>> +     * 0x1FC:
>>> +     *   PMECC version for AT91SAM9X5, AT91SAM9N12.
>>> +     *   HSMC version for SAMA5D3, SAMA5D4. Can refer as PMECC version.
>>> +     */
>>
>> ditto.
> Like mentioned above, I want avoid the confusion about PMECC_ERRLOC/HSMC.
> 
>>
>>> +    u32 version;
>>>   };
>>>
>>>   /* For Error Location Configuration Register */
>>> @@ -137,6 +151,12 @@ struct pmecc_errloc_regs {
>>>   #define        PMERRLOC_ERR_NUM_MASK        (0x1f << 8)
>>>   #define        PMERRLOC_CALC_DONE        (1 << 0)
>>>
>>> +/* PMECC IP version */
>>> +#define PMECC_VERSION_SAMA5D4            0x113
>>> +#define PMECC_VERSION_SAMA5D3            0x112
>>> +#define PMECC_VERSION_AT91SAM9N12        0x102
>>
>> No where will use the upper three definitions, we can drop them.
> I don't have strong option about this. Just think the version
> information is useful for other to reference.

Can someone else get this information from the datasheets? If not please
let this information there.


Best regards

Andreas Bießmann