From mboxrd@z Thu Jan  1 00:00:00 1970
From: Josh Wu <josh.wu@atmel.com>
Date: Fri, 16 Jan 2015 14:50:16 +0800
Subject: [U-Boot] [PATCH] mtd: atmel_nand: according to pmecc version to
 perform 0xff page correction
In-Reply-To: <54B8A154.3000603@atmel.com>
References: <1421380486-22138-1-git-send-email-josh.wu@atmel.com>
	<54B8A154.3000603@atmel.com>
Message-ID: <54B8B4A8.8010702@atmel.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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>

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.

>
>> +        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.

>
>> +    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.

>
>> +#define PMECC_VERSION_AT91SAM9X5 0x101
>
> If hard coded, we can drop it also.
>
>> +
>>   /* Galois field dimension */
>>   #define PMECC_GF_DIMENSION_13            13
>>   #define PMECC_GF_DIMENSION_14            14
>>
>
> Best Regards,
> Bo Shen

Thanks.
Best Regards,
Josh Wu