Re: [PATCH] clocksource/drivers/riscv: Increase the clock source rating

[Samuel Holland]

Hi Palmer,

On 12/29/22 10:22, Palmer Dabbelt wrote:
> On Tue, 27 Dec 2022 16:44:44 PST (-0800), samuel@xxxxxxxxxxxx wrote:
>> RISC-V provides an architectural clock source via the time CSR. This
>> clock source exposes a 64-bit counter synchronized across all CPUs.
>> Because it is accessed using a CSR, it is much more efficient to read
>> than MMIO clock sources. For example, on the Allwinner D1, reading the
>> sun4i timer in a loop takes 131 cycles/iteration, while reading the
>> RISC-V time CSR takes only 5 cycles/iteration.
>>
>> Adjust the RISC-V clock source rating so it is preferred over the
>> various platform-specific MMIO clock sources.
>>
>> Signed-off-by: Samuel Holland <samuel@xxxxxxxxxxxx>
>> ---
>>
>>  drivers/clocksource/timer-riscv.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clocksource/timer-riscv.c
>> b/drivers/clocksource/timer-riscv.c
>> index a0d66fabf073..55dad7965f43 100644
>> --- a/drivers/clocksource/timer-riscv.c
>> +++ b/drivers/clocksource/timer-riscv.c
>> @@ -73,7 +73,7 @@ static u64 notrace riscv_sched_clock(void)
>>
>>  static struct clocksource riscv_clocksource = {
>>      .name        = "riscv_clocksource",
>> -    .rating        = 300,
>> +    .rating        = 400,
>>      .mask        = CLOCKSOURCE_MASK(64),
>>      .flags        = CLOCK_SOURCE_IS_CONTINUOUS,
>>      .read        = riscv_clocksource_rdtime,
> 
> I've never really understood what we're supposed to do here, it seems
> like we're just picking arbitrary ratings for the various clock drivers
> to get the one we want.  That's really a property of the whole platform,
> though, not the drivers, so trying to encode it as part of the driver
> seems awkward -- if anything I'd expect the ISA clock drivers to be the
> worst on any platform, as otherwise what's the point of adding the
> platform-specific mechanism?

For sunxi at least, the platform-specific MMIO timers came first, as the
ARM architectural timer was not implemented in the first couple of SoCs.
The ARM architectural timer was universally better for timekeeping
(percpu events, faster access, larger range), so we completely ignored
the MMIO timers once it was available.

We eventually went back and enabled the MMIO timers everywhere because
they are useful for PM. Either they don't have CLOCK_EVT_FEAT_C3STOP, or
they do have CLOCK_SOURCE_SUSPEND_NONSTOP, or both.

I imagine the RISC-V situation will be similar. The time CSR will be
used as the primary clocksource and for sched_clock, while MMIO clock
sources will only be used in specific PM scenarios where the time CSR is
unavailable.

Plus there is the distinction between clock sources and clock events.
Drivers usually provide both, but there is no requirement that both be
used. The RISC-V architecture provides a high quality clock source (time
CSR) but a poor quality clock event (SBI call) unless Sstc is available.
So platforms without Sstc may use a combination of drivers: the RISC-V
clock source along with a platform-specific clock event.

> That said, I'm fine with this as long as it's improving things on the
> platforms that actually exist.  IIUC it's only the D1 that has multiple
> clock drivers currently, so if it's good there it's good for me.  We'll

RZ/Five also has a platform MMIO timer driver[1].

[1]:
https://lore.kernel.org/lkml/20221211215843.24024-1-prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx/

> go crazy trying to reason about all possible future hardware, so we can
> just sort out how to make stuff work as it shows up.  So:
> 
> Acked-by: Palmer Dabbelt <palmer@xxxxxxxxxxxx>
> Reviewed-by: Palmer Dabbelt <palmer@xxxxxxxxxxxx>
> 
> I'll let the clock folks chime in, happy to take it through the RISC-V
> tree but unless someone says something I'm going to assume it's aimed
> over there.

Thanks for the review!

Regards,
Samuel