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

From: John Stultz <john.stultz@linaro.org>
To: Grygorii Strashko <grygorii.strashko@ti.com>
Cc: santosh shilimkar <santosh.shilimkar@oracle.com>,
	Russell King - ARM Linux <linux@arm.linux.org.uk>,
	Daniel Lezcano <daniel.lezcano@linaro.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Srinivas Kandagatla <srinivas.kandagatla@gmail.com>,
	Maxime Coquelin <maxime.coquelin@st.com>,
	lkml <linux-kernel@vger.kernel.org>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>,
	kernel@stlinux.com,
	"linux-omap@vger.kernel.org" <linux-omap@vger.kernel.org>,
	Arnd Bergmann <arnd@arndb.de>, Felipe Balbi <balbi@ti.com>,
	Tony Lindgren <tony@atomide.com>,
	Santosh Shilimkar <ssantosh@kernel.org>,
	Marc Zyngier <marc.zyngier@arm.com>
Subject: Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume
Date: Fri, 20 Nov 2015 11:09:21 -0800	[thread overview]
Message-ID: <CALAqxLVLQLaAq7fPwJnyOWP_dXV39Dh+QreecxSVY35wi=N0Og@mail.gmail.com> (raw)
In-Reply-To: <564F67ED.9000905@ti.com>

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