Skip to contentSkip to conversation

Freescale NFC NAND driver

PATCH Superseded John Rigby 6 participants 9 messages Nov 5, 2008 – Jun 4, 2009

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

John Rigby

author
#0 lore raw

PATCH v3 1/1 Freescale NFC NAND driver

v3: Fixed problem with CFG vs CONFIG_SYS in board/ads5121/ads5121.c

v2: Reworked MPC5121 NAND driver.
Attempted to address all the problems listed by Scott Wood. Driver is now board independent. Will still need more work to be SOC independent.

Driver for the NAND controller on MPC5121.

This driver has been tested on ADS5121 rev4 / MPC5121e rev2 only which has the following configuration:
2K page size
8 bit device width

This should work on other boards with MPC5121 rev2 silicon with little or no change to the driver.

Various vintages of this controller exist on some iMX parts. Getting it to work on an iMX with the same controller version should be fairly easy. More work if it is an iMX with a different version on the controller.

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         |   16 +
 drivers/mtd/nand/Makefile       |    1 +
 drivers/mtd/nand/fsl_nfc_nand.c | 1105 +++++++++++++++++++++++++++++++++++++++
 include/configs/ads5121.h       |   38 ++
 4 files changed, 1160 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mtd/nand/fsl_nfc_nand.c
board/ads5121/ads5121.c +16 -0
index 0610928..bd66e5b 100644
@@ -24,6 +24,7 @@
 #include <common.h>
 #include <mpc512x.h>
 #include <asm/bitops.h>
+#include <asm/io.h>
 #include <command.h>
 #include <asm/processor.h>
 #include <fdt_support.h>

@@ -34,6 +35,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 |				\

@@ -312,3 +314,17 @@ void ft_board_setup(void *blob, bd_t *bd)
 	fdt_fixup_memory(blob, (u64)bd->bi_memstart, (u64)bd->bi_memsize);
 }
 #endif /* defined(CONFIG_OF_LIBFDT) && defined(CONFIG_OF_BOARD_SETUP) */
+
+#if defined(CONFIG_NAND_FSL_NFC)
+void ads5121_fsl_nfc_board_cs(int chip)
+{
+	unsigned char *csreg = (unsigned char *)CONFIG_SYS_CPLD_BASE + 0x09;
+	u8 v;
+
+	v = in_8(csreg);
+	v |= 0xf;
+	v &= ~(1<<chip);
+
+	out_8(csreg, v);
+}
+#endif
drivers/mtd/nand/Makefile +1 -0
index b0abe6e..b010b55 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_FSL_NFC) += fsl_nfc_nand.o
 endif
 
 COBJS	:= $(COBJS-y)
drivers/mtd/nand/fsl_nfc_nand.c new file +1105 -0

1,105 lines not on this page · raw message

