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.shen@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