Added support for HSE, LSE, different PLL sources, PLL calibration of MSI, and some more improvements

[korken89]

Hi, I did some refactoring of the RCC module to better support other clock and PLL configs.
We are using the STM32L412CB in our new product, so I will most likely come with a lot of PRs as I develop the codebase for the product. :)

Usage example, where the MSI is used (with PLL calibration and as source to system PLL):

    let mut flash = dp.FLASH.constrain();
    let rcc = dp.RCC.constrain();

    let clocks = rcc
        .cfgr
        .lse(CrystalBypass::Disable)
        .msi(MsiFreq::RANGE4M)
        .sysclk(80.mhz())
        .pclk1(80.mhz())
        .pclk2(80.mhz())
        .freeze(&mut flash.acr);

All the best!

Edit: Tested on Nucleo-L412RB

[MathiasKoch]

LGTM 👍

Awesome work, and nice to see someone else dedicating their work to this HAL due to professional needs.
We too are using it in a product, and has & will as such contribute a lot.

[korken89]

Awesome!

Maybe hold on merging this for a little bit though, I need to fix the documentation for this (which I will do tomorrow).

[MabezDev]

Thanks for the PR, it's looking very nice! No complaints here :)

Please do ping me when you think its ready to merge.

[korken89]

I did some more testing and the generated PLL was not correct, I updated the PLL calculation and added sanity checks.

Got the codesize I expected now as well. Optimized ("z") build now generates 140 bytes of code for setting up the clock with the following config:

    let clocks = rcc
        .cfgr
        .lse(CrystalBypass::Disable)
        .msi(MsiFreq::RANGE4M)
        .sysclk(80.mhz())
        .pclk1(80.mhz())
        .pclk2(80.mhz())
        .freeze(&mut flash.acr);

So it seems all the added checks do not impact code size after constant propagation eliminates all asserts. :)

[korken89]

@MabezDev / @MathiasKoch do any of you have a card with LSE?
I'd like someone else to give the LSE code a try. It seems to work sporadically for me but seems correct from the datasheet, and the STLink/V2 and me are having issues so I am not sure if it's the code or the debugger.

If you have a card with LSE, here is an example code that should arrive to the hprintln:

#![no_main]
#![no_std]

use cortex_m_rt::entry;
use cortex_m_semihosting::hprintln;
use hal::{
    prelude::*,
    rcc::{CrystalBypass, MsiFreq},
};
use panic_semihosting as _;
use stm32l4xx_hal as hal;

#[entry]
fn main() -> ! {
    let dp = unsafe { hal::stm32::Peripherals::steal() };

    let mut flash = dp.FLASH.constrain();
    let rcc = dp.RCC.constrain();

    let clocks = rcc
        .cfgr
        .lse(CrystalBypass::Disable)
        .msi(MsiFreq::RANGE4M)
        .sysclk(80.mhz())
        .pclk1(80.mhz())
        .pclk2(80.mhz())
        .freeze(&mut flash.acr);

    hprintln!("Start with clocks: {:#?}", clocks).ok();

    loop {
        continue;
    }
}

I'll investigate more.

[MathiasKoch]

Yep, i can try to give it a whack when i get the time.

One thing we discovered is that it seems like the PLLConfig is a bit ambiguous.. Using this code it seems like it expects m, n & r to be bits (eg, 0b0).

#[derive(Clone, Copy, Debug)]
/// Pll Configuration - Calculation = ((SourceClk / m) * n) / r
pub struct PllConfig {
    /// Main PLL Division factor
    pub m: u8,
    /// Main Pll Multiplication factor
    pub n: u8,
    /// Main PLL division factor for PLLCLK (system clock)
    pub r: u8,
}

Maybe we should consider entering it as divisors, eg 2, 4, 6, 8 instead of 0b0, 0b01 etc.? That would make ((SourceClk / m) * n) / r much easier to use.

[korken89]

Yeah, in a previous I used enums to represent the divisors as they are difficult to understand for a normal user.
I will update to that! :)

[MathiasKoch]

If you are up for it, adding Clock Security System (CSS) support would be awesome!
And also it seems like the PLLSAIx configuration parts are still missing?

That would give an almost finished/complete RCC implementation to the HAL.

[unizippro]

I gave the LSE code a try. I added .lse(CrystalBypass::Disable) to my existing config. But it seems to hang at in the rcc.rs line 457 while rcc.bdcr.read().lserdy().bit_is_clear() {} since removing this line caused the program to continue correctly.

[korken89]

@MathiasKoch I can add that as well! A complete setup would be great.

@unizippro Thanks for testing! Then I know I have a bug with enabling the LSE, I will investigate more.

