From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760721AbbKTT3M (ORCPT ); Fri, 20 Nov 2015 14:29:12 -0500 Received: from arroyo.ext.ti.com ([192.94.94.40]:59832 "EHLO arroyo.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751699AbbKTT3K (ORCPT ); Fri, 20 Nov 2015 14:29:10 -0500 Subject: Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume To: John Stultz References: <1448027861-21472-1-git-send-email-grygorii.strashko@ti.com> <564F570C.7030409@oracle.com> <564F67ED.9000905@ti.com> CC: santosh shilimkar , Russell King - ARM Linux , Daniel Lezcano , Thomas Gleixner , Srinivas Kandagatla , Maxime Coquelin , lkml , "linux-arm-kernel@lists.infradead.org" , , "linux-omap@vger.kernel.org" , Arnd Bergmann , Felipe Balbi , Tony Lindgren , Santosh Shilimkar , Marc Zyngier From: Grygorii Strashko Message-ID: <564F7447.8000504@ti.com> Date: Fri, 20 Nov 2015 21:28:07 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/20/2015 09:09 PM, John Stultz wrote: > On Fri, Nov 20, 2015 at 10:35 AM, 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 >> 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