[PATCH 0/2] PM / Domains: Initial improvements of system PM code in genpd

[PATCH 0/2] PM / Domains: Initial improvements of system PM code in genpd

[Ulf Hansson]

The system PM code in genpd needs a round of modernization and clean-up. This is
only the first initial part.

I intend to in step by step improve the code, so in a later point enable to do
optimizations. Such as, avoid waking up devices unless it's really needed.

Ulf Hansson (2):
  PM / Domains: Remove redundant pm_runtime_get|put*() in
    pm_genpd_prepare()
  PM / Domains: Drop unnecessary wakeup code from pm_genpd_prepare()

 drivers/base/power/domain.c | 14 +-------------
 1 file changed, 1 insertion(+), 13 deletions(-)

-- 
1.9.1

[PATCH 1/2] PM / Domains: Remove redundant pm_runtime_get|put*() in pm_genpd_prepare()

[Ulf Hansson]

The PM core increases the runtime PM usage count in the system PM prepare
phase. This makes some of the pm_runtime_get|put*() calls in
pm_gendp_prepare() redundant, so let's remove them.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 4ce4ce0..60a3573 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -730,14 +730,11 @@ static int pm_genpd_prepare(struct device *dev)
 	 * at this point and a system wakeup event should be reported if it's
 	 * set up to wake up the system from sleep states.
 	 */
-	pm_runtime_get_noresume(dev);
 	if (pm_runtime_barrier(dev) && device_may_wakeup(dev))
 		pm_wakeup_event(dev, 0);
 
-	if (pm_wakeup_pending()) {
-		pm_runtime_put(dev);
+	if (pm_wakeup_pending())
 		return -EBUSY;
-	}
 
 	if (resume_needed(dev, genpd))
 		pm_runtime_resume(dev);
@@ -751,10 +748,8 @@ static int pm_genpd_prepare(struct device *dev)
 
 	mutex_unlock(&genpd->lock);
 
-	if (genpd->suspend_power_off) {
-		pm_runtime_put_noidle(dev);
+	if (genpd->suspend_power_off)
 		return 0;
-	}
 
 	/*
 	 * The PM domain must be in the GPD_STATE_ACTIVE state at this point,
@@ -776,7 +771,6 @@ static int pm_genpd_prepare(struct device *dev)
 		pm_runtime_enable(dev);
 	}
 
-	pm_runtime_put(dev);
 	return ret;
 }
 
-- 
1.9.1

Re: [PATCH 1/2] PM / Domains: Remove redundant pm_runtime_get|put*() in pm_genpd_prepare()

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> The PM core increases the runtime PM usage count in the system PM prepare
> phase.

nit: increases/decreases ... prepare/complete

> This makes some of the pm_runtime_get|put*() calls in
> pm_gendp_prepare() redundant, so let's remove them.

s/gendp/genpd/

And also add pm_genpd_complete()

> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>

Otherwise...

Reviewed-by: Kevin Hilman <khilman@baylibre.com>

> ---
>  drivers/base/power/domain.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 4ce4ce0..60a3573 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -730,14 +730,11 @@ static int pm_genpd_prepare(struct device *dev)
>  	 * at this point and a system wakeup event should be reported if it's
>  	 * set up to wake up the system from sleep states.
>  	 */
> -	pm_runtime_get_noresume(dev);
>  	if (pm_runtime_barrier(dev) && device_may_wakeup(dev))
>  		pm_wakeup_event(dev, 0);
>  
> -	if (pm_wakeup_pending()) {
> -		pm_runtime_put(dev);
> +	if (pm_wakeup_pending())
>  		return -EBUSY;
> -	}
>  
>  	if (resume_needed(dev, genpd))
>  		pm_runtime_resume(dev);
> @@ -751,10 +748,8 @@ static int pm_genpd_prepare(struct device *dev)
>  
>  	mutex_unlock(&genpd->lock);
>  
> -	if (genpd->suspend_power_off) {
> -		pm_runtime_put_noidle(dev);
> +	if (genpd->suspend_power_off)
>  		return 0;
> -	}
>  
>  	/*
>  	 * The PM domain must be in the GPD_STATE_ACTIVE state at this point,
> @@ -776,7 +771,6 @@ static int pm_genpd_prepare(struct device *dev)
>  		pm_runtime_enable(dev);
>  	}
>  
> -	pm_runtime_put(dev);
>  	return ret;
>  }

Re: [PATCH 1/2] PM / Domains: Remove redundant pm_runtime_get|put*() in pm_genpd_prepare()

[Ulf Hansson]

On 22 April 2016 at 20:43, Kevin Hilman <khilman@baylibre.com> wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
>
>> The PM core increases the runtime PM usage count in the system PM prepare
>> phase.
>
> nit: increases/decreases ... prepare/complete
>
>> This makes some of the pm_runtime_get|put*() calls in
>> pm_gendp_prepare() redundant, so let's remove them.
>
> s/gendp/genpd/
>
> And also add pm_genpd_complete()

As a matter fact that isn't the case. There's no pm_runtime_put*() in
pm_genpd_complete().

Although, perhaps you were thinking about enabling/disabling of
runtime PM, as that is being done in pm_genpd_prepare() and
pm_genpd_complete(). I intend to change as well, but in a separate
patch.

>
>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Otherwise...
>
> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
>

Thanks for reviewing, I will update the change log accordingly!

Kind regards
Uffe

>> ---
>>  drivers/base/power/domain.c | 10 ++--------
>>  1 file changed, 2 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index 4ce4ce0..60a3573 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -730,14 +730,11 @@ static int pm_genpd_prepare(struct device *dev)
>>        * at this point and a system wakeup event should be reported if it's
>>        * set up to wake up the system from sleep states.
>>        */
>> -     pm_runtime_get_noresume(dev);
>>       if (pm_runtime_barrier(dev) && device_may_wakeup(dev))
>>               pm_wakeup_event(dev, 0);
>>
>> -     if (pm_wakeup_pending()) {
>> -             pm_runtime_put(dev);
>> +     if (pm_wakeup_pending())
>>               return -EBUSY;
>> -     }
>>
>>       if (resume_needed(dev, genpd))
>>               pm_runtime_resume(dev);
>> @@ -751,10 +748,8 @@ static int pm_genpd_prepare(struct device *dev)
>>
>>       mutex_unlock(&genpd->lock);
>>
>> -     if (genpd->suspend_power_off) {
>> -             pm_runtime_put_noidle(dev);
>> +     if (genpd->suspend_power_off)
>>               return 0;
>> -     }
>>
>>       /*
>>        * The PM domain must be in the GPD_STATE_ACTIVE state at this point,
>> @@ -776,7 +771,6 @@ static int pm_genpd_prepare(struct device *dev)
>>               pm_runtime_enable(dev);
>>       }
>>
>> -     pm_runtime_put(dev);
>>       return ret;
>>  }

Re: [PATCH 1/2] PM / Domains: Remove redundant pm_runtime_get|put*() in pm_genpd_prepare()

[Rafael J. Wysocki]

On Monday, April 25, 2016 10:21:34 AM Ulf Hansson wrote:
> On 22 April 2016 at 20:43, Kevin Hilman <khilman@baylibre.com> wrote:
> > Ulf Hansson <ulf.hansson@linaro.org> writes:
> >
> >> The PM core increases the runtime PM usage count in the system PM prepare
> >> phase.
> >
> > nit: increases/decreases ... prepare/complete
> >
> >> This makes some of the pm_runtime_get|put*() calls in
> >> pm_gendp_prepare() redundant, so let's remove them.
> >
> > s/gendp/genpd/
> >
> > And also add pm_genpd_complete()
> 
> As a matter fact that isn't the case. There's no pm_runtime_put*() in
> pm_genpd_complete().
> 
> Although, perhaps you were thinking about enabling/disabling of
> runtime PM, as that is being done in pm_genpd_prepare() and
> pm_genpd_complete(). I intend to change as well, but in a separate
> patch.
> 
> >
> >> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> >
> > Otherwise...
> >
> > Reviewed-by: Kevin Hilman <khilman@baylibre.com>
> >
> 
> Thanks for reviewing, I will update the change log accordingly!

OK, I'll be expecting an update of the series then.

Thanks,
Rafael

Re: [PATCH 1/2] PM / Domains: Remove redundant pm_runtime_get|put*() in pm_genpd_prepare()

[Kevin Hilman]

Ulf Hansson <ulf.hansson@linaro.org> writes:

> On 22 April 2016 at 20:43, Kevin Hilman <khilman@baylibre.com> wrote:
>> Ulf Hansson <ulf.hansson@linaro.org> writes:
>>
>>> The PM core increases the runtime PM usage count in the system PM prepare
>>> phase.
>>
>> nit: increases/decreases ... prepare/complete
>>
>>> This makes some of the pm_runtime_get|put*() calls in
>>> pm_gendp_prepare() redundant, so let's remove them.
>>
>> s/gendp/genpd/
>>
>> And also add pm_genpd_complete()
>
> As a matter fact that isn't the case. There's no pm_runtime_put*() in
> pm_genpd_complete().
>
> Although, perhaps you were thinking about enabling/disabling of
> runtime PM, as that is being done in pm_genpd_prepare() and
> pm_genpd_complete(). I intend to change as well, but in a separate
> patch.

Actually, I was thinking of device_complete() where there is the _put()

>>
>>> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
>>
>> Otherwise...
>>
>> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
>>
>
> Thanks for reviewing, I will update the change log accordingly!

Thanks,

Kevin

[PATCH 2/2] PM / Domains: Drop unnecessary wakeup code from pm_genpd_prepare()

[Ulf Hansson]

As the PM core already have wakeup management during the system PM phase,
it seems reasonable that genpd and its users should be able to rely on
that. Therefore let's remove this from pm_genpd_prepare().

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 60a3573..de23b64 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -730,12 +730,6 @@ static int pm_genpd_prepare(struct device *dev)
 	 * at this point and a system wakeup event should be reported if it's
 	 * set up to wake up the system from sleep states.
 	 */
-	if (pm_runtime_barrier(dev) && device_may_wakeup(dev))
-		pm_wakeup_event(dev, 0);
-
-	if (pm_wakeup_pending())
-		return -EBUSY;
-
 	if (resume_needed(dev, genpd))
 		pm_runtime_resume(dev);
 
-- 
1.9.1