include/configs/ads5121.h +38 -0
index 8ec5e9d..d967c2e 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,43 @@
 #undef CONFIG_SYS_FLASH_CHECKSUM
 
 /*
+ * NAND FLASH
+ * drivers/mtd/nand/mpc5121_mpc.c (rev 2 silicon/rev 4 boards only)
+ */
+#define CONFIG_NAND_FSL_NFC
+#ifdef CONFIG_NAND_FSL_NFC
+#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_FSL_NFC_WIDTH 1
+#define CONFIG_FSL_NFC_WRITE_SIZE 2048
+#define CONFIG_FSL_NFC_SPARE_SIZE 64
+#define CONFIG_FSL_NFC_CHIPS 2
+
+#ifndef __ASSEMBLY__
+/*
+ * ADS board as a custom chip select
+ */
+extern void ads5121_fsl_nfc_board_cs(int);
+#define CONFIG_FSL_NFC_BOARD_CS_FUNC ads5121_fsl_nfc_board_cs
+#endif /* __ASSEMBLY__ */
+#endif /* CONFIG_NAND_FSL_NFC */
+
+
+/*
  * 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 v3 1/1 Freescale NFC NAND driver

Quoted 43 lines · John Rigby

From: John Rigby <[email protected]>
Subject: [U-Boot] [PATCH v3] Freescale NFC NAND driver
To: u-boot at lists.denx.de, "Scott Wood" <[email protected]>
Cc: "John Rigby" <[email protected]>
Date: Tuesday, November 4, 2008, 11:02 PM
v3: Fixed problem with CFG vs CONFIG_SYS in board/ads5121/ads5121.c

v2: Reworked MPC5121 NAND driver.
Attempted to address all the problems listed by Scott Wood. Driver is now board independent. Will still need more work to be SOC independent.

Driver for the NAND controller on MPC5121.

This driver has been tested on ADS5121 rev4 / MPC5121e rev2 only
which has the following configuration:
2K page size
8 bit device width

This should work on other boards with MPC5121 rev2 silicon with
little or no change to the driver.

Various vintages of this controller exist on some iMX parts.
Getting it to work on an iMX with the same controller version
should be fairly easy. More work if it is an iMX with a different
version on the controller.

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.

...

Quoted 9 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +9 -0
+#define NFC_BUF_ADDR			(NFC_REG_BASE + 0x1E04)
+#define NFC_FLASH_ADDR			(NFC_REG_BASE + 0x1E06)
+#define NFC_FLASH_CMD			(NFC_REG_BASE + 0x1E08)
+#define NFC_CONFIG			(NFC_REG_BASE + 0x1E0A)
+#define NFC_ECC_STATUS1			(NFC_REG_BASE + 0x1E0C)
+#define NFC_ECC_STATUS2			(NFC_REG_BASE + 0x1E0E)
+#define NFC_SPAS			(NFC_REG_BASE + 0x1E10)
+#define NFC_WRPROT			(NFC_REG_BASE + 0x1E12)
+#define NFC_NF_WRPRST			(NFC_REG_BASE + 0x1E18)

drivers/mtd/nand/fsl_nfc_nand.c line 66

Quoted 10 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +10 -0
+#define NFC_CONFIG1			(NFC_REG_BASE + 0x1E1A)
+#define NFC_CONFIG2			(NFC_REG_BASE + 0x1E1C)
+#define NFC_UNLOCKSTART_BLKADDR0	(NFC_REG_BASE + 0x1E20)
+#define NFC_UNLOCKEND_BLKADDR0		(NFC_REG_BASE + 0x1E22)
+#define NFC_UNLOCKSTART_BLKADDR1	(NFC_REG_BASE + 0x1E24)
+#define NFC_UNLOCKEND_BLKADDR1		(NFC_REG_BASE + 0x1E26)
+#define NFC_UNLOCKSTART_BLKADDR2	(NFC_REG_BASE + 0x1E28)
+#define NFC_UNLOCKEND_BLKADDR2		(NFC_REG_BASE + 0x1E2A)
+#define NFC_UNLOCKSTART_BLKADDR3	(NFC_REG_BASE + 0x1E2C)
+#define NFC_UNLOCKEND_BLKADDR3		(NFC_REG_BASE + 0x1E2E)

On MX31 and also according to the current MPC5121 Reference Manual on the web the offsets of the registers above seem to have an extra offset of 0x1000.

MX31 and MPC5121 manuals state the following offsets:

#define NFC_BUF_ADDR (NFC_REG_BASE + 0xE04)
#define NFC_FLASH_ADDR (NFC_REG_BASE + 0xE06)
...

Is there a newer MPC5121 manual that changed the NAND registers offsets?

Regards,

Fabio Estevam

author
#2 lore raw

PATCH v3 1/1 Freescale NFC NAND driver

Quoted 24 lines · Fabio Estevam
Quoted 11 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +9 -0
+#define NFC_BUF_ADDR			(NFC_REG_BASE + 0x1E04)
+#define NFC_FLASH_ADDR			(NFC_REG_BASE + 0x1E06)
+#define NFC_FLASH_CMD			(NFC_REG_BASE + 0x1E08)
+#define NFC_CONFIG			(NFC_REG_BASE + 0x1E0A)
+#define NFC_ECC_STATUS1			(NFC_REG_BASE + 0x1E0C)
+#define NFC_ECC_STATUS2			(NFC_REG_BASE + 0x1E0E)
+#define NFC_SPAS			(NFC_REG_BASE + 0x1E10)
+#define NFC_WRPROT			(NFC_REG_BASE + 0x1E12)
+#define NFC_NF_WRPRST			(NFC_REG_BASE + 0x1E18)

drivers/mtd/nand/fsl_nfc_nand.c line 66

Quoted 10 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +10 -0
+#define NFC_CONFIG1			(NFC_REG_BASE + 0x1E1A)
+#define NFC_CONFIG2			(NFC_REG_BASE + 0x1E1C)
+#define NFC_UNLOCKSTART_BLKADDR0	(NFC_REG_BASE + 0x1E20)
+#define NFC_UNLOCKEND_BLKADDR0		(NFC_REG_BASE + 0x1E22)
+#define NFC_UNLOCKSTART_BLKADDR1	(NFC_REG_BASE + 0x1E24)
+#define NFC_UNLOCKEND_BLKADDR1		(NFC_REG_BASE + 0x1E26)
+#define NFC_UNLOCKSTART_BLKADDR2	(NFC_REG_BASE + 0x1E28)
+#define NFC_UNLOCKEND_BLKADDR2		(NFC_REG_BASE + 0x1E2A)
+#define NFC_UNLOCKSTART_BLKADDR3	(NFC_REG_BASE + 0x1E2C)
+#define NFC_UNLOCKEND_BLKADDR3		(NFC_REG_BASE + 0x1E2E)

On MX31 and also according to the current MPC5121 Reference Manual on the web the offsets of the registers above seem to have an extra offset of 0x1000.

Quoted 8 lines

MX31 and MPC5121 manuals state the following offsets:

#define NFC_BUF_ADDR (NFC_REG_BASE + 0xE04)
#define NFC_FLASH_ADDR (NFC_REG_BASE + 0xE06)
...

Is there a newer MPC5121 manual that changed the NAND registers offsets?

As the patch states this is for silicon rev2. One of the changes with rev2 was support for 4k nand so the number of 512 byte buffers is now 8 instead of 4 so everything else has moved up by 2K (0x1000).

The rev2 manual is available online here:
http://www.freescale.com/files/32bit/doc/ref_manual/MPC5121ERM.pdf

Quoted 8 lines

Regards,

Fabio Estevam

#3 lore raw

PATCH v3 1/1 Freescale NFC NAND driver

In reply to John Rigby
Quoted 1 line · John Rigby

+#define MIN(x, y) ((x < y) ? x : y)

Please use the min() macro defined in include/common.h.

drivers/mtd/nand/fsl_nfc_nand.c line 38

Quoted 10 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +10 -0
+static struct fsl_nfc_private {
+	struct mtd_info mtd;
+	char spare_only;
+	char status_req;
+	u16 col_addr;
+	int writesize;
+	int sparesize;
+	int width;
+	int chipsel;
+} *priv;

Is it plausable that there will ever be a chip with more than one of these controllers?

drivers/mtd/nand/fsl_nfc_nand.c line 144

Quoted 12 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +12 -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,
+	},
+	.oobfree = {
+		{0, 5} /* byte 5 is factory bad block marker */
+	},
+};

Is byte 6 free?

Quoted 22 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +22 -0
+static struct nand_ecclayout nand_hw_eccoob_4k_218_spare = {
+	.eccbytes = 64,	/* actually 144 but only room for 64 */
+	.eccpos = {
+		/* 18 bytes of ecc for each 512 bytes of data */
+		7, 8, 9, 10, 11, 12, 13, 14, 15,
+		    16, 17, 18, 19, 20, 21, 22, 23, 24,
+		33, 34, 35, 36, 37, 38, 39, 40, 41,
+		    42, 43, 44, 45, 46, 47, 48, 49, 50,
+		59, 60, 61, 62, 63, 64, 65, 66, 67,
+		    68, 69, 70, 71, 72, 73, 74, 75, 76,
+		85, 86, 87, 88, 89, 90, 91, 92, 93,
+		    94, /* 95, 96, 97, 98, 99, 100, 101, 102,
+		111, 112, 113, 114, 115, 116, 117, 118, 119,
+		    120, 121, 122, 123, 124, 125, 126, 127, 128,
+		137, 138, 139, 140, 141, 142, 143, 144, 145,
+		    146, 147, 148, 149, 150, 151, 152, 153, 154,
+		163, 164, 165, 166, 167, 168, 169, 170, 171,
+		    172, 173, 174, 175, 176, 177, 178, 179, 180,
+		189, 190, 191, 192, 193, 194, 195, 196, 197,
+		    198, 199, 200, 201, 202, 203, 204, 205, 206, */
+	},
+	.oobfree = {

drivers/mtd/nand/fsl_nfc_nand.c line 219

Quoted 10 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +10 -0
+		{2, 5}, /* bytes 0 and 1 are factory bad block markers */
+		{25, 8},
+		{51, 8},
+		{77, 8},
+		{103, 8},
+		{129, 8},
+		{155, 8},
+		{181, 8},
+	},
+};

Need to change NAND_MAX_OOBSIZE.

drivers/mtd/nand/fsl_nfc_nand.c line 279

Quoted 9 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +9 -0
+/*!
+ * This function polls the NFC to wait for the basic operation to complete by
+ * checking the INT bit of config2 register.
+ *
+ * @max_retries	number of retry attempts (separated by 1 us)
+ */
+static void wait_op_done(int max_retries)
+{
+

No blank line here.

Review

Quoted 2 lines
(quoted diff) +2 -0
+	}
+	if (max_retries <= 0)

Blank line between the last two.

drivers/mtd/nand/fsl_nfc_nand.c line 451

Quoted 5 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +5 -0
+static void fsl_nfc_enable_hwecc(struct mtd_info *mtd, int mode)
+{
+	NFC_WRITEW(NFC_CONFIG1, (NFC_READW(NFC_CONFIG1) | NFC_ECC_EN));
+	return;
+}

Unnecessary "return" keyword.

Quoted 9 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +9 -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 fsl_nfc_write_buf(struct mtd_info *mtd,

drivers/mtd/nand/fsl_nfc_nand.c line 571

Quoted 10 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +10 -0
+					const u_char *buf, int len)
+{
+	if (priv->col_addr >= mtd->writesize || priv->spare_only) {
+		copy_to_spare(mtd, (char *)buf, len);
+		return;
+	} else {
+		priv->col_addr += len;
+		memcpy_toio(MAIN_AREA(0), (void *)buf, len);
+	}
+}

What if both main and spare areas are written at once?

Also, rewrite

if (...) {
	foo;
	return;
} else {
	bar;
}

as:

if (...) {
	foo;
	return;
}

bar;

or

if (...)

	foo;
else
	bar;
Quoted 11 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +11 -0
+#ifndef CONFIG_FSL_NFC_BOARD_CS_FUNC
+static void fsl_nfc_select_chip(u8 cs)
+{
+	u32 val = NFC_READW(NFC_BUF_ADDR);
+
+	val &= ~0x60;
+	val |= cs << 5;
+	NFC_WRITEW(NFC_BUF_ADDR, val);
+}
+#define CONFIG_FSL_NFC_BOARD_CS_FUNC fsl_nfc_select_chip
+#endif

drivers/mtd/nand/fsl_nfc_nand.c line 730

Quoted 10 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +10 -0
+
+
+/*!
+ * This function is used by upper layer for select and deselect of the NAND
+ * chip
+ *
+ * @mtd		MTD structure for the NAND Flash
+ * @chip	val indicating select or deselect
+ */
+static void fsl_nfc_select_chip(struct mtd_info *mtd, int chip)

This looks like a compilation error if CONFIG_FSL_NFC_BOARD_CS_FUNC is not defined.

Quoted 6 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +6 -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);

drivers/mtd/nand/fsl_nfc_nand.c line 800

Quoted 10 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +10 -0
+
+	fsl_nfc_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);
+	}
+}

Factor out the send_read_page(0);

drivers/mtd/nand/fsl_nfc_nand.c line 841

Quoted 5 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +5 -0
+	case NAND_CMD_READOOB:
+		priv->col_addr = column;
+		priv->spare_only = 1;
+		command = NAND_CMD_READ0;	/* only READ0 is valid */
+		break;

What about small-page flash that takes an actual READOOB command?

Quoted 24 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +24 -0
+static int fsl_nfc_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 fsl_nfc_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);
+	fsl_nfc_do_addr_cycle(mtd, read_oob_col, page);
+
+	/* copy the oob data */
+	copy_to_spare(mtd, chip->oob_poi, mtd->oobsize);

drivers/mtd/nand/fsl_nfc_nand.c line 930

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

Again, I'm fairly sure you don't need these if the other functions are defined properly.

Quoted 13 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +13 -0
+/*
+ * These are identical to the generic versions except
+ * for the offsets.
+ */
+static struct nand_bbt_descr bbt_main_descr = {
+	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
+	    | NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
+	.offs = 0,
+	.len = 4,
+	.veroffs = 4,
+	.maxblocks = 4,
+	.pattern = bbt_pattern
+};

drivers/mtd/nand/fsl_nfc_nand.c line 988

Quoted 10 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +10 -0
+
+static struct nand_bbt_descr bbt_mirror_descr = {
+	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
+	    | NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
+	.offs = 0,
+	.len = 4,
+	.veroffs = 4,
+	.maxblocks = 4,
+	.pattern = mirror_pattern
+};

This will overlap the bad block marker on large-page flash.

-Scott

#4 lore raw

PATCH v3 1/1 Freescale NFC NAND driver

Hi Scott,

I'll try to continue with this patch so that we can integrate it hopefully soon. I already addressed some of your comments (the easy ones ;)). Please find some further questions below (I'm still new to the FSL NFC):

Quoted 5 lines · On Thursday 06 November 2008 00:06:48 Scott Wood wrote:
Quoted 5 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +5 -0
+static struct fsl_nfc_private {
+	struct mtd_info mtd;
+	char spare_only;
+	char status_req;
+	u16 col_addr;
Quoted 8 lines

drivers/mtd/nand/fsl_nfc_nand.c line 38

Quoted 5 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +5 -0
+	int writesize;
+	int sparesize;
+	int width;
+	int chipsel;
+} *priv;

Is it plausable that there will ever be a chip with more than one of these controllers?

I have no idea. What do you suggest I should change here?

Quoted 6 lines
Quoted 6 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +6 -0
+/*
+ * OOB placement block for use with hardware ecc generation
+ */
+static struct nand_ecclayout nand_hw_eccoob_512 = {
+	.eccbytes = 9,
+	.eccpos = {
Quoted 8 lines

drivers/mtd/nand/fsl_nfc_nand.c line 144

Quoted 6 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +6 -0
+		7, 8, 9, 10, 11, 12, 13, 14, 15,
+	},
+	.oobfree = {
+		{0, 5} /* byte 5 is factory bad block marker */
+	},
+};

Is byte 6 free?

Looks this way. I'll add it to the free area. John, please shout if you think this is not correct.