Edit: I was not disabling the write protection of the backup domain, this is why LSE is failing

[MabezDev]

I did some more testing and the generated PLL was not correct, I updated the PLL calculation and added sanity checks.

Not entirely surprised.. I threw that together super quickly as I was limited on time, thanks for fixing it!

One really nice thing the f4xx hal does, it actually derives all the pll divisors based on the the target clock speeds, which is really nice; not that I expect it to be done in this PR.

I'll pull out the project I was originally using this for (hopefully tonight, if not the weekend) and take these changes out for a spin.

[korken89]

Not entirely surprised.. I threw that together super quickly as I was limited on time, thanks for fixing it!

No problem!

I'll pull out the project I was originally using this for (hopefully tonight, if not the weekend) and take these changes out for a spin.

Awesome, I hope it works well!

One really nice thing the f4xx hal does, it actually derives all the pll divisors based on the the target clock speeds, which is really nice; not that I expect it to be done in this PR.

I did use the f4xx one, the issue is that it sometimes (often) does not optimize well and you have 4 kB of flash being used for the clock setup. Not sure if this is fixed yet.
But it should not be an issue to make the "best effort PLL config" there is now better, we should only be careful so it properly optimizes still. :) Probably by making as much as possible const fn.

[MabezDev]

the issue is that it sometimes (often) does not optimize well and you have 4 kB of flash being used for the clock setup

Good point, I still don't think this is fully fixed, AFAIK it seems to change between compiler version (or perhaps more likely, llvm version). I think the manual solution is absolutely fine as clocks aren't something that is changed all that often.

[korken89]

@MathiasKoch I took a look at the PLLSAIx, the board I have (STM32L412) does not have this so I am unable to give it a try unfortunately.

[MathiasKoch]

@korken89
Alright, no problem! We can add the PLLSAIx config parts behind the relevant device feature flags as soon as this is merged 👍

[korken89]

I think this is close to ready. I have not tested the CSS yet, I do not have a way to break the connection to the crystal on the Nucleo board to test it.

Does anyone of you have a way to test it?

[MathiasKoch]

@korken89
Unfortunately i don't have any way of testing it either.. Guess we might just have to assume that it works, until otherwise proven..

Great job otherwise on this! 👍

@MabezDev
Can we merge this, so we can add the remaining PLLSAIx config needed for our ADC PR?

[MabezDev]

@korken89 Got around to testing in my project; unfortunately, it this PR seems to break it. In my project, I don't end up using any PLL stuff, just the internal HSI and enable the LSI.

https://github.com/MWatch/kernel/blob/master/src/main.rs#L125

Seems like this PR makes the assumption of always using the PLL?

[korken89]

Aah I see the issue, thanks for testing and I'll fix!

[korken89]

@MabezDev This should now be fixed!

---

@MabezDev / @MathiasKoch One question, the current implementation assumes that HSI 16 MHz is default, but from the HW the MSI 4 MHz is default. Should we change to use the MSI @ 4 MHz as the default to follow the chip defaults?

[MathiasKoch]

@korken89
Looking at the reference manual it appears you are correct..
With regards to the default impl. of the HAL, i have no hard feelings though.
I can follow the logic of defaulting it to the actual chip factory defaults, but I won't get hurt if @MabezDev does not agree, as it would probably break quite a few applications out there, if they haven't explicitly set their msi range 😄

[korken89]

For now I added code to disable auto-started clocks, then we are not paying for having MSI and HSI running.

[MathiasKoch]

Cool 👍

[korken89]

Now we should be on the safe, I also added an output in the Clock structure to indicate where the PLL is fed from.

Now it should be ready for a proper review :)

[MathiasKoch]

One minor thing.

Given the config:

let clocks = rcc
            .cfgr
            // .hsi48(true)
            .lse(CrystalBypass::Disable, ClockSecuritySystem::Enable)
            .hse(8.mhz(), CrystalBypass::Disable, ClockSecuritySystem::Enable)
            .sysclk_with_pll(80.mhz(), PllConfig::new(1, 20, PllDivider::Div2))
            .pll_source(PllSource::HSE)

            // Temp fix unti PLLSAI1 is implemented
            .msi(MsiFreq::RANGE48M)
            .hclk(80.mhz())
            .pclk1(80.mhz())
            .pclk2(80.mhz())
            .freeze(&mut flash.acr, &mut pwr);

clocks.lsi ends up being false, cause it is not explicitly enabled in the config, but it is actually on due to the ClockSecuritySystem.. Maybe that should be reflected in the final object?

[korken89]

@MathiasKoch The last commit should fix it!

[MathiasKoch]

Yup 👍
LGTM

[MabezDev]

Tested this again, all working! Thanks for the PR, definitely a great improvement!