Hi,
On Fri, 9 Jul 2021 at 11:25, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>
> On Fri, 9 Jul 2021 at 06:32, Dmitry Baryshkov
> <dmitry.baryshkov@xxxxxxxxxx> 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@xxxxxxxxxx>
>
> 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
