cpuidle fixes and cleanup

Pre/post platform specific cpuidle operations are handled by pm_notifier. But these operations are not same for all cpuidle states. Handle this by moving cpuidle specific code from pm_notifier to cpuidle specific function.

Signed-off-by Chander Kashyap <[email protected]>

---
 arch/arm/mach-exynos/common.h    |    2 +-
 arch/arm/mach-exynos/pm.c        |   45 ++++++++++----------------------------
 drivers/cpuidle/cpuidle-exynos.c |    7 ++++--
 3 files changed, 17 insertions(+), 37 deletions(-)

arch/arm/mach-exynos/common.h +1 -1

index 1ee9176..7769f58 100644
@@ -166,7 +166,7 @@ extern int  exynos_cpu_power_state(int cpu);
 extern void exynos_cluster_power_down(int cluster);
 extern void exynos_cluster_power_up(int cluster);
 extern int  exynos_cluster_power_state(int cluster);
-extern void exynos_enter_aftr(void);
+extern void exynos_enter_aftr(int entering_idle);
 
 extern void s5p_init_cpu(void __iomem *cpuid_addr);
 extern unsigned int samsung_rev(void);

arch/arm/mach-exynos/pm.c +1 -26

index a092cc3..328644f 100644
@@ -188,14 +188,6 @@ static void exynos_cpu_set_boot_vector(long flags)
 	__raw_writel(flags, EXYNOS_BOOT_VECTOR_FLAG);
 }
 
