[PATCH] drivers: cpuidle: assign enter_freeze to same as enter callback function

[PATCH] drivers: cpuidle: assign enter_freeze to same as enter callback function

[Sudeep Holla]

enter_freeze() callback is expected atleast to do the same as enter()
but it has to guarantee that interrupts aren't enabled at any point
in its execution, as the tick is frozen.

CPUs execute ->enter_freeze with the local tick or entire timekeeping
suspended, so it must not re-enable interrupts at any point (even
temporarily) or attempt to change states of clock event devices.

It will be called when the system goes to suspend-to-idle and will
reduce power usage because CPUs won't be awaken for unnecessary IRQs
(i.e. woken up only on IRQs from "wakeup sources")

Since for all the states that have CPUIDLE_FLAG_TIMER_STOP flag set,
local tick is stopped, we can reuse the same code for both the enter()
and enter_freeze() callbacks. Only "coupled" cpuidle mechanism enables
interrupts and doing that with timekeeping suspended is generally not
safe. Since this generic DT based idle driver doesn't support "coupled"
states, it is safe to assume that the interrupts are not re-enabled.

This patch assign enter_freeze to same as enter callback function which
helps to save power without any intermittent spurious wakeups from
suspend-to-idle.

Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/cpuidle/dt_idle_states.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c
index a5c111b67f37..5a087d108475 100644
--- a/drivers/cpuidle/dt_idle_states.c
+++ b/drivers/cpuidle/dt_idle_states.c
@@ -79,8 +79,17 @@ static int init_state_node(struct cpuidle_state *idle_state,
 		desc = state_node->name;

 	idle_state->flags = 0;
-	if (of_property_read_bool(state_node, "local-timer-stop"))
+	if (of_property_read_bool(state_node, "local-timer-stop")) {
 		idle_state->flags |= CPUIDLE_FLAG_TIMER_STOP;
+		/*
+		 * CPUIDLE_FLAG_TIMER_STOP guarantees that the local tick is
+		 * stopped and since this is not a "coupled" state interrupts
+		 * won't be enabled when it exits allowing the tick to be
+		 * frozen safely. So enter() can be also enter_freeze()
+		 * callback.
+		 */
+		idle_state->enter_freeze = match_id->data;
+	}
 	/*
 	 * TODO:
 	 *	replace with kstrdup and pointer assignment when name
--
2.7.4

Re: [PATCH] drivers: cpuidle: assign enter_freeze to same as enter callback function

[Lorenzo Pieralisi]

On Wed, Nov 09, 2016 at 05:43:30PM +0000, Sudeep Holla wrote:
> enter_freeze() callback is expected atleast to do the same as enter()
> but it has to guarantee that interrupts aren't enabled at any point
> in its execution, as the tick is frozen.
> 
> CPUs execute ->enter_freeze with the local tick or entire timekeeping
> suspended, so it must not re-enable interrupts at any point (even
> temporarily) or attempt to change states of clock event devices.
> 
> It will be called when the system goes to suspend-to-idle and will
> reduce power usage because CPUs won't be awaken for unnecessary IRQs
> (i.e. woken up only on IRQs from "wakeup sources")
> 
> Since for all the states that have CPUIDLE_FLAG_TIMER_STOP flag set,
> local tick is stopped, we can reuse the same code for both the enter()
> and enter_freeze() callbacks. Only "coupled" cpuidle mechanism enables
> interrupts and doing that with timekeeping suspended is generally not
> safe. Since this generic DT based idle driver doesn't support "coupled"
> states, it is safe to assume that the interrupts are not re-enabled.
> 
> This patch assign enter_freeze to same as enter callback function which
> helps to save power without any intermittent spurious wakeups from
> suspend-to-idle.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/cpuidle/dt_idle_states.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c
> index a5c111b67f37..5a087d108475 100644
> --- a/drivers/cpuidle/dt_idle_states.c
> +++ b/drivers/cpuidle/dt_idle_states.c
> @@ -79,8 +79,17 @@ static int init_state_node(struct cpuidle_state *idle_state,
>  		desc = state_node->name;
> 
>  	idle_state->flags = 0;
> -	if (of_property_read_bool(state_node, "local-timer-stop"))
> +	if (of_property_read_bool(state_node, "local-timer-stop")) {
>  		idle_state->flags |= CPUIDLE_FLAG_TIMER_STOP;
> +		/*
> +		 * CPUIDLE_FLAG_TIMER_STOP guarantees that the local tick is
> +		 * stopped and since this is not a "coupled" state interrupts
> +		 * won't be enabled when it exits allowing the tick to be
> +		 * frozen safely. So enter() can be also enter_freeze()
> +		 * callback.
> +		 */

I do not think that represents a guarantee for enter_freeze() to be
functional, we can initialize enter_freeze() with a function that
does _not_ enable IRQs while executing, it has not much to do with
the local timer losing HW state.

I would just init the enter_freeze() pointer and be done with that,
adding code to check whether the idle back-end enables IRQs when it
enters idle is a major PITA that really is not worth the hassle and
apart from coupled C-states (which we do not support in DT as you said)
I can't find another example (and on top of that it is not even
something we can solve through DT since it is not a property of the idle
state but more related to its kernel implementation).

If we wanted to do it _properly_ we have to add an arch hook to check
if the given idle state enter function back-end, ie cpu_ops on ARM64 or
or cpuidle_ops on ARM, enables IRQs while executing, I would honestly
avoid it but comments are nonetheless welcome.

Thanks for putting it together,
Lorenzo

> +		idle_state->enter_freeze = match_id->data;
> +	}
>  	/*
>  	 * TODO:
>  	 *	replace with kstrdup and pointer assignment when name
> --
> 2.7.4
> 

Re: [PATCH] drivers: cpuidle: assign enter_freeze to same as enter callback function

[Sudeep Holla]

On 09/11/16 18:39, Lorenzo Pieralisi wrote:
> On Wed, Nov 09, 2016 at 05:43:30PM +0000, Sudeep Holla wrote:
>> enter_freeze() callback is expected atleast to do the same as enter()
>> but it has to guarantee that interrupts aren't enabled at any point
>> in its execution, as the tick is frozen.
>>
>> CPUs execute ->enter_freeze with the local tick or entire timekeeping
>> suspended, so it must not re-enable interrupts at any point (even
>> temporarily) or attempt to change states of clock event devices.
>>
>> It will be called when the system goes to suspend-to-idle and will
>> reduce power usage because CPUs won't be awaken for unnecessary IRQs
>> (i.e. woken up only on IRQs from "wakeup sources")
>>
>> Since for all the states that have CPUIDLE_FLAG_TIMER_STOP flag set,
>> local tick is stopped, we can reuse the same code for both the enter()
>> and enter_freeze() callbacks. Only "coupled" cpuidle mechanism enables
>> interrupts and doing that with timekeeping suspended is generally not
>> safe. Since this generic DT based idle driver doesn't support "coupled"
>> states, it is safe to assume that the interrupts are not re-enabled.
>>
>> This patch assign enter_freeze to same as enter callback function which
>> helps to save power without any intermittent spurious wakeups from
>> suspend-to-idle.
>>
>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>> ---
>>  drivers/cpuidle/dt_idle_states.c | 11 ++++++++++-
>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c
>> index a5c111b67f37..5a087d108475 100644
>> --- a/drivers/cpuidle/dt_idle_states.c
>> +++ b/drivers/cpuidle/dt_idle_states.c
>> @@ -79,8 +79,17 @@ static int init_state_node(struct cpuidle_state *idle_state,
>>  		desc = state_node->name;
>>
>>  	idle_state->flags = 0;
>> -	if (of_property_read_bool(state_node, "local-timer-stop"))
>> +	if (of_property_read_bool(state_node, "local-timer-stop")) {
>>  		idle_state->flags |= CPUIDLE_FLAG_TIMER_STOP;
>> +		/*
>> +		 * CPUIDLE_FLAG_TIMER_STOP guarantees that the local tick is
>> +		 * stopped and since this is not a "coupled" state interrupts
>> +		 * won't be enabled when it exits allowing the tick to be
>> +		 * frozen safely. So enter() can be also enter_freeze()
>> +		 * callback.
>> +		 */
>
> I do not think that represents a guarantee for enter_freeze() to be
> functional, we can initialize enter_freeze() with a function that
> does _not_ enable IRQs while executing, it has not much to do with
> the local timer losing HW state.
>

I agree, and I didn't mean that with the above comment. But reading
again, I see your point.

> I would just init the enter_freeze() pointer and be done with that,
> adding code to check whether the idle back-end enables IRQs when it
> enters idle is a major PITA that really is not worth the hassle and
> apart from coupled C-states (which we do not support in DT as you said)
> I can't find another example (and on top of that it is not even
> something we can solve through DT since it is not a property of the idle
> state but more related to its kernel implementation).
>

Makes sense, I was just trying to avoid setting for a state like
CPU/Cluster retention but I agree, we need not do that.

> If we wanted to do it _properly_ we have to add an arch hook to check
> if the given idle state enter function back-end, ie cpu_ops on ARM64 or
> or cpuidle_ops on ARM, enables IRQs while executing, I would honestly
> avoid it but comments are nonetheless welcome.
>

Yes, that's may be unnecessary addition of some code when we can do it
in simple ways, but I am open to suggestions.

-- 
Regards,
Sudeep

Re: [PATCH] drivers: cpuidle: assign enter_freeze to same as enter callback function

[Vincent Guittot]

On 9 November 2016 at 19:48, Sudeep Holla <sudeep.holla@arm.com> wrote:
>
>
> On 09/11/16 18:39, Lorenzo Pieralisi wrote:
>>
>> On Wed, Nov 09, 2016 at 05:43:30PM +0000, Sudeep Holla wrote:
>>>
>>> enter_freeze() callback is expected atleast to do the same as enter()
>>> but it has to guarantee that interrupts aren't enabled at any point
>>> in its execution, as the tick is frozen.
>>>
>>> CPUs execute ->enter_freeze with the local tick or entire timekeeping
>>> suspended, so it must not re-enable interrupts at any point (even
>>> temporarily) or attempt to change states of clock event devices.
>>>
>>> It will be called when the system goes to suspend-to-idle and will
>>> reduce power usage because CPUs won't be awaken for unnecessary IRQs
>>> (i.e. woken up only on IRQs from "wakeup sources")
>>>
>>> Since for all the states that have CPUIDLE_FLAG_TIMER_STOP flag set,
>>> local tick is stopped, we can reuse the same code for both the enter()
>>> and enter_freeze() callbacks. Only "coupled" cpuidle mechanism enables
>>> interrupts and doing that with timekeeping suspended is generally not
>>> safe. Since this generic DT based idle driver doesn't support "coupled"
>>> states, it is safe to assume that the interrupts are not re-enabled.
>>>
>>> This patch assign enter_freeze to same as enter callback function which
>>> helps to save power without any intermittent spurious wakeups from
>>> suspend-to-idle.
>>>
>>> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
>>> ---
>>>  drivers/cpuidle/dt_idle_states.c | 11 ++++++++++-
>>>  1 file changed, 10 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/cpuidle/dt_idle_states.c
>>> b/drivers/cpuidle/dt_idle_states.c
>>> index a5c111b67f37..5a087d108475 100644
>>> --- a/drivers/cpuidle/dt_idle_states.c
>>> +++ b/drivers/cpuidle/dt_idle_states.c
>>> @@ -79,8 +79,17 @@ static int init_state_node(struct cpuidle_state
>>> *idle_state,
>>>                 desc = state_node->name;
>>>
>>>         idle_state->flags = 0;
>>> -       if (of_property_read_bool(state_node, "local-timer-stop"))
>>> +       if (of_property_read_bool(state_node, "local-timer-stop")) {
>>>                 idle_state->flags |= CPUIDLE_FLAG_TIMER_STOP;
>>> +               /*
>>> +                * CPUIDLE_FLAG_TIMER_STOP guarantees that the local tick
>>> is
>>> +                * stopped and since this is not a "coupled" state
>>> interrupts
>>> +                * won't be enabled when it exits allowing the tick to be
>>> +                * frozen safely. So enter() can be also enter_freeze()
>>> +                * callback.
>>> +                */
>>
>>
>> I do not think that represents a guarantee for enter_freeze() to be
>> functional, we can initialize enter_freeze() with a function that
>> does _not_ enable IRQs while executing, it has not much to do with
>> the local timer losing HW state.
>>
>
> I agree, and I didn't mean that with the above comment. But reading
> again, I see your point.
>
>> I would just init the enter_freeze() pointer and be done with that,
>> adding code to check whether the idle back-end enables IRQs when it
>> enters idle is a major PITA that really is not worth the hassle and
>> apart from coupled C-states (which we do not support in DT as you said)
>> I can't find another example (and on top of that it is not even
>> something we can solve through DT since it is not a property of the idle
>> state but more related to its kernel implementation).
>>
>
> Makes sense, I was just trying to avoid setting for a state like
> CPU/Cluster retention but I agree, we need not do that.

I agree with Lorenzo and would prefer to keep it simple

Regards,
Vincent

>
>> If we wanted to do it _properly_ we have to add an arch hook to check
>> if the given idle state enter function back-end, ie cpu_ops on ARM64 or
>> or cpuidle_ops on ARM, enables IRQs while executing, I would honestly
>> avoid it but comments are nonetheless welcome.
>>
>
> Yes, that's may be unnecessary addition of some code when we can do it
> in simple ways, but I am open to suggestions.
>
> --
> Regards,
> Sudeep

Re: [PATCH] drivers: cpuidle: assign enter_freeze to same as enter callback function

[Sudeep Holla]

On 10/11/16 10:28, Vincent Guittot wrote:
> On 9 November 2016 at 19:48, Sudeep Holla <sudeep.holla@arm.com> wrote:
>>
>>
>> On 09/11/16 18:39, Lorenzo Pieralisi wrote:

[..]

>>> I would just init the enter_freeze() pointer and be done with that,
>>> adding code to check whether the idle back-end enables IRQs when it
>>> enters idle is a major PITA that really is not worth the hassle and
>>> apart from coupled C-states (which we do not support in DT as you said)
>>> I can't find another example (and on top of that it is not even
>>> something we can solve through DT since it is not a property of the idle
>>> state but more related to its kernel implementation).
>>>
>>
>> Makes sense, I was just trying to avoid setting for a state like
>> CPU/Cluster retention but I agree, we need not do that.
>
> I agree with Lorenzo and would prefer to keep it simple
>

