Re: [PATCH 6/7] tty: serial: samsung_tty: Support runtime PM

[Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>]

On 05/10/2021 17:59, Hector Martin wrote:
> This allows idle UART devices to be suspended using the standard
> runtime-PM framework. The logic is modeled after stm32-usart.
> 
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>  drivers/tty/serial/samsung_tty.c | 88 ++++++++++++++++++++------------
>  1 file changed, 54 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index e2f49863e9c2..d68e3341adc6 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -40,6 +40,7 @@
>  #include <linux/clk.h>
>  #include <linux/cpufreq.h>
>  #include <linux/of.h>
> +#include <linux/pm_runtime.h>
>  #include <asm/irq.h>
>  
>  /* UART name and device definitions */
> @@ -1381,31 +1382,49 @@ static void exynos_usi_init(struct uart_port *port)
>  
>  /* power power management control */
>  
> -static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
> -			      unsigned int old)
> +static int __maybe_unused s3c24xx_serial_runtime_suspend(struct device *dev)
>  {
> +	struct uart_port *port = dev_get_drvdata(dev);
>  	struct s3c24xx_uart_port *ourport = to_ourport(port);
>  	int timeout = 10000;
>  
> -	ourport->pm_level = level;
> +	while (--timeout && !s3c24xx_serial_txempty_nofifo(port))
> +		udelay(100);
>  
> -	switch (level) {
> -	case 3:
> -		while (--timeout && !s3c24xx_serial_txempty_nofifo(port))
> -			udelay(100);
> +	if (!IS_ERR(ourport->baudclk))
> +		clk_disable_unprepare(ourport->baudclk);
>  
> -		if (!IS_ERR(ourport->baudclk))
> -			clk_disable_unprepare(ourport->baudclk);
> +	clk_disable_unprepare(ourport->clk);
> +	return 0;
> +};
>  
> -		clk_disable_unprepare(ourport->clk);
> -		break;
> +static int __maybe_unused s3c24xx_serial_runtime_resume(struct device *dev)
> +{
> +	struct uart_port *port = dev_get_drvdata(dev);
> +	struct s3c24xx_uart_port *ourport = to_ourport(port);
>  
> -	case 0:
> -		clk_prepare_enable(ourport->clk);
> +	clk_prepare_enable(ourport->clk);
>  
> -		if (!IS_ERR(ourport->baudclk))
> -			clk_prepare_enable(ourport->baudclk);
> +	if (!IS_ERR(ourport->baudclk))
> +		clk_prepare_enable(ourport->baudclk);
> +	return 0;
> +};
> +
> +static void s3c24xx_serial_pm(struct uart_port *port, unsigned int level,
> +			      unsigned int old)
> +{
> +	struct s3c24xx_uart_port *ourport = to_ourport(port);
> +
> +	ourport->pm_level = level;
>  
> +	switch (level) {
> +	case UART_PM_STATE_OFF:
> +		pm_runtime_mark_last_busy(port->dev);
> +		pm_runtime_put_sync(port->dev);
> +		break;
> +
> +	case UART_PM_STATE_ON:
> +		pm_runtime_get_sync(port->dev);
>  		exynos_usi_init(port);
>  		break;
>  	default:
> @@ -2282,18 +2301,15 @@ static int s3c24xx_serial_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	pm_runtime_get_noresume(&pdev->dev);
> +	pm_runtime_set_active(&pdev->dev);
> +	pm_runtime_enable(&pdev->dev);
> +

You need to cleanup in error paths (put/disable).

>  	dev_dbg(&pdev->dev, "%s: adding port\n", __func__);
>  	uart_add_one_port(&s3c24xx_uart_drv, &ourport->port);
>  	platform_set_drvdata(pdev, &ourport->port);
>  
> -	/*
> -	 * Deactivate the clock enabled in s3c24xx_serial_init_port here,
> -	 * so that a potential re-enablement through the pm-callback overlaps
> -	 * and keeps the clock enabled in this case.
> -	 */
> -	clk_disable_unprepare(ourport->clk);
> -	if (!IS_ERR(ourport->baudclk))
> -		clk_disable_unprepare(ourport->baudclk);
> +	pm_runtime_put_sync(&pdev->dev);
>  
>  	ret = s3c24xx_serial_cpufreq_register(ourport);
>  	if (ret < 0)
> @@ -2309,8 +2325,14 @@ static int s3c24xx_serial_remove(struct platform_device *dev)
>  	struct uart_port *port = s3c24xx_dev_to_port(&dev->dev);
>  
>  	if (port) {
> +		pm_runtime_get_sync(&dev->dev);

1. You need to check return status.
2. Why do you need to resume the device here?

> +
>  		s3c24xx_serial_cpufreq_deregister(to_ourport(port));
>  		uart_remove_one_port(&s3c24xx_uart_drv, port);
> +
> +		pm_runtime_disable(&dev->dev);

Why disabling it only if port!=NULL? Can remove() be called if
platform_set_drvdata() was not?

> +		pm_runtime_set_suspended(&dev->dev);
> +		pm_runtime_put_noidle(&dev->dev);
>  	}
>  
>  	uart_unregister_driver(&s3c24xx_uart_drv);
> @@ -2319,8 +2341,8 @@ static int s3c24xx_serial_remove(struct platform_device *dev)
>  }
>  

Best regards,
Krzysztof