[PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems
Anup Patel
Anup.Patel at wdc.com
Sat Sep 19 00:54:10 EDT 2020
> -----Original Message-----
> From: Palmer Dabbelt <palmerdabbelt at google.com>
> Sent: 19 September 2020 10:07
> 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 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.
That's why I had made it a callback function which CLINT driver can set and
other timer drivers can also set it. This way we are not mandating CLINT for
NoMMU kernel.
There is one more issue in your patch. You need to check "clint_time_val"
for NULL before using it because some parts of kernel will call delay timer
before CLINT driver is probed.
Regards,
Anup
>
> >
> > 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
More information about the linux-riscv
mailing list