[PATCH 0/3] Timer events for OpenSBI

[PATCH 0/3] Timer events for OpenSBI

[Anup Patel]

This series extends the sbi_timer framework to support
timer events usable from any part of OpenSBI. The platform
drivers in OpenSBI can use timer events for timeouts or
periodic checks.

These patches can also be found in sbi_timer_imp_v1 branch
at: https://github.com/avpatel/opensbi.git

Anup Patel (3):
  include: sbi: Add sbi_scratch_hartindex() macro
  lib: sbi_timer: Introduce per-HART timer state
  lib: sbi_timer: Add support for timer events

 include/sbi/sbi_scratch.h  |   6 +-
 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        | 221 +++++++++++++++++++++++++++++++------
 5 files changed, 234 insertions(+), 44 deletions(-)

-- 
2.43.0


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH 1/3] include: sbi: Add sbi_scratch_hartindex() macro

[Anup Patel]

Add helper macro to extract hart index from scratch pointer. This
can be used to check whether scratch pointer belongs to a particular
hart or not.

Signed-off-by: Anup Patel <anup.patel@oss.qualcomm.com>
---
 include/sbi/sbi_scratch.h | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/include/sbi/sbi_scratch.h b/include/sbi/sbi_scratch.h
index 58d54628..c12aa796 100644
--- a/include/sbi/sbi_scratch.h
+++ b/include/sbi/sbi_scratch.h
@@ -166,9 +166,11 @@ do {									\
 					= (__type)(__ptr);		\
 } while (0)
 
+/** Get the hart index of a particular sbi_scratch */
+#define sbi_scratch_hartindex(__scratch) ((__scratch)->hartindex)
+
 /** Get the hart index of the current hart */
-#define current_hartindex() \
-	(sbi_scratch_thishart_ptr()->hartindex)
+#define current_hartindex() sbi_scratch_hartindex(sbi_scratch_thishart_ptr())
 
 /** Number of harts managed by this OpenSBI instance */
 extern u32 sbi_scratch_hart_count;
-- 
2.43.0


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Re: [PATCH 1/3] include: sbi: Add sbi_scratch_hartindex() macro

[Nicholas Piggin]

On Wed, Apr 15, 2026 at 04:30:00PM +0530, Anup Patel wrote:
> Add helper macro to extract hart index from scratch pointer. This
> can be used to check whether scratch pointer belongs to a particular
> hart or not.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> 
> Signed-off-by: Anup Patel <anup.patel@oss.qualcomm.com>
> ---
>  include/sbi/sbi_scratch.h | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/include/sbi/sbi_scratch.h b/include/sbi/sbi_scratch.h
> index 58d54628..c12aa796 100644
> --- a/include/sbi/sbi_scratch.h
> +++ b/include/sbi/sbi_scratch.h
> @@ -166,9 +166,11 @@ do {									\
>  					= (__type)(__ptr);		\
>  } while (0)
>  
> +/** Get the hart index of a particular sbi_scratch */
> +#define sbi_scratch_hartindex(__scratch) ((__scratch)->hartindex)
> +
>  /** Get the hart index of the current hart */
> -#define current_hartindex() \
> -	(sbi_scratch_thishart_ptr()->hartindex)
> +#define current_hartindex() sbi_scratch_hartindex(sbi_scratch_thishart_ptr())
>  
>  /** Number of harts managed by this OpenSBI instance */
>  extern u32 sbi_scratch_hart_count;
> -- 
> 2.43.0
> 
> 
> -- 
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH 2/3] lib: sbi_timer: Introduce per-HART timer state

[Anup Patel]

Currently, only time_delta is per-HART so introduce per-HART timer
state for having more per-HART timer information.

Signed-off-by: Anup Patel <anup.patel@oss.qualcomm.com>
---
 lib/sbi/sbi_timer.c | 44 +++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

diff --git a/lib/sbi/sbi_timer.c b/lib/sbi/sbi_timer.c
index 4088a597..9806c033 100644
--- a/lib/sbi/sbi_timer.c
+++ b/lib/sbi/sbi_timer.c
@@ -18,7 +18,11 @@
 #include <sbi/sbi_scratch.h>
 #include <sbi/sbi_timer.h>
 
-static unsigned long time_delta_off;
+struct timer_state {
+	u64 time_delta;
+};
+
+static unsigned long timer_state_off;
 static u64 (*get_time_val)(void);
 static const struct sbi_timer_device *timer_dev = NULL;
 
@@ -98,35 +102,37 @@ u64 sbi_timer_value(void)
 
 u64 sbi_timer_virt_value(void)
 {
-	u64 *time_delta = sbi_scratch_offset_ptr(sbi_scratch_thishart_ptr(),
-						 time_delta_off);
+	struct timer_state *tstate = sbi_scratch_thishart_offset_ptr(timer_state_off);
 
-	return sbi_timer_value() + *time_delta;
+	return sbi_timer_value() + tstate->time_delta;
 }
 
 u64 sbi_timer_get_delta(void)
 {
-	u64 *time_delta = sbi_scratch_offset_ptr(sbi_scratch_thishart_ptr(),
-						 time_delta_off);
+	struct timer_state *tstate = sbi_scratch_thishart_offset_ptr(timer_state_off);
 
-	return *time_delta;
+	return tstate->time_delta;
 }
 
 void sbi_timer_set_delta(ulong delta)
 {
-	ulong *time_delta = sbi_scratch_offset_ptr(sbi_scratch_thishart_ptr(),
-						   time_delta_off);
+	struct timer_state *tstate = sbi_scratch_thishart_offset_ptr(timer_state_off);
 
-	*time_delta = delta;
+#if __riscv_xlen == 32
+	tstate->time_delta &= ~0xffffffffUL;
+	tstate->time_delta |= (u32)delta;
+#else
+	tstate->time_delta = delta;
+#endif
 }
 
 #if __riscv_xlen == 32
 void sbi_timer_set_delta_upper(ulong delta_upper)
 {
-	ulong *time_delta = sbi_scratch_offset_ptr(sbi_scratch_thishart_ptr(),
-						   time_delta_off);
+	struct timer_state *tstate = sbi_scratch_thishart_offset_ptr(timer_state_off);
 
-	*(time_delta + 1) = delta_upper;
+	tstate->time_delta &= 0xffffffffUL;
+	tstate->time_delta |= (u64)delta << 32;
 }
 #endif
 
