Fix incorrect QSPI setup (#2231)

* Fix QSPI mode on non-ESP32

* Update tests to count pulses on pins separately

* Fix ESP32 addressing phase issue

* Use defmt's assert

* Set fastrd bit

* Apply pulldowns to define signal level when not driven

* Transfer address bits in data phase on ESP32

* Changelog

* Use a separate buffer for the address, make the workaround configurable

* Remove now-unnecessary additions

* Force SpiDma to remain Send

* Clarify wording, remove prefix

* Clean up manual register manipulation

* Fix byte order

esp-hal/CHANGELOG.md

@@ -72,6 +72,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - 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)
+- SPI: ESP32: Send address with correct data mode even when no data is sent. (#2231)
 - PARL_IO: Fixed an issue that caused garbage to be output at the start of some requests (#2211)
 - TWAI on ESP32 (#2207)
 

esp-hal/build.rs

@@ -128,11 +128,18 @@ fn main() -> Result<(), Box<dyn Error>> {
     // emit config
     generate_config(
         "esp_hal",
-        &[(
-            "place-spi-driver-in-ram",
-            Value::Bool(false),
-            "Places the SPI driver in RAM for better performance",
-        )],
+        &[
+            (
+                "place-spi-driver-in-ram",
+                Value::Bool(false),
+                "Places the SPI driver in RAM for better performance",
+            ),
+            (
+                "spi-address-workaround",
+                Value::Bool(true),
+                "(ESP32 only) Enables a workaround for the issue where SPI in half-duplex mode incorrectly transmits the address on a single line if the data buffer is empty.",
+            ),
+        ],
         true,
     );
 

esp-hal/src/dma/mod.rs

@@ -514,7 +514,7 @@ macro_rules! declare_aligned_dma_buffer {
 #[doc(hidden)]
 #[macro_export]
 macro_rules! as_mut_byte_array {
-    ($name:ident, $size:expr) => {
+    ($name:expr, $size:expr) => {
         unsafe { &mut *($name.as_mut_ptr() as *mut [u8; $size]) }
     };
 }
@@ -731,6 +731,18 @@ pub enum DmaError {
     InvalidChunkSize,
 }
 
+impl From<DmaBufError> for DmaError {
+    fn from(error: DmaBufError) -> Self {
+        // FIXME: use nested errors
+        match error {
+            DmaBufError::InsufficientDescriptors => DmaError::OutOfDescriptors,
+            DmaBufError::UnsupportedMemoryRegion => DmaError::UnsupportedMemoryRegion,
+            DmaBufError::InvalidAlignment => DmaError::InvalidAlignment,
+            DmaBufError::InvalidChunkSize => DmaError::InvalidChunkSize,
+        }
+    }
+}
+
 /// DMA Priorities
 #[cfg(gdma)]
 #[derive(Debug, Clone, Copy, PartialEq)]

esp-hal/src/spi/master.rs

@@ -829,6 +829,26 @@ where
             return Err(Error::FifoSizeExeeded);
         }
 
+        cfg_if::cfg_if! {
+            if #[cfg(all(esp32, spi_address_workaround))] {
+                let mut buffer = buffer;
+                let mut data_mode = data_mode;
+                let mut address = address;
+                let addr_bytes;
+                if buffer.is_empty() && !address.is_none() {
+                    // If the buffer is empty, we need to send a dummy byte
+                    // to trigger the address phase.
+                    let bytes_to_write = address.width().div_ceil(8);
+                    // The address register is read in big-endian order,
+                    // we have to prepare the emulated write in the same way.
+                    addr_bytes = address.value().to_be_bytes();
+                    buffer = &addr_bytes[4 - bytes_to_write..][..bytes_to_write];
+                    data_mode = address.mode();
+                    address = Address::None;
+                }
+            }
+        }
+
         self.spi.setup_half_duplex(
             true,
             cmd,
@@ -933,11 +953,7 @@ mod dma {
             C::P: SpiPeripheral + Spi2Peripheral,
             DmaMode: Mode,
         {
-            SpiDma {
-                spi: self.spi,
-                channel,
-                _mode: PhantomData,
-            }
+            SpiDma::new(self.spi, channel)
         }
     }
 
@@ -960,11 +976,7 @@ mod dma {
             C::P: SpiPeripheral + Spi3Peripheral,
             DmaMode: Mode,
         {
-            SpiDma {
-                spi: self.spi,
-                channel,
-                _mode: PhantomData,
-            }
+            SpiDma::new(self.spi, channel)
         }
     }
 
@@ -984,9 +996,21 @@ mod dma {
     {
         pub(crate) spi: PeripheralRef<'d, T>,
         pub(crate) channel: Channel<'d, C, M>,
+        #[cfg(all(esp32, spi_address_workaround))]
+        address_buffer: DmaTxBuf,
         _mode: PhantomData<D>,
     }
 
+    #[cfg(all(esp32, spi_address_workaround))]
+    unsafe impl<'d, T, C, D, M> Send for SpiDma<'d, T, C, D, M>
+    where
+        C: DmaChannel,
+        C::P: SpiPeripheral,
+        D: DuplexMode,
+        M: Mode,
+    {
+    }
+
     impl<'d, T, C, D, M> core::fmt::Debug for SpiDma<'d, T, C, D, M>
     where
         C: DmaChannel,
@@ -1011,6 +1035,36 @@ mod dma {
         D: DuplexMode,
         M: Mode,
     {
+        fn new(spi: PeripheralRef<'d, T>, channel: Channel<'d, C, M>) -> Self {
+            cfg_if::cfg_if! {
+                if #[cfg(all(esp32, spi_address_workaround))] {
+                    use crate::dma::DmaDescriptor;
+                    const SPI_NUM: usize = 2;
+                    static mut DESCRIPTORS: [[DmaDescriptor; 1]; SPI_NUM] =
+                        [[DmaDescriptor::EMPTY]; SPI_NUM];
+                    static mut BUFFERS: [[u32; 1]; SPI_NUM] = [[0; 1]; SPI_NUM];
+
+                    let id = spi.spi_num() as usize - 2;
+
+                    Self {
+                        spi,
+                        channel,
+                        address_buffer: unwrap!(DmaTxBuf::new(
+                            unsafe { &mut DESCRIPTORS[id] },
+                            crate::as_mut_byte_array!(BUFFERS[id], 4)
+                        )),
+                        _mode: PhantomData,
+                    }
+                } else {
+                    Self {
+                        spi,
+                        channel,
+                        _mode: PhantomData,
+                    }
+                }
+            }
+        }
+
         /// Sets the interrupt handler
         ///
         /// Interrupts are not enabled at the peripheral level here.
@@ -1374,6 +1428,43 @@ mod dma {
                 return Err((Error::MaxDmaTransferSizeExceeded, self, buffer));
             }
 
+            #[cfg(all(esp32, spi_address_workaround))]
+            {
+                // On the ESP32, if we don't have data, the address is always sent
+                // on a single line, regardless of its data mode.
+                if bytes_to_write == 0 && address.mode() != SpiDataMode::Single {
+                    let bytes_to_write = address.width().div_ceil(8);
+                    // The address register is read in big-endian order,
+                    // we have to prepare the emulated write in the same way.
+                    let addr_bytes = address.value().to_be_bytes();
+                    let addr_bytes = &addr_bytes[4 - bytes_to_write..][..bytes_to_write];
+                    self.address_buffer.fill(addr_bytes);
+
+                    self.spi.setup_half_duplex(
+                        true,
+                        cmd,
+                        Address::None,
+                        false,
+                        dummy,
+                        bytes_to_write == 0,
+                        address.mode(),
+                    );
+
+                    let result = unsafe {
+                        self.spi.start_write_bytes_dma(
+                            &mut self.address_buffer,
+                            &mut self.channel.tx,
+                            false,
+                        )
+                    };
+                    if let Err(e) = result {
+                        return Err((e, self, buffer));
+                    }
+
+                    return Ok(SpiDmaTransfer::new(self, buffer, false, bytes_to_write > 0));
+                }
+            }
+
             self.spi.setup_half_duplex(
                 true,
                 cmd,
@@ -2461,6 +2552,10 @@ pub trait Instance: private::Sealed {
             w.fread_dual().bit(data_mode == SpiDataMode::Dual);
             w.fread_quad().bit(data_mode == SpiDataMode::Quad)
         });
+        reg_block.user().modify(|_, w| {
+            w.fwrite_dual().bit(data_mode == SpiDataMode::Dual);
+            w.fwrite_quad().bit(data_mode == SpiDataMode::Quad)
+        });
     }
 
     #[cfg(esp32)]
@@ -2494,6 +2589,7 @@ pub trait Instance: private::Sealed {
             }
             address_mode if address_mode == data_mode => {
                 reg_block.ctrl().modify(|_, w| {
+                    w.fastrd_mode().set_bit();
                     w.fread_dio().bit(address_mode == SpiDataMode::Dual);
                     w.fread_qio().bit(address_mode == SpiDataMode::Quad);
                     w.fread_dual().clear_bit();
@@ -3019,25 +3115,17 @@ fn set_up_common_phases(reg_block: &RegisterBlock, cmd: Command, address: Addres
         });
     }
 
-    #[cfg(not(esp32))]
     if !address.is_none() {
         reg_block
             .user1()
             .modify(|_, w| unsafe { w.usr_addr_bitlen().bits((address.width() - 1) as u8) });
 
         let addr = address.value() << (32 - address.width());
+        #[cfg(not(esp32))]
         reg_block
             .addr()
             .write(|w| unsafe { w.usr_addr_value().bits(addr) });
-    }
-
-    #[cfg(esp32)]
-    if !address.is_none() {
-        reg_block.user1().modify(|r, w| unsafe {
-            w.bits(r.bits() & !(0x3f << 26) | (((address.width() - 1) as u32) & 0x3f) << 26)
-        });
-
-        let addr = address.value() << (32 - address.width());
+        #[cfg(esp32)]
         reg_block.addr().write(|w| unsafe { w.bits(addr) });
     }
 

hil-test/tests/qspi.rs

@@ -5,6 +5,7 @@
 #![no_std]
 #![no_main]
 
+use defmt::assert_eq;
 #[cfg(pcnt)]
 use esp_hal::pcnt::{channel::EdgeMode, unit::Unit, Pcnt};
 use esp_hal::{
@@ -125,7 +126,13 @@ fn execute_write_read(mut spi: SpiUnderTest, mut mosi_mirror: Output<'static>, e
 }
 
 #[cfg(pcnt)]
-fn execute_write(unit: Unit<'static, 0>, mut spi: SpiUnderTest, write: u8) {
+fn execute_write(
+    unit0: Unit<'static, 0>,
+    unit1: Unit<'static, 1>,
+    mut spi: SpiUnderTest,
+    write: u8,
+    data_on_multiple_pins: bool,
+) {
     const DMA_BUFFER_SIZE: usize = 4;
 
     let (_, _, buffer, descriptors) = dma_buffers!(0, DMA_BUFFER_SIZE);
@@ -134,18 +141,40 @@ fn execute_write(unit: Unit<'static, 0>, mut spi: SpiUnderTest, write: u8) {
     for command_data_mode in COMMAND_DATA_MODES {
         dma_tx_buf.fill(&[write; DMA_BUFFER_SIZE]);
 
-        // Send command + data.
-        // Should read 8 bits: 1 command bit, 3 address bits, 4 data bits
-        unit.clear();
+        // Send command + address + data.
+        // Should read 8 high bits: 1 command bit, 3 address bits, 4 data bits
+        unit0.clear();
+        unit1.clear();
         (spi, dma_tx_buf) = transfer_write(spi, dma_tx_buf, write, command_data_mode);
-        assert_eq!(unit.get_value(), 8);
+        assert_eq!(unit0.get_value() + unit1.get_value(), 8);
+
+        if data_on_multiple_pins {
+            if command_data_mode == SpiDataMode::Single {
+                assert_eq!(unit0.get_value(), 1);
+                assert_eq!(unit1.get_value(), 7);
+            } else {
+                assert_eq!(unit0.get_value(), 0);
+                assert_eq!(unit1.get_value(), 8);
+            }
+        }
 
         // Send command + address only
-        // Should read 4 bits: 1 command bit, 3 address bits
+        // Should read 4 bits high: 1 command bit, 3 address bits
         dma_tx_buf.set_length(0);
-        unit.clear();
+        unit0.clear();
+        unit1.clear();
         (spi, dma_tx_buf) = transfer_write(spi, dma_tx_buf, write, command_data_mode);
-        assert_eq!(unit.get_value(), 4);
+        assert_eq!(unit0.get_value() + unit1.get_value(), 4);
+
+        if data_on_multiple_pins {
+            if command_data_mode == SpiDataMode::Single {
+                assert_eq!(unit0.get_value(), 1);
+                assert_eq!(unit1.get_value(), 3);
+            } else {
+                assert_eq!(unit0.get_value(), 0);
+                assert_eq!(unit1.get_value(), 4);
+            }
+        }
     }
 }
 
@@ -164,9 +193,9 @@ mod tests {
         let mut unconnected_pin = hil_test::unconnected_pin!(io);
 
         // Make sure pins have no pullups
-        let _ = Input::new(&mut pin, Pull::None);
-        let _ = Input::new(&mut pin_mirror, Pull::None);
-        let _ = Input::new(&mut unconnected_pin, Pull::None);
+        let _ = Input::new(&mut pin, Pull::Down);
+        let _ = Input::new(&mut pin_mirror, Pull::Down);
+        let _ = Input::new(&mut unconnected_pin, Pull::Down);
 
         let dma = Dma::new(peripherals.DMA);
 
@@ -306,17 +335,19 @@ mod tests {
         let [_, _, mosi] = ctx.gpios;
 
         let pcnt = Pcnt::new(ctx.pcnt);
-        let unit = pcnt.unit0;
+        let unit0 = pcnt.unit0;
+        let unit1 = pcnt.unit1;
 
-        unit.channel0.set_edge_signal(mosi.peripheral_input());
-        unit.channel0
+        unit0.channel0.set_edge_signal(mosi.peripheral_input());
+        unit0
+            .channel0
             .set_input_mode(EdgeMode::Hold, EdgeMode::Increment);
 
         let spi = Spi::new_half_duplex(ctx.spi, 100.kHz(), SpiMode::Mode0)
             .with_pins(NoPin, mosi, NoPin, NoPin, NoPin, NoPin)
             .with_dma(ctx.dma_channel);
 
-        super::execute_write(unit, spi, 0b0000_0001);
+        super::execute_write(unit0, unit1, spi, 0b0000_0001, false);
     }
 
     #[test]
@@ -328,21 +359,24 @@ mod tests {
         let [gpio, _, mosi] = ctx.gpios;
 
         let pcnt = Pcnt::new(ctx.pcnt);
-        let unit = pcnt.unit0;
+        let unit0 = pcnt.unit0;
+        let unit1 = pcnt.unit1;
 
-        unit.channel0.set_edge_signal(mosi.peripheral_input());
-        unit.channel0
+        unit0.channel0.set_edge_signal(mosi.peripheral_input());
+        unit0
+            .channel0
             .set_input_mode(EdgeMode::Hold, EdgeMode::Increment);
 
-        unit.channel1.set_edge_signal(gpio.peripheral_input());
-        unit.channel1
+        unit1.channel0.set_edge_signal(gpio.peripheral_input());
+        unit1
+            .channel0
             .set_input_mode(EdgeMode::Hold, EdgeMode::Increment);
 
         let spi = Spi::new_half_duplex(ctx.spi, 100.kHz(), SpiMode::Mode0)
             .with_pins(NoPin, mosi, gpio, NoPin, NoPin, NoPin)
             .with_dma(ctx.dma_channel);
 
-        super::execute_write(unit, spi, 0b0000_0010);
+        super::execute_write(unit0, unit1, spi, 0b0000_0010, true);
     }
 
     #[test]
@@ -354,21 +388,24 @@ mod tests {
         let [gpio, _, mosi] = ctx.gpios;
 
         let pcnt = Pcnt::new(ctx.pcnt);
-        let unit = pcnt.unit0;
+        let unit0 = pcnt.unit0;
+        let unit1 = pcnt.unit1;
 
-        unit.channel0.set_edge_signal(mosi.peripheral_input());
-        unit.channel0
+        unit0.channel0.set_edge_signal(mosi.peripheral_input());
+        unit0
+            .channel0
             .set_input_mode(EdgeMode::Hold, EdgeMode::Increment);
 
-        unit.channel1.set_edge_signal(gpio.peripheral_input());
-        unit.channel1
+        unit1.channel0.set_edge_signal(gpio.peripheral_input());
+        unit1
+            .channel0
             .set_input_mode(EdgeMode::Hold, EdgeMode::Increment);
 
         let spi = Spi::new_half_duplex(ctx.spi, 100.kHz(), SpiMode::Mode0)
             .with_pins(NoPin, mosi, NoPin, gpio, NoPin, NoPin)
             .with_dma(ctx.dma_channel);
 
-        super::execute_write(unit, spi, 0b0000_0100);
+        super::execute_write(unit0, unit1, spi, 0b0000_0100, true);
     }
 
     #[test]
@@ -380,20 +417,23 @@ mod tests {
         let [gpio, _, mosi] = ctx.gpios;
 
         let pcnt = Pcnt::new(ctx.pcnt);
-        let unit = pcnt.unit0;
+        let unit0 = pcnt.unit0;
+        let unit1 = pcnt.unit1;
 
-        unit.channel0.set_edge_signal(mosi.peripheral_input());
-        unit.channel0
+        unit0.channel0.set_edge_signal(mosi.peripheral_input());
+        unit0
+            .channel0
             .set_input_mode(EdgeMode::Hold, EdgeMode::Increment);
 
-        unit.channel1.set_edge_signal(gpio.peripheral_input());
-        unit.channel1
+        unit1.channel0.set_edge_signal(gpio.peripheral_input());
+        unit1
+            .channel0
             .set_input_mode(EdgeMode::Hold, EdgeMode::Increment);
 
         let spi = Spi::new_half_duplex(ctx.spi, 100.kHz(), SpiMode::Mode0)
             .with_pins(NoPin, mosi, NoPin, NoPin, gpio, NoPin)
             .with_dma(ctx.dma_channel);
 
-        super::execute_write(unit, spi, 0b0000_1000);
+        super::execute_write(unit0, unit1, spi, 0b0000_1000, true);
     }
 }