diff mbox series

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

Message ID 20210709043136.533205-4-dmitry.baryshkov@linaro.org
State Superseded
Headers show
Series clk: qcom: use power-domain for sm8250's clock controllers | expand

Commit Message

Dmitry Baryshkov July 9, 2021, 4:31 a.m. UTC 
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>

---
 drivers/base/power/domain.c | 33 +++++++++++++++++++++++++++++++++
 include/linux/pm_domain.h   |  6 ++++++
 2 files changed, 39 insertions(+)

-- 
2.30.2

Comments

Ulf Hansson July 9, 2021, 8:24 a.m. UTC | #1 
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.

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
>
Dmitry Baryshkov July 9, 2021, 11:39 a.m. UTC | #2 
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
Ulf Hansson July 9, 2021, 12:20 p.m. UTC | #3 
On Fri, 9 Jul 2021 at 13:39, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>

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


Yes, $subject patch isn't the way to go.

Let's continue discussing things on patch3/7 to conclude on the way forward.

[...]

Kind regards
Uffe
diff mbox series

Patch

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 */