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

[James Clark <james.clark@linaro.org>]

On 16/06/2025 12:56 pm, Robin Murphy wrote:
> 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.
> 

In this case the device needs a control word to be written into the 
buffer for every SPI word to be sent, so it didn't fit into what was in 
the core layer. But we did look into doing it that way.

James

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