sys: introduce sysclk function to retrieve core clock frequency #17342

aabadie · 2021-12-05T11:30:17Z

Contribution description

This PR introduces the sysclk() to get the value of the system core clock frequency. In most situation, this frequency corresponds to the CLOCK_CORECLOCK constant. But in some cases, this constant is wrong or not defined:

with native it's not defined at all so when using CLOCK_CORECLOCK some preprocessor checks are needed
on fe310, when using the internal oscillator clocks, the constant is not defined but the system clock is computed at runtime. This behavior could be extend to other families: on STM32, one could imagine fine tuning at runtime the trimming value of internal oscillator to get an approximate of a desired clock value (HSI16 or HSI48 - without autotrim - for example).
As an example, doing the following doesn't build on hifive1b:

$ CFLAGS=-DCONFIG_USE_CLOCK_HFROSC=1 BUILD_IN_DOCKER=1 BOARD=hifive1b make -C examples/blinky --no-print-directory 
Launching build container using image "riot/riotbuild:latest".
docker run --rm --tty --user $(id -u) -v '/usr/share/zoneinfo/Europe/Paris:/etc/localtime:ro' -v '/work/riot/RIOT:/data/riotbuild/riotbase:delegated' -e 'RIOTBASE=/data/riotbuild/riotbase' -e 'CCACHE_BASEDIR=/data/riotbuild/riotbase' -e 'BUILD_DIR=/data/riotbuild/riotbase/build' -e 'RIOTPROJECT=/data/riotbuild/riotbase' -e 'RIOTCPU=/data/riotbuild/riotbase/cpu' -e 'RIOTBOARD=/data/riotbuild/riotbase/boards' -e 'RIOTMAKE=/data/riotbuild/riotbase/makefiles'      -e 'BOARD=hifive1b' -e 'CFLAGS=-DCONFIG_USE_CLOCK_HFROSC=1'  -w '/data/riotbuild/riotbase/examples/blinky/' 'riot/riotbuild:latest' make     
Building application "blinky" for "hifive1b" with MCU "fe310".

/data/riotbuild/riotbase/examples/blinky/main.c: In function 'delay':
/data/riotbuild/riotbase/examples/blinky/main.c:44:43: error: 'CLOCK_CORECLOCK' undeclared (first use in this function)
   44 |         for (volatile uint32_t i = 0; i < CLOCK_CORECLOCK / 20; i++) { }
      |                                           ^~~~~~~~~~~~~~~
/data/riotbuild/riotbase/examples/blinky/main.c:44:43: note: each undeclared identifier is reported only once for each function it appears in
make[1]: *** [/data/riotbuild/riotbase/Makefile.base:146: /data/riotbuild/riotbase/examples/blinky/bin/hifive1b/application_blinky/main.o] Error 1

So the idea of this PR is to wrap CLOCK_CORECLOCK in a static inline function called sysclk (suggestions for a better name welcome), inspired by what was done with fe310 and let the cpu implementation define its own version when it cannot be determined at compile time, but only a runtime.
With compiler optimization, using this function has no effect compared to directly using the constant.

All tests, examples, drivers and pkg that are using CLOCK_CORECLOCK are adapted to use this function. We could also imagine to use it in the CPU implementations directly (like the fe310).

Last, but not least, I wasn't sure what's the best location for the header clk.h. For the moment, it's in sys/, before I put it in core/ but maybe it would be preferable to put it in cpu/ and in this case, where ? Suggestion welcome.

Testing procedure

Green Murdock
Check the firmware sizes are unchanged (maybe not for tests/driver_ws281x
fe310 features (uart, i2c, spi) are still working
affected test applications are still working
openwsn is still working

Issues/PRs references

fjmolinas · 2021-12-06T08:09:44Z

Some static checks nitpicks, lets see if murdock spots some important issues.

kfessel · 2021-12-06T13:05:18Z

since this is replacing a cpu_freq() is this cpu_freq() ? - my stm32 datasheet tells me that cpu_freq might be only a part of sysclk (there is at clk devider in front)

kaspar030 · 2021-12-06T13:08:03Z

sys/include/clk.h

+    return CLOCK_CORECLOCK;
+}
+#else
+uint32_t sysclk(void);


can we require this to be a static inline?

