cyw43: dma_channel_abort interacting weirdly with chained DMA

[Gadgetoid]

I've been chasing an issue with wireless in our MicroPython builds, and after confirming that it's related to our DMA/PIO drivers for LED matrix displays came up with a reproduction: micropython/micropython#16627

This reproduction reliably shows that setting up chained DMA (yes it's in MicroPython but my original code was a C module) upsets the cyw43 driver. This only happens on Pico 2 W/RP2350 (possibly related to DMA errata E5?) and the same (original C) code has worked on RP2040 since the original Pico W launched.

@MichaelBell rolled with this and found that adding asm volatile (" nop\n nop\n nop " ); after calls to dma_channel_abort in cyw43_bus_pio_spi.c would somehow fix this, coming up with the following patch:

diff --git a/src/rp2_common/pico_cyw43_driver/cyw43_bus_pio_spi.c b/src/rp2_common/pico_cyw43_driver/cyw43_bus_pio_spi.c
index bcc7284..1c6bd84 100644
--- a/src/rp2_common/pico_cyw43_driver/cyw43_bus_pio_spi.c
+++ b/src/rp2_common/pico_cyw43_driver/cyw43_bus_pio_spi.c
@@ -260,6 +260,7 @@ int cyw43_spi_transfer(cyw43_int_t *self, const uint8_t *tx, size_t tx_length, u
         pio_sm_exec(bus_data->pio, bus_data->pio_sm, pio_encode_jmp(bus_data->pio_offset));
         dma_channel_abort(bus_data->dma_out);
         dma_channel_abort(bus_data->dma_in);
+        asm volatile (" nop\n nop\n nop " );
 
         dma_channel_config out_config = dma_channel_get_default_config(bus_data->dma_out);
         channel_config_set_bswap(&out_config, true);
@@ -301,6 +302,7 @@ int cyw43_spi_transfer(cyw43_int_t *self, const uint8_t *tx, size_t tx_length, u
         pio_sm_exec(bus_data->pio, bus_data->pio_sm, pio_encode_out(pio_y, 32));
         pio_sm_exec(bus_data->pio, bus_data->pio_sm, pio_encode_jmp(bus_data->pio_offset));
         dma_channel_abort(bus_data->dma_out);
+        asm volatile (" nop\n nop\n nop " );
 
         dma_channel_config out_config = dma_channel_get_default_config(bus_data->dma_out);
         channel_config_set_bswap(&out_config, true);

We don't have a good sense for exactly what is happening, and it seems to vary between compilers (breaks as expected with GCC 14.2.1), but it's clear that it's a uniquely RP2350 problem and that a mitigation should be possible in Pico SDK.

[Gadgetoid]

Note that I believe clearing/setting the enable bits around the dma_channel_abort calls - as per E5 - did not work.

[MichaelBell]

Yep, I did try that and it didn't help.

What I haven't tried is polling the abort register for the abort completion, which is a new thing on RP2350, and feels like it might be the correct thing to do? Although potentially even if that worked it might just be because the check took enough cycles, which is presumably all the nops are doing. Note the three nops was just my first guess, not anything scientifically determined.

But I think this needs looking at by someone that understands what is going on under the hood, rather than speculative fixes!

[Gadgetoid]

Quick and dirty C repro FAO folks who understand what is going on under the hood!

Notably, and perhaps as a surprise to nobody, the nop workaround does not work here.

#include "pico/stdlib.h"
#include "pico/cyw43_arch.h"
#include "hardware/dma.h"

#define DATA_LENGTH 1024

uint32_t dma_src[DATA_LENGTH / 4] = {0};
const uint32_t dma_src_addr = (uint32_t)dma_src;
uint32_t dma_dst;

int main() {
    stdio_init_all();
    while (!stdio_usb_connected());
    printf("Hello World\n");

    int dma_channel = dma_claim_unused_channel(true);
    int dma_ctrl_channel = dma_claim_unused_channel(true);

    dma_channel_config ctrl_config = dma_channel_get_default_config(dma_ctrl_channel);
    channel_config_set_transfer_data_size(&ctrl_config, DMA_SIZE_32);
    channel_config_set_read_increment(&ctrl_config, false);
    channel_config_set_write_increment(&ctrl_config, false);
    channel_config_set_chain_to(&ctrl_config, dma_channel);

    dma_channel_configure(
      dma_ctrl_channel,
      &ctrl_config,
      &dma_hw->ch[dma_channel].read_addr,
      &dma_src_addr,
      1,
      false
    );

    dma_channel_config config = dma_channel_get_default_config(dma_channel);
    channel_config_set_transfer_data_size(&config, DMA_SIZE_32);
    channel_config_set_bswap(&config, false); // byte swap to reverse little endian
    channel_config_set_chain_to(&config, dma_ctrl_channel); 

    dma_channel_configure(
      dma_channel,
      &config,
      &dma_dst,
      NULL,
      DATA_LENGTH / 4,
      false);

    // Comment this out and WiFi will work
    dma_start_channel_mask(1u << dma_ctrl_channel);

    if (cyw43_arch_init()) {
        printf("Wi-Fi init failed");
        return -1;
    }
    while (true) {
        cyw43_arch_gpio_put(CYW43_WL_GPIO_LED_PIN, 1);
        sleep_ms(250);
        cyw43_arch_gpio_put(CYW43_WL_GPIO_LED_PIN, 0);
        sleep_ms(250);
    }
}

Result of running this on a Pico 2 W for me is:

Running it on a Pico W is fine, LED blinks as expected.

Reminder that YMMV depending on wind direction (though the wind seems to have died down finally) or your compiler version.

Edit: Totally not quietly dropping sleep_ms... thanks @MichaelBell

[lurch]

Original issue title mentions dma_channel_abort, but that doesn't appear to be used in the "dirty C repro" 😉
I wonder if this might be a similar thing to #2123 ? Perhaps it's worth having a look at the bus performance counters?

[MichaelBell]

The aborts in question are in https://github.com/raspberrypi/pico-sdk/blob/master/src/rp2_common/pico_cyw43_driver/cyw43_bus_pio_spi.c#L261-L262

[Gadgetoid]

but that doesn't appear to be used in the "dirty C repro"

It was implicating the use of dma_channel_abort in the cyw43 SPI driver, so it's there... just buried layers deep.

Nonetheless the linked issue looks very similar, though we are not using multiple CPUs nor messing with priority. At worst it's another thing to cross off the list- though I'm in over my head already 😆

[peterharperuk]

I can reproduce this with your code in a release build (slightly curious).
Firmware verification fails which either means we're writing junk or reading back junk.

[MichaelBell]

Note that like #2152 it seems sensitive to compiler version (I couldn't repro the problem when I built Micropython with gcc 10.3.1).

[peterharperuk]

Can you see if this improves things? peterharperuk@2a810a9

[MichaelBell]

That looks to fix the Micropython repro case.