mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc

Signed-off-by Zhou Wang <[email protected]>

---
 drivers/mtd/nand/Kconfig        |    5 +
 drivers/mtd/nand/Makefile       |    1 +
 drivers/mtd/nand/hisi504_nand.c |  846 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 852 insertions(+)
 create mode 100644 drivers/mtd/nand/hisi504_nand.c

drivers/mtd/nand/Kconfig +0 -0

index dd10646..e451a08 100644

drivers/mtd/nand/Makefile +1 -0

index 9c847e4..fb1b2e4 100644
@@ -50,5 +50,6 @@ obj-$(CONFIG_MTD_NAND_JZ4740)		+= jz4740_nand.o
 obj-$(CONFIG_MTD_NAND_GPMI_NAND)	+= gpmi-nand/
 obj-$(CONFIG_MTD_NAND_XWAY)		+= xway_nand.o
 obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH)	+= bcm47xxnflash/
+obj-$(CONFIG_MTD_NAND_HISI504)	        += hisi504_nand.o
 
 nand-objs := nand_base.o nand_bbt.o nand_timings.o

drivers/mtd/nand/hisi504_nand.c new file +63 -0

new file mode 100644
index 0000000..a169cd8
@@ -0,0 +1,846 @@
+/*
+ * Hisilicon NAND Flash controller driver
+ *
+ * Copyright © 2012-2014 HiSilicon Technologies Co., Ltd.
+ *              http://www.hisilicon.com
+ *
+ * Author: Zhou Wang <[email protected]>
+ * The initial developer of the original code is Zhiyong Cai
+ * <[email protected]>
+ *
+ * This program 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/of.h>
+#include <linux/of_mtd.h>
+#include <linux/mtd/mtd.h>
+#include <linux/sizes.h>
+#include <linux/clk.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/mtd/nand.h>
+#include <linux/dma-mapping.h>
+#include <linux/platform_device.h>
+#include <linux/mtd/partitions.h>
+
+#define HINFC504_MAX_CHIP                               (4)
+#define HINFC504_W_LATCH                                (5)
+#define HINFC504_R_LATCH                                (7)
+#define HINFC504_RW_LATCH                               (3)
+
+#define HINFC504_NFC_TIMEOUT				(2 * HZ)
+#define HINFC504_NFC_DMA_TIMEOUT			(5 * HZ)
+#define HINFC504_CHIP_DELAY				(25)
+
+#define HINFC504_REG_BASE_ADDRESS_LEN			(0x100)
+#define HINFC504_BUFFER_BASE_ADDRESS_LEN		(2048 + 128)
+
+#define HINFC504_ADDR_CYCLE_MASK			0x4
+
+#define HINFC504_CON					0x00
+#define HINFC504_CON_OP_MODE_NORMAL			(1U << 0)
+#define HINFC504_CON_PAGEISZE_SHIFT			(1)
+#define HINFC504_CON_PAGESIZE_MASK			(0x07)
+#define HINFC504_CON_BUS_WIDTH				(1U << 4)
+#define HINFC504_CON_READY_BUSY_SEL			(1U << 8)
+#define HINFC504_CON_ECCTYPE_SHIFT			(9)
+#define HINFC504_CON_ECCTYPE_MASK			(0x07)
+
+#define HINFC504_PWIDTH					0x04
+#define SET_HINFC504_PWIDTH(_w_lcnt, _r_lcnt, _rw_hcnt) \
+	((_w_lcnt) | (((_r_lcnt) & 0x0F) << 4) | (((_rw_hcnt) & 0x0F) << 8))
+
+#define HINFC504_CMD					0x0C
+#define HINFC504_ADDRL					0x10

drivers/mtd/nand/hisi504_nand.c new file +50 -0

+
+#define HINFC504_INTS					0x28
+#define HINFC504_INTS_DMA				(1U << 9)
+#define HINFC504_INTS_UE				(1U << 6)
+#define HINFC504_INTS_CE				(1U << 5)
+
+#define HINFC504_INTCLR					0x2C
+#define HINFC504_INTCLR_DMA				(1U << 9)
+#define HINFC504_INTCLR_UE				(1U << 6)
+#define HINFC504_INTCLR_CE				(1U << 5)
+
+#define HINFC504_ECC_STATUS                             0x5C
+#define HINFC504_ECC_1_BIT_SHIFT                        16
+#define HINFC504_ECC_16_BIT_SHIFT                       12
+
+#define HINFC504_DMA_CTRL				0x60
+#define HINFC504_DMA_CTRL_DMA_START			(1U << 0)
+#define HINFC504_DMA_CTRL_WE				(1U << 1)
+#define HINFC504_DMA_CTRL_DATA_AREA_EN			(1U << 2)
+#define HINFC504_DMA_CTRL_OOB_AREA_EN			(1U << 3)
+#define HINFC504_DMA_CTRL_BURST4_EN			(1U << 4)
+#define HINFC504_DMA_CTRL_BURST8_EN			(1U << 5)
+#define HINFC504_DMA_CTRL_BURST16_EN			(1U << 6)
+#define HINFC504_DMA_CTRL_ADDR_NUM_SHIFT		(7)
+#define HINFC504_DMA_CTRL_ADDR_NUM_MASK			(1)
+#define HINFC504_DMA_CTRL_CS_SHIFT			(8)
+#define HINFC504_DMA_CTRL_CS_MASK			(0x03)
+
+#define HINFC504_DMA_ADDR_DATA				0x64
+#define HINFC504_DMA_ADDR_OOB				0x68
+
+#define HINFC504_DMA_LEN				0x6C
+#define HINFC504_DMA_LEN_OOB_SHIFT			(16)
+#define HINFC504_DMA_LEN_OOB_MASK			(0xFFF)
+
+#define HINFC504_DMA_PARA				0x70
+#define HINFC504_DMA_PARA_DATA_RW_EN			(1U << 0)
+#define HINFC504_DMA_PARA_OOB_RW_EN			(1U << 1)
+#define HINFC504_DMA_PARA_DATA_EDC_EN			(1U << 2)
+#define HINFC504_DMA_PARA_OOB_EDC_EN			(1U << 3)
+#define HINFC504_DMA_PARA_DATA_ECC_EN			(1U << 4)
+#define HINFC504_DMA_PARA_OOB_ECC_EN			(1U << 5)
+
+#define HINFC_VERSION                                   0x74
+#define HINFC504_LOG_READ_ADDR				0x7C
+#define HINFC504_LOG_READ_LEN				0x80
+
+#define HINFC504_NANDINFO_LEN				0x10
+
+struct hinfc_host {

drivers/mtd/nand/hisi504_nand.c new file +71 -0

+	unsigned int		cache_addr_value[2];
+	char			*buffer;
+	dma_addr_t		dma_buffer;
+	dma_addr_t		dma_oob;
+	int			version;
+	unsigned int            ecc_bits;
+	unsigned int            irq_status; /* interrupt status */
+};
+
+static inline unsigned int hinfc_read(struct hinfc_host *host, unsigned int reg)
+{
+	return readl(host->iobase + reg);
+}
+
+static inline void hinfc_write(struct hinfc_host *host, unsigned int value,
+			       unsigned int reg)
+{
+	writel(value, host->iobase + reg);
+}
+
+static void wait_controller_finished(struct hinfc_host *host)
+{
+	unsigned long timeout = jiffies + HINFC504_NFC_TIMEOUT;
+	int val;
+
+	while (time_before(jiffies, timeout)) {
+		val = hinfc_read(host, HINFC504_STATUS);
+		if (host->command == NAND_CMD_ERASE2) {
+			/* nfc is ready */
+			while (!(val & HINFC504_READY))	{
+				usleep_range(500, 1000);
+				val = hinfc_read(host, HINFC504_STATUS);
+			}
+			return;
+		}
+
+		if (val & HINFC504_READY)
+			return;
+	}
+
+	/* wait cmd timeout */
+	dev_err(host->dev, "Wait NAND controller exec cmd timeout.\n");
+}
+
+static void hisi_nfc_dma_transfer(struct hinfc_host *host, int todev)
+{
+	struct mtd_info	*mtd = &host->mtd;
+	struct nand_chip *chip = mtd->priv;
+	unsigned long val;
+	int ret;
+
+	hinfc_write(host, host->dma_buffer, HINFC504_DMA_ADDR_DATA);
+	hinfc_write(host, host->dma_oob, HINFC504_DMA_ADDR_OOB);
+
+	if (chip->ecc.mode == NAND_ECC_NONE) {
+		hinfc_write(host, ((mtd->oobsize & HINFC504_DMA_LEN_OOB_MASK)
+			<< HINFC504_DMA_LEN_OOB_SHIFT), HINFC504_DMA_LEN);
+
+		hinfc_write(host, HINFC504_DMA_PARA_DATA_RW_EN
+			| HINFC504_DMA_PARA_OOB_RW_EN, HINFC504_DMA_PARA);
+	} else {
+		hinfc_write(host, HINFC504_DMA_PARA_DATA_RW_EN
+		| HINFC504_DMA_PARA_OOB_RW_EN | HINFC504_DMA_PARA_DATA_EDC_EN
+		| HINFC504_DMA_PARA_OOB_EDC_EN | HINFC504_DMA_PARA_DATA_ECC_EN
+		| HINFC504_DMA_PARA_OOB_ECC_EN, HINFC504_DMA_PARA);
+	}
+
+	val = (HINFC504_DMA_CTRL_DMA_START | HINFC504_DMA_CTRL_BURST4_EN
+		| HINFC504_DMA_CTRL_BURST8_EN | HINFC504_DMA_CTRL_BURST16_EN
+		| HINFC504_DMA_CTRL_DATA_AREA_EN | HINFC504_DMA_CTRL_OOB_AREA_EN
+		| ((host->addr_cycle == 4 ? 1 : 0)

drivers/mtd/nand/hisi504_nand.c new file +173 -0

+		else
+			dev_err(host->dev, "dma is really timeout!");
+	}
+}
+
+static int hisi_nfc_send_cmd_pageprog(struct hinfc_host *host)
+{
+	host->addr_value[0] &= 0xffff0000;
+
+	hinfc_write(host, host->addr_value[0], HINFC504_ADDRL);
+	hinfc_write(host, host->addr_value[1], HINFC504_ADDRH);
+	hinfc_write(host, NAND_CMD_PAGEPROG << 8 | NAND_CMD_SEQIN,
+		    HINFC504_CMD);
+
+	hisi_nfc_dma_transfer(host, 1);
+
+	return 0;
+}
+
+static int hisi_nfc_send_cmd_readstart(struct hinfc_host *host)
+{
+	struct mtd_info	*mtd = &host->mtd;
+
+	if ((host->addr_value[0] == host->cache_addr_value[0]) &&
+	    (host->addr_value[1] == host->cache_addr_value[1]))
+		return 0;
+
+	host->addr_value[0] &= 0xffff0000;
+
+	hinfc_write(host, host->addr_value[0], HINFC504_ADDRL);
+	hinfc_write(host, host->addr_value[1], HINFC504_ADDRH);
+	hinfc_write(host, NAND_CMD_READSTART << 8 | NAND_CMD_READ0,
+		    HINFC504_CMD);
+
+	hinfc_write(host, 0, HINFC504_LOG_READ_ADDR);
+	hinfc_write(host, mtd->writesize + mtd->oobsize,
+		    HINFC504_LOG_READ_LEN);
+
+	hisi_nfc_dma_transfer(host, 0);
+
+	host->cache_addr_value[0] = host->addr_value[0];
+	host->cache_addr_value[1] = host->addr_value[1];
+
+	return 0;
+}
+
+static int hisi_nfc_send_cmd_erase(struct hinfc_host *host)
+{
+	hinfc_write(host, host->addr_value[0], HINFC504_ADDRL);
+	hinfc_write(host, (NAND_CMD_ERASE2 << 8) | NAND_CMD_ERASE1,
+		    HINFC504_CMD);
+
+	hinfc_write(host, HINFC504_OP_WAIT_READY_EN
+		| HINFC504_OP_CMD2_EN
+		| HINFC504_OP_CMD1_EN
+		| HINFC504_OP_ADDR_EN
+		| ((host->chipselect & HINFC504_OP_NF_CS_MASK)
+			<< HINFC504_OP_NF_CS_SHIFT)
+		| ((host->addr_cycle & HINFC504_OP_ADDR_CYCLE_MASK)
+			<< HINFC504_OP_ADDR_CYCLE_SHIFT),
+		HINFC504_OP);
+
+	wait_controller_finished(host);
+
+	return 0;
+}
+
+static int hisi_nfc_send_cmd_readid(struct hinfc_host *host)
+{
+	hinfc_write(host, HINFC504_NANDINFO_LEN, HINFC504_DATA_NUM);
+	hinfc_write(host, NAND_CMD_READID, HINFC504_CMD);
+	hinfc_write(host, 0, HINFC504_ADDRL);
+
+	hinfc_write(host, HINFC504_OP_CMD1_EN | HINFC504_OP_ADDR_EN
+		| HINFC504_OP_READ_DATA_EN
+		| ((host->chipselect & HINFC504_OP_NF_CS_MASK)
+			<< HINFC504_OP_NF_CS_SHIFT)
+		| 1 << HINFC504_OP_ADDR_CYCLE_SHIFT, HINFC504_OP);
+
+	wait_controller_finished(host);
+
+	return 0;
+}
+
+static int hisi_nfc_send_cmd_status(struct hinfc_host *host)
+{
+	hinfc_write(host, HINFC504_NANDINFO_LEN, HINFC504_DATA_NUM);
+	hinfc_write(host, NAND_CMD_STATUS, HINFC504_CMD);
+	hinfc_write(host, HINFC504_OP_CMD1_EN
+		| HINFC504_OP_READ_DATA_EN
+		| ((host->chipselect & HINFC504_OP_NF_CS_MASK)
+			<< HINFC504_OP_NF_CS_SHIFT),
+		HINFC504_OP);
+
+	wait_controller_finished(host);
+
+	return 0;
+}
+
+static int hisi_nfc_send_cmd_reset(struct hinfc_host *host, int chipselect)
+{
+	hinfc_write(host, NAND_CMD_RESET, HINFC504_CMD);
+
+	hinfc_write(host, HINFC504_OP_CMD1_EN
+		| ((chipselect & HINFC504_OP_NF_CS_MASK)
+			<< HINFC504_OP_NF_CS_SHIFT)
+		| HINFC504_OP_WAIT_READY_EN,
+		HINFC504_OP);
+
+	wait_controller_finished(host);
+
+	return 0;
+}
+
+static void hisi_nfc_select_chip(struct mtd_info *mtd, int chipselect)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct hinfc_host *host = chip->priv;
+
+	if (chipselect < 0)
+		return;
+
+	host->chipselect = chipselect;
+}
+
+static uint8_t hisi_nfc_read_byte(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct hinfc_host *host = chip->priv;
+
+	if (host->command == NAND_CMD_STATUS)
+		return readb(chip->IO_ADDR_R);
+
+	host->offset++;
+
+	if (host->command == NAND_CMD_READID)
+		return readb(chip->IO_ADDR_R + host->offset - 1);
+
+	return readb(host->buffer + host->offset - 1);
+}
+
+static u16 hisi_nfc_read_word(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct hinfc_host *host = chip->priv;
+
+	host->offset += 2;
+	return readw(host->buffer + host->offset - 2);
+}
+
+static void
+hisi_nfc_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct hinfc_host *host = chip->priv;
+
+	memcpy(host->buffer + host->offset, buf, len);
+	host->offset += len;
+}
+
+static void hisi_nfc_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct hinfc_host *host = chip->priv;
+
+	memcpy(buf, host->buffer + host->offset, len);
+	host->offset += len;
+}
+
+static void set_addr(struct mtd_info *mtd, int column, int page_addr)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct hinfc_host *host = chip->priv;

drivers/mtd/nand/hisi504_nand.c new file +280 -0

+
+		host->addr_value[0] = column & 0xffff;
+		host->addr_cycle    = 2;
+	}
+	if (page_addr != -1) {
+		host->addr_value[0] |= (page_addr & 0xffff)
+			<< (host->addr_cycle * 8);
+		host->addr_cycle    += 2;
+		/* One more address cycle for devices > 128MiB */
+		if (chip->chipsize > (128 << 20)) {
+			host->addr_cycle += 1;
+			if (host->command == NAND_CMD_ERASE1)
+				host->addr_value[0] |= ((page_addr >> 16) & 0xff) << 16;
+			else
+				host->addr_value[1] |= ((page_addr >> 16) & 0xff);
+		}
+	}
+}
+
+static void hisi_nfc_cmdfunc(struct mtd_info *mtd, unsigned command, int column,
+		int page_addr)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct hinfc_host *host = chip->priv;
+	int is_cache_invalid = 1;
+	unsigned int flag = 0;
+
+	host->command =  command;
+
+	switch (command) {
+	case NAND_CMD_READ0:
+	case NAND_CMD_READOOB:
+		if (command == NAND_CMD_READ0)
+			host->offset = column;
+		else
+			host->offset = column + mtd->writesize;
+
+		is_cache_invalid = 0;
+		set_addr(mtd, column, page_addr);
+		hisi_nfc_send_cmd_readstart(host);
+		break;
+
+	case NAND_CMD_SEQIN:
+		host->offset = column;
+		set_addr(mtd, column, page_addr);
+		break;
+
+	case NAND_CMD_ERASE1:
+		set_addr(mtd, column, page_addr);
+		break;
+
+	case NAND_CMD_PAGEPROG:
+		hisi_nfc_send_cmd_pageprog(host);
+		break;
+
+	case NAND_CMD_ERASE2:
+		hisi_nfc_send_cmd_erase(host);
+		break;
+
+	case NAND_CMD_READID:
+		host->offset = column;
+		memset(chip->IO_ADDR_R, 0, 0x10);
+		hisi_nfc_send_cmd_readid(host);
+		break;
+
+	case NAND_CMD_STATUS:
+		flag = hinfc_read(host, HINFC504_CON);
+		if (chip->ecc.mode == NAND_ECC_HW)
+			hinfc_write(host,
+				    flag && ~(HINFC504_CON_ECCTYPE_MASK <<
+				    HINFC504_CON_ECCTYPE_SHIFT), HINFC504_CON);
+
+		host->offset = 0;
+		memset(chip->IO_ADDR_R, 0, 0x10);
+		hisi_nfc_send_cmd_status(host);
+		hinfc_write(host, flag, HINFC504_CON);
+		break;
+
+	case NAND_CMD_RESET:
+		hisi_nfc_send_cmd_reset(host, host->chipselect);
+		break;
+
+	default:
+		dev_err(host->dev, "Error: unsupported cmd(cmd=%x, col=%x, page=%x)\n",
+			command, column, page_addr);
+	}
+
+	if (is_cache_invalid) {
+		host->cache_addr_value[0] = ~0;
+		host->cache_addr_value[1] = ~0;
+	}
+}
+
+static irqreturn_t hinfc_irq_handle(int irq, void *devid)
+{
+	struct hinfc_host *host = devid;
+	unsigned int flag;
+
+	flag = hinfc_read(host, HINFC504_INTS);
+	/* store interrupts state */
+	host->irq_status |= flag;
+
+	if (flag & HINFC504_INTS_DMA) {
+		hinfc_write(host, HINFC504_INTCLR_DMA, HINFC504_INTCLR);
+		complete(&host->cmd_complete);
+	} else if (flag & HINFC504_INTS_CE) {
+		hinfc_write(host, HINFC504_INTCLR_CE, HINFC504_INTCLR);
+	} else if (flag & HINFC504_INTS_UE) {
+		hinfc_write(host, HINFC504_INTCLR_UE, HINFC504_INTCLR);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int hisi_nand_read_page_hwecc(struct mtd_info *mtd,
+	struct nand_chip *chip, uint8_t *buf, int oob_required, int page)
+{
+	struct hinfc_host *host = chip->priv;
+	int max_bitflips = 0, stat = 0;
+
+	chip->read_buf(mtd, buf, mtd->writesize);
+	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
+
+	/* errors which can not be corrected by ECC */
+	if (host->irq_status & HINFC504_INTS_UE) {
+		mtd->ecc_stats.failed++;
+	} else if (host->irq_status & HINFC504_INTS_CE) {
+		/* need add other ECC modes! */
+		switch (host->ecc_bits) {
+		case 1:
+			stat = hweight8(hinfc_read(host, HINFC504_ECC_STATUS)>>
+						HINFC504_ECC_1_BIT_SHIFT);
+			break;
+		case 6:
+			stat = hweight16(hinfc_read(host, HINFC504_ECC_STATUS)>>
+			HINFC504_ECC_16_BIT_SHIFT & 0x0fff);
+		}
+		mtd->ecc_stats.corrected += stat;
+		max_bitflips = max_t(unsigned int, max_bitflips, stat);
+	}
+	host->irq_status = 0;
+
+	return max_bitflips;
+}
+
+static int hisi_nand_write_page_hwecc(struct mtd_info *mtd,
+		struct nand_chip *chip, const uint8_t *buf, int oob_required)
+{
+	chip->write_buf(mtd, buf, mtd->writesize);
+	if (oob_required)
+		chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
+
+	return 0;
+}
+
+static void hisi_nfc_host_init(struct hinfc_host *host)
+{
+	struct nand_chip *chip = &host->chip;
+	unsigned int flag = 0;
+
+	host->version = hinfc_read(host, HINFC_VERSION);
+	host->addr_cycle		= 0;
+	host->addr_value[0]		= 0;
+	host->addr_value[1]		= 0;
+	host->cache_addr_value[0]	= ~0;
+	host->cache_addr_value[1]	= ~0;
+	host->chipselect		= 0;
+
+	/* default page size: 2K, ecc_none. need modify */
+	flag = HINFC504_CON_OP_MODE_NORMAL | HINFC504_CON_READY_BUSY_SEL
+		| ((0x001 & HINFC504_CON_PAGESIZE_MASK)
+			<< HINFC504_CON_PAGEISZE_SHIFT)
+		| ((0x0 & HINFC504_CON_ECCTYPE_MASK)
+			<< HINFC504_CON_ECCTYPE_SHIFT)
+		| ((chip->options & NAND_BUSWIDTH_16) ?
+			HINFC504_CON_BUS_WIDTH : 0);
+	hinfc_write(host, flag, HINFC504_CON);
+
+	memset(chip->IO_ADDR_R, 0xff, HINFC504_BUFFER_BASE_ADDRESS_LEN);
+
+	hinfc_write(host, SET_HINFC504_PWIDTH(HINFC504_W_LATCH,
+		    HINFC504_R_LATCH, HINFC504_RW_LATCH), HINFC504_PWIDTH);
+
+	/* enable dma irq */
+	hinfc_write(host, HINFC504_INTEN_DMA, HINFC504_INTEN);
+}
+
+static struct nand_ecclayout nand_ecc_2K_1bit = {
+	.oobfree = { {24, 40} }
+};
+
+static struct nand_ecclayout nand_ecc_2K_16bits = {
+	.oobavail = 6,
+	.oobfree = { {2, 6} },
+};
+
+static int hisi_nfc_ecc_probe(struct hinfc_host *host)
+{
+	struct nand_chip *chip = &host->chip;
+	unsigned int flag;
+
+	chip->ecc.read_page = hisi_nand_read_page_hwecc;
+	chip->ecc.write_page = hisi_nand_write_page_hwecc;
+
+	switch (host->ecc_bits) {
+	case 1:
+		chip->ecc.layout = &nand_ecc_2K_1bit;
+		chip->ecc.strength = 1;
+		chip->ecc.size = 512;
+		break;
+	case 6:
+		chip->ecc.layout = &nand_ecc_2K_16bits;
+		chip->ecc.strength = 16;
+		chip->ecc.size = 1024;
+	}
+
+	flag = hinfc_read(host, HINFC504_CON);
+	/* add ecc type configure */
+	flag |= ((host->ecc_bits & HINFC504_CON_ECCTYPE_MASK)
+						<< HINFC504_CON_ECCTYPE_SHIFT);
+	hinfc_write(host, flag, HINFC504_CON);
+
+	/* enable ecc irq */
+	flag = hinfc_read(host, HINFC504_INTEN) & 0xfff;
+	hinfc_write(host, flag | HINFC504_INTEN_UE | HINFC504_INTEN_CE,
+		    HINFC504_INTEN);
+
+	return 0;
+}
+
+static int hisi_nfc_probe(struct platform_device *pdev)
+{
+	int ret = 0, irq, buswidth, flag, max_chips = HINFC504_MAX_CHIP;
+	struct device *dev = &pdev->dev;
+	struct hinfc_host *host;
+	struct nand_chip  *chip;
+	struct mtd_info   *mtd;
+	struct resource	  *res;
+	struct device_node *np = dev->of_node;
+	struct mtd_part_parser_data ppdata;
+
+	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
+	if (!host)
+		return -ENOMEM;
+	host->dev = dev;
+
+	platform_set_drvdata(pdev, host);
+	chip = &host->chip;
+	mtd  = &host->mtd;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "no IRQ resource defined\n");
+		ret = -ENXIO;
+		goto err_res;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	host->iobase = devm_ioremap_resource(dev, res);
+	if (IS_ERR(host->iobase)) {
+		ret = PTR_ERR(host->iobase);
+		dev_err(dev, "devm_ioremap_resource[0] fail\n");
+		goto err_res;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	chip->IO_ADDR_R = chip->IO_ADDR_W = devm_ioremap_resource(dev, res);
+	if (IS_ERR(chip->IO_ADDR_R)) {
+		ret = PTR_ERR(chip->IO_ADDR_R);
+		dev_err(dev, "devm_ioremap_resource[1] fail\n");
+		goto err_res;
+	}
+
+	mtd->priv		= chip;
+	mtd->owner		= THIS_MODULE;
+	mtd->name		= "hisi_nand";
+	mtd->dev.parent         = &pdev->dev;
+
+	chip->priv		= host;
+	chip->cmdfunc		= hisi_nfc_cmdfunc;

drivers/mtd/nand/hisi504_nand.c new file +17 -0

+	if (host->ecc_bits != 0 && host->ecc_bits != 1 && host->ecc_bits != 6) {
+		ret = -EINVAL;
+		dev_err(dev, "invalid nand-ecc-bits: %u\n", host->ecc_bits);
+		goto err_res;
+	}
+
+	buswidth = of_get_nand_bus_width(np);
+	if (buswidth == 16)
+		chip->options |= NAND_BUSWIDTH_16;
+
+	hisi_nfc_host_init(host);
+
+	ret = devm_request_irq(dev, irq, hinfc_irq_handle, IRQF_DISABLED,
+				"nandc", host);
+	if (ret) {
+		dev_err(dev, "failed to request IRQ\n");
+		goto err_irq;

drivers/mtd/nand/hisi504_nand.c new file +6 -0

+	if (!host->buffer) {
+		dev_err(dev, "Can't malloc memory for NAND driver.\n");
+		ret = -ENOMEM;
+		goto err_buf;
+	}
+	host->dma_oob = host->dma_buffer + mtd->writesize;

drivers/mtd/nand/hisi504_nand.c new file +4 -0

+
+	ppdata.of_node = np;
+	ret = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0);
+	if (ret) {

drivers/mtd/nand/hisi504_nand.c new file +6 -0

+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int hisi_nfc_suspend(struct platform_device *pdev,

drivers/mtd/nand/hisi504_nand.c new file +32 -0

+
+	return 0;
+}
+
+static int hisi_nfc_resume(struct platform_device *pdev)
+{
+	int cs;
+	struct hinfc_host *host = platform_get_drvdata(pdev);
+	struct nand_chip *chip = &host->chip;
+
+	for (cs = 0; cs < chip->numchips; cs++)
+		hisi_nfc_send_cmd_reset(host, cs);
+	hinfc_write(host, SET_HINFC504_PWIDTH(HINFC504_W_LATCH,
+		    HINFC504_R_LATCH, HINFC504_RW_LATCH), HINFC504_PWIDTH);
+
+	return 0;
+}
+#endif
+static SIMPLE_DEV_PM_OPS(hisi_nfc_pm_ops, hisi_nfc_suspend, hisi_nfc_resume);
+
+static const struct of_device_id nfc_id_table[] = {
+	{ .compatible = "hisilicon,504-nfc" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, nfc_id_table);
+
+static struct platform_driver hisi_nfc_driver = {
+	.driver = {
+		.name  = "hisi_nand",
+		.of_match_table = of_match_ptr(nfc_id_table),
+		.pm = &hisi_nfc_pm_ops,
+	},

Quoted 1 line · Zhou Wang

Signed-off-by Zhou Wang <[email protected]>

This driver mostly looks good. A few comments.

Quoted 15 lines

---
  drivers/mtd/nand/Kconfig        |    5 +
  drivers/mtd/nand/Makefile       |    1 +
  drivers/mtd/nand/hisi504_nand.c |  846 +++++++++++++++++++++++++++++++++++++++
  3 files changed, 852 insertions(+)

create mode 100644 drivers/mtd/nand/hisi504_nand.c

drivers/mtd/nand/Kconfig +0 -0

index dd10646..e451a08 100644
@@ -516,4 +516,9 @@ config MTD_NAND_XWAY
  	  Enables support for NAND Flash chips on Lantiq XWAY SoCs. NAND is attached
  	  to the External Bus Unit (EBU).
 

drivers/mtd/nand/Kconfig config MTD_NAND_XWAY · line 524

Quoted 6 lines

drivers/mtd/nand/Kconfig +5 -0

+config MTD_NAND_HISI504
+	tristate "Support for NAND controller on Hisilicon SoC Hip04"
+	help
+	  Enables support for NAND controller on Hisilicon SoC Hip04.
+
  endif # MTD_NAND

You'll want to rebase on l2-mtd.git when you resend. There are some small Kconfig and Makefile conflicts. (I can fix them myself, but if you're going to resend anyway...)

Quoted 84 lines

drivers/mtd/nand/Makefile +1 -0

index 9c847e4..fb1b2e4 100644
@@ -50,5 +50,6 @@ obj-$(CONFIG_MTD_NAND_JZ4740)		+= jz4740_nand.o
  obj-$(CONFIG_MTD_NAND_GPMI_NAND)	+= gpmi-nand/
  obj-$(CONFIG_MTD_NAND_XWAY)		+= xway_nand.o
  obj-$(CONFIG_MTD_NAND_BCM47XXNFLASH)	+= bcm47xxnflash/
+obj-$(CONFIG_MTD_NAND_HISI504)	        += hisi504_nand.o
 
  nand-objs := nand_base.o nand_bbt.o nand_timings.o

drivers/mtd/nand/hisi504_nand.c new file +67 -0

new file mode 100644
index 0000000..a169cd8
@@ -0,0 +1,846 @@
+/*
+ * Hisilicon NAND Flash controller driver
+ *
+ * Copyright © 2012-2014 HiSilicon Technologies Co., Ltd.
+ *              http://www.hisilicon.com
+ *
+ * Author: Zhou Wang <[email protected]>
+ * The initial developer of the original code is Zhiyong Cai
+ * <[email protected]>
+ *
+ * This program 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+#include <linux/of.h>
+#include <linux/of_mtd.h>
+#include <linux/mtd/mtd.h>
+#include <linux/sizes.h>
+#include <linux/clk.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/interrupt.h>
+#include <linux/mtd/nand.h>
+#include <linux/dma-mapping.h>
+#include <linux/platform_device.h>
+#include <linux/mtd/partitions.h>
+
+#define HINFC504_MAX_CHIP                               (4)
+#define HINFC504_W_LATCH                                (5)
+#define HINFC504_R_LATCH                                (7)
+#define HINFC504_RW_LATCH                               (3)
+
+#define HINFC504_NFC_TIMEOUT				(2 * HZ)
+#define HINFC504_NFC_DMA_TIMEOUT			(5 * HZ)
+#define HINFC504_CHIP_DELAY				(25)
+
+#define HINFC504_REG_BASE_ADDRESS_LEN			(0x100)
+#define HINFC504_BUFFER_BASE_ADDRESS_LEN		(2048 + 128)
+
+#define HINFC504_ADDR_CYCLE_MASK			0x4
+
+#define HINFC504_CON					0x00
+#define HINFC504_CON_OP_MODE_NORMAL			(1U << 0)
+#define HINFC504_CON_PAGEISZE_SHIFT			(1)
+#define HINFC504_CON_PAGESIZE_MASK			(0x07)
+#define HINFC504_CON_BUS_WIDTH				(1U << 4)
+#define HINFC504_CON_READY_BUSY_SEL			(1U << 8)
+#define HINFC504_CON_ECCTYPE_SHIFT			(9)
+#define HINFC504_CON_ECCTYPE_MASK			(0x07)
+
+#define HINFC504_PWIDTH					0x04
+#define SET_HINFC504_PWIDTH(_w_lcnt, _r_lcnt, _rw_hcnt) \
+	((_w_lcnt) | (((_r_lcnt) & 0x0F) << 4) | (((_rw_hcnt) & 0x0F) << 8))
+
+#define HINFC504_CMD					0x0C
+#define HINFC504_ADDRL					0x10
+#define HINFC504_ADDRH					0x14
+#define HINFC504_DATA_NUM				0x18
+
+#define HINFC504_OP					0x1C

drivers/mtd/nand/hisi504_nand.c line 73

Quoted 6 lines

drivers/mtd/nand/hisi504_nand.c new file +6 -0

+#define HINFC504_OP_READ_DATA_EN			(1U << 1)
+#define HINFC504_OP_WAIT_READY_EN			(1U << 2)
+#define HINFC504_OP_CMD2_EN				(1U << 3)
+#define HINFC504_OP_WRITE_DATA_EN			(1U << 4)
+#define HINFC504_OP_ADDR_EN				(1U << 5)
+#define HINFC504_OP_CMD1_EN				(1U << 6)

Might try the BIT() macros here.

drivers/mtd/nand/hisi504_nand.c line 85

Quoted 13 lines

drivers/mtd/nand/hisi504_nand.c new file +12 -0

+#define HINFC504_OP_NF_CS_SHIFT				(7)
+#define HINFC504_OP_NF_CS_MASK				(3)
+#define HINFC504_OP_ADDR_CYCLE_SHIFT			(9)
+#define HINFC504_OP_ADDR_CYCLE_MASK			(7)
+
+#define HINFC504_STATUS					0x20
+#define HINFC504_READY					(1U << 0)
+
+#define HINFC504_INTEN					0x24
+#define HINFC504_INTEN_DMA				(1U << 9)
+#define HINFC504_INTEN_UE				(1U << 6)
+#define HINFC504_INTEN_CE				(1U << 5)

Same here, and in the next few blocks.

Quoted 56 lines

drivers/mtd/nand/hisi504_nand.c new file +56 -0

+
+#define HINFC504_INTS					0x28
+#define HINFC504_INTS_DMA				(1U << 9)
+#define HINFC504_INTS_UE				(1U << 6)
+#define HINFC504_INTS_CE				(1U << 5)
+
+#define HINFC504_INTCLR					0x2C
+#define HINFC504_INTCLR_DMA				(1U << 9)
+#define HINFC504_INTCLR_UE				(1U << 6)
+#define HINFC504_INTCLR_CE				(1U << 5)
+
+#define HINFC504_ECC_STATUS                             0x5C
+#define HINFC504_ECC_1_BIT_SHIFT                        16
+#define HINFC504_ECC_16_BIT_SHIFT                       12
+
+#define HINFC504_DMA_CTRL				0x60
+#define HINFC504_DMA_CTRL_DMA_START			(1U << 0)
+#define HINFC504_DMA_CTRL_WE				(1U << 1)
+#define HINFC504_DMA_CTRL_DATA_AREA_EN			(1U << 2)
+#define HINFC504_DMA_CTRL_OOB_AREA_EN			(1U << 3)
+#define HINFC504_DMA_CTRL_BURST4_EN			(1U << 4)
+#define HINFC504_DMA_CTRL_BURST8_EN			(1U << 5)
+#define HINFC504_DMA_CTRL_BURST16_EN			(1U << 6)
+#define HINFC504_DMA_CTRL_ADDR_NUM_SHIFT		(7)
+#define HINFC504_DMA_CTRL_ADDR_NUM_MASK			(1)
+#define HINFC504_DMA_CTRL_CS_SHIFT			(8)
+#define HINFC504_DMA_CTRL_CS_MASK			(0x03)
+
+#define HINFC504_DMA_ADDR_DATA				0x64
+#define HINFC504_DMA_ADDR_OOB				0x68
+
+#define HINFC504_DMA_LEN				0x6C
+#define HINFC504_DMA_LEN_OOB_SHIFT			(16)
+#define HINFC504_DMA_LEN_OOB_MASK			(0xFFF)
+
+#define HINFC504_DMA_PARA				0x70
+#define HINFC504_DMA_PARA_DATA_RW_EN			(1U << 0)
+#define HINFC504_DMA_PARA_OOB_RW_EN			(1U << 1)
+#define HINFC504_DMA_PARA_DATA_EDC_EN			(1U << 2)
+#define HINFC504_DMA_PARA_OOB_EDC_EN			(1U << 3)
+#define HINFC504_DMA_PARA_DATA_ECC_EN			(1U << 4)
+#define HINFC504_DMA_PARA_OOB_ECC_EN			(1U << 5)
+
+#define HINFC_VERSION                                   0x74
+#define HINFC504_LOG_READ_ADDR				0x7C
+#define HINFC504_LOG_READ_LEN				0x80
+
+#define HINFC504_NANDINFO_LEN				0x10
+
+struct hinfc_host {
+	struct nand_chip	chip;
+	struct mtd_info		mtd;
+	struct device		*dev;
+	void __iomem		*iobase;
+	struct completion       cmd_complete;
+	unsigned int		offset;

drivers/mtd/nand/hisi504_nand.c line 145

Quoted 4 lines

drivers/mtd/nand/hisi504_nand.c new file +4 -0

+	unsigned int		command;
+	int			chipselect;
+	unsigned int		addr_cycle;
+	unsigned int		addr_value[2];

It looks like you're using addr_value as 32-bit copies of the HW register. Might make sense to declare them as u32, since you care about the width.

Quoted 76 lines

drivers/mtd/nand/hisi504_nand.c new file +76 -0

+	unsigned int		cache_addr_value[2];
+	char			*buffer;
+	dma_addr_t		dma_buffer;
+	dma_addr_t		dma_oob;
+	int			version;
+	unsigned int            ecc_bits;
+	unsigned int            irq_status; /* interrupt status */
+};
+
+static inline unsigned int hinfc_read(struct hinfc_host *host, unsigned int reg)
+{
+	return readl(host->iobase + reg);
+}
+
+static inline void hinfc_write(struct hinfc_host *host, unsigned int value,
+			       unsigned int reg)
+{
+	writel(value, host->iobase + reg);
+}
+
+static void wait_controller_finished(struct hinfc_host *host)
+{
+	unsigned long timeout = jiffies + HINFC504_NFC_TIMEOUT;
+	int val;
+
+	while (time_before(jiffies, timeout)) {
+		val = hinfc_read(host, HINFC504_STATUS);
+		if (host->command == NAND_CMD_ERASE2) {
+			/* nfc is ready */
+			while (!(val & HINFC504_READY))	{
+				usleep_range(500, 1000);
+				val = hinfc_read(host, HINFC504_STATUS);
+			}
+			return;
+		}
+
+		if (val & HINFC504_READY)
+			return;
+	}
+
+	/* wait cmd timeout */
+	dev_err(host->dev, "Wait NAND controller exec cmd timeout.\n");
+}
+
+static void hisi_nfc_dma_transfer(struct hinfc_host *host, int todev)
+{
+	struct mtd_info	*mtd = &host->mtd;
+	struct nand_chip *chip = mtd->priv;
+	unsigned long val;
+	int ret;
+
+	hinfc_write(host, host->dma_buffer, HINFC504_DMA_ADDR_DATA);
+	hinfc_write(host, host->dma_oob, HINFC504_DMA_ADDR_OOB);
+
+	if (chip->ecc.mode == NAND_ECC_NONE) {
+		hinfc_write(host, ((mtd->oobsize & HINFC504_DMA_LEN_OOB_MASK)
+			<< HINFC504_DMA_LEN_OOB_SHIFT), HINFC504_DMA_LEN);
+
+		hinfc_write(host, HINFC504_DMA_PARA_DATA_RW_EN
+			| HINFC504_DMA_PARA_OOB_RW_EN, HINFC504_DMA_PARA);
+	} else {
+		hinfc_write(host, HINFC504_DMA_PARA_DATA_RW_EN
+		| HINFC504_DMA_PARA_OOB_RW_EN | HINFC504_DMA_PARA_DATA_EDC_EN
+		| HINFC504_DMA_PARA_OOB_EDC_EN | HINFC504_DMA_PARA_DATA_ECC_EN
+		| HINFC504_DMA_PARA_OOB_ECC_EN, HINFC504_DMA_PARA);
+	}
+
+	val = (HINFC504_DMA_CTRL_DMA_START | HINFC504_DMA_CTRL_BURST4_EN
+		| HINFC504_DMA_CTRL_BURST8_EN | HINFC504_DMA_CTRL_BURST16_EN
+		| HINFC504_DMA_CTRL_DATA_AREA_EN | HINFC504_DMA_CTRL_OOB_AREA_EN
+		| ((host->addr_cycle == 4 ? 1 : 0)
+			<< HINFC504_DMA_CTRL_ADDR_NUM_SHIFT)
+		| ((host->chipselect & HINFC504_DMA_CTRL_CS_MASK)
+			<< HINFC504_DMA_CTRL_CS_SHIFT));
+
+	if (todev)

drivers/mtd/nand/hisi504_nand.c line 226

Quoted 5 lines

drivers/mtd/nand/hisi504_nand.c new file +5 -0

+		val |= HINFC504_DMA_CTRL_WE;
+
+	hinfc_write(host, val, HINFC504_DMA_CTRL);
+
+	init_completion(&host->cmd_complete);

You need to run init_completion() *before* you kick off your I/O. Otherwise, your interrupt handler is racing with this function.

drivers/mtd/nand/hisi504_nand.c line 235

Quoted 9 lines

drivers/mtd/nand/hisi504_nand.c new file +9 -0

+	ret = wait_for_completion_timeout(&host->cmd_complete,
+			HINFC504_NFC_DMA_TIMEOUT);
+
+	if (!ret) {
+		dev_err(host->dev, "DMA operation(irq) timeout!\n");
+		/* sanity check */
+		val = hinfc_read(host, HINFC504_DMA_CTRL);
+		if (!(val & HINFC504_DMA_CTRL_DMA_START))
+			dev_err(host->dev, "dma is already done but without irq ACK!");

You're missing a newline at the end of this message. Please check your other prints too.

Also, s/dma/DMA/

Quoted 180 lines

drivers/mtd/nand/hisi504_nand.c new file +179 -0

+		else
+			dev_err(host->dev, "dma is really timeout!");
+	}
+}
+
+static int hisi_nfc_send_cmd_pageprog(struct hinfc_host *host)
+{
+	host->addr_value[0] &= 0xffff0000;
+
+	hinfc_write(host, host->addr_value[0], HINFC504_ADDRL);
+	hinfc_write(host, host->addr_value[1], HINFC504_ADDRH);
+	hinfc_write(host, NAND_CMD_PAGEPROG << 8 | NAND_CMD_SEQIN,
+		    HINFC504_CMD);
+
+	hisi_nfc_dma_transfer(host, 1);
+
+	return 0;
+}
+
+static int hisi_nfc_send_cmd_readstart(struct hinfc_host *host)
+{
+	struct mtd_info	*mtd = &host->mtd;
+
+	if ((host->addr_value[0] == host->cache_addr_value[0]) &&
+	    (host->addr_value[1] == host->cache_addr_value[1]))
+		return 0;
+
+	host->addr_value[0] &= 0xffff0000;
+
+	hinfc_write(host, host->addr_value[0], HINFC504_ADDRL);
+	hinfc_write(host, host->addr_value[1], HINFC504_ADDRH);
+	hinfc_write(host, NAND_CMD_READSTART << 8 | NAND_CMD_READ0,
+		    HINFC504_CMD);
+
+	hinfc_write(host, 0, HINFC504_LOG_READ_ADDR);
+	hinfc_write(host, mtd->writesize + mtd->oobsize,
+		    HINFC504_LOG_READ_LEN);
+
+	hisi_nfc_dma_transfer(host, 0);
+
+	host->cache_addr_value[0] = host->addr_value[0];
+	host->cache_addr_value[1] = host->addr_value[1];
+
+	return 0;
+}
+
+static int hisi_nfc_send_cmd_erase(struct hinfc_host *host)
+{
+	hinfc_write(host, host->addr_value[0], HINFC504_ADDRL);
+	hinfc_write(host, (NAND_CMD_ERASE2 << 8) | NAND_CMD_ERASE1,
+		    HINFC504_CMD);
+
+	hinfc_write(host, HINFC504_OP_WAIT_READY_EN
+		| HINFC504_OP_CMD2_EN
+		| HINFC504_OP_CMD1_EN
+		| HINFC504_OP_ADDR_EN
+		| ((host->chipselect & HINFC504_OP_NF_CS_MASK)
+			<< HINFC504_OP_NF_CS_SHIFT)
+		| ((host->addr_cycle & HINFC504_OP_ADDR_CYCLE_MASK)
+			<< HINFC504_OP_ADDR_CYCLE_SHIFT),
+		HINFC504_OP);
+
+	wait_controller_finished(host);
+
+	return 0;
+}
+
+static int hisi_nfc_send_cmd_readid(struct hinfc_host *host)
+{
+	hinfc_write(host, HINFC504_NANDINFO_LEN, HINFC504_DATA_NUM);
+	hinfc_write(host, NAND_CMD_READID, HINFC504_CMD);
+	hinfc_write(host, 0, HINFC504_ADDRL);
+
+	hinfc_write(host, HINFC504_OP_CMD1_EN | HINFC504_OP_ADDR_EN
+		| HINFC504_OP_READ_DATA_EN
+		| ((host->chipselect & HINFC504_OP_NF_CS_MASK)
+			<< HINFC504_OP_NF_CS_SHIFT)
+		| 1 << HINFC504_OP_ADDR_CYCLE_SHIFT, HINFC504_OP);
+
+	wait_controller_finished(host);
+
+	return 0;
+}
+
+static int hisi_nfc_send_cmd_status(struct hinfc_host *host)
+{
+	hinfc_write(host, HINFC504_NANDINFO_LEN, HINFC504_DATA_NUM);
+	hinfc_write(host, NAND_CMD_STATUS, HINFC504_CMD);
+	hinfc_write(host, HINFC504_OP_CMD1_EN
+		| HINFC504_OP_READ_DATA_EN
+		| ((host->chipselect & HINFC504_OP_NF_CS_MASK)
+			<< HINFC504_OP_NF_CS_SHIFT),
+		HINFC504_OP);
+
+	wait_controller_finished(host);
+
+	return 0;
+}
+
+static int hisi_nfc_send_cmd_reset(struct hinfc_host *host, int chipselect)
+{
+	hinfc_write(host, NAND_CMD_RESET, HINFC504_CMD);
+
+	hinfc_write(host, HINFC504_OP_CMD1_EN
+		| ((chipselect & HINFC504_OP_NF_CS_MASK)
+			<< HINFC504_OP_NF_CS_SHIFT)
+		| HINFC504_OP_WAIT_READY_EN,
+		HINFC504_OP);
+
+	wait_controller_finished(host);
+
+	return 0;
+}
+
+static void hisi_nfc_select_chip(struct mtd_info *mtd, int chipselect)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct hinfc_host *host = chip->priv;
+
+	if (chipselect < 0)
+		return;
+
+	host->chipselect = chipselect;
+}
+
+static uint8_t hisi_nfc_read_byte(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct hinfc_host *host = chip->priv;
+
+	if (host->command == NAND_CMD_STATUS)
+		return readb(chip->IO_ADDR_R);
+
+	host->offset++;
+
+	if (host->command == NAND_CMD_READID)
+		return readb(chip->IO_ADDR_R + host->offset - 1);
+
+	return readb(host->buffer + host->offset - 1);
+}
+
+static u16 hisi_nfc_read_word(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct hinfc_host *host = chip->priv;
+
+	host->offset += 2;
+	return readw(host->buffer + host->offset - 2);
+}
+
+static void
+hisi_nfc_write_buf(struct mtd_info *mtd, const uint8_t *buf, int len)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct hinfc_host *host = chip->priv;
+
+	memcpy(host->buffer + host->offset, buf, len);
+	host->offset += len;
+}
+
+static void hisi_nfc_read_buf(struct mtd_info *mtd, uint8_t *buf, int len)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct hinfc_host *host = chip->priv;
+
+	memcpy(buf, host->buffer + host->offset, len);
+	host->offset += len;
+}
+
+static void set_addr(struct mtd_info *mtd, int column, int page_addr)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct hinfc_host *host = chip->priv;
+
+	host->addr_cycle    = 0;
+	host->addr_value[0] = 0;
+	host->addr_value[1] = 0;
+
+	/* Serially input address */

drivers/mtd/nand/hisi504_nand.c line 418

Quoted 4 lines

drivers/mtd/nand/hisi504_nand.c new file +4 -0

+	if (column != -1) {
+		/* Adjust columns for 16 bit buswidth */
+		if (chip->options & NAND_BUSWIDTH_16)
+			column >>= 1;

It doesn't look like you're supporting ONFI parameter pages yet, but you might consider checking for !nand_opcode_8bits(opcode) before adjusting the address. We had to fix some other drivers for this recently.

Quoted 285 lines

drivers/mtd/nand/hisi504_nand.c new file +285 -0

+
+		host->addr_value[0] = column & 0xffff;
+		host->addr_cycle    = 2;
+	}
+	if (page_addr != -1) {
+		host->addr_value[0] |= (page_addr & 0xffff)
+			<< (host->addr_cycle * 8);
+		host->addr_cycle    += 2;
+		/* One more address cycle for devices > 128MiB */
+		if (chip->chipsize > (128 << 20)) {
+			host->addr_cycle += 1;
+			if (host->command == NAND_CMD_ERASE1)
+				host->addr_value[0] |= ((page_addr >> 16) & 0xff) << 16;
+			else
+				host->addr_value[1] |= ((page_addr >> 16) & 0xff);
+		}
+	}
+}
+
+static void hisi_nfc_cmdfunc(struct mtd_info *mtd, unsigned command, int column,
+		int page_addr)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct hinfc_host *host = chip->priv;
+	int is_cache_invalid = 1;
+	unsigned int flag = 0;
+
+	host->command =  command;
+
+	switch (command) {
+	case NAND_CMD_READ0:
+	case NAND_CMD_READOOB:
+		if (command == NAND_CMD_READ0)
+			host->offset = column;
+		else
+			host->offset = column + mtd->writesize;
+
+		is_cache_invalid = 0;
+		set_addr(mtd, column, page_addr);
+		hisi_nfc_send_cmd_readstart(host);
+		break;
+
+	case NAND_CMD_SEQIN:
+		host->offset = column;
+		set_addr(mtd, column, page_addr);
+		break;
+
+	case NAND_CMD_ERASE1:
+		set_addr(mtd, column, page_addr);
+		break;
+
+	case NAND_CMD_PAGEPROG:
+		hisi_nfc_send_cmd_pageprog(host);
+		break;
+
+	case NAND_CMD_ERASE2:
+		hisi_nfc_send_cmd_erase(host);
+		break;
+
+	case NAND_CMD_READID:
+		host->offset = column;
+		memset(chip->IO_ADDR_R, 0, 0x10);
+		hisi_nfc_send_cmd_readid(host);
+		break;
+
+	case NAND_CMD_STATUS:
+		flag = hinfc_read(host, HINFC504_CON);
+		if (chip->ecc.mode == NAND_ECC_HW)
+			hinfc_write(host,
+				    flag && ~(HINFC504_CON_ECCTYPE_MASK <<
+				    HINFC504_CON_ECCTYPE_SHIFT), HINFC504_CON);
+
+		host->offset = 0;
+		memset(chip->IO_ADDR_R, 0, 0x10);
+		hisi_nfc_send_cmd_status(host);
+		hinfc_write(host, flag, HINFC504_CON);
+		break;
+
+	case NAND_CMD_RESET:
+		hisi_nfc_send_cmd_reset(host, host->chipselect);
+		break;
+
+	default:
+		dev_err(host->dev, "Error: unsupported cmd(cmd=%x, col=%x, page=%x)\n",
+			command, column, page_addr);
+	}
+
+	if (is_cache_invalid) {
+		host->cache_addr_value[0] = ~0;
+		host->cache_addr_value[1] = ~0;
+	}
+}
+
+static irqreturn_t hinfc_irq_handle(int irq, void *devid)
+{
+	struct hinfc_host *host = devid;
+	unsigned int flag;
+
+	flag = hinfc_read(host, HINFC504_INTS);
+	/* store interrupts state */
+	host->irq_status |= flag;
+
+	if (flag & HINFC504_INTS_DMA) {
+		hinfc_write(host, HINFC504_INTCLR_DMA, HINFC504_INTCLR);
+		complete(&host->cmd_complete);
+	} else if (flag & HINFC504_INTS_CE) {
+		hinfc_write(host, HINFC504_INTCLR_CE, HINFC504_INTCLR);
+	} else if (flag & HINFC504_INTS_UE) {
+		hinfc_write(host, HINFC504_INTCLR_UE, HINFC504_INTCLR);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int hisi_nand_read_page_hwecc(struct mtd_info *mtd,
+	struct nand_chip *chip, uint8_t *buf, int oob_required, int page)
+{
+	struct hinfc_host *host = chip->priv;
+	int max_bitflips = 0, stat = 0;
+
+	chip->read_buf(mtd, buf, mtd->writesize);
+	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
+
+	/* errors which can not be corrected by ECC */
+	if (host->irq_status & HINFC504_INTS_UE) {
+		mtd->ecc_stats.failed++;
+	} else if (host->irq_status & HINFC504_INTS_CE) {
+		/* need add other ECC modes! */
+		switch (host->ecc_bits) {
+		case 1:
+			stat = hweight8(hinfc_read(host, HINFC504_ECC_STATUS)>>
+						HINFC504_ECC_1_BIT_SHIFT);
+			break;
+		case 6:
+			stat = hweight16(hinfc_read(host, HINFC504_ECC_STATUS)>>
+			HINFC504_ECC_16_BIT_SHIFT & 0x0fff);
+		}
+		mtd->ecc_stats.corrected += stat;
+		max_bitflips = max_t(unsigned int, max_bitflips, stat);
+	}
+	host->irq_status = 0;
+
+	return max_bitflips;
+}
+
+static int hisi_nand_write_page_hwecc(struct mtd_info *mtd,
+		struct nand_chip *chip, const uint8_t *buf, int oob_required)
+{
+	chip->write_buf(mtd, buf, mtd->writesize);
+	if (oob_required)
+		chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
+
+	return 0;
+}
+
+static void hisi_nfc_host_init(struct hinfc_host *host)
+{
+	struct nand_chip *chip = &host->chip;
+	unsigned int flag = 0;
+
+	host->version = hinfc_read(host, HINFC_VERSION);
+	host->addr_cycle		= 0;
+	host->addr_value[0]		= 0;
+	host->addr_value[1]		= 0;
+	host->cache_addr_value[0]	= ~0;
+	host->cache_addr_value[1]	= ~0;
+	host->chipselect		= 0;
+
+	/* default page size: 2K, ecc_none. need modify */
+	flag = HINFC504_CON_OP_MODE_NORMAL | HINFC504_CON_READY_BUSY_SEL
+		| ((0x001 & HINFC504_CON_PAGESIZE_MASK)
+			<< HINFC504_CON_PAGEISZE_SHIFT)
+		| ((0x0 & HINFC504_CON_ECCTYPE_MASK)
+			<< HINFC504_CON_ECCTYPE_SHIFT)
+		| ((chip->options & NAND_BUSWIDTH_16) ?
+			HINFC504_CON_BUS_WIDTH : 0);
+	hinfc_write(host, flag, HINFC504_CON);
+
+	memset(chip->IO_ADDR_R, 0xff, HINFC504_BUFFER_BASE_ADDRESS_LEN);
+
+	hinfc_write(host, SET_HINFC504_PWIDTH(HINFC504_W_LATCH,
+		    HINFC504_R_LATCH, HINFC504_RW_LATCH), HINFC504_PWIDTH);
+
+	/* enable dma irq */
+	hinfc_write(host, HINFC504_INTEN_DMA, HINFC504_INTEN);
+}
+
+static struct nand_ecclayout nand_ecc_2K_1bit = {
+	.oobfree = { {24, 40} }
+};
+
+static struct nand_ecclayout nand_ecc_2K_16bits = {
+	.oobavail = 6,
+	.oobfree = { {2, 6} },
+};
+
+static int hisi_nfc_ecc_probe(struct hinfc_host *host)
+{
+	struct nand_chip *chip = &host->chip;
+	unsigned int flag;
+
+	chip->ecc.read_page = hisi_nand_read_page_hwecc;
+	chip->ecc.write_page = hisi_nand_write_page_hwecc;
+
+	switch (host->ecc_bits) {
+	case 1:
+		chip->ecc.layout = &nand_ecc_2K_1bit;
+		chip->ecc.strength = 1;
+		chip->ecc.size = 512;
+		break;
+	case 6:
+		chip->ecc.layout = &nand_ecc_2K_16bits;
+		chip->ecc.strength = 16;
+		chip->ecc.size = 1024;
+	}
+
+	flag = hinfc_read(host, HINFC504_CON);
+	/* add ecc type configure */
+	flag |= ((host->ecc_bits & HINFC504_CON_ECCTYPE_MASK)
+						<< HINFC504_CON_ECCTYPE_SHIFT);
+	hinfc_write(host, flag, HINFC504_CON);
+
+	/* enable ecc irq */
+	flag = hinfc_read(host, HINFC504_INTEN) & 0xfff;
+	hinfc_write(host, flag | HINFC504_INTEN_UE | HINFC504_INTEN_CE,
+		    HINFC504_INTEN);
+
+	return 0;
+}
+
+static int hisi_nfc_probe(struct platform_device *pdev)
+{
+	int ret = 0, irq, buswidth, flag, max_chips = HINFC504_MAX_CHIP;
+	struct device *dev = &pdev->dev;
+	struct hinfc_host *host;
+	struct nand_chip  *chip;
+	struct mtd_info   *mtd;
+	struct resource	  *res;
+	struct device_node *np = dev->of_node;
+	struct mtd_part_parser_data ppdata;
+
+	host = devm_kzalloc(dev, sizeof(*host), GFP_KERNEL);
+	if (!host)
+		return -ENOMEM;
+	host->dev = dev;
+
+	platform_set_drvdata(pdev, host);
+	chip = &host->chip;
+	mtd  = &host->mtd;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(dev, "no IRQ resource defined\n");
+		ret = -ENXIO;
+		goto err_res;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	host->iobase = devm_ioremap_resource(dev, res);
+	if (IS_ERR(host->iobase)) {
+		ret = PTR_ERR(host->iobase);
+		dev_err(dev, "devm_ioremap_resource[0] fail\n");
+		goto err_res;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	chip->IO_ADDR_R = chip->IO_ADDR_W = devm_ioremap_resource(dev, res);
+	if (IS_ERR(chip->IO_ADDR_R)) {
+		ret = PTR_ERR(chip->IO_ADDR_R);
+		dev_err(dev, "devm_ioremap_resource[1] fail\n");
+		goto err_res;
+	}
+
+	mtd->priv		= chip;
+	mtd->owner		= THIS_MODULE;
+	mtd->name		= "hisi_nand";
+	mtd->dev.parent         = &pdev->dev;
+
+	chip->priv		= host;
+	chip->cmdfunc		= hisi_nfc_cmdfunc;
+	chip->select_chip	= hisi_nfc_select_chip;
+	chip->read_byte		= hisi_nfc_read_byte;
+	chip->read_word		= hisi_nfc_read_word;
+	chip->write_buf		= hisi_nfc_write_buf;
+	chip->read_buf		= hisi_nfc_read_buf;

drivers/mtd/nand/hisi504_nand.c line 708

Quoted 5 lines

drivers/mtd/nand/hisi504_nand.c new file +5 -0

+	chip->chip_delay	= HINFC504_CHIP_DELAY;
+
+	chip->ecc.mode = of_get_nand_ecc_mode(np);
+	/* read ecc-bits from dts */
+	of_property_read_u32(np, "hisi,nand-ecc-bits", &host->ecc_bits);

Replace this with of_get_nand_ecc_step_size() and
of_get_nand_ecc_strength().

Quoted 23 lines

drivers/mtd/nand/hisi504_nand.c new file +22 -0

+	if (host->ecc_bits != 0 && host->ecc_bits != 1 && host->ecc_bits != 6) {
+		ret = -EINVAL;
+		dev_err(dev, "invalid nand-ecc-bits: %u\n", host->ecc_bits);
+		goto err_res;
+	}
+
+	buswidth = of_get_nand_bus_width(np);
+	if (buswidth == 16)
+		chip->options |= NAND_BUSWIDTH_16;
+
+	hisi_nfc_host_init(host);
+
+	ret = devm_request_irq(dev, irq, hinfc_irq_handle, IRQF_DISABLED,
+				"nandc", host);
+	if (ret) {
+		dev_err(dev, "failed to request IRQ\n");
+		goto err_irq;
+	}
+
+	ret = nand_scan_ident(mtd, max_chips, NULL);
+	if (ret) {
+		ret = -ENODEV;

drivers/mtd/nand/hisi504_nand.c line 735

Quoted 5 lines

drivers/mtd/nand/hisi504_nand.c new file +5 -0

+		goto err_ident;
+	}
+
+	host->buffer = dma_alloc_coherent(dev, mtd->writesize + mtd->oobsize,
+		&host->dma_buffer, GFP_KERNEL);

Can you use dmam_alloc_coherent()? That will save you some of the cleanup on error and removal paths.

Quoted 12 lines

drivers/mtd/nand/hisi504_nand.c new file +12 -0

+	if (!host->buffer) {
+		dev_err(dev, "Can't malloc memory for NAND driver.\n");
+		ret = -ENOMEM;
+		goto err_buf;
+	}
+	host->dma_oob = host->dma_buffer + mtd->writesize;
+	memset(host->buffer, 0xff, mtd->writesize + mtd->oobsize);
+
+	flag = hinfc_read(host, HINFC504_CON);
+	flag &= ~(HINFC504_CON_PAGESIZE_MASK << HINFC504_CON_PAGEISZE_SHIFT);
+	switch (mtd->writesize) {
+	case 2048:

drivers/mtd/nand/hisi504_nand.c line 751

Quoted 4 lines

drivers/mtd/nand/hisi504_nand.c new file +4 -0

+		flag |= (0x001 << HINFC504_CON_PAGEISZE_SHIFT);
+	/* add more pagesize support
+	 * default pagesize has been set in hisi_nfc_host_init
+	 */

Does this mean you can't handle any other page size? You might want to put the words 'TODO' or 'FIXME' in the comment, and you might want to print an error and exit if you see some other value for mtd->writesize.

drivers/mtd/nand/hisi504_nand.c line 758

Quoted 7 lines

drivers/mtd/nand/hisi504_nand.c new file +7 -0

+	}
+	hinfc_write(host, flag, HINFC504_CON);
+
+	if (chip->ecc.mode == NAND_ECC_HW)
+		hisi_nfc_ecc_probe(host);
+
+	nand_scan_tail(mtd);

Please capture and handle the return value here.

Quoted 8 lines

drivers/mtd/nand/hisi504_nand.c new file +8 -0

+
+	ppdata.of_node = np;
+	ret = mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0);
+	if (ret) {
+		dev_err(dev, "Err MTD partition=%d\n", ret);
+		goto err_mtd;
+	}
+

drivers/mtd/nand/hisi504_nand.c line 772

Quoted 6 lines

drivers/mtd/nand/hisi504_nand.c new file +6 -0

+	return 0;
+
+err_mtd:
+	nand_release(mtd);
+err_ident:
+err_irq:

Do you really need these empty labels?

drivers/mtd/nand/hisi504_nand.c line 776

Quoted 4 lines

drivers/mtd/nand/hisi504_nand.c new file +4 -0

+err_buf:
+	if (host->buffer)
+		dma_free_coherent(dev, mtd->writesize + mtd->oobsize,
+				  host->buffer, host->dma_buffer);

This will go away.

drivers/mtd/nand/hisi504_nand.c line 788

Quoted 12 lines

drivers/mtd/nand/hisi504_nand.c new file +12 -0

+err_res:
+	return ret;
+}
+
+static int hisi_nfc_remove(struct platform_device *pdev)
+{
+	struct hinfc_host *host = platform_get_drvdata(pdev);
+	struct mtd_info *mtd = &host->mtd;
+
+	nand_release(mtd);
+	dma_free_coherent(&pdev->dev, mtd->writesize + mtd->oobsize,
+			  host->buffer, host->dma_buffer);

Same here.

Quoted 11 lines

drivers/mtd/nand/hisi504_nand.c new file +11 -0

+
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int hisi_nfc_suspend(struct platform_device *pdev,
+			       pm_message_t state)
+{
+	struct hinfc_host *host = platform_get_drvdata(pdev);
+
+	while ((hinfc_read(host, HINFC504_STATUS) & 0x1) == 0x0)

drivers/mtd/nand/hisi504_nand.c line 804

Quoted 5 lines

drivers/mtd/nand/hisi504_nand.c new file +5 -0

+		;
+
+	while ((hinfc_read(host, HINFC504_DMA_CTRL))
+		& HINFC504_DMA_CTRL_DMA_START)
+		_cond_resched();

It's probably best if these don't spin forever. Can you implement a timeout, and return an error on failure?

Quoted 36 lines

drivers/mtd/nand/hisi504_nand.c new file +36 -0

+
+	return 0;
+}
+
+static int hisi_nfc_resume(struct platform_device *pdev)
+{
+	int cs;
+	struct hinfc_host *host = platform_get_drvdata(pdev);
+	struct nand_chip *chip = &host->chip;
+
+	for (cs = 0; cs < chip->numchips; cs++)
+		hisi_nfc_send_cmd_reset(host, cs);
+	hinfc_write(host, SET_HINFC504_PWIDTH(HINFC504_W_LATCH,
+		    HINFC504_R_LATCH, HINFC504_RW_LATCH), HINFC504_PWIDTH);
+
+	return 0;
+}
+#endif
+static SIMPLE_DEV_PM_OPS(hisi_nfc_pm_ops, hisi_nfc_suspend, hisi_nfc_resume);
+
+static const struct of_device_id nfc_id_table[] = {
+	{ .compatible = "hisilicon,504-nfc" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, nfc_id_table);
+
+static struct platform_driver hisi_nfc_driver = {
+	.driver = {
+		.name  = "hisi_nand",
+		.of_match_table = of_match_ptr(nfc_id_table),
+		.pm = &hisi_nfc_pm_ops,
+	},
+	.probe		= hisi_nfc_probe,
+	.remove		= hisi_nfc_remove,
+};
+

drivers/mtd/nand/hisi504_nand.c line 846

Quoted 6 lines

drivers/mtd/nand/hisi504_nand.c new file +6 -0

+module_platform_driver(hisi_nfc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Zhiyong Cai");
+MODULE_AUTHOR("Zhou Wang");
+MODULE_DESCRIPTION("Hisilicon Nand Flash Controller Driver");

Brian

Quoted 2 lines · Brian Norris

Quoted 1 line · Zhou Wang

Signed-off-by Zhou Wang <[email protected]>

Quoted 32 lines

Quoted 32 lines

drivers/mtd/nand/hisi504_nand.c new file +32 -0

+static void hisi_nfc_dma_transfer(struct hinfc_host *host, int todev)
+{
+	struct mtd_info	*mtd = &host->mtd;
+	struct nand_chip *chip = mtd->priv;
+	unsigned long val;
+	int ret;
+
+	hinfc_write(host, host->dma_buffer, HINFC504_DMA_ADDR_DATA);
+	hinfc_write(host, host->dma_oob, HINFC504_DMA_ADDR_OOB);
+
+	if (chip->ecc.mode == NAND_ECC_NONE) {
+		hinfc_write(host, ((mtd->oobsize & HINFC504_DMA_LEN_OOB_MASK)
+			<< HINFC504_DMA_LEN_OOB_SHIFT), HINFC504_DMA_LEN);
+
+		hinfc_write(host, HINFC504_DMA_PARA_DATA_RW_EN
+			| HINFC504_DMA_PARA_OOB_RW_EN, HINFC504_DMA_PARA);
+	} else {
+		hinfc_write(host, HINFC504_DMA_PARA_DATA_RW_EN
+		| HINFC504_DMA_PARA_OOB_RW_EN | HINFC504_DMA_PARA_DATA_EDC_EN
+		| HINFC504_DMA_PARA_OOB_EDC_EN | HINFC504_DMA_PARA_DATA_ECC_EN
+		| HINFC504_DMA_PARA_OOB_ECC_EN, HINFC504_DMA_PARA);
+	}
+
+	val = (HINFC504_DMA_CTRL_DMA_START | HINFC504_DMA_CTRL_BURST4_EN
+		| HINFC504_DMA_CTRL_BURST8_EN | HINFC504_DMA_CTRL_BURST16_EN
+		| HINFC504_DMA_CTRL_DATA_AREA_EN | HINFC504_DMA_CTRL_OOB_AREA_EN
+		| ((host->addr_cycle == 4 ? 1 : 0)
+			<< HINFC504_DMA_CTRL_ADDR_NUM_SHIFT)
+		| ((host->chipselect & HINFC504_DMA_CTRL_CS_MASK)
+			<< HINFC504_DMA_CTRL_CS_SHIFT));
+
+	if (todev)

Quoted 9 lines

drivers/mtd/nand/hisi504_nand.c line 226

Quoted 5 lines

drivers/mtd/nand/hisi504_nand.c new file +5 -0

+		val |= HINFC504_DMA_CTRL_WE;
+
+	hinfc_write(host, val, HINFC504_DMA_CTRL);
+
+	init_completion(&host->cmd_complete);

You need to run init_completion() *before* you kick off your I/O. Otherwise, your interrupt handler is racing with this function.

Do you mean that it needs put init_completion() before hinfc_write()? If not so, interrupt handler maybe execute before init_completion() which will cause something wrong.

Quoted 2 lines

Quoted 2 lines

drivers/mtd/nand/hisi504_nand.c new file +2 -0

+	ret = wait_for_completion_timeout(&host->cmd_complete,
+			HINFC504_NFC_DMA_TIMEOUT);

Quoted 10 lines

Quoted 10 lines

drivers/mtd/nand/hisi504_nand.c new file +10 -0

+static void set_addr(struct mtd_info *mtd, int column, int page_addr)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct hinfc_host *host = chip->priv;
+
+	host->addr_cycle    = 0;
+	host->addr_value[0] = 0;
+	host->addr_value[1] = 0;
+
+	/* Serially input address */

Quoted 9 lines

drivers/mtd/nand/hisi504_nand.c line 418

Quoted 4 lines

drivers/mtd/nand/hisi504_nand.c new file +4 -0

+	if (column != -1) {
+		/* Adjust columns for 16 bit buswidth */
+		if (chip->options & NAND_BUSWIDTH_16)
+			column >>= 1;

It doesn't look like you're supporting ONFI parameter pages yet, but you might consider checking for !nand_opcode_8bits(opcode) before adjusting the address. We had to fix some other drivers for this recently.

sorry, could you give me more clue about this? Do you mean that we should first make sure it is not 8 bits bus width?

Quoted 4 lines

Quoted 4 lines

drivers/mtd/nand/hisi504_nand.c new file +4 -0

+
+		host->addr_value[0] = column & 0xffff;
+		host->addr_cycle    = 2;
+	}

Quoted 11 lines

drivers/mtd/nand/hisi504_nand.c line 751

Quoted 6 lines

drivers/mtd/nand/hisi504_nand.c new file +6 -0

+	switch (mtd->writesize) {
+	case 2048:
+		flag |= (0x001 << HINFC504_CON_PAGEISZE_SHIFT);
+	/* add more pagesize support
+	 * default pagesize has been set in hisi_nfc_host_init
+	 */

Does this mean you can't handle any other page size? You might want to put the words 'TODO' or 'FIXME' in the comment, and you might want to print an error and exit if you see some other value for mtd->writesize.

Yes, this NAND controller can handle not only 2048 page size. But the NAND controller on hip04-d01 board which I used to test this driver connects with a NAND flash chip which is just 2048 page size. So here I
just listed 'case 2048', Maybe I need indicate this by 'TODO:...'?

drivers/mtd/nand/hisi504_nand.c new file +5 -0

+static int hisi_nfc_remove(struct platform_device *pdev)
+{
+	struct hinfc_host *host = platform_get_drvdata(pdev);
+	struct mtd_info *mtd = &host->mtd;
+

drivers/mtd/nand/hisi504_nand.c new file +4 -0

+{
+	struct hinfc_host *host = platform_get_drvdata(pdev);
+
+	while ((hinfc_read(host, HINFC504_STATUS) & 0x1) == 0x0)

drivers/mtd/nand/hisi504_nand.c new file +4 -0

+{
+	int cs;
+	struct hinfc_host *host = platform_get_drvdata(pdev);
+	struct nand_chip *chip = &host->chip;

I forgot to mention these comments:

Quoted 9 lines · Zhou Wang

drivers/mtd/nand/hisi504_nand.c new file +9 -0

+static int hisi_nfc_remove(struct platform_device *pdev)
+{
+	struct hinfc_host *host = platform_get_drvdata(pdev);
+	struct mtd_info *mtd = &host->mtd;
+
+	nand_release(mtd);
+	dma_free_coherent(&pdev->dev, mtd->writesize + mtd->oobsize,
+			  host->buffer, host->dma_buffer);
+

drivers/mtd/nand/hisi504_nand.c line 795

Quoted 6 lines

drivers/mtd/nand/hisi504_nand.c new file +6 -0

+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int hisi_nfc_suspend(struct platform_device *pdev,
+			       pm_message_t state)

This is the wrong prototype. See struct dev_pm_ops.

Quoted 4 lines

drivers/mtd/nand/hisi504_nand.c new file +4 -0

+{
+	struct hinfc_host *host = platform_get_drvdata(pdev);
+
+	while ((hinfc_read(host, HINFC504_STATUS) & 0x1) == 0x0)

drivers/mtd/nand/hisi504_nand.c line 809

Quoted 10 lines

drivers/mtd/nand/hisi504_nand.c new file +10 -0

+		;
+
+	while ((hinfc_read(host, HINFC504_DMA_CTRL))
+		& HINFC504_DMA_CTRL_DMA_START)
+		_cond_resched();
+
+	return 0;
+}
+
+static int hisi_nfc_resume(struct platform_device *pdev)

Same here.

Quoted 4 lines

drivers/mtd/nand/hisi504_nand.c new file +4 -0

+{
+	int cs;
+	struct hinfc_host *host = platform_get_drvdata(pdev);
+	struct nand_chip *chip = &host->chip;

drivers/mtd/nand/hisi504_nand.c line 823

Quoted 10 lines

drivers/mtd/nand/hisi504_nand.c new file +10 -0

+
+	for (cs = 0; cs < chip->numchips; cs++)
+		hisi_nfc_send_cmd_reset(host, cs);
+	hinfc_write(host, SET_HINFC504_PWIDTH(HINFC504_W_LATCH,
+		    HINFC504_R_LATCH, HINFC504_RW_LATCH), HINFC504_PWIDTH);
+
+	return 0;
+}
+#endif
+static SIMPLE_DEV_PM_OPS(hisi_nfc_pm_ops, hisi_nfc_suspend, hisi_nfc_resume);

Brian

Signed-off-by Zhou Wang <[email protected]>

---
 .../devicetree/bindings/mtd/hisi504-nand.txt       |   40 ++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/hisi504-nand.txt

Documentation/devicetree/bindings/mtd/hisi504-nand.txt new file +0 -0

new file mode 100644
index 0000000..c8b3988

Documentation/devicetree/bindings/mtd/hisi504-nand.txt new file +16 -0

+- #address-cells:      partition address, should be set 1.
+- #size-cells:         partition size, should be set 1.
+
+Flash chip may optionally contain additional sub-nodes describing partitions of
+the address space. See partition.txt for more detail.
+
+Example:
+
+	nand: nand@4020000 {
+		compatible = "hisilicon,504-nfc";
+		reg = <0x4020000 0x10000>, <0x5000000 0x1000>;
+		interrupts = <0 379 4>;
+		nand-bus-width = <8>;
+		nand-ecc-mode = "hw";
+		hisi,nand-ecc-bits = <1>;
+		#address-cells = <1>;

Quoted 16 lines · Zhou Wang

Signed-off-by Zhou Wang <[email protected]>

---
  .../devicetree/bindings/mtd/hisi504-nand.txt       |   40 ++++++++++++++++++++
  1 file changed, 40 insertions(+)

create mode 100644 Documentation/devicetree/bindings/mtd/hisi504-nand.txt

Documentation/devicetree/bindings/mtd/hisi504-nand.txt new file +4 -0

new file mode 100644
index 0000000..c8b3988
@@ -0,0 +1,40 @@
+Hisilicon Hip04 Soc NAND controller DT binding
+
+Required properties:
+- compatible:          Should be "hisilicon,504-nfc".

Documentation/devicetree/bindings/mtd/hisi504-nand.txt line 10

Quoted 6 lines

Documentation/devicetree/bindings/mtd/hisi504-nand.txt new file +6 -0

+- reg:                 The first contains base physical address and size of
+                       NAND controller's registers. The second contains base
+                       physical address and size of NAND controller's buffer.
+- interrupts:          Interrupt number for nfc.
+- nand-bus-width:      See nand.txt.
+- nand-ecc-mode:       See nand.txt.

Do you support all modes, or just "hw"? Might be worth noting here.

Documentation/devicetree/bindings/mtd/hisi504-nand.txt line 14

Quoted 4 lines

Documentation/devicetree/bindings/mtd/hisi504-nand.txt new file +4 -0

+- hisi,nand-ecc-bits:  ECC bits type support.
+                 <0>:  none ecc
+                 <1>:  Can correct 1bit per 512byte.
+                 <6>:  Can correct 16bits per 1K byte.

You should re-use the nand-ecc-strength and nand-ecc-step-size properties here. So you'll support these options:

   nand-ecc-strength=0  nand-ecc-step-size=<don't care>
   nand-ecc-strength=1  nand-ecc-step-size=512
   nand-ecc-strength=16 nand-ecc-step-size=1024

Quoted 21 lines

Documentation/devicetree/bindings/mtd/hisi504-nand.txt new file +20 -0

+- #address-cells:      partition address, should be set 1.
+- #size-cells:         partition size, should be set 1.
+
+Flash chip may optionally contain additional sub-nodes describing partitions of
+the address space. See partition.txt for more detail.
+
+Example:
+
+	nand: nand@4020000 {
+		compatible = "hisilicon,504-nfc";
+		reg = <0x4020000 0x10000>, <0x5000000 0x1000>;
+		interrupts = <0 379 4>;
+		nand-bus-width = <8>;
+		nand-ecc-mode = "hw";
+		hisi,nand-ecc-bits = <1>;
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		partition@0 {
+			label = "nand_text";

Documentation/devicetree/bindings/mtd/hisi504-nand.txt line 40

Quoted 6 lines

Documentation/devicetree/bindings/mtd/hisi504-nand.txt new file +6 -0

+			reg = <0x00000000 0x00400000>;
+		};
+
+		...
+
+	};

Brian

One more thing:

Documentation/devicetree/bindings/mtd/hisi504-nand.txt line 4

Quoted 10 lines · Zhou Wang

Documentation/devicetree/bindings/mtd/hisi504-nand.txt new file +4 -0

new file mode 100644
index 0000000..c8b3988
@@ -0,0 +1,40 @@
+Hisilicon Hip04 Soc NAND controller DT binding
+
+Required properties:
+- compatible:          Should be "hisilicon,504-nfc".

I notice you use "NFC" naming throughout. I know others have had complaints about overloading the acronym NFC which sometimes means "Near Field Communication", instead of the intended "NAND Flash Controller." Is there any kind of naming convention within Hisilicon documentation that insists on calling this NFC, or can we avoid that confusing name?

If possible, I'd suggest "hisilicon,504-nand" as an alternative.

Brian

This patchset adds the support for NAND controller of hisilicon hip04 Soc. The NAND controller IP was developed by hisilicon and needs a new driver to support it. This patchset is based on v3.18-rc1. I have tested that NAND flash controller works fine in Hip04 D01 board.

Changes in v4:
- add mtd->dev.parent = &pdev->dev, thanks Frans Klaver.
Changes in v3:
- Modify code to eliminate some code style warnings.
- add ecc-bits input check.
- avoid using waterfall style in hisi_nfc_cmdfunc().
Changes in v2:
- Remove the patch for device tree, now patchset only has the driver and its device tree binding documentation.
- Change the file name: hisi_nand.c to hisi504_nand.c.
Changes in v1:
- Remove callback functions out of struct hinfc_host, and call them directly in relative functions.
- Change hinfc_read and hinfc_write from macros to inline functions.
- Instead of putting pointers, embed struct nand_chip and struct mtd_info in struct hinfc_host directly.
- rewrite some unclear lines in device tree binding document, correct some code style error.

Link on v3:
- https://lkml.org/lkml/2014/10/28/386
Link on v2:
- https://www.mail-archive.com/[email protected]/msg750071.html
Link on v1:
- https://lkml.org/lkml/2014/7/15/198

Zhou Wang (2):
mtd: hisilicon: add a new NAND controller driver for hisilicon hip04 Soc
mtd: hisilicon: add device tree binding documentation

 .../devicetree/bindings/mtd/hisi504-nand.txt       |   40 +
 drivers/mtd/nand/Kconfig                           |    5 +
 drivers/mtd/nand/Makefile                          |    1 +
 drivers/mtd/nand/hisi504_nand.c                    |  846 ++++++++++++++++++++
 4 files changed, 892 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/hisi504-nand.txt
 create mode 100644 drivers/mtd/nand/hisi504_nand.c

--
1.7.9.5

This patchset adds the support for NAND controller of hisilicon hip04 Soc. The NAND controller IP was developed by hisilicon and needs a new driver to support it. This patchset is based on v3.18-rc1. I have tested that NAND flash controller works fine in Hip04 D01 board.

Changes in v4:
- add mtd->dev.parent = &pdev->dev, thanks Frans Klaver.
Changes in v3:
- Modify code to eliminate some code style warnings.
- add ecc-bits input check.
- avoid using waterfall style in hisi_nfc_cmdfunc().
Changes in v2:
- Remove the patch for device tree, now patchset only has the driver and its device tree binding documentation.
- Change the file name: hisi_nand.c to hisi504_nand.c.
Changes in v1:
- Remove callback functions out of struct hinfc_host, and call them directly in relative functions.
- Change hinfc_read and hinfc_write from macros to inline functions.
- Instead of putting pointers, embed struct nand_chip and struct mtd_info in struct hinfc_host directly.
- rewrite some unclear lines in device tree binding document, correct some code style error.

Link on v3:
- https://lkml.org/lkml/2014/10/28/386
Link on v2:
- https://www.mail-archive.com/[email protected]/msg750071.html
Link on v1:
- https://lkml.org/lkml/2014/7/15/198

Zhou Wang (2):
mtd: hisilicon: add a new NAND controller driver for hisilicon hip04 Soc
mtd: hisilicon: add device tree binding documentation

 .../devicetree/bindings/mtd/hisi504-nand.txt       |   40 +
 drivers/mtd/nand/Kconfig                           |    5 +
 drivers/mtd/nand/Makefile                          |    1 +
 drivers/mtd/nand/hisi504_nand.c                    |  846 ++++++++++++++++++++
 4 files changed, 892 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/hisi504-nand.txt
 create mode 100644 drivers/mtd/nand/hisi504_nand.c

--
1.7.9.5

This patchset adds the support for NAND controller of hisilicon hip04 Soc. The NAND controller IP was developed by hisilicon and needs a new driver to support it. This patchset is based on v3.18-rc1. I have tested that NAND flash controller works fine in Hip04 D01 board.

Quoted 4 lines · Zhou Wang

This patchset adds the support for NAND controller of hisilicon hip04 Soc. The NAND controller IP was developed by hisilicon and needs a new driver to support it. This patchset is based on v3.18-rc1. I have tested that NAND flash controller works fine in Hip04 D01 board.

Have you tested on the MTD test modules (drivers/mtd/tests/*)? This is important, as we seem to regularly get UBIFS bug reports from users whose drivers have not even passed some of the simple tests.

Also, it might be worth testing out the UBI tests found in the mtd-utils package.

Brian

Quoted 4 lines · Zhou Wang

This patchset adds the support for NAND controller of hisilicon hip04 Soc. The NAND controller IP was developed by hisilicon and needs a new driver to support it. This patchset is based on v3.18-rc1. I have tested that NAND flash controller works fine in Hip04 D01 board.

Have you tested on the MTD test modules (drivers/mtd/tests/*)? This is important, as we seem to regularly get UBIFS bug reports from users whose drivers have not even passed some of the simple tests.

Also, it might be worth testing out the UBI tests found in the mtd-utils package.

Brian

Quoted 6 lines · Brian Norris

Have you tested on the MTD test modules (drivers/mtd/tests/*)? This is important, as we seem to regularly get UBIFS bug reports from users whose drivers have not even passed some of the simple tests.

Also, it might be worth testing out the UBI tests found in the mtd-utils package.

I have tested this NAND controller driver on the MTD test modules (drivers/mtd/tests/*). All tests passed except mtd_nandbiterrs.ko.

Here is the test log for mtd_nandbiterrs:
/home # insmod mtd_nandbiterrs.ko dev=2 page_offset=1 seed=110 mode=0

[  100.484995]
[  100.490082] ==================================================
[  100.509657] mtd_nandbiterrs: MTD device: 2
[  100.523413] mtd_nandbiterrs: MTD device size 8388608,
eraseblock=131072, page=2048, oob=64
[  100.551134] mtd_nandbiterrs: Device uses 2 subpages of 1024 bytes
[  100.571585] mtd_nandbiterrs: Using page=1, offset=2048, eraseblock=0
[  104.431136] mtd_nandbiterrs: incremental biterrors test
[  104.448872] mtd_nandbiterrs: write_page
[  104.463193] mtd_nandbiterrs: rewrite page
[  104.477620] mtd_nandbiterrs: read_page
[  104.490898] mtd_nandbiterrs: verify_page
[  104.504338] mtd_nandbiterrs: Successfully corrected 0 bit errors
per subpage
[  104.527985] mtd_nandbiterrs: Inserted biterror @ 0/5
[  104.544673] mtd_nandbiterrs: Inserted biterror @ 1024/2
[  104.562197] mtd_nandbiterrs: rewrite page
[  104.576766] mtd_nandbiterrs: read_page
[  104.590052] mtd_nandbiterrs: verify_page
[  104.603252] mtd_nandbiterrs: Error: page offset 0, expected 23, got 03
[  104.625203] mtd_nandbiterrs: Error: page offset 1024, expected 06, got 02
[  104.648056] mtd_nandbiterrs: ECC failure, read data is incorrect

despite read success
insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error

The reason for above failure is that:
In ECC mode, when rewriting page data to NAND flash, the NAND controller will also produce ECC code and write them to NAND flash as well. So when we read data from NAND flash, there is no need to correct the error bit. We read what we write to the flash.

In mtd_nandbiterrs test, We call nand_write_page_raw indeed to perform rewrite operation. My question is that: in this NAND controller hardware design, it is hard to implement hardware specific write_page_raw to write page data without producing ECC code, will this bring some bad effects somewhere? It will be very nice if you and anyone can give me some advice.

Quoted 44 lines · Zhou Wang

Quoted 6 lines · Brian Norris

Have you tested on the MTD test modules (drivers/mtd/tests/*)? This is important, as we seem to regularly get UBIFS bug reports from users whose drivers have not even passed some of the simple tests.

Also, it might be worth testing out the UBI tests found in the mtd-utils package.

I have tested this NAND controller driver on the MTD test modules (drivers/mtd/tests/*). All tests passed except mtd_nandbiterrs.ko.

Here is the test log for mtd_nandbiterrs:
/home # insmod mtd_nandbiterrs.ko dev=2 page_offset=1 seed=110 mode=0

[  100.484995]
[  100.490082] ==================================================
[  100.509657] mtd_nandbiterrs: MTD device: 2
[  100.523413] mtd_nandbiterrs: MTD device size 8388608,
eraseblock=131072, page=2048, oob=64
[  100.551134] mtd_nandbiterrs: Device uses 2 subpages of 1024 bytes
[  100.571585] mtd_nandbiterrs: Using page=1, offset=2048, eraseblock=0
[  104.431136] mtd_nandbiterrs: incremental biterrors test
[  104.448872] mtd_nandbiterrs: write_page
[  104.463193] mtd_nandbiterrs: rewrite page
[  104.477620] mtd_nandbiterrs: read_page
[  104.490898] mtd_nandbiterrs: verify_page
[  104.504338] mtd_nandbiterrs: Successfully corrected 0 bit errors
per subpage
[  104.527985] mtd_nandbiterrs: Inserted biterror @ 0/5
[  104.544673] mtd_nandbiterrs: Inserted biterror @ 1024/2
[  104.562197] mtd_nandbiterrs: rewrite page
[  104.576766] mtd_nandbiterrs: read_page
[  104.590052] mtd_nandbiterrs: verify_page
[  104.603252] mtd_nandbiterrs: Error: page offset 0, expected 23, got 03
[  104.625203] mtd_nandbiterrs: Error: page offset 1024, expected 06, got 02
[  104.648056] mtd_nandbiterrs: ECC failure, read data is incorrect

despite read success
insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error

The reason for above failure is that:
In ECC mode, when rewriting page data to NAND flash, the NAND controller will also produce ECC code and write them to NAND flash as well. So when we read data from NAND flash, there is no need to correct the error bit. We read what we write to the flash.

Quoted 6 lines

In mtd_nandbiterrs test, We call nand_write_page_raw indeed to perform rewrite operation. My question is that: in this NAND controller hardware design, it is hard to implement hardware specific write_page_raw to write page data without producing ECC code, will this bring some bad effects somewhere? It will be very nice if you and anyone can give me some advice.

As of now, read_page_raw()/write_page_raw() are not required for any normal operation. They are useful for debugging and testing though, with tests like this one.

In the future, we might use read_page_raw() for implementing (optional) software-based detection of blank/erased pages that have bitflips in them -- i.e., all 0xff but with a few bitflips. See:

   http://lists.infradead.org/pipermail/linux-mtd/2014-December/056749.html
   http://article.gmane.org/gmane.linux.drivers.mtd/52183/

Brian

I have tested this NAND controller driver on the MTD test modules (drivers/mtd/tests/*). All tests passed except mtd_nandbiterrs.ko.

Here is the test log for mtd_nandbiterrs:
/home # insmod mtd_nandbiterrs.ko dev=2 page_offset=1 seed=110 mode=0

[  100.484995]
[  100.490082] ==================================================
[  100.509657] mtd_nandbiterrs: MTD device: 2
[  100.523413] mtd_nandbiterrs: MTD device size 8388608,
eraseblock=131072, page=2048, oob=64
[  100.551134] mtd_nandbiterrs: Device uses 2 subpages of 1024 bytes
[  100.571585] mtd_nandbiterrs: Using page=1, offset=2048, eraseblock=0
[  104.431136] mtd_nandbiterrs: incremental biterrors test
[  104.448872] mtd_nandbiterrs: write_page
[  104.463193] mtd_nandbiterrs: rewrite page
[  104.477620] mtd_nandbiterrs: read_page
[  104.490898] mtd_nandbiterrs: verify_page
[  104.504338] mtd_nandbiterrs: Successfully corrected 0 bit errors
per subpage
[  104.527985] mtd_nandbiterrs: Inserted biterror @ 0/5
[  104.544673] mtd_nandbiterrs: Inserted biterror @ 1024/2
[  104.562197] mtd_nandbiterrs: rewrite page
[  104.576766] mtd_nandbiterrs: read_page
[  104.590052] mtd_nandbiterrs: verify_page
[  104.603252] mtd_nandbiterrs: Error: page offset 0, expected 23, got 03
[  104.625203] mtd_nandbiterrs: Error: page offset 1024, expected 06, got 02
[  104.648056] mtd_nandbiterrs: ECC failure, read data is incorrect

despite read success
insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error

The reason for above failure is that:
In ECC mode, when rewriting page data to NAND flash, the NAND controller will also produce ECC code and write them to NAND flash as well. So when we read data from NAND flash, there is no need to correct the error bit. We read what we write to the flash.

Quoted 27 lines · Zhou Wang

I have tested this NAND controller driver on the MTD test modules (drivers/mtd/tests/*). All tests passed except mtd_nandbiterrs.ko.

Here is the test log for mtd_nandbiterrs:
/home # insmod mtd_nandbiterrs.ko dev=2 page_offset=1 seed=110 mode=0

[  100.484995]
[  100.490082] ==================================================
[  100.509657] mtd_nandbiterrs: MTD device: 2
[  100.523413] mtd_nandbiterrs: MTD device size 8388608,
eraseblock=131072, page=2048, oob=64
[  100.551134] mtd_nandbiterrs: Device uses 2 subpages of 1024 bytes
[  100.571585] mtd_nandbiterrs: Using page=1, offset=2048, eraseblock=0
[  104.431136] mtd_nandbiterrs: incremental biterrors test
[  104.448872] mtd_nandbiterrs: write_page
[  104.463193] mtd_nandbiterrs: rewrite page
[  104.477620] mtd_nandbiterrs: read_page
[  104.490898] mtd_nandbiterrs: verify_page
[  104.504338] mtd_nandbiterrs: Successfully corrected 0 bit errors
per subpage
[  104.527985] mtd_nandbiterrs: Inserted biterror @ 0/5
[  104.544673] mtd_nandbiterrs: Inserted biterror @ 1024/2
[  104.562197] mtd_nandbiterrs: rewrite page
[  104.576766] mtd_nandbiterrs: read_page
[  104.590052] mtd_nandbiterrs: verify_page
[  104.603252] mtd_nandbiterrs: Error: page offset 0, expected 23, got 03
[  104.625203] mtd_nandbiterrs: Error: page offset 1024, expected 06, got 02
[  104.648056] mtd_nandbiterrs: ECC failure, read data is incorrect

Quoted 8 lines

despite read success
insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error

The reason for above failure is that:
In ECC mode, when rewriting page data to NAND flash, the NAND controller will also produce ECC code and write them to NAND flash as well. So when we read data from NAND flash, there is no need to correct the error bit. We read what we write to the flash.

BTW, your explanation doesn't seem quite right. The problem is that even though mtd_read() didn't report errors, the data doesn't match what's written. It's not that there was "no need to correct the error bit".

I'd recommend digging a little more to figure out what's wrong here. You might need to instrument the nandbiterrs test. This is possibly

highlighting a driver bug [1].

Brian

[1] Besised simply that you didn't implement write_page_raw(). The default nand_write_page_raw() implementation looks just like your non-raw version.

On 2014年12月17日 14:23, Brian Norris wrote:

Quoted 10 lines

Quoted 9 lines

[  104.648056] mtd_nandbiterrs: ECC failure, read data is incorrect

despite read success
insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error

The reason for above failure is that:
In ECC mode, when rewriting page data to NAND flash, the NAND controller will also produce ECC code and write them to NAND flash as well. So when we read data from NAND flash, there is no need to correct the error bit. We read what we write to the flash.

Quoted 4 lines

BTW, your explanation doesn't seem quite right. The problem is that even though mtd_read() didn't report errors, the data doesn't match what's written. It's not that there was "no need to correct the error bit".

Maybe I did not express clearly. In the nandbiterrs test, firstly write data to flash with ECC code in oob area, then change some bits and rewrite data to flash with old ECC code in oob area, at last read data out with ECC to test if the "error bits" can be corrected. My explanation is that in rewriting process NAND controller also produces new ECC code of the data and write both data and new ECC code to flash. So in next step we will get what was writen without "correction".

Quoted 2 lines

I'd recommend digging a little more to figure out what's wrong here. You might need to instrument the nandbiterrs test. This is possibly

Thanks, I will do it.

Quoted 4 lines

highlighting a driver bug [1].

Brian

Quoted 3 lines

[1] Besised simply that you didn't implement write_page_raw(). The default nand_write_page_raw() implementation looks just like your non-raw version.

Yes. In ECC mode, as the NAND controller must write page as a whole with ECC code, the default nand_write_page_raw() looks just like non-raw version.

Following up on this last comment from last year's thread:

Quoted 1 line · Zhou Wang

On 2014年12月17日 14:23, Brian Norris wrote:

[...]

Quoted 15 lines

Quoted 10 lines

Quoted 9 lines

[  104.648056] mtd_nandbiterrs: ECC failure, read data is incorrect

despite read success
insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error

The reason for above failure is that:
In ECC mode, when rewriting page data to NAND flash, the NAND controller will also produce ECC code and write them to NAND flash as well. So when we read data from NAND flash, there is no need to correct the error bit. We read what we write to the flash.

Quoted 4 lines

BTW, your explanation doesn't seem quite right. The problem is that even though mtd_read() didn't report errors, the data doesn't match what's written. It's not that there was "no need to correct the error bit".

Quoted 7 lines

Maybe I did not express clearly. In the nandbiterrs test, firstly write data to flash with ECC code in oob area, then change some bits and rewrite data to flash with old ECC code in oob area, at last read data out with ECC to test if the "error bits" can be corrected. My explanation is that in rewriting process NAND controller also produces new ECC code of the data and write both data and new ECC code to flash. So in next step we will get what was writen without "correction".

But we should at least get an -EBADMSG return status, right? If you're "rewriting" the data, this should result in two sets of data written on top of each other, which (depending on the flash layout charecteristics) might turn up as a kind of logical AND of all the data+OOB. This is "probably" not correctable.

But that last "probably" leaves room for the possibility you mentioned, I guess; that the ECC code is just correcting the data to look like the second set of (intentionally) erroneous data.

Quoted 14 lines

Quoted 3 lines

I'd recommend digging a little more to figure out what's wrong here. You might need to instrument the nandbiterrs test. This is possibly

Thanks, I will do it.

Quoted 7 lines

highlighting a driver bug [1].

Brian

[1] Besised simply that you didn't implement write_page_raw(). The default nand_write_page_raw() implementation looks just like your non-raw version.

Quoted 3 lines

Yes. In ECC mode, as the NAND controller must write page as a whole with ECC code, the default nand_write_page_raw() looks just like non-raw version.

Are you saying you cannot implement the raw() hooks for this IP? Or just that you haven't yet? The latter is probably OK for now (I'd recommend doing this, or at least mark a TODO in the code), but the former is a little disturbing.

Brian

.

Very sorry for late, I made tests again and also had a talk with the NAND controller hardware colleague. Please find my reply below.

Quoted 6 lines · Zhou Wang

Quoted 1 line · Brian Norris

[...]

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

[  104.648056] mtd_nandbiterrs: ECC failure, read data is incorrect

despite read success
insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error

I made testes again in 1bit/ECC and 16bit/ECC modes using 2K(page)+64B(oob) NAND flash. here are the logs, I also printed ECC code in OOB area.

Results are:
1. in 16bit/ECC, it will return -EBADMSG as the ECC codes have been broken.
2. in 1bit/ECC, it will not reture -EBADMSG because a hardware design problem. I will explain the detail below.

Test logs:
1. in 16bit/ECC(print ECC codes):

/home # insmod mtd_nandbiterrs.ko dev=2 page_offset=1 seed=110 mode=0

mtd_nandbiterrs: error: read failed at 0x800
mtd_nandbiterrs: After 1 biterrors per subpage, read reported error -74

mtd_nandbiterrs: finished successfully.
==================================================
insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error

2. in 1bit/ECC(print ECC codes):
/home # insmod mtd_nandbiterrs.ko dev=2 page_offset=1 seed=110 mode=0

mtd_nandbiterrs: ECC failure, read data is incorrect despite read success
insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error

Reason about above 1bit/ECC test result:

It can not correct this kind of 2bit errors in 1bit/ECC mode in this NAND controller, however, it will trigger a correctable interrupt. As a result, software can not find this 1bit error in page data.

This is a hardware problem of this NAND controller.
I plan to remove the 1bit/ECC mode support in patch of next version.

Quoted 4 lines

Are you saying you cannot implement the raw() hooks for this IP? Or just that you haven't yet? The latter is probably OK for now (I'd recommend doing this, or at least mark a TODO in the code), but the former is a little disturbing.

The function of raw() hooks is just writing the page data to flash, is this right?

In none ECC mode, it can write page date alone to flash. But in ECC mode, NAND controller will produce related ECC code automatically, write page data and ECC code to flash. In ECC mode, it can not write page date alone to flash for this NAND controller.

As a result, the nandbiterrs test can not pass.

I don't know if I have explained these two problems clearly. If still have something confused, please let me know.

Hi Zhou,

Quoted 2 lines · Zhou Wang

Very sorry for late, I made tests again and also had a talk with the NAND controller hardware colleague. Please find my reply below.

No problem. Glad to hear you followed through on this one, as the results were curious.

Quoted 7 lines · Brian Norris

Quoted 6 lines · Zhou Wang

Quoted 1 line · Brian Norris

[...]

Quoted 3 lines

Quoted 3 lines

Quoted 3 lines

[  104.648056] mtd_nandbiterrs: ECC failure, read data is incorrect

despite read success
insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error

[...]

Quoted 12 lines

I made testes again in 1bit/ECC and 16bit/ECC modes using 2K(page)+64B(oob) NAND flash. here are the logs, I also printed ECC code in OOB area.

Results are:
1. in 16bit/ECC, it will return -EBADMSG as the ECC codes have been broken.
2. in 1bit/ECC, it will not reture -EBADMSG because a hardware design problem. I will explain the detail below.

Test logs:
1. in 16bit/ECC(print ECC codes):

/home # insmod mtd_nandbiterrs.ko dev=2 page_offset=1 seed=110 mode=0

...

Quoted 2 lines

mtd_nandbiterrs: error: read failed at 0x800
mtd_nandbiterrs: After 1 biterrors per subpage, read reported error -74

^^^ Ah, that's what I would expect from a driver that doesn't implement the raw() functions.

Quoted 6 lines

mtd_nandbiterrs: finished successfully.
==================================================
insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error

2. in 1bit/ECC(print ECC codes):
/home # insmod mtd_nandbiterrs.ko dev=2 page_offset=1 seed=110 mode=0

...

Quoted 4 lines

mtd_nandbiterrs: ECC failure, read data is incorrect despite read success
insmod: can't insert 'mtd_nandbiterrs.ko': Input/output error

Reason about above 1bit/ECC test result:

...

Quoted 3 lines

It can not correct this kind of 2bit errors in 1bit/ECC mode in this NAND controller, however, it will trigger a correctable interrupt. As a result, software can not find this 1bit error in page data.

IOW, uncorrectable errors are getting reported as corrected bitflips? That does sound bad.

Quoted 3 lines

This is a hardware problem of this NAND controller.
I plan to remove the 1bit/ECC mode support in patch of next version.

OK, sounds good. 1-bit HW ECC is not really very useful these days anyway, if your higher-bit ECC can serve to replace it. Can the ECC bytes still fit in the same spare area, though?

Quoted 6 lines

Quoted 4 lines

Are you saying you cannot implement the raw() hooks for this IP? Or just that you haven't yet? The latter is probably OK for now (I'd recommend doing this, or at least mark a TODO in the code), but the former is a little disturbing.

The function of raw() hooks is just writing the page data to flash, is this right?

Right, just data (and OOB, if calling the _oob_ functions) without any ECC parity bytes.

Quoted 3 lines

In none ECC mode, it can write page date alone to flash. But in ECC mode, NAND controller will produce related ECC code automatically, write page data and ECC code to flash. In ECC mode, it can not write page date alone to flash for this NAND controller.

Perhaps you can switch between ECC mode and non-ECC mode?

At any rate, this isn't absolutely required.

Quoted 4 lines

As a result, the nandbiterrs test can not pass.

I don't know if I have explained these two problems clearly. If still have something confused, please let me know.

Brian

.

Quoted 1 line · Brian Norris

Perhaps you can switch between ECC mode and non-ECC mode?

I also tried to swith to non-ECC mode to implement raw_write, and it is easy to do this.

But, event we do this, it will bring me little help. The reason to implement raw_write is to run mtd_nandbiterrs correctly as the discussion above. However, this NAND controller writes data and ECC code to flash using different layouts in different modes.

So we should also read data
and ECC code from flash, change related bits, then rewrite the data and ECC code to flash in none-ECC mode to run mtd_nandbiterrs. It is not easy to do this.

So my plan is not to implement raw_write hook if this function is just used in test.