[PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc

[PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc

[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.

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/linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 1/2] mtd: hisilicon: add a new NAND controller driver for hisilicon hip04 Soc

[Zhou Wang]

Signed-off-by: Zhou Wang <wangzhou.bry@gmail.com>
---
 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

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index dd10646..e451a08 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -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).
 
+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
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index 9c847e4..fb1b2e4 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -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
diff --git a/drivers/mtd/nand/hisi504_nand.c b/drivers/mtd/nand/hisi504_nand.c
new file mode 100644
index 0000000..a169cd8
--- /dev/null
+++ b/drivers/mtd/nand/hisi504_nand.c
@@ -0,0 +1,846 @@
+/*
+ * Hisilicon NAND Flash controller driver
+ *
+ * Copyright © 2012-2014 HiSilicon Technologies Co., Ltd.
+ *              http://www.hisilicon.com
+ *
+ * Author: Zhou Wang <wangzhou.bry@gmail.com>
+ * The initial developer of the original code is Zhiyong Cai
+ * <caizhiyong@huawei.com>
+ *
+ * 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
+#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)
+#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)
+
+#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;
+	unsigned int		command;
+	int			chipselect;
+	unsigned int		addr_cycle;
+	unsigned int		addr_value[2];
+	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)
+		val |= HINFC504_DMA_CTRL_WE;
+
+	hinfc_write(host, val, HINFC504_DMA_CTRL);
+
+	init_completion(&host->cmd_complete);
+	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!");
+		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 */
+	if (column != -1) {
+		/* Adjust columns for 16 bit buswidth */
+		if (chip->options & NAND_BUSWIDTH_16)
+			column >>= 1;
+
+		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;
+	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);
+	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;
+		goto err_ident;
+	}
+
+	host->buffer = dma_alloc_coherent(dev, mtd->writesize + mtd->oobsize,
+		&host->dma_buffer, GFP_KERNEL);
+	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:
+		flag |= (0x001 << HINFC504_CON_PAGEISZE_SHIFT);
+	/* add more pagesize support
+	 * default pagesize has been set in hisi_nfc_host_init
+	 */
+	}
+	hinfc_write(host, flag, HINFC504_CON);
+
+	if (chip->ecc.mode == NAND_ECC_HW)
+		hisi_nfc_ecc_probe(host);
+
+	nand_scan_tail(mtd);
+
+	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;
+	}
+
+	return 0;
+
+err_mtd:
+	nand_release(mtd);
+err_ident:
+err_irq:
+err_buf:
+	if (host->buffer)
+		dma_free_coherent(dev, mtd->writesize + mtd->oobsize,
+				  host->buffer, host->dma_buffer);
+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);
+
+	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)
+		;
+
+	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)
+{
+	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,
+};
+
+module_platform_driver(hisi_nfc_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Zhiyong Cai");
+MODULE_AUTHOR("Zhou Wang");
+MODULE_DESCRIPTION("Hisilicon Nand Flash Controller Driver");
-- 
1.7.9.5

Re: [PATCH v4 1/2] mtd: hisilicon: add a new NAND controller driver for hisilicon hip04 Soc

[Brian Norris]

On Tue, Nov 04, 2014 at 08:47:00PM +0800, Zhou Wang wrote:
> Signed-off-by: Zhou Wang <wangzhou.bry-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

This driver mostly looks good. A few comments.

> ---
>  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
> 
> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
> index dd10646..e451a08 100644
> --- a/drivers/mtd/nand/Kconfig
> +++ b/drivers/mtd/nand/Kconfig
> @@ -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).
>  
> +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...)

> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
> index 9c847e4..fb1b2e4 100644
> --- a/drivers/mtd/nand/Makefile
> +++ b/drivers/mtd/nand/Makefile
> @@ -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
> diff --git a/drivers/mtd/nand/hisi504_nand.c b/drivers/mtd/nand/hisi504_nand.c
> new file mode 100644
> index 0000000..a169cd8
> --- /dev/null
> +++ b/drivers/mtd/nand/hisi504_nand.c
> @@ -0,0 +1,846 @@
> +/*
> + * Hisilicon NAND Flash controller driver
> + *
> + * Copyright © 2012-2014 HiSilicon Technologies Co., Ltd.
> + *              http://www.hisilicon.com
> + *
> + * Author: Zhou Wang <wangzhou.bry-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> + * The initial developer of the original code is Zhiyong Cai
> + * <caizhiyong-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
> + *
> + * 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
> +#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.

> +#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.

> +
> +#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;
> +	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.

> +	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)
> +		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.

> +	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/

> +		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 */
> +	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.

> +
> +		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;
> +	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().

> +	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;
> +		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.

> +	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:
> +		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.

> +	}
> +	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.

> +
> +	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;
> +	}
> +
> +	return 0;
> +
> +err_mtd:
> +	nand_release(mtd);
> +err_ident:
> +err_irq:

Do you really need these empty labels?

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

This will go away.

> +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.
> +
> +	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)
> +		;
> +
> +	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?

> +
> +	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,
> +};
> +
> +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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] mtd: hisilicon: add a new NAND controller driver for hisilicon hip04 Soc

[Zhou Wang]

On 2014年11月30日 17:35, Brian Norris wrote:
> On Tue, Nov 04, 2014 at 08:47:00PM +0800, Zhou Wang wrote:
>> Signed-off-by: Zhou Wang <wangzhou.bry-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> This driver mostly looks good. A few comments.
>
>> ---
>>   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
>>
>> diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
>> index dd10646..e451a08 100644
>> --- a/drivers/mtd/nand/Kconfig
>> +++ b/drivers/mtd/nand/Kconfig
>> @@ -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).
>>
>> +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...)
>

Hi Brian,

I will rebase the patchset on l2-mtd.git when resend.

>> diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
>> index 9c847e4..fb1b2e4 100644
>> --- a/drivers/mtd/nand/Makefile
>> +++ b/drivers/mtd/nand/Makefile
>> @@ -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
>> diff --git a/drivers/mtd/nand/hisi504_nand.c b/drivers/mtd/nand/hisi504_nand.c
>> new file mode 100644
>> index 0000000..a169cd8
>> --- /dev/null
>> +++ b/drivers/mtd/nand/hisi504_nand.c
>> @@ -0,0 +1,846 @@
>> +/*
>> + * Hisilicon NAND Flash controller driver
>> + *
>> + * Copyright © 2012-2014 HiSilicon Technologies Co., Ltd.
>> + *              http://www.hisilicon.com
>> + *
>> + * Author: Zhou Wang <wangzhou.bry-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> + * The initial developer of the original code is Zhiyong Cai
>> + * <caizhiyong-hv44wF8Li93QT0dZR+AlfA@public.gmane.org>
>> + *
>> + * 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
>> +#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.

will modify this and use BIT(), thanks.

>
>> +#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.
>
>> +
>> +#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;
>> +	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.
>

Right, thanks.

>> +	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)
>> +		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.

>> +	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/

will modify this, thanks!

>
>> +		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 */
>> +	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?

>> +
>> +		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;
>> +	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().

OK, will modify this.

>
>> +	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;
>> +		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.
>

OK, will modify this and related code.

>> +	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:
>> +		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:...'?

>> +	}
>> +	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.
>

will modify this, thanks!

>> +
>> +	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;
>> +	}
>> +
>> +	return 0;
>> +
>> +err_mtd:
>> +	nand_release(mtd);
>> +err_ident:
>> +err_irq:
>
> Do you really need these empty labels?
>

will merge the err_ident, err_irq, err_buf into err_res.

>> +err_buf:
>> +	if (host->buffer)
>> +		dma_free_coherent(dev, mtd->writesize + mtd->oobsize,
>> +				  host->buffer, host->dma_buffer);
>
> This will go away.
>
>> +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.
>> +
>> +	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)
>> +		;
>> +
>> +	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?
>

will add a timeout here, thanks!

>> +
>> +	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,
>> +};
>> +
>> +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
>
Thanks!
Zhou Wang

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] mtd: hisilicon: add a new NAND controller driver for hisilicon hip04 Soc

[Brian Norris]

On Mon, Dec 01, 2014 at 09:08:42PM +0800, Zhou Wang wrote:
> On 2014年11月30日 17:35, Brian Norris wrote:
> >On Tue, Nov 04, 2014 at 08:47:00PM +0800, Zhou Wang wrote:
> >>Signed-off-by: Zhou Wang <wangzhou.bry-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
[...]
> >>+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)
> >>+		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.

Yes.

	init_completion(&host->cmd_complete);

should come sometime before

	hinfc_write(host, val, HINFC504_DMA_CTRL);

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

[...]

> >>+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 */
> >>+	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?

Look for use of nand_opcode_8bits() in nand_base.c.

I'm suggesting that you don't necessarily want to "adjust the column"
for all commands; there are some opcodes that take their address only on
the lower x8 bits, even if the flash has an x16 buswidth.

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

[...]

> >>+	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:...'?

Not just a TODO; make sure to also error out, so that users don't get
unexplained failures if they find themselves with a non-2KB page size
NAND.

Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] mtd: hisilicon: add a new NAND controller driver for hisilicon hip04 Soc

[Brian Norris]

I forgot to mention these comments:

On Tue, Nov 04, 2014 at 08:47:00PM +0800, Zhou Wang wrote:
> +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);
> +
> +	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.

> +{
> +	struct hinfc_host *host = platform_get_drvdata(pdev);
> +
> +	while ((hinfc_read(host, HINFC504_STATUS) & 0x1) == 0x0)
> +		;
> +
> +	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.

> +{
> +	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);

Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/2] mtd: hisilicon: add a new NAND controller driver for hisilicon hip04 Soc

[Zhou Wang]

On 2014年12月01日 17:25, Brian Norris wrote:
> I forgot to mention these comments:
>
> On Tue, Nov 04, 2014 at 08:47:00PM +0800, Zhou Wang wrote:
>> +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);
>> +
>> +	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.
>

My mistake here, will modify this, thanks a lot!!

>> +{
>> +	struct hinfc_host *host = platform_get_drvdata(pdev);
>> +
>> +	while ((hinfc_read(host, HINFC504_STATUS) & 0x1) == 0x0)
>> +		;
>> +
>> +	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.
>
>> +{
>> +	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);
>
> Brian
>

Thanks,
Zhou Wang


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 2/2] mtd: hisilicon: add device tree binding documentation

[Zhou Wang]

Signed-off-by: Zhou Wang <wangzhou.bry@gmail.com>
---
 .../devicetree/bindings/mtd/hisi504-nand.txt       |   40 ++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/hisi504-nand.txt

diff --git a/Documentation/devicetree/bindings/mtd/hisi504-nand.txt b/Documentation/devicetree/bindings/mtd/hisi504-nand.txt
new file mode 100644
index 0000000..c8b3988
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/hisi504-nand.txt
@@ -0,0 +1,40 @@
+Hisilicon Hip04 Soc NAND controller DT binding
+
+Required properties:
+- compatible:          Should be "hisilicon,504-nfc".
+- 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.
+- hisi,nand-ecc-bits:  ECC bits type support.
+                 <0>:  none ecc
+                 <1>:  Can correct 1bit per 512byte.
+                 <6>:  Can correct 16bits per 1K byte.
+- #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";
+			reg = <0x00000000 0x00400000>;
+		};
+
+		...
+
+	};
-- 
1.7.9.5

Re: [PATCH v4 2/2] mtd: hisilicon: add device tree binding documentation

[Brian Norris]

On Tue, Nov 04, 2014 at 08:47:01PM +0800, Zhou Wang wrote:
> Signed-off-by: Zhou Wang <wangzhou.bry@gmail.com>
> ---
>  .../devicetree/bindings/mtd/hisi504-nand.txt       |   40 ++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/mtd/hisi504-nand.txt
> 
> diff --git a/Documentation/devicetree/bindings/mtd/hisi504-nand.txt b/Documentation/devicetree/bindings/mtd/hisi504-nand.txt
> new file mode 100644
> index 0000000..c8b3988
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/hisi504-nand.txt
> @@ -0,0 +1,40 @@
> +Hisilicon Hip04 Soc NAND controller DT binding
> +
> +Required properties:
> +- compatible:          Should be "hisilicon,504-nfc".
> +- 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.

> +- 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

> +- #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";
> +			reg = <0x00000000 0x00400000>;
> +		};
> +
> +		...
> +
> +	};

Brian

Re: [PATCH v4 2/2] mtd: hisilicon: add device tree binding documentation

[Zhou Wang]

On 2014年11月30日 16:56, Brian Norris wrote:
> On Tue, Nov 04, 2014 at 08:47:01PM +0800, Zhou Wang wrote:
>> Signed-off-by: Zhou Wang <wangzhou.bry-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> ---
>>   .../devicetree/bindings/mtd/hisi504-nand.txt       |   40 ++++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/mtd/hisi504-nand.txt
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/hisi504-nand.txt b/Documentation/devicetree/bindings/mtd/hisi504-nand.txt
>> new file mode 100644
>> index 0000000..c8b3988
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mtd/hisi504-nand.txt
>> @@ -0,0 +1,40 @@
>> +Hisilicon Hip04 Soc NAND controller DT binding
>> +
>> +Required properties:
>> +- compatible:          Should be "hisilicon,504-nfc".
>> +- 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.
>

The driver just supports "hw" mode, will modify this.

>> +- 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

Thanks, will modify this!

>
>> +- #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";
>> +			reg = <0x00000000 0x00400000>;
>> +		};
>> +
>> +		...
>> +
>> +	};
>
> Brian
>

Thanks,
Zhou Wang

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] mtd: hisilicon: add device tree binding documentation

[Brian Norris]

One more thing:

On Tue, Nov 04, 2014 at 08:47:01PM +0800, Zhou Wang wrote:
> diff --git a/Documentation/devicetree/bindings/mtd/hisi504-nand.txt b/Documentation/devicetree/bindings/mtd/hisi504-nand.txt
> new file mode 100644
> index 0000000..c8b3988
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mtd/hisi504-nand.txt
> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 2/2] mtd: hisilicon: add device tree binding documentation

[Zhou Wang]

On 2014年11月30日 17:01, Brian Norris wrote:
> One more thing:
>
> On Tue, Nov 04, 2014 at 08:47:01PM +0800, Zhou Wang wrote:
>> diff --git a/Documentation/devicetree/bindings/mtd/hisi504-nand.txt b/Documentation/devicetree/bindings/mtd/hisi504-nand.txt
>> new file mode 100644
>> index 0000000..c8b3988
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/mtd/hisi504-nand.txt
>> @@ -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.
>

I think that is a good idear to call "hisilicon,504-nand", in fact, at
the beginning, I also confused with the name "nfc".

> Brian
>

Thanks,
Zhou Wang


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc

[Haojian Zhuang]

On Tue, Nov 4, 2014 at 8:46 PM, Zhou Wang <wangzhou.bry-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 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/linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/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
>

Acked-by: Haojian Zhuang <haojian.zhuang-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc

[Haojian Zhuang]

On Tue, Nov 4, 2014 at 8:46 PM, Zhou Wang <wangzhou.bry-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> 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/linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/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
>

Hi David & Brian,

How do you think about this nand patch set? Could it be merged into v3.19?

Best Regards
Haojian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc

[Brian Norris]

On Tue, Nov 04, 2014 at 08:46:59PM +0800, Zhou Wang wrote:
> 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

Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc

[Zhou Wang]

On 2014年11月30日 17:08, Brian Norris wrote:
> On Tue, Nov 04, 2014 at 08:46:59PM +0800, Zhou Wang wrote:
>> 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
>

Hi Brian,

I have not tested on the MTD test modules, I will do this test and
the UBI tests ASAP. Thank you very much for so many helpful comments!

Best Regards,
Zhou Wang
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc

[Zhou Wang]

On 2014年11月30日 17:08, Brian Norris wrote:
> On Tue, Nov 04, 2014 at 08:46:59PM +0800, Zhou Wang wrote:
>> 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
>

Hi 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.

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.

Thanks,
Zhou Wang
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc

[Brian Norris]

On Thu, Dec 11, 2014 at 08:02:27PM +0800, Zhou Wang wrote:
> On 2014年11月30日 17:08, Brian Norris wrote:
> >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.

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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc

[Zhou Wang]

On 2014年12月17日 09:03, Brian Norris wrote:
> On Thu, Dec 11, 2014 at 08:02:27PM +0800, Zhou Wang wrote:
>> On 2014年11月30日 17:08, Brian Norris wrote:
>>> 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.
>
> 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
>

Got it, thanks for your explanation, Brian. I will resend the new
version of patchset based on v3.19-rc1 later.

Regards,
Zhou

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc

[Brian Norris]

On Thu, Dec 11, 2014 at 08:02:27PM +0800, Zhou Wang wrote:
> 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.

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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc

[Zhou Wang]

On 2014年12月17日 14:23, Brian Norris wrote:
> On Thu, Dec 11, 2014 at 08:02:27PM +0800, Zhou Wang wrote:
>> 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.
>
> 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".

>
> 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.

> 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.

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.

Thanks a lot,
Zhou

>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc

[Brian Norris]

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

On Wed, Dec 17, 2014 at 07:05:47PM +0800, Zhou Wang wrote:
> On 2014年12月17日 14:23, Brian Norris wrote:
[...]
> >>[  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.
> >
> >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".

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.

> >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.
> 
> >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.
> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc

[Zhou Wang]

Hi 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.

On 2015/1/13 12:17, Brian Norris wrote:
> Following up on this last comment from last year's thread:
> 
> On Wed, Dec 17, 2014 at 07:05:47PM +0800, Zhou Wang wrote:
>> On 2014年12月17日 14:23, Brian Norris wrote:
> [...]
>>>> [  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.
>>>
>>> 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".
> 
> 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.

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: MTD device: 2
mtd_nandbiterrs: MTD device size 8388608, eraseblock=131072, page=2048, oob=64
mtd_nandbiterrs: Device uses 2 subpages of 1024 bytes
mtd_nandbiterrs: Using page=1, offset=2048, eraseblock=0
mtd_nandbiterrs: incremental biterrors test
mtd_nandbiterrs: write_page

ECC code:
96 ec 7e c0 dc 8e 38 68 e7 29 6a f8 f3 c1 9e 4e 9b 2e b7 31 40 46 88 ec cf 65 55 94 28 07 09 3c
c5 1c f6 cb 3e c0 26 d7 cf 2d 15 77 59 e8 5f fd a7 cc be 17 cb ee 39 de

mtd_nandbiterrs: rewrite page

ECC code:
96 ec 7e c0 dc 8e 38 68 e7 29 6a f8 f3 c1 9e 4e 9b 2e b7 31 40 46 88 ec cf 65 55 94 28 07 09 3c
c5 1c f6 cb 3e c0 26 d7 cf 2d 15 77 59 e8 5f fd a7 cc be 17 cb ee 39 de

mtd_nandbiterrs: read_page

ECC code:
96 ec 7e c0 dc 8e 38 68 e7 29 6a f8 f3 c1 9e 4e 9b 2e b7 31 40 46 88 ec cf 65 55 94 28 07 09 3c
c5 1c f6 cb 3e c0 26 d7 cf 2d 15 77 59 e8 5f fd a7 cc be 17 cb ee 39 de

mtd_nandbiterrs: verify_page
mtd_nandbiterrs: Successfully corrected 0 bit errors per subpage
mtd_nandbiterrs: Inserted biterror @ 0/5
mtd_nandbiterrs: Inserted biterror @ 1024/2
mtd_nandbiterrs: rewrite page

ECC code:(the ECC code in NAND controller buffer, which will be wrote to NAND flash)
11 70 40 2e ab 6e 17 34 1d 2d 2b b0 51 a4 c1 af 05 a6 44 12 25 f1 10 49 7d 0b bd 95 28 07 09 3c
c5 1c f6 cb 3e c0 26 d7 cf 2d 15 77 59 e8 5f fd a7 cc be 17 cb ee 39 de

mtd_nandbiterrs: read_page

ECC code:
10 60 40 00 88 0e 10 20 05 29 2a b0 51 80 80 0e 01 26 04 10 00 40 00 48 4d 01 15 94 28 07 09 3c
c5 1c f6 cb 3e c0 26 d7 cf 2d 15 77 59 e8 5f fd a7 cc be 17 cb ee 39 de

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: MTD device: 2
mtd_nandbiterrs: MTD device size 8388608, eraseblock=131072, page=2048, oob=64
mtd_nandbiterrs: Device uses 4 subpages of 512 bytes
mtd_nandbiterrs: Using page=1, offset=2048, eraseblock=0
mtd_nandbiterrs: incremental biterrors test
mtd_nandbiterrs: write_page

ECC code:
ff ff 3f ff ff ff ff ff 3f ff ff ff

mtd_nandbiterrs: rewrite page

ECC code:
ff ff 3f ff ff ff ff ff 3f ff ff ff

mtd_nandbiterrs: read_page

ECC code:
ff ff 3f ff ff ff ff ff 3f ff ff ff

mtd_nandbiterrs: verify_page
mtd_nandbiterrs: Successfully corrected 0 bit errors per subpage
mtd_nandbiterrs: Inserted biterror @ 0/5
mtd_nandbiterrs: Inserted biterror @ 512/6
mtd_nandbiterrs: Inserted biterror @ 1024/2
mtd_nandbiterrs: Inserted biterror @ 1536/6
mtd_nandbiterrs: rewrite page

ECC code:(the ECC code in NAND controller buffer, which will be wrote to NAND flash)
aa aa 59 aa aa 96 aa aa 66 aa aa 96

mtd_nandbiterrs: read_page

ECC code:
aa aa 19 aa aa 96 aa aa 26 aa aa 96

mtd_nandbiterrs: Read reported 2 corrected bit errors
mtd_nandbiterrs: verify_page
mtd_nandbiterrs: Error: page offset 0, expected 23, got 03
mtd_nandbiterrs: Error: page offset 512, expected 69, got 29
mtd_nandbiterrs: Error: page offset 1024, expected 06, got 02
mtd_nandbiterrs: Error: page offset 1536, expected 4c, got 0c
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:
As the log above, in rewriting process, the ECC code should be "aa aa 59 aa aa 96 aa aa 66 aa aa 96",
however actually it writes ECC code "aa aa 19 aa aa 96 aa aa 26 aa aa 96" to
flash. ECC code of the first and third 512B of 2k page in flash are "aa aa 19"
which each has 1bit error in ECC code. Taking the first 512B as an example,
there are 2bit errors: 1bit in page data and 1bit in related ECC code.
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.

> 
>>> 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.
>>
>>> 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.
>>
>> 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.

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.

> 
> Brian

Many thanks for your comments!
Zhou Wang

> 
> .
> 


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc

[Brian Norris]

Hi Zhou,

On Thu, Jan 22, 2015 at 11:27:01AM +0800, Zhou Wang wrote:
> 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.

> On 2015/1/13 12:17, Brian Norris wrote:
> > On Wed, Dec 17, 2014 at 07:05:47PM +0800, Zhou Wang wrote:
> >> On 2014年12月17日 14:23, Brian Norris wrote:
> > [...]
> >>>> [  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

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

> 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.

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

> 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?

> > 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.

> 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.

> 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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc

[Zhou Wang]

On 2015/1/22 16:45, Brian Norris wrote:
> Hi Zhou,
> 
> On Thu, Jan 22, 2015 at 11:27:01AM +0800, Zhou Wang wrote:
>> 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.
> 
>> On 2015/1/13 12:17, Brian Norris wrote:
>>> On Wed, Dec 17, 2014 at 07:05:47PM +0800, Zhou Wang wrote:
>>>> On 2014年12月17日 14:23, Brian Norris wrote:
>>> [...]
>>>>>> [  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
> 
> ^^^ Ah, that's what I would expect from a driver that doesn't implement
> the raw() functions.
> 
>> 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.
> 
> IOW, uncorrectable errors are getting reported as corrected bitflips?
> That does sound bad.

Yes, so I will not support 1bit ECC mode in patch of next version.

> 
>> 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?
>

No, different ECC modes have different ECC layouts in oob area.

>>> 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.
> 
>> 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?

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.

Many thanks for your reply,
Zhou Wang

.
> 
> At any rate, this isn't absolutely required.
> 
>> 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
> 
> .
> 


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/2] mtd: hisilicon: add a new driver for NAND controller of hisilicon hip04 Soc

[Brian Norris]

On Sun, Jan 25, 2015 at 04:19:43PM +0800, Zhou Wang wrote:
> On 2015/1/22 16:45, Brian Norris wrote:
> > 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.

Ugh, that's no fun. GPMI seems to do a similarly silly thing.

> 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.

OK. There may eventually be good reasons to do this still, but it's not
important right now.

Brian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html