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

Ulf Hansson
2021-06-03T10:22:50Z

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