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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxxxx>
>>>>> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
>>>>> Cc: Amit Daniel Kachhap <amit.kachhap@xxxxxxxxxx>
>>>>> Cc: Tomasz Figa <t.figa@xxxxxxxxxxx>
>>>>> Cc: Kukjin Kim <kgene.kim@xxxxxxxxxxx>
>>>>> Cc: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
>>>>> Cc: "Rafael J. Wysocki" <rjw@xxxxxxx>
>>>>> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
>>>>> Signed-off-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@xxxxxxxxxxx>
>>>>> ---
>>>>>
>>>>>  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

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




[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux