OMAP2+: Serial: Runtime adaptation + cleanup

Cleanup serial.c file in preparation to addition of runtime API's in omap-serial file. Remove all clock handling mechanism as this will be taken care with pm runtime API's in omap-serial.c file itself.

1.) Remove omap-device enable and disable. We can can use get_sync/put_sync API's 2.) Remove context save/restore can be done with runtime_resume callback for
get_sync call. No need to save context as all reg details available in uart_port structure can be used for restore, so add missing regs in uart port struct.
3.) Add func to identify console uart.

4.) Erratum handling informed as flag to driver and func to handle erratum
can be moved to omap-serial driver itself.

Acked-by Alan Cox <[email protected]>

Signed-off-by Govindraj.R <[email protected]>

---
 arch/arm/mach-omap2/serial.c                  |  742 ++-----------------------
 arch/arm/plat-omap/include/plat/omap-serial.h |   11 +-
 2 files changed, 53 insertions(+), 700 deletions(-)

arch/arm/mach-omap2/serial.c +2 -16

+	for (i = 0; i < OMAP_MAX_HSUART_PORTS; i++) {
+		oh = omap_uart_hwmod_lookup(i);
 		if (!oh)
-			break;
-
-		uart = kzalloc(sizeof(struct omap_uart_state), GFP_KERNEL);
-		if (WARN_ON(!uart))
-			return -ENODEV;
-
-		uart->oh = oh;
-		uart->num = i++;
-		list_add_tail(&uart->node, &uart_list);
-		num_uarts++;
-
-		/*
-		 * NOTE: omap_hwmod_setup*() has not yet been called,
-		 *       so no hwmod functions will work yet.
-		 */
-

Quoted 4 lines · Govindraj.R

Cleanup serial.c file in preparation to addition of runtime API's in omap-serial file. Remove all clock handling mechanism as this will be taken care with pm runtime API's in omap-serial.c file itself.

Quoted 6 lines

1.) Remove omap-device enable and disable. We can can use get_sync/put_sync API's 2.) Remove context save/restore can be done with runtime_resume callback for ? ? get_sync call. No need to save context as all reg details available in ? ? uart_port structure can be used for restore, so add missing regs in ? ? uart port struct.
3.) Add func to identify console uart.

I don't see that as part of this patch.

Quoted 3 lines

4.) Erratum handling informed as flag to driver and func to handle erratum ? ? can be moved to omap-serial driver itself.

OK, but I see two erratum flags removed, but only flag added back. Also, the erratum handling is completely removed here and not added to the driver.

In general, this way of moving things makes it very difficult to review. You remove a bunch of things in this patch (and the previous one) and imply that some of it is added back to the driver. ?However, that's really difficult to verify with patch review.

If code is being moved, it is customary to remove it and add it to the new place in the same patch.

Quoted 6 lines

Acked-by Alan Cox <[email protected]>

Signed-off-by Govindraj.R <[email protected]>

---
?arch/arm/mach-omap2/serial.c ? ? ? ? ? ? ? ? ?| ?742 ++----------------------- ?arch/arm/plat-omap/include/plat/omap-serial.h | ? 11 +-
?2 files changed, 53 insertions(+), 700 deletions(-)

[...]

arch/arm/mach-omap2/serial.c static void omap_uart_idle_init(struct omap_uart_state *uart) · line 121

Quoted 7 lines

arch/arm/mach-omap2/serial.c +1 -6

-static DEVICE_ATTR(sleep_timeout, 0644, sleep_timeout_show,
- ? ? ? ? ? ? sleep_timeout_store);
-#define DEV_CREATE_FILE(dev, attr) WARN_ON(device_create_file(dev, attr))
-#else
-static inline void omap_uart_idle_init(struct omap_uart_state *uart) {}
-static void omap_uart_block_sleep(struct omap_uart_state *uart)
+struct omap_hwmod *omap_uart_hwmod_lookup(int num)

function should be static

[...]

Quoted 28 lines

+ ? ? for (i = 0; i < OMAP_MAX_HSUART_PORTS; i++) {
+ ? ? ? ? ? ? oh = omap_uart_hwmod_lookup(i); ? ? ? ? ? ? ? if (!oh)
- ? ? ? ? ? ? ? ? ? ? break;

(quoted diff) +1 -22

-
- ? ? ? ? ? ? uart = kzalloc(sizeof(struct omap_uart_state), GFP_KERNEL);
- ? ? ? ? ? ? if (WARN_ON(!uart))
- ? ? ? ? ? ? ? ? ? ? return -ENODEV;
-
- ? ? ? ? ? ? uart->oh = oh;
- ? ? ? ? ? ? uart->num = i++;
- ? ? ? ? ? ? list_add_tail(&uart->node, &uart_list);
- ? ? ? ? ? ? num_uarts++;
-
- ? ? ? ? ? ? /*
- ? ? ? ? ? ? ?* NOTE: omap_hwmod_setup*() has not yet been called,
- ? ? ? ? ? ? ?* ? ? ? so no hwmod functions will work yet.
- ? ? ? ? ? ? ?*/
-
- ? ? ? ? ? ? /*
- ? ? ? ? ? ? ?* During UART early init, device need to be probed
- ? ? ? ? ? ? ?* to determine SoC specific init before omap_device
- ? ? ? ? ? ? ?* is ready. ?Therefore, don't allow idle here
- ? ? ? ? ? ? ?*/
- ? ? ? ? ? ? uart->oh->flags |= HWMOD_INIT_NO_IDLE | HWMOD_INIT_NO_RESET;
- ? ? } while (1);
+ ? ? ? ? ? ? ? ? ? ? continue;

Quoted 1 line

+ ? ? ? ? ? ? oh->flags |= HWMOD_INIT_NO_IDLE | HWMOD_INIT_NO_RESET;

With proper runtime PM in the driver, I did not expect these to be needed any longer by the end of the series, but I see they are still there. ?Are they really needed?

[...]

Quoted 8 lines

(quoted diff) +0 -0

@@ -864,15 +213,14 @@ void __init omap_serial_init_port(struct omap_board_data *bdata)

? */
?void __init omap_serial_init(void)
?{
- ? ? struct omap_uart_state *uart;
? ? ? struct omap_board_data bdata;
+ ? ? u8 i;

Quoted 3 lines

- ? ? list_for_each_entry(uart, &uart_list, node) {
- ? ? ? ? ? ? bdata.id = uart->num;
+ ? ? for (i = 0; i < OMAP_MAX_HSUART_PORTS; i++) {

This should probably use the hwmod class iterator.

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

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

1.) Moving context_restore func to driver from serial.c 2.) Use runtime irq safe to use get_sync from irq context. 3.) autosuspend for port specific activities and put for reg access.

4.) for earlyprintk usage the debug macro will write to console uart
so keep uart clocks active from boot, idle using hwmod API's re-enable back using runtime API's.

5.) Moving context restore to runtime suspend and using reg values from port
structure to restore the uart port context based on context_loss_count. Maintain internal state machine for avoiding repeated enable/disable of uart port wakeup mechanism.
6.) Add API to enable wakeup and check wakeup status.

Acked-by Alan Cox <[email protected]>

Signed-off-by Govindraj.R <[email protected]>

---
 arch/arm/mach-omap2/serial.c                  |   49 ++++++
 arch/arm/plat-omap/include/plat/omap-serial.h |   10 ++
 drivers/tty/serial/omap-serial.c              |  211 ++++++++++++++++++++++---
 3 files changed, 250 insertions(+), 20 deletions(-)

arch/arm/mach-omap2/serial.c +36 -0

index 2e10357..7117220 100644
@@ -30,6 +30,7 @@
 #include <plat/common.h>
 #include <plat/board.h>
 #include <plat/omap_device.h>
+#include <plat/omap-pm.h>
 
 #include "prm2xxx_3xxx.h"
 #include "pm.h"

@@ -246,6 +247,51 @@ static void omap_uart_idle_init(struct omap_uart_port_info *uart,
 	}
 }
 
+static bool omap_uart_chk_wakeup(struct platform_device *pdev)
+{
+	struct omap_uart_port_info *up = pdev->dev.platform_data;
+	struct omap_device *od;
+	u32 wkst = 0;
+	bool ret = false;
+
+	if (up->wk_st && up->wk_en && up->wk_mask) {
+		/* Check for normal UART wakeup (and clear it) */
+		wkst = __raw_readl(up->wk_st) & up->wk_mask;
+		if (wkst) {
+			__raw_writel(up->wk_mask, up->wk_st);
+			ret = true;
+		}
+	}
+
+	od = to_omap_device(pdev);
+	if (omap_hwmod_pad_get_wakeup_status(od->hwmods[0]) == true)
+		ret = true;
+
+	return ret;
+}
+
+static void omap_uart_wakeup_enable(struct platform_device *pdev, bool enable)
+{
+	struct omap_device *od;
+	struct omap_uart_port_info *up = pdev->dev.platform_data;
+
+	/* Set or clear wake-enable bit */
+	if (up->wk_en && up->wk_mask) {
+		u32 v = __raw_readl(up->wk_en);
+		if (enable)
+			v |= up->wk_mask;
+		else
+			v &= ~up->wk_mask;

arch/arm/mach-omap2/serial.c +3 -0

 struct omap_hwmod *omap_uart_hwmod_lookup(int num)
 {
 	struct omap_hwmod *oh;

@@ -317,6 +363,9 @@ 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;
+	pdata->chk_wakeup = omap_uart_chk_wakeup;
+	pdata->get_context_loss_count = omap_pm_get_dev_context_loss_count;
 
 	pdev = omap_device_build(name, bdata->id, oh, pdata,
 				sizeof(*pdata),	omap_uart_latency,

arch/arm/plat-omap/include/plat/omap-serial.h +3 -0

index 06e3aa2..7fc63b8 100644
@@ -67,6 +67,9 @@ struct omap_uart_port_info {
 	void __iomem *wk_st;
 	void __iomem *wk_en;
 	u32 wk_mask;
+	void (*enable_wakeup)(struct platform_device *, bool);
+	bool (*chk_wakeup)(struct platform_device *);
+	u32 (*get_context_loss_count)(struct device *);
 };
 
 struct uart_omap_dma {

arch/arm/plat-omap/include/plat/omap-serial.h +0 -0

 };
 
 #endif /* __OMAP_SERIAL_H__ */

drivers/tty/serial/omap-serial.c +0 -0

index 6ac0ade..30bdd05 100644
@@ -37,10 +37,14 @@
 #include <linux/clk.h>

drivers/tty/serial/omap-serial.c +56 -5

 static struct uart_omap_port *ui[OMAP_MAX_HSUART_PORTS];
 

@@ -102,6 +106,8 @@ static void serial_omap_stop_rxdma(struct uart_omap_port *up)
 		omap_free_dma(up->uart_dma.rx_dma_channel);
 		up->uart_dma.rx_dma_channel = OMAP_UART_DMA_CH_FREE;
 		up->uart_dma.rx_dma_used = false;
+		pm_runtime_mark_last_busy(&up->pdev->dev);
+		pm_runtime_put_autosuspend(&up->pdev->dev);
 	}
 }
 

@@ -110,8 +116,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);
+
+	pm_runtime_get_sync(&up->pdev->dev);
 	up->ier |= UART_IER_MSI;
 	serial_out(up, UART_IER, up->ier);
+	pm_runtime_put(&up->pdev->dev);
 }
 
 static void serial_omap_stop_tx(struct uart_port *port)

@@ -129,23 +138,32 @@ static void serial_omap_stop_tx(struct uart_port *port)
 		omap_stop_dma(up->uart_dma.tx_dma_channel);
 		omap_free_dma(up->uart_dma.tx_dma_channel);
 		up->uart_dma.tx_dma_channel = OMAP_UART_DMA_CH_FREE;
+		pm_runtime_mark_last_busy(&up->pdev->dev);
+		pm_runtime_put_autosuspend(&up->pdev->dev);
 	}
 
+	pm_runtime_get_sync(&up->pdev->dev);
 	if (up->ier & UART_IER_THRI) {
 		up->ier &= ~UART_IER_THRI;
 		serial_out(up, UART_IER, up->ier);
 	}
+
+	pm_runtime_mark_last_busy(&up->pdev->dev);
+	pm_runtime_put_autosuspend(&up->pdev->dev);
 }
 
 static void serial_omap_stop_rx(struct uart_port *port)
 {
 	struct uart_omap_port *up = (struct uart_omap_port *)port;
 
+	pm_runtime_get_sync(&up->pdev->dev);
 	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);
+	pm_runtime_mark_last_busy(&up->pdev->dev);
+	pm_runtime_put_autosuspend(&up->pdev->dev);
 }
 
 static inline void receive_chars(struct uart_omap_port *up, int *status)

@@ -262,7 +280,10 @@ static void serial_omap_start_tx(struct uart_port *port)
 	int ret = 0;
 
 	if (!up->use_dma) {
+		pm_runtime_get_sync(&up->pdev->dev);
 		serial_omap_enable_ier_thri(up);
+		pm_runtime_mark_last_busy(&up->pdev->dev);
+		pm_runtime_put_autosuspend(&up->pdev->dev);
 		return;
 	}
 

@@ -272,6 +293,7 @@ static void serial_omap_start_tx(struct uart_port *port)
 	xmit = &up->port.state->xmit;
 
 	if (up->uart_dma.tx_dma_channel == OMAP_UART_DMA_CH_FREE) {
+		pm_runtime_get_sync(&up->pdev->dev);
 		ret = omap_request_dma(up->uart_dma.uart_dma_tx,
 				"UART Tx DMA",
 				(void *)uart_tx_dma_callback, up,

@@ -354,9 +376,12 @@ static inline irqreturn_t serial_omap_irq(int irq, void *dev_id)
 	unsigned int iir, lsr;
 	unsigned long flags;
 
+	pm_runtime_get_sync(&up->pdev->dev);
 	iir = serial_in(up, UART_IIR);
-	if (iir & UART_IIR_NO_INT)
+	if (iir & UART_IIR_NO_INT) {
+		pm_runtime_put(&up->pdev->dev);
 		return IRQ_NONE;
+	}
 
 	spin_lock_irqsave(&up->port.lock, flags);
 	lsr = serial_in(up, UART_LSR);

@@ -378,6 +403,9 @@ static inline irqreturn_t serial_omap_irq(int irq, void *dev_id)
 		transmit_chars(up);
 
 	spin_unlock_irqrestore(&up->port.lock, flags);
+	pm_runtime_mark_last_busy(&up->pdev->dev);
+	pm_runtime_put_autosuspend(&up->pdev->dev);
+
 	up->port_activity = jiffies;
 	return IRQ_HANDLED;
 }

@@ -388,11 +416,12 @@ static unsigned int serial_omap_tx_empty(struct uart_port *port)
 	unsigned long flags = 0;
 	unsigned int ret = 0;
 
+	pm_runtime_get_sync(&up->pdev->dev);
 	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);
-
+	pm_runtime_put(&up->pdev->dev);
 	return ret;
 }
 

@@ -402,7 +431,10 @@ static unsigned int serial_omap_get_mctrl(struct uart_port *port)
 	unsigned char status;
 	unsigned int ret = 0;
 
+	pm_runtime_get_sync(&up->pdev->dev);
 	status = check_modem_status(up);
+	pm_runtime_put(&up->pdev->dev);
+
 	dev_dbg(up->port.dev, "serial_omap_get_mctrl+%d\n", up->pdev->id);
 
 	if (status & UART_MSR_DCD)

