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