lkml.org 
[lkml]   [2013]   [Jun]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH/RFC] clockevents: Ignore C3STOP when CPUIdle is disabled
From
Hi Daniel,

On Tue, Jun 18, 2013 at 5:30 PM, Daniel Lezcano
<daniel.lezcano@linaro.org> wrote:
> On 06/18/2013 09:17 AM, Magnus Damm wrote:
>> From: Magnus Damm <damm@opensource.se>
>>
>> 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 <damm@opensource.se>
>> ---
>>
>> 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


\
 
 \ /
  Last update: 2013-06-21 01:21    [W:0.183 / U:0.348 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site