[PATCH] clocksource/drivers/riscv: Increase the clock_event rating

[PATCH] clocksource/drivers/riscv: Increase the clock_event rating

[Prabhakar]

From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Renesas RZ/Five SoC has OSTM blocks which can be used for clock_event and
clocksource [0]. The clock_event rating for the OSTM is set 300 but
whereas the rating for riscv-timer clock_event is set to 100 due to which
the kernel is choosing OSTM for clock_event.

As riscv-timer is much more efficient than MMIO clock_event, increase the
rating to 400 so that the kernel prefers riscv-timer over the MMIO based
clock_event.

[0] drivers/clocksource/renesas-ostm.c

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
Note, Ive set the rating similar to RISC-V clocksource, on ARM architecture
the rating for clk_event is set to 450.
---
 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 da3071b387eb..e4fc5da119a2 100644
--- a/drivers/clocksource/timer-riscv.c
+++ b/drivers/clocksource/timer-riscv.c
@@ -54,7 +54,7 @@ static unsigned int riscv_clock_event_irq;
 static DEFINE_PER_CPU(struct clock_event_device, riscv_clock_event) = {
 	.name			= "riscv_timer_clockevent",
 	.features		= CLOCK_EVT_FEAT_ONESHOT,
-	.rating			= 100,
+	.rating			= 400,
 	.set_next_event		= riscv_clock_next_event,
 };
 
-- 
2.34.1

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

[Geert Uytterhoeven]

On Thu, Sep 28, 2023 at 12:45 PM Prabhakar <prabhakar.csengg@gmail.com> wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>
> Renesas RZ/Five SoC has OSTM blocks which can be used for clock_event and
> clocksource [0]. The clock_event rating for the OSTM is set 300 but
> whereas the rating for riscv-timer clock_event is set to 100 due to which
> the kernel is choosing OSTM for clock_event.
>
> As riscv-timer is much more efficient than MMIO clock_event, increase the
> rating to 400 so that the kernel prefers riscv-timer over the MMIO based
> clock_event.
>
> [0] drivers/clocksource/renesas-ostm.c
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> Note, Ive set the rating similar to RISC-V clocksource, on ARM architecture
> the rating for clk_event is set to 450.

Makes sense.
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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

[Samuel Holland]

On 2023-09-28 5:45 AM, Prabhakar wrote:
> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> 
> Renesas RZ/Five SoC has OSTM blocks which can be used for clock_event and
> clocksource [0]. The clock_event rating for the OSTM is set 300 but
> whereas the rating for riscv-timer clock_event is set to 100 due to which
> the kernel is choosing OSTM for clock_event.
> 
> As riscv-timer is much more efficient than MMIO clock_event, increase the
> rating to 400 so that the kernel prefers riscv-timer over the MMIO based
> clock_event.

This is only true if you have the Sstc extension and can set stimecmp directly.
Otherwise you have the overhead of an SBI call, which is going to be much higher
than an MMIO write. So the rating should depend on Sstc, as in this patch:

https://lore.kernel.org/linux-riscv/20230710131902.1459180-3-apatel@ventanamicro.com/

Regards,
Samuel

> 
> [0] drivers/clocksource/renesas-ostm.c
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
> Note, Ive set the rating similar to RISC-V clocksource, on ARM architecture
> the rating for clk_event is set to 450.
> ---
>  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 da3071b387eb..e4fc5da119a2 100644
> --- a/drivers/clocksource/timer-riscv.c
> +++ b/drivers/clocksource/timer-riscv.c
> @@ -54,7 +54,7 @@ static unsigned int riscv_clock_event_irq;
>  static DEFINE_PER_CPU(struct clock_event_device, riscv_clock_event) = {
>  	.name			= "riscv_timer_clockevent",
>  	.features		= CLOCK_EVT_FEAT_ONESHOT,
> -	.rating			= 100,
> +	.rating			= 400,
>  	.set_next_event		= riscv_clock_next_event,
>  };
>  

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

[Lad, Prabhakar]

Hi Samuel,

On Thu, Sep 28, 2023 at 5:04 PM Samuel Holland
<samuel.holland@sifive.com> wrote:
>
> On 2023-09-28 5:45 AM, Prabhakar wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> >
> > Renesas RZ/Five SoC has OSTM blocks which can be used for clock_event and
> > clocksource [0]. The clock_event rating for the OSTM is set 300 but
> > whereas the rating for riscv-timer clock_event is set to 100 due to which
> > the kernel is choosing OSTM for clock_event.
> >
> > As riscv-timer is much more efficient than MMIO clock_event, increase the
> > rating to 400 so that the kernel prefers riscv-timer over the MMIO based
> > clock_event.
>
> This is only true if you have the Sstc extension and can set stimecmp directly.
> Otherwise you have the overhead of an SBI call, which is going to be much higher
> than an MMIO write. So the rating should depend on Sstc, as in this patch:
>
> https://lore.kernel.org/linux-riscv/20230710131902.1459180-3-apatel@ventanamicro.com/
>
Thank you for the pointer. Do you know any tool/util which I can use
to make comparisons?

Cheers,
Prabhakar

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

[Samuel Holland]

Hi Prabhakar,

On 2023-09-28 11:18 AM, Lad, Prabhakar wrote:
> On Thu, Sep 28, 2023 at 5:04 PM Samuel Holland
> <samuel.holland@sifive.com> wrote:
>>
>> On 2023-09-28 5:45 AM, Prabhakar wrote:
>>> From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
>>>
>>> Renesas RZ/Five SoC has OSTM blocks which can be used for clock_event and
>>> clocksource [0]. The clock_event rating for the OSTM is set 300 but
>>> whereas the rating for riscv-timer clock_event is set to 100 due to which
>>> the kernel is choosing OSTM for clock_event.
>>>
>>> As riscv-timer is much more efficient than MMIO clock_event, increase the
>>> rating to 400 so that the kernel prefers riscv-timer over the MMIO based
>>> clock_event.
>>
>> This is only true if you have the Sstc extension and can set stimecmp directly.
>> Otherwise you have the overhead of an SBI call, which is going to be much higher
>> than an MMIO write. So the rating should depend on Sstc, as in this patch:
>>
>> https://lore.kernel.org/linux-riscv/20230710131902.1459180-3-apatel@ventanamicro.com/
>>
> Thank you for the pointer. Do you know any tool/util which I can use
> to make comparisons?

To measure the latency of the trap to M-mode when receiving the timer interrupt,
you could use the timerlat tracer. This computes the delta between the
programmed timestamp, and when the IRQ is actually handled.

To measure the latency of setting the timer, you could add code to compute the
duration of the set_next_event functions. (min_delta_ns won't tell you anything
because set_next_event never fails.)

Regards,
Samuel