[PATCH 0/5] riscv: improve nommu and timer-clint

[PATCH 0/5] riscv: improve nommu and timer-clint

[Jisheng Zhang]

As is known, the sophgo CV1800B contains so called little core, which
is C906 w/o MMU, so I want to run nommu linux on it. This series is
the result of the bring up. After this series, w/ proper dts, we can
run nommu linux on milkv duo's little core.

First of all, patch1 removes the PAGE_OFFSET hardcoding by introducing
DRAM_BASE Kconfig option.

The following patches try to improve the get_cycles and timer-clint
by always using TIME CSR, because Per the 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.".

The last patch adds T-Head C9xxx clint support to timer-clint driver.

Jisheng Zhang (5):
  riscv: nommu: remove PAGE_OFFSET hardcoding
  riscv: nommu: use CSR_TIME* for get_cycles* implementation
  clocksource/drivers/timer-clint: Remove clint_time_val
  clocksource/drivers/timer-clint: Use get_cycles()
  clocksource/drivers/timer-clint: Add T-Head C9xx clint support

 arch/riscv/Kconfig                |  8 +++-
 arch/riscv/include/asm/clint.h    | 26 ------------
 arch/riscv/include/asm/timex.h    | 40 -------------------
 drivers/clocksource/timer-clint.c | 66 ++++++++++++-------------------
 4 files changed, 32 insertions(+), 108 deletions(-)
 delete mode 100644 arch/riscv/include/asm/clint.h

-- 
2.43.0

[PATCH 1/5] riscv: nommu: remove PAGE_OFFSET hardcoding

[Jisheng Zhang]

Currently, PAGE_OFFSET is hardcoded as 0x8000_0000, it works fine since
there's only one nommu platform in the mainline. However, there are
many cases where the (S)DRAM base address isn't 0x8000_0000, so remove
the hardcoding value, and introduce DRAM_BASE which will be set by
users during configuring. DRAM_BASE is 0x8000_0000 by default.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 arch/riscv/Kconfig | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 7895c77545f1..afd51dbdc253 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -247,10 +247,16 @@ config MMU
 	  Select if you want MMU-based virtualised addressing space
 	  support by paged memory management. If unsure, say 'Y'.
 
+if !MMU
+config DRAM_BASE
+	hex '(S)DRAM Base Address'
+	default 0x80000000
+endif
+
 config PAGE_OFFSET
 	hex
 	default 0xC0000000 if 32BIT && MMU
-	default 0x80000000 if !MMU
+	default DRAM_BASE if !MMU
 	default 0xff60000000000000 if 64BIT
 
 config KASAN_SHADOW_OFFSET
-- 
2.43.0

Re: [PATCH 1/5] riscv: nommu: remove PAGE_OFFSET hardcoding

[Bo Gan]

On 3/25/24 9:40 AM, Jisheng Zhang wrote:
> Currently, PAGE_OFFSET is hardcoded as 0x8000_0000, it works fine since
> there's only one nommu platform in the mainline. However, there are
> many cases where the (S)DRAM base address isn't 0x8000_0000, so remove
> the hardcoding value, and introduce DRAM_BASE which will be set by
> users during configuring. DRAM_BASE is 0x8000_0000 by default.
> 
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>   arch/riscv/Kconfig | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 7895c77545f1..afd51dbdc253 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -247,10 +247,16 @@ config MMU
>   	  Select if you want MMU-based virtualised addressing space
>   	  support by paged memory management. If unsure, say 'Y'.
>   
> +if !MMU
> +config DRAM_BASE
> +	hex '(S)DRAM Base Address'
> +	default 0x80000000
> +endif
> +
>   config PAGE_OFFSET
>   	hex
>   	default 0xC0000000 if 32BIT && MMU
> -	default 0x80000000 if !MMU
> +	default DRAM_BASE if !MMU
>   	default 0xff60000000000000 if 64BIT
>   
>   config KASAN_SHADOW_OFFSET
> 

Thanks for this patch. I did something similar in my local nommu
linux-6.8 tree in order to run it on the S7 hart of JH7110. I have
another suggestion for you. Perhaps we should also make TASK_SIZE
configurable, and let it default to `0xffffffff if 32BIT && !MMU`
and `DRAM_BASE + DRAM_SIZE if 64BIT && !MMU`. Currently TASK_SIZE
is effectively `0xffffffff if !MMU`, which doesn't work if I run
rv64 linux-nommu with DDR that spans across 4G boundary.

I see there's another patchset that tries to define TASK_SIZE_MAX
for __access_ok(). Looks like that only affects the MMU case, and
NOMMU is not touched. My aforementioned change won't conflict with
it should it get merged.

Bo

Re: [PATCH 1/5] riscv: nommu: remove PAGE_OFFSET hardcoding

[Jisheng Zhang]

On Mon, Mar 25, 2024 at 03:46:01PM -0700, Bo Gan wrote:
> On 3/25/24 9:40 AM, Jisheng Zhang wrote:
> > Currently, PAGE_OFFSET is hardcoded as 0x8000_0000, it works fine since
> > there's only one nommu platform in the mainline. However, there are
> > many cases where the (S)DRAM base address isn't 0x8000_0000, so remove
> > the hardcoding value, and introduce DRAM_BASE which will be set by
> > users during configuring. DRAM_BASE is 0x8000_0000 by default.
> > 
> > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > ---
> >   arch/riscv/Kconfig | 8 +++++++-
> >   1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > index 7895c77545f1..afd51dbdc253 100644
> > --- a/arch/riscv/Kconfig
> > +++ b/arch/riscv/Kconfig
> > @@ -247,10 +247,16 @@ config MMU
> >   	  Select if you want MMU-based virtualised addressing space
> >   	  support by paged memory management. If unsure, say 'Y'.
> > +if !MMU
> > +config DRAM_BASE
> > +	hex '(S)DRAM Base Address'
> > +	default 0x80000000
> > +endif
> > +
> >   config PAGE_OFFSET
> >   	hex
> >   	default 0xC0000000 if 32BIT && MMU
> > -	default 0x80000000 if !MMU
> > +	default DRAM_BASE if !MMU
> >   	default 0xff60000000000000 if 64BIT
> >   config KASAN_SHADOW_OFFSET
> > 
> 
> Thanks for this patch. I did something similar in my local nommu
> linux-6.8 tree in order to run it on the S7 hart of JH7110. I have
> another suggestion for you. Perhaps we should also make TASK_SIZE
> configurable, and let it default to `0xffffffff if 32BIT && !MMU`
> and `DRAM_BASE + DRAM_SIZE if 64BIT && !MMU`. Currently TASK_SIZE
> is effectively `0xffffffff if !MMU`, which doesn't work if I run
> rv64 linux-nommu with DDR that spans across 4G boundary.

I must admit that there's such nommu linux with 4GB DDR case in
theory, but it doesn't exist in real world: who will make such
strange platform ;) But anyway this improvement can be made when
the patchset talking about TASK_SIZE_MAX is settled down.

> 
> I see there's another patchset that tries to define TASK_SIZE_MAX
> for __access_ok(). Looks like that only affects the MMU case, and
> NOMMU is not touched. My aforementioned change won't conflict with
> it should it get merged.
> 
> Bo

Re: [PATCH 1/5] riscv: nommu: remove PAGE_OFFSET hardcoding

[Samuel Holland]

On 2024-03-25 8:28 PM, Jisheng Zhang wrote:
> On Mon, Mar 25, 2024 at 03:46:01PM -0700, Bo Gan wrote:
>> On 3/25/24 9:40 AM, Jisheng Zhang wrote:
>>> Currently, PAGE_OFFSET is hardcoded as 0x8000_0000, it works fine since
>>> there's only one nommu platform in the mainline. However, there are
>>> many cases where the (S)DRAM base address isn't 0x8000_0000, so remove
>>> the hardcoding value, and introduce DRAM_BASE which will be set by
>>> users during configuring. DRAM_BASE is 0x8000_0000 by default.
>>>
>>> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
>>> ---
>>>   arch/riscv/Kconfig | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
>>> index 7895c77545f1..afd51dbdc253 100644
>>> --- a/arch/riscv/Kconfig
>>> +++ b/arch/riscv/Kconfig
>>> @@ -247,10 +247,16 @@ config MMU
>>>   	  Select if you want MMU-based virtualised addressing space
>>>   	  support by paged memory management. If unsure, say 'Y'.
>>> +if !MMU
>>> +config DRAM_BASE
>>> +	hex '(S)DRAM Base Address'
>>> +	default 0x80000000
>>> +endif
>>> +
>>>   config PAGE_OFFSET
>>>   	hex

Another option would be to change this to:

  hex "DRAM Base Address" if !MMU

so the prompt is only visible for NOMMU, but we don't need a new symbol.

>>>   	default 0xC0000000 if 32BIT && MMU
>>> -	default 0x80000000 if !MMU
>>> +	default DRAM_BASE if !MMU
>>>   	default 0xff60000000000000 if 64BIT
>>>   config KASAN_SHADOW_OFFSET
>>>
>>
>> Thanks for this patch. I did something similar in my local nommu
>> linux-6.8 tree in order to run it on the S7 hart of JH7110. I have
>> another suggestion for you. Perhaps we should also make TASK_SIZE
>> configurable, and let it default to `0xffffffff if 32BIT && !MMU`
>> and `DRAM_BASE + DRAM_SIZE if 64BIT && !MMU`. Currently TASK_SIZE
>> is effectively `0xffffffff if !MMU`, which doesn't work if I run
>> rv64 linux-nommu with DDR that spans across 4G boundary.
> 
> I must admit that there's such nommu linux with 4GB DDR case in
> theory, but it doesn't exist in real world: who will make such
> strange platform ;) But anyway this improvement can be made when
> the patchset talking about TASK_SIZE_MAX is settled down.