@@ -176,13 +182,13 @@ void sbi_timer_set_device(const struct sbi_timer_device *dev)
 
 int sbi_timer_init(struct sbi_scratch *scratch, bool cold_boot)
 {
-	u64 *time_delta;
 	const struct sbi_platform *plat = sbi_platform_ptr(scratch);
+	struct timer_state *tstate;
 	int ret;
 
 	if (cold_boot) {
-		time_delta_off = sbi_scratch_alloc_offset(sizeof(*time_delta));
-		if (!time_delta_off)
+		timer_state_off = sbi_scratch_alloc_offset(sizeof(*tstate));
+		if (!timer_state_off)
 			return SBI_ENOMEM;
 
 		if (sbi_hart_has_csr(scratch, SBI_HART_CSR_TIME))
@@ -192,12 +198,12 @@ int sbi_timer_init(struct sbi_scratch *scratch, bool cold_boot)
 		if (ret)
 			return ret;
 	} else {
-		if (!time_delta_off)
+		if (!timer_state_off)
 			return SBI_ENOMEM;
 	}
 
-	time_delta = sbi_scratch_offset_ptr(scratch, time_delta_off);
-	*time_delta = 0;
+	tstate = sbi_scratch_offset_ptr(scratch, timer_state_off);
+	tstate->time_delta = 0;
 
 	if (timer_dev && timer_dev->warm_init) {
 		ret = timer_dev->warm_init();
-- 
2.43.0


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Re: [PATCH 2/3] lib: sbi_timer: Introduce per-HART timer state

[Nicholas Piggin]

On Wed, Apr 15, 2026 at 04:30:01PM +0530, Anup Patel wrote:
> Currently, only time_delta is per-HART so introduce per-HART timer
> state for having more per-HART timer information.
> 
> Signed-off-by: Anup Patel <anup.patel@oss.qualcomm.com>

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

> ---
>  lib/sbi/sbi_timer.c | 44 +++++++++++++++++++++++++-------------------
>  1 file changed, 25 insertions(+), 19 deletions(-)
> 
> diff --git a/lib/sbi/sbi_timer.c b/lib/sbi/sbi_timer.c
> index 4088a597..9806c033 100644
> --- a/lib/sbi/sbi_timer.c
> +++ b/lib/sbi/sbi_timer.c
> @@ -18,7 +18,11 @@
>  #include <sbi/sbi_scratch.h>
>  #include <sbi/sbi_timer.h>
>  
> -static unsigned long time_delta_off;
> +struct timer_state {
> +	u64 time_delta;
> +};
> +
> +static unsigned long timer_state_off;
>  static u64 (*get_time_val)(void);
>  static const struct sbi_timer_device *timer_dev = NULL;
>  
> @@ -98,35 +102,37 @@ u64 sbi_timer_value(void)
>  
>  u64 sbi_timer_virt_value(void)
>  {
> -	u64 *time_delta = sbi_scratch_offset_ptr(sbi_scratch_thishart_ptr(),
> -						 time_delta_off);
> +	struct timer_state *tstate = sbi_scratch_thishart_offset_ptr(timer_state_off);
>  
> -	return sbi_timer_value() + *time_delta;
> +	return sbi_timer_value() + tstate->time_delta;
>  }
>  
>  u64 sbi_timer_get_delta(void)
>  {
> -	u64 *time_delta = sbi_scratch_offset_ptr(sbi_scratch_thishart_ptr(),
> -						 time_delta_off);
> +	struct timer_state *tstate = sbi_scratch_thishart_offset_ptr(timer_state_off);
>  
> -	return *time_delta;
> +	return tstate->time_delta;
>  }
>  
>  void sbi_timer_set_delta(ulong delta)
>  {
> -	ulong *time_delta = sbi_scratch_offset_ptr(sbi_scratch_thishart_ptr(),
> -						   time_delta_off);
> +	struct timer_state *tstate = sbi_scratch_thishart_offset_ptr(timer_state_off);
>  
> -	*time_delta = delta;
> +#if __riscv_xlen == 32
> +	tstate->time_delta &= ~0xffffffffUL;
> +	tstate->time_delta |= (u32)delta;
> +#else
> +	tstate->time_delta = delta;
> +#endif
>  }
>  
>  #if __riscv_xlen == 32
>  void sbi_timer_set_delta_upper(ulong delta_upper)
>  {
> -	ulong *time_delta = sbi_scratch_offset_ptr(sbi_scratch_thishart_ptr(),
> -						   time_delta_off);
> +	struct timer_state *tstate = sbi_scratch_thishart_offset_ptr(timer_state_off);
>  
> -	*(time_delta + 1) = delta_upper;
> +	tstate->time_delta &= 0xffffffffUL;
> +	tstate->time_delta |= (u64)delta << 32;
>  }
>  #endif
>  
> @@ -176,13 +182,13 @@ void sbi_timer_set_device(const struct sbi_timer_device *dev)
>  
>  int sbi_timer_init(struct sbi_scratch *scratch, bool cold_boot)
>  {
> -	u64 *time_delta;
>  	const struct sbi_platform *plat = sbi_platform_ptr(scratch);
> +	struct timer_state *tstate;
>  	int ret;
>  
>  	if (cold_boot) {
> -		time_delta_off = sbi_scratch_alloc_offset(sizeof(*time_delta));
> -		if (!time_delta_off)
> +		timer_state_off = sbi_scratch_alloc_offset(sizeof(*tstate));
> +		if (!timer_state_off)
>  			return SBI_ENOMEM;
>  
>  		if (sbi_hart_has_csr(scratch, SBI_HART_CSR_TIME))
> @@ -192,12 +198,12 @@ int sbi_timer_init(struct sbi_scratch *scratch, bool cold_boot)
>  		if (ret)
>  			return ret;
>  	} else {
> -		if (!time_delta_off)
> +		if (!timer_state_off)
>  			return SBI_ENOMEM;
>  	}
>  
> -	time_delta = sbi_scratch_offset_ptr(scratch, time_delta_off);
> -	*time_delta = 0;
> +	tstate = sbi_scratch_offset_ptr(scratch, timer_state_off);
> +	tstate->time_delta = 0;
>  
>  	if (timer_dev && timer_dev->warm_init) {
>  		ret = timer_dev->warm_init();
> -- 
> 2.43.0
> 
> 
> -- 
> opensbi mailing list
> opensbi@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

[PATCH 3/3] lib: sbi_timer: Add support for timer events

[Anup Patel]

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@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() */
+	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) {
+		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);
+	}
+
+	/* 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);
+}
+
+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()) {
+			__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)
@@ -204,6 +331,11 @@ int sbi_timer_init(struct sbi_scratch *scratch, bool cold_boot)
 
 	tstate = sbi_scratch_offset_ptr(scratch, timer_state_off);
 	tstate->time_delta = 0;
+	SPIN_LOCK_INIT(tstate->event_list_lock);
+	SBI_INIT_LIST_HEAD(&tstate->event_list);
+	SBI_INIT_TIMER_EVENT(&tstate->smode_ev,
+			     sbi_timer_smode_event_callback,
+			     sbi_timer_smode_event_cleanup, NULL);
 
 	if (timer_dev && timer_dev->warm_init) {
 		ret = timer_dev->warm_init();
@@ -216,9 +348,22 @@ int sbi_timer_init(struct sbi_scratch *scratch, bool cold_boot)
 
 void sbi_timer_exit(struct sbi_scratch *scratch)
 {
-	if (timer_dev && timer_dev->timer_event_stop)
-		timer_dev->timer_event_stop();
+	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);
+		__sbi_timer_event_stop(ev);
+		if (ev->cleanup) {
+			spin_unlock(&tstate->event_list_lock);
+			ev->cleanup(ev);
+			spin_lock(&tstate->event_list_lock);
+		}
+	}
+
+	__sbi_timer_update_device(tstate);
 
-	csr_clear(CSR_MIP, MIP_STIP);
-	csr_clear(CSR_MIE, MIP_MTIP);
+	spin_unlock(&tstate->event_list_lock);
 }
-- 
2.43.0


-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Re: [PATCH 3/3] lib: sbi_timer: Add support for timer events

[Nicholas Piggin]

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@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

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Re: [PATCH 3/3] lib: sbi_timer: Add support for timer events

[Anup Patel]

On Thu, Apr 23, 2026 at 12:24 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> 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@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.

Yes, it's a typo. I will update.

>
> > +     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 */

It is generally good to have ability to stop a running timer
event from any CPU but I agree the problem you are highlighting.
I think the real problem in this patch is the spin_unlock()/spin_lock()
dance around the callback function called by sbi_timer_process().

To avoid the spin_unlock()/spin_lock() dance in sbi_timer_process(),
I think it is better to call the callback function with lock held and callback
function can optionally return next_time_stamp if it wants to re-add
periodic timer. Suggestions ?

>
> > +             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.

We might have to waste a lot of CPU cycles in the busy-loop.

>
> > +     }
> > +
> > +     /* 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()

Yes, good suggestion. I will update.

>
> > +}
> > +
> > +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()?

Yes, good suggestion. I will update.

Regards,
Anup

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Re: [PATCH 3/3] lib: sbi_timer: Add support for timer events

[Nicholas Piggin]

On Thu, Apr 23, 2026 at 01:55:59PM +0530, Anup Patel wrote:
> On Thu, Apr 23, 2026 at 12:24 PM Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > On Wed, Apr 15, 2026 at 04:30:02PM +0530, Anup Patel wrote:

> > > +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 */
> 
> It is generally good to have ability to stop a running timer
> event from any CPU but I agree the problem you are highlighting.
> I think the real problem in this patch is the spin_unlock()/spin_lock()
> dance around the callback function called by sbi_timer_process().
> 
> To avoid the spin_unlock()/spin_lock() dance in sbi_timer_process(),
> I think it is better to call the callback function with lock held and callback
> function can optionally return next_time_stamp if it wants to re-add
> periodic timer. Suggestions ?

Yes, if the timer callbacks are fine with it, then the simpler the better.

Returning next_time_stamp if a reschedule is needed seems reasonable to
me. sbi_timer_event_start_from_callback() would be okay too.

> > > +             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.
> 
> We might have to waste a lot of CPU cycles in the busy-loop.

That's true but I would expect it to not be common. Having the simpler
sync API as the first / default one would be good, add something more
complex if you find the need.

In any case, I think keeping the lock held over the callback should
make this a synchronous delete.

Thanks,
Nick

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Re: [PATCH 0/3] Timer events for OpenSBI

[Nicholas Piggin]

On Wed, Apr 15, 2026 at 04:29:59PM +0530, Anup Patel wrote:
> This series extends the sbi_timer framework to support
> timer events usable from any part of OpenSBI. The platform
> drivers in OpenSBI can use timer events for timeouts or
> periodic checks.
> 
> These patches can also be found in sbi_timer_imp_v1 branch
> at: https://github.com/avpatel/opensbi.git

What is your m mode use case for these, I would be interested
in the context of reviewing it. Generally I thought it looks
good.

Thanks,
Nick

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Re: [PATCH 0/3] Timer events for OpenSBI

[Anup Patel]

On Thu, Apr 23, 2026 at 12:27 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>
> On Wed, Apr 15, 2026 at 04:29:59PM +0530, Anup Patel wrote:
> > This series extends the sbi_timer framework to support
> > timer events usable from any part of OpenSBI. The platform
> > drivers in OpenSBI can use timer events for timeouts or
> > periodic checks.
> >
> > These patches can also be found in sbi_timer_imp_v1 branch
> > at: https://github.com/avpatel/opensbi.git
>
> What is your m mode use case for these, I would be interested
> in the context of reviewing it. Generally I thought it looks
> good.
>

We have a RPMI watchdog proposal under development. This
series will provide foundation for the PoC work of the RPMI
watchdog proposal. Basically, the OpenSBI firmware can emulate
RPMI watchdog over SBI MPXY channel for the supervisor software.
This OpenSBI can provide a separate watchdog for each supervisor
domain. In absence of HW watchdog for M-mode firmware, the PuC
firmware can also implement RPMI watchdog over RPMI shared
memory transport.

Regards,
Anup

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi

Re: [PATCH 0/3] Timer events for OpenSBI

[Nicholas Piggin]

On Thu, Apr 23, 2026 at 01:38:55PM +0530, Anup Patel wrote:
> On Thu, Apr 23, 2026 at 12:27 PM Nicholas Piggin <npiggin@gmail.com> wrote:
> >
> > On Wed, Apr 15, 2026 at 04:29:59PM +0530, Anup Patel wrote:
> > > This series extends the sbi_timer framework to support
> > > timer events usable from any part of OpenSBI. The platform
> > > drivers in OpenSBI can use timer events for timeouts or
> > > periodic checks.
> > >
> > > These patches can also be found in sbi_timer_imp_v1 branch
> > > at: https://github.com/avpatel/opensbi.git
> >
> > What is your m mode use case for these, I would be interested
> > in the context of reviewing it. Generally I thought it looks
> > good.
> >
> 
> We have a RPMI watchdog proposal under development. This
> series will provide foundation for the PoC work of the RPMI
> watchdog proposal. Basically, the OpenSBI firmware can emulate
> RPMI watchdog over SBI MPXY channel for the supervisor software.
> This OpenSBI can provide a separate watchdog for each supervisor
> domain. In absence of HW watchdog for M-mode firmware, the PuC
> firmware can also implement RPMI watchdog over RPMI shared
> memory transport.

Oh nice, that's an important feature for HA. Good use of m mode
timers.

Thanks,
Nick

-- 
opensbi mailing list
opensbi@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/opensbi