Discussion:
[PATCH v3 0/8] Add the Quadspi driver for vf610-twr
Huang Shijie
2013-08-30 02:07:21 UTC
Permalink
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


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Huang Shijie
2013-08-30 02:07:22 UTC
Permalink
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.)

Signed-off-by: Huang Shijie <***@freescale.com>
---
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

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 26b14f9..299cc69 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -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
diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
new file mode 100644
index 0000000..f2637b9
--- /dev/null
+++ b/include/linux/mtd/spi-nor.h
@@ -0,0 +1,53 @@
+/*
+ * This header contains the SPI-NOR commands and the relative macros
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#ifndef __LINUX_MTD_SPI_NOR_H
+#define __LINUX_MTD_SPI_NOR_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 */
+
+#endif /* __LINUX_MTD_SPI_NOR_H */
--
1.7.1


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Huang Shijie
2013-08-30 02:07:23 UTC
Permalink
Signed-off-by: Huang Shijie <***@freescale.com>
---
drivers/mtd/devices/m25p80.c | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 299cc69..cae419e 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -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


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Huang Shijie
2013-08-30 02:07:24 UTC
Permalink
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.

Signed-off-by: Huang Shijie <***@freescale.com>
---
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(-)

diff --git a/Documentation/devicetree/bindings/mtd/m25p80.txt b/Documentation/devicetree/bindings/mtd/m25p80.txt
index 6d3d576..b33313f 100644
--- a/Documentation/devicetree/bindings/mtd/m25p80.txt
+++ b/Documentation/devicetree/bindings/mtd/m25p80.txt
@@ -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: ***@0 {
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index cae419e..0645c9f 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -103,6 +103,40 @@ static int write_sr(struct m25p *flash, u8 val)
}

/*
+ * Read the configuration register, returning its value in the location
+ * Return the configuration register value.
+ * Returns negative if error occurred.
+ */
+static int read_cr(struct m25p *flash)
+{
+ u8 code = OPCODE_RDCR;
+ int ret;
+ u8 val;
+
+ ret = spi_write_then_read(flash->spi, &code, 1, &val, 1);
+ if (ret < 0) {
+ dev_err(&flash->spi->dev, "error %d reading CR\n", ret);
+ return ret;
+ }
+ return val;
+}
+
+/*
+ * Write status register and configuration register with 2 bytes
+ * The first byte will be written to the status register, while the second byte
+ * will be written to the configuration register.
+ * Returns negative if error occurred.
+ */
+static int write_sr_cr(struct m25p *flash, u16 val)
+{
+ flash->command[0] = OPCODE_WRSR;
+ flash->command[1] = val & 0xff;
+ flash->command[2] = (val >> 8);
+
+ return spi_write(flash->spi, flash->command, 3);
+}
+
+/*
* Set write enable latch with Write Enable command.
* Returns negative if error occurred.
*/
@@ -874,6 +908,31 @@ static const struct spi_device_id *jedec_probe(struct spi_device *spi)
return ERR_PTR(-ENODEV);
}

+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")) {
+ /* The configuration register is set by the second byte. */
+ sr_cr = CR_QUAD << 8;
+
+ /* Write the QUAD bit to the Configuration Register. */
+ write_enable(flash);
+ if (write_sr_cr(flash, sr_cr))
+ return;
+
+ /* read back and check it */
+ ret = read_cr(flash);
+ if (!(ret > 0 && (ret & CR_QUAD)))
+ return;
+
+ if (flash->mtd.size <= SZ_16M)
+ flash->read_opcode = OPCODE_QIOR;
+ else
+ flash->read_opcode = OPCODE_4QIOR;
+ }
+}

/*
* board specific setup should have ensured the SPI clock used here
@@ -1048,6 +1107,9 @@ static int m25p_probe(struct spi_device *spi)
flash->addr_width = 3;
}

+ /* Try to enable the Quad Read */
+ m25p80_check_quad_read(flash, np);
+
dev_info(&spi->dev, "%s (%lld Kbytes)\n", id->name,
(long long)flash->mtd.size >> 10);

diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index f2637b9..e6c3309 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -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


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Huang Shijie
2013-08-30 02:07:25 UTC
Permalink
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.

Signed-off-by: Huang Shijie <***@freescale.com>
---
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(-)

diff --git a/Documentation/devicetree/bindings/mtd/m25p80.txt b/Documentation/devicetree/bindings/mtd/m25p80.txt
index b33313f..a01c6b7 100644
--- a/Documentation/devicetree/bindings/mtd/m25p80.txt
+++ b/Documentation/devicetree/bindings/mtd/m25p80.txt
@@ -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: ***@0 {
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 0645c9f..32ccdc7 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -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;
+ }
}
}

diff --git a/include/linux/mtd/spi-nor.h b/include/linux/mtd/spi-nor.h
index e6c3309..a985336 100644
--- a/include/linux/mtd/spi-nor.h
+++ b/include/linux/mtd/spi-nor.h
@@ -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


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Huang Shijie
2013-08-30 02:07:27 UTC
Permalink
This patch adds the binding file for Freescale Quadspi driver.

Signed-off-by: Huang Shijie <***@freescale.com>
---
.../devicetree/bindings/spi/fsl-quadspi.txt | 32 ++++++++++++++++++++
1 files changed, 32 insertions(+), 0 deletions(-)
create mode 100644 Documentation/devicetree/bindings/spi/fsl-quadspi.txt

diff --git a/Documentation/devicetree/bindings/spi/fsl-quadspi.txt b/Documentation/devicetree/bindings/spi/fsl-quadspi.txt
new file mode 100644
index 0000000..d86ee84
--- /dev/null
+++ b/Documentation/devicetree/bindings/spi/fsl-quadspi.txt
@@ -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: ***@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: ***@0 {
+ ....
+ };
+};
--
1.7.1


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Huang Shijie
2013-08-30 02:07:26 UTC
Permalink
(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)
--------------------------------------------------------------------

Signed-off-by: Huang Shijie <***@freescale.com>
---
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

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 92b2373..dc38063 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -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
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index b25f385..7fe505c 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -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
diff --git a/drivers/spi/spi-fsl-quadspi.c b/drivers/spi/spi-fsl-quadspi.c
new file mode 100644
index 0000000..bbf5d97
--- /dev/null
+++ b/drivers/spi/spi-fsl-quadspi.c
@@ -0,0 +1,1031 @@
+/*
+ * Freescale Quad SPI driver.
+ *
+ * Copyright (C) 2013 Freescale Semiconductor, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/interrupt.h>
+#include <linux/errno.h>
+#include <linux/platform_device.h>
+#include <linux/sched.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/spi/spi.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/timer.h>
+#include <linux/jiffies.h>
+#include <linux/completion.h>
+#include <linux/mtd/spi-nor.h>
+
+/* The registers */
+#define QUADSPI_MCR 0x00
+#define QUADSPI_MCR_RESERVED_SHIFT 16
+#define QUADSPI_MCR_RESERVED_MASK (0xF << QUADSPI_MCR_RESERVED_SHIFT)
+#define QUADSPI_MCR_MDIS_SHIFT 14
+#define QUADSPI_MCR_MDIS_MASK (1 << QUADSPI_MCR_MDIS_SHIFT)
+#define QUADSPI_MCR_CLR_TXF_SHIFT 11
+#define QUADSPI_MCR_CLR_TXF_MASK (1 << QUADSPI_MCR_CLR_TXF_SHIFT)
+#define QUADSPI_MCR_CLR_RXF_SHIFT 10
+#define QUADSPI_MCR_CLR_RXF_MASK (1 << QUADSPI_MCR_CLR_RXF_SHIFT)
+#define QUADSPI_MCR_DDR_EN_SHIFT 7
+#define QUADSPI_MCR_DDR_EN_MASK (1 << QUADSPI_MCR_DDR_EN_SHIFT)
+#define QUADSPI_MCR_SWRSTHD_SHIFT 1
+#define QUADSPI_MCR_SWRSTHD_MASK (1 << QUADSPI_MCR_SWRSTHD_SHIFT)
+#define QUADSPI_MCR_SWRSTSD_SHIFT 0
+#define QUADSPI_MCR_SWRSTSD_MASK (1 << QUADSPI_MCR_SWRSTSD_SHIFT)
+
+#define QUADSPI_IPCR 0x08
+#define QUADSPI_IPCR_SEQID_SHIFT 24
+#define QUADSPI_IPCR_SEQID_MASK (0xF << QUADSPI_IPCR_SEQID_SHIFT)
+
+#define QUADSPI_BUF0CR 0x10
+#define QUADSPI_BUF1CR 0x14
+#define QUADSPI_BUF2CR 0x18
+#define QUADSPI_BUFXCR_INVALID_MSTRID 0xe
+
+#define QUADSPI_BUF3CR 0x1c
+#define QUADSPI_BUF3CR_ALLMST_SHIFT 31
+#define QUADSPI_BUF3CR_ALLMST (1 << QUADSPI_BUF3CR_ALLMST_SHIFT)
+
+#define QUADSPI_BFGENCR 0x20
+#define QUADSPI_BFGENCR_PAR_EN_SHIFT 16
+#define QUADSPI_BFGENCR_PAR_EN_MASK (1 << (QUADSPI_BFGENCR_PAR_EN_SHIFT))
+#define QUADSPI_BFGENCR_SEQID_SHIFT 12
+#define QUADSPI_BFGENCR_SEQID_MASK (0xF << QUADSPI_BFGENCR_SEQID_SHIFT)
+
+#define QUADSPI_BUF0IND 0x30
+#define QUADSPI_BUF1IND 0x34
+#define QUADSPI_BUF2IND 0x38
+#define QUADSPI_SFAR 0x100
+
+#define QUADSPI_SMPR 0x108
+#define QUADSPI_SMPR_DDRSMP_SHIFT 16
+#define QUADSPI_SMPR_DDRSMP_MASK (7 << QUADSPI_SMPR_DDRSMP_SHIFT)
+#define QUADSPI_SMPR_FSDLY_SHIFT 6
+#define QUADSPI_SMPR_FSDLY_MASK (1 << QUADSPI_SMPR_FSDLY_SHIFT)
+#define QUADSPI_SMPR_FSPHS_SHIFT 5
+#define QUADSPI_SMPR_FSPHS_MASK (1 << QUADSPI_SMPR_FSPHS_SHIFT)
+#define QUADSPI_SMPR_HSENA_SHIFT 0
+#define QUADSPI_SMPR_HSENA_MASK (1 << QUADSPI_SMPR_HSENA_SHIFT)
+
+#define QUADSPI_RBSR 0x10c
+#define QUADSPI_RBSR_RDBFL_SHIFT 8
+#define QUADSPI_RBSR_RDBFL_MASK (0x3F << QUADSPI_RBSR_RDBFL_SHIFT)
+
+#define QUADSPI_RBCT 0x110
+#define QUADSPI_RBCT_WMRK_MASK 0x1F
+#define QUADSPI_RBCT_RXBRD_SHIFT 8
+#define QUADSPI_RBCT_RXBRD_USEIPS (0x1 << QUADSPI_RBCT_RXBRD_SHIFT)
+
+#define QUADSPI_TBSR 0x150
+#define QUADSPI_TBDR 0x154
+#define QUADSPI_SR 0x15c
+
+#define QUADSPI_FR 0x160
+#define QUADSPI_FR_TFF_MASK 0x1
+
+#define QUADSPI_SFA1AD 0x180
+#define QUADSPI_SFA2AD 0x184
+#define QUADSPI_SFB1AD 0x188
+#define QUADSPI_SFB2AD 0x18c
+#define QUADSPI_RBDR 0x200
+
+#define QUADSPI_LUTKEY 0x300
+#define QUADSPI_LUTKEY_VALUE 0x5AF05AF0
+
+#define QUADSPI_LCKCR 0x304
+#define QUADSPI_LCKER_LOCK 0x1
+#define QUADSPI_LCKER_UNLOCK 0x2
+
+#define QUADSPI_RSER 0x164
+#define QUADSPI_RSER_TFIE (0x1 << 0)
+
+#define QUADSPI_LUT_BASE 0x310
+
+/*
+ * The definition of the LUT register shows below:
+ *
+ * ---------------------------------------------------
+ * | INSTR1 | PAD1 | OPRND1 | INSTR0 | PAD0 | OPRND0 |
+ * ---------------------------------------------------
+ */
+#define OPRND0_SHIFT 0
+#define PAD0_SHIFT 8
+#define INSTR0_SHIFT 10
+#define OPRND1_SHIFT 16
+
+/* Instruction set for the LUT register. */
+#define LUT_STOP 0
+#define LUT_CMD 1
+#define LUT_ADDR 2
+#define LUT_DUMMY 3
+#define LUT_MODE 4
+#define LUT_MODE2 5
+#define LUT_MODE4 6
+#define LUT_READ 7
+#define LUT_WRITE 8
+#define LUT_JMP_ON_CS 9
+#define LUT_ADDR_DDR 10
+#define LUT_MODE_DDR 11
+#define LUT_MODE2_DDR 12
+#define LUT_MODE4_DDR 13
+#define LUT_READ_DDR 14
+#define LUT_WRITE_DDR 15
+#define LUT_DATA_LEARN 16
+
+/*
+ * The PAD definitions for LUT register.
+ *
+ * The pad stands for the lines number of IO[0:3].
+ * For example, the Quad read need four IO lines, so you should
+ * set LUT_PAD4 which means we use four IO lines.
+ */
+#define LUT_PAD1 0
+#define LUT_PAD2 1
+#define LUT_PAD4 2
+
+/* Oprands for the LUT register. */
+#define ADDR24BIT 0x18
+#define ADDR32BIT 0x20
+
+/* Macros for constructing the LUT register. */
+#define LUT0(ins, pad, opr) \
+ (((opr) << OPRND0_SHIFT) | ((LUT_##pad) << PAD0_SHIFT) | \
+ ((LUT_##ins) << INSTR0_SHIFT))
+
+#define LUT1(ins, pad, opr) (LUT0(ins, pad, opr) << OPRND1_SHIFT)
+
+/* other macros for LUT register. */
+#define QUADSPI_LUT(x) (QUADSPI_LUT_BASE + (x) * 4)
+#define QUADSPI_LUT_NUM 64
+
+/* SEQID -- we can have 16 seqids at most. */
+#define SEQID_QUAD_READ 0
+#define SEQID_WREN 1
+#define SEQID_FAST_READ 2
+#define SEQID_RDSR 3
+#define SEQID_SE 4
+#define SEQID_CHIP_ERASE 5
+#define SEQID_PP 6
+#define SEQID_RDID 7
+#define SEQID_WRSR 8
+#define SEQID_RDCR 9
+#define SEQID_DDRQUAD_READ 10
+
+struct fsl_qspi_handler {
+ int (*setup)(struct spi_device *);
+ int (*do_one_msg)(struct spi_master *, struct spi_message *);
+};
+
+enum fsl_qspi_devtype {
+ FSL_QUADSPI_VYBRID,
+ FSL_QUADSPI_IMX6SLX
+};
+
+struct fsl_qspi_devtype_data {
+ enum fsl_qspi_devtype devtype;
+ u32 memmap_base;
+ int rxfifo;
+ int txfifo;
+};
+
+static struct fsl_qspi_devtype_data vybrid_data = {
+ .devtype = FSL_QUADSPI_VYBRID,
+ .memmap_base = 0x20000000,
+ .rxfifo = 128,
+ .txfifo = 64
+};
+
+struct fsl_qspi {
+ void __iomem *iobase;
+ struct clk *clk, *clk_en;
+ struct device *dev;
+ struct fsl_qspi_handler *h;
+ struct completion c;
+ struct fsl_qspi_devtype_data *devtype_data;
+ void __iomem *ahb_base; /* Used when read from AHB bus */
+ unsigned int chip_base_addr; /* We may support two chips. */
+ unsigned int addr;
+ u32 nor_size; /* for mapping */
+ u8 cmd;
+ unsigned int quad_read_enabled:1;
+ unsigned int has_inited:1;
+};
+
+static inline int is_vybrid_qspi(struct fsl_qspi *q)
+{
+ return q->devtype_data->devtype == FSL_QUADSPI_VYBRID;
+}
+
+/*
+ * An IC bug makes us to re-arrange the 32-bit data.
+ * The following chips, such as IMX6SLX, have fixed this bug.
+ */
+static inline u32 fsl_qspi_endian_xchg(struct fsl_qspi *q, u32 a)
+{
+ return is_vybrid_qspi(q) ? __swab32(a) : a;
+}
+
+static inline void fsl_qspi_unlock_lut(struct fsl_qspi *q)
+{
+ writel(QUADSPI_LUTKEY_VALUE, q->iobase + QUADSPI_LUTKEY);
+ writel(QUADSPI_LCKER_UNLOCK, q->iobase + QUADSPI_LCKCR);
+}
+
+static inline void fsl_qspi_lock_lut(struct fsl_qspi *q)
+{
+ writel(QUADSPI_LUTKEY_VALUE, q->iobase + QUADSPI_LUTKEY);
+ writel(QUADSPI_LCKER_LOCK, q->iobase + QUADSPI_LCKCR);
+}
+
+static irqreturn_t fsl_qspi_irq_handler(int irq, void *dev_id)
+{
+ struct fsl_qspi *q = dev_id;
+ u32 reg;
+
+ /* clear interrupt */
+ reg = readl(q->iobase + QUADSPI_FR);
+ writel(reg, q->iobase + QUADSPI_FR);
+
+ if (reg & QUADSPI_FR_TFF_MASK)
+ complete(&q->c);
+
+ dev_dbg(q->dev, "QUADSPI_FR : 0x%.8x\n", reg);
+ return IRQ_HANDLED;
+}
+
+/* Init the LUT table. All the parameters are from the S25FL128S. */
+static void fsl_qspi_init_lut(struct fsl_qspi *q)
+{
+ void *__iomem base = q->iobase;
+ int rxfifo = q->devtype_data->rxfifo;
+ u32 lut_base;
+ u8 cmd, addrlen, dummy;
+ int i;
+
+ fsl_qspi_unlock_lut(q);
+
+ /* Clear all the LUT table */
+ for (i = 0; i < QUADSPI_LUT_NUM; i++)
+ writel(0, base + QUADSPI_LUT_BASE + i * 4);
+
+ /* Quad Read */
+ lut_base = SEQID_QUAD_READ * 4;
+
+ if (q->nor_size <= SZ_16M) {
+ cmd = OPCODE_QIOR;
+ addrlen = ADDR24BIT;
+ dummy = 4;
+ } else {
+ cmd = OPCODE_4QIOR;
+ addrlen = ADDR32BIT;
+ dummy = 4;
+ }
+
+ writel(LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD4, addrlen),
+ base + QUADSPI_LUT(lut_base));
+ writel(LUT0(MODE, PAD4, 0xff) | LUT1(DUMMY, PAD4, dummy),
+ base + QUADSPI_LUT(lut_base + 1));
+ writel(LUT0(READ, PAD4, rxfifo), base + QUADSPI_LUT(lut_base + 2));
+
+ /* Write enable */
+ lut_base = SEQID_WREN * 4;
+ writel(LUT0(CMD, PAD1, OPCODE_WREN), base + QUADSPI_LUT(lut_base));
+
+ /* Fast Read */
+ lut_base = SEQID_FAST_READ * 4;
+
+ if (q->nor_size <= SZ_16M) {
+ cmd = OPCODE_FAST_READ;
+ addrlen = ADDR24BIT;
+ dummy = 8;
+ } else {
+ cmd = OPCODE_FAST_READ_4B;
+ addrlen = ADDR32BIT;
+ dummy = 8;
+ }
+ writel(LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
+ base + QUADSPI_LUT(lut_base));
+ writel(LUT0(DUMMY, PAD1, dummy) | LUT1(READ, PAD1, rxfifo),
+ base + QUADSPI_LUT(lut_base + 1));
+
+ /* Page Program */
+ lut_base = SEQID_PP * 4;
+
+ if (q->nor_size <= SZ_16M) {
+ cmd = OPCODE_PP;
+ addrlen = ADDR24BIT;
+ } else {
+ cmd = OPCODE_PP_4B;
+ addrlen = ADDR32BIT;
+ }
+
+ writel(LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
+ base + QUADSPI_LUT(lut_base));
+ writel(LUT0(WRITE, PAD1, 0), base + QUADSPI_LUT(lut_base + 1));
+
+ /* Read Status */
+ lut_base = SEQID_RDSR * 4;
+ writel(LUT0(CMD, PAD1, OPCODE_RDSR) | LUT1(READ, PAD1, 0x1),
+ base + QUADSPI_LUT(lut_base));
+
+ /* Erase a sector */
+ lut_base = SEQID_SE * 4;
+
+ if (q->nor_size <= SZ_16M) {
+ cmd = OPCODE_SE;
+ addrlen = ADDR24BIT;
+ } else {
+ cmd = OPCODE_SE_4B;
+ addrlen = ADDR32BIT;
+ }
+
+ writel(LUT0(CMD, PAD1, cmd) | LUT1(ADDR, PAD1, addrlen),
+ base + QUADSPI_LUT(lut_base));
+
+ /* Erase the whole chip */
+ lut_base = SEQID_CHIP_ERASE * 4;
+ writel(LUT0(CMD, PAD1, OPCODE_CHIP_ERASE),
+ base + QUADSPI_LUT(lut_base));
+
+ /* READ ID */
+ lut_base = SEQID_RDID * 4;
+ writel(LUT0(CMD, PAD1, OPCODE_RDID) | LUT1(READ, PAD1, 0x8),
+ base + QUADSPI_LUT(lut_base));
+
+ /* Write Register */
+ lut_base = SEQID_WRSR * 4;
+ writel(LUT0(CMD, PAD1, OPCODE_WRSR) | LUT1(WRITE, PAD1, 0x2),
+ base + QUADSPI_LUT(lut_base));
+
+ /* Read Configuration Register */
+ lut_base = SEQID_RDCR * 4;
+ writel(LUT0(CMD, PAD1, OPCODE_RDCR) | LUT1(READ, PAD1, 0x1),
+ base + QUADSPI_LUT(lut_base));
+
+ /* DDR Quad Read */
+ lut_base = SEQID_DDRQUAD_READ * 4;
+
+ if (q->nor_size <= SZ_16M) {
+ cmd = OPCODE_DDRQIOR;
+ addrlen = ADDR24BIT;
+ dummy = 6;
+ } else {
+ cmd = OPCODE_4DDRQIOR;
+ addrlen = ADDR32BIT;
+ dummy = 6;
+ }
+
+ writel(LUT0(CMD, PAD1, cmd) | LUT1(ADDR_DDR, PAD4, addrlen),
+ base + QUADSPI_LUT(lut_base));
+ writel(LUT0(MODE_DDR, PAD4, 0xff) | LUT1(DUMMY, PAD1, dummy),
+ base + QUADSPI_LUT(lut_base + 1));
+ writel(LUT0(READ_DDR, PAD4, rxfifo),
+ base + QUADSPI_LUT(lut_base + 2));
+
+ fsl_qspi_lock_lut(q);
+}
+
+/* Get the SEQID for the command */
+static int fsl_qspi_get_seqid(struct fsl_qspi *q, u8 cmd)
+{
+ switch (cmd) {
+ case OPCODE_QIOR:
+ case OPCODE_4QIOR:
+ return SEQID_QUAD_READ;
+ case OPCODE_WREN:
+ return SEQID_WREN;
+ case OPCODE_RDSR:
+ return SEQID_RDSR;
+ case OPCODE_SE:
+ return SEQID_SE;
+ case OPCODE_CHIP_ERASE:
+ return SEQID_CHIP_ERASE;
+ case OPCODE_PP:
+ return SEQID_PP;
+ case OPCODE_RDID:
+ return SEQID_RDID;
+ case OPCODE_WRSR:
+ return SEQID_WRSR;
+ case OPCODE_RDCR:
+ return SEQID_RDCR;
+ case OPCODE_DDRQIOR:
+ case OPCODE_4DDRQIOR:
+ return SEQID_DDRQUAD_READ;
+ default:
+ dev_err(q->dev, "Unsupported cmd 0x%.2x\n", cmd);
+ break;
+ }
+ return -EINVAL;
+}
+
+static int
+fsl_qspi_runcmd(struct fsl_qspi *q, u8 cmd, unsigned int addr, int len)
+{
+ void *__iomem base = q->iobase;
+ int seqid;
+ u32 reg;
+ int err;
+
+ init_completion(&q->c);
+ dev_dbg(q->dev, "to 0x%.8x:0x%.8x, len:%d, cmd:%.2x\n",
+ q->chip_base_addr, addr, len, cmd);
+
+ /* save the reg */
+ reg = readl(base + QUADSPI_MCR);
+
+ writel(q->devtype_data->memmap_base + q->chip_base_addr + addr,
+ base + QUADSPI_SFAR);
+ writel(QUADSPI_RBCT_WMRK_MASK | QUADSPI_RBCT_RXBRD_USEIPS,
+ base + QUADSPI_RBCT);
+ writel(reg | QUADSPI_MCR_CLR_RXF_MASK, base + QUADSPI_MCR);
+
+ /* trigger the LUT now */
+ seqid = fsl_qspi_get_seqid(q, cmd);
+ writel((seqid << QUADSPI_IPCR_SEQID_SHIFT) | len, base + QUADSPI_IPCR);
+
+ /* Wait for the interrupt. */
+ err = wait_for_completion_timeout(&q->c, msecs_to_jiffies(1000));
+ if (!err) {
+ dev_err(q->dev,
+ "cmd 0x%.2x timeout, addr@%.8x, FR:0x%.8x, SR:0x%.8x\n",
+ cmd, addr, readl(base + QUADSPI_FR),
+ readl(base + QUADSPI_SR));
+ err = -ETIMEDOUT;
+ } else {
+ err = 0;
+ }
+
+ /* restore the MCR */
+ writel(reg, base + QUADSPI_MCR);
+
+ return err;
+}
+
+static unsigned int fsl_qspi_get_addr(struct fsl_qspi *q,
+ struct spi_transfer *t)
+{
+ unsigned int addr;
+ u8 *buf = (u8 *)t->tx_buf;
+
+ /* 3-byte address */
+ if (q->nor_size <= SZ_16M)
+ addr = (buf[1] << 16) | (buf[2] << 8) | buf[3];
+ else /* 4-byte address */
+ addr = (buf[1] << 24) | (buf[2] << 16) | (buf[3] << 8) | buf[4];
+
+ return addr;
+}
+
+/* Read out the data from the QUADSPI_RBDR buffer registers. */
+static void fsl_qspi_read_data(struct fsl_qspi *q, int len, u32 *rxbuf)
+{
+ u32 tmp;
+ int i = 0;
+
+ while (len > 0) {
+ tmp = readl(q->iobase + QUADSPI_RBDR + i * 4);
+ *rxbuf = fsl_qspi_endian_xchg(q, tmp);
+ dev_dbg(q->dev, "rcv: 0x%.8x, tmp : 0x%.8x\n", *rxbuf, tmp);
+
+ rxbuf++;
+ len -= 4;
+ i++;
+ }
+}
+
+/* Read out the data directly from the AHB buffer.*/
+static int fsl_qspi_read_data_ahb(struct fsl_qspi *q, struct spi_transfer *t)
+{
+ dev_dbg(q->dev, "cmd [%x],read from (0x%p, 0x%.8x, 0x%.8x),len:%d\n",
+ q->cmd, q->ahb_base, q->chip_base_addr, q->addr, t->len);
+ memcpy(t->rx_buf, q->ahb_base + q->chip_base_addr + q->addr, t->len);
+ return 0;
+}
+
+static u32 fsl_qspi_read_sr(struct fsl_qspi *q)
+{
+ u32 val = -EINVAL;
+ int ret;
+
+ ret = fsl_qspi_runcmd(q, OPCODE_RDSR, 0, 1);
+ if (!ret)
+ fsl_qspi_read_data(q, 1, &val);
+ else
+ return ret;
+ return val;
+}
+
+static int fsl_qspi_wait_till_ready(struct fsl_qspi *q)
+{
+ unsigned long deadline;
+ u32 sr;
+
+ deadline = jiffies + msecs_to_jiffies(40000);
+
+ do {
+ cond_resched();
+
+ if ((sr = fsl_qspi_read_sr(q)) < 0)
+ break;
+ else if (!(sr & SR_WIP))
+ return 0;
+ } while (!time_after_eq(jiffies, deadline));
+
+ return (sr < 0) ? sr : -ETIMEDOUT;
+}
+
+/*
+ * If we have changed the content of the flash by writing or erasing,
+ * we need to invalidate the AHB buffer. If we do not do so, we may read out
+ * the wrong data. The spec tells us reset the AHB domain and Serial Flash
+ * domain at the same time.
+ */
+static inline void fsl_qspi_invalid(struct fsl_qspi *q)
+{
+ u32 reg;
+
+ reg = readl(q->iobase + QUADSPI_MCR);
+ reg |= QUADSPI_MCR_SWRSTHD_MASK | QUADSPI_MCR_SWRSTSD_MASK;
+ writel(reg, q->iobase + QUADSPI_MCR);
+
+ /*
+ * The minimum delay : 1 AHB + 2 SFCK clocks.
+ * Delay 1 us is enough.
+ */
+ udelay(1);
+
+ reg &= ~(QUADSPI_MCR_SWRSTHD_MASK | QUADSPI_MCR_SWRSTSD_MASK);
+ writel(reg, q->iobase + QUADSPI_MCR);
+}
+
+static int fsl_qspi_nor_write(struct fsl_qspi *q, u32 *txbuf, unsigned count)
+{
+ unsigned int addr = q->addr;
+ int txfifo_size = q->devtype_data->txfifo;
+ int ret = 0;
+ int tx_size;
+ u32 tmp;
+ int i, j;
+ u8 cmd = q->cmd;
+
+ q->cmd = -1; /* clear the cmd */
+ dev_dbg(q->dev, "to 0x%.8x:0x%.8x, len : %d\n",
+ q->chip_base_addr, addr, count);
+
+ while (count > 0) {
+ tx_size = (count > txfifo_size) ? txfifo_size : count;
+
+ /* clear the TX FIFO. */
+ tmp = readl(q->iobase + QUADSPI_MCR);
+ writel(tmp | QUADSPI_MCR_CLR_RXF_MASK, q->iobase + QUADSPI_MCR);
+
+ /* fill the TX data to the FIFO */
+ for (j = 0, i = ((tx_size + 3) / 4); j < i; j++) {
+ tmp = fsl_qspi_endian_xchg(q, *txbuf);
+ writel(tmp, q->iobase + QUADSPI_TBDR);
+ txbuf++;
+ }
+
+ /* Trigger it */
+ ret = fsl_qspi_runcmd(q, cmd, addr, tx_size);
+
+ addr += tx_size;
+ count -= tx_size;
+
+ /*
+ * If the TX FIFO is smaller then the size of Page Program,
+ * we have to wait until this Write is finished.
+ * For example, the TX FIFO is 64 bytes in the Vybrid,
+ * but the Page Program may writes 265 bytes per time.
+ * We are lucky that some chip(IMX6SLX) has increased the
+ * TX FIFO to 512 bytes.
+ *
+ * If we can change the @m25p->page_size, we can remove the
+ * following code.
+ */
+ if (count > 0) {
+ ret = fsl_qspi_wait_till_ready(q);
+ if (ret) {
+ dev_err(q->dev, "Reading SR, err:%d\n", ret);
+ break;
+ }
+
+ /* Write Enable again. */
+ ret = fsl_qspi_runcmd(q, OPCODE_WREN, 0, 0);
+ if (ret) {
+ dev_err(q->dev, "Write Enable, err:%d\n", ret);
+ break;
+ }
+ }
+ }
+ return ret;
+}
+
+/* Switch to Quad read or DDR Quad read now. */
+static inline void fsl_qspi_enable_quad_read(struct fsl_qspi *q, u8 cmd)
+{
+ int seqid;
+ u32 reg, reg2;
+
+ seqid = fsl_qspi_get_seqid(q, cmd);
+ writel(seqid << QUADSPI_BFGENCR_SEQID_SHIFT,
+ q->iobase + QUADSPI_BFGENCR);
+
+ /* should we enable the DDR ? */
+ if (seqid == SEQID_DDRQUAD_READ) {
+ 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 |= (1 << QUADSPI_SMPR_DDRSMP_SHIFT);
+ writel(reg2, q->iobase + QUADSPI_SMPR);
+
+ /* Enable the module again (enable the DDR too) */
+ writel(reg | QUADSPI_MCR_DDR_EN_MASK, q->iobase + QUADSPI_MCR);
+ }
+
+ q->quad_read_enabled = 1;
+}
+
+static int fsl_qspi_nor_tx(struct fsl_qspi *q, struct spi_transfer *t)
+{
+ unsigned int addr = 0;
+ bool need_invalid = false;
+ int ret = 0;
+ u8 cmd;
+
+ /* This is the second spi_transfer for Page Program. */
+ if (q->cmd == OPCODE_PP) {
+ ret = fsl_qspi_nor_write(q, (u32 *)t->tx_buf, t->len);
+ need_invalid = true;
+ goto qspi_tx_out;
+ }
+
+ cmd = *(u8 *)t->tx_buf;
+ dev_dbg(q->dev, "NOR cmd is [0x%.2x], len : %d.\n", cmd, t->len);
+
+ switch (cmd) {
+ case OPCODE_SE:
+ addr = fsl_qspi_get_addr(q, t);
+ /* fall through */
+ case OPCODE_CHIP_ERASE:
+ need_invalid = true;
+ /* fall through */
+ case OPCODE_WREN:
+ ret = fsl_qspi_runcmd(q, cmd, addr, 0);
+ q->cmd = -1;
+ break;
+
+ case OPCODE_4DDRQIOR:
+ case OPCODE_DDRQIOR:
+ case OPCODE_4QIOR:
+ case OPCODE_QIOR:
+ if (!q->quad_read_enabled)
+ fsl_qspi_enable_quad_read(q, cmd);
+ /* fall through */
+ case OPCODE_FAST_READ:
+ case OPCODE_PP:
+ q->cmd = cmd;
+ q->addr = fsl_qspi_get_addr(q, t);
+ break;
+
+ case OPCODE_WRSR:
+ q->addr = 0;
+ q->cmd = cmd;
+ ret = fsl_qspi_nor_write(q,
+ (u32*)(((u8 *)t->tx_buf) + 1),/* skip the cmd */
+ t->len - 1);
+ break;
+
+ default:
+ q->cmd = cmd;
+ break;
+ }
+
+qspi_tx_out:
+ if (need_invalid)
+ fsl_qspi_invalid(q);
+ return ret;
+}
+
+static int fsl_qspi_nor_rx(struct fsl_qspi *q, struct spi_transfer *t)
+{
+ int ret = 0;
+
+ switch (q->cmd) {
+ case OPCODE_RDSR:
+ case OPCODE_RDCR:
+ case OPCODE_RDID:
+ ret = fsl_qspi_runcmd(q, q->cmd, 0, t->len);
+ if (!ret)
+ fsl_qspi_read_data(q, t->len, t->rx_buf);
+ break;
+
+ case OPCODE_4DDRQIOR:
+ case OPCODE_DDRQIOR:
+ case OPCODE_4QIOR:
+ case OPCODE_QIOR:
+ case OPCODE_FAST_READ:
+ ret = fsl_qspi_read_data_ahb(q, t);
+ break;
+ default:
+ dev_err(q->dev, "Unsupported cmd : %x\n", q->cmd);
+ return -EINVAL;
+ }
+ return ret;
+}
+
+static int fsl_qspi_nor_do_one_msg(struct spi_master *master,
+ struct spi_message *m)
+{
+ struct fsl_qspi *q = spi_master_get_devdata(master);
+ struct spi_transfer *t;
+ int ret = 0;
+
+ /* The chip address we are working. */
+ q->chip_base_addr = q->nor_size * m->spi->chip_select;
+
+ list_for_each_entry(t, &m->transfers, transfer_list) {
+ if (t->tx_buf) {
+ ret = fsl_qspi_nor_tx(q, t);
+ if (!ret)
+ m->actual_length += t->len;
+ continue;
+ }
+
+ if (t->rx_buf) {
+ ret = fsl_qspi_nor_rx(q, t);
+ if (!ret)
+ m->actual_length += t->len;
+ }
+ }
+
+ m->status = ret;
+ spi_finalize_current_message(master);
+ return ret;
+}
+
+/*
+ * There are two different ways to read out the data from the flash:
+ * the "IP Command Read" and the "AHB Command Read".
+ *
+ * The IC guy suggests we use the "AHB Command Read" which is faster
+ * then the "IP Command Read". (What's more is that there is a bug in
+ * the "IP Command Read" in the Vybrid.)
+ *
+ * After we set up the registers for the "AHB Command Read", we can use
+ * the memcpy to read the data directly. A "missed" access to the buffer
+ * causes the controller to clear the buffer, and use the sequence pointed
+ * by the QUADSPI_BFGENCR[SEQID] to initiate a read from the flash.
+ */
+static int fsl_qspi_init_abh_read(struct fsl_qspi *q, struct spi_device *spi)
+{
+ void __iomem *base = q->iobase;
+ u32 memmap_base = q->devtype_data->memmap_base;
+ int nor_size = q->nor_size;
+ int nor_num = spi->master->num_chipselect;
+
+ /* We only can support two NOR flash at the most. */
+ if (nor_num > 2)
+ nor_num = 2;
+
+ /* Map the SPI NOR to accessiable address */
+ writel(nor_size | memmap_base, base + QUADSPI_SFA1AD);
+ writel(nor_size | memmap_base, base + QUADSPI_SFA2AD);
+ writel((nor_size * nor_num) | memmap_base, base + QUADSPI_SFB1AD);
+ writel((nor_size * nor_num) | memmap_base, base + QUADSPI_SFB2AD);
+
+ /* AHB configuration for access buffer 0/1/2 .*/
+ writel(QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF0CR);
+ writel(QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF1CR);
+ writel(QUADSPI_BUFXCR_INVALID_MSTRID, base + QUADSPI_BUF2CR);
+ writel(QUADSPI_BUF3CR_ALLMST, base + QUADSPI_BUF3CR);
+
+ /* We only use the buffer3 */
+ writel(0, base + QUADSPI_BUF0IND);
+ writel(0, base + QUADSPI_BUF1IND);
+ writel(0, base + QUADSPI_BUF2IND);
+
+ /* Set the default lut sequence for AHB Read. */
+ writel(SEQID_FAST_READ << QUADSPI_BFGENCR_SEQID_SHIFT,
+ base + QUADSPI_BFGENCR);
+
+ /* Map the AHB address for read. */
+ q->ahb_base = devm_ioremap(q->dev, memmap_base, nor_size * nor_num);
+ if (!q->ahb_base)
+ return -ENOMEM;
+ return 0;
+}
+
+static int fsl_qspi_nor_setup(struct spi_device *spi)
+{
+ struct fsl_qspi *q = spi_master_get_devdata(spi->master);
+ void __iomem *base = q->iobase;
+ u32 reg;
+ int ret;
+
+ /* We may support two NOR chips, we only need to init one times. */
+ if (q->has_inited)
+ return 0;
+
+ /* The DDR Quad read can run at 66MHz for the S25FL128S. */
+ ret = clk_set_rate(q->clk, spi->max_speed_hz);
+ if (ret)
+ return ret;
+
+ fsl_qspi_init_lut(q);
+ ret = fsl_qspi_init_abh_read(q, spi);
+ if (ret < 0)
+ return ret;
+
+ /* Disable the module */
+ writel(QUADSPI_MCR_MDIS_MASK | QUADSPI_MCR_RESERVED_MASK,
+ base + QUADSPI_MCR);
+
+ reg = readl(base + QUADSPI_SMPR);
+ writel(reg & ~(QUADSPI_SMPR_FSDLY_MASK
+ | QUADSPI_SMPR_FSPHS_MASK
+ | QUADSPI_SMPR_HSENA_MASK
+ | QUADSPI_SMPR_DDRSMP_MASK), base + QUADSPI_SMPR);
+
+ /* Enable the module */
+ writel(QUADSPI_MCR_RESERVED_MASK, base + QUADSPI_MCR);
+
+ /* enable the interrupt */
+ writel(QUADSPI_RSER_TFIE, q->iobase + QUADSPI_RSER);
+
+ q->has_inited = 1;
+ return 0;
+}
+
+/* We only support the NOR now. */
+static struct fsl_qspi_handler fsl_qspi_nor_handler = {
+ .setup = fsl_qspi_nor_setup,
+ .do_one_msg = fsl_qspi_nor_do_one_msg,
+};
+
+static int fsl_qspi_setup(struct spi_device *spi)
+{
+ struct fsl_qspi *q = spi_master_get_devdata(spi->master);
+
+ if (q->h && q->h->setup)
+ return q->h->setup(spi);
+ return 0;
+}
+
+static int fsl_qspi_do_one_msg(struct spi_master *master,
+ struct spi_message *m)
+{
+ struct fsl_qspi *q = spi_master_get_devdata(master);
+
+ if (q->h && q->h->do_one_msg)
+ return q->h->do_one_msg(master, m);
+ return 0;
+}
+
+static struct of_device_id fsl_qspi_dt_ids[] = {
+ { .compatible = "fsl,vf610-qspi", .data = (void*)&vybrid_data, },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, fsl_qspi_dt_ids);
+
+static int fsl_qspi_probe(struct platform_device *pdev)
+{
+ struct device_node *np = pdev->dev.of_node;
+ struct spi_master *master;
+ struct fsl_qspi *q;
+ struct resource *res;
+ int num_cs, ret;
+ const struct of_device_id *of_id =
+ of_match_device(fsl_qspi_dt_ids, &pdev->dev);
+
+ ret = of_property_read_u32(np, "num-cs", &num_cs);
+ if (ret < 0)
+ return ret;
+
+ master = spi_alloc_master(&pdev->dev, sizeof(*q));
+ if (!master)
+ return -ENOMEM;
+
+ q = spi_master_get_devdata(master);
+
+ ret = of_property_read_u32(np, "fsl,nor-size", &q->nor_size);
+ if (!ret && q->nor_size > 0)
+ q->h = &fsl_qspi_nor_handler; /* The default is for NOR.*/
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ q->iobase = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(q->iobase)) {
+ ret = PTR_ERR(q->iobase);
+ goto map_failed;
+ }
+
+ q->clk_en = devm_clk_get(&pdev->dev, "qspi_en");
+ if (IS_ERR(q->clk_en)) {
+ ret = PTR_ERR(q->clk_en);
+ goto map_failed;
+ }
+
+ q->clk = devm_clk_get(&pdev->dev, "qspi");
+ if (IS_ERR(q->clk)) {
+ ret = PTR_ERR(q->clk);
+ goto map_failed;
+ }
+
+ ret = clk_prepare_enable(q->clk_en);
+ if (ret) {
+ dev_err(&pdev->dev, "can not enable the qspi_en clock\n");
+ goto map_failed;
+ }
+
+ ret = clk_prepare_enable(q->clk);
+ if (ret) {
+ clk_disable_unprepare(q->clk_en);
+ dev_err(&pdev->dev, "can not enable the qspi clock\n");
+ goto map_failed;
+ }
+
+ ret = platform_get_irq(pdev, 0);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to get the irq\n");
+ goto irq_failed;
+ }
+
+ ret = devm_request_irq(&pdev->dev, ret,
+ fsl_qspi_irq_handler, 0, pdev->name, q);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to request irq.\n");
+ goto irq_failed;
+ }
+
+ q->dev = &pdev->dev;
+ q->devtype_data = (struct fsl_qspi_devtype_data *)of_id->data;
+
+ master->bus_num = pdev->id;
+ master->num_chipselect = num_cs;
+ master->dev.of_node = pdev->dev.of_node;
+ master->setup = fsl_qspi_setup;
+ master->transfer_one_message = fsl_qspi_do_one_msg;
+ master->flags = SPI_MASTER_HALF_DUPLEX;
+ platform_set_drvdata(pdev, master);
+
+ ret = spi_register_master(master);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to register the spi master.\n");
+ goto irq_failed;
+ }
+ dev_info(&pdev->dev, "QuadSPI bus driver\n");
+ return 0;
+
+irq_failed:
+ clk_disable_unprepare(q->clk);
+ clk_disable_unprepare(q->clk_en);
+map_failed:
+ spi_master_put(master);
+
+ dev_err(&pdev->dev, "Freescale QuadSPI probe failed\n");
+ return ret;
+}
+
+static int fsl_qspi_remove(struct platform_device *pdev)
+{
+ struct spi_master *master = platform_get_drvdata(pdev);
+ struct fsl_qspi *q = spi_master_get_devdata(master);
+
+ /* disable the hardware */
+ writel(QUADSPI_MCR_MDIS_MASK, q->iobase + QUADSPI_MCR);
+ writel(0x0, q->iobase + QUADSPI_RSER);
+
+ clk_disable_unprepare(q->clk);
+ clk_disable_unprepare(q->clk_en);
+ spi_master_put(master);
+ return 0;
+}
+
+static struct platform_driver fsl_qspi_driver = {
+ .driver = {
+ .name = "fsl-quadspi",
+ .owner = THIS_MODULE,
+ .of_match_table = fsl_qspi_dt_ids,
+ },
+ .probe = fsl_qspi_probe,
+ .remove = fsl_qspi_remove,
+};
+module_platform_driver(fsl_qspi_driver);
+
+MODULE_DESCRIPTION("Freescale QuadSPI Controller Driver");
+MODULE_LICENSE("GPL v2");
--
1.7.1


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
2013-09-10 18:09:23 UTC
Permalink
On Fri, Aug 30, 2013 at 10:07:26AM +0800, Huang Shijie wrote:
> (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.

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.
Huang Shijie
2013-09-11 02:40:51 UTC
Permalink
=D3=DA 2013=C4=EA09=D4=C211=C8=D5 02:09, Mark Brown =D0=B4=B5=C0:
> On Fri, Aug 30, 2013 at 10:07:26AM +0800, Huang Shijie wrote:
>> (0) What is the Quadspi controller?
>>
>> The Quadspi(Quad Serial Peripheral Interface) acts as an interfa=
ce to
>> one single or two external serial flash devices, each with up to=
4
>> bidirectional data lines.
> 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.
okay. I can remove the quad-read support in next version.

thanks
Huang Shijie

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
2013-09-11 10:39:03 UTC
Permalink
On Wed, Sep 11, 2013 at 10:40:51AM +0800, Huang Shijie wrote:
> 于 2013幎09月11日 02:09, Mark Brown 写道:

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

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

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.
Huang Shijie
2013-08-30 02:07:28 UTC
Permalink
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.

Signed-off-by: Huang Shijie <***@freescale.com>
---
arch/arm/boot/dts/vf610.dtsi | 24 ++++++++++++------------
1 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/arm/boot/dts/vf610.dtsi b/arch/arm/boot/dts/vf610.dtsi
index 67d929c..c622562 100644
--- a/arch/arm/boot/dts/vf610.dtsi
+++ b/arch/arm/boot/dts/vf610.dtsi
@@ -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


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Huang Shijie
2013-08-30 02:07:29 UTC
Permalink
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.

Signed-off-by: Huang Shijie <***@freescale.com>
---
arch/arm/boot/dts/vf610-twr.dts | 36 ++++++++++++++++++++++++++++++++++++
1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/arch/arm/boot/dts/vf610-twr.dts b/arch/arm/boot/dts/vf610-twr.dts
index 1a58678..063e94d 100644
--- a/arch/arm/boot/dts/vf610-twr.dts
+++ b/arch/arm/boot/dts/vf610-twr.dts
@@ -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: ***@0 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "spansion,s25fl128s";
+ spi-max-frequency = <66000000>;
+ m25p,ddr-quad-read = <1>;
+ reg = <0>;
+
+ ***@0 {
+ label = "s25fl128s-0";
+ reg = <0x0 0x1000000>;
+ };
+ };
+
+ flash1: ***@1 {
+ #address-cells = <1>;
+ #size-cells = <1>;
+ compatible = "spansion,s25fl128s";
+ spi-max-frequency = <66000000>;
+ m25p,ddr-quad-read = <1>;
+ reg = <1>;
+
+ ***@0x0 {
+ label = "s25fl128s-1";
+ reg = <0x0 0x1000000>;
+ };
+ };
+};
--
1.7.1


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Huang Shijie
2013-09-04 02:16:09 UTC
Permalink
于 2013年08月30日 10:07, Huang Shijie 写道:
> (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.
>
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
Mark Brown
2013-09-04 09:55:23 UTC
Permalink
On Wed, Sep 04, 2013 at 10:16:09AM +0800, Huang Shijie wrote:

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

Why would a SPI driver depend on MTD?
Huang Shijie
2013-09-04 10:29:34 UTC
Permalink
=D3=DA 2013=C4=EA09=D4=C204=C8=D5 17:55, Mark Brown =D0=B4=B5=C0:
> On Wed, Sep 04, 2013 at 10:16:09AM +0800, Huang Shijie wrote:
>
>> Since this quadspi driver depends on the mtd code, could this driver
>> goes to the kernel by
>> the l2-mtd tree?
> Why would a SPI driver depend on MTD?
The patch 1-4 are for m25p80.c, and this quadspi controller drivers
depends on the
patch 1-4.



thanks
Huang Shijie

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
2013-09-04 11:33:22 UTC
Permalink
On Wed, Sep 04, 2013 at 06:29:34PM +0800, Huang Shijie wrote:
> 于 2013幎09月04日 17:55, Mark Brown 写道:
> > On Wed, Sep 04, 2013 at 10:16:09AM +0800, Huang Shijie wrote:

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

> > Why would a SPI driver depend on MTD?

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

In what way does the controller driver depend on those changes?
Huang Shijie
2013-09-05 01:43:53 UTC
Permalink
On Wed, Sep 04, 2013 at 12:33:22PM +0100, Mark Brown wrote:
> On Wed, Sep 04, 2013 at 06:29:34PM +0800, Huang Shijie wrote:
> > =E4=BA=8E 2013=E5=B9=B409=E6=9C=8804=E6=97=A5 17:55, Mark Brown =E5=
=86=99=E9=81=93:
> > > On Wed, Sep 04, 2013 at 10:16:09AM +0800, Huang Shijie wrote:
>=20
> > >> Since this quadspi driver depends on the mtd code, could this dr=
iver
> > >> goes to the kernel by
> > >> the l2-mtd tree?
>=20
> > > Why would a SPI driver depend on MTD?
>=20
> > The patch 1-4 are for m25p80.c, and this quadspi controller drivers
> > depends on the
> > patch 1-4.
>=20
> In what way does the controller driver depend on those changes?
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 seprat=
e
header spi-nor.h, and this driver includes this new header.

thanks
Huang Shijie

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
t***@lantiq.com
2013-09-04 13:45:40 UTC
Permalink
Hello Huang,

Huang Shijie wrote on 2013-09-05:
> On Wed, Sep 04, 2013 at 12:33:22PM +0100, Mark Brown wrote:
>> On Wed, Sep 04, 2013 at 06:29:34PM +0800, Huang Shijie wrote:
>>> 于 2013年09月04日 17:55, Mark Brown 写道:
>>>> On Wed, Sep 04, 2013 at 10:16:09AM +0800, Huang Shijie wrote:
>>
>>>>> Since this quadspi driver depends on the mtd code, could this driver
>>>>> goes to the kernel by
>>>>> the l2-mtd tree?
>>
>>>> Why would a SPI driver depend on MTD?
>>
>>> The patch 1-4 are for m25p80.c, and this quadspi controller drivers
>>> depends on the
>>> patch 1-4.
>>
>> In what way does the controller driver depend on those changes?
> 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.
>
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
Huang Shijie
2013-09-05 02:04:37 UTC
Permalink
On Wed, Sep 04, 2013 at 01:45:40PM +0000, ***@lantiq.com wrote:
> Hello Huang,
>
> >> In what way does the controller driver depend on those changes?
> > 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.
> >
> Then some questions come up:
> - Why does the spi controller need to know this?
The hardware works in this way.
> - What is this LUT register at all?
Please see the patch 1.
> - What happens if something different that a flash is connected and
> the data starts with one of these opcodes?
Submit a new patch to fix it.


thanks
Huang Shijie
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Gupta, Pekon
2013-09-05 04:25:39 UTC
Permalink
Huang Shijie
2013-09-05 05:34:15 UTC
Permalink
=E4=BA=8E 2013=E5=B9=B409=E6=9C=8805=E6=97=A5 12:25, Gupta, Pekon =E5=86=
=99=E9=81=93:
> I would rather suggest to get these values from DT bindings specific =
to
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

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Gupta, Pekon
2013-09-05 06:32:58 UTC
Permalink
Huang Shijie
2013-09-05 07:45:30 UTC
Permalink
=E4=BA=8E 2013=E5=B9=B409=E6=9C=8805=E6=97=A5 14:32, Gupta, Pekon =E5=86=
=99=E9=81=93:
>> =E4=BA=8E 2013=E5=B9=B409=E6=9C=8805=E6=97=A5 12:25, Gupta, Pekon =E5=
=86=99=E9=81=93:
>>> I would rather suggest to get these values from DT bindings specifi=
c to
>> could you show me what values should i set in the DT bindings?
>> The spi nor command? or the dummy? or something else?
>>
> 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" =3D<4-byte/3-byte>
> "qspi, flash-dummy-cycles" =3D<integer>
thanks for your suggestion.

I ever thought of this solution.

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

=46irstly, the NOR flash S25FL128S may be used by other boards or other=
=20
platform.
So if other person uses the S25FL128S, he has to add the SPI NOR=20
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=
=20
need other SPI NOR commands
such as Write-Enable/Read-status/Configurate the Register. if we add=20
these SPI NOR commands to
the DT binding, it will be more ugly to veryone. That's why the Patch 1=
=20
is needed.


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

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

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








> Example: How to select opcodes in DT ?
> (step-1) eliminate opcode which cannot be used due to board constrain=
s.
> 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 limit=
ed to
> DUAL or SINGLE modes only.
we have 4 data I/O lines between the device and controller, i only need=
=20
the Quad mode. :)

thanks
Huang Shijie

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Gupta, Pekon
2013-09-05 09:11:59 UTC
Permalink
> 于 2013年09月05日 14:32, Gupta, Pekon 写道:
> >> 于 2013年09月05日 12:25, Gupta, Pekon 写道:
> >>> I would rather suggest to get these values from DT bindings specific to
> >> could you show me what values should i set in the DT bindings?
> >> The spi nor command? or the dummy? or something else?
> >>
> > 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>
> thanks for your suggestion.
>
> I ever thought of this solution.
>
> But i do not think this is not a good solution. :(
>
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.

> 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.
>
No, SPI generic framework (struct spi_transfer, spi_message,...)
should be kept independent of any MTD flash specific data handlers.
<***@gmail.com> 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.
>
>
>
>
>
>
>
> > 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.
> we have 4 data I/O lines between the device and controller, i only need
> the Quad mode. :)
>
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
Huang Shijie
2013-09-05 09:30:26 UTC
Permalink
=E4=BA=8E 2013=E5=B9=B409=E6=9C=8805=E6=97=A5 17:11, Gupta, Pekon =E5=86=
=99=E9=81=93:
> No, SPI generic framework (struct spi_transfer, spi_message,...)
> should be kept independent of any MTD flash specific data handlers.
> <***@gmail.com> 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 ke=
pt out
> of SPI generic framework.
> This is where if you could use DT based approach, things would have b=
een
> simpler, because you give end-user the freedom to enter device-info f=
rom
> device datasheet.
ok. i think i do not need to change the spi code now.
>> > =20
>> > =20
>> > =20
>> > =20
>> > =20
>> > =20
>> > =20
>>> > > Example: How to select opcodes in DT ?
>>> > > (step-1) eliminate opcode which cannot be used due to board c=
onstrains.
>>> > > your board connects only 2 data I/O between device and con=
troller, So you
>>> > > cannot use any of the QUAD Read opcodes. Thus your choice=
is limited to
>>> > > DUAL or SINGLE modes only.
>> > we have 4 data I/O lines between the device and controller, i onl=
y need
>> > the Quad mode.:)
>> > =20
> 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=E2=80=99s the price economics which m=
ostly 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 supp=
ort
> that too in future.
>
> So my suggestion is think again. As you are inviting lot of re-work f=
or yourself
> and for others too:-)
>
> Anyway, if you really want to continue with this is, then please re-n=
ame
> 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.
i think there is no specific thing for this driver. The S25FL128S is a=20
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

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
2013-09-05 13:50:32 UTC
Permalink
On Thu, Sep 05, 2013 at 05:30:26PM +0800, Huang Shijie wrote:
> 于 2013幎09月05日 17:11, Gupta, Pekon 写道:

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

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

Looking at the current header there just seem to be defines in there, no
abstraction for the chips. Perhaps that is what is missing?
Huang Shijie
2013-09-06 02:36:44 UTC
Permalink
On Thu, Sep 05, 2013 at 02:50:32PM +0100, Mark Brown wrote:
> On Thu, Sep 05, 2013 at 05:30:26PM +0800, Huang Shijie wrote:
> > =E4=BA=8E 2013=E5=B9=B409=E6=9C=8805=E6=97=A5 17:11, Gupta, Pekon =E5=
=86=99=E9=81=93:
>=20
> > >Anyway, if you really want to continue with this is, then please r=
e-name
> > >include/linux/mtd/spi-nor.h to
> > >include/linux/mtd/spi-fsl-quadspi.h
> > >because something specific for your driver should not conflict wit=
h
> > >generic spi-nor framework added later.
>=20
> > i think there is no specific thing for this driver. The S25FL128S i=
s
> > a common flash,
> > other person may uses it too. Could you show me what is specific?
>=20
> > so, i prefer to spi-nor.h.
>=20
> Looking at the current header there just seem to be defines in there,=
no
> abstraction for the chips. Perhaps that is what is missing?
we do not need any abstraction for this chip, we only need the SPI NOR=20
commands, such as Quad read / DDR quad read.


