Clock speed sometimes incorrect

[chrissuozzo]

Bug description

The call to Clocks::configure() during esp_hal::init() will only set the CPU clock speed to non-default values. This is fine most of the time because the second-stage bootloader will usually set the CPU to the default rate... except in the following cases:

Case 1

There is a CPU0 reset. In this case, the second-stage bootloader doesn't configure any of the clocks, so the app will inherit whatever CPU speed was previously set. During an OTA update where you're going from an app running at a non-default rate, to one running at the default rate, and you use a CPU0 reset to initiate the update, the newer version of the app will be running at the wrong rate. This has been verified by making a few small changes to the OTA update example, by running it at max() rate, and initiating the update via software_reset_cpu(0):

--- a/examples/ota/update/src/main.rs
+++ b/examples/ota/update/src/main.rs
@@ -39,6 +39,8 @@ use esp_backtrace as _;
 use esp_hal::{
     gpio::{Input, InputConfig, Pull},
     main,
+    rom::software_reset_cpu,
+    time::{Duration, Instant},
 };
 use esp_println::println;
 use esp_storage::FlashStorage;
@@ -50,7 +52,8 @@ static OTA_IMAGE: &[u8] = include_bytes!("../../../target/ota_image");
 #[main]
 fn main() -> ! {
     esp_println::logger::init_logger_from_env();
-    let peripherals = esp_hal::init(esp_hal::Config::default());
+    let peripherals =
+        esp_hal::init(esp_hal::Config::default().with_cpu_clock(esp_hal::clock::CpuClock::max()));

     let mut flash = FlashStorage::new(peripherals.FLASH);

@@ -122,6 +125,10 @@ fn main() -> ! {
             ota.activate_next_partition().unwrap();
             ota.set_current_ota_state(esp_bootloader_esp_idf::ota::OtaImageState::New)
                 .unwrap();
+            println!("Triggering software reset...");
+            let delay_start = Instant::now();
+            while delay_start.elapsed() < Duration::from_secs(2) {}
+            software_reset_cpu(0);
         }
     }
 }

And making an OTA image from esp-generate for, say the ESP32C6, run at the default rate:

#[main]
fn main() -> ! {
    // generator version: 1.0.0
    esp_println::logger::init_logger_from_env();
    let clock = CpuClock::default();
    let config = esp_hal::Config::default().with_cpu_clock(clock);
    let peripherals = esp_hal::init(config);

    let div = peripherals
        .PCR
        .register_block()
        .cpu_freq_conf()
        .read()
        .cpu_hs_div_num()
        .bits();
    let freq = match div {
        0 => "160MHz",
        1 => "80Mhz",
        _ => "INVALID",
    };
    let reason = rtc_get_reset_reason(0);
    info!("Set {clock:?}, got {freq}; (div = {div}, reason = {reason})");

    loop {}
}

Abbreviated output:

I (368) esp_image: segment 3: paddr=00117fb4 vaddr=40800014 size=00ebch (  3772) load
I (384) boot: Loaded app from partition at offset 0x110000
I (398) boot: Disabling RNG early entropy source...
INFO - Set _80MHz, got 160MHz; (div = 0, reason = 12)

Case 2

You're using the esp-hal in a second-stage bootloader. In this case, esp_hal::init() does an excellent job of doing all the same low-level clock configuration the IDF bootloader normally does, except when it comes to the CPU clock. In this case we really do want it to run at the default rate, as that's what the app is expecting, but we can't set it using the HAL. I'm not 100% sure because the code is not open-source, but these comments make it sound like the ROM bootloader doesn't do any clock configuration, meaning out of POR the CPU clock source would be the XTAL, which is what this second-stage bootloader would then pass to the app...

Expected behavior

I'd like to remove the assumption that the system is running at the default CPU speed within Clocks::configure(), and just always set it to the desired rate:

@@ -1267,21 +1267,17 @@ impl Clocks {
         let xtal_freq = Self::measure_xtal_frequency();

         let apb_freq;
-        if cpu_clock_speed != CpuClock::default() {
             if cpu_clock_speed.mhz() <= xtal_freq.mhz() {
                 apb_freq = ApbClock::ApbFreqOther(cpu_clock_speed.mhz());
                 clocks_ll::esp32c6_rtc_update_to_xtal(xtal_freq, 1);
                 clocks_ll::esp32c6_rtc_apb_freq_update(apb_freq);
             } else {
                 let pll_freq = PllClock::Pll480MHz;
                 apb_freq = ApbClock::ApbFreq80MHz;
                 clocks_ll::esp32c6_rtc_bbpll_enable();
                 clocks_ll::esp32c6_rtc_bbpll_configure(xtal_freq, pll_freq);
                 clocks_ll::esp32c6_rtc_freq_to_pll_mhz(cpu_clock_speed);
                 clocks_ll::esp32c6_rtc_apb_freq_update(apb_freq);
             }
-        } else {
-             apb_freq = ApbClock::ApbFreq80MHz;
-        }

If this sounds good to you, LMK and I'll get a PR ready for you.

Thanks for all the hard-work you all put into the esp-hal! (and the tooling!)

Environment

• Target device: (All; tested on ESP32C6)
• Crate name and version: esp-hal 1.0

[bugadani]

Thank you, I'll make sure to address this with the upcoming clock system rework.

[bugadani]

I just ran into this same issue on the C2. Your patch, while technically configures the CPU to the default clock speed, also makes one of the i2c tests fail on that chip - which is the same issue I'm facing with #4517 as well.