[PATCH 0/3] ARM: EXYNOS: cpuidle driver fixes

[PATCH 0/3] ARM: EXYNOS: cpuidle driver fixes

[Bartlomiej Zolnierkiewicz]

Hi,

This patchset makes ARM EXYNOS cpuidle driver work on EXYNOS4210,
prepares it for the future ARM multiplatform support and finally
moves the driver to drivers/cpuidle/.

Patches are against v3.10-rc6 (they also apply to next-20130624).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics


Bartlomiej Zolnierkiewicz (3):
  ARM: EXYNOS: remove non-working AFTR mode support
  ARM: EXYNOS: init cpuidle driver in exynos_init_late()
  ARM: EXYNOS: move cpuidle driver to drivers/cpuidle/

 arch/arm/mach-exynos/Makefile    |   1 -
 arch/arm/mach-exynos/common.c    |   8 +-
 arch/arm/mach-exynos/common.h    |   6 ++
 arch/arm/mach-exynos/cpuidle.c   | 225 ---------------------------------------
 drivers/cpuidle/Makefile         |   3 +
 drivers/cpuidle/cpuidle-exynos.c |  92 ++++++++++++++++
 6 files changed, 105 insertions(+), 230 deletions(-)
 delete mode 100644 arch/arm/mach-exynos/cpuidle.c
 create mode 100644 drivers/cpuidle/cpuidle-exynos.c

-- 
1.8.2.3

[PATCH 1/3] ARM: EXYNOS: remove non-working AFTR mode support

[Bartlomiej Zolnierkiewicz]

