| Message ID | 20240312193306.1814593-1-samuel.holland@sifive.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | clocksource/drivers/timer-riscv: Drop extra CSR write | expand |
On Wed, Mar 13, 2024 at 1:03 AM Samuel Holland <samuel.holland@sifive.com> wrote: > > On riscv32, the time comparator value is split across two CSRs. We write > both when stopping the timer, but realistically the time is just as > unlikely to reach 0xffffffff00000000 as 0xffffffffffffffff, so there is > no need to write the low CSR. Even though unlikely, there is still a theoretical possibility of counter reaching value 0xffffffff00000000. The good thing about value 0xffffffffffffffff is that the counter will immediately wrap around after reaching it. Regards, Anup > > Signed-off-by: Samuel Holland <samuel.holland@sifive.com> > --- > > drivers/clocksource/timer-riscv.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c > index e66dcbd66566..eaaf01f3c34b 100644 > --- a/drivers/clocksource/timer-riscv.c > +++ b/drivers/clocksource/timer-riscv.c > @@ -35,9 +35,10 @@ static bool riscv_timer_cannot_wake_cpu; > 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 > + csr_write(CSR_STIMECMP, ULONG_MAX); > } else { > sbi_set_timer(U64_MAX); > } > -- > 2.43.1 > >
On Wed, 13 Mar 2024 09:56:34 PDT (-0700), apatel@ventanamicro.com wrote: > On Wed, Mar 13, 2024 at 1:03 AM Samuel Holland > <samuel.holland@sifive.com> wrote: >> >> On riscv32, the time comparator value is split across two CSRs. We write >> both when stopping the timer, but realistically the time is just as >> unlikely to reach 0xffffffff00000000 as 0xffffffffffffffff, so there is >> no need to write the low CSR. > > Even though unlikely, there is still a theoretical possibility of > counter reaching value 0xffffffff00000000. I guess that depends on the timebase frequency, but if my math is right then we've got some timers that will overflow a 32-bit counter in 10 minutes -- take that with a grain of salt as they're all 64-bit systems (we don't have any 32-bit DTs upstream?), but it still seems plausible for 32-bit overflows to happen here on real systems. > The good thing about value 0xffffffffffffffff is that the counter will > immediately wrap around after reaching it. I'm not sure how that's good? Luckily we've got ~100,000 years to figure out, even on these systems with pretty fast timers ;) > Regards, > Anup > > >> >> Signed-off-by: Samuel Holland <samuel.holland@sifive.com> >> --- >> >> drivers/clocksource/timer-riscv.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c >> index e66dcbd66566..eaaf01f3c34b 100644 >> --- a/drivers/clocksource/timer-riscv.c >> +++ b/drivers/clocksource/timer-riscv.c >> @@ -35,9 +35,10 @@ static bool riscv_timer_cannot_wake_cpu; >> 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 >> + csr_write(CSR_STIMECMP, ULONG_MAX); >> } else { >> sbi_set_timer(U64_MAX); >> } >> -- >> 2.43.1 >> >>
On Wed, Apr 17, 2024 at 7:20 AM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > On Wed, 13 Mar 2024 09:56:34 PDT (-0700), apatel@ventanamicro.com wrote: > > On Wed, Mar 13, 2024 at 1:03 AM Samuel Holland > > <samuel.holland@sifive.com> wrote: > >> > >> On riscv32, the time comparator value is split across two CSRs. We write > >> both when stopping the timer, but realistically the time is just as > >> unlikely to reach 0xffffffff00000000 as 0xffffffffffffffff, so there is > >> no need to write the low CSR. > > > > Even though unlikely, there is still a theoretical possibility of > > counter reaching value 0xffffffff00000000. > > I guess that depends on the timebase frequency, but if my math is right > then we've got some timers that will overflow a 32-bit counter in 10 > minutes -- take that with a grain of salt as they're all 64-bit systems > (we don't have any 32-bit DTs upstream?), but it still seems plausible > for 32-bit overflows to happen here on real systems. > > > The good thing about value 0xffffffffffffffff is that the counter will > > immediately wrap around after reaching it. > > I'm not sure how that's good? Luckily we've got ~100,000 years to > figure out, even on these systems with pretty fast timers ;) The RISC-V server soc spec mandates timer frequency to be at least 100 MHz so we can certainly have 100MHz as the timer frequency and based on this time CSR will overflow in roughly 5840 years. Also, nothing stops a SoC vendor from having a GHz timer frequency. In addition to timer frequency, changing the initial value of time CSR can also impact when the time CSR overflows. This particularly applies to virtualization because KVM RISC-V allows the KVM user space tool to set the desired initial value of time CSR visible to the Guest/VM. For example, if the timer frequency is 10MHz and KVM user space tool sets initial time CSR value to 0xffffffff_00000000 then Guest/VM time CSR will overflow in roughly 7 mins. Regards, Anup > > > Regards, > > Anup > > > > > >> > >> Signed-off-by: Samuel Holland <samuel.holland@sifive.com> > >> --- > >> > >> drivers/clocksource/timer-riscv.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c > >> index e66dcbd66566..eaaf01f3c34b 100644 > >> --- a/drivers/clocksource/timer-riscv.c > >> +++ b/drivers/clocksource/timer-riscv.c > >> @@ -35,9 +35,10 @@ static bool riscv_timer_cannot_wake_cpu; > >> 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 > >> + csr_write(CSR_STIMECMP, ULONG_MAX); > >> } else { > >> sbi_set_timer(U64_MAX); > >> } > >> -- > >> 2.43.1 > >> > >> >
diff --git a/drivers/clocksource/timer-riscv.c b/drivers/clocksource/timer-riscv.c index e66dcbd66566..eaaf01f3c34b 100644 --- a/drivers/clocksource/timer-riscv.c +++ b/drivers/clocksource/timer-riscv.c @@ -35,9 +35,10 @@ static bool riscv_timer_cannot_wake_cpu; 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 + csr_write(CSR_STIMECMP, ULONG_MAX); } else { sbi_set_timer(U64_MAX); }
On riscv32, the time comparator value is split across two CSRs. We write both when stopping the timer, but realistically the time is just as unlikely to reach 0xffffffff00000000 as 0xffffffffffffffff, so there is no need to write the low CSR. Signed-off-by: Samuel Holland <samuel.holland@sifive.com> --- drivers/clocksource/timer-riscv.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)