[PATCH 0/3] PM: domains: Avoid boilerplate code for DVFS in subsystem/drivers

[PATCH 0/3] PM: domains: Avoid boilerplate code for DVFS in subsystem/drivers

[Ulf Hansson]

Various discussions on LKML have pointed out that many subsystem/drivers for
devices that may be attached to a genpd and which manages DVFS/OPP though the
genpd performance states, would need very similar updates.

More precisely, they would likely have to call dev_pm_opp_set_rate|opp() to
drop and restore OPPs (which propagates upwards into performance states votes
in genpd), every time their devices should enter/exit a low power state, via
their device PM callbacks.

Rather than having to add the boilerplate code for these things into the
subsystems/drivers, this series implements the logic internally into genpd.

Concerns have been raised about this approach, mostly by myself, around that it
limits flexibility. On the other hand, it starts to look like more and more
people are requesting this to be manged internally in genpd, for good reasons.
So, I think it's worth to give this a try.

In the long run, if it turns out that the flexibility was indeed needed, we can
always deal with that as special cases on top.

Test and reviews are of course greatly appreciated!

Kind regards
Ulf Hansson

Ulf Hansson (3):
  PM: domains: Split code in dev_pm_genpd_set_performance_state()
  PM: domains: Drop/restore performance state votes for devices at
    runtime PM
  PM: domains: Drop/restore performance state votes for devices at
    system PM

 drivers/base/power/domain.c | 66 +++++++++++++++++++++++++++++--------
 include/linux/pm_domain.h   |  2 ++
 2 files changed, 54 insertions(+), 14 deletions(-)

-- 
2.25.1

[PATCH 1/3] PM: domains: Split code in dev_pm_genpd_set_performance_state()

[Ulf Hansson]

To prepare some of the code in dev_pm_genpd_set_performance_state() to be
re-used from subsequent changes, let's split it up into two functions.

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

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index b6a782c31613..a3b6e751f366 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -379,6 +379,24 @@ static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
 	return ret;
 }
 
