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

[Tomeu Vizoso]

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@xxxxxxxxxxxxx>
>> Cc: Rafael J. Wysocki <rafael.j.wysocki@xxxxxxxxx>
>> ---
>>  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@xxxxxxxxxxxxxxx
>> 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-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/