Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755233Ab3FRI4k (ORCPT ); Tue, 18 Jun 2013 04:56:40 -0400 Received: from mail-la0-f53.google.com ([209.85.215.53]:56385 "EHLO mail-la0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753314Ab3FRI4e (ORCPT ); Tue, 18 Jun 2013 04:56:34 -0400 MIME-Version: 1.0 In-Reply-To: <51C01A97.5000903@linaro.org> References: <20130618071756.22598.51046.sendpatchset@w520> <51C01A97.5000903@linaro.org> Date: Tue, 18 Jun 2013 17:56:32 +0900 Message-ID: Subject: Re: [PATCH/RFC] clockevents: Ignore C3STOP when CPUIdle is disabled From: Magnus Damm To: Daniel Lezcano Cc: linux-kernel , Mark Rutland , SH-Linux , Marc Zyngier , Catalin Marinas , "Simon Horman [Horms]" , John Stultz , Shinya Kuribayashi , Thomas Gleixner Content-Type: text/plain; charset=ISO-8859-1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6039 Lines: 157 Hi Daniel, On Tue, Jun 18, 2013 at 5:30 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. >> >> Not-Yet-Signed-off-by: Magnus Damm >> --- >> >> An earlier ARM arch timer specific version of this patch was >> posted yesterday as: >> "[PATCH/RFC] arm: arch_timer: Do not set C3STOP in case CPU_IDLE=n" >> >> Many thanks to Mark Rutland for his kind feedback. >> >> kernel/time/tick-broadcast.c | 8 ++++---- >> kernel/time/tick-common.c | 2 +- >> kernel/time/tick-internal.h | 11 +++++++++++ >> 3 files changed, 16 insertions(+), 5 deletions(-) >> >> --- 0001/kernel/time/tick-broadcast.c >> +++ work/kernel/time/tick-broadcast.c 2013-06-18 15:36:21.000000000 +0900 >> @@ -71,7 +71,7 @@ int tick_check_broadcast_device(struct c >> if ((dev->features & CLOCK_EVT_FEAT_DUMMY) || >> (tick_broadcast_device.evtdev && >> tick_broadcast_device.evtdev->rating >= dev->rating) || >> - (dev->features & CLOCK_EVT_FEAT_C3STOP)) >> + tick_device_may_c3stop(dev)) >> return 0; >> >> clockevents_exchange_device(tick_broadcast_device.evtdev, dev); >> @@ -146,7 +146,7 @@ int tick_device_uses_broadcast(struct cl >> * feature and the cpu is marked in the broadcast mask >> * then clear the broadcast bit. >> */ >> - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) { >> + if (!tick_device_may_c3stop(dev)) { >> int cpu = smp_processor_id(); >> cpumask_clear_cpu(cpu, tick_broadcast_mask); >> tick_broadcast_clear_oneshot(cpu); >> @@ -270,7 +270,7 @@ static void tick_do_broadcast_on_off(uns >> /* >> * Is the device not affected by the powerstate ? >> */ >> - if (!dev || !(dev->features & CLOCK_EVT_FEAT_C3STOP)) >> + if (!dev || !tick_device_may_c3stop(dev)) >> goto out; >> >> if (!tick_device_is_functional(dev)) >> @@ -568,7 +568,7 @@ void tick_broadcast_oneshot_control(unsi >> td = &per_cpu(tick_cpu_device, cpu); >> dev = td->evtdev; >> >> - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) >> + if (!tick_device_may_c3stop(dev)) >> return; >> >> bc = tick_broadcast_device.evtdev; >> --- 0001/kernel/time/tick-common.c >> +++ work/kernel/time/tick-common.c 2013-06-18 15:36:29.000000000 +0900 >> @@ -52,7 +52,7 @@ int tick_is_oneshot_available(void) >> >> if (!dev || !(dev->features & CLOCK_EVT_FEAT_ONESHOT)) >> return 0; >> - if (!(dev->features & CLOCK_EVT_FEAT_C3STOP)) >> + if (!tick_device_may_c3stop(dev)) >> return 1; >> return tick_broadcast_oneshot_available(); >> } >> --- 0001/kernel/time/tick-internal.h >> +++ work/kernel/time/tick-internal.h 2013-06-18 15:40:10.000000000 +0900 >> @@ -141,6 +141,17 @@ static inline int tick_device_is_functio >> return !(dev->features & CLOCK_EVT_FEAT_DUMMY); >> } >> >> +/* >> + * Check, if the device has C3STOP behavior and CPU Idle is enabled >> + */ >> +static inline bool tick_device_may_c3stop(struct clock_event_device *dev) > > I prefer tick_device_is_reliable(struct clock_event_device *dev). Sure. I took the name from the flag, thought that made it easy to follow. I wonder what the timekeeping maintainers prefer? >> +{ >> + /* The C3 sleep mode can only trigger when CPU Idle is enabled, >> + * so if CPU Idle is disabled then the C3STOP flag can be ignored */ >> + return (IS_ENABLED(CONFIG_CPU_IDLE) && >> + (dev->features & CLOCK_EVT_FEAT_C3STOP)); >> +} > > Preferably you may use the format: > > #ifdef CONFIG_CPU_IDLE > static inline bool tick_device_is_reliable(struct clock_event_device *dev) > { > return dev->features & CLOCK_EVT_FEAT_C3STOP; > } > #else > static inline bool tick_device_is_reliable(struct clock_event_device *dev) > { > return true; > } > #endif > > to conform the header style format already present in the file. I agree with you about following the same style. Actually, I wrote the code to follow the code right above the function, but I decided to return bool instead of int. I don't mind so much in general though, except trying to keep the code at least half well-commented and relatively compact. So regarding stylistic things, sure, we can move around things. Question is just if this is acceptable or not. =) Thanks, / magnus -- 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/