[v3,04/12] Serial: OMAP: Add runtime pm support for omap-serial driver

Commit Message

Govindraj.R June 8, 2011, 11:23 a.m. UTC

Adapts omap-serial driver to use pm_runtime api's.

1.) Populate reg values to uart port which can be used for context restore.
2.) Moving context_restore func to driver from serial.c
3.) Adding port_enable/disable func to enable/disable given uart port.
    enable port using get_sync and disable using autosuspend.
4.) using runtime irq safe api to make get_sync be called from irq context.

Acked-by: Alan Cox <[e-mail]>
Signed-off-by: Govindraj.R <[e-mail]>
---
 arch/arm/mach-omap2/serial.c                  |   22 +++
 arch/arm/plat-omap/include/plat/omap-serial.h |    2 +
 drivers/tty/serial/omap-serial.c              |  212 ++++++++++++++++++++++---
 3 files changed, 210 insertions(+), 26 deletions(-)

Comments

Hunter, Jon June 8, 2011, 8:39 p.m. UTC | #1

Hi Govindraj,

On 6/8/2011 6:23 AM, Govindraj.R wrote:

[snip]

> +
> +#define OMAP_UART_AUTOSUSPEND_DELAY (30 * HZ) /* Value is msecs */

[snip]

> @@ -1295,18 +1381,36 @@ 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);

Something is weird here...DEFAULT_AUTOSUSPEND_DELAY is defined as 
(30*HZ) which would appear to be jiffies (ticks per second) and NOT 
msecs. However, pm_runtime_set_autosuspend is clearly expecting msecs. 
So this seems to conflict. By default 30*HZ for omap would be 30*128 = 
3840ms so not quite 4 seconds.

What were you intending here?

Cheers
Jon

Govindraj R June 9, 2011, 4:35 a.m. UTC | #2

On Thu, Jun 9, 2011 at 2:09 AM, Jon Hunter <[e-mail]> wrote:
> Hi Govindraj,
>
> On 6/8/2011 6:23 AM, Govindraj.R wrote:
>
> [snip]
>
>> +
>> +#define OMAP_UART_AUTOSUSPEND_DELAY (30 * HZ) /* Value is msecs */
>
> [snip]
>
>> @@ -1295,18 +1381,36 @@ 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);
>
> Something is weird here...DEFAULT_AUTOSUSPEND_DELAY is defined as (30*HZ)
> which would appear to be jiffies (ticks per second) and NOT msecs. However,
> pm_runtime_set_autosuspend is clearly expecting msecs. So this seems to
> conflict. By default 30*HZ for omap would be 30*128 = 3840ms so not quite 4
> seconds.
>
> What were you intending here?

Intention is to get approx 3 secs timeout for autosuspend.

--
Thanks,
Govindraj.R

>
> Cheers
> Jon
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Hunter, Jon June 9, 2011, 8:49 p.m. UTC | #3

Hi Govindraj

On 06/08/2011 11:35 PM, Govindraj wrote:
> On Thu, Jun 9, 2011 at 2:09 AM, Jon Hunter<[e-mail]>  wrote:
>> Hi Govindraj,
>>
>> On 6/8/2011 6:23 AM, Govindraj.R wrote:
>>
>> [snip]
>>
>>> +
>>> +#define OMAP_UART_AUTOSUSPEND_DELAY (30 * HZ) /* Value is msecs */
>>
>> [snip]
>>
>>> @@ -1295,18 +1381,36 @@ 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);
>>
>> Something is weird here...DEFAULT_AUTOSUSPEND_DELAY is defined as (30*HZ)
>> which would appear to be jiffies (ticks per second) and NOT msecs. However,
>> pm_runtime_set_autosuspend is clearly expecting msecs. So this seems to
>> conflict. By default 30*HZ for omap would be 30*128 = 3840ms so not quite 4
>> seconds.
>>
>> What were you intending here?
>
> Intention is to get approx 3 secs timeout for autosuspend.

In that case you should just define DEFAULT_AUTOSUSPEND_DELAY as 30000. 
The above is just confusing as you are mixing time types and hence, it 
is not clear what you intend the default timeout to be.

Jon

Hunter, Jon June 9, 2011, 8:51 p.m. UTC | #4

On 06/09/2011 03:49 PM, Jon Hunter wrote:
> Hi Govindraj
>
> On 06/08/2011 11:35 PM, Govindraj wrote:
>> On Thu, Jun 9, 2011 at 2:09 AM, Jon Hunter<[e-mail]> wrote:
>>> Hi Govindraj,
>>>
>>> On 6/8/2011 6:23 AM, Govindraj.R wrote:
>>>
>>> [snip]
>>>
>>>> +
>>>> +#define OMAP_UART_AUTOSUSPEND_DELAY (30 * HZ) /* Value is msecs */
>>>
>>> [snip]
>>>
>>>> @@ -1295,18 +1381,36 @@ 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);
>>>
>>> Something is weird here...DEFAULT_AUTOSUSPEND_DELAY is defined as
>>> (30*HZ)
>>> which would appear to be jiffies (ticks per second) and NOT msecs.
>>> However,
>>> pm_runtime_set_autosuspend is clearly expecting msecs. So this seems to
>>> conflict. By default 30*HZ for omap would be 30*128 = 3840ms so not
>>> quite 4
>>> seconds.
>>>
>>> What were you intending here?
>>
>> Intention is to get approx 3 secs timeout for autosuspend.
>
> In that case you should just define DEFAULT_AUTOSUSPEND_DELAY as 30000.
> The above is just confusing as you are mixing time types and hence, it
> is not clear what you intend the default timeout to be.

Sorry, I meant 3000 and not 30000 above, if you want 3 secs.

Jon

Kevin Hilman June 24, 2011, 11:30 p.m. UTC | #5

"Govindraj.R" <[e-mail]> writes:

> Adapts omap-serial driver to use pm_runtime api's.
>
> 1.) Populate reg values to uart port which can be used for context restore.

Please make this part a separate patch.

> 2.) Moving context_restore func to driver from serial.c
> 3.) Adding port_enable/disable func to enable/disable given uart port.
>     enable port using get_sync and disable using autosuspend.
> 4.) using runtime irq safe api to make get_sync be called from irq context.

>
> Acked-by: Alan Cox <[e-mail]>
> Signed-off-by: Govindraj.R <[e-mail]>

This is great!  we're almost there.   Some minor comments below...

> ---
>  arch/arm/mach-omap2/serial.c                  |   22 +++
>  arch/arm/plat-omap/include/plat/omap-serial.h |    2 +
>  drivers/tty/serial/omap-serial.c              |  212 ++++++++++++++++++++++---
>  3 files changed, 210 insertions(+), 26 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
> index 8c1a4c7..1651c2c 100644
> --- a/arch/arm/mach-omap2/serial.c
> +++ b/arch/arm/mach-omap2/serial.c
> @@ -189,6 +189,27 @@ static void omap_serial_fill_default_pads(struct omap_board_data *bdata)
>  	}
>  }
>  
> +static void omap_uart_wakeup_enable(struct platform_device *pdev, bool enable)
> +{
> +	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);
> +	}

I think I asked this in previous series, but can't remember the answer
now...  can't we enable bits in the WKEN registers once at init time,
and then just use omap_hwmod_[enable|disable]_wakeup() here?

> +	/* Enable or clear io-pad wakeup */
> +	if (enable)
> +		omap_device_enable_ioring_wakeup(pdev);
> +	else
> +		omap_device_disable_ioring_wakeup(pdev);
> +}
> +
>  static void omap_uart_idle_init(struct omap_uart_port_info *uart,
>  				unsigned short num)
>  {
> @@ -332,6 +353,7 @@ void __init omap_serial_init_port(struct omap_board_data *bdata)
>  
>  	pdata->uartclk = OMAP24XX_BASE_BAUD * 16;
>  	pdata->flags = UPF_BOOT_AUTOCONF;
> +	pdata->enable_wakeup = omap_uart_wakeup_enable;
>  	if (bdata->id == omap_uart_con_id)
>  		pdata->console_uart = true;
>  
> diff --git a/arch/arm/plat-omap/include/plat/omap-serial.h b/arch/arm/plat-omap/include/plat/omap-serial.h
> index 2ca885b..ac30de8 100644
> --- a/arch/arm/plat-omap/include/plat/omap-serial.h
> +++ b/arch/arm/plat-omap/include/plat/omap-serial.h
> @@ -65,6 +65,7 @@ struct omap_uart_port_info {
>  	unsigned int		errata;
>  	unsigned int		console_uart;
>  
> +	void (*enable_wakeup)(struct platform_device *, bool);
>  	void __iomem *wk_st;
>  	void __iomem *wk_en;
>  	u32 wk_mask;
> @@ -120,6 +121,7 @@ struct uart_omap_port {
>  	char			name[20];
>  	unsigned long		port_activity;
>  	unsigned int		errata;
> +	void (*enable_wakeup)(struct platform_device *, bool);
>  };
>  
>  #endif /* __OMAP_SERIAL_H__ */
> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
> index 47cadf4..897416f 100644
> --- a/drivers/tty/serial/omap-serial.c
> +++ b/drivers/tty/serial/omap-serial.c
> @@ -37,10 +37,14 @@
>  #include <linux/clk.h>
>  #include <linux/serial_core.h>
>  #include <linux/irq.h>
> +#include <linux/pm_runtime.h>
>  
>  #include <plat/dma.h>
>  #include <plat/dmtimer.h>
>  #include <plat/omap-serial.h>
> +#include <plat/omap_device.h>
> +
> +#define OMAP_UART_AUTOSUSPEND_DELAY (30 * HZ) /* Value is msecs */

As Jon already pointed out, the HZ here is wrong.  Please define this
value in msecs.

>  static struct uart_omap_port *ui[OMAP_MAX_HSUART_PORTS];
>  
> @@ -94,6 +98,17 @@ serial_omap_get_divisor(struct uart_port *port, unsigned int baud)
>  	return port->uartclk/(baud * divisor);
>  }
>  
> +static inline void serial_omap_port_disable(struct uart_omap_port *up)
> +{
> +	pm_runtime_mark_last_busy(&up->pdev->dev);
> +	pm_runtime_put_autosuspend(&up->pdev->dev);
> +}
> +
> +static inline void serial_omap_port_enable(struct uart_omap_port *up)
> +{
> +	pm_runtime_get_sync(&up->pdev->dev);
> +}

These inlines are not needed.  Please use runtime PM calls directly in
the code.  For _enable(), this is straight forward.  For _disable()
though, I don't think you want (or need) to _mark_last_busy() for every
instance of omap_port_disable().  You only need to do this for ones
TX/RX transactions...

>  static void serial_omap_stop_rxdma(struct uart_omap_port *up)
>  {
>  	if (up->uart_dma.rx_dma_used) {
> @@ -110,8 +125,11 @@ static void serial_omap_enable_ms(struct uart_port *port)
>  	struct uart_omap_port *up = (struct uart_omap_port *)port;
>  
>  	dev_dbg(up->port.dev, "serial_omap_enable_ms+%d\n", up->pdev->id);
> +
> +	serial_omap_port_enable(up);
>  	up->ier |= UART_IER_MSI;
>  	serial_out(up, UART_IER, up->ier);
> +	serial_omap_port_disable(up);

...for example, this one is not really activity based, so should
probably just be pm_runtime_get_sync(), write the register, then
pm_runtime_put() (async version.)

I didn't look at all the others below, but they should be looked at
individually.

>  }
>  
>  static void serial_omap_stop_tx(struct uart_port *port)
> @@ -131,21 +149,26 @@ static void serial_omap_stop_tx(struct uart_port *port)
>  		up->uart_dma.tx_dma_channel = OMAP_UART_DMA_CH_FREE;
>  	}
>  
> +	serial_omap_port_enable(up);
>  	if (up->ier & UART_IER_THRI) {
>  		up->ier &= ~UART_IER_THRI;
>  		serial_out(up, UART_IER, up->ier);
>  	}
> +
> +	serial_omap_port_disable(up);
>  }
>  
>  static void serial_omap_stop_rx(struct uart_port *port)
>  {
>  	struct uart_omap_port *up = (struct uart_omap_port *)port;
>  
> +	serial_omap_port_enable(up);
>  	if (up->use_dma)
>  		serial_omap_stop_rxdma(up);
>  	up->ier &= ~UART_IER_RLSI;
>  	up->port.read_status_mask &= ~UART_LSR_DR;
>  	serial_out(up, UART_IER, up->ier);
> +	serial_omap_port_disable(up);
>  }
>  
>  static inline void receive_chars(struct uart_omap_port *up, int *status)
> @@ -261,8 +284,10 @@ static void serial_omap_start_tx(struct uart_port *port)
>  	unsigned int start;
>  	int ret = 0;
>  
> +	serial_omap_port_enable(up);
>  	if (!up->use_dma) {
>  		serial_omap_enable_ier_thri(up);
> +		serial_omap_port_disable(up);
>  		return;
>  	}
>  
> @@ -354,9 +379,12 @@ static inline irqreturn_t serial_omap_irq(int irq, void *dev_id)
>  	unsigned int iir, lsr;
>  	unsigned long flags;
>  
> +	serial_omap_port_enable(up);
>  	iir = serial_in(up, UART_IIR);
> -	if (iir & UART_IIR_NO_INT)
> +	if (iir & UART_IIR_NO_INT) {
> +		serial_omap_port_disable(up);
>  		return IRQ_NONE;
> +	}
>  
>  	spin_lock_irqsave(&up->port.lock, flags);
>  	lsr = serial_in(up, UART_LSR);
> @@ -378,6 +406,8 @@ static inline irqreturn_t serial_omap_irq(int irq, void *dev_id)
>  		transmit_chars(up);
>  
>  	spin_unlock_irqrestore(&up->port.lock, flags);
> +	serial_omap_port_disable(up);
> +
>  	up->port_activity = jiffies;
>  	return IRQ_HANDLED;
>  }
> @@ -388,11 +418,12 @@ static unsigned int serial_omap_tx_empty(struct uart_port *port)
>  	unsigned long flags = 0;
>  	unsigned int ret = 0;
>  
> +	serial_omap_port_enable(up);
>  	dev_dbg(up->port.dev, "serial_omap_tx_empty+%d\n", up->pdev->id);
>  	spin_lock_irqsave(&up->port.lock, flags);
>  	ret = serial_in(up, UART_LSR) & UART_LSR_TEMT ? TIOCSER_TEMT : 0;
>  	spin_unlock_irqrestore(&up->port.lock, flags);
> -
> +	serial_omap_port_disable(up);
>  	return ret;
>  }
>  
> @@ -402,7 +433,10 @@ static unsigned int serial_omap_get_mctrl(struct uart_port *port)
>  	unsigned char status;
>  	unsigned int ret = 0;
>  
> +	serial_omap_port_enable(up);
>  	status = check_modem_status(up);
> +	serial_omap_port_disable(up);
> +
>  	dev_dbg(up->port.dev, "serial_omap_get_mctrl+%d\n", up->pdev->id);
>  
>  	if (status & UART_MSR_DCD)
> @@ -434,7 +468,9 @@ static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  		mcr |= UART_MCR_LOOP;
>  
>  	mcr |= up->mcr;
> +	serial_omap_port_enable(up);
>  	serial_out(up, UART_MCR, mcr);
> +	serial_omap_port_disable(up);
>  }
>  
>  static void serial_omap_break_ctl(struct uart_port *port, int break_state)
> @@ -443,6 +479,7 @@ static void serial_omap_break_ctl(struct uart_port *port, int break_state)
>  	unsigned long flags = 0;
>  
>  	dev_dbg(up->port.dev, "serial_omap_break_ctl+%d\n", up->pdev->id);
> +	serial_omap_port_enable(up);
>  	spin_lock_irqsave(&up->port.lock, flags);
>  	if (break_state == -1)
>  		up->lcr |= UART_LCR_SBC;
> @@ -450,6 +487,7 @@ static void serial_omap_break_ctl(struct uart_port *port, int break_state)
>  		up->lcr &= ~UART_LCR_SBC;
>  	serial_out(up, UART_LCR, up->lcr);
>  	spin_unlock_irqrestore(&up->port.lock, flags);
> +	serial_omap_port_disable(up);
>  }
>  
>  static int serial_omap_startup(struct uart_port *port)
> @@ -468,6 +506,7 @@ static int serial_omap_startup(struct uart_port *port)
>  
>  	dev_dbg(up->port.dev, "serial_omap_startup+%d\n", up->pdev->id);
>  
> +	serial_omap_port_enable(up);
>  	/*
>  	 * Clear the FIFO buffers and disable them.
>  	 * (they will be reenabled in set_termios())
> @@ -523,6 +562,7 @@ static int serial_omap_startup(struct uart_port *port)
>  	/* Enable module level wake up */
>  	serial_out(up, UART_OMAP_WER, OMAP_UART_WER_MOD_WKUP);
>  
> +	serial_omap_port_disable(up);
>  	up->port_activity = jiffies;
>  	return 0;
>  }
> @@ -533,6 +573,8 @@ static void serial_omap_shutdown(struct uart_port *port)
>  	unsigned long flags = 0;
>  
>  	dev_dbg(up->port.dev, "serial_omap_shutdown+%d\n", up->pdev->id);
> +
> +	serial_omap_port_enable(up);
>  	/*
>  	 * Disable interrupts from this port
>  	 */
> @@ -566,6 +608,7 @@ static void serial_omap_shutdown(struct uart_port *port)
>  			up->uart_dma.rx_buf_dma_phys);
>  		up->uart_dma.rx_buf = NULL;
>  	}
> +	serial_omap_port_disable(up);
>  	free_irq(up->port.irq, up);
>  }
>  
> @@ -671,6 +714,10 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>  	baud = uart_get_baud_rate(port, termios, old, 0, port->uartclk/13);
>  	quot = serial_omap_get_divisor(port, baud);
>  
> +	up->dll = quot & 0xff;
> +	up->dlh = quot >> 8;
> +	up->mdr1 = UART_OMAP_MDR1_DISABLE;
> +
>  	up->fcr = UART_FCR_R_TRIG_01 | UART_FCR_T_TRIG_01 |
>  			UART_FCR_ENABLE_FIFO;
>  	if (up->use_dma)
> @@ -680,6 +727,7 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>  	 * Ok, we're now changing the port state. Do it with
>  	 * interrupts disabled.
>  	 */
> +	serial_omap_port_enable(up);
>  	spin_lock_irqsave(&up->port.lock, flags);
>  
>  	/*
> @@ -723,6 +771,7 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>  		up->ier |= UART_IER_MSI;
>  	serial_out(up, UART_IER, up->ier);
>  	serial_out(up, UART_LCR, cval);		/* reset DLAB */
> +	up->lcr = cval;
>  
>  	/* FIFOs and DMA Settings */
>  
> @@ -758,8 +807,7 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>  	serial_out(up, UART_MCR, up->mcr);
>  
>  	/* Protocol, Baud Rate, and Interrupt Settings */
> -
> -	serial_out(up, UART_OMAP_MDR1, UART_OMAP_MDR1_DISABLE);
> +	serial_out(up, UART_OMAP_MDR1, up->mdr1);
>  	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
>  
>  	up->efr = serial_in(up, UART_EFR);
> @@ -769,8 +817,8 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>  	serial_out(up, UART_IER, 0);
>  	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
>  
> -	serial_out(up, UART_DLL, quot & 0xff);          /* LS of divisor */
> -	serial_out(up, UART_DLM, quot >> 8);            /* MS of divisor */
> +	serial_out(up, UART_DLL, up->dll);	/* LS of divisor */
> +	serial_out(up, UART_DLM, up->dlh);	/* MS of divisor */
>  
>  	serial_out(up, UART_LCR, 0);
>  	serial_out(up, UART_IER, up->ier);
> @@ -780,9 +828,11 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>  	serial_out(up, UART_LCR, cval);
>  
>  	if (baud > 230400 && baud != 3000000)
> -		serial_out(up, UART_OMAP_MDR1, UART_OMAP_MDR1_13X_MODE);
> +		up->mdr1 = UART_OMAP_MDR1_13X_MODE;
>  	else
> -		serial_out(up, UART_OMAP_MDR1, UART_OMAP_MDR1_16X_MODE);
> +		up->mdr1 = UART_OMAP_MDR1_16X_MODE;
> +
> +	serial_out(up, UART_OMAP_MDR1, up->mdr1)
>  
>  	/* Hardware Flow Control Configuration */
>  
> @@ -810,6 +860,7 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>  		serial_omap_configure_xonxoff(up, termios);
>  
>  	spin_unlock_irqrestore(&up->port.lock, flags);
> +	serial_omap_port_disable(up);
>  	dev_dbg(up->port.dev, "serial_omap_set_termios+%d\n", up->pdev->id);
>  }
>  
> @@ -821,6 +872,8 @@ serial_omap_pm(struct uart_port *port, unsigned int state,
>  	unsigned char efr;
>  
>  	dev_dbg(up->port.dev, "serial_omap_pm+%d\n", up->pdev->id);
> +
> +	serial_omap_port_enable(up);
>  	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
>  	efr = serial_in(up, UART_EFR);
>  	serial_out(up, UART_EFR, efr | UART_EFR_ECB);
> @@ -830,6 +883,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)
> +		pm_runtime_put_sync(&up->pdev->dev);
> +	else
> +		serial_omap_port_disable(up);

OK, so this boils down to either a _put_sync() or a _mark_last_busy +
_put_autosuspend(), depending on whether this is suspending or resuming,
right?

Why the difference? 

>  }
>  
>  static void serial_omap_release_port(struct uart_port *port)
> @@ -907,25 +964,31 @@ static inline void wait_for_xmitr(struct uart_omap_port *up)
>  static void serial_omap_poll_put_char(struct uart_port *port, unsigned char ch)
>  {
>  	struct uart_omap_port *up = (struct uart_omap_port *)port;
> +
> +	serial_omap_port_enable(up);
>  	wait_for_xmitr(up);
>  	serial_out(up, UART_TX, ch);
> +	serial_omap_port_disable(up);
>  }
>  
>  static int serial_omap_poll_get_char(struct uart_port *port)
>  {
>  	struct uart_omap_port *up = (struct uart_omap_port *)port;
> -	unsigned int status = serial_in(up, UART_LSR);
> +	unsigned int status;
>  
> +	serial_omap_port_enable(up);
> +	status = serial_in(up, UART_LSR);
>  	if (!(status & UART_LSR_DR))
>  		return NO_POLL_CHAR;
>  
> -	return serial_in(up, UART_RX);
> +	status = serial_in(up, UART_RX);
> +	serial_omap_port_disable(up);
> +	return status;
>  }
>  
>  #endif /* CONFIG_CONSOLE_POLL */
>  
>  #ifdef CONFIG_SERIAL_OMAP_CONSOLE
> -
>  static struct uart_omap_port *serial_omap_console_ports[4];
>  
>  static struct uart_driver serial_omap_reg;
> @@ -955,6 +1018,8 @@ serial_omap_console_write(struct console *co, const char *s,
>  	else
>  		spin_lock(&up->port.lock);
>  
> +	serial_omap_port_enable(up);
> +
>  	/*
>  	 * First save the IER then disable the interrupts
>  	 */
> @@ -979,6 +1044,7 @@ serial_omap_console_write(struct console *co, const char *s,
>  	if (up->msr_saved_flags)
>  		check_modem_status(up);
>  
> +	serial_omap_port_disable(up);
>  	if (locked)
>  		spin_unlock(&up->port.lock);
>  	local_irq_restore(flags);
> @@ -1061,22 +1127,27 @@ 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);
> +		console_trylock();

This locking needs to be explained.  Why it is needed, but very
importantly, why you are not checking the return value of the _trylock()

> +		serial_omap_pm(&up->port, 3, 0);

Why is this call needed?

uart_suspend_port() calls uart_change_pm() which should call the ->pm
method of struct uart_ops (which is serial_omap_pm).  What am I missing?

Also notice you're not calling serial_omap_pm() in the resume method
below to change it back.

> +	}
>  	return 0;
>  }

Also, device wakeup capability should be checked and enabled/disabled in
the suspend path (not only the runtime suspend path.)

> -static int serial_omap_resume(struct platform_device *dev)
> +static int serial_omap_resume(struct device *dev)
>  {
> -	struct uart_omap_port *up = platform_get_drvdata(dev);
> +	struct uart_omap_port *up = dev_get_drvdata(dev);
>  
> -	if (up)
> +	if (up) {
>  		uart_resume_port(&serial_omap_reg, &up->port);
> +		console_unlock();

Again, please describe locking in comments.

Also, what happens here if the console_trylock() in your suspend method
failed?

> +	}
> +
>  	return 0;
>  }
>  
> @@ -1097,6 +1168,7 @@ static void serial_omap_rx_timeout(unsigned long uart_no)
>  			serial_omap_stop_rxdma(up);
>  			up->ier |= (UART_IER_RDI | UART_IER_RLSI);
>  			serial_out(up, UART_IER, up->ier);
> +			serial_omap_port_disable(up);
>  		}
>  		return;
>  	}
> @@ -1128,6 +1200,9 @@ static void serial_omap_rx_timeout(unsigned long uart_no)
>  
>  static void uart_rx_dma_callback(int lch, u16 ch_status, void *data)
>  {
> +	struct uart_omap_port *up = (struct uart_omap_port *)data;
> +
> +	serial_omap_port_disable(up);
>  	return;
>  }
>  
> @@ -1135,6 +1210,7 @@ static int serial_omap_start_rxdma(struct uart_omap_port *up)
>  {
>  	int ret = 0;
>  
> +	serial_omap_port_enable(up);
>  	if (up->uart_dma.rx_dma_channel == -1) {
>  		ret = omap_request_dma(up->uart_dma.uart_dma_rx,
>  				"UART Rx DMA",
> @@ -1214,6 +1290,7 @@ static void uart_tx_dma_callback(int lch, u16 ch_status, void *data)
>  		serial_omap_stop_tx(&up->port);
>  		up->uart_dma.tx_dma_used = false;
>  		spin_unlock(&(up->uart_dma.tx_lock));
> +		serial_omap_port_disable(up);
>  	} else {
>  		omap_stop_dma(up->uart_dma.tx_dma_channel);
>  		serial_omap_continue_tx(up);
> @@ -1224,9 +1301,10 @@ static void uart_tx_dma_callback(int lch, u16 ch_status, void *data)
>  
>  static int serial_omap_probe(struct platform_device *pdev)
>  {
> -	struct uart_omap_port	*up;
> +	struct uart_omap_port	*up = NULL;
>  	struct resource		*mem, *irq, *dma_tx, *dma_rx;
>  	struct omap_uart_port_info *omap_up_info = pdev->dev.platform_data;
> +	struct omap_device *od;
>  	int ret = -ENOSPC;
>  
>  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -1276,12 +1354,20 @@ static int serial_omap_probe(struct platform_device *pdev)
>  	up->port.ops = &serial_omap_pops;
>  	up->port.line = pdev->id;
>  
> -	up->port.membase = omap_up_info->membase;
> -	up->port.mapbase = omap_up_info->mapbase;
> +	up->port.mapbase = mem->start;
> +	up->port.membase = ioremap(mem->start, mem->end - mem->start);
> +
> +	if (!up->port.membase) {
> +		dev_err(&pdev->dev, "can't ioremap UART\n");
> +		ret = -ENOMEM;
> +		goto err1;
> +	}
> +
>  	up->port.flags = omap_up_info->flags;
> -	up->port.irqflags = omap_up_info->irqflags;
>  	up->port.uartclk = omap_up_info->uartclk;
>  	up->uart_dma.uart_base = mem->start;
> +	up->errata = omap_up_info->errata;
> +	up->enable_wakeup = omap_up_info->enable_wakeup;
>  
>  	if (omap_up_info->dma_enabled) {
>  		up->uart_dma.uart_dma_tx = dma_tx->start;
> @@ -1295,18 +1381,36 @@ 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_enable(&pdev->dev);
> +	pm_runtime_irq_safe(&pdev->dev);
> +
> +	if (omap_up_info->console_uart) {
> +		od = to_omap_device(up->pdev);
> +		omap_hwmod_idle(od->hwmods[0]);
> +		serial_omap_port_enable(up);
> +		serial_omap_port_disable(up);
> +	}
> +
>  	ui[pdev->id] = up;
>  	serial_omap_add_console_port(up);
>  
>  	ret = uart_add_one_port(&serial_omap_reg, &up->port);
>  	if (ret != 0)
> -		goto do_release_region;
> +		goto err1;
>  
> +	dev_set_drvdata(&pdev->dev, up);
>  	platform_set_drvdata(pdev, up);
> +
>  	return 0;
>  err:
>  	dev_err(&pdev->dev, "[UART%d]: failure [%s]: %d\n",
>  				pdev->id, __func__, ret);
> +err1:
> +	kfree(up);
>  do_release_region:
>  	release_mem_region(mem->start, (mem->end - mem->start) + 1);
>  	return ret;
> @@ -1318,20 +1422,76 @@ static int serial_omap_remove(struct platform_device *dev)
>  
>  	platform_set_drvdata(dev, NULL);
>  	if (up) {
> +		pm_runtime_disable(&up->pdev->dev);
>  		uart_remove_one_port(&serial_omap_reg, &up->port);
>  		kfree(up);
>  	}
>  	return 0;
>  }
>  
> +static void omap_uart_restore_context(struct uart_omap_port *up)
> +{
> +	u16 efr = 0;
> +
> +	serial_out(up, UART_OMAP_MDR1, up->mdr1);
> +	serial_out(up, UART_LCR, 0xBF); /* Config B mode */
> +	efr = serial_in(up, UART_EFR);
> +	serial_out(up, UART_EFR, UART_EFR_ECB);
> +	serial_out(up, UART_LCR, 0x0); /* Operational mode */
> +	serial_out(up, UART_IER, 0x0);
> +	serial_out(up, UART_LCR, 0xBF); /* Config B mode */
> +	serial_out(up, UART_DLL, up->dll);
> +	serial_out(up, UART_DLM, up->dlh);
> +	serial_out(up, UART_LCR, 0x0); /* Operational mode */
> +	serial_out(up, UART_IER, up->ier);
> +	serial_out(up, UART_FCR, up->fcr);
> +	serial_out(up, UART_LCR, 0x80);
> +	serial_out(up, UART_MCR, up->mcr);
> +	serial_out(up, UART_LCR, 0xBF); /* Config B mode */
> +	serial_out(up, UART_EFR, efr);
> +	serial_out(up, UART_LCR, up->lcr);
> +	/* UART 16x mode */
> +	serial_out(up, UART_OMAP_MDR1, up->mdr1);
> +}
> +
> +static int omap_serial_runtime_suspend(struct device *dev)
> +{
> +	struct uart_omap_port *up = dev_get_drvdata(dev);
> +
> +	if (!up)
> +		goto done;
> +
> +	if (device_may_wakeup(dev))
> +		up->enable_wakeup(up->pdev, true);
> +	else
> +		up->enable_wakeup(up->pdev, false);

I know the earlier code was doing it this way too, but an optimization
would be to have the driver keep state whether the wakeups are enabled
or disabled, so you don't have to keep calling this function (which can
be expensive with the PRCM reads/writes.

That state can also be used in the suspend path, which should also be
managing wakeups.

> +done:
> +	return 0;
> +}
> +
> +static int omap_serial_runtime_resume(struct device *dev)
> +{
> +	struct uart_omap_port *up = dev_get_drvdata(dev);
> +
> +	if (up)
> +		omap_uart_restore_context(up);

You only need to restore context when returning from off-mode.  You
should be using omap_device_get_context_loss_count (called via pdata
function pointer) for this.  See the MMC driver or DSS driver for how
this is done.

> +	return 0;
> +}
> +
> +static const struct dev_pm_ops omap_serial_dev_pm_ops = {
> +	.suspend = serial_omap_suspend,
> +	.resume	= serial_omap_resume,
> +	.runtime_suspend = omap_serial_runtime_suspend,
> +	.runtime_resume = omap_serial_runtime_resume,
> +};
> +
>  static struct platform_driver serial_omap_driver = {
>  	.probe          = serial_omap_probe,
>  	.remove         = serial_omap_remove,
> -
> -	.suspend	= serial_omap_suspend,
> -	.resume		= serial_omap_resume,
>  	.driver		= {
>  		.name	= DRIVER_NAME,
> +		.pm = &omap_serial_dev_pm_ops,
>  	},
>  };

Kevin

Govindraj R June 27, 2011, 2:31 p.m. UTC | #6

On Sat, Jun 25, 2011 at 5:00 AM, Kevin Hilman <[e-mail]> wrote:
> "Govindraj.R" <[e-mail]> writes:
>
>> Adapts omap-serial driver to use pm_runtime api's.
>>
>> 1.) Populate reg values to uart port which can be used for context restore.
>
> Please make this part a separate patch.
>
>> 2.) Moving context_restore func to driver from serial.c
>> 3.) Adding port_enable/disable func to enable/disable given uart port.
>>     enable port using get_sync and disable using autosuspend.
>> 4.) using runtime irq safe api to make get_sync be called from irq context.
>
>>
>> Acked-by: Alan Cox <[e-mail]>
>> Signed-off-by: Govindraj.R <[e-mail]>
>
> This is great!  we're almost there.   Some minor comments below...
>
>> ---
>>  arch/arm/mach-omap2/serial.c                  |   22 +++
>>  arch/arm/plat-omap/include/plat/omap-serial.h |    2 +
>>  drivers/tty/serial/omap-serial.c              |  212 ++++++++++++++++++++++---
>>  3 files changed, 210 insertions(+), 26 deletions(-)
>>
>> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
>> index 8c1a4c7..1651c2c 100644
>> --- a/arch/arm/mach-omap2/serial.c
>> +++ b/arch/arm/mach-omap2/serial.c
>> @@ -189,6 +189,27 @@ static void omap_serial_fill_default_pads(struct omap_board_data *bdata)
>>       }
>>  }
>>
>> +static void omap_uart_wakeup_enable(struct platform_device *pdev, bool enable)
>> +{
>> +     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);
>> +     }
>
> I think I asked this in previous series, but can't remember the answer
> now...  can't we enable bits in the WKEN registers once at init time,
> and then just use omap_hwmod_[enable|disable]_wakeup() here?
>

by default all bits are enabled in WKEN,

I will use omap_hwmod_[enable|disable]_wakeup() api's

if these API's take care of WKEN regs, then no issue
using the same.


>> +     /* Enable or clear io-pad wakeup */
>> +     if (enable)
>> +             omap_device_enable_ioring_wakeup(pdev);
>> +     else
>> +             omap_device_disable_ioring_wakeup(pdev);
>> +}
>> +
>>  static void omap_uart_idle_init(struct omap_uart_port_info *uart,
>>                               unsigned short num)
>>  {
>> @@ -332,6 +353,7 @@ void __init omap_serial_init_port(struct omap_board_data *bdata)
>>
>>       pdata->uartclk = OMAP24XX_BASE_BAUD * 16;
>>       pdata->flags = UPF_BOOT_AUTOCONF;
>> +     pdata->enable_wakeup = omap_uart_wakeup_enable;
>>       if (bdata->id == omap_uart_con_id)
>>               pdata->console_uart = true;
>>
>> diff --git a/arch/arm/plat-omap/include/plat/omap-serial.h b/arch/arm/plat-omap/include/plat/omap-serial.h
>> index 2ca885b..ac30de8 100644
>> --- a/arch/arm/plat-omap/include/plat/omap-serial.h
>> +++ b/arch/arm/plat-omap/include/plat/omap-serial.h
>> @@ -65,6 +65,7 @@ struct omap_uart_port_info {
>>       unsigned int            errata;
>>       unsigned int            console_uart;
>>
>> +     void (*enable_wakeup)(struct platform_device *, bool);
>>       void __iomem *wk_st;
>>       void __iomem *wk_en;
>>       u32 wk_mask;
>> @@ -120,6 +121,7 @@ struct uart_omap_port {
>>       char                    name[20];
>>       unsigned long           port_activity;
>>       unsigned int            errata;
>> +     void (*enable_wakeup)(struct platform_device *, bool);
>>  };
>>
>>  #endif /* __OMAP_SERIAL_H__ */
>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>> index 47cadf4..897416f 100644
>> --- a/drivers/tty/serial/omap-serial.c
>> +++ b/drivers/tty/serial/omap-serial.c
>> @@ -37,10 +37,14 @@
>>  #include <linux/clk.h>
>>  #include <linux/serial_core.h>
>>  #include <linux/irq.h>
>> +#include <linux/pm_runtime.h>
>>
>>  #include <plat/dma.h>
>>  #include <plat/dmtimer.h>
>>  #include <plat/omap-serial.h>
>> +#include <plat/omap_device.h>
>> +
>> +#define OMAP_UART_AUTOSUSPEND_DELAY (30 * HZ) /* Value is msecs */
>
> As Jon already pointed out, the HZ here is wrong.  Please define this
> value in msecs.
>

corrected.

>>  static struct uart_omap_port *ui[OMAP_MAX_HSUART_PORTS];
>>
>> @@ -94,6 +98,17 @@ serial_omap_get_divisor(struct uart_port *port, unsigned int baud)
>>       return port->uartclk/(baud * divisor);
>>  }
>>
>> +static inline void serial_omap_port_disable(struct uart_omap_port *up)
>> +{
>> +     pm_runtime_mark_last_busy(&up->pdev->dev);
>> +     pm_runtime_put_autosuspend(&up->pdev->dev);
>> +}
>> +
>> +static inline void serial_omap_port_enable(struct uart_omap_port *up)
>> +{
>> +     pm_runtime_get_sync(&up->pdev->dev);
>> +}
>
> These inlines are not needed.  Please use runtime PM calls directly in
> the code.  For _enable(), this is straight forward.  For _disable()
> though, I don't think you want (or need) to _mark_last_busy() for every
> instance of omap_port_disable().  You only need to do this for ones
> TX/RX transactions...
>

Fine. will modify.


>>  static void serial_omap_stop_rxdma(struct uart_omap_port *up)
>>  {
>>       if (up->uart_dma.rx_dma_used) {
>> @@ -110,8 +125,11 @@ static void serial_omap_enable_ms(struct uart_port *port)
>>       struct uart_omap_port *up = (struct uart_omap_port *)port;
>>
>>       dev_dbg(up->port.dev, "serial_omap_enable_ms+%d\n", up->pdev->id);
>> +
>> +     serial_omap_port_enable(up);
>>       up->ier |= UART_IER_MSI;
>>       serial_out(up, UART_IER, up->ier);
>> +     serial_omap_port_disable(up);
>
> ...for example, this one is not really activity based, so should
> probably just be pm_runtime_get_sync(), write the register, then
> pm_runtime_put() (async version.)
>
> I didn't look at all the others below, but they should be looked at
> individually.
>

ok. I will check them.

>>  }
>>
>>  static void serial_omap_stop_tx(struct uart_port *port)
>> @@ -131,21 +149,26 @@ static void serial_omap_stop_tx(struct uart_port *port)
>>               up->uart_dma.tx_dma_channel = OMAP_UART_DMA_CH_FREE;
>>       }
>>
>> +     serial_omap_port_enable(up);
>>       if (up->ier & UART_IER_THRI) {
>>               up->ier &= ~UART_IER_THRI;
>>               serial_out(up, UART_IER, up->ier);
>>       }
>> +
>> +     serial_omap_port_disable(up);
>>  }
>>
>>  static void serial_omap_stop_rx(struct uart_port *port)
>>  {
>>       struct uart_omap_port *up = (struct uart_omap_port *)port;
>>
>> +     serial_omap_port_enable(up);
>>       if (up->use_dma)
>>               serial_omap_stop_rxdma(up);
>>       up->ier &= ~UART_IER_RLSI;
>>       up->port.read_status_mask &= ~UART_LSR_DR;
>>       serial_out(up, UART_IER, up->ier);
>> +     serial_omap_port_disable(up);
>>  }
>>
>>  static inline void receive_chars(struct uart_omap_port *up, int *status)
>> @@ -261,8 +284,10 @@ static void serial_omap_start_tx(struct uart_port *port)
>>       unsigned int start;
>>       int ret = 0;
>>
>> +     serial_omap_port_enable(up);
>>       if (!up->use_dma) {
>>               serial_omap_enable_ier_thri(up);
>> +             serial_omap_port_disable(up);
>>               return;
>>       }
>>
>> @@ -354,9 +379,12 @@ static inline irqreturn_t serial_omap_irq(int irq, void *dev_id)
>>       unsigned int iir, lsr;
>>       unsigned long flags;
>>
>> +     serial_omap_port_enable(up);
>>       iir = serial_in(up, UART_IIR);
>> -     if (iir & UART_IIR_NO_INT)
>> +     if (iir & UART_IIR_NO_INT) {
>> +             serial_omap_port_disable(up);
>>               return IRQ_NONE;
>> +     }
>>
>>       spin_lock_irqsave(&up->port.lock, flags);
>>       lsr = serial_in(up, UART_LSR);
>> @@ -378,6 +406,8 @@ static inline irqreturn_t serial_omap_irq(int irq, void *dev_id)
>>               transmit_chars(up);
>>
>>       spin_unlock_irqrestore(&up->port.lock, flags);
>> +     serial_omap_port_disable(up);
>> +
>>       up->port_activity = jiffies;
>>       return IRQ_HANDLED;
>>  }
>> @@ -388,11 +418,12 @@ static unsigned int serial_omap_tx_empty(struct uart_port *port)
>>       unsigned long flags = 0;
>>       unsigned int ret = 0;
>>
>> +     serial_omap_port_enable(up);
>>       dev_dbg(up->port.dev, "serial_omap_tx_empty+%d\n", up->pdev->id);
>>       spin_lock_irqsave(&up->port.lock, flags);
>>       ret = serial_in(up, UART_LSR) & UART_LSR_TEMT ? TIOCSER_TEMT : 0;
>>       spin_unlock_irqrestore(&up->port.lock, flags);
>> -
>> +     serial_omap_port_disable(up);
>>       return ret;
>>  }
>>
>> @@ -402,7 +433,10 @@ static unsigned int serial_omap_get_mctrl(struct uart_port *port)
>>       unsigned char status;
>>       unsigned int ret = 0;
>>
>> +     serial_omap_port_enable(up);
>>       status = check_modem_status(up);
>> +     serial_omap_port_disable(up);
>> +
>>       dev_dbg(up->port.dev, "serial_omap_get_mctrl+%d\n", up->pdev->id);
>>
>>       if (status & UART_MSR_DCD)
>> @@ -434,7 +468,9 @@ static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl)
>>               mcr |= UART_MCR_LOOP;
>>
>>       mcr |= up->mcr;
>> +     serial_omap_port_enable(up);
>>       serial_out(up, UART_MCR, mcr);
>> +     serial_omap_port_disable(up);
>>  }
>>
>>  static void serial_omap_break_ctl(struct uart_port *port, int break_state)
>> @@ -443,6 +479,7 @@ static void serial_omap_break_ctl(struct uart_port *port, int break_state)
>>       unsigned long flags = 0;
>>
>>       dev_dbg(up->port.dev, "serial_omap_break_ctl+%d\n", up->pdev->id);
>> +     serial_omap_port_enable(up);
>>       spin_lock_irqsave(&up->port.lock, flags);
>>       if (break_state == -1)
>>               up->lcr |= UART_LCR_SBC;
>> @@ -450,6 +487,7 @@ static void serial_omap_break_ctl(struct uart_port *port, int break_state)
>>               up->lcr &= ~UART_LCR_SBC;
>>       serial_out(up, UART_LCR, up->lcr);
>>       spin_unlock_irqrestore(&up->port.lock, flags);
>> +     serial_omap_port_disable(up);
>>  }
>>
>>  static int serial_omap_startup(struct uart_port *port)
>> @@ -468,6 +506,7 @@ static int serial_omap_startup(struct uart_port *port)
>>
>>       dev_dbg(up->port.dev, "serial_omap_startup+%d\n", up->pdev->id);
>>
>> +     serial_omap_port_enable(up);
>>       /*
>>        * Clear the FIFO buffers and disable them.
>>        * (they will be reenabled in set_termios())
>> @@ -523,6 +562,7 @@ static int serial_omap_startup(struct uart_port *port)
>>       /* Enable module level wake up */
>>       serial_out(up, UART_OMAP_WER, OMAP_UART_WER_MOD_WKUP);
>>
>> +     serial_omap_port_disable(up);
>>       up->port_activity = jiffies;
>>       return 0;
>>  }
>> @@ -533,6 +573,8 @@ static void serial_omap_shutdown(struct uart_port *port)
>>       unsigned long flags = 0;
>>
>>       dev_dbg(up->port.dev, "serial_omap_shutdown+%d\n", up->pdev->id);
>> +
>> +     serial_omap_port_enable(up);
>>       /*
>>        * Disable interrupts from this port
>>        */
>> @@ -566,6 +608,7 @@ static void serial_omap_shutdown(struct uart_port *port)
>>                       up->uart_dma.rx_buf_dma_phys);
>>               up->uart_dma.rx_buf = NULL;
>>       }
>> +     serial_omap_port_disable(up);
>>       free_irq(up->port.irq, up);
>>  }
>>
>> @@ -671,6 +714,10 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>>       baud = uart_get_baud_rate(port, termios, old, 0, port->uartclk/13);
>>       quot = serial_omap_get_divisor(port, baud);
>>
>> +     up->dll = quot & 0xff;
>> +     up->dlh = quot >> 8;
>> +     up->mdr1 = UART_OMAP_MDR1_DISABLE;
>> +
>>       up->fcr = UART_FCR_R_TRIG_01 | UART_FCR_T_TRIG_01 |
>>                       UART_FCR_ENABLE_FIFO;
>>       if (up->use_dma)
>> @@ -680,6 +727,7 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>>        * Ok, we're now changing the port state. Do it with
>>        * interrupts disabled.
>>        */
>> +     serial_omap_port_enable(up);
>>       spin_lock_irqsave(&up->port.lock, flags);
>>
>>       /*
>> @@ -723,6 +771,7 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>>               up->ier |= UART_IER_MSI;
>>       serial_out(up, UART_IER, up->ier);
>>       serial_out(up, UART_LCR, cval);         /* reset DLAB */
>> +     up->lcr = cval;
>>
>>       /* FIFOs and DMA Settings */
>>
>> @@ -758,8 +807,7 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>>       serial_out(up, UART_MCR, up->mcr);
>>
>>       /* Protocol, Baud Rate, and Interrupt Settings */
>> -
>> -     serial_out(up, UART_OMAP_MDR1, UART_OMAP_MDR1_DISABLE);
>> +     serial_out(up, UART_OMAP_MDR1, up->mdr1);
>>       serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
>>
>>       up->efr = serial_in(up, UART_EFR);
>> @@ -769,8 +817,8 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>>       serial_out(up, UART_IER, 0);
>>       serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
>>
>> -     serial_out(up, UART_DLL, quot & 0xff);          /* LS of divisor */
>> -     serial_out(up, UART_DLM, quot >> 8);            /* MS of divisor */
>> +     serial_out(up, UART_DLL, up->dll);      /* LS of divisor */
>> +     serial_out(up, UART_DLM, up->dlh);      /* MS of divisor */
>>
>>       serial_out(up, UART_LCR, 0);
>>       serial_out(up, UART_IER, up->ier);
>> @@ -780,9 +828,11 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>>       serial_out(up, UART_LCR, cval);
>>
>>       if (baud > 230400 && baud != 3000000)
>> -             serial_out(up, UART_OMAP_MDR1, UART_OMAP_MDR1_13X_MODE);
>> +             up->mdr1 = UART_OMAP_MDR1_13X_MODE;
>>       else
>> -             serial_out(up, UART_OMAP_MDR1, UART_OMAP_MDR1_16X_MODE);
>> +             up->mdr1 = UART_OMAP_MDR1_16X_MODE;
>> +
>> +     serial_out(up, UART_OMAP_MDR1, up->mdr1)
>>
>>       /* Hardware Flow Control Configuration */
>>
>> @@ -810,6 +860,7 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>>               serial_omap_configure_xonxoff(up, termios);
>>
>>       spin_unlock_irqrestore(&up->port.lock, flags);
>> +     serial_omap_port_disable(up);
>>       dev_dbg(up->port.dev, "serial_omap_set_termios+%d\n", up->pdev->id);
>>  }
>>
>> @@ -821,6 +872,8 @@ serial_omap_pm(struct uart_port *port, unsigned int state,
>>       unsigned char efr;
>>
>>       dev_dbg(up->port.dev, "serial_omap_pm+%d\n", up->pdev->id);
>> +
>> +     serial_omap_port_enable(up);
>>       serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
>>       efr = serial_in(up, UART_EFR);
>>       serial_out(up, UART_EFR, efr | UART_EFR_ECB);
>> @@ -830,6 +883,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)
>> +             pm_runtime_put_sync(&up->pdev->dev);
>> +     else
>> +             serial_omap_port_disable(up);
>
> OK, so this boils down to either a _put_sync() or a _mark_last_busy +
> _put_autosuspend(), depending on whether this is suspending or resuming,
> right?
>

