| Message ID | 20240829033904.477200-3-nick.hu@sifive.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | Support SSTC while PM operations | expand |
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
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
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
On Thu, Aug 29 2024 at 11:39, Nick Hu wrote:
clocksource/drivers/timer-riscv: is the proper prefix
Thanks,
tglx
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
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
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; }
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(-)