This series improves the RISC-V timer driver to set CLOCK_EVT_FEAT_C3STOP
feature based on RISC-V platform capabilities.
These patches can also be found in riscv_timer_dt_imp_v4 branch at:
https://github.com/avpatel/linux.git
Changes since v3:
- Rebased on Linux-6.1-rc7
- Replaced PATCH1 with a patch to initialize broadcast timer
Changes since v2:
- Include Conor's revert patch as the first patch and rebased other patches
- Update PATCH2 to document bindings for separate RISC-V timer DT node
- Update PATCH3 based on RISC-V timer DT node bindings
Changes since v1:
- Rebased on Linux-5.19-rc8
- Renamed "riscv,always-on" DT property to "riscv,timer-can-wake-cpu"
Anup Patel (2):
dt-bindings: timer: Add bindings for the RISC-V timer device
clocksource: timer-riscv: Set CLOCK_EVT_FEAT_C3STOP based on DT
Conor Dooley (1):
RISC-V: time: initialize broadcast hrtimer based clock event device
.../bindings/timer/riscv,timer.yaml | 52 +++++++++++++++++++
arch/riscv/kernel/time.c | 3 ++
drivers/clocksource/timer-riscv.c | 12 ++++-
3 files changed, 66 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/timer/riscv,timer.yaml
--
2.34.1
We add DT bindings for a separate RISC-V timer DT node which can
be used to describe implementation specific behaviour (such as
timer interrupt not triggered during non-retentive suspend).
Signed-off-by: Anup Patel <[email protected]>
Reviewed-by: Conor Dooley <[email protected]>
---
.../bindings/timer/riscv,timer.yaml | 52 +++++++++++++++++++
1 file changed, 52 insertions(+)
create mode 100644 Documentation/devicetree/bindings/timer/riscv,timer.yaml
diff --git a/Documentation/devicetree/bindings/timer/riscv,timer.yaml b/Documentation/devicetree/bindings/timer/riscv,timer.yaml
new file mode 100644
index 000000000000..cf53dfff90bc
--- /dev/null
+++ b/Documentation/devicetree/bindings/timer/riscv,timer.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/timer/riscv,timer.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: RISC-V timer
+
+maintainers:
+ - Anup Patel <[email protected]>
+
+description: |+
+ RISC-V platforms always have a RISC-V timer device for the supervisor-mode
+ based on the time CSR defined by the RISC-V privileged specification. The
+ timer interrupts of this device are configured using the RISC-V SBI Time
+ extension or the RISC-V Sstc extension.
+
+ The clock frequency of RISC-V timer device is specified via the
+ "timebase-frequency" DT property of "/cpus" DT node which is described
+ in Documentation/devicetree/bindings/riscv/cpus.yaml
+
+properties:
+ compatible:
+ enum:
+ - riscv,timer
+
+ interrupts-extended:
+ minItems: 1
+ maxItems: 4096 # Should be enough?
+
+ riscv,timer-cant-wake-cpu:
+ type: boolean
+ description:
+ If present, the timer interrupt can't wake up the CPU from
+ suspend/idle state.
+
+additionalProperties: false
+
+required:
+ - compatible
+ - interrupts-extended
+
+examples:
+ - |
+ timer {
+ compatible = "riscv,timer";
+ interrupts-extended = <&cpu1intc 5>,
+ <&cpu2intc 5>,
+ <&cpu3intc 5>,
+ <&cpu4intc 5>;
+ };
+...
--
2.34.1
From: Conor Dooley <[email protected]>
Similarly to commit 022eb8ae8b5e ("ARM: 8938/1: kernel: initialize
broadcast hrtimer based clock event device"), RISC-V needs to initiate
hrtimers before C3STOP can be used. Otherwise, the introduction of C3STOP
for the RISC-V arch timer in commit 232ccac1bd9b
("clocksource/drivers/riscv: Events are stopped during CPU suspend")
breaks timer behaviour, for example clock_nanosleep().
A test app that sleeps each cpu for 6, 5, 4, 3 ms respectively, HZ=250
& C3STOP enabled, the sleep times are rounded up to the next jiffy:
== CPU: 1 == == CPU: 2 == == CPU: 3 == == CPU: 4 ==
Mean: 7.974992 Mean: 7.976534 Mean: 7.962591 Mean: 3.952179
Std Dev: 0.154374 Std Dev: 0.156082 Std Dev: 0.171018 Std Dev: 0.076193
Hi: 9.472000 Hi: 10.495000 Hi: 8.864000 Hi: 4.736000
Lo: 6.087000 Lo: 6.380000 Lo: 4.872000 Lo: 3.403000
Samples: 521 Samples: 521 Samples: 521 Samples: 521
Link: https://lore.kernel.org/linux-riscv/YzYTNQRxLr7Q9JR0@spud/
Fixes: 232ccac1bd9b ("clocksource/drivers/riscv: Events are stopped during CPU suspend")
Suggested-by: Samuel Holland <[email protected]>
Signed-off-by: Conor Dooley <[email protected]>
---
arch/riscv/kernel/time.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/riscv/kernel/time.c b/arch/riscv/kernel/time.c
index 8217b0f67c6c..1cf21db4fcc7 100644
--- a/arch/riscv/kernel/time.c
+++ b/arch/riscv/kernel/time.c
@@ -5,6 +5,7 @@
*/
#include <linux/of_clk.h>
+#include <linux/clockchips.h>
#include <linux/clocksource.h>
#include <linux/delay.h>
#include <asm/sbi.h>
@@ -29,6 +30,8 @@ void __init time_init(void)
of_clk_init(NULL);
timer_probe();
+
+ tick_setup_hrtimer_broadcast();
}
void clocksource_arch_init(struct clocksource *cs)
--
2.34.1
On Tue, 29 Nov 2022 06:03:10 PST (-0800), [email protected] wrote:
> This series improves the RISC-V timer driver to set CLOCK_EVT_FEAT_C3STOP
> feature based on RISC-V platform capabilities.
>
> These patches can also be found in riscv_timer_dt_imp_v4 branch at:
> https://github.com/avpatel/linux.git
>
> Changes since v3:
> - Rebased on Linux-6.1-rc7
> - Replaced PATCH1 with a patch to initialize broadcast timer
>
> Changes since v2:
> - Include Conor's revert patch as the first patch and rebased other patches
> - Update PATCH2 to document bindings for separate RISC-V timer DT node
> - Update PATCH3 based on RISC-V timer DT node bindings
>
> Changes since v1:
> - Rebased on Linux-5.19-rc8
> - Renamed "riscv,always-on" DT property to "riscv,timer-can-wake-cpu"
>
> Anup Patel (2):
> dt-bindings: timer: Add bindings for the RISC-V timer device
> clocksource: timer-riscv: Set CLOCK_EVT_FEAT_C3STOP based on DT
>
> Conor Dooley (1):
> RISC-V: time: initialize broadcast hrtimer based clock event device
>
> .../bindings/timer/riscv,timer.yaml | 52 +++++++++++++++++++
> arch/riscv/kernel/time.c | 3 ++
> drivers/clocksource/timer-riscv.c | 12 ++++-
> 3 files changed, 66 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/devicetree/bindings/timer/riscv,timer.yaml
Acked-by: Palmer Dabbelt <[email protected]>
IIRC the main issue here were the DT bindings, though, so I think we'll
need to make sure that's sorted out.
On 11/29/22 08:03, Anup Patel wrote:
> From: Conor Dooley <[email protected]>
>
> Similarly to commit 022eb8ae8b5e ("ARM: 8938/1: kernel: initialize
> broadcast hrtimer based clock event device"), RISC-V needs to initiate
> hrtimers before C3STOP can be used. Otherwise, the introduction of C3STOP
Specifically it is the hrtimer-based broadcast clockevent that we need
to initialize, not hrtimers as a whole.
> for the RISC-V arch timer in commit 232ccac1bd9b
> ("clocksource/drivers/riscv: Events are stopped during CPU suspend")
Maybe add some more details here:
... leaves us without any broadcast timer registered. This prevents the
kernel from entering oneshot mode, which ...
> breaks timer behaviour, for example clock_nanosleep().
>
> A test app that sleeps each cpu for 6, 5, 4, 3 ms respectively, HZ=250
> & C3STOP enabled, the sleep times are rounded up to the next jiffy:
> == CPU: 1 == == CPU: 2 == == CPU: 3 == == CPU: 4 ==
> Mean: 7.974992 Mean: 7.976534 Mean: 7.962591 Mean: 3.952179
> Std Dev: 0.154374 Std Dev: 0.156082 Std Dev: 0.171018 Std Dev: 0.076193
> Hi: 9.472000 Hi: 10.495000 Hi: 8.864000 Hi: 4.736000
> Lo: 6.087000 Lo: 6.380000 Lo: 4.872000 Lo: 3.403000
> Samples: 521 Samples: 521 Samples: 521 Samples: 521
>
> Link: https://lore.kernel.org/linux-riscv/YzYTNQRxLr7Q9JR0@spud/
> Fixes: 232ccac1bd9b ("clocksource/drivers/riscv: Events are stopped during CPU suspend")
> Suggested-by: Samuel Holland <[email protected]>
> Signed-off-by: Conor Dooley <[email protected]>
Either way:
Reviewed-by: Samuel Holland <[email protected]>
> ---
> arch/riscv/kernel/time.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/riscv/kernel/time.c b/arch/riscv/kernel/time.c
> index 8217b0f67c6c..1cf21db4fcc7 100644
> --- a/arch/riscv/kernel/time.c
> +++ b/arch/riscv/kernel/time.c
> @@ -5,6 +5,7 @@
> */
>
> #include <linux/of_clk.h>
> +#include <linux/clockchips.h>
> #include <linux/clocksource.h>
> #include <linux/delay.h>
> #include <asm/sbi.h>
> @@ -29,6 +30,8 @@ void __init time_init(void)
>
> of_clk_init(NULL);
> timer_probe();
> +
> + tick_setup_hrtimer_broadcast();
> }
>
> void clocksource_arch_init(struct clocksource *cs)
On 11/29/22 08:03, Anup Patel wrote:
> We add DT bindings for a separate RISC-V timer DT node which can
> be used to describe implementation specific behaviour (such as
> timer interrupt not triggered during non-retentive suspend).
>
> Signed-off-by: Anup Patel <[email protected]>
> Reviewed-by: Conor Dooley <[email protected]>
> ---
> .../bindings/timer/riscv,timer.yaml | 52 +++++++++++++++++++
> 1 file changed, 52 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/timer/riscv,timer.yaml
>
> diff --git a/Documentation/devicetree/bindings/timer/riscv,timer.yaml b/Documentation/devicetree/bindings/timer/riscv,timer.yaml
> new file mode 100644
> index 000000000000..cf53dfff90bc
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/timer/riscv,timer.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/timer/riscv,timer.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: RISC-V timer
> +
> +maintainers:
> + - Anup Patel <[email protected]>
> +
> +description: |+
> + RISC-V platforms always have a RISC-V timer device for the supervisor-mode
> + based on the time CSR defined by the RISC-V privileged specification. The
> + timer interrupts of this device are configured using the RISC-V SBI Time
> + extension or the RISC-V Sstc extension.
> +
> + The clock frequency of RISC-V timer device is specified via the
> + "timebase-frequency" DT property of "/cpus" DT node which is described
> + in Documentation/devicetree/bindings/riscv/cpus.yaml
> +
> +properties:
> + compatible:
> + enum:
> + - riscv,timer
> +
> + interrupts-extended:
> + minItems: 1
> + maxItems: 4096 # Should be enough?
> +
> + riscv,timer-cant-wake-cpu:
I don't want to derail getting this merged, but if you do end up sending
another version, could you please spell out the word "cannot" here and
in the code? The missing apostrophe makes this jarring (and an entirely
different word).
> + type: boolean
> + description:
> + If present, the timer interrupt can't wake up the CPU from
> + suspend/idle state.
And in that case I would also suggest clarifying this as "one or more
suspend/idle states", since the limitation does not apply to all idle
states. At least it should never apply to the architectural WFI state;
for the SBI idle state binding, it only applies to those with the
"local-timer-stop" property.
> +
> +additionalProperties: false
> +
> +required:
> + - compatible
> + - interrupts-extended
> +
> +examples:
> + - |
> + timer {
> + compatible = "riscv,timer";
> + interrupts-extended = <&cpu1intc 5>,
> + <&cpu2intc 5>,
> + <&cpu3intc 5>,
> + <&cpu4intc 5>;
The CLINT and PLIC bindings also include the M-mode interrupts. Should
we do the same here?
Regards,
Samuel
On Wed, Nov 30, 2022 at 10:15 AM Samuel Holland <[email protected]> wrote:
>
> On 11/29/22 08:03, Anup Patel wrote:
> > We add DT bindings for a separate RISC-V timer DT node which can
> > be used to describe implementation specific behaviour (such as
> > timer interrupt not triggered during non-retentive suspend).
> >
> > Signed-off-by: Anup Patel <[email protected]>
> > Reviewed-by: Conor Dooley <[email protected]>
> > ---
> > .../bindings/timer/riscv,timer.yaml | 52 +++++++++++++++++++
> > 1 file changed, 52 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/timer/riscv,timer.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/timer/riscv,timer.yaml b/Documentation/devicetree/bindings/timer/riscv,timer.yaml
> > new file mode 100644
> > index 000000000000..cf53dfff90bc
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/timer/riscv,timer.yaml
> > @@ -0,0 +1,52 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/timer/riscv,timer.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: RISC-V timer
> > +
> > +maintainers:
> > + - Anup Patel <[email protected]>
> > +
> > +description: |+
> > + RISC-V platforms always have a RISC-V timer device for the supervisor-mode
> > + based on the time CSR defined by the RISC-V privileged specification. The
> > + timer interrupts of this device are configured using the RISC-V SBI Time
> > + extension or the RISC-V Sstc extension.
> > +
> > + The clock frequency of RISC-V timer device is specified via the
> > + "timebase-frequency" DT property of "/cpus" DT node which is described
> > + in Documentation/devicetree/bindings/riscv/cpus.yaml
> > +
> > +properties:
> > + compatible:
> > + enum:
> > + - riscv,timer
> > +
> > + interrupts-extended:
> > + minItems: 1
> > + maxItems: 4096 # Should be enough?
> > +
> > + riscv,timer-cant-wake-cpu:
>
> I don't want to derail getting this merged, but if you do end up sending
> another version, could you please spell out the word "cannot" here and
> in the code? The missing apostrophe makes this jarring (and an entirely
> different word).
Okay, I will update.
>
> > + type: boolean
> > + description:
> > + If present, the timer interrupt can't wake up the CPU from
> > + suspend/idle state.
>
> And in that case I would also suggest clarifying this as "one or more
> suspend/idle states", since the limitation does not apply to all idle
> states. At least it should never apply to the architectural WFI state;
> for the SBI idle state binding, it only applies to those with the
> "local-timer-stop" property.
Okay, I will update.
>
> > +
> > +additionalProperties: false
> > +
> > +required:
> > + - compatible
> > + - interrupts-extended
> > +
> > +examples:
> > + - |
> > + timer {
> > + compatible = "riscv,timer";
> > + interrupts-extended = <&cpu1intc 5>,
> > + <&cpu2intc 5>,
> > + <&cpu3intc 5>,
> > + <&cpu4intc 5>;
>
> The CLINT and PLIC bindings also include the M-mode interrupts. Should
> we do the same here?
The RISC-V timer uses SBI time extension or RISC-V Sstc extension hence
it is only for S-mode software. In other words, the RISC-V timer is a S-mode
only timer.
The M-mode software is supposed to have its own platform specific MMIO
based timer.
Regards,
Anup