Patchwork [5/5] ARM: tegra20: cpuidle: apply coupled cpuidle for powered-down mode

login
register
mail settings
Submitter Joseph Lo
Date Dec. 3, 2012, 3 a.m.
Message ID <1354503607-13707-6-git-send-email-josephl@nvidia.com>
Download mbox | patch
Permalink /patch/203268/
State Superseded, archived
Headers show

Comments

Joseph Lo - Dec. 3, 2012, 3 a.m.
The "powered-down" cpuidle mode of Tegra20 needs the CPU0 be the last one
core to go into this mode before other core. The coupled cpuidle framework
can help to sync the MPCore to coupled state then go into "powered-down"
idle mode together. The driver can just assume the MPCore come into
"powered-down" mode at the same time. No need to take care if the CPU_0
goes into this mode along and only can put it into safe idle mode (WFI).

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
 arch/arm/mach-tegra/Kconfig           |    1 +
 arch/arm/mach-tegra/cpuidle-tegra20.c |   37 +++++++++++++++++----------------
 2 files changed, 20 insertions(+), 18 deletions(-)
Stephen Warren - Dec. 3, 2012, 6:52 p.m.
On 12/02/2012 08:00 PM, Joseph Lo wrote:
> The "powered-down" cpuidle mode of Tegra20 needs the CPU0 be the last one
> core to go into this mode before other core. The coupled cpuidle framework
> can help to sync the MPCore to coupled state then go into "powered-down"
> idle mode together. The driver can just assume the MPCore come into
> "powered-down" mode at the same time. No need to take care if the CPU_0
> goes into this mode along and only can put it into safe idle mode (WFI).

I wonder if it wouldn't be simpler to squash this patch into one of the
earlier patches, and just use coupled cpuidle from the very start?

> +static int __cpuinit tegra20_idle_lp2_coupled(struct cpuidle_device *dev,
> +					      struct cpuidle_driver *drv,
> +					      int index)

> -	if (cpu == 0) {
> -		if (last_cpu)
> -			entered_lp2 = tegra20_cpu_cluster_power_down(dev, drv,
> -								     index);
> -		else
> -			cpu_do_idle();
> -	} else {
> +	if (cpu == 0)
> +		entered_lp2 = tegra20_cpu_cluster_power_down(dev, drv, index);
> +	else
>  		entered_lp2 = tegra20_idle_enter_lp2_cpu_1(dev, drv, index);
> -	}

So I assume that coupled cpuidle only calls this function on CPU 0 once
it has guaranteed that CPU n are all in this same idle state. What does
CPU 0 do now, when it wants to enter LP2 but can't because CPU n aren't
in LP2? Do we need to explicitly provide some kind of function to
implement this waiting state, or does coupled cpuidle ensure the LP3 is
entered, or implement WFI itself, or ...?

> @@ -258,6 +256,9 @@ int __init tegra20_cpuidle_init(void)

> +#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
> +		dev->coupled_cpus = *cpu_online_mask;
> +#endif

What if that changes; I assume couple cpuidle handles that by
registering a notifier?

Is there any way that the kernel can boot with only CPU 0 "plugged in",
but later have the other CPU hotplugged in? In other words, should that
hard-code a mask (3?) that describes the HW, rather than relying on the
runtime hotplug status? (think about what happens when this idle code is
moved into drivers/cpuidle/ and built as a module, and hence could be
initialized with only 1 CPU hotplugged in).
--
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
Joseph Lo - Dec. 4, 2012, 10:20 a.m.
On Tue, 2012-12-04 at 02:52 +0800, Stephen Warren wrote:
> On 12/02/2012 08:00 PM, Joseph Lo wrote:
> > The "powered-down" cpuidle mode of Tegra20 needs the CPU0 be the last one
> > core to go into this mode before other core. The coupled cpuidle framework
> > can help to sync the MPCore to coupled state then go into "powered-down"
> > idle mode together. The driver can just assume the MPCore come into
> > "powered-down" mode at the same time. No need to take care if the CPU_0
> > goes into this mode along and only can put it into safe idle mode (WFI).
> 
> I wonder if it wouldn't be simpler to squash this patch into one of the
> earlier patches, and just use coupled cpuidle from the very start?
> 
OK. I can do this. And I want to add one more patch that can cover the
IPI miss handling issue in coupled cpuidle framework. I will prepare
that for V2. And it means that after V2, you don't need to consider the
fix that I purposed for coupled cpuidle framework. You can merge it if
it pass your review.

