Re: [PATCH/RFC] clockevents: Ignore C3STOP when CPUIdle is disabled

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 06/18/2013 10:49 AM, Magnus Damm wrote:
> Hi Daniel,
> 
> On Tue, Jun 18, 2013 at 5:24 PM, Daniel Lezcano
> <daniel.lezcano@linaro.org> wrote:
>> On 06/18/2013 09:39 AM, Magnus Damm wrote:
>>> On Tue, Jun 18, 2013 at 4:32 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.
>>>>
>>>> Do we need a broadcast timer when cpuidle is not compiled in the kernel ?
>>>
>>> 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.
> 
> No problem. Thanks for your comments!
> 
>> 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.
> 
> 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.
> 
>> 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.
> 
> That may very well be the case. Care to hack up a patch? =)

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


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog