[PATCH v3] ARM: tegra: cpuidle: implement cpuidle_state.enter_freeze()

[PATCH v3] ARM: tegra: cpuidle: implement cpuidle_state.enter_freeze()

[Tomeu Vizoso]

This callback is expected 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.

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.

By setting the CPUIDLE_FLAG_TIMER_STOP flag, we can reuse the same code
for both the enter() and enter_freeze() callbacks.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

---

v3:	* Set CPUIDLE_FLAG_TIMER_STOP to simplify things as suggested by
	Lorenzo Pieralisi
v2:	* Disable FIQs also when suspending-to-idle
---
 arch/arm/mach-tegra/cpuidle-tegra114.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-tegra/cpuidle-tegra114.c b/arch/arm/mach-tegra/cpuidle-tegra114.c
index 155807f..7e59ac0 100644
--- a/arch/arm/mach-tegra/cpuidle-tegra114.c
+++ b/arch/arm/mach-tegra/cpuidle-tegra114.c
@@ -44,16 +44,12 @@ static int tegra114_idle_power_down(struct cpuidle_device *dev,
 	tegra_set_cpu_in_lp2();
 	cpu_pm_enter();
 
-	tick_broadcast_enter();
-
 	call_firmware_op(prepare_idle);
 
 	/* Do suspend by ourselves if the firmware does not implement it */
 	if (call_firmware_op(do_idle, 0) == -ENOSYS)
 		cpu_suspend(0, tegra30_sleep_cpu_secondary_finish);
 
-	tick_broadcast_exit();
-
 	cpu_pm_exit();
 	tegra_clear_cpu_in_lp2();
 
@@ -61,6 +57,13 @@ static int tegra114_idle_power_down(struct cpuidle_device *dev,
 
 	return index;
 }
+
+static void tegra114_idle_enter_freeze(struct cpuidle_device *dev,
+				       struct cpuidle_driver *drv,
+				       int index)
+{
+       tegra114_idle_power_down(dev, drv, index);
+}
 #endif
 
 static struct cpuidle_driver tegra_idle_driver = {
@@ -72,8 +75,10 @@ static struct cpuidle_driver tegra_idle_driver = {
 #ifdef CONFIG_PM_SLEEP
 		[1] = {
 			.enter			= tegra114_idle_power_down,
+			.enter_freeze		= tegra114_idle_enter_freeze,
 			.exit_latency		= 500,
 			.target_residency	= 1000,
+			.flags			= CPUIDLE_FLAG_TIMER_STOP,
 			.power_usage		= 0,
 			.name			= "powered-down",
 			.desc			= "CPU power gated",
-- 
2.4.1

Re: [PATCH v3] ARM: tegra: cpuidle: implement cpuidle_state.enter_freeze()

[Lorenzo Pieralisi]

On Tue, May 19, 2015 at 03:49:12PM +0100, Tomeu Vizoso wrote:
> This callback is expected 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.
> 
> 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.
> 
> By setting the CPUIDLE_FLAG_TIMER_STOP flag, we can reuse the same code
> for both the enter() and enter_freeze() callbacks.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> ---
> 
> v3:	* Set CPUIDLE_FLAG_TIMER_STOP to simplify things as suggested by
> 	Lorenzo Pieralisi
> v2:	* Disable FIQs also when suspending-to-idle
> ---
>  arch/arm/mach-tegra/cpuidle-tegra114.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-tegra/cpuidle-tegra114.c b/arch/arm/mach-tegra/cpuidle-tegra114.c
> index 155807f..7e59ac0 100644
> --- a/arch/arm/mach-tegra/cpuidle-tegra114.c
> +++ b/arch/arm/mach-tegra/cpuidle-tegra114.c
> @@ -44,16 +44,12 @@ static int tegra114_idle_power_down(struct cpuidle_device *dev,
>  	tegra_set_cpu_in_lp2();
>  	cpu_pm_enter();
>  
> -	tick_broadcast_enter();
> -
>  	call_firmware_op(prepare_idle);
>  
>  	/* Do suspend by ourselves if the firmware does not implement it */
>  	if (call_firmware_op(do_idle, 0) == -ENOSYS)
>  		cpu_suspend(0, tegra30_sleep_cpu_secondary_finish);
>  
> -	tick_broadcast_exit();
> -
>  	cpu_pm_exit();
>  	tegra_clear_cpu_in_lp2();
>  
> @@ -61,6 +57,13 @@ static int tegra114_idle_power_down(struct cpuidle_device *dev,
>  
>  	return index;
>  }
> +
> +static void tegra114_idle_enter_freeze(struct cpuidle_device *dev,
> +				       struct cpuidle_driver *drv,
> +				       int index)
> +{
> +       tegra114_idle_power_down(dev, drv, index);
> +}

You do not need this function, remove it and update the enter_freeze
pointer below with tegra114_idle_power_down please.

Thanks,
Lorenzo

>  #endif
>  
>  static struct cpuidle_driver tegra_idle_driver = {
> @@ -72,8 +75,10 @@ static struct cpuidle_driver tegra_idle_driver = {
>  #ifdef CONFIG_PM_SLEEP
>  		[1] = {
>  			.enter			= tegra114_idle_power_down,
> +			.enter_freeze		= tegra114_idle_enter_freeze,
>  			.exit_latency		= 500,
>  			.target_residency	= 1000,
> +			.flags			= CPUIDLE_FLAG_TIMER_STOP,
>  			.power_usage		= 0,
>  			.name			= "powered-down",
>  			.desc			= "CPU power gated",
> -- 
> 2.4.1
> 

Re: [PATCH v3] ARM: tegra: cpuidle: implement cpuidle_state.enter_freeze()

[Tomeu Vizoso]

On 05/20/2015 10:44 AM, Lorenzo Pieralisi wrote:
> On Tue, May 19, 2015 at 03:49:12PM +0100, Tomeu Vizoso wrote:
>> This callback is expected 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.
>>
>> 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.
>>
>> By setting the CPUIDLE_FLAG_TIMER_STOP flag, we can reuse the same code
>> for both the enter() and enter_freeze() callbacks.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>
>> ---
>>
>> v3:	* Set CPUIDLE_FLAG_TIMER_STOP to simplify things as suggested by
>> 	Lorenzo Pieralisi
>> v2:	* Disable FIQs also when suspending-to-idle
>> ---
>>  arch/arm/mach-tegra/cpuidle-tegra114.c | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arm/mach-tegra/cpuidle-tegra114.c b/arch/arm/mach-tegra/cpuidle-tegra114.c
>> index 155807f..7e59ac0 100644
>> --- a/arch/arm/mach-tegra/cpuidle-tegra114.c
>> +++ b/arch/arm/mach-tegra/cpuidle-tegra114.c
>> @@ -44,16 +44,12 @@ static int tegra114_idle_power_down(struct cpuidle_device *dev,
>>  	tegra_set_cpu_in_lp2();
>>  	cpu_pm_enter();
>>  
>> -	tick_broadcast_enter();
>> -
>>  	call_firmware_op(prepare_idle);
>>  
>>  	/* Do suspend by ourselves if the firmware does not implement it */
>>  	if (call_firmware_op(do_idle, 0) == -ENOSYS)
>>  		cpu_suspend(0, tegra30_sleep_cpu_secondary_finish);
>>  
>> -	tick_broadcast_exit();
>> -
>>  	cpu_pm_exit();
>>  	tegra_clear_cpu_in_lp2();
>>  
>> @@ -61,6 +57,13 @@ static int tegra114_idle_power_down(struct cpuidle_device *dev,
>>  
>>  	return index;
>>  }
>> +
>> +static void tegra114_idle_enter_freeze(struct cpuidle_device *dev,
>> +				       struct cpuidle_driver *drv,
>> +				       int index)
>> +{
>> +       tegra114_idle_power_down(dev, drv, index);
>> +}
> 
> You do not need this function, remove it and update the enter_freeze
> pointer below with tegra114_idle_power_down please.

How so? The callbacks have different signatures.

Regards,

Tomeu

> Thanks,
> Lorenzo
> 
>>  #endif
>>  
>>  static struct cpuidle_driver tegra_idle_driver = {
>> @@ -72,8 +75,10 @@ static struct cpuidle_driver tegra_idle_driver = {
>>  #ifdef CONFIG_PM_SLEEP
>>  		[1] = {
>>  			.enter			= tegra114_idle_power_down,
>> +			.enter_freeze		= tegra114_idle_enter_freeze,
>>  			.exit_latency		= 500,
>>  			.target_residency	= 1000,
>> +			.flags			= CPUIDLE_FLAG_TIMER_STOP,
>>  			.power_usage		= 0,
>>  			.name			= "powered-down",
>>  			.desc			= "CPU power gated",
>> -- 
>> 2.4.1
>>

Re: [PATCH v3] ARM: tegra: cpuidle: implement cpuidle_state.enter_freeze()

[Lorenzo Pieralisi]

On Wed, May 20, 2015 at 09:55:41AM +0100, Tomeu Vizoso wrote:
> On 05/20/2015 10:44 AM, Lorenzo Pieralisi wrote:
> > On Tue, May 19, 2015 at 03:49:12PM +0100, Tomeu Vizoso wrote:
> >> This callback is expected 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.
> >>
> >> 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.
> >>
> >> By setting the CPUIDLE_FLAG_TIMER_STOP flag, we can reuse the same code
> >> for both the enter() and enter_freeze() callbacks.
> >>
> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> >> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >>
> >> ---
> >>
> >> v3:	* Set CPUIDLE_FLAG_TIMER_STOP to simplify things as suggested by
> >> 	Lorenzo Pieralisi
> >> v2:	* Disable FIQs also when suspending-to-idle
> >> ---
> >>  arch/arm/mach-tegra/cpuidle-tegra114.c | 13 +++++++++----
> >>  1 file changed, 9 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-tegra/cpuidle-tegra114.c b/arch/arm/mach-tegra/cpuidle-tegra114.c
> >> index 155807f..7e59ac0 100644
> >> --- a/arch/arm/mach-tegra/cpuidle-tegra114.c
> >> +++ b/arch/arm/mach-tegra/cpuidle-tegra114.c
> >> @@ -44,16 +44,12 @@ static int tegra114_idle_power_down(struct cpuidle_device *dev,
> >>  	tegra_set_cpu_in_lp2();
> >>  	cpu_pm_enter();
> >>  
> >> -	tick_broadcast_enter();
> >> -
> >>  	call_firmware_op(prepare_idle);
> >>  
> >>  	/* Do suspend by ourselves if the firmware does not implement it */
> >>  	if (call_firmware_op(do_idle, 0) == -ENOSYS)
> >>  		cpu_suspend(0, tegra30_sleep_cpu_secondary_finish);
> >>  
> >> -	tick_broadcast_exit();
> >> -
> >>  	cpu_pm_exit();
> >>  	tegra_clear_cpu_in_lp2();
> >>  
> >> @@ -61,6 +57,13 @@ static int tegra114_idle_power_down(struct cpuidle_device *dev,
> >>  
> >>  	return index;
> >>  }
> >> +
> >> +static void tegra114_idle_enter_freeze(struct cpuidle_device *dev,
> >> +				       struct cpuidle_driver *drv,
> >> +				       int index)
> >> +{
> >> +       tegra114_idle_power_down(dev, drv, index);
> >> +}
> > 
> > You do not need this function, remove it and update the enter_freeze
> > pointer below with tegra114_idle_power_down please.
> 
> How so? The callbacks have different signatures.

Sorry, glossed over it, never mind.

Lorenzo

Re: [PATCH v3] ARM: tegra: cpuidle: implement cpuidle_state.enter_freeze()

[Rafael J. Wysocki]

On Tuesday, May 19, 2015 04:49:12 PM Tomeu Vizoso wrote:
> This callback is expected 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.
> 
> 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.
> 
> By setting the CPUIDLE_FLAG_TIMER_STOP flag, we can reuse the same code
> for both the enter() and enter_freeze() callbacks.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Makes sense to me, but I suppose it should go in via arm-soc.


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH v3] ARM: tegra: cpuidle: implement cpuidle_state.enter_freeze()

[Tomeu Vizoso]

On 21 May 2015 at 03:02, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Tuesday, May 19, 2015 04:49:12 PM Tomeu Vizoso wrote:
>> This callback is expected 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.
>>
>> 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.
>>
>> By setting the CPUIDLE_FLAG_TIMER_STOP flag, we can reuse the same code
>> for both the enter() and enter_freeze() callbacks.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> Makes sense to me, but I suppose it should go in via arm-soc.

Hi Thierry,

what's the plan regarding merging of this patch?

It needs df8d9ee ("cpuidle: Run tick_broadcast_exit() with disabled
interrupts") so it doesn't break boot.

Thanks,

Tomeu

Re: [PATCH v3] ARM: tegra: cpuidle: implement cpuidle_state.enter_freeze()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1374 bytes --]

On Thu, Jun 18, 2015 at 09:09:29AM +0200, Tomeu Vizoso wrote:
> On 21 May 2015 at 03:02, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Tuesday, May 19, 2015 04:49:12 PM Tomeu Vizoso wrote:
> >> This callback is expected 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.
> >>
> >> 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.
> >>
> >> By setting the CPUIDLE_FLAG_TIMER_STOP flag, we can reuse the same code
> >> for both the enter() and enter_freeze() callbacks.
> >>
> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> >> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > Makes sense to me, but I suppose it should go in via arm-soc.
> 
> Hi Thierry,
> 
> what's the plan regarding merging of this patch?
> 
> It needs df8d9ee ("cpuidle: Run tick_broadcast_exit() with disabled
> interrupts") so it doesn't break boot.

By the looks of it the above patch was merged in 4.1, so I can easily
take this through the Tegra tree. The commit message doesn't say whether
or not this fixes a bug. Is this urgent? Should it go into 4.2? It
sounds more like it's a power-saving feature, so 4.3 is probably more
appropriate.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3] ARM: tegra: cpuidle: implement cpuidle_state.enter_freeze()

[Tomeu Vizoso]

On 29 June 2015 at 11:00, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Thu, Jun 18, 2015 at 09:09:29AM +0200, Tomeu Vizoso wrote:
>> On 21 May 2015 at 03:02, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> > On Tuesday, May 19, 2015 04:49:12 PM Tomeu Vizoso wrote:
>> >> This callback is expected 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.
>> >>
>> >> 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.
>> >>
>> >> By setting the CPUIDLE_FLAG_TIMER_STOP flag, we can reuse the same code
>> >> for both the enter() and enter_freeze() callbacks.
>> >>
>> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> >> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> >
>> > Makes sense to me, but I suppose it should go in via arm-soc.
>>
>> Hi Thierry,
>>
>> what's the plan regarding merging of this patch?
>>
>> It needs df8d9ee ("cpuidle: Run tick_broadcast_exit() with disabled
>> interrupts") so it doesn't break boot.
>
> By the looks of it the above patch was merged in 4.1, so I can easily
> take this through the Tegra tree. The commit message doesn't say whether
> or not this fixes a bug. Is this urgent? Should it go into 4.2? It
> sounds more like it's a power-saving feature, so 4.3 is probably more
> appropriate.

Yeah, it doesn't fix any bug and its effect should only be reduced
power consumption when in suspend-to-idle.

Thanks,

Tomeu

> Thierry

Re: [PATCH v3] ARM: tegra: cpuidle: implement cpuidle_state.enter_freeze()

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 943 bytes --]

On Tue, May 19, 2015 at 04:49:12PM +0200, Tomeu Vizoso wrote:
> This callback is expected 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.
> 
> 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.
> 
> By setting the CPUIDLE_FLAG_TIMER_STOP flag, we can reuse the same code
> for both the enter() and enter_freeze() callbacks.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> ---
> 
> v3:	* Set CPUIDLE_FLAG_TIMER_STOP to simplify things as suggested by
> 	Lorenzo Pieralisi
> v2:	* Disable FIQs also when suspending-to-idle
> ---
>  arch/arm/mach-tegra/cpuidle-tegra114.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)

Applied, thanks.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]