[U-Boot] [PATCH 5/5] PXA: PXA3xx NAND Driver

[Marek Vasut <marek.vasut@gmail.com>]

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 <none@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 <marek.vasut@gmail.com>
> > ---
> > 
> >  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