[PATCH 0/5] Add memory mapped read support for ti-qspi

[PATCH 0/5] Add memory mapped read support for ti-qspi

[Vignesh R]

Hi,

This patch series adds support for memory mapped read port of ti-qspi.
ti-qspi has a special memory mapped port through which SPI flash
memories can be accessed directly via SoC specific memory region.

First patch adds a method to pass flash specific information like read
opcode, dummy bytes etc and to request mmap read. Second patch
implements mmap read method in ti-qspi driver. Patch 3 adapts m25p80 to
use mmap read method before trying normal SPI transfer. Patch 4 and 5
add memory map region DT entries for DRA7xx and AM43xx SoCs.

This patch series is based on the discussions here:
http://www.spinics.net/lists/linux-spi/msg04796.html

Tested on DRA74 EVM and AM437x-SK.
Read performance increases from ~100kB/s to ~2.5MB/s.


Vignesh R (5):
  spi: introduce mmap read support for spi flash devices
  spi: spi-ti-qspi: add mmap mode read support
  mtd: devices: m25p80: add support for mmap read request
  ARM: dts: DRA7: add entry for qspi mmap region
  ARM: dts: AM4372: add entry for qspi mmap region

 Documentation/devicetree/bindings/spi/ti_qspi.txt |  18 +++-
 arch/arm/boot/dts/am4372.dtsi                     |   4 +-
 arch/arm/boot/dts/dra7.dtsi                       |   6 +-
 drivers/mtd/devices/m25p80.c                      |   8 ++
 drivers/spi/spi-ti-qspi.c                         | 106 +++++++++++++++++++++-
 include/linux/spi/spi.h                           |  21 +++++
 6 files changed, 154 insertions(+), 9 deletions(-)

-- 
2.5.1

[PATCH 1/5] spi: introduce mmap read support for spi flash devices

[Vignesh R]

In addition to providing direct access to SPI bus, some spi controller
hardwares (like ti-qspi) provide special memory mapped port
to accesses SPI flash devices in order to increase read performance.
This means the controller can automatically send the SPI signals
required to read data from the SPI flash device.
For this, spi controller needs to know flash specific information like
read command to use, dummy bytes and address width. Once these settings
are populated in hardware registers, any read accesses to flash's memory
map region(SoC specific) through memcpy or mem-to-mem DMA copy will be
handled by controller hardware. The hardware will automatically generate
spi signals required to read data from flash and present it to CPU or
DMA engine.

Introduce spi_mtd_mmap_read() method to support memory mapped read
over SPI flash devices. SPI master drivers can implement this method to
support memory mapped read interfaces. m25p80 flash driver and other
flash drivers can call this to request memory mapped read. The interface
should only be used mtd flashes and cannot be used with other spi devices.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 include/linux/spi/spi.h | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index d673072346f2..b74a3f169fc2 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -293,6 +293,23 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
  * @handle_err: the subsystem calls the driver to handle an error that occurs
  *		in the generic implementation of transfer_one_message().
  * @unprepare_message: undo any work done by prepare_message().
+ * @spi_mtd_mmap_read: some spi-controller hardwares provide memory
+ *                     mapped interface to communicate with mtd flashes.
+ *                     For this, spi  controller needs to know flash
+ *                     memory settings like read command to use, dummy
+ *                     bytes and address width. Once these settings are
+ *                     populated in hardware registers, any read
+ *                     accesses to flash's memory map region(as defined
+ *                     by SoC) through memcpy or mem-to-mem DMA copy
+ *                     will be handled by controller hardware. The
+ *                     hardware will automatically generate spi signals
+ *                     required to read data from flash and present it
+ *                     to CPU or DMA. SPI master drivers can use this
+ *                     callback to implement memory mapped read
+ *                     interface. Flash driver (like m25p80) requests
+ *                     memory mapped read via this method. The interface
+ *                     should  only be used mtd flashes and cannot be
+ *                     used with other spi devices.
  * @cs_gpios: Array of GPIOs to use as chip select lines; one per CS
  *	number. Any individual value may be -ENOENT for CS lines that
  *	are not GPIOs (driven by the SPI controller itself).
@@ -438,6 +455,10 @@ struct spi_master {
 			       struct spi_message *message);
 	int (*unprepare_message)(struct spi_master *master,
 				 struct spi_message *message);
+	int (*spi_mtd_mmap_read)(struct  spi_device *spi,
+				 loff_t from, size_t len, size_t *retlen,
+				 u_char *buf, u8 read_opcode,
+				 u8 addr_width, u8 dummy_bytes);
 
 	/*
 	 * These hooks are for drivers that use a generic implementation
-- 
2.5.1

[PATCH 2/5] spi: spi-ti-qspi: add mmap mode read support

[Vignesh R]

ti-qspi controller provides mmap port to read data from SPI flashes.
mmap port is enabled in QSPI_SPI_SWITCH_REG (ctrl module bits may
also need to be updated for some SoCs). The QSPI_SPI_SETUP_REGx needs to
be populated with flash specific information like read opcode, read
mode(quad, dual, normal), address width and dummy bytes. Once,
controller is in mmap mode, the whole flash memory is available as a
memory region at SoC specific address. This region can be accessed using
normal memcpy() or mem-to-mem dma copy. The ti-qspi controller hardware
will internally communicate with SPI flash over SPI bus and get the
requested data.

Implement spi_mtd_mmap_read() method to support mmap read over SPI
flash devices. With this, the read throughput increases from ~100kB/s to
~2.5 MB/s.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 drivers/spi/spi-ti-qspi.c | 106 ++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 102 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
index aa6d284131e0..a07610b84bc9 100644
--- a/drivers/spi/spi-ti-qspi.c
+++ b/drivers/spi/spi-ti-qspi.c
@@ -71,11 +71,8 @@ struct ti_qspi {
 #define QSPI_SPI_CMD_REG		(0x48)
 #define QSPI_SPI_STATUS_REG		(0x4c)
 #define QSPI_SPI_DATA_REG		(0x50)
-#define QSPI_SPI_SETUP0_REG		(0x54)
+#define QSPI_SPI_SETUP_REG(n)		((0x54 + 4 * n))
 #define QSPI_SPI_SWITCH_REG		(0x64)
-#define QSPI_SPI_SETUP1_REG		(0x58)
-#define QSPI_SPI_SETUP2_REG		(0x5c)
-#define QSPI_SPI_SETUP3_REG		(0x60)
 #define QSPI_SPI_DATA_REG_1		(0x68)
 #define QSPI_SPI_DATA_REG_2		(0x6c)
 #define QSPI_SPI_DATA_REG_3		(0x70)
@@ -120,6 +117,16 @@ struct ti_qspi {
 
 #define QSPI_AUTOSUSPEND_TIMEOUT         2000
 
+#define MEM_CS_EN(n)			((n + 1) << 8)
+
+#define MM_SWITCH			0x1
+
+#define QSPI_SETUP_RD_NORMAL		(0x0 << 12)
+#define QSPI_SETUP_RD_DUAL		(0x1 << 12)
+#define QSPI_SETUP_RD_QUAD		(0x3 << 12)
+#define QSPI_SETUP_ADDR_SHIFT		8
+#define QSPI_SETUP_DUMMY_SHIFT		10
+
 static inline unsigned long ti_qspi_read(struct ti_qspi *qspi,
 		unsigned long reg)
 {
@@ -361,6 +368,96 @@ static int qspi_transfer_msg(struct ti_qspi *qspi, struct spi_transfer *t)
 	return 0;
 }
 
+static void ti_qspi_enable_memory_map(struct spi_device *spi)
+{
+	struct ti_qspi  *qspi = spi_master_get_devdata(spi->master);
+	u32 val;
+
+	ti_qspi_write(qspi, MM_SWITCH, QSPI_SPI_SWITCH_REG);
+	if (qspi->ctrl_mod) {
+		val = readl(qspi->ctrl_base);
+		val |= MEM_CS_EN(spi->chip_select);
+		writel(val, qspi->ctrl_base);
+	}
+}
+
+static void ti_qspi_disable_memory_map(struct spi_device *spi)
+{
+	struct ti_qspi  *qspi = spi_master_get_devdata(spi->master);
+	u32 val;
+
+	ti_qspi_write(qspi, 0, QSPI_SPI_SWITCH_REG);
+	if (qspi->ctrl_mod) {
+		val = readl(qspi->ctrl_base);
+		val &= ~MEM_CS_EN(spi->chip_select);
+		writel(val, qspi->ctrl_base);
+	}
+}
+
+static void ti_qspi_setup_mmap_read(struct spi_device *spi,
+				    u8 read_opcode, u8 addr_width,
+				    u8 dummy_bytes)
+{
+	struct ti_qspi  *qspi = spi_master_get_devdata(spi->master);
+	u32 mode = spi->mode & (SPI_RX_DUAL | SPI_RX_QUAD);
+	u32 memval = read_opcode;
+
+	switch (mode) {
+	case SPI_RX_QUAD:
+		memval |= QSPI_SETUP_RD_QUAD;
+		break;
+	case SPI_RX_DUAL:
+		memval |= QSPI_SETUP_RD_DUAL;
+		break;
+	default:
+		memval |= QSPI_SETUP_RD_NORMAL;
+		break;
+	}
+	memval |= ((addr_width - 1) << QSPI_SETUP_ADDR_SHIFT |
+		   dummy_bytes << QSPI_SETUP_DUMMY_SHIFT);
+	ti_qspi_write(qspi, memval,
+		      QSPI_SPI_SETUP_REG(spi->chip_select));
+}
+
+static int ti_qspi_spi_mtd_mmap_read(struct  spi_device *spi,
+				     loff_t from, size_t len,
+				     size_t *retlen, u_char *buf,
+				     u8 read_opcode, u8 addr_width,
+				     u8 dummy_bytes)
+{
+	struct ti_qspi *qspi = spi_master_get_devdata(spi->master);
+	int ret = 0;
+
+	spi_bus_lock(qspi->master);
+	mutex_lock(&qspi->list_lock);
+	ret = pm_runtime_get_sync(qspi->dev);
+	if (ret < 0) {
+		dev_err(qspi->dev, "pm_runtime_get_sync() failed\n");
+		return ret;
+	}
+
+	/* disable WC interrupt during memcpy */
+	ti_qspi_write(qspi, QSPI_WC_INT_DISABLE, QSPI_INTR_ENABLE_CLEAR_REG);
+	ti_qspi_enable_memory_map(spi);
+	ti_qspi_setup_mmap_read(spi, read_opcode, addr_width,
+				dummy_bytes);
+
+	if (qspi_is_busy(qspi)) {
+		ret = -EBUSY;
+		goto err;
+	}
+
+	memcpy(buf, (__force void *)(qspi->mmap_base + from), len);
+	*retlen = len;
+
+err:
+	ti_qspi_disable_memory_map(spi);
+	pm_runtime_put(qspi->dev);
+	mutex_unlock(&qspi->list_lock);
+	spi_bus_unlock(qspi->master);
+	return ret;
+}
+
 static int ti_qspi_start_transfer_one(struct spi_master *master,
 		struct spi_message *m)
 {
@@ -479,6 +576,7 @@ static int ti_qspi_probe(struct platform_device *pdev)
 	master->setup = ti_qspi_setup;
 	master->auto_runtime_pm = true;
 	master->transfer_one_message = ti_qspi_start_transfer_one;
+	master->spi_mtd_mmap_read = ti_qspi_spi_mtd_mmap_read;
 	master->dev.of_node = pdev->dev.of_node;
 	master->bits_per_word_mask = SPI_BPW_MASK(32) | SPI_BPW_MASK(16) |
 				     SPI_BPW_MASK(8);
-- 
2.5.1

[PATCH 3/5] mtd: devices: m25p80: add support for mmap read request

[Vignesh R]

Certain spi controllers may support memory mapped interface to read from
m25p80 type flash devices. This interface provides better read
performance than regular SPI interface.
Call spi_mtd_mmap_read() function, if available, to make use of
memory-mapped interface.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 drivers/mtd/devices/m25p80.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index d313f948b96c..b8b391aab331 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -133,6 +133,14 @@ static int m25p80_read(struct spi_nor *nor, loff_t from, size_t len,
 	/* convert the dummy cycles to the number of bytes */
 	dummy /= 8;
 
+	if (spi->master->spi_mtd_mmap_read) {
+		return  spi->master->spi_mtd_mmap_read(spi, from, len,
+						       retlen, buf,
+						       nor->read_opcode,
+						       nor->addr_width,
+						       dummy);
+	}
+
 	spi_message_init(&m);
 	memset(t, 0, (sizeof t));
 
-- 
2.5.1

[PATCH 4/5] ARM: dts: DRA7: add entry for qspi mmap region

[Vignesh R]

Add qspi memory mapped region entries for DRA7xx based SoCs. Also,
update the binding documents for the controller to document this change.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 Documentation/devicetree/bindings/spi/ti_qspi.txt | 13 +++++++++++++
 arch/arm/boot/dts/dra7.dtsi                       |  6 ++++--
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/ti_qspi.txt b/Documentation/devicetree/bindings/spi/ti_qspi.txt
index 601a360531a5..f05dd631bef1 100644
--- a/Documentation/devicetree/bindings/spi/ti_qspi.txt
+++ b/Documentation/devicetree/bindings/spi/ti_qspi.txt
@@ -26,3 +26,16 @@ qspi: qspi@4b300000 {
 	spi-max-frequency = <25000000>;
 	ti,hwmods = "qspi";
 };
+
+For dra7xx:
+qspi: qspi@4b300000 {
+	compatible = "ti,dra7xxx-qspi";
+	reg = <0x4b300000 0x100>, <0x4a002558 0x4>,
+	      <0x5c000000 0x4000000>;
+	reg-names = "qspi_base", "qspi_ctrlmod",
+		    "qspi_mmap";
+	#address-cells = <1>;
+	#size-cells = <0>;
+	spi-max-frequency = <48000000>;
+	ti,hwmods = "qspi";
+};
diff --git a/arch/arm/boot/dts/dra7.dtsi b/arch/arm/boot/dts/dra7.dtsi
index 1e29ccf77ea2..f6798d6ecd60 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -1103,8 +1103,10 @@
 
 		qspi: qspi@4b300000 {
 			compatible = "ti,dra7xxx-qspi";
-			reg = <0x4b300000 0x100>;
-			reg-names = "qspi_base";
+			reg = <0x4b300000 0x100>, <0x4a002558 0x4>,
+			      <0x5c000000 0x4000000>;
+			reg-names = "qspi_base", "qspi_ctrlmod",
+				    "qspi_mmap";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			ti,hwmods = "qspi";
-- 
2.5.1

[PATCH 5/5] ARM: dts: AM4372: add entry for qspi mmap region

[Vignesh R]

Add qspi memory mapped region entries for AM43xx based SoCs. Also,
update the binding documents for the controller to document this change.

Signed-off-by: Vignesh R <vigneshr@ti.com>
---
 Documentation/devicetree/bindings/spi/ti_qspi.txt | 5 +++--
 arch/arm/boot/dts/am4372.dtsi                     | 4 +++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/ti_qspi.txt b/Documentation/devicetree/bindings/spi/ti_qspi.txt
index f05dd631bef1..05488970060b 100644
--- a/Documentation/devicetree/bindings/spi/ti_qspi.txt
+++ b/Documentation/devicetree/bindings/spi/ti_qspi.txt
@@ -17,9 +17,10 @@ Recommended properties:
 
 Example:
 
+For am4372:
 qspi: qspi@4b300000 {
-	compatible = "ti,dra7xxx-qspi";
-	reg = <0x47900000 0x100>, <0x30000000 0x3ffffff>;
+	compatible = "ti,am4372-qspi";
+	reg = <0x47900000 0x100>, <0x30000000 0x4000000>;
 	reg-names = "qspi_base", "qspi_mmap";
 	#address-cells = <1>;
 	#size-cells = <0>;
diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
index ade28c790f4b..52cf4846b8e1 100644
--- a/arch/arm/boot/dts/am4372.dtsi
+++ b/arch/arm/boot/dts/am4372.dtsi
@@ -902,7 +902,9 @@
 
 		qspi: qspi@47900000 {
 			compatible = "ti,am4372-qspi";
-			reg = <0x47900000 0x100>;
+			reg = <0x47900000 0x100>,
+			      <0x30000000 0x4000000>;
+			reg-names = "qspi_base", "qspi_mmap";
 			#address-cells = <1>;
 			#size-cells = <0>;
 			ti,hwmods = "qspi";
-- 
2.5.1

Re: [PATCH 1/5] spi: introduce mmap read support for spi flash devices

[Jagan Teki]

On 4 September 2015 at 13:59, Vignesh R <vigneshr@ti.com> wrote:
> In addition to providing direct access to SPI bus, some spi controller
> hardwares (like ti-qspi) provide special memory mapped port
> to accesses SPI flash devices in order to increase read performance.
> This means the controller can automatically send the SPI signals
> required to read data from the SPI flash device.
> For this, spi controller needs to know flash specific information like
> read command to use, dummy bytes and address width. Once these settings
> are populated in hardware registers, any read accesses to flash's memory
> map region(SoC specific) through memcpy or mem-to-mem DMA copy will be
> handled by controller hardware. The hardware will automatically generate
> spi signals required to read data from flash and present it to CPU or
> DMA engine.
>
> Introduce spi_mtd_mmap_read() method to support memory mapped read
> over SPI flash devices. SPI master drivers can implement this method to
> support memory mapped read interfaces. m25p80 flash driver and other
> flash drivers can call this to request memory mapped read. The interface
> should only be used mtd flashes and cannot be used with other spi devices.
>
> Signed-off-by: Vignesh R <vigneshr@ti.com>
> ---
>  include/linux/spi/spi.h | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index d673072346f2..b74a3f169fc2 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -293,6 +293,23 @@ static inline void spi_unregister_driver(struct spi_driver *sdrv)
>   * @handle_err: the subsystem calls the driver to handle an error that occurs
>   *             in the generic implementation of transfer_one_message().
>   * @unprepare_message: undo any work done by prepare_message().
> + * @spi_mtd_mmap_read: some spi-controller hardwares provide memory
> + *                     mapped interface to communicate with mtd flashes.
> + *                     For this, spi  controller needs to know flash
> + *                     memory settings like read command to use, dummy
> + *                     bytes and address width. Once these settings are
> + *                     populated in hardware registers, any read
> + *                     accesses to flash's memory map region(as defined
> + *                     by SoC) through memcpy or mem-to-mem DMA copy
> + *                     will be handled by controller hardware. The
> + *                     hardware will automatically generate spi signals
> + *                     required to read data from flash and present it
> + *                     to CPU or DMA. SPI master drivers can use this
> + *                     callback to implement memory mapped read
> + *                     interface. Flash driver (like m25p80) requests
> + *                     memory mapped read via this method. The interface
> + *                     should  only be used mtd flashes and cannot be
> + *                     used with other spi devices.
>   * @cs_gpios: Array of GPIOs to use as chip select lines; one per CS
>   *     number. Any individual value may be -ENOENT for CS lines that
>   *     are not GPIOs (driven by the SPI controller itself).
> @@ -438,6 +455,10 @@ struct spi_master {
>                                struct spi_message *message);
>         int (*unprepare_message)(struct spi_master *master,
>                                  struct spi_message *message);
> +       int (*spi_mtd_mmap_read)(struct  spi_device *spi,
> +                                loff_t from, size_t len, size_t *retlen,
> +                                u_char *buf, u8 read_opcode,
> +                                u8 addr_width, u8 dummy_bytes);

This looks un-manageable to know spi core or master knows what are the
command or opcode used by spi-nor flash and also I think mmap support
seems to be flash related or any justification this is spi bus
master/controller feature?

thanks!
-- 
Jagan | openedev.

Re: [PATCH 2/5] spi: spi-ti-qspi: add mmap mode read support

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1529 bytes --]

On Fri, Sep 04, 2015 at 01:59:59PM +0530, Vignesh R wrote:

> +static int ti_qspi_spi_mtd_mmap_read(struct  spi_device *spi,
> +				     loff_t from, size_t len,
> +				     size_t *retlen, u_char *buf,
> +				     u8 read_opcode, u8 addr_width,
> +				     u8 dummy_bytes)
> +{
> +	struct ti_qspi *qspi = spi_master_get_devdata(spi->master);
> +	int ret = 0;
> +
> +	spi_bus_lock(qspi->master);

I suspect I'm going to see the answer to this in another patch but the
fact that we're having to take this lock in a driver when it's an op the
core should be calling.

> +	ret = pm_runtime_get_sync(qspi->dev);
> +	if (ret < 0) {
> +		dev_err(qspi->dev, "pm_runtime_get_sync() failed\n");
> +		return ret;
> +	}

This would be better outside the lock, there's no need to have the lock
before we power on and this fixes the fact that you don't release the
lock here.

> +	memcpy(buf, (__force void *)(qspi->mmap_base + from), len);

The fact that you're having to cast here should be a warning that
there's someting wrong here.  I think you're looking for
memcpy_fromio().

> @@ -479,6 +576,7 @@ static int ti_qspi_probe(struct platform_device *pdev)
>  	master->setup = ti_qspi_setup;
>  	master->auto_runtime_pm = true;
>  	master->transfer_one_message = ti_qspi_start_transfer_one;
> +	master->spi_mtd_mmap_read = ti_qspi_spi_mtd_mmap_read;
>  	master->dev.of_node = pdev->dev.of_node;
>  	master->bits_per_word_mask = SPI_BPW_MASK(32) | SPI_BPW_MASK(16) |
>  				     SPI_BPW_MASK(8);

Don't we need to map a resource somewhere?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 3/5] mtd: devices: m25p80: add support for mmap read request

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 394 bytes --]

On Fri, Sep 04, 2015 at 02:00:00PM +0530, Vignesh R wrote:

> +	if (spi->master->spi_mtd_mmap_read) {
> +		return  spi->master->spi_mtd_mmap_read(spi, from, len,
> +						       retlen, buf,
> +						       nor->read_opcode,
> +						       nor->addr_width,
> +						       dummy);
> +	}

We should be calling some API provided by the SPI core here, not peering
directly into the ops struture.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/5] spi: introduce mmap read support for spi flash devices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2215 bytes --]

On Fri, Sep 04, 2015 at 04:55:33PM +0530, Jagan Teki wrote:
> On 4 September 2015 at 13:59, Vignesh R <vigneshr@ti.com> wrote:

> > + * @spi_mtd_mmap_read: some spi-controller hardwares provide memory
> > + *                     mapped interface to communicate with mtd flashes.
> > + *                     For this, spi  controller needs to know flash
> > + *                     memory settings like read command to use, dummy
> > + *                     bytes and address width. Once these settings are
> > + *                     populated in hardware registers, any read
> > + *                     accesses to flash's memory map region(as defined
> > + *                     by SoC) through memcpy or mem-to-mem DMA copy
> > + *                     will be handled by controller hardware. The
> > + *                     hardware will automatically generate spi signals
> > + *                     required to read data from flash and present it
> > + *                     to CPU or DMA. SPI master drivers can use this
> > + *                     callback to implement memory mapped read
> > + *                     interface. Flash driver (like m25p80) requests
> > + *                     memory mapped read via this method. The interface
> > + *                     should  only be used mtd flashes and cannot be
> > + *                     used with other spi devices.

This comment is *way* too verbose - probably you just need up to the
"Once" here.

> > +       int (*spi_mtd_mmap_read)(struct  spi_device *spi,
> > +                                loff_t from, size_t len, size_t *retlen,
> > +                                u_char *buf, u8 read_opcode,
> > +                                u8 addr_width, u8 dummy_bytes);

> This looks un-manageable to know spi core or master knows what are the
> command or opcode used by spi-nor flash and also I think mmap support
> seems to be flash related or any justification this is spi bus
> master/controller feature?

There seem to be a reasonable number of SPI controllers out there which
have as an extension the ability to do memory mapped reads but are
otherwise perfectly normal SPI controllers and which rely on that for
everything except reads.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/5] spi: introduce mmap read support for spi flash devices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 618 bytes --]

On Fri, Sep 04, 2015 at 01:59:58PM +0530, Vignesh R wrote:
> In addition to providing direct access to SPI bus, some spi controller
> hardwares (like ti-qspi) provide special memory mapped port
> to accesses SPI flash devices in order to increase read performance.
> This means the controller can automatically send the SPI signals
> required to read data from the SPI flash device.

Sorry, also meant to say here: as I kind of indicated in response to the
flash patch I'd expect to see the SPI core know something about this and
export an API for this which is integrated with things like the existing
message queue.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/5] spi: introduce mmap read support for spi flash devices

[Vignesh R]

On 09/15/2015 12:05 AM, Mark Brown wrote:
> On Fri, Sep 04, 2015 at 04:55:33PM +0530, Jagan Teki wrote:
>> On 4 September 2015 at 13:59, Vignesh R <vigneshr@ti.com> wrote:
> 
>>> + * @spi_mtd_mmap_read: some spi-controller hardwares provide memory
>>> + *                     mapped interface to communicate with mtd flashes.
>>> + *                     For this, spi  controller needs to know flash
>>> + *                     memory settings like read command to use, dummy
>>> + *                     bytes and address width. Once these settings are
>>> + *                     populated in hardware registers, any read
>>> + *                     accesses to flash's memory map region(as defined
>>> + *                     by SoC) through memcpy or mem-to-mem DMA copy
>>> + *                     will be handled by controller hardware. The
>>> + *                     hardware will automatically generate spi signals
>>> + *                     required to read data from flash and present it
>>> + *                     to CPU or DMA. SPI master drivers can use this
>>> + *                     callback to implement memory mapped read
>>> + *                     interface. Flash driver (like m25p80) requests
>>> + *                     memory mapped read via this method. The interface
>>> + *                     should  only be used mtd flashes and cannot be
>>> + *                     used with other spi devices.
> 
> This comment is *way* too verbose - probably you just need up to the
> "Once" here.
> 

Ok, I will move the extra text to commit log.


-- 
Thanks,
Vignesh

Re: [PATCH 3/5] mtd: devices: m25p80: add support for mmap read request

[Vignesh R]

On 09/14/2015 11:57 PM, Mark Brown wrote:
> On Fri, Sep 04, 2015 at 02:00:00PM +0530, Vignesh R wrote:
> 
>> +	if (spi->master->spi_mtd_mmap_read) {
>> +		return  spi->master->spi_mtd_mmap_read(spi, from, len,
>> +						       retlen, buf,
>> +						       nor->read_opcode,
>> +						       nor->addr_width,
>> +						       dummy);
>> +	}
> 
> We should be calling some API provided by the SPI core here, not peering
> directly into the ops struture.
> 

Ok..
-- 
Regards
Vignesh

Re: [PATCH 2/5] spi: spi-ti-qspi: add mmap mode read support

[Vignesh R]

On 09/14/2015 11:56 PM, Mark Brown wrote:
> On Fri, Sep 04, 2015 at 01:59:59PM +0530, Vignesh R wrote:
> 
>> +static int ti_qspi_spi_mtd_mmap_read(struct  spi_device *spi,
>> +				     loff_t from, size_t len,
>> +				     size_t *retlen, u_char *buf,
>> +				     u8 read_opcode, u8 addr_width,
>> +				     u8 dummy_bytes)
>> +{
>> +	struct ti_qspi *qspi = spi_master_get_devdata(spi->master);
>> +	int ret = 0;
>> +
>> +	spi_bus_lock(qspi->master);
> 
> I suspect I'm going to see the answer to this in another patch but the
> fact that we're having to take this lock in a driver when it's an op the
> core should be calling.
>

Agree..

>> +	ret = pm_runtime_get_sync(qspi->dev);
>> +	if (ret < 0) {
>> +		dev_err(qspi->dev, "pm_runtime_get_sync() failed\n");
>> +		return ret;
>> +	}
> 
> This would be better outside the lock, there's no need to have the lock
> before we power on and this fixes the fact that you don't release the
> lock here.

Will take care of this in SPI core API

> 
>> +	memcpy(buf, (__force void *)(qspi->mmap_base + from), len);
> 
> The fact that you're having to cast here should be a warning that
> there's someting wrong here.  I think you're looking for
> memcpy_fromio().

Ok, will change to memcpy_fromio()

> 
>> @@ -479,6 +576,7 @@ static int ti_qspi_probe(struct platform_device *pdev)
>>  	master->setup = ti_qspi_setup;
>>  	master->auto_runtime_pm = true;
>>  	master->transfer_one_message = ti_qspi_start_transfer_one;
>> +	master->spi_mtd_mmap_read = ti_qspi_spi_mtd_mmap_read;
>>  	master->dev.of_node = pdev->dev.of_node;
>>  	master->bits_per_word_mask = SPI_BPW_MASK(32) | SPI_BPW_MASK(16) |
>>  				     SPI_BPW_MASK(8);
> 
> Don't we need to map a resource somewhere?
> 

The current driver code already does the resource mapping:

	res_mmap = platform_get_resource_byname(pdev,
			IORESOURCE_MEM, "qspi_mmap");


-- 
Thanks,
Vignesh

Re: [PATCH 1/5] spi: introduce mmap read support for spi flash devices

[Vignesh R]

On 09/15/2015 12:07 AM, Mark Brown wrote:
> On Fri, Sep 04, 2015 at 01:59:58PM +0530, Vignesh R wrote:
>> In addition to providing direct access to SPI bus, some spi controller
>> hardwares (like ti-qspi) provide special memory mapped port
>> to accesses SPI flash devices in order to increase read performance.
>> This means the controller can automatically send the SPI signals
>> required to read data from the SPI flash device.
> 
> Sorry, also meant to say here: as I kind of indicated in response to the
> flash patch I'd expect to see the SPI core know something about this and
> export an API for this which is integrated with things like the existing
> message queue.
> 


Adding an API to SPI core makes sense to me. This can take care of spi
bus locking and runtime pm.
But, I didn't get how to integrate with existing message queue. Memory
mapped read by-passes message queue of SPI core. Could you please
explain a bit more on integrating with message queue? Did you mean
locking the existing message queue when memory mapped read is being
requested?


Thanks,
Vignesh

Re: [PATCH 1/5] spi: introduce mmap read support for spi flash devices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 530 bytes --]

