[PATCH 0/6] SPI NAND for everyone

[PATCH 0/6] SPI NAND for everyone

[Ezequiel Garcia]

During the discussion of Ionela Voinescu's patch to support GD5F SPI NAND
devices [1], it was decided that a proper SPI NAND framework was needed
to support the mt29f device (driver currently in staging area) and the new
gd5f.

This patchset is a first attempt to address this.

The SPI NAND framework allows devices to register as NAND drivers. This is
useful to take advantage of the bad block management code. Given the
SPI NAND and the bare NAND commands are different, the SPI NAND framework
implements its own .cmdfunc callback, which is in charge of calling
device-specific hooks for each of the flash operations (read, program, erase,
etc).

The SPI NAND framework does not deal with SPI transactions per-se. Instead,
the SPI messages should be prepared by the users of the SPI NAND framework
(those that implement the device-specific hooks).

The result can be expressed in the following hierarchy:

    Userspace
  ------------------
    MTD
  ------------------
    NAND core
  ------------------
    SPI NAND core
  ------------------
    SPI NAND device
  ------------------
    SPI core
  ------------------
    SPI master
  ------------------
    Hardware

Notice there was a previous attempt to propose an SPI NAND framework,
by Sourav Poddar and Mona Anonuevo. We didn't find this proposal suitable,
so this series is a completely new work.

This series is based on v3.18-rc7. Tests have been performed with a Gigadevice
GD5F 4 Gbit device, including nandtest runs and filesystem testing on top
of UBI. I don't have MT29F devices yet, but the amount of changes required to
support it should be fairly small.

I don't intend this first proposal to be complete, but I hope it's a good
starting point to support SPI NAND properly.

[1] http://lists.infradead.org/pipermail/linux-mtd/2014-November/056364.html
[2] https://lkml.org/lkml/2013/6/26/83

Ezequiel Garcia (6):
  mtd: nand: Check length of ID before reading bits per cell
  mtd: nand: Add JEDEC manufacturer ID for Gigadevice
  mtd: nand: Allow to set a per-device ECC layout
  mtd: Introduce SPI NAND framework
  mtd: spi-nand: Add devicetree binding
  mtd: spi-nand: Support common SPI NAND devices

 Documentation/devicetree/bindings/mtd/spi-nand.txt |  21 +
 drivers/mtd/Kconfig                                |   2 +
 drivers/mtd/Makefile                               |   1 +
 drivers/mtd/nand/nand_base.c                       |   4 +-
 drivers/mtd/nand/nand_ids.c                        |   1 +
 drivers/mtd/spi-nand/Kconfig                       |  18 +
 drivers/mtd/spi-nand/Makefile                      |   2 +
 drivers/mtd/spi-nand/spi-nand-base.c               | 530 +++++++++++++++++++++
 drivers/mtd/spi-nand/spi-nand-device.c             | 500 +++++++++++++++++++
 include/linux/mtd/nand.h                           |   3 +
 include/linux/mtd/spi-nand.h                       |  54 +++
 11 files changed, 1135 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/spi-nand.txt
 create mode 100644 drivers/mtd/spi-nand/Kconfig
 create mode 100644 drivers/mtd/spi-nand/Makefile
 create mode 100644 drivers/mtd/spi-nand/spi-nand-base.c
 create mode 100644 drivers/mtd/spi-nand/spi-nand-device.c
 create mode 100644 include/linux/mtd/spi-nand.h

-- 
2.1.0

[PATCH 1/6] mtd: nand: Check length of ID before reading bits per cell

[Ezequiel Garcia]

The table-based NAND identification currently reads the number
of bits per cell from the 3rd byte of the extended ID. This is done
for the so-called 'full ID' devices; i.e. devices that have a known
length ID.

However, if the ID length is shorter than three, there's no 3rd byte,
and so it's wrong to read the bits per cell from there. Fix this by
adding a check for the ID length.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
---
 drivers/mtd/nand/nand_base.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index 5b5c627..a4c9cee 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3589,7 +3589,8 @@ static bool find_full_id_nand(struct mtd_info *mtd, struct nand_chip *chip,
 		mtd->erasesize = type->erasesize;
 		mtd->oobsize = type->oobsize;
 
-		chip->bits_per_cell = nand_get_bits_per_cell(id_data[2]);
+		if (type->id_len > 2)
+			chip->bits_per_cell = nand_get_bits_per_cell(id_data[2]);
 		chip->chipsize = (uint64_t)type->chipsize << 20;
 		chip->options |= type->options;
 		chip->ecc_strength_ds = NAND_ECC_STRENGTH(type);
-- 
2.1.0

Re: [PATCH 1/6] mtd: nand: Check length of ID before reading bits per cell

[Daniel Ehrenberg]

On Tue, Dec 2, 2014 at 4:58 AM, Ezequiel Garcia
<ezequiel.garcia@imgtec.com> wrote:
> The table-based NAND identification currently reads the number
> of bits per cell from the 3rd byte of the extended ID. This is done
> for the so-called 'full ID' devices; i.e. devices that have a known
> length ID.
>
> However, if the ID length is shorter than three, there's no 3rd byte,
> and so it's wrong to read the bits per cell from there. Fix this by
> adding a check for the ID length.
>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>

Reviewed-by: Dan Ehrenberg <dehrenberg@chromium.org>

Re: [PATCH 1/6] mtd: nand: Check length of ID before reading bits per cell

[Brian Norris]

A little tangential to the big-picture discussion, but I'll get this out
there:

On Tue, Dec 02, 2014 at 09:58:51AM -0300, Ezequiel Garcia wrote:
> The table-based NAND identification currently reads the number
> of bits per cell from the 3rd byte of the extended ID. This is done
> for the so-called 'full ID' devices; i.e. devices that have a known
> length ID.
> 
> However, if the ID length is shorter than three, there's no 3rd byte,
> and so it's wrong to read the bits per cell from there. Fix this by
> adding a check for the ID length.

You're right about the problem, but I'm not sure about the fix. I
actually don't know why we are using the ID bytes to guess at the number
of bits per cell here; the point of the full-id checks is that with some
NAND, we just can't decode all the information reliably from the ID.

I'd kinda rather just see bits-per-cell represented as a separate bit;
either a new field, or in the 'options' field.

> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
> ---
>  drivers/mtd/nand/nand_base.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 5b5c627..a4c9cee 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3589,7 +3589,8 @@ static bool find_full_id_nand(struct mtd_info *mtd, struct nand_chip *chip,
>  		mtd->erasesize = type->erasesize;
>  		mtd->oobsize = type->oobsize;
>  
> -		chip->bits_per_cell = nand_get_bits_per_cell(id_data[2]);
> +		if (type->id_len > 2)
> +			chip->bits_per_cell = nand_get_bits_per_cell(id_data[2]);
>  		chip->chipsize = (uint64_t)type->chipsize << 20;
>  		chip->options |= type->options;
>  		chip->ecc_strength_ds = NAND_ECC_STRENGTH(type);

Brian

[PATCH 2/6] mtd: nand: Add JEDEC manufacturer ID for Gigadevice

[Ezequiel Garcia]

This commit adds Gigadevice to the list of manufacturer ID and name strings.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
---
 drivers/mtd/nand/nand_ids.c | 1 +
 include/linux/mtd/nand.h    | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/mtd/nand/nand_ids.c b/drivers/mtd/nand/nand_ids.c
index fbde8910..437196e 100644
--- a/drivers/mtd/nand/nand_ids.c
+++ b/drivers/mtd/nand/nand_ids.c
@@ -178,6 +178,7 @@ struct nand_manufacturers nand_manuf_ids[] = {
 	{NAND_MFR_EON, "Eon"},
 	{NAND_MFR_SANDISK, "SanDisk"},
 	{NAND_MFR_INTEL, "Intel"},
+	{NAND_MFR_GIGADEVICE, "Gigadevice"},
 	{0x0, "Unknown"}
 };
 
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index e4d451e..a36fff1 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -723,6 +723,7 @@ struct nand_chip {
 #define NAND_MFR_EON		0x92
 #define NAND_MFR_SANDISK	0x45
 #define NAND_MFR_INTEL		0x89
+#define NAND_MFR_GIGADEVICE	0xc8
 
 /* The maximum expected count of bytes in the NAND ID sequence */
 #define NAND_MAX_ID_LEN 8
-- 
2.1.0

Re: [PATCH 2/6] mtd: nand: Add JEDEC manufacturer ID for Gigadevice

[Daniel Ehrenberg]

On Tue, Dec 2, 2014 at 4:58 AM, Ezequiel Garcia
<ezequiel.garcia@imgtec.com> wrote:
> This commit adds Gigadevice to the list of manufacturer ID and name strings.
>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>

Reviewed-by: Dan Ehrenberg <dehrenberg@chromium.org>

Re: [PATCH 2/6] mtd: nand: Add JEDEC manufacturer ID for Gigadevice

[Ezequiel Garcia]

On 12/12/2014 09:49 PM, Daniel Ehrenberg wrote:
> On Tue, Dec 2, 2014 at 4:58 AM, Ezequiel Garcia
> <ezequiel.garcia@imgtec.com> wrote:
>> This commit adds Gigadevice to the list of manufacturer ID and name strings.
>>
>> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
> 
> Reviewed-by: Dan Ehrenberg <dehrenberg@chromium.org>
> 

Hey Brian,

Do you think you can pick this one?

Thanks,
-- 
Ezequiel

Re: [PATCH 2/6] mtd: nand: Add JEDEC manufacturer ID for Gigadevice

[Brian Norris]

On Tue, Apr 21, 2015 at 08:04:27PM -0300, Ezequiel Garcia wrote:
> On 12/12/2014 09:49 PM, Daniel Ehrenberg wrote:
> > On Tue, Dec 2, 2014 at 4:58 AM, Ezequiel Garcia
> > <ezequiel.garcia@imgtec.com> wrote:
> >> This commit adds Gigadevice to the list of manufacturer ID and name strings.
> >>
> >> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
> > 
> > Reviewed-by: Dan Ehrenberg <dehrenberg@chromium.org>
> > 
> 
> Hey Brian,
> 
> Do you think you can pick this one?

Why would I do that, when I thought we were considering having a
separate SPI NAND vs. parallel NAND driver (including separate ID
tables)? Or are you seeing parallel NAND by Gigadevice?

Brian

[PATCH 3/6] mtd: nand: Allow to set a per-device ECC layout

[Ezequiel Garcia]

Until now, the ECC was handled either by Linux (i.e. software ECC) or by the
NAND controller (i.e. hardware ECC). In each of these, the ECC layout is
defined by the controller of the NAND device, not by the NAND device itself.

However, devices with on-die ECC support have their own ECC layout so we need
a way to specify a per-device ECC layout.

This commit adds a new field to the nand_flash_dev structure, to allow devices
to specify its own ECC layout.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
---
 drivers/mtd/nand/nand_base.c | 1 +
 include/linux/mtd/nand.h     | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index a4c9cee..10f5ac9 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -3595,6 +3595,7 @@ static bool find_full_id_nand(struct mtd_info *mtd, struct nand_chip *chip,
 		chip->options |= type->options;
 		chip->ecc_strength_ds = NAND_ECC_STRENGTH(type);
 		chip->ecc_step_ds = NAND_ECC_STEP(type);
+		chip->ecc.layout = type->ecc.layout;
 		chip->onfi_timing_mode_default =
 					type->onfi_timing_mode_default;
 
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index a36fff1..a36d628 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -780,6 +780,7 @@ struct nand_chip {
  *               @ecc_step_ds in nand_chip{}, also from the datasheet.
  *               For example, the "4bit ECC for each 512Byte" can be set with
  *               NAND_ECC_INFO(4, 512).
+ * @ecc.layout: If the device has on-die ECC, it can provide its own ECC layout.
  * @onfi_timing_mode_default: the default ONFI timing mode entered after a NAND
  *			      reset. Should be deduced from timings described
  *			      in the datasheet.
@@ -803,6 +804,7 @@ struct nand_flash_dev {
 	struct {
 		uint16_t strength_ds;
 		uint16_t step_ds;
+		struct nand_ecclayout *layout;
 	} ecc;
 	int onfi_timing_mode_default;
 };
-- 
2.1.0

Re: [PATCH 3/6] mtd: nand: Allow to set a per-device ECC layout

[Daniel Ehrenberg]

On Tue, Dec 2, 2014 at 4:58 AM, Ezequiel Garcia
<ezequiel.garcia@imgtec.com> wrote:
> Until now, the ECC was handled either by Linux (i.e. software ECC) or by the
> NAND controller (i.e. hardware ECC). In each of these, the ECC layout is
> defined by the controller of the NAND device, not by the NAND device itself.
>
> However, devices with on-die ECC support have their own ECC layout so we need
> a way to specify a per-device ECC layout.
>
> This commit adds a new field to the nand_flash_dev structure, to allow devices
> to specify its own ECC layout.
>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>

Reviewed-by: Dan Ehrenberg <dehrenberg@chromium.org>

[PATCH 4/6] mtd: Introduce SPI NAND framework

[Ezequiel Garcia]

Add a new framework, to support SPI NAND devices. The framework registers
a NAND chip and handles the generic SPI NAND protocol, calling device-specific
hooks for each SPI NAND command.

The following is the stack design, from userspace to hardware. This commit
adds the "SPI NAND core" layer.

    Userspace
  ------------------
    MTD
  ------------------
    NAND core
  ------------------
    SPI NAND core
  ------------------
    SPI NAND device
  ------------------
    SPI core
  ------------------
    SPI master
  ------------------
    Hardware

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
---
 drivers/mtd/Kconfig                  |   2 +
 drivers/mtd/Makefile                 |   1 +
 drivers/mtd/spi-nand/Kconfig         |   6 +
 drivers/mtd/spi-nand/Makefile        |   1 +
 drivers/mtd/spi-nand/spi-nand-base.c | 530 +++++++++++++++++++++++++++++++++++
 include/linux/mtd/spi-nand.h         |  54 ++++
 6 files changed, 594 insertions(+)
 create mode 100644 drivers/mtd/spi-nand/Kconfig
 create mode 100644 drivers/mtd/spi-nand/Makefile
 create mode 100644 drivers/mtd/spi-nand/spi-nand-base.c
 create mode 100644 include/linux/mtd/spi-nand.h

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index 94b8210..7bab890 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -323,6 +323,8 @@ source "drivers/mtd/lpddr/Kconfig"
 
 source "drivers/mtd/spi-nor/Kconfig"
 
+source "drivers/mtd/spi-nand/Kconfig"
+
 source "drivers/mtd/ubi/Kconfig"
 
 endif # MTD
diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
index 99bb9a1..581688f 100644
--- a/drivers/mtd/Makefile
+++ b/drivers/mtd/Makefile
@@ -33,4 +33,5 @@ inftl-objs		:= inftlcore.o inftlmount.o
 obj-y		+= chips/ lpddr/ maps/ devices/ nand/ onenand/ tests/
 
 obj-$(CONFIG_MTD_SPI_NOR)	+= spi-nor/
+obj-$(CONFIG_MTD_SPI_NAND)	+= spi-nand/
 obj-$(CONFIG_MTD_UBI)		+= ubi/
diff --git a/drivers/mtd/spi-nand/Kconfig b/drivers/mtd/spi-nand/Kconfig
new file mode 100644
index 0000000..3868477
--- /dev/null
+++ b/drivers/mtd/spi-nand/Kconfig
@@ -0,0 +1,6 @@
+menuconfig MTD_SPI_NAND
+	tristate "SPI NAND device support"
+	depends on MTD
+	select MTD_NAND
+	help
+	  This is the framework for the SPI NAND.
diff --git a/drivers/mtd/spi-nand/Makefile b/drivers/mtd/spi-nand/Makefile
new file mode 100644
index 0000000..d454c52
--- /dev/null
+++ b/drivers/mtd/spi-nand/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_MTD_SPI_NAND)		+= spi-nand-base.o
diff --git a/drivers/mtd/spi-nand/spi-nand-base.c b/drivers/mtd/spi-nand/spi-nand-base.c
new file mode 100644
index 0000000..f41d3c5
--- /dev/null
+++ b/drivers/mtd/spi-nand/spi-nand-base.c
@@ -0,0 +1,530 @@
+/*
+ * Copyright (C) 2014 Imagination Technologies Ltd.
+ *
+ * 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; version 2 of the License.
+ *
+ * Notes:
+ * 1. Erase and program operations need to call write_enable() first,
+ *    to clear the enable bit. This bit is cleared automatically after
+ *    the erase or program operation.
+ *
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mtd/nand.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+#include <linux/mtd/spi-nand.h>
+#include <linux/of_platform.h>
+#include <linux/of_mtd.h>
+#include <linux/slab.h>
+
+/* Registers common to all devices */
+#define SPI_NAND_LOCK_REG		0xa0
+#define SPI_NAND_PROT_UNLOCK_ALL	0x0
+
+#define SPI_NAND_FEATURE_REG		0xb0
+#define SPI_NAND_ECC_EN			BIT(4)
+
+#define SPI_NAND_STATUS_REG		0xc0
+#define SPI_NAND_STATUS_REG_ECC_MASK	0x3
+#define SPI_NAND_STATUS_REG_ECC_SHIFT	4
+#define SPI_NAND_STATUS_REG_PROG_FAIL	BIT(3)
+#define SPI_NAND_STATUS_REG_ERASE_FAIL	BIT(2)
+#define SPI_NAND_STATUS_REG_WREN	BIT(1)
+#define SPI_NAND_STATUS_REG_BUSY	BIT(0)
+
+#define SPI_NAND_CMD_BUF_LEN		8
+
+/* Rewind and fill the buffer with 0xff */
+static void spi_nand_clear_buffer(struct spi_nand *snand)
+{
+	snand->buf_start = 0;
+	memset(snand->data_buf, 0xff, snand->buf_size);
+}
+
+static int spi_nand_enable_ecc(struct spi_nand *snand)
+{
+	int ret;
+
+	ret = snand->read_reg(snand, SPI_NAND_FEATURE_REG, snand->buf);
+	if (ret)
+		return ret;
+
+	snand->buf[0] |= SPI_NAND_ECC_EN;
+	ret = snand->write_reg(snand, SPI_NAND_FEATURE_REG, snand->buf);
+	if (ret)
+		return ret;
+	snand->ecc = true;
+
+	return 0;
+}
+
+static int spi_nand_disable_ecc(struct spi_nand *snand)
+{
+	int ret;
+
+	ret = snand->read_reg(snand, SPI_NAND_FEATURE_REG, snand->buf);
+	if (ret)
+		return ret;
+
+	snand->buf[0] &= ~SPI_NAND_ECC_EN;
+	ret = snand->write_reg(snand, SPI_NAND_FEATURE_REG, snand->buf);
+	if (ret)
+		return ret;
+	snand->ecc = false;
+
+	return 0;
+}
+
+/*
+ * Wait until the status register busy bit is cleared.
+ * Returns a negatie errno on error or time out, and a non-negative status
+ * value if the device is ready.
+ */
+static int spi_nand_wait_till_ready(struct spi_nand *snand)
+{
+	unsigned long deadline = jiffies + msecs_to_jiffies(100);
+	bool timeout = false;
+	int ret;
+
+	/*
+	 * Perhaps we should set a different timeout for each
+	 * operation (reset, read, write, erase).
+	 */
+	while (!timeout) {
+		if (time_after_eq(jiffies, deadline))
+			timeout = true;
+
+		ret = snand->read_reg(snand, SPI_NAND_STATUS_REG, snand->buf);
+		if (ret < 0) {
+			dev_err(snand->dev, "error reading status register\n");
+			return ret;
+		} else if (!(snand->buf[0] & SPI_NAND_STATUS_REG_BUSY)) {
+			return snand->buf[0];
+		}
+
+		cond_resched();
+	}
+
+	dev_err(snand->dev, "operation timed out\n");
+
+	return -ETIMEDOUT;
+}
+
+static int spi_nand_reset(struct spi_nand *snand)
+{
+	int ret;
+
+	ret = snand->reset(snand);
+	if (ret < 0) {
+		dev_err(snand->dev, "reset command failed\n");
+		return ret;
+	}
+
+	/*
+	 * The NAND core won't wait after a device reset, so we need
+	 * to do that here.
+	 */
+	ret = spi_nand_wait_till_ready(snand);
+	if (ret < 0)
+		return ret;
+	return 0;
+}
+
+static int spi_nand_status(struct spi_nand *snand)
+{
+	int ret, status;
+
+	ret = snand->read_reg(snand, SPI_NAND_STATUS_REG, snand->buf);
+	if (ret < 0) {
+		dev_err(snand->dev, "error reading status register\n");
+		return ret;
+	}
+	status = snand->buf[0];
+
+	/* Convert this into standard NAND_STATUS values */
+	if (status & SPI_NAND_STATUS_REG_BUSY)
+		snand->buf[0] = 0;
+	else
+		snand->buf[0] = NAND_STATUS_READY;
+
+	if (status & SPI_NAND_STATUS_REG_PROG_FAIL ||
+	    status & SPI_NAND_STATUS_REG_ERASE_FAIL)
+		snand->buf[0] |= NAND_STATUS_FAIL;
+
+	/*
+	 * Since we unlock the entire device at initialization, unconditionally
+	 * set the WP bit to indicate it's not protected.
+	 */
+	snand->buf[0] |= NAND_STATUS_WP;
+	return 0;
+}
+
+static int spi_nand_erase(struct spi_nand *snand, int page_addr)
+{
+	int ret;
+
+	ret = snand->write_enable(snand);
+	if (ret < 0) {
+		dev_err(snand->dev, "write enable command failed\n");
+		return ret;
+	}
+
+	ret = snand->block_erase(snand, page_addr);
+	if (ret < 0) {
+		dev_err(snand->dev, "block erase command failed\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int spi_nand_write(struct spi_nand *snand)
+{
+	int ret;
+
+	/* Store the page to cache */
+	ret = snand->store_cache(snand, 0, snand->buf_size, snand->data_buf);
+	if (ret < 0) {
+		dev_err(snand->dev, "error %d storing page 0x%x to cache\n",
+			ret, snand->page_addr);
+		return ret;
+	}
+
+	ret = spi_nand_wait_till_ready(snand);
+	if (ret)
+		return ret;
+
+	ret = snand->write_enable(snand);
+	if (ret < 0) {
+		dev_err(snand->dev, "write enable command failed\n");
+		return ret;
+	}
+
+	/* Get page from the device cache into our internal buffer */
+	ret = snand->write_page(snand, snand->page_addr);
+	if (ret < 0) {
+		dev_err(snand->dev, "error %d reading page 0x%x from cache\n",
+			ret, snand->page_addr);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int spi_nand_read_id(struct spi_nand *snand)
+{
+	int ret;
+
+	ret = snand->read_id(snand, snand->data_buf);
+	if (ret < 0) {
+		dev_err(snand->dev, "error %d reading ID\n", ret);
+		return ret;
+	}
+	return 0;
+}
+
+static int spi_nand_read_page(struct spi_nand *snand, unsigned int page_addr,
+			      unsigned int page_offset, size_t length)
+{
+	unsigned int corrected = 0, ecc_error = 0;
+	int ret;
+
+	/* Load a page into the cache register */
+	ret = snand->load_page(snand, page_addr);
+	if (ret < 0) {
+		dev_err(snand->dev, "error %d loading page 0x%x to cache\n",
+			ret, page_addr);
+		return ret;
+	}
+
+	ret = spi_nand_wait_till_ready(snand);
+	if (ret < 0)
+		return ret;
+
+	if (snand->ecc) {
+		snand->get_ecc_status(ret, &corrected, &ecc_error);
+		snand->bitflips = corrected;
+
+		/*
+		 * 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) {
+			dev_err(snand->dev,
+				"internal ECC error reading page 0x%x\n",
+				page_addr);
+			snand->mtd.ecc_stats.failed++;
+		}
+	}
+
+	/* Get page from the device cache into our internal buffer */
+	ret = snand->read_cache(snand, page_offset, length, snand->data_buf);
+	if (ret < 0) {
+		dev_err(snand->dev, "error %d reading page 0x%x from cache\n",
+			ret, page_addr);
+		return ret;
+	}
+	return 0;
+}
+
+static u8 spi_nand_read_byte(struct mtd_info *mtd)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct spi_nand *snand = chip->priv;
+	char val = 0xff;
+
+	if (snand->buf_start < snand->buf_size)
+		val = snand->data_buf[snand->buf_start++];
+	return val;
+}
+
+static void spi_nand_write_buf(struct mtd_info *mtd, const u8 *buf, int len)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct spi_nand *snand = chip->priv;
+	size_t n = min_t(size_t, len, snand->buf_size - snand->buf_start);
+
+	memcpy(snand->data_buf + snand->buf_start, buf, n);
+	snand->buf_start += n;
+}
+
+static void spi_nand_read_buf(struct mtd_info *mtd, u8 *buf, int len)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct spi_nand *snand = chip->priv;
+	size_t n = min_t(size_t, len, snand->buf_size - snand->buf_start);
+
+	memcpy(buf, snand->data_buf + snand->buf_start, n);
+	snand->buf_start += n;
+}
+
+static int spi_nand_write_page_hwecc(struct mtd_info *mtd,
+		struct nand_chip *chip, const uint8_t *buf, int oob_required)
+{
+	chip->write_buf(mtd, buf, mtd->writesize);
+	chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
+
+	return 0;
+}
+
+static int spi_nand_read_page_hwecc(struct mtd_info *mtd,
+		struct nand_chip *chip, uint8_t *buf, int oob_required,
+		int page)
+{
+	struct spi_nand *snand = chip->priv;
+
+	chip->read_buf(mtd, buf, mtd->writesize);
+	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
+
+	return snand->bitflips;
+}
+
+static int spi_nand_waitfunc(struct mtd_info *mtd, struct nand_chip *chip)
+{
+	struct spi_nand *snand = chip->priv;
+	int ret;
+
+	ret = spi_nand_wait_till_ready(snand);
+
+	if (ret < 0) {
+		return NAND_STATUS_FAIL;
+	} else if (ret & SPI_NAND_STATUS_REG_PROG_FAIL) {
+		dev_err(snand->dev, "page program failed\n");
+		return NAND_STATUS_FAIL;
+	} else if (ret & SPI_NAND_STATUS_REG_ERASE_FAIL) {
+		dev_err(snand->dev, "block erase failed\n");
+		return NAND_STATUS_FAIL;
+	}
+
+	return NAND_STATUS_READY;
+}
+
+static void spi_nand_cmdfunc(struct mtd_info *mtd, unsigned int command,
+			     int column, int page_addr)
+{
+	struct nand_chip *chip = mtd->priv;
+	struct spi_nand *snand = chip->priv;
+
+	/*
+	 * In case there's any unsupported command, let's make sure
+	 * we don't keep garbage around in the buffer.
+	 */
+	if (command != NAND_CMD_PAGEPROG) {
+		spi_nand_clear_buffer(snand);
+		snand->page_addr = 0;
+	}
+
+	switch (command) {
+	case NAND_CMD_READ0:
+		spi_nand_read_page(snand, page_addr, 0, mtd->writesize);
+		break;
+	case NAND_CMD_READOOB:
+		spi_nand_disable_ecc(snand);
+		spi_nand_read_page(snand, page_addr, mtd->writesize,
+				   mtd->oobsize);
+		spi_nand_enable_ecc(snand);
+		break;
+	case NAND_CMD_READID:
+		spi_nand_read_id(snand);
+		break;
+	case NAND_CMD_ERASE1:
+		spi_nand_erase(snand, page_addr);
+		break;
+	case NAND_CMD_ERASE2:
+		/* There's nothing to do here, as the erase is one-step */
+		break;
+	case NAND_CMD_SEQIN:
+		snand->buf_start = column;
+		snand->page_addr = page_addr;
+		break;
+	case NAND_CMD_PAGEPROG:
+		spi_nand_write(snand);
+		break;
+	case NAND_CMD_STATUS:
+		spi_nand_status(snand);
+		break;
+	case NAND_CMD_RESET:
+		spi_nand_reset(snand);
+		break;
+	default:
+		dev_err(&mtd->dev, "unknown command 0x%x\n", command);
+	}
+}
+
+static void spi_nand_select_chip(struct mtd_info *mtd, int chip)
+{
+	/* We need this to override the default */
+}
+
+int spi_nand_check(struct spi_nand *snand)
+{
+	if (!snand->dev)
+		return -ENODEV;
+	if (!snand->read_cache)
+		return -ENODEV;
+	if (!snand->load_page)
+		return -ENODEV;
+	if (!snand->store_cache)
+		return -ENODEV;
+	if (!snand->write_page)
+		return -ENODEV;
+	if (!snand->write_reg)
+		return -ENODEV;
+	if (!snand->read_reg)
+		return -ENODEV;
+	if (!snand->block_erase)
+		return -ENODEV;
+	if (!snand->reset)
+		return -ENODEV;
+	if (!snand->write_enable)
+		return -ENODEV;
+	if (!snand->write_disable)
+		return -ENODEV;
+	if (!snand->get_ecc_status)
+		return -ENODEV;
+	return 0;
+}
+
+int spi_nand_register(struct spi_nand *snand, struct nand_flash_dev *flash_ids)
+{
+	struct nand_chip *chip = &snand->nand_chip;
+	struct mtd_part_parser_data ppdata = {};
+	struct mtd_info *mtd = &snand->mtd;
+	struct device_node *np = snand->dev->of_node;
+	int ret;
+
+	/* Let's check all the hooks are in-place so we don't panic later */
+	ret = spi_nand_check(snand);
+	if (ret)
+		return ret;
+
+	chip->priv	= snand;
+	chip->read_buf	= spi_nand_read_buf;
+	chip->write_buf	= spi_nand_write_buf;
+	chip->read_byte	= spi_nand_read_byte;
+	chip->cmdfunc	= spi_nand_cmdfunc;
+	chip->waitfunc	= spi_nand_waitfunc;
+	chip->select_chip = spi_nand_select_chip;
+	chip->options |= NAND_NO_SUBPAGE_WRITE;
+	chip->bits_per_cell = 1;
+
+	chip->ecc.read_page	= spi_nand_read_page_hwecc;
+	chip->ecc.write_page	= spi_nand_write_page_hwecc;
+	chip->ecc.mode		= NAND_ECC_HW;
+
+	if (of_get_nand_on_flash_bbt(np))
+		chip->bbt_options |= NAND_BBT_USE_FLASH | NAND_BBT_NO_OOB;
+
+	mtd->name = snand->name;
+	mtd->owner = THIS_MODULE;
+	mtd->priv = chip;
+
+	/* Allocate buffer to be used to read/write the internal registers */
+	snand->buf = kmalloc(SPI_NAND_CMD_BUF_LEN, GFP_KERNEL);
+	if (!snand->buf)
+		return -ENOMEM;
+
+	/* Preallocate buffer for flash identification (NAND_CMD_READID) */
+	snand->buf_size = SPI_NAND_CMD_BUF_LEN;
+	snand->data_buf = kmalloc(snand->buf_size, GFP_KERNEL);
+
+	ret = nand_scan_ident(mtd, 1, flash_ids);
+	if (ret)
+		return ret;
+
+	/*
+	 * SPI NAND has on-die ECC, which means we can correct as much as
+	 * we are required to. This must be done after identification of
+	 * the device.
+	 */
+	chip->ecc.strength = chip->ecc_strength_ds;
+	chip->ecc.size = chip->ecc_step_ds;
+
+	/*
+	 * Unlock all the device before calling nand_scan_tail. This is needed
+	 * in case the in-flash bad block table needs to be created.
+	 * We could override __nand_unlock(), but since it's not currently used
+	 * by the NAND core we call this explicitly.
+	 */
+	snand->buf[0] = SPI_NAND_PROT_UNLOCK_ALL;
+	ret = snand->write_reg(snand, SPI_NAND_LOCK_REG, snand->buf);
+	if (ret)
+		return ret;
+
+	/* Free the buffer and allocate a good one, to fit a page plus OOB */
+	kfree(snand->data_buf);
+
+	snand->buf_size = mtd->writesize + mtd->oobsize;
+	snand->data_buf = kmalloc(snand->buf_size, GFP_KERNEL);
+	if (!snand->data_buf)
+		return -ENOMEM;
+
+	ret = nand_scan_tail(mtd);
+	if (ret)
+		return ret;
+
+	ppdata.of_node = np;
+	return mtd_device_parse_register(mtd, NULL, &ppdata, NULL, 0);
+}
+EXPORT_SYMBOL_GPL(spi_nand_register);
+
+void spi_nand_unregister(struct spi_nand *snand)
+{
+	kfree(snand->buf);
+	kfree(snand->data_buf);
+	nand_release(&snand->mtd);
+}
+EXPORT_SYMBOL_GPL(spi_nand_unregister);
+
+MODULE_AUTHOR("Ezequiel Garcia <ezequiel.garcia@imgtec.com>");
+MODULE_DESCRIPTION("Framework for SPI NAND");
+MODULE_LICENSE("GPL v2");
diff --git a/include/linux/mtd/spi-nand.h b/include/linux/mtd/spi-nand.h
new file mode 100644
index 0000000..9dd2ad6
--- /dev/null
+++ b/include/linux/mtd/spi-nand.h
@@ -0,0 +1,54 @@
+/*
+ * Copyright (C) 2014 Imagination Technologies Ltd.
+ *
+ * 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; version 2 of the License.
+ */
+
+#ifndef __LINUX_MTD_SPI_NAND_H
+#define __LINUX_MTD_SPI_NAND_H
+
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/nand.h>
+
+struct spi_nand {
+	struct mtd_info		mtd;
+	struct nand_chip	nand_chip;
+	struct device		*dev;
+	const char		*name;
+
+	u8			*buf, *data_buf;
+	size_t			buf_size;
+	off_t			buf_start;
+	unsigned int		page_addr;
+	unsigned int		bitflips;
+	bool			ecc;
+
+	int (*reset)(struct spi_nand *snand);
+	int (*read_id)(struct spi_nand *snand, u8 *buf);
+
+	int (*write_disable)(struct spi_nand *snand);
+	int (*write_enable)(struct spi_nand *snand);
+
+	int (*read_reg)(struct spi_nand *snand, u8 opcode, u8 *buf);
+	int (*write_reg)(struct spi_nand *snand, u8 opcode, u8 *buf);
+	void (*get_ecc_status)(unsigned int status,
+			       unsigned int *corrected,
+			       unsigned int *ecc_errors);
+
+	int (*store_cache)(struct spi_nand *snand, unsigned int page_offset,
+			   size_t length, u8 *write_buf);
+	int (*write_page)(struct spi_nand *snand, unsigned int page_addr);
+	int (*load_page)(struct spi_nand *snand, unsigned int page_addr);
+	int (*read_cache)(struct spi_nand *snand, unsigned int page_offset,
+			  size_t length, u8 *read_buf);
+	int (*block_erase)(struct spi_nand *snand, unsigned int page_addr);
+
+	void *priv;
+};
+
+int spi_nand_register(struct spi_nand *snand, struct nand_flash_dev *flash_ids);
+void spi_nand_unregister(struct spi_nand *snand);
+
+#endif
-- 
2.1.0

Re: [PATCH 4/6] mtd: Introduce SPI NAND framework

[Daniel Ehrenberg]

On Tue, Dec 2, 2014 at 4:58 AM, Ezequiel Garcia
<ezequiel.garcia@imgtec.com> wrote:
> +static int spi_nand_read_page(struct spi_nand *snand, unsigned int page_addr,
> +                             unsigned int page_offset, size_t length)
> +{
> +       unsigned int corrected = 0, ecc_error = 0;
> +       int ret;
> +
> +       /* Load a page into the cache register */
> +       ret = snand->load_page(snand, page_addr);
> +       if (ret < 0) {
> +               dev_err(snand->dev, "error %d loading page 0x%x to cache\n",
> +                       ret, page_addr);
> +               return ret;
> +       }
> +
> +       ret = spi_nand_wait_till_ready(snand);
> +       if (ret < 0)
> +               return ret;
> +
> +       if (snand->ecc) {
> +               snand->get_ecc_status(ret, &corrected, &ecc_error);
> +               snand->bitflips = corrected;
> +
> +               /*
> +                * 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) {
> +                       dev_err(snand->dev,
> +                               "internal ECC error reading page 0x%x\n",
> +                               page_addr);
> +                       snand->mtd.ecc_stats.failed++;
> +               }
> +       }
> +
> +       /* Get page from the device cache into our internal buffer */
> +       ret = snand->read_cache(snand, page_offset, length, snand->data_buf);
> +       if (ret < 0) {
> +               dev_err(snand->dev, "error %d reading page 0x%x from cache\n",
> +                       ret, page_addr);
> +               return ret;
> +       }
> +       return 0;
> +}

Should you increment snand->mtd.corrected by corrected in this function?

Dan

Re: [PATCH 4/6] mtd: Introduce SPI NAND framework

[Ezequiel Garcia]

On 12/15/2014 06:18 PM, Daniel Ehrenberg wrote:
> On Tue, Dec 2, 2014 at 4:58 AM, Ezequiel Garcia
> <ezequiel.garcia@imgtec.com> wrote:
>> +static int spi_nand_read_page(struct spi_nand *snand, unsigned int page_addr,
>> +                             unsigned int page_offset, size_t length)
>> +{
>> +       unsigned int corrected = 0, ecc_error = 0;
>> +       int ret;
>> +
>> +       /* Load a page into the cache register */
>> +       ret = snand->load_page(snand, page_addr);
>> +       if (ret < 0) {
>> +               dev_err(snand->dev, "error %d loading page 0x%x to cache\n",
>> +                       ret, page_addr);
>> +               return ret;
>> +       }
>> +
>> +       ret = spi_nand_wait_till_ready(snand);
>> +       if (ret < 0)
>> +               return ret;
>> +
>> +       if (snand->ecc) {
>> +               snand->get_ecc_status(ret, &corrected, &ecc_error);
>> +               snand->bitflips = corrected;
>> +
>> +               /*
>> +                * 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) {
>> +                       dev_err(snand->dev,
>> +                               "internal ECC error reading page 0x%x\n",
>> +                               page_addr);
>> +                       snand->mtd.ecc_stats.failed++;
>> +               }
>> +       }
>> +
>> +       /* Get page from the device cache into our internal buffer */
>> +       ret = snand->read_cache(snand, page_offset, length, snand->data_buf);
>> +       if (ret < 0) {
>> +               dev_err(snand->dev, "error %d reading page 0x%x from cache\n",
>> +                       ret, page_addr);
>> +               return ret;
>> +       }
>> +       return 0;
>> +}
> 
> Should you increment snand->mtd.corrected by corrected in this function?
> 

Yes, it looks like I missed that.

Thanks for the feedback,
-- 
Ezequiel

[PATCH 4/6] mtd: Introduce SPI NAND framework

[Qi Wang 王起 (qiwang)]

Hi Ezequiel,

> +/*
> + * Wait until the status register busy bit is cleared.
> + * Returns a negatie errno on error or time out, and a non-negative
> +status
> + * value if the device is ready.
> + */
> +static int spi_nand_wait_till_ready(struct spi_nand *snand) {
> +	unsigned long deadline = jiffies + msecs_to_jiffies(100);

100ms will be applied to all operation, but I think it would be more
make sense to use different timeout value for different operation, 
just like Parallel NAND as below:
"
static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
{

	int status, state = chip->state;
	unsigned long timeo = (state == FL_ERASING ? 400 : 20);
"	


> +static int spi_nand_write(struct spi_nand *snand) {
> +	int ret;
> +
> +	/* Store the page to cache */
> +	ret = snand->store_cache(snand, 0, snand->buf_size, snand-
> >data_buf);
> +	if (ret < 0) {
> +		dev_err(snand->dev, "error %d storing page 0x%x to cache\n",
> +			ret, snand->page_addr);
> +		return ret;
> +	}
> +
> +	ret = spi_nand_wait_till_ready(snand);
> +	if (ret)
> +		return ret;
> +
> +	ret = snand->write_enable(snand);
> +	if (ret < 0) {
> +		dev_err(snand->dev, "write enable command failed\n");
> +		return ret;
> +	}
> +
> +	/* Get page from the device cache into our internal buffer */
> +	ret = snand->write_page(snand, snand->page_addr);


The page program sequence in datasheet is write_enable->program_load->
program_execute->read_status. Seems different with the procedure in 
your code.
	
Thanks
- Qi Wang

Re: [PATCH 4/6] mtd: Introduce SPI NAND framework

[Ezequiel Garcia]

Dear Qi Wang,

Thanks for the review,

On 12/22/2014 01:34 AM, Qi Wang 王起 (qiwang) wrote:
> Hi Ezequiel,
> 
>> +/*
>> + * Wait until the status register busy bit is cleared.
>> + * Returns a negatie errno on error or time out, and a non-negative
>> +status
>> + * value if the device is ready.
>> + */
>> +static int spi_nand_wait_till_ready(struct spi_nand *snand) {
>> +	unsigned long deadline = jiffies + msecs_to_jiffies(100);
> 
> 100ms will be applied to all operation, but I think it would be more
> make sense to use different timeout value for different operation, 
> just like Parallel NAND as below:
> "

Yes, indeed. You are right. Now we need to find out the appropriate
value in each case. Any suggestions?

> static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
> {
> 
> 	int status, state = chip->state;
> 	unsigned long timeo = (state == FL_ERASING ? 400 : 20);
> "	
> 
> 
>> +static int spi_nand_write(struct spi_nand *snand) {
>> +	int ret;
>> +
>> +	/* Store the page to cache */
>> +	ret = snand->store_cache(snand, 0, snand->buf_size, snand-
>>> data_buf);
>> +	if (ret < 0) {
>> +		dev_err(snand->dev, "error %d storing page 0x%x to cache\n",
>> +			ret, snand->page_addr);
>> +		return ret;
>> +	}
>> +
>> +	ret = spi_nand_wait_till_ready(snand);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = snand->write_enable(snand);
>> +	if (ret < 0) {
>> +		dev_err(snand->dev, "write enable command failed\n");
>> +		return ret;
>> +	}
>> +
>> +	/* Get page from the device cache into our internal buffer */
>> +	ret = snand->write_page(snand, snand->page_addr);
> 
> 
> The page program sequence in datasheet is write_enable->program_load->
> program_execute->read_status. Seems different with the procedure in 
> your code.
> 	

Right. I thought calling write_enable() before actually programming the
page would be enough. Do you think we should do it before the cache
programming instead?

And what happens if it fails, will we need to call write_disable explicitly?

-- 
Ezequiel

Re: [PATCH 4/6] mtd: Introduce SPI NAND framework

[Brian Norris]

On Mon, Dec 22, 2014 at 12:44:23PM -0300, Ezequiel Garcia wrote:
> On 12/22/2014 01:34 AM, Qi Wang 王起 (qiwang) wrote:
> >> +/*
> >> + * Wait until the status register busy bit is cleared.
> >> + * Returns a negatie errno on error or time out, and a non-negative
> >> +status
> >> + * value if the device is ready.
> >> + */
> >> +static int spi_nand_wait_till_ready(struct spi_nand *snand) {
> >> +	unsigned long deadline = jiffies + msecs_to_jiffies(100);
> > 
> > 100ms will be applied to all operation, but I think it would be more
> > make sense to use different timeout value for different operation, 
> > just like Parallel NAND as below:
> > "
> 
> Yes, indeed. You are right. Now we need to find out the appropriate
> value in each case. Any suggestions?

Isn't the value of 'deadline' only important for the uncommon case of a
misbehaving flash? If so, I don't think it's too important to tune it to
be as small as possible.

> > static int nand_wait(struct mtd_info *mtd, struct nand_chip *chip)
> > {
> > 
> > 	int status, state = chip->state;
> > 	unsigned long timeo = (state == FL_ERASING ? 400 : 20);
> > "	

Brian

[PATCH 5/6] mtd: spi-nand: Add devicetree binding

[Ezequiel Garcia]

This commit adds the devicetree binding document that specifies the
SPI NAND devices support.

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
---
 Documentation/devicetree/bindings/mtd/spi-nand.txt | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mtd/spi-nand.txt

diff --git a/Documentation/devicetree/bindings/mtd/spi-nand.txt b/Documentation/devicetree/bindings/mtd/spi-nand.txt
new file mode 100644
index 0000000..1c5e412
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/spi-nand.txt
@@ -0,0 +1,21 @@
+* NAND driver for MT29F, GD5F and similar serial NAND flash chips
+
+Required properties:
+- #address-cells, #size-cells : Must be present if the device has sub-nodes
+  representing partitions.
+- compatible : Should be the manufacturer and the name of the chip. Bear in mind
+               the DT binding is not Linux-only, but in case of Linux, see the
+               "spi_nand_id_table" array in drivers/mtd/spi-nand/spi-nand-devices.c
+               for the list of supported chips.
+- reg : Chip-Select number
+- spi-max-frequency : Maximum frequency of the SPI bus the chip can operate at
+
+Example:
+
+	flash: flash@0 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "gigadevice,gd5f";
+		reg = <0>;
+		spi-max-frequency = <40000000>;
+	};
-- 
2.1.0

Re: [PATCH 5/6] mtd: spi-nand: Add devicetree binding

[Daniel Ehrenberg]

On Tue, Dec 2, 2014 at 4:58 AM, Ezequiel Garcia
<ezequiel.garcia@imgtec.com> wrote:
> This commit adds the devicetree binding document that specifies the
> SPI NAND devices support.
>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>

Reviewed-by: Dan Ehrenberg <dehrenberg@chromium.org>

[PATCH 6/6] mtd: spi-nand: Support common SPI NAND devices

[Ezequiel Garcia]

This commit uses the recently introduced SPI NAND framework to support
Micron MT29F and Gigadevice GD5F serial NAND devices. These two families
of devices are fairly similar and so they are supported with only minimal
differences.

The current support includes:

  * Page read and page program operations (using on-die ECC)
  * Page out-of-band read
  * Erase
  * Reset
  * Device status retrieval
  * Device ID retrieval

Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
---
 drivers/mtd/spi-nand/Kconfig           |  12 +
 drivers/mtd/spi-nand/Makefile          |   1 +
 drivers/mtd/spi-nand/spi-nand-device.c | 500 +++++++++++++++++++++++++++++++++
 3 files changed, 513 insertions(+)
 create mode 100644 drivers/mtd/spi-nand/spi-nand-device.c

diff --git a/drivers/mtd/spi-nand/Kconfig b/drivers/mtd/spi-nand/Kconfig
index 3868477..df29abe 100644
--- a/drivers/mtd/spi-nand/Kconfig
+++ b/drivers/mtd/spi-nand/Kconfig
@@ -4,3 +4,15 @@ menuconfig MTD_SPI_NAND
 	select MTD_NAND
 	help
 	  This is the framework for the SPI NAND.
+
+if MTD_SPI_NAND
+
+config MTD_SPI_NAND_DEVICES
+	tristate "Support for SPI NAND devices (MT29F, GD5F)"
+	default y
+	depends on MTD_SPI_NAND
+	help
+	  Select this option if you require support for the most common SPI NAND
+	  devices such as mt29f and gd5f.
+
+endif # MTD_SPI_NAND
diff --git a/drivers/mtd/spi-nand/Makefile b/drivers/mtd/spi-nand/Makefile
index d454c52..f4f95b7 100644
--- a/drivers/mtd/spi-nand/Makefile
+++ b/drivers/mtd/spi-nand/Makefile
@@ -1 +1,2 @@
 obj-$(CONFIG_MTD_SPI_NAND)		+= spi-nand-base.o
+obj-$(CONFIG_MTD_SPI_NAND_DEVICES)	+= spi-nand-device.o
diff --git a/drivers/mtd/spi-nand/spi-nand-device.c b/drivers/mtd/spi-nand/spi-nand-device.c
new file mode 100644
index 0000000..73050f3
--- /dev/null
+++ b/drivers/mtd/spi-nand/spi-nand-device.c
@@ -0,0 +1,500 @@
+/*
+ * Copyright (C) 2014 Imagination Technologies Ltd.
+ *
+ * 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; version 2 of the License.
+ *
+ * Notes:
+ * 1. We avoid using a stack-allocated buffer for SPI messages. Using
+ *    a kmalloced buffer is probably better, given we shouldn't assume
+ *    any particular usage by SPI core.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+#include <linux/mtd/spi-nand.h>
+#include <linux/sizes.h>
+#include <linux/spi/spi.h>
+
+/* SPI NAND commands */
+#define	SPI_NAND_WRITE_ENABLE		0x06
+#define	SPI_NAND_WRITE_DISABLE		0x04
+#define	SPI_NAND_GET_FEATURE		0x0f
+#define	SPI_NAND_SET_FEATURE		0x1f
+#define	SPI_NAND_PAGE_READ		0x13
+#define	SPI_NAND_READ_CACHE		0x03
+#define	SPI_NAND_FAST_READ_CACHE	0x0b
+#define	SPI_NAND_READ_CACHE_X2		0x3b
+#define	SPI_NAND_READ_CACHE_X4		0x6b
+#define	SPI_NAND_READ_CACHE_DUAL_IO	0xbb
+#define	SPI_NAND_READ_CACHE_QUAD_IO	0xeb
+#define	SPI_NAND_READ_ID		0x9f
+#define	SPI_NAND_PROGRAM_LOAD		0x02
+#define	SPI_NAND_PROGRAM_LOAD4		0x32
+#define	SPI_NAND_PROGRAM_EXEC		0x10
+#define	SPI_NAND_PROGRAM_LOAD_RANDOM	0x84
+#define	SPI_NAND_PROGRAM_LOAD_RANDOM4	0xc4
+#define	SPI_NAND_BLOCK_ERASE		0xd8
+#define	SPI_NAND_RESET			0xff
+
+#define SPI_NAND_GD5F_READID_LEN	2
+#define SPI_NAND_MT29F_READID_LEN	2
+
+#define SPI_NAND_GD5F_ECC_MASK		(BIT(0) | BIT(1) | BIT(2))
+#define SPI_NAND_GD5F_ECC_UNCORR	(BIT(0) | BIT(1) | BIT(2))
+#define SPI_NAND_GD5F_ECC_SHIFT		4
+
+#define SPI_NAND_MT29F_ECC_MASK		(BIT(0) | BIT(1))
+#define SPI_NAND_MT29F_ECC_UNCORR	(BIT(1))
+#define SPI_NAND_MT29F_ECC_SHIFT		4
+
+static struct nand_ecclayout ecc_layout_gd5f = {
+	.eccbytes = 128,
+	.eccpos = {
+		128, 129, 130, 131, 132, 133, 134, 135,
+		136, 137, 138, 139, 140, 141, 142, 143,
+		144, 145, 146, 147, 148, 149, 150, 151,
+		152, 153, 154, 155, 156, 157, 158, 159,
+		160, 161, 162, 163, 164, 165, 166, 167,
+		168, 169, 170, 171, 172, 173, 174, 175,
+		176, 177, 178, 179, 180, 181, 182, 183,
+		184, 185, 186, 187, 188, 189, 190, 191,
+		192, 193, 194, 195, 196, 197, 198, 199,
+		200, 201, 202, 203, 204, 205, 206, 207,
+		208, 209, 210, 211, 212, 213, 214, 215,
+		216, 217, 218, 219, 220, 221, 222, 223,
+		224, 225, 226, 227, 228, 229, 230, 231,
+		232, 233, 234, 235, 236, 237, 238, 239,
+		240, 241, 242, 243, 244, 245, 246, 247,
+		248, 249, 250, 251, 252, 253, 254, 255
+	},
+	.oobfree = { {1, 127} }
+};
+
+static struct nand_ecclayout ecc_layout_mt29f = {
+	.eccbytes = 32,
+	.eccpos = {
+		8, 9, 10, 11, 12, 13, 14, 15,
+		24, 25, 26, 27, 28, 29, 30, 31,
+		40, 41, 42, 43, 44, 45, 46, 47,
+		56, 57, 58, 59, 60, 61, 62, 63,
+	 },
+};
+
+static struct nand_flash_dev spi_nand_flash_ids[] = {
+	{
+		.name = "SPI NAND 512MiB 3,3V",
+		.id = { NAND_MFR_GIGADEVICE, 0xb4 },
+		.chipsize = 512,
+		.pagesize = SZ_4K,
+		.erasesize = SZ_256K,
+		.id_len = 2,
+		.oobsize = 256,
+		.ecc.strength_ds = 8,
+		.ecc.step_ds = 512,
+		.ecc.layout = &ecc_layout_gd5f,
+	},
+	{
+		.name = "SPI NAND 512MiB 1,8V",
+		.id = { NAND_MFR_GIGADEVICE, 0xa4 },
+		.chipsize = 512,
+		.pagesize = SZ_4K,
+		.erasesize = SZ_256K,
+		.id_len = 2,
+		.oobsize = 256,
+		.ecc.strength_ds = 8,
+		.ecc.step_ds = 512,
+		.ecc.layout = &ecc_layout_gd5f,
+	},
+	{
+		.name = "SPI NAND 512MiB 3,3V",
+		.id = { NAND_MFR_MICRON, 0x32 },
+		.chipsize = 512,
+		.pagesize = SZ_2K,
+		.erasesize = SZ_128K,
+		.id_len = 2,
+		.oobsize = 64,
+		.ecc.strength_ds = 4,
+		.ecc.step_ds = 512,
+		.ecc.layout = &ecc_layout_mt29f,
+	},
+	{
+		.name = "SPI NAND 256MiB 3,3V",
+		.id = { NAND_MFR_MICRON, 0x22 },
+		.chipsize = 256,
+		.pagesize = SZ_2K,
+		.erasesize = SZ_128K,
+		.id_len = 2,
+		.oobsize = 64,
+		.ecc.strength_ds = 4,
+		.ecc.step_ds = 512,
+		.ecc.layout = &ecc_layout_mt29f,
+	},
+};
+
+enum spi_nand_device_variant {
+	SPI_NAND_GENERIC,
+	SPI_NAND_MT29F,
+	SPI_NAND_GD5F,
+};
+
+struct spi_nand_device_cmd {
+	u8 cmd;
+	u32 n_addr;
+	u8 addr[3];
+	u32 n_tx;
+	u8 *tx_buf;
+	u32 n_rx;
+	u8 *rx_buf;
+};
+
+struct spi_nand_device {
+	struct spi_nand	spi_nand;
+	struct spi_device *spi;
+
+	struct spi_nand_device_cmd cmd;
+};
+
+static int spi_nand_send_command(struct spi_device *spi, int command,
+				 struct spi_nand_device_cmd *cmd)
+{
+	struct spi_message message;
+	struct spi_transfer x[4];
+
+	spi_message_init(&message);
+	memset(x, 0, sizeof(x));
+
+	/* Command */
+	cmd->cmd = command;
+	x[0].len = 1;
+	x[0].tx_buf = &cmd->cmd;
+	spi_message_add_tail(&x[0], &message);
+
+	/* Address */
+	if (cmd->n_addr) {
+		x[1].len = cmd->n_addr;
+		x[1].tx_buf = cmd->addr;
+		spi_message_add_tail(&x[1], &message);
+	}
+
+	/* Data to be transmitted */
+	if (cmd->n_tx) {
+		x[3].len = cmd->n_tx;
+		x[3].tx_buf = cmd->tx_buf;
+		spi_message_add_tail(&x[3], &message);
+	}
+
+	/* Data to be received */
+	if (cmd->n_rx) {
+		x[3].len = cmd->n_rx;
+		x[3].rx_buf = cmd->rx_buf;
+		spi_message_add_tail(&x[3], &message);
+	}
+
+	return spi_sync(spi, &message);
+}
+
+static int spi_nand_device_reset(struct spi_nand *snand)
+{
+	struct spi_nand_device *snand_dev = snand->priv;
+	struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
+
+	memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
+
+	dev_dbg(snand->dev, "%s\n", __func__);
+
+	return spi_nand_send_command(snand_dev->spi, SPI_NAND_RESET, cmd);
+}
+
+static int spi_nand_device_read_reg(struct spi_nand *snand, u8 opcode, u8 *buf)
+{
+	struct spi_nand_device *snand_dev = snand->priv;
+	struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
+
+	memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
+	cmd->n_addr = 1;
+	cmd->addr[0] = opcode;
+	cmd->n_rx = 1;
+	cmd->rx_buf = buf;
+
+	dev_dbg(snand->dev, "%s: reg 0%x\n", __func__, opcode);
+
+	return spi_nand_send_command(snand_dev->spi, SPI_NAND_GET_FEATURE, cmd);
+}
+
+static int spi_nand_device_write_reg(struct spi_nand *snand, u8 opcode, u8 *buf)
+{
+	struct spi_nand_device *snand_dev = snand->priv;
+	struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
+
+	memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
+	cmd->n_addr = 1;
+	cmd->addr[0] = opcode;
+	cmd->n_tx = 1;
+	cmd->tx_buf = buf;
+
+	dev_dbg(snand->dev, "%s: reg 0%x\n", __func__, opcode);
+
+	return spi_nand_send_command(snand_dev->spi, SPI_NAND_SET_FEATURE, cmd);
+}
+
+static int spi_nand_device_write_enable(struct spi_nand *snand)
+{
+	struct spi_nand_device *snand_dev = snand->priv;
+	struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
+
+	memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
+
+	dev_dbg(snand->dev, "%s\n", __func__);
+
+	return spi_nand_send_command(snand_dev->spi, SPI_NAND_WRITE_ENABLE, cmd);
+}
+
+static int spi_nand_device_write_disable(struct spi_nand *snand)
+{
+	struct spi_nand_device *snand_dev = snand->priv;
+	struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
+
+	memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
+
+	dev_dbg(snand->dev, "%s\n", __func__);
+
+	return spi_nand_send_command(snand_dev->spi, SPI_NAND_WRITE_DISABLE, cmd);
+}
+
+static int spi_nand_device_write_page(struct spi_nand *snand, unsigned int page_addr)
+{
+	struct spi_nand_device *snand_dev = snand->priv;
+	struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
+
+	memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
+	cmd->n_addr = 3;
+	cmd->addr[0] = (u8)((page_addr & 0xff0000) >> 16);
+	cmd->addr[1] = (u8)((page_addr & 0xff00) >> 8);
+	cmd->addr[2] = (u8)(page_addr & 0xff);
+
+	dev_dbg(snand->dev, "%s: page 0x%x\n", __func__, page_addr);
+
+	return spi_nand_send_command(snand_dev->spi, SPI_NAND_PROGRAM_EXEC, cmd);
+}
+
+static int spi_nand_device_store_cache(struct spi_nand *snand,
+				       unsigned int page_offset, size_t length,
+				       u8 *write_buf)
+{
+	struct spi_nand_device *snand_dev = snand->priv;
+	struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
+
+	memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
+	cmd->n_addr = 2;
+	cmd->addr[0] = (u8)((page_offset & 0xff00) >> 8);
+	cmd->addr[1] = (u8)(page_offset & 0xff);
+	cmd->n_tx = length;
+	cmd->tx_buf = write_buf;
+
+	dev_dbg(snand->dev, "%s: offset 0x%x\n", __func__, page_offset);
+
+	return spi_nand_send_command(snand_dev->spi, SPI_NAND_PROGRAM_LOAD, cmd);
+}
+
+static int spi_nand_device_load_page(struct spi_nand *snand, unsigned int page_addr)
+{
+	struct spi_nand_device *snand_dev = snand->priv;
+	struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
+
+	memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
+	cmd->n_addr = 3;
+	cmd->addr[0] = (u8)((page_addr & 0xff0000) >> 16);
+	cmd->addr[1] = (u8)((page_addr & 0xff00) >> 8);
+	cmd->addr[2] = (u8)(page_addr & 0xff);
+
+	dev_dbg(snand->dev, "%s: page 0x%x\n", __func__, page_addr);
+
+	return spi_nand_send_command(snand_dev->spi, SPI_NAND_PAGE_READ, cmd);
+}
+
+static int spi_nand_device_read_cache(struct spi_nand *snand,
+				      unsigned int page_offset, size_t length,
+				      u8 *read_buf)
+{
+	struct spi_nand_device *snand_dev = snand->priv;
+	struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
+
+	memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
+	cmd->n_addr = 3;
+	cmd->addr[0] = 0;
+	cmd->addr[1] = (u8)((page_offset & 0xff00) >> 8);
+	cmd->addr[2] = (u8)(page_offset & 0xff);
+	cmd->n_rx = length;
+	cmd->rx_buf = read_buf;
+
+	dev_dbg(snand->dev, "%s: offset 0x%x\n", __func__, page_offset);
+
+	return spi_nand_send_command(snand_dev->spi, SPI_NAND_READ_CACHE, cmd);
+
+	return 0;
+}
+
+static int spi_nand_device_block_erase(struct spi_nand *snand, unsigned int page_addr)
+{
+	struct spi_nand_device *snand_dev = snand->priv;
+	struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
+
+	memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
+	cmd->n_addr = 3;
+	cmd->addr[0] = 0;
+	cmd->addr[1] = (u8)((page_addr & 0xff00) >> 8);
+	cmd->addr[2] = (u8)(page_addr & 0xff);
+
+	dev_dbg(snand->dev, "%s: block 0x%x\n", __func__, page_addr);
+
+	return spi_nand_send_command(snand_dev->spi, SPI_NAND_BLOCK_ERASE, cmd);
+}
+
+static int spi_nand_gd5f_read_id(struct spi_nand *snand, u8 *buf)
+{
+	struct spi_nand_device *snand_dev = snand->priv;
+	struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
+
+	memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
+	cmd->n_rx = SPI_NAND_GD5F_READID_LEN;
+	cmd->rx_buf = buf;
+
+	dev_dbg(snand->dev, "%s\n", __func__);
+
+	return spi_nand_send_command(snand_dev->spi, SPI_NAND_READ_ID, cmd);
+}
+
+static int spi_nand_mt29f_read_id(struct spi_nand *snand, u8 *buf)
+{
+	struct spi_nand_device *snand_dev = snand->priv;
+	struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
+
+	memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
+	cmd->n_rx = SPI_NAND_MT29F_READID_LEN;
+	cmd->rx_buf = buf;
+
+	dev_dbg(snand->dev, "%s\n", __func__);
+
+	return spi_nand_send_command(snand_dev->spi, SPI_NAND_READ_ID, cmd);
+}
+
+static void spi_nand_mt29f_ecc_status(unsigned int status,
+				      unsigned int *corrected,
+				      unsigned int *ecc_error)
+{
+	unsigned int ecc_status = (status >> SPI_NAND_MT29F_ECC_SHIFT) &
+					     SPI_NAND_MT29F_ECC_MASK;
+
+	*ecc_error = (ecc_status == SPI_NAND_MT29F_ECC_UNCORR) ? 1 : 0;
+	if (*ecc_error == 0)
+		*corrected = ecc_status;
+}
+
+static void spi_nand_gd5f_ecc_status(unsigned int status,
+				     unsigned int *corrected,
+				     unsigned int *ecc_error)
+{
+	unsigned int ecc_status = (status >> SPI_NAND_GD5F_ECC_SHIFT) &
+					     SPI_NAND_GD5F_ECC_MASK;
+
+	*ecc_error = (ecc_status == SPI_NAND_GD5F_ECC_UNCORR) ? 1 : 0;
+	if (*ecc_error == 0)
+		*corrected = 2 + ecc_status;
+}
+
+static int spi_nand_device_probe(struct spi_device *spi)
+{
+	enum spi_nand_device_variant variant;
+	struct spi_nand_device *priv;
+	struct spi_nand *snand;
+	int ret;
+
+	priv = devm_kzalloc(&spi->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	snand = &priv->spi_nand;
+
+	snand->read_cache = spi_nand_device_read_cache;
+	snand->load_page = spi_nand_device_load_page;
+	snand->store_cache = spi_nand_device_store_cache;
+	snand->write_page = spi_nand_device_write_page;
+	snand->write_reg = spi_nand_device_write_reg;
+	snand->read_reg = spi_nand_device_read_reg;
+	snand->block_erase = spi_nand_device_block_erase;
+	snand->reset = spi_nand_device_reset;
+	snand->write_enable = spi_nand_device_write_enable;
+	snand->write_disable = spi_nand_device_write_disable;
+	snand->dev = &spi->dev;
+	snand->priv = priv;
+
+	/*
+	 * gd5f reads three ID bytes, and mt29f reads one dummy address byte
+	 * and two ID bytes. Therefore, we could detect both in the same
+	 * read_id implementation by reading _with_ and _without_ a dummy byte,
+	 * until a proper manufacturer is found.
+	 *
+	 * This'll mean we won't need to specify any specific compatible string
+	 * for a given device, and instead just support spi-nand.
+	 */
+	variant = spi_get_device_id(spi)->driver_data;
+	switch (variant) {
+	case SPI_NAND_MT29F:
+		snand->read_id = spi_nand_mt29f_read_id;
+		snand->get_ecc_status = spi_nand_mt29f_ecc_status;
+		break;
+	case SPI_NAND_GD5F:
+		snand->read_id = spi_nand_gd5f_read_id;
+		snand->get_ecc_status = spi_nand_gd5f_ecc_status;
+		break;
+	default:
+		dev_err(snand->dev, "unknown device\n");
+		return -ENODEV;
+	}
+
+	spi_set_drvdata(spi, snand);
+	priv->spi = spi;
+
+	ret = spi_nand_register(snand, spi_nand_flash_ids);
+	if (ret)
+		return ret;
+	return 0;
+}
+
+static int spi_nand_device_remove(struct spi_device *spi)
+{
+	struct spi_nand *snand = spi_get_drvdata(spi);
+
+	spi_nand_unregister(snand);
+
+	return 0;
+}
+
+const struct spi_device_id spi_nand_id_table[] = {
+	{ "spi-nand", SPI_NAND_GENERIC },
+	{ "mt29f", SPI_NAND_MT29F },
+	{ "gd5f", SPI_NAND_GD5F },
+	{ },
+};
+MODULE_DEVICE_TABLE(spi, spi_nand_id_table);
+
+static struct spi_driver spi_nand_device_driver = {
+	.driver = {
+		.name	= "spi_nand_device",
+		.owner	= THIS_MODULE,
+	},
+	.id_table = spi_nand_id_table,
+	.probe	= spi_nand_device_probe,
+	.remove	= spi_nand_device_remove,
+};
+module_spi_driver(spi_nand_device_driver);
+
+MODULE_AUTHOR("Ezequiel Garcia <ezequiel.garcia@imgtec.com>");
+MODULE_DESCRIPTION("SPI NAND device support");
+MODULE_LICENSE("GPL v2");
-- 
2.1.0

Re: [PATCH 6/6] mtd: spi-nand: Support common SPI NAND devices

[Daniel Ehrenberg]

On Tue, Dec 2, 2014 at 4:58 AM, Ezequiel Garcia
<ezequiel.garcia@imgtec.com> wrote:
> This commit uses the recently introduced SPI NAND framework to support
> Micron MT29F and Gigadevice GD5F serial NAND devices. These two families
> of devices are fairly similar and so they are supported with only minimal
> differences.
>
> The current support includes:
>
>   * Page read and page program operations (using on-die ECC)
>   * Page out-of-band read
>   * Erase
>   * Reset
>   * Device status retrieval
>   * Device ID retrieval
>
> Signed-off-by: Ezequiel Garcia <ezequiel.garcia@imgtec.com>
> ---
>  drivers/mtd/spi-nand/Kconfig           |  12 +
>  drivers/mtd/spi-nand/Makefile          |   1 +
>  drivers/mtd/spi-nand/spi-nand-device.c | 500 +++++++++++++++++++++++++++++++++
>  3 files changed, 513 insertions(+)
>  create mode 100644 drivers/mtd/spi-nand/spi-nand-device.c
>
> diff --git a/drivers/mtd/spi-nand/Kconfig b/drivers/mtd/spi-nand/Kconfig
> index 3868477..df29abe 100644
> --- a/drivers/mtd/spi-nand/Kconfig
> +++ b/drivers/mtd/spi-nand/Kconfig
> @@ -4,3 +4,15 @@ menuconfig MTD_SPI_NAND
>         select MTD_NAND
>         help
>           This is the framework for the SPI NAND.
> +
> +if MTD_SPI_NAND
> +
> +config MTD_SPI_NAND_DEVICES
> +       tristate "Support for SPI NAND devices (MT29F, GD5F)"
> +       default y
> +       depends on MTD_SPI_NAND
> +       help
> +         Select this option if you require support for the most common SPI NAND
> +         devices such as mt29f and gd5f.
> +
> +endif # MTD_SPI_NAND
> diff --git a/drivers/mtd/spi-nand/Makefile b/drivers/mtd/spi-nand/Makefile
> index d454c52..f4f95b7 100644
> --- a/drivers/mtd/spi-nand/Makefile
> +++ b/drivers/mtd/spi-nand/Makefile
> @@ -1 +1,2 @@
>  obj-$(CONFIG_MTD_SPI_NAND)             += spi-nand-base.o
> +obj-$(CONFIG_MTD_SPI_NAND_DEVICES)     += spi-nand-device.o
> diff --git a/drivers/mtd/spi-nand/spi-nand-device.c b/drivers/mtd/spi-nand/spi-nand-device.c
> new file mode 100644
> index 0000000..73050f3
> --- /dev/null
> +++ b/drivers/mtd/spi-nand/spi-nand-device.c
> @@ -0,0 +1,500 @@
> +/*
> + * Copyright (C) 2014 Imagination Technologies Ltd.
> + *
> + * 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; version 2 of the License.
> + *
> + * Notes:
> + * 1. We avoid using a stack-allocated buffer for SPI messages. Using
> + *    a kmalloced buffer is probably better, given we shouldn't assume
> + *    any particular usage by SPI core.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/errno.h>
> +#include <linux/module.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/partitions.h>
> +#include <linux/mtd/spi-nand.h>
> +#include <linux/sizes.h>
> +#include <linux/spi/spi.h>
> +
> +/* SPI NAND commands */
> +#define        SPI_NAND_WRITE_ENABLE           0x06
> +#define        SPI_NAND_WRITE_DISABLE          0x04
> +#define        SPI_NAND_GET_FEATURE            0x0f
> +#define        SPI_NAND_SET_FEATURE            0x1f
> +#define        SPI_NAND_PAGE_READ              0x13
> +#define        SPI_NAND_READ_CACHE             0x03
> +#define        SPI_NAND_FAST_READ_CACHE        0x0b
> +#define        SPI_NAND_READ_CACHE_X2          0x3b
> +#define        SPI_NAND_READ_CACHE_X4          0x6b
> +#define        SPI_NAND_READ_CACHE_DUAL_IO     0xbb
> +#define        SPI_NAND_READ_CACHE_QUAD_IO     0xeb
> +#define        SPI_NAND_READ_ID                0x9f
> +#define        SPI_NAND_PROGRAM_LOAD           0x02
> +#define        SPI_NAND_PROGRAM_LOAD4          0x32
> +#define        SPI_NAND_PROGRAM_EXEC           0x10
> +#define        SPI_NAND_PROGRAM_LOAD_RANDOM    0x84
> +#define        SPI_NAND_PROGRAM_LOAD_RANDOM4   0xc4
> +#define        SPI_NAND_BLOCK_ERASE            0xd8
> +#define        SPI_NAND_RESET                  0xff
> +
> +#define SPI_NAND_GD5F_READID_LEN       2
> +#define SPI_NAND_MT29F_READID_LEN      2
> +
> +#define SPI_NAND_GD5F_ECC_MASK         (BIT(0) | BIT(1) | BIT(2))
> +#define SPI_NAND_GD5F_ECC_UNCORR       (BIT(0) | BIT(1) | BIT(2))
> +#define SPI_NAND_GD5F_ECC_SHIFT                4
> +
> +#define SPI_NAND_MT29F_ECC_MASK                (BIT(0) | BIT(1))
> +#define SPI_NAND_MT29F_ECC_UNCORR      (BIT(1))
> +#define SPI_NAND_MT29F_ECC_SHIFT               4
> +
> +static struct nand_ecclayout ecc_layout_gd5f = {
> +       .eccbytes = 128,
> +       .eccpos = {
> +               128, 129, 130, 131, 132, 133, 134, 135,
> +               136, 137, 138, 139, 140, 141, 142, 143,
> +               144, 145, 146, 147, 148, 149, 150, 151,
> +               152, 153, 154, 155, 156, 157, 158, 159,
> +               160, 161, 162, 163, 164, 165, 166, 167,
> +               168, 169, 170, 171, 172, 173, 174, 175,
> +               176, 177, 178, 179, 180, 181, 182, 183,
> +               184, 185, 186, 187, 188, 189, 190, 191,
> +               192, 193, 194, 195, 196, 197, 198, 199,
> +               200, 201, 202, 203, 204, 205, 206, 207,
> +               208, 209, 210, 211, 212, 213, 214, 215,
> +               216, 217, 218, 219, 220, 221, 222, 223,
> +               224, 225, 226, 227, 228, 229, 230, 231,
> +               232, 233, 234, 235, 236, 237, 238, 239,
> +               240, 241, 242, 243, 244, 245, 246, 247,
> +               248, 249, 250, 251, 252, 253, 254, 255
> +       },
> +       .oobfree = { {1, 127} }
> +};
> +
> +static struct nand_ecclayout ecc_layout_mt29f = {
> +       .eccbytes = 32,
> +       .eccpos = {
> +               8, 9, 10, 11, 12, 13, 14, 15,
> +               24, 25, 26, 27, 28, 29, 30, 31,
> +               40, 41, 42, 43, 44, 45, 46, 47,
> +               56, 57, 58, 59, 60, 61, 62, 63,
> +        },
> +};
> +
> +static struct nand_flash_dev spi_nand_flash_ids[] = {
> +       {
> +               .name = "SPI NAND 512MiB 3,3V",
> +               .id = { NAND_MFR_GIGADEVICE, 0xb4 },
> +               .chipsize = 512,
> +               .pagesize = SZ_4K,
> +               .erasesize = SZ_256K,
> +               .id_len = 2,
> +               .oobsize = 256,
> +               .ecc.strength_ds = 8,
> +               .ecc.step_ds = 512,
> +               .ecc.layout = &ecc_layout_gd5f,
> +       },
> +       {
> +               .name = "SPI NAND 512MiB 1,8V",
> +               .id = { NAND_MFR_GIGADEVICE, 0xa4 },
> +               .chipsize = 512,
> +               .pagesize = SZ_4K,
> +               .erasesize = SZ_256K,
> +               .id_len = 2,
> +               .oobsize = 256,
> +               .ecc.strength_ds = 8,
> +               .ecc.step_ds = 512,
> +               .ecc.layout = &ecc_layout_gd5f,
> +       },
> +       {
> +               .name = "SPI NAND 512MiB 3,3V",
> +               .id = { NAND_MFR_MICRON, 0x32 },
> +               .chipsize = 512,
> +               .pagesize = SZ_2K,
> +               .erasesize = SZ_128K,
> +               .id_len = 2,
> +               .oobsize = 64,
> +               .ecc.strength_ds = 4,
> +               .ecc.step_ds = 512,
> +               .ecc.layout = &ecc_layout_mt29f,
> +       },
> +       {
> +               .name = "SPI NAND 256MiB 3,3V",
> +               .id = { NAND_MFR_MICRON, 0x22 },
> +               .chipsize = 256,
> +               .pagesize = SZ_2K,
> +               .erasesize = SZ_128K,
> +               .id_len = 2,
> +               .oobsize = 64,
> +               .ecc.strength_ds = 4,
> +               .ecc.step_ds = 512,
> +               .ecc.layout = &ecc_layout_mt29f,
> +       },
> +};
> +
> +enum spi_nand_device_variant {
> +       SPI_NAND_GENERIC,
> +       SPI_NAND_MT29F,
> +       SPI_NAND_GD5F,
> +};

What is the purpose of including SPI_NAND_GENERIC? It looks like it
doesn't work anyway.

> +
> +struct spi_nand_device_cmd {
> +       u8 cmd;
> +       u32 n_addr;
> +       u8 addr[3];
> +       u32 n_tx;
> +       u8 *tx_buf;
> +       u32 n_rx;
> +       u8 *rx_buf;
> +};
> +
> +struct spi_nand_device {
> +       struct spi_nand spi_nand;
> +       struct spi_device *spi;
> +
> +       struct spi_nand_device_cmd cmd;
> +};
> +
> +static int spi_nand_send_command(struct spi_device *spi, int command,
> +                                struct spi_nand_device_cmd *cmd)
> +{
> +       struct spi_message message;
> +       struct spi_transfer x[4];
> +
> +       spi_message_init(&message);
> +       memset(x, 0, sizeof(x));
> +
> +       /* Command */
> +       cmd->cmd = command;
> +       x[0].len = 1;
> +       x[0].tx_buf = &cmd->cmd;
> +       spi_message_add_tail(&x[0], &message);
> +
> +       /* Address */
> +       if (cmd->n_addr) {
> +               x[1].len = cmd->n_addr;
> +               x[1].tx_buf = cmd->addr;
> +               spi_message_add_tail(&x[1], &message);
> +       }
> +
> +       /* Data to be transmitted */
> +       if (cmd->n_tx) {
> +               x[3].len = cmd->n_tx;
> +               x[3].tx_buf = cmd->tx_buf;
> +               spi_message_add_tail(&x[3], &message);
> +       }
> +
> +       /* Data to be received */
> +       if (cmd->n_rx) {
> +               x[3].len = cmd->n_rx;
> +               x[3].rx_buf = cmd->rx_buf;
> +               spi_message_add_tail(&x[3], &message);
> +       }
> +
> +       return spi_sync(spi, &message);
> +}
> +
> +static int spi_nand_device_reset(struct spi_nand *snand)
> +{
> +       struct spi_nand_device *snand_dev = snand->priv;
> +       struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
> +
> +       memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
> +
> +       dev_dbg(snand->dev, "%s\n", __func__);
> +
> +       return spi_nand_send_command(snand_dev->spi, SPI_NAND_RESET, cmd);
> +}
> +
> +static int spi_nand_device_read_reg(struct spi_nand *snand, u8 opcode, u8 *buf)
> +{
> +       struct spi_nand_device *snand_dev = snand->priv;
> +       struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
> +
> +       memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
> +       cmd->n_addr = 1;
> +       cmd->addr[0] = opcode;
> +       cmd->n_rx = 1;
> +       cmd->rx_buf = buf;
> +
> +       dev_dbg(snand->dev, "%s: reg 0%x\n", __func__, opcode);
> +
> +       return spi_nand_send_command(snand_dev->spi, SPI_NAND_GET_FEATURE, cmd);
> +}
> +
> +static int spi_nand_device_write_reg(struct spi_nand *snand, u8 opcode, u8 *buf)
> +{
> +       struct spi_nand_device *snand_dev = snand->priv;
> +       struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
> +
> +       memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
> +       cmd->n_addr = 1;
> +       cmd->addr[0] = opcode;
> +       cmd->n_tx = 1;
> +       cmd->tx_buf = buf;
> +
> +       dev_dbg(snand->dev, "%s: reg 0%x\n", __func__, opcode);
> +
> +       return spi_nand_send_command(snand_dev->spi, SPI_NAND_SET_FEATURE, cmd);
> +}
> +
> +static int spi_nand_device_write_enable(struct spi_nand *snand)
> +{
> +       struct spi_nand_device *snand_dev = snand->priv;
> +       struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
> +
> +       memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
> +
> +       dev_dbg(snand->dev, "%s\n", __func__);
> +
> +       return spi_nand_send_command(snand_dev->spi, SPI_NAND_WRITE_ENABLE, cmd);
> +}
> +
> +static int spi_nand_device_write_disable(struct spi_nand *snand)
> +{
> +       struct spi_nand_device *snand_dev = snand->priv;
> +       struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
> +
> +       memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
> +
> +       dev_dbg(snand->dev, "%s\n", __func__);
> +
> +       return spi_nand_send_command(snand_dev->spi, SPI_NAND_WRITE_DISABLE, cmd);
> +}
> +
> +static int spi_nand_device_write_page(struct spi_nand *snand, unsigned int page_addr)
> +{
> +       struct spi_nand_device *snand_dev = snand->priv;
> +       struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
> +
> +       memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
> +       cmd->n_addr = 3;
> +       cmd->addr[0] = (u8)((page_addr & 0xff0000) >> 16);
> +       cmd->addr[1] = (u8)((page_addr & 0xff00) >> 8);
> +       cmd->addr[2] = (u8)(page_addr & 0xff);
> +
> +       dev_dbg(snand->dev, "%s: page 0x%x\n", __func__, page_addr);
> +
> +       return spi_nand_send_command(snand_dev->spi, SPI_NAND_PROGRAM_EXEC, cmd);
> +}
> +
> +static int spi_nand_device_store_cache(struct spi_nand *snand,
> +                                      unsigned int page_offset, size_t length,
> +                                      u8 *write_buf)
> +{
> +       struct spi_nand_device *snand_dev = snand->priv;
> +       struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
> +
> +       memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
> +       cmd->n_addr = 2;
> +       cmd->addr[0] = (u8)((page_offset & 0xff00) >> 8);
> +       cmd->addr[1] = (u8)(page_offset & 0xff);
> +       cmd->n_tx = length;
> +       cmd->tx_buf = write_buf;
> +
> +       dev_dbg(snand->dev, "%s: offset 0x%x\n", __func__, page_offset);
> +
> +       return spi_nand_send_command(snand_dev->spi, SPI_NAND_PROGRAM_LOAD, cmd);
> +}
> +
> +static int spi_nand_device_load_page(struct spi_nand *snand, unsigned int page_addr)
> +{
> +       struct spi_nand_device *snand_dev = snand->priv;
> +       struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
> +
> +       memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
> +       cmd->n_addr = 3;
> +       cmd->addr[0] = (u8)((page_addr & 0xff0000) >> 16);
> +       cmd->addr[1] = (u8)((page_addr & 0xff00) >> 8);
> +       cmd->addr[2] = (u8)(page_addr & 0xff);
> +
> +       dev_dbg(snand->dev, "%s: page 0x%x\n", __func__, page_addr);
> +
> +       return spi_nand_send_command(snand_dev->spi, SPI_NAND_PAGE_READ, cmd);
> +}
> +
> +static int spi_nand_device_read_cache(struct spi_nand *snand,
> +                                     unsigned int page_offset, size_t length,
> +                                     u8 *read_buf)
> +{
> +       struct spi_nand_device *snand_dev = snand->priv;
> +       struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
> +
> +       memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
> +       cmd->n_addr = 3;
> +       cmd->addr[0] = 0;
> +       cmd->addr[1] = (u8)((page_offset & 0xff00) >> 8);
> +       cmd->addr[2] = (u8)(page_offset & 0xff);
> +       cmd->n_rx = length;
> +       cmd->rx_buf = read_buf;
> +
> +       dev_dbg(snand->dev, "%s: offset 0x%x\n", __func__, page_offset);
> +
> +       return spi_nand_send_command(snand_dev->spi, SPI_NAND_READ_CACHE, cmd);
> +
> +       return 0;
> +}
> +
> +static int spi_nand_device_block_erase(struct spi_nand *snand, unsigned int page_addr)
> +{
> +       struct spi_nand_device *snand_dev = snand->priv;
> +       struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
> +
> +       memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
> +       cmd->n_addr = 3;
> +       cmd->addr[0] = 0;
> +       cmd->addr[1] = (u8)((page_addr & 0xff00) >> 8);
> +       cmd->addr[2] = (u8)(page_addr & 0xff);
> +
> +       dev_dbg(snand->dev, "%s: block 0x%x\n", __func__, page_addr);
> +
> +       return spi_nand_send_command(snand_dev->spi, SPI_NAND_BLOCK_ERASE, cmd);
> +}
> +
> +static int spi_nand_gd5f_read_id(struct spi_nand *snand, u8 *buf)
> +{
> +       struct spi_nand_device *snand_dev = snand->priv;
> +       struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
> +
> +       memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
> +       cmd->n_rx = SPI_NAND_GD5F_READID_LEN;
> +       cmd->rx_buf = buf;
> +
> +       dev_dbg(snand->dev, "%s\n", __func__);
> +
> +       return spi_nand_send_command(snand_dev->spi, SPI_NAND_READ_ID, cmd);
> +}
> +
> +static int spi_nand_mt29f_read_id(struct spi_nand *snand, u8 *buf)
> +{
> +       struct spi_nand_device *snand_dev = snand->priv;
> +       struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
> +
> +       memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
> +       cmd->n_rx = SPI_NAND_MT29F_READID_LEN;
> +       cmd->rx_buf = buf;
> +
> +       dev_dbg(snand->dev, "%s\n", __func__);
> +
> +       return spi_nand_send_command(snand_dev->spi, SPI_NAND_READ_ID, cmd);
> +}
> +
> +static void spi_nand_mt29f_ecc_status(unsigned int status,
> +                                     unsigned int *corrected,
> +                                     unsigned int *ecc_error)
> +{
> +       unsigned int ecc_status = (status >> SPI_NAND_MT29F_ECC_SHIFT) &
> +                                            SPI_NAND_MT29F_ECC_MASK;
> +
> +       *ecc_error = (ecc_status == SPI_NAND_MT29F_ECC_UNCORR) ? 1 : 0;
> +       if (*ecc_error == 0)
> +               *corrected = ecc_status;
> +}
> +
> +static void spi_nand_gd5f_ecc_status(unsigned int status,
> +                                    unsigned int *corrected,
> +                                    unsigned int *ecc_error)
> +{
> +       unsigned int ecc_status = (status >> SPI_NAND_GD5F_ECC_SHIFT) &
> +                                            SPI_NAND_GD5F_ECC_MASK;
> +
> +       *ecc_error = (ecc_status == SPI_NAND_GD5F_ECC_UNCORR) ? 1 : 0;
> +       if (*ecc_error == 0)
> +               *corrected = 2 + ecc_status;
> +}
> +
> +static int spi_nand_device_probe(struct spi_device *spi)
> +{
> +       enum spi_nand_device_variant variant;
> +       struct spi_nand_device *priv;
> +       struct spi_nand *snand;
> +       int ret;
> +
> +       priv = devm_kzalloc(&spi->dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       snand = &priv->spi_nand;
> +
> +       snand->read_cache = spi_nand_device_read_cache;
> +       snand->load_page = spi_nand_device_load_page;
> +       snand->store_cache = spi_nand_device_store_cache;
> +       snand->write_page = spi_nand_device_write_page;
> +       snand->write_reg = spi_nand_device_write_reg;
> +       snand->read_reg = spi_nand_device_read_reg;
> +       snand->block_erase = spi_nand_device_block_erase;
> +       snand->reset = spi_nand_device_reset;
> +       snand->write_enable = spi_nand_device_write_enable;
> +       snand->write_disable = spi_nand_device_write_disable;
> +       snand->dev = &spi->dev;
> +       snand->priv = priv;
> +
> +       /*
> +        * gd5f reads three ID bytes, and mt29f reads one dummy address byte
> +        * and two ID bytes. Therefore, we could detect both in the same
> +        * read_id implementation by reading _with_ and _without_ a dummy byte,
> +        * until a proper manufacturer is found.
> +        *
> +        * This'll mean we won't need to specify any specific compatible string
> +        * for a given device, and instead just support spi-nand.
> +        */
> +       variant = spi_get_device_id(spi)->driver_data;
> +       switch (variant) {
> +       case SPI_NAND_MT29F:
> +               snand->read_id = spi_nand_mt29f_read_id;
> +               snand->get_ecc_status = spi_nand_mt29f_ecc_status;
> +               break;
> +       case SPI_NAND_GD5F:
> +               snand->read_id = spi_nand_gd5f_read_id;
> +               snand->get_ecc_status = spi_nand_gd5f_ecc_status;
> +               break;
> +       default:
> +               dev_err(snand->dev, "unknown device\n");
> +               return -ENODEV;
> +       }
> +
> +       spi_set_drvdata(spi, snand);
> +       priv->spi = spi;
> +
> +       ret = spi_nand_register(snand, spi_nand_flash_ids);
> +       if (ret)
> +               return ret;
> +       return 0;
> +}
> +
> +static int spi_nand_device_remove(struct spi_device *spi)
> +{
> +       struct spi_nand *snand = spi_get_drvdata(spi);
> +
> +       spi_nand_unregister(snand);
> +
> +       return 0;
> +}
> +
> +const struct spi_device_id spi_nand_id_table[] = {
> +       { "spi-nand", SPI_NAND_GENERIC },
> +       { "mt29f", SPI_NAND_MT29F },
> +       { "gd5f", SPI_NAND_GD5F },
> +       { },
> +};
> +MODULE_DEVICE_TABLE(spi, spi_nand_id_table);
> +
> +static struct spi_driver spi_nand_device_driver = {
> +       .driver = {
> +               .name   = "spi_nand_device",
> +               .owner  = THIS_MODULE,
> +       },
> +       .id_table = spi_nand_id_table,
> +       .probe  = spi_nand_device_probe,
> +       .remove = spi_nand_device_remove,
> +};
> +module_spi_driver(spi_nand_device_driver);
> +
> +MODULE_AUTHOR("Ezequiel Garcia <ezequiel.garcia@imgtec.com>");
> +MODULE_DESCRIPTION("SPI NAND device support");
> +MODULE_LICENSE("GPL v2");
> --
> 2.1.0
>
>
> ______________________________________________________
> Linux MTD discussion mailing list
> http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 6/6] mtd: spi-nand: Support common SPI NAND devices

[Ezequiel Garcia]

On 12/12/2014 10:27 PM, Daniel Ehrenberg wrote:
> On Tue, Dec 2, 2014 at 4:58 AM, Ezequiel Garcia
> <ezequiel.garcia@imgtec.com> wrote:
[..]
>> +
>> +enum spi_nand_device_variant {
>> +       SPI_NAND_GENERIC,
>> +       SPI_NAND_MT29F,
>> +       SPI_NAND_GD5F,
>> +};
> 
> What is the purpose of including SPI_NAND_GENERIC? It looks like it
> doesn't work anyway.
> 

For the time being, I'm not sure we will require a device-specific compatible
string, or rather, auto-detect the device using the device ID.

So, this patch leaves the door opened for both options.

I've been talking to Brian on IRC, and auto-detecting the device seems like
the way to go. However, I wanted to have some feedback on the general approach
having the community approval, before moving forward in any direction.

Thanks for the feedback,
-- 
Ezequiel

Re: [PATCH 6/6] mtd: spi-nand: Support common SPI NAND devices

[Daniel Ehrenberg]

Sounds good to me.

Reviewed-by: Dan Ehrenberg <dehrenberg@chromium.org>

On Mon, Dec 15, 2014 at 11:36 AM, Ezequiel Garcia
<ezequiel.garcia@imgtec.com> wrote:
>
>
> On 12/12/2014 10:27 PM, Daniel Ehrenberg wrote:
>> On Tue, Dec 2, 2014 at 4:58 AM, Ezequiel Garcia
>> <ezequiel.garcia@imgtec.com> wrote:
> [..]
>>> +
>>> +enum spi_nand_device_variant {
>>> +       SPI_NAND_GENERIC,
>>> +       SPI_NAND_MT29F,
>>> +       SPI_NAND_GD5F,
>>> +};
>>
>> What is the purpose of including SPI_NAND_GENERIC? It looks like it
>> doesn't work anyway.
>>
>
> For the time being, I'm not sure we will require a device-specific compatible
> string, or rather, auto-detect the device using the device ID.
>
> So, this patch leaves the door opened for both options.
>
> I've been talking to Brian on IRC, and auto-detecting the device seems like
> the way to go. However, I wanted to have some feedback on the general approach
> having the community approval, before moving forward in any direction.
>
> Thanks for the feedback,
> --
> Ezequiel

[PATCH 6/6] mtd: spi-nand: Support common SPI NAND devices

[Qi Wang 王起 (qiwang)]

Hi Ezequiel,

> +static struct nand_ecclayout ecc_layout_mt29f = {
> +	.eccbytes = 32,
> +	.eccpos = {
> +		8, 9, 10, 11, 12, 13, 14, 15,
> +		24, 25, 26, 27, 28, 29, 30, 31,
> +		40, 41, 42, 43, 44, 45, 46, 47,
> +		56, 57, 58, 59, 60, 61, 62, 63,
> +	 },

Seems "OOB free" variable is missed. As below:

.oobfree = {
{ .offset = 2, .length = 6 },
{ .offset = 16, .length = 8 },
{ .offset = 32, .length = 8 },
{ .offset = 48, .length = 8 },
},


> +static int spi_nand_send_command(struct spi_device *spi, int command,
> +				 struct spi_nand_device_cmd *cmd) {
> +	struct spi_message message;
> +	struct spi_transfer x[4];
> +
> +	spi_message_init(&message);
> +	memset(x, 0, sizeof(x));
> +
> +	/* Command */
> +	cmd->cmd = command;
> +	x[0].len = 1;
> +	x[0].tx_buf = &cmd->cmd;
> +	spi_message_add_tail(&x[0], &message);
> +
> +	/* Address */
> +	if (cmd->n_addr) {
> +		x[1].len = cmd->n_addr;
> +		x[1].tx_buf = cmd->addr;
> +		spi_message_add_tail(&x[1], &message);
> +	}
> +
> +	/* Data to be transmitted */
> +	if (cmd->n_tx) {
> +		x[3].len = cmd->n_tx;
> +		x[3].tx_buf = cmd->tx_buf;
> +		spi_message_add_tail(&x[3], &message);
> +	}
> +
> +	/* Data to be received */
> +	if (cmd->n_rx) {
> +		x[3].len = cmd->n_rx;
> +		x[3].rx_buf = cmd->rx_buf;
> +		spi_message_add_tail(&x[3], &message);
> +	}

Only x[3] is used in above code, I suggest to separate transfer and received 
For x[2] and x[3], in case some command need to send dummy byte before received any data.

> +static int spi_nand_device_block_erase(struct spi_nand *snand,
> unsigned
> +int page_addr) {
> +	struct spi_nand_device *snand_dev = snand->priv;
> +	struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
> +
> +	memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
> +	cmd->n_addr = 3;
> +	cmd->addr[0] = 0;

Page address may beyond 16 bit in large density product. 
So I don't think addr[0] can be forced to be set 0


> +static int spi_nand_mt29f_read_id(struct spi_nand *snand, u8 *buf) {
> +	struct spi_nand_device *snand_dev = snand->priv;
> +	struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
> +
> +	memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
> +	cmd->n_rx = SPI_NAND_MT29F_READID_LEN;
> +	cmd->rx_buf = buf;
> +

Micron SPI NAND need 1 more dummy byte before received ID data.
Code for mt29f_read_id should be like this:

static int spi_nand_mt29f_read_id(struct spi_nand *snand, u8 *buf) {
	struct spi_nand_device *snand_dev = snand->priv;
	struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
	u8 dummy = 0;
	memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
"
	cmd->n_tx = 1;
	cmd->tx_buf = &dummy;
"
	cmd->n_rx = SPI_NAND_MT29F_READID_LEN;
	cmd->rx_buf = buf;

Thanks
Qi Wang

Re: [PATCH 6/6] mtd: spi-nand: Support common SPI NAND devices

[Ezequiel Garcia]

On 12/22/2014 01:34 AM, Qi Wang 王起 (qiwang) wrote:
> Hi Ezequiel,
> 
>> +static struct nand_ecclayout ecc_layout_mt29f = {
>> +	.eccbytes = 32,
>> +	.eccpos = {
>> +		8, 9, 10, 11, 12, 13, 14, 15,
>> +		24, 25, 26, 27, 28, 29, 30, 31,
>> +		40, 41, 42, 43, 44, 45, 46, 47,
>> +		56, 57, 58, 59, 60, 61, 62, 63,
>> +	 },
> 
> Seems "OOB free" variable is missed. As below:
> 
> .oobfree = {
> { .offset = 2, .length = 6 },
> { .offset = 16, .length = 8 },
> { .offset = 32, .length = 8 },
> { .offset = 48, .length = 8 },
> },
> 

OK, great.

Have you tested this and made sure it works for MT29F and allows to
access the OOB?

> 
>> +static int spi_nand_send_command(struct spi_device *spi, int command,
>> +				 struct spi_nand_device_cmd *cmd) {
>> +	struct spi_message message;
>> +	struct spi_transfer x[4];
>> +
>> +	spi_message_init(&message);
>> +	memset(x, 0, sizeof(x));
>> +
>> +	/* Command */
>> +	cmd->cmd = command;
>> +	x[0].len = 1;
>> +	x[0].tx_buf = &cmd->cmd;
>> +	spi_message_add_tail(&x[0], &message);
>> +
>> +	/* Address */
>> +	if (cmd->n_addr) {
>> +		x[1].len = cmd->n_addr;
>> +		x[1].tx_buf = cmd->addr;
>> +		spi_message_add_tail(&x[1], &message);
>> +	}
>> +
>> +	/* Data to be transmitted */
>> +	if (cmd->n_tx) {
>> +		x[3].len = cmd->n_tx;
>> +		x[3].tx_buf = cmd->tx_buf;
>> +		spi_message_add_tail(&x[3], &message);
>> +	}
>> +
>> +	/* Data to be received */
>> +	if (cmd->n_rx) {
>> +		x[3].len = cmd->n_rx;
>> +		x[3].rx_buf = cmd->rx_buf;
>> +		spi_message_add_tail(&x[3], &message);
>> +	}
> 
> Only x[3] is used in above code, I suggest to separate transfer and received 
> For x[2] and x[3], in case some command need to send dummy byte before received any data.
> 

Ah, yes. Good catch.

>> +static int spi_nand_device_block_erase(struct spi_nand *snand,
>> unsigned
>> +int page_addr) {
>> +	struct spi_nand_device *snand_dev = snand->priv;
>> +	struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
>> +
>> +	memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
>> +	cmd->n_addr = 3;
>> +	cmd->addr[0] = 0;
> 
> Page address may beyond 16 bit in large density product. 
> So I don't think addr[0] can be forced to be set 0
> 

Right, this was a bad copy paste from the staging driver.

> 
>> +static int spi_nand_mt29f_read_id(struct spi_nand *snand, u8 *buf) {
>> +	struct spi_nand_device *snand_dev = snand->priv;
>> +	struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
>> +
>> +	memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
>> +	cmd->n_rx = SPI_NAND_MT29F_READID_LEN;
>> +	cmd->rx_buf = buf;
>> +
> 
> Micron SPI NAND need 1 more dummy byte before received ID data.
> Code for mt29f_read_id should be like this:
> 
> static int spi_nand_mt29f_read_id(struct spi_nand *snand, u8 *buf) {
> 	struct spi_nand_device *snand_dev = snand->priv;
> 	struct spi_nand_device_cmd *cmd = &snand_dev->cmd;
> 	u8 dummy = 0;
> 	memset(cmd, 0, sizeof(struct spi_nand_device_cmd));
> "
> 	cmd->n_tx = 1;
> 	cmd->tx_buf = &dummy;
> "
> 	cmd->n_rx = SPI_NAND_MT29F_READID_LEN;
> 	cmd->rx_buf = buf;
> 

That's true. However, notice the staging driver does not use any dummy
byte, and instead fakes it by reading an extra byte.

Maybe you can test this on a MT29F and point at all the required fixes?

Thanks a lot for the feedback!
-- 
Ezequiel

Re: [PATCH 0/6] SPI NAND for everyone

[Ezequiel Garcia]

Hi all,

On 12/02/2014 09:58 AM, Ezequiel Garcia wrote:
> During the discussion of Ionela Voinescu's patch to support GD5F SPI NAND
> devices [1], it was decided that a proper SPI NAND framework was needed
> to support the mt29f device (driver currently in staging area) and the new
> gd5f.
> 
> This patchset is a first attempt to address this.
> 
> The SPI NAND framework allows devices to register as NAND drivers. This is
> useful to take advantage of the bad block management code. Given the
> SPI NAND and the bare NAND commands are different, the SPI NAND framework
> implements its own .cmdfunc callback, which is in charge of calling
> device-specific hooks for each of the flash operations (read, program, erase,
> etc).
> 
> The SPI NAND framework does not deal with SPI transactions per-se. Instead,
> the SPI messages should be prepared by the users of the SPI NAND framework
> (those that implement the device-specific hooks).
> 
> The result can be expressed in the following hierarchy:
> 
>     Userspace
>   ------------------
>     MTD
>   ------------------
>     NAND core
>   ------------------
>     SPI NAND core
>   ------------------
>     SPI NAND device
>   ------------------
>     SPI core
>   ------------------
>     SPI master
>   ------------------
>     Hardware
> 
> Notice there was a previous attempt to propose an SPI NAND framework,
> by Sourav Poddar and Mona Anonuevo. We didn't find this proposal suitable,
> so this series is a completely new work.
> 
> This series is based on v3.18-rc7. Tests have been performed with a Gigadevice
> GD5F 4 Gbit device, including nandtest runs and filesystem testing on top
> of UBI. I don't have MT29F devices yet, but the amount of changes required to
> support it should be fairly small.
> 
> I don't intend this first proposal to be complete, but I hope it's a good
> starting point to support SPI NAND properly.
> 

I'm aware we are in the middle of the merge window, and probably
everyone is getting ready for Christmas and holidays.

However, if anyone can take a look at the proposal, and in particular at
the SPI NAND framework API, it would be awesome to move forward wit this
discussion so we can get mt29f out of staging and support gd5f.

Thanks a lot!
-- 
Ezequiel

Re: [PATCH 0/6] SPI NAND for everyone

[Brian Norris]

Hi Ezequiel,

On Tue, Dec 02, 2014 at 09:58:50AM -0300, Ezequiel Garcia wrote:
> During the discussion of Ionela Voinescu's patch to support GD5F SPI NAND
> devices [1], it was decided that a proper SPI NAND framework was needed
> to support the mt29f device (driver currently in staging area) and the new
> gd5f.
> 
> This patchset is a first attempt to address this.

Thanks for this effort. I've finally had a better chance to look through
your implementation and to give it some better thought.

> The SPI NAND framework allows devices to register as NAND drivers. This is
> useful to take advantage of the bad block management code.

I'm not so sure about this choice, but then I'm not so sure about the
alternatives earlier. I understand that you and Qi Wang might have had
some discussion off-list (please feel free to fill me and others in
here, and I can add my thoughts).

The BBT code is something we definitely want to share, but it's actually
not very closely tied to nand_base.c, and it looks pretty easy to adapt
to any MTD that implements mtd_read_oob()/mtd_write_oob(). We'd just
need to parameterize a few relevant device details into a new nand_bbt
struct, rather than using struct nand_chip directly.

It also looks like the identification code (read ID, ONFI / JEDEC
parameter pages) may differ, and SPI NAND may continue to evolve
differently. For instance, I see that Micron SPI NAND has a
(non-standardized) ONFI-like parameter page. It's mostly compatible with
ONFI, except it leaves out N/A fields and doesn't use an official
'revision' bitfield. This probably isn't 100% shareable with parallel
NAND code, if we use it at all.

So, I think we'll need to weigh the options of which one gives more bang
for the buck.

FWIW, I don't think the implementation in this series is that bad. Even
if we want to migrate to have less dependence on the current nand_base
implementation for non-parallel-NAND code, I think it's doable after
merging. We'd just need to keep the drivers/spi/ and DT binding formats
stable, while remaining free to refactor internally.

> Given the
> SPI NAND and the bare NAND commands are different, the SPI NAND framework
> implements its own .cmdfunc callback, which is in charge of calling
> device-specific hooks for each of the flash operations (read, program, erase,
> etc).

I'm not really a big fan of the switch/case remapping of one command set
into another (in this case, translating parallel NAND commands into
their SPI NAND equivalents). At times they may be necessary, but as food
for thought, we might consider alternatives, like additional replaceable
hooks in struct nand_chip.

> The SPI NAND framework does not deal with SPI transactions per-se. Instead,
> the SPI messages should be prepared by the users of the SPI NAND framework
> (those that implement the device-specific hooks).

Sounds reasonable. Does this leave room for hardware that is optimized
for SPI NAND, without implementing a full SPI controller, and therefore
does not use the drivers/spi/ infrastructure? Not sure if such a thing
exists yet (or will exist), but it does pop up in SPI NOR.

> The result can be expressed in the following hierarchy:
> 
>     Userspace
>   ------------------
>     MTD
>   ------------------
>     NAND core
>   ------------------
>     SPI NAND core
>   ------------------
>     SPI NAND device
>   ------------------
>     SPI core
>   ------------------
>     SPI master
>   ------------------
>     Hardware

This is a pretty good description, but some details are not quite right.
e.g., "userspace" should probably be removed or elaborated -- the
hierarchy could just stop at the "MTD API" (mtd_read(), mtd_write(),
mtd_erase(), etc.). Its users might be user-space (via mtdchar) or UBI,
JFFS2, etc.

Also, it might help to either flesh out what these hierarchies actually
mean by pointing to specific files, by additional commentary, or both.
It helps if you can also use the same language as the SPI documentation
for the lower levels of this hierarchy.

We might want to add something like this as official documentation,
possibly to Documentation/mtd/. Short and sweet is fine (i.e., not much
more than a cleaned up version of this cover letter).

Side note: I should probably add something to Documentation/mtd/ to
point to linux-mtd.infradead.org.

> Notice there was a previous attempt to propose an SPI NAND framework,
> by Sourav Poddar and Mona Anonuevo. We didn't find this proposal suitable,
> so this series is a completely new work.
> 
> This series is based on v3.18-rc7.

It has trivial conflicts on v3.19-rc1 now (and l2-mtd.git).

> Tests have been performed with a Gigadevice
> GD5F 4 Gbit device, including nandtest runs and filesystem testing on top
> of UBI. I don't have MT29F devices yet, but the amount of changes required to
> support it should be fairly small.
> 
> I don't intend this first proposal to be complete, but I hope it's a good
> starting point to support SPI NAND properly.
> 
> [1] http://lists.infradead.org/pipermail/linux-mtd/2014-November/056364.html
> [2] https://lkml.org/lkml/2013/6/26/83
> 
> Ezequiel Garcia (6):
>   mtd: nand: Check length of ID before reading bits per cell
>   mtd: nand: Add JEDEC manufacturer ID for Gigadevice
>   mtd: nand: Allow to set a per-device ECC layout
>   mtd: Introduce SPI NAND framework
>   mtd: spi-nand: Add devicetree binding
>   mtd: spi-nand: Support common SPI NAND devices
> 
>  Documentation/devicetree/bindings/mtd/spi-nand.txt |  21 +
>  drivers/mtd/Kconfig                                |   2 +
>  drivers/mtd/Makefile                               |   1 +
>  drivers/mtd/nand/nand_base.c                       |   4 +-
>  drivers/mtd/nand/nand_ids.c                        |   1 +
>  drivers/mtd/spi-nand/Kconfig                       |  18 +
>  drivers/mtd/spi-nand/Makefile                      |   2 +
>  drivers/mtd/spi-nand/spi-nand-base.c               | 530 +++++++++++++++++++++
>  drivers/mtd/spi-nand/spi-nand-device.c             | 500 +++++++++++++++++++
>  include/linux/mtd/nand.h                           |   3 +
>  include/linux/mtd/spi-nand.h                       |  54 +++
>  11 files changed, 1135 insertions(+), 1 deletion(-)
>  create mode 100644 Documentation/devicetree/bindings/mtd/spi-nand.txt
>  create mode 100644 drivers/mtd/spi-nand/Kconfig
>  create mode 100644 drivers/mtd/spi-nand/Makefile
>  create mode 100644 drivers/mtd/spi-nand/spi-nand-base.c
>  create mode 100644 drivers/mtd/spi-nand/spi-nand-device.c
>  create mode 100644 include/linux/mtd/spi-nand.h

I have some more code review comments, but those aren't as important at
the moment if we're still not sure about the overall approach.

Regards,
Brian

Re: [PATCH 0/6] SPI NAND for everyone

[Ezequiel Garcia]

On 01/06/2015 12:30 AM, Brian Norris wrote:
> Hi Ezequiel,
> 
> On Tue, Dec 02, 2014 at 09:58:50AM -0300, Ezequiel Garcia wrote:
>> During the discussion of Ionela Voinescu's patch to support GD5F SPI NAND
>> devices [1], it was decided that a proper SPI NAND framework was needed
>> to support the mt29f device (driver currently in staging area) and the new
>> gd5f.
>>
>> This patchset is a first attempt to address this.
> 
> Thanks for this effort. I've finally had a better chance to look through
> your implementation and to give it some better thought.
> 

Great, thanks for the review.

>> The SPI NAND framework allows devices to register as NAND drivers. This is
>> useful to take advantage of the bad block management code.
> 
> I'm not so sure about this choice, but then I'm not so sure about the
> alternatives earlier. I understand that you and Qi Wang might have had
> some discussion off-list (please feel free to fill me and others in
> here, and I can add my thoughts).
> 

Indeed, I believe Qi Wang have these same concerns.

> The BBT code is something we definitely want to share, but it's actually
> not very closely tied to nand_base.c, and it looks pretty easy to adapt
> to any MTD that implements mtd_read_oob()/mtd_write_oob(). We'd just
> need to parameterize a few relevant device details into a new nand_bbt
> struct, rather than using struct nand_chip directly.
> 
> It also looks like the identification code (read ID, ONFI / JEDEC
> parameter pages) may differ, and SPI NAND may continue to evolve
> differently. For instance, I see that Micron SPI NAND has a
> (non-standardized) ONFI-like parameter page. It's mostly compatible with
> ONFI, except it leaves out N/A fields and doesn't use an official
> 'revision' bitfield. This probably isn't 100% shareable with parallel
> NAND code, if we use it at all.
> 
> So, I think we'll need to weigh the options of which one gives more bang
> for the buck.
> 
> FWIW, I don't think the implementation in this series is that bad. Even
> if we want to migrate to have less dependence on the current nand_base
> implementation for non-parallel-NAND code, I think it's doable after
> merging. We'd just need to keep the drivers/spi/ and DT binding formats
> stable, while remaining free to refactor internally.
> 

Well, the DT binding itself looks pretty simple, it's just a compatible
and the usual SPI child properties. Regarding the SPI interface, that's
not related to the "based on NAND or not" discussion.

The SPI API is called only in the spi-nand-device.c file, through the
spi-nand hooks. These hooks would remain, as they mirror the SPI NAND
commands I've seen in Micron and Gigadevice flashes.

On the other side, I'm not convinced about merging the driver knowing
we will have to refactor it to make it independent of the NAND core.
Perhaps I should give it a shot and see how hard is it to make it
based on MTD, yet use the BBT code.

>> Given the
>> SPI NAND and the bare NAND commands are different, the SPI NAND framework
>> implements its own .cmdfunc callback, which is in charge of calling
>> device-specific hooks for each of the flash operations (read, program, erase,
>> etc).
> 
> I'm not really a big fan of the switch/case remapping of one command set
> into another (in this case, translating parallel NAND commands into
> their SPI NAND equivalents). At times they may be necessary, but as food
> for thought, we might consider alternatives, like additional replaceable
> hooks in struct nand_chip.
> 
>> The SPI NAND framework does not deal with SPI transactions per-se. Instead,
>> the SPI messages should be prepared by the users of the SPI NAND framework
>> (those that implement the device-specific hooks).
> 
> Sounds reasonable. Does this leave room for hardware that is optimized
> for SPI NAND, without implementing a full SPI controller, and therefore
> does not use the drivers/spi/ infrastructure? Not sure if such a thing
> exists yet (or will exist), but it does pop up in SPI NOR.
> 

Exactly, this separation is meant not only to properly split
responsabilities, but also to allow to introduce such drivers in an
elegante fashion.

>> The result can be expressed in the following hierarchy:
>>
>>     Userspace
>>   ------------------
>>     MTD
>>   ------------------
>>     NAND core
>>   ------------------
>>     SPI NAND core
>>   ------------------
>>     SPI NAND device
>>   ------------------
>>     SPI core
>>   ------------------
>>     SPI master
>>   ------------------
>>     Hardware
> 
> This is a pretty good description, but some details are not quite right.
> e.g., "userspace" should probably be removed or elaborated -- the
> hierarchy could just stop at the "MTD API" (mtd_read(), mtd_write(),
> mtd_erase(), etc.). Its users might be user-space (via mtdchar) or UBI,
> JFFS2, etc.
> 
> Also, it might help to either flesh out what these hierarchies actually
> mean by pointing to specific files, by additional commentary, or both.
> It helps if you can also use the same language as the SPI documentation
> for the lower levels of this hierarchy.
> 
> We might want to add something like this as official documentation,
> possibly to Documentation/mtd/. Short and sweet is fine (i.e., not much
> more than a cleaned up version of this cover letter).
> 

OK, that's certainly doable, once we agree on the overall approach.

[..]
> 
> I have some more code review comments, but those aren't as important at
> the moment if we're still not sure about the overall approach.
> 

Sure.

BTW, regarding the compatible string, I was thinking about using
a generic spi-nand compatible, if at all possible, and let the probe
routine detect the specific device and pick hooks, set flags, etc.

How does this sound?
-- 
Ezequiel

RE: [PATCH 0/6] SPI NAND for everyone

[Qi Wang 王起 (qiwang)]

Hi Brian and Ezequiel,

On 01/07/2015 5:03 AM, Ezequiel Garcia wrote:
>
>
>On 01/06/2015 12:30 AM, Brian Norris wrote:
>
>>> The SPI NAND framework allows devices to register as NAND drivers. This
>is
>>> useful to take advantage of the bad block management code.
>>
>> I'm not so sure about this choice, but then I'm not so sure about the
>> alternatives earlier. I understand that you and Qi Wang might have had
>> some discussion off-list (please feel free to fill me and others in
>> here, and I can add my thoughts).
>>
>
>Indeed, I believe Qi Wang have these same concerns.

Yes, I also consider this a lot. Actually, I already complete a new SPI NAND
Framework to implement SPI NAND code independent with Parallel NAND.
Testing on Micron Device is finished, my colleague will send out this 
patch to community soon for both you review.

Thanks

Qi Wang

Re: [PATCH 0/6] SPI NAND for everyone

[Ezequiel Garcia]

On 01/06/2015 09:55 PM, Qi Wang 王起 (qiwang) wrote:
> Hi Brian and Ezequiel,
> 
> On 01/07/2015 5:03 AM, Ezequiel Garcia wrote:
>>
>>
>> On 01/06/2015 12:30 AM, Brian Norris wrote:
>>
>>>> The SPI NAND framework allows devices to register as NAND drivers. This
>> is
>>>> useful to take advantage of the bad block management code.
>>>
>>> I'm not so sure about this choice, but then I'm not so sure about the
>>> alternatives earlier. I understand that you and Qi Wang might have had
>>> some discussion off-list (please feel free to fill me and others in
>>> here, and I can add my thoughts).
>>>
>>
>> Indeed, I believe Qi Wang have these same concerns.
> 
> Yes, I also consider this a lot. Actually, I already complete a new SPI NAND
> Framework to implement SPI NAND code independent with Parallel NAND.
> Testing on Micron Device is finished, my colleague will send out this 
> patch to community soon for both you review.
> 

Great!

However, I don't think it's a good plan to delay its submission. At this
stage we won't care about the little details, but we are mostly
interested in seeing and discussing the overall approach.

That will allow us to start the discussion earlier.

We'll worry about properly testing it and polishing it later.

"Release early, release often".
-- 
Ezequiel

[PATCH 6/6] mtd: spi-nand: Support common SPI NAND devices

[Qi Wang 王起 (qiwang)]

Hi Ezequiel, Brian

> +
> +static struct nand_ecclayout ecc_layout_gd5f = {
> +	.eccbytes = 128,
> +	.eccpos = {
> +		128, 129, 130, 131, 132, 133, 134, 135,
> +		136, 137, 138, 139, 140, 141, 142, 143,
> +		144, 145, 146, 147, 148, 149, 150, 151,
> +		152, 153, 154, 155, 156, 157, 158, 159,
> +		160, 161, 162, 163, 164, 165, 166, 167,
> +		168, 169, 170, 171, 172, 173, 174, 175,
> +		176, 177, 178, 179, 180, 181, 182, 183,
> +		184, 185, 186, 187, 188, 189, 190, 191,
> +		192, 193, 194, 195, 196, 197, 198, 199,
> +		200, 201, 202, 203, 204, 205, 206, 207,
> +		208, 209, 210, 211, 212, 213, 214, 215,
> +		216, 217, 218, 219, 220, 221, 222, 223,
> +		224, 225, 226, 227, 228, 229, 230, 231,
> +		232, 233, 234, 235, 236, 237, 238, 239,
> +		240, 241, 242, 243, 244, 245, 246, 247,
> +		248, 249, 250, 251, 252, 253, 254, 255
> +	},
> +	.oobfree = { {1, 127} }
> +};
> +
> +static struct nand_ecclayout ecc_layout_mt29f = {
> +	.eccbytes = 32,
> +	.eccpos = {
> +		8, 9, 10, 11, 12, 13, 14, 15,
> +		24, 25, 26, 27, 28, 29, 30, 31,
> +		40, 41, 42, 43, 44, 45, 46, 47,
> +		56, 57, 58, 59, 60, 61, 62, 63,
> +	 },
> +};
> +

As they are using On die ecc, oob layout definition is defined by SPI NAND vendors. 
Part of oob is protected by ecc, and the rest is unprotect.
I am thinking if we can define a variable to indicate the range, what is protect, 
and what is unprotect. 
This would be very useful for filesystem, which want to write data into OOB.

For example, Jffs2 program clean marker data to oob first when format operation, 
then will program main data into NAND flash main area when normal write operation. 
That mean JFFS2 can only store clean marker into unprotect area of oob. 
But for YAFFS2, it program main data and spare area data together into NAND flash, then
YAFFS2 can store data into protect area of OOB.
How do you think?

Thanks
 

Re: [PATCH 6/6] mtd: spi-nand: Support common SPI NAND devices

[Ezequiel Garcia]

On 12/03/2014 06:52 AM, Qi Wang 王起 (qiwang) wrote:
>> +
>> +static struct nand_ecclayout ecc_layout_gd5f = {
>> +	.eccbytes = 128,
>> +	.eccpos = {
>> +		128, 129, 130, 131, 132, 133, 134, 135,
>> +		136, 137, 138, 139, 140, 141, 142, 143,
>> +		144, 145, 146, 147, 148, 149, 150, 151,
>> +		152, 153, 154, 155, 156, 157, 158, 159,
>> +		160, 161, 162, 163, 164, 165, 166, 167,
>> +		168, 169, 170, 171, 172, 173, 174, 175,
>> +		176, 177, 178, 179, 180, 181, 182, 183,
>> +		184, 185, 186, 187, 188, 189, 190, 191,
>> +		192, 193, 194, 195, 196, 197, 198, 199,
>> +		200, 201, 202, 203, 204, 205, 206, 207,
>> +		208, 209, 210, 211, 212, 213, 214, 215,
>> +		216, 217, 218, 219, 220, 221, 222, 223,
>> +		224, 225, 226, 227, 228, 229, 230, 231,
>> +		232, 233, 234, 235, 236, 237, 238, 239,
>> +		240, 241, 242, 243, 244, 245, 246, 247,
>> +		248, 249, 250, 251, 252, 253, 254, 255
>> +	},
>> +	.oobfree = { {1, 127} }
>> +};
>> +
>> +static struct nand_ecclayout ecc_layout_mt29f = {
>> +	.eccbytes = 32,
>> +	.eccpos = {
>> +		8, 9, 10, 11, 12, 13, 14, 15,
>> +		24, 25, 26, 27, 28, 29, 30, 31,
>> +		40, 41, 42, 43, 44, 45, 46, 47,
>> +		56, 57, 58, 59, 60, 61, 62, 63,
>> +	 },
>> +};
>> +
> 
> As they are using On die ecc, oob layout definition is defined by SPI NAND vendors. 
> Part of oob is protected by ecc, and the rest is unprotect.
> I am thinking if we can define a variable to indicate the range, what is protect, 
> and what is unprotect. 

Yes, we should add the free out-of-band regions.

> This would be very useful for filesystem, which want to write data into OOB.
> 
> For example, Jffs2 program clean marker data to oob first when format operation, 
> then will program main data into NAND flash main area when normal write operation. 
> That mean JFFS2 can only store clean marker into unprotect area of oob. 

Yeah, but are these devices capable of writing to the OOB region only
(without messing with the data) ?

If I understand this correctly, supporting JFFS2 would mean implementing
a sort of read-modify-write operation to be able to write the OOB
without touching the main area, and viceversa.

I'd say JFFS2 is not the target filesystem for these SPI NAND devices,
and UBI-based filesystems should be used instead (UBI don't use OOB).

> But for YAFFS2, it program main data and spare area data together into NAND flash, then
> YAFFS2 can store data into protect area of OOB.

Well, frankly, I'm not too worried about YAFFS2. It was never mainlined,
and probably it will never be.

Do you have any reason for *not* using a UBI-based FS?
-- 
Ezequiel

RE: [PATCH 6/6] mtd: spi-nand: Support common SPI NAND devices

[Qi Wang 王起 (qiwang)]

On 12/03/2014 07:14 AM, Ezequiel Garcia wrote:
> 
> On 12/03/2014 06:52 AM, Qi Wang 王起 (qiwang) wrote:
> >> +
> >> +static struct nand_ecclayout ecc_layout_gd5f = {
> >> +	.eccbytes = 128,
> >> +	.eccpos = {
> >> +		128, 129, 130, 131, 132, 133, 134, 135,
> >> +		136, 137, 138, 139, 140, 141, 142, 143,
> >> +		144, 145, 146, 147, 148, 149, 150, 151,
> >> +		152, 153, 154, 155, 156, 157, 158, 159,
> >> +		160, 161, 162, 163, 164, 165, 166, 167,
> >> +		168, 169, 170, 171, 172, 173, 174, 175,
> >> +		176, 177, 178, 179, 180, 181, 182, 183,
> >> +		184, 185, 186, 187, 188, 189, 190, 191,
> >> +		192, 193, 194, 195, 196, 197, 198, 199,
> >> +		200, 201, 202, 203, 204, 205, 206, 207,
> >> +		208, 209, 210, 211, 212, 213, 214, 215,
> >> +		216, 217, 218, 219, 220, 221, 222, 223,
> >> +		224, 225, 226, 227, 228, 229, 230, 231,
> >> +		232, 233, 234, 235, 236, 237, 238, 239,
> >> +		240, 241, 242, 243, 244, 245, 246, 247,
> >> +		248, 249, 250, 251, 252, 253, 254, 255
> >> +	},
> >> +	.oobfree = { {1, 127} }
> >> +};
> >> +
> >> +static struct nand_ecclayout ecc_layout_mt29f = {
> >> +	.eccbytes = 32,
> >> +	.eccpos = {
> >> +		8, 9, 10, 11, 12, 13, 14, 15,
> >> +		24, 25, 26, 27, 28, 29, 30, 31,
> >> +		40, 41, 42, 43, 44, 45, 46, 47,
> >> +		56, 57, 58, 59, 60, 61, 62, 63,
> >> +	 },
> >> +};
> >> +
> >
> > As they are using On die ecc, oob layout definition is defined by SPI
> NAND vendors.
> > Part of oob is protected by ecc, and the rest is unprotect.
> > I am thinking if we can define a variable to indicate the range, what
> > is protect, and what is unprotect.
> 
> Yes, we should add the free out-of-band regions.

Sorry, probably I didn't explain clearly. 
I mean if we can define oob free area like this:
.oobfree_unprotect = { {1, 63} },
.oobfree_protect = { {64, 127} },
To differentiate the which OOB area is protected, which isn't protected.

> > This would be very useful for filesystem, which want to write data into
> OOB.
> >
> > For example, Jffs2 program clean marker data to oob first when format
> > operation, then will program main data into NAND flash main area when
> normal write operation.
> > That mean JFFS2 can only store clean marker into unprotect area of oob.
> 
> Yeah, but are these devices capable of writing to the OOB region only
> (without messing with the data) ?

Yes, SPI NAND have this capability. SPI NAND also have definition for NOP,
just similar with Parallel NAND, fill 0xFF to the area that don't want to 
write anything is ok.

> If I understand this correctly, supporting JFFS2 would mean implementing a
> sort of read-modify-write operation to be able to write the OOB without
> touching the main area, and viceversa.

Yes, just like this operation, but JFFS2 write OOB area first, then main area.
So JFFS2 should write data into unprotect area in OOB, then write main data,
Right? Because if write data into OOB protect area, on die ecc will generate
a ecc code and write into eccpos area. When JFFS2 write main data, a new ecc 
code will generate, and will cause conflict with old one.  

> I'd say JFFS2 is not the target filesystem for these SPI NAND devices, and
> UBI-based filesystems should be used instead (UBI don't use OOB).
> 
> > But for YAFFS2, it program main data and spare area data together into
> > NAND flash, then
> > YAFFS2 can store data into protect area of OOB.
> 
> Well, frankly, I'm not too worried about YAFFS2. It was never mainlined,
> and probably it will never be.
> 
> Do you have any reason for *not* using a UBI-based FS?

I agree with you, I just considered some legacy users customer still using JFFS2.

Thanks   

Re: [PATCH 6/6] mtd: spi-nand: Support common SPI NAND devices

[Ezequiel Garcia]

On 12/03/2014 09:08 AM, Qi Wang 王起 (qiwang) wrote:
[..]
>>> For example, Jffs2 program clean marker data to oob first when format
>>> operation, then will program main data into NAND flash main area when
>> normal write operation.
>>> That mean JFFS2 can only store clean marker into unprotect area of oob.
>>
>> Yeah, but are these devices capable of writing to the OOB region only
>> (without messing with the data) ?
> 
> Yes, SPI NAND have this capability. SPI NAND also have definition for NOP,
> just similar with Parallel NAND, fill 0xFF to the area that don't want to 
> write anything is ok.
> 

Which SPI NAND command allows to do this?

AFAICS, you need to issue the following sequence to be able to write to
only the main area, or only the OOB area:

1. Read page to cache
2. Write to cache
3. Write page from cache

So it doesn't seem to support it natively, but through a workaround.
Doesn't this hurt JFFS2 performance a lot?

[..]
>>
>> Do you have any reason for *not* using a UBI-based FS?
> 
> I agree with you, I just considered some legacy users customer still using JFFS2.
> 

Hm.. right. It's important to support legacy users, but I'd say it's not
our primary goal.

-- 
Ezequiel