drivers: clock_control: stm32: fail to increase flash latency when MSI increase

[chienhung-lin]

Describe the bug The bug happens after commit #62133 when set_up_fixed_clock_sources call before update flash latency.

https://github.com/zephyrproject-rtos/zephyr/blob/5670bad5057b1c2af7dd13b275f993866ad5420f/drivers/clock_control/clock_stm32_ll_common.c#L726-L746

B-L475E-IOT01A clock source is MSI at 4MHz after reset, and it will be set up to 48MHz in set_up_fixed_clock_sources for preparation as USB clock source further.

https://github.com/zephyrproject-rtos/zephyr/blob/5670bad5057b1c2af7dd13b275f993866ad5420f/boards/arm/disco_l475_iot1/disco_l475_iot1.dts#L88-L92

https://github.com/zephyrproject-rtos/zephyr/blob/5670bad5057b1c2af7dd13b275f993866ad5420f/dts/arm/st/l4/stm32l475.dtsi#L22-L23

The flash latency should increase before the system clock changes from 4MHz to 48MHz.

To Reproduce Steps to reproduce the behavior:

1. cd zephyr
2. west build -p always -b disco_l475_iot1 samples/helloworld
3. west flash

Expected behavior See helloworld on console.

Environment (please complete the following information):

• OS: Ubuntu 22.04 LTS
• Toolchain: zephyr-sdk-0.16.1
• Commit SHA or Version used: 5670bad

[TilmannUnte]

Unfortunately I can't help at this point, but I can confirm the problem at least. The disco_l475_iot1 board has been non-functional beginning with 5670bad5057b1c2af7dd13b275f993866ad5420f.

[gautierg-st]

Hello @maxvankessel, Since you worked on this part, do you see an elegant solution to fix this problem while keeping the feature you introduced?

[maxvankessel]

I'm very sorry that my bug fix led to someone else's bug. I'm not completely familiar with this L series, in contrary to the F series. But could you point me in the direction where this:

The flash latency should increase before the system clock changes from 4MHz to 48MHz.

Can be found in the documentation? Reference manual or the Datasheet?

[gautierg-st]

You can have a look at the Ref Man of STM32L47 (RM0351), § 3.3.3 Read access latency.

For L4, the default clock source at reset is MSI, but MSI is configurable, and calling set_up_fixed_clock_sources changes it frequency before the Flash latency is modified.

By the way, I'm not asking you to provide a fix. I'm merely asking your opinion on the best way forward since I'm not familiar with the overdrive feature you added.

[maxvankessel]

Thanks @gautierg-st, and I'm willing to help!

I'll try to look into it tomorrow, and get back to you. I think it will be easy to fix. But I also believe more devices suffer from my change.

[gautierg-st]

Thanks for tackling this issue. And, yes, the issue will likely occur on all series that use MSI as a default reset clock.

[erwango]

@maxvankessel Thanks for having a look. Let's revert https://github.com/zephyrproject-rtos/zephyr/pull/62133/commits/60c8ba0e8f8270427703e573cf7e55646d77f75f if we're not able to find a solution quickly. Improvement is minor compared to the impact of the regression (but don't worry, this happens and as a reviewer, I also have my share of responsibility).

[maxvankessel]

So, if I follow documentation correctly their shouldn't be an issue. But setting up clocks (set_up_fixed_clock_sources) might change the value assigned to the old_flash_freq value. And this might skip the this code:

https://github.com/zephyrproject-rtos/zephyr/blob/2243d7b717e090025c7915db23a7392f26d1b64f/drivers/clock_control/clock_stm32_ll_common.c#L742C1-L745C1

The documentation states, and this is almost the same for F7 and L4:

1. Program the new number of wait states to the LATENCY bits
2. ....
3. Modify the CPU clock source by writing the SW bits in the RCC_CFGR register.

Step 3 changes the SYSCLOCK (SW bits), so the order is correct. Because SYSCLOCK is set after this code block. Can @chienhung-lin check if by changing the code to this:

    /* Some clocks would be activated by default */
    config_enable_default_clocks();

#if defined(FLASH_ACR_LATENCY)
    uint32_t old_flash_freq;

    old_flash_freq = RCC_CALC_FLASH_FREQ(HAL_RCC_GetSysClockFreq(),
                           GET_CURRENT_FLASH_PRESCALER());
#endif

    /* Set up indiviual enabled clocks */
    set_up_fixed_clock_sources();

    /* Set up PLLs */
    set_up_plls();

#if defined(FLASH_ACR_LATENCY)
    uint32_t new_flash_freq;

    new_flash_freq = RCC_CALC_FLASH_FREQ(CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC,
                      STM32_FLASH_PRESCALER);

    /* If freq increases, set flash latency before any clock setting */
    if (old_flash_freq < CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC) {
        LL_SetFlashLatency(CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC);
    }
#endif /* FLASH_ACR_LATENCY */

The issues is resolved? This would be the most elegant way, a shortcut can be taken, by always calling LL_SetFlashLatency

Also we should remove new_flash_freq its unused. @erwango

[maxvankessel]

It can also be found here: https://github.com/maxvankessel/zephyr/tree/pr/flash-latency-setup

[erwango]

Also we should remove new_flash_freq its unused. @erwango

Indeed, it was introduced here: https://github.com/zephyrproject-rtos/zephyr/commit/efd8ee465c72dd415e15e9fd834f4e1aa4a626a8.

I don't think it should be removed but rather re-instanciate the former logic.

[chienhung-lin]

Thanks everyone join discussion and have a look this issue.

So, if I follow documentation correctly their shouldn't be an issue. But setting up clocks (set_up_fixed_clock_sources) might change the value assigned to the old_flash_freq value. And this might skip the this code:

https://github.com/zephyrproject-rtos/zephyr/blob/2243d7b717e090025c7915db23a7392f26d1b64f/drivers/clock_control/clock_stm32_ll_common.c#L742C1-L745C1

Yes, it looks like manual only mention the case of switching clock source.

Step 3 changes the SYSCLOCK (SW bits), so the order is correct. Because SYSCLOCK is set after this code block. Can @chienhung-lin check if by changing the code to this: code block... The issues is resolved? ....

It did not work.

LL_SetFlashLatency need to be call before system clock change. I move flash latency increase before set up fixed_clock_sources. The following code works on B-L475E-IOT01A :

        /* Some clocks would be activated by default */
        config_enable_default_clocks();

#if defined(FLASH_ACR_LATENCY)
        uint32_t old_flash_freq;
        uint32_t new_flash_freq;

        old_flash_freq = RCC_CALC_FLASH_FREQ(HAL_RCC_GetSysClockFreq(),
                                               GET_CURRENT_FLASH_PRESCALER());

        new_flash_freq = RCC_CALC_FLASH_FREQ(CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC,
                                      STM32_FLASH_PRESCALER);

        /* If freq increases, set flash latency before any clock setting */
        if (old_flash_freq < CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC) {
                LL_SetFlashLatency(CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC);
        }
#endif /* FLASH_ACR_LATENCY */

        /* Set up indiviual enabled clocks */
        set_up_fixed_clock_sources();

        /* Set up PLLs */
        set_up_plls();

        if (DT_PROP(DT_NODELABEL(rcc), undershoot_prevention) &&
                (STM32_CORE_PRESCALER == LL_RCC_SYSCLK_DIV_1) &&
                (MHZ(80) < CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC)) {
                LL_RCC_SetAHBPrescaler(LL_RCC_SYSCLK_DIV_2);
        } else {
                LL_RCC_SetAHBPrescaler(ahb_prescaler(STM32_CORE_PRESCALER));
        }

@maxvankessel Can you try this code and check if it also work on F7 series?

I am wondering if this part can be more elegant. Like the flash latency change right before system clock change. Because some STM32 MCUs do not use MSI and their system clock change when switching system clock source. They will run in higher latency but the clock not increase yet.

[maxvankessel]

Hi @chienhung-lin, may I ask how you determine this?

It did not work.

Flash ACR register updated accordingly on F7 and G0. I need some pointers on how you validate this?

[chienhung-lin]

Sorry for late replying. @maxvankessel

Oh, I just mean the new modified code does not work.

I tested both version by compiling hello world sample and see if the console print message or not. I currently don't have a strict or proper way to validate this part.

From my understanding about Ref Man of STM32L47 (RM0351) 3.3.3 and 3.3.4, the flash clock could not catch up when mcu's clock is high. MCU have to wait a specific cycles for reading instruction or data from flash through bus. §3.3.4 have a diagram showing mcu pipeline and stall(read latency).

I "assume" two points:

1. MCU behavior may be undefined when flash latency is lower than it suppose to be. Like this bug case: MSI increases from 4 MHz to 48 MHz but not adjust flash latency.
2. Flash latency can be higher even though it doesn't have to. For example, increase latency to 4 wait cycle even system clock is 4 MHz and not necessary need any latency.

So I just move flash latency increase part before set_up_fixed_clock_sources and set_up_plls.

[erwango]

I've just pushed https://github.com/zephyrproject-rtos/zephyr/pull/63358. Tested on L4 and F7, can you have a check ? Thanks

[maxvankessel]

The correct behavior is still OK for the F7. Only odd thing I see when building a "release" (optimize, bootloader, etc) New flash frequency is 270MHz instead of 216MHz. This is 216MHz when building a "debug" version, I don't think it's related to the debugger.

This is when attaching the debugger to MCUboot. The application (loaded by MCUboot) has correct timings.

[erwango]

This is when attaching the debugger to MCUboot. The application (loaded by MCUboot) has correct timings.

Strange. are you talking about that new_flash_freq ? Can you check if this was already the case before and after https://github.com/zephyrproject-rtos/zephyr/pull/62133 ?

[maxvankessel]

Yes new_flash_freq, you can see in the top left "Variables". The old code (after #62133) reports 216MHz:

[erwango]

The old code (after https://github.com/zephyrproject-rtos/zephyr/pull/62133) reports 216MHz:

What about the older code (after #62133) ?

[erwango]

Note that on my side, using https://github.com/zephyrproject-rtos/zephyr/pull/63358, I get new_flash_freq = 216 in mcuboot. Tested with samples/application_development/sysbuild/with_mcuboot

[vgeneves]

Hi,

Sorry to re-open this, but this breaks my board (based on STM32F723). Main clock is running from PLL @216MHz (derived from HSE @25MHz).

After https://github.com/zephyrproject-rtos/zephyr/pull/63358, LL_SetFlashLatency is called before setting the overdrive mode.

Unfortunately LL_SetFlashLatency computes the wait states based on HClk and overdrive mode. I end up with a altency of 5 WS instead of 7.