thanks
Huang Shijie





--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
2013-09-08 13:47:25 UTC
Permalink
On Thu, Sep 05, 2013 at 10:36:44PM -0400, Huang Shijie wrote:
> On Thu, Sep 05, 2013 at 02:50:32PM +0100, Mark Brown wrote:

> > Looking at the current header there just seem to be defines in there, no
> > abstraction for the chips. Perhaps that is what is missing?

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

So the abstraction could say what the commands are, or what's supported
if they're standard?
Huang Shijie
2013-09-09 03:07:37 UTC
Permalink
=D3=DA 2013=C4=EA09=D4=C208=C8=D5 21:47, Mark Brown =D0=B4=B5=C0:
> On Thu, Sep 05, 2013 at 10:36:44PM -0400, Huang Shijie wrote:
>> On Thu, Sep 05, 2013 at 02:50:32PM +0100, Mark Brown wrote:
>>> Looking at the current header there just seem to be defines in ther=
e, no
>>> abstraction for the chips. Perhaps that is what is missing?
>> we do not need any abstraction for this chip, we only need the SPI N=
OR=20
>> commands, such as Quad read / DDR quad read.
> So the abstraction could say what the commands are, or what's support=
ed
thanks.

I have already added the comment for what the commands are, please see
patch 3 & patch 4.
> if they're standard?
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




