Fix off-by-one in the allowed range of the spi clock calculations (#3266)

* Fix off-by-one in the allowed range of the spi clock calculations

* Update CHANGELOG

* Add test to verify calculation

* Use actual frequency limits in validation

* Rename variables

---------

Co-authored-by: Dániel Buga <bugadani@gmail.com>

esp-hal/CHANGELOG.md

@@ -38,6 +38,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - ESP32/ESP32-S2: Avoid running into timeouts with reads/writes larger than the FIFO (#3199)
 - ESP32: Enforce required pointer alignments in DMA buffers (#3296)
 - ESP32-C6: Keep ADC enabled to improve radio signal strength (#3249)
+- Fix off-by-one in the allowed range of the spi clock calculations (#3266)
 
 ### Removed
 

esp-hal/src/spi/master.rs

@@ -498,18 +498,23 @@ impl Config {
         self
     }
 
-    fn recalculate(&self) -> Result<u32, ConfigError> {
+    fn clock_source_freq_hz(&self) -> Rate {
-        // taken from https://github.com/apache/incubator-nuttx/blob/8267a7618629838231256edfa666e44b5313348e/arch/risc-v/src/esp32c3/esp32c3_spi.c#L496
-
         let clocks = Clocks::get();
+
+        // FIXME: take clock source into account
         cfg_if::cfg_if! {
             if #[cfg(esp32h2)] {
                 // ESP32-H2 is using PLL_48M_CLK source instead of APB_CLK
-                let apb_clk_freq = clocks.pll_48m_clock;
+                clocks.pll_48m_clock
             } else {
-                let apb_clk_freq = clocks.apb_clock;
+                clocks.apb_clock
             }
         }
+    }
+
+    fn recalculate(&self) -> Result<u32, ConfigError> {
+        // taken from https://github.com/apache/incubator-nuttx/blob/8267a7618629838231256edfa666e44b5313348e/arch/risc-v/src/esp32c3/esp32c3_spi.c#L496
+        let source_freq = self.clock_source_freq_hz();
 
         let reg_val: u32;
         let duty_cycle = 128;
@@ -517,7 +522,7 @@ impl Config {
         // In HW, n, h and l fields range from 1 to 64, pre ranges from 1 to 8K.
         // The value written to register is one lower than the used value.
 
-        if self.frequency > ((apb_clk_freq / 4) * 3) {
+        if self.frequency > ((source_freq / 4) * 3) {
             // Using APB frequency directly will give us the best result here.
             reg_val = 1 << 31;
         } else {
@@ -534,17 +539,17 @@ impl Config {
             let mut besterr: i32 = 0;
             let mut errval: i32;
 
-            let raw_freq = self.frequency.as_hz() as i32;
+            let target_freq_hz = self.frequency.as_hz() as i32;
-            let raw_apb_freq = apb_clk_freq.as_hz() as i32;
+            let source_freq_hz = source_freq.as_hz() as i32;
 
             // Start at n = 2. We need to be able to set h/l so we have at least
             // one high and one low pulse.
 
-            for n in 2..64 {
+            for n in 2..=64 {
                 // Effectively, this does:
                 // pre = round((APB_CLK_FREQ / n) / frequency)
 
-                pre = ((raw_apb_freq / n) + (raw_freq / 2)) / raw_freq;
+                pre = ((source_freq_hz / n) + (target_freq_hz / 2)) / target_freq_hz;
 
                 if pre <= 0 {
                     pre = 1;
@@ -554,7 +559,7 @@ impl Config {
                     pre = 16;
                 }
 
-                errval = (raw_apb_freq / (pre * n) - raw_freq).abs();
+                errval = (source_freq_hz / (pre * n) - target_freq_hz).abs();
                 if bestn == -1 || errval <= besterr {
                     besterr = errval;
                     bestn = n;
@@ -588,17 +593,17 @@ impl Config {
     }
 
     fn validate(&self) -> Result<(), ConfigError> {
-        cfg_if::cfg_if! {
+        let source_freq = self.clock_source_freq_hz();
-            if #[cfg(esp32h2)] {
+        let min_divider = 1;
-                if self.frequency < Rate::from_khz(70) || self.frequency > Rate::from_mhz(48) {
+        // FIXME: while ESP32 and S2 support pre dividers as large as 8192,
-                    return Err(ConfigError::UnsupportedFrequency);
+        // those values are not currently supported by the divider calculation.
-                }
+        let max_divider = 16 * 64; // n * pre
-            } else {
+
-                if self.frequency < Rate::from_khz(70) || self.frequency > Rate::from_mhz(80) {
+        if self.frequency < source_freq / max_divider || self.frequency > source_freq / min_divider
-                    return Err(ConfigError::UnsupportedFrequency);
+        {
-                }
+            return Err(ConfigError::UnsupportedFrequency);
-            }
         }
+
         Ok(())
     }
 }

hil-test/tests/spi_full_duplex.rs

@@ -14,6 +14,7 @@ use embedded_hal_async::spi::SpiBus as SpiBusAsync;
 use esp_hal::{
     gpio::Input,
     peripheral::Peripheral,
+    peripherals::SPI2,
     spi::master::{Config, Spi},
     time::Rate,
     Blocking,
@@ -887,4 +888,38 @@ mod tests {
 
         assert_eq!(tx_buf, rx_buf);
     }
+
+    #[test]
+    #[cfg(feature = "unstable")] // Needed for register access
+    fn test_clock_calculation_accuracy(mut ctx: Context) {
+        let lowest = if cfg!(esp32h2) { 78048 } else { 78125 };
+
+        let f_mst = if cfg!(esp32c2) {
+            40_000_000
+        } else if cfg!(esp32h2) {
+            48_000_000
+        } else {
+            80_000_000
+        };
+        let inputs = [lowest, 100_000, 1_000_000, f_mst];
+        let expected_outputs = [lowest, 100_000, 1_000_000, f_mst];
+
+        for (input, expectation) in inputs.into_iter().zip(expected_outputs.into_iter()) {
+            ctx.spi
+                .apply_config(&Config::default().with_frequency(Rate::from_hz(input)))
+                .unwrap();
+
+            // Read back effective SCLK
+            let spi2 = unsafe { SPI2::steal() };
+
+            let clock = spi2.register_block().clock().read();
+
+            let n = clock.clkcnt_n().bits() as u32;
+            let pre = clock.clkdiv_pre().bits() as u32;
+
+            let actual = f_mst / ((n + 1) * (pre + 1));
+
+            assert_eq!(actual, expectation);
+        }
+    }
 }