[PATCH 0/2] spi: omap2-mcspi: add FIFO buffer support

[PATCH 0/2] spi: omap2-mcspi: add FIFO buffer support

[Illia Smyrnov]

These patches introduce FIFO support for TI OMAP4/OMAP5 MCSPI controller.
Using FIFO unload the DMA and improve data throughput. On Blaze (OMAP 4460) 
ethernet throughput with 2 bytes MCSPI FIFO buffer enabled was increased 
from 7.0877 Mbps to 7.9200 Mbps. The FIFO implementation sanity test on 
OMAP5 Panda board also has been done.

The FIFO buffer can be used with some limitations, so if enabled, it will be
set up only for suitable SPI transfers:
* FIFO depth size defined as a multiple of the SPI word length;
* transfer's data size is a multiple of FIFO depth;
* transfer's words count less or equal 65535.

To store FIFO parameters in DT modified Matthias Brugger patch has been used
(see the discussion at https://patchwork.kernel.org/patch/2333911/)

Illia Smyrnov (1):
  spi: omap2-mcspi: Add FIFO buffer support

Matthias Brugger (1):
  spi: spi-omap2-mcspi.c: Add dts for slave device configuration.

 Documentation/devicetree/bindings/spi/omap-spi.txt |   19 ++
 drivers/spi/spi-omap2-mcspi.c                      |  199 ++++++++++++++++++--
 include/linux/platform_data/spi-omap2-mcspi.h      |    1 +
 3 files changed, 203 insertions(+), 16 deletions(-)

[PATCH 1/2] spi: spi-omap2-mcspi.c: Add dts for slave device configuration.

[Illia Smyrnov]

From: Matthias Brugger <matthias.bgg@gmail.com>

TI omap2 mcspi allows the slave devices to configure the behavior of
the SPI master. This patch adds device tree support to the existing
options.

[Illia: added changes discussed at
https://patchwork.kernel.org/patch/2333911/]

Signed-off-by: Matthias Brugger <matthias.bgg@gmail.com>
Signed-off-by: Illia Smyrnov <illia.smyrnov@ti.com>
---
 Documentation/devicetree/bindings/spi/omap-spi.txt |   17 ++++++++
 drivers/spi/spi-omap2-mcspi.c                      |   40 ++++++++++++++++++++
 2 files changed, 57 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/omap-spi.txt b/Documentation/devicetree/bindings/spi/omap-spi.txt
index 938809c..87b2841 100644
--- a/Documentation/devicetree/bindings/spi/omap-spi.txt
+++ b/Documentation/devicetree/bindings/spi/omap-spi.txt
@@ -10,8 +10,15 @@ Required properties:
 			  input. The default is D0 as input and
 			  D1 as output.
 
+SPI Controller specific data in SPI slave nodes:
+- The spi slave nodes can provide the following information which is used
+  by the spi controller:
+  - ti,spi-turbo-mode: Set turbo mode for this device.
+
 Example:
 
+- SoC Specific Portion:
+
 mcspi1: mcspi@1 {
     #address-cells = <1>;
     #size-cells = <0>;
@@ -20,3 +27,13 @@ mcspi1: mcspi@1 {
     ti,spi-num-cs = <4>;
 };
 
+- Board Specific Portion:
+
+	spi-device@0 {
+		#address-cells = <1>;
+		#size-cells = <1>;
+
+		controller-data {
+			ti,spi-turbo-mode;
+		};
+	};
diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index 86d2158..67d0409 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -735,6 +735,38 @@ static u32 omap2_mcspi_calc_divisor(u32 speed_hz)
 	return 15;
 }
 
+static struct omap2_mcspi_device_config *omap2_mcspi_get_slave_ctrldata(
+			struct spi_device *spi)
+{
+	struct omap2_mcspi_device_config *cd;
+	struct device_node *slave_np, *data_np = NULL;
+
+	slave_np = spi->dev.of_node;
+	if (!slave_np) {
+		dev_err(&spi->dev, "device node not found\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	data_np = of_get_child_by_name(slave_np, "controller-data");
+	if (!data_np) {
+		dev_err(&spi->dev, "child node 'controller-data' not found\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	cd = kzalloc(sizeof(*cd), GFP_KERNEL);
+	if (!cd) {
+		dev_err(&spi->dev, "could not allocate memory for controller data\n");
+		of_node_put(data_np);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	if (of_find_property(data_np, "ti,spi-turbo-mode", NULL))
+		cd->turbo_mode = 1;
+
+	of_node_put(data_np);
+	return cd;
+}
+
 /* called only when no transfer is active to this device */
 static int omap2_mcspi_setup_transfer(struct spi_device *spi,
 		struct spi_transfer *t)
@@ -856,6 +888,11 @@ static int omap2_mcspi_setup(struct spi_device *spi)
 	struct omap2_mcspi_regs	*ctx = &mcspi->ctx;
 	struct omap2_mcspi_dma	*mcspi_dma;
 	struct omap2_mcspi_cs	*cs = spi->controller_state;
+	struct omap2_mcspi_device_config *cd;
+
+	if (spi->dev.of_node)
+		spi->controller_data = omap2_mcspi_get_slave_ctrldata(spi);
+	cd = spi->controller_data;
 
 	if (spi->bits_per_word < 4 || spi->bits_per_word > 32) {
 		dev_dbg(&spi->dev, "setup: unsupported %d bit words\n",
@@ -902,6 +939,9 @@ static void omap2_mcspi_cleanup(struct spi_device *spi)
 
 	mcspi = spi_master_get_devdata(spi->master);
 
+	if (spi->dev.of_node && spi->controller_data)
+		kfree(spi->controller_data);
+
 	if (spi->controller_state) {
 		/* Unlink controller state from context save list */
 		cs = spi->controller_state;
-- 
1.7.0.4

Re: [PATCH 1/2] spi: spi-omap2-mcspi.c: Add dts for slave device configuration.

[Mark Brown]

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

On Wed, Jun 05, 2013 at 02:39:57PM +0300, Illia Smyrnov wrote:

> +SPI Controller specific data in SPI slave nodes:
> +- The spi slave nodes can provide the following information which is used
> +  by the spi controller:
> +  - ti,spi-turbo-mode: Set turbo mode for this device.
> +

What is turb mode and why would we not want to just enable it all the
time?  Based on this documentation it's not really possible to tell...

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

Re: [PATCH 1/2] spi: spi-omap2-mcspi.c: Add dts for slave device configuration.

[Illia Smyrnov]

On 06/05/2013 02:57 PM, Mark Brown wrote:
> On Wed, Jun 05, 2013 at 02:39:57PM +0300, Illia Smyrnov wrote:
>
>> +SPI Controller specific data in SPI slave nodes:
>> +- The spi slave nodes can provide the following information which is used
>> +  by the spi controller:
>> +  - ti,spi-turbo-mode: Set turbo mode for this device.
>> +
>
> What is turb mode and

According to OMAP TRM [1] MCSPI turbo mode improves the throughput of 
the SPI interface when a single channel is enabled by allowing transfers 
until the shift register and the MCSPI_RXx register are full.

I tested turbo mode using KS8851 SPI Ethernet controller on Blaze with 
OMAP4460 and nuttcp tool with -r for RX throughput measuring. Enabling 
turbo mode was increased throughput form 7.5538 Mbps to 8.3848 Mbps

> why would we not want to just enable it all the
> time?

Turbo mode gives the expected results not for all cases. There are some 
limitations:
- works only if a single channel is enabled (no effect when several 
channels are enable);
- improves the throughput on RX direction only;
- effective only when a transfer exceeds two words. For single SPI
word transfers OMAP TRM [1] recommends deactivate turbo mode.

So it is useful to have the property in DT, that allow us to switch 
turbo mode off/on for certain slave.

> Based on this documentation it's not really possible to tell...
>
I will add turbo mode description to documentation in the next patch version

[1]: http://www.ti.com/lit/ug/swpu235z/swpu235z.pdf

Re: [PATCH 1/2] spi: spi-omap2-mcspi.c: Add dts for slave device configuration.

[Mark Brown]

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

On Thu, Jun 06, 2013 at 01:08:46PM +0300, Illia Smyrnov wrote:
> On 06/05/2013 02:57 PM, Mark Brown wrote:

> Turbo mode gives the expected results not for all cases. There are
> some limitations:
> - works only if a single channel is enabled (no effect when several
> channels are enable);
> - improves the throughput on RX direction only;
> - effective only when a transfer exceeds two words. For single SPI
> word transfers OMAP TRM [1] recommends deactivate turbo mode.

> So it is useful to have the property in DT, that allow us to switch
> turbo mode off/on for certain slave.

Why?  These sound like conditions that can readily be detected at
runtime.

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

[PATCH 2/2] spi: omap2-mcspi: Add FIFO buffer support

[Illia Smyrnov]

The MCSPI controller has a built-in FIFO buffer to unload the DMA or
interrupt handler and improve data throughput.

The FIFO could be enabled by setting up the fifo_depth configuration
parameter. If enabled, the FIFO will be used, only for transfers,
which data size is a multiple of FIFO buffer size (fifo_depth).

Signed-off-by: Illia Smyrnov <illia.smyrnov@ti.com>
---
 Documentation/devicetree/bindings/spi/omap-spi.txt |    2 +
 drivers/spi/spi-omap2-mcspi.c                      |  159 ++++++++++++++++++--
 include/linux/platform_data/spi-omap2-mcspi.h      |    1 +
 3 files changed, 146 insertions(+), 16 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/omap-spi.txt b/Documentation/devicetree/bindings/spi/omap-spi.txt
index 87b2841..46459e8 100644
--- a/Documentation/devicetree/bindings/spi/omap-spi.txt
+++ b/Documentation/devicetree/bindings/spi/omap-spi.txt
@@ -14,6 +14,7 @@ SPI Controller specific data in SPI slave nodes:
 - The spi slave nodes can provide the following information which is used
   by the spi controller:
   - ti,spi-turbo-mode: Set turbo mode for this device.
+  - ti,spi-fifo-depth: Enable FIFO and set up buffer depth.
 
 Example:
 
@@ -35,5 +36,6 @@ mcspi1: mcspi@1 {
 
 		controller-data {
 			ti,spi-turbo-mode;
+			ti,spi-fifo-depth = <64>;
 		};
 	};
diff --git a/drivers/spi/spi-omap2-mcspi.c b/drivers/spi/spi-omap2-mcspi.c
index 67d0409..f973656 100644
--- a/drivers/spi/spi-omap2-mcspi.c
+++ b/drivers/spi/spi-omap2-mcspi.c
@@ -45,6 +45,8 @@
 #include <linux/platform_data/spi-omap2-mcspi.h>
 
 #define OMAP2_MCSPI_MAX_FREQ		48000000
+#define OMAP2_MCSPI_MAX_FIFODEPTH	64
+#define OMAP2_MCSPI_MAX_FIFOWCNT	0xFFFF
 #define SPI_AUTOSUSPEND_TIMEOUT		2000
 
 #define OMAP2_MCSPI_REVISION		0x00
@@ -54,6 +56,7 @@
 #define OMAP2_MCSPI_WAKEUPENABLE	0x20
 #define OMAP2_MCSPI_SYST		0x24
 #define OMAP2_MCSPI_MODULCTRL		0x28
+#define OMAP2_MCSPI_XFERLEVEL		0x7c
 
 /* per-channel banks, 0x14 bytes each, first is: */
 #define OMAP2_MCSPI_CHCONF0		0x2c
@@ -63,6 +66,7 @@
 #define OMAP2_MCSPI_RX0			0x3c
 
 /* per-register bitmasks: */
+#define OMAP2_MCSPI_IRQSTATUS_EOW	BIT(17)
 
 #define OMAP2_MCSPI_MODULCTRL_SINGLE	BIT(0)
 #define OMAP2_MCSPI_MODULCTRL_MS	BIT(2)
@@ -83,10 +87,13 @@
 #define OMAP2_MCSPI_CHCONF_IS		BIT(18)
 #define OMAP2_MCSPI_CHCONF_TURBO	BIT(19)
 #define OMAP2_MCSPI_CHCONF_FORCE	BIT(20)
+#define OMAP2_MCSPI_CHCONF_FFET		BIT(27)
+#define OMAP2_MCSPI_CHCONF_FFER		BIT(28)
 
 #define OMAP2_MCSPI_CHSTAT_RXS		BIT(0)
 #define OMAP2_MCSPI_CHSTAT_TXS		BIT(1)
 #define OMAP2_MCSPI_CHSTAT_EOT		BIT(2)
+#define OMAP2_MCSPI_CHSTAT_TXFFE	BIT(3)
 
 #define OMAP2_MCSPI_CHCTRL_EN		BIT(0)
 
@@ -129,6 +136,7 @@ struct omap2_mcspi {
 	struct omap2_mcspi_dma	*dma_channels;
 	struct device		*dev;
 	struct omap2_mcspi_regs ctx;
+	unsigned short		fifo_depth;
 	unsigned int		pin_dir:1;
 };
 
@@ -248,6 +256,59 @@ static void omap2_mcspi_set_master_mode(struct spi_master *master)
 	ctx->modulctrl = l;
 }
 
+static void omap2_mcspi_set_fifo(const struct spi_device *spi,
+				struct spi_transfer *t, int enable)
+{
+	struct spi_master *master = spi->master;
+	struct omap2_mcspi_cs *cs = spi->controller_state;
+	struct omap2_mcspi_device_config *cd = spi->controller_data;
+	struct omap2_mcspi *mcspi;
+	unsigned int wcnt;
+	int is_read, bytes_per_word;
+	u16 level;
+	u32 chconf;
+
+	mcspi = spi_master_get_devdata(master);
+	chconf = mcspi_cached_chconf0(spi);
+	is_read = (t->rx_buf != NULL) ? 1 : 0;
+
+	if (enable) {
+		if (!cd || cd->fifo_depth <= 0)
+			return;
+
+		bytes_per_word = (cs->word_len <= 8) ? 1 :
+				(cs->word_len <= 16) ? 2 :
+				/* cs->word_len <= 32 */ 4;
+
+		if (cd->fifo_depth % bytes_per_word != 0
+			|| (t->len > cd->fifo_depth
+				&& t->len % cd->fifo_depth != 0))
+			return;
+
+		wcnt = t->len / bytes_per_word;
+
+		if (wcnt > OMAP2_MCSPI_MAX_FIFOWCNT)
+			return;
+
+		mcspi->fifo_depth = cd->fifo_depth;
+
+		level = (t->len < mcspi->fifo_depth ? t->len :
+			mcspi->fifo_depth) - 1;
+
+		mcspi_write_reg(master, OMAP2_MCSPI_XFERLEVEL,
+			((wcnt << 16) | (level << (is_read ? 8 : 0))));
+
+		chconf |= is_read ? OMAP2_MCSPI_CHCONF_FFER :
+			OMAP2_MCSPI_CHCONF_FFET;
+	} else {
+		mcspi->fifo_depth = 0;
+		chconf &= ~(is_read ? OMAP2_MCSPI_CHCONF_FFER :
+			OMAP2_MCSPI_CHCONF_FFET);
+	}
+
+	mcspi_write_chconf0(spi, chconf);
+}
+
 static void omap2_mcspi_restore_ctx(struct omap2_mcspi *mcspi)
 {
 	struct spi_master	*spi_cntrl = mcspi->master;
@@ -364,7 +425,7 @@ omap2_mcspi_rx_dma(struct spi_device *spi, struct spi_transfer *xfer,
 {
 	struct omap2_mcspi	*mcspi;
 	struct omap2_mcspi_dma  *mcspi_dma;
-	unsigned int		count;
+	unsigned int		count, dma_count;
 	u32			l;
 	int			elements = 0;
 	int			word_len, element_count;
@@ -372,6 +433,7 @@ omap2_mcspi_rx_dma(struct spi_device *spi, struct spi_transfer *xfer,
 	mcspi = spi_master_get_devdata(spi->master);
 	mcspi_dma = &mcspi->dma_channels[spi->chip_select];
 	count = xfer->len;
+	dma_count = xfer->len - ((mcspi->fifo_depth == 0) ? es : 0);
 	word_len = cs->word_len;
 	l = mcspi_cached_chconf0(spi);
 
@@ -385,16 +447,15 @@ omap2_mcspi_rx_dma(struct spi_device *spi, struct spi_transfer *xfer,
 	if (mcspi_dma->dma_rx) {
 		struct dma_async_tx_descriptor *tx;
 		struct scatterlist sg;
-		size_t len = xfer->len - es;
 
 		dmaengine_slave_config(mcspi_dma->dma_rx, &cfg);
 
-		if (l & OMAP2_MCSPI_CHCONF_TURBO)
-			len -= es;
+		if ((l & OMAP2_MCSPI_CHCONF_TURBO) && (mcspi->fifo_depth == 0))
+			dma_count -= es;
 
 		sg_init_table(&sg, 1);
 		sg_dma_address(&sg) = xfer->rx_dma;
-		sg_dma_len(&sg) = len;
+		sg_dma_len(&sg) = dma_count;
 
 		tx = dmaengine_prep_slave_sg(mcspi_dma->dma_rx, &sg, 1,
 				DMA_DEV_TO_MEM, DMA_PREP_INTERRUPT |
@@ -414,6 +475,10 @@ omap2_mcspi_rx_dma(struct spi_device *spi, struct spi_transfer *xfer,
 	wait_for_completion(&mcspi_dma->dma_rx_completion);
 	dma_unmap_single(mcspi->dev, xfer->rx_dma, count,
 			 DMA_FROM_DEVICE);
+
+	if (mcspi->fifo_depth > 0)
+		return count;
+
 	omap2_mcspi_set_enable(spi, 0);
 
 	elements = element_count - 1;
@@ -475,7 +540,10 @@ omap2_mcspi_txrx_dma(struct spi_device *spi, struct spi_transfer *xfer)
 	struct dma_slave_config	cfg;
 	enum dma_slave_buswidth width;
 	unsigned es;
+	u32			burst;
 	void __iomem		*chstat_reg;
+	void __iomem            *irqstat_reg;
+	int			wait_res;
 
 	mcspi = spi_master_get_devdata(spi->master);
 	mcspi_dma = &mcspi->dma_channels[spi->chip_select];
@@ -493,19 +561,27 @@ omap2_mcspi_txrx_dma(struct spi_device *spi, struct spi_transfer *xfer)
 		es = 4;
 	}
 
+	count = xfer->len;
+	burst = 1;
+
+	if (mcspi->fifo_depth > 0) {
+		if (count > mcspi->fifo_depth)
+			burst = mcspi->fifo_depth / es;
+		else
+			burst = count / es;
+	}
+
 	memset(&cfg, 0, sizeof(cfg));
 	cfg.src_addr = cs->phys + OMAP2_MCSPI_RX0;
 	cfg.dst_addr = cs->phys + OMAP2_MCSPI_TX0;
 	cfg.src_addr_width = width;
 	cfg.dst_addr_width = width;
-	cfg.src_maxburst = 1;
-	cfg.dst_maxburst = 1;
+	cfg.src_maxburst = burst;
+	cfg.dst_maxburst = burst;
 
 	rx = xfer->rx_buf;
 	tx = xfer->tx_buf;
 
-	count = xfer->len;
-
 	if (tx != NULL)
 		omap2_mcspi_tx_dma(spi, xfer, cfg);
 
@@ -513,18 +589,38 @@ omap2_mcspi_txrx_dma(struct spi_device *spi, struct spi_transfer *xfer)
 		count = omap2_mcspi_rx_dma(spi, xfer, cfg, es);
 
 	if (tx != NULL) {
-		chstat_reg = cs->base + OMAP2_MCSPI_CHSTAT0;
 		wait_for_completion(&mcspi_dma->dma_tx_completion);
 		dma_unmap_single(mcspi->dev, xfer->tx_dma, xfer->len,
 				 DMA_TO_DEVICE);
 
+		if (mcspi->fifo_depth > 0) {
+			irqstat_reg = mcspi->base + OMAP2_MCSPI_IRQSTATUS;
+
+			if (mcspi_wait_for_reg_bit(irqstat_reg,
+						OMAP2_MCSPI_IRQSTATUS_EOW) < 0)
+				dev_err(&spi->dev, "EOW timed out\n");
+
+			mcspi_write_reg(mcspi->master, OMAP2_MCSPI_IRQSTATUS,
+					OMAP2_MCSPI_IRQSTATUS_EOW);
+		}
+
 		/* for TX_ONLY mode, be sure all words have shifted out */
 		if (rx == NULL) {
-			if (mcspi_wait_for_reg_bit(chstat_reg,
-						OMAP2_MCSPI_CHSTAT_TXS) < 0)
-				dev_err(&spi->dev, "TXS timed out\n");
-			else if (mcspi_wait_for_reg_bit(chstat_reg,
-						OMAP2_MCSPI_CHSTAT_EOT) < 0)
+			chstat_reg = cs->base + OMAP2_MCSPI_CHSTAT0;
+			if (mcspi->fifo_depth > 0) {
+				wait_res = mcspi_wait_for_reg_bit(chstat_reg,
+						OMAP2_MCSPI_CHSTAT_TXFFE);
+				if (wait_res < 0)
+					dev_err(&spi->dev, "TXFFE timed out\n");
+			} else {
+				wait_res = mcspi_wait_for_reg_bit(chstat_reg,
+						OMAP2_MCSPI_CHSTAT_TXS);
+				if (wait_res < 0)
+					dev_err(&spi->dev, "TXS timed out\n");
+			}
+			if (wait_res >= 0
+				&& (mcspi_wait_for_reg_bit(chstat_reg,
+					OMAP2_MCSPI_CHSTAT_EOT) < 0))
 				dev_err(&spi->dev, "EOT timed out\n");
 		}
 	}
@@ -740,6 +836,7 @@ static struct omap2_mcspi_device_config *omap2_mcspi_get_slave_ctrldata(
 {
 	struct omap2_mcspi_device_config *cd;
 	struct device_node *slave_np, *data_np = NULL;
+	u32 fifo_depth;
 
 	slave_np = spi->dev.of_node;
 	if (!slave_np) {
@@ -763,6 +860,11 @@ static struct omap2_mcspi_device_config *omap2_mcspi_get_slave_ctrldata(
 	if (of_find_property(data_np, "ti,spi-turbo-mode", NULL))
 		cd->turbo_mode = 1;
 
+	if (of_property_read_u32(data_np, "ti,spi-fifo-depth", &fifo_depth) == 0)
+		cd->fifo_depth = fifo_depth;
+	else
+		cd->fifo_depth = 0;
+
 	of_node_put(data_np);
 	return cd;
 }
@@ -900,6 +1002,17 @@ static int omap2_mcspi_setup(struct spi_device *spi)
 		return -EINVAL;
 	}
 
+	if (cd && cd->fifo_depth > 0) {
+		int bytes_per_word =	(spi->bits_per_word <= 8)  ? 1 :
+					(spi->bits_per_word <= 16) ? 2 :
+					/*spi->bits_per_word <= 32*/ 4;
+		if ((cd->fifo_depth % bytes_per_word) != 0) {
+			dev_dbg(&spi->dev, "setup: invalid %u fifo depth\n",
+				cd->fifo_depth);
+			return -EINVAL;
+		}
+	}
+
 	mcspi_dma = &mcspi->dma_channels[spi->chip_select];
 
 	if (!cs) {
@@ -991,7 +1104,7 @@ static void omap2_mcspi_work(struct omap2_mcspi *mcspi, struct spi_message *m)
 	cs = spi->controller_state;
 	cd = spi->controller_data;
 
-	omap2_mcspi_set_enable(spi, 1);
+	omap2_mcspi_set_enable(spi, 0);
 	list_for_each_entry(t, &m->transfers, transfer_list) {
 		if (t->tx_buf == NULL && t->rx_buf == NULL && t->len) {
 			status = -EINVAL;
@@ -1039,6 +1152,12 @@ static void omap2_mcspi_work(struct omap2_mcspi *mcspi, struct spi_message *m)
 		if (t->len) {
 			unsigned	count;
 
+			if ((mcspi_dma->dma_rx && mcspi_dma->dma_tx) &&
+			    (m->is_dma_mapped || t->len >= DMA_MIN_BYTES))
+				omap2_mcspi_set_fifo(spi, t, 1);
+
+			omap2_mcspi_set_enable(spi, 1);
+
 			/* RX_ONLY mode needs dummy data in TX reg */
 			if (t->tx_buf == NULL)
 				__raw_writel(0, cs->base
@@ -1065,6 +1184,11 @@ static void omap2_mcspi_work(struct omap2_mcspi *mcspi, struct spi_message *m)
 			omap2_mcspi_force_cs(spi, 0);
 			cs_active = 0;
 		}
+
+		omap2_mcspi_set_enable(spi, 0);
+
+		if (mcspi->fifo_depth > 0)
+			omap2_mcspi_set_fifo(spi, t, 0);
 	}
 	/* Restore defaults if they were overriden */
 	if (par_override) {
@@ -1085,6 +1209,9 @@ static void omap2_mcspi_work(struct omap2_mcspi *mcspi, struct spi_message *m)
 
 	omap2_mcspi_set_enable(spi, 0);
 
+	if (mcspi->fifo_depth > 0 && t)
+		omap2_mcspi_set_fifo(spi, t, 0);
+
 	m->status = status;
 
 }
diff --git a/include/linux/platform_data/spi-omap2-mcspi.h b/include/linux/platform_data/spi-omap2-mcspi.h
index c100456..27d8ed9 100644
--- a/include/linux/platform_data/spi-omap2-mcspi.h
+++ b/include/linux/platform_data/spi-omap2-mcspi.h
@@ -21,6 +21,7 @@ struct omap2_mcspi_dev_attr {
 };
 
 struct omap2_mcspi_device_config {
+	unsigned short fifo_depth;
 	unsigned turbo_mode:1;
 
 	/* toggle chip select after every word */
-- 
1.7.0.4

Re: [PATCH 2/2] spi: omap2-mcspi: Add FIFO buffer support

[Mark Brown]

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

On Wed, Jun 05, 2013 at 02:39:58PM +0300, Illia Smyrnov wrote:

>  - The spi slave nodes can provide the following information which is used
>    by the spi controller:
>    - ti,spi-turbo-mode: Set turbo mode for this device.
> +  - ti,spi-fifo-depth: Enable FIFO and set up buffer depth.

Why is this defined for slaves?  Surely the size of the FIFO in the
controller is a property of the controller not the slave?

> +		bytes_per_word = (cs->word_len <= 8) ? 1 :
> +				(cs->word_len <= 16) ? 2 :
> +				/* cs->word_len <= 32 */ 4;

This isn't legible.  Use a switch statement, or better yet just divide.

> +		level = (t->len < mcspi->fifo_depth ? t->len :
> +			mcspi->fifo_depth) - 1;
> +
> +		mcspi_write_reg(master, OMAP2_MCSPI_XFERLEVEL,
> +			((wcnt << 16) | (level << (is_read ? 8 : 0))));
> +
> +		chconf |= is_read ? OMAP2_MCSPI_CHCONF_FFER :
> +			OMAP2_MCSPI_CHCONF_FFET;

Please avoid such extensive use of the ternery operator, it's not good
for legibility.

> +	} else {
> +		mcspi->fifo_depth = 0;
> +		chconf &= ~(is_read ? OMAP2_MCSPI_CHCONF_FFER :
> +			OMAP2_MCSPI_CHCONF_FFET);
> +	}
> +
> +	mcspi_write_chconf0(spi, chconf);

We have a bunch of return statements further up the function in cases
where the FIFO can't be used which means that if we're in FIFO mode then
hit one of those we'll not disable FIFO mode as far as I can tell?

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

Re: [PATCH 2/2] spi: omap2-mcspi: Add FIFO buffer support

[Illia Smyrnov]

On 06/05/2013 03:03 PM, Mark Brown wrote:
> On Wed, Jun 05, 2013 at 02:39:58PM +0300, Illia Smyrnov wrote:
>
>>   - The spi slave nodes can provide the following information which is used
>>     by the spi controller:
>>     - ti,spi-turbo-mode: Set turbo mode for this device.
>> +  - ti,spi-fifo-depth: Enable FIFO and set up buffer depth.
>
> Why is this defined for slaves?  Surely the size of the FIFO in the
> controller is a property of the controller not the slave?

According to OMAP TRM [1] the FIFO buffer can be used by only one 
channel at a time. If several channels are selected and several FIFO 
enable bit fields are set to 1, the controller forces the buffer not to 
be used.

If there are several slaves on the controller we must select which of 
slaves will use the FIFO for SPI transfers. Also, optimal FIFO size is 
heavily dependent of the SPI transfers length specific for certain slave.

>
>> +		bytes_per_word = (cs->word_len <= 8) ? 1 :
>> +				(cs->word_len <= 16) ? 2 :
>> +				/* cs->word_len <= 32 */ 4;
>
> This isn't legible.  Use a switch statement, or better yet just divide.
>
>> +		level = (t->len < mcspi->fifo_depth ? t->len :
>> +			mcspi->fifo_depth) - 1;
>> +
>> +		mcspi_write_reg(master, OMAP2_MCSPI_XFERLEVEL,
>> +			((wcnt << 16) | (level << (is_read ? 8 : 0))));
>> +
>> +		chconf |= is_read ? OMAP2_MCSPI_CHCONF_FFER :
>> +			OMAP2_MCSPI_CHCONF_FFET;
>
> Please avoid such extensive use of the ternery operator, it's not good
> for legibility.
>> +	} else {
>> +		mcspi->fifo_depth = 0;
>> +		chconf &= ~(is_read ? OMAP2_MCSPI_CHCONF_FFER :
>> +			OMAP2_MCSPI_CHCONF_FFET);
>> +	}
>> +
>> +	mcspi_write_chconf0(spi, chconf);
>
> We have a bunch of return statements further up the function in cases
> where the FIFO can't be used which means that if we're in FIFO mode then
> hit one of those we'll not disable FIFO mode as far as I can tell?
>

I will rework this in the next patch version.

[1]: http://www.ti.com/lit/ug/swpu235z/swpu235z.pdf

Re: [PATCH 2/2] spi: omap2-mcspi: Add FIFO buffer support

[Mark Brown]

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

On Thu, Jun 06, 2013 at 01:08:56PM +0300, Illia Smyrnov wrote:
> On 06/05/2013 03:03 PM, Mark Brown wrote:

> >Why is this defined for slaves?  Surely the size of the FIFO in the
> >controller is a property of the controller not the slave?

> According to OMAP TRM [1] the FIFO buffer can be used by only one
> channel at a time. If several channels are selected and several FIFO
> enable bit fields are set to 1, the controller forces the buffer not
> to be used.

The controller ought to be able to figure this out for itself.  As a
first pass just grabbing the FIFO on a first come first served basis
will probably work well most of the time, the device would have to be
very active for it to constantly be doing transfers on all channels.

If there's more contention than that we probably ought to be looking at
how we handle this in general, it seems like we'd have more problems
than just the FIFO to worry about.

> If there are several slaves on the controller we must select which
> of slaves will use the FIFO for SPI transfers. Also, optimal FIFO

A single controller is only going to be able to talk to one slave at
once, everything on the bus except chip select is shared.

> size is heavily dependent of the SPI transfers length specific for
> certain slave.

The transfer length doesn't seem like something that we want to be
encoding in DT, particularly not indirectly - it is obviously readily
available at runtime, variable during runtime (eg, firmware download may
do large transfers on a device that only does small transfers most of
the time) and is something that updates to the drivers could change.

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

Re: [PATCH 2/2] spi: omap2-mcspi: Add FIFO buffer support

[Illia Smyrnov]

On 06/06/2013 01:30 PM, Mark Brown wrote:
> On Thu, Jun 06, 2013 at 01:08:56PM +0300, Illia Smyrnov wrote:
>> On 06/05/2013 03:03 PM, Mark Brown wrote:
>
>>> Why is this defined for slaves?  Surely the size of the FIFO in the
>>> controller is a property of the controller not the slave?
>
>> According to OMAP TRM [1] the FIFO buffer can be used by only one
>> channel at a time. If several channels are selected and several FIFO
>> enable bit fields are set to 1, the controller forces the buffer not
>> to be used.
>
> The controller ought to be able to figure this out for itself.  As a
> first pass just grabbing the FIFO on a first come first served basis
> will probably work well most of the time, the device would have to be
> very active for it to constantly be doing transfers on all channels.
>
> If there's more contention than that we probably ought to be looking at
> how we handle this in general, it seems like we'd have more problems
> than just the FIFO to worry about.
>
>> If there are several slaves on the controller we must select which
>> of slaves will use the FIFO for SPI transfers. Also, optimal FIFO
>
> A single controller is only going to be able to talk to one slave at
> once, everything on the bus except chip select is shared.
>
>> size is heavily dependent of the SPI transfers length specific for
>> certain slave.
>
> The transfer length doesn't seem like something that we want to be
> encoding in DT, particularly not indirectly - it is obviously readily
> available at runtime, variable during runtime (eg, firmware download may
> do large transfers on a device that only does small transfers most of
> the time) and is something that updates to the drivers could change.

I sent patches for v2 of the MCSPI FIFO support implementation. In this 
version driver will calculate the largest possible FIFO buffer size for 
each SPI transfer.
Also FIFO could be enabled by setting up parameter in the MCSPI 
controller (master) DT node.