Various minor SPI fixes (#3201)

* Correctly clear fastrd_mode when data mode is Single

* Print logs in QA test

* Port SPI timing tuning from esp-idf

* Fix ESP32 QSPI signals

* Print the reason of an Unsupported error

* Partial changelog

esp-hal/CHANGELOG.md

@@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `Uart::flush_async` should no longer return prematurely (#3186)
 - Detecting a UART overflow now clears the RX FIFO. (#3190)
 - ESP32-S2: Fixed PSRAM initialization (#3196)
+- ESP32: Fixed SPI3 QSPI signals (#3201)
 
 ### Removed
 

esp-hal/src/spi/master.rs

@@ -34,6 +34,8 @@
 //! [`embedded-hal-bus`]: https://docs.rs/embedded-hal-bus/latest/embedded_hal_bus/spi/index.html
 //! [`embassy-embedded-hal`]: embassy_embedded_hal::shared_bus
 
+#[cfg(esp32)]
+use core::cell::Cell;
 use core::marker::PhantomData;
 
 #[instability::unstable]
@@ -1132,6 +1134,7 @@ where
         }
 
         if buffer.is_empty() {
+            error!("Half-duplex mode does not support empty buffer");
             return Err(Error::Unsupported);
         }
 
@@ -1194,6 +1197,7 @@ where
 
                 if dummy > 0 {
                     // FIXME: https://github.com/esp-rs/esp-hal/issues/2240
+                    error!("Dummy bits are not supported without data");
                     return Err(Error::Unsupported);
                 }
             }
@@ -1539,6 +1543,7 @@ mod dma {
         ) -> Result<(), Error> {
             if dummy > 0 {
                 // FIXME: https://github.com/esp-rs/esp-hal/issues/2240
+                error!("Dummy bits are not supported when there is no data to write");
                 return Err(Error::Unsupported);
             }
 
@@ -2981,12 +2986,17 @@ impl Driver {
             DataMode::Single => (),
             DataMode::SingleTwoDataLines => (),
             // FIXME: more detailed error - Only 1-bit commands are supported.
-            _ => return Err(Error::Unsupported),
+            _ => {
+                error!("Commands must be single bit wide");
+                return Err(Error::Unsupported);
+            }
         }
 
         match address_mode {
             DataMode::Single | DataMode::SingleTwoDataLines => {
                 self.regs().ctrl().modify(|_, w| {
+                    w.fastrd_mode()
+                        .bit(matches!(data_mode, DataMode::Dual | DataMode::Quad));
                     w.fread_dio().clear_bit();
                     w.fread_qio().clear_bit();
                     w.fread_dual().bit(data_mode == DataMode::Dual);
@@ -3002,7 +3012,8 @@ impl Driver {
             }
             address_mode if address_mode == data_mode => {
                 self.regs().ctrl().modify(|_, w| {
-                    w.fastrd_mode().set_bit();
+                    w.fastrd_mode()
+                        .bit(matches!(data_mode, DataMode::Dual | DataMode::Quad));
                     w.fread_dio().bit(address_mode == DataMode::Dual);
                     w.fread_qio().bit(address_mode == DataMode::Quad);
                     w.fread_dual().clear_bit();
@@ -3016,8 +3027,11 @@ impl Driver {
                     w.fwrite_quad().clear_bit()
                 });
             }
-            // FIXME: more detailed error - Unsupported combination of data-modes,
-            _ => return Err(Error::Unsupported),
+            _ => {
+                // FIXME: more detailed error - Unsupported combination of data-modes
+                error!("Address mode must be single bit wide or equal to the data mode");
+                return Err(Error::Unsupported);
+            }
         }
 
         Ok(())
@@ -3147,9 +3161,60 @@ impl Driver {
         self.ch_bus_freq(config)?;
         self.set_bit_order(config.read_bit_order, config.write_bit_order);
         self.set_data_mode(config.mode);
+
+        #[cfg(esp32)]
+        self.calculate_half_duplex_values(config);
+
         Ok(())
     }
 
+    #[cfg(esp32)]
+    fn calculate_half_duplex_values(&self, config: &Config) {
+        let f_apb = 80_000_000;
+        let source_freq_hz = match config.clock_source {
+            ClockSource::Apb => f_apb,
+        };
+
+        let clock_reg = self.regs().clock().read();
+        let eff_clk = if clock_reg.clk_equ_sysclk().bit_is_set() {
+            f_apb
+        } else {
+            let pre = clock_reg.clkdiv_pre().bits() as i32 + 1;
+            let n = clock_reg.clkcnt_n().bits() as i32 + 1;
+            f_apb / (pre * n)
+        };
+
+        let apbclk_khz = source_freq_hz / 1000;
+        // how many apb clocks a period has
+        let spiclk_apb_n = source_freq_hz / eff_clk;
+
+        // How many apb clocks the delay is, the 1 is to compensate in case
+        // ``input_delay_ns`` is rounded off. Change from esp-idf: we use
+        // `input_delay_ns` to also represent the GPIO matrix delay.
+        let input_delay_ns = 25; // TODO: allow configuring input delay.
+        let delay_apb_n = (1 + input_delay_ns) * apbclk_khz / 1000 / 1000;
+
+        let dummy_required = delay_apb_n / spiclk_apb_n;
+        let timing_miso_delay = if dummy_required > 0 {
+            // due to the clock delay between master and slave, there's a range in which
+            // data is random give MISO a delay if needed to make sure we
+            // sample at the time MISO is stable
+            Some(((dummy_required + 1) * spiclk_apb_n - delay_apb_n - 1) as u8)
+        } else if delay_apb_n * 4 <= spiclk_apb_n {
+            // if the dummy is not required, maybe we should also delay half a SPI clock if
+            // the data comes too early
+            None
+        } else {
+            Some(0)
+        };
+
+        self.state.esp32_hack.extra_dummy.set(dummy_required as u8);
+        self.state
+            .esp32_hack
+            .timing_miso_delay
+            .set(timing_miso_delay);
+    }
+
     fn set_data_mode(&self, data_mode: Mode) {
         cfg_if::cfg_if! {
             if #[cfg(esp32)] {
@@ -3415,6 +3480,14 @@ impl Driver {
             DataMode::SingleTwoDataLines,
             DataMode::SingleTwoDataLines,
         )?;
+
+        // For full-duplex, we don't need compensation according to esp-idf.
+        #[cfg(esp32)]
+        self.regs().ctrl2().modify(|_, w| unsafe {
+            w.miso_delay_mode().bits(0);
+            w.miso_delay_num().bits(0)
+        });
+
         Ok(())
     }
 
@@ -3438,11 +3511,41 @@ impl Driver {
                 || (address != Address::None && address.mode() != DataMode::Single)
                 || data_mode != DataMode::Single)
         {
+            error!("Three-wire mode is only supported for single data line mode");
             return Err(Error::Unsupported);
         }
 
         self.init_spi_data_mode(cmd.mode(), address.mode(), data_mode)?;
 
+        #[cfg(esp32)]
+        let mut dummy = dummy;
+
+        #[cfg(esp32)]
+        self.regs().ctrl2().modify(|_, w| {
+            let mut delay_mode = 0;
+            let mut delay_num = 0;
+
+            if !is_write {
+                // Values are set up in apply_config
+                let timing_miso_delay = self.state.esp32_hack.timing_miso_delay.get();
+                let extra_dummy = self.state.esp32_hack.extra_dummy.get();
+                dummy += extra_dummy;
+
+                if let Some(delay) = timing_miso_delay {
+                    delay_num = if extra_dummy > 0 { delay } else { 0 };
+                } else {
+                    let out_edge = self.regs().user().read().ck_out_edge().bit_is_set();
+                    // SPI modes 1 and 2 need delay mode 1 according to esp-idf.
+                    delay_mode = if out_edge { 1 } else { 2 };
+                }
+            }
+
+            unsafe {
+                w.miso_delay_mode().bits(delay_mode);
+                w.miso_delay_num().bits(delay_num)
+            }
+        });
+
         let reg_block = self.regs();
         reg_block.user().modify(|_, w| {
             w.usr_miso_highpart().clear_bit();
@@ -3627,7 +3730,7 @@ cfg_if::cfg_if! {
 #[cfg(spi3)]
 cfg_if::cfg_if! {
     if #[cfg(esp32)] {
-        spi_instance!(3, VSPICLK, VSPID, VSPIQ, VSPICS0, HSPIWP, HSPIHD);
+        spi_instance!(3, VSPICLK, VSPID, VSPIQ, VSPICS0, VSPIWP, VSPIHD);
     } else if #[cfg(esp32s3)] {
         spi_instance!(3, SPI3_CLK, SPI3_D, SPI3_Q, SPI3_CS0, SPI3_WP, SPI3_HD);
     } else {
@@ -3652,8 +3755,20 @@ impl QspiInstance for super::AnySpi {}
 #[doc(hidden)]
 pub struct State {
     waker: AtomicWaker,
+
+    #[cfg(esp32)]
+    esp32_hack: Esp32Hack,
 }
 
+#[cfg(esp32)]
+struct Esp32Hack {
+    timing_miso_delay: Cell<Option<u8>>,
+    extra_dummy: Cell<u8>,
+}
+
+#[cfg(esp32)]
+unsafe impl Sync for Esp32Hack {}
+
 #[cfg_attr(place_spi_driver_in_ram, ram)]
 fn handle_async<I: Instance>(instance: I) {
     let state = instance.state();
@@ -3678,6 +3793,11 @@ macro_rules! master_instance {
             fn state(&self) -> &'static State {
                 static STATE: State = State {
                     waker: AtomicWaker::new(),
+                    #[cfg(esp32)]
+                    esp32_hack: Esp32Hack {
+                        timing_miso_delay: Cell::new(None),
+                        extra_dummy: Cell::new(0),
+                    },
                 };
 
                 &STATE

qa-test/src/bin/spi_halfduplex_read_manufacturer_id.rs

@@ -43,6 +43,7 @@ use esp_println::println;
 
 #[main]
 fn main() -> ! {
+    esp_println::logger::init_logger_from_env();
     let peripherals = esp_hal::init(esp_hal::Config::default());
 
     cfg_if::cfg_if! {