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

[Changhuang Liang]

Hi, Mark

Thanks for the review.

> 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@xxxxxxxxxxxxxxxx>
> > ---
> >  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.
> 

Yes, previously, considering platforms with only one DMA channel where the tx 
and rx channels can only request one at a time, the initial thought was to request 
them at runtime. I'll try to see if I can re-optimize it to request them during the 
probe stage and eliminate this repeated request/release overhead.

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

Currently, the enhance SPI has clock stretching enabled, so there will be no 
FIFO errors. Therefore, it seems that only DMA errors need to be checked?

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

OK

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

OK, I'll give it a try.

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

The initial idea here was to request the DMA channel at runtime. If the driver 
already has a channel, it would be considered that the previous DMA transfer 
has not yet finished.

Best Regards,
Changhuang