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

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

On 06/10/2021 15:25, Rafael J. Wysocki wrote:
> On Wed, Oct 6, 2021 at 9:44 AM Krzysztof Kozlowski
> <krzysztof.kozlowski@canonical.com> wrote:
>>
>> 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.
> 
> Why?  What can be done differently if the resume fails?

The answer is connected with the point (2) below. If there were for
example register access here, maybe it should be simply skipped to avoid
imprecise abort...

> 
>> 2. Why do you need to resume the device here?
> 
> This appears to be to prevent the device from suspending after the given point.

Makes sense.

> 
>>> +
>>>               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


Best regards,
Krzysztof