Add the Quadspi driver for vf610-twr

The patch set is based on Li Xiaochun's init version.
http://marc.info/?l=linux-arm-kernel&m=137181252311126&w=2

(0) What is the Quadspi controller?

The Quadspi(Quad Serial Peripheral Interface) acts as an interface to one single or two external serial flash devices, each with up to 4 bidirectional data lines.

(1) The Quadspi controller is driven by the LUT(Look-up Table) registers. The LUT registers are a look-up-table for sequences of instructions. A valid sequence consists of four LUT registers.

(2) The definition of the LUT register shows below:

---------------------------------------------------
| INSTR1 | PAD1 | OPRND1 | INSTR0 | PAD0 | OPRND0 |
---------------------------------------------------

There are several types of INSTRx, such as:
CMD : the SPI NOR command.
ADDR : the address for the SPI NOR command.
DUMMY : the dummy cycles needed by the SPI NOR command.
....

(3) We connect the NOR the QuadSPI now. I am not sure, but i think the QuadSPI will be only used for the NOR. We may connect other devices to it. But, for the reason of (2), we have to parse out the SPI NOR command for the QuadSPI.

(4) Test this driver with the JFFS2 and UBIFS with the Spansion s25fl128s :

For jffs2:
#flash_eraseall /dev/mtd0
#mount -t jffs2 /dev/mtdblock0 tmp
#bonnie++ -d tmp -u 0 -s 10 -r 5

For ubifs:
#flash_eraseall /dev/mtd0
#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

(5) The test result of the DDR QUAD Read (66MHz) performance:

        #insmod mtd_speedtest.ko dev=0

	[  194.831313] =================================================
	[  194.825453] mtd_speedtest: MTD device: 0
	[  194.818670] mtd_speedtest: not NAND flash, assume page size is 512 bytes.
	[  194.811705] mtd_speedtest: MTD device size 16777216, eraseblock size 65536,
               page size 512, count of eraseblocks 256, pages per eraseblock 128, OOB size 0
	[  228.482355] mtd_speedtest: testing eraseblock write speed
	[  213.024166] mtd_speedtest: eraseblock write speed is 203 KiB/s
	[  213.018306] mtd_speedtest: testing eraseblock read speed
	[  212.660856] mtd_speedtest: eraseblock read speed is 46545 KiB/s
	[  181.728267] mtd_speedtest: testing page write speed
	[  231.434842] mtd_speedtest: page write speed is 203 KiB/s
	[  231.429515] mtd_speedtest: testing page read speed
	[  228.957422] mtd_speedtest: page read speed is 6641 KiB/s
	[  197.778872] mtd_speedtest: testing 2 page write speed
	[  247.338069] mtd_speedtest: 2 page write speed is 203 KiB/s
	[  247.332514] mtd_speedtest: testing 2 page read speed
	[  245.925048] mtd_speedtest: 2 page read speed is 11686 KiB/s
	[  245.919460] mtd_speedtest: Testing erase speed
	[  214.612341] mtd_speedtest: erase speed is 523 KiB/s
	[  214.607410] mtd_speedtest: Testing 2x multi-block erase speed
	[  245.545971] mtd_speedtest: 2x multi-block erase speed is 480 KiB/s
	[  245.539744] mtd_speedtest: Testing 4x multi-block erase speed
	[  211.141696] mtd_speedtest: 4x multi-block erase speed is 476 KiB/s
	[  211.135496] mtd_speedtest: Testing 8x multi-block erase speed
	[  241.761502] mtd_speedtest: 8x multi-block erase speed is 475 KiB/s
	[  241.755269] mtd_speedtest: Testing 16x multi-block erase speed
	[  272.307979] mtd_speedtest: 16x multi-block erase speed is 474 KiB/s
	[  272.301660] mtd_speedtest: Testing 32x multi-block erase speed
	[  237.637902] mtd_speedtest: 32x multi-block erase speed is 472 KiB/s
	[  237.631581] mtd_speedtest: Testing 64x multi-block erase speed
	[  267.954341] mtd_speedtest: 64x multi-block erase speed is 471 KiB/s
	[  267.948005] mtd_speedtest: finished
	[  267.944478] =================================================

     * Conclusion *:
     --------------------------------------------------------------------
       We can get the 46.5 MiB/s read speed when the DDR Quad Read is enabled.
	 (From S25FL128S's spec, the maximum read rate of DDR Quad Read is
	 66MiB/s)
     --------------------------------------------------------------------

Changelog:
v2 --> v3:
[1] use the "num-cs" property, and remove the "fsl,spi-num-chipselects".
[2] remove the redundant log.

v1 --> v2:
[1] add DDR QUAD read support.
[2] add the support for two NOR chips
[3] misc.

init --> v1:
[1] since the ic bug in the IPS read, abandon the IPS read, use the AHB read.
[2] change the mtd code, add the quad read support.
[3] add the quad read support the QuadSpi driver.
[4] misc.

Huang Shijie (8):
mtd: m25p80: move the spi-nor commands to a header
mtd: m25p80: add support for Spansion s25fl128s chip
mtd: m25p80: add the quad-read support
mtd: m25p80: add the DDR quad-read support
spi: Add Freescale QuadSpi driver
Documentation: add the binding file for Quadspi driver
ARM: dts: vf610: change the PAD values for Quadspi
ARM: dts: vf610-twr: Add SPI NOR support

 Documentation/devicetree/bindings/mtd/m25p80.txt   |   10 +
 .../devicetree/bindings/spi/fsl-quadspi.txt        |   32 +
 arch/arm/boot/dts/vf610-twr.dts                    |   36 +
 arch/arm/boot/dts/vf610.dtsi                       |   24 +-
 drivers/mtd/devices/m25p80.c                       |  116 ++-
 drivers/spi/Kconfig                                |    7 +
 drivers/spi/Makefile                               |    1 +
 drivers/spi/spi-fsl-quadspi.c                      | 1031 ++++++++++++++++++++
 include/linux/mtd/spi-nor.h                        |   61 ++
 9 files changed, 1265 insertions(+), 53 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/spi/fsl-quadspi.txt
 create mode 100644 drivers/spi/spi-fsl-quadspi.c
 create mode 100644 include/linux/mtd/spi-nor.h

Why should move the spi-nor commands to a header?

The reason is caused by the Freescale's Quadspi controller.

(0) What is the Quadspi controller?

The Quadspi(Quad Serial Peripheral Interface) acts as an interface to one single or two external serial flash devices, each with up to 4 bidirectional data lines.

(1) The Quadspi controller is driven by the LUT(Look-up Table) registers. The LUT registers are a look-up-table for sequences of instructions. A valid sequence consists of four LUT registers.

(2) The definition of the LUT register shows below:

---------------------------------------------------
| INSTR1 | PAD1 | OPRND1 | INSTR0 | PAD0 | OPRND0 |
---------------------------------------------------

There are several types of INSTRx, such as:
CMD : the SPI NOR command.
ADDR : the address for the SPI NOR command.
DUMMY : the dummy cycles needed by the SPI NOR command.
....

(3) We should pre-populate the LUT table before the Quadspi controller starts to work. Take the SPI Write Enable command for example, we should set the LUT table like this:

INSTR0 = CMD; PAD0 = 0; OPRND0 = 0x06 (SPI Write Enable command)

(4) Conclusion:
We need to move the SPI NOR commands to a separate header which can be used the m25p80.c and the drivers, such Quadspi controller. (It seems the spear-smi.c driver also needs this header.)

---
 drivers/mtd/devices/m25p80.c |   42 +--------------------------------
 include/linux/mtd/spi-nor.h  |   53 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+), 41 deletions(-)
 create mode 100644 include/linux/mtd/spi-nor.h

index 26b14f9..299cc69 100644
@@ -30,52 +30,12 @@
 #include <linux/mtd/cfi.h>
 #include <linux/mtd/mtd.h>
 #include <linux/mtd/partitions.h>
+#include <linux/mtd/spi-nor.h>
 #include <linux/of_platform.h>
 
 #include <linux/spi/spi.h>
 #include <linux/spi/flash.h>
 
-/* Flash opcodes. */
-#define	OPCODE_WREN		0x06	/* Write enable */
-#define	OPCODE_RDSR		0x05	/* Read status register */
-#define	OPCODE_WRSR		0x01	/* Write status register 1 byte */
-#define	OPCODE_NORM_READ	0x03	/* Read data bytes (low frequency) */
-#define	OPCODE_FAST_READ	0x0b	/* Read data bytes (high frequency) */
-#define	OPCODE_PP		0x02	/* Page program (up to 256 bytes) */
-#define	OPCODE_BE_4K		0x20	/* Erase 4KiB block */
-#define	OPCODE_BE_4K_PMC	0xd7	/* Erase 4KiB block on PMC chips */
-#define	OPCODE_BE_32K		0x52	/* Erase 32KiB block */
-#define	OPCODE_CHIP_ERASE	0xc7	/* Erase whole flash chip */
-#define	OPCODE_SE		0xd8	/* Sector erase (usually 64KiB) */
-#define	OPCODE_RDID		0x9f	/* Read JEDEC ID */
-
-/* 4-byte address opcodes - used on Spansion and some Macronix flashes. */
-#define	OPCODE_NORM_READ_4B	0x13	/* Read data bytes (low frequency) */
-#define	OPCODE_FAST_READ_4B	0x0c	/* Read data bytes (high frequency) */
-#define	OPCODE_PP_4B		0x12	/* Page program (up to 256 bytes) */
-#define	OPCODE_SE_4B		0xdc	/* Sector erase (usually 64KiB) */
-
-/* Used for SST flashes only. */
-#define	OPCODE_BP		0x02	/* Byte program */
-#define	OPCODE_WRDI		0x04	/* Write disable */
-#define	OPCODE_AAI_WP		0xad	/* Auto address increment word program */
-
-/* Used for Macronix and Winbond flashes. */
-#define	OPCODE_EN4B		0xb7	/* Enter 4-byte mode */
-#define	OPCODE_EX4B		0xe9	/* Exit 4-byte mode */
-
-/* Used for Spansion flashes only. */
-#define	OPCODE_BRWR		0x17	/* Bank register write */
-
-/* Status Register bits. */
-#define	SR_WIP			1	/* Write in progress */
-#define	SR_WEL			2	/* Write enable latch */
-/* meaning of other SR_* bits may differ between vendors */
-#define	SR_BP0			4	/* Block protect 0 */
-#define	SR_BP1			8	/* Block protect 1 */
-#define	SR_BP2			0x10	/* Block protect 2 */
-#define	SR_SRWD			0x80	/* SR write protect */
-
 /* Define max times to check status register before we give up. */
 #define	MAX_READY_WAIT_JIFFIES	(40 * HZ)	/* M25P16 specs 40s max chip erase */
 #define	MAX_CMD_SIZE		5

1.7.1

---
 drivers/mtd/devices/m25p80.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

