[PATCH v2 0/8] Improvements for SPI IMX driver for Freescale IMX53 and IMX6 families

[PATCH v2 0/8] Improvements for SPI IMX driver for Freescale IMX53 and IMX6 families

[Anton Bondarenko]

A number of patches to impove or add the implementation
for the spi-imx driver related to Freescale IMX53 and IMX6.
It would also possible some of patches can be applied for other
Freescale controllers using spi-imx driver but could not be tested
due to lack of hardware.

Anton Bondarenko (8):
  spi: imx: Fix DMA transfer
  spi: imx: replace fixed timeout with calculated one
  spi: imx: add support for all SPI word width for DMA transfer
  spi: imx: add selection for iMX53 and iMX6 controller type
  spi: imx: Add support for loopback for ECSPI controllers
  spi: imx: return error from dma channel request
  spi: imx: defer spi initialization, if DMA engine is pending
  spi: imx: Add support for SPI Slave mode for imx53 and imx6 chips

 .../devicetree/bindings/spi/fsl-imx-cspi.txt       |   3 +
 drivers/spi/spi-imx.c                              | 486 ++++++++++++++++-----
 2 files changed, 388 insertions(+), 101 deletions(-)

-- 
2.5.2

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/8] spi: imx: Fix DMA transfer

[Anton Bondarenko]

RX DMA tail data handling doesn't work correctly in many cases with
current implementation. It happens because SPI core was setup
to generates both RX watermark level and RX DATA TAIL events
incorrectly. SPI transfer triggering for DMA also done in wrong way.

SPI client wants to transfer 70 words for example. The old DMA
implementation setup RX DATA TAIL equal 6 words. In this case
RX DMA event will be generated after 6 words read from RX FIFO.
The garbage can be read out from RX FIFO because SPI HW does
not receive all required words to trigger RX watermark event.

New implementation change handling of RX data tail. DMA is used to process
all TX data and only full chunks of RX data with size aligned to FIFO/2.
Driver is waiting until both TX and RX DMA transaction done and all
TX data are pushed out. At that moment there is only RX data tail in
the RX FIFO. This data read out using PIO.

Transfer triggering changed to avoid RX data loss.

Signed-off-by: Anton Bondarenko <anton_bondarenko@mentor.com>
---
 drivers/spi/spi-imx.c | 105 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 66 insertions(+), 39 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index f9deb84..165bc2c 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -39,6 +39,8 @@
 #include <linux/of_device.h>
 #include <linux/of_gpio.h>
 
+#include <asm/cacheflush.h>
+
 #include <linux/platform_data/dma-imx.h>
 #include <linux/platform_data/spi-imx.h>
 
@@ -53,6 +55,7 @@
 /* generic defines to abstract from the different register layouts */
 #define MXC_INT_RR	(1 << 0) /* Receive data ready interrupt */
 #define MXC_INT_TE	(1 << 1) /* Transmit FIFO empty interrupt */
+#define MXC_INT_TCEN	BIT(7)   /* Transfer complete */
 
 /* The maximum  bytes that a sdma BD can transfer.*/
 #define MAX_SDMA_BD_BYTES  (1 << 15)
@@ -104,9 +107,7 @@ struct spi_imx_data {
 	unsigned int dma_is_inited;
 	unsigned int dma_finished;
 	bool usedma;
-	u32 rx_wml;
-	u32 tx_wml;
-	u32 rxt_wml;
+	u32 wml;
 	struct completion dma_rx_completion;
 	struct completion dma_tx_completion;
 
@@ -201,9 +202,8 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
 {
 	struct spi_imx_data *spi_imx = spi_master_get_devdata(master);
 
-	if (spi_imx->dma_is_inited
-	    && transfer->len > spi_imx->rx_wml * sizeof(u32)
-	    && transfer->len > spi_imx->tx_wml * sizeof(u32))
+	if (spi_imx->dma_is_inited &&
+	    (transfer->len > spi_imx->wml * sizeof(u32)))
 		return true;
 	return false;
 }
@@ -228,6 +228,7 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
 #define MX51_ECSPI_INT		0x10
 #define MX51_ECSPI_INT_TEEN		(1 <<  0)
 #define MX51_ECSPI_INT_RREN		(1 <<  3)
+#define MX51_ECSPI_INT_TCEN		BIT(7)
 
 #define MX51_ECSPI_DMA      0x14
 #define MX51_ECSPI_DMA_TX_WML_OFFSET	0
@@ -292,6 +293,9 @@ static void __maybe_unused mx51_ecspi_intctrl(struct spi_imx_data *spi_imx, int
 	if (enable & MXC_INT_RR)
 		val |= MX51_ECSPI_INT_RREN;
 
+	if (enable & MXC_INT_TCEN)
+		val |= MX51_ECSPI_INT_TCEN;
+
 	writel(val, spi_imx->base + MX51_ECSPI_INT);
 }
 
@@ -311,8 +315,9 @@ static void __maybe_unused mx51_ecspi_trigger(struct spi_imx_data *spi_imx)
 static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
 		struct spi_imx_config *config)
 {
-	u32 ctrl = MX51_ECSPI_CTRL_ENABLE, cfg = 0, dma = 0;
-	u32 tx_wml_cfg, rx_wml_cfg, rxt_wml_cfg;
+	u32 ctrl = MX51_ECSPI_CTRL_ENABLE, dma = 0;
+	u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
+
 	u32 clk = config->speed_hz, delay;
 
 	/*
@@ -369,19 +374,10 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
 	 * and enable DMA request.
 	 */
 	if (spi_imx->dma_is_inited) {
-		dma = readl(spi_imx->base + MX51_ECSPI_DMA);
-
-		spi_imx->rxt_wml = spi_imx_get_fifosize(spi_imx) / 2;
-		rx_wml_cfg = spi_imx->rx_wml << MX51_ECSPI_DMA_RX_WML_OFFSET;
-		tx_wml_cfg = spi_imx->tx_wml << MX51_ECSPI_DMA_TX_WML_OFFSET;
-		rxt_wml_cfg = spi_imx->rxt_wml << MX51_ECSPI_DMA_RXT_WML_OFFSET;
-		dma = (dma & ~MX51_ECSPI_DMA_TX_WML_MASK
-			   & ~MX51_ECSPI_DMA_RX_WML_MASK
-			   & ~MX51_ECSPI_DMA_RXT_WML_MASK)
-			   | rx_wml_cfg | tx_wml_cfg | rxt_wml_cfg
-			   |(1 << MX51_ECSPI_DMA_TEDEN_OFFSET)
-			   |(1 << MX51_ECSPI_DMA_RXDEN_OFFSET)
-			   |(1 << MX51_ECSPI_DMA_RXTDEN_OFFSET);
+		dma = (spi_imx->wml - 1) << MX51_ECSPI_DMA_RX_WML_OFFSET
+		      | (spi_imx->wml - 1) << MX51_ECSPI_DMA_TX_WML_OFFSET
+		      | (1 << MX51_ECSPI_DMA_TEDEN_OFFSET)
+		      | (1 << MX51_ECSPI_DMA_RXDEN_OFFSET);
 
 		writel(dma, spi_imx->base + MX51_ECSPI_DMA);
 	}
@@ -825,6 +821,8 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
 	if (of_machine_is_compatible("fsl,imx6dl"))
 		return 0;
 
+	spi_imx->wml = spi_imx_get_fifosize(spi_imx) / 2;
+
 	/* Prepare for TX DMA: */
 	master->dma_tx = dma_request_slave_channel(dev, "tx");
 	if (!master->dma_tx) {
@@ -836,7 +834,8 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
 	slave_config.direction = DMA_MEM_TO_DEV;
 	slave_config.dst_addr = res->start + MXC_CSPITXDATA;
 	slave_config.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
-	slave_config.dst_maxburst = spi_imx_get_fifosize(spi_imx) / 2;
+	slave_config.dst_maxburst = spi_imx_get_fifosize(spi_imx)
+					- spi_imx->wml;
 	ret = dmaengine_slave_config(master->dma_tx, &slave_config);
 	if (ret) {
 		dev_err(dev, "error in TX dma configuration.\n");
@@ -854,7 +853,8 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
 	slave_config.direction = DMA_DEV_TO_MEM;
 	slave_config.src_addr = res->start + MXC_CSPIRXDATA;
 	slave_config.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
-	slave_config.src_maxburst = spi_imx_get_fifosize(spi_imx) / 2;
+	slave_config.src_maxburst = spi_imx_get_fifosize(spi_imx)
+					- spi_imx->wml;
 	ret = dmaengine_slave_config(master->dma_rx, &slave_config);
 	if (ret) {
 		dev_err(dev, "error in RX dma configuration.\n");
@@ -867,8 +867,6 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
 	master->max_dma_len = MAX_SDMA_BD_BYTES;
 	spi_imx->bitbang.master->flags = SPI_MASTER_MUST_RX |
 					 SPI_MASTER_MUST_TX;
-	spi_imx->tx_wml = spi_imx_get_fifosize(spi_imx) / 2;
-	spi_imx->rx_wml = spi_imx_get_fifosize(spi_imx) / 2;
 	spi_imx->dma_is_inited = 1;
 
 	return 0;
@@ -897,8 +895,7 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
 	struct dma_async_tx_descriptor *desc_tx = NULL, *desc_rx = NULL;
 	int ret;
 	unsigned long timeout;
-	u32 dma;
-	int left;
+	const int left = transfer->len % spi_imx->wml;
 	struct spi_master *master = spi_imx->bitbang.master;
 	struct sg_table *tx = &transfer->tx_sg, *rx = &transfer->rx_sg;
 
@@ -915,9 +912,23 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
 	}
 
 	if (rx) {
+		/* Cut RX data tail */
+		const unsigned int old_nents = rx->nents;
+
+		WARN_ON(sg_dma_len(&rx->sgl[rx->nents - 1]) < left);
+		sg_dma_len(&rx->sgl[rx->nents - 1]) -= left;
+		if (sg_dma_len(&rx->sgl[rx->nents - 1]) == 0)
+			--rx->nents;
+
 		desc_rx = dmaengine_prep_slave_sg(master->dma_rx,
 					rx->sgl, rx->nents, DMA_DEV_TO_MEM,
 					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+
+		/* Restore old SG table state */
+		if (old_nents > rx->nents)
+			++rx->nents;
+		sg_dma_len(&rx->sgl[rx->nents - 1]) += left;
+
 		if (!desc_rx)
 			goto no_dma;
 
@@ -932,17 +943,10 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
 	/* Trigger the cspi module. */
 	spi_imx->dma_finished = 0;
 
-	dma = readl(spi_imx->base + MX51_ECSPI_DMA);
-	dma = dma & (~MX51_ECSPI_DMA_RXT_WML_MASK);
-	/* Change RX_DMA_LENGTH trigger dma fetch tail data */
-	left = transfer->len % spi_imx->rxt_wml;
-	if (left)
-		writel(dma | (left << MX51_ECSPI_DMA_RXT_WML_OFFSET),
-				spi_imx->base + MX51_ECSPI_DMA);
+	dma_async_issue_pending(master->dma_rx);
+	dma_async_issue_pending(master->dma_tx);
 	spi_imx->devtype_data->trigger(spi_imx);
 
-	dma_async_issue_pending(master->dma_tx);
-	dma_async_issue_pending(master->dma_rx);
 	/* Wait SDMA to finish the data transfer.*/
 	timeout = wait_for_completion_timeout(&spi_imx->dma_tx_completion,
 						IMX_DMA_TIMEOUT);
@@ -951,6 +955,7 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
 			dev_driver_string(&master->dev),
 			dev_name(&master->dev));
 		dmaengine_terminate_all(master->dma_tx);
+		dmaengine_terminate_all(master->dma_rx);
 	} else {
 		timeout = wait_for_completion_timeout(
 				&spi_imx->dma_rx_completion, IMX_DMA_TIMEOUT);
@@ -960,10 +965,32 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
 				dev_name(&master->dev));
 			spi_imx->devtype_data->reset(spi_imx);
 			dmaengine_terminate_all(master->dma_rx);
+		} else if (left) {
+			void *pio_buffer = transfer->rx_buf
+						+ (transfer->len - left);
+
+			dma_sync_sg_for_cpu(master->dma_rx->device->dev,
+					    rx->sgl, rx->nents,
+					    DMA_FROM_DEVICE);
+
+			spi_imx->rx_buf = pio_buffer;
+			spi_imx->txfifo = left;
+			reinit_completion(&spi_imx->xfer_done);
+
+			spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TCEN);
+
+			timeout = wait_for_completion_timeout(
+					&spi_imx->xfer_done, IMX_DMA_TIMEOUT);
+			if (!timeout) {
+				pr_warn("%s %s: I/O Error in RX tail\n",
+					dev_driver_string(&master->dev),
+					dev_name(&master->dev));
+			}
+
+			dmac_flush_range(pio_buffer, pio_buffer + left);
+			outer_flush_range(virt_to_phys(pio_buffer),
+					  virt_to_phys(pio_buffer) + left);
 		}
-		writel(dma |
-		       spi_imx->rxt_wml << MX51_ECSPI_DMA_RXT_WML_OFFSET,
-		       spi_imx->base + MX51_ECSPI_DMA);
 	}
 
 	spi_imx->dma_finished = 1;
-- 
2.5.2

Re: [PATCH v2 1/8] spi: imx: Fix DMA transfer

[kbuild test robot]

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

Hi Anton,

[auto build test results on v4.3-rc2 -- if it's inappropriate base, please ignore]

config: arm-arm5 (attached as .config)
reproduce:
  wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
  chmod +x ~/bin/make.cross
  git checkout 82f1264caef01600e640af28b9f92d5a86efd25b
  # save the attached .config to linux build tree
  make.cross ARCH=arm 

All error/warnings (new ones prefixed by >>):

>> ERROR: "arm926_dma_flush_range" [drivers/spi/spi-imx.ko] undefined!

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 25917 bytes --]

Re: [PATCH v2 1/8] spi: imx: Fix DMA transfer

[Robin Gong]