yes also during bootup.
disable the ports immediately that are not used during bootup.

> Why the difference?

Need to put the ports down immediately during system wide suspend
other wise autosuspend delay will prevent system to enter
suspend state immediately.

>
>>  }
>>
>>  static void serial_omap_release_port(struct uart_port *port)
>> @@ -907,25 +964,31 @@ static inline void wait_for_xmitr(struct uart_omap_port *up)
>>  static void serial_omap_poll_put_char(struct uart_port *port, unsigned char ch)
>>  {
>>       struct uart_omap_port *up = (struct uart_omap_port *)port;
>> +
>> +     serial_omap_port_enable(up);
>>       wait_for_xmitr(up);
>>       serial_out(up, UART_TX, ch);
>> +     serial_omap_port_disable(up);
>>  }
>>
>>  static int serial_omap_poll_get_char(struct uart_port *port)
>>  {
>>       struct uart_omap_port *up = (struct uart_omap_port *)port;
>> -     unsigned int status = serial_in(up, UART_LSR);
>> +     unsigned int status;
>>
>> +     serial_omap_port_enable(up);
>> +     status = serial_in(up, UART_LSR);
>>       if (!(status & UART_LSR_DR))
>>               return NO_POLL_CHAR;
>>
>> -     return serial_in(up, UART_RX);
>> +     status = serial_in(up, UART_RX);
>> +     serial_omap_port_disable(up);
>> +     return status;
>>  }
>>
>>  #endif /* CONFIG_CONSOLE_POLL */
>>
>>  #ifdef CONFIG_SERIAL_OMAP_CONSOLE
>> -
>>  static struct uart_omap_port *serial_omap_console_ports[4];
>>
>>  static struct uart_driver serial_omap_reg;
>> @@ -955,6 +1018,8 @@ serial_omap_console_write(struct console *co, const char *s,
>>       else
>>               spin_lock(&up->port.lock);
>>
>> +     serial_omap_port_enable(up);
>> +
>>       /*
>>        * First save the IER then disable the interrupts
>>        */
>> @@ -979,6 +1044,7 @@ serial_omap_console_write(struct console *co, const char *s,
>>       if (up->msr_saved_flags)
>>               check_modem_status(up);
>>
>> +     serial_omap_port_disable(up);
>>       if (locked)
>>               spin_unlock(&up->port.lock);
>>       local_irq_restore(flags);
>> @@ -1061,22 +1127,27 @@ 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);
>> +             console_trylock();
>
> This locking needs to be explained.  Why it is needed, but very
> importantly, why you are not checking the return value of the _trylock()
>

any print after this point can result in failure of immediate system suspending
due to delayed autosuspend from omap_console_write.

log prints after uart suspend and print them during resume.


>> +             serial_omap_pm(&up->port, 3, 0);
>
> Why is this call needed?
>

Actually this is needed if no_console_suspend is used, for that
case the console will remain active since serial_omap_pm is not
getting called from serial_core and port is not suspended
immediately with put_sync.

prints during system wide suspend and delayed autosuspend
from console_write keeps system active in no_console_suspend case
so put in forced suspend state and log all prints to be printed later.

probably needs to a condition check if (no_console_suspend)


> uart_suspend_port() calls uart_change_pm() which should call the ->pm
> method of struct uart_ops (which is serial_omap_pm).  What am I missing?
>
> Also notice you're not calling serial_omap_pm() in the resume method
> below to change it back.
>
>> +     }
>>       return 0;
>>  }
>
> Also, device wakeup capability should be checked and enabled/disabled in
> the suspend path (not only the runtime suspend path.)
>

ok.


>> -static int serial_omap_resume(struct platform_device *dev)
>> +static int serial_omap_resume(struct device *dev)
>>  {
>> -     struct uart_omap_port *up = platform_get_drvdata(dev);
>> +     struct uart_omap_port *up = dev_get_drvdata(dev);
>>
>> -     if (up)
>> +     if (up) {
>>               uart_resume_port(&serial_omap_reg, &up->port);
>> +             console_unlock();
>
> Again, please describe locking in comments.
>
> Also, what happens here if the console_trylock() in your suspend method
> failed?
>

need to add a flag to check and unlock
from return status of trylock.

>> +     }
>> +
>>       return 0;
>>  }
>>
>> @@ -1097,6 +1168,7 @@ static void serial_omap_rx_timeout(unsigned long uart_no)
>>                       serial_omap_stop_rxdma(up);
>>                       up->ier |= (UART_IER_RDI | UART_IER_RLSI);
>>                       serial_out(up, UART_IER, up->ier);
>> +                     serial_omap_port_disable(up);
>>               }
>>               return;
>>       }
>> @@ -1128,6 +1200,9 @@ static void serial_omap_rx_timeout(unsigned long uart_no)
>>
>>  static void uart_rx_dma_callback(int lch, u16 ch_status, void *data)
>>  {
>> +     struct uart_omap_port *up = (struct uart_omap_port *)data;
>> +
>> +     serial_omap_port_disable(up);
>>       return;
>>  }
>>
>> @@ -1135,6 +1210,7 @@ static int serial_omap_start_rxdma(struct uart_omap_port *up)
>>  {
>>       int ret = 0;
>>
>> +     serial_omap_port_enable(up);
>>       if (up->uart_dma.rx_dma_channel == -1) {
>>               ret = omap_request_dma(up->uart_dma.uart_dma_rx,
>>                               "UART Rx DMA",
>> @@ -1214,6 +1290,7 @@ static void uart_tx_dma_callback(int lch, u16 ch_status, void *data)
>>               serial_omap_stop_tx(&up->port);
>>               up->uart_dma.tx_dma_used = false;
>>               spin_unlock(&(up->uart_dma.tx_lock));
>> +             serial_omap_port_disable(up);
>>       } else {
>>               omap_stop_dma(up->uart_dma.tx_dma_channel);
>>               serial_omap_continue_tx(up);
>> @@ -1224,9 +1301,10 @@ static void uart_tx_dma_callback(int lch, u16 ch_status, void *data)
>>
>>  static int serial_omap_probe(struct platform_device *pdev)
>>  {
>> -     struct uart_omap_port   *up;
>> +     struct uart_omap_port   *up = NULL;
>>       struct resource         *mem, *irq, *dma_tx, *dma_rx;
>>       struct omap_uart_port_info *omap_up_info = pdev->dev.platform_data;
>> +     struct omap_device *od;
>>       int ret = -ENOSPC;
>>
>>       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> @@ -1276,12 +1354,20 @@ static int serial_omap_probe(struct platform_device *pdev)
>>       up->port.ops = &serial_omap_pops;
>>       up->port.line = pdev->id;
>>
>> -     up->port.membase = omap_up_info->membase;
>> -     up->port.mapbase = omap_up_info->mapbase;
>> +     up->port.mapbase = mem->start;
>> +     up->port.membase = ioremap(mem->start, mem->end - mem->start);
>> +
>> +     if (!up->port.membase) {
>> +             dev_err(&pdev->dev, "can't ioremap UART\n");
>> +             ret = -ENOMEM;
>> +             goto err1;
>> +     }
>> +
>>       up->port.flags = omap_up_info->flags;
>> -     up->port.irqflags = omap_up_info->irqflags;
>>       up->port.uartclk = omap_up_info->uartclk;
>>       up->uart_dma.uart_base = mem->start;
>> +     up->errata = omap_up_info->errata;
>> +     up->enable_wakeup = omap_up_info->enable_wakeup;
>>
>>       if (omap_up_info->dma_enabled) {
>>               up->uart_dma.uart_dma_tx = dma_tx->start;
>> @@ -1295,18 +1381,36 @@ 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_enable(&pdev->dev);
>> +     pm_runtime_irq_safe(&pdev->dev);
>> +
>> +     if (omap_up_info->console_uart) {
>> +             od = to_omap_device(up->pdev);
>> +             omap_hwmod_idle(od->hwmods[0]);
>> +             serial_omap_port_enable(up);
>> +             serial_omap_port_disable(up);
>> +     }
>> +
>>       ui[pdev->id] = up;
>>       serial_omap_add_console_port(up);
>>
>>       ret = uart_add_one_port(&serial_omap_reg, &up->port);
>>       if (ret != 0)
>> -             goto do_release_region;
>> +             goto err1;
>>
>> +     dev_set_drvdata(&pdev->dev, up);
>>       platform_set_drvdata(pdev, up);
>> +
>>       return 0;
>>  err:
>>       dev_err(&pdev->dev, "[UART%d]: failure [%s]: %d\n",
>>                               pdev->id, __func__, ret);
>> +err1:
>> +     kfree(up);
>>  do_release_region:
>>       release_mem_region(mem->start, (mem->end - mem->start) + 1);
>>       return ret;
>> @@ -1318,20 +1422,76 @@ static int serial_omap_remove(struct platform_device *dev)
>>
>>       platform_set_drvdata(dev, NULL);
>>       if (up) {
>> +             pm_runtime_disable(&up->pdev->dev);
>>               uart_remove_one_port(&serial_omap_reg, &up->port);
>>               kfree(up);
>>       }
>>       return 0;
>>  }
>>
>> +static void omap_uart_restore_context(struct uart_omap_port *up)
>> +{
>> +     u16 efr = 0;
>> +
>> +     serial_out(up, UART_OMAP_MDR1, up->mdr1);
>> +     serial_out(up, UART_LCR, 0xBF); /* Config B mode */
>> +     efr = serial_in(up, UART_EFR);
>> +     serial_out(up, UART_EFR, UART_EFR_ECB);
>> +     serial_out(up, UART_LCR, 0x0); /* Operational mode */
>> +     serial_out(up, UART_IER, 0x0);
>> +     serial_out(up, UART_LCR, 0xBF); /* Config B mode */
>> +     serial_out(up, UART_DLL, up->dll);
>> +     serial_out(up, UART_DLM, up->dlh);
>> +     serial_out(up, UART_LCR, 0x0); /* Operational mode */
>> +     serial_out(up, UART_IER, up->ier);
>> +     serial_out(up, UART_FCR, up->fcr);
>> +     serial_out(up, UART_LCR, 0x80);
>> +     serial_out(up, UART_MCR, up->mcr);
>> +     serial_out(up, UART_LCR, 0xBF); /* Config B mode */
>> +     serial_out(up, UART_EFR, efr);
>> +     serial_out(up, UART_LCR, up->lcr);
>> +     /* UART 16x mode */
>> +     serial_out(up, UART_OMAP_MDR1, up->mdr1);
>> +}
>> +
>> +static int omap_serial_runtime_suspend(struct device *dev)
>> +{
>> +     struct uart_omap_port *up = dev_get_drvdata(dev);
>> +
>> +     if (!up)
>> +             goto done;
>> +
>> +     if (device_may_wakeup(dev))
>> +             up->enable_wakeup(up->pdev, true);
>> +     else
>> +             up->enable_wakeup(up->pdev, false);
>
> I know the earlier code was doing it this way too, but an optimization
> would be to have the driver keep state whether the wakeups are enabled
> or disabled, so you don't have to keep calling this function (which can
> be expensive with the PRCM reads/writes.
>

if dynamically disabled from user space from sys-fs interface.
it may not reflect disable_wakup immediately if internal state machine of
wakeup is maintained within uart driver.

also need to modify the internals of this func pointer to use
hmwod API's as commented above.

> That state can also be used in the suspend path, which should also be
> managing wakeups.
>

ok.

>> +done:
>> +     return 0;
>> +}
>> +
>> +static int omap_serial_runtime_resume(struct device *dev)
>> +{
>> +     struct uart_omap_port *up = dev_get_drvdata(dev);
>> +
>> +     if (up)
>> +             omap_uart_restore_context(up);
>
> You only need to restore context when returning from off-mode.  You
> should be using omap_device_get_context_loss_count (called via pdata
> function pointer) for this.  See the MMC driver or DSS driver for how
> this is done.
>

agree, will use that API.

--
Thanks,
Govindraj.R

>> +     return 0;
>> +}
>> +
>> +static const struct dev_pm_ops omap_serial_dev_pm_ops = {
>> +     .suspend = serial_omap_suspend,
>> +     .resume = serial_omap_resume,
>> +     .runtime_suspend = omap_serial_runtime_suspend,
>> +     .runtime_resume = omap_serial_runtime_resume,
>> +};
>> +
>>  static struct platform_driver serial_omap_driver = {
>>       .probe          = serial_omap_probe,
>>       .remove         = serial_omap_remove,
>> -
>> -     .suspend        = serial_omap_suspend,
>> -     .resume         = serial_omap_resume,
>>       .driver         = {
>>               .name   = DRIVER_NAME,
>> +             .pm = &omap_serial_dev_pm_ops,
>>       },
>>  };
>
> Kevin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Kevin Hilman June 27, 2011, 10:57 p.m. UTC | #7

Govindraj <[e-mail]> writes:

> On Sat, Jun 25, 2011 at 5:00 AM, Kevin Hilman <[e-mail]> wrote:
>> "Govindraj.R" <[e-mail]> writes:
>>
>>> Adapts omap-serial driver to use pm_runtime api's.
>>>
>>> 1.) Populate reg values to uart port which can be used for context restore.
>>
>> Please make this part a separate patch.
>>
>>> 2.) Moving context_restore func to driver from serial.c
>>> 3.) Adding port_enable/disable func to enable/disable given uart port.
>>>     enable port using get_sync and disable using autosuspend.
>>> 4.) using runtime irq safe api to make get_sync be called from irq context.
>>
>>>
>>> Acked-by: Alan Cox <[e-mail]>
>>> Signed-off-by: Govindraj.R <[e-mail]>
>>
>> This is great!  we're almost there.   Some minor comments below...
>>
>>> ---
>>>  arch/arm/mach-omap2/serial.c                  |   22 +++
>>>  arch/arm/plat-omap/include/plat/omap-serial.h |    2 +
>>>  drivers/tty/serial/omap-serial.c              |  212 ++++++++++++++++++++++---
>>>  3 files changed, 210 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
>>> index 8c1a4c7..1651c2c 100644
>>> --- a/arch/arm/mach-omap2/serial.c
>>> +++ b/arch/arm/mach-omap2/serial.c
>>> @@ -189,6 +189,27 @@ static void omap_serial_fill_default_pads(struct omap_board_data *bdata)
>>>       }
>>>  }
>>>
>>> +static void omap_uart_wakeup_enable(struct platform_device *pdev, bool enable)
>>> +{
>>> +     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);
>>> +     }
>>
>> I think I asked this in previous series, but can't remember the answer
>> now...  can't we enable bits in the WKEN registers once at init time,
>> and then just use omap_hwmod_[enable|disable]_wakeup() here?
>>
>
> by default all bits are enabled in WKEN,

where default is the PRCM init code, yes.

> I will use omap_hwmod_[enable|disable]_wakeup() api's
>
> if these API's take care of WKEN regs, then no issue
> using the same.

No, these APIs only affect the module-level wakeup bit in the modules
SYSCONFIG register (ENAWAKEUP bit.)

My question is assuming the PRCM WKEN bits are just left enabled, is
simply toggling the modules SYSCONFIG.ENAWAKEUP bit enough to
enable/disable module wakeups.  I assume so, but have not validated it.

>>> +     /* Enable or clear io-pad wakeup */
>>> +     if (enable)
>>> +             omap_device_enable_ioring_wakeup(pdev);
>>> +     else
>>> +             omap_device_disable_ioring_wakeup(pdev);
>>> +}
>>> +
>>>  static void omap_uart_idle_init(struct omap_uart_port_info *uart,
>>>                               unsigned short num)
>>>  {
>>> @@ -332,6 +353,7 @@ void __init omap_serial_init_port(struct omap_board_data *bdata)
>>>
>>>       pdata->uartclk = OMAP24XX_BASE_BAUD * 16;
>>>       pdata->flags = UPF_BOOT_AUTOCONF;
>>> +     pdata->enable_wakeup = omap_uart_wakeup_enable;
>>>       if (bdata->id == omap_uart_con_id)
>>>               pdata->console_uart = true;
>>>
>>> diff --git a/arch/arm/plat-omap/include/plat/omap-serial.h b/arch/arm/plat-omap/include/plat/omap-serial.h
>>> index 2ca885b..ac30de8 100644
>>> --- a/arch/arm/plat-omap/include/plat/omap-serial.h
>>> +++ b/arch/arm/plat-omap/include/plat/omap-serial.h
>>> @@ -65,6 +65,7 @@ struct omap_uart_port_info {
>>>       unsigned int            errata;
>>>       unsigned int            console_uart;
>>>
>>> +     void (*enable_wakeup)(struct platform_device *, bool);
>>>       void __iomem *wk_st;
>>>       void __iomem *wk_en;
>>>       u32 wk_mask;
>>> @@ -120,6 +121,7 @@ struct uart_omap_port {
>>>       char                    name[20];
>>>       unsigned long           port_activity;
>>>       unsigned int            errata;
>>> +     void (*enable_wakeup)(struct platform_device *, bool);
>>>  };
>>>
>>>  #endif /* __OMAP_SERIAL_H__ */
>>> diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
>>> index 47cadf4..897416f 100644
>>> --- a/drivers/tty/serial/omap-serial.c
>>> +++ b/drivers/tty/serial/omap-serial.c
>>> @@ -37,10 +37,14 @@
>>>  #include <linux/clk.h>
>>>  #include <linux/serial_core.h>
>>>  #include <linux/irq.h>
>>> +#include <linux/pm_runtime.h>
>>>
>>>  #include <plat/dma.h>
>>>  #include <plat/dmtimer.h>
>>>  #include <plat/omap-serial.h>
>>> +#include <plat/omap_device.h>
>>> +
>>> +#define OMAP_UART_AUTOSUSPEND_DELAY (30 * HZ) /* Value is msecs */
>>
>> As Jon already pointed out, the HZ here is wrong.  Please define this
>> value in msecs.
>>
>
> corrected.
>
>>>  static struct uart_omap_port *ui[OMAP_MAX_HSUART_PORTS];
>>>
>>> @@ -94,6 +98,17 @@ serial_omap_get_divisor(struct uart_port *port, unsigned int baud)
>>>       return port->uartclk/(baud * divisor);
>>>  }
>>>
>>> +static inline void serial_omap_port_disable(struct uart_omap_port *up)
>>> +{
>>> +     pm_runtime_mark_last_busy(&up->pdev->dev);
>>> +     pm_runtime_put_autosuspend(&up->pdev->dev);
>>> +}
>>> +
>>> +static inline void serial_omap_port_enable(struct uart_omap_port *up)
>>> +{
>>> +     pm_runtime_get_sync(&up->pdev->dev);
>>> +}
>>
>> These inlines are not needed.  Please use runtime PM calls directly in
>> the code.  For _enable(), this is straight forward.  For _disable()
>> though, I don't think you want (or need) to _mark_last_busy() for every
>> instance of omap_port_disable().  You only need to do this for ones
>> TX/RX transactions...
>>
>
> Fine. will modify.
>
>
>>>  static void serial_omap_stop_rxdma(struct uart_omap_port *up)
>>>  {
>>>       if (up->uart_dma.rx_dma_used) {
>>> @@ -110,8 +125,11 @@ static void serial_omap_enable_ms(struct uart_port *port)
>>>       struct uart_omap_port *up = (struct uart_omap_port *)port;
>>>
>>>       dev_dbg(up->port.dev, "serial_omap_enable_ms+%d\n", up->pdev->id);
>>> +
>>> +     serial_omap_port_enable(up);
>>>       up->ier |= UART_IER_MSI;
>>>       serial_out(up, UART_IER, up->ier);
>>> +     serial_omap_port_disable(up);
>>
>> ...for example, this one is not really activity based, so should
>> probably just be pm_runtime_get_sync(), write the register, then
>> pm_runtime_put() (async version.)
>>
>> I didn't look at all the others below, but they should be looked at
>> individually.
>>
>
> ok. I will check them.
>
>>>  }
>>>
>>>  static void serial_omap_stop_tx(struct uart_port *port)
>>> @@ -131,21 +149,26 @@ static void serial_omap_stop_tx(struct uart_port *port)
>>>               up->uart_dma.tx_dma_channel = OMAP_UART_DMA_CH_FREE;
>>>       }
>>>
>>> +     serial_omap_port_enable(up);
>>>       if (up->ier & UART_IER_THRI) {
>>>               up->ier &= ~UART_IER_THRI;
>>>               serial_out(up, UART_IER, up->ier);
>>>       }
>>> +
>>> +     serial_omap_port_disable(up);
>>>  }
>>>
>>>  static void serial_omap_stop_rx(struct uart_port *port)
>>>  {
>>>       struct uart_omap_port *up = (struct uart_omap_port *)port;
>>>
>>> +     serial_omap_port_enable(up);
>>>       if (up->use_dma)
>>>               serial_omap_stop_rxdma(up);
>>>       up->ier &= ~UART_IER_RLSI;
>>>       up->port.read_status_mask &= ~UART_LSR_DR;
>>>       serial_out(up, UART_IER, up->ier);
>>> +     serial_omap_port_disable(up);
>>>  }
>>>
>>>  static inline void receive_chars(struct uart_omap_port *up, int *status)
>>> @@ -261,8 +284,10 @@ static void serial_omap_start_tx(struct uart_port *port)
>>>       unsigned int start;
>>>       int ret = 0;
>>>
>>> +     serial_omap_port_enable(up);
>>>       if (!up->use_dma) {
>>>               serial_omap_enable_ier_thri(up);
>>> +             serial_omap_port_disable(up);
>>>               return;
>>>       }
>>>
>>> @@ -354,9 +379,12 @@ static inline irqreturn_t serial_omap_irq(int irq, void *dev_id)
>>>       unsigned int iir, lsr;
>>>       unsigned long flags;
>>>
>>> +     serial_omap_port_enable(up);
>>>       iir = serial_in(up, UART_IIR);
>>> -     if (iir & UART_IIR_NO_INT)
>>> +     if (iir & UART_IIR_NO_INT) {
>>> +             serial_omap_port_disable(up);
>>>               return IRQ_NONE;
>>> +     }
>>>
>>>       spin_lock_irqsave(&up->port.lock, flags);
>>>       lsr = serial_in(up, UART_LSR);
>>> @@ -378,6 +406,8 @@ static inline irqreturn_t serial_omap_irq(int irq, void *dev_id)
>>>               transmit_chars(up);
>>>
>>>       spin_unlock_irqrestore(&up->port.lock, flags);
>>> +     serial_omap_port_disable(up);
>>> +
>>>       up->port_activity = jiffies;
>>>       return IRQ_HANDLED;
>>>  }
>>> @@ -388,11 +418,12 @@ static unsigned int serial_omap_tx_empty(struct uart_port *port)
>>>       unsigned long flags = 0;
>>>       unsigned int ret = 0;
>>>
>>> +     serial_omap_port_enable(up);
>>>       dev_dbg(up->port.dev, "serial_omap_tx_empty+%d\n", up->pdev->id);
>>>       spin_lock_irqsave(&up->port.lock, flags);
>>>       ret = serial_in(up, UART_LSR) & UART_LSR_TEMT ? TIOCSER_TEMT : 0;
>>>       spin_unlock_irqrestore(&up->port.lock, flags);
>>> -
>>> +     serial_omap_port_disable(up);
>>>       return ret;
>>>  }
>>>
>>> @@ -402,7 +433,10 @@ static unsigned int serial_omap_get_mctrl(struct uart_port *port)
>>>       unsigned char status;
>>>       unsigned int ret = 0;
>>>
>>> +     serial_omap_port_enable(up);
>>>       status = check_modem_status(up);
>>> +     serial_omap_port_disable(up);
>>> +
>>>       dev_dbg(up->port.dev, "serial_omap_get_mctrl+%d\n", up->pdev->id);
>>>
>>>       if (status & UART_MSR_DCD)
>>> @@ -434,7 +468,9 @@ static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl)
>>>               mcr |= UART_MCR_LOOP;
>>>
>>>       mcr |= up->mcr;
>>> +     serial_omap_port_enable(up);
>>>       serial_out(up, UART_MCR, mcr);
>>> +     serial_omap_port_disable(up);
>>>  }
>>>
>>>  static void serial_omap_break_ctl(struct uart_port *port, int break_state)
>>> @@ -443,6 +479,7 @@ static void serial_omap_break_ctl(struct uart_port *port, int break_state)
>>>       unsigned long flags = 0;
>>>
>>>       dev_dbg(up->port.dev, "serial_omap_break_ctl+%d\n", up->pdev->id);
>>> +     serial_omap_port_enable(up);
>>>       spin_lock_irqsave(&up->port.lock, flags);
>>>       if (break_state == -1)
>>>               up->lcr |= UART_LCR_SBC;
>>> @@ -450,6 +487,7 @@ static void serial_omap_break_ctl(struct uart_port *port, int break_state)
>>>               up->lcr &= ~UART_LCR_SBC;
>>>       serial_out(up, UART_LCR, up->lcr);
>>>       spin_unlock_irqrestore(&up->port.lock, flags);
>>> +     serial_omap_port_disable(up);
>>>  }
>>>
>>>  static int serial_omap_startup(struct uart_port *port)
>>> @@ -468,6 +506,7 @@ static int serial_omap_startup(struct uart_port *port)
>>>
>>>       dev_dbg(up->port.dev, "serial_omap_startup+%d\n", up->pdev->id);
>>>
>>> +     serial_omap_port_enable(up);
>>>       /*
>>>        * Clear the FIFO buffers and disable them.
>>>        * (they will be reenabled in set_termios())
>>> @@ -523,6 +562,7 @@ static int serial_omap_startup(struct uart_port *port)
>>>       /* Enable module level wake up */
>>>       serial_out(up, UART_OMAP_WER, OMAP_UART_WER_MOD_WKUP);
>>>
>>> +     serial_omap_port_disable(up);
>>>       up->port_activity = jiffies;
>>>       return 0;
>>>  }
>>> @@ -533,6 +573,8 @@ static void serial_omap_shutdown(struct uart_port *port)
>>>       unsigned long flags = 0;
>>>
>>>       dev_dbg(up->port.dev, "serial_omap_shutdown+%d\n", up->pdev->id);
>>> +
>>> +     serial_omap_port_enable(up);
>>>       /*
>>>        * Disable interrupts from this port
>>>        */
>>> @@ -566,6 +608,7 @@ static void serial_omap_shutdown(struct uart_port *port)
>>>                       up->uart_dma.rx_buf_dma_phys);
>>>               up->uart_dma.rx_buf = NULL;
>>>       }
>>> +     serial_omap_port_disable(up);
>>>       free_irq(up->port.irq, up);
>>>  }
>>>
>>> @@ -671,6 +714,10 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>>>       baud = uart_get_baud_rate(port, termios, old, 0, port->uartclk/13);
>>>       quot = serial_omap_get_divisor(port, baud);
>>>
>>> +     up->dll = quot & 0xff;
>>> +     up->dlh = quot >> 8;
>>> +     up->mdr1 = UART_OMAP_MDR1_DISABLE;
>>> +
>>>       up->fcr = UART_FCR_R_TRIG_01 | UART_FCR_T_TRIG_01 |
>>>                       UART_FCR_ENABLE_FIFO;
>>>       if (up->use_dma)
>>> @@ -680,6 +727,7 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>>>        * Ok, we're now changing the port state. Do it with
>>>        * interrupts disabled.
>>>        */
>>> +     serial_omap_port_enable(up);
>>>       spin_lock_irqsave(&up->port.lock, flags);
>>>
>>>       /*
>>> @@ -723,6 +771,7 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>>>               up->ier |= UART_IER_MSI;
>>>       serial_out(up, UART_IER, up->ier);
>>>       serial_out(up, UART_LCR, cval);         /* reset DLAB */
>>> +     up->lcr = cval;
>>>
>>>       /* FIFOs and DMA Settings */
>>>
>>> @@ -758,8 +807,7 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>>>       serial_out(up, UART_MCR, up->mcr);
>>>
>>>       /* Protocol, Baud Rate, and Interrupt Settings */
>>> -
>>> -     serial_out(up, UART_OMAP_MDR1, UART_OMAP_MDR1_DISABLE);
>>> +     serial_out(up, UART_OMAP_MDR1, up->mdr1);
>>>       serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
>>>
>>>       up->efr = serial_in(up, UART_EFR);
>>> @@ -769,8 +817,8 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>>>       serial_out(up, UART_IER, 0);
>>>       serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
>>>
>>> -     serial_out(up, UART_DLL, quot & 0xff);          /* LS of divisor */
>>> -     serial_out(up, UART_DLM, quot >> 8);            /* MS of divisor */
>>> +     serial_out(up, UART_DLL, up->dll);      /* LS of divisor */
>>> +     serial_out(up, UART_DLM, up->dlh);      /* MS of divisor */
>>>
>>>       serial_out(up, UART_LCR, 0);
>>>       serial_out(up, UART_IER, up->ier);
>>> @@ -780,9 +828,11 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>>>       serial_out(up, UART_LCR, cval);
>>>
>>>       if (baud > 230400 && baud != 3000000)
>>> -             serial_out(up, UART_OMAP_MDR1, UART_OMAP_MDR1_13X_MODE);
>>> +             up->mdr1 = UART_OMAP_MDR1_13X_MODE;
>>>       else
>>> -             serial_out(up, UART_OMAP_MDR1, UART_OMAP_MDR1_16X_MODE);
>>> +             up->mdr1 = UART_OMAP_MDR1_16X_MODE;
>>> +
>>> +     serial_out(up, UART_OMAP_MDR1, up->mdr1)
>>>
>>>       /* Hardware Flow Control Configuration */
>>>
>>> @@ -810,6 +860,7 @@ serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
>>>               serial_omap_configure_xonxoff(up, termios);
>>>
>>>       spin_unlock_irqrestore(&up->port.lock, flags);
>>> +     serial_omap_port_disable(up);
>>>       dev_dbg(up->port.dev, "serial_omap_set_termios+%d\n", up->pdev->id);
>>>  }
>>>
>>> @@ -821,6 +872,8 @@ serial_omap_pm(struct uart_port *port, unsigned int state,
>>>       unsigned char efr;
>>>
>>>       dev_dbg(up->port.dev, "serial_omap_pm+%d\n", up->pdev->id);
>>> +
>>> +     serial_omap_port_enable(up);
>>>       serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
>>>       efr = serial_in(up, UART_EFR);
>>>       serial_out(up, UART_EFR, efr | UART_EFR_ECB);
>>> @@ -830,6 +883,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)
>>> +             pm_runtime_put_sync(&up->pdev->dev);
>>> +     else
>>> +             serial_omap_port_disable(up);
>>
>> OK, so this boils down to either a _put_sync() or a _mark_last_busy +
>> _put_autosuspend(), depending on whether this is suspending or resuming,
>> right?
>>
>
> yes also during bootup.
> disable the ports immediately that are not used during bootup.
>
>> Why the difference?
>
> Need to put the ports down immediately during system wide suspend
> other wise autosuspend delay will prevent system to enter
> suspend state immediately.

