drivers: timer: silabs: Add sleeptimer timer driver #80227

asmellby · 2024-10-22T12:51:40Z

Add timer driver using the Sleeptimer HAL for use as OS timer. The Sleeptimer is the unified system timer on EFR32, and integrates with power management and radio subsystems.

The driver passes kernel timer tests on both xG24 and xG27:

<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
  <testsuite name="xg24_dk2601b" timestamp="2024-10-22T14:42:34.092791" time="70.08" tests="30" failures="0" errors="0" skipped="0">
    <properties>
      <property name="version" value="v3.7.0-4830-g0afaa8eb552d" />
    </properties>
    <testcase classname="kernel.timer" name="kernel.timer.timer.jitter_drift_timer_startdelay" time="19.73" />
    <testcase classname="kernel.timer" name="kernel.timer.timer.jitter_drift_timer_period" time="19.73" />
    <testcase classname="kernel.timer" name="kernel.timer.timer.one_tick_timer_train" time="10.36" />
    <testcase classname="kernel.timer" name="kernel.timer.error_case.timer_add_timeout" time="0.00" />
    <testcase classname="kernel.timer" name="kernel.timer.error_case.timer_user_data_set_null" time="0.04" />
    <testcase classname="kernel.timer" name="kernel.timer.error_case.timer_user_data_get_null" time="0.04" />
    <testcase classname="kernel.timer" name="kernel.timer.error_case.timer_expires_ticks_null" time="0.04" />
    <testcase classname="kernel.timer" name="kernel.timer.error_case.timer_remaining_ticks_null" time="0.05" />
    <testcase classname="kernel.timer" name="kernel.timer.error_case.timer_status_sync_null" time="0.04" />
    <testcase classname="kernel.timer" name="kernel.timer.error_case.timer_status_get_null" time="0.04" />
    <testcase classname="kernel.timer" name="kernel.timer.error_case.timer_stop_null" time="0.04" />
    <testcase classname="kernel.timer" name="kernel.timer.error_case.timer_start_null" time="0.04" />
    <testcase classname="kernel.timer" name="kernel.timer.timepoints.comparison" time="0.00" />
    <testcase classname="kernel.timer" name="kernel.timer.timepoints.timepoint_api" time="1.10" />
    <testcase classname="kernel.timer" name="kernel.timer.monotonic.timer" time="4.61" />
    <testcase classname="kernel.timer" name="kernel.timer.sleep_abs" time="0.10" />
    <testcase classname="kernel.timer" name="kernel.timer.timeout_abs" time="0.00" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_remaining" time="0.05" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_user_data" time="0.30" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_k_define" time="0.70" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_status_sync" time="0.36" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_status_get_anytime" time="0.33" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_status_get" time="0.00" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_periodicity" time="0.26" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_expirefn_null" time="0.33" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_period_k_forever" time="0.20" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_period_0" time="0.20" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_restart" time="0.28" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_duration_period" time="0.33" />
    <testcase classname="kernel.timer" name="kernel.timer.time_conversions" time="0.07" />
  </testsuite>
</testsuites>

<?xml version="1.0" encoding="UTF-8"?>
<testsuites>
  <testsuite name="xg27_dk2602a" timestamp="2024-10-22T14:38:07.639547" time="56.559999999999995" tests="30" failures="0" errors="0" skipped="0">
    <properties>
      <property name="version" value="v3.7.0-4830-g0afaa8eb552d" />
    </properties>
    <testcase classname="kernel.timer" name="kernel.timer.timer.jitter_drift_timer_startdelay" time="13.86" />
    <testcase classname="kernel.timer" name="kernel.timer.timer.jitter_drift_timer_period" time="13.88" />
    <testcase classname="kernel.timer" name="kernel.timer.timer.one_tick_timer_train" time="10.36" />
    <testcase classname="kernel.timer" name="kernel.timer.error_case.timer_add_timeout" time="0.00" />
    <testcase classname="kernel.timer" name="kernel.timer.error_case.timer_user_data_set_null" time="0.04" />
    <testcase classname="kernel.timer" name="kernel.timer.error_case.timer_user_data_get_null" time="0.04" />
    <testcase classname="kernel.timer" name="kernel.timer.error_case.timer_expires_ticks_null" time="0.04" />
    <testcase classname="kernel.timer" name="kernel.timer.error_case.timer_remaining_ticks_null" time="0.05" />
    <testcase classname="kernel.timer" name="kernel.timer.error_case.timer_status_sync_null" time="0.04" />
    <testcase classname="kernel.timer" name="kernel.timer.error_case.timer_status_get_null" time="0.04" />
    <testcase classname="kernel.timer" name="kernel.timer.error_case.timer_stop_null" time="0.04" />
    <testcase classname="kernel.timer" name="kernel.timer.error_case.timer_start_null" time="0.04" />
    <testcase classname="kernel.timer" name="kernel.timer.timepoints.comparison" time="0.00" />
    <testcase classname="kernel.timer" name="kernel.timer.timepoints.timepoint_api" time="1.10" />
    <testcase classname="kernel.timer" name="kernel.timer.monotonic.timer" time="4.02" />
    <testcase classname="kernel.timer" name="kernel.timer.sleep_abs" time="0.10" />
    <testcase classname="kernel.timer" name="kernel.timer.timeout_abs" time="0.01" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_remaining" time="0.05" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_user_data" time="0.30" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_k_define" time="0.70" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_status_sync" time="0.36" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_status_get_anytime" time="0.33" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_status_get" time="0.00" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_periodicity" time="0.26" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_expirefn_null" time="0.33" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_period_k_forever" time="0.20" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_period_0" time="0.20" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_restart" time="0.28" />
    <testcase classname="kernel.timer" name="kernel.timer.timer_duration_period" time="0.33" />
    <testcase classname="kernel.timer" name="kernel.timer.time_conversions" time="0.09" />
  </testsuite>
</testsuites>

This PR is stacked on top of #79415 and will be rebased once that PR is merged. Only the last 4 commits are unique to this PR.

zephyrbot · 2024-10-22T12:52:36Z

The following west manifest projects have been modified in this Pull Request:

Name	Old Revision	New Revision	Diff

Note: This message is automatically posted and updated by the Manifest GitHub Action.

jerome-pouiller · 2024-10-22T13:56:32Z

boards/silabs/dev_kits/sltb010a/thunderboard.dtsi

If I understand right, Silabs have 5 different counters (burtc, prortc, rtcc, sysrtc, timer). Depending of the SOC_FAMILY we use HAL choose the proper driver, that's it?

Timer is a high-frequency counter, the others are low-frequency.
Prortc is internal to the radio (protocol rtc), and devices with this one also has rtcc. Prortc integrates with protimer, the high-frequency radio protocol timer.
Devices with sysrtc don't have a prortc, the protimer is instead integrated with a second channel group in the single sysrtc.
All devices have burtc (backup rtc).

The sleeptimer HAL API is backed by SYSRTC on devices that have that peripheral, and RTCC on the ones that don't (by default -- it can also be backed by other timers based on config options that we currently don't expose in Zephyr).

I believe each of these counters should have its own driver and the instantiation of the right driver should done through the device tree.

However, this is clearly out of the scope of the current PR.

jerome-pouiller · 2024-10-22T14:03:16Z

drivers/timer/silabs_sleeptimer_timer.c

+/* Set to true when timer is initialized */
+static bool g_init;
+
+static sl_sleeptimer_timer_handle_t sys_timer;


I suggest to group all these variables in a struct (eg. struct sleeptimer_data).

Then, when possible, I suggest to try to pass it as argument (eg. data argument in sleeptimer_cb()) rather than using the global declaration.

It is also possible to only use the syscalls definitions to get the global instance and call the real function:

void sys_clock_set_timeout(int32_t ticks, bool idle) { return sleetimer_clock_set_timeout(g_sleeptimer_data, ticks, idle); }

Thus, we can keep global variables in a small place.

BTW, I tend to avoid storing value I am able to re-compute. Thus, the reviewer don't have to check all the places the fields is written in the code.

Will take a look at this tomorrow, agree that it makes things a bit less messy. The current implementation is very close to the existing gecko_burtc_timer.

jerome-pouiller · 2024-10-22T14:49:27Z

drivers/timer/silabs_sleeptimer_timer.c

+#include <zephyr/drivers/timer/system_timer.h>
+#include <zephyr/logging/log.h>
+
+#include <sl_atomic.h>


I don't see any call to sl_atomic. Is it useful?

jerome-pouiller · 2024-10-22T15:05:03Z

drivers/timer/silabs_sleeptimer_timer.c

