spi: dw: Fix non-DMA transmit-only transfers #6286
Conversation
|
It is a bit better now, but still not quite right, the TX transfer is often missing one or two clock cycles, see eg this screenshot where TX phase was one cycle short and the ID read back as 2881 instead of 5102: I've also seen one case where the kernel reported 1440 (so 2 cycles missing) and another one where the ID was read fine but a later SPI transfer failed |
|
Although the RP1 SPI interface supports transmit-only, it lacks an interrupt-on-idle. When the TX-FIFO-empty interrupt fires, there may be some data left in the TX shift register. Spinning in interrupt context is not recommended, and by the next data is ready to be sent it may not even be necessary to wait, so we can replace any post-TX-wait with a pre-transfer idle-wait. If this works for you I'll squash it with the previous patch. |
|
Still no joy, in case of a single write-only transfer chip-select is de-asserted too early: |
|
I was hoping not to have to resort to a timer, but I can't think of an alternative with decent performance. Third time lucky? |
drivers/spi/spi-dw-core.c
Outdated
| @@ -246,7 +272,16 @@ static irqreturn_t dw_spi_transfer_handler(struct dw_spi *dws) | |||
| } | |||
| } else if (!dws->tx_len) { | |||
| dw_spi_mask_intr(dws, DW_SPI_INT_TXEI); | |||
| spi_finalize_current_transfer(dws->host); | |||
| if (dw_spi_ctlr_busy(dws)) { | |||
| dws->idle_wait_retries = 8; | |||
There was a problem hiding this comment.
If this is an analogue for bits per data element aka "word width", SPI can be configured up to 32. This value should be in one of the TX configuration registers...
There was a problem hiding this comment.
There should be somewhere more accessible.
There was a problem hiding this comment.
And there was - dws->n_bytes.
|
Thanks a lot for the update, testing with the cirrus logic audio card looks fine so far (also on the scope) |
|
Thanks for the feedback. I'm trying to tune the wait intervals - exponential backoff feels better than up to 32 timer interrupts - but otherwise this feels like the right fix. |
|
Apart from squashing the commits, this is the release candidate. It should reduce the number of interrupts taken to detect the end-of-transfer. |
Ensure the transmit FIFO has emptied before ending the transfer by dropping the TX threshold to 0 when the last byte has been pushed into the FIFO. Include a similar fix for the non-IRQ paths. See: raspberrypi#6285 Fixes: 6014649 ("spi: dw: Save bandwidth with the TMOD_TO feature") Signed-off-by: Phil Elwell <phil@raspberrypi.com>
The DW SPI interface has a 16-bit clock divider, where the bottom bit of the divisor must be 0. Limit how low the clock speed can go to prevent the clock divider from being truncated, as that could lead to a much higher clock rate than requested. Signed-off-by: Phil Elwell <phil@raspberrypi.com>
|
Now tested over a wide range of speeds, during the course of which I discovered that a speed of 3kHz or lower can lead to effectively random speeds due to clock divisor overflow - now fixed in the second commit. |
kernel: spi: dw: Fix non-DMA transmit-only transfers See: raspberrypi/linux#6286 kernel: ARM: dts: bcm2712: Fix invalid polling-delay-passive setting See: raspberrypi/linux#6290 kernel: dtoverlays: Add overlay for HD44780 via I2C PCF8574 backpack See: raspberrypi/linux#6293 kernel: DTS: bcm2712: enable SD slot CQE by default on Pi 5 See: raspberrypi/linux#6301
kernel: spi: dw: Fix non-DMA transmit-only transfers See: raspberrypi/linux#6286 kernel: ARM: dts: bcm2712: Fix invalid polling-delay-passive setting See: raspberrypi/linux#6290 kernel: dtoverlays: Add overlay for HD44780 via I2C PCF8574 backpack See: raspberrypi/linux#6293 kernel: DTS: bcm2712: enable SD slot CQE by default on Pi 5 See: raspberrypi/linux#6301
Ensure the transmit FIFO has emptied before ending the transfer by dropping the TX threshold to 0 when the last byte has been pushed into the FIFO. Include a similar fix for the non-IRQ paths.
See: #6285
Fixes: 6014649 ("spi: dw: Save bandwidth with the TMOD_TO feature")