On Sat, Jun 25, 2011 at 5:00 AM, Kevin Hilman <khilman@xxxxxx> wrote: > "Govindraj.R" <govindraj.raja@xxxxxx> 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 <alan@xxxxxxxxxxxxxxx> >> Signed-off-by: Govindraj.R <govindraj.raja@xxxxxx> > > 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 majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html