serial: 8250_omap: Drop pm_runtime_irq_safe()

PATCH In review Tony Lindgren 2 participants 2 messages Oct 4, 2023 – Oct 5, 2023 TTY layer and serial drivers

Share & tools

Tony Lindgren

author

2023-10-04 06:26 UTC #0 lore raw

PATCH v2 1/1 serial: 8250_omap: Drop pm_runtime_irq_safe()

Let's drop the use of pm_runtime_irq_safe() for 8250_omap. The use of pm_runtime_irq_safe() is not nice as it takes a permanent usage count on the parent device.

We can finally drop pm_runtime_irq_safe() safely as the kernel now knows when the uart port tx is active. This changed with commit 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM").

For serial port rx, we already use Linux generic wakeirqs for 8250_omap.

To drop pm_runtime_irq_safe(), we need to add handling for shallow idle state where the port hardware may already be awake and an IO interrupt happens. We also need to replace the serial8250_rpm sync calls in the interrupt handlers with async runtime PM calls.

Note that omap8250_irq() calls omap_8250_dma_handle_irq(), so we don't need separate runtime PM calls in omap_8250_dma_handle_irq().

While at it, let's also add the missing line break to the end of omap8250_runtime_resume() to group the calls.

Signed-off-by Tony Lindgren <[email protected]>

---

Changes since v1:

- Fix build warning if CONFIG_SERIAL_8250_DMA is not set as reported by kernel test robot

---
 drivers/tty/serial/8250/8250_omap.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

drivers/tty/serial/8250/8250_omap.c +21 -8

@@ -8,6 +8,7 @@
  *
  */
 
+#include <linux/atomic.h>
 #include <linux/clk.h>
 #include <linux/device.h>
 #include <linux/io.h>

@@ -130,6 +131,7 @@ struct omap8250_priv {
 
 	u8 tx_trigger;
 	u8 rx_trigger;
+	atomic_t active;
 	bool is_suspending;
 	int wakeirq;
 	int wakeups_enabled;

@@ -632,14 +634,23 @@ static irqreturn_t omap8250_irq(int irq, void *dev_id)
 	unsigned int iir, lsr;
 	int ret;
 
+	pm_runtime_get_noresume(port->dev);
+
+	/* Shallow idle state wake-up to an IO interrupt? */
+	if (atomic_add_unless(&priv->active, 1, 1)) {
+		priv->latency = priv->calc_latency;
+		schedule_work(&priv->qos_work);
+	}
+
 #ifdef CONFIG_SERIAL_8250_DMA
 	if (up->dma) {
 		ret = omap_8250_dma_handle_irq(port);
+		pm_runtime_mark_last_busy(port->dev);
+		pm_runtime_put(port->dev);
 		return IRQ_RETVAL(ret);
 	}
 #endif
 
-	serial8250_rpm_get(up);
 	lsr = serial_port_in(port, UART_LSR);
 	iir = serial_port_in(port, UART_IIR);
 	ret = serial8250_handle_irq(port, iir);

@@ -676,7 +687,8 @@ static irqreturn_t omap8250_irq(int irq, void *dev_id)
 		schedule_delayed_work(&up->overrun_backoff, delay);
 	}
 
-	serial8250_rpm_put(up);
+	pm_runtime_mark_last_busy(port->dev);
+	pm_runtime_put(port->dev);
 
 	return IRQ_RETVAL(ret);
 }

@@ -1270,11 +1282,8 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
 	u16 status;
 	u8 iir;
 
-	serial8250_rpm_get(up);
-
 	iir = serial_port_in(port, UART_IIR);
 	if (iir & UART_IIR_NO_INT) {
-		serial8250_rpm_put(up);
 		return IRQ_HANDLED;
 	}

@@ -1305,7 +1314,6 @@ static int omap_8250_dma_handle_irq(struct uart_port *port)
 
 	uart_unlock_and_check_sysrq(port);
 
-	serial8250_rpm_put(up);
 	return 1;
 }

@@ -1503,8 +1511,6 @@ static int omap8250_probe(struct platform_device *pdev)
 	if (!of_get_available_child_count(pdev->dev.of_node))
 		pm_runtime_set_autosuspend_delay(&pdev->dev, -1);
 
-	pm_runtime_irq_safe(&pdev->dev);
-
 	pm_runtime_get_sync(&pdev->dev);
 
 	omap_serial_fill_features_erratas(&up, priv);

@@ -1743,6 +1749,7 @@ static int omap8250_runtime_suspend(struct device *dev)
 
 	priv->latency = PM_QOS_CPU_LATENCY_DEFAULT_VALUE;
 	schedule_work(&priv->qos_work);
+	atomic_set(&priv->active, 0);
 
 	return 0;
 }

@@ -1752,6 +1759,10 @@ static int omap8250_runtime_resume(struct device *dev)
 	struct omap8250_priv *priv = dev_get_drvdata(dev);
 	struct uart_8250_port *up = NULL;
 
+	/* Did the hardware wake to a device IO interrupt before a wakeirq? */
+	if (atomic_read(&priv->active))
+		return 0;
+
 	if (priv->line >= 0)
 		up = serial8250_get_port(priv->line);

@@ -1767,8 +1778,10 @@ static int omap8250_runtime_resume(struct device *dev)
 		uart_port_unlock_irq(&up->port);
 	}
 
+	atomic_set(&priv->active, 1);
 	priv->latency = priv->calc_latency;
 	schedule_work(&priv->qos_work);
+
 	return 0;
 }

Signature · 1 line

2.42.0

Andy Shevchenko

Reviewed-by on v2 1/1

2023-10-05 14:25 UTC #1 lore raw

Quoted 15 lines · Tony Lindgren

Let's drop the use of pm_runtime_irq_safe() for 8250_omap. The use of pm_runtime_irq_safe() is not nice as it takes a permanent usage count on the parent device.
We can finally drop pm_runtime_irq_safe() safely as the kernel now knows when the uart port tx is active. This changed with commit 84a9582fd203 ("serial: core: Start managing serial controllers to enable runtime PM").
For serial port rx, we already use Linux generic wakeirqs for 8250_omap.
To drop pm_runtime_irq_safe(), we need to add handling for shallow idle state where the port hardware may already be awake and an IO interrupt happens. We also need to replace the serial8250_rpm sync calls in the interrupt handlers with async runtime PM calls.

Quoted 5 lines

Note that omap8250_irq() calls omap_8250_dma_handle_irq(), so we don't need separate runtime PM calls in omap_8250_dma_handle_irq().
While at it, let's also add the missing line break to the end of omap8250_runtime_resume() to group the calls.

Yoo-hoo! A few years only to get to the point :-)

Reviewed-by Andy Shevchenko <[email protected]>

Signature · 2 lines

With Best Regards,
Andy Shevchenko