Re: [PATCH 3/3] ARM: tegra114: cpuidle: add powered-down state

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 05/31/2013 11:12 AM, Joseph Lo wrote:
> Hi Daniel,
> 
> Thanks for your review.
> 
> On Thu, 2013-05-30 at 22:35 +0800, Daniel Lezcano wrote:
>> On 05/30/2013 01:19 PM, Joseph Lo wrote:
>>> This supports CPU core power down on each CPU when CPU idle. When CPU go
>>> into this state, it saves it's context and needs a proper configuration
>>> in flow controller to power gate the CPU when CPU runs into WFI
>>> instruction. And the CPU also needs to set the IRQ as CPU power down idle
>>> wake up event in flow controller.
>>>
>>> Signed-off-by: Joseph Lo <josephl@nvidia.com>
>>> ---
>>
>> Hi Joseph,
>>
>> a new flag has been added in the cpuidle framework to let this one to
>> handle the timer broadcast : CPUIDLE_FLAG_TIMER_STOP
>>
>> It is the commit b60e6a0eb0273132cbb60a9806abf5f47a4aee1c
>>
>> You can get rid of the clockevent notify stuff by adding this flag to
>> the state.
>>
> 
> I just tested this flag on Tegra20, 30 and 114. It is only OK on
> Tegra20. It caused a warning message below on Tegra30/114 at boot time.
> 
> I gave it a quick check I found it can't clean
> tick_broadcast_pending_mask sometimes if apply the flag. But I keep the
> code right now it's always OK. Not sure why? 

Is it possible you didn't intialized the timer broadcast with
CLOCK_EVT_NOTIFY_BROADCAST_ON in the driver and then this one is
actually disabled for you driver ?

> [   25.629539] ------------[ cut here ]------------
> [   25.629559] WARNING: CPU: 2 PID: 0 at
> kernel/time/tick-broadcast.c:578 tick_broadcast_oneshot_control
> +0x1a4/0x1d0()
> [   25.629565] Modules linked in:
> [   25.629574] CPU: 2 PID: 0 Comm: swapper/2 Not tainted
> 3.10.0-rc3-next-20130529+ #15
> [   25.629601] [<c00148f4>] (unwind_backtrace+0x0/0xf8) from
> [<c0011040>] (show_stack+0x10/0x14)
> [   25.629616] [<c0011040>] (show_stack+0x10/0x14) from [<c0504248>]
> (dump_stack+0x80/0xc4)
> [   25.629633] [<c0504248>] (dump_stack+0x80/0xc4) from [<c00231ac>]
> (warn_slowpath_common+0x64/0x88)
> [   25.629646] [<c00231ac>] (warn_slowpath_common+0x64/0x88) from
> [<c00231ec>] (warn_slowpath_null+0x1c/0x24)
> [   25.629657] [<c00231ec>] (warn_slowpath_null+0x1c/0x24) from
> [<c00667f8>] (tick_broadcast_oneshot_control+0x1a4/0x1d0)
> [   25.629670] [<c00667f8>] (tick_broadcast_oneshot_control+0x1a4/0x1d0)
> from [<c0065cd0>] (tick_notify+0x240/0x40c)
> [   25.629685] [<c0065cd0>] (tick_notify+0x240/0x40c) from [<c0044724>]
> (notifier_call_chain+0x44/0x84)
> [   25.629699] [<c0044724>] (notifier_call_chain+0x44/0x84) from
> [<c0044828>] (raw_notifier_call_chain+0x18/0x20)
> [   25.629712] [<c0044828>] (raw_notifier_call_chain+0x18/0x20) from
> [<c00650cc>] (clockevents_notify+0x28/0x170)
> [   25.629726] [<c00650cc>] (clockevents_notify+0x28/0x170) from
> [<c033f1f0>] (cpuidle_idle_call+0x11c/0x168)
> [   25.629739] [<c033f1f0>] (cpuidle_idle_call+0x11c/0x168) from
> [<c000ea94>] (arch_cpu_idle+0x8/0x38)
> [   25.629755] [<c000ea94>] (arch_cpu_idle+0x8/0x38) from [<c005ea80>]
> (cpu_startup_entry+0x60/0x134)
> [   25.629767] [<c005ea80>] (cpu_startup_entry+0x60/0x134) from
> [<804fe9a4>] (0x804fe9a4)
> [   25.629772] ---[ end trace 5484e77e2531bccc ]---
> 
> 
>> Also, can you clarify why tegra3 code is used in tegra114 ?
>>
> Yes, because the flow controller that was used to control CPU power is
> similar for both SoCs. The function is shared for Tegra30/114.
> 
>>
>>>  arch/arm/mach-tegra/cpuidle-tegra114.c | 62 +++++++++++++++++++++++++++++++++-
>>>  1 file changed, 61 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/mach-tegra/cpuidle-tegra114.c b/arch/arm/mach-tegra/cpuidle-tegra114.c
>>> index 1d1c602..e7d21f5 100644
>>> --- a/arch/arm/mach-tegra/cpuidle-tegra114.c
>>> +++ b/arch/arm/mach-tegra/cpuidle-tegra114.c
>>> @@ -17,19 +17,79 @@
>>>  #include <linux/kernel.h>
>>>  #include <linux/module.h>
>>>  #include <linux/cpuidle.h>
>>> +#include <linux/cpu_pm.h>
>>> +#include <linux/clockchips.h>
>>>  
>>>  #include <asm/cpuidle.h>
>>> +#include <asm/suspend.h>
>>> +#include <asm/smp_plat.h>
>>> +
>>> +#include "pm.h"
>>> +#include "sleep.h"
>>> +
>>> +#ifdef CONFIG_PM_SLEEP
>>> +static int tegra114_idle_power_down(struct cpuidle_device *dev,
>>> +				    struct cpuidle_driver *drv,
>>> +				    int index);
>>
>> Isn't possible to move the driver declaration after the
>> tegra114_idle_power_down function, before the init function, so we
>> prevent a forward declaration ?
>>
>>> +#define TEGRA114_MAX_STATES 2
>>> +#else
>>> +#define TEGRA114_MAX_STATES 1
>>> +#endif
>>>  
>>>  static struct cpuidle_driver tegra_idle_driver = {
>>>  	.name = "tegra_idle",
>>>  	.owner = THIS_MODULE,
>>> -	.state_count = 1,
>>> +	.state_count = TEGRA114_MAX_STATES,
>>>  	.states = {
>>>  		[0] = ARM_CPUIDLE_WFI_STATE_PWR(600),
>>> +#ifdef CONFIG_PM_SLEEP
>>> +		[1] = {
>>> +			.enter			= tegra114_idle_power_down,
>>> +			.exit_latency		= 500,
>>> +			.target_residency	= 1000,
>>> +			.power_usage		= 0,
>>> +			.flags			= CPUIDLE_FLAG_TIME_VALID,
>>> +			.name			= "powered-down",
>>> +			.desc			= "CPU power gated",
>>> +		},
>>> +#endif
>>>  	},
>>>  };
>>>  
>>> +#ifdef CONFIG_PM_SLEEP
>>> +static int tegra114_idle_power_down(struct cpuidle_device *dev,
>>> +				    struct cpuidle_driver *drv,
>>> +				    int index)
>>> +{
>>> +	u32 cpu = is_smp() ? cpu_logical_map(dev->cpu) : dev->cpu;
>>
>> IMO, it is up to the tegra_set_cpu_in_lp2 / tegra_clear_cpu_in_lp2
>> functions to do the cpu map conversion instead of adding this into a
>> routine working with logical cpu.
>>
> Good idea, I can create another patch to do that.
> 
>>> +	local_fiq_disable();
>>> +
>>> +	tegra_set_cpu_in_lp2(cpu);
>>> +	cpu_pm_enter();
>>> +
>>> +	clockevents_notify(CLOCK_EVT_NOTIFY_BROADCAST_ENTER, &dev->cpu);
>>> +	smp_wmb();
>>
>> Why do you need a memory barrier here ?
>>
> Yeah, Looks I don't have any static data that needs a barrier to sync
> data for SMP here. Will remove them.
> 
>>> +	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(cpu);
>>> +
>>> +	local_fiq_enable();
>>> +
>>> +	smp_rmb();
>>
>> Why do you need a memory barrier here ?
>>
>>> +	return index;
>>> +}
>>> +#endif
>>> +
>>>  int __init tegra114_cpuidle_init(void)
>>>  {
>>> +#ifdef CONFIG_PM_SLEEP
>>> +	tegra_tear_down_cpu = tegra30_tear_down_cpu;
>>> +#endif
>>
>> Doesn't this code should go in the pm.c file ?
>>
> Yes, I had a plan to do that too. Currently It had a timing issue about
> this. Because the CPU idle driver was registered by device_initcall, but
> the PM init function was registered at late init. I need to sync the
> init time first.
> OK, will do that too.
> 
> Thanks,
> Joseph
> 
> 


-- 
 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog