The 10th revision includes some more review comments, including an
improved handling of empty NAND pages with hardware ECC.
More information and the full test log of earlier patchset version
can be found in the cover letter of the last revision v6:
http://thread.gmane.org/gmane.linux.kernel/1979868
Changes since v9:
- Remove inline of vf610_nfc_done
- Add __iomem to src argument of vf610_nfc_memcpy
- Handle return value of mtd_device_parse_register correctly
- Count bits in OOB too (only non-ECC bits)
- Return bitflips in ecc.read_page callback vf610_nfc_read_page
- Fall-through ALT_BUF_ONFI
- Use BIT macros
Changes since v8:
- Fix 16-Bit NAND flash support by splitting up initialization
(introduce vf610_nfc_preinit_controller)
- Updated comments in initialziation functions
Changes since v7:
- vf610-twr.dts: Moved NFC pinmux into the existing iomuxc node
and sort new nfc node behind the existing iomuxc node as well.
- vf610-twr.dts/vf-colibri.dtsi: Dropped _1 suffixes
Changes since v6:
- Rebased ontop of l2-mtd/master (v4.2-rc1 based)
- Removed HAVE_NAND_VF610_NFC and use depends on. This made
"[PATCH v6 4/6] ARM: vf610: enable NAND Flash Controller" unnecessary
Changes since v5:
- Removed fsl,mpc5125-nfc compatible string
- Removed readl/writel_relaxed
- Change interface of vf610_nfc_transfer_size to match other accessors
Changes since v4:
- Rebased ontop of l2-mtd/master (v4.1-rc4 based)
- Eliminate unnecessary page read (NAND_CMD_SEQIN) since the driver does
not support sub-page writes anyway (improves write performance)
- Support ONFI by enabling READID command with offset and parameter page
reads (CMD_PARAM)
- Change to dedicated read_page/write_page function, enables raw writes
- Use __LITTLE_ENDIAN to distingush between LE/BE relevant statements
- Eliminated vf610_nfc_probe_dt in favor of common DT init code
- Use wait_for_completion_timeout
- Some style fixes (spaces, etc.)
Changes since v3:
- Make the driver selectable when COMPILE_TEST is set
- Fix compile error due to superfluous ECC_STATUS configuration in initial
patch (without ECC correction ECC_STATUS does not need to be configured)
- Remove custom BBT pattern and switch to in-band BBT in the initial patch
- Include two bug fixes, for details see the corresponding U-Boot patches:
http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/215802
Changes since v2:
- Updated binding documentation
Changes since v1:
- Nest nfc_config struct within the main nfc struct
- Use assigned clock binding to specify NFC clock
- Rebased ontop of MSCM IR patchset (driver parts have been merged)
- Split out arch Kconfig in a separate config
- Fix module license
- Updated MAINTAINERS
Changes since RFC (Bill Pringlemeir):
- Renamed driver from fsl_nfc to vf610_nfc
- Use readl/writel for all register in accessor functions
- Optimized field accessor functions
- Implemented PM (suspend/resume) functions
- Implemented basic support for ECC strength/ECC step size from dt
- Improved performance of count_written_bits by using hweight32
- Support ECC with 60-bytes to correct up to 32 bit errors
- Changed to in-band BBT (NAND_BBT_NO_OOB) which also allows ECC modes
which uses up to 60 bytes on 64 byte OOB
- Removed custom (downstream) BBT pattern since BBT table won't be
compatible anyway (due to the change above)
*** SUBJECT HERE ***
*** BLURB HERE ***
Stefan Agner (5):
mtd: nand: vf610_nfc: Freescale NFC for VF610, MPC5125 and others
mtd: nand: vf610_nfc: add hardware BCH-ECC support
mtd: nand: vf610_nfc: add device tree bindings
ARM: dts: vf610twr: add NAND flash controller peripherial
ARM: dts: vf-colibri: enable NAND flash controller
.../devicetree/bindings/mtd/vf610-nfc.txt | 45 ++
MAINTAINERS | 6 +
arch/arm/boot/dts/vf-colibri.dtsi | 32 +
arch/arm/boot/dts/vf610-twr.dts | 40 +
arch/arm/boot/dts/vfxxx.dtsi | 8 +
drivers/mtd/nand/Kconfig | 11 +
drivers/mtd/nand/Makefile | 1 +
drivers/mtd/nand/vf610_nfc.c | 849 +++++++++++++++++++++
8 files changed, 992 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mtd/vf610-nfc.txt
create mode 100644 drivers/mtd/nand/vf610_nfc.c
--
2.5.0
This driver supports Freescale NFC (NAND flash controller) found on
Vybrid (VF610), MPC5125, MCF54418 and Kinetis K70. The driver has
been tested on 8-bit and 16-bit NAND interface and supports ONFI
parameter page reading.
Limitations:
- DMA and pipelining not used
- Pages larger than 2k are not supported
- No hardware ECC
The driver has only been tested on Vybrid SoC VF610 and VF500.
Some paths have been hand-optimized and evaluated by measurements
made using mtd_speedtest.ko on a 100MB MTD partition.
Colibri VF50
eb write % eb read % page write % page read %
rel/opt 5175 11537 4560 11039
opt 5164 -0.21 11420 -1.01 4737 +3.88 10918 -1.10
none 5113 -1.20 11352 -1.60 4490 -1.54 10865 -1.58
Colibri VF61
eb write % eb read % page write % page read %
rel/opt 5766 13096 5459 12846
opt 5883 +2.03 13064 -0.24 5561 +1.87 12802 -0.34
none 5701 -1.13 12980 -0.89 5488 +0.53 12735 -0.86
rel = using readl_relaxed/writel_relaxed in optimized paths
opt = hand-optimized by combining multiple accesses into one read/write
The measurements have not been statistically verfied, hence use them
with care. The author came to the conclusion that using the relaxed
variants of readl/writel are not worth the additional code.
Signed-off-by: Bill Pringlemeir <[email protected]>
Tested-by: Albert ARIBAUD <[email protected]>
Signed-off-by: Stefan Agner <[email protected]>
---
MAINTAINERS | 6 +
drivers/mtd/nand/Kconfig | 9 +
drivers/mtd/nand/Makefile | 1 +
drivers/mtd/nand/vf610_nfc.c | 645 +++++++++++++++++++++++++++++++++++++++++++
4 files changed, 661 insertions(+)
create mode 100644 drivers/mtd/nand/vf610_nfc.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 9567329..59975c7 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10835,6 +10835,12 @@ S: Maintained
F: Documentation/fb/uvesafb.txt
F: drivers/video/fbdev/uvesafb.*
+VF610 NAND DRIVER
+M: Stefan Agner <[email protected]>
+L: [email protected]
+S: Supported
+F: drivers/mtd/nand/vf610_nfc.c
+
VFAT/FAT/MSDOS FILESYSTEM
M: OGAWA Hirofumi <[email protected]>
S: Maintained
diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 5b2806a..8550b14 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -463,6 +463,15 @@ config MTD_NAND_MPC5121_NFC
This enables the driver for the NAND flash controller on the
MPC5121 SoC.
+config MTD_NAND_VF610_NFC
+ tristate "Support for Freescale NFC for VF610/MPC5125"
+ depends on (SOC_VF610 || COMPILE_TEST)
+ help
+ Enables support for NAND Flash Controller on some Freescale
+ processors like the VF610, MPC5125, MCF54418 or 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 1f897ec..a490af8 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -45,6 +45,7 @@ obj-$(CONFIG_MTD_NAND_SOCRATES) += socrates_nand.o
obj-$(CONFIG_MTD_NAND_TXX9NDFMC) += txx9ndfmc.o
obj-$(CONFIG_MTD_NAND_NUC900) += nuc900_nand.o
obj-$(CONFIG_MTD_NAND_MPC5121_NFC) += mpc5121_nfc.o
+obj-$(CONFIG_MTD_NAND_VF610_NFC) += vf610_nfc.o
obj-$(CONFIG_MTD_NAND_RICOH) += r852.o
obj-$(CONFIG_MTD_NAND_JZ4740) += jz4740_nand.o
obj-$(CONFIG_MTD_NAND_GPMI_NAND) += gpmi-nand/
diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
new file mode 100644
index 0000000..5c8dfe8
--- /dev/null
+++ b/drivers/mtd/nand/vf610_nfc.c
@@ -0,0 +1,645 @@
+/*
+ * Copyright 2009-2015 Freescale Semiconductor, Inc. and others
+ *
+ * Description: MPC5125, VF610, MCF54418 and Kinetis K70 Nand driver.
+ * Jason ported to M54418TWR and MVFA5 (VF610).
+ * Authors: Stefan Agner <[email protected]>
+ * Bill Pringlemeir <[email protected]>
+ * Shaohui Xie <[email protected]>
+ * Jason Jin <[email protected]>
+ *
+ * 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/bitops.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 "vf610_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 READ_ONFI_PARAM_CMD_CODE 0x4860
+#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 BIT(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_ADDR_MASK 0x7FC00000
+#define CONFIG_ECC_SRAM_ADDR_SHIFT 22
+#define CONFIG_ECC_SRAM_REQ_BIT BIT(21)
+#define CONFIG_DMA_REQ_BIT BIT(20)
+#define CONFIG_ECC_MODE_MASK 0x000E0000
+#define CONFIG_ECC_MODE_SHIFT 17
+#define CONFIG_FAST_FLASH_BIT BIT(16)
+#define CONFIG_16BIT BIT(7)
+#define CONFIG_BOOT_MODE_BIT BIT(6)
+#define CONFIG_ADDR_AUTO_INCR_BIT BIT(5)
+#define CONFIG_BUFNO_AUTO_INCR_BIT BIT(4)
+#define CONFIG_PAGE_CNT_MASK 0xF
+#define CONFIG_PAGE_CNT_SHIFT 0
+
+/* NFC_IRQ_STATUS Field */
+#define IDLE_IRQ_BIT BIT(29)
+#define IDLE_EN_BIT BIT(20)
+#define CMD_DONE_CLEAR_BIT BIT(18)
+#define IDLE_CLEAR_BIT BIT(17)
+
+struct vf610_nfc {
+ struct mtd_info mtd;
+ struct nand_chip chip;
+ struct device *dev;
+ void __iomem *regs;
+ struct completion cmd_done;
+ uint buf_offset;
+ int page_sz;
+ /* Status and ID are in alternate locations. */
+ int alt_buf;
+#define ALT_BUF_ID 1
+#define ALT_BUF_STAT 2
+#define ALT_BUF_ONFI 3
+ struct clk *clk;
+};
+
+#define mtd_to_nfc(_mtd) container_of(_mtd, struct vf610_nfc, mtd)
+
+static inline u32 vf610_nfc_read(struct vf610_nfc *nfc, uint reg)
+{
+ return readl(nfc->regs + reg);
+}
+
+static inline void vf610_nfc_write(struct vf610_nfc *nfc, uint reg, u32 val)
+{
+ writel(val, nfc->regs + reg);
+}
+
+static inline void vf610_nfc_set(struct vf610_nfc *nfc, uint reg, u32 bits)
+{
+ vf610_nfc_write(nfc, reg, vf610_nfc_read(nfc, reg) | bits);
+}
+
+static inline void vf610_nfc_clear(struct vf610_nfc *nfc, uint reg, u32 bits)
+{
+ vf610_nfc_write(nfc, reg, vf610_nfc_read(nfc, reg) & ~bits);
+}
+
+static inline void vf610_nfc_set_field(struct vf610_nfc *nfc, u32 reg,
+ u32 mask, u32 shift, u32 val)
+{
+ vf610_nfc_write(nfc, reg,
+ (vf610_nfc_read(nfc, reg) & (~mask)) | val << shift);
+}
+
+static inline void vf610_nfc_memcpy(void *dst, const void __iomem *src,
+ size_t n)
+{
+ /*
+ * Use this accessor for the internal SRAM buffers. On the ARM
+ * Freescale Vybrid SoC it's known that the driver can treat
+ * the SRAM buffer as if it's memory. Other platform might need
+ * to treat the buffers differently.
+ *
+ * For the time being, use memcpy
+ */
+ memcpy(dst, src, n);
+}
+
+/* Clear flags for upcoming command */
+static inline void vf610_nfc_clear_status(struct vf610_nfc *nfc)
+{
+ u32 tmp = vf610_nfc_read(nfc, NFC_IRQ_STATUS);
+
+ tmp |= CMD_DONE_CLEAR_BIT | IDLE_CLEAR_BIT;
+ vf610_nfc_write(nfc, NFC_IRQ_STATUS, tmp);
+}
+
+static void vf610_nfc_done(struct vf610_nfc *nfc)
+{
+ unsigned long timeout = msecs_to_jiffies(100);
+
+ /*
+ * Barrier is needed after this write. This write need
+ * to be done before reading the next register the first
+ * time.
+ * vf610_nfc_set implicates such a barrier by using writel
+ * to write to the register.
+ */
+ vf610_nfc_set(nfc, NFC_IRQ_STATUS, IDLE_EN_BIT);
+ vf610_nfc_set(nfc, NFC_FLASH_CMD2, START_BIT);
+
+ if (!(vf610_nfc_read(nfc, NFC_IRQ_STATUS) & IDLE_IRQ_BIT)) {
+ if (!wait_for_completion_timeout(&nfc->cmd_done, timeout))
+ dev_warn(nfc->dev, "Timeout while waiting for BUSY.\n");
+ }
+ vf610_nfc_clear_status(nfc);
+}
+
+static u8 vf610_nfc_get_id(struct vf610_nfc *nfc, int col)
+{
+ u32 flash_id;
+
+ if (col < 4) {
+ flash_id = vf610_nfc_read(nfc, NFC_FLASH_STATUS1);
+ flash_id >>= (3 - col) * 8;
+ } else {
+ flash_id = vf610_nfc_read(nfc, NFC_FLASH_STATUS2);
+ flash_id >>= 24;
+ }
+
+ return flash_id & 0xff;
+}
+
+static u8 vf610_nfc_get_status(struct vf610_nfc *nfc)
+{
+ return vf610_nfc_read(nfc, NFC_FLASH_STATUS2) & STATUS_BYTE1_MASK;
+}
+
+static void vf610_nfc_send_command(struct vf610_nfc *nfc, u32 cmd_byte1,
+ u32 cmd_code)
+{
+ u32 tmp;
+
+ vf610_nfc_clear_status(nfc);
+
+ tmp = vf610_nfc_read(nfc, NFC_FLASH_CMD2);
+ tmp &= ~(CMD_BYTE1_MASK | CMD_CODE_MASK | BUFNO_MASK);
+ tmp |= cmd_byte1 << CMD_BYTE1_SHIFT;
+ tmp |= cmd_code << CMD_CODE_SHIFT;
+ vf610_nfc_write(nfc, NFC_FLASH_CMD2, tmp);
+}
+
+static void vf610_nfc_send_commands(struct vf610_nfc *nfc, u32 cmd_byte1,
+ u32 cmd_byte2, u32 cmd_code)
+{
+ u32 tmp;
+
+ vf610_nfc_send_command(nfc, cmd_byte1, cmd_code);
+
+ tmp = vf610_nfc_read(nfc, NFC_FLASH_CMD1);
+ tmp &= ~CMD_BYTE2_MASK;
+ tmp |= cmd_byte2 << CMD_BYTE2_SHIFT;
+ vf610_nfc_write(nfc, NFC_FLASH_CMD1, tmp);
+}
+
+static irqreturn_t vf610_nfc_irq(int irq, void *data)
+{
+ struct mtd_info *mtd = data;
+ struct vf610_nfc *nfc = mtd_to_nfc(mtd);
+
+ vf610_nfc_clear(nfc, NFC_IRQ_STATUS, IDLE_EN_BIT);
+ complete(&nfc->cmd_done);
+
+ return IRQ_HANDLED;
+}
+
+static void vf610_nfc_addr_cycle(struct vf610_nfc *nfc, int column, int page)
+{
+ if (column != -1) {
+ if (nfc->chip.options & NAND_BUSWIDTH_16)
+ column = column / 2;
+ vf610_nfc_set_field(nfc, NFC_COL_ADDR, COL_ADDR_MASK,
+ COL_ADDR_SHIFT, column);
+ }
+ if (page != -1)
+ vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK,
+ ROW_ADDR_SHIFT, page);
+}
+
+static inline void vf610_nfc_transfer_size(struct vf610_nfc *nfc, int size)
+{
+ vf610_nfc_write(nfc, NFC_SECTOR_SIZE, size);
+}
+
+static void vf610_nfc_command(struct mtd_info *mtd, unsigned command,
+ int column, int page)
+{
+ struct vf610_nfc *nfc = mtd_to_nfc(mtd);
+ int page_sz = nfc->chip.options & NAND_BUSWIDTH_16 ? 1 : 0;
+
+ nfc->buf_offset = max(column, 0);
+ nfc->alt_buf = 0;
+
+ switch (command) {
+ case NAND_CMD_SEQIN:
+ /* Use valid column/page from preread... */
+ vf610_nfc_addr_cycle(nfc, column, page);
+ /*
+ * SEQIN => data => PAGEPROG sequence is done by the controller
+ * hence we do not need to issue the command here...
+ */
+ return;
+ case NAND_CMD_PAGEPROG:
+ page_sz += mtd->writesize + mtd->oobsize;
+ vf610_nfc_transfer_size(nfc, page_sz);
+ vf610_nfc_send_commands(nfc, NAND_CMD_SEQIN,
+ command, PROGRAM_PAGE_CMD_CODE);
+ break;
+
+ case NAND_CMD_RESET:
+ vf610_nfc_transfer_size(nfc, 0);
+ vf610_nfc_send_command(nfc, command, RESET_CMD_CODE);
+ break;
+
+ case NAND_CMD_READOOB:
+ page_sz += mtd->oobsize;
+ column = mtd->writesize;
+ vf610_nfc_transfer_size(nfc, page_sz);
+ vf610_nfc_send_commands(nfc, NAND_CMD_READ0,
+ NAND_CMD_READSTART, READ_PAGE_CMD_CODE);
+ vf610_nfc_addr_cycle(nfc, column, page);
+ break;
+
+ case NAND_CMD_READ0:
+ page_sz += mtd->writesize + mtd->oobsize;
+ vf610_nfc_transfer_size(nfc, page_sz);
+ vf610_nfc_send_commands(nfc, NAND_CMD_READ0,
+ NAND_CMD_READSTART, READ_PAGE_CMD_CODE);
+ vf610_nfc_addr_cycle(nfc, column, page);
+ break;
+
+ case NAND_CMD_PARAM:
+ nfc->alt_buf = ALT_BUF_ONFI;
+ vf610_nfc_transfer_size(nfc, 768);
+ vf610_nfc_send_command(nfc, command, READ_ONFI_PARAM_CMD_CODE);
+ vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK,
+ ROW_ADDR_SHIFT, column);
+ break;
+
+ case NAND_CMD_ERASE1:
+ vf610_nfc_transfer_size(nfc, 0);
+ vf610_nfc_send_commands(nfc, command,
+ NAND_CMD_ERASE2, ERASE_CMD_CODE);
+ vf610_nfc_addr_cycle(nfc, column, page);
+ break;
+
+ case NAND_CMD_READID:
+ nfc->alt_buf = ALT_BUF_ID;
+ nfc->buf_offset = 0;
+ vf610_nfc_transfer_size(nfc, 0);
+ vf610_nfc_send_command(nfc, command, READ_ID_CMD_CODE);
+ vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK,
+ ROW_ADDR_SHIFT, column);
+ break;
+
+ case NAND_CMD_STATUS:
+ nfc->alt_buf = ALT_BUF_STAT;
+ vf610_nfc_transfer_size(nfc, 0);
+ vf610_nfc_send_command(nfc, command, STATUS_READ_CMD_CODE);
+ break;
+ default:
+ return;
+ }
+
+ vf610_nfc_done(nfc);
+}
+
+static void vf610_nfc_read_buf(struct mtd_info *mtd, u_char *buf, int len)
+{
+ struct vf610_nfc *nfc = mtd_to_nfc(mtd);
+ uint c = nfc->buf_offset;
+
+ /* Alternate buffers are only supported through read_byte */
+ WARN_ON(nfc->alt_buf);
+
+ vf610_nfc_memcpy(buf, nfc->regs + NFC_MAIN_AREA(0) + c, len);
+
+ nfc->buf_offset += len;
+}
+
+static void vf610_nfc_write_buf(struct mtd_info *mtd, const uint8_t *buf,
+ int len)
+{
+ struct vf610_nfc *nfc = mtd_to_nfc(mtd);
+ uint c = nfc->buf_offset;
+ uint l;
+
+ l = min_t(uint, len, mtd->writesize + mtd->oobsize - c);
+ vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + c, buf, l);
+
+ nfc->buf_offset += l;
+}
+
+static uint8_t vf610_nfc_read_byte(struct mtd_info *mtd)
+{
+ struct vf610_nfc *nfc = mtd_to_nfc(mtd);
+ u8 tmp;
+ uint c = nfc->buf_offset;
+
+ switch (nfc->alt_buf) {
+ case ALT_BUF_ID:
+ tmp = vf610_nfc_get_id(nfc, c);
+ break;
+ case ALT_BUF_STAT:
+ tmp = vf610_nfc_get_status(nfc);
+ break;
+#ifdef __LITTLE_ENDIAN
+ case ALT_BUF_ONFI:
+ /* Reverse byte since the controller uses big endianness */
+ c = nfc->buf_offset ^ 0x3;
+ /* fall-through */
+#endif
+ default:
+ tmp = *((u8 *)(nfc->regs + NFC_MAIN_AREA(0) + c));
+ break;
+ }
+ nfc->buf_offset++;
+ return tmp;
+}
+
+static u16 vf610_nfc_read_word(struct mtd_info *mtd)
+{
+ u16 tmp;
+
+ vf610_nfc_read_buf(mtd, (u_char *)&tmp, sizeof(tmp));
+ return tmp;
+}
+
+/* If not provided, upper layers apply a fixed delay. */
+static int vf610_nfc_dev_ready(struct mtd_info *mtd)
+{
+ /* NFC handles R/B internally; always ready. */
+ return 1;
+}
+
+/*
+ * This function supports Vybrid only (MPC5125 would have full RB and four CS)
+ */
+static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip)
+{
+#ifdef CONFIG_SOC_VF610
+ struct vf610_nfc *nfc = mtd_to_nfc(mtd);
+ u32 tmp = vf610_nfc_read(nfc, NFC_ROW_ADDR);
+
+ tmp &= ~(ROW_ADDR_CHIP_SEL_RB_MASK | ROW_ADDR_CHIP_SEL_MASK);
+ tmp |= 1 << ROW_ADDR_CHIP_SEL_RB_SHIFT;
+
+ if (chip == 0)
+ tmp |= 1 << ROW_ADDR_CHIP_SEL_SHIFT;
+ else if (chip == 1)
+ tmp |= 2 << ROW_ADDR_CHIP_SEL_SHIFT;
+
+ vf610_nfc_write(nfc, NFC_ROW_ADDR, tmp);
+#endif
+}
+
+static const struct of_device_id vf610_nfc_dt_ids[] = {
+ { .compatible = "fsl,vf610-nfc" },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, vf610_nfc_dt_ids);
+
+static void vf610_nfc_preinit_controller(struct vf610_nfc *nfc)
+{
+ vf610_nfc_clear(nfc, NFC_FLASH_CONFIG, CONFIG_16BIT);
+ vf610_nfc_clear(nfc, NFC_FLASH_CONFIG, CONFIG_ADDR_AUTO_INCR_BIT);
+ vf610_nfc_clear(nfc, NFC_FLASH_CONFIG, CONFIG_BUFNO_AUTO_INCR_BIT);
+ vf610_nfc_clear(nfc, NFC_FLASH_CONFIG, CONFIG_BOOT_MODE_BIT);
+ vf610_nfc_clear(nfc, NFC_FLASH_CONFIG, CONFIG_DMA_REQ_BIT);
+ vf610_nfc_set(nfc, NFC_FLASH_CONFIG, CONFIG_FAST_FLASH_BIT);
+
+ /* Disable virtual pages, only one elementary transfer unit */
+ vf610_nfc_set_field(nfc, NFC_FLASH_CONFIG, CONFIG_PAGE_CNT_MASK,
+ CONFIG_PAGE_CNT_SHIFT, 1);
+}
+
+static void vf610_nfc_init_controller(struct vf610_nfc *nfc)
+{
+ if (nfc->chip.options & NAND_BUSWIDTH_16)
+ vf610_nfc_set(nfc, NFC_FLASH_CONFIG, CONFIG_16BIT);
+ else
+ vf610_nfc_clear(nfc, NFC_FLASH_CONFIG, CONFIG_16BIT);
+}
+
+static int vf610_nfc_probe(struct platform_device *pdev)
+{
+ struct vf610_nfc *nfc;
+ struct resource *res;
+ struct mtd_info *mtd;
+ struct nand_chip *chip;
+ int err = 0;
+ int irq;
+
+ nfc = devm_kzalloc(&pdev->dev, sizeof(*nfc), GFP_KERNEL);
+ if (!nfc)
+ return -ENOMEM;
+
+ nfc->dev = &pdev->dev;
+ mtd = &nfc->mtd;
+ chip = &nfc->chip;
+
+ mtd->priv = chip;
+ mtd->owner = THIS_MODULE;
+ mtd->dev.parent = nfc->dev;
+ mtd->name = DRV_NAME;
+
+ 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);
+
+ err = clk_prepare_enable(nfc->clk);
+ if (err) {
+ dev_err(nfc->dev, "Unable to enable clock!\n");
+ return err;
+ }
+
+ chip->dn = nfc->dev->of_node;
+ chip->dev_ready = vf610_nfc_dev_ready;
+ chip->cmdfunc = vf610_nfc_command;
+ chip->read_byte = vf610_nfc_read_byte;
+ chip->read_word = vf610_nfc_read_word;
+ chip->read_buf = vf610_nfc_read_buf;
+ chip->write_buf = vf610_nfc_write_buf;
+ chip->select_chip = vf610_nfc_select_chip;
+
+ chip->options |= NAND_NO_SUBPAGE_WRITE;
+
+ init_completion(&nfc->cmd_done);
+
+ err = devm_request_irq(nfc->dev, irq, vf610_nfc_irq, 0, DRV_NAME, mtd);
+ if (err) {
+ dev_err(nfc->dev, "Error requesting IRQ!\n");
+ goto error;
+ }
+
+ vf610_nfc_preinit_controller(nfc);
+
+ /* first scan to find the device and get the page size */
+ if (nand_scan_ident(mtd, 1, NULL)) {
+ err = -ENXIO;
+ goto error;
+ }
+
+ vf610_nfc_init_controller(nfc);
+
+ /* Bad block options. */
+ if (chip->bbt_options & NAND_BBT_USE_FLASH)
+ chip->bbt_options |= NAND_BBT_NO_OOB;
+
+ /* Single buffer only, max 256 OOB minus ECC status */
+ if (mtd->writesize + mtd->oobsize > PAGE_2K + 256 - 8) {
+ dev_err(nfc->dev, "Unsupported flash page size\n");
+ err = -ENXIO;
+ goto error;
+ }
+
+ /* second phase scan */
+ if (nand_scan_tail(mtd)) {
+ err = -ENXIO;
+ goto error;
+ }
+
+ platform_set_drvdata(pdev, mtd);
+
+ /* Register device in MTD */
+ return mtd_device_parse_register(mtd, NULL,
+ &(struct mtd_part_parser_data){
+ .of_node = pdev->dev.of_node,
+ },
+ NULL, 0);
+
+error:
+ clk_disable_unprepare(nfc->clk);
+ return err;
+}
+
+static int vf610_nfc_remove(struct platform_device *pdev)
+{
+ struct mtd_info *mtd = platform_get_drvdata(pdev);
+ struct vf610_nfc *nfc = mtd_to_nfc(mtd);
+
+ nand_release(mtd);
+ clk_disable_unprepare(nfc->clk);
+ return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int vf610_nfc_suspend(struct device *dev)
+{
+ struct mtd_info *mtd = dev_get_drvdata(dev);
+ struct vf610_nfc *nfc = mtd_to_nfc(mtd);
+
+ clk_disable_unprepare(nfc->clk);
+ return 0;
+}
+
+static int vf610_nfc_resume(struct device *dev)
+{
+ struct mtd_info *mtd = dev_get_drvdata(dev);
+ struct vf610_nfc *nfc = mtd_to_nfc(mtd);
+
+ pinctrl_pm_select_default_state(dev);
+
+ clk_prepare_enable(nfc->clk);
+
+ vf610_nfc_preinit_controller(nfc);
+ vf610_nfc_init_controller(nfc);
+ return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(vf610_nfc_pm_ops, vf610_nfc_suspend, vf610_nfc_resume);
+
+static struct platform_driver vf610_nfc_driver = {
+ .driver = {
+ .name = DRV_NAME,
+ .of_match_table = vf610_nfc_dt_ids,
+ .pm = &vf610_nfc_pm_ops,
+ },
+ .probe = vf610_nfc_probe,
+ .remove = vf610_nfc_remove,
+};
+
+module_platform_driver(vf610_nfc_driver);
+
+MODULE_AUTHOR("Stefan Agner <[email protected]>");
+MODULE_DESCRIPTION("Freescale VF610/MPC5125 NFC MTD NAND driver");
+MODULE_LICENSE("GPL");
--
2.5.0
This adds hardware ECC support using the BCH encoder in the NFC IP.
The ECC encoder supports up to 32-bit correction by using 60 error
correction bytes. There is no sub-page ECC step, ECC is calculated
always accross the whole page (up to 2k pages). Raw writes writes
are possible through the common nand_write_page_raw implementation,
however raw reads are not possible since the hardware ECC mode need
to be enabled at command time.
Signed-off-by: Bill Pringlemeir <[email protected]>
Signed-off-by: Stefan Agner <[email protected]>
---
drivers/mtd/nand/Kconfig | 6 +-
drivers/mtd/nand/vf610_nfc.c | 206 ++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 209 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 8550b14..e05f53c 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -469,8 +469,10 @@ config MTD_NAND_VF610_NFC
help
Enables support for NAND Flash Controller on some Freescale
processors like the VF610, MPC5125, MCF54418 or 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 up to 32-bit error
+ correction is supported. Hardware ECC is only enabled through
+ device tree.
config MTD_NAND_MXC
tristate "MXC NAND support"
diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
index 5c8dfe8..247f06a 100644
--- a/drivers/mtd/nand/vf610_nfc.c
+++ b/drivers/mtd/nand/vf610_nfc.c
@@ -19,6 +19,8 @@
* - Untested on MPC5125 and M54418.
* - DMA not used.
* - 2K pages or less.
+ * - Only 2K page w. 64+ OOB and hardware ECC.
+ * - Raw page reads not implemented when using ECC.
*/
#include <linux/module.h>
@@ -73,6 +75,8 @@
/* NFC ECC mode define */
#define ECC_BYPASS 0
+#define ECC_45_BYTE 6
+#define ECC_60_BYTE 7
/*** Register Mask and bit definitions */
@@ -125,6 +129,21 @@
#define CMD_DONE_CLEAR_BIT BIT(18)
#define IDLE_CLEAR_BIT BIT(17)
+/* 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
+
+/*
+ * ECC status is stored at NFC_CFG[ECCADD] +4 for little-endian
+ * and +7 for big-endian SoCs.
+ */
+#ifdef __LITTLE_ENDIAN
+#define ECC_OFFSET 4
+#else
+#define ECC_OFFSET 7
+#endif
+
struct vf610_nfc {
struct mtd_info mtd;
struct nand_chip chip;
@@ -139,10 +158,40 @@ struct vf610_nfc {
#define ALT_BUF_STAT 2
#define ALT_BUF_ONFI 3
struct clk *clk;
+ bool use_hw_ecc;
+ u32 ecc_mode;
};
#define mtd_to_nfc(_mtd) container_of(_mtd, struct vf610_nfc, mtd)
+static struct nand_ecclayout vf610_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 = 2,
+ .length = 17} }
+};
+
+static struct nand_ecclayout vf610_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 inline u32 vf610_nfc_read(struct vf610_nfc *nfc, uint reg)
{
return readl(nfc->regs + reg);
@@ -285,6 +334,13 @@ static void vf610_nfc_addr_cycle(struct vf610_nfc *nfc, int column, int page)
ROW_ADDR_SHIFT, page);
}
+static inline void vf610_nfc_ecc_mode(struct vf610_nfc *nfc, int ecc_mode)
+{
+ vf610_nfc_set_field(nfc, NFC_FLASH_CONFIG,
+ CONFIG_ECC_MODE_MASK,
+ CONFIG_ECC_MODE_SHIFT, ecc_mode);
+}
+
static inline void vf610_nfc_transfer_size(struct vf610_nfc *nfc, int size)
{
vf610_nfc_write(nfc, NFC_SECTOR_SIZE, size);
@@ -303,13 +359,20 @@ static void vf610_nfc_command(struct mtd_info *mtd, unsigned command,
case NAND_CMD_SEQIN:
/* Use valid column/page from preread... */
vf610_nfc_addr_cycle(nfc, column, page);
+ nfc->buf_offset = 0;
+
/*
* SEQIN => data => PAGEPROG sequence is done by the controller
* hence we do not need to issue the command here...
*/
return;
case NAND_CMD_PAGEPROG:
- page_sz += mtd->writesize + mtd->oobsize;
+ page_sz += nfc->page_sz;
+ if (nfc->use_hw_ecc)
+ vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
+ else
+ vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
+
vf610_nfc_transfer_size(nfc, page_sz);
vf610_nfc_send_commands(nfc, NAND_CMD_SEQIN,
command, PROGRAM_PAGE_CMD_CODE);
@@ -327,11 +390,13 @@ static void vf610_nfc_command(struct mtd_info *mtd, unsigned command,
vf610_nfc_send_commands(nfc, NAND_CMD_READ0,
NAND_CMD_READSTART, READ_PAGE_CMD_CODE);
vf610_nfc_addr_cycle(nfc, column, page);
+ vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
break;
case NAND_CMD_READ0:
page_sz += mtd->writesize + mtd->oobsize;
vf610_nfc_transfer_size(nfc, page_sz);
+ vf610_nfc_ecc_mode(nfc, nfc->ecc_mode);
vf610_nfc_send_commands(nfc, NAND_CMD_READ0,
NAND_CMD_READSTART, READ_PAGE_CMD_CODE);
vf610_nfc_addr_cycle(nfc, column, page);
@@ -343,6 +408,7 @@ static void vf610_nfc_command(struct mtd_info *mtd, unsigned command,
vf610_nfc_send_command(nfc, command, READ_ONFI_PARAM_CMD_CODE);
vf610_nfc_set_field(nfc, NFC_ROW_ADDR, ROW_ADDR_MASK,
ROW_ADDR_SHIFT, column);
+ vf610_nfc_ecc_mode(nfc, ECC_BYPASS);
break;
case NAND_CMD_ERASE1:
@@ -371,6 +437,9 @@ static void vf610_nfc_command(struct mtd_info *mtd, unsigned command,
}
vf610_nfc_done(nfc);
+
+ nfc->use_hw_ecc = false;
+ nfc->page_sz = 0;
}
static void vf610_nfc_read_buf(struct mtd_info *mtd, u_char *buf, int len)
@@ -396,6 +465,7 @@ static void vf610_nfc_write_buf(struct mtd_info *mtd, const uint8_t *buf,
l = min_t(uint, len, mtd->writesize + mtd->oobsize - c);
vf610_nfc_memcpy(nfc->regs + NFC_MAIN_AREA(0) + c, buf, l);
+ nfc->page_sz += l;
nfc->buf_offset += l;
}
@@ -462,6 +532,91 @@ static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip)
#endif
}
+/* Count the number of 0's in buff up to max_bits */
+static inline int count_written_bits(uint8_t *buff, int size, int max_bits)
+{
+ uint32_t *buff32 = (uint32_t *)buff;
+ int k, written_bits = 0;
+
+ for (k = 0; k < (size / 4); k++) {
+ written_bits += hweight32(~buff32[k]);
+ if (written_bits > max_bits)
+ break;
+ }
+
+ return written_bits;
+}
+
+static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
+ uint8_t *oob, int oob_loaded)
+{
+ struct vf610_nfc *nfc = mtd_to_nfc(mtd);
+ u8 ecc_status;
+ u8 ecc_count;
+ int flip;
+
+ ecc_status = __raw_readb(nfc->regs + ECC_SRAM_ADDR * 8 + ECC_OFFSET);
+ ecc_count = ecc_status & ECC_ERR_COUNT;
+
+ if (!(ecc_status & ECC_STATUS_MASK))
+ return ecc_count;
+
+ if (!oob_loaded)
+ vf610_nfc_read_buf(mtd, oob, mtd->oobsize);
+
+ /*
+ * On an erased page, bit count (including OOB) should be zero or
+ * at least less then half of the ECC strength.
+ */
+ flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);
+ flip += count_written_bits(oob, mtd->oobsize - nfc->chip.ecc.bytes,
+ ecc_count);
+
+ if (flip > ecc_count && flip > (nfc->chip.ecc.strength / 2))
+ return -1;
+
+ /* Erased page. */
+ memset(dat, 0xff, nfc->chip.ecc.size);
+ return 0;
+}
+
+static int vf610_nfc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
+ uint8_t *buf, int oob_required, int page)
+{
+ int eccsize = chip->ecc.size;
+ int stat;
+
+ vf610_nfc_read_buf(mtd, buf, eccsize);
+ if (oob_required)
+ vf610_nfc_read_buf(mtd, chip->oob_poi, mtd->oobsize);
+
+ stat = vf610_nfc_correct_data(mtd, buf, chip->oob_poi, oob_required);
+
+ if (stat < 0) {
+ mtd->ecc_stats.failed++;
+ return 0;
+ } else {
+ mtd->ecc_stats.corrected += stat;
+ return stat;
+ }
+}
+
+static int vf610_nfc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
+ const uint8_t *buf, int oob_required)
+{
+ struct vf610_nfc *nfc = mtd_to_nfc(mtd);
+
+ vf610_nfc_write_buf(mtd, buf, mtd->writesize);
+ if (oob_required)
+ vf610_nfc_write_buf(mtd, chip->oob_poi, mtd->oobsize);
+
+ /* Always write whole page including OOB due to HW ECC */
+ nfc->use_hw_ecc = true;
+ nfc->page_sz = mtd->writesize + mtd->oobsize;
+
+ return 0;
+}
+
static const struct of_device_id vf610_nfc_dt_ids[] = {
{ .compatible = "fsl,vf610-nfc" },
{ /* sentinel */ }
@@ -488,6 +643,16 @@ static void vf610_nfc_init_controller(struct vf610_nfc *nfc)
vf610_nfc_set(nfc, NFC_FLASH_CONFIG, CONFIG_16BIT);
else
vf610_nfc_clear(nfc, NFC_FLASH_CONFIG, CONFIG_16BIT);
+
+ if (nfc->chip.ecc.mode == NAND_ECC_HW) {
+ /* Set ECC status offset in SRAM */
+ vf610_nfc_set_field(nfc, NFC_FLASH_CONFIG,
+ CONFIG_ECC_SRAM_ADDR_MASK,
+ CONFIG_ECC_SRAM_ADDR_SHIFT, ECC_SRAM_ADDR);
+
+ /* Enable ECC status in SRAM */
+ vf610_nfc_set(nfc, NFC_FLASH_CONFIG, CONFIG_ECC_SRAM_REQ_BIT);
+ }
}
static int vf610_nfc_probe(struct platform_device *pdev)
@@ -571,6 +736,45 @@ static int vf610_nfc_probe(struct platform_device *pdev)
goto error;
}
+ if (chip->ecc.mode == NAND_ECC_HW) {
+ if (mtd->writesize != PAGE_2K && mtd->oobsize < 64) {
+ dev_err(nfc->dev, "Unsupported flash with hwecc\n");
+ err = -ENXIO;
+ goto error;
+ }
+
+ if (chip->ecc.size != mtd->writesize) {
+ dev_err(nfc->dev, "Step size needs to be page size\n");
+ err = -ENXIO;
+ goto error;
+ }
+
+ /* Only 64 byte ECC layouts known */
+ if (mtd->oobsize > 64)
+ mtd->oobsize = 64;
+
+ if (chip->ecc.strength == 32) {
+ nfc->ecc_mode = ECC_60_BYTE;
+ chip->ecc.bytes = 60;
+ chip->ecc.layout = &vf610_nfc_ecc60;
+ } else if (chip->ecc.strength == 24) {
+ nfc->ecc_mode = ECC_45_BYTE;
+ chip->ecc.bytes = 45;
+ chip->ecc.layout = &vf610_nfc_ecc45;
+ } else {
+ dev_err(nfc->dev, "Unsupported ECC strength\n");
+ err = -ENXIO;
+ goto error;
+ }
+
+ /* propagate ecc.layout to mtd_info */
+ mtd->ecclayout = chip->ecc.layout;
+ chip->ecc.read_page = vf610_nfc_read_page;
+ chip->ecc.write_page = vf610_nfc_write_page;
+
+ chip->ecc.size = PAGE_2K;
+ }
+
/* second phase scan */
if (nand_scan_tail(mtd)) {
err = -ENXIO;
--
2.5.0
Signed-off-by: Bill Pringlemeir <[email protected]>
Acked-by: Shawn Guo <[email protected]>
Reviewed-by: Brian Norris <[email protected]>
Signed-off-by: Stefan Agner <[email protected]>
---
.../devicetree/bindings/mtd/vf610-nfc.txt | 45 ++++++++++++++++++++++
1 file changed, 45 insertions(+)
create mode 100644 Documentation/devicetree/bindings/mtd/vf610-nfc.txt
diff --git a/Documentation/devicetree/bindings/mtd/vf610-nfc.txt b/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
new file mode 100644
index 0000000..cae5f25
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
@@ -0,0 +1,45 @@
+Freescale's NAND flash controller (NFC)
+
+This variant of the Freescale NAND flash controller (NFC) can be found on
+Vybrid (vf610), MPC5125, MCF54418 and Kinetis K70.
+
+Required properties:
+- compatible: Should be set to "fsl,vf610-nfc"
+- reg: address range of the NFC
+- interrupts: interrupt of the NFC
+- nand-bus-width: see nand.txt
+- nand-ecc-mode: see nand.txt
+- nand-on-flash-bbt: see nand.txt
+- assigned-clocks: main clock from the SoC, for Vybrid <&clks VF610_CLK_NFC>;
+- assigned-clock-rates: The NAND bus timing is derived from this clock
+ rate and should not exceed maximum timing for any NAND memory chip
+ in a board stuffing. Typical NAND memory timings derived from this
+ clock are found in the SoC hardware reference manual. Furthermore,
+ there might be restrictions on maximum rates when using hardware ECC.
+
+- #address-cells, #size-cells : Must be present if the device has sub-nodes
+ representing partitions.
+
+Required properties for hardware ECC:
+- nand-ecc-strength: supported strengths are 24 and 32 bit (see nand.txt)
+- nand-ecc-step-size: step size equals page size, currently only 2k pages are
+ supported
+
+Example:
+
+ nfc: nand@400e0000 {
+ compatible = "fsl,vf610-nfc";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ reg = <0x400e0000 0x4000>;
+ interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clks VF610_CLK_NFC>;
+ clock-names = "nfc";
+ assigned-clocks = <&clks VF610_CLK_NFC>;
+ assigned-clock-rates = <33000000>;
+ nand-bus-width = <8>;
+ nand-ecc-mode = "hw";
+ nand-ecc-strength = <32>;
+ nand-ecc-step-size = <2048>;
+ nand-on-flash-bbt;
+ };
--
2.5.0
This adds the NAND flash controller (NFC) peripherial. The driver
supports the SLC NAND chips found on Freescale's Vybrid Tower System
Module. The Micron NAND chip on the module needs 4-bit ECC per 512
byte page. Use 24-bit ECC per 2k page, which is supported by the
driver.
Signed-off-by: Bill Pringlemeir <[email protected]>
Reviewed-by: Brian Norris <[email protected]>
Signed-off-by: Stefan Agner <[email protected]>
---
arch/arm/boot/dts/vf610-twr.dts | 40 ++++++++++++++++++++++++++++++++++++++++
arch/arm/boot/dts/vfxxx.dtsi | 8 ++++++++
2 files changed, 48 insertions(+)
diff --git a/arch/arm/boot/dts/vf610-twr.dts b/arch/arm/boot/dts/vf610-twr.dts
index 375ab23..2a278a2 100644
--- a/arch/arm/boot/dts/vf610-twr.dts
+++ b/arch/arm/boot/dts/vf610-twr.dts
@@ -237,6 +237,33 @@
>;
};
+ pinctrl_nfc: nfcgrp {
+ 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
+ >;
+ };
+
pinctrl_pwm0: pwm0grp {
fsl,pins = <
VF610_PAD_PTB0__FTM0_CH0 0x1582
@@ -274,6 +301,19 @@
};
};
+&nfc {
+ assigned-clocks = <&clks VF610_CLK_NFC>;
+ assigned-clock-rates = <33000000>;
+ nand-bus-width = <16>;
+ nand-ecc-mode = "hw";
+ nand-ecc-step-size = <2048>;
+ nand-ecc-strength = <24>;
+ nand-on-flash-bbt;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_nfc>;
+ status = "okay";
+};
+
&pwm0 {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_pwm0>;
diff --git a/arch/arm/boot/dts/vfxxx.dtsi b/arch/arm/boot/dts/vfxxx.dtsi
index 4aa3351..2f4b04d 100644
--- a/arch/arm/boot/dts/vfxxx.dtsi
+++ b/arch/arm/boot/dts/vfxxx.dtsi
@@ -520,6 +520,14 @@
status = "disabled";
};
+ nfc: nand@400e0000 {
+ compatible = "fsl,vf610-nfc";
+ reg = <0x400e0000 0x4000>;
+ interrupts = <83 IRQ_TYPE_LEVEL_HIGH>;
+ clocks = <&clks VF610_CLK_NFC>;
+ clock-names = "nfc";
+ status = "disabled";
+ };
};
};
};
--
2.5.0
Enable NAND access by adding pinmux and NAND flash controller node
to device tree. The NAND chips currently used on the Colibri VF61
requires 8-bit ECC per 512 byte page, hence specify 32-bit ECC
strength per 2k page size.
Reviewed-by: Brian Norris <[email protected]>
Signed-off-by: Stefan Agner <[email protected]>
---
arch/arm/boot/dts/vf-colibri.dtsi | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/arch/arm/boot/dts/vf-colibri.dtsi b/arch/arm/boot/dts/vf-colibri.dtsi
index 68ca125..ab2e74b 100644
--- a/arch/arm/boot/dts/vf-colibri.dtsi
+++ b/arch/arm/boot/dts/vf-colibri.dtsi
@@ -52,6 +52,19 @@
pinctrl-0 = <&pinctrl_i2c0>;
};
+&nfc {
+ assigned-clocks = <&clks VF610_CLK_NFC>;
+ assigned-clock-rates = <33000000>;
+ nand-bus-width = <8>;
+ nand-ecc-mode = "hw";
+ nand-ecc-step-size = <2048>;
+ nand-ecc-strength = <32>;
+ nand-on-flash-bbt;
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_nfc>;
+ status = "okay";
+};
+
&pwm0 {
pinctrl-names = "default";
pinctrl-0 = <&pinctrl_pwm0>;
@@ -156,6 +169,25 @@
>;
};
+ pinctrl_nfc: nfcgrp {
+ fsl,pins = <
+ 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
+ >;
+ };
+
pinctrl_pwm0: pwm0grp {
fsl,pins = <
VF610_PAD_PTB0__FTM0_CH0 0x1182
--
2.5.0
Hi Brian,
On 2015-08-03 11:27, Stefan Agner wrote:
<snip>
> +static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
> + uint8_t *oob, int oob_loaded)
> +{
> + struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> + u8 ecc_status;
> + u8 ecc_count;
> + int flip;
> +
> + ecc_status = __raw_readb(nfc->regs + ECC_SRAM_ADDR * 8 + ECC_OFFSET);
> + ecc_count = ecc_status & ECC_ERR_COUNT;
> +
> + if (!(ecc_status & ECC_STATUS_MASK))
> + return ecc_count;
> +
> + if (!oob_loaded)
> + vf610_nfc_read_buf(mtd, oob, mtd->oobsize);
> +
> + /*
> + * On an erased page, bit count (including OOB) should be zero or
> + * at least less then half of the ECC strength.
> + */
> + flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);
> + flip += count_written_bits(oob, mtd->oobsize - nfc->chip.ecc.bytes,
> + ecc_count);
With ECC the controller seems to clear the ECC bytes in SRAM buffer.
This is a dump of 64 Bit OOB with the 32-error ECC mode which requires
60 bytes of OOB for ECC:
[ 22.190273] ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 22.209698] vf610_nfc_correct_data, flips 1
Not sure if this is acceptable, but I now only count the bits in the
non-ECC area of the OOB.
Btw, if the ECC check fails, the controller seems kind of count the
amount of bitflips. It works for most devices reliable, but we had
devices for which that number was not accurate, see:
http://thread.gmane.org/gmane.linux.ports.arm.kernel/357439
--
Stefan
Hi Stefan,
Le Mon, 3 Aug 2015 11:27:25 +0200, Stefan Agner <[email protected]> a
écrit :
> The 10th revision includes some more review comments, including an
> improved handling of empty NAND pages with hardware ECC.
>
> More information and the full test log of earlier patchset version
> can be found in the cover letter of the last revision v6:
> http://thread.gmane.org/gmane.linux.kernel/1979868
>
> Changes since v9:
> - Remove inline of vf610_nfc_done
> - Add __iomem to src argument of vf610_nfc_memcpy
> - Handle return value of mtd_device_parse_register correctly
> - Count bits in OOB too (only non-ECC bits)
> - Return bitflips in ecc.read_page callback vf610_nfc_read_page
> - Fall-through ALT_BUF_ONFI
> - Use BIT macros
>
> Changes since v8:
> - Fix 16-Bit NAND flash support by splitting up initialization
> (introduce vf610_nfc_preinit_controller)
> - Updated comments in initialziation functions
>
> Changes since v7:
> - vf610-twr.dts: Moved NFC pinmux into the existing iomuxc node
> and sort new nfc node behind the existing iomuxc node as well.
> - vf610-twr.dts/vf-colibri.dtsi: Dropped _1 suffixes
>
> Changes since v6:
> - Rebased ontop of l2-mtd/master (v4.2-rc1 based)
> - Removed HAVE_NAND_VF610_NFC and use depends on. This made
> "[PATCH v6 4/6] ARM: vf610: enable NAND Flash Controller" unnecessary
>
> Changes since v5:
> - Removed fsl,mpc5125-nfc compatible string
> - Removed readl/writel_relaxed
> - Change interface of vf610_nfc_transfer_size to match other accessors
>
> Changes since v4:
> - Rebased ontop of l2-mtd/master (v4.1-rc4 based)
> - Eliminate unnecessary page read (NAND_CMD_SEQIN) since the driver does
> not support sub-page writes anyway (improves write performance)
> - Support ONFI by enabling READID command with offset and parameter page
> reads (CMD_PARAM)
> - Change to dedicated read_page/write_page function, enables raw writes
> - Use __LITTLE_ENDIAN to distingush between LE/BE relevant statements
> - Eliminated vf610_nfc_probe_dt in favor of common DT init code
> - Use wait_for_completion_timeout
> - Some style fixes (spaces, etc.)
>
> Changes since v3:
> - Make the driver selectable when COMPILE_TEST is set
> - Fix compile error due to superfluous ECC_STATUS configuration in initial
> patch (without ECC correction ECC_STATUS does not need to be configured)
> - Remove custom BBT pattern and switch to in-band BBT in the initial patch
> - Include two bug fixes, for details see the corresponding U-Boot patches:
> http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/215802
>
> Changes since v2:
> - Updated binding documentation
>
> Changes since v1:
> - Nest nfc_config struct within the main nfc struct
> - Use assigned clock binding to specify NFC clock
> - Rebased ontop of MSCM IR patchset (driver parts have been merged)
> - Split out arch Kconfig in a separate config
> - Fix module license
> - Updated MAINTAINERS
>
> Changes since RFC (Bill Pringlemeir):
> - Renamed driver from fsl_nfc to vf610_nfc
> - Use readl/writel for all register in accessor functions
> - Optimized field accessor functions
> - Implemented PM (suspend/resume) functions
> - Implemented basic support for ECC strength/ECC step size from dt
> - Improved performance of count_written_bits by using hweight32
> - Support ECC with 60-bytes to correct up to 32 bit errors
> - Changed to in-band BBT (NAND_BBT_NO_OOB) which also allows ECC modes
> which uses up to 60 bytes on 64 byte OOB
> - Removed custom (downstream) BBT pattern since BBT table won't be
> compatible anyway (due to the change above)
For the sake of regression testing:
Tested-by: Albert ARIBAUD <[email protected]>
Still works fine here on a Vybrid and 16-bit Micron NAND.
Cordialement,
Albert ARIBAUD
3ADEV
On Mon, Aug 03, 2015 at 11:28:43AM +0200, Stefan Agner wrote:
> On 2015-08-03 11:27, Stefan Agner wrote:
> <snip>
> > +static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
> > + uint8_t *oob, int oob_loaded)
> > +{
> > + struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> > + u8 ecc_status;
> > + u8 ecc_count;
> > + int flip;
> > +
> > + ecc_status = __raw_readb(nfc->regs + ECC_SRAM_ADDR * 8 + ECC_OFFSET);
Why __raw_readb()? That's not normally encourage, and it has issues with
endianness. It looks like maybe this is actulaly a 32-bit register, and
you're having trouble when trying to do bytewise access? I see this
earlier:
/*
* ECC status is stored at NFC_CFG[ECCADD] +4 for little-endian
* and +7 for big-endian SoCs.
*/
#ifdef __LITTLE_ENDIAN
#define ECC_OFFSET 4
#else
#define ECC_OFFSET 7
#endif
So maybe you really just want:
#define ECC_OFFSET 4
...
ecc_status = vf610_nfc_read(ECC_SRAM_ADDR * 8 + ECC_OFFSET) & 0xff;
?
> > + ecc_count = ecc_status & ECC_ERR_COUNT;
> > +
> > + if (!(ecc_status & ECC_STATUS_MASK))
> > + return ecc_count;
> > +
> > + if (!oob_loaded)
> > + vf610_nfc_read_buf(mtd, oob, mtd->oobsize);
> > +
> > + /*
> > + * On an erased page, bit count (including OOB) should be zero or
> > + * at least less then half of the ECC strength.
> > + */
> > + flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);
Another side note: why are you using ecc_count as a max threshold? AIUI,
an ECC algorithm doesn't really report useful error count information if
it's above the correction limit. So wouldn't we be looking to count up
to our SW threshold? i.e., ecc.strength / 2, or similar? Similar
comments below.
> > + flip += count_written_bits(oob, mtd->oobsize - nfc->chip.ecc.bytes,
> > + ecc_count);
>
> With ECC the controller seems to clear the ECC bytes in SRAM buffer.
> This is a dump of 64 Bit OOB with the 32-error ECC mode which requires
> 60 bytes of OOB for ECC:
>
> [ 22.190273] ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
Hmm, that's not really good. The point is that we need to make sure that
everything that could have been programmed (including the ECC area) was
not actually programmed. But your ECC controller is not, contrary to
MTD's expectations, dumping raw uncorrected data here.
> [ 22.209698] vf610_nfc_correct_data, flips 1
>
> Not sure if this is acceptable, but I now only count the bits in the
> non-ECC area of the OOB.
That's not the intention of my suggestion. You're still missing out on a
class of patterns that might look close to all 0xff but are not
actually.
If the HW ECC really doesn't give you valid data+OOB at this point, then
you might have to re-read with ECC disabled. Of course, that's got a
performance cost...
Or perhaps Boris has a better suggestion? He's been surveying other NAND
drivers that need to do similar things, and he's working on providing
some support code for common design patterns.
> Btw, if the ECC check fails, the controller seems kind of count the
> amount of bitflips. It works for most devices reliable, but we had
> devices for which that number was not accurate, see:
> http://thread.gmane.org/gmane.linux.ports.arm.kernel/357439
I'm a little confused there. Why would you be expecting to get a count
of bitflips, when the ECC engine can't correct all errors? How is it
supposed to know what the "right" data is if the bit errors are beyond
the correction strength?
Brian
A few more comments.
On Mon, Aug 03, 2015 at 11:27:26AM +0200, Stefan Agner wrote:
> diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
> new file mode 100644
> index 0000000..5c8dfe8
> --- /dev/null
> +++ b/drivers/mtd/nand/vf610_nfc.c
> @@ -0,0 +1,645 @@
...
> +/*
> + * This function supports Vybrid only (MPC5125 would have full RB and four CS)
> + */
> +static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip)
> +{
> +#ifdef CONFIG_SOC_VF610
Why the #ifdef? I don't see anything compile-time specific to SOC_VF610.
If this is trying to handle the comment above ("This function supports
Vybrid only (MPC5125 would have full RB and four CS)") then that's the
wrong way of doing it, as you need to support multiplatform kernels.
You'll need to have a way to differentiate the different platform
support at runtime, not compile time.
> + struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> + u32 tmp = vf610_nfc_read(nfc, NFC_ROW_ADDR);
> +
> + tmp &= ~(ROW_ADDR_CHIP_SEL_RB_MASK | ROW_ADDR_CHIP_SEL_MASK);
> + tmp |= 1 << ROW_ADDR_CHIP_SEL_RB_SHIFT;
> +
> + if (chip == 0)
> + tmp |= 1 << ROW_ADDR_CHIP_SEL_SHIFT;
> + else if (chip == 1)
> + tmp |= 2 << ROW_ADDR_CHIP_SEL_SHIFT;
else ... ?
Maybe you can write this as a formulaic pattern (e.g.:
tmp |= (chip + 1) << ROW_ADDR_CHIP_SEL_SHIFT;
) and just do the "max # of chips" checks on a per-platform basis in the
probe(). Then I'm guessing this same function can apply to both
platforms. (I'm not looking at HW datasheets for this, BTW, just
guessing based on the context here.)
But wait...I see that you call nand_scan_ident() with a max of 1 chip.
So you won't ever see the chip > 0 case, right?
So does this driver support multiple flash attached or not? Looks like
you're assuming you'll only be using chip-select 0. (This is fine for
now, but at least your code should acknowledge this. Perhaps a comment
at the top under "limitations.")
> +
> + vf610_nfc_write(nfc, NFC_ROW_ADDR, tmp);
> +#endif
> +}
...
> +static int vf610_nfc_probe(struct platform_device *pdev)
> +{
...
> + /* first scan to find the device and get the page size */
> + if (nand_scan_ident(mtd, 1, NULL)) {
> + err = -ENXIO;
> + goto error;
> + }
...
Brian
Sorry, I realized a potential issue here.
On Mon, Aug 03, 2015 at 11:27:28AM +0200, Stefan Agner wrote:
> Signed-off-by: Bill Pringlemeir <[email protected]>
> Acked-by: Shawn Guo <[email protected]>
> Reviewed-by: Brian Norris <[email protected]>
> Signed-off-by: Stefan Agner <[email protected]>
> ---
> .../devicetree/bindings/mtd/vf610-nfc.txt | 45 ++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mtd/vf610-nfc.txt
>
> diff --git a/Documentation/devicetree/bindings/mtd/vf610-nfc.txt b/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
> new file mode 100644
> index 0000000..cae5f25
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
> @@ -0,0 +1,45 @@
> +Freescale's NAND flash controller (NFC)
> +
> +This variant of the Freescale NAND flash controller (NFC) can be found on
> +Vybrid (vf610), MPC5125, MCF54418 and Kinetis K70.
> +
> +Required properties:
> +- compatible: Should be set to "fsl,vf610-nfc"
> +- reg: address range of the NFC
> +- interrupts: interrupt of the NFC
> +- nand-bus-width: see nand.txt
> +- nand-ecc-mode: see nand.txt
> +- nand-on-flash-bbt: see nand.txt
Stumbling across the "multi-CS" questions on the driver reminds me: it
typically makes sense to define new NAND bindings using separate NAND
*controller* and *flash* device nodes. The above 3 properties, at least,
would apply on a per-flash basis, not per-controller typically. See
sunxi-nand, for instance:
http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/mtd/sunxi-nand.txt
brcmnand had a similar pattern:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
(Perhaps it's time we standardized this a little more formally...)
> +- assigned-clocks: main clock from the SoC, for Vybrid <&clks VF610_CLK_NFC>;
> +- assigned-clock-rates: The NAND bus timing is derived from this clock
> + rate and should not exceed maximum timing for any NAND memory chip
> + in a board stuffing. Typical NAND memory timings derived from this
> + clock are found in the SoC hardware reference manual. Furthermore,
> + there might be restrictions on maximum rates when using hardware ECC.
> +
> +- #address-cells, #size-cells : Must be present if the device has sub-nodes
> + representing partitions.
> +
> +Required properties for hardware ECC:
> +- nand-ecc-strength: supported strengths are 24 and 32 bit (see nand.txt)
> +- nand-ecc-step-size: step size equals page size, currently only 2k pages are
> + supported
> +
> +Example:
> +
> + nfc: nand@400e0000 {
> + compatible = "fsl,vf610-nfc";
> + #address-cells = <1>;
> + #size-cells = <1>;
> + reg = <0x400e0000 0x4000>;
> + interrupts = <GIC_SPI 83 IRQ_TYPE_LEVEL_HIGH>;
> + clocks = <&clks VF610_CLK_NFC>;
> + clock-names = "nfc";
> + assigned-clocks = <&clks VF610_CLK_NFC>;
> + assigned-clock-rates = <33000000>;
> + nand-bus-width = <8>;
> + nand-ecc-mode = "hw";
> + nand-ecc-strength = <32>;
> + nand-ecc-step-size = <2048>;
> + nand-on-flash-bbt;
> + };
Brian
One more thing...
On Mon, Aug 03, 2015 at 11:27:26AM +0200, Stefan Agner wrote:
> --- /dev/null
> +++ b/drivers/mtd/nand/vf610_nfc.c
> @@ -0,0 +1,645 @@
...
> +struct vf610_nfc {
> + struct mtd_info mtd;
> + struct nand_chip chip;
> + struct device *dev;
> + void __iomem *regs;
> + struct completion cmd_done;
> + uint buf_offset;
> + int page_sz;
AFAICT (even with the 2nd patch), you never really use this field. You
just set it/increment it, but don't use it for anything. Kill it?
> + /* Status and ID are in alternate locations. */
> + int alt_buf;
> +#define ALT_BUF_ID 1
> +#define ALT_BUF_STAT 2
> +#define ALT_BUF_ONFI 3
> + struct clk *clk;
> +};
Brian
Brian, Stefan,
On Tue, 25 Aug 2015 12:54:11 -0700
Brian Norris <[email protected]> wrote:
> On Mon, Aug 03, 2015 at 11:28:43AM +0200, Stefan Agner wrote:
> > On 2015-08-03 11:27, Stefan Agner wrote:
> > <snip>
> > > +static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
> > > + uint8_t *oob, int oob_loaded)
> > > +{
> > > + struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> > > + u8 ecc_status;
> > > + u8 ecc_count;
> > > + int flip;
> > > +
> > > + ecc_status = __raw_readb(nfc->regs + ECC_SRAM_ADDR * 8 + ECC_OFFSET);
>
> Why __raw_readb()? That's not normally encourage, and it has issues with
> endianness. It looks like maybe this is actulaly a 32-bit register, and
> you're having trouble when trying to do bytewise access? I see this
> earlier:
>
> /*
> * ECC status is stored at NFC_CFG[ECCADD] +4 for little-endian
> * and +7 for big-endian SoCs.
> */
> #ifdef __LITTLE_ENDIAN
> #define ECC_OFFSET 4
> #else
> #define ECC_OFFSET 7
> #endif
>
> So maybe you really just want:
>
> #define ECC_OFFSET 4
> ...
> ecc_status = vf610_nfc_read(ECC_SRAM_ADDR * 8 + ECC_OFFSET) & 0xff;
>
> ?
>
> > > + ecc_count = ecc_status & ECC_ERR_COUNT;
> > > +
> > > + if (!(ecc_status & ECC_STATUS_MASK))
> > > + return ecc_count;
> > > +
> > > + if (!oob_loaded)
> > > + vf610_nfc_read_buf(mtd, oob, mtd->oobsize);
> > > +
> > > + /*
> > > + * On an erased page, bit count (including OOB) should be zero or
> > > + * at least less then half of the ECC strength.
> > > + */
> > > + flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);
>
> Another side note: why are you using ecc_count as a max threshold? AIUI,
> an ECC algorithm doesn't really report useful error count information if
> it's above the correction limit. So wouldn't we be looking to count up
> to our SW threshold? i.e., ecc.strength / 2, or similar? Similar
> comments below.
The exact threshold value is still something I'm not sure about, though
I'm sure it should be correlated to ecc.strength value (whether it's
directly set to ecc.strength or less than ecc.strength is something
we'll have to figure out).
>
> > > + flip += count_written_bits(oob, mtd->oobsize - nfc->chip.ecc.bytes,
> > > + ecc_count);
> >
> > With ECC the controller seems to clear the ECC bytes in SRAM buffer.
> > This is a dump of 64 Bit OOB with the 32-error ECC mode which requires
> > 60 bytes of OOB for ECC:
> >
> > [ 22.190273] ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
> > 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>
> Hmm, that's not really good. The point is that we need to make sure that
> everything that could have been programmed (including the ECC area) was
> not actually programmed. But your ECC controller is not, contrary to
> MTD's expectations, dumping raw uncorrected data here.
Yep, for this test we really need the ECC bytes generated for the chunk
you're currently testing.
How to retrieve those bytes really depends on your NAND controller, but
such controllers usually provides a way to disable the ECC engine. The
only thing you'll have to do in this case is disable the ECC engine and
read the OOB data (using RNDOUT and read_buf for example).
>
> > [ 22.209698] vf610_nfc_correct_data, flips 1
> >
> > Not sure if this is acceptable, but I now only count the bits in the
> > non-ECC area of the OOB.
>
> That's not the intention of my suggestion. You're still missing out on a
> class of patterns that might look close to all 0xff but are not
> actually.
Exactly.
>
> If the HW ECC really doesn't give you valid data+OOB at this point, then
> you might have to re-read with ECC disabled. Of course, that's got a
> performance cost...
As suggested above, if that's possible, reading the OOB area (or a
portion of the OOB area) with the ECC engine disabled should be enough.
>
> Or perhaps Boris has a better suggestion? He's been surveying other NAND
> drivers that need to do similar things, and he's working on providing
> some support code for common design patterns.
Yep, the patch series is here in case you want to have a look [1].
Best Regards,
Boris
[1]https://patchwork.ozlabs.org/patch/509970/
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On 25 Aug 2015, [email protected] wrote:
> Sorry, I realized a potential issue here.
> On Mon, Aug 03, 2015 at 11:27:28AM +0200, Stefan Agner wrote:
>> Signed-off-by: Bill Pringlemeir <[email protected]>
>> Acked-by: Shawn Guo <[email protected]>
>> Reviewed-by: Brian Norris <[email protected]>
>> Signed-off-by: Stefan Agner <[email protected]>
>> ---
>> .../devicetree/bindings/mtd/vf610-nfc.txt | 45 ++++++++++++++++++++++
>> 1 file changed, 45 insertions(+) create mode 100644
>> Documentation/devicetree/bindings/mtd/vf610-nfc.txt
>> diff --git a/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
>> b/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
>> new file mode 100644
>> index 0000000..cae5f25
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
>>>> -0,0 +1,45 @@
>> +- nand-bus-width: see nand.txt
>> +- nand-ecc-mode: see nand.txt
>> +- nand-on-flash-bbt: see nand.txt
> Stumbling across the "multi-CS" questions on the driver reminds me: it
> typically makes sense to define new NAND bindings using separate NAND
> *controller* and *flash* device nodes. The above 3 properties, at
> least, would apply on a per-flash basis, not per-controller
> typically. See sunxi-nand, for instance:
> http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/mtd/sunxi-nand.txt
> brcmnand had a similar pattern:
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
> (Perhaps it's time we standardized this a little more formally...)
These would apply per chip, but the controller has to be configured to
support each and every one. Every time an operation was performed, we
would have to check the chip type and reconfigure the controller.
Currently, the driver does not support this and it would add a lot of
overhead in some cases unless a register cache was used.
Is the flexibility of using a system with combined 8/16bit devices
really worth all the overhead? Isn't it sort of brain dead hardware not
to make all of the chips similar? Why would everyone have to pay for
such a crazy setup?
To separate it would at least be a lie versus the code in the current
form. As well, there are only a few SOC which support multiple chip
selects. The 'multi-CS' register bits of this controller varies between
PowerPC, 68K/Coldfire and ARM platforms.
I looked briefly at the brcmnand.c and it seems that it is not
supporting different ECC per chip even though the nodes are broken out
this way. In fact, if some raw functions are called, I think it will
put it in ECC mode even if it wasn't before? Well, I agree that this
would be good generically, I think it puts a lot of effort in the
drivers for not so much payoff?
Fwiw,
Bill Pringlemeir.
Hi Bill,
On Wed, 26 Aug 2015 11:26:36 -0400
Bill Pringlemeir <[email protected]> wrote:
> On 25 Aug 2015, [email protected] wrote:
>
> > Sorry, I realized a potential issue here.
>
> > On Mon, Aug 03, 2015 at 11:27:28AM +0200, Stefan Agner wrote:
> >> Signed-off-by: Bill Pringlemeir <[email protected]>
> >> Acked-by: Shawn Guo <[email protected]>
> >> Reviewed-by: Brian Norris <[email protected]>
> >> Signed-off-by: Stefan Agner <[email protected]>
> >> ---
> >> .../devicetree/bindings/mtd/vf610-nfc.txt | 45 ++++++++++++++++++++++
> >> 1 file changed, 45 insertions(+) create mode 100644
> >> Documentation/devicetree/bindings/mtd/vf610-nfc.txt
>
> >> diff --git a/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
> >> b/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
> >> new file mode 100644
> >> index 0000000..cae5f25
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
> >>>> -0,0 +1,45 @@
> >> +- nand-bus-width: see nand.txt
> >> +- nand-ecc-mode: see nand.txt
> >> +- nand-on-flash-bbt: see nand.txt
>
> > Stumbling across the "multi-CS" questions on the driver reminds me: it
> > typically makes sense to define new NAND bindings using separate NAND
> > *controller* and *flash* device nodes. The above 3 properties, at
> > least, would apply on a per-flash basis, not per-controller
> > typically. See sunxi-nand, for instance:
>
> > http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/mtd/sunxi-nand.txt
>
> > brcmnand had a similar pattern:
>
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
>
> > (Perhaps it's time we standardized this a little more formally...)
>
> These would apply per chip, but the controller has to be configured to
> support each and every one. Every time an operation was performed, we
> would have to check the chip type and reconfigure the controller.
> Currently, the driver does not support this and it would add a lot of
> overhead in some cases unless a register cache was used.
>
> Is the flexibility of using a system with combined 8/16bit devices
> really worth all the overhead? Isn't it sort of brain dead hardware not
> to make all of the chips similar? Why would everyone have to pay for
> such a crazy setup?
>
> To separate it would at least be a lie versus the code in the current
> form. As well, there are only a few SOC which support multiple chip
> selects. The 'multi-CS' register bits of this controller varies between
> PowerPC, 68K/Coldfire and ARM platforms.
>
> I looked briefly at the brcmnand.c and it seems that it is not
> supporting different ECC per chip even though the nodes are broken out
> this way. In fact, if some raw functions are called, I think it will
> put it in ECC mode even if it wasn't before? Well, I agree that this
> would be good generically, I think it puts a lot of effort in the
> drivers for not so much payoff?
Hm, the sunxi driver supports it, and it does not add such a big
overhead...
The only thing you have to do is cache a bunch of register values
per-chip and restore/apply them when the chip is selected
(in your ->select_chip() implementation).
Anyway, even if the suggested DT representation is a lie in regards to
your implementation, it's actually pretty accurate from an hardware
POV, and this is exactly what DT is supposed to represent.
Best Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
On 2015-08-25 12:54, Brian Norris wrote:
> On Mon, Aug 03, 2015 at 11:28:43AM +0200, Stefan Agner wrote:
>> On 2015-08-03 11:27, Stefan Agner wrote:
>> <snip>
>> > +static inline int vf610_nfc_correct_data(struct mtd_info *mtd, uint8_t *dat,
>> > + uint8_t *oob, int oob_loaded)
>> > +{
>> > + struct vf610_nfc *nfc = mtd_to_nfc(mtd);
>> > + u8 ecc_status;
>> > + u8 ecc_count;
>> > + int flip;
>> > +
>> > + ecc_status = __raw_readb(nfc->regs + ECC_SRAM_ADDR * 8 + ECC_OFFSET);
>
> Why __raw_readb()? That's not normally encourage, and it has issues with
> endianness. It looks like maybe this is actulaly a 32-bit register, and
> you're having trouble when trying to do bytewise access? I see this
> earlier:
>
> /*
> * ECC status is stored at NFC_CFG[ECCADD] +4 for little-endian
> * and +7 for big-endian SoCs.
> */
> #ifdef __LITTLE_ENDIAN
> #define ECC_OFFSET 4
> #else
> #define ECC_OFFSET 7
> #endif
>
> So maybe you really just want:
>
> #define ECC_OFFSET 4
> ...
> ecc_status = vf610_nfc_read(ECC_SRAM_ADDR * 8 + ECC_OFFSET) & 0xff;
>
> ?
>
Agreed, much cleaner.
>> > + ecc_count = ecc_status & ECC_ERR_COUNT;
>> > +
>> > + if (!(ecc_status & ECC_STATUS_MASK))
>> > + return ecc_count;
>> > +
>> > + if (!oob_loaded)
>> > + vf610_nfc_read_buf(mtd, oob, mtd->oobsize);
>> > +
>> > + /*
>> > + * On an erased page, bit count (including OOB) should be zero or
>> > + * at least less then half of the ECC strength.
>> > + */
>> > + flip = count_written_bits(dat, nfc->chip.ecc.size, ecc_count);
>
> Another side note: why are you using ecc_count as a max threshold? AIUI,
> an ECC algorithm doesn't really report useful error count information if
> it's above the correction limit. So wouldn't we be looking to count up
> to our SW threshold? i.e., ecc.strength / 2, or similar? Similar
> comments below.
>
Initially, that was the only threshold below, hence it made sense. But I
agree, we should count up to the threshold used below...
>> > + flip += count_written_bits(oob, mtd->oobsize - nfc->chip.ecc.bytes,
>> > + ecc_count);
>>
>> With ECC the controller seems to clear the ECC bytes in SRAM buffer.
>> This is a dump of 64 Bit OOB with the 32-error ECC mode which requires
>> 60 bytes of OOB for ECC:
>>
>> [ 22.190273] ff ff ff ff 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>> 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
>
> Hmm, that's not really good. The point is that we need to make sure that
> everything that could have been programmed (including the ECC area) was
> not actually programmed. But your ECC controller is not, contrary to
> MTD's expectations, dumping raw uncorrected data here.
>
>> [ 22.209698] vf610_nfc_correct_data, flips 1
>>
>> Not sure if this is acceptable, but I now only count the bits in the
>> non-ECC area of the OOB.
>
> That's not the intention of my suggestion. You're still missing out on a
> class of patterns that might look close to all 0xff but are not
> actually.
>
> If the HW ECC really doesn't give you valid data+OOB at this point, then
> you might have to re-read with ECC disabled. Of course, that's got a
> performance cost...
Yes I can do that. Not sure yet how it will look like exactly, maybe I
only need to reread the OOB area and (re-)use the main data part since
those arrive uncorrected in the error case.
>
> Or perhaps Boris has a better suggestion? He's been surveying other NAND
> drivers that need to do similar things, and he's working on providing
> some support code for common design patterns.
>
>> Btw, if the ECC check fails, the controller seems kind of count the
>> amount of bitflips. It works for most devices reliable, but we had
>> devices for which that number was not accurate, see:
>> http://thread.gmane.org/gmane.linux.ports.arm.kernel/357439
>
> I'm a little confused there. Why would you be expecting to get a count
> of bitflips, when the ECC engine can't correct all errors? How is it
> supposed to know what the "right" data is if the bit errors are beyond
> the correction strength?
When printing the ECC error count on ECC fail when reading an erased
NAND flash, the numbers of bit flips (stuck at zero) seem to widely
correlate with the number returned by the controller. While it seems to
correlate widely, there are exceptions, as discussed in the thread:
http://thread.gmane.org/gmane.linux.ports.arm.kernel/295424
Maybe this is an artifact of the ECC algorithm we just can't/shouldn't
rely on? I am not sure where this originated, I did not found any
indication in the reference manual about what that value contains in the
error case.
Bill, do you have an idea why we used that value as threshold in early
implementations?
Otherwise I also think we should just drop the use of this value.
--
Stefan
On 2015-08-26 08:39, Boris Brezillon wrote:
> Hi Bill,
>
> On Wed, 26 Aug 2015 11:26:36 -0400
> Bill Pringlemeir <[email protected]> wrote:
>
>> On 25 Aug 2015, [email protected] wrote:
>>
>> > Sorry, I realized a potential issue here.
>>
>> > On Mon, Aug 03, 2015 at 11:27:28AM +0200, Stefan Agner wrote:
>> >> Signed-off-by: Bill Pringlemeir <[email protected]>
>> >> Acked-by: Shawn Guo <[email protected]>
>> >> Reviewed-by: Brian Norris <[email protected]>
>> >> Signed-off-by: Stefan Agner <[email protected]>
>> >> ---
>> >> .../devicetree/bindings/mtd/vf610-nfc.txt | 45 ++++++++++++++++++++++
>> >> 1 file changed, 45 insertions(+) create mode 100644
>> >> Documentation/devicetree/bindings/mtd/vf610-nfc.txt
>>
>> >> diff --git a/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
>> >> b/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
>> >> new file mode 100644
>> >> index 0000000..cae5f25
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/mtd/vf610-nfc.txt
>> >>>> -0,0 +1,45 @@
>> >> +- nand-bus-width: see nand.txt
>> >> +- nand-ecc-mode: see nand.txt
>> >> +- nand-on-flash-bbt: see nand.txt
>>
>> > Stumbling across the "multi-CS" questions on the driver reminds me: it
>> > typically makes sense to define new NAND bindings using separate NAND
>> > *controller* and *flash* device nodes. The above 3 properties, at
>> > least, would apply on a per-flash basis, not per-controller
>> > typically. See sunxi-nand, for instance:
>>
>> > http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/mtd/sunxi-nand.txt
>>
>> > brcmnand had a similar pattern:
>>
>> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mtd/brcm,brcmnand.txt
>>
>> > (Perhaps it's time we standardized this a little more formally...)
>>
>> These would apply per chip, but the controller has to be configured to
>> support each and every one. Every time an operation was performed, we
>> would have to check the chip type and reconfigure the controller.
>> Currently, the driver does not support this and it would add a lot of
>> overhead in some cases unless a register cache was used.
>>
>> Is the flexibility of using a system with combined 8/16bit devices
>> really worth all the overhead? Isn't it sort of brain dead hardware not
>> to make all of the chips similar? Why would everyone have to pay for
>> such a crazy setup?
>>
>> To separate it would at least be a lie versus the code in the current
>> form. As well, there are only a few SOC which support multiple chip
>> selects. The 'multi-CS' register bits of this controller varies between
>> PowerPC, 68K/Coldfire and ARM platforms.
The DT can be a lie versus the code. The DT should reflect how the
hardware is wired, afaik, if we take shortcuts in the driver code, that
is fine. If we don't support a certain configuration right now (e.g.
second NAND chip), the driver can just return an appropriate error code.
>>
>> I looked briefly at the brcmnand.c and it seems that it is not
>> supporting different ECC per chip even though the nodes are broken out
>> this way. In fact, if some raw functions are called, I think it will
>> put it in ECC mode even if it wasn't before? Well, I agree that this
>> would be good generically, I think it puts a lot of effort in the
>> drivers for not so much payoff?
>
> Hm, the sunxi driver supports it, and it does not add such a big
> overhead...
> The only thing you have to do is cache a bunch of register values
> per-chip and restore/apply them when the chip is selected
> (in your ->select_chip() implementation).
>
> Anyway, even if the suggested DT representation is a lie in regards to
> your implementation, it's actually pretty accurate from an hardware
> POV, and this is exactly what DT is supposed to represent.
I agree with both of you. I don't see much value implementing multi-NAND
chip support, especially with different configurations, at the moment. I
am not aware of any hardware making use of that now.
I will update the driver to parse a NAND sub node and get the ECC
properties from the per flash configuration. However, I won't add chip
select or multi-NAND support right now...
Any objection?
--
Stefan
On Wed, Aug 26, 2015 at 02:15:45PM -0700, Stefan Agner wrote:
> On 2015-08-26 08:39, Boris Brezillon wrote:
> > On Wed, 26 Aug 2015 11:26:36 -0400
> > Bill Pringlemeir <[email protected]> wrote:
> >> These would apply per chip, but the controller has to be configured to
> >> support each and every one. Every time an operation was performed, we
> >> would have to check the chip type and reconfigure the controller.
> >> Currently, the driver does not support this and it would add a lot of
> >> overhead in some cases unless a register cache was used.
> >>
> >> Is the flexibility of using a system with combined 8/16bit devices
> >> really worth all the overhead? Isn't it sort of brain dead hardware not
> >> to make all of the chips similar? Why would everyone have to pay for
> >> such a crazy setup?
> >>
> >> To separate it would at least be a lie versus the code in the current
> >> form. As well, there are only a few SOC which support multiple chip
> >> selects. The 'multi-CS' register bits of this controller varies between
> >> PowerPC, 68K/Coldfire and ARM platforms.
>
> The DT can be a lie versus the code. The DT should reflect how the
> hardware is wired, afaik, if we take shortcuts in the driver code, that
> is fine. If we don't support a certain configuration right now (e.g.
> second NAND chip), the driver can just return an appropriate error code.
Right, I was only asking for:
(1) a more accurate DT and
(2) clarity in the driver; the clarity might just be "we don't support
multi-CS"
> >> I looked briefly at the brcmnand.c and it seems that it is not
> >> supporting different ECC per chip even though the nodes are broken out
> >> this way. In fact, if some raw functions are called, I think it will
> >> put it in ECC mode even if it wasn't before? Well, I agree that this
> >> would be good generically, I think it puts a lot of effort in the
> >> drivers for not so much payoff?
> >
> > Hm, the sunxi driver supports it, and it does not add such a big
> > overhead...
> > The only thing you have to do is cache a bunch of register values
> > per-chip and restore/apply them when the chip is selected
> > (in your ->select_chip() implementation).
> >
> > Anyway, even if the suggested DT representation is a lie in regards to
> > your implementation, it's actually pretty accurate from an hardware
> > POV, and this is exactly what DT is supposed to represent.
>
> I agree with both of you. I don't see much value implementing multi-NAND
> chip support, especially with different configurations, at the moment. I
> am not aware of any hardware making use of that now.
>
> I will update the driver to parse a NAND sub node and get the ECC
> properties from the per flash configuration. However, I won't add chip
> select or multi-NAND support right now...
>
> Any objection?
Nope, sounds good to me.
A few tips:
* be defensive; i.e., error out if someone specifies 2 flash in the DT
* use the 'reg' property to be the addressing index in the flash
sub-node; i.e., the chip-select. This fits the practice done by most
others, I think.
Regards,
Brian
On Wed, Aug 26, 2015 at 10:57:38AM -0700, Stefan Agner wrote:
> On 2015-08-25 12:54, Brian Norris wrote:
> > On Mon, Aug 03, 2015 at 11:28:43AM +0200, Stefan Agner wrote:
> >> Btw, if the ECC check fails, the controller seems kind of count the
> >> amount of bitflips. It works for most devices reliable, but we had
> >> devices for which that number was not accurate, see:
> >> http://thread.gmane.org/gmane.linux.ports.arm.kernel/357439
> >
> > I'm a little confused there. Why would you be expecting to get a count
> > of bitflips, when the ECC engine can't correct all errors? How is it
> > supposed to know what the "right" data is if the bit errors are beyond
> > the correction strength?
>
> When printing the ECC error count on ECC fail when reading an erased
> NAND flash, the numbers of bit flips (stuck at zero) seem to widely
> correlate with the number returned by the controller. While it seems to
> correlate widely, there are exceptions, as discussed in the thread:
> http://thread.gmane.org/gmane.linux.ports.arm.kernel/295424
>
> Maybe this is an artifact of the ECC algorithm we just can't/shouldn't
> rely on? I am not sure where this originated, I did not found any
> indication in the reference manual about what that value contains in the
> error case.
Doesn't sound too reliable to me. And I'm not sure even if it was
reliable, that it would provide much value. We have to a lot of
re-counting anyway, so we might as well just be using our own threshold.
Or maybe I'm missing the point.
> Bill, do you have an idea why we used that value as threshold in early
> implementations?
>
> Otherwise I also think we should just drop the use of this value.
Brian
On 2015-08-25 13:16, Brian Norris wrote:
> A few more comments.
>
> On Mon, Aug 03, 2015 at 11:27:26AM +0200, Stefan Agner wrote:
>> diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
>> new file mode 100644
>> index 0000000..5c8dfe8
>> --- /dev/null
>> +++ b/drivers/mtd/nand/vf610_nfc.c
>> @@ -0,0 +1,645 @@
>
> ...
>
>> +/*
>> + * This function supports Vybrid only (MPC5125 would have full RB and four CS)
>> + */
>> +static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip)
>> +{
>> +#ifdef CONFIG_SOC_VF610
>
> Why the #ifdef? I don't see anything compile-time specific to SOC_VF610.
>
> If this is trying to handle the comment above ("This function supports
> Vybrid only (MPC5125 would have full RB and four CS)") then that's the
> wrong way of doing it, as you need to support multiplatform kernels.
> You'll need to have a way to differentiate the different platform
> support at runtime, not compile time.
Yes it is trying to handle the comment above. Well, the other two
platforms I am aware of are also different architectures... (PowerPC and
ColdFire). I think we won't have a multi-architecture kernel anytime
soon, hence I think removing the code at compile time is the right thing
todo.
However, probably CONFIG_SOC_VF610 is the wrong symbol then, I could
just use CONFIG_ARM and add a comment that this might be different on
another other ARM SoC than VF610.
Just checked CodingStyle, and I see that IS_ENABLED is the preferred way
for conditional compiling.
So my suggestion:
static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip)
{
struct vf610_nfc *nfc = mtd_to_nfc(mtd);
u32 tmp = vf610_nfc_read(nfc, NFC_ROW_ADDR);
if (!IS_ENABLED(CONFIG_ARM))
return;
/*
* This code is only tested on the ARM platform VF610
* PowerPC based MPC5125 would have full RB and four CS
*/
....
With that the compiler should be able to remove this (currently) ARM
VF610 specific code on the other supported architectures...
What do you think?
>
>> + struct vf610_nfc *nfc = mtd_to_nfc(mtd);
>> + u32 tmp = vf610_nfc_read(nfc, NFC_ROW_ADDR);
>> +
>> + tmp &= ~(ROW_ADDR_CHIP_SEL_RB_MASK | ROW_ADDR_CHIP_SEL_MASK);
>> + tmp |= 1 << ROW_ADDR_CHIP_SEL_RB_SHIFT;
>> +
>> + if (chip == 0)
>> + tmp |= 1 << ROW_ADDR_CHIP_SEL_SHIFT;
>> + else if (chip == 1)
>> + tmp |= 2 << ROW_ADDR_CHIP_SEL_SHIFT;
>
> else ... ?
>
> Maybe you can write this as a formulaic pattern (e.g.:
>
> tmp |= (chip + 1) << ROW_ADDR_CHIP_SEL_SHIFT;
>
> ) and just do the "max # of chips" checks on a per-platform basis in the
> probe(). Then I'm guessing this same function can apply to both
> platforms. (I'm not looking at HW datasheets for this, BTW, just
> guessing based on the context here.)
It seems that MCP5125 is different than VF610. MCP5125 has 4 chip
selects and 4 R/B signals, whereas VF610 has only 2 chip selects and
just 1 R/B signals...
> But wait...I see that you call nand_scan_ident() with a max of 1 chip.
> So you won't ever see the chip > 0 case, right?
>
> So does this driver support multiple flash attached or not? Looks like
> you're assuming you'll only be using chip-select 0. (This is fine for
> now, but at least your code should acknowledge this. Perhaps a comment
> at the top under "limitations.")
>
Ok, will add that information under limitations.
>> +
>> + vf610_nfc_write(nfc, NFC_ROW_ADDR, tmp);
>> +#endif
>> +}
>
> ...
>
>> +static int vf610_nfc_probe(struct platform_device *pdev)
>> +{
>
> ...
>
>> + /* first scan to find the device and get the page size */
>> + if (nand_scan_ident(mtd, 1, NULL)) {
>> + err = -ENXIO;
>> + goto error;
>> + }
--
Stefan
On 2015-08-25 13:34, Brian Norris wrote:
> One more thing...
>
> On Mon, Aug 03, 2015 at 11:27:26AM +0200, Stefan Agner wrote:
>> --- /dev/null
>> +++ b/drivers/mtd/nand/vf610_nfc.c
>> @@ -0,0 +1,645 @@
> ...
>> +struct vf610_nfc {
>> + struct mtd_info mtd;
>> + struct nand_chip chip;
>> + struct device *dev;
>> + void __iomem *regs;
>> + struct completion cmd_done;
>> + uint buf_offset;
>> + int page_sz;
>
> AFAICT (even with the 2nd patch), you never really use this field. You
> just set it/increment it, but don't use it for anything. Kill it?
It is used in the write path, I think I meant to use it for subpage
writes, when I thought it would just mean to transfer only parts of the
page to the controller.
However, as the subpage discussion basically concluded in not using it
for now on this controller, we can as well transfer the complete page
(page_sz). Or is there another case in which vf610_nfc_write_buf could
be called with less than page_sz?
--
Stefan
On Wed, Aug 26, 2015 at 06:02:31PM -0700, Stefan Agner wrote:
> On 2015-08-25 13:16, Brian Norris wrote:
> > On Mon, Aug 03, 2015 at 11:27:26AM +0200, Stefan Agner wrote:
> >> diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
> >> new file mode 100644
> >> index 0000000..5c8dfe8
> >> --- /dev/null
> >> +++ b/drivers/mtd/nand/vf610_nfc.c
> >> @@ -0,0 +1,645 @@
> >
> > ...
> >
> >> +/*
> >> + * This function supports Vybrid only (MPC5125 would have full RB and four CS)
> >> + */
> >> +static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip)
> >> +{
> >> +#ifdef CONFIG_SOC_VF610
> >
> > Why the #ifdef? I don't see anything compile-time specific to SOC_VF610.
> >
> > If this is trying to handle the comment above ("This function supports
> > Vybrid only (MPC5125 would have full RB and four CS)") then that's the
> > wrong way of doing it, as you need to support multiplatform kernels.
> > You'll need to have a way to differentiate the different platform
> > support at runtime, not compile time.
>
> Yes it is trying to handle the comment above. Well, the other two
> platforms I am aware of are also different architectures... (PowerPC and
> ColdFire). I think we won't have a multi-architecture kernel anytime
> soon,
Ha, right. Sorry, I don't really know this particular IP.
> hence I think removing the code at compile time is the right thing
> todo.
I don't believe that conclusion follows though.
> However, probably CONFIG_SOC_VF610 is the wrong symbol then, I could
> just use CONFIG_ARM and add a comment that this might be different on
> another other ARM SoC than VF610.
>
> Just checked CodingStyle, and I see that IS_ENABLED is the preferred way
> for conditional compiling.
>
> So my suggestion:
>
> static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip)
> {
> struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> u32 tmp = vf610_nfc_read(nfc, NFC_ROW_ADDR);
>
> if (!IS_ENABLED(CONFIG_ARM))
> return;
>
> /*
> * This code is only tested on the ARM platform VF610
> * PowerPC based MPC5125 would have full RB and four CS
> */
> ....
>
> With that the compiler should be able to remove this (currently) ARM
> VF610 specific code on the other supported architectures...
>
> What do you think?
The code structure isn't bad, and yes, IS_ENABLED() would be preferable,
as it removes some of the problems with #ifdef, but I still don't think
the processor architecture has much to do with the version of the IP.
The canonical way of distiguishing per-IP revisions is to key on the
compatible property. So you'd have some kind of enum, which would
currently only have an entry for VF610. i.e.:
/* MPC5125 not yet supported */
if (nfc->revision != NAND_VFC610)
return;
> >> + struct vf610_nfc *nfc = mtd_to_nfc(mtd);
> >> + u32 tmp = vf610_nfc_read(nfc, NFC_ROW_ADDR);
> >> +
> >> + tmp &= ~(ROW_ADDR_CHIP_SEL_RB_MASK | ROW_ADDR_CHIP_SEL_MASK);
> >> + tmp |= 1 << ROW_ADDR_CHIP_SEL_RB_SHIFT;
> >> +
> >> + if (chip == 0)
> >> + tmp |= 1 << ROW_ADDR_CHIP_SEL_SHIFT;
> >> + else if (chip == 1)
> >> + tmp |= 2 << ROW_ADDR_CHIP_SEL_SHIFT;
> >
> > else ... ?
> >
> > Maybe you can write this as a formulaic pattern (e.g.:
> >
> > tmp |= (chip + 1) << ROW_ADDR_CHIP_SEL_SHIFT;
> >
> > ) and just do the "max # of chips" checks on a per-platform basis in the
> > probe(). Then I'm guessing this same function can apply to both
> > platforms. (I'm not looking at HW datasheets for this, BTW, just
> > guessing based on the context here.)
>
> It seems that MCP5125 is different than VF610. MCP5125 has 4 chip
> selects and 4 R/B signals, whereas VF610 has only 2 chip selects and
> just 1 R/B signals...
OK I don't presume to know what the different IP versions look like. And
if you just note they are unsupported/untested, you're fine.
Brian
On Wed, Aug 26, 2015 at 06:10:05PM -0700, Stefan Agner wrote:
> On 2015-08-25 13:34, Brian Norris wrote:
> > One more thing...
> >
> > On Mon, Aug 03, 2015 at 11:27:26AM +0200, Stefan Agner wrote:
> >> --- /dev/null
> >> +++ b/drivers/mtd/nand/vf610_nfc.c
> >> @@ -0,0 +1,645 @@
> > ...
> >> +struct vf610_nfc {
> >> + struct mtd_info mtd;
> >> + struct nand_chip chip;
> >> + struct device *dev;
> >> + void __iomem *regs;
> >> + struct completion cmd_done;
> >> + uint buf_offset;
> >> + int page_sz;
> >
> > AFAICT (even with the 2nd patch), you never really use this field. You
> > just set it/increment it, but don't use it for anything. Kill it?
>
> It is used in the write path, I think I meant to use it for subpage
> writes, when I thought it would just mean to transfer only parts of the
> page to the controller.
Ah, you're right. Sorry, I missed that. I got mixed up seeing most of
your uses of 'page_sz' were for a local variable of the same name, not
this field.
> However, as the subpage discussion basically concluded in not using it
> for now on this controller, we can as well transfer the complete page
> (page_sz). Or is there another case in which vf610_nfc_write_buf could
> be called with less than page_sz?
I'll leave that up to you. I'm perfectly fine leaving it in, now that I
see its proper use. Just in case things change in the future, I think it
does help to clarify the flow of information a little. Although, I might
recommend a change in naming, since it could get confused with the
actual page size -- which is normally constant -- whereas this field
changes dynamically depending on the command-in-flight.
Perhaps the struct could have 'write_len' (to help represent an action)
and the local variable in vf610_nfc_command() could be 'tfr_len' (to
distinguish how it isn't necessarily identical to 'write_len')? Just
throwing (likely bad) ideas out there.
Regards,
Brian
On 2015-08-27 09:34, Brian Norris wrote:
> On Wed, Aug 26, 2015 at 06:02:31PM -0700, Stefan Agner wrote:
>> On 2015-08-25 13:16, Brian Norris wrote:
>> > On Mon, Aug 03, 2015 at 11:27:26AM +0200, Stefan Agner wrote:
>> >> diff --git a/drivers/mtd/nand/vf610_nfc.c b/drivers/mtd/nand/vf610_nfc.c
>> >> new file mode 100644
>> >> index 0000000..5c8dfe8
>> >> --- /dev/null
>> >> +++ b/drivers/mtd/nand/vf610_nfc.c
>> >> @@ -0,0 +1,645 @@
>> >
>> > ...
>> >
>> >> +/*
>> >> + * This function supports Vybrid only (MPC5125 would have full RB and four CS)
>> >> + */
>> >> +static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip)
>> >> +{
>> >> +#ifdef CONFIG_SOC_VF610
>> >
>> > Why the #ifdef? I don't see anything compile-time specific to SOC_VF610.
>> >
>> > If this is trying to handle the comment above ("This function supports
>> > Vybrid only (MPC5125 would have full RB and four CS)") then that's the
>> > wrong way of doing it, as you need to support multiplatform kernels.
>> > You'll need to have a way to differentiate the different platform
>> > support at runtime, not compile time.
>>
>> Yes it is trying to handle the comment above. Well, the other two
>> platforms I am aware of are also different architectures... (PowerPC and
>> ColdFire). I think we won't have a multi-architecture kernel anytime
>> soon,
>
> Ha, right. Sorry, I don't really know this particular IP.
>
>> hence I think removing the code at compile time is the right thing
>> todo.
>
> I don't believe that conclusion follows though.
>
>> However, probably CONFIG_SOC_VF610 is the wrong symbol then, I could
>> just use CONFIG_ARM and add a comment that this might be different on
>> another other ARM SoC than VF610.
>>
>> Just checked CodingStyle, and I see that IS_ENABLED is the preferred way
>> for conditional compiling.
>>
>> So my suggestion:
>>
>> static void vf610_nfc_select_chip(struct mtd_info *mtd, int chip)
>> {
>> struct vf610_nfc *nfc = mtd_to_nfc(mtd);
>> u32 tmp = vf610_nfc_read(nfc, NFC_ROW_ADDR);
>>
>> if (!IS_ENABLED(CONFIG_ARM))
>> return;
>>
>> /*
>> * This code is only tested on the ARM platform VF610
>> * PowerPC based MPC5125 would have full RB and four CS
>> */
>> ....
>>
>> With that the compiler should be able to remove this (currently) ARM
>> VF610 specific code on the other supported architectures...
>>
>> What do you think?
>
> The code structure isn't bad, and yes, IS_ENABLED() would be preferable,
> as it removes some of the problems with #ifdef, but I still don't think
> the processor architecture has much to do with the version of the IP.
Well yes, the processor architecture has probably not much to do with
the IP version.
However, that particular problem, the wiring up of the CS/RB signals, is
probably more SoC (as a whole) specific, and how that is done might have
some relation which architecture is in use...
I do not have a strong opinion on this, so we might as well go with the
run-time distinction using compatible. If there are different IP
variants within one architecture, we anyway would need to do that.
> The canonical way of distiguishing per-IP revisions is to key on the
> compatible property. So you'd have some kind of enum, which would
> currently only have an entry for VF610. i.e.:
>
> /* MPC5125 not yet supported */
> if (nfc->revision != NAND_VFC610)
> return;
>
Ok, just checked, I can use the data field of the of table to assign
specific data to a compatible string, similar to how pxa3xx_nand.c does
it.
--
Stefan
On 26 Aug 2015, [email protected] wrote:
> On Wed, Aug 26, 2015 at 10:57:38AM -0700, Stefan Agner wrote:
>> When printing the ECC error count on ECC fail when reading an erased
>> NAND flash, the numbers of bit flips (stuck at zero) seem to widely
>> correlate with the number returned by the controller. While it seems
>> to correlate widely, there are exceptions, as discussed in the
>> thread: http://thread.gmane.org/gmane.linux.ports.arm.kernel/295424
>> Maybe this is an artifact of the ECC algorithm we just
>> can't/shouldn't rely on? I am not sure where this originated, I did
>> not found any indication in the reference manual about what that
>> value contains in the error case.
> Doesn't sound too reliable to me. And I'm not sure even if it was
> reliable, that it would provide much value. We have to a lot of
> re-counting anyway, so we might as well just be using our own
> threshold. Or maybe I'm missing the point.
>> Bill, do you have an idea why we used that value as threshold in
>> early implementations?
>> Otherwise I also think we should just drop the use of this value.
Yes, using this value is not especially useful if we re-read with ECC
disabled to count the bit flips for erased pages. I think this is what
Stefan has done in the 11th patch set.
--
Married men live longer than single men, but married men are much more
willing to die - Dilworth