On Wed, Oct 6, 2021 at 9:44 AM Krzysztof Kozlowski
<krzysztof.kozlowski@xxxxxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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
