drivers: clock_control: Allow PLL clock configuration with HSI on STM… #9300

therealprof · 2018-08-05T16:57:20Z

…32F1xx

The STM32F1xx allow to use the built-in oscillator to be used as PLL
input. This change sets sets both the fixed /2 prescaling divisor if HSI
is selected as input and prevents waiting for PLL2 to come online (which
only works for HSE input and will never happen with HSI input).

Signed-off-by: Daniel Egger daniel@eggers-club.de

codecov-io · 2018-08-05T17:26:17Z

Codecov Report

Merging #9300 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #9300   +/-   ##
=======================================
  Coverage   52.47%   52.47%           
=======================================
  Files         201      201           
  Lines       25317    25317           
  Branches     5287     5287           
=======================================
  Hits        13284    13284           
  Misses       9948     9948           
  Partials     2085     2085

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2cf47fe...632908c. Read the comment docs.

erwango · 2018-08-06T08:45:20Z

drivers/clock_control/stm32f1x_ll_clock.c

@@ -67,6 +67,11 @@ void config_pll_init(LL_UTILS_PLLInitTypeDef *pllinit)
 	 */
 	pllinit->Prediv = LL_RCC_PREDIV_DIV_1;
 #endif /* CONFIG_CLOCK_STM32_PLL_XTPRE */
+#else
+#if defined(CONFIG_CLOCK_STM32_PLL_SRC_HSI)


Seems this applies to both DENSITY and CONNECTIVITY lines, can we put this out of the #else ( /* CONFIG_SOC_STM32F10X_DENSITY_DEVICE */) ?
Something like :

#ifdef CONFIG_CLOCK_STM32_PLL_SRC_HSI /* Your code */ #else /* Existing PLL HSE code */ #endif

erwango · 2018-08-06T08:56:06Z

ext/hal/st/stm32cube/stm32f1xx/drivers/src/stm32f1xx_ll_utils.c

@@ -559,15 +559,15 @@ static ErrorStatus UTILS_EnablePLLAndSwitchSystem(uint32_t SYSCLK_Frequency, LL_
  /* Update system clock configuration */
  if (status == SUCCESS)
  {
-#if defined(RCC_PLL2_SUPPORT)
+#if defined(RCC_PLL2_SUPPORT) && !defined(CONFIG_CLOCK_STM32_PLL_SRC_HSI)


Ideally we shouldn't modify hal with Zephyr code.
Isn't there a non zephyr specific way to fix this?

Oops, didn't realise that this comes from the STM32 HAL driver. I just checked that this is indeed an upstream problem, however CubeMX generated code does not stumble over it because it's not using LL_PLL_ConfigSystemClock_HSI at all, this is what it does:

LL_RCC_HSI_Enable(); /* Wait till HSI is ready */ while(LL_RCC_HSI_IsReady() != 1) {} LL_RCC_PLL_ConfigDomain_SYS(LL_RCC_PLLSOURCE_HSI_DIV_2, LL_RCC_PLL_MUL_9); LL_RCC_PLL_Enable(); /* Wait till PLL is ready */ while(LL_RCC_PLL_IsReady() != 1) {} LL_RCC_SetAHBPrescaler(LL_RCC_SYSCLK_DIV_1); LL_RCC_SetAPB1Prescaler(LL_RCC_APB1_DIV_1); LL_RCC_SetAPB2Prescaler(LL_RCC_APB2_DIV_1); LL_RCC_SetSysClkSource(LL_RCC_SYS_CLKSOURCE_PLL); /* Wait till System clock is ready */ while(LL_RCC_GetSysClkSource() != LL_RCC_SYS_CLKSOURCE_STATUS_PLL) {}

The only alternative I see is to #undef RCC_PLL2_SUPPORT when CONFIG_CLOCK_STM32_PLL_SRC_HSI is defined which is globally set for the STM32F105/STM32F107 (only!), however I'm not sure where to put that so it takes effect.

I think in ext/hal/st/stm32cube/stm32f1xx/CMakeLists.txt you should be able to unset it.
Please document the README, I'll raise an issue in HAL, I'll give you the tracking number

tracking number: 51414

That doesn't seem to work. It doesn't surprise me since RCC_PLL2_SUPPORT is not a config option but a #define.

In the end if this is an actual problem

Well, if you consider the device just hanging in an endless loop during init, then this would be an actual problem.

The only alternative fixes outside of the LL API I see are:

Not using this magic do-it-all utility function to do the clock initialisation (like the CubeMX version)

Somehow undefining RCC_PLL2_SUPPORT

Can't we check for clock source set in registers and act upon this information?

How would that work? If we use UTILS_EnablePLLAndSwitchSystem() we will run in this problem (unless we can get rid of the define) -- always. Even if there was a way around it by checking registers (no idea at the moment whether that's a real possibility), if this function is used it's game over.

Luckily there's another severe problem with the HAL for the STM32F107 somewhere which I couldn't localise yet so for me this has no priority at the moment... Maybe it'll get fixed upstream some time.

My point was to use LL_RCC_PLL_GetMainSource in UTILS_EnablePLLAndSwitchSystem() to do this check before enabling PLL2.
Can't it work?

@therealprof ,

Coming back on this. Here is my proposal, which I'm unfortunately unable to test.
To be clear my proposal is to modify UTILS_EnablePLLAndSwitchSystem() using cube function, so in a way it could be maintained outside of zephyr.

Suggested change

#if defined(RCC_PLL2_SUPPORT) && !defined(CONFIG_CLOCK_STM32_PLL_SRC_HSI)

#if defined(RCC_PLL2_SUPPORT)

- /* Enable PLL2 */

- LL_RCC_PLL2_Enable();

- while (LL_RCC_PLL2_IsReady() != 1U)

- {

- /* Wait for PLL2 ready */

+ /* Wait PLL2 only if set as PLL source */

+ if (LL_RCC_PLL_GetMainSource() == LL_RCC_PLLSOURCE_PLL2) {

+ /* Enable PLL2 */

+ LL_RCC_PLL2_Enable();

+ while (LL_RCC_PLL2_IsReady() != 1U)

+ {

+ /* Wait for PLL2 ready */

+ }

Hm, I'd have to check that possibility but I don't know where my (orphaned) prototype went; might have to build another one but I'm lacking the time for that at the moment, I'm afraid.

Ok. Thanks for the answer anyway. I'll raise an internal issue

The STM32F1xx allow to use the built-in oscillator to be used as PLL input. This change sets sets both the fixed /2 prescaling divisor if HSI is selected as input and prevents waiting for PLL2 to come online (which only works for HSE input and will never happen with HSI input). Signed-off-by: Daniel Egger <daniel@eggers-club.de>

erwango · 2018-10-19T15:07:12Z

@therealprof , any update on this point?

I hope you didn't feel like I was trying to block you on this. I haven't tested my proposal of using LL_RCC_PLL_GetMainSource(HSI) to by pass PLL2 while loop (and I don' recall all details of the discussion), but how do you feel about this proposal?

therealprof · 2018-10-19T15:29:10Z

@erwango I haven't looked into it any further due to a lot of other things on my plate at the moment. If I recall correctly your proposal will not work but I'd have to check again to say for sure.

erwango · 2019-03-18T10:40:37Z

ST internal bug tracker: 51414

erwango · 2019-07-26T08:13:37Z

@therealprof, a fix for this issue will be available in F1 cube packaged release V1.8.0 (Current 1.6.1).

This will be fixed as follows:

#if defined(RCC_PLL2_SUPPORT)
    if(LL_RCC_PLL_GetMainSource() != LL_RCC_PLLSOURCE_HSI_DIV_2)
    {
      /* Enable PLL2 */
      LL_RCC_PLL2_Enable();
      while (LL_RCC_PLL2_IsReady() != 1U)
      {
        /* Wait for PLL2 ready */
      }
    }
#endif /* RCC_PLL2_SUPPORT */

erwango · 2019-09-05T13:47:29Z

@therealprof, closing the ticket.
Sum up: this has been fixed in STM32 HAL and will be integrated in zephyr on the package is available

therealprof · 2019-09-05T13:49:04Z

Perfect!

therealprof requested a review from erwango as a code owner August 5, 2018 16:57

therealprof force-pushed the features/fix-f1xx-hsi-pll branch from 844f8c7 to 0019da0 Compare August 5, 2018 21:44

erwango requested changes Aug 6, 2018

View reviewed changes

erwango added the platform: STM32 ST Micro STM32 label Aug 6, 2018

therealprof force-pushed the features/fix-f1xx-hsi-pll branch from 0019da0 to 632908c Compare August 6, 2018 18:36

erwango mentioned this pull request Mar 8, 2019

STM32WB Series support #13742

Merged

ioannisg added the Stale PR label Jun 22, 2019

erwango closed this Sep 5, 2019

therealprof deleted the features/fix-f1xx-hsi-pll branch September 5, 2019 13:49

therealprof mentioned this pull request Apr 27, 2020

Support for connectivity line devices (stm32f105 and stm32f107) stm32-rs/stm32f1xx-hal#205

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: clock_control: Allow PLL clock configuration with HSI on STM… #9300

drivers: clock_control: Allow PLL clock configuration with HSI on STM… #9300

therealprof commented Aug 5, 2018

codecov-io commented Aug 5, 2018 •

edited

Loading

erwango Aug 6, 2018

therealprof Aug 6, 2018

erwango Aug 6, 2018

therealprof Aug 6, 2018 •

edited

Loading

erwango Aug 6, 2018

erwango Aug 6, 2018

therealprof Aug 6, 2018

therealprof Sep 7, 2018

erwango Sep 7, 2018

erwango Dec 6, 2018

therealprof Mar 8, 2019

erwango Mar 8, 2019

erwango commented Oct 19, 2018

therealprof commented Oct 19, 2018

erwango commented Mar 18, 2019

erwango commented Jul 26, 2019

erwango commented Sep 5, 2019

therealprof commented Sep 5, 2019

drivers: clock_control: Allow PLL clock configuration with HSI on STM… #9300

drivers: clock_control: Allow PLL clock configuration with HSI on STM… #9300

Conversation

therealprof commented Aug 5, 2018

codecov-io commented Aug 5, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

therealprof Aug 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwango commented Oct 19, 2018

therealprof commented Oct 19, 2018

erwango commented Mar 18, 2019

erwango commented Jul 26, 2019

erwango commented Sep 5, 2019

therealprof commented Sep 5, 2019

codecov-io commented Aug 5, 2018 •

edited

Loading

therealprof Aug 6, 2018 •

edited

Loading