Re: [PATCH v2 2/5] spi: spi-fsl-dspi: Use non-coherent memory for DMA

[Robin Murphy <robin.murphy@arm.com>]

On 2025-06-13 10:28 am, James Clark wrote:
> Using coherent memory here isn't functionally necessary. Because the
> change to use non-coherent memory isn't overly complex and only a few
> synchronization points are required, we might as well do it while fixing
> up some other DMA issues.

If it doesn't need coherent memory then does the driver really need to 
do its own bounce-buffering at all? Could you not simplify the whole lot 
even more by getting rid of {tx,rx}_dma_buf altogether and relying on 
the SPI core helpers to DMA-map the messages in-place?

Thanks,
Robin.

> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: James Clark <james.clark@linaro.org>
> ---
>   drivers/spi/spi-fsl-dspi.c | 55 +++++++++++++++++++++++++++++-----------------
>   1 file changed, 35 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c
> index 744dfc561db2..f19404e10c92 100644
> --- a/drivers/spi/spi-fsl-dspi.c
> +++ b/drivers/spi/spi-fsl-dspi.c
> @@ -379,6 +379,11 @@ static bool is_s32g_dspi(struct fsl_dspi *data)
>   	       data->devtype_data == &devtype_data[S32G_TARGET];
>   }
>   
> +static int dspi_dma_transfer_size(struct fsl_dspi *dspi)
> +{
> +	return dspi->words_in_flight * DMA_SLAVE_BUSWIDTH_4_BYTES;
> +}
> +
>   static void dspi_native_host_to_dev(struct fsl_dspi *dspi, u32 *txdata)
>   {
>   	switch (dspi->oper_word_size) {
> @@ -493,7 +498,10 @@ static void dspi_tx_dma_callback(void *arg)
>   {
>   	struct fsl_dspi *dspi = arg;
>   	struct fsl_dspi_dma *dma = dspi->dma;
> +	struct device *dev = &dspi->pdev->dev;
>   
> +	dma_sync_single_for_cpu(dev, dma->tx_dma_phys,
> +				dspi_dma_transfer_size(dspi), DMA_TO_DEVICE);
>   	complete(&dma->cmd_tx_complete);
>   }
>   
> @@ -501,9 +509,13 @@ static void dspi_rx_dma_callback(void *arg)
>   {
>   	struct fsl_dspi *dspi = arg;
>   	struct fsl_dspi_dma *dma = dspi->dma;
> +	struct device *dev = &dspi->pdev->dev;
>   	int i;
>   
>   	if (dspi->rx) {
> +		dma_sync_single_for_cpu(dev, dma->rx_dma_phys,
> +					dspi_dma_transfer_size(dspi),
> +					DMA_FROM_DEVICE);
>   		for (i = 0; i < dspi->words_in_flight; i++)
>   			dspi_push_rx(dspi, dspi->dma->rx_dma_buf[i]);
>   	}
> @@ -513,6 +525,7 @@ static void dspi_rx_dma_callback(void *arg)
>   
>   static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
>   {
> +	size_t size = dspi_dma_transfer_size(dspi);
>   	struct device *dev = &dspi->pdev->dev;
>   	struct fsl_dspi_dma *dma = dspi->dma;
>   	int time_left;
> @@ -521,10 +534,9 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
>   	for (i = 0; i < dspi->words_in_flight; i++)
>   		dspi->dma->tx_dma_buf[i] = dspi_pop_tx_pushr(dspi);
>   
> +	dma_sync_single_for_device(dev, dma->tx_dma_phys, size, DMA_TO_DEVICE);
>   	dma->tx_desc = dmaengine_prep_slave_single(dma->chan_tx,
> -					dma->tx_dma_phys,
> -					dspi->words_in_flight *
> -					DMA_SLAVE_BUSWIDTH_4_BYTES,
> +					dma->tx_dma_phys, size,
>   					DMA_MEM_TO_DEV,
>   					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>   	if (!dma->tx_desc) {
> @@ -539,10 +551,10 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi)
>   		return -EINVAL;
>   	}
>   
> +	dma_sync_single_for_device(dev, dma->rx_dma_phys, size,
> +				   DMA_FROM_DEVICE);
>   	dma->rx_desc = dmaengine_prep_slave_single(dma->chan_rx,
> -					dma->rx_dma_phys,
> -					dspi->words_in_flight *
> -					DMA_SLAVE_BUSWIDTH_4_BYTES,
> +					dma->rx_dma_phys, size,
>   					DMA_DEV_TO_MEM,
>   					DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>   	if (!dma->rx_desc) {
> @@ -644,17 +656,17 @@ static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
>   		goto err_tx_channel;
>   	}
>   
> -	dma->tx_dma_buf = dma_alloc_coherent(dma->chan_tx->device->dev,
> -					     dma_bufsize, &dma->tx_dma_phys,
> -					     GFP_KERNEL);
> +	dma->tx_dma_buf = dma_alloc_noncoherent(dma->chan_tx->device->dev,
> +						dma_bufsize, &dma->tx_dma_phys,
> +						DMA_TO_DEVICE, GFP_KERNEL);
>   	if (!dma->tx_dma_buf) {
>   		ret = -ENOMEM;
>   		goto err_tx_dma_buf;
>   	}
>   
> -	dma->rx_dma_buf = dma_alloc_coherent(dma->chan_rx->device->dev,
> -					     dma_bufsize, &dma->rx_dma_phys,
> -					     GFP_KERNEL);
> +	dma->rx_dma_buf = dma_alloc_noncoherent(dma->chan_rx->device->dev,
> +						dma_bufsize, &dma->rx_dma_phys,
> +						DMA_FROM_DEVICE, GFP_KERNEL);
>   	if (!dma->rx_dma_buf) {
>   		ret = -ENOMEM;
>   		goto err_rx_dma_buf;
> @@ -689,11 +701,12 @@ static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr)
>   	return 0;
>   
>   err_slave_config:
> -	dma_free_coherent(dma->chan_rx->device->dev,
> -			  dma_bufsize, dma->rx_dma_buf, dma->rx_dma_phys);
> +	dma_free_noncoherent(dma->chan_rx->device->dev, dma_bufsize,
> +			     dma->rx_dma_buf, dma->rx_dma_phys,
> +			     DMA_FROM_DEVICE);
>   err_rx_dma_buf:
> -	dma_free_coherent(dma->chan_tx->device->dev,
> -			  dma_bufsize, dma->tx_dma_buf, dma->tx_dma_phys);
> +	dma_free_noncoherent(dma->chan_tx->device->dev, dma_bufsize,
> +			     dma->tx_dma_buf, dma->tx_dma_phys, DMA_TO_DEVICE);
>   err_tx_dma_buf:
>   	dma_release_channel(dma->chan_tx);
>   err_tx_channel:
> @@ -714,14 +727,16 @@ static void dspi_release_dma(struct fsl_dspi *dspi)
>   		return;
>   
>   	if (dma->chan_tx) {
> -		dma_free_coherent(dma->chan_tx->device->dev, dma_bufsize,
> -				  dma->tx_dma_buf, dma->tx_dma_phys);
> +		dma_free_noncoherent(dma->chan_tx->device->dev, dma_bufsize,
> +				     dma->tx_dma_buf, dma->tx_dma_phys,
> +				     DMA_TO_DEVICE);
>   		dma_release_channel(dma->chan_tx);
>   	}
>   
>   	if (dma->chan_rx) {
> -		dma_free_coherent(dma->chan_rx->device->dev, dma_bufsize,
> -				  dma->rx_dma_buf, dma->rx_dma_phys);
> +		dma_free_noncoherent(dma->chan_rx->device->dev, dma_bufsize,
> +				     dma->rx_dma_buf, dma->rx_dma_phys,
> +				     DMA_FROM_DEVICE);
>   		dma_release_channel(dma->chan_rx);
>   	}
>   }
>