On Fri, Oct 14, 2011 at 10:34 PM, Kevin Hilman [off-list ref] wrote:

Govindraj [off-list ref] writes:

quoted

On Fri, Oct 14, 2011 at 2:52 AM, Kevin Hilman [off-list ref] wrote:

quoted

Govindraj [off-list ref] writes:

quoted

On Thu, Oct 13, 2011 at 5:36 AM, Kevin Hilman [off-list ref] wrote:

quoted

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

[...]

quoted

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.

[...]

quoted

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

quoted

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

okay, the right place would be serial_omap_pm
to do add runtime forbid.

whenever the uart port is used it will call serial_omap_pm
with state param assigned value zero from serial_core layer
for open system call from user space.

Also when uart port is closed it will shutdown where we can add runtime_allow.
So for uart ports not wakeup capable keep uart clocks on until
used and once port is shutdown cut uart clocks.

diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 8a955fc..2c43744 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c

@@ -608,6 +608,10 @@ static void serial_omap_shutdown(struct uart_port *port)
                        up->uart_dma.rx_buf_dma_phys);
                up->uart_dma.rx_buf = NULL;
        }
+
+       if (!device_may_wakeup(&up->pdev->dev))
+               pm_runtime_allow(&up->pdev->dev);
+
        pm_runtime_put(&up->pdev->dev);
        free_irq(up->port.irq, up);
 }

@@ -893,6 +897,10 @@ serial_omap_pm(struct uart_port *port, unsigned int state,
        serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
        serial_out(up, UART_EFR, efr);
        serial_out(up, UART_LCR, 0);
+
+       if (!state && !device_may_wakeup(&up->pdev->dev))
+               pm_runtime_forbid(&up->pdev->dev);
+
        pm_runtime_put(&up->pdev->dev);
 }


--

Thanks,
Govindraj.R