VF610+ColdFireM54418 controller.

VF610+ColdFireM54418 controller.

[Bill Pringlemeir]

There are some mtd drivers for this NAND flash controller on the web.

Eg,
 https://dev.openwrt.org/browser/trunk/target/linux/coldfire/patches/016-Add-nand-driver-support-for-M54418TWR-board.patch?rev=31546
 https://github.com/Timesys/linux-timesys/blob/3.0-mvf/drivers/mtd/nand/fsl_nfc.c

The device has 9K SRAM for main and spare areas.  The register layout
is,

  off name      desc
   00 NFC_CMD1    Flash command 1
   04 NFC_CMD2    Flash command 2
   08 NFC_CAR     Column address
   0C NFC_RAR     Row address 
   10 NFC_RPT     Flash command repeat
   14 NFC_RAI     Row address increment
   18 NFC_SR1     Flash status 1 *read only*
   1C NFC_SR2     Flash status 2 *read only*
   20 NFC_DMA_CH1 DMA channel 1 address
   24 NFC_DMACFG  DMA configuration
   28 NFC_SWAP    Cach swap
   2C NFC_SECSZ   Sector size
   30 NFC_CFG     Flash configuration
   34 NFC_DMA_CH2 DMA channel 2 address
   38 NFC_ISR     Interrupt status

All registers are 32bit R/W unless noted, from section 31.3 of the
Vybrid NAND chapter.

Is anyone working on support for this chip set?  
Is there an existing driver that can be adapted?  
Is the 'fsl_nfc' name appropriate?  If not, what name?
Is there any reason an updated driver won't be considered for the
mainline?

Thanks,
Bill Pringlemeir.

Re: VF610+ColdFireM54418 controller.

[Bill Pringlemeir]

On 21 Nov 2013, bpringlemeir@nbsps.com wrote:

> All registers are 32bit R/W unless noted, from section 31.3 of the
> Vybrid NAND chapter.

> Is there an existing driver that can be adapted?  

It looks like the mpc5125 SOC has the same NAND controller; the CPU/SOC
has support in the tree, but no NAND controller in mpc5125twr.dts.  The
registers and buffers differ from the mpc5121 NAND controller which is
in the tree; the difference between the mpc5125+vf610+ColdFireM54418
NAND controller and the mpc5121 NAND controller look significant to me.

Fwiw,
Bill Pringlemeir.

[RFC 0/5] Nand

[Bill Pringlemeir]

This is an update of a driver originally by Shaohui Xie <b21989@freescale.com>
and then updated by Jason Jin <Jason.jin@freescale.com>.  It is used and tested
on the Vybrid Tower system (VF610-TWR).  However, the 'fsl_nfc' is present on
many Freescale SOC devices.  Such as the MPC5125 (PowerPC), MCF54418 (ColdFire)
and the Kinetis K70 (ARM Cortex-M).

The code in this patch is about 4x faster than other versions on the Internet,
at least with the Vybrid SOC and gcc-4.8.  Currently the MTD OOB test fails with
or without hardware ECC.  All of the other tests print 'success'.

The current 'device tree' bindings for the 'vf610-twr' will testing of the entire
flash device.  A command line option such as,

 mtdparts=fsl_nfc:256k(loader),8M(boot0),8M(boot1),-(root)

can be used to partion the existing NAND flash or the 'vf610-twr.dts' can be 
modified to add some similar partitions. 

cumentation/devicetree/bindings/mtd/fsl-nfc.txt |  25 ++++
 arch/arm/boot/dts/vf610-twr.dts                   |  41 +++++++
 arch/arm/boot/dts/vf610.dtsi                      |   9 ++
 arch/arm/mach-imx/Kconfig                         |   1 +
 drivers/mtd/nand/Kconfig                          |  13 +++
 drivers/mtd/nand/Makefile                         |   1 +
 drivers/mtd/nand/fsl_nfc.c                        | 790 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 7 files changed, 880 insertions(+)

[RFC 1/5] mtd:fsl_nfc: Nand flash controller for VF610, MPC5125, etc.

[Bill Pringlemeir]

Limitations:
- Untested on MPC5125, MCF54418 and Kinetis K70.
- DMA not used.
- 2K pages or less.
- No hardware ECC.

Signed-off-by: Bill Pringlemeir <bpringlemeir@nbsps.com>
---
 drivers/mtd/nand/Kconfig   |  12 +
 drivers/mtd/nand/Makefile  |   1 +
 drivers/mtd/nand/fsl_nfc.c | 676 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 689 insertions(+)
 create mode 100644 drivers/mtd/nand/fsl_nfc.c

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 93ae6a6..7e0c695 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -449,6 +449,18 @@ config MTD_NAND_MPC5121_NFC
 	  This enables the driver for the NAND flash controller on the
 	  MPC5121 SoC.
 
+config HAVE_NAND_FSL_NFC
+	bool
+
+config MTD_NAND_FSL_NFC
+	tristate "Support for Freescale NFC"
+	depends on HAVE_NAND_FSL_NFC
+	help
+	  Enables support for NAND Flash controller on some Freescale
+	  processors like the MPC5125, VF610, MCF54418, and Kinetis K70.
+	  The driver supports a maximum 2k page size.
+	  The driver currently does not support hardware ECC.
+
 config MTD_NAND_MXC
 	tristate "MXC NAND support"
 	depends on ARCH_MXC
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index 542b568..116f827 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -35,6 +35,7 @@ obj-$(CONFIG_MTD_NAND_PASEMI)		+= pasemi_nand.o
 obj-$(CONFIG_MTD_NAND_ORION)		+= orion_nand.o
 obj-$(CONFIG_MTD_NAND_FSL_ELBC)		+= fsl_elbc_nand.o
 obj-$(CONFIG_MTD_NAND_FSL_IFC)		+= fsl_ifc_nand.o
+obj-$(CONFIG_MTD_NAND_FSL_NFC)		+= fsl_nfc.o
 obj-$(CONFIG_MTD_NAND_FSL_UPM)		+= fsl_upm.o
 obj-$(CONFIG_MTD_NAND_SLC_LPC32XX)      += lpc32xx_slc.o
 obj-$(CONFIG_MTD_NAND_MLC_LPC32XX)      += lpc32xx_mlc.o
diff --git a/drivers/mtd/nand/fsl_nfc.c b/drivers/mtd/nand/fsl_nfc.c
new file mode 100644
index 0000000..eb4e353
--- /dev/null
+++ b/drivers/mtd/nand/fsl_nfc.c
@@ -0,0 +1,676 @@
+/*
+ * Copyright 2009-2012 Freescale Semiconductor, Inc.
+ *
+ * Description: MPC5125, VF610, MCF54418 and Kinetis K70 Nand driver.
+ * Jason ported to M54418TWR and MVFA5.
+ * Authors: Shaohui Xie <b21989@freescale.com>
+ *          Jason Jin <Jason.jin@freescale.com>
+ *
+ * Based on original driver mpc5121_nfc.c.
+ *
+ * This is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * Limitations:
+ * - Untested on MPC5125 and M54418.
+ * - DMA not used.
+ * - 2K pages or less.
+ */
+
+#include <linux/module.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
+#include <linux/mtd/partitions.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/of_mtd.h>
+
+#define	DRV_NAME		"fsl_nfc"
+
+/* Register Offsets */
+#define NFC_FLASH_CMD1			0x3F00
+#define NFC_FLASH_CMD2			0x3F04
+#define NFC_COL_ADDR			0x3F08
+#define NFC_ROW_ADDR			0x3F0c
+#define NFC_ROW_ADDR_INC		0x3F14
+#define NFC_FLASH_STATUS1		0x3F18
+#define NFC_FLASH_STATUS2		0x3F1c
+#define NFC_CACHE_SWAP			0x3F28
+#define NFC_SECTOR_SIZE			0x3F2c
+#define NFC_FLASH_CONFIG		0x3F30
+#define NFC_IRQ_STATUS			0x3F38
+
+/* Addresses for NFC MAIN RAM BUFFER areas */
+#define NFC_MAIN_AREA(n)		((n) *  0x1000)
+
+#define PAGE_2K				0x0800
+#define OOB_64				0x0040
+
+/* NFC_CMD2[CODE] values. See section:
+    - 31.4.7 Flash Command Code Description, Vybrid manual
+    - 23.8.6 Flash Command Sequencer, MPC5125 manual
+
+    Briefly these are bitmasks of controller cycles.
+*/
+#define READ_PAGE_CMD_CODE		0x7EE0
+#define PROGRAM_PAGE_CMD_CODE	0x7FC0
+#define ERASE_CMD_CODE			0x4EC0
+#define READ_ID_CMD_CODE		0x4804
+#define RESET_CMD_CODE			0x4040
+#define STATUS_READ_CMD_CODE	0x4068
+
+/* NFC ECC mode define */
+#define ECC_BYPASS			0
+
+/*** Register Mask and bit definitions */
+
+/* NFC_FLASH_CMD1 Field */
+#define CMD_BYTE2_MASK				0xFF000000
+#define CMD_BYTE2_SHIFT				24
+
+/* NFC_FLASH_CM2 Field */
+#define CMD_BYTE1_MASK				0xFF000000
+#define CMD_BYTE1_SHIFT				24
+#define CMD_CODE_MASK				0x00FFFF00
+#define CMD_CODE_SHIFT				8
+#define BUFNO_MASK				0x00000006
+#define BUFNO_SHIFT				1
+#define START_BIT				(1<<0)
+
+/* NFC_COL_ADDR Field */
+#define COL_ADDR_MASK				0x0000FFFF
+#define COL_ADDR_SHIFT				0
+
+/* NFC_ROW_ADDR Field */
+#define ROW_ADDR_MASK				0x00FFFFFF
+#define ROW_ADDR_SHIFT				0
+#define ROW_ADDR_CHIP_SEL_RB_MASK		0xF0000000
+#define ROW_ADDR_CHIP_SEL_RB_SHIFT		28
+#define ROW_ADDR_CHIP_SEL_MASK			0x0F000000
+#define ROW_ADDR_CHIP_SEL_SHIFT			24
+
+/* NFC_FLASH_STATUS2 Field */
+#define STATUS_BYTE1_MASK			0x000000FF
+
+/* NFC_FLASH_CONFIG Field */
+#define CONFIG_ECC_SRAM_REQ_BIT			(1<<21)
+#define CONFIG_DMA_REQ_BIT			(1<<20)
+#define CONFIG_ECC_MODE_MASK			0x000E0000
+#define CONFIG_ECC_MODE_SHIFT			17
+#define CONFIG_FAST_FLASH_BIT			(1<<16)
+#define CONFIG_16BIT				(1<<7)
+#define CONFIG_BOOT_MODE_BIT			(1<<6)
+#define CONFIG_ADDR_AUTO_INCR_BIT		(1<<5)
+#define CONFIG_BUFNO_AUTO_INCR_BIT		(1<<4)
+#define CONFIG_PAGE_CNT_MASK			0xF
+#define CONFIG_PAGE_CNT_SHIFT			0
+
+/* NFC_IRQ_STATUS Field */
+#define IDLE_IRQ_BIT				(1<<29)
+#define IDLE_EN_BIT				(1<<20)
+#define CMD_DONE_CLEAR_BIT			(1<<18)
+#define IDLE_CLEAR_BIT				(1<<17)
+
+#define NFC_TIMEOUT		(HZ)
+
+struct fsl_nfc {
+	struct mtd_info	   mtd;
+	struct nand_chip   chip;
+	struct device	  *dev;
+	void __iomem	  *regs;
+	wait_queue_head_t  irq_waitq;
+	uint               column;
+	int                spareonly;
+	int                page;
+	/* Status and ID are in alternate locations. */
+	int                alt_buf;
+#define ALT_BUF_ID   1
+#define ALT_BUF_STAT 2
+	struct clk        *clk;
+};
+#define mtd_to_nfc(_mtd) container_of(_mtd, struct fsl_nfc, mtd)
+
+static u8 bbt_pattern[] = {'B', 'b', 't', '0' };
+static u8 mirror_pattern[] = {'1', 't', 'b', 'B' };
+
+static struct nand_bbt_descr bbt_main_descr = {
+	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE |
+		   NAND_BBT_2BIT | NAND_BBT_VERSION,
+	.offs =	11,
+	.len = 4,
+	.veroffs = 15,
+	.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,
+	.offs =	11,
+	.len = 4,
+	.veroffs = 15,
+	.maxblocks = 4,
+	.pattern = mirror_pattern,
+};
+
+static u32 nfc_read(struct mtd_info *mtd, uint reg)
+{
+	struct fsl_nfc *nfc = mtd_to_nfc(mtd);
+	if (reg == NFC_FLASH_STATUS1 ||
+	    reg == NFC_FLASH_STATUS2 ||
+	    reg == NFC_IRQ_STATUS)
+		return __raw_readl(nfc->regs + reg);
+	/* Gang read/writes together for most registers. */
+	else
+		return *(u32 *)(nfc->regs + reg);
+}
+
+static void nfc_write(struct mtd_info *mtd, uint reg, u32 val)
+{
+	struct fsl_nfc *nfc = mtd_to_nfc(mtd);
+
+	if (reg == NFC_FLASH_STATUS1 ||
+	    reg == NFC_FLASH_STATUS2 ||
+	    reg == NFC_IRQ_STATUS)
+		__raw_writel(val, nfc->regs + reg);
+	/* Gang read/writes together for most registers. */
+	else
+		*(u32 *)(nfc->regs + reg) = val;
+}
+
+static void nfc_set(struct mtd_info *mtd, uint reg, u32 bits)
+{
+	nfc_write(mtd, reg, nfc_read(mtd, reg) | bits);
+}
+
+static void nfc_clear(struct mtd_info *mtd, uint reg, u32 bits)
+{
+	nfc_write(mtd, reg, nfc_read(mtd, reg) & ~bits);
+}
+
+static void nfc_set_field(struct mtd_info *mtd, u32 reg,
+			  u32 mask, u32 shift, u32 val)
+{
+	nfc_write(mtd, reg, (nfc_read(mtd, reg) & (~mask)) | val << shift);
+}
+
+/* Clear flags for upcoming command */
+static void nfc_clear_status(struct mtd_info *mtd)
+{
+	nfc_set(mtd, NFC_IRQ_STATUS, CMD_DONE_CLEAR_BIT);
+	nfc_set(mtd, NFC_IRQ_STATUS, IDLE_CLEAR_BIT);
+}
+
+/* Wait for complete operation */
+static void nfc_done(struct mtd_info *mtd)
+{
+	struct fsl_nfc *nfc = mtd_to_nfc(mtd);
+	int rv;
+
+	nfc_set(mtd, NFC_IRQ_STATUS, IDLE_EN_BIT);
+	/* CMD2 is almost non-volatile, START_BIT is not */
+	barrier();
+	nfc_set(mtd, NFC_FLASH_CMD2, START_BIT);
+	barrier();
+
+	if (!(nfc_read(mtd, NFC_IRQ_STATUS) & IDLE_IRQ_BIT)) {
+		rv = wait_event_timeout(nfc->irq_waitq,
+			(nfc_read(mtd, NFC_IRQ_STATUS) & IDLE_IRQ_BIT),
+					NFC_TIMEOUT);
+		if (!rv)
+			dev_warn(nfc->dev, "Timeout while waiting for BUSY.\n");
+	}
+	nfc_clear_status(mtd);
+}
+
+static u8 nfc_get_id(struct mtd_info *mtd, int col)
+{
+	u32 flash_id;
+
+	if (col < 4) {
+		flash_id = nfc_read(mtd, NFC_FLASH_STATUS1);
+		return (flash_id >> (3-col)*8) & 0xff;
+	} else {
+		flash_id = nfc_read(mtd, NFC_FLASH_STATUS2);
+		return flash_id >> 24;
+	}
+}
+
+static u8 nfc_get_status(struct mtd_info *mtd)
+{
+	return nfc_read(mtd, NFC_FLASH_STATUS2) & STATUS_BYTE1_MASK;
+}
+
+/* Single command */
+static void nfc_send_command(struct mtd_info *mtd, u32 cmd_byte1,
+			     u32 cmd_code)
+{
+	nfc_clear_status(mtd);
+
+	nfc_set_field(mtd, NFC_FLASH_CMD2, CMD_BYTE1_MASK,
+			CMD_BYTE1_SHIFT, cmd_byte1);
+
+	nfc_clear(mtd, NFC_FLASH_CMD2, BUFNO_MASK);
+
+	nfc_set_field(mtd, NFC_FLASH_CMD2, CMD_CODE_MASK,
+			CMD_CODE_SHIFT, cmd_code);
+}
+
+/* Two commands */
+static void nfc_send_commands(struct mtd_info *mtd, u32 cmd_byte1,
+			      u32 cmd_byte2, u32 cmd_code)
+{
+	nfc_send_command(mtd, cmd_byte1, cmd_code);
+
+	nfc_set_field(mtd, NFC_FLASH_CMD1, CMD_BYTE2_MASK,
+			CMD_BYTE2_SHIFT, cmd_byte2);
+}
+
+static irqreturn_t nfc_irq(int irq, void *data)
+{
+	struct mtd_info *mtd = data;
+	struct fsl_nfc *nfc = mtd_to_nfc(mtd);
+
+	nfc_clear(mtd, NFC_IRQ_STATUS, IDLE_EN_BIT);
+	wake_up(&nfc->irq_waitq);
+
+	return IRQ_HANDLED;
+}
+
+static void nfc_addr_cycle(struct mtd_info *mtd, int column, int page)
+{
+	if (column != -1) {
+		struct fsl_nfc *nfc = mtd_to_nfc(mtd);
+		if (nfc->chip.options | NAND_BUSWIDTH_16)
+			column = column/2;
+		nfc_set_field(mtd, NFC_COL_ADDR, COL_ADDR_MASK,
+			      COL_ADDR_SHIFT, column);
+	}
+	if (page != -1)
+		nfc_set_field(mtd, NFC_ROW_ADDR, ROW_ADDR_MASK,
+				ROW_ADDR_SHIFT, page);
+}
+
+/* Send command to NAND chip */
+static void nfc_command(struct mtd_info *mtd, unsigned command,
+			int column, int page)
+{
+	struct fsl_nfc *nfc = mtd_to_nfc(mtd);
+
+	nfc->column     = max(column, 0);
+	nfc->spareonly	= 0;
+	nfc->alt_buf	= 0;
+
+	switch (command) {
+	case NAND_CMD_PAGEPROG:
+		nfc->page = -1;
+		nfc_send_commands(mtd, NAND_CMD_SEQIN,
+			     command, PROGRAM_PAGE_CMD_CODE);
+		nfc_addr_cycle(mtd, column, page);
+		break;
+
+	case NAND_CMD_RESET:
+		nfc_send_command(mtd, command, RESET_CMD_CODE);
+		break;
+	/*
+	 * NFC does not support sub-page reads and writes,
+	 * so emulate them using full page transfers.
+	 */
+	case NAND_CMD_READOOB:
+		nfc->spareonly = 1;
+	case NAND_CMD_SEQIN: /* Pre-read for partial writes. */
+	case NAND_CMD_READ0:
+		column = 0;
+		/* Already read? */
+		if (nfc->page == page)
+			return;
+		nfc->page = page;
+		nfc_send_commands(mtd, NAND_CMD_READ0,
+				  NAND_CMD_READSTART, READ_PAGE_CMD_CODE);
+		nfc_addr_cycle(mtd, column, page);
+		break;
+
+	case NAND_CMD_ERASE1:
+		if (nfc->page == page)
+			nfc->page = -1;
+		nfc_send_commands(mtd, command,
+				  NAND_CMD_ERASE2, ERASE_CMD_CODE);
+		nfc_addr_cycle(mtd, column, page);
+		break;
+
+	case NAND_CMD_READID:
+		nfc->alt_buf = ALT_BUF_ID;
+		nfc_send_command(mtd, command, READ_ID_CMD_CODE);
+		break;
+
+	case NAND_CMD_STATUS:
+		nfc->alt_buf = ALT_BUF_STAT;
+		nfc_send_command(mtd, command, STATUS_READ_CMD_CODE);
+		break;
+	default:
+		return;
+	}
+
+	nfc_done(mtd);
+}
+
+static void nfc_read_spare(struct mtd_info *mtd, void *buf, int len)
+{
+	struct fsl_nfc *nfc = mtd_to_nfc(mtd);
+
+	len = min(mtd->oobsize, (uint)len);
+	if (len > 0)
+		memcpy(buf, nfc->regs + mtd->writesize, len);
+}
+
+/* Read data from NFC buffers */
+static void nfc_read_buf(struct mtd_info *mtd, u_char *buf, int len)
+{
+	struct fsl_nfc *nfc = mtd_to_nfc(mtd);
+	uint c = nfc->column;
+	uint l;
+
+	/* Handle main area */
+	if (!nfc->spareonly) {
+
+		l = min((uint)len, mtd->writesize - c);
+		nfc->column += l;
+
+		if (!nfc->alt_buf)
+			memcpy(buf, nfc->regs + NFC_MAIN_AREA(0) + c, l);
+		else
+			if (nfc->alt_buf & ALT_BUF_ID)
+				*buf = nfc_get_id(mtd, c);
+			else
+				*buf = nfc_get_status(mtd);
+		buf += l;
+		len -= l;
+	}
+
+	/* Handle spare area access */
+	if (len) {
+		nfc->column += len;
+		nfc_read_spare(mtd, buf, len);
+	}
+}
+
+/* Write data to NFC buffers */
+static void nfc_write_buf(struct mtd_info *mtd, const u_char *buf, int len)
+{
+	struct fsl_nfc *nfc = mtd_to_nfc(mtd);
+	uint c = nfc->column;
+	uint l;
+
+	l = min((uint)len, mtd->writesize + mtd->oobsize - c);
+	nfc->column += l;
+
+	memcpy(nfc->regs + NFC_MAIN_AREA(0) + c, buf, l);
+}
+
+/* Read byte from NFC buffers */
+static u8 nfc_read_byte(struct mtd_info *mtd)
+{
+	u8 tmp;
+	nfc_read_buf(mtd, &tmp, sizeof(tmp));
+	return tmp;
+}
+
+/* Read word from NFC buffers */
+static u16 nfc_read_word(struct mtd_info *mtd)
+{
+	u16 tmp;
+	nfc_read_buf(mtd, (u_char *)&tmp, sizeof(tmp));
+	return tmp;
+}
+
+/* If not provided, upper layers apply a fixed delay. */
+static int nfc_dev_ready(struct mtd_info *mtd)
+{
+	/* NFC handles R/B internally; always ready.  */
+	return 1;
+}
+
+/* Vybrid only.  MPC5125 has full RB and four CS. Assume boot loader
+ * has set this register for now.
+ */
+static void
+nfc_select_chip(struct mtd_info *mtd, int chip)
+{
+#ifdef CONFIG_SOC_VF610
+	nfc_set_field(mtd, NFC_ROW_ADDR, ROW_ADDR_CHIP_SEL_RB_MASK,
+		      ROW_ADDR_CHIP_SEL_RB_SHIFT, 1);
+
+	if (chip == 0)
+		nfc_set_field(mtd, NFC_ROW_ADDR, ROW_ADDR_CHIP_SEL_MASK,
+			      ROW_ADDR_CHIP_SEL_SHIFT, 1);
+	else if (chip == 1)
+		nfc_set_field(mtd, NFC_ROW_ADDR, ROW_ADDR_CHIP_SEL_MASK,
+			      ROW_ADDR_CHIP_SEL_SHIFT, 2);
+	else
+		nfc_clear(mtd, NFC_ROW_ADDR, ROW_ADDR_CHIP_SEL_MASK);
+#endif
+}
+
+struct nfc_config {
+	int width;
+	int flash_bbt;
+	u32 clkrate;
+};
+
+#ifdef CONFIG_OF_MTD
+static struct of_device_id nfc_dt_ids[] = {
+	{ .compatible = "fsl,vf610-nfc" },
+	{ .compatible = "fsl,mpc5125-nfc" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, nfc_dt_ids);
+
+static int __init nfc_probe_dt(struct device *dev, struct nfc_config *cfg)
+{
+	struct device_node *np = dev->of_node;
+	int buswidth;
+	u32 clkrate;
+
+	memset(cfg, 0, sizeof(*cfg));
+
+	if (!np)
+		return 1;
+
+	cfg->flash_bbt = of_get_nand_on_flash_bbt(np);
+
+	if (!of_property_read_u32(np, "clock-frequency", &clkrate))
+		cfg->clkrate = clkrate;
+
+	buswidth = of_get_nand_bus_width(np);
+	if (buswidth < 0)
+		return buswidth;
+
+	cfg->width = buswidth;
+
+	return 0;
+}
+#else
+static int __init nfc_probe_dt(struct device *dev, struct nfc_config *cfg)
+{
+	memset(cfg, 0, sizeof(*cfg));
+	return 0;
+}
+#endif
+
+static int nfc_probe(struct platform_device *pdev)
+{
+	struct fsl_nfc *nfc;
+	struct resource *res;
+	struct mtd_info *mtd;
+	struct nand_chip *chip;
+	struct nfc_config cfg;
+	int err = 0;
+	int page_sz;
+	int irq;
+
+	nfc = devm_kzalloc(&pdev->dev, sizeof(*nfc), GFP_KERNEL);
+	if (!nfc)
+		return -ENOMEM;
+
+	nfc->dev = &pdev->dev;
+	nfc->page = -1;
+	mtd = &nfc->mtd;
+	chip = &nfc->chip;
+
+	mtd->priv = chip;
+	mtd->owner = THIS_MODULE;
+	mtd->dev.parent = nfc->dev;
+	mtd->name = DRV_NAME;
+
+	err = nfc_probe_dt(nfc->dev, &cfg);
+	if (err)
+		return -ENODEV;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq <= 0)
+		return -EINVAL;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	nfc->regs = devm_ioremap_resource(nfc->dev, res);
+	if (IS_ERR(nfc->regs))
+		return PTR_ERR(nfc->regs);
+
+	nfc->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(nfc->clk))
+		return PTR_ERR(nfc->clk);
+
+	if (cfg.clkrate && clk_set_rate(nfc->clk, cfg.clkrate)) {
+		dev_err(nfc->dev, "Clock rate not set.");
+		return -EINVAL;
+	}
+
+	err = clk_prepare_enable(nfc->clk);
+	if (err) {
+		dev_err(nfc->dev, "Unable to enable clock!\n");
+		return err;
+	}
+
+	if (cfg.width == 16) {
+		chip->options |= NAND_BUSWIDTH_16;
+		nfc_set(mtd, NFC_FLASH_CONFIG, CONFIG_16BIT);
+	} else {
+		chip->options &= ~NAND_BUSWIDTH_16;
+		nfc_clear(mtd, NFC_FLASH_CONFIG, CONFIG_16BIT);
+	}
+
+	chip->dev_ready = nfc_dev_ready;
+	chip->cmdfunc = nfc_command;
+	chip->read_byte = nfc_read_byte;
+	chip->read_word = nfc_read_word;
+	chip->read_buf = nfc_read_buf;
+	chip->write_buf = nfc_write_buf;
+	chip->select_chip = nfc_select_chip;
+
+	/* Bad block options. */
+	if (cfg.flash_bbt)
+		chip->bbt_options = NAND_BBT_USE_FLASH | NAND_BBT_CREATE;
+
+	/* Default to software ECC until flash ID. */
+	nfc_set_field(mtd, NFC_FLASH_CONFIG,
+		      CONFIG_ECC_MODE_MASK,
+		      CONFIG_ECC_MODE_SHIFT, ECC_BYPASS);
+
+	chip->bbt_td = &bbt_main_descr;
+	chip->bbt_md = &bbt_mirror_descr;
+
+	init_waitqueue_head(&nfc->irq_waitq);
+
+	err = devm_request_irq(nfc->dev, irq, nfc_irq, 0, DRV_NAME, mtd);
+	if (err) {
+		dev_err(nfc->dev, "Error requesting IRQ!\n");
+		goto error;
+	}
+
+	page_sz = PAGE_2K + OOB_64;
+	page_sz += cfg.width == 16 ? 1 : 0;
+	nfc_write(mtd, NFC_SECTOR_SIZE, page_sz);
+
+	/* Set configuration register. */
+	nfc_clear(mtd, NFC_FLASH_CONFIG, CONFIG_ADDR_AUTO_INCR_BIT);
+	nfc_clear(mtd, NFC_FLASH_CONFIG, CONFIG_BUFNO_AUTO_INCR_BIT);
+	nfc_clear(mtd, NFC_FLASH_CONFIG, CONFIG_BOOT_MODE_BIT);
+	nfc_clear(mtd, NFC_FLASH_CONFIG, CONFIG_DMA_REQ_BIT);
+	nfc_set(mtd, NFC_FLASH_CONFIG, CONFIG_FAST_FLASH_BIT);
+
+	/* PAGE_CNT = 1 */
+	nfc_set_field(mtd, NFC_FLASH_CONFIG, CONFIG_PAGE_CNT_MASK,
+			CONFIG_PAGE_CNT_SHIFT, 1);
+
+	/* first scan to find the device and get the page size */
+	if (nand_scan_ident(mtd, 1, NULL)) {
+		err = -ENXIO;
+		goto error;
+	}
+
+	chip->ecc.mode = NAND_ECC_SOFT; /* default */
+
+	page_sz = mtd->writesize + mtd->oobsize;
+
+	/* Single buffer only, max 256 OOB minus ECC status */
+	if (page_sz > PAGE_2K + 256 - 8) {
+		dev_err(nfc->dev, "Unsupported flash size\n");
+		err = -ENXIO;
+		goto error;
+	}
+	page_sz += cfg.width == 16 ? 1 : 0;
+	nfc_write(mtd, NFC_SECTOR_SIZE, page_sz);
+
+	/* second phase scan */
+	if (nand_scan_tail(mtd)) {
+		err = -ENXIO;
+		goto error;
+	}
+
+	/* Register device in MTD */
+	mtd_device_parse_register(mtd, NULL,
+		&(struct mtd_part_parser_data){
+			.of_node = pdev->dev.of_node,
+		},
+		NULL, 0);
+
+	platform_set_drvdata(pdev, mtd);
+
+	return 0;
+
+error:
+	clk_disable_unprepare(nfc->clk);
+	return err;
+}
+
+static int nfc_remove(struct platform_device *pdev)
+{
+	struct mtd_info *mtd = platform_get_drvdata(pdev);
+	struct fsl_nfc *nfc = mtd_to_nfc(mtd);
+
+	nand_release(mtd);
+	clk_disable_unprepare(nfc->clk);
+	return 0;
+}
+
+static struct platform_driver nfc_driver = {
+	.driver		= {
+		.name	= DRV_NAME,
+		.owner	= THIS_MODULE,
+		.of_match_table = nfc_dt_ids,
+	},
+	.probe		= nfc_probe,
+	.remove		= nfc_remove,
+};
+
+module_platform_driver(nfc_driver);
+
+MODULE_AUTHOR("Freescale Semiconductor, Inc.");
+MODULE_DESCRIPTION("FSL NFC MTD driver");
+MODULE_LICENSE("GPL");
-- 
1.8.0.2

