Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cannot calculate PLL for stm32f303xc with 8MHz HSE running at maximum speed (72MHz) #151

Closed
vgao1996 opened this issue Oct 12, 2020 · 4 comments · Fixed by #152
Closed
Assignees
Labels
bug Something isn't working

Comments

@vgao1996
Copy link

vgao1996 commented Oct 12, 2020

Hi, I was having trouble setting a 72MHz sysclk with an 8MHz HSE.

let _clocks = rcc
    .cfgr
    .use_hse(8.mhz())
    .sysclk(72.mhz())
    .hclk(72.mhz())
    .pclk1(36.mhz())
    .pclk2(72.mhz())
    .freeze(&mut flash.acr);

Through a little bit of debugging I found out fn calc_pll might not be deriving the correct divisor and multiplier for this configuration, causing an assertion to fail.

#[cfg(not(any(
        feature = "stm32f302xd",
        feature = "stm32f302xe",
        feature = "stm32f303xd",
        feature = "stm32f303xe",
        feature = "stm32f398"
    )))]
    fn calc_pll(&self, sysclk: u32) -> (u32, PllConfig) {
        let pllsrcclk = self.hse.unwrap_or(HSI / 2);
        // Get the optimal value for the pll divisor (PLL_DIV) and multiplier (PLL_MUL)
        // Only for HSE PLL_DIV can be changed
        let (pll_mul, pll_div): (u32, Option<u32>) = if self.hse.is_some() {
            // Get the optimal value for the pll divisor (PLL_DIV) and multiplier (PLL_MUL)
            // with the greatest common divisor calculation.
            let common_divisor = gcd(sysclk, pllsrcclk);
            let mut multiplier = sysclk / common_divisor;  // <<<<<<<< multiplier = 9
            let mut divisor = pllsrcclk / common_divisor;  // <<<<<<<< divisor = 1

            // Check if the multiplier can be represented by PLL_MUL
            // or if the divisor can be represented by PRE_DIV
            if multiplier == 1 || divisor == 1 {           // <<<<<<<< multiplier & divisor gets doubled
                // PLL_MUL minimal value is 2
                multiplier *= 2;
                // PRE_DIV minimal value is 2
                divisor *= 2;
            }

            // PLL_MUL maximal value is 16
            assert!(divisor <= 16);
            // PRE_DIV maximal value is 16
            assert!(multiplier <= 16);                     // <<<<<<<< since multiplier = 18, this assertion fails

            (multiplier, Some(divisor))
        }

For reference, here's a clock configuration generated by STM32CudeIDE:
Capture

Here are some of my guesses, but since I'm new to this project and embedded in general, please take what I say with a grain of salt:

  • Doubling the divisor and multiplier feel wrong to me.
    • For my particular case (8MHz HSE + 72MHz sysclk), the ratio 9/1 is perfectly representable yet calc_pll rejects it by making it 18/2. The comments say the minimal value for both PLL_MUL and PRE_DIV is 2, but STM32CudeIDE uses RCC_HSE_PREDIV_DIV1 and RCC_PLL_MUL9 in the code it generates and since PREDIV_A does have a DIV1 variant, should we try doubling only when the multiplier is 1?
    • For other configurations that currently work, it's still a little bit weird. For example, for an 8MHz HSE + 48MHz sysclk, calc_pll represents the ratio as 12/2 instead of 6/1.
  • I wonder if what we really need to check here is "whether the ratio can be represented as N/M where N and M are integers such that 2 <= N <= 16 and 1 <= M <= 16". Although it's unlikely this alone is sufficient for a valid clock configuration.

Anyway, I'd be happy to put up a PR if someone could confirm my guesses or point me in the right direction.

@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Oct 12, 2020

Hey it's me who implemented that logic. Thanks for the report.

I implemented the logic while referencing the data sheet

image RM0316 p.92

As seen in the clock tree, PLLSRC is divided by 2 at least, so with 8 MHz HSE Crystal, 72 MHz are indeed not reachable as you observed. For this case I implemented this check

stm32f3xx-hal/src/rcc.rs

Lines 373 to 378 in 54e8261

if multiplier == 1 || divisor == 1 {
// PLL_MUL minimal value is 2
multiplier *= 2;
// PRE_DIV minimal value is 2
divisor *= 2;
}

As you can see, for devices like the stm32f303xD, these constraints on the
divisor are not existing.

stm32f3xx-hal/src/rcc.rs

Lines 448 to 452 in 54e8261

if multiplier == 1 {
// PLL_MUL minimal value is 2
multiplier *= 2;
divisor *= 2;
}

I hope this logic is correct, but I only referenced the data sheet (which can be wrong sometimes).

Maybe STM32CubeIDE is right and this constrain on PLLDIV is really not an
issue, or you have selected a different MCU (303xD instead of 303xC).

In general the greatest-common-divisor function should generate the
correct fraction with the lowest possible values.

TBH, I've not tested it on hardware. Is the code, which you are
generating via STMCubeIDE working on your device?

@vgao1996
Copy link
Author

vgao1996 commented Oct 12, 2020

I didn't notice the datasheet was only listing /2, /3, .... Thanks for pointing this out!

or you have selected a different MCU (303xD instead of 303xC).

I double checked the MCU I'm using and it's indeed an STM32F303CCT6, so not a 303xB or 303xD

Is the code, which you are generating via STMCubeIDE working on your device?

Yes. I didn't measure the clock directly, but a few peripherals all worked fine:

  • SPI and USB worked (STMCude was using the /1.5 divisor for USB and if from my understanding, USB needs to work at 48MHz, which gives us back a sysclk of 72MHz)
  • I once used TIM3 to generate a PWM signal with a period of 65535 and was able to see a frequency of 1098Hz on my oscilloscope. If I'm understanding correctly this matches the theoretical value 72,000,000 / 65536 ~= 1098.6.
    I also tried deleting divisor == 1 and the a simple LED blinker written in rust ran without problems.
    I'll do some more tests/measurements later today to ensure the MCU is indeed running at 72MHz.

@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Oct 13, 2020

Yes. I didn't measure the clock directly, but a few peripherals all worked fine:

That is good to here. TBH it would be strange if STM wouldn't allow the chip to reach 72 MHz with it's internal crystal external crystal is used here.

But I don't really know how to go about this change.
Following the hard facts, we only know for fore sure, that PLL_DIV can also be 1 for stm32f303xc devices.

In the datasheet, there is no information about PLL_DIV not being able to be set to 1, IIRC (except the clock tree drawing).

So I would go and look at stmCubeIDE for most of the chip, which are effected by this (probably) wrong conditional:

stm32f3xx-hal/src/rcc.rs

Lines 373 to 378 in 54e8261

if multiplier == 1 || divisor == 1 {
// PLL_MUL minimal value is 2
multiplier *= 2;
// PRE_DIV minimal value is 2
divisor *= 2;
}

Side-Note:

I hope to make this code more ergonomic for the user sometime. Assertion are not really idiomatic in a library, and it is not really obvious for embedded controllers, when assertions are hit, as you have to debug it, to see, if the chip clock is misconfigured.

Either the freeze() function should be made fallible (my favorite)

pub fn freeze(self, acr: &mut ACR) -> Clocks {

Or include const_assertaion and move the clock calculation to compile time, which should be possible sometime with a newer rustc version, but it is not yet.

@Sh3Rm4n Sh3Rm4n added the bug Something isn't working label Oct 13, 2020
@Sh3Rm4n Sh3Rm4n self-assigned this Oct 13, 2020
@teskje
Copy link
Collaborator

teskje commented Oct 13, 2020

Looking at the register descriptions in the RM it looks like the clock tree is simply inaccurate. For the PREDIV field in RCC_CFGR2 (which is the one we are concerned about I believe) the RM says:

0000: HSE input to PLL not divided
0001: HSE input to PLL divided by 2
0010: HSE input to PLL divided by 3
...

(section 9.4.12)

Note that setting the value to 0000 (the default) will make it so the HSE input to the PLL is not divided.

As an additional datapoint, I was definitely able to achieve 72 MHz on my F3DISCOVERY with the built-in HSE. That was using my own HAL, with this RCC abstraction: https://gitlab.com/ra_kete/stm32f303xc-hal/-/blob/master/src/rcc.rs (which doesn't bother with setting PREDIV at all).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants