From mboxrd@z Thu Jan 1 00:00:00 1970 From: Josh Wu Date: Fri, 16 Jan 2015 16:51:43 +0800 Subject: [U-Boot] [PATCH] mtd: atmel_nand: according to pmecc version to perform 0xff page correction In-Reply-To: <54B8CAAD.5060407@gmail.com> References: <1421380486-22138-1-git-send-email-josh.wu@atmel.com> <54B8A154.3000603@atmel.com> <54B8B4A8.8010702@atmel.com> <54B8CAAD.5060407@gmail.com> Message-ID: <54B8D11F.9090708@atmel.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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 >>> except the nitpick. >>> >>> Acked-by: Bo Shen > Acked-by: Andreas Bie?mann 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