OK, as I stated above.  All of these serial_omap_port_disable() calls
need to be removed and replaced with runtime PM API calls directly.

Here, I think it can always be a basic _put() for suspend and
_get_sync() for resume.

Incendentally, the 'else' clause of that code (state == 0) should be the
resume case, but you're calling _disable().  That doesn't look right.

>>
>>>  }
>>>
>>>  static void serial_omap_release_port(struct uart_port *port)
>>> @@ -907,25 +964,31 @@ static inline void wait_for_xmitr(struct uart_omap_port *up)
>>>  static void serial_omap_poll_put_char(struct uart_port *port, unsigned char ch)
>>>  {
>>>       struct uart_omap_port *up = (struct uart_omap_port *)port;
>>> +
>>> +     serial_omap_port_enable(up);
>>>       wait_for_xmitr(up);
>>>       serial_out(up, UART_TX, ch);
>>> +     serial_omap_port_disable(up);
>>>  }
>>>
>>>  static int serial_omap_poll_get_char(struct uart_port *port)
>>>  {
>>>       struct uart_omap_port *up = (struct uart_omap_port *)port;
>>> -     unsigned int status = serial_in(up, UART_LSR);
>>> +     unsigned int status;
>>>
>>> +     serial_omap_port_enable(up);
>>> +     status = serial_in(up, UART_LSR);
>>>       if (!(status & UART_LSR_DR))
>>>               return NO_POLL_CHAR;
>>>
>>> -     return serial_in(up, UART_RX);
>>> +     status = serial_in(up, UART_RX);
>>> +     serial_omap_port_disable(up);
>>> +     return status;
>>>  }
>>>
>>>  #endif /* CONFIG_CONSOLE_POLL */
>>>
>>>  #ifdef CONFIG_SERIAL_OMAP_CONSOLE
>>> -
>>>  static struct uart_omap_port *serial_omap_console_ports[4];
>>>
>>>  static struct uart_driver serial_omap_reg;
>>> @@ -955,6 +1018,8 @@ serial_omap_console_write(struct console *co, const char *s,
>>>       else
>>>               spin_lock(&up->port.lock);
>>>
>>> +     serial_omap_port_enable(up);
>>> +
>>>       /*
>>>        * First save the IER then disable the interrupts
>>>        */
>>> @@ -979,6 +1044,7 @@ serial_omap_console_write(struct console *co, const char *s,
>>>       if (up->msr_saved_flags)
>>>               check_modem_status(up);
>>>
>>> +     serial_omap_port_disable(up);
>>>       if (locked)
>>>               spin_unlock(&up->port.lock);
>>>       local_irq_restore(flags);
>>> @@ -1061,22 +1127,27 @@ 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);
>>> +             console_trylock();
>>
>> This locking needs to be explained.  Why it is needed, but very
>> importantly, why you are not checking the return value of the _trylock()
>>
>
> any print after this point can result in failure of immediate system suspending
> due to delayed autosuspend from omap_console_write.

I mean explained in the changelog and preferrably the code.

Also, technically speaking, it's not until the clocks are cut (after the
devices runtime_suspend callback is called) that UART writes will fault.

> log prints after uart suspend and print them during resume.
>
>
>>> +             serial_omap_pm(&up->port, 3, 0);
>>
>> Why is this call needed?
>>
>
> Actually this is needed if no_console_suspend is used, for that
> case the console will remain active since serial_omap_pm is not
> getting called from serial_core and port is not suspended
> immediately with put_sync.
>
> prints during system wide suspend and delayed autosuspend
> from console_write keeps system active in no_console_suspend case
> so put in forced suspend state and log all prints to be printed later.
>
> probably needs to a condition check if (no_console_suspend)

No.  If the user has requested no_console_suspend, then the user should
expect that the UARTs remain active and thus prevent low-power.

>> uart_suspend_port() calls uart_change_pm() which should call the ->pm
>> method of struct uart_ops (which is serial_omap_pm).  What am I missing?
>>
>> Also notice you're not calling serial_omap_pm() in the resume method
>> below to change it back.
>>
>>> +     }
>>>       return 0;
>>>  }
>>
>> Also, device wakeup capability should be checked and enabled/disabled in
>> the suspend path (not only the runtime suspend path.)
>>
>
> ok.
>
>
>>> -static int serial_omap_resume(struct platform_device *dev)
>>> +static int serial_omap_resume(struct device *dev)
>>>  {
>>> -     struct uart_omap_port *up = platform_get_drvdata(dev);
>>> +     struct uart_omap_port *up = dev_get_drvdata(dev);
>>>
>>> -     if (up)
>>> +     if (up) {
>>>               uart_resume_port(&serial_omap_reg, &up->port);
>>> +             console_unlock();
>>
>> Again, please describe locking in comments.
>>
>> Also, what happens here if the console_trylock() in your suspend method
>> failed?
>>
>
> need to add a flag to check and unlock
> from return status of trylock.
>
>>> +     }
>>> +
>>>       return 0;
>>>  }
>>>
>>> @@ -1097,6 +1168,7 @@ static void serial_omap_rx_timeout(unsigned long uart_no)
>>>                       serial_omap_stop_rxdma(up);
>>>                       up->ier |= (UART_IER_RDI | UART_IER_RLSI);
>>>                       serial_out(up, UART_IER, up->ier);
>>> +                     serial_omap_port_disable(up);
>>>               }
>>>               return;
>>>       }
>>> @@ -1128,6 +1200,9 @@ static void serial_omap_rx_timeout(unsigned long uart_no)
>>>
>>>  static void uart_rx_dma_callback(int lch, u16 ch_status, void *data)
>>>  {
>>> +     struct uart_omap_port *up = (struct uart_omap_port *)data;
>>> +
>>> +     serial_omap_port_disable(up);
>>>       return;
>>>  }
>>>
>>> @@ -1135,6 +1210,7 @@ static int serial_omap_start_rxdma(struct uart_omap_port *up)
>>>  {
>>>       int ret = 0;
>>>
>>> +     serial_omap_port_enable(up);
>>>       if (up->uart_dma.rx_dma_channel == -1) {
>>>               ret = omap_request_dma(up->uart_dma.uart_dma_rx,
>>>                               "UART Rx DMA",
>>> @@ -1214,6 +1290,7 @@ static void uart_tx_dma_callback(int lch, u16 ch_status, void *data)
>>>               serial_omap_stop_tx(&up->port);
>>>               up->uart_dma.tx_dma_used = false;
>>>               spin_unlock(&(up->uart_dma.tx_lock));
>>> +             serial_omap_port_disable(up);
>>>       } else {
>>>               omap_stop_dma(up->uart_dma.tx_dma_channel);
>>>               serial_omap_continue_tx(up);
>>> @@ -1224,9 +1301,10 @@ static void uart_tx_dma_callback(int lch, u16 ch_status, void *data)
>>>
>>>  static int serial_omap_probe(struct platform_device *pdev)
>>>  {
>>> -     struct uart_omap_port   *up;
>>> +     struct uart_omap_port   *up = NULL;
>>>       struct resource         *mem, *irq, *dma_tx, *dma_rx;
>>>       struct omap_uart_port_info *omap_up_info = pdev->dev.platform_data;
>>> +     struct omap_device *od;
>>>       int ret = -ENOSPC;
>>>
>>>       mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> @@ -1276,12 +1354,20 @@ static int serial_omap_probe(struct platform_device *pdev)
>>>       up->port.ops = &serial_omap_pops;
>>>       up->port.line = pdev->id;
>>>
>>> -     up->port.membase = omap_up_info->membase;
>>> -     up->port.mapbase = omap_up_info->mapbase;
>>> +     up->port.mapbase = mem->start;
>>> +     up->port.membase = ioremap(mem->start, mem->end - mem->start);
>>> +
>>> +     if (!up->port.membase) {
>>> +             dev_err(&pdev->dev, "can't ioremap UART\n");
>>> +             ret = -ENOMEM;
>>> +             goto err1;
>>> +     }
>>> +
>>>       up->port.flags = omap_up_info->flags;
>>> -     up->port.irqflags = omap_up_info->irqflags;
>>>       up->port.uartclk = omap_up_info->uartclk;
>>>       up->uart_dma.uart_base = mem->start;
>>> +     up->errata = omap_up_info->errata;
>>> +     up->enable_wakeup = omap_up_info->enable_wakeup;
>>>
>>>       if (omap_up_info->dma_enabled) {
>>>               up->uart_dma.uart_dma_tx = dma_tx->start;
>>> @@ -1295,18 +1381,36 @@ 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_enable(&pdev->dev);
>>> +     pm_runtime_irq_safe(&pdev->dev);
>>> +
>>> +     if (omap_up_info->console_uart) {
>>> +             od = to_omap_device(up->pdev);
>>> +             omap_hwmod_idle(od->hwmods[0]);
>>> +             serial_omap_port_enable(up);
>>> +             serial_omap_port_disable(up);
>>> +     }
>>> +
>>>       ui[pdev->id] = up;
>>>       serial_omap_add_console_port(up);
>>>
>>>       ret = uart_add_one_port(&serial_omap_reg, &up->port);
>>>       if (ret != 0)
>>> -             goto do_release_region;
>>> +             goto err1;
>>>
>>> +     dev_set_drvdata(&pdev->dev, up);
>>>       platform_set_drvdata(pdev, up);
>>> +
>>>       return 0;
>>>  err:
>>>       dev_err(&pdev->dev, "[UART%d]: failure [%s]: %d\n",
>>>                               pdev->id, __func__, ret);
>>> +err1:
>>> +     kfree(up);
>>>  do_release_region:
>>>       release_mem_region(mem->start, (mem->end - mem->start) + 1);
>>>       return ret;
>>> @@ -1318,20 +1422,76 @@ static int serial_omap_remove(struct platform_device *dev)
>>>
>>>       platform_set_drvdata(dev, NULL);
>>>       if (up) {
>>> +             pm_runtime_disable(&up->pdev->dev);
>>>               uart_remove_one_port(&serial_omap_reg, &up->port);
>>>               kfree(up);
>>>       }
>>>       return 0;
>>>  }
>>>
>>> +static void omap_uart_restore_context(struct uart_omap_port *up)
>>> +{
>>> +     u16 efr = 0;
>>> +
>>> +     serial_out(up, UART_OMAP_MDR1, up->mdr1);
>>> +     serial_out(up, UART_LCR, 0xBF); /* Config B mode */
>>> +     efr = serial_in(up, UART_EFR);
>>> +     serial_out(up, UART_EFR, UART_EFR_ECB);
>>> +     serial_out(up, UART_LCR, 0x0); /* Operational mode */
>>> +     serial_out(up, UART_IER, 0x0);
>>> +     serial_out(up, UART_LCR, 0xBF); /* Config B mode */
>>> +     serial_out(up, UART_DLL, up->dll);
>>> +     serial_out(up, UART_DLM, up->dlh);
>>> +     serial_out(up, UART_LCR, 0x0); /* Operational mode */
>>> +     serial_out(up, UART_IER, up->ier);
>>> +     serial_out(up, UART_FCR, up->fcr);
>>> +     serial_out(up, UART_LCR, 0x80);
>>> +     serial_out(up, UART_MCR, up->mcr);
>>> +     serial_out(up, UART_LCR, 0xBF); /* Config B mode */
>>> +     serial_out(up, UART_EFR, efr);
>>> +     serial_out(up, UART_LCR, up->lcr);
>>> +     /* UART 16x mode */
>>> +     serial_out(up, UART_OMAP_MDR1, up->mdr1);
>>> +}
>>> +
>>> +static int omap_serial_runtime_suspend(struct device *dev)
>>> +{
>>> +     struct uart_omap_port *up = dev_get_drvdata(dev);
>>> +
>>> +     if (!up)
>>> +             goto done;
>>> +
>>> +     if (device_may_wakeup(dev))
>>> +             up->enable_wakeup(up->pdev, true);
>>> +     else
>>> +             up->enable_wakeup(up->pdev, false);
>>
>> I know the earlier code was doing it this way too, but an optimization
>> would be to have the driver keep state whether the wakeups are enabled
>> or disabled, so you don't have to keep calling this function (which can
>> be expensive with the PRCM reads/writes.
>>
>
> if dynamically disabled from user space from sys-fs interface.
> it may not reflect disable_wakup immediately if internal state machine of
> wakeup is maintained within uart driver.

Why does that matter?  It does not need to take effect immedately, it
only needs to take effect on suspend.

Just update internal state whenever you change the hardware, and whenver
you're about to change the hardware again, check device_may_wakeup() and
check the driver's internal state. 

> also need to modify the internals of this func pointer to use
> hmwod API's as commented above.

OK

Patch

diff --git a/arch/arm/mach-omap2/serial.c b/arch/arm/mach-omap2/serial.c
index 8c1a4c7..1651c2c 100644
--- a/arch/arm/mach-omap2/serial.c
+++ b/arch/arm/mach-omap2/serial.c
@@ -189,6 +189,27 @@  static void omap_serial_fill_default_pads(struct omap_board_data *bdata)
 	}
 }
 
+static void omap_uart_wakeup_enable(struct platform_device *pdev, bool enable)
+{
+	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);
+	}
+
+	/* Enable or clear io-pad wakeup */
+	if (enable)
+		omap_device_enable_ioring_wakeup(pdev);
+	else
+		omap_device_disable_ioring_wakeup(pdev);
+}
+
 static void omap_uart_idle_init(struct omap_uart_port_info *uart,
 				unsigned short num)
 {
@@ -332,6 +353,7 @@  void __init omap_serial_init_port(struct omap_board_data *bdata)
 
 	pdata->uartclk = OMAP24XX_BASE_BAUD * 16;
 	pdata->flags = UPF_BOOT_AUTOCONF;
+	pdata->enable_wakeup = omap_uart_wakeup_enable;
 	if (bdata->id == omap_uart_con_id)
 		pdata->console_uart = true;
 
diff --git a/arch/arm/plat-omap/include/plat/omap-serial.h b/arch/arm/plat-omap/include/plat/omap-serial.h
index 2ca885b..ac30de8 100644
--- a/arch/arm/plat-omap/include/plat/omap-serial.h
+++ b/arch/arm/plat-omap/include/plat/omap-serial.h
@@ -65,6 +65,7 @@  struct omap_uart_port_info {
 	unsigned int		errata;
 	unsigned int		console_uart;
 
+	void (*enable_wakeup)(struct platform_device *, bool);
 	void __iomem *wk_st;
 	void __iomem *wk_en;
 	u32 wk_mask;
@@ -120,6 +121,7 @@  struct uart_omap_port {
 	char			name[20];
 	unsigned long		port_activity;
 	unsigned int		errata;
+	void (*enable_wakeup)(struct platform_device *, bool);
 };
 
 #endif /* __OMAP_SERIAL_H__ */
diff --git a/drivers/tty/serial/omap-serial.c b/drivers/tty/serial/omap-serial.c
index 47cadf4..897416f 100644
--- a/drivers/tty/serial/omap-serial.c
+++ b/drivers/tty/serial/omap-serial.c
@@ -37,10 +37,14 @@ 
 #include <linux/clk.h>
 #include <linux/serial_core.h>
 #include <linux/irq.h>
+#include <linux/pm_runtime.h>
 
 #include <plat/dma.h>
 #include <plat/dmtimer.h>
 #include <plat/omap-serial.h>
+#include <plat/omap_device.h>
+
+#define OMAP_UART_AUTOSUSPEND_DELAY (30 * HZ) /* Value is msecs */
 
 static struct uart_omap_port *ui[OMAP_MAX_HSUART_PORTS];
 
@@ -94,6 +98,17 @@  serial_omap_get_divisor(struct uart_port *port, unsigned int baud)
 	return port->uartclk/(baud * divisor);
 }
 
+static inline void serial_omap_port_disable(struct uart_omap_port *up)
+{
+	pm_runtime_mark_last_busy(&up->pdev->dev);
+	pm_runtime_put_autosuspend(&up->pdev->dev);
+}
+
+static inline void serial_omap_port_enable(struct uart_omap_port *up)
+{
+	pm_runtime_get_sync(&up->pdev->dev);
+}
+
 static void serial_omap_stop_rxdma(struct uart_omap_port *up)
 {
 	if (up->uart_dma.rx_dma_used) {
@@ -110,8 +125,11 @@  static void serial_omap_enable_ms(struct uart_port *port)
 	struct uart_omap_port *up = (struct uart_omap_port *)port;
 
 	dev_dbg(up->port.dev, "serial_omap_enable_ms+%d\n", up->pdev->id);
+
+	serial_omap_port_enable(up);
 	up->ier |= UART_IER_MSI;
 	serial_out(up, UART_IER, up->ier);
+	serial_omap_port_disable(up);
 }
 
 static void serial_omap_stop_tx(struct uart_port *port)
@@ -131,21 +149,26 @@  static void serial_omap_stop_tx(struct uart_port *port)
 		up->uart_dma.tx_dma_channel = OMAP_UART_DMA_CH_FREE;
 	}
 
+	serial_omap_port_enable(up);
 	if (up->ier & UART_IER_THRI) {
 		up->ier &= ~UART_IER_THRI;
 		serial_out(up, UART_IER, up->ier);
 	}
+
+	serial_omap_port_disable(up);
 }
 
 static void serial_omap_stop_rx(struct uart_port *port)
 {
 	struct uart_omap_port *up = (struct uart_omap_port *)port;
 
+	serial_omap_port_enable(up);
 	if (up->use_dma)
 		serial_omap_stop_rxdma(up);
 	up->ier &= ~UART_IER_RLSI;
 	up->port.read_status_mask &= ~UART_LSR_DR;
 	serial_out(up, UART_IER, up->ier);
+	serial_omap_port_disable(up);
 }
 
 static inline void receive_chars(struct uart_omap_port *up, int *status)
@@ -261,8 +284,10 @@  static void serial_omap_start_tx(struct uart_port *port)
 	unsigned int start;
 	int ret = 0;
 
+	serial_omap_port_enable(up);
 	if (!up->use_dma) {
 		serial_omap_enable_ier_thri(up);
+		serial_omap_port_disable(up);
 		return;
 	}
 
@@ -354,9 +379,12 @@  static inline irqreturn_t serial_omap_irq(int irq, void *dev_id)
 	unsigned int iir, lsr;
 	unsigned long flags;
 
+	serial_omap_port_enable(up);
 	iir = serial_in(up, UART_IIR);
-	if (iir & UART_IIR_NO_INT)
+	if (iir & UART_IIR_NO_INT) {
+		serial_omap_port_disable(up);
 		return IRQ_NONE;
+	}
 
 	spin_lock_irqsave(&up->port.lock, flags);
 	lsr = serial_in(up, UART_LSR);
@@ -378,6 +406,8 @@  static inline irqreturn_t serial_omap_irq(int irq, void *dev_id)
 		transmit_chars(up);
 
 	spin_unlock_irqrestore(&up->port.lock, flags);
+	serial_omap_port_disable(up);
+
 	up->port_activity = jiffies;
 	return IRQ_HANDLED;
 }
@@ -388,11 +418,12 @@  static unsigned int serial_omap_tx_empty(struct uart_port *port)
 	unsigned long flags = 0;
 	unsigned int ret = 0;
 
+	serial_omap_port_enable(up);
 	dev_dbg(up->port.dev, "serial_omap_tx_empty+%d\n", up->pdev->id);
 	spin_lock_irqsave(&up->port.lock, flags);
 	ret = serial_in(up, UART_LSR) & UART_LSR_TEMT ? TIOCSER_TEMT : 0;
 	spin_unlock_irqrestore(&up->port.lock, flags);
-
+	serial_omap_port_disable(up);
 	return ret;
 }
 
@@ -402,7 +433,10 @@  static unsigned int serial_omap_get_mctrl(struct uart_port *port)
 	unsigned char status;
 	unsigned int ret = 0;
 
+	serial_omap_port_enable(up);
 	status = check_modem_status(up);
+	serial_omap_port_disable(up);
+
 	dev_dbg(up->port.dev, "serial_omap_get_mctrl+%d\n", up->pdev->id);
 
 	if (status & UART_MSR_DCD)
@@ -434,7 +468,9 @@  static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl)
 		mcr |= UART_MCR_LOOP;
 
 	mcr |= up->mcr;
+	serial_omap_port_enable(up);
 	serial_out(up, UART_MCR, mcr);
+	serial_omap_port_disable(up);
 }
 
 static void serial_omap_break_ctl(struct uart_port *port, int break_state)
@@ -443,6 +479,7 @@  static void serial_omap_break_ctl(struct uart_port *port, int break_state)
 	unsigned long flags = 0;
 
 	dev_dbg(up->port.dev, "serial_omap_break_ctl+%d\n", up->pdev->id);
+	serial_omap_port_enable(up);
 	spin_lock_irqsave(&up->port.lock, flags);
 	if (break_state == -1)
 		up->lcr |= UART_LCR_SBC;
@@ -450,6 +487,7 @@  static void serial_omap_break_ctl(struct uart_port *port, int break_state)
 		up->lcr &= ~UART_LCR_SBC;
 	serial_out(up, UART_LCR, up->lcr);
 	spin_unlock_irqrestore(&up->port.lock, flags);
+	serial_omap_port_disable(up);
 }
 
 static int serial_omap_startup(struct uart_port *port)
@@ -468,6 +506,7 @@  static int serial_omap_startup(struct uart_port *port)
 
 	dev_dbg(up->port.dev, "serial_omap_startup+%d\n", up->pdev->id);
 
+	serial_omap_port_enable(up);
 	/*
 	 * Clear the FIFO buffers and disable them.
 	 * (they will be reenabled in set_termios())
@@ -523,6 +562,7 @@  static int serial_omap_startup(struct uart_port *port)
 	/* Enable module level wake up */
 	serial_out(up, UART_OMAP_WER, OMAP_UART_WER_MOD_WKUP);
 
+	serial_omap_port_disable(up);
 	up->port_activity = jiffies;
 	return 0;
 }
@@ -533,6 +573,8 @@  static void serial_omap_shutdown(struct uart_port *port)
 	unsigned long flags = 0;
 
 	dev_dbg(up->port.dev, "serial_omap_shutdown+%d\n", up->pdev->id);
+
+	serial_omap_port_enable(up);
 	/*
 	 * Disable interrupts from this port
 	 */
@@ -566,6 +608,7 @@  static void serial_omap_shutdown(struct uart_port *port)
 			up->uart_dma.rx_buf_dma_phys);
 		up->uart_dma.rx_buf = NULL;
 	}
+	serial_omap_port_disable(up);
 	free_irq(up->port.irq, up);
 }
 
@@ -671,6 +714,10 @@  serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
 	baud = uart_get_baud_rate(port, termios, old, 0, port->uartclk/13);
 	quot = serial_omap_get_divisor(port, baud);
 
+	up->dll = quot & 0xff;
+	up->dlh = quot >> 8;
+	up->mdr1 = UART_OMAP_MDR1_DISABLE;
+
 	up->fcr = UART_FCR_R_TRIG_01 | UART_FCR_T_TRIG_01 |
 			UART_FCR_ENABLE_FIFO;
 	if (up->use_dma)
@@ -680,6 +727,7 @@  serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
 	 * Ok, we're now changing the port state. Do it with
 	 * interrupts disabled.
 	 */
+	serial_omap_port_enable(up);
 	spin_lock_irqsave(&up->port.lock, flags);
 
 	/*
@@ -723,6 +771,7 @@  serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
 		up->ier |= UART_IER_MSI;
 	serial_out(up, UART_IER, up->ier);
 	serial_out(up, UART_LCR, cval);		/* reset DLAB */
+	up->lcr = cval;
 
 	/* FIFOs and DMA Settings */
 
@@ -758,8 +807,7 @@  serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
 	serial_out(up, UART_MCR, up->mcr);
 
 	/* Protocol, Baud Rate, and Interrupt Settings */
-
-	serial_out(up, UART_OMAP_MDR1, UART_OMAP_MDR1_DISABLE);
+	serial_out(up, UART_OMAP_MDR1, up->mdr1);
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
 
 	up->efr = serial_in(up, UART_EFR);
@@ -769,8 +817,8 @@  serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
 	serial_out(up, UART_IER, 0);
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
 
-	serial_out(up, UART_DLL, quot & 0xff);          /* LS of divisor */
-	serial_out(up, UART_DLM, quot >> 8);            /* MS of divisor */
+	serial_out(up, UART_DLL, up->dll);	/* LS of divisor */
+	serial_out(up, UART_DLM, up->dlh);	/* MS of divisor */
 
 	serial_out(up, UART_LCR, 0);
 	serial_out(up, UART_IER, up->ier);
@@ -780,9 +828,11 @@  serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
 	serial_out(up, UART_LCR, cval);
 
 	if (baud > 230400 && baud != 3000000)
-		serial_out(up, UART_OMAP_MDR1, UART_OMAP_MDR1_13X_MODE);
+		up->mdr1 = UART_OMAP_MDR1_13X_MODE;
 	else
-		serial_out(up, UART_OMAP_MDR1, UART_OMAP_MDR1_16X_MODE);
+		up->mdr1 = UART_OMAP_MDR1_16X_MODE;
+
+	serial_out(up, UART_OMAP_MDR1, up->mdr1)
 
 	/* Hardware Flow Control Configuration */
 
@@ -810,6 +860,7 @@  serial_omap_set_termios(struct uart_port *port, struct ktermios *termios,
 		serial_omap_configure_xonxoff(up, termios);
 
 	spin_unlock_irqrestore(&up->port.lock, flags);
+	serial_omap_port_disable(up);
 	dev_dbg(up->port.dev, "serial_omap_set_termios+%d\n", up->pdev->id);
 }
 
@@ -821,6 +872,8 @@  serial_omap_pm(struct uart_port *port, unsigned int state,
 	unsigned char efr;
 
 	dev_dbg(up->port.dev, "serial_omap_pm+%d\n", up->pdev->id);
+
+	serial_omap_port_enable(up);
 	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B);
 	efr = serial_in(up, UART_EFR);
 	serial_out(up, UART_EFR, efr | UART_EFR_ECB);
@@ -830,6 +883,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)
+		pm_runtime_put_sync(&up->pdev->dev);
+	else
+		serial_omap_port_disable(up);
 }
 
 static void serial_omap_release_port(struct uart_port *port)
@@ -907,25 +964,31 @@  static inline void wait_for_xmitr(struct uart_omap_port *up)
 static void serial_omap_poll_put_char(struct uart_port *port, unsigned char ch)
 {
 	struct uart_omap_port *up = (struct uart_omap_port *)port;
+
+	serial_omap_port_enable(up);
 	wait_for_xmitr(up);
 	serial_out(up, UART_TX, ch);
+	serial_omap_port_disable(up);
 }
 
 static int serial_omap_poll_get_char(struct uart_port *port)
 {
 	struct uart_omap_port *up = (struct uart_omap_port *)port;
-	unsigned int status = serial_in(up, UART_LSR);
+	unsigned int status;
 
+	serial_omap_port_enable(up);
+	status = serial_in(up, UART_LSR);
 	if (!(status & UART_LSR_DR))
 		return NO_POLL_CHAR;
 
-	return serial_in(up, UART_RX);
+	status = serial_in(up, UART_RX);
+	serial_omap_port_disable(up);
+	return status;
 }
 
 #endif /* CONFIG_CONSOLE_POLL */
 
 #ifdef CONFIG_SERIAL_OMAP_CONSOLE
-
 static struct uart_omap_port *serial_omap_console_ports[4];
 
 static struct uart_driver serial_omap_reg;
@@ -955,6 +1018,8 @@  serial_omap_console_write(struct console *co, const char *s,
 	else
 		spin_lock(&up->port.lock);
 
+	serial_omap_port_enable(up);
+
 	/*
 	 * First save the IER then disable the interrupts
 	 */
@@ -979,6 +1044,7 @@  serial_omap_console_write(struct console *co, const char *s,
 	if (up->msr_saved_flags)
 		check_modem_status(up);
 
+	serial_omap_port_disable(up);
 	if (locked)
 		spin_unlock(&up->port.lock);
 	local_irq_restore(flags);
@@ -1061,22 +1127,27 @@  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);
+		console_trylock();
+		serial_omap_pm(&up->port, 3, 0);
+	}
 	return 0;
 }
 
-static int serial_omap_resume(struct platform_device *dev)
+static int serial_omap_resume(struct device *dev)
 {
-	struct uart_omap_port *up = platform_get_drvdata(dev);
+	struct uart_omap_port *up = dev_get_drvdata(dev);
 
-	if (up)
+	if (up) {
 		uart_resume_port(&serial_omap_reg, &up->port);
+		console_unlock();
+	}
+
 	return 0;
 }
 
@@ -1097,6 +1168,7 @@  static void serial_omap_rx_timeout(unsigned long uart_no)
 			serial_omap_stop_rxdma(up);
 			up->ier |= (UART_IER_RDI | UART_IER_RLSI);
 			serial_out(up, UART_IER, up->ier);
+			serial_omap_port_disable(up);
 		}
 		return;
 	}
@@ -1128,6 +1200,9 @@  static void serial_omap_rx_timeout(unsigned long uart_no)
 
 static void uart_rx_dma_callback(int lch, u16 ch_status, void *data)
 {
+	struct uart_omap_port *up = (struct uart_omap_port *)data;
+
+	serial_omap_port_disable(up);
 	return;
 }
 
@@ -1135,6 +1210,7 @@  static int serial_omap_start_rxdma(struct uart_omap_port *up)
 {
 	int ret = 0;
 
+	serial_omap_port_enable(up);
 	if (up->uart_dma.rx_dma_channel == -1) {
 		ret = omap_request_dma(up->uart_dma.uart_dma_rx,
 				"UART Rx DMA",
@@ -1214,6 +1290,7 @@  static void uart_tx_dma_callback(int lch, u16 ch_status, void *data)
 		serial_omap_stop_tx(&up->port);
 		up->uart_dma.tx_dma_used = false;
 		spin_unlock(&(up->uart_dma.tx_lock));
+		serial_omap_port_disable(up);
 	} else {
 		omap_stop_dma(up->uart_dma.tx_dma_channel);
 		serial_omap_continue_tx(up);
@@ -1224,9 +1301,10 @@  static void uart_tx_dma_callback(int lch, u16 ch_status, void *data)
 
 static int serial_omap_probe(struct platform_device *pdev)
 {
-	struct uart_omap_port	*up;
+	struct uart_omap_port	*up = NULL;
 	struct resource		*mem, *irq, *dma_tx, *dma_rx;
 	struct omap_uart_port_info *omap_up_info = pdev->dev.platform_data;
+	struct omap_device *od;
 	int ret = -ENOSPC;
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -1276,12 +1354,20 @@  static int serial_omap_probe(struct platform_device *pdev)
 	up->port.ops = &serial_omap_pops;
 	up->port.line = pdev->id;
 
-	up->port.membase = omap_up_info->membase;
-	up->port.mapbase = omap_up_info->mapbase;
+	up->port.mapbase = mem->start;
+	up->port.membase = ioremap(mem->start, mem->end - mem->start);
+
+	if (!up->port.membase) {
+		dev_err(&pdev->dev, "can't ioremap UART\n");
+		ret = -ENOMEM;
+		goto err1;
+	}
+
 	up->port.flags = omap_up_info->flags;
-	up->port.irqflags = omap_up_info->irqflags;
 	up->port.uartclk = omap_up_info->uartclk;
 	up->uart_dma.uart_base = mem->start;
+	up->errata = omap_up_info->errata;
+	up->enable_wakeup = omap_up_info->enable_wakeup;
 
 	if (omap_up_info->dma_enabled) {
 		up->uart_dma.uart_dma_tx = dma_tx->start;
@@ -1295,18 +1381,36 @@  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_enable(&pdev->dev);
+	pm_runtime_irq_safe(&pdev->dev);
+
+	if (omap_up_info->console_uart) {
+		od = to_omap_device(up->pdev);
+		omap_hwmod_idle(od->hwmods[0]);
+		serial_omap_port_enable(up);
+		serial_omap_port_disable(up);
+	}
+
 	ui[pdev->id] = up;
 	serial_omap_add_console_port(up);
 
 	ret = uart_add_one_port(&serial_omap_reg, &up->port);
 	if (ret != 0)
-		goto do_release_region;
+		goto err1;
 
+	dev_set_drvdata(&pdev->dev, up);
 	platform_set_drvdata(pdev, up);
+
 	return 0;
 err:
 	dev_err(&pdev->dev, "[UART%d]: failure [%s]: %d\n",
 				pdev->id, __func__, ret);
+err1:
+	kfree(up);
 do_release_region:
 	release_mem_region(mem->start, (mem->end - mem->start) + 1);
 	return ret;
@@ -1318,20 +1422,76 @@  static int serial_omap_remove(struct platform_device *dev)
 
 	platform_set_drvdata(dev, NULL);
 	if (up) {
+		pm_runtime_disable(&up->pdev->dev);
 		uart_remove_one_port(&serial_omap_reg, &up->port);
 		kfree(up);
 	}
 	return 0;
 }
 
+static void omap_uart_restore_context(struct uart_omap_port *up)
+{
+	u16 efr = 0;
+
+	serial_out(up, UART_OMAP_MDR1, up->mdr1);
+	serial_out(up, UART_LCR, 0xBF); /* Config B mode */
+	efr = serial_in(up, UART_EFR);
+	serial_out(up, UART_EFR, UART_EFR_ECB);
+	serial_out(up, UART_LCR, 0x0); /* Operational mode */
+	serial_out(up, UART_IER, 0x0);
+	serial_out(up, UART_LCR, 0xBF); /* Config B mode */
+	serial_out(up, UART_DLL, up->dll);
+	serial_out(up, UART_DLM, up->dlh);
+	serial_out(up, UART_LCR, 0x0); /* Operational mode */
+	serial_out(up, UART_IER, up->ier);
+	serial_out(up, UART_FCR, up->fcr);
+	serial_out(up, UART_LCR, 0x80);
+	serial_out(up, UART_MCR, up->mcr);
+	serial_out(up, UART_LCR, 0xBF); /* Config B mode */
+	serial_out(up, UART_EFR, efr);
+	serial_out(up, UART_LCR, up->lcr);
+	/* UART 16x mode */
+	serial_out(up, UART_OMAP_MDR1, up->mdr1);
+}
+
+static int omap_serial_runtime_suspend(struct device *dev)
+{
+	struct uart_omap_port *up = dev_get_drvdata(dev);
+
+	if (!up)
+		goto done;
+
+	if (device_may_wakeup(dev))
+		up->enable_wakeup(up->pdev, true);
+	else
+		up->enable_wakeup(up->pdev, false);
+done:
+	return 0;
+}
+
+static int omap_serial_runtime_resume(struct device *dev)
+{
+	struct uart_omap_port *up = dev_get_drvdata(dev);
+
+	if (up)
+		omap_uart_restore_context(up);
+
+	return 0;
+}
+
+static const struct dev_pm_ops omap_serial_dev_pm_ops = {
+	.suspend = serial_omap_suspend,
+	.resume	= serial_omap_resume,
+	.runtime_suspend = omap_serial_runtime_suspend,
+	.runtime_resume = omap_serial_runtime_resume,
+};
+
 static struct platform_driver serial_omap_driver = {
 	.probe          = serial_omap_probe,
 	.remove         = serial_omap_remove,
-
-	.suspend	= serial_omap_suspend,
-	.resume		= serial_omap_resume,
 	.driver		= {
 		.name	= DRIVER_NAME,
+		.pm = &omap_serial_dev_pm_ops,
 	},
 };