This case is quite easy to hit with QEMU :) In fact I sent a patch making this
exact change:

https://lore.kernel.org/linux-riscv/20240227003630.3634533-2-samuel.holland@sifive.com/

It's not really related to TASK_SIZE_MAX. access_ok() is a no-op on NOMMU,
because you can't prevent userspace from poking the kernel anyway.

Regards,
Samuel

[PATCH 2/5] riscv: nommu: use CSR_TIME* for get_cycles* implementation

[Jisheng Zhang]

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(-)

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)
 {
-- 
2.43.0

Re: [PATCH 2/5] riscv: nommu: use CSR_TIME* for get_cycles* implementation

[Samuel Holland]

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)
>  {

Re: [PATCH 2/5] riscv: nommu: use CSR_TIME* for get_cycles* implementation

[Jisheng Zhang]

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)
> >  {
> 

Re: [External] Re: [PATCH 2/5] riscv: nommu: use CSR_TIME* for get_cycles* implementation

[yunhui cui]

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

Re: [PATCH 2/5] riscv: nommu: use CSR_TIME* for get_cycles* implementation

[Jisheng Zhang]

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)
> > >  {
> > 

[PATCH 3/5] clocksource/drivers/timer-clint: Remove clint_time_val

[Jisheng Zhang]

The usage have been removed, so it's time to remove clint_time_val.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 arch/riscv/include/asm/clint.h    | 26 --------------------------
 drivers/clocksource/timer-clint.c | 17 -----------------
 2 files changed, 43 deletions(-)
 delete mode 100644 arch/riscv/include/asm/clint.h

diff --git a/arch/riscv/include/asm/clint.h b/arch/riscv/include/asm/clint.h
deleted file mode 100644
index 0789fd37b40a..000000000000
--- a/arch/riscv/include/asm/clint.h
+++ /dev/null
@@ -1,26 +0,0 @@
-/* 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/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
index 09fd292eb83d..56acaa93b6c3 100644
--- a/drivers/clocksource/timer-clint.c
+++ b/drivers/clocksource/timer-clint.c
@@ -24,10 +24,6 @@
 #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
 #define CLINT_TIMER_VAL_OFF	0xbff8
@@ -40,11 +36,6 @@ 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;
-EXPORT_SYMBOL(clint_time_val);
-#endif
-
 #ifdef CONFIG_SMP
 static void clint_send_ipi(unsigned int cpu)
 {
@@ -217,14 +208,6 @@ 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.43.0

[PATCH 4/5] clocksource/drivers/timer-clint: Use get_cycles()

[Jisheng Zhang]

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.", so it's fine to use
time CSR to implement sched_clock and clint clockevent/clocksource.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 drivers/clocksource/timer-clint.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
index 56acaa93b6c3..4537c77e623c 100644
--- a/drivers/clocksource/timer-clint.c
+++ b/drivers/clocksource/timer-clint.c
@@ -32,7 +32,6 @@
 static u32 __iomem *clint_ipi_base;
 static unsigned int clint_ipi_irq;
 static u64 __iomem *clint_timer_cmp;
-static u64 __iomem *clint_timer_val;
 static unsigned long clint_timer_freq;
 static unsigned int clint_timer_irq;
 
@@ -60,31 +59,10 @@ static void clint_ipi_interrupt(struct irq_desc *desc)
 }
 #endif
 
-#ifdef CONFIG_64BIT
-#define clint_get_cycles()	readq_relaxed(clint_timer_val)
-#else
-#define clint_get_cycles()	readl_relaxed(clint_timer_val)
-#define clint_get_cycles_hi()	readl_relaxed(((u32 *)clint_timer_val) + 1)
-#endif
-
-#ifdef CONFIG_64BIT
 static u64 notrace clint_get_cycles64(void)
 {
-	return clint_get_cycles();
-}
-#else /* CONFIG_64BIT */
-static u64 notrace clint_get_cycles64(void)
-{
-	u32 hi, lo;
-
-	do {
-		hi = clint_get_cycles_hi();
-		lo = clint_get_cycles();
-	} while (hi != clint_get_cycles_hi());
-
-	return ((u64)hi << 32) | lo;
+	return get_cycles64();
 }
-#endif /* CONFIG_64BIT */
 
 static u64 clint_rdtime(struct clocksource *cs)
 {
@@ -205,7 +183,6 @@ static int __init clint_timer_init_dt(struct device_node *np)
 
 	clint_ipi_base = base + CLINT_IPI_OFF;
 	clint_timer_cmp = base + CLINT_TIMER_CMP_OFF;
-	clint_timer_val = base + CLINT_TIMER_VAL_OFF;
 	clint_timer_freq = riscv_timebase;
 
 	pr_info("%pOFP: timer running at %ld Hz\n", np, clint_timer_freq);
-- 
2.43.0

[PATCH 5/5] clocksource/drivers/timer-clint: Add T-Head C9xx clint support

[Jisheng Zhang]

The mtimecmp in T-Head C9xx clint only supports 32bit read/write,
implement such support.

Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
---
 drivers/clocksource/timer-clint.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
index 4537c77e623c..71188732e8a3 100644
--- a/drivers/clocksource/timer-clint.c
+++ b/drivers/clocksource/timer-clint.c
@@ -34,6 +34,7 @@ static unsigned int clint_ipi_irq;
 static u64 __iomem *clint_timer_cmp;
 static unsigned long clint_timer_freq;
 static unsigned int clint_timer_irq;
+static bool is_c900_clint;
 
 #ifdef CONFIG_SMP
 static void clint_send_ipi(unsigned int cpu)
@@ -88,6 +89,19 @@ static int clint_clock_next_event(unsigned long delta,
 	return 0;
 }
 
+static int c900_clint_clock_next_event(unsigned long delta,
+				       struct clock_event_device *ce)
+{
+	void __iomem *r = clint_timer_cmp +
+			  cpuid_to_hartid_map(smp_processor_id());
+	u64 val = clint_get_cycles64() + delta;
+
+	csr_set(CSR_IE, IE_TIE);
+	writel_relaxed(val, r);
+	writel_relaxed(val >> 32, r + 4);
+	return 0;
+}
+
 static DEFINE_PER_CPU(struct clock_event_device, clint_clock_event) = {
 	.name		= "clint_clockevent",
 	.features	= CLOCK_EVT_FEAT_ONESHOT,
@@ -99,6 +113,9 @@ static int clint_timer_starting_cpu(unsigned int cpu)
 {
 	struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
 
+	if (is_c900_clint)
+		ce->set_next_event = c900_clint_clock_next_event;
+
 	ce->cpumask = cpumask_of(cpu);
 	clockevents_config_and_register(ce, clint_timer_freq, 100, ULONG_MAX);
 
@@ -233,5 +250,12 @@ static int __init clint_timer_init_dt(struct device_node *np)
 	return rc;
 }
 
+static int __init c900_clint_timer_init_dt(struct device_node *np)
+{
+	is_c900_clint = true;
+	return clint_timer_init_dt(np);
+}
+
 TIMER_OF_DECLARE(clint_timer, "riscv,clint0", clint_timer_init_dt);
 TIMER_OF_DECLARE(clint_timer1, "sifive,clint0", clint_timer_init_dt);
+TIMER_OF_DECLARE(clint_timer2, "thead,c900-clint", clint_timer_init_dt);
-- 
2.43.0

Re: [PATCH 5/5] clocksource/drivers/timer-clint: Add T-Head C9xx clint support

[Jisheng Zhang]

On Tue, Mar 26, 2024 at 12:40:21AM +0800, Jisheng Zhang wrote:
> The mtimecmp in T-Head C9xx clint only supports 32bit read/write,
> implement such support.
> 
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>  drivers/clocksource/timer-clint.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
> index 4537c77e623c..71188732e8a3 100644
> --- a/drivers/clocksource/timer-clint.c
> +++ b/drivers/clocksource/timer-clint.c
> @@ -34,6 +34,7 @@ static unsigned int clint_ipi_irq;
>  static u64 __iomem *clint_timer_cmp;
>  static unsigned long clint_timer_freq;
>  static unsigned int clint_timer_irq;
> +static bool is_c900_clint;
>  
>  #ifdef CONFIG_SMP
>  static void clint_send_ipi(unsigned int cpu)
> @@ -88,6 +89,19 @@ static int clint_clock_next_event(unsigned long delta,
>  	return 0;
>  }
>  
> +static int c900_clint_clock_next_event(unsigned long delta,
> +				       struct clock_event_device *ce)
> +{
> +	void __iomem *r = clint_timer_cmp +
> +			  cpuid_to_hartid_map(smp_processor_id());
> +	u64 val = clint_get_cycles64() + delta;
> +
> +	csr_set(CSR_IE, IE_TIE);
> +	writel_relaxed(val, r);
> +	writel_relaxed(val >> 32, r + 4);
> +	return 0;
> +}
> +
>  static DEFINE_PER_CPU(struct clock_event_device, clint_clock_event) = {
>  	.name		= "clint_clockevent",
>  	.features	= CLOCK_EVT_FEAT_ONESHOT,
> @@ -99,6 +113,9 @@ static int clint_timer_starting_cpu(unsigned int cpu)
>  {
>  	struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
>  
> +	if (is_c900_clint)
> +		ce->set_next_event = c900_clint_clock_next_event;
> +
>  	ce->cpumask = cpumask_of(cpu);
>  	clockevents_config_and_register(ce, clint_timer_freq, 100, ULONG_MAX);
>  
> @@ -233,5 +250,12 @@ static int __init clint_timer_init_dt(struct device_node *np)
>  	return rc;
>  }
>  
> +static int __init c900_clint_timer_init_dt(struct device_node *np)
> +{
> +	is_c900_clint = true;
> +	return clint_timer_init_dt(np);
> +}
> +
>  TIMER_OF_DECLARE(clint_timer, "riscv,clint0", clint_timer_init_dt);
>  TIMER_OF_DECLARE(clint_timer1, "sifive,clint0", clint_timer_init_dt);
> +TIMER_OF_DECLARE(clint_timer2, "thead,c900-clint", clint_timer_init_dt);

oops, this should be "c900_clint_timer_init_dt", will update in v2.

> -- 
> 2.43.0
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH 5/5] clocksource/drivers/timer-clint: Add T-Head C9xx clint support

[Bo Gan]

On 3/25/24 9:40 AM, Jisheng Zhang wrote:
> The mtimecmp in T-Head C9xx clint only supports 32bit read/write,
> implement such support.
> 
> Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> ---
>   drivers/clocksource/timer-clint.c | 24 ++++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
> index 4537c77e623c..71188732e8a3 100644
> --- a/drivers/clocksource/timer-clint.c
> +++ b/drivers/clocksource/timer-clint.c
> @@ -34,6 +34,7 @@ static unsigned int clint_ipi_irq;
>   static u64 __iomem *clint_timer_cmp;
>   static unsigned long clint_timer_freq;
>   static unsigned int clint_timer_irq;
> +static bool is_c900_clint;
>   
>   #ifdef CONFIG_SMP
>   static void clint_send_ipi(unsigned int cpu)
> @@ -88,6 +89,19 @@ static int clint_clock_next_event(unsigned long delta,
>   	return 0;
>   }
>   
> +static int c900_clint_clock_next_event(unsigned long delta,
> +				       struct clock_event_device *ce)
> +{
> +	void __iomem *r = clint_timer_cmp +
> +			  cpuid_to_hartid_map(smp_processor_id());
> +	u64 val = clint_get_cycles64() + delta;
> +
> +	csr_set(CSR_IE, IE_TIE);
Perhaps you should do a writel_relaxed(-1, r) here. just like openSBI, because the update
to mtimecmp is now split into 2 parts.
https://github.com/riscv-software-src/opensbi/blob/v1.4/lib/utils/timer/aclint_mtimer.c#L54

> +	writel_relaxed(val, r);
> +	writel_relaxed(val >> 32, r + 4);
> +	return 0;
> +}
> +>   static DEFINE_PER_CPU(struct clock_event_device, clint_clock_event) = {
>   	.name		= "clint_clockevent",
>   	.features	= CLOCK_EVT_FEAT_ONESHOT,
> @@ -99,6 +113,9 @@ static int clint_timer_starting_cpu(unsigned int cpu)
>   {
>   	struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
>   
> +	if (is_c900_clint)
> +		ce->set_next_event = c900_clint_clock_next_event;
> +
>   	ce->cpumask = cpumask_of(cpu);
>   	clockevents_config_and_register(ce, clint_timer_freq, 100, ULONG_MAX);
>   
> @@ -233,5 +250,12 @@ static int __init clint_timer_init_dt(struct device_node *np)
>   	return rc;
>   }
>   
> +static int __init c900_clint_timer_init_dt(struct device_node *np)
> +{
> +	is_c900_clint = true;
> +	return clint_timer_init_dt(np);
> +}
> +
>   TIMER_OF_DECLARE(clint_timer, "riscv,clint0", clint_timer_init_dt);
>   TIMER_OF_DECLARE(clint_timer1, "sifive,clint0", clint_timer_init_dt);
> +TIMER_OF_DECLARE(clint_timer2, "thead,c900-clint", clint_timer_init_dt);
> 
Better use a more generic term to describe the fact that mtimecmp doesn't support
64-bit mmio, just like what openSBI is currently doing, instead of making it c900 specific:

https://github.com/riscv-software-src/opensbi/blob/v1.4/lib/utils/timer/fdt_timer_mtimer.c#L152

Then your `is_c900_clint` becomes something like `timecmp_64bit_mmio`.

Bo

Re: [PATCH 5/5] clocksource/drivers/timer-clint: Add T-Head C9xx clint support

[Jisheng Zhang]

On Mon, Mar 25, 2024 at 03:22:11PM -0700, Bo Gan wrote:
> On 3/25/24 9:40 AM, Jisheng Zhang wrote:
> > The mtimecmp in T-Head C9xx clint only supports 32bit read/write,
> > implement such support.
> > 
> > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > ---
> >   drivers/clocksource/timer-clint.c | 24 ++++++++++++++++++++++++
> >   1 file changed, 24 insertions(+)
> > 
> > diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
> > index 4537c77e623c..71188732e8a3 100644
> > --- a/drivers/clocksource/timer-clint.c
> > +++ b/drivers/clocksource/timer-clint.c
> > @@ -34,6 +34,7 @@ static unsigned int clint_ipi_irq;
> >   static u64 __iomem *clint_timer_cmp;
> >   static unsigned long clint_timer_freq;
> >   static unsigned int clint_timer_irq;
> > +static bool is_c900_clint;
> >   #ifdef CONFIG_SMP
> >   static void clint_send_ipi(unsigned int cpu)
> > @@ -88,6 +89,19 @@ static int clint_clock_next_event(unsigned long delta,
> >   	return 0;
> >   }
> > +static int c900_clint_clock_next_event(unsigned long delta,
> > +				       struct clock_event_device *ce)
> > +{
> > +	void __iomem *r = clint_timer_cmp +
> > +			  cpuid_to_hartid_map(smp_processor_id());
> > +	u64 val = clint_get_cycles64() + delta;
> > +
> > +	csr_set(CSR_IE, IE_TIE);
> Perhaps you should do a writel_relaxed(-1, r) here. just like openSBI, because the update
> to mtimecmp is now split into 2 parts.
> https://github.com/riscv-software-src/opensbi/blob/v1.4/lib/utils/timer/aclint_mtimer.c#L54

Thanks, I also noticed the mtimecmp update on 32bit platforms doesn't
strictly follow the riscv spec:

# New comparand is in a1:a0.
li t0, -1
la t1, mtimecmp
sw t0, 0(t1) # No smaller than old value.
sw a1, 4(t1) # No smaller than new value.
sw a0, 0(t1) # New value.

So A new bug fix patch will be added in v2.


> 
> > +	writel_relaxed(val, r);
> > +	writel_relaxed(val >> 32, r + 4);
> > +	return 0;
> > +}
> > +>   static DEFINE_PER_CPU(struct clock_event_device, clint_clock_event) = {
> >   	.name		= "clint_clockevent",
> >   	.features	= CLOCK_EVT_FEAT_ONESHOT,
> > @@ -99,6 +113,9 @@ static int clint_timer_starting_cpu(unsigned int cpu)
> >   {
> >   	struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
> > +	if (is_c900_clint)
> > +		ce->set_next_event = c900_clint_clock_next_event;
> > +
> >   	ce->cpumask = cpumask_of(cpu);
> >   	clockevents_config_and_register(ce, clint_timer_freq, 100, ULONG_MAX);
> > @@ -233,5 +250,12 @@ static int __init clint_timer_init_dt(struct device_node *np)
> >   	return rc;
> >   }
> > +static int __init c900_clint_timer_init_dt(struct device_node *np)
> > +{
> > +	is_c900_clint = true;
> > +	return clint_timer_init_dt(np);
> > +}
> > +
> >   TIMER_OF_DECLARE(clint_timer, "riscv,clint0", clint_timer_init_dt);
> >   TIMER_OF_DECLARE(clint_timer1, "sifive,clint0", clint_timer_init_dt);
> > +TIMER_OF_DECLARE(clint_timer2, "thead,c900-clint", clint_timer_init_dt);
> > 
> Better use a more generic term to describe the fact that mtimecmp doesn't support
> 64-bit mmio, just like what openSBI is currently doing, instead of making it c900 specific:

This has been mentioned in commit msg, but adding a code comment seems fine.
> 
> https://github.com/riscv-software-src/opensbi/blob/v1.4/lib/utils/timer/fdt_timer_mtimer.c#L152
> 
> Then your `is_c900_clint` becomes something like `timecmp_64bit_mmio`.
> 
> Bo

Re: [PATCH 5/5] clocksource/drivers/timer-clint: Add T-Head C9xx clint support

[Jisheng Zhang]

On Tue, Mar 26, 2024 at 09:25:09AM +0800, Jisheng Zhang wrote:
> On Mon, Mar 25, 2024 at 03:22:11PM -0700, Bo Gan wrote:
> > On 3/25/24 9:40 AM, Jisheng Zhang wrote:
> > > The mtimecmp in T-Head C9xx clint only supports 32bit read/write,
> > > implement such support.
> > > 
> > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > > ---
> > >   drivers/clocksource/timer-clint.c | 24 ++++++++++++++++++++++++
> > >   1 file changed, 24 insertions(+)
> > > 
> > > diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
> > > index 4537c77e623c..71188732e8a3 100644
> > > --- a/drivers/clocksource/timer-clint.c
> > > +++ b/drivers/clocksource/timer-clint.c
> > > @@ -34,6 +34,7 @@ static unsigned int clint_ipi_irq;
> > >   static u64 __iomem *clint_timer_cmp;
> > >   static unsigned long clint_timer_freq;
> > >   static unsigned int clint_timer_irq;
> > > +static bool is_c900_clint;
> > >   #ifdef CONFIG_SMP
> > >   static void clint_send_ipi(unsigned int cpu)
> > > @@ -88,6 +89,19 @@ static int clint_clock_next_event(unsigned long delta,
> > >   	return 0;
> > >   }
> > > +static int c900_clint_clock_next_event(unsigned long delta,
> > > +				       struct clock_event_device *ce)
> > > +{
> > > +	void __iomem *r = clint_timer_cmp +
> > > +			  cpuid_to_hartid_map(smp_processor_id());
> > > +	u64 val = clint_get_cycles64() + delta;
> > > +
> > > +	csr_set(CSR_IE, IE_TIE);
> > Perhaps you should do a writel_relaxed(-1, r) here. just like openSBI, because the update
> > to mtimecmp is now split into 2 parts.
> > https://github.com/riscv-software-src/opensbi/blob/v1.4/lib/utils/timer/aclint_mtimer.c#L54
> 
> Thanks, I also noticed the mtimecmp update on 32bit platforms doesn't
> strictly follow the riscv spec:
> 
> # New comparand is in a1:a0.
> li t0, -1
> la t1, mtimecmp
> sw t0, 0(t1) # No smaller than old value.
> sw a1, 4(t1) # No smaller than new value.
> sw a0, 0(t1) # New value.
> 
> So A new bug fix patch will be added in v2.
> 

wait, I found a similar bug in timer-riscv.c, and since these are fixes,
I'd like to send fixes as a seperate series. I'm cooking the patches

> 
> > 
> > > +	writel_relaxed(val, r);
> > > +	writel_relaxed(val >> 32, r + 4);
> > > +	return 0;
> > > +}
> > > +>   static DEFINE_PER_CPU(struct clock_event_device, clint_clock_event) = {
> > >   	.name		= "clint_clockevent",
> > >   	.features	= CLOCK_EVT_FEAT_ONESHOT,
> > > @@ -99,6 +113,9 @@ static int clint_timer_starting_cpu(unsigned int cpu)
> > >   {
> > >   	struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
> > > +	if (is_c900_clint)
> > > +		ce->set_next_event = c900_clint_clock_next_event;
> > > +
> > >   	ce->cpumask = cpumask_of(cpu);
> > >   	clockevents_config_and_register(ce, clint_timer_freq, 100, ULONG_MAX);
> > > @@ -233,5 +250,12 @@ static int __init clint_timer_init_dt(struct device_node *np)
> > >   	return rc;
> > >   }
> > > +static int __init c900_clint_timer_init_dt(struct device_node *np)
> > > +{
> > > +	is_c900_clint = true;
> > > +	return clint_timer_init_dt(np);
> > > +}
> > > +
> > >   TIMER_OF_DECLARE(clint_timer, "riscv,clint0", clint_timer_init_dt);
> > >   TIMER_OF_DECLARE(clint_timer1, "sifive,clint0", clint_timer_init_dt);
> > > +TIMER_OF_DECLARE(clint_timer2, "thead,c900-clint", clint_timer_init_dt);
> > > 
> > Better use a more generic term to describe the fact that mtimecmp doesn't support
> > 64-bit mmio, just like what openSBI is currently doing, instead of making it c900 specific:
> 
> This has been mentioned in commit msg, but adding a code comment seems fine.
> > 
> > https://github.com/riscv-software-src/opensbi/blob/v1.4/lib/utils/timer/fdt_timer_mtimer.c#L152
> > 
> > Then your `is_c900_clint` becomes something like `timecmp_64bit_mmio`.
> > 
> > Bo

Re: [PATCH 5/5] clocksource/drivers/timer-clint: Add T-Head C9xx clint support

[Jisheng Zhang]

On Tue, Mar 26, 2024 at 09:31:14AM +0800, Jisheng Zhang wrote:
> On Tue, Mar 26, 2024 at 09:25:09AM +0800, Jisheng Zhang wrote:
> > On Mon, Mar 25, 2024 at 03:22:11PM -0700, Bo Gan wrote:
> > > On 3/25/24 9:40 AM, Jisheng Zhang wrote:
> > > > The mtimecmp in T-Head C9xx clint only supports 32bit read/write,
> > > > implement such support.
> > > > 
> > > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > > > ---
> > > >   drivers/clocksource/timer-clint.c | 24 ++++++++++++++++++++++++
> > > >   1 file changed, 24 insertions(+)
> > > > 
> > > > diff --git a/drivers/clocksource/timer-clint.c b/drivers/clocksource/timer-clint.c
> > > > index 4537c77e623c..71188732e8a3 100644
> > > > --- a/drivers/clocksource/timer-clint.c
> > > > +++ b/drivers/clocksource/timer-clint.c
> > > > @@ -34,6 +34,7 @@ static unsigned int clint_ipi_irq;
> > > >   static u64 __iomem *clint_timer_cmp;
> > > >   static unsigned long clint_timer_freq;
> > > >   static unsigned int clint_timer_irq;
> > > > +static bool is_c900_clint;
> > > >   #ifdef CONFIG_SMP
> > > >   static void clint_send_ipi(unsigned int cpu)
> > > > @@ -88,6 +89,19 @@ static int clint_clock_next_event(unsigned long delta,
> > > >   	return 0;
> > > >   }
> > > > +static int c900_clint_clock_next_event(unsigned long delta,
> > > > +				       struct clock_event_device *ce)
> > > > +{
> > > > +	void __iomem *r = clint_timer_cmp +
> > > > +			  cpuid_to_hartid_map(smp_processor_id());
> > > > +	u64 val = clint_get_cycles64() + delta;
> > > > +
> > > > +	csr_set(CSR_IE, IE_TIE);
> > > Perhaps you should do a writel_relaxed(-1, r) here. just like openSBI, because the update
> > > to mtimecmp is now split into 2 parts.
> > > https://github.com/riscv-software-src/opensbi/blob/v1.4/lib/utils/timer/aclint_mtimer.c#L54

Hi,

The reason of "writel_relaxed(-1, r)" is to avoid spurious irq, this is
well explained by riscv spec. 
After more thoughts, this is not needed here in linux kernel because
set_next_event() is only called in ISR, so the irq has been disabled,
we don't suffer from spurious irq problem.

Thanks
> > 
> > Thanks, I also noticed the mtimecmp update on 32bit platforms doesn't
> > strictly follow the riscv spec:
> > 
> > # New comparand is in a1:a0.
> > li t0, -1
> > la t1, mtimecmp
> > sw t0, 0(t1) # No smaller than old value.
> > sw a1, 4(t1) # No smaller than new value.
> > sw a0, 0(t1) # New value.
> > 
> > So A new bug fix patch will be added in v2.
> > 
> 
> wait, I found a similar bug in timer-riscv.c, and since these are fixes,
> I'd like to send fixes as a seperate series. I'm cooking the patches
> 
> > 
> > > 
> > > > +	writel_relaxed(val, r);
> > > > +	writel_relaxed(val >> 32, r + 4);
> > > > +	return 0;
> > > > +}
> > > > +>   static DEFINE_PER_CPU(struct clock_event_device, clint_clock_event) = {
> > > >   	.name		= "clint_clockevent",
> > > >   	.features	= CLOCK_EVT_FEAT_ONESHOT,
> > > > @@ -99,6 +113,9 @@ static int clint_timer_starting_cpu(unsigned int cpu)
> > > >   {
> > > >   	struct clock_event_device *ce = per_cpu_ptr(&clint_clock_event, cpu);
> > > > +	if (is_c900_clint)
> > > > +		ce->set_next_event = c900_clint_clock_next_event;
> > > > +
> > > >   	ce->cpumask = cpumask_of(cpu);
> > > >   	clockevents_config_and_register(ce, clint_timer_freq, 100, ULONG_MAX);
> > > > @@ -233,5 +250,12 @@ static int __init clint_timer_init_dt(struct device_node *np)
> > > >   	return rc;
> > > >   }
> > > > +static int __init c900_clint_timer_init_dt(struct device_node *np)
> > > > +{
> > > > +	is_c900_clint = true;
> > > > +	return clint_timer_init_dt(np);
> > > > +}
> > > > +
> > > >   TIMER_OF_DECLARE(clint_timer, "riscv,clint0", clint_timer_init_dt);
> > > >   TIMER_OF_DECLARE(clint_timer1, "sifive,clint0", clint_timer_init_dt);
> > > > +TIMER_OF_DECLARE(clint_timer2, "thead,c900-clint", clint_timer_init_dt);
> > > > 
> > > Better use a more generic term to describe the fact that mtimecmp doesn't support
> > > 64-bit mmio, just like what openSBI is currently doing, instead of making it c900 specific:
> > 
> > This has been mentioned in commit msg, but adding a code comment seems fine.
> > > 
> > > https://github.com/riscv-software-src/opensbi/blob/v1.4/lib/utils/timer/fdt_timer_mtimer.c#L152
> > > 
> > > Then your `is_c900_clint` becomes something like `timecmp_64bit_mmio`.
> > > 
> > > Bo

Re: [PATCH 5/5] clocksource/drivers/timer-clint: Add T-Head C9xx clint support

[kernel test robot]

Hi Jisheng,

kernel test robot noticed the following build warnings:

[auto build test WARNING on linus/master]
[also build test WARNING on v6.9-rc1 next-20240327]
[cannot apply to tip/timers/core]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Jisheng-Zhang/riscv-nommu-remove-PAGE_OFFSET-hardcoding/20240326-021401
base:   linus/master
patch link:    https://lore.kernel.org/r/20240325164021.3229-6-jszhang%40kernel.org
patch subject: [PATCH 5/5] clocksource/drivers/timer-clint: Add T-Head C9xx clint support
config: riscv-randconfig-r131-20240326 (https://download.01.org/0day-ci/archive/20240328/202403280636.SLv93x6B-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240328/202403280636.SLv93x6B-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202403280636.SLv93x6B-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/clocksource/timer-clint.c:253:19: warning: 'c900_clint_timer_init_dt' defined but not used [-Wunused-function]
     253 | static int __init c900_clint_timer_init_dt(struct device_node *np)
         |                   ^~~~~~~~~~~~~~~~~~~~~~~~


vim +/c900_clint_timer_init_dt +253 drivers/clocksource/timer-clint.c

   252	
 > 253	static int __init c900_clint_timer_init_dt(struct device_node *np)
   254	{
   255		is_c900_clint = true;
   256		return clint_timer_init_dt(np);
   257	}
   258	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki