[PATCH v5 00/11] Improvements for SPI IMX driver for Freescale IMX51, IMX53 and IMX6

STALE3908d

Revision v5 of 3 in this series.

19 messages, 2 authors, 2015-12-08 · open the first message on its own page

[PATCH v5 00/11] Improvements for SPI IMX driver for Freescale IMX51, IMX53 and IMX6

From: Anton Bondarenko <hidden>
Date: 2015-12-05 16:57:15

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 V4:
 * Split [PATCH v4 01] into several smaller commits
 * Change [PATCH v4 01] into workaround to disable DMA for transfer which len mod WML
not equal 0
 * Rework some patches to isolate changes in one place

Anton Bondarenko (11):
  spi: imx: terminate RX DMA transaction in case of TX DMA timeout
  spi: imx: reorder HW operations enable order to avoid possible RX data
    loss
  spi: imx: replace multiple watermarks with single for RX, TX and RXT
  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
  spi: imx: allow only WML aligned transfers to use DMA
  spi: imx: remove dead RX DMA tail handling code
  spi: imx: replace fixed timeout with calculated
  spi: imx: add support for all SPI word width for DMA

 drivers/spi/spi-imx.c | 249 ++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 179 insertions(+), 70 deletions(-)

-- 
2.6.3

[PATCH v5 02/11] spi: imx: reorder HW operations enable order to avoid possible RX data loss

From: Anton Bondarenko <hidden>
Date: 2015-12-05 16:57:17

The overflow may happen due to rescheduling for another task and/or interrupt
if we enable SPI HW before starting RX DMA. 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.

The risk rise in case of heavy system load and high SPI clock.

Signed-off-by: Anton Bondarenko <redacted>
---
 drivers/spi/spi-imx.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index fb3bcc4..17e8f9e 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c

@@ -946,10 +946,18 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
 	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);

-- 
2.6.3

[PATCH v5 04/11] spi: imx: add function to check for IMX51 family controller

From: Anton Bondarenko <hidden>
Date: 2015-12-05 16:57:20

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

Signed-off-by: Anton Bondarenko <redacted>
---
 drivers/spi/spi-imx.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index f811f68..363276d 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c

@@ -122,9 +122,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)						\

@@ -1199,8 +1204,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 (is_imx51_ecspi(spi_imx) &&
+	    spi_imx_sdma_init(&pdev->dev, spi_imx, master, res))
 		dev_err(&pdev->dev, "dma setup error,use pio instead\n");
 
 	spi_imx->devtype_data->reset(spi_imx);

-- 
2.6.3

[PATCH v5 06/11] spi: imx: return error from dma channel request

From: Anton Bondarenko <hidden>
Date: 2015-12-05 16:57:23

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 <redacted>
Signed-off-by: Anton Bondarenko <redacted>
---
 drivers/spi/spi-imx.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 3525616..277dd75 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c

@@ -846,10 +846,11 @@ 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;
 	}

