ARM: tegra20: cpuidle: apply coupled cpuidle for powered-down mode

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

The powered-down state of Tegra20 requires power gating both CPU cores. When the secondary CPU requests to enter powered-down state, it saves its own contexts and then enters WFI for waiting CPU0 in the same state. When the CPU0 requests powered-down state, it attempts to put the secondary CPU into reset to prevent it from waking up. Then power down both CPUs together and power off the cpu rail.

Be aware of that, you may see the legacy power state "LP2" in the code which is exactly the same meaning of "CPU power down".

Based on the work by:
Colin Cross <[email protected]>
Gary King <[email protected]>

Signed-off-by Joseph Lo <[email protected]>

---
V4:
* rename the function to "tegra20_wake_cpu1_from_reset"
* make the coupled cpuidle can be disabled if SMP is disabled V3:
* sqash last two patch in previous version to support coupled cpuidle directly
V2:
* refine the cpu control function that dedicate for CPU_1
* rename "tegra_cpu_pllp" to "tegra_switch_cpu_to_pllp"
---

arch/arm/mach-tegra/cpuidle-tegra20.c +0 -0

index 50f984d..63ab9c2 100644

arch/arm/mach-tegra/cpuidle-tegra20.c +0 -2

@@ -87,20 +176,31 @@ static inline bool tegra20_idle_enter_lp2_cpu_1(struct cpuidle_device *dev,
 }
 #endif
 
