Re: [PATCH v4] clocksource/drivers/timer-clint: Add T-Head C9xx clint

[Charlie Jenkins <charlie@rivosinc.com>]

On Tue, Jan 07, 2025 at 11:14:23AM +0530, Anup Patel wrote:
> On Tue, Jan 7, 2025 at 8:49 AM Jesse Taube <mr.bossman075@gmail.com> wrote:
> >
> > From: Jisheng Zhang <jszhang@kernel.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@kernel.org>
> > Signed-off-by: Jesse Taube <Mr.Bossman075@gmail.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.

Just for more context Jesse, you should be able to do
'riscv_has_extension_likely(RISCV_ISA_EXT_ZICNTR)' instead of the static
branches.

- Charlie

> 
> Regards,
> Anup
> 
> _______________________________________________
> 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