On Fri, Sep 25, 2015 at 07:57:08PM +0200, Anton Bondarenko wrote:
> RX DMA tail data handling doesn't work correctly in many cases with
> current implementation. It happens because SPI core was setup
> to generates both RX watermark level and RX DATA TAIL events
> incorrectly. SPI transfer triggering for DMA also done in wrong way.
> 
> SPI client wants to transfer 70 words for example. The old DMA
> implementation setup RX DATA TAIL equal 6 words. In this case
> RX DMA event will be generated after 6 words read from RX FIFO.
> The garbage can be read out from RX FIFO because SPI HW does
> not receive all required words to trigger RX watermark event.
> 
> New implementation change handling of RX data tail. DMA is used to process
> all TX data and only full chunks of RX data with size aligned to FIFO/2.
> Driver is waiting until both TX and RX DMA transaction done and all
> TX data are pushed out. At that moment there is only RX data tail in
> the RX FIFO. This data read out using PIO.
> 
> Transfer triggering changed to avoid RX data loss.
> 
> Signed-off-by: Anton Bondarenko <anton_bondarenko-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/spi/spi-imx.c | 105 +++++++++++++++++++++++++++++++-------------------
>  1 file changed, 66 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index f9deb84..165bc2c 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -39,6 +39,8 @@
>  #include <linux/of_device.h>
>  #include <linux/of_gpio.h>
>  
> +#include <asm/cacheflush.h>
> +
>  #include <linux/platform_data/dma-imx.h>
>  #include <linux/platform_data/spi-imx.h>
>  
> @@ -53,6 +55,7 @@
>  /* generic defines to abstract from the different register layouts */
>  #define MXC_INT_RR	(1 << 0) /* Receive data ready interrupt */
>  #define MXC_INT_TE	(1 << 1) /* Transmit FIFO empty interrupt */
> +#define MXC_INT_TCEN	BIT(7)   /* Transfer complete */
>  
>  /* The maximum  bytes that a sdma BD can transfer.*/
>  #define MAX_SDMA_BD_BYTES  (1 << 15)
> @@ -104,9 +107,7 @@ struct spi_imx_data {
>  	unsigned int dma_is_inited;
>  	unsigned int dma_finished;
>  	bool usedma;
> -	u32 rx_wml;
> -	u32 tx_wml;
> -	u32 rxt_wml;
> +	u32 wml;
>  	struct completion dma_rx_completion;
>  	struct completion dma_tx_completion;
>  
> @@ -201,9 +202,8 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
>  {
>  	struct spi_imx_data *spi_imx = spi_master_get_devdata(master);
>  
> -	if (spi_imx->dma_is_inited
> -	    && transfer->len > spi_imx->rx_wml * sizeof(u32)
> -	    && transfer->len > spi_imx->tx_wml * sizeof(u32))
> +	if (spi_imx->dma_is_inited &&
> +	    (transfer->len > spi_imx->wml * sizeof(u32)))
Add Sascha in the loop. I don't think "* sizeof(u32)", since even 1 byte data
will consume one position of 32bit FIFO Thus if here
spi_imx->wml = spi_imx_get_fifosize(spi_imx) / 2 = 32, the threshold value
which judge DMA mode used or not should be 32 not 32 * 4.
Of course, it will not cause any function break since both DMA and PIO can work
,but I think we'd better correct it.
>  		return true;
>  	return false;
>  }
> @@ -228,6 +228,7 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
>  #define MX51_ECSPI_INT		0x10
>  #define MX51_ECSPI_INT_TEEN		(1 <<  0)
>  #define MX51_ECSPI_INT_RREN		(1 <<  3)
> +#define MX51_ECSPI_INT_TCEN		BIT(7)
>  
>  #define MX51_ECSPI_DMA      0x14
>  #define MX51_ECSPI_DMA_TX_WML_OFFSET	0
> @@ -292,6 +293,9 @@ static void __maybe_unused mx51_ecspi_intctrl(struct spi_imx_data *spi_imx, int
>  	if (enable & MXC_INT_RR)
>  		val |= MX51_ECSPI_INT_RREN;
>  
> +	if (enable & MXC_INT_TCEN)
> +		val |= MX51_ECSPI_INT_TCEN;
> +
>  	writel(val, spi_imx->base + MX51_ECSPI_INT);
>  }
>  
> @@ -311,8 +315,9 @@ static void __maybe_unused mx51_ecspi_trigger(struct spi_imx_data *spi_imx)
>  static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
>  		struct spi_imx_config *config)
>  {
> -	u32 ctrl = MX51_ECSPI_CTRL_ENABLE, cfg = 0, dma = 0;
> -	u32 tx_wml_cfg, rx_wml_cfg, rxt_wml_cfg;
> +	u32 ctrl = MX51_ECSPI_CTRL_ENABLE, dma = 0;
> +	u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
> +
>  	u32 clk = config->speed_hz, delay;
>  
>  	/*
> @@ -369,19 +374,10 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
>  	 * and enable DMA request.
>  	 */
>  	if (spi_imx->dma_is_inited) {
> -		dma = readl(spi_imx->base + MX51_ECSPI_DMA);
> -
> -		spi_imx->rxt_wml = spi_imx_get_fifosize(spi_imx) / 2;
> -		rx_wml_cfg = spi_imx->rx_wml << MX51_ECSPI_DMA_RX_WML_OFFSET;
> -		tx_wml_cfg = spi_imx->tx_wml << MX51_ECSPI_DMA_TX_WML_OFFSET;
> -		rxt_wml_cfg = spi_imx->rxt_wml << MX51_ECSPI_DMA_RXT_WML_OFFSET;
> -		dma = (dma & ~MX51_ECSPI_DMA_TX_WML_MASK
> -			   & ~MX51_ECSPI_DMA_RX_WML_MASK
> -			   & ~MX51_ECSPI_DMA_RXT_WML_MASK)
> -			   | rx_wml_cfg | tx_wml_cfg | rxt_wml_cfg
> -			   |(1 << MX51_ECSPI_DMA_TEDEN_OFFSET)
> -			   |(1 << MX51_ECSPI_DMA_RXDEN_OFFSET)
> -			   |(1 << MX51_ECSPI_DMA_RXTDEN_OFFSET);
> +		dma = (spi_imx->wml - 1) << MX51_ECSPI_DMA_RX_WML_OFFSET
> +		      | (spi_imx->wml - 1) << MX51_ECSPI_DMA_TX_WML_OFFSET
> +		      | (1 << MX51_ECSPI_DMA_TEDEN_OFFSET)
> +		      | (1 << MX51_ECSPI_DMA_RXDEN_OFFSET);
Please set tx threshold as 0 as your v1 patch if I remember right, as our
internal tree done:
http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/commit/drivers/spi/spi-imx.c?h=imx_3.14.28_7d_alpha&id=2e7615e2f399e39c58dd31f84a31f7c2592da7e7
>  
>  		writel(dma, spi_imx->base + MX51_ECSPI_DMA);
>  	}
> @@ -825,6 +821,8 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
>  	if (of_machine_is_compatible("fsl,imx6dl"))
>  		return 0;
>  
> +	spi_imx->wml = spi_imx_get_fifosize(spi_imx) / 2;
> +
>  	/* Prepare for TX DMA: */
>  	master->dma_tx = dma_request_slave_channel(dev, "tx");
>  	if (!master->dma_tx) {
> @@ -836,7 +834,8 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
>  	slave_config.direction = DMA_MEM_TO_DEV;
>  	slave_config.dst_addr = res->start + MXC_CSPITXDATA;
>  	slave_config.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> -	slave_config.dst_maxburst = spi_imx_get_fifosize(spi_imx) / 2;
> +	slave_config.dst_maxburst = spi_imx_get_fifosize(spi_imx)
> +					- spi_imx->wml;
slave_config.dst_maxburst = spi_imx->wml;?
>  	ret = dmaengine_slave_config(master->dma_tx, &slave_config);
>  	if (ret) {
>  		dev_err(dev, "error in TX dma configuration.\n");
> @@ -854,7 +853,8 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
>  	slave_config.direction = DMA_DEV_TO_MEM;
>  	slave_config.src_addr = res->start + MXC_CSPIRXDATA;
>  	slave_config.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> -	slave_config.src_maxburst = spi_imx_get_fifosize(spi_imx) / 2;
> +	slave_config.src_maxburst = spi_imx_get_fifosize(spi_imx)
> +					- spi_imx->wml;
slave_config.src_maxburst = spi_imx->wml;?
>  	ret = dmaengine_slave_config(master->dma_rx, &slave_config);
>  	if (ret) {
>  		dev_err(dev, "error in RX dma configuration.\n");
> @@ -867,8 +867,6 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
>  	master->max_dma_len = MAX_SDMA_BD_BYTES;
>  	spi_imx->bitbang.master->flags = SPI_MASTER_MUST_RX |
>  					 SPI_MASTER_MUST_TX;
> -	spi_imx->tx_wml = spi_imx_get_fifosize(spi_imx) / 2;
> -	spi_imx->rx_wml = spi_imx_get_fifosize(spi_imx) / 2;
>  	spi_imx->dma_is_inited = 1;
>  
>  	return 0;
> @@ -897,8 +895,7 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
>  	struct dma_async_tx_descriptor *desc_tx = NULL, *desc_rx = NULL;
>  	int ret;
>  	unsigned long timeout;
> -	u32 dma;
> -	int left;
> +	const int left = transfer->len % spi_imx->wml;
>  	struct spi_master *master = spi_imx->bitbang.master;
>  	struct sg_table *tx = &transfer->tx_sg, *rx = &transfer->rx_sg;
>  
> @@ -915,9 +912,23 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
>  	}
>  
>  	if (rx) {
> +		/* Cut RX data tail */
> +		const unsigned int old_nents = rx->nents;
> +
> +		WARN_ON(sg_dma_len(&rx->sgl[rx->nents - 1]) < left);
> +		sg_dma_len(&rx->sgl[rx->nents - 1]) -= left;
> +		if (sg_dma_len(&rx->sgl[rx->nents - 1]) == 0)
> +			--rx->nents;
> +
>  		desc_rx = dmaengine_prep_slave_sg(master->dma_rx,
>  					rx->sgl, rx->nents, DMA_DEV_TO_MEM,
>  					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> +
> +		/* Restore old SG table state */
> +		if (old_nents > rx->nents)
> +			++rx->nents;
> +		sg_dma_len(&rx->sgl[rx->nents - 1]) += left;
> +
>  		if (!desc_rx)
>  			goto no_dma;
>  
> @@ -932,17 +943,10 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
>  	/* Trigger the cspi module. */
>  	spi_imx->dma_finished = 0;
>  
> -	dma = readl(spi_imx->base + MX51_ECSPI_DMA);
> -	dma = dma & (~MX51_ECSPI_DMA_RXT_WML_MASK);
> -	/* Change RX_DMA_LENGTH trigger dma fetch tail data */
> -	left = transfer->len % spi_imx->rxt_wml;
> -	if (left)
> -		writel(dma | (left << MX51_ECSPI_DMA_RXT_WML_OFFSET),
> -				spi_imx->base + MX51_ECSPI_DMA);
> +	dma_async_issue_pending(master->dma_rx);
> +	dma_async_issue_pending(master->dma_tx);
>  	spi_imx->devtype_data->trigger(spi_imx);
>  
> -	dma_async_issue_pending(master->dma_tx);
> -	dma_async_issue_pending(master->dma_rx);
why change the sequence of issue_pending and trigger? I don't think need to do so.
>  	/* Wait SDMA to finish the data transfer.*/
>  	timeout = wait_for_completion_timeout(&spi_imx->dma_tx_completion,
>  						IMX_DMA_TIMEOUT);
> @@ -951,6 +955,7 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
>  			dev_driver_string(&master->dev),
>  			dev_name(&master->dev));
>  		dmaengine_terminate_all(master->dma_tx);
> +		dmaengine_terminate_all(master->dma_rx);
>  	} else {
>  		timeout = wait_for_completion_timeout(
>  				&spi_imx->dma_rx_completion, IMX_DMA_TIMEOUT);
> @@ -960,10 +965,32 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
>  				dev_name(&master->dev));
>  			spi_imx->devtype_data->reset(spi_imx);
>  			dmaengine_terminate_all(master->dma_rx);
> +		} else if (left) {
> +			void *pio_buffer = transfer->rx_buf
> +						+ (transfer->len - left);
> +
> +			dma_sync_sg_for_cpu(master->dma_rx->device->dev,
> +					    rx->sgl, rx->nents,
> +					    DMA_FROM_DEVICE);
Only the last entry needed:
dma_sync_sg_for_cpu(master->dma_rx->device->dev,
			rx->sgl[rx->nents - 1], 1,
			DMA_FROM_DEVICE);
> +
> +			spi_imx->rx_buf = pio_buffer;
> +			spi_imx->txfifo = left;
> +			reinit_completion(&spi_imx->xfer_done);
> +
> +			spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TCEN);
> +
> +			timeout = wait_for_completion_timeout(
> +					&spi_imx->xfer_done, IMX_DMA_TIMEOUT);
> +			if (!timeout) {
> +				pr_warn("%s %s: I/O Error in RX tail\n",
> +					dev_driver_string(&master->dev),
> +					dev_name(&master->dev));
> +			}
> +
> +			dmac_flush_range(pio_buffer, pio_buffer + left);
> +			outer_flush_range(virt_to_phys(pio_buffer),
> +					  virt_to_phys(pio_buffer) + left);
>  		}
> -		writel(dma |
> -		       spi_imx->rxt_wml << MX51_ECSPI_DMA_RXT_WML_OFFSET,
> -		       spi_imx->base + MX51_ECSPI_DMA);
>  	}
>  
>  	spi_imx->dma_finished = 1;
> -- 
> 2.5.2
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/8] spi: imx: Fix DMA transfer

[Bondarenko, Anton]

>> @@ -201,9 +202,8 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
>>  {
>>  	struct spi_imx_data *spi_imx = spi_master_get_devdata(master);
>>  
>> -	if (spi_imx->dma_is_inited
>> -	    && transfer->len > spi_imx->rx_wml * sizeof(u32)
>> -	    && transfer->len > spi_imx->tx_wml * sizeof(u32))
>> +	if (spi_imx->dma_is_inited &&
>> +	    (transfer->len > spi_imx->wml * sizeof(u32)))
> Add Sascha in the loop. I don't think "* sizeof(u32)", since even 1 byte data
> will consume one position of 32bit FIFO Thus if here
> spi_imx->wml = spi_imx_get_fifosize(spi_imx) / 2 = 32, the threshold value
> which judge DMA mode used or not should be 32 not 32 * 4.
> Of course, it will not cause any function break since both DMA and PIO can work
> ,but I think we'd better correct it.
I agree, in case of 1 byte SPI word we do not need to multiply by 4.
But for 16 bit and 32 bit SPI words it's necessary. This part is
addressed in patch 3.
I could remove "* sizeof(u32)" for now.
>>  		return true;
>>  	return false;
>>  }
>> @@ -369,19 +374,10 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
>>  	 * and enable DMA request.
>>  	 */
>>  	if (spi_imx->dma_is_inited) {
>> -		dma = readl(spi_imx->base + MX51_ECSPI_DMA);
>> -
>> -		spi_imx->rxt_wml = spi_imx_get_fifosize(spi_imx) / 2;
>> -		rx_wml_cfg = spi_imx->rx_wml << MX51_ECSPI_DMA_RX_WML_OFFSET;
>> -		tx_wml_cfg = spi_imx->tx_wml << MX51_ECSPI_DMA_TX_WML_OFFSET;
>> -		rxt_wml_cfg = spi_imx->rxt_wml << MX51_ECSPI_DMA_RXT_WML_OFFSET;
>> -		dma = (dma & ~MX51_ECSPI_DMA_TX_WML_MASK
>> -			   & ~MX51_ECSPI_DMA_RX_WML_MASK
>> -			   & ~MX51_ECSPI_DMA_RXT_WML_MASK)
>> -			   | rx_wml_cfg | tx_wml_cfg | rxt_wml_cfg
>> -			   |(1 << MX51_ECSPI_DMA_TEDEN_OFFSET)
>> -			   |(1 << MX51_ECSPI_DMA_RXDEN_OFFSET)
>> -			   |(1 << MX51_ECSPI_DMA_RXTDEN_OFFSET);
>> +		dma = (spi_imx->wml - 1) << MX51_ECSPI_DMA_RX_WML_OFFSET
>> +		      | (spi_imx->wml - 1) << MX51_ECSPI_DMA_TX_WML_OFFSET
>> +		      | (1 << MX51_ECSPI_DMA_TEDEN_OFFSET)
>> +		      | (1 << MX51_ECSPI_DMA_RXDEN_OFFSET);
> Please set tx threshold as 0 as your v1 patch if I remember right, as our
> internal tree done:
> http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/commit/drivers/spi/spi-imx.c?h=imx_3.14.28_7d_alpha&id=2e7615e2f399e39c58dd31f84a31f7c2592da7e7
Will be fixed in V3 patchset
>>  
>>  		writel(dma, spi_imx->base + MX51_ECSPI_DMA);
>>  	}
>> @@ -825,6 +821,8 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
>>  	if (of_machine_is_compatible("fsl,imx6dl"))
>>  		return 0;
>>  
>> +	spi_imx->wml = spi_imx_get_fifosize(spi_imx) / 2;
>> +
>>  	/* Prepare for TX DMA: */
>>  	master->dma_tx = dma_request_slave_channel(dev, "tx");
>>  	if (!master->dma_tx) {
>> @@ -836,7 +834,8 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
>>  	slave_config.direction = DMA_MEM_TO_DEV;
>>  	slave_config.dst_addr = res->start + MXC_CSPITXDATA;
>>  	slave_config.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
>> -	slave_config.dst_maxburst = spi_imx_get_fifosize(spi_imx) / 2;
>> +	slave_config.dst_maxburst = spi_imx_get_fifosize(spi_imx)
>> +					- spi_imx->wml;
> slave_config.dst_maxburst = spi_imx->wml;?
Will be fixed in V3
>>  	ret = dmaengine_slave_config(master->dma_tx, &slave_config);
>>  	if (ret) {
>>  		dev_err(dev, "error in TX dma configuration.\n");
>> @@ -854,7 +853,8 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
>>  	slave_config.direction = DMA_DEV_TO_MEM;
>>  	slave_config.src_addr = res->start + MXC_CSPIRXDATA;
>>  	slave_config.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
>> -	slave_config.src_maxburst = spi_imx_get_fifosize(spi_imx) / 2;
>> +	slave_config.src_maxburst = spi_imx_get_fifosize(spi_imx)
>> +					- spi_imx->wml;
> slave_config.src_maxburst = spi_imx->wml;?
Will be fixed in V3
>>  	ret = dmaengine_slave_config(master->dma_rx, &slave_config);
>>  	if (ret) {
>>  		dev_err(dev, "error in RX dma configuration.\n");
>> @@ -867,8 +867,6 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
>>  	master->max_dma_len = MAX_SDMA_BD_BYTES;
>>  	spi_imx->bitbang.master->flags = SPI_MASTER_MUST_RX |
>>  					 SPI_MASTER_MUST_TX;
>> -	spi_imx->tx_wml = spi_imx_get_fifosize(spi_imx) / 2;
>> -	spi_imx->rx_wml = spi_imx_get_fifosize(spi_imx) / 2;
>>  	spi_imx->dma_is_inited = 1;
>>  
>>  	return 0;
>> @@ -897,8 +895,7 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
>>  	struct dma_async_tx_descriptor *desc_tx = NULL, *desc_rx = NULL;
>>  	int ret;
>>  	unsigned long timeout;
>> -	u32 dma;
>> -	int left;
>> +	const int left = transfer->len % spi_imx->wml;
>>  	struct spi_master *master = spi_imx->bitbang.master;
>>  	struct sg_table *tx = &transfer->tx_sg, *rx = &transfer->rx_sg;
>>  
>> @@ -915,9 +912,23 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
>>  	}
>>  
>>  	if (rx) {
>> +		/* Cut RX data tail */
>> +		const unsigned int old_nents = rx->nents;
>> +
>> +		WARN_ON(sg_dma_len(&rx->sgl[rx->nents - 1]) < left);
>> +		sg_dma_len(&rx->sgl[rx->nents - 1]) -= left;
>> +		if (sg_dma_len(&rx->sgl[rx->nents - 1]) == 0)
>> +			--rx->nents;
>> +
>>  		desc_rx = dmaengine_prep_slave_sg(master->dma_rx,
>>  					rx->sgl, rx->nents, DMA_DEV_TO_MEM,
>>  					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>> +
>> +		/* Restore old SG table state */
>> +		if (old_nents > rx->nents)
>> +			++rx->nents;
>> +		sg_dma_len(&rx->sgl[rx->nents - 1]) += left;
>> +
>>  		if (!desc_rx)
>>  			goto no_dma;
>>  
>> @@ -932,17 +943,10 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
>>  	/* Trigger the cspi module. */
>>  	spi_imx->dma_finished = 0;
>>  
>> -	dma = readl(spi_imx->base + MX51_ECSPI_DMA);
>> -	dma = dma & (~MX51_ECSPI_DMA_RXT_WML_MASK);
>> -	/* Change RX_DMA_LENGTH trigger dma fetch tail data */
>> -	left = transfer->len % spi_imx->rxt_wml;
>> -	if (left)
>> -		writel(dma | (left << MX51_ECSPI_DMA_RXT_WML_OFFSET),
>> -				spi_imx->base + MX51_ECSPI_DMA);
>> +	dma_async_issue_pending(master->dma_rx);
>> +	dma_async_issue_pending(master->dma_tx);
>>  	spi_imx->devtype_data->trigger(spi_imx);
>>  
>> -	dma_async_issue_pending(master->dma_tx);
>> -	dma_async_issue_pending(master->dma_rx);
> why change the sequence of issue_pending and trigger? I don't think need to do so.
The reason for order change for TX/RX requests is avoiding buffer
overflow for RX. This will happen if our code will be interrupted after
SPI HW and TX DMA started. This mean we will sent TX data, but there is
no one to consume RX data. So RX DMA should start before TX DMA.
On other hand TX DMA should start work earlier to fill buffer before SPI
HW starts pushing data out. This will give us a small performance bonus.
Not a big one, but still something for free.
>>  	/* Wait SDMA to finish the data transfer.*/
>>  	timeout = wait_for_completion_timeout(&spi_imx->dma_tx_completion,
>>  						IMX_DMA_TIMEOUT);
>> @@ -951,6 +955,7 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
>>  			dev_driver_string(&master->dev),
>>  			dev_name(&master->dev));
>>  		dmaengine_terminate_all(master->dma_tx);
>> +		dmaengine_terminate_all(master->dma_rx);
>>  	} else {
>>  		timeout = wait_for_completion_timeout(
>>  				&spi_imx->dma_rx_completion, IMX_DMA_TIMEOUT);
>> @@ -960,10 +965,32 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
>>  				dev_name(&master->dev));
>>  			spi_imx->devtype_data->reset(spi_imx);
>>  			dmaengine_terminate_all(master->dma_rx);
>> +		} else if (left) {
>> +			void *pio_buffer = transfer->rx_buf
>> +						+ (transfer->len - left);
>> +
>> +			dma_sync_sg_for_cpu(master->dma_rx->device->dev,
>> +					    rx->sgl, rx->nents,
>> +					    DMA_FROM_DEVICE);
> Only the last entry needed:
> dma_sync_sg_for_cpu(master->dma_rx->device->dev,
> 			rx->sgl[rx->nents - 1], 1,
> 			DMA_FROM_DEVICE);
Agree. Will be fixed in V3
>> +
>> +			spi_imx->rx_buf = pio_buffer;
>> +			spi_imx->txfifo = left;
>> +			reinit_completion(&spi_imx->xfer_done);
>> +
>> +			spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TCEN);
>> +
>> +			timeout = wait_for_completion_timeout(
>> +					&spi_imx->xfer_done, IMX_DMA_TIMEOUT);
>> +			if (!timeout) {
>> +				pr_warn("%s %s: I/O Error in RX tail\n",
>> +					dev_driver_string(&master->dev),
>> +					dev_name(&master->dev));
>> +			}
>> +
>> +			dmac_flush_range(pio_buffer, pio_buffer + left);
The line above causing build error in some configurations. Replacing it
with dma_sync_sg call similar to previous one, but with
>> +			outer_flush_range(virt_to_phys(pio_buffer),
>> +					  virt_to_phys(pio_buffer) + left);
>>  		}
>> -		writel(dma |
>> -		       spi_imx->rxt_wml << MX51_ECSPI_DMA_RXT_WML_OFFSET,
>> -		       spi_imx->base + MX51_ECSPI_DMA);
>>  	}
>>  
>>  	spi_imx->dma_finished = 1;
>> -- 
>> 2.5.2
>>