@@ -864,10 +865,11 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
 	}
 
 	/* 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;
 	}

@@ -1217,9 +1219,12 @@ 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, res))
-		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, res);
+		if (ret < 0)
+			dev_err(&pdev->dev, "dma setup error %d, use pio\n",
+				ret);
+	}
 
 	spi_imx->devtype_data->reset(spi_imx);

-- 
2.6.3

[PATCH v5 09/11] spi: imx: remove dead RX DMA tail handling code

From: Anton Bondarenko <hidden>
Date: 2015-12-05 16:57:26

transfer->len % wml for DMA capable transactions will always be 0
due to recent change in can_dma checks. So it's safe to remove dead code
in processing DMA.

Signed-off-by: Anton Bondarenko <redacted>
---
 drivers/spi/spi-imx.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 7a68c62..adfa9cf 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c

@@ -917,8 +917,6 @@ 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;
 	struct spi_master *master = spi_imx->bitbang.master;
 	struct sg_table *tx = &transfer->tx_sg, *rx = &transfer->rx_sg;

@@ -952,13 +950,6 @@ 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->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

@@ -990,10 +981,6 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
 			spi_imx->devtype_data->reset(spi_imx);
 			dmaengine_terminate_all(master->dma_rx);
 		}
-		dma &= ~MX51_ECSPI_DMA_RXT_WML_MASK;
-		writel(dma |
-		       spi_imx->wml << MX51_ECSPI_DMA_RXT_WML_OFFSET,
-		       spi_imx->base + MX51_ECSPI_DMA);
 	}
 
 	spi_imx->dma_finished = 1;

-- 
2.6.3

[PATCH v5 11/11] spi: imx: add support for all SPI word width for DMA

From: Anton Bondarenko <hidden>
Date: 2015-12-05 16:57:37

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 16 and 32 BPW.

Signed-off-by: Anton Bondarenko <redacted>
---
 drivers/spi/spi-imx.c | 121 +++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 95 insertions(+), 26 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index f7ee288..d1b9903 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c

@@ -88,11 +88,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 *);

@@ -199,13 +203,32 @@ 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 &&
-	    (transfer->len % spi_imx->wml) == 0)
+	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) &&
+	    (transfer->len % (spi_imx->wml * bpw)) == 0)
 		return true;
 	return false;
 }

@@ -784,11 +807,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;

@@ -812,9 +884,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);
+	}
 
-	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)

@@ -838,7 +920,6 @@ static int spi_imx_sdma_init(struct device *dev, struct spi_imx_data *spi_imx,
 			     struct spi_master *master,
 			     const struct resource *res)
 {
-	struct dma_slave_config slave_config = {};
 	int ret;
 
 	/* use pio mode for i.mx6dl chip TKT238285 */

@@ -856,16 +937,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_reason(dev, "rx");
 	if (IS_ERR(master->dma_rx)) {

@@ -875,22 +946,20 @@ 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;
+	spi_imx->base_phys = res->start;
+	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;

@@ -992,7 +1061,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) {

-- 
2.6.3

[PATCH v5 10/11] spi: imx: replace fixed timeout with calculated

From: Anton Bondarenko <hidden>
Date: 2015-12-05 16:57:55

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 HW internal CS change.

Signed-off-by: Anton Bondarenko <redacted>
---
 drivers/spi/spi-imx.c | 33 +++++++++++++++++++++++++++------
 1 file changed, 27 insertions(+), 6 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index adfa9cf..f7ee288 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c

@@ -56,7 +56,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;

@@ -92,6 +91,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 *);

@@ -318,7 +318,7 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
 {
 	u32 ctrl = MX51_ECSPI_CTRL_ENABLE, cfg = 0, dma = 0;
 	u32 tx_wml_cfg, rx_wml_cfg, rxt_wml_cfg;
-	u32 clk = config->speed_hz, delay;
+	u32 delay;
 	u32 lpb = 0;
 
 	/*

@@ -331,7 +331,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);

@@ -377,7 +379,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 */

@@ -911,11 +913,26 @@ 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 timeout = 0;
+
+	/* Time with actual data transfer and CS change delay related to HW */
+	timeout = (8 + 4) * size / spi_imx->spi_bus_clk;
+
+	/* Add extra second for scheduler related activities */
+	timeout += 1;
+
+	/* Double calculated timeout */
+	return msecs_to_jiffies(2 * timeout * MSEC_PER_SEC);
+}
+
 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 transfer_timeout;
 	unsigned long timeout;
 	struct spi_master *master = spi_imx->bitbang.master;
 	struct sg_table *tx = &transfer->tx_sg, *rx = &transfer->rx_sg;

@@ -962,9 +979,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),

@@ -972,8 +991,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),

-- 
2.6.3

[PATCH v5 08/11] spi: imx: allow only WML aligned transfers to use DMA

From: Anton Bondarenko <hidden>
Date: 2015-12-05 16:57:56

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
and RX TAIL events. And RX TAIL event does not work correctly.
This can be easily verified by sending SPI transaction with size modulus
WML(32 in our case) not equal 0.

