Skip to contentSkip to conversation

ADS5121 NAND driver

PATCH New John Rigby 3 participants 7 messages Oct 29, 2008 – Oct 31, 2008

Subsystems (2)
Memory technology devices (MTD) NAND flash
lore
Share & tools
lore

John Rigby

author
#0 lore raw

PATCH 1/1 ADS5121 NAND driver

ADS5121 rev4 / MPC5121e rev2 only

Only tested with:
2K page size
8 bit device width

This controller treats 2K pages as 4 512 byte pages and the hw ecc is over the combined 512 byte main area and the first 7 bytes of the spare area.

The hw ecc is stored in the last 9 bytes of the
spare area.

This all means the the spare area can not be written separately from the main. This means unmodified JFFS2 will not work.

Signed-off-by John Rigby <[email protected]>
---
 board/ads5121/ads5121.c             |    1 +
 drivers/mtd/nand/Makefile           |    1 +
 drivers/mtd/nand/mpc5121rev2_nand.c | 1122 +++++++++++++++++++++++++++++++++++
 include/configs/ads5121.h           |   27 +
 4 files changed, 1151 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mtd/nand/mpc5121rev2_nand.c
board/ads5121/ads5121.c +1 -0
index 0610928..3329f61 100644
@@ -34,6 +34,7 @@
 /* Clocks in use */
 #define SCCR1_CLOCKS_EN	(CLOCK_SCCR1_CFG_EN |				\
 			 CLOCK_SCCR1_LPC_EN |				\
+			 CLOCK_SCCR1_NFC_EN |				\
 			 CLOCK_SCCR1_PSC_EN(CONFIG_PSC_CONSOLE) |	\
 			 CLOCK_SCCR1_PSCFIFO_EN |			\
 			 CLOCK_SCCR1_DDR_EN |				\
drivers/mtd/nand/Makefile +1 -0
index b0abe6e..7addda6 100644
@@ -38,6 +38,7 @@ endif
 COBJS-$(CONFIG_NAND_FSL_ELBC) += fsl_elbc_nand.o
 COBJS-$(CONFIG_NAND_FSL_UPM) += fsl_upm.o
 COBJS-$(CONFIG_NAND_S3C64XX) += s3c64xx.o
+COBJS-$(CONFIG_NAND_MPC5121) += mpc5121rev2_nand.o
 endif
 
 COBJS	:= $(COBJS-y)
drivers/mtd/nand/mpc5121rev2_nand.c new file +1122 -0

1,122 lines not on this page · raw message

include/configs/ads5121.h +27 -0
index 8ec5e9d..ea79cd1 100644
@@ -33,6 +33,7 @@
  *
  * 0x0000_0000 - 0x0FFF_FFFF	DDR RAM (256 MB)
  * 0x3000_0000 - 0x3001_FFFF	SRAM (128 KB)
+ * 0x4000_0000 - 0x400F_FFFF	NFC (1 MB)
  * 0x8000_0000 - 0x803F_FFFF	IMMR (4 MB)
  * 0x8200_0000 - 0x8200_001F	CPLD (32 B)
  * 0x8400_0000 - 0x82FF_FFFF	PCI I/O space (16 MB)

@@ -199,6 +200,32 @@
 #undef CONFIG_SYS_FLASH_CHECKSUM
 
 /*
+ * NAND FLASH
+ * drivers/mtd/nand/mpc5121_mpc.c (rev 2 silicon/rev 4 boards only)
+ */
+#ifdef CONFIG_NAND_SPL
+#define CONFIG_SYS_NAND_BASE		0xFFF00000
+#else
+#define CONFIG_SYS_NAND_BASE		0x40000000
+#endif
+#define CONFIG_CMD_NAND 1
+/*
+ * The flash on ADS5121 board is two flash chips in one package
+ */
+#define CONFIG_SYS_MAX_NAND_DEVICE	2
+#define NAND_MAX_CHIPS		CONFIG_SYS_MAX_NAND_DEVICE
+#define CONFIG_SYS_NAND_SELECT_DEVICE	1
+#define CONFIG_NAND_MPC5121
+/*
+ * Configuration parameters for MPC5121 NAND driver
+ */
+#define CONFIG_MPC5121_NAND_WIDTH 1
+#define CONFIG_MPC5121_NAND_WRITE_SIZE 2048
+#define CONFIG_MPC5121_NAND_SPARE_SIZE 64
+#define CONFIG_MPC5121_NAND_CHIPS 2
+
+
+/*
  * CPLD registers area is really only 32 bytes in size, but the smallest possible LP
  * window is 64KB
  */
Signature · 1 line
1.5.6.2.255.gbed62
#1 lore raw

PATCH v2 1/1 ADS5121 NAND driver

Quoted 1 line · John Rigby

ADS5121 rev4 / MPC5121e rev2 only

What is unique about that revision that it cannot share a driver? It certainly shouldn't be board-specific...

Quoted 10 lines

This controller treats 2K pages as 4 512 byte pages and the hw ecc is over the combined 512 byte main area and the first 7 bytes of the spare area.

The hw ecc is stored in the last 9 bytes of the
spare area.

This all means the the spare area can not be written separately from the main. This means unmodified JFFS2 will not work.

Sigh... Smack the hardware people for me.

Quoted 5 lines
Signed-off-by John Rigby <[email protected]>
---
 board/ads5121/ads5121.c             |    1 +
 drivers/mtd/nand/Makefile           |    1 +
 drivers/mtd/nand/mpc5121rev2_nand.c | 1122 +++++++++++++++++++++++++++++++++++

This filename is very specific, especially since the i.MX NAND controller looks very similar. How about fsl_nfc_nand.c?

drivers/mtd/nand/mpc5121rev2_nand.c line 5

Quoted 11 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +5 -0
new file mode 100644
index 0000000..88555ab
@@ -0,0 +1,1122 @@
+/*
+ * Copyright 2004-2008 Freescale Semiconductor, Inc. All Rights Reserved.
+ *
+ * Based on drivers/mtd/nand/mpc5121_nand.c
+ * which was forked from drivers/mtd/nand/mxc_nd.c

Forking's bad, mmkay?

drivers/mtd/nand/mpc5121rev2_nand.c line 126

Quoted 13 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +13 -0
+/*
+ * OOB placement block for use with hardware ecc generation
+ */
+static struct nand_ecclayout nand_hw_eccoob_512 = {
+	.eccbytes = 9,
+	.eccpos = {
+		7, 8, 9, 10, 11, 12, 13, 14, 15,
+	},
+	.oobavail = 5,
+	.oobfree = {
+		{0, 5}
+	},
+};

Looks like you also have a free byte at offset 6.

Leave out oobavail, it will be calculated by generic code.

Quoted 7 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +7 -0
+static struct nand_ecclayout nand_hw_eccoob_2k = {
+	.eccbytes = 36,
+	.eccpos = {
+		/* 9 bytes of ecc for each 512 bytes of data */
+		7, 8, 9, 10, 11, 12, 13, 14, 15,
+		23, 24, 25, 26, 27, 28, 29, 30, 31,
+		39, 40, 41, 42, 43, 44, 45, 46, 47,

drivers/mtd/nand/mpc5121rev2_nand.c line 144

Quoted 10 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +10 -0
+		55, 56, 57, 58, 59, 60, 61, 62, 63,
+	},
+	.oobavail = 26,
+	.oobfree = {
+		{0, 5},
+		{16, 7},
+		{32, 7},
+		{48, 7},
+	},
+};

Byte zero is not free (it's used for bad-block markers), and byte one should be reserved for this as well. Bytes 5 and 6 are free.

drivers/mtd/nand/mpc5121rev2_nand.c line 147

Quoted 2 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +2 -0
+static struct nand_ecclayout nand_hw_eccoob_4k = {
+	.eccbytes = 64,	/* actually 72 but only room for 64 */

Let's fix that, then.

drivers/mtd/nand/mpc5121rev2_nand.c line 215

Quoted 10 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +10 -0
+/*
+ * Functions to transfer data to/from spare erea.
+ */
+static void copy_from_spare(struct mtd_info *mtd, void *pbuf, int len)
+{
+	u16 ooblen = mtd->oobsize;
+	u8 i, count, size;
+
+	count = mtd->writesize >> 9;
+	size = (ooblen / count >> 1) << 1;

If you want to round down to the nearest even number, use & ~1 (why would it ever be odd?). If not, what's going on?

drivers/mtd/nand/mpc5121rev2_nand.c line 221

Quoted 5 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +5 -0
+	for (i = 0; i < count - 1; i++) {
+		memcpy_fromio(pbuf, SPARE_AREA(i), size);
+		pbuf += size;
+		len -= size;
+	}

Shouldn't you check to make sure that len doesn't go negative?

drivers/mtd/nand/mpc5121rev2_nand.c line 246

Quoted 6 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +6 -0
+/*!
+ * This function polls the NFC to wait for the basic operation to complete by
+ * checking the INT bit of config2 register.
+ *
+ * @maxRetries	number of retry attempts (separated by 1 us)
+ */

noJavaCase, please.

Quoted 15 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +15 -0
+/*
+ * Function to correct the detected errors. This NFC corrects all the errors
+ * detected. So this function is not required.
+ */
+static int mpc5121_nand_correct_data(struct mtd_info *mtd, u_char *dat,
+				 u_char *read_ecc, u_char *calc_ecc)
+{
+	panic("Shouldn't be called here: %d\n", __LINE__);
+	return 0;		/* FIXME */
+}
+
+/*
+ * Function to calculate the ECC for the data to be stored in the Nand device.
+ * This NFC has a hardware RS(511,503) ECC engine together with the RS ECC
+ * CONTROL blocks are responsible for detection  and correction of up to

drivers/mtd/nand/mpc5121rev2_nand.c line 470

Quoted 10 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +10 -0
+ * 4 symbols of 9 bits each in 528 byte page.
+ * So this function is not required.
+ */
+
+static int mpc5121_nand_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
+					u_char *ecc_code)
+{
+	panic("Shouldn't be called here %d \n", __LINE__);
+	return 0;		/* FIXME */
+}

Just leave these function pointers NULL, since you replace read_page and write_page.

+static u_char mpc5121_nand_read_byte(struct mtd_info *mtd) +{
+ void *area_buf;
+ u_char rv;
+
+ /* Check for status request */
+ if (priv->status_req) {
+ rv = get_dev_status() & 0xff;
+ return rv;
+ }
+
+ if (priv->spare_only)
+ area_buf = SPARE_AREA(0);
+ else
+ area_buf = MAIN_AREA(0);
+
+ rv = in_8(area_buf + priv->col_addr);
+ priv->col_addr++;
+ return rv;
+}

You appear to support using read_byte on the spare area with READOOB, but not if the buffer had merely advanced past the main area. Not sure that it matters here, though.

Quoted 5 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +5 -0
+/*!
+ * This function reads byte from the NAND Flash
+ *
+ * @mtd		MTD structure for the NAND Flash
+ *

drivers/mtd/nand/mpc5121rev2_nand.c line 539

Quoted 10 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +10 -0
+ * @return	data read from the NAND Flash
+ */
+static u_char mpc5121_nand_read_byte16(struct mtd_info *mtd)
+{
+	/* Check for status request */
+	if (priv->status_req)
+		return (get_dev_status() & 0xff);
+
+	return mpc5121_nand_read_word(mtd) & 0xff;
+}

read_byte should never be called with 16-bit NAND.

drivers/mtd/nand/mpc5121rev2_nand.c line 552

Quoted 12 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +12 -0
+/*!
+ * This function writes data of length \b len from buffer \b buf to the NAND
+ * internal RAM buffer's MAIN area 0.
+ *
+ * @mtd		MTD structure for the NAND Flash
+ * @buf		data to be written to NAND Flash
+ * @len		number of bytes to be written
+ */
+static void mpc5121_nand_write_buf(struct mtd_info *mtd,
+					const u_char *buf, int len)
+{
+	printf("re-work may be needed?\n");

Answer this before we apply the patch. :-)

drivers/mtd/nand/mpc5121rev2_nand.c line 768

Quoted 7 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +7 -0
+	if (page_addr != -1)
+		do {
+			send_addr((page_addr & 0xff));
+			page_mask >>= 8;
+			page_addr >>= 8;
+		} while (page_mask != 0);
+}

Braces around multi-line if-bodies.

Quoted 5 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +5 -0
+
+/*
+ * Function to read a page from nand device.
+ */
+static void read_full_page(struct mtd_info *mtd, int page_addr)

drivers/mtd/nand/mpc5121rev2_nand.c line 783

Quoted 10 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +10 -0
+{
+	send_cmd(NAND_CMD_READ0);
+
+	mpc5121_nand_do_addr_cycle(mtd, 0, page_addr);
+
+	if (IS_LARGE_PAGE_NAND) {
+		send_cmd(NAND_CMD_READSTART);
+		send_read_page(0);
+	} else
+		send_read_page(0);

If you put braces around one half of the if, put it around the other.

drivers/mtd/nand/mpc5121rev2_nand.c line 879

Quoted 4 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +4 -0
+static int mpc5121_nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
+{
+	return (int)(get_dev_status());
+}

Unnecessary cast.

Quoted 24 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +24 -0
+static int mpc5121_nand_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
+				int page, int sndcmd)
+{
+	if (sndcmd) {
+		read_full_page(mtd, page);
+		sndcmd = 0;
+	}
+
+	copy_from_spare(mtd, chip->oob_poi, mtd->oobsize);
+	return sndcmd;
+}
+
+static int mpc5121_nand_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
+					int page)
+{
+	int status = 0;
+	int read_oob_col = 0;
+
+	send_cmd(NAND_CMD_READ0);
+	send_cmd(NAND_CMD_SEQIN);
+	mpc5121_nand_do_addr_cycle(mtd, read_oob_col, page);
+
+	/* copy the oob data */
+	copy_to_spare(mtd, chip->oob_poi, mtd->oobsize);

drivers/mtd/nand/mpc5121rev2_nand.c line 914

Quoted 10 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +10 -0
+
+	send_prog_page(0);
+
+	send_cmd(NAND_CMD_PAGEPROG);
+
+	status = mpc5121_nand_wait(mtd, chip);
+	if (status & NAND_STATUS_FAIL)
+		return -1;
+	return 0;
+}

Why override these rather than let them go through the command function?

drivers/mtd/nand/mpc5121rev2_nand.c line 952

Quoted 6 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +6 -0
+static struct nand_bbt_descr smallpage_memorybased = {
+	.options = NAND_BBT_SCAN2NDPAGE,
+	.offs = 5,
+	.len = 1,
+	.pattern = scan_ff_pattern
+};

This is the default.

drivers/mtd/nand/mpc5121rev2_nand.c line 959

Quoted 6 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +6 -0
+static struct nand_bbt_descr largepage_memorybased = {
+	.options = 0,
+	.offs = 5,
+	.len = 2,
+	.pattern = scan_ff_pattern
+};

This isn't normal -- why are you starting it at offset 5?

Quoted 11 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +11 -0
+static int mpc5121_nand_scan_bbt(struct mtd_info *mtd)
+{
+	struct nand_chip *this = mtd->priv;
+
+	if (IS_2K_PAGE_NAND)
+		this->ecc.layout = &nand_hw_eccoob_2k;
+	else if (IS_4K_PAGE_NAND)
+		if (priv->sparesize == 128)
+			this->ecc.layout = &nand_hw_eccoob_4k;
+		else
+			this->ecc.layout = &nand_hw_eccoob_4k_218_spare;

drivers/mtd/nand/mpc5121rev2_nand.c line 1006

Quoted 10 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +10 -0
+	else
+		this->ecc.layout = &nand_hw_eccoob_512;
+
+	/* propagate ecc.layout to mtd_info */
+	mtd->ecclayout = this->ecc.layout;
+
+#if 0
+	/* jffs2 should not write oob */
+	mtd->flags &= ~MTD_OOB_WRITEABLE;
+#endif

Should this be set, or not? No dead code.

Quoted 4 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +4 -0
+	/* use flash based bbt */
+	this->bbt_td = &bbt_main_descr;
+	this->bbt_md = &bbt_mirror_descr;
+

drivers/mtd/nand/mpc5121rev2_nand.c line 1021

Quoted 10 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +10 -0
+	/* update flash based bbt */
+	this->options |= NAND_USE_FLASH_BBT;
+
+	if (!this->badblock_pattern)
+		this->badblock_pattern = (mtd->writesize > 512) ?
+		    &largepage_memorybased : &smallpage_memorybased;
+
+	/* Build bad block table */
+	return nand_scan_bbt(mtd, this->badblock_pattern);
+}

I'd rather scan_bbt not be abused for this -- we need to fix the NAND probe code so that drivers can do things between nand_scan() and nand_scan_tail().

-Scott

author
#2 lore raw

PATCH v2 1/1 ADS5121 NAND driver

Scott, thanks for your feedback. I can easily fix most of the issues.

The one question I have is if this can go in only supporting 5121 rev2. If I need to add rev1 support it will take more time than I have right now.

Thanks
John

Quoted 1 line
Quoted 6 lines · Scott Wood
Quoted 2 lines · John Rigby

ADS5121 rev4 / MPC5121e rev2 only

What is unique about that revision that it cannot share a driver?

They could but it would be ugly.

Quoted 3 lines

It
certainly shouldn't be board-specific...

I agree, the config belongs in the board config file and the chip select belongs in a board file.

Quoted 23 lines
Quoted 5 lines

This controller treats 2K pages as 4 512 byte pages and the hw ecc is over the combined 512 byte main area and the first 7 bytes of the spare area.

Quoted 7 lines

The hw ecc is stored in the last 9 bytes of the
spare area.

This all means the the spare area can not be written separately from the main. This means unmodified JFFS2 will not work.

Sigh... Smack the hardware people for me.

Quoted 6 lines
Signed-off-by John Rigby <[email protected]>
---
 board/ads5121/ads5121.c             |    1 +
 drivers/mtd/nand/Makefile           |    1 +
 drivers/mtd/nand/mpc5121rev2_nand.c | 1122 +++++++++++++++++++++++++++++++++++
Quoted 3 lines

This filename is very specific, especially since the i.MX NAND controller looks very similar. How about fsl_nfc_nand.c?

Ok.

Quoted 16 lines

drivers/mtd/nand/mpc5121rev2_nand.c line 5

Quoted 13 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +5 -0
new file mode 100644
index 0000000..88555ab
@@ -0,0 +1,1122 @@
+/*
+ * Copyright 2004-2008 Freescale Semiconductor, Inc. All Rights Reserved.
+ *
+ * Based on drivers/mtd/nand/mpc5121_nand.c
+ * which was forked from drivers/mtd/nand/mxc_nd.c

Forking's bad, mmkay?

Yes, I'm guilty. The original was pretty ugly.

Quoted 18 lines
Quoted 5 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +3 -0
+/*
+ * OOB placement block for use with hardware ecc generation
+ */