@@ -433,9 +465,11 @@ static void serial_omap_set_mctrl(struct uart_port *port, unsigned int mctrl)
 	if (mctrl & TIOCM_LOOP)
 		mcr |= UART_MCR_LOOP;
 
+	pm_runtime_get_sync(&up->pdev->dev);
 	up->mcr = serial_in(up, UART_MCR);
 	up->mcr |= mcr;
 	serial_out(up, UART_MCR, up->mcr);
+	pm_runtime_put(&up->pdev->dev);
 }
 
 static void serial_omap_break_ctl(struct uart_port *port, int break_state)

@@ -444,6 +478,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);
+	pm_runtime_get_sync(&up->pdev->dev);
 	spin_lock_irqsave(&up->port.lock, flags);
 	if (break_state == -1)
 		up->lcr |= UART_LCR_SBC;

@@ -451,6 +486,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);
+	pm_runtime_put(&up->pdev->dev);
 }
 
 static int serial_omap_startup(struct uart_port *port)

@@ -469,6 +505,7 @@ static int serial_omap_startup(struct uart_port *port)
 
 	dev_dbg(up->port.dev, "serial_omap_startup+%d\n", up->pdev->id);
 
+	pm_runtime_get_sync(&up->pdev->dev);
 	/*
 	 * Clear the FIFO buffers and disable them.
 	 * (they will be reenabled in set_termios())

@@ -524,6 +561,8 @@ 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);
 
+	pm_runtime_mark_last_busy(&up->pdev->dev);
+	pm_runtime_put_autosuspend(&up->pdev->dev);
 	up->port_activity = jiffies;
 	return 0;
 }

@@ -534,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);
+
+	pm_runtime_get_sync(&up->pdev->dev);
 	/*
 	 * Disable interrupts from this port
 	 */

@@ -567,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;
 	}
+	pm_runtime_put(&up->pdev->dev);
 	free_irq(up->port.irq, up);
 }
 

@@ -682,6 +724,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.
 	 */
+	pm_runtime_get_sync(&up->pdev->dev);
 	spin_lock_irqsave(&up->port.lock, flags);
 
 	/*

@@ -814,6 +857,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);
+	pm_runtime_put(&up->pdev->dev);
 	dev_dbg(up->port.dev, "serial_omap_set_termios+%d\n", up->pdev->id);
 }
 

@@ -825,6 +869,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);
+
+	pm_runtime_get_sync(&up->pdev->dev);
 	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);

@@ -834,6 +880,7 @@ 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);
+	pm_runtime_put(&up->pdev->dev);
 }
 
 static void serial_omap_release_port(struct uart_port *port)

@@ -911,25 +958,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;
+
+	pm_runtime_get_sync(&up->pdev->dev);
 	wait_for_xmitr(up);
 	serial_out(up, UART_TX, ch);
+	pm_runtime_put(&up->pdev->dev);
 }
 
 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;
 
+	pm_runtime_get_sync(&up->pdev->dev);
+	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);
+	pm_runtime_put(&up->pdev->dev);
+	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;

@@ -951,6 +1004,8 @@ serial_omap_console_write(struct console *co, const char *s,
 	unsigned int ier;
 	int locked = 1;
 
+	pm_runtime_get_sync(&up->pdev->dev);
+
 	local_irq_save(flags);
 	if (up->port.sysrq)
 		locked = 0;

drivers/tty/serial/omap-serial.c +1 -3

 }
 
 static int __init

@@ -1065,22 +1123,33 @@ static struct uart_driver serial_omap_reg = {
 	.cons		= OMAP_CONSOLE,
 };
 
-static int
-serial_omap_suspend(struct platform_device *pdev, pm_message_t state)
+static int serial_omap_suspend(struct device *dev)
 {
-	struct uart_omap_port *up = platform_get_drvdata(pdev);

drivers/tty/serial/omap-serial.c +13 -4

 	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);
+		if (up->port.line == up->port.cons->index &&
+					up->console_locked) {
+			console_unlock();
+			up->console_locked = 0;
+		}
+	}
+
 	return 0;
 }
 

@@ -1140,6 +1209,7 @@ static int serial_omap_start_rxdma(struct uart_omap_port *up)
 	int ret = 0;
 
 	if (up->uart_dma.rx_dma_channel == -1) {
+		pm_runtime_get_sync(&up->pdev->dev);
 		ret = omap_request_dma(up->uart_dma.uart_dma_rx,
 				"UART Rx DMA",
 				(void *)uart_rx_dma_callback, up,

@@ -1228,9 +1298,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);

drivers/tty/serial/omap-serial.c +8 -1

+	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->chk_wakeup = omap_up_info->chk_wakeup;
 
 	if (omap_up_info->dma_enabled) {
 		up->uart_dma.uart_dma_tx = dma_tx->start;

@@ -1299,18 +1378,34 @@ static int serial_omap_probe(struct platform_device *pdev)
 		up->uart_dma.rx_dma_channel = OMAP_UART_DMA_CH_FREE;
 	}

drivers/tty/serial/omap-serial.c +31 -1

+		pm_runtime_get_sync(&up->pdev->dev);
+		up->clocked = 1;
+	}
+
 	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;
 
 	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, resource_size(mem));
 	return ret;

@@ -1322,20 +1417,96 @@ 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)
+{
+	serial_out(up, UART_OMAP_MDR1, up->mdr1);
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B); /* Config B mode */
+	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, UART_LCR_CONF_MODE_B); /* 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, UART_LCR_CONF_MODE_A);
+	serial_out(up, UART_MCR, up->mcr);
+	serial_out(up, UART_LCR, UART_LCR_CONF_MODE_B); /* Config B mode */
+	serial_out(up, UART_EFR, up->efr);
+	serial_out(up, UART_LCR, up->lcr);
+	/* UART 16x mode */
+	serial_out(up, UART_OMAP_MDR1, up->mdr1);
+}
+

drivers/tty/serial/omap-serial.c +14 -0

+	return 0;
+}
+
+static int omap_serial_runtime_resume(struct device *dev)
+{
+	struct uart_omap_port *up = dev_get_drvdata(dev);
+	struct omap_uart_port_info *pdata = dev->platform_data;
+
+	if (up) {
+		if (pdata->get_context_loss_count) {
+			u32 loss_cnt = pdata->get_context_loss_count(dev);
+
+			if (up->context_loss_cnt != loss_cnt)
+				omap_uart_restore_context(up);

Quoted 2 lines · Govindraj.R

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

Quoted 3 lines

1.) Moving context_restore func to driver from serial.c 2.) Use runtime irq safe to use get_sync from irq context. 3.) autosuspend for port specific activities and put for reg access.

Re: autosuspend, it looks like the driver now has 2 mechanisms for tracking activity. ?The runtime PM 'mark last busy' and the existing 'up->port_activity = jiffies' stuff. ?Do you think those could be unified?

At first glance, it looks like most places with a _mark_last_busy() also have a up->port_activity update. ?I'm not familiar enough with the driver to make the call, but they look awfully similar.

Quoted 3 lines

4.) for earlyprintk usage the debug macro will write to console uart ? ? so keep uart clocks active from boot, idle using hwmod API's re-enable back ? ? using runtime API's.

/me doesn't understand

Quoted 15 lines

5.) Moving context restore to runtime suspend and using reg values from port ? ? structure to restore the uart port context based on context_loss_count. ? ? Maintain internal state machine for avoiding repeated enable/disable of ? ? uart port wakeup mechanism.
6.) Add API to enable wakeup and check wakeup status.

Acked-by Alan Cox <[email protected]>

Signed-off-by Govindraj.R <[email protected]>

---

?arch/arm/mach-omap2/serial.c ? ? ? ? ? ? ? ? ?| ? 49 ++++++
?arch/arm/plat-omap/include/plat/omap-serial.h | ? 10 ++
?drivers/tty/serial/omap-serial.c ? ? ? ? ? ? ?| ?211 ++++++++++++++++++++++---

