[PATCH] clocksource: arch_timer: Mark ARMv8 system timer as 'always-on'

[PATCH] clocksource: arch_timer: Mark ARMv8 system timer as 'always-on'

[Anatol Pomozov]

Quoting ARMv8 Reference Manual section D6.1:
"The system counter must be implemented in an always-on power domain."

There is no need to keep 'always-on' configurable on ARMv8. We ignore
this dts property value and unconditionally set it to true.

The issue was discovered while working on ARMv8 board with only one timer
(arm arch timer). If 'always-on' is false then it disables high-resolution
timer functionality.

Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
---
 .../devicetree/bindings/arm/arch_timer.txt          |  8 ++++++--
 drivers/clocksource/arm_arch_timer.c                | 21 +++++++++++++++++----
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt
index 37b2caf..cb2d572 100644
--- a/Documentation/devicetree/bindings/arm/arch_timer.txt
+++ b/Documentation/devicetree/bindings/arm/arch_timer.txt
@@ -19,8 +19,12 @@ to deliver its interrupts via SPIs.
 
 - clock-frequency : The frequency of the main counter, in Hz. Optional.
 
-- always-on : a boolean property. If present, the timer is powered through an
-  always-on power domain, therefore it never loses context.
+- always-on : an "armv7-timer" specific boolean property.
+  If present, the timer is powered through an always-on power domain, therefore
+  it never loses context.
+  ARMv8 system timer is in always-on power domain (per Architecture Reference
+  section D6.1) thus this proprty is ignored for "arm,armv8-timer" compatible
+  timer.
 
 Example:
 
diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 2133f9d..610052d 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -719,13 +719,26 @@ static void __init arch_timer_init(struct device_node *np)
 		}
 	}
 
-	arch_timer_c3stop = !of_property_read_bool(np, "always-on");
-
 	arch_timer_register();
 	arch_timer_common_init();
 }
-CLOCKSOURCE_OF_DECLARE(armv7_arch_timer, "arm,armv7-timer", arch_timer_init);
-CLOCKSOURCE_OF_DECLARE(armv8_arch_timer, "arm,armv8-timer", arch_timer_init);
+
+static void __init v7_arch_timer_init(struct device_node *np)
+{
+	arch_timer_c3stop = !of_property_read_bool(np, "always-on");
+	arch_timer_init(np);
+}
+
+static void __init v8_arch_timer_init(struct device_node *np)
+{
+	/* Per ARMv8 Architecture Reference Manual section D6.1
+	 * the system timer is in always-on power domain.
+	 */
+	arch_timer_c3stop = false;
+	arch_timer_init(np);
+}
+CLOCKSOURCE_OF_DECLARE(armv7_arch_timer, "arm,armv7-timer", v7_arch_timer_init);
+CLOCKSOURCE_OF_DECLARE(armv8_arch_timer, "arm,armv8-timer", v8_arch_timer_init);
 
 static void __init arch_timer_mem_init(struct device_node *np)
 {
-- 
2.1.0.rc2.206.gedb03e5

Re: [PATCH] clocksource: arch_timer: Mark ARMv8 system timer as 'always-on'

[Marc Zyngier]

Hi Anatol,

On 2014-11-06 17:57, Anatol Pomozov wrote:
> Quoting ARMv8 Reference Manual section D6.1:
> "The system counter must be implemented in an always-on power 
> domain."

Yes. And the key words here are "system counter". That's the global 
counter, not the per-cpu view of that, and critically nor comparators.

The timers (per-cpu counter+comparator) will happily go down with the 
CPU, and won't be able to wake it up. Exactly like ARMv7, by the way.

> There is no need to keep 'always-on' configurable on ARMv8. We ignore
> this dts property value and unconditionally set it to true.
>
> The issue was discovered while working on ARMv8 board with only one 
> timer
> (arm arch timer). If 'always-on' is false then it disables 
> high-resolution
> timer functionality.

Yup, and that's by design (see above). The *only* case where it is 
really "always-on" is when a virtual machine is running (if the vcpu is 
running then the physical CPU is, and so is the timer).

If your system really never does any form of PM, then fine, just put 
the DT property in. Removing it is, I'm sorry to say, a bit short 
sighted.

Thanks,

         M.
-- 
Fast, cheap, reliable. Pick two.

Re: [PATCH] clocksource: arch_timer: Mark ARMv8 system timer as 'always-on'

[Anatol Pomozov]

Hi

On Thu, Nov 6, 2014 at 10:09 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> Hi Anatol,
>
> On 2014-11-06 17:57, Anatol Pomozov wrote:
>>
>> Quoting ARMv8 Reference Manual section D6.1:
>> "The system counter must be implemented in an always-on power domain."
>
>
> Yes. And the key words here are "system counter". That's the global counter,
> not the per-cpu view of that, and critically nor comparators.
>
> The timers (per-cpu counter+comparator) will happily go down with the CPU,
> and won't be able to wake it up. Exactly like ARMv7, by the way.
>
>> There is no need to keep 'always-on' configurable on ARMv8. We ignore
>> this dts property value and unconditionally set it to true.
>>
>> The issue was discovered while working on ARMv8 board with only one timer
>> (arm arch timer). If 'always-on' is false then it disables high-resolution
>> timer functionality.
>
>
> Yup, and that's by design (see above). The *only* case where it is really
> "always-on" is when a virtual machine is running (if the vcpu is running
> then the physical CPU is, and so is the timer).
>
> If your system really never does any form of PM, then fine, just put the DT
> property in. Removing it is, I'm sorry to say, a bit short sighted.

Thanks for the clarification Marc. I will look if it possible to get
the other (always-on) timer back.

Ignore my patch.

Re: [PATCH] clocksource: arch_timer: Mark ARMv8 system timer as 'always-on'

[Lorenzo Pieralisi]

On Thu, Nov 06, 2014 at 05:57:19PM +0000, Anatol Pomozov wrote:
> Quoting ARMv8 Reference Manual section D6.1:
> "The system counter must be implemented in an always-on power domain."

Do not mix up the system counter with arch timers. System counter is
always-on, but the arch timer(s) logic (that implements eg timers
comparators against the system counter value) might not be (timer logic
is in the processor), so it can be lost on processor(s) power down.

NAK.

Lorenzo

> There is no need to keep 'always-on' configurable on ARMv8. We ignore
> this dts property value and unconditionally set it to true.


> 
> The issue was discovered while working on ARMv8 board with only one timer
> (arm arch timer). If 'always-on' is false then it disables high-resolution
> timer functionality.
> 
> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
> ---
>  .../devicetree/bindings/arm/arch_timer.txt          |  8 ++++++--
>  drivers/clocksource/arm_arch_timer.c                | 21 +++++++++++++++++----
>  2 files changed, 23 insertions(+), 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt
> index 37b2caf..cb2d572 100644
> --- a/Documentation/devicetree/bindings/arm/arch_timer.txt
> +++ b/Documentation/devicetree/bindings/arm/arch_timer.txt
> @@ -19,8 +19,12 @@ to deliver its interrupts via SPIs.
>  
>  - clock-frequency : The frequency of the main counter, in Hz. Optional.
>  
> -- always-on : a boolean property. If present, the timer is powered through an
> -  always-on power domain, therefore it never loses context.
> +- always-on : an "armv7-timer" specific boolean property.
> +  If present, the timer is powered through an always-on power domain, therefore
> +  it never loses context.
> +  ARMv8 system timer is in always-on power domain (per Architecture Reference
> +  section D6.1) thus this proprty is ignored for "arm,armv8-timer" compatible
> +  timer.
>  
>  Example:
>  
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 2133f9d..610052d 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -719,13 +719,26 @@ static void __init arch_timer_init(struct device_node *np)
>  		}
>  	}
>  
> -	arch_timer_c3stop = !of_property_read_bool(np, "always-on");
> -
>  	arch_timer_register();
>  	arch_timer_common_init();
>  }
> -CLOCKSOURCE_OF_DECLARE(armv7_arch_timer, "arm,armv7-timer", arch_timer_init);
> -CLOCKSOURCE_OF_DECLARE(armv8_arch_timer, "arm,armv8-timer", arch_timer_init);
> +
> +static void __init v7_arch_timer_init(struct device_node *np)
> +{
> +	arch_timer_c3stop = !of_property_read_bool(np, "always-on");
> +	arch_timer_init(np);
> +}
> +
> +static void __init v8_arch_timer_init(struct device_node *np)
> +{
> +	/* Per ARMv8 Architecture Reference Manual section D6.1
> +	 * the system timer is in always-on power domain.
> +	 */
> +	arch_timer_c3stop = false;
> +	arch_timer_init(np);
> +}
> +CLOCKSOURCE_OF_DECLARE(armv7_arch_timer, "arm,armv7-timer", v7_arch_timer_init);
> +CLOCKSOURCE_OF_DECLARE(armv8_arch_timer, "arm,armv8-timer", v8_arch_timer_init);
>  
>  static void __init arch_timer_mem_init(struct device_node *np)
>  {
> -- 
> 2.1.0.rc2.206.gedb03e5
> 
> 

Re: [PATCH] clocksource: arch_timer: Mark ARMv8 system timer as 'always-on'

[Anatol Pomozov]

Hi

On Thu, Nov 6, 2014 at 10:23 AM, Lorenzo Pieralisi
<lorenzo.pieralisi@arm.com> wrote:
> On Thu, Nov 06, 2014 at 05:57:19PM +0000, Anatol Pomozov wrote:
>> Quoting ARMv8 Reference Manual section D6.1:
>> "The system counter must be implemented in an always-on power domain."
>
> Do not mix up the system counter with arch timers. System counter is
> always-on, but the arch timer(s) logic (that implements eg timers
> comparators against the system counter value) might not be (timer logic
> is in the processor), so it can be lost on processor(s) power down.

Thanks everyone for replies.

I am trying to enable tegra20_timer on my board. But this driver does
not compile because it uses headers from arch/arm. It particular it
needs register_persistent_clock() function. How I suppose to port
tegra20_timer to ARM64? Do I just need to duplicate the function
declaration/implementation to arch/arm64?

>
> NAK.
>
> Lorenzo
>
>> There is no need to keep 'always-on' configurable on ARMv8. We ignore
>> this dts property value and unconditionally set it to true.
>
>
>>
>> The issue was discovered while working on ARMv8 board with only one timer
>> (arm arch timer). If 'always-on' is false then it disables high-resolution
>> timer functionality.
>>
>> Signed-off-by: Anatol Pomozov <anatol.pomozov@gmail.com>
>> ---
>>  .../devicetree/bindings/arm/arch_timer.txt          |  8 ++++++--
>>  drivers/clocksource/arm_arch_timer.c                | 21 +++++++++++++++++----
>>  2 files changed, 23 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/arm/arch_timer.txt b/Documentation/devicetree/bindings/arm/arch_timer.txt
>> index 37b2caf..cb2d572 100644
>> --- a/Documentation/devicetree/bindings/arm/arch_timer.txt
>> +++ b/Documentation/devicetree/bindings/arm/arch_timer.txt
>> @@ -19,8 +19,12 @@ to deliver its interrupts via SPIs.
>>
>>  - clock-frequency : The frequency of the main counter, in Hz. Optional.
>>
>> -- always-on : a boolean property. If present, the timer is powered through an
>> -  always-on power domain, therefore it never loses context.
>> +- always-on : an "armv7-timer" specific boolean property.
>> +  If present, the timer is powered through an always-on power domain, therefore
>> +  it never loses context.
>> +  ARMv8 system timer is in always-on power domain (per Architecture Reference
>> +  section D6.1) thus this proprty is ignored for "arm,armv8-timer" compatible
>> +  timer.
>>
>>  Example:
>>
>> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
>> index 2133f9d..610052d 100644
>> --- a/drivers/clocksource/arm_arch_timer.c
>> +++ b/drivers/clocksource/arm_arch_timer.c
>> @@ -719,13 +719,26 @@ static void __init arch_timer_init(struct device_node *np)
>>               }
>>       }
>>
>> -     arch_timer_c3stop = !of_property_read_bool(np, "always-on");
>> -
>>       arch_timer_register();
>>       arch_timer_common_init();
>>  }
>> -CLOCKSOURCE_OF_DECLARE(armv7_arch_timer, "arm,armv7-timer", arch_timer_init);
>> -CLOCKSOURCE_OF_DECLARE(armv8_arch_timer, "arm,armv8-timer", arch_timer_init);
>> +
>> +static void __init v7_arch_timer_init(struct device_node *np)
>> +{
>> +     arch_timer_c3stop = !of_property_read_bool(np, "always-on");
>> +     arch_timer_init(np);
>> +}
>> +
>> +static void __init v8_arch_timer_init(struct device_node *np)
>> +{
>> +     /* Per ARMv8 Architecture Reference Manual section D6.1
>> +      * the system timer is in always-on power domain.
>> +      */
>> +     arch_timer_c3stop = false;
>> +     arch_timer_init(np);
>> +}
>> +CLOCKSOURCE_OF_DECLARE(armv7_arch_timer, "arm,armv7-timer", v7_arch_timer_init);
>> +CLOCKSOURCE_OF_DECLARE(armv8_arch_timer, "arm,armv8-timer", v8_arch_timer_init);
>>
>>  static void __init arch_timer_mem_init(struct device_node *np)
>>  {
>> --
>> 2.1.0.rc2.206.gedb03e5
>>
>>