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

[Josh Wu <josh.wu@atmel.com>]

Hi, Andreas

On 1/16/2015 4:24 PM, Andreas Bie?mann wrote:
> 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@atmel.com>
>>> except the nitpick.
>>>
>>> Acked-by: Bo Shen <voice.shen@atmel.com>
> Acked-by: Andreas Bie?mann <andreas.devel@googlemail.com>

Thanks.

>
>> 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?
No, This information are not in datasheet.

> If not please
> let this information there.
Okay. let's keep this as it is.

Best Regards,
Josh Wu

>
> Best regards
>
> Andreas Bie?mann