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.

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

boards: fix the sys ticks per second for emsk #25406

Conversation

vonhust
Copy link

@vonhust vonhust commented May 18, 2020

As a slow FPGA platform with max. freq < 25 Mhz,
the default CON_SYS_CLOCK_TICKS_PER_SEC=10000 is
not suitable, will bring some failures in sanitycheck tests(tests/kernel/timer/timer_api, tests/kernel/sched/schedule_api) . CON_SYS_CLOCK_TICKS_PER_SEC=100 is
a better value.

Signed-off-by: Wayne Ren wei.ren@synopsys.com

Fixes #25511

As a slow FPGA platform with max. freq < 25 Mhz,
the default CON_SYS_CLOCK_TICKS_PER_SEC=10000 is
not suitable. CON_SYS_CLOCK_TICKS_PER_SEC=100 is
a better value.

Signed-off-by: Wayne Ren <wei.ren@synopsys.com>
Copy link
Collaborator

@abrodkin abrodkin left a comment

Choose a reason for hiding this comment

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

Looking at how that config option is defined in kernel/Kconfig I'm not sure we do the right thing here.
So here's the definition:

config SYS_CLOCK_TICKS_PER_SEC
	int "System tick frequency (in ticks/second)"
	default 100 if QEMU_TARGET || SOC_POSIX
	default 10000 if TICKLESS_CAPABLE
	default 100

See SYS_CLOCK_TICKS_PER_SEC is only set to that higher 10.000 value if the timer used is TICKLESS_CAPABLE, otherwise we default to 100 (which we enforce in this patch).

That said don't we want to disable TICKLESS_CAPABLE for such a slow FPGA-based platfrom instead?
I.e. add a condition like that (though I guess this example is not technically correct):

diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig
index 2c7fdf2c47..3a7d1a1cf9 100644
--- a/drivers/timer/Kconfig
+++ b/drivers/timer/Kconfig
@@ -98,7 +98,7 @@ menuconfig ARCV2_TIMER
        bool "ARC Timer"
        default y
        depends on ARC
-       select TICKLESS_CAPABLE
+       select TICKLESS_CAPABLE if !BOARD_EM_STARTERKIT
        help
          This module implements a kernel device driver for the ARCv2 processor timer 0
          and provides the standard "system clock driver" interfaces.

But something along the lines.

Now if we look at other ARC boards we may see this:

boards/arc/emsdp/emsdp_defconfig:7:CONFIG_SYS_CLOCK_TICKS_PER_SEC=100
boards/arc/emsdp/emsdp_em4_defconfig:7:CONFIG_SYS_CLOCK_TICKS_PER_SEC=100
boards/arc/emsdp/emsdp_em5d_defconfig:7:CONFIG_SYS_CLOCK_TICKS_PER_SEC=100
boards/arc/emsdp/emsdp_em6_defconfig:7:CONFIG_SYS_CLOCK_TICKS_PER_SEC=100
boards/arc/emsdp/emsdp_em7d_defconfig:7:CONFIG_SYS_CLOCK_TICKS_PER_SEC=100
boards/arc/emsdp/emsdp_em7d_esp_defconfig:7:CONFIG_SYS_CLOCK_TICKS_PER_SEC=100
boards/arc/emsdp/emsdp_em9d_defconfig:7:CONFIG_SYS_CLOCK_TICKS_PER_SEC=100
boards/arc/hsdk/hsdk_defconfig:6:CONFIG_SYS_CLOCK_TICKS_PER_SEC=100
boards/arc/nsim/nsim_em_defconfig:7:CONFIG_SYS_CLOCK_TICKS_PER_SEC=100
boards/arc/nsim/nsim_hs_defconfig:7:CONFIG_SYS_CLOCK_TICKS_PER_SEC=100
boards/arc/nsim/nsim_hs_smp_defconfig:7:CONFIG_SYS_CLOCK_TICKS_PER_SEC=100
boards/arc/nsim/nsim_sem_defconfig:7:CONFIG_SYS_CLOCK_TICKS_PER_SEC=100
boards/arc/nsim/nsim_sem_mpu_stack_guard_defconfig:7:CONFIG_SYS_CLOCK_TICKS_PER_SEC=100

So even HSDK has CONFIG_SYS_CLOCK_TICKS_PER_SEC force set to 100.
The same goes to nSIM platform - is that exactly what we want?

@ruuddw
Copy link
Member

ruuddw commented May 18, 2020

Tickless makes sense (even more) for FPGA/slower targets. The problem for this test is that it assumes two timer related function calls can be made within one tick period. That's not realistic in this case, so it makes sense to increase the tick period (10k ticks @25mhz -> 2500 cycles for 1 period; with userspace syscall overhead, locking etc. that's simply not enough).

100 is on the safe side for TICKLESS, imho 1000 would be a better, maybe 5000 works as well.

As Alexey points out, a consistent similar definition for the other ARC boards would be good.

@vonhust
Copy link
Author

vonhust commented May 19, 2020

@ruuddw is right, we can enable TICKLESS for simulators, 1000 or 10000 can improve the accuracy of timeout. The root cause is the tests which assume some operation should be finished in one tick, that's possible for high-freq targets like iotdk/hsdk, but too fast for slow targets, especially for emsk+ddr ram +userspace (BTW: iotdk also may fail, if slow memory is used)

I think 100 ticks per second is a safe value to cover different cases

Copy link
Collaborator

@abrodkin abrodkin left a comment

Choose a reason for hiding this comment

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

LGTM after discussion and explanation in this thread.

Copy link
Member

@ruuddw ruuddw left a comment

Choose a reason for hiding this comment

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

100 is safe, 1000 would be better for testing/accuracy (might catch issues on timer accuracy problems).
As this is a bugfix, please add an issue and appropriate labels to get this in 2.3.

@carlescufi carlescufi merged commit d72d903 into zephyrproject-rtos:master May 21, 2020
@vonhust vonhust deleted the topic-fix-emsk branch May 26, 2020 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

arc em_starterkit_em11d failed in tests/kernel/timer/timer_api
5 participants