Re: [PATCH v4 07/11] Serial: OMAP: Add runtime pm support for omap-serial driver

[Kevin Hilman <khilman@ti.com>]

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

> On Fri, Sep 9, 2011 at 5:54 AM, Kevin Hilman <khilman@ti.com> wrote:
>> "Govindraj.R" <govindraj.raja@ti.com> writes:
>>
>>> Adapts omap-serial driver to use pm_runtime API's.
>>>
>>> 1.) Moving context_restore func to driver from serial.c
>>> 2.) Use runtime irq safe to use get_sync from irq context.
>>> 3.) autosuspend for port specific activities and put for reg access.
>>
>> Re: autosuspend, it looks like the driver now has 2 mechanisms for
>> tracking activity.  The runtime PM 'mark last busy' and the existing
>> 'up->port_activity = jiffies' stuff.  Do you think those could be
>> unified?
>>
>
> Is there a way where we can retrieve the last busy jiffie value
> using runtime API? I don't find that available.
>

Not currently.  The question is more along the lines of: what is the
port_activity jiffies used for, and can it be replaced by using
_mark_last_busy().

If it cannot, that's fine, but it should be described.

>> At first glance, it looks like most places with a _mark_last_busy() also
>> have a up->port_activity update.  I'm not familiar enough with the
>> driver to make the call, but they look awfully similar.
>>
>
> Ok, I will check whether I can get rid if it.
>
>>> 4.) for earlyprintk usage the debug macro will write to console uart
>>>     so keep uart clocks active from boot, idle using hwmod API's re-enable back
>>>     using runtime API's.
>>
>> /me doesn't understand
>
> I have added this reason in early mail reply to 04/11 patch review
> [needed for earlyprintk option from low level debug ]

OK, but AFAIK, DEBUG_LL + earlyprintk do not use this driver.  Instead
they use the debug macros, so any hacks to deal with that do not belong
in the driver.

If there are hacks required, they should be in a separate patch, with a
separate descriptive changelog.

>>
>>> 5.) Moving context restore to runtime suspend and using reg values from port
>>>     structure to restore the uart port context based on context_loss_count.
>>>     Maintain internal state machine for avoiding repeated enable/disable of
>>>     uart port wakeup mechanism.
>>> 6.) Add API to enable wakeup and check wakeup status.
>>>
>>> Acked-by: Alan Cox <alan@linux.intel.com>
>>> Signed-off-by: Govindraj.R <govindraj.raja@ti.com>
>>> ---
>>>  arch/arm/mach-omap2/serial.c                  |   49 ++++++
>>>  arch/arm/plat-omap/include/plat/omap-serial.h |   10 ++
>>>  drivers/tty/serial/omap-serial.c              |  211 ++++++++++++++++++++++---
>>>  3 files changed, 250 insertions(+), 20 deletions(-)
>>>
>
> [..]
>
>>> +
>>> +static void omap_uart_wakeup_enable(struct platform_device *pdev, bool enable)
>>> +{
>>> +     struct omap_device *od;
>>> +     struct omap_uart_port_info *up = pdev->dev.platform_data;
>>> +
>>> +     /* Set or clear wake-enable bit */
>>> +     if (up->wk_en && up->wk_mask) {
>>> +             u32 v = __raw_readl(up->wk_en);
>>> +             if (enable)
>>> +                     v |= up->wk_mask;
>>> +             else
>>> +                     v &= ~up->wk_mask;
>>> +             __raw_writel(v, up->wk_en);
>>> +     }
>>> +
>>> +     od = to_omap_device(pdev);
>>> +     if (enable)
>>> +             omap_hwmod_enable_wakeup(od->hwmods[0]);
>>> +     else
>>> +             omap_hwmod_disable_wakeup(od->hwmods[0]);
>>> +}
>>> +
>>
>> Here again, this is difficult to review because you removed the code in
>> [4/11] and add it back here, but with rather different functionality
>> with no description as to why.
>>
>
> will move this change as part of 4/11 itself.
>

The point was not just that the move was confusing, but that you changed
the functionality along with the move without any description.

>> The previous version enabled wakeups at the PRCM and at the IO ring.
>> This version enables wakeups at the PRCM as well but instead of enabling
>> them at the IO ring, enables them at the module.
>>
>
> Since we are gating using prepare idle call I think
> we can use this enable wake-up call as part of prepare idle call itself,
> as done earlier.

Not sure what that has to with my comment.

In moving this code, you removed the enable/disable of IO ring wakeups.
It probably continues to work because they're enabled by the bootloader,
but that is not correct and the driver should be managing the IO ring
wakeups as before.

[...]

>>>  }
>>>
>>>  static int __init
>>> @@ -1065,22 +1123,33 @@ static struct uart_driver serial_omap_reg = {
>>>       .cons           = OMAP_CONSOLE,
>>>  };
>>>
>>> -static int
>>> -serial_omap_suspend(struct platform_device *pdev, pm_message_t state)
>>> +static int serial_omap_suspend(struct device *dev)
>>>  {
>>> -     struct uart_omap_port *up = platform_get_drvdata(pdev);
>>> +     struct uart_omap_port *up = dev_get_drvdata(dev);
>>>
>>> -     if (up)
>>> +     if (up) {
>>>               uart_suspend_port(&serial_omap_reg, &up->port);
>>> +             if (up->port.line == up->port.cons->index &&
>>> +                             !is_console_locked())
>>> +                     up->console_locked = console_trylock();
>>> +     }
>>> +
>>
>> Motiviation for console locking in this version of the series is not
>> clear, and not described in the changelog.
>>
>> It's also present here in static suspend/resume but not in runtime
>> suspend/resume, which also is suspicious.
>>
>
> Yes will add to change log,
>
> We needed this for no console suspend use case
> no console lock is taken in no_console_suspend is specified,
>
> Since our pwr_dmn hooks disable clocks left enabled during suspend,
> so any prints after pwr_dmn hooks can cause problems since clocks
> are already cut from pwr_dmn hooks.
>
> So buffer prints while entering suspend by taking console lock
> if not taken already and print back after uart state machine restores
> console uart.

Any special consideration for features like no_console_suspend should be
done in a separate patch with a separate changelog.

However, as I've stated before during previous reviews, if someone has
put no_console_suspend on the command line, that means they've
specifically stated they *want* to see console writes during suspend.
That means, we should not cut clocks at all.

Of course, that means the system will not hit the low power state becase
the UART clocks are not gated, but that is what the user requested.


Kevin
--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html