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

Message ID 1478787873-18180-1-git-send-email-sudeep.holla@arm.com
State New
Headers show

Commit Message

Sudeep Holla Nov. 10, 2016, 2:24 p.m.
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.

--
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Andy Gross Nov. 10, 2016, 6:18 p.m. | #1
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>

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sudeep Holla Nov. 15, 2016, 2:20 p.m. | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki Nov. 24, 2016, 1:15 a.m. | #3
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

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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