[PATCH v2 0/3] Support SSTC while PM operations

[PATCH v2 0/3] Support SSTC while PM operations

[Nick Hu]

When the cpu is going to be hotplug, stop the stimecmp to prevent pending
interrupt.
When the cpu is going to be suspended, save the stimecmp before entering
the suspend state and restore it in the resume path.

changes in v2:
1. Add csr_read/write_hi_lo operations
2. Apply the suggestion from Anup.
link: https://lore.kernel.org/lkml/20240829033904.477200-3-nick.hu@sifive.com/T/#u

Nick Hu (3):
  riscv: Add csr_read/write_hi_lo support
  riscv: Add stimecmp save and restore
  clocksource/drivers/timer-riscv: Stop stimecmp when cpu hotplug

 arch/riscv/include/asm/csr.h      | 22 ++++++++++++++++++++++
 arch/riscv/include/asm/suspend.h  |  1 +
 arch/riscv/kernel/suspend.c       |  6 ++++++
 drivers/clocksource/timer-riscv.c |  6 ++++++
 4 files changed, 35 insertions(+)

-- 
2.34.1

[PATCH v2 1/3] riscv: Add csr_read/write_hi_lo support

[Nick Hu]

In RV32, we may have the need to read both low 32 bit and high 32 bit of
the CSR. Therefore adding the csr_read_hi_lo() and csr_write_hi_lo() to
support such case.

Suggested-by: Andrew Jones <ajones@ventanamicro.com>
Suggested-by: Zong Li <zong.li@sifive.com>
Signed-off-by: Nick Hu <nick.hu@sifive.com>
---
 arch/riscv/include/asm/csr.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
index 25966995da04..54198284eb22 100644
--- a/arch/riscv/include/asm/csr.h
+++ b/arch/riscv/include/asm/csr.h
@@ -501,6 +501,17 @@
 	__v;							\
 })
 
+#if __riscv_xlen < 64
+#define csr_read_hi_lo(csr)					\
+({								\
+	u32 hi = csr_read(csr##H);				\
+	u32 lo = csr_read(csr);					\
+	lo | ((u64)hi << 32);					\
+})
+#else
+#define csr_read_hi_lo	csr_read
+#endif
+
 #define csr_write(csr, val)					\
 ({								\
 	unsigned long __v = (unsigned long)(val);		\
@@ -509,6 +520,17 @@
 			      : "memory");			\
 })
 
