Decouple timers from system clock #16958

carlescufi · 2019-06-20T14:29:25Z

Second split out from #16469. Includes #16957.
Do not merge until #16957 is merged

The clock control initialization code used system clock frequency as a CPU clock frequency. This commit corrects that by obtaining the needed value from DTS. Signed-off-by: Piotr Zięcik <[email protected]>

The log_backend_swo used system clock frequency as a base for SWO clock calculation. This commit corrects that by obtaining the needed value from DTS. Signed-off-by: Piotr Zięcik <[email protected]>

The spi_dw driver used system clock frequency as a base for SPI bus frequency calculation. This commit corrects that by obtaining the needed value from DTS. Signed-off-by: Piotr Zięcik <[email protected]>

The uart_cmsdk_apb driver used system clock frequency as a base for baudrate calculation. This commit corrects that by obtaining the needed value from DTS. Signed-off-by: Piotr Zięcik <[email protected]>

The uart_pl011 driver used system clock frequency as a base for baudrate calculation. This commit corrects that by obtaining the needed value from DTS. Signed-off-by: Piotr Zięcik <[email protected]>

The uart_cc32xx driver used system clock frequency as a base for baudrate calculation. This commit corrects that by obtaining the needed value from DTS. Signed-off-by: Piotr Zięcik <[email protected]>

The uart_msp432p4xx driver used system clock frequency as a base for baudrate calculation. This commit corrects that by obtaining the needed value from DTS. Signed-off-by: Piotr Zięcik <[email protected]>

The uart_qmsi driver used system clock frequency as a base for baudrate calculation. This commit corrects that by obtaining the needed value from DTS. Signed-off-by: Piotr Zięcik <[email protected]>

The uart_stellaris driver used system clock frequency as a base for baudrate calculation. This commit corrects that by obtaining the needed value from DTS. Signed-off-by: Piotr Zięcik <[email protected]>

The uart_cc13xx_cc26xx driver used system clock frequency as a base for baudrate calculation. This commit corrects that by obtaining the needed value from DTS. Signed-off-by: Piotr Zięcik <[email protected]>

The wdog_cmsdk_apb driver used system clock frequency as a base for timeout calculation. This commit corrects that by obtaining the needed value from DTS. Signed-off-by: Piotr Zięcik <[email protected]>

The adc_stm32 driver used system timer frequency as a base for busy-wait delay calculation. This commit corrects that by obtaining the needed value from SystemCoreClock variable. Signed-off-by: Piotr Zięcik <[email protected]>

This commit adds the kernel.h include in order to provide sys_clock_hw_cycles_per_sec() declaration. Signed-off-by: Piotr Zięcik <[email protected]>

erwango · 2019-07-22T13:49:34Z

@galak

@erwango drivers/adc/adc_stm32.c why use SystemCoreClock?

We use SystemCoreClock as on STM devices clock freq changes between boot time and run time. SystemCoreClock is updated to hold the current frequency. Note that frequency change happens only at clock_control driver init:

Default boot time frequency use to be a default low speed freq (SystemCoreClock is set in soc.c)
High speed freq is set at clock_control driver init when configuring RCC IP (PLL configuration for instance). SystemCoreClock is then updated to the configured freq.

galak · 2019-07-22T21:11:07Z

We use SystemCoreClock as on STM devices clock freq changes between boot time and run time. SystemCoreClock is updated to hold the current frequency. Note that frequency change happens only at clock_control driver init:

Default boot time frequency use to be a default low speed freq (SystemCoreClock is set in soc.c)

High speed freq is set at clock_control driver init when configuring RCC IP (PLL configuration for instance). SystemCoreClock is then updated to the configured freq.

So does that mean sys_clock_hw_cycles_per_sec will report the wrong thing?

erwango · 2019-07-23T08:01:01Z

@galak

So does that mean sys_clock_hw_cycles_per_sec will report the wrong thing?

This is indeed the case before clock_control driver init (done before kernel init).
So, looking into it more, I don't think we should face many issues using sys_clock_hw_cycles_per_sec instead of SystemCoreClock.

Also, practically, I'm not 100% sure CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC holds the correct information (since it was not used). So it would have to be checked vs boards clock configuration.

erwango · 2019-07-23T08:40:41Z

@galak

Anyway, point is for now STM32 don't use CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC, directly or indirectly (since sys_clock_hw_cycles_per_sec is still using CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC value), so we're in line with current PR.

I agree STM32 should also move to clock_control configuration to DT, and eventually get rid of SystemCoreClock and use sys_clock_hw_cycles_per_sec instead (once it will return dt value and/or able to return run time updated frequency). But I don't think this should block current PR.

galak · 2019-07-23T18:22:55Z

Do any of these still need cleaning up:

drivers/clock_control/clock_stm32_ll_common.c:	 * SystemCoreClock is preferred to CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC
drivers/clock_control/clock_stm32_ll_h7.c:	 * SystemCoreClock is preferred to CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC
drivers/clock_control/clock_stm32_ll_h7.c:	SysTick_Config(CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC / 1000);
drivers/clock_control/clock_stm32_ll_h7.c:	SystemCoreClock = CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC;
drivers/timer/Kconfig:	  TSC frequency to CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC (the local APIC
drivers/timer/Kconfig:	  value of CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC will be used instead;
drivers/timer/apic_timer.c: * CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC=<hz> must contain the frequency seen
drivers/timer/apic_timer.c: *     the TSC frequency to CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC. This can be
drivers/timer/apic_timer.c:	(CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC / CONFIG_SYS_CLOCK_TICKS_PER_SEC)
drivers/timer/rv32m1_lptmr_timer.c:    (MHZ(8) != CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC)
include/sys_clock.h:	return CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC;
include/sys_clock.h:	(CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC == 0)
include/sys_clock.h:	(CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC % CONFIG_SYS_CLOCK_TICKS_PER_SEC)
kernel/timeout.c:int z_clock_hw_cycles_per_sec = CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC;
subsys/testsuite/ztest/include/ztest.h:#define CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC 10000000
tests/benchmarks/boot_time/testcase.yaml:    filter: CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC >= 1000000

carlescufi · 2019-07-24T13:09:55Z

Do any of these still need cleaning up:

drivers/clock_control/clock_stm32_ll_common.c:	 * SystemCoreClock is preferred to CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC
drivers/clock_control/clock_stm32_ll_h7.c:	 * SystemCoreClock is preferred to CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC
drivers/clock_control/clock_stm32_ll_h7.c:	SysTick_Config(CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC / 1000);
drivers/clock_control/clock_stm32_ll_h7.c:	SystemCoreClock = CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC;
drivers/timer/Kconfig:	  TSC frequency to CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC (the local APIC
drivers/timer/Kconfig:	  value of CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC will be used instead;
drivers/timer/apic_timer.c: * CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC=<hz> must contain the frequency seen
drivers/timer/apic_timer.c: *     the TSC frequency to CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC. This can be
drivers/timer/apic_timer.c:	(CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC / CONFIG_SYS_CLOCK_TICKS_PER_SEC)
drivers/timer/rv32m1_lptmr_timer.c:    (MHZ(8) != CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC)
include/sys_clock.h:	return CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC;
include/sys_clock.h:	(CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC == 0)
include/sys_clock.h:	(CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC % CONFIG_SYS_CLOCK_TICKS_PER_SEC)
kernel/timeout.c:int z_clock_hw_cycles_per_sec = CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC;
subsys/testsuite/ztest/include/ztest.h:#define CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC 10000000
tests/benchmarks/boot_time/testcase.yaml:    filter: CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC >= 1000000

From what I can tell, no, but we need to confirm it. Since Piotr is away what I will do is merge this PR to avoid it getting stale, and make sure he takes a look at the list once he's back.

pizi-nordic · 2019-08-12T12:17:22Z

./drivers/clock_control/clock_stm32_ll_common.c:         * SystemCoreClock is preferred to CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC
./drivers/clock_control/clock_stm32_ll_h7.c:     * SystemCoreClock is preferred to CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC
./drivers/clock_control/clock_stm32_ll_h7.c:    SysTick_Config(CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC / 1000);
./drivers/clock_control/clock_stm32_ll_h7.c:    SystemCoreClock = CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC;

The code above configures the clocking tree. Ideally, the information to perform this task should be taken from DTS, not the CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC. On the other hand, it also configures SysTick, which has to be set basing on CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC. I do not know STM SoC enough to say if the two things mentioned above could be easily decoupled.

./drivers/timer/Kconfig:          TSC frequency to CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC (the local APIC
./drivers/timer/Kconfig:          value of CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC will be used instead;
./drivers/timer/apic_timer.c: * CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC=<hz> must contain the frequency seen
./drivers/timer/apic_timer.c: *     the TSC frequency to CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC. This can be
./drivers/timer/apic_timer.c:   (CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC / CONFIG_SYS_CLOCK_TICKS_PER_SEC)
./drivers/timer/mchp_xec_rtos_timer.c:BUILD_ASSERT_MSG(CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC == 32768,
./drivers/timer/mchp_xec_rtos_timer.c: * CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC=<hz> must be set to 32768.
./drivers/timer/mchp_xec_rtos_timer.c:  (CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC / CONFIG_SYS_CLOCK_TICKS_PER_SEC)
./drivers/timer/mchp_xec_rtos_timer.c:#define CPT1000 ((CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC * 1000UL) \
./drivers/timer/rv32m1_lptmr_timer.c:    (MHZ(8) != CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC)

./include/sys_clock.h:  return CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC;
./include/sys_clock.h:  (CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC == 0)
./include/sys_clock.h:  (CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC % CONFIG_SYS_CLOCK_TICKS_PER_SEC)

./kernel/timeout.c:int z_clock_hw_cycles_per_sec = CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC;

These are parts of system timer implementation. The value of CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC has to be known there in order to configure hardware for given frequency.

./subsys/testsuite/ztest/include/ztest.h:#define CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC 10000000

Here I do not know why the value of CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC is used. Maybe someone knowing ztest will tell us why this is not taken form the board configuration.

./tests/benchmarks/boot_time/testcase.yaml:    filter: CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC >= 1000000

This looks like hack ensuring precise measurement of time. I am not sure it this is still needed after @andyross fixes in the area.

carlescufi requested review from andrewboie, andyross, dcpleung, fallrisk, galak, ioannisg, jakub-uC, kgugala, Mani-Sadhasivam, MaureenHelm, nashif, nategraff-sifive, nordic-krch, pgielda, ruuddw, tbursztyka, vanti and vonhust as code owners June 20, 2019 14:29

carlescufi removed request for vanti, fallrisk, tbursztyka, pgielda, andrewboie, kgugala, Mani-Sadhasivam, jakub-uC, dcpleung and nategraff-sifive June 20, 2019 14:29

carlescufi mentioned this pull request Jun 20, 2019

Decouple system timer and drivers #16469

Closed

carlescufi added the DNM This PR should not be merged (Do Not Merge) label Jun 20, 2019

pizi-nordic added 13 commits July 18, 2019 13:04

drivers: beetle_clock_control: Get CPU clock frequency from DTS

c8c9cf3

The clock control initialization code used system clock frequency as a CPU clock frequency. This commit corrects that by obtaining the needed value from DTS. Signed-off-by: Piotr Zięcik <[email protected]>

logging: log_backend_swo: Get clock frequency from DTS

75d1fc7

The log_backend_swo used system clock frequency as a base for SWO clock calculation. This commit corrects that by obtaining the needed value from DTS. Signed-off-by: Piotr Zięcik <[email protected]>

drivers: spi_dw: Get clock frequency from DTS

2b2800d

The spi_dw driver used system clock frequency as a base for SPI bus frequency calculation. This commit corrects that by obtaining the needed value from DTS. Signed-off-by: Piotr Zięcik <[email protected]>

drivers: uart_cmsdk_apb: Get clock frequency from DTS

9000a51

The uart_cmsdk_apb driver used system clock frequency as a base for baudrate calculation. This commit corrects that by obtaining the needed value from DTS. Signed-off-by: Piotr Zięcik <[email protected]>

drivers: uart_pl011: Get clock frequency from DTS

0e25db3

The uart_pl011 driver used system clock frequency as a base for baudrate calculation. This commit corrects that by obtaining the needed value from DTS. Signed-off-by: Piotr Zięcik <[email protected]>

drivers: uart_cc32xx: Get clock frequency from DTS

3098758

The uart_cc32xx driver used system clock frequency as a base for baudrate calculation. This commit corrects that by obtaining the needed value from DTS. Signed-off-by: Piotr Zięcik <[email protected]>

drivers: uart_msp432p4xx: Get clock frequency from DTS

8219a3a

The uart_msp432p4xx driver used system clock frequency as a base for baudrate calculation. This commit corrects that by obtaining the needed value from DTS. Signed-off-by: Piotr Zięcik <[email protected]>

drivers: uart_qmsi: Get clock frequency from DTS

7ae312a

The uart_qmsi driver used system clock frequency as a base for baudrate calculation. This commit corrects that by obtaining the needed value from DTS. Signed-off-by: Piotr Zięcik <[email protected]>

drivers: uart_stellaris: Get clock frequency from DTS

1ef851b

The uart_stellaris driver used system clock frequency as a base for baudrate calculation. This commit corrects that by obtaining the needed value from DTS. Signed-off-by: Piotr Zięcik <[email protected]>

drivers: uart_cc13xx_cc26xx: Get clock frequency from DTS

d5db8cf

The uart_cc13xx_cc26xx driver used system clock frequency as a base for baudrate calculation. This commit corrects that by obtaining the needed value from DTS. Signed-off-by: Piotr Zięcik <[email protected]>

drivers: wdog_cmsdk_apb: Get clock frequency from DTS

bccdf56

The wdog_cmsdk_apb driver used system clock frequency as a base for timeout calculation. This commit corrects that by obtaining the needed value from DTS. Signed-off-by: Piotr Zięcik <[email protected]>

drivers: adc_stm32: Get clock frequency from DTS

efebd07

The adc_stm32 driver used system timer frequency as a base for busy-wait delay calculation. This commit corrects that by obtaining the needed value from SystemCoreClock variable. Signed-off-by: Piotr Zięcik <[email protected]>

debug: tracing: Fix compilation error

7850fb5

This commit adds the kernel.h include in order to provide sys_clock_hw_cycles_per_sec() declaration. Signed-off-by: Piotr Zięcik <[email protected]>

pizi-nordic force-pushed the decouple-timers-v2 branch from 23f777d to 7850fb5 Compare July 18, 2019 13:40

nordic-krch approved these changes Jul 22, 2019

View reviewed changes

erwango approved these changes Jul 22, 2019

View reviewed changes

carlescufi removed the DNM This PR should not be merged (Do Not Merge) label Jul 22, 2019

galak approved these changes Jul 23, 2019

View reviewed changes

This was referenced Jul 24, 2019

stm32: Move clock configuration to device tree #17745

Closed

stm32: clock-control: Remove usage of SystemCoreClock #17748

Closed

carlescufi merged commit 4fc2445 into zephyrproject-rtos:master Jul 24, 2019

mrfuchs mentioned this pull request Jul 31, 2019

soc: stm32: add SWO logger backend support #17520

Merged

pabigot removed the dev-review To be discussed in dev-review meeting label Oct 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decouple timers from system clock #16958

Decouple timers from system clock #16958

carlescufi commented Jun 20, 2019 •

edited by pizi-nordic

Loading

erwango commented Jul 22, 2019

galak commented Jul 22, 2019

erwango commented Jul 23, 2019 •

edited

Loading

erwango commented Jul 23, 2019

galak commented Jul 23, 2019

carlescufi commented Jul 24, 2019

pizi-nordic commented Aug 12, 2019

Decouple timers from system clock #16958

Decouple timers from system clock #16958

Conversation

carlescufi commented Jun 20, 2019 • edited by pizi-nordic Loading

erwango commented Jul 22, 2019

galak commented Jul 22, 2019

erwango commented Jul 23, 2019 • edited Loading

erwango commented Jul 23, 2019

galak commented Jul 23, 2019

carlescufi commented Jul 24, 2019

pizi-nordic commented Aug 12, 2019

carlescufi commented Jun 20, 2019 •

edited by pizi-nordic

Loading

erwango commented Jul 23, 2019 •

edited

Loading