Merge #1228

1228: stm32/sdmmc: Implement proper clock configuration r=chemicstry a=chemicstry

This implements proper clock configuration for sdmmc based on chip family, because `RccPeripheral::frequency()` is almost always incorrect. This can't be fixed in PAC, because sdmmc uses two clock domains, one for memory bus and one for sd card. `RccPeripheral::frequency()` usually returns the memory bus clock, but SDIO clock calculations need sd card domain clock. Moreover, chips have multiple clock source selection bits, which makes this even more complicated. I'm not sure if it's worth implementing all this logic in `RccPeripheral::frequency()` instead of cfg's in sdmmc.

Some chips (Lx, U5, H7) require RCC updates to expose required clocks. I didn't want to mash everything in a single PR so left a TODO comment. I also left a `T::frequency()` fallback, which seemed to work in H7 case even though the clock is most certainly incorrect.

In addition, added support for clock divider bypass for sdmmc_v1, which allows reaching a maximum clock of 48 MHz. The peripheral theoretically supports up to 50 MHz, but for that ST recommends setting pll48 frequency to 50 MHz 🤔

Co-authored-by: chemicstry <chemicstry@gmail.com>

embassy-stm32/src/sdmmc/mod.rs

@@ -140,10 +140,21 @@ cfg_if::cfg_if! {
         /// Calculate clock divisor. Returns a SDMMC_CK less than or equal to
         /// `sdmmc_ck` in Hertz.
         ///
-        /// Returns `(clk_div, clk_f)`, where `clk_div` is the divisor register
+        /// Returns `(bypass, clk_div, clk_f)`, where `bypass` enables clock divisor bypass (only sdmmc_v1),
-        /// value and `clk_f` is the resulting new clock frequency.
+        /// `clk_div` is the divisor register value and `clk_f` is the resulting new clock frequency.
-        fn clk_div(ker_ck: Hertz, sdmmc_ck: u32) -> Result<(u8, Hertz), Error> {
+        fn clk_div(ker_ck: Hertz, sdmmc_ck: u32) -> Result<(bool, u8, Hertz), Error> {
-            let clk_div = match ker_ck.0 / sdmmc_ck {
+            // sdmmc_v1 maximum clock is 50 MHz
+            if sdmmc_ck > 50_000_000 {
+                return Err(Error::BadClock);
+            }
+
+            // bypass divisor
+            if ker_ck.0 <= sdmmc_ck {
+                return Ok((true, 0, ker_ck));
+            }
+
+            // `ker_ck / sdmmc_ck` rounded up
+            let clk_div = match (ker_ck.0 + sdmmc_ck - 1) / sdmmc_ck {
                 0 | 1 => Ok(0),
                 x @ 2..=258 => {
                     Ok((x - 2) as u8)
@@ -153,22 +164,24 @@ cfg_if::cfg_if! {
 
             // SDIO_CK frequency = SDIOCLK / [CLKDIV + 2]
             let clk_f = Hertz(ker_ck.0 / (clk_div as u32 + 2));
-            Ok((clk_div, clk_f))
+            Ok((false, clk_div, clk_f))
         }
     } else if #[cfg(sdmmc_v2)] {
         /// Calculate clock divisor. Returns a SDMMC_CK less than or equal to
         /// `sdmmc_ck` in Hertz.
         ///
-        /// Returns `(clk_div, clk_f)`, where `clk_div` is the divisor register
+        /// Returns `(bypass, clk_div, clk_f)`, where `bypass` enables clock divisor bypass (only sdmmc_v1),
-        /// value and `clk_f` is the resulting new clock frequency.
+        /// `clk_div` is the divisor register value and `clk_f` is the resulting new clock frequency.
-        fn clk_div(ker_ck: Hertz, sdmmc_ck: u32) -> Result<(u16, Hertz), Error> {
+        fn clk_div(ker_ck: Hertz, sdmmc_ck: u32) -> Result<(bool, u16, Hertz), Error> {
+            // `ker_ck / sdmmc_ck` rounded up
             match (ker_ck.0 + sdmmc_ck - 1) / sdmmc_ck {
-                0 | 1 => Ok((0, ker_ck)),
+                0 | 1 => Ok((false, 0, ker_ck)),
                 x @ 2..=2046 => {
+                    // SDMMC_CK frequency = SDMMCCLK / [CLKDIV + 2]
                     let clk_div = ((x + 1) / 2) as u16;
                     let clk = Hertz(ker_ck.0 / (clk_div as u32 * 2));
 
-                    Ok((clk_div, clk))
+                    Ok((false, clk_div, clk))
                 }
                 _ => Err(Error::BadClock),
             }
@@ -478,7 +491,7 @@ impl<'d, T: Instance, Dma: SdmmcDma<T>> Sdmmc<'d, T, Dma> {
                 bus_width,
                 &mut self.card,
                 &mut self.signalling,
-                T::frequency(),
+                T::kernel_clk(),
                 &mut self.clock,
                 T::state(),
                 self.config.data_transfer_timeout,
@@ -625,7 +638,7 @@ impl SdmmcInner {
         unsafe {
             // While the SD/SDIO card or eMMC is in identification mode,
             // the SDMMC_CK frequency must be no more than 400 kHz.
-            let (clkdiv, init_clock) = unwrap!(clk_div(ker_ck, SD_INIT_FREQ.0));
+            let (_bypass, clkdiv, init_clock) = unwrap!(clk_div(ker_ck, SD_INIT_FREQ.0));
             *clock = init_clock;
 
             // CPSMACT and DPSMACT must be 0 to set WIDBUS
@@ -634,6 +647,8 @@ impl SdmmcInner {
             regs.clkcr().modify(|w| {
                 w.set_widbus(0);
                 w.set_clkdiv(clkdiv);
+                #[cfg(sdmmc_v1)]
+                w.set_bypass(_bypass);
             });
 
             regs.power().modify(|w| w.set_pwrctrl(PowerCtrl::On as u8));
@@ -1052,7 +1067,8 @@ impl SdmmcInner {
             _ => panic!("Invalid Bus Width"),
         };
 
-        let (clkdiv, new_clock) = clk_div(ker_ck, freq)?;
+        let (_bypass, clkdiv, new_clock) = clk_div(ker_ck, freq)?;
+
         // Enforce AHB and SDMMC_CK clock relation. See RM0433 Rev 7
         // Section 55.5.8
         let sdmmc_bus_bandwidth = new_clock.0 * width_u32;
@@ -1063,7 +1079,11 @@ impl SdmmcInner {
         unsafe {
             // CPSMACT and DPSMACT must be 0 to set CLKDIV
             self.wait_idle();
-            regs.clkcr().modify(|w| w.set_clkdiv(clkdiv));
+            regs.clkcr().modify(|w| {
+                w.set_clkdiv(clkdiv);
+                #[cfg(sdmmc_v1)]
+                w.set_bypass(_bypass);
+            });
         }
 
         Ok(())
@@ -1152,7 +1172,6 @@ impl SdmmcInner {
     }
 
     /// Query the card status (CMD13, returns R1)
-    ///
     fn read_status(&self, card: &Card) -> Result<CardStatus, Error> {
         let regs = self.0;
         let rca = card.rca;
@@ -1523,6 +1542,7 @@ pub(crate) mod sealed {
 
         fn inner() -> SdmmcInner;
         fn state() -> &'static AtomicWaker;
+        fn kernel_clk() -> Hertz;
     }
 
     pub trait Pins<T: Instance> {}
@@ -1550,6 +1570,61 @@ cfg_if::cfg_if! {
     }
 }
 
+cfg_if::cfg_if! {
+    // TODO, these could not be implemented, because required clocks are not exposed in RCC:
+    // - H7 uses pll1_q_ck or pll2_r_ck depending on SDMMCSEL
+    // - L1 uses pll48
+    // - L4 uses clk48(pll48)
+    // - L4+, L5, U5 uses clk48(pll48) or PLLSAI3CLK(PLLP) depending on SDMMCSEL
+    if #[cfg(stm32f1)] {
+        // F1 uses AHB1(HCLK), which is correct in PAC
+        macro_rules! kernel_clk {
+            ($inst:ident) => {
+                <peripherals::$inst as crate::rcc::sealed::RccPeripheral>::frequency()
+            }
+        }
+    } else if #[cfg(any(stm32f2, stm32f4))] {
+        // F2, F4 always use pll48
+        macro_rules! kernel_clk {
+            ($inst:ident) => {
+                critical_section::with(|_| unsafe {
+                    crate::rcc::get_freqs().pll48
+                }).expect("PLL48 is required for SDIO")
+            }
+        }
+    } else if #[cfg(stm32f7)] {
+        macro_rules! kernel_clk {
+            (SDMMC1) => {
+                critical_section::with(|_| unsafe {
+                    let sdmmcsel = crate::pac::RCC.dckcfgr2().read().sdmmc1sel();
+                    if sdmmcsel == crate::pac::rcc::vals::Sdmmcsel::SYSCLK {
+                        crate::rcc::get_freqs().sys
+                    } else {
+                        crate::rcc::get_freqs().pll48.expect("PLL48 is required for SDMMC")
+                    }
+                })
+            };
+            (SDMMC2) => {
+                critical_section::with(|_| unsafe {
+                    let sdmmcsel = crate::pac::RCC.dckcfgr2().read().sdmmc2sel();
+                    if sdmmcsel == crate::pac::rcc::vals::Sdmmcsel::SYSCLK {
+                        crate::rcc::get_freqs().sys
+                    } else {
+                        crate::rcc::get_freqs().pll48.expect("PLL48 is required for SDMMC")
+                    }
+                })
+            };
+        }
+    } else {
+        // Use default peripheral clock and hope it works
+        macro_rules! kernel_clk {
+            ($inst:ident) => {
+                <peripherals::$inst as crate::rcc::sealed::RccPeripheral>::frequency()
+            }
+        }
+    }
+}
+
 foreach_peripheral!(
     (sdmmc, $inst:ident) => {
         impl sealed::Instance for peripherals::$inst {
@@ -1564,6 +1639,10 @@ foreach_peripheral!(
                 static WAKER: ::embassy_sync::waitqueue::AtomicWaker = ::embassy_sync::waitqueue::AtomicWaker::new();
                 &WAKER
             }
+
+            fn kernel_clk() -> Hertz {
+                kernel_clk!($inst)
+            }
         }
 
         impl Instance for peripherals::$inst {}

examples/stm32f4/src/bin/sdmmc.rs

@@ -17,6 +17,7 @@ const ALLOW_WRITES: bool = false;
 async fn main(_spawner: Spawner) -> ! {
     let mut config = Config::default();
     config.rcc.sys_ck = Some(mhz(48));
+    config.rcc.pll48 = true;
     let p = embassy_stm32::init(config);
     info!("Hello World!");
 
@@ -38,7 +39,7 @@ async fn main(_spawner: Spawner) -> ! {
     // Should print 400kHz for initialization
     info!("Configured clock: {}", sdmmc.clock().0);
 
-    unwrap!(sdmmc.init_card(mhz(24)).await);
+    unwrap!(sdmmc.init_card(mhz(48)).await);
 
     let card = unwrap!(sdmmc.card());
 

examples/stm32f7/src/bin/sdmmc.rs

@@ -13,6 +13,7 @@ use {defmt_rtt as _, panic_probe as _};
 async fn main(_spawner: Spawner) -> ! {
     let mut config = Config::default();
     config.rcc.sys_ck = Some(mhz(200));
+    config.rcc.pll48 = true;
     let p = embassy_stm32::init(config);
 
     info!("Hello World!");