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

On Tue, Jul 06, 2010 at 03:12:49AM +0200, Marek Vasut wrote:

From: Compulab uboot none@none

Hmm?

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