ARM: tegra: cpuidle: implement cpuidle_state.enter_freeze()
diff mbox

Message ID 1428490480-10144-1-git-send-email-tomeu.vizoso@collabora.com
State Deferred
Headers show

Commit Message

Tomeu Vizoso April 8, 2015, 10:54 a.m. UTC
This callback is expected to do the same as enter() only that all
non-wakeup IRQs are expected to be disabled.

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.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 arch/arm/mach-tegra/cpuidle-tegra114.c | 31 ++++++++++++++++++++++++-------
 1 file changed, 24 insertions(+), 7 deletions(-)

Comments

Lorenzo Pieralisi April 8, 2015, 11:55 a.m. UTC | #1
On Wed, Apr 08, 2015 at 11:54:38AM +0100, Tomeu Vizoso wrote:
> This callback is expected to do the same as enter() only that all
> non-wakeup IRQs are expected to be disabled.

This is not true or at least it is misworded. The enter_freeze() function
is expected to return from the state with IRQs disabled at CPU level, or
put it differently it must not re-enable IRQs while executing since 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.
> 
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  arch/arm/mach-tegra/cpuidle-tegra114.c | 31 ++++++++++++++++++++++++-------
>  1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm/mach-tegra/cpuidle-tegra114.c b/arch/arm/mach-tegra/cpuidle-tegra114.c
> index f2b586d..ef06001 100644
> --- a/arch/arm/mach-tegra/cpuidle-tegra114.c
> +++ b/arch/arm/mach-tegra/cpuidle-tegra114.c
> @@ -39,28 +39,44 @@ static int tegra114_idle_power_down(struct cpuidle_device *dev,
>  				    struct cpuidle_driver *drv,
>  				    int index)
>  {
> -	local_fiq_disable();
> -
>  	tegra_set_cpu_in_lp2();
>  	cpu_pm_enter();
>  
> -	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
> -
>  	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);
>  
> -	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
> -
>  	cpu_pm_exit();
>  	tegra_clear_cpu_in_lp2();
>  
> +	return index;
> +}
> +
> +static int tegra114_idle_enter(struct cpuidle_device *dev,
> +			       struct cpuidle_driver *drv,
> +			       int index)
> +{
> +	local_fiq_disable();
> +
> +	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
> +
> +	index = tegra114_idle_power_down(dev, drv, index);
> +
> +	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
> +
>  	local_fiq_enable();
>  
>  	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);

Cool. So if the problem is FIQs, you don't disabled them on entry
which means you enter the "frozen" state with FIQs enabled and tick frozen,
unless I am missing something.

The question here is: are we allowed to enable FIQs before returning
from an enter_freeze() call (and to enter it with FIQs enabled) ?

If we are not your code here certainly does not solve the issue, since
it does _not_ disable FIQs upon enter_freeze call anyway.

Lorenzo

> +}
>  #endif
>  
>  static struct cpuidle_driver tegra_idle_driver = {
> @@ -71,7 +87,8 @@ static struct cpuidle_driver tegra_idle_driver = {
>  		[0] = ARM_CPUIDLE_WFI_STATE_PWR(600),
>  #ifdef CONFIG_PM_SLEEP
>  		[1] = {
> -			.enter			= tegra114_idle_power_down,
> +			.enter			= tegra114_idle_enter,
> +			.enter_freeze		= tegra114_idle_enter_freeze,
>  			.exit_latency		= 500,
>  			.target_residency	= 1000,
>  			.power_usage		= 0,
> -- 
> 2.3.4
> 
> --
> 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/
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomeu Vizoso April 9, 2015, 9:18 a.m. UTC | #2
On 04/08/2015 01:55 PM, Lorenzo Pieralisi wrote:
> On Wed, Apr 08, 2015 at 11:54:38AM +0100, Tomeu Vizoso wrote:
>> This callback is expected to do the same as enter() only that all
>> non-wakeup IRQs are expected to be disabled.
> 
> This is not true or at least it is misworded. The enter_freeze() function
> is expected to return from the state with IRQs disabled at CPU level, or
> put it differently it must not re-enable IRQs while executing since the
> tick is frozen.

True, only that it mentions interrupts in general, not just IRQs (I
don't know if the terminology used in the base code matches the one in
ARM's documentation).

/*
 * 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.
>>
>> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> ---
>>  arch/arm/mach-tegra/cpuidle-tegra114.c | 31 ++++++++++++++++++++++++-------
>>  1 file changed, 24 insertions(+), 7 deletions(-)
>>
>> diff --git a/arch/arm/mach-tegra/cpuidle-tegra114.c b/arch/arm/mach-tegra/cpuidle-tegra114.c
>> index f2b586d..ef06001 100644
>> --- a/arch/arm/mach-tegra/cpuidle-tegra114.c
>> +++ b/arch/arm/mach-tegra/cpuidle-tegra114.c
>> @@ -39,28 +39,44 @@ static int tegra114_idle_power_down(struct cpuidle_device *dev,
>>  				    struct cpuidle_driver *drv,
>>  				    int index)
>>  {
>> -	local_fiq_disable();
>> -
>>  	tegra_set_cpu_in_lp2();
>>  	cpu_pm_enter();
>>  
>> -	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
>> -
>>  	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);
>>  
>> -	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
>> -
>>  	cpu_pm_exit();
>>  	tegra_clear_cpu_in_lp2();
>>  
>> +	return index;
>> +}
>> +
>> +static int tegra114_idle_enter(struct cpuidle_device *dev,
>> +			       struct cpuidle_driver *drv,
>> +			       int index)
>> +{
>> +	local_fiq_disable();
>> +
>> +	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
>> +
>> +	index = tegra114_idle_power_down(dev, drv, index);
>> +
>> +	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
>> +
>>  	local_fiq_enable();
>>  
>>  	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);
> 
> Cool. So if the problem is FIQs, you don't disabled them on entry
> which means you enter the "frozen" state with FIQs enabled and tick frozen,
> unless I am missing something.

I have gone a bit deeper in the code and that's correct, AFAICS.

> The question here is: are we allowed to enable FIQs before returning
> from an enter_freeze() call (and to enter it with FIQs enabled) ?
> 
> If we are not your code here certainly does not solve the issue, since
> it does _not_ disable FIQs upon enter_freeze call anyway.

I think doing that would go against the wording of the comment I quoted
above, so I see two ways of fixing this:

* Change the wording to refer to normal IRQs and leave the task of
enabling and disabling FIQs to the enter_freeze implementation (ugly and
I don't see any good reason for this)

* Have FIQs already disabled when enter_freeze gets called, probably by
having arch_cpu_idle_enter do it on ARM (and the inverse in
arch_cpu_idle_exit)?

Rafael, what's your opinion on this?

Thanks,

Tomeu

> Lorenzo
> 
>> +}
>>  #endif
>>  
>>  static struct cpuidle_driver tegra_idle_driver = {
>> @@ -71,7 +87,8 @@ static struct cpuidle_driver tegra_idle_driver = {
>>  		[0] = ARM_CPUIDLE_WFI_STATE_PWR(600),
>>  #ifdef CONFIG_PM_SLEEP
>>  		[1] = {
>> -			.enter			= tegra114_idle_power_down,
>> +			.enter			= tegra114_idle_enter,
>> +			.enter_freeze		= tegra114_idle_enter_freeze,
>>  			.exit_latency		= 500,
>>  			.target_residency	= 1000,
>>  			.power_usage		= 0,
>> -- 
>> 2.3.4
>>
>> --
>> 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/
>>

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" 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 April 9, 2015, 9:19 p.m. UTC | #3
On Thursday, April 09, 2015 11:18:25 AM Tomeu Vizoso wrote:
> On 04/08/2015 01:55 PM, Lorenzo Pieralisi wrote:
> > On Wed, Apr 08, 2015 at 11:54:38AM +0100, Tomeu Vizoso wrote:
> >> This callback is expected to do the same as enter() only that all
> >> non-wakeup IRQs are expected to be disabled.
> > 
> > This is not true or at least it is misworded. The enter_freeze() function
> > is expected to return from the state with IRQs disabled at CPU level, or
> > put it differently it must not re-enable IRQs while executing since the
> > tick is frozen.
> 
> True, only that it mentions interrupts in general, not just IRQs (I
> don't know if the terminology used in the base code matches the one in
> ARM's documentation).
> 
> /*
>  * 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.
>  */

This means interrupts on the local CPU (ie. the thing done by local_irq_disable()).

> >> 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.
> >>
> >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> >> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >> ---
> >>  arch/arm/mach-tegra/cpuidle-tegra114.c | 31 ++++++++++++++++++++++++-------
> >>  1 file changed, 24 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/arch/arm/mach-tegra/cpuidle-tegra114.c b/arch/arm/mach-tegra/cpuidle-tegra114.c
> >> index f2b586d..ef06001 100644
> >> --- a/arch/arm/mach-tegra/cpuidle-tegra114.c
> >> +++ b/arch/arm/mach-tegra/cpuidle-tegra114.c
> >> @@ -39,28 +39,44 @@ static int tegra114_idle_power_down(struct cpuidle_device *dev,
> >>  				    struct cpuidle_driver *drv,
> >>  				    int index)
> >>  {
> >> -	local_fiq_disable();
> >> -
> >>  	tegra_set_cpu_in_lp2();
> >>  	cpu_pm_enter();
> >>  
> >> -	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
> >> -
> >>  	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);
> >>  
> >> -	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
> >> -
> >>  	cpu_pm_exit();
> >>  	tegra_clear_cpu_in_lp2();
> >>  
> >> +	return index;
> >> +}
> >> +
> >> +static int tegra114_idle_enter(struct cpuidle_device *dev,
> >> +			       struct cpuidle_driver *drv,
> >> +			       int index)
> >> +{
> >> +	local_fiq_disable();
> >> +
> >> +	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
> >> +
> >> +	index = tegra114_idle_power_down(dev, drv, index);
> >> +
> >> +	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
> >> +
> >>  	local_fiq_enable();
> >>  
> >>  	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);
> > 
> > Cool. So if the problem is FIQs, you don't disabled them on entry
> > which means you enter the "frozen" state with FIQs enabled and tick frozen,
> > unless I am missing something.
> 
> I have gone a bit deeper in the code and that's correct, AFAICS.
> 
> > The question here is: are we allowed to enable FIQs before returning
> > from an enter_freeze() call (and to enter it with FIQs enabled) ?
> > 
> > If we are not your code here certainly does not solve the issue, since
> > it does _not_ disable FIQs upon enter_freeze call anyway.
> 
> I think doing that would go against the wording of the comment I quoted
> above, so I see two ways of fixing this:
> 
> * Change the wording to refer to normal IRQs and leave the task of
> enabling and disabling FIQs to the enter_freeze implementation (ugly and
> I don't see any good reason for this)
> 
> * Have FIQs already disabled when enter_freeze gets called, probably by
> having arch_cpu_idle_enter do it on ARM (and the inverse in
> arch_cpu_idle_exit)?
> 
> Rafael, what's your opinion on this?

I don't know what FIQs are. :-)

->enter_freeze is entered with interrupts disabled on the local CPU.  It is
not supposed to re-enable them.  That is, while in the ->enter_freeze callback
routine, the CPU must not be interrupted aby anything other than NMI.
Lorenzo Pieralisi April 10, 2015, 10:08 a.m. UTC | #4
On Thu, Apr 09, 2015 at 10:19:59PM +0100, Rafael J. Wysocki wrote:
> On Thursday, April 09, 2015 11:18:25 AM Tomeu Vizoso wrote:
> > On 04/08/2015 01:55 PM, Lorenzo Pieralisi wrote:
> > > On Wed, Apr 08, 2015 at 11:54:38AM +0100, Tomeu Vizoso wrote:
> > >> This callback is expected to do the same as enter() only that all
> > >> non-wakeup IRQs are expected to be disabled.
> > > 
> > > This is not true or at least it is misworded. The enter_freeze() function
> > > is expected to return from the state with IRQs disabled at CPU level, or
> > > put it differently it must not re-enable IRQs while executing since the
> > > tick is frozen.
> > 
> > True, only that it mentions interrupts in general, not just IRQs (I
> > don't know if the terminology used in the base code matches the one in
> > ARM's documentation).
> > 
> > /*
> >  * 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.
> >  */
> 
> This means interrupts on the local CPU (ie. the thing done by local_irq_disable()).
> 
> > >> 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.
> > >>
> > >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> > >> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > >> ---
> > >>  arch/arm/mach-tegra/cpuidle-tegra114.c | 31 ++++++++++++++++++++++++-------
> > >>  1 file changed, 24 insertions(+), 7 deletions(-)
> > >>
> > >> diff --git a/arch/arm/mach-tegra/cpuidle-tegra114.c b/arch/arm/mach-tegra/cpuidle-tegra114.c
> > >> index f2b586d..ef06001 100644
> > >> --- a/arch/arm/mach-tegra/cpuidle-tegra114.c
> > >> +++ b/arch/arm/mach-tegra/cpuidle-tegra114.c
> > >> @@ -39,28 +39,44 @@ static int tegra114_idle_power_down(struct cpuidle_device *dev,
> > >>  				    struct cpuidle_driver *drv,
> > >>  				    int index)
> > >>  {
> > >> -	local_fiq_disable();
> > >> -
> > >>  	tegra_set_cpu_in_lp2();
> > >>  	cpu_pm_enter();
> > >>  
> > >> -	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
> > >> -
> > >>  	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);
> > >>  
> > >> -	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
> > >> -
> > >>  	cpu_pm_exit();
> > >>  	tegra_clear_cpu_in_lp2();
> > >>  
> > >> +	return index;
> > >> +}
> > >> +
> > >> +static int tegra114_idle_enter(struct cpuidle_device *dev,
> > >> +			       struct cpuidle_driver *drv,
> > >> +			       int index)
> > >> +{
> > >> +	local_fiq_disable();
> > >> +
> > >> +	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
> > >> +
> > >> +	index = tegra114_idle_power_down(dev, drv, index);
> > >> +
> > >> +	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
> > >> +
> > >>  	local_fiq_enable();
> > >>  
> > >>  	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);
> > > 
> > > Cool. So if the problem is FIQs, you don't disabled them on entry
> > > which means you enter the "frozen" state with FIQs enabled and tick frozen,
> > > unless I am missing something.
> > 
> > I have gone a bit deeper in the code and that's correct, AFAICS.
> > 
> > > The question here is: are we allowed to enable FIQs before returning
> > > from an enter_freeze() call (and to enter it with FIQs enabled) ?
> > > 
> > > If we are not your code here certainly does not solve the issue, since
> > > it does _not_ disable FIQs upon enter_freeze call anyway.
> > 
> > I think doing that would go against the wording of the comment I quoted
> > above, so I see two ways of fixing this:
> > 
> > * Change the wording to refer to normal IRQs and leave the task of
> > enabling and disabling FIQs to the enter_freeze implementation (ugly and
> > I don't see any good reason for this)
> > 
> > * Have FIQs already disabled when enter_freeze gets called, probably by
> > having arch_cpu_idle_enter do it on ARM (and the inverse in
> > arch_cpu_idle_exit)?
> > 
> > Rafael, what's your opinion on this?
> 
> I don't know what FIQs are. :-)

In short, fast IRQs, it is a separate IRQ line handled as a separate
exception source with some private (banked) registers that minimize registers
saving/restoring. They are not identical to NMI on x86, since
their behaviour (handling) may be overriden by platforms and they
can be masked.

> ->enter_freeze is entered with interrupts disabled on the local CPU.  It is
> not supposed to re-enable them.  That is, while in the ->enter_freeze callback
> routine, the CPU must not be interrupted aby anything other than NMI.

It boils down to what FIQs handlers are allowed to do with tick frozen
and what they are (may be) currently used for.

Russell has more insights on this than I do, in particular what FIQs are
currently used for on ARM and if we can leave them enabled safely with tick
frozen.

Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomeu Vizoso April 16, 2015, 2:37 p.m. UTC | #5
On 10 April 2015 at 12:08, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Thu, Apr 09, 2015 at 10:19:59PM +0100, Rafael J. Wysocki wrote:
>> On Thursday, April 09, 2015 11:18:25 AM Tomeu Vizoso wrote:
>> > On 04/08/2015 01:55 PM, Lorenzo Pieralisi wrote:
>> > > On Wed, Apr 08, 2015 at 11:54:38AM +0100, Tomeu Vizoso wrote:
>> > >> This callback is expected to do the same as enter() only that all
>> > >> non-wakeup IRQs are expected to be disabled.
>> > >
>> > > This is not true or at least it is misworded. The enter_freeze() function
>> > > is expected to return from the state with IRQs disabled at CPU level, or
>> > > put it differently it must not re-enable IRQs while executing since the
>> > > tick is frozen.
>> >
>> > True, only that it mentions interrupts in general, not just IRQs (I
>> > don't know if the terminology used in the base code matches the one in
>> > ARM's documentation).
>> >
>> > /*
>> >  * 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.
>> >  */
>>
>> This means interrupts on the local CPU (ie. the thing done by local_irq_disable()).
>>
>> > >> 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.
>> > >>
>> > >> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
>> > >> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>> > >> ---
>> > >>  arch/arm/mach-tegra/cpuidle-tegra114.c | 31 ++++++++++++++++++++++++-------
>> > >>  1 file changed, 24 insertions(+), 7 deletions(-)
>> > >>
>> > >> diff --git a/arch/arm/mach-tegra/cpuidle-tegra114.c b/arch/arm/mach-tegra/cpuidle-tegra114.c
>> > >> index f2b586d..ef06001 100644
>> > >> --- a/arch/arm/mach-tegra/cpuidle-tegra114.c
>> > >> +++ b/arch/arm/mach-tegra/cpuidle-tegra114.c
>> > >> @@ -39,28 +39,44 @@ static int tegra114_idle_power_down(struct cpuidle_device *dev,
>> > >>                                      struct cpuidle_driver *drv,
>> > >>                                      int index)
>> > >>  {
>> > >> -        local_fiq_disable();
>> > >> -
>> > >>          tegra_set_cpu_in_lp2();
>> > >>          cpu_pm_enter();
>> > >>
>> > >> -        clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
>> > >> -
>> > >>          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);
>> > >>
>> > >> -        clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
>> > >> -
>> > >>          cpu_pm_exit();
>> > >>          tegra_clear_cpu_in_lp2();
>> > >>
>> > >> +        return index;
>> > >> +}
>> > >> +
>> > >> +static int tegra114_idle_enter(struct cpuidle_device *dev,
>> > >> +                               struct cpuidle_driver *drv,
>> > >> +                               int index)
>> > >> +{
>> > >> +        local_fiq_disable();
>> > >> +
>> > >> +        clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
>> > >> +
>> > >> +        index = tegra114_idle_power_down(dev, drv, index);
>> > >> +
>> > >> +        clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
>> > >> +
>> > >>          local_fiq_enable();
>> > >>
>> > >>          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);
>> > >
>> > > Cool. So if the problem is FIQs, you don't disabled them on entry
>> > > which means you enter the "frozen" state with FIQs enabled and tick frozen,
>> > > unless I am missing something.
>> >
>> > I have gone a bit deeper in the code and that's correct, AFAICS.
>> >
>> > > The question here is: are we allowed to enable FIQs before returning
>> > > from an enter_freeze() call (and to enter it with FIQs enabled) ?
>> > >
>> > > If we are not your code here certainly does not solve the issue, since
>> > > it does _not_ disable FIQs upon enter_freeze call anyway.
>> >
>> > I think doing that would go against the wording of the comment I quoted
>> > above, so I see two ways of fixing this:
>> >
>> > * Change the wording to refer to normal IRQs and leave the task of
>> > enabling and disabling FIQs to the enter_freeze implementation (ugly and
>> > I don't see any good reason for this)
>> >
>> > * Have FIQs already disabled when enter_freeze gets called, probably by
>> > having arch_cpu_idle_enter do it on ARM (and the inverse in
>> > arch_cpu_idle_exit)?
>> >
>> > Rafael, what's your opinion on this?
>>
>> I don't know what FIQs are. :-)
>
> In short, fast IRQs, it is a separate IRQ line handled as a separate
> exception source with some private (banked) registers that minimize registers
> saving/restoring. They are not identical to NMI on x86, since
> their behaviour (handling) may be overriden by platforms and they
> can be masked.
>
>> ->enter_freeze is entered with interrupts disabled on the local CPU.  It is
>> not supposed to re-enable them.  That is, while in the ->enter_freeze callback
>> routine, the CPU must not be interrupted aby anything other than NMI.
>
> It boils down to what FIQs handlers are allowed to do with tick frozen
> and what they are (may be) currently used for.
>
> Russell has more insights on this than I do, in particular what FIQs are
> currently used for on ARM and if we can leave them enabled safely with tick
> frozen.

But even if it's currently safe to leave them enabled, is there any
reason for not disabling them?

Regards,

Tomeu

> Lorenzo
> --
> 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/
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lorenzo Pieralisi April 17, 2015, 2:08 p.m. UTC | #6
On Thu, Apr 16, 2015 at 03:37:19PM +0100, Tomeu Vizoso wrote:

[...]

> >> I don't know what FIQs are. :-)
> >
> > In short, fast IRQs, it is a separate IRQ line handled as a separate
> > exception source with some private (banked) registers that minimize registers
> > saving/restoring. They are not identical to NMI on x86, since
> > their behaviour (handling) may be overriden by platforms and they
> > can be masked.
> >
> >> ->enter_freeze is entered with interrupts disabled on the local CPU.  It is
> >> not supposed to re-enable them.  That is, while in the ->enter_freeze callback
> >> routine, the CPU must not be interrupted aby anything other than NMI.
> >
> > It boils down to what FIQs handlers are allowed to do with tick frozen
> > and what they are (may be) currently used for.
> >
> > Russell has more insights on this than I do, in particular what FIQs are
> > currently used for on ARM and if we can leave them enabled safely with tick
> > frozen.
> 
> But even if it's currently safe to leave them enabled, is there any
> reason for not disabling them?

Ok, the point here is: either it is safe, and you leave them enabled,
or it is not and we must disable them *before* enter_freeze() is entered.

Disabling them in the platform enter_freeze() hook does not make sense,
because this means we run with FIQs enabled with tick frozen, either
it is safe or it is not, it can't be both.

I would ask Russell opinion on this, before making any decision.

Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomeu Vizoso April 17, 2015, 3:02 p.m. UTC | #7
On 17 April 2015 at 16:08, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Thu, Apr 16, 2015 at 03:37:19PM +0100, Tomeu Vizoso wrote:
>
> [...]
>
>> >> I don't know what FIQs are. :-)
>> >
>> > In short, fast IRQs, it is a separate IRQ line handled as a separate
>> > exception source with some private (banked) registers that minimize registers
>> > saving/restoring. They are not identical to NMI on x86, since
>> > their behaviour (handling) may be overriden by platforms and they
>> > can be masked.
>> >
>> >> ->enter_freeze is entered with interrupts disabled on the local CPU.  It is
>> >> not supposed to re-enable them.  That is, while in the ->enter_freeze callback
>> >> routine, the CPU must not be interrupted aby anything other than NMI.
>> >
>> > It boils down to what FIQs handlers are allowed to do with tick frozen
>> > and what they are (may be) currently used for.
>> >
>> > Russell has more insights on this than I do, in particular what FIQs are
>> > currently used for on ARM and if we can leave them enabled safely with tick
>> > frozen.
>>
>> But even if it's currently safe to leave them enabled, is there any
>> reason for not disabling them?
>
> Ok, the point here is: either it is safe, and you leave them enabled,
> or it is not and we must disable them *before* enter_freeze() is entered.
>
> Disabling them in the platform enter_freeze() hook does not make sense,
> because this means we run with FIQs enabled with tick frozen, either
> it is safe or it is not, it can't be both.

Sure, that's why I proposed doing it in arch_cpu_idle_enter/exit.

> I would ask Russell opinion on this, before making any decision.

Sure, that would be very welcome.

Regards,

Tomeu

> Lorenzo
> --
> 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/
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomeu Vizoso April 28, 2015, 3:07 p.m. UTC | #8
On 17 April 2015 at 17:02, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> On 17 April 2015 at 16:08, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
>> On Thu, Apr 16, 2015 at 03:37:19PM +0100, Tomeu Vizoso wrote:
>>
>> [...]
>>
>>> >> I don't know what FIQs are. :-)
>>> >
>>> > In short, fast IRQs, it is a separate IRQ line handled as a separate
>>> > exception source with some private (banked) registers that minimize registers
>>> > saving/restoring. They are not identical to NMI on x86, since
>>> > their behaviour (handling) may be overriden by platforms and they
>>> > can be masked.
>>> >
>>> >> ->enter_freeze is entered with interrupts disabled on the local CPU.  It is
>>> >> not supposed to re-enable them.  That is, while in the ->enter_freeze callback
>>> >> routine, the CPU must not be interrupted aby anything other than NMI.
>>> >
>>> > It boils down to what FIQs handlers are allowed to do with tick frozen
>>> > and what they are (may be) currently used for.
>>> >
>>> > Russell has more insights on this than I do, in particular what FIQs are
>>> > currently used for on ARM and if we can leave them enabled safely with tick
>>> > frozen.
>>>
>>> But even if it's currently safe to leave them enabled, is there any
>>> reason for not disabling them?
>>
>> Ok, the point here is: either it is safe, and you leave them enabled,
>> or it is not and we must disable them *before* enter_freeze() is entered.
>>
>> Disabling them in the platform enter_freeze() hook does not make sense,
>> because this means we run with FIQs enabled with tick frozen, either
>> it is safe or it is not, it can't be both.
>
> Sure, that's why I proposed doing it in arch_cpu_idle_enter/exit.
>
>> I would ask Russell opinion on this, before making any decision.
>
> Sure, that would be very welcome.

Hi Russell,

do you think FIQs should be disabled when the tick is frozen when
going to suspend to idle?

Thanks,

Tomeu
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomeu Vizoso May 15, 2015, 9:03 a.m. UTC | #9
On 17 April 2015 at 16:08, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> On Thu, Apr 16, 2015 at 03:37:19PM +0100, Tomeu Vizoso wrote:
>
> [...]
>
>> >> I don't know what FIQs are. :-)
>> >
>> > In short, fast IRQs, it is a separate IRQ line handled as a separate
>> > exception source with some private (banked) registers that minimize registers
>> > saving/restoring. They are not identical to NMI on x86, since
>> > their behaviour (handling) may be overriden by platforms and they
>> > can be masked.
>> >
>> >> ->enter_freeze is entered with interrupts disabled on the local CPU.  It is
>> >> not supposed to re-enable them.  That is, while in the ->enter_freeze callback
>> >> routine, the CPU must not be interrupted aby anything other than NMI.
>> >
>> > It boils down to what FIQs handlers are allowed to do with tick frozen
>> > and what they are (may be) currently used for.
>> >
>> > Russell has more insights on this than I do, in particular what FIQs are
>> > currently used for on ARM and if we can leave them enabled safely with tick
>> > frozen.
>>
>> But even if it's currently safe to leave them enabled, is there any
>> reason for not disabling them?
>
> Ok, the point here is: either it is safe, and you leave them enabled,
> or it is not and we must disable them *before* enter_freeze() is entered.
>
> Disabling them in the platform enter_freeze() hook does not make sense,
> because this means we run with FIQs enabled with tick frozen, either
> it is safe or it is not, it can't be both.

I have been looking and asking around, and seems like we should
actually leave FIQs enabled when the tick is frozen and only disable
them within enter_freeze.

My understanding is that FIQ handlers are extremely limited in what
they can do, with the main use being NMI-like functionality. There are
a handful of other FIQ handlers in mainline, but they don't call into
any other kernel code and limit themselves to doing some basic I/O.

The only reason why they should be disabled in enter_freeze is that
the context of the CPU that is going to a lower power state could
become corrupted if a FIQ gets fired during a save or restore
operation.

Regards,

Tomeu

> I would ask Russell opinion on this, before making any decision.
>
> Lorenzo
> --
> 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/
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux May 15, 2015, 11:30 p.m. UTC | #10
On Fri, May 15, 2015 at 11:03:35AM +0200, Tomeu Vizoso wrote:
> I have been looking and asking around, and seems like we should
> actually leave FIQs enabled when the tick is frozen and only disable
> them within enter_freeze.
> 
> My understanding is that FIQ handlers are extremely limited in what
> they can do, with the main use being NMI-like functionality. There are
> a handful of other FIQ handlers in mainline, but they don't call into
> any other kernel code and limit themselves to doing some basic I/O.
> 
> The only reason why they should be disabled in enter_freeze is that
> the context of the CPU that is going to a lower power state could
> become corrupted if a FIQ gets fired during a save or restore
> operation.

Right, that makes total sense.

(And yes, FIQ handlers are very limited in what they're allowed to do,
since they can't take any locks without the potential for deadlock.)

Patch
diff mbox

diff --git a/arch/arm/mach-tegra/cpuidle-tegra114.c b/arch/arm/mach-tegra/cpuidle-tegra114.c
index f2b586d..ef06001 100644
--- a/arch/arm/mach-tegra/cpuidle-tegra114.c
+++ b/arch/arm/mach-tegra/cpuidle-tegra114.c
@@ -39,28 +39,44 @@  static int tegra114_idle_power_down(struct cpuidle_device *dev,
 				    struct cpuidle_driver *drv,
 				    int index)
 {
-	local_fiq_disable();
-
 	tegra_set_cpu_in_lp2();
 	cpu_pm_enter();
 
-	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
-
 	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);
 
-	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
-
 	cpu_pm_exit();
 	tegra_clear_cpu_in_lp2();
 
+	return index;
+}
+
+static int tegra114_idle_enter(struct cpuidle_device *dev,
+			       struct cpuidle_driver *drv,
+			       int index)
+{
+	local_fiq_disable();
+
+	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
+
+	index = tegra114_idle_power_down(dev, drv, index);
+
+	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_EXIT, &dev->cpu);
+
 	local_fiq_enable();
 
 	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 = {
@@ -71,7 +87,8 @@  static struct cpuidle_driver tegra_idle_driver = {
 		[0] = ARM_CPUIDLE_WFI_STATE_PWR(600),
 #ifdef CONFIG_PM_SLEEP
 		[1] = {
-			.enter			= tegra114_idle_power_down,
+			.enter			= tegra114_idle_enter,
+			.enter_freeze		= tegra114_idle_enter_freeze,
 			.exit_latency		= 500,
 			.target_residency	= 1000,
 			.power_usage		= 0,