Re: [PATCH] clocksource/drivers/timer-riscv: Drop extra CSR write

[Palmer Dabbelt]

On Wed, 13 Mar 2024 09:56:34 PDT (-0700), apatel@xxxxxxxxxxxxxxxx wrote:
> On Wed, Mar 13, 2024 at 1:03 AM Samuel Holland
> <samuel.holland@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >
> >  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