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

[Daniel Lezcano <daniel.lezcano@linaro.org>]

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