Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1760633AbbKTTcJ (ORCPT ); Fri, 20 Nov 2015 14:32:09 -0500 Received: from mail-ig0-f181.google.com ([209.85.213.181]:36318 "EHLO mail-ig0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751836AbbKTTcH (ORCPT ); Fri, 20 Nov 2015 14:32:07 -0500 MIME-Version: 1.0 In-Reply-To: <564F7447.8000504@ti.com> References: <1448027861-21472-1-git-send-email-grygorii.strashko@ti.com> <564F570C.7030409@oracle.com> <564F67ED.9000905@ti.com> <564F7447.8000504@ti.com> Date: Fri, 20 Nov 2015 11:32:05 -0800 Message-ID: Subject: Re: [PATCH v2] clocksource: arm_global_timer: fix suspend resume From: John Stultz To: Grygorii Strashko Cc: santosh shilimkar , Russell King - ARM Linux , Daniel Lezcano , Thomas Gleixner , Srinivas Kandagatla , Maxime Coquelin , lkml , "linux-arm-kernel@lists.infradead.org" , kernel@stlinux.com, "linux-omap@vger.kernel.org" , Arnd Bergmann , Felipe Balbi , Tony Lindgren , Santosh Shilimkar , Marc Zyngier Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3719 Lines: 87 On Fri, Nov 20, 2015 at 11:28 AM, Grygorii Strashko wrote: > 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* 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/