From 7d07f1b791e4101f886565b8abc3eaf051e3ec19 Mon Sep 17 00:00:00 2001 From: Tethys Svensson Date: Fri, 28 Mar 2025 17:18:34 +0100 Subject: [PATCH] Fix off-by-one in the allowed range of the spi clock calculations (#3266) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 --- esp-hal/CHANGELOG.md | 1 + esp-hal/src/spi/master.rs | 47 +++++++++++++++++-------------- hil-test/tests/spi_full_duplex.rs | 35 +++++++++++++++++++++++ 3 files changed, 62 insertions(+), 21 deletions(-) diff --git a/esp-hal/CHANGELOG.md b/esp-hal/CHANGELOG.md index 314c19050..9e3b9a0e8 100644 --- a/esp-hal/CHANGELOG.md +++ b/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 diff --git a/esp-hal/src/spi/master.rs b/esp-hal/src/spi/master.rs index b1379b3bf..6b1fa0a96 100644 --- a/esp-hal/src/spi/master.rs +++ b/esp-hal/src/spi/master.rs @@ -498,18 +498,23 @@ impl Config { self } - fn recalculate(&self) -> Result { - // taken from https://github.com/apache/incubator-nuttx/blob/8267a7618629838231256edfa666e44b5313348e/arch/risc-v/src/esp32c3/esp32c3_spi.c#L496 - + fn clock_source_freq_hz(&self) -> Rate { 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 { + // 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 raw_apb_freq = apb_clk_freq.as_hz() as i32; + let target_freq_hz = self.frequency.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! { - if #[cfg(esp32h2)] { - if self.frequency < Rate::from_khz(70) || self.frequency > Rate::from_mhz(48) { - return Err(ConfigError::UnsupportedFrequency); - } - } else { - if self.frequency < Rate::from_khz(70) || self.frequency > Rate::from_mhz(80) { - return Err(ConfigError::UnsupportedFrequency); - } - } + let source_freq = self.clock_source_freq_hz(); + let min_divider = 1; + // FIXME: while ESP32 and S2 support pre dividers as large as 8192, + // those values are not currently supported by the divider calculation. + let max_divider = 16 * 64; // n * pre + + if self.frequency < source_freq / max_divider || self.frequency > source_freq / min_divider + { + return Err(ConfigError::UnsupportedFrequency); } + Ok(()) } } diff --git a/hil-test/tests/spi_full_duplex.rs b/hil-test/tests/spi_full_duplex.rs index cc80d89e0..a2cd6bdf1 100644 --- a/hil-test/tests/spi_full_duplex.rs +++ b/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); + } + } }