| Message ID | 1397212815-16068-17-git-send-email-daniel.lezcano@linaro.org |
|---|---|
| State | New |
| Headers | show |
On Fri, Apr 11, 2014 at 4:10 PM, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > The code to initiate and exit the powerdown sequence is the same in pm.c and > cpuidle.c. > > Let's split the common part in the pm.c and reuse it from the cpu_pm notifier. > > That is one more step forward to make the cpuidle driver arch indenpendant. > > Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org> > Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org> > Reviewed-by: Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com> > --- > arch/arm/mach-exynos/cpuidle.c | 22 ---------------------- > arch/arm/mach-exynos/pm.c | 24 ++++++++++++++++++++---- > 2 files changed, 20 insertions(+), 26 deletions(-) > > diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c > index e6d813d..02609ac 100644 > --- a/arch/arm/mach-exynos/cpuidle.c > +++ b/arch/arm/mach-exynos/cpuidle.c > @@ -28,7 +28,6 @@ > #include <mach/map.h> > > #include "common.h" > -#include "regs-pmu.h" > > static int idle_finisher(unsigned long flags) > { > @@ -42,31 +41,10 @@ static int exynos_enter_core0_aftr(struct cpuidle_device *dev, > struct cpuidle_driver *drv, > int index) > { > - unsigned long tmp; > - > - /* 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); > cpu_pm_exit(); > > - /* > - * 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; > } > > diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c > index 50b6b4d..6d9ef69 100644 > --- a/arch/arm/mach-exynos/pm.c > +++ b/arch/arm/mach-exynos/pm.c > @@ -213,15 +213,21 @@ static void exynos_pm_prepare(void) > __raw_writel(virt_to_phys(exynos_cpu_resume), S5P_INFORM0); > } > > -static int exynos_pm_suspend(void) > +static void exynos_pm_central_suspend(void) > { > unsigned long tmp; > > /* 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); > +} > + > +static int exynos_pm_suspend(void) > +{ > + unsigned long tmp; > + > + exynos_pm_central_suspend(); > > /* Setting SEQ_OPTION register */ > > @@ -234,7 +240,7 @@ static int exynos_pm_suspend(void) > return 0; > } > > -static void exynos_pm_resume(void) > +static int exynos_pm_central_resume(void) > { > unsigned long tmp; > > @@ -251,9 +257,17 @@ static void exynos_pm_resume(void) > /* clear the wakeup state register */ > __raw_writel(0x0, S5P_WAKEUP_STAT); > /* No need to perform below restore code */ > - goto early_wakeup; > + return -1; > } > > + return 0; > +} > + > +static void exynos_pm_resume(void) > +{ > + if (exynos_pm_central_resume()) > + goto early_wakeup; > + > if (!soc_is_exynos5250()) > exynos_cpu_restore_register(); > > @@ -359,6 +373,7 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self, > switch (cmd) { > case CPU_PM_ENTER: > if (cpu == 0) { > + exynos_pm_central_suspend(); > exynos_cpu_save_register(); > } > break; > @@ -368,6 +383,7 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self, > if (!soc_is_exynos5250()) > scu_enable(S5P_VA_SCU); > exynos_cpu_restore_register(); > + exynos_pm_central_resume(); This notifier is called for system wide suspend and cpuidle. In case of Exynos cpuidle only AFTR and LPA state need to program central_sequencer and store/restore the registers. But in 5420 (core-power-down), this is not required, and causing the regression. Hence need to remove this notifier, or need to find a way to differentiate the cpuidle state. > } > break; > } > -- > 1.7.9.5 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Chander, On 26.06.2014 11:07, Chander Kashyap wrote: > On Fri, Apr 11, 2014 at 4:10 PM, Daniel Lezcano > <daniel.lezcano@linaro.org> wrote: [snip] >> @@ -359,6 +373,7 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self, >> switch (cmd) { >> case CPU_PM_ENTER: >> if (cpu == 0) { >> + exynos_pm_central_suspend(); >> exynos_cpu_save_register(); >> } >> break; >> @@ -368,6 +383,7 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self, >> if (!soc_is_exynos5250()) >> scu_enable(S5P_VA_SCU); >> exynos_cpu_restore_register(); >> + exynos_pm_central_resume(); > > This notifier is called for system wide suspend and cpuidle. > > In case of Exynos cpuidle only AFTR and LPA state need to program > central_sequencer and store/restore the registers. > > But in 5420 (core-power-down), this is not required, and causing the regression. > > Hence need to remove this notifier, or need to find a way to > differentiate the cpuidle state. This patch is already present in v3.16. Moreover, Exynos5420 cpuidle has not been merged yet. This means that this issue is not a regression and I believe any further work on this should be carried out as further patches on top of this change. Anyway, this change has introduced a regression, though, but in another area - it broke suspend, at least on Exynos4-based devices, because now certain steps are performed twice. I've sent a patch for 3.16-rc3 to fix this by dropping custom suspend-specific syscore ops, effectively moving most of the handling to CPU PM notifier, which also matches requirements of AFTR and lower power states. See [1]. [1] http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg32935.html However, in this case, moving back to suspend-specific syscore_ops and simply duplicating some code for lower cpuidle states might be a better option. Care to send a patch (fix for 3.16, replacing mine) or I should do it? Best regards, Tomasz -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 26, 2014 at 3:18 PM, Tomasz Figa <t.figa@samsung.com> wrote: > Hi Chander, > > On 26.06.2014 11:07, Chander Kashyap wrote: >> On Fri, Apr 11, 2014 at 4:10 PM, Daniel Lezcano >> <daniel.lezcano@linaro.org> wrote: > > [snip] > >>> @@ -359,6 +373,7 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self, >>> switch (cmd) { >>> case CPU_PM_ENTER: >>> if (cpu == 0) { >>> + exynos_pm_central_suspend(); >>> exynos_cpu_save_register(); >>> } >>> break; >>> @@ -368,6 +383,7 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self, >>> if (!soc_is_exynos5250()) >>> scu_enable(S5P_VA_SCU); >>> exynos_cpu_restore_register(); >>> + exynos_pm_central_resume(); >> >> This notifier is called for system wide suspend and cpuidle. >> >> In case of Exynos cpuidle only AFTR and LPA state need to program >> central_sequencer and store/restore the registers. >> >> But in 5420 (core-power-down), this is not required, and causing the regression. >> >> Hence need to remove this notifier, or need to find a way to >> differentiate the cpuidle state. > > This patch is already present in v3.16. Moreover, Exynos5420 cpuidle has > not been merged yet. This means that this issue is not a regression and > I believe any further work on this should be carried out as further > patches on top of this change. > > Anyway, this change has introduced a regression, though, but in another > area - it broke suspend, at least on Exynos4-based devices, because now > certain steps are performed twice. I've sent a patch for 3.16-rc3 to fix > this by dropping custom suspend-specific syscore ops, effectively moving > most of the handling to CPU PM notifier, which also matches requirements > of AFTR and lower power states. See [1]. > > [1] > http://www.mail-archive.com/linux-samsung-soc@vger.kernel.org/msg32935.html > > However, in this case, moving back to suspend-specific syscore_ops and > simply duplicating some code for lower cpuidle states might be a better > option. Care to send a patch (fix for 3.16, replacing mine) or I should > do it? Yes need to move the common code to lower cpuildle implementation and removing the notifier all together. I will do it and send the patch. > > Best regards, > Tomasz > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/mach-exynos/cpuidle.c b/arch/arm/mach-exynos/cpuidle.c index e6d813d..02609ac 100644 --- a/arch/arm/mach-exynos/cpuidle.c +++ b/arch/arm/mach-exynos/cpuidle.c @@ -28,7 +28,6 @@ #include <mach/map.h> #include "common.h" -#include "regs-pmu.h" static int idle_finisher(unsigned long flags) { @@ -42,31 +41,10 @@ static int exynos_enter_core0_aftr(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index) { - unsigned long tmp; - - /* 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); cpu_pm_exit(); - /* - * 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; } diff --git a/arch/arm/mach-exynos/pm.c b/arch/arm/mach-exynos/pm.c index 50b6b4d..6d9ef69 100644 --- a/arch/arm/mach-exynos/pm.c +++ b/arch/arm/mach-exynos/pm.c @@ -213,15 +213,21 @@ static void exynos_pm_prepare(void) __raw_writel(virt_to_phys(exynos_cpu_resume), S5P_INFORM0); } -static int exynos_pm_suspend(void) +static void exynos_pm_central_suspend(void) { unsigned long tmp; /* 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); +} + +static int exynos_pm_suspend(void) +{ + unsigned long tmp; + + exynos_pm_central_suspend(); /* Setting SEQ_OPTION register */ @@ -234,7 +240,7 @@ static int exynos_pm_suspend(void) return 0; } -static void exynos_pm_resume(void) +static int exynos_pm_central_resume(void) { unsigned long tmp; @@ -251,9 +257,17 @@ static void exynos_pm_resume(void) /* clear the wakeup state register */ __raw_writel(0x0, S5P_WAKEUP_STAT); /* No need to perform below restore code */ - goto early_wakeup; + return -1; } + return 0; +} + +static void exynos_pm_resume(void) +{ + if (exynos_pm_central_resume()) + goto early_wakeup; + if (!soc_is_exynos5250()) exynos_cpu_restore_register(); @@ -359,6 +373,7 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self, switch (cmd) { case CPU_PM_ENTER: if (cpu == 0) { + exynos_pm_central_suspend(); exynos_cpu_save_register(); } break; @@ -368,6 +383,7 @@ static int exynos_cpu_pm_notifier(struct notifier_block *self, if (!soc_is_exynos5250()) scu_enable(S5P_VA_SCU); exynos_cpu_restore_register(); + exynos_pm_central_resume(); } break; }