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 <drivers/mtd/nand/nand_spl_simple.c> as reference.
>
> Signed-off-by: Bo Shen <voice.s...@atmel.com>
> ---
> 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.
> 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.
> +#ifdef CONFIG_SPL_NAND_SOFTECC
This symbol needs to be documented (I realize it isn't new).
> +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).
> +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.
-Scott
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
