[PATCH v6 14/16] OMAP2+: UART: Take console_lock in suspend path if not taken

[Govindraj]

On Thu, Oct 13, 2011 at 5:17 AM, Kevin Hilman <khilman at ti.com> wrote:
> Govindraj <govindraj.ti at gmail.com> writes:
>
>> On Wed, Oct 12, 2011 at 12:31 AM, Kevin Hilman <khilman at ti.com> wrote:
>>> "Govindraj.R" <govindraj.raja at ti.com> 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.
>
>> <<SNIP>>
>>
>> +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