[PATCH 0/2] tty: serial: uartlite: PM Runtime

[PATCH 0/2] tty: serial: uartlite: PM Runtime

[Shubhrajyoti Datta]

Add PM runtime support.
In case of errors disable clocks.
Add runtime PM support.

Shubhrajyoti Datta (2):
  tty: serial: uartlite: Disable clocks in case of errors
  tty: serial: uartlite: Add runtime pm support

 drivers/tty/serial/uartlite.c | 65 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 56 insertions(+), 9 deletions(-)

-- 
2.1.1

[PATCH 1/2] tty: serial: uartlite: Disable clocks in case of errors

[Shubhrajyoti Datta]

In case the uart registration fails the clocks are left enabled.
Disable the clock in case of errors.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
---
 drivers/tty/serial/uartlite.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
index f42ccc4..39ea495 100644
--- a/drivers/tty/serial/uartlite.c
+++ b/drivers/tty/serial/uartlite.c
@@ -799,7 +799,7 @@ static int ulite_probe(struct platform_device *pdev)
 		ret = uart_register_driver(&ulite_uart_driver);
 		if (ret < 0) {
 			dev_err(&pdev->dev, "Failed to register driver\n");
-			return ret;
+			goto err_out_clk_disable;
 		}
 	}
 
@@ -808,6 +808,10 @@ static int ulite_probe(struct platform_device *pdev)
 	clk_disable(pdata->clk);
 
 	return ret;
+
+err_out_clk_disable:
+	clk_disable_unprepare(pdata->clk);
+	return ret;
 }
 
 static int ulite_remove(struct platform_device *pdev)
-- 
2.1.1

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

[Shubhrajyoti Datta]

Add runtime pm support.

Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
---
 drivers/tty/serial/uartlite.c | 59 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 51 insertions(+), 8 deletions(-)

diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
index 39ea495..581689e 100644
--- a/drivers/tty/serial/uartlite.c
+++ b/drivers/tty/serial/uartlite.c
@@ -22,6 +22,7 @@
 #include <linux/of_device.h>
 #include <linux/of_platform.h>
 #include <linux/clk.h>
+#include <linux/pm_runtime.h>
 
 #define ULITE_NAME		"ttyUL"
 #define ULITE_MAJOR		204
@@ -54,6 +55,7 @@
 #define ULITE_CONTROL_RST_TX	0x01
 #define ULITE_CONTROL_RST_RX	0x02
 #define ULITE_CONTROL_IE	0x10
+#define UART_AUTOSUSPEND_TIMEOUT	3000
 
 /* Static pointer to console port */
 #ifdef CONFIG_SERIAL_UARTLITE_CONSOLE
@@ -390,12 +392,16 @@ static int ulite_verify_port(struct uart_port *port, struct serial_struct *ser)
 static void ulite_pm(struct uart_port *port, unsigned int state,
 		     unsigned int oldstate)
 {
-	struct uartlite_data *pdata = port->private_data;
+	int ret;
 
-	if (!state)
-		clk_enable(pdata->clk);
-	else
-		clk_disable(pdata->clk);
+	if (!state) {
+		ret = pm_runtime_get_sync(port->dev);
+		if (ret < 0)
+			dev_err(port->dev, "Failed to enable clocks\n");
+	} else {
+		pm_runtime_mark_last_busy(port->dev);
+		pm_runtime_put_autosuspend(port->dev);
+	}
 }
 
 #ifdef CONFIG_CONSOLE_POLL
@@ -734,11 +740,37 @@ static int __maybe_unused ulite_resume(struct device *dev)
 	return 0;
 }
 
+static int __maybe_unused ulite_runtime_suspend(struct device *dev)
+{
+	struct uart_port *port = dev_get_drvdata(dev);
+	struct uartlite_data *pdata = port->private_data;
+
+	clk_disable(pdata->clk);
+	return 0;
+};
+
+static int __maybe_unused ulite_runtime_resume(struct device *dev)
+{
+	struct uart_port *port = dev_get_drvdata(dev);
+	struct uartlite_data *pdata = port->private_data;
+	int ret;
+
+	ret = clk_enable(pdata->clk);
+	if (ret) {
+		dev_err(dev, "Cannot enable clock.\n");
+		return ret;
+	}
+	return 0;
+}
 /* ---------------------------------------------------------------------
  * Platform bus binding
  */
 
-static SIMPLE_DEV_PM_OPS(ulite_pm_ops, ulite_suspend, ulite_resume);
+static const struct dev_pm_ops ulite_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(ulite_suspend, ulite_resume)
+	SET_RUNTIME_PM_OPS(ulite_runtime_suspend,
+			   ulite_runtime_resume, NULL)
+};
 
 #if defined(CONFIG_OF)
 /* Match table for of_platform binding */
@@ -803,9 +835,15 @@ static int ulite_probe(struct platform_device *pdev)
 		}
 	}
 
+	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 = ulite_assign(&pdev->dev, id, res->start, irq, pdata);
 
-	clk_disable(pdata->clk);
+	pm_runtime_mark_last_busy(&pdev->dev);
+	pm_runtime_put_autosuspend(&pdev->dev);
 
 	return ret;
 
@@ -818,9 +856,14 @@ static int ulite_remove(struct platform_device *pdev)
 {
 	struct uart_port *port = dev_get_drvdata(&pdev->dev);
 	struct uartlite_data *pdata = port->private_data;
+	int rc;
 
 	clk_disable_unprepare(pdata->clk);
-	return ulite_release(&pdev->dev);
+	rc = ulite_release(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+	pm_runtime_set_suspended(&pdev->dev);
+	pm_runtime_dont_use_autosuspend(&pdev->dev);
+	return rc;
 }
 
 /* work with hotplug and coldplug */
-- 
2.1.1

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

[Greg KH]

On Tue, May 25, 2021 at 01:34:04PM +0530, Shubhrajyoti Datta wrote:
> Add runtime pm support.

We need more than a 4 word changelog text.

Please be more descriptive as to why you need this and what it is doing.

> +#define UART_AUTOSUSPEND_TIMEOUT	3000

Units?



>  
>  /* Static pointer to console port */
>  #ifdef CONFIG_SERIAL_UARTLITE_CONSOLE
> @@ -390,12 +392,16 @@ static int ulite_verify_port(struct uart_port *port, struct serial_struct *ser)
>  static void ulite_pm(struct uart_port *port, unsigned int state,
>  		     unsigned int oldstate)
>  {
> -	struct uartlite_data *pdata = port->private_data;
> +	int ret;
>  
> -	if (!state)
> -		clk_enable(pdata->clk);
> -	else
> -		clk_disable(pdata->clk);
> +	if (!state) {
> +		ret = pm_runtime_get_sync(port->dev);
> +		if (ret < 0)
> +			dev_err(port->dev, "Failed to enable clocks\n");
> +	} else {
> +		pm_runtime_mark_last_busy(port->dev);
> +		pm_runtime_put_autosuspend(port->dev);
> +	}
>  }
>  
>  #ifdef CONFIG_CONSOLE_POLL
> @@ -734,11 +740,37 @@ static int __maybe_unused ulite_resume(struct device *dev)
>  	return 0;
>  }
>  
> +static int __maybe_unused ulite_runtime_suspend(struct device *dev)
> +{
> +	struct uart_port *port = dev_get_drvdata(dev);
> +	struct uartlite_data *pdata = port->private_data;
> +
> +	clk_disable(pdata->clk);
> +	return 0;
> +};
> +
> +static int __maybe_unused ulite_runtime_resume(struct device *dev)
> +{
> +	struct uart_port *port = dev_get_drvdata(dev);
> +	struct uartlite_data *pdata = port->private_data;
> +	int ret;
> +
> +	ret = clk_enable(pdata->clk);
> +	if (ret) {
> +		dev_err(dev, "Cannot enable clock.\n");
> +		return ret;
> +	}
> +	return 0;
> +}
>  /* ---------------------------------------------------------------------

Blank line missing.

thanks,

greg k-h

Re: [PATCH 1/2] tty: serial: uartlite: Disable clocks in case of errors

[Greg KH]

On Tue, May 25, 2021 at 01:34:03PM +0530, Shubhrajyoti Datta wrote:
> In case the uart registration fails the clocks are left enabled.
> Disable the clock in case of errors.
> 
> Signed-off-by: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> ---
>  drivers/tty/serial/uartlite.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
> index f42ccc4..39ea495 100644
> --- a/drivers/tty/serial/uartlite.c
> +++ b/drivers/tty/serial/uartlite.c
> @@ -799,7 +799,7 @@ static int ulite_probe(struct platform_device *pdev)
>  		ret = uart_register_driver(&ulite_uart_driver);
>  		if (ret < 0) {
>  			dev_err(&pdev->dev, "Failed to register driver\n");
> -			return ret;
> +			goto err_out_clk_disable;
>  		}
>  	}
>  
> @@ -808,6 +808,10 @@ static int ulite_probe(struct platform_device *pdev)
>  	clk_disable(pdata->clk);
>  
>  	return ret;
> +
> +err_out_clk_disable:
> +	clk_disable_unprepare(pdata->clk);

Why not put this above in your error code?

Also, are you sure that you are calling uart_register_driver() in the
correct place?  Shouldn't everything be set up before it is called?  You
have more logic after it is called, what happens if your device is
opened before the uart_register_driver() call returns?

thanks,

greg k-h