| Message ID | 20200919202141.2072628-1-palmerdabbelt@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] RISC-V: Resurrect the MMIO timer implementation for M-mode systems | expand |
On Sat, 2020-09-19 at 13:21 -0700, 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. > > This also adds the CLINT driver as an !MMU dependency, as it's currently the > only timer driver availiable for these systems and without it we get a build > failure for some configurations. > > Tested-by: Damien Le Moal <damien.lemoal@wdc.com> > Signed-off-by: Palmer Dabbelt <palmerdabbelt@google.com> There is no changelog... What changed from V1 ? Do I need to test again ? > --- > arch/riscv/Kconfig | 1 + > arch/riscv/include/asm/clint.h | 26 ++++++++++++++++++++++++++ > arch/riscv/include/asm/timex.h | 27 +++++++++++++++++++++++++++ > drivers/clocksource/timer-clint.c | 17 +++++++++++++++++ > 4 files changed, 71 insertions(+) > create mode 100644 arch/riscv/include/asm/clint.h > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig > index df18372861d8..7766e1289468 100644 > --- a/arch/riscv/Kconfig > +++ b/arch/riscv/Kconfig > @@ -32,6 +32,7 @@ config RISCV > select ARCH_WANT_FRAME_POINTERS > select ARCH_WANT_HUGE_PMD_SHARE if 64BIT > select CLONE_BACKWARDS > + select CLINT_TIMER if !MMU > select COMMON_CLK > select EDAC_SUPPORT > select GENERIC_ARCH_TOPOLOGY if SMP > 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);
On Sat, 19 Sep 2020 20:31:32 PDT (-0700), Damien Le Moal wrote: > On Sat, 2020-09-19 at 13:21 -0700, 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. >> >> This also adds the CLINT driver as an !MMU dependency, as it's currently the >> only timer driver availiable for these systems and without it we get a build >> failure for some configurations. >> >> Tested-by: Damien Le Moal <damien.lemoal@wdc.com> >> Signed-off-by: Palmer Dabbelt <palmerdabbelt@google.com> > > There is no changelog... What changed from V1 ? Do I need to test again ? It's just the driver dependency, so allnoconfig and such build. Testing again shouldn't be necessary, but I guess it never hurts. > >> --- >> arch/riscv/Kconfig | 1 + >> arch/riscv/include/asm/clint.h | 26 ++++++++++++++++++++++++++ >> arch/riscv/include/asm/timex.h | 27 +++++++++++++++++++++++++++ >> drivers/clocksource/timer-clint.c | 17 +++++++++++++++++ >> 4 files changed, 71 insertions(+) >> create mode 100644 arch/riscv/include/asm/clint.h >> >> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig >> index df18372861d8..7766e1289468 100644 >> --- a/arch/riscv/Kconfig >> +++ b/arch/riscv/Kconfig >> @@ -32,6 +32,7 @@ config RISCV >> select ARCH_WANT_FRAME_POINTERS >> select ARCH_WANT_HUGE_PMD_SHARE if 64BIT >> select CLONE_BACKWARDS >> + select CLINT_TIMER if !MMU >> select COMMON_CLK >> select EDAC_SUPPORT >> select GENERIC_ARCH_TOPOLOGY if SMP >> 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
diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig index df18372861d8..7766e1289468 100644 --- a/arch/riscv/Kconfig +++ b/arch/riscv/Kconfig @@ -32,6 +32,7 @@ config RISCV select ARCH_WANT_FRAME_POINTERS select ARCH_WANT_HUGE_PMD_SHARE if 64BIT select CLONE_BACKWARDS + select CLINT_TIMER if !MMU select COMMON_CLK select EDAC_SUPPORT select GENERIC_ARCH_TOPOLOGY if SMP 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);