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 

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

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 

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

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 

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

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

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

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 

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

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 

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 
---
  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(>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,
+   >sgl[rx->nents - 1], 1,
+   DMA_FROM_DEVICE);
+
+   spi_imx->rx_buf = pio_buffer;
+   spi_imx->txfifo = left;
+   reinit_completion(_imx->xfer_done);
+
+   spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TCEN);
+
+   timeout = wait_for_completion_timeout(
+   _imx->xfer_done, IMX_DMA_TIMEOUT);
+   if (!timeout) {
+   pr_warn("%s %s: I/O Error in RX tail\n",
+   dev_driver_string(>dev),
+   dev_name(>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,
+  >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


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

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 

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 
---
  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(>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,
+   >sgl[rx->nents - 1], 1,
+   DMA_FROM_DEVICE);
+
+   spi_imx->rx_buf = pio_buffer;
+   spi_imx->txfifo = left;
+   reinit_completion(_imx->xfer_done);
+
+   spi_imx->devtype_data->intctrl(spi_imx, MXC_INT_TCEN);
+
+   timeout = wait_for_completion_timeout(
+   _imx->xfer_done, IMX_DMA_TIMEOUT);
+   if (!timeout) {
+   pr_warn("%s %s: I/O Error in RX tail\n",
+   dev_driver_string(>dev),
+   dev_name(>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,
+  >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


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

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 
> 
> 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 
> ---
>  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);
>  
>   

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 
> 
> 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 
> ---
>  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)
> +   

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

[Anton Bondarenko]

From: Anton Bondarenko 

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

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

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

Transfer triggering changed to avoid RX data loss.

Signed-off-by: Anton Bondarenko 
---
 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_TCENBIT(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 

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

[Anton Bondarenko]

From: Anton Bondarenko 

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

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

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

Transfer triggering changed to avoid RX data loss.

Signed-off-by: Anton Bondarenko 
---
 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_TCENBIT(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