[PATCH 0/2] Support SSTC while PM operations

[PATCH 0/2] 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.

Nick Hu (2):
  riscv: Add stimecmp save and restore
  time-riscv: Stop stimecmp when cpu hotplug

 arch/riscv/include/asm/suspend.h  |  4 ++++
 arch/riscv/kernel/suspend.c       | 13 +++++++++++++
 drivers/clocksource/timer-riscv.c | 22 +++++++++++++++-------
 3 files changed, 32 insertions(+), 7 deletions(-)

-- 
2.34.1

[PATCH 1/2] 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 |  4 ++++
 arch/riscv/kernel/suspend.c      | 13 +++++++++++++
 2 files changed, 17 insertions(+)

diff --git a/arch/riscv/include/asm/suspend.h b/arch/riscv/include/asm/suspend.h
index 4ffb022b097f..ffaac2efabb5 100644
--- a/arch/riscv/include/asm/suspend.h
+++ b/arch/riscv/include/asm/suspend.h
@@ -16,6 +16,10 @@ struct suspend_context {
 	unsigned long envcfg;
 	unsigned long tvec;
 	unsigned long ie;
+#if __riscv_xlen < 64
+	unsigned long stimecmph;
+#endif
+	unsigned long stimecmp;
 #ifdef CONFIG_MMU
 	unsigned long satp;
 #endif
diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c
index c8cec0cc5833..3afd86e1abf7 100644
--- a/arch/riscv/kernel/suspend.c
+++ b/arch/riscv/kernel/suspend.c
@@ -19,6 +19,12 @@ void suspend_save_csrs(struct suspend_context *context)
 	context->tvec = csr_read(CSR_TVEC);
 	context->ie = csr_read(CSR_IE);
 
+	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SSTC)) {
+		context->stimecmp = csr_read(CSR_STIMECMP);
+#if __riscv_xlen < 64
+		context->stimecmph = csr_read(CSR_STIMECMPH);
+#endif
+	}
 	/*
 	 * No need to save/restore IP CSR (i.e. MIP or SIP) because:
 	 *
@@ -42,6 +48,13 @@ void suspend_restore_csrs(struct suspend_context *context)
 	csr_write(CSR_TVEC, context->tvec);
 	csr_write(CSR_IE, context->ie);
 
+	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SSTC)) {
+		csr_write(CSR_STIMECMP, context->stimecmp);
+#if __riscv_xlen < 64
+		csr_write(CSR_STIMECMPH, context->stimecmph);
+#endif
+	}
+
 #ifdef CONFIG_MMU
 	csr_write(CSR_SATP, context->satp);
 #endif
-- 
2.34.1

[PATCH 2/2] time-riscv: Stop stimecmp when cpu hotplug

[Nick Hu]

Stop the stimecmp when the cpu is going to be off otherwise the timer
interrupt may pending while performing power down operation.

Signed-off-by: Nick Hu <nick.hu@sifive.com>
---
 drivers/clocksource/timer-riscv.c | 22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
index 48ce50c5f5e6..9a6acaa8dfb0 100644
--- a/drivers/clocksource/timer-riscv.c
+++ b/drivers/clocksource/timer-riscv.c
@@ -32,15 +32,19 @@
 static DEFINE_STATIC_KEY_FALSE(riscv_sstc_available);
 static bool riscv_timer_cannot_wake_cpu;
 
+static void riscv_clock_stop_stimecmp(void)
+{
+	csr_write(CSR_STIMECMP, ULONG_MAX);
+	if (IS_ENABLED(CONFIG_32BIT))
+		csr_write(CSR_STIMECMPH, ULONG_MAX);
+}
+
 static void riscv_clock_event_stop(void)
 {
-	if (static_branch_likely(&riscv_sstc_available)) {
-		csr_write(CSR_STIMECMP, ULONG_MAX);
-		if (IS_ENABLED(CONFIG_32BIT))
-			csr_write(CSR_STIMECMPH, ULONG_MAX);
-	} else {
+	if (static_branch_likely(&riscv_sstc_available))
+		riscv_clock_stop_stimecmp();
+	else
 		sbi_set_timer(U64_MAX);
-	}
 }
 
 static int riscv_clock_next_event(unsigned long delta,
@@ -126,7 +130,11 @@ 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);
+	if (static_branch_likely(&riscv_sstc_available))
+		riscv_clock_stop_stimecmp();
+	else
+		disable_percpu_irq(riscv_clock_event_irq);
+
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH 1/2] riscv: Add stimecmp save and restore

[Anup Patel]

On Thu, Aug 29, 2024 at 9:09 AM Nick Hu <nick.hu@sifive.com> wrote:
>
> 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 |  4 ++++
>  arch/riscv/kernel/suspend.c      | 13 +++++++++++++
>  2 files changed, 17 insertions(+)
>
> diff --git a/arch/riscv/include/asm/suspend.h b/arch/riscv/include/asm/suspend.h
> index 4ffb022b097f..ffaac2efabb5 100644
> --- a/arch/riscv/include/asm/suspend.h
> +++ b/arch/riscv/include/asm/suspend.h
> @@ -16,6 +16,10 @@ struct suspend_context {
>         unsigned long envcfg;
>         unsigned long tvec;
>         unsigned long ie;
> +#if __riscv_xlen < 64
> +       unsigned long stimecmph;
> +#endif
> +       unsigned long stimecmp;
>  #ifdef CONFIG_MMU
>         unsigned long satp;
>  #endif
> diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c
> index c8cec0cc5833..3afd86e1abf7 100644
> --- a/arch/riscv/kernel/suspend.c
> +++ b/arch/riscv/kernel/suspend.c
> @@ -19,6 +19,12 @@ void suspend_save_csrs(struct suspend_context *context)
>         context->tvec = csr_read(CSR_TVEC);
>         context->ie = csr_read(CSR_IE);
>
> +       if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SSTC)) {
> +               context->stimecmp = csr_read(CSR_STIMECMP);
> +#if __riscv_xlen < 64
> +               context->stimecmph = csr_read(CSR_STIMECMPH);
> +#endif
> +       }

The suspend save/restore is enabled for the NoMMU kernel as well
(which runs in M-mode) so it is better to save/restore stimecmp CSR
only for MMU enabled kernels (just like satp CSR).

>         /*
>          * No need to save/restore IP CSR (i.e. MIP or SIP) because:
>          *
> @@ -42,6 +48,13 @@ void suspend_restore_csrs(struct suspend_context *context)
>         csr_write(CSR_TVEC, context->tvec);
>         csr_write(CSR_IE, context->ie);
>
> +       if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SSTC)) {
> +               csr_write(CSR_STIMECMP, context->stimecmp);
> +#if __riscv_xlen < 64
> +               csr_write(CSR_STIMECMPH, context->stimecmph);
> +#endif
> +       }
> +
>  #ifdef CONFIG_MMU
>         csr_write(CSR_SATP, context->satp);
>  #endif
> --
> 2.34.1
>
>

Regards,
Anup

Re: [PATCH 2/2] time-riscv: Stop stimecmp when cpu hotplug

[Anup Patel]

On Thu, Aug 29, 2024 at 9:10 AM Nick Hu <nick.hu@sifive.com> wrote:
>
> Stop the stimecmp when the cpu is going to be off otherwise the timer
> interrupt may pending while performing power down operation.
>
> Signed-off-by: Nick Hu <nick.hu@sifive.com>
> ---
>  drivers/clocksource/timer-riscv.c | 22 +++++++++++++++-------
>  1 file changed, 15 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
> index 48ce50c5f5e6..9a6acaa8dfb0 100644
> --- a/drivers/clocksource/timer-riscv.c
> +++ b/drivers/clocksource/timer-riscv.c
> @@ -32,15 +32,19 @@
>  static DEFINE_STATIC_KEY_FALSE(riscv_sstc_available);
>  static bool riscv_timer_cannot_wake_cpu;
>
> +static void riscv_clock_stop_stimecmp(void)
> +{
> +       csr_write(CSR_STIMECMP, ULONG_MAX);
> +       if (IS_ENABLED(CONFIG_32BIT))
> +               csr_write(CSR_STIMECMPH, ULONG_MAX);
> +}
> +
>  static void riscv_clock_event_stop(void)
>  {
> -       if (static_branch_likely(&riscv_sstc_available)) {
> -               csr_write(CSR_STIMECMP, ULONG_MAX);
> -               if (IS_ENABLED(CONFIG_32BIT))
> -                       csr_write(CSR_STIMECMPH, ULONG_MAX);
> -       } else {
> +       if (static_branch_likely(&riscv_sstc_available))
> +               riscv_clock_stop_stimecmp();
> +       else
>                 sbi_set_timer(U64_MAX);
> -       }
>  }
>
>  static int riscv_clock_next_event(unsigned long delta,
> @@ -126,7 +130,11 @@ 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);
> +       if (static_branch_likely(&riscv_sstc_available))
> +               riscv_clock_stop_stimecmp();
> +       else
> +               disable_percpu_irq(riscv_clock_event_irq);
> +

Not disabling riscv_clock_event_irq here for Sstc would now
cause riscv_timer_starting_cpu() to unnecessarily enable it
when the CPU is powered-up.

I think the below change is sufficient for this patch:

diff --git a/drivers/clocksource/timer-riscv.c
b/drivers/clocksource/timer-riscv.c
index 48ce50c5f5e6..546fd248f4ff 100644
--- a/drivers/clocksource/timer-riscv.c
+++ b/drivers/clocksource/timer-riscv.c
@@ -127,6 +127,11 @@ 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;
 }

Regards,
Anup

Re: [PATCH 1/2] riscv: Add stimecmp save and restore

[Nick Hu]

Hi Anup,

On Thu, Aug 29, 2024 at 1:18 PM Anup Patel <apatel@ventanamicro.com> wrote:
>
> On Thu, Aug 29, 2024 at 9:09 AM Nick Hu <nick.hu@sifive.com> wrote:
> >
> > 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 |  4 ++++
> >  arch/riscv/kernel/suspend.c      | 13 +++++++++++++
> >  2 files changed, 17 insertions(+)
> >
> > diff --git a/arch/riscv/include/asm/suspend.h b/arch/riscv/include/asm/suspend.h
> > index 4ffb022b097f..ffaac2efabb5 100644
> > --- a/arch/riscv/include/asm/suspend.h
> > +++ b/arch/riscv/include/asm/suspend.h
> > @@ -16,6 +16,10 @@ struct suspend_context {
> >         unsigned long envcfg;
> >         unsigned long tvec;
> >         unsigned long ie;
> > +#if __riscv_xlen < 64
> > +       unsigned long stimecmph;
> > +#endif
> > +       unsigned long stimecmp;
> >  #ifdef CONFIG_MMU
> >         unsigned long satp;
> >  #endif
> > diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c
> > index c8cec0cc5833..3afd86e1abf7 100644
> > --- a/arch/riscv/kernel/suspend.c
> > +++ b/arch/riscv/kernel/suspend.c
> > @@ -19,6 +19,12 @@ void suspend_save_csrs(struct suspend_context *context)
> >         context->tvec = csr_read(CSR_TVEC);
> >         context->ie = csr_read(CSR_IE);
> >
> > +       if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SSTC)) {
> > +               context->stimecmp = csr_read(CSR_STIMECMP);
> > +#if __riscv_xlen < 64
> > +               context->stimecmph = csr_read(CSR_STIMECMPH);
> > +#endif
> > +       }
>
> The suspend save/restore is enabled for the NoMMU kernel as well
> (which runs in M-mode) so it is better to save/restore stimecmp CSR
> only for MMU enabled kernels (just like satp CSR).
>
Good point. Will update that in the next version.
Thanks for the feedback.

> >         /*
> >          * No need to save/restore IP CSR (i.e. MIP or SIP) because:
> >          *
> > @@ -42,6 +48,13 @@ void suspend_restore_csrs(struct suspend_context *context)
> >         csr_write(CSR_TVEC, context->tvec);
> >         csr_write(CSR_IE, context->ie);
> >
> > +       if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SSTC)) {
> > +               csr_write(CSR_STIMECMP, context->stimecmp);
> > +#if __riscv_xlen < 64
> > +               csr_write(CSR_STIMECMPH, context->stimecmph);
> > +#endif
> > +       }
> > +
> >  #ifdef CONFIG_MMU
> >         csr_write(CSR_SATP, context->satp);
> >  #endif
> > --
> > 2.34.1
> >
> >
>
> Regards,
> Anup

Regards,
Nick

Re: [PATCH 2/2] time-riscv: Stop stimecmp when cpu hotplug

[Nick Hu]

Hi Anup

On Thu, Aug 29, 2024 at 1:18 PM Anup Patel <apatel@ventanamicro.com> wrote:
>
> On Thu, Aug 29, 2024 at 9:10 AM Nick Hu <nick.hu@sifive.com> wrote:
> >
> > Stop the stimecmp when the cpu is going to be off otherwise the timer
> > interrupt may pending while performing power down operation.
> >
> > Signed-off-by: Nick Hu <nick.hu@sifive.com>
> > ---
> >  drivers/clocksource/timer-riscv.c | 22 +++++++++++++++-------
> >  1 file changed, 15 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
> > index 48ce50c5f5e6..9a6acaa8dfb0 100644
> > --- a/drivers/clocksource/timer-riscv.c
> > +++ b/drivers/clocksource/timer-riscv.c
> > @@ -32,15 +32,19 @@
> >  static DEFINE_STATIC_KEY_FALSE(riscv_sstc_available);
> >  static bool riscv_timer_cannot_wake_cpu;
> >
> > +static void riscv_clock_stop_stimecmp(void)
> > +{
> > +       csr_write(CSR_STIMECMP, ULONG_MAX);
> > +       if (IS_ENABLED(CONFIG_32BIT))
> > +               csr_write(CSR_STIMECMPH, ULONG_MAX);
> > +}
> > +
> >  static void riscv_clock_event_stop(void)
> >  {
> > -       if (static_branch_likely(&riscv_sstc_available)) {
> > -               csr_write(CSR_STIMECMP, ULONG_MAX);
> > -               if (IS_ENABLED(CONFIG_32BIT))
> > -                       csr_write(CSR_STIMECMPH, ULONG_MAX);
> > -       } else {
> > +       if (static_branch_likely(&riscv_sstc_available))
> > +               riscv_clock_stop_stimecmp();
> > +       else
> >                 sbi_set_timer(U64_MAX);
> > -       }
> >  }
> >
> >  static int riscv_clock_next_event(unsigned long delta,
> > @@ -126,7 +130,11 @@ 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);
> > +       if (static_branch_likely(&riscv_sstc_available))
> > +               riscv_clock_stop_stimecmp();
> > +       else
> > +               disable_percpu_irq(riscv_clock_event_irq);
> > +
>
> Not disabling riscv_clock_event_irq here for Sstc would now
> cause riscv_timer_starting_cpu() to unnecessarily enable it
> when the CPU is powered-up.
>
> I think the below change is sufficient for this patch:
>
> diff --git a/drivers/clocksource/timer-riscv.c
> b/drivers/clocksource/timer-riscv.c
> index 48ce50c5f5e6..546fd248f4ff 100644
> --- a/drivers/clocksource/timer-riscv.c
> +++ b/drivers/clocksource/timer-riscv.c
> @@ -127,6 +127,11 @@ 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;
>  }
>
The sbi_exit() of OpenSBI will disable mtimecmp when
sbi_hsm_hart_stop() so the mtimecmp will be disabled twice if there is
no SSTC.
How about adding a SSTC available check before the riscv_clock_event_stop?

  /*
   * Stop the timer when the cpu is going to be offline otherwise
   * the timer interrupt may be pending while performing power-down.
   */
if (static_branch_likely(&riscv_sstc_available))
    riscv_clock_event_stop();

> Regards,
> Anup

Re: [PATCH 2/2] time-riscv: Stop stimecmp when cpu hotplug

[Anup Patel]

On Thu, Aug 29, 2024 at 11:53 AM Nick Hu <nick.hu@sifive.com> wrote:
>
> Hi Anup
>
> On Thu, Aug 29, 2024 at 1:18 PM Anup Patel <apatel@ventanamicro.com> wrote:
> >
> > On Thu, Aug 29, 2024 at 9:10 AM Nick Hu <nick.hu@sifive.com> wrote:
> > >
> > > Stop the stimecmp when the cpu is going to be off otherwise the timer
> > > interrupt may pending while performing power down operation.
> > >
> > > Signed-off-by: Nick Hu <nick.hu@sifive.com>
> > > ---
> > >  drivers/clocksource/timer-riscv.c | 22 +++++++++++++++-------
> > >  1 file changed, 15 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
> > > index 48ce50c5f5e6..9a6acaa8dfb0 100644
> > > --- a/drivers/clocksource/timer-riscv.c
> > > +++ b/drivers/clocksource/timer-riscv.c
> > > @@ -32,15 +32,19 @@
> > >  static DEFINE_STATIC_KEY_FALSE(riscv_sstc_available);
> > >  static bool riscv_timer_cannot_wake_cpu;
> > >
> > > +static void riscv_clock_stop_stimecmp(void)
> > > +{
> > > +       csr_write(CSR_STIMECMP, ULONG_MAX);
> > > +       if (IS_ENABLED(CONFIG_32BIT))
> > > +               csr_write(CSR_STIMECMPH, ULONG_MAX);
> > > +}
> > > +
> > >  static void riscv_clock_event_stop(void)
> > >  {
> > > -       if (static_branch_likely(&riscv_sstc_available)) {
> > > -               csr_write(CSR_STIMECMP, ULONG_MAX);
> > > -               if (IS_ENABLED(CONFIG_32BIT))
> > > -                       csr_write(CSR_STIMECMPH, ULONG_MAX);
> > > -       } else {
> > > +       if (static_branch_likely(&riscv_sstc_available))
> > > +               riscv_clock_stop_stimecmp();
> > > +       else
> > >                 sbi_set_timer(U64_MAX);
> > > -       }
> > >  }
> > >
> > >  static int riscv_clock_next_event(unsigned long delta,
> > > @@ -126,7 +130,11 @@ 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);
> > > +       if (static_branch_likely(&riscv_sstc_available))
> > > +               riscv_clock_stop_stimecmp();
> > > +       else
> > > +               disable_percpu_irq(riscv_clock_event_irq);
> > > +
> >
> > Not disabling riscv_clock_event_irq here for Sstc would now
> > cause riscv_timer_starting_cpu() to unnecessarily enable it
> > when the CPU is powered-up.
> >
> > I think the below change is sufficient for this patch:
> >
> > diff --git a/drivers/clocksource/timer-riscv.c
> > b/drivers/clocksource/timer-riscv.c
> > index 48ce50c5f5e6..546fd248f4ff 100644
> > --- a/drivers/clocksource/timer-riscv.c
> > +++ b/drivers/clocksource/timer-riscv.c
> > @@ -127,6 +127,11 @@ 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;
> >  }
> >
> The sbi_exit() of OpenSBI will disable mtimecmp when
> sbi_hsm_hart_stop() so the mtimecmp will be disabled twice if there is
> no SSTC.
> How about adding a SSTC available check before the riscv_clock_event_stop?