Also removing change introduced in f6ee9b582d2db652497b73c1f117591dfb6d3a90
since this change only fix usecases with transfer size from 33 to 128 bytes
and does not fix 129 bytes etc.

This is output from transaction with len 138 bytes in loopback mode at 10Mhz:
TX0000: a3 97 a2 55 53 be f1 fc f9 79 6b 52 14 13 e9 e2
TX0010: 2d 51 8e 1f 56 08 57 27 a7 05 d4 d0 52 82 77 75
TX0020: 1b 99 4a ed 58 3d 6a 52 36 d5 24 4a 68 8e ad 95
TX0030: 5f 3c 35 b5 c4 8c dd 6c 11 32 3d e2 b4 b4 59 cf
TX0040: ce 23 3d 27 df a7 f9 96 fc 1e e0 66 2c 0e 7b 8c
TX0050: ca 30 42 8f bc 9f 7b ce d1 b8 b1 87 ec 8a d6 bb
TX0060: 2e 15 63 0e 3c dc a4 3a 7a 06 20 a7 93 1b 34 dd
TX0070: 4c f5 ec 88 96 68 d6 68 a0 09 6f 8e 93 47 c9 41
TX0080: db ac cf 97 89 f3 51 05 79 71

RX0000: a3 97 a2 55 53 be f1 fc f9 79 6b 52 14 13 e9 e2
RX0010: 2d 51 8e 1f 56 08 57 27 a7 05 d4 d0 52 82 77 75
RX0020: 1b 99 4a ed 58 3d 6a 52 36 d5 24 4a 68 8e ad 95
RX0030: 5f 3c 35 00 00 b5 00 00 00 c4 00 00 8c 00 00 dd
RX0040: 6c 11 32 3d e2 b4 b4 59 cf ce 23 3d 27 df a7 f9
RX0050: 96 fc 1e e0 66 2c 0e 7b 8c ca 30 42 8f 1f 1f bc
RX0060: 9f 7b ce d1 b8 b1 87 ec 8a d6 bb 2e 15 63 0e ed
RX0070: ed 3c 58 58 58 dc 3d 3d a4 6a 6a 3a 52 52 7a 36
RX0080: 06 20 a7 93 1b 34 dd 4c f5 ec

Zeros at offset 33 and 34 caused by reading empty RX FIFO which not possible
if DMA RX read was triggered by RX event. This mean DMA was triggered
by RX TAIL event.

Signed-off-by: Anton Bondarenko <redacted>
---
 drivers/spi/spi-imx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index fa24637..7a68c62 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c

@@ -204,8 +204,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->wml * sizeof(u32))
+	if (spi_imx->dma_is_inited && transfer->len > spi_imx->wml &&
+	    (transfer->len % spi_imx->wml) == 0)
 		return true;
 	return false;
 }

-- 
2.6.3

[PATCH v5 07/11] spi: imx: defer spi initialization, if DMA engine is

From: Anton Bondarenko <hidden>
Date: 2015-12-05 16:58:40

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: Anton Bondarenko <redacted>
---
 drivers/spi/spi-imx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 277dd75..fa24637 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c

@@ -1221,6 +1221,9 @@ static int spi_imx_probe(struct platform_device *pdev)
 	 */
 	if (is_imx51_ecspi(spi_imx)) {
 		ret = spi_imx_sdma_init(&pdev->dev, spi_imx, master, res);
+		if (ret == -EPROBE_DEFER)
+			goto out_clk_put;
+
 		if (ret < 0)
 			dev_err(&pdev->dev, "dma setup error %d, use pio\n",
 				ret);

-- 
2.6.3

[PATCH v5 05/11] spi: imx: Add support for loopback for ECSPI controllers

From: Anton Bondarenko <hidden>
Date: 2015-12-05 16:59:06

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

Signed-off-by: Mohsin Kazmi <redacted>
Signed-off-by: Anton Bondarenko <redacted>
---
 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 363276d..3525616 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c

@@ -246,6 +246,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)

@@ -316,6 +319,7 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
 	u32 ctrl = MX51_ECSPI_CTRL_ENABLE, cfg = 0, dma = 0;
 	u32 tx_wml_cfg, rx_wml_cfg, rxt_wml_cfg;
 	u32 clk = config->speed_hz, delay;
+	u32 lpb = 0;
 
 	/*
 	 * The hardware seems to have a race condition when changing modes. The

@@ -356,6 +360,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

@@ -1128,6 +1138,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)

@@ -1154,10 +1167,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.3

[PATCH v5 03/11] spi: imx: replace multiple watermarks with single for RX, TX and RXT

From: Anton Bondarenko <hidden>
Date: 2015-12-05 16:59:33

There is no need to have different watermarks levels since they are the same.
Merge them into one WML parameter.

Signed-off-by: Anton Bondarenko <redacted>
---
 drivers/spi/spi-imx.c | 29 +++++++++++++----------------
 1 file changed, 13 insertions(+), 16 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 17e8f9e..f811f68 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c

@@ -104,9 +104,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 +199,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;
 }

@@ -378,10 +375,9 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
 	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;
+		rx_wml_cfg = spi_imx->wml << MX51_ECSPI_DMA_RX_WML_OFFSET;
+		tx_wml_cfg = spi_imx->wml << MX51_ECSPI_DMA_TX_WML_OFFSET;
+		rxt_wml_cfg = spi_imx->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)

@@ -832,6 +828,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 +841,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 +859,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 +872,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;

@@ -942,7 +938,7 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
 	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;
+	left = transfer->len % spi_imx->wml;
 	if (left)
 		writel(dma | (left << MX51_ECSPI_DMA_RXT_WML_OFFSET),
 				spi_imx->base + MX51_ECSPI_DMA);

@@ -977,8 +973,9 @@ static int spi_imx_dma_transfer(struct spi_imx_data *spi_imx,
 			spi_imx->devtype_data->reset(spi_imx);
 			dmaengine_terminate_all(master->dma_rx);
 		}
+		dma &= ~MX51_ECSPI_DMA_RXT_WML_MASK;
 		writel(dma |
-		       spi_imx->rxt_wml << MX51_ECSPI_DMA_RXT_WML_OFFSET,
+		       spi_imx->wml << MX51_ECSPI_DMA_RXT_WML_OFFSET,
 		       spi_imx->base + MX51_ECSPI_DMA);
 	}

-- 
2.6.3

[PATCH v5 01/11] spi: imx: terminate RX DMA transaction in case of TX DMA timeout

From: Anton Bondarenko <hidden>
Date: 2015-12-05 16:59:55

Not only TX DMA should be terminated, but RX DMA also. It's required
to avoid accidential DMA memory writes from RX DMA channel and properly
terminate transaction.

Signed-off-by: Anton Bondarenko <redacted>
---
 drivers/spi/spi-imx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 0e5723a..fb3bcc4 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c

@@ -958,6 +958,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);

-- 
2.6.3

Re: [PATCH v5 05/11] spi: imx: Add support for loopback for ECSPI controllers

From: Sascha Hauer <s.hauer@pengutronix.de>
Date: 2015-12-07 09:27:55

On Sat, Dec 05, 2015 at 05:57:03PM +0100, Anton Bondarenko wrote:

quoted hunk

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

Signed-off-by: Mohsin Kazmi <redacted>
Signed-off-by: Anton Bondarenko <redacted>
---
 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 363276d..3525616 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c

@@ -246,6 +246,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)

@@ -316,6 +319,7 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
 	u32 ctrl = MX51_ECSPI_CTRL_ENABLE, cfg = 0, dma = 0;
 	u32 tx_wml_cfg, rx_wml_cfg, rxt_wml_cfg;
 	u32 clk = config->speed_hz, delay;
+	u32 lpb = 0;
 
 	/*
 	 * The hardware seems to have a race condition when changing modes. The

@@ -356,6 +360,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

@@ -1128,6 +1138,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)

@@ -1154,10 +1167,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);

Some unrelated lines are moved in these two hunks. Is this necessary or
just some leftover from development?

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 v5 06/11] spi: imx: return error from dma channel request

From: Sascha Hauer <s.hauer@pengutronix.de>
Date: 2015-12-07 09:32:42

On Sat, Dec 05, 2015 at 05:57:04PM +0100, Anton Bondarenko wrote:

quoted hunk

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 <redacted>
Signed-off-by: Anton Bondarenko <redacted>
---
 drivers/spi/spi-imx.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 3525616..277dd75 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c

@@ -846,10 +846,11 @@ 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");

When changing it can you add the error code to the message? That's
usually the next thing one wants to know when reading it. Also, isn't
dev_dbg enough here? Otherwise the driver gets really verbose when it
actually defers probe.

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 v5 08/11] spi: imx: allow only WML aligned transfers to use DMA

From: Sascha Hauer <s.hauer@pengutronix.de>
Date: 2015-12-07 09:42:32

On Sat, Dec 05, 2015 at 05:57:06PM +0100, Anton Bondarenko wrote:

quoted hunk

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
and RX TAIL events. And RX TAIL event does not work correctly.
This can be easily verified by sending SPI transaction with size modulus
WML(32 in our case) not equal 0.

Also removing change introduced in f6ee9b582d2db652497b73c1f117591dfb6d3a90
since this change only fix usecases with transfer size from 33 to 128 bytes
and does not fix 129 bytes etc.

This is output from transaction with len 138 bytes in loopback mode at 10Mhz:
TX0000: a3 97 a2 55 53 be f1 fc f9 79 6b 52 14 13 e9 e2
TX0010: 2d 51 8e 1f 56 08 57 27 a7 05 d4 d0 52 82 77 75
TX0020: 1b 99 4a ed 58 3d 6a 52 36 d5 24 4a 68 8e ad 95
TX0030: 5f 3c 35 b5 c4 8c dd 6c 11 32 3d e2 b4 b4 59 cf
TX0040: ce 23 3d 27 df a7 f9 96 fc 1e e0 66 2c 0e 7b 8c
TX0050: ca 30 42 8f bc 9f 7b ce d1 b8 b1 87 ec 8a d6 bb
TX0060: 2e 15 63 0e 3c dc a4 3a 7a 06 20 a7 93 1b 34 dd
TX0070: 4c f5 ec 88 96 68 d6 68 a0 09 6f 8e 93 47 c9 41
TX0080: db ac cf 97 89 f3 51 05 79 71

RX0000: a3 97 a2 55 53 be f1 fc f9 79 6b 52 14 13 e9 e2
RX0010: 2d 51 8e 1f 56 08 57 27 a7 05 d4 d0 52 82 77 75
RX0020: 1b 99 4a ed 58 3d 6a 52 36 d5 24 4a 68 8e ad 95
RX0030: 5f 3c 35 00 00 b5 00 00 00 c4 00 00 8c 00 00 dd
RX0040: 6c 11 32 3d e2 b4 b4 59 cf ce 23 3d 27 df a7 f9
RX0050: 96 fc 1e e0 66 2c 0e 7b 8c ca 30 42 8f 1f 1f bc
RX0060: 9f 7b ce d1 b8 b1 87 ec 8a d6 bb 2e 15 63 0e ed
RX0070: ed 3c 58 58 58 dc 3d 3d a4 6a 6a 3a 52 52 7a 36
RX0080: 06 20 a7 93 1b 34 dd 4c f5 ec

Zeros at offset 33 and 34 caused by reading empty RX FIFO which not possible
if DMA RX read was triggered by RX event. This mean DMA was triggered
by RX TAIL event.

Signed-off-by: Anton Bondarenko <redacted>
---
 drivers/spi/spi-imx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index fa24637..7a68c62 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c

@@ -204,8 +204,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->wml * sizeof(u32))
+	if (spi_imx->dma_is_inited && transfer->len > spi_imx->wml &&
+	    (transfer->len % spi_imx->wml) == 0)
 		return true;

Must transfer->len really be bigger than spi_imx->wml? I would assume it
should be >= instead. And where is the * sizeof(u32) gone? If that's
unnecessary I heven't understood why.

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 v5 05/11] spi: imx: Add support for loopback for ECSPI controllers

From: Anton Bondarenko <hidden>
Date: 2015-12-07 21:58:29


On 2015-12-07 10:27, Sascha Hauer wrote:

On Sat, Dec 05, 2015 at 05:57:03PM +0100, Anton Bondarenko wrote:

quoted

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

Signed-off-by: Mohsin Kazmi <redacted>
Signed-off-by: Anton Bondarenko <redacted>
---
  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 363276d..3525616 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c

@@ -246,6 +246,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)

@@ -316,6 +319,7 @@ static int __maybe_unused mx51_ecspi_config(struct spi_imx_data *spi_imx,
  	u32 ctrl = MX51_ECSPI_CTRL_ENABLE, cfg = 0, dma = 0;
  	u32 tx_wml_cfg, rx_wml_cfg, rxt_wml_cfg;
  	u32 clk = config->speed_hz, delay;
+	u32 lpb = 0;

  	/*
  	 * The hardware seems to have a race condition when changing modes. The

@@ -356,6 +360,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

@@ -1128,6 +1138,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)

@@ -1154,10 +1167,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);

Some unrelated lines are moved in these two hunks. Is this necessary or
just some leftover from development?

Sascha

Sascha,

This is necessary because is_imx51_ecspi function is base on content of 
spi_imx->devtype_data (see previous commit PATCH v5 04/11). I could try 
to move SPI_LOOP mode bit set to avoid change for completion initialization.

Regards, Anton

Re: [PATCH v5 06/11] spi: imx: return error from dma channel request

From: Anton Bondarenko <hidden>
Date: 2015-12-07 23:38:24


On 2015-12-07 10:32, Sascha Hauer wrote:

On Sat, Dec 05, 2015 at 05:57:04PM +0100, Anton Bondarenko wrote:

quoted

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 <redacted>
Signed-off-by: Anton Bondarenko <redacted>
---
  drivers/spi/spi-imx.c | 27 ++++++++++++++++-----------
  1 file changed, 16 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index 3525616..277dd75 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c

@@ -846,10 +846,11 @@ 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");

When changing it can you add the error code to the message? That's
usually the next thing one wants to know when reading it. Also, isn't
dev_dbg enough here? Otherwise the driver gets really verbose when it
actually defers probe.

Sascha

Agreed. But the error from spi_imx_sdma_init printed in probe. Anyway, 
I've changed the code as requested in upcoming series V6.

Regards, Anton

Re: [PATCH v5 08/11] spi: imx: allow only WML aligned transfers to use DMA

From: Anton Bondarenko <hidden>
Date: 2015-12-08 00:33:23


On 2015-12-07 10:42, Sascha Hauer wrote:

On Sat, Dec 05, 2015 at 05:57:06PM +0100, Anton Bondarenko wrote:

quoted

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
and RX TAIL events. And RX TAIL event does not work correctly.
This can be easily verified by sending SPI transaction with size modulus
WML(32 in our case) not equal 0.

Also removing change introduced in f6ee9b582d2db652497b73c1f117591dfb6d3a90
since this change only fix usecases with transfer size from 33 to 128 bytes
and does not fix 129 bytes etc.

This is output from transaction with len 138 bytes in loopback mode at 10Mhz:
TX0000: a3 97 a2 55 53 be f1 fc f9 79 6b 52 14 13 e9 e2
TX0010: 2d 51 8e 1f 56 08 57 27 a7 05 d4 d0 52 82 77 75
TX0020: 1b 99 4a ed 58 3d 6a 52 36 d5 24 4a 68 8e ad 95
TX0030: 5f 3c 35 b5 c4 8c dd 6c 11 32 3d e2 b4 b4 59 cf
TX0040: ce 23 3d 27 df a7 f9 96 fc 1e e0 66 2c 0e 7b 8c
TX0050: ca 30 42 8f bc 9f 7b ce d1 b8 b1 87 ec 8a d6 bb
TX0060: 2e 15 63 0e 3c dc a4 3a 7a 06 20 a7 93 1b 34 dd
TX0070: 4c f5 ec 88 96 68 d6 68 a0 09 6f 8e 93 47 c9 41
TX0080: db ac cf 97 89 f3 51 05 79 71

RX0000: a3 97 a2 55 53 be f1 fc f9 79 6b 52 14 13 e9 e2
RX0010: 2d 51 8e 1f 56 08 57 27 a7 05 d4 d0 52 82 77 75
RX0020: 1b 99 4a ed 58 3d 6a 52 36 d5 24 4a 68 8e ad 95
RX0030: 5f 3c 35 00 00 b5 00 00 00 c4 00 00 8c 00 00 dd
RX0040: 6c 11 32 3d e2 b4 b4 59 cf ce 23 3d 27 df a7 f9
RX0050: 96 fc 1e e0 66 2c 0e 7b 8c ca 30 42 8f 1f 1f bc
RX0060: 9f 7b ce d1 b8 b1 87 ec 8a d6 bb 2e 15 63 0e ed
RX0070: ed 3c 58 58 58 dc 3d 3d a4 6a 6a 3a 52 52 7a 36
RX0080: 06 20 a7 93 1b 34 dd 4c f5 ec

Zeros at offset 33 and 34 caused by reading empty RX FIFO which not possible
if DMA RX read was triggered by RX event. This mean DMA was triggered
by RX TAIL event.

Signed-off-by: Anton Bondarenko <redacted>
---
  drivers/spi/spi-imx.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index fa24637..7a68c62 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c

@@ -204,8 +204,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->wml * sizeof(u32))
+	if (spi_imx->dma_is_inited && transfer->len > spi_imx->wml &&
+	    (transfer->len % spi_imx->wml) == 0)
  		return true;

Must transfer->len really be bigger than spi_imx->wml? I would assume it
should be >= instead. And where is the * sizeof(u32) gone? If that's
unnecessary I heven't understood why.

Sascha

Agree on '>='. Will be in V6.
As for missing sizeof(u32).
According to SoC specification it's possible to operate with 8- or 16 
bits word directly by setting proper value in word_width field in CFG 
register. I do not know the what exactly is fixed by f6ee9b582d, but I 
could suspect such scenario:
Some SPI client (spi-nor?) trying to perform SPI transaction with len 
mod 32 not equal zero. It could be any value between 33 and 127, 
excluding 64. But since DMA RX tail functionality does not work 
correctly RX contains some garbage.

But since this commit make can_dma more strict there is no need in old 
fix anymore.

I'll double check with oscilloscope to be sure SPI stream looks good.

Sascha, if you still thinks we need to have sizeof(32) please provide 
the use case or example so I can work on it.

BTW, we need to multiply WML by word size to correctly support 16- and 
32-bits words, but this is done in following commit.

Regards, Anton

Re: [PATCH v5 08/11] spi: imx: allow only WML aligned transfers to use DMA

From: Sascha Hauer <s.hauer@pengutronix.de>
Date: 2015-12-08 06:05:23

On Tue, Dec 08, 2015 at 01:33:18AM +0100, Anton Bondarenko wrote:


On 2015-12-07 10:42, Sascha Hauer wrote:

quoted

On Sat, Dec 05, 2015 at 05:57:06PM +0100, Anton Bondarenko wrote:

quoted

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
and RX TAIL events. And RX TAIL event does not work correctly.
This can be easily verified by sending SPI transaction with size modulus
WML(32 in our case) not equal 0.

Also removing change introduced in f6ee9b582d2db652497b73c1f117591dfb6d3a90
since this change only fix usecases with transfer size from 33 to 128 bytes
and does not fix 129 bytes etc.

This is output from transaction with len 138 bytes in loopback mode at 10Mhz:
TX0000: a3 97 a2 55 53 be f1 fc f9 79 6b 52 14 13 e9 e2
TX0010: 2d 51 8e 1f 56 08 57 27 a7 05 d4 d0 52 82 77 75
TX0020: 1b 99 4a ed 58 3d 6a 52 36 d5 24 4a 68 8e ad 95
TX0030: 5f 3c 35 b5 c4 8c dd 6c 11 32 3d e2 b4 b4 59 cf
TX0040: ce 23 3d 27 df a7 f9 96 fc 1e e0 66 2c 0e 7b 8c
TX0050: ca 30 42 8f bc 9f 7b ce d1 b8 b1 87 ec 8a d6 bb
TX0060: 2e 15 63 0e 3c dc a4 3a 7a 06 20 a7 93 1b 34 dd
TX0070: 4c f5 ec 88 96 68 d6 68 a0 09 6f 8e 93 47 c9 41
TX0080: db ac cf 97 89 f3 51 05 79 71

RX0000: a3 97 a2 55 53 be f1 fc f9 79 6b 52 14 13 e9 e2
RX0010: 2d 51 8e 1f 56 08 57 27 a7 05 d4 d0 52 82 77 75
RX0020: 1b 99 4a ed 58 3d 6a 52 36 d5 24 4a 68 8e ad 95
RX0030: 5f 3c 35 00 00 b5 00 00 00 c4 00 00 8c 00 00 dd
RX0040: 6c 11 32 3d e2 b4 b4 59 cf ce 23 3d 27 df a7 f9
RX0050: 96 fc 1e e0 66 2c 0e 7b 8c ca 30 42 8f 1f 1f bc
RX0060: 9f 7b ce d1 b8 b1 87 ec 8a d6 bb 2e 15 63 0e ed
RX0070: ed 3c 58 58 58 dc 3d 3d a4 6a 6a 3a 52 52 7a 36
RX0080: 06 20 a7 93 1b 34 dd 4c f5 ec

Zeros at offset 33 and 34 caused by reading empty RX FIFO which not possible
if DMA RX read was triggered by RX event. This mean DMA was triggered
by RX TAIL event.

Signed-off-by: Anton Bondarenko <redacted>
---
 drivers/spi/spi-imx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi-imx.c b/drivers/spi/spi-imx.c
index fa24637..7a68c62 100644
--- a/drivers/spi/spi-imx.c
+++ b/drivers/spi/spi-imx.c

@@ -204,8 +204,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->wml * sizeof(u32))
+	if (spi_imx->dma_is_inited && transfer->len > spi_imx->wml &&
+	    (transfer->len % spi_imx->wml) == 0)
 		return true;

Must transfer->len really be bigger than spi_imx->wml? I would assume it
should be >= instead. And where is the * sizeof(u32) gone? If that's
unnecessary I heven't understood why.

Sascha

Agree on '>='. Will be in V6.
As for missing sizeof(u32).
According to SoC specification it's possible to operate with 8- or 16 bits
word directly by setting proper value in word_width field in CFG register. I
do not know the what exactly is fixed by f6ee9b582d, but I could suspect
such scenario:
Some SPI client (spi-nor?) trying to perform SPI transaction with len mod 32
not equal zero. It could be any value between 33 and 127, excluding 64. But
since DMA RX tail functionality does not work correctly RX contains some
garbage.

I just tested it. Back then I had 60byte transfers with the SPI NOR
driver. What I got is:

spi_master spi0: I/O Error in DMA RX
spi_master spi0: failed to transfer one message from queue

I can't follow anymore what led me to the assumption that this issue is
related to byte/wordsize mixup. The SPI NOR driver uses 8bit transfers
so one word is one byte, something the driver handles correctly.

I just tested your series successfully with this usecase. I can redo the
test with v6 once you send it and provide my tested-by tag.

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 |

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help