From mboxrd@z Thu Jan 1 00:00:00 1970 From: Josh Wu Date: Mon, 20 Aug 2012 18:13:56 +0800 Subject: [U-Boot] [PATCH v2 1/5] at91: atmel_nand: extract HWECC initialization code into one function: atmel_hw_nand_init_param(). In-Reply-To: <502CB5CC.10601@gmail.com> References: <1345093515-16761-1-git-send-email-josh.wu@atmel.com> <1345093515-16761-2-git-send-email-josh.wu@atmel.com> <502CB5CC.10601@gmail.com> Message-ID: <50320DE4.3050002@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, 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 >> --- >> 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