+#define DT_RTC DT_COMPAT_GET_ANY_STATUS_OKAY(silabs_gecko_stimer)
+
+/* Ensure interrupt names don't expand to register interface struct pointers */
+#undef RTCC


I don't see any reference to RTCC in this file.
(That's said this symbol is super intrusive and it should be fixed in the upstream)

Yeah, it's annoying. The sleeptimer HAL requires us to connect the _IRQHandler function for the peripheral backing the implementation to the interrupt vector table, since Zephyr doesn't use the standard CMSIS vector table. Unfortunately, this function has a peripheral-specific name, rather than a generic alias like sl_sleeptimer_irq_handler.

The "least bad" way of doing this I came up with (while avoiding HAL-isms in DT) was to encode the interrupt name in the interrupt-names DT property, and use CONCAT(DT_IRQ(...), _IRQHandler) to construct the correct function name. However, this causes unwanted macro expansion for RTCC to the CMSIS struct pointer for the peripheral block. SYSRTC is unaffected, since its peripheral macro is SYSRTC, but its interrupt name is SYSRTC_APP. Hence the single #undef, since as you note, we don't actually want or need the RTCC struct pointer in this driver.

We should work with upstream to get the interrupt handler aliased to sl_sleeptimer_irq_handler, similar to how the HFXO manager driver already has a generic sl_hfxo_manager_irq_handler symbol for its interrupt handler.

jerome-pouiller · 2024-10-22T15:17:52Z

drivers/timer/silabs_sleeptimer_timer.c

+		 "(min is %u). Config: SYS_CLOCK_TICKS_PER_SEC is "
+		 "%d and timer frequency is %u",
+		 g_cyc_per_tick, MIN_DELAY_CYC, CONFIG_SYS_CLOCK_TICKS_PER_SEC,
+		 z_clock_hw_cycles_per_sec);


Is it also true if !CONFIG_TICKLESS_KERNEL?

I guess it's technically not true, since the timer isn't restarted/reconfigured, so the N clock cycle delay of writing to LF peripheral registers doesn't apply. I don't see any harm in keeping the restriction for now, though.

jerome-pouiller · 2024-10-22T15:37:03Z

drivers/timer/silabs_sleeptimer_timer.c

+	BUILD_ASSERT(CONFIG_SYS_CLOCK_TICKS_PER_SEC > 0,
+		     "Invalid CONFIG_SYS_CLOCK_TICKS_PER_SEC value");
+
+	g_cyc_per_tick = z_clock_hw_cycles_per_sec / CONFIG_SYS_CLOCK_TICKS_PER_SEC;


What happen when z_clock_hw_cycles_per_sec % CONFIG_SYS_CLOCK_TICKS_PER_SEC != 0? I believe we don't need to be very accurate. However, if this division is not an integer, we will accumulate an error that could generate problem.

Depending on the numbers this can actually be very inaccurate: #80149

But I wouldn't block this PR for that since all drivers are affected by this.

An easy way to fix it would be to raise an assert (in this current driver) if z_clock_hw_cycles_per_sec % CONFIG_SYS_CLOCK_TICKS_PER_SEC != 0.

What's the value of z_clock_hw_cycles_per_sec for this driver? For a tickless kernel, the default value of CONFIG_SYS_CLOCK_TICKS_PER_SEC is 10000.

z_clock_hw_cycles_per_sec is 32768, and CONFIG_SYS_CLOCK_TICKS_PER_SEC is 1024 (default set in SoC-level Kconfig), so the default configuration cleanly divides with no loss of accuracy.

While all SoCs seem to be doing that, it does violate the kconfig documentation:
Also the number of cycles per tick should be chosen so that 1 millisecond is exactly represented by an integral number of ticks

jerome-pouiller · 2024-10-22T15:39:41Z

drivers/timer/silabs_sleeptimer_timer.c

+	z_clock_hw_cycles_per_sec = sl_sleeptimer_get_timer_frequency();
+
+	BUILD_ASSERT(CONFIG_SYS_CLOCK_TICKS_PER_SEC > 0,
+		     "Invalid CONFIG_SYS_CLOCK_TICKS_PER_SEC value");


Can this happen?

For current SoCs, it cannot, since it gets a nonzero default value in SoC-level Kconfig. But for a new SoC where it isn't set by default, it's possible in the future.

nitpick: I tend to group BUILD_ASSERT() on top of the file to avoid to cluttering functions.

jerome-pouiller · 2024-10-22T15:50:48Z

drivers/timer/silabs_sleeptimer_timer.c

+
+	IRQ_CONNECT(DT_IRQ(DT_RTC, irq), DT_IRQ(DT_RTC, priority),
+		    CONCAT(DT_STRING_UPPER_TOKEN_BY_IDX(DT_RTC, interrupt_names, 0), _IRQHandler),
+		    0, 0);


sleeptimer and stimer look entangled. I wonder if this file shouldn't be merged with counter_gecko_stimer.c. SILABS_SLEEPTIMER_TIMER would become an option of COUNTER_GECKO_STIMER.

Yes, the counter API implementation called stimer is another user of the sleeptimer. I agree it deserves to get looked at for a refactor at some point, but the role of the counter API and the timer API are different, since the timer API provides kernel/OS timebase, while the counter API is "userspace" timers, so I don't want to entangle that work into this. The sleeptimer-based OS timer is urgently needed to fix issues with sleep on SYSRTC-based devices, since the radio, power manager and OS don't play nicely together when they don't have the same understanding of "time until next wakeup from deep sleep".

The DT node "stimer0" represented two different hardware timers, RTCC and SYSRTC. Use the correct peripheral names for the nodes. Add interrupt names and missing interrupt numbers. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>

Add OS timer implementation making use of the Sleeptimer HAL. Sleeptimer integrates tightly with the Silabs Power Manager HAL, and must be used as the OS timer to achieve optimal power consumption when using the radio. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>

Disable BURTC timer in board defconfigs, as it's no longer used. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>

Power Manager no longer requires the Counter driver. This seems to have been a hack to get the Sleeptimer HAL included in the build, as the Sleeptimer is the real dependency of the Silabs Power Manager HAL. Since Sleeptimer is now used for the OS timer, this hack is not needed. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>

jhedberg · 2024-10-23T11:06:16Z

@asmellby even though it's a singleton, any reason why you didn't go for representing the sleep timer through a struct device, a DT node, etc? That'd have resulted in a more "normal" looking driver implementation and you also wouldn't need to use SYS_INIT() (which AFAIK is on its way out from the project).

jhedberg · 2024-10-23T11:10:42Z

@asmellby even though it's a singleton, any reason why you didn't go for representing the sleep timer through a struct device, a DT node, etc? That'd have resulted in a more "normal" looking driver implementation and you also wouldn't need to use SYS_INIT() (which AFAIK is on its way out from the project).

I see you're kind of building on top of "silabs,gecko-stimer", however would it e.g. make sense to have the sleep timer as a child node of that, or use a phandle to tie them together?

asmellby · 2024-10-23T11:17:46Z

No other timer driver creates a struct device, as far as I can tell. The timer API is also not "normal" looking -- it's global functions rather than an API struct with function pointers, and those functions don't give you a struct device pointer to work with either.

jhedberg

No other timer driver creates a struct device, as far as I can tell. The timer API is also not "normal" looking -- it's global functions rather than an API struct with function pointers, and those functions don't give you a struct device pointer to work with either.

Ok. Fair enough. I guess this can be revisited later if we want.

jerome-pouiller · 2024-10-24T07:26:59Z

boards/silabs/dev_kits/sltb010a/thunderboard.dtsi

I believe each of these counters should have its own driver and the instantiation of the right driver should done through the device tree.

However, this is clearly out of the scope of the current PR.

jerome-pouiller · 2024-10-24T07:29:40Z

drivers/timer/Kconfig.silabs

+	depends on DT_HAS_SILABS_GECKO_STIMER_ENABLED
+	select SOC_SILABS_SLEEPTIMER
+	select TICKLESS_CAPABLE
+	select TIMER_READS_ITS_FREQUENCY_AT_RUNTIME


Should we add default y?

asmellby added the DNM This PR should not be merged (Do Not Merge) label Oct 22, 2024

asmellby requested review from jhedberg and jerome-pouiller October 22, 2024 12:51

asmellby requested review from kgugala and fkokosinski as code owners October 22, 2024 12:51

zephyrbot added platform: Silabs Silicon Labs area: Process area: Counter area: Timer Timer area: Clock Control platform: Silabs SiM3U Silicon Labs SiM3U area: Regulators labels Oct 22, 2024

zephyrbot added the manifest label Oct 22, 2024

zephyrbot requested review from aasinclair, andyross, danieldegrasse, gmarull and M1cha October 22, 2024 12:52

zephyrbot added the manifest-hal_silabs label Oct 22, 2024

zephyrbot requested review from MaureenHelm, nashif, nordic-krch, rettichschnidi, stephanosio and teburd October 22, 2024 12:52

zephyrbot removed the DNM This PR should not be merged (Do Not Merge) label Oct 22, 2024

zephyrbot assigned andyross Oct 22, 2024

asmellby force-pushed the feature/series2-sleeptimer-timer branch 2 times, most recently from 26fe225 to 4cd8cd1 Compare October 22, 2024 12:57

jerome-pouiller reviewed Oct 22, 2024

View reviewed changes

jhedberg added this to the v4.0.0 milestone Oct 22, 2024

asmellby force-pushed the feature/series2-sleeptimer-timer branch from 4cd8cd1 to 7c9a83e Compare October 22, 2024 21:45

zephyrbot removed manifest manifest-hal_silabs labels Oct 22, 2024

asmellby force-pushed the feature/series2-sleeptimer-timer branch from 7c9a83e to 5766705 Compare October 23, 2024 06:31

jhedberg previously approved these changes Oct 23, 2024

View reviewed changes

asmellby added 3 commits October 23, 2024 12:10

soc: silabs: Default to sleeptimer for OS timer on Series 2

0aaece0

Disable BURTC timer in board defconfigs, as it's no longer used. Signed-off-by: Aksel Skauge Mellbye <aksel.mellbye@silabs.com>

asmellby dismissed jhedberg’s stale review via 6b76afe October 23, 2024 10:13

asmellby force-pushed the feature/series2-sleeptimer-timer branch from 5766705 to 6b76afe Compare October 23, 2024 10:13

jhedberg approved these changes Oct 23, 2024

View reviewed changes

jhedberg self-assigned this Oct 23, 2024

asmellby mentioned this pull request Oct 23, 2024

drivers: bluetooth: hci: silabs: Configure radio for sleep #80299

Merged

jerome-pouiller approved these changes Oct 24, 2024

View reviewed changes

carlescufi merged commit 8d4fa7b into zephyrproject-rtos:main Oct 24, 2024
29 checks passed

asmellby deleted the feature/series2-sleeptimer-timer branch October 24, 2024 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: timer: silabs: Add sleeptimer timer driver #80227

drivers: timer: silabs: Add sleeptimer timer driver #80227

asmellby commented Oct 22, 2024

zephyrbot commented Oct 22, 2024 •

edited

Loading

jerome-pouiller Oct 22, 2024

asmellby Oct 22, 2024

jerome-pouiller Oct 24, 2024

jerome-pouiller Oct 22, 2024

jerome-pouiller Oct 22, 2024

asmellby Oct 22, 2024

asmellby Oct 23, 2024

jerome-pouiller Oct 22, 2024

asmellby Oct 23, 2024

jerome-pouiller Oct 22, 2024

asmellby Oct 22, 2024

jerome-pouiller Oct 22, 2024

asmellby Oct 22, 2024

jerome-pouiller Oct 22, 2024

M1cha Oct 23, 2024

jerome-pouiller Oct 23, 2024

M1cha Oct 23, 2024

asmellby Oct 23, 2024 •

edited

Loading

M1cha Oct 24, 2024

jerome-pouiller Oct 22, 2024

asmellby Oct 22, 2024 •

edited

Loading

jerome-pouiller Oct 23, 2024

jerome-pouiller Oct 22, 2024

asmellby Oct 22, 2024

jhedberg commented Oct 23, 2024

jhedberg commented Oct 23, 2024 •

edited

Loading

asmellby commented Oct 23, 2024

jhedberg left a comment

jerome-pouiller Oct 24, 2024

jerome-pouiller Oct 24, 2024

drivers: timer: silabs: Add sleeptimer timer driver #80227

drivers: timer: silabs: Add sleeptimer timer driver #80227

Conversation

asmellby commented Oct 22, 2024

zephyrbot commented Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmellby Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asmellby Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhedberg commented Oct 23, 2024

jhedberg commented Oct 23, 2024 • edited Loading

asmellby commented Oct 23, 2024

jhedberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zephyrbot commented Oct 22, 2024 •

edited

Loading

asmellby Oct 23, 2024 •

edited

Loading

asmellby Oct 22, 2024 •

edited

Loading

jhedberg commented Oct 23, 2024 •

edited

Loading