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 <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.
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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