AFTR mode support was introduced by commit 67173ca ("ARM: EXYNOS: Add
support AFTR mode on EXYNOS4210") in v3.4 kernel.  Unfortunately even
in v3.4 kernel it hasn't worked as supposed and this is still the case
with v3.10-rc6 (it probably wasn't noticed because CONFIG_CPU_IDLE is
not turned on by default):

- on revision 0 of Exynos4210 (Universal C210 board) it causes lockup
  (on this revision only one core is usable so entry to AFTR mode is
  always attempted because the code tries to go into AFTR mode when all
  other CPUs except CPU0 are offlined)

- on revision 1.1 of Exynos4210 (Trats board) it causes a lockup when
  CPU1 is offlined (i.e. echo 0 > /sys/devices/system/cpu/cpu1/online)

- on later Exynos4/5 SoCs wrong registers may be accessed when all CPUs
  except CPU0 are offlined resulting in panic/lockup (REG_DIRECTGO_ADDR
  and REG_DIRECTGO_FLAG register selections was implemented only for
  Exynos4210)

Just remove AFTR mode support for now.

Cc: Jaecheol Lee <jc.lee@samsung.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Amit Daniel Kachhap <amit.kachhap@linaro.org>
Cc: Tomasz Figa <t.figa@samsung.com>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
---
 arch/arm/mach-exynos/cpuidle.c | 131 +----------------------------------------
 1 file changed, 1 insertion(+), 130 deletions(-)

diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
index 17a18ff..0a657ac 100644
--- a/arch/arm/mach-exynos/cpuidle.c
+++ b/arch/arm/mach-exynos/cpuidle.c
@@ -17,30 +17,11 @@
 #include <linux/time.h>
 
 #include <asm/proc-fns.h>
-#include <asm/smp_scu.h>
-#include <asm/suspend.h>
-#include <asm/unified.h>
 #include <asm/cpuidle.h>
 #include <mach/regs-clock.h>
-#include <mach/regs-pmu.h>
-
-#include <plat/cpu.h>
 
 #include "common.h"
 
-#define REG_DIRECTGO_ADDR	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
-			S5P_INFORM7 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
-			(S5P_VA_SYSRAM + 0x24) : S5P_INFORM0))
-#define REG_DIRECTGO_FLAG	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
-			S5P_INFORM6 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
-			(S5P_VA_SYSRAM + 0x20) : S5P_INFORM1))
-
-#define S5P_CHECK_AFTR		0xFCBA0D10
-
-static int exynos4_enter_lowpower(struct cpuidle_device *dev,
-				struct cpuidle_driver *drv,
-				int index);
-
 static DEFINE_PER_CPU(struct cpuidle_device, exynos4_cpuidle_device);
 
 static struct cpuidle_driver exynos4_idle_driver = {
@@ -48,117 +29,11 @@ static struct cpuidle_driver exynos4_idle_driver = {
 	.owner			= THIS_MODULE,
 	.states = {
 		[0] = ARM_CPUIDLE_WFI_STATE,
-		[1] = {
-			.enter			= exynos4_enter_lowpower,
-			.exit_latency		= 300,
-			.target_residency	= 100000,
-			.flags			= CPUIDLE_FLAG_TIME_VALID,
-			.name			= "C1",
-			.desc			= "ARM power down",
-		},
 	},
-	.state_count = 2,
+	.state_count = 1,
 	.safe_state_index = 0,
 };
 
-/* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */
-static void exynos4_set_wakeupmask(void)
-{
-	__raw_writel(0x0000ff3e, S5P_WAKEUP_MASK);
-}
-
-static unsigned int g_pwr_ctrl, g_diag_reg;
-
-static void save_cpu_arch_register(void)
-{
-	/*read power control register*/
-	asm("mrc p15, 0, %0, c15, c0, 0" : "=r"(g_pwr_ctrl) : : "cc");
-	/*read diagnostic register*/
-	asm("mrc p15, 0, %0, c15, c0, 1" : "=r"(g_diag_reg) : : "cc");
-	return;
-}
-
-static void restore_cpu_arch_register(void)
-{
-	/*write power control register*/
-	asm("mcr p15, 0, %0, c15, c0, 0" : : "r"(g_pwr_ctrl) : "cc");
-	/*write diagnostic register*/
-	asm("mcr p15, 0, %0, c15, c0, 1" : : "r"(g_diag_reg) : "cc");
-	return;
-}
-
-static int idle_finisher(unsigned long flags)
-{
-	cpu_do_idle();
-	return 1;
-}
-
-static int exynos4_enter_core0_aftr(struct cpuidle_device *dev,
-				struct cpuidle_driver *drv,
-				int index)
-{
-	unsigned long tmp;
-
-	exynos4_set_wakeupmask();
-
-	/* Set value of power down register for aftr mode */
-	exynos_sys_powerdown_conf(SYS_AFTR);
-
-	__raw_writel(virt_to_phys(s3c_cpu_resume), REG_DIRECTGO_ADDR);
-	__raw_writel(S5P_CHECK_AFTR, REG_DIRECTGO_FLAG);
-
-	save_cpu_arch_register();
-
-	/* Setting Central Sequence Register for power down mode */
-	tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
-	tmp &= ~S5P_CENTRAL_LOWPWR_CFG;
-	__raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
-
-	cpu_pm_enter();
-	cpu_suspend(0, idle_finisher);
-
-#ifdef CONFIG_SMP
-	if (!soc_is_exynos5250())
-		scu_enable(S5P_VA_SCU);
-#endif
-	cpu_pm_exit();
-
-	restore_cpu_arch_register();
-
-	/*
-	 * If PMU failed while entering sleep mode, WFI will be
-	 * ignored by PMU and then exiting cpu_do_idle().
-	 * S5P_CENTRAL_LOWPWR_CFG bit will not be set automatically
-	 * in this situation.
-	 */
-	tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
-	if (!(tmp & S5P_CENTRAL_LOWPWR_CFG)) {
-		tmp |= S5P_CENTRAL_LOWPWR_CFG;
-		__raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
-	}
-
-	/* Clear wakeup state register */
-	__raw_writel(0x0, S5P_WAKEUP_STAT);
-
-	return index;
-}
-
-static int exynos4_enter_lowpower(struct cpuidle_device *dev,
-				struct cpuidle_driver *drv,
-				int index)
-{
-	int new_index = index;
-
-	/* This mode only can be entered when other core's are offline */
-	if (num_online_cpus() > 1)
-		new_index = drv->safe_state_index;
-
-	if (new_index == 0)
-		return arm_cpuidle_simple_enter(dev, drv, new_index);
-	else
-		return exynos4_enter_core0_aftr(dev, drv, new_index);
-}
-
 static void __init exynos5_core_down_clk(void)
 {
 	unsigned int tmp;
@@ -209,10 +84,6 @@ static int __init exynos4_init_cpuidle(void)
 		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
 		device->cpu = cpu_id;
 
-		/* Support IDLE only */
-		if (cpu_id != 0)
-			device->state_count = 1;
-
 		ret = cpuidle_register_device(device);
 		if (ret) {
 			printk(KERN_ERR "CPUidle register device failed\n");
-- 
1.8.2.3

[PATCH 1/3] ARM: EXYNOS: remove non-working AFTR mode support

[Daniel Lezcano]

On 06/26/2013 12:13 PM, Bartlomiej Zolnierkiewicz wrote:
> AFTR mode support was introduced by commit 67173ca ("ARM: EXYNOS: Add
> support AFTR mode on EXYNOS4210") in v3.4 kernel.  Unfortunately even
> in v3.4 kernel it hasn't worked as supposed and this is still the case
> with v3.10-rc6 (it probably wasn't noticed because CONFIG_CPU_IDLE is
> not turned on by default):
> 
> - on revision 0 of Exynos4210 (Universal C210 board) it causes lockup
>   (on this revision only one core is usable so entry to AFTR mode is
>   always attempted because the code tries to go into AFTR mode when all
>   other CPUs except CPU0 are offlined)
> 
> - on revision 1.1 of Exynos4210 (Trats board) it causes a lockup when
>   CPU1 is offlined (i.e. echo 0 > /sys/devices/system/cpu/cpu1/online)
> 
> - on later Exynos4/5 SoCs wrong registers may be accessed when all CPUs
>   except CPU0 are offlined resulting in panic/lockup (REG_DIRECTGO_ADDR
>   and REG_DIRECTGO_FLAG register selections was implemented only for
>   Exynos4210)
> 
> Just remove AFTR mode support for now.

Ok, I will jump on the opportunity to talk about this state.

1. I tried different ways to make the AFTR state to be entered with
*both* cpu online. It goes successfully to this state. The CPU0 is
correctly woken up but the CPU1 is never woken up, why is it happening ?

https://bugs.launchpad.net/linaro-landing-team-samsung/+bug/1171518

2. The CPU1 hotplug bug should been fixed and if I am not wrong there is
a patch somewhere fixing this.

https://bugs.launchpad.net/linaro-power-kernel/+bug/1171382

3. What is the fix for Exynos5 ?

https://bugs.launchpad.net/linaro-power-kernel/+bug/1171253

It sounds like depending on Hypervisor mode is enabled or not, the AFTR
does not work correctly.

In other words, instead of removing the AFTR state I suggest to fix it:
both core being online, split driver for exynos4 and 5.

Thanks
  -- Daniel

> Cc: Jaecheol Lee <jc.lee@samsung.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Amit Daniel Kachhap <amit.kachhap@linaro.org>
> Cc: Tomasz Figa <t.figa@samsung.com>
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> ---
>  arch/arm/mach-exynos/cpuidle.c | 131 +----------------------------------------
>  1 file changed, 1 insertion(+), 130 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
> index 17a18ff..0a657ac 100644
> --- a/arch/arm/mach-exynos/cpuidle.c
> +++ b/arch/arm/mach-exynos/cpuidle.c
> @@ -17,30 +17,11 @@
>  #include <linux/time.h>
>  
>  #include <asm/proc-fns.h>
> -#include <asm/smp_scu.h>
> -#include <asm/suspend.h>
> -#include <asm/unified.h>
>  #include <asm/cpuidle.h>
>  #include <mach/regs-clock.h>
> -#include <mach/regs-pmu.h>
> -
> -#include <plat/cpu.h>
>  
>  #include "common.h"
>  
> -#define REG_DIRECTGO_ADDR	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
> -			S5P_INFORM7 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
> -			(S5P_VA_SYSRAM + 0x24) : S5P_INFORM0))
> -#define REG_DIRECTGO_FLAG	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
> -			S5P_INFORM6 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
> -			(S5P_VA_SYSRAM + 0x20) : S5P_INFORM1))
> -
> -#define S5P_CHECK_AFTR		0xFCBA0D10
> -
> -static int exynos4_enter_lowpower(struct cpuidle_device *dev,
> -				struct cpuidle_driver *drv,
> -				int index);
> -
>  static DEFINE_PER_CPU(struct cpuidle_device, exynos4_cpuidle_device);
>  
>  static struct cpuidle_driver exynos4_idle_driver = {
> @@ -48,117 +29,11 @@ static struct cpuidle_driver exynos4_idle_driver = {
>  	.owner			= THIS_MODULE,
>  	.states = {
>  		[0] = ARM_CPUIDLE_WFI_STATE,
> -		[1] = {
> -			.enter			= exynos4_enter_lowpower,
> -			.exit_latency		= 300,
> -			.target_residency	= 100000,
> -			.flags			= CPUIDLE_FLAG_TIME_VALID,
> -			.name			= "C1",
> -			.desc			= "ARM power down",
> -		},
>  	},
> -	.state_count = 2,
> +	.state_count = 1,
>  	.safe_state_index = 0,
>  };
>  
> -/* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */
> -static void exynos4_set_wakeupmask(void)
> -{
> -	__raw_writel(0x0000ff3e, S5P_WAKEUP_MASK);
> -}
> -
> -static unsigned int g_pwr_ctrl, g_diag_reg;
> -
> -static void save_cpu_arch_register(void)
> -{
> -	/*read power control register*/
> -	asm("mrc p15, 0, %0, c15, c0, 0" : "=r"(g_pwr_ctrl) : : "cc");
> -	/*read diagnostic register*/
> -	asm("mrc p15, 0, %0, c15, c0, 1" : "=r"(g_diag_reg) : : "cc");
> -	return;
> -}
> -
> -static void restore_cpu_arch_register(void)
> -{
> -	/*write power control register*/
> -	asm("mcr p15, 0, %0, c15, c0, 0" : : "r"(g_pwr_ctrl) : "cc");
> -	/*write diagnostic register*/
> -	asm("mcr p15, 0, %0, c15, c0, 1" : : "r"(g_diag_reg) : "cc");
> -	return;
> -}
> -
> -static int idle_finisher(unsigned long flags)
> -{
> -	cpu_do_idle();
> -	return 1;
> -}
> -
> -static int exynos4_enter_core0_aftr(struct cpuidle_device *dev,
> -				struct cpuidle_driver *drv,
> -				int index)
> -{
> -	unsigned long tmp;
> -
> -	exynos4_set_wakeupmask();
> -
> -	/* Set value of power down register for aftr mode */
> -	exynos_sys_powerdown_conf(SYS_AFTR);
> -
> -	__raw_writel(virt_to_phys(s3c_cpu_resume), REG_DIRECTGO_ADDR);
> -	__raw_writel(S5P_CHECK_AFTR, REG_DIRECTGO_FLAG);
> -
> -	save_cpu_arch_register();
> -
> -	/* Setting Central Sequence Register for power down mode */
> -	tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
> -	tmp &= ~S5P_CENTRAL_LOWPWR_CFG;
> -	__raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
> -
> -	cpu_pm_enter();
> -	cpu_suspend(0, idle_finisher);
> -
> -#ifdef CONFIG_SMP
> -	if (!soc_is_exynos5250())
> -		scu_enable(S5P_VA_SCU);
> -#endif
> -	cpu_pm_exit();
> -
> -	restore_cpu_arch_register();
> -
> -	/*
> -	 * If PMU failed while entering sleep mode, WFI will be
> -	 * ignored by PMU and then exiting cpu_do_idle().
> -	 * S5P_CENTRAL_LOWPWR_CFG bit will not be set automatically
> -	 * in this situation.
> -	 */
> -	tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
> -	if (!(tmp & S5P_CENTRAL_LOWPWR_CFG)) {
> -		tmp |= S5P_CENTRAL_LOWPWR_CFG;
> -		__raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
> -	}
> -
> -	/* Clear wakeup state register */
> -	__raw_writel(0x0, S5P_WAKEUP_STAT);
> -
> -	return index;
> -}
> -
> -static int exynos4_enter_lowpower(struct cpuidle_device *dev,
> -				struct cpuidle_driver *drv,
> -				int index)
> -{
> -	int new_index = index;
> -
> -	/* This mode only can be entered when other core's are offline */
> -	if (num_online_cpus() > 1)
> -		new_index = drv->safe_state_index;
> -
> -	if (new_index == 0)
> -		return arm_cpuidle_simple_enter(dev, drv, new_index);
> -	else
> -		return exynos4_enter_core0_aftr(dev, drv, new_index);
> -}
> -
>  static void __init exynos5_core_down_clk(void)
>  {
>  	unsigned int tmp;
> @@ -209,10 +84,6 @@ static int __init exynos4_init_cpuidle(void)
>  		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
>  		device->cpu = cpu_id;
>  
> -		/* Support IDLE only */
> -		if (cpu_id != 0)
> -			device->state_count = 1;
> -
>  		ret = cpuidle_register_device(device);
>  		if (ret) {
>  			printk(KERN_ERR "CPUidle register device failed\n");
> 


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

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

[PATCH 1/3] ARM: EXYNOS: remove non-working AFTR mode support

[Bartlomiej Zolnierkiewicz]

On Wednesday, June 26, 2013 12:36:12 PM Daniel Lezcano wrote:
> On 06/26/2013 12:13 PM, Bartlomiej Zolnierkiewicz wrote:
> > AFTR mode support was introduced by commit 67173ca ("ARM: EXYNOS: Add
> > support AFTR mode on EXYNOS4210") in v3.4 kernel.  Unfortunately even
> > in v3.4 kernel it hasn't worked as supposed and this is still the case
> > with v3.10-rc6 (it probably wasn't noticed because CONFIG_CPU_IDLE is
> > not turned on by default):
> > 
> > - on revision 0 of Exynos4210 (Universal C210 board) it causes lockup
> >   (on this revision only one core is usable so entry to AFTR mode is
> >   always attempted because the code tries to go into AFTR mode when all
> >   other CPUs except CPU0 are offlined)
> > 
> > - on revision 1.1 of Exynos4210 (Trats board) it causes a lockup when
> >   CPU1 is offlined (i.e. echo 0 > /sys/devices/system/cpu/cpu1/online)
> > 
> > - on later Exynos4/5 SoCs wrong registers may be accessed when all CPUs
> >   except CPU0 are offlined resulting in panic/lockup (REG_DIRECTGO_ADDR
> >   and REG_DIRECTGO_FLAG register selections was implemented only for
> >   Exynos4210)
> > 
> > Just remove AFTR mode support for now.
> 
> Ok, I will jump on the opportunity to talk about this state.
> 
> 1. I tried different ways to make the AFTR state to be entered with
> *both* cpu online. It goes successfully to this state. The CPU0 is
> correctly woken up but the CPU1 is never woken up, why is it happening ?
> 
> https://bugs.launchpad.net/linaro-landing-team-samsung/+bug/1171518

No idea here, AFTR doesn't work for me with upstream kernels even if
only one CPU is online.

Also the documentation says that before entering system-level power-down
mode (such as AFTR) when multiple CPUs cores are used all other CPU cores
should stop interrupt service so I'm not sure how the way attempted by
you should work.

> 2. The CPU1 hotplug bug should been fixed and if I am not wrong there is
> a patch somewhere fixing this.
> 
> https://bugs.launchpad.net/linaro-power-kernel/+bug/1171382

Unfortunately none of the patches there helps with my issues.

> 3. What is the fix for Exynos5 ?
> 
> https://bugs.launchpad.net/linaro-power-kernel/+bug/1171253
> 
> It sounds like depending on Hypervisor mode is enabled or not, the AFTR
> does not work correctly.

Sorry no idea here either.  On any SoCs later than EXYNOS4210 the
registers used for s3c_cpu_resume address and 0xFCBA0D10 magic number
may be different than EXYNOS4210 defaults (at least on EXYNOS4412 they
indeed are different, unfortunately I lack any info needed for EXYNOS5
support). You are lucky that it even works in some cases on EXYNOS5250.

> In other words, instead of removing the AFTR state I suggest to fix it:
> both core being online, split driver for exynos4 and 5.

My main problem is that with the upstream kernel even on EXYNOS4210
rev0 (only one core useable due to hardware issues) the kernel goes
into AFTR state for the first few times during boot and then it just
lockups (after going into cpu_do_idle() which is really cpu_v7_do_idle()
and which does wfi call) and doesn't wake up CPU0. I have currently
no idea how to fix or debug it further.

The issue happens with every upstream kernel version tried (from v3.4
to v3.10-rc6).  Lockups also happen on EXYNOS4210 rev1.1 when CPU1 is
offlined by hand and then cpuidle driver tries to go into AFTR mode
(because by default it doesn't go into AFTR mode on any SoC except
EXYNOS4210 rev0).

I don't have EXYNOS4210 rev1.0 but it seems that in the upstream AFTR
mode has never worked (even on hardware that it was originally developed
on) since its introduction in v3.4 (which was released on 20th May 2012).

IOW for over the year nobody cared to make it work and I have currently
no fix at hand so the corrent upstream resolution is to just remove the
known non-working code and re-introduce it later when/if needed (I can
just disable it with a small fix but we don't keep such long-term broken
code as placeholder in the upstream kernel).  If left as it is people
can hit the known issues and waste time debugging them, just like this
happenend for me [1].

If you have AFTR mode working (especially on EXYNOS4210) in Linaro
kernels please get fixes upstream ASAP. However I still wonder whether
the maintanance nightmare (bugs for different cases in your launchpad)
is worth gains over standard idle mode as the rumor around here is that
they are not that great (unfortunately no numbers were provided during
original feature addition).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

[1] Somebody enabled CONFIG_CPU_IDLE in the default config in one of our
internal kernel trees on a target on which this feature "works" (since
CPUs are not hot-unplugged by default on all targets except EXYNOS4210
rev0 the code for AFTR is not used) and resulted in lockups on boot on
my default testing target. It took my long time to find out that problem
is actually caused by just enabling exynos cpuidle driver.

> Thanks
>   -- Daniel
> 
> > Cc: Jaecheol Lee <jc.lee@samsung.com>
> > Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> > Cc: Amit Daniel Kachhap <amit.kachhap@linaro.org>
> > Cc: Tomasz Figa <t.figa@samsung.com>
> > Cc: Kukjin Kim <kgene.kim@samsung.com>
> > Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> > Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> > Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> > Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> > ---
> >  arch/arm/mach-exynos/cpuidle.c | 131 +----------------------------------------
> >  1 file changed, 1 insertion(+), 130 deletions(-)
> > 
> > diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
> > index 17a18ff..0a657ac 100644
> > --- a/arch/arm/mach-exynos/cpuidle.c
> > +++ b/arch/arm/mach-exynos/cpuidle.c
> > @@ -17,30 +17,11 @@
> >  #include <linux/time.h>
> >  
> >  #include <asm/proc-fns.h>
> > -#include <asm/smp_scu.h>
> > -#include <asm/suspend.h>
> > -#include <asm/unified.h>
> >  #include <asm/cpuidle.h>
> >  #include <mach/regs-clock.h>
> > -#include <mach/regs-pmu.h>
> > -
> > -#include <plat/cpu.h>
> >  
> >  #include "common.h"
> >  
> > -#define REG_DIRECTGO_ADDR	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
> > -			S5P_INFORM7 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
> > -			(S5P_VA_SYSRAM + 0x24) : S5P_INFORM0))
> > -#define REG_DIRECTGO_FLAG	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
> > -			S5P_INFORM6 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
> > -			(S5P_VA_SYSRAM + 0x20) : S5P_INFORM1))
> > -
> > -#define S5P_CHECK_AFTR		0xFCBA0D10
> > -
> > -static int exynos4_enter_lowpower(struct cpuidle_device *dev,
> > -				struct cpuidle_driver *drv,
> > -				int index);
> > -
> >  static DEFINE_PER_CPU(struct cpuidle_device, exynos4_cpuidle_device);
> >  
> >  static struct cpuidle_driver exynos4_idle_driver = {
> > @@ -48,117 +29,11 @@ static struct cpuidle_driver exynos4_idle_driver = {
> >  	.owner			= THIS_MODULE,
> >  	.states = {
> >  		[0] = ARM_CPUIDLE_WFI_STATE,
> > -		[1] = {
> > -			.enter			= exynos4_enter_lowpower,
> > -			.exit_latency		= 300,
> > -			.target_residency	= 100000,
> > -			.flags			= CPUIDLE_FLAG_TIME_VALID,
> > -			.name			= "C1",
> > -			.desc			= "ARM power down",
> > -		},
> >  	},
> > -	.state_count = 2,
> > +	.state_count = 1,
> >  	.safe_state_index = 0,
> >  };
> >  
> > -/* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */
> > -static void exynos4_set_wakeupmask(void)
> > -{
> > -	__raw_writel(0x0000ff3e, S5P_WAKEUP_MASK);
> > -}
> > -
> > -static unsigned int g_pwr_ctrl, g_diag_reg;
> > -
> > -static void save_cpu_arch_register(void)
> > -{
> > -	/*read power control register*/
> > -	asm("mrc p15, 0, %0, c15, c0, 0" : "=r"(g_pwr_ctrl) : : "cc");
> > -	/*read diagnostic register*/
> > -	asm("mrc p15, 0, %0, c15, c0, 1" : "=r"(g_diag_reg) : : "cc");
> > -	return;
> > -}
> > -
> > -static void restore_cpu_arch_register(void)
> > -{
> > -	/*write power control register*/
> > -	asm("mcr p15, 0, %0, c15, c0, 0" : : "r"(g_pwr_ctrl) : "cc");
> > -	/*write diagnostic register*/
> > -	asm("mcr p15, 0, %0, c15, c0, 1" : : "r"(g_diag_reg) : "cc");
> > -	return;
> > -}
> > -
> > -static int idle_finisher(unsigned long flags)
> > -{
> > -	cpu_do_idle();
> > -	return 1;
> > -}
> > -
> > -static int exynos4_enter_core0_aftr(struct cpuidle_device *dev,
> > -				struct cpuidle_driver *drv,
> > -				int index)
> > -{
> > -	unsigned long tmp;
> > -
> > -	exynos4_set_wakeupmask();
> > -
> > -	/* Set value of power down register for aftr mode */
> > -	exynos_sys_powerdown_conf(SYS_AFTR);
> > -
> > -	__raw_writel(virt_to_phys(s3c_cpu_resume), REG_DIRECTGO_ADDR);
> > -	__raw_writel(S5P_CHECK_AFTR, REG_DIRECTGO_FLAG);
> > -
> > -	save_cpu_arch_register();
> > -
> > -	/* Setting Central Sequence Register for power down mode */
> > -	tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
> > -	tmp &= ~S5P_CENTRAL_LOWPWR_CFG;
> > -	__raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
> > -
> > -	cpu_pm_enter();
> > -	cpu_suspend(0, idle_finisher);
> > -
> > -#ifdef CONFIG_SMP
> > -	if (!soc_is_exynos5250())
> > -		scu_enable(S5P_VA_SCU);
> > -#endif
> > -	cpu_pm_exit();
> > -
> > -	restore_cpu_arch_register();
> > -
> > -	/*
> > -	 * If PMU failed while entering sleep mode, WFI will be
> > -	 * ignored by PMU and then exiting cpu_do_idle().
> > -	 * S5P_CENTRAL_LOWPWR_CFG bit will not be set automatically
> > -	 * in this situation.
> > -	 */
> > -	tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
> > -	if (!(tmp & S5P_CENTRAL_LOWPWR_CFG)) {
> > -		tmp |= S5P_CENTRAL_LOWPWR_CFG;
> > -		__raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
> > -	}
> > -
> > -	/* Clear wakeup state register */
> > -	__raw_writel(0x0, S5P_WAKEUP_STAT);
> > -
> > -	return index;
> > -}
> > -
> > -static int exynos4_enter_lowpower(struct cpuidle_device *dev,
> > -				struct cpuidle_driver *drv,
> > -				int index)
> > -{
> > -	int new_index = index;
> > -
> > -	/* This mode only can be entered when other core's are offline */
> > -	if (num_online_cpus() > 1)
> > -		new_index = drv->safe_state_index;
> > -
> > -	if (new_index == 0)
> > -		return arm_cpuidle_simple_enter(dev, drv, new_index);
> > -	else
> > -		return exynos4_enter_core0_aftr(dev, drv, new_index);
> > -}
> > -
> >  static void __init exynos5_core_down_clk(void)
> >  {
> >  	unsigned int tmp;
> > @@ -209,10 +84,6 @@ static int __init exynos4_init_cpuidle(void)
> >  		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
> >  		device->cpu = cpu_id;
> >  
> > -		/* Support IDLE only */
> > -		if (cpu_id != 0)
> > -			device->state_count = 1;
> > -
> >  		ret = cpuidle_register_device(device);
> >  		if (ret) {
> >  			printk(KERN_ERR "CPUidle register device failed\n");

[PATCH 1/3] ARM: EXYNOS: remove non-working AFTR mode support

[Daniel Lezcano]

On 06/27/2013 08:10 PM, Bartlomiej Zolnierkiewicz wrote:
> On Wednesday, June 26, 2013 12:36:12 PM Daniel Lezcano wrote:
>> On 06/26/2013 12:13 PM, Bartlomiej Zolnierkiewicz wrote:
>>> AFTR mode support was introduced by commit 67173ca ("ARM: EXYNOS: Add
>>> support AFTR mode on EXYNOS4210") in v3.4 kernel.  Unfortunately even
>>> in v3.4 kernel it hasn't worked as supposed and this is still the case
>>> with v3.10-rc6 (it probably wasn't noticed because CONFIG_CPU_IDLE is
>>> not turned on by default):
>>>
>>> - on revision 0 of Exynos4210 (Universal C210 board) it causes lockup
>>>   (on this revision only one core is usable so entry to AFTR mode is
>>>   always attempted because the code tries to go into AFTR mode when all
>>>   other CPUs except CPU0 are offlined)
>>>
>>> - on revision 1.1 of Exynos4210 (Trats board) it causes a lockup when
>>>   CPU1 is offlined (i.e. echo 0 > /sys/devices/system/cpu/cpu1/online)
>>>
>>> - on later Exynos4/5 SoCs wrong registers may be accessed when all CPUs
>>>   except CPU0 are offlined resulting in panic/lockup (REG_DIRECTGO_ADDR
>>>   and REG_DIRECTGO_FLAG register selections was implemented only for
>>>   Exynos4210)
>>>
>>> Just remove AFTR mode support for now.
>>
>> Ok, I will jump on the opportunity to talk about this state.
>>
>> 1. I tried different ways to make the AFTR state to be entered with
>> *both* cpu online. It goes successfully to this state. The CPU0 is
>> correctly woken up but the CPU1 is never woken up, why is it happening ?
>>
>> https://bugs.launchpad.net/linaro-landing-team-samsung/+bug/1171518
> 
> No idea here, AFTR doesn't work for me with upstream kernels even if
> only one CPU is online.

What do you mean by "AFTR doesn't work" ? Is the kernel hanging ? The
state is never reached ?

> Also the documentation says that before entering system-level power-down
> mode (such as AFTR) when multiple CPUs cores are used all other CPU cores
> should stop interrupt service so I'm not sure how the way attempted by
> you should work.

The cpu enters the idle mode with the interrupts disabled.

>> 2. The CPU1 hotplug bug should been fixed and if I am not wrong there is
>> a patch somewhere fixing this.
>>
>> https://bugs.launchpad.net/linaro-power-kernel/+bug/1171382
> 
> Unfortunately none of the patches there helps with my issues.
> 
>> 3. What is the fix for Exynos5 ?
>>
>> https://bugs.launchpad.net/linaro-power-kernel/+bug/1171253
>>
>> It sounds like depending on Hypervisor mode is enabled or not, the AFTR
>> does not work correctly.
> 
> Sorry no idea here either.  On any SoCs later than EXYNOS4210 the
> registers used for s3c_cpu_resume address and 0xFCBA0D10 magic number
> may be different than EXYNOS4210 defaults (at least on EXYNOS4412 they
> indeed are different, unfortunately I lack any info needed for EXYNOS5
> support). You are lucky that it even works in some cases on EXYNOS5250.
> 
>> In other words, instead of removing the AFTR state I suggest to fix it:
>> both core being online, split driver for exynos4 and 5.
> 
> My main problem is that with the upstream kernel even on EXYNOS4210
> rev0 (only one core useable due to hardware issues) the kernel goes
> into AFTR state for the first few times during boot and then it just
> lockups (after going into cpu_do_idle() which is really cpu_v7_do_idle()
> and which does wfi call) and doesn't wake up CPU0. I have currently
> no idea how to fix or debug it further.

I have an Origen 4210 board Ver A. and it works without problem with the
AFTR mode (cpu1 unplugged).

> The issue happens with every upstream kernel version tried (from v3.4
> to v3.10-rc6).  Lockups also happen on EXYNOS4210 rev1.1 when CPU1 is
> offlined by hand and then cpuidle driver tries to go into AFTR mode
> (because by default it doesn't go into AFTR mode on any SoC except
> EXYNOS4210 rev0).
> 
> I don't have EXYNOS4210 rev1.0 but it seems that in the upstream AFTR
> mode has never worked (even on hardware that it was originally developed
> on) since its introduction in v3.4 (which was released on 20th May 2012).
> 
> IOW for over the year nobody cared to make it work and I have currently
> no fix at hand so the corrent upstream resolution is to just remove the
> known non-working code and re-introduce it later when/if needed (I can
> just disable it with a small fix but we don't keep such long-term broken
> code as placeholder in the upstream kernel).  If left as it is people
> can hit the known issues and waste time debugging them, just like this
> happenend for me [1].
> 
> If you have AFTR mode working (especially on EXYNOS4210) in Linaro
> kernels please get fixes upstream ASAP. However I still wonder whether
> the maintanance nightmare (bugs for different cases in your launchpad)
> is worth gains over standard idle mode as the rumor around here is that
> they are not that great (unfortunately no numbers were provided during
> original feature addition).

It works forme with a vanilla kernel 3.10.0-rc7.

Removing a feature because it seems not working is not a good solution.
The right way is to investigate what is happening and why.

Thanks
  -- Daniel

> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
> 
> [1] Somebody enabled CONFIG_CPU_IDLE in the default config in one of our
> internal kernel trees on a target on which this feature "works" (since
> CPUs are not hot-unplugged by default on all targets except EXYNOS4210
> rev0 the code for AFTR is not used) and resulted in lockups on boot on
> my default testing target. It took my long time to find out that problem
> is actually caused by just enabling exynos cpuidle driver.
> 
>> Thanks
>>   -- Daniel
>>
>>> Cc: Jaecheol Lee <jc.lee@samsung.com>
>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>> Cc: Amit Daniel Kachhap <amit.kachhap@linaro.org>
>>> Cc: Tomasz Figa <t.figa@samsung.com>
>>> Cc: Kukjin Kim <kgene.kim@samsung.com>
>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>>> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>>> ---
>>>  arch/arm/mach-exynos/cpuidle.c | 131 +----------------------------------------
>>>  1 file changed, 1 insertion(+), 130 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
>>> index 17a18ff..0a657ac 100644
>>> --- a/arch/arm/mach-exynos/cpuidle.c
>>> +++ b/arch/arm/mach-exynos/cpuidle.c
>>> @@ -17,30 +17,11 @@
>>>  #include <linux/time.h>
>>>  
>>>  #include <asm/proc-fns.h>
>>> -#include <asm/smp_scu.h>
>>> -#include <asm/suspend.h>
>>> -#include <asm/unified.h>
>>>  #include <asm/cpuidle.h>
>>>  #include <mach/regs-clock.h>
>>> -#include <mach/regs-pmu.h>
>>> -
>>> -#include <plat/cpu.h>
>>>  
>>>  #include "common.h"
>>>  
>>> -#define REG_DIRECTGO_ADDR	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
>>> -			S5P_INFORM7 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
>>> -			(S5P_VA_SYSRAM + 0x24) : S5P_INFORM0))
>>> -#define REG_DIRECTGO_FLAG	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
>>> -			S5P_INFORM6 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
>>> -			(S5P_VA_SYSRAM + 0x20) : S5P_INFORM1))
>>> -
>>> -#define S5P_CHECK_AFTR		0xFCBA0D10
>>> -
>>> -static int exynos4_enter_lowpower(struct cpuidle_device *dev,
>>> -				struct cpuidle_driver *drv,
>>> -				int index);
>>> -
>>>  static DEFINE_PER_CPU(struct cpuidle_device, exynos4_cpuidle_device);
>>>  
>>>  static struct cpuidle_driver exynos4_idle_driver = {
>>> @@ -48,117 +29,11 @@ static struct cpuidle_driver exynos4_idle_driver = {
>>>  	.owner			= THIS_MODULE,
>>>  	.states = {
>>>  		[0] = ARM_CPUIDLE_WFI_STATE,
>>> -		[1] = {
>>> -			.enter			= exynos4_enter_lowpower,
>>> -			.exit_latency		= 300,
>>> -			.target_residency	= 100000,
>>> -			.flags			= CPUIDLE_FLAG_TIME_VALID,
>>> -			.name			= "C1",
>>> -			.desc			= "ARM power down",
>>> -		},
>>>  	},
>>> -	.state_count = 2,
>>> +	.state_count = 1,
>>>  	.safe_state_index = 0,
>>>  };
>>>  
>>> -/* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */
>>> -static void exynos4_set_wakeupmask(void)
>>> -{
>>> -	__raw_writel(0x0000ff3e, S5P_WAKEUP_MASK);
>>> -}
>>> -
>>> -static unsigned int g_pwr_ctrl, g_diag_reg;
>>> -
>>> -static void save_cpu_arch_register(void)
>>> -{
>>> -	/*read power control register*/
>>> -	asm("mrc p15, 0, %0, c15, c0, 0" : "=r"(g_pwr_ctrl) : : "cc");
>>> -	/*read diagnostic register*/
>>> -	asm("mrc p15, 0, %0, c15, c0, 1" : "=r"(g_diag_reg) : : "cc");
>>> -	return;
>>> -}
>>> -
>>> -static void restore_cpu_arch_register(void)
>>> -{
>>> -	/*write power control register*/
>>> -	asm("mcr p15, 0, %0, c15, c0, 0" : : "r"(g_pwr_ctrl) : "cc");
>>> -	/*write diagnostic register*/
>>> -	asm("mcr p15, 0, %0, c15, c0, 1" : : "r"(g_diag_reg) : "cc");
>>> -	return;
>>> -}
>>> -
>>> -static int idle_finisher(unsigned long flags)
>>> -{
>>> -	cpu_do_idle();
>>> -	return 1;
>>> -}
>>> -
>>> -static int exynos4_enter_core0_aftr(struct cpuidle_device *dev,
>>> -				struct cpuidle_driver *drv,
>>> -				int index)
>>> -{
>>> -	unsigned long tmp;
>>> -
>>> -	exynos4_set_wakeupmask();
>>> -
>>> -	/* Set value of power down register for aftr mode */
>>> -	exynos_sys_powerdown_conf(SYS_AFTR);
>>> -
>>> -	__raw_writel(virt_to_phys(s3c_cpu_resume), REG_DIRECTGO_ADDR);
>>> -	__raw_writel(S5P_CHECK_AFTR, REG_DIRECTGO_FLAG);
>>> -
>>> -	save_cpu_arch_register();
>>> -
>>> -	/* Setting Central Sequence Register for power down mode */
>>> -	tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
>>> -	tmp &= ~S5P_CENTRAL_LOWPWR_CFG;
>>> -	__raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
>>> -
>>> -	cpu_pm_enter();
>>> -	cpu_suspend(0, idle_finisher);
>>> -
>>> -#ifdef CONFIG_SMP
>>> -	if (!soc_is_exynos5250())
>>> -		scu_enable(S5P_VA_SCU);
>>> -#endif
>>> -	cpu_pm_exit();
>>> -
>>> -	restore_cpu_arch_register();
>>> -
>>> -	/*
>>> -	 * If PMU failed while entering sleep mode, WFI will be
>>> -	 * ignored by PMU and then exiting cpu_do_idle().
>>> -	 * S5P_CENTRAL_LOWPWR_CFG bit will not be set automatically
>>> -	 * in this situation.
>>> -	 */
>>> -	tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
>>> -	if (!(tmp & S5P_CENTRAL_LOWPWR_CFG)) {
>>> -		tmp |= S5P_CENTRAL_LOWPWR_CFG;
>>> -		__raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
>>> -	}
>>> -
>>> -	/* Clear wakeup state register */
>>> -	__raw_writel(0x0, S5P_WAKEUP_STAT);
>>> -
>>> -	return index;
>>> -}
>>> -
>>> -static int exynos4_enter_lowpower(struct cpuidle_device *dev,
>>> -				struct cpuidle_driver *drv,
>>> -				int index)
>>> -{
>>> -	int new_index = index;
>>> -
>>> -	/* This mode only can be entered when other core's are offline */
>>> -	if (num_online_cpus() > 1)
>>> -		new_index = drv->safe_state_index;
>>> -
>>> -	if (new_index == 0)
>>> -		return arm_cpuidle_simple_enter(dev, drv, new_index);
>>> -	else
>>> -		return exynos4_enter_core0_aftr(dev, drv, new_index);
>>> -}
>>> -
>>>  static void __init exynos5_core_down_clk(void)
>>>  {
>>>  	unsigned int tmp;
>>> @@ -209,10 +84,6 @@ static int __init exynos4_init_cpuidle(void)
>>>  		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
>>>  		device->cpu = cpu_id;
>>>  
>>> -		/* Support IDLE only */
>>> -		if (cpu_id != 0)
>>> -			device->state_count = 1;
>>> -
>>>  		ret = cpuidle_register_device(device);
>>>  		if (ret) {
>>>  			printk(KERN_ERR "CPUidle register device failed\n");
> 


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

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

[PATCH 1/3] ARM: EXYNOS: remove non-working AFTR mode support

[Tomasz Figa]

Hi Daniel,

I've been fighting with this whole AFTR state as well, before Bartlomiej. 
Let me share my thoughts on this.

On Friday 28 of June 2013 11:57:25 Daniel Lezcano wrote:
> On 06/27/2013 08:10 PM, Bartlomiej Zolnierkiewicz wrote:
> > On Wednesday, June 26, 2013 12:36:12 PM Daniel Lezcano wrote:
> >> On 06/26/2013 12:13 PM, Bartlomiej Zolnierkiewicz wrote:
> >>> AFTR mode support was introduced by commit 67173ca ("ARM: EXYNOS: Add
> >>> support AFTR mode on EXYNOS4210") in v3.4 kernel.  Unfortunately even
> >>> in v3.4 kernel it hasn't worked as supposed and this is still the
> >>> case
> >>> with v3.10-rc6 (it probably wasn't noticed because CONFIG_CPU_IDLE is
> >>> not turned on by default):
> >>> 
> >>> - on revision 0 of Exynos4210 (Universal C210 board) it causes lockup
> >>> 
> >>>   (on this revision only one core is usable so entry to AFTR mode is
> >>>   always attempted because the code tries to go into AFTR mode when
> >>>   all
> >>>   other CPUs except CPU0 are offlined)
> >>> 
> >>> - on revision 1.1 of Exynos4210 (Trats board) it causes a lockup when
> >>> 
> >>>   CPU1 is offlined (i.e. echo 0 >
> >>>   /sys/devices/system/cpu/cpu1/online)
> >>> 
> >>> - on later Exynos4/5 SoCs wrong registers may be accessed when all
> >>> CPUs
> >>> 
> >>>   except CPU0 are offlined resulting in panic/lockup
> >>>   (REG_DIRECTGO_ADDR
> >>>   and REG_DIRECTGO_FLAG register selections was implemented only for
> >>>   Exynos4210)
> >>> 
> >>> Just remove AFTR mode support for now.
> >> 
> >> Ok, I will jump on the opportunity to talk about this state.
> >> 
> >> 1. I tried different ways to make the AFTR state to be entered with
> >> *both* cpu online. It goes successfully to this state. The CPU0 is
> >> correctly woken up but the CPU1 is never woken up, why is it happening
> >> ?
> >> 
> >> https://bugs.launchpad.net/linaro-landing-team-samsung/+bug/1171518
> > 
> > No idea here, AFTR doesn't work for me with upstream kernels even if
> > only one CPU is online.
> 
> What do you mean by "AFTR doesn't work" ? Is the kernel hanging ? The
> state is never reached ?

If you don't unplug all the CPUs >0 the state is obviously never reached. 
Otherwise the whole system hangs after it tries to enter this mode without 
any reaction for external events, other than reset.

> > Also the documentation says that before entering system-level
> > power-down
> > mode (such as AFTR) when multiple CPUs cores are used all other CPU
> > cores should stop interrupt service so I'm not sure how the way
> > attempted by you should work.
> 
> The cpu enters the idle mode with the interrupts disabled.

Hmm? What is supposed to wake it up then? AFAIK the whole idea of any idle 
or sleep is to sit in such low power mode until some interrupt fires (and so 
the name of the WFI, wait for interrupt, instruction).

> >> 2. The CPU1 hotplug bug should been fixed and if I am not wrong there
> >> is
> >> a patch somewhere fixing this.
> >> 
> >> https://bugs.launchpad.net/linaro-power-kernel/+bug/1171382
> > 
> > Unfortunately none of the patches there helps with my issues.
> > 
> >> 3. What is the fix for Exynos5 ?
> >> 
> >> https://bugs.launchpad.net/linaro-power-kernel/+bug/1171253
> >> 
> >> It sounds like depending on Hypervisor mode is enabled or not, the
> >> AFTR
> >> does not work correctly.
> > 
> > Sorry no idea here either.  On any SoCs later than EXYNOS4210 the
> > registers used for s3c_cpu_resume address and 0xFCBA0D10 magic number
> > may be different than EXYNOS4210 defaults (at least on EXYNOS4412 they
> > indeed are different, unfortunately I lack any info needed for EXYNOS5
> > support). You are lucky that it even works in some cases on EXYNOS5250.
> > 
> >> In other words, instead of removing the AFTR state I suggest to fix
> >> it:
> >> both core being online, split driver for exynos4 and 5.
> > 
> > My main problem is that with the upstream kernel even on EXYNOS4210
> > rev0 (only one core useable due to hardware issues) the kernel goes
> > into AFTR state for the first few times during boot and then it just
> > lockups (after going into cpu_do_idle() which is really
> > cpu_v7_do_idle()
> > and which does wfi call) and doesn't wake up CPU0. I have currently
> > no idea how to fix or debug it further.
> 
> I have an Origen 4210 board Ver A. and it works without problem with the
> AFTR mode (cpu1 unplugged).

Great!

Since benefits of this feature are rather questionable, especially when you 
consider all the maintenance burden caused by it, could you do some 
measurements to check if power saving thanks to this mode is of any 
significance?

> > The issue happens with every upstream kernel version tried (from v3.4
> > to v3.10-rc6).  Lockups also happen on EXYNOS4210 rev1.1 when CPU1 is
> > offlined by hand and then cpuidle driver tries to go into AFTR mode
> > (because by default it doesn't go into AFTR mode on any SoC except
> > EXYNOS4210 rev0).
> > 
> > I don't have EXYNOS4210 rev1.0 but it seems that in the upstream AFTR
> > mode has never worked (even on hardware that it was originally
> > developed
> > on) since its introduction in v3.4 (which was released on 20th May
> > 2012).
> > 
> > IOW for over the year nobody cared to make it work and I have currently
> > no fix at hand so the corrent upstream resolution is to just remove the
> > known non-working code and re-introduce it later when/if needed (I can
> > just disable it with a small fix but we don't keep such long-term
> > broken
> > code as placeholder in the upstream kernel).  If left as it is people
> > can hit the known issues and waste time debugging them, just like this
> > happenend for me [1].
> > 
> > If you have AFTR mode working (especially on EXYNOS4210) in Linaro
> > kernels please get fixes upstream ASAP. However I still wonder whether
> > the maintanance nightmare (bugs for different cases in your launchpad)
> > is worth gains over standard idle mode as the rumor around here is that
> > they are not that great (unfortunately no numbers were provided during
> > original feature addition).
> 
> It works forme with a vanilla kernel 3.10.0-rc7.

As Bartek already said, I haven't worked on any of our Exynos 4210 based 
boards since it got introduced in Linux 3.4, with exactly the same effect we 
described.

> Removing a feature because it seems not working is not a good solution.
> The right way is to investigate what is happening and why.

I can agree only partially. Keeping a feature that is broken and without 
any significant benefits does not make sense for me. Neither does putting 
efforts into fixing it, only to find that it is of no use.

However this is purely a speculation. Could you test on your Origen, on 
which it is supposed to work, if this feature is of any use?

Best regards,
Tomasz

> Thanks
>   -- Daniel
> 
> > --
> > Bartlomiej Zolnierkiewicz
> > Samsung R&D Institute Poland
> > Samsung Electronics
> > 
> > [1] Somebody enabled CONFIG_CPU_IDLE in the default config in one of
> > our
> > internal kernel trees on a target on which this feature "works" (since
> > CPUs are not hot-unplugged by default on all targets except EXYNOS4210
> > rev0 the code for AFTR is not used) and resulted in lockups on boot on
> > my default testing target. It took my long time to find out that
> > problem
> > is actually caused by just enabling exynos cpuidle driver.
> > 
> >> Thanks
> >> 
> >>   -- Daniel
> >>> 
> >>> Cc: Jaecheol Lee <jc.lee@samsung.com>
> >>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> >>> Cc: Amit Daniel Kachhap <amit.kachhap@linaro.org>
> >>> Cc: Tomasz Figa <t.figa@samsung.com>
> >>> Cc: Kukjin Kim <kgene.kim@samsung.com>
> >>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> >>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> >>> ---
> >>> 
> >>>  arch/arm/mach-exynos/cpuidle.c | 131
> >>>  +---------------------------------------- 1 file changed, 1
> >>>  insertion(+), 130 deletions(-)
> >>> 
> >>> diff --git a/arch/arm/mach-exynos/cpuidle.c
> >>> b/arch/arm/mach-exynos/cpuidle.c index 17a18ff..0a657ac 100644
> >>> --- a/arch/arm/mach-exynos/cpuidle.c
> >>> +++ b/arch/arm/mach-exynos/cpuidle.c
> >>> @@ -17,30 +17,11 @@
> >>> 
> >>>  #include <linux/time.h>
> >>>  
> >>>  #include <asm/proc-fns.h>
> >>> 
> >>> -#include <asm/smp_scu.h>
> >>> -#include <asm/suspend.h>
> >>> -#include <asm/unified.h>
> >>> 
> >>>  #include <asm/cpuidle.h>
> >>>  #include <mach/regs-clock.h>
> >>> 
> >>> -#include <mach/regs-pmu.h>
> >>> -
> >>> -#include <plat/cpu.h>
> >>> 
> >>>  #include "common.h"
> >>> 
> >>> -#define REG_DIRECTGO_ADDR	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
> >>> -			S5P_INFORM7 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
> >>> -			(S5P_VA_SYSRAM + 0x24) : S5P_INFORM0))
> >>> -#define REG_DIRECTGO_FLAG	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
> >>> -			S5P_INFORM6 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
> >>> -			(S5P_VA_SYSRAM + 0x20) : S5P_INFORM1))
> >>> -
> >>> -#define S5P_CHECK_AFTR		0xFCBA0D10
> >>> -
> >>> -static int exynos4_enter_lowpower(struct cpuidle_device *dev,
> >>> -				struct cpuidle_driver *drv,
> >>> -				int index);
> >>> -
> >>> 
> >>>  static DEFINE_PER_CPU(struct cpuidle_device,
> >>>  exynos4_cpuidle_device);
> >>>  
> >>>  static struct cpuidle_driver exynos4_idle_driver = {
> >>> 
> >>> @@ -48,117 +29,11 @@ static struct cpuidle_driver exynos4_idle_driver
> >>> = {>>> 
> >>>  	.owner			= THIS_MODULE,
> >>>  	.states = {
> >>>  	
> >>>  		[0] = ARM_CPUIDLE_WFI_STATE,
> >>> 
> >>> -		[1] = {
> >>> -			.enter			= exynos4_enter_lowpower,
> >>> -			.exit_latency		= 300,
> >>> -			.target_residency	= 100000,
> >>> -			.flags			= CPUIDLE_FLAG_TIME_VALID,
> >>> -			.name			= "C1",
> >>> -			.desc			= "ARM power down",
> >>> -		},
> >>> 
> >>>  	},
> >>> 
> >>> -	.state_count = 2,
> >>> +	.state_count = 1,
> >>> 
> >>>  	.safe_state_index = 0,
> >>>  
> >>>  };
> >>> 
> >>> -/* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */
> >>> -static void exynos4_set_wakeupmask(void)
> >>> -{
> >>> -	__raw_writel(0x0000ff3e, S5P_WAKEUP_MASK);
> >>> -}
> >>> -
> >>> -static unsigned int g_pwr_ctrl, g_diag_reg;
> >>> -
> >>> -static void save_cpu_arch_register(void)
> >>> -{
> >>> -	/*read power control register*/
> >>> -	asm("mrc p15, 0, %0, c15, c0, 0" : "=r"(g_pwr_ctrl) : : "cc");
> >>> -	/*read diagnostic register*/
> >>> -	asm("mrc p15, 0, %0, c15, c0, 1" : "=r"(g_diag_reg) : : "cc");
> >>> -	return;
> >>> -}
> >>> -
> >>> -static void restore_cpu_arch_register(void)
> >>> -{
> >>> -	/*write power control register*/
> >>> -	asm("mcr p15, 0, %0, c15, c0, 0" : : "r"(g_pwr_ctrl) : "cc");
> >>> -	/*write diagnostic register*/
> >>> -	asm("mcr p15, 0, %0, c15, c0, 1" : : "r"(g_diag_reg) : "cc");
> >>> -	return;
> >>> -}
> >>> -
> >>> -static int idle_finisher(unsigned long flags)
> >>> -{
> >>> -	cpu_do_idle();
> >>> -	return 1;
> >>> -}
> >>> -
> >>> -static int exynos4_enter_core0_aftr(struct cpuidle_device *dev,
> >>> -				struct cpuidle_driver *drv,
> >>> -				int index)
> >>> -{
> >>> -	unsigned long tmp;
> >>> -
> >>> -	exynos4_set_wakeupmask();
> >>> -
> >>> -	/* Set value of power down register for aftr mode */
> >>> -	exynos_sys_powerdown_conf(SYS_AFTR);
> >>> -
> >>> -	__raw_writel(virt_to_phys(s3c_cpu_resume), REG_DIRECTGO_ADDR);
> >>> -	__raw_writel(S5P_CHECK_AFTR, REG_DIRECTGO_FLAG);
> >>> -
> >>> -	save_cpu_arch_register();
> >>> -
> >>> -	/* Setting Central Sequence Register for power down mode */
> >>> -	tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
> >>> -	tmp &= ~S5P_CENTRAL_LOWPWR_CFG;
> >>> -	__raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
> >>> -
> >>> -	cpu_pm_enter();
> >>> -	cpu_suspend(0, idle_finisher);
> >>> -
> >>> -#ifdef CONFIG_SMP
> >>> -	if (!soc_is_exynos5250())
> >>> -		scu_enable(S5P_VA_SCU);
> >>> -#endif
> >>> -	cpu_pm_exit();
> >>> -
> >>> -	restore_cpu_arch_register();
> >>> -
> >>> -	/*
> >>> -	 * If PMU failed while entering sleep mode, WFI will be
> >>> -	 * ignored by PMU and then exiting cpu_do_idle().
> >>> -	 * S5P_CENTRAL_LOWPWR_CFG bit will not be set automatically
> >>> -	 * in this situation.
> >>> -	 */
> >>> -	tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
> >>> -	if (!(tmp & S5P_CENTRAL_LOWPWR_CFG)) {
> >>> -		tmp |= S5P_CENTRAL_LOWPWR_CFG;
> >>> -		__raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
> >>> -	}
> >>> -
> >>> -	/* Clear wakeup state register */
> >>> -	__raw_writel(0x0, S5P_WAKEUP_STAT);
> >>> -
> >>> -	return index;
> >>> -}
> >>> -
> >>> -static int exynos4_enter_lowpower(struct cpuidle_device *dev,
> >>> -				struct cpuidle_driver *drv,
> >>> -				int index)
> >>> -{
> >>> -	int new_index = index;
> >>> -
> >>> -	/* This mode only can be entered when other core's are offline */
> >>> -	if (num_online_cpus() > 1)
> >>> -		new_index = drv->safe_state_index;
> >>> -
> >>> -	if (new_index == 0)
> >>> -		return arm_cpuidle_simple_enter(dev, drv, new_index);
> >>> -	else
> >>> -		return exynos4_enter_core0_aftr(dev, drv, new_index);
> >>> -}
> >>> -
> >>> 
> >>>  static void __init exynos5_core_down_clk(void)
> >>>  {
> >>>  
> >>>  	unsigned int tmp;
> >>> 
> >>> @@ -209,10 +84,6 @@ static int __init exynos4_init_cpuidle(void)
> >>> 
> >>>  		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
> >>>  		device->cpu = cpu_id;
> >>> 
> >>> -		/* Support IDLE only */
> >>> -		if (cpu_id != 0)
> >>> -			device->state_count = 1;
> >>> -
> >>> 
> >>>  		ret = cpuidle_register_device(device);
> >>>  		if (ret) {
> >>>  		
> >>>  			printk(KERN_ERR "CPUidle register device failed\n");

[PATCH 1/3] ARM: EXYNOS: remove non-working AFTR mode support

[Lorenzo Pieralisi]

On Fri, Jun 28, 2013 at 11:11:24AM +0100, Tomasz Figa wrote:
> Hi Daniel,
> 
> I've been fighting with this whole AFTR state as well, before Bartlomiej.
> Let me share my thoughts on this.
> 
> On Friday 28 of June 2013 11:57:25 Daniel Lezcano wrote:
> > On 06/27/2013 08:10 PM, Bartlomiej Zolnierkiewicz wrote:
> > > On Wednesday, June 26, 2013 12:36:12 PM Daniel Lezcano wrote:
> > >> On 06/26/2013 12:13 PM, Bartlomiej Zolnierkiewicz wrote:
> > >>> AFTR mode support was introduced by commit 67173ca ("ARM: EXYNOS: Add
> > >>> support AFTR mode on EXYNOS4210") in v3.4 kernel.  Unfortunately even
> > >>> in v3.4 kernel it hasn't worked as supposed and this is still the
> > >>> case
> > >>> with v3.10-rc6 (it probably wasn't noticed because CONFIG_CPU_IDLE is
> > >>> not turned on by default):
> > >>>
> > >>> - on revision 0 of Exynos4210 (Universal C210 board) it causes lockup
> > >>>
> > >>>   (on this revision only one core is usable so entry to AFTR mode is
> > >>>   always attempted because the code tries to go into AFTR mode when
> > >>>   all
> > >>>   other CPUs except CPU0 are offlined)
> > >>>
> > >>> - on revision 1.1 of Exynos4210 (Trats board) it causes a lockup when
> > >>>
> > >>>   CPU1 is offlined (i.e. echo 0 >
> > >>>   /sys/devices/system/cpu/cpu1/online)
> > >>>
> > >>> - on later Exynos4/5 SoCs wrong registers may be accessed when all
> > >>> CPUs
> > >>>
> > >>>   except CPU0 are offlined resulting in panic/lockup
> > >>>   (REG_DIRECTGO_ADDR
> > >>>   and REG_DIRECTGO_FLAG register selections was implemented only for
> > >>>   Exynos4210)
> > >>>
> > >>> Just remove AFTR mode support for now.
> > >>
> > >> Ok, I will jump on the opportunity to talk about this state.
> > >>
> > >> 1. I tried different ways to make the AFTR state to be entered with
> > >> *both* cpu online. It goes successfully to this state. The CPU0 is
> > >> correctly woken up but the CPU1 is never woken up, why is it happening
> > >> ?
> > >>
> > >> https://bugs.launchpad.net/linaro-landing-team-samsung/+bug/1171518
> > >
> > > No idea here, AFTR doesn't work for me with upstream kernels even if
> > > only one CPU is online.
> >
> > What do you mean by "AFTR doesn't work" ? Is the kernel hanging ? The
> > state is never reached ?
> 
> If you don't unplug all the CPUs >0 the state is obviously never reached.
> Otherwise the whole system hangs after it tries to enter this mode without
> any reaction for external events, other than reset.
> 
> > > Also the documentation says that before entering system-level
> > > power-down
> > > mode (such as AFTR) when multiple CPUs cores are used all other CPU
> > > cores should stop interrupt service so I'm not sure how the way
> > > attempted by you should work.
> >
> > The cpu enters the idle mode with the interrupts disabled.
> 
> Hmm? What is supposed to wake it up then? AFAIK the whole idea of any idle
> or sleep is to sit in such low power mode until some interrupt fires (and so
> the name of the WFI, wait for interrupt, instruction).

WFI completes upon IRQ even if irqs are disabled for a CPU, unless the
GIC CPU interface is disabled for that CPU as well. How the power
controller wakes up the CPUs from shutdown is strictly related to the
power controller behaviour. The problem here I think is completely different
and it is related to how the power controller behaves on Exynos.
I think the issues are a combination of power controller/boot firmware that
prevents CPU1 to go into idle independently of CPU0, but I might be mistaken.

If you want to get to the bottom of this you need to get detailed information
on how the firmware and power controller behave, trial and error won't work.

> > >> 2. The CPU1 hotplug bug should been fixed and if I am not wrong there
> > >> is
> > >> a patch somewhere fixing this.
> > >>
> > >> https://bugs.launchpad.net/linaro-power-kernel/+bug/1171382
> > >
> > > Unfortunately none of the patches there helps with my issues.
> > >
> > >> 3. What is the fix for Exynos5 ?
> > >>
> > >> https://bugs.launchpad.net/linaro-power-kernel/+bug/1171253
> > >>
> > >> It sounds like depending on Hypervisor mode is enabled or not, the
> > >> AFTR
> > >> does not work correctly.
> > >
> > > Sorry no idea here either.  On any SoCs later than EXYNOS4210 the
> > > registers used for s3c_cpu_resume address and 0xFCBA0D10 magic number
> > > may be different than EXYNOS4210 defaults (at least on EXYNOS4412 they
> > > indeed are different, unfortunately I lack any info needed for EXYNOS5
> > > support). You are lucky that it even works in some cases on EXYNOS5250.
> > >
> > >> In other words, instead of removing the AFTR state I suggest to fix
> > >> it:
> > >> both core being online, split driver for exynos4 and 5.
> > >
> > > My main problem is that with the upstream kernel even on EXYNOS4210
> > > rev0 (only one core useable due to hardware issues) the kernel goes
> > > into AFTR state for the first few times during boot and then it just
> > > lockups (after going into cpu_do_idle() which is really
> > > cpu_v7_do_idle()
> > > and which does wfi call) and doesn't wake up CPU0. I have currently
> > > no idea how to fix or debug it further.
> >
> > I have an Origen 4210 board Ver A. and it works without problem with the
> > AFTR mode (cpu1 unplugged).
> 
> Great!
> 
> Since benefits of this feature are rather questionable, especially when you
> consider all the maintenance burden caused by it, could you do some
> measurements to check if power saving thanks to this mode is of any
> significance?

Questionable ? Questionable is the way this code has been put on the
backburner without upstreaming it properly.

> > > The issue happens with every upstream kernel version tried (from v3.4
> > > to v3.10-rc6).  Lockups also happen on EXYNOS4210 rev1.1 when CPU1 is
> > > offlined by hand and then cpuidle driver tries to go into AFTR mode
> > > (because by default it doesn't go into AFTR mode on any SoC except
> > > EXYNOS4210 rev0).
> > >
> > > I don't have EXYNOS4210 rev1.0 but it seems that in the upstream AFTR
> > > mode has never worked (even on hardware that it was originally
> > > developed
> > > on) since its introduction in v3.4 (which was released on 20th May
> > > 2012).
> > >
> > > IOW for over the year nobody cared to make it work and I have currently
> > > no fix at hand so the corrent upstream resolution is to just remove the
> > > known non-working code and re-introduce it later when/if needed (I can
> > > just disable it with a small fix but we don't keep such long-term
> > > broken
> > > code as placeholder in the upstream kernel).  If left as it is people
> > > can hit the known issues and waste time debugging them, just like this
> > > happenend for me [1].
> > >
> > > If you have AFTR mode working (especially on EXYNOS4210) in Linaro
> > > kernels please get fixes upstream ASAP. However I still wonder whether
> > > the maintanance nightmare (bugs for different cases in your launchpad)
> > > is worth gains over standard idle mode as the rumor around here is that
> > > they are not that great (unfortunately no numbers were provided during
> > > original feature addition).
> >
> > It works forme with a vanilla kernel 3.10.0-rc7.
> 
> As Bartek already said, I haven't worked on any of our Exynos 4210 based
> boards since it got introduced in Linux 3.4, with exactly the same effect we
> described.
> 
> > Removing a feature because it seems not working is not a good solution.
> > The right way is to investigate what is happening and why.
> 
> I can agree only partially. Keeping a feature that is broken and without
> any significant benefits does not make sense for me. Neither does putting
> efforts into fixing it, only to find that it is of no use.

Well, either power management HW works on these boards, so there are
significant benefits in fixing the code, or it does not and you are right
then.

If power management HW works there are benefits and they are significant for
manifold reasons (first and foremost, since these are dev boards, to show
how power management backends must be written, but honestly this driver
fails in that respect). It is absolutely worth putting effort in fixing the
behaviour, the problem is finding people who can answer questions on
how the power controller and firmware are *supposed* to be working,
otherwise we will never get to the bottom of this.

I understand that these bits of information are hard to come by, and for
that reason we might be forced to drop this code, but this does not mean
that these features are not useful.

> However this is purely a speculation. Could you test on your Origen, on
> which it is supposed to work, if this feature is of any use ?

If power management HW works properly on these boards, and I hope it
does (but the only way to check it is by measuring power drawn), this feature
is useful, now the question is to understand how and who is going to fix it,
if we can manage to do that.

Thanks,
Lorenzo

[PATCH 1/3] ARM: EXYNOS: remove non-working AFTR mode support

[Tomasz Figa]

Hi Lorenzo,

On Friday 28 of June 2013 12:13:33 Lorenzo Pieralisi wrote:
> On Fri, Jun 28, 2013 at 11:11:24AM +0100, Tomasz Figa wrote:
> > Hi Daniel,
> > 
> > I've been fighting with this whole AFTR state as well, before
> > Bartlomiej. Let me share my thoughts on this.
> > 
> > On Friday 28 of June 2013 11:57:25 Daniel Lezcano wrote:
> > > On 06/27/2013 08:10 PM, Bartlomiej Zolnierkiewicz wrote:
> > > > On Wednesday, June 26, 2013 12:36:12 PM Daniel Lezcano wrote:
> > > >> On 06/26/2013 12:13 PM, Bartlomiej Zolnierkiewicz wrote:
> > > >>> AFTR mode support was introduced by commit 67173ca ("ARM: EXYNOS:
> > > >>> Add
> > > >>> support AFTR mode on EXYNOS4210") in v3.4 kernel.  Unfortunately
> > > >>> even
> > > >>> in v3.4 kernel it hasn't worked as supposed and this is still the
> > > >>> case
> > > >>> with v3.10-rc6 (it probably wasn't noticed because
> > > >>> CONFIG_CPU_IDLE is
> > > >>> not turned on by default):
> > > >>> 
> > > >>> - on revision 0 of Exynos4210 (Universal C210 board) it causes
> > > >>> lockup
> > > >>> 
> > > >>>   (on this revision only one core is usable so entry to AFTR mode
> > > >>>   is
> > > >>>   always attempted because the code tries to go into AFTR mode
> > > >>>   when
> > > >>>   all
> > > >>>   other CPUs except CPU0 are offlined)
> > > >>> 
> > > >>> - on revision 1.1 of Exynos4210 (Trats board) it causes a lockup
> > > >>> when
> > > >>> 
> > > >>>   CPU1 is offlined (i.e. echo 0 >
> > > >>>   /sys/devices/system/cpu/cpu1/online)
> > > >>> 
> > > >>> - on later Exynos4/5 SoCs wrong registers may be accessed when
> > > >>> all
> > > >>> CPUs
> > > >>> 
> > > >>>   except CPU0 are offlined resulting in panic/lockup
> > > >>>   (REG_DIRECTGO_ADDR
> > > >>>   and REG_DIRECTGO_FLAG register selections was implemented only
> > > >>>   for
> > > >>>   Exynos4210)
> > > >>> 
> > > >>> Just remove AFTR mode support for now.
> > > >> 
> > > >> Ok, I will jump on the opportunity to talk about this state.
> > > >> 
> > > >> 1. I tried different ways to make the AFTR state to be entered
> > > >> with
> > > >> *both* cpu online. It goes successfully to this state. The CPU0 is
> > > >> correctly woken up but the CPU1 is never woken up, why is it
> > > >> happening
> > > >> ?
> > > >> 
> > > >> https://bugs.launchpad.net/linaro-landing-team-samsung/+bug/117151
> > > >> 8
> > > > 
> > > > No idea here, AFTR doesn't work for me with upstream kernels even
> > > > if
> > > > only one CPU is online.
> > > 
> > > What do you mean by "AFTR doesn't work" ? Is the kernel hanging ? The
> > > state is never reached ?
> > 
> > If you don't unplug all the CPUs >0 the state is obviously never
> > reached. Otherwise the whole system hangs after it tries to enter this
> > mode without any reaction for external events, other than reset.
> > 
> > > > Also the documentation says that before entering system-level
> > > > power-down
> > > > mode (such as AFTR) when multiple CPUs cores are used all other CPU
> > > > cores should stop interrupt service so I'm not sure how the way
> > > > attempted by you should work.
> > > 
> > > The cpu enters the idle mode with the interrupts disabled.
> > 
> > Hmm? What is supposed to wake it up then? AFAIK the whole idea of any
> > idle or sleep is to sit in such low power mode until some interrupt
> > fires (and so the name of the WFI, wait for interrupt, instruction).
> 
> WFI completes upon IRQ even if irqs are disabled for a CPU, unless the
> GIC CPU interface is disabled for that CPU as well. How the power
> controller wakes up the CPUs from shutdown is strictly related to the
> power controller behaviour. The problem here I think is completely
> different and it is related to how the power controller behaves on
> Exynos. I think the issues are a combination of power controller/boot
> firmware that prevents CPU1 to go into idle independently of CPU0, but I
> might be mistaken.

We are not talking here about the normal WFI idle mode. This one works 
correctly and gives huge power savings.

AFTR (which stands for ARM off, TOP running, TOP being a power domain) is a 
mode that is supposed to be deeper than WFI, with all ARM cores powered off, 
but some of the blocks around them keep their state, so the whole system 
can be quickly woke up.

Based on our measurements on Exynos 4210 and 4x12, powering off ARM cores 
don't give any significant amount of power savings (few milliamperes per 
core), with most of the savings already achieved by WFI idle. Since AFTR is 
basically just powering off all ARM cores, including CPU0, I doubt it is 
going to give much higher savings than that.

> If you want to get to the bottom of this you need to get detailed
> information on how the firmware and power controller behave, trial and
> error won't work.

I don't think we want to put unjustified effort into this. If we are to fix 
it, we should first make sure that it is something that is worth this effort.

The commit that introduced this feature didn't have any rationale to begin 
with, not even saying about any evidence of actual power saving such as 
measurements.

Just for reference, here's commit message of that commit:

	ARM: EXYNOS: Add support AFTR mode on EXYNOS4210

	This patch adds support AFTR(ARM OFF TOP RUNNING) mode in
	cpuidle driver. L2 cache keeps their data in this mode.
	This patch ports the code to the latest interfaces to
	save/restore CPU state inclusive of CPU PM notifiers, l2
	resume and cpu_suspend/resume.

Nothing saying _why_ this is added, just _what_ is added.

> > > >> 2. The CPU1 hotplug bug should been fixed and if I am not wrong
> > > >> there
> > > >> is
> > > >> a patch somewhere fixing this.
> > > >> 
> > > >> https://bugs.launchpad.net/linaro-power-kernel/+bug/1171382
> > > > 
> > > > Unfortunately none of the patches there helps with my issues.
> > > > 
> > > >> 3. What is the fix for Exynos5 ?
> > > >> 
> > > >> https://bugs.launchpad.net/linaro-power-kernel/+bug/1171253
> > > >> 
> > > >> It sounds like depending on Hypervisor mode is enabled or not, the
> > > >> AFTR
> > > >> does not work correctly.
> > > > 
> > > > Sorry no idea here either.  On any SoCs later than EXYNOS4210 the
> > > > registers used for s3c_cpu_resume address and 0xFCBA0D10 magic
> > > > number
> > > > may be different than EXYNOS4210 defaults (at least on EXYNOS4412
> > > > they
> > > > indeed are different, unfortunately I lack any info needed for
> > > > EXYNOS5
> > > > support). You are lucky that it even works in some cases on
> > > > EXYNOS5250.
> > > > 
> > > >> In other words, instead of removing the AFTR state I suggest to
> > > >> fix
> > > >> it:
> > > >> both core being online, split driver for exynos4 and 5.
> > > > 
> > > > My main problem is that with the upstream kernel even on EXYNOS4210
> > > > rev0 (only one core useable due to hardware issues) the kernel goes
> > > > into AFTR state for the first few times during boot and then it
> > > > just
> > > > lockups (after going into cpu_do_idle() which is really
> > > > cpu_v7_do_idle()
> > > > and which does wfi call) and doesn't wake up CPU0. I have currently
> > > > no idea how to fix or debug it further.
> > > 
> > > I have an Origen 4210 board Ver A. and it works without problem with
> > > the
> > > AFTR mode (cpu1 unplugged).
> > 
> > Great!
> > 
> > Since benefits of this feature are rather questionable, especially when
> > you consider all the maintenance burden caused by it, could you do
> > some measurements to check if power saving thanks to this mode is of
> > any significance?
> 
> Questionable ? Questionable is the way this code has been put on the
> backburner without upstreaming it properly.

I agree, it has not been upstreamed properly, becausE:
 1) it doesn't work,
 2) there was no rationale behind it or at least none was provided in patch 
descriptions.

> > > > The issue happens with every upstream kernel version tried (from
> > > > v3.4
> > > > to v3.10-rc6).  Lockups also happen on EXYNOS4210 rev1.1 when CPU1
> > > > is
> > > > offlined by hand and then cpuidle driver tries to go into AFTR mode
> > > > (because by default it doesn't go into AFTR mode on any SoC except
> > > > EXYNOS4210 rev0).
> > > > 
> > > > I don't have EXYNOS4210 rev1.0 but it seems that in the upstream
> > > > AFTR
> > > > mode has never worked (even on hardware that it was originally
> > > > developed
> > > > on) since its introduction in v3.4 (which was released on 20th May
> > > > 2012).
> > > > 
> > > > IOW for over the year nobody cared to make it work and I have
> > > > currently
> > > > no fix at hand so the corrent upstream resolution is to just remove
> > > > the
> > > > known non-working code and re-introduce it later when/if needed (I
> > > > can
> > > > just disable it with a small fix but we don't keep such long-term
> > > > broken
> > > > code as placeholder in the upstream kernel).  If left as it is
> > > > people
> > > > can hit the known issues and waste time debugging them, just like
> > > > this
> > > > happenend for me [1].
> > > > 
> > > > If you have AFTR mode working (especially on EXYNOS4210) in Linaro
> > > > kernels please get fixes upstream ASAP. However I still wonder
> > > > whether
> > > > the maintanance nightmare (bugs for different cases in your
> > > > launchpad)
> > > > is worth gains over standard idle mode as the rumor around here is
> > > > that
> > > > they are not that great (unfortunately no numbers were provided
> > > > during
> > > > original feature addition).
> > > 
> > > It works forme with a vanilla kernel 3.10.0-rc7.
> > 
> > As Bartek already said, I haven't worked on any of our Exynos 4210
> > based
> > boards since it got introduced in Linux 3.4, with exactly the same
> > effect we described.
> > 
> > > Removing a feature because it seems not working is not a good
> > > solution.
> > > The right way is to investigate what is happening and why.
> > 
> > I can agree only partially. Keeping a feature that is broken and
> > without
> > any significant benefits does not make sense for me. Neither does
> > putting efforts into fixing it, only to find that it is of no use.
> 
> Well, either power management HW works on these boards, so there are
> significant benefits in fixing the code, or it does not and you are right
> then.

This mode doesn't have anything to do with power management HW other than 
power management unit inside of the SoC, which is not board specific.

> If power management HW works there are benefits and they are significant
> for manifold reasons (first and foremost, since these are dev boards, to
> show how power management backends must be written, but honestly this
> driver fails in that respect). It is absolutely worth putting effort in
> fixing the behaviour, the problem is finding people who can answer
> questions on how the power controller and firmware are *supposed* to be
> working, otherwise we will never get to the bottom of this.

I don't see a point to have code that works, but doesn't do anything useful 
(other than some example/skeleton drivers which aren't supposed to be used 
in other way than as aid for developers). This is why I'd like to see some 
evidence that this feature is good to have, before we start putting effort 
into fixing it on other boards (because this is possibly firmware/bootloader 
specific issue).

> I understand that these bits of information are hard to come by, and for
> that reason we might be forced to drop this code, but this does not mean
> that these features are not useful.

I agree. This is not what I meant. What I wanted to say is that no 
rationale has ever been provided for this feature and so we don't know if 
it is worth putting effort into it.

Our internal measurements of similar features tell us that it is unlikely, 
but this is not a result that can be relied on, so I asked for measurements 
on boards for which it works.

> > However this is purely a speculation. Could you test on your Origen, on
> > which it is supposed to work, if this feature is of any use ?
> 
> If power management HW works properly on these boards, and I hope it
> does (but the only way to check it is by measuring power drawn), this
> feature is useful, now the question is to understand how and who is
> going to fix it, if we can manage to do that.

Again, this feature is only related to SoC internals and lowest level 
firmware (first stage bootloader, which resumes the CPU), so power saving is 
not board specific here, but SoC-specific.

Best regards,
Tomasz

[PATCH 1/3] ARM: EXYNOS: remove non-working AFTR mode support

[Daniel Lezcano]

On 06/28/2013 12:11 PM, Tomasz Figa wrote:
> Hi Daniel,
> 
> I've been fighting with this whole AFTR state as well, before Bartlomiej. 
> Let me share my thoughts on this.
> 
> On Friday 28 of June 2013 11:57:25 Daniel Lezcano wrote:
>> On 06/27/2013 08:10 PM, Bartlomiej Zolnierkiewicz wrote:
>>> On Wednesday, June 26, 2013 12:36:12 PM Daniel Lezcano wrote:
>>>> On 06/26/2013 12:13 PM, Bartlomiej Zolnierkiewicz wrote:
>>>>> AFTR mode support was introduced by commit 67173ca ("ARM: EXYNOS: Add
>>>>> support AFTR mode on EXYNOS4210") in v3.4 kernel.  Unfortunately even
>>>>> in v3.4 kernel it hasn't worked as supposed and this is still the
>>>>> case
>>>>> with v3.10-rc6 (it probably wasn't noticed because CONFIG_CPU_IDLE is
>>>>> not turned on by default):
>>>>>
>>>>> - on revision 0 of Exynos4210 (Universal C210 board) it causes lockup
>>>>>
>>>>>   (on this revision only one core is usable so entry to AFTR mode is
>>>>>   always attempted because the code tries to go into AFTR mode when
>>>>>   all
>>>>>   other CPUs except CPU0 are offlined)
>>>>>
>>>>> - on revision 1.1 of Exynos4210 (Trats board) it causes a lockup when
>>>>>
>>>>>   CPU1 is offlined (i.e. echo 0 >
>>>>>   /sys/devices/system/cpu/cpu1/online)
>>>>>
>>>>> - on later Exynos4/5 SoCs wrong registers may be accessed when all
>>>>> CPUs
>>>>>
>>>>>   except CPU0 are offlined resulting in panic/lockup
>>>>>   (REG_DIRECTGO_ADDR
>>>>>   and REG_DIRECTGO_FLAG register selections was implemented only for
>>>>>   Exynos4210)
>>>>>
>>>>> Just remove AFTR mode support for now.
>>>>
>>>> Ok, I will jump on the opportunity to talk about this state.
>>>>
>>>> 1. I tried different ways to make the AFTR state to be entered with
>>>> *both* cpu online. It goes successfully to this state. The CPU0 is
>>>> correctly woken up but the CPU1 is never woken up, why is it happening
>>>> ?
>>>>
>>>> https://bugs.launchpad.net/linaro-landing-team-samsung/+bug/1171518
>>>
>>> No idea here, AFTR doesn't work for me with upstream kernels even if
>>> only one CPU is online.
>>
>> What do you mean by "AFTR doesn't work" ? Is the kernel hanging ? The
>> state is never reached ?
> 
> If you don't unplug all the CPUs >0 the state is obviously never reached. 
> Otherwise the whole system hangs after it tries to enter this mode without 
> any reaction for external events, other than reset.

Need investigation.

What is the exynos board version where that occurs ?

>>> Also the documentation says that before entering system-level
>>> power-down
>>> mode (such as AFTR) when multiple CPUs cores are used all other CPU
>>> cores should stop interrupt service so I'm not sure how the way
>>> attempted by you should work.
>>
>> The cpu enters the idle mode with the interrupts disabled.
> 
> Hmm? What is supposed to wake it up then? AFAIK the whole idea of any idle 
> or sleep is to sit in such low power mode until some interrupt fires (and so 
> the name of the WFI, wait for interrupt, instruction).

It is handled by the hardware, for the exynos it should be the PMU. The
CPU stays clock/power gated and when an interrupt occurs the PMU wakes
up the CPU. This one continue its instructions after cpu_do_idle and
right after enables the local interrupts leading to the interrupt handling.

>>>> 2. The CPU1 hotplug bug should been fixed and if I am not wrong there
>>>> is
>>>> a patch somewhere fixing this.
>>>>
>>>> https://bugs.launchpad.net/linaro-power-kernel/+bug/1171382
>>>
>>> Unfortunately none of the patches there helps with my issues.
>>>
>>>> 3. What is the fix for Exynos5 ?
>>>>
>>>> https://bugs.launchpad.net/linaro-power-kernel/+bug/1171253
>>>>
>>>> It sounds like depending on Hypervisor mode is enabled or not, the
>>>> AFTR
>>>> does not work correctly.
>>>
>>> Sorry no idea here either.  On any SoCs later than EXYNOS4210 the
>>> registers used for s3c_cpu_resume address and 0xFCBA0D10 magic number
>>> may be different than EXYNOS4210 defaults (at least on EXYNOS4412 they
>>> indeed are different, unfortunately I lack any info needed for EXYNOS5
>>> support). You are lucky that it even works in some cases on EXYNOS5250.
>>>
>>>> In other words, instead of removing the AFTR state I suggest to fix
>>>> it:
>>>> both core being online, split driver for exynos4 and 5.
>>>
>>> My main problem is that with the upstream kernel even on EXYNOS4210
>>> rev0 (only one core useable due to hardware issues) the kernel goes
>>> into AFTR state for the first few times during boot and then it just
>>> lockups (after going into cpu_do_idle() which is really
>>> cpu_v7_do_idle()
>>> and which does wfi call) and doesn't wake up CPU0. I have currently
>>> no idea how to fix or debug it further.
>>
>> I have an Origen 4210 board Ver A. and it works without problem with the
>> AFTR mode (cpu1 unplugged).
> 
> Great!
> 
> Since benefits of this feature are rather questionable, especially when you 
> consider all the maintenance burden caused by it, could you do some 
> measurements to check if power saving thanks to this mode is of any 
> significance?

No I can't, no spare time for that and furthermore this work has already
be done by Amit Daniel when he submitted the driver.

Amit Daniel is no longer a Linaro assignee but it is still part of the
Samsung company (changed the email address to reach him).

>>> The issue happens with every upstream kernel version tried (from v3.4
>>> to v3.10-rc6).  Lockups also happen on EXYNOS4210 rev1.1 when CPU1 is
>>> offlined by hand and then cpuidle driver tries to go into AFTR mode
>>> (because by default it doesn't go into AFTR mode on any SoC except
>>> EXYNOS4210 rev0).
>>>
>>> I don't have EXYNOS4210 rev1.0 but it seems that in the upstream AFTR
>>> mode has never worked (even on hardware that it was originally
>>> developed
>>> on) since its introduction in v3.4 (which was released on 20th May
>>> 2012).
>>>
>>> IOW for over the year nobody cared to make it work and I have currently
>>> no fix at hand so the corrent upstream resolution is to just remove the
>>> known non-working code and re-introduce it later when/if needed (I can
>>> just disable it with a small fix but we don't keep such long-term
>>> broken
>>> code as placeholder in the upstream kernel).  If left as it is people
>>> can hit the known issues and waste time debugging them, just like this
>>> happenend for me [1].
>>>
>>> If you have AFTR mode working (especially on EXYNOS4210) in Linaro
>>> kernels please get fixes upstream ASAP. However I still wonder whether
>>> the maintanance nightmare (bugs for different cases in your launchpad)
>>> is worth gains over standard idle mode as the rumor around here is that
>>> they are not that great (unfortunately no numbers were provided during
>>> original feature addition).
>>
>> It works forme with a vanilla kernel 3.10.0-rc7.
> 
> As Bartek already said, I haven't worked on any of our Exynos 4210 based 
> boards since it got introduced in Linux 3.4, with exactly the same effect we 
> described.
> 
>> Removing a feature because it seems not working is not a good solution.
>> The right way is to investigate what is happening and why.
> 
> I can agree only partially. Keeping a feature that is broken and without 
> any significant benefits does not make sense for me. Neither does putting 
> efforts into fixing it, only to find that it is of no use.
> 
> However this is purely a speculation. Could you test on your Origen, on 
> which it is supposed to work, if this feature is of any use?

It is useless to do that. This work is already done.

The kernel is not a playground where you can upstream code and then
remove it because a feature seems broken and you don't have an idea of why.

I asked several times the reasons of why the AFTR state couldn't work
with multiple CPUs and I had no answer.

Frankly speaking I have a couple of hypothesis:

1. something is not correctly setup and the PMU does not wake up the CPU1
2. there is a silicon bug and no one wants to tell it is the case

In any case, this must be investigated and identified. And then we can
take a decision about this state.

>>> --
>>> Bartlomiej Zolnierkiewicz
>>> Samsung R&D Institute Poland
>>> Samsung Electronics
>>>
>>> [1] Somebody enabled CONFIG_CPU_IDLE in the default config in one of
>>> our
>>> internal kernel trees on a target on which this feature "works" (since
>>> CPUs are not hot-unplugged by default on all targets except EXYNOS4210
>>> rev0 the code for AFTR is not used) and resulted in lockups on boot on
>>> my default testing target. It took my long time to find out that
>>> problem
>>> is actually caused by just enabling exynos cpuidle driver.
>>>
>>>> Thanks
>>>>
>>>>   -- Daniel
>>>>>
>>>>> Cc: Jaecheol Lee <jc.lee@samsung.com>
>>>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>>>>> Cc: Amit Daniel Kachhap <amit.kachhap@linaro.org>
>>>>> Cc: Tomasz Figa <t.figa@samsung.com>
>>>>> Cc: Kukjin Kim <kgene.kim@samsung.com>
>>>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
>>>>> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
>>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
>>>>> ---
>>>>>
>>>>>  arch/arm/mach-exynos/cpuidle.c | 131
>>>>>  +---------------------------------------- 1 file changed, 1
>>>>>  insertion(+), 130 deletions(-)
>>>>>
>>>>> diff --git a/arch/arm/mach-exynos/cpuidle.c
>>>>> b/arch/arm/mach-exynos/cpuidle.c index 17a18ff..0a657ac 100644
>>>>> --- a/arch/arm/mach-exynos/cpuidle.c
>>>>> +++ b/arch/arm/mach-exynos/cpuidle.c
>>>>> @@ -17,30 +17,11 @@
>>>>>
>>>>>  #include <linux/time.h>
>>>>>  
>>>>>  #include <asm/proc-fns.h>
>>>>>
>>>>> -#include <asm/smp_scu.h>
>>>>> -#include <asm/suspend.h>
>>>>> -#include <asm/unified.h>
>>>>>
>>>>>  #include <asm/cpuidle.h>
>>>>>  #include <mach/regs-clock.h>
>>>>>
>>>>> -#include <mach/regs-pmu.h>
>>>>> -
>>>>> -#include <plat/cpu.h>
>>>>>
>>>>>  #include "common.h"
>>>>>
>>>>> -#define REG_DIRECTGO_ADDR	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
>>>>> -			S5P_INFORM7 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
>>>>> -			(S5P_VA_SYSRAM + 0x24) : S5P_INFORM0))
>>>>> -#define REG_DIRECTGO_FLAG	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
>>>>> -			S5P_INFORM6 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
>>>>> -			(S5P_VA_SYSRAM + 0x20) : S5P_INFORM1))
>>>>> -
>>>>> -#define S5P_CHECK_AFTR		0xFCBA0D10
>>>>> -
>>>>> -static int exynos4_enter_lowpower(struct cpuidle_device *dev,
>>>>> -				struct cpuidle_driver *drv,
>>>>> -				int index);
>>>>> -
>>>>>
>>>>>  static DEFINE_PER_CPU(struct cpuidle_device,
>>>>>  exynos4_cpuidle_device);
>>>>>  
>>>>>  static struct cpuidle_driver exynos4_idle_driver = {
>>>>>
>>>>> @@ -48,117 +29,11 @@ static struct cpuidle_driver exynos4_idle_driver
>>>>> = {>>> 
>>>>>  	.owner			= THIS_MODULE,
>>>>>  	.states = {
>>>>>  	
>>>>>  		[0] = ARM_CPUIDLE_WFI_STATE,
>>>>>
>>>>> -		[1] = {
>>>>> -			.enter			= exynos4_enter_lowpower,
>>>>> -			.exit_latency		= 300,
>>>>> -			.target_residency	= 100000,
>>>>> -			.flags			= CPUIDLE_FLAG_TIME_VALID,
>>>>> -			.name			= "C1",
>>>>> -			.desc			= "ARM power down",
>>>>> -		},
>>>>>
>>>>>  	},
>>>>>
>>>>> -	.state_count = 2,
>>>>> +	.state_count = 1,
>>>>>
>>>>>  	.safe_state_index = 0,
>>>>>  
>>>>>  };
>>>>>
>>>>> -/* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */
>>>>> -static void exynos4_set_wakeupmask(void)
>>>>> -{
>>>>> -	__raw_writel(0x0000ff3e, S5P_WAKEUP_MASK);
>>>>> -}
>>>>> -
>>>>> -static unsigned int g_pwr_ctrl, g_diag_reg;
>>>>> -
>>>>> -static void save_cpu_arch_register(void)
>>>>> -{
>>>>> -	/*read power control register*/
>>>>> -	asm("mrc p15, 0, %0, c15, c0, 0" : "=r"(g_pwr_ctrl) : : "cc");
>>>>> -	/*read diagnostic register*/
>>>>> -	asm("mrc p15, 0, %0, c15, c0, 1" : "=r"(g_diag_reg) : : "cc");
>>>>> -	return;
>>>>> -}
>>>>> -
>>>>> -static void restore_cpu_arch_register(void)
>>>>> -{
>>>>> -	/*write power control register*/
>>>>> -	asm("mcr p15, 0, %0, c15, c0, 0" : : "r"(g_pwr_ctrl) : "cc");
>>>>> -	/*write diagnostic register*/
>>>>> -	asm("mcr p15, 0, %0, c15, c0, 1" : : "r"(g_diag_reg) : "cc");
>>>>> -	return;
>>>>> -}
>>>>> -
>>>>> -static int idle_finisher(unsigned long flags)
>>>>> -{
>>>>> -	cpu_do_idle();
>>>>> -	return 1;
>>>>> -}
>>>>> -
>>>>> -static int exynos4_enter_core0_aftr(struct cpuidle_device *dev,
>>>>> -				struct cpuidle_driver *drv,
>>>>> -				int index)
>>>>> -{
>>>>> -	unsigned long tmp;
>>>>> -
>>>>> -	exynos4_set_wakeupmask();
>>>>> -
>>>>> -	/* Set value of power down register for aftr mode */
>>>>> -	exynos_sys_powerdown_conf(SYS_AFTR);
>>>>> -
>>>>> -	__raw_writel(virt_to_phys(s3c_cpu_resume), REG_DIRECTGO_ADDR);
>>>>> -	__raw_writel(S5P_CHECK_AFTR, REG_DIRECTGO_FLAG);
>>>>> -
>>>>> -	save_cpu_arch_register();
>>>>> -
>>>>> -	/* Setting Central Sequence Register for power down mode */
>>>>> -	tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
>>>>> -	tmp &= ~S5P_CENTRAL_LOWPWR_CFG;
>>>>> -	__raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
>>>>> -
>>>>> -	cpu_pm_enter();
>>>>> -	cpu_suspend(0, idle_finisher);
>>>>> -
>>>>> -#ifdef CONFIG_SMP
>>>>> -	if (!soc_is_exynos5250())
>>>>> -		scu_enable(S5P_VA_SCU);
>>>>> -#endif
>>>>> -	cpu_pm_exit();
>>>>> -
>>>>> -	restore_cpu_arch_register();
>>>>> -
>>>>> -	/*
>>>>> -	 * If PMU failed while entering sleep mode, WFI will be
>>>>> -	 * ignored by PMU and then exiting cpu_do_idle().
>>>>> -	 * S5P_CENTRAL_LOWPWR_CFG bit will not be set automatically
>>>>> -	 * in this situation.
>>>>> -	 */
>>>>> -	tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
>>>>> -	if (!(tmp & S5P_CENTRAL_LOWPWR_CFG)) {
>>>>> -		tmp |= S5P_CENTRAL_LOWPWR_CFG;
>>>>> -		__raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
>>>>> -	}
>>>>> -
>>>>> -	/* Clear wakeup state register */
>>>>> -	__raw_writel(0x0, S5P_WAKEUP_STAT);
>>>>> -
>>>>> -	return index;
>>>>> -}
>>>>> -
>>>>> -static int exynos4_enter_lowpower(struct cpuidle_device *dev,
>>>>> -				struct cpuidle_driver *drv,
>>>>> -				int index)
>>>>> -{
>>>>> -	int new_index = index;
>>>>> -
>>>>> -	/* This mode only can be entered when other core's are offline */
>>>>> -	if (num_online_cpus() > 1)
>>>>> -		new_index = drv->safe_state_index;
>>>>> -
>>>>> -	if (new_index == 0)
>>>>> -		return arm_cpuidle_simple_enter(dev, drv, new_index);
>>>>> -	else
>>>>> -		return exynos4_enter_core0_aftr(dev, drv, new_index);
>>>>> -}
>>>>> -
>>>>>
>>>>>  static void __init exynos5_core_down_clk(void)
>>>>>  {
>>>>>  
>>>>>  	unsigned int tmp;
>>>>>
>>>>> @@ -209,10 +84,6 @@ static int __init exynos4_init_cpuidle(void)
>>>>>
>>>>>  		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
>>>>>  		device->cpu = cpu_id;
>>>>>
>>>>> -		/* Support IDLE only */
>>>>> -		if (cpu_id != 0)
>>>>> -			device->state_count = 1;
>>>>> -
>>>>>
>>>>>  		ret = cpuidle_register_device(device);
>>>>>  		if (ret) {
>>>>>  		
>>>>>  			printk(KERN_ERR "CPUidle register device failed\n");


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

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

[PATCH 1/3] ARM: EXYNOS: remove non-working AFTR mode support

[Bartlomiej Zolnierkiewicz]

On Friday, June 28, 2013 01:20:09 PM Daniel Lezcano wrote:
> On 06/28/2013 12:11 PM, Tomasz Figa wrote:
> > Hi Daniel,
> > 
> > I've been fighting with this whole AFTR state as well, before Bartlomiej. 
> > Let me share my thoughts on this.
> > 
> > On Friday 28 of June 2013 11:57:25 Daniel Lezcano wrote:
> >> On 06/27/2013 08:10 PM, Bartlomiej Zolnierkiewicz wrote:
> >>> On Wednesday, June 26, 2013 12:36:12 PM Daniel Lezcano wrote:
> >>>> On 06/26/2013 12:13 PM, Bartlomiej Zolnierkiewicz wrote:
> >>>>> AFTR mode support was introduced by commit 67173ca ("ARM: EXYNOS: Add
> >>>>> support AFTR mode on EXYNOS4210") in v3.4 kernel.  Unfortunately even
> >>>>> in v3.4 kernel it hasn't worked as supposed and this is still the
> >>>>> case
> >>>>> with v3.10-rc6 (it probably wasn't noticed because CONFIG_CPU_IDLE is
> >>>>> not turned on by default):
> >>>>>
> >>>>> - on revision 0 of Exynos4210 (Universal C210 board) it causes lockup
> >>>>>
> >>>>>   (on this revision only one core is usable so entry to AFTR mode is
> >>>>>   always attempted because the code tries to go into AFTR mode when
> >>>>>   all
> >>>>>   other CPUs except CPU0 are offlined)
> >>>>>
> >>>>> - on revision 1.1 of Exynos4210 (Trats board) it causes a lockup when
> >>>>>
> >>>>>   CPU1 is offlined (i.e. echo 0 >
> >>>>>   /sys/devices/system/cpu/cpu1/online)
> >>>>>
> >>>>> - on later Exynos4/5 SoCs wrong registers may be accessed when all
> >>>>> CPUs
> >>>>>
> >>>>>   except CPU0 are offlined resulting in panic/lockup
> >>>>>   (REG_DIRECTGO_ADDR
> >>>>>   and REG_DIRECTGO_FLAG register selections was implemented only for
> >>>>>   Exynos4210)
> >>>>>
> >>>>> Just remove AFTR mode support for now.
> >>>>
> >>>> Ok, I will jump on the opportunity to talk about this state.
> >>>>
> >>>> 1. I tried different ways to make the AFTR state to be entered with
> >>>> *both* cpu online. It goes successfully to this state. The CPU0 is
> >>>> correctly woken up but the CPU1 is never woken up, why is it happening
> >>>> ?
> >>>>
> >>>> https://bugs.launchpad.net/linaro-landing-team-samsung/+bug/1171518
> >>>
> >>> No idea here, AFTR doesn't work for me with upstream kernels even if
> >>> only one CPU is online.
> >>
> >> What do you mean by "AFTR doesn't work" ? Is the kernel hanging ? The
> >> state is never reached ?
> > 
> > If you don't unplug all the CPUs >0 the state is obviously never reached. 
> > Otherwise the whole system hangs after it tries to enter this mode without 
> > any reaction for external events, other than reset.
> 
> Need investigation.
> 
> What is the exynos board version where that occurs ?

Could you please tell me what exactly do you mean by that?

I already wrote that we can reproduce the problem on EXYNOS4210 rev0
and rev1.1 (we don't have rev1.0). Tomek has also reproduced the problem
on some later SoCs (I hope that he can give you exact revisions).

In our testing we didn't encounter the board on which the problem
doesn't occur. Our current working theory is that the problem may be
u-boot (or first stage bootloader) related.

> >>> Also the documentation says that before entering system-level
> >>> power-down
> >>> mode (such as AFTR) when multiple CPUs cores are used all other CPU
> >>> cores should stop interrupt service so I'm not sure how the way
> >>> attempted by you should work.
> >>
> >> The cpu enters the idle mode with the interrupts disabled.
> > 
> > Hmm? What is supposed to wake it up then? AFAIK the whole idea of any idle 
> > or sleep is to sit in such low power mode until some interrupt fires (and so 
> > the name of the WFI, wait for interrupt, instruction).
> 
> It is handled by the hardware, for the exynos it should be the PMU. The
> CPU stays clock/power gated and when an interrupt occurs the PMU wakes
> up the CPU. This one continue its instructions after cpu_do_idle and
> right after enables the local interrupts leading to the interrupt handling.
> 
> >>>> 2. The CPU1 hotplug bug should been fixed and if I am not wrong there
> >>>> is
> >>>> a patch somewhere fixing this.
> >>>>
> >>>> https://bugs.launchpad.net/linaro-power-kernel/+bug/1171382
> >>>
> >>> Unfortunately none of the patches there helps with my issues.
> >>>
> >>>> 3. What is the fix for Exynos5 ?
> >>>>
> >>>> https://bugs.launchpad.net/linaro-power-kernel/+bug/1171253
> >>>>
> >>>> It sounds like depending on Hypervisor mode is enabled or not, the
> >>>> AFTR
> >>>> does not work correctly.
> >>>
> >>> Sorry no idea here either.  On any SoCs later than EXYNOS4210 the
> >>> registers used for s3c_cpu_resume address and 0xFCBA0D10 magic number
> >>> may be different than EXYNOS4210 defaults (at least on EXYNOS4412 they
> >>> indeed are different, unfortunately I lack any info needed for EXYNOS5
> >>> support). You are lucky that it even works in some cases on EXYNOS5250.
> >>>
> >>>> In other words, instead of removing the AFTR state I suggest to fix
> >>>> it:
> >>>> both core being online, split driver for exynos4 and 5.
> >>>
> >>> My main problem is that with the upstream kernel even on EXYNOS4210
> >>> rev0 (only one core useable due to hardware issues) the kernel goes
> >>> into AFTR state for the first few times during boot and then it just
> >>> lockups (after going into cpu_do_idle() which is really
> >>> cpu_v7_do_idle()
> >>> and which does wfi call) and doesn't wake up CPU0. I have currently
> >>> no idea how to fix or debug it further.
> >>
> >> I have an Origen 4210 board Ver A. and it works without problem with the
> >> AFTR mode (cpu1 unplugged).
> > 
> > Great!
> > 
> > Since benefits of this feature are rather questionable, especially when you 
> > consider all the maintenance burden caused by it, could you do some 
> > measurements to check if power saving thanks to this mode is of any 
> > significance?
> 
> No I can't, no spare time for that and furthermore this work has already
> be done by Amit Daniel when he submitted the driver.

Unfortunately the results were never made public...

> Amit Daniel is no longer a Linaro assignee but it is still part of the
> Samsung company (changed the email address to reach him).
> 
> >>> The issue happens with every upstream kernel version tried (from v3.4
> >>> to v3.10-rc6).  Lockups also happen on EXYNOS4210 rev1.1 when CPU1 is
> >>> offlined by hand and then cpuidle driver tries to go into AFTR mode
> >>> (because by default it doesn't go into AFTR mode on any SoC except
> >>> EXYNOS4210 rev0).
> >>>
> >>> I don't have EXYNOS4210 rev1.0 but it seems that in the upstream AFTR
> >>> mode has never worked (even on hardware that it was originally
> >>> developed
> >>> on) since its introduction in v3.4 (which was released on 20th May
> >>> 2012).
> >>>
> >>> IOW for over the year nobody cared to make it work and I have currently
> >>> no fix at hand so the corrent upstream resolution is to just remove the
> >>> known non-working code and re-introduce it later when/if needed (I can
> >>> just disable it with a small fix but we don't keep such long-term
> >>> broken
> >>> code as placeholder in the upstream kernel).  If left as it is people
> >>> can hit the known issues and waste time debugging them, just like this
> >>> happenend for me [1].
> >>>
> >>> If you have AFTR mode working (especially on EXYNOS4210) in Linaro
> >>> kernels please get fixes upstream ASAP. However I still wonder whether
> >>> the maintanance nightmare (bugs for different cases in your launchpad)
> >>> is worth gains over standard idle mode as the rumor around here is that
> >>> they are not that great (unfortunately no numbers were provided during
> >>> original feature addition).
> >>
> >> It works forme with a vanilla kernel 3.10.0-rc7.
> > 
> > As Bartek already said, I haven't worked on any of our Exynos 4210 based 
> > boards since it got introduced in Linux 3.4, with exactly the same effect we 
> > described.
> > 
> >> Removing a feature because it seems not working is not a good solution.
> >> The right way is to investigate what is happening and why.
> > 
> > I can agree only partially. Keeping a feature that is broken and without 
> > any significant benefits does not make sense for me. Neither does putting 
> > efforts into fixing it, only to find that it is of no use.
> > 
> > However this is purely a speculation. Could you test on your Origen, on 
> > which it is supposed to work, if this feature is of any use?
> 
> It is useless to do that. This work is already done.
> 
> The kernel is not a playground where you can upstream code and then
> remove it because a feature seems broken and you don't have an idea of why.

Neither me or Tomek did upstream this code and we couldn't react in
time because we haven't noticed that it is completely unusable for us
as EXYNOS cpuidle driver is not even used by default on EXYNOS (it is
not enabled either in defconfig or Kconfig).

Moreover the feature we are talking about (AFTR mode) is also not used
by default (except EXYNOS4210 rev0 on which it lockups system for us)
even with EXYNOS cpuidle driver being enabled (because this specific
feature depends on CPU hot-unplug which is not done automatically right
now).

Such things like unused/broken code removal is not something very
unusual in the upstream kernel (I'm speaking from the experience here
having maintained large subsystem for a couple of years). In this
particular case we are talking about ~130 lines of code which can
be trivially brought back later when/if needed.

Anyway if the code removal is controversial for you we can just disable
AFTR mode by default and enable it only when special command line option
is given (i.e. "aftr"). This would fix all the broken configuration
while still allowing the feature to be enabled on systems that had it
working previously (since you claim that it works on some chipset/u-boot
configurations).

> I asked several times the reasons of why the AFTR state couldn't work
> with multiple CPUs and I had no answer.

Unfortunately I don't know the answer for your question.

The AFTR mode doesn't work for us *at*all* (even with *one* CPU).

> Frankly speaking I have a couple of hypothesis:
> 
> 1. something is not correctly setup and the PMU does not wake up the CPU1
> 2. there is a silicon bug and no one wants to tell it is the case
> 
> In any case, this must be investigated and identified. And then we can
> take a decision about this state.

I don't have good idea currently how to investigate it further.

I also don't have any prove that the actual work is worth it
(and this work can easily take some weeks).

One of main responsibilities of the maintainer it to make sure that
the code does indeed work and that regressions (like these caused by
AFTR mode feature) are fixed in the timely manner, not let the code
sit in the limbo state for large periods of time.  It is already very
bad situation that the regression we are hitting was present since
v3.4 and we are in v3.10 now, I would like to have it fixed ASAP so
we may actually consider enabling cpuidle in our exynos_defconfig.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> >>> --
> >>> Bartlomiej Zolnierkiewicz
> >>> Samsung R&D Institute Poland
> >>> Samsung Electronics
> >>>
> >>> [1] Somebody enabled CONFIG_CPU_IDLE in the default config in one of
> >>> our
> >>> internal kernel trees on a target on which this feature "works" (since
> >>> CPUs are not hot-unplugged by default on all targets except EXYNOS4210
> >>> rev0 the code for AFTR is not used) and resulted in lockups on boot on
> >>> my default testing target. It took my long time to find out that
> >>> problem
> >>> is actually caused by just enabling exynos cpuidle driver.
> >>>
> >>>> Thanks
> >>>>
> >>>>   -- Daniel
> >>>>>
> >>>>> Cc: Jaecheol Lee <jc.lee@samsung.com>
> >>>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> >>>>> Cc: Amit Daniel Kachhap <amit.kachhap@linaro.org>
> >>>>> Cc: Tomasz Figa <t.figa@samsung.com>
> >>>>> Cc: Kukjin Kim <kgene.kim@samsung.com>
> >>>>> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> >>>>> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> >>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >>>>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> >>>>> ---
> >>>>>
> >>>>>  arch/arm/mach-exynos/cpuidle.c | 131
> >>>>>  +---------------------------------------- 1 file changed, 1
> >>>>>  insertion(+), 130 deletions(-)
> >>>>>
> >>>>> diff --git a/arch/arm/mach-exynos/cpuidle.c
> >>>>> b/arch/arm/mach-exynos/cpuidle.c index 17a18ff..0a657ac 100644
> >>>>> --- a/arch/arm/mach-exynos/cpuidle.c
> >>>>> +++ b/arch/arm/mach-exynos/cpuidle.c
> >>>>> @@ -17,30 +17,11 @@
> >>>>>
> >>>>>  #include <linux/time.h>
> >>>>>  
> >>>>>  #include <asm/proc-fns.h>
> >>>>>
> >>>>> -#include <asm/smp_scu.h>
> >>>>> -#include <asm/suspend.h>
> >>>>> -#include <asm/unified.h>
> >>>>>
> >>>>>  #include <asm/cpuidle.h>
> >>>>>  #include <mach/regs-clock.h>
> >>>>>
> >>>>> -#include <mach/regs-pmu.h>
> >>>>> -
> >>>>> -#include <plat/cpu.h>
> >>>>>
> >>>>>  #include "common.h"
> >>>>>
> >>>>> -#define REG_DIRECTGO_ADDR	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
> >>>>> -			S5P_INFORM7 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
> >>>>> -			(S5P_VA_SYSRAM + 0x24) : S5P_INFORM0))
> >>>>> -#define REG_DIRECTGO_FLAG	(samsung_rev() == EXYNOS4210_REV_1_1 ? \
> >>>>> -			S5P_INFORM6 : (samsung_rev() == EXYNOS4210_REV_1_0 ? \
> >>>>> -			(S5P_VA_SYSRAM + 0x20) : S5P_INFORM1))
> >>>>> -
> >>>>> -#define S5P_CHECK_AFTR		0xFCBA0D10
> >>>>> -
> >>>>> -static int exynos4_enter_lowpower(struct cpuidle_device *dev,
> >>>>> -				struct cpuidle_driver *drv,
> >>>>> -				int index);
> >>>>> -
> >>>>>
> >>>>>  static DEFINE_PER_CPU(struct cpuidle_device,
> >>>>>  exynos4_cpuidle_device);
> >>>>>  
> >>>>>  static struct cpuidle_driver exynos4_idle_driver = {
> >>>>>
> >>>>> @@ -48,117 +29,11 @@ static struct cpuidle_driver exynos4_idle_driver
> >>>>> = {>>> 
> >>>>>  	.owner			= THIS_MODULE,
> >>>>>  	.states = {
> >>>>>  	
> >>>>>  		[0] = ARM_CPUIDLE_WFI_STATE,
> >>>>>
> >>>>> -		[1] = {
> >>>>> -			.enter			= exynos4_enter_lowpower,
> >>>>> -			.exit_latency		= 300,
> >>>>> -			.target_residency	= 100000,
> >>>>> -			.flags			= CPUIDLE_FLAG_TIME_VALID,
> >>>>> -			.name			= "C1",
> >>>>> -			.desc			= "ARM power down",
> >>>>> -		},
> >>>>>
> >>>>>  	},
> >>>>>
> >>>>> -	.state_count = 2,
> >>>>> +	.state_count = 1,
> >>>>>
> >>>>>  	.safe_state_index = 0,
> >>>>>  
> >>>>>  };
> >>>>>
> >>>>> -/* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */
> >>>>> -static void exynos4_set_wakeupmask(void)
> >>>>> -{
> >>>>> -	__raw_writel(0x0000ff3e, S5P_WAKEUP_MASK);
> >>>>> -}
> >>>>> -
> >>>>> -static unsigned int g_pwr_ctrl, g_diag_reg;
> >>>>> -
> >>>>> -static void save_cpu_arch_register(void)
> >>>>> -{
> >>>>> -	/*read power control register*/
> >>>>> -	asm("mrc p15, 0, %0, c15, c0, 0" : "=r"(g_pwr_ctrl) : : "cc");
> >>>>> -	/*read diagnostic register*/
> >>>>> -	asm("mrc p15, 0, %0, c15, c0, 1" : "=r"(g_diag_reg) : : "cc");
> >>>>> -	return;
> >>>>> -}
> >>>>> -
> >>>>> -static void restore_cpu_arch_register(void)
> >>>>> -{
> >>>>> -	/*write power control register*/
> >>>>> -	asm("mcr p15, 0, %0, c15, c0, 0" : : "r"(g_pwr_ctrl) : "cc");
> >>>>> -	/*write diagnostic register*/
> >>>>> -	asm("mcr p15, 0, %0, c15, c0, 1" : : "r"(g_diag_reg) : "cc");
> >>>>> -	return;
> >>>>> -}
> >>>>> -
> >>>>> -static int idle_finisher(unsigned long flags)
> >>>>> -{
> >>>>> -	cpu_do_idle();
> >>>>> -	return 1;
> >>>>> -}
> >>>>> -
> >>>>> -static int exynos4_enter_core0_aftr(struct cpuidle_device *dev,
> >>>>> -				struct cpuidle_driver *drv,
> >>>>> -				int index)
> >>>>> -{
> >>>>> -	unsigned long tmp;
> >>>>> -
> >>>>> -	exynos4_set_wakeupmask();
> >>>>> -
> >>>>> -	/* Set value of power down register for aftr mode */
> >>>>> -	exynos_sys_powerdown_conf(SYS_AFTR);
> >>>>> -
> >>>>> -	__raw_writel(virt_to_phys(s3c_cpu_resume), REG_DIRECTGO_ADDR);
> >>>>> -	__raw_writel(S5P_CHECK_AFTR, REG_DIRECTGO_FLAG);
> >>>>> -
> >>>>> -	save_cpu_arch_register();
> >>>>> -
> >>>>> -	/* Setting Central Sequence Register for power down mode */
> >>>>> -	tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
> >>>>> -	tmp &= ~S5P_CENTRAL_LOWPWR_CFG;
> >>>>> -	__raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
> >>>>> -
> >>>>> -	cpu_pm_enter();
> >>>>> -	cpu_suspend(0, idle_finisher);
> >>>>> -
> >>>>> -#ifdef CONFIG_SMP
> >>>>> -	if (!soc_is_exynos5250())
> >>>>> -		scu_enable(S5P_VA_SCU);
> >>>>> -#endif
> >>>>> -	cpu_pm_exit();
> >>>>> -
> >>>>> -	restore_cpu_arch_register();
> >>>>> -
> >>>>> -	/*
> >>>>> -	 * If PMU failed while entering sleep mode, WFI will be
> >>>>> -	 * ignored by PMU and then exiting cpu_do_idle().
> >>>>> -	 * S5P_CENTRAL_LOWPWR_CFG bit will not be set automatically
> >>>>> -	 * in this situation.
> >>>>> -	 */
> >>>>> -	tmp = __raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION);
> >>>>> -	if (!(tmp & S5P_CENTRAL_LOWPWR_CFG)) {
> >>>>> -		tmp |= S5P_CENTRAL_LOWPWR_CFG;
> >>>>> -		__raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION);
> >>>>> -	}
> >>>>> -
> >>>>> -	/* Clear wakeup state register */
> >>>>> -	__raw_writel(0x0, S5P_WAKEUP_STAT);
> >>>>> -
> >>>>> -	return index;
> >>>>> -}
> >>>>> -
> >>>>> -static int exynos4_enter_lowpower(struct cpuidle_device *dev,
> >>>>> -				struct cpuidle_driver *drv,
> >>>>> -				int index)
> >>>>> -{
> >>>>> -	int new_index = index;
> >>>>> -
> >>>>> -	/* This mode only can be entered when other core's are offline */
> >>>>> -	if (num_online_cpus() > 1)
> >>>>> -		new_index = drv->safe_state_index;
> >>>>> -
> >>>>> -	if (new_index == 0)
> >>>>> -		return arm_cpuidle_simple_enter(dev, drv, new_index);
> >>>>> -	else
> >>>>> -		return exynos4_enter_core0_aftr(dev, drv, new_index);
> >>>>> -}
> >>>>> -
> >>>>>
> >>>>>  static void __init exynos5_core_down_clk(void)
> >>>>>  {
> >>>>>  
> >>>>>  	unsigned int tmp;
> >>>>>
> >>>>> @@ -209,10 +84,6 @@ static int __init exynos4_init_cpuidle(void)
> >>>>>
> >>>>>  		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
> >>>>>  		device->cpu = cpu_id;
> >>>>>
> >>>>> -		/* Support IDLE only */
> >>>>> -		if (cpu_id != 0)
> >>>>> -			device->state_count = 1;
> >>>>> -
> >>>>>
> >>>>>  		ret = cpuidle_register_device(device);
> >>>>>  		if (ret) {
> >>>>>  		
> >>>>>  			printk(KERN_ERR "CPUidle register device failed\n");

[PATCH 1/3] ARM: EXYNOS: remove non-working AFTR mode support

[Daniel Lezcano]

On 06/28/2013 06:27 PM, Bartlomiej Zolnierkiewicz wrote:
> On Friday, June 28, 2013 01:20:09 PM Daniel Lezcano wrote:
>> On 06/28/2013 12:11 PM, Tomasz Figa wrote:
>>> Hi Daniel,
>>>
>>> I've been fighting with this whole AFTR state as well, before Bartlomiej. 
>>> Let me share my thoughts on this.
>>>

[ ... ]

>>>
>>> If you don't unplug all the CPUs >0 the state is obviously never reached. 
>>> Otherwise the whole system hangs after it tries to enter this mode without 
>>> any reaction for external events, other than reset.
>>
>> Need investigation.
>>
>> What is the exynos board version where that occurs ?
> 
> Could you please tell me what exactly do you mean by that?
> 
> I already wrote that we can reproduce the problem on EXYNOS4210 rev0
> and rev1.1 (we don't have rev1.0). Tomek has also reproduced the problem
> on some later SoCs (I hope that he can give you exact revisions).
> 
> In our testing we didn't encounter the board on which the problem
> doesn't occur. Our current working theory is that the problem may be
> u-boot (or first stage bootloader) related.

Ok, the status for what I know:

Origen Exynos4210, board ver A: works for me
Arndale Exynos5250: works for me but only if u-boot does not enable the
hypervisor mode.
Chromebook Exynos5250: works for me

I found the following drivers:

https://github.com/AndreiLux/Perseus-UNIVERSAL5410/blob/samsung/arch/arm/mach-exynos/cpuidle.c

https://github.com/CyanogenMod/hardkernel-kernel-4412/blob/cm-10.1/arch/arm/mach-exynos/cpuidle-exynos4.c

Sounds like the num cpus > 1 is still there.

[ ... ]

>> The kernel is not a playground where you can upstream code and then
>> remove it because a feature seems broken and you don't have an idea of why.
> 
> Neither me or Tomek did upstream this code and we couldn't react in
> time because we haven't noticed that it is completely unusable for us
> as EXYNOS cpuidle driver is not even used by default on EXYNOS (it is
> not enabled either in defconfig or Kconfig).
> 
> Moreover the feature we are talking about (AFTR mode) is also not used
> by default (except EXYNOS4210 rev0 on which it lockups system for us)
> even with EXYNOS cpuidle driver being enabled (because this specific
> feature depends on CPU hot-unplug which is not done automatically right
> now).
> 
> Such things like unused/broken code removal is not something very
> unusual in the upstream kernel (I'm speaking from the experience here
> having maintained large subsystem for a couple of years). In this
> particular case we are talking about ~130 lines of code which can
> be trivially brought back later when/if needed.
> 
> Anyway if the code removal is controversial for you we can just disable
> AFTR mode by default and enable it only when special command line option
> is given (i.e. "aftr"). This would fix all the broken configuration
> while still allowing the feature to be enabled on systems that had it
> working previously (since you claim that it works on some chipset/u-boot
> configurations).

Actually, there are several reasons I am not in favor for the moment to
remove this code:

1) code can't be pushed upstream and then removed so easily
2) I asked several times what was this cpu1 hack, I had no answer
3) I tried to make both cpus entering the AFTR state, but the cpu1 never
wakes up, I asked but no answer.

I would like to have some answers :)

