[PATCH v2] clocksource: arm_global_timer: fix suspend resume

[PATCH v2] clocksource: arm_global_timer: fix suspend resume

[Grygorii Strashko]

Now the System stall is observed on TI AM437x based board
(am437x-gp-evm) during resuming from System suspend when ARM Global
timer is selected as clocksource device - SysRq are working, but
nothing else. The reason of stall is that ARM Global timer loses its
contexts.

The reason of stall is that ARM Global timer loses its contexts during
System suspend:
   GT_CONTROL.TIMER_ENABLE = 0 (unbanked)
   GT_COUNTERx = 0

Hence, update ARM Global timer driver to reflect above behaviour
- re-enable ARM Global timer on resume GT_CONTROL.TIMER_ENABLE = 1
- ensure clocksource and clockevent devices have coresponding flags
  (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set
  depending on presence of "always-on" DT property.

CC: Arnd Bergmann <arnd@arndb.de>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Tony Lindgren <tony@atomide.com>
Cc: Santosh Shilimkar <ssantosh@kernel.org>
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
Changes in v2:
 - suspend/resume simplified: nothing is stored any more and
   ARM GT just re-enabled
Link on v1:
 https://lkml.org/lkml/2015/11/13/456

 drivers/clocksource/arm_global_timer.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
index a2cb6fa..867e546 100644
--- a/drivers/clocksource/arm_global_timer.c
+++ b/drivers/clocksource/arm_global_timer.c
@@ -51,6 +51,7 @@ static void __iomem *gt_base;
 static unsigned long gt_clk_rate;
 static int gt_ppi;
 static struct clock_event_device __percpu *gt_evt;
+static bool gt_always_on;
 
 /*
  * To get the value from the Global Timer Counter register proceed as follows:
@@ -168,6 +169,9 @@ static int gt_clockevents_init(struct clock_event_device *clk)
 {
 	int cpu = smp_processor_id();
 
+	if (!gt_always_on)
+		clk->features |= CLOCK_EVT_FEAT_C3STOP;
+
 	clk->name = "arm_global_timer";
 	clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
 		CLOCK_EVT_FEAT_PERCPU;
@@ -195,12 +199,19 @@ static cycle_t gt_clocksource_read(struct clocksource *cs)
 	return gt_counter_read();
 }
 
+static void gt_resume(struct clocksource *cs)
+{
+	/* re-enable timer on resume */
+	writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
+}
+
 static struct clocksource gt_clocksource = {
 	.name	= "arm_global_timer",
 	.rating	= 300,
 	.read	= gt_clocksource_read,
 	.mask	= CLOCKSOURCE_MASK(64),
 	.flags	= CLOCK_SOURCE_IS_CONTINUOUS,
+	.resume = gt_resume,
 };
 
 #ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
@@ -218,6 +229,9 @@ static void __init gt_clocksource_init(void)
 	/* enables timer on all the cores */
 	writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
 
+	if (gt_always_on)
+		gt_clocksource.flags |= CLOCK_SOURCE_SUSPEND_NONSTOP;
+
 #ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
 	sched_clock_register(gt_sched_clock_read, 64, gt_clk_rate);
 #endif
@@ -289,6 +303,8 @@ static void __init global_timer_of_register(struct device_node *np)
 		goto out_clk;
 	}
 
