Re: [PATCH v2 5/6] ARM: EXYNOS: Fix suspend/resume sequences

[Tomasz Figa <t.figa@samsung.com>]

On 15.07.2014 13:19, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi,
> 
> On Monday, July 14, 2014 11:54:48 AM Tomasz Figa wrote:
>> Hi Kukjin,
>>
>> On 25.06.2014 13:52, Tomasz Figa wrote:
>>> Due to recent consolidation of Exynos suspend and cpuidle code, some
>>> parts of suspend and resume sequences are executed two times, once from
>>> exynos_pm_syscore_ops and then from exynos_cpu_pm_notifier() and thus it
>>> breaks suspend, at least on Exynos4-based boards.
>>>
>>> This patch fixes the issue by removing exynos_pm_syscore_ops completely
>>> and making the code rely only on CPU PM notifier.
>>>
>>> Tested on Exynos4210-based Trats board.
>>>
>>> Signed-off-by: Tomasz Figa <t.figa@samsung.com>
>>> ---
>>>  arch/arm/mach-exynos/pm.c | 23 ++++-------------------
>>>  1 file changed, 4 insertions(+), 19 deletions(-)
>>>
>>> Changes since v1:
>>>  - rebased onto Kukjin's fixes branch.
>>>
>>> diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c
>>> index 202ca73..f23cc77 100644
>>> --- a/arch/arm/mach-exynos/pm.c
>>> +++ b/arch/arm/mach-exynos/pm.c
>>> @@ -364,11 +364,6 @@ early_wakeup:
>>>  	return;
>>>  }
>>>  
>>> -static struct syscore_ops exynos_pm_syscore_ops = {
>>> -	.suspend	= exynos_pm_suspend,
>>> -	.resume		= exynos_pm_resume,
>>> -};
>>> -
>>>  /*
>>>   * Suspend Ops
>>>   */
>>> @@ -438,22 +433,13 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self,
>>>  
>>>  	switch (cmd) {
>>>  	case CPU_PM_ENTER:
>>> -		if (cpu == 0) {
>>> -			exynos_pm_central_suspend();
>>> -			if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9)
>>> -				exynos_cpu_save_register();
>>> -		}
>>> +		if (cpu == 0)
>>> +			exynos_pm_suspend();
>>>  		break;
>>>  
>>>  	case CPU_PM_EXIT:
>>> -		if (cpu == 0) {
>>> -			if (read_cpuid_part_number() ==
>>> -					ARM_CPU_PART_CORTEX_A9) {
>>> -				scu_enable(S5P_VA_SCU);
>>> -				exynos_cpu_restore_register();
>>> -			}
>>> -			exynos_pm_central_resume();
>>> -		}
>>> +		if (cpu == 0)
>>> +			exynos_pm_resume();
>>>  		break;
>>>  	}
>>>  
>>> @@ -478,6 +464,5 @@ void __init exynos_pm_init(void)
>>>  	tmp |= ((0xFF << 8) | (0x1F << 1));
>>>  	__raw_writel(tmp, S5P_WAKEUP_MASK);
>>>  
>>> -	register_syscore_ops(&exynos_pm_syscore_ops);
>>>  	suspend_set_ops(&exynos_suspend_ops);
>>>  }
>>>
>>
>> Please consider this patch for next fixes pull request. Without it
>> suspend/resume is broken for Exynos4 and probably other SoCs. This patch
>> just restores the sequence from before the patch moving things to PM
>> notifier, so I don't think it should need any special treatment.
> 
> Your patch fixes the regression and is a step in the good direction but it
> seems that it needs a bit more work:
> 
> Your patch adds to cpuidle AFTR code path restoring of exynos_core_save and
> exynos5_sys_save registers without saving them first (restoring is done
> through exynos_pm_resume() which is used by both suspend and cpuidle while
> saving is done through exynos_pm_prepare() which is used only by suspend). 

That's right, unfortunately. Interestingly enough AFTR worked fine on
Exynos4210 with this patch, but still this needs to be fixed.

Now, in fact, I recall that Chander had some objections to this patch as
well and we decided that he will send another patch to replace mine [1],
but I haven't heard from him since that.

Chander, do you have any progress with this? Keep in mind that we need
this as an rc fix and we already have rc5, so not much time left. If you
don't have time to work on this I can take care of this, proceeding as
we discussed in [1].

[1] http://thread.gmane.org/gmane.linux.kernel.samsung-soc/29085/focus=33975

Best regards,
Tomasz