Currently, OpenSBI uses mtimecmp only for implementing
SBI time extension but in the future, OpenSBI might implement
a per-hart timer event list to allow M-mode timer events.

Don't assume in what environment the driver is running.

It is possible that the driver is running under a hypervisor
which only provides SBI time extension and does not virtualize
Sstc extension.

>
>   /*
>    * Stop the timer when the cpu is going to be offline otherwise
>    * the timer interrupt may be pending while performing power-down.
>    */
> if (static_branch_likely(&riscv_sstc_available))
>     riscv_clock_event_stop();
>

Regards,
Anup

Re: [PATCH 1/2] riscv: Add stimecmp save and restore

[Andrew Jones]

On Thu, Aug 29, 2024 at 11:38:59AM GMT, Nick Hu wrote:
> 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 |  4 ++++
>  arch/riscv/kernel/suspend.c      | 13 +++++++++++++
>  2 files changed, 17 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/suspend.h b/arch/riscv/include/asm/suspend.h
> index 4ffb022b097f..ffaac2efabb5 100644
> --- a/arch/riscv/include/asm/suspend.h
> +++ b/arch/riscv/include/asm/suspend.h
> @@ -16,6 +16,10 @@ struct suspend_context {
>  	unsigned long envcfg;
>  	unsigned long tvec;
>  	unsigned long ie;
> +#if __riscv_xlen < 64
> +	unsigned long stimecmph;
> +#endif

I'm not sure the reduction in struct size is worth the #ifdeffery. If we
just always add stimecmph, then we can also change the #ifdef's below to
if's, i.e. if (__riscv_xlen < 64), which should still remove the code from
64-bit builds.

Or maybe we need something like

#if __riscv_xlen < 64
#define csrh_write(r, v) csr_write(r, v)
#else
#define csrh_write(r, v)
#endif

in asm/csr.h and then use it for all the *h csrs, but keep the #if in
the struct.

Thanks,
drew

> +	unsigned long stimecmp;
>  #ifdef CONFIG_MMU
>  	unsigned long satp;
>  #endif
> diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c
> index c8cec0cc5833..3afd86e1abf7 100644
> --- a/arch/riscv/kernel/suspend.c
> +++ b/arch/riscv/kernel/suspend.c
> @@ -19,6 +19,12 @@ void suspend_save_csrs(struct suspend_context *context)
>  	context->tvec = csr_read(CSR_TVEC);
>  	context->ie = csr_read(CSR_IE);
>  
> +	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SSTC)) {
> +		context->stimecmp = csr_read(CSR_STIMECMP);
> +#if __riscv_xlen < 64
> +		context->stimecmph = csr_read(CSR_STIMECMPH);
> +#endif
> +	}
>  	/*
>  	 * No need to save/restore IP CSR (i.e. MIP or SIP) because:
>  	 *
> @@ -42,6 +48,13 @@ void suspend_restore_csrs(struct suspend_context *context)
>  	csr_write(CSR_TVEC, context->tvec);
>  	csr_write(CSR_IE, context->ie);
>  
> +	if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SSTC)) {
> +		csr_write(CSR_STIMECMP, context->stimecmp);
> +#if __riscv_xlen < 64
> +		csr_write(CSR_STIMECMPH, context->stimecmph);
> +#endif
> +	}
> +
>  #ifdef CONFIG_MMU
>  	csr_write(CSR_SATP, context->satp);
>  #endif
> -- 
> 2.34.1
> 

Re: [PATCH 2/2] time-riscv: Stop stimecmp when cpu hotplug

[Thomas Gleixner]

On Thu, Aug 29 2024 at 11:39, Nick Hu wrote:

clocksource/drivers/timer-riscv: is the proper prefix

Thanks,

        tglx
        

Re: [PATCH 1/2] riscv: Add stimecmp save and restore

[Nick Hu]

Hi Andrew

On Thu, Aug 29, 2024 at 3:59 PM Andrew Jones <ajones@ventanamicro.com> wrote:
>
> On Thu, Aug 29, 2024 at 11:38:59AM GMT, Nick Hu wrote:
> > 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 |  4 ++++
> >  arch/riscv/kernel/suspend.c      | 13 +++++++++++++
> >  2 files changed, 17 insertions(+)
> >
> > diff --git a/arch/riscv/include/asm/suspend.h b/arch/riscv/include/asm/suspend.h
> > index 4ffb022b097f..ffaac2efabb5 100644
> > --- a/arch/riscv/include/asm/suspend.h
> > +++ b/arch/riscv/include/asm/suspend.h
> > @@ -16,6 +16,10 @@ struct suspend_context {
> >       unsigned long envcfg;
> >       unsigned long tvec;
> >       unsigned long ie;
> > +#if __riscv_xlen < 64
> > +     unsigned long stimecmph;
> > +#endif
>
> I'm not sure the reduction in struct size is worth the #ifdeffery. If we
> just always add stimecmph, then we can also change the #ifdef's below to
> if's, i.e. if (__riscv_xlen < 64), which should still remove the code from
> 64-bit builds.
>
> Or maybe we need something like
>
> #if __riscv_xlen < 64
> #define csrh_write(r, v) csr_write(r, v)
> #else
> #define csrh_write(r, v)
> #endif
>
> in asm/csr.h and then use it for all the *h csrs, but keep the #if in
> the struct.
>
> Thanks,
> drew
>
If no other comment, I'll choose the csrh_write() because it can save
some memory and update in the next version.
Thanks for the suggestion!

> > +     unsigned long stimecmp;
> >  #ifdef CONFIG_MMU
> >       unsigned long satp;
> >  #endif
> > diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c
> > index c8cec0cc5833..3afd86e1abf7 100644
> > --- a/arch/riscv/kernel/suspend.c
> > +++ b/arch/riscv/kernel/suspend.c
> > @@ -19,6 +19,12 @@ void suspend_save_csrs(struct suspend_context *context)
> >       context->tvec = csr_read(CSR_TVEC);
> >       context->ie = csr_read(CSR_IE);
> >
> > +     if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SSTC)) {
> > +             context->stimecmp = csr_read(CSR_STIMECMP);
> > +#if __riscv_xlen < 64
> > +             context->stimecmph = csr_read(CSR_STIMECMPH);
> > +#endif
> > +     }
> >       /*
> >        * No need to save/restore IP CSR (i.e. MIP or SIP) because:
> >        *
> > @@ -42,6 +48,13 @@ void suspend_restore_csrs(struct suspend_context *context)
> >       csr_write(CSR_TVEC, context->tvec);
> >       csr_write(CSR_IE, context->ie);
> >
> > +     if (riscv_has_extension_unlikely(RISCV_ISA_EXT_SSTC)) {
> > +             csr_write(CSR_STIMECMP, context->stimecmp);
> > +#if __riscv_xlen < 64
> > +             csr_write(CSR_STIMECMPH, context->stimecmph);
> > +#endif
> > +     }
> > +
> >  #ifdef CONFIG_MMU
> >       csr_write(CSR_SATP, context->satp);
> >  #endif
> > --
> > 2.34.1
> >

Regards,
Nick

Re: [PATCH 2/2] time-riscv: Stop stimecmp when cpu hotplug

[Nick Hu]

Hi Anup

On Thu, Aug 29, 2024 at 2:49 PM Anup Patel <apatel@ventanamicro.com> wrote:
>
> On Thu, Aug 29, 2024 at 11:53 AM Nick Hu <nick.hu@sifive.com> wrote:
> >
> > Hi Anup
> >
> > On Thu, Aug 29, 2024 at 1:18 PM Anup Patel <apatel@ventanamicro.com> wrote:
> > >
> > > On Thu, Aug 29, 2024 at 9:10 AM Nick Hu <nick.hu@sifive.com> wrote:
> > > >
> > > > Stop the stimecmp when the cpu is going to be off otherwise the timer
> > > > interrupt may pending while performing power down operation.
> > > >
> > > > Signed-off-by: Nick Hu <nick.hu@sifive.com>
> > > > ---
> > > >  drivers/clocksource/timer-riscv.c | 22 +++++++++++++++-------
> > > >  1 file changed, 15 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c
> > > > index 48ce50c5f5e6..9a6acaa8dfb0 100644
> > > > --- a/drivers/clocksource/timer-riscv.c
> > > > +++ b/drivers/clocksource/timer-riscv.c
> > > > @@ -32,15 +32,19 @@
> > > >  static DEFINE_STATIC_KEY_FALSE(riscv_sstc_available);
> > > >  static bool riscv_timer_cannot_wake_cpu;
> > > >
> > > > +static void riscv_clock_stop_stimecmp(void)
> > > > +{
> > > > +       csr_write(CSR_STIMECMP, ULONG_MAX);
> > > > +       if (IS_ENABLED(CONFIG_32BIT))
> > > > +               csr_write(CSR_STIMECMPH, ULONG_MAX);
> > > > +}
> > > > +
> > > >  static void riscv_clock_event_stop(void)
> > > >  {
> > > > -       if (static_branch_likely(&riscv_sstc_available)) {
> > > > -               csr_write(CSR_STIMECMP, ULONG_MAX);
> > > > -               if (IS_ENABLED(CONFIG_32BIT))
> > > > -                       csr_write(CSR_STIMECMPH, ULONG_MAX);
> > > > -       } else {
> > > > +       if (static_branch_likely(&riscv_sstc_available))
> > > > +               riscv_clock_stop_stimecmp();
> > > > +       else
> > > >                 sbi_set_timer(U64_MAX);
> > > > -       }
> > > >  }
> > > >
> > > >  static int riscv_clock_next_event(unsigned long delta,
> > > > @@ -126,7 +130,11 @@ 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);
> > > > +       if (static_branch_likely(&riscv_sstc_available))
> > > > +               riscv_clock_stop_stimecmp();
> > > > +       else
> > > > +               disable_percpu_irq(riscv_clock_event_irq);
> > > > +
> > >
> > > Not disabling riscv_clock_event_irq here for Sstc would now
> > > cause riscv_timer_starting_cpu() to unnecessarily enable it
> > > when the CPU is powered-up.
> > >
> > > I think the below change is sufficient for this patch:
> > >
> > > diff --git a/drivers/clocksource/timer-riscv.c
> > > b/drivers/clocksource/timer-riscv.c
> > > index 48ce50c5f5e6..546fd248f4ff 100644
> > > --- a/drivers/clocksource/timer-riscv.c
> > > +++ b/drivers/clocksource/timer-riscv.c
> > > @@ -127,6 +127,11 @@ 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;
> > >  }
> > >
> > The sbi_exit() of OpenSBI will disable mtimecmp when
> > sbi_hsm_hart_stop() so the mtimecmp will be disabled twice if there is
> > no SSTC.
> > How about adding a SSTC available check before the riscv_clock_event_stop?
>
> Currently, OpenSBI uses mtimecmp only for implementing
> SBI time extension but in the future, OpenSBI might implement
> a per-hart timer event list to allow M-mode timer events.
>
> Don't assume in what environment the driver is running.
>
> It is possible that the driver is running under a hypervisor
> which only provides SBI time extension and does not virtualize
> Sstc extension.
>
I see, Thank you for the explanation!

> >
> >   /*
> >    * Stop the timer when the cpu is going to be offline otherwise
> >    * the timer interrupt may be pending while performing power-down.
> >    */
> > if (static_branch_likely(&riscv_sstc_available))
> >     riscv_clock_event_stop();
> >
>
I'll update this one in the next version.

> Regards,
> Anup

Regards,
Nick

Re: [PATCH 2/2] time-riscv: Stop stimecmp when cpu hotplug

[Nick Hu]

Hi Thomas

On Thu, Aug 29, 2024 at 9:43 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Thu, Aug 29 2024 at 11:39, Nick Hu wrote:
>
> clocksource/drivers/timer-riscv: is the proper prefix
>
> Thanks,
>
>         tglx
>
Thanks, I'll update it in the next version

Regards,
Nick