Re: [PATCH v2 1/8] spi: imx: Fix DMA transfer

[Robin Gong]

On Thu, Oct 01, 2015 at 12:02:41AM +0000, Bondarenko, Anton wrote:
> >> @@ -201,9 +202,8 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
> >>  {
> >>  	struct spi_imx_data *spi_imx = spi_master_get_devdata(master);
> >>  
> >> -	if (spi_imx->dma_is_inited
> >> -	    && transfer->len > spi_imx->rx_wml * sizeof(u32)
> >> -	    && transfer->len > spi_imx->tx_wml * sizeof(u32))
> >> +	if (spi_imx->dma_is_inited &&
> >> +	    (transfer->len > spi_imx->wml * sizeof(u32)))
> > Add Sascha in the loop. I don't think "* sizeof(u32)", since even 1 byte data
> > will consume one position of 32bit FIFO Thus if here
> > spi_imx->wml = spi_imx_get_fifosize(spi_imx) / 2 = 32, the threshold value
> > which judge DMA mode used or not should be 32 not 32 * 4.
> > Of course, it will not cause any function break since both DMA and PIO can work
> > ,but I think we'd better correct it.
> I agree, in case of 1 byte SPI word we do not need to multiply by 4.
> But for 16 bit and 32 bit SPI words it's necessary. This part is
> addressed in patch 3.
> I could remove "* sizeof(u32)" for now.
I still think don't need *sizeof(u32) even for 16bit and 32bit, whatever bits
used as one spi word(<32bits), one spi word consume one position of SPI FIFO
(32bit).
> >>  		return true;
> >>  	return false;
> >>  }
> >> @@ -369,19 +374,10 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
> >>  	 * and enable DMA request.
> >>  	 */
> >>  	if (spi_imx->dma_is_inited) {
> >> -		dma = readl(spi_imx->base + MX51_ECSPI_DMA);
> >> -
> >> -		spi_imx->rxt_wml = spi_imx_get_fifosize(spi_imx) / 2;
> >> -		rx_wml_cfg = spi_imx->rx_wml << MX51_ECSPI_DMA_RX_WML_OFFSET;
> >> -		tx_wml_cfg = spi_imx->tx_wml << MX51_ECSPI_DMA_TX_WML_OFFSET;
> >> -		rxt_wml_cfg = spi_imx->rxt_wml << MX51_ECSPI_DMA_RXT_WML_OFFSET;
> >> -		dma = (dma & ~MX51_ECSPI_DMA_TX_WML_MASK
> >> -			   & ~MX51_ECSPI_DMA_RX_WML_MASK
> >> -			   & ~MX51_ECSPI_DMA_RXT_WML_MASK)
> >> -			   | rx_wml_cfg | tx_wml_cfg | rxt_wml_cfg
> >> -			   |(1 << MX51_ECSPI_DMA_TEDEN_OFFSET)
> >> -			   |(1 << MX51_ECSPI_DMA_RXDEN_OFFSET)
> >> -			   |(1 << MX51_ECSPI_DMA_RXTDEN_OFFSET);
> >> +		dma = (spi_imx->wml - 1) << MX51_ECSPI_DMA_RX_WML_OFFSET
> >> +		      | (spi_imx->wml - 1) << MX51_ECSPI_DMA_TX_WML_OFFSET
> >> +		      | (1 << MX51_ECSPI_DMA_TEDEN_OFFSET)
> >> +		      | (1 << MX51_ECSPI_DMA_RXDEN_OFFSET);
> > Please set tx threshold as 0 as your v1 patch if I remember right, as our
> > internal tree done:
> > http://git.freescale.com/git/cgit.cgi/imx/linux-2.6-imx.git/commit/drivers/spi/spi-imx.c?h=imx_3.14.28_7d_alpha&id=2e7615e2f399e39c58dd31f84a31f7c2592da7e7
> Will be fixed in V3 patchset
> >>  
> >>  		writel(dma, spi_imx->base + MX51_ECSPI_DMA);
> >>  	}
> >> @@ -825,6 +821,8 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
> >>  	if (of_machine_is_compatible("fsl,imx6dl"))
> >>  		return 0;
> >>  
> >> +	spi_imx->wml = spi_imx_get_fifosize(spi_imx) / 2;
> >> +
> >>  	/* Prepare for TX DMA: */
> >>  	master->dma_tx = dma_request_slave_channel(dev, "tx");
> >>  	if (!master->dma_tx) {
> >> @@ -836,7 +834,8 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
> >>  	slave_config.direction = DMA_MEM_TO_DEV;
> >>  	slave_config.dst_addr = res->start + MXC_CSPITXDATA;
> >>  	slave_config.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> >> -	slave_config.dst_maxburst = spi_imx_get_fifosize(spi_imx) / 2;
> >> +	slave_config.dst_maxburst = spi_imx_get_fifosize(spi_imx)
> >> +					- spi_imx->wml;
> > slave_config.dst_maxburst = spi_imx->wml;?
> Will be fixed in V3
> >>  	ret = dmaengine_slave_config(master->dma_tx, &slave_config);
> >>  	if (ret) {
> >>  		dev_err(dev, "error in TX dma configuration.\n");
> >> @@ -854,7 +853,8 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
> >>  	slave_config.direction = DMA_DEV_TO_MEM;
> >>  	slave_config.src_addr = res->start + MXC_CSPIRXDATA;
> >>  	slave_config.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> >> -	slave_config.src_maxburst = spi_imx_get_fifosize(spi_imx) / 2;
> >> +	slave_config.src_maxburst = spi_imx_get_fifosize(spi_imx)
> >> +					- spi_imx->wml;
> > slave_config.src_maxburst = spi_imx->wml;?
> Will be fixed in V3
> >>  	ret = dmaengine_slave_config(master->dma_rx, &slave_config);
> >>  	if (ret) {
> >>  		dev_err(dev, "error in RX dma configuration.\n");
> >> @@ -867,8 +867,6 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
> >>  	master->max_dma_len = MAX_SDMA_BD_BYTES;
> >>  	spi_imx->bitbang.master->flags = SPI_MASTER_MUST_RX |
> >>  					 SPI_MASTER_MUST_TX;
> >> -	spi_imx->tx_wml = spi_imx_get_fifosize(spi_imx) / 2;
> >> -	spi_imx->rx_wml = spi_imx_get_fifosize(spi_imx) / 2;
> >>  	spi_imx->dma_is_inited = 1;
> >>  
> >>  	return 0;
> >> @@ -897,8 +895,7 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
> >>  	struct dma_async_tx_descriptor *desc_tx = NULL, *desc_rx = NULL;
> >>  	int ret;
> >>  	unsigned long timeout;
> >> -	u32 dma;
> >> -	int left;
> >> +	const int left = transfer->len % spi_imx->wml;
> >>  	struct spi_master *master = spi_imx->bitbang.master;
> >>  	struct sg_table *tx = &transfer->tx_sg, *rx = &transfer->rx_sg;
> >>  
> >> @@ -915,9 +912,23 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
> >>  	}
> >>  
> >>  	if (rx) {
> >> +		/* Cut RX data tail */
> >> +		const unsigned int old_nents = rx->nents;
> >> +
> >> +		WARN_ON(sg_dma_len(&rx->sgl[rx->nents - 1]) < left);
> >> +		sg_dma_len(&rx->sgl[rx->nents - 1]) -= left;
> >> +		if (sg_dma_len(&rx->sgl[rx->nents - 1]) == 0)
> >> +			--rx->nents;
> >> +
> >>  		desc_rx = dmaengine_prep_slave_sg(master->dma_rx,
> >>  					rx->sgl, rx->nents, DMA_DEV_TO_MEM,
> >>  					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> >> +
> >> +		/* Restore old SG table state */
> >> +		if (old_nents > rx->nents)
> >> +			++rx->nents;
> >> +		sg_dma_len(&rx->sgl[rx->nents - 1]) += left;
> >> +
> >>  		if (!desc_rx)
> >>  			goto no_dma;
> >>  
> >> @@ -932,17 +943,10 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
> >>  	/* Trigger the cspi module. */
> >>  	spi_imx->dma_finished = 0;
> >>  
> >> -	dma = readl(spi_imx->base + MX51_ECSPI_DMA);
> >> -	dma = dma & (~MX51_ECSPI_DMA_RXT_WML_MASK);
> >> -	/* Change RX_DMA_LENGTH trigger dma fetch tail data */
> >> -	left = transfer->len % spi_imx->rxt_wml;
> >> -	if (left)
> >> -		writel(dma | (left << MX51_ECSPI_DMA_RXT_WML_OFFSET),
> >> -				spi_imx->base + MX51_ECSPI_DMA);
> >> +	dma_async_issue_pending(master->dma_rx);
> >> +	dma_async_issue_pending(master->dma_tx);
> >>  	spi_imx->devtype_data->trigger(spi_imx);
> >>  
> >> -	dma_async_issue_pending(master->dma_tx);
> >> -	dma_async_issue_pending(master->dma_rx);
> > why change the sequence of issue_pending and trigger? I don't think need to do so.
> The reason for order change for TX/RX requests is avoiding buffer
> overflow for RX. This will happen if our code will be interrupted after
> SPI HW and TX DMA started. This mean we will sent TX data, but there is
> no one to consume RX data. So RX DMA should start before TX DMA.
> On other hand TX DMA should start work earlier to fill buffer before SPI
> HW starts pushing data out. This will give us a small performance bonus.
> Not a big one, but still something for free.
> >>  	/* Wait SDMA to finish the data transfer.*/
> >>  	timeout = wait_for_completion_timeout(&spi_imx->dma_tx_completion,
> >>  						IMX_DMA_TIMEOUT);
> >> @@ -951,6 +955,7 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
> >>  			dev_driver_string(&master->dev),
> >>  			dev_name(&master->dev));
> >>  		dmaengine_terminate_all(master->dma_tx);
> >> +		dmaengine_terminate_all(master->dma_rx);
> >>  	} else {
> >>  		timeout = wait_for_completion_timeout(
> >>  				&spi_imx->dma_rx_completion, IMX_DMA_TIMEOUT);
> >> @@ -960,10 +965,32 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
> >>  				dev_name(&master->dev));
> >>  			spi_imx->devtype_data->reset(spi_imx);
> >>  			dmaengine_terminate_all(master->dma_rx);
> >> +		} else if (left) {
> >> +			void *pio_buffer = transfer->rx_buf
> >> +						+ (transfer->len - left);
> >> +
> >> +			dma_sync_sg_for_cpu(master->dma_rx->device->dev,
> >> +					    rx->sgl, rx->nents,
> >> +					    DMA_FROM_DEVICE);
> > Only the last entry needed:
> > dma_sync_sg_for_cpu(master->dma_rx->device->dev,
> > 			rx->sgl[rx->nents - 1], 1,
> > 			DMA_FROM_DEVICE);
> Agree. Will be fixed in V3
> >> +
> >> +			spi_imx->rx_buf = pio_buffer;
> >> +			spi_imx->txfifo = left;
> >> +			reinit_completion(&spi_imx->xfer_done);
> >> +
> >> +			spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TCEN);
> >> +
> >> +			timeout = wait_for_completion_timeout(
> >> +					&spi_imx->xfer_done, IMX_DMA_TIMEOUT);
> >> +			if (!timeout) {
> >> +				pr_warn("%s %s: I/O Error in RX tail\n",
> >> +					dev_driver_string(&master->dev),
> >> +					dev_name(&master->dev));
> >> +			}
> >> +
> >> +			dmac_flush_range(pio_buffer, pio_buffer + left);
> The line above causing build error in some configurations. Replacing it
> with dma_sync_sg call similar to previous one, but with
> >> +			outer_flush_range(virt_to_phys(pio_buffer),
> >> +					  virt_to_phys(pio_buffer) + left);
> >>  		}
> >> -		writel(dma |
> >> -		       spi_imx->rxt_wml << MX51_ECSPI_DMA_RXT_WML_OFFSET,
> >> -		       spi_imx->base + MX51_ECSPI_DMA);
> >>  	}
> >>  
> >>  	spi_imx->dma_finished = 1;
> >> -- 
> >> 2.5.2
> >>
> 

Re: [PATCH v2 1/8] spi: imx: Fix DMA transfer

[Anton Bondarenko]

