[PATCH 0/2] cpuidle fixes and cleanup

[PATCH 0/2] cpuidle fixes and cleanup

[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

ARM: save/restore power control register on Cortex-A9 suspend/resume
 http://www.spinics.net/lists/arm-kernel/msg343320.html

Chander Kashyap (2):
  ARM: Exynos: remove arm diagnostic and power register save/restore
    code
  cpuidle: Exynos: fix cpuidle for all states

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

-- 
1.7.9.5

[PATCH 0/2] cpuidle fixes and cleanup

[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

ARM: save/restore power control register on Cortex-A9 suspend/resume
 http://www.spinics.net/lists/arm-kernel/msg343320.html

Chander Kashyap (2):
  ARM: Exynos: remove arm diagnostic and power register save/restore
    code
  cpuidle: Exynos: fix cpuidle for all states

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

-- 
1.7.9.5

[PATCH 1/2] ARM: Exynos: remove arm diagnostic and power register save/restore code

[Chander Kashyap]

As save/restore of arm "diagnostic" and "power" registers is handled by
generic code, so remove the same.

Signed-off-by: Chander Kashyap <k.chander@samsung.com>
---
 arch/arm/mach-exynos/pm.c |   54 ++-------------------------------------------
 1 file changed, 2 insertions(+), 52 deletions(-)

diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
index 202ca73..a092cc3 100644
--- a/arch/arm/mach-exynos/pm.c
+++ b/arch/arm/mach-exynos/pm.c
@@ -196,45 +196,6 @@ void exynos_enter_aftr(void)
 	exynos_sys_powerdown_conf(SYS_AFTR);
 }
 
-/* For Cortex-A9 Diagnostic and Power control register */
-static unsigned int save_arm_register[2];
-
-static void exynos_cpu_save_register(void)
-{
-	unsigned long tmp;
-
-	/* Save Power control register */
-	asm ("mrc p15, 0, %0, c15, c0, 0"
-	     : "=r" (tmp) : : "cc");
-
-	save_arm_register[0] = tmp;
-
-	/* Save Diagnostic register */
-	asm ("mrc p15, 0, %0, c15, c0, 1"
-	     : "=r" (tmp) : : "cc");
-
-	save_arm_register[1] = tmp;
-}
-
-static void exynos_cpu_restore_register(void)
-{
-	unsigned long tmp;
-
-	/* Restore Power control register */
-	tmp = save_arm_register[0];
-
-	asm volatile ("mcr p15, 0, %0, c15, c0, 0"
-		      : : "r" (tmp)
-		      : "cc");
-
-	/* Restore Diagnostic register */
-	tmp = save_arm_register[1];
-
-	asm volatile ("mcr p15, 0, %0, c15, c0, 1"
-		      : : "r" (tmp)
-		      : "cc");
-}
-
 static int exynos_cpu_suspend(unsigned long arg)
 {
 #ifdef CONFIG_CACHE_L2X0
@@ -300,9 +261,6 @@ static int exynos_pm_suspend(void)
 	tmp = (S5P_USE_STANDBY_WFI0 | S5P_USE_STANDBY_WFE0);
 	__raw_writel(tmp, S5P_CENTRAL_SEQ_OPTION);
 
-	if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9)
-		exynos_cpu_save_register();
-
 	return 0;
 }
 
@@ -334,9 +292,6 @@ static void exynos_pm_resume(void)
 	if (exynos_pm_central_resume())
 		goto early_wakeup;
 
-	if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9)
-		exynos_cpu_restore_register();
-
 	/* For release retention */
 
 	__raw_writel((1 << 28), S5P_PAD_RET_MAUDIO_OPTION);
@@ -438,20 +393,15 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self,
 
 	switch (cmd) {
 	case CPU_PM_ENTER:
-		if (cpu == 0) {
+		if (cpu == 0)
 			exynos_pm_central_suspend();
-			if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9)
-				exynos_cpu_save_register();
-		}
 		break;
 
 	case CPU_PM_EXIT:
 		if (cpu == 0) {
 			if (read_cpuid_part_number() ==
-					ARM_CPU_PART_CORTEX_A9) {
+					ARM_CPU_PART_CORTEX_A9)
 				scu_enable(S5P_VA_SCU);
-				exynos_cpu_restore_register();
-			}
 			exynos_pm_central_resume();
 		}
 		break;
-- 
1.7.9.5

[PATCH 1/2] ARM: Exynos: remove arm diagnostic and power register save/restore code

[Chander Kashyap]

As save/restore of arm "diagnostic" and "power" registers is handled by
generic code, so remove the same.

Signed-off-by: Chander Kashyap <k.chander@samsung.com>
---
 arch/arm/mach-exynos/pm.c |   54 ++-------------------------------------------
 1 file changed, 2 insertions(+), 52 deletions(-)

diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
index 202ca73..a092cc3 100644
--- a/arch/arm/mach-exynos/pm.c
+++ b/arch/arm/mach-exynos/pm.c
@@ -196,45 +196,6 @@ void exynos_enter_aftr(void)
 	exynos_sys_powerdown_conf(SYS_AFTR);
 }
 
-/* For Cortex-A9 Diagnostic and Power control register */
-static unsigned int save_arm_register[2];
-
-static void exynos_cpu_save_register(void)
-{
-	unsigned long tmp;
-
-	/* Save Power control register */
-	asm ("mrc p15, 0, %0, c15, c0, 0"
-	     : "=r" (tmp) : : "cc");
-
-	save_arm_register[0] = tmp;
-
-	/* Save Diagnostic register */
-	asm ("mrc p15, 0, %0, c15, c0, 1"
-	     : "=r" (tmp) : : "cc");
-
-	save_arm_register[1] = tmp;
-}
-
-static void exynos_cpu_restore_register(void)
-{
-	unsigned long tmp;
-
-	/* Restore Power control register */
-	tmp = save_arm_register[0];
-
-	asm volatile ("mcr p15, 0, %0, c15, c0, 0"
-		      : : "r" (tmp)
-		      : "cc");
-
-	/* Restore Diagnostic register */
-	tmp = save_arm_register[1];
-
-	asm volatile ("mcr p15, 0, %0, c15, c0, 1"
-		      : : "r" (tmp)
-		      : "cc");
-}
-
 static int exynos_cpu_suspend(unsigned long arg)
 {
 #ifdef CONFIG_CACHE_L2X0
@@ -300,9 +261,6 @@ static int exynos_pm_suspend(void)
 	tmp = (S5P_USE_STANDBY_WFI0 | S5P_USE_STANDBY_WFE0);
 	__raw_writel(tmp, S5P_CENTRAL_SEQ_OPTION);
 
-	if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9)
-		exynos_cpu_save_register();
-
 	return 0;
 }
 
@@ -334,9 +292,6 @@ static void exynos_pm_resume(void)
 	if (exynos_pm_central_resume())
 		goto early_wakeup;
 
-	if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9)
-		exynos_cpu_restore_register();
-
 	/* For release retention */
 
 	__raw_writel((1 << 28), S5P_PAD_RET_MAUDIO_OPTION);
@@ -438,20 +393,15 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self,
 
 	switch (cmd) {
 	case CPU_PM_ENTER:
-		if (cpu == 0) {
+		if (cpu == 0)
 			exynos_pm_central_suspend();
-			if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9)
-				exynos_cpu_save_register();
-		}
 		break;
 
 	case CPU_PM_EXIT:
 		if (cpu == 0) {
 			if (read_cpuid_part_number() ==
-					ARM_CPU_PART_CORTEX_A9) {
+					ARM_CPU_PART_CORTEX_A9)
 				scu_enable(S5P_VA_SCU);
-				exynos_cpu_restore_register();
-			}
 			exynos_pm_central_resume();
 		}
 		break;
-- 
1.7.9.5

[PATCH 2/2] cpuidle: Exynos: fix cpuidle for all states

[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 <k.chander@samsung.com>
---
 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(-)

diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
index 1ee9176..7769f58 100644
--- a/arch/arm/mach-exynos/common.h
+++ b/arch/arm/mach-exynos/common.h
@@ -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);
diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
index a092cc3..328644f 100644
--- a/arch/arm/mach-exynos/pm.c
+++ b/arch/arm/mach-exynos/pm.c
@@ -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;
+	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();
 	}
-
-	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;
 
diff --git a/drivers/cpuidle/cpuidle-exynos.c b/drivers/cpuidle/cpuidle-exynos.c
index 7c01512..1196ca7 100644
--- a/drivers/cpuidle/cpuidle-exynos.c
+++ b/drivers/cpuidle/cpuidle-exynos.c
@@ -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;
@@ -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);
 	cpu_pm_exit();
 
 	return index;
-- 
1.7.9.5

[PATCH 2/2] cpuidle: Exynos: fix cpuidle for all states

[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 <k.chander@samsung.com>
---
 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(-)

diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
index 1ee9176..7769f58 100644
--- a/arch/arm/mach-exynos/common.h
+++ b/arch/arm/mach-exynos/common.h
@@ -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);
diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
index a092cc3..328644f 100644
--- a/arch/arm/mach-exynos/pm.c
+++ b/arch/arm/mach-exynos/pm.c
@@ -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;
+	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();
 	}
-
-	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;
 
diff --git a/drivers/cpuidle/cpuidle-exynos.c b/drivers/cpuidle/cpuidle-exynos.c
index 7c01512..1196ca7 100644
--- a/drivers/cpuidle/cpuidle-exynos.c
+++ b/drivers/cpuidle/cpuidle-exynos.c
@@ -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;
@@ -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);
 	cpu_pm_exit();
 
 	return index;
-- 
1.7.9.5

Re: [PATCH 2/2] cpuidle: Exynos: fix cpuidle for all states

[Tomasz Figa]

Hi Chander,

Please see my comments inline.

On 01.07.2014 16:32, Chander Kashyap wrote:
> 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 <k.chander@samsung.com>
> ---
>  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(-)
> 
> diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
> index 1ee9176..7769f58 100644
> --- a/arch/arm/mach-exynos/common.h
> +++ b/arch/arm/mach-exynos/common.h
> @@ -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);
> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> index a092cc3..328644f 100644
> --- a/arch/arm/mach-exynos/pm.c
> +++ b/arch/arm/mach-exynos/pm.c
> @@ -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;
> +	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.

>  	}
> -
> -	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;
>  
> diff --git a/drivers/cpuidle/cpuidle-exynos.c b/drivers/cpuidle/cpuidle-exynos.c
> index 7c01512..1196ca7 100644
> --- a/drivers/cpuidle/cpuidle-exynos.c
> +++ b/drivers/cpuidle/cpuidle-exynos.c
> @@ -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;
> @@ -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

[PATCH 2/2] cpuidle: Exynos: fix cpuidle for all states

[Tomasz Figa]

Hi Chander,

Please see my comments inline.

On 01.07.2014 16:32, Chander Kashyap wrote:
> 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 <k.chander@samsung.com>
> ---
>  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(-)
> 
> diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
> index 1ee9176..7769f58 100644
> --- a/arch/arm/mach-exynos/common.h
> +++ b/arch/arm/mach-exynos/common.h
> @@ -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);
> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
> index a092cc3..328644f 100644
> --- a/arch/arm/mach-exynos/pm.c
> +++ b/arch/arm/mach-exynos/pm.c
> @@ -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;
> +	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.

>  	}
> -
> -	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;
>  
> diff --git a/drivers/cpuidle/cpuidle-exynos.c b/drivers/cpuidle/cpuidle-exynos.c
> index 7c01512..1196ca7 100644
> --- a/drivers/cpuidle/cpuidle-exynos.c
> +++ b/drivers/cpuidle/cpuidle-exynos.c
> @@ -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;
> @@ -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

Re: [PATCH 2/2] cpuidle: Exynos: fix cpuidle for all states

[Chander Kashyap]

Hi Tomasz,

On Tue, Jul 15, 2014 at 11:11 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Chander,
>
> Please see my comments inline.
>
> On 01.07.2014 16:32, Chander Kashyap wrote:
>> 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 <k.chander@samsung.com>
>> ---
>>  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(-)
>>
>> diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
>> index 1ee9176..7769f58 100644
>> --- a/arch/arm/mach-exynos/common.h
>> +++ b/arch/arm/mach-exynos/common.h
>> @@ -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);
>> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
>> index a092cc3..328644f 100644
>> --- a/arch/arm/mach-exynos/pm.c
>> +++ b/arch/arm/mach-exynos/pm.c
>> @@ -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;
>> +     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.
>
>>       }
>> -
>> -     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;
>>
>> diff --git a/drivers/cpuidle/cpuidle-exynos.c b/drivers/cpuidle/cpuidle-exynos.c
>> index 7c01512..1196ca7 100644
>> --- a/drivers/cpuidle/cpuidle-exynos.c
>> +++ b/drivers/cpuidle/cpuidle-exynos.c
>> @@ -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;
>> @@ -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.

I am fine with this. In that case my patches can be ignored. Also take
the cleanup patch with yours series.

>
> Best regards,
> Tomasz
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] cpuidle: Exynos: fix cpuidle for all states

[Chander Kashyap]

Hi Tomasz,

On Tue, Jul 15, 2014 at 11:11 PM, Tomasz Figa <t.figa@samsung.com> wrote:
> Hi Chander,
>
> Please see my comments inline.
>
> On 01.07.2014 16:32, Chander Kashyap wrote:
>> 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 <k.chander@samsung.com>
>> ---
>>  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(-)
>>
>> diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
>> index 1ee9176..7769f58 100644
>> --- a/arch/arm/mach-exynos/common.h
>> +++ b/arch/arm/mach-exynos/common.h
>> @@ -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);
>> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
>> index a092cc3..328644f 100644
>> --- a/arch/arm/mach-exynos/pm.c
>> +++ b/arch/arm/mach-exynos/pm.c
>> @@ -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;
>> +     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.
>
>>       }
>> -
>> -     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;
>>
>> diff --git a/drivers/cpuidle/cpuidle-exynos.c b/drivers/cpuidle/cpuidle-exynos.c
>> index 7c01512..1196ca7 100644
>> --- a/drivers/cpuidle/cpuidle-exynos.c
>> +++ b/drivers/cpuidle/cpuidle-exynos.c
>> @@ -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;
>> @@ -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.

I am fine with this. In that case my patches can be ignored. Also take
the cleanup patch with yours series.

>
> Best regards,
> Tomasz
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 0/2] cpuidle fixes and cleanup

[Russell King - ARM Linux]

On Tue, Jul 01, 2014 at 08:02:36PM +0530, Chander Kashyap wrote:
> 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.

[PATCH 0/2] cpuidle fixes and cleanup

[Russell King - ARM Linux]

On Tue, Jul 01, 2014 at 08:02:36PM +0530, Chander Kashyap wrote:
> 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.

Re: [PATCH 0/2] cpuidle fixes and cleanup

[Chander Kashyap]

On Tue, Jul 1, 2014 at 8:22 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Jul 01, 2014 at 08:02:36PM +0530, Chander Kashyap wrote:
>> 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.

Sure, I will wait for the conclusion.

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

I will resend after your changes are posted.

Thanks
>
> --
> 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@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/2] cpuidle fixes and cleanup

[Chander Kashyap]

On Tue, Jul 1, 2014 at 8:22 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Jul 01, 2014 at 08:02:36PM +0530, Chander Kashyap wrote:
>> 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.

Sure, I will wait for the conclusion.

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

I will resend after your changes are posted.

Thanks
>
> --
> 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

Re: [PATCH 0/2] cpuidle fixes and cleanup

[Tomasz Figa]

On 02.07.2014 05:11, Chander Kashyap wrote:
> On Tue, Jul 1, 2014 at 8:22 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Tue, Jul 01, 2014 at 08:02:36PM +0530, Chander Kashyap wrote:
>>> 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]

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

However I believe this could be moved into generic helpers.

Best regards,
Tomasz

[PATCH 0/2] cpuidle fixes and cleanup

[Tomasz Figa]

On 02.07.2014 05:11, Chander Kashyap wrote:
> On Tue, Jul 1, 2014 at 8:22 PM, Russell King - ARM Linux
> <linux@arm.linux.org.uk> wrote:
>> On Tue, Jul 01, 2014 at 08:02:36PM +0530, Chander Kashyap wrote:
>>> 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]

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

However I believe this could be moved into generic helpers.

Best regards,
Tomasz

Re: [PATCH 0/2] cpuidle fixes and cleanup

[Russell King - ARM Linux]

On Tue, Jul 08, 2014 at 03:56:48PM +0200, Tomasz Figa wrote:
> On 02.07.2014 05:11, Chander Kashyap wrote:
> > On Tue, Jul 1, 2014 at 8:22 PM, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> >> On Tue, Jul 01, 2014 at 08:02:36PM +0530, Chander Kashyap wrote:
> >>> 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]
> 
> >>
> >> 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.

[PATCH 0/2] cpuidle fixes and cleanup

[Russell King - ARM Linux]

On Tue, Jul 08, 2014 at 03:56:48PM +0200, Tomasz Figa wrote:
> On 02.07.2014 05:11, Chander Kashyap wrote:
> > On Tue, Jul 1, 2014 at 8:22 PM, Russell King - ARM Linux
> > <linux@arm.linux.org.uk> wrote:
> >> On Tue, Jul 01, 2014 at 08:02:36PM +0530, Chander Kashyap wrote:
> >>> 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]
> 
> >>
> >> 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.

Re: [PATCH 0/2] cpuidle fixes and cleanup

[Chander Kashyap]

On Tue, Jul 8, 2014 at 7:47 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Jul 08, 2014 at 03:56:48PM +0200, Tomasz Figa wrote:
>> On 02.07.2014 05:11, Chander Kashyap wrote:
>> > On Tue, Jul 1, 2014 at 8:22 PM, Russell King - ARM Linux
>> > <linux@arm.linux.org.uk> wrote:
>> >> On Tue, Jul 01, 2014 at 08:02:36PM +0530, Chander Kashyap wrote:
>> >>> 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]
>>
>> >>
>> >> 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.

In that case i will re-post this after Russell post changes which
conflict with that patch.

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

[PATCH 0/2] cpuidle fixes and cleanup

[Chander Kashyap]

On Tue, Jul 8, 2014 at 7:47 PM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:
> On Tue, Jul 08, 2014 at 03:56:48PM +0200, Tomasz Figa wrote:
>> On 02.07.2014 05:11, Chander Kashyap wrote:
>> > On Tue, Jul 1, 2014 at 8:22 PM, Russell King - ARM Linux
>> > <linux@arm.linux.org.uk> wrote:
>> >> On Tue, Jul 01, 2014 at 08:02:36PM +0530, Chander Kashyap wrote:
>> >>> 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]
>>
>> >>
>> >> 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.

In that case i will re-post this after Russell post changes which
conflict with that patch.

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