On fe310, this function is not static inline. it could be but that would need an indirection to another function.

But that wouldn't be an issue, would it? It would be IMO easier to read to have

extern uint32_t sysclk_implementation(void); static uint32_t sysclk(void) { return sysclk_implementation (); }

well encapsulated in the implementation rather than preprocessor conditionals in tge public header. But I guess it is a matter of personal taste :)

encapsulated in the implementation rather than preprocessor conditionals in tge public header

That would move the preprocessor conditionals in all cpu implementations. But I agree that when CLOCK_CORECLOCK is not defined, we can use an extern to a concrete cpu level implementation.

aabadie · 2021-12-06T13:19:02Z

since this is replacing a cpu_freq() is this cpu_freq() ?

Maybe the name sysclk is not right. I agree that it has a different meaning on stm32. Maybe coreclk or core_clock() would be better ?

kfessel · 2021-12-06T13:43:03Z

these options both seem better to me they also are more similar to the marco they replace (I got a slight pref for the shorter one (there isn't much readability gain in the snakey _clock)) cpu_freq() seems also reasonable

fjmolinas · 2021-12-15T10:20:47Z

cpu/fe310/clock.c

+        cpu_coreclk = PRCI_measure_mcycle_freq(3000, RTC_FREQ);
+        cpu_coreclk = PRCI_measure_mcycle_freq(3000, RTC_FREQ);


Suggested change

cpu_coreclk = PRCI_measure_mcycle_freq(3000, RTC_FREQ);

cpu_coreclk = PRCI_measure_mcycle_freq(3000, RTC_FREQ);

cpu_coreclk = PRCI_measure_mcycle_freq(3000, RTC_FREQ);

cpu_coreclk = PRCI_measure_mcycle_freq(3000, RTC_FREQ);

fjmolinas · 2021-12-15T10:21:09Z

cpu/fe310/clock.c

-    /* Clock frequency with HFROSC cannot be determined precisely from
-       settings */
-    /* If not done already, estimate the CPU frequency */
-    if (_cpu_frequency == 0) {
        /* Ignore the first run (for icache reasons) */


Suggested change

/* Ignore the first run (for icache reasons) */

/* Ignore the first run (for icache reasons) */

fjmolinas · 2021-12-15T10:26:55Z

fe310 features (uart, i2c, spi) are still working

The binary diff changes for fe310 so I think it would be good to run a couple of tests...

fjmolinas

I also ran the changed tests on arduino-uno arduino-zero atmega256rfr2-xpro b-l072z-lrwan1 frdm-k64f frdm-kw41z hifive1b i-nucleo-lrwan1 nrf52dk nucleo-f030r8 nucleo-f091rc nucleo-f103rb nucleo-f207zg nucleo-f303k8 nucleo-f303re nucleo-f334r8 nucleo-f767zi nucleo-g071rb nucleo-g474re nucleo-l073rz nucleo-l152re nucleo-l433rc nucleo-l452re nucleo-l496zg nucleo-l4r5zi nucleo-l552ze-q nucleo-wl55jc openmote-b p-nucleo-wb55 samr21-xpro slstk3402a z1

All passed excepto some stm32 where there was a race condition in downloading cmsis and openmote-b where periph-timer fails on master as well. See here for details

fjmolinas

Please squash the fe310 nitpicks right away, once fe310 test results are provided I'll ACK.

aabadie · 2021-12-15T10:31:50Z

The binary diff changes for fe310 so I think it would be good to run a couple of tests...

It probably changes because of the removed if.

kfessel

I assume it is guaranteed that coreclk() always is either a read of a non-volatile global or a const; but somehow when i read this i assume there might be a function call and a beginner might be inspired by this.

examples/blinky/main.c

pkg/openwsn/contrib/leds.c

tests/periph_uart_mode/main.c

aabadie · 2021-12-15T11:43:40Z

Hifive1b output with examples/blinky:

$ BUILD_IN_DOCKER=1 make BOARD=hifive1b -C examples/blinky flash term --no-print-directory 
Launching build container using image "riot/riotbuild:latest".
docker run --rm --tty --user $(id -u) -v '/usr/share/zoneinfo/Europe/Paris:/etc/localtime:ro' -v '/work/riot/RIOT:/data/riotbuild/riotbase:delegated' -e 'RIOTBASE=/data/riotbuild/riotbase' -e 'CCACHE_BASEDIR=/data/riotbuild/riotbase' -e 'BUILD_DIR=/data/riotbuild/riotbase/build' -e 'RIOTPROJECT=/data/riotbuild/riotbase' -e 'RIOTCPU=/data/riotbuild/riotbase/cpu' -e 'RIOTBOARD=/data/riotbuild/riotbase/boards' -e 'RIOTMAKE=/data/riotbuild/riotbase/makefiles'      -e 'BOARD=hifive1b'  -w '/data/riotbuild/riotbase/examples/blinky/' 'riot/riotbuild:latest' make 'BOARD=hifive1b'    
Building application "blinky" for "hifive1b" with MCU "fe310".

"make" -C /data/riotbuild/riotbase/boards/hifive1b
"make" -C /data/riotbuild/riotbase/core
"make" -C /data/riotbuild/riotbase/cpu/fe310
"make" -C /data/riotbuild/riotbase/cpu/fe310/periph
"make" -C /data/riotbuild/riotbase/cpu/fe310/vendor
"make" -C /data/riotbuild/riotbase/cpu/riscv_common
"make" -C /data/riotbuild/riotbase/cpu/riscv_common/periph
"make" -C /data/riotbuild/riotbase/drivers
"make" -C /data/riotbuild/riotbase/drivers/periph_common
"make" -C /data/riotbuild/riotbase/sys
"make" -C /data/riotbuild/riotbase/sys/auto_init
"make" -C /data/riotbuild/riotbase/sys/frac
"make" -C /data/riotbuild/riotbase/sys/malloc_thread_safe
"make" -C /data/riotbuild/riotbase/sys/newlib_syscalls_default
"make" -C /data/riotbuild/riotbase/sys/stdio_uart
"make" -C /data/riotbuild/riotbase/sys/ztimer
/opt/xpack-riscv-none-embed-gcc-10.2.0-1.2/bin/../lib/gcc/riscv-none-embed/10.2.0/../../../../riscv-none-embed/bin/ld:riscv_base.ld:224: warning: memory region `rom' not declared
   text	   data	    bss	    dec	    hex	filename
  10126	    152	   2328	  12606	   313e	/data/riotbuild/riotbase/examples/blinky/bin/hifive1b/blinky.elf
/work/riot/RIOT/dist/tools/jlink/jlink.sh flash /work/riot/RIOT/examples/blinky/bin/hifive1b/blinky.bin
### Flashing Target ###
### Flashing at base address 0x20010000 with offset 0 ###
SEGGER J-Link Commander V6.94b (Compiled Jan 26 2021 18:05:49)
DLL version V6.94b, compiled Jan 26 2021 18:05:34

J-Link Commander will now exit on Error

J-Link Command File read successfully.
Processing script file...

J-Link connection not established yet but required for command.
Connecting to J-Link via USB...O.K.
Firmware: J-Link OB-K22-SiFive compiled Jan 18 2021 09:05:42
Hardware version: V1.00
S/N: 979001370
VTref=3.300V
Target connection not established yet but required for command.
Device "FE310" selected.


Connecting to target via JTAG
ConfigTargetSettings() start
ConfigTargetSettings() end
TotalIRLen = 5, IRPrint = 0x01
JTAG chain detection found 1 devices:
 #0 Id: 0x20000913, IRLen: 05, Unknown device
Debug architecture:
  RISC-V debug: 0.13
  AddrBits: 7
  DataBits: 32
  IdleClks: 5
Memory access:
  Via system bus: No
  Via ProgBuf: Yes (16 ProgBuf entries)
DataBuf: 1 entries
  autoexec[0] implemented: Yes
Detected: RV32 core
CSR access via abs. commands: No
Temp. halted CPU for NumHWBP detection
HW instruction/data BPs: 8
Support set/clr BPs while running: No
HW data BPs trigger before execution of inst
RISC-V identified.
Halting CPU for downloading file.
Downloading file [/work/riot/RIOT/examples/blinky/bin/hifive1b/blinky.bin]...
Comparing flash   [100%] Done.
Erasing flash     [100%] Done.
Programming flash [100%] Done.
J-Link: Flash download: Bank 0 @ 0x20000000: 1 range affected (65536 bytes)
J-Link: Flash download: Total: 0.451s (Prepare: 0.049s, Compare: 0.037s, Erase: 0.169s, Program & Verify: 0.175s, Restore: 0.019s)
J-Link: Flash download: Program & Verify speed: 365 KB/s
O.K.

Reset delay: 0 ms
Reset type Normal: Resets core & peripherals using <ndmreset> bit in <dmcontrol> debug register.
RISC-V: Performing reset via <ndmreset>



Script processing completed.

/work/riot/RIOT/dist/tools/pyterm/pyterm -p "/dev/ttyACM0" -b "115200"  
Twisted not available, please install it if you want to use pyterm's JSON capabilities
2021-12-15 12:42:35,915 # Connect to serial port /dev/ttyACM0
Welcome to pyterm!
Bench Clock Reset Complete
2021-12-15 12:42:36,922 # 
2021-12-15 12:42:36,922 # ATE0-->ATE0
2021-12-15 12:42:36,923 # OK
2021-12-15 12:42:36,923 # AT+BLEINIT=0-->OK
2021-12-15 12:42:36,985 # AT+CWMODE=0-->OK
2021-12-15 12:42:36,985 # 
2021-12-15 12:42:36,994 # main(): This is RIOT! (Version: 2022.01-devel-1289-gc64b0-pr/sysclk)
2021-12-15 12:42:39,686 # Exiting Pyterm

(And the LED is blinking)

kfessel · 2021-12-15T12:00:10Z

@aabadie: did you disable the use of ztimer in blinkys-main?

blinky and tests/periph_uart_mode default to ztimer if it is available -> they will not test these changes by default

This allows access to CLOCK_CORECLOCK from periph_conf.h which is used by clk.h

aabadie · 2021-12-15T12:17:18Z

did you disable the use of ztimer in blinkys-main?

Not in the first place but I just did and got the same result.

aabadie · 2021-12-15T12:17:35Z

All last comments are addressed and directly squashed.

gschorcht · 2021-12-15T12:42:58Z

@gschorcht are the esp changes fine?

For ESPs there no code changes, only two additional includes that are correct.

kfessel · 2021-12-15T14:00:43Z

lets ping @fjmolinas for the ack

fjmolinas

ACK from my side, I tested as much as I could.

aabadie requested review from bergzand, fjmolinas, kaspar030, leandrolanzieri, maribu, MichelRottleuthner and smlng as code owners December 5, 2021 11:30

fjmolinas added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Dec 6, 2021

aabadie force-pushed the pr/sysclk branch from e940456 to c4ec728 Compare December 6, 2021 08:46

aabadie requested a review from gschorcht as a code owner December 6, 2021 09:59

github-actions bot added the Platform: ESP Platform: This PR/issue effects ESP-based platforms label Dec 6, 2021

aabadie force-pushed the pr/sysclk branch from 2c73038 to 8f33f38 Compare December 6, 2021 10:05

github-actions bot added the Area: boards Area: Board ports label Dec 6, 2021

aabadie force-pushed the pr/sysclk branch from 0b21e1e to de5a9a8 Compare December 6, 2021 12:25

kaspar030 reviewed Dec 6, 2021

View reviewed changes

aabadie force-pushed the pr/sysclk branch from de5a9a8 to 839f277 Compare December 6, 2021 13:32

aabadie force-pushed the pr/sysclk branch from 839f277 to c7a31c9 Compare December 6, 2021 13:52

fjmolinas reviewed Dec 15, 2021

View reviewed changes

kfessel reviewed Dec 15, 2021

View reviewed changes

examples/blinky/main.c Outdated Show resolved Hide resolved

pkg/openwsn/contrib/leds.c Outdated Show resolved Hide resolved

kfessel reviewed Dec 15, 2021

View reviewed changes

tests/periph_uart_mode/main.c Outdated Show resolved Hide resolved

aabadie added 2 commits December 15, 2021 12:41

sys: introduce coreclk utility function

ec41c43

cpu/native: define default CLOCK_CORECLOCK constant

b206658

aabadie added 8 commits December 15, 2021 13:14

cpu/fe310: use coreclk instead of cpu_freq

2eb800c

tests: use coreclk() instead of CLOCK_CORECLOCK

ea3c59f

drivers/ws281x: use coreclk() instead of CLOCK_CORECLOCK

16baad8

examples/blinky: use coreclk() instead of CLOCK_CORECLOCK

91927c9

pkg/openwsn: use coreclk() instead of CLOCK_CORECLOCK

084e33d

cpu/esp32: include sdk_conf.h in periph_cpu.h

2e74e5e

This allows access to CLOCK_CORECLOCK from periph_conf.h which is used by clk.h

cpu/esp8266: include cpu_conf.h in periph_cpu.h

c9207e8

boards/qn9080dk: move CLOCK_CORECLOCK definition to periph_conf.h

d755c3a

aabadie force-pushed the pr/sysclk branch from dbd873f to d755c3a Compare December 15, 2021 12:14

fjmolinas approved these changes Dec 15, 2021

View reviewed changes

aabadie merged commit 9c71dd7 into RIOT-OS:master Dec 15, 2021

aabadie deleted the pr/sysclk branch December 15, 2021 14:41

aabadie mentioned this pull request Jan 17, 2022

cpu/esp32: use macros/units #16341

Merged

fjmolinas added this to the Release 2022.01 milestone Jan 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

sys: introduce sysclk function to retrieve core clock frequency #17342

sys: introduce sysclk function to retrieve core clock frequency #17342

aabadie commented Dec 5, 2021

fjmolinas commented Dec 6, 2021

kfessel commented Dec 6, 2021 •

edited

Loading

kaspar030 Dec 6, 2021

aabadie Dec 6, 2021

maribu Dec 6, 2021

aabadie Dec 6, 2021

aabadie commented Dec 6, 2021

kfessel commented Dec 6, 2021 •

edited

Loading

fjmolinas Dec 15, 2021

fjmolinas Dec 15, 2021

fjmolinas commented Dec 15, 2021

fjmolinas left a comment

fjmolinas left a comment

aabadie commented Dec 15, 2021

kfessel left a comment

aabadie commented Dec 15, 2021

kfessel commented Dec 15, 2021 •

edited

Loading

aabadie commented Dec 15, 2021

aabadie commented Dec 15, 2021

gschorcht commented Dec 15, 2021

kfessel commented Dec 15, 2021

fjmolinas left a comment

		cpu_coreclk = PRCI_measure_mcycle_freq(3000, RTC_FREQ);
		cpu_coreclk = PRCI_measure_mcycle_freq(3000, RTC_FREQ);

	/* Ignore the first run (for icache reasons) */
	/* Ignore the first run (for icache reasons) */

sys: introduce sysclk function to retrieve core clock frequency #17342

sys: introduce sysclk function to retrieve core clock frequency #17342

Conversation

aabadie commented Dec 5, 2021

Contribution description

Testing procedure

Issues/PRs references

fjmolinas commented Dec 6, 2021

kfessel commented Dec 6, 2021 • edited Loading

kaspar030 Dec 6, 2021

Choose a reason for hiding this comment

aabadie Dec 6, 2021

Choose a reason for hiding this comment

maribu Dec 6, 2021

Choose a reason for hiding this comment

aabadie Dec 6, 2021

Choose a reason for hiding this comment

aabadie commented Dec 6, 2021

kfessel commented Dec 6, 2021 • edited Loading

fjmolinas Dec 15, 2021

Choose a reason for hiding this comment

fjmolinas Dec 15, 2021

Choose a reason for hiding this comment

fjmolinas commented Dec 15, 2021

fjmolinas left a comment

Choose a reason for hiding this comment

fjmolinas left a comment

Choose a reason for hiding this comment

aabadie commented Dec 15, 2021

kfessel left a comment

Choose a reason for hiding this comment

aabadie commented Dec 15, 2021

kfessel commented Dec 15, 2021 • edited Loading

aabadie commented Dec 15, 2021

aabadie commented Dec 15, 2021

gschorcht commented Dec 15, 2021

kfessel commented Dec 15, 2021

fjmolinas left a comment

Choose a reason for hiding this comment

kfessel commented Dec 6, 2021 •

edited

Loading

kfessel commented Dec 6, 2021 •

edited

Loading

kfessel commented Dec 15, 2021 •

edited

Loading