Re: [PATCH v7 1/2] clocksource/drivers/timer-clint: Add ZICNTR support

[Charlie Jenkins <thecharlesjenkins@gmail.com>]

On Mon, May 25, 2026 at 02:21:23PM -0400, Jesse Taube wrote:
> ZICNTR allows the CPU time to be read using CSRs, which used to be the
> default. If ZICNTR is supported prefer using CSRs over MMIO.
> This may also be faster as CSRs are not emulated in M-mode.

I think the rationale can be cleaned up a bit here. You say it "may" be
faster, if it's not guaranteed to be faster, why prefer it over mmio?

The patch d5be89a8d118 ("RISC-V: Resurrect the MMIO timer implementation
for M-mode systems") is the one that changed this behavior, can you
include a reference to that patch to point out when it was the default?

> 
> Signed-off-by: Jesse Taube <jtaubepe@redhat.com>
> ---
> Treat this as a completely new patch, as it is mostly rewritten.
> Original:
> https://lore.kernel.org/all/20240410142347.964-3-jszhang@kernel.org/
> V3 -> V4:
>  - Add riscv,csr-clint
>  - Allow using of CSRs in S mode
>  - Change if return else return to if return return
>  - Change static_branch_likely to static_branch_unlikely
>  - Fix 32-bit clint_get_cycles64 csr_available check being inverted
>  - Fix is_c900_clint being uninitialized
> V4 -> V5:
>  - Remove riscv,csr-clint
>  - Remove riscv_csr_time_available
>  - Replace all static_branch_unlikely(&riscv_csr_time_available) with
>     riscv_has_extension_likely(RISCV_ISA_EXT_ZICNTR)
> V5 -> V6:
>  - Add csr_clock_next_event
>  - Add csr_get_cycles64 to sched_clock_register
>  - Break get_cycles64 into csr_get_cycles64 and clint_get_cycles64
>  - Change default behavior to use CSR time register
> V6 -> V7:
>  - Split csr and c900 parts into different commits
> 
> Posible future work:
>  - Check for SSTC support and use stimercmp
>  - SSTC depends on smode which cpufeature doesnt check for.
> ---
>  arch/riscv/include/asm/timex.h    | 12 ++++++-
>  drivers/clocksource/timer-clint.c | 57 ++++++++++++++++++++++++++++---
>  2 files changed, 64 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h
> index a06697846e69..d78075351bf4 100644
> --- a/arch/riscv/include/asm/timex.h
> +++ b/arch/riscv/include/asm/timex.h
> @@ -17,17 +17,26 @@ typedef unsigned long cycles_t;
>  #ifdef CONFIG_64BIT
>  static inline cycles_t get_cycles(void)
>  {
> +	if (riscv_has_extension_likely(RISCV_ISA_EXT_ZICNTR))
> +		return csr_read(CSR_TIME);
> +
>  	return readq_relaxed(clint_time_val);
>  }
>  #else /* !CONFIG_64BIT */
>  static inline u32 get_cycles(void)
>  {
> +	if (riscv_has_extension_likely(RISCV_ISA_EXT_ZICNTR))
> +		return csr_read(CSR_TIME);
> +
>  	return readl_relaxed(((u32 *)clint_time_val));
>  }
>  #define get_cycles get_cycles
>  
>  static inline u32 get_cycles_hi(void)
>  {
> +	if (riscv_has_extension_likely(RISCV_ISA_EXT_ZICNTR))
> +		return csr_read(CSR_TIMEH);
> +
>  	return readl_relaxed(((u32 *)clint_time_val) + 1);
>  }
>  #define get_cycles_hi get_cycles_hi
> @@ -40,7 +49,8 @@ static inline u32 get_cycles_hi(void)
>   */
>  static inline unsigned long random_get_entropy(void)
>  {
> -	if (unlikely(clint_time_val == NULL))
> +	if (!riscv_has_extension_likely(RISCV_ISA_EXT_ZICNTR) &&
> +	   (unlikely(clint_time_val == NULL)))
>  		return random_get_entropy_fallback();
>  	return get_cycles();
>  }
> diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
> index 0bdd9d7ec545..18f7bf7d26c6 100644
> --- a/drivers/clocksource/timer-clint.c
> +++ b/drivers/clocksource/timer-clint.c
> @@ -81,6 +81,12 @@ static u64 notrace clint_get_cycles64(void)
>  {
>  	return clint_get_cycles();
>  }
> +
> +static u64 notrace csr_get_cycles64(void)
> +{
> +	return csr_read(CSR_TIME);
> +}
> +
>  #else /* CONFIG_64BIT */
>  static u64 notrace clint_get_cycles64(void)
>  {
> @@ -93,8 +99,26 @@ static u64 notrace clint_get_cycles64(void)
>  
>  	return ((u64)hi << 32) | lo;
>  }
> +
> +static u64 notrace csr_get_cycles64(void)
> +{
> +	u32 hi, lo;
> +
> +	do {
> +		hi = csr_read(CSR_TIMEH);
> +		lo = csr_read(CSR_TIME);
> +	} while (hi != csr_read(CSR_TIMEH));
> +
> +	return ((u64)hi << 32) | lo;
> +}
> +
>  #endif /* CONFIG_64BIT */
>  
> +static u64 csr_rdtime(struct clocksource *cs)
> +{
> +	return csr_get_cycles64();
> +}
> +
>  static u64 clint_rdtime(struct clocksource *cs)
>  {
>  	return clint_get_cycles64();
> @@ -105,9 +129,20 @@ static struct clocksource clint_clocksource = {
>  	.rating		= 300,
>  	.mask		= CLOCKSOURCE_MASK(64),
>  	.flags		= CLOCK_SOURCE_IS_CONTINUOUS,
> -	.read		= clint_rdtime,
> +	.read		= csr_rdtime,
>  };
>  
> +static int csr_clock_next_event(unsigned long delta,
> +				   struct clock_event_device *ce)
> +{
> +	void __iomem *r = clint_timer_cmp +
> +			  cpuid_to_hartid_map(smp_processor_id());
> +
> +	csr_set(CSR_IE, IE_TIE);
> +	writeq_relaxed(csr_get_cycles64() + delta, r);
> +	return 0;
> +}
> +
>  static int clint_clock_next_event(unsigned long delta,
>  				   struct clock_event_device *ce)
>  {
> @@ -123,13 +158,16 @@ static DEFINE_PER_CPU(struct clock_event_device, clint_clock_event) = {
>  	.name		= "clint_clockevent",
>  	.features	= CLOCK_EVT_FEAT_ONESHOT,
>  	.rating		= 100,
> -	.set_next_event	= clint_clock_next_event,
> +	.set_next_event	= csr_clock_next_event,
>  };
>  
>  static int clint_timer_starting_cpu(unsigned int cpu)
>  {
>  	struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
>  
> +	if (!riscv_has_extension_likely(RISCV_ISA_EXT_ZICNTR))
> +		ce->set_next_event = clint_clock_next_event;
> +
>  	ce->cpumask = cpumask_of(cpu);
>  	clockevents_config_and_register(ce, clint_timer_freq, 100, ULONG_MAX);
>  
> @@ -161,7 +199,7 @@ static irqreturn_t clint_timer_interrupt(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> -static int __init clint_timer_init_dt(struct device_node *np)
> +static int __init clint_timer_init(struct device_node *np)
>  {
>  	int rc;
>  	u32 i, nr_irqs;
> @@ -227,13 +265,19 @@ static int __init clint_timer_init_dt(struct device_node *np)
>  
>  	pr_info("%pOFP: timer running at %ld Hz\n", np, clint_timer_freq);
>  
> +	if (!riscv_has_extension_likely(RISCV_ISA_EXT_ZICNTR))
> +		clint_clocksource.read = clint_rdtime;
> +
>  	rc = clocksource_register_hz(&clint_clocksource, clint_timer_freq);
>  	if (rc) {
>  		pr_err("%pOFP: clocksource register failed [%d]\n", np, rc);
>  		goto fail_iounmap;
>  	}
>  
> -	sched_clock_register(clint_get_cycles64, 64, clint_timer_freq);
> +	if (riscv_has_extension_likely(RISCV_ISA_EXT_ZICNTR))
> +		sched_clock_register(csr_get_cycles64, 64, clint_timer_freq);
> +	else
> +		sched_clock_register(clint_get_cycles64, 64, clint_timer_freq);
>  
>  	rc = request_percpu_irq(clint_timer_irq, clint_timer_interrupt,
>  				 "clint-timer", &clint_clock_event);
> @@ -273,5 +317,10 @@ static int __init clint_timer_init_dt(struct device_node *np)
>  	return rc;
>  }
>  
> +static int __init clint_timer_init_dt(struct device_node *np)
> +{
> +	return clint_timer_init(np);
> +}
> +
>  TIMER_OF_DECLARE(clint_timer, "riscv,clint0", clint_timer_init_dt);
>  TIMER_OF_DECLARE(clint_timer1, "sifive,clint0", clint_timer_init_dt);
> -- 
> 2.54.0
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
> 

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv