Re: [PATCH v1 2/2] spi: dw: Add DMA support for enhanced memory operations

[Mark Brown <broonie@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 5843 bytes --]

On Wed, Sep 23, 2026 at 02:57:05AM -0700, Changhuang Liang wrote:
> Implement DMA support for enhanced SPI memory operations. On some
> platforms, such as JHB100, only one DMA channel is available for the
> enhanced SPI controller, so the channel is allocated dynamically based
> on the transfer direction.
> 
> Add enhanced memory DMA callbacks (init/exit, setup, can_dma, transfer)
> to struct dw_spi_dma_ops and hook them into the generic DMA operations.
> The channel is requested per operation and released afterwards.
> 
> In dw_spi_exec_enh_mem_op(), record the direction in dws->tx_dir and use
> the DMA path when a channel is available, can_dma_enh_mem() accepts the
> transfer, and the transfer exceeds the FIFO length; otherwise fall back
> to the interrupt path.
> 
> Rework dw_spi_dma_wait_tx_done() to take an explicit speed_hz, add one
> to TXFLR for the word possibly left in the shift register, and compute
> the delay in ns or us, dropping the dependency on the xfer pointer.
> 
> Also move the udelay(5) workaround into dw_spi_enh_write_cmd_addr() so
> it applies to both paths, and enable the generic DMA setup in
> dw_spi_jhb100_init().
> 
> Signed-off-by: Changhuang Liang <changhuang.liang@starfivetech.com>
> ---
>  drivers/spi/spi-dw-core.c | 107 ++++++++++-----
>  drivers/spi/spi-dw-dma.c  | 281 +++++++++++++++++++++++++++++++++++++-
>  drivers/spi/spi-dw-mmio.c |   2 +
>  drivers/spi/spi-dw.h      |  10 ++
>  4 files changed, 357 insertions(+), 43 deletions(-)
> 
> diff --git a/drivers/spi/spi-dw-core.c b/drivers/spi/spi-dw-core.c
> index 04a5b48373e1..6da3812f72f6 100644
> --- a/drivers/spi/spi-dw-core.c
> +++ b/drivers/spi/spi-dw-core.c
> @@ -970,6 +970,15 @@ static void dw_spi_enh_write_cmd_addr(struct dw_spi *dws, const struct spi_mem_o
>  
>  		dw_spi_set_cs(mem->spi, false);
>  	}
> +
> +	/*
> +	 * FIXME: The exact reason for this delay is not fully understood,
> +	 * but empirical testing shows it significantly improves the stability
> +	 * of read/write operations. Without this delay, occasional transfer
> +	 * errors or timeouts may occur under certain conditions.
> +	 * Keeping it as a safeguard based on practical validation.
> +	 */
> +	udelay(5);
>  }
>  
>  static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *op)
> @@ -981,6 +990,8 @@ static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *
>  	unsigned long long ms;
>  	int ret;
>  
> +	dws->dma_mapped = false;
> +
>  	switch (op->data.buswidth) {
>  	case 0:
>  	case 1:
> @@ -1004,10 +1015,13 @@ static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *
>  	cfg.dfs = 8;
>  	cfg.freq = clamp(op->max_freq, 0U, dws->max_mem_freq);
>  	cfg.ndf = op->data.nbytes;
> -	if (op->data.dir == SPI_MEM_DATA_IN)
> +	if (op->data.dir == SPI_MEM_DATA_IN) {
>  		cfg.tmode = DW_SPI_CTRLR0_TMOD_RO;
> -	else
> +		dws->tx_dir = false;
> +	} else {
>  		cfg.tmode = DW_SPI_CTRLR0_TMOD_TO;
> +		dws->tx_dir = true;
> +	}
>  
>  	if (op->data.buswidth == op->addr.buswidth &&
>  	    op->data.buswidth == op->cmd.buswidth)
> @@ -1043,47 +1057,68 @@ static int dw_spi_exec_enh_mem_op(struct spi_mem *mem, const struct spi_mem_op *
>  		}
>  	}
>  
> -	dw_spi_enh_write_cmd_addr(dws, op, mem);
> -
> -	/*
> -	 * FIXME: The exact reason for this delay is not fully understood,
> -	 * but empirical testing shows it significantly improves the stability
> -	 * of read/write operations. Without this delay, occasional transfer
> -	 * errors or timeouts may occur under certain conditions.
> -	 * Keeping it as a safeguard based on practical validation.
> -	 */
> -	udelay(5);
> +	if (dws->dma_ops && dws->dma_ops->dma_enh_mem_init &&
> +	    dws->dma_ops->can_dma_enh_mem && op->data.nbytes > dws->fifo_len) {
> +		ret = dws->dma_ops->dma_enh_mem_init(ctlr->dev.parent, dws);
> +		if (ret) {
> +			dev_dbg(&ctlr->dev, "DMA enh mem init failed (%d)\n", ret);

This doesn't pay attention to the probe time detection of DMA, and
there's no probe time path for requesting the channels so we'll always
drop and request the channel.

> +	if (dws->dma_mapped) {
> +		dw_spi_enh_write_cmd_addr(dws, op, mem);
>  
> -	/* Use timeout calculation from spi_transfer_wait() */
> -	ms = 8LL * MSEC_PER_SEC * (dws->rx_len ? dws->rx_len : dws->tx_len);
> -	do_div(ms, dws->current_freq);
> +		ret = dws->dma_ops->dma_enh_mem_transfer(mem, op);
>  
> -	/*
> -	 * Increase it twice and add 200 ms tolerance, use
> -	 * predefined maximum in case of overflow.
> -	 */
> -	ms += ms + 200;
> -	if (ms > UINT_MAX)
> -		ms = UINT_MAX;
> +		dws->dma_ops->dma_enh_mem_exit(dws);

Do we check for errors from the hardware anywhere?

> +static int dw_spi_dma_wait_tx_done(struct dw_spi *dws, u32 speed_hz)
>  {
>  	int retry = DW_SPI_WAIT_RETRIES;
>  	struct spi_delay delay;
> +	unsigned long ns, us;
>  	u32 nents;
>  
> -	nents = dw_readl(dws, DW_SPI_TXFLR);
> -	delay.unit = SPI_DELAY_UNIT_SCK;
> -	delay.value = nents * dws->n_bytes * BITS_PER_BYTE;
> +	/* Account for the word that may still be in the shift register */
> +	nents = dw_readl(dws, DW_SPI_TXFLR) + 1;

This affects all DMA users and wasn't really explained, it should
probably be a separate patch.

> +static int dw_spi_enh_mem_dma_caps_init(struct dw_spi *dws)
> +{

> +static void dw_spi_enh_mem_dma_maxburst_init(struct dw_spi *dws)
> +{

These are very close to the non-enhanced versions, it would be better to
factor out the common bits.

> +static int dw_spi_enh_mem_dma_init_generic(struct device *dev, struct dw_spi *dws)
> +{
> +	int ret;
> +
> +	if (dws->tx_dir) {
> +		if (dws->txchan)
> +			return -EBUSY;

Surely if the driver already has a channel there's no error?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]