--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
2013-09-09 15:14:50 UTC
Permalink
On Mon, Sep 09, 2013 at 11:07:37AM +0800, Huang Shijie wrote:
> 于 2013幎09月08日 21:47, Mark Brown 写道:

> > So the abstraction could say what the commands are, or what's supported

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

> > if they're standard?

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

So then an abstraction which hides the details of which commands are
needed from the drivers seems sensible?
Huang Shijie
2013-09-10 06:59:07 UTC
Permalink
=D3=DA 2013=C4=EA09=D4=C209=C8=D5 23:14, Mark Brown =D0=B4=B5=C0:
> So then an abstraction which hides the details of which commands are
> needed from the drivers seems sensible?
sorry, Mark. I feel confused at this comment, i don't catch your meanin=
g. :(

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

--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
2013-09-10 18:07:04 UTC
Permalink
On Tue, Sep 10, 2013 at 02:59:07PM +0800, Huang Shijie wrote:

> 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?

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.
Huang Shijie
2013-09-11 02:38:02 UTC
Permalink
=D3=DA 2013=C4=EA09=D4=C211=C8=D5 02:07, Mark Brown =D0=B4=B5=C0:
> The code to work out what opcodes to send to the device should be spl=
it
> out of the device driver so that other drivers talking to these chips
this code should be in the MTD code, not in the drivers.
I have spilted this code, please see patch 3 & patch 4.


> don't need to reimplement the logic and new chips with new opcodes do=
n't
> need to be added to multiple drivers.
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




--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
2013-09-11 10:41:26 UTC
Permalink
On Wed, Sep 11, 2013 at 10:38:02AM +0800, Huang Shijie wrote:
> 于 2013幎09月11日 02:07, Mark Brown 写道:

> > don't need to reimplement the logic and new chips with new opcodes don't
> > need to be added to multiple drivers.

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

So why does the SPI driver have references to the opcodes in it?
Huang Shijie
2013-09-11 10:54:55 UTC
Permalink
=D3=DA 2013=C4=EA09=D4=C211=C8=D5 18:41, Mark Brown =D0=B4=B5=C0:
> So why does the SPI driver have references to the opcodes in it?
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.
=2E...
-------------------------------------------------------------------

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

=20
thanks
Huang Shijie




--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
2013-09-11 11:30:02 UTC
Permalink
On Wed, Sep 11, 2013 at 06:54:55PM +0800, Huang Shijie wrote:

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

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.
Gupta, Pekon
2013-09-11 12:26:10 UTC
Permalink
Mark Brown
2013-09-11 12:35:53 UTC
Permalink
On Wed, Sep 11, 2013 at 12:26:10PM +0000, Gupta, Pekon wrote:
> Hi Mark, Shijie,

> > Indeed - what I was wondering was if those opcodes could be requested
> > from the flash driver rather than coded in there.

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

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.
Huang Shijie
2013-09-12 09:18:23 UTC
Permalink
=D3=DA 2013=C4=EA09=D4=C211=C8=D5 19:30, Mark Brown =D0=B4=B5=C0:
> Indeed - what I was wondering was if those opcodes could be requested
> from the flash driver rather than coded in there. The driver needs t=
o
> write the commands to the device but it's not clear to me if it reall=
y
> needs to know the specific commands.
yes. The driver uses the commands to find the _right_ LUT index, and
uses the LUT index to
trigger the controller.

=46or 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


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
2013-09-12 10:20:27 UTC
Permalink
On Thu, Sep 12, 2013 at 05:18:23PM +0800, Huang Shijie wrote:

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

What is a LUT index?
Huang Shijie
2013-09-13 03:30:10 UTC
Permalink
On Thu, Sep 12, 2013 at 11:20:27AM +0100, Mark Brown wrote:
> On Thu, Sep 12, 2013 at 05:18:23PM +0800, Huang Shijie wrote:
>
> > yes. The driver uses the commands to find the _right_ LUT index, and
> > uses the LUT index to
> > trigger the controller.
>
> What is a LUT index?
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
David Woodhouse
2013-09-12 15:32:55 UTC
Permalink
On Thu, 2013-09-12 at 23:30 -0400, Huang Shijie wrote:
>
> 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.

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
David Woodhouse
2013-09-12 10:39:55 UTC
Permalink
On Thu, 2013-09-12 at 17:18 +0800, Huang Shijie wrote:
> 于 2013幎09月11日 19:30, Mark Brown 写道:
> > 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.
> 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.

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
Gupta, Pekon
2013-09-12 10:56:14 UTC
Permalink
> > > 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.
> > 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.
>
> 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...
>
Agree..
A minor update is required in m25p80_read() for different types of reads.
For taking examples mentioned earlier:
@@m25p80_read()
> > the read status only use a single line
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 */

> > , while the quad read uses 4 lines.
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
David Woodhouse
2013-09-12 11:17:45 UTC
Permalink
On Thu, 2013-09-12 at 10:56 +0000, Gupta, Pekon wrote:
>
> 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.

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
Mark Brown
2013-09-12 11:48:31 UTC
Permalink
On Thu, Sep 12, 2013 at 12:17:45PM +0100, David Woodhouse wrote:

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

So, to repeat my earlier question can someone tell me what a LUT is?
Russell King - ARM Linux
2013-09-12 11:55:49 UTC
Permalink
On Thu, Sep 12, 2013 at 12:48:31PM +0100, Mark Brown wrote:
> On Thu, Sep 12, 2013 at 12:17:45PM +0100, David Woodhouse wrote:
>
> > 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.
>
> So, to repeat my earlier question can someone tell me what a LUT is?

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
2013-09-12 13:24:04 UTC
Permalink
On Thu, Sep 12, 2013 at 12:55:49PM +0100, Russell King - ARM Linux wrote:
> On Thu, Sep 12, 2013 at 12:48:31PM +0100, Mark Brown wrote:

> > So, to repeat my earlier question can someone tell me what a LUT is?

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

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

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

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.

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

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

I wouldn't assume that a quick scan is going to tell you what's going
on, nor that things have been explained clearly.
Ricard Wanderlof
2013-09-12 13:25:45 UTC
Permalink
On Thu, 12 Sep 2013, Russell King - ARM Linux wrote:

> On Thu, Sep 12, 2013 at 12:48:31PM +0100, Mark Brown wrote:
>> On Thu, Sep 12, 2013 at 12:17:45PM +0100, David Woodhouse wrote:
>>
>>> 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.
>>
>> So, to repeat my earlier question can someone tell me what a LUT is?
>
> 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
> ...

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
Russell King - ARM Linux
2013-09-12 14:10:11 UTC
Permalink
On Thu, Sep 12, 2013 at 03:25:45PM +0200, Ricard Wanderlof wrote:
> 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.

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
David Woodhouse
2013-09-12 12:02:03 UTC
Permalink
On Thu, 2013-09-12 at 12:48 +0100, Mark Brown wrote:
> On Thu, Sep 12, 2013 at 12:17:45PM +0100, David Woodhouse wrote:
>
> > 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.
>
> So, to repeat my earlier question can someone tell me what a LUT is?

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
Mark Brown
2013-09-12 13:43:45 UTC
Permalink
On Thu, Sep 12, 2013 at 01:02:03PM +0100, David Woodhouse wrote:
> On Thu, 2013-09-12 at 12:48 +0100, Mark Brown wrote:

> > So, to repeat my earlier question can someone tell me what a LUT is?

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

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

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

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?
David Woodhouse
2013-09-12 14:03:16 UTC
Permalink
On Thu, 2013-09-12 at 14:43 +0100, Mark Brown wrote:
>
> > 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.
>
> OK, so the LUT itself isn't a generic mtd thing, though the commands
> it's apparently sending are?

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
Mark Brown
2013-09-12 14:40:02 UTC
Permalink
On Thu, Sep 12, 2013 at 03:03:16PM +0100, David Woodhouse wrote:

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

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.

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

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

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

...like you say some clarity would be good.
Huang Shijie
2013-09-13 03:14:47 UTC
Permalink
On Thu, Sep 12, 2013 at 03:03:16PM +0100, David Woodhouse wrote:
> On Thu, 2013-09-12 at 14:43 +0100, Mark Brown wrote:
> >
>
> 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.

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
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
David Woodhouse
2013-09-11 14:05:15 UTC
Permalink
On Wed, 2013-09-11 at 18:54 +0800, Huang Shijie wrote:
> 于 2013幎09月11日 18:41, Mark Brown 写道:
> > So why does the SPI driver have references to the opcodes in it?
> 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:

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
Mark Brown
2013-09-11 15:07:05 UTC
Permalink
On Wed, Sep 11, 2013 at 03:05:15PM +0100, David Woodhouse wrote:

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

Yeah, that's my question too...

> 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?

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.
David Woodhouse
2013-09-12 09:50:31 UTC
Permalink
On Thu, 2013-09-12 at 17:28 +0800, Huang Shijie wrote:
> 于 2013幎09月11日 22:05, David Woodhouse 写道:
> > What*actually* happens, on the wire(s), when the flash driver asks the
> > SPI controller to perform a transaction?
> The LUT registers tell the controller how many wires are needed for a
> transaction.
> For example, the read status only use a single line, while the quad read
> uses 4 lines.

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
Mark Brown
2013-09-12 10:19:35 UTC
Permalink
On Thu, Sep 12, 2013 at 10:50:31AM +0100, David Woodhouse wrote:

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

Note that spi_write_then_read() is just a helper for building up a spi
transfer in a common pattern.
Huang Shijie
2013-09-13 02:58:30 UTC
Permalink
On Thu, Sep 12, 2013 at 10:50:31AM +0100, David Woodhouse wrote:
> On Thu, 2013-09-12 at 17:28 +0800, Huang Shijie wrote:
> > =E4=BA=8E 2013=E5=B9=B409=E6=9C=8811=E6=97=A5 22:05, David Woodhous=
e =E5=86=99=E9=81=93:
> > > What*actually* happens, on the wire(s), when the flash driver as=
ks the
> > > SPI controller to perform a transaction?
> > The LUT registers tell the controller how many wires are needed for=
a=20
> > transaction.
> > For example, the read status only use a single line, while the quad=
read=20
> > uses 4 lines.
>=20
> Is this not something that could theoretically be provided by the cal=
ler
> when it *makes* the transaction?
>=20
> Conceptually speaking, could it not be an additional argument to
> spi_write_then_read() ?=20
>=20
> After all, it's the *device* driver (m25p80.c etc.) which will know w=
hat
> the transaction actually *is*, and how many lines the device will wan=
t
> to use for each transaction?
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


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
David Woodhouse
2013-09-12 15:22:02 UTC
Permalink
On Thu, 2013-09-12 at 22:58 -0400, Huang Shijie wrote:
>
> 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.

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
Huang Shijie
2013-09-13 04:12:14 UTC
Permalink
On Thu, Sep 12, 2013 at 04:22:02PM +0100, David Woodhouse wrote:
> On Thu, 2013-09-12 at 22:58 -0400, Huang Shijie wrote:
> >
> > 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.
>
> But the controller driver shouldn't *have* that incestuous knowledge of
> the command set of the chip that happens to be connected to it.

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

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







--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
David Woodhouse
2013-09-12 16:26:12 UTC
Permalink
On Fri, 2013-09-13 at 00:12 -0400, Huang Shijie wrote:
> I will send you the datasheet tomorrow.

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

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

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?

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

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?

> [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{}

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
Huang Shijie
2013-09-13 03:06:42 UTC
Permalink
=E4=BA=8E 2013=E5=B9=B409=E6=9C=8813=E6=97=A5 00:26, David Woodhouse =E5=
=86=99=E9=81=93:
> On Fri, 2013-09-13 at 00:12 -0400, Huang Shijie wrote:
>> I will send you the datasheet tomorrow.
> Thanks. Although according to the date header on your email, it is
> *already* tomorrow. Any chance of fixing your clock, please?
>
I was at home then. but now i am at office.
>> Mark and you want to create the LUT instruction sequence at the runt=
ime,
>> But there is some disadvantage if we do so:
>> [1] low efficiency:
>> If you want to change the LUT regitster, you should unlock th=
e LUT
>> register, and change the LUT regitsters, and lock the LUT
>> regitsters again.
> Although there aren't *that* many operations we perform, so you'd qui=
te
> quickly find yourself using things which are *already* programmed int=
o
> the LUT instead of having to re-program them again, surely?
>
If we can not parse out the SPI NOR commands, we can not quickly find t=
he
LUT which already programmed last time.
>> [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 Quad=
spi
>> controller only has a 64-byte TX-FIFO, so the controller shou=
ld
>> write a 64bytes firstly, then sends to the NOR with a read-st=
atus
>> 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.
> I'm not entirely sure I understand this. What *exactly* happens "on t=
he
> 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?
>
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.

>> [3] We may can not create the LUT instruction sequence at the run=
time,
>> 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 transactio=
n.
>>
>> We can not get the dummy info from the spi_transfer{}
> Again, what does that actually *mean*, on the wire?
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.

> I thought SPI was just 'send some bytes, fetch some bytes'. Why is th=
e
> controller doing anything other than that?
>
As the time goes on, the SPI devices or SPI controllers have become mor=
e=20
and more complicated.


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




thanks
Huang Shijie


--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Fabio Estevam
2013-09-12 16:27:12 UTC
Permalink
On Fri, Sep 13, 2013 at 1:12 AM, Huang Shijie <***@gmail.com> wrote:

> I will send you the datasheet tomorrow.

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)
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Huang Shijie
2013-09-13 02:21:43 UTC
Permalink
=E4=BA=8E 2013=E5=B9=B409=E6=9C=8813=E6=97=A5 00:27, Fabio Estevam =E5=86=
=99=E9=81=93:
> On Fri, Sep 13, 2013 at 1:12 AM, Huang Shijie<shijie8-***@public.gmane.org> wro=
te:
>
>> I will send you the datasheet tomorrow.
> 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)
>
Hi Fabio:

thanks a lot ! :)

