On Tue, Jul 06, 2010 at 03:12:49AM +0200, Marek Vasut wrote: > From: Compulab uboot <n...@none>
Hmm? > Signed-off-by: Marek Vasut <[email protected]> > --- > drivers/mtd/nand/Makefile | 1 + > drivers/mtd/nand/pxa3xx_nand.c | 848 > ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 849 insertions(+), 0 deletions(-) > create mode 100644 drivers/mtd/nand/pxa3xx_nand.c > > diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile > index 28f27da..cd840cd 100644 > --- a/drivers/mtd/nand/Makefile > +++ b/drivers/mtd/nand/Makefile > @@ -50,6 +50,7 @@ COBJS-$(CONFIG_NAND_S3C64XX) += s3c64xx.o > COBJS-$(CONFIG_NAND_SPEAR) += spr_nand.o > COBJS-$(CONFIG_NAND_OMAP_GPMC) += omap_gpmc.o > COBJS-$(CONFIG_NAND_PLAT) += nand_plat.o > +COBJS-$(CONFIG_NAND_PXA3XX) += pxa3xx_nand.o > endif > > COBJS := $(COBJS-y) > diff --git a/drivers/mtd/nand/pxa3xx_nand.c b/drivers/mtd/nand/pxa3xx_nand.c > new file mode 100644 > index 0000000..380c918 > --- /dev/null > +++ b/drivers/mtd/nand/pxa3xx_nand.c > @@ -0,0 +1,848 @@ > +/* > + * drivers/mtd/nand/pxa3xx_nand.c > + * > + * Copyright ? 2005 Intel Corporation > + * Copyright ? 2006 Marvell International Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#include <common.h> > + > +#if defined(CONFIG_CMD_NAND) The makefile already limits compilation of this file to when CONFIG_NAND_PXA3XX is defined. > +#ifdef CONFIG_NEW_NAND_CODE Hmm? > +#define WAIT_EVENT_TIMEOUT (2 * CONFIG_SYS_HZ/10) > + > +static int wait_for_event(struct pxa3xx_nand_info *info, uint32_t event) > +{ > + int timeout = WAIT_EVENT_TIMEOUT; > + uint32_t ndsr; > + > + while (timeout--) { > + ndsr = NDSR & NDSR_MASK; > + if (ndsr & event) { > + NDSR = ndsr; > + return 0; > + } > + udelay(10); > + } You're defining WAIT_EVENT_TIMEOUT in terms of CONFIG_SYS_HZ ticks (which are supposed to be 1ms, but in any case you should only use CONFIG_SYS_HZ when you're interacting with get_ticks() or similar), but you're interpreting it as a number of 10us intervals. > + if (info->col_addr_cycles == 2) { > + /* large block, 2 cycles for column address > + * row address starts from 3rd cycle > + */ > + info->ndcb1 |= page_addr << 16; > + if (info->row_addr_cycles == 3) > + info->ndcb2 = (page_addr >> 16) & 0xff; > + } else > + /* small block, 1 cycles for column address > + * row address starts from 2nd cycle > + */ > + info->ndcb1 = page_addr << 8; If you put braces on one half of the if, please put it on the other half (and also when you have a multi-line body even if it's technically only one statement). > +/* calculate delta between OSCR values start and now */ > +static unsigned long get_delta(unsigned long start) > +{ > + unsigned long cur = OSCR; > + > + if(cur < start) /* OSCR overflowed */ > + return (cur + (start^0xffffffff)); > + else > + return (cur - start); > +} What's wrong with get_ticks()? > + > +/* wait_event with timeout */ > +static unsigned int wait_event(unsigned long event) > +{ > + unsigned long ndsr, timeout, start = OSCR; > + > + if(!event) > + return 0xff000000; > + > + if(event & (NDSR_CS0_CMDD | NDSR_CS0_BBD)) > + timeout = CONFIG_SYS_NAND_PROG_ERASE_TO * OSCR_CLK_FREQ; > + else > + timeout = CONFIG_SYS_NAND_OTHER_TO * OSCR_CLK_FREQ; > + > + while(1) { Space after keywords like "if" and "while". > +static int pxa3xx_nand_do_cmd(struct pxa3xx_nand_info *info, uint32_t event) > +{ > + unsigned int status; > + > + if (write_cmd(info)) { > + info->retcode = ERR_SENDCMD; > + goto fail_stop; > + } > + > + info->state = STATE_CMD_HANDLE; > + > + status = wait_event(event); > + if (status & (NDSR_RDDREQ | NDSR_DBERR)) { > + if (status & NDSR_DBERR) > + info->retcode = ERR_DBERR; > + > + info->state = STATE_PIO_READING; > + } else if (status & NDSR_WRDREQ) { > + info->state = STATE_PIO_WRITING; > + } else if (status & (NDSR_CS0_BBD | NDSR_CS0_CMDD)) { > + if (status & NDSR_CS0_BBD) > + info->retcode = ERR_BBERR; > + > + info->state = STATE_READY; > + } > +/* NDSR = status; */ Why commented out? > +fail_stop: > + NDCR &= ~NDCR_ND_RUN; > + udelay(10); > + return -ETIMEDOUT; Why is the udelay needed? > + default: > + printk(KERN_ERR "non-supported command.\n"); That's a bit vague -- tell the user this message comes from the NAND driver, and what the bad command is. > +/* > + * not required for Monahans DFC > + */ > +static void pxa3xx_nand_cmd_ctrl(struct mtd_info *mtd, int cmd, unsigned int > ctrl) > +{ > + return; > +} Just don't provide it at all. > +static int __readid(struct pxa3xx_nand_info *info, uint32_t *id) > +{ > + const struct pxa3xx_nand_flash *f = info->flash_info; > + const struct pxa3xx_nand_cmdset *cmdset = f->cmdset; > + uint8_t id_buff[8]; > + int i; > + unsigned long *long_buf; > + > + if (prepare_other_cmd(info, cmdset->read_id)) { > + printk(KERN_ERR "failed to prepare command\n"); > + return -EINVAL; > + } > + > + /* Send command */ > + if (write_cmd(info)) > + goto fail_timeout; > + > + /* Wait for CMDDM(command done successfully) */ > + if (wait_for_event(info, NDSR_RDDREQ)) > + goto fail_timeout; > + > + for (i = 0; i < 8; i += 4) { > + long_buf = (unsigned long *) &id_buff[i]; > + *long_buf = NDDB; > + } > + *id = id_buff[0] | (id_buff[1] << 8); > + return 0; > + > +fail_timeout: > + NDCR &= ~NDCR_ND_RUN; > + udelay(10); > + return -ETIMEDOUT; > +} > + Why is this done differently here than when the generic layer issues a READID command? > +static int pxa3xx_nand_config_flash(struct pxa3xx_nand_info *info, > + const struct pxa3xx_nand_flash *f) > +{ > + uint32_t ndcr = 0x00000FFF; /* disable all interrupts */ > + > + if (f->page_size != 2048 && f->page_size != 512) > + return -EINVAL; > + > + if (f->flash_width != 16 && f->flash_width != 8) > + return -EINVAL; > + > + /* calculate flash information */ > + info->oob_size = (f->page_size == 2048) ? 64 : 16; > + info->read_id_bytes = (f->page_size == 2048) ? 4 : 2; > + > + /* calculate addressing information */ > + info->col_addr_cycles = (f->page_size == 2048) ? 2 : 1; > + > + if (f->num_blocks * f->page_per_block > 65536) > + info->row_addr_cycles = 3; > + else > + info->row_addr_cycles = 2; This looks duplicative of pxa3xx_nand_detect_config. > + /* set info fields needed to __readid */ > + info->flash_info = &default_flash; > + info->read_id_bytes = (default_flash.page_size == 2048) ? 4 : 2; > + info->reg_ndcr = ndcr; > + > + if (__readid(info, &id)) > + return -ENODEV; > + > + /* Lookup the flash id */ > + id = (id >> 8) & 0xff; /* device id is byte 2 */ > + > + for (i = 0; nand_flash_ids[i].name != NULL; i++) { > + if (id == nand_flash_ids[i].id) { > + type = &nand_flash_ids[i]; > + break; > + } > + } Why do you need to do this here? The generic NAND code will look this up. > +static int pxa3xx_nand_detect_flash(struct pxa3xx_nand_info *info) > +{ > + uint32_t id = -1; > + int i; > + > + if (pxa3xx_nand_detect_config(info) == 0) > + return 0; > + > + for (i = 0; i < 1; ++i) { What's this one-iteration loop for? > + if (pxa3xx_nand_config_flash(info, info->flash_info)) > + continue; > + > + if (__readid(info, &id)) > + continue; > + > + return 0; > + } > + > + printf("failed to detect configured nand flash; found %04x instead > of\n", id); Instead of what? Did you really "find" anything in this case? pxa3xx_nand_detect_config already does the __readid, why do you need to do it again here? You don't do anything with the id if __readid succeeds. > + > + return -ENODEV; > +} > + > +static struct nand_ecclayout hw_smallpage_ecclayout = { > + .eccbytes = 6, > + .eccpos = {8, 9, 10, 11, 12, 13 }, > + .oobfree = { {2, 6} } > +}; Normally with small page NAND the bad block marker is at byte 5, at least with 8-bit chips. If you really have bad block information somewhere else, you'll want to provide a non-default badblock_pattern. > +#else > +#error "U-Boot legacy NAND support not available for Monahans DFC." > +#endif > +#endif Legacy NAND has been removed from u-boot altogether. Please don't reintroduce references to it. -Scott _______________________________________________ U-Boot mailing list [email protected] http://lists.denx.de/mailman/listinfo/u-boot
