diff mbox series

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

Message ID 20211005155923.173399-7-marcan@marcan.st (mailing list archive)
State Not Applicable
Headers show
Series Apple SoC PMGR device power states driver | expand

Commit Message

Hector Martin Oct. 5, 2021, 3:59 p.m. UTC
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(-)

Comments

Krzysztof Kozlowski Oct. 6, 2021, 7:43 a.m. UTC | #1
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
Rafael J. Wysocki Oct. 6, 2021, 1:25 p.m. UTC | #2
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
Krzysztof Kozlowski Oct. 6, 2021, 1:29 p.m. UTC | #3
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
Hector Martin Oct. 11, 2021, 5:32 a.m. UTC | #4
On 06/10/2021 16.43, Krzysztof Kozlowski wrote:
> On 05/10/2021 17:59, Hector Martin wrote:
>> +	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).

There are none though, this function always returns success past this point.

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

As Rafael mentioned, this is basically disabling PM so the device is 
enabled when not bound (which seems to be expected behavior). Not sure 
what I'd do if the resume fails... this is the remove path after all, 
it's not like we're doing anything else with the device at this 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?

Good question, I'm not entirely sure why these code paths have a check 
for NULL there. They were already there, do you happen to know why? To 
me it sounds like remove would only be called if probe succeeds, at 
which point drvdata should always be set.
Krzysztof Kozlowski Oct. 11, 2021, 6:48 a.m. UTC | #5
On 11/10/2021 07:32, Hector Martin wrote:
>>> +
>>>   		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?
> 
> Good question, I'm not entirely sure why these code paths have a check 
> for NULL there. They were already there, do you happen to know why? To 
> me it sounds like remove would only be called if probe succeeds, at 
> which point drvdata should always be set.
> 

Exactly, anyway it is not part of your patch, so no problem.


Best regards,
Krzysztof
Johan Hovold Oct. 11, 2021, 8:27 a.m. UTC | #6
On Mon, Oct 11, 2021 at 02:32:29PM +0900, Hector Martin wrote:
> On 06/10/2021 16.43, Krzysztof Kozlowski wrote:
> > On 05/10/2021 17:59, Hector Martin wrote:

> >>   	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?
> 
> As Rafael mentioned, this is basically disabling PM so the device is 
> enabled when not bound (which seems to be expected behavior). Not sure 
> what I'd do if the resume fails... this is the remove path after all, 
> it's not like we're doing anything else with the device at this point.

You still need to disable the clocks before returning from remove().
Consider what happens when the driver is rebound otherwise.

Johan
diff mbox series

Patch

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);
+
 	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);
+
 		s3c24xx_serial_cpufreq_deregister(to_ourport(port));
 		uart_remove_one_port(&s3c24xx_uart_drv, port);
+
+		pm_runtime_disable(&dev->dev);
+		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)
 }
 
 /* UART power management code */
-#ifdef CONFIG_PM_SLEEP
-static int s3c24xx_serial_suspend(struct device *dev)
+
+static int __maybe_unused s3c24xx_serial_suspend(struct device *dev)
 {
 	struct uart_port *port = s3c24xx_dev_to_port(dev);
 
@@ -2330,7 +2352,7 @@  static int s3c24xx_serial_suspend(struct device *dev)
 	return 0;
 }
 
-static int s3c24xx_serial_resume(struct device *dev)
+static int __maybe_unused s3c24xx_serial_resume(struct device *dev)
 {
 	struct uart_port *port = s3c24xx_dev_to_port(dev);
 	struct s3c24xx_uart_port *ourport = to_ourport(port);
@@ -2350,7 +2372,7 @@  static int s3c24xx_serial_resume(struct device *dev)
 	return 0;
 }
 
-static int s3c24xx_serial_resume_noirq(struct device *dev)
+static int __maybe_unused s3c24xx_serial_resume_noirq(struct device *dev)
 {
 	struct uart_port *port = s3c24xx_dev_to_port(dev);
 	struct s3c24xx_uart_port *ourport = to_ourport(port);
@@ -2420,16 +2442,14 @@  static int s3c24xx_serial_resume_noirq(struct device *dev)
 }
 
 static const struct dev_pm_ops s3c24xx_serial_pm_ops = {
+#ifdef CONFIG_PM_SLEEP
 	.suspend = s3c24xx_serial_suspend,
 	.resume = s3c24xx_serial_resume,
 	.resume_noirq = s3c24xx_serial_resume_noirq,
+#endif
+	SET_RUNTIME_PM_OPS(s3c24xx_serial_runtime_suspend,
+			   s3c24xx_serial_runtime_resume, NULL)
 };
-#define SERIAL_SAMSUNG_PM_OPS	(&s3c24xx_serial_pm_ops)
-
-#else /* !CONFIG_PM_SLEEP */
-
-#define SERIAL_SAMSUNG_PM_OPS	NULL
-#endif /* CONFIG_PM_SLEEP */
 
 /* Console code */
 
@@ -2924,7 +2944,7 @@  static struct platform_driver samsung_serial_driver = {
 	.id_table	= s3c24xx_serial_driver_ids,
 	.driver		= {
 		.name	= "samsung-uart",
-		.pm	= SERIAL_SAMSUNG_PM_OPS,
+		.pm	= &s3c24xx_serial_pm_ops,
 		.of_match_table	= of_match_ptr(s3c24xx_uart_dt_match),
 	},
 };