twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally

+Optional
+- twd_never_stops : Don't set the CLOCK_EVT_FEAT_C3STOP flag.

In 5388a6b266 ("ARM: SMP: Always enable clock event broadcast support") Russell noted that "the TWD local timers are unable to wake up the CPU when it is placed into a low power mode".

However, some platforms do not stop the TWD block in low-power mode, and can thus use the TWD timers in one-shot mode, without setting up a broadcast device.

Make the driver check for the "twd_never_stops" property, and set the CLOCK_EVT_FEAT_C3STOP flag accordingly.

Signed-off-by Marc Gonzalez <[email protected]>

---
 Documentation/devicetree/bindings/arm/twd.txt | 5 +++++
 arch/arm/kernel/smp_twd.c                     | 5 ++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/arm/twd.txt +0 -0

index 75b8610939fa..7f849235e12a 100644

Documentation/devicetree/bindings/arm/twd.txt +1 -0

+
 Example:
 
 	twd-timer at 2c000600 {

arch/arm/kernel/smp_twd.c +4 -1

index e8f6d241881f..79298d4fb41f 100644
@@ -33,6 +33,7 @@ static unsigned long twd_timer_rate;
 static DEFINE_PER_CPU(bool, percpu_setup_called);
 
 static struct clock_event_device __percpu *twd_evt;
+static int feat_c3stop;
 static int twd_ppi;
 
 static void twd_set_mode(enum clock_event_mode mode,

@@ -293,7 +294,7 @@ static void twd_timer_setup(void)
 
 	clk->name = "local_timer";
 	clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
-			CLOCK_EVT_FEAT_C3STOP;
+			feat_c3stop;
 	clk->rating = 350;
 	clk->set_mode = twd_set_mode;
 	clk->set_next_event = twd_set_next_event;

@@ -345,6 +346,8 @@ static int __init twd_local_timer_common_register(struct device_node *np)
 		goto out_irq;
 
 	twd_get_clock(np);
+	if (!of_property_read_bool(np, "twd-never-stops"))
+		feat_c3stop = CLOCK_EVT_FEAT_C3STOP;
 
 	/*
 	 * Immediately configure the timer on the boot CPU, unless we need

Signature · 1 line

2.4.5

In 5388a6b266 ("ARM: SMP: Always enable clock event broadcast support") Russell noted that "the TWD local timers are unable to wake up the CPU when it is placed into a low power mode".

However, some platforms do not stop the TWD block in low-power mode, and can thus use the TWD timer in one-shot mode, without setting up a broadcast device.

Make the driver check for the "twd-never-stops" boolean property, and set the CLOCK_EVT_FEAT_C3STOP flag accordingly.

Signed-off-by Marc Gonzalez <[email protected]>

---
 Documentation/devicetree/bindings/arm/twd.txt | 4 ++++
 arch/arm/kernel/smp_twd.c                     | 5 ++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

Quoted 8 lines · Marc Gonzalez

In 5388a6b266 ("ARM: SMP: Always enable clock event broadcast support") Russell noted that "the TWD local timers are unable to wake up the CPU when it is placed into a low power mode".

However, some platforms do not stop the TWD block in low-power mode, and can thus use the TWD timer in one-shot mode, without setting up a broadcast device.

Quoted 8 lines

Make the driver check for the "twd-never-stops" boolean property, and set the CLOCK_EVT_FEAT_C3STOP flag accordingly.

Signed-off-by Marc Gonzalez <[email protected]>

---
 Documentation/devicetree/bindings/arm/twd.txt | 4 ++++
 arch/arm/kernel/smp_twd.c                     | 5 ++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

Sorry, I don't fully understand the submission process.

Should I now submit my patch to the patch queue?
http://www.arm.linux.org.uk/developer/patches/

In 5388a6b266 ("ARM: SMP: Always enable clock event broadcast support") Russell noted that "the TWD local timers are unable to wake up the CPU when it is placed into a low power mode".

However, some platforms do not stop the TWD block in low-power mode, and can thus use the TWD timer in one-shot mode, without setting up a broadcast device.

Make the driver check for the "twd-never-stops" boolean property, and set the CLOCK_EVT_FEAT_C3STOP flag accordingly.

Signed-off-by Marc Gonzalez <[email protected]>

---
 Documentation/devicetree/bindings/arm/twd.txt | 4 ++++
 arch/arm/kernel/smp_twd.c                     | 5 ++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/arm/twd.txt +0 -0

index 75b8610939fa..700a517b000e 100644

Documentation/devicetree/bindings/arm/twd.txt +1 -0

+
 Example:
 
 	twd-timer at 2c000600 {

arch/arm/kernel/smp_twd.c +4 -1

index e8f6d241881f..79298d4fb41f 100644
@@ -33,6 +33,7 @@ static unsigned long twd_timer_rate;
 static DEFINE_PER_CPU(bool, percpu_setup_called);
 
 static struct clock_event_device __percpu *twd_evt;
+static int feat_c3stop;
 static int twd_ppi;
 
 static void twd_set_mode(enum clock_event_mode mode,

@@ -293,7 +294,7 @@ static void twd_timer_setup(void)
 
 	clk->name = "local_timer";
 	clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
-			CLOCK_EVT_FEAT_C3STOP;
+			feat_c3stop;
 	clk->rating = 350;
 	clk->set_mode = twd_set_mode;
 	clk->set_next_event = twd_set_next_event;

@@ -345,6 +346,8 @@ static int __init twd_local_timer_common_register(struct device_node *np)
 		goto out_irq;
 
 	twd_get_clock(np);
+	if (!of_property_read_bool(np, "twd-never-stops"))
+		feat_c3stop = CLOCK_EVT_FEAT_C3STOP;
 
 	/*
 	 * Immediately configure the timer on the boot CPU, unless we need

Signature · 1 line

2.4.5

Quoted 22 lines · Marc Gonzalez

In 5388a6b266 ("ARM: SMP: Always enable clock event broadcast support") Russell noted that "the TWD local timers are unable to wake up the CPU when it is placed into a low power mode".

However, some platforms do not stop the TWD block in low-power mode, and can thus use the TWD timer in one-shot mode, without setting up a broadcast device.

Make the driver check for the "twd-never-stops" boolean property, and set the CLOCK_EVT_FEAT_C3STOP flag accordingly.

Signed-off-by Marc Gonzalez <[email protected]>

---
 Documentation/devicetree/bindings/arm/twd.txt | 4 ++++
 arch/arm/kernel/smp_twd.c                     | 5 ++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/arm/twd.txt +0 -0

index 75b8610939fa..700a517b000e 100644

Documentation/devicetree/bindings/arm/twd.txt interrupts. · line 24

Quoted 6 lines

Documentation/devicetree/bindings/arm/twd.txt +3 -0

@@ -19,6 +19,10 @@ interrupts.
 - reg : Specify the base address and the size of the TWD timer
 	register window.
 
+Optional
+- twd-never-stops : boolean property. If present, TWD timers are expected
+  to keep generating interrupts, even when the CPU is in low-power mode.

Sorry for the last minute bikeshed, but it would be better if we could align this with the ARM generic timer binding, both in naming and description:

- always-on : a boolean property. If present, the timer is powered through an always-on power domain, therefore it never loses context.

Otherwise this looks ok. If you can respin with the above wording, and s/twd-never-stops/always-on/ in the patch, you can add:

Acked-by Mark Rutland <[email protected]>

Quoted 24 lines

arch/arm/kernel/smp_twd.c +2 -1

index e8f6d241881f..79298d4fb41f 100644
@@ -33,6 +33,7 @@ static unsigned long twd_timer_rate;
 static DEFINE_PER_CPU(bool, percpu_setup_called);
 
 static struct clock_event_device __percpu *twd_evt;
+static int feat_c3stop;
 static int twd_ppi;
 
 static void twd_set_mode(enum clock_event_mode mode,

@@ -293,7 +294,7 @@ static void twd_timer_setup(void)
 
 	clk->name = "local_timer";
 	clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
-			CLOCK_EVT_FEAT_C3STOP;
+			feat_c3stop;
 	clk->rating = 350;
 	clk->set_mode = twd_set_mode;
 	clk->set_next_event = twd_set_next_event;

@@ -345,6 +346,8 @@ static int __init twd_local_timer_common_register(struct device_node *np)
 		goto out_irq;
 

arch/arm/kernel/smp_twd.c static int __init twd_local_timer_common_register(struct device_node *np) · line 350

Quoted 3 lines

arch/arm/kernel/smp_twd.c +2 -0

 	twd_get_clock(np);
+	if (!of_property_read_bool(np, "twd-never-stops"))
+		feat_c3stop = CLOCK_EVT_FEAT_C3STOP;

Quoted 4 lines · Mark Rutland

Otherwise this looks ok. If you can respin with the above wording, and s/twd-never-stops/always-on/ in the patch, you can add:

Acked-by Mark Rutland <[email protected]>

Wouldn't you feel like going all-in and Signing-off? ;-)

Quoted 4 lines · Mason

Quoted 3 lines · Mark Rutland

Otherwise this looks ok. If you can respin with the above wording, and s/twd-never-stops/always-on/ in the patch, you can add:

Quoted 3 lines

Quoted 1 line

Acked-by Mark Rutland <[email protected]>

Wouldn't you feel like going all-in and Signing-off? ;-)

Then I'd reject it, because it would be wrong.

Documentation/SubmittingPatches: [snip]

Quoted 7 lines · Russell King - ARM Linux

Quoted 6 lines · Mason

Quoted 4 lines · Mark Rutland

Otherwise this looks ok. If you can respin with the above wording, and s/twd-never-stops/always-on/ in the patch, you can add:

Acked-by Mark Rutland <[email protected]>

Quoted 5 lines

Quoted 1 line

Wouldn't you feel like going all-in and Signing-off? ;-)

Then I'd reject it, because it would be wrong.

Documentation/SubmittingPatches: [snip]

Could someone explain the difference between Acked-by and Reviewed-by?

Quoted 12 lines · Mason

Quoted 11 lines · Russell King - ARM Linux

Quoted 7 lines · Mason

Quoted 4 lines · Mark Rutland

Otherwise this looks ok. If you can respin with the above wording, and s/twd-never-stops/always-on/ in the patch, you can add:

Acked-by Mark Rutland <[email protected]>

Wouldn't you feel like going all-in and Signing-off? ;-)

Then I'd reject it, because it would be wrong.

Quoted 4 lines

Quoted 1 line

Documentation/SubmittingPatches: [snip]

Could someone explain the difference between Acked-by and Reviewed-by?

Again, I could quote from the above file, but you already have a copy of it locally, so you could just read that version instead. Section 13.

Is it possible that twd_local_timer_common_register() would be called more than once?

twd_local_timer_register() guards against multiple invocations, but twd_local_timer_of_register() doesn't (but maybe there is some guarantee from OF, or it's invalid to have multiple "arm,cortex-a9-twd-timer" strings in the DT).

Quoted 7 lines

Is it possible that twd_local_timer_common_register() would be called more than once?

twd_local_timer_register() guards against multiple invocations, but twd_local_timer_of_register() doesn't (but maybe there is some guarantee from OF, or it's invalid to have multiple "arm,cortex-a9-twd-timer" strings in the DT).

It does appear that were you to have have two or more "arm,cortex-a9-twd-timer" nodes in the DT, things would go wrong. As far as I am aware, that would result in multiple calls to twd_local_timer_of_register.

I don't think it would ever be valid to have two nodes for this particular device, but it's probably worth having the same check as twd_local_timer_register does, with a warning that there was more than one such node logged to the console.

Quoted 13 lines · Mark Rutland

Quoted 7 lines

Is it possible that twd_local_timer_common_register() would be called more than once?

twd_local_timer_register() guards against multiple invocations, but twd_local_timer_of_register() doesn't (but maybe there is some guarantee from OF, or it's invalid to have multiple "arm,cortex-a9-twd-timer" strings in the DT).

It does appear that were you to have have two or more "arm,cortex-a9-twd-timer" nodes in the DT, things would go wrong. As far as I am aware, that would result in multiple calls to twd_local_timer_of_register.

Quoted 4 lines

I don't think it would ever be valid to have two nodes for this particular device, but it's probably worth having the same check as twd_local_timer_register does, with a warning that there was more than one such node logged to the console.

Hmmm, I think I'll do it in a follow-up patch, once Felipe's revert hits mainline.

Sorry for the last minute bikeshed, but it would be better if we could align this with the ARM generic timer binding, both in naming and description: