Re: [PATCH 6.12.y] spi: spi-fsl-dspi: Avoid setup_accel logic for DMA transfers

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

On 11/08/2026 22:28, Vladimir Oltean wrote:
> On Tue, Aug 11, 2026 at 02:55:56PM -0400, Sasha Levin wrote:
>>> Either way the behaviour is the same. On vf610 in DMA mode the accel path drops
>>> the tail of odd length transfers and byte swaps under SPI_LSB_FIRST, and this
>>> commit removes both. That is what makes it worth having in 6.12.y, whatever the
>>> original intent was.
>>
>> cac7e5054115 ("spi: spi-fsl-dspi: Avoid setup_accel logic for DMA transfers")
>> applies cleanly to 6.12 with no dependencies, so this is not a mechanical
>> question - it is whether it qualifies. It has no Fixes: tag and no stable tag,
>> and James reads it as a refactor.
>>
>> Larisa, Mark, Vladimir - was this a fix? If so, a Fixes: tag would let me take
>> it here and on the older trees, where it applies just as cleanly.
>>
>> -- 
>> Thanks,
>> Sasha
> 
> It wasn't understood as a correctness change until now, but yes, it is a fix.
> 
> Fixes: a957499bd437 ("spi: spi-fsl-dspi: Fix bits-per-word acceleration in DMA mode")
> Acked-by: Vladimir Oltean <olteanv@gmail.com>
> 
> Explanation:
> As part of the original introduction of dspi_setup_accel() in commit
> 6c1c26ecd9a3 ("spi: spi-fsl-dspi: Accelerate transfers using larger word
> size if possible"), it was well understood that this is not applicable
> to DMA transfers.
> 
> The reason is that the correct clustering of 8 bit frames into 16 bit PUSHR
> transfers ultimately depends on the ability to modify the SPI_CTAR_FMSZ
> (frame size) on the go. In the case of a 3 byte SPI transfer using
> 8-on-16 acceleration, the logic of this clustering is to first transfer
> the first 2 bytes using a 16-bit PUSHR transfer (with SPI_CTAR_FMSZ=15),
> then to update SPI_CTAR_FMSZ=7 in order to be able to push the last byte
> using a single 8-bit PUSHR write.
> 
> The difference between FIFO mode and DMA mode is that in DMA mode, there
> is no software hook to update SPI_CTAR_FMSZ in between PUSHR FIFO
> updates. The DMA engine handles them.
> 
> This was well understood and was the basis of this code path, which
> explicitly excluded DMA from dspi_setup_accel() with its dynamic frame
> size updating:
> 
> 		/*
> 		 * Static CTAR setup for modes that don't dynamically adjust it
> 		 * via dspi_setup_accel (aka for DMA)
> 		 */
> 		regmap_write(dspi->regmap, SPI_CTAR(0),
> 			     dspi->cur_chip->ctar_val |
> 			     SPI_FRAME_BITS(transfer->bits_per_word));
> 
> However, this truth was forgotten soon after, because as soon as a bug
> report came in - the trigger behind commit a957499bd437 ("spi:
> spi-fsl-dspi: Fix bits-per-word acceleration in DMA mode") - it became
> broken.
> 
> Namely, the separate code path for static SPI_CTAR_FMSZ settings for DMA
> mode got deleted, and dspi_dma_xfer() started calling dspi_setup_accel().
> This had two effects:
> - dspi_setup_accel() correctly updates dspi->oper_word_size, necessary
>    in common code: intended, fixes the bug reported by Michael Walle
> - dspi_setup_accel() enables 8-on-16 acceleration for DMA mode now,
>    which will transfer 1 byte too few if the buffer size is odd (it
>    incorrectly assumes that the caller can dynamically alter
>    SPI_CTAR_FMSZ and then send the trailing word separately):
>    unintended, causes the bug reported by Mehmet Fide
> 
> The breakage probably went largely unnoticed because Michael Walle's
> peripheral only used even-sized buffers (a flash, IIRC), and the silicon
> on which I regularly test the DSPI driver doesn't use DMA.
> 
> The commit under question here - cac7e5054115 ("spi: spi-fsl-dspi: Avoid
> setup_accel logic for DMA transfers") - fixes the unintended side effect
> while maintaining the intention of previous bug fix a957499bd437 ("spi:
> spi-fsl-dspi: Fix bits-per-word acceleration in DMA mode"). By having
> the "goto no_accel", we bypass the 8-on-16 acceleration on DMA, while
> still assigning dspi->oper_word_size - which was the reason for calling
> dspi_setup_accel() in the first place.
> 
> Note that 8-on-16 acceleration is not intrinsically broken for DMA mode
> (it can yield a DMA buffer more densely packed with PUSHR data), it just

My commit message on cac7e5054115 was probably a bit misleading then, 
because there is some benefit. I was only thinking from the point of the 
FIFO, not the memory backing a DMA transfer.

> needs more work to skip it for odd-sized transfers. However, that work
> may or may not be justified from a performance standpoint, so the
> approach taken here is reasonable.
> 

The commit could have mentioned that it wastes 1 byte per entry in favor 
of simplicity and correctness. But DMA isn't limited in size like the 
FIFO, so waste isn't an issue.

> 
> Regarding the SPI_LSB_FIRST issue - from the description it seems to be
> a completely distinct problem not intrinsically limited to DMA mode
> (should also be visible in XSPI mode), so disabling dspi_setup_accel()
> on Vyber and Coldfire only partially addresses it.
> 
> I don't have a use case for SPI_LSB_FIRST peripherals, so I don't
> personally mind another "goto no_accel" follow-up patch rather than
> fixing the underlying byte packing mechanism, BUT this should be done
> by the issue reporter with a proper explanation in the commit message
> now that the issue is more clearly understood, rather than just be
> happy that backporting commit cac7e5054115 sidesteps the problem on his
> platform.
> 
> I am currently on vacation, and I am unable to do much testing on actual
> hardware. I also haven't completely evaluated the SPI_LSB_FIRST behaviour
> with 8-on-16 acceleration, it just *seems* plausible that there is an issue.