[PATCH v3 0/8] Introduction to SPI NAND framework

[PATCH v3 0/8] Introduction to SPI NAND framework

[Peter Pan]

First of all, thanks for Boris Brezillon, Arnaud Mouiche and
Thomas Petazzoni. This v3 cannot come out without your valuable
comments on v2.

This series introductes a SPI NAND framework.
SPI NAND is a new NAND family device with SPI protocol as
its interface. And its command set is totally different
with parallel NAND.

Our first attempt was more than 2 years ago[1]. At that
time, I didn't make BBT shareable and there were too many
duplicate code with parallel NAND, so that serie stoped.
But the discussion never stops. Now Boris has a plan to
make a generic NAND framework which can be shared with
both parallel and SPI NAND. Now the first step of the
new generic NAND framework is finished. And it is waiting
for a user. After discussion with Boris. We both think it's
time to rebuild SPI NAND framework based on the new NAND
framework and send out for reviewing.

This series is based on Boris's nand/generic branch[2], which
is on 4.11-rc1. In this serie, BBT code is totally shared.
Of course SPI NAND can share more code with parallel, this
requires to put more in new NAND core (now only BBT included).
I'd like to send this serie out first, then we can decide
which part should be in new NAND core.

This series only supports basic SPI NAND features and uses
generic spi controller for data transfer, on-die ECC for data
correction. Support advanced features and specific SPI NAND
controller with hardware ECC is the next step.

This series is tested on Xilinx Zedboard with Micron
MT29F2G01ABAGDSF SPI NAND chip.


[1]http://lists.infradead.org/pipermail/linux-mtd/2015-January/057223.html
[2]https://github.com/bbrezillon/linux-0day/tree/nand/generic

v3 changes:
- rebase patch on 4.11-rc1[1]
- change read ID method. read 4 bytes ID out then let ->detect() of each
  manufacutre driver to decode ID and detect the device.
- make SPI NAND id table private to each manufacutre driver
- fix coding style to make checkpatch.pl happy
- update the MAINTAINERS file for spi nand code
- add nand_size() helper in nand.h
- use nand_for_each_page() helper in spinand_do_read/write_ops()
- create helper to check boundaries in generic NAND code and use it
  in SPI NAND core
- rename spinand_base.c to core.c
- manufactures' drivers expose spinand_manufacturer struct instead of
  spinand_manufacturer_ops struct to keep Manufacture ID macro in
  manufactures' drivers and rename spinand_ids.c to manufacture.c
- rename spinand_micron.c to micron.c
- rename chips/ directory to controllers/
- rename generic_spi.c to generic-spi.c
- replace ->build_column_addr() and ->get_dummy() hooks with ->prepare_op() in
  spinand_manufacturer_ops struct
- rename __spinand_erase() to spinand_erase()
- rename spinand_erase() to spinand_erase_skip_bbt()
- rename spinand_scan_ident() to spinand_detect()
- rename spinand_scan_tail() to spinand_init()
- move non detect related code from spinand_detect() to spinand_init()
- remove spinand_fill_nandd, assign nand->ops in spinand_detect()
- merge v2 patch 3(bad block support) and patch 4(BBT support)
- drop getchip parameter, remove spinand_get/remove_device(), take the lock
  by caller directly
- fix function comment headers
- use nand_bbt_is_initialized() helper
- replace spinand_ecc_engine and spinand_controller object in spinand_device
  struct with pointer
- replace struct spinand_manufacturer_ops pointer in spinand_device struct
  with spinand_manufacturer struct


v2 changes:
- replace "spi_nand" with "spinand".
- rename spi nand related structs for better understanding.
- introduce spi nand controller, manufacturer and ecc_engine struct.
- add spi nand manufacturer initialization function refer to Boris's
  manuf-init branch.
- remove NAND_SKIP_BBTSCAN from series. Add it later when enabling HW ECC.
- reorganize series according to Boris's suggestion.

Peter Pan (8):
  mtd: nand: add more helpers in nand.h
  mtd: nand: add oob iterator in nand_for_each_page
  nand: spi: add basic blocks for infrastructure
  nand: spi: add basic operations support
  nand: spi: Add bad block support
  nand: spi: add Micron spi nand support
  nand: spi: Add generic SPI controller support
  MAINTAINERS: Add SPI NAND entry

 MAINTAINERS                                    |    9 +
 drivers/mtd/nand/Kconfig                       |    1 +
 drivers/mtd/nand/Makefile                      |    1 +
 drivers/mtd/nand/spi/Kconfig                   |    7 +
 drivers/mtd/nand/spi/Makefile                  |    4 +
 drivers/mtd/nand/spi/controllers/Kconfig       |    5 +
 drivers/mtd/nand/spi/controllers/Makefile      |    1 +
 drivers/mtd/nand/spi/controllers/generic-spi.c |  158 +++
 drivers/mtd/nand/spi/core.c                    | 1456 ++++++++++++++++++++++++
 drivers/mtd/nand/spi/manufactures.c            |   27 +
 drivers/mtd/nand/spi/micron.c                  |  243 ++++
 include/linux/mtd/nand.h                       |   87 +-
 include/linux/mtd/spinand.h                    |  285 +++++
 13 files changed, 2281 insertions(+), 3 deletions(-)
 create mode 100644 drivers/mtd/nand/spi/Kconfig
 create mode 100644 drivers/mtd/nand/spi/Makefile
 create mode 100644 drivers/mtd/nand/spi/controllers/Kconfig
 create mode 100644 drivers/mtd/nand/spi/controllers/Makefile
 create mode 100644 drivers/mtd/nand/spi/controllers/generic-spi.c
 create mode 100644 drivers/mtd/nand/spi/core.c
 create mode 100644 drivers/mtd/nand/spi/manufactures.c
 create mode 100644 drivers/mtd/nand/spi/micron.c
 create mode 100644 include/linux/mtd/spinand.h

-- 
1.9.1

[PATCH v3 1/8] mtd: nand: add more helpers in nand.h

[Peter Pan]

This commit adds some helpers in nand.h
    nand_size()
    valid_nand_address()
    valid_nand_oob_ops()
    nand_oob_ops_across_page()
    valid_nand_erase_ops()

Signed-off-by: Peter Pan <peterpandong@micron.com>
---
 include/linux/mtd/nand.h | 59 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index c2197b4..4a812e6 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -410,6 +410,65 @@ static inline int nand_neraseblocks(struct nand_device *nand)
 }
 
 /**
+ * nand_size - Get NAND size
+ * @nand: NAND device
+ *
+ * Returns the total size exposed by @nand.
+ */
+static inline u64 nand_size(struct nand_device *nand)
+{
+	return nand->memorg.ndies * nand->memorg.diesize;
+}
+
+static inline bool valid_nand_address(struct nand_device *nand, loff_t addr)
+{
+	return addr < nand_size(nand);
+}
+
+static inline bool valid_nand_oob_ops(struct nand_device *nand, loff_t start,
+				      struct mtd_oob_ops *ops)
+{
+	struct mtd_info *mtd = nand_to_mtd(nand);
+	int ooblen = ops->mode == MTD_OPS_AUTO_OOB ?
+		mtd->oobavail : mtd->oobsize;
+
+	if (ops->ooboffs >= ooblen)
+		return false;
+	if (ops->ooboffs + ops->ooblen >
+	    (nand_len_to_pages(nand, nand_size(nand)) -
+			       nand_offs_to_page(nand, start)) * ooblen)
+		return false;
+
+	return true;
+}
+
+static inline bool nand_oob_ops_across_page(struct nand_device *nand,
+					    struct mtd_oob_ops *ops)
+{
+	struct mtd_info *mtd = nand_to_mtd(nand);
+	int ooblen = ops->mode == MTD_OPS_AUTO_OOB ?
+		     mtd->oobavail : mtd->oobsize;
+
+	return (ops->ooboffs + ops->ooblen) > ooblen;
+}
+
+static inline bool valid_nand_erase_ops(struct nand_device *nand,
+					struct erase_info *einfo)
+{
+	/* check address align on block boundary */
+	if (einfo->addr & (nand_eraseblock_size(nand) - 1))
+		return false;
+	/* check lendth align on block boundary */
+	if (einfo->len & (nand_eraseblock_size(nand) - 1))
+		return false;
+	/* Do not allow erase past end of device */
+	if ((einfo->addr + einfo->len) > nand_size(nand))
+		return false;
+
+	return true;
+}
+
+/**
  * nand_register - Register a NAND device
  * @nand: NAND device
  *
-- 
1.9.1

[PATCH v3 2/8] mtd: nand: add oob iterator in nand_for_each_page

[Peter Pan]

Iterate nand pages by both page and oob operation.

Signed-off-by: Peter Pan <peterpandong@micron.com>
---
 include/linux/mtd/nand.h | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 4a812e6..822547e 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -87,6 +87,9 @@ struct nand_page_iter {
 	loff_t offs;
 	int page;
 	int pageoffs;
+	int ooboffs;
+	int oobsize;
+	size_t oobleft;
 };
 
 /**
@@ -194,6 +197,7 @@ static inline int nand_per_page_oobsize(struct nand_device *nand)
  * @iter: page iterator
  */
 static inline void nand_page_iter_init(struct nand_device *nand, loff_t offs,
+				       u32 ooboffs, size_t ooblen, u32 oobsize,
 				       struct nand_page_iter *iter)
 {
 	u64 page = offs;
@@ -201,6 +205,9 @@ static inline void nand_page_iter_init(struct nand_device *nand, loff_t offs,
 	iter->pageoffs = do_div(page, nand->memorg.pagesize);
 	iter->page = page;
 	iter->offs = offs;
+	iter->ooboffs = ooboffs;
+	iter->oobsize = oobsize;
+	iter->oobleft = ooblen;
 }
 
 /**
@@ -214,11 +221,26 @@ static inline void nand_page_iter_next(struct nand_device *nand,
 	iter->page++;
 	iter->offs += nand_page_size(nand) - iter->pageoffs;
 	iter->pageoffs = 0;
+	if (iter->oobleft)
+		iter->oobleft -= min_t(int, iter->oobsize - iter->ooboffs,
+				       iter->oobleft);
+}
+
+static inline bool nand_page_iter_end(struct nand_device *nand, loff_t offs,
+				      u32 len, struct nand_page_iter *iter)
+{
+	if (iter->offs < offs + len)
+		return false;
+	if (iter->oobleft)
+		return false;
+	return true;
 }
 
-#define nand_for_each_page(nand, start, len, iter)		\
-	for (nand_page_iter_init(nand, start, iter);		\
-	     (iter)->offs < (start) + (len);			\
+#define nand_for_each_page(nand, start, len, ooboffs, ooblen,	\
+			   oobsize, iter)	\
+	for (nand_page_iter_init(nand, start, ooboffs,		\
+				 ooblen, oobsize, iter);	\
+	     !nand_page_iter_end(nand, start, len, iter);		\
 	     nand_page_iter_next(nand, iter))
 
 /**
-- 
1.9.1

[PATCH v3 3/8] nand: spi: add basic blocks for infrastructure

[Peter Pan]

This is the first commit for spi nand framkework.
This commit is to add add basic building blocks
for the SPI NAND infrastructure.

Signed-off-by: Peter Pan <peterpandong@micron.com>
---
 drivers/mtd/nand/Kconfig            |   1 +
 drivers/mtd/nand/Makefile           |   1 +
 drivers/mtd/nand/spi/Kconfig        |   5 +
 drivers/mtd/nand/spi/Makefile       |   2 +
 drivers/mtd/nand/spi/core.c         | 430 ++++++++++++++++++++++++++++++++++++
 drivers/mtd/nand/spi/manufactures.c |  24 ++
 include/linux/mtd/spinand.h         | 283 ++++++++++++++++++++++++
 7 files changed, 746 insertions(+)
 create mode 100644 drivers/mtd/nand/spi/Kconfig
 create mode 100644 drivers/mtd/nand/spi/Makefile
 create mode 100644 drivers/mtd/nand/spi/core.c
 create mode 100644 drivers/mtd/nand/spi/manufactures.c
 create mode 100644 include/linux/mtd/spinand.h

diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig
index 1c1a1f4..7695fd8 100644
--- a/drivers/mtd/nand/Kconfig
+++ b/drivers/mtd/nand/Kconfig
@@ -2,3 +2,4 @@ config MTD_NAND_CORE
 	tristate
 
 source "drivers/mtd/nand/raw/Kconfig"
+source "drivers/mtd/nand/spi/Kconfig"
diff --git a/drivers/mtd/nand/Makefile b/drivers/mtd/nand/Makefile
index fe430d9..6221958 100644
--- a/drivers/mtd/nand/Makefile
+++ b/drivers/mtd/nand/Makefile
@@ -3,3 +3,4 @@ obj-$(CONFIG_MTD_NAND_CORE) += nandcore.o
 nandcore-objs :=  bbt.o
 
 obj-y	+= raw/
+obj-$(CONFIG_MTD_SPI_NAND)	+= spi/
diff --git a/drivers/mtd/nand/spi/Kconfig b/drivers/mtd/nand/spi/Kconfig
new file mode 100644
index 0000000..d77c46e
--- /dev/null
+++ b/drivers/mtd/nand/spi/Kconfig
@@ -0,0 +1,5 @@
+menuconfig MTD_SPI_NAND
+	tristate "SPI NAND device Support"
+	depends on MTD_NAND
+	help
+	  This is the framework for the SPI NAND device drivers.
diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile
new file mode 100644
index 0000000..eabdb81
--- /dev/null
+++ b/drivers/mtd/nand/spi/Makefile
@@ -0,0 +1,2 @@
+obj-$(CONFIG_MTD_SPI_NAND) += core.o
+obj-$(CONFIG_MTD_SPI_NAND) += manufactures.o
diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
new file mode 100644
index 0000000..12a6eef
--- /dev/null
+++ b/drivers/mtd/nand/spi/core.c
@@ -0,0 +1,430 @@
+/*
+ *
+ * Copyright (c) 2009-2017 Micron Technology, Inc.
+ *
+ * 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.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/jiffies.h>
+#include <linux/mtd/spinand.h>
+#include <linux/slab.h>
+
+/*
+ * spinand_exec_op - execute SPI NAND operation by controller ->exec_op() hook
+ * @chip: SPI NAND device structure
+ * @op: pointer to spinand_op struct
+ */
+static inline int spinand_exec_op(struct spinand_device *chip,
+				  struct spinand_op *op)
+{
+	return chip->controller->ops->exec_op(chip, op);
+}
+
+/*
+ * spinand_op_init - initialize spinand_op struct
+ * @op: pointer to spinand_op struct
+ */
+static inline void spinand_op_init(struct spinand_op *op)
+{
+	memset(op, 0, sizeof(struct spinand_op));
+	op->addr_nbits = 1;
+	op->data_nbits = 1;
+}
+
+/*
+ * spinand_read_reg - send command 0Fh to read register
+ * @chip: SPI NAND device structure
+ * @reg; register to read
+ * @buf: buffer to store value
+ */
+static int spinand_read_reg(struct spinand_device *chip, u8 reg, u8 *buf)
+{
+	struct spinand_op op;
+	int ret;
+
+	spinand_op_init(&op);
+	op.cmd = SPINAND_CMD_GET_FEATURE;
+	op.n_addr = 1;
+	op.addr[0] = reg;
+	op.n_rx = 1;
+	op.rx_buf = buf;
+
+	ret = spinand_exec_op(chip, &op);
+	if (ret < 0)
+		pr_err("err: %d read register %d\n", ret, reg);
+
+	return ret;
+}
+
+/*
+ * spinand_write_reg - send command 1Fh to write register
+ * @chip: SPI NAND device structure
+ * @reg; register to write
+ * @buf: buffer stored value
+ */
+static int spinand_write_reg(struct spinand_device *chip, u8 reg, u8 *buf)
+{
+	struct spinand_op op;
+	int ret;
+
+	spinand_op_init(&op);
+	op.cmd = SPINAND_CMD_SET_FEATURE;
+	op.n_addr = 1;
+	op.addr[0] = reg;
+	op.n_tx = 1,
+	op.tx_buf = buf,
+
+	ret = spinand_exec_op(chip, &op);
+	if (ret < 0)
+		pr_err("err: %d write register %d\n", ret, reg);
+
+	return ret;
+}
+
+/*
+ * spinand_read_status - get status register value
+ * @chip: SPI NAND device structure
+ * @status: buffer to store value
+ * Description:
+ *   After read, write, or erase, the NAND device is expected to set the
+ *   busy status.
+ *   This function is to allow reading the status of the command: read,
+ *   write, and erase.
+ */
+static int spinand_read_status(struct spinand_device *chip, uint8_t *status)
+{
+	return spinand_read_reg(chip, REG_STATUS, status);
+}
+
+/*
+ * spinand_wait - wait until the command is done
+ * @chip: SPI NAND device structure
+ * @s: buffer to store status register value (can be NULL)
+ */
+static int spinand_wait(struct spinand_device *chip, u8 *s)
+{
+	unsigned long timeo = msecs_to_jiffies(400);
+	u8 status;
+
+	do {
+		spinand_read_status(chip, &status);
+		if ((status & STATUS_OIP_MASK) == STATUS_READY)
+			goto out;
+	} while (time_before(jiffies, timeo));
+
+	/*
+	 * Extra read, just in case the STATUS_READY bit has changed
+	 * since our last check
+	 */
+	spinand_read_status(chip, &status);
+out:
+	if (s)
+		*s = status;
+
+	return (status & STATUS_OIP_MASK) == STATUS_READY ? 0 :	-ETIMEDOUT;
+}
+
+/*
+ * spinand_read_id - send 9Fh command to get ID
+ * @chip: SPI NAND device structure
+ * @buf: buffer to store id
+ * Description:
+ *   Manufacturers' read ID method is not unique. Some need a dummy before
+ *   reading, some's ID has three byte.
+ *   This function send one byte opcode (9Fh) and then read
+ *   SPINAND_MAX_ID_LEN (4 currently) bytes. Manufacturer's detect function
+ *   need to filter out real ID from the 4 bytes.
+ */
+static int spinand_read_id(struct spinand_device *chip, u8 *buf)
+{
+	struct spinand_op op;
+
+	spinand_op_init(&op);
+	op.cmd = SPINAND_CMD_READ_ID;
+	op.n_rx = SPINAND_MAX_ID_LEN;
+	op.rx_buf = buf;
+
+	return spinand_exec_op(chip, &op);
+}
+
+/*
+ * spinand_reset - reset device by FFh command.
+ * @chip: SPI NAND device structure
+ */
+static int spinand_reset(struct spinand_device *chip)
+{
+	struct spinand_op op;
+	int ret;
+
+	spinand_op_init(&op);
+	op.cmd = SPINAND_CMD_RESET;
+
+	ret = spinand_exec_op(chip, &op);
+	if (ret < 0) {
+		pr_err("spinand reset failed!\n");
+		goto out;
+	}
+	ret = spinand_wait(chip, NULL);
+
+out:
+	return ret;
+}
+
+/*
+ * spinand_lock_block - write block lock register to lock/unlock device
+ * @chip: SPI NAND device structure
+ * @lock: value to set to block lock register
+ */
+static int spinand_lock_block(struct spinand_device *chip, u8 lock)
+{
+	return spinand_write_reg(chip, REG_BLOCK_LOCK, &lock);
+}
+
+/*
+ * spinand_set_rd_wr_op - choose the best read write command
+ * @chip: SPI NAND device structure
+ * Description:
+ *   Chose the fastest r/w command according to spi controller's and
+ *   device's ability.
+ */
+static void spinand_set_rd_wr_op(struct spinand_device *chip)
+{
+	u32 controller_cap = chip->controller->caps;
+	u32 rw_mode = chip->rw_mode;
+
+	if ((controller_cap & SPINAND_CAP_RD_QUAD) &&
+	    (rw_mode & SPINAND_RD_QUAD))
+		chip->read_cache_op = SPINAND_CMD_READ_FROM_CACHE_QUAD_IO;
+	else if ((controller_cap & SPINAND_CAP_RD_X4) &&
+		 (rw_mode & SPINAND_RD_X4))
+		chip->read_cache_op = SPINAND_CMD_READ_FROM_CACHE_X4;
+	else if ((controller_cap & SPINAND_CAP_RD_DUAL) &&
+		 (rw_mode & SPINAND_RD_DUAL))
+		chip->read_cache_op = SPINAND_CMD_READ_FROM_CACHE_DUAL_IO;
+	else if ((controller_cap & SPINAND_CAP_RD_X2) &&
+		 (rw_mode & SPINAND_RD_X2))
+		chip->read_cache_op = SPINAND_CMD_READ_FROM_CACHE_X2;
+	else
+		chip->read_cache_op = SPINAND_CMD_READ_FROM_CACHE_FAST;
+
+	if ((controller_cap & SPINAND_CAP_WR_X4) &&
+	    (rw_mode & SPINAND_WR_X4))
+		chip->write_cache_op = SPINAND_CMD_PROG_LOAD_X4;
+	else
+		chip->write_cache_op = SPINAND_CMD_PROG_LOAD;
+}
+
+/*
+ * spinand_manufacturer_detect - detect SPI NAND device by each manufacturer
+ * @chip: SPI NAND device structure
+ * @raw_id: raw id buffer. raw id is read by spinand_read_id(), should be
+ *          decoded by manufacturers
+ * @id: real id buffer. manufacturer's ->detect() should put real id in
+ *      this buffer.
+ *
+ * ->detect() should decode raw id and initialize device related part in
+ * spinand_device structure if it is the right device.
+ * ->detect() can not be NULL.
+ */
+static int spinand_manufacturer_detect(struct spinand_device *chip,
+				       u8 *raw_id, u8 *id)
+{
+	int i = 0;
+
+	for (; spinand_manufacturers[i]->id != 0x0; i++) {
+		if (spinand_manufacturers[i]->ops &&
+		    spinand_manufacturers[i]->ops->detect) {
+			if (spinand_manufacturers[i]->ops->detect(chip,
+								  raw_id, id))
+				return 0;
+		}
+	}
+
+	return -ENODEV;
+}
+
+/*
+ * spinand_manufacturer_init - manufacturer initialization function.
+ * @chip: SPI NAND device structure
+ *
+ * Manufacturer drivers should put all their specific initialization code in
+ * their ->init() hook.
+ */
+static int spinand_manufacturer_init(struct spinand_device *chip)
+{
+	if (chip->manufacturer.manu->ops && chip->manufacturer.manu->ops->init)
+		return chip->manufacturer.manu->ops->init(chip);
+
+	return 0;
+}
+
+/*
+ * spinand_manufacturer_cleanup - manufacturer cleanup function.
+ * @chip: SPI NAND device structure
+ *
+ * Manufacturer drivers should put all their specific cleanup code in their
+ * ->cleanup() hook.
+ */
+static void spinand_manufacturer_cleanup(struct spinand_device *chip)
+{
+	/* Release manufacturer private data */
+	if (chip->manufacturer.manu->ops &&
+	    chip->manufacturer.manu->ops->cleanup)
+		return chip->manufacturer.manu->ops->cleanup(chip);
+}
+
+/*
+ * spinand_fill_id - fill id in spinand_device structure.
+ * @chip: SPI NAND device structure
+ * @id: id buffer
+ */
+static void spinand_fill_id(struct spinand_device *chip, u8 *id)
+{
+	memcpy(chip->id.data, id, SPINAND_MAX_ID_LEN);
+	chip->id.len = SPINAND_MAX_ID_LEN;
+}
+
+/*
+ * spinand_get_mfr_id - get device's manufacturer ID.
+ * @chip: SPI NAND device structure
+ */
+static u8 spinand_get_mfr_id(struct spinand_device *chip)
+{
+	return chip->id.data[SPINAND_MFR_ID];
+}
+
+/*
+ * spinand_get_dev_id - get device's device ID.
+ * @chip: SPI NAND device structure
+ */
+static u8 spinand_get_dev_id(struct spinand_device *chip)
+{
+	return chip->id.data[SPINAND_DEV_ID];
+}
+
+/*
+ * spinand_set_manufacturer_ops - set manufacture ops in spinand_device
+ * structure.
+ * @chip: SPI NAND device structure
+ * @mfr_id: device's manufacturer ID
+ */
+static void spinand_set_manufacturer_ops(struct spinand_device *chip,
+					 u8 mfr_id)
+{
+	int i = 0;
+
+	for (; spinand_manufacturers[i]->id != 0x0; i++) {
+		if (spinand_manufacturers[i]->id == mfr_id)
+			break;
+	}
+
+	chip->manufacturer.manu = spinand_manufacturers[i];
+}
+
+/*
+ * spinand_detect - [SPI NAND Interface] detect the SPI NAND device
+ * @chip: SPI NAND device structure
+ */
+int spinand_detect(struct spinand_device *chip)
+{
+	struct nand_device *nand = &chip->base;
+	u8 raw_id[SPINAND_MAX_ID_LEN] = {0};
+	u8 id[SPINAND_MAX_ID_LEN] = {0};
+	int ret;
+
+	spinand_reset(chip);
+	spinand_read_id(chip, raw_id);
+	ret = spinand_manufacturer_detect(chip, raw_id, id);
+	if (ret)
+		return ret;
+
+	spinand_fill_id(chip, id);
+
+	pr_info("SPI NAND: %s is found.\n", chip->name);
+	pr_info("Manufacturer ID: 0x%02x, Chip ID: 0x%02x\n",
+		spinand_get_mfr_id(chip), spinand_get_dev_id(chip));
+	pr_info("%d MiB, block size: %d KiB, page size: %d, OOB size: %d\n",
+		(int)(nand_size(nand) >> 20), nand_eraseblock_size(nand) >> 10,
+		nand_page_size(nand), nand_per_page_oobsize(nand));
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(spinand_detect);
+
+/*
+ * spinand_init - [SPI NAND Interface] initialize the SPI NAND device
+ * @chip: SPI NAND device structure
+ */
+int spinand_init(struct spinand_device *chip)
+{
+	struct mtd_info *mtd = spinand_to_mtd(chip);
+	struct nand_device *nand = mtd_to_nand(mtd);
+	struct spinand_ecc_engine *ecc_engine = chip->ecc.engine;
+	int ret;
+
+	mutex_init(&chip->lock);
+	spinand_set_manufacturer_ops(chip, spinand_get_mfr_id(chip));
+	spinand_manufacturer_init(chip);
+	spinand_set_rd_wr_op(chip);
+	chip->buf = kzalloc(nand_page_size(nand) + nand_per_page_oobsize(nand),
+			    GFP_KERNEL);
+	if (!chip->buf)
+		return -ENOMEM;
+
+	chip->oobbuf = chip->buf + nand_page_size(nand);
+	mtd->name = chip->name;
+	mtd->size = nand_size(nand);
+	mtd->erasesize = nand_eraseblock_size(nand);
+	mtd->writesize = nand_page_size(nand);
+	mtd->writebufsize = mtd->writesize;
+	mtd->owner = THIS_MODULE;
+	mtd->type = MTD_NANDFLASH;
+	mtd->flags = MTD_CAP_NANDFLASH;
+	if (!mtd->ecc_strength)
+		mtd->ecc_strength = ecc_engine->strength ?
+				    ecc_engine->strength : 1;
+
+	mtd->oobsize = nand_per_page_oobsize(nand);
+	ret = mtd_ooblayout_count_freebytes(mtd);
+	if (ret < 0)
+		ret = 0;
+	mtd->oobavail = ret;
+
+	if (!mtd->bitflip_threshold)
+		mtd->bitflip_threshold = DIV_ROUND_UP(mtd->ecc_strength * 3,
+						      4);
+	/* After power up, all blocks are locked, so unlock it here. */
+	spinand_lock_block(chip, BL_ALL_UNLOCKED);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(spinand_init);
+
+/*
+ * spinand_cleanup - [SPI NAND Interface] clean up footprint of SPI NAND device
+ * @chip: SPI NAND device structure
+ */
+int spinand_cleanup(struct spinand_device *chip)
+{
+	spinand_manufacturer_cleanup(chip);
+	kfree(chip->buf);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(spinand_cleanup);
+
+MODULE_DESCRIPTION("SPI NAND framework");
+MODULE_AUTHOR("Peter Pan<peterpandong@micron.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/mtd/nand/spi/manufactures.c b/drivers/mtd/nand/spi/manufactures.c
new file mode 100644
index 0000000..7e0b42d
--- /dev/null
+++ b/drivers/mtd/nand/spi/manufactures.c
@@ -0,0 +1,24 @@
+/**
+ *
+ * Copyright (c) 2009-2017 Micron Technology, Inc.
+ *
+ * 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/module.h>
+#include <linux/mtd/spinand.h>
+
+struct spinand_manufacturer spinand_manufacturer_end = {0x0, "Unknown", NULL};
+
+struct spinand_manufacturer *spinand_manufacturers[] = {
+	&spinand_manufacturer_end,
+};
+EXPORT_SYMBOL(spinand_manufacturers);
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
new file mode 100644
index 0000000..4e785da
--- /dev/null
+++ b/include/linux/mtd/spinand.h
@@ -0,0 +1,283 @@
+/**
+ *
+ * Copyright (c) 2009-2017 Micron Technology, Inc.
+ *
+ * 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.
+ */
+#ifndef __LINUX_MTD_SPINAND_H
+#define __LINUX_MTD_SPINAND_H
+
+#include <linux/wait.h>
+#include <linux/spinlock.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/flashchip.h>
+#include <linux/mtd/nand.h>
+
+/*
+ * Standard SPI-NAND flash commands
+ */
+#define SPINAND_CMD_RESET			0xff
+#define SPINAND_CMD_GET_FEATURE			0x0f
+#define SPINAND_CMD_SET_FEATURE			0x1f
+#define SPINAND_CMD_PAGE_READ			0x13
+#define SPINAND_CMD_READ_PAGE_CACHE_RDM		0x30
+#define SPINAND_CMD_READ_PAGE_CACHE_LAST	0x3f
+#define SPINAND_CMD_READ_FROM_CACHE		0x03
+#define SPINAND_CMD_READ_FROM_CACHE_FAST	0x0b
+#define SPINAND_CMD_READ_FROM_CACHE_X2		0x3b
+#define SPINAND_CMD_READ_FROM_CACHE_DUAL_IO	0xbb
+#define SPINAND_CMD_READ_FROM_CACHE_X4		0x6b
+#define SPINAND_CMD_READ_FROM_CACHE_QUAD_IO	0xeb
+#define SPINAND_CMD_BLK_ERASE			0xd8
+#define SPINAND_CMD_PROG_EXC			0x10
+#define SPINAND_CMD_PROG_LOAD			0x02
+#define SPINAND_CMD_PROG_LOAD_RDM_DATA		0x84
+#define SPINAND_CMD_PROG_LOAD_X4		0x32
+#define SPINAND_CMD_PROG_LOAD_RDM_DATA_X4	0x34
+#define SPINAND_CMD_READ_ID			0x9f
+#define SPINAND_CMD_WR_DISABLE			0x04
+#define SPINAND_CMD_WR_ENABLE			0x06
+#define SPINAND_CMD_END				0x0
+
+/* feature registers */
+#define REG_BLOCK_LOCK		0xa0
+#define REG_CFG			0xb0
+#define REG_STATUS		0xc0
+#define REG_DIE_SELECT		0xd0
+
+/* status */
+#define STATUS_OIP_MASK		0x01
+#define STATUS_CRBSY_MASK	0x80
+#define STATUS_READY		(0 << 0)
+#define STATUS_BUSY		(1 << 0)
+
+#define STATUS_E_FAIL_MASK	0x04
+#define STATUS_E_FAIL		(1 << 2)
+
+#define STATUS_P_FAIL_MASK	0x08
+#define STATUS_P_FAIL		(1 << 3)
+
+/*Configuration register defines*/
+#define CFG_QE_MASK		0x01
+#define CFG_QE_ENABLE		0x01
+#define CFG_ECC_MASK		0x10
+#define CFG_ECC_ENABLE		0x10
+#define CFG_LOT_MASK		0x20
+#define CFG_LOT_ENABLE		0x20
+#define CFG_OTP_MASK		0xc2
+#define CFG_OTP_ENTER		0x40
+#define CFG_OTP_EXIT		0x00
+
+/* block lock */
+#define BL_ALL_LOCKED		0x7c
+#define BL_U_1_1024_LOCKED		0x08
+#define BL_U_1_512_LOCKED		0x10
+#define BL_U_1_256_LOCKED		0x18
+#define BL_U_1_128_LOCKED		0x20
+#define BL_U_1_64_LOCKED		0x28
+#define BL_U_1_32_LOCKED		0x30
+#define BL_U_1_16_LOCKED		0x38
+#define BL_U_1_8_LOCKED		0x40
+#define BL_U_1_4_LOCKED		0x48
+#define BL_U_1_2_LOCKED		0x50
+#define BL_L_1_1024_LOCKED		0x0c
+#define BL_L_1_512_LOCKED		0x14
+#define BL_L_1_256_LOCKED		0x1c
+#define BL_L_1_128_LOCKED		0x24
+#define BL_L_1_64_LOCKED		0x2c
+#define BL_L_1_32_LOCKED		0x34
+#define BL_L_1_16_LOCKED		0x3c
+#define BL_L_1_8_LOCKED		0x44
+#define BL_L_1_4_LOCKED		0x4c
+#define BL_L_1_2_LOCKED		0x54
+#define BL_ALL_UNLOCKED		0X00
+
+/* die select */
+#define DIE_SELECT_MASK		0x40
+#define DIE_SELECT_DS0		0x00
+#define DIE_SELECT_DS1		0x40
+
+struct spinand_op;
+struct spinand_device;
+
+#define SPINAND_MAX_ID_LEN		4
+#define SPINAND_MFR_ID		0
+#define SPINAND_DEV_ID		1
+/**
+ * struct nand_id - NAND id structure
+ * @data: buffer containing the id bytes. Currently 8 bytes large, but can
+ *	  be extended if required.
+ * @len: ID length.
+ */
+struct spinand_id {
+	u8 data[SPINAND_MAX_ID_LEN];
+	int len;
+};
+
+struct spinand_controller_ops {
+	int (*exec_op)(struct spinand_device *chip,
+		       struct spinand_op *op);
+};
+
+struct spinand_manufacturer_ops {
+	bool (*detect)(struct spinand_device *chip, u8 *raw_id, u8 *id);
+	int (*init)(struct spinand_device *chip);
+	void (*cleanup)(struct spinand_device *chip);
+};
+
+struct spinand_manufacturer {
+	u8 id;
+	char *name;
+	const struct spinand_manufacturer_ops *ops;
+};
+
+extern struct spinand_manufacturer *spinand_manufacturers[];
+
+struct spinand_ecc_engine_ops {
+	void (*get_ecc_status)(struct spinand_device *chip,
+			       unsigned int status, unsigned int *corrected,
+			       unsigned int *ecc_errors);
+	void (*disable_ecc)(struct spinand_device *chip);
+	void (*enable_ecc)(struct spinand_device *chip);
+};
+
+enum spinand_ecc_mode {
+	SPINAND_ECC_ONDIE,
+	SPINAND_ECC_HW,
+};
+
+struct spinand_ecc_engine {
+	enum spinand_ecc_mode mode;
+	u32 strength;
+	u32 steps;
+	struct spinand_ecc_engine_ops *ops;
+};
+
+#define SPINAND_CAP_RD_X1 BIT(0)
+#define SPINAND_CAP_RD_X2 BIT(1)
+#define SPINAND_CAP_RD_X4 BIT(2)
+#define SPINAND_CAP_RD_DUAL BIT(3)
+#define SPINAND_CAP_RD_QUAD BIT(4)
+#define SPINAND_CAP_WR_X1 BIT(5)
+#define SPINAND_CAP_WR_X2 BIT(6)
+#define SPINAND_CAP_WR_X4 BIT(7)
+#define SPINAND_CAP_WR_DUAL BIT(8)
+#define SPINAND_CAP_WR_QUAD BIT(9)
+#define SPINAND_CAP_HW_ECC BIT(10)
+struct spinand_controller {
+	struct spinand_controller_ops *ops;
+	u32 caps;
+	void *priv;
+};
+
+/**
+ * struct spinand_device - SPI-NAND Private Flash Chip Data
+ * @base: NAND device instance
+ * @lock: protection lock
+ * @name: name of the chip
+ * @id: ID structure
+ * @read_cache_op: Opcode of read from cache
+ * @write_cache_op: Opcode of program load
+ * @buf: buffer for read/write data
+ * @oobbuf: buffer for read/write oob
+ * @rw_mode: read/write mode of SPI NAND chip
+ * @controller: SPI NAND controller instance
+ * @manufacturer: SPI NAND manufacturer instance, describe
+ *                manufacturer related objects
+ * @ecc_engine: SPI NAND ECC engine instance
+ */
+struct spinand_device {
+	struct nand_device base;
+	struct mutex lock;
+	char *name;
+	struct spinand_id id;
+	u8 read_cache_op;
+	u8 write_cache_op;
+	u8 *buf;
+	u8 *oobbuf;
+	u32 rw_mode;
+	struct spinand_controller *controller;
+	struct {
+		const struct spinand_manufacturer *manu;
+		void *priv;
+	} manufacturer;
+	struct {
+		struct spinand_ecc_engine *engine;
+		void *context;
+	} ecc;
+};
+
+static inline struct spinand_device *mtd_to_spinand(struct mtd_info *mtd)
+{
+	return container_of(mtd_to_nand(mtd), struct spinand_device, base);
+}
+
+static inline struct mtd_info *spinand_to_mtd(struct spinand_device *chip)
+{
+	return nand_to_mtd(&chip->base);
+}
+
+static inline void spinand_set_controller_data(struct spinand_device *chip,
+					       void *data)
+{
+	chip->controller->priv = data;
+}
+
+static inline void *spinand_get_controller_data(struct spinand_device *chip)
+{
+	return chip->controller->priv;
+}
+
+#define SPINAND_MAX_ADDR_LEN		4
+
+struct spinand_op {
+	u8 cmd;
+	u8 n_addr;
+	u8 addr_nbits;
+	u8 dummy_bytes;
+	u8 addr[SPINAND_MAX_ADDR_LEN];
+	u32 n_tx;
+	const u8 *tx_buf;
+	u32 n_rx;
+	u8 *rx_buf;
+	u8 data_nbits;
+};
+
+struct spinand_op_def {
+	u8 opcode;
+	u8 addr_bytes;
+	u8 addr_bits;
+	u8 dummy_bytes;
+	u8 data_bits;
+};
+
+/* SPI NAND supported OP mode */
+#define SPINAND_RD_X1		0x00000001
+#define SPINAND_RD_X2		0x00000002
+#define SPINAND_RD_X4		0x00000004
+#define SPINAND_RD_DUAL		0x00000008
+#define SPINAND_RD_QUAD		0x00000010
+#define SPINAND_WR_X1		0x00000020
+#define SPINAND_WR_X2		0x00000040
+#define SPINAND_WR_X4		0x00000080
+#define SPINAND_WR_DUAL		0x00000100
+#define SPINAND_WR_QUAD		0x00000200
+
+#define SPINAND_RD_COMMON	(SPINAND_RD_X1 | SPINAND_RD_X2 | \
+				 SPINAND_RD_X4 | SPINAND_RD_DUAL | \
+				 SPINAND_RD_QUAD)
+#define SPINAND_WR_COMMON	(SPINAND_WR_X1 | SPINAND_WR_X4)
+#define SPINAND_OP_COMMON	(SPINAND_RD_COMMON | SPINAND_WR_COMMON)
+
+int spinand_detect(struct spinand_device *chip);
+int spinand_init(struct spinand_device *chip);
+int spinand_cleanup(struct spinand_device *chip);
+#endif /* __LINUX_MTD_SPINAND_H */
-- 
1.9.1

[PATCH v3 4/8] nand: spi: add basic operations support

[Peter Pan]

This commit is to support read, readoob, write,
writeoob and erase operations in the new spi nand
framework.

Signed-off-by: Peter Pan <peterpandong@micron.com>
---
 drivers/mtd/nand/spi/core.c | 741 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/mtd/spinand.h |   2 +
 2 files changed, 743 insertions(+)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 12a6eef..7834c3d 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -109,6 +109,232 @@ static int spinand_read_status(struct spinand_device *chip, uint8_t *status)
 }
 
 /*
+ * spinand_get_cfg - get configuration register value
+ * @chip: SPI NAND device structure
+ * @cfg: buffer to store value
+ * Description:
+ *   Configuration register includes OTP config, Lock Tight enable/disable
+ *   and Internal ECC enable/disable.
+ */
+static int spinand_get_cfg(struct spinand_device *chip, u8 *cfg)
+{
+	return spinand_read_reg(chip, REG_CFG, cfg);
+}
+
+/*
+ * spinand_set_cfg - set value to configuration register
+ * @chip: SPI NAND device structure
+ * @cfg: buffer stored value
+ * Description:
+ *   Configuration register includes OTP config, Lock Tight enable/disable
+ *   and Internal ECC enable/disable.
+ */
+static int spinand_set_cfg(struct spinand_device *chip, u8 *cfg)
+{
+	return spinand_write_reg(chip, REG_CFG, cfg);
+}
+
+/*
+ * spinand_enable_ecc - enable internal ECC
+ * @chip: SPI NAND device structure
+ */
+static void spinand_enable_ecc(struct spinand_device *chip)
+{
+	u8 cfg = 0;
+
+	spinand_get_cfg(chip, &cfg);
+	if ((cfg & CFG_ECC_MASK) == CFG_ECC_ENABLE)
+		return;
+	cfg |= CFG_ECC_ENABLE;
+	spinand_set_cfg(chip, &cfg);
+}
+
+/*
+ * spinand_disable_ecc - disable internal ECC
+ * @chip: SPI NAND device structure
+ */
+static void spinand_disable_ecc(struct spinand_device *chip)
+{
+	u8 cfg = 0;
+
+	spinand_get_cfg(chip, &cfg);
+	if ((cfg & CFG_ECC_MASK) == CFG_ECC_ENABLE) {
+		cfg &= ~CFG_ECC_ENABLE;
+		spinand_set_cfg(chip, &cfg);
+	}
+}
+
+/*
+ * spinand_write_enable - send command 06h to enable write or erase the
+ * NAND cells
+ * @chip: SPI NAND device structure
+ */
+static int spinand_write_enable(struct spinand_device *chip)
+{
+	struct spinand_op op;
+
+	spinand_op_init(&op);
+	op.cmd = SPINAND_CMD_WR_ENABLE;
+
+	return spinand_exec_op(chip, &op);
+}
+
+/*
+ * spinand_read_page_to_cache - send command 13h to read data from NAND array
+ * to cache
+ * @chip: SPI NAND device structure
+ * @page_addr: page to read
+ */
+static int spinand_read_page_to_cache(struct spinand_device *chip,
+				      u32 page_addr)
+{
+	struct spinand_op op;
+
+	spinand_op_init(&op);
+	op.cmd = SPINAND_CMD_PAGE_READ;
+	op.n_addr = 3;
+	op.addr[0] = (u8)(page_addr >> 16);
+	op.addr[1] = (u8)(page_addr >> 8);
+	op.addr[2] = (u8)page_addr;
+
+	return spinand_exec_op(chip, &op);
+}
+
+/*
+ * spinand_get_address_bits - return address should be transferred
+ * by how many bits
+ * @opcode: command's operation code
+ */
+static int spinand_get_address_bits(u8 opcode)
+{
+	switch (opcode) {
+	case SPINAND_CMD_READ_FROM_CACHE_QUAD_IO:
+		return 4;
+	case SPINAND_CMD_READ_FROM_CACHE_DUAL_IO:
+		return 2;
+	default:
+		return 1;
+	}
+}
+
+/*
+ * spinand_get_data_bits - return data should be transferred by how many bits
+ * @opcode: command's operation code
+ */
+static int spinand_get_data_bits(u8 opcode)
+{
+	switch (opcode) {
+	case SPINAND_CMD_READ_FROM_CACHE_QUAD_IO:
+	case SPINAND_CMD_READ_FROM_CACHE_X4:
+	case SPINAND_CMD_PROG_LOAD_X4:
+	case SPINAND_CMD_PROG_LOAD_RDM_DATA_X4:
+		return 4;
+	case SPINAND_CMD_READ_FROM_CACHE_DUAL_IO:
+	case SPINAND_CMD_READ_FROM_CACHE_X2:
+		return 2;
+	default:
+		return 1;
+	}
+}
+
+/*
+ * spinand_read_from_cache - read data out from cache register
+ * @chip: SPI NAND device structure
+ * @page_addr: page to read
+ * @column: the location to read from the cache
+ * @len: number of bytes to read
+ * @rbuf: buffer held @len bytes
+ */
+static int spinand_read_from_cache(struct spinand_device *chip, u32 page_addr,
+				   u32 column, size_t len, u8 *rbuf)
+{
+	struct spinand_op op;
+
+	spinand_op_init(&op);
+	op.cmd = chip->read_cache_op;
+	op.n_addr = 2;
+	op.addr[0] = (u8)(column >> 8);
+	op.addr[1] = (u8)column;
+	op.addr_nbits = spinand_get_address_bits(chip->read_cache_op);
+	op.n_rx = len;
+	op.rx_buf = rbuf;
+	op.data_nbits = spinand_get_data_bits(chip->read_cache_op);
+	if (chip->manufacturer.manu->ops->prepare_op)
+		chip->manufacturer.manu->ops->prepare_op(chip, &op,
+							 page_addr, column);
+
+	return spinand_exec_op(chip, &op);
+}
+
+/*
+ * spinand_write_to_cache - write data to cache register
+ * @chip: SPI NAND device structure
+ * @page_addr: page to write
+ * @column: the location to write to the cache
+ * @len: number of bytes to write
+ * @wrbuf: buffer held @len bytes
+ */
+static int spinand_write_to_cache(struct spinand_device *chip, u32 page_addr,
+				  u32 column, size_t len, const u8 *wbuf)
+{
+	struct spinand_op op;
+
+	spinand_op_init(&op);
+	op.cmd = chip->write_cache_op;
+	op.n_addr = 2;
+	op.addr[0] = (u8)(column >> 8);
+	op.addr[1] = (u8)column;
+	op.addr_nbits = spinand_get_address_bits(chip->write_cache_op);
+	op.n_tx = len;
+	op.tx_buf = wbuf;
+	op.data_nbits = spinand_get_data_bits(chip->write_cache_op);
+	if (chip->manufacturer.manu->ops->prepare_op)
+		chip->manufacturer.manu->ops->prepare_op(chip, &op,
+							 page_addr, column);
+
+	return spinand_exec_op(chip, &op);
+}
+
+/*
+ * spinand_program_execute - send command 10h to write a page from
+ * cache to the NAND array
+ * @chip: SPI NAND device structure
+ * @page_addr: the physical page location to write the page.
+ */
+static int spinand_program_execute(struct spinand_device *chip, u32 page_addr)
+{
+	struct spinand_op op;
+
+	spinand_op_init(&op);
+	op.cmd = SPINAND_CMD_PROG_EXC;
+	op.n_addr = 3;
+	op.addr[0] = (u8)(page_addr >> 16);
+	op.addr[1] = (u8)(page_addr >> 8);
+	op.addr[2] = (u8)page_addr;
+
+	return spinand_exec_op(chip, &op);
+}
+
+/*
+ * spinand_erase_block_erase - send command D8h to erase a block
+ * @chip: SPI NAND device structure
+ * @page_addr: the start page address of block to be erased.
+ */
+static int spinand_erase_block(struct spinand_device *chip, u32 page_addr)
+{
+	struct spinand_op op;
+
+	spinand_op_init(&op);
+	op.cmd = SPINAND_CMD_BLK_ERASE;
+	op.n_addr = 3;
+	op.addr[0] = (u8)(page_addr >> 16);
+	op.addr[1] = (u8)(page_addr >> 8);
+	op.addr[2] = (u8)page_addr;
+
+	return spinand_exec_op(chip, &op);
+}
+
+/*
  * spinand_wait - wait until the command is done
  * @chip: SPI NAND device structure
  * @s: buffer to store status register value (can be NULL)
@@ -193,6 +419,516 @@ static int spinand_lock_block(struct spinand_device *chip, u8 lock)
 }
 
 /*
+ * spinand_get_ecc_status - get ecc correction information from status register
+ * @chip: SPI NAND device structure
+ * @status: status register value
+ * @corrected: corrected bit flip number
+ * @ecc_error: ecc correction error or not
+ */
+static void spinand_get_ecc_status(struct spinand_device *chip,
+				   unsigned int status,
+				   unsigned int *corrected,
+				   unsigned int *ecc_error)
+{
+	return chip->ecc.engine->ops->get_ecc_status(chip, status, corrected,
+						     ecc_error);
+}
+
+/*
+ * spinand_do_read_page - read page from device to buffer
+ * @mtd: MTD device structure
+ * @page_addr: page address/raw address
+ * @ecc_off: without ecc or not
+ * @corrected: how many bit flip corrected
+ * @oob_only: read OOB only or the whole page
+ */
+static int spinand_do_read_page(struct mtd_info *mtd, u32 page_addr,
+				bool ecc_off, int *corrected, bool oob_only)
+{
+	struct spinand_device *chip = mtd_to_spinand(mtd);
+	struct nand_device *nand = mtd_to_nand(mtd);
+	int ret, ecc_error = 0;
+	u8 status;
+
+	spinand_read_page_to_cache(chip, page_addr);
+	ret = spinand_wait(chip, &status);
+	if (ret < 0) {
+		pr_err("error %d waiting page 0x%x to cache\n",
+		       ret, page_addr);
+		return ret;
+	}
+	if (!oob_only)
+		spinand_read_from_cache(chip, page_addr, 0,
+					nand_page_size(nand) +
+					nand_per_page_oobsize(nand),
+					chip->buf);
+	else
+		spinand_read_from_cache(chip, page_addr, nand_page_size(nand),
+					nand_per_page_oobsize(nand),
+					chip->oobbuf);
+	if (!ecc_off) {
+		spinand_get_ecc_status(chip, status, corrected, &ecc_error);
+		/*
+		 * If there's an ECC error, print a message and notify MTD
+		 * about it. Then complete the read, to load actual data on
+		 * the buffer (instead of the status result).
+		 */
+		if (ecc_error) {
+			pr_err("internal ECC error reading page 0x%x\n",
+			       page_addr);
+			mtd->ecc_stats.failed++;
+		} else if (*corrected) {
+			mtd->ecc_stats.corrected += *corrected;
+		}
+	}
+
+	return 0;
+}
+
+/*
+ * spinand_do_write_page - write data from buffer to device
+ * @mtd: MTD device structure
+ * @page_addr: page address/raw address
+ * @oob_only: write OOB only or the whole page
+ */
+static int spinand_do_write_page(struct mtd_info *mtd, u32 page_addr,
+				 bool oob_only)
+{
+	struct spinand_device *chip = mtd_to_spinand(mtd);
+	struct nand_device *nand = mtd_to_nand(mtd);
+	u8 status;
+	int ret = 0;
+
+	spinand_write_enable(chip);
+	if (!oob_only)
+		spinand_write_to_cache(chip, page_addr, 0,
+				       nand_page_size(nand) +
+				       nand_per_page_oobsize(nand), chip->buf);
+	else
+		spinand_write_to_cache(chip, page_addr, nand_page_size(nand),
+				       nand_per_page_oobsize(nand),
+				       chip->oobbuf);
+	spinand_program_execute(chip, page_addr);
+	ret = spinand_wait(chip, &status);
+	if (ret < 0) {
+		pr_err("error %d reading page 0x%x from cache\n",
+		       ret, page_addr);
+		return ret;
+	}
+	if ((status & STATUS_P_FAIL_MASK) == STATUS_P_FAIL) {
+		pr_err("program page 0x%x failed\n", page_addr);
+		ret = -EIO;
+	}
+	return ret;
+}
+
+/*
+ * spinand_transfer_oob - transfer oob to client buffer
+ * @chip: SPI NAND device structure
+ * @oob: oob destination address
+ * @ops: oob ops structure
+ * @len: size of oob to transfer
+ */
+static int spinand_transfer_oob(struct spinand_device *chip, u8 *oob,
+				struct mtd_oob_ops *ops, size_t len)
+{
+	struct mtd_info *mtd = spinand_to_mtd(chip);
+	int ret = 0;
+
+	switch (ops->mode) {
+	case MTD_OPS_PLACE_OOB:
+	case MTD_OPS_RAW:
+		memcpy(oob, chip->oobbuf + ops->ooboffs, len);
+		break;
+	case MTD_OPS_AUTO_OOB:
+		ret = mtd_ooblayout_get_databytes(mtd, oob, chip->oobbuf,
+						  ops->ooboffs, len);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+	return ret;
+}
+
+/*
+ * spinand_fill_oob - transfer client buffer to oob
+ * @chip: SPI NAND device structure
+ * @oob: oob data buffer
+ * @len: oob data write length
+ * @ops: oob ops structure
+ */
+static int spinand_fill_oob(struct spinand_device *chip, uint8_t *oob,
+			    size_t len, struct mtd_oob_ops *ops)
+{
+	struct mtd_info *mtd = spinand_to_mtd(chip);
+	struct nand_device *nand = mtd_to_nand(mtd);
+	int ret = 0;
+
+	memset(chip->oobbuf, 0xff, nand_per_page_oobsize(nand));
+	switch (ops->mode) {
+	case MTD_OPS_PLACE_OOB:
+	case MTD_OPS_RAW:
+		memcpy(chip->oobbuf + ops->ooboffs, oob, len);
+		break;
+	case MTD_OPS_AUTO_OOB:
+		ret = mtd_ooblayout_set_databytes(mtd, oob, chip->oobbuf,
+						  ops->ooboffs, len);
+		break;
+	default:
+		ret = -EINVAL;
+	}
+	return ret;
+}
+
+/*
+ * spinand_read_pages - read data from device to buffer
+ * @mtd: MTD device structure
+ * @from: offset to read from
+ * @ops: oob operations description structure
+ * @max_bitflips: maximum bitflip count
+ */
+static int spinand_read_pages(struct mtd_info *mtd, loff_t from,
+			      struct mtd_oob_ops *ops,
+			      unsigned int *max_bitflips)
+{
+	struct spinand_device *chip = mtd_to_spinand(mtd);
+	struct nand_device *nand = mtd_to_nand(mtd);
+	int size, ret;
+	unsigned int corrected = 0;
+	bool ecc_off = ops->mode == MTD_OPS_RAW;
+	int ooblen = ops->mode == MTD_OPS_AUTO_OOB ?
+		     mtd->oobavail : mtd->oobsize;
+	bool oob_only = !ops->datbuf;
+	struct nand_page_iter iter;
+
+	ops->retlen = 0;
+	ops->oobretlen = 0;
+	*max_bitflips = 0;
+
+	nand_for_each_page(nand, from, ops->len, ops->ooboffs, ops->ooblen,
+			   ooblen, &iter) {
+		ret = spinand_do_read_page(mtd, iter.page, ecc_off,
+					   &corrected, oob_only);
+		if (ret)
+			break;
+		*max_bitflips = max(*max_bitflips, corrected);
+		if (ops->datbuf) {
+			size = min_t(int, from + ops->len - iter.offs,
+				     nand_page_size(nand) - iter.pageoffs);
+			memcpy(ops->datbuf + ops->retlen,
+			       chip->buf + iter.pageoffs, size);
+			ops->retlen += size;
+		}
+		if (ops->oobbuf) {
+			size = min_t(int, iter.oobleft, ooblen);
+			ret = spinand_transfer_oob(chip,
+						   ops->oobbuf + ops->oobretlen,
+						   ops, size);
+			if (ret) {
+				pr_err("Transfer oob error %d\n", ret);
+				return ret;
+			}
+			ops->oobretlen += size;
+		}
+	}
+
+	return ret;
+}
+
+/*
+ * spinand_do_read_ops - read data from device to buffer
+ * @mtd: MTD device structure
+ * @from: offset to read from
+ * @ops: oob operations description structure
+ */
+static int spinand_do_read_ops(struct mtd_info *mtd, loff_t from,
+			       struct mtd_oob_ops *ops)
+{
+	struct spinand_device *chip = mtd_to_spinand(mtd);
+	struct nand_device *nand = mtd_to_nand(mtd);
+	int ret;
+	struct mtd_ecc_stats stats;
+	unsigned int max_bitflips = 0;
+	bool ecc_off = ops->mode == MTD_OPS_RAW;
+
+	if (!valid_nand_address(nand, from)) {
+		pr_err("%s: invalid read address\n", __func__);
+		return -EINVAL;
+	}
+	if ((ops->ooblen > 0) && !valid_nand_oob_ops(nand, from, ops)) {
+		pr_err("%s: invalid oob operation input\n", __func__);
+		return -EINVAL;
+	}
+	mutex_lock(&chip->lock);
+	stats = mtd->ecc_stats;
+	if (ecc_off)
+		spinand_disable_ecc(chip);
+	ret = spinand_read_pages(mtd, from, ops, &max_bitflips);
+	if (ecc_off)
+		spinand_enable_ecc(chip);
+	if (ret)
+		goto out;
+
+	if (mtd->ecc_stats.failed - stats.failed) {
+		ret = -EBADMSG;
+		goto out;
+	}
+	ret = max_bitflips;
+
+out:
+	mutex_unlock(&chip->lock);
+	return ret;
+}
+
+/*
+ * spinand_write_pages - write data from buffer to device
+ * @mtd: MTD device structure
+ * @to: offset to write to
+ * @ops: oob operations description structure
+ */
+static int spinand_write_pages(struct mtd_info *mtd, loff_t to,
+			       struct mtd_oob_ops *ops)
+{
+	struct spinand_device *chip = mtd_to_spinand(mtd);
+	struct nand_device *nand = mtd_to_nand(mtd);
+	int ret = 0;
+	int size = 0;
+	int oob_size = 0;
+	int ooblen = ops->mode == MTD_OPS_AUTO_OOB ?
+		     mtd->oobavail : mtd->oobsize;
+	bool oob_only = !ops->datbuf;
+	struct nand_page_iter iter;
+
+	ops->retlen = 0;
+	ops->oobretlen = 0;
+
+	nand_for_each_page(nand, to, ops->len, ops->ooboffs, ops->ooblen,
+			   ooblen, &iter) {
+		memset(chip->buf, 0xff,
+		       nand_page_size(nand) + nand_per_page_oobsize(nand));
+		if (ops->oobbuf) {
+			oob_size = min_t(int, iter.oobleft, ooblen);
+			ret = spinand_fill_oob(chip,
+					       ops->oobbuf + ops->oobretlen,
+					       oob_size, ops);
+			if (ret) {
+				pr_err("Fill oob error %d\n", ret);
+				return ret;
+			}
+		}
+		if (ops->datbuf) {
+			size = min_t(int, to + ops->len - iter.offs,
+				     nand_page_size(nand) - iter.pageoffs);
+			memcpy(chip->buf + iter.pageoffs,
+			       ops->datbuf + ops->retlen, size);
+		}
+		ret = spinand_do_write_page(mtd, iter.page, oob_only);
+		if (ret) {
+			pr_err("error %d writing page 0x%x\n",
+			       ret, iter.page);
+			return ret;
+		}
+		if (ops->datbuf)
+			ops->retlen += size;
+		if (ops->oobbuf)
+			ops->oobretlen += oob_size;
+	}
+
+	return ret;
+}
+
+/*
+ * spinand_do_write_ops - write data from buffer to device
+ * @mtd: MTD device structure
+ * @to: offset to write to
+ * @ops: oob operations description structure
+ */
+static int spinand_do_write_ops(struct mtd_info *mtd, loff_t to,
+				struct mtd_oob_ops *ops)
+{
+	struct spinand_device *chip = mtd_to_spinand(mtd);
+	struct nand_device *nand = mtd_to_nand(mtd);
+	int ret = 0;
+	bool ecc_off = ops->mode == MTD_OPS_RAW;
+
+	if (!valid_nand_address(nand, to)) {
+		pr_err("%s: invalid read address\n", __func__);
+		return -EINVAL;
+	}
+	if ((ops->ooblen > 0) && !valid_nand_oob_ops(nand, to, ops)) {
+		pr_err("%s: invalid oob operation input\n", __func__);
+		return -EINVAL;
+	}
+	if (nand_oob_ops_across_page(mtd_to_nand(mtd), ops)) {
+		pr_err("%s: try to across page when writing with OOB\n",
+		       __func__);
+		return -EINVAL;
+	}
+
+	mutex_lock(&chip->lock);
+	if (ecc_off)
+		spinand_disable_ecc(chip);
+	ret = spinand_write_pages(mtd, to, ops);
+	if (ecc_off)
+		spinand_enable_ecc(chip);
+	mutex_unlock(&chip->lock);
+
+	return ret;
+}
+
+/*
+ * spinand_read - [MTD Interface] read page data
+ * @mtd: MTD device structure
+ * @from: offset to read from
+ * @len: number of bytes to read
+ * @retlen: pointer to variable to store the number of read bytes
+ * @buf: the databuffer to put data
+ */
+static int spinand_read(struct mtd_info *mtd, loff_t from, size_t len,
+			size_t *retlen, u8 *buf)
+{
+	struct mtd_oob_ops ops;
+	int ret;
+
+	memset(&ops, 0, sizeof(ops));
+	ops.len = len;
+	ops.datbuf = buf;
+	ops.mode = MTD_OPS_PLACE_OOB;
+	ret = spinand_do_read_ops(mtd, from, &ops);
+	*retlen = ops.retlen;
+
+	return ret;
+}
+
+/*
+ * spinand_write - [MTD Interface] write page data
+ * @mtd: MTD device structure
+ * @to: offset to write to
+ * @len: number of bytes to write
+ * @retlen: pointer to variable to store the number of written bytes
+ * @buf: the data to write
+ */
+static int spinand_write(struct mtd_info *mtd, loff_t to, size_t len,
+			 size_t *retlen, const u8 *buf)
+{
+	struct mtd_oob_ops ops;
+	int ret;
+
+	memset(&ops, 0, sizeof(ops));
+	ops.len = len;
+	ops.datbuf = (uint8_t *)buf;
+	ops.mode = MTD_OPS_PLACE_OOB;
+	ret =  spinand_do_write_ops(mtd, to, &ops);
+	*retlen = ops.retlen;
+
+	return ret;
+}
+
+/*
+ * spinand_read_oob - [MTD Interface] read page data and/or out-of-band
+ * @mtd: MTD device structure
+ * @from: offset to read from
+ * @ops: oob operation description structure
+ */
+static int spinand_read_oob(struct mtd_info *mtd, loff_t from,
+			    struct mtd_oob_ops *ops)
+{
+	int ret = -ENOTSUPP;
+
+	ops->retlen = 0;
+	switch (ops->mode) {
+	case MTD_OPS_PLACE_OOB:
+	case MTD_OPS_AUTO_OOB:
+	case MTD_OPS_RAW:
+		ret = spinand_do_read_ops(mtd, from, ops);
+		break;
+	}
+
+	return ret;
+}
+
+/*
+ * spinand_write_oob - [MTD Interface] write page data and/or out-of-band
+ * @mtd: MTD device structure
+ * @to: offset to write to
+ * @ops: oob operation description structure
+ */
+static int spinand_write_oob(struct mtd_info *mtd, loff_t to,
+			     struct mtd_oob_ops *ops)
+{
+	int ret = -ENOTSUPP;
+
+	ops->retlen = 0;
+	switch (ops->mode) {
+	case MTD_OPS_PLACE_OOB:
+	case MTD_OPS_AUTO_OOB:
+	case MTD_OPS_RAW:
+		ret = spinand_do_write_ops(mtd, to, ops);
+		break;
+	}
+
+	return ret;
+}
+
+/*
+ * spinand_erase - [MTD Interface] erase block(s)
+ * @mtd: MTD device structure
+ * @einfo: erase instruction
+ */
+static int spinand_erase(struct mtd_info *mtd, struct erase_info *einfo)
+{
+	struct spinand_device *chip = mtd_to_spinand(mtd);
+	struct nand_device *nand = mtd_to_nand(mtd);
+	loff_t offs = einfo->addr, len = einfo->len;
+	u8 status;
+	int ret = 0;
+
+	if (!valid_nand_erase_ops(nand, einfo)) {
+		pr_err("%s: invalid erase operation input\n", __func__);
+		return -EINVAL;
+	}
+
+	mutex_lock(&chip->lock);
+	einfo->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
+	einfo->state = MTD_ERASING;
+
+	while (len) {
+		spinand_write_enable(chip);
+		spinand_erase_block(chip, nand_offs_to_page(nand, offs));
+		ret = spinand_wait(chip, &status);
+		if (ret < 0) {
+			pr_err("block erase command wait failed\n");
+			einfo->state = MTD_ERASE_FAILED;
+			goto erase_exit;
+		}
+		if ((status & STATUS_E_FAIL_MASK) == STATUS_E_FAIL) {
+			pr_err("erase block 0x%012llx failed\n", offs);
+			einfo->state = MTD_ERASE_FAILED;
+			einfo->fail_addr = offs;
+			goto erase_exit;
+		}
+
+		/* Increment page address and decrement length */
+		len -= nand_eraseblock_size(nand);
+		offs += nand_eraseblock_size(nand);
+	}
+
+	einfo->state = MTD_ERASE_DONE;
+
+erase_exit:
+
+	ret = einfo->state == MTD_ERASE_DONE ? 0 : -EIO;
+
+	mutex_unlock(&chip->lock);
+
+	/* Do call back function */
+	if (!ret)
+		mtd_erase_callback(einfo);
+
+	return ret;
+}
+
+/*
  * spinand_set_rd_wr_op - choose the best read write command
  * @chip: SPI NAND device structure
  * Description:
@@ -401,6 +1137,11 @@ int spinand_init(struct spinand_device *chip)
 	if (ret < 0)
 		ret = 0;
 	mtd->oobavail = ret;
+	mtd->_erase = spinand_erase;
+	mtd->_read = spinand_read;
+	mtd->_write = spinand_write;
+	mtd->_read_oob = spinand_read_oob;
+	mtd->_write_oob = spinand_write_oob;
 
 	if (!mtd->bitflip_threshold)
 		mtd->bitflip_threshold = DIV_ROUND_UP(mtd->ecc_strength * 3,
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index 4e785da..4e0fae4 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -131,6 +131,8 @@ struct spinand_manufacturer_ops {
 	bool (*detect)(struct spinand_device *chip, u8 *raw_id, u8 *id);
 	int (*init)(struct spinand_device *chip);
 	void (*cleanup)(struct spinand_device *chip);
+	void (*prepare_op)(struct spinand_device *chip, struct spinand_op *op,
+			   u32 page, u32 column);
 };
 
 struct spinand_manufacturer {
-- 
1.9.1

[PATCH v3 5/8] nand: spi: Add bad block support

[Peter Pan]

Add isbad and markbad support for SPI NAND. And do not
erase bad blocks in spi_nand_erase. BBT is also enabled
in this patch.

Signed-off-by: Peter Pan <peterpandong@micron.com>
---
 drivers/mtd/nand/spi/core.c | 293 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 289 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 7834c3d..a880a18 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -21,6 +21,9 @@
 #include <linux/mtd/spinand.h>
 #include <linux/slab.h>
 
+static int spinand_erase_skip_bbt(struct mtd_info *mtd,
+				  struct erase_info *einfo);
+
 /*
  * spinand_exec_op - execute SPI NAND operation by controller ->exec_op() hook
  * @chip: SPI NAND device structure
@@ -871,11 +874,154 @@ static int spinand_write_oob(struct mtd_info *mtd, loff_t to,
 }
 
 /*
- * spinand_erase - [MTD Interface] erase block(s)
+ * spinand_block_bad - check if block at offset is bad by bad block marker
+ * @mtd: MTD device structure
+ * @offs: offset from device start
+ */
+static int spinand_block_bad(struct mtd_info *mtd, loff_t offs)
+{
+	struct nand_device *nand = mtd_to_nand(mtd);
+	struct mtd_oob_ops ops = {0};
+	u32 block_addr;
+	u8 bad[2] = {0, 0};
+	u8 ret = 0;
+	unsigned int max_bitflips;
+
+	block_addr = nand_offs_to_eraseblock(nand, offs);
+	ops.mode = MTD_OPS_PLACE_OOB;
+	ops.ooblen = 2;
+	ops.oobbuf = bad;
+	spinand_read_pages(mtd, nand_eraseblock_to_offs(nand, block_addr),
+			   &ops, &max_bitflips);
+	if (bad[0] != 0xFF || bad[1] != 0xFF)
+		ret =  1;
+
+	return ret;
+}
+
+/*
+ * spinand_block_checkbad - check if a block is marked bad
+ * @mtd: MTD device structure
+ * @offs: offset from device start
+ * @allowbbt: 1, if allowe to access the bbt area
+ * Description:
+ *   Check, if the block is bad. Either by reading the bad block table or
+ *   reading bad block marker.
+ */
+static int spinand_block_checkbad(struct mtd_info *mtd, loff_t offs,
+				  int allowbbt)
+{
+	struct nand_device *nand = mtd_to_nand(mtd);
+	int ret;
+
+	if (nand_bbt_is_initialized(nand))
+		ret = nand_isbad_bbt(nand, offs, 0);
+	else
+		ret = spinand_block_bad(mtd, offs);
+
+	return ret;
+}
+
+/*
+ * spinand_block_isbad - [MTD Interface] check if block at offset is bad
+ * @mtd: MTD device structure
+ * @offs: offset from device start
+ */
+static int spinand_block_isbad(struct mtd_info *mtd, loff_t offs)
+{
+	struct spinand_device *chip = mtd_to_spinand(mtd);
+	int ret;
+
+	mutex_lock(&chip->lock);
+	ret = spinand_block_checkbad(mtd, offs, 0);
+	mutex_unlock(&chip->lock);
+
+	return ret;
+}
+
+/*
+ * spinand_block_markbad_lowlevel - mark a block bad
+ * @mtd: MTD device structure
+ * @offs: offset from device start
+ *
+ * This function performs the generic bad block marking steps (i.e., bad
+ * block table(s) and/or marker(s)).
+ *
+ * We try operations in the following order:
+ *  (1) erase the affected block, to allow OOB marker to be written cleanly
+ *  (2) write bad block marker to OOB area of affected block (unless flag
+ *      NAND_BBT_NO_OOB_BBM is present)
+ *  (3) update the BBT
+ */
+static int spinand_block_markbad_lowlevel(struct mtd_info *mtd, loff_t offs)
+{
+	struct nand_device *nand = mtd_to_nand(mtd);
+	struct mtd_oob_ops ops = {0};
+	struct erase_info einfo = {0};
+	u32 block_addr;
+	u8 buf[2] = {0, 0};
+	int res, ret = 0;
+
+	if (!nand_bbt_is_initialized(nand) ||
+	    !(nand->bbt.options & NAND_BBT_NO_OOB_BBM)) {
+		/*erase bad block before mark bad block*/
+		einfo.mtd = mtd;
+		einfo.addr = offs;
+		einfo.len = nand_eraseblock_size(nand);
+		spinand_erase_skip_bbt(mtd, &einfo);
+
+		block_addr = nand_offs_to_eraseblock(nand, offs);
+		ops.mode = MTD_OPS_PLACE_OOB;
+		ops.ooblen = 2;
+		ops.oobbuf = buf;
+		ret = spinand_do_write_ops(mtd,
+					   nand_eraseblock_to_offs(nand,
+								   block_addr),
+					   &ops);
+	}
+
+	/* Mark block bad in BBT */
+	if (nand_bbt_is_initialized(nand)) {
+		res = nand_markbad_bbt(nand, offs);
+		if (!ret)
+			ret = res;
+	}
+
+	if (!ret)
+		mtd->ecc_stats.badblocks++;
+
+	return ret;
+}
+
+/*
+ * spinand_block_markbad - [MTD Interface] mark block at the given offset
+ * as bad
+ * @mtd: MTD device structure
+ * @offs: offset relative to mtd start
+ */
+static int spinand_block_markbad(struct mtd_info *mtd, loff_t offs)
+{
+	int ret;
+
+	ret = spinand_block_isbad(mtd, offs);
+	if (ret) {
+		/* If it was bad already, return success and do nothing */
+		if (ret > 0)
+			return 0;
+		return ret;
+	}
+
+	return spinand_block_markbad_lowlevel(mtd, offs);
+}
+
+/*
+ * spinand_erase - erase block(s)
  * @mtd: MTD device structure
  * @einfo: erase instruction
+ * @allowbbt: allow to access bbt
  */
-static int spinand_erase(struct mtd_info *mtd, struct erase_info *einfo)
+static int spinand_erase(struct mtd_info *mtd, struct erase_info *einfo,
+			 int allowbbt)
 {
 	struct spinand_device *chip = mtd_to_spinand(mtd);
 	struct nand_device *nand = mtd_to_nand(mtd);
@@ -893,6 +1039,13 @@ static int spinand_erase(struct mtd_info *mtd, struct erase_info *einfo)
 	einfo->state = MTD_ERASING;
 
 	while (len) {
+		/* Check if we have a bad block, we do not erase bad blocks! */
+		if (spinand_block_checkbad(mtd, offs, allowbbt)) {
+			pr_warn("%s: attempt to erase a bad block at 0x%012llx\n",
+				__func__, offs);
+			einfo->state = MTD_ERASE_FAILED;
+			goto erase_exit;
+		}
 		spinand_write_enable(chip);
 		spinand_erase_block(chip, nand_offs_to_page(nand, offs));
 		ret = spinand_wait(chip, &status);
@@ -929,6 +1082,33 @@ static int spinand_erase(struct mtd_info *mtd, struct erase_info *einfo)
 }
 
 /*
+ * spinand_erase_skip_bbt - [MTD Interface] erase block(s) except BBT
+ * @mtd: MTD device structure
+ * @einfo: erase instruction
+ */
+static int spinand_erase_skip_bbt(struct mtd_info *mtd,
+				  struct erase_info *einfo)
+{
+	return spinand_erase(mtd, einfo, 0);
+}
+
+/*
+ * spinand_block_isreserved - [MTD Interface] check if a block is
+ * marked reserved.
+ * @mtd: MTD device structure
+ * @offs: offset from device start
+ */
+static int spinand_block_isreserved(struct mtd_info *mtd, loff_t offs)
+{
+	struct nand_device *nand = mtd_to_nand(mtd);
+
+	if (!nand_bbt_is_initialized(nand))
+		return 0;
+	/* Return info from the table */
+	return nand_isreserved_bbt(nand, offs);
+}
+
+/*
  * spinand_set_rd_wr_op - choose the best read write command
  * @chip: SPI NAND device structure
  * Description:
@@ -963,6 +1143,100 @@ static void spinand_set_rd_wr_op(struct spinand_device *chip)
 }
 
 /*
+ * spinand_erase_bbt - erase block(s) including BBT
+ * @nand: nand device structure
+ * @einfo: erase instruction
+ */
+static int spinand_erase_bbt(struct nand_device *nand,
+			     struct erase_info *einfo)
+{
+	return spinand_erase(nand_to_mtd(nand), einfo, 1);
+}
+
+/*
+ * spinand_erase_bbt - write bad block marker to certain block
+ * @nand: nand device structure
+ * @block: block to mark bad
+ */
+static int spinand_markbad(struct nand_device *nand, int block)
+{
+	struct mtd_oob_ops ops = {0};
+	u8 buf[2] = {0, 0};
+
+	ops.mode = MTD_OPS_PLACE_OOB;
+	ops.ooboffs = 0;
+	ops.ooblen = 2;
+	ops.oobbuf = buf;
+
+	return spinand_do_write_ops(nand_to_mtd(nand),
+				    nand_eraseblock_to_offs(nand, block),
+				    &ops);
+}
+
+static const struct nand_ops spinand_ops = {
+	.erase = spinand_erase_bbt,
+	.markbad = spinand_markbad,
+};
+
+/*
+ * Define some generic bad/good block scan pattern which are used
+ * while scanning a device for factory marked good/bad blocks.
+ */
+static u8 scan_ff_pattern[] = { 0xff, 0xff };
+
+#define BADBLOCK_SCAN_MASK (~NAND_BBT_NO_OOB)
+
+/*
+ * spinand_create_badblock_pattern - creates a BBT descriptor structure
+ * @chip: SPI NAND device structure
+ *
+ * This function allocates and initializes a nand_bbt_descr for BBM detection.
+ * The new descriptor is stored in nand->bbt.bbp. Thus, nand->bbt.bbp should
+ * be NULL when passed to this function.
+ */
+static int spinand_create_badblock_pattern(struct spinand_device *chip)
+{
+	struct nand_device *nand = &chip->base;
+	struct nand_bbt_descr *bd;
+
+	if (nand->bbt.bbp) {
+		pr_warn("Bad block pattern already allocated; not replacing\n");
+		return -EINVAL;
+	}
+	bd = kzalloc(sizeof(*bd), GFP_KERNEL);
+	if (!bd)
+		return -ENOMEM;
+	bd->options = nand->bbt.options & BADBLOCK_SCAN_MASK;
+	bd->offs = 0;
+	bd->len = 2;
+	bd->pattern = scan_ff_pattern;
+	bd->options |= NAND_BBT_DYNAMICSTRUCT;
+	nand->bbt.bbp = bd;
+
+	return 0;
+}
+
+/*
+ * spinand_scan_bbt - scan BBT in SPI NAND device
+ * @chip: SPI NAND device structure
+ */
+static int spinand_scan_bbt(struct spinand_device *chip)
+{
+	struct nand_device *nand = &chip->base;
+	int ret;
+
+	nand->bbt.options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
+	nand->bbt.td = NULL;
+	nand->bbt.md = NULL;
+
+	ret = spinand_create_badblock_pattern(chip);
+	if (ret)
+		return ret;
+
+	return nand_scan_bbt(nand);
+}
+
+/*
  * spinand_manufacturer_detect - detect SPI NAND device by each manufacturer
  * @chip: SPI NAND device structure
  * @raw_id: raw id buffer. raw id is read by spinand_read_id(), should be
@@ -1120,6 +1394,7 @@ int spinand_init(struct spinand_device *chip)
 		return -ENOMEM;
 
 	chip->oobbuf = chip->buf + nand_page_size(nand);
+	nand->ops = &spinand_ops;
 	mtd->name = chip->name;
 	mtd->size = nand_size(nand);
 	mtd->erasesize = nand_eraseblock_size(nand);
@@ -1137,11 +1412,14 @@ int spinand_init(struct spinand_device *chip)
 	if (ret < 0)
 		ret = 0;
 	mtd->oobavail = ret;
-	mtd->_erase = spinand_erase;
+	mtd->_erase = spinand_erase_skip_bbt;
 	mtd->_read = spinand_read;
 	mtd->_write = spinand_write;
 	mtd->_read_oob = spinand_read_oob;
 	mtd->_write_oob = spinand_write_oob;
+	mtd->_block_isbad = spinand_block_isbad;
+	mtd->_block_markbad = spinand_block_markbad;
+	mtd->_block_isreserved = spinand_block_isreserved;
 
 	if (!mtd->bitflip_threshold)
 		mtd->bitflip_threshold = DIV_ROUND_UP(mtd->ecc_strength * 3,
@@ -1149,7 +1427,8 @@ int spinand_init(struct spinand_device *chip)
 	/* After power up, all blocks are locked, so unlock it here. */
 	spinand_lock_block(chip, BL_ALL_UNLOCKED);
 
-	return 0;
+	/* Build bad block table */
+	return spinand_scan_bbt(chip);
 }
 EXPORT_SYMBOL_GPL(spinand_init);
 
@@ -1159,8 +1438,14 @@ int spinand_init(struct spinand_device *chip)
  */
 int spinand_cleanup(struct spinand_device *chip)
 {
+	struct nand_device *nand = &chip->base;
+	struct nand_bbt_descr *bd = nand->bbt.bbp;
+
 	spinand_manufacturer_cleanup(chip);
 	kfree(chip->buf);
+	kfree(nand->bbt.bbt);
+	if (bd->options & NAND_BBT_DYNAMICSTRUCT)
+		kfree(bd);
 
 	return 0;
 }
-- 
1.9.1

[PATCH v3 6/8] nand: spi: add Micron spi nand support

[Peter Pan]

This commit is to add support for Micron MT29F2G01ABAGD
spi nand chip.

Signed-off-by: Peter Pan <peterpandong@micron.com>
---
 drivers/mtd/nand/spi/Makefile       |   1 +
 drivers/mtd/nand/spi/manufactures.c |   3 +
 drivers/mtd/nand/spi/micron.c       | 243 ++++++++++++++++++++++++++++++++++++
 3 files changed, 247 insertions(+)
 create mode 100644 drivers/mtd/nand/spi/micron.c

diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile
index eabdb81..db0b91b 100644
--- a/drivers/mtd/nand/spi/Makefile
+++ b/drivers/mtd/nand/spi/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_MTD_SPI_NAND) += core.o
 obj-$(CONFIG_MTD_SPI_NAND) += manufactures.o
+obj-$(CONFIG_MTD_SPI_NAND) += micron.o
diff --git a/drivers/mtd/nand/spi/manufactures.c b/drivers/mtd/nand/spi/manufactures.c
index 7e0b42d..40dae11 100644
--- a/drivers/mtd/nand/spi/manufactures.c
+++ b/drivers/mtd/nand/spi/manufactures.c
@@ -16,9 +16,12 @@
 #include <linux/module.h>
 #include <linux/mtd/spinand.h>
 
+extern struct spinand_manufacturer micron_spinand_manufacture;
+
 struct spinand_manufacturer spinand_manufacturer_end = {0x0, "Unknown", NULL};
 
 struct spinand_manufacturer *spinand_manufacturers[] = {
+	&micron_spinand_manufacture,
 	&spinand_manufacturer_end,
 };
 EXPORT_SYMBOL(spinand_manufacturers);
diff --git a/drivers/mtd/nand/spi/micron.c b/drivers/mtd/nand/spi/micron.c
new file mode 100644
index 0000000..fc7e91d
--- /dev/null
+++ b/drivers/mtd/nand/spi/micron.c
@@ -0,0 +1,243 @@
+/*
+ *
+ * Copyright (c) 2009-2017 Micron Technology, Inc.
+ *
+ * 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/mtd/spinand.h>
+
+#define SPINAND_MFR_MICRON		0x2C
+
+#define SPI_NAND_MT29F_ECC_MASK		0x70
+#define SPI_NAND_MT29F_ECC_0_BIT	0x00
+#define SPI_NAND_MT29F_ECC_1_3_BIT	0x10
+#define SPI_NAND_MT29F_ECC_4_6_BIT	0x30
+#define SPI_NAND_MT29F_ECC_7_8_BIT	0x50
+#define SPI_NAND_MT29F_ECC_UNCORR	0x20
+
+struct micron_spinand_info {
+	char *name;
+	u8 dev_id;
+	u32 page_size;
+	u32 oob_size;
+	u32 pages_per_blk;
+	u32 blks_per_lun;
+	u32 luns_per_chip;
+	u32 ecc_strength;
+	u32 rw_mode;
+};
+
+#define MICRON_SPI_NAND_INFO(nm, did, pagesz, oobsz, pg_per_blk,	\
+		      blk_per_lun, lun_per_chip, ecc_stren, rwmode)	\
+	{	\
+		.name = (nm), .dev_id = (did),		\
+		.page_size = (pagesz), .oob_size = (oobsz),		\
+		.pages_per_blk = (pg_per_blk),		\
+		.blks_per_lun = (blk_per_lun),		\
+		.luns_per_chip = (lun_per_chip),	\
+		.ecc_strength = (ecc_stren),	\
+		.rw_mode = (rwmode)		\
+	}
+
+static const struct micron_spinand_info micron_spinand_table[] = {
+	MICRON_SPI_NAND_INFO("MT29F2G01ABAGD", 0x24, 2048, 128, 64, 2048,
+			     1, 8, SPINAND_OP_COMMON),
+	{.name = NULL},
+};
+
+/*
+ * micron_spinand_scan_id_table - scan chip info in id table
+ * @chip: SPI-NAND device structure
+ * @id: point to manufacture id and device id
+ * Description:
+ *   If found in id table, config chip with table information.
+ */
+static bool micron_spinand_scan_id_table(struct spinand_device *chip, u8 *id)
+{
+	struct nand_device *nand = &chip->base;
+	struct micron_spinand_info *type = NULL;
+	struct nand_memory_organization *memorg = &nand->memorg;
+	struct spinand_ecc_engine *ecc_engine = chip->ecc.engine;
+
+	for (type = (struct micron_spinand_info *)micron_spinand_table;
+	     type->name; type++) {
+		if (id[SPINAND_DEV_ID] != type->dev_id)
+			continue;
+		chip->name = type->name;
+		memorg->eraseblocksize = type->page_size
+				* type->pages_per_blk;
+		memorg->pagesize = type->page_size;
+		memorg->oobsize = type->oob_size;
+		memorg->diesize = memorg->eraseblocksize * type->blks_per_lun;
+		memorg->ndies = type->luns_per_chip;
+		ecc_engine->strength = type->ecc_strength;
+		chip->rw_mode = type->rw_mode;
+
+		return true;
+	}
+
+	return false;
+}
+
+static int micron_ooblayout_ecc_128(struct mtd_info *mtd, int section,
+				    struct mtd_oob_region *oobregion)
+{
+	if (section)
+		return -ERANGE;
+
+	oobregion->length = 64;
+	oobregion->offset = 64;
+
+	return 0;
+}
+
+static int micron_ooblayout_free_128(struct mtd_info *mtd, int section,
+				     struct mtd_oob_region *oobregion)
+{
+	if (section)
+		return -ERANGE;
+
+	oobregion->length = 62;
+	oobregion->offset = 2;
+
+	return 0;
+}
+
+static const struct mtd_ooblayout_ops micron_ooblayout_128_ops = {
+	.ecc = micron_ooblayout_ecc_128,
+	.free = micron_ooblayout_free_128,
+};
+
+static int micron_spinand_get_dummy(struct spinand_device *chip,
+				    struct spinand_op *op)
+{
+	u8 opcode = op->cmd;
+
+	switch (opcode) {
+	case SPINAND_CMD_READ_FROM_CACHE:
+	case SPINAND_CMD_READ_FROM_CACHE_FAST:
+	case SPINAND_CMD_READ_FROM_CACHE_X2:
+	case SPINAND_CMD_READ_FROM_CACHE_DUAL_IO:
+	case SPINAND_CMD_READ_FROM_CACHE_X4:
+	case SPINAND_CMD_READ_ID:
+		return 1;
+	case SPINAND_CMD_READ_FROM_CACHE_QUAD_IO:
+		return 2;
+	default:
+		return 0;
+	}
+}
+
+/*
+ * mt29f_get_ecc_status - get ecc correction information from status register
+ * @chip: SPI NAND device structure
+ * @status: status register value
+ * @corrected: corrected bit flip number
+ * @ecc_error: ecc correction error or not
+ */
+static void mt29f_get_ecc_status(struct spinand_device *chip,
+				 unsigned int status, unsigned int *corrected,
+				 unsigned int *ecc_error)
+{
+	unsigned int ecc_status = status & SPI_NAND_MT29F_ECC_MASK;
+
+	*ecc_error = (ecc_status == SPI_NAND_MT29F_ECC_UNCORR);
+	switch (ecc_status) {
+	case SPI_NAND_MT29F_ECC_0_BIT:
+		*corrected = 0;
+		break;
+	case SPI_NAND_MT29F_ECC_1_3_BIT:
+		*corrected = 3;
+		break;
+	case SPI_NAND_MT29F_ECC_4_6_BIT:
+		*corrected = 6;
+		break;
+	case SPI_NAND_MT29F_ECC_7_8_BIT:
+		*corrected = 8;
+		break;
+	}
+}
+
+static struct spinand_ecc_engine_ops generic_spi_ecc_engine_ops = {
+	.get_ecc_status = mt29f_get_ecc_status,
+};
+
+/*
+ * micron_spinand_detect - initialize device related part in spinand_device
+ * struct if it is Micron device.
+ * @chip: SPI NAND device structure
+ * @raw_id: raw id buffer. raw id is read by spinand_read_id(), should be
+ *          decoded by manufacturers
+ * @id: real id buffer. manufacturer's ->detect() should put real id in
+ *      this buffer.
+ */
+static bool micron_spinand_detect(struct spinand_device *chip,
+				  u8 *raw_id, u8 *id)
+{
+	/*
+	 * Micron SPI NAND read ID need a dummy byte,
+	 * so the first byte in raw_id is dummy.
+	 */
+	if (raw_id[1] != SPINAND_MFR_MICRON)
+		return false;
+
+	id[SPINAND_MFR_ID] = raw_id[1];
+	id[SPINAND_DEV_ID] = raw_id[2];
+
+	return micron_spinand_scan_id_table(chip, id);
+}
+
+/*
+ * micron_spinand_init - Micron device specified initialization function.
+ * @chip: SPI NAND device structure
+ *
+ * Set Micron device ->get_ecc_status() and OOB layout.
+ */
+static int micron_spinand_init(struct spinand_device *chip)
+{
+	struct mtd_info *mtd = spinand_to_mtd(chip);
+	struct nand_device *nand = mtd_to_nand(mtd);
+
+	chip->ecc.engine->ops = &generic_spi_ecc_engine_ops;
+	if (nand_per_page_oobsize(nand) == 128)
+		mtd_set_ooblayout(mtd, &micron_ooblayout_128_ops);
+
+	return 0;
+}
+
+/*
+ * micron_spinand_prepare_op - Fix address for cache operation.
+ * @chip: SPI NAND device structure
+ * @op: pointer to spinand_op struct
+ * @page: page address
+ * @column: column address
+ */
+static void micron_spinand_prepare_op(struct spinand_device *chip,
+				      struct spinand_op *op, u32 page,
+				      u32 column)
+{
+	op->addr[0] |= (u8)((nand_page_to_eraseblock(&chip->base, page)
+				& 0x1) << 4);
+	op->n_addr += micron_spinand_get_dummy(chip, op);
+}
+
+static const struct spinand_manufacturer_ops micron_spinand_manuf_ops = {
+	.detect = micron_spinand_detect,
+	.init = micron_spinand_init,
+	.prepare_op = micron_spinand_prepare_op,
+};
+
+const struct spinand_manufacturer micron_spinand_manufacture = {
+	.id = SPINAND_MFR_MICRON,
+	.name = "Micron",
+	.ops = &micron_spinand_manuf_ops,
+};
-- 
1.9.1

[PATCH v3 7/8] nand: spi: Add generic SPI controller support

[Peter Pan]

This commit supports to use generic spi controller
as spi nand controller.

Signed-off-by: Peter Pan <peterpandong@micron.com>
---
 drivers/mtd/nand/spi/Kconfig                   |   2 +
 drivers/mtd/nand/spi/Makefile                  |   1 +
 drivers/mtd/nand/spi/controllers/Kconfig       |   5 +
 drivers/mtd/nand/spi/controllers/Makefile      |   1 +
 drivers/mtd/nand/spi/controllers/generic-spi.c | 158 +++++++++++++++++++++++++
 5 files changed, 167 insertions(+)
 create mode 100644 drivers/mtd/nand/spi/controllers/Kconfig
 create mode 100644 drivers/mtd/nand/spi/controllers/Makefile
 create mode 100644 drivers/mtd/nand/spi/controllers/generic-spi.c

diff --git a/drivers/mtd/nand/spi/Kconfig b/drivers/mtd/nand/spi/Kconfig
index d77c46e..6bd1c65 100644
--- a/drivers/mtd/nand/spi/Kconfig
+++ b/drivers/mtd/nand/spi/Kconfig
@@ -3,3 +3,5 @@ menuconfig MTD_SPI_NAND
 	depends on MTD_NAND
 	help
 	  This is the framework for the SPI NAND device drivers.
+
+source "drivers/mtd/nand/spi/controllers/Kconfig"
diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile
index db0b91b..6ad5f24 100644
--- a/drivers/mtd/nand/spi/Makefile
+++ b/drivers/mtd/nand/spi/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_MTD_SPI_NAND) += core.o
 obj-$(CONFIG_MTD_SPI_NAND) += manufactures.o
 obj-$(CONFIG_MTD_SPI_NAND) += micron.o
+obj-$(CONFIG_MTD_SPI_NAND) += controllers/
diff --git a/drivers/mtd/nand/spi/controllers/Kconfig b/drivers/mtd/nand/spi/controllers/Kconfig
new file mode 100644
index 0000000..8ab7023
--- /dev/null
+++ b/drivers/mtd/nand/spi/controllers/Kconfig
@@ -0,0 +1,5 @@
+config GENERIC_SPI_NAND
+	tristate "SPI NAND with generic SPI bus Support"
+	depends on MTD_SPI_NAND && SPI
+	help
+	  This is to support SPI NAND device with generic SPI bus.
diff --git a/drivers/mtd/nand/spi/controllers/Makefile b/drivers/mtd/nand/spi/controllers/Makefile
new file mode 100644
index 0000000..46cbf29
--- /dev/null
+++ b/drivers/mtd/nand/spi/controllers/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_GENERIC_SPI_NAND) += generic-spi.o
diff --git a/drivers/mtd/nand/spi/controllers/generic-spi.c b/drivers/mtd/nand/spi/controllers/generic-spi.c
new file mode 100644
index 0000000..38b4a56
--- /dev/null
+++ b/drivers/mtd/nand/spi/controllers/generic-spi.c
@@ -0,0 +1,158 @@
+/*
+ * Copyright (c) 2009-2017 Micron Technology, Inc.
+ *
+ * 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/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+#include <linux/spi/spi.h>
+#include <linux/mtd/spinand.h>
+
+/*
+ * generic_spinand_cmd_fn - to process a command to send to the SPI NAND
+ * by generic SPI bus
+ * @chip: SPI NAND device structure
+ * @op: SPI NAND operation descriptor
+ */
+static int generic_spinand_cmd_fn(struct spinand_device *chip,
+				  struct spinand_op *op)
+{
+	struct spi_message message;
+	struct spi_transfer x[3];
+	struct spi_device *spi = spinand_get_controller_data(chip);
+
+	spi_message_init(&message);
+	memset(x, 0, sizeof(x));
+	x[0].len = 1;
+	x[0].tx_nbits = 1;
+	x[0].tx_buf = &op->cmd;
+	spi_message_add_tail(&x[0], &message);
+
+	if (op->n_addr + op->dummy_bytes) {
+		x[1].len = op->n_addr + op->dummy_bytes;
+		x[1].tx_nbits = op->addr_nbits;
+		x[1].tx_buf = op->addr;
+		spi_message_add_tail(&x[1], &message);
+	}
+	if (op->n_tx) {
+		x[2].len = op->n_tx;
+		x[2].tx_nbits = op->data_nbits;
+		x[2].tx_buf = op->tx_buf;
+		spi_message_add_tail(&x[2], &message);
+	} else if (op->n_rx) {
+		x[2].len = op->n_rx;
+		x[2].rx_nbits = op->data_nbits;
+		x[2].rx_buf = op->rx_buf;
+		spi_message_add_tail(&x[2], &message);
+	}
+	return spi_sync(spi, &message);
+}
+
+static struct spinand_controller_ops generic_spi_ops = {
+	.exec_op = generic_spinand_cmd_fn,
+};
+
+static int generic_spinand_probe(struct spi_device *spi)
+{
+	struct spinand_device *chip;
+	struct mtd_info *mtd;
+	struct spinand_controller *controller;
+	struct spinand_ecc_engine *ecc_engine;
+	int ret;
+	u32 max_speed_hz = spi->max_speed_hz;
+
+	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
+	if (!chip) {
+		ret = -ENOMEM;
+		goto err1;
+	}
+	mtd = spinand_to_mtd(chip);
+	controller = kzalloc(sizeof(*controller), GFP_KERNEL);
+	if (!controller) {
+		ret = -ENOMEM;
+		goto err2;
+	}
+	controller->ops = &generic_spi_ops;
+	controller->caps = SPINAND_CAP_RD_X1 | SPINAND_CAP_WR_X1;
+	if (spi->mode & SPI_RX_QUAD)
+		controller->caps |= SPINAND_CAP_RD_QUAD | SPINAND_CAP_RD_X4;
+	if (spi->mode & SPI_RX_DUAL)
+		controller->caps |= SPINAND_CAP_RD_DUAL | SPINAND_CAP_RD_X2;
+	if (spi->mode & SPI_TX_QUAD)
+		controller->caps |= SPINAND_CAP_WR_QUAD | SPINAND_CAP_WR_X4;
+	if (spi->mode & SPI_TX_DUAL)
+		controller->caps |= SPINAND_CAP_WR_DUAL | SPINAND_CAP_WR_X2;
+	chip->controller = controller;
+	ecc_engine = kzalloc(sizeof(*ecc_engine), GFP_KERNEL);
+	if (!ecc_engine) {
+		ret = -ENOMEM;
+		goto err3;
+	}
+	ecc_engine->mode = SPINAND_ECC_ONDIE;
+	chip->ecc.engine = ecc_engine;
+	spinand_set_controller_data(chip, spi);
+	spi_set_drvdata(spi, chip);
+	spi->max_speed_hz = min_t(int, 25000000, max_speed_hz);
+	ret = spinand_detect(chip);
+	if (ret)
+		goto err4;
+
+	spi->max_speed_hz = max_speed_hz;
+	ret = spinand_init(chip);
+	if (ret)
+		goto err5;
+
+	mtd_set_of_node(mtd, spi->dev.of_node);
+
+	ret = mtd_device_register(mtd, NULL, 0);
+	if (!ret)
+		return 0;
+
+err5:
+	spinand_cleanup(chip);
+err4:
+	kfree(ecc_engine);
+err3:
+	kfree(controller);
+err2:
+	kfree(chip);
+err1:
+	return ret;
+}
+
+static int generic_spinand_remove(struct spi_device *spi)
+{
+	struct spinand_device *chip = spi_get_drvdata(spi);
+	struct mtd_info *mtd = spinand_to_mtd(chip);
+
+	mtd_device_unregister(mtd);
+	spinand_cleanup(chip);
+	kfree(chip->ecc.engine);
+	kfree(chip->controller);
+	kfree(chip);
+
+	return 0;
+}
+
+static struct spi_driver generic_spinand_driver = {
+	.driver = {
+		.name	= "generic_spinand",
+		.owner	= THIS_MODULE,
+	},
+	.probe	= generic_spinand_probe,
+	.remove	= generic_spinand_remove,
+};
+module_spi_driver(generic_spinand_driver);
+
+MODULE_DESCRIPTION("Generic SPI controller to support SPI NAND");
+MODULE_AUTHOR("Peter Pan<peterpandong@micron.com>");
+MODULE_LICENSE("GPL v2");
-- 
1.9.1

[PATCH v3 8/8] MAINTAINERS: Add SPI NAND entry

[Peter Pan]

Add maintainer information for SPI NAND.

Signed-off-by: Peter Pan <peterpandong@micron.com>
---
 MAINTAINERS | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 09ec991..dc38bda 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8610,6 +8610,15 @@ S:	Maintained
 F:	drivers/mtd/nand/
 F:	include/linux/mtd/*nand*.h
 
+SPI NAND FLASH DRIVER
+M:	Peter Pan <peterpandong@micron.com>
+L:	linux-mtd@lists.infradead.org
+W:	http://www.linux-mtd.infradead.org/
+Q:	http://patchwork.ozlabs.org/project/linux-mtd/list/
+S:	Maintained
+F:	drivers/mtd/nand/spi/
+F:	include/linux/mtd/spinand.h
+
 NATSEMI ETHERNET DRIVER (DP8381x)
 S:	Orphan
 F:	drivers/net/ethernet/natsemi/natsemi.c
-- 
1.9.1

Re: [PATCH v3 3/8] nand: spi: add basic blocks for infrastructure

[Boris Brezillon]

On Thu, 16 Mar 2017 14:47:32 +0800
Peter Pan <peterpandong@micron.com> wrote:

> +
> +#define SPINAND_CAP_RD_X1 BIT(0)
> +#define SPINAND_CAP_RD_X2 BIT(1)
> +#define SPINAND_CAP_RD_X4 BIT(2)
> +#define SPINAND_CAP_RD_DUAL BIT(3)
> +#define SPINAND_CAP_RD_QUAD BIT(4)
> +#define SPINAND_CAP_WR_X1 BIT(5)
> +#define SPINAND_CAP_WR_X2 BIT(6)
> +#define SPINAND_CAP_WR_X4 BIT(7)
> +#define SPINAND_CAP_WR_DUAL BIT(8)
> +#define SPINAND_CAP_WR_QUAD BIT(9)
> +#define SPINAND_CAP_HW_ECC BIT(10)

Empty line please.

> +struct spinand_controller {
> +	struct spinand_controller_ops *ops;
> +	u32 caps;
> +	void *priv;

Nope, the ->priv field is a per-device private data, so it should be
placed in struct spinand_device (see below), otherwise, if you have the
same controller connected to 2 different chips, each time you call
spinand_set_controller_data() you will overwrite the ->priv value.

Each spinand_controller should then inherit from struct
spinand_controller:

struct my_spinand_controller {
	struct spinand_controller base;

	/* put your SPI NAND controller specific fields here. */
};

> +};
> +
> +/**
> + * struct spinand_device - SPI-NAND Private Flash Chip Data
> + * @base: NAND device instance
> + * @lock: protection lock
> + * @name: name of the chip
> + * @id: ID structure
> + * @read_cache_op: Opcode of read from cache
> + * @write_cache_op: Opcode of program load
> + * @buf: buffer for read/write data
> + * @oobbuf: buffer for read/write oob
> + * @rw_mode: read/write mode of SPI NAND chip
> + * @controller: SPI NAND controller instance
> + * @manufacturer: SPI NAND manufacturer instance, describe
> + *                manufacturer related objects
> + * @ecc_engine: SPI NAND ECC engine instance
> + */
> +struct spinand_device {
> +	struct nand_device base;
> +	struct mutex lock;
> +	char *name;
> +	struct spinand_id id;
> +	u8 read_cache_op;
> +	u8 write_cache_op;
> +	u8 *buf;
> +	u8 *oobbuf;
> +	u32 rw_mode;
> +	struct spinand_controller *controller;

	struct {
		struct spinand_controller *controller;
		void *priv;
	} controller;

> +	struct {
> +		const struct spinand_manufacturer *manu;
> +		void *priv;
> +	} manufacturer;
> +	struct {
> +		struct spinand_ecc_engine *engine;
> +		void *context;
> +	} ecc;
> +};

Re: [PATCH v3 3/8] nand: spi: add basic blocks for infrastructure

[Peter Pan]

Hi Boris,

On Thu, Mar 16, 2017 at 5:55 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Thu, 16 Mar 2017 14:47:32 +0800
> Peter Pan <peterpandong@micron.com> wrote:
>
>> +
>> +#define SPINAND_CAP_RD_X1 BIT(0)
>> +#define SPINAND_CAP_RD_X2 BIT(1)
>> +#define SPINAND_CAP_RD_X4 BIT(2)
>> +#define SPINAND_CAP_RD_DUAL BIT(3)
>> +#define SPINAND_CAP_RD_QUAD BIT(4)
>> +#define SPINAND_CAP_WR_X1 BIT(5)
>> +#define SPINAND_CAP_WR_X2 BIT(6)
>> +#define SPINAND_CAP_WR_X4 BIT(7)
>> +#define SPINAND_CAP_WR_DUAL BIT(8)
>> +#define SPINAND_CAP_WR_QUAD BIT(9)
>> +#define SPINAND_CAP_HW_ECC BIT(10)
>
> Empty line please.

Fix this in v4

>
>> +struct spinand_controller {
>> +     struct spinand_controller_ops *ops;
>> +     u32 caps;
>> +     void *priv;
>
> Nope, the ->priv field is a per-device private data, so it should be
> placed in struct spinand_device (see below), otherwise, if you have the
> same controller connected to 2 different chips, each time you call
> spinand_set_controller_data() you will overwrite the ->priv value.
>
> Each spinand_controller should then inherit from struct
> spinand_controller:
>
> struct my_spinand_controller {
>         struct spinand_controller base;
>
>         /* put your SPI NAND controller specific fields here. */
> };

Yes, you are right Boris, I wasn't think so deeply. I will fix this in v4

Thanks
Peter Pan

Re: [PATCH v3 0/8] Introduction to SPI NAND framework

[Arnaud Mouiche]

Hi All,

Do some test on MT29F1G01AAADD.
Finally, I don't have MT29F1G01AAADD spares
(Peter, If you have 1 or 2 MT29F1G01AAADD samples to send to me, I would 
be pleased).

MT29Fn1G01AAADD are little different. They have 64bytes oob and ecc 
layout is different. But all the rest should be the nearly same.
I did some change to support this device, and I'm now locked in the bad 
block scan, as for some reasons, the scan tries to read all pages from 
all blocks...

I will comment the different things I have seen in the various patches.

Arnaud

On 16/03/2017 07:47, Peter Pan wrote:
> First of all, thanks for Boris Brezillon, Arnaud Mouiche and
> Thomas Petazzoni. This v3 cannot come out without your valuable
> comments on v2.
>
> This series introductes a SPI NAND framework.
> SPI NAND is a new NAND family device with SPI protocol as
> its interface. And its command set is totally different
> with parallel NAND.
>
> Our first attempt was more than 2 years ago[1]. At that
> time, I didn't make BBT shareable and there were too many
> duplicate code with parallel NAND, so that serie stoped.
> But the discussion never stops. Now Boris has a plan to
> make a generic NAND framework which can be shared with
> both parallel and SPI NAND. Now the first step of the
> new generic NAND framework is finished. And it is waiting
> for a user. After discussion with Boris. We both think it's
> time to rebuild SPI NAND framework based on the new NAND
> framework and send out for reviewing.
>
> This series is based on Boris's nand/generic branch[2], which
> is on 4.11-rc1. In this serie, BBT code is totally shared.
> Of course SPI NAND can share more code with parallel, this
> requires to put more in new NAND core (now only BBT included).
> I'd like to send this serie out first, then we can decide
> which part should be in new NAND core.
>
> This series only supports basic SPI NAND features and uses
> generic spi controller for data transfer, on-die ECC for data
> correction. Support advanced features and specific SPI NAND
> controller with hardware ECC is the next step.
>
> This series is tested on Xilinx Zedboard with Micron
> MT29F2G01ABAGDSF SPI NAND chip.
>
>
> [1]http://lists.infradead.org/pipermail/linux-mtd/2015-January/057223.html
> [2]https://github.com/bbrezillon/linux-0day/tree/nand/generic
>
> v3 changes:
> - rebase patch on 4.11-rc1[1]
> - change read ID method. read 4 bytes ID out then let ->detect() of each
>    manufacutre driver to decode ID and detect the device.
> - make SPI NAND id table private to each manufacutre driver
> - fix coding style to make checkpatch.pl happy
> - update the MAINTAINERS file for spi nand code
> - add nand_size() helper in nand.h
> - use nand_for_each_page() helper in spinand_do_read/write_ops()
> - create helper to check boundaries in generic NAND code and use it
>    in SPI NAND core
> - rename spinand_base.c to core.c
> - manufactures' drivers expose spinand_manufacturer struct instead of
>    spinand_manufacturer_ops struct to keep Manufacture ID macro in
>    manufactures' drivers and rename spinand_ids.c to manufacture.c
> - rename spinand_micron.c to micron.c
> - rename chips/ directory to controllers/
> - rename generic_spi.c to generic-spi.c
> - replace ->build_column_addr() and ->get_dummy() hooks with ->prepare_op() in
>    spinand_manufacturer_ops struct
> - rename __spinand_erase() to spinand_erase()
> - rename spinand_erase() to spinand_erase_skip_bbt()
> - rename spinand_scan_ident() to spinand_detect()
> - rename spinand_scan_tail() to spinand_init()
> - move non detect related code from spinand_detect() to spinand_init()
> - remove spinand_fill_nandd, assign nand->ops in spinand_detect()
> - merge v2 patch 3(bad block support) and patch 4(BBT support)
> - drop getchip parameter, remove spinand_get/remove_device(), take the lock
>    by caller directly
> - fix function comment headers
> - use nand_bbt_is_initialized() helper
> - replace spinand_ecc_engine and spinand_controller object in spinand_device
>    struct with pointer
> - replace struct spinand_manufacturer_ops pointer in spinand_device struct
>    with spinand_manufacturer struct
>
>
> v2 changes:
> - replace "spi_nand" with "spinand".
> - rename spi nand related structs for better understanding.
> - introduce spi nand controller, manufacturer and ecc_engine struct.
> - add spi nand manufacturer initialization function refer to Boris's
>    manuf-init branch.
> - remove NAND_SKIP_BBTSCAN from series. Add it later when enabling HW ECC.
> - reorganize series according to Boris's suggestion.
>
> Peter Pan (8):
>    mtd: nand: add more helpers in nand.h
>    mtd: nand: add oob iterator in nand_for_each_page
>    nand: spi: add basic blocks for infrastructure
>    nand: spi: add basic operations support
>    nand: spi: Add bad block support
>    nand: spi: add Micron spi nand support
>    nand: spi: Add generic SPI controller support
>    MAINTAINERS: Add SPI NAND entry
>
>   MAINTAINERS                                    |    9 +
>   drivers/mtd/nand/Kconfig                       |    1 +
>   drivers/mtd/nand/Makefile                      |    1 +
>   drivers/mtd/nand/spi/Kconfig                   |    7 +
>   drivers/mtd/nand/spi/Makefile                  |    4 +
>   drivers/mtd/nand/spi/controllers/Kconfig       |    5 +
>   drivers/mtd/nand/spi/controllers/Makefile      |    1 +
>   drivers/mtd/nand/spi/controllers/generic-spi.c |  158 +++
>   drivers/mtd/nand/spi/core.c                    | 1456 ++++++++++++++++++++++++
>   drivers/mtd/nand/spi/manufactures.c            |   27 +
>   drivers/mtd/nand/spi/micron.c                  |  243 ++++
>   include/linux/mtd/nand.h                       |   87 +-
>   include/linux/mtd/spinand.h                    |  285 +++++
>   13 files changed, 2281 insertions(+), 3 deletions(-)
>   create mode 100644 drivers/mtd/nand/spi/Kconfig
>   create mode 100644 drivers/mtd/nand/spi/Makefile
>   create mode 100644 drivers/mtd/nand/spi/controllers/Kconfig
>   create mode 100644 drivers/mtd/nand/spi/controllers/Makefile
>   create mode 100644 drivers/mtd/nand/spi/controllers/generic-spi.c
>   create mode 100644 drivers/mtd/nand/spi/core.c
>   create mode 100644 drivers/mtd/nand/spi/manufactures.c
>   create mode 100644 drivers/mtd/nand/spi/micron.c
>   create mode 100644 include/linux/mtd/spinand.h
>

Re: [PATCH v3 3/8] nand: spi: add basic blocks for infrastructure

[Arnaud Mouiche]

[...]
> +/*
> + * spinand_detect - [SPI NAND Interface] detect the SPI NAND device
> + * @chip: SPI NAND device structure
> + */
> +int spinand_detect(struct spinand_device *chip)
> +{
> +	struct nand_device *nand = &chip->base;
> +	u8 raw_id[SPINAND_MAX_ID_LEN] = {0};
> +	u8 id[SPINAND_MAX_ID_LEN] = {0};
> +	int ret;
> +
> +	spinand_reset(chip);
> +	spinand_read_id(chip, raw_id);
> +	ret = spinand_manufacturer_detect(chip, raw_id, id);
> +	if (ret)

Would be great to return an error or warning message when the raw_id 
doesn't match any known manufacturer, or when the manufacturer doesn't 
know about this device.
something like:

     pr_err("SPI NAND: unknown raw ID %*phN\n", SPINAND_MAX_ID_LEN, raw_id);

> +		return ret;
> +
> +	spinand_fill_id(chip, id);
> +
> +	pr_info("SPI NAND: %s is found.\n", chip->name);
> +	pr_info("Manufacturer ID: 0x%02x, Chip ID: 0x%02x\n",
> +		spinand_get_mfr_id(chip), spinand_get_dev_id(chip));
> +	pr_info("%d MiB, block size: %d KiB, page size: %d, OOB size: %d\n",
> +		(int)(nand_size(nand) >> 20), nand_eraseblock_size(nand) >> 10,
> +		nand_page_size(nand), nand_per_page_oobsize(nand));
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(spinand_detect);
> +

Re: [PATCH v3 3/8] nand: spi: add basic blocks for infrastructure

[Peter Pan]

Hi Arnaud,

On Fri, Mar 17, 2017 at 6:20 PM, Arnaud Mouiche
<arnaud.mouiche@gmail.com> wrote:
> [...]
>>
>> +/*
>> + * spinand_detect - [SPI NAND Interface] detect the SPI NAND device
>> + * @chip: SPI NAND device structure
>> + */
>> +int spinand_detect(struct spinand_device *chip)
>> +{
>> +       struct nand_device *nand = &chip->base;
>> +       u8 raw_id[SPINAND_MAX_ID_LEN] = {0};
>> +       u8 id[SPINAND_MAX_ID_LEN] = {0};
>> +       int ret;
>> +
>> +       spinand_reset(chip);
>> +       spinand_read_id(chip, raw_id);
>> +       ret = spinand_manufacturer_detect(chip, raw_id, id);
>> +       if (ret)
>
>
> Would be great to return an error or warning message when the raw_id doesn't
> match any known manufacturer, or when the manufacturer doesn't know about
> this device.
> something like:
>
>     pr_err("SPI NAND: unknown raw ID %*phN\n", SPINAND_MAX_ID_LEN, raw_id);
>

Yes, you are right. I missed this part! I will add it in v4

Thanks
Peter Pan

Re: [PATCH v3 4/8] nand: spi: add basic operations support

[Arnaud Mouiche]

On 16/03/2017 07:47, Peter Pan wrote:
> [...]
> +
> +/*
> + * spinand_read_pages - read data from device to buffer
> + * @mtd: MTD device structure
> + * @from: offset to read from
> + * @ops: oob operations description structure
> + * @max_bitflips: maximum bitflip count
> + */
> +static int spinand_read_pages(struct mtd_info *mtd, loff_t from,
> +			      struct mtd_oob_ops *ops,
> +			      unsigned int *max_bitflips)
> +{
> +	struct spinand_device *chip = mtd_to_spinand(mtd);
> +	struct nand_device *nand = mtd_to_nand(mtd);
> +	int size, ret;
> +	unsigned int corrected = 0;
> +	bool ecc_off = ops->mode == MTD_OPS_RAW;
> +	int ooblen = ops->mode == MTD_OPS_AUTO_OOB ?
> +		     mtd->oobavail : mtd->oobsize;
> +	bool oob_only = !ops->datbuf;
> +	struct nand_page_iter iter;
> +
> +	ops->retlen = 0;
> +	ops->oobretlen = 0;
> +	*max_bitflips = 0;
> +

I'm stuck in a infinite bad block scan on the very first nand bad block 
mark read attempt.
Indeed, here I'm in the first page read attempt of scan_block_fast() 
where scan_block_fast() fills "struct mtd_oob_ops ops" the following way
     struct mtd_oob_ops ops;

     ops.ooblen = nand_per_page_oobsize(this);   <= 64
     ops.oobbuf = buf;
     ops.ooboffs = 0;
     ops.datbuf = NULL;
     ops.mode = MTD_OPS_PLACE_OOB;

It just forget to also set ops.len which is left to its uninitialized 
value, and is equal to 0xFFFFFFFF in my case.
Since we only try to read from oob, obviously retlen is never increased, 
and we never except the loop.
But more obviously, either ops.len should have been set to zero 
somewhere because we only read oob, either nand_for_each_page() should 
take in count this fact.

Arnaud
> +	nand_for_each_page(nand, from, ops->len, ops->ooboffs, ops->ooblen,
> +			   ooblen, &iter) {
> +		ret = spinand_do_read_page(mtd, iter.page, ecc_off,
> +					   &corrected, oob_only);
> +		if (ret)
> +			break;
> +		*max_bitflips = max(*max_bitflips, corrected);
> +		if (ops->datbuf) {
> +			size = min_t(int, from + ops->len - iter.offs,
> +				     nand_page_size(nand) - iter.pageoffs);
> +			memcpy(ops->datbuf + ops->retlen,
> +			       chip->buf + iter.pageoffs, size);
> +			ops->retlen += size;
> +		}
> +		if (ops->oobbuf) {
> +			size = min_t(int, iter.oobleft, ooblen);
> +			ret = spinand_transfer_oob(chip,
> +						   ops->oobbuf + ops->oobretlen,
> +						   ops, size);
> +			if (ret) {
> +				pr_err("Transfer oob error %d\n", ret);
> +				return ret;
> +			}
> +			ops->oobretlen += size;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +/*
> + * spinand_do_read_ops - read data from device to buffer
> + * @mtd: MTD device structure
> + * @from: offset to read from
> + * @ops: oob operations description structure
> + */
> +static int spinand_do_read_ops(struct mtd_info *mtd, loff_t from,
> +			       struct mtd_oob_ops *ops)
> +{
> +	struct spinand_device *chip = mtd_to_spinand(mtd);
> +	struct nand_device *nand = mtd_to_nand(mtd);
> +	int ret;
> +	struct mtd_ecc_stats stats;
> +	unsigned int max_bitflips = 0;
> +	bool ecc_off = ops->mode == MTD_OPS_RAW;
> +
> +	if (!valid_nand_address(nand, from)) {
> +		pr_err("%s: invalid read address\n", __func__);
> +		return -EINVAL;
> +	}
> +	if ((ops->ooblen > 0) && !valid_nand_oob_ops(nand, from, ops)) {
> +		pr_err("%s: invalid oob operation input\n", __func__);
> +		return -EINVAL;
> +	}
> +	mutex_lock(&chip->lock);
> +	stats = mtd->ecc_stats;
> +	if (ecc_off)
> +		spinand_disable_ecc(chip);
> +	ret = spinand_read_pages(mtd, from, ops, &max_bitflips);
> +	if (ecc_off)
> +		spinand_enable_ecc(chip);
> +	if (ret)
> +		goto out;
> +
> +	if (mtd->ecc_stats.failed - stats.failed) {
> +		ret = -EBADMSG;
> +		goto out;
> +	}
> +	ret = max_bitflips;
> +
> +out:
> +	mutex_unlock(&chip->lock);
> +	return ret;
> +}
> +

Re: [PATCH v3 0/8] Introduction to SPI NAND framework

[Peter Pan]

Hi Arnaud,

First of all, thanks a lot for your effort to test the patches.

On Fri, Mar 17, 2017 at 6:02 PM, Arnaud Mouiche
<arnaud.mouiche@gmail.com> wrote:
> Hi All,
>
> Do some test on MT29F1G01AAADD.
> Finally, I don't have MT29F1G01AAADD spares
> (Peter, If you have 1 or 2 MT29F1G01AAADD samples to send to me, I would be
> pleased).

Unfortunately, I don't have extra MT29F1G01AAADD on my hand, but I will ask
my college to send you some if sample is available. Could you send me
the ship info
please?

>
> MT29Fn1G01AAADD are little different. They have 64bytes oob and ecc layout
> is different. But all the rest should be the nearly same.

Yes, the only difference is oob size, ecc layout and ecc strength as
far as I can see.

> I did some change to support this device, and I'm now locked in the bad
> block scan, as for some reasons, the scan tries to read all pages from all
> blocks...

Actually I met the same issue when testing v3. And I should fix it once.
Maybe I lost some code when making patches, I didn't use an empty chip
for testing
before sending patches out, so BBT scan could be broken. I'm sorry for
missing this part...
I will check it next Monday.


Thanks,
Peter Pan

Re: [PATCH v3 4/8] nand: spi: add basic operations support

[Peter Pan]

Hi Arnaud,

On Fri, Mar 17, 2017 at 6:33 PM, Arnaud Mouiche
<arnaud.mouiche@gmail.com> wrote:
>
>
> On 16/03/2017 07:47, Peter Pan wrote:
>>
>> [...]
>>
>> +
>> +/*
>> + * spinand_read_pages - read data from device to buffer
>> + * @mtd: MTD device structure
>> + * @from: offset to read from
>> + * @ops: oob operations description structure
>> + * @max_bitflips: maximum bitflip count
>> + */
>> +static int spinand_read_pages(struct mtd_info *mtd, loff_t from,
>> +                             struct mtd_oob_ops *ops,
>> +                             unsigned int *max_bitflips)
>> +{
>> +       struct spinand_device *chip = mtd_to_spinand(mtd);
>> +       struct nand_device *nand = mtd_to_nand(mtd);
>> +       int size, ret;
>> +       unsigned int corrected = 0;
>> +       bool ecc_off = ops->mode == MTD_OPS_RAW;
>> +       int ooblen = ops->mode == MTD_OPS_AUTO_OOB ?
>> +                    mtd->oobavail : mtd->oobsize;
>> +       bool oob_only = !ops->datbuf;
>> +       struct nand_page_iter iter;
>> +
>> +       ops->retlen = 0;
>> +       ops->oobretlen = 0;
>> +       *max_bitflips = 0;
>> +
>
>
> I'm stuck in a infinite bad block scan on the very first nand bad block mark
> read attempt.
> Indeed, here I'm in the first page read attempt of scan_block_fast() where
> scan_block_fast() fills "struct mtd_oob_ops ops" the following way
>     struct mtd_oob_ops ops;
>
>     ops.ooblen = nand_per_page_oobsize(this);   <= 64
>     ops.oobbuf = buf;
>     ops.ooboffs = 0;
>     ops.datbuf = NULL;
>     ops.mode = MTD_OPS_PLACE_OOB;
>
> It just forget to also set ops.len which is left to its uninitialized value,
> and is equal to 0xFFFFFFFF in my case.
> Since we only try to read from oob, obviously retlen is never increased, and
> we never except the loop.
> But more obviously, either ops.len should have been set to zero somewhere
> because we only read oob, either nand_for_each_page() should take in count
> this fact.

Yes, you are right. I added some code to assign ops->len to 0 when ops->datbuf
is NULL. It is lost somehow...  Thanks for your quick debug.
I'm still thinking whether the caller should guarantee ops->len is 0
when reading
oob only or core.c guarantee it. What's your opinion Boris and Arnaud?

Peter Pan

>
> Arnaud
>
>> +       nand_for_each_page(nand, from, ops->len, ops->ooboffs,
>> ops->ooblen,
>> +                          ooblen, &iter) {
>> +               ret = spinand_do_read_page(mtd, iter.page, ecc_off,
>> +                                          &corrected, oob_only);
>> +               if (ret)
>> +                       break;
>> +               *max_bitflips = max(*max_bitflips, corrected);
>> +               if (ops->datbuf) {
>> +                       size = min_t(int, from + ops->len - iter.offs,
>> +                                    nand_page_size(nand) -
>> iter.pageoffs);
>> +                       memcpy(ops->datbuf + ops->retlen,
>> +                              chip->buf + iter.pageoffs, size);
>> +                       ops->retlen += size;
>> +               }
>> +               if (ops->oobbuf) {
>> +                       size = min_t(int, iter.oobleft, ooblen);
>> +                       ret = spinand_transfer_oob(chip,
>> +                                                  ops->oobbuf +
>> ops->oobretlen,
>> +                                                  ops, size);
>> +                       if (ret) {
>> +                               pr_err("Transfer oob error %d\n", ret);
>> +                               return ret;
>> +                       }
>> +                       ops->oobretlen += size;
>> +               }
>> +       }
>> +
>> +       return ret;
>> +}
>> +
>> +/*
>> + * spinand_do_read_ops - read data from device to buffer
>> + * @mtd: MTD device structure
>> + * @from: offset to read from
>> + * @ops: oob operations description structure
>> + */
>> +static int spinand_do_read_ops(struct mtd_info *mtd, loff_t from,
>> +                              struct mtd_oob_ops *ops)
>> +{
>> +       struct spinand_device *chip = mtd_to_spinand(mtd);
>> +       struct nand_device *nand = mtd_to_nand(mtd);
>> +       int ret;
>> +       struct mtd_ecc_stats stats;
>> +       unsigned int max_bitflips = 0;
>> +       bool ecc_off = ops->mode == MTD_OPS_RAW;
>> +
>> +       if (!valid_nand_address(nand, from)) {
>> +               pr_err("%s: invalid read address\n", __func__);
>> +               return -EINVAL;
>> +       }
>> +       if ((ops->ooblen > 0) && !valid_nand_oob_ops(nand, from, ops)) {
>> +               pr_err("%s: invalid oob operation input\n", __func__);
>> +               return -EINVAL;
>> +       }
>> +       mutex_lock(&chip->lock);
>> +       stats = mtd->ecc_stats;
>> +       if (ecc_off)
>> +               spinand_disable_ecc(chip);
>> +       ret = spinand_read_pages(mtd, from, ops, &max_bitflips);
>> +       if (ecc_off)
>> +               spinand_enable_ecc(chip);
>> +       if (ret)
>> +               goto out;
>> +
>> +       if (mtd->ecc_stats.failed - stats.failed) {
>> +               ret = -EBADMSG;
>> +               goto out;
>> +       }
>> +       ret = max_bitflips;
>> +
>> +out:
>> +       mutex_unlock(&chip->lock);
>> +       return ret;
>> +}
>> +
>
>

Re: [PATCH v3 4/8] nand: spi: add basic operations support

[Boris Brezillon]

On Fri, 17 Mar 2017 18:49:08 +0800
Peter Pan <peterpansjtu@gmail.com> wrote:

> Hi Arnaud,
> 
> On Fri, Mar 17, 2017 at 6:33 PM, Arnaud Mouiche
> <arnaud.mouiche@gmail.com> wrote:
> >
> >
> > On 16/03/2017 07:47, Peter Pan wrote:  
> >>
> >> [...]
> >>
> >> +
> >> +/*
> >> + * spinand_read_pages - read data from device to buffer
> >> + * @mtd: MTD device structure
> >> + * @from: offset to read from
> >> + * @ops: oob operations description structure
> >> + * @max_bitflips: maximum bitflip count
> >> + */
> >> +static int spinand_read_pages(struct mtd_info *mtd, loff_t from,
> >> +                             struct mtd_oob_ops *ops,
> >> +                             unsigned int *max_bitflips)
> >> +{
> >> +       struct spinand_device *chip = mtd_to_spinand(mtd);
> >> +       struct nand_device *nand = mtd_to_nand(mtd);
> >> +       int size, ret;
> >> +       unsigned int corrected = 0;
> >> +       bool ecc_off = ops->mode == MTD_OPS_RAW;
> >> +       int ooblen = ops->mode == MTD_OPS_AUTO_OOB ?
> >> +                    mtd->oobavail : mtd->oobsize;
> >> +       bool oob_only = !ops->datbuf;
> >> +       struct nand_page_iter iter;
> >> +
> >> +       ops->retlen = 0;
> >> +       ops->oobretlen = 0;
> >> +       *max_bitflips = 0;
> >> +  
> >
> >
> > I'm stuck in a infinite bad block scan on the very first nand bad block mark
> > read attempt.
> > Indeed, here I'm in the first page read attempt of scan_block_fast() where
> > scan_block_fast() fills "struct mtd_oob_ops ops" the following way
> >     struct mtd_oob_ops ops;
> >
> >     ops.ooblen = nand_per_page_oobsize(this);   <= 64
> >     ops.oobbuf = buf;
> >     ops.ooboffs = 0;
> >     ops.datbuf = NULL;
> >     ops.mode = MTD_OPS_PLACE_OOB;
> >
> > It just forget to also set ops.len which is left to its uninitialized value,
> > and is equal to 0xFFFFFFFF in my case.
> > Since we only try to read from oob, obviously retlen is never increased, and
> > we never except the loop.
> > But more obviously, either ops.len should have been set to zero somewhere
> > because we only read oob, either nand_for_each_page() should take in count
> > this fact.  
> 
> Yes, you are right. I added some code to assign ops->len to 0 when ops->datbuf
> is NULL. It is lost somehow...  Thanks for your quick debug.
> I'm still thinking whether the caller should guarantee ops->len is 0
> when reading
> oob only or core.c guarantee it. What's your opinion Boris and Arnaud?

Hm, we could add something to the core to check the mtd op consistency,
but, in any case, we should probably fix the nand-bbt code to
initialize the ops object to 0:

	struct mtd_oob_ops ops = { };

Re: [PATCH v3 4/8] nand: spi: add basic operations support

[Peter Pan]

Hi Boris,

On Fri, Mar 17, 2017 at 7:02 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Fri, 17 Mar 2017 18:49:08 +0800
> Peter Pan <peterpansjtu@gmail.com> wrote:
>
>> Hi Arnaud,
>>
>> On Fri, Mar 17, 2017 at 6:33 PM, Arnaud Mouiche
>> <arnaud.mouiche@gmail.com> wrote:
>> >
>> >
>> > On 16/03/2017 07:47, Peter Pan wrote:
>> >>
>> >> [...]
>> >>
>> >> +
>> >> +/*
>> >> + * spinand_read_pages - read data from device to buffer
>> >> + * @mtd: MTD device structure
>> >> + * @from: offset to read from
>> >> + * @ops: oob operations description structure
>> >> + * @max_bitflips: maximum bitflip count
>> >> + */
>> >> +static int spinand_read_pages(struct mtd_info *mtd, loff_t from,
>> >> +                             struct mtd_oob_ops *ops,
>> >> +                             unsigned int *max_bitflips)
>> >> +{
>> >> +       struct spinand_device *chip = mtd_to_spinand(mtd);
>> >> +       struct nand_device *nand = mtd_to_nand(mtd);
>> >> +       int size, ret;
>> >> +       unsigned int corrected = 0;
>> >> +       bool ecc_off = ops->mode == MTD_OPS_RAW;
>> >> +       int ooblen = ops->mode == MTD_OPS_AUTO_OOB ?
>> >> +                    mtd->oobavail : mtd->oobsize;
>> >> +       bool oob_only = !ops->datbuf;
>> >> +       struct nand_page_iter iter;
>> >> +
>> >> +       ops->retlen = 0;
>> >> +       ops->oobretlen = 0;
>> >> +       *max_bitflips = 0;
>> >> +
>> >
>> >
>> > I'm stuck in a infinite bad block scan on the very first nand bad block mark
>> > read attempt.
>> > Indeed, here I'm in the first page read attempt of scan_block_fast() where
>> > scan_block_fast() fills "struct mtd_oob_ops ops" the following way
>> >     struct mtd_oob_ops ops;
>> >
>> >     ops.ooblen = nand_per_page_oobsize(this);   <= 64
>> >     ops.oobbuf = buf;
>> >     ops.ooboffs = 0;
>> >     ops.datbuf = NULL;
>> >     ops.mode = MTD_OPS_PLACE_OOB;
>> >
>> > It just forget to also set ops.len which is left to its uninitialized value,
>> > and is equal to 0xFFFFFFFF in my case.
>> > Since we only try to read from oob, obviously retlen is never increased, and
>> > we never except the loop.
>> > But more obviously, either ops.len should have been set to zero somewhere
>> > because we only read oob, either nand_for_each_page() should take in count
>> > this fact.
>>
>> Yes, you are right. I added some code to assign ops->len to 0 when ops->datbuf
>> is NULL. It is lost somehow...  Thanks for your quick debug.
>> I'm still thinking whether the caller should guarantee ops->len is 0
>> when reading
>> oob only or core.c guarantee it. What's your opinion Boris and Arnaud?
>
> Hm, we could add something to the core to check the mtd op consistency,
> but, in any case, we should probably fix the nand-bbt code to
> initialize the ops object to 0:
>
>         struct mtd_oob_ops ops = { };

I will fix the bbt code in v4 if you like. Add warning message in core.c?

Peter Pan

Re: [PATCH v3 4/8] nand: spi: add basic operations support

[Boris Brezillon]

On Fri, 17 Mar 2017 19:09:38 +0800
Peter Pan <peterpansjtu@gmail.com> wrote:

> Hi Boris,
> 
> On Fri, Mar 17, 2017 at 7:02 PM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > On Fri, 17 Mar 2017 18:49:08 +0800
> > Peter Pan <peterpansjtu@gmail.com> wrote:
> >  
> >> Hi Arnaud,
> >>
> >> On Fri, Mar 17, 2017 at 6:33 PM, Arnaud Mouiche
> >> <arnaud.mouiche@gmail.com> wrote:  
> >> >
> >> >
> >> > On 16/03/2017 07:47, Peter Pan wrote:  
> >> >>
> >> >> [...]
> >> >>
> >> >> +
> >> >> +/*
> >> >> + * spinand_read_pages - read data from device to buffer
> >> >> + * @mtd: MTD device structure
> >> >> + * @from: offset to read from
> >> >> + * @ops: oob operations description structure
> >> >> + * @max_bitflips: maximum bitflip count
> >> >> + */
> >> >> +static int spinand_read_pages(struct mtd_info *mtd, loff_t from,
> >> >> +                             struct mtd_oob_ops *ops,
> >> >> +                             unsigned int *max_bitflips)
> >> >> +{
> >> >> +       struct spinand_device *chip = mtd_to_spinand(mtd);
> >> >> +       struct nand_device *nand = mtd_to_nand(mtd);
> >> >> +       int size, ret;
> >> >> +       unsigned int corrected = 0;
> >> >> +       bool ecc_off = ops->mode == MTD_OPS_RAW;
> >> >> +       int ooblen = ops->mode == MTD_OPS_AUTO_OOB ?
> >> >> +                    mtd->oobavail : mtd->oobsize;
> >> >> +       bool oob_only = !ops->datbuf;
> >> >> +       struct nand_page_iter iter;
> >> >> +
> >> >> +       ops->retlen = 0;
> >> >> +       ops->oobretlen = 0;
> >> >> +       *max_bitflips = 0;
> >> >> +  
> >> >
> >> >
> >> > I'm stuck in a infinite bad block scan on the very first nand bad block mark
> >> > read attempt.
> >> > Indeed, here I'm in the first page read attempt of scan_block_fast() where
> >> > scan_block_fast() fills "struct mtd_oob_ops ops" the following way
> >> >     struct mtd_oob_ops ops;
> >> >
> >> >     ops.ooblen = nand_per_page_oobsize(this);   <= 64
> >> >     ops.oobbuf = buf;
> >> >     ops.ooboffs = 0;
> >> >     ops.datbuf = NULL;
> >> >     ops.mode = MTD_OPS_PLACE_OOB;
> >> >
> >> > It just forget to also set ops.len which is left to its uninitialized value,
> >> > and is equal to 0xFFFFFFFF in my case.
> >> > Since we only try to read from oob, obviously retlen is never increased, and
> >> > we never except the loop.
> >> > But more obviously, either ops.len should have been set to zero somewhere
> >> > because we only read oob, either nand_for_each_page() should take in count
> >> > this fact.  
> >>
> >> Yes, you are right. I added some code to assign ops->len to 0 when ops->datbuf
> >> is NULL. It is lost somehow...  Thanks for your quick debug.
> >> I'm still thinking whether the caller should guarantee ops->len is 0
> >> when reading
> >> oob only or core.c guarantee it. What's your opinion Boris and Arnaud?  
> >
> > Hm, we could add something to the core to check the mtd op consistency,
> > but, in any case, we should probably fix the nand-bbt code to
> > initialize the ops object to 0:
> >
> >         struct mtd_oob_ops ops = { };  
> 
> I will fix the bbt code in v4 if you like. Add warning message in core.c?

Add an error message, and return an error. Not sure silently fixing the
problem is the right thing to do.

Re: [PATCH v3 4/8] nand: spi: add basic operations support

[Peter Pan]

On Fri, Mar 17, 2017 at 7:12 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Fri, 17 Mar 2017 19:09:38 +0800
> Peter Pan <peterpansjtu@gmail.com> wrote:
>
>> Hi Boris,
>>
>> On Fri, Mar 17, 2017 at 7:02 PM, Boris Brezillon
>> <boris.brezillon@free-electrons.com> wrote:
>> > On Fri, 17 Mar 2017 18:49:08 +0800
>> > Peter Pan <peterpansjtu@gmail.com> wrote:
>> >
>> >> Hi Arnaud,
>> >>
>> >> On Fri, Mar 17, 2017 at 6:33 PM, Arnaud Mouiche
>> >> <arnaud.mouiche@gmail.com> wrote:
>> >> >
>> >> >
>> >> > On 16/03/2017 07:47, Peter Pan wrote:
>> >> >>
>> >> >> [...]
>> >> >>
>> >> >> +
>> >> >> +/*
>> >> >> + * spinand_read_pages - read data from device to buffer
>> >> >> + * @mtd: MTD device structure
>> >> >> + * @from: offset to read from
>> >> >> + * @ops: oob operations description structure
>> >> >> + * @max_bitflips: maximum bitflip count
>> >> >> + */
>> >> >> +static int spinand_read_pages(struct mtd_info *mtd, loff_t from,
>> >> >> +                             struct mtd_oob_ops *ops,
>> >> >> +                             unsigned int *max_bitflips)
>> >> >> +{
>> >> >> +       struct spinand_device *chip = mtd_to_spinand(mtd);
>> >> >> +       struct nand_device *nand = mtd_to_nand(mtd);
>> >> >> +       int size, ret;
>> >> >> +       unsigned int corrected = 0;
>> >> >> +       bool ecc_off = ops->mode == MTD_OPS_RAW;
>> >> >> +       int ooblen = ops->mode == MTD_OPS_AUTO_OOB ?
>> >> >> +                    mtd->oobavail : mtd->oobsize;
>> >> >> +       bool oob_only = !ops->datbuf;
>> >> >> +       struct nand_page_iter iter;
>> >> >> +
>> >> >> +       ops->retlen = 0;
>> >> >> +       ops->oobretlen = 0;
>> >> >> +       *max_bitflips = 0;
>> >> >> +
>> >> >
>> >> >
>> >> > I'm stuck in a infinite bad block scan on the very first nand bad block mark
>> >> > read attempt.
>> >> > Indeed, here I'm in the first page read attempt of scan_block_fast() where
>> >> > scan_block_fast() fills "struct mtd_oob_ops ops" the following way
>> >> >     struct mtd_oob_ops ops;
>> >> >
>> >> >     ops.ooblen = nand_per_page_oobsize(this);   <= 64
>> >> >     ops.oobbuf = buf;
>> >> >     ops.ooboffs = 0;
>> >> >     ops.datbuf = NULL;
>> >> >     ops.mode = MTD_OPS_PLACE_OOB;
>> >> >
>> >> > It just forget to also set ops.len which is left to its uninitialized value,
>> >> > and is equal to 0xFFFFFFFF in my case.
>> >> > Since we only try to read from oob, obviously retlen is never increased, and
>> >> > we never except the loop.
>> >> > But more obviously, either ops.len should have been set to zero somewhere
>> >> > because we only read oob, either nand_for_each_page() should take in count
>> >> > this fact.
>> >>
>> >> Yes, you are right. I added some code to assign ops->len to 0 when ops->datbuf
>> >> is NULL. It is lost somehow...  Thanks for your quick debug.
>> >> I'm still thinking whether the caller should guarantee ops->len is 0
>> >> when reading
>> >> oob only or core.c guarantee it. What's your opinion Boris and Arnaud?
>> >
>> > Hm, we could add something to the core to check the mtd op consistency,
>> > but, in any case, we should probably fix the nand-bbt code to
>> > initialize the ops object to 0:
>> >
>> >         struct mtd_oob_ops ops = { };
>>
>> I will fix the bbt code in v4 if you like. Add warning message in core.c?
>
> Add an error message, and return an error. Not sure silently fixing the
> problem is the right thing to do.

I see,

Re: [PATCH v3 5/8] nand: spi: Add bad block support

[Arnaud Mouiche]

On 16/03/2017 07:47, Peter Pan wrote:
> [...]
> +
> +/*
> + * spinand_scan_bbt - scan BBT in SPI NAND device
> + * @chip: SPI NAND device structure
> + */
> +static int spinand_scan_bbt(struct spinand_device *chip)
> +{
> +	struct nand_device *nand = &chip->base;
> +	int ret;
> +
> +	nand->bbt.options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
> +	nand->bbt.td = NULL;
> +	nand->bbt.md = NULL;
> +
> +	ret = spinand_create_badblock_pattern(chip);
> +	if (ret)
> +		return ret;
> +
> +	return nand_scan_bbt(nand);
> +}
> +

Boris, Peter,

I'm not a big fan of NAND_BBT_USE_FLASH for small capacity nand flash 
(eg. 1Gb with 1024 blocks, where a complete bad block scan on boot is 
fast enough).
Do you consider NAND_BBT_USE_FLASH as mandatory, or does a optional 
"of_get_nand_on_flash_bbt(dn))" device tree configuration is something 
possible ?

Regards,
Arnaud

> +/*
>    * spinand_manufacturer_detect - detect SPI NAND device by each manufacturer
>    * @chip: SPI NAND device structure
>    * @raw_id: raw id buffer. raw id is read by spinand_read_id(), should be
> @@ -1120,6 +1394,7 @@ int spinand_init(struct spinand_device *chip)
>   		return -ENOMEM;
>   
>   	chip->oobbuf = chip->buf + nand_page_size(nand);
> +	nand->ops = &spinand_ops;
>   	mtd->name = chip->name;
>   	mtd->size = nand_size(nand);
>   	mtd->erasesize = nand_eraseblock_size(nand);
> @@ -1137,11 +1412,14 @@ int spinand_init(struct spinand_device *chip)
>   	if (ret < 0)
>   		ret = 0;
>   	mtd->oobavail = ret;
> -	mtd->_erase = spinand_erase;
> +	mtd->_erase = spinand_erase_skip_bbt;
>   	mtd->_read = spinand_read;
>   	mtd->_write = spinand_write;
>   	mtd->_read_oob = spinand_read_oob;
>   	mtd->_write_oob = spinand_write_oob;
> +	mtd->_block_isbad = spinand_block_isbad;
> +	mtd->_block_markbad = spinand_block_markbad;
> +	mtd->_block_isreserved = spinand_block_isreserved;
>   
>   	if (!mtd->bitflip_threshold)
>   		mtd->bitflip_threshold = DIV_ROUND_UP(mtd->ecc_strength * 3,
> @@ -1149,7 +1427,8 @@ int spinand_init(struct spinand_device *chip)
>   	/* After power up, all blocks are locked, so unlock it here. */
>   	spinand_lock_block(chip, BL_ALL_UNLOCKED);
>   
> -	return 0;
> +	/* Build bad block table */
> +	return spinand_scan_bbt(chip);
>   }

Re: [PATCH v3 5/8] nand: spi: Add bad block support

[Boris Brezillon]

On Fri, 17 Mar 2017 13:22:17 +0100
Arnaud Mouiche <arnaud.mouiche@gmail.com> wrote:

> On 16/03/2017 07:47, Peter Pan wrote:
> > [...]
> > +
> > +/*
> > + * spinand_scan_bbt - scan BBT in SPI NAND device
> > + * @chip: SPI NAND device structure
> > + */
> > +static int spinand_scan_bbt(struct spinand_device *chip)
> > +{
> > +	struct nand_device *nand = &chip->base;
> > +	int ret;
> > +
> > +	nand->bbt.options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
> > +	nand->bbt.td = NULL;
> > +	nand->bbt.md = NULL;
> > +
> > +	ret = spinand_create_badblock_pattern(chip);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return nand_scan_bbt(nand);
> > +}
> > +  
> 
> Boris, Peter,
> 
> I'm not a big fan of NAND_BBT_USE_FLASH for small capacity nand flash 
> (eg. 1Gb with 1024 blocks, where a complete bad block scan on boot is 
> fast enough).
> Do you consider NAND_BBT_USE_FLASH as mandatory, or does a optional 
> "of_get_nand_on_flash_bbt(dn))" device tree configuration is something 
> possible ?

It should be optional indeed.

Re: [PATCH v3 1/8] mtd: nand: add more helpers in nand.h

[Boris Brezillon]

On Thu, 16 Mar 2017 14:47:30 +0800
Peter Pan <peterpandong@micron.com> wrote:

> This commit adds some helpers in nand.h
>     nand_size()
>     valid_nand_address()
>     valid_nand_oob_ops()
>     nand_oob_ops_across_page()
>     valid_nand_erase_ops()
> 
> Signed-off-by: Peter Pan <peterpandong@micron.com>
> ---
>  include/linux/mtd/nand.h | 59 ++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 59 insertions(+)
> 
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index c2197b4..4a812e6 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -410,6 +410,65 @@ static inline int nand_neraseblocks(struct nand_device *nand)
>  }
>  
>  /**
> + * nand_size - Get NAND size
> + * @nand: NAND device
> + *
> + * Returns the total size exposed by @nand.
> + */
> +static inline u64 nand_size(struct nand_device *nand)
> +{
> +	return nand->memorg.ndies * nand->memorg.diesize;
> +}
> +
> +static inline bool valid_nand_address(struct nand_device *nand, loff_t addr)

Please keep consistent prefixes. Maybe we should name it
nand_check_address(), return -EINVAL if it's wrong and 0 otherwise:

static inline int nand_check_address(struct nand_device *nand,
				     loff_t addr)
{
	return addr < nand_size(nand) ? 0 : -EINVAL;
}

> +{
> +	return addr < nand_size(nand);
> +}
> +
> +static inline bool valid_nand_oob_ops(struct nand_device *nand, loff_t start,
> +				      struct mtd_oob_ops *ops)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(nand);
> +	int ooblen = ops->mode == MTD_OPS_AUTO_OOB ?
> +		mtd->oobavail : mtd->oobsize;
> +
> +	if (ops->ooboffs >= ooblen)
> +		return false;
> +	if (ops->ooboffs + ops->ooblen >
> +	    (nand_len_to_pages(nand, nand_size(nand)) -
> +			       nand_offs_to_page(nand, start)) * ooblen)
> +		return false;
> +
> +	return true;
> +}

Ditto.

> +
> +static inline bool nand_oob_ops_across_page(struct nand_device *nand,
> +					    struct mtd_oob_ops *ops)
> +{
> +	struct mtd_info *mtd = nand_to_mtd(nand);
> +	int ooblen = ops->mode == MTD_OPS_AUTO_OOB ?
> +		     mtd->oobavail : mtd->oobsize;
> +
> +	return (ops->ooboffs + ops->ooblen) > ooblen;
> +}
> +
> +static inline bool valid_nand_erase_ops(struct nand_device *nand,
> +					struct erase_info *einfo)

Ditto.

> +{
> +	/* check address align on block boundary */
> +	if (einfo->addr & (nand_eraseblock_size(nand) - 1))
> +		return false;
> +	/* check lendth align on block boundary */
> +	if (einfo->len & (nand_eraseblock_size(nand) - 1))
> +		return false;
> +	/* Do not allow erase past end of device */
> +	if ((einfo->addr + einfo->len) > nand_size(nand))
> +		return false;
> +
> +	return true;
> +}
> +
> +/**
>   * nand_register - Register a NAND device
>   * @nand: NAND device
>   *

Re: [PATCH v3 2/8] mtd: nand: add oob iterator in nand_for_each_page

[Boris Brezillon]

On Thu, 16 Mar 2017 14:47:31 +0800
Peter Pan <peterpandong@micron.com> wrote:

> Iterate nand pages by both page and oob operation.
> 
> Signed-off-by: Peter Pan <peterpandong@micron.com>
> ---
>  include/linux/mtd/nand.h | 28 +++++++++++++++++++++++++---
>  1 file changed, 25 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 4a812e6..822547e 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -87,6 +87,9 @@ struct nand_page_iter {
>  	loff_t offs;
>  	int page;
>  	int pageoffs;
> +	int ooboffs;
> +	int oobsize;
> +	size_t oobleft;

Maybe we should also add dataleft so that you don't have to pass start
and len when you call nand_page_iter_end().

>  };
>  
>  /**
> @@ -194,6 +197,7 @@ static inline int nand_per_page_oobsize(struct nand_device *nand)
>   * @iter: page iterator
>   */
>  static inline void nand_page_iter_init(struct nand_device *nand, loff_t offs,
> +				       u32 ooboffs, size_t ooblen, u32 oobsize,
>  				       struct nand_page_iter *iter)
>  {
>  	u64 page = offs;
> @@ -201,6 +205,9 @@ static inline void nand_page_iter_init(struct nand_device *nand, loff_t offs,
>  	iter->pageoffs = do_div(page, nand->memorg.pagesize);
>  	iter->page = page;
>  	iter->offs = offs;
> +	iter->ooboffs = ooboffs;
> +	iter->oobsize = oobsize;
> +	iter->oobleft = ooblen;
>  }
>  
>  /**
> @@ -214,11 +221,26 @@ static inline void nand_page_iter_next(struct nand_device *nand,
>  	iter->page++;
>  	iter->offs += nand_page_size(nand) - iter->pageoffs;
>  	iter->pageoffs = 0;
> +	if (iter->oobleft)
> +		iter->oobleft -= min_t(int, iter->oobsize - iter->ooboffs,
> +				       iter->oobleft);
> +}
> +
> +static inline bool nand_page_iter_end(struct nand_device *nand, loff_t offs,
> +				      u32 len, struct nand_page_iter *iter)
> +{
> +	if (iter->offs < offs + len)
> +		return false;
> +	if (iter->oobleft)
> +		return false;
> +	return true;
>  }
>  
> -#define nand_for_each_page(nand, start, len, iter)		\
> -	for (nand_page_iter_init(nand, start, iter);		\
> -	     (iter)->offs < (start) + (len);			\
> +#define nand_for_each_page(nand, start, len, ooboffs, ooblen,	\
> +			   oobsize, iter)	\
> +	for (nand_page_iter_init(nand, start, ooboffs,		\
> +				 ooblen, oobsize, iter);	\
> +	     !nand_page_iter_end(nand, start, len, iter);		\
>  	     nand_page_iter_next(nand, iter))
>  
>  /**

Re: [PATCH v3 3/8] nand: spi: add basic blocks for infrastructure

[Boris Brezillon]

On Thu, 16 Mar 2017 14:47:32 +0800
Peter Pan <peterpandong@micron.com> wrote:


> +
> +/*
> + * spinand_manufacturer_detect - detect SPI NAND device by each manufacturer
> + * @chip: SPI NAND device structure
> + * @raw_id: raw id buffer. raw id is read by spinand_read_id(), should be
> + *          decoded by manufacturers
> + * @id: real id buffer. manufacturer's ->detect() should put real id in
> + *      this buffer.
> + *
> + * ->detect() should decode raw id and initialize device related part in
> + * spinand_device structure if it is the right device.
> + * ->detect() can not be NULL.
> + */
> +static int spinand_manufacturer_detect(struct spinand_device *chip,
> +				       u8 *raw_id, u8 *id)
> +{
> +	int i = 0;
> +
> +	for (; spinand_manufacturers[i]->id != 0x0; i++) {
> +		if (spinand_manufacturers[i]->ops &&
> +		    spinand_manufacturers[i]->ops->detect) {

AFAIU, ->detect() is mandatory, otherwise you have no way to attach a
NAND to this manufacturer which makes it useless.
Am I missing something?

If I'm right, you should assume that ->detect() is never NULL and
complain loudly if it is.

> +			if (spinand_manufacturers[i]->ops->detect(chip,
> +								  raw_id, id))
> +				return 0;
> +		}
> +	}
> +
> +	return -ENODEV;
> +}
> +
> +/*
> + * spinand_manufacturer_init - manufacturer initialization function.
> + * @chip: SPI NAND device structure
> + *
> + * Manufacturer drivers should put all their specific initialization code in
> + * their ->init() hook.
> + */
> +static int spinand_manufacturer_init(struct spinand_device *chip)
> +{
> +	if (chip->manufacturer.manu->ops && chip->manufacturer.manu->ops->init)
> +		return chip->manufacturer.manu->ops->init(chip);
> +
> +	return 0;
> +}
> +
> +/*
> + * spinand_manufacturer_cleanup - manufacturer cleanup function.
> + * @chip: SPI NAND device structure
> + *
> + * Manufacturer drivers should put all their specific cleanup code in their
> + * ->cleanup() hook.
> + */
> +static void spinand_manufacturer_cleanup(struct spinand_device *chip)
> +{
> +	/* Release manufacturer private data */
> +	if (chip->manufacturer.manu->ops &&
> +	    chip->manufacturer.manu->ops->cleanup)
> +		return chip->manufacturer.manu->ops->cleanup(chip);
> +}
> +
> +/*
> + * spinand_fill_id - fill id in spinand_device structure.
> + * @chip: SPI NAND device structure
> + * @id: id buffer
> + */
> +static void spinand_fill_id(struct spinand_device *chip, u8 *id)
> +{
> +	memcpy(chip->id.data, id, SPINAND_MAX_ID_LEN);
> +	chip->id.len = SPINAND_MAX_ID_LEN;
> +}
> +
> +/*
> + * spinand_get_mfr_id - get device's manufacturer ID.
> + * @chip: SPI NAND device structure
> + */
> +static u8 spinand_get_mfr_id(struct spinand_device *chip)
> +{
> +	return chip->id.data[SPINAND_MFR_ID];
> +}
> +
> +/*
> + * spinand_get_dev_id - get device's device ID.
> + * @chip: SPI NAND device structure
> + */
> +static u8 spinand_get_dev_id(struct spinand_device *chip)
> +{
> +	return chip->id.data[SPINAND_DEV_ID];
> +}

As said below, I'm not sure we need to identify the manufacturer and
device ID bytes if we go for the raw-id approach.

> +
> +/*
> + * spinand_set_manufacturer_ops - set manufacture ops in spinand_device
> + * structure.
> + * @chip: SPI NAND device structure
> + * @mfr_id: device's manufacturer ID
> + */
> +static void spinand_set_manufacturer_ops(struct spinand_device *chip,
> +					 u8 mfr_id)
> +{
> +	int i = 0;
> +
> +	for (; spinand_manufacturers[i]->id != 0x0; i++) {
> +		if (spinand_manufacturers[i]->id == mfr_id)
> +			break;
> +	}
> +
> +	chip->manufacturer.manu = spinand_manufacturers[i];

Clearly something that should be done in spinand_manufacturer_detect()
when the ->detect() method returns true.

> +}
> +
> +/*
> + * spinand_detect - [SPI NAND Interface] detect the SPI NAND device
> + * @chip: SPI NAND device structure
> + */
> +int spinand_detect(struct spinand_device *chip)
> +{
> +	struct nand_device *nand = &chip->base;
> +	u8 raw_id[SPINAND_MAX_ID_LEN] = {0};
> +	u8 id[SPINAND_MAX_ID_LEN] = {0};
> +	int ret;
> +
> +	spinand_reset(chip);
> +	spinand_read_id(chip, raw_id);

Directly store the raw id in chip->id.data.

> +	ret = spinand_manufacturer_detect(chip, raw_id, id);

Why do you need to differentiate raw and non-raw IDs. If we consider the
ID to be the 4 first bytes without any dummy-cycles, then we'd better
store this one in chip->id.data and let manufacturer code parse it.

> +	if (ret)
> +		return ret;
> +
> +	spinand_fill_id(chip, id);
> +
> +	pr_info("SPI NAND: %s is found.\n", chip->name);
> +	pr_info("Manufacturer ID: 0x%02x, Chip ID: 0x%02x\n",
> +		spinand_get_mfr_id(chip), spinand_get_dev_id(chip));

Is this really important to print those raw IDs? How about printing the
manufacturer name (which should be part of struct spinand_manufacturer)
and the model name (a readable string that can be extracted from the
device id during detect).

> +	pr_info("%d MiB, block size: %d KiB, page size: %d, OOB size: %d\n",
> +		(int)(nand_size(nand) >> 20), nand_eraseblock_size(nand) >> 10,
> +		nand_page_size(nand), nand_per_page_oobsize(nand));
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(spinand_detect);


> +struct spinand_manufacturer_ops {
> +	bool (*detect)(struct spinand_device *chip, u8 *raw_id, u8 *id);

Just pass chip, the id should be stored in chip->id.

> +	int (*init)(struct spinand_device *chip);
> +	void (*cleanup)(struct spinand_device *chip);
> +};
> +

Re: [PATCH v3 7/8] nand: spi: Add generic SPI controller support

[Boris Brezillon]

On Thu, 16 Mar 2017 14:47:36 +0800
Peter Pan <peterpandong@micron.com> wrote:

> This commit supports to use generic spi controller
> as spi nand controller.
> 
> Signed-off-by: Peter Pan <peterpandong@micron.com>
> ---
>  drivers/mtd/nand/spi/Kconfig                   |   2 +
>  drivers/mtd/nand/spi/Makefile                  |   1 +
>  drivers/mtd/nand/spi/controllers/Kconfig       |   5 +
>  drivers/mtd/nand/spi/controllers/Makefile      |   1 +
>  drivers/mtd/nand/spi/controllers/generic-spi.c | 158 +++++++++++++++++++++++++
>  5 files changed, 167 insertions(+)
>  create mode 100644 drivers/mtd/nand/spi/controllers/Kconfig
>  create mode 100644 drivers/mtd/nand/spi/controllers/Makefile
>  create mode 100644 drivers/mtd/nand/spi/controllers/generic-spi.c
> 
> diff --git a/drivers/mtd/nand/spi/Kconfig b/drivers/mtd/nand/spi/Kconfig
> index d77c46e..6bd1c65 100644
> --- a/drivers/mtd/nand/spi/Kconfig
> +++ b/drivers/mtd/nand/spi/Kconfig
> @@ -3,3 +3,5 @@ menuconfig MTD_SPI_NAND
>  	depends on MTD_NAND
>  	help
>  	  This is the framework for the SPI NAND device drivers.
> +
> +source "drivers/mtd/nand/spi/controllers/Kconfig"
> diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile
> index db0b91b..6ad5f24 100644
> --- a/drivers/mtd/nand/spi/Makefile
> +++ b/drivers/mtd/nand/spi/Makefile
> @@ -1,3 +1,4 @@
>  obj-$(CONFIG_MTD_SPI_NAND) += core.o
>  obj-$(CONFIG_MTD_SPI_NAND) += manufactures.o
>  obj-$(CONFIG_MTD_SPI_NAND) += micron.o
> +obj-$(CONFIG_MTD_SPI_NAND) += controllers/
> diff --git a/drivers/mtd/nand/spi/controllers/Kconfig b/drivers/mtd/nand/spi/controllers/Kconfig
> new file mode 100644
> index 0000000..8ab7023
> --- /dev/null
> +++ b/drivers/mtd/nand/spi/controllers/Kconfig
> @@ -0,0 +1,5 @@
> +config GENERIC_SPI_NAND
> +	tristate "SPI NAND with generic SPI bus Support"
> +	depends on MTD_SPI_NAND && SPI
> +	help
> +	  This is to support SPI NAND device with generic SPI bus.
> diff --git a/drivers/mtd/nand/spi/controllers/Makefile b/drivers/mtd/nand/spi/controllers/Makefile
> new file mode 100644
> index 0000000..46cbf29
> --- /dev/null
> +++ b/drivers/mtd/nand/spi/controllers/Makefile
> @@ -0,0 +1 @@
> +obj-$(CONFIG_GENERIC_SPI_NAND) += generic-spi.o
> diff --git a/drivers/mtd/nand/spi/controllers/generic-spi.c b/drivers/mtd/nand/spi/controllers/generic-spi.c
> new file mode 100644
> index 0000000..38b4a56
> --- /dev/null
> +++ b/drivers/mtd/nand/spi/controllers/generic-spi.c
> @@ -0,0 +1,158 @@
> +/*
> + * Copyright (c) 2009-2017 Micron Technology, Inc.
> + *
> + * 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/module.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/partitions.h>
> +#include <linux/spi/spi.h>
> +#include <linux/mtd/spinand.h>
> +
> +/*
> + * generic_spinand_cmd_fn - to process a command to send to the SPI NAND
> + * by generic SPI bus
> + * @chip: SPI NAND device structure
> + * @op: SPI NAND operation descriptor
> + */
> +static int generic_spinand_cmd_fn(struct spinand_device *chip,
> +				  struct spinand_op *op)

I'd prefer to see the controller word somewhere in the function name,
like gen_spinand_controller_exec_op().

> +{
> +	struct spi_message message;
> +	struct spi_transfer x[3];
> +	struct spi_device *spi = spinand_get_controller_data(chip);
> +
> +	spi_message_init(&message);
> +	memset(x, 0, sizeof(x));
> +	x[0].len = 1;
> +	x[0].tx_nbits = 1;
> +	x[0].tx_buf = &op->cmd;
> +	spi_message_add_tail(&x[0], &message);
> +
> +	if (op->n_addr + op->dummy_bytes) {
> +		x[1].len = op->n_addr + op->dummy_bytes;
> +		x[1].tx_nbits = op->addr_nbits;
> +		x[1].tx_buf = op->addr;
> +		spi_message_add_tail(&x[1], &message);
> +	}
> +	if (op->n_tx) {
> +		x[2].len = op->n_tx;
> +		x[2].tx_nbits = op->data_nbits;
> +		x[2].tx_buf = op->tx_buf;
> +		spi_message_add_tail(&x[2], &message);
> +	} else if (op->n_rx) {
> +		x[2].len = op->n_rx;
> +		x[2].rx_nbits = op->data_nbits;
> +		x[2].rx_buf = op->rx_buf;
> +		spi_message_add_tail(&x[2], &message);
> +	}
> +	return spi_sync(spi, &message);
> +}
> +
> +static struct spinand_controller_ops generic_spi_ops = {
> +	.exec_op = generic_spinand_cmd_fn,
> +};
> +
> +static int generic_spinand_probe(struct spi_device *spi)
> +{
> +	struct spinand_device *chip;
> +	struct mtd_info *mtd;
> +	struct spinand_controller *controller;
> +	struct spinand_ecc_engine *ecc_engine;
> +	int ret;
> +	u32 max_speed_hz = spi->max_speed_hz;
> +
> +	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
> +	if (!chip) {
> +		ret = -ENOMEM;
> +		goto err1;
> +	}
> +	mtd = spinand_to_mtd(chip);
> +	controller = kzalloc(sizeof(*controller), GFP_KERNEL);
> +	if (!controller) {
> +		ret = -ENOMEM;
> +		goto err2;
> +	}
> +	controller->ops = &generic_spi_ops;
> +	controller->caps = SPINAND_CAP_RD_X1 | SPINAND_CAP_WR_X1;
> +	if (spi->mode & SPI_RX_QUAD)
> +		controller->caps |= SPINAND_CAP_RD_QUAD | SPINAND_CAP_RD_X4;
> +	if (spi->mode & SPI_RX_DUAL)
> +		controller->caps |= SPINAND_CAP_RD_DUAL | SPINAND_CAP_RD_X2;
> +	if (spi->mode & SPI_TX_QUAD)
> +		controller->caps |= SPINAND_CAP_WR_QUAD | SPINAND_CAP_WR_X4;
> +	if (spi->mode & SPI_TX_DUAL)
> +		controller->caps |= SPINAND_CAP_WR_DUAL | SPINAND_CAP_WR_X2;
> +	chip->controller = controller;
> +	ecc_engine = kzalloc(sizeof(*ecc_engine), GFP_KERNEL);
> +	if (!ecc_engine) {
> +		ret = -ENOMEM;
> +		goto err3;
> +	}
> +	ecc_engine->mode = SPINAND_ECC_ONDIE;
> +	chip->ecc.engine = ecc_engine;

The ECC engine is on the chip, so you should let the core (or
manufacturer) code initialize it.

> +	spinand_set_controller_data(chip, spi);
> +	spi_set_drvdata(spi, chip);
> +	spi->max_speed_hz = min_t(int, 25000000, max_speed_hz);
> +	ret = spinand_detect(chip);
> +	if (ret)
> +		goto err4;
> +
> +	spi->max_speed_hz = max_speed_hz;
> +	ret = spinand_init(chip);
> +	if (ret)
> +		goto err5;
> +
> +	mtd_set_of_node(mtd, spi->dev.of_node);

Should be done before calling spinand_detect(), just in case some DT
props need to be parsed before the detection step.

This being said, I'm not sure we should let the spinand controller
driver do the registration step. I'm currently trying to rework the
parallel NAND framework to act like all other busses, and I think SPI
NAND controllers should also follow this road:

1/ spinand controller driver register a controller to the spinand core
  (spinand_controller_register())
2/ the core parses the DT (or board info if DT is not available) and
   creates N spinand devices (the N value depends on the number of SPI
   nands connected to the controller)
3/ for each spinand device the detection/initialization takes place
   directly in the core (the spinand_controller_ops should contain
   anything required to do the detection)
4/ for each spinand dev the spinand_controller_ops->add() is called, to
   let the controller driver attach private data to the device (if
   required), and/or let it use it's own ECC engine (optional as well).
5/ underlying MTD device registered by spinand core code and spinand
   dev added to the list of devices controlled by this controller (done
   in the core)

When removing a device, you just have the reverse steps:

1/ remove from list and unregister the MTD dev
2/ call spinand_controller_ops->remove()
3/ core/manufacturer cleanup

Not sure how feasible this is, especially for the generic SPI NAND
controller case where the SPI NAND controller does not have a node in
the DT, but that would avoid all this boiler-plate duplication we have
in the // NAND framework.

> +
> +	ret = mtd_device_register(mtd, NULL, 0);
> +	if (!ret)
> +		return 0;
> +
> +err5:
> +	spinand_cleanup(chip);
> +err4:
> +	kfree(ecc_engine);
> +err3:
> +	kfree(controller);
> +err2:
> +	kfree(chip);
> +err1:
> +	return ret;
> +}
> +
> +static int generic_spinand_remove(struct spi_device *spi)
> +{
> +	struct spinand_device *chip = spi_get_drvdata(spi);
> +	struct mtd_info *mtd = spinand_to_mtd(chip);
> +
> +	mtd_device_unregister(mtd);
> +	spinand_cleanup(chip);
> +	kfree(chip->ecc.engine);
> +	kfree(chip->controller);
> +	kfree(chip);
> +
> +	return 0;
> +}
> +
> +static struct spi_driver generic_spinand_driver = {
> +	.driver = {
> +		.name	= "generic_spinand",
> +		.owner	= THIS_MODULE,
> +	},
> +	.probe	= generic_spinand_probe,
> +	.remove	= generic_spinand_remove,
> +};
> +module_spi_driver(generic_spinand_driver);
> +
> +MODULE_DESCRIPTION("Generic SPI controller to support SPI NAND");
> +MODULE_AUTHOR("Peter Pan<peterpandong@micron.com>");
> +MODULE_LICENSE("GPL v2");

Re: [PATCH v3 7/8] nand: spi: Add generic SPI controller support

[Arnaud Mouiche]

On 17/03/2017 15:20, Boris Brezillon wrote:
> [...]
> Should be done before calling spinand_detect(), just in case some DT
> props need to be parsed before the detection step.
>
> This being said, I'm not sure we should let the spinand controller
> driver do the registration step. I'm currently trying to rework the
> parallel NAND framework to act like all other busses, and I think SPI
> NAND controllers should also follow this road:
>
> 1/ spinand controller driver register a controller to the spinand core
>    (spinand_controller_register())
> 2/ the core parses the DT (or board info if DT is not available) and
>     creates N spinand devices (the N value depends on the number of SPI
>     nands connected to the controller)
> 3/ for each spinand device the detection/initialization takes place
>     directly in the core (the spinand_controller_ops should contain
>     anything required to do the detection)
> 4/ for each spinand dev the spinand_controller_ops->add() is called, to
>     let the controller driver attach private data to the device (if
>     required), and/or let it use it's own ECC engine (optional as well).
> 5/ underlying MTD device registered by spinand core code and spinand
>     dev added to the list of devices controlled by this controller (done
>     in the core)
>
> When removing a device, you just have the reverse steps:
>
> 1/ remove from list and unregister the MTD dev
> 2/ call spinand_controller_ops->remove()
> 3/ core/manufacturer cleanup
>
> Not sure how feasible this is, especially for the generic SPI NAND
> controller case where the SPI NAND controller does not have a node in
> the DT, but that would avoid all this boiler-plate duplication we have
> in the // NAND framework.

Since the probe for generic spi devices is generally triggered by the 
SPI layer, it will not match easily in the way you would like the 
registration done.
Can we let this registration question not be a showstopper for Peter's 
effort ?

Regards,
Arnaud
>> +
>> +	ret = mtd_device_register(mtd, NULL, 0);
>> +	if (!ret)
>> +		return 0;
>> +
>> +err5:
>> +	spinand_cleanup(chip);
>> +err4:
>> +	kfree(ecc_engine);
>> +err3:
>> +	kfree(controller);
>> +err2:
>> +	kfree(chip);
>> +err1:
>> +	return ret;
>> +}
>> +
>> +static int generic_spinand_remove(struct spi_device *spi)
>> +{
>> +	struct spinand_device *chip = spi_get_drvdata(spi);
>> +	struct mtd_info *mtd = spinand_to_mtd(chip);
>> +
>> +	mtd_device_unregister(mtd);
>> +	spinand_cleanup(chip);
>> +	kfree(chip->ecc.engine);
>> +	kfree(chip->controller);
>> +	kfree(chip);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct spi_driver generic_spinand_driver = {
>> +	.driver = {
>> +		.name	= "generic_spinand",
>> +		.owner	= THIS_MODULE,
>> +	},
>> +	.probe	= generic_spinand_probe,
>> +	.remove	= generic_spinand_remove,
>> +};
>> +module_spi_driver(generic_spinand_driver);
>> +
>> +MODULE_DESCRIPTION("Generic SPI controller to support SPI NAND");
>> +MODULE_AUTHOR("Peter Pan<peterpandong@micron.com>");
>> +MODULE_LICENSE("GPL v2");

Re: [PATCH v3 7/8] nand: spi: Add generic SPI controller support

[Boris Brezillon]

On Fri, 17 Mar 2017 18:32:28 +0100
Arnaud Mouiche <arnaud.mouiche@gmail.com> wrote:

> On 17/03/2017 15:20, Boris Brezillon wrote:
> > [...]
> > Should be done before calling spinand_detect(), just in case some DT
> > props need to be parsed before the detection step.
> >
> > This being said, I'm not sure we should let the spinand controller
> > driver do the registration step. I'm currently trying to rework the
> > parallel NAND framework to act like all other busses, and I think SPI
> > NAND controllers should also follow this road:
> >
> > 1/ spinand controller driver register a controller to the spinand core
> >    (spinand_controller_register())
> > 2/ the core parses the DT (or board info if DT is not available) and
> >     creates N spinand devices (the N value depends on the number of SPI
> >     nands connected to the controller)
> > 3/ for each spinand device the detection/initialization takes place
> >     directly in the core (the spinand_controller_ops should contain
> >     anything required to do the detection)
> > 4/ for each spinand dev the spinand_controller_ops->add() is called, to
> >     let the controller driver attach private data to the device (if
> >     required), and/or let it use it's own ECC engine (optional as well).
> > 5/ underlying MTD device registered by spinand core code and spinand
> >     dev added to the list of devices controlled by this controller (done
> >     in the core)
> >
> > When removing a device, you just have the reverse steps:
> >
> > 1/ remove from list and unregister the MTD dev
> > 2/ call spinand_controller_ops->remove()
> > 3/ core/manufacturer cleanup
> >
> > Not sure how feasible this is, especially for the generic SPI NAND
> > controller case where the SPI NAND controller does not have a node in
> > the DT, but that would avoid all this boiler-plate duplication we have
> > in the // NAND framework.  
> 
> Since the probe for generic spi devices is generally triggered by the 
> SPI layer, it will not match easily in the way you would like the 
> registration done.

That's true, but I think we can find something to handle this case.

> Can we let this registration question not be a showstopper for Peter's 
> effort ?

Sure, I was just thinking out loud.

Re: [PATCH v3 5/8] nand: spi: Add bad block support

[Peter Pan]

Hi Arnaud and Boris,

On Fri, Mar 17, 2017 at 8:31 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Fri, 17 Mar 2017 13:22:17 +0100
> Arnaud Mouiche <arnaud.mouiche@gmail.com> wrote:
>
>> On 16/03/2017 07:47, Peter Pan wrote:
>> > [...]
>> > +
>> > +/*
>> > + * spinand_scan_bbt - scan BBT in SPI NAND device
>> > + * @chip: SPI NAND device structure
>> > + */
>> > +static int spinand_scan_bbt(struct spinand_device *chip)
>> > +{
>> > +   struct nand_device *nand = &chip->base;
>> > +   int ret;
>> > +
>> > +   nand->bbt.options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
>> > +   nand->bbt.td = NULL;
>> > +   nand->bbt.md = NULL;
>> > +
>> > +   ret = spinand_create_badblock_pattern(chip);
>> > +   if (ret)
>> > +           return ret;
>> > +
>> > +   return nand_scan_bbt(nand);
>> > +}
>> > +
>>
>> Boris, Peter,
>>
>> I'm not a big fan of NAND_BBT_USE_FLASH for small capacity nand flash
>> (eg. 1Gb with 1024 blocks, where a complete bad block scan on boot is
>> fast enough).
>> Do you consider NAND_BBT_USE_FLASH as mandatory, or does a optional
>> "of_get_nand_on_flash_bbt(dn))" device tree configuration is something
>> possible ?
>
> It should be optional indeed.

I agree with you. Let's make it optional.

Thanks,
Peter Pan

Re: [PATCH v3 1/8] mtd: nand: add more helpers in nand.h

[Peter Pan]

Hi Boris,

On Fri, Mar 17, 2017 at 9:07 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Thu, 16 Mar 2017 14:47:30 +0800
> Peter Pan <peterpandong@micron.com> wrote:
>
>> This commit adds some helpers in nand.h
>>     nand_size()
>>     valid_nand_address()
>>     valid_nand_oob_ops()
>>     nand_oob_ops_across_page()
>>     valid_nand_erase_ops()
>>
>> Signed-off-by: Peter Pan <peterpandong@micron.com>
>> ---
>>  include/linux/mtd/nand.h | 59 ++++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 59 insertions(+)
>>
>> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
>> index c2197b4..4a812e6 100644
>> --- a/include/linux/mtd/nand.h
>> +++ b/include/linux/mtd/nand.h
>> @@ -410,6 +410,65 @@ static inline int nand_neraseblocks(struct nand_device *nand)
>>  }
>>
>>  /**
>> + * nand_size - Get NAND size
>> + * @nand: NAND device
>> + *
>> + * Returns the total size exposed by @nand.
>> + */
>> +static inline u64 nand_size(struct nand_device *nand)
>> +{
>> +     return nand->memorg.ndies * nand->memorg.diesize;
>> +}
>> +
>> +static inline bool valid_nand_address(struct nand_device *nand, loff_t addr)
>
> Please keep consistent prefixes. Maybe we should name it
> nand_check_address(), return -EINVAL if it's wrong and 0 otherwise:
>
> static inline int nand_check_address(struct nand_device *nand,
>                                      loff_t addr)
> {
>         return addr < nand_size(nand) ? 0 : -EINVAL;
> }

Fix this in v4

>
>> +{
>> +     return addr < nand_size(nand);
>> +}
>> +
>> +static inline bool valid_nand_oob_ops(struct nand_device *nand, loff_t start,
>> +                                   struct mtd_oob_ops *ops)
>> +{
>> +     struct mtd_info *mtd = nand_to_mtd(nand);
>> +     int ooblen = ops->mode == MTD_OPS_AUTO_OOB ?
>> +             mtd->oobavail : mtd->oobsize;
>> +
>> +     if (ops->ooboffs >= ooblen)
>> +             return false;
>> +     if (ops->ooboffs + ops->ooblen >
>> +         (nand_len_to_pages(nand, nand_size(nand)) -
>> +                            nand_offs_to_page(nand, start)) * ooblen)
>> +             return false;
>> +
>> +     return true;
>> +}
>
> Ditto.

Fix this in v4.
BTW, I considered to put check the mtd ops consistency here. What do you think?

>
>> +
>> +static inline bool nand_oob_ops_across_page(struct nand_device *nand,
>> +                                         struct mtd_oob_ops *ops)
>> +{
>> +     struct mtd_info *mtd = nand_to_mtd(nand);
>> +     int ooblen = ops->mode == MTD_OPS_AUTO_OOB ?
>> +                  mtd->oobavail : mtd->oobsize;
>> +
>> +     return (ops->ooboffs + ops->ooblen) > ooblen;
>> +}
>> +
>> +static inline bool valid_nand_erase_ops(struct nand_device *nand,
>> +                                     struct erase_info *einfo)
>
> Ditto.

Fix this in v4.

Thanks
Peter Pan

Re: [PATCH v3 2/8] mtd: nand: add oob iterator in nand_for_each_page

[Peter Pan]

Hi Boris,

On Fri, Mar 17, 2017 at 9:11 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Thu, 16 Mar 2017 14:47:31 +0800
> Peter Pan <peterpandong@micron.com> wrote:
>
>> Iterate nand pages by both page and oob operation.
>>
>> Signed-off-by: Peter Pan <peterpandong@micron.com>
>> ---
>>  include/linux/mtd/nand.h | 28 +++++++++++++++++++++++++---
>>  1 file changed, 25 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
>> index 4a812e6..822547e 100644
>> --- a/include/linux/mtd/nand.h
>> +++ b/include/linux/mtd/nand.h
>> @@ -87,6 +87,9 @@ struct nand_page_iter {
>>       loff_t offs;
>>       int page;
>>       int pageoffs;
>> +     int ooboffs;
>> +     int oobsize;
>> +     size_t oobleft;
>
> Maybe we should also add dataleft so that you don't have to pass start
> and len when you call nand_page_iter_end().

Yes, it's better. Fix this in v4

Thanks,
Peter Pan

Re: [PATCH v3 3/8] nand: spi: add basic blocks for infrastructure

[Peter Pan]

Hi Boris,

On Fri, Mar 17, 2017 at 9:38 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Thu, 16 Mar 2017 14:47:32 +0800
> Peter Pan <peterpandong@micron.com> wrote:
>
>
>> +
>> +/*
>> + * spinand_manufacturer_detect - detect SPI NAND device by each manufacturer
>> + * @chip: SPI NAND device structure
>> + * @raw_id: raw id buffer. raw id is read by spinand_read_id(), should be
>> + *          decoded by manufacturers
>> + * @id: real id buffer. manufacturer's ->detect() should put real id in
>> + *      this buffer.
>> + *
>> + * ->detect() should decode raw id and initialize device related part in
>> + * spinand_device structure if it is the right device.
>> + * ->detect() can not be NULL.
>> + */
>> +static int spinand_manufacturer_detect(struct spinand_device *chip,
>> +                                    u8 *raw_id, u8 *id)
>> +{
>> +     int i = 0;
>> +
>> +     for (; spinand_manufacturers[i]->id != 0x0; i++) {
>> +             if (spinand_manufacturers[i]->ops &&
>> +                 spinand_manufacturers[i]->ops->detect) {
>
> AFAIU, ->detect() is mandatory, otherwise you have no way to attach a
> NAND to this manufacturer which makes it useless.
> Am I missing something?
>
> If I'm right, you should assume that ->detect() is never NULL and
> complain loudly if it is.

You are totally right. Actually I said ->detect() cannot be NULL in
function header....
I will fix this in v4.

>
>> +                     if (spinand_manufacturers[i]->ops->detect(chip,
>> +                                                               raw_id, id))
>> +                             return 0;
>> +             }
>> +     }
>> +
>> +     return -ENODEV;
>> +}
>> +
>> +/*
>> + * spinand_manufacturer_init - manufacturer initialization function.
>> + * @chip: SPI NAND device structure
>> + *
>> + * Manufacturer drivers should put all their specific initialization code in
>> + * their ->init() hook.
>> + */
>> +static int spinand_manufacturer_init(struct spinand_device *chip)
>> +{
>> +     if (chip->manufacturer.manu->ops && chip->manufacturer.manu->ops->init)
>> +             return chip->manufacturer.manu->ops->init(chip);
>> +
>> +     return 0;
>> +}
>> +
>> +/*
>> + * spinand_manufacturer_cleanup - manufacturer cleanup function.
>> + * @chip: SPI NAND device structure
>> + *
>> + * Manufacturer drivers should put all their specific cleanup code in their
>> + * ->cleanup() hook.
>> + */
>> +static void spinand_manufacturer_cleanup(struct spinand_device *chip)
>> +{
>> +     /* Release manufacturer private data */
>> +     if (chip->manufacturer.manu->ops &&
>> +         chip->manufacturer.manu->ops->cleanup)
>> +             return chip->manufacturer.manu->ops->cleanup(chip);
>> +}
>> +
>> +/*
>> + * spinand_fill_id - fill id in spinand_device structure.
>> + * @chip: SPI NAND device structure
>> + * @id: id buffer
>> + */
>> +static void spinand_fill_id(struct spinand_device *chip, u8 *id)
>> +{
>> +     memcpy(chip->id.data, id, SPINAND_MAX_ID_LEN);
>> +     chip->id.len = SPINAND_MAX_ID_LEN;
>> +}
>> +
>> +/*
>> + * spinand_get_mfr_id - get device's manufacturer ID.
>> + * @chip: SPI NAND device structure
>> + */
>> +static u8 spinand_get_mfr_id(struct spinand_device *chip)
>> +{
>> +     return chip->id.data[SPINAND_MFR_ID];
>> +}
>> +
>> +/*
>> + * spinand_get_dev_id - get device's device ID.
>> + * @chip: SPI NAND device structure
>> + */
>> +static u8 spinand_get_dev_id(struct spinand_device *chip)
>> +{
>> +     return chip->id.data[SPINAND_DEV_ID];
>> +}
>
> As said below, I'm not sure we need to identify the manufacturer and
> device ID bytes if we go for the raw-id approach.
>
>> +
>> +/*
>> + * spinand_set_manufacturer_ops - set manufacture ops in spinand_device
>> + * structure.
>> + * @chip: SPI NAND device structure
>> + * @mfr_id: device's manufacturer ID
>> + */
>> +static void spinand_set_manufacturer_ops(struct spinand_device *chip,
>> +                                      u8 mfr_id)
>> +{
>> +     int i = 0;
>> +
>> +     for (; spinand_manufacturers[i]->id != 0x0; i++) {
>> +             if (spinand_manufacturers[i]->id == mfr_id)
>> +                     break;
>> +     }
>> +
>> +     chip->manufacturer.manu = spinand_manufacturers[i];
>
> Clearly something that should be done in spinand_manufacturer_detect()
> when the ->detect() method returns true.

Yes. Fix this in v4.

>
>> +}
>> +
>> +/*
>> + * spinand_detect - [SPI NAND Interface] detect the SPI NAND device
>> + * @chip: SPI NAND device structure
>> + */
>> +int spinand_detect(struct spinand_device *chip)
>> +{
>> +     struct nand_device *nand = &chip->base;
>> +     u8 raw_id[SPINAND_MAX_ID_LEN] = {0};
>> +     u8 id[SPINAND_MAX_ID_LEN] = {0};
>> +     int ret;
>> +
>> +     spinand_reset(chip);
>> +     spinand_read_id(chip, raw_id);
>
> Directly store the raw id in chip->id.data.
>
>> +     ret = spinand_manufacturer_detect(chip, raw_id, id);
>
> Why do you need to differentiate raw and non-raw IDs. If we consider the
> ID to be the 4 first bytes without any dummy-cycles, then we'd better
> store this one in chip->id.data and let manufacturer code parse it.

Fix this in v4

>
>> +     if (ret)
>> +             return ret;
>> +
>> +     spinand_fill_id(chip, id);
>> +
>> +     pr_info("SPI NAND: %s is found.\n", chip->name);
>> +     pr_info("Manufacturer ID: 0x%02x, Chip ID: 0x%02x\n",
>> +             spinand_get_mfr_id(chip), spinand_get_dev_id(chip));
>
> Is this really important to print those raw IDs? How about printing the
> manufacturer name (which should be part of struct spinand_manufacturer)
> and the model name (a readable string that can be extracted from the
> device id during detect).

Fix this in v4

Thanks,
Peter Pan

Re: [PATCH v3 7/8] nand: spi: Add generic SPI controller support

[Peter Pan]

Hi Boris and Arnaud,

On Sat, Mar 18, 2017 at 1:48 AM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Fri, 17 Mar 2017 18:32:28 +0100
> Arnaud Mouiche <arnaud.mouiche@gmail.com> wrote:
>
>> On 17/03/2017 15:20, Boris Brezillon wrote:
>> > [...]
>> > Should be done before calling spinand_detect(), just in case some DT
>> > props need to be parsed before the detection step.
>> >
>> > This being said, I'm not sure we should let the spinand controller
>> > driver do the registration step. I'm currently trying to rework the
>> > parallel NAND framework to act like all other busses, and I think SPI
>> > NAND controllers should also follow this road:
>> >
>> > 1/ spinand controller driver register a controller to the spinand core
>> >    (spinand_controller_register())
>> > 2/ the core parses the DT (or board info if DT is not available) and
>> >     creates N spinand devices (the N value depends on the number of SPI
>> >     nands connected to the controller)
>> > 3/ for each spinand device the detection/initialization takes place
>> >     directly in the core (the spinand_controller_ops should contain
>> >     anything required to do the detection)
>> > 4/ for each spinand dev the spinand_controller_ops->add() is called, to
>> >     let the controller driver attach private data to the device (if
>> >     required), and/or let it use it's own ECC engine (optional as well).
>> > 5/ underlying MTD device registered by spinand core code and spinand
>> >     dev added to the list of devices controlled by this controller (done
>> >     in the core)
>> >
>> > When removing a device, you just have the reverse steps:
>> >
>> > 1/ remove from list and unregister the MTD dev
>> > 2/ call spinand_controller_ops->remove()
>> > 3/ core/manufacturer cleanup
>> >
>> > Not sure how feasible this is, especially for the generic SPI NAND
>> > controller case where the SPI NAND controller does not have a node in
>> > the DT, but that would avoid all this boiler-plate duplication we have
>> > in the // NAND framework.
>>
>> Since the probe for generic spi devices is generally triggered by the
>> SPI layer, it will not match easily in the way you would like the
>> registration done.
>
> That's true, but I think we can find something to handle this case.
>
>> Can we let this registration question not be a showstopper for Peter's
>> effort ?
>
> Sure, I was just thinking out loud.

Excellent idea, It's a quite big change, Let me try to how far I can go in v4.

Thanks,
Peter Pan

Re: [PATCH v3 7/8] nand: spi: Add generic SPI controller support

[Boris Brezillon]

+Cyrille and Marek.

On Mon, 20 Mar 2017 12:58:26 +0800
Peter Pan <peterpansjtu@gmail.com> wrote:

> Hi Boris and Arnaud,
> 
> On Sat, Mar 18, 2017 at 1:48 AM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > On Fri, 17 Mar 2017 18:32:28 +0100
> > Arnaud Mouiche <arnaud.mouiche@gmail.com> wrote:
> >  
> >> On 17/03/2017 15:20, Boris Brezillon wrote:  
> >> > [...]
> >> > Should be done before calling spinand_detect(), just in case some DT
> >> > props need to be parsed before the detection step.
> >> >
> >> > This being said, I'm not sure we should let the spinand controller
> >> > driver do the registration step. I'm currently trying to rework the
> >> > parallel NAND framework to act like all other busses, and I think SPI
> >> > NAND controllers should also follow this road:
> >> >
> >> > 1/ spinand controller driver register a controller to the spinand core
> >> >    (spinand_controller_register())
> >> > 2/ the core parses the DT (or board info if DT is not available) and
> >> >     creates N spinand devices (the N value depends on the number of SPI
> >> >     nands connected to the controller)
> >> > 3/ for each spinand device the detection/initialization takes place
> >> >     directly in the core (the spinand_controller_ops should contain
> >> >     anything required to do the detection)
> >> > 4/ for each spinand dev the spinand_controller_ops->add() is called, to
> >> >     let the controller driver attach private data to the device (if
> >> >     required), and/or let it use it's own ECC engine (optional as well).
> >> > 5/ underlying MTD device registered by spinand core code and spinand
> >> >     dev added to the list of devices controlled by this controller (done
> >> >     in the core)
> >> >
> >> > When removing a device, you just have the reverse steps:
> >> >
> >> > 1/ remove from list and unregister the MTD dev
> >> > 2/ call spinand_controller_ops->remove()
> >> > 3/ core/manufacturer cleanup
> >> >
> >> > Not sure how feasible this is, especially for the generic SPI NAND
> >> > controller case where the SPI NAND controller does not have a node in
> >> > the DT, but that would avoid all this boiler-plate duplication we have
> >> > in the // NAND framework.  
> >>
> >> Since the probe for generic spi devices is generally triggered by the
> >> SPI layer, it will not match easily in the way you would like the
> >> registration done.  
> >
> > That's true, but I think we can find something to handle this case.
> >  
> >> Can we let this registration question not be a showstopper for Peter's
> >> effort ?  
> >
> > Sure, I was just thinking out loud.  
> 
> Excellent idea, It's a quite big change, Let me try to how far I can go in v4.

Let's keep that for later. Also, can you wait a bit before sending a
v4, I know Cyrille wanted to comment on the SPI/dual-SPI/quad-SPI
handling. Actually, I'd like to have feedback from both SPI NOR
maintainers (Cyrille and Marek), can you Cc them in your next version?

Re: [PATCH v3 7/8] nand: spi: Add generic SPI controller support

[Peter Pan]

On Mon, Mar 20, 2017 at 1:56 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> +Cyrille and Marek.
>
> On Mon, 20 Mar 2017 12:58:26 +0800
> Peter Pan <peterpansjtu@gmail.com> wrote:
>
>> Hi Boris and Arnaud,
>>
>> On Sat, Mar 18, 2017 at 1:48 AM, Boris Brezillon
>> <boris.brezillon@free-electrons.com> wrote:
>> > On Fri, 17 Mar 2017 18:32:28 +0100
>> > Arnaud Mouiche <arnaud.mouiche@gmail.com> wrote:
>> >
>> >> On 17/03/2017 15:20, Boris Brezillon wrote:
>> >> > [...]
>> >> > Should be done before calling spinand_detect(), just in case some DT
>> >> > props need to be parsed before the detection step.
>> >> >
>> >> > This being said, I'm not sure we should let the spinand controller
>> >> > driver do the registration step. I'm currently trying to rework the
>> >> > parallel NAND framework to act like all other busses, and I think SPI
>> >> > NAND controllers should also follow this road:
>> >> >
>> >> > 1/ spinand controller driver register a controller to the spinand core
>> >> >    (spinand_controller_register())
>> >> > 2/ the core parses the DT (or board info if DT is not available) and
>> >> >     creates N spinand devices (the N value depends on the number of SPI
>> >> >     nands connected to the controller)
>> >> > 3/ for each spinand device the detection/initialization takes place
>> >> >     directly in the core (the spinand_controller_ops should contain
>> >> >     anything required to do the detection)
>> >> > 4/ for each spinand dev the spinand_controller_ops->add() is called, to
>> >> >     let the controller driver attach private data to the device (if
>> >> >     required), and/or let it use it's own ECC engine (optional as well).
>> >> > 5/ underlying MTD device registered by spinand core code and spinand
>> >> >     dev added to the list of devices controlled by this controller (done
>> >> >     in the core)
>> >> >
>> >> > When removing a device, you just have the reverse steps:
>> >> >
>> >> > 1/ remove from list and unregister the MTD dev
>> >> > 2/ call spinand_controller_ops->remove()
>> >> > 3/ core/manufacturer cleanup
>> >> >
>> >> > Not sure how feasible this is, especially for the generic SPI NAND
>> >> > controller case where the SPI NAND controller does not have a node in
>> >> > the DT, but that would avoid all this boiler-plate duplication we have
>> >> > in the // NAND framework.
>> >>
>> >> Since the probe for generic spi devices is generally triggered by the
>> >> SPI layer, it will not match easily in the way you would like the
>> >> registration done.
>> >
>> > That's true, but I think we can find something to handle this case.
>> >
>> >> Can we let this registration question not be a showstopper for Peter's
>> >> effort ?
>> >
>> > Sure, I was just thinking out loud.
>>
>> Excellent idea, It's a quite big change, Let me try to how far I can go in v4.
>
> Let's keep that for later. Also, can you wait a bit before sending a
> v4, I know Cyrille wanted to comment on the SPI/dual-SPI/quad-SPI
> handling. Actually, I'd like to have feedback from both SPI NOR
> maintainers (Cyrille and Marek), can you Cc them in your next version?

Yes, Let's wait for their feed back.

Peter Pan