On Wed, Sep 16, 2015 at 03:38:09PM +0530, Vignesh R wrote:

> But, I didn't get how to integrate with existing message queue. Memory
> mapped read by-passes message queue of SPI core. Could you please
> explain a bit more on integrating with message queue? Did you mean
> locking the existing message queue when memory mapped read is being
> requested?

Yes, and also making sure that we don't everything gets processed in
order so memory mapped requests come after any commands needed to set
them up and don't starve other work.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/5] spi: introduce mmap read support for spi flash devices

[Jagan Teki]

On 15 September 2015 at 00:05, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Sep 04, 2015 at 04:55:33PM +0530, Jagan Teki wrote:
>> On 4 September 2015 at 13:59, Vignesh R <vigneshr@ti.com> wrote:
>
>> > + * @spi_mtd_mmap_read: some spi-controller hardwares provide memory
>> > + *                     mapped interface to communicate with mtd flashes.
>> > + *                     For this, spi  controller needs to know flash
>> > + *                     memory settings like read command to use, dummy
>> > + *                     bytes and address width. Once these settings are
>> > + *                     populated in hardware registers, any read
>> > + *                     accesses to flash's memory map region(as defined
>> > + *                     by SoC) through memcpy or mem-to-mem DMA copy
>> > + *                     will be handled by controller hardware. The
>> > + *                     hardware will automatically generate spi signals
>> > + *                     required to read data from flash and present it
>> > + *                     to CPU or DMA. SPI master drivers can use this
>> > + *                     callback to implement memory mapped read
>> > + *                     interface. Flash driver (like m25p80) requests
>> > + *                     memory mapped read via this method. The interface
>> > + *                     should  only be used mtd flashes and cannot be
>> > + *                     used with other spi devices.
>
> This comment is *way* too verbose - probably you just need up to the
> "Once" here.
>
>> > +       int (*spi_mtd_mmap_read)(struct  spi_device *spi,
>> > +                                loff_t from, size_t len, size_t *retlen,
>> > +                                u_char *buf, u8 read_opcode,
>> > +                                u8 addr_width, u8 dummy_bytes);
>
>> This looks un-manageable to know spi core or master knows what are the
>> command or opcode used by spi-nor flash and also I think mmap support
>> seems to be flash related or any justification this is spi bus
>> master/controller feature?
>
> There seem to be a reasonable number of SPI controllers out there which
> have as an extension the ability to do memory mapped reads but are
> otherwise perfectly normal SPI controllers and which rely on that for
> everything except reads.

This is true, but exposing direct spi-nor arguments or features like
read_opcode, addr_width or dummy_byte to spi layer isn't fine? because
for spi layer there is a spi to flash transition mtd m25p80.c is there
to handle is it?

thanks!
-- 
Jagan | openedev.

Re: [PATCH 1/5] spi: introduce mmap read support for spi flash devices

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 906 bytes --]

On Wed, Sep 16, 2015 at 06:16:55PM +0530, Jagan Teki wrote:
> On 15 September 2015 at 00:05, Mark Brown <broonie@kernel.org> wrote:

> > There seem to be a reasonable number of SPI controllers out there which
> > have as an extension the ability to do memory mapped reads but are
> > otherwise perfectly normal SPI controllers and which rely on that for
> > everything except reads.

> This is true, but exposing direct spi-nor arguments or features like
> read_opcode, addr_width or dummy_byte to spi layer isn't fine? because
> for spi layer there is a spi to flash transition mtd m25p80.c is there
> to handle is it?

The m25p80 driver is a generic, SPI level piece of code not a driver for
specific controller hardware.  The controller hardware drivers live in
the SPI subsystem.  In order to support this memory mapped feature we
need the controller drivers to expose an interface for controlling it.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/5] spi: introduce mmap read support for spi flash devices

[Michal Suchanek]

