From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Lezcano Date: Wed, 19 Jun 2013 13:55:48 +0000 Subject: Re: [PATCH/RFC] clockevents: Ignore C3STOP when CPUIdle is disabled Message-Id: <51C1B864.1010201@linaro.org> List-Id: References: <20130618071756.22598.51046.sendpatchset@w520> <51C00D08.3080406@linaro.org> <51C0193A.40607@linaro.org> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: quoted-printable To: Magnus Damm Cc: linux-kernel , Mark Rutland , SH-Linux , Marc Zyngier , Catalin Marinas , "Simon Horman [Horms]" , John Stultz , Shinya Kuribayashi , Thomas Gleixner On 06/18/2013 10:49 AM, Magnus Damm wrote: > Hi Daniel, >=20 > On Tue, Jun 18, 2013 at 5:24 PM, Daniel Lezcano > wrote: >> On 06/18/2013 09:39 AM, Magnus Damm wrote: >>> On Tue, Jun 18, 2013 at 4:32 PM, Daniel Lezcano >>> wrote: >>>> On 06/18/2013 09:17 AM, Magnus Damm wrote: >>>>> From: Magnus Damm >>>>> >>>>> Introduce the function tick_device_may_c3stop() that >>>>> ignores the C3STOP flag in case CPUIdle is disabled. >>>>> >>>>> The C3STOP flag tells the system that a clock event >>>>> device may be stopped during deep sleep, but if this >>>>> will happen or not depends on things like if CPUIdle >>>>> is enabled and if a CPUIdle driver is available. >>>>> >>>>> This patch assumes that if CPUIdle is disabled then >>>>> the sleep mode triggering C3STOP will never be entered. >>>>> So by ignoring C3STOP when CPUIdle is disabled then it >>>>> becomes possible to use high resolution timers with only >>>>> per-cpu local timers - regardless if they have the >>>>> C3STOP flag set or not. >>>>> >>>>> Observed on the r8a73a4 SoC that at this point only uses >>>>> ARM architected timers for clock event and clock sources. >>>>> >>>>> Without this patch high resolution timers are run time >>>>> disabled on the r8a73a4 SoC - this regardless of CPUIdle >>>>> is disabled or not. >>>>> >>>>> The less short term fix is to add support for more timers >>>>> on the r8a73a4 SoC, but until CPUIdle support is enabled >>>>> it must be possible to use high resoultion timers without >>>>> additional timers. >>>>> >>>>> I'd like to hear some feedback and also test this on more >>>>> systems before merging the code, see the non-SOB below. >>>> >>>> Do we need a broadcast timer when cpuidle is not compiled in the kerne= l ? >>> >>> Yes, if there is no per-cpu timer available. It depends on what the >>> SMP support code for a particular SoC or architecture happen to >>> enable. >> >> Ok thanks for the information. >=20 > No problem. Thanks for your comments! >=20 >> There is here a multiple occurrence of the information "the timer will >> stop when power is saved": CLOCK_EVT_FEAT_C3STOP and >> CPUIDLE_FLAG_TIMER_STOP, so I am wondering if some code simplification >> couldn't be done before your patch. >=20 > I'm sure it's possible to rearrange things in many ways, and the area > that you point out indeed seems to have some overlap. Somehow > describing which timers that stop during what CPUIdle sleep state > would be nice to have. Also, today clock event drivers simply state > C3STOP but there may be shallow sleep modes where the timer doesn't > have to stop. It all seems a bit coarse grained to me as-is. >=20 >> The function: >> >> tick_broadcast_oneshot_control is called from clockevents_notify. This >> one is called from the cpuidle framework or the back-end cpuidle driver. >> The caller knows the timer will be stop and this is why it is switching >> to the broadcast mode. But we have a sanity check in >> tick_broadcast_oneshot_control function: >> >> if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) >> return; >> >> In other words, CPUIDLE_FLAG_TIMER_STOP will tell the framework to call >> clockevents_notify and the tick broadcast code will re-check the device >> will effectively go down. IMHO, we can get rid of this check. >> >> The same happens for the tick_do_broadcast_on_off function. >> >> That reduces the number of C3STOP usage. >=20 > That may very well be the case. Care to hack up a patch? =3D) No problem, I will write one as soon as I can. > The goal with this patch is simply to make it possible to use high > resolution timers if CPUIdle is disabled. Right now the ARM > architected timer is sort of optimized for power, so it sets the > C3STOP flag to say that on some SoCs during some sleep modes these > timers may stop. My point is that this flag doesn't matter as long as > CPUIdle is disabled. Yes, I understand. That makes sense. Thanks -- Daniel --=20 Linaro.org =E2=94=82 Open source software for ARM= SoCs Follow Linaro: Facebook | Twitter | Blog