+static int genpd_set_performance_state(struct device *dev, unsigned int state)
+{
+	struct generic_pm_domain *genpd = dev_to_genpd(dev);
+	struct generic_pm_domain_data *gpd_data = dev_gpd_data(dev);
+	unsigned int prev_state;
+	int ret;
+
+	prev_state = gpd_data->performance_state;
+	gpd_data->performance_state = state;
+
+	state = _genpd_reeval_performance_state(genpd, state);
+	ret = _genpd_set_performance_state(genpd, state, 0);
+	if (ret)
+		gpd_data->performance_state = prev_state;
+
+	return ret;
+}
+
 /**
  * dev_pm_genpd_set_performance_state- Set performance state of device's power
  * domain.
@@ -397,8 +415,6 @@ static int _genpd_set_performance_state(struct generic_pm_domain *genpd,
 int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
 {
 	struct generic_pm_domain *genpd;
-	struct generic_pm_domain_data *gpd_data;
-	unsigned int prev;
 	int ret;
 
 	genpd = dev_to_genpd_safe(dev);
@@ -410,16 +426,7 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
 		return -EINVAL;
 
 	genpd_lock(genpd);
-
-	gpd_data = to_gpd_data(dev->power.subsys_data->domain_data);
-	prev = gpd_data->performance_state;
-	gpd_data->performance_state = state;
-
-	state = _genpd_reeval_performance_state(genpd, state);
-	ret = _genpd_set_performance_state(genpd, state, 0);
-	if (ret)
-		gpd_data->performance_state = prev;
-
+	ret = genpd_set_performance_state(dev, state);
 	genpd_unlock(genpd);
 
 	return ret;
-- 
2.25.1

[PATCH 2/3] PM: domains: Drop/restore performance state votes for devices at runtime PM

[Ulf Hansson]

A subsystem/driver that need to manage OPPs for its device, should
typically drop its vote for the OPP when the device becomes runtime
suspended. In this way, the corresponding aggregation of the performance
state votes that is managed in genpd for the attached PM domain, may find
that the aggregated vote can be decreased. Hence, it may allow genpd to set
the lower performance state for the PM domain, thus avoiding to waste
energy.

To accomplish this, typically a subsystem/driver would need to call
dev_pm_opp_set_rate|opp() for its device from its ->runtime_suspend()
callback, to drop the vote for the OPP. Accordingly, it needs another call
to dev_pm_opp_set_rate|opp() to restore the vote for the OPP from its
->runtime_resume() callback.

To avoid boilerplate code in subsystems/driver to deal with these things,
let's instead manage this internally in genpd.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 21 +++++++++++++++++++--
 include/linux/pm_domain.h   |  1 +
 2 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index a3b6e751f366..81b9d4652b90 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -397,6 +397,18 @@ static int genpd_set_performance_state(struct device *dev, unsigned int state)
 	return ret;
 }
 
+static int genpd_drop_performance_state(struct device *dev)
+{
+	struct generic_pm_domain_data *gpd_data = dev_gpd_data(dev);
+	unsigned int prev_state;
+
+	prev_state = gpd_data->performance_state;
+	if (prev_state && !genpd_set_performance_state(dev, 0))
+		return prev_state;
+
+	return 0;
+}
+
 /**
  * dev_pm_genpd_set_performance_state- Set performance state of device's power
  * domain.
@@ -839,7 +851,8 @@ static int genpd_runtime_suspend(struct device *dev)
 {
 	struct generic_pm_domain *genpd;
 	bool (*suspend_ok)(struct device *__dev);
-	struct gpd_timing_data *td = &dev_gpd_data(dev)->td;
+	struct generic_pm_domain_data *gpd_data = dev_gpd_data(dev);
+	struct gpd_timing_data *td = &gpd_data->td;
 	bool runtime_pm = pm_runtime_enabled(dev);
 	ktime_t time_start;
 	s64 elapsed_ns;
@@ -896,6 +909,7 @@ static int genpd_runtime_suspend(struct device *dev)
 		return 0;
 
 	genpd_lock(genpd);
+	gpd_data->rpm_saved_pstate = genpd_drop_performance_state(dev);
 	genpd_power_off(genpd, true, 0);
 	genpd_unlock(genpd);
 
@@ -913,7 +927,8 @@ static int genpd_runtime_suspend(struct device *dev)
 static int genpd_runtime_resume(struct device *dev)
 {
 	struct generic_pm_domain *genpd;
-	struct gpd_timing_data *td = &dev_gpd_data(dev)->td;
+	struct generic_pm_domain_data *gpd_data = dev_gpd_data(dev);
+	struct gpd_timing_data *td = &gpd_data->td;
 	bool runtime_pm = pm_runtime_enabled(dev);
 	ktime_t time_start;
 	s64 elapsed_ns;
@@ -937,6 +952,8 @@ static int genpd_runtime_resume(struct device *dev)
 
 	genpd_lock(genpd);
 	ret = genpd_power_on(genpd, 0);
+	if (!ret && gpd_data->rpm_saved_pstate)
+		genpd_set_performance_state(dev, gpd_data->rpm_saved_pstate);
 	genpd_unlock(genpd);
 
 	if (ret)
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index dfcfbcecc34b..c3d6c15788a3 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -198,6 +198,7 @@ struct generic_pm_domain_data {
 	struct notifier_block *power_nb;
 	int cpu;
 	unsigned int performance_state;
+	unsigned int rpm_saved_pstate;
 	ktime_t	next_wakeup;
 	void *data;
 };
-- 
2.25.1

[PATCH 3/3] PM: domains: Drop/restore performance state votes for devices at system PM

[Ulf Hansson]

Recent changes in genpd drops and restore performance state votes for
devices during runtime PM.

For the similar reasons, but to avoid the same kind of boilerplate code in
device PM callbacks for system sleep in subsystems/drivers, let's drop and
restore performance states votes in genpd for the attached devices during
system sleep.

Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/power/domain.c | 14 ++++++++++++++
 include/linux/pm_domain.h   |  1 +
 2 files changed, 15 insertions(+)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 81b9d4652b90..8487f1690deb 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -1162,6 +1162,7 @@ static int genpd_prepare(struct device *dev)
  */
 static int genpd_finish_suspend(struct device *dev, bool poweroff)
 {
+	struct generic_pm_domain_data *gpd_data = dev_gpd_data(dev);
 	struct generic_pm_domain *genpd;
 	int ret = 0;
 
@@ -1192,6 +1193,7 @@ static int genpd_finish_suspend(struct device *dev, bool poweroff)
 	}
 
 	genpd_lock(genpd);
+	gpd_data->pm_saved_pstate = genpd_drop_performance_state(dev);
 	genpd->suspended_count++;
 	genpd_sync_power_off(genpd, true, 0);
 	genpd_unlock(genpd);
@@ -1221,6 +1223,7 @@ static int genpd_suspend_noirq(struct device *dev)
  */
 static int genpd_resume_noirq(struct device *dev)
 {
+	struct generic_pm_domain_data *gpd_data = dev_gpd_data(dev);
 	struct generic_pm_domain *genpd;
 	int ret;
 
@@ -1236,6 +1239,8 @@ static int genpd_resume_noirq(struct device *dev)
 	genpd_lock(genpd);
 	genpd_sync_power_on(genpd, true, 0);
 	genpd->suspended_count--;
+	if (gpd_data->pm_saved_pstate)
+		genpd_set_performance_state(dev, gpd_data->pm_saved_pstate);
 	genpd_unlock(genpd);
 
 	if (genpd->dev_ops.stop && genpd->dev_ops.start &&
@@ -1331,6 +1336,7 @@ static int genpd_poweroff_noirq(struct device *dev)
  */
 static int genpd_restore_noirq(struct device *dev)
 {
+	struct generic_pm_domain_data *gpd_data = dev_gpd_data(dev);
 	struct generic_pm_domain *genpd;
 	int ret = 0;
 
@@ -1355,6 +1361,8 @@ static int genpd_restore_noirq(struct device *dev)
 	}
 
 	genpd_sync_power_on(genpd, true, 0);
+	if (gpd_data->pm_saved_pstate)
+		genpd_set_performance_state(dev, gpd_data->pm_saved_pstate);
 	genpd_unlock(genpd);
 
 	if (genpd->dev_ops.stop && genpd->dev_ops.start &&
@@ -1400,23 +1408,29 @@ static void genpd_complete(struct device *dev)
 static void genpd_switch_state(struct device *dev, bool suspend)
 {
 	struct generic_pm_domain *genpd;
+	struct generic_pm_domain_data *gpd_data;
 	bool use_lock;
 
 	genpd = dev_to_genpd_safe(dev);
 	if (!genpd)
 		return;
 
+	gpd_data = dev_gpd_data(dev);
+
 	use_lock = genpd_is_irq_safe(genpd);
 
 	if (use_lock)
 		genpd_lock(genpd);
 
 	if (suspend) {
+		gpd_data->pm_saved_pstate = genpd_drop_performance_state(dev);
 		genpd->suspended_count++;
 		genpd_sync_power_off(genpd, use_lock, 0);
 	} else {
 		genpd_sync_power_on(genpd, use_lock, 0);
 		genpd->suspended_count--;
+		if(gpd_data->pm_saved_pstate)
+			genpd_set_performance_state(dev, gpd_data->pm_saved_pstate);
 	}
 
 	if (use_lock)
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index c3d6c15788a3..3eb215ac8adf 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -199,6 +199,7 @@ struct generic_pm_domain_data {
 	int cpu;
 	unsigned int performance_state;
 	unsigned int rpm_saved_pstate;
+	unsigned int pm_saved_pstate;
 	ktime_t	next_wakeup;
 	void *data;
 };
-- 
2.25.1

Re: [PATCH 1/3] PM: domains: Split code in dev_pm_genpd_set_performance_state()

[Viresh Kumar]

On 02-06-21, 12:12, Ulf Hansson wrote:
> To prepare some of the code in dev_pm_genpd_set_performance_state() to be
> re-used from subsequent changes, let's split it up into two functions.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/base/power/domain.c | 31 +++++++++++++++++++------------
>  1 file changed, 19 insertions(+), 12 deletions(-)

Reviewed-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH 2/3] PM: domains: Drop/restore performance state votes for devices at runtime PM

[Viresh Kumar]

On 02-06-21, 12:12, Ulf Hansson wrote:
> A subsystem/driver that need to manage OPPs for its device, should
> typically drop its vote for the OPP when the device becomes runtime
> suspended. In this way, the corresponding aggregation of the performance
> state votes that is managed in genpd for the attached PM domain, may find
> that the aggregated vote can be decreased. Hence, it may allow genpd to set
> the lower performance state for the PM domain, thus avoiding to waste
> energy.
> 
> To accomplish this, typically a subsystem/driver would need to call
> dev_pm_opp_set_rate|opp() for its device from its ->runtime_suspend()
> callback, to drop the vote for the OPP. Accordingly, it needs another call
> to dev_pm_opp_set_rate|opp() to restore the vote for the OPP from its
> ->runtime_resume() callback.
> 
> To avoid boilerplate code in subsystems/driver to deal with these things,
> let's instead manage this internally in genpd.
> 
> Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> ---
>  drivers/base/power/domain.c | 21 +++++++++++++++++++--
>  include/linux/pm_domain.h   |  1 +
>  2 files changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index a3b6e751f366..81b9d4652b90 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -397,6 +397,18 @@ static int genpd_set_performance_state(struct device *dev, unsigned int state)
>  	return ret;
>  }
>  
> +static int genpd_drop_performance_state(struct device *dev)
> +{
> +	struct generic_pm_domain_data *gpd_data = dev_gpd_data(dev);
> +	unsigned int prev_state;
> +
> +	prev_state = gpd_data->performance_state;
> +	if (prev_state && !genpd_set_performance_state(dev, 0))

What about adding this prev_state check in
genpd_set_performance_state() itself ? We already have one for the
genpd in _genpd_set_performance_state(), why not one for the device ?

> +		return prev_state;
> +
> +	return 0;

Hmm, we will return 0 in case genpd_set_performance_state() fails,
which will make us set the state to 0 again on resume. Maybe add a
comment for this somewhere ?

> +}
> +
>  /**
>   * dev_pm_genpd_set_performance_state- Set performance state of device's power
>   * domain.
> @@ -839,7 +851,8 @@ static int genpd_runtime_suspend(struct device *dev)
>  {
>  	struct generic_pm_domain *genpd;
>  	bool (*suspend_ok)(struct device *__dev);
> -	struct gpd_timing_data *td = &dev_gpd_data(dev)->td;
> +	struct generic_pm_domain_data *gpd_data = dev_gpd_data(dev);
> +	struct gpd_timing_data *td = &gpd_data->td;
>  	bool runtime_pm = pm_runtime_enabled(dev);
>  	ktime_t time_start;
>  	s64 elapsed_ns;
> @@ -896,6 +909,7 @@ static int genpd_runtime_suspend(struct device *dev)
>  		return 0;
>  
>  	genpd_lock(genpd);
> +	gpd_data->rpm_saved_pstate = genpd_drop_performance_state(dev);
>  	genpd_power_off(genpd, true, 0);
>  	genpd_unlock(genpd);
>  
> @@ -913,7 +927,8 @@ static int genpd_runtime_suspend(struct device *dev)
>  static int genpd_runtime_resume(struct device *dev)
>  {
>  	struct generic_pm_domain *genpd;
> -	struct gpd_timing_data *td = &dev_gpd_data(dev)->td;
> +	struct generic_pm_domain_data *gpd_data = dev_gpd_data(dev);
> +	struct gpd_timing_data *td = &gpd_data->td;
>  	bool runtime_pm = pm_runtime_enabled(dev);
>  	ktime_t time_start;
>  	s64 elapsed_ns;
> @@ -937,6 +952,8 @@ static int genpd_runtime_resume(struct device *dev)
>  
>  	genpd_lock(genpd);
>  	ret = genpd_power_on(genpd, 0);
> +	if (!ret && gpd_data->rpm_saved_pstate)
> +		genpd_set_performance_state(dev, gpd_data->rpm_saved_pstate);
>  	genpd_unlock(genpd);
>  
>  	if (ret)
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index dfcfbcecc34b..c3d6c15788a3 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -198,6 +198,7 @@ struct generic_pm_domain_data {
>  	struct notifier_block *power_nb;
>  	int cpu;
>  	unsigned int performance_state;
> +	unsigned int rpm_saved_pstate;
>  	ktime_t	next_wakeup;
>  	void *data;
>  };
> -- 
> 2.25.1

-- 
viresh

Re: [PATCH 2/3] PM: domains: Drop/restore performance state votes for devices at runtime PM

[Viresh Kumar]

On 03-06-21, 08:04, Viresh Kumar wrote:
> On 02-06-21, 12:12, Ulf Hansson wrote:
> > A subsystem/driver that need to manage OPPs for its device, should
> > typically drop its vote for the OPP when the device becomes runtime
> > suspended. In this way, the corresponding aggregation of the performance
> > state votes that is managed in genpd for the attached PM domain, may find
> > that the aggregated vote can be decreased. Hence, it may allow genpd to set
> > the lower performance state for the PM domain, thus avoiding to waste
> > energy.
> > 
> > To accomplish this, typically a subsystem/driver would need to call
> > dev_pm_opp_set_rate|opp() for its device from its ->runtime_suspend()
> > callback, to drop the vote for the OPP. Accordingly, it needs another call
> > to dev_pm_opp_set_rate|opp() to restore the vote for the OPP from its
> > ->runtime_resume() callback.
> > 
> > To avoid boilerplate code in subsystems/driver to deal with these things,
> > let's instead manage this internally in genpd.
> > 
> > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > ---
> >  drivers/base/power/domain.c | 21 +++++++++++++++++++--
> >  include/linux/pm_domain.h   |  1 +
> >  2 files changed, 20 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index a3b6e751f366..81b9d4652b90 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -397,6 +397,18 @@ static int genpd_set_performance_state(struct device *dev, unsigned int state)
> >  	return ret;
> >  }
> >  
> > +static int genpd_drop_performance_state(struct device *dev)
> > +{
> > +	struct generic_pm_domain_data *gpd_data = dev_gpd_data(dev);
> > +	unsigned int prev_state;
> > +
> > +	prev_state = gpd_data->performance_state;
> > +	if (prev_state && !genpd_set_performance_state(dev, 0))
> 
> What about adding this prev_state check in
> genpd_set_performance_state() itself ? We already have one for the
> genpd in _genpd_set_performance_state(), why not one for the device ?
> 
> > +		return prev_state;
> > +
> > +	return 0;
> 
> Hmm, we will return 0 in case genpd_set_performance_state() fails,
> which will make us set the state to 0 again on resume. Maybe add a
> comment for this somewhere ?

No, we won't as you check for rpm_saved_pstate there, so the device
will stay disabled.

Again adding the check into genpd_set_performance_state() may help
reducing similar checks elsewhere.

> > +}
> > +
> >  /**
> >   * dev_pm_genpd_set_performance_state- Set performance state of device's power
> >   * domain.
> > @@ -839,7 +851,8 @@ static int genpd_runtime_suspend(struct device *dev)
> >  {
> >  	struct generic_pm_domain *genpd;
> >  	bool (*suspend_ok)(struct device *__dev);
> > -	struct gpd_timing_data *td = &dev_gpd_data(dev)->td;
> > +	struct generic_pm_domain_data *gpd_data = dev_gpd_data(dev);
> > +	struct gpd_timing_data *td = &gpd_data->td;
> >  	bool runtime_pm = pm_runtime_enabled(dev);
> >  	ktime_t time_start;
> >  	s64 elapsed_ns;
> > @@ -896,6 +909,7 @@ static int genpd_runtime_suspend(struct device *dev)
> >  		return 0;
> >  
> >  	genpd_lock(genpd);
> > +	gpd_data->rpm_saved_pstate = genpd_drop_performance_state(dev);
> >  	genpd_power_off(genpd, true, 0);
> >  	genpd_unlock(genpd);
> >  
> > @@ -913,7 +927,8 @@ static int genpd_runtime_suspend(struct device *dev)
> >  static int genpd_runtime_resume(struct device *dev)
> >  {
> >  	struct generic_pm_domain *genpd;
> > -	struct gpd_timing_data *td = &dev_gpd_data(dev)->td;
> > +	struct generic_pm_domain_data *gpd_data = dev_gpd_data(dev);
> > +	struct gpd_timing_data *td = &gpd_data->td;
> >  	bool runtime_pm = pm_runtime_enabled(dev);
> >  	ktime_t time_start;
> >  	s64 elapsed_ns;
> > @@ -937,6 +952,8 @@ static int genpd_runtime_resume(struct device *dev)
> >  
> >  	genpd_lock(genpd);
> >  	ret = genpd_power_on(genpd, 0);
> > +	if (!ret && gpd_data->rpm_saved_pstate)
> > +		genpd_set_performance_state(dev, gpd_data->rpm_saved_pstate);
> >  	genpd_unlock(genpd);

-- 
viresh

Re: [PATCH 2/3] PM: domains: Drop/restore performance state votes for devices at runtime PM

[Ulf Hansson]

On Thu, 3 Jun 2021 at 04:37, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 03-06-21, 08:04, Viresh Kumar wrote:
> > On 02-06-21, 12:12, Ulf Hansson wrote:
> > > A subsystem/driver that need to manage OPPs for its device, should
> > > typically drop its vote for the OPP when the device becomes runtime
> > > suspended. In this way, the corresponding aggregation of the performance
> > > state votes that is managed in genpd for the attached PM domain, may find
> > > that the aggregated vote can be decreased. Hence, it may allow genpd to set
> > > the lower performance state for the PM domain, thus avoiding to waste
> > > energy.
> > >
> > > To accomplish this, typically a subsystem/driver would need to call
> > > dev_pm_opp_set_rate|opp() for its device from its ->runtime_suspend()
> > > callback, to drop the vote for the OPP. Accordingly, it needs another call
> > > to dev_pm_opp_set_rate|opp() to restore the vote for the OPP from its
> > > ->runtime_resume() callback.
> > >
> > > To avoid boilerplate code in subsystems/driver to deal with these things,
> > > let's instead manage this internally in genpd.
> > >
> > > Signed-off-by: Ulf Hansson <ulf.hansson@linaro.org>
> > > ---
> > >  drivers/base/power/domain.c | 21 +++++++++++++++++++--
> > >  include/linux/pm_domain.h   |  1 +
> > >  2 files changed, 20 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > > index a3b6e751f366..81b9d4652b90 100644
> > > --- a/drivers/base/power/domain.c
> > > +++ b/drivers/base/power/domain.c
> > > @@ -397,6 +397,18 @@ static int genpd_set_performance_state(struct device *dev, unsigned int state)
> > >     return ret;
> > >  }
> > >
> > > +static int genpd_drop_performance_state(struct device *dev)
> > > +{
> > > +   struct generic_pm_domain_data *gpd_data = dev_gpd_data(dev);
> > > +   unsigned int prev_state;
> > > +
> > > +   prev_state = gpd_data->performance_state;
> > > +   if (prev_state && !genpd_set_performance_state(dev, 0))
> >
> > What about adding this prev_state check in
> > genpd_set_performance_state() itself ? We already have one for the
> > genpd in _genpd_set_performance_state(), why not one for the device ?
> >
> > > +           return prev_state;
> > > +
> > > +   return 0;
> >
> > Hmm, we will return 0 in case genpd_set_performance_state() fails,
> > which will make us set the state to 0 again on resume. Maybe add a
> > comment for this somewhere ?
>
> No, we won't as you check for rpm_saved_pstate there, so the device
> will stay disabled.
>
> Again adding the check into genpd_set_performance_state() may help
> reducing similar checks elsewhere.

Yes, at closer look, I think it makes sense to me as well.

Although, as it means a change in behaviour, I decided to make it a
separate patch. Let me respin the series to fold it in.

[...]

Thanks for reviewing!

Kind regards
Uffe