stm32l1/stmclk: refactor clck init to use stmclk.h, add msi option by fjmolinas · Pull Request #8402 · RIOT-OS/RIOT

fjmolinas · 2018-01-22T18:54:39Z

Contribution description

This PR is based on work by @olegart, refactors to use stmclk.h structure and adds MSI clock use.

Issues/PRs references

#8403 depends on this PR

kYc0o · 2018-01-22T21:59:26Z

Wow, I just have started to work on this but this have already a good shape. I'll make a quick review and a test with all the associated PRs.

Thanks a lot for tackling this!

kYc0o

Some changes to do especially on the format of #if statements. Please align all of them to the left and add some comments where needed.

kYc0o · 2018-01-22T22:04:55Z

cpu/stm32l1/stmclk.c

+    RCC->CR |= CLOCK_CR_SOURCE;
+    /* Wait till the clock source is ready
+     * NOTE: the MCU will stay here forever if you use an external clock source and it's not connected */
+    while (!(RCC->CR & CLOCK_CR_SOURCE_RDY)) {}


I was about to introduce a timeout here to know if the HSE is ready, otherwise we enable the HSI. Would you mind to introduce something like that?

@kYc0o isn't what you are refering introduced at line 129-136

while (!(RCC->CR & clock_source_rdy)) { timeout++; if (timeout > 10000) { clock_source_rdy = RCC_CR_HSIRDY; timeout = 0; } }

Oh yeah, didn't see that code in depth. However I should ask, why only when defined(CLOCK_HS_MULTI)? IMHO this should be present whenever any external clock source is being used. So, in your case, are you assuming that HSI can't be used standalone? Otherwise, I think it's worth to show explicitly in the code when only HSI is being used.

There's also a consensus, especially with nucleo boards on which some revisions don't have any HSE or LSE, that we try systematically to use HSE, if it's not present, we switch to internal.

Well because when defined(CLOCK_HS_MULTI) is not defined CLOCK_CR_SOURCE would be HSI or HSE, and if CLOCK_HSI is not defined and HSE is not responding for some reason than the only option would be MSI which as far as I know isn't supported as base clock in RIOT, I'm not sure if it could interfere with other drivers (ADC at least wouldn't work); and PLL can't be configured from MSI.

What you mean is that only HSI == MSI? As far as I can see in the code for CLOCK_HS_MULTI you need both HSI and HSE, and above you're forcing CLOCK_HS_MULTI when HSI and HSE are defined. Thus, I need some clarification on how this works and which are the possibilities with boards without HSE and LSE.

If HSE try to configure it, if timeout go to HSI

Discussing offline with @haukepetersen convinced me that this might not be necessary, since it's also somehow delaying the startup, so you can just assume the configuration is given by the user who knows if HSE is available.

If HSI use it directly, no timeout needed

Agree.

MSI not yet an option as a run clock since there aren't function to properly handle the lower core clock speed this would have.

Looking at the implementatin for L4, it seems it's also fallbacking to MSI under certain conditions, though L4 can actually go to 48MHz... Can you just take a look and if needed try to adapt it to this PR?

convinced me that this might not be necessary, since it's also somehow delaying the startup

Why would this delay start-up?

Looking at the implementation for L4, it seems it's also fall-backing to MSI under certain conditions, though L4 can actually go to 48MHz... Can you just take a look and if needed try to adapt it to this PR?

There are two problems and difference I see here related to L4.

L1 MSI goes up to 4 MHz on L1, and usually targeted CORE_CLOCK on nucleo boards is 32MHz, and CORE_CLOCK is used by other driver to configure their clock speed, which wouldn't be accurate in this case.
*L4 MSI can be used to run the PLL and also ADC threw the SYSCLK, which is not the case for L1.

IMHO, for this to work there should first be support for changing CORE_CLOCK speed for this to work properly since drivers (spi, uart) should be able to adjust to this. And also somehow adc should be disabled when running on MSI in L1. And I think these would be bigger changes since the current clock speed would have to be exposed, and that would start a hole different discussion.

adc should be disabled when running on MSI in L1

Why? ADC on L1 needs HSI to be enabled, but it doesn't care about system clock. On L0 and L4, both HSI and APB clock can be selected as ADC clock source.

@olegart you are right, I understood that part wrong.

@olegart do you see a problem with leaving the use of MSI as a sysclk source and all it's config for a later PR (this would come with functions to recalculate uart baudrate, xtimer and others) ??

kYc0o · 2018-01-22T22:06:29Z

cpu/stm32l1/stmclk.c

+    tmpreg &= ~(RCC_CR_HSEBYP | RCC_CR_CSSON | RCC_CR_PLLON);
+    RCC->CR = tmpreg;
+
+    cpu_clock_global = 65536 * (1 << (msi_range >> 13));


Can you use defines instead of only numbers here? And also add some comments about the usage of such constants.

Thinking I should remove this. It isn't really needed unless runing MCU on MSI clock in run-mode. In that case CORE_CLOCK isn't the mcu actual speed and might need to recalculate timer, uart and other setting accordingly to this. But that would mean exposing a global variable and stuff like that. What do you think?

kYc0o

Found some other issues.

In our current supported boards there's no definition of HSI for stm32l1 based boards, though I can imagine a custom board or custom application using and defining both of them, but I wonder, what's the behaviour when only HSI is being used?

kYc0o · 2018-01-23T22:02:17Z

cpu/stm32l1/stmclk.c

+    RCC->CR |= CLOCK_CR_SOURCE;
+    /* Wait till the clock source is ready
+     * NOTE: the MCU will stay here forever if you use an external clock source and it's not connected */
+    while (!(RCC->CR & CLOCK_CR_SOURCE_RDY)) {}


Oh yeah, didn't see that code in depth. However I should ask, why only when defined(CLOCK_HS_MULTI)? IMHO this should be present whenever any external clock source is being used. So, in your case, are you assuming that HSI can't be used standalone? Otherwise, I think it's worth to show explicitly in the code when only HSI is being used.

There's also a consensus, especially with nucleo boards on which some revisions don't have any HSE or LSE, that we try systematically to use HSE, if it's not present, we switch to internal.

kYc0o · 2018-01-23T22:03:27Z

examples/hello-world/main.c

    printf("This board features a(n) %s MCU.\n", RIOT_MCU);

+    pm_set(1);
+


I think this got in a commit without being noticed...

kYc0o · 2018-01-23T22:04:50Z

cpu/stm32l1/stmclk.c

@@ -0,0 +1,293 @@
+/*
+ * Copyright (C) 2014 Freie Universität Berlin


Please add your, or your institute/company copyright.

kYc0o · 2018-01-23T22:05:36Z

cpu/stm32l1/stmclk.c

+ * @{
+ *
+ * @file
+ * @brief       Implementation of the kernel cpu functions


I think here something like Implemetation of CPU clock functions might be more suitable.

kYc0o · 2018-01-23T22:08:06Z

cpu/stm32l1/stmclk.c

+/* See if we want to use the PLL */
+#if defined(CLOCK_PLL_DIV) || defined(CLOCK_PLL_MUL) || \
+defined(CLOCK_PLL_DIV_HSE) || defined(CLOCK_PLL_MUL_HSE) || \
+defined(CLOCK_PLL_DIV_HSI) || defined(CLOCK_PLL_MUL_HSI)


You aligned the #if statement, but the subsequent defines which belong to the same statement are also aligned between them, thus:

#if defined(CLOCK_PLL_DIV) || defined(CLOCK_PLL_MUL) || \ defined(CLOCK_PLL_DIV_HSE) || defined(CLOCK_PLL_MUL_HSE) || \ defined(CLOCK_PLL_DIV_HSI) || defined(CLOCK_PLL_MUL_HSI)

is the result we want.

kYc0o

I have some other questions regarding the code...

kYc0o · 2018-01-25T16:54:16Z

cpu/stm32l1/stmclk.c

+#else
+#define CORE_VOLTAGE (PWR_CR_VOS_1 | PWR_CR_VOS_0)
+#endif
+


Well now that the #ifdef's got aligned is difficult to follow each statement. Can you add some doc (preferably doxygen, otherwise normal comments would be ok) to the statements?

kYc0o · 2018-01-25T18:23:04Z

cpu/stm32l1/stmclk.c

+    RCC->CR |= CLOCK_CR_SOURCE;
+    /* Wait till the clock source is ready
+     * NOTE: the MCU will stay here forever if you use an external clock source and it's not connected */
+    while (!(RCC->CR & CLOCK_CR_SOURCE_RDY)) {}


What you mean is that only HSI == MSI? As far as I can see in the code for CLOCK_HS_MULTI you need both HSI and HSE, and above you're forcing CLOCK_HS_MULTI when HSI and HSE are defined. Thus, I need some clarification on how this works and which are the possibilities with boards without HSE and LSE.

fjmolinas · 2018-01-26T15:21:53Z

@kYc0o are some of the issues you mentioned in #8401 related to (or could affect) this PR or #8403??

kYc0o · 2018-01-26T15:38:52Z

AFAIK no, this is IMHO a separated issue which will (should) be included further when we repair the state of stm32l1xx devices, but I'm testing separately as much as I can.

kYc0o · 2018-02-02T14:45:59Z

I was looking more closer to the Lxx clock implementations to see if we can migrate them together but they look quite different. Thus I like this approach of stmclk and with the needed adjustments it can be merged IMHO. @haukepetersen if you can detail your concerns about this PR it would be awesome, so we can focus the efforts on something which can be merged and benefit a better support for the currently broken L1 platform. Though this won't solve the current problems (mostly related to PM), it's a step forward.

fjmolinas · 2018-02-02T15:24:12Z

@kYc0o simplified some of th the clock intialization (assumed HSI allways enabled). Some functionality was removed sutch as using MSI as core clock since there isn't support for this to happen in other RIOT modules or function, might address this on another PR since some code is there already. .Please tell me what you think :).

fjmolinas · 2018-02-02T16:20:13Z

@kYc0o reposting this since the older comment thread isn't visible anymore.

Discussing offline with @haukepetersen convinced me that this might not be necessary, since it's also somehow delaying the startup, so you can just assume the configuration is given by the user who knows if HSE is available.

Why would this delay start-up?

Looking at the implementation for L4, it seems it's also fall-backing to MSI under certain conditions, though L4 can actually go to 48MHz... Can you just take a look and if needed try to adapt it to this PR?

There are two problems and difference I see here related to L4.

L1 MSI goes up to 4 MHz on L1, and usually targeted CORE_CLOCK on nucleo boards is 32MHz, and CORE_CLOCK is used by other driver to configure their clock speed, which wouldn't be accurate in this case.
*L4 MSI can be used to run the PLL and also ADC threw the SYSCLK, which is not the case for L1.

IMHO, for this to work there should first be support for changing CORE_CLOCK speed for this to work properly since drivers (spi, uart) should be able to adjust to this. And also somehow adc should be disabled when running on MSI in L1. And I think these would be bigger changes since the current clock speed would have to be exposed, and that would start a hole different discussion.

kYc0o · 2018-02-02T16:40:44Z

Why would this delay start-up?

Because of the timeout needed to know if HSE is present.

IMHO, for this to work there should first be support for changing CORE_CLOCK speed for this to work properly since drivers (spi, uart) should be able to adjust to this. And also somehow adc should be disabled when running on MSI in L1. And I think these would be bigger changes since the current clock speed would have to be exposed, and that would start a hole different discussion.

Agree, let's see if MSI can be added in the future.

kYc0o

A small comment yet...

kYc0o · 2018-02-02T16:41:21Z

cpu/stm32_common/include/stmclk.h

+ * @param[in]   msi_range MSI frequency range
+ * @param[in]   ahb_divider AHB bus divider
+ */
+void stmclk_switch_msi(uint32_t msi_range, uint32_t ahb_divider);


if MSI is not present anymore this is not needed.

needed for #8403 to switch to low-power sleep mode

Hmmm yeah but that's independent of this PR, so it should be included either in a separated (upcoming) PR or in #8493, but I'd say in a different PR enhancing this one.

kYc0o · 2018-02-02T16:44:30Z

cpu/stm32l1/stmclk.c

+    /* Reset HSION, HSEON, CSSON and PLLON bits */
+    RCC->CR &= ~(RCC_CR_HSION | RCC_CR_HSEON | RCC_CR_HSEBYP | RCC_CR_CSSON | RCC_CR_PLLON);
+    /* Clear all interrupts */
+    RCC->CIR = 0x0;


Maybe this whole initialisation can follow a bit the one from L4, which takes into account the previous state of the interrupts.

There's something I don't like about this approach. One ends up having multiple useless calls to irq_disable; irq_restore. This function in only used in cpu_init (are there any isr that should be restored here?), and in pm. And in pm ir_restore, irq_disable already has to be called around this function. Anyway, I saw this comment in th l4 code: /* disable any interrupts. Global interrupts could be enabled if this is * called from some kind of bootloader... */ . I don't now much about that case so if that is a concern I will include it, I leave it up to you @kYc0o, just feels weird having so many nested calls to irq_disable/restore.

Well actually I'm working on this platform because I want to support the RIOT bootlaoder I'm working on, towards OTA updates. Thus, the case of a bootlaoder is very important since I had several problems with clocking when it was already initialised by the bootloader. I can actually test it soon, as I'm approaching an end to the support of this platform (if #8518 gets merged soon)

OK!, will add then. Will wait for #8518 though :), awesome to see nucleo-l1 problems are getting fixed.

kYc0o · 2018-02-02T16:45:55Z

cpu/stm32l1/stmclk.c

+    RCC->CR |= RCC_CR_HSION;
+    /* Wait till the clock source is ready
+     * NOTE: no timeout for HSI, should allways be available*/
+    while (!(RCC->CR & RCC_CR_HSIRDY)) {}


Here I think you can make use of stmclk_enable_hsi();

kYc0o · 2018-02-02T16:46:48Z

cpu/stm32l1/stmclk.c

+
+    /* Wait for sysyem clock to be ready before desabling other clocks*/
+    while ((RCC->CFGR & (uint32_t)RCC_CFGR_SWS) != clock_cfgr_sw_rdy) {}
+    RCC->CR &= ~(clock_disable_clocks);


And here stmclk_disable_hsi(); instead.

it's not always HSI, can be HSI+MSI or HSE+MSI. MSI is not enabled (software wise) as a core clock but still used on start-up and reset and needes to be disabled afterwards.

fjmolinas · 2018-02-02T17:09:46Z

Why would this delay start-up?
Because of the timeout needed to know if HSE is present.

If HSE is ready (RCC->CR & clock_source_rdy) will be valid and it would exit the loop, it doesn't wait for the whole timeout, I really can't see any delay, if HSE isn't configured on the board it doesn't even get into the timeout part of the code. I don't actually mind assuming user now id HSE is or isn't present, just can't see how this would delay start_up, more thatn the time it takes the cpu to add 1 to the timeout variable.

kYc0o · 2018-02-21T11:28:49Z

cpu/stm32l1/stmclk.c

+    irq_restore(state);
+}
+
+void stmclk_switch_msi(uint32_t msi_range, uint32_t ahb_divider)