Re: [RFC 1/5] mtd:fsl_nfc: Nand flash controller for VF610, MPC5125, etc.

[Stefan Agner]

Hi Bill,

The driver works fine for me using 3.14 on Colibri VF61 (8-Bit bus
width, Samsung NAND, 2k page size). Also tested with the Hardware ECC.
Do you plan to send an update patch of the driver?

FYI, I ported the driver to U-Boot and will send a patch to the U-Boot
mailing list soon.

Some minor comments below:

Am 2014-01-09 00:07, schrieb Bill Pringlemeir:
<snip>
> +static u8 bbt_pattern[] = {'B', 'b', 't', '0' };
> +static u8 mirror_pattern[] = {'1', 't', 'b', 'B' };
> +
> +static struct nand_bbt_descr bbt_main_descr = {
> +	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE |
> +		   NAND_BBT_2BIT | NAND_BBT_VERSION,
> +	.offs =	11,
> +	.len = 4,
> +	.veroffs = 15,
> +	.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,
> +	.offs =	11,
> +	.len = 4,
> +	.veroffs = 15,
> +	.maxblocks = 4,
> +	.pattern = mirror_pattern,
> +};

This is the same BBT descriptor as used on Timesys Vybrid BSP. Other
than this "backward compatibility", is there another reason to use non
default BBT descriptor? As far as I can tell, the ECC does not conflict
with the default BBT description.

> +/* Write data to NFC buffers */
> +static void nfc_write_buf(struct mtd_info *mtd, const u_char *buf, int len)
> +{
...
IMHO this type of commands are not really required, the function name is
descriptive enough.

> +/* Vybrid only.  MPC5125 has full RB and four CS. Assume boot loader
> + * has set this register for now.
> + */

Multi-line comment style (there are some malformed multi-line comments
in the ECC patch as well)

--
Stefan

[RFC 1/5] mtd:fsl_nfc: Nand flash controller for VF610, MPC5125, etc.

[Stefan Agner]

Hi Bill,

The driver works fine for me using 3.14 on Colibri VF61 (8-Bit bus
width, Samsung NAND, 2k page size). Also tested with the Hardware ECC.
Do you plan to send an update patch of the driver?

FYI, I ported the driver to U-Boot and will send a patch to the U-Boot
mailing list soon.

Some minor comments below:

Am 2014-01-09 00:07, schrieb Bill Pringlemeir:
<snip>
> +static u8 bbt_pattern[] = {'B', 'b', 't', '0' };
> +static u8 mirror_pattern[] = {'1', 't', 'b', 'B' };
> +
> +static struct nand_bbt_descr bbt_main_descr = {
> +	.options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE |
> +		   NAND_BBT_2BIT | NAND_BBT_VERSION,
> +	.offs =	11,
> +	.len = 4,
> +	.veroffs = 15,
> +	.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,
> +	.offs =	11,
> +	.len = 4,
> +	.veroffs = 15,
> +	.maxblocks = 4,
> +	.pattern = mirror_pattern,
> +};

This is the same BBT descriptor as used on Timesys Vybrid BSP. Other
than this "backward compatibility", is there another reason to use non
default BBT descriptor? As far as I can tell, the ECC does not conflict
with the default BBT description.

> +/* Write data to NFC buffers */
> +static void nfc_write_buf(struct mtd_info *mtd, const u_char *buf, int len)
> +{
...
IMHO this type of commands are not really required, the function name is
descriptive enough.

> +/* Vybrid only.  MPC5125 has full RB and four CS. Assume boot loader
> + * has set this register for now.
> + */

Multi-line comment style (there are some malformed multi-line comments
in the ECC patch as well)

--
Stefan

Re: [RFC 1/5] mtd:fsl_nfc: Nand flash controller for VF610, MPC5125, etc.

[Bill Pringlemeir]

On 28 Apr 2014, stefan@agner.ch wrote:

> The driver works fine for me using 3.14 on Colibri VF61 (8-Bit bus
> width, Samsung NAND, 2k page size). Also tested with the Hardware ECC.
> Do you plan to send an update patch of the driver?

> FYI, I ported the driver to U-Boot and will send a patch to the U-Boot
> mailing list soon.

> Some minor comments below:

Well, I would love it if there are only 'minor comments'.  I don't think
people will like the 'nfc' name.  I wanted a better name.  Also, the
'linux-mtd' list bounced my post because I used some 'Ref:' to refer to
another message.  It also bounce on the ARM list, but some kind
moderator put it through.

 http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/226623.html
 http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/226627.html
 etc.

Besides the Vybrid, the controller can support several other SOCs (some
ARM, some note), Such as the MPC5125 (PowerPC), MCF54418 (ColdFire) and
the Kinetis K70 (ARM Cortex-M).

I also have some tickets open on the Hardware ECC with the Vybrid.

 https://community.freescale.com/message/358284 - booting
 https://community.freescale.com/message/368216 - ECC value
 https://community.freescale.com/message/384556 - clocking

[There are also non-public freescale PR tickets]

Especially, the ECC layout is important.  I think that an HW ECC layout
with sub-page support is best.  The Linux-MTD community will want this
to be right.  

The email "reference" was a previous email I sent some time ago to the
MTD mailing list.  I wondered if anyone was interested and I knew that
people would not like the name 'fsl_nfc'.  But I don't know what to call
it; it is a bike shed issue to me (specifics of what to call it), but I
see how people will want to avoid a generic ambigious name like
'fsl_nfc'.

I was waiting to see about the clocking with HW-ECC; it seems above
33MHz, the HW-ECC module doesn't seem to work (at least for me).

Fwiw,
Bill Pringlemeir.

[RFC 1/5] mtd:fsl_nfc: Nand flash controller for VF610, MPC5125, etc.

[Bill Pringlemeir]

On 28 Apr 2014, stefan at agner.ch wrote:

> The driver works fine for me using 3.14 on Colibri VF61 (8-Bit bus
> width, Samsung NAND, 2k page size). Also tested with the Hardware ECC.
> Do you plan to send an update patch of the driver?

> FYI, I ported the driver to U-Boot and will send a patch to the U-Boot
> mailing list soon.

> Some minor comments below:

Well, I would love it if there are only 'minor comments'.  I don't think
people will like the 'nfc' name.  I wanted a better name.  Also, the
'linux-mtd' list bounced my post because I used some 'Ref:' to refer to
another message.  It also bounce on the ARM list, but some kind
moderator put it through.

 http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/226623.html
 http://lists.infradead.org/pipermail/linux-arm-kernel/2014-January/226627.html
 etc.

Besides the Vybrid, the controller can support several other SOCs (some
ARM, some note), Such as the MPC5125 (PowerPC), MCF54418 (ColdFire) and
the Kinetis K70 (ARM Cortex-M).

I also have some tickets open on the Hardware ECC with the Vybrid.

 https://community.freescale.com/message/358284 - booting
 https://community.freescale.com/message/368216 - ECC value
 https://community.freescale.com/message/384556 - clocking

[There are also non-public freescale PR tickets]

Especially, the ECC layout is important.  I think that an HW ECC layout
with sub-page support is best.  The Linux-MTD community will want this
to be right.  

The email "reference" was a previous email I sent some time ago to the
MTD mailing list.  I wondered if anyone was interested and I knew that
people would not like the name 'fsl_nfc'.  But I don't know what to call
it; it is a bike shed issue to me (specifics of what to call it), but I
see how people will want to avoid a generic ambigious name like
'fsl_nfc'.

I was waiting to see about the clocking with HW-ECC; it seems above
33MHz, the HW-ECC module doesn't seem to work (at least for me).

Fwiw,
Bill Pringlemeir.

Re: [RFC 1/5] mtd:fsl_nfc: Nand flash controller for VF610, MPC5125, etc.

[Stefan Agner]

Hi Bill,

Am 2014-04-28 18:51, schrieb Bill Pringlemeir:
> The email "reference" was a previous email I sent some time ago to the
> MTD mailing list.  I wondered if anyone was interested and I knew that
> people would not like the name 'fsl_nfc'.  But I don't know what to call
> it; it is a bike shed issue to me (specifics of what to call it), but I
> see how people will want to avoid a generic ambigious name like
> 'fsl_nfc'.

In all documentation and block diagram I've read Freescale itself calls
that IP "NAND Flash Controller", even with the abbreviation NFC. IMHO,
its a valid name then. The only alternative would probably be MCF5441x,
since this is the first device that controller appeared, but since this
is ColdFire microprocessor which isn't suited for Linux, a driver named
after it in the kernel tree would be somewhat weird.


> I was waiting to see about the clocking with HW-ECC; it seems above
> 33MHz, the HW-ECC module doesn't seem to work (at least for me).

Yes, I could also verify this (see my comment on that Freescale
Community post).

Appreciate your work, count me as interested!

This is my U-Boot port of the driver, I'm going to cleanup and test it
on the Tower:
http://git.toradex.com/gitweb/u-boot-toradex.git/shortlog/refs/heads/2014.04-colibri_vf

--
Stefan

[RFC 1/5] mtd:fsl_nfc: Nand flash controller for VF610, MPC5125, etc.

[Stefan Agner]

Hi Bill,

Am 2014-04-28 18:51, schrieb Bill Pringlemeir:
> The email "reference" was a previous email I sent some time ago to the
> MTD mailing list.  I wondered if anyone was interested and I knew that
> people would not like the name 'fsl_nfc'.  But I don't know what to call
> it; it is a bike shed issue to me (specifics of what to call it), but I
> see how people will want to avoid a generic ambigious name like
> 'fsl_nfc'.

In all documentation and block diagram I've read Freescale itself calls
that IP "NAND Flash Controller", even with the abbreviation NFC. IMHO,
its a valid name then. The only alternative would probably be MCF5441x,
since this is the first device that controller appeared, but since this
is ColdFire microprocessor which isn't suited for Linux, a driver named
after it in the kernel tree would be somewhat weird.


> I was waiting to see about the clocking with HW-ECC; it seems above
> 33MHz, the HW-ECC module doesn't seem to work (at least for me).

Yes, I could also verify this (see my comment on that Freescale
Community post).

Appreciate your work, count me as interested!

This is my U-Boot port of the driver, I'm going to cleanup and test it
on the Tower:
http://git.toradex.com/gitweb/u-boot-toradex.git/shortlog/refs/heads/2014.04-colibri_vf

--
Stefan

Re: [RFC 1/5] mtd:fsl_nfc: Nand flash controller for VF610, MPC5125, etc.

[Bill Pringlemeir]

On 28 Apr 2014, stefan@agner.ch wrote:

> The driver works fine for me using 3.14 on Colibri VF61 (8-Bit bus
> width, Samsung NAND, 2k page size). Also tested with the Hardware ECC.
> Do you plan to send an update patch of the driver?

> FYI, I ported the driver to U-Boot and will send a patch to the U-Boot
> mailing list soon.

> Some minor comments below:

> Am 2014-01-09 00:07, schrieb Bill Pringlemeir:
> <snip>
>> +static u8 bbt_pattern[] = {'B', 'b', 't', '0' }; +static u8
>> mirror_pattern[] = {'1', 't', 'b', 'B' }; + +static struct
>> nand_bbt_descr bbt_main_descr = { + .options = NAND_BBT_LASTBLOCK |
>> NAND_BBT_CREATE | NAND_BBT_WRITE | + NAND_BBT_2BIT |
>> NAND_BBT_VERSION, + .offs = 11, + .len = 4, + .veroffs = 15, +
>> .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, + .offs = 11, + .len = 4, + .veroffs = 15, +
>> .maxblocks = 4, + .pattern = mirror_pattern, +};

> This is the same BBT descriptor as used on Timesys Vybrid BSP. Other
> than this "backward compatibility", is there another reason to use non
> default BBT descriptor? As far as I can tell, the ECC does not
> conflict with the default BBT description.

Beyond the "TimeSys", there are OpenWRT projects and others that seem to
use this BBT structure.  Myself, I don't care.  The question would be
will someone ever get to use this driver with some other system?  It is
simple enough to patch the driver; although a device tree binding
supported by the driver might be more flexible to allow both from
multi-machine builds.  This particular chip is not really a candidate as
it always seems to be used with a different architecture; PowerPC,
ColdFire/68k, ARM Cortex-A or Cortex-M.

>> +/* Write data to NFC buffers */ 
>> +static void nfc_write_buf(struct mtd_info *mtd, const u_char *buf,
>> int len)
>> +{
> ...  IMHO this type of commands are not really required, the function
> name is descriptive enough.

The comments are from the original.

https://github.com/Timesys/linux-timesys/blob/3.0-mvf/drivers/mtd/nand/fsl_nfc.c#L170

I agree they are not needed.

>> +/* Vybrid only.  MPC5125 has full RB and four CS. Assume boot loader
>> + * has set this register for now.
>> + */

> Multi-line comment style (there are some malformed multi-line comments
> in the ECC patch as well)

This is my comment.  I must of missed the advice on multi-line comments.
I did all sorts of strange things with test emails, building with
different git trees, running different scripts, re-ordering the patches
to try and make them understandable, testing different variants,
benchmarking, etc.  White space is not a riveting issue for me.  I just
cribbed some emacs code and hope it works.

   (defun linux-c-mode ()
     "C mode with adjusted defaults for use with the Linux kernel."
     (interactive)
     (c-mode)
     (c-set-style "K&R")
     (setq tab-width 8)
     (setq indent-tabs-mode t)
     (setq truncate-lines t)
     (setq show-trailing-whitespace t)
     (setq c-basic-offset 8))

I looked again, you mean that I should have the first "star" line blank
or is there more?

There are other issues, like Shawn Guo's IMX tree has a better structure
for the IOMUX bindings.  I am also concerned that people don't like the
order of the patches and I have to fsck with git to re-arrange them
(again).  However, I think that having the MTD people willing to accept
is my major hurdle on working on it further.  I don't know if they want
yet another controller?  I am glad that you can use it too and have
tested it with 8-bit flash.

Thanks,
Bill Pringlemeir.

[RFC 1/5] mtd:fsl_nfc: Nand flash controller for VF610, MPC5125, etc.

[Bill Pringlemeir]

On 28 Apr 2014, stefan at agner.ch wrote:

> The driver works fine for me using 3.14 on Colibri VF61 (8-Bit bus
> width, Samsung NAND, 2k page size). Also tested with the Hardware ECC.
> Do you plan to send an update patch of the driver?

> FYI, I ported the driver to U-Boot and will send a patch to the U-Boot
> mailing list soon.

> Some minor comments below:

> Am 2014-01-09 00:07, schrieb Bill Pringlemeir:
> <snip>
>> +static u8 bbt_pattern[] = {'B', 'b', 't', '0' }; +static u8
>> mirror_pattern[] = {'1', 't', 'b', 'B' }; + +static struct
>> nand_bbt_descr bbt_main_descr = { + .options = NAND_BBT_LASTBLOCK |
>> NAND_BBT_CREATE | NAND_BBT_WRITE | + NAND_BBT_2BIT |
>> NAND_BBT_VERSION, + .offs = 11, + .len = 4, + .veroffs = 15, +
>> .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, + .offs = 11, + .len = 4, + .veroffs = 15, +
>> .maxblocks = 4, + .pattern = mirror_pattern, +};

> This is the same BBT descriptor as used on Timesys Vybrid BSP. Other
> than this "backward compatibility", is there another reason to use non
> default BBT descriptor? As far as I can tell, the ECC does not
> conflict with the default BBT description.

Beyond the "TimeSys", there are OpenWRT projects and others that seem to
use this BBT structure.  Myself, I don't care.  The question would be
will someone ever get to use this driver with some other system?  It is
simple enough to patch the driver; although a device tree binding
supported by the driver might be more flexible to allow both from
multi-machine builds.  This particular chip is not really a candidate as
it always seems to be used with a different architecture; PowerPC,
ColdFire/68k, ARM Cortex-A or Cortex-M.

>> +/* Write data to NFC buffers */ 
>> +static void nfc_write_buf(struct mtd_info *mtd, const u_char *buf,
>> int len)
>> +{
> ...  IMHO this type of commands are not really required, the function
> name is descriptive enough.

The comments are from the original.

https://github.com/Timesys/linux-timesys/blob/3.0-mvf/drivers/mtd/nand/fsl_nfc.c#L170

I agree they are not needed.

>> +/* Vybrid only.  MPC5125 has full RB and four CS. Assume boot loader
>> + * has set this register for now.
>> + */

> Multi-line comment style (there are some malformed multi-line comments
> in the ECC patch as well)

This is my comment.  I must of missed the advice on multi-line comments.
I did all sorts of strange things with test emails, building with
different git trees, running different scripts, re-ordering the patches
to try and make them understandable, testing different variants,
benchmarking, etc.  White space is not a riveting issue for me.  I just
cribbed some emacs code and hope it works.

   (defun linux-c-mode ()
     "C mode with adjusted defaults for use with the Linux kernel."
     (interactive)
     (c-mode)
     (c-set-style "K&R")
     (setq tab-width 8)
     (setq indent-tabs-mode t)
     (setq truncate-lines t)
     (setq show-trailing-whitespace t)
     (setq c-basic-offset 8))

I looked again, you mean that I should have the first "star" line blank
or is there more?

There are other issues, like Shawn Guo's IMX tree has a better structure
for the IOMUX bindings.  I am also concerned that people don't like the
order of the patches and I have to fsck with git to re-arrange them
(again).  However, I think that having the MTD people willing to accept
is my major hurdle on working on it further.  I don't know if they want
yet another controller?  I am glad that you can use it too and have
tested it with 8-bit flash.

Thanks,
Bill Pringlemeir.

[RFC 2/5] mtd:fsl_nfc: Add hardware 45 byte BHC-ECC support for 24 bit corrections.

[Bill Pringlemeir]

Signed-off-by: Bill Pringlemeir <bpringlemeir@nbsps.com>
---
 drivers/mtd/nand/Kconfig   |   5 +-
 drivers/mtd/nand/fsl_nfc.c | 114 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 7e0c695..8ac9923 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -458,8 +458,9 @@ config MTD_NAND_FSL_NFC
 	help
 	  Enables support for NAND Flash controller on some Freescale
 	  processors like the MPC5125, VF610, MCF54418, and Kinetis K70.
-	  The driver supports a maximum 2k page size.
-	  The driver currently does not support hardware ECC.
+	  The driver supports a maximum 2k page size.  With 2k pages and
+	  64 bytes or more of OOB, hardware ECC with 24bit correction is
+	  supported.  Only MTD_OF (device tree) can enable the hardware ECC.
 
 config MTD_NAND_MXC
 	tristate "MXC NAND support"
diff --git a/drivers/mtd/nand/fsl_nfc.c b/drivers/mtd/nand/fsl_nfc.c
index eb4e353..703511d 100644
--- a/drivers/mtd/nand/fsl_nfc.c
+++ b/drivers/mtd/nand/fsl_nfc.c
@@ -17,6 +17,7 @@
  * - Untested on MPC5125 and M54418.
  * - DMA not used.
  * - 2K pages or less.
+ * - Only 2K page w. 64+OOB and hardware ECC.
  */
 
 #include <linux/module.h>
@@ -68,6 +69,7 @@
 
 /* NFC ECC mode define */
 #define ECC_BYPASS			0
+#define ECC_45_BYTE			6
 
 /*** Register Mask and bit definitions */
 
@@ -100,6 +102,8 @@
 #define STATUS_BYTE1_MASK			0x000000FF
 
 /* NFC_FLASH_CONFIG Field */
+#define CONFIG_ECC_SRAM_ADDR_MASK		0x7FC00000
+#define CONFIG_ECC_SRAM_ADDR_SHIFT		22
 #define CONFIG_ECC_SRAM_REQ_BIT			(1<<21)
 #define CONFIG_DMA_REQ_BIT			(1<<20)
 #define CONFIG_ECC_MODE_MASK			0x000E0000
@@ -120,6 +124,11 @@
 
 #define NFC_TIMEOUT		(HZ)
 
+/* ECC status placed@end of buffers. */
+#define ECC_SRAM_ADDR	((PAGE_2K+256-8) >> 3)
+#define ECC_STATUS_MASK	0x80
+#define ECC_ERR_COUNT	0x3F
+
 struct fsl_nfc {
 	struct mtd_info	   mtd;
 	struct nand_chip   chip;
@@ -160,6 +169,19 @@ static struct nand_bbt_descr bbt_mirror_descr = {
 	.pattern = mirror_pattern,
 };
 
+static struct nand_ecclayout nfc_ecc45 = {
+	.eccbytes = 45,
+	.eccpos = {19, 20, 21, 22, 23,
+		   24, 25, 26, 27, 28, 29, 30, 31,
+		   32, 33, 34, 35, 36, 37, 38, 39,
+		   40, 41, 42, 43, 44, 45, 46, 47,
+		   48, 49, 50, 51, 52, 53, 54, 55,
+		   56, 57, 58, 59, 60, 61, 62, 63},
+	.oobfree = {
+		{.offset = 8,
+		 .length = 11} }
+};
+
 static u32 nfc_read(struct mtd_info *mtd, uint reg)
 {
 	struct fsl_nfc *nfc = mtd_to_nfc(mtd);
@@ -458,7 +480,61 @@ nfc_select_chip(struct mtd_info *mtd, int chip)
 #endif
 }
 
+/* Count the number of 0's in buff upto max_bits */
+static int count_written_bits(uint8_t *buff, int size, int max_bits)
+{
+	int k, written_bits = 0;
+
+	for (k = 0; k < size; k++) {
+		written_bits += hweight8(~buff[k]);
+		if (written_bits > max_bits)
+			break;
+	}
+
+	return written_bits;
+}
+
+static int nfc_correct_data(struct mtd_info *mtd, u_char *dat,
+				 u_char *read_ecc, u_char *calc_ecc)
+{
+	struct fsl_nfc *nfc = mtd_to_nfc(mtd);
+	u32 ecc_status;
+	u8 ecc_count;
+	int flip;
+
+	/* Errata: ECC status is stored at NFC_CFG[ECCADD] +4 for
+	   little-endian and +7 for big-endian SOC.  Access as 32 bits
+	   and use low byte.
+	*/
+	ecc_status = __raw_readl(nfc->regs + ECC_SRAM_ADDR * 8 + 4);
+	ecc_count = ecc_status & ECC_ERR_COUNT;
+	if (!(ecc_status & ECC_STATUS_MASK))
+		return ecc_count;
+
+	/* If 'ecc_count' zero or less then buffer is all 0xff or erased. */
+	flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);
+
+	/* ECC failed. */
+	if (flip > ecc_count)
+		return -1;
+
+	/* Erased page. */
+	memset(dat, 0xff, nfc->chip.ecc.size);
+	return 0;
+}
+
+static int nfc_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
+				  u_char *ecc_code)
+{
+	return 0;
+}
+
+static void nfc_enable_hwecc(struct mtd_info *mtd, int mode)
+{
+}
+
 struct nfc_config {
+	int hardware_ecc;
 	int width;
 	int flash_bbt;
 	u32 clkrate;
@@ -483,6 +559,9 @@ static int __init nfc_probe_dt(struct device *dev, struct nfc_config *cfg)
 	if (!np)
 		return 1;
 
+	if (of_get_nand_ecc_mode(np) >= NAND_ECC_HW)
+		cfg->hardware_ecc = 1;
+
 	cfg->flash_bbt = of_get_nand_on_flash_bbt(np);
 
 	if (!of_property_read_u32(np, "clock-frequency", &clkrate))
@@ -608,6 +687,11 @@ static int nfc_probe(struct platform_device *pdev)
 	nfc_set_field(mtd, NFC_FLASH_CONFIG, CONFIG_PAGE_CNT_MASK,
 			CONFIG_PAGE_CNT_SHIFT, 1);
 
+	/* Set ECC_STATUS offset */
+	nfc_set_field(mtd, NFC_FLASH_CONFIG,
+		      CONFIG_ECC_SRAM_ADDR_MASK,
+		      CONFIG_ECC_SRAM_ADDR_SHIFT, ECC_SRAM_ADDR);
+
 	/* first scan to find the device and get the page size */
 	if (nand_scan_ident(mtd, 1, NULL)) {
 		err = -ENXIO;
@@ -627,6 +711,36 @@ static int nfc_probe(struct platform_device *pdev)
 	page_sz += cfg.width == 16 ? 1 : 0;
 	nfc_write(mtd, NFC_SECTOR_SIZE, page_sz);
 
+	if (cfg.hardware_ecc) {
+		if (mtd->writesize != PAGE_2K && mtd->oobsize < 64) {
+			dev_err(nfc->dev, "Unsupported flash with hwecc\n");
+			err = -ENXIO;
+			goto error;
+		}
+
+		chip->ecc.layout = &nfc_ecc45;
+
+		/* propagate ecc.layout to mtd_info */
+		mtd->ecclayout = chip->ecc.layout;
+		chip->ecc.calculate = nfc_calculate_ecc;
+		chip->ecc.hwctl = nfc_enable_hwecc;
+		chip->ecc.correct = nfc_correct_data;
+		chip->ecc.mode = NAND_ECC_HW;
+
+		chip->ecc.bytes = 45;
+		chip->ecc.size = PAGE_2K;
+		chip->ecc.strength = 24;
+
+		/* set ECC mode to 45 bytes OOB with 24 bits correction */
+		nfc_set_field(mtd, NFC_FLASH_CONFIG,
+				CONFIG_ECC_MODE_MASK,
+				CONFIG_ECC_MODE_SHIFT, ECC_45_BYTE);
+
+		/* Enable ECC_STATUS */
+		nfc_set(mtd, NFC_FLASH_CONFIG, CONFIG_ECC_SRAM_REQ_BIT);
+
+	}
+
 	/* second phase scan */
 	if (nand_scan_tail(mtd)) {
 		err = -ENXIO;
-- 
1.8.0.2

Re: [RFC 2/5] mtd:fsl_nfc: Add hardware 45 byte BHC-ECC support for 24 bit corrections.

[Stefan Agner]

Hi Bill,

On one of our Colibri VF61 modules I discovered an issue using this
driver:

[    0.758327] ECC failed to correct all errors (ebd9fd80)
[    0.767005] ECC failed to correct all errors (ebd9fd80)
[    0.775525] ECC failed to correct all errors (ebd9fd80)
[    0.784004] ECC failed to correct all errors (ebd9fd80)
[    0.791938] UBI error: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 28:2048, 
read 2048 bytes

That page supposed to be empty, and I got several of this messages.
Hence I did not believe that they have really ECC errors, so I digged a
bit deeper:

@@ -673,11 +673,13 @@ static int fsl_nfc_check_ecc_status(struct
mtd_info *mtd, u_char *dat)
                return ecc_count;
 
        /* If 'ecc_count' zero or less then buffer is all 0xff or
erased. */
-       flip = count_written_bits(dat, chip->ecc.size, ecc_count);
+       flip = count_written_bits(dat, chip->ecc.size, 100);
 
 
        if (flip > ecc_count) {
-               printk("ECC failed to correct all errors (%08x)\n",
ecc_status);
+               printk("ECC failed to correct all errors (%08x, flips
%d)\n",
+                               ecc_status, flip);
                return -1;
        }


[    0.758624] ECC failed to correct all errors (ebdded80, flips 1)
[    0.768123] ECC failed to correct all errors (ebdded80, flips 1)
[    0.777468] ECC failed to correct all errors (ebdded80, flips 1)
[    0.786773] ECC failed to correct all errors (ebdded80, flips 1)
[    0.795535] UBI error: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 28:2048, read 2048 bytes
[    1.462183] ECC failed to correct all errors (ebdded82, flips 3)
[    1.471623] ECC failed to correct all errors (ebdded82, flips 3)
[    1.481025] ECC failed to correct all errors (ebdded82, flips 3)
[    1.490336] ECC failed to correct all errors (ebdded82, flips 3)
[    1.498953] UBI error: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 664:2048, read 2048 bytes
[    1.551421] ECC failed to correct all errors (ebdded80, flips 1)
[    1.560616] ECC failed to correct all errors (ebdded80, flips 1)
[    1.569695] ECC failed to correct all errors (ebdded80, flips 1)
[    1.578711] ECC failed to correct all errors (ebdded80, flips 1)
[    1.587192] UBI error: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 666:2048, read 2048 bytes
[    1.744612] ECC failed to correct all errors (ebdded81, flips 2)
[    1.753943] ECC failed to correct all errors (ebdded81, flips 2)
[    1.763146] ECC failed to correct all errors (ebdded81, flips 2)
[    1.772247] ECC failed to correct all errors (ebdded81, flips 2)
[    1.780722] UBI error: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 776:2048, read 2048 bytes


Interesting thought, the returned ecc_count is exactly one below the
actual flipped bytes counted...

One comment below regarding this...

Am 2014-01-09 00:07, schrieb Bill Pringlemeir:
> Signed-off-by: Bill Pringlemeir <bpringlemeir@nbsps.com>
> ---
>  drivers/mtd/nand/Kconfig   |   5 +-
>  drivers/mtd/nand/fsl_nfc.c | 114 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 117 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index 7e0c695..8ac9923 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -458,8 +458,9 @@ config MTD_NAND_FSL_NFC
>  	help
>  	  Enables support for NAND Flash controller on some Freescale
>  	  processors like the MPC5125, VF610, MCF54418, and Kinetis K70.
> -	  The driver supports a maximum 2k page size.
> -	  The driver currently does not support hardware ECC.
> +	  The driver supports a maximum 2k page size.  With 2k pages and
> +	  64 bytes or more of OOB, hardware ECC with 24bit correction is
> +	  supported.  Only MTD_OF (device tree) can enable the hardware ECC.
>  
>  config MTD_NAND_MXC
>  	tristate "MXC NAND support"
> diff --git a/drivers/mtd/nand/fsl_nfc.c b/drivers/mtd/nand/fsl_nfc.c
> index eb4e353..703511d 100644
> --- a/drivers/mtd/nand/fsl_nfc.c
> +++ b/drivers/mtd/nand/fsl_nfc.c
> @@ -17,6 +17,7 @@
>   * - Untested on MPC5125 and M54418.
>   * - DMA not used.
>   * - 2K pages or less.
> + * - Only 2K page w. 64+OOB and hardware ECC.
>   */
>  
>  #include <linux/module.h>
> @@ -68,6 +69,7 @@
>  
>  /* NFC ECC mode define */
>  #define ECC_BYPASS			0
> +#define ECC_45_BYTE			6
>  
>  /*** Register Mask and bit definitions */
>  
> @@ -100,6 +102,8 @@
>  #define STATUS_BYTE1_MASK			0x000000FF
>  
>  /* NFC_FLASH_CONFIG Field */
> +#define CONFIG_ECC_SRAM_ADDR_MASK		0x7FC00000
> +#define CONFIG_ECC_SRAM_ADDR_SHIFT		22
>  #define CONFIG_ECC_SRAM_REQ_BIT			(1<<21)
>  #define CONFIG_DMA_REQ_BIT			(1<<20)
>  #define CONFIG_ECC_MODE_MASK			0x000E0000
> @@ -120,6 +124,11 @@
>  
>  #define NFC_TIMEOUT		(HZ)
>  
> +/* ECC status placed at end of buffers. */
> +#define ECC_SRAM_ADDR	((PAGE_2K+256-8) >> 3)
> +#define ECC_STATUS_MASK	0x80
> +#define ECC_ERR_COUNT	0x3F
> +
>  struct fsl_nfc {
>  	struct mtd_info	   mtd;
>  	struct nand_chip   chip;
> @@ -160,6 +169,19 @@ static struct nand_bbt_descr bbt_mirror_descr = {
>  	.pattern = mirror_pattern,
>  };
>  
> +static struct nand_ecclayout nfc_ecc45 = {
> +	.eccbytes = 45,
> +	.eccpos = {19, 20, 21, 22, 23,
> +		   24, 25, 26, 27, 28, 29, 30, 31,
> +		   32, 33, 34, 35, 36, 37, 38, 39,
> +		   40, 41, 42, 43, 44, 45, 46, 47,
> +		   48, 49, 50, 51, 52, 53, 54, 55,
> +		   56, 57, 58, 59, 60, 61, 62, 63},
> +	.oobfree = {
> +		{.offset = 8,
> +		 .length = 11} }
> +};
> +
>  static u32 nfc_read(struct mtd_info *mtd, uint reg)
>  {
>  	struct fsl_nfc *nfc = mtd_to_nfc(mtd);
> @@ -458,7 +480,61 @@ nfc_select_chip(struct mtd_info *mtd, int chip)
>  #endif
>  }
>  
> +/* Count the number of 0's in buff upto max_bits */
> +static int count_written_bits(uint8_t *buff, int size, int max_bits)
> +{
> +	int k, written_bits = 0;
> +
> +	for (k = 0; k < size; k++) {
> +		written_bits += hweight8(~buff[k]);
> +		if (written_bits > max_bits)
> +			break;
> +	}
> +
> +	return written_bits;
> +}
> +
> +static int nfc_correct_data(struct mtd_info *mtd, u_char *dat,
> +				 u_char *read_ecc, u_char *calc_ecc)
> +{
> +	struct fsl_nfc *nfc = mtd_to_nfc(mtd);
> +	u32 ecc_status;
> +	u8 ecc_count;
> +	int flip;
> +
> +	/* Errata: ECC status is stored at NFC_CFG[ECCADD] +4 for
> +	   little-endian and +7 for big-endian SOC.  Access as 32 bits
> +	   and use low byte.
> +	*/
> +	ecc_status = __raw_readl(nfc->regs + ECC_SRAM_ADDR * 8 + 4);
> +	ecc_count = ecc_status & ECC_ERR_COUNT;
> +	if (!(ecc_status & ECC_STATUS_MASK))
> +		return ecc_count;
> +
> +	/* If 'ecc_count' zero or less then buffer is all 0xff or erased. */
> +	flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);

Also, I could not find that the reference manual states that ecc_count
represents the amount of flipped byte in a empty page. Is this given by
the ECC algorithm?



> +
> +	/* ECC failed. */
> +	if (flip > ecc_count)
> +		return -1;
> +
> +	/* Erased page. */
> +	memset(dat, 0xff, nfc->chip.ecc.size);
> +	return 0;
> +}
> +
> +static int nfc_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
> +				  u_char *ecc_code)
> +{
> +	return 0;
> +}
> +
> +static void nfc_enable_hwecc(struct mtd_info *mtd, int mode)
> +{
> +}
> +
>  struct nfc_config {
> +	int hardware_ecc;
>  	int width;
>  	int flash_bbt;
>  	u32 clkrate;
> @@ -483,6 +559,9 @@ static int __init nfc_probe_dt(struct device *dev,
> struct nfc_config *cfg)
>  	if (!np)
>  		return 1;
>  
> +	if (of_get_nand_ecc_mode(np) >= NAND_ECC_HW)
> +		cfg->hardware_ecc = 1;
> +
>  	cfg->flash_bbt = of_get_nand_on_flash_bbt(np);
>  
>  	if (!of_property_read_u32(np, "clock-frequency", &clkrate))
> @@ -608,6 +687,11 @@ static int nfc_probe(struct platform_device *pdev)
>  	nfc_set_field(mtd, NFC_FLASH_CONFIG, CONFIG_PAGE_CNT_MASK,
>  			CONFIG_PAGE_CNT_SHIFT, 1);
>  
> +	/* Set ECC_STATUS offset */
> +	nfc_set_field(mtd, NFC_FLASH_CONFIG,
> +		      CONFIG_ECC_SRAM_ADDR_MASK,
> +		      CONFIG_ECC_SRAM_ADDR_SHIFT, ECC_SRAM_ADDR);
> +
>  	/* first scan to find the device and get the page size */
>  	if (nand_scan_ident(mtd, 1, NULL)) {
>  		err = -ENXIO;
> @@ -627,6 +711,36 @@ static int nfc_probe(struct platform_device *pdev)
>  	page_sz += cfg.width == 16 ? 1 : 0;
>  	nfc_write(mtd, NFC_SECTOR_SIZE, page_sz);
>  
> +	if (cfg.hardware_ecc) {
> +		if (mtd->writesize != PAGE_2K && mtd->oobsize < 64) {
> +			dev_err(nfc->dev, "Unsupported flash with hwecc\n");
> +			err = -ENXIO;
> +			goto error;
> +		}
> +
> +		chip->ecc.layout = &nfc_ecc45;
> +
> +		/* propagate ecc.layout to mtd_info */
> +		mtd->ecclayout = chip->ecc.layout;
> +		chip->ecc.calculate = nfc_calculate_ecc;
> +		chip->ecc.hwctl = nfc_enable_hwecc;
> +		chip->ecc.correct = nfc_correct_data;
> +		chip->ecc.mode = NAND_ECC_HW;
> +
> +		chip->ecc.bytes = 45;
> +		chip->ecc.size = PAGE_2K;
> +		chip->ecc.strength = 24;
> +
> +		/* set ECC mode to 45 bytes OOB with 24 bits correction */
> +		nfc_set_field(mtd, NFC_FLASH_CONFIG,
> +				CONFIG_ECC_MODE_MASK,
> +				CONFIG_ECC_MODE_SHIFT, ECC_45_BYTE);
> +
> +		/* Enable ECC_STATUS */
> +		nfc_set(mtd, NFC_FLASH_CONFIG, CONFIG_ECC_SRAM_REQ_BIT);
> +
> +	}
> +
>  	/* second phase scan */
>  	if (nand_scan_tail(mtd)) {
>  		err = -ENXIO;


--
Stefan

[RFC 2/5] mtd:fsl_nfc: Add hardware 45 byte BHC-ECC support for 24 bit corrections.

[Stefan Agner]

Hi Bill,

On one of our Colibri VF61 modules I discovered an issue using this
driver:

[    0.758327] ECC failed to correct all errors (ebd9fd80)
[    0.767005] ECC failed to correct all errors (ebd9fd80)
[    0.775525] ECC failed to correct all errors (ebd9fd80)
[    0.784004] ECC failed to correct all errors (ebd9fd80)
[    0.791938] UBI error: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 28:2048, 
read 2048 bytes

That page supposed to be empty, and I got several of this messages.
Hence I did not believe that they have really ECC errors, so I digged a
bit deeper:

@@ -673,11 +673,13 @@ static int fsl_nfc_check_ecc_status(struct
mtd_info *mtd, u_char *dat)
                return ecc_count;
 
        /* If 'ecc_count' zero or less then buffer is all 0xff or
erased. */
-       flip = count_written_bits(dat, chip->ecc.size, ecc_count);
+       flip = count_written_bits(dat, chip->ecc.size, 100);
 
 
        if (flip > ecc_count) {
-               printk("ECC failed to correct all errors (%08x)\n",
ecc_status);
+               printk("ECC failed to correct all errors (%08x, flips
%d)\n",
+                               ecc_status, flip);
                return -1;
        }


[    0.758624] ECC failed to correct all errors (ebdded80, flips 1)
[    0.768123] ECC failed to correct all errors (ebdded80, flips 1)
[    0.777468] ECC failed to correct all errors (ebdded80, flips 1)
[    0.786773] ECC failed to correct all errors (ebdded80, flips 1)
[    0.795535] UBI error: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 28:2048, read 2048 bytes
[    1.462183] ECC failed to correct all errors (ebdded82, flips 3)
[    1.471623] ECC failed to correct all errors (ebdded82, flips 3)
[    1.481025] ECC failed to correct all errors (ebdded82, flips 3)
[    1.490336] ECC failed to correct all errors (ebdded82, flips 3)
[    1.498953] UBI error: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 664:2048, read 2048 bytes
[    1.551421] ECC failed to correct all errors (ebdded80, flips 1)
[    1.560616] ECC failed to correct all errors (ebdded80, flips 1)
[    1.569695] ECC failed to correct all errors (ebdded80, flips 1)
[    1.578711] ECC failed to correct all errors (ebdded80, flips 1)
[    1.587192] UBI error: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 666:2048, read 2048 bytes
[    1.744612] ECC failed to correct all errors (ebdded81, flips 2)
[    1.753943] ECC failed to correct all errors (ebdded81, flips 2)
[    1.763146] ECC failed to correct all errors (ebdded81, flips 2)
[    1.772247] ECC failed to correct all errors (ebdded81, flips 2)
[    1.780722] UBI error: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 776:2048, read 2048 bytes


Interesting thought, the returned ecc_count is exactly one below the
actual flipped bytes counted...

One comment below regarding this...

Am 2014-01-09 00:07, schrieb Bill Pringlemeir:
> Signed-off-by: Bill Pringlemeir <bpringlemeir@nbsps.com>
> ---
>  drivers/mtd/nand/Kconfig   |   5 +-
>  drivers/mtd/nand/fsl_nfc.c | 114 +++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 117 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index 7e0c695..8ac9923 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -458,8 +458,9 @@ config MTD_NAND_FSL_NFC
>  	help
>  	  Enables support for NAND Flash controller on some Freescale
>  	  processors like the MPC5125, VF610, MCF54418, and Kinetis K70.
> -	  The driver supports a maximum 2k page size.
> -	  The driver currently does not support hardware ECC.
> +	  The driver supports a maximum 2k page size.  With 2k pages and
> +	  64 bytes or more of OOB, hardware ECC with 24bit correction is
> +	  supported.  Only MTD_OF (device tree) can enable the hardware ECC.
>  
>  config MTD_NAND_MXC
>  	tristate "MXC NAND support"
> diff --git a/drivers/mtd/nand/fsl_nfc.c b/drivers/mtd/nand/fsl_nfc.c
> index eb4e353..703511d 100644
> --- a/drivers/mtd/nand/fsl_nfc.c
> +++ b/drivers/mtd/nand/fsl_nfc.c
> @@ -17,6 +17,7 @@
>   * - Untested on MPC5125 and M54418.
>   * - DMA not used.
>   * - 2K pages or less.
> + * - Only 2K page w. 64+OOB and hardware ECC.
>   */
>  
>  #include <linux/module.h>
> @@ -68,6 +69,7 @@
>  
>  /* NFC ECC mode define */
>  #define ECC_BYPASS			0
> +#define ECC_45_BYTE			6
>  
>  /*** Register Mask and bit definitions */
>  
> @@ -100,6 +102,8 @@
>  #define STATUS_BYTE1_MASK			0x000000FF
>  
>  /* NFC_FLASH_CONFIG Field */
> +#define CONFIG_ECC_SRAM_ADDR_MASK		0x7FC00000
> +#define CONFIG_ECC_SRAM_ADDR_SHIFT		22
>  #define CONFIG_ECC_SRAM_REQ_BIT			(1<<21)
>  #define CONFIG_DMA_REQ_BIT			(1<<20)
>  #define CONFIG_ECC_MODE_MASK			0x000E0000
> @@ -120,6 +124,11 @@
>  
>  #define NFC_TIMEOUT		(HZ)
>  
> +/* ECC status placed at end of buffers. */
> +#define ECC_SRAM_ADDR	((PAGE_2K+256-8) >> 3)
> +#define ECC_STATUS_MASK	0x80
> +#define ECC_ERR_COUNT	0x3F
> +
>  struct fsl_nfc {
>  	struct mtd_info	   mtd;
>  	struct nand_chip   chip;
> @@ -160,6 +169,19 @@ static struct nand_bbt_descr bbt_mirror_descr = {
>  	.pattern = mirror_pattern,
>  };
>  
> +static struct nand_ecclayout nfc_ecc45 = {
> +	.eccbytes = 45,
> +	.eccpos = {19, 20, 21, 22, 23,
> +		   24, 25, 26, 27, 28, 29, 30, 31,
> +		   32, 33, 34, 35, 36, 37, 38, 39,
> +		   40, 41, 42, 43, 44, 45, 46, 47,
> +		   48, 49, 50, 51, 52, 53, 54, 55,
> +		   56, 57, 58, 59, 60, 61, 62, 63},
> +	.oobfree = {
> +		{.offset = 8,
> +		 .length = 11} }
> +};
> +
>  static u32 nfc_read(struct mtd_info *mtd, uint reg)
>  {
>  	struct fsl_nfc *nfc = mtd_to_nfc(mtd);
> @@ -458,7 +480,61 @@ nfc_select_chip(struct mtd_info *mtd, int chip)
>  #endif
>  }
>  
> +/* Count the number of 0's in buff upto max_bits */
> +static int count_written_bits(uint8_t *buff, int size, int max_bits)
> +{
> +	int k, written_bits = 0;
> +
> +	for (k = 0; k < size; k++) {
> +		written_bits += hweight8(~buff[k]);
> +		if (written_bits > max_bits)
> +			break;
> +	}
> +
> +	return written_bits;
> +}
> +
> +static int nfc_correct_data(struct mtd_info *mtd, u_char *dat,
> +				 u_char *read_ecc, u_char *calc_ecc)
> +{
> +	struct fsl_nfc *nfc = mtd_to_nfc(mtd);
> +	u32 ecc_status;
> +	u8 ecc_count;
> +	int flip;
> +
> +	/* Errata: ECC status is stored at NFC_CFG[ECCADD] +4 for
> +	   little-endian and +7 for big-endian SOC.  Access as 32 bits
> +	   and use low byte.
> +	*/
> +	ecc_status = __raw_readl(nfc->regs + ECC_SRAM_ADDR * 8 + 4);
> +	ecc_count = ecc_status & ECC_ERR_COUNT;
> +	if (!(ecc_status & ECC_STATUS_MASK))
> +		return ecc_count;
> +
> +	/* If 'ecc_count' zero or less then buffer is all 0xff or erased. */
> +	flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);

Also, I could not find that the reference manual states that ecc_count
represents the amount of flipped byte in a empty page. Is this given by
the ECC algorithm?



> +
> +	/* ECC failed. */
> +	if (flip > ecc_count)
> +		return -1;
> +
> +	/* Erased page. */
> +	memset(dat, 0xff, nfc->chip.ecc.size);
> +	return 0;
> +}
> +
> +static int nfc_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
> +				  u_char *ecc_code)
> +{
> +	return 0;
> +}
> +
> +static void nfc_enable_hwecc(struct mtd_info *mtd, int mode)
> +{
> +}
> +
>  struct nfc_config {
> +	int hardware_ecc;
>  	int width;
>  	int flash_bbt;
>  	u32 clkrate;
> @@ -483,6 +559,9 @@ static int __init nfc_probe_dt(struct device *dev,
> struct nfc_config *cfg)
>  	if (!np)
>  		return 1;
>  
> +	if (of_get_nand_ecc_mode(np) >= NAND_ECC_HW)
> +		cfg->hardware_ecc = 1;
> +
>  	cfg->flash_bbt = of_get_nand_on_flash_bbt(np);
>  
>  	if (!of_property_read_u32(np, "clock-frequency", &clkrate))
> @@ -608,6 +687,11 @@ static int nfc_probe(struct platform_device *pdev)
>  	nfc_set_field(mtd, NFC_FLASH_CONFIG, CONFIG_PAGE_CNT_MASK,
>  			CONFIG_PAGE_CNT_SHIFT, 1);
>  
> +	/* Set ECC_STATUS offset */
> +	nfc_set_field(mtd, NFC_FLASH_CONFIG,
> +		      CONFIG_ECC_SRAM_ADDR_MASK,
> +		      CONFIG_ECC_SRAM_ADDR_SHIFT, ECC_SRAM_ADDR);
> +
>  	/* first scan to find the device and get the page size */
>  	if (nand_scan_ident(mtd, 1, NULL)) {
>  		err = -ENXIO;
> @@ -627,6 +711,36 @@ static int nfc_probe(struct platform_device *pdev)
>  	page_sz += cfg.width == 16 ? 1 : 0;
>  	nfc_write(mtd, NFC_SECTOR_SIZE, page_sz);
>  
> +	if (cfg.hardware_ecc) {
> +		if (mtd->writesize != PAGE_2K && mtd->oobsize < 64) {
> +			dev_err(nfc->dev, "Unsupported flash with hwecc\n");
> +			err = -ENXIO;
> +			goto error;
> +		}
> +
> +		chip->ecc.layout = &nfc_ecc45;
> +
> +		/* propagate ecc.layout to mtd_info */
> +		mtd->ecclayout = chip->ecc.layout;
> +		chip->ecc.calculate = nfc_calculate_ecc;
> +		chip->ecc.hwctl = nfc_enable_hwecc;
> +		chip->ecc.correct = nfc_correct_data;
> +		chip->ecc.mode = NAND_ECC_HW;
> +
> +		chip->ecc.bytes = 45;
> +		chip->ecc.size = PAGE_2K;
> +		chip->ecc.strength = 24;
> +
> +		/* set ECC mode to 45 bytes OOB with 24 bits correction */
> +		nfc_set_field(mtd, NFC_FLASH_CONFIG,
> +				CONFIG_ECC_MODE_MASK,
> +				CONFIG_ECC_MODE_SHIFT, ECC_45_BYTE);
> +
> +		/* Enable ECC_STATUS */
> +		nfc_set(mtd, NFC_FLASH_CONFIG, CONFIG_ECC_SRAM_REQ_BIT);
> +
> +	}
> +
>  	/* second phase scan */
>  	if (nand_scan_tail(mtd)) {
>  		err = -ENXIO;


--
Stefan

Re: [RFC 2/5] mtd:fsl_nfc: Add hardware 45 byte BHC-ECC support for 24 bit corrections.

[Bill Pringlemeir]

On 17 Sep 2014, stefan@agner.ch wrote:

> On one of our Colibri VF61 modules I discovered an issue using this
> driver:

> [    0.758327] ECC failed to correct all errors (ebd9fd80)
> [    0.767005] ECC failed to correct all errors (ebd9fd80)
> [    0.775525] ECC failed to correct all errors (ebd9fd80)
> [    0.784004] ECC failed to correct all errors (ebd9fd80)
> [    0.791938] UBI error: ubi_io_read: error -74 (ECC error) while
> reading 2048 bytes from PEB 28:2048, 
> read 2048 bytes

> That page supposed to be empty, and I got several of this messages.
> Hence I did not believe that they have really ECC errors, so I digged
> a bit deeper:

>>> -673,11 +673,13 @@ static int fsl_nfc_check_ecc_status(struct
> mtd_info *mtd, u_char *dat)
> return ecc_count;

> /* If 'ecc_count' zero or less then buffer is all 0xff or
> erased. */
> -       flip = count_written_bits(dat, chip->ecc.size, ecc_count);
> +       flip = count_written_bits(dat, chip->ecc.size, 100);

> if (flip > ecc_count) {
> -               printk("ECC failed to correct all errors (%08x)\n",
> ecc_status);
> +               printk("ECC failed to correct all errors (%08x, flips
> %d)\n",
> +                               ecc_status, flip);
> return -1;
> }

> [    0.758624] ECC failed to correct all errors (ebdded80, flips 1)
> [    0.768123] ECC failed to correct all errors (ebdded80, flips 1)
> [    0.777468] ECC failed to correct all errors (ebdded80, flips 1)
> [    0.786773] ECC failed to correct all errors (ebdded80, flips 1)
> [    0.795535] UBI error: ubi_io_read: error -74 (ECC error) while
> reading 2048 bytes from PEB 28:2048, read 2048 bytes

[snip]

> Interesting thought, the returned ecc_count is exactly one below the
> actual flipped bytes counted...
>
> One comment below regarding this...

[snip]

>> +
>> +	/* If 'ecc_count' zero or less then buffer is all 0xff or erased. */
>> +	flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);

> Also, I could not find that the reference manual states that ecc_count
> represents the amount of flipped byte in a empty page. Is this given
> by the ECC algorithm?

I was using this information.

   Table 31-18. ECC Status Word Field Definition

   7            0 Page has been successfully corrected
   CORFAIL      1 Page is uncorrectable

   5–0          Number of errors that have been corrected in this page
   ERROR_COUNT

This is from the Vybrid RM, but the MPC5125RM has the same information.

I have definitely tested the detection of 'erased pages'.  However, I
don't know that I ever had actual bit flips.

The ECC controller has no idea whether the page is empty or not.  Are
you saying you have an erased page with bit flips?  I have definitely
not tested this.

>> +	/* If 'ecc_count' zero or less then buffer is all 0xff or erased. */
>> +	flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);

There are two issues.

 1) erased page with some physical flipped bits (zero).
 2) erased page were controller flipped some bits.

Currently, the code is only handling case 2 (that is what the controller
counts).  I believe that your physical page has actual 'stuck at zero'
bits.  The current driver gives up.  If you want to handle this then we
should replace the lines,

> +	/* ECC failed. */
> +	if (flip > ecc_count)
> +		return -1;

With something like,

 +	/* Not completely empty. */
 +	if (flip > ecc_count) {
 +           re_read_page_w_ecc_off();
 +           if (count_written_bits() > strength) /* strength/2 ? */
 +		return -1;
 +      }

There is also a discussion on this here,

http://lists.infradead.org/pipermail/linux-mtd/2014-March/052507.html
http://lists.infradead.org/pipermail/linux-mtd/2014-March/052508.html
http://lists.infradead.org/pipermail/linux-mtd/2014-March/052509.html
http://lists.infradead.org/pipermail/linux-mtd/2014-March/052526.html
http://lists.infradead.org/pipermail/linux-mtd/2014-March/052527.html
http://lists.infradead.org/pipermail/linux-mtd/2014-March/052619.html
... etc.  Use the view thread.

Especially,
http://lists.infradead.org/pipermail/linux-mtd/2014-March/052647.html

Here Brian says that 3b applies to SLC NAND.  I guess this is you :).

It is fairly common to read an erased page.  Doing 'raw reads' all the
time on an erased page will slow the file system.  However, doing
re-reads for an erased page with bit flips is probably fairly uncommon.
A flash in this state maybe near EOL or the sector was bad from the
factory but not marked so.

I am chasing an DDR2 issue on another CPU and haven't had much more time
to the look at the Vybrid nor follow the MTD mailing list.  I don't know
if the 'NAND_ECC_NEED_CHECK_FF' feature has been merged?  It may also
solve the issue.

Regards,
Bill Pringlemeir.

[RFC 2/5] mtd:fsl_nfc: Add hardware 45 byte BHC-ECC support for 24 bit corrections.

[Bill Pringlemeir]

On 17 Sep 2014, stefan at agner.ch wrote:

> On one of our Colibri VF61 modules I discovered an issue using this
> driver:

> [    0.758327] ECC failed to correct all errors (ebd9fd80)
> [    0.767005] ECC failed to correct all errors (ebd9fd80)
> [    0.775525] ECC failed to correct all errors (ebd9fd80)
> [    0.784004] ECC failed to correct all errors (ebd9fd80)
> [    0.791938] UBI error: ubi_io_read: error -74 (ECC error) while
> reading 2048 bytes from PEB 28:2048, 
> read 2048 bytes

> That page supposed to be empty, and I got several of this messages.
> Hence I did not believe that they have really ECC errors, so I digged
> a bit deeper:

>>> -673,11 +673,13 @@ static int fsl_nfc_check_ecc_status(struct
> mtd_info *mtd, u_char *dat)
> return ecc_count;

> /* If 'ecc_count' zero or less then buffer is all 0xff or
> erased. */
> -       flip = count_written_bits(dat, chip->ecc.size, ecc_count);
> +       flip = count_written_bits(dat, chip->ecc.size, 100);

> if (flip > ecc_count) {
> -               printk("ECC failed to correct all errors (%08x)\n",
> ecc_status);
> +               printk("ECC failed to correct all errors (%08x, flips
> %d)\n",
> +                               ecc_status, flip);
> return -1;
> }

> [    0.758624] ECC failed to correct all errors (ebdded80, flips 1)
> [    0.768123] ECC failed to correct all errors (ebdded80, flips 1)
> [    0.777468] ECC failed to correct all errors (ebdded80, flips 1)
> [    0.786773] ECC failed to correct all errors (ebdded80, flips 1)
> [    0.795535] UBI error: ubi_io_read: error -74 (ECC error) while
> reading 2048 bytes from PEB 28:2048, read 2048 bytes

[snip]

> Interesting thought, the returned ecc_count is exactly one below the
> actual flipped bytes counted...
>
> One comment below regarding this...

[snip]

>> +
>> +	/* If 'ecc_count' zero or less then buffer is all 0xff or erased. */
>> +	flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);

> Also, I could not find that the reference manual states that ecc_count
> represents the amount of flipped byte in a empty page. Is this given
> by the ECC algorithm?

I was using this information.

   Table 31-18. ECC Status Word Field Definition

   7            0 Page has been successfully corrected
   CORFAIL      1 Page is uncorrectable

   5?0          Number of errors that have been corrected in this page
   ERROR_COUNT

This is from the Vybrid RM, but the MPC5125RM has the same information.

I have definitely tested the detection of 'erased pages'.  However, I
don't know that I ever had actual bit flips.

The ECC controller has no idea whether the page is empty or not.  Are
you saying you have an erased page with bit flips?  I have definitely
not tested this.

>> +	/* If 'ecc_count' zero or less then buffer is all 0xff or erased. */
>> +	flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);

There are two issues.

 1) erased page with some physical flipped bits (zero).
 2) erased page were controller flipped some bits.

Currently, the code is only handling case 2 (that is what the controller
counts).  I believe that your physical page has actual 'stuck at zero'
bits.  The current driver gives up.  If you want to handle this then we
should replace the lines,

> +	/* ECC failed. */
> +	if (flip > ecc_count)
> +		return -1;

With something like,

 +	/* Not completely empty. */
 +	if (flip > ecc_count) {
 +           re_read_page_w_ecc_off();
 +           if (count_written_bits() > strength) /* strength/2 ? */
 +		return -1;
 +      }

There is also a discussion on this here,

http://lists.infradead.org/pipermail/linux-mtd/2014-March/052507.html
http://lists.infradead.org/pipermail/linux-mtd/2014-March/052508.html
http://lists.infradead.org/pipermail/linux-mtd/2014-March/052509.html
http://lists.infradead.org/pipermail/linux-mtd/2014-March/052526.html
http://lists.infradead.org/pipermail/linux-mtd/2014-March/052527.html
http://lists.infradead.org/pipermail/linux-mtd/2014-March/052619.html
... etc.  Use the view thread.

Especially,
http://lists.infradead.org/pipermail/linux-mtd/2014-March/052647.html

Here Brian says that 3b applies to SLC NAND.  I guess this is you :).

It is fairly common to read an erased page.  Doing 'raw reads' all the
time on an erased page will slow the file system.  However, doing
re-reads for an erased page with bit flips is probably fairly uncommon.
A flash in this state maybe near EOL or the sector was bad from the
factory but not marked so.

I am chasing an DDR2 issue on another CPU and haven't had much more time
to the look at the Vybrid nor follow the MTD mailing list.  I don't know
if the 'NAND_ECC_NEED_CHECK_FF' feature has been merged?  It may also
solve the issue.

Regards,
Bill Pringlemeir.

Re: [RFC 2/5] mtd:fsl_nfc: Add hardware 45 byte BHC-ECC support for 24 bit corrections.

[Stefan Agner]

Am 2014-09-17 20:06, schrieb Bill Pringlemeir:
> On 17 Sep 2014, stefan@agner.ch wrote:
> 
>> On one of our Colibri VF61 modules I discovered an issue using this
>> driver:
> 
>> [    0.758327] ECC failed to correct all errors (ebd9fd80)
>> [    0.767005] ECC failed to correct all errors (ebd9fd80)
>> [    0.775525] ECC failed to correct all errors (ebd9fd80)
>> [    0.784004] ECC failed to correct all errors (ebd9fd80)
>> [    0.791938] UBI error: ubi_io_read: error -74 (ECC error) while
>> reading 2048 bytes from PEB 28:2048,
>> read 2048 bytes
> 
>> That page supposed to be empty, and I got several of this messages.
>> Hence I did not believe that they have really ECC errors, so I digged
>> a bit deeper:
> 
>>>> -673,11 +673,13 @@ static int fsl_nfc_check_ecc_status(struct
>> mtd_info *mtd, u_char *dat)
>> return ecc_count;
> 
>> /* If 'ecc_count' zero or less then buffer is all 0xff or
>> erased. */
>> -       flip = count_written_bits(dat, chip->ecc.size, ecc_count);
>> +       flip = count_written_bits(dat, chip->ecc.size, 100);
> 
>> if (flip > ecc_count) {
>> -               printk("ECC failed to correct all errors (%08x)\n",
>> ecc_status);
>> +               printk("ECC failed to correct all errors (%08x, flips
>> %d)\n",
>> +                               ecc_status, flip);
>> return -1;
>> }
> 
>> [    0.758624] ECC failed to correct all errors (ebdded80, flips 1)
>> [    0.768123] ECC failed to correct all errors (ebdded80, flips 1)
>> [    0.777468] ECC failed to correct all errors (ebdded80, flips 1)
>> [    0.786773] ECC failed to correct all errors (ebdded80, flips 1)
>> [    0.795535] UBI error: ubi_io_read: error -74 (ECC error) while
>> reading 2048 bytes from PEB 28:2048, read 2048 bytes
> 
> [snip]
> 
>> Interesting thought, the returned ecc_count is exactly one below the
>> actual flipped bytes counted...
>>
>> One comment below regarding this...
> 
> [snip]
> 
>>> +
>>> +	/* If 'ecc_count' zero or less then buffer is all 0xff or erased. */
>>> +	flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);
> 
>> Also, I could not find that the reference manual states that ecc_count
>> represents the amount of flipped byte in a empty page. Is this given
>> by the ECC algorithm?
> 
> I was using this information.
> 
>    Table 31-18. ECC Status Word Field Definition
> 
>    7            0 Page has been successfully corrected
>    CORFAIL      1 Page is uncorrectable
> 
>    5–0          Number of errors that have been corrected in this page
>    ERROR_COUNT
> 
> This is from the Vybrid RM, but the MPC5125RM has the same information.
> 
> I have definitely tested the detection of 'erased pages'.  However, I
> don't know that I ever had actual bit flips.
> 
> The ECC controller has no idea whether the page is empty or not.  Are
> you saying you have an erased page with bit flips?  I have definitely
> not tested this.

Yes, that's what it looks like. Note that this output is on first boot
after flashing the device (with the UBI auto-grow option). I guess UBI
is reading all pages once to verify that they are empty.

>>> +	/* If 'ecc_count' zero or less then buffer is all 0xff or erased. */
>>> +	flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);
> 
> There are two issues.
> 
>  1) erased page with some physical flipped bits (zero).
>  2) erased page were controller flipped some bits.

I did not know that issue 2 exists. How can 2 happen on a erased page?
With a 'stuck at zero' in OOB?

> Currently, the code is only handling case 2 (that is what the controller
> counts).  I believe that your physical page has actual 'stuck at zero'
> bits.  The current driver gives up.  If you want to handle this then we
> should replace the lines,
> 

I assumed that your code assumes that the controller returns the flipped
bit count when reading an erase page (case 1), which I did not found in
the documentation. 

>> +	/* ECC failed. */
>> +	if (flip > ecc_count)
>> +		return -1;
> 
> With something like,
> 
>  +	/* Not completely empty. */
>  +	if (flip > ecc_count) {
>  +           re_read_page_w_ecc_off();
>  +           if (count_written_bits() > strength) /* strength/2 ? */
>  +		return -1;
>  +      }
> 
> There is also a discussion on this here,
> 
> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052507.html
> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052508.html
> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052509.html
> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052526.html
> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052527.html
> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052619.html
> ... etc.  Use the view thread.
> 
> Especially,
> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052647.html
> 
> Here Brian says that 3b applies to SLC NAND.  I guess this is you :).

Yes, we are using Macronix SLC NAND.

> 
> It is fairly common to read an erased page.  Doing 'raw reads' all the
> time on an erased page will slow the file system.  However, doing
> re-reads for an erased page with bit flips is probably fairly uncommon.
> A flash in this state maybe near EOL or the sector was bad from the
> factory but not marked so.

This is a new device, but its one out of several dozens. The device had
two factory marked bad page. This four page would then be 6 bad pages. I
would say that your guess is probably the case at hand (should be
considered bad, but were marked by factory).  

> 
> I am chasing an DDR2 issue on another CPU and haven't had much more time
> to the look at the Vybrid nor follow the MTD mailing list.  I don't know
> if the 'NAND_ECC_NEED_CHECK_FF' feature has been merged?  It may also
> solve the issue.

A quick search through 3.17-rc3 didn't found that string.

I will read the pages using raw again and check how many bit flips it
shows. But I guess regarding Brian's comment to 3b, the driver actually
behaves correct and return -1, because we have bit flips on erased page
which is not good...

UBI starts to scrub & torture those 4 pages then, but starts doing this
with other pages too. All the pages start to fail then! I'm still
investigating that issue.

[   30.901345] UBI: scrubbed PEB 1436 (LEB 0:356), data moved to PEB
3965
[   31.100247] UBI: run torture test for PEB 1436
[   31.280845] UBI error: torture_peb: read problems on freshly erased
PEB 1436, must be bad
[   31.300656] UBI error: erase_worker: failed to erase PEB 1436, error
-5
[   31.312626] UBI: mark PEB 1436 as bad
[   31.338216] UBI: 12 PEBs left in the reserve
[   31.519044] UBI: scrubbed PEB 1390 (LEB 0:313), data moved to PEB
3963
[   31.697812] UBI: run torture test for PEB 1390
[   31.880470] UBI error: torture_peb: read problems on freshly erased
PEB 1390, must be bad
[   31.898220] UBI error: erase_worker: failed to erase PEB 1390, error
-5
[   31.909687] UBI: mark PEB 1390 as bad
[   31.931620] UBI: 11 PEBs left in the reserve
[   32.170599] UBI: scrubbed PEB 1470 (LEB 0:389), data moved to PEB
3961
[   32.344564] UBI: run torture test for PEB 1470
[   32.522842] UBI error: torture_peb: read problems on freshly erased
PEB 1470, must be bad
[   32.540587] UBI error: erase_worker: failed to erase PEB 1470, error
-5
[   32.552060] UBI: mark PEB 1470 as bad

[RFC 2/5] mtd:fsl_nfc: Add hardware 45 byte BHC-ECC support for 24 bit corrections.

[Stefan Agner]

Am 2014-09-17 20:06, schrieb Bill Pringlemeir:
> On 17 Sep 2014, stefan at agner.ch wrote:
> 
>> On one of our Colibri VF61 modules I discovered an issue using this
>> driver:
> 
>> [    0.758327] ECC failed to correct all errors (ebd9fd80)
>> [    0.767005] ECC failed to correct all errors (ebd9fd80)
>> [    0.775525] ECC failed to correct all errors (ebd9fd80)
>> [    0.784004] ECC failed to correct all errors (ebd9fd80)
>> [    0.791938] UBI error: ubi_io_read: error -74 (ECC error) while
>> reading 2048 bytes from PEB 28:2048,
>> read 2048 bytes
> 
>> That page supposed to be empty, and I got several of this messages.
>> Hence I did not believe that they have really ECC errors, so I digged
>> a bit deeper:
> 
>>>> -673,11 +673,13 @@ static int fsl_nfc_check_ecc_status(struct
>> mtd_info *mtd, u_char *dat)
>> return ecc_count;
> 
>> /* If 'ecc_count' zero or less then buffer is all 0xff or
>> erased. */
>> -       flip = count_written_bits(dat, chip->ecc.size, ecc_count);
>> +       flip = count_written_bits(dat, chip->ecc.size, 100);
> 
>> if (flip > ecc_count) {
>> -               printk("ECC failed to correct all errors (%08x)\n",
>> ecc_status);
>> +               printk("ECC failed to correct all errors (%08x, flips
>> %d)\n",
>> +                               ecc_status, flip);
>> return -1;
>> }
> 
>> [    0.758624] ECC failed to correct all errors (ebdded80, flips 1)
>> [    0.768123] ECC failed to correct all errors (ebdded80, flips 1)
>> [    0.777468] ECC failed to correct all errors (ebdded80, flips 1)
>> [    0.786773] ECC failed to correct all errors (ebdded80, flips 1)
>> [    0.795535] UBI error: ubi_io_read: error -74 (ECC error) while
>> reading 2048 bytes from PEB 28:2048, read 2048 bytes
> 
> [snip]
> 
>> Interesting thought, the returned ecc_count is exactly one below the
>> actual flipped bytes counted...
>>
>> One comment below regarding this...
> 
> [snip]
> 
>>> +
>>> +	/* If 'ecc_count' zero or less then buffer is all 0xff or erased. */
>>> +	flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);
> 
>> Also, I could not find that the reference manual states that ecc_count
>> represents the amount of flipped byte in a empty page. Is this given
>> by the ECC algorithm?
> 
> I was using this information.
> 
>    Table 31-18. ECC Status Word Field Definition
> 
>    7            0 Page has been successfully corrected
>    CORFAIL      1 Page is uncorrectable
> 
>    5?0          Number of errors that have been corrected in this page
>    ERROR_COUNT
> 
> This is from the Vybrid RM, but the MPC5125RM has the same information.
> 
> I have definitely tested the detection of 'erased pages'.  However, I
> don't know that I ever had actual bit flips.
> 
> The ECC controller has no idea whether the page is empty or not.  Are
> you saying you have an erased page with bit flips?  I have definitely
> not tested this.

Yes, that's what it looks like. Note that this output is on first boot
after flashing the device (with the UBI auto-grow option). I guess UBI
is reading all pages once to verify that they are empty.

>>> +	/* If 'ecc_count' zero or less then buffer is all 0xff or erased. */
>>> +	flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);
> 
> There are two issues.
> 
>  1) erased page with some physical flipped bits (zero).
>  2) erased page were controller flipped some bits.

I did not know that issue 2 exists. How can 2 happen on a erased page?
With a 'stuck at zero' in OOB?

> Currently, the code is only handling case 2 (that is what the controller
> counts).  I believe that your physical page has actual 'stuck at zero'
> bits.  The current driver gives up.  If you want to handle this then we
> should replace the lines,
> 

I assumed that your code assumes that the controller returns the flipped
bit count when reading an erase page (case 1), which I did not found in
the documentation. 

>> +	/* ECC failed. */
>> +	if (flip > ecc_count)
>> +		return -1;
> 
> With something like,
> 
>  +	/* Not completely empty. */
>  +	if (flip > ecc_count) {
>  +           re_read_page_w_ecc_off();
>  +           if (count_written_bits() > strength) /* strength/2 ? */
>  +		return -1;
>  +      }
> 
> There is also a discussion on this here,
> 
> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052507.html
> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052508.html
> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052509.html
> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052526.html
> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052527.html
> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052619.html
> ... etc.  Use the view thread.
> 
> Especially,
> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052647.html
> 
> Here Brian says that 3b applies to SLC NAND.  I guess this is you :).

Yes, we are using Macronix SLC NAND.

> 
> It is fairly common to read an erased page.  Doing 'raw reads' all the
> time on an erased page will slow the file system.  However, doing
> re-reads for an erased page with bit flips is probably fairly uncommon.
> A flash in this state maybe near EOL or the sector was bad from the
> factory but not marked so.

This is a new device, but its one out of several dozens. The device had
two factory marked bad page. This four page would then be 6 bad pages. I
would say that your guess is probably the case at hand (should be
considered bad, but were marked by factory).  

> 
> I am chasing an DDR2 issue on another CPU and haven't had much more time
> to the look at the Vybrid nor follow the MTD mailing list.  I don't know
> if the 'NAND_ECC_NEED_CHECK_FF' feature has been merged?  It may also
> solve the issue.

A quick search through 3.17-rc3 didn't found that string.

I will read the pages using raw again and check how many bit flips it
shows. But I guess regarding Brian's comment to 3b, the driver actually
behaves correct and return -1, because we have bit flips on erased page
which is not good...

UBI starts to scrub & torture those 4 pages then, but starts doing this
with other pages too. All the pages start to fail then! I'm still
investigating that issue.

[   30.901345] UBI: scrubbed PEB 1436 (LEB 0:356), data moved to PEB
3965
[   31.100247] UBI: run torture test for PEB 1436
[   31.280845] UBI error: torture_peb: read problems on freshly erased
PEB 1436, must be bad
[   31.300656] UBI error: erase_worker: failed to erase PEB 1436, error
-5
[   31.312626] UBI: mark PEB 1436 as bad
[   31.338216] UBI: 12 PEBs left in the reserve
[   31.519044] UBI: scrubbed PEB 1390 (LEB 0:313), data moved to PEB
3963
[   31.697812] UBI: run torture test for PEB 1390
[   31.880470] UBI error: torture_peb: read problems on freshly erased
PEB 1390, must be bad
[   31.898220] UBI error: erase_worker: failed to erase PEB 1390, error
-5
[   31.909687] UBI: mark PEB 1390 as bad
[   31.931620] UBI: 11 PEBs left in the reserve
[   32.170599] UBI: scrubbed PEB 1470 (LEB 0:389), data moved to PEB
3961
[   32.344564] UBI: run torture test for PEB 1470
[   32.522842] UBI error: torture_peb: read problems on freshly erased
PEB 1470, must be bad
[   32.540587] UBI error: erase_worker: failed to erase PEB 1470, error
-5
[   32.552060] UBI: mark PEB 1470 as bad

Re: [RFC 2/5] mtd:fsl_nfc: Add hardware 45 byte BHC-ECC support for 24 bit corrections.

[Bill Pringlemeir]

On 17 Sep 2014, stefan@agner.ch wrote:

> Am 2014-09-17 20:06, schrieb Bill Pringlemeir:
>> On 17 Sep 2014, stefan@agner.ch wrote:

>>> On one of our Colibri VF61 modules I discovered an issue using this
>>> driver:

>>> [    0.758327] ECC failed to correct all errors (ebd9fd80)
>>> [    0.767005] ECC failed to correct all errors (ebd9fd80)
>>> [    0.775525] ECC failed to correct all errors (ebd9fd80)
>>> [    0.784004] ECC failed to correct all errors (ebd9fd80)
>>> [    0.791938] UBI error: ubi_io_read: error -74 (ECC error) while
>>> reading 2048 bytes from PEB 28:2048,
>>> read 2048 bytes

[snip]

>>> Also, I could not find that the reference manual states that
>>> ecc_count represents the amount of flipped byte in a empty page. Is
>>> this given by the ECC algorithm?

>> I was using this information.
>>
>> Table 31-18. ECC Status Word Field Definition
>>
>> 7            0 Page has been successfully corrected
>> CORFAIL      1 Page is uncorrectable
>>
>> 5–0          Number of errors that have been corrected in this page
>> ERROR_COUNT
>>
>> This is from the Vybrid RM, but the MPC5125RM has the same
>> information.
>>
>> I have definitely tested the detection of 'erased pages'.  However, I
>> don't know that I ever had actual bit flips.

>> The ECC controller has no idea whether the page is empty or not.  Are
>> you saying you have an erased page with bit flips?  I have definitely
>> not tested this.

> Yes, that's what it looks like. Note that this output is on first boot
> after flashing the device (with the UBI auto-grow option). I guess UBI
> is reading all pages once to verify that they are empty.

>>>> + /* If 'ecc_count' zero or less then buffer is all 0xff or
>>>> erased. */ + flip = count_written_bits(dat, nfc->chip.ecc.size,
>>>> ecc_count);
>>
>> There are two issues.
>>
>> 1) erased page with some physical flipped bits (zero).
>> 2) erased page were controller flipped some bits.

> I did not know that issue 2 exists. How can 2 happen on a erased page?
> With a 'stuck at zero' in OOB?

Issue 2 is always an issue when reading an erased page with hardware
ECC.  So we have,

                    Main         Spare
    Good program:   Data    OOB-data+HW-ECC
    Good erased:    FF      FF  FF
    Stuck erased1:  FF      FF  xx
    Stuck erased2:  xx      xx  FF

For the 'good erased' the controller goes about like it is case 'good
program'.  It reads a sector (raw read or software ECC case) and a the
same time, the error corrector is running.  This examines the 'OOB
data', which is 0xff for the erased sector and it blindly begins to
apply the error correction to the main data section.  The 'correction'
for an erased block will always flip '1' to '0'.  So for 'good erased',
the 'error_count' will equal the flipped bits.

Now, your sectors aren't that bad.  I believe you only have one bit
error on an erase.  However, it is really hairy to know what the ECC
logic will do when you have bit flips in the ECC code.  It is also next
to impossible to examine the buffer and know if zero bits are due to the
ECC engine flipping the bits and/or the actual physical flash having the
stuck bit.  It appears that the ECC engine runs from start to finish.
So most of the ECC corrections should be at the start; use this info for
diagnostics, but not coding!

The best course IMHO is to do what Brian (and other) did and do a 'raw'
read, so you take the ECC engine out of the picture and stop it from
flipping bits.  You can experiment with always doing the raw read, but I
believe you will see a decrease in performance (as others mentioned and
I saw).

>> Currently, the code is only handling case 2 (that is what the
>> controller counts).  I believe that your physical page has actual
>> 'stuck at zero' bits.  The current driver gives up.  If you want to
>> handle this then we should replace the lines,

> I assumed that your code assumes that the controller returns the
> flipped bit count when reading an erase page (case 1), which I did not
> found in the documentation.

>>> +	/* ECC failed. */
>>> +	if (flip > ecc_count)
>>> +		return -1;

>> With something like,
>>
>> +	/* Not completely empty. */
>> +	if (flip > ecc_count) {
>> +           re_read_page_w_ecc_off();
>> +           if (count_written_bits() > strength) /* strength/2 ? */
>> +		return -1;
>> +      }
>>
>> There is also a discussion on this here,
>>
>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052507.html
>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052508.html
>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052509.html
>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052526.html
>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052527.html
>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052619.html
>> ... etc.  Use the view thread.
>>
>> Especially,
>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052647.html
>>
>> Here Brian says that 3b applies to SLC NAND.  I guess this is you :).

> Yes, we are using Macronix SLC NAND.

Well, it is everyone.  Sorry, I made the assumption this wouldn't
happen, but apparently it does.

>> It is fairly common to read an erased page.  Doing 'raw reads' all
>> the time on an erased page will slow the file system.  However, doing
>> re-reads for an erased page with bit flips is probably fairly
>> uncommon.  A flash in this state maybe near EOL or the sector was bad
>> from the factory but not marked so.

> This is a new device, but its one out of several dozens. The device
> had two factory marked bad page. This four page would then be 6 bad
> pages. I would say that your guess is probably the case at hand
> (should be considered bad, but were marked by factory).

>> I am chasing an DDR2 issue on another CPU and haven't had much more
>> time to the look at the Vybrid nor follow the MTD mailing list.  I
>> don't know if the 'NAND_ECC_NEED_CHECK_FF' feature has been merged?
>> It may also solve the issue.
>
> A quick search through 3.17-rc3 didn't found that string.

> I will read the pages using raw again and check how many bit flips it
> shows. But I guess regarding Brian's comment to 3b, the driver
> actually behaves correct and return -1, because we have bit flips on
> erased page which is not good...

Hmm.  No, it should be ok; I think Brian meant we should accept and
expect this.  We had this conversation after the RFC.  However, we
should probably only accept strength/2 zero values at most; I believe
what to accept was kind of hanging in that conversation.  If the zeros
are below our threshold with a raw read, then we can report the page is
all '\xff' and return a 'bit flip' count as the number of zeros found in
a raw read.  Also, in the 'all FFs' case we should probably memset the
OOB data.  I see that other patches mentioned in the MTD threads above
do this.

 +	/* Not completely empty. */
 +	if (flip > ecc_count) {
 +           re_read_page_w_ecc_off();
 +           ecc_count = count_zero_bits();
 +           if (ecc_count > strength/2)
 +		return -1;
 +      } else
 +          ecc_count = 0;  /* 'normal' case all ff */
 +
 +	/* Erased page. */
 +	memset(dat, 0xff, nfc->chip.ecc.size);
 +	memset(oob, 0xff, ???);  /* ??? */
 +	return ecc_count;

I don't think that UBI/UbiFs use the OOB data at all, but that is
something that I don't think is completely correct in this path?

> UBI starts to scrub & torture those 4 pages then, but starts doing
> this with other pages too. All the pages start to fail then! I'm still
> investigating that issue.

Arg.  I am not quite sure.  It seems ok to have a few 'stuck at one'
errors.  As you move closer to the ECC strength this seems a little
crazy.  At some point the sector/erase block is becoming useless.  I am
not sure if this is an issue with this device; it seems a little
suspect.  Maybe you could code something that emulates the stuck bits on
a good device and see if it still behaves the same; triggers a software
condition.  You might also reduce the NFC bus clock just to see on the
'stuck zero' device.

Hth,
Bill Pringlemeir.

[RFC 2/5] mtd:fsl_nfc: Add hardware 45 byte BHC-ECC support for 24 bit corrections.

[Bill Pringlemeir]

On 17 Sep 2014, stefan at agner.ch wrote:

> Am 2014-09-17 20:06, schrieb Bill Pringlemeir:
>> On 17 Sep 2014, stefan at agner.ch wrote:

>>> On one of our Colibri VF61 modules I discovered an issue using this
>>> driver:

>>> [    0.758327] ECC failed to correct all errors (ebd9fd80)
>>> [    0.767005] ECC failed to correct all errors (ebd9fd80)
>>> [    0.775525] ECC failed to correct all errors (ebd9fd80)
>>> [    0.784004] ECC failed to correct all errors (ebd9fd80)
>>> [    0.791938] UBI error: ubi_io_read: error -74 (ECC error) while
>>> reading 2048 bytes from PEB 28:2048,
>>> read 2048 bytes

[snip]

>>> Also, I could not find that the reference manual states that
>>> ecc_count represents the amount of flipped byte in a empty page. Is
>>> this given by the ECC algorithm?

>> I was using this information.
>>
>> Table 31-18. ECC Status Word Field Definition
>>
>> 7            0 Page has been successfully corrected
>> CORFAIL      1 Page is uncorrectable
>>
>> 5?0          Number of errors that have been corrected in this page
>> ERROR_COUNT
>>
>> This is from the Vybrid RM, but the MPC5125RM has the same
>> information.
>>
>> I have definitely tested the detection of 'erased pages'.  However, I
>> don't know that I ever had actual bit flips.

>> The ECC controller has no idea whether the page is empty or not.  Are
>> you saying you have an erased page with bit flips?  I have definitely
>> not tested this.

> Yes, that's what it looks like. Note that this output is on first boot
> after flashing the device (with the UBI auto-grow option). I guess UBI
> is reading all pages once to verify that they are empty.

>>>> + /* If 'ecc_count' zero or less then buffer is all 0xff or
>>>> erased. */ + flip = count_written_bits(dat, nfc->chip.ecc.size,
>>>> ecc_count);
>>
>> There are two issues.
>>
>> 1) erased page with some physical flipped bits (zero).
>> 2) erased page were controller flipped some bits.

> I did not know that issue 2 exists. How can 2 happen on a erased page?
> With a 'stuck at zero' in OOB?

Issue 2 is always an issue when reading an erased page with hardware
ECC.  So we have,

                    Main         Spare
    Good program:   Data    OOB-data+HW-ECC
    Good erased:    FF      FF  FF
    Stuck erased1:  FF      FF  xx
    Stuck erased2:  xx      xx  FF

For the 'good erased' the controller goes about like it is case 'good
program'.  It reads a sector (raw read or software ECC case) and a the
same time, the error corrector is running.  This examines the 'OOB
data', which is 0xff for the erased sector and it blindly begins to
apply the error correction to the main data section.  The 'correction'
for an erased block will always flip '1' to '0'.  So for 'good erased',
the 'error_count' will equal the flipped bits.

Now, your sectors aren't that bad.  I believe you only have one bit
error on an erase.  However, it is really hairy to know what the ECC
logic will do when you have bit flips in the ECC code.  It is also next
to impossible to examine the buffer and know if zero bits are due to the
ECC engine flipping the bits and/or the actual physical flash having the
stuck bit.  It appears that the ECC engine runs from start to finish.
So most of the ECC corrections should be at the start; use this info for
diagnostics, but not coding!

The best course IMHO is to do what Brian (and other) did and do a 'raw'
read, so you take the ECC engine out of the picture and stop it from
flipping bits.  You can experiment with always doing the raw read, but I
believe you will see a decrease in performance (as others mentioned and
I saw).

>> Currently, the code is only handling case 2 (that is what the
>> controller counts).  I believe that your physical page has actual
>> 'stuck at zero' bits.  The current driver gives up.  If you want to
>> handle this then we should replace the lines,

> I assumed that your code assumes that the controller returns the
> flipped bit count when reading an erase page (case 1), which I did not
> found in the documentation.

>>> +	/* ECC failed. */
>>> +	if (flip > ecc_count)
>>> +		return -1;

>> With something like,
>>
>> +	/* Not completely empty. */
>> +	if (flip > ecc_count) {
>> +           re_read_page_w_ecc_off();
>> +           if (count_written_bits() > strength) /* strength/2 ? */
>> +		return -1;
>> +      }
>>
>> There is also a discussion on this here,
>>
>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052507.html
>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052508.html
>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052509.html
>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052526.html
>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052527.html
>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052619.html
>> ... etc.  Use the view thread.
>>
>> Especially,
>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052647.html
>>
>> Here Brian says that 3b applies to SLC NAND.  I guess this is you :).

> Yes, we are using Macronix SLC NAND.

Well, it is everyone.  Sorry, I made the assumption this wouldn't
happen, but apparently it does.

>> It is fairly common to read an erased page.  Doing 'raw reads' all
>> the time on an erased page will slow the file system.  However, doing
>> re-reads for an erased page with bit flips is probably fairly
>> uncommon.  A flash in this state maybe near EOL or the sector was bad
>> from the factory but not marked so.

> This is a new device, but its one out of several dozens. The device
> had two factory marked bad page. This four page would then be 6 bad
> pages. I would say that your guess is probably the case at hand
> (should be considered bad, but were marked by factory).

>> I am chasing an DDR2 issue on another CPU and haven't had much more
>> time to the look at the Vybrid nor follow the MTD mailing list.  I
>> don't know if the 'NAND_ECC_NEED_CHECK_FF' feature has been merged?
>> It may also solve the issue.
>
> A quick search through 3.17-rc3 didn't found that string.

> I will read the pages using raw again and check how many bit flips it
> shows. But I guess regarding Brian's comment to 3b, the driver
> actually behaves correct and return -1, because we have bit flips on
> erased page which is not good...

Hmm.  No, it should be ok; I think Brian meant we should accept and
expect this.  We had this conversation after the RFC.  However, we
should probably only accept strength/2 zero values at most; I believe
what to accept was kind of hanging in that conversation.  If the zeros
are below our threshold with a raw read, then we can report the page is
all '\xff' and return a 'bit flip' count as the number of zeros found in
a raw read.  Also, in the 'all FFs' case we should probably memset the
OOB data.  I see that other patches mentioned in the MTD threads above
do this.

 +	/* Not completely empty. */
 +	if (flip > ecc_count) {
 +           re_read_page_w_ecc_off();
 +           ecc_count = count_zero_bits();
 +           if (ecc_count > strength/2)
 +		return -1;
 +      } else
 +          ecc_count = 0;  /* 'normal' case all ff */
 +
 +	/* Erased page. */
 +	memset(dat, 0xff, nfc->chip.ecc.size);
 +	memset(oob, 0xff, ???);  /* ??? */
 +	return ecc_count;

I don't think that UBI/UbiFs use the OOB data at all, but that is
something that I don't think is completely correct in this path?

> UBI starts to scrub & torture those 4 pages then, but starts doing
> this with other pages too. All the pages start to fail then! I'm still
> investigating that issue.

Arg.  I am not quite sure.  It seems ok to have a few 'stuck at one'
errors.  As you move closer to the ECC strength this seems a little
crazy.  At some point the sector/erase block is becoming useless.  I am
not sure if this is an issue with this device; it seems a little
suspect.  Maybe you could code something that emulates the stuck bits on
a good device and see if it still behaves the same; triggers a software
condition.  You might also reduce the NFC bus clock just to see on the
'stuck zero' device.

Hth,
Bill Pringlemeir.

Re: [RFC 2/5] mtd:fsl_nfc: Add hardware 45 byte BHC-ECC support for 24 bit corrections.

[Stefan Agner]

Hi Bill,

On 2014-09-18 00:21, Bill Pringlemeir wrote:
> On 17 Sep 2014, stefan@agner.ch wrote:
> 
>> Am 2014-09-17 20:06, schrieb Bill Pringlemeir:
>>> On 17 Sep 2014, stefan@agner.ch wrote:
> 
>>>> On one of our Colibri VF61 modules I discovered an issue using this
>>>> driver:
> 
>>>> [    0.758327] ECC failed to correct all errors (ebd9fd80)
>>>> [    0.767005] ECC failed to correct all errors (ebd9fd80)
>>>> [    0.775525] ECC failed to correct all errors (ebd9fd80)
>>>> [    0.784004] ECC failed to correct all errors (ebd9fd80)
>>>> [    0.791938] UBI error: ubi_io_read: error -74 (ECC error) while
>>>> reading 2048 bytes from PEB 28:2048,
>>>> read 2048 bytes
> 
> [snip]
> 
>>>> Also, I could not find that the reference manual states that
>>>> ecc_count represents the amount of flipped byte in a empty page. Is
>>>> this given by the ECC algorithm?
> 
>>> I was using this information.
>>>
>>> Table 31-18. ECC Status Word Field Definition
>>>
>>> 7            0 Page has been successfully corrected
>>> CORFAIL      1 Page is uncorrectable
>>>
>>> 5–0          Number of errors that have been corrected in this page
>>> ERROR_COUNT
>>>
>>> This is from the Vybrid RM, but the MPC5125RM has the same
>>> information.
>>>
>>> I have definitely tested the detection of 'erased pages'.  However, I
>>> don't know that I ever had actual bit flips.
> 
>>> The ECC controller has no idea whether the page is empty or not.  Are
>>> you saying you have an erased page with bit flips?  I have definitely
>>> not tested this.
> 
>> Yes, that's what it looks like. Note that this output is on first boot
>> after flashing the device (with the UBI auto-grow option). I guess UBI
>> is reading all pages once to verify that they are empty.
> 
>>>>> + /* If 'ecc_count' zero or less then buffer is all 0xff or
>>>>> erased. */ + flip = count_written_bits(dat, nfc->chip.ecc.size,
>>>>> ecc_count);
>>>
>>> There are two issues.
>>>
>>> 1) erased page with some physical flipped bits (zero).
>>> 2) erased page were controller flipped some bits.
> 
>> I did not know that issue 2 exists. How can 2 happen on a erased page?
>> With a 'stuck at zero' in OOB?
> 
> Issue 2 is always an issue when reading an erased page with hardware
> ECC.  So we have,
> 
>                     Main         Spare
>     Good program:   Data    OOB-data+HW-ECC
>     Good erased:    FF      FF  FF
>     Stuck erased1:  FF      FF  xx
>     Stuck erased2:  xx      xx  FF
> 
> For the 'good erased' the controller goes about like it is case 'good
> program'.  It reads a sector (raw read or software ECC case) and a the
> same time, the error corrector is running.  This examines the 'OOB
> data', which is 0xff for the erased sector and it blindly begins to
> apply the error correction to the main data section.  The 'correction'
> for an erased block will always flip '1' to '0'.  So for 'good erased',
> the 'error_count' will equal the flipped bits.
> 
> Now, your sectors aren't that bad.  I believe you only have one bit
> error on an erase.  However, it is really hairy to know what the ECC
> logic will do when you have bit flips in the ECC code.  It is also next
> to impossible to examine the buffer and know if zero bits are due to the
> ECC engine flipping the bits and/or the actual physical flash having the
> stuck bit.  It appears that the ECC engine runs from start to finish.
> So most of the ECC corrections should be at the start; use this info for
> diagnostics, but not coding!

Two errors are at the beginning, one is almost at the end.

> The best course IMHO is to do what Brian (and other) did and do a 'raw'
> read, so you take the ECC engine out of the picture and stop it from
> flipping bits.  You can experiment with always doing the raw read, but I
> believe you will see a decrease in performance (as others mentioned and
> I saw).
> 

Hm, we currently don't have a non ECC read function as we enable ECC
unconditionally. I guess this would need a read_oob_raw implementation,
where we disable ECC, read the page, and reenable ECC...

>>> Currently, the code is only handling case 2 (that is what the
>>> controller counts).  I believe that your physical page has actual
>>> 'stuck at zero' bits.  The current driver gives up.  If you want to
>>> handle this then we should replace the lines,
> 
>> I assumed that your code assumes that the controller returns the
>> flipped bit count when reading an erase page (case 1), which I did not
>> found in the documentation.
> 
>>>> +	/* ECC failed. */
>>>> +	if (flip > ecc_count)
>>>> +		return -1;
> 
>>> With something like,
>>>
>>> +	/* Not completely empty. */
>>> +	if (flip > ecc_count) {
>>> +           re_read_page_w_ecc_off();
>>> +           if (count_written_bits() > strength) /* strength/2 ? */
>>> +		return -1;
>>> +      }
>>>
>>> There is also a discussion on this here,
>>>
>>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052507.html
>>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052508.html
>>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052509.html
>>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052526.html
>>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052527.html
>>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052619.html
>>> ... etc.  Use the view thread.
>>>
>>> Especially,
>>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052647.html
>>>
>>> Here Brian says that 3b applies to SLC NAND.  I guess this is you :).
> 
>> Yes, we are using Macronix SLC NAND.
> 
> Well, it is everyone.  Sorry, I made the assumption this wouldn't
> happen, but apparently it does.
> 
>>> It is fairly common to read an erased page.  Doing 'raw reads' all
>>> the time on an erased page will slow the file system.  However, doing
>>> re-reads for an erased page with bit flips is probably fairly
>>> uncommon.  A flash in this state maybe near EOL or the sector was bad
>>> from the factory but not marked so.
> 
>> This is a new device, but its one out of several dozens. The device
>> had two factory marked bad page. This four page would then be 6 bad
>> pages. I would say that your guess is probably the case at hand
>> (should be considered bad, but were marked by factory).
> 
>>> I am chasing an DDR2 issue on another CPU and haven't had much more
>>> time to the look at the Vybrid nor follow the MTD mailing list.  I
>>> don't know if the 'NAND_ECC_NEED_CHECK_FF' feature has been merged?
>>> It may also solve the issue.
>>
>> A quick search through 3.17-rc3 didn't found that string.
> 
>> I will read the pages using raw again and check how many bit flips it
>> shows. But I guess regarding Brian's comment to 3b, the driver
>> actually behaves correct and return -1, because we have bit flips on
>> erased page which is not good...
> 
> Hmm.  No, it should be ok; I think Brian meant we should accept and
> expect this.  We had this conversation after the RFC.  However, we
> should probably only accept strength/2 zero values at most; I believe
> what to accept was kind of hanging in that conversation.  If the zeros
> are below our threshold with a raw read, then we can report the page is
> all '\xff' and return a 'bit flip' count as the number of zeros found in
> a raw read.  Also, in the 'all FFs' case we should probably memset the
> OOB data.  I see that other patches mentioned in the MTD threads above
> do this.
> 
>  +	/* Not completely empty. */
>  +	if (flip > ecc_count) {
>  +           re_read_page_w_ecc_off();
>  +           ecc_count = count_zero_bits();
>  +           if (ecc_count > strength/2)
>  +		return -1;
>  +      } else
>  +          ecc_count = 0;  /* 'normal' case all ff */
>  +
>  +	/* Erased page. */
>  +	memset(dat, 0xff, nfc->chip.ecc.size);
>  +	memset(oob, 0xff, ???);  /* ??? */
>  +	return ecc_count;
> 

What I currently did, is just accept strength / 2 bits. This is not a
clean solution since it will also count the ECC bits, but it works for
now:
--- a/drivers/mtd/nand/fsl_nfc.c
+++ b/drivers/mtd/nand/fsl_nfc.c
@@ -524,7 +524,7 @@ static int nfc_correct_data(struct mtd_info *mtd,
u_char *dat,
        flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);
 
        /* ECC failed. */
-       if (flip > ecc_count)
+       if (flip > ecc_count && flip > (nfc->chip.ecc.strength / 2))
                return -1;
 
        /* Erased page. */



> I don't think that UBI/UbiFs use the OOB data at all, but that is
> something that I don't think is completely correct in this path?
> 
>> UBI starts to scrub & torture those 4 pages then, but starts doing
>> this with other pages too. All the pages start to fail then! I'm still
>> investigating that issue.
> 
> Arg.  I am not quite sure.  It seems ok to have a few 'stuck at one'
> errors.  As you move closer to the ECC strength this seems a little
> crazy.  At some point the sector/erase block is becoming useless.  I am
> not sure if this is an issue with this device; it seems a little
> suspect.  Maybe you could code something that emulates the stuck bits on
> a good device and see if it still behaves the same; triggers a software
> condition.  You might also reduce the NFC bus clock just to see on the
> 'stuck zero' device.
> 

I think we are facing multiple issues here. One might contain general
software/hardware issues (non bit-flip related). I had this issue again
on a different module with 3.18-rc5 (without the "fix" above). The
kernel output looks like this:

[    1.744672] UBI: default fastmap pool size: 200
[    1.752573] UBI: default fastmap WL pool size: 25
[    1.760577] UBI: attaching mtd3 to ubi0
[    1.769093] UBI warning: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 0:2048, read only 2048 bytes, retry
[    1.787579] UBI warning: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 0:2048, read only 2048 bytes, retry
[    1.806383] UBI warning: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 0:2048, read only 2048 bytes, retry
[    1.825649] UBI error: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 0:2048, read 2048 bytes
[    1.843804] CPU: 0 PID: 1 Comm: swapper Not tainted
3.18.0-rc5-00136-g0d91606-dirty #1607
[    1.860148] Backtrace:
[    1.866779] [<80011a04>] (dump_backtrace) from [<80011ce0>]
(show_stack+0x18/0x1c)
[    1.883061]  r6:00000000 r5:00000800 r4:ffffffb6 r3:00000000
[    1.893395] [<80011cc8>] (show_stack) from [<805a5018>]
(dump_stack+0x24/0x28)
[    1.909684] [<805a4ff4>] (dump_stack) from [<8034e9ac>]
(ubi_io_read+0x130/0x300)
[    1.926615] [<8034e87c>] (ubi_io_read) from [<8034efac>]
(ubi_io_read_vid_hdr+0x50/0x220)
[    1.944255]  r10:00000000 r9:00000000 r8:8ea4c000 r7:00000000
r6:8eaa3000 r5:00000000
[    1.961788]  r4:8ea4c000
[    1.969163] [<8034ef5c>] (ubi_io_read_vid_hdr) from [<803539ec>]
(scan_peb.part.9+0x128/0x6a4)
[    1.987614]  r10:00000000 r9:8e843dec r8:8ea4c000 r7:00000000
r6:8ead2900 r5:00000000
[    2.005493]  r4:80847a4c
[    2.012847] [<803538c4>] (scan_peb.part.9) from [<80354ed4>]
(ubi_attach+0x13c/0x398)
[    2.030220]  r10:ffffffff r9:80847a4c r8:ffffffff r7:7ffff000
r6:8ead2900 r5:8ea4c000
[    2.047761]  r4:00000000
[    2.054949] [<80354d98>] (ubi_attach) from [<80349504>]
(ubi_attach_mtd_dev+0x6a8/0xd00)
[    2.072405]  r10:00000050 r9:8ea13c00 r8:000007ff r7:8ea4c000
r6:00000000 r5:8ea13c00
[    2.089965]  r4:00000990
[    2.097179] [<80348e5c>] (ubi_attach_mtd_dev) from [<807933f0>]
(ubi_init+0x214/0x2b8)
[    2.114458]  r10:00000000 r9:8ead9200 r8:807785e8 r7:8ea13c00
r6:807a9700 r5:00000000
[    2.131856]  r4:807a9704
[    2.138962] [<807931dc>] (ubi_init) from [<80008770>]
(do_one_initcall+0x94/0x1d4)
[    2.155573]  r8:807785e8 r7:8e842038 r6:807931dc r5:807b9460
r4:807b9460
[    2.167084] [<800086dc>] (do_one_initcall) from [<80778e38>]
(kernel_init_freeable+0x130/0x1d0)
[    2.185090]  r10:807a44b0 r9:807a44a8 r8:807785e8 r7:807f4b80
r6:807f4b80 r5:00000007
[    2.202418]  r4:807ad4d8
[    2.209488] [<80778d08>] (kernel_init_freeable) from [<805a1b64>]
(kernel_init+0x10/0xf0)
[    2.226946]  r10:00000000 r9:00000000 r8:00000000 r7:00000000
r6:00000000 r5:805a1b54
[    2.244265]  r4:00000000
[    2.251337] [<805a1b54>] (kernel_init) from [<8000e638>]
(ret_from_fork+0x14/0x3c)
[    2.268179]  r4:00000000 r3:8e842000
[    2.297506] UBI warning: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 2:2048, read only 2048 bytes, retry
[    2.318557] UBI warning: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 2:2048, read only 2048 bytes, retry
[    2.340077] UBI warning: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 2:2048, read only 2048 bytes, retry
[    2.361438] UBI error: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 2:2048, read 2048 bytes
[    2.381479] CPU: 0 PID: 1 Comm: swapper Not tainted
3.18.0-rc5-00136-g0d91606-dirty #1607

Interesting is that this error happens every second PEB (every 128 page,
but erase block size is 64) and it is always the second page. On that
device, this is completely reproduceable, e.g. I can erase everything
and flash it again, the same happens. 


I dumped the block in question:

Page 00240800 dump:
        ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
        ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
        ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
        ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
        ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
        ff ff ff ff ff ff ff ff  f7 ff ff ff ff ff ff ff
        ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
        ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
        ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
        ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
        ff ff ff ff ff ff ff ff  ff ff fb ff ff ff ff ff
        ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
....
        ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff f7
....

I also printed flip count and ecc_count the values for all those pages
are: flip 3, ecc_count 2

Now the interesting part: When I erase the block, and dump that page
again, it is completely empty! No flips, no ecc_count anymore! UBI
attach writes something into the first page, hence it looks like this
write into the first page influences the values of the second page... I
verified this behavior this using U-Boot and the Linux kernel.

I digged a bit deeper, and wrote just zeros into the first page. In the
second page some bits are flipped. However, writing into the second page
does not influence the third page. But a bit in the first page is
flipped. And the third page influences the forth page. It looks like the
pages behave in pairs.... Any idea what kind of issue we are facing
here?

--
Stefan

[RFC 2/5] mtd:fsl_nfc: Add hardware 45 byte BHC-ECC support for 24 bit corrections.

[Stefan Agner]

Hi Bill,

On 2014-09-18 00:21, Bill Pringlemeir wrote:
> On 17 Sep 2014, stefan at agner.ch wrote:
> 
>> Am 2014-09-17 20:06, schrieb Bill Pringlemeir:
>>> On 17 Sep 2014, stefan at agner.ch wrote:
> 
>>>> On one of our Colibri VF61 modules I discovered an issue using this
>>>> driver:
> 
>>>> [    0.758327] ECC failed to correct all errors (ebd9fd80)
>>>> [    0.767005] ECC failed to correct all errors (ebd9fd80)
>>>> [    0.775525] ECC failed to correct all errors (ebd9fd80)
>>>> [    0.784004] ECC failed to correct all errors (ebd9fd80)
>>>> [    0.791938] UBI error: ubi_io_read: error -74 (ECC error) while
>>>> reading 2048 bytes from PEB 28:2048,
>>>> read 2048 bytes
> 
> [snip]
> 
>>>> Also, I could not find that the reference manual states that
>>>> ecc_count represents the amount of flipped byte in a empty page. Is
>>>> this given by the ECC algorithm?
> 
>>> I was using this information.
>>>
>>> Table 31-18. ECC Status Word Field Definition
>>>
>>> 7            0 Page has been successfully corrected
>>> CORFAIL      1 Page is uncorrectable
>>>
>>> 5?0          Number of errors that have been corrected in this page
>>> ERROR_COUNT
>>>
>>> This is from the Vybrid RM, but the MPC5125RM has the same
>>> information.
>>>
>>> I have definitely tested the detection of 'erased pages'.  However, I
>>> don't know that I ever had actual bit flips.
> 
>>> The ECC controller has no idea whether the page is empty or not.  Are
>>> you saying you have an erased page with bit flips?  I have definitely
>>> not tested this.
> 
>> Yes, that's what it looks like. Note that this output is on first boot
>> after flashing the device (with the UBI auto-grow option). I guess UBI
>> is reading all pages once to verify that they are empty.
> 
>>>>> + /* If 'ecc_count' zero or less then buffer is all 0xff or
>>>>> erased. */ + flip = count_written_bits(dat, nfc->chip.ecc.size,
>>>>> ecc_count);
>>>
>>> There are two issues.
>>>
>>> 1) erased page with some physical flipped bits (zero).
>>> 2) erased page were controller flipped some bits.
> 
>> I did not know that issue 2 exists. How can 2 happen on a erased page?
>> With a 'stuck at zero' in OOB?
> 
> Issue 2 is always an issue when reading an erased page with hardware
> ECC.  So we have,
> 
>                     Main         Spare
>     Good program:   Data    OOB-data+HW-ECC
>     Good erased:    FF      FF  FF
>     Stuck erased1:  FF      FF  xx
>     Stuck erased2:  xx      xx  FF
> 
> For the 'good erased' the controller goes about like it is case 'good
> program'.  It reads a sector (raw read or software ECC case) and a the
> same time, the error corrector is running.  This examines the 'OOB
> data', which is 0xff for the erased sector and it blindly begins to
> apply the error correction to the main data section.  The 'correction'
> for an erased block will always flip '1' to '0'.  So for 'good erased',
> the 'error_count' will equal the flipped bits.
> 
> Now, your sectors aren't that bad.  I believe you only have one bit
> error on an erase.  However, it is really hairy to know what the ECC
> logic will do when you have bit flips in the ECC code.  It is also next
> to impossible to examine the buffer and know if zero bits are due to the
> ECC engine flipping the bits and/or the actual physical flash having the
> stuck bit.  It appears that the ECC engine runs from start to finish.
> So most of the ECC corrections should be at the start; use this info for
> diagnostics, but not coding!

Two errors are at the beginning, one is almost at the end.

> The best course IMHO is to do what Brian (and other) did and do a 'raw'
> read, so you take the ECC engine out of the picture and stop it from
> flipping bits.  You can experiment with always doing the raw read, but I
> believe you will see a decrease in performance (as others mentioned and
> I saw).
> 

Hm, we currently don't have a non ECC read function as we enable ECC
unconditionally. I guess this would need a read_oob_raw implementation,
where we disable ECC, read the page, and reenable ECC...

>>> Currently, the code is only handling case 2 (that is what the
>>> controller counts).  I believe that your physical page has actual
>>> 'stuck at zero' bits.  The current driver gives up.  If you want to
>>> handle this then we should replace the lines,
> 
>> I assumed that your code assumes that the controller returns the
>> flipped bit count when reading an erase page (case 1), which I did not
>> found in the documentation.
> 
>>>> +	/* ECC failed. */
>>>> +	if (flip > ecc_count)
>>>> +		return -1;
> 
>>> With something like,
>>>
>>> +	/* Not completely empty. */
>>> +	if (flip > ecc_count) {
>>> +           re_read_page_w_ecc_off();
>>> +           if (count_written_bits() > strength) /* strength/2 ? */
>>> +		return -1;
>>> +      }
>>>
>>> There is also a discussion on this here,
>>>
>>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052507.html
>>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052508.html
>>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052509.html
>>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052526.html
>>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052527.html
>>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052619.html
>>> ... etc.  Use the view thread.
>>>
>>> Especially,
>>> http://lists.infradead.org/pipermail/linux-mtd/2014-March/052647.html
>>>
>>> Here Brian says that 3b applies to SLC NAND.  I guess this is you :).
> 
>> Yes, we are using Macronix SLC NAND.
> 
> Well, it is everyone.  Sorry, I made the assumption this wouldn't
> happen, but apparently it does.
> 
>>> It is fairly common to read an erased page.  Doing 'raw reads' all
>>> the time on an erased page will slow the file system.  However, doing
>>> re-reads for an erased page with bit flips is probably fairly
>>> uncommon.  A flash in this state maybe near EOL or the sector was bad
>>> from the factory but not marked so.
> 
>> This is a new device, but its one out of several dozens. The device
>> had two factory marked bad page. This four page would then be 6 bad
>> pages. I would say that your guess is probably the case at hand
>> (should be considered bad, but were marked by factory).
> 
>>> I am chasing an DDR2 issue on another CPU and haven't had much more
>>> time to the look at the Vybrid nor follow the MTD mailing list.  I
>>> don't know if the 'NAND_ECC_NEED_CHECK_FF' feature has been merged?
>>> It may also solve the issue.
>>
>> A quick search through 3.17-rc3 didn't found that string.
> 
>> I will read the pages using raw again and check how many bit flips it
>> shows. But I guess regarding Brian's comment to 3b, the driver
>> actually behaves correct and return -1, because we have bit flips on
>> erased page which is not good...
> 
> Hmm.  No, it should be ok; I think Brian meant we should accept and
> expect this.  We had this conversation after the RFC.  However, we
> should probably only accept strength/2 zero values at most; I believe
> what to accept was kind of hanging in that conversation.  If the zeros
> are below our threshold with a raw read, then we can report the page is
> all '\xff' and return a 'bit flip' count as the number of zeros found in
> a raw read.  Also, in the 'all FFs' case we should probably memset the
> OOB data.  I see that other patches mentioned in the MTD threads above
> do this.
> 
>  +	/* Not completely empty. */
>  +	if (flip > ecc_count) {
>  +           re_read_page_w_ecc_off();
>  +           ecc_count = count_zero_bits();
>  +           if (ecc_count > strength/2)
>  +		return -1;
>  +      } else
>  +          ecc_count = 0;  /* 'normal' case all ff */
>  +
>  +	/* Erased page. */
>  +	memset(dat, 0xff, nfc->chip.ecc.size);
>  +	memset(oob, 0xff, ???);  /* ??? */
>  +	return ecc_count;
> 

What I currently did, is just accept strength / 2 bits. This is not a
clean solution since it will also count the ECC bits, but it works for
now:
--- a/drivers/mtd/nand/fsl_nfc.c
+++ b/drivers/mtd/nand/fsl_nfc.c
@@ -524,7 +524,7 @@ static int nfc_correct_data(struct mtd_info *mtd,
u_char *dat,
        flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);
 
        /* ECC failed. */
-       if (flip > ecc_count)
+       if (flip > ecc_count && flip > (nfc->chip.ecc.strength / 2))
                return -1;
 
        /* Erased page. */



> I don't think that UBI/UbiFs use the OOB data at all, but that is
> something that I don't think is completely correct in this path?
> 
>> UBI starts to scrub & torture those 4 pages then, but starts doing
>> this with other pages too. All the pages start to fail then! I'm still
>> investigating that issue.
> 
> Arg.  I am not quite sure.  It seems ok to have a few 'stuck at one'
> errors.  As you move closer to the ECC strength this seems a little
> crazy.  At some point the sector/erase block is becoming useless.  I am
> not sure if this is an issue with this device; it seems a little
> suspect.  Maybe you could code something that emulates the stuck bits on
> a good device and see if it still behaves the same; triggers a software
> condition.  You might also reduce the NFC bus clock just to see on the
> 'stuck zero' device.
> 

I think we are facing multiple issues here. One might contain general
software/hardware issues (non bit-flip related). I had this issue again
on a different module with 3.18-rc5 (without the "fix" above). The
kernel output looks like this:

[    1.744672] UBI: default fastmap pool size: 200
[    1.752573] UBI: default fastmap WL pool size: 25
[    1.760577] UBI: attaching mtd3 to ubi0
[    1.769093] UBI warning: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 0:2048, read only 2048 bytes, retry
[    1.787579] UBI warning: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 0:2048, read only 2048 bytes, retry
[    1.806383] UBI warning: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 0:2048, read only 2048 bytes, retry
[    1.825649] UBI error: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 0:2048, read 2048 bytes
[    1.843804] CPU: 0 PID: 1 Comm: swapper Not tainted
3.18.0-rc5-00136-g0d91606-dirty #1607
[    1.860148] Backtrace:
[    1.866779] [<80011a04>] (dump_backtrace) from [<80011ce0>]
(show_stack+0x18/0x1c)
[    1.883061]  r6:00000000 r5:00000800 r4:ffffffb6 r3:00000000
[    1.893395] [<80011cc8>] (show_stack) from [<805a5018>]
(dump_stack+0x24/0x28)
[    1.909684] [<805a4ff4>] (dump_stack) from [<8034e9ac>]
(ubi_io_read+0x130/0x300)
[    1.926615] [<8034e87c>] (ubi_io_read) from [<8034efac>]
(ubi_io_read_vid_hdr+0x50/0x220)
[    1.944255]  r10:00000000 r9:00000000 r8:8ea4c000 r7:00000000
r6:8eaa3000 r5:00000000
[    1.961788]  r4:8ea4c000
[    1.969163] [<8034ef5c>] (ubi_io_read_vid_hdr) from [<803539ec>]
(scan_peb.part.9+0x128/0x6a4)
[    1.987614]  r10:00000000 r9:8e843dec r8:8ea4c000 r7:00000000
r6:8ead2900 r5:00000000
[    2.005493]  r4:80847a4c
[    2.012847] [<803538c4>] (scan_peb.part.9) from [<80354ed4>]
(ubi_attach+0x13c/0x398)
[    2.030220]  r10:ffffffff r9:80847a4c r8:ffffffff r7:7ffff000
r6:8ead2900 r5:8ea4c000
[    2.047761]  r4:00000000
[    2.054949] [<80354d98>] (ubi_attach) from [<80349504>]
(ubi_attach_mtd_dev+0x6a8/0xd00)
[    2.072405]  r10:00000050 r9:8ea13c00 r8:000007ff r7:8ea4c000
r6:00000000 r5:8ea13c00
[    2.089965]  r4:00000990
[    2.097179] [<80348e5c>] (ubi_attach_mtd_dev) from [<807933f0>]
(ubi_init+0x214/0x2b8)
[    2.114458]  r10:00000000 r9:8ead9200 r8:807785e8 r7:8ea13c00
r6:807a9700 r5:00000000
[    2.131856]  r4:807a9704
[    2.138962] [<807931dc>] (ubi_init) from [<80008770>]
(do_one_initcall+0x94/0x1d4)
[    2.155573]  r8:807785e8 r7:8e842038 r6:807931dc r5:807b9460
r4:807b9460
[    2.167084] [<800086dc>] (do_one_initcall) from [<80778e38>]
(kernel_init_freeable+0x130/0x1d0)
[    2.185090]  r10:807a44b0 r9:807a44a8 r8:807785e8 r7:807f4b80
r6:807f4b80 r5:00000007
[    2.202418]  r4:807ad4d8
[    2.209488] [<80778d08>] (kernel_init_freeable) from [<805a1b64>]
(kernel_init+0x10/0xf0)
[    2.226946]  r10:00000000 r9:00000000 r8:00000000 r7:00000000
r6:00000000 r5:805a1b54
[    2.244265]  r4:00000000
[    2.251337] [<805a1b54>] (kernel_init) from [<8000e638>]
(ret_from_fork+0x14/0x3c)
[    2.268179]  r4:00000000 r3:8e842000
[    2.297506] UBI warning: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 2:2048, read only 2048 bytes, retry
[    2.318557] UBI warning: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 2:2048, read only 2048 bytes, retry
[    2.340077] UBI warning: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 2:2048, read only 2048 bytes, retry
[    2.361438] UBI error: ubi_io_read: error -74 (ECC error) while
reading 2048 bytes from PEB 2:2048, read 2048 bytes
[    2.381479] CPU: 0 PID: 1 Comm: swapper Not tainted
3.18.0-rc5-00136-g0d91606-dirty #1607

Interesting is that this error happens every second PEB (every 128 page,
but erase block size is 64) and it is always the second page. On that
device, this is completely reproduceable, e.g. I can erase everything
and flash it again, the same happens. 


I dumped the block in question:

Page 00240800 dump:
        ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
        ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
        ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
        ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
        ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
        ff ff ff ff ff ff ff ff  f7 ff ff ff ff ff ff ff
        ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
        ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
        ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
        ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
        ff ff ff ff ff ff ff ff  ff ff fb ff ff ff ff ff
        ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
....
        ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff f7
....

I also printed flip count and ecc_count the values for all those pages
are: flip 3, ecc_count 2

Now the interesting part: When I erase the block, and dump that page
again, it is completely empty! No flips, no ecc_count anymore! UBI
attach writes something into the first page, hence it looks like this
write into the first page influences the values of the second page... I
verified this behavior this using U-Boot and the Linux kernel.

I digged a bit deeper, and wrote just zeros into the first page. In the
second page some bits are flipped. However, writing into the second page
does not influence the third page. But a bit in the first page is
flipped. And the third page influences the forth page. It looks like the
pages behave in pairs.... Any idea what kind of issue we are facing
here?

--
Stefan

Re: [RFC 2/5] mtd:fsl_nfc: Add hardware 45 byte BHC-ECC support for 24 bit corrections.

[Bill Pringlemeir]

>>>> On 17 Sep 2014, stefan@agner.ch wrote:

>>> Yes, we are using Macronix SLC NAND.

>>>> On 17 Sep 2014, stefan@agner.ch wrote:

>>> This is a new device, but its one out of several dozens. The device
>>> had two factory marked bad page. This four page would then be 6 bad
>>> pages. I would say that your guess is probably the case at hand
>>> (should be considered bad, but were marked by factory).

On 10 Dec 2014, stefan@agner.ch wrote:

> What I currently did, is just accept strength / 2 bits. This is not a
> clean solution since it will also count the ECC bits, but it works for
> now:
> --- a/drivers/mtd/nand/fsl_nfc.c
> +++ b/drivers/mtd/nand/fsl_nfc.c
> @@ -524,7 +524,7 @@ static int nfc_correct_data(struct mtd_info *mtd,
> u_char *dat,
> flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);
>
> /* ECC failed. */
> -       if (flip > ecc_count)
> +       if (flip > ecc_count && flip > (nfc->chip.ecc.strength / 2))
> return -1;
>
> /* Erased page. */

> I think we are facing multiple issues here. One might contain general
> software/hardware issues (non bit-flip related). I had this issue
> again on a different module with 3.18-rc5 (without the "fix"
> above). The kernel output looks like this:

[snip]

> Interesting is that this error happens every second PEB (every 128
> page, but erase block size is 64) and it is always the second page. On
> that device, this is completely reproduceable, e.g. I can erase
> everything and flash it again, the same happens.

> I dumped the block in question:

> Page 00240800 dump:
> ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> ....
> ff ff ff ff ff ff ff ff  f7 ff ff ff ff ff ff ff
> ....
> ff ff ff ff ff ff ff ff  ff ff fb ff ff ff ff ff
> ....
> ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff f7
> ....

> I also printed flip count and ecc_count the values for all those pages
> are: flip 3, ecc_count 2

> Now the interesting part: When I erase the block, and dump that page
> again, it is completely empty! No flips, no ecc_count anymore! UBI
> attach writes something into the first page, hence it looks like this
> write into the first page influences the values of the second
> page... I verified this behavior this using U-Boot and the Linux
> kernel.

> I digged a bit deeper, and wrote just zeros into the first page. In
> the second page some bits are flipped. However, writing into the
> second page does not influence the third page. But a bit in the first
> page is flipped. And the third page influences the forth page. It
> looks like the pages behave in pairs.... Any idea what kind of issue
> we are facing here?

Hmm.  It sounds like MLC flash, but you say you have SLC.  It could be
that some bus signalling is marginal?  Could you reduce the clocks a bit
on this device and see if the behaviour changes?  I am pretty sure that
stuck-at-zero errors will stay that way.

I would love to get back to this controller code to fix some issues you
noted and bring in the changes to the u-boot review.  Unfortunately, I
keep getting stuck with legacy hw issues.

fwiw,
Bill.

[RFC 2/5] mtd:fsl_nfc: Add hardware 45 byte BHC-ECC support for 24 bit corrections.

[Bill Pringlemeir]

>>>> On 17 Sep 2014, stefan at agner.ch wrote:

>>> Yes, we are using Macronix SLC NAND.

>>>> On 17 Sep 2014, stefan at agner.ch wrote:

>>> This is a new device, but its one out of several dozens. The device
>>> had two factory marked bad page. This four page would then be 6 bad
>>> pages. I would say that your guess is probably the case at hand
>>> (should be considered bad, but were marked by factory).

On 10 Dec 2014, stefan at agner.ch wrote:

> What I currently did, is just accept strength / 2 bits. This is not a
> clean solution since it will also count the ECC bits, but it works for
> now:
> --- a/drivers/mtd/nand/fsl_nfc.c
> +++ b/drivers/mtd/nand/fsl_nfc.c
> @@ -524,7 +524,7 @@ static int nfc_correct_data(struct mtd_info *mtd,
> u_char *dat,
> flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);
>
> /* ECC failed. */
> -       if (flip > ecc_count)
> +       if (flip > ecc_count && flip > (nfc->chip.ecc.strength / 2))
> return -1;
>
> /* Erased page. */

> I think we are facing multiple issues here. One might contain general
> software/hardware issues (non bit-flip related). I had this issue
> again on a different module with 3.18-rc5 (without the "fix"
> above). The kernel output looks like this:

[snip]

> Interesting is that this error happens every second PEB (every 128
> page, but erase block size is 64) and it is always the second page. On
> that device, this is completely reproduceable, e.g. I can erase
> everything and flash it again, the same happens.

> I dumped the block in question:

> Page 00240800 dump:
> ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
> ....
> ff ff ff ff ff ff ff ff  f7 ff ff ff ff ff ff ff
> ....
> ff ff ff ff ff ff ff ff  ff ff fb ff ff ff ff ff
> ....
> ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff f7
> ....

> I also printed flip count and ecc_count the values for all those pages
> are: flip 3, ecc_count 2

> Now the interesting part: When I erase the block, and dump that page
> again, it is completely empty! No flips, no ecc_count anymore! UBI
> attach writes something into the first page, hence it looks like this
> write into the first page influences the values of the second
> page... I verified this behavior this using U-Boot and the Linux
> kernel.

> I digged a bit deeper, and wrote just zeros into the first page. In
> the second page some bits are flipped. However, writing into the
> second page does not influence the third page. But a bit in the first
> page is flipped. And the third page influences the forth page. It
> looks like the pages behave in pairs.... Any idea what kind of issue
> we are facing here?

Hmm.  It sounds like MLC flash, but you say you have SLC.  It could be
that some bus signalling is marginal?  Could you reduce the clocks a bit
on this device and see if the behaviour changes?  I am pretty sure that
stuck-at-zero errors will stay that way.

I would love to get back to this controller code to fix some issues you
noted and bring in the changes to the u-boot review.  Unfortunately, I
keep getting stuck with legacy hw issues.

fwiw,
Bill.

Re: [RFC 2/5] mtd:fsl_nfc: Add hardware 45 byte BHC-ECC support for 24 bit corrections.

[Stefan Agner]

On 2014-12-11 17:44, Bill Pringlemeir wrote:
>>>>> On 17 Sep 2014, stefan@agner.ch wrote:
> 
>>>> Yes, we are using Macronix SLC NAND.
> 
>>>>> On 17 Sep 2014, stefan@agner.ch wrote:
> 
>>>> This is a new device, but its one out of several dozens. The device
>>>> had two factory marked bad page. This four page would then be 6 bad
>>>> pages. I would say that your guess is probably the case at hand
>>>> (should be considered bad, but were marked by factory).
> 
> On 10 Dec 2014, stefan@agner.ch wrote:
> 
>> What I currently did, is just accept strength / 2 bits. This is not a
>> clean solution since it will also count the ECC bits, but it works for
>> now:
>> --- a/drivers/mtd/nand/fsl_nfc.c
>> +++ b/drivers/mtd/nand/fsl_nfc.c
>> @@ -524,7 +524,7 @@ static int nfc_correct_data(struct mtd_info *mtd,
>> u_char *dat,
>> flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);
>>
>> /* ECC failed. */
>> -       if (flip > ecc_count)
>> +       if (flip > ecc_count && flip > (nfc->chip.ecc.strength / 2))
>> return -1;
>>
>> /* Erased page. */
> 
>> I think we are facing multiple issues here. One might contain general
>> software/hardware issues (non bit-flip related). I had this issue
>> again on a different module with 3.18-rc5 (without the "fix"
>> above). The kernel output looks like this:
> 
> [snip]
> 
>> Interesting is that this error happens every second PEB (every 128
>> page, but erase block size is 64) and it is always the second page. On
>> that device, this is completely reproduceable, e.g. I can erase
>> everything and flash it again, the same happens.
> 
>> I dumped the block in question:
> 
>> Page 00240800 dump:
>> ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
>> ....
>> ff ff ff ff ff ff ff ff  f7 ff ff ff ff ff ff ff
>> ....
>> ff ff ff ff ff ff ff ff  ff ff fb ff ff ff ff ff
>> ....
>> ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff f7
>> ....
> 
>> I also printed flip count and ecc_count the values for all those pages
>> are: flip 3, ecc_count 2
> 
>> Now the interesting part: When I erase the block, and dump that page
>> again, it is completely empty! No flips, no ecc_count anymore! UBI
>> attach writes something into the first page, hence it looks like this
>> write into the first page influences the values of the second
>> page... I verified this behavior this using U-Boot and the Linux
>> kernel.
> 
>> I digged a bit deeper, and wrote just zeros into the first page. In
>> the second page some bits are flipped. However, writing into the
>> second page does not influence the third page. But a bit in the first
>> page is flipped. And the third page influences the forth page. It
>> looks like the pages behave in pairs.... Any idea what kind of issue
>> we are facing here?
> 
> Hmm.  It sounds like MLC flash, but you say you have SLC.  It could be
> that some bus signalling is marginal?  Could you reduce the clocks a bit
> on this device and see if the behaviour changes?  I am pretty sure that
> stuck-at-zero errors will stay that way.
> 
> I would love to get back to this controller code to fix some issues you
> noted and bring in the changes to the u-boot review.  Unfortunately, I
> keep getting stuck with legacy hw issues.
> 
> fwiw,
> Bill.

Hi Bill,

The flash chip mentioned above requires 8-bit error correction per 512
byte block, hence I increased the ECC to the maximum available level
(60-byte ECC, see page below). One thing which is not very nice, in
order to fit the 60-byte ECC into the 64-byte OOB, I had to shorten the
BBT pattern and set it at the very beginning of the page. This works
fine, however this basically sets the page also to factory bad, I'm not
sure if this is ok? Otherwise, we also could use a BBT pattern of length
1 (used by cafe_nand.c too).

What do you think? I would like to respin the NFC patch, with my U-Boot
changes and this change included...

--
Stefan

diff --git a/drivers/mtd/nand/fsl_nfc.c b/drivers/mtd/nand/fsl_nfc.c
index bfc7b7b..3d6da80 100644
--- a/drivers/mtd/nand/fsl_nfc.c
+++ b/drivers/mtd/nand/fsl_nfc.c
@@ -70,6 +70,7 @@
 /* NFC ECC mode define */
 #define ECC_BYPASS                     0
 #define ECC_45_BYTE                    6
+#define ECC_60_BYTE                    7
 
 /*** Register Mask and bit definitions */
 
@@ -155,15 +156,15 @@ struct fsl_nfc {
 };
 #define mtd_to_nfc(_mtd) container_of(_mtd, struct fsl_nfc, mtd)
 
-static u8 bbt_pattern[] = {'B', 'b', 't', '0' };
-static u8 mirror_pattern[] = {'1', 't', 'b', 'B' };
+static u8 bbt_pattern[] = {'B', 'b', 't' };
+static u8 mirror_pattern[] = {'t', 'b', 'B' };
 
 static struct nand_bbt_descr bbt_main_descr = {
        .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
|
                   NAND_BBT_2BIT | NAND_BBT_VERSION,
-       .offs = 11,
-       .len = 4,
-       .veroffs = 15,
+       .offs = 0,
+       .len = 3,
+       .veroffs = 3,
        .maxblocks = 4,
        .pattern = bbt_pattern,
 };
@@ -171,9 +172,9 @@ static struct nand_bbt_descr bbt_main_descr = {
 static struct nand_bbt_descr bbt_mirror_descr = {
        .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
|
                   NAND_BBT_2BIT | NAND_BBT_VERSION,
-       .offs = 11,
-       .len = 4,
-       .veroffs = 15,
+       .offs = 0,
+       .len = 3,
+       .veroffs = 3,
        .maxblocks = 4,
        .pattern = mirror_pattern,
 };
@@ -191,6 +192,21 @@ static struct nand_ecclayout nfc_ecc45 = {
                 .length = 11} }
 };
 
+static struct nand_ecclayout nfc_ecc60 = {
+       .eccbytes = 60,
+       .eccpos = { 4,  5,  6,  7,  8,  9, 10, 11,
+                  12, 13, 14, 15, 16, 17, 18, 19,
+                  20, 21, 22, 23, 24, 25, 26, 27,
+                  28, 29, 30, 31, 32, 33, 34, 35,
+                  36, 37, 38, 39, 40, 41, 42, 43,
+                  44, 45, 46, 47, 48, 49, 50, 51,
diff --git a/drivers/mtd/nand/fsl_nfc.c b/drivers/mtd/nand/fsl_nfc.c
index bfc7b7b..3d6da80 100644
--- a/drivers/mtd/nand/fsl_nfc.c
+++ b/drivers/mtd/nand/fsl_nfc.c
@@ -70,6 +70,7 @@
 /* NFC ECC mode define */
 #define ECC_BYPASS                     0
 #define ECC_45_BYTE                    6
+#define ECC_60_BYTE                    7
 
 /*** Register Mask and bit definitions */
 
@@ -155,15 +156,15 @@ struct fsl_nfc {
 };
 #define mtd_to_nfc(_mtd) container_of(_mtd, struct fsl_nfc, mtd)
 
-static u8 bbt_pattern[] = {'B', 'b', 't', '0' };
-static u8 mirror_pattern[] = {'1', 't', 'b', 'B' };
+static u8 bbt_pattern[] = {'B', 'b', 't' };
+static u8 mirror_pattern[] = {'t', 'b', 'B' };
 
 static struct nand_bbt_descr bbt_main_descr = {
        .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
|
                   NAND_BBT_2BIT | NAND_BBT_VERSION,
-       .offs = 11,
-       .len = 4,
-       .veroffs = 15,
+       .offs = 0,
+       .len = 3,
+       .veroffs = 3,
        .maxblocks = 4,
        .pattern = bbt_pattern,
 };
@@ -171,9 +172,9 @@ static struct nand_bbt_descr bbt_main_descr = {
 static struct nand_bbt_descr bbt_mirror_descr = {
        .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
|
                   NAND_BBT_2BIT | NAND_BBT_VERSION,
-       .offs = 11,
-       .len = 4,
-       .veroffs = 15,
+       .offs = 0,
+       .len = 3,
+       .veroffs = 3,
        .maxblocks = 4,
        .pattern = mirror_pattern,
 };
@@ -191,6 +192,21 @@ static struct nand_ecclayout nfc_ecc45 = {
                 .length = 11} }
 };
 
+static struct nand_ecclayout nfc_ecc60 = {
+       .eccbytes = 60,
+       .eccpos = { 4,  5,  6,  7,  8,  9, 10, 11,
+                  12, 13, 14, 15, 16, 17, 18, 19,
+                  20, 21, 22, 23, 24, 25, 26, 27,
+                  28, 29, 30, 31, 32, 33, 34, 35,
+                  36, 37, 38, 39, 40, 41, 42, 43,
+                  44, 45, 46, 47, 48, 49, 50, 51,
+                  52, 53, 54, 55, 56, 57, 58, 59,
+                  60, 61, 62, 63 },
+       .oobfree = {
+               {.offset = 2,
+                .length = 2} }
+};
+
 static u32 nfc_read(struct mtd_info *mtd, uint reg)
 {
        struct fsl_nfc *nfc = mtd_to_nfc(mtd);
@@ -608,10 +624,10 @@ static int nfc_init_controller(struct mtd_info
*mtd, struct nfc_config *cfg, int
        nfc_write(mtd, NFC_SECTOR_SIZE, page_sz);
 
        if (cfg->hardware_ecc) {
-               /* set ECC mode to 45 bytes OOB with 24 bits correction
*/
+               /* set ECC mode to 60 bytes OOB with 32 bits correction
*/
                nfc_set_field(mtd, NFC_FLASH_CONFIG,
                                CONFIG_ECC_MODE_MASK,
-                               CONFIG_ECC_MODE_SHIFT, ECC_45_BYTE);
+                               CONFIG_ECC_MODE_SHIFT, ECC_60_BYTE);
 
                /* Enable ECC_STATUS */
                nfc_set(mtd, NFC_FLASH_CONFIG, CONFIG_ECC_SRAM_REQ_BIT);
@@ -742,7 +758,7 @@ static int nfc_probe(struct platform_device *pdev)
                        goto error;
                }
 
-               chip->ecc.layout = &nfc_ecc45;
+               chip->ecc.layout = &nfc_ecc60;
 
                /* propagate ecc.layout to mtd_info */
                mtd->ecclayout = chip->ecc.layout;
@@ -751,14 +767,14 @@ static int nfc_probe(struct platform_device *pdev)
                chip->ecc.correct = nfc_correct_data;
                chip->ecc.mode = NAND_ECC_HW;
 
-               chip->ecc.bytes = 45;
+               chip->ecc.bytes = 60;
                chip->ecc.size = PAGE_2K;
-               chip->ecc.strength = 24;
+               chip->ecc.strength = 32;
 
-               /* set ECC mode to 45 bytes OOB with 24 bits correction
*/
+               /* set ECC mode to 60 bytes OOB with 32 bits correction
*/
                nfc_set_field(mtd, NFC_FLASH_CONFIG,
                                CONFIG_ECC_MODE_MASK,
-                               CONFIG_ECC_MODE_SHIFT, ECC_45_BYTE);
+                               CONFIG_ECC_MODE_SHIFT, ECC_60_BYTE);
        }
 
        /* second phase scan */

[RFC 2/5] mtd:fsl_nfc: Add hardware 45 byte BHC-ECC support for 24 bit corrections.

[Stefan Agner]

On 2014-12-11 17:44, Bill Pringlemeir wrote:
>>>>> On 17 Sep 2014, stefan at agner.ch wrote:
> 
>>>> Yes, we are using Macronix SLC NAND.
> 
>>>>> On 17 Sep 2014, stefan at agner.ch wrote:
> 
>>>> This is a new device, but its one out of several dozens. The device
>>>> had two factory marked bad page. This four page would then be 6 bad
>>>> pages. I would say that your guess is probably the case at hand
>>>> (should be considered bad, but were marked by factory).
> 
> On 10 Dec 2014, stefan at agner.ch wrote:
> 
>> What I currently did, is just accept strength / 2 bits. This is not a
>> clean solution since it will also count the ECC bits, but it works for
>> now:
>> --- a/drivers/mtd/nand/fsl_nfc.c
>> +++ b/drivers/mtd/nand/fsl_nfc.c
>> @@ -524,7 +524,7 @@ static int nfc_correct_data(struct mtd_info *mtd,
>> u_char *dat,
>> flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);
>>
>> /* ECC failed. */
>> -       if (flip > ecc_count)
>> +       if (flip > ecc_count && flip > (nfc->chip.ecc.strength / 2))
>> return -1;
>>
>> /* Erased page. */
> 
>> I think we are facing multiple issues here. One might contain general
>> software/hardware issues (non bit-flip related). I had this issue
>> again on a different module with 3.18-rc5 (without the "fix"
>> above). The kernel output looks like this:
> 
> [snip]
> 
>> Interesting is that this error happens every second PEB (every 128
>> page, but erase block size is 64) and it is always the second page. On
>> that device, this is completely reproduceable, e.g. I can erase
>> everything and flash it again, the same happens.
> 
>> I dumped the block in question:
> 
>> Page 00240800 dump:
>> ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff ff
>> ....
>> ff ff ff ff ff ff ff ff  f7 ff ff ff ff ff ff ff
>> ....
>> ff ff ff ff ff ff ff ff  ff ff fb ff ff ff ff ff
>> ....
>> ff ff ff ff ff ff ff ff  ff ff ff ff ff ff ff f7
>> ....
> 
>> I also printed flip count and ecc_count the values for all those pages
>> are: flip 3, ecc_count 2
> 
>> Now the interesting part: When I erase the block, and dump that page
>> again, it is completely empty! No flips, no ecc_count anymore! UBI
>> attach writes something into the first page, hence it looks like this
>> write into the first page influences the values of the second
>> page... I verified this behavior this using U-Boot and the Linux
>> kernel.
> 
>> I digged a bit deeper, and wrote just zeros into the first page. In
>> the second page some bits are flipped. However, writing into the
>> second page does not influence the third page. But a bit in the first
>> page is flipped. And the third page influences the forth page. It
>> looks like the pages behave in pairs.... Any idea what kind of issue
>> we are facing here?
> 
> Hmm.  It sounds like MLC flash, but you say you have SLC.  It could be
> that some bus signalling is marginal?  Could you reduce the clocks a bit
> on this device and see if the behaviour changes?  I am pretty sure that
> stuck-at-zero errors will stay that way.
> 
> I would love to get back to this controller code to fix some issues you
> noted and bring in the changes to the u-boot review.  Unfortunately, I
> keep getting stuck with legacy hw issues.
> 
> fwiw,
> Bill.

Hi Bill,

The flash chip mentioned above requires 8-bit error correction per 512
byte block, hence I increased the ECC to the maximum available level
(60-byte ECC, see page below). One thing which is not very nice, in
order to fit the 60-byte ECC into the 64-byte OOB, I had to shorten the
BBT pattern and set it at the very beginning of the page. This works
fine, however this basically sets the page also to factory bad, I'm not
sure if this is ok? Otherwise, we also could use a BBT pattern of length
1 (used by cafe_nand.c too).

What do you think? I would like to respin the NFC patch, with my U-Boot
changes and this change included...

--
Stefan

diff --git a/drivers/mtd/nand/fsl_nfc.c b/drivers/mtd/nand/fsl_nfc.c
index bfc7b7b..3d6da80 100644
--- a/drivers/mtd/nand/fsl_nfc.c
+++ b/drivers/mtd/nand/fsl_nfc.c
@@ -70,6 +70,7 @@
 /* NFC ECC mode define */
 #define ECC_BYPASS                     0
 #define ECC_45_BYTE                    6
+#define ECC_60_BYTE                    7
 
 /*** Register Mask and bit definitions */
 
@@ -155,15 +156,15 @@ struct fsl_nfc {
 };
 #define mtd_to_nfc(_mtd) container_of(_mtd, struct fsl_nfc, mtd)
 
-static u8 bbt_pattern[] = {'B', 'b', 't', '0' };
-static u8 mirror_pattern[] = {'1', 't', 'b', 'B' };
+static u8 bbt_pattern[] = {'B', 'b', 't' };
+static u8 mirror_pattern[] = {'t', 'b', 'B' };
 
 static struct nand_bbt_descr bbt_main_descr = {
        .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
|
                   NAND_BBT_2BIT | NAND_BBT_VERSION,
-       .offs = 11,
-       .len = 4,
-       .veroffs = 15,
+       .offs = 0,
+       .len = 3,
+       .veroffs = 3,
        .maxblocks = 4,
        .pattern = bbt_pattern,
 };
@@ -171,9 +172,9 @@ static struct nand_bbt_descr bbt_main_descr = {
 static struct nand_bbt_descr bbt_mirror_descr = {
        .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
|
                   NAND_BBT_2BIT | NAND_BBT_VERSION,
-       .offs = 11,
-       .len = 4,
-       .veroffs = 15,
+       .offs = 0,
+       .len = 3,
+       .veroffs = 3,
        .maxblocks = 4,
        .pattern = mirror_pattern,
 };
@@ -191,6 +192,21 @@ static struct nand_ecclayout nfc_ecc45 = {
                 .length = 11} }
 };
 
+static struct nand_ecclayout nfc_ecc60 = {
+       .eccbytes = 60,
+       .eccpos = { 4,  5,  6,  7,  8,  9, 10, 11,
+                  12, 13, 14, 15, 16, 17, 18, 19,
+                  20, 21, 22, 23, 24, 25, 26, 27,
+                  28, 29, 30, 31, 32, 33, 34, 35,
+                  36, 37, 38, 39, 40, 41, 42, 43,
+                  44, 45, 46, 47, 48, 49, 50, 51,
diff --git a/drivers/mtd/nand/fsl_nfc.c b/drivers/mtd/nand/fsl_nfc.c
index bfc7b7b..3d6da80 100644
--- a/drivers/mtd/nand/fsl_nfc.c
+++ b/drivers/mtd/nand/fsl_nfc.c
@@ -70,6 +70,7 @@
 /* NFC ECC mode define */
 #define ECC_BYPASS                     0
 #define ECC_45_BYTE                    6
+#define ECC_60_BYTE                    7
 
 /*** Register Mask and bit definitions */
 
@@ -155,15 +156,15 @@ struct fsl_nfc {
 };
 #define mtd_to_nfc(_mtd) container_of(_mtd, struct fsl_nfc, mtd)
 
-static u8 bbt_pattern[] = {'B', 'b', 't', '0' };
-static u8 mirror_pattern[] = {'1', 't', 'b', 'B' };
+static u8 bbt_pattern[] = {'B', 'b', 't' };
+static u8 mirror_pattern[] = {'t', 'b', 'B' };
 
 static struct nand_bbt_descr bbt_main_descr = {
        .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
|
                   NAND_BBT_2BIT | NAND_BBT_VERSION,
-       .offs = 11,
-       .len = 4,
-       .veroffs = 15,
+       .offs = 0,
+       .len = 3,
+       .veroffs = 3,
        .maxblocks = 4,
        .pattern = bbt_pattern,
 };
@@ -171,9 +172,9 @@ static struct nand_bbt_descr bbt_main_descr = {
 static struct nand_bbt_descr bbt_mirror_descr = {
        .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
|
                   NAND_BBT_2BIT | NAND_BBT_VERSION,
-       .offs = 11,
-       .len = 4,
-       .veroffs = 15,
+       .offs = 0,
+       .len = 3,
+       .veroffs = 3,
        .maxblocks = 4,
        .pattern = mirror_pattern,
 };
@@ -191,6 +192,21 @@ static struct nand_ecclayout nfc_ecc45 = {
                 .length = 11} }
 };
 
+static struct nand_ecclayout nfc_ecc60 = {
+       .eccbytes = 60,
+       .eccpos = { 4,  5,  6,  7,  8,  9, 10, 11,
+                  12, 13, 14, 15, 16, 17, 18, 19,
+                  20, 21, 22, 23, 24, 25, 26, 27,
+                  28, 29, 30, 31, 32, 33, 34, 35,
+                  36, 37, 38, 39, 40, 41, 42, 43,
+                  44, 45, 46, 47, 48, 49, 50, 51,
+                  52, 53, 54, 55, 56, 57, 58, 59,
+                  60, 61, 62, 63 },
+       .oobfree = {
+               {.offset = 2,
+                .length = 2} }
+};
+
 static u32 nfc_read(struct mtd_info *mtd, uint reg)
 {
        struct fsl_nfc *nfc = mtd_to_nfc(mtd);
@@ -608,10 +624,10 @@ static int nfc_init_controller(struct mtd_info
*mtd, struct nfc_config *cfg, int
        nfc_write(mtd, NFC_SECTOR_SIZE, page_sz);
 
        if (cfg->hardware_ecc) {
-               /* set ECC mode to 45 bytes OOB with 24 bits correction
*/
+               /* set ECC mode to 60 bytes OOB with 32 bits correction
*/
                nfc_set_field(mtd, NFC_FLASH_CONFIG,
                                CONFIG_ECC_MODE_MASK,
-                               CONFIG_ECC_MODE_SHIFT, ECC_45_BYTE);
+                               CONFIG_ECC_MODE_SHIFT, ECC_60_BYTE);
 
                /* Enable ECC_STATUS */
                nfc_set(mtd, NFC_FLASH_CONFIG, CONFIG_ECC_SRAM_REQ_BIT);
@@ -742,7 +758,7 @@ static int nfc_probe(struct platform_device *pdev)
                        goto error;
                }
 
-               chip->ecc.layout = &nfc_ecc45;
+               chip->ecc.layout = &nfc_ecc60;
 
                /* propagate ecc.layout to mtd_info */
                mtd->ecclayout = chip->ecc.layout;
@@ -751,14 +767,14 @@ static int nfc_probe(struct platform_device *pdev)
                chip->ecc.correct = nfc_correct_data;
                chip->ecc.mode = NAND_ECC_HW;
 
-               chip->ecc.bytes = 45;
+               chip->ecc.bytes = 60;
                chip->ecc.size = PAGE_2K;
-               chip->ecc.strength = 24;
+               chip->ecc.strength = 32;
 
-               /* set ECC mode to 45 bytes OOB with 24 bits correction
*/
+               /* set ECC mode to 60 bytes OOB with 32 bits correction
*/
                nfc_set_field(mtd, NFC_FLASH_CONFIG,
                                CONFIG_ECC_MODE_MASK,
-                               CONFIG_ECC_MODE_SHIFT, ECC_45_BYTE);
+                               CONFIG_ECC_MODE_SHIFT, ECC_60_BYTE);
        }
 
        /* second phase scan */

Re: [RFC 2/5] mtd:fsl_nfc: Add hardware 45 byte BHC-ECC support for 24 bit corrections.

[Bill Pringlemeir]

On 28 Feb 2015, stefan@agner.ch wrote:

> The flash chip mentioned above requires 8-bit error correction per 512
> byte block, hence I increased the ECC to the maximum available level
> (60-byte ECC, see page below). One thing which is not very nice, in
> order to fit the 60-byte ECC into the 64-byte OOB, I had to shorten
> the BBT pattern and set it at the very beginning of the page. This
> works fine, however this basically sets the page also to factory bad,
> I'm not sure if this is ok? Otherwise, we also could use a BBT pattern
> of length 1 (used by cafe_nand.c too).

I guess that is a DT option?  I wouldn't be an expert on this.  So
submitting it to the linux-mtd is good.  

I am also not sure if the HW ECC will work with 'sub-pages'.  I think a
college of your at Toradex submitted a patch to the u-boot.  I am pretty
sure that it could work with software ECC, but maybe disabling it is
easiest.

> What do you think? I would like to respin the NFC patch, with my
> U-Boot changes and this change included...

Please go ahead.  Markus M <marb@ixxat.de> is also using the fsl_nfc
driver on a Freescale MPC5125 board, so it is probably good to copy your
patches to him.  At least, he can test on a BE platform.

People also complained about JFFS and this version of the driver.  I
didn't investigate that.

Thanks,
Bill Pringlemeir.

[RFC 2/5] mtd:fsl_nfc: Add hardware 45 byte BHC-ECC support for 24 bit corrections.

[Bill Pringlemeir]

On 28 Feb 2015, stefan at agner.ch wrote:

> The flash chip mentioned above requires 8-bit error correction per 512
> byte block, hence I increased the ECC to the maximum available level
> (60-byte ECC, see page below). One thing which is not very nice, in
> order to fit the 60-byte ECC into the 64-byte OOB, I had to shorten
> the BBT pattern and set it at the very beginning of the page. This
> works fine, however this basically sets the page also to factory bad,
> I'm not sure if this is ok? Otherwise, we also could use a BBT pattern
> of length 1 (used by cafe_nand.c too).

I guess that is a DT option?  I wouldn't be an expert on this.  So
submitting it to the linux-mtd is good.  

I am also not sure if the HW ECC will work with 'sub-pages'.  I think a
college of your at Toradex submitted a patch to the u-boot.  I am pretty
sure that it could work with software ECC, but maybe disabling it is
easiest.

> What do you think? I would like to respin the NFC patch, with my
> U-Boot changes and this change included...

Please go ahead.  Markus M <marb@ixxat.de> is also using the fsl_nfc
driver on a Freescale MPC5125 board, so it is probably good to copy your
patches to him.  At least, he can test on a BE platform.

People also complained about JFFS and this version of the driver.  I
didn't investigate that.

Thanks,
Bill Pringlemeir.

Re: [RFC 2/5] mtd:fsl_nfc: Add hardware 45 byte BHC-ECC support for 24 bit corrections.

[Aaron Brice]

On 03/02/2015 08:05 AM, Bill Pringlemeir wrote:
> On 28 Feb 2015, stefan@agner.ch wrote:
>
>> The flash chip mentioned above requires 8-bit error correction per 512
>> byte block, hence I increased the ECC to the maximum available level
>> (60-byte ECC, see page below). One thing which is not very nice, in
>> order to fit the 60-byte ECC into the 64-byte OOB, I had to shorten
>> the BBT pattern and set it at the very beginning of the page. This
>> works fine, however this basically sets the page also to factory bad,
>> I'm not sure if this is ok? Otherwise, we also could use a BBT pattern
>> of length 1 (used by cafe_nand.c too).
> I guess that is a DT option?  I wouldn't be an expert on this.  So
> submitting it to the linux-mtd is good.
>
> I am also not sure if the HW ECC will work with 'sub-pages'.  I think a
> college of your at Toradex submitted a patch to the u-boot.  I am pretty
> sure that it could work with software ECC, but maybe disabling it is
> easiest.
>
>> What do you think? I would like to respin the NFC patch, with my
>> U-Boot changes and this change included...
> Please go ahead.  Markus M <marb@ixxat.de> is also using the fsl_nfc
> driver on a Freescale MPC5125 board, so it is probably good to copy your
> patches to him.  At least, he can test on a BE platform.
>
> People also complained about JFFS and this version of the driver.  I
> didn't investigate that.

I also noticed a problem with JFFS2 and the driver, where fs changes 
were lost after reboot.  Didn't investigate, switched to UBIFS..

[RFC 2/5] mtd:fsl_nfc: Add hardware 45 byte BHC-ECC support for 24 bit corrections.

[Aaron Brice]

On 03/02/2015 08:05 AM, Bill Pringlemeir wrote:
> On 28 Feb 2015, stefan at agner.ch wrote:
>
>> The flash chip mentioned above requires 8-bit error correction per 512
>> byte block, hence I increased the ECC to the maximum available level
>> (60-byte ECC, see page below). One thing which is not very nice, in
>> order to fit the 60-byte ECC into the 64-byte OOB, I had to shorten
>> the BBT pattern and set it at the very beginning of the page. This
>> works fine, however this basically sets the page also to factory bad,
>> I'm not sure if this is ok? Otherwise, we also could use a BBT pattern
>> of length 1 (used by cafe_nand.c too).
> I guess that is a DT option?  I wouldn't be an expert on this.  So
> submitting it to the linux-mtd is good.
>
> I am also not sure if the HW ECC will work with 'sub-pages'.  I think a
> college of your at Toradex submitted a patch to the u-boot.  I am pretty
> sure that it could work with software ECC, but maybe disabling it is
> easiest.
>
>> What do you think? I would like to respin the NFC patch, with my
>> U-Boot changes and this change included...
> Please go ahead.  Markus M <marb@ixxat.de> is also using the fsl_nfc
> driver on a Freescale MPC5125 board, so it is probably good to copy your
> patches to him.  At least, he can test on a BE platform.
>
> People also complained about JFFS and this version of the driver.  I
> didn't investigate that.

I also noticed a problem with JFFS2 and the driver, where fs changes 
were lost after reboot.  Didn't investigate, switched to UBIFS..

Re: [RFC 2/5] mtd:fsl_nfc: Add hardware 45 byte BHC-ECC support for 24 bit corrections.

[Stefan Agner]

On 2015-03-02 22:39, Aaron Brice wrote:
> On 03/02/2015 08:05 AM, Bill Pringlemeir wrote:
>> On 28 Feb 2015, stefan@agner.ch wrote:
>>
>>> The flash chip mentioned above requires 8-bit error correction per 512
>>> byte block, hence I increased the ECC to the maximum available level
>>> (60-byte ECC, see page below). One thing which is not very nice, in
>>> order to fit the 60-byte ECC into the 64-byte OOB, I had to shorten
>>> the BBT pattern and set it at the very beginning of the page. This
>>> works fine, however this basically sets the page also to factory bad,
>>> I'm not sure if this is ok? Otherwise, we also could use a BBT pattern
>>> of length 1 (used by cafe_nand.c too).
>> I guess that is a DT option?  I wouldn't be an expert on this.  So
>> submitting it to the linux-mtd is good.
>>
>> I am also not sure if the HW ECC will work with 'sub-pages'.  I think a
>> college of your at Toradex submitted a patch to the u-boot.  I am pretty
>> sure that it could work with software ECC, but maybe disabling it is
>> easiest.
>>
>>> What do you think? I would like to respin the NFC patch, with my
>>> U-Boot changes and this change included...
>> Please go ahead.  Markus M <marb@ixxat.de> is also using the fsl_nfc
>> driver on a Freescale MPC5125 board, so it is probably good to copy your
>> patches to him.  At least, he can test on a BE platform.
>>
>> People also complained about JFFS and this version of the driver.  I
>> didn't investigate that.
> 
> I also noticed a problem with JFFS2 and the driver, where fs changes
> were lost after reboot.  Didn't investigate, switched to UBIFS..

Did you happen to use HW ECC? The controller seems to use byte 19
onwards to store the ECC bytes, where also JFFS2 stores it's meta data
when using a NAND chip with 64-byte OOB (see
http://www.linux-mtd.infradead.org/doc/nand.html). However, I think
UBI/UBIFS is a good choice anyway.

--
Stefan

[RFC 2/5] mtd:fsl_nfc: Add hardware 45 byte BHC-ECC support for 24 bit corrections.

[Stefan Agner]

On 2015-03-02 22:39, Aaron Brice wrote:
> On 03/02/2015 08:05 AM, Bill Pringlemeir wrote:
>> On 28 Feb 2015, stefan at agner.ch wrote:
>>
>>> The flash chip mentioned above requires 8-bit error correction per 512
>>> byte block, hence I increased the ECC to the maximum available level
>>> (60-byte ECC, see page below). One thing which is not very nice, in
>>> order to fit the 60-byte ECC into the 64-byte OOB, I had to shorten
>>> the BBT pattern and set it at the very beginning of the page. This
>>> works fine, however this basically sets the page also to factory bad,
>>> I'm not sure if this is ok? Otherwise, we also could use a BBT pattern
>>> of length 1 (used by cafe_nand.c too).
>> I guess that is a DT option?  I wouldn't be an expert on this.  So
>> submitting it to the linux-mtd is good.
>>
>> I am also not sure if the HW ECC will work with 'sub-pages'.  I think a
>> college of your at Toradex submitted a patch to the u-boot.  I am pretty
>> sure that it could work with software ECC, but maybe disabling it is
>> easiest.
>>
>>> What do you think? I would like to respin the NFC patch, with my
>>> U-Boot changes and this change included...
>> Please go ahead.  Markus M <marb@ixxat.de> is also using the fsl_nfc
>> driver on a Freescale MPC5125 board, so it is probably good to copy your
>> patches to him.  At least, he can test on a BE platform.
>>
>> People also complained about JFFS and this version of the driver.  I
>> didn't investigate that.
> 
> I also noticed a problem with JFFS2 and the driver, where fs changes
> were lost after reboot.  Didn't investigate, switched to UBIFS..

Did you happen to use HW ECC? The controller seems to use byte 19
onwards to store the ECC bytes, where also JFFS2 stores it's meta data
when using a NAND chip with 64-byte OOB (see
http://www.linux-mtd.infradead.org/doc/nand.html). However, I think
UBI/UBIFS is a good choice anyway.

--
Stefan

[RFC 3/5] mtd:fsl_nfc: Add device tree documentation.

[Bill Pringlemeir]

Signed-off-by: Bill Pringlemeir <bpringlemeir@nbsps.com>
---
 Documentation/devicetree/bindings/mtd/fsl-nfc.txt | 25 +++++++++++++++++++++++
 1 file changed, 25 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/fsl-nfc.txt

diff --git a/Documentation/devicetree/bindings/mtd/fsl-nfc.txt b/Documentation/devicetree/bindings/mtd/fsl-nfc.txt
new file mode 100644
index 0000000..1d339b1
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/fsl-nfc.txt
@@ -0,0 +1,25 @@
+* Freescale's NFC (nand flash controller)
+
+Required properties:
+- compatible : "fsl,XXX-nfc"
+- reg : Address range of the mtd chip
+- interrupts: Should contain the STMMAC interrupts
+- nand-bus-width: see nand.txt
+- nand-ecc-mode: see nand.txt
+- nand-on-flash-bbt: see nand.txt
+- clock-frequency : Optional clock rate to NFC in Hz
+- #address-cells, #size-cells : Must be present if the device has sub-nodes
+  representing partitions.
+
+Example:
+
+	nfc: nand at 400e0000 {
+		compatible = "fsl,vf610-nfc";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		reg = <0x400e0000 0x4000>;
+		interrupts = <0 83 0x04>;
+		clocks = <&clks VF610_CLK_NFC>;
+		clock-names = "nfc";
+		clock-frequency = <33000000>;
+	};
-- 
1.8.0.2

[RFC 4/5] imx:vf610: Add device tree support for the fsl_nfc driver and NAND interface.

[Bill Pringlemeir]

Signed-off-by: Bill Pringlemeir <bpringlemeir@nbsps.com>
---
 arch/arm/boot/dts/vf610-twr.dts | 41 +++++++++++++++++++++++++++++++++++++++++
 arch/arm/boot/dts/vf610.dtsi    |  9 +++++++++
 2 files changed, 50 insertions(+)

diff --git a/arch/arm/boot/dts/vf610-twr.dts b/arch/arm/boot/dts/vf610-twr.dts
index c8047ca..a652516 100644
--- a/arch/arm/boot/dts/vf610-twr.dts
+++ b/arch/arm/boot/dts/vf610-twr.dts
@@ -74,6 +74,47 @@
 	status = "okay";
 };
 
+&iomuxc {
+        vf610-twr {
+		pinctrl_nfc_1: nfcgrp_1 {
+			fsl,pins = <
+			VF610_PAD_PTD31__NF_IO15	0x28df
+			VF610_PAD_PTD30__NF_IO14	0x28df
+			VF610_PAD_PTD29__NF_IO13	0x28df
+			VF610_PAD_PTD28__NF_IO12	0x28df
+			VF610_PAD_PTD27__NF_IO11	0x28df
+			VF610_PAD_PTD26__NF_IO10	0x28df
+			VF610_PAD_PTD25__NF_IO9		0x28df
+			VF610_PAD_PTD24__NF_IO8		0x28df
+			VF610_PAD_PTD23__NF_IO7		0x28df
+			VF610_PAD_PTD22__NF_IO6		0x28df
+			VF610_PAD_PTD21__NF_IO5		0x28df
+			VF610_PAD_PTD20__NF_IO4		0x28df
+			VF610_PAD_PTD19__NF_IO3		0x28df
+			VF610_PAD_PTD18__NF_IO2		0x28df
+			VF610_PAD_PTD17__NF_IO1		0x28df
+			VF610_PAD_PTD16__NF_IO0		0x28df
+			VF610_PAD_PTB24__NF_WE_B	0x28c2
+			VF610_PAD_PTB25__NF_CE0_B	0x28c2
+			VF610_PAD_PTB27__NF_RE_B	0x28c2
+			VF610_PAD_PTC26__NF_RB_B	0x283d
+			VF610_PAD_PTC27__NF_ALE		0x28c2
+			VF610_PAD_PTC28__NF_CLE		0x28c2
+			>;
+		};
+	};
+};
+
+&nfc {
+	nand-bus-width = <16>;
+	nand-ecc-mode = "hw";
+	nand-on-flash-bbt;
+	clock-frequency = <33000000>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_nfc_1>;
+	status = "okay";
+};
+
 &uart1 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_uart1_1>;
diff --git a/arch/arm/boot/dts/vf610.dtsi b/arch/arm/boot/dts/vf610.dtsi
index d31ce1b..04bc727 100644
--- a/arch/arm/boot/dts/vf610.dtsi
+++ b/arch/arm/boot/dts/vf610.dtsi
@@ -471,6 +471,15 @@
 				clock-names = "ipg", "ahb", "ptp";
 				status = "disabled";
 			};
+
+			nfc: nand at 400e0000 {
+				compatible = "fsl,vf610-nfc";
+				reg = <0x400e0000 0x4000>;
+				interrupts = <0 83 0x04>;
+				clocks = <&clks VF610_CLK_NFC>;
+				clock-names = "nfc";
+				status = "disabled";
+			};
 		};
 	};
 };
-- 
1.8.0.2

[RFC 5/5] imx:vf610: Allow user to enable NAND controller for the VF610 SOC.

[Bill Pringlemeir]

Signed-off-by: Bill Pringlemeir <bpringlemeir@nbsps.com>
---
 arch/arm/mach-imx/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-imx/Kconfig b/arch/arm/mach-imx/Kconfig
index b0c6eb3..02902ca 100644
--- a/arch/arm/mach-imx/Kconfig
+++ b/arch/arm/mach-imx/Kconfig
@@ -842,6 +842,7 @@ config SOC_VF610
 	select PL310_ERRATA_588369 if CACHE_PL310
 	select PL310_ERRATA_727915 if CACHE_PL310
 	select PL310_ERRATA_769419 if CACHE_PL310
+	select HAVE_NAND_FSL_NFC
 
 	help
 	  This enable support for Freescale Vybrid VF610 processor.
-- 
1.8.0.2