On 08.10.2015 11:19, Robin Gong wrote:
> On Thu, Oct 01, 2015 at 12:02:41AM +0000, Bondarenko, Anton wrote:
>>>> @@ -201,9 +202,8 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
>>>>   {
>>>>   	struct spi_imx_data *spi_imx = spi_master_get_devdata(master);
>>>>
>>>> -	if (spi_imx->dma_is_inited
>>>> -	    && transfer->len > spi_imx->rx_wml * sizeof(u32)
>>>> -	    && transfer->len > spi_imx->tx_wml * sizeof(u32))
>>>> +	if (spi_imx->dma_is_inited &&
>>>> +	    (transfer->len > spi_imx->wml * sizeof(u32)))
>>> Add Sascha in the loop. I don't think "* sizeof(u32)", since even 1 byte data
>>> will consume one position of 32bit FIFO Thus if here
>>> spi_imx->wml = spi_imx_get_fifosize(spi_imx) / 2 = 32, the threshold value
>>> which judge DMA mode used or not should be 32 not 32 * 4.
>>> Of course, it will not cause any function break since both DMA and PIO can work
>>> ,but I think we'd better correct it.
>> I agree, in case of 1 byte SPI word we do not need to multiply by 4.
>> But for 16 bit and 32 bit SPI words it's necessary. This part is
>> addressed in patch 3.
>> I could remove "* sizeof(u32)" for now.
> I still think don't need *sizeof(u32) even for 16bit and 32bit, whatever bits
> used as one spi word(<32bits), one spi word consume one position of SPI FIFO
> (32bit).
Will be removed in V3 for this patch.
>>>>   		return true;
>>>>   	return false;
>>>>   }

Regards,
Anton
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 2/8] spi: imx: replace fixed timeout with calculated one

[Anton Bondarenko]

Fixed timeout value can fire while transaction is ongoing. This may happen
because there are no strict requirements on SPI transaction duration.
Dynamic timeout value is generated based on SCLK and transaction size.

There is also 4 * SCLK delay between TX bursts related to CS change.

Signed-off-by: Anton Bondarenko <anton_bondarenko@mentor.com>
---
 drivers/spi/spi-imx.c | 49 +++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 41 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 165bc2c..6c98eda 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -59,7 +59,6 @@
 
 /* The maximum  bytes that a sdma BD can transfer.*/
 #define MAX_SDMA_BD_BYTES  (1 << 15)