I won't squash this in V2 for you to check what difference. If you think
that is ok, I will squash this in V3.


> > +static int __cpuinit tegra20_idle_lp2_coupled(struct cpuidle_device *dev,
> > +					      struct cpuidle_driver *drv,
> > +					      int index)
> 
> > -	if (cpu == 0) {
> > -		if (last_cpu)
> > -			entered_lp2 = tegra20_cpu_cluster_power_down(dev, drv,
> > -								     index);
> > -		else
> > -			cpu_do_idle();
> > -	} else {
> > +	if (cpu == 0)
> > +		entered_lp2 = tegra20_cpu_cluster_power_down(dev, drv, index);
> > +	else
> >  		entered_lp2 = tegra20_idle_enter_lp2_cpu_1(dev, drv, index);
> > -	}
> 
> So I assume that coupled cpuidle only calls this function on CPU 0 once
> it has guaranteed that CPU n are all in this same idle state. What does
> CPU 0 do now, when it wants to enter LP2 but can't because CPU n aren't
> in LP2? Do we need to explicitly provide some kind of function to
> implement this waiting state, or does coupled cpuidle ensure the LP3 is
> entered, or implement WFI itself, or ...?
> 
Indeed, this is important for CPU0. For Tegra30, we definitely need a
loop to sync CPUn to power down status first. For Tegra20, the CPU0 only
need to put CPU1 in reset. And we handle these procedure when CPU0 going
to LP2.

> > @@ -258,6 +256,9 @@ int __init tegra20_cpuidle_init(void)
> 
> > +#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
> > +		dev->coupled_cpus = *cpu_online_mask;
> > +#endif
> 
> What if that changes; I assume couple cpuidle handles that by
> registering a notifier?
> 
Yes. This was used for the coupled driver initialization. It need to
know how many CPUs need to be coupled when registration. It also use the
notifier for updating the mask.

> Is there any way that the kernel can boot with only CPU 0 "plugged in",
> but later have the other CPU hotplugged in? In other words, should that
> hard-code a mask (3?) that describes the HW, rather than relying on the
> runtime hotplug status? (think about what happens when this idle code is
> moved into drivers/cpuidle/ and built as a module, and hence could be
> initialized with only 1 CPU hotplugged in).
Hmmm. Currently it can't get these info from HW. The procedure is just
like above. Syncing the cpu online mask to know how many CPUs need to be
coupled and updating the mask by notifier.

Thanks,
Joseph

--
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
Peter De Schrijver - Dec. 4, 2012, 12:41 p.m.
> 
> > @@ -258,6 +256,9 @@ int __init tegra20_cpuidle_init(void)
> 
> > +#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
> > +		dev->coupled_cpus = *cpu_online_mask;
> > +#endif
> 
> What if that changes; I assume couple cpuidle handles that by
> registering a notifier?
> 

Yes. see drivers/cpuidle/coupled.c:651

> Is there any way that the kernel can boot with only CPU 0 "plugged in",
> but later have the other CPU hotplugged in? In other words, should that
> hard-code a mask (3?) that describes the HW, rather than relying on the
> runtime hotplug status? (think about what happens when this idle code is
> moved into drivers/cpuidle/ and built as a module, and hence could be
> initialized with only 1 CPU hotplugged in).

Using the cpu_possible_mask might be a better idea. The only other user
of coupled cpuidle (OMAP4) also uses cpu_online_mask however.

Cheers,

Peter.
--
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

Patch

diff --git a/arch/arm/mach-tegra/Kconfig b/arch/arm/mach-tegra/Kconfig
index e426d1b..e07241a 100644
--- a/arch/arm/mach-tegra/Kconfig
+++ b/arch/arm/mach-tegra/Kconfig
@@ -4,6 +4,7 @@  comment "NVIDIA Tegra options"
 
 config ARCH_TEGRA_2x_SOC
 	bool "Enable support for Tegra20 family"
+	select ARCH_NEEDS_CPU_IDLE_COUPLED
 	select ARCH_REQUIRE_GPIOLIB
 	select ARM_ERRATA_720789
 	select ARM_ERRATA_742230
diff --git a/arch/arm/mach-tegra/cpuidle-tegra20.c b/arch/arm/mach-tegra/cpuidle-tegra20.c
index 9371a00..02b2cf7 100644
--- a/arch/arm/mach-tegra/cpuidle-tegra20.c
+++ b/arch/arm/mach-tegra/cpuidle-tegra20.c
@@ -37,9 +37,10 @@ 
 #include "flowctrl.h"
 
 #ifdef CONFIG_PM_SLEEP
-static int tegra20_idle_lp2(struct cpuidle_device *dev,
-			    struct cpuidle_driver *drv,
-			    int index);
+static atomic_t abort_barrier;
+static int tegra20_idle_lp2_coupled(struct cpuidle_device *dev,
+				    struct cpuidle_driver *drv,
+				    int index);
 #endif
 
 static struct cpuidle_driver tegra_idle_driver = {
@@ -55,11 +56,12 @@  static struct cpuidle_driver tegra_idle_driver = {
 		[0] = ARM_CPUIDLE_WFI_STATE_PWR(600),
 #ifdef CONFIG_PM_SLEEP
 		[1] = {
-			.enter			= tegra20_idle_lp2,
+			.enter			= tegra20_idle_lp2_coupled,
 			.exit_latency		= 5000,
 			.target_residency	= 10000,
 			.power_usage		= 0,
-			.flags			= CPUIDLE_FLAG_TIME_VALID,
+			.flags			= CPUIDLE_FLAG_TIME_VALID |
+						  CPUIDLE_FLAG_COUPLED,
 			.name			= "powered-down",
 			.desc			= "CPU power gated",
 		},
@@ -204,28 +206,24 @@  static inline bool tegra20_idle_enter_lp2_cpu_1(struct cpuidle_device *dev,
 }
 #endif
 
-static int __cpuinit tegra20_idle_lp2(struct cpuidle_device *dev,
-				      struct cpuidle_driver *drv,
-				      int index)
+static int __cpuinit tegra20_idle_lp2_coupled(struct cpuidle_device *dev,
+					      struct cpuidle_driver *drv,
+					      int index)
 {
 	u32 cpu = is_smp() ? cpu_logical_map(dev->cpu) : dev->cpu;
 	bool entered_lp2 = false;
-	bool last_cpu;
+
+	cpuidle_coupled_parallel_barrier(dev, &abort_barrier);
 
 	local_fiq_disable();
 
-	last_cpu =  tegra_set_cpu_in_lp2(cpu);
+	tegra_set_cpu_in_lp2(cpu);
 	cpu_pm_enter();
 
-	if (cpu == 0) {
-		if (last_cpu)
-			entered_lp2 = tegra20_cpu_cluster_power_down(dev, drv,
-								     index);
-		else
-			cpu_do_idle();
-	} else {
+	if (cpu == 0)
+		entered_lp2 = tegra20_cpu_cluster_power_down(dev, drv, index);
+	else
 		entered_lp2 = tegra20_idle_enter_lp2_cpu_1(dev, drv, index);
-	}
 
 	cpu_pm_exit();
 	tegra_clear_cpu_in_lp2(cpu);
@@ -258,6 +256,9 @@  int __init tegra20_cpuidle_init(void)
 	for_each_possible_cpu(cpu) {
 		dev = &per_cpu(tegra_idle_device, cpu);
 		dev->cpu = cpu;
+#ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
+		dev->coupled_cpus = *cpu_online_mask;
+#endif
 
 		dev->state_count = drv->state_count;
 		ret = cpuidle_register_device(dev);