Re: [PATCH v6 7/8] serial: qcom-geni: Enable PM runtime for serial driver
From: Praveen Talari
Date: Mon Jul 14 2025 - 12:26:17 EST
Hi Dmitry/Bjorn/Krzysztof,
Thank you for review.
On 7/1/2025 4:42 PM, Dmitry Baryshkov wrote:
On Mon, Jun 30, 2025 at 10:40:25AM +0530, Praveen Talari wrote:
Hi Bjorn,
Thank you for review.
On 6/17/2025 9:23 PM, Bjorn Andersson wrote:
On Fri, Jun 06, 2025 at 10:51:13PM +0530, Praveen Talari wrote:
Add Power Management (PM) runtime support to Qualcomm GENI
serial driver.
Doesn't this have impact on the behavior outside of your
project? Or is the transition from qcom_geni_serial_pm() to explicit
RPM merely moving code around?
Seems like this deserves to not be hidden in a middle of a patch series.
Introduce necessary callbacks and updates to ensure seamless
transitions between power states, enhancing overall power
efficiency.
This commit message fails to state why we need runtime PM support in the
driver.
Introduce PM runtime support to the Qualcomm GENI serial driver to enable
better power efficiency and modularity across diverse resource control
mechanisms, including Linux and firmware-managed systems.
As part of this enhancement, the existing qcom_geni_serial_pm() logic to
use standard PM runtime APIs such as pm_runtime_resume_and_get() and
pm_runtime_put_sync(). Power state transitions are now handled through
the geni_serial_resources_on() and geni_serial_resources_off() functions.
Is it fine?
Please guide me/correct me if needed
Please start by stating out the problem that you are trying to solve.
There is no actual issue description in your patch.
I hope this commit describes the actual problem.
The GENI serial driver currently handles power resource management
through calls to the statically defined geni_serial_resources_on() and
geni_serial_resources_off() functions. This approach reduces modularity
and limits support for platforms with diverse power management
mechanisms, including resource managed by firmware.
Improve modularity and enable better integration with platform-specific
power management, introduce support for runtime PM. Use
pm_runtime_resume_and_get() and pm_runtime_put_sync() within the
qcom_geni_serial_pm() callback to control resource power state
transitions based on UART power state changes.
Thanks,
Praveen Talari
Thanks,
Praveen Talari
Also, start your commit message with a problem description, per
https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@xxxxxxxxxx>
Signed-off-by: Praveen Talari <quic_ptalari@xxxxxxxxxxx>
---
v5 -> v6
- added reviewed-by tag in commit
- added __maybe_unused to PM callback functions to avoid
warnings of defined but not used
---
drivers/tty/serial/qcom_geni_serial.c | 33 +++++++++++++++++++++++----
1 file changed, 29 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/qcom_geni_serial.c b/drivers/tty/serial/qcom_geni_serial.c
index b6fa7dc9b1fb..3691340ce7e8 100644
--- a/drivers/tty/serial/qcom_geni_serial.c
+++ b/drivers/tty/serial/qcom_geni_serial.c
@@ -1686,10 +1686,10 @@ static void qcom_geni_serial_pm(struct uart_port *uport,
old_state = UART_PM_STATE_OFF;
if (new_state == UART_PM_STATE_ON && old_state == UART_PM_STATE_OFF)
- geni_serial_resources_on(uport);
+ pm_runtime_resume_and_get(uport->dev);
else if (new_state == UART_PM_STATE_OFF &&
old_state == UART_PM_STATE_ON)
- geni_serial_resources_off(uport);
+ pm_runtime_put_sync(uport->dev);
}
@@ -1827,9 +1827,11 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
return ret;
}
+ pm_runtime_enable(port->se.dev);
Any reason not to use devm_pm_runtime_enable() and avoid the
two pm_runtime_disable() below?
Regards,
Bjorn
+
ret = uart_add_one_port(drv, uport);
if (ret)
- return ret;
+ goto error;
if (port->wakeup_irq > 0) {
device_init_wakeup(&pdev->dev, true);
@@ -1839,11 +1841,15 @@ static int qcom_geni_serial_probe(struct platform_device *pdev)
device_init_wakeup(&pdev->dev, false);
ida_free(&port_ida, uport->line);
uart_remove_one_port(drv, uport);
- return ret;
+ goto error;
}
}
return 0;
+
+error:
+ pm_runtime_disable(port->se.dev);
+ return ret;
}
static void qcom_geni_serial_remove(struct platform_device *pdev)
@@ -1855,9 +1861,26 @@ static void qcom_geni_serial_remove(struct platform_device *pdev)
dev_pm_clear_wake_irq(&pdev->dev);
device_init_wakeup(&pdev->dev, false);
ida_free(&port_ida, uport->line);
+ pm_runtime_disable(port->se.dev);
uart_remove_one_port(drv, &port->uport);
}
+static int __maybe_unused qcom_geni_serial_runtime_suspend(struct device *dev)
+{
+ struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
+ struct uart_port *uport = &port->uport;
+
+ return geni_serial_resources_off(uport);
+}
+
+static int __maybe_unused qcom_geni_serial_runtime_resume(struct device *dev)
+{
+ struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
+ struct uart_port *uport = &port->uport;
+
+ return geni_serial_resources_on(uport);
+}
+
static int qcom_geni_serial_suspend(struct device *dev)
{
struct qcom_geni_serial_port *port = dev_get_drvdata(dev);
@@ -1901,6 +1924,8 @@ static const struct qcom_geni_device_data qcom_geni_uart_data = {
};
static const struct dev_pm_ops qcom_geni_serial_pm_ops = {
+ SET_RUNTIME_PM_OPS(qcom_geni_serial_runtime_suspend,
+ qcom_geni_serial_runtime_resume, NULL)
SYSTEM_SLEEP_PM_OPS(qcom_geni_serial_suspend, qcom_geni_serial_resume)
};
--
2.17.1