-#define IMX_DMA_TIMEOUT (msecs_to_jiffies(3000))
 struct spi_imx_config {
 	unsigned int speed_hz;
 	unsigned int bpw;
@@ -95,6 +94,7 @@ struct spi_imx_data {
 	struct clk *clk_per;
 	struct clk *clk_ipg;
 	unsigned long spi_clk;
+	unsigned int spi_bus_clk;
 
 	unsigned int count;
 	void (*tx)(struct spi_imx_data *);
@@ -317,8 +317,7 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
 {
 	u32 ctrl = MX51_ECSPI_CTRL_ENABLE, dma = 0;
 	u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
-
-	u32 clk = config->speed_hz, delay;
+	u32 delay;
 
 	/*
 	 * The hardware seems to have a race condition when changing modes. The
@@ -330,7 +329,9 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
 	ctrl |= MX51_ECSPI_CTRL_MODE_MASK;
 
 	/* set clock speed */
-	ctrl |= mx51_ecspi_clkdiv(spi_imx->spi_clk, config->speed_hz, &clk);
+	spi_imx->spi_bus_clk = config->speed_hz;
+	ctrl |= mx51_ecspi_clkdiv(spi_imx->spi_clk, config->speed_hz,
+				  &spi_imx->spi_bus_clk);
 
 	/* set chip select to use */
 	ctrl |= MX51_ECSPI_CTRL_CS(config->cs);
@@ -363,7 +364,7 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
 	 * the SPI communication as the device on the other end would consider
 	 * the change of SCLK polarity as a clock tick already.
 	 */
-	delay = (2 * 1000000) / clk;
+	delay = (2 * USEC_PER_SEC) / spi_imx->spi_bus_clk;
 	if (likely(delay < 10))	/* SCLK is faster than 100 kHz */
 		udelay(delay);
 	else			/* SCLK is _very_ slow */
@@ -889,12 +890,40 @@ static void spi_imx_dma_tx_callback(void *cookie)
 	complete(&spi_imx->dma_tx_completion);
 }
 
+static int spi_imx_calculate_timeout(struct spi_imx_data *spi_imx, int size)
+{
+	unsigned long coef1 = 1;
+	unsigned long coef2 = MSEC_PER_SEC;
+	unsigned long timeout = 0;
+
+	/* Swap coeficients to avoid div by 0 */
+	if (spi_imx->spi_bus_clk < MSEC_PER_SEC) {
+		coef1 = MSEC_PER_SEC;
+		coef2 = 1;
+	}
+
+	/* Time with actual data transfer */
+	timeout += DIV_ROUND_UP(8 * size * coef1,
+				spi_imx->spi_bus_clk / coef2);
+
+	/* Take CS change delay related to HW */
+	timeout += DIV_ROUND_UP((size - 1) * 4 * coef1,
+				spi_imx->spi_bus_clk / coef2);
+
+	/* Add extra second for scheduler related activities */
+	timeout += MSEC_PER_SEC;
+
+	/* Double calculated timeout */
+	return msecs_to_jiffies(2 * timeout);
+}
+
 static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
 				struct spi_transfer *transfer)
 {
 	struct dma_async_tx_descriptor *desc_tx = NULL, *desc_rx = NULL;
 	int ret;
 	unsigned long timeout;
+	unsigned long transfer_timeout;
 	const int left = transfer->len % spi_imx->wml;
 	struct spi_master *master = spi_imx->bitbang.master;
 	struct sg_table *tx = &transfer->tx_sg, *rx = &transfer->rx_sg;
@@ -947,9 +976,11 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
 	dma_async_issue_pending(master->dma_tx);
 	spi_imx->devtype_data->trigger(spi_imx);
 
+	transfer_timeout = spi_imx_calculate_timeout(spi_imx, transfer->len);
+
 	/* Wait SDMA to finish the data transfer.*/
 	timeout = wait_for_completion_timeout(&spi_imx->dma_tx_completion,
-						IMX_DMA_TIMEOUT);
+						transfer_timeout);
 	if (!timeout) {
 		pr_warn("%s %s: I/O Error in DMA TX\n",
 			dev_driver_string(&master->dev),
@@ -957,8 +988,10 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
 		dmaengine_terminate_all(master->dma_tx);
 		dmaengine_terminate_all(master->dma_rx);
 	} else {
+		transfer_timeout = spi_imx_calculate_timeout(spi_imx,
+							     spi_imx->wml * 2);
 		timeout = wait_for_completion_timeout(
-				&spi_imx->dma_rx_completion, IMX_DMA_TIMEOUT);
+				&spi_imx->dma_rx_completion, transfer_timeout);
 		if (!timeout) {
 			pr_warn("%s %s: I/O Error in DMA RX\n",
 				dev_driver_string(&master->dev),
@@ -980,7 +1013,7 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
 			spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TCEN);
 
 			timeout = wait_for_completion_timeout(
-					&spi_imx->xfer_done, IMX_DMA_TIMEOUT);
+					&spi_imx->xfer_done, transfer_timeout);
 			if (!timeout) {
 				pr_warn("%s %s: I/O Error in RX tail\n",
 					dev_driver_string(&master->dev),
-- 
2.5.2

Re: [PATCH v2 2/8] spi: imx: replace fixed timeout with calculated one

[Robin Gong]

On Fri, Sep 25, 2015 at 07:57:09PM +0200, Anton Bondarenko wrote:
> Fixed timeout value can fire while transaction is ongoing. This may happen
> because there are no strict requirements on SPI transaction duration.
> Dynamic timeout value is generated based on SCLK and transaction size.
> 
> There is also 4 * SCLK delay between TX bursts related to CS change.
> 
> Signed-off-by: Anton Bondarenko <anton_bondarenko@mentor.com>
> ---
>  drivers/spi/spi-imx.c | 49 +++++++++++++++++++++++++++++++++++++++++--------
>  1 file changed, 41 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 165bc2c..6c98eda 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -59,7 +59,6 @@
>  
>  /* The maximum  bytes that a sdma BD can transfer.*/
>  #define MAX_SDMA_BD_BYTES  (1 << 15)
> -#define IMX_DMA_TIMEOUT (msecs_to_jiffies(3000))
>  struct spi_imx_config {
>  	unsigned int speed_hz;
>  	unsigned int bpw;
> @@ -95,6 +94,7 @@ struct spi_imx_data {
>  	struct clk *clk_per;
>  	struct clk *clk_ipg;
>  	unsigned long spi_clk;
> +	unsigned int spi_bus_clk;
>  
>  	unsigned int count;
>  	void (*tx)(struct spi_imx_data *);
> @@ -317,8 +317,7 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
>  {
>  	u32 ctrl = MX51_ECSPI_CTRL_ENABLE, dma = 0;
>  	u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
> -
> -	u32 clk = config->speed_hz, delay;
> +	u32 delay;
>  
>  	/*
>  	 * The hardware seems to have a race condition when changing modes. The
> @@ -330,7 +329,9 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
>  	ctrl |= MX51_ECSPI_CTRL_MODE_MASK;
>  
>  	/* set clock speed */
> -	ctrl |= mx51_ecspi_clkdiv(spi_imx->spi_clk, config->speed_hz, &clk);
> +	spi_imx->spi_bus_clk = config->speed_hz;
> +	ctrl |= mx51_ecspi_clkdiv(spi_imx->spi_clk, config->speed_hz,
> +				  &spi_imx->spi_bus_clk);
>  
>  	/* set chip select to use */
>  	ctrl |= MX51_ECSPI_CTRL_CS(config->cs);
> @@ -363,7 +364,7 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
>  	 * the SPI communication as the device on the other end would consider
>  	 * the change of SCLK polarity as a clock tick already.
>  	 */
> -	delay = (2 * 1000000) / clk;
> +	delay = (2 * USEC_PER_SEC) / spi_imx->spi_bus_clk;
>  	if (likely(delay < 10))	/* SCLK is faster than 100 kHz */
>  		udelay(delay);
>  	else			/* SCLK is _very_ slow */
> @@ -889,12 +890,40 @@ static void spi_imx_dma_tx_callback(void *cookie)
>  	complete(&spi_imx->dma_tx_completion);
>  }
>  
> +static int spi_imx_calculate_timeout(struct spi_imx_data *spi_imx, int size)
> +{
> +	unsigned long coef1 = 1;
> +	unsigned long coef2 = MSEC_PER_SEC;
> +	unsigned long timeout = 0;
> +
> +	/* Swap coeficients to avoid div by 0 */
> +	if (spi_imx->spi_bus_clk < MSEC_PER_SEC) {
> +		coef1 = MSEC_PER_SEC;
> +		coef2 = 1;
> +	}
> +
> +	/* Time with actual data transfer */
> +	timeout += DIV_ROUND_UP(8 * size * coef1,
> +				spi_imx->spi_bus_clk / coef2);
> +
> +	/* Take CS change delay related to HW */
> +	timeout += DIV_ROUND_UP((size - 1) * 4 * coef1,
> +				spi_imx->spi_bus_clk / coef2);
> +
> +	/* Add extra second for scheduler related activities */
> +	timeout += MSEC_PER_SEC;
> +
> +	/* Double calculated timeout */
> +	return msecs_to_jiffies(2 * timeout);
> +}
> +
>  static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
>  				struct spi_transfer *transfer)
>  {
>  	struct dma_async_tx_descriptor *desc_tx = NULL, *desc_rx = NULL;
>  	int ret;
>  	unsigned long timeout;
> +	unsigned long transfer_timeout;
>  	const int left = transfer->len % spi_imx->wml;
>  	struct spi_master *master = spi_imx->bitbang.master;
>  	struct sg_table *tx = &transfer->tx_sg, *rx = &transfer->rx_sg;
> @@ -947,9 +976,11 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
>  	dma_async_issue_pending(master->dma_tx);
>  	spi_imx->devtype_data->trigger(spi_imx);
>  
> +	transfer_timeout = spi_imx_calculate_timeout(spi_imx, transfer->len);
> +
>  	/* Wait SDMA to finish the data transfer.*/
>  	timeout = wait_for_completion_timeout(&spi_imx->dma_tx_completion,
> -						IMX_DMA_TIMEOUT);
> +						transfer_timeout);
>  	if (!timeout) {
>  		pr_warn("%s %s: I/O Error in DMA TX\n",
>  			dev_driver_string(&master->dev),
> @@ -957,8 +988,10 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
>  		dmaengine_terminate_all(master->dma_tx);
>  		dmaengine_terminate_all(master->dma_rx);
>  	} else {
> +		transfer_timeout = spi_imx_calculate_timeout(spi_imx,
> +							     spi_imx->wml * 2);
*2 again here? Although spi_imx_calculate_timeout has double it.
>  		timeout = wait_for_completion_timeout(
> -				&spi_imx->dma_rx_completion, IMX_DMA_TIMEOUT);
> +				&spi_imx->dma_rx_completion, transfer_timeout);
>  		if (!timeout) {
>  			pr_warn("%s %s: I/O Error in DMA RX\n",
>  				dev_driver_string(&master->dev),
> @@ -980,7 +1013,7 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
>  			spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TCEN);
>  
>  			timeout = wait_for_completion_timeout(
> -					&spi_imx->xfer_done, IMX_DMA_TIMEOUT);
> +					&spi_imx->xfer_done, transfer_timeout);
>  			if (!timeout) {
>  				pr_warn("%s %s: I/O Error in RX tail\n",
>  					dev_driver_string(&master->dev),
> -- 
> 2.5.2
> 

Re: [PATCH v2 2/8] spi: imx: replace fixed timeout with calculated one

[Bondarenko, Anton]

On 30.09.2015 10:31, Robin Gong wrote:
> On Fri, Sep 25, 2015 at 07:57:09PM +0200, Anton Bondarenko wrote:
>> @@ -957,8 +988,10 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
>>  		dmaengine_terminate_all(master->dma_tx);
>>  		dmaengine_terminate_all(master->dma_rx);
>>  	} else {
>> +		transfer_timeout = spi_imx_calculate_timeout(spi_imx,
>> +							     spi_imx->wml * 2);
> *2 again here? Although spi_imx_calculate_timeout has double it.
WIll be fixed in V3
>>  		timeout = wait_for_completion_timeout(
>> -				&spi_imx->dma_rx_completion, IMX_DMA_TIMEOUT);
>> +				&spi_imx->dma_rx_completion, transfer_timeout);
>>  		if (!timeout) {
>>  			pr_warn("%s %s: I/O Error in DMA RX\n",
>>  				dev_driver_string(&master->dev),

[PATCH v2 3/8] spi: imx: add support for all SPI word width for DMA transfer

[Anton Bondarenko]

DMA transfer for SPI was limited to up to 8 bits word size until now.
Sync in SPI burst size and DMA bus width is necessary to correctly
support other BPW supported by HW.

Signed-off-by: Anton Bondarenko <anton_bondarenko@mentor.com>
---
 drivers/spi/spi-imx.c | 121 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 87 insertions(+), 34 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 6c98eda..d9b730d 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -91,11 +91,15 @@ struct spi_imx_data {
 
 	struct completion xfer_done;
 	void __iomem *base;
+	unsigned long base_phys;
+
 	struct clk *clk_per;
 	struct clk *clk_ipg;
 	unsigned long spi_clk;
 	unsigned int spi_bus_clk;
 
+	unsigned int bpw_w;
+
 	unsigned int count;
 	void (*tx)(struct spi_imx_data *);
 	void (*rx)(struct spi_imx_data *);
@@ -201,9 +205,13 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
 			 struct spi_transfer *transfer)
 {
 	struct spi_imx_data *spi_imx = spi_master_get_devdata(master);
+	unsigned int bpw_w = transfer->bits_per_word;
 
-	if (spi_imx->dma_is_inited &&
-	    (transfer->len > spi_imx->wml * sizeof(u32)))
+	if (!bpw_w)
+		bpw_w = spi->bits_per_word;
+
+	bpw_w = DIV_ROUND_UP(bpw_w, 8);
+	if (spi_imx->dma_is_inited && (transfer->len > spi_imx->wml * bpw_w))
 		return true;
 	return false;
 }
@@ -761,11 +769,62 @@ static irqreturn_t spi_imx_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int spi_imx_sdma_configure(struct spi_master *master)
+{
+	int ret;
+	enum dma_slave_buswidth dsb_default = DMA_SLAVE_BUSWIDTH_1_BYTE;
+	struct dma_slave_config slave_config = {};
+	struct spi_imx_data *spi_imx = spi_master_get_devdata(master);
+
+	switch (spi_imx->bpw_w) {
+	case 4:
+		dsb_default = DMA_SLAVE_BUSWIDTH_4_BYTES;
+		break;
+	case 2:
+		dsb_default = DMA_SLAVE_BUSWIDTH_2_BYTES;
+		break;
+	case 1:
+		dsb_default = DMA_SLAVE_BUSWIDTH_1_BYTE;
+		break;
+	default:
+		pr_err("Not supported word size %d\n", spi_imx->bpw_w);
+		ret = -EINVAL;
+		goto err;
+	}
+
+	slave_config.direction = DMA_MEM_TO_DEV;
+	slave_config.dst_addr = spi_imx->base_phys + MXC_CSPITXDATA;
+	slave_config.dst_addr_width = dsb_default;
+	slave_config.dst_maxburst = spi_imx_get_fifosize(spi_imx)
+					- spi_imx->wml;
+	ret = dmaengine_slave_config(master->dma_tx, &slave_config);
+	if (ret) {
+		pr_err("error in TX dma configuration.\n");
+		goto err;
+	}
+
+	memset(&slave_config, 0, sizeof(slave_config));
+
+	slave_config.direction = DMA_DEV_TO_MEM;
+	slave_config.src_addr = spi_imx->base_phys + MXC_CSPIRXDATA;
+	slave_config.src_addr_width = dsb_default;
+	slave_config.src_maxburst = spi_imx_get_fifosize(spi_imx)
+					- spi_imx->wml;
+	ret = dmaengine_slave_config(master->dma_rx, &slave_config);
+	if (ret)
+		pr_err("error in RX dma configuration.\n");
+
+err:
+	return ret;
+}
+
 static int spi_imx_setupxfer(struct spi_device *spi,
 				 struct spi_transfer *t)
 {
 	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
 	struct spi_imx_config config;
+	unsigned int bpw_w_new;
+	int ret = 0;
 
 	config.bpw = t ? t->bits_per_word : spi->bits_per_word;
 	config.speed_hz  = t ? t->speed_hz : spi->max_speed_hz;
@@ -789,9 +848,18 @@ static int spi_imx_setupxfer(struct spi_device *spi,
 		spi_imx->tx = spi_imx_buf_tx_u32;
 	}
 
-	spi_imx->devtype_data->config(spi_imx, &config);
+	bpw_w_new = DIV_ROUND_UP(config.bpw, 8);
+	if (spi_imx->dma_is_inited && spi_imx->bpw_w != bpw_w_new) {
+		spi_imx->bpw_w = bpw_w_new;
+		ret = spi_imx_sdma_configure(spi->master);
+		if (ret != 0)
+			pr_err("Can't configure SDMA, error %d\n", ret);
+	}
 
-	return 0;
+	if (!ret)
+		ret = spi_imx->devtype_data->config(spi_imx, &config);
+
+	return ret;
 }
 
 static void spi_imx_sdma_exit(struct spi_imx_data *spi_imx)
@@ -812,10 +880,8 @@ static void spi_imx_sdma_exit(struct spi_imx_data *spi_imx)
 }
 
 static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
-			     struct spi_master *master,
-			     const struct resource *res)
+			     struct spi_master *master)
 {
-	struct dma_slave_config slave_config = {};
 	int ret;
 
 	/* use pio mode for i.mx6dl chip TKT238285 */
@@ -832,17 +898,6 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
 		goto err;
 	}
 
-	slave_config.direction = DMA_MEM_TO_DEV;
-	slave_config.dst_addr = res->start + MXC_CSPITXDATA;
-	slave_config.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
-	slave_config.dst_maxburst = spi_imx_get_fifosize(spi_imx)
-					- spi_imx->wml;
-	ret = dmaengine_slave_config(master->dma_tx, &slave_config);
-	if (ret) {
-		dev_err(dev, "error in TX dma configuration.\n");
-		goto err;
-	}
-
 	/* Prepare for RX : */
 	master->dma_rx = dma_request_slave_channel(dev, "rx");
 	if (!master->dma_rx) {
@@ -851,23 +906,19 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
 		goto err;
 	}
 
-	slave_config.direction = DMA_DEV_TO_MEM;
-	slave_config.src_addr = res->start + MXC_CSPIRXDATA;
-	slave_config.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
-	slave_config.src_maxburst = spi_imx_get_fifosize(spi_imx)
-					- spi_imx->wml;
-	ret = dmaengine_slave_config(master->dma_rx, &slave_config);
-	if (ret) {
-		dev_err(dev, "error in RX dma configuration.\n");
-		goto err;
-	}
-
 	init_completion(&spi_imx->dma_rx_completion);
 	init_completion(&spi_imx->dma_tx_completion);
 	master->can_dma = spi_imx_can_dma;
 	master->max_dma_len = MAX_SDMA_BD_BYTES;
 	spi_imx->bitbang.master->flags = SPI_MASTER_MUST_RX |
 					 SPI_MASTER_MUST_TX;
+	spi_imx->bpw_w = 1;
+	ret = spi_imx_sdma_configure(master);
+	if (ret) {
+		dev_info(dev, "cannot get setup DMA.\n");
+		goto err;
+	}
+
 	spi_imx->dma_is_inited = 1;
 
 	return 0;
@@ -924,7 +975,7 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
 	int ret;
 	unsigned long timeout;
 	unsigned long transfer_timeout;
-	const int left = transfer->len % spi_imx->wml;
+	const int left = transfer->len % (spi_imx->wml * spi_imx->bpw_w);
 	struct spi_master *master = spi_imx->bitbang.master;
 	struct sg_table *tx = &transfer->tx_sg, *rx = &transfer->rx_sg;
 
@@ -989,7 +1040,7 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
 		dmaengine_terminate_all(master->dma_rx);
 	} else {
 		transfer_timeout = spi_imx_calculate_timeout(spi_imx,
-							     spi_imx->wml * 2);
+					spi_imx->bpw_w * spi_imx->wml * 2);
 		timeout = wait_for_completion_timeout(
 				&spi_imx->dma_rx_completion, transfer_timeout);
 		if (!timeout) {
@@ -1007,7 +1058,8 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
 					    DMA_FROM_DEVICE);
 
 			spi_imx->rx_buf = pio_buffer;
-			spi_imx->txfifo = left;
+			spi_imx->txfifo = DIV_ROUND_UP(left, spi_imx->bpw_w);
+
 			reinit_completion(&spi_imx->xfer_done);
 
 			spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TCEN);
@@ -1207,6 +1259,7 @@ static int spi_imx_probe(struct platform_device *pdev)
 		ret = PTR_ERR(spi_imx->base);
 		goto out_master_put;
 	}
+	spi_imx->base_phys = res->start;
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
@@ -1246,8 +1299,8 @@ static int spi_imx_probe(struct platform_device *pdev)
 	 * Only validated on i.mx6 now, can remove the constrain if validated on
 	 * other chips.
 	 */
-	if (spi_imx->devtype_data == &imx51_ecspi_devtype_data
-	    && spi_imx_sdma_init(&pdev->dev, spi_imx, master, res))
+	if (spi_imx->devtype_data == &imx51_ecspi_devtype_data &&
+	    spi_imx_sdma_init(&pdev->dev, spi_imx, master))
 		dev_err(&pdev->dev, "dma setup error,use pio instead\n");
 
 	spi_imx->devtype_data->reset(spi_imx);
-- 
2.5.2

Re: [PATCH v2 3/8] spi: imx: add support for all SPI word width for DMA transfer

[Robin Gong]

On Fri, Sep 25, 2015 at 07:57:10PM +0200, Anton Bondarenko wrote:
> DMA transfer for SPI was limited to up to 8 bits word size until now.
> Sync in SPI burst size and DMA bus width is necessary to correctly
> support other BPW supported by HW.
> 
> Signed-off-by: Anton Bondarenko <anton_bondarenko@mentor.com>
> ---
>  drivers/spi/spi-imx.c | 121 ++++++++++++++++++++++++++++++++++++--------------
>  1 file changed, 87 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 6c98eda..d9b730d 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -91,11 +91,15 @@ struct spi_imx_data {
>  
>  	struct completion xfer_done;
>  	void __iomem *base;
> +	unsigned long base_phys;
> +
>  	struct clk *clk_per;
>  	struct clk *clk_ipg;
>  	unsigned long spi_clk;
>  	unsigned int spi_bus_clk;
>  
> +	unsigned int bpw_w;
> +
It's better to change bytes_per_word for clear understanding,since bpw in spi means
bits_per_word...
>  	unsigned int count;
>  	void (*tx)(struct spi_imx_data *);
>  	void (*rx)(struct spi_imx_data *);
> @@ -201,9 +205,13 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
>  			 struct spi_transfer *transfer)
>  {
>  	struct spi_imx_data *spi_imx = spi_master_get_devdata(master);
> +	unsigned int bpw_w = transfer->bits_per_word;
>  
> -	if (spi_imx->dma_is_inited &&
> -	    (transfer->len > spi_imx->wml * sizeof(u32)))
> +	if (!bpw_w)
> +		bpw_w = spi->bits_per_word;
> +
> +	bpw_w = DIV_ROUND_UP(bpw_w, 8);
> +	if (spi_imx->dma_is_inited && (transfer->len > spi_imx->wml * bpw_w))
Please remove bpw_w here as I talked in the first patch.
>  		return true;
>  	return false;
>  }
> @@ -761,11 +769,62 @@ static irqreturn_t spi_imx_isr(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +static int spi_imx_sdma_configure(struct spi_master *master)
> +{
> +	int ret;
> +	enum dma_slave_buswidth dsb_default = DMA_SLAVE_BUSWIDTH_1_BYTE;
> +	struct dma_slave_config slave_config = {};
> +	struct spi_imx_data *spi_imx = spi_master_get_devdata(master);
> +
> +	switch (spi_imx->bpw_w) {
> +	case 4:
> +		dsb_default = DMA_SLAVE_BUSWIDTH_4_BYTES;
> +		break;
> +	case 2:
> +		dsb_default = DMA_SLAVE_BUSWIDTH_2_BYTES;
> +		break;
> +	case 1:
> +		dsb_default = DMA_SLAVE_BUSWIDTH_1_BYTE;
> +		break;
> +	default:
> +		pr_err("Not supported word size %d\n", spi_imx->bpw_w);
> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	slave_config.direction = DMA_MEM_TO_DEV;
> +	slave_config.dst_addr = spi_imx->base_phys + MXC_CSPITXDATA;
> +	slave_config.dst_addr_width = dsb_default;
> +	slave_config.dst_maxburst = spi_imx_get_fifosize(spi_imx)
> +					- spi_imx->wml;
> +	ret = dmaengine_slave_config(master->dma_tx, &slave_config);
> +	if (ret) {
> +		pr_err("error in TX dma configuration.\n");
> +		goto err;
> +	}
> +
> +	memset(&slave_config, 0, sizeof(slave_config));
> +
> +	slave_config.direction = DMA_DEV_TO_MEM;
> +	slave_config.src_addr = spi_imx->base_phys + MXC_CSPIRXDATA;
> +	slave_config.src_addr_width = dsb_default;
> +	slave_config.src_maxburst = spi_imx_get_fifosize(spi_imx)
> +					- spi_imx->wml;
> +	ret = dmaengine_slave_config(master->dma_rx, &slave_config);
> +	if (ret)
> +		pr_err("error in RX dma configuration.\n");
> +
> +err:
> +	return ret;
> +}
> +
>  static int spi_imx_setupxfer(struct spi_device *spi,
>  				 struct spi_transfer *t)
>  {
>  	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
>  	struct spi_imx_config config;
> +	unsigned int bpw_w_new;
> +	int ret = 0;
>  
>  	config.bpw = t ? t->bits_per_word : spi->bits_per_word;
>  	config.speed_hz  = t ? t->speed_hz : spi->max_speed_hz;
> @@ -789,9 +848,18 @@ static int spi_imx_setupxfer(struct spi_device *spi,
>  		spi_imx->tx = spi_imx_buf_tx_u32;
>  	}
>  
> -	spi_imx->devtype_data->config(spi_imx, &config);
> +	bpw_w_new = DIV_ROUND_UP(config.bpw, 8);
> +	if (spi_imx->dma_is_inited && spi_imx->bpw_w != bpw_w_new) {
> +		spi_imx->bpw_w = bpw_w_new;
> +		ret = spi_imx_sdma_configure(spi->master);
> +		if (ret != 0)
> +			pr_err("Can't configure SDMA, error %d\n", ret);
> +	}
>  
> -	return 0;
> +	if (!ret)
> +		ret = spi_imx->devtype_data->config(spi_imx, &config);
> +
> +	return ret;
>  }
>  
>  static void spi_imx_sdma_exit(struct spi_imx_data *spi_imx)
> @@ -812,10 +880,8 @@ static void spi_imx_sdma_exit(struct spi_imx_data *spi_imx)
>  }
>  
>  static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
> -			     struct spi_master *master,
> -			     const struct resource *res)
> +			     struct spi_master *master)
>  {
> -	struct dma_slave_config slave_config = {};
>  	int ret;
>  
>  	/* use pio mode for i.mx6dl chip TKT238285 */
> @@ -832,17 +898,6 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
>  		goto err;
>  	}
>  
> -	slave_config.direction = DMA_MEM_TO_DEV;
> -	slave_config.dst_addr = res->start + MXC_CSPITXDATA;
> -	slave_config.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> -	slave_config.dst_maxburst = spi_imx_get_fifosize(spi_imx)
> -					- spi_imx->wml;
> -	ret = dmaengine_slave_config(master->dma_tx, &slave_config);
> -	if (ret) {
> -		dev_err(dev, "error in TX dma configuration.\n");
> -		goto err;
> -	}
> -
>  	/* Prepare for RX : */
>  	master->dma_rx = dma_request_slave_channel(dev, "rx");
>  	if (!master->dma_rx) {
> @@ -851,23 +906,19 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
>  		goto err;
>  	}
>  
> -	slave_config.direction = DMA_DEV_TO_MEM;
> -	slave_config.src_addr = res->start + MXC_CSPIRXDATA;
> -	slave_config.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> -	slave_config.src_maxburst = spi_imx_get_fifosize(spi_imx)
> -					- spi_imx->wml;
> -	ret = dmaengine_slave_config(master->dma_rx, &slave_config);
> -	if (ret) {
> -		dev_err(dev, "error in RX dma configuration.\n");
> -		goto err;
> -	}
> -
>  	init_completion(&spi_imx->dma_rx_completion);
>  	init_completion(&spi_imx->dma_tx_completion);
>  	master->can_dma = spi_imx_can_dma;
>  	master->max_dma_len = MAX_SDMA_BD_BYTES;
>  	spi_imx->bitbang.master->flags = SPI_MASTER_MUST_RX |
>  					 SPI_MASTER_MUST_TX;
> +	spi_imx->bpw_w = 1;
> +	ret = spi_imx_sdma_configure(master);
> +	if (ret) {
> +		dev_info(dev, "cannot get setup DMA.\n");
> +		goto err;
> +	}
> +
>  	spi_imx->dma_is_inited = 1;
>  
>  	return 0;
> @@ -924,7 +975,7 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
>  	int ret;
>  	unsigned long timeout;
>  	unsigned long transfer_timeout;
> -	const int left = transfer->len % spi_imx->wml;
> +	const int left = transfer->len % (spi_imx->wml * spi_imx->bpw_w);
>  	struct spi_master *master = spi_imx->bitbang.master;
>  	struct sg_table *tx = &transfer->tx_sg, *rx = &transfer->rx_sg;
>  
> @@ -989,7 +1040,7 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
>  		dmaengine_terminate_all(master->dma_rx);
>  	} else {
>  		transfer_timeout = spi_imx_calculate_timeout(spi_imx,
> -							     spi_imx->wml * 2);
> +					spi_imx->bpw_w * spi_imx->wml * 2);
>  		timeout = wait_for_completion_timeout(
>  				&spi_imx->dma_rx_completion, transfer_timeout);
>  		if (!timeout) {
> @@ -1007,7 +1058,8 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
>  					    DMA_FROM_DEVICE);
>  
>  			spi_imx->rx_buf = pio_buffer;
> -			spi_imx->txfifo = left;
> +			spi_imx->txfifo = DIV_ROUND_UP(left, spi_imx->bpw_w);
> +
Looks not enough here, you have to set 'spi_imx->rx' per the right bpw.
>  			reinit_completion(&spi_imx->xfer_done);
>  
>  			spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TCEN);
> @@ -1207,6 +1259,7 @@ static int spi_imx_probe(struct platform_device *pdev)
>  		ret = PTR_ERR(spi_imx->base);
>  		goto out_master_put;
>  	}
> +	spi_imx->base_phys = res->start;
>  
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0) {
> @@ -1246,8 +1299,8 @@ static int spi_imx_probe(struct platform_device *pdev)
>  	 * Only validated on i.mx6 now, can remove the constrain if validated on
>  	 * other chips.
>  	 */
> -	if (spi_imx->devtype_data == &imx51_ecspi_devtype_data
> -	    && spi_imx_sdma_init(&pdev->dev, spi_imx, master, res))
> +	if (spi_imx->devtype_data == &imx51_ecspi_devtype_data &&
> +	    spi_imx_sdma_init(&pdev->dev, spi_imx, master))
>  		dev_err(&pdev->dev, "dma setup error,use pio instead\n");
>  
>  	spi_imx->devtype_data->reset(spi_imx);
> -- 
> 2.5.2
> 

Re: [PATCH v2 3/8] spi: imx: add support for all SPI word width for DMA transfer

[Bondarenko, Anton]

On 30.09.2015 10:35, Robin Gong wrote:
> On Fri, Sep 25, 2015 at 07:57:10PM +0200, Anton Bondarenko wrote:
>> @@ -91,11 +91,15 @@ struct spi_imx_data {
>>  
>>  	struct completion xfer_done;
>>  	void __iomem *base;
>> +	unsigned long base_phys;
>> +
>>  	struct clk *clk_per;
>>  	struct clk *clk_ipg;
>>  	unsigned long spi_clk;
>>  	unsigned int spi_bus_clk;
>>  
>> +	unsigned int bpw_w;
>> +
> It's better to change bytes_per_word for clear understanding,since bpw in spi means
> bits_per_word...
Agree. Fixed in V3
>>  	unsigned int count;
>>  	void (*tx)(struct spi_imx_data *);
>>  	void (*rx)(struct spi_imx_data *);
>> @@ -201,9 +205,13 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
>>  			 struct spi_transfer *transfer)
>>  {
>>  	struct spi_imx_data *spi_imx = spi_master_get_devdata(master);
>> +	unsigned int bpw_w = transfer->bits_per_word;
>>  
>> -	if (spi_imx->dma_is_inited &&
>> -	    (transfer->len > spi_imx->wml * sizeof(u32)))
>> +	if (!bpw_w)
>> +		bpw_w = spi->bits_per_word;
>> +
>> +	bpw_w = DIV_ROUND_UP(bpw_w, 8);
>> +	if (spi_imx->dma_is_inited && (transfer->len > spi_imx->wml * bpw_w))
> Please remove bpw_w here as I talked in the first patch.
As explained in patch1 we need to use SPI word size in calculation to decide 
if we want to go with DMA or PIO mode. Just a short example:
We need to transfer 24 SPI words with BPW == 32. This will take ~ 24 PIO writes
to FIFO (and same for reads). But transfer->len will be 24*4=96 bytes.
WML is 32 SPI words. The decision will be incorrect if we do not take into account
SPI bits per word.  
>>  		return true;
>>  	return false;
>>  }
>> @@ -1007,7 +1058,8 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
>>  					    DMA_FROM_DEVICE);
>>  
>>  			spi_imx->rx_buf = pio_buffer;
>> -			spi_imx->txfifo = left;
>> +			spi_imx->txfifo = DIV_ROUND_UP(left, spi_imx->bpw_w);
>> +
> Looks not enough here, you have to set 'spi_imx->rx' per the right bpw.
What do you mean? We have had bpw_w == 1 before so 
spi_imx->txfifo = DIV_ROUND_UP(left, spi_imx->bpw_w);
is equivalent to
spi_imx->txfifo = left;
Now we could have bpw_w equal to 2 or 4 also. txfifo is number of SPI words
and not number of bytes.
>>  			reinit_completion(&spi_imx->xfer_done);
>>  
>>  			spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TCEN);

