Merge #1218 #1219

1218: Lora: sx126x: Change timing window to match values found experimentally. r=Dirbaio a=CBJamo

As mentioned in #1188.

1219: stm32/sdmmc: Fix SDIOv1 writes r=Dirbaio a=chemicstry

This fixes writes on sdmmc v1 (SDIO). I'm pretty sure I tested writes in #669, but maybe I was just lucky or I just forgot.

There were two problems:
- Writes require DMA FIFO mode, otherwise SDIO FIFO is under/overrun depending on sdio/pclk2 clock ratio.
- Hardware flow control is broken for sdmmc v1 (I checked F1 and F4 erratas). This causes clock glitches above 12 MHz and results in write CRC errors.

Co-authored-by: Caleb Jamison <caleb@cbjamo.com>
Co-authored-by: chemicstry <chemicstry@gmail.com>

embassy-lora/src/sx126x/mod.rs

@@ -55,10 +55,10 @@ where
     BUS: Error + Format + 'static,
 {
     fn get_rx_window_offset_ms(&self) -> i32 {
-        -3
+        -50
     }
     fn get_rx_window_duration_ms(&self) -> u32 {
-        1003
+        1050
     }
 }
 

embassy-stm32/src/dma/bdma.rs

@@ -192,6 +192,7 @@ mod low_level_api {
             options.flow_ctrl == crate::dma::FlowControl::Dma,
             "Peripheral flow control not supported"
         );
+        assert!(options.fifo_threshold.is_none(), "FIFO mode not supported");
 
         let ch = dma.ch(channel_number as _);
 

embassy-stm32/src/dma/dma.rs

@@ -4,7 +4,7 @@ use core::task::Waker;
 use embassy_cortex_m::interrupt::Priority;
 use embassy_sync::waitqueue::AtomicWaker;
 
-use super::{Burst, FlowControl, Request, TransferOptions, Word, WordSize};
+use super::{Burst, FifoThreshold, FlowControl, Request, TransferOptions, Word, WordSize};
 use crate::_generated::DMA_CHANNEL_COUNT;
 use crate::interrupt::{Interrupt, InterruptExt};
 use crate::pac::dma::{regs, vals};
@@ -40,6 +40,17 @@ impl From<FlowControl> for vals::Pfctrl {
     }
 }
 
+impl From<FifoThreshold> for vals::Fth {
+    fn from(value: FifoThreshold) -> Self {
+        match value {
+            FifoThreshold::Quarter => vals::Fth::QUARTER,
+            FifoThreshold::Half => vals::Fth::HALF,
+            FifoThreshold::ThreeQuarters => vals::Fth::THREEQUARTERS,
+            FifoThreshold::Full => vals::Fth::FULL,
+        }
+    }
+}
+
 struct ChannelState {
     waker: AtomicWaker,
 }
