mbox series

[0/2] Support SSTC while PM operations

Message ID 20240829033904.477200-1-nick.hu@sifive.com
Headers show
Series Support SSTC while PM operations | expand

Message

Nick Hu Aug. 29, 2024, 3:38 a.m. UTC 
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(-)

Comments

Anup Patel Aug. 29, 2024, 5:18 a.m. UTC | #1 
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
Nick Hu Aug. 29, 2024, 6:16 a.m. UTC | #2 
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
Andrew Jones Aug. 29, 2024, 7:59 a.m. UTC | #3 
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
>
Nick Hu Aug. 30, 2024, 5:53 a.m. UTC | #4 
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