From 5a993fed3828fa3e6e15f0e912a8725505832f01 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?D=C3=A1niel=20Buga?= Date: Fri, 4 Oct 2024 08:56:34 +0200 Subject: [PATCH] Fix Rx/Tx order in SPI slave driver (#2272) * Add basic test and fix SPI slave dma_transfer arg order * Reset peripheral * Add safe way to read signal value --- esp-hal/src/gpio/interconnect.rs | 5 + esp-hal/src/spi/slave.rs | 104 +++++++----------- hil-test/Cargo.toml | 4 + hil-test/tests/spi_slave.rs | 176 +++++++++++++++++++++++++++++++ 4 files changed, 223 insertions(+), 66 deletions(-) create mode 100644 hil-test/tests/spi_slave.rs diff --git a/esp-hal/src/gpio/interconnect.rs b/esp-hal/src/gpio/interconnect.rs index 4919dfcad..db0dd706d 100644 --- a/esp-hal/src/gpio/interconnect.rs +++ b/esp-hal/src/gpio/interconnect.rs @@ -63,6 +63,11 @@ impl InputSignal { } } + /// Returns the current signal level. + pub fn get_level(&self) -> Level { + self.is_input_high(private::Internal).into() + } + /// Inverts the peripheral's input signal. /// /// Calling this function multiple times toggles the setting. diff --git a/esp-hal/src/spi/slave.rs b/esp-hal/src/spi/slave.rs index ee9e4a8c5..d1ab89546 100644 --- a/esp-hal/src/spi/slave.rs +++ b/esp-hal/src/spi/slave.rs @@ -73,7 +73,7 @@ use core::marker::PhantomData; use super::{Error, FullDuplexMode, SpiMode}; use crate::{ dma::{DescriptorChain, DmaPeripheral, Rx, Tx}, - gpio::{InputSignal, OutputSignal, PeripheralInput, PeripheralOutput, Pull}, + gpio::{InputSignal, OutputSignal, PeripheralInput, PeripheralOutput}, peripheral::{Peripheral, PeripheralRef}, peripherals::spi2::RegisterBlock, private, @@ -110,16 +110,16 @@ where ) -> Spi<'d, T, FullDuplexMode> { crate::into_ref!(spi, sclk, mosi, miso, cs); - sclk.init_input(Pull::None, private::Internal); + sclk.enable_input(true, private::Internal); sclk.connect_input_to_peripheral(spi.sclk_signal(), private::Internal); - mosi.init_input(Pull::None, private::Internal); + mosi.enable_input(true, private::Internal); mosi.connect_input_to_peripheral(spi.mosi_signal(), private::Internal); miso.set_to_push_pull_output(private::Internal); miso.connect_peripheral_to_output(spi.miso_signal(), private::Internal); - cs.init_input(Pull::None, private::Internal); + cs.enable_input(true, private::Internal); cs.connect_input_to_peripheral(spi.cs_signal(), private::Internal); Self::new_internal(spi, mode) @@ -129,6 +129,7 @@ where spi: PeripheralRef<'d, T>, mode: SpiMode, ) -> Spi<'d, T, FullDuplexMode> { + spi.reset_peripheral(); spi.enable_peripheral(); let mut spi = Spi { @@ -366,8 +367,8 @@ pub mod dma { /// line. pub fn dma_transfer<'t, RXBUF, TXBUF>( &'t mut self, - words: &'t TXBUF, read_buffer: &'t mut RXBUF, + words: &'t TXBUF, ) -> Result, Error> where RXBUF: WriteBuffer, @@ -633,7 +634,17 @@ pub trait Instance: private::Sealed { fn cs_signal(&self) -> InputSignal; - fn enable_peripheral(&self); + fn peripheral(&self) -> crate::system::Peripheral; + + #[inline(always)] + fn reset_peripheral(&self) { + PeripheralClockControl::reset(self.peripheral()); + } + + #[inline(always)] + fn enable_peripheral(&self) { + PeripheralClockControl::enable(self.peripheral()); + } fn spi_num(&self) -> u8; @@ -737,13 +748,22 @@ pub trait Instance: private::Sealed { } } -#[cfg(any(esp32c2, esp32c3, esp32c6, esp32h2))] impl Instance for crate::peripherals::SPI2 { #[inline(always)] fn register_block(&self) -> &RegisterBlock { self } + #[inline(always)] + fn peripheral(&self) -> crate::system::Peripheral { + crate::system::Peripheral::Spi2 + } + + #[inline(always)] + fn spi_num(&self) -> u8 { + 2 + } + #[inline(always)] fn sclk_signal(&self) -> InputSignal { InputSignal::FSPICLK @@ -763,63 +783,25 @@ impl Instance for crate::peripherals::SPI2 { fn cs_signal(&self) -> InputSignal { InputSignal::FSPICS0 } - - #[inline(always)] - fn enable_peripheral(&self) { - PeripheralClockControl::enable(crate::system::Peripheral::Spi2); - } - - #[inline(always)] - fn spi_num(&self) -> u8 { - 2 - } } -#[cfg(any(esp32s2, esp32s3))] -impl Instance for crate::peripherals::SPI2 { - #[inline(always)] - fn register_block(&self) -> &RegisterBlock { - self - } - - #[inline(always)] - fn sclk_signal(&self) -> InputSignal { - InputSignal::FSPICLK - } - - #[inline(always)] - fn mosi_signal(&self) -> InputSignal { - InputSignal::FSPID - } - - #[inline(always)] - fn miso_signal(&self) -> OutputSignal { - OutputSignal::FSPIQ - } - - #[inline(always)] - fn cs_signal(&self) -> InputSignal { - InputSignal::FSPICS0 - } - - #[inline(always)] - fn enable_peripheral(&self) { - PeripheralClockControl::enable(crate::system::Peripheral::Spi2) - } - - #[inline(always)] - fn spi_num(&self) -> u8 { - 2 - } -} - -#[cfg(any(esp32s2, esp32s3))] +#[cfg(spi3)] impl Instance for crate::peripherals::SPI3 { #[inline(always)] fn register_block(&self) -> &RegisterBlock { self } + #[inline(always)] + fn peripheral(&self) -> crate::system::Peripheral { + crate::system::Peripheral::Spi3 + } + + #[inline(always)] + fn spi_num(&self) -> u8 { + 3 + } + #[inline(always)] fn sclk_signal(&self) -> InputSignal { InputSignal::SPI3_CLK @@ -839,14 +821,4 @@ impl Instance for crate::peripherals::SPI3 { fn cs_signal(&self) -> InputSignal { InputSignal::SPI3_CS0 } - - #[inline(always)] - fn enable_peripheral(&self) { - PeripheralClockControl::enable(crate::system::Peripheral::Spi3) - } - - #[inline(always)] - fn spi_num(&self) -> u8 { - 3 - } } diff --git a/hil-test/Cargo.toml b/hil-test/Cargo.toml index 466be49eb..b0c0a3f3e 100644 --- a/hil-test/Cargo.toml +++ b/hil-test/Cargo.toml @@ -99,6 +99,10 @@ harness = false name = "spi_half_duplex_write_psram" harness = false +[[test]] +name = "spi_slave" +harness = false + [[test]] name = "systimer" harness = false diff --git a/hil-test/tests/spi_slave.rs b/hil-test/tests/spi_slave.rs new file mode 100644 index 000000000..a8b73a4cd --- /dev/null +++ b/hil-test/tests/spi_slave.rs @@ -0,0 +1,176 @@ +//! SPI slave mode test suite. + +//% CHIPS: esp32c2 esp32c3 esp32c6 esp32h2 esp32s2 esp32s3 + +#![no_std] +#![no_main] + +use esp_hal::{ + dma::{Dma, DmaPriority}, + dma_buffers, + gpio::{interconnect::InputSignal, Io, Level, Output, PeripheralInput}, + peripherals::SPI2, + spi::{slave::Spi, FullDuplexMode, SpiMode}, +}; +use hil_test as _; + +cfg_if::cfg_if! { + if #[cfg(any(esp32, esp32s2))] { + type DmaChannelCreator = esp_hal::dma::Spi2DmaChannelCreator; + } else { + type DmaChannelCreator = esp_hal::dma::ChannelCreator<0>; + } +} + +struct Context { + spi: Spi<'static, SPI2, FullDuplexMode>, + dma_channel: DmaChannelCreator, + bitbang_spi: BitbangSpi, +} + +struct BitbangSpi { + sclk: Output<'static>, + mosi: Output<'static>, + miso: InputSignal, + cs: Output<'static>, +} + +impl BitbangSpi { + fn new( + sclk: Output<'static>, + mosi: Output<'static>, + miso: InputSignal, + cs: Output<'static>, + ) -> Self { + Self { + sclk, + mosi, + miso, + cs, + } + } + + fn assert_cs(&mut self) { + self.cs.set_level(Level::Low); + } + + fn deassert_cs(&mut self) { + self.cs.set_level(Level::High); + } + + // Mode 0, so sampled on the rising edge and set on the falling edge. + fn shift_bit(&mut self, bit: bool) -> bool { + self.mosi.set_level(Level::from(bit)); + self.sclk.set_level(Level::Low); + + let miso = self.miso.get_level().into(); + self.sclk.set_level(Level::High); + + miso + } + + // Shift a byte out and in, MSB first. + fn shift_byte(&mut self, byte: u8) -> u8 { + let mut out = 0; + for i in 0..8 { + let shift = 7 - i; + out |= (self.shift_bit((byte >> shift) & 1 != 0) as u8) << shift; + } + out + } + + fn transfer_buf(&mut self, rx: &mut [u8], tx: &[u8]) { + self.assert_cs(); + for (tx, rx) in tx.iter().zip(rx.iter_mut()) { + *rx = self.shift_byte(*tx); + } + self.deassert_cs(); + } +} + +#[cfg(test)] +#[embedded_test::tests(executor = esp_hal_embassy::Executor::new())] +mod tests { + use super::*; + + #[init] + fn init() -> Context { + let peripherals = esp_hal::init(esp_hal::Config::default()); + + let io = Io::new(peripherals.GPIO, peripherals.IO_MUX); + let (mosi_pin, miso_pin) = hil_test::i2c_pins!(io); + let (sclk_pin, sclk_gpio) = hil_test::common_test_pins!(io); + let cs_pin = hil_test::unconnected_pin!(io); + + let dma = Dma::new(peripherals.DMA); + + cfg_if::cfg_if! { + if #[cfg(any(esp32, esp32s2))] { + let dma_channel = dma.spi2channel; + } else { + let dma_channel = dma.channel0; + } + } + + let cs = cs_pin.peripheral_input(); + let mosi = mosi_pin.peripheral_input(); + let mut miso = miso_pin.peripheral_input(); + + let mosi_gpio = Output::new(mosi_pin, Level::Low); + let cs_gpio = Output::new(cs_pin, Level::High); + let sclk_gpio = Output::new(sclk_gpio, Level::Low); + + let spi = Spi::new( + peripherals.SPI2, + sclk_pin, + mosi, + miso_pin, + cs, + SpiMode::Mode0, + ); + + miso.enable_input(true, unsafe { esp_hal::Internal::conjure() }); + + Context { + spi, + dma_channel, + bitbang_spi: BitbangSpi::new(sclk_gpio, mosi_gpio, miso, cs_gpio), + } + } + + #[test] + #[timeout(10)] + fn test_basic(mut ctx: Context) { + const DMA_SIZE: usize = 32; + let (rx_buffer, rx_descriptors, tx_buffer, tx_descriptors) = dma_buffers!(DMA_SIZE); + let mut spi = ctx.spi.with_dma( + ctx.dma_channel.configure(false, DmaPriority::Priority0), + tx_descriptors, + rx_descriptors, + ); + let slave_send = tx_buffer; + let slave_receive = rx_buffer; + + // The transfer stops if the buffers are full, not when the master + // deasserts CS. Therefore, these need to be the same size as the DMA buffers. + let master_send = &mut [0u8; DMA_SIZE]; + let master_receive = &mut [0xFFu8; DMA_SIZE]; + + for (i, v) in master_send.iter_mut().enumerate() { + *v = (i % 255) as u8; + } + for (i, v) in slave_send.iter_mut().enumerate() { + *v = (254 - (i % 255)) as u8; + } + slave_receive.fill(0xFF); + + let transfer = spi.dma_transfer(slave_receive, &slave_send).unwrap(); + + ctx.bitbang_spi.transfer_buf(master_receive, master_send); + + transfer.wait().unwrap(); + + assert_eq!(slave_receive, master_send); + assert_eq!(master_receive, slave_send); + } +}