[PATCH 0/2] fsl_lpuart: add some new features for lpuart driver

[PATCH 0/2] fsl_lpuart: add some new features for lpuart driver

[Sherry Sun]

The patches have added the wakeup source support for lpuart, also enable the
runtime pm for lpuart driver, it has been tested on imx8ulp-evk board, and the
two new features can work well.

Sherry Sun (2):
  tty: serial: fsl_lpuart: enable wakeup source for lpuart
  tty: serial: fsl_lpuart: Add runtime pm support

 drivers/tty/serial/fsl_lpuart.c | 341 ++++++++++++++++++++++++--------
 1 file changed, 260 insertions(+), 81 deletions(-)

-- 
2.17.1

[PATCH 1/2] tty: serial: fsl_lpuart: enable wakeup source for lpuart

[Sherry Sun]

LPUART supports both synchronous wakeup and asynchronous wakeup(wakeup
the system when the UART clocks are shut-off), the synchronous wakeup is
configured by UARTCTRL_RIE interrupt, and the asynchronous wakeup is
configured by UARTBAUD_RXEDGIE interrupt.

Add lpuart_uport_is_active() to determine if the uart port needs to get
into the suspend states, also add lpuart_suspend_noirq() and
lpuart_resume_noirq() to enable and disable the wakeup irq bits if the
uart port needs to be set as wakeup source.

When use lpuart with DMA mode, it still needs to switch to the cpu mode
in .suspend() that enable cpu interrupts RIE and RXEDGIE as wakeup
source, after system resume back, needs to setup DMA again, .resume()
will share the HW setup code with .startup(), so abstract the same code
to the api like lpuart32_hw_setup().

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
---
 drivers/tty/serial/fsl_lpuart.c | 281 +++++++++++++++++++++++---------
 1 file changed, 200 insertions(+), 81 deletions(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 43d9d6a6e94a..474943cb06b2 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -18,6 +18,7 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_dma.h>
+#include <linux/pinctrl/consumer.h>
 #include <linux/serial_core.h>
 #include <linux/slab.h>
 #include <linux/tty_flip.h>
@@ -1630,10 +1631,23 @@ static void lpuart_rx_dma_startup(struct lpuart_port *sport)
 	sport->lpuart_dma_rx_use = false;
 }
 
+static void lpuart_hw_setup(struct lpuart_port *sport)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&sport->port.lock, flags);
+
+	lpuart_setup_watermark_enable(sport);
+
+	lpuart_rx_dma_startup(sport);
+	lpuart_tx_dma_startup(sport);
+
+	spin_unlock_irqrestore(&sport->port.lock, flags);
+}
+
 static int lpuart_startup(struct uart_port *port)
 {
 	struct lpuart_port *sport = container_of(port, struct lpuart_port, port);
-	unsigned long flags;
 	unsigned char temp;
 
 	/* determine FIFO size and enable FIFO mode */
@@ -1647,15 +1661,7 @@ static int lpuart_startup(struct uart_port *port)
 					    UARTPFIFO_FIFOSIZE_MASK);
 
 	lpuart_request_dma(sport);
-
-	spin_lock_irqsave(&sport->port.lock, flags);
-
-	lpuart_setup_watermark_enable(sport);
-
-	lpuart_rx_dma_startup(sport);
-	lpuart_tx_dma_startup(sport);
-
-	spin_unlock_irqrestore(&sport->port.lock, flags);
+	lpuart_hw_setup(sport);
 
 	return 0;
 }
@@ -1678,10 +1684,25 @@ static void lpuart32_configure(struct lpuart_port *sport)
 	lpuart32_write(&sport->port, temp, UARTCTRL);
 }
 
+static void lpuart32_hw_setup(struct lpuart_port *sport)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&sport->port.lock, flags);
+
+	lpuart32_setup_watermark_enable(sport);
+
+	lpuart_rx_dma_startup(sport);
+	lpuart_tx_dma_startup(sport);
+
+	lpuart32_configure(sport);
+
+	spin_unlock_irqrestore(&sport->port.lock, flags);
+}
+
 static int lpuart32_startup(struct uart_port *port)
 {
 	struct lpuart_port *sport = container_of(port, struct lpuart_port, port);
-	unsigned long flags;
 	unsigned long temp;
 
 	/* determine FIFO size */
@@ -1706,17 +1727,8 @@ static int lpuart32_startup(struct uart_port *port)
 	}
 
 	lpuart_request_dma(sport);
+	lpuart32_hw_setup(sport);
 
-	spin_lock_irqsave(&sport->port.lock, flags);
-
-	lpuart32_setup_watermark_enable(sport);
-
-	lpuart_rx_dma_startup(sport);
-	lpuart_tx_dma_startup(sport);
-
-	lpuart32_configure(sport);
-
-	spin_unlock_irqrestore(&sport->port.lock, flags);
 	return 0;
 }
 
@@ -2780,97 +2792,204 @@ static int lpuart_remove(struct platform_device *pdev)
 	return 0;
 }
 
-static int __maybe_unused lpuart_suspend(struct device *dev)
+static void serial_lpuart_enable_wakeup(struct lpuart_port *sport, bool on)
 {
-	struct lpuart_port *sport = dev_get_drvdata(dev);
-	unsigned long temp;
-	bool irq_wake;
+	unsigned int val, baud;
 
 	if (lpuart_is_32(sport)) {
-		/* disable Rx/Tx and interrupts */
-		temp = lpuart32_read(&sport->port, UARTCTRL);
-		temp &= ~(UARTCTRL_TE | UARTCTRL_TIE | UARTCTRL_TCIE);
-		lpuart32_write(&sport->port, temp, UARTCTRL);
+		val = lpuart32_read(&sport->port, UARTCTRL);
+		baud = lpuart32_read(&sport->port, UARTBAUD);
+		if (on) {
+			/* set rx_watermark to 0 in wakeup source mode */
+			lpuart32_write(&sport->port, 0, UARTWATER);
+			val |= UARTCTRL_RIE;
+			/* clear RXEDGIF flag before enable RXEDGIE interrupt */
+			lpuart32_write(&sport->port, UARTSTAT_RXEDGIF, UARTSTAT);
+			baud |= UARTBAUD_RXEDGIE;
+		} else {
+			val &= ~UARTCTRL_RIE;
+			baud &= ~UARTBAUD_RXEDGIE;
+		}
+		lpuart32_write(&sport->port, val, UARTCTRL);
+		lpuart32_write(&sport->port, baud, UARTBAUD);
 	} else {
-		/* disable Rx/Tx and interrupts */
-		temp = readb(sport->port.membase + UARTCR2);
-		temp &= ~(UARTCR2_TE | UARTCR2_TIE | UARTCR2_TCIE);
-		writeb(temp, sport->port.membase + UARTCR2);
+		val = readb(sport->port.membase + UARTCR2);
+		if (on)
+			val |= UARTCR2_RIE;
+		else
+			val &= ~UARTCR2_RIE;
+		writeb(val, sport->port.membase + UARTCR2);
 	}
+}
 
-	uart_suspend_port(&lpuart_reg, &sport->port);
+static bool lpuart_uport_is_active(struct lpuart_port *sport)
+{
+	struct tty_port *port = &sport->port.state->port;
+	struct tty_struct *tty;
+	struct device *tty_dev;
+	int may_wake = 0;
 
-	/* uart_suspend_port() might set wakeup flag */
-	irq_wake = irqd_is_wakeup_set(irq_get_irq_data(sport->port.irq));
+	tty = tty_port_tty_get(port);
+	if (tty) {
+		tty_dev = tty->dev;
+		may_wake = device_may_wakeup(tty_dev);
+		tty_kref_put(tty);
+	}
 
-	if (sport->lpuart_dma_rx_use) {
-		/*
-		 * EDMA driver during suspend will forcefully release any
-		 * non-idle DMA channels. If port wakeup is enabled or if port
-		 * is console port or 'no_console_suspend' is set the Rx DMA
-		 * cannot resume as expected, hence gracefully release the
-		 * Rx DMA path before suspend and start Rx DMA path on resume.
-		 */
-		if (irq_wake) {
-			del_timer_sync(&sport->lpuart_timer);
-			lpuart_dma_rx_free(&sport->port);
-		}
+	if ((tty_port_initialized(port) && may_wake) ||
+	    (!console_suspend_enabled && uart_console(&sport->port)))
+		return true;
+
+	return false;
+}
+
+static int lpuart_suspend_noirq(struct device *dev)
+{
+	struct lpuart_port *sport = dev_get_drvdata(dev);
+	bool irq_wake = irqd_is_wakeup_set(irq_get_irq_data(sport->port.irq));
+
+	if (lpuart_uport_is_active(sport))
+		serial_lpuart_enable_wakeup(sport, !!irq_wake);
+
+	pinctrl_pm_select_sleep_state(dev);
+
+	return 0;
+}
+
+static int lpuart_resume_noirq(struct device *dev)
+{
+	struct lpuart_port *sport = dev_get_drvdata(dev);
+	unsigned int val;
+
+	pinctrl_pm_select_default_state(dev);
+
+	if (lpuart_uport_is_active(sport)) {
+		serial_lpuart_enable_wakeup(sport, false);
 
-		/* Disable Rx DMA to use UART port as wakeup source */
+		/* clear the wakeup flags */
 		if (lpuart_is_32(sport)) {
-			temp = lpuart32_read(&sport->port, UARTBAUD);
-			lpuart32_write(&sport->port, temp & ~UARTBAUD_RDMAE,
-				       UARTBAUD);
-		} else {
-			writeb(readb(sport->port.membase + UARTCR5) &
-			       ~UARTCR5_RDMAS, sport->port.membase + UARTCR5);
+			val = lpuart32_read(&sport->port, UARTSTAT);
+			lpuart32_write(&sport->port, val, UARTSTAT);
 		}
 	}
 
-	if (sport->lpuart_dma_tx_use) {
-		sport->dma_tx_in_progress = false;
-		dmaengine_terminate_all(sport->dma_tx_chan);
-	}
-
-	if (sport->port.suspended && !irq_wake)
-		lpuart_disable_clks(sport);
-
 	return 0;
 }
 
-static int __maybe_unused lpuart_resume(struct device *dev)
+static int __maybe_unused lpuart_suspend(struct device *dev)
 {
 	struct lpuart_port *sport = dev_get_drvdata(dev);
-	bool irq_wake = irqd_is_wakeup_set(irq_get_irq_data(sport->port.irq));
+	unsigned long temp, flags;
 
-	if (sport->port.suspended && !irq_wake)
-		lpuart_enable_clks(sport);
+	uart_suspend_port(&lpuart_reg, &sport->port);
 
-	if (lpuart_is_32(sport))
-		lpuart32_setup_watermark_enable(sport);
-	else
-		lpuart_setup_watermark_enable(sport);
+	if (lpuart_uport_is_active(sport)) {
+		spin_lock_irqsave(&sport->port.lock, flags);
+		if (lpuart_is_32(sport)) {
+			/* disable Rx/Tx and interrupts */
+			temp = lpuart32_read(&sport->port, UARTCTRL);
+			temp &= ~(UARTCTRL_TE | UARTCTRL_TIE | UARTCTRL_TCIE);
+			lpuart32_write(&sport->port, temp, UARTCTRL);
+		} else {
+			/* disable Rx/Tx and interrupts */
+			temp = readb(sport->port.membase + UARTCR2);
+			temp &= ~(UARTCR2_TE | UARTCR2_TIE | UARTCR2_TCIE);
+			writeb(temp, sport->port.membase + UARTCR2);
+		}
+		spin_unlock_irqrestore(&sport->port.lock, flags);
 
-	if (sport->lpuart_dma_rx_use) {
-		if (irq_wake) {
-			if (!lpuart_start_rx_dma(sport))
-				rx_dma_timer_init(sport);
-			else
-				sport->lpuart_dma_rx_use = false;
+		if (sport->lpuart_dma_rx_use) {
+			/*
+			 * EDMA driver during suspend will forcefully release any
+			 * non-idle DMA channels. If port wakeup is enabled or if port
+			 * is console port or 'no_console_suspend' is set the Rx DMA
+			 * cannot resume as expected, hence gracefully release the
+			 * Rx DMA path before suspend and start Rx DMA path on resume.
+			 */
+			del_timer_sync(&sport->lpuart_timer);
+			lpuart_dma_rx_free(&sport->port);
+
+			/* Disable Rx DMA to use UART port as wakeup source */
+			spin_lock_irqsave(&sport->port.lock, flags);
+			if (lpuart_is_32(sport)) {
+				temp = lpuart32_read(&sport->port, UARTBAUD);
+				lpuart32_write(&sport->port, temp & ~UARTBAUD_RDMAE,
+					       UARTBAUD);
+			} else {
+				writeb(readb(sport->port.membase + UARTCR5) &
+				       ~UARTCR5_RDMAS, sport->port.membase + UARTCR5);
+			}
+			spin_unlock_irqrestore(&sport->port.lock, flags);
+		}
+
+		if (sport->lpuart_dma_tx_use) {
+			spin_lock_irqsave(&sport->port.lock, flags);
+			if (lpuart_is_32(sport)) {
+				temp = lpuart32_read(&sport->port, UARTBAUD);
+				temp &= ~UARTBAUD_TDMAE;
+				lpuart32_write(&sport->port, temp, UARTBAUD);
+			} else {
+				temp = readb(sport->port.membase + UARTCR5);
+				temp &= ~UARTCR5_TDMAS;
+				writeb(temp, sport->port.membase + UARTCR5);
+			}
+			spin_unlock_irqrestore(&sport->port.lock, flags);
+			sport->dma_tx_in_progress = false;
+			dmaengine_terminate_all(sport->dma_tx_chan);
 		}
 	}
 
-	lpuart_tx_dma_startup(sport);
+	return 0;
+}
 
-	if (lpuart_is_32(sport))
-		lpuart32_configure(sport);
+static void lpuart_console_fixup(struct lpuart_port *sport)
+{
+	struct tty_port *port = &sport->port.state->port;
+	struct uart_port *uport = &sport->port;
+	struct ktermios termios;
+
+	/* i.MX7ULP enter VLLS mode that lpuart module power off and registers
+	 * all lost no matter the port is wakeup source.
+	 * For console port, console baud rate setting lost and print messy
+	 * log when enable the console port as wakeup source. To avoid the
+	 * issue happen, user should not enable uart port as wakeup source
+	 * in VLLS mode, or restore console setting here.
+	 */
+	if (is_imx7ulp_lpuart(sport) && lpuart_uport_is_active(sport) &&
+	    console_suspend_enabled && uart_console(&sport->port)) {
+
+		mutex_lock(&port->mutex);
+		memset(&termios, 0, sizeof(struct ktermios));
+		termios.c_cflag = uport->cons->cflag;
+		if (port->tty && termios.c_cflag == 0)
+			termios = port->tty->termios;
+		uport->ops->set_termios(uport, &termios, NULL);
+		mutex_unlock(&port->mutex);
+	}
+}
+
+static int __maybe_unused lpuart_resume(struct device *dev)
+{
+	struct lpuart_port *sport = dev_get_drvdata(dev);
+
+	if (lpuart_uport_is_active(sport)) {
+		if (lpuart_is_32(sport))
+			lpuart32_hw_setup(sport);
+		else
+			lpuart_hw_setup(sport);
+	}
 
+	lpuart_console_fixup(sport);
 	uart_resume_port(&lpuart_reg, &sport->port);
 
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(lpuart_pm_ops, lpuart_suspend, lpuart_resume);
+static const struct dev_pm_ops lpuart_pm_ops = {
+	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(lpuart_suspend_noirq,
+				      lpuart_resume_noirq)
+	SET_SYSTEM_SLEEP_PM_OPS(lpuart_suspend, lpuart_resume)
+};
 
 static struct platform_driver lpuart_driver = {
 	.probe		= lpuart_probe,
-- 
2.17.1

[PATCH 2/2] tty: serial: fsl_lpuart: Add runtime pm support

[Sherry Sun]

Add runtime pm support to manage the lpuart clock.

Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
---
 drivers/tty/serial/fsl_lpuart.c | 60 +++++++++++++++++++++++++++++++++
 1 file changed, 60 insertions(+)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index 474943cb06b2..e678a7aaf7e4 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -19,6 +19,7 @@
 #include <linux/of_device.h>
 #include <linux/of_dma.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/pm_runtime.h>
 #include <linux/serial_core.h>
 #include <linux/slab.h>
 #include <linux/tty_flip.h>
@@ -235,6 +236,7 @@
 
 /* Rx DMA timeout in ms, which is used to calculate Rx ring buffer size */
 #define DMA_RX_TIMEOUT		(10)
+#define UART_AUTOSUSPEND_TIMEOUT	3000
 
 #define DRIVER_NAME	"fsl-lpuart"
 #define DEV_NAME	"ttyLP"
@@ -795,6 +797,20 @@ static void lpuart32_start_tx(struct uart_port *port)
 	}
 }
 
+static void
+lpuart_uart_pm(struct uart_port *port, unsigned int state, unsigned int oldstate)
+{
+	switch (state) {
+	case UART_PM_STATE_OFF:
+		pm_runtime_mark_last_busy(port->dev);
+		pm_runtime_put_autosuspend(port->dev);
+		break;
+	default:
+		pm_runtime_get_sync(port->dev);
+		break;
+	}
+}
+
 /* return TIOCSER_TEMT when transmitter is not busy */
 static unsigned int lpuart_tx_empty(struct uart_port *port)
 {
@@ -2243,6 +2259,7 @@ static const struct uart_ops lpuart_pops = {
 	.startup	= lpuart_startup,
 	.shutdown	= lpuart_shutdown,
 	.set_termios	= lpuart_set_termios,
+	.pm		= lpuart_uart_pm,
 	.type		= lpuart_type,
 	.request_port	= lpuart_request_port,
 	.release_port	= lpuart_release_port,
@@ -2267,6 +2284,7 @@ static const struct uart_ops lpuart32_pops = {
 	.startup	= lpuart32_startup,
 	.shutdown	= lpuart32_shutdown,
 	.set_termios	= lpuart32_set_termios,
+	.pm		= lpuart_uart_pm,
 	.type		= lpuart_type,
 	.request_port	= lpuart_request_port,
 	.release_port	= lpuart_release_port,
@@ -2747,6 +2765,11 @@ static int lpuart_probe(struct platform_device *pdev)
 		handler = lpuart_int;
 	}
 
+	pm_runtime_use_autosuspend(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, UART_AUTOSUSPEND_TIMEOUT);
+	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_enable(&pdev->dev);
+
 	ret = lpuart_global_reset(sport);
 	if (ret)
 		goto failed_reset;
@@ -2771,6 +2794,9 @@ static int lpuart_probe(struct platform_device *pdev)
 failed_attach_port:
 failed_get_rs485:
 failed_reset:
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	lpuart_disable_clks(sport);
 	return ret;
 }
@@ -2789,9 +2815,30 @@ static int lpuart_remove(struct platform_device *pdev)
 	if (sport->dma_rx_chan)
 		dma_release_channel(sport->dma_rx_chan);
 
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
 	return 0;
 }
 
+static int __maybe_unused lpuart_runtime_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct lpuart_port *sport = platform_get_drvdata(pdev);
+
+	lpuart_disable_clks(sport);
+
+	return 0;
+};
+
+static int __maybe_unused lpuart_runtime_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct lpuart_port *sport = platform_get_drvdata(pdev);
+
+	return lpuart_enable_clks(sport);
+};
+
 static void serial_lpuart_enable_wakeup(struct lpuart_port *sport, bool on)
 {
 	unsigned int val, baud;
@@ -2937,6 +2984,10 @@ static int __maybe_unused lpuart_suspend(struct device *dev)
 			sport->dma_tx_in_progress = false;
 			dmaengine_terminate_all(sport->dma_tx_chan);
 		}
+	} else if (pm_runtime_active(sport->port.dev)) {
+		lpuart_disable_clks(sport);
+		pm_runtime_disable(sport->port.dev);
+		pm_runtime_set_suspended(sport->port.dev);
 	}
 
 	return 0;
@@ -2971,12 +3022,19 @@ static void lpuart_console_fixup(struct lpuart_port *sport)
 static int __maybe_unused lpuart_resume(struct device *dev)
 {
 	struct lpuart_port *sport = dev_get_drvdata(dev);
+	int ret;
 
 	if (lpuart_uport_is_active(sport)) {
 		if (lpuart_is_32(sport))
 			lpuart32_hw_setup(sport);
 		else
 			lpuart_hw_setup(sport);
+	} else if (pm_runtime_active(sport->port.dev)) {
+		ret = lpuart_enable_clks(sport);
+		if (ret)
+			return ret;
+		pm_runtime_set_active(sport->port.dev);
+		pm_runtime_enable(sport->port.dev);
 	}
 
 	lpuart_console_fixup(sport);
@@ -2986,6 +3044,8 @@ static int __maybe_unused lpuart_resume(struct device *dev)
 }
 
 static const struct dev_pm_ops lpuart_pm_ops = {
+	SET_RUNTIME_PM_OPS(lpuart_runtime_suspend,
+			   lpuart_runtime_resume, NULL)
 	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(lpuart_suspend_noirq,
 				      lpuart_resume_noirq)
 	SET_SYSTEM_SLEEP_PM_OPS(lpuart_suspend, lpuart_resume)
-- 
2.17.1

Re: [PATCH 2/2] tty: serial: fsl_lpuart: Add runtime pm support

[Ilpo Järvinen]

On Wed, 9 Nov 2022, Sherry Sun wrote:

> Add runtime pm support to manage the lpuart clock.
> 
> Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> ---
>  drivers/tty/serial/fsl_lpuart.c | 60 +++++++++++++++++++++++++++++++++
>  1 file changed, 60 insertions(+)
> 
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> index 474943cb06b2..e678a7aaf7e4 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -19,6 +19,7 @@
>  #include <linux/of_device.h>
>  #include <linux/of_dma.h>
>  #include <linux/pinctrl/consumer.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/serial_core.h>
>  #include <linux/slab.h>
>  #include <linux/tty_flip.h>
> @@ -235,6 +236,7 @@
>  
>  /* Rx DMA timeout in ms, which is used to calculate Rx ring buffer size */
>  #define DMA_RX_TIMEOUT		(10)
> +#define UART_AUTOSUSPEND_TIMEOUT	3000
>  
>  #define DRIVER_NAME	"fsl-lpuart"
>  #define DEV_NAME	"ttyLP"
> @@ -795,6 +797,20 @@ static void lpuart32_start_tx(struct uart_port *port)
>  	}
>  }
>  
> +static void
> +lpuart_uart_pm(struct uart_port *port, unsigned int state, unsigned int oldstate)
> +{
> +	switch (state) {
> +	case UART_PM_STATE_OFF:
> +		pm_runtime_mark_last_busy(port->dev);
> +		pm_runtime_put_autosuspend(port->dev);
> +		break;
> +	default:
> +		pm_runtime_get_sync(port->dev);
> +		break;
> +	}
> +}
> +
>  /* return TIOCSER_TEMT when transmitter is not busy */
>  static unsigned int lpuart_tx_empty(struct uart_port *port)
>  {
> @@ -2243,6 +2259,7 @@ static const struct uart_ops lpuart_pops = {
>  	.startup	= lpuart_startup,
>  	.shutdown	= lpuart_shutdown,
>  	.set_termios	= lpuart_set_termios,
> +	.pm		= lpuart_uart_pm,
>  	.type		= lpuart_type,
>  	.request_port	= lpuart_request_port,
>  	.release_port	= lpuart_release_port,
> @@ -2267,6 +2284,7 @@ static const struct uart_ops lpuart32_pops = {
>  	.startup	= lpuart32_startup,
>  	.shutdown	= lpuart32_shutdown,
>  	.set_termios	= lpuart32_set_termios,
> +	.pm		= lpuart_uart_pm,
>  	.type		= lpuart_type,
>  	.request_port	= lpuart_request_port,
>  	.release_port	= lpuart_release_port,
> @@ -2747,6 +2765,11 @@ static int lpuart_probe(struct platform_device *pdev)
>  		handler = lpuart_int;
>  	}
>  
> +	pm_runtime_use_autosuspend(&pdev->dev);
> +	pm_runtime_set_autosuspend_delay(&pdev->dev, UART_AUTOSUSPEND_TIMEOUT);
> +	pm_runtime_set_active(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +
>  	ret = lpuart_global_reset(sport);
>  	if (ret)
>  		goto failed_reset;
> @@ -2771,6 +2794,9 @@ static int lpuart_probe(struct platform_device *pdev)
>  failed_attach_port:
>  failed_get_rs485:
>  failed_reset:
> +	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_set_suspended(&pdev->dev);
> +	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	lpuart_disable_clks(sport);
>  	return ret;
>  }
> @@ -2789,9 +2815,30 @@ static int lpuart_remove(struct platform_device *pdev)
>  	if (sport->dma_rx_chan)
>  		dma_release_channel(sport->dma_rx_chan);
>  
> +	pm_runtime_disable(&pdev->dev);
> +	pm_runtime_set_suspended(&pdev->dev);
> +	pm_runtime_dont_use_autosuspend(&pdev->dev);
>  	return 0;
>  }
>  
> +static int __maybe_unused lpuart_runtime_suspend(struct device *dev)

IIRC, the PM_OPS macros contain special logic to make __maybe_unused 
unnecessary for these functions.

-- 
 i.


> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct lpuart_port *sport = platform_get_drvdata(pdev);
> +
> +	lpuart_disable_clks(sport);
> +
> +	return 0;
> +};
> +
> +static int __maybe_unused lpuart_runtime_resume(struct device *dev)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +	struct lpuart_port *sport = platform_get_drvdata(pdev);
> +
> +	return lpuart_enable_clks(sport);
> +};
> +
>  static void serial_lpuart_enable_wakeup(struct lpuart_port *sport, bool on)
>  {
>  	unsigned int val, baud;
> @@ -2937,6 +2984,10 @@ static int __maybe_unused lpuart_suspend(struct device *dev)
>  			sport->dma_tx_in_progress = false;
>  			dmaengine_terminate_all(sport->dma_tx_chan);
>  		}
> +	} else if (pm_runtime_active(sport->port.dev)) {
> +		lpuart_disable_clks(sport);
> +		pm_runtime_disable(sport->port.dev);
> +		pm_runtime_set_suspended(sport->port.dev);
>  	}
>  
>  	return 0;
> @@ -2971,12 +3022,19 @@ static void lpuart_console_fixup(struct lpuart_port *sport)
>  static int __maybe_unused lpuart_resume(struct device *dev)
>  {
>  	struct lpuart_port *sport = dev_get_drvdata(dev);
> +	int ret;
>  
>  	if (lpuart_uport_is_active(sport)) {
>  		if (lpuart_is_32(sport))
>  			lpuart32_hw_setup(sport);
>  		else
>  			lpuart_hw_setup(sport);
> +	} else if (pm_runtime_active(sport->port.dev)) {
> +		ret = lpuart_enable_clks(sport);
> +		if (ret)
> +			return ret;
> +		pm_runtime_set_active(sport->port.dev);
> +		pm_runtime_enable(sport->port.dev);
>  	}
>  
>  	lpuart_console_fixup(sport);
> @@ -2986,6 +3044,8 @@ static int __maybe_unused lpuart_resume(struct device *dev)
>  }
>  
>  static const struct dev_pm_ops lpuart_pm_ops = {
> +	SET_RUNTIME_PM_OPS(lpuart_runtime_suspend,
> +			   lpuart_runtime_resume, NULL)
>  	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(lpuart_suspend_noirq,
>  				      lpuart_resume_noirq)
>  	SET_SYSTEM_SLEEP_PM_OPS(lpuart_suspend, lpuart_resume)
> 

RE: [PATCH 2/2] tty: serial: fsl_lpuart: Add runtime pm support

[Sherry Sun]

> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Sent: 2022年11月9日 20:39
> To: Sherry Sun <sherry.sun@nxp.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Jiri Slaby
> <jirislaby@kernel.org>; linux-serial <linux-serial@vger.kernel.org>; LKML
> <linux-kernel@vger.kernel.org>; dl-linux-imx <linux-imx@nxp.com>
> Subject: Re: [PATCH 2/2] tty: serial: fsl_lpuart: Add runtime pm support
> 
> On Wed, 9 Nov 2022, Sherry Sun wrote:
> 
> > Add runtime pm support to manage the lpuart clock.
> >
> > Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> > ---
> >  drivers/tty/serial/fsl_lpuart.c | 60
> > +++++++++++++++++++++++++++++++++
> >  1 file changed, 60 insertions(+)
> >
> > diff --git a/drivers/tty/serial/fsl_lpuart.c
> > b/drivers/tty/serial/fsl_lpuart.c index 474943cb06b2..e678a7aaf7e4
> > 100644
> > --- a/drivers/tty/serial/fsl_lpuart.c
> > +++ b/drivers/tty/serial/fsl_lpuart.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/of_device.h>
> >  #include <linux/of_dma.h>
> >  #include <linux/pinctrl/consumer.h>
> > +#include <linux/pm_runtime.h>
> >  #include <linux/serial_core.h>
> >  #include <linux/slab.h>
> >  #include <linux/tty_flip.h>
> > @@ -235,6 +236,7 @@
> >
> >  /* Rx DMA timeout in ms, which is used to calculate Rx ring buffer size */
> >  #define DMA_RX_TIMEOUT		(10)
> > +#define UART_AUTOSUSPEND_TIMEOUT	3000
> >
> >  #define DRIVER_NAME	"fsl-lpuart"
> >  #define DEV_NAME	"ttyLP"
> > @@ -795,6 +797,20 @@ static void lpuart32_start_tx(struct uart_port
> *port)
> >  	}
> >  }
> >
> > +static void
> > +lpuart_uart_pm(struct uart_port *port, unsigned int state, unsigned
> > +int oldstate) {
> > +	switch (state) {
> > +	case UART_PM_STATE_OFF:
> > +		pm_runtime_mark_last_busy(port->dev);
> > +		pm_runtime_put_autosuspend(port->dev);
> > +		break;
> > +	default:
> > +		pm_runtime_get_sync(port->dev);
> > +		break;
> > +	}
> > +}
> > +
> >  /* return TIOCSER_TEMT when transmitter is not busy */  static
> > unsigned int lpuart_tx_empty(struct uart_port *port)  { @@ -2243,6
> > +2259,7 @@ static const struct uart_ops lpuart_pops = {
> >  	.startup	= lpuart_startup,
> >  	.shutdown	= lpuart_shutdown,
> >  	.set_termios	= lpuart_set_termios,
> > +	.pm		= lpuart_uart_pm,
> >  	.type		= lpuart_type,
> >  	.request_port	= lpuart_request_port,
> >  	.release_port	= lpuart_release_port,
> > @@ -2267,6 +2284,7 @@ static const struct uart_ops lpuart32_pops = {
> >  	.startup	= lpuart32_startup,
> >  	.shutdown	= lpuart32_shutdown,
> >  	.set_termios	= lpuart32_set_termios,
> > +	.pm		= lpuart_uart_pm,
> >  	.type		= lpuart_type,
> >  	.request_port	= lpuart_request_port,
> >  	.release_port	= lpuart_release_port,
> > @@ -2747,6 +2765,11 @@ static int lpuart_probe(struct platform_device
> *pdev)
> >  		handler = lpuart_int;
> >  	}
> >
> > +	pm_runtime_use_autosuspend(&pdev->dev);
> > +	pm_runtime_set_autosuspend_delay(&pdev->dev,
> UART_AUTOSUSPEND_TIMEOUT);
> > +	pm_runtime_set_active(&pdev->dev);
> > +	pm_runtime_enable(&pdev->dev);
> > +
> >  	ret = lpuart_global_reset(sport);
> >  	if (ret)
> >  		goto failed_reset;
> > @@ -2771,6 +2794,9 @@ static int lpuart_probe(struct platform_device
> > *pdev)
> >  failed_attach_port:
> >  failed_get_rs485:
> >  failed_reset:
> > +	pm_runtime_disable(&pdev->dev);
> > +	pm_runtime_set_suspended(&pdev->dev);
> > +	pm_runtime_dont_use_autosuspend(&pdev->dev);
> >  	lpuart_disable_clks(sport);
> >  	return ret;
> >  }
> > @@ -2789,9 +2815,30 @@ static int lpuart_remove(struct platform_device
> *pdev)
> >  	if (sport->dma_rx_chan)
> >  		dma_release_channel(sport->dma_rx_chan);
> >
> > +	pm_runtime_disable(&pdev->dev);
> > +	pm_runtime_set_suspended(&pdev->dev);
> > +	pm_runtime_dont_use_autosuspend(&pdev->dev);
> >  	return 0;
> >  }
> >
> > +static int __maybe_unused lpuart_runtime_suspend(struct device *dev)
> 
> IIRC, the PM_OPS macros contain special logic to make __maybe_unused
> unnecessary for these functions.
> 

