Dne Pá 9. července 2010 22:04:00 Scott Wood napsal(a): > On Tue, Jul 06, 2010 at 03:12:49AM +0200, Marek Vasut wrote: > > From: Compulab uboot <n...@none> > > Hmm?
Well I was unable to figure out who was the author, though the license of the code is GPL so it should be OK ? I'll try poking around a little bit more, but it's unlikely I'll find some more info. I dropped this patch from -next until this is fixed so -next can be pulled. Thanks for reviewing. > > > 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
