Changes since v2:
Remove mmap_lock_mutex.
Optimize enable/disable of mmap mode.
Changes since v1:
Introduce API in SPI core that MTD flash driver can call for mmap read
instead of directly calling spi-master driver callback. This API makes
sure that SPI core msg queue is locked during mmap transfers.
v1: https://lkml.org/lkml/2015/9/4/103
Cover letter:
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 | 19 ++++-
arch/arm/boot/dts/am4372.dtsi | 4 +-
arch/arm/boot/dts/dra7.dtsi | 7 +-
drivers/mtd/devices/m25p80.c | 5 ++
drivers/spi/spi-ti-qspi.c | 99 ++++++++++++++++++++++-
drivers/spi/spi.c | 34 ++++++++
include/linux/spi/spi.h | 20 +++++
7 files changed, 179 insertions(+), 9 deletions(-)
--
2.6.3
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/DMA.
Introduce spi_mtd_mmap_read() interface to support memory mapped read
over SPI flash devices. SPI master drivers can implement this callback 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 <[email protected]>
---
v3:
* Remove use of mmap_lock_mutex, use bus_lock_mutex instead.
drivers/spi/spi.c | 34 ++++++++++++++++++++++++++++++++++
include/linux/spi/spi.h | 20 ++++++++++++++++++++
2 files changed, 54 insertions(+)
diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index e2415be209d5..0448d29fefc8 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -1134,6 +1134,7 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread)
}
}
+ mutex_lock(&master->bus_lock_mutex);
trace_spi_message_start(master->cur_msg);
if (master->prepare_message) {
@@ -1143,6 +1144,7 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread)
"failed to prepare message: %d\n", ret);
master->cur_msg->status = ret;
spi_finalize_current_message(master);
+ mutex_unlock(&master->bus_lock_mutex);
return;
}
master->cur_msg_prepared = true;
@@ -1152,6 +1154,7 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread)
if (ret) {
master->cur_msg->status = ret;
spi_finalize_current_message(master);
+ mutex_unlock(&master->bus_lock_mutex);
return;
}
@@ -1159,8 +1162,10 @@ static void __spi_pump_messages(struct spi_master *master, bool in_kthread)
if (ret) {
dev_err(&master->dev,
"failed to transfer one message from queue\n");
+ mutex_unlock(&master->bus_lock_mutex);
return;
}
+ mutex_unlock(&master->bus_lock_mutex);
}
/**
@@ -2327,6 +2332,35 @@ int spi_async_locked(struct spi_device *spi, struct spi_message *message)
EXPORT_SYMBOL_GPL(spi_async_locked);
+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)
+
+{
+ struct spi_master *master = spi->master;
+ int ret;
+
+ if (master->auto_runtime_pm) {
+ ret = pm_runtime_get_sync(master->dev.parent);
+ if (ret < 0) {
+ dev_err(&master->dev, "Failed to power device: %d\n",
+ ret);
+ goto err;
+ }
+ }
+ mutex_lock(&master->bus_lock_mutex);
+ ret = master->spi_mtd_mmap_read(spi, from, len, retlen, buf,
+ read_opcode, addr_width,
+ dummy_bytes);
+ mutex_unlock(&master->bus_lock_mutex);
+ if (master->auto_runtime_pm)
+ pm_runtime_put(master->dev.parent);
+
+err:
+ return ret;
+}
+EXPORT_SYMBOL_GPL(spi_mtd_mmap_read);
+
/*-------------------------------------------------------------------------*/
/* Utility methods for SPI master protocol drivers, layered on
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index cce80e6dc7d1..2f2c431b8917 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -361,6 +361,11 @@ 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.
+ * Flash drivers (like m25p80) can request memory
+ * mapped read via this method. This interface
+ * should only be used by mtd flashes and cannot be
+ * used by 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).
@@ -507,6 +512,11 @@ 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
@@ -999,6 +1009,16 @@ static inline ssize_t spi_w8r16be(struct spi_device *spi, u8 cmd)
return be16_to_cpu(result);
}
+/* SPI core interface for memory mapped read support */
+static inline bool spi_mmap_read_supported(struct spi_device *spi)
+{
+ return spi->master->spi_mtd_mmap_read ? true : false;
+}
+
+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);
+
/*---------------------------------------------------------------------------*/
/*
--
2.6.3
ti-qspi controller provides mmap port to read data from SPI flashes.
mmap port is enabled in QSPI_SPI_SWITCH_REG. ctrl module register may
also need to be accessed 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() callback 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 <[email protected]>
---
v3:
* optimize enable/disable of mmap mode
drivers/spi/spi-ti-qspi.c | 99 +++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 95 insertions(+), 4 deletions(-)
diff --git a/drivers/spi/spi-ti-qspi.c b/drivers/spi/spi-ti-qspi.c
index 64318fcfacf2..295c11f48440 100644
--- a/drivers/spi/spi-ti-qspi.c
+++ b/drivers/spi/spi-ti-qspi.c
@@ -56,6 +56,7 @@ struct ti_qspi {
u32 dc;
bool ctrl_mod;
+ bool mmap_enabled;
};
#define QSPI_PID (0x0)
@@ -65,11 +66,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)
@@ -109,6 +107,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)
{
@@ -366,6 +374,84 @@ 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);
+ /* dummy readl to ensure bus sync */
+ readl(qspi->ctrl_base);
+ }
+ qspi->mmap_enabled = true;
+}
+
+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);
+ }
+ qspi->mmap_enabled = false;
+}
+
+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;
+
+ mutex_lock(&qspi->list_lock);
+
+ if (!qspi->mmap_enabled)
+ ti_qspi_enable_memory_map(spi);
+ ti_qspi_setup_mmap_read(spi, read_opcode, addr_width,
+ dummy_bytes);
+ memcpy_fromio(buf, qspi->mmap_base + from, len);
+ *retlen = len;
+
+ mutex_unlock(&qspi->list_lock);
+
+ return ret;
+}
+
static int ti_qspi_start_transfer_one(struct spi_master *master,
struct spi_message *m)
{
@@ -398,6 +484,9 @@ static int ti_qspi_start_transfer_one(struct spi_master *master,
mutex_lock(&qspi->list_lock);
+ if (qspi->mmap_enabled)
+ ti_qspi_disable_memory_map(spi);
+
list_for_each_entry(t, &m->transfers, transfer_list) {
qspi->cmd |= QSPI_WLEN(t->bits_per_word);
@@ -526,7 +615,9 @@ static int ti_qspi_probe(struct platform_device *pdev)
ret = PTR_ERR(qspi->mmap_base);
goto free_master;
}
+ master->spi_mtd_mmap_read = ti_qspi_spi_mtd_mmap_read;
}
+ qspi->mmap_enabled = false;
qspi->fclk = devm_clk_get(&pdev->dev, "fck");
if (IS_ERR(qspi->fclk)) {
--
2.6.3
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() interface, if supported, to make use of
memory-mapped interface.
Signed-off-by: Vignesh R <[email protected]>
---
drivers/mtd/devices/m25p80.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/mtd/devices/m25p80.c b/drivers/mtd/devices/m25p80.c
index fe9ceb7b5405..7ef0c5009ead 100644
--- a/drivers/mtd/devices/m25p80.c
+++ b/drivers/mtd/devices/m25p80.c
@@ -131,6 +131,11 @@ 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_mmap_read_supported(spi))
+ return 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.6.3
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 <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
Documentation/devicetree/bindings/spi/ti_qspi.txt | 14 ++++++++++++++
arch/arm/boot/dts/dra7.dtsi | 7 +++++--
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/spi/ti_qspi.txt b/Documentation/devicetree/bindings/spi/ti_qspi.txt
index 601a360531a5..334aa3f32cbc 100644
--- a/Documentation/devicetree/bindings/spi/ti_qspi.txt
+++ b/Documentation/devicetree/bindings/spi/ti_qspi.txt
@@ -26,3 +26,17 @@ qspi: qspi@4b300000 {
spi-max-frequency = <25000000>;
ti,hwmods = "qspi";
};
+
+For dra7xx:
+qspi: qspi@4b300000 {
+ compatible = "ti,dra7xxx-qspi";
+ reg = <0x4b300000 0x100>,
+ <0x5c000000 0x4000000>,
+ <0x4a002558 0x4>;
+ reg-names = "qspi_base", "qspi_mmap",
+ "qspi_ctrlmod";
+ #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 8fedddc35999..ad93fe2ccab8 100644
--- a/arch/arm/boot/dts/dra7.dtsi
+++ b/arch/arm/boot/dts/dra7.dtsi
@@ -1108,8 +1108,11 @@
qspi: qspi@4b300000 {
compatible = "ti,dra7xxx-qspi";
- reg = <0x4b300000 0x100>;
- reg-names = "qspi_base";
+ reg = <0x4b300000 0x100>,
+ <0x5c000000 0x4000000>,
+ <0x4a002558 0x4>;
+ reg-names = "qspi_base", "qspi_mmap",
+ "qspi_ctrlmod";
#address-cells = <1>;
#size-cells = <0>;
ti,hwmods = "qspi";
--
2.6.3
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 <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
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 334aa3f32cbc..5a1542eda387 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 d83ff9c9701e..e32d164102d1 100644
--- a/arch/arm/boot/dts/am4372.dtsi
+++ b/arch/arm/boot/dts/am4372.dtsi
@@ -963,7 +963,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.6.3
Hi Vignesh,
Sorry for the late review. I did not have time to review much back when
you submitted your first RFCs for this.
On Tue, Nov 10, 2015 at 10:59:55AM +0530, Vignesh R wrote:
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index cce80e6dc7d1..2f2c431b8917 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -361,6 +361,11 @@ 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.
> + * Flash drivers (like m25p80) can request memory
> + * mapped read via this method. This interface
> + * should only be used by mtd flashes and cannot be
> + * used by 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).
> @@ -507,6 +512,11 @@ 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);
Is this API really sufficient? There are actually quite a few other
flash-related parameters that might be relevant to a controller. I
presume you happen not hit them because of the particular cases you're
using this for right now, but:
* How many I/O lines are being used? These can vary depending on the
type of flash and the number of I/O lines supported by the controller
and connected on the board.
* The previous point can vary across parts of the message. There are
various combinations of 1/2/4 lines used for opcode/address/data. We
only support a few of those combinations in m25p80 right now, but
you're not specifying any of that in this API. I guess you're just
making assumptions? (BTW, I think there are others having problems
with the difference between different "quad" modes on Micron flash; I
haven't sorted through all the discussion there.)
There are typically both flash device and SPI controller constraints
on this question, so there needs to be some kind of negotiation
involved, I expect. Or at least, the SPI master needs to expose which
modes it can support with this flash-read API.
Also, this API doesn't actually have anything to do with memory mapping.
It has to do with the de facto standard flash protocol. So I don't think
mmap belongs in the name; it should be something about flash. (I know of
at least one other controller that could probably use this API, excpet
it doesn't use memory mapping to accomplish the accelerated flash read.)
>
> /*
> * These hooks are for drivers that use a generic implementation
Brian
Hi Brain,
On 11/11/2015 4:53 AM, Brian Norris wrote:
> Hi Vignesh,
>
> Sorry for the late review. I did not have time to review much back when
> you submitted your first RFCs for this.
>
> On Tue, Nov 10, 2015 at 10:59:55AM +0530, Vignesh R wrote:
>> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
>> index cce80e6dc7d1..2f2c431b8917 100644Hi
>> --- a/include/linux/spi/spi.h
>> +++ b/include/linux/spi/spi.h
>> @@ -361,6 +361,11 @@ 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.
>> + * Flash drivers (like m25p80) can request memory
>> + * mapped read via this method. This interface
>> + * should only be used by mtd flashes and cannot be
>> + * used by 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).
>> @@ -507,6 +512,11 @@ 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);
>
> Is this API really sufficient? There are actually quite a few other
> flash-related parameters that might be relevant to a controller. I
> presume you happen not hit them because of the particular cases you're
> using this for right now, but:
>
> * How many I/O lines are being used? These can vary depending on the
> type of flash and the number of I/O lines supported by the controller
> and connected on the board.
>
This API communicates whatever data is currently communicated via
spi_message through spi_sync/transfer_one interfaces.
> * The previous point can vary across parts of the message. There are
> various combinations of 1/2/4 lines used for opcode/address/data. We
> only support a few of those combinations in m25p80 right now, but
> you're not specifying any of that in this API. I guess you're just
> making assumptions? (BTW, I think there are others having problems
> with the difference between different "quad" modes on Micron flash; I
> haven't sorted through all the discussion there.)
>
How is the spi controller currently being made aware of this via
m25p80_read / spi_sync() interface? AFAIK, mode field of spi_device
struct tell whether to do normal/dual/quad read but there is no info
communicated wrt 1/2/4 opcode/address/data combinations. And there is no
info indicating capabilities of spi-master wrt no of IO lines for
opcode/address/data that it can support.
> There are typically both flash device and SPI controller constraints
> on this question, so there needs to be some kind of negotiation
> involved, I expect. Or at least, the SPI master needs to expose which
> modes it can support with this flash-read API.
>
If spi-master capabilities are known then spi_mmap_read_supported() (or
a new function perhaps) can be used for negotiation. These capabilities
can be added incrementally once ability to specify spi-master
capabilities are in place.
> Also, this API doesn't actually have anything to do with memory mapping.
> It has to do with the de facto standard flash protocol. So I don't think
> mmap belongs in the name; it should be something about flash. (I know of
> at least one other controller that could probably use this API, excpet
> it doesn't use memory mapping to accomplish the accelerated flash read.)
>
As far as TI QSPI controller is concerned, the accelerated read happens
via mmap port whereby a predefined memory address space of SoC is
exposed as QSPI mmap region. This region can be accessed like normal
RAM(via memcpy()) and the QSPI controller interface takes care of
fetching data from flash on SPI bus automatically hence, I named it as
above. But, I have no hard feelings if it needs to be generalized to
spi_mtd_read() or something else.
Regards
Vignesh
Hi,
On Wed, Nov 11, 2015 at 12:20:46PM +0530, R, Vignesh wrote:
> On 11/11/2015 4:53 AM, Brian Norris wrote:
> > On Tue, Nov 10, 2015 at 10:59:55AM +0530, Vignesh R wrote:
> >> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> >> index cce80e6dc7d1..2f2c431b8917 100644Hi
> >> --- a/include/linux/spi/spi.h
> >> +++ b/include/linux/spi/spi.h
> >> @@ -361,6 +361,11 @@ 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.
> >> + * Flash drivers (like m25p80) can request memory
> >> + * mapped read via this method. This interface
> >> + * should only be used by mtd flashes and cannot be
> >> + * used by 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).
> >> @@ -507,6 +512,11 @@ 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);
> >
> > Is this API really sufficient? There are actually quite a few other
> > flash-related parameters that might be relevant to a controller. I
> > presume you happen not hit them because of the particular cases you're
> > using this for right now, but:
> >
> > * How many I/O lines are being used? These can vary depending on the
> > type of flash and the number of I/O lines supported by the controller
> > and connected on the board.
> >
>
> This API communicates whatever data is currently communicated via
> spi_message through spi_sync/transfer_one interfaces.
No it doesn't. A spi_message consists of a list of spi_transfer's, and
each spi_transfer has tx_nbits and rx_nbits fields.
> > * The previous point can vary across parts of the message. There are
> > various combinations of 1/2/4 lines used for opcode/address/data. We
> > only support a few of those combinations in m25p80 right now, but
> > you're not specifying any of that in this API. I guess you're just
> > making assumptions? (BTW, I think there are others having problems
> > with the difference between different "quad" modes on Micron flash; I
> > haven't sorted through all the discussion there.)
> >
>
> How is the spi controller currently being made aware of this via
> m25p80_read / spi_sync() interface? AFAIK, mode field of spi_device
> struct tell whether to do normal/dual/quad read but there is no info
> communicated wrt 1/2/4 opcode/address/data combinations.
Yes there is. m25p80 fills out spi_transfer::rx_nbits. Currently, we
only use this for the data portion, but it's possible to support more
lines for the address and opcode portions too, using the rx_nbits for
the opcode and address spi_transfer struct(s) (currently, m25p80_read()
uses 2 spi_transfers per message, where the first one contains opcode +
address + dummy on a single line, and the second transfer receives the
data on 1, 2, or 4 lines).
> And there is no
> info indicating capabilities of spi-master wrt no of IO lines for
> opcode/address/data that it can support.
For a true SPI controller, there is no need to specify something
different for opcode/address/data, since all those are treated the same;
they're just bits on 1, 2, or 4 lines. So the SPI_{TX,RX}_{DUAL,QUAD}
mode flags in struct spi_master tell m25p80 all it needs to know.
> > There are typically both flash device and SPI controller constraints
> > on this question, so there needs to be some kind of negotiation
> > involved, I expect. Or at least, the SPI master needs to expose which
> > modes it can support with this flash-read API.
> >
>
> If spi-master capabilities are known
For generic SPI handling, these are already known. But now you're adding
flash-specific capabilities, and I'm not going to assume that all
accelerated-read (e.g., your TI mmap'ed flash read) support all the same
modes as your generic modes.
So, which modes does your mmap'ed read handle? 1/1/1? 1/1/2? 1/1/4?
4/4/4? (where x/y/z means x lines for opcode, y lines for address, and z
lines for data)
> then spi_mmap_read_supported() (or
> a new function perhaps) can be used for negotiation. These capabilities
> can be added incrementally once ability to specify spi-master
> capabilities are in place.
spi_master capabilities are already in place. So now you need to
describe them for your new interface too.
> > Also, this API doesn't actually have anything to do with memory mapping.
> > It has to do with the de facto standard flash protocol. So I don't think
> > mmap belongs in the name; it should be something about flash. (I know of
> > at least one other controller that could probably use this API, excpet
> > it doesn't use memory mapping to accomplish the accelerated flash read.)
> >
>
> As far as TI QSPI controller is concerned, the accelerated read happens
> via mmap port whereby a predefined memory address space of SoC is
> exposed as QSPI mmap region. This region can be accessed like normal
> RAM(via memcpy()) and the QSPI controller interface takes care of
> fetching data from flash on SPI bus automatically
I understand all that, but the API as it currently stands is not tied to
that implementation at all.
> hence, I named it as
> above. But, I have no hard feelings if it needs to be generalized to
> spi_mtd_read() or something else.
Maybe spi_flash_read()? It's technically not limited to MTD, though it
probably would only be used there. (And please, don't tread on the
'spi_nor_' prefix, as we already have a related, but distinct, framework
using that.)
Brian
In addition to my other comments:
On Tue, Nov 10, 2015 at 10:59:55AM +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.
> 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/DMA.
>
> Introduce spi_mtd_mmap_read() interface to support memory mapped read
> over SPI flash devices. SPI master drivers can implement this callback 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 <[email protected]>
> ---
...
> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
> index cce80e6dc7d1..2f2c431b8917 100644
> --- a/include/linux/spi/spi.h
> +++ b/include/linux/spi/spi.h
> @@ -361,6 +361,11 @@ 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.
> + * Flash drivers (like m25p80) can request memory
> + * mapped read via this method. This interface
> + * should only be used by mtd flashes and cannot be
> + * used by 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).
> @@ -507,6 +512,11 @@ 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 is seeming to be a longer and longer list of arguments. I know MTD
has a bad habit of long argument lists (which then cause a ton of
unnecessary churn when things need changed in the API), but perhaps we
can limit the damage to the SPI layer. Perhaps this deserves a struct to
encapsulate all the flash read arguments? Like:
struct spi_flash_read_message {
loff_t from;
size_t len;
size_t *retlen;
void *buf;
u8 read_opcode;
u8 addr_width;
u8 dummy_bits;
// additional fields to describe rx_nbits for opcode/addr/data
};
struct spi_master {
...
int (*spi_flash_read)(struct spi_device *spi,
struct spi_flash_message *msg);
};
>
> /*
> * These hooks are for drivers that use a generic implementation
...
Brian
Hi Brian,
On 11/12/2015 12:54 AM, Brian Norris wrote:
> In addition to my other comments:
>
[...]
>> + 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 is seeming to be a longer and longer list of arguments. I know MTD
> has a bad habit of long argument lists (which then cause a ton of
> unnecessary churn when things need changed in the API), but perhaps we
> can limit the damage to the SPI layer. Perhaps this deserves a struct to
> encapsulate all the flash read arguments? Like:
>
> struct spi_flash_read_message {
> loff_t from;
> size_t len;
> size_t *retlen;
> void *buf;
> u8 read_opcode;
> u8 addr_width;
> u8 dummy_bits;
> // additional fields to describe rx_nbits for opcode/addr/data
> };
>
> struct spi_master {
> ...
> int (*spi_flash_read)(struct spi_device *spi,
> struct spi_flash_message *msg);
> };
Yeah.. I think struct encapsulation helps, this can also be used to pass
sg lists for dma in future. I will rework the series with your
suggestion to include nbits for opcode/addr/data.
Also, will add validation logic (similar to __spi_validate()) to check
whether master supports dual/quad mode for opcode/addr/data. I am
planning to add this validation code to spi_flash_read_validate(in place
of spi_mmap_read_supported())
Thanks!
--
Regards
Vignesh
On 11-11-15 07:50, R, Vignesh wrote:
> Hi Brain,
>
> On 11/11/2015 4:53 AM, Brian Norris wrote:
>> Hi Vignesh,
...
>> Also, this API doesn't actually have anything to do with memory mapping.
>> It has to do with the de facto standard flash protocol. So I don't think
>> mmap belongs in the name; it should be something about flash. (I know of
>> at least one other controller that could probably use this API, excpet
>> it doesn't use memory mapping to accomplish the accelerated flash read.)
The Zynq has a similar way of accessing QSPI flash through a memory map. The
memory window is only 16MB, so some form of paging would be needed.
It's why I have been following this thread with great interest, since the QSPI
performance on the Zynq is way below what it could potentially be.
> As far as TI QSPI controller is concerned, the accelerated read happens
> via mmap port whereby a predefined memory address space of SoC is
> exposed as QSPI mmap region. This region can be accessed like normal
> RAM(via memcpy()) and the QSPI controller interface takes care of
> fetching data from flash on SPI bus automatically hence, I named it as
> above. But, I have no hard feelings if it needs to be generalized to
> spi_mtd_read() or something else.
I know that on the Zynq, you can even let the DMA controller access the QSPI
flash via this memory mapping. The QSPI controller itself doesn't support any
DMA at all.
If something similar applies to the TI platform (most of the TI procs have
nice DMA controllers) one could go one step further and implement a generic
DMA-through-mmap access to QSPI flash.
Mike.
Kind regards,
Mike Looijmans
System Expert
TOPIC Embedded Products
Eindhovenseweg 32-C, NL-5683 KH Best
Postbus 440, NL-5680 AK Best
Telefoon: +31 (0) 499 33 69 79
Telefax: +31 (0) 499 33 69 70
E-mail: [email protected]
Website: http://www.topicproducts.com
Please consider the environment before printing this e-mail
Visit us at : Aerospace Electrical Systems Expo Europe which will be held from 17.11.2015 till 19.11.2015, Findorffstrasse 101 Bremen, Germany, Hall 5, stand number C65
http://www.aesexpo.eu
Hi Brian,
Le 11/11/2015 08:20, Brian Norris a ?crit :
> Hi,
>
> On Wed, Nov 11, 2015 at 12:20:46PM +0530, R, Vignesh wrote:
>> On 11/11/2015 4:53 AM, Brian Norris wrote:
>>> On Tue, Nov 10, 2015 at 10:59:55AM +0530, Vignesh R wrote:
>>>> diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
>>>> index cce80e6dc7d1..2f2c431b8917 100644Hi
>>>> --- a/include/linux/spi/spi.h
>>>> +++ b/include/linux/spi/spi.h
>>>> @@ -361,6 +361,11 @@ 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.
>>>> + * Flash drivers (like m25p80) can request memory
>>>> + * mapped read via this method. This interface
>>>> + * should only be used by mtd flashes and cannot be
>>>> + * used by 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).
>>>> @@ -507,6 +512,11 @@ 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);
>>>
>>> Is this API really sufficient? There are actually quite a few other
>>> flash-related parameters that might be relevant to a controller. I
>>> presume you happen not hit them because of the particular cases you're
>>> using this for right now, but:
>>>
>>> * How many I/O lines are being used? These can vary depending on the
>>> type of flash and the number of I/O lines supported by the controller
>>> and connected on the board.
>>>
>>
>> This API communicates whatever data is currently communicated via
>> spi_message through spi_sync/transfer_one interfaces.
>
> No it doesn't. A spi_message consists of a list of spi_transfer's, and
> each spi_transfer has tx_nbits and rx_nbits fields.
>
>>> * The previous point can vary across parts of the message. There are
>>> various combinations of 1/2/4 lines used for opcode/address/data. We
>>> only support a few of those combinations in m25p80 right now, but
>>> you're not specifying any of that in this API. I guess you're just
>>> making assumptions? (BTW, I think there are others having problems
>>> with the difference between different "quad" modes on Micron flash; I
>>> haven't sorted through all the discussion there.)
>>>
>>
>> How is the spi controller currently being made aware of this via
>> m25p80_read / spi_sync() interface? AFAIK, mode field of spi_device
>> struct tell whether to do normal/dual/quad read but there is no info
>> communicated wrt 1/2/4 opcode/address/data combinations.
>
> Yes there is. m25p80 fills out spi_transfer::rx_nbits. Currently, we
> only use this for the data portion, but it's possible to support more
> lines for the address and opcode portions too, using the rx_nbits for
> the opcode and address spi_transfer struct(s) (currently, m25p80_read()
> uses 2 spi_transfers per message, where the first one contains opcode +
> address + dummy on a single line, and the second transfer receives the
> data on 1, 2, or 4 lines).
>
In September I've sent a series of patches to enhance the support of QSPI flash
memories. Patch 4 was dedicated to the m25p80 driver and set the
rx_nbits / tx_nbits fields of spi_transfer struct(s) in order to configure the
number of I/O lines independently for the opcode, address and data parts.
The work was done for m25p80_read() but also for _read_reg(), _write_reg() and
_write().
The patched m25p80 driver was then tested with an at25 memory to check non-
regression.
This series of patches also added 4 enum spi_protocol fields inside struct
spi_nor so the spi-nor framework can tell the (Q)SPI controller driver what SPI
protocol should be use for erase, read, write and register read/write
operations, depending on the memory manufacturer and the command opcode.
This was done to better support Micron, Spansion and Macronix QSPI memories.
I have tested the series with Micron QSPI memories and Atmel QSPI controller
and I guess Marek also tested it on his side with Spansion QSPI memories and
another QSPI controller.
So if it can help other developers to develop QSPI controller drivers, the
series is still available there:
for the whole series:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/371170.html
for patch 4 (depends on patch 2 for enum spi_protocol):
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/371173.html
Best Regards,
Cyrille
Hi Brian,
On 11/13/2015 09:35 PM, Cyrille Pitchen wrote:
[...]
>
> In September I've sent a series of patches to enhance the support of QSPI flash
> memories. Patch 4 was dedicated to the m25p80 driver and set the
> rx_nbits / tx_nbits fields of spi_transfer struct(s) in order to configure the
> number of I/O lines independently for the opcode, address and data parts.
> The work was done for m25p80_read() but also for _read_reg(), _write_reg() and
> _write().
> The patched m25p80 driver was then tested with an at25 memory to check non-
> regression.
>
> This series of patches also added 4 enum spi_protocol fields inside struct
> spi_nor so the spi-nor framework can tell the (Q)SPI controller driver what SPI
> protocol should be use for erase, read, write and register read/write
> operations, depending on the memory manufacturer and the command opcode.
> This was done to better support Micron, Spansion and Macronix QSPI memories.
>
> I have tested the series with Micron QSPI memories and Atmel QSPI controller
> and I guess Marek also tested it on his side with Spansion QSPI memories and
> another QSPI controller.
>
> So if it can help other developers to develop QSPI controller drivers, the
> series is still available there:
>
> for the whole series:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/371170.html
>
> for patch 4 (depends on patch 2 for enum spi_protocol):
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/371173.html
>
Should I rebase my next version on top of above patches by Cyrille or
shall I post on top of 4.4-rc1?
--
Regards
Vignesh
Hi,
On Tue, Nov 17, 2015 at 12:02:29PM +0530, Vignesh R wrote:
> On 11/13/2015 09:35 PM, Cyrille Pitchen wrote:
> >
> > In September I've sent a series of patches to enhance the support of QSPI flash
> > memories. Patch 4 was dedicated to the m25p80 driver and set the
> > rx_nbits / tx_nbits fields of spi_transfer struct(s) in order to configure the
> > number of I/O lines independently for the opcode, address and data parts.
> > The work was done for m25p80_read() but also for _read_reg(), _write_reg() and
> > _write().
> > The patched m25p80 driver was then tested with an at25 memory to check non-
> > regression.
> >
> > This series of patches also added 4 enum spi_protocol fields inside struct
> > spi_nor so the spi-nor framework can tell the (Q)SPI controller driver what SPI
> > protocol should be use for erase, read, write and register read/write
> > operations, depending on the memory manufacturer and the command opcode.
> > This was done to better support Micron, Spansion and Macronix QSPI memories.
> >
> > I have tested the series with Micron QSPI memories and Atmel QSPI controller
> > and I guess Marek also tested it on his side with Spansion QSPI memories and
> > another QSPI controller.
> >
> > So if it can help other developers to develop QSPI controller drivers, the
> > series is still available there:
> >
> > for the whole series:
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/371170.html
> >
> > for patch 4 (depends on patch 2 for enum spi_protocol):
> > http://lists.infradead.org/pipermail/linux-arm-kernel/2015-September/371173.html
> >
>
> Should I rebase my next version on top of above patches by Cyrille or
> shall I post on top of 4.4-rc1?
I'm sorry to say I really haven't had the time to review that properly.
I'm also not sure it is a true dependency for your series, as you're
tackling different pieces of the puzzle. So it's mostly just a conflict,
not a real direct help.
So unless I'm misunderstanding, I'd suggest submitting MTD stuff against
the latest l2-mtd.git (or linux-next.git; l2-mtd.git is included there),
and I'll let you know if conflicts come up that need fixing.
Brian