Huang Shijie

--
To unsubscribe from this list: send the line "unsubscribe devicetree" i=
n
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
2013-09-12 20:56:44 UTC
Permalink
On Fri, Sep 13, 2013 at 12:12:14AM -0400, Huang Shijie wrote:
> On Thu, Sep 12, 2013 at 04:22:02PM +0100, David Woodhouse wrote:

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

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

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

> 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:

> [2] we may can not create all the LUT instruction sequence at the
> runtime. For example, the buffer program(OPCODE_PP):

>
> [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:

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.
Huang Shijie
2013-09-13 04:55:47 UTC
Permalink
=D3=DA 2013=C4=EA09=D4=C213=C8=D5 04:56, Mark Brown =D0=B4=B5=C0:
> On Fri, Sep 13, 2013 at 12:12:14AM -0400, Huang Shijie wrote:
>
>> I think the controller is designed for the NOR flash, yes, a little
>> strange.
> 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).
>
The QuadSpi is not a traditional SPI controller, but i think it is stil=
l
a SPI controller.
It connects and controls the SPI NOR FLASH, so what do you think it is?


>> Mark and you want to create the LUT instruction sequence at the runt=
ime,
>> But there is some disadvantage if we do so:
>> [1] low efficiency:=20
>> [2] we may can not create all the LUT instruction sequence at the
>> runtime. For example, the buffer program(OPCODE_PP):
>> [3] We may can not create the LUT instruction sequence at the runt=
ime,
>> since we can not get enough information from the spi_transfer{=
}.
>> A whole LUT instruction sequence may needs the following info:
> What this is saying to me is that you should not be impementing this =
as
If the quadspi controller is not a SPI controller, what controller is i=
t?


