[PATCH v2 00/10] mtd: spi-nor: Add the DDR quad read support

[PATCH v2 00/10] mtd: spi-nor: Add the DDR quad read support

[Huang Shijie]

 (0) This patch set depends on the patch:
	http://lists.infradead.org/pipermail/linux-mtd/2014-April/053308.html
  
 (1) This patch set tries to add the DDR quad read support for the SPI
     NOR framework, and it also adds the DDR quad read support for FREESCALE
     quadspi controller driver.

 (2) Test this patch set with Spansion s25fl128s, the performance with mtd_speedtest.ko:
     =================================================
     mtd_speedtest: MTD device: 0
     mtd_speedtest: not NAND flash, assume page size is 512 bytes.
     mtd_speedtest: MTD device size 16777216, eraseblock size 65536, page size 512,
                    count of eraseblocks 256, pages per eraseblock 128, OOB size 0
     mtd_speedtest: testing eraseblock write speed
     mtd_speedtest: eraseblock write speed is 665 KiB/s
     mtd_speedtest: testing eraseblock read speed
     mtd_speedtest: eraseblock read speed is 49799 KiB/s
     mtd_speedtest: testing page write speed
     mtd_speedtest: page write speed is 662 KiB/s
     mtd_speedtest: testing page read speed
     mtd_speedtest: page read speed is 24236 KiB/s
     mtd_speedtest: testing 2 page write speed
     mtd_speedtest: 2 page write speed is 657 KiB/s
     mtd_speedtest: testing 2 page read speed
     mtd_speedtest: 2 page read speed is 32637 KiB/s
     mtd_speedtest: Testing erase speed
     mtd_speedtest: erase speed is 518 KiB/s
     mtd_speedtest: Testing 2x multi-block erase speed
     mtd_speedtest: 2x multi-block erase speed is 506 KiB/s
     mtd_speedtest: Testing 4x multi-block erase speed
     mtd_speedtest: 4x multi-block erase speed is 503 KiB/s
     mtd_speedtest: Testing 8x multi-block erase speed
     mtd_speedtest: 8x multi-block erase speed is 501 KiB/s
     mtd_speedtest: Testing 16x multi-block erase speed
     mtd_speedtest: 16x multi-block erase speed is 498 KiB/s
     mtd_speedtest: Testing 32x multi-block erase speed
     mtd_speedtest: 32x multi-block erase speed is 496 KiB/s
     mtd_speedtest: Testing 64x multi-block erase speed
     mtd_speedtest: 64x multi-block erase speed is 495 KiB/s
     mtd_speedtest: finished
     =================================================

  (3) Add the DDR quad read support for Micron N25Q256A:
    =================================================
    mtd_speedtest: MTD device: 1
    mtd_speedtest: not NAND flash, assume page size is 512 bytes.
    mtd_speedtest: MTD device size 33554432, eraseblock size 65536,
                   page size 512, count of eraseblocks 512, pages per eraseblock 128, OOB size 0
    mtd_speedtest: testing eraseblock write speed
    mtd_speedtest: eraseblock write speed is 2426 KiB/s
    mtd_speedtest: testing eraseblock read speed
    mtd_speedtest: eraseblock read speed is 32157 KiB/s
    mtd_speedtest: testing page write speed
    mtd_speedtest: page write speed is 2362 KiB/s
    mtd_speedtest: testing page read speed
    mtd_speedtest: page read speed is 17741 KiB/s
    mtd_speedtest: testing 2 page write speed
    mtd_speedtest: 2 page write speed is 2384 KiB/s
    mtd_speedtest: testing 2 page read speed
    mtd_speedtest: 2 page read speed is 24058 KiB/s
    mtd_speedtest: Testing erase speed
    mtd_speedtest: erase speed is 1927529 KiB/s
    mtd_speedtest: Testing 2x multi-block erase speed
    mtd_speedtest: 2x multi-block erase speed is 2184533 KiB/s
    mtd_speedtest: Testing 4x multi-block erase speed
    mtd_speedtest: 4x multi-block erase speed is 2184533 KiB/s
    mtd_speedtest: Testing 8x multi-block erase speed
    mtd_speedtest: 8x multi-block erase speed is 2340571 KiB/s
    mtd_speedtest: Testing 16x multi-block erase speed
    mtd_speedtest: 16x multi-block erase speed is 2340571 KiB/s
    mtd_speedtest: Testing 32x multi-block erase speed
    mtd_speedtest: 32x multi-block erase speed is 2340571 KiB/s
    mtd_speedtest: Testing 64x multi-block erase speed
    mtd_speedtest: 64x multi-block erase speed is 2340571 KiB/s
    mtd_speedtest: finished

  (3) Conclusion:
     The DDR quad read could be 49799 KiB/s for Spansion s25fl128s,
     The DDR quad read could be 32157/s for Micron N25Q256A;
     
Changlog:

v1 --> v2:
  [1] add the new patch: " mtd: spi-nor: add a new field for spi_nor{}"
  [2] remove the patch :
        "mtd: fsl-quadspi: get the dummy cycles for DDR Quad read from the DT
	    property"
  [3] add the DDR quad read for Micron N25Q256A.	    
  [4] fix types.
  [5] others.

before v1:  

  About this patch set:
     For patch 1, please see the old discusstion:
     http://lists.infradead.org/pipermail/linux-mtd/2014-April/053370.html

     For patch 2, please see the old discusstion:
     http://lists.infradead.org/pipermail/linux-mtd/2014-April/053374.html


Huang Shijie (10):
  mtd: spi-nor: fix the wrong dummy value
  mtd: spi-nor: add a new field for spi_nor{}
  mtd: spi-nor: add DDR quad read support
  Documentation: mtd: add a new document for SPI NOR flash
  Documentation: fsl-quadspi: update the document
  mtd: fsl-quadspi: use the information stored in spi-nor{}
  mtd: fsl-quadspi: add the DDR quad read support for Spansion NOR
  mtd: spi-nor: add more read transfer flags for n25q256a
  mtd: spi-nor: add DDR quad read support for Micron
  mtd: fsl-quadspi: add DDR quad read support for Micron

 .../devicetree/bindings/mtd/fsl-quadspi.txt        |   16 +++
 .../devicetree/bindings/mtd/spi-nor-flash.txt      |    7 +
 drivers/mtd/devices/m25p80.c                       |    6 +-
 drivers/mtd/spi-nor/fsl-quadspi.c                  |  132 ++++++++++++++------
 drivers/mtd/spi-nor/spi-nor.c                      |   65 +++++++++-
 include/linux/mtd/spi-nor.h                        |   12 ++-
 6 files changed, 189 insertions(+), 49 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/spi-nor-flash.txt

-- 
1.7.2.rc3

[PATCH v2 01/10] mtd: spi-nor: fix the wrong dummy value

[Huang Shijie]

For the DDR Quad read, the dummy cycles maybe 3 or 6 which is less then 8.
The dummy cycles is actually 8 for SPI fast/dual/quad read.

This patch makes preparations for the DDR quad read, it fixes the wrong dummy
value for both the spi-nor.c and m25p80.c.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
This ia actually v3.
---
 drivers/mtd/devices/m25p80.c  |    5 ++++-
 drivers/mtd/spi-nor/spi-nor.c |    2 +-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 1557d8f..3a1d9e0 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -128,9 +128,12 @@ static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 	struct spi_device *spi = flash->spi;
 	struct spi_transfer t[2];
 	struct spi_message m;
-	int dummy = nor->read_dummy;
+	unsigned int dummy = nor->read_dummy;
 	int ret;
 
+	/* convert the dummy cycles to the number of bytes */
+	dummy /= 8;
+
 	/* Wait till previous write/erase is done. */
 	ret = nor->wait_till_ready(nor);
 	if (ret)
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index f76f3fc..1a12f81 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -77,7 +77,7 @@ static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor)
 	case SPI_NOR_FAST:
 	case SPI_NOR_DUAL:
 	case SPI_NOR_QUAD:
-		return 1;
+		return 8;
 	case SPI_NOR_NORMAL:
 		return 0;
 	}
-- 
1.7.2.rc3

[PATCH v2 02/10] mtd: spi-nor: add a new field for spi_nor{}

[Huang Shijie]

We need the SPI NOR child node to store some specific features, such as the
dummy cycles for the DDR Quad read.

But now, we only have the @dev field in the spi_nor{}. The @dev may points to a
spi_device{} for m25p80, while it may points to a platform_deivice{} for the
SPI NOR controller, such as fsl_quadspi.c.

It is not convenient for us to get come information from the SPI NOR flash.

This patch adds a new field @np to spi_nor{}, it points to the child node for
the SPI NOR flash.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/devices/m25p80.c  |    1 +
 drivers/mtd/spi-nor/spi-nor.c |    2 +-
 include/linux/mtd/spi-nor.h   |    3 +++
 3 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 3a1d9e0..f996c3a 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -215,6 +215,7 @@ static int m25p_probe(struct spi_device *spi)
 	nor->read_reg = m25p80_read_reg;
 
 	nor->dev = &spi->dev;
+	nor->np = spi->dev.of_node;
 	nor->mtd = &flash->mtd;
 	nor->priv = flash;
 
diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 1a12f81..f374e44 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -891,7 +891,7 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id,
 	struct flash_platform_data	*data;
 	struct device *dev = nor->dev;
 	struct mtd_info *mtd = nor->mtd;
