[v6] Enable QUPs and Serial on SA8255p Qualcomm platforms

[PATCH v6 7/8] serial: qcom-geni: Enable PM runtime for serial driver

Posted by Praveen Talari 8 months, 1 week ago

Add Power Management (PM) runtime support to Qualcomm GENI
serial driver.

Introduce necessary callbacks and updates to ensure seamless
transitions between power states, enhancing overall power
efficiency.

Reviewed-by: Bryan O'Donoghue <bryan.odonoghue@linaro.org>
Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
---
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);
+
 	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

Re: [PATCH v6 7/8] serial: qcom-geni: Enable PM runtime for serial driver

Posted by Bjorn Andersson 7 months, 3 weeks ago

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.

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@linaro.org>
> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
> ---
> 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
>

Re: [PATCH v6 7/8] serial: qcom-geni: Enable PM runtime for serial driver

Posted by Praveen Talari 7 months, 2 weeks ago

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

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@linaro.org>
>> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
>> ---
>> 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
>>

Re: [PATCH v6 7/8] serial: qcom-geni: Enable PM runtime for serial driver

Posted by Dmitry Baryshkov 7 months, 1 week ago

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.

> 
> 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@linaro.org>
> > > Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
> > > ---
> > > 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
> > > 

-- 
With best wishes
Dmitry

Re: [PATCH v6 7/8] serial: qcom-geni: Enable PM runtime for serial driver

Posted by Praveen Talari 7 months ago

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@linaro.org>
>>>> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
>>>> ---
>>>> 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
>>>>
>

Re: [PATCH v6 7/8] serial: qcom-geni: Enable PM runtime for serial driver

Posted by Dmitry Baryshkov 6 months, 4 weeks ago

On Mon, 14 Jul 2025 at 19:21, Praveen Talari <quic_ptalari@quicinc.com> wrote:
>
> 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.

LGTM

>
> 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@linaro.org>
> >>>> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
> >>>> ---
> >>>> 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
> >>>>
> >



-- 
With best wishes
Dmitry

Re: [PATCH v6 7/8] serial: qcom-geni: Enable PM runtime for serial driver

Posted by Krzysztof Kozlowski 7 months, 2 weeks ago

On 30/06/2025 07:10, 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.


No, not better, that's some marketing blob without saying how this power
efficiency is visible/observable.

Best regards,
Krzysztof

Re: [PATCH v6 7/8] serial: qcom-geni: Enable PM runtime for serial driver

Posted by Praveen Talari 7 months, 3 weeks ago

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?
There is no impact on functionality and behavior with this change.

The transition is purely refactor.

Using PM runtime APIs such as
pm_runtime_resume_and_get() and pm_runtime_put_sync() to manage resource 
both locally and firmware.

> 
> Seems like this deserves to not be hidden in a middle of a patch series.
This depends on refactored patches.
> 
>> 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.
Sure, will update commit text.
> 
> 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@linaro.org>
>> Signed-off-by: Praveen Talari <quic_ptalari@quicinc.com>
>> ---
>> 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?

I agree, will update in next patch-set.
> 
> 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
>>