Re: [PATCH] riscv: timer: Add thead,c900-clint compatible

[Yao Zi <ziyao@disroot.org>]

On Thu, Feb 20, 2025 at 10:31:53AM +0000, Yao Zi wrote:
> Current implementation of riscv_timer.c only assumes readable TIMER CSRs
> present (IOW, Zicntr extension is available). Core Local Interruptors
> (CLINT) found on T-Head C9xx cores expose its mtime register through
> TIME CSR directly instead of a MMIO register, thus is compatible with
> the driver.
> 
> As running in S-Mode isn't necessary for the driver to operate, Kconfig
> and comments are also adapted to avoid confusion.
> 
> Reference: https://github.com/riscv-software-src/opensbi/commit/ca7810aecdba
> Signed-off-by: Yao Zi <ziyao@disroot.org>
> ---
> 
> This is necessary for several platforms based on T-Head C9xx to get
> system ticks, e.g. TH1520, K230 and CV1800. Tested on
> th1520-lichee-pi-4a in both M-mode and S-mode (with mainline OpenSBI).

Correction: it isn't necessary for all these C9xx platforms, since
drivers/cpu/riscv_cpu.c tries to bind a timer with the boot HART.

But there're still several problems left to solve,

1. It's really surprising to find out the driver may be manually bound
   somewhere else.
2. The comments in riscv_timer.c and its corresponding Kconfig help text
   are misleading. The driver DOES work under M-mode and there're
   already platforms depending on its behaviour in M-Mode (C9xx-based
   ones, shipping a T-Head CLINT).
3. Unconditionally binding riscv_timer driver to every "riscv"
   compatible core is technically wrong. RISC-V guarantees valid access
   to TIME CSR only when Zicntr extension exists, which isn't even part
   of the common baseline, RV64GC.
4. As a side effect of manually binding the timer driver, we lose the
   special early-stage initialization for timers in
   lib/time.c:get_ticks(). What's worse is
5. Earlytimer-related code in riscv_timer.c is conditionally compiled
   for S-mode only.

My proposal is,

- Remove the timer-binding code in drivers/cpu/riscv_cpu.c
- Correct help text and comments for riscv_timer.c, just like what has
  been done in this patch.
- Rename RISCV_SMODE_TIMER_FREQ to RISCV_EARLY_TIMER_FREQ, clean up
  preprocessor instructions that limit some functions to S-Mode only.
  They shouldn't be S-mode only stuff.
- For future RISC-V cores that are capable of reading timestamp from
  TIME CSR, we could either register the compatible string of its timer
  to riscv_timer.c (T-Head case, the underlying CLINT isn't a real
  SSTC-capable device) or add a "riscv,timer" node.

I'm willing to work on a series as RFC.

Best regards,
Yao Zi

>  drivers/timer/Kconfig       | 4 ++--
>  drivers/timer/riscv_timer.c | 3 ++-
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/timer/Kconfig b/drivers/timer/Kconfig
> index cb6fc0e7fda..00c4067a23b 100644
> --- a/drivers/timer/Kconfig
> +++ b/drivers/timer/Kconfig
> @@ -241,8 +241,8 @@ config RISCV_TIMER
>  	bool "RISC-V timer support"
>  	depends on TIMER && RISCV
>  	help
> -	  Select this to enable support for a generic RISC-V S-Mode timer
> -	  driver.
> +	  Select this to enable support for a generic timer driver based
> +	  on RISC-V TIMER CSR.
>  
>  config ROCKCHIP_TIMER
>  	bool "Rockchip timer support"
> diff --git a/drivers/timer/riscv_timer.c b/drivers/timer/riscv_timer.c
> index 1f4980ceb38..3e6f0cd6d11 100644
> --- a/drivers/timer/riscv_timer.c
> +++ b/drivers/timer/riscv_timer.c
> @@ -7,7 +7,7 @@
>   *
>   * RISC-V architecturally-defined generic timer driver
>   *
> - * This driver provides generic timer support for S-mode U-Boot.
> + * This driver provides generic timer support through TIME CSR for U-Boot.
>   */
>  
>  #include <config.h>
> @@ -106,6 +106,7 @@ static const struct timer_ops riscv_timer_ops = {
>  
>  static const struct udevice_id riscv_timer_ids[] = {
>  	{ .compatible = "riscv,timer", },
> +	{ .compatible = "thead,c900-clint" },
>  	{ }
>  };
>  
> -- 
> 2.48.1
>