[PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems

Sat Sep 19 00:36:44 EDT 2020

On Fri, 18 Sep 2020 20:32:12 PDT (-0700), Anup Patel wrote:
> 
> 
>> -----Original Message-----
>> From: Palmer Dabbelt <palmerdabbelt at google.com>
>> Sent: 19 September 2020 08:24
>> To: Anup Patel <Anup.Patel at wdc.com>
>> Cc: linux-riscv at lists.infradead.org; hch at infradead.org; kernel-
>> team at android.com; anup at brainfault.org
>> Subject: RE: [PATCH] RISC-V: Resurrect the MMIO timer implementation for
>> M-mode systems
>> 
>> On Mon, 14 Sep 2020 19:11:46 PDT (-0700), Anup Patel wrote:
>> >
>> >
>> >> -----Original Message-----
>> >> From: Palmer Dabbelt <palmerdabbelt at google.com>
>> >> Sent: 14 September 2020 22:27
>> >> To: linux-riscv at lists.infradead.org
>> >> Cc: hch at infradead.org; Anup Patel <Anup.Patel at wdc.com>; Palmer
>> >> Dabbelt <palmerdabbelt at google.com>; kernel-team at android.com
>> >> Subject: [PATCH] RISC-V: Resurrect the MMIO timer implementation for
>> >> M- mode systems
>> >>
>> >> The K210 doesn't implement rdtime in M-mode, and since that's where
>> >> Linux runs in the NOMMU systems that means we can't use rdtime.  The
>> >> K210 is the only system that anyone is currently running NOMMU or
>> >> M-mode on, so here we're just inlining the timer read directly.
>> >>
>> >> Signed-off-by: Palmer Dabbelt <palmerdabbelt at google.com>
>> >> ---
>> >> I don't actually have a K210 so I haven't tested this.  If nobody
>> >> else has the time to I'll put together a QEMU that doesn't support
>> >> rdtime in M-mode, but I've yet to mess around with the !MMU stuff so
>> that might take a little while.
>> >> This certainly doesn't seem worse than what's there right now,
>> >> though, as rdtime isn't valid in M-mode on the K210 (our only M-mode
>> platform).
>> >
>> > I think you missed my comments in the previous email thread.
>> >
>> > I would request to make this patch bit more CLINT independent.
>> >
>> > Please see inline below.
>> >
>> >> ---
>> >>  arch/riscv/include/asm/clint.h    | 26 ++++++++++++++++++++++++++
>> >>  arch/riscv/include/asm/timex.h    | 27 +++++++++++++++++++++++++++
>> >>  drivers/clocksource/timer-clint.c | 17 +++++++++++++++++
>> >>  3 files changed, 70 insertions(+)
>> >>  create mode 100644 arch/riscv/include/asm/clint.h
>> >>
>> >> diff --git a/arch/riscv/include/asm/clint.h
>> >> b/arch/riscv/include/asm/clint.h new file mode 100644 index
>> >> 000000000000..0789fd37b40a
>> >> --- /dev/null
>> >> +++ b/arch/riscv/include/asm/clint.h
>> >> @@ -0,0 +1,26 @@
>> >> +/* SPDX-License-Identifier: GPL-2.0-only */
>> >> +/*
>> >> + * Copyright (C) 2020 Google, Inc
>> >> + */
>> >> +
>> >> +#ifndef _ASM_RISCV_CLINT_H
>> >> +#define _ASM_RISCV_CLINT_H
>> >> +
>> >> +#include <linux/types.h>
>> >> +#include <asm/mmio.h>
>> >> +
>> >> +#ifdef CONFIG_RISCV_M_MODE
>> >> +/*
>> >> + * This lives in the CLINT driver, but is accessed directly by
>> >> +timex.h to avoid
>> >> + * any overhead when accessing the MMIO timer.
>> >> + *
>> >> + * The ISA defines mtime as a 64-bit memory-mapped register that
>> >> +increments at
>> >> + * a constant frequency, but it doesn't define some other
>> >> +constraints we depend
>> >> + * on (most notably ordering constraints, but also some simpler
>> >> +stuff like the
>> >> + * memory layout).  Thus, this is called "clint_time_val" instead of
>> >> +something
>> >> + * like "riscv_mtime", to signify that these non-ISA assumptions must
>> hold.
>> >> + */
>> >> +extern u64 __iomem *clint_time_val;
>> >> +#endif
>> >> +
>> >> +#endif
>> >> diff --git a/arch/riscv/include/asm/timex.h
>> >> b/arch/riscv/include/asm/timex.h index a3fb85d505d4..7f659dda0032
>> >> 100644
>> >> --- a/arch/riscv/include/asm/timex.h
>> >> +++ b/arch/riscv/include/asm/timex.h
>> >> @@ -10,6 +10,31 @@
>> >>
>> >>  typedef unsigned long cycles_t;
>> >
>> > The entire asm/clint.h can be avoided by adding "extern u64 __iomem
>> > *riscv_mmio_time_val;"
>> > here in n asm/timex.h
>> >
>> > Also, this will make it bit more CLINT independent.
>> 
>> Except that, as per the comments, it's not CLINT independent.
> 
> We just need some 64bit MMIO register. It could be CLINT TIME register
> or some other timer. I don't see anything CLINT specific apart from the
> variable name "clint_time_val".

No, clint_time_val has other constraints.  The comment I added to describe
these after you asked the first time is only a few lines above this:

    +#ifdef CONFIG_RISCV_M_MODE
    +/*
    + * This lives in the CLINT driver, but is accessed directly by timex.h to avoid
    + * any overhead when accessing the MMIO timer.
    + *
    + * The ISA defines mtime as a 64-bit memory-mapped register that increments at
    + * a constant frequency, but it doesn't define some other constraints we depend
    + * on (most notably ordering constraints, but also some simpler stuff like the
    + * memory layout).  Thus, this is called "clint_time_val" instead of something
    + * like "riscv_mtime", to signify that these non-ISA assumptions must hold.
    + */
    +extern u64 __iomem *clint_time_val;
    +#endif

To be more detailed, there's two issues:

* The ISA doesn't define a memory layout.  This one is a bit pedantic, as
  there's an example of how to use the mtime/mtimeh split that would only work
  for this layout, but those sorts of things tend to be non-canonical in the ISA
  manual.
* The ISA doesn't define any ordering constraints on mtime.  This one is way
  more important, as right now we have a big pile of implicit ordering
  constraints on IO regions that are required to make systems actually work.
  IIUC the argument right now is that we're just relying on the various device
  specifications to provide these (ie, if you have PCI then you better respect
  their memory ordering consraints for your system), but I'm not sure that's as
  strong of a position as we should be taking.

  RISC-V defines no orderings, however, for mtime.  IIRC there are some
  implicit orderings on rdtime that we depend on (though I can't dig the text out
  of the ISA tonight) but those aren't guaranteed by the ISA for mtime.  This all
  works on SiFive's systems because the CLINT provides these guarntees, but
  that's not generally the case for mtime as defined by RISC-V.

  Since mtime is a RISC-V specific thing we can't rely on some other
  specification like we do for other devices.  We could, of course, fence
  around this but that's likely to be way more expensive than the predictable
  indirection that was called out as being too slow in the first place.

So this really is a CLINT-specific register.  That all seems a bit too much for
the commit text, though, so I'm inclined to leave it as is.

> 
> Regards,
> Anup
> 
>> 
>> >> +#ifdef CONFIG_RISCV_M_MODE
>> >> +
>> >> +#include <asm/clint.h>
>> >> +
>> >> +#ifdef CONFIG_64BIT
>> >> +static inline cycles_t get_cycles(void) {
>> >> +	return readq_relaxed(clint_time_val); } #else /* !CONFIG_64BIT */
>> >> +static inline u32 get_cycles(void) {
>> >> +	return readl_relaxed(((u32 *)clint_time_val)); } #define get_cycles
>> >> +get_cycles
>> >> +
>> >> +static inline u32 get_cycles_hi(void) {
>> >> +	return readl_relaxed(((u32 *)clint_time_val) + 1); } #define
>> >> +get_cycles_hi get_cycles_hi #endif /* CONFIG_64BIT */
>> >
>> > Use riscv_mmio_time_val instead of clint_time_val above.
>> >
>> >> +
>> >> +#else /* CONFIG_RISCV_M_MODE */
>> >> +
>> >>  static inline cycles_t get_cycles(void)  {
>> >>  	return csr_read(CSR_TIME);
>> >> @@ -41,6 +66,8 @@ static inline u64 get_cycles64(void)  }  #endif /*
>> >> CONFIG_64BIT */
>> >>
>> >> +#endif /* !CONFIG_RISCV_M_MODE */
>> >> +
>> >>  #define ARCH_HAS_READ_CURRENT_TIMER
>> >>  static inline int read_current_timer(unsigned long *timer_val)  {
>> >> diff --git a/drivers/clocksource/timer-clint.c
>> >> b/drivers/clocksource/timer-clint.c
>> >> index 8eeafa82c03d..d17367dee02c 100644
>> >> --- a/drivers/clocksource/timer-clint.c
>> >> +++ b/drivers/clocksource/timer-clint.c
>> >> @@ -19,6 +19,11 @@
>> >>  #include <linux/interrupt.h>
>> >>  #include <linux/of_irq.h>
>> >>  #include <linux/smp.h>
>> >> +#include <linux/timex.h>
>> >> +
>> >> +#ifndef CONFIG_RISCV_M_MODE
>> >> +#include <asm/clint.h>
>> >> +#endif
>> >
>> > As-per, above suggestion drop this #include.
>> >
>> >>
>> >>  #define CLINT_IPI_OFF		0
>> >>  #define CLINT_TIMER_CMP_OFF	0x4000
>> >> @@ -31,6 +36,10 @@ static u64 __iomem *clint_timer_val;  static
>> >> unsigned long clint_timer_freq;  static unsigned int clint_timer_irq;
>> >>
>> >> +#ifdef CONFIG_RISCV_M_MODE
>> >> +u64 __iomem *clint_time_val;
>> >> +#endif
>> >
>> > Define and export "u64 __iomem *riscv_mmio_time_val" in
>> > arch/riscv/kernel/time.c
>> >
>> > This way no "clint_time_val" definition required here as well and
>> > other MMIO timers can also set riscv_mmio_time_val.
>> >
>> >> +
>> >>  static void clint_send_ipi(const struct cpumask *target)  {
>> >>  	unsigned int cpu;
>> >> @@ -184,6 +193,14 @@ static int __init clint_timer_init_dt(struct
>> >> device_node *np)
>> >>  	clint_timer_val = base + CLINT_TIMER_VAL_OFF;
>> >>  	clint_timer_freq = riscv_timebase;
>> >>
>> >> +#ifdef CONFIG_RISCV_M_MODE
>> >> +	/*
>> >> +	 * Yes, that's an odd naming scheme.  time_val is public, but
>> >> hopefully
>> >> +	 * will die in favor of something cleaner.
>> >> +	 */
>> >> +	clint_time_val = clint_timer_val;
>> >> +#endif
>> >> +
>> >
>> > Just set " riscv_mmio_time_val = clint_timer_val" here.
>> >
>> >>  	pr_info("%pOFP: timer running at %ld Hz\n", np, clint_timer_freq);
>> >>
>> >>  	rc = clocksource_register_hz(&clint_clocksource, clint_timer_freq);
>> >> --
>> >> 2.28.0.618.gf4bc123cb7-goog
>> >
>> > Regards,
>> > Anup