On Sat, Aug 08, 2015 at 05:58:04PM -0700, Steve Rae wrote: > From: Jiandong Zheng <jdzh...@broadcom.com> > > Add support for the iproc NAND, and enable on Cygnus and NSP boards. > > Signed-off-by: Jiandong Zheng <jdzh...@broadcom.com> > Signed-off-by: Steve Rae <s...@broadcom.com> > --- > > arch/arm/include/asm/arch-bcmcygnus/configs.h | 11 + > arch/arm/include/asm/arch-bcmnsp/configs.h | 11 + > drivers/mtd/nand/Makefile | 1 + > drivers/mtd/nand/iproc_nand.c | 1732 > +++++++++++++++++++++++++ > drivers/mtd/nand/iproc_nand_cygnus.h | 111 ++ > drivers/mtd/nand/iproc_nand_ns_plus.h | 113 ++ > 6 files changed, 1979 insertions(+) > create mode 100644 drivers/mtd/nand/iproc_nand.c > create mode 100644 drivers/mtd/nand/iproc_nand_cygnus.h > create mode 100644 drivers/mtd/nand/iproc_nand_ns_plus.h > > diff --git a/arch/arm/include/asm/arch-bcmcygnus/configs.h > b/arch/arm/include/asm/arch-bcmcygnus/configs.h > index 5354637..5338598 100644 > --- a/arch/arm/include/asm/arch-bcmcygnus/configs.h > +++ b/arch/arm/include/asm/arch-bcmcygnus/configs.h > @@ -10,6 +10,7 @@ > #include <asm/iproc-common/configs.h> > > /* uArchitecture specifics */ > +#define CONFIG_CYGNUS
Undocumented, should be in kconfig, and the name is too vague. > /* Serial Info */ > /* Post pad 3 bytes after each reg addr */ > @@ -22,4 +23,14 @@ > #define CONFIG_CONS_INDEX 3 > #define CONFIG_SYS_NS16550_COM3 0x18023000 > > +/* NAND configuration */ > +#define CONFIG_CMD_NAND > +#define CONFIG_NAND_IPROC > +#define CONFIG_IPROC_NAND_TIMING_MODE 5 > +#define CONFIG_SYS_NAND_BASE 0 > +#define CONFIG_SYS_MAX_NAND_DEVICE 1 > +#define CONFIG_SYS_MAX_NAND_CHIPS 1 > +#define CONFIG_SYS_NAND_SELF_INIT > +#define CONFIG_SYS_NAND_ONFI_DETECTION CONFIG_IPROC_NAND_TIMING_MODE and CONFIG_NAND_IPROC are undocumented, and new symbols should be done in kconfig. Please be consistent about CONFIG_NAND_IPROC* versus CONFIG_IPROC_NAND* (I prefer the former). If this timing mode is describing hardware, rather than user config, it should be CONFIG_SYS_NAND_IPROC_TIMING_MODE. > #endif /* __ARCH_CONFIGS_H */ > diff --git a/arch/arm/include/asm/arch-bcmnsp/configs.h > b/arch/arm/include/asm/arch-bcmnsp/configs.h > index 786deae..66f2266 100644 > --- a/arch/arm/include/asm/arch-bcmnsp/configs.h > +++ b/arch/arm/include/asm/arch-bcmnsp/configs.h > @@ -10,6 +10,7 @@ > #include <asm/iproc-common/configs.h> > > /* uArchitecture specifics */ > +#define CONFIG_NS_PLUS Undocumented, should be in kconfig, and the name is too vague. > +/* > + * Definitions > + */ > +#define DRV_NAME "iproc_nand" I suggest using __func__ instead. > +/* > + * IRQ operations > + */ > +#define NAND_ACK_IRQ(bit) writel(1, ((u32 *)ctrl.nand_intr_regs) + (bit)) > + > +#define NAND_TEST_IRQ(bit) (readl(((u32 *)ctrl.nand_intr_regs) + (bit)) & 1) This is the only place you ever use nand_intr_regs, so why not just define it as a u32 pointer? Also, use __iomem. > + > +/* > + * Data access macros for endianness > + */ > +#ifdef __LITTLE_ENDIAN > +#define NAND_BEGIN_DATA_ACCESS() \ > + writel(readl(ctrl.nand_idm_io_ctrl_direct_reg) | \ > + IDMFLD_NAND_IO_CONTROL_DIRECT_APB_LE_MODE, \ > + ctrl.nand_idm_io_ctrl_direct_reg) > +#define NAND_END_DATA_ACCESS() > \ > + writel(readl(ctrl.nand_idm_io_ctrl_direct_reg) & \ > + ~IDMFLD_NAND_IO_CONTROL_DIRECT_APB_LE_MODE, \ > + ctrl.nand_idm_io_ctrl_direct_reg) > +#else /* !__LITTLE_ENDIAN */ > +#define NAND_BEGIN_DATA_ACCESS() > +#define NAND_END_DATA_ACCESS() > +#endif /* !__LITTLE_ENDIAN */ Huh? > + > +/* > + * Misc NAND controller configuration/status macros > + */ > +#define NC_REG_CONFIG(cs) (NCREG_CONFIG_CS0 + ((cs) << 4)) > + > +#define WR_CONFIG(cs, field, val) do { > \ > + u32 reg = NC_REG_CONFIG(cs), contents = NAND_REG_RD(reg); \ > + contents &= ~(NCFLD_CONFIG_CS0_##field##_MASK); \ > + contents |= (val) << NCFLD_CONFIG_CS0_##field##_SHIFT; \ > + NAND_REG_WR(reg, contents); \ > +} while (0) > + > +#define RD_CONFIG(cs, field) \ > + ((NAND_REG_RD(NC_REG_CONFIG(cs)) & NCFLD_CONFIG_CS0_##field##_MASK) \ > + >> NCFLD_CONFIG_CS0_##field##_SHIFT) > + > +#define NC_REG_ACC_CONTROL(cs) (NCREG_ACC_CONTROL_CS0 + ((cs) << 4)) > + > +#define WR_ACC_CONTROL(cs, field, val) do { \ > + u32 reg = NC_REG_ACC_CONTROL(cs), contents = NAND_REG_RD(reg); \ > + contents &= ~(NCFLD_ACC_CONTROL_CS0_##field##_MASK); \ > + contents |= (val) << NCFLD_ACC_CONTROL_CS0_##field##_SHIFT; \ > + NAND_REG_WR(reg, contents); \ > +} while (0) > + > +#define RD_ACC_CONTROL(cs, field) \ > + ((NAND_REG_RD(NC_REG_ACC_CONTROL(cs)) & \ > + NCFLD_ACC_CONTROL_CS0_##field##_MASK) \ > + >> NCFLD_ACC_CONTROL_CS0_##field##_SHIFT) > + > +#define CORR_ERROR_COUNT (NAND_REG_RD(NCREG_CORR_ERROR_COUNT)) > +#define UNCORR_ERROR_COUNT (NAND_REG_RD(NCREG_UNCORR_ERROR_COUNT)) > + > +#define WR_CORR_THRESH(cs, val) do { \ > + u32 contents = NAND_REG_RD(NCREG_CORR_STAT_THRESHOLD); \ > + u32 shift = NCFLD_CORR_STAT_THRESHOLD_CS1_SHIFT * (cs); \ > + contents &= ~(NCFLD_CORR_STAT_THRESHOLD_CS0_MASK << shift); \ > + contents |= ((val) & NCFLD_CORR_STAT_THRESHOLD_CS0_MASK) << shift; \ > + NAND_REG_WR(NCREG_CORR_STAT_THRESHOLD, contents); \ > +} while (0) > + > +#define NC_REG_TIMING1(cs) (NCREG_TIMING_1_CS0 + ((cs) << 4)) > +#define NC_REG_TIMING2(cs) (NCREG_TIMING_2_CS0 + ((cs) << 4)) > + > +#define NAND_STRAP_TYPE > \ > + ((readl(ctrl.nand_strap_regs) & ctrl.data->strap_type_bitfield.mask) \ > + >> ctrl.data->strap_type_bitfield.shift) > +#define NAND_STRAP_PAGE > \ > + ((readl(ctrl.nand_strap_regs) & ctrl.data->strap_page_bitfield.mask) \ > + >> ctrl.data->strap_page_bitfield.shift) Please use functions instead of macros where possible. > + > +/* > + * Internal structures > + */ > +struct nand_strap_type { > + uint8_t sector_1k; > + uint8_t ecc_level; > + uint16_t spare_size; > +}; > + > +struct nand_strap_bitfield { > + uint32_t mask; > + uint32_t shift; > +}; What does "strap" mean here? > +/* > + * Internal support functions > + */ > +static inline int fls64(u64 x) > +{ > + u32 h = x >> 32; > + if (h) > + return fls(h) + 32; > + return fls(x); > +} Put this in a generic header. > +/* > + * NAND MTD API: read/program/erase > + */ > + > +static void iproc_nand_cmd_ctrl(struct mtd_info *mtd, > + int dat, unsigned int ctrl) > +{ > + /* intentionally left blank */ > +} Leave cmd_ctrl NULL instead of making it empty. > +static int iproc_nand_read(struct mtd_info *mtd, > + struct nand_chip *chip, u64 addr, unsigned int trans, > + u32 *buf, u8 *oob) > +{ > + struct iproc_nand_host *host = chip->priv; > + u64 start_addr = addr; > + int i; > + int oob_bytes; > + unsigned int max_bitflips, bitflips; > + unsigned int corr_error_count, uncorr_error_count; > + u32 *sector_buf = buf; > + u8 *sector_oob = oob; > + > + debug("%s %llx -> %p (trans %x)\n", __func__, > + (unsigned long long)addr, buf, trans); > + > + BUG_ON(!oob); > + > + NAND_ACK_IRQ(NCINTR_UNC); > + NAND_ACK_IRQ(NCINTR_CORR); > + max_bitflips = 0; > + corr_error_count = 0; > + uncorr_error_count = 0; > + > + NAND_REG_WR(NCREG_CMD_EXT_ADDRESS, > + (host->cs << 16) | ((addr >> 32) & 0xffff)); > + > + for (i = 0; i < trans; i++, addr += FC_BYTES) { > + if (!host->hwcfg.sector_size_1k || ((i & 0x1) == 0)) { > + sector_buf = buf; > + sector_oob = oob; > + } > + > + NAND_REG_WR(NCREG_CMD_ADDRESS, addr & 0xffffffff); > + > + if (ctrl.data_cache_invalid) { > + if ((i == 0) && RD_ACC_CONTROL(host->cs, PAGE_HIT_EN)) > + /* > + * temporarily disable the PAGE_HIT to force > + * data to be read from NAND > + */ > + WR_ACC_CONTROL(host->cs, PAGE_HIT_EN, 0); > + else > + ctrl.data_cache_invalid = 0; > + } > + > + /* SPARE_AREA_READ does not use ECC, so just use PAGE_READ */ > + iproc_nand_send_cmd(CMD_PAGE_READ); > + iproc_nand_waitfunc(mtd, chip); > + > + /* OOB bytes per sector */ > + oob_bytes = (mtd->oobsize / trans) << > + host->hwcfg.sector_size_1k; > + /* OOB bytes per 512B transfer */ > + if (host->hwcfg.sector_size_1k && (i & 0x01)) > + oob_bytes = max(0, oob_bytes - > + MAX_CONTROLLER_OOB_BYTES); > + oob_bytes = min(oob_bytes, MAX_CONTROLLER_OOB_BYTES); > + > + iproc_nand_cache_read(&buf, &oob, oob_bytes); > + > + if (ctrl.data_cache_invalid) { > + /* re-enable PAGE_HIT */ > + WR_ACC_CONTROL(host->cs, PAGE_HIT_EN, 1); > + ctrl.data_cache_invalid = 0; > + } > + > + if (buf && (!host->hwcfg.sector_size_1k || (i & 0x1))) { > + /* check uncorrectable errors */ > + if (NAND_TEST_IRQ(NCINTR_UNC)) { > + if (erased_sector(mtd, > + (u8 *)sector_buf, > + sector_oob, > + &bitflips)) { > + corr_error_count += bitflips; > + if (bitflips > max_bitflips) > + max_bitflips = bitflips; > + } else { > + uncorr_error_count += 1; > + } This level of indentation should be a hint that this function needs to be broken up into smaller ones. > + NAND_ACK_IRQ(NCINTR_UNC); > + ctrl.data_cache_invalid = 1; > + } > + /* check correctable errors */ > + if (NAND_TEST_IRQ(NCINTR_CORR)) { > + bitflips = CORR_ERROR_COUNT; > + corr_error_count += bitflips; > + if (bitflips > max_bitflips) > + max_bitflips = bitflips; > + NAND_ACK_IRQ(NCINTR_CORR); > + ctrl.data_cache_invalid = 1; > + } > + } > + } > + if (uncorr_error_count) { > + printf(DRV_NAME ": %d uncorrectable errors at 0x%llx\n", > + uncorr_error_count, (unsigned long long)start_addr); > + mtd->ecc_stats.failed += uncorr_error_count; > + /* NAND layer expects zero on ECC errors */ > + return 0; > + } > + if (max_bitflips) { > + debug("%s: corrected %d bit errors at 0x%llx\n", > + __func__, max_bitflips, (unsigned long long)start_addr); > + mtd->ecc_stats.corrected += corr_error_count; > + return max_bitflips; > + } > + > + return 0; > +} > + > +static int iproc_nand_read_page(struct mtd_info *mtd, > + struct nand_chip *chip, > + uint8_t *buf, > + int oob_required, > + int page) > +{ > + struct iproc_nand_host *host = chip->priv; > + > + BUG_ON(!buf); > + What's with all these !buf checks? Do you think it's more likely that you'll get a NULL buf than any of the other pointers that get passed in? > +static int iproc_nand_mark_bad(struct mtd_info *mtd, loff_t ofs) > +{ > + struct nand_chip *chip = mtd->priv; > + u64 blk_addr = (u64)ofs; > + > + debug("%s %llx\n", __func__, (unsigned long long)ofs); > + > + /* get NAND block address */ > + blk_addr &= ~((1 << chip->phys_erase_shift) - 1); > + > + memset(chip->oob_poi, 0xff, mtd->oobsize); > + chip->oob_poi[chip->badblockpos] = 0; > + > + return iproc_nand_write(mtd, chip, blk_addr, NULL, > + (u8 *)chip->oob_poi); > +} Why do you need a custom version of this? > + > +/* > + * Per-CS setup (1 NAND device) > + */ > + > +static const unsigned int block_sizes[] = { 8, 16, 128, 256, 512, 1024, 2048 > }; > +static const unsigned int page_sizes[] = { 512, 2048, 4096, 8192 }; > + > +static void iproc_nand_set_cfg(struct iproc_nand_host *host, > + struct iproc_nand_cfg *cfg) > +{ > + int i, found; > + > + for (i = 0, found = 0; i < ARRAY_SIZE(block_sizes); i++) > + if ((block_sizes[i] << 10) == cfg->block_size) { > + WR_CONFIG(host->cs, BLOCK_SIZE, i); > + found = 1; > + } U-Boot coding style requires braces around multiline statements. > + debug("%s: mtd configuration\n" > + "\tdevice_size 0x%llx\n" > + "\tblock_size 0x%x\n" > + "\tpage_size 0x%x\n" > + "\tdevice_width 0x%x\n" > + "\tcol_adr_bytes 0x%x\n" > + "\tblk_adr_bytes 0x%x\n" > + "\tspare_area_size 0x%x\n" > + "\tecc_level 0x%x\n" > + "\tsector_size_1k 0x%x\n", > + __func__, > + new_cfg.device_size, > + new_cfg.block_size, > + new_cfg.page_size, > + new_cfg.device_width, > + new_cfg.col_adr_bytes, > + new_cfg.blk_adr_bytes, > + new_cfg.spare_area_size, > + new_cfg.ecc_level, > + new_cfg.sector_size_1k); > + > + /* check settings determined by controller auto-init */ > + if (ctrl.auto_inited) { > + debug("%s: auto-init configuration\n" > + "\tdevice_size 0x%llx\n" > + "\tblock_size 0x%x\n" > + "\tpage_size 0x%x\n" > + "\tdevice_width 0x%x\n" > + "\tcol_adr_bytes 0x%x\n" > + "\tblk_adr_bytes 0x%x\n" > + "\tspare_area_size 0x%x\n" > + "\tecc_level 0x%x\n" > + "\tsector_size_1k 0x%x\n", > + __func__, > + orig_cfg.device_size, > + orig_cfg.block_size, > + orig_cfg.page_size, > + orig_cfg.device_width, > + orig_cfg.col_adr_bytes, > + orig_cfg.blk_adr_bytes, > + orig_cfg.spare_area_size, > + orig_cfg.ecc_level, > + orig_cfg.sector_size_1k); > + /* check basic device attributes first */ > + if (orig_cfg.device_size != new_cfg.device_size || > + orig_cfg.block_size != new_cfg.block_size || > + orig_cfg.page_size != new_cfg.page_size || > + orig_cfg.device_width != new_cfg.device_width || > + orig_cfg.col_adr_bytes != new_cfg.col_adr_bytes || > + orig_cfg.blk_adr_bytes != new_cfg.blk_adr_bytes || > + orig_cfg.ful_adr_bytes != new_cfg.ful_adr_bytes || > + orig_cfg.ecc_level == 0 || > + ((orig_cfg.ecc_code == ECC_CODE_BCH) && > + (orig_cfg.ecc_level > > + iproc_max_bch_ecc_level[orig_cfg.sector_size_1k])) || > + orig_cfg.spare_area_size > new_cfg.spare_area_size || > + ((orig_cfg.ecc_code == ECC_CODE_BCH) && > + (iproc_bch_ecc_bytes[orig_cfg.ecc_level] > > + orig_cfg.spare_area_size))) { > + /* ignore invalid auto-init settings */ > + ctrl.auto_inited = 0; > + printf(DRV_NAME ": invalid auto-init settings\n"); > + > + } else { > + /* auto-init has initialized the flash correctly. */ > + new_cfg = orig_cfg; > + printf(DRV_NAME ": following auto-init settings\n"); > + } > + } This is a bit elaborate... what is "auto-init"? > +static void iproc_nand_timing_setup(struct iproc_nand_host *host) > +{ > + struct nand_chip *chip = &host->chip; > + int onfi_tmode; /* bit mask of supported onfi timing modes */ > + > + /* > + * ctrl.tmode has the configured tmode upper limit [0-5] > + * or is -1 to indicate power-on default timing > + */ > + if (ctrl.tmode < 0 || ctrl.tmode >= ONFI_TIMING_MODES) > + return; > + > + if (chip->onfi_version) { > + onfi_tmode = le16_to_cpu(get_unaligned( > + (u16 *)&chip->onfi_params.async_timing_mode)); Fix onfi_get_async_timing_mode() instead, and have it use get_unaligned_le16(). > + if ((onfi_tmode == 0) || (onfi_tmode & ~0x3F)) { > + printf(DRV_NAME > + ": invalid ONFI timing mode ignored 0x%x\n", > + onfi_tmode); > + } else { > + /* > + * select the maximum supported ONFI timing mode > + * that is lower than the configured limit > + */ > + while (ctrl.tmode > 0) { > + if (onfi_tmode & (1 << ctrl.tmode)) > + break; > + ctrl.tmode--; > + } > + } > + } > + > + NAND_REG_WR(NC_REG_TIMING1(host->cs), > + ctrl.data->onfi_tmode[ctrl.tmode].timing1); > + NAND_REG_WR(NC_REG_TIMING2(host->cs), > + ctrl.data->onfi_tmode[ctrl.tmode].timing2); > + printf(DRV_NAME ": timing mode %d\n", ctrl.tmode); This verbosity is not appropriate for normal driver init. > +#ifdef CONFIG_SYS_MAX_NAND_CHIPS > +#if (CONFIG_SYS_MAX_NAND_CHIPS > NAND_MAX_CS) > +#error "Invalid CONFIG_SYS_MAX_NAND_CHIPS value" > +#endif > + ctrl.max_cs = CONFIG_SYS_MAX_NAND_CHIPS; > +#else > + ctrl.max_cs = 1; > +#endif > + > + /* write protect configuration */ > +#ifdef CONFIG_IPROC_NAND_WP_MODE > +#if ((CONFIG_IPROC_NAND_WP_MODE < 0) || \ > + (CONFIG_IPROC_NAND_WP_MODE > WP_ALWAYS_CLEARED)) > +#error "Invalid CONFIG_IPROC_NAND_WP_MODE value" > +#endif > + ctrl.wp_mode = CONFIG_IPROC_NAND_WP_MODE; > +#else > + ctrl.wp_mode = WP_SET_BY_DEFAULT; > +#endif > + > + /* timing mode configuration */ > +#ifdef CONFIG_IPROC_NAND_TIMING_MODE > +#if ((CONFIG_IPROC_NAND_TIMING_MODE < 0) || \ > + (CONFIG_IPROC_NAND_TIMING_MODE >= ONFI_TIMING_MODES)) > +#error "Invalid CONFIG_IPROC_NAND_TIMING_MODE value" > +#endif > + ctrl.tmode = CONFIG_IPROC_NAND_TIMING_MODE; > +#else > + ctrl.tmode = -1; /* use default timing configuration */ > +#endif These would be more readable as: #ifdef CONFIG_FOO #define CONFIG_FOO <default_value> #endif ... BUILD_BUG_ON(CONFIG_FOO is bad); ctrl.foo = CONFIG_FOO; > + debug("%s: nand_regs %p\n", __func__, ctrl.nand_regs); > + debug("%s: nand_intr_regs %p\n", __func__, ctrl.nand_intr_regs); > + debug("%s: nand_idm_regs %p\n", __func__, ctrl.nand_idm_regs); > + debug("%s: nand_idm_io_ctrl_direct_reg %p\n", __func__, > + ctrl.nand_idm_io_ctrl_direct_reg); > + debug("%s: nand_strap_regs %p\n", __func__, ctrl.nand_strap_regs); > + debug("%s: max_cs %d\n", __func__, ctrl.max_cs); > + debug("%s: wp_mode %d\n", __func__, ctrl.wp_mode); This driver has a lot of debug clutter. Is it really > +#ifdef CONFIG_IPROC_NAND_AUTO_INIT Undocumented, and should be in kconfig. > +int iproc_nand_init(void) > +{ > + struct iproc_nand_host *host; > + struct mtd_info *mtd; > + struct nand_chip *chip; > + > + NAND_REG_WR(NCREG_SEMAPHORE, 0); > + > + host = &nand_host; > + memset(host, 0, sizeof(*host)); > + > + host->mtd = &nand_info[0]; > + > + host->cs = 0; > + > + mtd = host->mtd; > + chip = &host->chip; > + > + chip->priv = host; > + mtd->priv = chip; > + mtd->name = "iproc_nand"; > + /* > + * set mtd bitflip threshold to 1 as the desired threshold will > + * be set in the controller register. > + */ > + mtd->bitflip_threshold = 1; > + > + chip->IO_ADDR_R = (void *)0xdeadbeef; > + chip->IO_ADDR_W = (void *)0xdeadbeef; Why? > + chip->cmd_ctrl = iproc_nand_cmd_ctrl; > + chip->cmdfunc = iproc_nand_cmdfunc; > + chip->waitfunc = iproc_nand_waitfunc; > + chip->read_byte = iproc_nand_read_byte; > + chip->read_buf = iproc_nand_read_buf; > + if (ctrl.max_cs > 1) > + chip->select_chip = iproc_nand_select_chip; > + > + chip->block_bad = iproc_nand_block_bad; > + chip->block_markbad = iproc_nand_mark_bad; > + > + chip->ecc.mode = NAND_ECC_HW; > + chip->ecc.layout = &iproc_nand_oob_layout; > + chip->ecc.read_page = iproc_nand_read_page; > + chip->ecc.write_page = iproc_nand_write_page; > + chip->ecc.read_page_raw = iproc_nand_read_page_raw; > + chip->ecc.write_page_raw = iproc_nand_write_page_raw; > + > + chip->ecc.write_oob_raw = iproc_nand_write_oob_raw; > + chip->ecc.read_oob_raw = iproc_nand_read_oob_raw; > + > + chip->ecc.read_oob = (void *)iproc_nand_read_oob; > + chip->ecc.write_oob = iproc_nand_write_oob; > + > + chip->controller = &ctrl.controller; > + > + if (nand_scan_ident(mtd, ctrl.max_cs, NULL)) > + return -ENXIO; > + > + chip->options |= NAND_NO_SUBPAGE_WRITE | NAND_SKIP_BBTSCAN; > + > + chip->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB; Why skip the BBT scan? -Scott _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot