[PATCH v2 0/6] Introduction to SPI NAND framework

[PATCH v2 0/6] Introduction to SPI NAND framework

[Peter Pan]

First of all, thanks for Boris's valuable comments on v1.

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

Changes since v1:
- 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 (6):
  nand: spi: Add init/release function
  nand: spi: add basic operations support
  nand: spi: Add bad block support
  nand: spi: Add BBT support
  nand: spi: add Micron spi nand support
  nand: spi: Add generic SPI controller support

 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/chips/Kconfig       |    5 +
 drivers/mtd/nand/spi/chips/Makefile      |    1 +
 drivers/mtd/nand/spi/chips/generic_spi.c |  158 +++
 drivers/mtd/nand/spi/spinand_base.c      | 1630 ++++++++++++++++++++++++++++++
 drivers/mtd/nand/spi/spinand_ids.c       |   34 +
 drivers/mtd/nand/spi/spinand_micron.c    |  133 +++
 include/linux/mtd/spinand.h              |  317 ++++++
 11 files changed, 2291 insertions(+)
 create mode 100644 drivers/mtd/nand/spi/Kconfig
 create mode 100644 drivers/mtd/nand/spi/Makefile
 create mode 100644 drivers/mtd/nand/spi/chips/Kconfig
 create mode 100644 drivers/mtd/nand/spi/chips/Makefile
 create mode 100644 drivers/mtd/nand/spi/chips/generic_spi.c
 create mode 100644 drivers/mtd/nand/spi/spinand_base.c
 create mode 100644 drivers/mtd/nand/spi/spinand_ids.c
 create mode 100644 drivers/mtd/nand/spi/spinand_micron.c
 create mode 100644 include/linux/mtd/spinand.h

-- 
1.9.1

[PATCH v2 1/6] nand: spi: Add init/release function

[Peter Pan]

This is the first commit for spi nand framkework.
This commit is to add spi nand initialization and
release functions.

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/spinand_base.c | 469 ++++++++++++++++++++++++++++++++++++
 drivers/mtd/nand/spi/spinand_ids.c  |  29 +++
 include/linux/mtd/spinand.h         | 315 ++++++++++++++++++++++++
 7 files changed, 822 insertions(+)
 create mode 100644 drivers/mtd/nand/spi/Kconfig
 create mode 100644 drivers/mtd/nand/spi/Makefile
 create mode 100644 drivers/mtd/nand/spi/spinand_base.c
 create mode 100644 drivers/mtd/nand/spi/spinand_ids.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..04a7b71
--- /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..6f0d622
--- /dev/null
+++ b/drivers/mtd/nand/spi/Makefile
@@ -0,0 +1,2 @@
+obj-$(CONFIG_MTD_SPI_NAND) += spinand_base.o
+obj-$(CONFIG_MTD_SPI_NAND) += spinand_ids.o
diff --git a/drivers/mtd/nand/spi/spinand_base.c b/drivers/mtd/nand/spi/spinand_base.c
new file mode 100644
index 0000000..97d47146
--- /dev/null
+++ b/drivers/mtd/nand/spi/spinand_base.c
@@ -0,0 +1,469 @@
+/**
+* spi-nand-base.c
+*
+* 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/sched.h>
+#include <linux/delay.h>
+#include <linux/jiffies.h>
+#include <linux/mtd/spinand.h>
+#include <linux/slab.h>
+
+
+static u64 spinand_get_chip_size(struct spinand_device *chip)
+{
+	struct nand_device *nand = &chip->base;
+
+	return nand_diesize(nand) * nand_ndies(nand);
+}
+
+static inline int spinand_exec_cmd(struct spinand_device *chip,
+				struct spinand_op *cmd)
+{
+	return chip->controller.ops->exec_op(chip, cmd);
+}
+
+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,
+			uint8_t reg, uint8_t *buf)
+{
+	struct spinand_op cmd;
+	int ret;
+
+	spinand_op_init(&cmd);
+	cmd.cmd = SPINAND_CMD_GET_FEATURE;
+	cmd.n_addr = 1;
+	cmd.addr[0] = reg;
+	cmd.n_rx = 1;
+	cmd.rx_buf = buf;
+
+	ret = spinand_exec_cmd(chip, &cmd);
+	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,
+			uint8_t reg, uint8_t *buf)
+{
+	struct spinand_op cmd;
+	int ret;
+
+	spinand_op_init(&cmd);
+	cmd.cmd = SPINAND_CMD_SET_FEATURE;
+	cmd.n_addr = 1;
+	cmd.addr[0] = reg;
+	cmd.n_tx = 1,
+	cmd.tx_buf = buf,
+
+	ret = spinand_exec_cmd(chip, &cmd);
+	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.
+ *   Once the status turns to be ready, the other status bits also are
+ *   valid status bits.
+ */
+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(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 out 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
+ */
+static int spinand_read_id(struct spinand_device *chip, u8 *buf)
+{
+	struct spinand_op cmd;
+
+	spinand_op_init(&cmd);
+	cmd.cmd = SPINAND_CMD_READ_ID;
+	if (chip->manufacturer.ops->get_dummy)
+		cmd.dummy_bytes = chip->manufacturer.ops->get_dummy(chip, &cmd);
+	cmd.n_rx = 2;
+	cmd.rx_buf = buf;
+
+	return spinand_exec_cmd(chip, &cmd);
+}
+
+/**
+ * spinand_reset - send command FFh to reset chip.
+ * @chip: SPI-NAND device structure
+ */
+static int spinand_reset(struct spinand_device *chip)
+{
+	struct spinand_op cmd;
+	int ret;
+
+	spinand_op_init(&cmd);
+	cmd.cmd = SPINAND_CMD_RESET;
+
+	ret = spinand_exec_cmd(chip, &cmd);
+	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
+ * @spi: spi device structure
+ * @lock: value to set to block lock register
+ * Description:
+ *   After power up, all the Nand blocks are locked.  This function allows
+ *   one to unlock the blocks, and so it can be written or erased.
+ */
+static int spinand_lock_block(struct spinand_device *chip, u8 lock)
+{
+	return spinand_write_reg(chip, REG_BLOCK_LOCK, &lock);
+}
+
+/**
+ * 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 spinand_scan_id_table(struct spinand_device *chip, u8 *id)
+{
+	struct nand_device *nand = &chip->base;
+	struct spinand_flash *type = spinand_table;
+	struct nand_memory_organization *memorg = &nand->memorg;
+	struct spinand_ecc_engine *ecc_engine = &chip->ecc_engine;
+
+	for (; type->name; type++) {
+		if (id[0] == type->mfr_id && id[1] == type->dev_id) {
+			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;
+}
+
+/**
+ * spinand_set_rd_wr_op - Chose the best read write command
+ * @chip: SPI-NAND device structure
+ * Description:
+ *   Chose the fastest r/w command according to spi controller's ability.
+ * Note:
+ *   If 03h/0Bh follows SPI NAND protocol, there is no difference,
+ *   while if follows SPI NOR protocol, 03h command is working under
+ *   <=20Mhz@3.3V,<=5MHz@1.8V; 0Bh command is working under
+ *   133Mhz@3.3v, 83Mhz@1.8V.
+ */
+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;
+}
+
+/*
+ * Manufacturer detection. Only used when the NAND is not ONFI or JEDEC
+ * compliant and does not have a full-id or legacy-id entry in the nand_ids
+ * table.
+ */
+static bool spinand_manufacturer_detect(struct spinand_device *chip)
+{
+	if (chip->manufacturer.ops && chip->manufacturer.ops->detect)
+		return chip->manufacturer.ops->detect(chip);
+	return false;
+}
+
+/*
+ * Manufacturer initialization. This function is called for all NANDs including
+ * ONFI and JEDEC compliant ones.
+ * 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.ops && chip->manufacturer.ops->init)
+		return chip->manufacturer.ops->init(chip);
+
+	return 0;
+}
+
+/*
+ * Manufacturer cleanup. This function is called for all NANDs including
+ * ONFI and JEDEC compliant ones.
+ * 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.ops && chip->manufacturer.ops->cleanup)
+		return chip->manufacturer.ops->cleanup(chip);
+}
+
+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;
+}
+
+static u8 spinand_get_mfr_id(struct spinand_device *chip)
+{
+	return chip->id.data[SPINAND_MFR_ID];
+}
+
+static u8 spinand_get_dev_id(struct spinand_device *chip)
+{
+	return chip->id.data[SPINAND_DEV_ID];
+}
+
+static void spinand_set_manufacturer_ops(struct spinand_device *chip, u8 mfr_id)
+{
+	int i = 0;
+
+	for (; spinand_manuf_ids[i].id != 0x0; i++) {
+		if (spinand_manuf_ids[i].id == mfr_id)
+			break;
+	}
+	chip->manufacturer.ops = spinand_manuf_ids[i].ops;
+}
+
+/**
+ * spinand_scan_ident - [SPI-NAND Interface] Scan for the SPI-NAND device
+ * @chip: SPI-NAND device structure
+ * Description:
+ *   This is the first phase of the initiazation. It reads the flash ID and
+ *   sets up spinand_device fields accordingly.
+ */
+int spinand_scan_ident(struct spinand_device *chip, u8 expect_mfr_id)
+{
+	struct nand_device *nand = &chip->base;
+	u8 id[SPINAND_MAX_ID_LEN] = {0};
+	int id_retry = 2;
+
+	spinand_set_manufacturer_ops(chip, expect_mfr_id);
+	spinand_reset(chip);
+read_id:
+	spinand_read_id(chip, id);
+	if (spinand_scan_id_table(chip, id))
+		goto ident_done;
+	if (id_retry--)
+		goto read_id;
+	pr_info("SPI-NAND type mfr_id: %x, dev_id: %x is not in id table.\n",
+				id[SPINAND_MFR_ID], id[SPINAND_DEV_ID]);
+	if (spinand_manufacturer_detect(chip))
+		goto ident_done;
+
+	return -ENODEV;
+
+ident_done:
+	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)(spinand_get_chip_size(chip) >> 20), nand_eraseblock_size(nand) >> 10,
+		nand_page_size(nand), nand_per_page_oobsize(nand));
+	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);
+	spinand_lock_block(chip, BL_ALL_UNLOCKED);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(spinand_scan_ident);
+
+/**
+ * spinand_scan_tail - [SPI-NAND Interface] Scan for the SPI-NAND device
+ * @chip: SPI-NAND device structure
+ * Description:
+ *   This is the second phase of the initiazation. It fills out all the
+ *   uninitialized fields of spinand_device and mtd fields.
+ */
+int spinand_scan_tail(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);
+
+	mtd->name = chip->name;
+	mtd->size = spinand_get_chip_size(chip);
+	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);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(spinand_scan_tail);
+
+/**
+ * spinand_scan_ident_release - [SPI-NAND Interface] Free resources
+ * applied by spinand_scan_ident
+ * @chip: SPI-NAND device structure
+ */
+int spinand_scan_ident_release(struct spinand_device *chip)
+{
+	spinand_manufacturer_cleanup(chip);
+	kfree(chip->buf);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(spinand_scan_ident_release);
+
+/**
+ * spinand_scan_tail_release - [SPI-NAND Interface] Free resources
+ * applied by spinand_scan_tail
+ * @chip: SPI-NAND device structure
+ */
+int spinand_scan_tail_release(struct spinand_device *chip)
+{
+	return 0;
+}
+EXPORT_SYMBOL_GPL(spinand_scan_tail_release);
+
+/**
+ * spinand_release - [SPI-NAND Interface] Free resources held by the SPI-NAND
+ * device
+ * @chip: SPI-NAND device structure
+ */
+int spinand_release(struct spinand_device *chip)
+{
+	struct mtd_info *mtd = spinand_to_mtd(chip);
+
+	mtd_device_unregister(mtd);
+	spinand_manufacturer_cleanup(chip);
+	kfree(chip->buf);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(spinand_release);
+
+MODULE_DESCRIPTION("SPI NAND framework");
+MODULE_AUTHOR("Peter Pan<peterpandong@micron.com>");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/mtd/nand/spi/spinand_ids.c b/drivers/mtd/nand/spi/spinand_ids.c
new file mode 100644
index 0000000..7706ae3
--- /dev/null
+++ b/drivers/mtd/nand/spi/spinand_ids.c
@@ -0,0 +1,29 @@
+/**
+* spi-nand-base.c
+*
+* 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_flash spinand_table[] = {
+	{.name = NULL},
+};
+
+
+struct spinand_manufacturer spinand_manuf_ids[] = {
+	{0x0, "Unknown"}
+};
+
+EXPORT_SYMBOL(spinand_manuf_ids);
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
new file mode 100644
index 0000000..f3d0351
--- /dev/null
+++ b/include/linux/mtd/spinand.h
@@ -0,0 +1,315 @@
+/**
+* spinand.h
+*
+* 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		2
+/**
+ * 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 {
+	#define SPINAND_MFR_ID		0
+	#define SPINAND_DEV_ID		1
+	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);
+	int (*init)(struct spinand_device *chip);
+	void (*cleanup)(struct spinand_device *chip);
+	int (*get_dummy)(struct spinand_device *chip, struct spinand_op *op);
+};
+
+struct spinand_manufacturer {
+	u8 id;
+	char *name;
+	struct spinand_manufacturer_ops *ops;
+};
+
+extern struct spinand_manufacturer spinand_manuf_ids[];
+
+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);
+};
+
+typedef enum {
+	SPINAND_ECC_ONDIE,
+	SPINAND_ECC_HW,
+} spinand_ecc_modes_t;
+
+
+struct spinand_ecc_engine {
+	spinand_ecc_modes_t mode;
+	u32 strength;
+	u32 steps;
+	struct spinand_ecc_engine_ops *ops;
+};
+
+/**
+ * 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 {
+		struct spinand_controller_ops *ops;
+#define SPINAND_CAP_RD_X1 (1 << 0)
+#define SPINAND_CAP_RD_X2 (1 << 1)
+#define SPINAND_CAP_RD_X4 (1 << 2)
+#define SPINAND_CAP_RD_DUAL (1 << 3)
+#define SPINAND_CAP_RD_QUAD (1 << 4)
+#define SPINAND_CAP_WR_X1 (1 << 5)
+#define SPINAND_CAP_WR_X2 (1 << 6)
+#define SPINAND_CAP_WR_X4 (1 << 7)
+#define SPINAND_CAP_WR_DUAL (1 << 8)
+#define SPINAND_CAP_WR_QUAD (1 << 9)
+#define SPINAND_CAP_HW_ECC (1 << 10)
+		u32 caps;
+		void *priv;
+	} controller;
+	struct {
+		struct spinand_manufacturer_ops *ops;
+		void *priv;
+	} manufacturer;
+	struct spinand_ecc_engine ecc_engine;
+};
+
+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;
+}
+
+
+struct spinand_flash {
+	char *name;
+	u8 mfr_id;
+	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 options;
+	u32 rw_mode;
+};
+
+extern struct spinand_flash spinand_table[];
+
+#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
+
+#define SPI_NAND_INFO(nm, mid, did, pagesz, oobsz, pg_per_blk,\
+	blk_per_lun, lun_per_chip, ecc_stren, rwmode)	\
+	{ .name = (nm), .mfr_id = (mid), .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) }
+
+/*SPI NAND manufacture ID definition*/
+#define SPINAND_MFR_MICRON		0x2C
+
+int spinand_scan_ident(struct spinand_device *chip, u8 expect_mfr_id);
+int spinand_scan_tail(struct spinand_device *chip);
+int spinand_scan_ident_release(struct spinand_device *chip);
+int spinand_scan_tail_release(struct spinand_device *chip);
+int spinand_release(struct spinand_device *chip);
+int spinand_set_cmd_cfg_table(int mfr);
+struct spinand_op_def *spinand_get_cmd_cfg(u8 opcode);
+#endif /* __LINUX_MTD_SPINAND_H */
-- 
1.9.1

[PATCH v2 2/6] nand: spi: add basic operations support

[Peter Pan]

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

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

diff --git a/drivers/mtd/nand/spi/spinand_base.c b/drivers/mtd/nand/spi/spinand_base.c
index 97d47146..1bc57e9 100644
--- a/drivers/mtd/nand/spi/spinand_base.c
+++ b/drivers/mtd/nand/spi/spinand_base.c
@@ -25,6 +25,31 @@
 #include <linux/slab.h>
 
 
+static int spinand_erase(struct mtd_info *mtd, struct erase_info *einfo);
+
+/**
+ * spinand_get_device - [GENERIC] Get chip for selected access
+ * @mtd: MTD device structure
+ * @new_state: the state which is requested
+ *
+ * Get the device and lock it for exclusive access
+ */
+static void spinand_get_device(struct spinand_device *chip)
+{
+	mutex_lock(&chip->lock);
+}
+
+/**
+ * spinand_release_device - [GENERIC] release chip
+ * @mtd: MTD device structure
+ *
+ * Deselect, release chip lock and wake up anyone waiting on the device
+ */
+static void spinand_release_device(struct spinand_device *chip)
+{
+	mutex_unlock(&chip->lock);
+}
+
 static u64 spinand_get_chip_size(struct spinand_device *chip)
 {
 	struct nand_device *nand = &chip->base;
@@ -115,6 +140,258 @@ 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
+ * Description:
+ *   There is one bit( bit 0x10 ) to set or to clear the internal ECC.
+ *   Enable chip internal ECC, set the bit to 1
+ *   Disable chip internal ECC, clear the bit to 0
+ */
+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
+ * Description:
+ *   There is one bit( bit 0x10 ) to set or to clear the internal ECC.
+ *   Enable chip internal ECC, set the bit to 1
+ *   Disable chip internal ECC, clear the bit to 0
+ */
+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
+ * Description:
+ *   Before write and erase the Nand cells, the write enable has to be set.
+ *   After the write or erase, the write enable bit is automatically
+ *   cleared (status register bit 2)
+ *   Set the bit 2 of the status register has the same effect
+ */
+static int spinand_write_enable(struct spinand_device *chip)
+{
+	struct spinand_op cmd;
+
+	spinand_op_init(&cmd);
+	cmd.cmd = SPINAND_CMD_WR_ENABLE;
+
+	return spinand_exec_cmd(chip, &cmd);
+}
+
+/**
+ * spinand_read_page_to_cache - send command 13h to read data from Nand
+ * 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 cmd;
+
+	spinand_op_init(&cmd);
+	cmd.cmd = SPINAND_CMD_PAGE_READ;
+	cmd.n_addr = 3;
+	cmd.addr[0] = (u8)(page_addr >> 16);
+	cmd.addr[1] = (u8)(page_addr >> 8);
+	cmd.addr[2] = (u8)page_addr;
+
+	return spinand_exec_cmd(chip, &cmd);
+}
+
+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;
+	}
+}
+
+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
+ * Description:
+ *   Command can be 03h, 0Bh, 3Bh, 6Bh, BBh, EBh
+ *   The read can specify 1 to (page size + spare size) bytes of data read at
+ *   the corresponding locations.
+ *   No tRd delay.
+ */
+static int spinand_read_from_cache(struct spinand_device *chip,
+		u32 page_addr, u32 column, size_t len, u8 *rbuf)
+{
+	struct spinand_op cmd;
+
+	spinand_op_init(&cmd);
+	cmd.cmd = chip->read_cache_op;
+	if (chip->manufacturer.ops->build_column_addr) {
+		chip->manufacturer.ops->build_column_addr(chip, &cmd,
+						          page_addr, column);
+	} else {
+		cmd.n_addr = 2;
+		cmd.addr[0] = (u8)(column >> 8);
+		cmd.addr[1] = (u8)column;
+	}
+	if (chip->manufacturer.ops->get_dummy)
+		cmd.dummy_bytes = chip->manufacturer.ops->get_dummy(chip, &cmd);
+	cmd.addr_nbits = spinand_get_address_bits(chip->read_cache_op);
+	cmd.n_rx = len;
+	cmd.rx_buf = rbuf;
+	cmd.data_nbits = spinand_get_data_bits(chip->read_cache_op);
+
+	return spinand_exec_cmd(chip, &cmd);
+}
+
+/**
+ * spinand_program_data_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
+ * @clr_cache: clear cache register or not
+ * Description:
+ *   Command can be 02h, 32h, 84h, 34h
+ *   02h and 32h will clear the cache with 0xff value first
+ *   Since it is writing the data to cache, there is no tPROG time.
+ */
+static int spinand_program_data_to_cache(struct spinand_device *chip,
+			u32 page_addr, u32 column, size_t len, const u8 *wbuf)
+{
+	struct spinand_op cmd;
+
+	spinand_op_init(&cmd);
+	cmd.cmd = chip->write_cache_op;
+	if (chip->manufacturer.ops->build_column_addr) {
+		chip->manufacturer.ops->build_column_addr(chip, &cmd,
+						          page_addr, column);
+	} else {
+		cmd.n_addr = 2;
+		cmd.addr[0] = (u8)(column >> 8);
+		cmd.addr[1] = (u8)column;
+	}
+	cmd.addr_nbits = spinand_get_address_bits(chip->write_cache_op);
+	cmd.n_tx = len;
+	cmd.tx_buf = wbuf;
+	cmd.data_nbits = spinand_get_data_bits(chip->write_cache_op);
+
+	return spinand_exec_cmd(chip, &cmd);
+}
+
+/**
+ * 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.
+ * Description:
+ *   Need to wait for tPROG time to finish the transaction.
+ */
+static int spinand_program_execute(struct spinand_device *chip, u32 page_addr)
+{
+	struct spinand_op cmd;
+
+	spinand_op_init(&cmd);
+	cmd.cmd = SPINAND_CMD_PROG_EXC;
+	cmd.n_addr = 3;
+	cmd.addr[0] = (u8)(page_addr >> 16);
+	cmd.addr[1] = (u8)(page_addr >> 8);
+	cmd.addr[2] = (u8)page_addr;
+
+	return spinand_exec_cmd(chip, &cmd);
+}
+
+
+/**
+ * spinand_erase_block_erase - send command D8h to erase a block
+ * @chip: SPI-NAND device structure
+ * @page_addr: the page to erase.
+ * Description:
+ *   Need to wait for tERS.
+ */
+static int spinand_erase_block(struct spinand_device *chip,
+					u32 page_addr)
+{
+	struct spinand_op cmd;
+
+	spinand_op_init(&cmd);
+	cmd.cmd = SPINAND_CMD_BLK_ERASE;
+	cmd.n_addr = 3;
+	cmd.addr[0] = (u8)(page_addr >> 16);
+	cmd.addr[1] = (u8)(page_addr >> 8);
+	cmd.addr[2] = (u8)page_addr;
+
+	return spinand_exec_cmd(chip, &cmd);
+}
+
+/**
  * spinand_wait - wait until the command is done
  * @chip: SPI-NAND device structure
  * @s: buffer to store status register(can be NULL)
@@ -197,6 +474,611 @@ static int spinand_lock_block(struct spinand_device *chip, u8 lock)
 	return spinand_write_reg(chip, REG_BLOCK_LOCK, &lock);
 }
 
+static void spinand_get_ecc_status(struct spinand_device *chip,
+	unsigned int status, unsigned int *corrected, unsigned int *ecc_errors)
+{
+	return chip->ecc_engine.ops->get_ecc_status(chip, status, corrected, ecc_errors);
+}
+
+/**
+ * spinand_do_read_page - read page from flash to buffer
+ * @mtd: MTD device structure
+ * @page_addr: page address/raw address
+ * @column: column address
+ * @ecc_off: without ecc or not
+ * @corrected: how many bit error corrected
+ * @buf: data buffer
+ * @len: data length to read
+ */
+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 flash
+ * @mtd: MTD device structure
+ * @page_addr: page address/raw address
+ * @column: column address
+ * @buf: data buffer
+ * @len: data length to write
+ * @clr_cache: clear cache register with 0xFF or not
+ */
+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_program_data_to_cache(chip, page_addr, 0,
+		nand_page_size(nand) + nand_per_page_oobsize(nand), chip->buf);
+	else
+		spinand_program_data_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 flash to buffer
+ * @mtd: MTD device structure
+ * @from: offset to read from
+ * @ops: oob operations description structure
+ * @max_bitflips: maximum bitflip count
+ * Description:
+ *   Normal read function, read one page to buffer before issue
+ *   another.
+ */
+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 page_addr, page_offset, size, ret;
+	unsigned int corrected = 0;
+	int readlen = ops->len;
+	int oobreadlen = ops->ooblen;
+	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 == NULL;
+
+	page_addr = nand_offs_to_page(nand, from);
+	page_offset = from & (nand_page_size(nand) - 1);
+	ops->retlen = 0;
+	*max_bitflips = 0;
+
+	while (1) {
+		ret = spinand_do_read_page(mtd, page_addr, ecc_off,
+						&corrected, oob_only);
+		if (ret)
+			break;
+		*max_bitflips = max(*max_bitflips, corrected);
+		if (ops->datbuf) {
+			size = min_t(int, readlen, nand_page_size(nand) - page_offset);
+			memcpy(ops->datbuf + ops->retlen,
+				chip->buf + page_offset, size);
+			ops->retlen += size;
+			readlen -= size;
+			page_offset = 0;
+			if (!readlen)
+				break;
+		}
+		if (ops->oobbuf) {
+			size = min(oobreadlen, 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;
+			oobreadlen -= size;
+			if (!oobreadlen)
+				break;
+		}
+		page_addr++;
+	}
+
+	return ret;
+}
+
+/**
+ * spinand_do_read_ops - read data from flash to buffer
+ * @mtd: MTD device structure
+ * @from: offset to read from
+ * @ops: oob ops structure
+ * Description:
+ *   Disable internal ECC before reading when MTD_OPS_RAW set.
+ */
+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;
+	int oobreadlen = ops->ooblen;
+	bool ecc_off = ops->mode == MTD_OPS_RAW;
+	int ooblen = ops->mode == MTD_OPS_AUTO_OOB ?
+		mtd->oobavail : mtd->oobsize;
+
+	if (unlikely(from >= mtd->size)) {
+		pr_err("%s: attempt to read beyond end of device\n",
+				__func__);
+		return -EINVAL;
+	}
+	if (oobreadlen > 0) {
+		if (unlikely(ops->ooboffs >= ooblen)) {
+			pr_err("%s: attempt to start read outside oob\n",
+					__func__);
+			return -EINVAL;
+		}
+		if (unlikely(ops->ooboffs + oobreadlen >
+		(nand_len_to_pages(nand, mtd->size) - nand_offs_to_page(nand, from))
+		* ooblen)) {
+			pr_err("%s: attempt to read beyond end of device\n",
+					__func__);
+			return -EINVAL;
+		}
+		ooblen -= ops->ooboffs;
+		ops->oobretlen = 0;
+	}
+	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)
+		return ret;
+
+	if (mtd->ecc_stats.failed - stats.failed)
+		return -EBADMSG;
+
+	return max_bitflips;
+}
+
+/**
+ * spinand_write_pages - write data from buffer to flash
+ * @mtd: MTD device structure
+ * @to: offset to write to
+ * @ops: oob operations description structure
+ * @max_bitflips: maximum bitflip count
+ * Description:
+ *   Normal read function, read one page to buffer before issue
+ *   another.
+ */
+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 page_addr, page_offset, ret;
+	int size = 0;
+	int oob_size = 0;
+	int writelen = ops->len;
+	int oobwritelen = ops->ooblen;
+	int ooblen = ops->mode == MTD_OPS_AUTO_OOB ?
+		mtd->oobavail : mtd->oobsize;
+	bool oob_only = ops->datbuf == NULL;
+
+	page_addr = nand_offs_to_page(nand, to);
+	page_offset = to & (nand_page_size(nand) - 1);
+	ops->retlen = 0;
+
+	while (1) {
+		memset(chip->buf, 0xff, nand_page_size(nand) + nand_per_page_oobsize(nand));
+		if (ops->oobbuf) {
+			oob_size = min(oobwritelen, 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, writelen, nand_page_size(nand) - page_offset);
+			memcpy(chip->buf + page_offset,
+				ops->datbuf + ops->retlen, size);
+		}
+		ret = spinand_do_write_page(mtd, page_addr, oob_only);
+		if (ret) {
+			pr_err("error %d writing page 0x%x\n",
+				ret, page_addr);
+			return ret;
+		}
+		if (ops->datbuf) {
+			ops->retlen += size;
+			writelen -= size;
+			page_offset = 0;
+			if (!writelen)
+				break;
+		}
+		if (ops->oobbuf) {
+			ops->oobretlen += oob_size;
+			oobwritelen -= oob_size;
+			if (!oobwritelen)
+				break;
+		}
+		page_addr++;
+	}
+
+	return ret;
+}
+
+/**
+ * spinand_do_write_ops - write data from buffer to flash
+ * @mtd: MTD device structure
+ * @to: offset to write to
+ * @ops: oob operations description structure
+ * Description:
+ *   Disable internal ECC before writing when MTD_OPS_RAW set.
+ */
+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 page_addr, page_offset;
+	int oobwritelen = ops->ooblen;
+	int ret = 0;
+	int ooblen = ops->mode == MTD_OPS_AUTO_OOB ?
+		mtd->oobavail : mtd->oobsize;
+	bool ecc_off = ops->mode == MTD_OPS_RAW;
+
+	/* Do not allow reads past end of device */
+	if (unlikely(to >= mtd->size)) {
+		pr_err("%s: attempt to write beyond end of device\n",
+				__func__);
+		return -EINVAL;
+	}
+
+	page_addr = nand_offs_to_page(nand, to);
+	page_offset = to & (nand_page_size(nand) - 1);
+	ops->retlen = 0;
+
+	/* for oob */
+	if (oobwritelen > 0) {
+		/* Do not allow write past end of page */
+		if ((ops->ooboffs + oobwritelen) > ooblen) {
+			pr_err("%s: attempt to write past end of page\n",
+					__func__);
+			return -EINVAL;
+		}
+
+		if (unlikely(ops->ooboffs >= ooblen)) {
+			pr_err("%s: attempt to start write outside oob\n",
+					__func__);
+			return -EINVAL;
+		}
+		if (unlikely(ops->ooboffs + oobwritelen >
+		(nand_len_to_pages(nand, mtd->size) - nand_offs_to_page(nand, to))
+			* ooblen)) {
+			pr_err("%s: attempt to write beyond end of device\n",
+					__func__);
+			return -EINVAL;
+		}
+		ooblen -= ops->ooboffs;
+		ops->oobretlen = 0;
+	}
+
+	if (ecc_off)
+		spinand_disable_ecc(chip);
+	ret = spinand_write_pages(mtd, to, ops);
+	if (ecc_off)
+		spinand_enable_ecc(chip);
+
+	return ret;
+}
+
+/**
+ * spinand_read - [MTD Interface] SPI-NAND read
+ * @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;
+
+	spinand_get_device(mtd_to_spinand(mtd));
+
+	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;
+
+	spinand_release_device(mtd_to_spinand(mtd));
+
+	return ret;
+}
+
+/**
+ * spinand_write - [MTD Interface] SPI-NAND write
+ * @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;
+
+	spinand_get_device(mtd_to_spinand(mtd));
+
+	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;
+
+	spinand_release_device(mtd_to_spinand(mtd));
+
+	return ret;
+}
+
+/**
+ * spinand_read_oob - [MTD Interface] read 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;
+	spinand_get_device(mtd_to_spinand(mtd));
+
+	switch (ops->mode) {
+	case MTD_OPS_PLACE_OOB:
+	case MTD_OPS_AUTO_OOB:
+	case MTD_OPS_RAW:
+		break;
+
+	default:
+		goto out;
+	}
+
+	ret = spinand_do_read_ops(mtd, from, ops);
+
+out:
+	spinand_release_device(mtd_to_spinand(mtd));
+
+	return ret;
+}
+
+/**
+ * spinand_write_oob - [MTD Interface] write 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;
+	spinand_get_device(mtd_to_spinand(mtd));
+
+	switch (ops->mode) {
+	case MTD_OPS_PLACE_OOB:
+	case MTD_OPS_AUTO_OOB:
+	case MTD_OPS_RAW:
+		break;
+
+	default:
+		goto out;
+	}
+
+	ret = spinand_do_write_ops(mtd, to, ops);
+
+out:
+	spinand_release_device(mtd_to_spinand(mtd));
+
+	return ret;
+}
+
+
+/**
+ * spinand_erase - [MTD Interface] erase block(s)
+ * @mtd: MTD device structure
+ * @einfo: erase instruction
+ *
+ * Erase one ore more blocks
+ */
+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;
+
+	/* check address align on block boundary */
+	if (offs & (nand_eraseblock_size(nand) - 1)) {
+		pr_err("%s: Unaligned address\n", __func__);
+		return -EINVAL;
+	}
+
+	if (len & (nand_eraseblock_size(nand) - 1)) {
+		pr_err("%s: Length not block aligned\n", __func__);
+		return -EINVAL;
+	}
+
+	/* Do not allow erase past end of device */
+	if ((offs + len) > mtd->size) {
+		pr_err("%s: Erase past end of device\n", __func__);
+		return -EINVAL;
+	}
+
+	einfo->fail_addr = MTD_FAIL_ADDR_UNKNOWN;
+
+	/* Grab the lock and see if the device is available */
+	spinand_get_device(chip);
+
+	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;
+
+	spinand_release_device(mtd_to_spinand(mtd));
+
+	/* Do call back function */
+	if (!ret)
+		mtd_erase_callback(einfo);
+
+	/* Return more or less happy */
+	return ret;
+}
 /**
  * spinand_scan_id_table - scan chip info in id table
  * @chip: SPI-NAND device structure
@@ -415,6 +1297,11 @@ int spinand_scan_tail(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, 4);
diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
index f3d0351..ee447c1 100644
--- a/include/linux/mtd/spinand.h
+++ b/include/linux/mtd/spinand.h
@@ -135,6 +135,8 @@ struct spinand_manufacturer_ops {
 	bool(*detect)(struct spinand_device *chip);
 	int (*init)(struct spinand_device *chip);
 	void (*cleanup)(struct spinand_device *chip);
+	void (*build_column_addr)(struct spinand_device *chip,
+				  struct spinand_op *op, u32 page, u32 column);
 	int (*get_dummy)(struct spinand_device *chip, struct spinand_op *op);
 };
 
-- 
1.9.1

[PATCH v2 3/6] 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.

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

diff --git a/drivers/mtd/nand/spi/spinand_base.c b/drivers/mtd/nand/spi/spinand_base.c
index 1bc57e9..442997d 100644
--- a/drivers/mtd/nand/spi/spinand_base.c
+++ b/drivers/mtd/nand/spi/spinand_base.c
@@ -1003,6 +1003,113 @@ static int spinand_write_oob(struct mtd_info *mtd, loff_t to,
 	return ret;
 }
 
+/**
+ * spinand_block_bad - Check if block at offset is bad
+ * @mtd: MTD device structure
+ * @offs: offset relative to mtd start
+ * @getchip: 0, if the chip is already selected
+ */
+static int spinand_block_bad(struct mtd_info *mtd, loff_t ofs, int getchip)
+{
+	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;
+
+	block_addr = nand_offs_to_eraseblock(nand, ofs);
+	ops.mode = MTD_OPS_PLACE_OOB;
+	ops.ooblen = 2;
+	ops.oobbuf = bad;
+
+	if (getchip)
+		spinand_get_device(mtd_to_spinand(mtd));
+	spinand_do_read_ops(mtd, nand_eraseblock_to_offs(nand, block_addr), &ops);
+	if (getchip)
+		spinand_release_device(mtd_to_spinand(mtd));
+	if (bad[0] != 0xFF || bad[1] != 0xFF)
+		ret =  1;
+
+	return ret;
+}
+
+/**
+ * spinand_block_isbad - [MTD Interface] Check if block at offset is bad
+ * @mtd: MTD device structure
+ * @offs: offset relative to mtd start
+ */
+static int spinand_block_isbad(struct mtd_info *mtd, loff_t offs)
+{
+	return spinand_block_bad(mtd, offs, 1);
+}
+
+/**
+ * spinand_block_markbad_lowlevel - mark a block bad
+ * @mtd: MTD device structure
+ * @ofs: offset from device start
+ *
+ * This function performs the generic bad block marking steps (i.e., bad
+ * block table(s) and/or marker(s)). We only allow the hardware driver to
+ * specify how to write bad block markers to OOB (chip->block_markbad).
+ *
+ * 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
+ * Note that we retain the first error encountered in (2) or (3), finish the
+ * procedures, and dump the error in the end.
+*/
+static int spinand_block_markbad_lowlevel(struct mtd_info *mtd, loff_t ofs)
+{
+	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 ret = 0;
+
+	/*erase bad block before mark bad block*/
+	einfo.mtd = mtd;
+	einfo.addr = ofs;
+	einfo.len = nand_eraseblock_size(nand);
+	spinand_erase(mtd, &einfo);
+
+	block_addr = nand_offs_to_eraseblock(nand, ofs);
+	ops.mode = MTD_OPS_PLACE_OOB;
+	ops.ooblen = 2;
+	ops.oobbuf = buf;
+	spinand_get_device(mtd_to_spinand(mtd));
+	ret = spinand_do_write_ops(mtd,
+		nand_eraseblock_to_offs(nand, block_addr), &ops);
+	spinand_release_device(mtd_to_spinand(mtd));
+
+	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
+ * @ofs: offset relative to mtd start
+ */
+static int spinand_block_markbad(struct mtd_info *mtd, loff_t ofs)
+{
+	int ret;
+
+	ret = spinand_block_isbad(mtd, ofs);
+	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, ofs);
+}
 
 /**
  * spinand_erase - [MTD Interface] erase block(s)
@@ -1044,6 +1151,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_bad(mtd, offs, 0)) {
+			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);
@@ -1302,6 +1416,8 @@ int spinand_scan_tail(struct spinand_device *chip)
 	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;
 
 	if (!mtd->bitflip_threshold)
 		mtd->bitflip_threshold = DIV_ROUND_UP(mtd->ecc_strength * 3, 4);
-- 
1.9.1

[PATCH v2 4/6] nand: spi: Add BBT support

[Peter Pan]

This commit enables BBT for spi nand. spi nand shares
the same BBT code with raw nand.

Signed-off-by: Peter Pan <peterpandong@micron.com>
---
 drivers/mtd/nand/spi/spinand_base.c | 198 ++++++++++++++++++++++++++++++++----
 1 file changed, 178 insertions(+), 20 deletions(-)

diff --git a/drivers/mtd/nand/spi/spinand_base.c b/drivers/mtd/nand/spi/spinand_base.c
index 442997d..6848289 100644
--- a/drivers/mtd/nand/spi/spinand_base.c
+++ b/drivers/mtd/nand/spi/spinand_base.c
@@ -1034,13 +1034,35 @@ static int spinand_block_bad(struct mtd_info *mtd, loff_t ofs, int getchip)
 }
 
 /**
+ * spinand_block_checkbad - Check if a block is marked bad
+ * @mtd: MTD device structure
+ * @ofs: offset from device start
+ * @getchip: 0, if the chip is already selected
+ * @allowbbt: 1, if its allowed to access the bbt area
+ *
+ * Check, if the block is bad. Either by reading the bad block table or
+ * calling of the scan function.
+ */
+static int spinand_block_checkbad(struct mtd_info *mtd, loff_t ofs,
+			int getchip, int allowbbt)
+{
+	struct nand_device *nand = mtd_to_nand(mtd);
+
+	if (!nand->bbt.bbt)
+		return spinand_block_bad(mtd, ofs, getchip);
+
+	/* Return info from the table */
+	return nand_isbad_bbt(nand, ofs, allowbbt);
+}
+
+/**
  * spinand_block_isbad - [MTD Interface] Check if block at offset is bad
  * @mtd: MTD device structure
  * @offs: offset relative to mtd start
  */
 static int spinand_block_isbad(struct mtd_info *mtd, loff_t offs)
 {
-	return spinand_block_bad(mtd, offs, 1);
+	return spinand_block_checkbad(mtd, offs, 1, 0);
 }
 
 /**
@@ -1067,22 +1089,31 @@ static int spinand_block_markbad_lowlevel(struct mtd_info *mtd, loff_t ofs)
 	struct erase_info einfo = {0};
 	u32 block_addr;
 	u8 buf[2] = {0, 0};
-	int ret = 0;
-
-	/*erase bad block before mark bad block*/
-	einfo.mtd = mtd;
-	einfo.addr = ofs;
-	einfo.len = nand_eraseblock_size(nand);
-	spinand_erase(mtd, &einfo);
+	int res, ret = 0;
+
+	if (!nand->bbt.bbt || !(nand->bbt.options & NAND_BBT_NO_OOB_BBM)) {
+		/*erase bad block before mark bad block*/
+		einfo.mtd = mtd;
+		einfo.addr = ofs;
+		einfo.len = nand_eraseblock_size(nand);
+		spinand_erase(mtd, &einfo);
+
+		block_addr = nand_offs_to_eraseblock(nand, ofs);
+		ops.mode = MTD_OPS_PLACE_OOB;
+		ops.ooblen = 2;
+		ops.oobbuf = buf;
+		spinand_get_device(mtd_to_spinand(mtd));
+		ret = spinand_do_write_ops(mtd,
+			nand_eraseblock_to_offs(nand, block_addr), &ops);
+		spinand_release_device(mtd_to_spinand(mtd));
+	}
 
-	block_addr = nand_offs_to_eraseblock(nand, ofs);
-	ops.mode = MTD_OPS_PLACE_OOB;
-	ops.ooblen = 2;
-	ops.oobbuf = buf;
-	spinand_get_device(mtd_to_spinand(mtd));
-	ret = spinand_do_write_ops(mtd,
-		nand_eraseblock_to_offs(nand, block_addr), &ops);
-	spinand_release_device(mtd_to_spinand(mtd));
+	/* Mark block bad in BBT */
+	if (nand->bbt.bbt) {
+		res = nand_markbad_bbt(nand, ofs);
+		if (!ret)
+			ret = res;
+	}
 
 	if (!ret)
 		mtd->ecc_stats.badblocks++;
@@ -1112,13 +1143,15 @@ static int spinand_block_markbad(struct mtd_info *mtd, loff_t ofs)
 }
 
 /**
- * spinand_erase - [MTD Interface] erase block(s)
+ * __spinand_erase - erase block(s)
  * @mtd: MTD device structure
  * @einfo: erase instruction
+ * @allowbbt: allow to access bbt
  *
  * Erase one ore more blocks
  */
-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);
@@ -1152,7 +1185,7 @@ static int spinand_erase(struct mtd_info *mtd, struct erase_info *einfo)
 
 	while (len) {
 		/* Check if we have a bad block, we do not erase bad blocks! */
-		if (spinand_block_bad(mtd, offs, 0)) {
+		if (spinand_block_checkbad(mtd, offs, 0, allowbbt)) {
 			pr_warn("%s: attempt to erase a bad block at 0x%012llx\n",
 			__func__, offs);
 			einfo->state = MTD_ERASE_FAILED;
@@ -1193,6 +1226,55 @@ static int spinand_erase(struct mtd_info *mtd, struct erase_info *einfo)
 	/* Return more or less happy */
 	return ret;
 }
+
+/**
+ * spinand_erase - [MTD Interface] erase block(s)
+ * @mtd: MTD device structure
+ * @einfo: erase instruction
+ *
+ * Erase one ore more blocks
+ */
+static int spinand_erase(struct mtd_info *mtd, struct erase_info *einfo)
+{
+	return __spinand_erase(mtd, einfo, 0);
+}
+
+
+static int spinand_erase_bbt(struct nand_device *nand, struct erase_info *einfo)
+{
+	return __spinand_erase(nand_to_mtd(nand), einfo, 1);
+}
+
+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);
+}
+
+/**
+ * spinand_block_isreserved - [MTD Interface] Check if a block is
+ * marked reserved.
+ * @mtd: MTD device structure
+ * @ofs: offset from device start
+ */
+static int spinand_block_isreserved(struct mtd_info *mtd, loff_t ofs)
+{
+	struct nand_device *nand = mtd_to_nand(mtd);
+
+	if (!nand->bbt.bbt)
+		return 0;
+	/* Return info from the table */
+	return nand_isreserved_bbt(nand, ofs);
+}
+
 /**
  * spinand_scan_id_table - scan chip info in id table
  * @chip: SPI-NAND device structure
@@ -1260,6 +1342,74 @@ static void spinand_set_rd_wr_op(struct spinand_device *chip)
 		chip->write_cache_op = SPINAND_CMD_PROG_LOAD;
 }
 
+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 uint8_t scan_ff_pattern[] = { 0xff, 0xff };
+
+#define BADBLOCK_SCAN_MASK (~NAND_BBT_NO_OOB)
+
+/**
+ * nand_create_badblock_pattern - [INTERN] Creates a BBT descriptor structure
+ * @this: NAND chip to create descriptor for
+ * @chip: SPI-NAND device structure
+ *
+ * This function allocates and initializes a nand_bbt_descr for BBM detection
+ * based on the properties of @this. The new descriptor is stored in
+ * this->badblock_pattern. Thus, this->badblock_pattern 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;
+}
+
+static int spinand_default_bbt(struct mtd_info *mtd)
+{
+	struct nand_device *nand = mtd_to_nand(mtd);
+	struct spinand_device *chip = mtd_to_spinand(mtd);
+	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);
+}
+
+static void spinand_fill_nandd(struct spinand_device *chip)
+{
+	struct nand_device *nand = &chip->base;
+
+	nand->ops = &spinand_ops;
+}
+
 /*
  * Manufacturer detection. Only used when the NAND is not ONFI or JEDEC
  * compliant and does not have a full-id or legacy-id entry in the nand_ids
@@ -1372,6 +1522,7 @@ int spinand_scan_ident(struct spinand_device *chip, u8 expect_mfr_id)
 		return -ENOMEM;
 
 	chip->oobbuf = chip->buf + nand_page_size(nand);
+	spinand_fill_nandd(chip);
 	spinand_lock_block(chip, BL_ALL_UNLOCKED);
 
 	return 0;
@@ -1418,11 +1569,13 @@ int spinand_scan_tail(struct spinand_device *chip)
 	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, 4);
 
-	return 0;
+	/* Build bad block table */
+	return spinand_default_bbt(mtd);
 }
 EXPORT_SYMBOL_GPL(spinand_scan_tail);
 
@@ -1458,10 +1611,15 @@ int spinand_scan_tail_release(struct spinand_device *chip)
 int spinand_release(struct spinand_device *chip)
 {
 	struct mtd_info *mtd = spinand_to_mtd(chip);
+	struct nand_device *nand = mtd_to_nand(mtd);
+	struct nand_bbt_descr *bd = nand->bbt.bbp;
 
 	mtd_device_unregister(mtd);
 	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 v2 5/6] 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/spinand_ids.c    |   5 ++
 drivers/mtd/nand/spi/spinand_micron.c | 133 ++++++++++++++++++++++++++++++++++
 3 files changed, 139 insertions(+)
 create mode 100644 drivers/mtd/nand/spi/spinand_micron.c

diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile
index 6f0d622..84b9bcc 100644
--- a/drivers/mtd/nand/spi/Makefile
+++ b/drivers/mtd/nand/spi/Makefile
@@ -1,2 +1,3 @@
 obj-$(CONFIG_MTD_SPI_NAND) += spinand_base.o
 obj-$(CONFIG_MTD_SPI_NAND) += spinand_ids.o
+obj-$(CONFIG_MTD_SPI_NAND) += spinand_micron.o
diff --git a/drivers/mtd/nand/spi/spinand_ids.c b/drivers/mtd/nand/spi/spinand_ids.c
index 7706ae3..10f74f1 100644
--- a/drivers/mtd/nand/spi/spinand_ids.c
+++ b/drivers/mtd/nand/spi/spinand_ids.c
@@ -18,11 +18,16 @@
 #include <linux/mtd/spinand.h>
 
 struct spinand_flash spinand_table[] = {
+	SPI_NAND_INFO("MT29F2G01ABAGD", 0x2C, 0x24, 2048, 128, 64, 2048,
+			1, 8, SPINAND_OP_COMMON),
 	{.name = NULL},
 };
 
 
+extern struct spinand_manufacturer_ops micron_spinand_manuf_ops;
+
 struct spinand_manufacturer spinand_manuf_ids[] = {
+	{SPINAND_MFR_MICRON, "Micron", &micron_spinand_manuf_ops},
 	{0x0, "Unknown"}
 };
 
diff --git a/drivers/mtd/nand/spi/spinand_micron.c b/drivers/mtd/nand/spi/spinand_micron.c
new file mode 100644
index 0000000..d978935
--- /dev/null
+++ b/drivers/mtd/nand/spi/spinand_micron.c
@@ -0,0 +1,133 @@
+/**
+* spi-nand-base.c
+*
+* 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 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
+
+
+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 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 void mt29f_build_column_addr(struct spinand_device *chip,
+				           struct spinand_op *op,
+				           u32 page, u32 column)
+{
+	struct nand_device *nand = &chip->base;
+
+	op->n_addr = 2;
+	op->addr[0] = (u8)(column >> 8);
+	op->addr[0] |= (u8)((nand_page_to_eraseblock(nand, page)
+				& 0x1) << 4);
+	op->addr[1] = (u8)column;
+}
+
+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;
+	}
+}
+
+static struct spinand_ecc_engine_ops generic_spi_ecc_engine_ops = {
+	.get_ecc_status = mt29f_get_ecc_status,
+};
+
+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;
+}
+
+const struct spinand_manufacturer_ops micron_spinand_manuf_ops = {
+	.init = micron_spinand_init,
+	.build_column_addr = mt29f_build_column_addr,
+	.get_dummy = micron_spinand_get_dummy,
+};
-- 
1.9.1

[PATCH v2 6/6] 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/chips/Kconfig       |   5 +
 drivers/mtd/nand/spi/chips/Makefile      |   1 +
 drivers/mtd/nand/spi/chips/generic_spi.c | 158 +++++++++++++++++++++++++++++++
 5 files changed, 167 insertions(+)
 create mode 100644 drivers/mtd/nand/spi/chips/Kconfig
 create mode 100644 drivers/mtd/nand/spi/chips/Makefile
 create mode 100644 drivers/mtd/nand/spi/chips/generic_spi.c

diff --git a/drivers/mtd/nand/spi/Kconfig b/drivers/mtd/nand/spi/Kconfig
index 04a7b71..07ebbb0 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/chips/Kconfig"
diff --git a/drivers/mtd/nand/spi/Makefile b/drivers/mtd/nand/spi/Makefile
index 84b9bcc..700878d 100644
--- a/drivers/mtd/nand/spi/Makefile
+++ b/drivers/mtd/nand/spi/Makefile
@@ -1,3 +1,4 @@
 obj-$(CONFIG_MTD_SPI_NAND) += spinand_base.o
 obj-$(CONFIG_MTD_SPI_NAND) += spinand_ids.o
 obj-$(CONFIG_MTD_SPI_NAND) += spinand_micron.o
+obj-$(CONFIG_MTD_SPI_NAND) += chips/
diff --git a/drivers/mtd/nand/spi/chips/Kconfig b/drivers/mtd/nand/spi/chips/Kconfig
new file mode 100644
index 0000000..337a94f
--- /dev/null
+++ b/drivers/mtd/nand/spi/chips/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/chips/Makefile b/drivers/mtd/nand/spi/chips/Makefile
new file mode 100644
index 0000000..2cb7763
--- /dev/null
+++ b/drivers/mtd/nand/spi/chips/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_GENERIC_SPI_NAND) += generic_spi.o
diff --git a/drivers/mtd/nand/spi/chips/generic_spi.c b/drivers/mtd/nand/spi/chips/generic_spi.c
new file mode 100644
index 0000000..4f47144
--- /dev/null
+++ b/drivers/mtd/nand/spi/chips/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
+ * @cmd: command structure
+ * Description:
+ *   Set up the command buffer to send to the SPI controller.
+ *   The command buffer has to initialized to 0.
+ */
+static int generic_spinand_cmd_fn(struct spinand_device *chip,
+				struct spinand_op *cmd)
+{
+	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 = &cmd->cmd;
+	spi_message_add_tail(&x[0], &message);
+
+	if (cmd->n_addr + cmd->dummy_bytes) {
+		x[1].len = cmd->n_addr + cmd->dummy_bytes;
+		x[1].tx_nbits = cmd->addr_nbits;
+		x[1].tx_buf = cmd->addr;
+		spi_message_add_tail(&x[1], &message);
+	}
+	if (cmd->n_tx) {
+		x[2].len = cmd->n_tx;
+		x[2].tx_nbits = cmd->data_nbits;
+		x[2].tx_buf = cmd->tx_buf;
+		spi_message_add_tail(&x[2], &message);
+	} else if (cmd->n_rx) {
+		x[2].len = cmd->n_rx;
+		x[2].rx_nbits = cmd->data_nbits;
+		x[2].rx_buf = cmd->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;
+	int mfr;
+	struct mtd_info *mtd;
+	int ret;
+	u32 max_speed_hz = spi->max_speed_hz;
+
+	chip = kzalloc(sizeof(struct spinand_device), GFP_KERNEL);
+	if (!chip) {
+		ret = -ENOMEM;
+		goto err1;
+	}
+	mtd = spinand_to_mtd(chip);
+	chip->controller.ops = &generic_spi_ops;
+	chip->controller.caps = SPINAND_CAP_RD_X1 | SPINAND_CAP_WR_X1;
+	if (spi->mode & SPI_RX_QUAD)
+		chip->controller.caps |= SPINAND_CAP_RD_QUAD | SPINAND_CAP_RD_X4;
+	if (spi->mode & SPI_RX_DUAL)
+		chip->controller.caps |= SPINAND_CAP_RD_DUAL | SPINAND_CAP_RD_X2;
+	if (spi->mode & SPI_TX_QUAD)
+		chip->controller.caps |= SPINAND_CAP_WR_QUAD | SPINAND_CAP_WR_X4;
+	if (spi->mode & SPI_TX_DUAL)
+		chip->controller.caps |= SPINAND_CAP_WR_DUAL | SPINAND_CAP_WR_X2;
+
+	spinand_set_controller_data(chip, spi);
+	spi_set_drvdata(spi, chip);
+	/*
+	 * read ID command format might be different for different manufactory
+	 * such as, Micron SPI NAND need extra one dummy byte after perform
+	 * read ID command but Giga device don't need.
+	 *
+	 * So, specify manufactory of device in device tree is obligatory
+	 */
+	mfr = spi_get_device_id(spi)->driver_data;
+	spi->max_speed_hz = min_t(int, 25000000, max_speed_hz);
+	ret = spinand_scan_ident(chip, mfr);
+	if (ret)
+		goto err2;
+
+	spi->max_speed_hz = max_speed_hz;
+	ret = spinand_scan_tail(chip);
+	if (ret)
+		goto err3;
+
+	mtd_set_of_node(mtd, spi->dev.of_node);
+
+	ret = mtd_device_parse_register(mtd, NULL, NULL, NULL, 0);
+	if (!ret)
+		return 0;
+
+	spinand_scan_tail_release(chip);
+err3:
+	spinand_scan_ident_release(chip);
+err2:
+	kfree(chip);
+err1:
+	return ret;
+}
+
+static int generic_spinand_remove(struct spi_device *spi)
+{
+	struct spinand_device *chip = spi_get_drvdata(spi);
+
+	spinand_release(chip);
+	kfree(chip);
+
+	return 0;
+}
+
+
+static const struct spi_device_id spinand_id_table[] = {
+	{ "mt29f", SPINAND_MFR_MICRON },
+	{ },
+};
+MODULE_DEVICE_TABLE(spi, spinand_id_table);
+
+static struct spi_driver generic_spinand_driver = {
+	.driver = {
+		.name	= "generic_spinand",
+		.owner	= THIS_MODULE,
+	},
+	.id_table = spinand_id_table,
+	.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

Re: [PATCH v2 1/6] nand: spi: Add init/release function

[Boris Brezillon]

+Arnaud

Hi Peter,

Can you please Cc Arnaud (and in general all reviewers) each time you
send a new version.

On Wed, 1 Mar 2017 16:52:05 +0800
Peter Pan <peterpandong@micron.com> wrote:

> This is the first commit for spi nand framkework.
> This commit is to add spi nand initialization and
> release functions.

Hm, actually you're doing more than that. Just say that you 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/spinand_base.c | 469 ++++++++++++++++++++++++++++++++++++
>  drivers/mtd/nand/spi/spinand_ids.c  |  29 +++
>  include/linux/mtd/spinand.h         | 315 ++++++++++++++++++++++++

If you're okay with that, I'd like you to add an entry in MAINTAINERS
for the spinand sub-sub-sub-system :-). This way you'll be tagged as
the maintainer of this code and will be Cc when a patch is submitted.

>  7 files changed, 822 insertions(+)
>  create mode 100644 drivers/mtd/nand/spi/Kconfig
>  create mode 100644 drivers/mtd/nand/spi/Makefile
>  create mode 100644 drivers/mtd/nand/spi/spinand_base.c
>  create mode 100644 drivers/mtd/nand/spi/spinand_ids.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..04a7b71
> --- /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..6f0d622
> --- /dev/null
> +++ b/drivers/mtd/nand/spi/Makefile
> @@ -0,0 +1,2 @@
> +obj-$(CONFIG_MTD_SPI_NAND) += spinand_base.o
> +obj-$(CONFIG_MTD_SPI_NAND) += spinand_ids.o
> diff --git a/drivers/mtd/nand/spi/spinand_base.c b/drivers/mtd/nand/spi/spinand_base.c
> new file mode 100644
> index 0000000..97d47146
> --- /dev/null
> +++ b/drivers/mtd/nand/spi/spinand_base.c

How about renaming this file into core.c?

> @@ -0,0 +1,469 @@
> +/**
> +* spi-nand-base.c

Please do not put the file name in the copyright header. It's not even
correct, which shows how useless this information is ;-).

> +*
> +* 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/sched.h>
> +#include <linux/delay.h>
> +#include <linux/jiffies.h>
> +#include <linux/mtd/spinand.h>
> +#include <linux/slab.h>
> +
> +
> +static u64 spinand_get_chip_size(struct spinand_device *chip)

I would change the parameter name here: s/chip/spinand/. This is
applicable to the whole submission.

> +{
> +	struct nand_device *nand = &chip->base;
> +
> +	return nand_diesize(nand) * nand_ndies(nand);
> +}

Probably something that should go in include/linux/mtd/nand.h or
drivers/mtd/nand/core.c.

> +
> +static inline int spinand_exec_cmd(struct spinand_device *chip,
> +				struct spinand_op *cmd)
> +{
> +	return chip->controller.ops->exec_op(chip, cmd);
> +}
> +
> +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,
> +			uint8_t reg, uint8_t *buf)

Align parameters on the open parenthesis (run checkpatch.pl --strict
and fix everything you can).

> +{
> +	struct spinand_op cmd;
> +	int ret;
> +
> +	spinand_op_init(&cmd);
> +	cmd.cmd = SPINAND_CMD_GET_FEATURE;
> +	cmd.n_addr = 1;
> +	cmd.addr[0] = reg;
> +	cmd.n_rx = 1;
> +	cmd.rx_buf = buf;
> +
> +	ret = spinand_exec_cmd(chip, &cmd);
> +	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,
> +			uint8_t reg, uint8_t *buf)
> +{
> +	struct spinand_op cmd;
> +	int ret;
> +
> +	spinand_op_init(&cmd);
> +	cmd.cmd = SPINAND_CMD_SET_FEATURE;
> +	cmd.n_addr = 1;
> +	cmd.addr[0] = reg;
> +	cmd.n_tx = 1,
> +	cmd.tx_buf = buf,
> +
> +	ret = spinand_exec_cmd(chip, &cmd);
> +	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.
> + *   Once the status turns to be ready, the other status bits also are
> + *   valid status bits.
> + */
> +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(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 out last check

		 ^ our last check.

> +	 */
> +	spinand_read_status(chip, &status);
> +out:
> +	if (s)
> +		*s = status;
> +
> +	return (status & STATUS_OIP_MASK) == STATUS_READY ? 0 :	-ETIMEDOUT;;

Extra semi-colon at the end of the line.

> +}
> +
> +/**
> + * spinand_read_id - send 9Fh command to get ID
> + * @chip: SPI-NAND device structure
> + * @buf: buffer to store id
> + */
> +static int spinand_read_id(struct spinand_device *chip, u8 *buf)
> +{
> +	struct spinand_op cmd;
> +
> +	spinand_op_init(&cmd);
> +	cmd.cmd = SPINAND_CMD_READ_ID;
> +	if (chip->manufacturer.ops->get_dummy)
> +		cmd.dummy_bytes = chip->manufacturer.ops->get_dummy(chip, &cmd);
> +	cmd.n_rx = 2;
> +	cmd.rx_buf = buf;
> +
> +	return spinand_exec_cmd(chip, &cmd);
> +}
> +
> +/**
> + * spinand_reset - send command FFh to reset chip.
> + * @chip: SPI-NAND device structure
> + */
> +static int spinand_reset(struct spinand_device *chip)
> +{
> +	struct spinand_op cmd;
> +	int ret;
> +
> +	spinand_op_init(&cmd);
> +	cmd.cmd = SPINAND_CMD_RESET;
> +
> +	ret = spinand_exec_cmd(chip, &cmd);
> +	if (ret < 0) {
> +		pr_err("spinand reset failed!\n");
> +		goto out;
> +	}
> +	ret = spinand_wait(chip, NULL);

Add an empty line here.

> +out:
> +	return ret;
> +}
> +
> +/**
> + * spinand_lock_block - write block lock register to
> + * lock/unlock device
> + * @spi: spi device structure
> + * @lock: value to set to block lock register
> + * Description:
> + *   After power up, all the Nand blocks are locked.  This function allows
> + *   one to unlock the blocks, and so it can be written or erased.
> + */
> +static int spinand_lock_block(struct spinand_device *chip, u8 lock)
> +{
> +	return spinand_write_reg(chip, REG_BLOCK_LOCK, &lock);
> +}
> +
> +/**
> + * 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 spinand_scan_id_table(struct spinand_device *chip, u8 *id)
> +{
> +	struct nand_device *nand = &chip->base;
> +	struct spinand_flash *type = spinand_table;
> +	struct nand_memory_organization *memorg = &nand->memorg;
> +	struct spinand_ecc_engine *ecc_engine = &chip->ecc_engine;
> +
> +	for (; type->name; type++) {
> +		if (id[0] == type->mfr_id && id[1] == type->dev_id) {
> +			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;
> +}
> +
> +/**
> + * spinand_set_rd_wr_op - Chose the best read write command
> + * @chip: SPI-NAND device structure
> + * Description:
> + *   Chose the fastest r/w command according to spi controller's ability.
> + * Note:
> + *   If 03h/0Bh follows SPI NAND protocol, there is no difference,
> + *   while if follows SPI NOR protocol, 03h command is working under
> + *   <=20Mhz@3.3V,<=5MHz@1.8V; 0Bh command is working under
> + *   133Mhz@3.3v, 83Mhz@1.8V.
> + */
> +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))

Try to make checkpatch happy here as well:

	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;
> +}
> +
> +/*
> + * Manufacturer detection. Only used when the NAND is not ONFI or JEDEC
> + * compliant and does not have a full-id or legacy-id entry in the nand_ids
> + * table.
> + */
> +static bool spinand_manufacturer_detect(struct spinand_device *chip)
> +{
> +	if (chip->manufacturer.ops && chip->manufacturer.ops->detect)
> +		return chip->manufacturer.ops->detect(chip);

Add an empty line.

> +	return false;
> +}
> +
> +/*
> + * Manufacturer initialization. This function is called for all NANDs including
> + * ONFI and JEDEC compliant ones.
> + * 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.ops && chip->manufacturer.ops->init)
> +		return chip->manufacturer.ops->init(chip);
> +
> +	return 0;
> +}
> +
> +/*
> + * Manufacturer cleanup. This function is called for all NANDs including
> + * ONFI and JEDEC compliant ones.
> + * 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.ops && chip->manufacturer.ops->cleanup)
> +		return chip->manufacturer.ops->cleanup(chip);
> +}
> +
> +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;
> +}
> +
> +static u8 spinand_get_mfr_id(struct spinand_device *chip)
> +{
> +	return chip->id.data[SPINAND_MFR_ID];
> +}
> +
> +static u8 spinand_get_dev_id(struct spinand_device *chip)
> +{
> +	return chip->id.data[SPINAND_DEV_ID];
> +}
> +
> +static void spinand_set_manufacturer_ops(struct spinand_device *chip, u8 mfr_id)
> +{
> +	int i = 0;
> +
> +	for (; spinand_manuf_ids[i].id != 0x0; i++) {
> +		if (spinand_manuf_ids[i].id == mfr_id)
> +			break;
> +	}

Add an empty line here.

> +	chip->manufacturer.ops = spinand_manuf_ids[i].ops;
> +}
> +
> +/**
> + * spinand_scan_ident - [SPI-NAND Interface] Scan for the SPI-NAND device
> + * @chip: SPI-NAND device structure
> + * Description:
> + *   This is the first phase of the initiazation. It reads the flash ID and
> + *   sets up spinand_device fields accordingly.
> + */
> +int spinand_scan_ident(struct spinand_device *chip, u8 expect_mfr_id)
> +{
> +	struct nand_device *nand = &chip->base;
> +	u8 id[SPINAND_MAX_ID_LEN] = {0};
> +	int id_retry = 2;
> +
> +	spinand_set_manufacturer_ops(chip, expect_mfr_id);
> +	spinand_reset(chip);
> +read_id:
> +	spinand_read_id(chip, id);
> +	if (spinand_scan_id_table(chip, id))
> +		goto ident_done;
> +	if (id_retry--)
> +		goto read_id;
> +	pr_info("SPI-NAND type mfr_id: %x, dev_id: %x is not in id table.\n",
> +				id[SPINAND_MFR_ID], id[SPINAND_DEV_ID]);
> +	if (spinand_manufacturer_detect(chip))
> +		goto ident_done;
> +
> +	return -ENODEV;
> +
> +ident_done:


	for (i = 0; i < MAX_READID_RETRY; i++) {
		ret = spinand_read_id(chip, id);
		if (ret)
			return ret;

		if (spinand_scan_id_table(chip, id))
			break;

		if (spinand_manufacturer_detect(chip))
			break;
	}

	if (i == MAX_READID_RETRY) {
		/* Error message. */
		return -ENODEV;
	}

BTW, why do we need to retry 2 times? I thought that if
spinand_read_id() returns 0 this means we read the ID correctly, and if
the NAND detection fails once it will always fail.

> +	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)(spinand_get_chip_size(chip) >> 20), nand_eraseblock_size(nand) >> 10,
> +		nand_page_size(nand), nand_per_page_oobsize(nand));
> +	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);
> +	spinand_lock_block(chip, BL_ALL_UNLOCKED);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(spinand_scan_ident);
> +
> +/**
> + * spinand_scan_tail - [SPI-NAND Interface] Scan for the SPI-NAND device
> + * @chip: SPI-NAND device structure
> + * Description:
> + *   This is the second phase of the initiazation. It fills out all the
> + *   uninitialized fields of spinand_device and mtd fields.
> + */
> +int spinand_scan_tail(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);
> +
> +	mtd->name = chip->name;
> +	mtd->size = spinand_get_chip_size(chip);
> +	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);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(spinand_scan_tail);
> +
> +/**
> + * spinand_scan_ident_release - [SPI-NAND Interface] Free resources
> + * applied by spinand_scan_ident
> + * @chip: SPI-NAND device structure
> + */
> +int spinand_scan_ident_release(struct spinand_device *chip)
> +{
> +	spinand_manufacturer_cleanup(chip);
> +	kfree(chip->buf);
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(spinand_scan_ident_release);

Drop this function, and only define spinand_release

> +
> +/**
> + * spinand_scan_tail_release - [SPI-NAND Interface] Free resources
> + * applied by spinand_scan_tail
> + * @chip: SPI-NAND device structure
> + */
> +int spinand_scan_tail_release(struct spinand_device *chip)
> +{
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(spinand_scan_tail_release);

Why is this needed?

> +
> +/**
> + * spinand_release - [SPI-NAND Interface] Free resources held by the SPI-NAND
> + * device
> + * @chip: SPI-NAND device structure
> + */
> +int spinand_release(struct spinand_device *chip)

I'd call it spinand_cleanup(), because the spinand device has not been
allocated by the core.

> +{
> +	struct mtd_info *mtd = spinand_to_mtd(chip);
> +
> +	mtd_device_unregister(mtd);

Please don't do that. I know it's taken directly from the // NAND
framework, but it is confusing: the MTD dev registration is left to the
SPI NAND controller, so we should let it unregister the MTD device by
himself.

> +	spinand_manufacturer_cleanup(chip);
> +	kfree(chip->buf);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(spinand_release);
> +
> +MODULE_DESCRIPTION("SPI NAND framework");
> +MODULE_AUTHOR("Peter Pan<peterpandong@micron.com>");
> +MODULE_LICENSE("GPL v2");
> diff --git a/drivers/mtd/nand/spi/spinand_ids.c b/drivers/mtd/nand/spi/spinand_ids.c
> new file mode 100644
> index 0000000..7706ae3
> --- /dev/null
> +++ b/drivers/mtd/nand/spi/spinand_ids.c

Remove the spinand_ prefix in the file name, we know it's spinand
related thanks to the path (drivers/mtd/nand/spi/):

s/spinand_ids.c/ids.c/

> @@ -0,0 +1,29 @@
> +/**
> +* spi-nand-base.c
> +*
> +* 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_flash spinand_table[] = {
> +	{.name = NULL},
> +};
> +
> +
> +struct spinand_manufacturer spinand_manuf_ids[] = {
> +	{0x0, "Unknown"}
> +};
> +
> +EXPORT_SYMBOL(spinand_manuf_ids);
> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> new file mode 100644
> index 0000000..f3d0351
> --- /dev/null
> +++ b/include/linux/mtd/spinand.h
> @@ -0,0 +1,315 @@
> +/**
> +* spinand.h
> +*
> +* 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		2
> +/**
> + * 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 {
> +	#define SPINAND_MFR_ID		0
> +	#define SPINAND_DEV_ID		1

Please move these defines just below the SPINAND_MAX_ID_LEN definition.

> +	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);
> +	int (*init)(struct spinand_device *chip);
> +	void (*cleanup)(struct spinand_device *chip);
> +	int (*get_dummy)(struct spinand_device *chip, struct spinand_op *op);
> +};
> +
> +struct spinand_manufacturer {
> +	u8 id;
> +	char *name;
> +	struct spinand_manufacturer_ops *ops;
> +};
> +
> +extern struct spinand_manufacturer spinand_manuf_ids[];
> +
> +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);
> +};

At some point we should find a way to make the ECC handling generic
(there's nothing SPINAND specific here), but let's keep that for later.

> +
> +typedef enum {
> +	SPINAND_ECC_ONDIE,
> +	SPINAND_ECC_HW,
> +} spinand_ecc_modes_t;

No typedefs, just

enum spinand_ecc_mode {
	SPINAND_ECC_ONDIE,
	...
};

> +
> +
> +struct spinand_ecc_engine {
> +	spinand_ecc_modes_t mode;
> +	u32 strength;
> +	u32 steps;
> +	struct spinand_ecc_engine_ops *ops;
> +};
> +
> +/**
> + * 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 {
> +		struct spinand_controller_ops *ops;
> +#define SPINAND_CAP_RD_X1 (1 << 0)
> +#define SPINAND_CAP_RD_X2 (1 << 1)
> +#define SPINAND_CAP_RD_X4 (1 << 2)
> +#define SPINAND_CAP_RD_DUAL (1 << 3)
> +#define SPINAND_CAP_RD_QUAD (1 << 4)
> +#define SPINAND_CAP_WR_X1 (1 << 5)
> +#define SPINAND_CAP_WR_X2 (1 << 6)
> +#define SPINAND_CAP_WR_X4 (1 << 7)
> +#define SPINAND_CAP_WR_DUAL (1 << 8)
> +#define SPINAND_CAP_WR_QUAD (1 << 9)
> +#define SPINAND_CAP_HW_ECC (1 << 10)

Again, I don't like when struct definitions and macros are intermixed.
Please move that before the struct def.

> +		u32 caps;
> +		void *priv;
> +	} controller;

Hm, shouldn't we point to a spinand_controller object? I mean, maybe
there are some situations where you have a single spinand_controller
which interacts with several spinand devices.

struct spinand_controller {
	struct spinand_controller_ops *ops;
	u32 caps;
};

and then in spinand_device:

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

> +	struct {
> +		struct spinand_manufacturer_ops *ops;

Should be const, and let's store the spinand_manufacturer pointer
directly, this way we have the manufacture name directly accessible.

> +		void *priv;
> +	} manufacturer;
> +	struct spinand_ecc_engine ecc_engine;

Same here, the ECC engine should allocated separately, and
spinand_device should point to it.

	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;
> +}
> +
> +
> +struct spinand_flash {

s/spinand_flash/spinand_desc/ or s/spinand_flash/spinand_info/ ?

> +	char *name;
> +	u8 mfr_id;
> +	u8 dev_id;

We'd better use an array of u8 here, just in case manufacturers decide
to put more 2 id bytes ;-).

> +	u32 page_size;
> +	u32 oob_size;
> +	u32 pages_per_blk;
> +	u32 blks_per_lun;
> +	u32 luns_per_chip;
> +	u32 ecc_strength;
> +	u32 options;
> +	u32 rw_mode;
> +};
> +
> +extern struct spinand_flash spinand_table[];
> +
> +#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
> +
> +#define SPI_NAND_INFO(nm, mid, did, pagesz, oobsz, pg_per_blk,\
> +	blk_per_lun, lun_per_chip, ecc_stren, rwmode)	\
> +	{ .name = (nm), .mfr_id = (mid), .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) }

Please make this more readable.

#define SPI_NAND_INFO(nm, mid, did, pagesz, oobsz, pg_per_blk,	\
		      blk_per_lun, lun_per_chip, ecc_stren,	\
		      rwmode)					\
	{							\
		.name = (nm), .mfr_id = (mid), .dev_id = (did),	\
		....						\
	}

Also, I'm wondering, is this ID table still useful if we have
per-manufacturer init functions? If it's not, maybe we should get rid
of it.

That does not mean manufacture drivers can't have their own table, but
if there's nothing to share between manufacturers (IOW, if the dev_id
field is not standardized), then there's no need to expose a huge id
table in the core.

> +
> +/*SPI NAND manufacture ID definition*/
> +#define SPINAND_MFR_MICRON		0x2C

Should not be exposed here (keep it in your vendor source file.

> +
> +int spinand_scan_ident(struct spinand_device *chip, u8 expect_mfr_id);
> +int spinand_scan_tail(struct spinand_device *chip);
> +int spinand_scan_ident_release(struct spinand_device *chip);
> +int spinand_scan_tail_release(struct spinand_device *chip);
> +int spinand_release(struct spinand_device *chip);

How about clarifying a bit the interface:
- s/spinand_scan_ident/spinand_detect/
- s/spinand_scan_tail/spinand_init/
- s/spinand_release/spinand_cleanup/

> +int spinand_set_cmd_cfg_table(int mfr);
> +struct spinand_op_def *spinand_get_cmd_cfg(u8 opcode);
> +#endif /* __LINUX_MTD_SPINAND_H */

Regards,

Boris

Re: [PATCH v2 1/6] nand: spi: Add init/release function

[Thomas Petazzoni]

Hello,

On Wed, 1 Mar 2017 16:52:05 +0800, Peter Pan wrote:

> +static bool spinand_scan_id_table(struct spinand_device *chip, u8 *id)
> +{
> +	struct nand_device *nand = &chip->base;
> +	struct spinand_flash *type = spinand_table;
> +	struct nand_memory_organization *memorg = &nand->memorg;
> +	struct spinand_ecc_engine *ecc_engine = &chip->ecc_engine;
> +
> +	for (; type->name; type++) {
> +		if (id[0] == type->mfr_id && id[1] == type->dev_id) {
> +			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;
> +		}
> +	}

One minor nit, based on a very quick look. What about instead:

	for (type = spinand_table; type->name; type++) {
		if (id[0] != type->mfr_id)
			continue;
		if (id[1] != type->dev_id)
			continue;
		chip->name = type->name;
		...
		return true;
	}

I.e, use the initialization part of the for() loop, and inside the
for() loop, avoid one indentation level by handling the non-interesting
case first, and skipping to the next table entry if needed.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Re: [PATCH v2 1/6] nand: spi: Add init/release function

[Peter Pan]

Hi Boris,

Thanks again for your comments.

Hi Arnaud,
I'm so sorry for missing you in my list. And thanks for your comments
in v1 also.

On Wed, Mar 1, 2017 at 5:58 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> +Arnaud
>
> Hi Peter,
>
> Can you please Cc Arnaud (and in general all reviewers) each time you
> send a new version.

I will add all reviewers in the next versions.

>
> On Wed, 1 Mar 2017 16:52:05 +0800
> Peter Pan <peterpandong@micron.com> wrote:
>
>> This is the first commit for spi nand framkework.
>> This commit is to add spi nand initialization and
>> release functions.
>
> Hm, actually you're doing more than that. Just say that you add basic
> building blocks for the SPI NAND infrastructure.

Fix in v3

>
>>
>> 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/spinand_base.c | 469 ++++++++++++++++++++++++++++++++++++
>>  drivers/mtd/nand/spi/spinand_ids.c  |  29 +++
>>  include/linux/mtd/spinand.h         | 315 ++++++++++++++++++++++++
>
> If you're okay with that, I'd like you to add an entry in MAINTAINERS
> for the spinand sub-sub-sub-system :-). This way you'll be tagged as
> the maintainer of this code and will be Cc when a patch is submitted.

If you are OK with this. I'm glad to update the MAINTAINERS file. :)
Besides, is there a rule to add this info?

>
>>  7 files changed, 822 insertions(+)
>>  create mode 100644 drivers/mtd/nand/spi/Kconfig
>>  create mode 100644 drivers/mtd/nand/spi/Makefile
>>  create mode 100644 drivers/mtd/nand/spi/spinand_base.c
>>  create mode 100644 drivers/mtd/nand/spi/spinand_ids.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..04a7b71
>> --- /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..6f0d622
>> --- /dev/null
>> +++ b/drivers/mtd/nand/spi/Makefile
>> @@ -0,0 +1,2 @@
>> +obj-$(CONFIG_MTD_SPI_NAND) += spinand_base.o
>> +obj-$(CONFIG_MTD_SPI_NAND) += spinand_ids.o
>> diff --git a/drivers/mtd/nand/spi/spinand_base.c b/drivers/mtd/nand/spi/spinand_base.c
>> new file mode 100644
>> index 0000000..97d47146
>> --- /dev/null
>> +++ b/drivers/mtd/nand/spi/spinand_base.c
>
> How about renaming this file into core.c?

core.c is much more clear while spinand_base.c matches rawnand_base.c
Should we rename them at the same time or rename spinand_base.c first?

>
>> @@ -0,0 +1,469 @@
>> +/**
>> +* spi-nand-base.c
>
> Please do not put the file name in the copyright header. It's not even
> correct, which shows how useless this information is ;-).

Fix this in v3

>
>> +*
>> +* 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/sched.h>
>> +#include <linux/delay.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/mtd/spinand.h>
>> +#include <linux/slab.h>
>> +
>> +
>> +static u64 spinand_get_chip_size(struct spinand_device *chip)
>
> I would change the parameter name here: s/chip/spinand/. This is
> applicable to the whole submission.

Fix this in v3.

>
>> +{
>> +     struct nand_device *nand = &chip->base;
>> +
>> +     return nand_diesize(nand) * nand_ndies(nand);
>> +}
>
> Probably something that should go in include/linux/mtd/nand.h or
> drivers/mtd/nand/core.c.

Yes. I will add an interface in include/linux/mtd/nand.h.
nand_chip_size() or nand_device_size(), which one is better?

>
>> +
>> +static inline int spinand_exec_cmd(struct spinand_device *chip,
>> +                             struct spinand_op *cmd)
>> +{
>> +     return chip->controller.ops->exec_op(chip, cmd);
>> +}
>> +
>> +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,
>> +                     uint8_t reg, uint8_t *buf)
>
> Align parameters on the open parenthesis (run checkpatch.pl --strict
> and fix everything you can).

Fix this in v3

>
>> +{
>> +     struct spinand_op cmd;
>> +     int ret;
>> +
>> +     spinand_op_init(&cmd);
>> +     cmd.cmd = SPINAND_CMD_GET_FEATURE;
>> +     cmd.n_addr = 1;
>> +     cmd.addr[0] = reg;
>> +     cmd.n_rx = 1;
>> +     cmd.rx_buf = buf;
>> +
>> +     ret = spinand_exec_cmd(chip, &cmd);
>> +     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,
>> +                     uint8_t reg, uint8_t *buf)
>> +{
>> +     struct spinand_op cmd;
>> +     int ret;
>> +
>> +     spinand_op_init(&cmd);
>> +     cmd.cmd = SPINAND_CMD_SET_FEATURE;
>> +     cmd.n_addr = 1;
>> +     cmd.addr[0] = reg;
>> +     cmd.n_tx = 1,
>> +     cmd.tx_buf = buf,
>> +
>> +     ret = spinand_exec_cmd(chip, &cmd);
>> +     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.
>> + *   Once the status turns to be ready, the other status bits also are
>> + *   valid status bits.
>> + */
>> +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(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 out last check
>
>                  ^ our last check.

Fix this in v3

>
>> +      */
>> +     spinand_read_status(chip, &status);
>> +out:
>> +     if (s)
>> +             *s = status;
>> +
>> +     return (status & STATUS_OIP_MASK) == STATUS_READY ? 0 : -ETIMEDOUT;;
>
> Extra semi-colon at the end of the line.

Fix this in v3

>
>> +}
>> +
>> +/**
>> + * spinand_read_id - send 9Fh command to get ID
>> + * @chip: SPI-NAND device structure
>> + * @buf: buffer to store id
>> + */
>> +static int spinand_read_id(struct spinand_device *chip, u8 *buf)
>> +{
>> +     struct spinand_op cmd;
>> +
>> +     spinand_op_init(&cmd);
>> +     cmd.cmd = SPINAND_CMD_READ_ID;
>> +     if (chip->manufacturer.ops->get_dummy)
>> +             cmd.dummy_bytes = chip->manufacturer.ops->get_dummy(chip, &cmd);
>> +     cmd.n_rx = 2;
>> +     cmd.rx_buf = buf;
>> +
>> +     return spinand_exec_cmd(chip, &cmd);
>> +}
>> +
>> +/**
>> + * spinand_reset - send command FFh to reset chip.
>> + * @chip: SPI-NAND device structure
>> + */
>> +static int spinand_reset(struct spinand_device *chip)
>> +{
>> +     struct spinand_op cmd;
>> +     int ret;
>> +
>> +     spinand_op_init(&cmd);
>> +     cmd.cmd = SPINAND_CMD_RESET;
>> +
>> +     ret = spinand_exec_cmd(chip, &cmd);
>> +     if (ret < 0) {
>> +             pr_err("spinand reset failed!\n");
>> +             goto out;
>> +     }
>> +     ret = spinand_wait(chip, NULL);
>
> Add an empty line here.

Fix this in v3

>
>> +out:
>> +     return ret;
>> +}
>> +
>> +/**
>> + * spinand_lock_block - write block lock register to
>> + * lock/unlock device
>> + * @spi: spi device structure
>> + * @lock: value to set to block lock register
>> + * Description:
>> + *   After power up, all the Nand blocks are locked.  This function allows
>> + *   one to unlock the blocks, and so it can be written or erased.
>> + */
>> +static int spinand_lock_block(struct spinand_device *chip, u8 lock)
>> +{
>> +     return spinand_write_reg(chip, REG_BLOCK_LOCK, &lock);
>> +}
>> +
>> +/**
>> + * 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 spinand_scan_id_table(struct spinand_device *chip, u8 *id)
>> +{
>> +     struct nand_device *nand = &chip->base;
>> +     struct spinand_flash *type = spinand_table;
>> +     struct nand_memory_organization *memorg = &nand->memorg;
>> +     struct spinand_ecc_engine *ecc_engine = &chip->ecc_engine;
>> +
>> +     for (; type->name; type++) {
>> +             if (id[0] == type->mfr_id && id[1] == type->dev_id) {
>> +                     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;
>> +}
>> +
>> +/**
>> + * spinand_set_rd_wr_op - Chose the best read write command
>> + * @chip: SPI-NAND device structure
>> + * Description:
>> + *   Chose the fastest r/w command according to spi controller's ability.
>> + * Note:
>> + *   If 03h/0Bh follows SPI NAND protocol, there is no difference,
>> + *   while if follows SPI NOR protocol, 03h command is working under
>> + *   <=20Mhz@3.3V,<=5MHz@1.8V; 0Bh command is working under
>> + *   133Mhz@3.3v, 83Mhz@1.8V.
>> + */
>> +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))
>
> Try to make checkpatch happy here as well:

Fix this in v3

>
>         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;
>> +}
>> +
>> +/*
>> + * Manufacturer detection. Only used when the NAND is not ONFI or JEDEC
>> + * compliant and does not have a full-id or legacy-id entry in the nand_ids
>> + * table.
>> + */
>> +static bool spinand_manufacturer_detect(struct spinand_device *chip)
>> +{
>> +     if (chip->manufacturer.ops && chip->manufacturer.ops->detect)
>> +             return chip->manufacturer.ops->detect(chip);
>
> Add an empty line.

Fix this in v3

>
>> +     return false;
>> +}
>> +
>> +/*
>> + * Manufacturer initialization. This function is called for all NANDs including
>> + * ONFI and JEDEC compliant ones.
>> + * 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.ops && chip->manufacturer.ops->init)
>> +             return chip->manufacturer.ops->init(chip);
>> +
>> +     return 0;
>> +}
>> +
>> +/*
>> + * Manufacturer cleanup. This function is called for all NANDs including
>> + * ONFI and JEDEC compliant ones.
>> + * 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.ops && chip->manufacturer.ops->cleanup)
>> +             return chip->manufacturer.ops->cleanup(chip);
>> +}
>> +
>> +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;
>> +}
>> +
>> +static u8 spinand_get_mfr_id(struct spinand_device *chip)
>> +{
>> +     return chip->id.data[SPINAND_MFR_ID];
>> +}
>> +
>> +static u8 spinand_get_dev_id(struct spinand_device *chip)
>> +{
>> +     return chip->id.data[SPINAND_DEV_ID];
>> +}
>> +
>> +static void spinand_set_manufacturer_ops(struct spinand_device *chip, u8 mfr_id)
>> +{
>> +     int i = 0;
>> +
>> +     for (; spinand_manuf_ids[i].id != 0x0; i++) {
>> +             if (spinand_manuf_ids[i].id == mfr_id)
>> +                     break;
>> +     }
>
> Add an empty line here.

Fix this in v3

>
>> +     chip->manufacturer.ops = spinand_manuf_ids[i].ops;
>> +}
>> +
>> +/**
>> + * spinand_scan_ident - [SPI-NAND Interface] Scan for the SPI-NAND device
>> + * @chip: SPI-NAND device structure
>> + * Description:
>> + *   This is the first phase of the initiazation. It reads the flash ID and
>> + *   sets up spinand_device fields accordingly.
>> + */
>> +int spinand_scan_ident(struct spinand_device *chip, u8 expect_mfr_id)
>> +{
>> +     struct nand_device *nand = &chip->base;
>> +     u8 id[SPINAND_MAX_ID_LEN] = {0};
>> +     int id_retry = 2;
>> +
>> +     spinand_set_manufacturer_ops(chip, expect_mfr_id);
>> +     spinand_reset(chip);
>> +read_id:
>> +     spinand_read_id(chip, id);
>> +     if (spinand_scan_id_table(chip, id))
>> +             goto ident_done;
>> +     if (id_retry--)
>> +             goto read_id;
>> +     pr_info("SPI-NAND type mfr_id: %x, dev_id: %x is not in id table.\n",
>> +                             id[SPINAND_MFR_ID], id[SPINAND_DEV_ID]);
>> +     if (spinand_manufacturer_detect(chip))
>> +             goto ident_done;
>> +
>> +     return -ENODEV;
>> +
>> +ident_done:
>
>
>         for (i = 0; i < MAX_READID_RETRY; i++) {
>                 ret = spinand_read_id(chip, id);
>                 if (ret)
>                         return ret;
>
>                 if (spinand_scan_id_table(chip, id))
>                         break;
>
>                 if (spinand_manufacturer_detect(chip))
>                         break;
>         }
>
>         if (i == MAX_READID_RETRY) {
>                 /* Error message. */
>                 return -ENODEV;
>         }
>
> BTW, why do we need to retry 2 times? I thought that if
> spinand_read_id() returns 0 this means we read the ID correctly, and if
> the NAND detection fails once it will always fail.

Fix this in v3

>
>> +     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)(spinand_get_chip_size(chip) >> 20), nand_eraseblock_size(nand) >> 10,
>> +             nand_page_size(nand), nand_per_page_oobsize(nand));
>> +     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);
>> +     spinand_lock_block(chip, BL_ALL_UNLOCKED);
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(spinand_scan_ident);
>> +
>> +/**
>> + * spinand_scan_tail - [SPI-NAND Interface] Scan for the SPI-NAND device
>> + * @chip: SPI-NAND device structure
>> + * Description:
>> + *   This is the second phase of the initiazation. It fills out all the
>> + *   uninitialized fields of spinand_device and mtd fields.
>> + */
>> +int spinand_scan_tail(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);
>> +
>> +     mtd->name = chip->name;
>> +     mtd->size = spinand_get_chip_size(chip);
>> +     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);
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(spinand_scan_tail);
>> +
>> +/**
>> + * spinand_scan_ident_release - [SPI-NAND Interface] Free resources
>> + * applied by spinand_scan_ident
>> + * @chip: SPI-NAND device structure
>> + */
>> +int spinand_scan_ident_release(struct spinand_device *chip)
>> +{
>> +     spinand_manufacturer_cleanup(chip);
>> +     kfree(chip->buf);
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(spinand_scan_ident_release);
>
> Drop this function, and only define spinand_release

Fix this in v3

>
>> +
>> +/**
>> + * spinand_scan_tail_release - [SPI-NAND Interface] Free resources
>> + * applied by spinand_scan_tail
>> + * @chip: SPI-NAND device structure
>> + */
>> +int spinand_scan_tail_release(struct spinand_device *chip)
>> +{
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(spinand_scan_tail_release);
>
> Why is this needed?

Remove it in v3

>
>> +
>> +/**
>> + * spinand_release - [SPI-NAND Interface] Free resources held by the SPI-NAND
>> + * device
>> + * @chip: SPI-NAND device structure
>> + */
>> +int spinand_release(struct spinand_device *chip)
>
> I'd call it spinand_cleanup(), because the spinand device has not been
> allocated by the core.
>
>> +{
>> +     struct mtd_info *mtd = spinand_to_mtd(chip);
>> +
>> +     mtd_device_unregister(mtd);
>
> Please don't do that. I know it's taken directly from the // NAND
> framework, but it is confusing: the MTD dev registration is left to the
> SPI NAND controller, so we should let it unregister the MTD device by
> himself.

Fix this in v3

>
>> +     spinand_manufacturer_cleanup(chip);
>> +     kfree(chip->buf);
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(spinand_release);
>> +
>> +MODULE_DESCRIPTION("SPI NAND framework");
>> +MODULE_AUTHOR("Peter Pan<peterpandong@micron.com>");
>> +MODULE_LICENSE("GPL v2");
>> diff --git a/drivers/mtd/nand/spi/spinand_ids.c b/drivers/mtd/nand/spi/spinand_ids.c
>> new file mode 100644
>> index 0000000..7706ae3
>> --- /dev/null
>> +++ b/drivers/mtd/nand/spi/spinand_ids.c
>
> Remove the spinand_ prefix in the file name, we know it's spinand
> related thanks to the path (drivers/mtd/nand/spi/):
>
> s/spinand_ids.c/ids.c/
>
Fix this in v3

>> @@ -0,0 +1,29 @@
>> +/**
>> +* spi-nand-base.c
>> +*
>> +* 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_flash spinand_table[] = {
>> +     {.name = NULL},
>> +};
>> +
>> +
>> +struct spinand_manufacturer spinand_manuf_ids[] = {
>> +     {0x0, "Unknown"}
>> +};
>> +
>> +EXPORT_SYMBOL(spinand_manuf_ids);
>> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
>> new file mode 100644
>> index 0000000..f3d0351
>> --- /dev/null
>> +++ b/include/linux/mtd/spinand.h
>> @@ -0,0 +1,315 @@
>> +/**
>> +* spinand.h
>> +*
>> +* 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           2
>> +/**
>> + * 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 {
>> +     #define SPINAND_MFR_ID          0
>> +     #define SPINAND_DEV_ID          1
>
> Please move these defines just below the SPINAND_MAX_ID_LEN definition.

Fix this in v3

>
>> +     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);
>> +     int (*init)(struct spinand_device *chip);
>> +     void (*cleanup)(struct spinand_device *chip);
>> +     int (*get_dummy)(struct spinand_device *chip, struct spinand_op *op);
>> +};
>> +
>> +struct spinand_manufacturer {
>> +     u8 id;
>> +     char *name;
>> +     struct spinand_manufacturer_ops *ops;
>> +};
>> +
>> +extern struct spinand_manufacturer spinand_manuf_ids[];
>> +
>> +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);
>> +};
>
> At some point we should find a way to make the ECC handling generic
> (there's nothing SPINAND specific here), but let's keep that for later.

I agree with you. ECC ops should be shared in the feature.

>
>> +
>> +typedef enum {
>> +     SPINAND_ECC_ONDIE,
>> +     SPINAND_ECC_HW,
>> +} spinand_ecc_modes_t;
>
> No typedefs, just
>
> enum spinand_ecc_mode {
>         SPINAND_ECC_ONDIE,
>         ...
> };
>

Fix this in v3

>> +
>> +
>> +struct spinand_ecc_engine {
>> +     spinand_ecc_modes_t mode;
>> +     u32 strength;
>> +     u32 steps;
>> +     struct spinand_ecc_engine_ops *ops;
>> +};
>> +
>> +/**
>> + * 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 {
>> +             struct spinand_controller_ops *ops;
>> +#define SPINAND_CAP_RD_X1 (1 << 0)
>> +#define SPINAND_CAP_RD_X2 (1 << 1)
>> +#define SPINAND_CAP_RD_X4 (1 << 2)
>> +#define SPINAND_CAP_RD_DUAL (1 << 3)
>> +#define SPINAND_CAP_RD_QUAD (1 << 4)
>> +#define SPINAND_CAP_WR_X1 (1 << 5)
>> +#define SPINAND_CAP_WR_X2 (1 << 6)
>> +#define SPINAND_CAP_WR_X4 (1 << 7)
>> +#define SPINAND_CAP_WR_DUAL (1 << 8)
>> +#define SPINAND_CAP_WR_QUAD (1 << 9)
>> +#define SPINAND_CAP_HW_ECC (1 << 10)
>
> Again, I don't like when struct definitions and macros are intermixed.
> Please move that before the struct def.

Fix this in v3

>
>> +             u32 caps;
>> +             void *priv;
>> +     } controller;
>
> Hm, shouldn't we point to a spinand_controller object? I mean, maybe
> there are some situations where you have a single spinand_controller
> which interacts with several spinand devices.
>
> struct spinand_controller {
>         struct spinand_controller_ops *ops;
>         u32 caps;
> };
>
> and then in spinand_device:
>
> struct spinand_device {
>         struct {
>                 struct spinand_controller *master;
>                 void *priv;
>         } controller;
> }

Good comments. Fix this in v3

>
>> +     struct {
>> +             struct spinand_manufacturer_ops *ops;
>
> Should be const, and let's store the spinand_manufacturer pointer
> directly, this way we have the manufacture name directly accessible.

Fix this in v3

>
>> +             void *priv;
>> +     } manufacturer;
>> +     struct spinand_ecc_engine ecc_engine;
>
> Same here, the ECC engine should allocated separately, and
> spinand_device should point to it.
>
>         struct {
>                 struct spinand_ecc_engine *engine;
>                 void *context;
>         } ecc;
>

Fix this in v3

>> +};
>> +
>> +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;
>> +}
>> +
>> +
>> +struct spinand_flash {
>
> s/spinand_flash/spinand_desc/ or s/spinand_flash/spinand_info/ ?

Fix this in v3

>
>> +     char *name;
>> +     u8 mfr_id;
>> +     u8 dev_id;
>
> We'd better use an array of u8 here, just in case manufacturers decide
> to put more 2 id bytes ;-).

Fix this in v3

>
>> +     u32 page_size;
>> +     u32 oob_size;
>> +     u32 pages_per_blk;
>> +     u32 blks_per_lun;
>> +     u32 luns_per_chip;
>> +     u32 ecc_strength;
>> +     u32 options;
>> +     u32 rw_mode;
>> +};
>> +
>> +extern struct spinand_flash spinand_table[];
>> +
>> +#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
>> +
>> +#define SPI_NAND_INFO(nm, mid, did, pagesz, oobsz, pg_per_blk,\
>> +     blk_per_lun, lun_per_chip, ecc_stren, rwmode)   \
>> +     { .name = (nm), .mfr_id = (mid), .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) }
>
> Please make this more readable.
>
> #define SPI_NAND_INFO(nm, mid, did, pagesz, oobsz, pg_per_blk,  \
>                       blk_per_lun, lun_per_chip, ecc_stren,     \
>                       rwmode)                                   \
>         {                                                       \
>                 .name = (nm), .mfr_id = (mid), .dev_id = (did), \
>                 ....                                            \
>         }
>
> Also, I'm wondering, is this ID table still useful if we have
> per-manufacturer init functions? If it's not, maybe we should get rid
> of it.
>
> That does not mean manufacture drivers can't have their own table, but
> if there's nothing to share between manufacturers (IOW, if the dev_id
> field is not standardized), then there's no need to expose a huge id
> table in the core.

Good comment. Let manufacture's detect function to handle there own table.
What do you think, Boris?
BTW, there is another question. read id method is not unique. Micron spi nand
need a dummy byte before reading ID while some vendors don't. Now I define
vendor alias in DTS and use this info to choose right manufacture ops. Do you
have a better idea?

>
>> +
>> +/*SPI NAND manufacture ID definition*/
>> +#define SPINAND_MFR_MICRON           0x2C
>
> Should not be exposed here (keep it in your vendor source file.
>
>> +
>> +int spinand_scan_ident(struct spinand_device *chip, u8 expect_mfr_id);
>> +int spinand_scan_tail(struct spinand_device *chip);
>> +int spinand_scan_ident_release(struct spinand_device *chip);
>> +int spinand_scan_tail_release(struct spinand_device *chip);
>> +int spinand_release(struct spinand_device *chip);
>
> How about clarifying a bit the interface:
> - s/spinand_scan_ident/spinand_detect/
> - s/spinand_scan_tail/spinand_init/
> - s/spinand_release/spinand_cleanup/

Fix this in v3.

Thanks
Peter Pan

Re: [PATCH v2 1/6] nand: spi: Add init/release function

[Peter Pan]

Hi Thomas,

On Wed, Mar 1, 2017 at 9:21 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Hello,
>
> On Wed, 1 Mar 2017 16:52:05 +0800, Peter Pan wrote:
>
>> +static bool spinand_scan_id_table(struct spinand_device *chip, u8 *id)
>> +{
>> +     struct nand_device *nand = &chip->base;
>> +     struct spinand_flash *type = spinand_table;
>> +     struct nand_memory_organization *memorg = &nand->memorg;
>> +     struct spinand_ecc_engine *ecc_engine = &chip->ecc_engine;
>> +
>> +     for (; type->name; type++) {
>> +             if (id[0] == type->mfr_id && id[1] == type->dev_id) {
>> +                     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;
>> +             }
>> +     }
>
> One minor nit, based on a very quick look. What about instead:
>
>         for (type = spinand_table; type->name; type++) {
>                 if (id[0] != type->mfr_id)
>                         continue;
>                 if (id[1] != type->dev_id)
>                         continue;
>                 chip->name = type->name;
>                 ...
>                 return true;
>         }
>
> I.e, use the initialization part of the for() loop, and inside the
> for() loop, avoid one indentation level by handling the non-interesting
> case first, and skipping to the next table entry if needed.

Thanks for your comment. I will fix this in v3

Thanks,
Peter Pan

Re: [PATCH v2 1/6] nand: spi: Add init/release function

[Boris Brezillon]

On Fri, 3 Mar 2017 16:37:55 +0800
Peter Pan <peterpansjtu@gmail.com> wrote:

> >  
> >>
> >> 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/spinand_base.c | 469 ++++++++++++++++++++++++++++++++++++
> >>  drivers/mtd/nand/spi/spinand_ids.c  |  29 +++
> >>  include/linux/mtd/spinand.h         | 315 ++++++++++++++++++++++++  
> >
> > If you're okay with that, I'd like you to add an entry in MAINTAINERS
> > for the spinand sub-sub-sub-system :-). This way you'll be tagged as
> > the maintainer of this code and will be Cc when a patch is submitted.  
> 
> If you are OK with this. I'm glad to update the MAINTAINERS file. :)

Of course I am. Anything that can help me maintain the NAND
sub-system is welcome.

> Besides, is there a rule to add this info?

You can be a driver maintainer, or a sub-system maintainer. People
usually don't do this for drivers, but it's actually good to have
someone that can review/test changes.
So, my rule is: if you had a new driver or a new subsystem (which is
the case here), add an entry in MAINTAINERS.

> >> diff --git a/drivers/mtd/nand/spi/spinand_base.c b/drivers/mtd/nand/spi/spinand_base.c
> >> new file mode 100644
> >> index 0000000..97d47146
> >> --- /dev/null
> >> +++ b/drivers/mtd/nand/spi/spinand_base.c  
> >
> > How about renaming this file into core.c?  
> 
> core.c is much more clear while spinand_base.c matches rawnand_base.c
> Should we rename them at the same time or rename spinand_base.c first?

No, let's keep rawnand code unchanged.


> >  
> >> +{
> >> +     struct nand_device *nand = &chip->base;
> >> +
> >> +     return nand_diesize(nand) * nand_ndies(nand);
> >> +}  
> >
> > Probably something that should go in include/linux/mtd/nand.h or
> > drivers/mtd/nand/core.c.  
> 
> Yes. I will add an interface in include/linux/mtd/nand.h.
> nand_chip_size() or nand_device_size(), which one is better?

Maybe just nand_size(), to be consistent with other function names.


> > That does not mean manufacture drivers can't have their own table, but
> > if there's nothing to share between manufacturers (IOW, if the dev_id
> > field is not standardized), then there's no need to expose a huge id
> > table in the core.  
> 
> Good comment. Let manufacture's detect function to handle there own table.
> What do you think, Boris?

Actually, I'm the one asking? :-)
If there's nothing to share between manufacturers, then yes, let's keep
the NAND id table private to each manufacturer driver.

> BTW, there is another question. read id method is not unique. Micron spi nand
> need a dummy byte before reading ID while some vendors don't. Now I define
> vendor alias in DTS and use this info to choose right manufacture ops. Do you
> have a better idea?

Ouch. That's bad news. How about letting the manufacturer code read the
ID and detect the NAND?

That means you'll iterate over all manufacturer entries in the
manufacturer table and call ->detect(). The ->detect() hook will be
responsible for reading the ID (with the proper read-id sequence) and
initialize the NAND parameters.

If we find a common pattern between different vendors, we can then
provide default helpers for the read-id and/or detect implementation.

Re: [PATCH v2 1/6] nand: spi: Add init/release function

[Arnaud Mouiche]

On 03/03/2017 10:28, Boris Brezillon wrote:
> On Fri, 3 Mar 2017 16:37:55 +0800
> Peter Pan <peterpansjtu@gmail.com> wrote:
>
> [..]
>> BTW, there is another question. read id method is not unique. Micron spi nand
>> need a dummy byte before reading ID while some vendors don't. Now I define
>> vendor alias in DTS and use this info to choose right manufacture ops. Do you
>> have a better idea?
> Ouch. That's bad news. How about letting the manufacturer code read the
> ID and detect the NAND?
>
> That means you'll iterate over all manufacturer entries in the
> manufacturer table and call ->detect(). The ->detect() hook will be
> responsible for reading the ID (with the proper read-id sequence) and
> initialize the NAND parameters.
>
> If we find a common pattern between different vendors, we can then
> provide default helpers for the read-id and/or detect implementation.
A effective way will be to read the up to 4 bytes of ID response, and 
ask every manufacturer to provide an ID and mask to compare with.
Here is the list of ID/MASK I have compiled.

         "MT29F1G01AAADD",
         .id = 0x002C1200,  .id_mask = 0x00FFFF00,

         "MT29F2G01AAAED",

         "MT29F4G01AAADD",
         .id = 0x002C3200,  .id_mask = 0x00FFFF00,

         "GD5F1GQ4xC", /* version U (3.3V) or R (1.8) */
         .id = 0xC8A14800,  .id_mask = 0xffefff00, /* 0xC8A148 or 
0xC8B148 */

         "GD5F2GQ4xC", /* version U (3.3V) or R (1.8) */
         .id = 0xC8A24800,  .id_mask = 0xffefff00, /* 0xC8A248 or 
0xC8B248 */

         "GD5F1GQ4xBYIG", /* version U (3.3V) or R (1.8) */
         .id = 0x00C8D100,  .id_mask = 0x00ffef00, /* C8D1 or C8C1 */

         "GD5F2GQ4xBYIG", /* version U (3.3V) or R (1.8) */
         .id = 0x00C8D200,  .id_mask = 0x00ffef00, /* C8D2 or C8C2 */

         "F50L1G41A", /* ESMT */
         .id = 0x00C8217F,  .id_mask = 0x00ffffff,

         "W25N01GVZEIG", /* Winbond */
         .id = 0x00EFAA21,  .id_mask = 0x00ffffff,

         "MX35LF1GE4AB", /* Micronix */
         .id = 0x00C21200,  .id_mask = 0x00ffff00,


Arnaud

Re: [PATCH v2 1/6] nand: spi: Add init/release function

[Boris Brezillon]

On Fri, 3 Mar 2017 10:37:04 +0100
Arnaud Mouiche <arnaud.mouiche@gmail.com> wrote:

> On 03/03/2017 10:28, Boris Brezillon wrote:
> > On Fri, 3 Mar 2017 16:37:55 +0800
> > Peter Pan <peterpansjtu@gmail.com> wrote:
> >
> > [..]  
> >> BTW, there is another question. read id method is not unique. Micron spi nand
> >> need a dummy byte before reading ID while some vendors don't. Now I define
> >> vendor alias in DTS and use this info to choose right manufacture ops. Do you
> >> have a better idea?  
> > Ouch. That's bad news. How about letting the manufacturer code read the
> > ID and detect the NAND?
> >
> > That means you'll iterate over all manufacturer entries in the
> > manufacturer table and call ->detect(). The ->detect() hook will be
> > responsible for reading the ID (with the proper read-id sequence) and
> > initialize the NAND parameters.
> >
> > If we find a common pattern between different vendors, we can then
> > provide default helpers for the read-id and/or detect implementation.  
> A effective way will be to read the up to 4 bytes of ID response, and 
> ask every manufacturer to provide an ID and mask to compare with.
> Here is the list of ID/MASK I have compiled.
> 
>          "MT29F1G01AAADD",
>          .id = 0x002C1200,  .id_mask = 0x00FFFF00,
> 
>          "MT29F2G01AAAED",
> 
>          "MT29F4G01AAADD",
>          .id = 0x002C3200,  .id_mask = 0x00FFFF00,
> 
>          "GD5F1GQ4xC", /* version U (3.3V) or R (1.8) */
>          .id = 0xC8A14800,  .id_mask = 0xffefff00, /* 0xC8A148 or 
> 0xC8B148 */
> 
>          "GD5F2GQ4xC", /* version U (3.3V) or R (1.8) */
>          .id = 0xC8A24800,  .id_mask = 0xffefff00, /* 0xC8A248 or 
> 0xC8B248 */
> 
>          "GD5F1GQ4xBYIG", /* version U (3.3V) or R (1.8) */
>          .id = 0x00C8D100,  .id_mask = 0x00ffef00, /* C8D1 or C8C1 */
> 
>          "GD5F2GQ4xBYIG", /* version U (3.3V) or R (1.8) */
>          .id = 0x00C8D200,  .id_mask = 0x00ffef00, /* C8D2 or C8C2 */
> 
>          "F50L1G41A", /* ESMT */
>          .id = 0x00C8217F,  .id_mask = 0x00ffffff,
> 
>          "W25N01GVZEIG", /* Winbond */
>          .id = 0x00EFAA21,  .id_mask = 0x00ffffff,
> 
>          "MX35LF1GE4AB", /* Micronix */
>          .id = 0x00C21200,  .id_mask = 0x00ffff00,

I'm not a big fan of this approach. See how each vendor seems to have
its own scheme, and we're not even sure they will use the same for
their next chips. That's what happened with raw NANDs, and the NAND ID
parsing just became a huge pile of hacks like that:

	if(vendorX and revisionY) then id-should-decoded-like-that;

By letting the detection process to manufacturer code, we just get rid
of this complexity in the core, which is a good thing IMO.

Re: [PATCH v2 1/6] nand: spi: Add init/release function

[Arnaud Mouiche]

On 03/03/2017 11:00, Boris Brezillon wrote:
> On Fri, 3 Mar 2017 10:37:04 +0100
> Arnaud Mouiche <arnaud.mouiche@gmail.com> wrote:
>
>> On 03/03/2017 10:28, Boris Brezillon wrote:
>>> On Fri, 3 Mar 2017 16:37:55 +0800
>>> Peter Pan <peterpansjtu@gmail.com> wrote:
>>>
>>> [..]
>>>> BTW, there is another question. read id method is not unique. Micron spi nand
>>>> need a dummy byte before reading ID while some vendors don't. Now I define
>>>> vendor alias in DTS and use this info to choose right manufacture ops. Do you
>>>> have a better idea?
>>> Ouch. That's bad news. How about letting the manufacturer code read the
>>> ID and detect the NAND?
>>>
>>> That means you'll iterate over all manufacturer entries in the
>>> manufacturer table and call ->detect(). The ->detect() hook will be
>>> responsible for reading the ID (with the proper read-id sequence) and
>>> initialize the NAND parameters.
>>>
>>> If we find a common pattern between different vendors, we can then
>>> provide default helpers for the read-id and/or detect implementation.
>> A effective way will be to read the up to 4 bytes of ID response, and
>> ask every manufacturer to provide an ID and mask to compare with.
>> Here is the list of ID/MASK I have compiled.
>>
>>           "MT29F1G01AAADD",
>>           .id = 0x002C1200,  .id_mask = 0x00FFFF00,
>>
>>           "MT29F2G01AAAED",
>>
>>           "MT29F4G01AAADD",
>>           .id = 0x002C3200,  .id_mask = 0x00FFFF00,
>>
>>           "GD5F1GQ4xC", /* version U (3.3V) or R (1.8) */
>>           .id = 0xC8A14800,  .id_mask = 0xffefff00, /* 0xC8A148 or
>> 0xC8B148 */
>>
>>           "GD5F2GQ4xC", /* version U (3.3V) or R (1.8) */
>>           .id = 0xC8A24800,  .id_mask = 0xffefff00, /* 0xC8A248 or
>> 0xC8B248 */
>>
>>           "GD5F1GQ4xBYIG", /* version U (3.3V) or R (1.8) */
>>           .id = 0x00C8D100,  .id_mask = 0x00ffef00, /* C8D1 or C8C1 */
>>
>>           "GD5F2GQ4xBYIG", /* version U (3.3V) or R (1.8) */
>>           .id = 0x00C8D200,  .id_mask = 0x00ffef00, /* C8D2 or C8C2 */
>>
>>           "F50L1G41A", /* ESMT */
>>           .id = 0x00C8217F,  .id_mask = 0x00ffffff,
>>
>>           "W25N01GVZEIG", /* Winbond */
>>           .id = 0x00EFAA21,  .id_mask = 0x00ffffff,
>>
>>           "MX35LF1GE4AB", /* Micronix */
>>           .id = 0x00C21200,  .id_mask = 0x00ffff00,
> I'm not a big fan of this approach. See how each vendor seems to have
> its own scheme, and we're not even sure they will use the same for
> their next chips. That's what happened with raw NANDs, and the NAND ID
> parsing just became a huge pile of hacks like that:
>
> 	if(vendorX and revisionY) then id-should-decoded-like-that;
>
> By letting the detection process to manufacturer code, we just get rid
> of this complexity in the core, which is a good thing IMO.

I agree, but only if you do the SPI command once, and ask each vendor 
"->detect()" to check the content of the returned data.
Otherwise, it may take some [long] time to finally know the real ID of 
one chip.

Arnaud

Re: [PATCH v2 1/6] nand: spi: Add init/release function

[Boris Brezillon]

On Fri, 3 Mar 2017 11:12:12 +0100
Arnaud Mouiche <arnaud.mouiche@gmail.com> wrote:

> On 03/03/2017 11:00, Boris Brezillon wrote:
> > On Fri, 3 Mar 2017 10:37:04 +0100
> > Arnaud Mouiche <arnaud.mouiche@gmail.com> wrote:
> >  
> >> On 03/03/2017 10:28, Boris Brezillon wrote:  
> >>> On Fri, 3 Mar 2017 16:37:55 +0800
> >>> Peter Pan <peterpansjtu@gmail.com> wrote:
> >>>
> >>> [..]  
> >>>> BTW, there is another question. read id method is not unique. Micron spi nand
> >>>> need a dummy byte before reading ID while some vendors don't. Now I define
> >>>> vendor alias in DTS and use this info to choose right manufacture ops. Do you
> >>>> have a better idea?  
> >>> Ouch. That's bad news. How about letting the manufacturer code read the
> >>> ID and detect the NAND?
> >>>
> >>> That means you'll iterate over all manufacturer entries in the
> >>> manufacturer table and call ->detect(). The ->detect() hook will be
> >>> responsible for reading the ID (with the proper read-id sequence) and
> >>> initialize the NAND parameters.
> >>>
> >>> If we find a common pattern between different vendors, we can then
> >>> provide default helpers for the read-id and/or detect implementation.  
> >> A effective way will be to read the up to 4 bytes of ID response, and
> >> ask every manufacturer to provide an ID and mask to compare with.
> >> Here is the list of ID/MASK I have compiled.
> >>
> >>           "MT29F1G01AAADD",
> >>           .id = 0x002C1200,  .id_mask = 0x00FFFF00,
> >>
> >>           "MT29F2G01AAAED",
> >>
> >>           "MT29F4G01AAADD",
> >>           .id = 0x002C3200,  .id_mask = 0x00FFFF00,
> >>
> >>           "GD5F1GQ4xC", /* version U (3.3V) or R (1.8) */
> >>           .id = 0xC8A14800,  .id_mask = 0xffefff00, /* 0xC8A148 or
> >> 0xC8B148 */
> >>
> >>           "GD5F2GQ4xC", /* version U (3.3V) or R (1.8) */
> >>           .id = 0xC8A24800,  .id_mask = 0xffefff00, /* 0xC8A248 or
> >> 0xC8B248 */
> >>
> >>           "GD5F1GQ4xBYIG", /* version U (3.3V) or R (1.8) */
> >>           .id = 0x00C8D100,  .id_mask = 0x00ffef00, /* C8D1 or C8C1 */
> >>
> >>           "GD5F2GQ4xBYIG", /* version U (3.3V) or R (1.8) */
> >>           .id = 0x00C8D200,  .id_mask = 0x00ffef00, /* C8D2 or C8C2 */
> >>
> >>           "F50L1G41A", /* ESMT */
> >>           .id = 0x00C8217F,  .id_mask = 0x00ffffff,
> >>
> >>           "W25N01GVZEIG", /* Winbond */
> >>           .id = 0x00EFAA21,  .id_mask = 0x00ffffff,
> >>
> >>           "MX35LF1GE4AB", /* Micronix */
> >>           .id = 0x00C21200,  .id_mask = 0x00ffff00,  
> > I'm not a big fan of this approach. See how each vendor seems to have
> > its own scheme, and we're not even sure they will use the same for
> > their next chips. That's what happened with raw NANDs, and the NAND ID
> > parsing just became a huge pile of hacks like that:
> >
> > 	if(vendorX and revisionY) then id-should-decoded-like-that;
> >
> > By letting the detection process to manufacturer code, we just get rid
> > of this complexity in the core, which is a good thing IMO.  
> 
> I agree, but only if you do the SPI command once, and ask each vendor 
> "->detect()" to check the content of the returned data.
> Otherwise, it may take some [long] time to finally know the real ID of 
> one chip.

Hm, we don't have that many manufacturers, and I don't think reading
the id takes a lot of time, but yes, we could define an array of 4
bytes (might need to be extended at some point), read the id once and
let the manufacturer code detect the NAND.


> 
> Arnaud

Re: [PATCH v2 2/6] nand: spi: add basic operations support

[Boris Brezillon]

Hi Peter,

I have a few comments (see below).

On Wed, 1 Mar 2017 16:52:06 +0800
Peter Pan <peterpandong@micron.com> wrote:

> This commit is to support read, readoob, write,
> writeoob and erase operations in spi nand framwork.
> 
> Signed-off-by: Peter Pan <peterpandong@micron.com>
> ---
>  drivers/mtd/nand/spi/spinand_base.c | 887 ++++++++++++++++++++++++++++++++++++
>  include/linux/mtd/spinand.h         |   2 +
>  2 files changed, 889 insertions(+)
> 
> diff --git a/drivers/mtd/nand/spi/spinand_base.c b/drivers/mtd/nand/spi/spinand_base.c
> index 97d47146..1bc57e9 100644
> --- a/drivers/mtd/nand/spi/spinand_base.c
> +++ b/drivers/mtd/nand/spi/spinand_base.c
> @@ -25,6 +25,31 @@
>  #include <linux/slab.h>
>  
>  
> +static int spinand_erase(struct mtd_info *mtd, struct erase_info *einfo);
> +
> +/**
> + * spinand_get_device - [GENERIC] Get chip for selected access
> + * @mtd: MTD device structure
> + * @new_state: the state which is requested
> + *
> + * Get the device and lock it for exclusive access
> + */
> +static void spinand_get_device(struct spinand_device *chip)
> +{
> +	mutex_lock(&chip->lock);
> +}
> +
> +/**
> + * spinand_release_device - [GENERIC] release chip
> + * @mtd: MTD device structure
> + *
> + * Deselect, release chip lock and wake up anyone waiting on the device
> + */
> +static void spinand_release_device(struct spinand_device *chip)
> +{
> +	mutex_unlock(&chip->lock);
> +}
> +

Is there a good reason to provide those get/release helpers instead of
letting the callers directly take/release the lock?

>  static u64 spinand_get_chip_size(struct spinand_device *chip)
>  {
>  	struct nand_device *nand = &chip->base;
> @@ -115,6 +140,258 @@ 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
> + * Description:
> + *   There is one bit( bit 0x10 ) to set or to clear the internal ECC.
> + *   Enable chip internal ECC, set the bit to 1
> + *   Disable chip internal ECC, clear the bit to 0

Be careful, you tend to describe the register layout instead of
describing what the function does (true for all other comments). If you
want to describe the layout, do that in the header file just before the
according macro definitions.

> + */
> +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
> + * Description:
> + *   There is one bit( bit 0x10 ) to set or to clear the internal ECC.
> + *   Enable chip internal ECC, set the bit to 1
> + *   Disable chip internal ECC, clear the bit to 0
> + */
> +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

      ^ NAND

> + * @chip: SPI-NAND device structure
> + * Description:
> + *   Before write and erase the Nand cells, the write enable has to be set.
> + *   After the write or erase, the write enable bit is automatically
> + *   cleared (status register bit 2)
> + *   Set the bit 2 of the status register has the same effect

Again, this not really what I would expect from a function description.
This comment would be perfectly placed inside the code blocks calling
spinand_write_enable() though.

> + */
> +static int spinand_write_enable(struct spinand_device *chip)
> +{
> +	struct spinand_op cmd;
> +
> +	spinand_op_init(&cmd);
> +	cmd.cmd = SPINAND_CMD_WR_ENABLE;
> +
> +	return spinand_exec_cmd(chip, &cmd);
> +}
> +
> +/**
> + * spinand_read_page_to_cache - send command 13h to read data from Nand
> + * 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 cmd;
> +
> +	spinand_op_init(&cmd);
> +	cmd.cmd = SPINAND_CMD_PAGE_READ;
> +	cmd.n_addr = 3;
> +	cmd.addr[0] = (u8)(page_addr >> 16);
> +	cmd.addr[1] = (u8)(page_addr >> 8);
> +	cmd.addr[2] = (u8)page_addr;
> +
> +	return spinand_exec_cmd(chip, &cmd);
> +}
> +
> +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;
> +	}
> +}
> +
> +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
> + * Description:
> + *   Command can be 03h, 0Bh, 3Bh, 6Bh, BBh, EBh
> + *   The read can specify 1 to (page size + spare size) bytes of data read at
> + *   the corresponding locations.
> + *   No tRd delay.
> + */
> +static int spinand_read_from_cache(struct spinand_device *chip,
> +		u32 page_addr, u32 column, size_t len, u8 *rbuf)
> +{
> +	struct spinand_op cmd;
> +
> +	spinand_op_init(&cmd);
> +	cmd.cmd = chip->read_cache_op;
> +	if (chip->manufacturer.ops->build_column_addr) {
> +		chip->manufacturer.ops->build_column_addr(chip, &cmd,
> +						          page_addr, column);
> +	} else {
> +		cmd.n_addr = 2;
> +		cmd.addr[0] = (u8)(column >> 8);
> +		cmd.addr[1] = (u8)column;
> +	}
> +	if (chip->manufacturer.ops->get_dummy)
> +		cmd.dummy_bytes = chip->manufacturer.ops->get_dummy(chip, &cmd);
> +	cmd.addr_nbits = spinand_get_address_bits(chip->read_cache_op);
> +	cmd.n_rx = len;
> +	cmd.rx_buf = rbuf;
> +	cmd.data_nbits = spinand_get_data_bits(chip->read_cache_op);
> +
> +	return spinand_exec_cmd(chip, &cmd);
> +}
> +
> +/**
> + * spinand_program_data_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
> + * @clr_cache: clear cache register or not
> + * Description:
> + *   Command can be 02h, 32h, 84h, 34h
> + *   02h and 32h will clear the cache with 0xff value first
> + *   Since it is writing the data to cache, there is no tPROG time.
> + */
> +static int spinand_program_data_to_cache(struct spinand_device *chip,
> +			u32 page_addr, u32 column, size_t len, const u8 *wbuf)
> +{
> +	struct spinand_op cmd;
> +
> +	spinand_op_init(&cmd);
> +	cmd.cmd = chip->write_cache_op;
> +	if (chip->manufacturer.ops->build_column_addr) {
> +		chip->manufacturer.ops->build_column_addr(chip, &cmd,
> +						          page_addr, column);
> +	} else {
> +		cmd.n_addr = 2;
> +		cmd.addr[0] = (u8)(column >> 8);
> +		cmd.addr[1] = (u8)column;
> +	}
> +	cmd.addr_nbits = spinand_get_address_bits(chip->write_cache_op);
> +	cmd.n_tx = len;
> +	cmd.tx_buf = wbuf;
> +	cmd.data_nbits = spinand_get_data_bits(chip->write_cache_op);
> +
> +	return spinand_exec_cmd(chip, &cmd);
> +}
> +
> +/**
> + * 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.
> + * Description:
> + *   Need to wait for tPROG time to finish the transaction.

I see that you mention those tXX timings in different places. Maybe
it's worth having a spinand_timings struct. And it seems that the
controller/core is supposed to wait after executing some commands.
Can we extend struct spinand_op to pass this information and let the
controller wait for this time (or use it as a timeout?).
Something like, cmd.wait_ns.

> + */
> +static int spinand_program_execute(struct spinand_device *chip, u32 page_addr)
> +{
> +	struct spinand_op cmd;
> +
> +	spinand_op_init(&cmd);
> +	cmd.cmd = SPINAND_CMD_PROG_EXC;
> +	cmd.n_addr = 3;
> +	cmd.addr[0] = (u8)(page_addr >> 16);
> +	cmd.addr[1] = (u8)(page_addr >> 8);
> +	cmd.addr[2] = (u8)page_addr;
> +
> +	return spinand_exec_cmd(chip, &cmd);
> +}
> +
> +
> +/**
> + * spinand_erase_block_erase - send command D8h to erase a block
> + * @chip: SPI-NAND device structure
> + * @page_addr: the page to erase.
> + * Description:
> + *   Need to wait for tERS.
> + */
> +static int spinand_erase_block(struct spinand_device *chip,
> +					u32 page_addr)
> +{
> +	struct spinand_op cmd;
> +
> +	spinand_op_init(&cmd);
> +	cmd.cmd = SPINAND_CMD_BLK_ERASE;
> +	cmd.n_addr = 3;
> +	cmd.addr[0] = (u8)(page_addr >> 16);
> +	cmd.addr[1] = (u8)(page_addr >> 8);
> +	cmd.addr[2] = (u8)page_addr;
> +
> +	return spinand_exec_cmd(chip, &cmd);
> +}
> +
> +/**
>   * spinand_wait - wait until the command is done
>   * @chip: SPI-NAND device structure
>   * @s: buffer to store status register(can be NULL)
> @@ -197,6 +474,611 @@ static int spinand_lock_block(struct spinand_device *chip, u8 lock)
>  	return spinand_write_reg(chip, REG_BLOCK_LOCK, &lock);
>  }
>  

Missing comment header.

> +static void spinand_get_ecc_status(struct spinand_device *chip,
> +	unsigned int status, unsigned int *corrected, unsigned int *ecc_errors)
> +{
> +	return chip->ecc_engine.ops->get_ecc_status(chip, status, corrected, ecc_errors);
> +}
> +
> +/**
> + * spinand_do_read_page - read page from flash to buffer
> + * @mtd: MTD device structure
> + * @page_addr: page address/raw address
> + * @column: column address
> + * @ecc_off: without ecc or not
> + * @corrected: how many bit error corrected
> + * @buf: data buffer
> + * @len: data length to read
> + */
> +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 flash
> + * @mtd: MTD device structure
> + * @page_addr: page address/raw address
> + * @column: column address
> + * @buf: data buffer
> + * @len: data length to write
> + * @clr_cache: clear cache register with 0xFF or not
> + */
> +static int spinand_do_write_page(struct mtd_info *mtd, u32 page_addr,
> +						bool oob_only)

Parameter is not aligned to the open parenthesis (and this is not the
only place showing this problem).

> +{
> +	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_program_data_to_cache(chip, page_addr, 0,
> +		nand_page_size(nand) + nand_per_page_oobsize(nand), chip->buf);
> +	else
> +		spinand_program_data_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 flash to buffer
> + * @mtd: MTD device structure
> + * @from: offset to read from
> + * @ops: oob operations description structure
> + * @max_bitflips: maximum bitflip count
> + * Description:
> + *   Normal read function, read one page to buffer before issue
> + *   another.
> + */
> +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 page_addr, page_offset, size, ret;
> +	unsigned int corrected = 0;
> +	int readlen = ops->len;
> +	int oobreadlen = ops->ooblen;
> +	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 == NULL;
> +
> +	page_addr = nand_offs_to_page(nand, from);
> +	page_offset = from & (nand_page_size(nand) - 1);
> +	ops->retlen = 0;
> +	*max_bitflips = 0;
> +
> +	while (1) {
> +		ret = spinand_do_read_page(mtd, page_addr, ecc_off,
> +						&corrected, oob_only);
> +		if (ret)
> +			break;
> +		*max_bitflips = max(*max_bitflips, corrected);
> +		if (ops->datbuf) {
> +			size = min_t(int, readlen, nand_page_size(nand) - page_offset);
> +			memcpy(ops->datbuf + ops->retlen,
> +				chip->buf + page_offset, size);
> +			ops->retlen += size;
> +			readlen -= size;
> +			page_offset = 0;
> +			if (!readlen)
> +				break;
> +		}
> +		if (ops->oobbuf) {
> +			size = min(oobreadlen, 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;
> +			oobreadlen -= size;
> +			if (!oobreadlen)
> +				break;
> +		}
> +		page_addr++;
> +	}
> +
> +	return ret;
> +}

Hm, it seems you need pretty much the same logic for write_pages(). How
about creating a nand_for_each_page(nand, start, len, page) helper?

#define nand_for_each_page(nand, from, len, page, &poffs)	 \
	for (page = nand_offs_to_page(nand, from, &poffs),	 \
	     page = nand_offs_to_page(nand, from + len, &poffs); \
	     page++, poffs = 0)

Assuming we change the prototype of nand_offs_to_page() to return the
page offset in the 3 parameter (poffs).

> +
> +/**
> + * spinand_do_read_ops - read data from flash to buffer
> + * @mtd: MTD device structure
> + * @from: offset to read from
> + * @ops: oob ops structure
> + * Description:
> + *   Disable internal ECC before reading when MTD_OPS_RAW set.
> + */
> +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;
> +	int oobreadlen = ops->ooblen;
> +	bool ecc_off = ops->mode == MTD_OPS_RAW;
> +	int ooblen = ops->mode == MTD_OPS_AUTO_OOB ?
> +		mtd->oobavail : mtd->oobsize;
> +
> +	if (unlikely(from >= mtd->size)) {
> +		pr_err("%s: attempt to read beyond end of device\n",
> +				__func__);
> +		return -EINVAL;
> +	}

Again, we can an helper to check the boundaries in the generic NAND
code.

> +	if (oobreadlen > 0) {
> +		if (unlikely(ops->ooboffs >= ooblen)) {
> +			pr_err("%s: attempt to start read outside oob\n",
> +					__func__);
> +			return -EINVAL;
> +		}
> +		if (unlikely(ops->ooboffs + oobreadlen >
> +		(nand_len_to_pages(nand, mtd->size) - nand_offs_to_page(nand, from))
> +		* ooblen)) {
> +			pr_err("%s: attempt to read beyond end of device\n",
> +					__func__);
> +			return -EINVAL;
> +		}
> +		ooblen -= ops->ooboffs;
> +		ops->oobretlen = 0;
> +	}
> +	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)
> +		return ret;
> +
> +	if (mtd->ecc_stats.failed - stats.failed)
> +		return -EBADMSG;
> +
> +	return max_bitflips;
> +}
> +

[...]

> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> index f3d0351..ee447c1 100644
> --- a/include/linux/mtd/spinand.h
> +++ b/include/linux/mtd/spinand.h
> @@ -135,6 +135,8 @@ struct spinand_manufacturer_ops {
>  	bool(*detect)(struct spinand_device *chip);
>  	int (*init)(struct spinand_device *chip);
>  	void (*cleanup)(struct spinand_device *chip);
> +	void (*build_column_addr)(struct spinand_device *chip,
> +				  struct spinand_op *op, u32 page, u32 column);

Okay, I think Arnaud was right, maybe we should have vendor specific
ops for basic operations like ->prepare_read/write_op(), instead of
having these ->get_dummy() and ->build_column_addr() hooks.
Or maybe just a ->prepare_op() hook that can prepare things for any
basic operation (read, write, ...).

>  	int (*get_dummy)(struct spinand_device *chip, struct spinand_op *op);
>  };
>  

Re: [PATCH v2 3/6] nand: spi: Add bad block support

[Boris Brezillon]

On Wed, 1 Mar 2017 16:52:07 +0800
Peter Pan <peterpandong@micron.com> wrote:

> Add isbad and markbad support for SPI NAND. And do not
> erase bad blocks in spi_nand_erase.

Hm, can you put patch 4 before patch 3 (or maybe merge the 2 patches?).

> 
> Signed-off-by: Peter Pan <peterpandong@micron.com>
> ---
>  drivers/mtd/nand/spi/spinand_base.c | 116 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 116 insertions(+)
> 
> diff --git a/drivers/mtd/nand/spi/spinand_base.c b/drivers/mtd/nand/spi/spinand_base.c
> index 1bc57e9..442997d 100644
> --- a/drivers/mtd/nand/spi/spinand_base.c
> +++ b/drivers/mtd/nand/spi/spinand_base.c
> @@ -1003,6 +1003,113 @@ static int spinand_write_oob(struct mtd_info *mtd, loff_t to,
>  	return ret;
>  }
>  
> +/**
> + * spinand_block_bad - Check if block at offset is bad
> + * @mtd: MTD device structure
> + * @offs: offset relative to mtd start
> + * @getchip: 0, if the chip is already selected
> + */
> +static int spinand_block_bad(struct mtd_info *mtd, loff_t ofs, int getchip)

Please don't drop the getchip parameter and let the caller take the
spi NAND lock. If you want to make it clear that the function should be
called with the lock held, you can rename it spinand_block_bad_locked()
and/or document this expectation in the function doc header.

> +{
> +	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;
> +
> +	block_addr = nand_offs_to_eraseblock(nand, ofs);
> +	ops.mode = MTD_OPS_PLACE_OOB;
> +	ops.ooblen = 2;
> +	ops.oobbuf = bad;
> +
> +	if (getchip)
> +		spinand_get_device(mtd_to_spinand(mtd));
> +	spinand_do_read_ops(mtd, nand_eraseblock_to_offs(nand, block_addr), &ops);
> +	if (getchip)
> +		spinand_release_device(mtd_to_spinand(mtd));
> +	if (bad[0] != 0xFF || bad[1] != 0xFF)
> +		ret =  1;
> +
> +	return ret;
> +}

Re: [PATCH v2 3/6] nand: spi: Add bad block support

[Peter Pan]

Hi Boris,

On Wed, Mar 8, 2017 at 3:23 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Wed, 1 Mar 2017 16:52:07 +0800
> Peter Pan <peterpandong@micron.com> wrote:
>
>> Add isbad and markbad support for SPI NAND. And do not
>> erase bad blocks in spi_nand_erase.
>
> Hm, can you put patch 4 before patch 3 (or maybe merge the 2 patches?).

Let's merge these two patches.

>
>>
>> Signed-off-by: Peter Pan <peterpandong@micron.com>
>> ---
>>  drivers/mtd/nand/spi/spinand_base.c | 116 ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 116 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/spi/spinand_base.c b/drivers/mtd/nand/spi/spinand_base.c
>> index 1bc57e9..442997d 100644
>> --- a/drivers/mtd/nand/spi/spinand_base.c
>> +++ b/drivers/mtd/nand/spi/spinand_base.c
>> @@ -1003,6 +1003,113 @@ static int spinand_write_oob(struct mtd_info *mtd, loff_t to,
>>       return ret;
>>  }
>>
>> +/**
>> + * spinand_block_bad - Check if block at offset is bad
>> + * @mtd: MTD device structure
>> + * @offs: offset relative to mtd start
>> + * @getchip: 0, if the chip is already selected
>> + */
>> +static int spinand_block_bad(struct mtd_info *mtd, loff_t ofs, int getchip)
>
> Please don't drop the getchip parameter and let the caller take the
> spi NAND lock. If you want to make it clear that the function should be
> called with the lock held, you can rename it spinand_block_bad_locked()
> and/or document this expectation in the function doc header.

Noted. Fix it in v3

>
>> +{
>> +     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;
>> +
>> +     block_addr = nand_offs_to_eraseblock(nand, ofs);
>> +     ops.mode = MTD_OPS_PLACE_OOB;
>> +     ops.ooblen = 2;
>> +     ops.oobbuf = bad;
>> +
>> +     if (getchip)
>> +             spinand_get_device(mtd_to_spinand(mtd));
>> +     spinand_do_read_ops(mtd, nand_eraseblock_to_offs(nand, block_addr), &ops);
>> +     if (getchip)
>> +             spinand_release_device(mtd_to_spinand(mtd));
>> +     if (bad[0] != 0xFF || bad[1] != 0xFF)
>> +             ret =  1;
>> +
>> +     return ret;
>> +}

Re: [PATCH v2 4/6] nand: spi: Add BBT support

[Boris Brezillon]

On Wed, 1 Mar 2017 16:52:08 +0800
Peter Pan <peterpandong@micron.com> wrote:

>  /**
> - * spinand_erase - [MTD Interface] erase block(s)
> + * __spinand_erase - erase block(s)
>   * @mtd: MTD device structure
>   * @einfo: erase instruction
> + * @allowbbt: allow to access bbt
>   *
>   * Erase one ore more blocks
>   */
> -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)

Please no __ prefixes. Not sure you need the wrappers you define below
BTW.

>  {
>  	struct spinand_device *chip = mtd_to_spinand(mtd);
>  	struct nand_device *nand = mtd_to_nand(mtd);
> @@ -1152,7 +1185,7 @@ static int spinand_erase(struct mtd_info *mtd, struct erase_info *einfo)
>  
>  	while (len) {
>  		/* Check if we have a bad block, we do not erase bad blocks! */
> -		if (spinand_block_bad(mtd, offs, 0)) {
> +		if (spinand_block_checkbad(mtd, offs, 0, allowbbt)) {
>  			pr_warn("%s: attempt to erase a bad block at 0x%012llx\n",
>  			__func__, offs);
>  			einfo->state = MTD_ERASE_FAILED;
> @@ -1193,6 +1226,55 @@ static int spinand_erase(struct mtd_info *mtd, struct erase_info *einfo)
>  	/* Return more or less happy */
>  	return ret;
>  }
> +
> +/**
> + * spinand_erase - [MTD Interface] erase block(s)
> + * @mtd: MTD device structure
> + * @einfo: erase instruction
> + *
> + * Erase one ore more blocks
> + */
> +static int spinand_erase(struct mtd_info *mtd, struct erase_info *einfo)
> +{
> +	return __spinand_erase(mtd, einfo, 0);
> +}
> +
> +
> +static int spinand_erase_bbt(struct nand_device *nand, struct erase_info *einfo)
> +{
> +	return __spinand_erase(nand_to_mtd(nand), einfo, 1);
> +}
> +

[...]


> +static void spinand_fill_nandd(struct spinand_device *chip)
> +{
> +	struct nand_device *nand = &chip->base;
> +
> +	nand->ops = &spinand_ops;
> +}

This is not needed. I did that in the raw NAND framework to ease the
transition to the generic NAND layer, but here, you're
natively supporting it. Just assign nand->ops in spinand_scan_ident().

Re: [PATCH v2 5/6] nand: spi: add Micron spi nand support

[Boris Brezillon]

On Wed, 1 Mar 2017 16:52:09 +0800
Peter Pan <peterpandong@micron.com> wrote:

> 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/spinand_ids.c    |   5 ++
>  drivers/mtd/nand/spi/spinand_micron.c | 133 ++++++++++++++++++++++++++++++++++

micron.c should be enough.

Re: [PATCH v2 6/6] nand: spi: Add generic SPI controller support

[Boris Brezillon]

On Wed, 1 Mar 2017 16:52:10 +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/chips/Kconfig       |   5 +
>  drivers/mtd/nand/spi/chips/Makefile      |   1 +
>  drivers/mtd/nand/spi/chips/generic_spi.c | 158 +++++++++++++++++++++++++++++++

Hm, I would name the directory controllers and not chips. What could go
into chips are all the vendor specific drivers.

>  5 files changed, 167 insertions(+)
>  create mode 100644 drivers/mtd/nand/spi/chips/Kconfig
>  create mode 100644 drivers/mtd/nand/spi/chips/Makefile
>  create mode 100644 drivers/mtd/nand/spi/chips/generic_spi.c

Just controllers/generic.c?

Re: [PATCH v2 2/6] nand: spi: add basic operations support

[Peter Pan]

Hi Boris,

On Wed, Mar 8, 2017 at 1:57 AM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> Hi Peter,
>
> I have a few comments (see below).

Thanks for your valuable comments at first.

>
> On Wed, 1 Mar 2017 16:52:06 +0800
> Peter Pan <peterpandong@micron.com> wrote:
>
>> This commit is to support read, readoob, write,
>> writeoob and erase operations in spi nand framwork.
>>
>> Signed-off-by: Peter Pan <peterpandong@micron.com>
>> ---
>>  drivers/mtd/nand/spi/spinand_base.c | 887 ++++++++++++++++++++++++++++++++++++
>>  include/linux/mtd/spinand.h         |   2 +
>>  2 files changed, 889 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/spi/spinand_base.c b/drivers/mtd/nand/spi/spinand_base.c
>> index 97d47146..1bc57e9 100644
>> --- a/drivers/mtd/nand/spi/spinand_base.c
>> +++ b/drivers/mtd/nand/spi/spinand_base.c
>> @@ -25,6 +25,31 @@
>>  #include <linux/slab.h>
>>
>>
>> +static int spinand_erase(struct mtd_info *mtd, struct erase_info *einfo);
>> +
>> +/**
>> + * spinand_get_device - [GENERIC] Get chip for selected access
>> + * @mtd: MTD device structure
>> + * @new_state: the state which is requested
>> + *
>> + * Get the device and lock it for exclusive access
>> + */
>> +static void spinand_get_device(struct spinand_device *chip)
>> +{
>> +     mutex_lock(&chip->lock);
>> +}
>> +
>> +/**
>> + * spinand_release_device - [GENERIC] release chip
>> + * @mtd: MTD device structure
>> + *
>> + * Deselect, release chip lock and wake up anyone waiting on the device
>> + */
>> +static void spinand_release_device(struct spinand_device *chip)
>> +{
>> +     mutex_unlock(&chip->lock);
>> +}
>> +
>
> Is there a good reason to provide those get/release helpers instead of
> letting the callers directly take/release the lock?

Just in case we need more than a mutex lock to protect in the feature. Of course
we can let the callers directly take/release the lock. Let me fix this in v3.

>
>>  static u64 spinand_get_chip_size(struct spinand_device *chip)
>>  {
>>       struct nand_device *nand = &chip->base;
>> @@ -115,6 +140,258 @@ 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
>> + * Description:
>> + *   There is one bit( bit 0x10 ) to set or to clear the internal ECC.
>> + *   Enable chip internal ECC, set the bit to 1
>> + *   Disable chip internal ECC, clear the bit to 0
>
> Be careful, you tend to describe the register layout instead of
> describing what the function does (true for all other comments). If you
> want to describe the layout, do that in the header file just before the
> according macro definitions.

Actually I think we can just remove these register layout info.

>
>> + */
>> +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
>> + * Description:
>> + *   There is one bit( bit 0x10 ) to set or to clear the internal ECC.
>> + *   Enable chip internal ECC, set the bit to 1
>> + *   Disable chip internal ECC, clear the bit to 0
>> + */
>> +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
>
>       ^ NAND

Fix this in v3.

>
>> + * @chip: SPI-NAND device structure
>> + * Description:
>> + *   Before write and erase the Nand cells, the write enable has to be set.
>> + *   After the write or erase, the write enable bit is automatically
>> + *   cleared (status register bit 2)
>> + *   Set the bit 2 of the status register has the same effect
>
> Again, this not really what I would expect from a function description.
> This comment would be perfectly placed inside the code blocks calling
> spinand_write_enable() though.

Fix this in v3.

>
>> + */
>> +static int spinand_write_enable(struct spinand_device *chip)
>> +{
>> +     struct spinand_op cmd;
>> +
>> +     spinand_op_init(&cmd);
>> +     cmd.cmd = SPINAND_CMD_WR_ENABLE;
>> +
>> +     return spinand_exec_cmd(chip, &cmd);
>> +}
>> +
>> +/**
>> + * spinand_read_page_to_cache - send command 13h to read data from Nand
>> + * 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 cmd;
>> +
>> +     spinand_op_init(&cmd);
>> +     cmd.cmd = SPINAND_CMD_PAGE_READ;
>> +     cmd.n_addr = 3;
>> +     cmd.addr[0] = (u8)(page_addr >> 16);
>> +     cmd.addr[1] = (u8)(page_addr >> 8);
>> +     cmd.addr[2] = (u8)page_addr;
>> +
>> +     return spinand_exec_cmd(chip, &cmd);
>> +}
>> +
>> +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;
>> +     }
>> +}
>> +
>> +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
>> + * Description:
>> + *   Command can be 03h, 0Bh, 3Bh, 6Bh, BBh, EBh
>> + *   The read can specify 1 to (page size + spare size) bytes of data read at
>> + *   the corresponding locations.
>> + *   No tRd delay.
>> + */
>> +static int spinand_read_from_cache(struct spinand_device *chip,
>> +             u32 page_addr, u32 column, size_t len, u8 *rbuf)
>> +{
>> +     struct spinand_op cmd;
>> +
>> +     spinand_op_init(&cmd);
>> +     cmd.cmd = chip->read_cache_op;
>> +     if (chip->manufacturer.ops->build_column_addr) {
>> +             chip->manufacturer.ops->build_column_addr(chip, &cmd,
>> +                                                       page_addr, column);
>> +     } else {
>> +             cmd.n_addr = 2;
>> +             cmd.addr[0] = (u8)(column >> 8);
>> +             cmd.addr[1] = (u8)column;
>> +     }
>> +     if (chip->manufacturer.ops->get_dummy)
>> +             cmd.dummy_bytes = chip->manufacturer.ops->get_dummy(chip, &cmd);
>> +     cmd.addr_nbits = spinand_get_address_bits(chip->read_cache_op);
>> +     cmd.n_rx = len;
>> +     cmd.rx_buf = rbuf;
>> +     cmd.data_nbits = spinand_get_data_bits(chip->read_cache_op);
>> +
>> +     return spinand_exec_cmd(chip, &cmd);
>> +}
>> +
>> +/**
>> + * spinand_program_data_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
>> + * @clr_cache: clear cache register or not
>> + * Description:
>> + *   Command can be 02h, 32h, 84h, 34h
>> + *   02h and 32h will clear the cache with 0xff value first
>> + *   Since it is writing the data to cache, there is no tPROG time.
>> + */
>> +static int spinand_program_data_to_cache(struct spinand_device *chip,
>> +                     u32 page_addr, u32 column, size_t len, const u8 *wbuf)
>> +{
>> +     struct spinand_op cmd;
>> +
>> +     spinand_op_init(&cmd);
>> +     cmd.cmd = chip->write_cache_op;
>> +     if (chip->manufacturer.ops->build_column_addr) {
>> +             chip->manufacturer.ops->build_column_addr(chip, &cmd,
>> +                                                       page_addr, column);
>> +     } else {
>> +             cmd.n_addr = 2;
>> +             cmd.addr[0] = (u8)(column >> 8);
>> +             cmd.addr[1] = (u8)column;
>> +     }
>> +     cmd.addr_nbits = spinand_get_address_bits(chip->write_cache_op);
>> +     cmd.n_tx = len;
>> +     cmd.tx_buf = wbuf;
>> +     cmd.data_nbits = spinand_get_data_bits(chip->write_cache_op);
>> +
>> +     return spinand_exec_cmd(chip, &cmd);
>> +}
>> +
>> +/**
>> + * 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.
>> + * Description:
>> + *   Need to wait for tPROG time to finish the transaction.
>
> I see that you mention those tXX timings in different places. Maybe
> it's worth having a spinand_timings struct. And it seems that the
> controller/core is supposed to wait after executing some commands.
> Can we extend struct spinand_op to pass this information and let the
> controller wait for this time (or use it as a timeout?).
> Something like, cmd.wait_ns.

Actually these tXX timing value are not used by the code right now. Now
the code uses spinand_wait() to poll OIP bit instead. We can remove tXX
from comments.

>
>> + */
>> +static int spinand_program_execute(struct spinand_device *chip, u32 page_addr)
>> +{
>> +     struct spinand_op cmd;
>> +
>> +     spinand_op_init(&cmd);
>> +     cmd.cmd = SPINAND_CMD_PROG_EXC;
>> +     cmd.n_addr = 3;
>> +     cmd.addr[0] = (u8)(page_addr >> 16);
>> +     cmd.addr[1] = (u8)(page_addr >> 8);
>> +     cmd.addr[2] = (u8)page_addr;
>> +
>> +     return spinand_exec_cmd(chip, &cmd);
>> +}
>> +
>> +
>> +/**
>> + * spinand_erase_block_erase - send command D8h to erase a block
>> + * @chip: SPI-NAND device structure
>> + * @page_addr: the page to erase.
>> + * Description:
>> + *   Need to wait for tERS.
>> + */
>> +static int spinand_erase_block(struct spinand_device *chip,
>> +                                     u32 page_addr)
>> +{
>> +     struct spinand_op cmd;
>> +
>> +     spinand_op_init(&cmd);
>> +     cmd.cmd = SPINAND_CMD_BLK_ERASE;
>> +     cmd.n_addr = 3;
>> +     cmd.addr[0] = (u8)(page_addr >> 16);
>> +     cmd.addr[1] = (u8)(page_addr >> 8);
>> +     cmd.addr[2] = (u8)page_addr;
>> +
>> +     return spinand_exec_cmd(chip, &cmd);
>> +}
>> +
>> +/**
>>   * spinand_wait - wait until the command is done
>>   * @chip: SPI-NAND device structure
>>   * @s: buffer to store status register(can be NULL)
>> @@ -197,6 +474,611 @@ static int spinand_lock_block(struct spinand_device *chip, u8 lock)
>>       return spinand_write_reg(chip, REG_BLOCK_LOCK, &lock);
>>  }
>>
>
> Missing comment header.

Fix this in v3.

>
>> +static void spinand_get_ecc_status(struct spinand_device *chip,
>> +     unsigned int status, unsigned int *corrected, unsigned int *ecc_errors)
>> +{
>> +     return chip->ecc_engine.ops->get_ecc_status(chip, status, corrected, ecc_errors);
>> +}
>> +
>> +/**
>> + * spinand_do_read_page - read page from flash to buffer
>> + * @mtd: MTD device structure
>> + * @page_addr: page address/raw address
>> + * @column: column address
>> + * @ecc_off: without ecc or not
>> + * @corrected: how many bit error corrected
>> + * @buf: data buffer
>> + * @len: data length to read
>> + */
>> +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 flash
>> + * @mtd: MTD device structure
>> + * @page_addr: page address/raw address
>> + * @column: column address
>> + * @buf: data buffer
>> + * @len: data length to write
>> + * @clr_cache: clear cache register with 0xFF or not
>> + */
>> +static int spinand_do_write_page(struct mtd_info *mtd, u32 page_addr,
>> +                                             bool oob_only)
>
> Parameter is not aligned to the open parenthesis (and this is not the
> only place showing this problem).

Fix this in v3.

>
>> +{
>> +     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_program_data_to_cache(chip, page_addr, 0,
>> +             nand_page_size(nand) + nand_per_page_oobsize(nand), chip->buf);
>> +     else
>> +             spinand_program_data_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 flash to buffer
>> + * @mtd: MTD device structure
>> + * @from: offset to read from
>> + * @ops: oob operations description structure
>> + * @max_bitflips: maximum bitflip count
>> + * Description:
>> + *   Normal read function, read one page to buffer before issue
>> + *   another.
>> + */
>> +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 page_addr, page_offset, size, ret;
>> +     unsigned int corrected = 0;
>> +     int readlen = ops->len;
>> +     int oobreadlen = ops->ooblen;
>> +     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 == NULL;
>> +
>> +     page_addr = nand_offs_to_page(nand, from);
>> +     page_offset = from & (nand_page_size(nand) - 1);
>> +     ops->retlen = 0;
>> +     *max_bitflips = 0;
>> +
>> +     while (1) {
>> +             ret = spinand_do_read_page(mtd, page_addr, ecc_off,
>> +                                             &corrected, oob_only);
>> +             if (ret)
>> +                     break;
>> +             *max_bitflips = max(*max_bitflips, corrected);
>> +             if (ops->datbuf) {
>> +                     size = min_t(int, readlen, nand_page_size(nand) - page_offset);
>> +                     memcpy(ops->datbuf + ops->retlen,
>> +                             chip->buf + page_offset, size);
>> +                     ops->retlen += size;
>> +                     readlen -= size;
>> +                     page_offset = 0;
>> +                     if (!readlen)
>> +                             break;
>> +             }
>> +             if (ops->oobbuf) {
>> +                     size = min(oobreadlen, 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;
>> +                     oobreadlen -= size;
>> +                     if (!oobreadlen)
>> +                             break;
>> +             }
>> +             page_addr++;
>> +     }
>> +
>> +     return ret;
>> +}
>
> Hm, it seems you need pretty much the same logic for write_pages(). How
> about creating a nand_for_each_page(nand, start, len, page) helper?
>
> #define nand_for_each_page(nand, from, len, page, &poffs)        \
>         for (page = nand_offs_to_page(nand, from, &poffs),       \
>              page = nand_offs_to_page(nand, from + len, &poffs); \
>              page++, poffs = 0)
>
> Assuming we change the prototype of nand_offs_to_page() to return the
> page offset in the 3 parameter (poffs).

Much better idea! Fix this in v3.

>
>> +
>> +/**
>> + * spinand_do_read_ops - read data from flash to buffer
>> + * @mtd: MTD device structure
>> + * @from: offset to read from
>> + * @ops: oob ops structure
>> + * Description:
>> + *   Disable internal ECC before reading when MTD_OPS_RAW set.
>> + */
>> +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;
>> +     int oobreadlen = ops->ooblen;
>> +     bool ecc_off = ops->mode == MTD_OPS_RAW;
>> +     int ooblen = ops->mode == MTD_OPS_AUTO_OOB ?
>> +             mtd->oobavail : mtd->oobsize;
>> +
>> +     if (unlikely(from >= mtd->size)) {
>> +             pr_err("%s: attempt to read beyond end of device\n",
>> +                             __func__);
>> +             return -EINVAL;
>> +     }
>
> Again, we can an helper to check the boundaries in the generic NAND
> code.

Let's keep the code the same and move to generic NAND code later.
What do you think Boris?

>
>> +     if (oobreadlen > 0) {
>> +             if (unlikely(ops->ooboffs >= ooblen)) {
>> +                     pr_err("%s: attempt to start read outside oob\n",
>> +                                     __func__);
>> +                     return -EINVAL;
>> +             }
>> +             if (unlikely(ops->ooboffs + oobreadlen >
>> +             (nand_len_to_pages(nand, mtd->size) - nand_offs_to_page(nand, from))
>> +             * ooblen)) {
>> +                     pr_err("%s: attempt to read beyond end of device\n",
>> +                                     __func__);
>> +                     return -EINVAL;
>> +             }
>> +             ooblen -= ops->ooboffs;
>> +             ops->oobretlen = 0;
>> +     }
>> +     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)
>> +             return ret;
>> +
>> +     if (mtd->ecc_stats.failed - stats.failed)
>> +             return -EBADMSG;
>> +
>> +     return max_bitflips;
>> +}
>> +
>
> [...]
>
>> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
>> index f3d0351..ee447c1 100644
>> --- a/include/linux/mtd/spinand.h
>> +++ b/include/linux/mtd/spinand.h
>> @@ -135,6 +135,8 @@ struct spinand_manufacturer_ops {
>>       bool(*detect)(struct spinand_device *chip);
>>       int (*init)(struct spinand_device *chip);
>>       void (*cleanup)(struct spinand_device *chip);
>> +     void (*build_column_addr)(struct spinand_device *chip,
>> +                               struct spinand_op *op, u32 page, u32 column);
>
> Okay, I think Arnaud was right, maybe we should have vendor specific
> ops for basic operations like ->prepare_read/write_op(), instead of
> having these ->get_dummy() and ->build_column_addr() hooks.
> Or maybe just a ->prepare_op() hook that can prepare things for any
> basic operation (read, write, ...).

I prefer ->prepare_read_op() and ->prepare_write_op. Fix this in v3


Thanks
Peter Pan

Re: [PATCH v2 4/6] nand: spi: Add BBT support

[Peter Pan]

Hi Boris,

On Wed, Mar 8, 2017 at 4:31 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Wed, 1 Mar 2017 16:52:08 +0800
> Peter Pan <peterpandong@micron.com> wrote:
>
>>  /**
>> - * spinand_erase - [MTD Interface] erase block(s)
>> + * __spinand_erase - erase block(s)
>>   * @mtd: MTD device structure
>>   * @einfo: erase instruction
>> + * @allowbbt: allow to access bbt
>>   *
>>   * Erase one ore more blocks
>>   */
>> -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)
>
> Please no __ prefixes. Not sure you need the wrappers you define below
> BTW.

mtd->_erase needs a function to treat BBT block as bad block while
nand_ops->erase not. What's your suggestion if we don't use the wrappers?

>
>>  {
>>       struct spinand_device *chip = mtd_to_spinand(mtd);
>>       struct nand_device *nand = mtd_to_nand(mtd);
>> @@ -1152,7 +1185,7 @@ static int spinand_erase(struct mtd_info *mtd, struct erase_info *einfo)
>>
>>       while (len) {
>>               /* Check if we have a bad block, we do not erase bad blocks! */
>> -             if (spinand_block_bad(mtd, offs, 0)) {
>> +             if (spinand_block_checkbad(mtd, offs, 0, allowbbt)) {
>>                       pr_warn("%s: attempt to erase a bad block at 0x%012llx\n",
>>                       __func__, offs);
>>                       einfo->state = MTD_ERASE_FAILED;
>> @@ -1193,6 +1226,55 @@ static int spinand_erase(struct mtd_info *mtd, struct erase_info *einfo)
>>       /* Return more or less happy */
>>       return ret;
>>  }
>> +
>> +/**
>> + * spinand_erase - [MTD Interface] erase block(s)
>> + * @mtd: MTD device structure
>> + * @einfo: erase instruction
>> + *
>> + * Erase one ore more blocks
>> + */
>> +static int spinand_erase(struct mtd_info *mtd, struct erase_info *einfo)
>> +{
>> +     return __spinand_erase(mtd, einfo, 0);
>> +}
>> +
>> +
>> +static int spinand_erase_bbt(struct nand_device *nand, struct erase_info *einfo)
>> +{
>> +     return __spinand_erase(nand_to_mtd(nand), einfo, 1);
>> +}
>> +
>
> [...]
>
>
>> +static void spinand_fill_nandd(struct spinand_device *chip)
>> +{
>> +     struct nand_device *nand = &chip->base;
>> +
>> +     nand->ops = &spinand_ops;
>> +}
>
> This is not needed. I did that in the raw NAND framework to ease the
> transition to the generic NAND layer, but here, you're
> natively supporting it. Just assign nand->ops in spinand_scan_ident().

Noted/ Fix this in v3.

Thanks
Peter Pan

Re: [PATCH v2 5/6] nand: spi: add Micron spi nand support

[Peter Pan]

Hi Boris,

On Wed, Mar 8, 2017 at 4:32 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Wed, 1 Mar 2017 16:52:09 +0800
> Peter Pan <peterpandong@micron.com> wrote:
>
>> 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/spinand_ids.c    |   5 ++
>>  drivers/mtd/nand/spi/spinand_micron.c | 133 ++++++++++++++++++++++++++++++++++
>
> micron.c should be enough.

Noted. Fix this in v3

Thanks
Peter Pan

Re: [PATCH v2 6/6] nand: spi: Add generic SPI controller support

[Peter Pan]

Hi Boris,

On Wed, Mar 8, 2017 at 4:44 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Wed, 1 Mar 2017 16:52:10 +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/chips/Kconfig       |   5 +
>>  drivers/mtd/nand/spi/chips/Makefile      |   1 +
>>  drivers/mtd/nand/spi/chips/generic_spi.c | 158 +++++++++++++++++++++++++++++++
>
> Hm, I would name the directory controllers and not chips. What could go
> into chips are all the vendor specific drivers.

You are right. Should be controllers. Fix this in v3

>
>>  5 files changed, 167 insertions(+)
>>  create mode 100644 drivers/mtd/nand/spi/chips/Kconfig
>>  create mode 100644 drivers/mtd/nand/spi/chips/Makefile
>>  create mode 100644 drivers/mtd/nand/spi/chips/generic_spi.c
>
> Just controllers/generic.c?

generic.c or just spi.c?

Thanks,
Peter Pan

Re: [PATCH v2 2/6] nand: spi: add basic operations support

[Boris Brezillon]

On Thu, 9 Mar 2017 09:43:36 +0800
Peter Pan <peterpansjtu@gmail.com> wrote:

> >> +
> >> +/**
> >> + * spinand_do_read_ops - read data from flash to buffer
> >> + * @mtd: MTD device structure
> >> + * @from: offset to read from
> >> + * @ops: oob ops structure
> >> + * Description:
> >> + *   Disable internal ECC before reading when MTD_OPS_RAW set.
> >> + */
> >> +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;
> >> +     int oobreadlen = ops->ooblen;
> >> +     bool ecc_off = ops->mode == MTD_OPS_RAW;
> >> +     int ooblen = ops->mode == MTD_OPS_AUTO_OOB ?
> >> +             mtd->oobavail : mtd->oobsize;
> >> +
> >> +     if (unlikely(from >= mtd->size)) {
> >> +             pr_err("%s: attempt to read beyond end of device\n",
> >> +                             __func__);
> >> +             return -EINVAL;
> >> +     }  
> >
> > Again, we can an helper to check the boundaries in the generic NAND
> > code.  
> 
> Let's keep the code the same and move to generic NAND code later.
> What do you think Boris?

For things we know will be needed and are simple enough to provide, I'd
recommend doing the modifications now. That's a bit different when it
comes to providing a generic ECC engine infrastructure or trying to
move part of the page read/write logic into the generic NAND layer,
because we don't know where we're going yet.

> 
> >  
> >> +     if (oobreadlen > 0) {
> >> +             if (unlikely(ops->ooboffs >= ooblen)) {
> >> +                     pr_err("%s: attempt to start read outside oob\n",
> >> +                                     __func__);
> >> +                     return -EINVAL;
> >> +             }
> >> +             if (unlikely(ops->ooboffs + oobreadlen >
> >> +             (nand_len_to_pages(nand, mtd->size) - nand_offs_to_page(nand, from))
> >> +             * ooblen)) {
> >> +                     pr_err("%s: attempt to read beyond end of device\n",
> >> +                                     __func__);
> >> +                     return -EINVAL;
> >> +             }
> >> +             ooblen -= ops->ooboffs;
> >> +             ops->oobretlen = 0;
> >> +     }
> >> +     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)
> >> +             return ret;
> >> +
> >> +     if (mtd->ecc_stats.failed - stats.failed)
> >> +             return -EBADMSG;
> >> +
> >> +     return max_bitflips;
> >> +}
> >> +  
> >
> > [...]
> >  
> >> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> >> index f3d0351..ee447c1 100644
> >> --- a/include/linux/mtd/spinand.h
> >> +++ b/include/linux/mtd/spinand.h
> >> @@ -135,6 +135,8 @@ struct spinand_manufacturer_ops {
> >>       bool(*detect)(struct spinand_device *chip);
> >>       int (*init)(struct spinand_device *chip);
> >>       void (*cleanup)(struct spinand_device *chip);
> >> +     void (*build_column_addr)(struct spinand_device *chip,
> >> +                               struct spinand_op *op, u32 page, u32 column);  
> >
> > Okay, I think Arnaud was right, maybe we should have vendor specific
> > ops for basic operations like ->prepare_read/write_op(), instead of
> > having these ->get_dummy() and ->build_column_addr() hooks.
> > Or maybe just a ->prepare_op() hook that can prepare things for any
> > basic operation (read, write, ...).  
> 
> I prefer ->prepare_read_op() and ->prepare_write_op. Fix this in v3

I'd like to have Arnaud's feedback on this. Can you wait a bit before
sending a new version?

Re: [PATCH v2 6/6] nand: spi: Add generic SPI controller support

[Boris Brezillon]

On Thu, 9 Mar 2017 10:02:03 +0800
Peter Pan <peterpansjtu@gmail.com> wrote:

 
> >  
> >>  5 files changed, 167 insertions(+)
> >>  create mode 100644 drivers/mtd/nand/spi/chips/Kconfig
> >>  create mode 100644 drivers/mtd/nand/spi/chips/Makefile
> >>  create mode 100644 drivers/mtd/nand/spi/chips/generic_spi.c  
> >
> > Just controllers/generic.c?  
> 
> generic.c or just spi.c?

As you prefer. BTW, I prefer '-' to '_' in file names :-).
generic-spi.c is fine too, I'm just nitpicking. 

Re: [PATCH v2 4/6] nand: spi: Add BBT support

[Boris Brezillon]

On Thu, 9 Mar 2017 09:58:51 +0800
Peter Pan <peterpansjtu@gmail.com> wrote:

> Hi Boris,
> 
> On Wed, Mar 8, 2017 at 4:31 PM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > On Wed, 1 Mar 2017 16:52:08 +0800
> > Peter Pan <peterpandong@micron.com> wrote:
> >  
> >>  /**
> >> - * spinand_erase - [MTD Interface] erase block(s)
> >> + * __spinand_erase - erase block(s)
> >>   * @mtd: MTD device structure
> >>   * @einfo: erase instruction
> >> + * @allowbbt: allow to access bbt
> >>   *
> >>   * Erase one ore more blocks
> >>   */
> >> -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)  
> >
> > Please no __ prefixes. Not sure you need the wrappers you define below
> > BTW.  
> 
> mtd->_erase needs a function to treat BBT block as bad block while
> nand_ops->erase not. What's your suggestion if we don't use the wrappers?

Oh, I forgot about that. Then keep the wrapper, but find a better name
for this function.

> 
> >  
> >>  {
> >>       struct spinand_device *chip = mtd_to_spinand(mtd);
> >>       struct nand_device *nand = mtd_to_nand(mtd);
> >> @@ -1152,7 +1185,7 @@ static int spinand_erase(struct mtd_info *mtd, struct erase_info *einfo)
> >>
> >>       while (len) {
> >>               /* Check if we have a bad block, we do not erase bad blocks! */
> >> -             if (spinand_block_bad(mtd, offs, 0)) {
> >> +             if (spinand_block_checkbad(mtd, offs, 0, allowbbt)) {
> >>                       pr_warn("%s: attempt to erase a bad block at 0x%012llx\n",
> >>                       __func__, offs);
> >>                       einfo->state = MTD_ERASE_FAILED;
> >> @@ -1193,6 +1226,55 @@ static int spinand_erase(struct mtd_info *mtd, struct erase_info *einfo)
> >>       /* Return more or less happy */
> >>       return ret;
> >>  }
> >> +
> >> +/**
> >> + * spinand_erase - [MTD Interface] erase block(s)
> >> + * @mtd: MTD device structure
> >> + * @einfo: erase instruction
> >> + *
> >> + * Erase one ore more blocks
> >> + */
> >> +static int spinand_erase(struct mtd_info *mtd, struct erase_info *einfo)

Or maybe you can rename this function spinand_erase_skip_bbt(), and
keep spinand_erase() for the generic one.

> >> +{
> >> +     return __spinand_erase(mtd, einfo, 0);
> >> +}
> >> +
> >> +

Remove this extra empty line.

> >> +static int spinand_erase_bbt(struct nand_device *nand, struct erase_info *einfo)
> >> +{
> >> +     return __spinand_erase(nand_to_mtd(nand), einfo, 1);
> >> +}
> >> +  
> >

Re: [PATCH v2 2/6] nand: spi: add basic operations support

[Arnaud Mouiche]

On 09/03/2017 07:02, Boris Brezillon wrote:
> [...]
>>>   
>>>> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
>>>> index f3d0351..ee447c1 100644
>>>> --- a/include/linux/mtd/spinand.h
>>>> +++ b/include/linux/mtd/spinand.h
>>>> @@ -135,6 +135,8 @@ struct spinand_manufacturer_ops {
>>>>        bool(*detect)(struct spinand_device *chip);
>>>>        int (*init)(struct spinand_device *chip);
>>>>        void (*cleanup)(struct spinand_device *chip);
>>>> +     void (*build_column_addr)(struct spinand_device *chip,
>>>> +                               struct spinand_op *op, u32 page, u32 column);
>>> Okay, I think Arnaud was right, maybe we should have vendor specific
>>> ops for basic operations like ->prepare_read/write_op(), instead of
>>> having these ->get_dummy() and ->build_column_addr() hooks.
>>> Or maybe just a ->prepare_op() hook that can prepare things for any
>>> basic operation (read, write, ...).
>> I prefer ->prepare_read_op() and ->prepare_write_op. Fix this in v3
> I'd like to have Arnaud's feedback on this. Can you wait a bit before
> sending a new version?

->prepare_read_op() and ->prepare_write_op
looks fine to me.

FYI: I have prepared a imx6sl board which can run vanilla mtd/master kernels.
On this board, I can swap the various spinand samples I have:
- Micron (the one supported by those patches)
- winbond
- macronix
- ESMT

Looking forward to test v3 ;)
Arnaud

Re: [PATCH v2 2/6] nand: spi: add basic operations support

[Peter Pan]

On Fri, Mar 10, 2017 at 1:09 AM, Arnaud Mouiche
<arnaud.mouiche@gmail.com> wrote:
>
>
> On 09/03/2017 07:02, Boris Brezillon wrote:
>>
>> [...]
>>>>
>>>>
>>>>>
>>>>> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
>>>>> index f3d0351..ee447c1 100644
>>>>> --- a/include/linux/mtd/spinand.h
>>>>> +++ b/include/linux/mtd/spinand.h
>>>>> @@ -135,6 +135,8 @@ struct spinand_manufacturer_ops {
>>>>>        bool(*detect)(struct spinand_device *chip);
>>>>>        int (*init)(struct spinand_device *chip);
>>>>>        void (*cleanup)(struct spinand_device *chip);
>>>>> +     void (*build_column_addr)(struct spinand_device *chip,
>>>>> +                               struct spinand_op *op, u32 page, u32
>>>>> column);
>>>>
>>>> Okay, I think Arnaud was right, maybe we should have vendor specific
>>>> ops for basic operations like ->prepare_read/write_op(), instead of
>>>> having these ->get_dummy() and ->build_column_addr() hooks.
>>>> Or maybe just a ->prepare_op() hook that can prepare things for any
>>>> basic operation (read, write, ...).
>>>
>>> I prefer ->prepare_read_op() and ->prepare_write_op. Fix this in v3
>>
>> I'd like to have Arnaud's feedback on this. Can you wait a bit before
>> sending a new version?
>
>
> ->prepare_read_op() and ->prepare_write_op
> looks fine to me.

Arnaud and Boris,

For prepare_read/write_op, should we put common code in
spinand_read_from_cache(), spinand_program_data_to_cache() of spinandbase.c
and then prepare_read/write_op hook just do some fixing ? Code likes this:
      static int spinand_read_from_cache(struct spinand_device *chip,
                  u32 page_addr, u32 column, size_t len, u8 *rbuf)
     {
           struct spinand_op cmd;

           spinand_op_init(&cmd);
           cmd.cmd = chip->read_cache_op;
           cmd.n_addr = 2;
           cmd.addr[0] = (u8)(column >> 8);
           cmd.addr[1] = (u8)column;
           cmd.addr_nbits = spinand_get_address_bits(chip->read_cache_op);
           cmd.n_rx = len;
           cmd.rx_buf = rbuf;
           cmd.data_nbits = spinand_get_data_bits(chip->read_cache_op);
           if (chip->manufacturer.manu->ops->prepare_op)
                  chip->manufacturer.manu->ops->prepare_op(chip, &cmd,

      page_addr, column);

          return spinand_exec_cmd(chip, &cmd);
    }


>
> FYI: I have prepared a imx6sl board which can run vanilla mtd/master
> kernels.
> On this board, I can swap the various spinand samples I have:
> - Micron (the one supported by those patches)
> - winbond
> - macronix
> - ESMT
>
> Looking forward to test v3 ;)

Arnaud,

Thanks for your help in advance!


> Arnaud
>

Re: [PATCH v2 1/6] nand: spi: Add init/release function

[Peter Pan]

On Wed, Mar 1, 2017 at 5:58 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> +Arnaud
>
> Hi Peter,
>
> Can you please Cc Arnaud (and in general all reviewers) each time you
> send a new version.
>
> On Wed, 1 Mar 2017 16:52:05 +0800
> Peter Pan <peterpandong@micron.com> wrote:
>
>> This is the first commit for spi nand framkework.
>> This commit is to add spi nand initialization and
>> release functions.
>
> Hm, actually you're doing more than that. Just say that you 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/spinand_base.c | 469 ++++++++++++++++++++++++++++++++++++
>>  drivers/mtd/nand/spi/spinand_ids.c  |  29 +++
>>  include/linux/mtd/spinand.h         | 315 ++++++++++++++++++++++++
>
> If you're okay with that, I'd like you to add an entry in MAINTAINERS
> for the spinand sub-sub-sub-system :-). This way you'll be tagged as
> the maintainer of this code and will be Cc when a patch is submitted.
>
>>  7 files changed, 822 insertions(+)
>>  create mode 100644 drivers/mtd/nand/spi/Kconfig
>>  create mode 100644 drivers/mtd/nand/spi/Makefile
>>  create mode 100644 drivers/mtd/nand/spi/spinand_base.c
>>  create mode 100644 drivers/mtd/nand/spi/spinand_ids.c
>>  create mode 100644 include/linux/mtd/spinand.h
>>
[...]
>> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
>> new file mode 100644
>> index 0000000..f3d0351
>> --- /dev/null
>> +++ b/include/linux/mtd/spinand.h
>> @@ -0,0 +1,315 @@
>> +/**
>> +* spinand.h
>> +*
>> +* 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>

[...]

>> +
>> +/*SPI NAND manufacture ID definition*/
>> +#define SPINAND_MFR_MICRON           0x2C
>
> Should not be exposed here (keep it in your vendor source file.

Boris and Arnaud,

I'm thinking about where to keep these SPINAND_MFR_XXX macros.
Now the case is both spinand_ids.c and spinand_micron(vendor).c need
this macro. spinand_ids.c stores struct spinand_manufacturer
spinand_manuf_ids[] table
and spinand_micron(vendor).c need this macro when detecting device by
4 bytes data from read ID. Of course we can create a spinand-manufacture.h
to keep these macros. Another way is spinand_vendor.c defines own
struct spinand_manufacturer (instead of struct spinand_manufacturer_ops), in
this way, the table in spinand_ids.c only contains pointers to these
struct spinand_manufacturer.
What's your opinion?

Thanks,
Peter Pan

Re: [PATCH v2 2/6] nand: spi: add basic operations support

[Arnaud Mouiche]

On 10/03/2017 02:58, Peter Pan wrote:
> On Fri, Mar 10, 2017 at 1:09 AM, Arnaud Mouiche
> <arnaud.mouiche@gmail.com> wrote:
>>
>> On 09/03/2017 07:02, Boris Brezillon wrote:
>>> [...]
>>>>>
>>>>>> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
>>>>>> index f3d0351..ee447c1 100644
>>>>>> --- a/include/linux/mtd/spinand.h
>>>>>> +++ b/include/linux/mtd/spinand.h
>>>>>> @@ -135,6 +135,8 @@ struct spinand_manufacturer_ops {
>>>>>>         bool(*detect)(struct spinand_device *chip);
>>>>>>         int (*init)(struct spinand_device *chip);
>>>>>>         void (*cleanup)(struct spinand_device *chip);
>>>>>> +     void (*build_column_addr)(struct spinand_device *chip,
>>>>>> +                               struct spinand_op *op, u32 page, u32
>>>>>> column);
>>>>> Okay, I think Arnaud was right, maybe we should have vendor specific
>>>>> ops for basic operations like ->prepare_read/write_op(), instead of
>>>>> having these ->get_dummy() and ->build_column_addr() hooks.
>>>>> Or maybe just a ->prepare_op() hook that can prepare things for any
>>>>> basic operation (read, write, ...).
>>>> I prefer ->prepare_read_op() and ->prepare_write_op. Fix this in v3
>>> I'd like to have Arnaud's feedback on this. Can you wait a bit before
>>> sending a new version?
>>
>> ->prepare_read_op() and ->prepare_write_op
>> looks fine to me.
> Arnaud and Boris,
>
> For prepare_read/write_op, should we put common code in
> spinand_read_from_cache(), spinand_program_data_to_cache() of spinandbase.c
> and then prepare_read/write_op hook just do some fixing ? Code likes this:
>        static int spinand_read_from_cache(struct spinand_device *chip,
>                    u32 page_addr, u32 column, size_t len, u8 *rbuf)
>       {
>             struct spinand_op cmd;
>
>             spinand_op_init(&cmd);
>             cmd.cmd = chip->read_cache_op;
>             cmd.n_addr = 2;
>             cmd.addr[0] = (u8)(column >> 8);
>             cmd.addr[1] = (u8)column;
>             cmd.addr_nbits = spinand_get_address_bits(chip->read_cache_op);
>             cmd.n_rx = len;
>             cmd.rx_buf = rbuf;
>             cmd.data_nbits = spinand_get_data_bits(chip->read_cache_op);
>             if (chip->manufacturer.manu->ops->prepare_op)
>                    chip->manufacturer.manu->ops->prepare_op(chip, &cmd,
>
>        page_addr, column);
>
>            return spinand_exec_cmd(chip, &cmd);
>      }

Personally, I would let ops->prepare_op do all the

            cmd.n_addr = 2;
            cmd.addr[0] = (u8)(column >> 8);
            cmd.addr[1] = (u8)column;
            cmd.addr_nbits = spinand_get_address_bits(chip->read_cache_op);

part unconditionally.
But it is a matter of taste. I would not cost a lot to change the 
behavior if a 2 byte address would not be the main case with future devices.

Arnaud
>
>> FYI: I have prepared a imx6sl board which can run vanilla mtd/master
>> kernels.
>> On this board, I can swap the various spinand samples I have:
>> - Micron (the one supported by those patches)
>> - winbond
>> - macronix
>> - ESMT
>>
>> Looking forward to test v3 ;)
> Arnaud,
>
> Thanks for your help in advance!
>
>
>> Arnaud
>>

Re: [PATCH v2 1/6] nand: spi: Add init/release function

[Arnaud Mouiche]

On 10/03/2017 08:50, Peter Pan wrote:
> On Wed, Mar 1, 2017 at 5:58 PM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
>> +Arnaud
>>
>> Hi Peter,
>>
>> Can you please Cc Arnaud (and in general all reviewers) each time you
>> send a new version.
>>
>> On Wed, 1 Mar 2017 16:52:05 +0800
>> Peter Pan <peterpandong@micron.com> wrote:
>>
>>> This is the first commit for spi nand framkework.
>>> This commit is to add spi nand initialization and
>>> release functions.
>> Hm, actually you're doing more than that. Just say that you 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/spinand_base.c | 469 ++++++++++++++++++++++++++++++++++++
>>>   drivers/mtd/nand/spi/spinand_ids.c  |  29 +++
>>>   include/linux/mtd/spinand.h         | 315 ++++++++++++++++++++++++
>> If you're okay with that, I'd like you to add an entry in MAINTAINERS
>> for the spinand sub-sub-sub-system :-). This way you'll be tagged as
>> the maintainer of this code and will be Cc when a patch is submitted.
>>
>>>   7 files changed, 822 insertions(+)
>>>   create mode 100644 drivers/mtd/nand/spi/Kconfig
>>>   create mode 100644 drivers/mtd/nand/spi/Makefile
>>>   create mode 100644 drivers/mtd/nand/spi/spinand_base.c
>>>   create mode 100644 drivers/mtd/nand/spi/spinand_ids.c
>>>   create mode 100644 include/linux/mtd/spinand.h
>>>
> [...]
>>> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
>>> new file mode 100644
>>> index 0000000..f3d0351
>>> --- /dev/null
>>> +++ b/include/linux/mtd/spinand.h
>>> @@ -0,0 +1,315 @@
>>> +/**
>>> +* spinand.h
>>> +*
>>> +* 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>
> [...]
>
>>> +
>>> +/*SPI NAND manufacture ID definition*/
>>> +#define SPINAND_MFR_MICRON           0x2C
>> Should not be exposed here (keep it in your vendor source file.
> Boris and Arnaud,
>
> I'm thinking about where to keep these SPINAND_MFR_XXX macros.
> Now the case is both spinand_ids.c and spinand_micron(vendor).c need
> this macro. spinand_ids.c stores struct spinand_manufacturer
> spinand_manuf_ids[] table
> and spinand_micron(vendor).c need this macro when detecting device by
> 4 bytes data from read ID. Of course we can create a spinand-manufacture.h
> to keep these macros. Another way is spinand_vendor.c defines own
> struct spinand_manufacturer (instead of struct spinand_manufacturer_ops), in
> this way, the table in spinand_ids.c only contains pointers to these
> struct spinand_manufacturer.
> What's your opinion?

It depends on how you have implemented the scan in your v3.

If the core call every manufacturer op->detect to tell if the 4 bytes 
are matching, AND  to return/fill the the chip info and parameters (size 
/ layout / etc...), the core doesn't even need to know the manufacturer ID.

On the other hand, if manufacturer op->detect just tell "I'm the 
manufacturer and this is it's ID" (leaving the core to lookup in its 
spinand ID table which device it is finally) then yes, you need a 
centralized spinand-manufacture.h

I prefer the first option, where detect returns directly the chip ops + 
info + layout at once.
Arnaud
>
> Thanks,
> Peter Pan