-static int tegra20_idle_lp2(struct cpuidle_device *dev,
-                           struct cpuidle_driver *drv,

arch/arm/mach-tegra/cpuidle-tegra20.c +14 -1

+
+       cpuidle_coupled_parallel_barrier(dev, &abort_barrier);
+
+       if (atomic_read(&abort_flag) > 0) {
+               cpuidle_coupled_parallel_barrier(dev, &abort_barrier);
+               atomic_set(&abort_flag, 0);     /* clean flag for next coming */
+               return -EINTR;
+       }
+
        local_fiq_disable();
 
        tegra_set_cpu_in_lp2(cpu);
        cpu_pm_enter();
 
        if (cpu == 0)
-               cpu_do_idle();
+               entered_lp2 = tegra20_cpu_cluster_power_down(dev, drv, index);
        else
                entered_lp2 = tegra20_idle_enter_lp2_cpu_1(dev, drv, index);
 

@@ -122,6 +222,10 @@ int __init tegra20_cpuidle_init(void)
        struct cpuidle_device *dev;
        struct cpuidle_driver *drv = &tegra_idle_driver;
 
+#ifdef CONFIG_PM_SLEEP
+       tegra_tear_down_cpu = tegra20_tear_down_cpu;
+#endif
+
        drv->state_count = ARRAY_SIZE(tegra_idle_states);
        memcpy(drv->states, tegra_idle_states,
                        drv->state_count * sizeof(drv->states[0]));

arch/arm/mach-tegra/cpuidle-tegra20.c +1 -0

+#endif
 
                dev->state_count = drv->state_count;
                ret = cpuidle_register_device(dev);

Quoted 7 lines · Joseph Lo

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

Quoted 8 lines

The powered-down state of Tegra20 requires power gating both CPU cores. When the secondary CPU requests to enter powered-down state, it saves its own contexts and then enters WFI for waiting CPU0 in the same state. When the CPU0 requests powered-down state, it attempts to put the secondary CPU into reset to prevent it from waking up. Then power down both CPUs together and power off the cpu rail.

Be aware of that, you may see the legacy power state "LP2" in the code

Quoted 15 lines · Colin Cross

Quoted 14 lines · Joseph Lo

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

The powered-down state of Tegra20 requires power gating both CPU cores. When the secondary CPU requests to enter powered-down state, it saves its own contexts and then enters WFI for waiting CPU0 in the same state. When the CPU0 requests powered-down state, it attempts to put the secondary CPU into reset to prevent it from waking up. Then power down both CPUs together and power off the cpu rail.

Quoted 1 line

Quoted 1 line

Be aware of that, you may see the legacy power state "LP2" in the code

Colin, since you only raised a few small issues on this series, does that mean you're OK with it once those issues are fixed?

Joseph, we'll be merging Tegra114 in the near future. How will this patch series affect Tegra114? Will the cpuidle driver simply fail to register on Tegra114 (which would be fine until we explicitly add support), or would we need to disable cpuidle in Kconfig to get a working Tegra114 kernel. Does this patch series affect the answer to the previous question? Thanks.

Quoted 8 lines · Colin Cross

Quoted 7 lines · Joseph Lo

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

Quoted 8 lines

Quoted 8 lines

The powered-down state of Tegra20 requires power gating both CPU cores. When the secondary CPU requests to enter powered-down state, it saves its own contexts and then enters WFI for waiting CPU0 in the same state. When the CPU0 requests powered-down state, it attempts to put the secondary CPU into reset to prevent it from waking up. Then power down both CPUs together and power off the cpu rail.

Be aware of that, you may see the legacy power state "LP2" in the code

Colin, since you only raised a few small issues on this series, does that mean you're OK with it once those issues are fixed?

Joseph, we'll be merging Tegra114 in the near future. How will this patch series affect Tegra114? Will the cpuidle driver simply fail to register on Tegra114 (which would be fine until we explicitly add support), or would we need to disable cpuidle in Kconfig to get a working Tegra114 kernel. Does this patch series affect the answer to the previous question? Thanks.

Quoted 21 lines · Stephen Warren

Quoted 8 lines · Colin Cross

Quoted 7 lines · Joseph Lo

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

Quoted 8 lines

Quoted 8 lines

The powered-down state of Tegra20 requires power gating both CPU cores. When the secondary CPU requests to enter powered-down state, it saves its own contexts and then enters WFI for waiting CPU0 in the same state. When the CPU0 requests powered-down state, it attempts to put the secondary CPU into reset to prevent it from waking up. Then power down both CPUs together and power off the cpu rail.

Be aware of that, you may see the legacy power state "LP2" in the code

Colin, since you only raised a few small issues on this series, does that mean you're OK with it once those issues are fixed?

Quoted 7 lines

Joseph, we'll be merging Tegra114 in the near future. How will this patch series affect Tegra114? Will the cpuidle driver simply fail to register on Tegra114 (which would be fine until we explicitly add support), or would we need to disable cpuidle in Kconfig to get a working Tegra114 kernel. Does this patch series affect the answer to the previous question? Thanks.

Stephen,

This patch series won't affect Tegra114. I think I need to add a simple cpuidle driver for Tegra114 to support just WFI state first.

And I am going do some re-works about CPUs and cluster power status sync functions that be introduced recently[1] for CPU hotplug, idle and secondary booting in Tegra tree. I hope I can hide all the power related function behind that and make the CPU idle driver be more generic for all Tegra series.

Before that re-works, I will still keep upstream the platform suspend function for Tegra.
(If I am not in the right direction, please pull me back.)

Quoted 33 lines · Joseph Lo

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

The powered-down state of Tegra20 requires power gating both CPU cores. When the secondary CPU requests to enter powered-down state, it saves its own contexts and then enters WFI for waiting CPU0 in the same state. When the CPU0 requests powered-down state, it attempts to put the secondary CPU into reset to prevent it from waking up. Then power down both CPUs together and power off the cpu rail.

Be aware of that, you may see the legacy power state "LP2" in the code which is exactly the same meaning of "CPU power down".

Based on the work by:
Colin Cross <[email protected]>
Gary King <[email protected]>

Signed-off-by Joseph Lo <[email protected]>

---
V4:
* rename the function to "tegra20_wake_cpu1_from_reset"
* make the coupled cpuidle can be disabled if SMP is disabled V3:
* sqash last two patch in previous version to support coupled cpuidle directly
V2:
* refine the cpu control function that dedicate for CPU_1
* rename "tegra_cpu_pllp" to "tegra_switch_cpu_to_pllp"
---

<snip>

Quoted 4 lines

arch/arm/mach-tegra/cpuidle-tegra20.c +0 -0

index 50f984d..63ab9c2 100644

<snip>

Quoted 14 lines

arch/arm/mach-tegra/cpuidle-tegra20.c +3 -3

@@ -87,20 +176,31 @@ static inline bool tegra20_idle_enter_lp2_cpu_1(struct cpuidle_device *dev,
 }
 #endif
 
-static int tegra20_idle_lp2(struct cpuidle_device *dev,
-                           struct cpuidle_driver *drv,
-                           int index)
+static int 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;
 

arch/arm/mach-tegra/cpuidle-tegra20.c static inline bool tegra20_idle_enter_lp2_cpu_1(struct cpuidle_device *dev, · line 187

Quoted 2 lines

arch/arm/mach-tegra/cpuidle-tegra20.c +2 -0

+       if (tegra_pending_sgi())
+               atomic_inc(&abort_flag);

Minor nit, this doesn't need to be atomic. You could just use abort_flag = true, or ACCESS_ONCE(abort_flag) = true. Each cpu will either not touch this variable or write 1 to it, so there is no read/modify/write race.

Quoted 34 lines

arch/arm/mach-tegra/cpuidle-tegra20.c +14 -1

+
+       cpuidle_coupled_parallel_barrier(dev, &abort_barrier);
+
+       if (atomic_read(&abort_flag) > 0) {
+               cpuidle_coupled_parallel_barrier(dev, &abort_barrier);
+               atomic_set(&abort_flag, 0);     /* clean flag for next coming */
+               return -EINTR;
+       }
+
        local_fiq_disable();
 
        tegra_set_cpu_in_lp2(cpu);
        cpu_pm_enter();
 
        if (cpu == 0)
-               cpu_do_idle();
+               entered_lp2 = tegra20_cpu_cluster_power_down(dev, drv, index);
        else
                entered_lp2 = tegra20_idle_enter_lp2_cpu_1(dev, drv, index);
 

@@ -122,6 +222,10 @@ int __init tegra20_cpuidle_init(void)
        struct cpuidle_device *dev;
        struct cpuidle_driver *drv = &tegra_idle_driver;
 
+#ifdef CONFIG_PM_SLEEP
+       tegra_tear_down_cpu = tegra20_tear_down_cpu;
+#endif
+
        drv->state_count = ARRAY_SIZE(tegra_idle_states);
        memcpy(drv->states, tegra_idle_states,
                        drv->state_count * sizeof(drv->states[0]));

@@ -135,6 +239,9 @@ int __init tegra20_cpuidle_init(void)
        for_each_possible_cpu(cpu) {
                dev = &per_cpu(tegra_idle_device, cpu);

arch/arm/mach-tegra/cpuidle-tegra20.c int __init tegra20_cpuidle_init(void) · line 243

Quoted 3 lines

arch/arm/mach-tegra/cpuidle-tegra20.c +2 -0

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

I think this should be cpu_possible_mask instead of cpu_online_mask. coupled.c already makes sure that only online cpus are considered by the synchronization code. If cpuidle were compiled as a module and loaded after offlining a cpu, you would hit the WARN_ON in cpuidle_coupled_register_device when onlining a cpu.

Quoted 4 lines

arch/arm/mach-tegra/cpuidle-tegra20.c +1 -0

+#endif
 
                dev->state_count = drv->state_count;
                ret = cpuidle_register_device(dev);

Quoted 58 lines · Colin Cross

Quoted 33 lines · Joseph Lo

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

The powered-down state of Tegra20 requires power gating both CPU cores. When the secondary CPU requests to enter powered-down state, it saves its own contexts and then enters WFI for waiting CPU0 in the same state. When the CPU0 requests powered-down state, it attempts to put the secondary CPU into reset to prevent it from waking up. Then power down both CPUs together and power off the cpu rail.

Be aware of that, you may see the legacy power state "LP2" in the code which is exactly the same meaning of "CPU power down".

Based on the work by:
Colin Cross <[email protected]>
Gary King <[email protected]>

Signed-off-by Joseph Lo <[email protected]>

---
V4:
* rename the function to "tegra20_wake_cpu1_from_reset"
* make the coupled cpuidle can be disabled if SMP is disabled V3:
* sqash last two patch in previous version to support coupled cpuidle directly
V2:
* refine the cpu control function that dedicate for CPU_1
* rename "tegra_cpu_pllp" to "tegra_switch_cpu_to_pllp"
---

<snip>

Quoted 4 lines

arch/arm/mach-tegra/cpuidle-tegra20.c +0 -0

index 50f984d..63ab9c2 100644

<snip>

Quoted 14 lines

arch/arm/mach-tegra/cpuidle-tegra20.c +3 -3

@@ -87,20 +176,31 @@ static inline bool tegra20_idle_enter_lp2_cpu_1(struct cpuidle_device *dev,
 }
 #endif
 
-static int tegra20_idle_lp2(struct cpuidle_device *dev,
-                           struct cpuidle_driver *drv,
-                           int index)
+static int 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;
 

Quoted 7 lines

arch/arm/mach-tegra/cpuidle-tegra20.c static inline bool tegra20_idle_enter_lp2_cpu_1(struct cpuidle_device *dev, · line 187

Quoted 2 lines

arch/arm/mach-tegra/cpuidle-tegra20.c +2 -0

+       if (tegra_pending_sgi())
+               atomic_inc(&abort_flag);

Minor nit, this doesn't need to be atomic. You could just use abort_flag = true, or ACCESS_ONCE(abort_flag) = true. Each cpu will either not touch this variable or write 1 to it, so there is no read/modify/write race.

Thanks for your remind. There is a reason I don't use a boolean flag here. The SGI register was per CPU register. Different CPU may get different content of the register depend on there is a SGI pending or not. That's why I don't use a boolean flag that may be overwritten by the other CPU here.

So I think I can just modify as follows and remove atomic operation.

        if (tegra_pending_sgi())
                abort_flag++;

        if (abort_flag > 0)
                abort coupled cpuidle;

Quoted 72 lines · Joseph Lo

Quoted 58 lines · Colin Cross

Quoted 33 lines · Joseph Lo

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

The powered-down state of Tegra20 requires power gating both CPU cores. When the secondary CPU requests to enter powered-down state, it saves its own contexts and then enters WFI for waiting CPU0 in the same state. When the CPU0 requests powered-down state, it attempts to put the secondary CPU into reset to prevent it from waking up. Then power down both CPUs together and power off the cpu rail.

Be aware of that, you may see the legacy power state "LP2" in the code which is exactly the same meaning of "CPU power down".

Based on the work by:
Colin Cross <[email protected]>
Gary King <[email protected]>

Signed-off-by Joseph Lo <[email protected]>

---
V4:
* rename the function to "tegra20_wake_cpu1_from_reset"
* make the coupled cpuidle can be disabled if SMP is disabled V3:
* sqash last two patch in previous version to support coupled cpuidle directly
V2:
* refine the cpu control function that dedicate for CPU_1
* rename "tegra_cpu_pllp" to "tegra_switch_cpu_to_pllp"
---

<snip>

Quoted 4 lines

arch/arm/mach-tegra/cpuidle-tegra20.c +0 -0

index 50f984d..63ab9c2 100644

<snip>

Quoted 14 lines

arch/arm/mach-tegra/cpuidle-tegra20.c +3 -3

@@ -87,20 +176,31 @@ static inline bool tegra20_idle_enter_lp2_cpu_1(struct cpuidle_device *dev,
 }
 #endif
 
-static int tegra20_idle_lp2(struct cpuidle_device *dev,
-                           struct cpuidle_driver *drv,
-                           int index)
+static int 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;
 

Quoted 7 lines

arch/arm/mach-tegra/cpuidle-tegra20.c static inline bool tegra20_idle_enter_lp2_cpu_1(struct cpuidle_device *dev, · line 187

Quoted 2 lines

arch/arm/mach-tegra/cpuidle-tegra20.c +2 -0

+       if (tegra_pending_sgi())
+               atomic_inc(&abort_flag);

Minor nit, this doesn't need to be atomic. You could just use abort_flag = true, or ACCESS_ONCE(abort_flag) = true. Each cpu will either not touch this variable or write 1 to it, so there is no read/modify/write race.

Thanks for your remind. There is a reason I don't use a boolean flag here. The SGI register was per CPU register. Different CPU may get different content of the register depend on there is a SGI pending or not. That's why I don't use a boolean flag that may be overwritten by the other CPU here.

Quoted 7 lines

So I think I can just modify as follows and remove atomic operation.

        if (tegra_pending_sgi())
                abort_flag++;

        if (abort_flag > 0)
                abort coupled cpuidle;

abort_flag++ is a bad idea, it will do a read-modify-write on the variable, and two concurrent read-modify-writes will not result in the correct value.

In this case you don't care about counting how many cpus have sgis, you just care if any cpu does. ACCESS_ONCE(abort_flag) = true will work fine, because cpus that do not have a pending sgi will not set the variable to false.