Quoted 26 lines
Quoted 26 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +26 -0
+static struct nand_ecclayout nand_hw_eccoob_4k_218_spare = {
+	.eccbytes = 64,	/* actually 144 but only room for 64 */
+	.eccpos = {
+		/* 18 bytes of ecc for each 512 bytes of data */
+		7, 8, 9, 10, 11, 12, 13, 14, 15,
+		    16, 17, 18, 19, 20, 21, 22, 23, 24,
+		33, 34, 35, 36, 37, 38, 39, 40, 41,
+		    42, 43, 44, 45, 46, 47, 48, 49, 50,
+		59, 60, 61, 62, 63, 64, 65, 66, 67,
+		    68, 69, 70, 71, 72, 73, 74, 75, 76,
+		85, 86, 87, 88, 89, 90, 91, 92, 93,
+		    94, /* 95, 96, 97, 98, 99, 100, 101, 102,
+		111, 112, 113, 114, 115, 116, 117, 118, 119,
+		    120, 121, 122, 123, 124, 125, 126, 127, 128,
+		137, 138, 139, 140, 141, 142, 143, 144, 145,
+		    146, 147, 148, 149, 150, 151, 152, 153, 154,
+		163, 164, 165, 166, 167, 168, 169, 170, 171,
+		    172, 173, 174, 175, 176, 177, 178, 179, 180,
+		189, 190, 191, 192, 193, 194, 195, 196, 197,
+		    198, 199, 200, 201, 202, 203, 204, 205, 206, */
+	},
+	.oobfree = {
+		{2, 5}, /* bytes 0 and 1 are factory bad block markers */
+		{25, 8},
+		{51, 8},
+		{77, 8},
Quoted 8 lines

drivers/mtd/nand/fsl_nfc_nand.c line 219

Quoted 6 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +6 -0
+		{103, 8},
+		{129, 8},
+		{155, 8},
+		{181, 8},
+	},
+};

Need to change NAND_MAX_OOBSIZE.

I'll send a patch for this shortly.

Quoted 16 lines
Quoted 16 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +15 -0
+#ifndef CONFIG_FSL_NFC_BOARD_CS_FUNC
+static void fsl_nfc_select_chip(u8 cs)
+{
+	u32 val = NFC_READW(NFC_BUF_ADDR);
+
+	val &= ~0x60;
+	val |= cs << 5;
+	NFC_WRITEW(NFC_BUF_ADDR, val);
+}
+#define CONFIG_FSL_NFC_BOARD_CS_FUNC fsl_nfc_select_chip
+#endif
+
+
+/*!
+ * This function is used by upper layer for select and deselect of the

NAND + * chip

Quoted 8 lines
Quoted 5 lines

+ *
+ * @mtd MTD structure for the NAND Flash
+ * @chip val indicating select or deselect
+ */
+static void fsl_nfc_select_chip(struct mtd_info *mtd, int chip)

This looks like a compilation error if CONFIG_FSL_NFC_BOARD_CS_FUNC is not defined.

Works fine here on a board without CONFIG_FSL_NFC_BOARD_CS_FUNC defined. So I'll leave it as is.

Quoted 7 lines

drivers/mtd/nand/fsl_nfc_nand.c line 841

Quoted 5 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +5 -0
+	case NAND_CMD_READOOB:
+		priv->col_addr = column;
+		priv->spare_only = 1;
+		command = NAND_CMD_READ0;	/* only READ0 is valid */
+		break;

What about small-page flash that takes an actual READOOB command?

I don't have access to a board with small-page NAND. So I can't test anything here.

Quoted 29 lines
Quoted 29 lines

+static int fsl_nfc_read_oob(struct mtd_info *mtd, struct nand_chip
*chip, + int page, int sndcmd)

drivers/mtd/nand/fsl_nfc_nand.c new file +11 -0
+{
+	if (sndcmd) {
+		read_full_page(mtd, page);
+		sndcmd = 0;
+	}
+
+	copy_from_spare(mtd, chip->oob_poi, mtd->oobsize);
+	return sndcmd;
+}
+
+static int fsl_nfc_write_oob(struct mtd_info *mtd, struct nand_chip

*chip, + int page)

drivers/mtd/nand/fsl_nfc_nand.c new file +15 -0
+{
+	int status = 0;
+	int read_oob_col = 0;
+
+	send_cmd(NAND_CMD_READ0);
+	send_cmd(NAND_CMD_SEQIN);
+	fsl_nfc_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 8 lines

drivers/mtd/nand/fsl_nfc_nand.c line 930

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

Again, I'm fairly sure you don't need these if the other functions are defined properly.

OK, I removed these functions and tested a bit on my MPC5121 board. Everything seems to be working fine without it.

Again, John please let me know if you think these functions are really necessary.

Quoted 17 lines
Quoted 17 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +17 -0
+/*
+ * These are identical to the generic versions except
+ * for the offsets.
+ */
+static struct nand_bbt_descr bbt_main_descr = {
+	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
+	    | NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
+	.offs = 0,
+	.len = 4,
+	.veroffs = 4,
+	.maxblocks = 4,
+	.pattern = bbt_pattern
+};
+
+static struct nand_bbt_descr bbt_mirror_descr = {
+	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
+	    | NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
Quoted 8 lines

drivers/mtd/nand/fsl_nfc_nand.c line 988

Quoted 6 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +6 -0
+	.offs = 0,
+	.len = 4,
+	.veroffs = 4,
+	.maxblocks = 4,
+	.pattern = mirror_pattern
+};

This will overlap the bad block marker on large-page flash.

Good catch. Do you have any idea how can this be solved?

Thanks.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de
=====================================================================

#5 lore raw

PATCH v3 1/1 Freescale NFC NAND driver

Stefan,

My only concern is that the u-boot and linux nand drivers need to have the same approach regarding ecc. The linux driver recently submitted only supports sw ecc because using hw ecc means the spare area is not writeable. The u-boot driver that I submitted supported hw_ecc only and was compatible with the linux driver in ltib. Unusable spare is an issue for JFFS2 but not UBIFS. If I were to make the decision I would just say not to JFFS2 on NAND. UBIFS is a far better solution for NAND anyway.

I have seen at least one other controller where the controller included the spare in the ECC so I don't know if this is a trend or not.

John

Quoted 203 lines · Stefan Roese

Hi Scott,

I'll try to continue with this patch so that we can integrate it hopefully soon. I already addressed some of your comments (the easy ones ;)). Please find some further questions below (I'm still new to the FSL NFC):

Quoted 5 lines · On Thursday 06 November 2008 00:06:48 Scott Wood wrote:
Quoted 5 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +5 -0
+static struct fsl_nfc_private {
+   struct mtd_info mtd;
+   char spare_only;
+   char status_req;
+   u16 col_addr;
Quoted 8 lines

drivers/mtd/nand/fsl_nfc_nand.c line 38

Quoted 5 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +5 -0
+   int writesize;
+   int sparesize;
+   int width;
+   int chipsel;
+} *priv;

Is it plausable that there will ever be a chip with more than one of these controllers?

I have no idea. What do you suggest I should change here?

Quoted 6 lines
Quoted 6 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +6 -0
+/*
+ * OOB placement block for use with hardware ecc generation
+ */
+static struct nand_ecclayout nand_hw_eccoob_512 = {
+   .eccbytes = 9,
+   .eccpos = {
Quoted 8 lines

drivers/mtd/nand/fsl_nfc_nand.c line 144

Quoted 6 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +6 -0
+           7, 8, 9, 10, 11, 12, 13, 14, 15,
+   },
+   .oobfree = {
+           {0, 5} /* byte 5 is factory bad block marker */
+   },
+};

Is byte 6 free?

Looks this way. I'll add it to the free area. John, please shout if you think
this is not correct.

Quoted 26 lines
Quoted 26 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +26 -0
+static struct nand_ecclayout nand_hw_eccoob_4k_218_spare = {
+   .eccbytes = 64, /* actually 144 but only room for 64 */
+   .eccpos = {
+           /* 18 bytes of ecc for each 512 bytes of data */
+           7, 8, 9, 10, 11, 12, 13, 14, 15,
+               16, 17, 18, 19, 20, 21, 22, 23, 24,
+           33, 34, 35, 36, 37, 38, 39, 40, 41,
+               42, 43, 44, 45, 46, 47, 48, 49, 50,
+           59, 60, 61, 62, 63, 64, 65, 66, 67,
+               68, 69, 70, 71, 72, 73, 74, 75, 76,
+           85, 86, 87, 88, 89, 90, 91, 92, 93,
+               94, /* 95, 96, 97, 98, 99, 100, 101, 102,
+           111, 112, 113, 114, 115, 116, 117, 118, 119,
+               120, 121, 122, 123, 124, 125, 126, 127, 128,
+           137, 138, 139, 140, 141, 142, 143, 144, 145,
+               146, 147, 148, 149, 150, 151, 152, 153, 154,
+           163, 164, 165, 166, 167, 168, 169, 170, 171,
+               172, 173, 174, 175, 176, 177, 178, 179, 180,
+           189, 190, 191, 192, 193, 194, 195, 196, 197,
+               198, 199, 200, 201, 202, 203, 204, 205, 206, */
+   },
+   .oobfree = {
+           {2, 5}, /* bytes 0 and 1 are factory bad block markers */
+           {25, 8},
+           {51, 8},
+           {77, 8},
Quoted 8 lines

drivers/mtd/nand/fsl_nfc_nand.c line 219

Quoted 6 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +6 -0
+           {103, 8},
+           {129, 8},
+           {155, 8},
+           {181, 8},
+   },
+};

Need to change NAND_MAX_OOBSIZE.

I'll send a patch for this shortly.

Quoted 16 lines
Quoted 16 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +15 -0
+#ifndef CONFIG_FSL_NFC_BOARD_CS_FUNC
+static void fsl_nfc_select_chip(u8 cs)
+{
+   u32 val = NFC_READW(NFC_BUF_ADDR);
+
+   val &= ~0x60;
+   val |= cs << 5;
+   NFC_WRITEW(NFC_BUF_ADDR, val);
+}
+#define CONFIG_FSL_NFC_BOARD_CS_FUNC fsl_nfc_select_chip
+#endif
+
+
+/*!
+ * This function is used by upper layer for select and deselect of the

NAND + * chip

Quoted 8 lines
Quoted 5 lines

+ *
+ * @mtd MTD structure for the NAND Flash
+ * @chip val indicating select or deselect
+ */
+static void fsl_nfc_select_chip(struct mtd_info *mtd, int chip)

This looks like a compilation error if CONFIG_FSL_NFC_BOARD_CS_FUNC is not defined.

Works fine here on a board without CONFIG_FSL_NFC_BOARD_CS_FUNC defined. So I'll leave it as is.

Quoted 7 lines

drivers/mtd/nand/fsl_nfc_nand.c line 841

Quoted 5 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +5 -0
+   case NAND_CMD_READOOB:
+           priv->col_addr = column;
+           priv->spare_only = 1;
+           command = NAND_CMD_READ0;       /* only READ0 is valid */
+           break;

What about small-page flash that takes an actual READOOB command?

I don't have access to a board with small-page NAND. So I can't test anything
here.

Quoted 29 lines
Quoted 29 lines

+static int fsl_nfc_read_oob(struct mtd_info *mtd, struct nand_chip
*chip, + int page, int sndcmd)

drivers/mtd/nand/fsl_nfc_nand.c new file +11 -0
+{
+   if (sndcmd) {
+           read_full_page(mtd, page);
+           sndcmd = 0;
+   }
+
+   copy_from_spare(mtd, chip->oob_poi, mtd->oobsize);
+   return sndcmd;
+}
+
+static int fsl_nfc_write_oob(struct mtd_info *mtd, struct nand_chip

*chip, + int page)

drivers/mtd/nand/fsl_nfc_nand.c new file +15 -0
+{
+   int status = 0;
+   int read_oob_col = 0;
+
+   send_cmd(NAND_CMD_READ0);
+   send_cmd(NAND_CMD_SEQIN);
+   fsl_nfc_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 8 lines

drivers/mtd/nand/fsl_nfc_nand.c line 930

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

Again, I'm fairly sure you don't need these if the other functions are defined properly.

OK, I removed these functions and tested a bit on my MPC5121 board. Everything
seems to be working fine without it.

Again, John please let me know if you think these functions are really necessary.

Quoted 17 lines
Quoted 17 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +17 -0
+/*
+ * These are identical to the generic versions except
+ * for the offsets.
+ */
+static struct nand_bbt_descr bbt_main_descr = {
+   .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
+       | NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
+   .offs = 0,
+   .len = 4,
+   .veroffs = 4,
+   .maxblocks = 4,
+   .pattern = bbt_pattern
+};
+
+static struct nand_bbt_descr bbt_mirror_descr = {
+   .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
+       | NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
Quoted 8 lines

drivers/mtd/nand/fsl_nfc_nand.c line 988

Quoted 6 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +6 -0
+   .offs = 0,
+   .len = 4,
+   .veroffs = 4,
+   .maxblocks = 4,
+   .pattern = mirror_pattern
+};

This will overlap the bad block marker on large-page flash.

Good catch. Do you have any idea how can this be solved?

Thanks.

Best regards,
Stefan

=====================================================================
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de
=====================================================================

#6 lore raw

PATCH v3 1/1 Freescale NFC NAND driver

Quoted 7 lines · John Rigby

My only concern is that the u-boot and linux nand drivers need to have the same approach regarding ecc. The linux driver recently submitted only supports sw ecc because using hw ecc means the spare area is not writeable. The u-boot driver that I submitted supported hw_ecc only and was compatible with the linux driver in ltib. Unusable spare is an issue for JFFS2 but not UBIFS. If I were to make the decision I would just say not to JFFS2 on NAND. UBIFS is a far better solution for NAND anyway.

How much of a performance hit is the sw ecc? We should at least support it as an option, and probably by default if that's what Linux does.

Quoted 2 lines

I have seen at least one other controller where the controller included the spare in the ECC so I don't know if this is a trend or not.

I hope not. :-(

Quoted 7 lines · Stefan Roese
Quoted 6 lines

Hi Scott,

I'll try to continue with this patch so that we can integrate it hopefully soon. I already addressed some of your comments (the easy ones ;)). Please find some further questions below (I'm still new to the FSL NFC):

Thanks!

Quoted 13 lines
Quoted 13 lines · On Thursday 06 November 2008 00:06:48 Scott Wood wrote:
Quoted 12 lines

drivers/mtd/nand/fsl_nfc_nand.c line 38

Quoted 10 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +10 -0
+static struct fsl_nfc_private {
+   struct mtd_info mtd;
+   char spare_only;
+   char status_req;
+   u16 col_addr;
+   int writesize;
+   int sparesize;
+   int width;
+   int chipsel;
+} *priv;

Is it plausable that there will ever be a chip with more than one of these controllers?

Quoted 1 line
Quoted 1 line

I have no idea. What do you suggest I should change here?

Remove the "*priv" at the end and use mtd->priv instead.

Quoted 23 lines
Quoted 23 lines
Quoted 23 lines
Quoted 15 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +15 -0
+#ifndef CONFIG_FSL_NFC_BOARD_CS_FUNC
+static void fsl_nfc_select_chip(u8 cs)
+{
+   u32 val = NFC_READW(NFC_BUF_ADDR);
+
+   val &= ~0x60;
+   val |= cs << 5;
+   NFC_WRITEW(NFC_BUF_ADDR, val);
+}
+#define CONFIG_FSL_NFC_BOARD_CS_FUNC fsl_nfc_select_chip
+#endif
+
+
+/*!
+ * This function is used by upper layer for select and deselect of the
Quoted 6 lines

NAND + * chip
+ *
+ * @mtd MTD structure for the NAND Flash
+ * @chip val indicating select or deselect
+ */
+static void fsl_nfc_select_chip(struct mtd_info *mtd, int chip)

This looks like a compilation error if CONFIG_FSL_NFC_BOARD_CS_FUNC is not defined.

Quoted 2 lines
Quoted 2 lines

Works fine here on a board without CONFIG_FSL_NFC_BOARD_CS_FUNC defined. So I'll leave it as is.

But there are two definitions of fsl_nfc_select_chip in that case... or am I missing something?

Quoted 9 lines
Quoted 9 lines
Quoted 6 lines

drivers/mtd/nand/fsl_nfc_nand.c line 841

Quoted 5 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +5 -0
+   case NAND_CMD_READOOB:
+           priv->col_addr = column;
+           priv->spare_only = 1;
+           command = NAND_CMD_READ0;       /* only READ0 is valid */
+           break;

What about small-page flash that takes an actual READOOB command?

I don't have access to a board with small-page NAND. So I can't test anything
here.

Still, better to have code that might work than code that will definitely break. :-)

Alternatively, make it obvious that the driver does not support small-page.

Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
Quoted 13 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +13 -0
+/*
+ * These are identical to the generic versions except
+ * for the offsets.
+ */
+static struct nand_bbt_descr bbt_main_descr = {
+   .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
+       | NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
+   .offs = 0,
+   .len = 4,
+   .veroffs = 4,
+   .maxblocks = 4,
+   .pattern = bbt_pattern
+};

drivers/mtd/nand/fsl_nfc_nand.c line 988

Quoted 10 lines
drivers/mtd/nand/fsl_nfc_nand.c new file +10 -0
+
+static struct nand_bbt_descr bbt_mirror_descr = {
+   .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
+       | NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
+   .offs = 0,
+   .len = 4,
+   .veroffs = 4,
+   .maxblocks = 4,
+   .pattern = mirror_pattern
+};

This will overlap the bad block marker on large-page flash.

Quoted 1 line
Quoted 1 line

Good catch. Do you have any idea how can this be solved?

Change the offset. :-)

Perhaps with different offsets for small and large page.

-Scott

#7 lore raw

PATCH v3 1/1 Freescale NFC NAND driver

In reply to John Rigby

Dear John Rigby,

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

v3: Fixed problem with CFG vs CONFIG_SYS in board/ads5121/ads5121.c

v2: Reworked MPC5121 NAND driver.
Attempted to address all the problems listed by Scott Wood. Driver is now board independent. Will still need more work to be SOC independent.

Driver for the NAND controller on MPC5121.

I am still waiting for some pull request (including this patch and the other 512x specific updates).

Could you please comment ?

Best regards,

Wolfgang Denk

Signature · 4 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
Forty two.
#8 lore raw

PATCH v3 1/1 Freescale NFC NAND driver

Quoted 10 lines · Wolfgang Denk

Dear John Rigby,

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

v3: Fixed problem with CFG vs CONFIG_SYS in board/ads5121/ads5121.c

v2: Reworked MPC5121 NAND driver.
Attempted to address all the problems listed by Scott Wood. Driver is now board independent. Will still need more work to be SOC independent.

Quoted 6 lines
Quoted 1 line

Driver for the NAND controller on MPC5121.

I am still waiting for some pull request (including this patch and the other 512x specific updates).

Could you please comment ?

I did comment:
http://lists.denx.de/pipermail/u-boot/2008-November/043065.html

-Scott