@@ -236,6 +247,16 @@ mod low_level_api {
         ch.par().write_value(peri_addr as u32);
         ch.m0ar().write_value(mem_addr as u32);
         ch.ndtr().write_value(regs::Ndtr(mem_len as _));
+        ch.fcr().write(|w| {
+            if let Some(fth) = options.fifo_threshold {
+                // FIFO mode
+                w.set_dmdis(vals::Dmdis::DISABLED);
+                w.set_fth(fth.into());
+            } else {
+                // Direct mode
+                w.set_dmdis(vals::Dmdis::ENABLED);
+            }
+        });
         ch.cr().write(|w| {
             w.set_dir(dir);
             w.set_msize(data_size);

embassy-stm32/src/dma/gpdma.rs

@@ -176,8 +176,16 @@ mod low_level_api {
         mem_len: usize,
         incr_mem: bool,
         data_size: WordSize,
-        _options: TransferOptions,
+        options: TransferOptions,
     ) {
+        assert!(options.mburst == crate::dma::Burst::Single, "Burst mode not supported");
+        assert!(options.pburst == crate::dma::Burst::Single, "Burst mode not supported");
+        assert!(
+            options.flow_ctrl == crate::dma::FlowControl::Dma,
+            "Peripheral flow control not supported"
+        );
+        assert!(options.fifo_threshold.is_none(), "FIFO mode not supported");
+
         // "Preceding reads and writes cannot be moved past subsequent writes."
         fence(Ordering::SeqCst);
 

embassy-stm32/src/dma/mod.rs

@@ -186,6 +186,19 @@ pub enum FlowControl {
     Peripheral,
 }
 
+#[derive(Debug, Copy, Clone, PartialEq, Eq)]
+#[cfg_attr(feature = "defmt", derive(defmt::Format))]
+pub enum FifoThreshold {
+    /// 1/4 full FIFO
+    Quarter,
+    /// 1/2 full FIFO
+    Half,
+    /// 3/4 full FIFO
+    ThreeQuarters,
+    /// Full FIFO
+    Full,
+}
+
 #[derive(Debug, Copy, Clone, PartialEq, Eq)]
 #[cfg_attr(feature = "defmt", derive(defmt::Format))]
 pub struct TransferOptions {
@@ -195,6 +208,8 @@ pub struct TransferOptions {
     pub mburst: Burst,
     /// Flow control configuration
     pub flow_ctrl: FlowControl,
+    /// FIFO threshold for DMA FIFO mode. If none, direct mode is used.
+    pub fifo_threshold: Option<FifoThreshold>,
 }
 
 impl Default for TransferOptions {
@@ -203,6 +218,7 @@ impl Default for TransferOptions {
             pburst: Burst::Single,
             mburst: Burst::Single,
             flow_ctrl: FlowControl::Dma,
+            fifo_threshold: None,
         }
     }
 }

embassy-stm32/src/sdmmc/mod.rs

@@ -2,6 +2,7 @@
 
 use core::default::Default;
 use core::future::poll_fn;
+use core::ops::{Deref, DerefMut};
 use core::task::Poll;
 
 use embassy_hal_common::drop::OnDrop;
@@ -40,7 +41,23 @@ impl Default for Signalling {
 }
 
 #[repr(align(4))]
-pub struct DataBlock([u8; 512]);
+#[derive(Debug, Clone)]
+#[cfg_attr(feature = "defmt", derive(defmt::Format))]
+pub struct DataBlock(pub [u8; 512]);
+
+impl Deref for DataBlock {
+    type Target = [u8; 512];
+
+    fn deref(&self) -> &Self::Target {
+        &self.0
+    }
+}
+
+impl DerefMut for DataBlock {
+    fn deref_mut(&mut self) -> &mut Self::Target {
+        &mut self.0
+    }
+}
 
 /// Errors
 #[non_exhaustive]
@@ -570,6 +587,12 @@ impl SdmmcInner {
         regs.clkcr().write(|w| {
             w.set_pwrsav(false);
             w.set_negedge(false);
+
+            // Hardware flow control is broken on SDIOv1 and causes clock glitches, which result in CRC errors.
+            // See chip erratas for more details.
+            #[cfg(sdmmc_v1)]
+            w.set_hwfc_en(false);
+            #[cfg(sdmmc_v2)]
             w.set_hwfc_en(true);
 
             #[cfg(sdmmc_v1)]
@@ -807,10 +830,16 @@ impl SdmmcInner {
         let regs = self.0;
         let on_drop = OnDrop::new(|| unsafe { self.on_drop() });
 
+        // sdmmc_v1 uses different cmd/dma order than v2, but only for writes
+        #[cfg(sdmmc_v1)]
+        self.cmd(Cmd::write_single_block(address), true)?;
+
         unsafe {
             self.prepare_datapath_write(buffer as *const [u32; 128], 512, 9, data_transfer_timeout, dma);
             self.data_interrupts(true);
         }
+
+        #[cfg(sdmmc_v2)]
         self.cmd(Cmd::write_single_block(address), true)?;
 
         let res = poll_fn(|cx| {
@@ -922,7 +951,9 @@ impl SdmmcInner {
                 let request = dma.request();
                 dma.start_read(request, regs.fifor().ptr() as *const u32, buffer, crate::dma::TransferOptions {
                     pburst: crate::dma::Burst::Incr4,
+                    mburst: crate::dma::Burst::Incr4,
                     flow_ctrl: crate::dma::FlowControl::Peripheral,
+                    fifo_threshold: Some(crate::dma::FifoThreshold::Full),
                     ..Default::default()
                 });
             } else if #[cfg(sdmmc_v2)] {
@@ -970,7 +1001,9 @@ impl SdmmcInner {
                 let request = dma.request();
                 dma.start_write(request, buffer, regs.fifor().ptr() as *mut u32, crate::dma::TransferOptions {
                     pburst: crate::dma::Burst::Incr4,
+                    mburst: crate::dma::Burst::Incr4,
                     flow_ctrl: crate::dma::FlowControl::Peripheral,
+                    fifo_threshold: Some(crate::dma::FifoThreshold::Full),
                     ..Default::default()
                 });
             } else if #[cfg(sdmmc_v2)] {
@@ -982,6 +1015,11 @@ impl SdmmcInner {
         regs.dctrl().modify(|w| {
             w.set_dblocksize(block_size);
             w.set_dtdir(false);
+            #[cfg(sdmmc_v1)]
+            {
+                w.set_dmaen(true);
+                w.set_dten(true);
+            }
         });
     }
 

examples/stm32f4/src/bin/sdmmc.rs

@@ -4,11 +4,15 @@
 
 use defmt::*;
 use embassy_executor::Spawner;
-use embassy_stm32::sdmmc::Sdmmc;
+use embassy_stm32::sdmmc::{DataBlock, Sdmmc};
 use embassy_stm32::time::mhz;
 use embassy_stm32::{interrupt, Config};
 use {defmt_rtt as _, panic_probe as _};
 
+/// This is a safeguard to not overwrite any data on the SD card.
+/// If you don't care about SD card contents, set this to `true` to test writes.
+const ALLOW_WRITES: bool = false;
+
 #[embassy_executor::main]
 async fn main(_spawner: Spawner) -> ! {
     let mut config = Config::default();
@@ -34,11 +38,42 @@ async fn main(_spawner: Spawner) -> ! {
     // Should print 400kHz for initialization
     info!("Configured clock: {}", sdmmc.clock().0);
 
-    unwrap!(sdmmc.init_card(mhz(25)).await);
+    unwrap!(sdmmc.init_card(mhz(24)).await);
 
     let card = unwrap!(sdmmc.card());
 
     info!("Card: {:#?}", Debug2Format(card));
+    info!("Clock: {}", sdmmc.clock());
+
+    // Arbitrary block index
+    let block_idx = 16;
+
+    // SDMMC uses `DataBlock` instead of `&[u8]` to ensure 4 byte alignment required by the hardware.
+    let mut block = DataBlock([0u8; 512]);
+
+    sdmmc.read_block(block_idx, &mut block).await.unwrap();
+    info!("Read: {=[u8]:X}...{=[u8]:X}", block[..8], block[512 - 8..]);
+
+    if !ALLOW_WRITES {
+        info!("Writing is disabled.");
+        loop {}
+    }
+
+    info!("Filling block with 0x55");
+    block.fill(0x55);
+    sdmmc.write_block(block_idx, &block).await.unwrap();
+    info!("Write done");
+
+    sdmmc.read_block(block_idx, &mut block).await.unwrap();
+    info!("Read: {=[u8]:X}...{=[u8]:X}", block[..8], block[512 - 8..]);
+
+    info!("Filling block with 0xAA");
+    block.fill(0xAA);
+    sdmmc.write_block(block_idx, &block).await.unwrap();
+    info!("Write done");
+
+    sdmmc.read_block(block_idx, &mut block).await.unwrap();
+    info!("Read: {=[u8]:X}...{=[u8]:X}", block[..8], block[512 - 8..]);
 
     loop {}
 }