From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754655AbaHEMlx (ORCPT ); Tue, 5 Aug 2014 08:41:53 -0400 Received: from mailout3.samsung.com ([203.254.224.33]:58922 "EHLO mailout3.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753048AbaHEMlv (ORCPT ); Tue, 5 Aug 2014 08:41:51 -0400 X-AuditID: cbfee61a-f79e46d00000134f-a8-53e0d10cb874 From: Bartlomiej Zolnierkiewicz To: Kukjin Kim Cc: linux-samsung-soc@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Marek Szyprowski , Tomasz Figa , Arnd Bergmann , Olof Johansson , arm@kernel.org Subject: Re: [PATCH v4][next-20140804] ARM: EXYNOS: Fix suspend/resume sequences Date: Tue, 05 Aug 2014 14:41:46 +0200 Message-id: <5916828.8hQVGW3cAr@amdc1032> User-Agent: KMail/4.8.4 (Linux/3.2.0-54-generic-pae; KDE/4.8.5; i686; ; ) In-reply-to: <1634593.NW3m9xbfbC@amdc1032> References: <1634593.NW3m9xbfbC@amdc1032> MIME-version: 1.0 Content-transfer-encoding: 7Bit Content-type: text/plain; charset=us-ascii X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrBLMWRmVeSWpSXmKPExsVy+t9jQV3eiw+CDSYuYLY49mULm8XfScfY LXoXXGWz2PT4GqvF5V1z2CxmnN/HZLH2yF12i1PXP7NZrJ/xmsWB0+P3r0mMHptWdbJ5bF5S 73HlRBOrR9+WVYwenzfJBbBFcdmkpOZklqUW6dslcGXcmX6MqWCBW8WMh29YGxgfW3QxcnJI CJhITJl6lA3CFpO4cG89kM3FISQwnVHi9KWdTCAJIYEWJonbiypBbDYBK4mJ7asYQWwRATWJ nsVbGUEamAX6mCSmTekCSwgLBEi0tn0HaubgYBFQlbixnhUkzCugKbFl/zqwZaICnhI7tq8E szkFtCSO9T1kBSkXAqrpWMEDUS4o8WPyPRYQm1lAXmLf/qmsELaWxPqdx5kmMArMQlI2C0nZ LCRlCxiZVzGKphYkFxQnpeca6hUn5haX5qXrJefnbmIEh/8zqR2MKxssDjEKcDAq8fAKqN0P FmJNLCuuzD3EKMHBrCTCO+H8g2Ah3pTEyqrUovz4otKc1OJDjNIcLErivAdarQOFBNITS1Kz U1MLUotgskwcnFINjOY5ewOVdj783i3/akrNalWONbU2JYayh9Onb3mRtVUk8fbPUvZ9f6pe n504W+jtFa/FJzjnNj06cGploHP93+jHGq439gW9b/juOH3BrBO9izQ/r94be+21vsAJw5x/ On7VFue0OEMndsbenDIvZcqMDfZs3rH+R+0Dv9cd2r173s7pUj83nbuixFKckWioxVxUnAgA NhfN1XsCAAA= Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, I'm very sorry but this version also has a problem as it still uses read_cpuid_part_number() instead of read_cpuid_part(). v5 (which really should be the final one) to be posted in a minute. Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics On Monday, August 04, 2014 04:40:20 PM Bartlomiej Zolnierkiewicz wrote: > From: Tomasz Figa > > 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. In addition, simple > core power down from a cpuidle driver could, in case of CPU 0 could > result in calling functions that are specific to suspend and deeper idle > states. > > This patch fixes the issue by moving those operations outside the CPU PM > notifier into suspend and AFTR code paths. This leads to a bit of code > duplication, but allows additional code simplification, so in the end > more code is removed than added. > > Fixes: 85f9f90808b4 ("ARM: EXYNOS: Use the cpu_pm notifier for pm") > Cc: Kukjin Kim > Cc: Arnd Bergmann > Cc: Olof Johansson > Cc: arm@kernel.org > Signed-off-by: Tomasz Figa > [b.zolnierkie: ported patch over current changes] > [b.zolnierkie: fixed exynos_aftr_finisher() return value] > Signed-off-by: Bartlomiej Zolnierkiewicz > --- > arch/arm/mach-exynos/pm.c | 163 ++++++++++++++++++--------------------- > drivers/cpuidle/cpuidle-exynos.c | 25 ----- > 2 files changed, 80 insertions(+), 108 deletions(-) > > v4: > - fixed exynos_aftr_finisher() return value > > Index: b/arch/arm/mach-exynos/pm.c > =================================================================== > --- a/arch/arm/mach-exynos/pm.c 2014-08-04 16:36:18.927125851 +0200 > +++ b/arch/arm/mach-exynos/pm.c 2014-08-04 16:36:38.559126369 +0200 > @@ -114,26 +114,6 @@ static int exynos_irq_set_wake(struct ir > #define S5P_CHECK_AFTR 0xFCBA0D10 > #define S5P_CHECK_SLEEP 0x00000BAD > > -/* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */ > -static void exynos_set_wakeupmask(long mask) > -{ > - pmu_raw_writel(mask, S5P_WAKEUP_MASK); > -} > - > -static void exynos_cpu_set_boot_vector(long flags) > -{ > - __raw_writel(virt_to_phys(exynos_cpu_resume), EXYNOS_BOOT_VECTOR_ADDR); > - __raw_writel(flags, EXYNOS_BOOT_VECTOR_FLAG); > -} > - > -void exynos_enter_aftr(void) > -{ > - exynos_set_wakeupmask(0x0000ff3e); > - exynos_cpu_set_boot_vector(S5P_CHECK_AFTR); > - /* Set value of power down register for aftr mode */ > - exynos_sys_powerdown_conf(SYS_AFTR); > -} > - > /* For Cortex-A9 Diagnostic and Power control register */ > static unsigned int save_arm_register[2]; > > @@ -173,6 +153,82 @@ static void exynos_cpu_restore_register( > : "cc"); > } > > +static void exynos_pm_central_suspend(void) > +{ > + unsigned long tmp; > + > + /* Setting Central Sequence Register for power down mode */ > + tmp = pmu_raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION); > + tmp &= ~S5P_CENTRAL_LOWPWR_CFG; > + pmu_raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION); > +} > + > +static int exynos_pm_central_resume(void) > +{ > + unsigned long tmp; > + > + /* > + * 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 = pmu_raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION); > + if (!(tmp & S5P_CENTRAL_LOWPWR_CFG)) { > + tmp |= S5P_CENTRAL_LOWPWR_CFG; > + pmu_raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION); > + /* clear the wakeup state register */ > + pmu_raw_writel(0x0, S5P_WAKEUP_STAT); > + /* No need to perform below restore code */ > + return -1; > + } > + > + return 0; > +} > + > +/* Ext-GIC nIRQ/nFIQ is the only wakeup source in AFTR */ > +static void exynos_set_wakeupmask(long mask) > +{ > + pmu_raw_writel(mask, S5P_WAKEUP_MASK); > +} > + > +static void exynos_cpu_set_boot_vector(long flags) > +{ > + __raw_writel(virt_to_phys(exynos_cpu_resume), EXYNOS_BOOT_VECTOR_ADDR); > + __raw_writel(flags, EXYNOS_BOOT_VECTOR_FLAG); > +} > + > +static int exynos_aftr_finisher(unsigned long flags) > +{ > + exynos_set_wakeupmask(0x0000ff3e); > + exynos_cpu_set_boot_vector(S5P_CHECK_AFTR); > + /* Set value of power down register for aftr mode */ > + exynos_sys_powerdown_conf(SYS_AFTR); > + cpu_do_idle(); > + > + return 1; > +} > + > +void exynos_enter_aftr(void) > +{ > + cpu_pm_enter(); > + > + exynos_pm_central_suspend(); > + if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9) > + exynos_cpu_save_register(); > + > + cpu_suspend(0, exynos_aftr_finisher); > + > + if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9) { > + scu_enable(S5P_VA_SCU); > + exynos_cpu_restore_register(); > + } > + > + exynos_pm_central_resume(); > + > + cpu_pm_exit(); > +} > + > static int exynos_cpu_suspend(unsigned long arg) > { > #ifdef CONFIG_CACHE_L2X0 > @@ -217,16 +273,6 @@ static void exynos_pm_prepare(void) > pmu_raw_writel(virt_to_phys(exynos_cpu_resume), S5P_INFORM0); > } > > -static void exynos_pm_central_suspend(void) > -{ > - unsigned long tmp; > - > - /* Setting Central Sequence Register for power down mode */ > - tmp = pmu_raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION); > - tmp &= ~S5P_CENTRAL_LOWPWR_CFG; > - pmu_raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION); > -} > - > static int exynos_pm_suspend(void) > { > unsigned long tmp; > @@ -244,29 +290,6 @@ static int exynos_pm_suspend(void) > return 0; > } > > -static int exynos_pm_central_resume(void) > -{ > - unsigned long tmp; > - > - /* > - * 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 = pmu_raw_readl(S5P_CENTRAL_SEQ_CONFIGURATION); > - if (!(tmp & S5P_CENTRAL_LOWPWR_CFG)) { > - tmp |= S5P_CENTRAL_LOWPWR_CFG; > - pmu_raw_writel(tmp, S5P_CENTRAL_SEQ_CONFIGURATION); > - /* clear the wakeup state register */ > - pmu_raw_writel(0x0, S5P_WAKEUP_STAT); > - /* No need to perform below restore code */ > - return -1; > - } > - > - return 0; > -} > - > static void exynos_pm_resume(void) > { > if (exynos_pm_central_resume()) > @@ -369,44 +392,10 @@ static const struct platform_suspend_ops > .valid = suspend_valid_only_mem, > }; > > -static int exynos_cpu_pm_notifier(struct notifier_block *self, > - unsigned long cmd, void *v) > -{ > - int cpu = smp_processor_id(); > - > - switch (cmd) { > - case CPU_PM_ENTER: > - if (cpu == 0) { > - exynos_pm_central_suspend(); > - if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) > - exynos_cpu_save_register(); > - } > - break; > - > - case CPU_PM_EXIT: > - if (cpu == 0) { > - if (read_cpuid_part() == ARM_CPU_PART_CORTEX_A9) { > - scu_enable(S5P_VA_SCU); > - exynos_cpu_restore_register(); > - } > - exynos_pm_central_resume(); > - } > - break; > - } > - > - return NOTIFY_OK; > -} > - > -static struct notifier_block exynos_cpu_pm_notifier_block = { > - .notifier_call = exynos_cpu_pm_notifier, > -}; > - > void __init exynos_pm_init(void) > { > u32 tmp; > > - cpu_pm_register_notifier(&exynos_cpu_pm_notifier_block); > - > /* Platform-specific GIC callback */ > gic_arch_extn.irq_set_wake = exynos_irq_set_wake; > > Index: b/drivers/cpuidle/cpuidle-exynos.c > =================================================================== > --- a/drivers/cpuidle/cpuidle-exynos.c 2014-08-04 16:36:18.911125850 +0200 > +++ b/drivers/cpuidle/cpuidle-exynos.c 2014-08-04 16:36:22.783125951 +0200 > @@ -20,25 +20,6 @@ > > static void (*exynos_enter_aftr)(void); > > -static int idle_finisher(unsigned long flags) > -{ > - exynos_enter_aftr(); > - cpu_do_idle(); > - > - return 1; > -} > - > -static int exynos_enter_core0_aftr(struct cpuidle_device *dev, > - struct cpuidle_driver *drv, > - int index) > -{ > - cpu_pm_enter(); > - cpu_suspend(0, idle_finisher); > - cpu_pm_exit(); > - > - return index; > -} > - > static int exynos_enter_lowpower(struct cpuidle_device *dev, > struct cpuidle_driver *drv, > int index) > @@ -51,8 +32,10 @@ static int exynos_enter_lowpower(struct > > if (new_index == 0) > return arm_cpuidle_simple_enter(dev, drv, new_index); > - else > - return exynos_enter_core0_aftr(dev, drv, new_index); > + > + exynos_enter_aftr(); > + > + return new_index; > } > > static struct cpuidle_driver exynos_idle_driver = {