On 16 September 2015 at 14:46, Jagan Teki <jteki@openedev.com> wrote:
> On 15 September 2015 at 00:05, Mark Brown <broonie@kernel.org> wrote:
>> On Fri, Sep 04, 2015 at 04:55:33PM +0530, Jagan Teki wrote:
>>> On 4 September 2015 at 13:59, Vignesh R <vigneshr@ti.com> wrote:
>>
>>> > + * @spi_mtd_mmap_read: some spi-controller hardwares provide memory
>>> > + *                     mapped interface to communicate with mtd flashes.
>>> > + *                     For this, spi  controller needs to know flash
>>> > + *                     memory settings like read command to use, dummy
>>> > + *                     bytes and address width. Once these settings are
>>> > + *                     populated in hardware registers, any read
>>> > + *                     accesses to flash's memory map region(as defined
>>> > + *                     by SoC) through memcpy or mem-to-mem DMA copy
>>> > + *                     will be handled by controller hardware. The
>>> > + *                     hardware will automatically generate spi signals
>>> > + *                     required to read data from flash and present it
>>> > + *                     to CPU or DMA. SPI master drivers can use this
>>> > + *                     callback to implement memory mapped read
>>> > + *                     interface. Flash driver (like m25p80) requests
>>> > + *                     memory mapped read via this method. The interface
>>> > + *                     should  only be used mtd flashes and cannot be
>>> > + *                     used with other spi devices.
>>
>> This comment is *way* too verbose - probably you just need up to the
>> "Once" here.
>>
>>> > +       int (*spi_mtd_mmap_read)(struct  spi_device *spi,
>>> > +                                loff_t from, size_t len, size_t *retlen,
>>> > +                                u_char *buf, u8 read_opcode,
>>> > +                                u8 addr_width, u8 dummy_bytes);
>>
>>> This looks un-manageable to know spi core or master knows what are the
>>> command or opcode used by spi-nor flash and also I think mmap support
>>> seems to be flash related or any justification this is spi bus
>>> master/controller feature?
>>
>> There seem to be a reasonable number of SPI controllers out there which
>> have as an extension the ability to do memory mapped reads but are
>> otherwise perfectly normal SPI controllers and which rely on that for
>> everything except reads.
>
> This is true, but exposing direct spi-nor arguments or features like
> read_opcode, addr_width or dummy_byte to spi layer isn't fine? because
> for spi layer there is a spi to flash transition mtd m25p80.c is there
> to handle is it?

For the TI SPI controller this is programmable. So instead of
composing a SPI message that sends the opcode (and address and dummy
bytes) you set the opcode in a controller register and then perform
the memory read which sends the configured opcode to the flash memory.
So this needs to be exposed in an API which the m25p80 driver can use.

On 16 September 2015 at 12:08, Vignesh R <vigneshr@ti.com> wrote:
>
>
> On 09/15/2015 12:07 AM, Mark Brown wrote:
>> On Fri, Sep 04, 2015 at 01:59:58PM +0530, Vignesh R wrote:
>>> In addition to providing direct access to SPI bus, some spi controller
>>> hardwares (like ti-qspi) provide special memory mapped port
>>> to accesses SPI flash devices in order to increase read performance.
>>> This means the controller can automatically send the SPI signals
>>> required to read data from the SPI flash device.
>>
>> Sorry, also meant to say here: as I kind of indicated in response to the
>> flash patch I'd expect to see the SPI core know something about this and
>> export an API for this which is integrated with things like the existing
>> message queue.
>>
>
>
> Adding an API to SPI core makes sense to me. This can take care of spi
> bus locking and runtime pm.
> But, I didn't get how to integrate with existing message queue. Memory
> mapped read by-passes message queue of SPI core. Could you please
> explain a bit more on integrating with message queue? Did you mean
> locking the existing message queue when memory mapped read is being
> requested?

Presumably when you have a function in SPI core for this it takes the
same lock transfer_one does so either you do mmap transfer or normal
SPI transfer. You can probably manually sync on transfer completion in
a driver that uses both normal SPI messages and mmap transfer - which
m25p80 does anyway.

Thanks

Michal

Re: [PATCH 1/5] spi: introduce mmap read support for spi flash devices

[Vignesh R]

On 09/16/2015 04:26 PM, Mark Brown wrote:
> On Wed, Sep 16, 2015 at 03:38:09PM +0530, Vignesh R wrote:
> 
>> But, I didn't get how to integrate with existing message queue. Memory
>> mapped read by-passes message queue of SPI core. Could you please
>> explain a bit more on integrating with message queue? Did you mean
>> locking the existing message queue when memory mapped read is being
>> requested?
> 
> Yes, and also making sure that we don't everything gets processed in
> order so memory mapped requests come after any commands needed to set
> them up and don't starve other work.
> 

Ok, thanks for the clarification!

-- 
Regards
Vignesh