Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix DMA starving SPI #2152

Merged
merged 4 commits into from
Sep 13, 2024
Merged

Fix DMA starving SPI #2152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
995b1d3
Fix DMA starving SPI
bugadani Sep 12, 2024
e850a2b
Simplify cfgs
bugadani Sep 12, 2024
8cf2b8c
Trigger an update before starting transaction
bugadani Sep 12, 2024
b100f99
Do not update after enable_dma, use start_operation
bugadani Sep 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 43 additions & 60 deletions esp-hal/src/spi/master.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,9 @@
use core::marker::PhantomData;

pub use dma::*;
#[cfg(not(any(esp32, esp32s2)))]
#[cfg(gdma)]
use enumset::EnumSet;
#[cfg(not(any(esp32, esp32s2)))]
#[cfg(gdma)]
use enumset::EnumSetType;
use fugit::HertzU32;
#[cfg(feature = "place-spi-driver-in-ram")]
Expand Down Expand Up @@ -91,7 +91,7 @@ use crate::{
};

/// Enumeration of possible SPI interrupt events.
#[cfg(not(any(esp32, esp32s2)))]
#[cfg(gdma)]
#[derive(EnumSetType)]
#[cfg_attr(feature = "defmt", derive(defmt::Format))]
pub enum SpiInterrupt {
Expand Down Expand Up @@ -999,25 +999,25 @@ mod dma {
}

/// Listen for the given interrupts
#[cfg(not(any(esp32, esp32s2)))]
#[cfg(gdma)]
pub fn listen(&mut self, interrupts: EnumSet<SpiInterrupt>) {
self.spi.listen(interrupts);
}

/// Unlisten the given interrupts
#[cfg(not(any(esp32, esp32s2)))]
#[cfg(gdma)]
pub fn unlisten(&mut self, interrupts: EnumSet<SpiInterrupt>) {
self.spi.unlisten(interrupts);
}

/// Gets asserted interrupts
#[cfg(not(any(esp32, esp32s2)))]
#[cfg(gdma)]
pub fn interrupts(&mut self) -> EnumSet<SpiInterrupt> {
self.spi.interrupts()
}

/// Resets asserted interrupts
#[cfg(not(any(esp32, esp32s2)))]
#[cfg(gdma)]
pub fn clear_interrupts(&mut self, interrupts: EnumSet<SpiInterrupt>) {
self.spi.clear_interrupts(interrupts);
}
Expand Down Expand Up @@ -1516,23 +1516,23 @@ mod dma {
}

/// Listen for the given interrupts
#[cfg(not(any(esp32, esp32s2)))]
#[cfg(gdma)]
pub fn listen(&mut self, interrupts: EnumSet<SpiInterrupt>) {
let (mut spi_dma, rx_buf, tx_buf) = self.wait_for_idle();
spi_dma.listen(interrupts);
self.state = State::Idle(spi_dma, rx_buf, tx_buf);
}

/// Unlisten the given interrupts
#[cfg(not(any(esp32, esp32s2)))]
#[cfg(gdma)]
pub fn unlisten(&mut self, interrupts: EnumSet<SpiInterrupt>) {
let (mut spi_dma, rx_buf, tx_buf) = self.wait_for_idle();
spi_dma.unlisten(interrupts);
self.state = State::Idle(spi_dma, rx_buf, tx_buf);
}

/// Gets asserted interrupts
#[cfg(not(any(esp32, esp32s2)))]
#[cfg(gdma)]
pub fn interrupts(&mut self) -> EnumSet<SpiInterrupt> {
let (mut spi_dma, rx_buf, tx_buf) = self.wait_for_idle();
let interrupts = spi_dma.interrupts();
Expand All @@ -1542,7 +1542,7 @@ mod dma {
}

/// Resets asserted interrupts
#[cfg(not(any(esp32, esp32s2)))]
#[cfg(gdma)]
pub fn clear_interrupts(&mut self, interrupts: EnumSet<SpiInterrupt>) {
let (mut spi_dma, rx_buf, tx_buf) = self.wait_for_idle();
spi_dma.clear_interrupts(interrupts);
Expand Down Expand Up @@ -2231,9 +2231,8 @@ pub trait InstanceDma: Instance {
.modify(|_, w| w.usr_miso().bit(true).usr_mosi().bit(true));

self.enable_dma();
self.update();

self.clear_dma_interrupts();

reset_dma_before_load_dma_dscr(reg_block);
rx.prepare_transfer(self.dma_peripheral(), rx_buffer)
.and_then(|_| rx.start_transfer())?;
Expand All @@ -2242,7 +2241,7 @@ pub trait InstanceDma: Instance {

reset_dma_before_usr_cmd(reg_block);

reg_block.cmd().modify(|_, w| w.usr().set_bit());
self.start_operation();

Ok(())
}
Expand Down Expand Up @@ -2281,23 +2280,16 @@ pub trait InstanceDma: Instance {
}

self.enable_dma();
self.update();
self.clear_dma_interrupts();

reset_dma_before_load_dma_dscr(reg_block);
self.clear_dma_interrupts();

tx.prepare_transfer(self.dma_peripheral(), buffer)?;
tx.start_transfer()?;
reset_dma_before_usr_cmd(reg_block);

// Wait for at least one clock cycle for the DMA to fill the SPI async FIFO,
// before starting the SPI
#[cfg(riscv)]
riscv::asm::delay(1);
#[cfg(xtensa)]
xtensa_lx::timer::delay(1);
reset_dma_before_usr_cmd(reg_block);

reg_block.cmd().modify(|_, w| w.usr().set_bit());
self.start_operation();

Ok(())
}
Expand Down Expand Up @@ -2330,17 +2322,16 @@ pub trait InstanceDma: Instance {
}

self.enable_dma();
self.update();
self.clear_dma_interrupts();

reset_dma_before_load_dma_dscr(reg_block);

self.clear_dma_interrupts();
reset_dma_before_usr_cmd(reg_block);

rx.prepare_transfer(self.dma_peripheral(), buffer)?;
rx.start_transfer()?;

reg_block.cmd().modify(|_, w| w.usr().set_bit());
reset_dma_before_usr_cmd(reg_block);

self.start_operation();

Ok(())
}
Expand All @@ -2354,19 +2345,19 @@ pub trait InstanceDma: Instance {
}
}

#[cfg(any(esp32c2, esp32c3, esp32c6, esp32h2, esp32s3))]
#[cfg(gdma)]
fn enable_dma(&self) {
let reg_block = self.register_block();
reg_block.dma_conf().modify(|_, w| w.dma_tx_ena().set_bit());
reg_block.dma_conf().modify(|_, w| w.dma_rx_ena().set_bit());
}

#[cfg(any(esp32, esp32s2))]
#[cfg(pdma)]
fn enable_dma(&self) {
// for non GDMA this is done in `assign_tx_device` / `assign_rx_device`
}

#[cfg(any(esp32c2, esp32c3, esp32c6, esp32h2, esp32s3))]
#[cfg(gdma)]
fn clear_dma_interrupts(&self) {
let reg_block = self.register_block();
reg_block.dma_int_clr().write(|w| {
Expand All @@ -2383,7 +2374,7 @@ pub trait InstanceDma: Instance {
});
}

#[cfg(any(esp32, esp32s2))]
#[cfg(pdma)]
fn clear_dma_interrupts(&self) {
let reg_block = self.register_block();
reg_block.dma_int_clr().write(|w| {
Expand All @@ -2409,9 +2400,9 @@ pub trait InstanceDma: Instance {
}
}

#[cfg(not(any(esp32, esp32s2)))]
fn reset_dma_before_usr_cmd(reg_block: &RegisterBlock) {
reg_block.dma_conf().modify(|_, w| {
fn reset_dma_before_usr_cmd(_reg_block: &RegisterBlock) {
#[cfg(gdma)]
_reg_block.dma_conf().modify(|_, w| {
w.rx_afifo_rst()
.set_bit()
.buf_afifo_rst()
Expand All @@ -2421,13 +2412,10 @@ fn reset_dma_before_usr_cmd(reg_block: &RegisterBlock) {
});
}

#[cfg(any(esp32, esp32s2))]
fn reset_dma_before_usr_cmd(_reg_block: &RegisterBlock) {}

#[cfg(not(any(esp32, esp32s2)))]
#[cfg(gdma)]
fn reset_dma_before_load_dma_dscr(_reg_block: &RegisterBlock) {}

#[cfg(any(esp32, esp32s2))]
#[cfg(pdma)]
fn reset_dma_before_load_dma_dscr(reg_block: &RegisterBlock) {
reg_block.dma_conf().modify(|_, w| {
w.out_rst()
Expand Down Expand Up @@ -2514,7 +2502,7 @@ pub trait Instance: private::Sealed {
.clear_bit()
});

#[cfg(not(any(esp32, esp32s2)))]
#[cfg(gdma)]
reg_block.clk_gate().modify(|_, w| {
w.clk_en()
.set_bit()
Expand All @@ -2533,7 +2521,7 @@ pub trait Instance: private::Sealed {
.modify(|_, w| w.spi2_clkm_sel().bits(1));
}

#[cfg(not(any(esp32, esp32s2)))]
#[cfg(gdma)]
reg_block.ctrl().modify(|_, w| {
w.q_pol()
.clear_bit()
Expand Down Expand Up @@ -2853,7 +2841,7 @@ pub trait Instance: private::Sealed {
fn set_interrupt_handler(&mut self, handler: InterruptHandler);

/// Listen for the given interrupts
#[cfg(not(any(esp32, esp32s2)))]
#[cfg(gdma)]
fn listen(&mut self, interrupts: EnumSet<SpiInterrupt>) {
let reg_block = self.register_block();

Expand All @@ -2869,7 +2857,7 @@ pub trait Instance: private::Sealed {
}

/// Unlisten the given interrupts
#[cfg(not(any(esp32, esp32s2)))]
#[cfg(gdma)]
fn unlisten(&mut self, interrupts: EnumSet<SpiInterrupt>) {
let reg_block = self.register_block();

Expand All @@ -2885,7 +2873,7 @@ pub trait Instance: private::Sealed {
}

/// Gets asserted interrupts
#[cfg(not(any(esp32, esp32s2)))]
#[cfg(gdma)]
fn interrupts(&mut self) -> EnumSet<SpiInterrupt> {
let mut res = EnumSet::new();
let reg_block = self.register_block();
Expand All @@ -2900,7 +2888,7 @@ pub trait Instance: private::Sealed {
}

/// Resets asserted interrupts
#[cfg(not(any(esp32, esp32s2)))]
#[cfg(gdma)]
fn clear_interrupts(&mut self, interrupts: EnumSet<SpiInterrupt>) {
let reg_block = self.register_block();

Expand Down Expand Up @@ -2967,7 +2955,7 @@ pub trait Instance: private::Sealed {

fn ch_bus_freq(&mut self, frequency: HertzU32) {
// Disable clock source
#[cfg(not(any(esp32, esp32s2)))]
#[cfg(gdma)]
self.register_block().clk_gate().modify(|_, w| {
w.clk_en()
.clear_bit()
Expand All @@ -2981,7 +2969,7 @@ pub trait Instance: private::Sealed {
self.setup(frequency);

// Enable clock source
#[cfg(not(any(esp32, esp32s2)))]
#[cfg(gdma)]
self.register_block().clk_gate().modify(|_, w| {
w.clk_en()
.set_bit()
Expand Down Expand Up @@ -3048,9 +3036,7 @@ pub trait Instance: private::Sealed {
let reg_block = self.register_block();
reg_block.w(0).write(|w| w.buf().set(word.into()));

self.update();

reg_block.cmd().modify(|_, w| w.usr().set_bit());
self.start_operation();

Ok(())
}
Expand Down Expand Up @@ -3103,9 +3089,7 @@ pub trait Instance: private::Sealed {
}
}

self.update();

self.register_block().cmd().modify(|_, w| w.usr().set_bit());
self.start_operation();

// Wait for all chunks to complete except the last one.
// The function is allowed to return before the bus is idle.
Expand Down Expand Up @@ -3222,7 +3206,7 @@ pub trait Instance: private::Sealed {
.bit(command_state)
});

#[cfg(not(any(esp32, esp32s2)))]
#[cfg(gdma)]
reg_block.clk_gate().modify(|_, w| {
w.clk_en()
.set_bit()
Expand Down Expand Up @@ -3370,13 +3354,12 @@ pub trait Instance: private::Sealed {
}

self.configure_datalen(buffer.len() as u32 * 8);
self.update();
reg_block.cmd().modify(|_, w| w.usr().set_bit());
self.start_operation();
self.flush()?;
self.read_bytes_from_fifo(buffer)
}

#[cfg(not(any(esp32, esp32s2)))]
#[cfg(gdma)]
fn update(&self) {
let reg_block = self.register_block();

Expand All @@ -3387,7 +3370,7 @@ pub trait Instance: private::Sealed {
}
}

#[cfg(any(esp32, esp32s2))]
#[cfg(pdma)]
fn update(&self) {
// not need/available on ESP32/ESP32S2
}
Expand Down
36 changes: 22 additions & 14 deletions hil-test/tests/spi_full_duplex_dma.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,32 +60,40 @@ mod tests {
}
}

let mosi_loopback = mosi.peripheral_input();
let spi = Spi::new(peripherals.SPI2, 100.kHz(), SpiMode::Mode0)
let miso = mosi.peripheral_input();
let spi = Spi::new(peripherals.SPI2, 10000.kHz(), SpiMode::Mode0)
.with_sck(sclk)
.with_mosi(mosi)
.with_miso(mosi_loopback)
.with_miso(miso)
.with_dma(dma_channel.configure(false, DmaPriority::Priority0));

Context { spi }
}

#[test]
#[timeout(3)]
fn test_symmetric_dma_transfer(ctx: Context) {
let (rx_buffer, rx_descriptors, tx_buffer, tx_descriptors) = dma_buffers!(4);
let dma_rx_buf = DmaRxBuf::new(rx_descriptors, rx_buffer).unwrap();
// This test case sends a large amount of data, twice, to verify that
// https://github.com/esp-rs/esp-hal/issues/2151 is and remains fixed.
let (rx_buffer, rx_descriptors, tx_buffer, tx_descriptors) = dma_buffers!(32000);
let mut dma_rx_buf = DmaRxBuf::new(rx_descriptors, rx_buffer).unwrap();
let mut dma_tx_buf = DmaTxBuf::new(tx_descriptors, tx_buffer).unwrap();

dma_tx_buf.fill(&[0xde, 0xad, 0xbe, 0xef]);
for (i, v) in dma_tx_buf.as_mut_slice().iter_mut().enumerate() {
*v = (i % 255) as u8;
}

let transfer = ctx
.spi
.dma_transfer(dma_rx_buf, dma_tx_buf)
.map_err(|e| e.0)
.unwrap();
let (_, (dma_rx_buf, dma_tx_buf)) = transfer.wait();
assert_eq!(dma_tx_buf.as_slice(), dma_rx_buf.as_slice());
let mut spi = ctx.spi;
for i in 0..4 {
dma_tx_buf.as_mut_slice()[0] = i as u8;
*dma_tx_buf.as_mut_slice().last_mut().unwrap() = i as u8;
let transfer = spi
.dma_transfer(dma_rx_buf, dma_tx_buf)
.map_err(|e| e.0)
.unwrap();

(spi, (dma_rx_buf, dma_tx_buf)) = transfer.wait();
assert_eq!(dma_tx_buf.as_slice(), dma_rx_buf.as_slice());
}
}

#[test]
Expand Down
Loading