Code size report:

Reference:  mpy-cross/mpconfigport: Explicitly disable native code loading. [8fb848f]
Comparison: rp2/machine_spi: Improve RX overrun handling. [merge of 95a4749]
  mpy-cross:    +0 +0.000% 
   bare-arm:    +0 +0.000% 
minimal x86:    +0 +0.000% 
   unix x64:    +0 +0.000% standard
      stm32:    +0 +0.000% PYBV10
     mimxrt:    +0 +0.000% TEENSY40
        rp2:   +80 +0.009% RPI_PICO_W
       samd:    +0 +0.000% ADAFRUIT_ITSYBITSY_M4_EXPRESS
  qemu rv32:    +0 +0.000% VIRT_RV32

Is this a bug or a known limitation? If the bus arbitration for PSRAM can fail irretrievably in the face of concurrent access by two DMA channels, the fault will eventually occur in user code. In this case disabling DMA in the SPI driver is a workround rather than a fix. If, rather than a bug, there is an inherent limit on DMA data rates it should be documented.

Force pushed 403b109, which only disables DMA transfers if PSRAM is enabled. @dpgeorge Please have a look. Haven't tested yet, don't have hardware with me right now.

Is this a bug or a known limitation?

The fact that DMA transfers to/from PSRAM can stall other DMA channels is a known limitation. Section 4.4.3 of the RP2350 datasheet states this pretty clearly in the second paragraph:

...QMI serial transfers force lengthy bus stalls on the DMA... stalling the DMA prevents any other active DMA channels from making progress during this time, slowing overall DMA throughput.

However, the current MicroPython SPI implementation results in a bug where the processor can get stuck in an infinite loop, because it does nothing to consider the above limitation. IMO, it should at least do something to guarantee that it can't get stuck in an infinite loop.

In this case disabling DMA in the SPI driver is a workround rather than a fix.

Yes, outright disabling DMA transfers is certainly a workaround. Perhaps an alternative fix could be something like the following?

         dma_start_channel_mask((1u << chan_rx) | (1u << chan_tx));
-        dma_channel_wait_for_finish_blocking(chan_rx);
         dma_channel_wait_for_finish_blocking(chan_tx);

+        // Wait for SPI to finish, and all data to be received by RX DMA
+        while (spi_is_busy(self->spi_inst) || spi_is_readable(self->spi_inst)) {
+        }
+
+        // If RX DMA still has transfers pending, raise an error
+        if (dma_channel_hw_addr(chan_rx)->transfer_count) {
+            mp_raise_OSError(MP_EIO);
+        }
    }

Since the cause is a limitation of the hardware, and the bug in the code is the result of the code not properly handling the implications of that hardware limitation, I don't think that killing all DMA support for SPI as a workaround is appropriate.

I would favour your later proposal of having the code raise an error if the RX DMA doesn't complete. That way as long as the user doesn't set up their code in a way that overwhelms the SPRAM bus they can still have the benefits of using DMA for their SPI transfers.

@sfe-SparkFro On a related note, I just submitted PR #18622 to add support for the DMA controller's pacing timers. At the moment if you uses the rp2.DMA() class for a memory-to-memory move it will always run at full speed but with this new code you could update your failing example in #18471 to something like this:

...
dma_psram = rp2.DMA()
dma_psram_timer = rp2.DMATimer()
# Limit the transfer rate to 20 million transfers per second
dma_psram_timer.freq = 20_000_000
dma_psram_ctrl = dma_psram.pack_ctrl(size = 0, treq_sel=dma_psram_timer.treq_sel)
...

This way the SRAM to PSRAM transfer will be rate limited and the user can arrange not to swamp the bus.

It would be instructive if you could test this on a board with PSRAM to confirm that it can be used to fix the problem (but I still think the SPI code should be updated to raise an error rather than stall if the RX DMA doesn't complete).

See 30fb190. Now this PR continues to use DMA transfers when possible, but can gracefully detect the SPI RX FIFO overrun condition and raise an OSError if that occurs.

I would like to see a solution that allows the user to force software transfers to guarantee this OSError never gets raised. Perhaps it can be added as an argument in the constructor? Eg. machine.SPI(dma=false). Can save for a different PR, let's just get this fix through 🙂

Couple improvements in 95a4749. Forgot to clear the overrun flag, and realized that an OSError doesn't need to be raised if write_only == true since RX data is being ignored anyways. This PR passes all my tests now!

Also, with my test code, I realized RX overruns occur much more frequently when transferring from PSRAM to SRAM, almost every loop iteration:

dma_psram.config(
    read = psram_buffer,
    write = sram_buffer,
    ...
)

Can also easily test SPI read versus write by changing spi.write(sram_buffer) to spi.readinto(sram_buffer), which highlights that the OSError does not get raised with writes (but I confirmed overruns were still happening by adding a mp_printf()).

It would be instructive if you could test this on a board with PSRAM to confirm that it can be used to fix the problem

Thanks! Just tested it out. Pacing the PSRAM DMA at 20 million transfers per second does not help (my test code typically freezes on the first loop iteration now that I'm transferring from PSRAM to SRAM). If I drop it to 2 million, then it does help, but does not fix the problem (first run froze after ~600 loop iterations).

Maybe it can be fixed by further reducing the transfer rate, but that would really start throttling the DMA, and I'm not even sure that pacing the PSRAM DMA transfer can be guaranteed to fix this. When the DMA encounters a bus stall during a PSRAM transfer, the amount of time its stalled is variable. I'm guessing it's possible for a single very long bus stall to cause an RX FIFO overrun, in which case, no amount of DMA pacing fixes the problem. Makes it a lot less likely, but not guaranteed. Or at least, I'm guessing that's the case, but even if not, you're sacrificing DMA throughput. Which is why I'd like an option to force software SPI transfers, because it's guaranteed to not have overruns while allowing the PSRAM DMA channel to transfer as fast as the XIP subsystem allows.