Messages in this thread | | | From | Ulf Hansson <> | Date | Thu, 3 Jun 2021 12:20:57 +0200 | Subject | Re: [PATCH v2 4/4] PM: domains: Drop/restore performance state votes for devices at system PM |
| |
+ Mark Brown, Dmitry Baryshkov
On Thu, 3 Jun 2021 at 11:34, Ulf Hansson <ulf.hansson@linaro.org> wrote: > > 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>
After a second thought, it looks like we maybe should defer to apply this final patch of the series. At least until we figured out how to address the below issue:
So, I noticed that we have things like "regulator-fixed-domain", that uses "required-opps" to enable/disable a regulator through the dev_pm_set_performance_state() interface. We likely don't want to drop the performance state internally in genpd when genpd_suspend_noirq() gets called, for the corresponding struct device for the regulator.
I guess if genpd should drop performance states like $subject patch suggest, we need some kind of additional coordination, that allows a subsystem/driver to inform genpd when it should avoid it. Or something along those lines.
Kind regards Uffe
> --- > > Changes in v2: > - Rebased. > - A few cosmetic changes. > > --- > drivers/base/power/domain.c | 9 +++++++++ > include/linux/pm_domain.h | 1 + > 2 files changed, 10 insertions(+) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index e5d97174c254..a33e5b341f3f 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1171,6 +1171,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; > > @@ -1201,6 +1202,7 @@ static int genpd_finish_suspend(struct device *dev, bool poweroff) > } > > genpd_lock(genpd); > + gpd_data->pm_pstate = genpd_drop_performance_state(dev); > genpd->suspended_count++; > genpd_sync_power_off(genpd, true, 0); > genpd_unlock(genpd); > @@ -1245,6 +1247,7 @@ static int genpd_resume_noirq(struct device *dev) > genpd_lock(genpd); > genpd_sync_power_on(genpd, true, 0); > genpd->suspended_count--; > + genpd_restore_performance_state(dev, dev_gpd_data(dev)->pm_pstate); > genpd_unlock(genpd); > > if (genpd->dev_ops.stop && genpd->dev_ops.start && > @@ -1364,6 +1367,7 @@ static int genpd_restore_noirq(struct device *dev) > } > > genpd_sync_power_on(genpd, true, 0); > + genpd_restore_performance_state(dev, dev_gpd_data(dev)->pm_pstate); > genpd_unlock(genpd); > > if (genpd->dev_ops.stop && genpd->dev_ops.start && > @@ -1409,23 +1413,28 @@ 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_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--; > + genpd_restore_performance_state(dev, gpd_data->pm_pstate); > } > > if (use_lock) > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index 21a0577305ef..f6e9dc28621c 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_pstate; > + unsigned int pm_pstate; > ktime_t next_wakeup; > void *data; > }; > -- > 2.25.1 >
| |