Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1284587 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v5 00/11] Improvements for SPI IMX driver for Freescale IMX51, IMX53 and IMX6 Anton Bondarenko <anton.bondarenko.sama@gmail.com> - 2015-12-05 18:00 +0100
    [PATCH v5 06/11] spi: imx: return error from dma channel request Anton Bondarenko <anton.bondarenko.sama@gmail.com> - 2015-12-05 18:00 +0100
      Re: [PATCH v5 06/11] spi: imx: return error from dma channel request Sascha Hauer <s.hauer@pengutronix.de> - 2015-12-07 10:40 +0100
        Re: [PATCH v5 06/11] spi: imx: return error from dma channel request Anton Bondarenko <anton.bondarenko.sama@gmail.com> - 2015-12-08 00:40 +0100
    [PATCH v5 01/11] spi: imx: terminate RX DMA transaction in case of TX DMA timeout Anton Bondarenko <anton.bondarenko.sama@gmail.com> - 2015-12-05 18:00 +0100
    [PATCH v5 08/11] spi: imx: allow only WML aligned transfers to use DMA Anton Bondarenko <anton.bondarenko.sama@gmail.com> - 2015-12-05 18:00 +0100
      Re: [PATCH v5 08/11] spi: imx: allow only WML aligned transfers to  use DMA Sascha Hauer <s.hauer@pengutronix.de> - 2015-12-07 10:50 +0100
        Re: [PATCH v5 08/11] spi: imx: allow only WML aligned transfers to  use DMA Anton Bondarenko <anton.bondarenko.sama@gmail.com> - 2015-12-08 01:40 +0100
          Re: [PATCH v5 08/11] spi: imx: allow only WML aligned transfers to  use DMA Sascha Hauer <s.hauer@pengutronix.de> - 2015-12-08 07:10 +0100
    [PATCH v5 07/11] spi: imx: defer spi initialization, if DMA engine is Anton Bondarenko <anton.bondarenko.sama@gmail.com> - 2015-12-05 18:00 +0100
    [PATCH v5 02/11] spi: imx: reorder HW operations enable order to avoid possible RX data loss Anton Bondarenko <anton.bondarenko.sama@gmail.com> - 2015-12-05 18:00 +0100
    [PATCH v5 10/11] spi: imx: replace fixed timeout with calculated Anton Bondarenko <anton.bondarenko.sama@gmail.com> - 2015-12-05 18:00 +0100

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

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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

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

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.sama@gmail.com>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On Sat, Dec 05, 2015 at 05:57:04PM +0100, Anton Bondarenko wrote:
> On SDMA initialization return exactly the same error, which is
> reported by dma_request_slave_channel_reason(), it is a preceding
> change to defer SPI DMA initialization, if SDMA module is not yet
> available.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> Signed-off-by: Anton Bondarenko <anton.bondarenko.sama@gmail.com>
> ---
>  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 |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On 2015-12-07 10:32, Sascha Hauer wrote:
> On Sat, Dec 05, 2015 at 05:57:04PM +0100, Anton Bondarenko wrote:
>> On SDMA initialization return exactly the same error, which is
>> reported by dma_request_slave_channel_reason(), it is a preceding
>> change to defer SPI DMA initialization, if SDMA module is not yet
>> available.
>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>> Signed-off-by: Anton Bondarenko <anton.bondarenko.sama@gmail.com>
>> ---
>>   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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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 <anton.bondarenko.sama@gmail.com>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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 <anton.bondarenko.sama@gmail.com>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On Sat, Dec 05, 2015 at 05:57:06PM +0100, Anton Bondarenko wrote:
> RX DMA tail data handling doesn't work correctly in many cases with current
> implementation. It happens because SPI core was setup to generates both RX
> 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 <anton.bondarenko.sama@gmail.com>
> ---
>  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 |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On 2015-12-07 10:42, Sascha Hauer wrote:
> On Sat, Dec 05, 2015 at 05:57:06PM +0100, Anton Bondarenko wrote:
>> RX DMA tail data handling doesn't work correctly in many cases with current
>> implementation. It happens because SPI core was setup to generates both RX
>> 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 <anton.bondarenko.sama@gmail.com>
>> ---
>>   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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

On Tue, Dec 08, 2015 at 01:33:18AM +0100, Anton Bondarenko wrote:
> 
> 
> On 2015-12-07 10:42, Sascha Hauer wrote:
> >On Sat, Dec 05, 2015 at 05:57:06PM +0100, Anton Bondarenko wrote:
> >>RX DMA tail data handling doesn't work correctly in many cases with current
> >>implementation. It happens because SPI core was setup to generates both RX
> >>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 <anton.bondarenko.sama@gmail.com>
> >>---
> >>  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 |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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 <anton.bondarenko.sama@gmail.com>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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 <anton.bondarenko.sama@gmail.com>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

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

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 <anton.bondarenko.sama@gmail.com>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web