cpu/stm32f[2|4|7]: unify stmclk and improve PLL configuration by vincent-d · Pull Request #7477 · RIOT-OS/RIOT

vincent-d · 2017-08-16T10:53:41Z

When initializing PLL, input frequency was hard-coded to 2Mhz, leading to invalid output frequency when the crystal is not an even frequency (25MHz for instance).

Now, PLL input frequency can be configured (PLL_IN_FREQ) and PLL M and N factors are checked.

smlng · 2017-08-16T11:58:33Z

side questions: as both files look (very) similar can (or should) this go into cpu/stm32_common somehow?

smlng · 2017-08-16T11:58:54Z

@MichelRottleuthner might be interesting for you, too?

vincent-d · 2017-08-16T14:20:10Z

@smlng I unified stm32f2,4,7 stmclk files in stm32_common. It might be adapted to other stm32...

Changed PR title accordingly

smlng · 2017-08-16T14:21:39Z

nice work! Hope, it doesn't break existing stuff 🤞 We only have very few STM nucleos available for testing, to hopefully @aabadie can help on that, too.

vincent-d · 2017-08-16T14:23:55Z

I think it would need some more documentation on how to configure everything, does someone have an idea where is the best place to write that?

smlng · 2017-08-16T14:23:58Z

though this needs some squashing, still Murdock can have a first look on this.

smlng · 2017-08-16T14:26:13Z

where is the best place to write that

how about cpu/stm32_common/include/stmclk.h or create a doc.txt in cpu/stm32_common and add it there?

vincent-d · 2017-08-16T17:21:32Z

Added coreclock max value check and documentation (doc.txt in cpu/stm32_common)

vincent-d · 2017-08-16T17:32:17Z

Removed coreclock check as RCC_MAX_FREQUENCY seems to be defined only for f4.

vincent-d · 2017-08-17T08:41:59Z

I split the configuration in one header per PLL. Now, main PLL, PLLI2S and PLLSAI can be configured.

aabadie

A few comments and question. Otherwise this is very good job. I'll be able to test that on hardware next week.

aabadie · 2017-08-17T09:01:01Z

cpu/stm32_common/clk/pll.h

+#define _PLL_N_FINISHED 1
+#endif
+#if !_PLL_N_FINISHED
+#undef P


Are those lines needed, since the macro are redefined just below ?

yes because P could be already defined above, which would cause a preprocessor error on the (re)define below.

aabadie · 2017-08-17T09:01:43Z

cpu/stm32_common/clk/plli2s.h

+#define PLLI2S_SRC              RCC_PLLI2SCFGR_PLLI2SSRC
+#else
+#define PLLI2S_IN               PLL_IN
+#define PLLI2S_SRC              0


Maybe use ()

aabadie · 2017-08-17T09:06:06Z

cpu/stm32_common/doc.txt

+/**
+ * @ingroup cpu
+ * @defgroup cpu_stm32_common STM32 common
+ * @{


I don't think this block @{ is required.

yep, I think the block is not needed, too - though it doesn't hurt either. Still I would re-order that to:

/** * @defgroup cpu_stm32_common STM32 common * @ingroup cpu * @brief STM32 common configurations [ * @{ ] <- optional *

the @brief should be above @{ (if still used)

smlng · 2017-08-17T09:38:59Z

I split the configuration in one header per PLL.

does it simplify [edit] things/configuration [/edit], because looks like you introduced lots of duplicate code/macros and more LOCs now than with the single header before,

vincent-d · 2017-08-17T09:43:15Z

does it simplify think, because looks like you introduced lots of duplicate code/macros and more LOCs now than with the single header before,

There is more features now. A lot can be calculated automatically. And the problems with macro are:

there is no loop structures
it's hard to reuse (I'm not 100% sure with some macro evaluation, though. maybe it could be refactored a bit)

vincent-d · 2017-08-17T12:23:50Z

Refactored some macros

haukepetersen · 2017-08-18T09:29:10Z

a big +1 for the changes in general. However, the macro-code is very hard to read and will be extremely hard to maintain in the future!

Without having tried this: but couldn't we solve a lot of this functionality using inline functions? As all parameters are constants, the compiler should actually be able reduce those inline functions to constant expressions, right? Especially when building with LTO=1, the code size increase for these functions should be more or less non-existent but the readability/maintainability should be vastly improved.

vincent-d · 2017-08-18T13:35:57Z

However, the macro-code is very hard to read

I can only agree with that ;) even if I tried my best to have something easy to read.

but couldn't we solve a lot of this functionality using inline functions?

I don't know if it's really possible. I can give a try. But I would like to avoid any runtime computation, as the clock init is called when waking up where saving time is important.
I'm open to any help and suggestions on this!

vincent-d · 2017-08-18T13:46:18Z

Another idea would be to write that in an external tool (in python for instance) and generate the right constants.
But:

I don't know how to read properly preprocessor macro (you need at least CLOCK_HSE and CLOCK_CORECLOCK plus some constants from vendor header) in a python tool
I don't know how to add such a tool in the build process

haukepetersen · 2017-08-21T08:01:15Z

Another idea would be to write that in an external tool (in python for instance) and generate the right constants.

I think I would like that. How about something simple in C like I did for the computation of SPI clock frequencies -> cpu/stm32_common/dist/spi_divtable/spi_divtable.c? Or we might even want to add the functionality into a single tool, which computes all the relevant things for clocking a board. This would not even have to be part of the build-system, but it could simply statically dump the new configuration which then has to be copied manually into the periph_conf.h file.

vincent-d · 2017-08-25T10:39:15Z

the size and efficiency of the tool are second order to clarity and maintainability

I know, when I wrote smaller, I meant, less line duplication (same init value -> line duplication) ;)

I think I found a way. I'll try to push something this afternoon.

haukepetersen · 2017-08-25T10:41:34Z

nice!

vincent-d · 2017-08-25T12:52:11Z

Refactored clk_conf. Config is now in header file with a structure and an array.

haukepetersen · 2017-08-25T13:35:25Z

nice, I like it!

vincent-d · 2017-08-25T14:56:24Z

Fixed some wrong APB values, fixed spaces, updated boards.

Should be OK now!

vincent-d · 2017-08-28T09:24:52Z

Removed stm32f0 code from clk_conf (will be added in #7500)

haukepetersen

Just some minor remarks left, otherwise I think this PR is good to go.

haukepetersen · 2017-08-28T11:31:39Z

cpu/stm32_common/dist/clk_conf/clk_conf.c

+    puts("");
+
+    printf("/**\n"
+           " * @name Clock settings\n"


indention slipped off, should be tab aligned -> @name Clock settings

Sorry, I don't understand

all the comments in the perihp_conf.h have the equivalent spaces to a tab character after the @brief/@name -> leading to 3 spaces after a @brief and 4 spaces after a @name. So this should be also outputted by the clk_conf tool so that the comment style is consistent in the periph_conf.h files.

Ok, I didn't know that. Will fix.

haukepetersen · 2017-08-28T11:31:46Z

cpu/stm32_common/dist/clk_conf/clk_conf.c

+    printf("/**\n"
+           " * @name Clock settings\n"
+           " *\n"
+           " * @note This is auto-generated from\n"


haukepetersen · 2017-08-28T11:36:28Z

cpu/stm32_common/dist/clk_conf/clk_conf.c

+           "#define CLOCK_HSE           (%uU)\n", pll_src ? pll_in : 0);
+    printf("/* 0: no external low speed crystal available,\n"
+           " * 1: external crystal available (always 32.768kHz) */\n"
+           "#define CLOCK_LSE           (%d)\n", is_lse);


how about (%dU) to make it consistent with CLOCK_HSE?

haukepetersen · 2017-08-28T12:28:51Z

one more thing I just noticed: could you add .gitignore file to /cpu/stm32_common/dist, stating

clk_conf
spi_divtable

Thanks!

kaspar030 · 2017-08-28T13:18:48Z

cpu/stm32_common/dist/clk_conf/clk_conf.c

+
+    /* Print constants */
+    puts("==============================================================");
+    puts("Please copy the following code into your board's periph_conf.h");


please make it somehow possible to do this automatically. Either by using stderr for all other output, or by (maybe optionally) writing the actual code into a file.

(we can do that as followup. @haukepetersen feel free to dismiss my review if you're happy.)

Used stderr for evrything but code. Is it OK?

Yes! thanks!

haukepetersen · 2017-08-28T13:28:30Z

cpu/stm32_common/dist/clk_conf/clk_conf.c

+    puts("");
+
+    printf("/**\n"
+           " * @name   Clock settings\n"


sorry, still one space missing.

crap, I can't count up to 4! :D

Should be OK now

haukepetersen

Looking good -> ACK from my side

vincent-d · 2017-08-28T15:19:31Z

Oops, maybe I squashed a bit quickly. @kaspar030 any objections?

…_conf

aabadie · 2017-08-29T08:50:08Z

@kaspar030, your comment has been addressed. Are you fine with the change ?

kaspar030 · 2017-08-29T19:35:02Z

&go!

vincent-d added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Aug 16, 2017

vincent-d added this to the Release 2017.10 milestone Aug 16, 2017

vincent-d requested review from aabadie and haukepetersen August 16, 2017 10:53

vincent-d assigned haukepetersen and aabadie Aug 16, 2017

vincent-d changed the title ~~cpu/stm32f[2|4]: improve PLL configuration~~ cpu/stm32f[2|4|7]: unify stmclk and improve PLL configuration Aug 16, 2017

aabadie requested changes Aug 17, 2017

View reviewed changes

vincent-d force-pushed the pr/fix_stm32_clk branch from 84ca10b to c6db16f Compare August 17, 2017 13:41

haukepetersen requested changes Aug 28, 2017

View reviewed changes

kaspar030 requested changes Aug 28, 2017

View reviewed changes

haukepetersen reviewed Aug 28, 2017

View reviewed changes

haukepetersen approved these changes Aug 28, 2017

View reviewed changes

haukepetersen added the CI: needs squashing Commits in this PR need to be squashed; If set, CI systems will mark this PR as unmergable label Aug 28, 2017

vincent-d force-pushed the pr/fix_stm32_clk branch from fd96c36 to 674fd1e Compare August 28, 2017 15:18

vincent-d force-pushed the pr/fix_stm32_clk branch from 674fd1e to 63704bc Compare August 28, 2017 15:27

Vincent Dupont added 4 commits August 28, 2017 17:51

cpu/stm32_common: add a tool to generate clock settings

4001585

cpu/stm32_common: unify stmclk for stm32f[2|4|7] and adapt to use clk…

d2a4ace

…_conf

cpu/stm32_common: add clock config documentation

33e8a41

boards: adapt stm32f[2|4|7] boards clock config

fc9fc57

vincent-d force-pushed the pr/fix_stm32_clk branch from 63704bc to fc9fc57 Compare August 28, 2017 15:51

kaspar030 approved these changes Aug 29, 2017

View reviewed changes

kaspar030 merged commit acfa727 into RIOT-OS:master Aug 29, 2017

vincent-d mentioned this pull request Aug 30, 2017

cpu/stm32_common: fix PLLs enabling and usage #7537

Merged

toonst deleted the pr/fix_stm32_clk branch August 21, 2018 15:31

aabadie mentioned this pull request Sep 23, 2020

boards/stm32f2/f4/f7: rework clock initialization and configuration #14946

Merged

Conversation

vincent-d commented Aug 16, 2017

Uh oh!

smlng commented Aug 16, 2017

Uh oh!

smlng commented Aug 16, 2017

Uh oh!

vincent-d commented Aug 16, 2017

Uh oh!

smlng commented Aug 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vincent-d commented Aug 16, 2017

Uh oh!

smlng commented Aug 16, 2017

Uh oh!

smlng commented Aug 16, 2017

Uh oh!

vincent-d commented Aug 16, 2017

Uh oh!

vincent-d commented Aug 16, 2017

Uh oh!

vincent-d commented Aug 17, 2017

Uh oh!

aabadie 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!

smlng commented Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vincent-d commented Aug 17, 2017

Uh oh!

vincent-d commented Aug 17, 2017

Uh oh!

haukepetersen commented Aug 18, 2017

Uh oh!

vincent-d commented Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vincent-d commented Aug 18, 2017

Uh oh!

haukepetersen commented Aug 21, 2017

Uh oh!

vincent-d commented Aug 25, 2017

Uh oh!

haukepetersen commented Aug 25, 2017

Uh oh!

vincent-d commented Aug 25, 2017

Uh oh!

haukepetersen commented Aug 25, 2017

Uh oh!

vincent-d commented Aug 25, 2017

Uh oh!

vincent-d commented Aug 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haukepetersen 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!

Choose a reason for hiding this comment

smlng commented Aug 16, 2017 •

edited

Loading

smlng commented Aug 17, 2017 •

edited

Loading

vincent-d commented Aug 18, 2017 •

edited

Loading

vincent-d commented Aug 28, 2017 •

edited

Loading