[PATCH 3/3] lib: sbi_timer: Add support for timer events
Nicholas Piggin
npiggin at gmail.com
Wed Apr 22 23:54:42 PDT 2026
On Wed, Apr 15, 2026 at 04:30:02PM +0530, Anup Patel wrote:
> Currently, the sbi_timer only supports timer events configured via
> SBI calls. Introduce struct sbi_timer_event and related functions
> to allow configuring timer events from any part of OpenSBI.
>
> Signed-off-by: Anup Patel <anup.patel at oss.qualcomm.com>
> ---
> include/sbi/sbi_timer.h | 43 ++++++++-
> lib/sbi/sbi_ecall_legacy.c | 4 +-
> lib/sbi/sbi_ecall_time.c | 4 +-
> lib/sbi/sbi_timer.c | 177 +++++++++++++++++++++++++++++++++----
> 4 files changed, 205 insertions(+), 23 deletions(-)
>
> diff --git a/include/sbi/sbi_timer.h b/include/sbi/sbi_timer.h
> index 2e1a7879..f23d72db 100644
> --- a/include/sbi/sbi_timer.h
> +++ b/include/sbi/sbi_timer.h
> @@ -10,7 +10,38 @@
> #ifndef __SBI_TIMER_H__
> #define __SBI_TIMER_H__
>
> -#include <sbi/sbi_types.h>
> +#include <sbi/sbi_list.h>
> +
> +/** Timer event abstraction */
> +struct sbi_timer_event {
> + /** List head for per-HART event list (Internal) */
> + struct sbi_dlist head;
> +
> + /** Hart on which the event is started / running (Internal) */
> + int hart_index;
> +
> + /** Time stamp when the event expires (Internal) */
> + u64 time_stamp;
> +
> + /** Event callback to be called upon expiry */
> + void (*callback)(struct sbi_timer_event *ev);
> +
> + /** Event cleanup to be called upon sbi_hart_exit() */
^ sbi_timer_exit()
Minor typo.
> + void (*cleanup)(struct sbi_timer_event *ev);
> +
> + /** Event specific private data */
> + void *priv;
> +};
> +
> +#define SBI_INIT_TIMER_EVENT(__ptr, __callback, __cleanup, __priv) \
> +do { \
> + SBI_INIT_LIST_HEAD(&(__ptr)->head); \
> + (__ptr)->hart_index = -1; \
> + (__ptr)->time_stamp = 0; \
> + (__ptr)->callback = (__callback); \
> + (__ptr)->cleanup = (__cleanup); \
> + (__ptr)->priv = (__priv); \
> +} while (0)
>
> /** Timer hardware device */
> struct sbi_timer_device {
> @@ -86,8 +117,14 @@ void sbi_timer_set_delta(ulong delta);
> void sbi_timer_set_delta_upper(ulong delta_upper);
> #endif
>
> -/** Start timer event for current HART */
> -void sbi_timer_event_start(u64 next_event);
> +/** Start timer event on current HART */
> +void sbi_timer_event_start(struct sbi_timer_event *ev, u64 next_event);
> +
> +/** Stop timer event on current HART */
> +void sbi_timer_event_stop(struct sbi_timer_event *ev);
> +
> +/** Start supervisor timer event on current HART */
> +void sbi_timer_smode_event_start(u64 next_event);
>
> /** Process timer event for current HART */
> void sbi_timer_process(void);
> diff --git a/lib/sbi/sbi_ecall_legacy.c b/lib/sbi/sbi_ecall_legacy.c
> index 50a7660d..4501c4ad 100644
> --- a/lib/sbi/sbi_ecall_legacy.c
> +++ b/lib/sbi/sbi_ecall_legacy.c
> @@ -54,9 +54,9 @@ static int sbi_ecall_legacy_handler(unsigned long extid, unsigned long funcid,
> switch (extid) {
> case SBI_EXT_0_1_SET_TIMER:
> #if __riscv_xlen == 32
> - sbi_timer_event_start((((u64)regs->a1 << 32) | (u64)regs->a0));
> + sbi_timer_smode_event_start((((u64)regs->a1 << 32) | (u64)regs->a0));
> #else
> - sbi_timer_event_start((u64)regs->a0);
> + sbi_timer_smode_event_start((u64)regs->a0);
> #endif
> break;
> case SBI_EXT_0_1_CONSOLE_PUTCHAR:
> diff --git a/lib/sbi/sbi_ecall_time.c b/lib/sbi/sbi_ecall_time.c
> index 6ea6f054..a0aa580d 100644
> --- a/lib/sbi/sbi_ecall_time.c
> +++ b/lib/sbi/sbi_ecall_time.c
> @@ -22,9 +22,9 @@ static int sbi_ecall_time_handler(unsigned long extid, unsigned long funcid,
>
> if (funcid == SBI_EXT_TIME_SET_TIMER) {
> #if __riscv_xlen == 32
> - sbi_timer_event_start((((u64)regs->a1 << 32) | (u64)regs->a0));
> + sbi_timer_smode_event_start((((u64)regs->a1 << 32) | (u64)regs->a0));
> #else
> - sbi_timer_event_start((u64)regs->a0);
> + sbi_timer_smode_event_start((u64)regs->a0);
> #endif
> } else
> ret = SBI_ENOTSUPP;
> diff --git a/lib/sbi/sbi_timer.c b/lib/sbi/sbi_timer.c
> index 9806c033..539157fa 100644
> --- a/lib/sbi/sbi_timer.c
> +++ b/lib/sbi/sbi_timer.c
> @@ -10,9 +10,11 @@
> #include <sbi/riscv_asm.h>
> #include <sbi/riscv_barrier.h>
> #include <sbi/riscv_encoding.h>
> +#include <sbi/riscv_locks.h>
> #include <sbi/sbi_console.h>
> #include <sbi/sbi_error.h>
> #include <sbi/sbi_hart.h>
> +#include <sbi/sbi_hartmask.h>
> #include <sbi/sbi_platform.h>
> #include <sbi/sbi_pmu.h>
> #include <sbi/sbi_scratch.h>
> @@ -20,6 +22,9 @@
>
> struct timer_state {
> u64 time_delta;
> + spinlock_t event_list_lock;
> + struct sbi_dlist event_list;
> + struct sbi_timer_event smode_ev;
> };
>
> static unsigned long timer_state_off;
> @@ -136,8 +141,119 @@ void sbi_timer_set_delta_upper(ulong delta_upper)
> }
> #endif
>
> -void sbi_timer_event_start(u64 next_event)
> +static void __sbi_timer_update_device(struct timer_state *tstate)
> {
> + struct sbi_timer_event *ev;
> +
> + if (!timer_dev)
> + return;
> +
> + if (sbi_list_empty(&tstate->event_list)) {
> + if (timer_dev->timer_event_stop)
> + timer_dev->timer_event_stop();
> + csr_clear(CSR_MIE, MIP_MTIP);
> + } else {
> + ev = sbi_list_first_entry(&tstate->event_list, struct sbi_timer_event, head);
> + if (timer_dev->timer_event_start)
> + timer_dev->timer_event_start(ev->time_stamp);
> + csr_set(CSR_MIE, MIP_MTIP);
> + }
> +}
> +
> +static void __sbi_timer_event_stop(struct sbi_timer_event *ev)
> +{
> + if (ev->hart_index > -1) {
> + sbi_list_del(&ev->head);
> + ev->hart_index = -1;
> + }
> +}
> +
> +void sbi_timer_event_start(struct sbi_timer_event *ev, u64 next_event)
> +{
> + struct sbi_timer_event *tev, *next_ev = NULL;
> + struct timer_state *tstate;
> +
> + if (!ev)
> + return;
> +
> + /* Ensure that event is not on the per-HART event list */
> + if (ev->hart_index > -1) {
It is a bit worrying to permit sbi_timer_event_start() if the timer is
already on a list. Is that necessary, or could you return an error in
this case?
I'm thinking problems like this -
HART0 HART1
sbi_timer_event_start() sbi_timer_process()
if (hart_index > -1) {
__sbi_timer_event_stop(ev)
if (ev->callback) {
spin_unlock();
spin_lock();
__sbi_timer_event_stop(ev)
spin_unlock();
callback_fn()
sbi_timer_event_start(); /* re-add periodic timer */
> + tstate = sbi_scratch_offset_ptr(sbi_hartindex_to_scratch(ev->hart_index),
> + timer_state_off);
> + spin_lock(&tstate->event_list_lock);
> + __sbi_timer_event_stop(ev);
> + spin_unlock(&tstate->event_list_lock);
> + }
> +
> + tstate = sbi_scratch_thishart_offset_ptr(timer_state_off);
> + spin_lock(&tstate->event_list_lock);
> +
> + /* Find where to insert the event in per-HART event list */
> + sbi_list_for_each_entry(tev, &tstate->event_list, head) {
> + if (next_event < tev->time_stamp) {
> + next_ev = tev;
> + break;
> + }
> + }
> +
> + /* Insert the event in per-HART event list */
> + ev->hart_index = current_hartindex();
> + ev->time_stamp = next_event;
> + if (next_ev)
> + sbi_list_add(&ev->head, &next_ev->head);
> + else
> + sbi_list_add_tail(&ev->head, &tstate->event_list);
> +
> + __sbi_timer_update_device(tstate);
> +
> + spin_unlock(&tstate->event_list_lock);
> +}
> +
> +void sbi_timer_event_stop(struct sbi_timer_event *ev)
> +{
> + struct timer_state *tstate;
> +
> + if (!ev)
> + return;
> +
> + /* Ensure that event is not on the per-HART event list */
> + if (ev->hart_index > -1) {
> + tstate = sbi_scratch_offset_ptr(sbi_hartindex_to_scratch(ev->hart_index),
> + timer_state_off);
> + spin_lock(&tstate->event_list_lock);
> + __sbi_timer_event_stop(ev);
> + spin_unlock(&tstate->event_list_lock);
Similar concern here, timer could be in the middle of running. Maybe it
is benign. I think it would be nice to make this a "sync" stop though,
so it can wait for potential running timers.
timer event could have a 'running' state which is updated inside the
lock, then sbi_timer_event_stop() (and sbi_timer_exit()) could wait for
it to clear.
> + }
> +
> + /* Re-program timer device on the current HART */
> + tstate = sbi_scratch_thishart_offset_ptr(timer_state_off);
> + spin_lock(&tstate->event_list_lock);
> + __sbi_timer_update_device(tstate);
> + spin_unlock(&tstate->event_list_lock);
This could be skipped if ev->hart_index != current_hartid()
> +}
> +
> +static void sbi_timer_smode_event_callback(struct sbi_timer_event *ev)
> +{
> + /*
> + * If sstc extension is available, supervisor can receive the timer
> + * directly without M-mode come in between. This function should
> + * only invoked if M-mode programs the timer for its own purpose.
> + */
> + if (!sbi_hart_has_extension(sbi_scratch_thishart_ptr(), SBI_HART_EXT_SSTC))
> + csr_set(CSR_MIP, MIP_STIP);
> +}
> +
> +static void sbi_timer_smode_event_cleanup(struct sbi_timer_event *ev)
> +{
> + if (!sbi_hart_has_extension(sbi_scratch_thishart_ptr(), SBI_HART_EXT_SSTC))
> + csr_clear(CSR_MIP, MIP_STIP);
> +}
> +
> +void sbi_timer_smode_event_start(u64 next_event)
> +{
> + struct timer_state *tstate = sbi_scratch_offset_ptr(sbi_scratch_thishart_ptr(),
> + timer_state_off);
> +
> sbi_pmu_ctr_incr_fw(SBI_PMU_FW_SET_TIMER);
>
> /**
> @@ -146,23 +262,34 @@ void sbi_timer_event_start(u64 next_event)
> */
> if (sbi_hart_has_extension(sbi_scratch_thishart_ptr(), SBI_HART_EXT_SSTC)) {
> csr_write64(CSR_STIMECMP, next_event);
> - } else if (timer_dev && timer_dev->timer_event_start) {
> - timer_dev->timer_event_start(next_event);
> + } else {
> csr_clear(CSR_MIP, MIP_STIP);
> + sbi_timer_event_start(&tstate->smode_ev, next_event);
> }
> - csr_set(CSR_MIE, MIP_MTIP);
> }
>
> void sbi_timer_process(void)
> {
> - csr_clear(CSR_MIE, MIP_MTIP);
> - /*
> - * If sstc extension is available, supervisor can receive the timer
> - * directly without M-mode come in between. This function should
> - * only invoked if M-mode programs the timer for its own purpose.
> - */
> - if (!sbi_hart_has_extension(sbi_scratch_thishart_ptr(), SBI_HART_EXT_SSTC))
> - csr_set(CSR_MIP, MIP_STIP);
> + struct timer_state *tstate = sbi_scratch_thishart_offset_ptr(timer_state_off);
> + struct sbi_timer_event *ev;
> +
> + spin_lock(&tstate->event_list_lock);
> +
> + while (!sbi_list_empty(&tstate->event_list)) {
> + ev = sbi_list_first_entry(&tstate->event_list, struct sbi_timer_event, head);
> + if (ev->time_stamp <= sbi_timer_value()) {
Since the list is sorted this could break if time_stamp >
sbi_timer_value()?
> + __sbi_timer_event_stop(ev);
> + if (ev->callback) {
> + spin_unlock(&tstate->event_list_lock);
> + ev->callback(ev);
> + spin_lock(&tstate->event_list_lock);
> + }
> + }
> + }
> +
> + __sbi_timer_update_device(tstate);
> +
> + spin_unlock(&tstate->event_list_lock);
> }
>
> const struct sbi_timer_device *sbi_timer_get_device(void)
Thanks,
Nick
More information about the opensbi
mailing list