> a SPI controller, trying to do that is breaking the abstracton that S=
PI
> is offering. Like people have said SPI is just about byte streams.
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.



> 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.
If i follow this advice, i have to create a new file cloned by the m25p=
80.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



--
To unsubscribe from this list: send the line "unsubscribe devicetree" i=
n
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
2013-09-13 10:21:27 UTC
Permalink
On Fri, Sep 13, 2013 at 12:55:47PM +0800, Huang Shijie wrote:
> 于 2013幎09月13日 04:56, Mark Brown 写道:

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

> 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?

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

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

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

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

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

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.

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

You will need to create them.
Huang Shijie
2013-09-16 02:40:23 UTC
Permalink
=D3=DA 2013=C4=EA09=D4=C213=C8=D5 18:21, Mark Brown =D0=B4=B5=C0:
> You will need to create them.
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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" i=
n
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
2013-09-16 10:19:12 UTC
Permalink
On Mon, Sep 16, 2013 at 10:40:23AM +0800, Huang Shijie wrote:
> 于 2013幎09月13日 18:21, Mark Brown 写道:
> > You will need to create them.

> 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 ?

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.
Huang Shijie
2013-09-16 10:27:35 UTC
Permalink
=D3=DA 2013=C4=EA09=D4=C216=C8=D5 18:19, Mark Brown =D0=B4=B5=C0:
> 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 jus=
t
> get told that some transfers should use more data lines.
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 contro=
ller.

thanks
Huang Shijie

--
To unsubscribe from this list: send the line "unsubscribe devicetree" i=
n
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
2013-09-16 11:06:34 UTC
Permalink
On Mon, Sep 16, 2013 at 06:27:35PM +0800, Huang Shijie wrote:
> 于 2013幎09月16日 18:19, Mark Brown 写道:
> > 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.

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

I don't know what you mean by "the dummy".
Huang Shijie
2013-09-17 02:14:25 UTC
Permalink
=D3=DA 2013=C4=EA09=D4=C216=C8=D5 19:06, Mark Brown =D0=B4=B5=C0:
> I don't know what you mean by "the dummy".
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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" i=
n
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Mark Brown
2013-09-17 10:51:42 UTC
Permalink
On Tue, Sep 17, 2013 at 10:14:25AM +0800, Huang Shijie wrote:

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

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.
Gerhard Sittig
2013-09-17 15:05:48 UTC
Permalink
[ 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 ]

On Tue, Sep 17, 2013 at 11:51 +0100, Mark Brown wrote:
>
> On Tue, Sep 17, 2013 at 10:14:25AM +0800, Huang Shijie wrote:
>
> > The dummy is just a delay in a command sequence.
> > The dummy maybe only several clock cycles, less then 8 bit.
>
> 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.

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-***@public.gmane.org
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Gerhard Sittig
2013-09-17 16:49:35 UTC
Permalink
On Tue, Sep 17, 2013 at 17:05 +0200, Gerhard Sittig wrote:
>
> 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.

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

> The datasheet had "block diagrams" (section 3.16) [ ... ]
> And the datasheet had "sequence diagrams" (section 4.2.1) [ ... ]


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: ***@denx.de
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Sourav Poddar
2013-09-17 19:54:49 UTC
Permalink
Hi,
On Tuesday 17 September 2013 10:19 PM, Gerhard Sittig wrote:
> On Tue, Sep 17, 2013 at 17:05 +0200, Gerhard Sittig wrote:
>> 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.
> 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
>
>> The datasheet had "block diagrams" (section 3.16) [ ... ]
>> And the datasheet had "sequence diagrams" (section 4.2.1) [ ... ]
>
> 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
>
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 <***@ti.com>
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.

Signed-off-by: Sourav Poddar <***@ti.com>
---
drivers/mtd/devices/m25p80.c | 87
++++++++++++++++++++++++++++++++++++++----
1 files changed, 79 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index 26b14f9..fb9058d 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -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


> virtually yours
> Gerhard Sittig
Mark Brown
2013-09-17 19:13:19 UTC
Permalink
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...

> On Tue, Sep 17, 2013 at 11:51 +0100, Mark Brown wrote:

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

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

So just a normal transfer then...

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

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.

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

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.

> [ 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".

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

> - 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)

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.

> - 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)

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.

> [ potential "LUT" optimization, implementation details ]

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.

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

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.
David Woodhouse
2013-09-18 15:40:22 UTC
Permalink
On Tue, 2013-09-17 at 20:13 +0100, Mark Brown wrote:
>
> > [ potential "LUT" optimization, implementation details ]
>
> 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.

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-***@public.gmane.org Intel Corporation
Mark Brown
2013-09-18 16:00:24 UTC
Permalink
On Wed, Sep 18, 2013 at 10:40:22AM -0500, David Woodhouse wrote:
> On Tue, 2013-09-17 at 20:13 +0100, Mark Brown wrote:

> > > [ potential "LUT" optimization, implementation details ]

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

> 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?

Yeah, assuming it's not too annoying to identify the repeated patterns
and doesn't blow up horribly just caching should work fine.
Loading...