* [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems
@ 2020-09-14 16:56 Palmer Dabbelt
2020-09-14 17:22 ` Mohammed Billoo
` (5 more replies)
0 siblings, 6 replies; 19+ messages in thread
From: Palmer Dabbelt @ 2020-09-14 16:56 UTC (permalink / raw)
To: linux-riscv; +Cc: Christoph Hellwig, Anup Patel, kernel-team, Palmer Dabbelt
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@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).
---
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);
--
2.28.0.618.gf4bc123cb7-goog
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems
2020-09-14 16:56 [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems Palmer Dabbelt
@ 2020-09-14 17:22 ` Mohammed Billoo
2020-09-15 3:06 ` Palmer Dabbelt
2020-09-15 5:10 ` Damien Le Moal
2020-09-15 2:11 ` Anup Patel
` (4 subsequent siblings)
5 siblings, 2 replies; 19+ messages in thread
From: Mohammed Billoo @ 2020-09-14 17:22 UTC (permalink / raw)
To: Palmer Dabbelt; +Cc: Christoph Hellwig, linux-riscv, Anup Patel, kernel-team
I can test this for you. Can you provide a high level description on
what you'd like tested (it's not obvious to me what impact your patch
will have)?
Thanks
On Mon, Sep 14, 2020 at 1:16 PM Palmer Dabbelt <palmerdabbelt@google.com> 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@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).
> ---
> 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);
> --
> 2.28.0.618.gf4bc123cb7-goog
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
--
Mohammed A Billoo
Founder
MAB Labs, LLC
www.mab-labs.com
201-338-2022
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems
2020-09-14 16:56 [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems Palmer Dabbelt
2020-09-14 17:22 ` Mohammed Billoo
@ 2020-09-15 2:11 ` Anup Patel
2020-09-19 2:54 ` Palmer Dabbelt
2020-09-15 5:08 ` Damien Le Moal
` (3 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Anup Patel @ 2020-09-15 2:11 UTC (permalink / raw)
To: Palmer Dabbelt, linux-riscv; +Cc: hch, Anup Patel, kernel-team
> -----Original Message-----
> From: Palmer Dabbelt <palmerdabbelt@google.com>
> Sent: 14 September 2020 22:27
> To: linux-riscv@lists.infradead.org
> Cc: hch@infradead.org; Anup Patel <Anup.Patel@wdc.com>; Palmer Dabbelt
> <palmerdabbelt@google.com>; kernel-team@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@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.
>
> +#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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems
2020-09-14 17:22 ` Mohammed Billoo
@ 2020-09-15 3:06 ` Palmer Dabbelt
2020-09-15 5:10 ` Damien Le Moal
1 sibling, 0 replies; 19+ messages in thread
From: Palmer Dabbelt @ 2020-09-15 3:06 UTC (permalink / raw)
To: mab; +Cc: Christoph Hellwig, linux-riscv, Anup Patel, kernel-team
On Mon, 14 Sep 2020 10:22:01 PDT (-0700), mab@mab-labs.com wrote:
> I can test this for you. Can you provide a high level description on
> what you'd like tested (it's not obvious to me what impact your patch
> will have)?
Just booting on the K210 should be sufficient. I doubt you even need
userspace, as you should get an illegal instruction pretty quickly without
this.
>
> Thanks
>
>
> On Mon, Sep 14, 2020 at 1:16 PM Palmer Dabbelt <palmerdabbelt@google.com> 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@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).
>> ---
>> 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);
>> --
>> 2.28.0.618.gf4bc123cb7-goog
>>
>>
>> _______________________________________________
>> 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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems
2020-09-14 16:56 [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems Palmer Dabbelt
2020-09-14 17:22 ` Mohammed Billoo
2020-09-15 2:11 ` Anup Patel
@ 2020-09-15 5:08 ` Damien Le Moal
2020-09-15 5:46 ` Christoph Hellwig
` (2 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Damien Le Moal @ 2020-09-15 5:08 UTC (permalink / raw)
To: Palmer Dabbelt, linux-riscv; +Cc: hch, Anup Patel, kernel-team
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@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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems
2020-09-14 17:22 ` Mohammed Billoo
2020-09-15 3:06 ` Palmer Dabbelt
@ 2020-09-15 5:10 ` Damien Le Moal
1 sibling, 0 replies; 19+ messages in thread
From: Damien Le Moal @ 2020-09-15 5:10 UTC (permalink / raw)
To: Mohammed Billoo, Palmer Dabbelt; +Cc: hch, linux-riscv, Anup Patel, kernel-team
On 2020/09/15 2:23, Mohammed Billoo wrote:
> I can test this for you. Can you provide a high level description on
> what you'd like tested (it's not obvious to me what impact your patch
> will have)?
Are you able to run a userspace on 5.9 on the kendryte ? How do you build it ?
All my attempts fail to run correctly for any kernel > 5.8.4.
>
> Thanks
>
>
> On Mon, Sep 14, 2020 at 1:16 PM Palmer Dabbelt <palmerdabbelt@google.com> 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@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).
>> ---
>> 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);
>> --
>> 2.28.0.618.gf4bc123cb7-goog
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
>
>
--
Damien Le Moal
Western Digital Research
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems
2020-09-14 16:56 [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems Palmer Dabbelt
` (2 preceding siblings ...)
2020-09-15 5:08 ` Damien Le Moal
@ 2020-09-15 5:46 ` Christoph Hellwig
2020-09-15 6:50 ` Damien Le Moal
2020-09-15 8:25 ` Damien Le Moal
2020-09-15 12:56 ` Damien Le Moal
5 siblings, 1 reply; 19+ messages in thread
From: Christoph Hellwig @ 2020-09-15 5:46 UTC (permalink / raw)
To: Palmer Dabbelt; +Cc: Christoph Hellwig, linux-riscv, Anup Patel, kernel-team
I won't have time to set up the K210, but this fixes the timer issues
on older Qemu versions that don't implement the time CSR. Although I
also need to revert the binfmt_flat patch to actually get to userspace.
Tested-by: Christoph Hellwig <hch@lst.de>
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems
2020-09-15 5:46 ` Christoph Hellwig
@ 2020-09-15 6:50 ` Damien Le Moal
2020-09-15 6:51 ` hch
0 siblings, 1 reply; 19+ messages in thread
From: Damien Le Moal @ 2020-09-15 6:50 UTC (permalink / raw)
To: hch, Palmer Dabbelt; +Cc: linux-riscv, Anup Patel, kernel-team
On 2020/09/15 14:47, Christoph Hellwig wrote:
> I won't have time to set up the K210, but this fixes the timer issues
> on older Qemu versions that don't implement the time CSR. Although I
> also need to revert the binfmt_flat patch to actually get to userspace.
So revert is needed ? Digging in the flatbin format, I do not see why the offset
you removed would be needed, and it does not seem to affect other arch anyway.
So I am really suspecting a problem with riscv uclibcb or elf2flt, but can't
quite yet figure out what may be wrong there.
>
> Tested-by: Christoph Hellwig <hch@lst.de>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
--
Damien Le Moal
Western Digital Research
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems
2020-09-15 6:50 ` Damien Le Moal
@ 2020-09-15 6:51 ` hch
2020-09-15 7:01 ` Damien Le Moal
0 siblings, 1 reply; 19+ messages in thread
From: hch @ 2020-09-15 6:51 UTC (permalink / raw)
To: Damien Le Moal; +Cc: hch, Anup Patel, linux-riscv, Palmer Dabbelt, kernel-team
On Tue, Sep 15, 2020 at 06:50:24AM +0000, Damien Le Moal wrote:
> On 2020/09/15 14:47, Christoph Hellwig wrote:
> > I won't have time to set up the K210, but this fixes the timer issues
> > on older Qemu versions that don't implement the time CSR. Although I
> > also need to revert the binfmt_flat patch to actually get to userspace.
>
> So revert is needed ? Digging in the flatbin format, I do not see why the offset
> you removed would be needed, and it does not seem to affect other arch anyway.
> So I am really suspecting a problem with riscv uclibcb or elf2flt, but can't
> quite yet figure out what may be wrong there.
I did not have time to find out if what we did in RISC-V nommu was
correct or wrong. But to get my existing userspace to work I had to
revert the revert. I'll try to find some time to dig into the binfmt
flat issue and elf2flt soon.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems
2020-09-15 6:51 ` hch
@ 2020-09-15 7:01 ` Damien Le Moal
2020-10-27 4:12 ` Atish Patra
0 siblings, 1 reply; 19+ messages in thread
From: Damien Le Moal @ 2020-09-15 7:01 UTC (permalink / raw)
To: hch; +Cc: Anup Patel, linux-riscv, Palmer Dabbelt, kernel-team
On 2020/09/15 15:51, hch@infradead.org wrote:
> On Tue, Sep 15, 2020 at 06:50:24AM +0000, Damien Le Moal wrote:
>> On 2020/09/15 14:47, Christoph Hellwig wrote:
>>> I won't have time to set up the K210, but this fixes the timer issues
>>> on older Qemu versions that don't implement the time CSR. Although I
>>> also need to revert the binfmt_flat patch to actually get to userspace.
>>
>> So revert is needed ? Digging in the flatbin format, I do not see why the offset
>> you removed would be needed, and it does not seem to affect other arch anyway.
>> So I am really suspecting a problem with riscv uclibcb or elf2flt, but can't
>> quite yet figure out what may be wrong there.
>
> I did not have time to find out if what we did in RISC-V nommu was
> correct or wrong. But to get my existing userspace to work I had to
> revert the revert. I'll try to find some time to dig into the binfmt
> flat issue and elf2flt soon.
OK. Thanks. I will keep digging too when I have time.
--
Damien Le Moal
Western Digital Research
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems
2020-09-14 16:56 [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems Palmer Dabbelt
` (3 preceding siblings ...)
2020-09-15 5:46 ` Christoph Hellwig
@ 2020-09-15 8:25 ` Damien Le Moal
2020-09-15 8:39 ` Damien Le Moal
2020-09-15 12:56 ` Damien Le Moal
5 siblings, 1 reply; 19+ messages in thread
From: Damien Le Moal @ 2020-09-15 8:25 UTC (permalink / raw)
To: linux-riscv, palmerdabbelt; +Cc: hch, Anup Patel, kernel-team
On Mon, 2020-09-14 at 09:56 -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.
>
> Signed-off-by: Palmer Dabbelt <palmerdabbelt@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).
> ---
> 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);
I tried this patch on the Kendryte, on top of for-next and on top of
fixes (not sure which one is your target). First of all, with for-next,
I get this compilation warning:
arch/riscv/mm/init.c:44:28: warning: ‘pt_ops’ defined but not used [-
Wunused-variable]
44 | static struct pt_alloc_ops pt_ops;
Now for the tests:
(1) With for-next, no patch applied I get a crash. Expected I guess...
[ 0.000000] Linux version 5.9.0-rc2-00022-g3a822d8ea3b3 (
damien@localhost.localdomain) (riscv64-linux-gcc.br_real (Buildroot
2020.08-368-g9148de6052-dirty) 9.3.0, GNU ld (GNU Binutils) 2.32) #93
SMP Tue Sep 15 17:13:25 JST 2020
[ 0.000000] earlycon: sifive0 at MMIO 0x0000000038000000 (options
'')
[ 0.000000] printk: bootconsole [sifive0] enabled
[ 0.000000] Zone ranges:
[ 0.000000] DMA32 [mem 0x0000000080000000-0x00000000807fffff]
[ 0.000000] Normal empty
[ 0.000000] Movable zone start for each node
[ 0.000000] Early memory node ranges
[ 0.000000] node 0: [mem 0x0000000080000000-0x00000000807fffff]
[ 0.000000] Initmem setup node 0 [mem 0x0000000080000000-
0x00000000807fffff]
[ 0.000000] riscv: ISA extensions acdfim
[ 0.000000] riscv: ELF capabilities acdfim
[ 0.000000] percpu: max_distance=0x18000 too large for vmalloc space
0x0
[ 0.000000] percpu: Embedded 12 pages/cpu s18400 r0 d30752 u49152
[ 0.000000] Built 1 zonelists, mobility grouping off. Total pages:
2020
[ 0.000000] Kernel command line: earlycon console=ttySIF0
[ 0.000000] Dentry cache hash table entries: 1024 (order: 1, 8192
bytes, linear)
[ 0.000000] Inode-cache hash table entries: 512 (order: 0, 4096
bytes, linear)
[ 0.000000] Sorting __ex_table...
[ 0.000000] mem auto-init: stack:off, heap alloc:off, heap free:off
[ 0.000000] Memory: 6192K/8192K available (1287K kernel code, 117K
rwdata, 178K rodata, 301K init, 97K bss, 2000K reserved, 0K cma-
reserved)
[ 0.000000] rcu: Hierarchical RCU implementation.
[ 0.000000] rcu: RCU calculated value of scheduler-enlistment delay
is 25 jiffies.
[ 0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0
[ 0.000000] riscv-intc: 64 local interrupts mapped
[ 0.000000] plic: interrupt-controller@c000000: mapped 65 interrupts
with 2 handlers for 4 contexts.
[ 0.000000] Oops - illegal instruction [#1]
[ 0.000000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.9.0-rc2-
00022-g3a822d8ea3b3 #93
[ 0.000000] epc: 000000008000d5c8 ra : 000000008000d4fa sp :
0000000080183f40
[ 0.000000] gp : 000000008019d350 tp : 0000000080187a00 t0 :
0000000000000019
[ 0.000000] t1 : 0000000000000018 t2 : 0000000000000001 s0 :
0000000080183f80
[ 0.000000] s1 : 0000000000000200 a0 : 0000000080196070 a1 :
0000000000000000
[ 0.000000] a2 : ffffffff80183f50 a3 : 0000000000000000 a4 :
0000000000000000
[ 0.000000] a5 : 0000000080168f70 a6 : 0000000000000000 a7 :
00000000000001e4
[ 0.000000] s2 : 000000008019d0a8 s3 : 000000008019d068 s4 :
00000000807e0b00
[ 0.000000] s5 : 000000008019d038 s6 : 000000008019d030 s7 :
00000000800123a8
[ 0.000000] s8 : 0000000000000000 s9 : 0000000000000000 s10:
0000000000000000
[ 0.000000] s11: 0000000000000000 t3 : 00000000801b42a8 t4 :
0000000000000068
[ 0.000000] t5 : 000000000000004c t6 : 0000000000000033
[ 0.000000] status: 0000000000001800 badaddr: 00000000001b727c
cause: 0000000000000002
[ 0.000000] random: get_random_bytes called from 0x0000000080051df8
with crng_init=0
[ 0.000000] ---[ end trace 0000000000000000 ]---
[ 0.000000] Kernel panic - not syncing: Fatal exception
[ 0.000000] ---[ end Kernel panic - not syncing: Fatal exception ]
---
(2) With for-next, patch applied, I get a hang:
[ 0.000000] Linux version 5.9.0-rc2-00023-gacd1158869ad (
damien@localhost.localdomain) (riscv64-linux-gcc.br_real (Buildroot
2020.08-368-g9148de6052-dirty) 9.3.0, GNU ld (GNU Binutils) 2.32) #94
SMP Tue Sep 15 17:15:59 JST 2020
[ 0.000000] earlycon: sifive0 at MMIO 0x0000000038000000 (options
'')
[ 0.000000] printk: bootconsole [sifive0] enabled
[ 0.000000] Zone ranges:
[ 0.000000] DMA32 [mem 0x0000000080000000-0x00000000807fffff]
[ 0.000000] Normal empty
[ 0.000000] Movable zone start for each node
[ 0.000000] Early memory node ranges
[ 0.000000] node 0: [mem 0x0000000080000000-0x00000000807fffff]
[ 0.000000] Initmem setup node 0 [mem 0x0000000080000000-
0x00000000807fffff]
[ 0.000000] riscv: ISA extensions acdfim
[ 0.000000] riscv: ELF capabilities acdfim
[ 0.000000] percpu: max_distance=0x18000 too large for vmalloc space
0x0
[ 0.000000] percpu: Embedded 12 pages/cpu s18400 r0 d30752 u49152
[ 0.000000] Built 1 zonelists, mobility grouping off. Total pages:
2020
[ 0.000000] Kernel command line: earlycon console=ttySIF0
[ 0.000000] Dentry cache hash table entries: 1024 (order: 1, 8192
bytes, linear)
[ 0.000000] Inode-cache hash table entries: 512 (order: 0, 4096
bytes, linear)
[ 0.000000] Sorting __ex_table...
[ 0.000000] mem auto-init: stack:off, heap alloc:off, heap free:off
[ 0.000000] Memory: 6176K/8192K available (1291K kernel code, 128K
rwdata, 178K rodata, 305K init, 97K bss, 2016K reserved, 0K cma-
reserved)
[ 0.000000] rcu: Hierarchical RCU implementation.
[ 0.000000] rcu: RCU calculated value of scheduler-enlistment delay
is 25 jiffies.
[ 0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0
[ 0.000000] riscv-intc: 64 local interrupts mapped
[ 0.000000] plic: interrupt-controller@c000000: mapped 65 interrupts
with 2 handlers for 4 contexts.
[ 0.000000] random: get_random_bytes called from 0x00000000800019b4
with crng_init=0
[ 0.000000] clint: interrupt-controller@2000000: timer irq not found
[ 0.000000] Failed to initialize '/soc/interrupt-controller@2000000'
: -19
[ 0.000000] timer_probe: no matching timers found
[ 0.000000] Console: colour dummy device 80x25
[ 0.000000] sched_clock: 64 bits at 250 Hz, resolution 4000000ns,
wraps every 9007199254000000ns
[ 0.000000] Calibrating delay loop (skipped), value calculated using
timer frequency.. 15.60 BogoMIPS (lpj=31200)
[ 0.000000] pid_max: default: 4096 minimum: 301
[ 0.000000] Mount-cache hash table entries: 512 (order: 0, 4096
bytes, linear)
[ 0.000000] Mountpoint-cache hash table entries: 512 (order: 0, 4096
bytes, linear)
[ 0.000000] rcu: Hierarchical SRCU implementation.
[ 0.000000] smp: Bringing up secondary CPUs ...
[ 0.000000] SMP: IPI inject method not available
In both cases, the timer does not seem to work at all (see the message
timestamps).
Exact same results with your fixes branch too: hang with the patch and
illegal instruction without it.
Something is broken.
--
Damien Le Moal
Western Digital Research
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems
2020-09-15 8:25 ` Damien Le Moal
@ 2020-09-15 8:39 ` Damien Le Moal
0 siblings, 0 replies; 19+ messages in thread
From: Damien Le Moal @ 2020-09-15 8:39 UTC (permalink / raw)
To: linux-riscv, palmerdabbelt; +Cc: hch, Anup Patel, kernel-team
On 2020/09/15 17:25, Damien Le Moal wrote:
> On Mon, 2020-09-14 at 09:56 -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.
>>
>> Signed-off-by: Palmer Dabbelt <palmerdabbelt@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).
>> ---
>> 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);
>
> I tried this patch on the Kendryte, on top of for-next and on top of
> fixes (not sure which one is your target). First of all, with for-next,
> I get this compilation warning:
>
> arch/riscv/mm/init.c:44:28: warning: ‘pt_ops’ defined but not used [-
> Wunused-variable]
> 44 | static struct pt_alloc_ops pt_ops;
>
> Now for the tests:
>
> (1) With for-next, no patch applied I get a crash. Expected I guess...
>
> [ 0.000000] Linux version 5.9.0-rc2-00022-g3a822d8ea3b3 (
> damien@localhost.localdomain) (riscv64-linux-gcc.br_real (Buildroot
> 2020.08-368-g9148de6052-dirty) 9.3.0, GNU ld (GNU Binutils) 2.32) #93
> SMP Tue Sep 15 17:13:25 JST 2020
> [ 0.000000] earlycon: sifive0 at MMIO 0x0000000038000000 (options
> '')
> [ 0.000000] printk: bootconsole [sifive0] enabled
> [ 0.000000] Zone ranges:
> [ 0.000000] DMA32 [mem 0x0000000080000000-0x00000000807fffff]
> [ 0.000000] Normal empty
> [ 0.000000] Movable zone start for each node
> [ 0.000000] Early memory node ranges
> [ 0.000000] node 0: [mem 0x0000000080000000-0x00000000807fffff]
> [ 0.000000] Initmem setup node 0 [mem 0x0000000080000000-
> 0x00000000807fffff]
> [ 0.000000] riscv: ISA extensions acdfim
> [ 0.000000] riscv: ELF capabilities acdfim
> [ 0.000000] percpu: max_distance=0x18000 too large for vmalloc space
> 0x0
> [ 0.000000] percpu: Embedded 12 pages/cpu s18400 r0 d30752 u49152
> [ 0.000000] Built 1 zonelists, mobility grouping off. Total pages:
> 2020
> [ 0.000000] Kernel command line: earlycon console=ttySIF0
> [ 0.000000] Dentry cache hash table entries: 1024 (order: 1, 8192
> bytes, linear)
> [ 0.000000] Inode-cache hash table entries: 512 (order: 0, 4096
> bytes, linear)
> [ 0.000000] Sorting __ex_table...
> [ 0.000000] mem auto-init: stack:off, heap alloc:off, heap free:off
> [ 0.000000] Memory: 6192K/8192K available (1287K kernel code, 117K
> rwdata, 178K rodata, 301K init, 97K bss, 2000K reserved, 0K cma-
> reserved)
> [ 0.000000] rcu: Hierarchical RCU implementation.
> [ 0.000000] rcu: RCU calculated value of scheduler-enlistment delay
> is 25 jiffies.
> [ 0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0
> [ 0.000000] riscv-intc: 64 local interrupts mapped
> [ 0.000000] plic: interrupt-controller@c000000: mapped 65 interrupts
> with 2 handlers for 4 contexts.
> [ 0.000000] Oops - illegal instruction [#1]
> [ 0.000000] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.9.0-rc2-
> 00022-g3a822d8ea3b3 #93
> [ 0.000000] epc: 000000008000d5c8 ra : 000000008000d4fa sp :
> 0000000080183f40
> [ 0.000000] gp : 000000008019d350 tp : 0000000080187a00 t0 :
> 0000000000000019
> [ 0.000000] t1 : 0000000000000018 t2 : 0000000000000001 s0 :
> 0000000080183f80
> [ 0.000000] s1 : 0000000000000200 a0 : 0000000080196070 a1 :
> 0000000000000000
> [ 0.000000] a2 : ffffffff80183f50 a3 : 0000000000000000 a4 :
> 0000000000000000
> [ 0.000000] a5 : 0000000080168f70 a6 : 0000000000000000 a7 :
> 00000000000001e4
> [ 0.000000] s2 : 000000008019d0a8 s3 : 000000008019d068 s4 :
> 00000000807e0b00
> [ 0.000000] s5 : 000000008019d038 s6 : 000000008019d030 s7 :
> 00000000800123a8
> [ 0.000000] s8 : 0000000000000000 s9 : 0000000000000000 s10:
> 0000000000000000
> [ 0.000000] s11: 0000000000000000 t3 : 00000000801b42a8 t4 :
> 0000000000000068
> [ 0.000000] t5 : 000000000000004c t6 : 0000000000000033
> [ 0.000000] status: 0000000000001800 badaddr: 00000000001b727c
> cause: 0000000000000002
> [ 0.000000] random: get_random_bytes called from 0x0000000080051df8
> with crng_init=0
> [ 0.000000] ---[ end trace 0000000000000000 ]---
> [ 0.000000] Kernel panic - not syncing: Fatal exception
> [ 0.000000] ---[ end Kernel panic - not syncing: Fatal exception ]
> ---
>
>
> (2) With for-next, patch applied, I get a hang:
>
> [ 0.000000] Linux version 5.9.0-rc2-00023-gacd1158869ad (
> damien@localhost.localdomain) (riscv64-linux-gcc.br_real (Buildroot
> 2020.08-368-g9148de6052-dirty) 9.3.0, GNU ld (GNU Binutils) 2.32) #94
> SMP Tue Sep 15 17:15:59 JST 2020
> [ 0.000000] earlycon: sifive0 at MMIO 0x0000000038000000 (options
> '')
> [ 0.000000] printk: bootconsole [sifive0] enabled
> [ 0.000000] Zone ranges:
> [ 0.000000] DMA32 [mem 0x0000000080000000-0x00000000807fffff]
> [ 0.000000] Normal empty
> [ 0.000000] Movable zone start for each node
> [ 0.000000] Early memory node ranges
> [ 0.000000] node 0: [mem 0x0000000080000000-0x00000000807fffff]
> [ 0.000000] Initmem setup node 0 [mem 0x0000000080000000-
> 0x00000000807fffff]
> [ 0.000000] riscv: ISA extensions acdfim
> [ 0.000000] riscv: ELF capabilities acdfim
> [ 0.000000] percpu: max_distance=0x18000 too large for vmalloc space
> 0x0
> [ 0.000000] percpu: Embedded 12 pages/cpu s18400 r0 d30752 u49152
> [ 0.000000] Built 1 zonelists, mobility grouping off. Total pages:
> 2020
> [ 0.000000] Kernel command line: earlycon console=ttySIF0
> [ 0.000000] Dentry cache hash table entries: 1024 (order: 1, 8192
> bytes, linear)
> [ 0.000000] Inode-cache hash table entries: 512 (order: 0, 4096
> bytes, linear)
> [ 0.000000] Sorting __ex_table...
> [ 0.000000] mem auto-init: stack:off, heap alloc:off, heap free:off
> [ 0.000000] Memory: 6176K/8192K available (1291K kernel code, 128K
> rwdata, 178K rodata, 305K init, 97K bss, 2016K reserved, 0K cma-
> reserved)
> [ 0.000000] rcu: Hierarchical RCU implementation.
> [ 0.000000] rcu: RCU calculated value of scheduler-enlistment delay
> is 25 jiffies.
> [ 0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0
> [ 0.000000] riscv-intc: 64 local interrupts mapped
> [ 0.000000] plic: interrupt-controller@c000000: mapped 65 interrupts
> with 2 handlers for 4 contexts.
> [ 0.000000] random: get_random_bytes called from 0x00000000800019b4
> with crng_init=0
> [ 0.000000] clint: interrupt-controller@2000000: timer irq not found
> [ 0.000000] Failed to initialize '/soc/interrupt-controller@2000000'
> : -19
> [ 0.000000] timer_probe: no matching timers found
Is it needed to fix the device tree ? That was working before.
> [ 0.000000] Console: colour dummy device 80x25
> [ 0.000000] sched_clock: 64 bits at 250 Hz, resolution 4000000ns,
> wraps every 9007199254000000ns
> [ 0.000000] Calibrating delay loop (skipped), value calculated using
> timer frequency.. 15.60 BogoMIPS (lpj=31200)
> [ 0.000000] pid_max: default: 4096 minimum: 301
> [ 0.000000] Mount-cache hash table entries: 512 (order: 0, 4096
> bytes, linear)
> [ 0.000000] Mountpoint-cache hash table entries: 512 (order: 0, 4096
> bytes, linear)
> [ 0.000000] rcu: Hierarchical SRCU implementation.
> [ 0.000000] smp: Bringing up secondary CPUs ...
> [ 0.000000] SMP: IPI inject method not available
>
>
> In both cases, the timer does not seem to work at all (see the message
> timestamps).
>
> Exact same results with your fixes branch too: hang with the patch and
> illegal instruction without it.
>
> Something is broken.
Note: just to be sure I was not doing something wrong, I use the same build
environment to check with a known good kernel (5.8.0) and I got a clean boot up
to user space. Here is the boot log:
[ 0.000000] Linux version 5.8.0 (damien@localhost.localdomain)
(riscv64-linux-gcc.br_real (Buildroot 2020.08-368-g9148de6052-dirty) 9.3.0, GNU
ld (GNU Binutils) 2.32) #97 SMP Tue Sep 15 17:30:04 JST 2020
[ 0.000000] earlycon: sifive0 at MMIO 0x0000000038000000 (options '')
[ 0.000000] printk: bootconsole [sifive0] enabled
[ 0.000000] Zone ranges:
[ 0.000000] DMA32 [mem 0x0000000080000000-0x00000000807fffff]
[ 0.000000] Normal empty
[ 0.000000] Movable zone start for each node
[ 0.000000] Early memory node ranges
[ 0.000000] node 0: [mem 0x0000000080000000-0x00000000807fffff]
[ 0.000000] Initmem setup node 0 [mem 0x0000000080000000-0x00000000807fffff]
[ 0.000000] riscv: ISA extensions acdfim
[ 0.000000] riscv: ELF capabilities acdfim
[ 0.000000] percpu: max_distance=0x18000 too large for vmalloc space 0x0
[ 0.000000] percpu: Embedded 12 pages/cpu s18336 r0 d30816 u49152
[ 0.000000] Built 1 zonelists, mobility grouping off. Total pages: 2020
[ 0.000000] Kernel command line: earlycon console=ttySIF0
[ 0.000000] Dentry cache hash table entries: 1024 (order: 1, 8192 bytes, linear)
[ 0.000000] Inode-cache hash table entries: 512 (order: 0, 4096 bytes, linear)
[ 0.000000] Sorting __ex_table...
[ 0.000000] mem auto-init: stack:off, heap alloc:off, heap free:off
[ 0.000000] Memory: 6312K/8192K available (939K kernel code, 127K rwdata,
169K rodata, 297K init, 97K bss, 1880K reserved, 0K cma-reserved)
[ 0.000000] rcu: Hierarchical RCU implementation.
[ 0.000000] rcu: RCU calculated value of scheduler-enlistment delay is 25
jiffies.
[ 0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0
[ 0.000000] riscv-intc: 64 local interrupts mapped
[ 0.000000] plic: interrupt-controller@c000000: mapped 65 interrupts with 2
handlers for 4 contexts.
[ 0.000000] riscv_timer_init_dt: Registering clocksource cpuid [0] hartid [0]
[ 0.000000] clocksource: riscv_clocksource: mask: 0xffffffffffffffff
max_cycles: 0x3990be68b, max_idle_ns: 881590404272 ns
[ 0.000014] sched_clock: 64 bits at 7MHz, resolution 128ns, wraps every
4398046511054ns
[ 0.008239] Console: colour dummy device 80x25
[ 0.012481] Calibrating delay loop (skipped), value calculated using timer
frequency.. 15.60 BogoMIPS (lpj=31200)
[ 0.022682] pid_max: default: 4096 minimum: 301
[ 0.027299] Mount-cache hash table entries: 512 (order: 0, 4096 bytes, linear)
[ 0.034417] Mountpoint-cache hash table entries: 512 (order: 0, 4096 bytes,
linear)
[ 0.044873] rcu: Hierarchical SRCU implementation.
[ 0.049771] smp: Bringing up secondary CPUs ...
[ 0.055055] smp: Brought up 1 node, 2 CPUs
[ 0.059445] devtmpfs: initialized
[ 0.065609] clocksource: jiffies: mask: 0xffffffff max_cycles: 0xffffffff,
max_idle_ns: 7645041785100000 ns
[ 0.074633] futex hash table entries: 16 (order: -2, 1024 bytes, linear)
[ 0.084141] Kendryte K210 SoC sysctl
[ 0.097636] clocksource: Switched to clocksource riscv_clocksource
[ 0.235346] workingset: timestamp_bits=62 max_order=11 bucket_order=0
[ 0.242991] 38000000.serial: ttySIF0 at MMIO 0x38000000 (irq = 1, base_baud =
0) is a SiFive UART v0
[ 0.251551] printk: console [ttySIF0] enabled
[ 0.251551] printk: console [ttySIF0] enabled
[ 0.260079] printk: bootconsole [sifive0] disabled
[ 0.260079] printk: bootconsole [sifive0] disabled
[ 0.271932] random: get_random_bytes called from 0x000000008005075c with
crng_init=0
[ 0.280470] Freeing unused kernel memory: 296K
[ 0.284186] This architecture does not have kernel memory protection.
[ 0.290631] Run /init as init process
+---------------------------+
| Kendryte K210 NOMMU Linux |
+---------------------------+
BusyBox v1.32.0 (2020-09-12 16:01:20 JST) hush - the humble shell
Enter 'help' for a list of built-in commands.
/ #
--
Damien Le Moal
Western Digital Research
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems
2020-09-14 16:56 [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems Palmer Dabbelt
` (4 preceding siblings ...)
2020-09-15 8:25 ` Damien Le Moal
@ 2020-09-15 12:56 ` Damien Le Moal
5 siblings, 0 replies; 19+ messages in thread
From: Damien Le Moal @ 2020-09-15 12:56 UTC (permalink / raw)
To: linux-riscv, palmerdabbelt; +Cc: hch, Anup Patel, kernel-team
On Mon, 2020-09-14 at 09:56 -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.
>
> Signed-off-by: Palmer Dabbelt <palmerdabbelt@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).
> ---
> 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);
With the Kendryte device tree patch that I sent, this works.
Tested-by: Damien Le Moal <damien.lemoal@wdc.com>
--
Damien Le Moal
Western Digital Research
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems
2020-09-15 2:11 ` Anup Patel
@ 2020-09-19 2:54 ` Palmer Dabbelt
2020-09-19 3:32 ` Anup Patel
0 siblings, 1 reply; 19+ messages in thread
From: Palmer Dabbelt @ 2020-09-19 2:54 UTC (permalink / raw)
To: Anup Patel; +Cc: Christoph Hellwig, anup, linux-riscv, kernel-team
On Mon, 14 Sep 2020 19:11:46 PDT (-0700), Anup Patel wrote:
>
>
>> -----Original Message-----
>> From: Palmer Dabbelt <palmerdabbelt@google.com>
>> Sent: 14 September 2020 22:27
>> To: linux-riscv@lists.infradead.org
>> Cc: hch@infradead.org; Anup Patel <Anup.Patel@wdc.com>; Palmer Dabbelt
>> <palmerdabbelt@google.com>; kernel-team@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@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.
>> +#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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems
2020-09-19 2:54 ` Palmer Dabbelt
@ 2020-09-19 3:32 ` Anup Patel
2020-09-19 4:36 ` Palmer Dabbelt
0 siblings, 1 reply; 19+ messages in thread
From: Anup Patel @ 2020-09-19 3:32 UTC (permalink / raw)
To: Palmer Dabbelt; +Cc: hch, anup, linux-riscv, kernel-team
> -----Original Message-----
> From: Palmer Dabbelt <palmerdabbelt@google.com>
> Sent: 19 September 2020 08:24
> To: Anup Patel <Anup.Patel@wdc.com>
> Cc: linux-riscv@lists.infradead.org; hch@infradead.org; kernel-
> team@android.com; anup@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@google.com>
> >> Sent: 14 September 2020 22:27
> >> To: linux-riscv@lists.infradead.org
> >> Cc: hch@infradead.org; Anup Patel <Anup.Patel@wdc.com>; Palmer
> >> Dabbelt <palmerdabbelt@google.com>; kernel-team@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@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".
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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems
2020-09-19 3:32 ` Anup Patel
@ 2020-09-19 4:36 ` Palmer Dabbelt
2020-09-19 4:54 ` Anup Patel
0 siblings, 1 reply; 19+ messages in thread
From: Palmer Dabbelt @ 2020-09-19 4:36 UTC (permalink / raw)
To: Anup Patel; +Cc: Christoph Hellwig, anup, linux-riscv, kernel-team
On Fri, 18 Sep 2020 20:32:12 PDT (-0700), Anup Patel wrote:
>
>
>> -----Original Message-----
>> From: Palmer Dabbelt <palmerdabbelt@google.com>
>> Sent: 19 September 2020 08:24
>> To: Anup Patel <Anup.Patel@wdc.com>
>> Cc: linux-riscv@lists.infradead.org; hch@infradead.org; kernel-
>> team@android.com; anup@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@google.com>
>> >> Sent: 14 September 2020 22:27
>> >> To: linux-riscv@lists.infradead.org
>> >> Cc: hch@infradead.org; Anup Patel <Anup.Patel@wdc.com>; Palmer
>> >> Dabbelt <palmerdabbelt@google.com>; kernel-team@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@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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 19+ messages in thread
* RE: [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems
2020-09-19 4:36 ` Palmer Dabbelt
@ 2020-09-19 4:54 ` Anup Patel
0 siblings, 0 replies; 19+ messages in thread
From: Anup Patel @ 2020-09-19 4:54 UTC (permalink / raw)
To: Palmer Dabbelt; +Cc: hch, anup, linux-riscv, kernel-team
> -----Original Message-----
> From: Palmer Dabbelt <palmerdabbelt@google.com>
> Sent: 19 September 2020 10:07
> To: Anup Patel <Anup.Patel@wdc.com>
> Cc: linux-riscv@lists.infradead.org; hch@infradead.org; kernel-
> team@android.com; anup@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@google.com>
> >> Sent: 19 September 2020 08:24
> >> To: Anup Patel <Anup.Patel@wdc.com>
> >> Cc: linux-riscv@lists.infradead.org; hch@infradead.org; kernel-
> >> team@android.com; anup@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@google.com>
> >> >> Sent: 14 September 2020 22:27
> >> >> To: linux-riscv@lists.infradead.org
> >> >> Cc: hch@infradead.org; Anup Patel <Anup.Patel@wdc.com>; Palmer
> >> >> Dabbelt <palmerdabbelt@google.com>; kernel-team@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@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
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems
2020-09-15 7:01 ` Damien Le Moal
@ 2020-10-27 4:12 ` Atish Patra
2020-10-27 4:25 ` Damien Le Moal
0 siblings, 1 reply; 19+ messages in thread
From: Atish Patra @ 2020-10-27 4:12 UTC (permalink / raw)
To: Damien Le Moal, Alistair Francis
Cc: hch, linux-riscv, Anup Patel, kernel-team, Palmer Dabbelt
On Tue, Sep 15, 2020 at 12:02 AM Damien Le Moal <Damien.LeMoal@wdc.com> wrote:
>
> On 2020/09/15 15:51, hch@infradead.org wrote:
> > On Tue, Sep 15, 2020 at 06:50:24AM +0000, Damien Le Moal wrote:
> >> On 2020/09/15 14:47, Christoph Hellwig wrote:
> >>> I won't have time to set up the K210, but this fixes the timer issues
> >>> on older Qemu versions that don't implement the time CSR. Although I
> >>> also need to revert the binfmt_flat patch to actually get to userspace.
> >>
> >> So revert is needed ? Digging in the flatbin format, I do not see why the offset
> >> you removed would be needed, and it does not seem to affect other arch anyway.
> >> So I am really suspecting a problem with riscv uclibcb or elf2flt, but can't
> >> quite yet figure out what may be wrong there.
> >
> > I did not have time to find out if what we did in RISC-V nommu was
> > correct or wrong. But to get my existing userspace to work I had to
> > revert the revert. I'll try to find some time to dig into the binfmt
> > flat issue and elf2flt soon.
>
> OK. Thanks. I will keep digging too when I have time.
>
Is there a solution to this ? As Christoph pointed out, we need to
revert the revert to get into user space for nommu kernel (v5.9 or
v5.10-rc1).
This commit was introduced between 5.9-rc2 & 5.9-rc3.
The concerned commit here is:
2217b982624680d19a80ebb4600d05c8586c4f96: binfmt_flat: revert
"binfmt_flat: don't offset the data start"
>
>
> --
> Damien Le Moal
> Western Digital Research
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
--
Regards,
Atish
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems
2020-10-27 4:12 ` Atish Patra
@ 2020-10-27 4:25 ` Damien Le Moal
0 siblings, 0 replies; 19+ messages in thread
From: Damien Le Moal @ 2020-10-27 4:25 UTC (permalink / raw)
To: Atish Patra, Alistair Francis
Cc: hch, linux-riscv, Anup Patel, kernel-team, Palmer Dabbelt
On 2020/10/27 13:12, Atish Patra wrote:
> On Tue, Sep 15, 2020 at 12:02 AM Damien Le Moal <Damien.LeMoal@wdc.com> wrote:
>>
>> On 2020/09/15 15:51, hch@infradead.org wrote:
>>> On Tue, Sep 15, 2020 at 06:50:24AM +0000, Damien Le Moal wrote:
>>>> On 2020/09/15 14:47, Christoph Hellwig wrote:
>>>>> I won't have time to set up the K210, but this fixes the timer issues
>>>>> on older Qemu versions that don't implement the time CSR. Although I
>>>>> also need to revert the binfmt_flat patch to actually get to userspace.
>>>>
>>>> So revert is needed ? Digging in the flatbin format, I do not see why the offset
>>>> you removed would be needed, and it does not seem to affect other arch anyway.
>>>> So I am really suspecting a problem with riscv uclibcb or elf2flt, but can't
>>>> quite yet figure out what may be wrong there.
>>>
>>> I did not have time to find out if what we did in RISC-V nommu was
>>> correct or wrong. But to get my existing userspace to work I had to
>>> revert the revert. I'll try to find some time to dig into the binfmt
>>> flat issue and elf2flt soon.
>>
>> OK. Thanks. I will keep digging too when I have time.
>>
>
> Is there a solution to this ? As Christoph pointed out, we need to
> revert the revert to get into user space for nommu kernel (v5.9 or
> v5.10-rc1).
> This commit was introduced between 5.9-rc2 & 5.9-rc3.
And it was back-ported to 5.8.5 and above too.
>
> The concerned commit here is:
> 2217b982624680d19a80ebb4600d05c8586c4f96: binfmt_flat: revert
> "binfmt_flat: don't offset the data start"
Nope, I do not have any solution yet. Inspecting in more details the riscv
elf2flt code and uclibc is needed I think to understand what is going on. Not
clear at this moment what the root cause of the problem is. I have been busy
finishing the Kendryte board support (SD card, I2C, GPIO, clocks, resets,
pinctrl). Now that this is mostly done, fixing this flatbin user space problem
is mandatory. Help welcome on this.
--
Damien Le Moal
Western Digital Research
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2020-10-27 4:26 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-14 16:56 [PATCH] RISC-V: Resurrect the MMIO timer implementation for M-mode systems Palmer Dabbelt
2020-09-14 17:22 ` Mohammed Billoo
2020-09-15 3:06 ` Palmer Dabbelt
2020-09-15 5:10 ` Damien Le Moal
2020-09-15 2:11 ` Anup Patel
2020-09-19 2:54 ` Palmer Dabbelt
2020-09-19 3:32 ` Anup Patel
2020-09-19 4:36 ` Palmer Dabbelt
2020-09-19 4:54 ` Anup Patel
2020-09-15 5:08 ` Damien Le Moal
2020-09-15 5:46 ` Christoph Hellwig
2020-09-15 6:50 ` Damien Le Moal
2020-09-15 6:51 ` hch
2020-09-15 7:01 ` Damien Le Moal
2020-10-27 4:12 ` Atish Patra
2020-10-27 4:25 ` Damien Le Moal
2020-09-15 8:25 ` Damien Le Moal
2020-09-15 8:39 ` Damien Le Moal
2020-09-15 12:56 ` Damien Le Moal
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.