Re: [PATCH v2 3/8] spi: imx: add support for all SPI word width for DMA transfer

[Robin Gong]

On Thu, Oct 01, 2015 at 12:34:54AM +0000, Bondarenko, Anton wrote:
> On 30.09.2015 10:35, Robin Gong wrote:
> > On Fri, Sep 25, 2015 at 07:57:10PM +0200, Anton Bondarenko wrote:
> >> @@ -91,11 +91,15 @@ struct spi_imx_data {
> >>  
> >>  	struct completion xfer_done;
> >>  	void __iomem *base;
> >> +	unsigned long base_phys;
> >> +
> >>  	struct clk *clk_per;
> >>  	struct clk *clk_ipg;
> >>  	unsigned long spi_clk;
> >>  	unsigned int spi_bus_clk;
> >>  
> >> +	unsigned int bpw_w;
> >> +
> > It's better to change bytes_per_word for clear understanding,since bpw in spi means
> > bits_per_word...
> Agree. Fixed in V3
> >>  	unsigned int count;
> >>  	void (*tx)(struct spi_imx_data *);
> >>  	void (*rx)(struct spi_imx_data *);
> >> @@ -201,9 +205,13 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
> >>  			 struct spi_transfer *transfer)
> >>  {
> >>  	struct spi_imx_data *spi_imx = spi_master_get_devdata(master);
> >> +	unsigned int bpw_w = transfer->bits_per_word;
> >>  
> >> -	if (spi_imx->dma_is_inited &&
> >> -	    (transfer->len > spi_imx->wml * sizeof(u32)))
> >> +	if (!bpw_w)
> >> +		bpw_w = spi->bits_per_word;
> >> +
> >> +	bpw_w = DIV_ROUND_UP(bpw_w, 8);
> >> +	if (spi_imx->dma_is_inited && (transfer->len > spi_imx->wml * bpw_w))
> > Please remove bpw_w here as I talked in the first patch.
> As explained in patch1 we need to use SPI word size in calculation to decide 
> if we want to go with DMA or PIO mode. Just a short example:
> We need to transfer 24 SPI words with BPW == 32. This will take ~ 24 PIO writes
> to FIFO (and same for reads). But transfer->len will be 24*4=96 bytes.
> WML is 32 SPI words. The decision will be incorrect if we do not take into account
> SPI bits per word. 
Yes, you are right, but I'm thinking so many 'DIV_ROUND_UP()'in your patch to
get bpw, can you centralize it or just use the 'bpw_w' in 'struct spi_imx_data'?
> >>  		return true;
> >>  	return false;
> >>  }
> >> @@ -1007,7 +1058,8 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
> >>  					    DMA_FROM_DEVICE);
> >>  
> >>  			spi_imx->rx_buf = pio_buffer;
> >> -			spi_imx->txfifo = left;
> >> +			spi_imx->txfifo = DIV_ROUND_UP(left, spi_imx->bpw_w);
> >> +
> > Looks not enough here, you have to set 'spi_imx->rx' per the right bpw.
> What do you mean? We have had bpw_w == 1 before so 
> spi_imx->txfifo = DIV_ROUND_UP(left, spi_imx->bpw_w);
> is equivalent to
> spi_imx->txfifo = left;
> Now we could have bpw_w equal to 2 or 4 also. txfifo is number of SPI words
> and not number of bytes.
Sorry, please ignore this comment, since 'spi_imx->rx' will be correctly set
per the right bpw in spi_imx_setupxfer whatever in DMA or PIO mode.
> >>  			reinit_completion(&spi_imx->xfer_done);
> >>  
> >>  			spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TCEN);

Re: [PATCH v2 3/8] spi: imx: add support for all SPI word width for DMA transfer

[Anton Bondarenko]

On 08.10.2015 11:51, Robin Gong wrote:
> On Thu, Oct 01, 2015 at 12:34:54AM +0000, Bondarenko, Anton wrote:
>> On 30.09.2015 10:35, Robin Gong wrote:
>>> On Fri, Sep 25, 2015 at 07:57:10PM +0200, Anton Bondarenko wrote:
>>>> @@ -91,11 +91,15 @@ struct spi_imx_data {
>>>>
>>>>   	struct completion xfer_done;
>>>>   	void __iomem *base;
>>>> +	unsigned long base_phys;
>>>> +
>>>>   	struct clk *clk_per;
>>>>   	struct clk *clk_ipg;
>>>>   	unsigned long spi_clk;
>>>>   	unsigned int spi_bus_clk;
>>>>
>>>> +	unsigned int bpw_w;
>>>> +
>>> It's better to change bytes_per_word for clear understanding,since bpw in spi means
>>> bits_per_word...
>> Agree. Fixed in V3
>>>>   	unsigned int count;
>>>>   	void (*tx)(struct spi_imx_data *);
>>>>   	void (*rx)(struct spi_imx_data *);
>>>> @@ -201,9 +205,13 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
>>>>   			 struct spi_transfer *transfer)
>>>>   {
>>>>   	struct spi_imx_data *spi_imx = spi_master_get_devdata(master);
>>>> +	unsigned int bpw_w = transfer->bits_per_word;
>>>>
>>>> -	if (spi_imx->dma_is_inited &&
>>>> -	    (transfer->len > spi_imx->wml * sizeof(u32)))
>>>> +	if (!bpw_w)
>>>> +		bpw_w = spi->bits_per_word;
>>>> +
>>>> +	bpw_w = DIV_ROUND_UP(bpw_w, 8);
>>>> +	if (spi_imx->dma_is_inited && (transfer->len > spi_imx->wml * bpw_w))
>>> Please remove bpw_w here as I talked in the first patch.
>> As explained in patch1 we need to use SPI word size in calculation to decide
>> if we want to go with DMA or PIO mode. Just a short example:
>> We need to transfer 24 SPI words with BPW == 32. This will take ~ 24 PIO writes
>> to FIFO (and same for reads). But transfer->len will be 24*4=96 bytes.
>> WML is 32 SPI words. The decision will be incorrect if we do not take into account
>> SPI bits per word.
> Yes, you are right, but I'm thinking so many 'DIV_ROUND_UP()'in your patch to
> get bpw, can you centralize it or just use the 'bpw_w' in 'struct spi_imx_data'?
We can't use bpw_w from spi_imx_data because this is external callback 
for SPI core and driver does not know the purpose of this call. We also 
can't update current bpw_w for the same reason.

I'll add a function like this:
int spi_imx_get_bytes_per_word(int bpw)
{
     return DIV_ROUND_UP(bpw, BITS_PER_BYTE);
}

>>>>   		return true;
>>>>   	return false;
>>>>   }

Regards,
Anton

[PATCH v2 4/8] spi: imx: add selection for iMX53 and iMX6 controller type

[Anton Bondarenko]

ECSPI contorller for iMX53 and iMX6 has few hardware issues in slave
mode and (32*n+1) SPI word size handling comparing to iMX51.
The change add possibility to detect the SPI controller is use and apply
workarounds/limitations.
Documentation for device tree bindings updated

Signed-off-by: Anton Bondarenko <anton_bondarenko@mentor.com>
---
 .../devicetree/bindings/spi/fsl-imx-cspi.txt       |  2 ++
 drivers/spi/spi-imx.c                              | 36 ++++++++++++++++++++--
 2 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt b/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
index 523341a..425485f 100644
--- a/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
+++ b/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
@@ -9,6 +9,8 @@ Required properties:
   - "fsl,imx31-cspi" for SPI compatible with the one integrated on i.MX31
   - "fsl,imx35-cspi" for SPI compatible with the one integrated on i.MX35
   - "fsl,imx51-ecspi" for SPI compatible with the one integrated on i.MX51
+  - "fsl,imx53-ecspi" for SPI compatible with the one integrated on i.MX53
+  - "fsl,imx6q-ecspi" for SPI compatible with the one integrated on i.MX6 family
 - reg : Offset and length of the register set for the device
 - interrupts : Should contain CSPI/eCSPI interrupt
 - fsl,spi-num-chipselects : Contains the number of the chipselect
diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index d9b730d..41c9cef 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -72,7 +72,8 @@ enum spi_imx_devtype {
 	IMX27_CSPI,
 	IMX31_CSPI,
 	IMX35_CSPI,	/* CSPI on all i.mx except above */
-	IMX51_ECSPI,	/* ECSPI on i.mx51 and later */
+	IMX51_ECSPI,	/* ECSPI on i.mx51 */
+	IMX53_ECSPI,	/* ECSPI on i.mx53 and later */
 };
 
 struct spi_imx_data;
@@ -129,9 +130,20 @@ static inline int is_imx35_cspi(struct spi_imx_data *d)
 	return d->devtype_data->devtype == IMX35_CSPI;
 }
 
+static inline int is_imx53_ecspi(struct spi_imx_data *d)
+{
+	return d->devtype_data->devtype == IMX53_ECSPI;
+}
+
+static inline int is_imx5x_ecspi(struct spi_imx_data *d)
+{
+	return d->devtype_data->devtype == IMX51_ECSPI ||
+	       d->devtype_data->devtype == IMX53_ECSPI;
+}
+
 static inline unsigned spi_imx_get_fifosize(struct spi_imx_data *d)
 {
-	return (d->devtype_data->devtype == IMX51_ECSPI) ? 64 : 8;
+	return is_imx5x_ecspi(d) ? 64 : 8;
 }
 
 #define MXC_SPI_BUF_RX(type)						\
@@ -680,6 +692,16 @@ static struct spi_imx_devtype_data imx51_ecspi_devtype_data = {
 	.devtype = IMX51_ECSPI,
 };
 
