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

Jump to bottom

Remove direct usage of CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC #15619

Closed
wants to merge 6 commits into from
Closed

Remove direct usage of CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC #15619

wants to merge 6 commits into from

Conversation

pizi-nordic
Copy link
Collaborator

@pizi-nordic pizi-nordic commented Apr 23, 2019 •
edited
Loading

On some SoCs the frequency of the system clock is obtained at run time
as the exact configuration of the hardware is not known at compile time.
On such platforms using CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC define directly
introduces timing errors.

This PR replaces CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC by the call to
inline function sys_clock_hw_cycles_per_sec() which always returns correct
frequency of the system clock.

Brings us closer to: #15363

@zephyrbot
Copy link
Collaborator

zephyrbot commented Apr 23, 2019 •
edited
Loading

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

@pizi-nordic pizi-nordic force-pushed the remove-direct-usage-of-CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC branch 2 times, most recently from 077ef66 to 6d7a01e Compare April 23, 2019 13:37
Sizurka
Sizurka suggested changes Apr 23, 2019
View reviewed changes
Copy link
Collaborator

@Sizurka Sizurka left a comment

Choose a reason for hiding this comment

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

This fails to build on SAM0 due to static assert failures in the RTC.

drivers/timer/sam0_rtc_timer.c Outdated
@@ -23,7 +23,7 @@
#define RTC0 ((RtcMode0 *) DT_RTC_SAM0_BASE_ADDRESS)

/* Number of sys timer cycles per on tick. */
#define CYCLES_PER_TICK (CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC \
#define CYCLES_PER_TICK (sys_clock_hw_cycles_per_sec() \
Copy link
Collaborator

Choose a reason for hiding this comment

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

This fails to build on SAM0, since the BUILD_ASSERT(s) below are no longer reference a constant.

In this case it should likely be SOC_ATMEL_SAM0_GCLK0_FREQ_HZ since that's the actual source of the RTC clock, currently (it's also the source of the undivided CPU clock, so it works out either way at present).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed. Thank you. Please check if the fix is correct.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks good to me now.

@pizi-nordic pizi-nordic force-pushed the remove-direct-usage-of-CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC branch 2 times, most recently from 49abe29 to 28a956b Compare April 24, 2019 09:36
@pizi-nordic
drivers: i2c_bitbang: Do not use CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC
18ad090 
On some SoCs the frequency of the system clock is obtained at run time
as the exact configuration of the hardware is not known at compile time.
On such platforms using CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC define
directly introduces timing errors.

This commit replaces CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC by the call to
inline function sys_clock_hw_cycles_per_sec() which always returns
correct frequency of the system clock.

Signed-off-by: Piotr Zięcik <[email protected]>
@pizi-nordic
drivers: pwm_qmsi: Do not use CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC
c42e89e 
On some SoCs the frequency of the system clock is obtained at run time
as the exact configuration of the hardware is not known at compile time.
On such platforms using CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC define
directly introduces timing errors.

This commit replaces CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC by the call
to inline function sys_clock_hw_cycles_per_sec() which always returns
correct frequency of the system clock.

Signed-off-by: Piotr Zięcik <[email protected]>
@pizi-nordic
drivers: timer: Do not use CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC
643294c 
On some SoCs the frequency of the system clock is obtained at run time
as the exact configuration of the hardware is not known at compile time.
On such platforms using CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC define
directly introduces timing errors.

This commit replaces CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC by the call
to inline function sys_clock_hw_cycles_per_sec() which always returns
correct frequency of the system clock.

Signed-off-by: Piotr Zięcik <[email protected]>
@pizi-nordic
debug: Do not use CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC
5420c85 
On some SoCs the frequency of the system clock is obtained at run time
as the exact configuration of the hardware is not known at compile time.
On such platforms using CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC define
directly introduces timing errors.

This commit replaces CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC by the call
to inline function sys_clock_hw_cycles_per_sec() which always returns
correct frequency of the system clock.

Signed-off-by: Piotr Zięcik <[email protected]>
@pizi-nordic pizi-nordic force-pushed the remove-direct-usage-of-CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC branch from 28a956b to 4165736 Compare April 24, 2019 10:24
@galak
Copy link
Collaborator

galak commented Apr 24, 2019

Aren't we missing a bunch of usages of CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC?

Just in drivers as an example:

[galak@spiff zephyr]$ git grep CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC drivers/
drivers/clock_control/beetle_clock_control.c:   .freq = CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC,
drivers/clock_control/stm32_ll_clock.c:  * SystemCoreClock is preferred to CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC
drivers/i2c/i2c_bitbang.c:      ((u64_t)CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC * (ns) / NSEC_PER_SEC + 1)
drivers/i2c/i2c_cc32xx.c:       MAP_I2CMasterInitExpClk(base, CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC,
drivers/pwm/pwm_qmsi.c:#define HW_CLOCK_CYCLES_PER_USEC  (CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC / \
drivers/serial/uart_cc32xx.c:   .sys_clk_freq = CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC,
drivers/serial/uart_cmsdk_apb.c:        .sys_clk_freq = CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC,
drivers/serial/uart_cmsdk_apb.c:        .sys_clk_freq = CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC,
drivers/serial/uart_cmsdk_apb.c:        .sys_clk_freq = CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC,
drivers/serial/uart_cmsdk_apb.c:        .sys_clk_freq = CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC,
drivers/serial/uart_cmsdk_apb.c:        .sys_clk_freq = CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC,
drivers/serial/uart_msp432p4xx.c:       .sys_clk_freq = CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC,
drivers/serial/uart_pl011.c:    ret = pl011_set_baudrate(dev, CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC,
drivers/serial/uart_pl011.c:    .sys_clk_freq = CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC,
drivers/serial/uart_pl011.c:    .sys_clk_freq = CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC,
drivers/serial/uart_qmsi.c:     ((CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC / (16 * baudrate)) & 0xFF)
drivers/serial/uart_qmsi.c:     (((CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC / (16 * baudrate)) & 0xFF00) >> 8)
drivers/spi/spi_dw.h:           ((CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC / ssi_clk_hz) & 0xFFFF)
drivers/timer/Kconfig:    value of CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC will be used instead;
drivers/timer/arcv2_timer0.c:#define CYC_PER_TICK (CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC   \
drivers/timer/cortex_m_systick.c:#define CYC_PER_TICK (CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC       \
drivers/timer/nrf_rtc_timer.c:#define CYC_PER_TICK (CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC  \
drivers/timer/riscv_machine_timer.c:#define CYC_PER_TICK ((u32_t)((u64_t)CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC     \
drivers/timer/rv32m1_lptmr_timer.c:#define CYCLES_PER_SEC  CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC
drivers/timer/sam0_rtc_timer.c:#define CYCLES_PER_TICK (CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC \
drivers/timer/xtensa_sys_timer.c:#define CYC_PER_TICK (CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC       \
drivers/watchdog/wdog_cmsdk_apb.c:      reload_s = config->window.max * CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC;

nordic-krch
nordic-krch requested changes Apr 24, 2019
View reviewed changes
subsys/logging/log_core.c Outdated
@@ -282,7 +282,7 @@ void log_hexdump_sync(struct log_msg_ids src_level, const char *metadata,

static u32_t timestamp_get(void)
Copy link
Contributor

Choose a reason for hiding this comment

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

in that form it will not be optimal as on every timestamp_get there will be runtime evaluation of cycles per sec.
You can remove that function and instead do something like in log_core_init:

timestamp_func = (sys_clock_hw_cycles_per_sec() > 1000000) ? k_uptime_get_32 : k_cycle_get_32;

which replaces:

timestamp_func = timestamp_get;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated. Please check the solution.

@pizi-nordic
Copy link
Collaborator Author

pizi-nordic commented Apr 25, 2019 •
edited
Loading

@galak: Please look at PRs linked to #15363.
I am trying to separate changes as one big PR might be hard to review.

@pizi-nordic pizi-nordic force-pushed the remove-direct-usage-of-CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC branch 2 times, most recently from 6100320 to fccd2b8 Compare April 25, 2019 09:52
@pizi-nordic
logging: Do not use CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC
d0ec1fb 
On some SoCs the frequency of the system clock is obtained at run time
as the exact configuration of the hardware is not known at compile time.
On such platforms using CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC define
directly introduces timing errors.

This commit replaces CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC by the call
to inline function sys_clock_hw_cycles_per_sec() which always returns
correct frequency of the system clock.

Signed-off-by: Piotr Zięcik <[email protected]>
@pizi-nordic
tests: benchmarks: Do not use CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC
e03f705 
On some SoCs the frequency of the system clock is obtained at run time
as the exact configuration of the hardware is not known at compile time.
On such platforms using CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC define
directly introduces timing errors.

This commit replaces CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC by the call
to inline function sys_clock_hw_cycles_per_sec() which always returns
correct frequency of the system clock.

Signed-off-by: Piotr Zięcik <[email protected]>
@pizi-nordic pizi-nordic force-pushed the remove-direct-usage-of-CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC branch from fccd2b8 to e03f705 Compare April 25, 2019 10:08
@pizi-nordic pizi-nordic added the dev-review To be discussed in dev-review meeting label May 2, 2019
@nashif nashif removed dev-review To be discussed in dev-review meeting labels May 2, 2019
@pizi-nordic pizi-nordic reopened this May 28, 2019
@pizi-nordic pizi-nordic deleted the remove-direct-usage-of-CONFIG_SYS_CLOCK_HW_CYCLES_PER_SEC branch May 29, 2019 11:03
@pizi-nordic pizi-nordic mentioned this pull request May 29, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sizurka Sizurka Sizurka approved these changes

@nordic-krch nordic-krch nordic-krch approved these changes

@carlescufi carlescufi Awaiting requested review from carlescufi

@ioannisg ioannisg Awaiting requested review from ioannisg

@nashif nashif Awaiting requested review from nashif

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@pizi-nordic @zephyrbot @galak @nordic-krch @Sizurka @nashif