[PATCH] twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally

[PATCH] twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally

[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 it 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 <marc_gonzalez@sigmadesigns.com>
---
 arch/arm/kernel/smp_twd.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index e8f6d241881f..0ca825e24d9c 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -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
-- 
2.4.5

[PATCH v2] twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally

[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 it 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 <marc_gonzalez@sigmadesigns.com>
---
 Documentation/devicetree/bindings/arm/twd.txt | 3 +++
 arch/arm/kernel/smp_twd.c                     | 5 ++++-
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/arm/twd.txt b/Documentation/devicetree/bindings/arm/twd.txt
index 75b8610939fa..7724665a437d 100644
--- a/Documentation/devicetree/bindings/arm/twd.txt
+++ b/Documentation/devicetree/bindings/arm/twd.txt
@@ -19,6 +19,9 @@ interrupts.
 - reg : Specify the base address and the size of the TWD timer
 	register window.
 
+Optional
+- twd_never_stops : Don't set the CLOCK_EVT_FEAT_C3STOP flag.
+
 Example:
 
 	twd-timer at 2c000600 {
diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index e8f6d241881f..0ca825e24d9c 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -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
-- 
2.4.5

[PATCH v2] twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally

[Linus Walleij]

On Mon, Oct 5, 2015 at 11:17 AM, Marc Gonzalez
<marc_gonzalez@sigmadesigns.com> wrote:

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

We don't use _underscores_ much in bindings.

twd-never-stops; is more common.

Write that this is a bool flag.

Yours,
Linus Walleij

[PATCH v3] twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally

[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 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 <marc_gonzalez@sigmadesigns.com>
---
 Documentation/devicetree/bindings/arm/twd.txt | 5 +++++
 arch/arm/kernel/smp_twd.c                     | 5 ++++-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/arm/twd.txt b/Documentation/devicetree/bindings/arm/twd.txt
index 75b8610939fa..7f849235e12a 100644
--- a/Documentation/devicetree/bindings/arm/twd.txt
+++ b/Documentation/devicetree/bindings/arm/twd.txt
@@ -19,6 +19,11 @@ interrupts.
 - reg : Specify the base address and the size of the TWD timer
 	register window.
 
+Optional
+- twd-never-stops : boolean property. If present, the twd driver will not set
+  the CLOCK_EVT_FEAT_C3STOP flag. TWD timer can then operate in one-shot mode,
+  no broadcast device required.
+
 Example:
 
 	twd-timer at 2c000600 {
diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index e8f6d241881f..79298d4fb41f 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -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
-- 
2.4.5

[PATCH v3] twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally

[Mark Rutland]

On Mon, Oct 05, 2015 at 11:50:23AM +0200, Marc Gonzalez wrote:
> 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 <marc_gonzalez@sigmadesigns.com>
> ---
>  Documentation/devicetree/bindings/arm/twd.txt | 5 +++++
>  arch/arm/kernel/smp_twd.c                     | 5 ++++-
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/twd.txt b/Documentation/devicetree/bindings/arm/twd.txt
> index 75b8610939fa..7f849235e12a 100644
> --- a/Documentation/devicetree/bindings/arm/twd.txt
> +++ b/Documentation/devicetree/bindings/arm/twd.txt
> @@ -19,6 +19,11 @@ interrupts.
>  - reg : Specify the base address and the size of the TWD timer
>  	register window.
>  
> +Optional
> +- twd-never-stops : boolean property. If present, the twd driver will not set
> +  the CLOCK_EVT_FEAT_C3STOP flag. TWD timer can then operate in one-shot mode,
> +  no broadcast device required.

Please describe the actual physical details (i.e. that the TWD cannot be
placed into a state where it cannot generate interrupts) rather than
describing the Linux implementation details.

Otherwise this looks ok.

Mark.

> +
>  Example:
>  
>  	twd-timer at 2c000600 {
> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
> index e8f6d241881f..79298d4fb41f 100644
> --- a/arch/arm/kernel/smp_twd.c
> +++ b/arch/arm/kernel/smp_twd.c
> @@ -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
> -- 
> 2.4.5
> 

[PATCH v4] twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally

[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 <marc_gonzalez@sigmadesigns.com>
---
 Documentation/devicetree/bindings/arm/twd.txt | 4 ++++
 arch/arm/kernel/smp_twd.c                     | 5 ++++-
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/arm/twd.txt b/Documentation/devicetree/bindings/arm/twd.txt
index 75b8610939fa..700a517b000e 100644
--- a/Documentation/devicetree/bindings/arm/twd.txt
+++ b/Documentation/devicetree/bindings/arm/twd.txt
@@ -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.
+
 Example:
 
 	twd-timer at 2c000600 {
diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
index e8f6d241881f..79298d4fb41f 100644
--- a/arch/arm/kernel/smp_twd.c
+++ b/arch/arm/kernel/smp_twd.c
@@ -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
-- 
2.4.5

[PATCH v4] twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally

[Marc Gonzalez]

On 05/10/2015 13:53, Marc Gonzalez wrote:
> 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 <marc_gonzalez@sigmadesigns.com>
> ---
>  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/

Regards.

[PATCH v4] twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally

[Marc Gonzalez]

On 05/10/2015 18:37, Marc Gonzalez wrote:
> On 05/10/2015 13:53, Marc Gonzalez wrote:
>> 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 <marc_gonzalez@sigmadesigns.com>
>> ---
>>  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/

Submitted as patch 8441/1
http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8441/1

[PATCH v4] twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally

[Mark Rutland]

On Mon, Oct 05, 2015 at 01:53:00PM +0200, Marc Gonzalez wrote:
> 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 <marc_gonzalez@sigmadesigns.com>
> ---
>  Documentation/devicetree/bindings/arm/twd.txt | 4 ++++
>  arch/arm/kernel/smp_twd.c                     | 5 ++++-
>  2 files changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/twd.txt b/Documentation/devicetree/bindings/arm/twd.txt
> index 75b8610939fa..700a517b000e 100644
> --- a/Documentation/devicetree/bindings/arm/twd.txt
> +++ b/Documentation/devicetree/bindings/arm/twd.txt
> @@ -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 <mark.rutland@arm.com>

Thanks,
Mark.

> +
>  Example:
>  
>  	twd-timer at 2c000600 {
> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
> index e8f6d241881f..79298d4fb41f 100644
> --- a/arch/arm/kernel/smp_twd.c
> +++ b/arch/arm/kernel/smp_twd.c
> @@ -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
> -- 
> 2.4.5
> 

[PATCH v4] twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally

[Mason]

On 08/10/2015 19:16, Mark Rutland wrote:
> On Mon, Oct 05, 2015 at 01:53:00PM +0200, Marc Gonzalez wrote:
>> 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 <marc_gonzalez@sigmadesigns.com>
>> ---
>>  Documentation/devicetree/bindings/arm/twd.txt | 4 ++++
>>  arch/arm/kernel/smp_twd.c                     | 5 ++++-
>>  2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/twd.txt b/Documentation/devicetree/bindings/arm/twd.txt
>> index 75b8610939fa..700a517b000e 100644
>> --- a/Documentation/devicetree/bindings/arm/twd.txt
>> +++ b/Documentation/devicetree/bindings/arm/twd.txt
>> @@ -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.

No problemo. Too bad I didn't think to copy that description
in the first place!

> 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 <mark.rutland@arm.com>

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

One last nit (see below).

>> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c
>> index e8f6d241881f..79298d4fb41f 100644
>> --- a/arch/arm/kernel/smp_twd.c
>> +++ b/arch/arm/kernel/smp_twd.c
>> @@ -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;

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).

Regards.

[PATCH v4] twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally

[Russell King - ARM Linux]

On Thu, Oct 08, 2015 at 07:38:53PM +0200, Mason wrote:
> On 08/10/2015 19:16, Mark Rutland wrote:
> > 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 <mark.rutland@arm.com>
> 
> Wouldn't you feel like going all-in and Signing-off? ;-)

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

Documentation/SubmittingPatches:

12) When to use Acked-by: and Cc:
---------------------------------

The Signed-off-by: tag indicates that the signer was involved in the
development of the patch, or that he/she was in the patch's delivery path.

If a person was not directly involved in the preparation or handling of a
patch but wishes to signify and record their approval of it then they can
ask to have an Acked-by: line added to the patch's changelog.

Acked-by: is often used by the maintainer of the affected code when that
maintainer neither contributed to nor forwarded the patch.

Acked-by: is not as formal as Signed-off-by:.  It is a record that the acker
has at least reviewed the patch and has indicated acceptance.  Hence patch
mergers will sometimes manually convert an acker's "yep, looks good to me"
into an Acked-by: (but note that it is usually better to ask for an
explicit ack).

Acked-by: does not necessarily indicate acknowledgement of the entire patch.
For example, if a patch affects multiple subsystems and has an Acked-by: from
one subsystem maintainer then this usually indicates acknowledgement of just
the part which affects that maintainer's code.  Judgement should be used here.
When in doubt people should refer to the original discussion in the mailing
list archives.


-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH v4] twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally

[Mason]

On 08/10/2015 19:43, Russell King - ARM Linux wrote:
> On Thu, Oct 08, 2015 at 07:38:53PM +0200, Mason wrote:
>> On 08/10/2015 19:16, Mark Rutland wrote:
>>> 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 <mark.rutland@arm.com>
>>
>> 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?

Regards.

[PATCH v4] twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally

[Russell King - ARM Linux]

On Thu, Oct 08, 2015 at 08:16:24PM +0200, Mason wrote:
> On 08/10/2015 19:43, Russell King - ARM Linux wrote:
> > On Thu, Oct 08, 2015 at 07:38:53PM +0200, Mason wrote:
> >> On 08/10/2015 19:16, Mark Rutland wrote:
> >>> 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 <mark.rutland@arm.com>
> >>
> >> 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?

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.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH v4] twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally

[Mason]

On 08/10/2015 20:22, Russell King - ARM Linux wrote:
> On Thu, Oct 08, 2015 at 08:16:24PM +0200, Mason wrote:
>> On 08/10/2015 19:43, Russell King - ARM Linux wrote:
>>> On Thu, Oct 08, 2015 at 07:38:53PM +0200, Mason wrote:
>>>> On 08/10/2015 19:16, Mark Rutland wrote:
>>>>> 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 <mark.rutland@arm.com>
>>>>
>>>> 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?
> 
> 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.

Acked-by: [...] is a record that the acker has at least
reviewed the patch and has indicated acceptance.

Reviewed-by: indicates that the patch has been reviewed and
found acceptable according to the Reviewer's Statement

IIUC Acked-by is stronger than Reviewed-by?

Regards.

[PATCH v4] twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally

[Mark Rutland]

> 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.

Thanks,
Mark

[PATCH v4] twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally

[Mason]

On 08/10/2015 19:57, Mark Rutland wrote:
>> 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.

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

Regards.

[PATCH v4] twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally

[Mark Rutland]

On Thu, Oct 08, 2015 at 08:25:07PM +0200, Mason wrote:
> On 08/10/2015 19:57, Mark Rutland wrote:
> >> 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.
> 
> Hmmm, I think I'll do it in a follow-up patch, once Felipe's
> revert hits mainline.

Sure, this is certainly independent of the CLOCK_EVT_FEAT_C3STOP patch.

Thanks,
Mark.

[PATCH v4] twd: Don't set CLOCK_EVT_FEAT_C3STOP unconditionally

[Marc Gonzalez]

On 08/10/2015 19:16, Mark Rutland wrote:

> 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:

Updated patch has been sent to Russell's patch mill.

http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8441/2

Regards.