Sure I will respin accordingly.

-- 
Regards,
Sudeep

[PATCH v2] drivers: cpuidle: assign enter_freeze to same as enter callback function

[Sudeep Holla]

enter_freeze() callback is expected atleast to do the same as enter()
but it has to guarantee that interrupts aren't enabled at any point
in its execution, as the tick is frozen.

CPUs execute ->enter_freeze with the local tick or entire timekeeping
suspended, so it must not re-enable interrupts at any point (even
temporarily) or attempt to change states of clock event devices.

It will be called when the system goes to suspend-to-idle and will
reduce power usage because CPUs won't be awaken for unnecessary IRQs
(i.e. woken up only on IRQs from "wakeup sources")

We can reuse the same code for both the enter() and enter_freeze()
callbacks as along as they don't re-enable interrupts. Only "coupled"
cpuidle mechanism enables interrupts and doing that with timekeeping
suspended is generally not safe.

Since this generic DT based idle driver doesn't support "coupled"
states, it is safe to assume that the interrupts are not re-enabled.

This patch assign enter_freeze to same as enter callback function which
helps to save power without any intermittent spurious wakeups from
suspend-to-idle.

Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
---
 drivers/cpuidle/dt_idle_states.c | 6 ++++++
 1 file changed, 6 insertions(+)

v1->v2:
	- Dropped checking and using only states with CPUIDLE_FLAG_TIMER_STOP
	  enabled.

diff --git a/drivers/cpuidle/dt_idle_states.c b/drivers/cpuidle/dt_idle_states.c
index a5c111b67f37..ffca4fc0061d 100644
--- a/drivers/cpuidle/dt_idle_states.c
+++ b/drivers/cpuidle/dt_idle_states.c
@@ -38,6 +38,12 @@ static int init_state_node(struct cpuidle_state *idle_state,
 	 * state enter function.
 	 */
 	idle_state->enter = match_id->data;
+	/*
+	 * Since this is not a "coupled" state, it's safe to assume interrupts
+	 * won't be enabled when it exits allowing the tick to be frozen
+	 * safely. So enter() can be also enter_freeze() callback.
+	 */
+	idle_state->enter_freeze = match_id->data;

 	err = of_property_read_u32(state_node, "wakeup-latency-us",
 				   &idle_state->exit_latency);
--
2.7.4

Re: [PATCH v2] drivers: cpuidle: assign enter_freeze to same as enter callback function

[Andy Gross]

On 10 November 2016 at 08:24, Sudeep Holla <sudeep.holla@arm.com> wrote:
> enter_freeze() callback is expected atleast to do the same as enter()
> but it has to guarantee that interrupts aren't enabled at any point
> in its execution, as the tick is frozen.
>
> CPUs execute ->enter_freeze with the local tick or entire timekeeping
> suspended, so it must not re-enable interrupts at any point (even
> temporarily) or attempt to change states of clock event devices.
>
> It will be called when the system goes to suspend-to-idle and will
> reduce power usage because CPUs won't be awaken for unnecessary IRQs
> (i.e. woken up only on IRQs from "wakeup sources")
>
> We can reuse the same code for both the enter() and enter_freeze()
> callbacks as along as they don't re-enable interrupts. Only "coupled"
> cpuidle mechanism enables interrupts and doing that with timekeeping
> suspended is generally not safe.
>
> Since this generic DT based idle driver doesn't support "coupled"
> states, it is safe to assume that the interrupts are not re-enabled.
>
> This patch assign enter_freeze to same as enter callback function which
> helps to save power without any intermittent spurious wakeups from
> suspend-to-idle.
>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>

Tested this on a Qualcomm 410c dragonboard.  Worked great.  Thanks for
the patch!


Tested-by: Andy Gross <andy.gross@linaro.org>

Re: [PATCH v2] drivers: cpuidle: assign enter_freeze to same as enter callback function

[Sudeep Holla]

Hi Rafael,

On 10/11/16 14:24, Sudeep Holla wrote:
> enter_freeze() callback is expected atleast to do the same as enter()
> but it has to guarantee that interrupts aren't enabled at any point
> in its execution, as the tick is frozen.
>
> CPUs execute ->enter_freeze with the local tick or entire timekeeping
> suspended, so it must not re-enable interrupts at any point (even
> temporarily) or attempt to change states of clock event devices.
>
> It will be called when the system goes to suspend-to-idle and will
> reduce power usage because CPUs won't be awaken for unnecessary IRQs
> (i.e. woken up only on IRQs from "wakeup sources")
>
> We can reuse the same code for both the enter() and enter_freeze()
> callbacks as along as they don't re-enable interrupts. Only "coupled"
> cpuidle mechanism enables interrupts and doing that with timekeeping
> suspended is generally not safe.
>
> Since this generic DT based idle driver doesn't support "coupled"
> states, it is safe to assume that the interrupts are not re-enabled.
>
> This patch assign enter_freeze to same as enter callback function which
> helps to save power without any intermittent spurious wakeups from
> suspend-to-idle.
>
> Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> ---
>  drivers/cpuidle/dt_idle_states.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> v1->v2:
> 	- Dropped checking and using only states with CPUIDLE_FLAG_TIMER_STOP
> 	  enabled.
>

Can you pick up this patch directly if there are no concerns ?

-- 
Regards,
Sudeep

Re: [PATCH v2] drivers: cpuidle: assign enter_freeze to same as enter callback function

[Rafael J. Wysocki]

On Tuesday, November 15, 2016 02:20:21 PM Sudeep Holla wrote:
> Hi Rafael,
> 
> On 10/11/16 14:24, Sudeep Holla wrote:
> > enter_freeze() callback is expected atleast to do the same as enter()
> > but it has to guarantee that interrupts aren't enabled at any point
> > in its execution, as the tick is frozen.
> >
> > CPUs execute ->enter_freeze with the local tick or entire timekeeping
> > suspended, so it must not re-enable interrupts at any point (even
> > temporarily) or attempt to change states of clock event devices.
> >
> > It will be called when the system goes to suspend-to-idle and will
> > reduce power usage because CPUs won't be awaken for unnecessary IRQs
> > (i.e. woken up only on IRQs from "wakeup sources")
> >
> > We can reuse the same code for both the enter() and enter_freeze()
> > callbacks as along as they don't re-enable interrupts. Only "coupled"
> > cpuidle mechanism enables interrupts and doing that with timekeeping
> > suspended is generally not safe.
> >
> > Since this generic DT based idle driver doesn't support "coupled"
> > states, it is safe to assume that the interrupts are not re-enabled.
> >
> > This patch assign enter_freeze to same as enter callback function which
> > helps to save power without any intermittent spurious wakeups from
> > suspend-to-idle.
> >
> > Cc: "Rafael J. Wysocki" <rjw@rjwysocki.net>
> > Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
> > ---
> >  drivers/cpuidle/dt_idle_states.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > v1->v2:
> > 	- Dropped checking and using only states with CPUIDLE_FLAG_TIMER_STOP
> > 	  enabled.
> >
> 
> Can you pick up this patch directly if there are no concerns ?

There were none, so it's been queued up for 4.10.

Thanks,
Rafael