Message-ID: <CAAhSdy3x9jhfwvgFwwoYHQyNXkF7h+zSQU0uPkqOj5M=xZUfbw@mail.gmail.com>
Date: Tue, 7 Jan 2025 11:14:23 +0530
From: Anup Patel <anup@...infault.org>
To: Jesse Taube <mr.bossman075@...il.com>
Cc: linux-riscv@...ts.infradead.org, Paul Walmsley <paul.walmsley@...ive.com>, 
	Palmer Dabbelt <palmer@...belt.com>, Albert Ou <aou@...s.berkeley.edu>, 
	Daniel Lezcano <daniel.lezcano@...aro.org>, Thomas Gleixner <tglx@...utronix.de>, 
	Samuel Holland <samuel.holland@...ive.com>, Conor Dooley <conor@...nel.org>, 
	Icenowy Zheng <uwu@...nowy.me>, Jisheng Zhang <jszhang@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] clocksource/drivers/timer-clint: Add T-Head C9xx clint

On Tue, Jan 7, 2025 at 8:49 AM Jesse Taube <mr.bossman075@...il.com> wrote:
>
> From: Jisheng Zhang <jszhang@...nel.org>
>
> To use the T-HEAD C9xx clint in RISCV-M NOMMU env, we need to take
> care two points:
>
> 1.The mtimecmp in T-Head C9xx clint only supports 32bit read/write,
> implement such support.
>
> 2. As pointed out by commit ca7810aecdba ("lib: utils/timer: mtimer:
> add a quirk for lacking mtime register") of opensbi:
>
> "T-Head developers surely have a different understanding of time CSR and
> CLINT's mtime register with SiFive ones, that they did not implement
> the mtime register at all -- as shown in openC906 source code, their
> time CSR value is just exposed at the top of their processor IP block
> and expects an external continous counter, which makes it not
> overrideable, and thus mtime register is not implemented, even not for
> reading. However, if CLINTEE is not enabled in T-Head's MXSTATUS
> extended CSR, these systems still rely on the mtimecmp registers to
> generate timer interrupts. This makes it necessary to implement T-Head
> C9xx CLINT support in OpenSBI MTIMER driver, which skips implementing
> reading mtime register and falls back to default code that reads time
> CSR."
>
> So, we need to fall back to read time CSR instead of mtime register.
> Add riscv_csr_time_available static key for this purpose.
>
> Signed-off-by: Jisheng Zhang <jszhang@...nel.org>
> Signed-off-by: Jesse Taube <Mr.Bossman075@...il.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
> ---
>  arch/riscv/include/asm/clint.h    |  2 +
>  arch/riscv/include/asm/timex.h    | 12 +++++-
>  drivers/clocksource/timer-clint.c | 64 ++++++++++++++++++++++++++++---
>  3 files changed, 72 insertions(+), 6 deletions(-)
>
> diff --git a/arch/riscv/include/asm/clint.h b/arch/riscv/include/asm/clint.h
> index 0789fd37b40a..9900357b855d 100644
> --- a/arch/riscv/include/asm/clint.h
> +++ b/arch/riscv/include/asm/clint.h
> @@ -8,6 +8,7 @@
>
>  #include <linux/types.h>
>  #include <asm/mmio.h>
> +#include <linux/jump_label.h>
>
>  #ifdef CONFIG_RISCV_M_MODE
>  /*
> @@ -23,4 +24,5 @@
>  extern u64 __iomem *clint_time_val;
>  #endif
>
> +DECLARE_STATIC_KEY_FALSE(riscv_csr_time_available);
>  #endif
> diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h
> index a06697846e69..914d52296c24 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 (static_branch_unlikely(&riscv_csr_time_available))
> +               return csr_read(CSR_TIME);
> +
>         return readq_relaxed(clint_time_val);
>  }
>  #else /* !CONFIG_64BIT */
>  static inline u32 get_cycles(void)
>  {
> +       if (static_branch_unlikely(&riscv_csr_time_available))
> +               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 (static_branch_unlikely(&riscv_csr_time_available))
> +               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 (!static_branch_unlikely(&riscv_csr_time_available) &&
> +          (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..dd0fa1550a6e 100644
> --- a/drivers/clocksource/timer-clint.c
> +++ b/drivers/clocksource/timer-clint.c
> @@ -39,12 +39,16 @@ static u64 __iomem *clint_timer_cmp;
>  static u64 __iomem *clint_timer_val;
>  static unsigned long clint_timer_freq;
>  static unsigned int clint_timer_irq;
> +static bool is_c900_clint;
>
>  #ifdef CONFIG_RISCV_M_MODE
>  u64 __iomem *clint_time_val;
>  EXPORT_SYMBOL(clint_time_val);
>  #endif
>
> +DEFINE_STATIC_KEY_FALSE(riscv_csr_time_available);
> +EXPORT_SYMBOL(riscv_csr_time_available);
> +
>  #ifdef CONFIG_SMP
>  static void clint_send_ipi(unsigned int cpu)
>  {
> @@ -79,6 +83,9 @@ static void clint_ipi_interrupt(struct irq_desc *desc)
>  #ifdef CONFIG_64BIT
>  static u64 notrace clint_get_cycles64(void)
>  {
> +       if (static_branch_unlikely(&riscv_csr_time_available))
> +               return csr_read(CSR_TIME);
> +
>         return clint_get_cycles();
>  }
>  #else /* CONFIG_64BIT */
> @@ -86,10 +93,17 @@ static u64 notrace clint_get_cycles64(void)
>  {
>         u32 hi, lo;
>
> -       do {
> -               hi = clint_get_cycles_hi();
> -               lo = clint_get_cycles();
> -       } while (hi != clint_get_cycles_hi());
> +       if (static_branch_unlikely(&riscv_csr_time_available)) {
> +               do {
> +                       hi = csr_read(CSR_TIMEH);
> +                       lo = csr_read(CSR_TIME);
> +               } while (hi != csr_read(CSR_TIMEH));
> +       } else {
> +               do {
> +                       hi = clint_get_cycles_hi();
> +                       lo = clint_get_cycles();
> +               } while (hi != clint_get_cycles_hi());
> +       }
>
>         return ((u64)hi << 32) | lo;
>  }
> @@ -119,6 +133,19 @@ static int clint_clock_next_event(unsigned long delta,
>         return 0;
>  }
>
> +static int c900_clint_clock_next_event(unsigned long delta,
> +                                      struct clock_event_device *ce)
> +{
> +       void __iomem *r = clint_timer_cmp +
> +                         cpuid_to_hartid_map(smp_processor_id());
> +       u64 val = clint_get_cycles64() + delta;
> +
> +       csr_set(CSR_IE, IE_TIE);
> +       writel_relaxed(val, r);
> +       writel_relaxed(val >> 32, r + 4);
> +       return 0;
> +}
> +
>  static DEFINE_PER_CPU(struct clock_event_device, clint_clock_event) = {
>         .name           = "clint_clockevent",
>         .features       = CLOCK_EVT_FEAT_ONESHOT,
> @@ -130,6 +157,9 @@ static int clint_timer_starting_cpu(unsigned int cpu)
>  {
>         struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
>
> +       if (is_c900_clint)
> +               ce->set_next_event = c900_clint_clock_next_event;
> +
>         ce->cpumask = cpumask_of(cpu);
>         clockevents_config_and_register(ce, clint_timer_freq, 100, ULONG_MAX);
>
> @@ -161,7 +191,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;
> @@ -273,5 +303,29 @@ static int __init clint_timer_init_dt(struct device_node *np)
>         return rc;
>  }
>
> +static int __init clint_timer_init_dt(struct device_node *np)
> +{
> +       is_c900_clint = false;
> +       return clint_timer_init(np);
> +}
> +
> +static int __init c900_clint_timer_init_dt(struct device_node *np)
> +{
> +       is_c900_clint = true;
> +       static_branch_enable(&riscv_csr_time_available);
> +
> +       return clint_timer_init(np);
> +}
> +
> +static int __init csr_clint_timer_init_dt(struct device_node *np)
> +{
> +       is_c900_clint = false;
> +       static_branch_enable(&riscv_csr_time_available);
> +
> +       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);
> +TIMER_OF_DECLARE(clint_timer2, "thead,c900-clint", c900_clint_timer_init_dt);
> +TIMER_OF_DECLARE(clint_timer3, "riscv,csr-clint", csr_clint_timer_init_dt);

Detect Zicntr from ISA string instead of introducing a new compatible string.

Regards,
Anup

Powered by blists - more mailing lists