Re: [PATCH v2] PM / Domains: Keep the pd status during system PM phases

[Elaine Zhang]

On 01/20/2017 09:16 PM, Ulf Hansson wrote:
> On 20 January 2017 at 03:21, Elaine Zhang <zhangqing@xxxxxxxxxxxxxx> wrote:
> > If a PM domain is powered off before system suspend,
> > we hope do nothing in system runtime suspend noirq phase
> > and system runtime resume noirq phase.
>
> One can hope, but that isn't good enough. :-)
>
>
> > This modify is to slove system resume issue for RK3399.
> > RK3399 SOC pd_gpu have voltage domain vdd_gpu,
> > so we must follow open vdd_gpu and power on pd_gpu,
> > power off pd_gpu and disable vdd_gpu.
> > Fix up in runtime resume noirq phase power on all PDs.
>
> This doesn't make any sense to me. Can please try to explain this is
> in great more detail, then I can try to help.
For example:
-->device suspend
   (mali gpu driver set pd_gpu off by pm_runtime_put_sync(),
    and then disabled the vdd_gpu by regulator_disable().)
	--> system suspend
		-->prepare
			-->suspend_noirq():
			(power off all pds)
	-->system resume
		-->resume_noirq():
		(power up all pds)
		(in this case the vdd_gpu is still disabled,
		 if power on the pd_gpu maybe make the system crash)
	-->complete : power off the not used pd
-->device resuem
   (mali gpu driver enable vdd_gpu by regulator_enable(),
    and then power up the pd_gpu by pm_runtime_get_sync())

So for RK3399 soc, if to set pd_gpu the vdd_gpu must be enabled, or else 
will can't get the ack back.
I hope the pd_gpu power up/off by the driver itself.

May be I solution is not the optimal solution,Do you have better suggestion?

> > Signed-off-by: Elaine Zhang <zhangqing@xxxxxxxxxxxxxx>
> > ---
> >   drivers/base/power/domain.c | 10 +++++++---
> >   include/linux/pm_domain.h   |  1 +
> >   2 files changed, 8 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 5711708532db..81351eec570f 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -842,8 +842,10 @@ static int pm_genpd_prepare(struct device *dev)
> >
> >          genpd_lock(genpd);
> >
> > -       if (genpd->prepared_count++ == 0)
> > +       if (genpd->prepared_count++ == 0) {
> >                  genpd->suspended_count = 0;
> > +               genpd->suspend_power_off = genpd->status == GPD_STATE_POWER_OFF;
> > +       }
> >
> >          genpd_unlock(genpd);
> >
> > @@ -877,7 +879,8 @@ static int pm_genpd_suspend_noirq(struct device *dev)
> >          if (IS_ERR(genpd))
> >                  return -EINVAL;
> >
> > -       if (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev))
> > +       if (genpd->suspend_power_off ||
> > +           (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev)))
> >                  return 0;
>
> This isn't going to work, because the PM domain might have been powered on.
>
> There is simply no guarantee to that the PM domain remains powered off
> during the system PM suspend phases. For example, any device in the PM
> domain that get runtime resumed after the ->prepare() callback is
> invoked this, will of course also trigger the PM domain to be powered
> on.
>
> >          if (genpd->dev_ops.stop && genpd->dev_ops.start) {
> > @@ -914,7 +917,8 @@ static int pm_genpd_resume_noirq(struct device *dev)
> >          if (IS_ERR(genpd))
> >                  return -EINVAL;
> >
> > -       if (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev))
> > +       if (genpd->suspend_power_off ||
> > +           (dev->power.wakeup_path && genpd_dev_active_wakeup(genpd, dev)))
> >                  return 0;
>
> Ditto, for the same reasons as above.
>
> >          /*
> > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> > index 81ece61075df..9c0dc364f089 100644
> > --- a/include/linux/pm_domain.h
> > +++ b/include/linux/pm_domain.h
> > @@ -62,6 +62,7 @@ struct generic_pm_domain {
> >          unsigned int device_count;      /* Number of devices */
> >          unsigned int suspended_count;   /* System suspend device counter */
> >          unsigned int prepared_count;    /* Suspend counter of prepared devices */
> > +       bool suspend_power_off; /* Power status before system suspend */
> >          int (*power_off)(struct generic_pm_domain *domain);
> >          int (*power_on)(struct generic_pm_domain *domain);
> >          struct gpd_dev_ops dev_ops;
> > --
> > 1.9.1
>
> Kind regards
> Uffe