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 <[email protected]>
---
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
On Wed, Dec 28, 2022 at 6:14 AM Samuel Holland <[email protected]> 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 <[email protected]>
Looks good to me.
Reviewed-by: Anup Patel <[email protected]>
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
>
Hi Samuel,
Thank you for the patch.
On Wed, Dec 28, 2022 at 12:44 AM Samuel Holland <[email protected]> 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 <[email protected]>
> ---
>
> drivers/clocksource/timer-riscv.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
Reviewed-by: Lad Prabhakar <[email protected]>
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
>
On Tue, 27 Dec 2022 16:44:44 PST (-0800), [email protected] 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 <[email protected]>
> ---
>
> 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 <[email protected]>
Reviewed-by: Palmer Dabbelt <[email protected]>
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!
Hi Palmer,
On 12/29/22 10:22, Palmer Dabbelt wrote:
> On Tue, 27 Dec 2022 16:44:44 PST (-0800), [email protected] 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 <[email protected]>
>> ---
>>
>> 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/[email protected]/
> 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 <[email protected]>
> Reviewed-by: Palmer Dabbelt <[email protected]>
>
> 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
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 <[email protected]>
> ---
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
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 <[email protected]>
AuthorDate: Tue, 27 Dec 2022 18:44:44 -06:00
Committer: Daniel Lezcano <[email protected]>
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 <[email protected]>
Acked-by: Palmer Dabbelt <[email protected]>
Reviewed-by: Palmer Dabbelt <[email protected]>
Reviewed-by: Anup Patel <[email protected]>
Reviewed-by: Lad Prabhakar <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
Signed-off-by: Daniel Lezcano <[email protected]>
---
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,