From mboxrd@z Thu Jan 1 00:00:00 1970 From: govindraj.ti@gmail.com (Govindraj) Date: Thu, 13 Oct 2011 06:41:37 +0530 Subject: [PATCH v6 14/16] OMAP2+: UART: Take console_lock in suspend path if not taken In-Reply-To: <87mxd5n55y.fsf@ti.com> References: <1317380561-661-1-git-send-email-govindraj.raja@ti.com> <1317380561-661-5-git-send-email-govindraj.raja@ti.com> <87sjmz1hfm.fsf@ti.com> <87mxd5n55y.fsf@ti.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Thu, Oct 13, 2011 at 5:17 AM, Kevin Hilman wrote: > Govindraj writes: > >> On Wed, Oct 12, 2011 at 12:31 AM, Kevin Hilman wrote: >>> "Govindraj.R" writes: >>> >>>> In suspend path the console_lock is taken by uart_port_suspend >>>> however when no_console_suspend is used console_lock is not taken. >>>> >>>> During system wide suspend omap_pwr_domain hooks cut all >>>> clocks that are left enabled. So its unsafe to proceed printing after >>>> clocks are cut by pwr_domain hooks. >>> >>> As I've mentioned in previous reviews, when no_console_suspend is >>> enabled, the user has explicitly requested console output during >>> suspend. ?In order to support that, we should not be cutting clocks at >>> all in that mode. >>> >>> One way to address this would be to just disable runtime PM in the >>> ->prepare method of the driver if no_console_suspend is enabled. >>> >> >> Okay fine exploring this option, right API's would be to use >> pm_runtime_forbid/allow. > > Yes. > >> <> >> >> +static int serial_omap_runtime_prepare(struct device *dev) >> +{ >> + ? ? ? if (!console_suspend_enabled) >> + ? ? ? ? ? ? ? pm_runtime_forbid(dev); >> + >> + ? ? ? return 0; >> +} >> + >> +static void serial_omap_runtime_complete(struct device *dev) >> +{ >> + ? ? ? if (!console_suspend_enabled) >> + ? ? ? ? ? ? ? pm_runtime_allow(dev); >> +} >> + >> ?static const struct dev_pm_ops serial_omap_dev_pm_ops = { >> ? ? ? ? SET_SYSTEM_SLEEP_PM_OPS(serial_omap_suspend, serial_omap_resume) >> ? ? ? ? SET_RUNTIME_PM_OPS(serial_omap_runtime_suspend, >> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? serial_omap_runtime_resume, NULL) >> + ? ? ? .prepare = serial_omap_runtime_prepare, >> + ? ? ? .complete = ?serial_omap_runtime_complete, >> ?}; > > OK, but please add comments to these functions about exactly why they > are needed. > >> >> But to either use runtime forbid or disable we have ensure that >> power_domain hooks don't go ahead and disable >> the clocks with omap_device_idle as *runtime forbid or disable will >> not set runtime_status to RPM_SUSPENDED* >> and will stay in RPM_ACTIVE if we call runtime disable or forbid from >> active state. > > Correct. > >> in power_domain hooks we just check the pm_runtime_status_suspended >> this will be false even if >> we do runtime disable/forbid and it will cut uart clocks always. >> >> So we may need below check also: >> >> diff --git a/arch/arm/plat-omap/omap_device.c b/arch/arm/plat-omap/omap_device.c >> index 26aee5c..286a534 100644 >> --- a/arch/arm/plat-omap/omap_device.c >> +++ b/arch/arm/plat-omap/omap_device.c >> @@ -592,7 +592,8 @@ static int _od_suspend_noirq(struct device *dev) >> >> ? ? ? ? ret = pm_generic_suspend_noirq(dev); >> >> - ? ? ? if (!ret && !pm_runtime_status_suspended(dev)) { >> + ? ? ? if (!ret && pm_runtime_enabled(dev) && >> + ? ? ? ? ? ? ? ? ? ? ? !pm_runtime_status_suspended(dev)) { >> ? ? ? ? ? ? ? ? if (pm_generic_runtime_suspend(dev) == 0) { >> ? ? ? ? ? ? ? ? ? ? ? ? omap_device_idle(pdev); >> ? ? ? ? ? ? ? ? ? ? ? ? od->flags |= OMAP_DEVICE_SUSPENDED; > > This isn't right either because devices that may not yet be initialized > (or loaded) may not have runtime PM enabled, so those devices may not > be properly idled. > > We have an omap_device API to disable this feature at the PM domain level: > omap_device_disable_idle_on_suspend(). ?All you should have to do is to > use this API in the device init code on the console UART if > no_console_suspend has been enabled. Yes seems okay to me, Will check if no_console_suspend is used then set omap_device_disable_idle_on_suspend in serial.c. -- Thanks, Govindraj.R