HI Ilpo, you are right, seems use pm_ptr() can remove the need to mark the pm functions as __maybe_unused.
I will change this in V2 patch.

Best Regards
Sherry


> --
>  i.
> 
> 
> > +{
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	struct lpuart_port *sport = platform_get_drvdata(pdev);
> > +
> > +	lpuart_disable_clks(sport);
> > +
> > +	return 0;
> > +};
> > +
> > +static int __maybe_unused lpuart_runtime_resume(struct device *dev)
> > +{
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +	struct lpuart_port *sport = platform_get_drvdata(pdev);
> > +
> > +	return lpuart_enable_clks(sport);
> > +};
> > +
> >  static void serial_lpuart_enable_wakeup(struct lpuart_port *sport, bool on)
> >  {
> >  	unsigned int val, baud;
> > @@ -2937,6 +2984,10 @@ static int __maybe_unused
> lpuart_suspend(struct device *dev)
> >  			sport->dma_tx_in_progress = false;
> >  			dmaengine_terminate_all(sport->dma_tx_chan);
> >  		}
> > +	} else if (pm_runtime_active(sport->port.dev)) {
> > +		lpuart_disable_clks(sport);
> > +		pm_runtime_disable(sport->port.dev);
> > +		pm_runtime_set_suspended(sport->port.dev);
> >  	}
> >
> >  	return 0;
> > @@ -2971,12 +3022,19 @@ static void lpuart_console_fixup(struct
> lpuart_port *sport)
> >  static int __maybe_unused lpuart_resume(struct device *dev)
> >  {
> >  	struct lpuart_port *sport = dev_get_drvdata(dev);
> > +	int ret;
> >
> >  	if (lpuart_uport_is_active(sport)) {
> >  		if (lpuart_is_32(sport))
> >  			lpuart32_hw_setup(sport);
> >  		else
> >  			lpuart_hw_setup(sport);
> > +	} else if (pm_runtime_active(sport->port.dev)) {
> > +		ret = lpuart_enable_clks(sport);
> > +		if (ret)
> > +			return ret;
> > +		pm_runtime_set_active(sport->port.dev);
> > +		pm_runtime_enable(sport->port.dev);
> >  	}
> >
> >  	lpuart_console_fixup(sport);
> > @@ -2986,6 +3044,8 @@ static int __maybe_unused lpuart_resume(struct
> device *dev)
> >  }
> >
> >  static const struct dev_pm_ops lpuart_pm_ops = {
> > +	SET_RUNTIME_PM_OPS(lpuart_runtime_suspend,
> > +			   lpuart_runtime_resume, NULL)
> >  	SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(lpuart_suspend_noirq,
> >  				      lpuart_resume_noirq)
> >  	SET_SYSTEM_SLEEP_PM_OPS(lpuart_suspend, lpuart_resume)
> >