From mboxrd@z Thu Jan 1 00:00:00 1970 From: Scott Wood Date: Tue, 18 Sep 2012 13:30:14 -0500 Subject: [U-Boot] [PATCH v3 09/11] S3C24XX: Add NAND Flash driver In-Reply-To: <5058BC02.7070705@inov.pt> (from jose.goncalves@inov.pt on Tue Sep 18 13:22:58 2012) References: <1347991325.15284.8@snotra> <5058BC02.7070705@inov.pt> Message-ID: <1347993014.15284.10@snotra> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 09/18/2012 01:22:58 PM, Jos? Miguel Gon?alves wrote: > On 18-09-2012 19:02, Scott Wood wrote: >> On 09/18/2012 12:40:36 PM, Jos? Miguel Gon?alves wrote: >>> +#define TACLS_VAL 7 /* CLE & ALE duration setting (0~7) */ >>> +#define TWRPH0_VAL 7 /* TWRPH0 duration setting (0~7) */ >>> +#define TWRPH1_VAL 7 /* TWRPH1 duration setting (0~7) */ >> >> Please use space, not tab, as a word separator (after the second >> #define). > > OK. > >> >>> + >>> +#define MAX_CHIPS 2 >>> +static int nand_cs[MAX_CHIPS] = { 0, 1 }; >> >> This needs explanation (and const). Better would be to use a priv >> struct, as discussed before. >> >>> +static void s3c_nand_select_chip(struct mtd_info *mtd, int chip) >>> +{ >>> + struct s3c24xx_nand *const nand = s3c24xx_get_base_nand(); >>> + u_long nfcont; >> >> s/u_long/u32/ > > Didn't catch this... Replace u_long with u32. >>> + nfcont = readl(&nand->nfcont); >>> + >>> + switch (chip) { >>> + case -1: >>> + nfcont |= NFCONT_NCE1 | NFCONT_NCE0; >>> + break; >>> + case 0: >>> + nfcont &= ~NFCONT_NCE0; >>> + break; >>> + case 1: >>> + nfcont &= ~NFCONT_NCE1; >>> + break; >>> + default: >>> + return; >> >> error message on default? > > OK. > >> >>> + } >>> + >>> + writel(nfcont, &nand->nfcont); >>> +} >>> + >>> +/* >>> + * Hardware specific access to control-lines function >>> + */ >>> +static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, >>> unsigned int ctrl) >>> +{ >>> + struct s3c24xx_nand *const nand = s3c24xx_get_base_nand(); >>> + struct nand_chip *this = mtd->priv; >>> + >>> + if (ctrl & NAND_CTRL_CHANGE) { >>> + if (ctrl & NAND_CLE) >>> + this->IO_ADDR_W = &nand->nfcmmd; >>> + else if (ctrl & NAND_ALE) >>> + this->IO_ADDR_W = &nand->nfaddr; >>> + else >>> + this->IO_ADDR_W = &nand->nfdata; >>> + if (ctrl & NAND_NCE) >>> + s3c_nand_select_chip(mtd, *(int *)this->priv); >>> + else >>> + s3c_nand_select_chip(mtd, -1); >>> + } >>> + >>> + if (cmd != NAND_CMD_NONE) >>> + writeb(cmd, this->IO_ADDR_W); >>> +} >> >> As discussed earlier, do you really need to mess with IO_ADDR_W or >> can you do it the way ndfc.c does? > > I will take a look at ndfc.c. Most of this driver was copy-paste from s3c64xx.c driver and an older patched U-Boot sources from Samsung, so I did not make any real code examination after it started to work... > >> >> I.e.: >> >> static void s3c_nand_hwcontrol(struct mtd_info *mtd, int cmd, >> unsigned int ctrl) >> { >> struct s3c24xx_nand *const nand = s3c24xx_get_base_nand(); >> struct nand_chip *this = mtd->priv; >> >> if (cmd == NAND_CMD_NONE) >> return; >> >> if (ctrl & NAND_CLE) >> writeb(cmd, &nand->nfcmmd); >> else >> writeb(cmd, &nand->nfaddr); >> } Oh sorry -- the above is missing the NCE part. Before the NAND_CMD_NONE check insert: if (ctrl & NAND_CTRL_CHANGE) { if (ctrl & NAND_NCE) s3c_nand_select_chip(mtd, *(const int *)this->priv); else s3c_nand_select_chip(mtd, -1); } >> >>> +/* >>> + * Board-specific NAND initialization. >>> + */ >>> +int board_nand_init(struct nand_chip *nand) >>> +{ >>> + static int chip_n; >>> + struct s3c24xx_nand *const nand_reg = s3c24xx_get_base_nand(); >>> + >>> + if (chip_n == 0) { >>> + /* Extend NAND timings to the maximum */ >>> + clrsetbits_le32(&nand_reg->nfconf, >>> + NFCONF_TACLS_MASK | NFCONF_TWRPH0_MASK | >>> + NFCONF_TWRPH1_MASK, >>> + NFCONF_TACLS(TACLS_VAL) | >>> + NFCONF_TWRPH0(TWRPH0_VAL) | >>> + NFCONF_TWRPH1(TWRPH1_VAL)); >>> + >>> + /* Disable chip selects and soft lock, enable controller */ >>> + clrsetbits_le32(&nand_reg->nfcont, NFCONT_WP, >>> + NFCONT_NCE1 | NFCONT_NCE0 | NFCONT_ENABLE); >>> + } else if (chip_n >= MAX_CHIPS) { >>> + return -ENODEV; >>> + } >>> + >>> + nand->IO_ADDR_R = &nand_reg->nfdata; >>> + nand->IO_ADDR_W = &nand_reg->nfdata; >>> + nand->cmd_ctrl = s3c_nand_hwcontrol; >>> + nand->dev_ready = s3c_nand_device_ready; >>> + nand->select_chip = s3c_nand_select_chip; >>> + nand->options = 0; >>> +#ifdef CONFIG_SPL_BUILD >>> + nand->read_buf = nand_read_buf; >>> +#endif >>> + >>> +#ifdef CONFIG_S3C24XX_NAND_HWECC >>> + nand->ecc.hwctl = s3c_nand_enable_hwecc; >>> + nand->ecc.calculate = s3c_nand_calculate_ecc; >>> + nand->ecc.correct = s3c_nand_correct_data; >>> + nand->ecc.mode = NAND_ECC_HW_OOB_FIRST; >>> + nand->ecc.size = CONFIG_SYS_NAND_ECCSIZE; >>> + nand->ecc.bytes = CONFIG_SYS_NAND_ECCBYTES; >>> +#else >>> + nand->ecc.mode = NAND_ECC_SOFT; >>> +#endif /* ! CONFIG_S3C24XX_NAND_HWECC */ >>> + >>> + nand->priv = nand_cs + chip_n++; >>> + >>> + return 0; >>> +} >> >> Please consider using the new SELF_INIT mechanism. >> > > Can you explain and/or point_to_resources for what this means (I'm a > U-Boot newbie)... See CONFIG_SYS_NAND_SELF_INIT in doc/README.nand I'd like the old way to be removed at some point. -Scott