stm32f3xx_hal::rcc::CFGR::freeze() gives wrong default clocks: it gives half of the real clocks

[zhenkyle]

Today when I'm doing a serial project with the usart1 hardware on my f3discovery board, to get the default clock configuration by the following code:

let clocks = rcc.cfgr.freeze(&mut flash.acr);

I can't get anything over serial line, after I debuged the hardward by a logic analyzer, I found out the actural bps of the usart1 hardware is twice as much as the intended bps value.

gdb give this clue, showing the hardware is runing on 4MHz sysclk:

(gdb) print clocks
$1 = stm32f3xx_hal::rcc::Clocks {hclk: stm32f3xx_hal::time::Hertz (4000000), pclk1: stm32f3xx_hal::time::Hertz (4000000), pclk2: stm32f3xx_hal::time::Hertz (4000000), ppre1: 1, ppre2: 1, sysclk: stm32f3xx_hal::time::Hertz (4000000), usbclk_valid: false}

which is curious, because I was told the default sysclk is 8MHz. If the default clock configuration gives half of the real clocks, it can explain the double of bps in usart1.

Freeze the clock configuration with 8MHz sysclk manually:

let clocks = rcc.cfgr.sysclk(8.mhz()).freeze(&mut flash.acr);

everythings works fine.

Switching to stm32f30x-hal crate and freeze with default clock configuration again, everything works fine too. And gdb gives me 8MHz sysclk.

The freeze() code in rcc.rs is quite complex, I haven't figure it out yet. but I think there's some thing wrong with PLLMUL setting, in the reference manual, the PLL multiplication factor can never be 1x.

[teskje]

AFAICT the issue is that (at least on some devices*) the HSI clock is divided by 2 when used as PLLSRC, but when the HSI is used as the system clock directly (i.e. PLL is not enabled), it is not devided.

In rcc.rs, the code that decides if the PLL is used and what the final SYSCLK will be is in calc_sysclk and calc_pll. Here is a simplified version of that logic (ignoring the HSE and PLL options selection):

const HSI: u32 = 8_000_000;

fn calc_sysclk(&self) -> (u32, Option<...>) {
    let pllsrcclk = HSI / 2;
    let pllmul = self.sysclk.unwrap_or(pllsrcclk) / pllsrcclk;

    if pllmul == 1 {
        return (pllsrcclk, None);
    }

    // calculate PLL options
    ...
    (sysclk, Some(...))
}

So if self.sysclk was not set or is equal to HSI / 2 (4 MHz), we decide to not use the PLL and instead use the HSI directly. There lies the problem: When the HSI is used directly, it is not divided, so we should return HSI (8 MHz), not HSI / 2.

There is the issue that, if we implement this fix, users requesting a 4 MHz sysclk will get a system configured to 8 MHz instead. Not sure if this is really a problem though, since:

• The clock configuration is already best-effort, i.e. we try to be as close as possible to what the user requested, but the final clocks don't always match exactly.
• AFAICT there is no way to set the SYSCLK to something lower than 8 MHz (at least on the STM32F303 devices).

So getting a SYSCLK of 8 MHz when 4 MHz was requested is actually what I'd expect anyway.

---

* Looking at the clock tree of the STM32F303xDxE and SMT32F303xE devices it seems like for those the HSI clock is not divided when used as PLLSRC, the full 8 MHz are used. Our code currently always assumes HSI / 2 for the PLLSRC, regardless of device. Is this another bug?

[Sh3Rm4n]

Thank you for your detailed comment @ra-kete!

Regarding the first point, maybe we shouldn't allow frequencies below 8 Mhz. This could be solved via unimplemented! which should panic at runtime. Then, the clock configuration wouldn't implicitly "fix" the wrong configuration and confuses the user.

Regarding point 2:

Looking at the code, we even special case the calc_pll for the devices, which do not implement the /2 divider for the HSI, but still do a division.

https://github.com/stm32-rs/stm32f3xx-hal/blob/403643c68824e635c25585ae3bec8c47f27d26fd/src/rcc.rs#L211-L218

This should be an easy fix.