diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index b46897c2c6..d05b1ee533 100644 --- a/esp-hal/CHANGELOG.md +++ b/esp-hal/CHANGELOG.md @@ -59,6 +59,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - Fix `SpiBus::transfer` transferring data twice in some cases (#2159) - Fixed UART freezing when using `RcFast` clock source on ESP32-C2/C3 (#2170) - I2S: on ESP32 and ESP32-S2 data is now output to the right (WS=1) channel first. (#2194) +- SPI: Fixed an issue where unexpected data was written outside of the read buffer (#2179) +- SPI: Fixed an issue where `wait` has returned before the DMA has finished writing the memory (#2179) +- SPI: Fixed an issue where repeated calls to `dma_transfer` may end up looping indefinitely (#2179) +- SPI: Fixed an issue that prevented correctly reading the first byte in a transaction (#2179) ### Removed diff --git a/esp-hal/src/dma/mod.rs b/esp-hal/src/dma/mod.rs index d6c693605b..bd1dc212ed 100644 --- a/esp-hal/src/dma/mod.rs +++ b/esp-hal/src/dma/mod.rs @@ -205,6 +205,7 @@ where bitfield::bitfield! { #[doc(hidden)] #[derive(Clone, Copy)] + #[cfg_attr(feature = "defmt", derive(defmt::Format))] pub struct DmaDescriptorFlags(u32); u16; @@ -227,6 +228,7 @@ impl Debug for DmaDescriptorFlags { /// A DMA transfer descriptor. #[derive(Clone, Copy, Debug)] +#[cfg_attr(feature = "defmt", derive(defmt::Format))] pub struct DmaDescriptor { pub(crate) flags: DmaDescriptorFlags, pub(crate) buffer: *mut u8, @@ -397,6 +399,29 @@ macro_rules! dma_circular_descriptors { }; } +/// Declares a DMA buffer with a specific size, aligned to 4 bytes +#[doc(hidden)] +#[macro_export] +macro_rules! declare_aligned_dma_buffer { + ($name:ident, $size:expr) => { + // ESP32 requires word alignment for DMA buffers. + // ESP32-S2 technically supports byte-aligned DMA buffers, but the + // transfer ends up writing out of bounds. + // if the buffer's length is 2 or 3 (mod 4). + static mut $name: [u32; ($size + 3) / 4] = [0; ($size + 3) / 4]; + }; +} + +/// Turns the potentially oversized static `u32`` array reference into a +/// correctly sized `u8` one +#[doc(hidden)] +#[macro_export] +macro_rules! as_mut_byte_array { + ($name:ident, $size:expr) => { + unsafe { &mut *($name.as_mut_ptr() as *mut [u8; $size]) } + }; +} + /// Convenience macro to create DMA buffers and descriptors with specific chunk /// size. /// @@ -414,15 +439,15 @@ macro_rules! dma_circular_descriptors { #[macro_export] macro_rules! dma_buffers_chunk_size { ($rx_size:expr, $tx_size:expr, $chunk_size:expr) => {{ - static mut RX_BUFFER: [u8; $rx_size] = [0u8; $rx_size]; - static mut TX_BUFFER: [u8; $tx_size] = [0u8; $tx_size]; + $crate::declare_aligned_dma_buffer!(RX_BUFFER, $rx_size); + $crate::declare_aligned_dma_buffer!(TX_BUFFER, $tx_size); let (mut rx_descriptors, mut tx_descriptors) = $crate::dma_descriptors_chunk_size!($rx_size, $tx_size, $chunk_size); unsafe { ( - &mut RX_BUFFER, + $crate::as_mut_byte_array!(RX_BUFFER, $rx_size), rx_descriptors, - &mut TX_BUFFER, + $crate::as_mut_byte_array!(TX_BUFFER, $tx_size), tx_descriptors, ) } @@ -450,15 +475,15 @@ macro_rules! dma_buffers_chunk_size { #[macro_export] macro_rules! dma_circular_buffers_chunk_size { ($rx_size:expr, $tx_size:expr, $chunk_size:expr) => {{ - static mut RX_BUFFER: [u8; $rx_size] = [0u8; $rx_size]; - static mut TX_BUFFER: [u8; $tx_size] = [0u8; $tx_size]; + $crate::declare_aligned_dma_buffer!(RX_BUFFER, $rx_size); + $crate::declare_aligned_dma_buffer!(TX_BUFFER, $tx_size); let (mut rx_descriptors, mut tx_descriptors) = $crate::dma_circular_descriptors_chunk_size!($rx_size, $tx_size, $chunk_size); unsafe { ( - &mut RX_BUFFER, + $crate::as_mut_byte_array!(RX_BUFFER, $rx_size), rx_descriptors, - &mut TX_BUFFER, + $crate::as_mut_byte_array!(TX_BUFFER, $tx_size), tx_descriptors, ) } diff --git a/esp-hal/src/soc/esp32/gpio.rs b/esp-hal/src/soc/esp32/gpio.rs index bc5474690d..47dea4116d 100644 --- a/esp-hal/src/soc/esp32/gpio.rs +++ b/esp-hal/src/soc/esp32/gpio.rs @@ -103,7 +103,7 @@ pub(crate) fn get_io_mux_reg(gpio_num: u8) -> &'static io_mux::GPIO0 { 37 => transmute::<&'static io_mux::GPIO37, &'static io_mux::GPIO0>(iomux.gpio37()), 38 => transmute::<&'static io_mux::GPIO38, &'static io_mux::GPIO0>(iomux.gpio38()), 39 => transmute::<&'static io_mux::GPIO39, &'static io_mux::GPIO0>(iomux.gpio39()), - _ => panic!(), + other => panic!("GPIO {} does not exist", other), } } } diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index f646d4101d..eef4304677 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -109,9 +109,8 @@ const FIFO_SIZE: usize = 64; const FIFO_SIZE: usize = 72; /// Padding byte for empty write transfers -const EMPTY_WRITE_PAD: u8 = 0x00u8; +const EMPTY_WRITE_PAD: u8 = 0x00; -#[allow(unused)] const MAX_DMA_SIZE: usize = 32736; /// SPI commands, each consisting of a 16-bit command value and a data mode. @@ -490,7 +489,7 @@ impl<'d, T> Spi<'d, T, FullDuplexMode> where T: Instance, { - /// Read bytes from SPI. + /// Read a byte from SPI. /// /// Sends out a stuffing byte for every byte to read. This function doesn't /// perform flushing. If you want to read the response to something you @@ -1154,8 +1153,10 @@ mod dma { /// /// This method blocks until the transfer is finished and returns the /// `SpiDma` instance and the associated buffer. - pub fn wait(mut self) -> (SpiDma<'d, T, C, D, M>, Buf) { - self.spi_dma.spi.flush().ok(); + pub fn wait(self) -> (SpiDma<'d, T, C, D, M>, Buf) { + while !self.is_done() { + // Wait for the transfer to complete + } fence(Ordering::Acquire); (self.spi_dma, self.dma_buf) } @@ -2221,10 +2222,7 @@ pub trait InstanceDma: Instance { reg_block.dma_in_link().write(|w| w.bits(0)); } - self.configure_datalen(usize::max(rx_buffer.length(), tx_buffer.length()) as u32 * 8); - - rx.is_done(); - tx.is_done(); + self.configure_datalen(rx_buffer.length(), tx_buffer.length()); // re-enable the MISO and MOSI reg_block @@ -2232,15 +2230,14 @@ pub trait InstanceDma: Instance { .modify(|_, w| w.usr_miso().bit(true).usr_mosi().bit(true)); self.enable_dma(); - 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())?; tx.prepare_transfer(self.dma_peripheral(), tx_buffer) .and_then(|_| tx.start_transfer())?; - reset_dma_before_usr_cmd(reg_block); + #[cfg(gdma)] + self.reset_dma(); self.start_operation(); @@ -2255,9 +2252,7 @@ pub trait InstanceDma: Instance { full_duplex: bool, ) -> Result<(), Error> { let reg_block = self.register_block(); - self.configure_datalen(buffer.length() as u32 * 8); - - tx.is_done(); + self.configure_datalen(0, buffer.length()); // disable MISO and re-enable MOSI (DON'T do it for half-duplex) if full_duplex { @@ -2281,14 +2276,12 @@ pub trait InstanceDma: Instance { } self.enable_dma(); - self.clear_dma_interrupts(); - - reset_dma_before_load_dma_dscr(reg_block); - tx.prepare_transfer(self.dma_peripheral(), buffer)?; - tx.start_transfer()?; + tx.prepare_transfer(self.dma_peripheral(), buffer) + .and_then(|_| tx.start_transfer())?; - reset_dma_before_usr_cmd(reg_block); + #[cfg(gdma)] + self.reset_dma(); self.start_operation(); @@ -2311,9 +2304,7 @@ pub trait InstanceDma: Instance { reg_block.dma_in_link().write(|w| w.bits(0)); } - self.configure_datalen(buffer.length() as u32 * 8); - - rx.is_done(); + self.configure_datalen(buffer.length(), 0); // re-enable MISO and disable MOSI (DON'T do it for half-duplex) if full_duplex { @@ -2323,14 +2314,12 @@ pub trait InstanceDma: Instance { } self.enable_dma(); - self.clear_dma_interrupts(); - reset_dma_before_load_dma_dscr(reg_block); - - rx.prepare_transfer(self.dma_peripheral(), buffer)?; - rx.start_transfer()?; + rx.prepare_transfer(self.dma_peripheral(), buffer) + .and_then(|_| rx.start_transfer())?; - reset_dma_before_usr_cmd(reg_block); + #[cfg(gdma)] + self.reset_dma(); self.start_operation(); @@ -2346,32 +2335,63 @@ pub trait InstanceDma: Instance { } } - #[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(gdma)] + { + // for non GDMA this is done in `assign_tx_device` / `assign_rx_device` + let reg_block = self.register_block(); + reg_block.dma_conf().modify(|_, w| { + w.dma_tx_ena().set_bit(); + w.dma_rx_ena().set_bit() + }); + } + #[cfg(pdma)] + self.reset_dma(); } - #[cfg(pdma)] - fn enable_dma(&self) { - // for non GDMA this is done in `assign_tx_device` / `assign_rx_device` + fn reset_dma(&self) { + #[cfg(pdma)] + { + let reg_block = self.register_block(); + reg_block.dma_conf().modify(|_, w| { + w.out_rst().set_bit(); + w.in_rst().set_bit(); + w.ahbm_fifo_rst().set_bit(); + w.ahbm_rst().set_bit() + }); + reg_block.dma_conf().modify(|_, w| { + w.out_rst().clear_bit(); + w.in_rst().clear_bit(); + w.ahbm_fifo_rst().clear_bit(); + w.ahbm_rst().clear_bit() + }); + } + #[cfg(gdma)] + { + let reg_block = self.register_block(); + reg_block.dma_conf().modify(|_, w| { + w.rx_afifo_rst().set_bit(); + w.buf_afifo_rst().set_bit(); + w.dma_afifo_rst().set_bit() + }); + reg_block.dma_conf().modify(|_, w| { + w.rx_afifo_rst().clear_bit(); + w.buf_afifo_rst().clear_bit(); + w.dma_afifo_rst().clear_bit() + }); + } + self.clear_dma_interrupts(); } #[cfg(gdma)] fn clear_dma_interrupts(&self) { let reg_block = self.register_block(); reg_block.dma_int_clr().write(|w| { - w.dma_infifo_full_err() - .clear_bit_by_one() - .dma_outfifo_empty_err() - .clear_bit_by_one() - .trans_done() - .clear_bit_by_one() - .mst_rx_afifo_wfull_err() - .clear_bit_by_one() - .mst_tx_afifo_rempty_err() - .clear_bit_by_one() + w.dma_infifo_full_err().clear_bit_by_one(); + w.dma_outfifo_empty_err().clear_bit_by_one(); + w.trans_done().clear_bit_by_one(); + w.mst_rx_afifo_wfull_err().clear_bit_by_one(); + w.mst_tx_afifo_rempty_err().clear_bit_by_one() }); } @@ -2379,68 +2399,19 @@ pub trait InstanceDma: Instance { fn clear_dma_interrupts(&self) { let reg_block = self.register_block(); reg_block.dma_int_clr().write(|w| { - w.inlink_dscr_empty() - .clear_bit_by_one() - .outlink_dscr_error() - .clear_bit_by_one() - .inlink_dscr_error() - .clear_bit_by_one() - .in_done() - .clear_bit_by_one() - .in_err_eof() - .clear_bit_by_one() - .in_suc_eof() - .clear_bit_by_one() - .out_done() - .clear_bit_by_one() - .out_eof() - .clear_bit_by_one() - .out_total_eof() - .clear_bit_by_one() + w.inlink_dscr_empty().clear_bit_by_one(); + w.outlink_dscr_error().clear_bit_by_one(); + w.inlink_dscr_error().clear_bit_by_one(); + w.in_done().clear_bit_by_one(); + w.in_err_eof().clear_bit_by_one(); + w.in_suc_eof().clear_bit_by_one(); + w.out_done().clear_bit_by_one(); + w.out_eof().clear_bit_by_one(); + w.out_total_eof().clear_bit_by_one() }); } } -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() - .set_bit() - .dma_afifo_rst() - .set_bit() - }); -} - -#[cfg(gdma)] -fn reset_dma_before_load_dma_dscr(_reg_block: &RegisterBlock) {} - -#[cfg(pdma)] -fn reset_dma_before_load_dma_dscr(reg_block: &RegisterBlock) { - reg_block.dma_conf().modify(|_, w| { - w.out_rst() - .set_bit() - .in_rst() - .set_bit() - .ahbm_fifo_rst() - .set_bit() - .ahbm_rst() - .set_bit() - }); - - reg_block.dma_conf().modify(|_, w| { - w.out_rst() - .clear_bit() - .in_rst() - .clear_bit() - .ahbm_fifo_rst() - .clear_bit() - .ahbm_rst() - .clear_bit() - }); -} - impl InstanceDma for crate::peripherals::SPI2 {} #[cfg(spi3)] @@ -3032,7 +3003,7 @@ pub trait Instance: private::Sealed { return Err(nb::Error::WouldBlock); } - self.configure_datalen(8); + self.configure_datalen(0, 1); let reg_block = self.register_block(); reg_block.w(0).write(|w| w.buf().set(word.into())); @@ -3061,7 +3032,7 @@ pub trait Instance: private::Sealed { // The fifo has a limited fixed size, so the data must be chunked and then // transmitted for (i, chunk) in words.chunks(FIFO_SIZE).enumerate() { - self.configure_datalen(chunk.len() as u32 * 8); + self.configure_datalen(0, chunk.len()); { // TODO: replace with `array_chunks` and `from_le_bytes` @@ -3130,7 +3101,7 @@ pub trait Instance: private::Sealed { let reg_block = self.register_block(); for chunk in words.chunks_mut(FIFO_SIZE) { - self.configure_datalen(chunk.len() as u32 * 8); + self.configure_datalen(chunk.len(), 0); for (index, w_reg) in (0..chunk.len()).step_by(4).zip(reg_block.w_iter()) { let reg_val = w_reg.read().bits(); @@ -3354,46 +3325,63 @@ pub trait Instance: private::Sealed { .modify(|_, w| unsafe { w.usr_dummy_cyclelen().bits(dummy - 1) }); } - self.configure_datalen(buffer.len() as u32 * 8); + self.configure_datalen(buffer.len(), 0); self.start_operation(); self.flush()?; self.read_bytes_from_fifo(buffer) } - #[cfg(gdma)] fn update(&self) { - let reg_block = self.register_block(); + cfg_if::cfg_if! { + if #[cfg(gdma)] { + let reg_block = self.register_block(); - reg_block.cmd().modify(|_, w| w.update().set_bit()); + reg_block.cmd().modify(|_, w| w.update().set_bit()); - while reg_block.cmd().read().update().bit_is_set() { - // wait + while reg_block.cmd().read().update().bit_is_set() { + // wait + } + } else if #[cfg(esp32)] { + xtensa_lx::timer::delay(1); + } else { + // Doesn't seem to be needed for ESP32-S2 + } } } - #[cfg(pdma)] - fn update(&self) { - // not need/available on ESP32/ESP32S2 - } - - fn configure_datalen(&self, len: u32) { + fn configure_datalen(&self, rx_len_bytes: usize, tx_len_bytes: usize) { let reg_block = self.register_block(); - let len = if len > 0 { len - 1 } else { 0 }; - #[cfg(any(esp32c2, esp32c3, esp32c6, esp32h2, esp32s3))] - reg_block - .ms_dlen() - .write(|w| unsafe { w.ms_data_bitlen().bits(len) }); + let rx_len = rx_len_bytes as u32 * 8; + let tx_len = tx_len_bytes as u32 * 8; - #[cfg(not(any(esp32c2, esp32c3, esp32c6, esp32h2, esp32s3)))] - { - reg_block - .mosi_dlen() - .write(|w| unsafe { w.usr_mosi_dbitlen().bits(len) }); + let rx_len = rx_len.saturating_sub(1); + let tx_len = tx_len.saturating_sub(1); + + cfg_if::cfg_if! { + if #[cfg(esp32)] { + let len = rx_len.max(tx_len); + reg_block + .mosi_dlen() + .write(|w| unsafe { w.usr_mosi_dbitlen().bits(len) }); + + reg_block + .miso_dlen() + .write(|w| unsafe { w.usr_miso_dbitlen().bits(len) }); + } else if #[cfg(esp32s2)] { + reg_block + .mosi_dlen() + .write(|w| unsafe { w.usr_mosi_dbitlen().bits(tx_len) }); + + reg_block + .miso_dlen() + .write(|w| unsafe { w.usr_miso_dbitlen().bits(rx_len) }); + } else { + reg_block + .ms_dlen() + .write(|w| unsafe { w.ms_data_bitlen().bits(rx_len.max(tx_len)) }); + } - reg_block - .miso_dlen() - .write(|w| unsafe { w.usr_miso_dbitlen().bits(len) }); } } } diff --git a/hil-test/tests/spi_full_duplex.rs b/hil-test/tests/spi_full_duplex.rs index e8c2e6245c..00b5e579f4 100644 --- a/hil-test/tests/spi_full_duplex.rs +++ b/hil-test/tests/spi_full_duplex.rs @@ -27,10 +27,7 @@ use esp_hal::{ use hil_test as _; cfg_if::cfg_if! { - if #[cfg(any( - feature = "esp32", - feature = "esp32s2", - ))] { + if #[cfg(any(esp32, esp32s2))] { type DmaChannelCreator = esp_hal::dma::Spi2DmaChannelCreator; } else { type DmaChannelCreator = esp_hal::dma::ChannelCreator<0>; @@ -64,7 +61,7 @@ mod tests { let dma = Dma::new(peripherals.DMA); cfg_if::cfg_if! { - if #[cfg(any(feature = "esp32", feature = "esp32s2"))] { + if #[cfg(any(esp32, esp32s2))] { let dma_channel = dma.spi2channel; } else { let dma_channel = dma.channel0; @@ -253,9 +250,8 @@ mod tests { } #[test] - #[cfg(not(esp32))] fn test_symmetric_dma_transfer(ctx: Context) { - // This test case sends a large amount of data, twice, to verify that + // This test case sends a large amount of data, multiple times 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(); @@ -278,13 +274,16 @@ mod tests { .unwrap(); (spi, (dma_rx_buf, dma_tx_buf)) = transfer.wait(); - assert_eq!(dma_tx_buf.as_slice(), dma_rx_buf.as_slice()); + if dma_tx_buf.as_slice() != dma_rx_buf.as_slice() { + defmt::info!("dma_tx_buf: {:?}", dma_tx_buf.as_slice()[0..100]); + defmt::info!("dma_rx_buf: {:?}", dma_rx_buf.as_slice()[0..100]); + panic!("Mismatch at iteration {}", i); + } } } #[test] #[timeout(3)] - #[cfg(not(feature = "esp32s2"))] fn test_asymmetric_dma_transfer(ctx: Context) { let (rx_buffer, rx_descriptors, tx_buffer, tx_descriptors) = dma_buffers!(2, 4); let dma_rx_buf = DmaRxBuf::new(rx_descriptors, rx_buffer).unwrap(); @@ -299,30 +298,19 @@ mod tests { .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()[0..1], dma_rx_buf.as_slice()[0..1]); - } + let (spi, (dma_rx_buf, mut dma_tx_buf)) = transfer.wait(); + assert_eq!(dma_tx_buf.as_slice()[0..2], dma_rx_buf.as_slice()[0..2]); - #[test] - #[timeout(3)] - fn test_symmetric_dma_transfer_huge_buffer(ctx: Context) { - let (rx_buffer, rx_descriptors, tx_buffer, tx_descriptors) = dma_buffers!(4096); - let dma_rx_buf = DmaRxBuf::new(rx_descriptors, rx_buffer).unwrap(); - let mut dma_tx_buf = DmaTxBuf::new(tx_descriptors, tx_buffer).unwrap(); + // Try transfer again to make sure DMA isn't in a broken state. - for (i, d) in dma_tx_buf.as_mut_slice().iter_mut().enumerate() { - *d = i as _; - } + dma_tx_buf.fill(&[0xaa, 0xdd, 0xef, 0xbe]); - let spi = ctx - .spi - .with_dma(ctx.dma_channel.configure(false, DmaPriority::Priority0)); let transfer = 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()); + assert_eq!(dma_tx_buf.as_slice()[0..2], dma_rx_buf.as_slice()[0..2]); } #[test]