-void exynos_enter_aftr(void)
-{
-	exynos_set_wakeupmask(0x0000ff3e);
-	exynos_cpu_set_boot_vector(S5P_CHECK_AFTR);
-	/* Set value of power down register for aftr mode */
-	exynos_sys_powerdown_conf(SYS_AFTR);
-}
-
 static int exynos_cpu_suspend(unsigned long arg)
 {
 #ifdef CONFIG_CACHE_L2X0

@@ -386,40 +378,25 @@ static const struct platform_suspend_ops exynos_suspend_ops = {
 	.valid		= suspend_valid_only_mem,
 };
 
-static int exynos_cpu_pm_notifier(struct notifier_block *self,
-				  unsigned long cmd, void *v)
+void exynos_enter_aftr(int entering_idle)
 {
-	int cpu = smp_processor_id();
-
-	switch (cmd) {
-	case CPU_PM_ENTER:
-		if (cpu == 0)
-			exynos_pm_central_suspend();
-		break;
-
-	case CPU_PM_EXIT:
-		if (cpu == 0) {
-			if (read_cpuid_part_number() ==
-					ARM_CPU_PART_CORTEX_A9)
-				scu_enable(S5P_VA_SCU);
-			exynos_pm_central_resume();
-		}
-		break;

arch/arm/mach-exynos/pm.c +0 -8

 	}
-
-	return NOTIFY_OK;
 }
 
-static struct notifier_block exynos_cpu_pm_notifier_block = {
-	.notifier_call = exynos_cpu_pm_notifier,
-};
-
 void __init exynos_pm_init(void)
 {
 	u32 tmp;
 
-	cpu_pm_register_notifier(&exynos_cpu_pm_notifier_block);
-
 	/* Platform-specific GIC callback */
 	gic_arch_extn.irq_set_wake = exynos_irq_set_wake;
 

drivers/cpuidle/cpuidle-exynos.c +1 -2

index 7c01512..1196ca7 100644
@@ -18,11 +18,10 @@
 #include <asm/suspend.h>
 #include <asm/cpuidle.h>
 
-static void (*exynos_enter_aftr)(void);
+static void (*exynos_enter_aftr)(int);
 
 static int idle_finisher(unsigned long flags)
 {
-	exynos_enter_aftr();
 	cpu_do_idle();
 
 	return 1;

Hi Chander,

Please see my comments inline.

Quoted 67 lines · Chander Kashyap

Pre/post platform specific cpuidle operations are handled by pm_notifier. But these operations are not same for all cpuidle states. Handle this by moving cpuidle specific code from pm_notifier to cpuidle specific function.

Signed-off-by Chander Kashyap <[email protected]>

---
 arch/arm/mach-exynos/common.h    |    2 +-
 arch/arm/mach-exynos/pm.c        |   45 ++++++++++----------------------------
 drivers/cpuidle/cpuidle-exynos.c |    7 ++++--
 3 files changed, 17 insertions(+), 37 deletions(-)

arch/arm/mach-exynos/common.h +1 -1

index 1ee9176..7769f58 100644
@@ -166,7 +166,7 @@ extern int  exynos_cpu_power_state(int cpu);
 extern void exynos_cluster_power_down(int cluster);
 extern void exynos_cluster_power_up(int cluster);
 extern int  exynos_cluster_power_state(int cluster);
-extern void exynos_enter_aftr(void);
+extern void exynos_enter_aftr(int entering_idle);
 
 extern void s5p_init_cpu(void __iomem *cpuid_addr);
 extern unsigned int samsung_rev(void);

arch/arm/mach-exynos/pm.c +1 -26

index a092cc3..328644f 100644
@@ -188,14 +188,6 @@ static void exynos_cpu_set_boot_vector(long flags)
      __raw_writel(flags, EXYNOS_BOOT_VECTOR_FLAG);
 }
 
-void exynos_enter_aftr(void)
-{
-     exynos_set_wakeupmask(0x0000ff3e);
-     exynos_cpu_set_boot_vector(S5P_CHECK_AFTR);
-     /* Set value of power down register for aftr mode */
-     exynos_sys_powerdown_conf(SYS_AFTR);
-}
-
 static int exynos_cpu_suspend(unsigned long arg)
 {
 #ifdef CONFIG_CACHE_L2X0

@@ -386,40 +378,25 @@ static const struct platform_suspend_ops exynos_suspend_ops = {
      .valid          = suspend_valid_only_mem,
 };
 
-static int exynos_cpu_pm_notifier(struct notifier_block *self,
-                               unsigned long cmd, void *v)
+void exynos_enter_aftr(int entering_idle)
 {
-     int cpu = smp_processor_id();
-
-     switch (cmd) {
-     case CPU_PM_ENTER:
-             if (cpu == 0)
-                     exynos_pm_central_suspend();
-             break;
-
-     case CPU_PM_EXIT:
-             if (cpu == 0) {
-                     if (read_cpuid_part_number() ==
-                                     ARM_CPU_PART_CORTEX_A9)
-                             scu_enable(S5P_VA_SCU);
-                     exynos_pm_central_resume();
-             }
-             break;

arch/arm/mach-exynos/pm.c static const struct platform_suspend_ops exynos_suspend_ops = { · line 392

Quoted 10 lines

arch/arm/mach-exynos/pm.c +10 -0

+     if (entering_idle) {
+             exynos_set_wakeupmask(0x0000ff3e);
+             exynos_cpu_set_boot_vector(S5P_CHECK_AFTR);
+             /* Set value of power down register for aftr mode */
+             exynos_sys_powerdown_conf(SYS_AFTR);
+             exynos_pm_central_suspend();
+     } else {
+             if (scu_a9_has_base())
+                     scu_enable(S5P_VA_SCU);
+             exynos_pm_central_resume();

Hmm. This is not very readable. Basically you have two functions that do completely different things packed into one function. I would suggest moving the calls to cpu_pm_enter/exit() and everything in between to this function then you wouldn't need anything like this and the whole low level logic would be in one place.

Quoted 36 lines

arch/arm/mach-exynos/pm.c +0 -8

      }
-
-     return NOTIFY_OK;
 }
 
-static struct notifier_block exynos_cpu_pm_notifier_block = {
-     .notifier_call = exynos_cpu_pm_notifier,
-};
-
 void __init exynos_pm_init(void)
 {
      u32 tmp;
 
-     cpu_pm_register_notifier(&exynos_cpu_pm_notifier_block);
-
      /* Platform-specific GIC callback */
      gic_arch_extn.irq_set_wake = exynos_irq_set_wake;
 

drivers/cpuidle/cpuidle-exynos.c +1 -2

index 7c01512..1196ca7 100644
@@ -18,11 +18,10 @@
 #include <asm/suspend.h>
 #include <asm/cpuidle.h>
 
-static void (*exynos_enter_aftr)(void);
+static void (*exynos_enter_aftr)(int);
 
 static int idle_finisher(unsigned long flags)
 {
-     exynos_enter_aftr();
      cpu_do_idle();
 
      return 1;

drivers/cpuidle/cpuidle-exynos.c static int exynos_enter_core0_aftr(struct cpuidle_device *dev, · line 39

Quoted 9 lines

drivers/cpuidle/cpuidle-exynos.c +4 -0

@@ -32,8 +31,12 @@ static int exynos_enter_core0_aftr(struct cpuidle_device *dev,
                              struct cpuidle_driver *drv,
                              int index)
 {
+     int entering_idle = true;
      cpu_pm_enter();
+     exynos_enter_aftr(entering_idle);
      cpu_suspend(0, idle_finisher);
+     entering_idle = false;
+     exynos_enter_aftr(entering_idle);

This doesn't look good. Couldn't you just have called it with constant arguments? E.g.

        exynos_enter_aftr(true);
        [...]
        exynos_enter_aftr(false);

Well, sorry for late comments, I have missed this series, probably because I'm not on Cc list. Anyway, since this patch will need to be respun anyway, maybe it would be better to use the one I just posted today, which IMHO is a bit cleaner.

Best regards,
Tomasz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

This patch series fixes the cpuidle for different states. Also removes arm diagnostic and power register save/restore code as it is made generic.

Based on:
ARM: save/restore diagnostic register on Cortex-A9 suspend/resume
http://www.spinics.net/lists/arm-kernel/msg340506.html

Quoted 6 lines · Chander Kashyap

This patch series fixes the cpuidle for different states. Also removes arm diagnostic and power register save/restore code as it is made generic.

Based on:
ARM: save/restore diagnostic register on Cortex-A9 suspend/resume
http://www.spinics.net/lists/arm-kernel/msg340506.html

As there is an outstanding issue with this patch, we can't proceed with this set of changes until we know what's going on there.

Also, bear in mind that I have changes which conflict with this in my tree (which I've just updated.)

--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Quoted 7 lines · Russell King - ARM Linux

Quoted 6 lines · Chander Kashyap

This patch series fixes the cpuidle for different states. Also removes arm diagnostic and power register save/restore code as it is made generic.

Based on:
ARM: save/restore diagnostic register on Cortex-A9 suspend/resume
http://www.spinics.net/lists/arm-kernel/msg340506.html

Quoted 3 lines

As there is an outstanding issue with this patch, we can't proceed with this set of changes until we know what's going on there.

Sure, I will wait for the conclusion.

Quoted 9 lines · Chander Kashyap

Quoted 7 lines · Russell King - ARM Linux

Quoted 6 lines · Chander Kashyap

This patch series fixes the cpuidle for different states. Also removes arm diagnostic and power register save/restore code as it is made generic.

Based on:
ARM: save/restore diagnostic register on Cortex-A9 suspend/resume
http://www.spinics.net/lists/arm-kernel/msg340506.html

[Ccing people who participated in discussion in that thread]

Quoted 1 line

Quoted 1 line

Quoted 4 lines

Quoted 2 lines

As there is an outstanding issue with this patch, we can't proceed with this set of changes until we know what's going on there.

Sure, I will wait for the conclusion.

So I believe the conclusion was that this can't be handled in generic way, because on systems running in non-secure mode writing to those registers faults.

Quoted 14 lines · Tomasz Figa

Quoted 9 lines · Chander Kashyap

Quoted 7 lines · Russell King - ARM Linux

Quoted 6 lines · Chander Kashyap

This patch series fixes the cpuidle for different states. Also removes arm diagnostic and power register save/restore code as it is made generic.

Based on:
ARM: save/restore diagnostic register on Cortex-A9 suspend/resume
http://www.spinics.net/lists/arm-kernel/msg340506.html

[Ccing people who participated in discussion in that thread]

Quoted 1 line

Quoted 1 line

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

As there is an outstanding issue with this patch, we can't proceed with this set of changes until we know what's going on there.

Sure, I will wait for the conclusion.

So I believe the conclusion was that this can't be handled in generic way, because on systems running in non-secure mode writing to those registers faults.

That is, unfortunately, correct.

There is a way around this, but people aren't going to like it. That is - we move it to the suspend and resume paths anyway.

In the resume path, we read the register, compare it with the value which we would update it to, and if it's identical, we avoid the write.

This gets secure-mode platforms working.

For non-secure mode platforms, we have to require them to insert some assembly code into the early resume path which calls their secure monitor to restore these registers before continuing on to cpu_resume, thereby ensuring that the CPU specific code sees that the value in the register is identical with the saved value, and omitting the write.

This isn't really a new principle - we already have this requirement for non-secure mode platforms when they're booting a kernel with various errata enabled.

I can't see any other alternative which satisfies everyone (by everyone, I'm including the requirements from the hardware folk who expect these registers to be static once the MMU is enabled.)

--
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly improving, and getting towards what was expected from it.

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo at vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html