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

[Damien Le Moal]

On 2020/09/15 2:17, Palmer Dabbelt wrote:
> 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).

All kernels from 5.8.5 break flatbin user space with nommu. The start of  the
problem is Christoph's recent flatbin loader change which was backported to
5.8.5. But that change is correct and the problem is likely due to uclibc or
elf2flt. But I have not yet figured out the exact root cause.

So I can test this on Kendryte, but only boot the kernel and then crash in
userspace... I can test on top of 5.8.0 too, with a userspace running.


> ---
>  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;
>  
> +#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 */
> +
> +#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
>  
>  #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
> +
>  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
> +
>  	pr_info("%pOFP: timer running at %ld Hz\n", np, clint_timer_freq);
>  
>  	rc = clocksource_register_hz(&clint_clocksource, clint_timer_freq);
> 


-- 
Damien Le Moal
Western Digital Research