Hi, Andreas
On 8/16/2012 4:56 PM, Andreas Bießmann wrote:
Dear Josh Wu,
On 16.08.2012 07:05, Josh Wu wrote:
Extract the hwecc initialization code into one function. It is a preparation for adding atmel PMECC support.
Signed-off-by: Josh Wu josh.wu@atmel.com
drivers/mtd/nand/atmel_nand.c | 108 ++++++++++++++++++++++------------------- 1 file changed, 57 insertions(+), 51 deletions(-)
diff --git a/drivers/mtd/nand/atmel_nand.c b/drivers/mtd/nand/atmel_nand.c index de66382..113da93 100644 --- a/drivers/mtd/nand/atmel_nand.c +++ b/drivers/mtd/nand/atmel_nand.c @@ -232,68 +232,19 @@ static int atmel_nand_correct(struct mtd_info *mtd, u_char *dat, static void atmel_nand_hwctl(struct mtd_info *mtd, int mode) { } -#endif
-static void at91_nand_hwcontrol(struct mtd_info *mtd,
int cmd, unsigned int ctrl)-{
- struct nand_chip *this = mtd->priv;
- if (ctrl & NAND_CTRL_CHANGE) {
ulong IO_ADDR_W = (ulong) this->IO_ADDR_W;IO_ADDR_W &= ~(CONFIG_SYS_NAND_MASK_ALE| CONFIG_SYS_NAND_MASK_CLE);if (ctrl & NAND_CLE)IO_ADDR_W |= CONFIG_SYS_NAND_MASK_CLE;if (ctrl & NAND_ALE)IO_ADDR_W |= CONFIG_SYS_NAND_MASK_ALE;-#ifdef CONFIG_SYS_NAND_ENABLE_PIN
at91_set_gpio_value(CONFIG_SYS_NAND_ENABLE_PIN,!(ctrl & NAND_NCE));-#endif
this->IO_ADDR_W = (void *) IO_ADDR_W;- }
- if (cmd != NAND_CMD_NONE)
writeb(cmd, this->IO_ADDR_W);-}
-#ifdef CONFIG_SYS_NAND_READY_PIN -static int at91_nand_ready(struct mtd_info *mtd) +int atmel_hw_nand_init_param(struct nand_chip *nand)
Grr ... just realized your kernel patch has the same named function. I would have named it with 'ecc' in ... nevertheless I would accept this.
I think I can change the name to 'atmel_hwecc_nand_init_param'. it sounds better.
{
- return at91_get_gpio_value(CONFIG_SYS_NAND_READY_PIN);
-} -#endif
-int board_nand_init(struct nand_chip *nand) -{ -#ifdef CONFIG_ATMEL_NAND_HWECC static int chip_nr = 0;
This seems to be an remnant. It seems this is a mixture between 'SELF_INIT' and older initialization by common nand code. Can you please adopt to the CONFIG_SYS_NAND_SELFINIT? -> please read doc/REDME.nand If I got this correctly you just need to move this static chip_nr plus mtd detection (nand_scan_ident()) into the board_nand_init() and call it _always_. Next step is to define CONFIG_SYS_NAND_SELFINIT in include/nand.h and fix compiler issues (board_nand_init then has no parameter).
Thank you for point it out. I was not pay much attention in this part. According to your suggestion and read the code and README.nand file, I think I will add a new function 'board_nand_init(void)', which use to initialize hwecc & pmecc parameters.
I dunno if this is ok to call the nand_scan_ident twice when hwecc is enabled. Scott, can you please comment?
With the above change, I can make sure only call nand_scan_ident once.
struct mtd_info *mtd; -#endif
- nand->ecc.mode = NAND_ECC_SOFT;
-#ifdef CONFIG_SYS_NAND_DBW_16
- nand->options = NAND_BUSWIDTH_16;
-#endif
- nand->cmd_ctrl = at91_nand_hwcontrol;
-#ifdef CONFIG_SYS_NAND_READY_PIN
- nand->dev_ready = at91_nand_ready;
-#endif
- nand->chip_delay = 20;
-#ifdef CONFIG_ATMEL_NAND_HWECC nand->ecc.mode = NAND_ECC_HW; nand->ecc.calculate = atmel_nand_calculate; nand->ecc.correct = atmel_nand_correct; nand->ecc.hwctl = atmel_nand_hwctl; nand->ecc.read_page = atmel_nand_read_page; nand->ecc.bytes = 4; -#endif
-#ifdef CONFIG_ATMEL_NAND_HWECC mtd = &nand_info[chip_nr++]; mtd->priv = nand;
I think this is the most problematic part. mtd->priv was setup in nand_init_chip(int) before, so why rewrite it here? I know it was this way before ... I wonder if anybody is using atmel_nand with hwecc.
I agree. It seems those code should only for CONFIG_SYS_NAND_SELFINIT is defined. I will move this part of code to 'board_nand_init(void)'.
@@ -339,7 +290,62 @@ int board_nand_init(struct nand_chip *nand) break; } } -#endif
return 0; }
+#endif
+static void at91_nand_hwcontrol(struct mtd_info *mtd,
int cmd, unsigned int ctrl)+{
- struct nand_chip *this = mtd->priv;
- if (ctrl & NAND_CTRL_CHANGE) {
ulong IO_ADDR_W = (ulong) this->IO_ADDR_W;IO_ADDR_W &= ~(CONFIG_SYS_NAND_MASK_ALE| CONFIG_SYS_NAND_MASK_CLE);if (ctrl & NAND_CLE)IO_ADDR_W |= CONFIG_SYS_NAND_MASK_CLE;if (ctrl & NAND_ALE)IO_ADDR_W |= CONFIG_SYS_NAND_MASK_ALE;+#ifdef CONFIG_SYS_NAND_ENABLE_PIN
at91_set_gpio_value(CONFIG_SYS_NAND_ENABLE_PIN,!(ctrl & NAND_NCE));+#endif
this->IO_ADDR_W = (void *) IO_ADDR_W;- }
- if (cmd != NAND_CMD_NONE)
writeb(cmd, this->IO_ADDR_W);+}
+#ifdef CONFIG_SYS_NAND_READY_PIN +static int at91_nand_ready(struct mtd_info *mtd) +{
- return at91_get_gpio_value(CONFIG_SYS_NAND_READY_PIN);
+} +#endif
+int board_nand_init(struct nand_chip *nand) +{
- int res = 0;
- nand->ecc.mode = NAND_ECC_SOFT;
+#ifdef CONFIG_SYS_NAND_DBW_16
- nand->options = NAND_BUSWIDTH_16;
+#endif
- nand->cmd_ctrl = at91_nand_hwcontrol;
+#ifdef CONFIG_SYS_NAND_READY_PIN
- nand->dev_ready = at91_nand_ready;
+#endif
- nand->chip_delay = 20;
+#ifdef CONFIG_ATMEL_NAND_HWECC
- res = atmel_hw_nand_init_param(nand);
+#endif
- return res;
+}
Best regards
Andreas Bießmann
Best Regards, Josh Wu