| Message ID | 20240325164021.3229-3-jszhang@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | riscv: improve nommu and timer-clint | expand |
Hi Jisheng, On 2024-03-25 11:40 AM, Jisheng Zhang wrote: > Per riscv privileged spec, "The time CSR is a read-only shadow of the > memory-mapped mtime register", "On RV32I the timeh CSR is a read-only > shadow of the upper 32 bits of the memory-mapped mtime register, while > time shadows only the lower 32 bits of mtime." Since get_cycles() only > reads the timer, it's fine to use CSR_TIME to implement get_cycles(). Unfortunately there are various implementations (e.g. FU740/Unmatched, probably K210 which this code was originally used for) which do not implement the time CSR, relying on M-mode software to emulate the CSR so S-mode software doesn't notice. So this code is needed to support those platforms when running Linux in M-mode. Maybe there should be an option to assume the time CSR is/is not implemented, like there is for misaligned access? Regards, Samuel > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > --- > arch/riscv/include/asm/timex.h | 40 ---------------------------------- > 1 file changed, 40 deletions(-) > > diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h > index a06697846e69..a3fb85d505d4 100644 > --- a/arch/riscv/include/asm/timex.h > +++ b/arch/riscv/include/asm/timex.h > @@ -10,44 +10,6 @@ > > 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 */ > - > -/* > - * Much like MIPS, we may not have a viable counter to use at an early point > - * in the boot process. Unfortunately we don't have a fallback, so instead > - * we just return 0. > - */ > -static inline unsigned long random_get_entropy(void) > -{ > - if (unlikely(clint_time_val == NULL)) > - return random_get_entropy_fallback(); > - return get_cycles(); > -} > -#define random_get_entropy() random_get_entropy() > - > -#else /* CONFIG_RISCV_M_MODE */ > - > static inline cycles_t get_cycles(void) > { > return csr_read(CSR_TIME); > @@ -60,8 +22,6 @@ static inline u32 get_cycles_hi(void) > } > #define get_cycles_hi get_cycles_hi > > -#endif /* !CONFIG_RISCV_M_MODE */ > - > #ifdef CONFIG_64BIT > static inline u64 get_cycles64(void) > {
On Mon, Mar 25, 2024 at 09:39:26PM -0500, Samuel Holland wrote: > Hi Jisheng, > > On 2024-03-25 11:40 AM, Jisheng Zhang wrote: > > Per riscv privileged spec, "The time CSR is a read-only shadow of the > > memory-mapped mtime register", "On RV32I the timeh CSR is a read-only > > shadow of the upper 32 bits of the memory-mapped mtime register, while > > time shadows only the lower 32 bits of mtime." Since get_cycles() only > > reads the timer, it's fine to use CSR_TIME to implement get_cycles(). > > Unfortunately there are various implementations (e.g. FU740/Unmatched, probably > K210 which this code was originally used for) which do not implement the time > CSR, relying on M-mode software to emulate the CSR so S-mode software doesn't > notice. So this code is needed to support those platforms when running Linux in > M-mode. OOPS, I knew this for the first time there are such implementations which doesn't implement the TIME CSR :( > > Maybe there should be an option to assume the time CSR is/is not implemented, > like there is for misaligned access? Yep, this seems the only solution. Then which should be the default choice? I.E Assume all NOMMU goes through TIME CSR, and provide an option for platform lacking of TIME CSR. This prefers TIME CSR. VS. By default, MTIME is used, and provide one Kconfig option for TIME CSR usage. This prefers MTIME which choice is better? Any suggestion? Thanks in advance > > Regards, > Samuel > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > > --- > > arch/riscv/include/asm/timex.h | 40 ---------------------------------- > > 1 file changed, 40 deletions(-) > > > > diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h > > index a06697846e69..a3fb85d505d4 100644 > > --- a/arch/riscv/include/asm/timex.h > > +++ b/arch/riscv/include/asm/timex.h > > @@ -10,44 +10,6 @@ > > > > 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 */ > > - > > -/* > > - * Much like MIPS, we may not have a viable counter to use at an early point > > - * in the boot process. Unfortunately we don't have a fallback, so instead > > - * we just return 0. > > - */ > > -static inline unsigned long random_get_entropy(void) > > -{ > > - if (unlikely(clint_time_val == NULL)) > > - return random_get_entropy_fallback(); > > - return get_cycles(); > > -} > > -#define random_get_entropy() random_get_entropy() > > - > > -#else /* CONFIG_RISCV_M_MODE */ > > - > > static inline cycles_t get_cycles(void) > > { > > return csr_read(CSR_TIME); > > @@ -60,8 +22,6 @@ static inline u32 get_cycles_hi(void) > > } > > #define get_cycles_hi get_cycles_hi > > > > -#endif /* !CONFIG_RISCV_M_MODE */ > > - > > #ifdef CONFIG_64BIT > > static inline u64 get_cycles64(void) > > { >
Hi Jisheng, On Wed, Mar 27, 2024 at 12:43 AM Jisheng Zhang <jszhang@kernel.org> wrote: > > On Mon, Mar 25, 2024 at 09:39:26PM -0500, Samuel Holland wrote: > > Hi Jisheng, > > > > On 2024-03-25 11:40 AM, Jisheng Zhang wrote: > > > Per riscv privileged spec, "The time CSR is a read-only shadow of the > > > memory-mapped mtime register", "On RV32I the timeh CSR is a read-only > > > shadow of the upper 32 bits of the memory-mapped mtime register, while > > > time shadows only the lower 32 bits of mtime." Since get_cycles() only > > > reads the timer, it's fine to use CSR_TIME to implement get_cycles(). > > > > Unfortunately there are various implementations (e.g. FU740/Unmatched, probably > > K210 which this code was originally used for) which do not implement the time > > CSR, relying on M-mode software to emulate the CSR so S-mode software doesn't > > notice. So this code is needed to support those platforms when running Linux in > > M-mode. > > OOPS, I knew this for the first time there are such implementations > which doesn't implement the TIME CSR :( > > > > > Maybe there should be an option to assume the time CSR is/is not implemented, > > like there is for misaligned access? > > Yep, this seems the only solution. Then which should be the default > choice? I.E > > Assume all NOMMU goes through TIME CSR, and provide an option for > platform lacking of TIME CSR. This prefers TIME CSR. > > VS. > > By default, MTIME is used, and provide one Kconfig option for TIME CSR > usage. This prefers MTIME > > which choice is better? Any suggestion? > > Thanks in advance > > > > > Regards, > > Samuel > > > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > > > --- > > > arch/riscv/include/asm/timex.h | 40 ---------------------------------- > > > 1 file changed, 40 deletions(-) > > > > > > diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h > > > index a06697846e69..a3fb85d505d4 100644 > > > --- a/arch/riscv/include/asm/timex.h > > > +++ b/arch/riscv/include/asm/timex.h > > > @@ -10,44 +10,6 @@ > > > > > > 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 */ > > > - > > > -/* > > > - * Much like MIPS, we may not have a viable counter to use at an early point > > > - * in the boot process. Unfortunately we don't have a fallback, so instead > > > - * we just return 0. > > > - */ > > > -static inline unsigned long random_get_entropy(void) > > > -{ > > > - if (unlikely(clint_time_val == NULL)) > > > - return random_get_entropy_fallback(); > > > - return get_cycles(); > > > -} > > > -#define random_get_entropy() random_get_entropy() > > > - > > > -#else /* CONFIG_RISCV_M_MODE */ > > > - > > > static inline cycles_t get_cycles(void) > > > { > > > return csr_read(CSR_TIME); > > > @@ -60,8 +22,6 @@ static inline u32 get_cycles_hi(void) > > > } > > > #define get_cycles_hi get_cycles_hi > > > > > > -#endif /* !CONFIG_RISCV_M_MODE */ > > > - > > > #ifdef CONFIG_64BIT > > > static inline u64 get_cycles64(void) > > > { > > > I'd like to take this thread to ask: csr_read(CSR_TIME) returns a timestamp, right? So why is the function name get_cycles()? Instead of get_timestamp()? Thanks, Yunhui
On Wed, Mar 27, 2024 at 12:29:13AM +0800, Jisheng Zhang wrote: > On Mon, Mar 25, 2024 at 09:39:26PM -0500, Samuel Holland wrote: > > Hi Jisheng, > > > > On 2024-03-25 11:40 AM, Jisheng Zhang wrote: > > > Per riscv privileged spec, "The time CSR is a read-only shadow of the > > > memory-mapped mtime register", "On RV32I the timeh CSR is a read-only > > > shadow of the upper 32 bits of the memory-mapped mtime register, while > > > time shadows only the lower 32 bits of mtime." Since get_cycles() only > > > reads the timer, it's fine to use CSR_TIME to implement get_cycles(). > > > > Unfortunately there are various implementations (e.g. FU740/Unmatched, probably > > K210 which this code was originally used for) which do not implement the time > > CSR, relying on M-mode software to emulate the CSR so S-mode software doesn't > > notice. So this code is needed to support those platforms when running Linux in > > M-mode. > > OOPS, I knew this for the first time there are such implementations > which doesn't implement the TIME CSR :( > > > > > Maybe there should be an option to assume the time CSR is/is not implemented, > > like there is for misaligned access? > > Yep, this seems the only solution. Then which should be the default > choice? I.E > > Assume all NOMMU goes through TIME CSR, and provide an option for > platform lacking of TIME CSR. This prefers TIME CSR. Hi all, v2 will prefer TIME CSR. Thanks > > VS. > > By default, MTIME is used, and provide one Kconfig option for TIME CSR > usage. This prefers MTIME > > which choice is better? Any suggestion? > > Thanks in advance > > > > > Regards, > > Samuel > > > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org> > > > --- > > > arch/riscv/include/asm/timex.h | 40 ---------------------------------- > > > 1 file changed, 40 deletions(-) > > > > > > diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h > > > index a06697846e69..a3fb85d505d4 100644 > > > --- a/arch/riscv/include/asm/timex.h > > > +++ b/arch/riscv/include/asm/timex.h > > > @@ -10,44 +10,6 @@ > > > > > > 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 */ > > > - > > > -/* > > > - * Much like MIPS, we may not have a viable counter to use at an early point > > > - * in the boot process. Unfortunately we don't have a fallback, so instead > > > - * we just return 0. > > > - */ > > > -static inline unsigned long random_get_entropy(void) > > > -{ > > > - if (unlikely(clint_time_val == NULL)) > > > - return random_get_entropy_fallback(); > > > - return get_cycles(); > > > -} > > > -#define random_get_entropy() random_get_entropy() > > > - > > > -#else /* CONFIG_RISCV_M_MODE */ > > > - > > > static inline cycles_t get_cycles(void) > > > { > > > return csr_read(CSR_TIME); > > > @@ -60,8 +22,6 @@ static inline u32 get_cycles_hi(void) > > > } > > > #define get_cycles_hi get_cycles_hi > > > > > > -#endif /* !CONFIG_RISCV_M_MODE */ > > > - > > > #ifdef CONFIG_64BIT > > > static inline u64 get_cycles64(void) > > > { > >
diff --git a/arch/riscv/include/asm/timex.h b/arch/riscv/include/asm/timex.h index a06697846e69..a3fb85d505d4 100644 --- a/arch/riscv/include/asm/timex.h +++ b/arch/riscv/include/asm/timex.h @@ -10,44 +10,6 @@ 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 */ - -/* - * Much like MIPS, we may not have a viable counter to use at an early point - * in the boot process. Unfortunately we don't have a fallback, so instead - * we just return 0. - */ -static inline unsigned long random_get_entropy(void) -{ - if (unlikely(clint_time_val == NULL)) - return random_get_entropy_fallback(); - return get_cycles(); -} -#define random_get_entropy() random_get_entropy() - -#else /* CONFIG_RISCV_M_MODE */ - static inline cycles_t get_cycles(void) { return csr_read(CSR_TIME); @@ -60,8 +22,6 @@ static inline u32 get_cycles_hi(void) } #define get_cycles_hi get_cycles_hi -#endif /* !CONFIG_RISCV_M_MODE */ - #ifdef CONFIG_64BIT static inline u64 get_cycles64(void) {
Per riscv privileged spec, "The time CSR is a read-only shadow of the memory-mapped mtime register", "On RV32I the timeh CSR is a read-only shadow of the upper 32 bits of the memory-mapped mtime register, while time shadows only the lower 32 bits of mtime." Since get_cycles() only reads the timer, it's fine to use CSR_TIME to implement get_cycles(). Signed-off-by: Jisheng Zhang <jszhang@kernel.org> --- arch/riscv/include/asm/timex.h | 40 ---------------------------------- 1 file changed, 40 deletions(-)