+	gt_always_on = of_property_read_bool(np, "always-on");
+
 	err = request_percpu_irq(gt_ppi, gt_clockevent_interrupt,
 				 "gt", gt_evt);
 	if (err) {
-- 
2.6.3

Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume

[santosh shilimkar]

+ Thomas, Marc

On 11/20/2015 5:57 AM, Grygorii Strashko wrote:
> Now the System stall is observed on TI AM437x based board
> (am437x-gp-evm) during resuming from System suspend when ARM Global
> timer is selected as clocksource device - SysRq are working, but
> nothing else. The reason of stall is that ARM Global timer loses its
> contexts.
>
> The reason of stall is that ARM Global timer loses its contexts during
> System suspend:
>     GT_CONTROL.TIMER_ENABLE = 0 (unbanked)
>     GT_COUNTERx = 0
>
> Hence, update ARM Global timer driver to reflect above behaviour
> - re-enable ARM Global timer on resume GT_CONTROL.TIMER_ENABLE = 1
> - ensure clocksource and clockevent devices have coresponding flags
>    (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set
>    depending on presence of "always-on" DT property.
>
Something which loses context in low power states can't be
called "always-on"

Also if the clock-soucre can't tick in the low power states
then that device shouldn't be used as a clock-source.

> CC: Arnd Bergmann <arnd@arndb.de>
> Cc: John Stultz <john.stultz@linaro.org>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: Tony Lindgren <tony@atomide.com>
> Cc: Santosh Shilimkar <ssantosh@kernel.org>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> ---
> Changes in v2:
>   - suspend/resume simplified: nothing is stored any more and
>     ARM GT just re-enabled
> Link on v1:
>   https://lkml.org/lkml/2015/11/13/456
>
>   drivers/clocksource/arm_global_timer.c | 16 ++++++++++++++++
>   1 file changed, 16 insertions(+)
>
> diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c
> index a2cb6fa..867e546 100644
> --- a/drivers/clocksource/arm_global_timer.c
> +++ b/drivers/clocksource/arm_global_timer.c
> @@ -51,6 +51,7 @@ static void __iomem *gt_base;
>   static unsigned long gt_clk_rate;
>   static int gt_ppi;
>   static struct clock_event_device __percpu *gt_evt;
> +static bool gt_always_on;
>
>   /*
>    * To get the value from the Global Timer Counter register proceed as follows:
> @@ -168,6 +169,9 @@ static int gt_clockevents_init(struct clock_event_device *clk)
>   {
>   	int cpu = smp_processor_id();
>
> +	if (!gt_always_on)
> +		clk->features |= CLOCK_EVT_FEAT_C3STOP;
> +
>   	clk->name = "arm_global_timer";
>   	clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
>   		CLOCK_EVT_FEAT_PERCPU;
> @@ -195,12 +199,19 @@ static cycle_t gt_clocksource_read(struct clocksource *cs)
>   	return gt_counter_read();
>   }
>
> +static void gt_resume(struct clocksource *cs)
> +{
> +	/* re-enable timer on resume */
> +	writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
> +}
> +
>   static struct clocksource gt_clocksource = {
>   	.name	= "arm_global_timer",
>   	.rating	= 300,
>   	.read	= gt_clocksource_read,
>   	.mask	= CLOCKSOURCE_MASK(64),
>   	.flags	= CLOCK_SOURCE_IS_CONTINUOUS,
> +	.resume = gt_resume,
>   };
>
>   #ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
> @@ -218,6 +229,9 @@ static void __init gt_clocksource_init(void)
>   	/* enables timer on all the cores */
>   	writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
>
> +	if (gt_always_on)
> +		gt_clocksource.flags |= CLOCK_SOURCE_SUSPEND_NONSTOP;
> +
>   #ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
>   	sched_clock_register(gt_sched_clock_read, 64, gt_clk_rate);
>   #endif
> @@ -289,6 +303,8 @@ static void __init global_timer_of_register(struct device_node *np)
>   		goto out_clk;
>   	}
>
> +	gt_always_on = of_property_read_bool(np, "always-on");
> +
>   	err = request_percpu_irq(gt_ppi, gt_clockevent_interrupt,
>   				 "gt", gt_evt);
>   	if (err) {
>
Am really confused with this patch. Because 'C3STOP' is a clock-event
flag which we use for cases where we have back-up broadcast timer which
continue to tick in low power states.

If the ARM global timer is considered as that device which actually
doesn't tick then we are doomed. Clocksource device must be *CONTINUOUS*
and if it is not then its not a viable device to be used as clock-source.

May be am missing the context but this whole patch doesn't make sense
to me.

Regards,
Santosh

Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume

[Grygorii Strashko]

Hi Santosh,

On 11/20/2015 07:23 PM, santosh shilimkar wrote:
> + Thomas, Marc
> 
> On 11/20/2015 5:57 AM, Grygorii Strashko wrote:
>> Now the System stall is observed on TI AM437x based board
>> (am437x-gp-evm) during resuming from System suspend when ARM Global
>> timer is selected as clocksource device - SysRq are working, but
>> nothing else. The reason of stall is that ARM Global timer loses its
>> contexts.
>>
>> The reason of stall is that ARM Global timer loses its contexts during
>> System suspend:
>>     GT_CONTROL.TIMER_ENABLE = 0 (unbanked)
>>     GT_COUNTERx = 0
>>
>> Hence, update ARM Global timer driver to reflect above behaviour
>> - re-enable ARM Global timer on resume GT_CONTROL.TIMER_ENABLE = 1
>> - ensure clocksource and clockevent devices have coresponding flags
>>    (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set
>>    depending on presence of "always-on" DT property.
>>
> Something which loses context in low power states can't be
> called "always-on"

Sry, it's kinda new area for me and I could make mistakes.

While working on this patch I've:
 - re-used implementation from ARM arch timer 
commit 82a5619410d4c4df65c04272db198eca5a867c18
Author: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Date:   Tue Apr 8 10:04:32 2014 +0100

    clocksource: arch_arm_timer: Fix age-old arch timer C3STOP detection issue


- and followed timekeeping.txt:
"Typically the clock source is a monotonic, atomic counter which will provide
 n bits which count from 0 to 2^(n-1) and then wraps around to 0 and start over.
It will ideally NEVER stop ticking as long as the system is running. It
may stop during system suspend."
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

And that exactly my use-case: I'd like to use ARM GT as clocksource
and with CPUIdle = n. But System suspend has to be allowed.


> 
> Also if the clock-soucre can't tick in the low power states
> then that device shouldn't be used as a clock-source.

Agree. clocksource can't (except with suspend). Have I missed something? 

> 
>> CC: Arnd Bergmann <arnd@arndb.de>
>> Cc: John Stultz <john.stultz@linaro.org>
>> Cc: Felipe Balbi <balbi@ti.com>
>> Cc: Tony Lindgren <tony@atomide.com>
>> Cc: Santosh Shilimkar <ssantosh@kernel.org>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> ---
>> Changes in v2:
>>   - suspend/resume simplified: nothing is stored any more and
>>     ARM GT just re-enabled
>> Link on v1:
>>   https://lkml.org/lkml/2015/11/13/456
>>
>>   drivers/clocksource/arm_global_timer.c | 16 ++++++++++++++++
>>   1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/clocksource/arm_global_timer.c 
>> b/drivers/clocksource/arm_global_timer.c
>> index a2cb6fa..867e546 100644
>> --- a/drivers/clocksource/arm_global_timer.c
>> +++ b/drivers/clocksource/arm_global_timer.c
>> @@ -51,6 +51,7 @@ static void __iomem *gt_base;
>>   static unsigned long gt_clk_rate;
>>   static int gt_ppi;
>>   static struct clock_event_device __percpu *gt_evt;
>> +static bool gt_always_on;
>>
>>   /*
>>    * To get the value from the Global Timer Counter register proceed 
>> as follows:
>> @@ -168,6 +169,9 @@ static int gt_clockevents_init(struct 
>> clock_event_device *clk)
>>   {
>>       int cpu = smp_processor_id();
>>
>> +    if (!gt_always_on)
>> +        clk->features |= CLOCK_EVT_FEAT_C3STOP;
>> +
I can drop ^

>>       clk->name = "arm_global_timer";
>>       clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
>>           CLOCK_EVT_FEAT_PERCPU;

and change this as
       clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT |
           CLOCK_EVT_FEAT_PERCPU | CLOCK_EVT_FEAT_C3STOP;

>> @@ -195,12 +199,19 @@ static cycle_t gt_clocksource_read(struct 
>> clocksource *cs)
>>       return gt_counter_read();
>>   }
>>
>> +static void gt_resume(struct clocksource *cs)
>> +{
>> +    /* re-enable timer on resume */
>> +    writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
>> +}
>> +
>>   static struct clocksource gt_clocksource = {
>>       .name    = "arm_global_timer",
>>       .rating    = 300,
>>       .read    = gt_clocksource_read,
>>       .mask    = CLOCKSOURCE_MASK(64),
>>       .flags    = CLOCK_SOURCE_IS_CONTINUOUS,
>> +    .resume = gt_resume,
>>   };
>>
>>   #ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
>> @@ -218,6 +229,9 @@ static void __init gt_clocksource_init(void)
>>       /* enables timer on all the cores */
>>       writel(GT_CONTROL_TIMER_ENABLE, gt_base + GT_CONTROL);
>>
>> +    if (gt_always_on)
>> +        gt_clocksource.flags |= CLOCK_SOURCE_SUSPEND_NONSTOP;
>> +

Or, may just drop only this? ^

>>   #ifdef CONFIG_CLKSRC_ARM_GLOBAL_TIMER_SCHED_CLOCK
>>       sched_clock_register(gt_sched_clock_read, 64, gt_clk_rate);
>>   #endif
>> @@ -289,6 +303,8 @@ static void __init global_timer_of_register(struct 
>> device_node *np)
>>           goto out_clk;
>>       }
>>
>> +    gt_always_on = of_property_read_bool(np, "always-on");
>> +
>>       err = request_percpu_irq(gt_ppi, gt_clockevent_interrupt,
>>                    "gt", gt_evt);
>>       if (err) {
>>
> Am really confused with this patch. Because 'C3STOP' is a clock-event
> flag which we use for cases where we have back-up broadcast timer which
> continue to tick in low power states.

Yes. And it will work with CPUIDLE only together with OMAP GP timer,
which will be used as broadcast timer (usual configuration for OMAP4
where TWD is used as clockevent, OMAP GP timer as broadcast device and
OMAP 32ksynctimer/counter_32 as clocksource).
Internally I had to fix cpuidle driver to make it work properly with
multiplatform/muliSoC build - 
https://git.ti.com/~gragst/ti-linux-kernel/gragsts-ti-linux-kernel/commit/143572633ff418548390827265d270a0cc915a81

The issue with ARM GT was discovered when It was enabled om AM437x SoC
(which is UP). And, again, my target is CPUIDLE=n and SUSPEND=y.

> 
> If the ARM global timer is considered as that device which actually
> doesn't tick then we are doomed. Clocksource device must be *CONTINUOUS*
> and if it is not then its not a viable device to be used as clock-source.
> 
> May be am missing the context but this whole patch doesn't make sense
> to me.

If it's better, I can drop "always-on" related code and just add
CLOCK_EVT_FEAT_C3STOP to clock_event_device gt_evt by default (as it's done
in arch/arm/kernel/smp_twd.c).

-- 
regards,
-grygorii

Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume

[Marc Zyngier]

On 20/11/15 18:35, Grygorii Strashko wrote:
> Hi Santosh,
> 
> On 11/20/2015 07:23 PM, santosh shilimkar wrote:
>> + Thomas, Marc
>>
>> On 11/20/2015 5:57 AM, Grygorii Strashko wrote:
>>> Now the System stall is observed on TI AM437x based board
>>> (am437x-gp-evm) during resuming from System suspend when ARM Global
>>> timer is selected as clocksource device - SysRq are working, but
>>> nothing else. The reason of stall is that ARM Global timer loses its
>>> contexts.
>>>
>>> The reason of stall is that ARM Global timer loses its contexts during
>>> System suspend:
>>>     GT_CONTROL.TIMER_ENABLE = 0 (unbanked)
>>>     GT_COUNTERx = 0
>>>
>>> Hence, update ARM Global timer driver to reflect above behaviour
>>> - re-enable ARM Global timer on resume GT_CONTROL.TIMER_ENABLE = 1
>>> - ensure clocksource and clockevent devices have coresponding flags
>>>    (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set
>>>    depending on presence of "always-on" DT property.
>>>
>> Something which loses context in low power states can't be
>> called "always-on"
> 
> Sry, it's kinda new area for me and I could make mistakes.
> 
> While working on this patch I've:
>  - re-used implementation from ARM arch timer 
> commit 82a5619410d4c4df65c04272db198eca5a867c18
> Author: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Date:   Tue Apr 8 10:04:32 2014 +0100
> 
>     clocksource: arch_arm_timer: Fix age-old arch timer C3STOP detection issue

[...]

This patch has a very specific purpose: instructing the core code that
this timer will never stop ticking, ever. It is really targeted at
virtual machines, whose timer is backed by the host timer, even when the
VM is not running.

Using it on actual hardware is may not be the best idea, specially in
the presence of PM.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume

[santosh shilimkar]

On 11/20/2015 10:46 AM, Marc Zyngier wrote:
> On 20/11/15 18:35, Grygorii Strashko wrote:
>> Hi Santosh,
>>
>> On 11/20/2015 07:23 PM, santosh shilimkar wrote:
>>> + Thomas, Marc
>>>
>>> On 11/20/2015 5:57 AM, Grygorii Strashko wrote:
>>>> Now the System stall is observed on TI AM437x based board
>>>> (am437x-gp-evm) during resuming from System suspend when ARM Global
>>>> timer is selected as clocksource device - SysRq are working, but
>>>> nothing else. The reason of stall is that ARM Global timer loses its
>>>> contexts.
>>>>
>>>> The reason of stall is that ARM Global timer loses its contexts during
>>>> System suspend:
>>>>      GT_CONTROL.TIMER_ENABLE = 0 (unbanked)
>>>>      GT_COUNTERx = 0
>>>>
>>>> Hence, update ARM Global timer driver to reflect above behaviour
>>>> - re-enable ARM Global timer on resume GT_CONTROL.TIMER_ENABLE = 1
>>>> - ensure clocksource and clockevent devices have coresponding flags
>>>>     (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set
>>>>     depending on presence of "always-on" DT property.
>>>>
>>> Something which loses context in low power states can't be
>>> called "always-on"
>>
>> Sry, it's kinda new area for me and I could make mistakes.
>>
>> While working on this patch I've:
>>   - re-used implementation from ARM arch timer
>> commit 82a5619410d4c4df65c04272db198eca5a867c18
>> Author: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Date:   Tue Apr 8 10:04:32 2014 +0100
>>
>>      clocksource: arch_arm_timer: Fix age-old arch timer C3STOP detection issue
>
> [...]
>
> This patch has a very specific purpose: instructing the core code that
> this timer will never stop ticking, ever. It is really targeted at
> virtual machines, whose timer is backed by the host timer, even when the
> VM is not running.
>
> Using it on actual hardware is may not be the best idea, specially in
> the presence of PM.
>
Exactly. Thanks for clarifying the commit Marc.

Regards,
Santosh

Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume

[John Stultz]

On Fri, Nov 20, 2015 at 10:35 AM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> Hi Santosh,
>
> On 11/20/2015 07:23 PM, santosh shilimkar wrote:
>> + Thomas, Marc
>>
>> On 11/20/2015 5:57 AM, Grygorii Strashko wrote:
>>> Now the System stall is observed on TI AM437x based board
>>> (am437x-gp-evm) during resuming from System suspend when ARM Global
>>> timer is selected as clocksource device - SysRq are working, but
>>> nothing else. The reason of stall is that ARM Global timer loses its
>>> contexts.
>>>
>>> The reason of stall is that ARM Global timer loses its contexts during
>>> System suspend:
>>>     GT_CONTROL.TIMER_ENABLE = 0 (unbanked)
>>>     GT_COUNTERx = 0
>>>
>>> Hence, update ARM Global timer driver to reflect above behaviour
>>> - re-enable ARM Global timer on resume GT_CONTROL.TIMER_ENABLE = 1
>>> - ensure clocksource and clockevent devices have coresponding flags
>>>    (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set
>>>    depending on presence of "always-on" DT property.
>>>
>> Something which loses context in low power states can't be
>> called "always-on"
>
> Sry, it's kinda new area for me and I could make mistakes.
>
> While working on this patch I've:
>  - re-used implementation from ARM arch timer
> commit 82a5619410d4c4df65c04272db198eca5a867c18
> Author: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Date:   Tue Apr 8 10:04:32 2014 +0100
>
>     clocksource: arch_arm_timer: Fix age-old arch timer C3STOP detection issue
>
>
> - and followed timekeeping.txt:
> "Typically the clock source is a monotonic, atomic counter which will provide
>  n bits which count from 0 to 2^(n-1) and then wraps around to 0 and start over.
> It will ideally NEVER stop ticking as long as the system is running. It
> may stop during system suspend."
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>
> And that exactly my use-case: I'd like to use ARM GT as clocksource
> and with CPUIdle = n. But System suspend has to be allowed.
>
>
>>
>> Also if the clock-soucre can't tick in the low power states
>> then that device shouldn't be used as a clock-source.
>
> Agree. clocksource can't (except with suspend). Have I missed something?

I think the point Stantosh is making is that if the clocksource stops
in suspend (which is allowed) you should not be setting
CLOCK_SOURCE_SUSPEND_NONSTOP (which promises the clocksource doesn't
stop in suspend, so it can be used for suspend timing as well).

The contradictory part in your patch is that you're also setting the
clockevent logic as  CLOCK_EVT_FEAT_C3STOP, which flags that the
clockevent hardware might stop in low-power idle states (ie: not
suspend, but while the system is running).  Usually hardware that
halts in low-power mode (like the apic on some x86 machines) is not
also used for timekeeping (instead we use the hpet/acpi there).

So its unlikely that the hardware both stays running through suspend,
but also might halt in idle. That would be "unique".

thanks
-john

Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume

[John Stultz]

On Fri, Nov 20, 2015 at 10:46 AM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>
> This patch has a very specific purpose: instructing the core code that
> this timer will never stop ticking, ever. It is really targeted at
> virtual machines, whose timer is backed by the host timer, even when the
> VM is not running.
>
> Using it on actual hardware is may not be the best idea, specially in
> the presence of PM.

And actually, the CLOCK_SOURCE_SUSPEND_NONSTOP flag was added to
support real hardware, so its not just limited to VMs. But that
hardware does have to keep the lights on for the counter backing the
clocksource, and I'm not sure how common it ever became.

thanks
-john

Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume

[Grygorii Strashko]

On 11/20/2015 08:52 PM, santosh shilimkar wrote:
> On 11/20/2015 10:46 AM, Marc Zyngier wrote:
>> On 20/11/15 18:35, Grygorii Strashko wrote:
>>> Hi Santosh,
>>>
>>> On 11/20/2015 07:23 PM, santosh shilimkar wrote:
>>>> + Thomas, Marc
>>>>
>>>> On 11/20/2015 5:57 AM, Grygorii Strashko wrote:
>>>>> Now the System stall is observed on TI AM437x based board
>>>>> (am437x-gp-evm) during resuming from System suspend when ARM Global
>>>>> timer is selected as clocksource device - SysRq are working, but
>>>>> nothing else. The reason of stall is that ARM Global timer loses its
>>>>> contexts.
>>>>>
>>>>> The reason of stall is that ARM Global timer loses its contexts during
>>>>> System suspend:
>>>>>      GT_CONTROL.TIMER_ENABLE = 0 (unbanked)
>>>>>      GT_COUNTERx = 0
>>>>>
>>>>> Hence, update ARM Global timer driver to reflect above behaviour
>>>>> - re-enable ARM Global timer on resume GT_CONTROL.TIMER_ENABLE = 1
>>>>> - ensure clocksource and clockevent devices have coresponding flags
>>>>>     (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set
>>>>>     depending on presence of "always-on" DT property.
>>>>>
>>>> Something which loses context in low power states can't be
>>>> called "always-on"
>>>
>>> Sry, it's kinda new area for me and I could make mistakes.
>>>
>>> While working on this patch I've:
>>>   - re-used implementation from ARM arch timer
>>> commit 82a5619410d4c4df65c04272db198eca5a867c18
>>> Author: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>> Date:   Tue Apr 8 10:04:32 2014 +0100
>>>
>>>      clocksource: arch_arm_timer: Fix age-old arch timer C3STOP 
>>> detection issue
>>
>> [...]
>>
>> This patch has a very specific purpose: instructing the core code that
>> this timer will never stop ticking, ever. It is really targeted at
>> virtual machines, whose timer is backed by the host timer, even when the
>> VM is not running.
>>
>> Using it on actual hardware is may not be the best idea, specially in
>> the presence of PM.
>>
> Exactly. Thanks for clarifying the commit Marc.

Thanks for your comments, but could you clarify pls - 
Can i use ARM GT for my use case CPUIDLE=n and SUSPEND=y,
taking into account that System time can be corrected by RTC core
during resume? (it will be used as clocksource only).

And would it be reasonable to resend new version if i will drop 
all "always-on" related code?

-- 
regards,
-grygorii

Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume

[Thomas Gleixner]

On Fri, 20 Nov 2015, John Stultz wrote:
> So its unlikely that the hardware both stays running through suspend,
> but also might halt in idle. That would be "unique".

The amount of creativity put into the next variants of differently
broken timers is amazing. So I wouldn't be too surprised if such a
thing actually surfaces.

Thanks,

	tglx

Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume

[Grygorii Strashko]

On 11/20/2015 09:09 PM, John Stultz wrote:
> On Fri, Nov 20, 2015 at 10:35 AM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
>> Hi Santosh,
>>
>> On 11/20/2015 07:23 PM, santosh shilimkar wrote:
>>> + Thomas, Marc
>>>
>>> On 11/20/2015 5:57 AM, Grygorii Strashko wrote:
>>>> Now the System stall is observed on TI AM437x based board
>>>> (am437x-gp-evm) during resuming from System suspend when ARM Global
>>>> timer is selected as clocksource device - SysRq are working, but
>>>> nothing else. The reason of stall is that ARM Global timer loses its
>>>> contexts.
>>>>
>>>> The reason of stall is that ARM Global timer loses its contexts during
>>>> System suspend:
>>>>      GT_CONTROL.TIMER_ENABLE = 0 (unbanked)
>>>>      GT_COUNTERx = 0
>>>>
>>>> Hence, update ARM Global timer driver to reflect above behaviour
>>>> - re-enable ARM Global timer on resume GT_CONTROL.TIMER_ENABLE = 1
>>>> - ensure clocksource and clockevent devices have coresponding flags
>>>>     (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set
>>>>     depending on presence of "always-on" DT property.
>>>>
>>> Something which loses context in low power states can't be
>>> called "always-on"
>>
>> Sry, it's kinda new area for me and I could make mistakes.
>>
>> While working on this patch I've:
>>   - re-used implementation from ARM arch timer
>> commit 82a5619410d4c4df65c04272db198eca5a867c18
>> Author: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Date:   Tue Apr 8 10:04:32 2014 +0100
>>
>>      clocksource: arch_arm_timer: Fix age-old arch timer C3STOP detection issue
>>
>>
>> - and followed timekeeping.txt:
>> "Typically the clock source is a monotonic, atomic counter which will provide
>>   n bits which count from 0 to 2^(n-1) and then wraps around to 0 and start over.
>> It will ideally NEVER stop ticking as long as the system is running. It
>> may stop during system suspend."
>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>
>> And that exactly my use-case: I'd like to use ARM GT as clocksource
>> and with CPUIdle = n. But System suspend has to be allowed.
>>
>>
>>>
>>> Also if the clock-soucre can't tick in the low power states
>>> then that device shouldn't be used as a clock-source.
>>
>> Agree. clocksource can't (except with suspend). Have I missed something?
> 
> I think the point Stantosh is making is that if the clocksource stops
> in suspend (which is allowed) you should not be setting
> CLOCK_SOURCE_SUSPEND_NONSTOP (which promises the clocksource doesn't
> stop in suspend, so it can be used for suspend timing as well).
> 

Ok. Thanks. I got it now. Adding CLOCK_SOURCE_SUSPEND_NONSTOP is mistake.

> The contradictory part in your patch is that you're also setting the
> clockevent logic as  CLOCK_EVT_FEAT_C3STOP, which flags that the
> clockevent hardware might stop in low-power idle states (ie: not
> suspend, but while the system is running).  Usually hardware that
> halts in low-power mode (like the apic on some x86 machines) is not
> also used for timekeeping (instead we use the hpet/acpi there).

Sry, I've set CLOCK_EVT_FEAT_C3STOP if "always-on" = *false*

^ this, I thought, might be valid because it will stop in low-power idle states
  and "always-on" was expected to indicate case when CPUIdle = n (no CPU PM),
  same way as Lorenzo did.
  Would it be correct to just set it always?

and CLOCK_SOURCE_SUSPEND_NONSTOP if "always-on" = *true*

^ this part is mistake

> 
> So its unlikely that the hardware both stays running through suspend,
> but also might halt in idle. That would be "unique".
> 

Thanks a lot.


-- 
regards,
-grygorii

Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume

[John Stultz]

On Fri, Nov 20, 2015 at 11:28 AM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> On 11/20/2015 09:09 PM, John Stultz wrote:
>> On Fri, Nov 20, 2015 at 10:35 AM, Grygorii Strashko
>> <grygorii.strashko@ti.com> wrote:
>>> Hi Santosh,
>>>
>>> On 11/20/2015 07:23 PM, santosh shilimkar wrote:
>>>> + Thomas, Marc
>>>>
>>>> On 11/20/2015 5:57 AM, Grygorii Strashko wrote:
>>>>> Now the System stall is observed on TI AM437x based board
>>>>> (am437x-gp-evm) during resuming from System suspend when ARM Global
>>>>> timer is selected as clocksource device - SysRq are working, but
>>>>> nothing else. The reason of stall is that ARM Global timer loses its
>>>>> contexts.
>>>>>
>>>>> The reason of stall is that ARM Global timer loses its contexts during
>>>>> System suspend:
>>>>>      GT_CONTROL.TIMER_ENABLE = 0 (unbanked)
>>>>>      GT_COUNTERx = 0
>>>>>
>>>>> Hence, update ARM Global timer driver to reflect above behaviour
>>>>> - re-enable ARM Global timer on resume GT_CONTROL.TIMER_ENABLE = 1
>>>>> - ensure clocksource and clockevent devices have coresponding flags
>>>>>     (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set
>>>>>     depending on presence of "always-on" DT property.
>>>>>
>>>> Something which loses context in low power states can't be
>>>> called "always-on"
>>>
>>> Sry, it's kinda new area for me and I could make mistakes.
>>>
>>> While working on this patch I've:
>>>   - re-used implementation from ARM arch timer
>>> commit 82a5619410d4c4df65c04272db198eca5a867c18
>>> Author: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>> Date:   Tue Apr 8 10:04:32 2014 +0100
>>>
>>>      clocksource: arch_arm_timer: Fix age-old arch timer C3STOP detection issue
>>>
>>>
>>> - and followed timekeeping.txt:
>>> "Typically the clock source is a monotonic, atomic counter which will provide
>>>   n bits which count from 0 to 2^(n-1) and then wraps around to 0 and start over.
>>> It will ideally NEVER stop ticking as long as the system is running. It
>>> may stop during system suspend."
>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>
>>> And that exactly my use-case: I'd like to use ARM GT as clocksource
>>> and with CPUIdle = n. But System suspend has to be allowed.
>>>
>>>
>>>>
>>>> Also if the clock-soucre can't tick in the low power states
>>>> then that device shouldn't be used as a clock-source.
>>>
>>> Agree. clocksource can't (except with suspend). Have I missed something?
>>
>> I think the point Stantosh is making is that if the clocksource stops
>> in suspend (which is allowed) you should not be setting
>> CLOCK_SOURCE_SUSPEND_NONSTOP (which promises the clocksource doesn't
>> stop in suspend, so it can be used for suspend timing as well).
>>
>
> Ok. Thanks. I got it now. Adding CLOCK_SOURCE_SUSPEND_NONSTOP is mistake.
>
>> The contradictory part in your patch is that you're also setting the
>> clockevent logic as  CLOCK_EVT_FEAT_C3STOP, which flags that the
>> clockevent hardware might stop in low-power idle states (ie: not
>> suspend, but while the system is running).  Usually hardware that
>> halts in low-power mode (like the apic on some x86 machines) is not
>> also used for timekeeping (instead we use the hpet/acpi there).
>
> Sry, I've set CLOCK_EVT_FEAT_C3STOP if "always-on" = *false*

You might also consider renaming that value from always_on to
something more descriptive, given the subtlety of the different states
here. Maybe instead use a flag called halts_in_idle or something?

thanks
-john

Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume

[Grygorii Strashko]

On 11/20/2015 09:32 PM, John Stultz wrote:
> On Fri, Nov 20, 2015 at 11:28 AM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
>> On 11/20/2015 09:09 PM, John Stultz wrote:
>>> On Fri, Nov 20, 2015 at 10:35 AM, Grygorii Strashko
>>> <grygorii.strashko@ti.com> wrote:
>>>> Hi Santosh,
>>>>
>>>> On 11/20/2015 07:23 PM, santosh shilimkar wrote:
>>>>> + Thomas, Marc
>>>>>
>>>>> On 11/20/2015 5:57 AM, Grygorii Strashko wrote:
>>>>>> Now the System stall is observed on TI AM437x based board
>>>>>> (am437x-gp-evm) during resuming from System suspend when ARM Global
>>>>>> timer is selected as clocksource device - SysRq are working, but
>>>>>> nothing else. The reason of stall is that ARM Global timer loses its
>>>>>> contexts.
>>>>>>
>>>>>> The reason of stall is that ARM Global timer loses its contexts during
>>>>>> System suspend:
>>>>>>       GT_CONTROL.TIMER_ENABLE = 0 (unbanked)
>>>>>>       GT_COUNTERx = 0
>>>>>>
>>>>>> Hence, update ARM Global timer driver to reflect above behaviour
>>>>>> - re-enable ARM Global timer on resume GT_CONTROL.TIMER_ENABLE = 1
>>>>>> - ensure clocksource and clockevent devices have coresponding flags
>>>>>>      (CLOCK_SOURCE_SUSPEND_NONSTOP and CLOCK_EVT_FEAT_C3STOP) set
>>>>>>      depending on presence of "always-on" DT property.
>>>>>>
>>>>> Something which loses context in low power states can't be
>>>>> called "always-on"
>>>>
>>>> Sry, it's kinda new area for me and I could make mistakes.
>>>>
>>>> While working on this patch I've:
>>>>    - re-used implementation from ARM arch timer
>>>> commit 82a5619410d4c4df65c04272db198eca5a867c18
>>>> Author: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>>> Date:   Tue Apr 8 10:04:32 2014 +0100
>>>>
>>>>       clocksource: arch_arm_timer: Fix age-old arch timer C3STOP detection issue
>>>>
>>>>
>>>> - and followed timekeeping.txt:
>>>> "Typically the clock source is a monotonic, atomic counter which will provide
>>>>    n bits which count from 0 to 2^(n-1) and then wraps around to 0 and start over.
>>>> It will ideally NEVER stop ticking as long as the system is running. It
>>>> may stop during system suspend."
>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>>>
>>>> And that exactly my use-case: I'd like to use ARM GT as clocksource
>>>> and with CPUIdle = n. But System suspend has to be allowed.
>>>>
>>>>
>>>>>
>>>>> Also if the clock-soucre can't tick in the low power states
>>>>> then that device shouldn't be used as a clock-source.
>>>>
>>>> Agree. clocksource can't (except with suspend). Have I missed something?
>>>
>>> I think the point Stantosh is making is that if the clocksource stops
>>> in suspend (which is allowed) you should not be setting
>>> CLOCK_SOURCE_SUSPEND_NONSTOP (which promises the clocksource doesn't
>>> stop in suspend, so it can be used for suspend timing as well).
>>>
>>
>> Ok. Thanks. I got it now. Adding CLOCK_SOURCE_SUSPEND_NONSTOP is mistake.
>>
>>> The contradictory part in your patch is that you're also setting the
>>> clockevent logic as  CLOCK_EVT_FEAT_C3STOP, which flags that the
>>> clockevent hardware might stop in low-power idle states (ie: not
>>> suspend, but while the system is running).  Usually hardware that
>>> halts in low-power mode (like the apic on some x86 machines) is not
>>> also used for timekeeping (instead we use the hpet/acpi there).
>>
>> Sry, I've set CLOCK_EVT_FEAT_C3STOP if "always-on" = *false*
> 
> You might also consider renaming that value from always_on to
> something more descriptive, given the subtlety of the different states
> here. Maybe instead use a flag called halts_in_idle or something?
> 

I'd better just drop it for now hence I'm still not sure what is more reasonable
continue with DT property or just add this flag always.

-- 
regards,
-grygorii