Received-SPF: pass (google.com: domain of linux-kernel+bounces-147834-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Tue, 16 Apr 2024 18:49:51 -0700 (PDT)
Subject:     Re: [PATCH] clocksource/drivers/timer-riscv: Drop extra CSR write
In-Reply-To: <CAK9=C2Vsq0iN5FW=zC7+2GHe115TPJOEEPcDCLs_40yBGpbVdw@mail.gmail.com>
CC: samuel.holland@sifive.com, daniel.lezcano@linaro.org, tglx@linutronix.de,
  aou@eecs.berkeley.edu, Paul Walmsley <paul.walmsley@sifive.com>, linux-kernel@vger.kernel.org,
  linux-riscv@lists.infradead.org
From: Palmer Dabbelt <palmer@dabbelt.com>
To: apatel@ventanamicro.com
Message-ID: <mhng-3a5fcdd0-0bf4-45d3-aa7d-fffddd852288@palmer-ri-x1c9a>
Precedence: bulk
Mime-Version: 1.0 (MHng)
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit

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
>>
>>