Re: [RESEND PATCH v2 3/7] PM: domains: Add support for runtime PM

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

Hi,

On Fri, 9 Jul 2021 at 11:25, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 9 Jul 2021 at 06:32, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > Registers for some genpds can be located in the SoC area, powered up by
> > another power domain. To enabled access to those registers, respective
> > domain should be turned on.
> >
> > This patch adds basic infrastructure to the genpd code to allow
> > implementing drivers for such genpd. PM domain can provide the parent
> > device through the genpd->dev.parent pointer. If its provided at the
> > pm_genpd_init() call time and if it is pm-enabled, genpd power_on and
> > power_off operations will call pm_runtime_get_sync() before powering up
> > the domain and pm_runtime_put_sync() after powering it down.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> Hi Dmitry,
>
> Using runtime PM for the genpd provider device, is not the correct
> approach. If the provider domain needs another domain to be powered on
> to work correctly, that per definition means that it has a parent
> domain.
>
> I suggest you try to build the correct PM domain topology, via using
> pm_genpd_add_subdomain() or of_genpd_add_subdomain(), then genpd will
> manages the power on/off for parent/child domain internally.

Indeed, this patch seems redundant now, with the
pm_genpd_add_subdomain call in place.
Would you like me to resend a v3 just dropping this patch?

>
> Kind regards
> Uffe
>
> > ---
> >  drivers/base/power/domain.c | 33 +++++++++++++++++++++++++++++++++
> >  include/linux/pm_domain.h   |  6 ++++++
> >  2 files changed, 39 insertions(+)
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index e5d97174c254..7d49531c9731 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -482,6 +482,30 @@ void dev_pm_genpd_set_next_wakeup(struct device *dev, ktime_t next)
> >  }
> >  EXPORT_SYMBOL_GPL(dev_pm_genpd_set_next_wakeup);
> >
> > +static int _genpd_pm_runtime_get(struct generic_pm_domain *genpd)
> > +{
> > +       int ret;
> > +
> > +       if (!(genpd->flags & _GENPD_FLAG_RPM_ENABLED))
> > +               return 0;
> > +
> > +       ret = pm_runtime_get_sync(genpd->dev.parent);
> > +       if (ret < 0) {
> > +               pm_runtime_put_noidle(genpd->dev.parent);
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> > +static void _genpd_pm_runtime_put(struct generic_pm_domain *genpd)
> > +{
> > +       if (!(genpd->flags & _GENPD_FLAG_RPM_ENABLED))
> > +               return;
> > +
> > +       pm_runtime_put_sync(genpd->dev.parent);
> > +}
> > +
> >  static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
> >  {
> >         unsigned int state_idx = genpd->state_idx;
> > @@ -497,6 +521,10 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
> >         if (ret)
> >                 return ret;
> >
> > +       ret = _genpd_pm_runtime_get(genpd);
> > +       if (ret)
> > +               return ret;
> > +
> >         if (!genpd->power_on)
> >                 goto out;
> >
> > @@ -526,6 +554,7 @@ static int _genpd_power_on(struct generic_pm_domain *genpd, bool timed)
> >         raw_notifier_call_chain(&genpd->power_notifiers, GENPD_NOTIFY_ON, NULL);
> >         return 0;
> >  err:
> > +       _genpd_pm_runtime_put(genpd);
> >         raw_notifier_call_chain(&genpd->power_notifiers, GENPD_NOTIFY_OFF,
> >                                 NULL);
> >         return ret;
> > @@ -572,6 +601,7 @@ static int _genpd_power_off(struct generic_pm_domain *genpd, bool timed)
> >                  genpd->name, "off", elapsed_ns);
> >
> >  out:
> > +       _genpd_pm_runtime_put(genpd);
> >         raw_notifier_call_chain(&genpd->power_notifiers, GENPD_NOTIFY_OFF,
> >                                 NULL);
> >         return 0;
> > @@ -1986,6 +2016,9 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
> >         genpd->domain.ops.complete = genpd_complete;
> >         genpd->domain.start = genpd_dev_pm_start;
> >
> > +       if (genpd->dev.parent && pm_runtime_enabled(genpd->dev.parent))
> > +               genpd->flags |= _GENPD_FLAG_RPM_ENABLED;
> > +
> >         if (genpd->flags & GENPD_FLAG_PM_CLK) {
> >                 genpd->dev_ops.stop = pm_clk_suspend;
> >                 genpd->dev_ops.start = pm_clk_resume;
> > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> > index 21a0577305ef..e86cd7cfc9ec 100644
> > --- a/include/linux/pm_domain.h
> > +++ b/include/linux/pm_domain.h
> > @@ -60,6 +60,10 @@
> >   * GENPD_FLAG_MIN_RESIDENCY:   Enable the genpd governor to consider its
> >   *                             components' next wakeup when determining the
> >   *                             optimal idle state.
> > + *
> > + * _GENPD_FLAG_RPM_ENABLED:    Use genpd's parent dev for runtime power
> > + *                             management. There is no need to set this flag,
> > + *                             it will be detected automatically.
> >   */
> >  #define GENPD_FLAG_PM_CLK       (1U << 0)
> >  #define GENPD_FLAG_IRQ_SAFE     (1U << 1)
> > @@ -69,6 +73,8 @@
> >  #define GENPD_FLAG_RPM_ALWAYS_ON (1U << 5)
> >  #define GENPD_FLAG_MIN_RESIDENCY (1U << 6)
> >
> > +#define _GENPD_FLAG_RPM_ENABLED         (1U << 31)
> > +
> >  enum gpd_status {
> >         GENPD_STATE_ON = 0,     /* PM domain is on */
> >         GENPD_STATE_OFF,        /* PM domain is off */
> > --
> > 2.30.2
> >



-- 
With best wishes
Dmitry