drivers/mtd/nand/mpc5121rev2_nand.c line 126

Quoted 10 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +10 -0
+static struct nand_ecclayout nand_hw_eccoob_512 = {
+	.eccbytes = 9,
+	.eccpos = {
+		7, 8, 9, 10, 11, 12, 13, 14, 15,
+	},
+	.oobavail = 5,
+	.oobfree = {
+		{0, 5}
+	},
+};

Looks like you also have a free byte at offset 6.

Good catch.

Quoted 2 lines

Leave out oobavail, it will be calculated by generic code.

Ok.

Quoted 14 lines
Quoted 14 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +13 -0
+static struct nand_ecclayout nand_hw_eccoob_2k = {
+	.eccbytes = 36,
+	.eccpos = {
+		/* 9 bytes of ecc for each 512 bytes of data */
+		7, 8, 9, 10, 11, 12, 13, 14, 15,
+		23, 24, 25, 26, 27, 28, 29, 30, 31,
+		39, 40, 41, 42, 43, 44, 45, 46, 47,
+		55, 56, 57, 58, 59, 60, 61, 62, 63,
+	},
+	.oobavail = 26,
+	.oobfree = {
+		{0, 5},
+		{16, 7},
Quoted 9 lines

drivers/mtd/nand/mpc5121rev2_nand.c line 144

Quoted 5 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +4 -0
+		{32, 7},
+		{48, 7},
+	},
+};

