[PATCH v3 0/7] Improvements for SPI IMX driver for Freescale IMX51, IMX53 and IMX6

[PATCH v3 0/7] Improvements for SPI IMX driver for Freescale IMX51, IMX53 and IMX6

[Anton Bondarenko]

A number of patches to impove or add the implementation
for the spi-imx driver related to Freescale IMX51, 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.

Changes since V2:
Removed patch V2 8/8 since it's not ready due to quality problems
Simlified patch 4/7 since V2 8/8 removed
Addressed all comments from reviewers and build bot failure

Anton Bondarenko (7):
  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 function to check for IMX51 family controller
  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

 drivers/spi/spi-imx.c | 333 +++++++++++++++++++++++++++++++++++++-------------
 1 file changed, 250 insertions(+), 83 deletions(-)

-- 
2.6.2

[PATCH v3 1/7] spi: imx: Fix DMA transfer

[Anton Bondarenko]

From: Anton Bondarenko <anton_bondarenko@mentor.com>

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 | 115 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 76 insertions(+), 39 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 0e5723a..bd7b721 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -53,6 +53,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 +105,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 +200,7 @@ 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)
 		return true;
 	return false;
 }
@@ -228,6 +225,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 +290,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 +312,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;
 
 	/*
@@ -376,19 +378,9 @@ 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
+		      | (1 << MX51_ECSPI_DMA_TEDEN_OFFSET)
+		      | (1 << MX51_ECSPI_DMA_RXDEN_OFFSET);
 
 		writel(dma, spi_imx->base + MX51_ECSPI_DMA);
 	}
@@ -832,6 +824,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) {
@@ -843,7 +837,7 @@ 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->wml;
 	ret = dmaengine_slave_config(master->dma_tx, &slave_config);
 	if (ret) {
 		dev_err(dev, "error in TX dma configuration.\n");
@@ -861,7 +855,7 @@ 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->wml;
 	ret = dmaengine_slave_config(master->dma_rx, &slave_config);
 	if (ret) {
 		dev_err(dev, "error in RX dma configuration.\n");
@@ -874,8 +868,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;
@@ -904,8 +896,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;
 
@@ -922,9 +913,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;
 
@@ -939,17 +944,18 @@ 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);
+	/*
+	 * Set these order to avoid potential RX overflow. The overflow may
+	 * happen if we enable SPI HW before starting RX DMA due to rescheduling
+	 * for another task and/or interrupt.
+	 * So RX DMA enabled first to make sure data would be read out from FIFO
+	 * ASAP. TX DMA enabled next to start filling TX FIFO with new data.
+	 * And finaly SPI HW enabled to start actual data transfer.
+	 */
+	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);
@@ -958,6 +964,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);
@@ -967,10 +974,40 @@ 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 - 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));
+			}
+
+			/*
+			 * WARNING: this call will cause DMA debug complains
+			 * about wrong combination of DMA direction and sync
+			 * function. But we must use it to make sure the data
+			 * read by PIO mode will be cleared from CPU cache.
+			 * Otherwise SPI core will invalidate it during unmap of
+			 * SG buffers.
+			 */
+			dma_sync_sg_for_device(master->dma_rx->device->dev,
+					       &rx->sgl[rx->nents - 1], 1,
+					       DMA_TO_DEVICE);
 		}
-		writel(dma |
-		       spi_imx->rxt_wml << MX51_ECSPI_DMA_RXT_WML_OFFSET,
-		       spi_imx->base + MX51_ECSPI_DMA);
 	}
 
 	spi_imx->dma_finished = 1;
-- 
2.6.2

Re: [PATCH v3 1/7] spi: imx: Fix DMA transfer

[Robin Gong]

On Sun, Nov 01, 2015 at 03:41:35PM +0100, Anton Bondarenko wrote:
> From: Anton Bondarenko <anton_bondarenko@mentor.com>
> 
> 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 | 115 +++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 76 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 0e5723a..bd7b721 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -53,6 +53,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 +105,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 +200,7 @@ 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)
>  		return true;
>  	return false;
>  }
> @@ -228,6 +225,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 +290,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 +312,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;
>  
>  	/*
> @@ -376,19 +378,9 @@ 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
> +		      | (1 << MX51_ECSPI_DMA_TEDEN_OFFSET)
> +		      | (1 << MX51_ECSPI_DMA_RXDEN_OFFSET);
>  
>  		writel(dma, spi_imx->base + MX51_ECSPI_DMA);
>  	}
> @@ -832,6 +824,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) {
> @@ -843,7 +837,7 @@ 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->wml;
>  	ret = dmaengine_slave_config(master->dma_tx, &slave_config);
>  	if (ret) {
>  		dev_err(dev, "error in TX dma configuration.\n");
> @@ -861,7 +855,7 @@ 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->wml;
>  	ret = dmaengine_slave_config(master->dma_rx, &slave_config);
>  	if (ret) {
>  		dev_err(dev, "error in RX dma configuration.\n");
> @@ -874,8 +868,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;
> @@ -904,8 +896,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;
>  
> @@ -922,9 +913,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;
>  
> @@ -939,17 +944,18 @@ 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);
> +	/*
> +	 * Set these order to avoid potential RX overflow. The overflow may
> +	 * happen if we enable SPI HW before starting RX DMA due to rescheduling
> +	 * for another task and/or interrupt.
> +	 * So RX DMA enabled first to make sure data would be read out from FIFO
> +	 * ASAP. TX DMA enabled next to start filling TX FIFO with new data.
> +	 * And finaly SPI HW enabled to start actual data transfer.
> +	 */
> +	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);
> @@ -958,6 +964,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);
> @@ -967,10 +974,40 @@ 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 - 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));
> +			}
> +
> +			/*
> +			 * WARNING: this call will cause DMA debug complains
> +			 * about wrong combination of DMA direction and sync
> +			 * function. But we must use it to make sure the data
> +			 * read by PIO mode will be cleared from CPU cache.
> +			 * Otherwise SPI core will invalidate it during unmap of
> +			 * SG buffers.
> +			 */
> +			dma_sync_sg_for_device(master->dma_rx->device->dev,
> +					       &rx->sgl[rx->nents - 1], 1,
> +					       DMA_TO_DEVICE);
I think the above dma_sync_sg_for_cpu for reading the last sgl by PIO mode is
enough, that move the right data into rx_buf which map by SPI core. And why
'DMA_TO_DEVICE' for rx here?
>  		}
> -		writel(dma |
> -		       spi_imx->rxt_wml << MX51_ECSPI_DMA_RXT_WML_OFFSET,
> -		       spi_imx->base + MX51_ECSPI_DMA);
>  	}
>  
>  	spi_imx->dma_finished = 1;
> -- 
> 2.6.2
> 

Re: [PATCH v3 1/7] spi: imx: Fix DMA transfer

[Anton Bondarenko]

On 03.11.2015 08:08, Robin Gong wrote:
> On Sun, Nov 01, 2015 at 03:41:35PM +0100, Anton Bondarenko wrote:
>> From: Anton Bondarenko <anton_bondarenko@mentor.com>
>>
>> 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 | 115 +++++++++++++++++++++++++++++++++-----------------
>>   1 file changed, 76 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
>> @@ -967,10 +974,40 @@ 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 - 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));
>> +			}
>> +
>> +			/*
>> +			 * WARNING: this call will cause DMA debug complains
>> +			 * about wrong combination of DMA direction and sync
>> +			 * function. But we must use it to make sure the data
>> +			 * read by PIO mode will be cleared from CPU cache.
>> +			 * Otherwise SPI core will invalidate it during unmap of
>> +			 * SG buffers.
>> +			 */
>> +			dma_sync_sg_for_device(master->dma_rx->device->dev,
>> +					       &rx->sgl[rx->nents - 1], 1,
>> +					       DMA_TO_DEVICE);
> I think the above dma_sync_sg_for_cpu for reading the last sgl by PIO mode is
> enough, that move the right data into rx_buf which map by SPI core. And why
> 'DMA_TO_DEVICE' for rx here?
Actually this is described in comments above the call. So after writing 
data tail by CPU we must ensure cache content is flushed to RAM. 
Otherwise SPI core during dma_unmap will invalidate data in the CPU 
cache and we loose our data tail. I'm able to reproduce this very fast 
during 20-30 SPI transactions.
Hope this make things clearer.

>>   		}
>> -		writel(dma |
>> -		       spi_imx->rxt_wml << MX51_ECSPI_DMA_RXT_WML_OFFSET,
>> -		       spi_imx->base + MX51_ECSPI_DMA);
>>   	}
>>
>>   	spi_imx->dma_finished = 1;
>> --
>> 2.6.2
>>

Re: [PATCH v3 1/7] spi: imx: Fix DMA transfer

[Sascha Hauer]

On Sun, Nov 01, 2015 at 03:41:35PM +0100, Anton Bondarenko wrote:
> From: Anton Bondarenko <anton_bondarenko@mentor.com>
> 
> 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.

Have you looked at the RX_DMA_LENGTH and RXTDEN fields of the DMA
register? These seem to be for handling the remaining bytes of a DMA
transfer which do not reach the watermark level. From reading the
documentation I haven't really understood how it works though.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v3 1/7] spi: imx: Fix DMA transfer

[Anton Bondarenko]

On 05.11.2015 09:34, Sascha Hauer wrote:
> On Sun, Nov 01, 2015 at 03:41:35PM +0100, Anton Bondarenko wrote:
>> From: Anton Bondarenko <anton_bondarenko@mentor.com>
>>
>> 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.
>
> Have you looked at the RX_DMA_LENGTH and RXTDEN fields of the DMA
> register? These seem to be for handling the remaining bytes of a DMA
> transfer which do not reach the watermark level. From reading the
> documentation I haven't really understood how it works though.
>
> Sascha
>

A lot of times. Current implementation is trying to use it, but works 
incorrectly if length % WML != 0 (which means RX_DMA_LENGTH == 0).

Regards, Anton

Re: [PATCH v3 1/7] spi: imx: Fix DMA transfer

[Anton Bondarenko]

On 05.11.2015 17:51, Anton Bondarenko wrote:
> On 05.11.2015 09:34, Sascha Hauer wrote:
>> On Sun, Nov 01, 2015 at 03:41:35PM +0100, Anton Bondarenko wrote:
>>> From: Anton Bondarenko <anton_bondarenko@mentor.com>
>>>
>>> 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.
>>
>> Have you looked at the RX_DMA_LENGTH and RXTDEN fields of the DMA
>> register? These seem to be for handling the remaining bytes of a DMA
>> transfer which do not reach the watermark level. From reading the
>> documentation I haven't really understood how it works though.
>>
>> Sascha
>>
>
> A lot of times. Current implementation is trying to use it, but works
> incorrectly if length % WML != 0 (which means RX_DMA_LENGTH == 0).
>
> Regards, Anton

Does anyone has other comments regarding this commit?

Regards, Anton

[PATCH v3 2/7] spi: imx: replace fixed timeout with calculated one

[Anton Bondarenko]

From: Anton Bondarenko <anton_bondarenko@mentor.com>

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 bd7b721..9b80c7f 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -57,7 +57,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;
@@ -93,6 +92,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 *);
@@ -314,8 +314,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
@@ -327,7 +326,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);
@@ -367,7 +368,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 */
@@ -890,12 +891,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;
@@ -956,9 +985,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),
@@ -966,8 +997,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);
 		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),
@@ -989,7 +1022,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.6.2

Re: [PATCH v3 2/7] spi: imx: replace fixed timeout with calculated one

[Sascha Hauer]

On Sun, Nov 01, 2015 at 03:41:36PM +0100, Anton Bondarenko wrote:
> From: Anton Bondarenko <anton_bondarenko@mentor.com>
> 
> 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 bd7b721..9b80c7f 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -57,7 +57,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;
> @@ -93,6 +92,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 *);
> @@ -314,8 +314,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
> @@ -327,7 +326,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);
> @@ -367,7 +368,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 */
> @@ -890,12 +891,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);
> +}

I think you can simplify this function to:

	timeout = size * 8 / spi_imx->spi_bus_clk;
	timeout += 2;

	return msecs_to_jiffies(timeout * MSEC_PER_SEC);

The rationale is that when size * 8 / spi_imx->spi_bus_clk is 0 then we
can add another second and be fine. No need to calculate this more
accurate, in the end it's important to catch the timeout. If we do this
one or two seconds too late doesn't matter.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v3 2/7] spi: imx: replace fixed timeout with calculated one

[Anton Bondarenko]

On 05.11.2015 09:47, Sascha Hauer wrote:
>> @@ -890,12 +891,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);
>> +}
>
> I think you can simplify this function to:
>
> 	timeout = size * 8 / spi_imx->spi_bus_clk;
> 	timeout += 2;
>
> 	return msecs_to_jiffies(timeout * MSEC_PER_SEC);
>
> The rationale is that when size * 8 / spi_imx->spi_bus_clk is 0 then we
> can add another second and be fine. No need to calculate this more
> accurate, in the end it's important to catch the timeout. If we do this
> one or two seconds too late doesn't matter.
>
> Sascha
>

Sascha,

What about something like this:
	timeout = size * (8 + 4) / spi_imx->spi_bus_clk;
	timeout += 2;

	return msecs_to_jiffies(2 * timeout * MSEC_PER_SEC);

I think we still need to take into account 4 CLKs between each burst.

Regards, Anton

Re: [PATCH v3 2/7] spi: imx: replace fixed timeout with calculated one

[Sascha Hauer]

On Tue, Nov 10, 2015 at 09:20:06PM +0100, Anton Bondarenko wrote:
> 
> 
> On 05.11.2015 09:47, Sascha Hauer wrote:
> >>@@ -890,12 +891,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);
> >>+}
> >
> >I think you can simplify this function to:
> >
> >	timeout = size * 8 / spi_imx->spi_bus_clk;
> >	timeout += 2;
> >
> >	return msecs_to_jiffies(timeout * MSEC_PER_SEC);
> >
> >The rationale is that when size * 8 / spi_imx->spi_bus_clk is 0 then we
> >can add another second and be fine. No need to calculate this more
> >accurate, in the end it's important to catch the timeout. If we do this
> >one or two seconds too late doesn't matter.
> >
> >Sascha
> >
> 
> Sascha,
> 
> What about something like this:
> 	timeout = size * (8 + 4) / spi_imx->spi_bus_clk;
> 	timeout += 2;
> 
> 	return msecs_to_jiffies(2 * timeout * MSEC_PER_SEC);
> 
> I think we still need to take into account 4 CLKs between each burst.

Fine with me.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v3 2/7] spi: imx: replace fixed timeout with calculated one

[Anton Bondarenko]

On 11.11.2015 09:12, Sascha Hauer wrote:
> On Tue, Nov 10, 2015 at 09:20:06PM +0100, Anton Bondarenko wrote:
>>
>>
>> On 05.11.2015 09:47, Sascha Hauer wrote:
>>>> @@ -890,12 +891,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);
>>>> +}
>>>
>>> I think you can simplify this function to:
>>>
>>> 	timeout = size * 8 / spi_imx->spi_bus_clk;
>>> 	timeout += 2;
>>>
>>> 	return msecs_to_jiffies(timeout * MSEC_PER_SEC);
>>>
>>> The rationale is that when size * 8 / spi_imx->spi_bus_clk is 0 then we
>>> can add another second and be fine. No need to calculate this more
>>> accurate, in the end it's important to catch the timeout. If we do this
>>> one or two seconds too late doesn't matter.
>>>
>>> Sascha
>>>
>>
>> Sascha,
>>
>> What about something like this:
>> 	timeout = size * (8 + 4) / spi_imx->spi_bus_clk;
>> 	timeout += 2;
>>
>> 	return msecs_to_jiffies(2 * timeout * MSEC_PER_SEC);
>>
>> I think we still need to take into account 4 CLKs between each burst.
>
> Fine with me.
>
> Sascha
>
>

Tested new implementation successfully. Will be V4 patchset. Does anyone 
has other comments regarding this commit?

Regards, Anton

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

[Anton Bondarenko]

From: Anton Bondarenko <anton_bondarenko@mentor.com>

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 | 133 ++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 102 insertions(+), 31 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 9b80c7f..06f52c3 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -89,11 +89,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 bytes_per_word;
+
 	unsigned int count;
 	void (*tx)(struct spi_imx_data *);
 	void (*rx)(struct spi_imx_data *);
@@ -195,12 +199,31 @@ static unsigned int spi_imx_clkdiv_2(unsigned int fin,
 	return 7;
 }
 
+static int spi_imx_get_bytes_per_word(const int bpw)
+{
+	return DIV_ROUND_UP(bpw, BITS_PER_BYTE);
+}
+
 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 = transfer->bits_per_word;
+
+	if (!bpw)
+		bpw = spi->bits_per_word;
 
-	if (spi_imx->dma_is_inited && transfer->len > spi_imx->wml)
+	bpw = spi_imx_get_bytes_per_word(bpw);
+
+	/*
+	 * 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.
+	 */
+	if (spi_imx->dma_is_inited && (transfer->len > spi_imx->wml * bpw))
 		return true;
 	return false;
 }
@@ -764,11 +787,60 @@ 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->bytes_per_word) {
+	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->bytes_per_word);
+		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->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->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 new_bytes_per_word;
+	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;
@@ -792,9 +864,19 @@ 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);
+	new_bytes_per_word = spi_imx_get_bytes_per_word(config.bpw);
+	if (spi_imx->dma_is_inited &&
+	    spi_imx->bytes_per_word != new_bytes_per_word) {
+		spi_imx->bytes_per_word = new_bytes_per_word;
+		ret = spi_imx_sdma_configure(spi->master);
+		if (ret != 0)
+			pr_err("Can't configure SDMA, error %d\n", ret);
+	}
+
+	if (!ret)
+		ret = spi_imx->devtype_data->config(spi_imx, &config);
 
-	return 0;
+	return ret;
 }
 
 static void spi_imx_sdma_exit(struct spi_imx_data *spi_imx)
@@ -815,10 +897,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 */
@@ -835,16 +915,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->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) {
@@ -853,22 +923,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->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->bytes_per_word = 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;
@@ -925,7 +992,8 @@ 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->bytes_per_word);
 	struct spi_master *master = spi_imx->bitbang.master;
 	struct sg_table *tx = &transfer->tx_sg, *rx = &transfer->rx_sg;
 
@@ -998,7 +1066,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);
+					spi_imx->bytes_per_word * spi_imx->wml);
 		timeout = wait_for_completion_timeout(
 				&spi_imx->dma_rx_completion, transfer_timeout);
 		if (!timeout) {
@@ -1016,7 +1084,9 @@ 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->bytes_per_word);
+
 			reinit_completion(&spi_imx->xfer_done);
 
 			spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TCEN);
@@ -1224,6 +1294,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) {
@@ -1263,8 +1334,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.6.2

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

[Anton Bondarenko]

On 01.11.2015 15:41, Anton Bondarenko wrote:
> From: Anton Bondarenko <anton_bondarenko@mentor.com>
>
> 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 | 133 ++++++++++++++++++++++++++++++++++++++------------
>   1 file changed, 102 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 9b80c7f..06f52c3 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -89,11 +89,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 bytes_per_word;
> +
>   	unsigned int count;
>   	void (*tx)(struct spi_imx_data *);
>   	void (*rx)(struct spi_imx_data *);
> @@ -195,12 +199,31 @@ static unsigned int spi_imx_clkdiv_2(unsigned int fin,
>   	return 7;
>   }
>
> +static int spi_imx_get_bytes_per_word(const int bpw)
> +{
> +	return DIV_ROUND_UP(bpw, BITS_PER_BYTE);
> +}
> +
>   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 = transfer->bits_per_word;
> +
> +	if (!bpw)
> +		bpw = spi->bits_per_word;
>
> -	if (spi_imx->dma_is_inited && transfer->len > spi_imx->wml)
> +	bpw = spi_imx_get_bytes_per_word(bpw);
> +
> +	/*
> +	 * 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.
> +	 */
> +	if (spi_imx->dma_is_inited && (transfer->len > spi_imx->wml * bpw))
>   		return true;
>   	return false;
>   }
> @@ -764,11 +787,60 @@ 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->bytes_per_word) {
> +	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->bytes_per_word);
> +		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->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->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 new_bytes_per_word;
> +	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;
> @@ -792,9 +864,19 @@ 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);
> +	new_bytes_per_word = spi_imx_get_bytes_per_word(config.bpw);
> +	if (spi_imx->dma_is_inited &&
> +	    spi_imx->bytes_per_word != new_bytes_per_word) {
> +		spi_imx->bytes_per_word = new_bytes_per_word;
> +		ret = spi_imx_sdma_configure(spi->master);
> +		if (ret != 0)
> +			pr_err("Can't configure SDMA, error %d\n", ret);
> +	}
> +
> +	if (!ret)
> +		ret = spi_imx->devtype_data->config(spi_imx, &config);
>
> -	return 0;
> +	return ret;
>   }
>
>   static void spi_imx_sdma_exit(struct spi_imx_data *spi_imx)
> @@ -815,10 +897,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 */
> @@ -835,16 +915,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->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) {
> @@ -853,22 +923,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->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->bytes_per_word = 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;
> @@ -925,7 +992,8 @@ 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->bytes_per_word);
>   	struct spi_master *master = spi_imx->bitbang.master;
>   	struct sg_table *tx = &transfer->tx_sg, *rx = &transfer->rx_sg;
>
> @@ -998,7 +1066,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);
> +					spi_imx->bytes_per_word * spi_imx->wml);
>   		timeout = wait_for_completion_timeout(
>   				&spi_imx->dma_rx_completion, transfer_timeout);
>   		if (!timeout) {
> @@ -1016,7 +1084,9 @@ 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->bytes_per_word);
> +
>   			reinit_completion(&spi_imx->xfer_done);
>
>   			spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TCEN);
> @@ -1224,6 +1294,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) {
> @@ -1263,8 +1334,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);
>

Does anyone has other comments regarding this commit?

Regards, Anton

[PATCH v3 4/7] spi: imx: add function to check for IMX51 family controller

[Anton Bondarenko]

From: Anton Bondarenko <anton_bondarenko@mentor.com>

Similar to other controller type checks add check function for
IMX51. This also includes IMX53 and IMX6.

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

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 06f52c3..48fdfa1 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -127,9 +127,14 @@ static inline int is_imx35_cspi(struct spi_imx_data *d)
 	return d->devtype_data->devtype == IMX35_CSPI;
 }
 
+static inline int is_imx51_ecspi(struct spi_imx_data *d)
+{
+	return d->devtype_data->devtype == IMX51_ECSPI;
+}
+
 static inline unsigned spi_imx_get_fifosize(struct spi_imx_data *d)
 {
-	return (d->devtype_data->devtype == IMX51_ECSPI) ? 64 : 8;
+	return is_imx51_ecspi(d) ? 64 : 8;
 }
 
 #define MXC_SPI_BUF_RX(type)						\
@@ -1334,7 +1339,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_imx51_ecspi(spi_imx) &&
 	    spi_imx_sdma_init(&pdev->dev, spi_imx, master))
 		dev_err(&pdev->dev, "dma setup error,use pio instead\n");
 
-- 
2.6.2

Re: [PATCH v3 4/7] spi: imx: add function to check for IMX51 family controller

[Anton Bondarenko]

On 01.11.2015 15:41, Anton Bondarenko wrote:
> From: Anton Bondarenko <anton_bondarenko@mentor.com>
>
> Similar to other controller type checks add check function for
> IMX51. This also includes IMX53 and IMX6.
>
> Signed-off-by: Anton Bondarenko <anton_bondarenko@mentor.com>
> ---
>   drivers/spi/spi-imx.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 06f52c3..48fdfa1 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -127,9 +127,14 @@ static inline int is_imx35_cspi(struct spi_imx_data *d)
>   	return d->devtype_data->devtype == IMX35_CSPI;
>   }
>
> +static inline int is_imx51_ecspi(struct spi_imx_data *d)
> +{
> +	return d->devtype_data->devtype == IMX51_ECSPI;
> +}
> +
>   static inline unsigned spi_imx_get_fifosize(struct spi_imx_data *d)
>   {
> -	return (d->devtype_data->devtype == IMX51_ECSPI) ? 64 : 8;
> +	return is_imx51_ecspi(d) ? 64 : 8;
>   }
>
>   #define MXC_SPI_BUF_RX(type)						\
> @@ -1334,7 +1339,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_imx51_ecspi(spi_imx) &&
>   	    spi_imx_sdma_init(&pdev->dev, spi_imx, master))
>   		dev_err(&pdev->dev, "dma setup error,use pio instead\n");
>
>

Does anyone has other comments regarding this commit?

Regards, Anton

[PATCH v3 5/7] spi: imx: Add support for loopback for ECSPI controllers

[Anton Bondarenko]

From: Anton Bondarenko <anton_bondarenko@mentor.com>

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 | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 48fdfa1..dc492e2 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -270,6 +270,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)
@@ -343,6 +346,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
@@ -385,6 +389,12 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
 	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
 	writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);
 
+	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);
+
 	/*
 	 * Wait until the changes in the configuration register CONFIGREG
 	 * propagate into the hardware. It takes exactly one tick of the
@@ -1262,6 +1272,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)
@@ -1288,10 +1301,10 @@ static int spi_imx_probe(struct platform_device *pdev)
 	spi_imx->bitbang.master->unprepare_message = spi_imx_unprepare_message;
 	spi_imx->bitbang.master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
 
-	init_completion(&spi_imx->xfer_done);
+	if (is_imx51_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.6.2

Re: [PATCH v3 5/7] spi: imx: Add support for loopback for ECSPI controllers

[Anton Bondarenko]

On 01.11.2015 15:41, Anton Bondarenko wrote:
> From: Anton Bondarenko <anton_bondarenko@mentor.com>
>
> 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 | 19 ++++++++++++++++---
>   1 file changed, 16 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
> index 48fdfa1..dc492e2 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -270,6 +270,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)
> @@ -343,6 +346,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
> @@ -385,6 +389,12 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
>   	writel(ctrl, spi_imx->base + MX51_ECSPI_CTRL);
>   	writel(cfg, spi_imx->base + MX51_ECSPI_CONFIG);
>
> +	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);
> +
>   	/*
>   	 * Wait until the changes in the configuration register CONFIGREG
>   	 * propagate into the hardware. It takes exactly one tick of the
> @@ -1262,6 +1272,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)
> @@ -1288,10 +1301,10 @@ static int spi_imx_probe(struct platform_device *pdev)
>   	spi_imx->bitbang.master->unprepare_message = spi_imx_unprepare_message;
>   	spi_imx->bitbang.master->mode_bits = SPI_CPOL | SPI_CPHA | SPI_CS_HIGH;
>
> -	init_completion(&spi_imx->xfer_done);
> +	if (is_imx51_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);
>

Does anyone has other comments regarding this commit?

Regards, Anton

[PATCH v3 6/7] spi: imx: return error from dma channel request

[Anton Bondarenko]

From: Anton Bondarenko <anton_bondarenko@mentor.com>

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 dc492e2..6035ddd 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -923,18 +923,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.6.2

Re: [PATCH v3 6/7] spi: imx: return error from dma channel request

[Sascha Hauer]

On Sun, Nov 01, 2015 at 03:41:40PM +0100, Anton Bondarenko wrote:
> From: Anton Bondarenko <anton_bondarenko@mentor.com>
> 
> 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.

Maybe something like: "fix probe deferral when dmaengine is not yet
available" in the subject line will better express that this is a patch
that should be applied ASAP. Also you can move this up to the front of
the series when resending.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v3 6/7] spi: imx: return error from dma channel request

[Anton Bondarenko]

On 05.11.2015 09:56, Sascha Hauer wrote:
> On Sun, Nov 01, 2015 at 03:41:40PM +0100, Anton Bondarenko wrote:
>> From: Anton Bondarenko <anton_bondarenko@mentor.com>
>>
>> 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.
>
> Maybe something like: "fix probe deferral when dmaengine is not yet
> available" in the subject line will better express that this is a patch
> that should be applied ASAP. Also you can move this up to the front of
> the series when resending.
>
> Sascha
>

This patch itself does not implement deferral for spi-imx if dmaengine 
not ready. The patch targets 2 things: give a bit more detailed error 
information of SPI dma slave channel initialization error and provide a 
basis for the next patch ([PATCH v3 7/7] spi: imx: defer spi 
initialization, if DMA engine is pending).

Regards, Anton

Re: [PATCH v3 6/7] spi: imx: return error from dma channel request

[Anton Bondarenko]

On 05.11.2015 17:00, Anton Bondarenko wrote:
> On 05.11.2015 09:56, Sascha Hauer wrote:
>> On Sun, Nov 01, 2015 at 03:41:40PM +0100, Anton Bondarenko wrote:
>>> From: Anton Bondarenko <anton_bondarenko@mentor.com>
>>>
>>> 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.
>>
>> Maybe something like: "fix probe deferral when dmaengine is not yet
>> available" in the subject line will better express that this is a patch
>> that should be applied ASAP. Also you can move this up to the front of
>> the series when resending.
>>
>> Sascha
>>
>
> This patch itself does not implement deferral for spi-imx if dmaengine
> not ready. The patch targets 2 things: give a bit more detailed error
> information of SPI dma slave channel initialization error and provide a
> basis for the next patch ([PATCH v3 7/7] spi: imx: defer spi
> initialization, if DMA engine is pending).
>
> Regards, Anton

Does anyone has other comments regarding this commit?

Regards, Anton

Re: [PATCH v3 6/7] spi: imx: return error from dma channel request

[Anton Bondarenko]

On 05.11.2015 17:00, Anton Bondarenko wrote:
> On 05.11.2015 09:56, Sascha Hauer wrote:
>> On Sun, Nov 01, 2015 at 03:41:40PM +0100, Anton Bondarenko wrote:
>>> From: Anton Bondarenko <anton_bondarenko@mentor.com>
>>>
>>> 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.
>>
>> Maybe something like: "fix probe deferral when dmaengine is not yet
>> available" in the subject line will better express that this is a patch
>> that should be applied ASAP. Also you can move this up to the front of
>> the series when resending.
>>
>> Sascha
>>
>
> This patch itself does not implement deferral for spi-imx if dmaengine
> not ready. The patch targets 2 things: give a bit more detailed error
> information of SPI dma slave channel initialization error and provide a
> basis for the next patch ([PATCH v3 7/7] spi: imx: defer spi
> initialization, if DMA engine is pending).
>
> Regards, Anton

Does anyone has other comments regarding this commit?

Regards, Anton

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

[Anton Bondarenko]

From: Anton Bondarenko <anton_bondarenko@mentor.com>

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 6035ddd..d2ea731 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c
@@ -1354,9 +1354,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_imx51_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_imx51_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.6.2

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

[Sascha Hauer]

On Sun, Nov 01, 2015 at 03:41:41PM +0100, Anton Bondarenko wrote:
> From: Anton Bondarenko <anton_bondarenko@mentor.com>
> 
> 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>

Ok, so 6/7 is only part of the deferral story. Maybe squash the two
patches into one?

Sascha

> ---
>  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 6035ddd..d2ea731 100644
> --- a/drivers/spi/spi-imx.c
> +++ b/drivers/spi/spi-imx.c
> @@ -1354,9 +1354,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_imx51_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_imx51_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.6.2
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Anton Bondarenko]

On 05.11.2015 09:59, Sascha Hauer wrote:
> On Sun, Nov 01, 2015 at 03:41:41PM +0100, Anton Bondarenko wrote:
>> From: Anton Bondarenko <anton_bondarenko@mentor.com>
>>
>> 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>
>
> Ok, so 6/7 is only part of the deferral story. Maybe squash the two
> patches into one?
>
> Sascha

I would like to keep these two changes separately since I have a small 
concern about this exact change.

Let's assume such scenario:
- ECSPI configured to use DMA in device tree and DMA controller 
description specified. But there is no driver for this controller in kernel.
In this case dmaengine will always return -EDEFER and driver postpone 
spi-imx initialization. So finally there will be no SPI support even in 
PIO mode.
Or for example spi-imx will be compiled into kernel, but DMA controller 
driver will be on SPI-NOR based rootfs.

So the question is "Should we allow user to use incorrect combination of 
kernel configuration and DTS and work at least with some limitation?" Or 
should we try to guarantee device tree file requirement to use DMA.

Any thoughts?

Regards, Anton

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

[Anton Bondarenko]

On 05.11.2015 17:18, Anton Bondarenko wrote:
> On 05.11.2015 09:59, Sascha Hauer wrote:
>> On Sun, Nov 01, 2015 at 03:41:41PM +0100, Anton Bondarenko wrote:
>>> From: Anton Bondarenko <anton_bondarenko@mentor.com>
>>>
>>> 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>
>>
>> Ok, so 6/7 is only part of the deferral story. Maybe squash the two
>> patches into one?
>>
>> Sascha
>
> I would like to keep these two changes separately since I have a small
> concern about this exact change.
>
> Let's assume such scenario:
> - ECSPI configured to use DMA in device tree and DMA controller
> description specified. But there is no driver for this controller in
> kernel.
> In this case dmaengine will always return -EDEFER and driver postpone
> spi-imx initialization. So finally there will be no SPI support even in
> PIO mode.
> Or for example spi-imx will be compiled into kernel, but DMA controller
> driver will be on SPI-NOR based rootfs.
>
> So the question is "Should we allow user to use incorrect combination of
> kernel configuration and DTS and work at least with some limitation?" Or
> should we try to guarantee device tree file requirement to use DMA.
>
> Any thoughts?
>
> Regards, Anton

Does anyone has other comments regarding this commit?

Regards, Anton