-	struct device_node *np = dev->of_node;
+	struct device_node *np = nor->np;
 	int ret;
 	int i;
 
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 5324184..48fe9fc 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -117,6 +117,8 @@ enum spi_nor_ops {
  * @mtd:		point to a mtd_info structure
  * @lock:		the lock for the read/write/erase/lock/unlock operations
  * @dev:		point to a spi device, or a spi nor controller device.
+ * @np:			If exit, it points to a device_node which stands for the
+ *			SPI NOR flash child node.
  * @page_size:		the page size of the SPI NOR
  * @addr_width:		number of address bytes
  * @erase_opcode:	the opcode for erasing a sector
@@ -148,6 +150,7 @@ struct spi_nor {
 	struct mtd_info		*mtd;
 	struct mutex		lock;
 	struct device		*dev;
+	struct device_node	*np;
 	u32			page_size;
 	u8			addr_width;
 	u8			erase_opcode;
-- 
1.7.2.rc3

[PATCH v2 03/10] mtd: spi-nor: add DDR quad read support

[Huang Shijie]

This patch adds the DDR quad read support by the following:

  [1] add SPI_NOR_DDR_QUAD read mode.

  [2] add DDR Quad read opcodes:
       SPINOR_OP_READ_1_4_4_D / SPINOR_OP_READ4_1_4_4_D

  [3] add set_ddr_quad_mode() to initialize for the DDR quad read.
      Currently it only works for Spansion NOR.

  [3] about the dummy cycles.
      We set the dummy with 8 for DDR quad read by default.
      The m25p80.c can not support the DDR quad read, but the SPI NOR controller
      can set the dummy value in its child DT node, and the SPI NOR framework
      can parse it out.

Test this patch for Spansion s25fl128s NOR flash.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/spi-nor/spi-nor.c |   54 +++++++++++++++++++++++++++++++++++++++-
 include/linux/mtd/spi-nor.h   |    8 ++++-
 2 files changed, 58 insertions(+), 4 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index f374e44..e0bc11a 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -73,7 +73,20 @@ static int read_cr(struct spi_nor *nor)
  */
 static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor)
 {
+	u32 dummy;
+
 	switch (nor->flash_read) {
+	case SPI_NOR_DDR_QUAD:
+		/*
+		 * The m25p80.c can not support the DDR quad read.
+		 * We set the dummy cycles to 8 by default. The SPI NOR
+		 * controller driver can set it in its child DT node.
+		 * We parse it out here.
+		 */
+		if (nor->np && !of_property_read_u32(nor->np,
+				"spi-nor,ddr-quad-read-dummy", &dummy)) {
+			return dummy;
+		}
 	case SPI_NOR_FAST:
 	case SPI_NOR_DUAL:
 	case SPI_NOR_QUAD:
@@ -402,6 +415,7 @@ struct flash_info {
 #define	SECT_4K_PMC		0x10	/* SPINOR_OP_BE_4K_PMC works uniformly */
 #define	SPI_NOR_DUAL_READ	0x20    /* Flash supports Dual Read */
 #define	SPI_NOR_QUAD_READ	0x40    /* Flash supports Quad Read */
+#define	SPI_NOR_DDR_QUAD_READ	0x80    /* Flash supports DDR Quad Read */
 };
 
 #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags)	\
@@ -846,6 +860,24 @@ static int spansion_quad_enable(struct spi_nor *nor)
 	return 0;
 }
 
+static int set_ddr_quad_mode(struct spi_nor *nor, u32 jedec_id)
+{
+	int status;
+
+	switch (JEDEC_MFR(jedec_id)) {
+	case CFI_MFR_AMD: /* Spansion, actually */
+		status = spansion_quad_enable(nor);
+		if (status) {
+			dev_err(nor->dev,
+				"Spansion DDR quad-read not enabled\n");
+			return status;
+		}
+		return status;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int set_quad_mode(struct spi_nor *nor, u32 jedec_id)
 {
 	int status;
@@ -1016,8 +1048,15 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id,
 	if (info->flags & SPI_NOR_NO_FR)
 		nor->flash_read = SPI_NOR_NORMAL;
 
-	/* Quad/Dual-read mode takes precedence over fast/normal */
-	if (mode == SPI_NOR_QUAD && info->flags & SPI_NOR_QUAD_READ) {
+	/* DDR Quad/Quad/Dual-read mode takes precedence over fast/normal */
+	if (mode == SPI_NOR_DDR_QUAD && info->flags & SPI_NOR_DDR_QUAD_READ) {
+		ret = set_ddr_quad_mode(nor, info->jedec_id);
+		if (ret) {
+			dev_err(dev, "DDR quad mode not supported\n");
+			return ret;
+		}
+		nor->flash_read = SPI_NOR_DDR_QUAD;
+	} else if (mode == SPI_NOR_QUAD && info->flags & SPI_NOR_QUAD_READ) {
 		ret = set_quad_mode(nor, info->jedec_id);
 		if (ret) {
 			dev_err(dev, "quad mode not supported\n");
@@ -1030,6 +1069,14 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id,
 
 	/* Default commands */
 	switch (nor->flash_read) {
+	case SPI_NOR_DDR_QUAD:
+		if (JEDEC_MFR(info->jedec_id) == CFI_MFR_AMD) { /* Spansion */
+			nor->read_opcode = SPINOR_OP_READ_1_4_4_D;
+		} else {
+			dev_err(dev, "DDR Quad Read is not supported.\n");
+			return -EINVAL;
+		}
+		break;
 	case SPI_NOR_QUAD:
 		nor->read_opcode = SPINOR_OP_READ_1_1_4;
 		break;
@@ -1057,6 +1104,9 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id,
 		if (JEDEC_MFR(info->jedec_id) == CFI_MFR_AMD) {
 			/* Dedicated 4-byte command set */
 			switch (nor->flash_read) {
+			case SPI_NOR_DDR_QUAD:
+				nor->read_opcode = SPINOR_OP_READ4_1_4_4_D;
+				break;
 			case SPI_NOR_QUAD:
 				nor->read_opcode = SPINOR_OP_READ4_1_1_4;
 				break;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index 48fe9fc..d191a6b 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -12,10 +12,11 @@
 
 /*
  * Note on opcode nomenclature: some opcodes have a format like
- * SPINOR_OP_FUNCTION{4,}_x_y_z. The numbers x, y, and z stand for the number
+ * SPINOR_OP_FUNCTION{4,}_x_y_z{_D}. The numbers x, y, and z stand for the number
  * of I/O lines used for the opcode, address, and data (respectively). The
  * FUNCTION has an optional suffix of '4', to represent an opcode which
- * requires a 4-byte (32-bit) address.
+ * requires a 4-byte (32-bit) address. The suffix of 'D' stands for the
+ * DDR mode.
  */
 
 /* Flash opcodes. */
@@ -26,6 +27,7 @@
 #define SPINOR_OP_READ_FAST	0x0b	/* Read data bytes (high frequency) */
 #define SPINOR_OP_READ_1_1_2	0x3b	/* Read data bytes (Dual SPI) */
 #define SPINOR_OP_READ_1_1_4	0x6b	/* Read data bytes (Quad SPI) */
+#define SPINOR_OP_READ_1_4_4_D	0xed	/* Read data bytes (DDR Quad SPI) */
 #define SPINOR_OP_PP		0x02	/* Page program (up to 256 bytes) */
 #define SPINOR_OP_BE_4K		0x20	/* Erase 4KiB block */
 #define SPINOR_OP_BE_4K_PMC	0xd7	/* Erase 4KiB block on PMC chips */
@@ -40,6 +42,7 @@
 #define SPINOR_OP_READ4_FAST	0x0c	/* Read data bytes (high frequency) */
 #define SPINOR_OP_READ4_1_1_2	0x3c	/* Read data bytes (Dual SPI) */
 #define SPINOR_OP_READ4_1_1_4	0x6c	/* Read data bytes (Quad SPI) */
+#define SPINOR_OP_READ4_1_4_4_D	0xee	/* Read data bytes (DDR Quad SPI) */
 #define SPINOR_OP_PP_4B		0x12	/* Page program (up to 256 bytes) */
 #define SPINOR_OP_SE_4B		0xdc	/* Sector erase (usually 64KiB) */
 
@@ -74,6 +77,7 @@ enum read_mode {
 	SPI_NOR_FAST,
 	SPI_NOR_DUAL,
 	SPI_NOR_QUAD,
+	SPI_NOR_DDR_QUAD,
 };
 
 /**
-- 
1.7.2.rc3

[PATCH v2 04/10] Documentation: mtd: add a new document for SPI NOR flash

[Huang Shijie]

We need a DT property to store the dummy cycles for DDR Quad read.
This is a common feature for the SPI NOR flash, such as Spansion and Micron
chips.

Add this file to describe this specific SPI NOR flash features which will
be referred by the SPI NOR flash drivers.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 .../devicetree/bindings/mtd/spi-nor-flash.txt      |    7 +++++++
 1 files changed, 7 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mtd/spi-nor-flash.txt

diff --git a/Documentation/devicetree/bindings/mtd/spi-nor-flash.txt b/Documentation/devicetree/bindings/mtd/spi-nor-flash.txt
new file mode 100644
index 0000000..aba4d54
--- /dev/null
+++ b/Documentation/devicetree/bindings/mtd/spi-nor-flash.txt
@@ -0,0 +1,7 @@
+This file defines some DT properties for specific SPI NOR flash features.
+The SPI NOR controller drivers may refer to this file, such as fsl-quadspi.txt
+
+Optional properties:
+  - spi-nor,ddr-quad-read-dummy: The dummy cycles used by the DDR Quad read.
+                                 Please refer to the chip's datasheet. This
+                                 property can be 4 or 6 which is less then 8.
-- 
1.7.2.rc3

[PATCH v2 05/10] Documentation: fsl-quadspi: update the document

[Huang Shijie]

The patch updates the document by adding more information to describe the
DT proporties used by the Freescale Quadspi driver and the childs nodes.

For the child node for SPI NOR flash, we add the required property
("spi-max-frequency"), and refer to spi-nor-flash.txt for the optional
properties.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 .../devicetree/bindings/mtd/fsl-quadspi.txt        |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt b/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt
index 823d134..7e1dbaf 100644
--- a/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt
+++ b/Documentation/devicetree/bindings/mtd/fsl-quadspi.txt
@@ -1,5 +1,11 @@
 * Freescale Quad Serial Peripheral Interface(QuadSPI)
 
+The QuadSPI controller acts as the SPI master. It is described with a node
+for the controller and a set of child nodes for each SPI NOR flash.
+
+Part I - The DT node for the controller:
+------------------------------
+
 Required properties:
   - compatible : Should be "fsl,vf610-qspi"
   - reg : the first contains the register location and length,
@@ -18,6 +24,16 @@ Optional properties:
 			      bus, you should enable this property.
 			      (Please check the board's schematic.)
 
+Part II - The DT nodes for each SPI NOR flash
+------------------------------
+Required properties:
+- spi-max-frequency : Maximum frequency of the SPI bus the chip can operate at
+
+Optional properties:
+  Please refer to the Documentation/devicetree/bindings/mtd/spi-nor-flash.txt
+  If you set the "spi-nor,ddr-quad-read-dummy", it means you enable the DDR
+  quad read feature for the driver.
+
 Example:
 
 qspi0: quadspi at 40044000 {
-- 
1.7.2.rc3

[PATCH v2 06/10] mtd: fsl-quadspi: use the information stored in spi-nor{}

[Huang Shijie]

We can get the read/write/erase opcode from the spi nor framework now.
What's more is that we can get the correct dummy cycles.

This patch uses the information stored in the spi_nor{} to remove the
hardcode in the fsl_qspi_init_lut().

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/spi-nor/fsl-quadspi.c |   57 ++++++++++++------------------------
 1 files changed, 19 insertions(+), 38 deletions(-)

diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
index 8d659a2..4adf79e 100644
--- a/drivers/mtd/spi-nor/fsl-quadspi.c
+++ b/drivers/mtd/spi-nor/fsl-quadspi.c
@@ -280,8 +280,10 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 {
 	void __iomem *base = q->iobase;
 	int rxfifo = q->devtype_data->rxfifo;
+	struct spi_nor *nor = &q->nor[0];
+	u8 addrlen = (nor->addr_width == 3) ? ADDR24BIT : ADDR32BIT;
 	u32 lut_base;
-	u8 cmd, addrlen, dummy;
+	u8 op, dm;
 	int i;
 
 	fsl_qspi_unlock_lut(q);
@@ -292,40 +294,28 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 
 	/* Quad Read */
 	lut_base = SEQID_QUAD_READ * 4;
-
-	if (q->nor_size <= SZ_16M) {
-		cmd = SPINOR_OP_READ_1_1_4;
-		addrlen = ADDR24BIT;
-		dummy = 8;
-	} else {
-		/* use the 4-byte address */
-		cmd = SPINOR_OP_READ_1_1_4;
-		addrlen = ADDR32BIT;
-		dummy = 8;
+	op = nor->read_opcode;
+	dm = nor->read_dummy;
+	if (nor->flash_read == SPI_NOR_QUAD) {
+		if (op == SPINOR_OP_READ_1_1_4 || op == SPINOR_OP_READ4_1_1_4) {
+			/* read mode : 1-1-4 */
+			writel(LUT0(CMD, PAD1, op) | LUT1(ADDR, PAD1, addrlen),
+				base + QUADSPI_LUT(lut_base));
+
+			writel(LUT0(DUMMY, PAD1, dm) | LUT1(READ, PAD4, rxfifo),
+				base + QUADSPI_LUT(lut_base + 1));
+		} else {
+			dev_err(nor->dev, "Unsupported opcode : 0x%.2x\n", op);
+		}
 	}
 
-	writel(LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
-			base + QUADSPI_LUT(lut_base));
-	writel(LUT0(DUMMY, PAD1, dummy) | LUT1(READ, PAD4, rxfifo),
-			base + QUADSPI_LUT(lut_base + 1));
-
 	/* Write enable */
 	lut_base = SEQID_WREN * 4;
 	writel(LUT0(CMD, PAD1, SPINOR_OP_WREN), base + QUADSPI_LUT(lut_base));
 
 	/* Page Program */
 	lut_base = SEQID_PP * 4;
-
-	if (q->nor_size <= SZ_16M) {
-		cmd = SPINOR_OP_PP;
-		addrlen = ADDR24BIT;
-	} else {
-		/* use the 4-byte address */
-		cmd = SPINOR_OP_PP;
-		addrlen = ADDR32BIT;
-	}
-
-	writel(LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
+	writel(LUT0(CMD, PAD1, nor->program_opcode) | LUT1(ADDR, PAD1, addrlen),
 			base + QUADSPI_LUT(lut_base));
 	writel(LUT0(WRITE, PAD1, 0), base + QUADSPI_LUT(lut_base + 1));
 
@@ -336,17 +326,7 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 
 	/* Erase a sector */
 	lut_base = SEQID_SE * 4;
-
-	if (q->nor_size <= SZ_16M) {
-		cmd = SPINOR_OP_SE;
-		addrlen = ADDR24BIT;
-	} else {
-		/* use the 4-byte address */
-		cmd = SPINOR_OP_SE;
-		addrlen = ADDR32BIT;
-	}
-
-	writel(LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
+	writel(LUT0(CMD, PAD1, nor->erase_opcode) | LUT1(ADDR, PAD1, addrlen),
 			base + QUADSPI_LUT(lut_base));
 
 	/* Erase the whole chip */
@@ -396,6 +376,7 @@ static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
 		return SEQID_WRDI;
 	case SPINOR_OP_RDSR:
 		return SEQID_RDSR;
+	case SPINOR_OP_BE_4K:
 	case SPINOR_OP_SE:
 		return SEQID_SE;
 	case SPINOR_OP_CHIP_ERASE:
-- 
1.7.2.rc3

[PATCH v2 07/10] mtd: fsl-quadspi: add the DDR quad read support for Spansion NOR

[Huang Shijie]

Add the DDR quad read support for the fsl-quadspi driver.

Check the "spi-nor,ddr-quad-read-dummy" DT property, if the DT node is exit,
it means we could enable the DDR quad read.

 (1) Test this patch with imx6sx-sdb board (Spansion s25fl128s)
     The clock rate is 66MHz.

 (2) The information of NOR flash:
     -----------------------------------------------
     root at imx6qdlsolo:~# mtdinfo /dev/mtd0
     mtd0
     Name:                           21e4000.qspi
     Type:                           nor
     Eraseblock size:                65536 bytes, 64.0 KiB
     Amount of eraseblocks:          256 (16777216 bytes, 16.0 MiB)
     Minimum input/output unit size: 1 byte
     Sub-page size:                  1 byte
     Character device major/minor:   90:0
     Bad blocks are allowed:         false
     Device is writable:             true
     -----------------------------------------------

 (3) Test this patch set with UBIFS & bonnie++:
     -----------------------------------------------
	ubiattach /dev/ubi_ctrl -m 0
	ubimkvol /dev/ubi0 -N test -m
	mount -t ubifs ubi0:test tmp
	bonnie++ -d tmp -u 0 -s 10 -r 5
     -----------------------------------------------

 (4) Test this patch with mtd_speedtest.ko

     root at imx6qdlsolo:~# insmod mtd_speedtest.ko dev=0
     =================================================
     mtd_speedtest: MTD device: 0
     mtd_speedtest: not NAND flash, assume page size is 512 bytes.
     mtd_speedtest: MTD device size 16777216, eraseblock size 65536, page size 512,
                    count of eraseblocks 256, pages per eraseblock 128, OOB size 0
     mtd_speedtest: testing eraseblock write speed
     mtd_speedtest: eraseblock write speed is 665 KiB/s
     mtd_speedtest: testing eraseblock read speed
     mtd_speedtest: eraseblock read speed is 49799 KiB/s
     mtd_speedtest: testing page write speed
     mtd_speedtest: page write speed is 662 KiB/s
     mtd_speedtest: testing page read speed
     mtd_speedtest: page read speed is 24236 KiB/s
     mtd_speedtest: testing 2 page write speed
     mtd_speedtest: 2 page write speed is 657 KiB/s
     mtd_speedtest: testing 2 page read speed
     mtd_speedtest: 2 page read speed is 32637 KiB/s
     mtd_speedtest: Testing erase speed
     mtd_speedtest: erase speed is 518 KiB/s
     mtd_speedtest: Testing 2x multi-block erase speed
     mtd_speedtest: 2x multi-block erase speed is 506 KiB/s
     mtd_speedtest: Testing 4x multi-block erase speed
     mtd_speedtest: 4x multi-block erase speed is 503 KiB/s
     mtd_speedtest: Testing 8x multi-block erase speed
     mtd_speedtest: 8x multi-block erase speed is 501 KiB/s
     mtd_speedtest: Testing 16x multi-block erase speed
     mtd_speedtest: 16x multi-block erase speed is 498 KiB/s
     mtd_speedtest: Testing 32x multi-block erase speed
     mtd_speedtest: 32x multi-block erase speed is 496 KiB/s
     mtd_speedtest: Testing 64x multi-block erase speed
     mtd_speedtest: 64x multi-block erase speed is 495 KiB/s
     mtd_speedtest: finished
     =================================================

  (5) Conclusion:
     The DDR quad read could be 49799 KiB/s.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/spi-nor/fsl-quadspi.c |   62 +++++++++++++++++++++++++++++++++++--
 1 files changed, 59 insertions(+), 3 deletions(-)

diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
index 4adf79e..a5dbc62 100644
--- a/drivers/mtd/spi-nor/fsl-quadspi.c
+++ b/drivers/mtd/spi-nor/fsl-quadspi.c
@@ -29,6 +29,9 @@
 
 /* The registers */
 #define QUADSPI_MCR			0x00
+#define MX6SX_QUADSPI_MCR_TX_DDR_DELAY_EN_SHIFT	29
+#define MX6SX_QUADSPI_MCR_TX_DDR_DELAY_EN_MASK	\
+				(1 << MX6SX_QUADSPI_MCR_TX_DDR_DELAY_EN_SHIFT)
 #define QUADSPI_MCR_RESERVED_SHIFT	16
 #define QUADSPI_MCR_RESERVED_MASK	(0xF << QUADSPI_MCR_RESERVED_SHIFT)
 #define QUADSPI_MCR_MDIS_SHIFT		14
@@ -292,7 +295,7 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 	for (i = 0; i < QUADSPI_LUT_NUM; i++)
 		writel(0, base + QUADSPI_LUT_BASE + i * 4);
 
-	/* Quad Read */
+	/* Quad Read and DDR Quad Read*/
 	lut_base = SEQID_QUAD_READ * 4;
 	op = nor->read_opcode;
 	dm = nor->read_dummy;
@@ -307,6 +310,24 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 		} else {
 			dev_err(nor->dev, "Unsupported opcode : 0x%.2x\n", op);
 		}
+	} else if (nor->flash_read == SPI_NOR_DDR_QUAD) {
+		if (op == SPINOR_OP_READ_1_4_4_D ||
+			 op == SPINOR_OP_READ4_1_4_4_D) {
+			/* read mode : 1-4-4, such as Spansion s25fl128s. */
+			writel(LUT0(CMD, PAD1, op)
+				| LUT1(ADDR_DDR, PAD4, addrlen),
+				base + QUADSPI_LUT(lut_base));
+
+			writel(LUT0(MODE_DDR, PAD4, 0xff)
+				| LUT1(DUMMY, PAD1, dm),
+				base + QUADSPI_LUT(lut_base + 1));
+
+			writel(LUT0(READ_DDR, PAD4, rxfifo)
+				| LUT1(JMP_ON_CS, PAD1, 0),
+				base + QUADSPI_LUT(lut_base + 2));
+		} else {
+			dev_err(nor->dev, "Unsupported opcode : 0x%.2x\n", op);
+		}
 	}
 
 	/* Write enable */
@@ -368,6 +389,9 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
 {
 	switch (cmd) {
+	case SPINOR_OP_READ_1_4_4_D:
+	case SPINOR_OP_READ4_1_4_4_D:
+	case SPINOR_OP_READ4_1_1_4:
 	case SPINOR_OP_READ_1_1_4:
 		return SEQID_QUAD_READ;
 	case SPINOR_OP_WREN:
@@ -558,6 +582,8 @@ static void fsl_qspi_set_map_addr(struct fsl_qspi *q)
 static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
 {
 	void __iomem *base = q->iobase;
+	struct spi_nor *nor = &q->nor[0];
+	u32 reg, reg2;
 	int seqid;
 
 	/* AHB configuration for access buffer 0/1/2 .*/
@@ -572,9 +598,30 @@ static void fsl_qspi_init_abh_read(struct fsl_qspi *q)
 	writel(0, base + QUADSPI_BUF2IND);
 
 	/* Set the default lut sequence for AHB Read. */
-	seqid = fsl_qspi_get_seqid(q, q->nor[0].read_opcode);
+	seqid = fsl_qspi_get_seqid(q, nor->read_opcode);
 	writel(seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
 		q->iobase + QUADSPI_BFGENCR);
+
+	/* enable the DDR quad read */
+	if (nor->flash_read == SPI_NOR_DDR_QUAD) {
+		reg = readl(q->iobase + QUADSPI_MCR);
+
+		/* Firstly, disable the module */
+		writel(reg | QUADSPI_MCR_MDIS_MASK, q->iobase + QUADSPI_MCR);
+
+		/* Set the Sampling Register for DDR */
+		reg2 = readl(q->iobase + QUADSPI_SMPR);
+		reg2 &= ~QUADSPI_SMPR_DDRSMP_MASK;
+		reg2 |= (2 << QUADSPI_SMPR_DDRSMP_SHIFT);
+		writel(reg2, q->iobase + QUADSPI_SMPR);
+
+		/* Enable the module again (enable the DDR too) */
+		reg |= QUADSPI_MCR_DDR_EN_MASK;
+		if (is_imx6sx_qspi(q))
+			reg |= MX6SX_QUADSPI_MCR_TX_DDR_DELAY_EN_MASK;
+
+		writel(reg, q->iobase + QUADSPI_MCR);
+	}
 }
 
 /* We use this function to do some basic init for spi_nor_scan(). */
@@ -863,7 +910,9 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 	/* iterate the subnodes. */
 	for_each_available_child_of_node(dev->of_node, np) {
 		const struct spi_device_id *id;
+		enum read_mode mode = SPI_NOR_QUAD;
 		char modalias[40];
+		u32 dummy = 0;
 
 		/* skip the holes */
 		if (!has_second_chip)
@@ -874,6 +923,7 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 
 		nor->mtd = mtd;
 		nor->dev = dev;
+		nor->np = np;
 		nor->priv = q;
 		mtd->priv = nor;
 
@@ -899,10 +949,16 @@ static int fsl_qspi_probe(struct platform_device *pdev)
 		if (ret < 0)
 			goto map_failed;
 
+		/* Can we enable the DDR Quad Read? */
+		ret = of_property_read_u32(np, "spi-nor,ddr-quad-read-dummy",
+					&dummy);
+		if (!ret && dummy > 0)
+			mode = SPI_NOR_DDR_QUAD;
+
 		/* set the chip address for READID */
 		fsl_qspi_set_base_addr(q, nor);
 
-		ret = spi_nor_scan(nor, id, SPI_NOR_QUAD);
+		ret = spi_nor_scan(nor, id, mode);
 		if (ret)
 			goto map_failed;
 
-- 
1.7.2.rc3

[PATCH v2 08/10] mtd: spi-nor: add more read transfer flags for n25q256a

[Huang Shijie]

The NOR flash can supports dual/quad/ddr-quad read.
Add more flags for these read transfers.

>From the datasheet, the chip support the 64K sector erase operation.
So remove the SECT_4K for the chip which makes the flash_erase faster.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/spi-nor/spi-nor.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index e0bc11a..07d249c 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -500,7 +500,7 @@ const struct spi_device_id spi_nor_ids[] = {
 	{ "n25q064",     INFO(0x20ba17, 0, 64 * 1024,  128, 0) },
 	{ "n25q128a11",  INFO(0x20bb18, 0, 64 * 1024,  256, 0) },
 	{ "n25q128a13",  INFO(0x20ba18, 0, 64 * 1024,  256, 0) },
-	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SECT_4K) },
+	{ "n25q256a",    INFO(0x20ba19, 0, 64 * 1024,  512, SPI_NOR_DUAL_READ | SPI_NOR_QUAD_READ | SPI_NOR_DDR_QUAD_READ) },
 	{ "n25q512a",    INFO(0x20bb20, 0, 64 * 1024, 1024, SECT_4K) },
 
 	/* PMC */
-- 
1.7.2.rc3

[PATCH v2 09/10] mtd: spi-nor: add DDR quad read support for Micron

[Huang Shijie]

This patch adds the DDR(or DTR) quad read support for the Micron
SPI NOR flash.

Tested with n25q256a.

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/spi-nor/spi-nor.c |    5 +++++
 include/linux/mtd/spi-nor.h   |    1 +
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
index 07d249c..c5ea969 100644
--- a/drivers/mtd/spi-nor/spi-nor.c
+++ b/drivers/mtd/spi-nor/spi-nor.c
@@ -873,6 +873,9 @@ static int set_ddr_quad_mode(struct spi_nor *nor, u32 jedec_id)
 			return status;
 		}
 		return status;
+	case CFI_MFR_ST: /* Micron, actually */
+		/* DTR quad read works with the Extended SPI protocol. */
+		return 0;
 	default:
 		return -EINVAL;
 	}
@@ -1072,6 +1075,8 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id,
 	case SPI_NOR_DDR_QUAD:
 		if (JEDEC_MFR(info->jedec_id) == CFI_MFR_AMD) { /* Spansion */
 			nor->read_opcode = SPINOR_OP_READ_1_4_4_D;
+		} else if (JEDEC_MFR(info->jedec_id) == CFI_MFR_ST) {
+			nor->read_opcode = SPINOR_OP_READ_1_1_4_D;
 		} else {
 			dev_err(dev, "DDR Quad Read is not supported.\n");
 			return -EINVAL;
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index d191a6b..2fb40b6 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -27,6 +27,7 @@
 #define SPINOR_OP_READ_FAST	0x0b	/* Read data bytes (high frequency) */
 #define SPINOR_OP_READ_1_1_2	0x3b	/* Read data bytes (Dual SPI) */
 #define SPINOR_OP_READ_1_1_4	0x6b	/* Read data bytes (Quad SPI) */
+#define SPINOR_OP_READ_1_1_4_D	0x6d	/* Read data bytes (DDR Quad SPI) */
 #define SPINOR_OP_READ_1_4_4_D	0xed	/* Read data bytes (DDR Quad SPI) */
 #define SPINOR_OP_PP		0x02	/* Page program (up to 256 bytes) */
 #define SPINOR_OP_BE_4K		0x20	/* Erase 4KiB block */
-- 
1.7.2.rc3

[PATCH v2 10/10] mtd: fsl-quadspi: add DDR quad read support for Micron

[Huang Shijie]

Add DDR quad read opcode and LUT sequence for Micron N25Q256A.

The performace :
    =================================================
    mtd_speedtest: MTD device: 1
    mtd_speedtest: not NAND flash, assume page size is 512 bytes.
    mtd_speedtest: MTD device size 33554432, eraseblock size 65536,
                   page size 512, count of eraseblocks 512, pages per eraseblock 128, OOB size 0
    mtd_speedtest: testing eraseblock write speed
    mtd_speedtest: eraseblock write speed is 2426 KiB/s
    mtd_speedtest: testing eraseblock read speed
    mtd_speedtest: eraseblock read speed is 32157 KiB/s
    mtd_speedtest: testing page write speed
    mtd_speedtest: page write speed is 2362 KiB/s
    mtd_speedtest: testing page read speed
    mtd_speedtest: page read speed is 17741 KiB/s
    mtd_speedtest: testing 2 page write speed
    mtd_speedtest: 2 page write speed is 2384 KiB/s
    mtd_speedtest: testing 2 page read speed
    mtd_speedtest: 2 page read speed is 24058 KiB/s
    mtd_speedtest: Testing erase speed
    mtd_speedtest: erase speed is 1927529 KiB/s
    mtd_speedtest: Testing 2x multi-block erase speed
    mtd_speedtest: 2x multi-block erase speed is 2184533 KiB/s
    mtd_speedtest: Testing 4x multi-block erase speed
    mtd_speedtest: 4x multi-block erase speed is 2184533 KiB/s
    mtd_speedtest: Testing 8x multi-block erase speed
    mtd_speedtest: 8x multi-block erase speed is 2340571 KiB/s
    mtd_speedtest: Testing 16x multi-block erase speed
    mtd_speedtest: 16x multi-block erase speed is 2340571 KiB/s
    mtd_speedtest: Testing 32x multi-block erase speed
    mtd_speedtest: 32x multi-block erase speed is 2340571 KiB/s
    mtd_speedtest: Testing 64x multi-block erase speed
    mtd_speedtest: 64x multi-block erase speed is 2340571 KiB/s
    mtd_speedtest: finished
    =================================================

Signed-off-by: Huang Shijie <b32955@freescale.com>
---
 drivers/mtd/spi-nor/fsl-quadspi.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/spi-nor/fsl-quadspi.c b/drivers/mtd/spi-nor/fsl-quadspi.c
index a5dbc62..08944cb 100644
--- a/drivers/mtd/spi-nor/fsl-quadspi.c
+++ b/drivers/mtd/spi-nor/fsl-quadspi.c
@@ -325,6 +325,18 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 			writel(LUT0(READ_DDR, PAD4, rxfifo)
 				| LUT1(JMP_ON_CS, PAD1, 0),
 				base + QUADSPI_LUT(lut_base + 2));
+		} else if (op == SPINOR_OP_READ_1_1_4_D) {
+			/* read mode : 1-1-4, such as Micron N25Q256A. */
+			writel(LUT0(CMD, PAD1, op)
+				| LUT1(ADDR_DDR, PAD1, addrlen),
+				base + QUADSPI_LUT(lut_base));
+
+			writel(LUT0(DUMMY, PAD1, dm)
+				| LUT1(READ_DDR, PAD4, rxfifo),
+				base + QUADSPI_LUT(lut_base + 1));
+
+			writel(LUT0(JMP_ON_CS, PAD1, 0),
+				base + QUADSPI_LUT(lut_base + 2));
 		} else {
 			dev_err(nor->dev, "Unsupported opcode : 0x%.2x\n", op);
 		}
@@ -389,6 +401,7 @@ static void fsl_qspi_init_lut(struct fsl_qspi *q)
 static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
 {
 	switch (cmd) {
+	case SPINOR_OP_READ_1_1_4_D:
 	case SPINOR_OP_READ_1_4_4_D:
 	case SPINOR_OP_READ4_1_4_4_D:
 	case SPINOR_OP_READ4_1_1_4:
-- 
1.7.2.rc3

[PATCH v2 01/10] mtd: spi-nor: fix the wrong dummy value

[Marek Vasut]

On Monday, April 28, 2014 at 05:53:38 AM, Huang Shijie wrote:
> For the DDR Quad read, the dummy cycles maybe 3 or 6 which is less then 8.
> The dummy cycles is actually 8 for SPI fast/dual/quad read.
> 
> This patch makes preparations for the DDR quad read, it fixes the wrong
> dummy value for both the spi-nor.c and m25p80.c.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
> This ia actually v3.

Acked-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

[PATCH v2 02/10] mtd: spi-nor: add a new field for spi_nor{}

[Marek Vasut]

On Monday, April 28, 2014 at 05:53:39 AM, Huang Shijie wrote:
> We need the SPI NOR child node to store some specific features, such as the
> dummy cycles for the DDR Quad read.
> 
> But now, we only have the @dev field in the spi_nor{}. The @dev may points
> to a spi_device{} for m25p80, while it may points to a platform_deivice{}
> for the SPI NOR controller, such as fsl_quadspi.c.
> 
> It is not convenient for us to get come information from the SPI NOR flash.
> 
> This patch adds a new field @np to spi_nor{}, it points to the child node
> for the SPI NOR flash.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>

Just handle the case where dev->of_node == NULL instead ?

Best regards,
Marek Vasut

[PATCH v2 03/10] mtd: spi-nor: add DDR quad read support

[Marek Vasut]

On Monday, April 28, 2014 at 05:53:40 AM, Huang Shijie wrote:
> This patch adds the DDR quad read support by the following:
> 
>   [1] add SPI_NOR_DDR_QUAD read mode.
> 
>   [2] add DDR Quad read opcodes:
>        SPINOR_OP_READ_1_4_4_D / SPINOR_OP_READ4_1_4_4_D
> 
>   [3] add set_ddr_quad_mode() to initialize for the DDR quad read.
>       Currently it only works for Spansion NOR.
> 
>   [3] about the dummy cycles.
>       We set the dummy with 8 for DDR quad read by default.
>       The m25p80.c can not support the DDR quad read, but the SPI NOR
> controller can set the dummy value in its child DT node, and the SPI NOR
> framework can parse it out.
> 
> Test this patch for Spansion s25fl128s NOR flash.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>

Acked-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

[PATCH v2 02/10] mtd: spi-nor: add a new field for spi_nor{}

[Huang Shijie]

On Mon, Apr 28, 2014 at 10:23:26PM +0200, Marek Vasut wrote:
> On Monday, April 28, 2014 at 05:53:39 AM, Huang Shijie wrote:
> > We need the SPI NOR child node to store some specific features, such as the
> > dummy cycles for the DDR Quad read.
> > 
> > But now, we only have the @dev field in the spi_nor{}. The @dev may points
> > to a spi_device{} for m25p80, while it may points to a platform_deivice{}
> > for the SPI NOR controller, such as fsl_quadspi.c.
> > 
> > It is not convenient for us to get come information from the SPI NOR flash.
> > 
> > This patch adds a new field @np to spi_nor{}, it points to the child node
> > for the SPI NOR flash.
> > 
> > Signed-off-by: Huang Shijie <b32955@freescale.com>
> 
> Just handle the case where dev->of_node == NULL instead ?
It is not enough. 

For the m25p80.c, @dev stands for a child node for the SPI master,
and it points to a spi_device{}. Yes, in this case, the dev->of_node is NULL.

But for the fsl_quadspi or other SPI NOR drivers, the @dev stands for the controller
itself, the @dev->of_node is a list of the child nodes, so we can _NOT_ know which
child node we are working at now.


So it's better to add a new field @np for the spi-nor{} which points the
child node we are working at.

thanks
Huang Shijie

[PATCH v2 02/10] mtd: spi-nor: add a new field for spi_nor{}

[Huang Shijie]

On Tue, Apr 29, 2014 at 08:54:24AM +0200, Marek Vasut wrote:
> On Tuesday, April 29, 2014 at 07:18:34 AM, Huang Shijie wrote:
> > For the m25p80.c, @dev stands for a child node for the SPI master,
> > and it points to a spi_device{}. Yes, in this case, the dev->of_node is
> > NULL.
> > 
> > But for the fsl_quadspi or other SPI NOR drivers, the @dev stands for the
> > controller itself, the @dev->of_node is a list of the child nodes, so we
> > can _NOT_ know which child node we are working at now.
> 
> Huh ? The dev is being recycled for two different kind of things ?
yes.

for the SPI bus, the of_register_spi_devices() will allocate a spi_device{}
for each child node for the SPI NOR flash. So in the m25p80.c, the @dev points
to a spi_device{}.

For the simplicity,  we do not allocate any *_device{} for the child
node in the SPI NOR flash driver, such as in the fsl-quadspi.c.

thanks
Huang Shijie

[PATCH v2 02/10] mtd: spi-nor: add a new field for spi_nor{}

[Marek Vasut]

On Tuesday, April 29, 2014 at 07:18:34 AM, Huang Shijie wrote:
> On Mon, Apr 28, 2014 at 10:23:26PM +0200, Marek Vasut wrote:
> > On Monday, April 28, 2014 at 05:53:39 AM, Huang Shijie wrote:
> > > We need the SPI NOR child node to store some specific features, such as
> > > the dummy cycles for the DDR Quad read.
> > > 
> > > But now, we only have the @dev field in the spi_nor{}. The @dev may
> > > points to a spi_device{} for m25p80, while it may points to a
> > > platform_deivice{} for the SPI NOR controller, such as fsl_quadspi.c.
> > > 
> > > It is not convenient for us to get come information from the SPI NOR
> > > flash.
> > > 
> > > This patch adds a new field @np to spi_nor{}, it points to the child
> > > node for the SPI NOR flash.
> > > 
> > > Signed-off-by: Huang Shijie <b32955@freescale.com>
> > 
> > Just handle the case where dev->of_node == NULL instead ?
> 
> It is not enough.
> 
> For the m25p80.c, @dev stands for a child node for the SPI master,
> and it points to a spi_device{}. Yes, in this case, the dev->of_node is
> NULL.
> 
> But for the fsl_quadspi or other SPI NOR drivers, the @dev stands for the
> controller itself, the @dev->of_node is a list of the child nodes, so we
> can _NOT_ know which child node we are working at now.

Huh ? The dev is being recycled for two different kind of things ?

> So it's better to add a new field @np for the spi-nor{} which points the
> child node we are working at.

Best regards,
Marek Vasut

[PATCH v2 03/10] mtd: spi-nor: add DDR quad read support

[Brian Norris]

Hi Huang,

Sorry to address this series so late.

I have a few questions about how you determine support for these DDR
modes.

On Mon, Apr 28, 2014 at 11:53:40AM +0800, Huang Shijie wrote:
> This patch adds the DDR quad read support by the following:

To Mark / linux-spi:

Are DDR modes in the scope of drivers/spi/ at all, so that we could
someday wire it up through m25p80.c? Or is 'DDR' such a distortion of
the meaning of 'SPI' such that it will be restricted only to SPI NOR
dedicated controllers?

>   [1] add SPI_NOR_DDR_QUAD read mode.
> 
>   [2] add DDR Quad read opcodes:
>        SPINOR_OP_READ_1_4_4_D / SPINOR_OP_READ4_1_4_4_D
> 
>   [3] add set_ddr_quad_mode() to initialize for the DDR quad read.
>       Currently it only works for Spansion NOR.
> 
>   [3] about the dummy cycles.
>       We set the dummy with 8 for DDR quad read by default.

Why? That seems wrong. You need to know for sure how many cycles should
be used, not just guess a default.

>       The m25p80.c can not support the DDR quad read, but the SPI NOR controller
>       can set the dummy value in its child DT node, and the SPI NOR framework
>       can parse it out.

Why does the dummy value belong in device tree? I think this can be
handled in software. You might, however, want a few other hardware
description parameters in device tree to help you.

So I think spi-nor.c needs to know a few things:

 1. Does the hardware/driver support DDR clocking?
 2. What granularity of dummy cycles are supported? So m25p80.c needs to
    communicate that it only supports dummy cycles of multiples of 8,
    and fsl-quadspi supports single clock cycles.

And spi-nor.c should be able to do the following:

 3. Set how many dummy cycles should be used.
 4. Write to the configuration register, to choose a Latency Code
    according to what the flash supports. I see modes that support 3, 6,
    7, or 8. We'd probably just go for the fastest mode, which requires
    8, right?

So far, none of this seems to require a DT binding, unless there's
something I'm missing about your fsl-quadspi controller.

> Test this patch for Spansion s25fl128s NOR flash.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
>  drivers/mtd/spi-nor/spi-nor.c |   54 +++++++++++++++++++++++++++++++++++++++-
>  include/linux/mtd/spi-nor.h   |    8 ++++-
>  2 files changed, 58 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/mtd/spi-nor/spi-nor.c b/drivers/mtd/spi-nor/spi-nor.c
> index f374e44..e0bc11a 100644
> --- a/drivers/mtd/spi-nor/spi-nor.c
> +++ b/drivers/mtd/spi-nor/spi-nor.c
> @@ -73,7 +73,20 @@ static int read_cr(struct spi_nor *nor)
>   */
>  static inline int spi_nor_read_dummy_cycles(struct spi_nor *nor)
>  {
> +	u32 dummy;
> +
>  	switch (nor->flash_read) {
> +	case SPI_NOR_DDR_QUAD:
> +		/*
> +		 * The m25p80.c can not support the DDR quad read.
> +		 * We set the dummy cycles to 8 by default. The SPI NOR
> +		 * controller driver can set it in its child DT node.
> +		 * We parse it out here.
> +		 */
> +		if (nor->np && !of_property_read_u32(nor->np,
> +				"spi-nor,ddr-quad-read-dummy", &dummy)) {
> +			return dummy;
> +		}
>  	case SPI_NOR_FAST:
>  	case SPI_NOR_DUAL:
>  	case SPI_NOR_QUAD:
> @@ -402,6 +415,7 @@ struct flash_info {
>  #define	SECT_4K_PMC		0x10	/* SPINOR_OP_BE_4K_PMC works uniformly */
>  #define	SPI_NOR_DUAL_READ	0x20    /* Flash supports Dual Read */
>  #define	SPI_NOR_QUAD_READ	0x40    /* Flash supports Quad Read */
> +#define	SPI_NOR_DDR_QUAD_READ	0x80    /* Flash supports DDR Quad Read */
>  };
>  
>  #define INFO(_jedec_id, _ext_id, _sector_size, _n_sectors, _flags)	\
> @@ -846,6 +860,24 @@ static int spansion_quad_enable(struct spi_nor *nor)
>  	return 0;
>  }
>  
> +static int set_ddr_quad_mode(struct spi_nor *nor, u32 jedec_id)
> +{
> +	int status;
> +
> +	switch (JEDEC_MFR(jedec_id)) {
> +	case CFI_MFR_AMD: /* Spansion, actually */
> +		status = spansion_quad_enable(nor);
> +		if (status) {
> +			dev_err(nor->dev,
> +				"Spansion DDR quad-read not enabled\n");
> +			return status;
> +		}
> +		return status;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static int set_quad_mode(struct spi_nor *nor, u32 jedec_id)
>  {
>  	int status;
> @@ -1016,8 +1048,15 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id,
>  	if (info->flags & SPI_NOR_NO_FR)
>  		nor->flash_read = SPI_NOR_NORMAL;
>  
> -	/* Quad/Dual-read mode takes precedence over fast/normal */
> -	if (mode == SPI_NOR_QUAD && info->flags & SPI_NOR_QUAD_READ) {
> +	/* DDR Quad/Quad/Dual-read mode takes precedence over fast/normal */
> +	if (mode == SPI_NOR_DDR_QUAD && info->flags & SPI_NOR_DDR_QUAD_READ) {

Hmm, I think I should probably take another look at the design of
spi-nor.c... Why does spi_nor_scan() take a single 'mode' argument? The
driver should be communicating which (multiple) modes it supports, not
selecting a single mode. spi-nor.c is the only one which knows what the
*flash* supports, so it should be combining knowledge from the
controller driver with its own knowledge of the flash.

> +		ret = set_ddr_quad_mode(nor, info->jedec_id);
> +		if (ret) {
> +			dev_err(dev, "DDR quad mode not supported\n");
> +			return ret;

A ramification of my comment above is that we should not be returning an
error in a situation like this; we should be able to fall back to
another known-supported mode, like SDR QUAD, SDR DUAL, or just plain
SPI -- if they're supported by the driver -- and spi-nor.c doesn't
currently have enough information to do this.

> +		}
> +		nor->flash_read = SPI_NOR_DDR_QUAD;
> +	} else if (mode == SPI_NOR_QUAD && info->flags & SPI_NOR_QUAD_READ) {
>  		ret = set_quad_mode(nor, info->jedec_id);
>  		if (ret) {
>  			dev_err(dev, "quad mode not supported\n");
> @@ -1030,6 +1069,14 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id,
>  
>  	/* Default commands */
>  	switch (nor->flash_read) {
> +	case SPI_NOR_DDR_QUAD:
> +		if (JEDEC_MFR(info->jedec_id) == CFI_MFR_AMD) { /* Spansion */
> +			nor->read_opcode = SPINOR_OP_READ_1_4_4_D;
> +		} else {
> +			dev_err(dev, "DDR Quad Read is not supported.\n");
> +			return -EINVAL;
> +		}
> +		break;
>  	case SPI_NOR_QUAD:
>  		nor->read_opcode = SPINOR_OP_READ_1_1_4;
>  		break;
> @@ -1057,6 +1104,9 @@ int spi_nor_scan(struct spi_nor *nor, const struct spi_device_id *id,
>  		if (JEDEC_MFR(info->jedec_id) == CFI_MFR_AMD) {
>  			/* Dedicated 4-byte command set */
>  			switch (nor->flash_read) {
> +			case SPI_NOR_DDR_QUAD:
> +				nor->read_opcode = SPINOR_OP_READ4_1_4_4_D;
> +				break;
>  			case SPI_NOR_QUAD:
>  				nor->read_opcode = SPINOR_OP_READ4_1_1_4;
>  				break;
> diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
> index 48fe9fc..d191a6b 100644
> --- a/include/linux/mtd/spi-nor.h
> +++ b/include/linux/mtd/spi-nor.h
> @@ -12,10 +12,11 @@
>  
>  /*
>   * Note on opcode nomenclature: some opcodes have a format like
> - * SPINOR_OP_FUNCTION{4,}_x_y_z. The numbers x, y, and z stand for the number
> + * SPINOR_OP_FUNCTION{4,}_x_y_z{_D}. The numbers x, y, and z stand for the number
>   * of I/O lines used for the opcode, address, and data (respectively). The
>   * FUNCTION has an optional suffix of '4', to represent an opcode which
> - * requires a 4-byte (32-bit) address.
> + * requires a 4-byte (32-bit) address. The suffix of 'D' stands for the
> + * DDR mode.
>   */
>  
>  /* Flash opcodes. */
> @@ -26,6 +27,7 @@
>  #define SPINOR_OP_READ_FAST	0x0b	/* Read data bytes (high frequency) */
>  #define SPINOR_OP_READ_1_1_2	0x3b	/* Read data bytes (Dual SPI) */
>  #define SPINOR_OP_READ_1_1_4	0x6b	/* Read data bytes (Quad SPI) */
> +#define SPINOR_OP_READ_1_4_4_D	0xed	/* Read data bytes (DDR Quad SPI) */
>  #define SPINOR_OP_PP		0x02	/* Page program (up to 256 bytes) */
>  #define SPINOR_OP_BE_4K		0x20	/* Erase 4KiB block */
>  #define SPINOR_OP_BE_4K_PMC	0xd7	/* Erase 4KiB block on PMC chips */
> @@ -40,6 +42,7 @@
>  #define SPINOR_OP_READ4_FAST	0x0c	/* Read data bytes (high frequency) */
>  #define SPINOR_OP_READ4_1_1_2	0x3c	/* Read data bytes (Dual SPI) */
>  #define SPINOR_OP_READ4_1_1_4	0x6c	/* Read data bytes (Quad SPI) */
> +#define SPINOR_OP_READ4_1_4_4_D	0xee	/* Read data bytes (DDR Quad SPI) */
>  #define SPINOR_OP_PP_4B		0x12	/* Page program (up to 256 bytes) */
>  #define SPINOR_OP_SE_4B		0xdc	/* Sector erase (usually 64KiB) */
>  
> @@ -74,6 +77,7 @@ enum read_mode {
>  	SPI_NOR_FAST,
>  	SPI_NOR_DUAL,
>  	SPI_NOR_QUAD,
> +	SPI_NOR_DDR_QUAD,
>  };
>  
>  /**

So, I'll have to take another hard look at spi-nor.c soon. I think we
may need to work on the abstractions here a bit more before I merge any
new features like this.

Regards,
Brian

P.S. Is there a good reason we've defined a whole read_xfer/write_xfer
API that is completely unused?

[PATCH v2 03/10] mtd: spi-nor: add DDR quad read support

[Huang Shijie]

On Tue, Jul 29, 2014 at 10:08:43PM -0700, Brian Norris wrote:
> 
> On Mon, Apr 28, 2014 at 11:53:40AM +0800, Huang Shijie wrote:
> > This patch adds the DDR quad read support by the following:
> 
> To Mark / linux-spi:
> 
> Are DDR modes in the scope of drivers/spi/ at all, so that we could
> someday wire it up through m25p80.c? Or is 'DDR' such a distortion of
> the meaning of 'SPI' such that it will be restricted only to SPI NOR
> dedicated controllers?

IMHO, the DDR modes can _NOT_ be handled by the driver/spi/*.
The SPI can only handles the byte streams. But the DDR mode may need to
handle the cycles(such as the dummy cycles could be 7 cycles) which is not byte.

So the DDR mode is handled by the SPI NOR controller now.

Please correct me if I am wrong. :)

> 
> >   [1] add SPI_NOR_DDR_QUAD read mode.
> > 
> >   [2] add DDR Quad read opcodes:
> >        SPINOR_OP_READ_1_4_4_D / SPINOR_OP_READ4_1_4_4_D
> > 
> >   [3] add set_ddr_quad_mode() to initialize for the DDR quad read.
> >       Currently it only works for Spansion NOR.
> > 
> >   [3] about the dummy cycles.
> >       We set the dummy with 8 for DDR quad read by default.
> 
> Why? That seems wrong. You need to know for sure how many cycles should
> be used, not just guess a default.

Do you mean that if people do not set the DT node for dummy, we should
return an -EINVAL immediately?


> 
> >       The m25p80.c can not support the DDR quad read, but the SPI NOR controller
> >       can set the dummy value in its child DT node, and the SPI NOR framework
> >       can parse it out.
> 
> Why does the dummy value belong in device tree? I think this can be
> handled in software. You might, however, want a few other hardware
> description parameters in device tree to help you.
> 
> So I think spi-nor.c needs to know a few things:
> 
>  1. Does the hardware/driver support DDR clocking?
>  2. What granularity of dummy cycles are supported? So m25p80.c needs to
>     communicate that it only supports dummy cycles of multiples of 8,
>     and fsl-quadspi supports single clock cycles.

I think you can send patches for these features. I does not clear about:
  for what does the spi-nor needs to know the above things.

> And spi-nor.c should be able to do the following:
> 
>  3. Set how many dummy cycles should be used.
where can we get the number of the cycles? 


>  4. Write to the configuration register, to choose a Latency Code
>     according to what the flash supports. I see modes that support 3, 6,
>     7, or 8. We'd probably just go for the fastest mode, which requires
>     8, right?
not right.

The DDR mode can not work if we set a wrong dummy cycles for the flash.

for some chips, the fastest mode may need 6 cycles, not 8. 

> 
> So far, none of this seems to require a DT binding, unless there's
> something I'm missing about your fsl-quadspi controller.
> 
> > Test this patch for Spansion s25fl128s NOR flash.
> > 
> > Signed-off-by: Huang Shijie <b32955@freescale.com>
> > ---
> > +	/* DDR Quad/Quad/Dual-read mode takes precedence over fast/normal */
> > +	if (mode == SPI_NOR_DDR_QUAD && info->flags & SPI_NOR_DDR_QUAD_READ) {
> 
> Hmm, I think I should probably take another look at the design of
> spi-nor.c... Why does spi_nor_scan() take a single 'mode' argument? The
> driver should be communicating which (multiple) modes it supports, not
> selecting a single mode. spi-nor.c is the only one which knows what the
> *flash* supports, so it should be combining knowledge from the
> controller driver with its own knowledge of the flash.

It is okay for me to add multiples modes for the spi_nor_scan().
I added the single mode for spi_nor_scan is because that the fsl-quadspi
does not want to support the low speed modes. (Of course, the fsl-quadspi
controller does support the low speed modes.)

> 
> > +		ret = set_ddr_quad_mode(nor, info->jedec_id);
> > +		if (ret) {
> > +			dev_err(dev, "DDR quad mode not supported\n");
> > +			return ret;
> 
> A ramification of my comment above is that we should not be returning an
> error in a situation like this; we should be able to fall back to
> another known-supported mode, like SDR QUAD, SDR DUAL, or just plain
> SPI -- if they're supported by the driver -- and spi-nor.c doesn't
> currently have enough information to do this.
ok.
> 
> > +		}
> > +		nor->flash_read = SPI_NOR_DDR_QUAD;
> >  
> >  /**
> 
> So, I'll have to take another hard look at spi-nor.c soon. I think we
> may need to work on the abstractions here a bit more before I merge any
> new features like this.

okay.  no problem.

> 
> Regards,
> Brian
> 
> P.S. Is there a good reason we've defined a whole read_xfer/write_xfer
> API that is completely unused?
These hooks are designed for other SPI NOR drivers, the fsl-quadspi does
not use them.

thanks
Huang Shijie

[PATCH v2 03/10] mtd: spi-nor: add DDR quad read support

[Brian Norris]

+ Mark

On Wed, Jul 30, 2014 at 02:44:13PM +0800, Huang Shijie wrote:
> On Tue, Jul 29, 2014 at 10:08:43PM -0700, Brian Norris wrote:
> > On Mon, Apr 28, 2014 at 11:53:40AM +0800, Huang Shijie wrote:
> > > This patch adds the DDR quad read support by the following:
> > 
> > To Mark / linux-spi:
> > 
> > Are DDR modes in the scope of drivers/spi/ at all, so that we could
> > someday wire it up through m25p80.c? Or is 'DDR' such a distortion of
> > the meaning of 'SPI' such that it will be restricted only to SPI NOR
> > dedicated controllers?
> 
> IMHO, the DDR modes can _NOT_ be handled by the driver/spi/*.

I agree to some extent, but I wanted to confirm with the SPI guys that
DDR is truly unique to SPI NOR. (I know it doesn't currently support
it.)

> The SPI can only handles the byte streams.

Sure.

> But the DDR mode may need to
> handle the cycles(such as the dummy cycles could be 7 cycles) which is not byte.

But that's the same story for some (but not all) of the dual and quad
modes now; some have dummy cycles that are not multiples of 8 bits.

Because there are some DDR modes which have 8 dummy cycles, it is
theoretically possible for the SPI subsystem to handle them.

> So the DDR mode is handled by the SPI NOR controller now.

Right.

BTW, does your quadspi controller unconditionally support DDR, or is
there any dependency on board/clock configuration? I'm just curious
whether you need a DT binding to describe DDR support, or if (as long as
the flash supports it, and we get the dummy cycles right) you can always
use DDR QUAD.

> Please correct me if I am wrong. :)
> 
> > >   [1] add SPI_NOR_DDR_QUAD read mode.
> > > 
> > >   [2] add DDR Quad read opcodes:
> > >        SPINOR_OP_READ_1_4_4_D / SPINOR_OP_READ4_1_4_4_D
> > > 
> > >   [3] add set_ddr_quad_mode() to initialize for the DDR quad read.
> > >       Currently it only works for Spansion NOR.
> > > 
> > >   [3] about the dummy cycles.
> > >       We set the dummy with 8 for DDR quad read by default.
> > 
> > Why? That seems wrong. You need to know for sure how many cycles should
> > be used, not just guess a default.
> 
> Do you mean that if people do not set the DT node for dummy, we should
> return an -EINVAL immediately?

Possibly. But I actually don't think this belongs in DT at all. See
below.

> > >       The m25p80.c can not support the DDR quad read, but the SPI NOR controller
> > >       can set the dummy value in its child DT node, and the SPI NOR framework
> > >       can parse it out.
> > 
> > Why does the dummy value belong in device tree? I think this can be
> > handled in software.

Can you answer me this question?

> > You might, however, want a few other hardware
> > description parameters in device tree to help you.
> > 
> > So I think spi-nor.c needs to know a few things:
> > 
> >  1. Does the hardware/driver support DDR clocking?
> >  2. What granularity of dummy cycles are supported? So m25p80.c needs to
> >     communicate that it only supports dummy cycles of multiples of 8,
> >     and fsl-quadspi supports single clock cycles.
> 
> I think you can send patches for these features. I does not clear about:
>   for what does the spi-nor needs to know the above things.

To properly abstract features between a driver and the spi-nor
"library." For example, we need to make sure we don't try to use a
command mode with 7 dummy cycles on m25p80.c; right now, each driver has
to (implicitly) know the details of dummy cycles when selecting a 'mode'
parameter for spi_nor_scan(). spi-nor.c should be selecting this for us,
not forcing the driver to make the selection.

> > And spi-nor.c should be able to do the following:
> > 
> >  3. Set how many dummy cycles should be used.
> where can we get the number of the cycles? 

This should be a property of the flash device, just like everything else
we detect in spi-nor.c.

> >  4. Write to the configuration register, to choose a Latency Code
> >     according to what the flash supports. I see modes that support 3, 6,
> >     7, or 8. We'd probably just go for the fastest mode, which requires
> >     8, right?
> not right.
> 
> The DDR mode can not work if we set a wrong dummy cycles for the flash.
> 
> for some chips, the fastest mode may need 6 cycles, not 8. 

OK, but can spi-nor.c detect this instead of putting this in DT? e.g.,
associate this with the device ID?

Or even better, we can support CFI detection for SPI NOR flash. I notice
the datasheet for your Spansion flash [1] has an extensive set of CFI
parameters defined, including the dummy cycle information. I think it
might be more sustainable to try to support CFI [2] and SFDP [3] for
detecting new flash, rather than adding new table entries ad-hoc.

> > So far, none of this seems to require a DT binding, unless there's
> > something I'm missing about your fsl-quadspi controller.
> > 
> > > Test this patch for Spansion s25fl128s NOR flash.
> > > 
> > > Signed-off-by: Huang Shijie <b32955@freescale.com>
> > > ---
> > > +	/* DDR Quad/Quad/Dual-read mode takes precedence over fast/normal */
> > > +	if (mode == SPI_NOR_DDR_QUAD && info->flags & SPI_NOR_DDR_QUAD_READ) {
> > 
> > Hmm, I think I should probably take another look at the design of
> > spi-nor.c... Why does spi_nor_scan() take a single 'mode' argument? The
> > driver should be communicating which (multiple) modes it supports, not
> > selecting a single mode. spi-nor.c is the only one which knows what the
> > *flash* supports, so it should be combining knowledge from the
> > controller driver with its own knowledge of the flash.
> 
> It is okay for me to add multiples modes for the spi_nor_scan().
> I added the single mode for spi_nor_scan is because that the fsl-quadspi
> does not want to support the low speed modes. (Of course, the fsl-quadspi
> controller does support the low speed modes.)

Right, fsl-quadspi only supports one mode. But m25p80.c is more
flexible, and I think we might have broken some of it in the process
(e.g., if the SPI controller supports single/dual/quad but the flash
only supports single, then we might fail to probe).

I can take a look at this problem if you don't. I'd just recommend that
we might take a step back on a few of these issues before blazing ahead
with something irrevocable, like a DT binding.

> > > +		ret = set_ddr_quad_mode(nor, info->jedec_id);
> > > +		if (ret) {
> > > +			dev_err(dev, "DDR quad mode not supported\n");
> > > +			return ret;
> > 
> > A ramification of my comment above is that we should not be returning an
> > error in a situation like this; we should be able to fall back to
> > another known-supported mode, like SDR QUAD, SDR DUAL, or just plain
> > SPI -- if they're supported by the driver -- and spi-nor.c doesn't
> > currently have enough information to do this.
> ok.
> > 
> > > +		}
> > > +		nor->flash_read = SPI_NOR_DDR_QUAD;
> > >  
> > >  /**
> > 
> > So, I'll have to take another hard look at spi-nor.c soon. I think we
> > may need to work on the abstractions here a bit more before I merge any
> > new features like this.
> 
> okay.  no problem.
> 
> > 
> > Regards,
> > Brian
> > 
> > P.S. Is there a good reason we've defined a whole read_xfer/write_xfer
> > API that is completely unused?
> These hooks are designed for other SPI NOR drivers, the fsl-quadspi does
> not use them.

Brian

[1] http://www.spansion.com/Support/Datasheets/S25FL128S_256S_00.pdf

[2] http://www.jedec.org/sites/default/files/docs/jesd68-01.pdf

    plus some of the vendor extensions which are documented in their
    datasheets

[3] http://www.macronix.com/Lists/ApplicationNote/Attachments/667/AN114v1-SFDP%20Introduction.pdf

    http://www.jedec.org/sites/default/files/docs/JESD216.pdf
    (login wall)

[PATCH v2 03/10] mtd: spi-nor: add DDR quad read support

[Mark Brown]

On Wed, Jul 30, 2014 at 12:45:00AM -0700, Brian Norris wrote:
> On Wed, Jul 30, 2014 at 02:44:13PM +0800, Huang Shijie wrote:
> > On Tue, Jul 29, 2014 at 10:08:43PM -0700, Brian Norris wrote:
> > > On Mon, Apr 28, 2014 at 11:53:40AM +0800, Huang Shijie wrote:
> > > > This patch adds the DDR quad read support by the following:

> > > Are DDR modes in the scope of drivers/spi/ at all, so that we could
> > > someday wire it up through m25p80.c? Or is 'DDR' such a distortion of
> > > the meaning of 'SPI' such that it will be restricted only to SPI NOR
> > > dedicated controllers?

> > IMHO, the DDR modes can _NOT_ be handled by the driver/spi/*.

> I agree to some extent, but I wanted to confirm with the SPI guys that
> DDR is truly unique to SPI NOR. (I know it doesn't currently support
> it.)

I don't know what DDR is in this context, sorry.  I'm guessing you're
right since it sounds like something to do with extra clocks and this is
probably not something used by generic SPI devices at present (if it
ends up being widely implemented by sufficiently generic controllers
that might change but the trend seems to be to flash specific
controllers).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140730/fc997013/attachment-0001.sig>

[PATCH v2 03/10] mtd: spi-nor: add DDR quad read support

[Huang Shijie]

On Wed, Jul 30, 2014 at 12:45:00AM -0700, Brian Norris wrote:
> 
> BTW, does your quadspi controller unconditionally support DDR, or is
> there any dependency on board/clock configuration? I'm just curious
> whether you need a DT binding to describe DDR support, or if (as long as
> the flash supports it, and we get the dummy cycles right) you can always
> use DDR QUAD.
The fsl-quadspi controller can support the DDR quad mode unconditionally.
In the other word, this controller is _designed_ for the DDR quad mode.

the driver does not needs a DT binding for the DDR. thanks.


> 
> > Please correct me if I am wrong. :)
> > 
> > > >   [1] add SPI_NOR_DDR_QUAD read mode.
> > > > 
> > > >   [2] add DDR Quad read opcodes:
> > > >        SPINOR_OP_READ_1_4_4_D / SPINOR_OP_READ4_1_4_4_D
> > > > 
> > > >   [3] add set_ddr_quad_mode() to initialize for the DDR quad read.
> > > >       Currently it only works for Spansion NOR.
> > > > 
> > > >   [3] about the dummy cycles.
> > > >       We set the dummy with 8 for DDR quad read by default.
> > > 
> > > Why? That seems wrong. You need to know for sure how many cycles should
> > > be used, not just guess a default.
> > 
> > Do you mean that if people do not set the DT node for dummy, we should
> > return an -EINVAL immediately?
> 
> Possibly. But I actually don't think this belongs in DT at all. See
> below.
> 
> > > >       The m25p80.c can not support the DDR quad read, but the SPI NOR controller
> > > >       can set the dummy value in its child DT node, and the SPI NOR framework
> > > >       can parse it out.
> > > 
> > > Why does the dummy value belong in device tree? I think this can be
> > > handled in software.

We can not handle it in the software way.
Why? since there are too many modes,and each mode needs different dummy
cycles. Even the same chips, different versions may uses different dummy
cycles in the same mode. ..

You can refer to the Spansion's  S25fs128s.

If you want to put these dummy cycles in the device ID, it will make you
crazy in the end. :)
> 
> Can you answer me this question?
> 
> > > You might, however, want a few other hardware
> > > description parameters in device tree to help you.
> > > 
> > > So I think spi-nor.c needs to know a few things:
> > > 
> > >  1. Does the hardware/driver support DDR clocking?
> > >  2. What granularity of dummy cycles are supported? So m25p80.c needs to
> > >     communicate that it only supports dummy cycles of multiples of 8,
> > >     and fsl-quadspi supports single clock cycles.
> > 
> > I think you can send patches for these features. I does not clear about:
> >   for what does the spi-nor needs to know the above things.
> 
> To properly abstract features between a driver and the spi-nor
> "library." For example, we need to make sure we don't try to use a
> command mode with 7 dummy cycles on m25p80.c; right now, each driver has
> to (implicitly) know the details of dummy cycles when selecting a 'mode'
> parameter for spi_nor_scan(). spi-nor.c should be selecting this for us,
> not forcing the driver to make the selection.
> 
> > > And spi-nor.c should be able to do the following:
> > > 
> > >  3. Set how many dummy cycles should be used.
> > where can we get the number of the cycles? 
> 
> This should be a property of the flash device, just like everything else
> we detect in spi-nor.c.
This is indeed a probably of the flash device.
and this is why I add a new DT binding file for the flash.

> 
> > >  4. Write to the configuration register, to choose a Latency Code
> > >     according to what the flash supports. I see modes that support 3, 6,
> > >     7, or 8. We'd probably just go for the fastest mode, which requires
> > >     8, right?
> > not right.
> > 
> > The DDR mode can not work if we set a wrong dummy cycles for the flash.
> > 
> > for some chips, the fastest mode may need 6 cycles, not 8. 
> 
> OK, but can spi-nor.c detect this instead of putting this in DT? e.g.,
> associate this with the device ID?

see the my comment above.
> 
> Or even better, we can support CFI detection for SPI NOR flash. I notice
> the datasheet for your Spansion flash [1] has an extensive set of CFI
> parameters defined, including the dummy cycle information. I think it
> might be more sustainable to try to support CFI [2] and SFDP [3] for
> detecting new flash, rather than adding new table entries ad-hoc.
I think different chip vendors may have different format to store the
info, just like the read-retry for nand chips.

do you want to add the code only available for Spansion?
What's more the code will be very long, i think.

Add a new DT binding file for the flash maybe is the simplest way.


> 
> > > So far, none of this seems to require a DT binding, unless there's
> > > something I'm missing about your fsl-quadspi controller.
> > > 
> > > > Test this patch for Spansion s25fl128s NOR flash.
> > > > 
> > > > Signed-off-by: Huang Shijie <b32955@freescale.com>
> > > > ---
> > > > +	/* DDR Quad/Quad/Dual-read mode takes precedence over fast/normal */
> > > > +	if (mode == SPI_NOR_DDR_QUAD && info->flags & SPI_NOR_DDR_QUAD_READ) {
> > > 
> > > Hmm, I think I should probably take another look at the design of
> > > spi-nor.c... Why does spi_nor_scan() take a single 'mode' argument? The
> > > driver should be communicating which (multiple) modes it supports, not
> > > selecting a single mode. spi-nor.c is the only one which knows what the
> > > *flash* supports, so it should be combining knowledge from the
> > > controller driver with its own knowledge of the flash.
> > 
> > It is okay for me to add multiples modes for the spi_nor_scan().
> > I added the single mode for spi_nor_scan is because that the fsl-quadspi
> > does not want to support the low speed modes. (Of course, the fsl-quadspi
> > controller does support the low speed modes.)
> 
> Right, fsl-quadspi only supports one mode. But m25p80.c is more
> flexible, and I think we might have broken some of it in the process
> (e.g., if the SPI controller supports single/dual/quad but the flash
> only supports single, then we might fail to probe).
> 
> I can take a look at this problem if you don't. I'd just recommend that
> we might take a step back on a few of these issues before blazing ahead
> with something irrevocable, like a DT binding.

I am glad you can spend some time on this issue. 


Could you please also read this patch ?:

http://lists.infradead.org/pipermail/linux-mtd/2014-May/054108.html

thanks
Huang Shijie

[PATCH v2 03/10] mtd: spi-nor: add DDR quad read support

[Brian Norris]

On Wed, Jul 30, 2014 at 11:46:07AM +0100, Mark Brown wrote:
> On Wed, Jul 30, 2014 at 12:45:00AM -0700, Brian Norris wrote:
> > On Wed, Jul 30, 2014 at 02:44:13PM +0800, Huang Shijie wrote:
> > > IMHO, the DDR modes can _NOT_ be handled by the driver/spi/*.
> 
> > I agree to some extent, but I wanted to confirm with the SPI guys that
> > DDR is truly unique to SPI NOR. (I know it doesn't currently support
> > it.)
> 
> I don't know what DDR is in this context, sorry.

I think it's just the ability to latch data on both the rising and
falling edges of the SPI clock. For SPI flash, it seems to be used for
the data portion of the opcode/address/data sequence.

> I'm guessing you're
> right since it sounds like something to do with extra clocks and this is
> probably not something used by generic SPI devices at present (if it
> ends up being widely implemented by sufficiently generic controllers
> that might change but the trend seems to be to flash specific
> controllers).

OK, thanks for chiming in.

Yeah, I suppose it could be wedged in later if drivers/spi/ ever adopts
a solution.

Brian

[PATCH v2 03/10] mtd: spi-nor: add DDR quad read support

[Geert Uytterhoeven]

On Sat, Aug 2, 2014 at 4:06 AM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Wed, Jul 30, 2014 at 11:46:07AM +0100, Mark Brown wrote:
>> On Wed, Jul 30, 2014 at 12:45:00AM -0700, Brian Norris wrote:
>> > On Wed, Jul 30, 2014 at 02:44:13PM +0800, Huang Shijie wrote:
>> > > IMHO, the DDR modes can _NOT_ be handled by the driver/spi/*.
>>
>> > I agree to some extent, but I wanted to confirm with the SPI guys that
>> > DDR is truly unique to SPI NOR. (I know it doesn't currently support
>> > it.)
>>
>> I don't know what DDR is in this context, sorry.
>
> I think it's just the ability to latch data on both the rising and
> falling edges of the SPI clock. For SPI flash, it seems to be used for
> the data portion of the opcode/address/data sequence.
>
>> I'm guessing you're
>> right since it sounds like something to do with extra clocks and this is
>> probably not something used by generic SPI devices at present (if it
>> ends up being widely implemented by sufficiently generic controllers
>> that might change but the trend seems to be to flash specific
>> controllers).
>
> OK, thanks for chiming in.
>
> Yeah, I suppose it could be wedged in later if drivers/spi/ ever adopts
> a solution.

I think this can just be another SPI_* spi_device.mode flag.

Do we need bindings for this in
Documentation/devicetree/bindings/spi/spi-bus.txt?
Unlike Quad SPI transfer support, this doesn't need special wiring, so DDR
capability is an intrinsic property of the SPI slave, and the mode bit can just
be set in the SPI slave driver, without any DT magic?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v2 03/10] mtd: spi-nor: add DDR quad read support

[Mark Brown]

On Sat, Aug 02, 2014 at 11:09:09AM +0200, Geert Uytterhoeven wrote:
> On Sat, Aug 2, 2014 at 4:06 AM, Brian Norris
> > On Wed, Jul 30, 2014 at 11:46:07AM +0100, Mark Brown wrote:

> >> I don't know what DDR is in this context, sorry.

> > I think it's just the ability to latch data on both the rising and
> > falling edges of the SPI clock. For SPI flash, it seems to be used for
> > the data portion of the opcode/address/data sequence.

> > Yeah, I suppose it could be wedged in later if drivers/spi/ ever adopts
> > a solution.

> I think this can just be another SPI_* spi_device.mode flag.

Sounds like it yes - I was wondering if this might be one of the modes
with extra clock cycles that I've heard mentioned before which might be
a little more fun.

> Do we need bindings for this in
> Documentation/devicetree/bindings/spi/spi-bus.txt?
> Unlike Quad SPI transfer support, this doesn't need special wiring, so DDR
> capability is an intrinsic property of the SPI slave, and the mode bit can just
> be set in the SPI slave driver, without any DT magic?

Right, unless we run into things like board design issues causing
constraints this is something that can be enabled by the two drivers
without needing DT configuration.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140804/391bf0b7/attachment.sig>

[PATCH v2 01/10] mtd: spi-nor: fix the wrong dummy value

[Brian Norris]

On Mon, Apr 28, 2014 at 11:53:38AM +0800, Huang Shijie wrote:
> For the DDR Quad read, the dummy cycles maybe 3 or 6 which is less then 8.
> The dummy cycles is actually 8 for SPI fast/dual/quad read.
> 
> This patch makes preparations for the DDR quad read, it fixes the wrong dummy
> value for both the spi-nor.c and m25p80.c.
> 
> Signed-off-by: Huang Shijie <b32955@freescale.com>
> ---
> This ia actually v3.

Don't know why this got left behind. Applied to l2-mtd.git.

Brian

[PATCH v2 03/10] mtd: spi-nor: add DDR quad read support

[Zhi Li]

On Mon, Aug 4, 2014 at 9:25 AM, Mark Brown <broonie@kernel.org> wrote:
> On Sat, Aug 02, 2014 at 11:09:09AM +0200, Geert Uytterhoeven wrote:
>> On Sat, Aug 2, 2014 at 4:06 AM, Brian Norris
>> > On Wed, Jul 30, 2014 at 11:46:07AM +0100, Mark Brown wrote:
>
>> >> I don't know what DDR is in this context, sorry.
>
>> > I think it's just the ability to latch data on both the rising and
>> > falling edges of the SPI clock. For SPI flash, it seems to be used for
>> > the data portion of the opcode/address/data sequence.
>
>> > Yeah, I suppose it could be wedged in later if drivers/spi/ ever adopts
>> > a solution.
>
>> I think this can just be another SPI_* spi_device.mode flag.
>
> Sounds like it yes - I was wondering if this might be one of the modes
> with extra clock cycles that I've heard mentioned before which might be
> a little more fun.
>
>> Do we need bindings for this in
>> Documentation/devicetree/bindings/spi/spi-bus.txt?
>> Unlike Quad SPI transfer support, this doesn't need special wiring, so DDR
>> capability is an intrinsic property of the SPI slave, and the mode bit can just
>> be set in the SPI slave driver, without any DT magic?
>
> Right, unless we run into things like board design issues causing
> constraints this is something that can be enabled by the two drivers
> without needing DT configuration.

All:

we plan resume this work.
I need direction how go ahead further.

I go through this email thread.

The discussion focus on how to get dummy cycle information.

Shijie get it from DT.

Brain suggest get from CFI or map id table if I understand correct.

Accodring to spec
http://www.spansion.com/Support/Datasheets/S25FL128S_256S_00.pdf

Table 8.10

Dummy cycle depend on frequency, read command.

if information saved in driver, it will be huge table.

>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

[PATCH v2 03/10] mtd: spi-nor: add DDR quad read support

[Zhi Li]

On Wed, Jul 22, 2015 at 1:15 PM, Zhi Li <lznuaa@gmail.com> wrote:
> On Mon, Aug 4, 2014 at 9:25 AM, Mark Brown <broonie@kernel.org> wrote:
>> On Sat, Aug 02, 2014 at 11:09:09AM +0200, Geert Uytterhoeven wrote:
>>> On Sat, Aug 2, 2014 at 4:06 AM, Brian Norris
>>> > On Wed, Jul 30, 2014 at 11:46:07AM +0100, Mark Brown wrote:
>>
>>> >> I don't know what DDR is in this context, sorry.
>>
>>> > I think it's just the ability to latch data on both the rising and
>>> > falling edges of the SPI clock. For SPI flash, it seems to be used for
>>> > the data portion of the opcode/address/data sequence.
>>
>>> > Yeah, I suppose it could be wedged in later if drivers/spi/ ever adopts
>>> > a solution.
>>
>>> I think this can just be another SPI_* spi_device.mode flag.
>>
>> Sounds like it yes - I was wondering if this might be one of the modes
>> with extra clock cycles that I've heard mentioned before which might be
>> a little more fun.
>>
>>> Do we need bindings for this in
>>> Documentation/devicetree/bindings/spi/spi-bus.txt?
>>> Unlike Quad SPI transfer support, this doesn't need special wiring, so DDR
>>> capability is an intrinsic property of the SPI slave, and the mode bit can just
>>> be set in the SPI slave driver, without any DT magic?
>>
>> Right, unless we run into things like board design issues causing
>> constraints this is something that can be enabled by the two drivers
>> without needing DT configuration.
>
> All:

Just update shijie's email address.

>
> we plan resume this work.
> I need direction how go ahead further.
>
> I go through this email thread.
>
> The discussion focus on how to get dummy cycle information.
>
> Shijie get it from DT.
>
> Brain suggest get from CFI or map id table if I understand correct.
>
> Accodring to spec
> http://www.spansion.com/Support/Datasheets/S25FL128S_256S_00.pdf
>
> Table 8.10
>
> Dummy cycle depend on frequency, read command.
>
> if information saved in driver, it will be huge table.
>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>