index 299cc69..cae419e 100644
@@ -751,6 +751,7 @@ static const struct spi_device_id m25p_ids[] = {
 	{ "s70fl01gs",  INFO(0x010221, 0x4d00, 256 * 1024, 256, 0) },
 	{ "s25sl12800", INFO(0x012018, 0x0300, 256 * 1024,  64, 0) },
 	{ "s25sl12801", INFO(0x012018, 0x0301,  64 * 1024, 256, 0) },
+	{ "s25fl128s", INFO(0x012018, 0x4d01,  64 * 1024, 256, 0) },
 	{ "s25fl129p0", INFO(0x012018, 0x4d00, 256 * 1024,  64, 0) },
 	{ "s25fl129p1", INFO(0x012018, 0x4d01,  64 * 1024, 256, 0) },
 	{ "s25sl004a",  INFO(0x010212,      0,  64 * 1024,   8, 0) },

1.7.1

This patch adds the quad read support:

(1) Add the relative commands:
OPCODE_QIOR, OPCODE_4QIOR, OPCODE_RDCR,

also add the relative macro for the Configuartion Register.

(2) add the "m25p,quad-read" property for the m25p80 driver If the dts has the "m25p,quad-read" property, the kernel will set the Quad bit of the configuration register, and when the setting suceedes, we will set the read opcode with the right spi nor command.

---
 Documentation/devicetree/bindings/mtd/m25p80.txt |    5 ++
 drivers/mtd/devices/m25p80.c                     |   62 ++++++++++++++++++++++
 include/linux/mtd/spi-nor.h                      |    6 ++
 3 files changed, 73 insertions(+), 0 deletions(-)

index 6d3d576..b33313f 100644
@@ -17,6 +17,11 @@ Optional properties:
                    Refer to your chips' datasheet to check if this is supported
                    by your chip.
 
+- m25p,quad-read : Use the "quad read" opcode to read data from the chip instead
+                   of the usual "read" opcode. This opcode is not supported by
+                   all chips and support for it can not be detected at runtime.
+                   Refer to your chips' datasheet to check if this is supported
+                   by your chip.
 Example:
 
 	flash: m25p80 at 0 {

index f2637b9..e6c3309 100644
@@ -40,6 +40,9 @@
 
 /* Used for Spansion flashes only. */
 #define	OPCODE_BRWR		0x17	/* Bank register write */
+#define	OPCODE_QIOR		0xeb	/* Quad read */
+#define	OPCODE_4QIOR		0xec	/* Quad read (4-byte)*/
+#define	OPCODE_RDCR		0x35	/* Read configuration register */
 
 /* Status Register bits. */
 #define	SR_WIP			1	/* Write in progress */

@@ -50,4 +53,7 @@
 #define	SR_BP2			0x10	/* Block protect 2 */
 #define	SR_SRWD			0x80	/* SR write protect */
 
+/* Configuration Register bits. */
+#define	CR_QUAD			0x2	/* Quad I/O */
+
 #endif /* __LINUX_MTD_SPI_NOR_H */

1.7.1

This patch adds the DDR quad read support by:

(1) Add the relative commands:
OPCODE_DDRQIOR, OPCODE_4DDRQIOR

(2) add the "m25p,ddr-quad-read" property for the m25p80 driver If the dts has the "m25p,ddr-quad-read" property, the kernel will set the Quad bit of the configuration register, and when the setting suceedes, we will set the read opcode with the right spi nor command.

---
 Documentation/devicetree/bindings/mtd/m25p80.txt |    5 +++++
 drivers/mtd/devices/m25p80.c                     |   21 ++++++++++++++++-----
 include/linux/mtd/spi-nor.h                      |    2 ++
 3 files changed, 23 insertions(+), 5 deletions(-)

index b33313f..a01c6b7 100644
@@ -22,6 +22,11 @@ Optional properties:
                    all chips and support for it can not be detected at runtime.
                    Refer to your chips' datasheet to check if this is supported
                    by your chip.
+- m25p,ddr-quad-read : Use the "ddr quad read" opcode to read data from the chip
+                   instead of the usual "read" opcode. This opcode is not
+                   supported by all chips and support for it can not be detected
+                   at runtime. Refer to your chips' datasheet to check if this
+                   is supported by your chip.
 Example:
 
 	flash: m25p80 at 0 {

index 0645c9f..32ccdc7 100644
@@ -913,7 +913,8 @@ static void m25p80_check_quad_read(struct m25p *flash, struct device_node *np)
 	int ret;
 	int sr_cr;
 
-	if (of_property_read_bool(np, "m25p,quad-read")) {
+	if (of_property_read_bool(np, "m25p,quad-read")
+		|| of_property_read_bool(np, "m25p,ddr-quad-read")) {
 		/* The configuration register is set by the second byte. */
 		sr_cr = CR_QUAD << 8;
 

@@ -927,10 +928,20 @@ static void m25p80_check_quad_read(struct m25p *flash, struct device_node *np)
 		if (!(ret > 0 && (ret & CR_QUAD)))
 			return;
 
-		if (flash->mtd.size <= SZ_16M)
-			flash->read_opcode = OPCODE_QIOR;
-		else
-			flash->read_opcode = OPCODE_4QIOR;
+		if (of_property_read_bool(np, "m25p,quad-read")) {
+			if (flash->mtd.size <= SZ_16M)
+				flash->read_opcode = OPCODE_QIOR;
+			else
+				flash->read_opcode = OPCODE_4QIOR;
+			return;
+		}
+
+		if (of_property_read_bool(np, "m25p,ddr-quad-read")) {
+			if (flash->mtd.size <= SZ_16M)
+				flash->read_opcode = OPCODE_DDRQIOR;
+			else
+				flash->read_opcode = OPCODE_4DDRQIOR;
+		}
 	}
 }
 

index e6c3309..a985336 100644
@@ -42,6 +42,8 @@
 #define	OPCODE_BRWR		0x17	/* Bank register write */
 #define	OPCODE_QIOR		0xeb	/* Quad read */
 #define	OPCODE_4QIOR		0xec	/* Quad read (4-byte)*/
+#define	OPCODE_DDRQIOR		0xed	/* DDR Quad read */
+#define	OPCODE_4DDRQIOR		0xee	/* DDR Quad read (4-byte)*/
 #define	OPCODE_RDCR		0x35	/* Read configuration register */
 
 /* Status Register bits. */

1.7.1

(0) What is the Quadspi controller?

The Quadspi(Quad Serial Peripheral Interface) acts as an interface to one single or two external serial flash devices, each with up to 4 bidirectional data lines.

(1) The Quadspi controller is driven by the LUT(Look-up Table) registers. The LUT registers are a look-up-table for sequences of instructions. A valid sequence consists of four LUT registers.

(2) The definition of the LUT register shows below:

---------------------------------------------------
| INSTR1 | PAD1 | OPRND1 | INSTR0 | PAD0 | OPRND0 |
---------------------------------------------------

There are several types of INSTRx, such as:
CMD : the SPI NOR command.
ADDR : the address for the SPI NOR command.
DUMMY : the dummy cycles needed by the SPI NOR command.
....

There are several types of PADx, such as:
PAD1 : use a singe I/O line.
PAD2 : use two I/O lines.
PAD4 : use quad I/O lines.
....

(3) We connect the NOR the QuadSPI now. I am not sure, but i think the QuadSPI will be only used for the NOR. We may connect other devices to it. But, for the reason of (2), we have to parse out the SPI NOR command for the QuadSPI.

(4) Test this driver with the JFFS2 and UBIFS:

For jffs2:
-------------
#flash_eraseall /dev/mtd0
#mount -t jffs2 /dev/mtdblock0 tmp
#bonnie++ -d tmp -u 0 -s 10 -r 5

For ubifs:
-------------
#flash_eraseall /dev/mtd0
#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

(5) The test result of the DDR QUAD Read (66MHz) performance:

        #insmod mtd_speedtest.ko dev=0

	[  194.831313] =================================================
	[  194.825453] mtd_speedtest: MTD device: 0
	[  194.818670] mtd_speedtest: not NAND flash, assume page size is 512 bytes.
	[  194.811705] mtd_speedtest: MTD device size 16777216, eraseblock size 65536,
               page size 512, count of eraseblocks 256, pages per eraseblock 128, OOB size 0
	[  228.482355] mtd_speedtest: testing eraseblock write speed
	[  213.024166] mtd_speedtest: eraseblock write speed is 203 KiB/s
	[  213.018306] mtd_speedtest: testing eraseblock read speed
	[  212.660856] mtd_speedtest: eraseblock read speed is 46545 KiB/s
	[  181.728267] mtd_speedtest: testing page write speed
	[  231.434842] mtd_speedtest: page write speed is 203 KiB/s
	[  231.429515] mtd_speedtest: testing page read speed
	[  228.957422] mtd_speedtest: page read speed is 6641 KiB/s
	[  197.778872] mtd_speedtest: testing 2 page write speed
	[  247.338069] mtd_speedtest: 2 page write speed is 203 KiB/s
	[  247.332514] mtd_speedtest: testing 2 page read speed
	[  245.925048] mtd_speedtest: 2 page read speed is 11686 KiB/s
	[  245.919460] mtd_speedtest: Testing erase speed
	[  214.612341] mtd_speedtest: erase speed is 523 KiB/s
	[  214.607410] mtd_speedtest: Testing 2x multi-block erase speed
	[  245.545971] mtd_speedtest: 2x multi-block erase speed is 480 KiB/s
	[  245.539744] mtd_speedtest: Testing 4x multi-block erase speed
	[  211.141696] mtd_speedtest: 4x multi-block erase speed is 476 KiB/s
	[  211.135496] mtd_speedtest: Testing 8x multi-block erase speed
	[  241.761502] mtd_speedtest: 8x multi-block erase speed is 475 KiB/s
	[  241.755269] mtd_speedtest: Testing 16x multi-block erase speed
	[  272.307979] mtd_speedtest: 16x multi-block erase speed is 474 KiB/s
	[  272.301660] mtd_speedtest: Testing 32x multi-block erase speed
	[  237.637902] mtd_speedtest: 32x multi-block erase speed is 472 KiB/s
	[  237.631581] mtd_speedtest: Testing 64x multi-block erase speed
	[  267.954341] mtd_speedtest: 64x multi-block erase speed is 471 KiB/s
	[  267.948005] mtd_speedtest: finished
	[  267.944478] =================================================

     * Conclusion *:
     --------------------------------------------------------------------
       We can get the 46.5 MiB/s read speed when the DDR Quad Read is enabled.
	(From S25FL128S's spec, the maximum read rate of DDR Quad Read is
	 66MiB/s)
     --------------------------------------------------------------------

---
 drivers/spi/Kconfig           |    7 +
 drivers/spi/Makefile          |    1 +
 drivers/spi/spi-fsl-quadspi.c | 1031 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 1039 insertions(+), 0 deletions(-)
 create mode 100644 drivers/spi/spi-fsl-quadspi.c

index 92b2373..dc38063 100644
@@ -187,6 +187,13 @@ config SPI_FALCON
 	  has only been tested with m25p80 type chips. The hardware has no
 	  support for other types of SPI peripherals.
 
+config SPI_FSL_QUADSPI
+	tristate "Freescale Quad SPI controller"
+	depends on ARCH_MXC
+	help
+	  This enables support for the Quad SPI controller in master mode.
+	  We only connect the NOR to this controller now.
+
 config SPI_GPIO
 	tristate "GPIO-based bitbanging SPI Master"
 	depends on GPIOLIB

index b25f385..7fe505c 100644
@@ -37,6 +37,7 @@ obj-$(CONFIG_SPI_FSL_ESPI)		+= spi-fsl-espi.o
 obj-$(CONFIG_SPI_FSL_SPI)		+= spi-fsl-spi.o
 obj-$(CONFIG_SPI_GPIO)			+= spi-gpio.o
 obj-$(CONFIG_SPI_IMX)			+= spi-imx.o
+obj-$(CONFIG_SPI_FSL_QUADSPI)           += spi-fsl-quadspi.o
 obj-$(CONFIG_SPI_LM70_LLP)		+= spi-lm70llp.o
 obj-$(CONFIG_SPI_MPC512x_PSC)		+= spi-mpc512x-psc.o
 obj-$(CONFIG_SPI_MPC52xx_PSC)		+= spi-mpc52xx-psc.o

1.7.1

For review and merge it would be much easier to send a patch which
implements just the basic SPI support for this device then add the
support for quad mode separately.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130910/287f2861/attachment.sig>

? 2013?09?11? 02:09, Mark Brown ??:

okay. I can remove the quad-read support in next version.

thanks
Huang Shijie

No need to remove it completely, just split it into another patch - that
way we can hopefully merge the bits that are just a standard SPI driver
first even if the other bits are still in discussion.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130911/ac3ade0b/attachment.sig>

This patch adds the binding file for Freescale Quadspi driver.

---
 .../devicetree/bindings/spi/fsl-quadspi.txt        |   32 ++++++++++++++++++++
 1 files changed, 32 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/spi/fsl-quadspi.txt

new file mode 100644
index 0000000..d86ee84
@@ -0,0 +1,32 @@
+* Freescale Quad Serial Peripheral Interface(QuadSPI)
+
+Required properties:
+- compatible : Should be "fsl,vf610-qspi"
+- reg : Offset and length of the register set for the device
+- interrupts : Should contain the interrupt for the device
+- clocks : The clocks needed by the QuadSPI controller
+- clock-names : the name of the clocks
+- num-cs : Contains the number of the chip selects
+
+Optional properties:
+- fsl,nor-size : If You set this property, it means that you connect the NOR
+                 flash to QuadSPI. This property is mainly used by the QuadSPI
+                 to map an AHB bus accessible address.
+
+Example:
+
+qspi0: quadspi at 40044000 {
+	compatible = "fsl,vf610-qspi";
+	reg = <0x40044000 0x1000>;
+	interrupts = <0 24 0x04>;
+	clocks = <&clks VF610_CLK_QSPI0_EN>,
+		<&clks VF610_CLK_QSPI0>;
+	clock-names = "qspi_en", "qspi";
+	fsl,nor-size = <0x1000000>;
+	num-cs = <1>;
+	status = "disabled";
+
+	flash0: s25fl128s at 0 {
+		....
+	};
+};

1.7.1

Current pad values do not works in the 66M DDR quad read mode.

This patch adjusts this pad values, and tested in the 66M DDR quad read mode with S25FL128S.

---
 arch/arm/boot/dts/vf610.dtsi |   24 ++++++++++++------------
 1 files changed, 12 insertions(+), 12 deletions(-)

index 67d929c..c622562 100644
@@ -285,18 +285,18 @@
 				qspi0 {
 					pinctrl_qspi0_1: qspi0grp_1 {
 						fsl,pins = <
-						VF610_PAD_PTD0__QSPI0_A_QSCK	0x307b
-						VF610_PAD_PTD1__QSPI0_A_CS0	0x307f
-						VF610_PAD_PTD2__QSPI0_A_DATA3	0x3073
-						VF610_PAD_PTD3__QSPI0_A_DATA2	0x3073
-						VF610_PAD_PTD4__QSPI0_A_DATA1	0x3073
-						VF610_PAD_PTD5__QSPI0_A_DATA0	0x307b
-						VF610_PAD_PTD7__QSPI0_B_QSCK	0x307b
-						VF610_PAD_PTD8__QSPI0_B_CS0	0x307f
-						VF610_PAD_PTD9__QSPI0_B_DATA3	0x3073
-						VF610_PAD_PTD10__QSPI0_B_DATA2	0x3073
-						VF610_PAD_PTD11__QSPI0_B_DATA1	0x3073
-						VF610_PAD_PTD12__QSPI0_B_DATA0	0x307b
+						VF610_PAD_PTD0__QSPI0_A_QSCK	0x31c3
+						VF610_PAD_PTD1__QSPI0_A_CS0	0x31ff
+						VF610_PAD_PTD2__QSPI0_A_DATA3	0x31c3
+						VF610_PAD_PTD3__QSPI0_A_DATA2	0x31c3
+						VF610_PAD_PTD4__QSPI0_A_DATA1	0x31c3
+						VF610_PAD_PTD5__QSPI0_A_DATA0	0x31c3
+						VF610_PAD_PTD7__QSPI0_B_QSCK	0x31c3
+						VF610_PAD_PTD8__QSPI0_B_CS0	0x31ff
+						VF610_PAD_PTD9__QSPI0_B_DATA3	0x31c3
+						VF610_PAD_PTD10__QSPI0_B_DATA2	0x31c3
+						VF610_PAD_PTD11__QSPI0_B_DATA1	0x31c3
+						VF610_PAD_PTD12__QSPI0_B_DATA0	0x31c3
 						>;
 					};
 				};

1.7.1

vf610-twr has two s25fl128s SPI NOR flashs connected to QuadSpi0. Add support for them.

Note: we enable the DDR Quad read for the two NOR flashs which runs in 66MHz.

---
 arch/arm/boot/dts/vf610-twr.dts |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

index 1a58678..063e94d 100644
@@ -62,3 +62,39 @@
 	pinctrl-0 = <&pinctrl_uart1_1>;
 	status = "okay";
 };
+
+&qspi0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_qspi0_1>;
+	num-cs = <2>;
+	fsl,nor-size = <0x1000000>;
+	status = "okay";
+
+	flash0: s25fl128s at 0 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "spansion,s25fl128s";
+		spi-max-frequency = <66000000>;
+		m25p,ddr-quad-read = <1>;
+		reg = <0>;
+
+		partition at 0 {
+			label = "s25fl128s-0";
+			reg = <0x0 0x1000000>;
+		};
+	};
+
+	flash1: s25fl128s at 1 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+		compatible = "spansion,s25fl128s";
+		spi-max-frequency = <66000000>;
+		m25p,ddr-quad-read = <1>;
+		reg = <1>;
+
+		partition at 0x0 {
+			label = "s25fl128s-1";
+			reg = <0x0 0x1000000>;
+		};
+	};
+};

1.7.1

? 2013?08?30? 10:07, Huang Shijie ??:

Hi Mark:

Since this quadspi driver depends on the mtd code, could this driver goes to the kernel by
the l2-mtd tree?

thanks
Huang Shijie

Why would a SPI driver depend on MTD?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130904/a0bb2746/attachment.sig>

? 2013?09?04? 17:55, Mark Brown ??:

The patch 1-4 are for m25p80.c, and this quadspi controller drivers depends on the
patch 1-4.

thanks
Huang Shijie

In what way does the controller driver depend on those changes?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130904/b4e2264d/attachment.sig>

This driver needs the spi nor command to fill the LUT register, such as OPCODE_WREN(0x06), so the patch 1 moves the spi nor command to a seprate header spi-nor.h, and this driver includes this new header.

thanks
Huang Shijie

Hello Huang,

Huang Shijie wrote on?2013-09-05:

Then some questions come up:
- Why does the spi controller need to know this?
- What is this LUT register at all?
- What happens if something different that a flash is connected and the data starts with one of these opcodes?

Best Regards,
Thomas

The hardware works in this way.

Please see the patch 1.

Submit a new patch to fix it.

thanks
Huang Shijie

Hi Shijie,

This is not the correct approach, because for every NOR flash device user needs to submit a patch to populate these LUT with CMD, OPERAND and DUMMY values specific to that NOR device into these header files. As the supported NOR device list grows, the header file would become unmanageable and cluttered.

I would rather suggest to get these values from DT bindings specific to your fsl-qspi-controller. This way you would provide scalability without affecting other framework. This should also help you to keep your MTD driver independent of QSPI-controller driver.

With regards, pekon

? 2013?09?05? 12:25, Gupta, Pekon ??:

could you show me what values should i set in the DT bindings? The spi nor command? or the dummy? or something else?

thanks
Huang Shijie

Taking example of READ command for S25FL128S NOR flash devices.. S25FL128S supports following flavors of READ modes.
4FAST_READ Read Fast (4-byte Address) 0C
4READ Read (4-byte Address) 13
4DOR Read Dual Out (4-byte Address) 3C
4QOR Read Quad Out (4-byte Address) 6C
4DIOR Dual I/O Read (4-byte Address) BC
4QIOR Quad I/O Read (4-byte Address) EC
4DDRFR Read DDR Fast (4-byte Address) 0E 4DDRDIOR DDR Dual I/O Read (4-byte Address) BE 4DDRQIOR DDR Quad I/O Read (4-byte Address) EE

But due to board constrains and your use-case, you would prefer only few read modes. Those opcodes you can specify via following DT property. "qspi, flash-read-command"

Same way you can have DT property for
"qspi, flash-write-command"
"qspi, flash-erase-command"
"qspi, flash-address-mode" = <4-byte/3-byte>
"qspi, flash-dummy-cycles" = <integer>

Example: How to select opcodes in DT ?
(step-1) eliminate opcode which cannot be used due to board constrains.
your board connects only 2 data I/O between device and controller, So you cannot use any of the QUAD Read opcodes. Thus your choice is limited to DUAL or SINGLE modes only.

(step-2) select opcode based on use-case.
(a) To maximize your throughput you would mostly use DUAL read opcode
- fsl-qspi, flash-read-opcode = 0x3C /* 4DOR Read Dual Out (4-byte Address) */
- fsl-qspi, flash-read-opcode = 0xBC /* 4DIOR Dual I/O Read (4-byte Address) */
(b) But If you are using XIP then you would single out to:
- fsl-qspi, flash-read-opcode = 0xBC /* 4DIOR Dual I/O Read (4-byte Address) */

Though I agree, end-user should be transparent of all these,
but this is better than asking them to submit patches for each new NOR device ..Any thoughts ?

with regards, pekon

? 2013?09?05? 14:32, Gupta, Pekon ??:

thanks for your suggestion.

I ever thought of this solution.

But i do not think this is not a good solution. :(

Firstly, the NOR flash S25FL128S may be used by other boards or other platform.
So if other person uses the S25FL128S, he has to add the SPI NOR command(such as 0xbe, 0xec),
this is really not needed. Why not add these SPI NOR command now?

Secondly, this controller not only needs the write/erase/read, but also need other SPI NOR commands
such as Write-Enable/Read-status/Configurate the Register. if we add these SPI NOR commands to
the DT binding, it will be more ugly to veryone. That's why the Patch 1 is needed.

The only value should be set in the DT is the _dummy_ value. But Add the dummy support should be an other
patchset's job. I think it is a little complicated to add the dummy support.

The fast-read uses 8bit dummy, the quad-read may needs 6bit dummy, and so on..

How can we transfer the dummy value to the device? Add a value to the spi_transfer{}?
I planed to submit another patchset to fix the dummy issue, since it maybe needs more discussion,
i did not include the dummy patches in this patch set.

we have 4 data I/O lines between the device and controller, i only need the Quad mode. :)

thanks
Huang Shijie

Sorry.. I din't mean to be offensive, I should re-word my feedback as your previous solution is not scalable. My suggestion is just a food-for-thought, approvals from other users is required here.

No, SPI generic framework (struct spi_transfer, spi_message,...) should be kept independent of any MTD flash specific data handlers. <[email protected]> added two new fields (tx_nbits, rx_nbits) to spi_device because those properties are part of SPI protocol. But, 'dummy_cycles' is no related to SPI protocol. So it should be kept out of SPI generic framework.
This is where if you could use DT based approach, things would have been simpler, because you give end-user the freedom to enter device-info from device datasheet.

May be because you are currently working on a development EVM or demo board, so you can live with QUAD mode.
But when customer will have custom boards with different QSPI devices then you would end-up supporting all sorts of configurations :-) And in production scenarios, it?s the price economics which mostly dominates which part to choose and how to connect it on board.
Like as I remember some freescale boards have WINBOND QSPI flash which uses different opcodes and semantics, so you might need to support that too in future.

So my suggestion is think again. As you are inviting lot of re-work for yourself and for others too :-)

Anyway, if you really want to continue with this is, then please re-name include/linux/mtd/spi-nor.h to include/linux/mtd/spi-fsl-quadspi.h
because something specific for your driver should not conflict with generic spi-nor framework added later.

with regards, pekon

? 2013?09?05? 17:11, Gupta, Pekon ??:

ok. i think i do not need to change the spi code now.

i think there is no specific thing for this driver. The S25FL128S is a common flash,
other person may uses it too. Could you show me what is specific?

so, i prefer to spi-nor.h.

thanks
Huang Shijie

Looking at the current header there just seem to be defines in there, no
abstraction for the chips. Perhaps that is what is missing?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130905/cddc4fba/attachment.sig>

we do not need any abstraction for this chip, we only need the SPI NOR commands, such as Quad read / DDR quad read.

thanks
Huang Shijie

So the abstraction could say what the commands are, or what's supported
if they're standard?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130908/9454044d/attachment-0001.sig>

? 2013?09?08? 21:47, Mark Brown ??:

thanks.

I have already added the comment for what the commands are, please see patch 3 & patch 4.

In actually, there is no standard for the SPI NOR commands now.

I added these commands just in the Spansion section now. (Of course, other NOR vendors may also uses the same commands.)

thanks
Huang Shijie

So then an abstraction which hides the details of which commands are
needed from the drivers seems sensible?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130909/907297e1/attachment-0001.sig>

? 2013?09?09? 23:14, Mark Brown ??:

sorry, Mark. I feel confused at this comment, i don't catch your meaning. :(

Since I do not expose any details in the spi-nor.h
(i only add some commands in this header), how can i hides the details with an abstraction?

could you explain it more clearly?

thanks
Huang Shijie

The code to work out what opcodes to send to the device should be split
out of the device driver so that other drivers talking to these chips
don't need to reimplement the logic and new chips with new opcodes don't
need to be added to multiple drivers.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130910/f128fe46/attachment-0001.sig>

? 2013?09?11? 02:07, Mark Brown ??:

this code should be in the MTD code, not in the drivers. I have spilted this code, please see patch 3 & patch 4.

Since there is no standard for SPI-NOR commands, we _should_ add new code for
new chips with new opcodes. For example, if a new chip supports the quad-read with
a new command 0xef, we can submit a small patch for m25p80 to fix it.

We have to do it in such way, no shortcut.

thanks
Huang Shijie

So why does the SPI driver have references to the opcodes in it?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130911/39d0f44d/attachment-0001.sig>

? 2013?09?11? 18:41, Mark Brown ??:

I admit that the Freescale's quadspi controller is very special (it is designed too coupled with the SPI Nor FLASH), it is driven by the LUT registers.

I just quote the comment from the patch 1:

-------------------------------------------------------------
(2) The definition of the LUT register shows below:

---------------------------------------------------
| INSTR1 | PAD1 | OPRND1 | INSTR0 | PAD0 | OPRND0 |
---------------------------------------------------

There are several types of INSTRx, such as:
CMD : the SPI NOR command.
ADDR : the address for the SPI NOR command. DUMMY : the dummy cycles needed by the SPI NOR command.
....
-------------------------------------------------------------------

The LUT registers needs to be filled with the SPI NOR command, such Fast-read/Quad-read.
So the Quadspi driver needs to know the spi commands, and that's why the SPI driver has
references to the spi command opcodes.

thanks
Huang Shijie

Indeed - what I was wondering was if those opcodes could be requested
from the flash driver rather than coded in there. The driver needs to
write the commands to the device but it's not clear to me if it really
needs to know the specific commands.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130911/3401fb00/attachment.sig>

Hi Mark, Shijie,

As the talk is going in SPI framework direction, therefore requesting again.. please do not clutter the *generic* SPI framework to pass on opcodes from flash-driver (like m25p80). (Q)SPI framework should remain generic and independent of any upper layer drivers.
Therefore I proposed DT based approach, as an alternative.
http://lists.infradead.org/pipermail/linux-mtd/2013-September/048552.html

But as Shijie's patches are centric to FSL use-case and qspi-controller, so I cannot pursue my suggestions further.

with regards, pekon

No, this isn't about the SPI framework doing this - this is about
something outside the SPI framework being able tell the SPI controllers
that want to know what the opcodes are so we don't have to do this in
every driver that may be affected.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130911/d6efa5a2/attachment.sig>

? 2013?09?11? 19:30, Mark Brown ??:

yes. The driver uses the commands to find the _right_ LUT index, and uses the LUT index to
trigger the controller.

For example, LUT0-LUT3 is used for the READ_STATUS, the driver will match the
0 index for the NOR's read status operation (0x05), and then the driver write 0 to the QSPI_IPCR
register to trigger the operation.

thanks
Huang Shijie

What is a LUT index?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130912/ef6c7710/attachment.sig>

we can setup 16 valid instruction sequences at most, for example:
LUT0 - LUT3: for read status of NOR LUT4 - LUT7: for write enable of NOR
...........

The LUT index is 0 for read-status;
the LUT index is 1 for write-enable.

thanks
Huang Shijie