As I said above, this function belongs to an upcoming PR.

kYc0o · 2018-02-21T11:29:49Z

I'll need a bit more time to test this PR but I think it's already in a good shape.

I'd like to have @haukepetersen or @kaspar030 ACK here...

fjmolinas · 2018-03-05T18:07:05Z

@kYc0o removed switch_msi_clk, postpone to another PR

fjmolinas · 2018-03-15T17:53:35Z

@kYc0o how are things going on your side? any new info?

fjmolinas · 2018-03-19T16:15:44Z

@kYc0o @kaspar030 @haukepetersen just saw that #8735 got merged 14 days ago. I get it implements the same thing in a different way, I also agree its cleaner/better. I just find it really annoying that you have me adapting the PR making changes for 3 month's, and apparently just wasting time just to open a different PR and have it closed the same day or the day after, at least since you are tagged in this PR letting me know you will work on a different solution would be cool.

I think kaspar's solution is cleaner, better; and does a lot of tiding up. It's just really annoying how the contribution process works in RIOT, it really dissuades anyone that isn't a maintainer or in direct contact with a maintainer to spend time adapting code to fit RIOT or making PR's. I don't really care about where ever one solution or another gets merged, the better, cleaner, smaller one should get merged, but I do care about wasting my time. A "contested" tag or don't like the solution from the start would have saved my time and also @kYc0o. Cheers,

