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

["Rafael J. Wysocki" <rafael@kernel.org>]

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?

> 2. Why do you need to resume the device here?

This appears to be to prevent the device from suspending after the given point.

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