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

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

[Mehmet Fide]

Please consider commit cac7e5054115 ("spi: spi-fsl-dspi: Avoid setup_accel
logic for DMA transfers") for 6.12.y. It is in mainline since v6.17 and
applies cleanly to 6.12.y.

It went in as a cleanup, so it carries neither a Fixes: tag nor a stable
tag, but on vf610 the logic it removes corrupts data in DMA mode, in two
ways:

1. For an odd number of bytes the accel logic packs into 16 bit frames and
   computes the word count as len / 2, so a 5 byte transfer puts 4 bytes in
   the first DMA transfer and leaves the last byte to a second one with
   different timing.

2. With SPI_LSB_FIRST the accel helpers byte swap through cpu_to_be16(),
   which combines with the hardware LSBFE bit and puts multi byte transfers
   on the wire in the wrong byte order.

Both are visible on a Colibri VF50 (vf610, DSPI in DMA mode), and both go
away with this commit, which is what we have been carrying locally against
6.12 to get correct transfers. Every stable tree older than 6.17 is
affected in the same way.

Thanks,
Mehmet

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

[Mehmet Fide]

Some measurements on the hardware, in case they help decide this one.

I ran the transfers that fail on 6.12 against 6.18.44, which carries
cac7e5054115, on the same board (Colibri VF50, vf610, DSPI in DMA mode) with
MOSI and MISO shorted at the connector, using spidev_test:

  3, 5, 7, 9, 11 and 13 byte transfers: every byte comes back, in order.
  Even lengths and a single byte: fine as well.
  SPI_LSB_FIRST at 2, 3, 5, 8 and 13 bytes: bytes come back unchanged.

Without the commit, the same odd length transfers lose the tail of the buffer,
because the accel path packs into 16 bit frames and computes the word count as
len / 2, and SPI_LSB_FIRST comes back byte swapped, because the accel helpers
byte swap through cpu_to_be16() on top of the hardware LSBFE bit. That is what
we have been carrying a local equivalent for.

So the commit is what makes those transfers correct on this hardware, not just
a saving of pointless repacking.

Thanks,
Mehmet

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

[James Clark]

On 11/08/2026 09:40, Mehmet Fide wrote:
> Some measurements on the hardware, in case they help decide this one.
> 
> I ran the transfers that fail on 6.12 against 6.18.44, which carries
> cac7e5054115, on the same board (Colibri VF50, vf610, DSPI in DMA mode) with
> MOSI and MISO shorted at the connector, using spidev_test:
> 
>    3, 5, 7, 9, 11 and 13 byte transfers: every byte comes back, in order.
>    Even lengths and a single byte: fine as well.
>    SPI_LSB_FIRST at 2, 3, 5, 8 and 13 bytes: bytes come back unchanged.
> 
> Without the commit, the same odd length transfers lose the tail of the buffer,
> because the accel path packs into 16 bit frames and computes the word count as
> len / 2, and SPI_LSB_FIRST comes back byte swapped, because the accel helpers
> byte swap through cpu_to_be16() on top of the hardware LSBFE bit. That is what
> we have been carrying a local equivalent for.
> 
> So the commit is what makes those transfers correct on this hardware, not just
> a saving of pointless repacking.
> 
> Thanks,
> Mehmet

It's possible that this was originally done as a fix but I interpreted 
it as a refactor. As part of upstreaming this I broke down and added 
commit messages to changes from NXPs board support branch. It does make 
sense that this change was added after seeing the bug, rather than just 
a clean up. But it could also be a coincidence.

James

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

[Mehmet Fide]

Thanks, that fits what we see. Larisa is on cc, so she may remember whether it
started as a fix in the NXP branch.

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.

Thanks,
Mehmet

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

[Sasha Levin]

> 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

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

[Vladimir Oltean]

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


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.

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

[James Clark]

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.