Imprecise PLL frequency calculation

[t123yh]

Consider this example for STM32F407:

    let mut config = Config::default();
    {
        config.rcc.hse = Some(Hse {
            freq: Hertz(25_000_000),
            mode: HseMode::Oscillator,
        });
        config.rcc.pll_src = PllSource::HSE;
        config.rcc.pll = Some(Pll {
            prediv: PllPreDiv::DIV15,
            mul: PllMul::MUL192,
            divp: Some(PllPDiv::DIV2),
            divq: None,
            divr: None,
        });
        config.rcc.ahb_pre = AHBPrescaler::DIV1;
        config.rcc.apb1_pre = APBPrescaler::DIV4;
        config.rcc.apb2_pre = APBPrescaler::DIV2;
        config.rcc.sys = Sysclk::PLL1_P;
    }

    let p = embassy_stm32::init(config);

This first divides input frequency (25MHz) by 15, multipliy it by 192, then divide it by 2. This would result in an actual sysclk of 25000000/15*192/2=160000000 (160MHz), which is a nice-looking frequency.

However, the PLL code in f4's rcc first divides the frequency, then multiplies. This would make sysclk = Hertz(159999936), rather than 160MHz, due to rounding issue (because 25 is not divisible by 15).

We cannot simply change the code to multiply first and then divide, because 25000000*192=4800000000, which is out of bound for u32 (Hertz).

The impact of this is somewhat huge. If you do something like this:

    let mut can1 = Can::new(p.CAN1, p.PD0, p.PD1, Irqs);

    can1.modify_filters()
        .enable_bank(0, Fifo::Fifo0, Mask32::accept_all());

    can1.modify_config()
        .set_loopback(false) // Receive own frames
        .set_silent(false)
        .set_bitrate(500_000);

set_bitrate will panic, because it considers APB1 to have 39999984Hz, thus cannot have 500000Hz baud rate.

[t123yh]

I think there might be 3 possible things here:

1. Most obviously, fix the PLL frequency calculation code, possibly by using u64;
2. Warn user about rounding issues when calculating frequency;
3. Make the peripherals tolerant the imprecise frequency by setting an error margin.

[Dirbaio]

I'd try using u64 and check if code size increases too much or not. I think it shouldn't, if you use LTO then RCC code usually gets optimized very well because the input is all compile-time-known constants.

I think in some cases we might want some error margin in the drivers anyway? even with u64 you can end up in situations where freq is something like 39999999Hz maybe?

[t123yh]

I'd try using u64 and check if code size increases too much or not. I think it shouldn't, if you use LTO then RCC code usually gets optimized very well because the input is all compile-time-known constants.

Yes, agree. Maybe I can try to implement it. Although it may cause the code to look slightly uglier.

Maybe Hertz should be changed to Hertz<T> where T can be u64? Or, have Hertz and BigHertz and make it possible to convert between?

I think in some cases we might want some error margin in the drivers anyway? even with u64 you can end up in situations where freq is something like 39999999Hz maybe?

Yes, agree

[Dirbaio]

Hertz is only to make the public api typesafe, it's fine to internally just use u32/u64. I wouldn't do BigHertz or Hertz<u64> since it doesn't appear in the public api at all.