Before taking the decision to remove this state (btw you can remove the
driver directly, no ? the default idle function is WFI), IMO it is worth
to investigate and to spend some time to clarify what is happening. Then
we can take a decision.

I am willing to help.

>> I asked several times the reasons of why the AFTR state couldn't work
>> with multiple CPUs and I had no answer.
> 
> Unfortunately I don't know the answer for your question.
> 
> The AFTR mode doesn't work for us *at*all* (even with *one* CPU).
> 
>> Frankly speaking I have a couple of hypothesis:
>>
>> 1. something is not correctly setup and the PMU does not wake up the CPU1
>> 2. there is a silicon bug and no one wants to tell it is the case
>>
>> In any case, this must be investigated and identified. And then we can
>> take a decision about this state.
> 
> I don't have good idea currently how to investigate it further.
> 
> I also don't have any prove that the actual work is worth it
> (and this work can easily take some weeks).
> 
> One of main responsibilities of the maintainer it to make sure that
> the code does indeed work and that regressions (like these caused by
> AFTR mode feature) are fixed in the timely manner, not let the code
> sit in the limbo state for large periods of time.  It is already very
> bad situation that the regression we are hitting was present since
> v3.4 and we are in v3.10 now, I would like to have it fixed ASAP so
> we may actually consider enabling cpuidle in our exynos_defconfig.

I agree.

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

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

[PATCH 1/3] ARM: EXYNOS: remove non-working AFTR mode support

[Tomasz Figa]

On Friday 28 of June 2013 23:47:49 Daniel Lezcano wrote:
> On 06/28/2013 06:27 PM, Bartlomiej Zolnierkiewicz wrote:
> > On Friday, June 28, 2013 01:20:09 PM Daniel Lezcano wrote:
> >> On 06/28/2013 12:11 PM, Tomasz Figa wrote:
> >>> Hi Daniel,
> >>> 
> >>> I've been fighting with this whole AFTR state as well, before
> >>> Bartlomiej. Let me share my thoughts on this.
> 
> [ ... ]
> 
> >>> If you don't unplug all the CPUs >0 the state is obviously never
> >>> reached. Otherwise the whole system hangs after it tries to enter
> >>> this mode without any reaction for external events, other than
> >>> reset.
> >> 
> >> Need investigation.
> >> 
> >> What is the exynos board version where that occurs ?
> > 
> > Could you please tell me what exactly do you mean by that?
> > 
> > I already wrote that we can reproduce the problem on EXYNOS4210 rev0
> > and rev1.1 (we don't have rev1.0). Tomek has also reproduced the
> > problem on some later SoCs (I hope that he can give you exact
> > revisions).
> > 
> > In our testing we didn't encounter the board on which the problem
> > doesn't occur. Our current working theory is that the problem may be
> > u-boot (or first stage bootloader) related.
> 
> Ok, the status for what I know:
> 
> Origen Exynos4210, board ver A: works for me
> Arndale Exynos5250: works for me but only if u-boot does not enable the
> hypervisor mode.
> Chromebook Exynos5250: works for me
> 
> I found the following drivers:
> 
> https://github.com/AndreiLux/Perseus-UNIVERSAL5410/blob/samsung/arch/arm
> /mach-exynos/cpuidle.c
> 
> https://github.com/CyanogenMod/hardkernel-kernel-4412/blob/cm-10.1/arch/
> arm/mach-exynos/cpuidle-exynos4.c
> 
> Sounds like the num cpus > 1 is still there.

As far as I know (and I can read from the documentation), AFTR is a state 
in which program execution context (e.g. PC, CPU registers, etc.) is lost. 
This is like sleep mode, but with much more state saved (all the 
peripherals and L2 cache), which lets you resume much faster.

Based on that, the execution flow is as following:
 - kernel stops all secondary cores,
 - kernel configures PMU to enter AFTR mode,
 - WFI is executed on boot CPU, which lets PMU enter the requested low 
power mode,
 - the system stays with all CPU cores powered off until a wake-up event 
happens,
 - a wake-up event causes a wake-up reset of the boot CPU, which resumes 
operation from reset vector (usually IROM),
 - here exactly the same procedure is used as on wake-up from sleep mode, 
with the exception that some assumptions can be made about kept state, 
like BL1 being still in IRAM/SYSRAM, without the need to reload it from 
boot device,
 - after going through all the boot code, it finally jumps to the resume 
handler saved by kernel in one of those INFORM registers or somewhere in 
SYSRAM,
 - resume handler reinitializes boot CPU, restores things like MMU, 
coprocessors, etc. and finally boots secondary cores again,
 - the system is operating normally again.

This is simplified a lot, with all things happened internally in PMU and 
boot code skipped, but should give the general picture. See all the 
analogies to normal sleep mode, which (IMHO) makes it understandable that 
this mode can't be entered with more than one core active. 

However, with something like coupled cpuidle, when possible, this mode can 
be entered by powering down remaining cores automatically, so this is not 
a problem with this mode itself, but rather with current approach of 
entering it.

Best regards,
Tomasz

> 
> [ ... ]
> 
> >> The kernel is not a playground where you can upstream code and then
> >> remove it because a feature seems broken and you don't have an idea
> >> of why.> 
> > Neither me or Tomek did upstream this code and we couldn't react in
> > time because we haven't noticed that it is completely unusable for us
> > as EXYNOS cpuidle driver is not even used by default on EXYNOS (it is
> > not enabled either in defconfig or Kconfig).
> > 
> > Moreover the feature we are talking about (AFTR mode) is also not used
> > by default (except EXYNOS4210 rev0 on which it lockups system for us)
> > even with EXYNOS cpuidle driver being enabled (because this specific
> > feature depends on CPU hot-unplug which is not done automatically
> > right
> > now).
> > 
> > Such things like unused/broken code removal is not something very
> > unusual in the upstream kernel (I'm speaking from the experience here
> > having maintained large subsystem for a couple of years). In this
> > particular case we are talking about ~130 lines of code which can
> > be trivially brought back later when/if needed.
> > 
> > Anyway if the code removal is controversial for you we can just
> > disable
> > AFTR mode by default and enable it only when special command line
> > option is given (i.e. "aftr"). This would fix all the broken
> > configuration while still allowing the feature to be enabled on
> > systems that had it working previously (since you claim that it works
> > on some chipset/u-boot configurations).
> 
> Actually, there are several reasons I am not in favor for the moment to
> remove this code:
> 
> 1) code can't be pushed upstream and then removed so easily
> 2) I asked several times what was this cpu1 hack, I had no answer
> 3) I tried to make both cpus entering the AFTR state, but the cpu1 never
> wakes up, I asked but no answer.
> 
> I would like to have some answers :)
> 
> Before taking the decision to remove this state (btw you can remove the
> driver directly, no ? the default idle function is WFI), IMO it is worth
> to investigate and to spend some time to clarify what is happening.
> Then we can take a decision.
> 
> I am willing to help.
> 
> >> I asked several times the reasons of why the AFTR state couldn't work
> >> with multiple CPUs and I had no answer.
> > 
> > Unfortunately I don't know the answer for your question.
> > 
> > The AFTR mode doesn't work for us *at*all* (even with *one* CPU).
> > 
> >> Frankly speaking I have a couple of hypothesis:
> >> 
> >> 1. something is not correctly setup and the PMU does not wake up the
> >> CPU1 2. there is a silicon bug and no one wants to tell it is the
> >> case
> >> 
> >> In any case, this must be investigated and identified. And then we
> >> can
> >> take a decision about this state.
> > 
> > I don't have good idea currently how to investigate it further.
> > 
> > I also don't have any prove that the actual work is worth it
> > (and this work can easily take some weeks).
> > 
> > One of main responsibilities of the maintainer it to make sure that
> > the code does indeed work and that regressions (like these caused by
> > AFTR mode feature) are fixed in the timely manner, not let the code
> > sit in the limbo state for large periods of time.  It is already very
> > bad situation that the regression we are hitting was present since
> > v3.4 and we are in v3.10 now, I would like to have it fixed ASAP so
> > we may actually consider enabling cpuidle in our exynos_defconfig.
> 
> I agree.
> 
> Thanks
>   -- Daniel

[PATCH 1/3] ARM: EXYNOS: remove non-working AFTR mode support

[Daniel Lezcano]

On 06/29/2013 12:47 AM, Tomasz Figa wrote:
> On Friday 28 of June 2013 23:47:49 Daniel Lezcano wrote:
>> On 06/28/2013 06:27 PM, Bartlomiej Zolnierkiewicz wrote:
>>> On Friday, June 28, 2013 01:20:09 PM Daniel Lezcano wrote:
>>>> On 06/28/2013 12:11 PM, Tomasz Figa wrote:
>>>>> Hi Daniel,
>>>>>
>>>>> I've been fighting with this whole AFTR state as well, before
>>>>> Bartlomiej. Let me share my thoughts on this.
>>
>> [ ... ]
>>
>>>>> If you don't unplug all the CPUs >0 the state is obviously never
>>>>> reached. Otherwise the whole system hangs after it tries to enter
>>>>> this mode without any reaction for external events, other than
>>>>> reset.
>>>>
>>>> Need investigation.
>>>>
>>>> What is the exynos board version where that occurs ?
>>>
>>> Could you please tell me what exactly do you mean by that?
>>>
>>> I already wrote that we can reproduce the problem on EXYNOS4210 rev0
>>> and rev1.1 (we don't have rev1.0). Tomek has also reproduced the
>>> problem on some later SoCs (I hope that he can give you exact
>>> revisions).
>>>
>>> In our testing we didn't encounter the board on which the problem
>>> doesn't occur. Our current working theory is that the problem may be
>>> u-boot (or first stage bootloader) related.
>>
>> Ok, the status for what I know:
>>
>> Origen Exynos4210, board ver A: works for me
>> Arndale Exynos5250: works for me but only if u-boot does not enable the
>> hypervisor mode.
>> Chromebook Exynos5250: works for me
>>
>> I found the following drivers:
>>
>> https://github.com/AndreiLux/Perseus-UNIVERSAL5410/blob/samsung/arch/arm
>> /mach-exynos/cpuidle.c
>>
>> https://github.com/CyanogenMod/hardkernel-kernel-4412/blob/cm-10.1/arch/
>> arm/mach-exynos/cpuidle-exynos4.c
>>
>> Sounds like the num cpus > 1 is still there.
> 
> As far as I know (and I can read from the documentation), AFTR is a state 
> in which program execution context (e.g. PC, CPU registers, etc.) is lost. 
> This is like sleep mode, but with much more state saved (all the 
> peripherals and L2 cache), which lets you resume much faster.
> 
> Based on that, the execution flow is as following:
>  - kernel stops all secondary cores,
>  - kernel configures PMU to enter AFTR mode,
>  - WFI is executed on boot CPU, which lets PMU enter the requested low 
> power mode,
>  - the system stays with all CPU cores powered off until a wake-up event 
> happens,
>  - a wake-up event causes a wake-up reset of the boot CPU, which resumes 
> operation from reset vector (usually IROM),
>  - here exactly the same procedure is used as on wake-up from sleep mode, 
> with the exception that some assumptions can be made about kept state, 
> like BL1 being still in IRAM/SYSRAM, without the need to reload it from 
> boot device,
>  - after going through all the boot code, it finally jumps to the resume 
> handler saved by kernel in one of those INFORM registers or somewhere in 
> SYSRAM,
>  - resume handler reinitializes boot CPU, restores things like MMU, 
> coprocessors, etc. and finally boots secondary cores again,
>  - the system is operating normally again.
> 
> This is simplified a lot, with all things happened internally in PMU and 
> boot code skipped, but should give the general picture. See all the 
> analogies to normal sleep mode, which (IMHO) makes it understandable that 
> this mode can't be entered with more than one core active. 
> 
> However, with something like coupled cpuidle, when possible, this mode can 
> be entered by powering down remaining cores automatically, so this is not 
> a problem with this mode itself, but rather with current approach of 
> entering it.

Are you saying only CPU0 can program the PMU to enter the AFTR mode ?
And only CPU0 can be woken up ?

Is it possible to have CPU0 to enter the AFTR idle state (without
switching to this mode), save its context and go to WFI. And then when
CPU1 goes to this state also, it saves its context and set the PMU to
enter the AFTR mode ?



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

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

[PATCH 1/3] ARM: EXYNOS: remove non-working AFTR mode support

[Bartlomiej Zolnierkiewicz]

Hi,

On Friday, June 28, 2013 11:47:49 PM Daniel Lezcano wrote:
> On 06/28/2013 06:27 PM, Bartlomiej Zolnierkiewicz wrote:
> > On Friday, June 28, 2013 01:20:09 PM Daniel Lezcano wrote:
> >> On 06/28/2013 12:11 PM, Tomasz Figa wrote:
> >>> Hi Daniel,
> >>>
> >>> I've been fighting with this whole AFTR state as well, before Bartlomiej. 
> >>> Let me share my thoughts on this.
> >>>
> 
> [ ... ]
> 
> >>>
> >>> If you don't unplug all the CPUs >0 the state is obviously never reached. 
> >>> Otherwise the whole system hangs after it tries to enter this mode without 
> >>> any reaction for external events, other than reset.
> >>
> >> Need investigation.
> >>
> >> What is the exynos board version where that occurs ?
> > 
> > Could you please tell me what exactly do you mean by that?
> > 
> > I already wrote that we can reproduce the problem on EXYNOS4210 rev0
> > and rev1.1 (we don't have rev1.0). Tomek has also reproduced the problem
> > on some later SoCs (I hope that he can give you exact revisions).
> > 
> > In our testing we didn't encounter the board on which the problem
> > doesn't occur. Our current working theory is that the problem may be
> > u-boot (or first stage bootloader) related.
> 
> Ok, the status for what I know:
> 
> Origen Exynos4210, board ver A: works for me
> Arndale Exynos5250: works for me but only if u-boot does not enable the
> hypervisor mode.
> Chromebook Exynos5250: works for me

I've also done some more testing. First I tested on some Exynos4412 devices
(M0 and SLP_PQ) and AFTR was not working on them. Then I got my hands on
Origen Exynos4210 (thanks to Tomek Figa for providing it) and AFTR is working
just fine on it. Finally I tried Trats board again but with the upstream
u-boot instead of our custom modified version (thanks to help from Lukasz
Majewski) and I found out that after this change AFTR works fine on it! It
also gives quite nice power savings (~80mA less current drawn in AFTR mode
compared to just WFI one).

With the above findings it now seems that the issue is on our side and is
outside the kernel. Thanks for help with narrowing down the problem and
sorry for wasting your time.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> I found the following drivers:
> 
> https://github.com/AndreiLux/Perseus-UNIVERSAL5410/blob/samsung/arch/arm/mach-exynos/cpuidle.c
> 
> https://github.com/CyanogenMod/hardkernel-kernel-4412/blob/cm-10.1/arch/arm/mach-exynos/cpuidle-exynos4.c
> 
> Sounds like the num cpus > 1 is still there.
> 
> [ ... ]
> 
> >> The kernel is not a playground where you can upstream code and then
> >> remove it because a feature seems broken and you don't have an idea of why.
> > 
> > Neither me or Tomek did upstream this code and we couldn't react in
> > time because we haven't noticed that it is completely unusable for us
> > as EXYNOS cpuidle driver is not even used by default on EXYNOS (it is
> > not enabled either in defconfig or Kconfig).
> > 
> > Moreover the feature we are talking about (AFTR mode) is also not used
> > by default (except EXYNOS4210 rev0 on which it lockups system for us)
> > even with EXYNOS cpuidle driver being enabled (because this specific
> > feature depends on CPU hot-unplug which is not done automatically right
> > now).
> > 
> > Such things like unused/broken code removal is not something very
> > unusual in the upstream kernel (I'm speaking from the experience here
> > having maintained large subsystem for a couple of years). In this
> > particular case we are talking about ~130 lines of code which can
> > be trivially brought back later when/if needed.
> > 
> > Anyway if the code removal is controversial for you we can just disable
> > AFTR mode by default and enable it only when special command line option
> > is given (i.e. "aftr"). This would fix all the broken configuration
> > while still allowing the feature to be enabled on systems that had it
> > working previously (since you claim that it works on some chipset/u-boot
> > configurations).
> 
> Actually, there are several reasons I am not in favor for the moment to
> remove this code:
> 
> 1) code can't be pushed upstream and then removed so easily
> 2) I asked several times what was this cpu1 hack, I had no answer
> 3) I tried to make both cpus entering the AFTR state, but the cpu1 never
> wakes up, I asked but no answer.
> 
> I would like to have some answers :)
> 
> Before taking the decision to remove this state (btw you can remove the
> driver directly, no ? the default idle function is WFI), IMO it is worth
> to investigate and to spend some time to clarify what is happening. Then
> we can take a decision.
> 
> I am willing to help.
> 
> >> I asked several times the reasons of why the AFTR state couldn't work
> >> with multiple CPUs and I had no answer.
> > 
> > Unfortunately I don't know the answer for your question.
> > 
> > The AFTR mode doesn't work for us *at*all* (even with *one* CPU).
> > 
> >> Frankly speaking I have a couple of hypothesis:
> >>
> >> 1. something is not correctly setup and the PMU does not wake up the CPU1
> >> 2. there is a silicon bug and no one wants to tell it is the case
> >>
> >> In any case, this must be investigated and identified. And then we can
> >> take a decision about this state.
> > 
> > I don't have good idea currently how to investigate it further.
> > 
> > I also don't have any prove that the actual work is worth it
> > (and this work can easily take some weeks).
> > 
> > One of main responsibilities of the maintainer it to make sure that
> > the code does indeed work and that regressions (like these caused by
> > AFTR mode feature) are fixed in the timely manner, not let the code
> > sit in the limbo state for large periods of time.  It is already very
> > bad situation that the regression we are hitting was present since
> > v3.4 and we are in v3.10 now, I would like to have it fixed ASAP so
> > we may actually consider enabling cpuidle in our exynos_defconfig.
> 
> I agree.
> 
> Thanks
>   -- Daniel

[PATCH 1/3] ARM: EXYNOS: remove non-working AFTR mode support

[Daniel Lezcano]

On 07/11/2013 03:14 PM, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Friday, June 28, 2013 11:47:49 PM Daniel Lezcano wrote:
>> On 06/28/2013 06:27 PM, Bartlomiej Zolnierkiewicz wrote:
>>> On Friday, June 28, 2013 01:20:09 PM Daniel Lezcano wrote:
>>>> On 06/28/2013 12:11 PM, Tomasz Figa wrote:
>>>>> Hi Daniel,
>>>>>
>>>>> I've been fighting with this whole AFTR state as well, before Bartlomiej. 
>>>>> Let me share my thoughts on this.
>>>>>
>>
>> [ ... ]
>>
>>>>>
>>>>> If you don't unplug all the CPUs >0 the state is obviously never reached. 
>>>>> Otherwise the whole system hangs after it tries to enter this mode without 
>>>>> any reaction for external events, other than reset.
>>>>
>>>> Need investigation.
>>>>
>>>> What is the exynos board version where that occurs ?
>>>
>>> Could you please tell me what exactly do you mean by that?
>>>
>>> I already wrote that we can reproduce the problem on EXYNOS4210 rev0
>>> and rev1.1 (we don't have rev1.0). Tomek has also reproduced the problem
>>> on some later SoCs (I hope that he can give you exact revisions).
>>>
>>> In our testing we didn't encounter the board on which the problem
>>> doesn't occur. Our current working theory is that the problem may be
>>> u-boot (or first stage bootloader) related.
>>
>> Ok, the status for what I know:
>>
>> Origen Exynos4210, board ver A: works for me
>> Arndale Exynos5250: works for me but only if u-boot does not enable the
>> hypervisor mode.
>> Chromebook Exynos5250: works for me
> 
> I've also done some more testing. First I tested on some Exynos4412 devices
> (M0 and SLP_PQ) and AFTR was not working on them. Then I got my hands on
> Origen Exynos4210 (thanks to Tomek Figa for providing it) and AFTR is working
> just fine on it. Finally I tried Trats board again but with the upstream
> u-boot instead of our custom modified version (thanks to help from Lukasz
> Majewski) and I found out that after this change AFTR works fine on it! It
> also gives quite nice power savings (~80mA less current drawn in AFTR mode
> compared to just WFI one).

Is the 80mA power saving comparing with:

 * (cpu0/cpu1 online) vs (cpu0 AFTR and cpu1 offline)
or
 * (cpu0 AFTR and cpu1 offline) vs (cpu0 WFI and cpu1 offline)

?

> With the above findings it now seems that the issue is on our side and is
> outside the kernel. Thanks for help with narrowing down the problem and
> sorry for wasting your time.

May be we were not working on the same tree.

I am on the linux-pm-next tree.

Now the merge window occurred, the AFTR is no longer working on my board.

After git bisecting:

commit 87107d89052bcec1fe91b309631de4ed294a5171
Author: Arnd Bergmann <arnd@arndb.de>
Date:   Wed Jun 19 01:36:52 2013 +0900

    ARM: EXYNOS: Remove legacy L2X0 initialization

    Since Exynos is now supporting only DT-based boot, the old L2X0
    initialization code is not needed anymore, so exynos4_l2x0_cache_init()
    can be greatly simplified.

    Signed-off-by: Arnd Bergmann <arnd@arndb.de>
    Signed-off-by: Tomasz Figa <t.figa@samsung.com>
    Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
    Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>

:040000 040000 79e1adfd6386256ba71b3f609592d5acf9c08222
9cb0026563b3b8657d906767493d26c501963269 M	arch

Reverting the patch solves the problem.

Any ideas ?

Thanks
  -- Daniel


> 
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
> 
>> I found the following drivers:
>>
>> https://github.com/AndreiLux/Perseus-UNIVERSAL5410/blob/samsung/arch/arm/mach-exynos/cpuidle.c
>>
>> https://github.com/CyanogenMod/hardkernel-kernel-4412/blob/cm-10.1/arch/arm/mach-exynos/cpuidle-exynos4.c
>>
>> Sounds like the num cpus > 1 is still there.
>>
>> [ ... ]
>>
>>>> The kernel is not a playground where you can upstream code and then
>>>> remove it because a feature seems broken and you don't have an idea of why.
>>>
>>> Neither me or Tomek did upstream this code and we couldn't react in
>>> time because we haven't noticed that it is completely unusable for us
>>> as EXYNOS cpuidle driver is not even used by default on EXYNOS (it is
>>> not enabled either in defconfig or Kconfig).
>>>
>>> Moreover the feature we are talking about (AFTR mode) is also not used
>>> by default (except EXYNOS4210 rev0 on which it lockups system for us)
>>> even with EXYNOS cpuidle driver being enabled (because this specific
>>> feature depends on CPU hot-unplug which is not done automatically right
>>> now).
>>>
>>> Such things like unused/broken code removal is not something very
>>> unusual in the upstream kernel (I'm speaking from the experience here
>>> having maintained large subsystem for a couple of years). In this
>>> particular case we are talking about ~130 lines of code which can
>>> be trivially brought back later when/if needed.
>>>
>>> Anyway if the code removal is controversial for you we can just disable
>>> AFTR mode by default and enable it only when special command line option
>>> is given (i.e. "aftr"). This would fix all the broken configuration
>>> while still allowing the feature to be enabled on systems that had it
>>> working previously (since you claim that it works on some chipset/u-boot
>>> configurations).
>>
>> Actually, there are several reasons I am not in favor for the moment to
>> remove this code:
>>
>> 1) code can't be pushed upstream and then removed so easily
>> 2) I asked several times what was this cpu1 hack, I had no answer
>> 3) I tried to make both cpus entering the AFTR state, but the cpu1 never
>> wakes up, I asked but no answer.
>>
>> I would like to have some answers :)
>>
>> Before taking the decision to remove this state (btw you can remove the
>> driver directly, no ? the default idle function is WFI), IMO it is worth
>> to investigate and to spend some time to clarify what is happening. Then
>> we can take a decision.
>>
>> I am willing to help.
>>
>>>> I asked several times the reasons of why the AFTR state couldn't work
>>>> with multiple CPUs and I had no answer.
>>>
>>> Unfortunately I don't know the answer for your question.
>>>
>>> The AFTR mode doesn't work for us *at*all* (even with *one* CPU).
>>>
>>>> Frankly speaking I have a couple of hypothesis:
>>>>
>>>> 1. something is not correctly setup and the PMU does not wake up the CPU1
>>>> 2. there is a silicon bug and no one wants to tell it is the case
>>>>
>>>> In any case, this must be investigated and identified. And then we can
>>>> take a decision about this state.
>>>
>>> I don't have good idea currently how to investigate it further.
>>>
>>> I also don't have any prove that the actual work is worth it
>>> (and this work can easily take some weeks).
>>>
>>> One of main responsibilities of the maintainer it to make sure that
>>> the code does indeed work and that regressions (like these caused by
>>> AFTR mode feature) are fixed in the timely manner, not let the code
>>> sit in the limbo state for large periods of time.  It is already very
>>> bad situation that the regression we are hitting was present since
>>> v3.4 and we are in v3.10 now, I would like to have it fixed ASAP so
>>> we may actually consider enabling cpuidle in our exynos_defconfig.
>>
>> I agree.
>>
>> Thanks
>>   -- Daniel
> 


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

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

[PATCH 1/3] ARM: EXYNOS: remove non-working AFTR mode support

[Lorenzo Pieralisi]

On Wed, Jul 17, 2013 at 01:57:30PM +0100, Daniel Lezcano wrote:
> On 07/11/2013 03:14 PM, Bartlomiej Zolnierkiewicz wrote:
> > 
> > Hi,
> > 
> > On Friday, June 28, 2013 11:47:49 PM Daniel Lezcano wrote:
> >> On 06/28/2013 06:27 PM, Bartlomiej Zolnierkiewicz wrote:
> >>> On Friday, June 28, 2013 01:20:09 PM Daniel Lezcano wrote:
> >>>> On 06/28/2013 12:11 PM, Tomasz Figa wrote:
> >>>>> Hi Daniel,
> >>>>>
> >>>>> I've been fighting with this whole AFTR state as well, before Bartlomiej. 
> >>>>> Let me share my thoughts on this.
> >>>>>
> >>
> >> [ ... ]
> >>
> >>>>>
> >>>>> If you don't unplug all the CPUs >0 the state is obviously never reached. 
> >>>>> Otherwise the whole system hangs after it tries to enter this mode without 
> >>>>> any reaction for external events, other than reset.
> >>>>
> >>>> Need investigation.
> >>>>
> >>>> What is the exynos board version where that occurs ?
> >>>
> >>> Could you please tell me what exactly do you mean by that?
> >>>
> >>> I already wrote that we can reproduce the problem on EXYNOS4210 rev0
> >>> and rev1.1 (we don't have rev1.0). Tomek has also reproduced the problem
> >>> on some later SoCs (I hope that he can give you exact revisions).
> >>>
> >>> In our testing we didn't encounter the board on which the problem
> >>> doesn't occur. Our current working theory is that the problem may be
> >>> u-boot (or first stage bootloader) related.
> >>
> >> Ok, the status for what I know:
> >>
> >> Origen Exynos4210, board ver A: works for me
> >> Arndale Exynos5250: works for me but only if u-boot does not enable the
> >> hypervisor mode.
> >> Chromebook Exynos5250: works for me
> > 
> > I've also done some more testing. First I tested on some Exynos4412 devices
> > (M0 and SLP_PQ) and AFTR was not working on them. Then I got my hands on
> > Origen Exynos4210 (thanks to Tomek Figa for providing it) and AFTR is working
> > just fine on it. Finally I tried Trats board again but with the upstream
> > u-boot instead of our custom modified version (thanks to help from Lukasz
> > Majewski) and I found out that after this change AFTR works fine on it! It
> > also gives quite nice power savings (~80mA less current drawn in AFTR mode
> > compared to just WFI one).
> 
> Is the 80mA power saving comparing with:
> 
>  * (cpu0/cpu1 online) vs (cpu0 AFTR and cpu1 offline)
> or
>  * (cpu0 AFTR and cpu1 offline) vs (cpu0 WFI and cpu1 offline)
> 
> ?
> 
> > With the above findings it now seems that the issue is on our side and is
> > outside the kernel. Thanks for help with narrowing down the problem and
> > sorry for wasting your time.
> 
> May be we were not working on the same tree.
> 
> I am on the linux-pm-next tree.
> 
> Now the merge window occurred, the AFTR is no longer working on my board.
> 
> After git bisecting:
> 
> commit 87107d89052bcec1fe91b309631de4ed294a5171
> Author: Arnd Bergmann <arnd@arndb.de>
> Date:   Wed Jun 19 01:36:52 2013 +0900
> 
>     ARM: EXYNOS: Remove legacy L2X0 initialization
> 
>     Since Exynos is now supporting only DT-based boot, the old L2X0
>     initialization code is not needed anymore, so exynos4_l2x0_cache_init()
>     can be greatly simplified.
> 
>     Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>     Signed-off-by: Tomasz Figa <t.figa@samsung.com>
>     Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>     Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> 
> :040000 040000 79e1adfd6386256ba71b3f609592d5acf9c08222
> 9cb0026563b3b8657d906767493d26c501963269 M	arch
> 
> Reverting the patch solves the problem.
> 
> Any ideas ?

It is certainly a problem related to restoring L2 control registers in
plat-samsung/s5p-sleep.S, probably DT init misses some registers
(prefetch and power control) that are restored to zero values most
likely, or the cluster cannot be shut down owing to those values not
being programmed properly. Chander posted a patch to fix that but I could
not follow that thread, I have no idea where he got to and if that fixes
the issue, I think so.

Lorenzo

[PATCH 1/3] ARM: EXYNOS: remove non-working AFTR mode support

[Tomasz Figa]

Hi,

On Wednesday 17 of July 2013 14:31:23 Lorenzo Pieralisi wrote:
> On Wed, Jul 17, 2013 at 01:57:30PM +0100, Daniel Lezcano wrote:
> > On 07/11/2013 03:14 PM, Bartlomiej Zolnierkiewicz wrote:
> > > Hi,
> > > 
> > > On Friday, June 28, 2013 11:47:49 PM Daniel Lezcano wrote:
> > >> On 06/28/2013 06:27 PM, Bartlomiej Zolnierkiewicz wrote:
> > >>> On Friday, June 28, 2013 01:20:09 PM Daniel Lezcano wrote:
> > >>>> On 06/28/2013 12:11 PM, Tomasz Figa wrote:
> > >>>>> Hi Daniel,
> > >>>>> 
> > >>>>> I've been fighting with this whole AFTR state as well, before
> > >>>>> Bartlomiej. Let me share my thoughts on this.
> > >> 
> > >> [ ... ]
> > >> 
> > >>>>> If you don't unplug all the CPUs >0 the state is obviously never
> > >>>>> reached. Otherwise the whole system hangs after it tries to
> > >>>>> enter this mode without any reaction for external events, other
> > >>>>> than reset.
> > >>>> 
> > >>>> Need investigation.
> > >>>> 
> > >>>> What is the exynos board version where that occurs ?
> > >>> 
> > >>> Could you please tell me what exactly do you mean by that?
> > >>> 
> > >>> I already wrote that we can reproduce the problem on EXYNOS4210
> > >>> rev0
> > >>> and rev1.1 (we don't have rev1.0). Tomek has also reproduced the
> > >>> problem
> > >>> on some later SoCs (I hope that he can give you exact revisions).
> > >>> 
> > >>> In our testing we didn't encounter the board on which the problem
> > >>> doesn't occur. Our current working theory is that the problem may
> > >>> be
> > >>> u-boot (or first stage bootloader) related.
> > >> 
> > >> Ok, the status for what I know:
> > >> 
> > >> Origen Exynos4210, board ver A: works for me
> > >> Arndale Exynos5250: works for me but only if u-boot does not enable
> > >> the
> > >> hypervisor mode.
> > >> Chromebook Exynos5250: works for me
> > > 
> > > I've also done some more testing. First I tested on some Exynos4412
> > > devices (M0 and SLP_PQ) and AFTR was not working on them. Then I got
> > > my hands on Origen Exynos4210 (thanks to Tomek Figa for providing
> > > it) and AFTR is working just fine on it. Finally I tried Trats board
> > > again but with the upstream u-boot instead of our custom modified
> > > version (thanks to help from Lukasz Majewski) and I found out that
> > > after this change AFTR works fine on it! It also gives quite nice
> > > power savings (~80mA less current drawn in AFTR mode compared to
> > > just WFI one).
> > 
> > Is the 80mA power saving comparing with:
> >  * (cpu0/cpu1 online) vs (cpu0 AFTR and cpu1 offline)
> > 
> > or
> > 
> >  * (cpu0 AFTR and cpu1 offline) vs (cpu0 WFI and cpu1 offline)
> > 
> > ?
> > 
> > > With the above findings it now seems that the issue is on our side
> > > and is outside the kernel. Thanks for help with narrowing down the
> > > problem and sorry for wasting your time.
> > 
> > May be we were not working on the same tree.
> > 
> > I am on the linux-pm-next tree.
> > 
> > Now the merge window occurred, the AFTR is no longer working on my
> > board.
> > 
> > After git bisecting:
> > 
> > commit 87107d89052bcec1fe91b309631de4ed294a5171
> > Author: Arnd Bergmann <arnd@arndb.de>
> > Date:   Wed Jun 19 01:36:52 2013 +0900
> > 
> >     ARM: EXYNOS: Remove legacy L2X0 initialization
> >     
> >     Since Exynos is now supporting only DT-based boot, the old L2X0
> >     initialization code is not needed anymore, so
> >     exynos4_l2x0_cache_init()
> >     can be greatly simplified.
> >     
> >     Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> >     Signed-off-by: Tomasz Figa <t.figa@samsung.com>
> >     Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >     Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> > :
> > :040000 040000 79e1adfd6386256ba71b3f609592d5acf9c08222
> > 
> > 9cb0026563b3b8657d906767493d26c501963269 M	arch
> > 
> > Reverting the patch solves the problem.
> > 
> > Any ideas ?
> 
> It is certainly a problem related to restoring L2 control registers in
> plat-samsung/s5p-sleep.S, probably DT init misses some registers
> (prefetch and power control) that are restored to zero values most
> likely, or the cluster cannot be shut down owing to those values not
> being programmed properly.

Looks like it.

> Chander posted a patch to fix that but I could
> not follow that thread, I have no idea where he got to and if that fixes
> the issue, I think so.

I will be doing a lot of cleanup of PM code for Samsung platforms starting 
from next week. Mostly low level suspend/resume handling (Samsung PM core), 
but since AFTR mode has much in common with sleep mode (e.g. both go 
through s5p-sleep.S after waking up), so I can look at this issue too, on 
our internal boards and Origen.

As for L2 suspend/resume on Samsung platforms, it has to be reworked 
completely, because on newer boards which have secure firmware it can't be 
resumed normally, because this requires calling several firmware operations. 
I already have patches for this, but need to do more testing, including 
checking how this interferes with things like AFTR mode.

Best regards,
Tomasz

[PATCH 1/3] ARM: EXYNOS: remove non-working AFTR mode support

[Bartlomiej Zolnierkiewicz]

On Wednesday, July 17, 2013 02:57:30 PM Daniel Lezcano wrote:
> On 07/11/2013 03:14 PM, Bartlomiej Zolnierkiewicz wrote:
> > 
> > Hi,
> > 
> > On Friday, June 28, 2013 11:47:49 PM Daniel Lezcano wrote:
> >> On 06/28/2013 06:27 PM, Bartlomiej Zolnierkiewicz wrote:
> >>> On Friday, June 28, 2013 01:20:09 PM Daniel Lezcano wrote:
> >>>> On 06/28/2013 12:11 PM, Tomasz Figa wrote:
> >>>>> Hi Daniel,
> >>>>>
> >>>>> I've been fighting with this whole AFTR state as well, before Bartlomiej. 
> >>>>> Let me share my thoughts on this.
> >>>>>
> >>
> >> [ ... ]
> >>
> >>>>>
> >>>>> If you don't unplug all the CPUs >0 the state is obviously never reached. 
> >>>>> Otherwise the whole system hangs after it tries to enter this mode without 
> >>>>> any reaction for external events, other than reset.
> >>>>
> >>>> Need investigation.
> >>>>
> >>>> What is the exynos board version where that occurs ?
> >>>
> >>> Could you please tell me what exactly do you mean by that?
> >>>
> >>> I already wrote that we can reproduce the problem on EXYNOS4210 rev0
> >>> and rev1.1 (we don't have rev1.0). Tomek has also reproduced the problem
> >>> on some later SoCs (I hope that he can give you exact revisions).
> >>>
> >>> In our testing we didn't encounter the board on which the problem
> >>> doesn't occur. Our current working theory is that the problem may be
> >>> u-boot (or first stage bootloader) related.
> >>
> >> Ok, the status for what I know:
> >>
> >> Origen Exynos4210, board ver A: works for me
> >> Arndale Exynos5250: works for me but only if u-boot does not enable the
> >> hypervisor mode.
> >> Chromebook Exynos5250: works for me
> > 
> > I've also done some more testing. First I tested on some Exynos4412 devices
> > (M0 and SLP_PQ) and AFTR was not working on them. Then I got my hands on
> > Origen Exynos4210 (thanks to Tomek Figa for providing it) and AFTR is working
> > just fine on it. Finally I tried Trats board again but with the upstream
> > u-boot instead of our custom modified version (thanks to help from Lukasz
> > Majewski) and I found out that after this change AFTR works fine on it! It
> > also gives quite nice power savings (~80mA less current drawn in AFTR mode
> > compared to just WFI one).
> 
> Is the 80mA power saving comparing with:
> 
>  * (cpu0/cpu1 online) vs (cpu0 AFTR and cpu1 offline)
> or
>  * (cpu0 AFTR and cpu1 offline) vs (cpu0 WFI and cpu1 offline)
> ?

The latter one and the following scenario:

 * (cpu0/cpu1 online) vs (cpu0 WFI and cpu1 offline)

gives only ~10mA saving. Maybe something is not working as it should be in
the hotplug support and the code is not doing as much as it could but these
are the actual results (using vanilla v3.10).

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

> > With the above findings it now seems that the issue is on our side and is
> > outside the kernel. Thanks for help with narrowing down the problem and
> > sorry for wasting your time.
> 
> May be we were not working on the same tree.
> 
> I am on the linux-pm-next tree.
> 
> Now the merge window occurred, the AFTR is no longer working on my board.
> 
> After git bisecting:
> 
> commit 87107d89052bcec1fe91b309631de4ed294a5171
> Author: Arnd Bergmann <arnd@arndb.de>
> Date:   Wed Jun 19 01:36:52 2013 +0900
> 
>     ARM: EXYNOS: Remove legacy L2X0 initialization
> 
>     Since Exynos is now supporting only DT-based boot, the old L2X0
>     initialization code is not needed anymore, so exynos4_l2x0_cache_init()
>     can be greatly simplified.
> 
>     Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>     Signed-off-by: Tomasz Figa <t.figa@samsung.com>
>     Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>     Signed-off-by: Kukjin Kim <kgene.kim@samsung.com>
> 
> :040000 040000 79e1adfd6386256ba71b3f609592d5acf9c08222
> 9cb0026563b3b8657d906767493d26c501963269 M	arch
> 
> Reverting the patch solves the problem.
> 
> Any ideas ?
> 
> Thanks
>   -- Daniel
> 
> 
> > 
> > Best regards,
> > --
> > Bartlomiej Zolnierkiewicz
> > Samsung R&D Institute Poland
> > Samsung Electronics
> > 
> >> I found the following drivers:
> >>
> >> https://github.com/AndreiLux/Perseus-UNIVERSAL5410/blob/samsung/arch/arm/mach-exynos/cpuidle.c
> >>
> >> https://github.com/CyanogenMod/hardkernel-kernel-4412/blob/cm-10.1/arch/arm/mach-exynos/cpuidle-exynos4.c
> >>
> >> Sounds like the num cpus > 1 is still there.
> >>
> >> [ ... ]
> >>
> >>>> The kernel is not a playground where you can upstream code and then
> >>>> remove it because a feature seems broken and you don't have an idea of why.
> >>>
> >>> Neither me or Tomek did upstream this code and we couldn't react in
> >>> time because we haven't noticed that it is completely unusable for us
> >>> as EXYNOS cpuidle driver is not even used by default on EXYNOS (it is
> >>> not enabled either in defconfig or Kconfig).
> >>>
> >>> Moreover the feature we are talking about (AFTR mode) is also not used
> >>> by default (except EXYNOS4210 rev0 on which it lockups system for us)
> >>> even with EXYNOS cpuidle driver being enabled (because this specific
> >>> feature depends on CPU hot-unplug which is not done automatically right
> >>> now).
> >>>
> >>> Such things like unused/broken code removal is not something very
> >>> unusual in the upstream kernel (I'm speaking from the experience here
> >>> having maintained large subsystem for a couple of years). In this
> >>> particular case we are talking about ~130 lines of code which can
> >>> be trivially brought back later when/if needed.
> >>>
> >>> Anyway if the code removal is controversial for you we can just disable
> >>> AFTR mode by default and enable it only when special command line option
> >>> is given (i.e. "aftr"). This would fix all the broken configuration
> >>> while still allowing the feature to be enabled on systems that had it
> >>> working previously (since you claim that it works on some chipset/u-boot
> >>> configurations).
> >>
> >> Actually, there are several reasons I am not in favor for the moment to
> >> remove this code:
> >>
> >> 1) code can't be pushed upstream and then removed so easily
> >> 2) I asked several times what was this cpu1 hack, I had no answer
> >> 3) I tried to make both cpus entering the AFTR state, but the cpu1 never
> >> wakes up, I asked but no answer.
> >>
> >> I would like to have some answers :)
> >>
> >> Before taking the decision to remove this state (btw you can remove the
> >> driver directly, no ? the default idle function is WFI), IMO it is worth
> >> to investigate and to spend some time to clarify what is happening. Then
> >> we can take a decision.
> >>
> >> I am willing to help.
> >>
> >>>> I asked several times the reasons of why the AFTR state couldn't work
> >>>> with multiple CPUs and I had no answer.
> >>>
> >>> Unfortunately I don't know the answer for your question.
> >>>
> >>> The AFTR mode doesn't work for us *at*all* (even with *one* CPU).
> >>>
> >>>> Frankly speaking I have a couple of hypothesis:
> >>>>
> >>>> 1. something is not correctly setup and the PMU does not wake up the CPU1
> >>>> 2. there is a silicon bug and no one wants to tell it is the case
> >>>>
> >>>> In any case, this must be investigated and identified. And then we can
> >>>> take a decision about this state.
> >>>
> >>> I don't have good idea currently how to investigate it further.
> >>>
> >>> I also don't have any prove that the actual work is worth it
> >>> (and this work can easily take some weeks).
> >>>
> >>> One of main responsibilities of the maintainer it to make sure that
> >>> the code does indeed work and that regressions (like these caused by
> >>> AFTR mode feature) are fixed in the timely manner, not let the code
> >>> sit in the limbo state for large periods of time.  It is already very
> >>> bad situation that the regression we are hitting was present since
> >>> v3.4 and we are in v3.10 now, I would like to have it fixed ASAP so
> >>> we may actually consider enabling cpuidle in our exynos_defconfig.
> >>
> >> I agree.
> >>
> >> Thanks
> >>   -- Daniel

[PATCH 1/3] ARM: EXYNOS: remove non-working AFTR mode support

[Tomasz Figa]

On Friday 28 of June 2013 13:20:09 Daniel Lezcano wrote:
> On 06/28/2013 12:11 PM, Tomasz Figa wrote:
> > Hi Daniel,
> > 
> > I've been fighting with this whole AFTR state as well, before
> > Bartlomiej. Let me share my thoughts on this.
> > 
> > On Friday 28 of June 2013 11:57:25 Daniel Lezcano wrote:
> >> On 06/27/2013 08:10 PM, Bartlomiej Zolnierkiewicz wrote:
> >>> On Wednesday, June 26, 2013 12:36:12 PM Daniel Lezcano wrote:
> >>>> On 06/26/2013 12:13 PM, Bartlomiej Zolnierkiewicz wrote:
> >>>>> AFTR mode support was introduced by commit 67173ca ("ARM: EXYNOS:
> >>>>> Add
> >>>>> support AFTR mode on EXYNOS4210") in v3.4 kernel.  Unfortunately
> >>>>> even
> >>>>> in v3.4 kernel it hasn't worked as supposed and this is still the
> >>>>> case
> >>>>> with v3.10-rc6 (it probably wasn't noticed because CONFIG_CPU_IDLE
> >>>>> is
> >>>>> not turned on by default):
> >>>>> 
> >>>>> - on revision 0 of Exynos4210 (Universal C210 board) it causes
> >>>>> lockup
> >>>>> 
> >>>>>   (on this revision only one core is usable so entry to AFTR mode
> >>>>>   is
> >>>>>   always attempted because the code tries to go into AFTR mode when
> >>>>>   all
> >>>>>   other CPUs except CPU0 are offlined)
> >>>>> 
> >>>>> - on revision 1.1 of Exynos4210 (Trats board) it causes a lockup
> >>>>> when
> >>>>> 
> >>>>>   CPU1 is offlined (i.e. echo 0 >
> >>>>>   /sys/devices/system/cpu/cpu1/online)
> >>>>> 
> >>>>> - on later Exynos4/5 SoCs wrong registers may be accessed when all
> >>>>> CPUs
> >>>>> 
> >>>>>   except CPU0 are offlined resulting in panic/lockup
> >>>>>   (REG_DIRECTGO_ADDR
> >>>>>   and REG_DIRECTGO_FLAG register selections was implemented only
> >>>>>   for
> >>>>>   Exynos4210)
> >>>>> 
> >>>>> Just remove AFTR mode support for now.
> >>>> 
> >>>> Ok, I will jump on the opportunity to talk about this state.
> >>>> 
> >>>> 1. I tried different ways to make the AFTR state to be entered with
> >>>> *both* cpu online. It goes successfully to this state. The CPU0 is
> >>>> correctly woken up but the CPU1 is never woken up, why is it
> >>>> happening
> >>>> ?
> >>>> 
> >>>> https://bugs.launchpad.net/linaro-landing-team-samsung/+bug/1171518
> >>> 
> >>> No idea here, AFTR doesn't work for me with upstream kernels even if
> >>> only one CPU is online.
> >> 
> >> What do you mean by "AFTR doesn't work" ? Is the kernel hanging ? The
> >> state is never reached ?
> > 
> > If you don't unplug all the CPUs >0 the state is obviously never
> > reached. Otherwise the whole system hangs after it tries to enter this
> > mode without any reaction for external events, other than reset.
> 
> Need investigation.

Not necessarily. If this feature is worth it, then sure, but otherwise it 
would be just wasted effort.

> What is the exynos board version where that occurs ?

If this is what you are asking about:
 - Universal C210 with Exynos 4210 rev 0.0,
 - Trats with Exynos 4210 rev. 1.1.

> >>> Also the documentation says that before entering system-level
> >>> power-down
> >>> mode (such as AFTR) when multiple CPUs cores are used all other CPU
> >>> cores should stop interrupt service so I'm not sure how the way
> >>> attempted by you should work.
> >> 
> >> The cpu enters the idle mode with the interrupts disabled.
> > 
> > Hmm? What is supposed to wake it up then? AFAIK the whole idea of any
> > idle or sleep is to sit in such low power mode until some interrupt
> > fires (and so the name of the WFI, wait for interrupt, instruction).
> 
> It is handled by the hardware, for the exynos it should be the PMU. The
> CPU stays clock/power gated and when an interrupt occurs the PMU wakes
> up the CPU. This one continue its instructions after cpu_do_idle and
> right after enables the local interrupts leading to the interrupt
> handling.

OK. I misunderstood you previously then, taking interrupts disabled as 
interrupts signals masked in the controller.

> >>>> 2. The CPU1 hotplug bug should been fixed and if I am not wrong
> >>>> there
> >>>> is
> >>>> a patch somewhere fixing this.
> >>>> 
> >>>> https://bugs.launchpad.net/linaro-power-kernel/+bug/1171382
> >>> 
> >>> Unfortunately none of the patches there helps with my issues.
> >>> 
> >>>> 3. What is the fix for Exynos5 ?
> >>>> 
> >>>> https://bugs.launchpad.net/linaro-power-kernel/+bug/1171253
> >>>> 
> >>>> It sounds like depending on Hypervisor mode is enabled or not, the
> >>>> AFTR
> >>>> does not work correctly.
> >>> 
> >>> Sorry no idea here either.  On any SoCs later than EXYNOS4210 the
> >>> registers used for s3c_cpu_resume address and 0xFCBA0D10 magic number
> >>> may be different than EXYNOS4210 defaults (at least on EXYNOS4412
> >>> they
> >>> indeed are different, unfortunately I lack any info needed for
> >>> EXYNOS5
> >>> support). You are lucky that it even works in some cases on
> >>> EXYNOS5250.
> >>> 
> >>>> In other words, instead of removing the AFTR state I suggest to fix
> >>>> it:
> >>>> both core being online, split driver for exynos4 and 5.
> >>> 
> >>> My main problem is that with the upstream kernel even on EXYNOS4210
> >>> rev0 (only one core useable due to hardware issues) the kernel goes
> >>> into AFTR state for the first few times during boot and then it just
> >>> lockups (after going into cpu_do_idle() which is really
> >>> cpu_v7_do_idle()
> >>> and which does wfi call) and doesn't wake up CPU0. I have currently
> >>> no idea how to fix or debug it further.
> >> 
> >> I have an Origen 4210 board Ver A. and it works without problem with
> >> the
> >> AFTR mode (cpu1 unplugged).
> > 
> > Great!
> > 
> > Since benefits of this feature are rather questionable, especially when
> > you consider all the maintenance burden caused by it, could you do
> > some measurements to check if power saving thanks to this mode is of
> > any significance?
> 
> No I can't, no spare time for that and furthermore this work has already
> be done by Amit Daniel when he submitted the driver.

I was unable to find any measurement results or even any other rationale. 
Would you mind pointing me to them? Thanks in advance.

> Amit Daniel is no longer a Linaro assignee but it is still part of the
> Samsung company (changed the email address to reach him).

OK, thanks.

> >>> The issue happens with every upstream kernel version tried (from v3.4
> >>> to v3.10-rc6).  Lockups also happen on EXYNOS4210 rev1.1 when CPU1 is
> >>> offlined by hand and then cpuidle driver tries to go into AFTR mode
> >>> (because by default it doesn't go into AFTR mode on any SoC except
> >>> EXYNOS4210 rev0).
> >>> 
> >>> I don't have EXYNOS4210 rev1.0 but it seems that in the upstream AFTR
> >>> mode has never worked (even on hardware that it was originally
> >>> developed
> >>> on) since its introduction in v3.4 (which was released on 20th May
> >>> 2012).
> >>> 
> >>> IOW for over the year nobody cared to make it work and I have
> >>> currently
> >>> no fix at hand so the corrent upstream resolution is to just remove
> >>> the
> >>> known non-working code and re-introduce it later when/if needed (I
> >>> can
> >>> just disable it with a small fix but we don't keep such long-term
> >>> broken
> >>> code as placeholder in the upstream kernel).  If left as it is people
> >>> can hit the known issues and waste time debugging them, just like
> >>> this
> >>> happenend for me [1].
> >>> 
> >>> If you have AFTR mode working (especially on EXYNOS4210) in Linaro
> >>> kernels please get fixes upstream ASAP. However I still wonder
> >>> whether
> >>> the maintanance nightmare (bugs for different cases in your
> >>> launchpad)
> >>> is worth gains over standard idle mode as the rumor around here is
> >>> that
> >>> they are not that great (unfortunately no numbers were provided
> >>> during
> >>> original feature addition).
> >> 
> >> It works forme with a vanilla kernel 3.10.0-rc7.
> > 
> > As Bartek already said, I haven't worked on any of our Exynos 4210
> > based
> > boards since it got introduced in Linux 3.4, with exactly the same
> > effect we described.
> > 
> >> Removing a feature because it seems not working is not a good
> >> solution.
> >> The right way is to investigate what is happening and why.
> > 
> > I can agree only partially. Keeping a feature that is broken and
> > without
> > any significant benefits does not make sense for me. Neither does
> > putting efforts into fixing it, only to find that it is of no use.
> > 
> > However this is purely a speculation. Could you test on your Origen, on
> > which it is supposed to work, if this feature is of any use?
> 
> It is useless to do that. This work is already done.

Hmm? As I said, I couldn't find any results of that work.

> The kernel is not a playground where you can upstream code and then
> remove it because a feature seems broken and you don't have an idea of
> why.

Well, first of all, it has not been upstreamed correctly: a) without any 
given rationale (or at least without any I could find) and b) without enough 
testing.

> I asked several times the reasons of why the AFTR state couldn't work
> with multiple CPUs and I had no answer.
> 
> Frankly speaking I have a couple of hypothesis:
> 
> 1. something is not correctly setup and the PMU does not wake up the CPU1
> 2. there is a silicon bug and no one wants to tell it is the case
> 
> In any case, this must be investigated and identified. And then we can
> take a decision about this state.

Well, everything you're saying is correct, assuming that this feature is 
useful, which needs confirmation. I'd still want any evidence of this 
feature being of any use first, to not waste time on something that is 
useless.

Best regards,
Tomasz

[PATCH 1/3] ARM: EXYNOS: remove non-working AFTR mode support

[Daniel Lezcano]

On 06/28/2013 07:31 PM, Tomasz Figa wrote:
> On Friday 28 of June 2013 13:20:09 Daniel Lezcano wrote:

[ ... ]

>> The kernel is not a playground where you can upstream code and then
>> remove it because a feature seems broken and you don't have an idea of
>> why.
> 
> Well, first of all, it has not been upstreamed correctly: a) without any 
> given rationale (or at least without any I could find) and b) without enough 
> testing.

+1

>> I asked several times the reasons of why the AFTR state couldn't work
>> with multiple CPUs and I had no answer.
>>
>> Frankly speaking I have a couple of hypothesis:
>>
>> 1. something is not correctly setup and the PMU does not wake up the CPU1
>> 2. there is a silicon bug and no one wants to tell it is the case
>>
>> In any case, this must be investigated and identified. And then we can
>> take a decision about this state.
> 
> Well, everything you're saying is correct, assuming that this feature is 
> useful, which needs confirmation. I'd still want any evidence of this 
> feature being of any use first, to not waste time on something that is 
> useless.

IMHO, it is useful. That's sure a highly integrated hardware with a lot
of peripherals, the power saving is lost in the noise but with a small
embedded device, the power saving is significant.

What I am worried about is the different SoCs being introduced in this
driver without investigating this cpu1 restriction and it sounds like
the AFTR seems to work (I have an odroid-u2 with a 4412 I will test to
check if the AFTR works on it) and nobody seems to be hurt by this and,
as you stated, there is no rational about it. That means the Exynos will
become really more and more power aggressive with 4/A15 or 8/A15 based
boards if the AFTR state is not correctly supported.

AFAICT, there is also the LPA state and the DEEPSLEEP, right ? If the
AFTR does not work, I don't see these states working too.

The cpuidle driver is critical for the new b.L architecture. If we
aren't able to put a cluster of big cpus in a power down state, we lose
the benefit of all the work made up-front at the scheduler level (eg.
small task packing, workqueue/timer migration) and more generally we
lose the benefit of the b.L architecture (except if we do some hacks
which are orthogonal to the generic approach).

I should have added this point to 4) to the mail I sent previously to
Bartlomiej...

  -- Daniel


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

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

[PATCH 1/3] ARM: EXYNOS: remove non-working AFTR mode support

[Tomasz Figa]

On Saturday 29 of June 2013 00:27:04 Daniel Lezcano wrote:
> On 06/28/2013 07:31 PM, Tomasz Figa wrote:
> > On Friday 28 of June 2013 13:20:09 Daniel Lezcano wrote:
> [ ... ]
> 
> >> The kernel is not a playground where you can upstream code and then
> >> remove it because a feature seems broken and you don't have an idea
> >> of
> >> why.
> > 
> > Well, first of all, it has not been upstreamed correctly: a) without
> > any given rationale (or at least without any I could find) and b)
> > without enough testing.
> 
> +1
> 
> >> I asked several times the reasons of why the AFTR state couldn't work
> >> with multiple CPUs and I had no answer.
> >> 
> >> Frankly speaking I have a couple of hypothesis:
> >> 
> >> 1. something is not correctly setup and the PMU does not wake up the
> >> CPU1 2. there is a silicon bug and no one wants to tell it is the
> >> case
> >> 
> >> In any case, this must be investigated and identified. And then we
> >> can
> >> take a decision about this state.
> > 
> > Well, everything you're saying is correct, assuming that this feature
> > is useful, which needs confirmation. I'd still want any evidence of
> > this feature being of any use first, to not waste time on something
> > that is useless.
> 
> IMHO, it is useful. That's sure a highly integrated hardware with a lot
> of peripherals, the power saving is lost in the noise but with a small
> embedded device, the power saving is significant.

Be aware that we might be talking here about savings of single 
milliamperes, because the highest, dynamic, power consumption is already 
bypassed in WFI idle mode, with stopped core clock.

Our measurements have shown that static leakage is pretty low on SoCs we 
checked, that is Exynos 4210 and Exynos 4412. I don't remember exactly 
ATM, because it was some time ago, but it was something around 2 mA per 
core.

Now it depends on overall power consumption of given system, whether such 
saving is significant or ignorable - in our case it is the latter, because 
consumption caused by rest of the system (think of display, wireless 
radios, etc.) is much higher.

> What I am worried about is the different SoCs being introduced in this
> driver without investigating this cpu1 restriction and it sounds like
> the AFTR seems to work (I have an odroid-u2 with a 4412 I will test to
> check if the AFTR works on it) and nobody seems to be hurt by this and,
> as you stated, there is no rational about it. That means the Exynos will
> become really more and more power aggressive with 4/A15 or 8/A15 based
> boards if the AFTR state is not correctly supported.

See my other reply, for a bit more information about this state and CPU1 
restriction.

> AFAICT, there is also the LPA state and the DEEPSLEEP, right ? If the
> AFTR does not work, I don't see these states working too.

Yes, there are other states as well. With similar execution flow, but 
different set of preserved and lost context, requiring more or less save 
and restore on kernel side.

> The cpuidle driver is critical for the new b.L architecture. If we
> aren't able to put a cluster of big cpus in a power down state, we lose
> the benefit of all the work made up-front at the scheduler level (eg.
> small task packing, workqueue/timer migration) and more generally we
> lose the benefit of the b.L architecture (except if we do some hacks
> which are orthogonal to the generic approach).

In this case just simple power down of CPU cores is enough, which doesn't 
require entering system-wide low power state, such as AFTR and friends. 
Such low power mode is needed only when you want to power off all the 
cores, which makes you lose program execution state.

When at least one of the cores is still active, the whole system is in 
control of kernel and no special care must be taken to bring up cores that 
are down and back, just standard SMP/hotplug ops.

Best regards,
Tomasz

[PATCH 1/3] ARM: EXYNOS: remove non-working AFTR mode support

[Daniel Lezcano]

On 06/29/2013 01:07 AM, Tomasz Figa wrote:
> On Saturday 29 of June 2013 00:27:04 Daniel Lezcano wrote:
>> On 06/28/2013 07:31 PM, Tomasz Figa wrote:
>>> On Friday 28 of June 2013 13:20:09 Daniel Lezcano wrote:
>> [ ... ]
>>
>>>> The kernel is not a playground where you can upstream code and then
>>>> remove it because a feature seems broken and you don't have an idea
>>>> of
>>>> why.
>>>
>>> Well, first of all, it has not been upstreamed correctly: a) without
>>> any given rationale (or at least without any I could find) and b)
>>> without enough testing.
>>
>> +1
>>
>>>> I asked several times the reasons of why the AFTR state couldn't work
>>>> with multiple CPUs and I had no answer.
>>>>
>>>> Frankly speaking I have a couple of hypothesis:
>>>>
>>>> 1. something is not correctly setup and the PMU does not wake up the
>>>> CPU1 2. there is a silicon bug and no one wants to tell it is the
>>>> case
>>>>
>>>> In any case, this must be investigated and identified. And then we
>>>> can
>>>> take a decision about this state.
>>>
>>> Well, everything you're saying is correct, assuming that this feature
>>> is useful, which needs confirmation. I'd still want any evidence of
>>> this feature being of any use first, to not waste time on something
>>> that is useless.
>>
>> IMHO, it is useful. That's sure a highly integrated hardware with a lot
>> of peripherals, the power saving is lost in the noise but with a small
>> embedded device, the power saving is significant.
> 
> Be aware that we might be talking here about savings of single 
> milliamperes, because the highest, dynamic, power consumption is already 
> bypassed in WFI idle mode, with stopped core clock.
> 
> Our measurements have shown that static leakage is pretty low on SoCs we 
> checked, that is Exynos 4210 and Exynos 4412. I don't remember exactly 
> ATM, because it was some time ago, but it was something around 2 mA per 
> core.

Ok I don't know exactly the values but I am a bit surprised by this
small value. When I measured the power consumption with a core offline
it saved roughly 100mA (AFAIR). Of course, there is no dynamic with the
hotplugging but there is really a big difference here.

With another board with similar hardware (but a different SoC), just
putting the cores in retention mode in the cpuidle drivers saved 13mA.

Is it possible for the exynos board, the target residency is too large
for AFTR mode ? Is this value really resulting from a measurement or
just put there arbitrarily ? And, may be, it does not enter enough in
this state to have a significant power saving.

> Now it depends on overall power consumption of given system, whether such 
> saving is significant or ignorable - in our case it is the latter, because 
> consumption caused by rest of the system (think of display, wireless 
> radios, etc.) is much higher.
> 
>> What I am worried about is the different SoCs being introduced in this
>> driver without investigating this cpu1 restriction and it sounds like
>> the AFTR seems to work (I have an odroid-u2 with a 4412 I will test to
>> check if the AFTR works on it) and nobody seems to be hurt by this and,
>> as you stated, there is no rational about it. That means the Exynos will
>> become really more and more power aggressive with 4/A15 or 8/A15 based
>> boards if the AFTR state is not correctly supported.
> 
> See my other reply, for a bit more information about this state and CPU1 
> restriction.
> 
>> AFAICT, there is also the LPA state and the DEEPSLEEP, right ? If the
>> AFTR does not work, I don't see these states working too.
> 
> Yes, there are other states as well. With similar execution flow, but 
> different set of preserved and lost context, requiring more or less save 
> and restore on kernel side.
> 
>> The cpuidle driver is critical for the new b.L architecture. If we
>> aren't able to put a cluster of big cpus in a power down state, we lose
>> the benefit of all the work made up-front at the scheduler level (eg.
>> small task packing, workqueue/timer migration) and more generally we
>> lose the benefit of the b.L architecture (except if we do some hacks
>> which are orthogonal to the generic approach).
> 
> In this case just simple power down of CPU cores is enough, which doesn't 
> require entering system-wide low power state, such as AFTR and friends. 
> Such low power mode is needed only when you want to power off all the 
> cores, which makes you lose program execution state.
> 
> When at least one of the cores is still active, the whole system is in 
> control of kernel and no special care must be taken to bring up cores that 
> are down and back, just standard SMP/hotplug ops.

Yes but unfortunately this approach has been nacked by the community
last year with the OMAP4.


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

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

  * English - detected
  * English
  * French

  * English
  * French

 <javascript:void(0);>

[PATCH 2/3] ARM: EXYNOS: init cpuidle driver in exynos_init_late()

[Bartlomiej Zolnierkiewicz]

Init cpuidle driver in exynos_init_late() instead of using
device_initcall().  This is needed for multiplatform support.

Tested on Exynos4210 (Universal C210 board).

Cc: Jaecheol Lee <jc.lee@samsung.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Amit Daniel Kachhap <amit.kachhap@linaro.org>
Cc: Tomasz Figa <t.figa@samsung.com>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
---
 arch/arm/mach-exynos/common.c  | 8 ++++----
 arch/arm/mach-exynos/common.h  | 6 ++++++
 arch/arm/mach-exynos/cpuidle.c | 3 +--
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/arch/arm/mach-exynos/common.c b/arch/arm/mach-exynos/common.c
index f7e504b..1c78a16 100644
--- a/arch/arm/mach-exynos/common.c
+++ b/arch/arm/mach-exynos/common.c
@@ -346,11 +346,11 @@ void exynos5_restart(char mode, const char *cmd)
 
 void __init exynos_init_late(void)
 {
-	if (of_machine_is_compatible("samsung,exynos5440"))
-		/* to be supported later */
-		return;
+	/* exynos5440 to be supported later */
+	if (!of_machine_is_compatible("samsung,exynos5440"))
+		exynos_pm_late_initcall();
 
-	exynos_pm_late_initcall();
+	exynos4_init_cpuidle();
 }
 
 #ifdef CONFIG_OF
diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
index 11fc1e2..a3b95a6 100644
--- a/arch/arm/mach-exynos/common.h
+++ b/arch/arm/mach-exynos/common.h
@@ -40,6 +40,12 @@ int exynos_pm_late_initcall(void);
 static inline int exynos_pm_late_initcall(void) { return 0; }
 #endif
 
+#ifdef CONFIG_CPU_IDLE
+int exynos4_init_cpuidle(void);
+#else
+static inline int exynos4_init_cpuidle(void) { return 0; }
+#endif
+
 #ifdef CONFIG_ARCH_EXYNOS4
 void exynos4_register_clocks(void);
 void exynos4_setup_clocks(void);
diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
index 0a657ac..d6e5115 100644
--- a/arch/arm/mach-exynos/cpuidle.c
+++ b/arch/arm/mach-exynos/cpuidle.c
@@ -66,7 +66,7 @@ static void __init exynos5_core_down_clk(void)
 	__raw_writel(tmp, EXYNOS5_PWR_CTRL2);
 }
 
-static int __init exynos4_init_cpuidle(void)
+int __init exynos4_init_cpuidle(void)
 {
 	int cpu_id, ret;
 	struct cpuidle_device *device;
@@ -93,4 +93,3 @@ static int __init exynos4_init_cpuidle(void)
 
 	return 0;
 }
-device_initcall(exynos4_init_cpuidle);
-- 
1.8.2.3

[PATCH 2/3] ARM: EXYNOS: init cpuidle driver in exynos_init_late()

[Daniel Lezcano]

On 06/26/2013 12:13 PM, Bartlomiej Zolnierkiewicz wrote:
> Init cpuidle driver in exynos_init_late() instead of using
> device_initcall().  This is needed for multiplatform support.
> 
> Tested on Exynos4210 (Universal C210 board).

Please, find a way to have the same level of initialization than
device_init_call. In the work of consolidating the drivers, there are
all at the device_init_call level.



> Cc: Jaecheol Lee <jc.lee@samsung.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Amit Daniel Kachhap <amit.kachhap@linaro.org>
> Cc: Tomasz Figa <t.figa@samsung.com>
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> ---
>  arch/arm/mach-exynos/common.c  | 8 ++++----
>  arch/arm/mach-exynos/common.h  | 6 ++++++
>  arch/arm/mach-exynos/cpuidle.c | 3 +--
>  3 files changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/mach-exynos/common.c b/arch/arm/mach-exynos/common.c
> index f7e504b..1c78a16 100644
> --- a/arch/arm/mach-exynos/common.c
> +++ b/arch/arm/mach-exynos/common.c
> @@ -346,11 +346,11 @@ void exynos5_restart(char mode, const char *cmd)
>  
>  void __init exynos_init_late(void)
>  {
> -	if (of_machine_is_compatible("samsung,exynos5440"))
> -		/* to be supported later */
> -		return;
> +	/* exynos5440 to be supported later */
> +	if (!of_machine_is_compatible("samsung,exynos5440"))
> +		exynos_pm_late_initcall();
>  
> -	exynos_pm_late_initcall();
> +	exynos4_init_cpuidle();
>  }
>  
>  #ifdef CONFIG_OF
> diff --git a/arch/arm/mach-exynos/common.h b/arch/arm/mach-exynos/common.h
> index 11fc1e2..a3b95a6 100644
> --- a/arch/arm/mach-exynos/common.h
> +++ b/arch/arm/mach-exynos/common.h
> @@ -40,6 +40,12 @@ int exynos_pm_late_initcall(void);
>  static inline int exynos_pm_late_initcall(void) { return 0; }
>  #endif
>  
> +#ifdef CONFIG_CPU_IDLE
> +int exynos4_init_cpuidle(void);
> +#else
> +static inline int exynos4_init_cpuidle(void) { return 0; }
> +#endif
> +
>  #ifdef CONFIG_ARCH_EXYNOS4
>  void exynos4_register_clocks(void);
>  void exynos4_setup_clocks(void);
> diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
> index 0a657ac..d6e5115 100644
> --- a/arch/arm/mach-exynos/cpuidle.c
> +++ b/arch/arm/mach-exynos/cpuidle.c
> @@ -66,7 +66,7 @@ static void __init exynos5_core_down_clk(void)
>  	__raw_writel(tmp, EXYNOS5_PWR_CTRL2);
>  }
>  
> -static int __init exynos4_init_cpuidle(void)
> +int __init exynos4_init_cpuidle(void)
>  {
>  	int cpu_id, ret;
>  	struct cpuidle_device *device;
> @@ -93,4 +93,3 @@ static int __init exynos4_init_cpuidle(void)
>  
>  	return 0;
>  }
> -device_initcall(exynos4_init_cpuidle);
> 


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

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

[PATCH 3/3] ARM: EXYNOS: move cpuidle driver to drivers/cpuidle/

[Bartlomiej Zolnierkiewicz]

While at it:
- remove "common.h" include
- remove wrong comment about file path

Tested on Exynos4210 (Universal C210 board).

Cc: Jaecheol Lee <jc.lee@samsung.com>
Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: Amit Daniel Kachhap <amit.kachhap@linaro.org>
Cc: Tomasz Figa <t.figa@samsung.com>
Cc: Kukjin Kim <kgene.kim@samsung.com>
Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
---
 arch/arm/mach-exynos/Makefile    |  1 -
 arch/arm/mach-exynos/cpuidle.c   | 95 ----------------------------------------
 drivers/cpuidle/Makefile         |  3 ++
 drivers/cpuidle/cpuidle-exynos.c | 92 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 95 insertions(+), 96 deletions(-)
 delete mode 100644 arch/arm/mach-exynos/cpuidle.c
 create mode 100644 drivers/cpuidle/cpuidle-exynos.c

diff --git a/arch/arm/mach-exynos/Makefile b/arch/arm/mach-exynos/Makefile
index b09b027..0f91c7a 100644
--- a/arch/arm/mach-exynos/Makefile
+++ b/arch/arm/mach-exynos/Makefile
@@ -16,7 +16,6 @@ obj-$(CONFIG_ARCH_EXYNOS)	+= common.o
 
 obj-$(CONFIG_PM)		+= pm.o
 obj-$(CONFIG_PM_GENERIC_DOMAINS) += pm_domains.o
-obj-$(CONFIG_CPU_IDLE)		+= cpuidle.o
 
 obj-$(CONFIG_ARCH_EXYNOS)	+= pmu.o
 
diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
deleted file mode 100644
index d6e5115..0000000
--- a/arch/arm/mach-exynos/cpuidle.c
+++ /dev/null
@@ -1,95 +0,0 @@
-/* linux/arch/arm/mach-exynos4/cpuidle.c
- *
- * Copyright (c) 2011 Samsung Electronics Co., Ltd.
- *		http://www.samsung.com
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
-*/
-
-#include <linux/kernel.h>
-#include <linux/init.h>
-#include <linux/cpuidle.h>
-#include <linux/cpu_pm.h>
-#include <linux/io.h>
-#include <linux/export.h>
-#include <linux/time.h>
-
-#include <asm/proc-fns.h>
-#include <asm/cpuidle.h>
-#include <mach/regs-clock.h>
-
-#include "common.h"
-
-static DEFINE_PER_CPU(struct cpuidle_device, exynos4_cpuidle_device);
-
-static struct cpuidle_driver exynos4_idle_driver = {
-	.name			= "exynos4_idle",
-	.owner			= THIS_MODULE,
-	.states = {
-		[0] = ARM_CPUIDLE_WFI_STATE,
-	},
-	.state_count = 1,
-	.safe_state_index = 0,
-};
-
-static void __init exynos5_core_down_clk(void)
-{
-	unsigned int tmp;
-
-	/*
-	 * Enable arm clock down (in idle) and set arm divider
-	 * ratios in WFI/WFE state.
-	 */
-	tmp = PWR_CTRL1_CORE2_DOWN_RATIO | \
-	      PWR_CTRL1_CORE1_DOWN_RATIO | \
-	      PWR_CTRL1_DIV2_DOWN_EN	 | \
-	      PWR_CTRL1_DIV1_DOWN_EN	 | \
-	      PWR_CTRL1_USE_CORE1_WFE	 | \
-	      PWR_CTRL1_USE_CORE0_WFE	 | \
-	      PWR_CTRL1_USE_CORE1_WFI	 | \
-	      PWR_CTRL1_USE_CORE0_WFI;
-	__raw_writel(tmp, EXYNOS5_PWR_CTRL1);
-
-	/*
-	 * Enable arm clock up (on exiting idle). Set arm divider
-	 * ratios when not in idle along with the standby duration
-	 * ratios.
-	 */
-	tmp = PWR_CTRL2_DIV2_UP_EN	 | \
-	      PWR_CTRL2_DIV1_UP_EN	 | \
-	      PWR_CTRL2_DUR_STANDBY2_VAL | \
-	      PWR_CTRL2_DUR_STANDBY1_VAL | \
-	      PWR_CTRL2_CORE2_UP_RATIO	 | \
-	      PWR_CTRL2_CORE1_UP_RATIO;
-	__raw_writel(tmp, EXYNOS5_PWR_CTRL2);
-}
-
-int __init exynos4_init_cpuidle(void)
-{
-	int cpu_id, ret;
-	struct cpuidle_device *device;
-
-	if (soc_is_exynos5250())
-		exynos5_core_down_clk();
-
-	ret = cpuidle_register_driver(&exynos4_idle_driver);
-	if (ret) {
-		printk(KERN_ERR "CPUidle failed to register driver\n");
-		return ret;
-	}
-
-	for_each_online_cpu(cpu_id) {
-		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
-		device->cpu = cpu_id;
-
-		ret = cpuidle_register_device(device);
-		if (ret) {
-			printk(KERN_ERR "CPUidle register device failed\n");
-			return ret;
-		}
-	}
-
-	return 0;
-}
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index 0d8bd55..37c6178 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -6,4 +6,7 @@ obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
 obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o
 
 obj-$(CONFIG_CPU_IDLE_CALXEDA) += cpuidle-calxeda.o
+ifeq ($(CONFIG_ARCH_EXYNOS),y)
+	obj-y += cpuidle-exynos.o
+endif
 obj-$(CONFIG_ARCH_KIRKWOOD) += cpuidle-kirkwood.o
diff --git a/drivers/cpuidle/cpuidle-exynos.c b/drivers/cpuidle/cpuidle-exynos.c
new file mode 100644
index 0000000..1c592d2
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-exynos.c
@@ -0,0 +1,92 @@
+/*
+ * Copyright (c) 2011 Samsung Electronics Co., Ltd.
+ *		http://www.samsung.com
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+*/
+
+#include <linux/kernel.h>
+#include <linux/init.h>
+#include <linux/cpuidle.h>
+#include <linux/cpu_pm.h>
+#include <linux/io.h>
+#include <linux/export.h>
+#include <linux/time.h>
+
+#include <asm/proc-fns.h>
+#include <asm/cpuidle.h>
+#include <mach/regs-clock.h>
+
+static DEFINE_PER_CPU(struct cpuidle_device, exynos4_cpuidle_device);
+
+static struct cpuidle_driver exynos4_idle_driver = {
+	.name			= "exynos4_idle",
+	.owner			= THIS_MODULE,
+	.states = {
+		[0] = ARM_CPUIDLE_WFI_STATE,
+	},
+	.state_count = 1,
+	.safe_state_index = 0,
+};
+
+static void __init exynos5_core_down_clk(void)
+{
+	unsigned int tmp;
+
+	/*
+	 * Enable arm clock down (in idle) and set arm divider
+	 * ratios in WFI/WFE state.
+	 */
+	tmp = PWR_CTRL1_CORE2_DOWN_RATIO | \
+	      PWR_CTRL1_CORE1_DOWN_RATIO | \
+	      PWR_CTRL1_DIV2_DOWN_EN	 | \
+	      PWR_CTRL1_DIV1_DOWN_EN	 | \
+	      PWR_CTRL1_USE_CORE1_WFE	 | \
+	      PWR_CTRL1_USE_CORE0_WFE	 | \
+	      PWR_CTRL1_USE_CORE1_WFI	 | \
+	      PWR_CTRL1_USE_CORE0_WFI;
+	__raw_writel(tmp, EXYNOS5_PWR_CTRL1);
+
+	/*
+	 * Enable arm clock up (on exiting idle). Set arm divider
+	 * ratios when not in idle along with the standby duration
+	 * ratios.
+	 */
+	tmp = PWR_CTRL2_DIV2_UP_EN	 | \
+	      PWR_CTRL2_DIV1_UP_EN	 | \
+	      PWR_CTRL2_DUR_STANDBY2_VAL | \
+	      PWR_CTRL2_DUR_STANDBY1_VAL | \
+	      PWR_CTRL2_CORE2_UP_RATIO	 | \
+	      PWR_CTRL2_CORE1_UP_RATIO;
+	__raw_writel(tmp, EXYNOS5_PWR_CTRL2);
+}
+
+int __init exynos4_init_cpuidle(void)
+{
+	int cpu_id, ret;
+	struct cpuidle_device *device;
+
+	if (soc_is_exynos5250())
+		exynos5_core_down_clk();
+
+	ret = cpuidle_register_driver(&exynos4_idle_driver);
+	if (ret) {
+		printk(KERN_ERR "CPUidle failed to register driver\n");
+		return ret;
+	}
+
+	for_each_online_cpu(cpu_id) {
+		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
+		device->cpu = cpu_id;
+
+		ret = cpuidle_register_device(device);
+		if (ret) {
+			printk(KERN_ERR "CPUidle register device failed\n");
+			return ret;
+		}
+	}
+
+	return 0;
+}
-- 
1.8.2.3

[PATCH 3/3] ARM: EXYNOS: move cpuidle driver to drivers/cpuidle/

[Daniel Lezcano]

On 06/26/2013 12:13 PM, Bartlomiej Zolnierkiewicz wrote:
> While at it:
> - remove "common.h" include
> - remove wrong comment about file path
> 
> Tested on Exynos4210 (Universal C210 board).

I agree to move the driver to the drivers/cpuidle, but the header
#include <mach/regs-clock.h> shouldn't appear in this file.

There were several attempts to move the cpuidle drivers to this
directory [1]  and we agree to separate the pm code from the driver
code. A bit of code encapsulation should be done here before.

Thanks
  -- Daniel

ps : "use -M to generate the patch" when moving files from one directory
to another one.

[1] https://patchwork.kernel.org/patch/2445191/

> Cc: Jaecheol Lee <jc.lee@samsung.com>
> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Cc: Amit Daniel Kachhap <amit.kachhap@linaro.org>
> Cc: Tomasz Figa <t.figa@samsung.com>
> Cc: Kukjin Kim <kgene.kim@samsung.com>
> Cc: Daniel Lezcano <daniel.lezcano@linaro.org>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
> ---
>  arch/arm/mach-exynos/Makefile    |  1 -
>  arch/arm/mach-exynos/cpuidle.c   | 95 ----------------------------------------
>  drivers/cpuidle/Makefile         |  3 ++
>  drivers/cpuidle/cpuidle-exynos.c | 92 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 95 insertions(+), 96 deletions(-)
>  delete mode 100644 arch/arm/mach-exynos/cpuidle.c
>  create mode 100644 drivers/cpuidle/cpuidle-exynos.c
> 
> diff --git a/arch/arm/mach-exynos/Makefile b/arch/arm/mach-exynos/Makefile
> index b09b027..0f91c7a 100644
> --- a/arch/arm/mach-exynos/Makefile
> +++ b/arch/arm/mach-exynos/Makefile
> @@ -16,7 +16,6 @@ obj-$(CONFIG_ARCH_EXYNOS)	+= common.o
>  
>  obj-$(CONFIG_PM)		+= pm.o
>  obj-$(CONFIG_PM_GENERIC_DOMAINS) += pm_domains.o
> -obj-$(CONFIG_CPU_IDLE)		+= cpuidle.o
>  
>  obj-$(CONFIG_ARCH_EXYNOS)	+= pmu.o
>  
> diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c
> deleted file mode 100644
> index d6e5115..0000000
> --- a/arch/arm/mach-exynos/cpuidle.c
> +++ /dev/null
> @@ -1,95 +0,0 @@
> -/* linux/arch/arm/mach-exynos4/cpuidle.c
> - *
> - * Copyright (c) 2011 Samsung Electronics Co., Ltd.
> - *		http://www.samsung.com
> - *
> - * This program is free software; you can redistribute it and/or modify
> - * it under the terms of the GNU General Public License version 2 as
> - * published by the Free Software Foundation.
> -*/
> -
> -#include <linux/kernel.h>
> -#include <linux/init.h>
> -#include <linux/cpuidle.h>
> -#include <linux/cpu_pm.h>
> -#include <linux/io.h>
> -#include <linux/export.h>
> -#include <linux/time.h>
> -
> -#include <asm/proc-fns.h>
> -#include <asm/cpuidle.h>
> -#include <mach/regs-clock.h>
> -
> -#include "common.h"
> -
> -static DEFINE_PER_CPU(struct cpuidle_device, exynos4_cpuidle_device);
> -
> -static struct cpuidle_driver exynos4_idle_driver = {
> -	.name			= "exynos4_idle",
> -	.owner			= THIS_MODULE,
> -	.states = {
> -		[0] = ARM_CPUIDLE_WFI_STATE,
> -	},
> -	.state_count = 1,
> -	.safe_state_index = 0,
> -};
> -
> -static void __init exynos5_core_down_clk(void)
> -{
> -	unsigned int tmp;
> -
> -	/*
> -	 * Enable arm clock down (in idle) and set arm divider
> -	 * ratios in WFI/WFE state.
> -	 */
> -	tmp = PWR_CTRL1_CORE2_DOWN_RATIO | \
> -	      PWR_CTRL1_CORE1_DOWN_RATIO | \
> -	      PWR_CTRL1_DIV2_DOWN_EN	 | \
> -	      PWR_CTRL1_DIV1_DOWN_EN	 | \
> -	      PWR_CTRL1_USE_CORE1_WFE	 | \
> -	      PWR_CTRL1_USE_CORE0_WFE	 | \
> -	      PWR_CTRL1_USE_CORE1_WFI	 | \
> -	      PWR_CTRL1_USE_CORE0_WFI;
> -	__raw_writel(tmp, EXYNOS5_PWR_CTRL1);
> -
> -	/*
> -	 * Enable arm clock up (on exiting idle). Set arm divider
> -	 * ratios when not in idle along with the standby duration
> -	 * ratios.
> -	 */
> -	tmp = PWR_CTRL2_DIV2_UP_EN	 | \
> -	      PWR_CTRL2_DIV1_UP_EN	 | \
> -	      PWR_CTRL2_DUR_STANDBY2_VAL | \
> -	      PWR_CTRL2_DUR_STANDBY1_VAL | \
> -	      PWR_CTRL2_CORE2_UP_RATIO	 | \
> -	      PWR_CTRL2_CORE1_UP_RATIO;
> -	__raw_writel(tmp, EXYNOS5_PWR_CTRL2);
> -}
> -
> -int __init exynos4_init_cpuidle(void)
> -{
> -	int cpu_id, ret;
> -	struct cpuidle_device *device;
> -
> -	if (soc_is_exynos5250())
> -		exynos5_core_down_clk();
> -
> -	ret = cpuidle_register_driver(&exynos4_idle_driver);
> -	if (ret) {
> -		printk(KERN_ERR "CPUidle failed to register driver\n");
> -		return ret;
> -	}
> -
> -	for_each_online_cpu(cpu_id) {
> -		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
> -		device->cpu = cpu_id;
> -
> -		ret = cpuidle_register_device(device);
> -		if (ret) {
> -			printk(KERN_ERR "CPUidle register device failed\n");
> -			return ret;
> -		}
> -	}
> -
> -	return 0;
> -}
> diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
> index 0d8bd55..37c6178 100644
> --- a/drivers/cpuidle/Makefile
> +++ b/drivers/cpuidle/Makefile
> @@ -6,4 +6,7 @@ obj-y += cpuidle.o driver.o governor.o sysfs.o governors/
>  obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o
>  
>  obj-$(CONFIG_CPU_IDLE_CALXEDA) += cpuidle-calxeda.o
> +ifeq ($(CONFIG_ARCH_EXYNOS),y)
> +	obj-y += cpuidle-exynos.o
> +endif
>  obj-$(CONFIG_ARCH_KIRKWOOD) += cpuidle-kirkwood.o
> diff --git a/drivers/cpuidle/cpuidle-exynos.c b/drivers/cpuidle/cpuidle-exynos.c
> new file mode 100644
> index 0000000..1c592d2
> --- /dev/null
> +++ b/drivers/cpuidle/cpuidle-exynos.c
> @@ -0,0 +1,92 @@
> +/*
> + * Copyright (c) 2011 Samsung Electronics Co., Ltd.
> + *		http://www.samsung.com
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> +*/
> +
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/cpuidle.h>
> +#include <linux/cpu_pm.h>
> +#include <linux/io.h>
> +#include <linux/export.h>
> +#include <linux/time.h>
> +
> +#include <asm/proc-fns.h>
> +#include <asm/cpuidle.h>
> +#include <mach/regs-clock.h>
> +
> +static DEFINE_PER_CPU(struct cpuidle_device, exynos4_cpuidle_device);
> +
> +static struct cpuidle_driver exynos4_idle_driver = {
> +	.name			= "exynos4_idle",
> +	.owner			= THIS_MODULE,
> +	.states = {
> +		[0] = ARM_CPUIDLE_WFI_STATE,
> +	},
> +	.state_count = 1,
> +	.safe_state_index = 0,
> +};
> +
> +static void __init exynos5_core_down_clk(void)
> +{
> +	unsigned int tmp;
> +
> +	/*
> +	 * Enable arm clock down (in idle) and set arm divider
> +	 * ratios in WFI/WFE state.
> +	 */
> +	tmp = PWR_CTRL1_CORE2_DOWN_RATIO | \
> +	      PWR_CTRL1_CORE1_DOWN_RATIO | \
> +	      PWR_CTRL1_DIV2_DOWN_EN	 | \
> +	      PWR_CTRL1_DIV1_DOWN_EN	 | \
> +	      PWR_CTRL1_USE_CORE1_WFE	 | \
> +	      PWR_CTRL1_USE_CORE0_WFE	 | \
> +	      PWR_CTRL1_USE_CORE1_WFI	 | \
> +	      PWR_CTRL1_USE_CORE0_WFI;
> +	__raw_writel(tmp, EXYNOS5_PWR_CTRL1);
> +
> +	/*
> +	 * Enable arm clock up (on exiting idle). Set arm divider
> +	 * ratios when not in idle along with the standby duration
> +	 * ratios.
> +	 */
> +	tmp = PWR_CTRL2_DIV2_UP_EN	 | \
> +	      PWR_CTRL2_DIV1_UP_EN	 | \
> +	      PWR_CTRL2_DUR_STANDBY2_VAL | \
> +	      PWR_CTRL2_DUR_STANDBY1_VAL | \
> +	      PWR_CTRL2_CORE2_UP_RATIO	 | \
> +	      PWR_CTRL2_CORE1_UP_RATIO;
> +	__raw_writel(tmp, EXYNOS5_PWR_CTRL2);
> +}
> +
> +int __init exynos4_init_cpuidle(void)
> +{
> +	int cpu_id, ret;
> +	struct cpuidle_device *device;
> +
> +	if (soc_is_exynos5250())
> +		exynos5_core_down_clk();
> +
> +	ret = cpuidle_register_driver(&exynos4_idle_driver);
> +	if (ret) {
> +		printk(KERN_ERR "CPUidle failed to register driver\n");
> +		return ret;
> +	}
> +
> +	for_each_online_cpu(cpu_id) {
> +		device = &per_cpu(exynos4_cpuidle_device, cpu_id);
> +		device->cpu = cpu_id;
> +
> +		ret = cpuidle_register_device(device);
> +		if (ret) {
> +			printk(KERN_ERR "CPUidle register device failed\n");
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> 


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

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