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

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

Posted by Samuel Holland 2 years, 8 months ago

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@sholland.org>
---

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

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

Posted by Daniel Lezcano 2 years, 8 months ago

On 28/12/2022 01:44, Samuel Holland 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@sholland.org>
> ---
Applied, thanks

-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

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

Posted by Palmer Dabbelt 2 years, 8 months ago

On Tue, 27 Dec 2022 16:44:44 PST (-0800), samuel@sholland.org 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@sholland.org>
> ---
>
>  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?

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 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@rivosinc.com>
Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>

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!

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

Posted by Samuel Holland 2 years, 8 months ago

Hi Palmer,

On 12/29/22 10:22, Palmer Dabbelt wrote:
> On Tue, 27 Dec 2022 16:44:44 PST (-0800), samuel@sholland.org 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@sholland.org>
>> ---
>>
>>  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@bp.renesas.com/

> 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@rivosinc.com>
> Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
> 
> 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

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

Posted by Lad, Prabhakar 2 years, 8 months ago

Hi Samuel,

Thank you for the patch.

On Wed, Dec 28, 2022 at 12:44 AM Samuel Holland <samuel@sholland.org> 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@sholland.org>
> ---
>
>  drivers/clocksource/timer-riscv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Cheers,
Prabhakar

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

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

Posted by Anup Patel 2 years, 8 months ago

On Wed, Dec 28, 2022 at 6:14 AM Samuel Holland <samuel@sholland.org> 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@sholland.org>

Looks good to me.

Reviewed-by: Anup Patel <anup@brainfault.org>

Regards,
Anup

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

[tip: timers/core] clocksource/drivers/riscv: Increase the clock source rating

Posted by tip-bot2 for Samuel Holland 2 years, 7 months ago

The following commit has been merged into the timers/core branch of tip:

Commit-ID:     674402b0098b66b8ba91fe93c0d27af703256098
Gitweb:        https://git.kernel.org/tip/674402b0098b66b8ba91fe93c0d27af703256098
Author:        Samuel Holland <samuel@sholland.org>
AuthorDate:    Tue, 27 Dec 2022 18:44:44 -06:00
Committer:     Daniel Lezcano <daniel.lezcano@linaro.org>
CommitterDate: Mon, 13 Feb 2023 13:10:16 +01:00

clocksource/drivers/riscv: Increase the clock source rating

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@sholland.org>
Acked-by: Palmer Dabbelt <palmer@rivosinc.com>
Reviewed-by: Palmer Dabbelt <palmer@rivosinc.com>
Reviewed-by: Anup Patel <anup@brainfault.org>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Link: https://lore.kernel.org/r/20221228004444.61568-1-samuel@sholland.org
Signed-off-by: Daniel Lezcano <daniel.lezcano@kernel.org>
---
 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 1b4b36d..adf7f98 100644
--- a/drivers/clocksource/timer-riscv.c
+++ b/drivers/clocksource/timer-riscv.c
@@ -74,7 +74,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,