Govindraj [off-list ref] writes:

On Fri, Sep 9, 2011 at 5:54 AM, Kevin Hilman [off-list ref] wrote:

quoted

"Govindraj.R" [off-list ref] writes:

quoted

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.

quoted

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.

quoted

quoted

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.

quoted

quoted

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 <redacted>
Signed-off-by: Govindraj.R <redacted>
---
?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(-)

[..]

quoted

quoted

+
+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.

quoted

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.

[...]

quoted

quoted

?}

?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