| Message ID | 20200212235134.12638-6-digetx@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Consolidate and improve NVIDIA Tegra CPUIDLE driver(s) | expand |
On Thu, Feb 13, 2020 at 02:51:22AM +0300, Dmitry Osipenko wrote: > Technically cpu_suspend() may fail and it's never good to lose information > about failure. For example things like cpuidle core could correctly sample > idling time in the case of failure. > > Acked-by: Peter De Schrijver <pdeschrijver@nvidia.com> > Tested-by: Peter Geis <pgwipeout@gmail.com> > Tested-by: Jasper Korten <jja2000@gmail.com> > Tested-by: David Heidelberg <david@ixit.cz> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- [ ... ] > cpu_cluster_pm_enter(); > suspend_cpu_complex(); > > - cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu); > + err = cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu); > > /* > * Resume L2 cache if it wasn't re-enabled early during resume, > @@ -208,6 +210,8 @@ void tegra_idle_lp2_last(void) > > restore_cpu_complex(); If the cpu_suspend fails, does restore_cpu_complex() need to be called ? > cpu_cluster_pm_exit(); > + > + return err; > } [ ... ]
21.02.2020 18:16, Daniel Lezcano пишет: > On Thu, Feb 13, 2020 at 02:51:22AM +0300, Dmitry Osipenko wrote: >> Technically cpu_suspend() may fail and it's never good to lose information >> about failure. For example things like cpuidle core could correctly sample >> idling time in the case of failure. >> >> Acked-by: Peter De Schrijver <pdeschrijver@nvidia.com> >> Tested-by: Peter Geis <pgwipeout@gmail.com> >> Tested-by: Jasper Korten <jja2000@gmail.com> >> Tested-by: David Heidelberg <david@ixit.cz> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- > > [ ... ] > >> cpu_cluster_pm_enter(); >> suspend_cpu_complex(); >> >> - cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu); >> + err = cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu); >> >> /* >> * Resume L2 cache if it wasn't re-enabled early during resume, >> @@ -208,6 +210,8 @@ void tegra_idle_lp2_last(void) >> >> restore_cpu_complex(); > > If the cpu_suspend fails, does restore_cpu_complex() need to be called ? Yes, because suspend_cpu_complex() didn't fail. I don't see any reason why restore_cpu_complex() shouldn't be called, please clarify yours thought.
On Fri, Feb 21, 2020 at 08:21:41PM +0300, Dmitry Osipenko wrote: > 21.02.2020 18:16, Daniel Lezcano пишет: > > On Thu, Feb 13, 2020 at 02:51:22AM +0300, Dmitry Osipenko wrote: > >> Technically cpu_suspend() may fail and it's never good to lose information > >> about failure. For example things like cpuidle core could correctly sample > >> idling time in the case of failure. > >> > >> Acked-by: Peter De Schrijver <pdeschrijver@nvidia.com> > >> Tested-by: Peter Geis <pgwipeout@gmail.com> > >> Tested-by: Jasper Korten <jja2000@gmail.com> > >> Tested-by: David Heidelberg <david@ixit.cz> > >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > >> --- > > > > [ ... ] > > > >> cpu_cluster_pm_enter(); > >> suspend_cpu_complex(); > >> > >> - cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu); > >> + err = cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu); > >> > >> /* > >> * Resume L2 cache if it wasn't re-enabled early during resume, > >> @@ -208,6 +210,8 @@ void tegra_idle_lp2_last(void) > >> > >> restore_cpu_complex(); > > > > If the cpu_suspend fails, does restore_cpu_complex() need to be called ? > > Yes, because suspend_cpu_complex() didn't fail. I don't see any reason > why restore_cpu_complex() shouldn't be called, please clarify yours thought. If the suspend fails, the power down does not happen, thus the logic is not lost and then it not necessary to restore something which has not been lost. I don't know the hardware details, so that may be partially correct.
21.02.2020 20:40, Daniel Lezcano пишет: > On Fri, Feb 21, 2020 at 08:21:41PM +0300, Dmitry Osipenko wrote: >> 21.02.2020 18:16, Daniel Lezcano пишет: >>> On Thu, Feb 13, 2020 at 02:51:22AM +0300, Dmitry Osipenko wrote: >>>> Technically cpu_suspend() may fail and it's never good to lose information >>>> about failure. For example things like cpuidle core could correctly sample >>>> idling time in the case of failure. >>>> >>>> Acked-by: Peter De Schrijver <pdeschrijver@nvidia.com> >>>> Tested-by: Peter Geis <pgwipeout@gmail.com> >>>> Tested-by: Jasper Korten <jja2000@gmail.com> >>>> Tested-by: David Heidelberg <david@ixit.cz> >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>> --- >>> >>> [ ... ] >>> >>>> cpu_cluster_pm_enter(); >>>> suspend_cpu_complex(); >>>> >>>> - cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu); >>>> + err = cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu); >>>> >>>> /* >>>> * Resume L2 cache if it wasn't re-enabled early during resume, >>>> @@ -208,6 +210,8 @@ void tegra_idle_lp2_last(void) >>>> >>>> restore_cpu_complex(); >>> >>> If the cpu_suspend fails, does restore_cpu_complex() need to be called ? >> >> Yes, because suspend_cpu_complex() didn't fail. I don't see any reason >> why restore_cpu_complex() shouldn't be called, please clarify yours thought. > > If the suspend fails, the power down does not happen, thus the logic is not > lost and then it not necessary to restore something which has not been lost. > > I don't know the hardware details, so that may be partially correct. At quick glance, the restore_cpu_complex() is only used for restoring the GIC's state on Tegra. I don't think it's really worth the effort to handle restore_cpu_complex() specially in a case of the error condition because the chance that the error will ever happen is very small and restoring the cluster's state won't cause any trouble in that case. Let's keep this patch as-is for simplicity :)
On 21/02/2020 19:42, Dmitry Osipenko wrote: > 21.02.2020 20:40, Daniel Lezcano пишет: >> On Fri, Feb 21, 2020 at 08:21:41PM +0300, Dmitry Osipenko wrote: >>> 21.02.2020 18:16, Daniel Lezcano пишет: >>>> On Thu, Feb 13, 2020 at 02:51:22AM +0300, Dmitry Osipenko wrote: >>>>> Technically cpu_suspend() may fail and it's never good to lose information >>>>> about failure. For example things like cpuidle core could correctly sample >>>>> idling time in the case of failure. >>>>> >>>>> Acked-by: Peter De Schrijver <pdeschrijver@nvidia.com> >>>>> Tested-by: Peter Geis <pgwipeout@gmail.com> >>>>> Tested-by: Jasper Korten <jja2000@gmail.com> >>>>> Tested-by: David Heidelberg <david@ixit.cz> >>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >>>>> --- >>>> >>>> [ ... ] >>>> >>>>> cpu_cluster_pm_enter(); >>>>> suspend_cpu_complex(); >>>>> >>>>> - cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu); >>>>> + err = cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu); >>>>> >>>>> /* >>>>> * Resume L2 cache if it wasn't re-enabled early during resume, >>>>> @@ -208,6 +210,8 @@ void tegra_idle_lp2_last(void) >>>>> >>>>> restore_cpu_complex(); >>>> >>>> If the cpu_suspend fails, does restore_cpu_complex() need to be called ? >>> >>> Yes, because suspend_cpu_complex() didn't fail. I don't see any reason >>> why restore_cpu_complex() shouldn't be called, please clarify yours thought. >> >> If the suspend fails, the power down does not happen, thus the logic is not >> lost and then it not necessary to restore something which has not been lost. >> >> I don't know the hardware details, so that may be partially correct. > > At quick glance, the restore_cpu_complex() is only used for restoring > the GIC's state on Tegra. > > I don't think it's really worth the effort to handle > restore_cpu_complex() specially in a case of the error condition because > the chance that the error will ever happen is very small and restoring > the cluster's state won't cause any trouble in that case. > > Let's keep this patch as-is for simplicity :) Yep, not a big deal. Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
diff --git a/arch/arm/mach-tegra/cpuidle-tegra20.c b/arch/arm/mach-tegra/cpuidle-tegra20.c index f7d5041e73cc..9789541adb7d 100644 --- a/arch/arm/mach-tegra/cpuidle-tegra20.c +++ b/arch/arm/mach-tegra/cpuidle-tegra20.c @@ -87,15 +87,17 @@ static bool tegra20_cpu_cluster_power_down(struct cpuidle_device *dev, struct cpuidle_driver *drv, int index) { + bool ret; + while (!tegra_cpu_rail_off_ready()) cpu_relax(); - tegra_idle_lp2_last(); + ret = !tegra_idle_lp2_last(); if (cpu_online(1)) tegra20_wake_cpu1_from_reset(); - return true; + return ret; } #ifdef CONFIG_SMP diff --git a/arch/arm/mach-tegra/cpuidle-tegra30.c b/arch/arm/mach-tegra/cpuidle-tegra30.c index a3ce8dabfe18..17cbd118abee 100644 --- a/arch/arm/mach-tegra/cpuidle-tegra30.c +++ b/arch/arm/mach-tegra/cpuidle-tegra30.c @@ -68,9 +68,7 @@ static bool tegra30_cpu_cluster_power_down(struct cpuidle_device *dev, return false; } - tegra_idle_lp2_last(); - - return true; + return !tegra_idle_lp2_last(); } #ifdef CONFIG_SMP diff --git a/arch/arm/mach-tegra/pm.c b/arch/arm/mach-tegra/pm.c index a72f9a2d3cb7..a094acaca307 100644 --- a/arch/arm/mach-tegra/pm.c +++ b/arch/arm/mach-tegra/pm.c @@ -189,14 +189,16 @@ static void tegra_pm_set(enum tegra_suspend_mode mode) tegra_pmc_enter_suspend_mode(mode); } -void tegra_idle_lp2_last(void) +int tegra_idle_lp2_last(void) { + int err; + tegra_pm_set(TEGRA_SUSPEND_LP2); cpu_cluster_pm_enter(); suspend_cpu_complex(); - cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu); + err = cpu_suspend(PHYS_OFFSET - PAGE_OFFSET, &tegra_sleep_cpu); /* * Resume L2 cache if it wasn't re-enabled early during resume, @@ -208,6 +210,8 @@ void tegra_idle_lp2_last(void) restore_cpu_complex(); cpu_cluster_pm_exit(); + + return err; } enum tegra_suspend_mode tegra_pm_validate_suspend_mode( diff --git a/arch/arm/mach-tegra/pm.h b/arch/arm/mach-tegra/pm.h index 2c294f6365c0..7d72f31dee77 100644 --- a/arch/arm/mach-tegra/pm.h +++ b/arch/arm/mach-tegra/pm.h @@ -25,7 +25,7 @@ void tegra30_sleep_core_init(void); void tegra_clear_cpu_in_lp2(void); void tegra_set_cpu_in_lp2(void); -void tegra_idle_lp2_last(void); +int tegra_idle_lp2_last(void); extern void (*tegra_tear_down_cpu)(void); #ifdef CONFIG_PM_SLEEP