From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bo Shen Date: Mon, 3 Mar 2014 10:41:53 +0800 Subject: [U-Boot] [PATCH 2/3] mtd: nand: atmel: prepare for nand spl boot support In-Reply-To: <1393547724.2697.64.camel@snotra.buserror.net> References: <1385954663-7682-1-git-send-email-voice.shen@atmel.com> <1385954663-7682-3-git-send-email-voice.shen@atmel.com> <1393547724.2697.64.camel@snotra.buserror.net> Message-ID: <5313EBF1.1010308@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 Scott, On 02/28/2014 08:35 AM, Scott Wood wrote: > On Mon, 2013-12-02 at 11:24 +0800, Bo Shen wrote: >> Prepare for nand spl boot support. It supports nand software ECC and >> hardware PMECC. >> This patch is take as reference. >> >> Signed-off-by: Bo Shen >> --- >> drivers/mtd/nand/atmel_nand.c | 206 +++++++++++++++++++++++++++++++++++++++++ >> include/nand.h | 6 ++ >> 2 files changed, 212 insertions(+) > > Looks OK but some style nits: > >> diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c >> index da83f06..64e11e1 100644 >> --- a/drivers/mtd/nand/atmel_nand.c >> +++ b/drivers/mtd/nand/atmel_nand.c >> @@ -32,6 +32,12 @@ >> >> #ifdef CONFIG_ATMEL_NAND_HW_PMECC >> >> +#ifdef CONFIG_SPL_BUILD >> +#ifdef CONFIG_SYS_NAND_ONFI_DETECTION >> +#undef CONFIG_SYS_NAND_ONFI_DETECTION >> +#endif >> +#endif > > There's no need to ifdef before undeffing. I will remove the ifdef. >> struct atmel_nand_host { >> struct pmecc_regs __iomem *pmecc; >> struct pmecc_errloc_regs __iomem *pmerrloc; >> @@ -1163,6 +1169,8 @@ static int at91_nand_ready(struct mtd_info *mtd) >> } >> #endif >> >> +#ifndef CONFIG_SPL_BUILD > > Use ifdef rather than ifndef for if/else. OK, I will use ifdef instead of ifndef. >> +#ifdef CONFIG_SPL_NAND_SOFTECC > > This symbol needs to be documented (I realize it isn't new). OK, I will document it. >> +static int nand_ecc_pos[] = CONFIG_SYS_NAND_ECCPOS; >> +#define ECCSTEPS (CONFIG_SYS_NAND_PAGE_SIZE / \ >> + CONFIG_SYS_NAND_ECCSIZE) >> +#define ECCTOTAL (ECCSTEPS * CONFIG_SYS_NAND_ECCBYTES) >> +#endif > > Is this stuff used anywhere but nand_read_page()? If not, move it > there (as local variables, not #defines). OK, I will move where use it. >> +static int nand_read_page(int block, int page, void *dst) >> +{ >> + struct nand_chip *this = mtd.priv; >> +#ifdef CONFIG_SPL_NAND_SOFTECC >> + u_char ecc_calc[ECCTOTAL]; >> + u_char ecc_code[ECCTOTAL]; >> + u_char oob_data[CONFIG_SYS_NAND_OOBSIZE]; >> + int eccsize = CONFIG_SYS_NAND_ECCSIZE; >> + int eccbytes = CONFIG_SYS_NAND_ECCBYTES; >> + int eccsteps = ECCSTEPS; >> + int i; >> + uint8_t *p = dst; >> +#endif >> + nand_command(block, page, 0, NAND_CMD_READ0); >> + >> +#ifdef CONFIG_SPL_NAND_SOFTECC >> + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) { >> + if (this->ecc.mode != NAND_ECC_SOFT) >> + this->ecc.hwctl(&mtd, NAND_ECC_READ); >> + this->read_buf(&mtd, p, eccsize); >> + this->ecc.calculate(&mtd, p, &ecc_calc[i]); >> + } >> + this->read_buf(&mtd, oob_data, CONFIG_SYS_NAND_OOBSIZE); >> + >> + for (i = 0; i < ECCTOTAL; i++) >> + ecc_code[i] = oob_data[nand_ecc_pos[i]]; >> + >> + eccsteps = ECCSTEPS; >> + p = dst; >> + >> + for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize) >> + this->ecc.correct(&mtd, p, &ecc_code[i], &ecc_calc[i]); >> +#else >> + atmel_nand_pmecc_read_page(&mtd, this, dst, 0, page); >> +#endif >> + >> + return 0; >> +} > > These don't share enough code to warrant interleaving like this -- just > have one big ifdef/else. It will be more readable. I will change in next version. > -Scott > > Best Regards, Bo Shen