?3 files changed, 250 insertions(+), 20 deletions(-)

Quoted 19 lines

(quoted diff) +19 -0

+
+static void omap_uart_wakeup_enable(struct platform_device *pdev, bool enable)
+{
+ ? ? struct omap_device *od;
+ ? ? struct omap_uart_port_info *up = pdev->dev.platform_data;
+
+ ? ? /* Set or clear wake-enable bit */
+ ? ? if (up->wk_en && up->wk_mask) {
+ ? ? ? ? ? ? u32 v = __raw_readl(up->wk_en);
+ ? ? ? ? ? ? if (enable)
+ ? ? ? ? ? ? ? ? ? ? v |= up->wk_mask;
+ ? ? ? ? ? ? else
+ ? ? ? ? ? ? ? ? ? ? v &= ~up->wk_mask;
+ ? ? ? ? ? ? __raw_writel(v, up->wk_en);
+ ? ? }
+
+ ? ? od = to_omap_device(pdev);
+ ? ? if (enable)
+ ? ? ? ? ? ? omap_hwmod_enable_wakeup(od->hwmods[0]);

Review

Quoted 4 lines

+ ? ? else
+ ? ? ? ? ? ? omap_hwmod_disable_wakeup(od->hwmods[0]);

(quoted diff) +2 -0

+}
+

Here again, this is difficult to review because you removed the code in [4/11] and add it back here, but with rather different functionality with no description as to why.

The previous version enabled wakeups at the PRCM and at the IO ring. This version enables wakeups at the PRCM as well but instead of enabling them at the IO ring, enables them at the module.

Quoted 26 lines

?struct omap_hwmod *omap_uart_hwmod_lookup(int num) ?{
? ? ? struct omap_hwmod *oh;

(quoted diff) +3 -0

@@ -317,6 +363,9 @@ 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;
+ ? ? pdata->chk_wakeup = omap_uart_chk_wakeup;
+ ? ? pdata->get_context_loss_count = omap_pm_get_dev_context_loss_count;
 
 ? ? ? pdev = omap_device_build(name, bdata->id, oh, pdata,
 ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? sizeof(*pdata), omap_uart_latency,

arch/arm/plat-omap/include/plat/omap-serial.h +3 -0

index 06e3aa2..7fc63b8 100644
@@ -67,6 +67,9 @@ struct omap_uart_port_info {
 ? ? ? void __iomem *wk_st;
 ? ? ? void __iomem *wk_en;
 ? ? ? u32 wk_mask;
+ ? ? void (*enable_wakeup)(struct platform_device *, bool);
+ ? ? bool (*chk_wakeup)(struct platform_device *);
+ ? ? u32 (*get_context_loss_count)(struct device *);
 ?};
 

arch/arm/plat-omap/include/plat/omap-serial.h struct uart_omap_port { · line 128

Quoted 10 lines

arch/arm/plat-omap/include/plat/omap-serial.h +4 -0

 ?struct uart_omap_dma {

@@ -118,7 +121,14 @@ struct uart_omap_port {
 ? ? ? unsigned char ? ? ? ? ? msr_saved_flags;
 ? ? ? char ? ? ? ? ? ? ? ? ? ?name[20];
 ? ? ? unsigned long ? ? ? ? ? port_activity;
+ ? ? u32 ? ? ? ? ? ? ? ? ? ? context_loss_cnt;
+ ? ? u8 ? ? ? ? ? ? ? ? ? ? ?wake_status;
+
 ? ? ? unsigned int ? ? ? ? ? ?errata;
+ ? ? unsigned int ? ? ? ? ? ?clocked;

This is a legacy from serial.c and should not be needed. ?Checking pm_runtime_suspended() will provide the same info.

arch/arm/plat-omap/include/plat/omap-serial.h struct uart_omap_port { · line 131

Quoted 3 lines

arch/arm/plat-omap/include/plat/omap-serial.h +3 -0

+ ? ? u8 ? ? ? ? ? ? ? ? ? ? ?console_locked;
+
+ ? ? bool (*chk_wakeup)(struct platform_device *);

s/chk/check/ please. ?It's not that much longer.

Quoted 13 lines

arch/arm/plat-omap/include/plat/omap-serial.h +0 -0

 ?};
 

?#endif /* __OMAP_SERIAL_H__ */

drivers/tty/serial/omap-serial.c +0 -0

index 6ac0ade..30bdd05 100644
@@ -37,10 +37,14 @@

?#include <linux/clk.h>
?#include <linux/serial_core.h>
?#include <linux/irq.h>
+#include <linux/pm_runtime.h>

drivers/tty/serial/omap-serial.c

Quoted 6 lines

?#include <plat/dma.h>
?#include <plat/dmtimer.h>
?#include <plat/omap-serial.h>

drivers/tty/serial/omap-serial.c +3 -0

+#include <plat/omap_device.h>
+
+#define OMAP_UART_AUTOSUSPEND_DELAY 3000 ? ? /* Value is msecs */

Because of the character lost problem when UARTs are idled, Tony prefers that the default on this be no auto timeout, like the current mainline code does.

Quoted 12 lines

?static struct uart_omap_port *ui[OMAP_MAX_HSUART_PORTS];

drivers/tty/serial/omap-serial.c +2 -0

@@ -102,6 +106,8 @@ static void serial_omap_stop_rxdma(struct uart_omap_port *up)
 ? ? ? ? ? ? ? omap_free_dma(up->uart_dma.rx_dma_channel);
 ? ? ? ? ? ? ? up->uart_dma.rx_dma_channel = OMAP_UART_DMA_CH_FREE;
 ? ? ? ? ? ? ? up->uart_dma.rx_dma_used = false;
+ ? ? ? ? ? ? pm_runtime_mark_last_busy(&up->pdev->dev);
+ ? ? ? ? ? ? pm_runtime_put_autosuspend(&up->pdev->dev);
 ? ? ? }
 ?}
 

@@ -110,8 +116,11 @@ static void serial_omap_enable_ms(struct uart_port *port)

Quoted 21 lines

?#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;

drivers/tty/serial/omap-serial.c +2 -0

@@ -951,6 +1004,8 @@ serial_omap_console_write(struct console *co, const char *s,
 ? ? ? unsigned int ier;
 ? ? ? int locked = 1;
 
+ ? ? pm_runtime_get_sync(&up->pdev->dev);
+
 ? ? ? local_irq_save(flags);
 ? ? ? if (up->port.sysrq)
 ? ? ? ? ? ? ? locked = 0;

@@ -983,9 +1038,12 @@ serial_omap_console_write(struct console *co, const char *s,
 ? ? ? if (up->msr_saved_flags)
 ? ? ? ? ? ? ? check_modem_status(up);
 

drivers/tty/serial/omap-serial.c serial_omap_console_write(struct console *co, const char *s, · line 1046

Quoted 6 lines

drivers/tty/serial/omap-serial.c +3 -0

+ ? ? pm_runtime_mark_last_busy(&up->pdev->dev);
+ ? ? pm_runtime_put_autosuspend(&up->pdev->dev);
 ? ? ? if (locked)
 ? ? ? ? ? ? ? spin_unlock(&up->port.lock);
 ? ? ? local_irq_restore(flags);
+ ? ? up->port_activity = jiffies;

hmm, why? ?this change looks suspicious.

Quoted 12 lines

drivers/tty/serial/omap-serial.c +0 -0

 ?}
 

?static int __init

drivers/tty/serial/omap-serial.c +1 -3

@@ -1065,22 +1123,33 @@ static struct uart_driver serial_omap_reg = {
 ? ? ? .cons ? ? ? ? ? = OMAP_CONSOLE,
 ?};
 
-static int
-serial_omap_suspend(struct platform_device *pdev, pm_message_t state)
+static int serial_omap_suspend(struct device *dev)
 ?{
- ? ? struct uart_omap_port *up = platform_get_drvdata(pdev);

drivers/tty/serial/omap-serial.c static struct uart_driver serial_omap_reg = { · line 1136

Quoted 10 lines

drivers/tty/serial/omap-serial.c +7 -1

+ ? ? struct uart_omap_port *up = dev_get_drvdata(dev);
 
- ? ? if (up)
+ ? ? if (up) {
 ? ? ? ? ? ? ? uart_suspend_port(&serial_omap_reg, &up->port);
+ ? ? ? ? ? ? if (up->port.line == up->port.cons->index &&
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? !is_console_locked())
+ ? ? ? ? ? ? ? ? ? ? up->console_locked = console_trylock();
+ ? ? }
+

Motiviation for console locking in this version of the series is not clear, and not described in the changelog.

It's also present here in static suspend/resume but not in runtime suspend/resume, which also is suspicious.

Quoted 11 lines

drivers/tty/serial/omap-serial.c +3 -3

 ? ? ? 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) {

Quoted 2 lines

+ ? ? up->port.mapbase = mem->start;
+ ? ? up->port.membase = ioremap(mem->start, mem->end - mem->start);

The size is (end - start + 1), but please use the resource_size() helper for this to avoid this common problem.

Quoted 18 lines

+ ? ? 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->chk_wakeup = omap_up_info->chk_wakeup;

? ? ? if (omap_up_info->dma_enabled) {
? ? ? ? ? ? ? up->uart_dma.uart_dma_tx = dma_tx->start;

drivers/tty/serial/omap-serial.c +0 -0

@@ -1299,18 +1378,34 @@ static int serial_omap_probe(struct platform_device *pdev)
 ? ? ? ? ? ? ? up->uart_dma.rx_dma_channel = OMAP_UART_DMA_CH_FREE;
 ? ? ? }

drivers/tty/serial/omap-serial.c static int serial_omap_probe(struct platform_device *pdev) · line 1389

Quoted 10 lines

drivers/tty/serial/omap-serial.c +9 -0

 
+ ? ? pm_runtime_use_autosuspend(&pdev->dev);
+ ? ? pm_runtime_set_autosuspend_delay(&pdev->dev,
+ ? ? ? ? ? ? ? ? ? ? OMAP_UART_AUTOSUSPEND_DELAY);
+
+ ? ? pm_runtime_irq_safe(&pdev->dev);
+ ? ? if (device_may_wakeup(&pdev->dev)) {
+ ? ? ? ? ? ? pm_runtime_enable(&pdev->dev);
+ ? ? ? ? ? ? od = to_omap_device(up->pdev);
+ ? ? ? ? ? ? omap_hwmod_idle(od->hwmods[0]);

No hwmod calls in drivers please.

In this case, this is a legacy of using HWMOD_INIT_NO_IDLE and _NO_RESET. ?That should be removed so this could be removed.

Quoted 7 lines

drivers/tty/serial/omap-serial.c +4 -0

+ ? ? ? ? ? ? pm_runtime_get_sync(&up->pdev->dev);
+ ? ? ? ? ? ? up->clocked = 1;
+ ? ? }
+
 ? ? ? ui[pdev->id] = up;
 ? ? ? serial_omap_add_console_port(up);
 

Quoted 2 lines

+ ? ? if (pdata->get_context_loss_count)
+ ? ? ? ? ? ? up->context_loss_cnt = pdata->get_context_loss_count(dev);

You need

? ? ? ?if (!pdata->enable_wakeup)
? ? ? ? ? ? ? ?return 0;

here in case there is no ->enable_wakeup provided. ?Otherwise...

Quoted 3 lines

+ ? ? if (device_may_wakeup(dev)) {
+ ? ? ? ? ? ? if (!up->wake_status) {
+ ? ? ? ? ? ? ? ? ? ? pdata->enable_wakeup(up->pdev, true);

...boom.

Quoted 8 lines

+ ? ? ? ? ? ? ? ? ? ? up->wake_status = true;
+ ? ? ? ? ? ? }
+ ? ? } else {
+ ? ? ? ? ? ? if (up->wake_status) {
+ ? ? ? ? ? ? ? ? ? ? pdata->enable_wakeup(up->pdev, false);
+ ? ? ? ? ? ? ? ? ? ? up->wake_status = false;
+ ? ? ? ? ? ? }
+ ? ? }

up->wake_status would be better named ->wakeups_enabled;

Quoted 15 lines

+ ? ? return 0;

drivers/tty/serial/omap-serial.c +13 -0

+}
+
+static int omap_serial_runtime_resume(struct device *dev)
+{
+ ? ? struct uart_omap_port *up = dev_get_drvdata(dev);
+ ? ? struct omap_uart_port_info *pdata = dev->platform_data;
+
+ ? ? if (up) {
+ ? ? ? ? ? ? if (pdata->get_context_loss_count) {
+ ? ? ? ? ? ? ? ? ? ? u32 loss_cnt = pdata->get_context_loss_count(dev);
+
+ ? ? ? ? ? ? ? ? ? ? if (up->context_loss_cnt != loss_cnt)
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? omap_uart_restore_context(up);

drivers/tty/serial/omap-serial.c

Quoted 10 lines

drivers/tty/serial/omap-serial.c +10 -0

+ ? ? ? ? ? ? }
+ ? ? }
+
+ ? ? return 0;
+}
+
+#ifdef CONFIG_PM_RUNTIME
+static const struct dev_pm_ops omap_serial_dev_pm_ops = {
+ ? ? .suspend = serial_omap_suspend,
+ ? ? .resume = serial_omap_resume,

Static suspend operations should exists even when runtime PM is disabled.

Quoted 2 lines

+ ? ? .runtime_suspend = omap_serial_runtime_suspend,
+ ? ? .runtime_resume = omap_serial_runtime_resume,

Note that some functions have serial_omap_ prefix and others have omap_serial_. ?It looks like serial_omap_ is used througout the driver. Please unify.

drivers/tty/serial/omap-serial.c static int serial_omap_remove(struct platform_device *dev) · line 1502

Quoted 5 lines

drivers/tty/serial/omap-serial.c +5 -0

+};
+#define SERIAL_PM_OPS (&omap_serial_dev_pm_ops)
+#else
+#define SERIAL_PM_OPS NULL
+#endif

Instead of this ifdef construct, please use SET_SYSTEM_SLEEP_PM_OPS() and SET_RUNTIME_PM_OPS() which take care of the right ifdefs for you, and also ensure that callbacks are available for suspend-to-disk (hibernate):

static const struct dev_pm_ops omap_serial_dev_pm_ops = {
? ? ? SET_SYSTEM_SLEEP_PM_OPS(serial_omap_suspend, serial_omap_resume), ? ? ? SET_RUNTIME_PM_OPS(runtime_suspend, runtime_resume, NULL), };

Quoted 11 lines

?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 = SERIAL_PM_OPS,
? ? ? },
?};

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

Quoted 8 lines · Kevin Hilman

Quoted 5 lines · Govindraj.R

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

1.) Moving context_restore func to driver from serial.c 2.) Use runtime irq safe to use get_sync from irq context. 3.) autosuspend for port specific activities and put for reg access.

Quoted 5 lines

Re: autosuspend, it looks like the driver now has 2 mechanisms for tracking activity. ?The runtime PM 'mark last busy' and the existing 'up->port_activity = jiffies' stuff. ?Do you think those could be unified?

Is there a way where we can retrieve the last busy jiffie value using runtime API? I don't find that available.

Quoted 4 lines

At first glance, it looks like most places with a _mark_last_busy() also have a up->port_activity update. ?I'm not familiar enough with the driver to make the call, but they look awfully similar.

Ok, I will check whether I can get rid if it.

Quoted 5 lines

Quoted 3 lines

4.) for earlyprintk usage the debug macro will write to console uart ? ? so keep uart clocks active from boot, idle using hwmod API's re-enable back ? ? using runtime API's.

/me doesn't understand

I have added this reason in early mail reply to 04/11 patch review [needed for earlyprintk option from low level debug ]

Quoted 15 lines

Quoted 15 lines

5.) Moving context restore to runtime suspend and using reg values from port ? ? structure to restore the uart port context based on context_loss_count. ? ? Maintain internal state machine for avoiding repeated enable/disable of ? ? uart port wakeup mechanism.
6.) Add API to enable wakeup and check wakeup status.

Acked-by Alan Cox <[email protected]>

Signed-off-by Govindraj.R <[email protected]>

---

?arch/arm/mach-omap2/serial.c ? ? ? ? ? ? ? ? ?| ? 49 ++++++
?arch/arm/plat-omap/include/plat/omap-serial.h | ? 10 ++
?drivers/tty/serial/omap-serial.c ? ? ? ? ? ? ?| ?211 ++++++++++++++++++++++---

?3 files changed, 250 insertions(+), 20 deletions(-)

[..]

Quoted 19 lines

Quoted 19 lines

(quoted diff) +19 -0

+
+static void omap_uart_wakeup_enable(struct platform_device *pdev, bool enable)
+{
+ ? ? struct omap_device *od;
+ ? ? struct omap_uart_port_info *up = pdev->dev.platform_data;
+
+ ? ? /* Set or clear wake-enable bit */
+ ? ? if (up->wk_en && up->wk_mask) {
+ ? ? ? ? ? ? u32 v = __raw_readl(up->wk_en);
+ ? ? ? ? ? ? if (enable)
+ ? ? ? ? ? ? ? ? ? ? v |= up->wk_mask;
+ ? ? ? ? ? ? else
+ ? ? ? ? ? ? ? ? ? ? v &= ~up->wk_mask;
+ ? ? ? ? ? ? __raw_writel(v, up->wk_en);
+ ? ? }
+
+ ? ? od = to_omap_device(pdev);
+ ? ? if (enable)
+ ? ? ? ? ? ? omap_hwmod_enable_wakeup(od->hwmods[0]);

Quoted 9 lines

Review

Quoted 4 lines

+ ? ? else
+ ? ? ? ? ? ? omap_hwmod_disable_wakeup(od->hwmods[0]);

(quoted diff) +2 -0

+}
+

Here again, this is difficult to review because you removed the code in [4/11] and add it back here, but with rather different functionality with no description as to why.

will move this change as part of 4/11 itself.

Quoted 4 lines

The previous version enabled wakeups at the PRCM and at the IO ring. This version enables wakeups at the PRCM as well but instead of enabling them at the IO ring, enables them at the module.

Since we are gating using prepare idle call I think
we can use this enable wake-up call as part of prepare idle call itself, as done earlier.

Quoted 26 lines

Quoted 12 lines

?}

?static int __init

(quoted diff) +1 -3

@@ -1065,22 +1123,33 @@ static struct uart_driver serial_omap_reg = {
 ? ? ? .cons ? ? ? ? ? = OMAP_CONSOLE,
 ?};
 
-static int
-serial_omap_suspend(struct platform_device *pdev, pm_message_t state)
+static int serial_omap_suspend(struct device *dev)
 ?{
- ? ? struct uart_omap_port *up = platform_get_drvdata(pdev);

Review static struct uart_driver serial_omap_reg = { · line 1136

Quoted 10 lines

(quoted diff) +7 -1

+ ? ? struct uart_omap_port *up = dev_get_drvdata(dev);
 
- ? ? if (up)
+ ? ? if (up) {
 ? ? ? ? ? ? ? uart_suspend_port(&serial_omap_reg, &up->port);
+ ? ? ? ? ? ? if (up->port.line == up->port.cons->index &&
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? !is_console_locked())
+ ? ? ? ? ? ? ? ? ? ? up->console_locked = console_trylock();
+ ? ? }
+

Motiviation for console locking in this version of the series is not clear, and not described in the changelog.

Quoted 3 lines

It's also present here in static suspend/resume but not in runtime suspend/resume, which also is suspicious.

Yes will add to change log,

We needed this for no console suspend use case
no console lock is taken in no_console_suspend is specified,

Since our pwr_dmn hooks disable clocks left enabled during suspend, so any prints after pwr_dmn hooks can cause problems since clocks are already cut from pwr_dmn hooks.

So buffer prints while entering suspend by taking console lock if not taken already and print back after uart state machine restores console uart.

Quoted 9 lines · Govindraj

Quoted 8 lines · Kevin Hilman

Quoted 5 lines · Govindraj.R

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

1.) Moving context_restore func to driver from serial.c 2.) Use runtime irq safe to use get_sync from irq context. 3.) autosuspend for port specific activities and put for reg access.

Quoted 9 lines

Quoted 5 lines

Re: autosuspend, it looks like the driver now has 2 mechanisms for tracking activity. ?The runtime PM 'mark last busy' and the existing 'up->port_activity = jiffies' stuff. ?Do you think those could be unified?

Is there a way where we can retrieve the last busy jiffie value using runtime API? I don't find that available.

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

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

Quoted 8 lines

Quoted 5 lines

At first glance, it looks like most places with a _mark_last_busy() also have a up->port_activity update. ?I'm not familiar enough with the driver to make the call, but they look awfully similar.

Ok, I will check whether I can get rid if it.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

4.) for earlyprintk usage the debug macro will write to console uart ? ? so keep uart clocks active from boot, idle using hwmod API's re-enable back ? ? using runtime API's.

/me doesn't understand

I have added this reason in early mail reply to 04/11 patch review [needed for earlyprintk option from low level debug ]

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

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

Quoted 47 lines

Quoted 15 lines

Quoted 15 lines

5.) Moving context restore to runtime suspend and using reg values from port ? ? structure to restore the uart port context based on context_loss_count. ? ? Maintain internal state machine for avoiding repeated enable/disable of ? ? uart port wakeup mechanism.
6.) Add API to enable wakeup and check wakeup status.

Acked-by Alan Cox <[email protected]>

Signed-off-by Govindraj.R <[email protected]>

---

?arch/arm/mach-omap2/serial.c ? ? ? ? ? ? ? ? ?| ? 49 ++++++
?arch/arm/plat-omap/include/plat/omap-serial.h | ? 10 ++
?drivers/tty/serial/omap-serial.c ? ? ? ? ? ? ?| ?211 ++++++++++++++++++++++---

?3 files changed, 250 insertions(+), 20 deletions(-)

[..]

Quoted 28 lines

Quoted 13 lines

(quoted diff) +13 -0

+
+static void omap_uart_wakeup_enable(struct platform_device *pdev, bool enable)
+{
+ ? ? struct omap_device *od;
+ ? ? struct omap_uart_port_info *up = pdev->dev.platform_data;
+
+ ? ? /* Set or clear wake-enable bit */
+ ? ? if (up->wk_en && up->wk_mask) {
+ ? ? ? ? ? ? u32 v = __raw_readl(up->wk_en);
+ ? ? ? ? ? ? if (enable)
+ ? ? ? ? ? ? ? ? ? ? v |= up->wk_mask;
+ ? ? ? ? ? ? else
+ ? ? ? ? ? ? ? ? ? ? v &= ~up->wk_mask;

Review

Quoted 10 lines

(quoted diff) +10 -0

+ ? ? ? ? ? ? __raw_writel(v, up->wk_en);
+ ? ? }
+
+ ? ? od = to_omap_device(pdev);
+ ? ? if (enable)
+ ? ? ? ? ? ? omap_hwmod_enable_wakeup(od->hwmods[0]);
+ ? ? else
+ ? ? ? ? ? ? omap_hwmod_disable_wakeup(od->hwmods[0]);
+}
+

Here again, this is difficult to review because you removed the code in [4/11] and add it back here, but with rather different functionality with no description as to why.

Quoted 2 lines

will move this change as part of 4/11 itself.

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

Quoted 8 lines

Quoted 4 lines

The previous version enabled wakeups at the PRCM and at the IO ring. This version enables wakeups at the PRCM as well but instead of enabling them at the IO ring, enables them at the module.

Since we are gating using prepare idle call I think
we can use this enable wake-up call as part of prepare idle call itself, as done earlier.

Not sure what that has to with my comment.

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

[...]

Quoted 39 lines

Quoted 26 lines

Quoted 12 lines

?}

?static int __init

(quoted diff) +1 -3

@@ -1065,22 +1123,33 @@ static struct uart_driver serial_omap_reg = {
 ? ? ? .cons ? ? ? ? ? = OMAP_CONSOLE,
 ?};
 
-static int
-serial_omap_suspend(struct platform_device *pdev, pm_message_t state)
+static int serial_omap_suspend(struct device *dev)
 ?{
- ? ? struct uart_omap_port *up = platform_get_drvdata(pdev);

Review static struct uart_driver serial_omap_reg = { · line 1136

Quoted 10 lines

(quoted diff) +7 -1

+ ? ? struct uart_omap_port *up = dev_get_drvdata(dev);
 
- ? ? if (up)
+ ? ? if (up) {
 ? ? ? ? ? ? ? uart_suspend_port(&serial_omap_reg, &up->port);
+ ? ? ? ? ? ? if (up->port.line == up->port.cons->index &&
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? !is_console_locked())
+ ? ? ? ? ? ? ? ? ? ? up->console_locked = console_trylock();
+ ? ? }
+

Motiviation for console locking in this version of the series is not clear, and not described in the changelog.

Quoted 3 lines

It's also present here in static suspend/resume but not in runtime suspend/resume, which also is suspicious.

Yes will add to change log,

We needed this for no console suspend use case
no console lock is taken in no_console_suspend is specified,

Since our pwr_dmn hooks disable clocks left enabled during suspend, so any prints after pwr_dmn hooks can cause problems since clocks are already cut from pwr_dmn hooks.

Quoted 3 lines

So buffer prints while entering suspend by taking console lock if not taken already and print back after uart state machine restores console uart.

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

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

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

Kevin

Quoted 21 lines · Kevin Hilman

Quoted 9 lines · Govindraj

Quoted 8 lines · Kevin Hilman

Quoted 5 lines · Govindraj.R

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

1.) Moving context_restore func to driver from serial.c 2.) Use runtime irq safe to use get_sync from irq context. 3.) autosuspend for port specific activities and put for reg access.

Quoted 9 lines

Quoted 5 lines

Re: autosuspend, it looks like the driver now has 2 mechanisms for tracking activity. ?The runtime PM 'mark last busy' and the existing 'up->port_activity = jiffies' stuff. ?Do you think those could be unified?

Is there a way where we can retrieve the last busy jiffie value using runtime API? I don't find that available.

Quoted 5 lines

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

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

Okay.

Quoted 17 lines

Quoted 8 lines

Quoted 5 lines

At first glance, it looks like most places with a _mark_last_busy() also have a up->port_activity update. ?I'm not familiar enough with the driver to make the call, but they look awfully similar.

Ok, I will check whether I can get rid if it.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

4.) for earlyprintk usage the debug macro will write to console uart ? ? so keep uart clocks active from boot, idle using hwmod API's re-enable back ? ? using runtime API's.

/me doesn't understand

I have added this reason in early mail reply to 04/11 patch review [needed for earlyprintk option from low level debug ]

Quoted 7 lines

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

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

Yes these are required, because early printk relies on clocks from bootloader and if clocks are gated even before console_driver is available it can lead to boot failures.

I can keep that part as a separate patch.

Quoted 47 lines

Quoted 47 lines

Quoted 15 lines

Quoted 15 lines

5.) Moving context restore to runtime suspend and using reg values from port ? ? structure to restore the uart port context based on context_loss_count. ? ? Maintain internal state machine for avoiding repeated enable/disable of ? ? uart port wakeup mechanism.
6.) Add API to enable wakeup and check wakeup status.

Acked-by Alan Cox <[email protected]>

Signed-off-by Govindraj.R <[email protected]>

---

?arch/arm/mach-omap2/serial.c ? ? ? ? ? ? ? ? ?| ? 49 ++++++
?arch/arm/plat-omap/include/plat/omap-serial.h | ? 10 ++
?drivers/tty/serial/omap-serial.c ? ? ? ? ? ? ?| ?211 ++++++++++++++++++++++---

?3 files changed, 250 insertions(+), 20 deletions(-)

[..]

Quoted 28 lines

Quoted 13 lines

(quoted diff) +13 -0

+
+static void omap_uart_wakeup_enable(struct platform_device *pdev, bool enable)
+{
+ ? ? struct omap_device *od;
+ ? ? struct omap_uart_port_info *up = pdev->dev.platform_data;
+
+ ? ? /* Set or clear wake-enable bit */
+ ? ? if (up->wk_en && up->wk_mask) {
+ ? ? ? ? ? ? u32 v = __raw_readl(up->wk_en);
+ ? ? ? ? ? ? if (enable)
+ ? ? ? ? ? ? ? ? ? ? v |= up->wk_mask;
+ ? ? ? ? ? ? else
+ ? ? ? ? ? ? ? ? ? ? v &= ~up->wk_mask;

Review

Quoted 10 lines

(quoted diff) +10 -0

+ ? ? ? ? ? ? __raw_writel(v, up->wk_en);
+ ? ? }
+
+ ? ? od = to_omap_device(pdev);
+ ? ? if (enable)
+ ? ? ? ? ? ? omap_hwmod_enable_wakeup(od->hwmods[0]);
+ ? ? else
+ ? ? ? ? ? ? omap_hwmod_disable_wakeup(od->hwmods[0]);
+}
+

Here again, this is difficult to review because you removed the code in [4/11] and add it back here, but with rather different functionality with no description as to why.

Quoted 6 lines

Quoted 2 lines

will move this change as part of 4/11 itself.

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

If are referring to uart rx pad wakeup its still retained but done using mux framework.

Quoted 9 lines

Quoted 8 lines

Quoted 4 lines

The previous version enabled wakeups at the PRCM and at the IO ring. This version enables wakeups at the PRCM as well but instead of enabling them at the IO ring, enables them at the module.

Since we are gating using prepare idle call I think
we can use this enable wake-up call as part of prepare idle call itself, as done earlier.

Quoted 7 lines

Not sure what that has to with my comment.

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

Prior to this we used io-pad offsets address and modified to enable wakeup mechanism, But since we received some comments from Tony to use mux framework for any mux changes, I removed using mux address and use mux framework from hwmod, I have not removed rx-pad wakeup still enable io-pad wakeup. otherwise wakeup from off-mode will fail on 3430SDP.

omap_uart_wakeup_enable
? ?--> omap_hwmod_enable_wakeup
? ? ? ? ? --> omap_hwmod_enable_ioring_wakeup [PATCH v4 01/11] OMAP2+: hwmod: Add API to enable IO ring wakeup.]

boot-loaders doesn't enable io pad wakeup for uart.

Quoted 41 lines

[...]

Quoted 39 lines

Quoted 26 lines

Quoted 12 lines

?}

?static int __init

(quoted diff) +1 -3

@@ -1065,22 +1123,33 @@ static struct uart_driver serial_omap_reg = {
 ? ? ? .cons ? ? ? ? ? = OMAP_CONSOLE,
 ?};
 
-static int
-serial_omap_suspend(struct platform_device *pdev, pm_message_t state)
+static int serial_omap_suspend(struct device *dev)
 ?{
- ? ? struct uart_omap_port *up = platform_get_drvdata(pdev);

Review static struct uart_driver serial_omap_reg = { · line 1136

Quoted 10 lines

(quoted diff) +7 -1

+ ? ? struct uart_omap_port *up = dev_get_drvdata(dev);
 
- ? ? if (up)
+ ? ? if (up) {
 ? ? ? ? ? ? ? uart_suspend_port(&serial_omap_reg, &up->port);
+ ? ? ? ? ? ? if (up->port.line == up->port.cons->index &&
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? !is_console_locked())
+ ? ? ? ? ? ? ? ? ? ? up->console_locked = console_trylock();
+ ? ? }
+

Motiviation for console locking in this version of the series is not clear, and not described in the changelog.

Quoted 3 lines

It's also present here in static suspend/resume but not in runtime suspend/resume, which also is suspicious.

Yes will add to change log,

We needed this for no console suspend use case
no console lock is taken in no_console_suspend is specified,

Since our pwr_dmn hooks disable clocks left enabled during suspend, so any prints after pwr_dmn hooks can cause problems since clocks are already cut from pwr_dmn hooks.

Quoted 7 lines

Quoted 3 lines

So buffer prints while entering suspend by taking console lock if not taken already and print back after uart state machine restores console uart.

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

will add as separate patch.

Quoted 7 lines

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

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

Yes correct, But pwr domain callback will now cut all enabled clocks now, so only available method was to use console_lock and print later.

pwr_domain callback done in omap-device.c
will force to low powers state by gating all clocks that where left enabled by driver.

--
Thanks,
Govindraj.R

Quoted 4 lines

Kevin

Quoted 21 lines · Kevin Hilman

Quoted 9 lines · Govindraj

Quoted 8 lines · Kevin Hilman

Quoted 5 lines · Govindraj.R

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

1.) Moving context_restore func to driver from serial.c 2.) Use runtime irq safe to use get_sync from irq context. 3.) autosuspend for port specific activities and put for reg access.

Quoted 9 lines

Quoted 5 lines

Re: autosuspend, it looks like the driver now has 2 mechanisms for tracking activity. ?The runtime PM 'mark last busy' and the existing 'up->port_activity = jiffies' stuff. ?Do you think those could be unified?

Is there a way where we can retrieve the last busy jiffie value using runtime API? I don't find that available.

Quoted 5 lines

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

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

Okay.

Quoted 21 lines

Quoted 8 lines

Quoted 5 lines

At first glance, it looks like most places with a _mark_last_busy() also have a up->port_activity update. ?I'm not familiar enough with the driver to make the call, but they look awfully similar.

Ok, I will check whether I can get rid if it.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

4.) for earlyprintk usage the debug macro will write to console uart ? ? so keep uart clocks active from boot, idle using hwmod API's re-enable back ? ? using runtime API's.

/me doesn't understand

I have added this reason in early mail reply to 04/11 patch review [needed for earlyprintk option from low level debug ]

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

Quoted 3 lines

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

Yes these are required, because early printk relies on clocks from bootloader and if clocks are gated even before console_driver is available it can lead to boot failures.

I can keep that part as a separate patch.

Quoted 50 lines

Quoted 49 lines

Quoted 15 lines

Quoted 15 lines

5.) Moving context restore to runtime suspend and using reg values from port ? ? structure to restore the uart port context based on context_loss_count. ? ? Maintain internal state machine for avoiding repeated enable/disable of ? ? uart port wakeup mechanism.
6.) Add API to enable wakeup and check wakeup status.

Acked-by Alan Cox <[email protected]>

Signed-off-by Govindraj.R <[email protected]>

---

?arch/arm/mach-omap2/serial.c ? ? ? ? ? ? ? ? ?| ? 49 ++++++
?arch/arm/plat-omap/include/plat/omap-serial.h | ? 10 ++
?drivers/tty/serial/omap-serial.c ? ? ? ? ? ? ?| ?211 ++++++++++++++++++++++---

?3 files changed, 250 insertions(+), 20 deletions(-)

[..]

Quoted 19 lines

Quoted 19 lines

(quoted diff) +19 -0

+
+static void omap_uart_wakeup_enable(struct platform_device *pdev, bool enable)
+{
+ ? ? struct omap_device *od;
+ ? ? struct omap_uart_port_info *up = pdev->dev.platform_data;
+
+ ? ? /* Set or clear wake-enable bit */
+ ? ? if (up->wk_en && up->wk_mask) {
+ ? ? ? ? ? ? u32 v = __raw_readl(up->wk_en);
+ ? ? ? ? ? ? if (enable)
+ ? ? ? ? ? ? ? ? ? ? v |= up->wk_mask;
+ ? ? ? ? ? ? else
+ ? ? ? ? ? ? ? ? ? ? v &= ~up->wk_mask;
+ ? ? ? ? ? ? __raw_writel(v, up->wk_en);
+ ? ? }
+
+ ? ? od = to_omap_device(pdev);
+ ? ? if (enable)
+ ? ? ? ? ? ? omap_hwmod_enable_wakeup(od->hwmods[0]);

Quoted 9 lines

Review

Quoted 4 lines

+ ? ? else
+ ? ? ? ? ? ? omap_hwmod_disable_wakeup(od->hwmods[0]);

(quoted diff) +2 -0

+}
+

Here again, this is difficult to review because you removed the code in [4/11] and add it back here, but with rather different functionality with no description as to why.

will move this change as part of 4/11 itself.

Quoted 3 lines

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

If are referring to uart rx pad wakeup its still retained but done using mux framework.

Quoted 9 lines

Quoted 8 lines

Quoted 4 lines

The previous version enabled wakeups at the PRCM and at the IO ring. This version enables wakeups at the PRCM as well but instead of enabling them at the IO ring, enables them at the module.

Since we are gating using prepare idle call I think
we can use this enable wake-up call as part of prepare idle call itself, as done earlier.

Quoted 7 lines

Not sure what that has to with my comment.

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

Prior to this we used io-pad offsets address and modified to enable wakeup mechanism, But since we received some comments from Tony to use mux framework for any mux changes, I removed using mux address and use mux framework from hwmod, I have not removed rx-pad wakeup still enable io-pad wakeup. otherwise wakeup from off-mode will fail on 3430SDP.

omap_uart_wakeup_enable

    --> omap_hwmod_enable_wakeup
           --> omap_hwmod_enable_ioring_wakeup [PATCH v4 01/11]

OMAP2+: hwmod: Add API to enable IO ring wakeup.]

boot-loaders doesn't enable io pad wakeup for uart.

Quoted 45 lines

[...]

Quoted 42 lines

Quoted 26 lines

Quoted 12 lines

?}

?static int __init

(quoted diff) +1 -3

@@ -1065,22 +1123,33 @@ static struct uart_driver serial_omap_reg = {
 ? ? ? .cons ? ? ? ? ? = OMAP_CONSOLE,
 ?};
 
-static int
-serial_omap_suspend(struct platform_device *pdev, pm_message_t state)
+static int serial_omap_suspend(struct device *dev)
 ?{
- ? ? struct uart_omap_port *up = platform_get_drvdata(pdev);

Review static struct uart_driver serial_omap_reg = { · line 1136

Quoted 10 lines

(quoted diff) +7 -1

+ ? ? struct uart_omap_port *up = dev_get_drvdata(dev);
 
- ? ? if (up)
+ ? ? if (up) {
 ? ? ? ? ? ? ? uart_suspend_port(&serial_omap_reg, &up->port);
+ ? ? ? ? ? ? if (up->port.line == up->port.cons->index &&
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? !is_console_locked())
+ ? ? ? ? ? ? ? ? ? ? up->console_locked = console_trylock();
+ ? ? }
+

Motiviation for console locking in this version of the series is not clear, and not described in the changelog.

Quoted 3 lines

It's also present here in static suspend/resume but not in runtime suspend/resume, which also is suspicious.

Yes will add to change log,

We needed this for no console suspend use case
no console lock is taken in no_console_suspend is specified,

Since our pwr_dmn hooks disable clocks left enabled during suspend, so any prints after pwr_dmn hooks can cause problems since clocks are already cut from pwr_dmn hooks.

So buffer prints while entering suspend by taking console lock if not taken already and print back after uart state machine restores console uart.