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

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

[Magnus Damm]

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)
+{
+	/* 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));
+}
+
 #endif
 
 extern void do_timer(unsigned long ticks);

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

[Daniel Lezcano]

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 ?

> 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)
> +{
> +	/* 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));
> +}
> +
>  #endif
>  
>  extern void do_timer(unsigned long ticks);
> --
> 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/
> 


-- 
 <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

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

[Magnus Damm]

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.

/ magnus

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

[Daniel Lezcano]

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.

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.

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.

-- 
 <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

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

[Magnus Damm]

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? =)

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.

Thanks,

/ magnus

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

[Daniel Lezcano]

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

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

[Daniel Lezcano]

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).

> +{
> +	/* 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.

>  extern void do_timer(unsigned long ticks);
> --
> 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/
> 


-- 
 <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

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

[Magnus Damm]

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

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

[Daniel Lezcano]

On 06/18/2013 10:56 AM, Magnus Damm wrote:
> 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?

Personally, I would prefer tick_device_is_reliable function instead of
c3stop because this one is coming from the C-state Intel semantic. On
the other architectures, the C-state does not make sense and, especially
for ARM, you can have idle state corresponding to the index #1 where the
timer is shutdown. This comment also apply to CLOCK_EVT_FEAT_C3STOP ...
but this is diverging from the purpose of your patch.

>>> +{
>>> +     /* 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. =)


-- 
 <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