+#if __riscv_xlen < 64
+#define csr_write_hi_lo(csr, val)				\
+({								\
+	u64 _v = (u64)(val);					\
+	csr_write(csr##H, (_v) >> 32);				\
+	csr_write(csr, (_v));					\
+})
+#else
+#define csr_write_hi_lo	csr_write
+#endif
+
 #define csr_read_set(csr, val)					\
 ({								\
 	unsigned long __v = (unsigned long)(val);		\
-- 
2.34.1

Re: [PATCH v2 1/3] riscv: Add csr_read/write_hi_lo support

[Andrew Jones]

On Thu, Sep 26, 2024 at 02:54:16PM GMT, Nick Hu wrote:
> In RV32, we may have the need to read both low 32 bit and high 32 bit of
> the CSR. Therefore adding the csr_read_hi_lo() and csr_write_hi_lo() to
> support such case.
> 
> Suggested-by: Andrew Jones <ajones@ventanamicro.com>
> Suggested-by: Zong Li <zong.li@sifive.com>
> Signed-off-by: Nick Hu <nick.hu@sifive.com>
> ---
>  arch/riscv/include/asm/csr.h | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
> index 25966995da04..54198284eb22 100644
> --- a/arch/riscv/include/asm/csr.h
> +++ b/arch/riscv/include/asm/csr.h
> @@ -501,6 +501,17 @@
>  	__v;							\
>  })
>  
> +#if __riscv_xlen < 64
> +#define csr_read_hi_lo(csr)					\
> +({								\
> +	u32 hi = csr_read(csr##H);				\
> +	u32 lo = csr_read(csr);					\
> +	lo | ((u64)hi << 32);					\
> +})
> +#else
> +#define csr_read_hi_lo	csr_read
> +#endif
> +
>  #define csr_write(csr, val)					\
>  ({								\
>  	unsigned long __v = (unsigned long)(val);		\
> @@ -509,6 +520,17 @@
>  			      : "memory");			\
>  })
>  
> +#if __riscv_xlen < 64
> +#define csr_write_hi_lo(csr, val)				\
> +({								\
> +	u64 _v = (u64)(val);					\
> +	csr_write(csr##H, (_v) >> 32);				\
> +	csr_write(csr, (_v));					\
> +})
> +#else
> +#define csr_write_hi_lo	csr_write
> +#endif
> +
>  #define csr_read_set(csr, val)					\
>  ({								\
>  	unsigned long __v = (unsigned long)(val);		\
> -- 
> 2.34.1
>

I know I suggested this, but I'm having second thoughts. The nice thing
about the

 csr_write(CSR, ...);
 if (__riscv_xlen < 64)
    csr_write(CSRH, ...);

pattern is that it matches the spec. With this helper we'll have

 csr_write_hi_lo(CSR, ...);

for both rv32 and rv64. That looks odd for rv64 and hides the hi register
access for rv32.

We could avoid the oddness of the helper's name for rv64 if we instead
added csr_write32 and csr_write64 which do the right things, but that
still hides the hi register access for rv32. Hiding the hi register
access is probably fine, though, since we can be pretty certain that
the spec will rarely, if ever, deviate from naming high registers with
the H suffix and/or not keep the upper bits compatible.

In summary, I think I'm in favor of just dropping this patch, keeping
the noisy, but explicit, pattern. Or, if the consensus is to add
helpers, then I'd rather have all csr_<op>32/64 helpers. Then, code
would match the spec by choosing the right helper based on the width
of the CSR being accessed, when the CSR has an explicit width, or still
use the current helpers for xlen-wide CSRs.

Thanks,
drew

Re: [PATCH v2 1/3] riscv: Add csr_read/write_hi_lo support

[Nick Hu]

It seems like my last mail didn't send out successfully.

On Wed, Oct 2, 2024 at 9:52 AM Nick Hu <nick.hu@sifive.com> wrote:
>
> Hi Andrew
>
> On Thu, Sep 26, 2024 at 3:59 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>>
>> On Thu, Sep 26, 2024 at 02:54:16PM GMT, Nick Hu wrote:
>> > In RV32, we may have the need to read both low 32 bit and high 32 bit of
>> > the CSR. Therefore adding the csr_read_hi_lo() and csr_write_hi_lo() to
>> > support such case.
>> >
>> > Suggested-by: Andrew Jones <ajones@ventanamicro.com>
>> > Suggested-by: Zong Li <zong.li@sifive.com>
>> > Signed-off-by: Nick Hu <nick.hu@sifive.com>
>> > ---
>> >  arch/riscv/include/asm/csr.h | 22 ++++++++++++++++++++++
>> >  1 file changed, 22 insertions(+)
>> >
>> > diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
>> > index 25966995da04..54198284eb22 100644
>> > --- a/arch/riscv/include/asm/csr.h
>> > +++ b/arch/riscv/include/asm/csr.h
>> > @@ -501,6 +501,17 @@
>> >       __v;                                                    \
>> >  })
>> >
>> > +#if __riscv_xlen < 64
>> > +#define csr_read_hi_lo(csr)                                  \
>> > +({                                                           \
>> > +     u32 hi = csr_read(csr##H);                              \
>> > +     u32 lo = csr_read(csr);                                 \
>> > +     lo | ((u64)hi << 32);                                   \
>> > +})
>> > +#else
>> > +#define csr_read_hi_lo       csr_read
>> > +#endif
>> > +
>> >  #define csr_write(csr, val)                                  \
>> >  ({                                                           \
>> >       unsigned long __v = (unsigned long)(val);               \
>> > @@ -509,6 +520,17 @@
>> >                             : "memory");                      \
>> >  })
>> >
>> > +#if __riscv_xlen < 64
>> > +#define csr_write_hi_lo(csr, val)                            \
>> > +({                                                           \
>> > +     u64 _v = (u64)(val);                                    \
>> > +     csr_write(csr##H, (_v) >> 32);                          \
>> > +     csr_write(csr, (_v));                                   \
>> > +})
>> > +#else
>> > +#define csr_write_hi_lo      csr_write
>> > +#endif
>> > +
>> >  #define csr_read_set(csr, val)                                       \
>> >  ({                                                           \
>> >       unsigned long __v = (unsigned long)(val);               \
>> > --
>> > 2.34.1
>> >
>>
>> I know I suggested this, but I'm having second thoughts. The nice thing
>> about the
>>
>>  csr_write(CSR, ...);
>>  if (__riscv_xlen < 64)
>>     csr_write(CSRH, ...);
>>
>> pattern is that it matches the spec. With this helper we'll have
>>
>>  csr_write_hi_lo(CSR, ...);
>>
>> for both rv32 and rv64. That looks odd for rv64 and hides the hi register
>> access for rv32.
>>
>> We could avoid the oddness of the helper's name for rv64 if we instead
>> added csr_write32 and csr_write64 which do the right things, but that
>> still hides the hi register access for rv32. Hiding the hi register
>> access is probably fine, though, since we can be pretty certain that
>> the spec will rarely, if ever, deviate from naming high registers with
>> the H suffix and/or not keep the upper bits compatible.
>>
>> In summary, I think I'm in favor of just dropping this patch, keeping
>> the noisy, but explicit, pattern. Or, if the consensus is to add
>> helpers, then I'd rather have all csr_<op>32/64 helpers. Then, code
>> would match the spec by choosing the right helper based on the width
>> of the CSR being accessed, when the CSR has an explicit width, or still
>> use the current helpers for xlen-wide CSRs.
>>
>> Thanks,
>> drew
>
Sure I'll drop the patch in the next version


Regards,
Nick

[PATCH v2 2/3] riscv: Add stimecmp save and restore

[Nick Hu]

If the HW support the SSTC extension, we should save and restore the
stimecmp register while cpu non retention suspend.

Signed-off-by: Nick Hu <nick.hu@sifive.com>
---
 arch/riscv/include/asm/suspend.h | 1 +
 arch/riscv/kernel/suspend.c      | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/arch/riscv/include/asm/suspend.h b/arch/riscv/include/asm/suspend.h
index 4ffb022b097f..4dfdb7134d0e 100644
--- a/arch/riscv/include/asm/suspend.h
+++ b/arch/riscv/include/asm/suspend.h
@@ -18,6 +18,7 @@ struct suspend_context {
 	unsigned long ie;
 #ifdef CONFIG_MMU
 	unsigned long satp;
+	u64 stimecmp;
 #endif
 };
 
diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c
index c8cec0cc5833..5d9036ea6784 100644
--- a/arch/riscv/kernel/suspend.c
+++ b/arch/riscv/kernel/suspend.c
@@ -30,6 +30,9 @@ void suspend_save_csrs(struct suspend_context *context)
 	 */
 
 #ifdef CONFIG_MMU
+	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SSTC))
+		context->stimecmp = csr_read_hi_lo(CSR_STIMECMP);
+
 	context->satp = csr_read(CSR_SATP);
 #endif
 }
@@ -43,6 +46,9 @@ void suspend_restore_csrs(struct suspend_context *context)
 	csr_write(CSR_IE, context->ie);
 
 #ifdef CONFIG_MMU
+	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SSTC))
+		csr_write_hi_lo(CSR_STIMECMP, context->stimecmp);
+
 	csr_write(CSR_SATP, context->satp);
 #endif
 }
-- 
2.34.1

[PATCH v2 3/3] clocksource/drivers/timer-riscv: Stop stimecmp when cpu hotplug

[Nick Hu]

Stop the stimecmp when the cpu is going to be off otherwise the timer
stimecmp register while cpu non retention suspend.

Suggested-by: Anup Patel <anup@brainfault.org>
Link: https://lore.kernel.org/lkml/20240829033904.477200-3-nick.hu@sifive.com/T/#u
Signed-off-by: Nick Hu <nick.hu@sifive.com>
---
 drivers/clocksource/timer-riscv.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
index 48ce50c5f5e6..166dee14e46b 100644
--- a/drivers/clocksource/timer-riscv.c
+++ b/drivers/clocksource/timer-riscv.c
@@ -127,6 +127,12 @@ static int riscv_timer_starting_cpu(unsigned int cpu)
 static int riscv_timer_dying_cpu(unsigned int cpu)
 {
 	disable_percpu_irq(riscv_clock_event_irq);
+	/*
+	 * Stop the timer when the cpu is going to be offline otherwise
+	 * the timer interrupt may be pending while performing power-down.
+	 */
+	riscv_clock_event_stop();
+
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH v2 3/3] clocksource/drivers/timer-riscv: Stop stimecmp when cpu hotplug

[Anup Patel]

On Thu, Sep 26, 2024 at 12:26 PM Nick Hu <nick.hu@sifive.com> wrote:
>
> Stop the stimecmp when the cpu is going to be off otherwise the timer
> stimecmp register while cpu non retention suspend.
>
> Suggested-by: Anup Patel <anup@brainfault.org>
> Link: https://lore.kernel.org/lkml/20240829033904.477200-3-nick.hu@sifive.com/T/#u
> Signed-off-by: Nick Hu <nick.hu@sifive.com>

LGTM.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

> ---
>  drivers/clocksource/timer-riscv.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
> index 48ce50c5f5e6..166dee14e46b 100644
> --- a/drivers/clocksource/timer-riscv.c
> +++ b/drivers/clocksource/timer-riscv.c
> @@ -127,6 +127,12 @@ static int riscv_timer_starting_cpu(unsigned int cpu)
>  static int riscv_timer_dying_cpu(unsigned int cpu)
>  {
>         disable_percpu_irq(riscv_clock_event_irq);
> +       /*
> +        * Stop the timer when the cpu is going to be offline otherwise
> +        * the timer interrupt may be pending while performing power-down.
> +        */
> +       riscv_clock_event_stop();
> +
>         return 0;
>  }
>
> --
> 2.34.1
>

Re: [PATCH v2 3/3] clocksource/drivers/timer-riscv: Stop stimecmp when cpu hotplug

[Andrew Jones]

On Thu, Sep 26, 2024 at 02:54:18PM GMT, Nick Hu wrote:
> Stop the stimecmp when the cpu is going to be off otherwise the timer
> stimecmp register while cpu non retention suspend.

This commit message seems to be missing some words. The comment below
reads much better.

Thanks,
drew

> 
> Suggested-by: Anup Patel <anup@brainfault.org>
> Link: https://lore.kernel.org/lkml/20240829033904.477200-3-nick.hu@sifive.com/T/#u
> Signed-off-by: Nick Hu <nick.hu@sifive.com>
> ---
>  drivers/clocksource/timer-riscv.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
> index 48ce50c5f5e6..166dee14e46b 100644
> --- a/drivers/clocksource/timer-riscv.c
> +++ b/drivers/clocksource/timer-riscv.c
> @@ -127,6 +127,12 @@ static int riscv_timer_starting_cpu(unsigned int cpu)
>  static int riscv_timer_dying_cpu(unsigned int cpu)
>  {
>  	disable_percpu_irq(riscv_clock_event_irq);
> +	/*
> +	 * Stop the timer when the cpu is going to be offline otherwise
> +	 * the timer interrupt may be pending while performing power-down.
> +	 */
> +	riscv_clock_event_stop();
> +
>  	return 0;
>  }
>  
> -- 
> 2.34.1
>