Skip to content

fix(spi): arm the SPI slave peripheral after its DMA channels start - #6291

Merged
bugadani merged 5 commits into
esp-rs:mainfrom
molqzone:fix/spi-slave-dma-arm-order
Sep 17, 2026
Merged

bugadani merged 5 commits into
esp-rs:mainfrom
molqzone:fix/spi-slave-dma-arm-order

Conversation

@molqzone

@molqzone molqzone commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo xtask fmt command to ensure that all changed code is formatted correctly.
  • I have added changelog entries and/or migration guide notes in the sections below, or I will ask a maintainer to add the skip-changelog or manual-changelog label as appropriate.
  • My changes are in accordance to the esp-rs developer guidelines

Extra:

Pull Request Details 📖

Description

A GDMA-driven SPI slave has no control over when the master starts clocking:
SCLK and CS are owned by the master, and the first edge can arrive while the
driver is still setting up a transfer.

start_transfer_dma armed the peripheral (cmd().usr().set_bit()) before
starting the RX/TX DMA channels. Any bits the master clocks in that window are
shifted without DMA service, so the first bytes of the transfer are lost or
corrupted.

This is easy to hit in practice but invisible to the current HIL test: in
test_basic_dma the bitbang master only starts clocking after the slave
transfer has been armed, so the race window never opens. A real deployment
where the master owns the clock (in my case an ESP32-C5 receiving a continuous
video stream) hits it on every transfer.

The fix reorders the setup in start_transfer_dma:

  • the RX/TX FIFOs are reset immediately before starting the corresponding DMA
    channel,
  • USR is set only after both GDMA directions can service the first edge.

The CPU buffer AFIFO (buf_afifo_rst) was only reset as part of the old
combined reset; it is not used by DMA slave transfers and is no longer
touched. The scope is unchanged: this only affects the AHB_GDMA/AXI_GDMA
engines (spi_slave_dma_engine != "SPI_DMA"); the ESP32/ESP32-S2 slave path
is untouched.

ESP-IDF reference

ESP-IDF's GDMA SPI slave driver follows exactly the ordering this PR
restores, which is where this patch takes its cue from.

Per-direction FIFO reset, immediately before the corresponding channel
starts (components/esp_driver_spi/src/gpspi/spi_slave.c,
s_spi_slave_dma_prepare_data):

if (hal->rx_buffer) {
    spicommon_dma_desc_setup_link(...);
    spi_dma_reset(dma_ctx->rx_dma_chan);
    spi_slave_hal_hw_prepare_rx(hal->hw);      // spi_ll_dma_rx_fifo_reset
    spi_dma_start(dma_ctx->rx_dma_chan, ...);
}
if (hal->tx_buffer) {
    spicommon_dma_desc_setup_link(...);
    spi_dma_reset(dma_ctx->tx_dma_chan);
    spi_slave_hal_hw_prepare_tx(hal->hw);      // spi_ll_dma_tx_fifo_reset
    spi_dma_start(dma_ctx->tx_dma_chan, ...);
}

The LL resets are per-direction toggles (write 1, then 0) and never touch
buf_afifo_rst — that bit belongs to the CPU-buffer FIFO path
(spi_ll_cpu_tx_fifo_reset, used by the non-DMA driver)
(components/esp_hal_gpspi/esp32c5/include/hal/spi_ll.h):

static inline void spi_ll_dma_rx_fifo_reset(spi_dev_t *hw)
{
    hw->dma_conf.rx_afifo_rst = 1;
    hw->dma_conf.rx_afifo_rst = 0;
}

static inline void spi_ll_dma_tx_fifo_reset(spi_dev_t *hw)
{
    hw->dma_conf.dma_afifo_rst = 1;
    hw->dma_conf.dma_afifo_rst = 0;
}

And the transfer is kicked off only after both DMA directions have been
started, with a comment describing the very race this PR fixes
(s_spi_slave_isr, same file):

spi_slave_hal_hw_reset(hal);
s_spi_slave_prepare_data(host);   // resets AFIFOs, starts RX and TX DMA

//The slave rx dma get disturbed by unexpected transaction. Only connect the CS and start DMA when slave is ready.
if (use_dma) {
    restore_cs(host);
}
//Kick off transfer
spi_slave_hal_user_start(hal);    // spi_ll_user_start: hw->cmd.usr = 1

So in ESP-IDF terms, this PR replaces esp-hal's "set USR, then start DMA"
with IDF's "start DMA, then set USR", and replaces the combined
set-and-leave reset with IDF's per-direction toggling resets.

Happy to follow up with a HIL regression case (a master that does not wait for
the slave before clocking) if maintainers think that is worth the CI hardware
time.

Testing

  • Validated on real hardware: ESP32-C5 SPI slave (GDMA) receiving a continuous
    byte stream from a master that owns SCLK/CS and does not wait for a
    slave-ready signal. Before this change the first bytes of every transfer
    were lost; after the change the stream is intact.
  • cargo check passes for the esp32c5 and esp32c6 targets.
  • Could not run cargo xtask fmt locally (nightly toolchain unavailable
    here); the change was formatted to match the surrounding code. Happy to
    reformat if CI flags anything.

Changelog

esp-hal/SPI slave

  • Fixed: GDMA SPI slave transfers no longer lose the first bytes when the
    master starts clocking before the DMA channels are started.

@bugadani bugadani added the esp-hal-backport Backport this PR to the latest esp-hal-x.y.x branch.” label Sep 17, 2026
@bugadani
bugadani enabled auto-merge September 17, 2026 07:01
@bugadani
bugadani added this pull request to the merge queue Sep 17, 2026
Merged via the queue into esp-rs:main with commit 246ff81 Sep 17, 2026
38 of 49 checks passed
@github-actions

Copy link
Copy Markdown

Successfully created backport PR for esp-hal-1.2.x:

MabezDev pushed a commit that referenced this pull request Sep 17, 2026
…6291) (#6350)

* fix(spi): arm the SPI slave peripheral after its DMA channels start

* style: format SPI slave imports

(cherry picked from commit 246ff81)

Co-authored-by: molqzone <toiperus@sohu.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

esp-hal-backport Backport this PR to the latest esp-hal-x.y.x branch.”

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants