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.

Sign up for GitHub

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

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

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion drivers/clock_control/stm32f1x_ll_clock.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
#undef RCC_PREDIV1_SOURCE_PLL2
#endif /* CONFIG_CLOCK_STM32_PLL_SRC_PLL2 */


/**
* @brief fill in pll configuration structure
*/
Expand All @@ -50,6 +49,11 @@ void config_pll_init(LL_UTILS_PLLInitTypeDef *pllinit)
pllinit->PLLMul = ((CONFIG_CLOCK_STM32_PLL_MULTIPLIER - 2)
<< RCC_CFGR_PLLMULL_Pos);

#if defined(CONFIG_CLOCK_STM32_PLL_SRC_HSI)
/* If HSI is used as PLL source an automatic divisor of 2 will be
* applied */
pllinit->Prediv = LL_RCC_PLLSOURCE_HSI_DIV_2;
#else
#ifdef CONFIG_SOC_STM32F10X_DENSITY_DEVICE
/* PLL prediv */
#ifdef CONFIG_CLOCK_STM32_PLL_XTPRE
Expand Down Expand Up @@ -78,6 +82,7 @@ void config_pll_init(LL_UTILS_PLLInitTypeDef *pllinit)
*/
pllinit->Prediv = CONFIG_CLOCK_STM32_PLL_PREDIV1 - 1;
#endif /* CONFIG_SOC_STM32F10X_DENSITY_DEVICE */
#endif /* CONFIG_CLOCK_STM32_PLL_SRC_HSI */
}

#endif /* CONFIG_CLOCK_STM32_SYSCLK_SRC_PLL */
Expand Down
4 changes: 2 additions & 2 deletions ext/hal/st/stm32cube/stm32f1xx/drivers/src/stm32f1xx_ll_utils.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@therealprof therealprof Aug 6, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tracking number: 51414

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 */
+ }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

#endif /* RCC_PLL2_SUPPORT */
/* Enable PLL */
LL_RCC_PLL_Enable();
while (LL_RCC_PLL_IsReady() != 1U)
Expand Down