So... you load LUT index 0 (buffer #0) with something which essentially makes it send the byte 0x05, then read one byte back.

And you load LUT index 1 (buffer #1) with something which makes it send the byte 0x06, and not read anything back.

And then if the transaction that you're asked to make by the client *happens* to match those pre-loaded buffers, you trigger those transactions. And if it doesn't, you fall over.

Have you ever considered just loading the buffer with the transaction you're *asked* to make, and then triggering it? Without preconfiging *anything* in the buffers at init time?

Use LUT index 0 for the first transaction you're asked to make, *regardless* of what it is. Copy the bytes in from the request.

Use LUT index 1 for the *next* transaction, unless it's identical to the first in which case you can re-use index 0.

etc.

dwmw2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5745 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130912/88a8a2ce/attachment-0001.bin>

But why?

Your SPI controller's device driver gets a spi_transfer struct which (indirectly) comes from the m25p80 *chip* driver. That should contain appropriate values for tx_nbits and rx_nbits to indicate the mode which its to be used for this particular command.

All you need to do then is pick a suitable slot in the LUT table, *fill* the LUTn-LUN(n+3) registers with appropriate values, then write (n/4) to the QSPI_IPCR register. Or something like that? (Can you provide a URL for the datasheet for this controller?)

And if this is a command/nbits combination which has recently been used, of course you can skip the 'write the LUT registers' step and just use the same index you used last time. Add some LRU scheme to handle *which* index you use... handwave...

dwmw2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5745 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130912/7f84803e/attachment.bin>

Agree..
A minor update is required in m25p80_read() for different types of reads. For taking examples mentioned earlier:
@@m25p80_read()

	t[0].tx_buf = flash->command; /* read_opcode */
	t[0].len = m25p_cmdsz(flash) + (flash->fast_read ? 1 : 0);

+ t[0].tx_nbits = SPI_NBITS_SINGLE; /* use single wire for this transfer */

	t[0].tx_buf = flash->command; /* read_opcode */
	t[0].len = m25p_cmdsz(flash) + (flash->fast_read ? 1 : 0);

+ t[0].tx_nbits = SPI_NBITS_QUAD; /* use single wire for this transfer */

'tx_nbits' and 'rx_nbits' are new fields added in struct *spi_transfer for this purpose only. Please refer below patch
http://lists.infradead.org/pipermail/linux-mtd/2013-August/047995.html

So, this way master-driver (here mtd/../m25p80.c) can pass the info to slave-driver (here spi/../qspi-controller.c), on how to send flash specific commands and opcodes, by embedding that info in spi_transfer.

And when the spi_transfer reaches spi/.../qspi-controller.c driver, then you can populate your LUT with info present in spi_transfer. And initiate the actual transfer on SPI bus.

Am I missing something here ?
(David please correct me if I'm wrong in above explanation)

with regards, pekon

Right. The issue here is that the LUT is currently *pre-populated*, with an incestuously-"known" set of commands that the slave is expected to support.

For each "known" command, at controller init time we pick an index in the LUT, then pre-configure that index to send the command in question.

Then when we're asked to actually *do* a transaction, we hope that it's one of the "known" ones, look up the right index to use, and just trigger the transaction.

We shouldn't have that horrid incestuous preconfiguration.

When we get an actual request for a transfer, we should find an unused slot in the LUT, *write* the appropriate values for the transaction we're being asked to make, then trigger it.

Obviously the immediate optimisation is to be able to *reuse* LUT configuration so you're not writing to the LUT over and over again for the *same* transactions, but let's not overcomplicate things to start with.

I certainly don't see why we should have the controller knowing in advance what the commands will be.

(Although hey - if you want to *guess* and prepopulate the LUT with the most likely options at init time, that would be fine. As long as you can cope with then being asked to send something unexpected, and rewrite the LUT as needed.)

dwmw2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5745 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130912/e8611a17/attachment-0001.bin>

So, to repeat my earlier question can someone tell me what a LUT is?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130912/f7f82354/attachment.sig>

It's a very common abbreviation - Look Up Table.

More specifically, it's a set of registers which give the SPI controller a sequence of 16-bit commands to execute, which tell it what to do on the SPI bus. Each register contains two commands, and it executes them successively throughout the register set until it reaches a STOP command. These commands tell the SPI controller the width of the bus, a command to send on SPI, whether to read or write data and how much, etc.

If you read the comments in patch 5, and the code, it's all explained there.

I never saw this before until about an hour ago when I took an interest in it after your "what is a LUT" question, and it took less than 15 minutes to work the above out.

Right, but that doesn't have an obvious mapping to SPI...

That's not quite the full story, these appear to be doing flash specific things which depend on the partcular flash being interacted with and which as a result aren't purely at the SPI level. It's not clear if this has some sort of generic meaning for flash or if it really is just a controller specific implementation detail, nor is it clear what the actual meaning of what the commands do is.

The biggest problem with most of this stuff has been poor abstractions and application specific assumptions so I'd not want to just assume that there isn't some broader meaning that's obvious in the flash context, it seems fairly clear that the hardware is specifically targetted at flash so generic flash concepts seem likely to appear in the hardware.

Also bear in mind that we need a respin at the big picture level here, the patches have already been discarded.

I wouldn't assume that a quick scan is going to tell you what's going
on, nor that things have been explained clearly.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130912/ce891ae5/attachment.sig>

A bit of an odd name when used in this context I think; LUTs are usually used to perform mapping or transformation functions, e.g. looking up a value 'y' given an input value 'x'. In this case it's just a table of commands to be executed. But I guess the name LUT has been used in some hardware specification so there's not much we can do about it.

/Ricard

Ricard Wolf Wanderl?f                           ricardw(at)axis.com
Axis Communications AB, Lund, Sweden            www.axis.com
Phone +46 46 272 2016                           Fax +46 46 13 61 30

Remember that not all designers speak the English language, so they may very well call something like this a lookup table, because it's very similar to what they may have implemented in things like video chips to translate colour indexes to actual colours... especially when they call the thing which controls where in the "table" entries are read from an "index". They could've also called it an "array" too.

Either way, it doesn't matter - the principle is that it's a sequence of consecutive programmable entries which provide commands for the controller to use.

Just remember all the problems of flat pack furnature where the instructions are a poor translation of another language - or indeed technological devices for that matter. There's plenty of examples out there where stuff is "lost in translation" and you're left wondering what they're talking about.

I only know what's in the patch that you have also received, but it seems to be a table of commands. To send a given command to the flash, you write the actual command to the some slot in the LUT, then 'trigger' it by writing its index to another register.

I think this whole thing is about the fact that they are *prepopulating* the LUT with a set of 'known' commands, and have incestuous knowledge about what commands will be used, rather than being a truly generic SPI driver and being able to cope with *any* commands that might be sent to the slave(s), dynamically setting up to the LUT as required.

dwmw2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5745 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130912/929d12e1/attachment.bin>

OK, so the LUT itself isn't a generic mtd thing, though the commands it's apparently sending are?

Yup, and I'm also worrying how this is going to work if the flash driver starts supporting SPI controllers which can do the extra data lines but are otherwise dumb and so need the device to explicitly send commands to the device. SPI is just byte streams, it's got no idea about commands.

I'm wondering if we want an abstraction between SPI and the flash chips
which understands SPI commands and could possibly have drivers itself?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130912/613f1d0f/attachment.sig>

Kind of.

Imagine you have a controller with a set of SRAM buffers for transmit data.

One might normally imagine that when you get a request to make a transaction, you'd load the data-to-be-sent into one of the buffers, then trigger a "send from buffer <X>".

Not so in this case. The controller driver appears "know", in advance, what commands are going to be sent. It preloads its buffers (the LUT) with what it *expects* us to send, and when we make a request it'll just trigger a send from the appropriate buffer. And it will crap itself if we actually try to send anything *different*.

Or so I understand it.

So to go back to your question: the commands that it "knows" we are going to send are indeed specific to (one particular type of) SPI NOR flash. But there's nothing on the MTD side which justifies that assumption. Even if you use a different type of SPI NOR flash which uses different opcodes for its commands, this broken scheme will fall over.

Unless, as I say, I've completely misunderstood what's going on here. Huang?

dwmw2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5745 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130912/61d78a7d/attachment-0001.bin>

Yes, that was pretty much my understanding of what was going on here - clearly the way the driver is currently figuring out what the commands are isn't good enough.

Right, the specific opcodes do need to variable at least and should be being provided by the driver for the flash - but is the generic format of the commands and requirement to send them abstractable? Given that it's apparently been baked into hardware and based on having a quick scan through drivers/mtd/devices I'd guess so but...

...like you say some clarity would be good.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130912/9fedf9a8/attachment.sig>

Beside the quad-read/ddr-quad-read commands, all the commands needed by this driver is common. If we do not enable the quad-read, this driver can works with the fast-read mode.

If we use a different NOR which uses different opcodes (different quad-read opcodes ?), i should add a small patch to fix it.

I think i should follow Mark's advice to split out the quad-read code.

thanks
Huang Shijie

That didn't really answer the question, for me. I'm still not entirely clear what's going on.

What *actually* happens, on the wire(s), when the flash driver asks the SPI controller to perform a transaction?

Why can't the flash driver *provide* the required information?

So far I seem to have got it into my head that we have a SPI controller which is connected to more than one device, and we can't send commands to one without sending to the other... and that means that we have to send a *NOP* to the "unwanted" device. Is that right?

The LUT registers just seem to be a controller-specific implementation detail which doesn't really explain the fundamental issue.

It does sound like the general case should ideally be solved by the driver for the *device* telling the SPI controller what to send as a 'NOP' command, if it really has to send something on this channel when we didn't ask it to.

But since we don't actually *probe* SPI devices (do we?) and we rely on them being enumerated in the device-tree or manually 'added' by other methods, perhaps putting the NOP command into the device tree is the better approach. At least it solves the issue of what to use as the NOP command when the m25p80 driver is probing the first of the two chips, before it's *tried* to identify the second.

Or have I completely missed the point here, somewhere?

dwmw2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5745 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130911/fb6d9ac9/attachment-0001.bin>

Yeah, that's my question too...

That shouldn't be the case, SPI has a chip select line which is asserted
to talk to a particular chip, while it's not asserted the slaves should
ignore any activity on the bus. If activity on one slave affects
another something is broken, this is the first time I noticed that being
an issue but I might've missed some of the MTD side of this thread.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130911/51bc0a32/attachment.sig>

Is this not something that could theoretically be provided by the caller when it *makes* the transaction?

Conceptually speaking, could it not be an additional argument to spi_write_then_read() ?

After all, it's the *device* driver (m25p80.c etc.) which will know what the transaction actually *is*, and how many lines the device will want to use for each transaction?

dwmw2

-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5745 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130912/426d49e6/attachment.bin>

Note that spi_write_then_read() is just a helper for building up a spi
transfer in a common pattern.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130912/aa64cf3c/attachment-0001.sig>

yes. we can set the lines information in the spi_write_then_read().

But for the quadspi driver, it does not need this information.

When the drivers knows that it is a Quad-read transaction, it will uses the relative LUT sequence which uses the 4 lines.

thanks
Huang Shijie

But the controller driver shouldn't *have* that incestuous knowledge of the command set of the chip that happens to be connected to it.

That's what we're *complaining* about.

It *should* "need this information", and should just do what it's *told* to do by the slave device driver.

dwmw2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5745 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130912/a88c7648/attachment.bin>

I think the controller is designed for the NOR flash, yes, a little strange.

I can add the lines info in the m25p80_read() for the quad-read, but the lines information is redundant to this Quadspi driver.

I will send you the datasheet tomorrow.

Mark and you want to create the LUT instruction sequence at the runtime, But there is some disadvantage if we do so:
[1] low efficiency:
If you want to change the LUT regitster, you should unlock the LUT register, and change the LUT regitsters, and lock the LUT regitsters again.

[2] we may can not create all the LUT instruction sequence at the runtime. For example, the buffer program(OPCODE_PP):
the m25p80_write() may write 256bytes at a time, but the Quadspi controller only has a 64-byte TX-FIFO, so the controller should write a 64bytes firstly, then sends to the NOR with a read-status command. Do you want to create a read-status LUT instruction sequence at run time? This is not good solution, we should pre-populate the read-status LUT information.

[3] We may can not create the LUT instruction sequence at the runtime, since we can not get enough information from the spi_transfer{}. A whole LUT instruction sequence may needs the following info:
1.) spi command.
2.) lines info: single line, dual lines, quad lines. 3.) Address width: 3 bytes address or 4 bytes address. 4.) instruction type: Read or write or other.
5.) length info: how many bytes for this transaction . 6.) dummy info: how many dummy is needed for this transaction.

We can not get the dummy info from the spi_transfer{}

I may still miss something. But If we want to create a LUT instruction sequence at the runtime, we should _PARSE_ out the SPI NOR command firstly. If we parse out the SPI nor commands, there is no difference between the pre-populete-LUT and create-LUT-at-runtime.

thanks
Huang Shijie

Thanks. Although according to the date header on your email, it is *already* tomorrow. Any chance of fixing your clock, please?

Although there aren't *that* many operations we perform, so you'd quite quickly find yourself using things which are *already* programmed into the LUT instead of having to re-program them again, surely?

I'm not entirely sure I understand this. What *exactly* happens "on the wire" in this case? Do you really send an OPCODE_RDSR (0x05) byte on the wire somehow, when you're supposed to be in the middle of sending the page data to the chip? How does the chip handle that?

Again, what does that actually *mean*, on the wire?

I thought SPI was just 'send some bytes, fetch some bytes'. Why is the controller doing anything other than that?

dwmw2
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5745 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130912/1198ff3f/attachment.bin>

? 2013?09?13? 00:26, David Woodhouse ??:

I was at home then. but now i am at office.

If we can not parse out the SPI NOR commands, we can not quickly find the LUT which already programmed last time.

Yes, I really need to send an OPCODE_RDSR in the middle of sending page data (Page Program) to the chip.

The NOR chip can accept the data input from 1-byte to 256bytes. So it can handle this.

Please see the datasheet of the NOR:
www.spansion.com/Support/Datasheets/S25FL128S_256S_00.pdf

Please see the page 100, the Quad I/O Read, the figure 10.37 shows the dummy.

The dummy is just a delay in a command sequence. The dummy maybe only several clock cycles, less then 8 bit.

As the time goes on, the SPI devices or SPI controllers have become more and more complicated.

If the Quadspi controller can not know the SPI NOR commands explicitly, it can not fill the
LUT correctly, and at last it can not work.

thanks
Huang Shijie

The Vybrid reference manual is available at:
http://cache.freescale.com/files/32bit/doc/ref_manual/VYBRIDRM.pdf

(Chapter 30 is the one for QuadSPI)

? 2013?09?13? 00:27, Fabio Estevam ??:

Hi Fabio:

thanks a lot ! :)

Huang Shijie

I think this is part of the problem - you're trying to represent something that isn't really a SPI controller as a SPI controller (or at least trying to implement functionality beyond that which a SPI controller has).

What this is saying to me is that you should not be impementing this as a SPI controller, trying to do that is breaking the abstracton that SPI is offering. Like people have said SPI is just about byte streams.

I think what you should be doing is refactoring the MTD code which
interfaces to SPI flashes to split out the code so that there's an
abstraction which can express what this controller (and presumably
other controllers) can do and then implement this functionaltiy at
that level.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130912/a0a6b3bb/attachment.sig>

? 2013?09?13? 04:56, Mark Brown ??:

The QuadSpi is not a traditional SPI controller, but i think it is still a SPI controller.
It connects and controls the SPI NOR FLASH, so what do you think it is?

If the quadspi controller is not a SPI controller, what controller is it?

yes, SPI is now just about the byte streams.

But the Quadspi controller can not work with the byte streams directly, it works with the LUT instruction sequences, so we have to parse the byte streams,
and then uses the right LUT to trigger the transaction.

If i follow this advice, i have to create a new file cloned by the m25p80.c, and replace all the SPI APIs with other APIs.

I have no idea what APIs can be used to replace the SPI APIs.

thanks
Huang Shijie

I think it is a SPI flash controller because...

...it has a programming model that seems strongly oriented around that. The device can offer a SPI interface too but as you say that's going to be less efficient. Externally it is still SPI but in terms of what it is offering to the processor the functionality you are trying to support looks like it will work a lot better if the code using it understands that it is talking to a flash rather than having a SPI driver which tries to reverse engineer the data that's being sent to it.

Not quite, you need to split that file up so that there is an API between the flash chip code and the SPI code - the code for the flash chip should be shared between regular SPI and other controllers like this one.

You will need to create them.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130913/347bf631/attachment-0001.sig>

? 2013?09?13? 18:21, Mark Brown ??:

thanks. I will try to do it if i have to do.

I just wonder : what other SPI controller driver would like if the controllers is going to support the
quad-read ?

Huang Shijie

I would expect that most controllers would look just the same as they do now except for the additional ability to do quad transfers - it'd just get told that some transfers should use more data lines.

With this controller it looks like even for normal SPI transfers it's
going to work better knowing that it's doing flash operations.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130916/09b3896c/attachment.sig>

? 2013?09?16? 18:19, Mark Brown ??:

The SPI is about the byte streams, but the dummy may be not a byte, the dummy maybe just 4bit.

It is interesting to handle the dummy issue for the future's SPI controller.

thanks
Huang Shijie

I don't know what you mean by "the dummy".
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130916/d9d31b2f/attachment.sig>

? 2013?09?16? 19:06, Mark Brown ??:

Please see the datasheet of the NOR:
www.spansion.com/Support/Datasheets/S25FL128S_256S_00.pdf

Please see the page 100, the Quad I/O Read, the figure 10.37 shows the dummy.

The dummy is just a delay in a command sequence. The dummy maybe only several clock cycles, less then 8 bit.

thanks
Huang Shijie

This is what the delay_usecs field in the spi_transfer struct is for,
the flash code should be using that to ensure that the minimum delay
requirement is met.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130917/51efd757/attachment.sig>

[ a lurker tries to summarize, and to outline an approach to
introducing generic support for enhanced SPI communication, where QuadSPI just becomes "a byproduct" enabled by extensions to the common infrastructure ]

Not quite. You talk about 'delay_usecs' which is about letting time pass by between partial transfers. The "dummies" mentioned here in this thread are "bit times with _clock pulses_" that get inserted between transfers with single and multiple data lines.

Compare this with M25P opcodes READ 0x03 vs FAST_READ 0x0b, where the latter "communicates" a dummy byte before real data is returned. So far (AFAICS) those dummy bit cycles always were multiples of 8 bits, i.e. complete bytes. So they could be done with bitbang as well as get simulated with "traditional" SPI transfers by just sending another byte without looking at the received data.

[ difference in quality compared to "traditional" SPI ]

There is a new qualitiy here that there are e.g. only 6 "dummy bits" involved, which appear to be encoded in hardware, and are hard to do with "old" SPI controllers. But OTOH those old controllers won't do dual or quad data line transfers either, so they won't need to do sub-byte dummy cycles.

I liked the S25F datasheet that was referenced here recently, it's useful for the discussion that is going on here. I liked the "Serial Peripheral Interface with Multi-I/O" subtitle, which suggests that SPI gets enhanced while nothing of it is specific to flash chips. And I liked the sequence diagrams for their overview or introduction nature, which you can compare to the SPI message submission API in the Linux kernel which connects SPI slave drivers and SPI controller drivers.

[ automatic feature use in the MTD layer considered harmful ]

The datasheet had "block diagrams" (section 3.16) which demonstrated that transfers which involve multiple data lines do depend on
- the SPI slave (the chip) supporting it
- the SPI controller (the chip, IP block) supporting it
- the wiring required to do it (the board)
- all involved software (the drivers) supporting it and (optionally) wanting to use it

which is why the flash chip's capability alone does not suffice to enable the use of such an optional feature. And users or admins may want to control the use of optional features for development, or diagnostics, or compatibility, or whatever reason.

[ generic support for enhanced SPI communication ]

And the datasheet had "sequence diagrams" (section 4.2.1) which one may use to check whether those "phases" in the course of communicating "a command" can get expressed by means of the SPI message submission API (struct spi_message, and struct spi_transfer). Currently "use DDR", "use single/dual/quad data lines", "append dummy _bits_" appear to be missing, but might get added similar to the existing "control the CS line".

The previous discussion suggests that we
- need to express the enhanced modes of SPI communication in terms of SPI message submission data structures (I believe that his can be done with a few straight forward extensions which remain neutral to existing setups)
- make SPI masters (controller drivers) support those optional additional features _if_ the hardware supports it (read: do nothing for existing drivers, and only add optional support to new drivers)
- announce the SPI master's optional support for enhanced features (do we have such a query API already? or is this another leap in quality for SPI? I think bit order and SPI modes 0-3 may already get matched but are not explicitly queried)
- make SPI slave drivers optionally query those master capabilities (plus other sources of configuration information) before they may adjust the SPI messages which they create and emit (which again is to do nothing for all SPI slaves, except for another translation layer in those MTD drivers which happen to use SPI for communication to the chip)

The point is that the M25P80 driver needs to get split into what is currently there (flash chip handling, detection of features and geometry), plus a translation layer which maps "flash chip access" level operations (erase/write/read) to specific "SPI messages which carry out that access".

Consideration of optional features and adjusting the construction of SPI messages (instruction opcodes, address field length, delay/dummy flags, number of data lines, double data rate, et al) all occur within that translation helper. Because the flash chip driver should not do this alone. This helper later may even be usable for other kinds of SPI slaves as well (or serve as a template).

These enhanced modes of SPI communication may be inspired by or may only now become visible because of flash chips, but they should fit into the overall SPI setup and shall be re-usable for other SPI slaves in the future.

[ potential "LUT" optimization, implementation details ]

Putting the "run this many dummy _bit_ cycles" at the "end" of a partial transfer, like is done today for "delay for this many microseconds", might help those controllers which want to get fed a data pattern which automatically modifies characteristics of subsequent data transmission. But this is just an implementation detail, another optional optimization (hopefully this "LUT" thing is an option and not mandatory).

Implementing the "run this partial transfer and _then_ switch to multiple data lines or double data rate" is another option which may help better map those features to hardware implementations.

But I feel that these extensions need to remain isolated to partial SPI message transfers or at the most to individual SPI messages, and cannot get setup in advance nor should they apply globally. It's just not true that every SPI slave is a flash chip. If one of the slaves is a flash chip, no other slave needs to be. And even if there are several flash chips, potentially of the same type or with similar capabilities, they need not be connected in identical ways.

I certainly don't want the physical SPI communication to change in arbitrary ways unexpectedly just because I happen to send a random byte of data to an arbitrary slave that is attached to the bus (like an EEPROM or LCD or any random sensor). BTW I still hope that I overestimate the hardware implementation's greed (haven't checked the QuadSPI controller reference manual). This LUT feature should be optional, and everything that may be changed in automatic ways should be adjustable by software in the transfer setup phase as well -- without being forced to transfer any data as a trigger.

There's a question whether an SPI master's .transfer_one_message() callback (or common code which dispatches messages to SPI controllers) should actively reject messages which require features that aren't supported by the controller. Or whether these flags need to tell "required, reject if not met" and "optional, please use if available" apart.

And one may think of multi data line communication with arbitrary numbers of lines and asymetric Rx/Tx numbers as well. It may not apply to current SPI implementations, but I've seen UARTs which can bundle 1+4 or 2+3 or 3+2 or 4+1 lines, in addition to the regular 1x RX and 1x TX setup.

It's essential to represent dummy cycles in terms of _bit_ numbers, since full bytes no longer universally apply (think:
software bitbang, not everything is done in hardware). And there are SPI controllers or slaves (can't remember who implemented it) where the presence of dummies is known, yet their number isn't, and the end of dummies and start of payload gets determined from a dummy data byte pattern. Is this generic enough a feature to get represented in the SPI message submission API?

virtually yours
Gerhard Sittig

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

noticed that I should have provided the URL so those interested need not search in the thread

www.spansion.com/Support/Datasheets/S25FL128S_256S_00.pdf

and the relevant design items for the SPI subsystem are:
- express those "phases" of communicating a flash chip related "command" in terms of SPI message submission data structures (spi_message, spi_transfer)
- make new SPI master drivers support the optional data rate, multi-line transfer, dummy bit times, etc features _if_ their controller hardware supports them
- announce support of these optional features such that slave drivers can query them
- make SPI slave drivers (specifically SPI attached MTD, i.e. m25p80) map their flash access operations to respective SPI transactions, by introducing an appropriate translation helper

and keep all of the "enhanced modes of SPI communication" independent from their motivation by and use in flash chip drivers

and keep internals of one specific flash chip instruction set out of the SPI transport layer

virtually yours
Gerhard Sittig

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de

Hi,

Stuffs for using dual/quad lines are already present in the SPI framework(3.12-rc1)
Here is the patch[1] that enables quad mode and are more or less use the ideas you suggested above. The patch is based on 3.12-rc1.

Your board file should define "spi-tx-bus-width" and "spi-rx-bus-width". Based on this,
spi->mode will be set in drivers/spi/spi.c. Once this mode is set, we can use
this in our m25p80 driver to enable quad mode and to set the mtd read pointer to
quad read.

Next point is the communication between the mtd layer and the qspi driver layer about
the read command (Normal/dual/quad) to use. For this, there is already a field added in
spi_transfer(tx_nbits/rx_nbits). We can set this field to 1, 2 and 4 etc.. for normal, dual
and quad read.

[1]:

From bd285ebf55a48d16675b92be0f101be7642f6fb2 Mon Sep 17 00:00:00 2001
From: Sourav Poddar <[email protected]>
Date: Wed, 7 Aug 2013 11:15:41 +0530
Subject: [PATCH] drivers: mtd: devices: Add quad read support.

Some flash also support quad read mode.
Adding support for adding quad mode in m25p80.

---
  drivers/mtd/devices/m25p80.c |   87 
++++++++++++++++++++++++++++++++++++++----
  1 files changed, 79 insertions(+), 8 deletions(-)

index 26b14f9..fb9058d 100644
@@ -41,6 +41,7 @@
  #define    OPCODE_WRSR        0x01    /* Write status register 1 byte */
  #define    OPCODE_NORM_READ    0x03    /* Read data bytes (low 

frequency) */
#define OPCODE_FAST_READ 0x0b /* Read data bytes (high
frequency) */
+#define OPCODE_QUAD_READ 0x6b /* QUAD READ */

  #define    OPCODE_PP        0x02    /* Page program (up to 256 bytes) */
  #define    OPCODE_BE_4K        0x20    /* Erase 4KiB block */
  #define    OPCODE_BE_4K_PMC    0xd7    /* Erase 4KiB block on PMC chips */
@@ -95,6 +96,7 @@ struct m25p {
      u8            program_opcode;
      u8            *command;
      bool            fast_read;

+ bool quad_read;
};

static inline struct m25p *mtd_to_m25p(struct mtd_info *mtd)
@@ -336,6 +338,67 @@ static int m25p80_erase(struct mtd_info *mtd, struct erase_info *instr)

      return 0;

}

+static int quad_enable(struct m25p *flash)
+{

+    u8 cmd[3];
+    cmd[0] = OPCODE_WRSR;
+    cmd[1] = 0x00;
+    cmd[2] = 0x02;

+
+ write_enable(flash);
+
+ spi_write(flash->spi, &cmd, 3);
+
+ if (wait_till_ready(flash))
+ return 1;
+
+ return 0;
+}
+
+static int m25p80_quad_read(struct mtd_info *mtd, loff_t from, size_t len,
+ size_t *retlen, u_char *buf)
+{

+    struct m25p *flash = mtd_to_m25p(mtd);
+    struct spi_transfer t[2];
+    struct spi_message m;
+    uint8_t opcode;

+
+ pr_debug("%s: %s from 0x%08x, len %zd\n", dev_name(&flash->spi->dev),
+ __func__, (u32)from, len);
+
+ spi_message_init(&m);
+ memset(t, 0, (sizeof t));
+

+    t[0].tx_buf = flash->command;
+    t[0].len = from;
+    spi_message_add_tail(&t[0], &m);
+
+    t[1].rx_nbits = SPI_NBITS_QUAD;
+    t[1].rx_buf = buf;
+    t[1].len = len;
+    spi_message_add_tail(&t[1], &m);

+
+ mutex_lock(&flash->lock);
+
+ /* Wait till previous write/erase is done. */
+ if (wait_till_ready(flash)) {
+ mutex_unlock(&flash->lock);
+ return 1;
+ }
+

+    opcode = OPCODE_QUAD_READ;
+    flash->command[0] = opcode;
+    m25p_addr2cmd(flash, from, flash->command);

+
+ spi_sync(flash->spi, &m);
+
+ *retlen = len;
+
+ mutex_unlock(&flash->lock);
+
+ return 0;
+}
+
/*
* Read an address range from the flash chip. The address range
* may be any size provided it is within the physical boundaries.
@@ -979,15 +1042,9 @@ static int m25p_probe(struct spi_device *spi)

          }
      }

- flash = kzalloc(sizeof *flash, GFP_KERNEL);

+    flash = devm_kzalloc(&spi->dev, sizeof *flash, GFP_KERNEL);
      if (!flash)
          return -ENOMEM;

- flash->command = kmalloc(MAX_CMD_SIZE + (flash->fast_read ? 1 : 0),
- GFP_KERNEL);
- if (!flash->command) {
- kfree(flash);
- return -ENOMEM;
- }

      flash->spi = spi;
      mutex_init(&flash->lock);
@@ -1015,7 +1072,14 @@ static int m25p_probe(struct spi_device *spi)
      flash->mtd.flags = MTD_CAP_NORFLASH;
      flash->mtd.size = info->sector_size * info->n_sectors;
      flash->mtd._erase = m25p80_erase;

- flash->mtd._read = m25p80_read;
+
+ flash->quad_read = false;
+ if (spi->mode && SPI_RX_QUAD) {

+        quad_enable(flash);
+        flash->mtd._read = m25p80_quad_read;
+        flash->quad_read = true;

+ } else
+ flash->mtd._read = m25p80_read;

      /* flash protection support for STmicro chips */
      if (JEDEC_MFR(info->jedec_id) == CFI_MFR_ST) {

@@ -1067,6 +1131,13 @@ static int m25p_probe(struct spi_device *spi)

      flash->program_opcode = OPCODE_PP;

+ flash->command = kmalloc(MAX_CMD_SIZE + (flash->fast_read ? 1 :
+ (flash->quad_read ? 1 : 0)), GFP_KERNEL);
+ if (!flash->command) {
+ kfree(flash);
+ return -ENOMEM;
+ }
+

      if (info->addr_width)
          flash->addr_width = info->addr_width;
      else if (flash->mtd.size > 0x1000000) {

1.7.1

On Tue, Sep 17, 2013 at 05:05:48PM +0200, Gerhard Sittig wrote:

Please consider writing smaller, more focused e-mails - I suspect quite a few people have tl;dred this...

So just a normal transfer then...

It's not even being simulated really, it's just a transfer - lots of devices have some dead space during their transfers to allow the device time to react. Doing it for less than a byte is an interesting design choice to say the least but doesn't seem hard to deal with.

There's a reason why there's a DT property (or platform data) that needs to be set when registering the flash device. I don't think administrative runtime control of this stuff is a terribly useful thing though, either the hardware works or it doesn't.

Dual and quad support is already present upstrem. I don't know what DDR is.

No, I imagine some existing devices support this stuff - the bitbang driver trivally does for example, if with questionable utility - and most systems will be able to bitbang a few extra cycles on the clock line if they have to.

Things like quad support that depend on the board need to be enabled by the board anyway. For everything else we should follow the existing approach of advertising capabilites in the master structure.

AFAICT the big issue with the LUTs in this controller is that they are expensive to reprogram so things are going to work better if there is a small set of known operations that will happen repeatedly. Otherwise walking the transfer list at runtime isn't too hard.

This stuff gets checked already, more or less. We could probably add
more checks but it's not been a big issue, by the time you're erroring
out it's too late anyway.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130917/c9e399e4/attachment.sig>

I think you can drop the word "known" there. With decent management of the LUT as a kind of LRU cache of recent operations, surely you'll quickly get into a situation where the operations you're actually *doing* on the flash are all already in the LUT and you're never actually *having* to reprogram it?

You don't need to have them set up in advance. Although once you have it correctly doing the LUT management at runtime, sticking some expected operations into the LUT at startup in *anticipation* can't hurt. But let's not talk about that yet.

David Woodhouse                            Open Source Technology Centre
David.Woodhouse at intel.com                              Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130918/0ef9a498/attachment.sig>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/x-pkcs7-signature
Size: 5745 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130918/0ef9a498/attachment.bin>

Yeah, assuming it's not too annoying to identify the repeated patterns
and doesn't blow up horribly just caching should work fine.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130918/41f92132/attachment.sig>