+static struct spi_imx_devtype_data imx53_ecspi_devtype_data = {
+	/* i.mx53 and later ecspi shares the functions with i.mx51 one */
+	.intctrl = mx51_ecspi_intctrl,
+	.config = mx51_ecspi_config,
+	.trigger = mx51_ecspi_trigger,
+	.rx_available = mx51_ecspi_rx_available,
+	.reset = mx51_ecspi_reset,
+	.devtype = IMX53_ECSPI,
+};
+
 static const struct platform_device_id spi_imx_devtype[] = {
 	{
 		.name = "imx1-cspi",
@@ -697,6 +719,12 @@ static const struct platform_device_id spi_imx_devtype[] = {
 		.name = "imx35-cspi",
 		.driver_data = (kernel_ulong_t) &imx35_cspi_devtype_data,
 	}, {
+		.name = "imx53-ecspi",
+		.driver_data = (kernel_ulong_t)&imx53_ecspi_devtype_data,
+	}, {
+		.name = "imx6q-ecspi",
+		.driver_data = (kernel_ulong_t)&imx53_ecspi_devtype_data,
+	}, {
 		.name = "imx51-ecspi",
 		.driver_data = (kernel_ulong_t) &imx51_ecspi_devtype_data,
 	}, {
@@ -710,6 +738,8 @@ static const struct of_device_id spi_imx_dt_ids[] = {
 	{ .compatible = "fsl,imx27-cspi", .data = &imx27_cspi_devtype_data, },
 	{ .compatible = "fsl,imx31-cspi", .data = &imx31_cspi_devtype_data, },
 	{ .compatible = "fsl,imx35-cspi", .data = &imx35_cspi_devtype_data, },
+	{ .compatible = "fsl,imx53-ecspi", .data = &imx53_ecspi_devtype_data, },
+	{ .compatible = "fsl,imx6q-ecspi", .data = &imx53_ecspi_devtype_data, },
 	{ .compatible = "fsl,imx51-ecspi", .data = &imx51_ecspi_devtype_data, },
 	{ /* sentinel */ }
 };
@@ -1299,7 +1329,7 @@ static int spi_imx_probe(struct platform_device *pdev)
 	 * Only validated on i.mx6 now, can remove the constrain if validated on
 	 * other chips.
 	 */
-	if (spi_imx->devtype_data == &imx51_ecspi_devtype_data &&
+	if (is_imx5x_ecspi(spi_imx) &&
 	    spi_imx_sdma_init(&pdev->dev, spi_imx, master))
 		dev_err(&pdev->dev, "dma setup error,use pio instead\n");
 
-- 
2.5.2

[PATCH v2 5/8] spi: imx: Add support for loopback for ECSPI controllers

[Anton Bondarenko]

Support for ECSPI loopback for IMX51,IMX53 and IMX6Q using TEST register.

Signed-off-by: Mohsin Kazmi <mohsin_kazmi@mentor.com>
Signed-off-by: Anton Bondarenko <anton_bondarenko@mentor.com>
---
 drivers/spi/spi-imx.c | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 41c9cef..4b3c16e 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -265,6 +265,9 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
 #define MX51_ECSPI_STAT		0x18
 #define MX51_ECSPI_STAT_RR		(1 <<  3)
 
+#define MX51_ECSPI_TEST			0x20
+#define MX51_ECSPI_LOOP			BIT(31)
+
 /* MX51 eCSPI */
 static unsigned int mx51_ecspi_clkdiv(unsigned int fin, unsigned int fspi,
 				      unsigned int *fres)
@@ -338,6 +341,7 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
 	u32 ctrl = MX51_ECSPI_CTRL_ENABLE, dma = 0;
 	u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
 	u32 delay;
+	u32 lpb = 0;
 
 	/*
 	 * The hardware seems to have a race condition when changing modes. The
@@ -370,8 +374,12 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
 	if (config->mode & SPI_CS_HIGH)
 		cfg |= MX51_ECSPI_CONFIG_SSBPOL(config->cs);
 
+	if (config->mode & SPI_LOOP)
+		lpb |= MX51_ECSPI_LOOP;
+
 	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
 	writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);
+	writel(lpb, spi_imx->base + MX51_ECSPI_TEST);
 
 	/*
 	 * Wait until the changes in the configuration register CONFIGREG
@@ -1252,6 +1260,9 @@ static int spi_imx_probe(struct platform_device *pdev)
 	spi_imx = spi_master_get_devdata(master);
 	spi_imx->bitbang.master = master;
 
+	spi_imx->devtype_data = of_id ? of_id->data :
+		(struct spi_imx_devtype_data *)pdev->id_entry->driver_data;
+
 	for (i = 0; i < master->num_chipselect; i++) {
 		int cs_gpio = of_get_named_gpio(np, "cs-gpios", i);
 		if (!gpio_is_valid(cs_gpio) && mxc_platform_info)
@@ -1276,12 +1287,13 @@ static int spi_imx_probe(struct platform_device *pdev)
 	spi_imx->bitbang.master->cleanup = spi_imx_cleanup;
 	spi_imx->bitbang.master->prepare_message = spi_imx_prepare_message;
 	spi_imx->bitbang.master->unprepare_message = spi_imx_unprepare_message;
-	spi_imx->bitbang.master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
+	spi_imx->bitbang.master->mode_bits = SPI_CPOL | SPI_CPHA |
+				SPI_CS_HIGH;
 
-	init_completion(&spi_imx->xfer_done);
+	if (is_imx5x_ecspi(spi_imx))
+		spi_imx->bitbang.master->mode_bits |= SPI_LOOP;
 
-	spi_imx->devtype_data = of_id ? of_id->data :
-		(struct spi_imx_devtype_data *) pdev->id_entry->driver_data;
+	init_completion(&spi_imx->xfer_done);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	spi_imx->base = devm_ioremap_resource(&pdev->dev, res);
-- 
2.5.2

Re: [PATCH v2 5/8] spi: imx: Add support for loopback for ECSPI controllers

[Robin Gong]

On Fri, Sep 25, 2015 at 07:57:12PM +0200, Anton Bondarenko wrote:
> Support for ECSPI loopback for IMX51,IMX53 and IMX6Q using TEST register.
> 
> Signed-off-by: Mohsin Kazmi <mohsin_kazmi@mentor.com>
> Signed-off-by: Anton Bondarenko <anton_bondarenko@mentor.com>
> ---
>  drivers/spi/spi-imx.c | 20 ++++++++++++++++----
>  1 file changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 41c9cef..4b3c16e 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -265,6 +265,9 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
>  #define MX51_ECSPI_STAT		0x18
>  #define MX51_ECSPI_STAT_RR		(1 <<  3)
>  
> +#define MX51_ECSPI_TEST			0x20
> +#define MX51_ECSPI_LOOP			BIT(31)
> +
>  /* MX51 eCSPI */
>  static unsigned int mx51_ecspi_clkdiv(unsigned int fin, unsigned int fspi,
>  				      unsigned int *fres)
> @@ -338,6 +341,7 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
>  	u32 ctrl = MX51_ECSPI_CTRL_ENABLE, dma = 0;
>  	u32 cfg = readl(spi_imx->base + MX51_ECSPI_CONFIG);
>  	u32 delay;
> +	u32 lpb = 0;
>  
>  	/*
>  	 * The hardware seems to have a race condition when changing modes. The
> @@ -370,8 +374,12 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
>  	if (config->mode & SPI_CS_HIGH)
>  		cfg |= MX51_ECSPI_CONFIG_SSBPOL(config->cs);
>  
> +	if (config->mode & SPI_LOOP)
> +		lpb |= MX51_ECSPI_LOOP;
> +
>  	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
>  	writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);
> +	writel(lpb, spi_imx->base + MX51_ECSPI_TEST);
It's better write this MX51_ECSPI_TEST only in LOOP mode.
>  
>  	/*
>  	 * Wait until the changes in the configuration register CONFIGREG
> @@ -1252,6 +1260,9 @@ static int spi_imx_probe(struct platform_device *pdev)
>  	spi_imx = spi_master_get_devdata(master);
>  	spi_imx->bitbang.master = master;
>  
> +	spi_imx->devtype_data = of_id ? of_id->data :
> +		(struct spi_imx_devtype_data *)pdev->id_entry->driver_data;
> +
>  	for (i = 0; i < master->num_chipselect; i++) {
>  		int cs_gpio = of_get_named_gpio(np, "cs-gpios", i);
>  		if (!gpio_is_valid(cs_gpio) && mxc_platform_info)
> @@ -1276,12 +1287,13 @@ static int spi_imx_probe(struct platform_device *pdev)
>  	spi_imx->bitbang.master->cleanup = spi_imx_cleanup;
>  	spi_imx->bitbang.master->prepare_message = spi_imx_prepare_message;
>  	spi_imx->bitbang.master->unprepare_message = spi_imx_unprepare_message;
> -	spi_imx->bitbang.master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
> +	spi_imx->bitbang.master->mode_bits = SPI_CPOL | SPI_CPHA |
> +				SPI_CS_HIGH;
Any change?
>  
> -	init_completion(&spi_imx->xfer_done);
> +	if (is_imx5x_ecspi(spi_imx))
> +		spi_imx->bitbang.master->mode_bits |= SPI_LOOP;
>  
> -	spi_imx->devtype_data = of_id ? of_id->data :
> -		(struct spi_imx_devtype_data *) pdev->id_entry->driver_data;
> +	init_completion(&spi_imx->xfer_done);
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	spi_imx->base = devm_ioremap_resource(&pdev->dev, res);
> -- 
> 2.5.2
> 

Re: [PATCH v2 5/8] spi: imx: Add support for loopback for ECSPI controllers

[Bondarenko, Anton]

On 30.09.2015 10:42, Robin Gong wrote:
> On Fri, Sep 25, 2015 at 07:57:12PM +0200, Anton Bondarenko wrote:
>> @@ -370,8 +374,12 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
>>  	if (config->mode & SPI_CS_HIGH)
>>  		cfg |= MX51_ECSPI_CONFIG_SSBPOL(config->cs);
>>  
>> +	if (config->mode & SPI_LOOP)
>> +		lpb |= MX51_ECSPI_LOOP;
>> +
>>  	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
>>  	writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);
>> +	writel(lpb, spi_imx->base + MX51_ECSPI_TEST);
> It's better write this MX51_ECSPI_TEST only in LOOP mode.
LOOP mode can be set dynamically. This mean we should be able to set and clear the flag.
Currently only LOOP flag is used. I did not find any problems with writing 0 in the rest
fields of TEST register.
>>  
>>  	/*
>>  	 * Wait until the changes in the configuration register CONFIGREG
>> @@ -1276,12 +1287,13 @@ static int spi_imx_probe(struct platform_device *pdev)
>>  	spi_imx->bitbang.master->cleanup = spi_imx_cleanup;
>>  	spi_imx->bitbang.master->prepare_message = spi_imx_prepare_message;
>>  	spi_imx->bitbang.master->unprepare_message = spi_imx_unprepare_message;
>> -	spi_imx->bitbang.master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
>> +	spi_imx->bitbang.master->mode_bits = SPI_CPOL | SPI_CPHA |
>> +				SPI_CS_HIGH;
> Any change?
Will be fixed in V3
>>  
>> -	init_completion(&spi_imx->xfer_done);
>> +	if (is_imx5x_ecspi(spi_imx))
>> +		spi_imx->bitbang.master->mode_bits |= SPI_LOOP;
>>  
>> -	spi_imx->devtype_data = of_id ? of_id->data :
>> -		(struct spi_imx_devtype_data *) pdev->id_entry->driver_data;
>> +	init_completion(&spi_imx->xfer_done);
>>  
>>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>  	spi_imx->base = devm_ioremap_resource(&pdev->dev, res);
>> -- 
>> 2.5.2
>>

Re: [PATCH v2 5/8] spi: imx: Add support for loopback for ECSPI controllers

[Robin Gong]

On Thu, Oct 01, 2015 at 12:16:49AM +0000, Bondarenko, Anton wrote:
> On 30.09.2015 10:42, Robin Gong wrote:
> > On Fri, Sep 25, 2015 at 07:57:12PM +0200, Anton Bondarenko wrote:
> >> @@ -370,8 +374,12 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
> >>  	if (config->mode & SPI_CS_HIGH)
> >>  		cfg |= MX51_ECSPI_CONFIG_SSBPOL(config->cs);
> >>  
> >> +	if (config->mode & SPI_LOOP)
> >> +		lpb |= MX51_ECSPI_LOOP;
> >> +
> >>  	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
> >>  	writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);
> >> +	writel(lpb, spi_imx->base + MX51_ECSPI_TEST);
> > It's better write this MX51_ECSPI_TEST only in LOOP mode.
> LOOP mode can be set dynamically. This mean we should be able to set and clear the flag.
> Currently only LOOP flag is used. I did not find any problems with writing 0 in the rest
> fields of TEST register.
Yes, I know writing 0 is harmless, but we never need to touch this register if
SPI_LOOP flags not set, I'd like below:
	if (config->mode & SPI_LOOP) {
		lpb |= MX51_ECSPI_LOOP;
		writel(lgb, spi_imx->base + MX51_ECSPI_TEST);
	}

> >>  
> >>  	/*
> >>  	 * Wait until the changes in the configuration register CONFIGREG
> >> @@ -1276,12 +1287,13 @@ static int spi_imx_probe(struct platform_device *pdev)
> >>  	spi_imx->bitbang.master->cleanup = spi_imx_cleanup;
> >>  	spi_imx->bitbang.master->prepare_message = spi_imx_prepare_message;
> >>  	spi_imx->bitbang.master->unprepare_message = spi_imx_unprepare_message;
> >> -	spi_imx->bitbang.master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
> >> +	spi_imx->bitbang.master->mode_bits = SPI_CPOL | SPI_CPHA |
> >> +				SPI_CS_HIGH;
> > Any change?
> Will be fixed in V3
> >>  
> >> -	init_completion(&spi_imx->xfer_done);
> >> +	if (is_imx5x_ecspi(spi_imx))
> >> +		spi_imx->bitbang.master->mode_bits |= SPI_LOOP;
> >>  
> >> -	spi_imx->devtype_data = of_id ? of_id->data :
> >> -		(struct spi_imx_devtype_data *) pdev->id_entry->driver_data;
> >> +	init_completion(&spi_imx->xfer_done);
> >>  
> >>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >>  	spi_imx->base = devm_ioremap_resource(&pdev->dev, res);
> >> -- 
> >> 2.5.2
> >>
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 5/8] spi: imx: Add support for loopback for ECSPI controllers

[Anton Bondarenko]

On 08.10.2015 11:28, Robin Gong wrote:
> On Thu, Oct 01, 2015 at 12:16:49AM +0000, Bondarenko, Anton wrote:
>> On 30.09.2015 10:42, Robin Gong wrote:
>>> On Fri, Sep 25, 2015 at 07:57:12PM +0200, Anton Bondarenko wrote:
>>>> @@ -370,8 +374,12 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
>>>>   	if (config->mode & SPI_CS_HIGH)
>>>>   		cfg |= MX51_ECSPI_CONFIG_SSBPOL(config->cs);
>>>>
>>>> +	if (config->mode & SPI_LOOP)
>>>> +		lpb |= MX51_ECSPI_LOOP;
>>>> +
>>>>   	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
>>>>   	writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);
>>>> +	writel(lpb, spi_imx->base + MX51_ECSPI_TEST);
>>> It's better write this MX51_ECSPI_TEST only in LOOP mode.
>> LOOP mode can be set dynamically. This mean we should be able to set and clear the flag.
>> Currently only LOOP flag is used. I did not find any problems with writing 0 in the rest
>> fields of TEST register.
> Yes, I know writing 0 is harmless, but we never need to touch this register if
> SPI_LOOP flags not set, I'd like below:
> 	if (config->mode & SPI_LOOP) {
> 		lpb |= MX51_ECSPI_LOOP;
> 		writel(lgb, spi_imx->base + MX51_ECSPI_TEST);
> 	}
>
Proposed code not able to clear LOOP mode in TEST register. What about 
something like this:
if (config->mode & SPI_LOOP)
	lpb |= MX51_ECSPI_LOOP;

if ((readl(spi_imx->base + MX51_ECSPI_TEST) & MX51_ECSPI_LOOP) != lpb)
	writel(lpb, spi_imx->base + MX51_ECSPI_TEST);

In this case driver will write TEST register only if needed.
>>>>
>>>>   	/*
>>>>   	 * Wait until the changes in the configuration register CONFIGREG

Regards,
Anton
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 6/8] spi: imx: return error from dma channel request

[Anton Bondarenko]

On SDMA initialization return exactly the same error, which is
reported by dma_request_slave_channel_reason(), it is a preceding
change to defer SPI DMA initialization, if SDMA module is not yet
available.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
Signed-off-by: Anton Bondarenko <anton_bondarenko@mentor.com>
---
 drivers/spi/spi-imx.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 4b3c16e..44d3cf0 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -929,18 +929,20 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
 	spi_imx->wml = spi_imx_get_fifosize(spi_imx) / 2;
 
 	/* Prepare for TX DMA: */
-	master->dma_tx = dma_request_slave_channel(dev, "tx");
-	if (!master->dma_tx) {
-		dev_err(dev, "cannot get the TX DMA channel!\n");
-		ret = -EINVAL;
+	master->dma_tx = dma_request_slave_channel_reason(dev, "tx");
+	if (IS_ERR(master->dma_tx)) {
+		dev_info(dev, "cannot get the TX DMA channel!\n");
+		ret = PTR_ERR(master->dma_tx);
+		master->dma_tx = NULL;
 		goto err;
 	}
 
 	/* Prepare for RX : */
-	master->dma_rx = dma_request_slave_channel(dev, "rx");
-	if (!master->dma_rx) {
-		dev_dbg(dev, "cannot get the DMA channel.\n");
-		ret = -EINVAL;
+	master->dma_rx = dma_request_slave_channel_reason(dev, "rx");
+	if (IS_ERR(master->dma_rx)) {
+		dev_info(dev, "cannot get the DMA channel.\n");
+		ret = PTR_ERR(master->dma_rx);
+		master->dma_rx = NULL;
 		goto err;
 	}
 
-- 
2.5.2

Re: [PATCH v2 6/8] spi: imx: return error from dma channel request

[Robin Gong]

On Fri, Sep 25, 2015 at 07:57:13PM +0200, Anton Bondarenko wrote:
> On SDMA initialization return exactly the same error, which is
> reported by dma_request_slave_channel_reason(), it is a preceding
> change to defer SPI DMA initialization, if SDMA module is not yet
> available.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> Signed-off-by: Anton Bondarenko <anton_bondarenko@mentor.com>
> ---
>  drivers/spi/spi-imx.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 4b3c16e..44d3cf0 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -929,18 +929,20 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
>  	spi_imx->wml = spi_imx_get_fifosize(spi_imx) / 2;
>  
>  	/* Prepare for TX DMA: */
> -	master->dma_tx = dma_request_slave_channel(dev, "tx");
> -	if (!master->dma_tx) {
> -		dev_err(dev, "cannot get the TX DMA channel!\n");
> -		ret = -EINVAL;
> +	master->dma_tx = dma_request_slave_channel_reason(dev, "tx");
> +	if (IS_ERR(master->dma_tx)) {
> +		dev_info(dev, "cannot get the TX DMA channel!\n");
> +		ret = PTR_ERR(master->dma_tx);
> +		master->dma_tx = NULL;
>  		goto err;
>  	}
>  
>  	/* Prepare for RX : */
> -	master->dma_rx = dma_request_slave_channel(dev, "rx");
> -	if (!master->dma_rx) {
> -		dev_dbg(dev, "cannot get the DMA channel.\n");
> -		ret = -EINVAL;
> +	master->dma_rx = dma_request_slave_channel_reason(dev, "rx");
> +	if (IS_ERR(master->dma_rx)) {
> +		dev_info(dev, "cannot get the DMA channel.\n");
> +		ret = PTR_ERR(master->dma_rx);
> +		master->dma_rx = NULL;
>  		goto err;
>  	}
>  
> -- 
> 2.5.2
> 

[PATCH v2 7/8] spi: imx: defer spi initialization, if DMA engine is pending

[Anton Bondarenko]

If SPI device supports DMA mode, but DMA controller is not yet
available due to e.g. a delay in the corresponding kernel module
initialization, retry to initialize SPI driver later on instead of
falling back into PIO only mode.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
Signed-off-by: Anton Bondarenko <anton_bondarenko@mentor.com>
---
 drivers/spi/spi-imx.c | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 44d3cf0..1bf0739a 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -1343,9 +1343,15 @@ static int spi_imx_probe(struct platform_device *pdev)
 	 * Only validated on i.mx6 now, can remove the constrain if validated on
 	 * other chips.
 	 */
-	if (is_imx5x_ecspi(spi_imx) &&
-	    spi_imx_sdma_init(&pdev->dev, spi_imx, master))
-		dev_err(&pdev->dev, "dma setup error,use pio instead\n");
+	if (is_imx5x_ecspi(spi_imx)) {
+		ret = spi_imx_sdma_init(&pdev->dev, spi_imx, master);
+		if (ret == -EPROBE_DEFER)
+			goto out_clk_put;
+
+		if (ret < 0)
+			dev_err(&pdev->dev, "dma setup error %d, use pio\n",
+				ret);
+	}
 
 	spi_imx->devtype_data->reset(spi_imx);
 
-- 
2.5.2

Re: [PATCH v2 7/8] spi: imx: defer spi initialization, if DMA engine is pending

[Robin Gong]

On Fri, Sep 25, 2015 at 07:57:14PM +0200, Anton Bondarenko wrote:
> If SPI device supports DMA mode, but DMA controller is not yet
> available due to e.g. a delay in the corresponding kernel module
> initialization, retry to initialize SPI driver later on instead of
> falling back into PIO only mode.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> Signed-off-by: Anton Bondarenko <anton_bondarenko@mentor.com>
> ---
>  drivers/spi/spi-imx.c | 12 +++++++++---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 44d3cf0..1bf0739a 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -1343,9 +1343,15 @@ static int spi_imx_probe(struct platform_device *pdev)
>  	 * Only validated on i.mx6 now, can remove the constrain if validated on
>  	 * other chips.
>  	 */
> -	if (is_imx5x_ecspi(spi_imx) &&
> -	    spi_imx_sdma_init(&pdev->dev, spi_imx, master))
> -		dev_err(&pdev->dev, "dma setup error,use pio instead\n");
> +	if (is_imx5x_ecspi(spi_imx)) {
> +		ret = spi_imx_sdma_init(&pdev->dev, spi_imx, master);
> +		if (ret == -EPROBE_DEFER)
> +			goto out_clk_put;
> +
> +		if (ret < 0)
> +			dev_err(&pdev->dev, "dma setup error %d, use pio\n",
> +				ret);
> +	}
>  
>  	spi_imx->devtype_data->reset(spi_imx);
>  
> -- 
> 2.5.2
> 

[PATCH v2 8/8] spi: imx: Add support for SPI Slave mode for imx53 and imx6 chips

[Anton Bondarenko]

Currently i.MX SPI controller only works in Master mode.
This patch adds support to work also in Slave mode by adding
"fsl,spi-slave-mode" in corresponding ecspi node in devicetree.

Currently SPI Slave mode support patch has the following limitations:
1. The stale data in RXFIFO will be dropped when the Slave does any new
   transfer.
2. One transfer can be finished only after all transfer->len data been
   transferred to master device
3. Slave device only accepts transfer->len data. Any data longer than this
   from master device will be dropped. Any data shorter than this from
   master will cause SPI to stuck du to mentioned HW limitation 2.

Following HW limitation applies:
1.  ECSPI has a HW issue when works in Slave mode, after 64
    words written to TXFIFO, even TXFIFO becomes empty,
    ECSPI_TXDATA keeps shift out the last word data,
    so we have to disable ECSPI when in slave mode after the
    transfer completes
2.  Due to Freescale errata ERR003775 "eCSPI: Burst completion by Chip
    Select (SS) signal in Slave mode is not functional" burst size must
    be set exactly to the size of the transfer. This limit SPI transaction
    with maximum 2^12 bits.

Signed-off-by: Jiada Wang <jiada_wang-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Muzaffar Mahmood <muzaffar_mahmood-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
Signed-off-by: Anton Bondarenko <anton_bondarenko-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
---
 .../devicetree/bindings/spi/fsl-imx-cspi.txt       |   1 +
 drivers/spi/spi-imx.c                              | 169 ++++++++++++++++++---
 2 files changed, 146 insertions(+), 24 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt b/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
index 425485f..ffe28f2 100644
--- a/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
+++ b/Documentation/devicetree/bindings/spi/fsl-imx-cspi.txt
@@ -15,6 +15,7 @@ Required properties:
 - interrupts : Should contain CSPI/eCSPI interrupt
 - fsl,spi-num-chipselects : Contains the number of the chipselect
 - cs-gpios : Specifies the gpio pins to be used for chipselects.
+- fsl,spi-slave-mode : Indicate the spi controller works in Slave mode.
 - clocks : Clock specifiers for both ipg and per clocks.
 - clock-names : Clock names should include both "ipg" and "per"
 See the clock consumer binding,
diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 1bf0739a..f271d1e 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -55,6 +55,7 @@
 /* generic defines to abstract from the different register layouts */
 #define MXC_INT_RR	(1 << 0) /* Receive data ready interrupt */
 #define MXC_INT_TE	(1 << 1) /* Transmit FIFO empty interrupt */
+#define MXC_INT_RDR	BIT(4) /* Receive date threshold interrupt */
 #define MXC_INT_TCEN	BIT(7)   /* Transfer complete */
 
 /* The maximum  bytes that a sdma BD can transfer.*/
@@ -84,6 +85,7 @@ struct spi_imx_devtype_data {
 	void (*trigger)(struct spi_imx_data *);
 	int (*rx_available)(struct spi_imx_data *);
 	void (*reset)(struct spi_imx_data *);
+	void (*disable)(struct spi_imx_data *);
 	enum spi_imx_devtype devtype;
 };
 
@@ -108,6 +110,10 @@ struct spi_imx_data {
 	const void *tx_buf;
 	unsigned int txfifo; /* number of words pushed in tx FIFO */
 
+	unsigned int slave_mode;
+	unsigned int slave_burst;
+	unsigned int total_count;
+
 	/* DMA */
 	unsigned int dma_is_inited;
 	unsigned int dma_finished;
@@ -248,6 +254,7 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
 #define MX51_ECSPI_INT		0x10
 #define MX51_ECSPI_INT_TEEN		(1 <<  0)
 #define MX51_ECSPI_INT_RREN		(1 <<  3)
+#define MX51_ECSPI_INT_RDREN	BIT(4)
 #define MX51_ECSPI_INT_TCEN		BIT(7)
 
 #define MX51_ECSPI_DMA      0x14
@@ -268,6 +275,52 @@ static bool spi_imx_can_dma(struct spi_master *master, struct spi_device *spi,
 #define MX51_ECSPI_TEST			0x20
 #define MX51_ECSPI_LOOP			BIT(31)
 
+static void mx53_ecspi_rx_slave(struct spi_imx_data *spi_imx)
+{
+	u32 val = be32_to_cpu(readl(spi_imx->base + MXC_CSPIRXDATA));
+
+	if (spi_imx->rx_buf) {
+		int shift = spi_imx->slave_burst % sizeof(val);
+
+		if (shift) {
+			memcpy(spi_imx->rx_buf,
+			       ((u8 *)&val) + sizeof(val) - shift, shift);
+		} else {
+			*((u32 *)spi_imx->rx_buf) = val;
+			shift = sizeof(val);
+		}
+
+		spi_imx->rx_buf += shift;
+		spi_imx->slave_burst -= shift;
+	}
+}
+
+static void mx53_ecspi_tx_slave(struct spi_imx_data *spi_imx)
+{
+	u32 val = 0;
+	int shift = spi_imx->count % sizeof(val);
+
+	if (spi_imx->tx_buf) {
+		if (shift) {
+			memcpy(((u8 *)&val) + sizeof(val) - shift,
+			       spi_imx->tx_buf, shift);
+		} else {
+			val = *((u32 *)spi_imx->tx_buf);
+			shift = sizeof(val);
+		}
+
+		val = cpu_to_be32(val);
+		spi_imx->tx_buf += shift;
+	}
+
+	if (!shift)
+		shift = sizeof(val);
+
+	spi_imx->count -= shift;
+
+	writel(val, spi_imx->base + MXC_CSPITXDATA);
+}
+
 /* MX51 eCSPI */
 static unsigned int mx51_ecspi_clkdiv(unsigned int fin, unsigned int fspi,
 				      unsigned int *fres)
@@ -316,6 +369,9 @@ static void __maybe_unused mx51_ecspi_intctrl(struct spi_imx_data *spi_imx, int
 	if (enable & MXC_INT_RR)
 		val |= MX51_ECSPI_INT_RREN;
 
+	if (enable & MXC_INT_RDR)
+		val |= MX51_ECSPI_INT_RDREN;
+
 	if (enable & MXC_INT_TCEN)
 		val |= MX51_ECSPI_INT_TCEN;
 
@@ -335,6 +391,15 @@ static void __maybe_unused mx51_ecspi_trigger(struct spi_imx_data *spi_imx)
 	writel(reg, spi_imx->base + MX51_ECSPI_CTRL);
 }
 
+static void __maybe_unused mx51_ecspi_disable(struct spi_imx_data *spi_imx)
+{
+	u32 ctrl;
+
+	ctrl = readl(spi_imx->base + MX51_ECSPI_CTRL);
+	ctrl &= ~MX51_ECSPI_CTRL_ENABLE;
+	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
+}
+
 static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
 		struct spi_imx_config *config)
 {
@@ -343,14 +408,11 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
 	u32 delay;
 	u32 lpb = 0;
 
-	/*
-	 * The hardware seems to have a race condition when changing modes. The
-	 * current assumption is that the selection of the channel arrives
-	 * earlier in the hardware than the mode bits when they are written at
-	 * the same time.
-	 * So set master mode for all channels as we do not support slave mode.
-	 */
-	ctrl |= MX51_ECSPI_CTRL_MODE_MASK;
+	/* set Master or Slave mode */
+	if (spi_imx->slave_mode)
+		ctrl &= ~MX51_ECSPI_CTRL_MODE_MASK;
+	else
+		ctrl |= MX51_ECSPI_CTRL_MODE_MASK;
 
 	/* set clock speed */
 	spi_imx->spi_bus_clk = config->speed_hz;
@@ -360,9 +422,22 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
 	/* set chip select to use */
 	ctrl |= MX51_ECSPI_CTRL_CS(config->cs);
 
-	ctrl |= (config->bpw - 1) << MX51_ECSPI_CTRL_BL_OFFSET;
+	if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
+		ctrl |= (spi_imx->slave_burst * 8 - 1)
+			<< MX51_ECSPI_CTRL_BL_OFFSET;
+	else
+		ctrl |= (config->bpw - 1) << MX51_ECSPI_CTRL_BL_OFFSET;
+
 
-	cfg |= MX51_ECSPI_CONFIG_SBBCTRL(config->cs);
+	/*
+	 * eCSPI burst completion by Chip Select signal in Slave mode
+	 * is not functional, config SPI burst completed when
+	 * BURST_LENGTH + 1 bits are received
+	 */
+	if (spi_imx->slave_mode && is_imx53_ecspi(spi_imx))
+		cfg &= ~MX51_ECSPI_CONFIG_SBBCTRL(config->cs);
+	else
+		cfg |= MX51_ECSPI_CONFIG_SBBCTRL(config->cs);
 
 	if (config->mode & SPI_CPHA)
 		cfg |= MX51_ECSPI_CONFIG_SCLKPHA(config->cs);
@@ -402,11 +477,11 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
 	 * Configure the DMA register: setup the watermark
 	 * and enable DMA request.
 	 */
-	if (spi_imx->dma_is_inited) {
-		dma = (spi_imx->wml - 1) << MX51_ECSPI_DMA_RX_WML_OFFSET
-		      | (spi_imx->wml - 1) << MX51_ECSPI_DMA_TX_WML_OFFSET
-		      | (1 << MX51_ECSPI_DMA_TEDEN_OFFSET)
-		      | (1 << MX51_ECSPI_DMA_RXDEN_OFFSET);
+	if (spi_imx->dma_is_inited || spi_imx->slave_mode) {
+		dma = (spi_imx->wml - 1) << MX51_ECSPI_DMA_RX_WML_OFFSET;
+		if (spi_imx->dma_is_inited)
+			dma |=  (1 << MX51_ECSPI_DMA_TEDEN_OFFSET)
+			      | (1 << MX51_ECSPI_DMA_RXDEN_OFFSET);
 
 		writel(dma, spi_imx->base + MX51_ECSPI_DMA);
 	}
@@ -697,6 +772,7 @@ static struct spi_imx_devtype_data imx51_ecspi_devtype_data = {
 	.trigger = mx51_ecspi_trigger,
 	.rx_available = mx51_ecspi_rx_available,
 	.reset = mx51_ecspi_reset,
+	.disable = mx51_ecspi_disable,
 	.devtype = IMX51_ECSPI,
 };
 
@@ -707,6 +783,7 @@ static struct spi_imx_devtype_data imx53_ecspi_devtype_data = {
 	.trigger = mx51_ecspi_trigger,
 	.rx_available = mx51_ecspi_rx_available,
 	.reset = mx51_ecspi_reset,
+	.disable = mx51_ecspi_disable,
 	.devtype = IMX53_ECSPI,
 };
 
@@ -775,14 +852,16 @@ static void spi_imx_push(struct spi_imx_data *spi_imx)
 		spi_imx->txfifo++;
 	}
 
-	spi_imx->devtype_data->trigger(spi_imx);
+	if (!spi_imx->slave_mode)
+		spi_imx->devtype_data->trigger(spi_imx);
 }
 
 static irqreturn_t spi_imx_isr(int irq, void *dev_id)
 {
 	struct spi_imx_data *spi_imx = dev_id;
 
-	while (spi_imx->devtype_data->rx_available(spi_imx)) {
+	while (spi_imx->txfifo &&
+	       spi_imx->devtype_data->rx_available(spi_imx)) {
 		spi_imx->rx(spi_imx);
 		spi_imx->txfifo--;
 	}
@@ -894,6 +973,12 @@ static int spi_imx_setupxfer(struct spi_device *spi,
 			pr_err("Can't configure SDMA, error %d\n", ret);
 	}
 
+	if (is_imx53_ecspi(spi_imx) && spi_imx->slave_mode) {
+		spi_imx->rx = mx53_ecspi_rx_slave;
+		spi_imx->tx = mx53_ecspi_tx_slave;
+		spi_imx->slave_burst = t->len;
+	}
+
 	if (!ret)
 		ret = spi_imx->devtype_data->config(spi_imx, &config);
 
@@ -926,8 +1011,6 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
 	if (of_machine_is_compatible("fsl,imx6dl"))
 		return 0;
 
-	spi_imx->wml = spi_imx_get_fifosize(spi_imx) / 2;
-
 	/* Prepare for TX DMA: */
 	master->dma_tx = dma_request_slave_channel_reason(dev, "tx");
 	if (IS_ERR(master->dma_tx)) {
@@ -1139,21 +1222,45 @@ static int spi_imx_pio_transfer(struct spi_device *spi,
 				struct spi_transfer *transfer)
 {
 	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
+	int result = transfer->len;
+
+	if (is_imx53_ecspi(spi_imx) && spi_imx->slave_mode &&
+	    transfer->len > 512) {
+		pr_err("Transaction too big, max size is %d bytes\n", 512);
+		return -EMSGSIZE;
+	}
 
 	spi_imx->tx_buf = transfer->tx_buf;
 	spi_imx->rx_buf = transfer->rx_buf;
 	spi_imx->count = transfer->len;
 	spi_imx->txfifo = 0;
 
+	if (spi_imx->slave_mode)
+		spi_imx->slave_burst = spi_imx->count;
+
 	reinit_completion(&spi_imx->xfer_done);
 
 	spi_imx_push(spi_imx);
 
-	spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TE);
+	if (spi_imx->slave_mode)
+		spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TE |
+							MXC_INT_RDR);
+	else
+		spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TE);
+
+	if (wait_for_completion_interruptible(&spi_imx->xfer_done) < 0)
+		result = -EINTR;
 
-	wait_for_completion(&spi_imx->xfer_done);
+	/* ecspi has a HW issue when works in Slave mode,
+	 * after 64 words writtern to TXFIFO, even TXFIFO becomes empty,
+	 * ECSPI_TXDATA keeps shift out the last word data,
+	 * so we have to disable ECSPI when in slave mode after the
+	 * transfer completes
+	 */
+	if (spi_imx->slave_mode && spi_imx->devtype_data->disable)
+		spi_imx->devtype_data->disable(spi_imx);
 
-	return transfer->len;
+	return result;
 }
 
 static int spi_imx_transfer(struct spi_device *spi,
@@ -1162,6 +1269,10 @@ static int spi_imx_transfer(struct spi_device *spi,
 	int ret;
 	struct spi_imx_data *spi_imx = spi_master_get_devdata(spi->master);
 
+	/* flush rxfifo before transfer */
+	while (spi_imx->devtype_data->rx_available(spi_imx))
+		spi_imx->rx(spi_imx);
+
 	if (spi_imx->bitbang.master->can_dma &&
 	    spi_imx_can_dma(spi_imx->bitbang.master, spi, transfer)) {
 		spi_imx->usedma = true;
@@ -1265,11 +1376,19 @@ static int spi_imx_probe(struct platform_device *pdev)
 	spi_imx->devtype_data = of_id ? of_id->data :
 		(struct spi_imx_devtype_data *)pdev->id_entry->driver_data;
 
+	if (of_get_property(np, "fsl,spi-slave-mode", NULL) &&
+	    is_imx5x_ecspi(spi_imx))
+		spi_imx->slave_mode = 1;
+
 	for (i = 0; i < master->num_chipselect; i++) {
 		int cs_gpio = of_get_named_gpio(np, "cs-gpios", i);
 		if (!gpio_is_valid(cs_gpio) && mxc_platform_info)
 			cs_gpio = mxc_platform_info->chipselect[i];
 
+		/* slave mode doesn't support cs gpio */
+		if (spi_imx->slave_mode)
+			cs_gpio = -ENODEV;
+
 		spi_imx->chipselect[i] = cs_gpio;
 		if (!gpio_is_valid(cs_gpio))
 			continue;
@@ -1292,11 +1411,13 @@ static int spi_imx_probe(struct platform_device *pdev)
 	spi_imx->bitbang.master->mode_bits = SPI_CPOL | SPI_CPHA |
 				SPI_CS_HIGH;
 
-	if (is_imx5x_ecspi(spi_imx))
+	if (is_imx5x_ecspi(spi_imx) && !spi_imx->slave_mode)
 		spi_imx->bitbang.master->mode_bits |= SPI_LOOP;
 
 	init_completion(&spi_imx->xfer_done);
 
+	spi_imx->wml = spi_imx_get_fifosize(spi_imx) / 2;
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	spi_imx->base = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(spi_imx->base)) {
@@ -1343,7 +1464,7 @@ static int spi_imx_probe(struct platform_device *pdev)
 	 * Only validated on i.mx6 now, can remove the constrain if validated on
 	 * other chips.
 	 */
-	if (is_imx5x_ecspi(spi_imx)) {
+	if (!spi_imx->slave_mode && is_imx5x_ecspi(spi_imx)) {
 		ret = spi_imx_sdma_init(&pdev->dev, spi_imx, master);
 		if (ret == -EPROBE_DEFER)
 			goto out_clk_put;
-- 
2.5.2

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html