[PATCH v6 09/16] OMAP2+: UART: Add runtime pm support for omap-serial driver

[Kevin Hilman]

Govindraj <govindraj.ti at gmail.com> writes:

> On Fri, Oct 14, 2011 at 2:52 AM, Kevin Hilman <khilman at ti.com> wrote:
>> Govindraj <govindraj.ti at gmail.com> writes:
>>
>>> On Thu, Oct 13, 2011 at 5:36 AM, Kevin Hilman <khilman at ti.com> wrote:
>>>> "Govindraj.R" <govindraj.raja at ti.com> writes:
>>>>
>>>> [...]
>>>>
>>>>> Use device_may_wakeup to check whether uart has wakeup capabilities
>>>>> and then enable uart runtime usage for the uart.
>>>>
>>>> Curious about what happens when device_may_wakeup() is not set during
>>>> device init.
>>>>
>>>> [...]
>>>>
>>>>> @@ -1305,6 +1363,16 @@ static int serial_omap_probe(struct platform_device *pdev)
>>>>>               up->uart_dma.rx_dma_channel = OMAP_UART_DMA_CH_FREE;
>>>>>       }
>>>>>
>>>>> +     pm_runtime_use_autosuspend(&pdev->dev);
>>>>> +     pm_runtime_set_autosuspend_delay(&pdev->dev,
>>>>> +                     OMAP_UART_AUTOSUSPEND_DELAY);
>>>>> +
>>>>> +     pm_runtime_irq_safe(&pdev->dev);
>>>>> +     if (device_may_wakeup(&pdev->dev)) {
>>>>> +             pm_runtime_enable(&pdev->dev);
>>>>
>>>> So if device_may_wakeup() is false, runtime PM is not enabled, then...
>>>>
>>>>> +             pm_runtime_get_sync(&pdev->dev);
>>>>
>>>> ...this get doesn't happen, and the first register access causes a crash.
>>>
>>> Actually no crash, clocks will left enabled from boot up (hwmod_no_reset/idle)
>>> that are idled and enabled back here.
>>>
>>> Since hwmod_idle is binded here later ([PATCH v6 15/16]),
>>
>> IMO, That's not a very maintainable solution.
>>
>> What happens when when someone fixes serial.c to only set
>> HWMOD_INIT_NO_IDLE on the console UART?  or if we fix things so we don't
>> need INIT_NO_IDLE anymore?  Then this will crash.
>>
>> Driver code should not make assumptions like this about what device init
>> code is or isn't doing.
>>
>
> Okay, How about doing as below ensuring get_sync always
> and forbid runtime to avoid clock_gating if wakeup is not set.
>

This is better, I'm not crazy about that either.

Consider a platform that doesn't have wakeups, but the UARTs are not
used (at least initially.)   That means on power up, all the UARTs are
left enabled to consume power even when they're not used.

Kevin