Improve CLOCK_EVT_FEAT_C3STOP feature setting
Subsystems (3)
v4 cover Improve CLOCK_EVT_FEAT_C3STOP feature setting
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
Signature · 1 line
2.34.1
PATCH v4 1/3 RISC-V: time: initialize broadcast hrtimer based clock event device
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
--- arch/riscv/kernel/time.c | 3 +++ 1 file changed, 3 insertions(+)
arch/riscv/kernel/time.c +3 -0
@@ -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)
Signature · 1 line
2.34.1
Re: [PATCH v4 1/3] RISC-V: time: initialize broadcast hrtimer based clock event device
Quoted 5 lines · Anup Patel
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.
Quoted 2 lines
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 ...
Quoted 11 lines
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.952179Std 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
Quoted 4 lines
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:
Quoted 25 lines
--- arch/riscv/kernel/time.c | 3 +++ 1 file changed, 3 insertions(+)arch/riscv/kernel/time.c +3
-0@@ -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)
PATCH v4 2/3 dt-bindings: timer: Add bindings for the RISC-V timer device
In reply to Anup PatelWe 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).
--- .../bindings/timer/riscv,timer.yaml | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 Documentation/devicetree/bindings/timer/riscv,timer.yaml
Documentation/devicetree/bindings/timer/riscv,timer.yaml new file +52 -0
@@ -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>; + }; +...
Signature · 1 line
2.34.1
Re: [PATCH v4 2/3] dt-bindings: timer: Add bindings for the RISC-V timer device
Quoted 38 lines · Anup Patel
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.yamlDocumentation/devicetree/bindings/timer/riscv,timer.yaml new file +21
-0@@ -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 +
Documentation/devicetree/bindings/timer/riscv,timer.yaml line 31
Quoted 10 lines
Documentation/devicetree/bindings/timer/riscv,timer.yaml new file +10
-0+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).
Documentation/devicetree/bindings/timer/riscv,timer.yaml line 35
Quoted 4 lines
Documentation/devicetree/bindings/timer/riscv,timer.yaml new file +4
-0+ 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.
Quoted 5 lines
Documentation/devicetree/bindings/timer/riscv,timer.yaml new file +5
-0+ +additionalProperties: false + +required: + - compatible
Documentation/devicetree/bindings/timer/riscv,timer.yaml line 50
Quoted 10 lines
Documentation/devicetree/bindings/timer/riscv,timer.yaml new file +10
-0+ - 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
Re: [PATCH v4 2/3] dt-bindings: timer: Add bindings for the RISC-V timer device
Quoted 47 lines · Samuel Holland
Quoted 45 lines · Anup Patel
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.yamlDocumentation/devicetree/bindings/timer/riscv,timer.yaml new file +28
-0@@ -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
Quoted 8 lines
Documentation/devicetree/bindings/timer/riscv,timer.yaml line 31
Quoted 3 lines
Documentation/devicetree/bindings/timer/riscv,timer.yaml new file +3
-0+ 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.
Quoted 11 lines
Documentation/devicetree/bindings/timer/riscv,timer.yaml line 35
Quoted 5 lines
Documentation/devicetree/bindings/timer/riscv,timer.yaml new file +4
-0+ 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.
Quoted 11 lines
Quoted 11 lines
Documentation/devicetree/bindings/timer/riscv,timer.yaml new file +10
-0+ +additionalProperties: false + +required: + - compatible + - interrupts-extended + +examples: + - | + timer {
Quoted 8 lines
Documentation/devicetree/bindings/timer/riscv,timer.yaml line 50
Quoted 5 lines
Documentation/devicetree/bindings/timer/riscv,timer.yaml new file +5
-0+ 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
PATCH v4 3/3 clocksource: timer-riscv: Set CLOCK_EVT_FEAT_C3STOP based on DT
In reply to Anup PatelWe should set CLOCK_EVT_FEAT_C3STOP for a clock_event_device only when riscv,timer-cant-wake-up DT property is present in the RISC-V timer DT node.
This way CLOCK_EVT_FEAT_C3STOP feature is set for clock_event_device based on RISC-V platform capabilities rather than having it set for all RISC-V platforms.
--- drivers/clocksource/timer-riscv.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)
drivers/clocksource/timer-riscv.c +11 -1
@@ -28,6 +28,7 @@#include <asm/timex.h> static DEFINE_STATIC_KEY_FALSE (riscv_sstc_available); +static bool riscv_timer_cant_wake_cpu;static int riscv_clock_next_event (unsigned long delta,struct clock_event_device *ce)
@@ -51,7 +52,7 @@ static int riscv_clock_next_event(unsigned long delta,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 |CLOCK_EVT_FEAT_C3STOP , + .features =CLOCK_EVT_FEAT_ONESHOT , .rating =100 , .set_next_event = riscv_clock_next_event, };
@@ -85,6 +86,8 @@ static int riscv_timer_starting_cpu(unsigned int cpu) ce->cpumask =cpumask_of (cpu); ce->irq = riscv_clock_event_irq; +if (riscv_timer_cant_wake_cpu) + ce->features |=CLOCK_EVT_FEAT_C3STOP ;clockevents_config_and_register (ce, riscv_timebase,100 ,0x7fffffff ); enable_percpu_irq(riscv_clock_event_irq,
@@ -139,6 +142,13 @@ static int __init riscv_timer_init_dt(struct device_node *n)if (cpuid !=smp_processor_id ())return 0 ; + child =of_find_compatible_node (NULL ,NULL ,"riscv,timer" ); +if (child) { + riscv_timer_cant_wake_cpu =of_property_read_bool (child, +"riscv,timer-cant-wake-cpu" ); +of_node_put (child); + } + domain =NULL ; child =of_get_compatible_child (n,"riscv,cpu-intc" );if (!child) {
Signature · 1 line
2.34.1
Re: [PATCH v4 3/3] clocksource: timer-riscv: Set CLOCK_EVT_FEAT_C3STOP based on DT
Quoted 9 lines · Anup Patel
We should set
CLOCK_EVT_FEAT_C3STOPfor aclock_event_deviceonly when riscv,timer-cant-wake-up DT property is present in the RISC-V timer DT node.This way
CLOCK_EVT_FEAT_C3STOPfeature is set forclock_event_devicebased on RISC-V platform capabilities rather than having it set for all RISC-V platforms.Signed-off-by Anup Patel <[email protected]>
I thought I had left an R-b on this one?
Also, I think that we need to backport *something* that disables C3STOP which is why I had suggested keeping the revert in place. Patch 1 of this series only solves the timer issues but does not restore sleep states to their prior behaviour, right?
Either this patch or the revert needs to go to stable IMO.
Thanks,
Conor.
Quoted 51 lines
--- drivers/clocksource/timer-riscv.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)drivers/clocksource/timer-riscv.c +11
-1@@ -28,6 +28,7 @@#include <asm/timex.h> static DEFINE_STATIC_KEY_FALSE (riscv_sstc_available); +static bool riscv_timer_cant_wake_cpu;static int riscv_clock_next_event (unsigned long delta,struct clock_event_device *ce)@@ -51,7 +52,7 @@ static int riscv_clock_next_event(unsigned long delta,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 |CLOCK_EVT_FEAT_C3STOP , + .features =CLOCK_EVT_FEAT_ONESHOT , .rating =100 , .set_next_event = riscv_clock_next_event, };@@ -85,6 +86,8 @@ static int riscv_timer_starting_cpu(unsigned int cpu) ce->cpumask =cpumask_of (cpu); ce->irq = riscv_clock_event_irq; +if (riscv_timer_cant_wake_cpu) + ce->features |=CLOCK_EVT_FEAT_C3STOP ;clockevents_config_and_register (ce, riscv_timebase,100 ,0x7fffffff ); enable_percpu_irq(riscv_clock_event_irq,@@ -139,6 +142,13 @@ static int __init riscv_timer_init_dt(struct device_node *n)if (cpuid !=smp_processor_id ())return 0 ; + child =of_find_compatible_node (NULL ,NULL ,"riscv,timer" ); +if (child) { + riscv_timer_cant_wake_cpu =of_property_read_bool (child, +"riscv,timer-cant-wake-cpu" ); +of_node_put (child); + } + domain =NULL ; child =of_get_compatible_child (n,"riscv,cpu-intc" );if (!child) {Signature · 1 line
2.34.1
Re: [PATCH v4 3/3] clocksource: timer-riscv: Set CLOCK_EVT_FEAT_C3STOP based on DT
Quoted 12 lines · Conor Dooley
Quoted 9 lines · Anup Patel
We should set
CLOCK_EVT_FEAT_C3STOPfor aclock_event_deviceonly when riscv,timer-cant-wake-up DT property is present in the RISC-V timer DT node.This way
CLOCK_EVT_FEAT_C3STOPfeature is set forclock_event_devicebased on RISC-V platform capabilities rather than having it set for all RISC-V platforms.Signed-off-by Anup Patel <[email protected]>
Quoted 8 lines
I thought I had left an R-b on this one?
Reviewed-by Conor Dooley <[email protected]>Also, I think that we need to backport *something* that disables C3STOP which is why I had suggested keeping the revert in place. Patch 1 of this series only solves the timer issues but does not restore sleep states to their prior behaviour, right?
Either this patch or the revert needs to go to stable IMO.
Since it works for you with the C3STOP set and broadcast timer enabled, we can directly go with this patch. I am fine including the revert as well.
Regards,
Anup
Quoted 55 lines
Thanks,
Conor.Quoted 51 lines
--- drivers/clocksource/timer-riscv.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)drivers/clocksource/timer-riscv.c +11
-2@@ -28,6 +28,7 @@#include <asm/timex.h> static DEFINE_STATIC_KEY_FALSE (riscv_sstc_available); +static bool riscv_timer_cant_wake_cpu;static int riscv_clock_next_event (unsigned long delta,struct clock_event_device *ce)@@ -51,7 +52,7 @@ static int riscv_clock_next_event(unsigned long delta,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 |CLOCK_EVT_FEAT_C3STOP , + .features =CLOCK_EVT_FEAT_ONESHOT , .rating =100 , .set_next_event = riscv_clock_next_event, };@@ -85,6 +86,8 @@ static int riscv_timer_starting_cpu(unsigned int cpu) ce->cpumask =cpumask_of (cpu); ce->irq = riscv_clock_event_irq; +if (riscv_timer_cant_wake_cpu) + ce->features |=CLOCK_EVT_FEAT_C3STOP ;clockevents_config_and_register (ce, riscv_timebase,100 ,0x7fffffff ); enable_percpu_irq(riscv_clock_event_irq,@@ -139,6 +142,13 @@ static int __init riscv_timer_init_dt(struct device_node *n)if (cpuid !=smp_processor_id ())return 0 ; + child =of_find_compatible_node (NULL ,NULL ,"riscv,timer" ); +if (child) { + riscv_timer_cant_wake_cpu =of_property_read_bool (child, +"riscv,timer-cant-wake-cpu" ); +of_node_put (child); + } + domain =NULL ; child =of_get_compatible_child (n,"riscv,cpu-intc" );if (!child) { --2.34.1
Re: [PATCH v4 3/3] clocksource: timer-riscv: Set CLOCK_EVT_FEAT_C3STOP based on DT
Quoted 16 lines · Anup Patel
Quoted 15 lines · Conor Dooley
Quoted 9 lines · Anup Patel
We should set
CLOCK_EVT_FEAT_C3STOPfor aclock_event_deviceonly when riscv,timer-cant-wake-up DT property is present in the RISC-V timer DT node.This way
CLOCK_EVT_FEAT_C3STOPfeature is set forclock_event_devicebased on RISC-V platform capabilities rather than having it set for all RISC-V platforms.Signed-off-by Anup Patel <[email protected]>I thought I had left an R-b on this one?
Reviewed-by Conor Dooley <[email protected]>
Quoted 8 lines
Quoted 5 lines
Also, I think that we need to backport *something* that disables C3STOP which is why I had suggested keeping the revert in place. Patch 1 of this series only solves the timer issues but does not restore sleep states to their prior behaviour, right?
Either this patch or the revert needs to go to stable IMO.Since it works for you with the C3STOP set and broadcast timer enabled, we can directly go with this patch. I am fine including the revert as well.
I don't mind which gets backported. To me, this one is preferable as it is more "complete" but it is a bit on the new feature side of things, no?
Whoever applies it can decide, and I'll backport the revert if they decide that this patch is not stable material :)
Thanks again for helping sort this mess out, I see it helped with your IPI series too!
Conor.
Quoted 51 lines
Quoted 51 lines
Quoted 51 lines
--- drivers/clocksource/timer-riscv.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)drivers/clocksource/timer-riscv.c +11
-2@@ -28,6 +28,7 @@#include <asm/timex.h> static DEFINE_STATIC_KEY_FALSE (riscv_sstc_available); +static bool riscv_timer_cant_wake_cpu;static int riscv_clock_next_event (unsigned long delta,struct clock_event_device *ce)@@ -51,7 +52,7 @@ static int riscv_clock_next_event(unsigned long delta,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 |CLOCK_EVT_FEAT_C3STOP , + .features =CLOCK_EVT_FEAT_ONESHOT , .rating =100 , .set_next_event = riscv_clock_next_event, };@@ -85,6 +86,8 @@ static int riscv_timer_starting_cpu(unsigned int cpu) ce->cpumask =cpumask_of (cpu); ce->irq = riscv_clock_event_irq; +if (riscv_timer_cant_wake_cpu) + ce->features |=CLOCK_EVT_FEAT_C3STOP ;clockevents_config_and_register (ce, riscv_timebase,100 ,0x7fffffff ); enable_percpu_irq(riscv_clock_event_irq,@@ -139,6 +142,13 @@ static int __init riscv_timer_init_dt(struct device_node *n)if (cpuid !=smp_processor_id ())return 0 ; + child =of_find_compatible_node (NULL ,NULL ,"riscv,timer" ); +if (child) { + riscv_timer_cant_wake_cpu =of_property_read_bool (child, +"riscv,timer-cant-wake-cpu" ); +of_node_put (child); + } + domain =NULL ; child =of_get_compatible_child (n,"riscv,cpu-intc" );if (!child) { --2.34.1
Re: [PATCH v4 3/3] clocksource: timer-riscv: Set CLOCK_EVT_FEAT_C3STOP based on DT
Quoted 31 lines · Conor Dooley
Quoted 16 lines · Anup Patel
Quoted 15 lines · Conor Dooley
Quoted 9 lines · Anup Patel
We should set
CLOCK_EVT_FEAT_C3STOPfor aclock_event_deviceonly when riscv,timer-cant-wake-up DT property is present in the RISC-V timer DT node.This way
CLOCK_EVT_FEAT_C3STOPfeature is set forclock_event_devicebased on RISC-V platform capabilities rather than having it set for all RISC-V platforms.Signed-off-by Anup Patel <[email protected]>I thought I had left an R-b on this one?
Reviewed-by Conor Dooley <[email protected]>Quoted 8 lines
Quoted 5 lines
Also, I think that we need to backport *something* that disables C3STOP which is why I had suggested keeping the revert in place. Patch 1 of this series only solves the timer issues but does not restore sleep states to their prior behaviour, right?
Either this patch or the revert needs to go to stable IMO.Since it works for you with the C3STOP set and broadcast timer enabled, we can directly go with this patch. I am fine including the revert as well.
I don't mind which gets backported. To me, this one is preferable as it is more "complete" but it is a bit on the new feature side of things, no?
Quoted 5 lines
Whoever applies it can decide, and I'll backport the revert if they decide that this patch is not stable material :)
Thanks again for helping sort this mess out, I see it helped with your IPI series too!
Yes, I was surprised to see that it helped the IPI series as well. Thanks for your patch.
Regards,
Anup
Quoted 54 lines
Conor.
Quoted 51 lines
Quoted 51 lines
Quoted 51 lines
--- drivers/clocksource/timer-riscv.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)drivers/clocksource/timer-riscv.c +11
-2@@ -28,6 +28,7 @@#include <asm/timex.h> static DEFINE_STATIC_KEY_FALSE (riscv_sstc_available); +static bool riscv_timer_cant_wake_cpu;static int riscv_clock_next_event (unsigned long delta,struct clock_event_device *ce)@@ -51,7 +52,7 @@ static int riscv_clock_next_event(unsigned long delta,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 |CLOCK_EVT_FEAT_C3STOP , + .features =CLOCK_EVT_FEAT_ONESHOT , .rating =100 , .set_next_event = riscv_clock_next_event, };@@ -85,6 +86,8 @@ static int riscv_timer_starting_cpu(unsigned int cpu) ce->cpumask =cpumask_of (cpu); ce->irq = riscv_clock_event_irq; +if (riscv_timer_cant_wake_cpu) + ce->features |=CLOCK_EVT_FEAT_C3STOP ;clockevents_config_and_register (ce, riscv_timebase,100 ,0x7fffffff ); enable_percpu_irq(riscv_clock_event_irq,@@ -139,6 +142,13 @@ static int __init riscv_timer_init_dt(struct device_node *n)if (cpuid !=smp_processor_id ())return 0 ; + child =of_find_compatible_node (NULL ,NULL ,"riscv,timer" ); +if (child) { + riscv_timer_cant_wake_cpu =of_property_read_bool (child, +"riscv,timer-cant-wake-cpu" ); +of_node_put (child); + } + domain =NULL ; child =of_get_compatible_child (n,"riscv,cpu-intc" );if (!child) { --2.34.1
Palmer Dabbelt
maintainerRe: [PATCH v4 3/3] clocksource: timer-riscv: Set CLOCK_EVT_FEAT_C3STOP based on DT
Quoted 28 lines · On Tue, 29 Nov 2022 09:22:22 PST (-0800), [email protected] wrote:
Quoted 27 lines · Conor Dooley
Quoted 24 lines · Anup Patel
Quoted 12 lines · Conor Dooley
Quoted 9 lines · Anup Patel
We should set
CLOCK_EVT_FEAT_C3STOPfor aclock_event_deviceonly when riscv,timer-cant-wake-up DT property is present in the RISC-V timer DT node.This way
CLOCK_EVT_FEAT_C3STOPfeature is set forclock_event_devicebased on RISC-V platform capabilities rather than having it set for all RISC-V platforms.Signed-off-by Anup Patel <[email protected]>Quoted 8 lines
I thought I had left an R-b on this one?
Reviewed-by Conor Dooley <[email protected]>Also, I think that we need to backport *something* that disables C3STOP which is why I had suggested keeping the revert in place. Patch 1 of this series only solves the timer issues but does not restore sleep states to their prior behaviour, right?
Either this patch or the revert needs to go to stable IMO.Since it works for you with the C3STOP set and broadcast timer enabled, we can directly go with this patch. I am fine including the revert as well.
Quoted 6 lines
Quoted 6 lines
I don't mind which gets backported. To me, this one is preferable as it is more "complete" but it is a bit on the new feature side of things, no?
Whoever applies it can decide, and I'll backport the revert if they decide that this patch is not stable material :)
IIRC the clock folks took the original C3 patch, so that's probably the best way to take these as well?
Quoted 64 lines
Quoted 3 lines
Thanks again for helping sort this mess out, I see it helped with your IPI series too!
Yes, I was surprised to see that it helped the IPI series as well. Thanks for your patch.
Regards,
AnupQuoted 54 lines
Conor.
Quoted 51 lines
Quoted 51 lines
Quoted 51 lines
--- drivers/clocksource/timer-riscv.c | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)drivers/clocksource/timer-riscv.c +11
-2@@ -28,6 +28,7 @@#include <asm/timex.h> static DEFINE_STATIC_KEY_FALSE (riscv_sstc_available); +static bool riscv_timer_cant_wake_cpu;static int riscv_clock_next_event (unsigned long delta,struct clock_event_device *ce)@@ -51,7 +52,7 @@ static int riscv_clock_next_event(unsigned long delta,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 |CLOCK_EVT_FEAT_C3STOP , + .features =CLOCK_EVT_FEAT_ONESHOT , .rating =100 , .set_next_event = riscv_clock_next_event, };@@ -85,6 +86,8 @@ static int riscv_timer_starting_cpu(unsigned int cpu) ce->cpumask =cpumask_of (cpu); ce->irq = riscv_clock_event_irq; +if (riscv_timer_cant_wake_cpu) + ce->features |=CLOCK_EVT_FEAT_C3STOP ;clockevents_config_and_register (ce, riscv_timebase,100 ,0x7fffffff ); enable_percpu_irq(riscv_clock_event_irq,@@ -139,6 +142,13 @@ static int __init riscv_timer_init_dt(struct device_node *n)if (cpuid !=smp_processor_id ())return 0 ; + child =of_find_compatible_node (NULL ,NULL ,"riscv,timer" ); +if (child) { + riscv_timer_cant_wake_cpu =of_property_read_bool (child, +"riscv,timer-cant-wake-cpu" ); +of_node_put (child); + } + domain =NULL ; child =of_get_compatible_child (n,"riscv,cpu-intc" );if (!child) { --2.34.1
Quoted 23 lines · 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_C3STOPfeature based on RISC-V platform capabilities.These patches can also be found in
riscv_timer_dt_imp_v4branch at:
https://github.com/avpatel/linux.gitChanges since v3:
- Rebased on Linux-6.1-rc7
- Replaced PATCH1 with a patch to initialize broadcast timerChanges 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 bindingsChanges 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: SetCLOCK_EVT_FEAT_C3STOPbased on DT
Quoted 8 lines
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
IIRC the main issue here were the DT bindings, though, so I think we'll need to make sure that's sorted out.