Byte zero is not free (it's used for bad-block markers), and byte one should be reserved for this as well. Bytes 5 and 6 are free.

Ok.

Quoted 7 lines

drivers/mtd/nand/mpc5121rev2_nand.c line 147

Quoted 4 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +2 -0
+static struct nand_ecclayout nand_hw_eccoob_4k = {
+	.eccbytes = 64,	/* actually 72 but only room for 64 */

Let's fix that, then.

This is defined in generic mtd code that has exposure to userland.

include/mtd/mtd-abi.h:
/*
* ECC layout control structure. Exported to userspace for
* diagnosis and to allow creation of raw images */

struct nand_ecclayout {
    uint32_t eccbytes;
    uint32_t eccpos[64];
    uint32_t oobavail;
    struct nand_oobfree oobfree[MTD_MAX_OOBFREE_ENTRIES];
};
Quoted 16 lines

drivers/mtd/nand/mpc5121rev2_nand.c line 215

Quoted 12 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +10 -0
+/*
+ * Functions to transfer data to/from spare erea.
+ */
+static void copy_from_spare(struct mtd_info *mtd, void *pbuf, int len)
+{
+	u16 ooblen = mtd->oobsize;
+	u8 i, count, size;
+
+	count = mtd->writesize >> 9;
+	size = (ooblen / count >> 1) << 1;

If you want to round down to the nearest even number, use & ~1 (why would it ever be odd?). If not, what's going on?

I'll clean this up. This is left over from the i.MX version where io access's have to be word at a time.

Quoted 10 lines

drivers/mtd/nand/mpc5121rev2_nand.c line 221

Quoted 7 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +5 -0
+	for (i = 0; i < count - 1; i++) {
+		memcpy_fromio(pbuf, SPARE_AREA(i), size);
+		pbuf += size;
+		len -= size;
+	}

Shouldn't you check to make sure that len doesn't go negative

yes

Quoted 11 lines

drivers/mtd/nand/mpc5121rev2_nand.c line 246

Quoted 8 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +6 -0
+/*!
+ * This function polls the NFC to wait for the basic operation to complete by
+ * checking the INT bit of config2 register.
+ *
+ * @maxRetries	number of retry attempts (separated by 1 us)
+ */

noJavaCase, please.

Oops, missed one.

Quoted 22 lines
Quoted 22 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +21 -0
+/*
+ * Function to correct the detected errors. This NFC corrects all the errors
+ * detected. So this function is not required.
+ */
+static int mpc5121_nand_correct_data(struct mtd_info *mtd, u_char *dat,
+				 u_char *read_ecc, u_char *calc_ecc)
+{
+	panic("Shouldn't be called here: %d\n", __LINE__);
+	return 0;		/* FIXME */
+}
+
+/*
+ * Function to calculate the ECC for the data to be stored in the Nand device.
+ * This NFC has a hardware RS(511,503) ECC engine together with the RS ECC
+ * CONTROL blocks are responsible for detection  and correction of up to
+ * 4 symbols of 9 bits each in 528 byte page.
+ * So this function is not required.
+ */
+
+static int mpc5121_nand_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
+					u_char *ecc_code)
Quoted 9 lines

drivers/mtd/nand/mpc5121rev2_nand.c line 470

Quoted 5 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +4 -0
+{
+	panic("Shouldn't be called here %d \n", __LINE__);
+	return 0;		/* FIXME */
+}

Just leave these function pointers NULL, since you replace read_page and write_page.

ok

Quoted 21 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +20 -0
+static u_char mpc5121_nand_read_byte(struct mtd_info *mtd)
+{
+	void *area_buf;
+	u_char rv;
+
+	/* Check for status request */
+	if (priv->status_req) {
+		rv = get_dev_status() & 0xff;
+		return rv;
+	}
+
+	if (priv->spare_only)
+		area_buf = SPARE_AREA(0);
+	else
+		area_buf = MAIN_AREA(0);
+
+	rv = in_8(area_buf + priv->col_addr);
+	priv->col_addr++;
+	return rv;
+}
Quoted 4 lines

You appear to support using read_byte on the spare area with READOOB, but not if the buffer had merely advanced past the main area. Not sure that it matters here, though.

ok

Quoted 11 lines
Quoted 11 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +10 -0
+/*!
+ * This function reads byte from the NAND Flash
+ *
+ * @mtd		MTD structure for the NAND Flash
+ *
+ * @return	data read from the NAND Flash
+ */
+static u_char mpc5121_nand_read_byte16(struct mtd_info *mtd)
+{
+	/* Check for status request */
Quoted 9 lines

drivers/mtd/nand/mpc5121rev2_nand.c line 539

Quoted 6 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +5 -0
+	if (priv->status_req)
+		return (get_dev_status() & 0xff);
+
+	return mpc5121_nand_read_word(mtd) & 0xff;
+}

read_byte should never be called with 16-bit NAND.

This is the replacement for nand_read_byte16 in nand_base.c. /**
* nand_read_byte16 - [DEFAULT] read one byte endianess aware from the chip
* @mtd: MTD device structure
*
* Default read function for 16bit buswith with
* endianess conversion
*/
static uint8_t nand_read_byte16(struct mtd_info *mtd)

{
    struct nand_chip *chip = mtd->priv;
    return (uint8_t) cpu_to_le16(readw(chip->IO_ADDR_R));
}
Quoted 17 lines
Quoted 4 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +2 -0
+/*!
+ * This function writes data of length \b len from buffer \b buf to the NAND

drivers/mtd/nand/mpc5121rev2_nand.c line 552

Quoted 10 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +10 -0
+ * internal RAM buffer's MAIN area 0.
+ *
+ * @mtd		MTD structure for the NAND Flash
+ * @buf		data to be written to NAND Flash
+ * @len		number of bytes to be written
+ */
+static void mpc5121_nand_write_buf(struct mtd_info *mtd,
+					const u_char *buf, int len)
+{
+	printf("re-work may be needed?\n");

Answer this before we apply the patch. :-)

Perhaps I can just get rid of this whole routine?

Quoted 12 lines

drivers/mtd/nand/mpc5121rev2_nand.c line 768

Quoted 9 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +7 -0
+	if (page_addr != -1)
+		do {
+			send_addr((page_addr & 0xff));
+			page_mask >>= 8;
+			page_addr >>= 8;
+		} while (page_mask != 0);
+}

Braces around multi-line if-bodies.

ok

Quoted 11 lines
Quoted 11 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +10 -0
+
+/*
+ * Function to read a page from nand device.
+ */
+static void read_full_page(struct mtd_info *mtd, int page_addr)
+{
+	send_cmd(NAND_CMD_READ0);
+
+	mpc5121_nand_do_addr_cycle(mtd, 0, page_addr);
+
Quoted 9 lines

drivers/mtd/nand/mpc5121rev2_nand.c line 783

Quoted 6 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +5 -0
+	if (IS_LARGE_PAGE_NAND) {
+		send_cmd(NAND_CMD_READSTART);
+		send_read_page(0);
+	} else
+		send_read_page(0);

If you put braces around one half of the if, put it around the other.

ok

Quoted 9 lines

drivers/mtd/nand/mpc5121rev2_nand.c line 879

Quoted 6 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +4 -0
+static int mpc5121_nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
+{
+	return (int)(get_dev_status());
+}

Unnecessary cast.

ok

Quoted 30 lines
Quoted 30 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +29 -0
+static int mpc5121_nand_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
+				int page, int sndcmd)
+{
+	if (sndcmd) {
+		read_full_page(mtd, page);
+		sndcmd = 0;
+	}
+
+	copy_from_spare(mtd, chip->oob_poi, mtd->oobsize);
+	return sndcmd;
+}
+
+static int mpc5121_nand_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
+					int page)
+{
+	int status = 0;
+	int read_oob_col = 0;
+
+	send_cmd(NAND_CMD_READ0);
+	send_cmd(NAND_CMD_SEQIN);
+	mpc5121_nand_do_addr_cycle(mtd, read_oob_col, page);
+
+	/* copy the oob data */
+	copy_to_spare(mtd, chip->oob_poi, mtd->oobsize);
+
+	send_prog_page(0);
+
+	send_cmd(NAND_CMD_PAGEPROG);
+
Quoted 9 lines

drivers/mtd/nand/mpc5121rev2_nand.c line 914

Quoted 6 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +5 -0
+	status = mpc5121_nand_wait(mtd, chip);
+	if (status & NAND_STATUS_FAIL)
+		return -1;
+	return 0;
+}

Why override these rather than let them go through the command function?

Not sure, I think it is so we can call the copy to/from spare routine.

Quoted 11 lines

drivers/mtd/nand/mpc5121rev2_nand.c line 952

Quoted 8 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +6 -0
+static struct nand_bbt_descr smallpage_memorybased = {
+	.options = NAND_BBT_SCAN2NDPAGE,
+	.offs = 5,
+	.len = 1,
+	.pattern = scan_ff_pattern
+};

This is the default.

ok

Quoted 11 lines

drivers/mtd/nand/mpc5121rev2_nand.c line 959

Quoted 8 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +6 -0
+static struct nand_bbt_descr largepage_memorybased = {
+	.options = 0,
+	.offs = 5,
+	.len = 2,
+	.pattern = scan_ff_pattern
+};

This isn't normal -- why are you starting it at offset 5?

will fix

Quoted 17 lines
Quoted 17 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +16 -0
+static int mpc5121_nand_scan_bbt(struct mtd_info *mtd)
+{
+	struct nand_chip *this = mtd->priv;
+
+	if (IS_2K_PAGE_NAND)
+		this->ecc.layout = &nand_hw_eccoob_2k;
+	else if (IS_4K_PAGE_NAND)
+		if (priv->sparesize == 128)
+			this->ecc.layout = &nand_hw_eccoob_4k;
+		else
+			this->ecc.layout = &nand_hw_eccoob_4k_218_spare;
+	else
+		this->ecc.layout = &nand_hw_eccoob_512;
+
+	/* propagate ecc.layout to mtd_info */
+	mtd->ecclayout = this->ecc.layout;
Quoted 9 lines

drivers/mtd/nand/mpc5121rev2_nand.c line 1006

Quoted 6 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +5 -0
+
+#if 0
+	/* jffs2 should not write oob */
+	mtd->flags &= ~MTD_OOB_WRITEABLE;
+#endif

Should this be set, or not? No dead code.

This is left over from the linux driver which has a patch that makes JFFS2 not write to the OOB area.

Quoted 12 lines
Quoted 12 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +11 -0
+	/* use flash based bbt */
+	this->bbt_td = &bbt_main_descr;
+	this->bbt_md = &bbt_mirror_descr;
+
+	/* update flash based bbt */
+	this->options |= NAND_USE_FLASH_BBT;
+
+	if (!this->badblock_pattern)
+		this->badblock_pattern = (mtd->writesize > 512) ?
+		    &largepage_memorybased : &smallpage_memorybased;
+
Quoted 9 lines

drivers/mtd/nand/mpc5121rev2_nand.c line 1021

Quoted 4 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +3 -0
+	/* Build bad block table */
+	return nand_scan_bbt(mtd, this->badblock_pattern);
+}

I'd rather scan_bbt not be abused for this -- we need to fix the NAND probe code so that drivers can do things between nand_scan() and nand_scan_tail().

And until then?

Quoted 3 lines

-Scott

#3 lore raw

PATCH v2 1/1 ADS5121 NAND driver

Dear John,

Quoted 3 lines · In message <[email protected]> you wrote:

The one question I have is if this can go in only supporting 5121 rev2. If I need to add rev1 support it will take more time than I have right now.

I think it is perfectly OK to support only Rev. 2 (and later) of the chip. Rev. 1 is not really in production use anywhere, and the few customers who have early eval bords are probably better off to update theire hardware anyway.

Best regards,

Wolfgang Denk

Signature · 5 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Those who will be able to conquer software will be able  to  conquer
the world."                 - Tadahiro Sekimoto, president, NEC Corp.
#4 lore raw

PATCH v2 1/1 ADS5121 NAND driver

In reply to John Rigby
Quoted 4 lines · John Rigby

Scott, thanks for your feedback. I can easily fix most of the issues.

The one question I have is if this can go in only supporting 5121 rev2. If I need to add rev1 support it will take more time than I have right now.

If rev1 is as dead as Wolfgang says, then I'm fine with leaving it out -- but that fact should be reflected in a comment, rather than the name of the driver. After all, this driver should support rev3 if there is one, right? :-)

Quoted 10 lines
Quoted 9 lines
Quoted 6 lines

+/*
+ * Copyright 2004-2008 Freescale Semiconductor, Inc. All Rights Reserved.
+ *
+ * Based on drivers/mtd/nand/mpc5121_nand.c
+ * which was forked from drivers/mtd/nand/mxc_nd.c

Forking's bad, mmkay?

Yes, I'm guilty. The original was pretty ugly.

If it's anything like the i.MX driver that was posted earlier, I agree entirely. :-)

I was just hoping that we could come up with one new, cleaned-up driver that supports both.

Quoted 13 lines
Quoted 6 lines

drivers/mtd/nand/mpc5121rev2_nand.c line 147

Quoted 3 lines
drivers/mtd/nand/mpc5121rev2_nand.c new file +2 -0
+static struct nand_ecclayout nand_hw_eccoob_4k = {
+	.eccbytes = 64,	/* actually 72 but only room for 64 */

Let's fix that, then.

This is defined in generic mtd code that has exposure to userland.

include/mtd/mtd-abi.h:
/*
* ECC layout control structure. Exported to userspace for
* diagnosis and to allow creation of raw images */

Quoted 6 lines
struct nand_ecclayout {
   uint32_t eccbytes;
   uint32_t eccpos[64];
   uint32_t oobavail;
   struct nand_oobfree oobfree[MTD_MAX_OOBFREE_ENTRIES];
};

Grr... I guess it's OK for now, as the main purpose of eccpos with hardware ECC is to exclude segments from use by other layers.

Very short-sighted API design, though.

Quoted 3 lines
Quoted 2 lines

read_byte should never be called with 16-bit NAND.

This is the replacement for nand_read_byte16 in nand_base.c.

Right, I should have verified my assumption before asserting it. :-P

Not sure why the generic nand_read_byte16 wasn't implemented in terms of the read_word method.

Quoted 17 lines
Quoted 8 lines
Quoted 8 lines

+/*!
+ * This function writes data of length \b len from buffer \b buf to the NAND
+ * internal RAM buffer's MAIN area 0.
+ *
+ * @mtd MTD structure for the NAND Flash
+ * @buf data to be written to NAND Flash
+ * @len number of bytes to be written

Quoted 9 lines

drivers/mtd/nand/mpc5121rev2_nand.c line 552

Quoted 6 lines

+ */

drivers/mtd/nand/mpc5121rev2_nand.c new file +4 -0
+static void mpc5121_nand_write_buf(struct mtd_info *mtd,
+					const u_char *buf, int len)
+{
+	printf("re-work may be needed?\n");

Answer this before we apply the patch. :-)

Quoted 1 line

Perhaps I can just get rid of this whole routine?

I don't think so...

Quoted 3 lines
Quoted 2 lines

Why override these rather than let them go through the command function?

Not sure, I think it is so we can call the copy to/from spare routine.

You can do that in write_buf/read_buf/etc.

Quoted 5 lines
Quoted 4 lines

I'd rather scan_bbt not be abused for this -- we need to fix the NAND probe code so that drivers can do things between nand_scan() and nand_scan_tail().

And until then?

It's OK for now -- that was more of a note to myself to hurry up and fix the NAND probing mechanism. :-)

-Scott

#5 lore raw

PATCH 1/1 ADS5121 NAND driver

In reply to John Rigby

Dear John Rigby,

Quoted 10 lines · In message <[email protected]> you wrote:

ADS5121 rev4 / MPC5121e rev2 only

Only tested with:
2K page size
8 bit device width

This controller treats 2K pages as 4 512 byte pages and the hw ecc is over the combined 512 byte main area and the first 7 bytes of the spare area.

Quoted 6 lines

The hw ecc is stored in the last 9 bytes of the spare area.

This all means the the spare area can not be written separately from the main. This means unmodified JFFS2 will not work.

Hm... then what exactly is this good for? If we cannot use it with the normal Linux code I see little use cases for such a driver?

Best regards,

Wolfgang Denk

Signature · 6 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
I've got to get something inside me. Some coffee  or  something.  And
then the world will somehow be better.
                                     - Terry Pratchett, _Men at Arms_
author
#6 lore raw

PATCH 1/1 ADS5121 NAND driver

Quoted 17 lines · Wolfgang Denk

Dear John Rigby,

Quoted 13 lines · In message <[email protected]> you wrote:

ADS5121 rev4 / MPC5121e rev2 only

Only tested with:
2K page size
8 bit device width

This controller treats 2K pages as 4 512 byte pages and the hw ecc is over the combined 512 byte main area and the first 7 bytes of the spare area.

The hw ecc is stored in the last 9 bytes of the spare area.

Quoted 8 lines
Quoted 4 lines

This all means the the spare area can not be written separately from the main. This means unmodified JFFS2 will not work.

Hm... then what exactly is this good for? If we cannot use it with the normal Linux code I see little use cases for such a driver?

We have gone over this before offline. It works great for UBIFS. It also works with JFFS2 if you teach JFFS2 to leave the OOB area alone, though there have been reports that there are still issues.

My 5121 linux tree has the two patches necessary for JFFS2 to work, but I would recommend using UBIFS.

It is useful in u-boot because you may want to use u-boot to write a UBIFS image to NAND.

Quoted 5 lines

Best regards,

Wolfgang Denk