From 995b1d38970598fb5e646915224e53bfe0e2b8af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Thu, 12 Sep 2024 19:42:18 +0200 Subject: [PATCH 1/4] Fix DMA starving SPI --- esp-hal/src/spi/master.rs | 29 ++++++++++----------- hil-test/tests/spi_full_duplex_dma.rs | 36 ++++++++++++++++----------- 2 files changed, 37 insertions(+), 28 deletions(-) diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index 2d79dd1a484..85b46f83202 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -2288,14 +2288,8 @@ pub trait InstanceDma: Instance { 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()); @@ -2335,11 +2329,12 @@ pub trait InstanceDma: Instance { 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()?; + reset_dma_before_usr_cmd(reg_block); + reg_block.cmd().modify(|_, w| w.usr().set_bit()); Ok(()) @@ -2409,9 +2404,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(not(any(esp32, esp32s2)))] + _reg_block.dma_conf().modify(|_, w| { w.rx_afifo_rst() .set_bit() .buf_afifo_rst() @@ -2419,10 +2414,16 @@ fn reset_dma_before_usr_cmd(reg_block: &RegisterBlock) { .dma_afifo_rst() .set_bit() }); -} -#[cfg(any(esp32, esp32s2))] -fn reset_dma_before_usr_cmd(_reg_block: &RegisterBlock) {} + // Wait for a few clock cycles for the DMA to fill the SPI async FIFO, + // before starting the SPI + let cycles = 20; + + #[cfg(riscv)] + riscv::asm::delay(cycles); + #[cfg(xtensa)] + xtensa_lx::timer::delay(cycles); +} #[cfg(not(any(esp32, esp32s2)))] fn reset_dma_before_load_dma_dscr(_reg_block: &RegisterBlock) {} diff --git a/hil-test/tests/spi_full_duplex_dma.rs b/hil-test/tests/spi_full_duplex_dma.rs index 34c53bb50d5..d3495fce4da 100644 --- a/hil-test/tests/spi_full_duplex_dma.rs +++ b/hil-test/tests/spi_full_duplex_dma.rs @@ -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] From e850a2b4133026a82e3da3e1ef14d1d43d8942c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Thu, 12 Sep 2024 21:47:55 +0200 Subject: [PATCH 2/4] Simplify cfgs --- esp-hal/src/spi/master.rs | 58 +++++++++++++++++++-------------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index 85b46f83202..c9c1893ed1a 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -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")] @@ -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 { @@ -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) { self.spi.listen(interrupts); } /// Unlisten the given interrupts - #[cfg(not(any(esp32, esp32s2)))] + #[cfg(gdma)] pub fn unlisten(&mut self, interrupts: EnumSet) { self.spi.unlisten(interrupts); } /// Gets asserted interrupts - #[cfg(not(any(esp32, esp32s2)))] + #[cfg(gdma)] pub fn interrupts(&mut self) -> EnumSet { self.spi.interrupts() } /// Resets asserted interrupts - #[cfg(not(any(esp32, esp32s2)))] + #[cfg(gdma)] pub fn clear_interrupts(&mut self, interrupts: EnumSet) { self.spi.clear_interrupts(interrupts); } @@ -1516,7 +1516,7 @@ mod dma { } /// Listen for the given interrupts - #[cfg(not(any(esp32, esp32s2)))] + #[cfg(gdma)] pub fn listen(&mut self, interrupts: EnumSet) { let (mut spi_dma, rx_buf, tx_buf) = self.wait_for_idle(); spi_dma.listen(interrupts); @@ -1524,7 +1524,7 @@ mod dma { } /// Unlisten the given interrupts - #[cfg(not(any(esp32, esp32s2)))] + #[cfg(gdma)] pub fn unlisten(&mut self, interrupts: EnumSet) { let (mut spi_dma, rx_buf, tx_buf) = self.wait_for_idle(); spi_dma.unlisten(interrupts); @@ -1532,7 +1532,7 @@ mod dma { } /// Gets asserted interrupts - #[cfg(not(any(esp32, esp32s2)))] + #[cfg(gdma)] pub fn interrupts(&mut self) -> EnumSet { let (mut spi_dma, rx_buf, tx_buf) = self.wait_for_idle(); let interrupts = spi_dma.interrupts(); @@ -1542,7 +1542,7 @@ mod dma { } /// Resets asserted interrupts - #[cfg(not(any(esp32, esp32s2)))] + #[cfg(gdma)] pub fn clear_interrupts(&mut self, interrupts: EnumSet) { let (mut spi_dma, rx_buf, tx_buf) = self.wait_for_idle(); spi_dma.clear_interrupts(interrupts); @@ -2349,19 +2349,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| { @@ -2378,7 +2378,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| { @@ -2405,7 +2405,7 @@ pub trait InstanceDma: Instance { } fn reset_dma_before_usr_cmd(_reg_block: &RegisterBlock) { - #[cfg(not(any(esp32, esp32s2)))] + #[cfg(gdma)] _reg_block.dma_conf().modify(|_, w| { w.rx_afifo_rst() .set_bit() @@ -2425,10 +2425,10 @@ fn reset_dma_before_usr_cmd(_reg_block: &RegisterBlock) { xtensa_lx::timer::delay(cycles); } -#[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() @@ -2515,7 +2515,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() @@ -2534,7 +2534,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() @@ -2854,7 +2854,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) { let reg_block = self.register_block(); @@ -2870,7 +2870,7 @@ pub trait Instance: private::Sealed { } /// Unlisten the given interrupts - #[cfg(not(any(esp32, esp32s2)))] + #[cfg(gdma)] fn unlisten(&mut self, interrupts: EnumSet) { let reg_block = self.register_block(); @@ -2886,7 +2886,7 @@ pub trait Instance: private::Sealed { } /// Gets asserted interrupts - #[cfg(not(any(esp32, esp32s2)))] + #[cfg(gdma)] fn interrupts(&mut self) -> EnumSet { let mut res = EnumSet::new(); let reg_block = self.register_block(); @@ -2901,7 +2901,7 @@ pub trait Instance: private::Sealed { } /// Resets asserted interrupts - #[cfg(not(any(esp32, esp32s2)))] + #[cfg(gdma)] fn clear_interrupts(&mut self, interrupts: EnumSet) { let reg_block = self.register_block(); @@ -2968,7 +2968,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() @@ -2982,7 +2982,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() @@ -3223,7 +3223,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() @@ -3377,7 +3377,7 @@ pub trait Instance: private::Sealed { self.read_bytes_from_fifo(buffer) } - #[cfg(not(any(esp32, esp32s2)))] + #[cfg(gdma)] fn update(&self) { let reg_block = self.register_block(); @@ -3388,7 +3388,7 @@ pub trait Instance: private::Sealed { } } - #[cfg(any(esp32, esp32s2))] + #[cfg(pdma)] fn update(&self) { // not need/available on ESP32/ESP32S2 } From 8cf2b8cff99713b1ab667a36e76f9b9b1ce293af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Thu, 12 Sep 2024 21:58:05 +0200 Subject: [PATCH 3/4] Trigger an update before starting transaction --- esp-hal/src/spi/master.rs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index c9c1893ed1a..e31883fb982 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -2242,6 +2242,8 @@ pub trait InstanceDma: Instance { reset_dma_before_usr_cmd(reg_block); + self.update(); + reg_block.cmd().modify(|_, w| w.usr().set_bit()); Ok(()) @@ -2291,6 +2293,8 @@ pub trait InstanceDma: Instance { reset_dma_before_usr_cmd(reg_block); + self.update(); + reg_block.cmd().modify(|_, w| w.usr().set_bit()); Ok(()) @@ -2335,6 +2339,8 @@ pub trait InstanceDma: Instance { reset_dma_before_usr_cmd(reg_block); + self.update(); + reg_block.cmd().modify(|_, w| w.usr().set_bit()); Ok(()) @@ -2414,15 +2420,6 @@ fn reset_dma_before_usr_cmd(_reg_block: &RegisterBlock) { .dma_afifo_rst() .set_bit() }); - - // Wait for a few clock cycles for the DMA to fill the SPI async FIFO, - // before starting the SPI - let cycles = 20; - - #[cfg(riscv)] - riscv::asm::delay(cycles); - #[cfg(xtensa)] - xtensa_lx::timer::delay(cycles); } #[cfg(gdma)] From b100f993733722ac4b4ce1f7437b209de8f9331e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Thu, 12 Sep 2024 23:01:04 +0200 Subject: [PATCH 4/4] Do not update after enable_dma, use start_operation --- esp-hal/src/spi/master.rs | 33 +++++++++------------------------ 1 file changed, 9 insertions(+), 24 deletions(-) diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index e31883fb982..6d039b81ee7 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -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())?; @@ -2242,9 +2241,7 @@ pub trait InstanceDma: Instance { reset_dma_before_usr_cmd(reg_block); - self.update(); - - reg_block.cmd().modify(|_, w| w.usr().set_bit()); + self.start_operation(); Ok(()) } @@ -2283,19 +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); - self.update(); - - reg_block.cmd().modify(|_, w| w.usr().set_bit()); + self.start_operation(); Ok(()) } @@ -2328,20 +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(); - rx.prepare_transfer(self.dma_peripheral(), buffer)?; rx.start_transfer()?; reset_dma_before_usr_cmd(reg_block); - self.update(); - - reg_block.cmd().modify(|_, w| w.usr().set_bit()); + self.start_operation(); Ok(()) } @@ -3046,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(()) } @@ -3101,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. @@ -3368,8 +3354,7 @@ 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) }