Francisco.

PS: we can close this one too

fjmolinas · 2018-03-20T14:37:51Z

Closed this PR, kind of duplicate of #8735.

fjmolinas mentioned this pull request Jan 22, 2018

stm32l1/pm: add low power modes for stm32l1 #8403

Closed

kYc0o requested changes Jan 22, 2018

View reviewed changes

kYc0o self-assigned this Jan 22, 2018

fjmolinas force-pushed the stm32l1_clk branch from 8eddc69 to b3f1827 Compare January 23, 2018 19:20

kYc0o requested changes Jan 23, 2018

View reviewed changes

fjmolinas mentioned this pull request Jan 24, 2018

stm32_common/uart: add baudrate recalculation based on clk settings #8401

Closed

fjmolinas force-pushed the stm32l1_clk branch from db10652 to 30c57db Compare January 24, 2018 15:00

kYc0o requested changes Jan 25, 2018

View reviewed changes

fjmolinas force-pushed the stm32l1_clk branch from 6376fff to 30c57db Compare January 26, 2018 15:12

fjmolinas force-pushed the stm32l1_clk branch from 1abbe8b to 593141d Compare February 2, 2018 15:25

kYc0o requested changes Feb 2, 2018

View reviewed changes

kYc0o mentioned this pull request Feb 6, 2018

cpu/cortexm: add __NOP(); after __WFI(); for stm32l152 to avoid hardfault #8518

Merged

fjmolinas force-pushed the stm32l1_clk branch from cef1bb7 to e181afa Compare February 7, 2018 19:36

fjmolinas added 5 commits February 12, 2018 17:34

cpu/stm32l1: refactor to use stmclk.h, add msi-clk option

fe3e45e

stm32l1/stmclk: fix ifdef alignment

863a391

stm32l1/stmclk: remove some uneeded variables

c8f0713

--fixup: remove unintended file, style and headers

84e4941

--fixup: simplify clk config, HSI assumed available

29b20ab

fjmolinas added 2 commits February 12, 2018 17:34

--fixup: use stmcl_enable_hsi

c878e79

--fixup: disable irq before sys_clik init, restore after

20dbd2e

fjmolinas force-pushed the stm32l1_clk branch from e181afa to 20dbd2e Compare February 12, 2018 20:34

kYc0o reviewed Feb 21, 2018

View reviewed changes

--fixup: remove stmclk_switch_msi function

04b354d

fjmolinas closed this Mar 20, 2018

fjmolinas deleted the stm32l1_clk branch July 30, 2021 13:54

		printf("This board features a(n) %s MCU.\n", RIOT_MCU);

		pm_set(1);

		@@ -0,0 +1,293 @@
		/*
		* Copyright (C) 2014 Freie Universität Berlin

Conversation

fjmolinas commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Issues/PRs references

Uh oh!

kYc0o commented Jan 22, 2018

Uh oh!

kYc0o left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjmolinas Jan 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjmolinas Jan 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjmolinas Feb 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kYc0o left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kYc0o left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Jan 26, 2018

Uh oh!

kYc0o commented Jan 26, 2018

Uh oh!

kYc0o commented Feb 2, 2018

Uh oh!

fjmolinas commented Feb 2, 2018

Uh oh!

fjmolinas commented Feb 2, 2018

Uh oh!

kYc0o commented Feb 2, 2018

Uh oh!

kYc0o left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fjmolinas commented Jan 22, 2018 •

edited

Loading

fjmolinas Jan 23, 2018 •

edited

Loading

fjmolinas Jan 24, 2018 •

edited

Loading

fjmolinas Feb 2, 2018 •

edited

Loading

fjmolinas Feb 9, 2018 •

edited

Loading

fjmolinas Feb 9, 2018 •

edited

Loading

fjmolinas Feb 2, 2018 •

edited

Loading

fjmolinas commented Mar 5, 2018 •

edited

Loading

fjmolinas commented Mar 19, 2018 •

edited

Loading