[RESEND PATCH v2 0/7] clk: qcom: use power-domain for sm8250's clock controllers

[RESEND PATCH v2 0/7] clk: qcom: use power-domain for sm8250's clock controllers

[Dmitry Baryshkov]

On SM8250 both the display and video clock controllers are powered up by
the MMCX power domain. Handle this by linking clock controllers to the
proper power domain, and using runtime power management to enable and
disable the MMCX power domain.

Dependencies:
- https://lore.kernel.org/linux-pm/20210603093438.138705-1-ulf.hansson@linaro.org/ (merged in 5.14)
- https://lore.kernel.org/linux-arm-msm/20210703005416.2668319-1-bjorn.andersson@linaro.org/
  (pending)

Patches resent because I missed one hunk in the PM domains patch, which
got stuck in the git index. Now the patch is fixed.

Changes since v1:
 - Rebase on top of Bjorn's patches, removing the need for setting
   performance state directly.
 - Move runtime PM calls from GDSC code to generic genpd code.
 - Always call pm_runtime_enable in the Qualcomm generic clock
   controller code.
 - Register GDSC power domains as subdomains of the domain powering the
   clock controller if there is one.

----------------------------------------------------------------
Dmitry Baryshkov (7):
      dt-bindings: clock: qcom,dispcc-sm8x50: add mmcx power domain
      dt-bindings: clock: qcom,videocc: add mmcx power domain
      PM: domains: Add support for runtime PM
      clk: qcom: gdsc: enable optional power domain support
      arm64: dts: qcom: sm8250: remove mmcx regulator
      clk: qcom: dispcc-sm8250: stop using mmcx regulator
      clk: qcom: videocc-sm8250: stop using mmcx regulator

 .../bindings/clock/qcom,dispcc-sm8x50.yaml         |  7 ++++
 .../devicetree/bindings/clock/qcom,videocc.yaml    |  7 ++++
 arch/arm64/boot/dts/qcom/sm8250.dtsi               | 11 ++-----
 drivers/base/power/domain.c                        | 33 +++++++++++++++++++
 drivers/clk/qcom/common.c                          | 37 ++++++++++++++++++----
 drivers/clk/qcom/dispcc-sm8250.c                   |  1 -
 drivers/clk/qcom/gdsc.c                            |  5 +++
 drivers/clk/qcom/videocc-sm8250.c                  |  4 ---
 include/linux/pm_domain.h                          |  6 ++++
 9 files changed, 91 insertions(+), 20 deletions(-)

[RESEND PATCH v2 1/7] dt-bindings: clock: qcom,dispcc-sm8x50: add mmcx power domain

[Dmitry Baryshkov]

On sm8250 dispcc requires MMCX power domain to be powered up before
clock controller's registers become available. For now sm8250 was using
external regulator driven by the power domain to describe this
relationship. Switch into specifying power-domain and required opp-state
directly.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 .../devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml      | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
index 0cdf53f41f84..d5c4fed56b6e 100644
--- a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
@@ -55,6 +55,11 @@ properties:
   reg:
     maxItems: 1
 
+  power-domains:
+    description:
+      A phandle and PM domain specifier for the MMCX power domain.
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -69,6 +74,7 @@ additionalProperties: false
 examples:
   - |
     #include <dt-bindings/clock/qcom,rpmh.h>
+    #include <dt-bindings/power/qcom-rpmpd.h>
     clock-controller@af00000 {
       compatible = "qcom,sm8250-dispcc";
       reg = <0x0af00000 0x10000>;
@@ -89,5 +95,6 @@ examples:
       #clock-cells = <1>;
       #reset-cells = <1>;
       #power-domain-cells = <1>;
+      power-domains = <&rpmhpd SM8250_MMCX>;
     };
 ...
-- 
2.30.2

[RESEND PATCH v2 2/7] dt-bindings: clock: qcom,videocc: add mmcx power domain

[Dmitry Baryshkov]

On sm8250 videocc requires MMCX power domain to be powered up before
clock controller's registers become available. For now sm8250 was using
external regulator driven by the power domain to describe this
relationship. Switch into specifying power-domain and required opp-state
directly.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 Documentation/devicetree/bindings/clock/qcom,videocc.yaml | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/qcom,videocc.yaml b/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
index 567202942b88..db4ada6acf27 100644
--- a/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
@@ -47,6 +47,11 @@ properties:
   reg:
     maxItems: 1
 
+  power-domains:
+    description:
+      A phandle and PM domain specifier for the MMCX power domain.
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -61,6 +66,7 @@ additionalProperties: false
 examples:
   - |
     #include <dt-bindings/clock/qcom,rpmh.h>
+    #include <dt-bindings/power/qcom-rpmpd.h>
     clock-controller@ab00000 {
       compatible = "qcom,sdm845-videocc";
       reg = <0x0ab00000 0x10000>;
@@ -69,5 +75,6 @@ examples:
       #clock-cells = <1>;
       #reset-cells = <1>;
       #power-domain-cells = <1>;
+      power-domains = <&rpmhpd SM8250_MMCX>;
     };
 ...
-- 
2.30.2

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

[Dmitry Baryshkov]

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

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

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

[Ulf Hansson]

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
>

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

[Dmitry Baryshkov]

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

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

[Ulf Hansson]

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

[RESEND PATCH v2 4/7] clk: qcom: gdsc: enable optional power domain support

[Dmitry Baryshkov]

On sm8250 dispcc and videocc registers are powered up by the MMCX power
domain. Currently we used a regulator to enable this domain on demand,
however this has some consequences, as genpd code is not reentrant.

Teach Qualcomm clock controller code about setting up power domains and
using them for gdsc control.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/common.c | 37 +++++++++++++++++++++++++++++++------
 drivers/clk/qcom/gdsc.c   |  5 +++++
 2 files changed, 36 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index 60d2a78d1395..43d8f8feeb3c 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -10,6 +10,7 @@
 #include <linux/clk-provider.h>
 #include <linux/reset-controller.h>
 #include <linux/of.h>
+#include <linux/pm_runtime.h>
 
 #include "common.h"
 #include "clk-rcg.h"
@@ -224,6 +225,11 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec,
 	return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
 }
 
+static void qcom_cc_pm_runtime_disable(void *data)
+{
+	pm_runtime_disable(data);
+}
+
 int qcom_cc_really_probe(struct platform_device *pdev,
 			 const struct qcom_cc_desc *desc, struct regmap *regmap)
 {
@@ -241,6 +247,18 @@ int qcom_cc_really_probe(struct platform_device *pdev,
 	if (!cc)
 		return -ENOMEM;
 
+	pm_runtime_enable(dev);
+	ret = pm_runtime_get_sync(dev);
+	if (ret < 0) {
+		pm_runtime_put(dev);
+		pm_runtime_disable(dev);
+		return ret;
+	}
+
+	ret = devm_add_action_or_reset(dev, qcom_cc_pm_runtime_disable, dev);
+	if (ret)
+		goto err;
+
 	reset = &cc->reset;
 	reset->rcdev.of_node = dev->of_node;
 	reset->rcdev.ops = &qcom_reset_ops;
@@ -251,7 +269,7 @@ int qcom_cc_really_probe(struct platform_device *pdev,
 
 	ret = devm_reset_controller_register(dev, &reset->rcdev);
 	if (ret)
-		return ret;
+		goto err;
 
 	if (desc->gdscs && desc->num_gdscs) {
 		scd = devm_kzalloc(dev, sizeof(*scd), GFP_KERNEL);
@@ -262,11 +280,11 @@ int qcom_cc_really_probe(struct platform_device *pdev,
 		scd->num = desc->num_gdscs;
 		ret = gdsc_register(scd, &reset->rcdev, regmap);
 		if (ret)
-			return ret;
+			goto err;
 		ret = devm_add_action_or_reset(dev, qcom_cc_gdsc_unregister,
 					       scd);
 		if (ret)
-			return ret;
+			goto err;
 	}
 
 	cc->rclks = rclks;
@@ -277,7 +295,7 @@ int qcom_cc_really_probe(struct platform_device *pdev,
 	for (i = 0; i < num_clk_hws; i++) {
 		ret = devm_clk_hw_register(dev, clk_hws[i]);
 		if (ret)
-			return ret;
+			goto err;
 	}
 
 	for (i = 0; i < num_clks; i++) {
@@ -286,14 +304,21 @@ int qcom_cc_really_probe(struct platform_device *pdev,
 
 		ret = devm_clk_register_regmap(dev, rclks[i]);
 		if (ret)
-			return ret;
+			goto err;
 	}
 
 	ret = devm_of_clk_add_hw_provider(dev, qcom_cc_clk_hw_get, cc);
 	if (ret)
-		return ret;
+		goto err;
+
+	pm_runtime_put(dev);
 
 	return 0;
+
+err:
+	pm_runtime_put(dev);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(qcom_cc_really_probe);
 
diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 51ed640e527b..9401d01533c8 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -427,6 +427,7 @@ int gdsc_register(struct gdsc_desc *desc,
 			continue;
 		scs[i]->regmap = regmap;
 		scs[i]->rcdev = rcdev;
+		scs[i]->pd.dev.parent = desc->dev;
 		ret = gdsc_init(scs[i]);
 		if (ret)
 			return ret;
@@ -439,6 +440,8 @@ int gdsc_register(struct gdsc_desc *desc,
 			continue;
 		if (scs[i]->parent)
 			pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
+		else if (!IS_ERR_OR_NULL(dev->pm_domain))
+			pm_genpd_add_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd);
 	}
 
 	return of_genpd_add_provider_onecell(dev->of_node, data);
@@ -457,6 +460,8 @@ void gdsc_unregister(struct gdsc_desc *desc)
 			continue;
 		if (scs[i]->parent)
 			pm_genpd_remove_subdomain(scs[i]->parent, &scs[i]->pd);
+		else if (!IS_ERR_OR_NULL(dev->pm_domain))
+			pm_genpd_remove_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd);
 	}
 	of_genpd_del_provider(dev->of_node);
 }
-- 
2.30.2

Re: [RESEND PATCH v2 4/7] clk: qcom: gdsc: enable optional power domain support

[Ulf Hansson]

On Fri, 9 Jul 2021 at 06:32, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On sm8250 dispcc and videocc registers are powered up by the MMCX power
> domain. Currently we used a regulator to enable this domain on demand,
> however this has some consequences, as genpd code is not reentrant.
>
> Teach Qualcomm clock controller code about setting up power domains and
> using them for gdsc control.
>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

[...]

> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 51ed640e527b..9401d01533c8 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -427,6 +427,7 @@ int gdsc_register(struct gdsc_desc *desc,
>                         continue;
>                 scs[i]->regmap = regmap;
>                 scs[i]->rcdev = rcdev;
> +               scs[i]->pd.dev.parent = desc->dev;
>                 ret = gdsc_init(scs[i]);
>                 if (ret)
>                         return ret;
> @@ -439,6 +440,8 @@ int gdsc_register(struct gdsc_desc *desc,
>                         continue;
>                 if (scs[i]->parent)
>                         pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
> +               else if (!IS_ERR_OR_NULL(dev->pm_domain))

So dev_pm_domain_attach() (which calls genpd_dev_pm_attach() is being
called for gdsc platform device from the platform bus', to try to
attach the device to its corresponding PM domain.

Looking a bit closer to genpd_dev_pm_attach(), I realize that we
shouldn't really try to attach a device to its PM domain, when its OF
node (dev->of_node) contains a "#power-domain-cells" specifier. This
is because it indicates that the device belongs to a genpd provider
itself. In this case, a "power-domains" specifier tells that it has a
parent domain.

I will post a patch that fixes this asap.

> +                       pm_genpd_add_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd);
>         }
>
>         return of_genpd_add_provider_onecell(dev->of_node, data);
> @@ -457,6 +460,8 @@ void gdsc_unregister(struct gdsc_desc *desc)
>                         continue;
>                 if (scs[i]->parent)
>                         pm_genpd_remove_subdomain(scs[i]->parent, &scs[i]->pd);
> +               else if (!IS_ERR_OR_NULL(dev->pm_domain))

Ditto.

> +                       pm_genpd_remove_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd);
>         }
>         of_genpd_del_provider(dev->of_node);
>  }
> --
> 2.30.2
>

Kind regards
Uffe

Re: [RESEND PATCH v2 4/7] clk: qcom: gdsc: enable optional power domain support

[Dmitry Baryshkov]

On Fri, 9 Jul 2021 at 12:33, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 9 Jul 2021 at 06:32, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On sm8250 dispcc and videocc registers are powered up by the MMCX power
> > domain. Currently we used a regulator to enable this domain on demand,
> > however this has some consequences, as genpd code is not reentrant.
> >
> > Teach Qualcomm clock controller code about setting up power domains and
> > using them for gdsc control.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> [...]
>
> > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > index 51ed640e527b..9401d01533c8 100644
> > --- a/drivers/clk/qcom/gdsc.c
> > +++ b/drivers/clk/qcom/gdsc.c
> > @@ -427,6 +427,7 @@ int gdsc_register(struct gdsc_desc *desc,
> >                         continue;
> >                 scs[i]->regmap = regmap;
> >                 scs[i]->rcdev = rcdev;
> > +               scs[i]->pd.dev.parent = desc->dev;
> >                 ret = gdsc_init(scs[i]);
> >                 if (ret)
> >                         return ret;
> > @@ -439,6 +440,8 @@ int gdsc_register(struct gdsc_desc *desc,
> >                         continue;
> >                 if (scs[i]->parent)
> >                         pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
> > +               else if (!IS_ERR_OR_NULL(dev->pm_domain))
>
> So dev_pm_domain_attach() (which calls genpd_dev_pm_attach() is being
> called for gdsc platform device from the platform bus', to try to
> attach the device to its corresponding PM domain.
>
> Looking a bit closer to genpd_dev_pm_attach(), I realize that we
> shouldn't really try to attach a device to its PM domain, when its OF
> node (dev->of_node) contains a "#power-domain-cells" specifier. This
> is because it indicates that the device belongs to a genpd provider
> itself. In this case, a "power-domains" specifier tells that it has a
> parent domain.
>
> I will post a patch that fixes this asap.

I think there is nothing to fix here. The dispcc/videocc drivers
provide clocks in addition to the gdsc power domain. And provided
clocks would definitely benefit from having the dispcc device being
attached to the power domain which governs clock registers (MMCX in
our case). Thus I think it is perfectly valid to have:

rpmhpd device:
 - provides MMCX domain.

dispcc device:
 - is attached to the MMCX domain,
 - provides MDSS_GDSC
 - provides clocks

>
> > +                       pm_genpd_add_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd);
> >         }
> >
> >         return of_genpd_add_provider_onecell(dev->of_node, data);
> > @@ -457,6 +460,8 @@ void gdsc_unregister(struct gdsc_desc *desc)
> >                         continue;
> >                 if (scs[i]->parent)
> >                         pm_genpd_remove_subdomain(scs[i]->parent, &scs[i]->pd);
> > +               else if (!IS_ERR_OR_NULL(dev->pm_domain))
>
> Ditto.
>
> > +                       pm_genpd_remove_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd);
> >         }
> >         of_genpd_del_provider(dev->of_node);
> >  }
> > --
> > 2.30.2
> >
>
> Kind regards
> Uffe



-- 
With best wishes
Dmitry

Re: [RESEND PATCH v2 4/7] clk: qcom: gdsc: enable optional power domain support

[Ulf Hansson]

On Fri, 9 Jul 2021 at 13:46, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Fri, 9 Jul 2021 at 12:33, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Fri, 9 Jul 2021 at 06:32, Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > On sm8250 dispcc and videocc registers are powered up by the MMCX power
> > > domain. Currently we used a regulator to enable this domain on demand,
> > > however this has some consequences, as genpd code is not reentrant.
> > >
> > > Teach Qualcomm clock controller code about setting up power domains and
> > > using them for gdsc control.
> > >
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >
> > [...]
> >
> > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > > index 51ed640e527b..9401d01533c8 100644
> > > --- a/drivers/clk/qcom/gdsc.c
> > > +++ b/drivers/clk/qcom/gdsc.c
> > > @@ -427,6 +427,7 @@ int gdsc_register(struct gdsc_desc *desc,
> > >                         continue;
> > >                 scs[i]->regmap = regmap;
> > >                 scs[i]->rcdev = rcdev;
> > > +               scs[i]->pd.dev.parent = desc->dev;
> > >                 ret = gdsc_init(scs[i]);
> > >                 if (ret)
> > >                         return ret;
> > > @@ -439,6 +440,8 @@ int gdsc_register(struct gdsc_desc *desc,
> > >                         continue;
> > >                 if (scs[i]->parent)
> > >                         pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
> > > +               else if (!IS_ERR_OR_NULL(dev->pm_domain))
> >
> > So dev_pm_domain_attach() (which calls genpd_dev_pm_attach() is being
> > called for gdsc platform device from the platform bus', to try to
> > attach the device to its corresponding PM domain.
> >
> > Looking a bit closer to genpd_dev_pm_attach(), I realize that we
> > shouldn't really try to attach a device to its PM domain, when its OF
> > node (dev->of_node) contains a "#power-domain-cells" specifier. This
> > is because it indicates that the device belongs to a genpd provider
> > itself. In this case, a "power-domains" specifier tells that it has a
> > parent domain.
> >
> > I will post a patch that fixes this asap.
>
> I think there is nothing to fix here. The dispcc/videocc drivers
> provide clocks in addition to the gdsc power domain. And provided
> clocks would definitely benefit from having the dispcc device being
> attached to the power domain which governs clock registers (MMCX in
> our case). Thus I think it is perfectly valid to have:
>
> rpmhpd device:
>  - provides MMCX domain.
>
> dispcc device:
>  - is attached to the MMCX domain,

We don't need this, it's redundant and weird to me.

Also I am kind of worried that you will hit another new path in genpd,
causing locking issues etc, as it has not been designed to work like
this (a provider device and a child domain sharing the same "parent").

>  - provides MDSS_GDSC

It's perfectly fine that dispcc acts as a genpd provider. In this
case, the corresponding PM domain should be assigned as a child for
the parent MMCX domain. That should make this work, I think.

>  - provides clocks

That sounds reasonable as well.

>
> >
> > > +                       pm_genpd_add_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd);
> > >         }
> > >
> > >         return of_genpd_add_provider_onecell(dev->of_node, data);
> > > @@ -457,6 +460,8 @@ void gdsc_unregister(struct gdsc_desc *desc)
> > >                         continue;
> > >                 if (scs[i]->parent)
> > >                         pm_genpd_remove_subdomain(scs[i]->parent, &scs[i]->pd);
> > > +               else if (!IS_ERR_OR_NULL(dev->pm_domain))
> >
> > Ditto.
> >
> > > +                       pm_genpd_remove_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd);
> > >         }
> > >         of_genpd_del_provider(dev->of_node);
> > >  }
> > > --
> > > 2.30.2
> > >
> >

Kind regards
Uffe

Re: [RESEND PATCH v2 4/7] clk: qcom: gdsc: enable optional power domain support

[Dmitry Baryshkov]

On Fri, 9 Jul 2021 at 15:18, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 9 Jul 2021 at 13:46, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Fri, 9 Jul 2021 at 12:33, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Fri, 9 Jul 2021 at 06:32, Dmitry Baryshkov
> > > <dmitry.baryshkov@linaro.org> wrote:
> > > >
> > > > On sm8250 dispcc and videocc registers are powered up by the MMCX power
> > > > domain. Currently we used a regulator to enable this domain on demand,
> > > > however this has some consequences, as genpd code is not reentrant.
> > > >
> > > > Teach Qualcomm clock controller code about setting up power domains and
> > > > using them for gdsc control.
> > > >
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > >
> > > [...]
> > >
> > > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > > > index 51ed640e527b..9401d01533c8 100644
> > > > --- a/drivers/clk/qcom/gdsc.c
> > > > +++ b/drivers/clk/qcom/gdsc.c
> > > > @@ -427,6 +427,7 @@ int gdsc_register(struct gdsc_desc *desc,
> > > >                         continue;
> > > >                 scs[i]->regmap = regmap;
> > > >                 scs[i]->rcdev = rcdev;
> > > > +               scs[i]->pd.dev.parent = desc->dev;
> > > >                 ret = gdsc_init(scs[i]);
> > > >                 if (ret)
> > > >                         return ret;
> > > > @@ -439,6 +440,8 @@ int gdsc_register(struct gdsc_desc *desc,
> > > >                         continue;
> > > >                 if (scs[i]->parent)
> > > >                         pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
> > > > +               else if (!IS_ERR_OR_NULL(dev->pm_domain))
> > >
> > > So dev_pm_domain_attach() (which calls genpd_dev_pm_attach() is being
> > > called for gdsc platform device from the platform bus', to try to
> > > attach the device to its corresponding PM domain.
> > >
> > > Looking a bit closer to genpd_dev_pm_attach(), I realize that we
> > > shouldn't really try to attach a device to its PM domain, when its OF
> > > node (dev->of_node) contains a "#power-domain-cells" specifier. This
> > > is because it indicates that the device belongs to a genpd provider
> > > itself. In this case, a "power-domains" specifier tells that it has a
> > > parent domain.
> > >
> > > I will post a patch that fixes this asap.
> >
> > I think there is nothing to fix here. The dispcc/videocc drivers
> > provide clocks in addition to the gdsc power domain. And provided
> > clocks would definitely benefit from having the dispcc device being
> > attached to the power domain which governs clock registers (MMCX in
> > our case). Thus I think it is perfectly valid to have:
> >
> > rpmhpd device:
> >  - provides MMCX domain.
> >
> > dispcc device:
> >  - is attached to the MMCX domain,
>
> We don't need this, it's redundant and weird to me.
>
> Also I am kind of worried that you will hit another new path in genpd,
> causing locking issues etc, as it has not been designed to work like
> this (a provider device and a child domain sharing the same "parent").

So, which domain should the dispcc device belong to? It's registers
are powered by the MMCX domain. I can not attach it to the child
(GDSC) domain either: in the case of videocc there are 4 child
domains.
An alternative would be to request that all users of the provided
clocks power on one of the child domains. However this is also not
perfect. If some generic code (e.g. clock framework) calls into
provided clocks (e.g. because of assigned-clock-rates), this can
happen w/o proper power domain being powered up yet.

>
> >  - provides MDSS_GDSC
>
> It's perfectly fine that dispcc acts as a genpd provider. In this
> case, the corresponding PM domain should be assigned as a child for
> the parent MMCX domain. That should make this work, I think.
>
> >  - provides clocks
>
> That sounds reasonable as well.
>
> >
> > >
> > > > +                       pm_genpd_add_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd);
> > > >         }
> > > >
> > > >         return of_genpd_add_provider_onecell(dev->of_node, data);
> > > > @@ -457,6 +460,8 @@ void gdsc_unregister(struct gdsc_desc *desc)
> > > >                         continue;
> > > >                 if (scs[i]->parent)
> > > >                         pm_genpd_remove_subdomain(scs[i]->parent, &scs[i]->pd);
> > > > +               else if (!IS_ERR_OR_NULL(dev->pm_domain))
> > >
> > > Ditto.
> > >
> > > > +                       pm_genpd_remove_subdomain(pd_to_genpd(dev->pm_domain), &scs[i]->pd);
> > > >         }
> > > >         of_genpd_del_provider(dev->of_node);
> > > >  }
> > > > --
> > > > 2.30.2
> > > >
> > >
>
> Kind regards
> Uffe



-- 
With best wishes
Dmitry

Re: [RESEND PATCH v2 4/7] clk: qcom: gdsc: enable optional power domain support

[Ulf Hansson]

On Fri, 9 Jul 2021 at 14:59, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Fri, 9 Jul 2021 at 15:18, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Fri, 9 Jul 2021 at 13:46, Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > On Fri, 9 Jul 2021 at 12:33, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > On Fri, 9 Jul 2021 at 06:32, Dmitry Baryshkov
> > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > >
> > > > > On sm8250 dispcc and videocc registers are powered up by the MMCX power
> > > > > domain. Currently we used a regulator to enable this domain on demand,
> > > > > however this has some consequences, as genpd code is not reentrant.
> > > > >
> > > > > Teach Qualcomm clock controller code about setting up power domains and
> > > > > using them for gdsc control.
> > > > >
> > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > >
> > > > [...]
> > > >
> > > > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > > > > index 51ed640e527b..9401d01533c8 100644
> > > > > --- a/drivers/clk/qcom/gdsc.c
> > > > > +++ b/drivers/clk/qcom/gdsc.c
> > > > > @@ -427,6 +427,7 @@ int gdsc_register(struct gdsc_desc *desc,
> > > > >                         continue;
> > > > >                 scs[i]->regmap = regmap;
> > > > >                 scs[i]->rcdev = rcdev;
> > > > > +               scs[i]->pd.dev.parent = desc->dev;
> > > > >                 ret = gdsc_init(scs[i]);
> > > > >                 if (ret)
> > > > >                         return ret;
> > > > > @@ -439,6 +440,8 @@ int gdsc_register(struct gdsc_desc *desc,
> > > > >                         continue;
> > > > >                 if (scs[i]->parent)
> > > > >                         pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
> > > > > +               else if (!IS_ERR_OR_NULL(dev->pm_domain))
> > > >
> > > > So dev_pm_domain_attach() (which calls genpd_dev_pm_attach() is being
> > > > called for gdsc platform device from the platform bus', to try to
> > > > attach the device to its corresponding PM domain.
> > > >
> > > > Looking a bit closer to genpd_dev_pm_attach(), I realize that we
> > > > shouldn't really try to attach a device to its PM domain, when its OF
> > > > node (dev->of_node) contains a "#power-domain-cells" specifier. This
> > > > is because it indicates that the device belongs to a genpd provider
> > > > itself. In this case, a "power-domains" specifier tells that it has a
> > > > parent domain.
> > > >
> > > > I will post a patch that fixes this asap.
> > >
> > > I think there is nothing to fix here. The dispcc/videocc drivers
> > > provide clocks in addition to the gdsc power domain. And provided
> > > clocks would definitely benefit from having the dispcc device being
> > > attached to the power domain which governs clock registers (MMCX in
> > > our case). Thus I think it is perfectly valid to have:
> > >
> > > rpmhpd device:
> > >  - provides MMCX domain.
> > >
> > > dispcc device:
> > >  - is attached to the MMCX domain,
> >
> > We don't need this, it's redundant and weird to me.
> >
> > Also I am kind of worried that you will hit another new path in genpd,
> > causing locking issues etc, as it has not been designed to work like
> > this (a provider device and a child domain sharing the same "parent").
>
> So, which domain should the dispcc device belong to? It's registers
> are powered by the MMCX domain. I can not attach it to the child
> (GDSC) domain either: in the case of videocc there are 4 child
> domains.

The dispcc device should *not* be attached to a PM domain.

Instead it should be registered as a genpd provider and the
corresponding PM domains it provides, should be assigned as child
domains to the MMCX domain.

This is exactly what the child/parent domain support in genpd is there
to help with.

> An alternative would be to request that all users of the provided
> clocks power on one of the child domains. However this is also not
> perfect. If some generic code (e.g. clock framework) calls into
> provided clocks (e.g. because of assigned-clock-rates), this can
> happen w/o proper power domain being powered up yet.

Issues with power on/off synchronization during genpd initializations
and genpd provider registration, certainly need to be fixed and I am
happy to help. However, my point is that I think it's a bad idea to
fix it through modelling the PM domain hierarchy in an incorrect way.

[...]

Kind regards
Uffe

Re: [RESEND PATCH v2 4/7] clk: qcom: gdsc: enable optional power domain support

[Dmitry Baryshkov]

On Fri, 9 Jul 2021 at 16:14, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 9 Jul 2021 at 14:59, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Fri, 9 Jul 2021 at 15:18, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Fri, 9 Jul 2021 at 13:46, Dmitry Baryshkov
> > > <dmitry.baryshkov@linaro.org> wrote:
> > > >
> > > > On Fri, 9 Jul 2021 at 12:33, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > On Fri, 9 Jul 2021 at 06:32, Dmitry Baryshkov
> > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > >
> > > > > > On sm8250 dispcc and videocc registers are powered up by the MMCX power
> > > > > > domain. Currently we used a regulator to enable this domain on demand,
> > > > > > however this has some consequences, as genpd code is not reentrant.
> > > > > >
> > > > > > Teach Qualcomm clock controller code about setting up power domains and
> > > > > > using them for gdsc control.
> > > > > >
> > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > >
> > > > > [...]
> > > > >
> > > > > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > > > > > index 51ed640e527b..9401d01533c8 100644
> > > > > > --- a/drivers/clk/qcom/gdsc.c
> > > > > > +++ b/drivers/clk/qcom/gdsc.c
> > > > > > @@ -427,6 +427,7 @@ int gdsc_register(struct gdsc_desc *desc,
> > > > > >                         continue;
> > > > > >                 scs[i]->regmap = regmap;
> > > > > >                 scs[i]->rcdev = rcdev;
> > > > > > +               scs[i]->pd.dev.parent = desc->dev;
> > > > > >                 ret = gdsc_init(scs[i]);
> > > > > >                 if (ret)
> > > > > >                         return ret;
> > > > > > @@ -439,6 +440,8 @@ int gdsc_register(struct gdsc_desc *desc,
> > > > > >                         continue;
> > > > > >                 if (scs[i]->parent)
> > > > > >                         pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
> > > > > > +               else if (!IS_ERR_OR_NULL(dev->pm_domain))
> > > > >
> > > > > So dev_pm_domain_attach() (which calls genpd_dev_pm_attach() is being
> > > > > called for gdsc platform device from the platform bus', to try to
> > > > > attach the device to its corresponding PM domain.
> > > > >
> > > > > Looking a bit closer to genpd_dev_pm_attach(), I realize that we
> > > > > shouldn't really try to attach a device to its PM domain, when its OF
> > > > > node (dev->of_node) contains a "#power-domain-cells" specifier. This
> > > > > is because it indicates that the device belongs to a genpd provider
> > > > > itself. In this case, a "power-domains" specifier tells that it has a
> > > > > parent domain.
> > > > >
> > > > > I will post a patch that fixes this asap.
> > > >
> > > > I think there is nothing to fix here. The dispcc/videocc drivers
> > > > provide clocks in addition to the gdsc power domain. And provided
> > > > clocks would definitely benefit from having the dispcc device being
> > > > attached to the power domain which governs clock registers (MMCX in
> > > > our case). Thus I think it is perfectly valid to have:
> > > >
> > > > rpmhpd device:
> > > >  - provides MMCX domain.
> > > >
> > > > dispcc device:
> > > >  - is attached to the MMCX domain,
> > >
> > > We don't need this, it's redundant and weird to me.
> > >
> > > Also I am kind of worried that you will hit another new path in genpd,
> > > causing locking issues etc, as it has not been designed to work like
> > > this (a provider device and a child domain sharing the same "parent").
> >
> > So, which domain should the dispcc device belong to? It's registers
> > are powered by the MMCX domain. I can not attach it to the child
> > (GDSC) domain either: in the case of videocc there are 4 child
> > domains.
>
> The dispcc device should *not* be attached to a PM domain.
>
> Instead it should be registered as a genpd provider and the
> corresponding PM domains it provides, should be assigned as child
> domains to the MMCX domain.
>
> This is exactly what the child/parent domain support in genpd is there
> to help with.

This is done in this patchset. If we stop attaching dispcc to the MMCX
genpd, I'll have to locate it in a different way, but the idea is
implemented here.

> > An alternative would be to request that all users of the provided
> > clocks power on one of the child domains. However this is also not
> > perfect. If some generic code (e.g. clock framework) calls into
> > provided clocks (e.g. because of assigned-clock-rates), this can
> > happen w/o proper power domain being powered up yet.
>
> Issues with power on/off synchronization during genpd initializations
> and genpd provider registration, certainly need to be fixed and I am
> happy to help. However, my point is that I think it's a bad idea to
> fix it through modelling the PM domain hierarchy in an incorrect way.

So, which device should I pass to clk_register to handle runtime PM
for the provided clocks? dispcc, should I not?
Then if the dispcc is not attached, we will have to manually handle
MMCX from dispcc's runtime pm callbacks. Correct?

Could you please be more specific, why is it so wrong to attach dispcc
to the MMCX genpd?


-- 
With best wishes
Dmitry

Re: [RESEND PATCH v2 4/7] clk: qcom: gdsc: enable optional power domain support

[Ulf Hansson]

On Fri, 9 Jul 2021 at 15:22, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Fri, 9 Jul 2021 at 16:14, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Fri, 9 Jul 2021 at 14:59, Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > On Fri, 9 Jul 2021 at 15:18, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > On Fri, 9 Jul 2021 at 13:46, Dmitry Baryshkov
> > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > >
> > > > > On Fri, 9 Jul 2021 at 12:33, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > >
> > > > > > On Fri, 9 Jul 2021 at 06:32, Dmitry Baryshkov
> > > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > > >
> > > > > > > On sm8250 dispcc and videocc registers are powered up by the MMCX power
> > > > > > > domain. Currently we used a regulator to enable this domain on demand,
> > > > > > > however this has some consequences, as genpd code is not reentrant.
> > > > > > >
> > > > > > > Teach Qualcomm clock controller code about setting up power domains and
> > > > > > > using them for gdsc control.
> > > > > > >
> > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > > > > > > index 51ed640e527b..9401d01533c8 100644
> > > > > > > --- a/drivers/clk/qcom/gdsc.c
> > > > > > > +++ b/drivers/clk/qcom/gdsc.c
> > > > > > > @@ -427,6 +427,7 @@ int gdsc_register(struct gdsc_desc *desc,
> > > > > > >                         continue;
> > > > > > >                 scs[i]->regmap = regmap;
> > > > > > >                 scs[i]->rcdev = rcdev;
> > > > > > > +               scs[i]->pd.dev.parent = desc->dev;
> > > > > > >                 ret = gdsc_init(scs[i]);
> > > > > > >                 if (ret)
> > > > > > >                         return ret;
> > > > > > > @@ -439,6 +440,8 @@ int gdsc_register(struct gdsc_desc *desc,
> > > > > > >                         continue;
> > > > > > >                 if (scs[i]->parent)
> > > > > > >                         pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
> > > > > > > +               else if (!IS_ERR_OR_NULL(dev->pm_domain))
> > > > > >
> > > > > > So dev_pm_domain_attach() (which calls genpd_dev_pm_attach() is being
> > > > > > called for gdsc platform device from the platform bus', to try to
> > > > > > attach the device to its corresponding PM domain.
> > > > > >
> > > > > > Looking a bit closer to genpd_dev_pm_attach(), I realize that we
> > > > > > shouldn't really try to attach a device to its PM domain, when its OF
> > > > > > node (dev->of_node) contains a "#power-domain-cells" specifier. This
> > > > > > is because it indicates that the device belongs to a genpd provider
> > > > > > itself. In this case, a "power-domains" specifier tells that it has a
> > > > > > parent domain.
> > > > > >
> > > > > > I will post a patch that fixes this asap.
> > > > >
> > > > > I think there is nothing to fix here. The dispcc/videocc drivers
> > > > > provide clocks in addition to the gdsc power domain. And provided
> > > > > clocks would definitely benefit from having the dispcc device being
> > > > > attached to the power domain which governs clock registers (MMCX in
> > > > > our case). Thus I think it is perfectly valid to have:
> > > > >
> > > > > rpmhpd device:
> > > > >  - provides MMCX domain.
> > > > >
> > > > > dispcc device:
> > > > >  - is attached to the MMCX domain,
> > > >
> > > > We don't need this, it's redundant and weird to me.
> > > >
> > > > Also I am kind of worried that you will hit another new path in genpd,
> > > > causing locking issues etc, as it has not been designed to work like
> > > > this (a provider device and a child domain sharing the same "parent").
> > >
> > > So, which domain should the dispcc device belong to? It's registers
> > > are powered by the MMCX domain. I can not attach it to the child
> > > (GDSC) domain either: in the case of videocc there are 4 child
> > > domains.
> >
> > The dispcc device should *not* be attached to a PM domain.
> >
> > Instead it should be registered as a genpd provider and the
> > corresponding PM domains it provides, should be assigned as child
> > domains to the MMCX domain.
> >
> > This is exactly what the child/parent domain support in genpd is there
> > to help with.
>
> This is done in this patchset. If we stop attaching dispcc to the MMCX
> genpd, I'll have to locate it in a different way, but the idea is
> implemented here.

Right. Perhaps it's not such a bad idea after all as it gives you two things:

1) The handle to the MMCX PM domain, which makes sure it has been
registered too before dispcc gets probed.
2) The possibility to control power for the MMCX PM domain via runtime
PM for the dispcc device. This seems useful for your use case.

>
> > > An alternative would be to request that all users of the provided
> > > clocks power on one of the child domains. However this is also not
> > > perfect. If some generic code (e.g. clock framework) calls into
> > > provided clocks (e.g. because of assigned-clock-rates), this can
> > > happen w/o proper power domain being powered up yet.
> >
> > Issues with power on/off synchronization during genpd initializations
> > and genpd provider registration, certainly need to be fixed and I am
> > happy to help. However, my point is that I think it's a bad idea to
> > fix it through modelling the PM domain hierarchy in an incorrect way.
>
> So, which device should I pass to clk_register to handle runtime PM
> for the provided clocks? dispcc, should I not?

Right, anything but dispcc seems wrong.

> Then if the dispcc is not attached, we will have to manually handle
> MMCX from dispcc's runtime pm callbacks. Correct?

Yep - and we don't want that either.

>
> Could you please be more specific, why is it so wrong to attach dispcc
> to the MMCX genpd?

In the end it seems like I just needed to make my brain feel a little
more comfortable with the ideas that you put forward.

It should work fine, I think! My apologies for all the noise.

KInd regards
Uffe

Re: [RESEND PATCH v2 4/7] clk: qcom: gdsc: enable optional power domain support

[Dmitry Baryshkov]

On Fri, 9 Jul 2021 at 17:11, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 9 Jul 2021 at 15:22, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Fri, 9 Jul 2021 at 16:14, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Fri, 9 Jul 2021 at 14:59, Dmitry Baryshkov
> > > <dmitry.baryshkov@linaro.org> wrote:
> > > >
> > > > On Fri, 9 Jul 2021 at 15:18, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > On Fri, 9 Jul 2021 at 13:46, Dmitry Baryshkov
> > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > >
> > > > > > On Fri, 9 Jul 2021 at 12:33, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > > >
> > > > > > > On Fri, 9 Jul 2021 at 06:32, Dmitry Baryshkov
> > > > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > > > >
> > > > > > > > On sm8250 dispcc and videocc registers are powered up by the MMCX power
> > > > > > > > domain. Currently we used a regulator to enable this domain on demand,
> > > > > > > > however this has some consequences, as genpd code is not reentrant.
> > > > > > > >
> > > > > > > > Teach Qualcomm clock controller code about setting up power domains and
> > > > > > > > using them for gdsc control.
> > > > > > > >
> > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > > > > > > > index 51ed640e527b..9401d01533c8 100644
> > > > > > > > --- a/drivers/clk/qcom/gdsc.c
> > > > > > > > +++ b/drivers/clk/qcom/gdsc.c
> > > > > > > > @@ -427,6 +427,7 @@ int gdsc_register(struct gdsc_desc *desc,
> > > > > > > >                         continue;
> > > > > > > >                 scs[i]->regmap = regmap;
> > > > > > > >                 scs[i]->rcdev = rcdev;
> > > > > > > > +               scs[i]->pd.dev.parent = desc->dev;
> > > > > > > >                 ret = gdsc_init(scs[i]);
> > > > > > > >                 if (ret)
> > > > > > > >                         return ret;
> > > > > > > > @@ -439,6 +440,8 @@ int gdsc_register(struct gdsc_desc *desc,
> > > > > > > >                         continue;
> > > > > > > >                 if (scs[i]->parent)
> > > > > > > >                         pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
> > > > > > > > +               else if (!IS_ERR_OR_NULL(dev->pm_domain))
> > > > > > >
> > > > > > > So dev_pm_domain_attach() (which calls genpd_dev_pm_attach() is being
> > > > > > > called for gdsc platform device from the platform bus', to try to
> > > > > > > attach the device to its corresponding PM domain.
> > > > > > >
> > > > > > > Looking a bit closer to genpd_dev_pm_attach(), I realize that we
> > > > > > > shouldn't really try to attach a device to its PM domain, when its OF
> > > > > > > node (dev->of_node) contains a "#power-domain-cells" specifier. This
> > > > > > > is because it indicates that the device belongs to a genpd provider
> > > > > > > itself. In this case, a "power-domains" specifier tells that it has a
> > > > > > > parent domain.
> > > > > > >
> > > > > > > I will post a patch that fixes this asap.
> > > > > >
> > > > > > I think there is nothing to fix here. The dispcc/videocc drivers
> > > > > > provide clocks in addition to the gdsc power domain. And provided
> > > > > > clocks would definitely benefit from having the dispcc device being
> > > > > > attached to the power domain which governs clock registers (MMCX in
> > > > > > our case). Thus I think it is perfectly valid to have:
> > > > > >
> > > > > > rpmhpd device:
> > > > > >  - provides MMCX domain.
> > > > > >
> > > > > > dispcc device:
> > > > > >  - is attached to the MMCX domain,
> > > > >
> > > > > We don't need this, it's redundant and weird to me.
> > > > >
> > > > > Also I am kind of worried that you will hit another new path in genpd,
> > > > > causing locking issues etc, as it has not been designed to work like
> > > > > this (a provider device and a child domain sharing the same "parent").
> > > >
> > > > So, which domain should the dispcc device belong to? It's registers
> > > > are powered by the MMCX domain. I can not attach it to the child
> > > > (GDSC) domain either: in the case of videocc there are 4 child
> > > > domains.
> > >
> > > The dispcc device should *not* be attached to a PM domain.
> > >
> > > Instead it should be registered as a genpd provider and the
> > > corresponding PM domains it provides, should be assigned as child
> > > domains to the MMCX domain.
> > >
> > > This is exactly what the child/parent domain support in genpd is there
> > > to help with.
> >
> > This is done in this patchset. If we stop attaching dispcc to the MMCX
> > genpd, I'll have to locate it in a different way, but the idea is
> > implemented here.
>
> Right. Perhaps it's not such a bad idea after all as it gives you two things:
>
> 1) The handle to the MMCX PM domain, which makes sure it has been
> registered too before dispcc gets probed.
> 2) The possibility to control power for the MMCX PM domain via runtime
> PM for the dispcc device. This seems useful for your use case.
>
> >
> > > > An alternative would be to request that all users of the provided
> > > > clocks power on one of the child domains. However this is also not
> > > > perfect. If some generic code (e.g. clock framework) calls into
> > > > provided clocks (e.g. because of assigned-clock-rates), this can
> > > > happen w/o proper power domain being powered up yet.
> > >
> > > Issues with power on/off synchronization during genpd initializations
> > > and genpd provider registration, certainly need to be fixed and I am
> > > happy to help. However, my point is that I think it's a bad idea to
> > > fix it through modelling the PM domain hierarchy in an incorrect way.
> >
> > So, which device should I pass to clk_register to handle runtime PM
> > for the provided clocks? dispcc, should I not?
>
> Right, anything but dispcc seems wrong.
>
> > Then if the dispcc is not attached, we will have to manually handle
> > MMCX from dispcc's runtime pm callbacks. Correct?
>
> Yep - and we don't want that either.
>
> >
> > Could you please be more specific, why is it so wrong to attach dispcc
> > to the MMCX genpd?
>
> In the end it seems like I just needed to make my brain feel a little
> more comfortable with the ideas that you put forward.
>
> It should work fine, I think! My apologies for all the noise.

No problem, it is always better to have a discussion (and a
conclusion) rather than not to have it.

Thank you for your comments!


-- 
With best wishes
Dmitry

Re: [RESEND PATCH v2 4/7] clk: qcom: gdsc: enable optional power domain support

[Bjorn Andersson]

On Fri 09 Jul 08:14 CDT 2021, Ulf Hansson wrote:

> On Fri, 9 Jul 2021 at 14:59, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Fri, 9 Jul 2021 at 15:18, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Fri, 9 Jul 2021 at 13:46, Dmitry Baryshkov
> > > <dmitry.baryshkov@linaro.org> wrote:
> > > >
> > > > On Fri, 9 Jul 2021 at 12:33, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > On Fri, 9 Jul 2021 at 06:32, Dmitry Baryshkov
> > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > >
> > > > > > On sm8250 dispcc and videocc registers are powered up by the MMCX power
> > > > > > domain. Currently we used a regulator to enable this domain on demand,
> > > > > > however this has some consequences, as genpd code is not reentrant.
> > > > > >
> > > > > > Teach Qualcomm clock controller code about setting up power domains and
> > > > > > using them for gdsc control.
> > > > > >
> > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > >
> > > > > [...]
> > > > >
> > > > > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > > > > > index 51ed640e527b..9401d01533c8 100644
> > > > > > --- a/drivers/clk/qcom/gdsc.c
> > > > > > +++ b/drivers/clk/qcom/gdsc.c
> > > > > > @@ -427,6 +427,7 @@ int gdsc_register(struct gdsc_desc *desc,
> > > > > >                         continue;
> > > > > >                 scs[i]->regmap = regmap;
> > > > > >                 scs[i]->rcdev = rcdev;
> > > > > > +               scs[i]->pd.dev.parent = desc->dev;
> > > > > >                 ret = gdsc_init(scs[i]);
> > > > > >                 if (ret)
> > > > > >                         return ret;
> > > > > > @@ -439,6 +440,8 @@ int gdsc_register(struct gdsc_desc *desc,
> > > > > >                         continue;
> > > > > >                 if (scs[i]->parent)
> > > > > >                         pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
> > > > > > +               else if (!IS_ERR_OR_NULL(dev->pm_domain))
> > > > >
> > > > > So dev_pm_domain_attach() (which calls genpd_dev_pm_attach() is being
> > > > > called for gdsc platform device from the platform bus', to try to
> > > > > attach the device to its corresponding PM domain.
> > > > >
> > > > > Looking a bit closer to genpd_dev_pm_attach(), I realize that we
> > > > > shouldn't really try to attach a device to its PM domain, when its OF
> > > > > node (dev->of_node) contains a "#power-domain-cells" specifier. This
> > > > > is because it indicates that the device belongs to a genpd provider
> > > > > itself. In this case, a "power-domains" specifier tells that it has a
> > > > > parent domain.
> > > > >
> > > > > I will post a patch that fixes this asap.
> > > >
> > > > I think there is nothing to fix here. The dispcc/videocc drivers
> > > > provide clocks in addition to the gdsc power domain. And provided
> > > > clocks would definitely benefit from having the dispcc device being
> > > > attached to the power domain which governs clock registers (MMCX in
> > > > our case). Thus I think it is perfectly valid to have:
> > > >
> > > > rpmhpd device:
> > > >  - provides MMCX domain.
> > > >
> > > > dispcc device:
> > > >  - is attached to the MMCX domain,
> > >
> > > We don't need this, it's redundant and weird to me.
> > >
> > > Also I am kind of worried that you will hit another new path in genpd,
> > > causing locking issues etc, as it has not been designed to work like
> > > this (a provider device and a child domain sharing the same "parent").
> >
> > So, which domain should the dispcc device belong to? It's registers
> > are powered by the MMCX domain. I can not attach it to the child
> > (GDSC) domain either: in the case of videocc there are 4 child
> > domains.
> 
> The dispcc device should *not* be attached to a PM domain.
> 

dispcc is powered by the MMCX power domain, so it needs to be on if you
want to touch the registers.

I presume that for genpd this might not be a problem as long as all the
exposed power domains are parented by the genpd provider's parent, as
the core would turn the parent on before and turn off after performing
those operations. But without attaching to the domain we don't have
power to get through probe/registration.

Further more, dispcc is also a clock driver and there's certainly
operations where the genpd framework won't save us.

> Instead it should be registered as a genpd provider and the
> corresponding PM domains it provides, should be assigned as child
> domains to the MMCX domain.
> 

Right, this relationship is today missing and is what Dmitry needs to
add - so that the parent domains stays powered even when we're not
keeping the parent domain enabled to poke the dispcc.

> This is exactly what the child/parent domain support in genpd is there
> to help with.
> 
> > An alternative would be to request that all users of the provided
> > clocks power on one of the child domains. However this is also not
> > perfect. If some generic code (e.g. clock framework) calls into
> > provided clocks (e.g. because of assigned-clock-rates), this can
> > happen w/o proper power domain being powered up yet.
> 
> Issues with power on/off synchronization during genpd initializations
> and genpd provider registration, certainly need to be fixed and I am
> happy to help. However, my point is that I think it's a bad idea to
> fix it through modelling the PM domain hierarchy in an incorrect way.
> 

This was my initial feeling to the patch as well and I think it might be
better to push the pm_runtime_get/put operations into gdsc.c - in
particular if you're saying that the general case is not for the genpd
provider itself to be powered by the specified parent domain.

At least we could start by doing it manually in gdsc.c and possibly move
it into the framework if we're confident that it's a good idea.

Regards,
Bjorn

Re: [RESEND PATCH v2 4/7] clk: qcom: gdsc: enable optional power domain support

[Ulf Hansson]

On Fri, 9 Jul 2021 at 16:04, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
>
> On Fri 09 Jul 08:14 CDT 2021, Ulf Hansson wrote:
>
> > On Fri, 9 Jul 2021 at 14:59, Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > On Fri, 9 Jul 2021 at 15:18, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > On Fri, 9 Jul 2021 at 13:46, Dmitry Baryshkov
> > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > >
> > > > > On Fri, 9 Jul 2021 at 12:33, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > >
> > > > > > On Fri, 9 Jul 2021 at 06:32, Dmitry Baryshkov
> > > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > > >
> > > > > > > On sm8250 dispcc and videocc registers are powered up by the MMCX power
> > > > > > > domain. Currently we used a regulator to enable this domain on demand,
> > > > > > > however this has some consequences, as genpd code is not reentrant.
> > > > > > >
> > > > > > > Teach Qualcomm clock controller code about setting up power domains and
> > > > > > > using them for gdsc control.
> > > > > > >
> > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > >
> > > > > > [...]
> > > > > >
> > > > > > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > > > > > > index 51ed640e527b..9401d01533c8 100644
> > > > > > > --- a/drivers/clk/qcom/gdsc.c
> > > > > > > +++ b/drivers/clk/qcom/gdsc.c
> > > > > > > @@ -427,6 +427,7 @@ int gdsc_register(struct gdsc_desc *desc,
> > > > > > >                         continue;
> > > > > > >                 scs[i]->regmap = regmap;
> > > > > > >                 scs[i]->rcdev = rcdev;
> > > > > > > +               scs[i]->pd.dev.parent = desc->dev;
> > > > > > >                 ret = gdsc_init(scs[i]);
> > > > > > >                 if (ret)
> > > > > > >                         return ret;
> > > > > > > @@ -439,6 +440,8 @@ int gdsc_register(struct gdsc_desc *desc,
> > > > > > >                         continue;
> > > > > > >                 if (scs[i]->parent)
> > > > > > >                         pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
> > > > > > > +               else if (!IS_ERR_OR_NULL(dev->pm_domain))
> > > > > >
> > > > > > So dev_pm_domain_attach() (which calls genpd_dev_pm_attach() is being
> > > > > > called for gdsc platform device from the platform bus', to try to
> > > > > > attach the device to its corresponding PM domain.
> > > > > >
> > > > > > Looking a bit closer to genpd_dev_pm_attach(), I realize that we
> > > > > > shouldn't really try to attach a device to its PM domain, when its OF
> > > > > > node (dev->of_node) contains a "#power-domain-cells" specifier. This
> > > > > > is because it indicates that the device belongs to a genpd provider
> > > > > > itself. In this case, a "power-domains" specifier tells that it has a
> > > > > > parent domain.
> > > > > >
> > > > > > I will post a patch that fixes this asap.
> > > > >
> > > > > I think there is nothing to fix here. The dispcc/videocc drivers
> > > > > provide clocks in addition to the gdsc power domain. And provided
> > > > > clocks would definitely benefit from having the dispcc device being
> > > > > attached to the power domain which governs clock registers (MMCX in
> > > > > our case). Thus I think it is perfectly valid to have:
> > > > >
> > > > > rpmhpd device:
> > > > >  - provides MMCX domain.
> > > > >
> > > > > dispcc device:
> > > > >  - is attached to the MMCX domain,
> > > >
> > > > We don't need this, it's redundant and weird to me.
> > > >
> > > > Also I am kind of worried that you will hit another new path in genpd,
> > > > causing locking issues etc, as it has not been designed to work like
> > > > this (a provider device and a child domain sharing the same "parent").
> > >
> > > So, which domain should the dispcc device belong to? It's registers
> > > are powered by the MMCX domain. I can not attach it to the child
> > > (GDSC) domain either: in the case of videocc there are 4 child
> > > domains.
> >
> > The dispcc device should *not* be attached to a PM domain.
> >
>
> dispcc is powered by the MMCX power domain, so it needs to be on if you
> want to touch the registers.
>
> I presume that for genpd this might not be a problem as long as all the
> exposed power domains are parented by the genpd provider's parent, as
> the core would turn the parent on before and turn off after performing
> those operations. But without attaching to the domain we don't have
> power to get through probe/registration.
>
> Further more, dispcc is also a clock driver and there's certainly
> operations where the genpd framework won't save us.
>
> > Instead it should be registered as a genpd provider and the
> > corresponding PM domains it provides, should be assigned as child
> > domains to the MMCX domain.
> >
>
> Right, this relationship is today missing and is what Dmitry needs to
> add - so that the parent domains stays powered even when we're not
> keeping the parent domain enabled to poke the dispcc.
>
> > This is exactly what the child/parent domain support in genpd is there
> > to help with.
> >
> > > An alternative would be to request that all users of the provided
> > > clocks power on one of the child domains. However this is also not
> > > perfect. If some generic code (e.g. clock framework) calls into
> > > provided clocks (e.g. because of assigned-clock-rates), this can
> > > happen w/o proper power domain being powered up yet.
> >
> > Issues with power on/off synchronization during genpd initializations
> > and genpd provider registration, certainly need to be fixed and I am
> > happy to help. However, my point is that I think it's a bad idea to
> > fix it through modelling the PM domain hierarchy in an incorrect way.
> >
>
> This was my initial feeling to the patch as well and I think it might be
> better to push the pm_runtime_get/put operations into gdsc.c - in
> particular if you're saying that the general case is not for the genpd
> provider itself to be powered by the specified parent domain.
>
> At least we could start by doing it manually in gdsc.c and possibly move
> it into the framework if we're confident that it's a good idea.

Yes, better to start making this Qcom specific, then we can take it from there.

Kind regards
Uffe

Re: [RESEND PATCH v2 4/7] clk: qcom: gdsc: enable optional power domain support

[Dmitry Baryshkov]

On Fri, 9 Jul 2021 at 17:13, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Fri, 9 Jul 2021 at 16:04, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
> >
> > On Fri 09 Jul 08:14 CDT 2021, Ulf Hansson wrote:
> >
> > > On Fri, 9 Jul 2021 at 14:59, Dmitry Baryshkov
> > > <dmitry.baryshkov@linaro.org> wrote:
> > > >
> > > > On Fri, 9 Jul 2021 at 15:18, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > On Fri, 9 Jul 2021 at 13:46, Dmitry Baryshkov
> > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > >
> > > > > > On Fri, 9 Jul 2021 at 12:33, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > > > >
> > > > > > > On Fri, 9 Jul 2021 at 06:32, Dmitry Baryshkov
> > > > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > > > >
> > > > > > > > On sm8250 dispcc and videocc registers are powered up by the MMCX power
> > > > > > > > domain. Currently we used a regulator to enable this domain on demand,
> > > > > > > > however this has some consequences, as genpd code is not reentrant.
> > > > > > > >
> > > > > > > > Teach Qualcomm clock controller code about setting up power domains and
> > > > > > > > using them for gdsc control.
> > > > > > > >
> > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > >
> > > > > > > [...]
> > > > > > >
> > > > > > > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > > > > > > > index 51ed640e527b..9401d01533c8 100644
> > > > > > > > --- a/drivers/clk/qcom/gdsc.c
> > > > > > > > +++ b/drivers/clk/qcom/gdsc.c
> > > > > > > > @@ -427,6 +427,7 @@ int gdsc_register(struct gdsc_desc *desc,
> > > > > > > >                         continue;
> > > > > > > >                 scs[i]->regmap = regmap;
> > > > > > > >                 scs[i]->rcdev = rcdev;
> > > > > > > > +               scs[i]->pd.dev.parent = desc->dev;
> > > > > > > >                 ret = gdsc_init(scs[i]);
> > > > > > > >                 if (ret)
> > > > > > > >                         return ret;
> > > > > > > > @@ -439,6 +440,8 @@ int gdsc_register(struct gdsc_desc *desc,
> > > > > > > >                         continue;
> > > > > > > >                 if (scs[i]->parent)
> > > > > > > >                         pm_genpd_add_subdomain(scs[i]->parent, &scs[i]->pd);
> > > > > > > > +               else if (!IS_ERR_OR_NULL(dev->pm_domain))
> > > > > > >
> > > > > > > So dev_pm_domain_attach() (which calls genpd_dev_pm_attach() is being
> > > > > > > called for gdsc platform device from the platform bus', to try to
> > > > > > > attach the device to its corresponding PM domain.
> > > > > > >
> > > > > > > Looking a bit closer to genpd_dev_pm_attach(), I realize that we
> > > > > > > shouldn't really try to attach a device to its PM domain, when its OF
> > > > > > > node (dev->of_node) contains a "#power-domain-cells" specifier. This
> > > > > > > is because it indicates that the device belongs to a genpd provider
> > > > > > > itself. In this case, a "power-domains" specifier tells that it has a
> > > > > > > parent domain.
> > > > > > >
> > > > > > > I will post a patch that fixes this asap.
> > > > > >
> > > > > > I think there is nothing to fix here. The dispcc/videocc drivers
> > > > > > provide clocks in addition to the gdsc power domain. And provided
> > > > > > clocks would definitely benefit from having the dispcc device being
> > > > > > attached to the power domain which governs clock registers (MMCX in
> > > > > > our case). Thus I think it is perfectly valid to have:
> > > > > >
> > > > > > rpmhpd device:
> > > > > >  - provides MMCX domain.
> > > > > >
> > > > > > dispcc device:
> > > > > >  - is attached to the MMCX domain,
> > > > >
> > > > > We don't need this, it's redundant and weird to me.
> > > > >
> > > > > Also I am kind of worried that you will hit another new path in genpd,
> > > > > causing locking issues etc, as it has not been designed to work like
> > > > > this (a provider device and a child domain sharing the same "parent").
> > > >
> > > > So, which domain should the dispcc device belong to? It's registers
> > > > are powered by the MMCX domain. I can not attach it to the child
> > > > (GDSC) domain either: in the case of videocc there are 4 child
> > > > domains.
> > >
> > > The dispcc device should *not* be attached to a PM domain.
> > >
> >
> > dispcc is powered by the MMCX power domain, so it needs to be on if you
> > want to touch the registers.
> >
> > I presume that for genpd this might not be a problem as long as all the
> > exposed power domains are parented by the genpd provider's parent, as
> > the core would turn the parent on before and turn off after performing
> > those operations. But without attaching to the domain we don't have
> > power to get through probe/registration.
> >
> > Further more, dispcc is also a clock driver and there's certainly
> > operations where the genpd framework won't save us.
> >
> > > Instead it should be registered as a genpd provider and the
> > > corresponding PM domains it provides, should be assigned as child
> > > domains to the MMCX domain.
> > >
> >
> > Right, this relationship is today missing and is what Dmitry needs to
> > add - so that the parent domains stays powered even when we're not
> > keeping the parent domain enabled to poke the dispcc.
> >
> > > This is exactly what the child/parent domain support in genpd is there
> > > to help with.
> > >
> > > > An alternative would be to request that all users of the provided
> > > > clocks power on one of the child domains. However this is also not
> > > > perfect. If some generic code (e.g. clock framework) calls into
> > > > provided clocks (e.g. because of assigned-clock-rates), this can
> > > > happen w/o proper power domain being powered up yet.
> > >
> > > Issues with power on/off synchronization during genpd initializations
> > > and genpd provider registration, certainly need to be fixed and I am
> > > happy to help. However, my point is that I think it's a bad idea to
> > > fix it through modelling the PM domain hierarchy in an incorrect way.
> > >
> >
> > This was my initial feeling to the patch as well and I think it might be
> > better to push the pm_runtime_get/put operations into gdsc.c - in
> > particular if you're saying that the general case is not for the genpd
> > provider itself to be powered by the specified parent domain.
> >
> > At least we could start by doing it manually in gdsc.c and possibly move
> > it into the framework if we're confident that it's a good idea.
>
> Yes, better to start making this Qcom specific, then we can take it from there.

I will re-add pm_runtime calls to gdsc.c and send a v3.

-- 
With best wishes
Dmitry

[RESEND PATCH v2 5/7] arm64: dts: qcom: sm8250: remove mmcx regulator

[Dmitry Baryshkov]

Switch dispcc and videocc into using MMCX domain directly. Drop the now
unused mmcx regulator.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8250.dtsi | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index 4c0de12aaba6..2a468b85dc09 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -271,13 +271,6 @@ memory@80000000 {
 		reg = <0x0 0x80000000 0x0 0x0>;
 	};
 
-	mmcx_reg: mmcx-reg {
-		compatible = "regulator-fixed-domain";
-		power-domains = <&rpmhpd SM8250_MMCX>;
-		required-opps = <&rpmhpd_opp_low_svs>;
-		regulator-name = "MMCX";
-	};
-
 	pmu {
 		compatible = "arm,armv8-pmuv3";
 		interrupts = <GIC_PPI 7 IRQ_TYPE_LEVEL_LOW>;
@@ -2362,7 +2355,7 @@ videocc: clock-controller@abf0000 {
 			clocks = <&gcc GCC_VIDEO_AHB_CLK>,
 				 <&rpmhcc RPMH_CXO_CLK>,
 				 <&rpmhcc RPMH_CXO_CLK_A>;
-			mmcx-supply = <&mmcx_reg>;
+			power-domains = <&rpmhpd SM8250_MMCX>;
 			clock-names = "iface", "bi_tcxo", "bi_tcxo_ao";
 			#clock-cells = <1>;
 			#reset-cells = <1>;
@@ -2627,7 +2620,7 @@ opp-358000000 {
 		dispcc: clock-controller@af00000 {
 			compatible = "qcom,sm8250-dispcc";
 			reg = <0 0x0af00000 0 0x10000>;
-			mmcx-supply = <&mmcx_reg>;
+			power-domains = <&rpmhpd SM8250_MMCX>;
 			clocks = <&rpmhcc RPMH_CXO_CLK>,
 				 <&dsi0_phy 0>,
 				 <&dsi0_phy 1>,
-- 
2.30.2

[RESEND PATCH v2 6/7] clk: qcom: dispcc-sm8250: stop using mmcx regulator

[Dmitry Baryshkov]

Now as the common qcom clock controller code has been taught about power
domains, stop mentioning mmcx supply as a way to power up the clock
controller's gdsc.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/dispcc-sm8250.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/clk/qcom/dispcc-sm8250.c b/drivers/clk/qcom/dispcc-sm8250.c
index de09cd5c209f..dfbfe64b12f6 100644
--- a/drivers/clk/qcom/dispcc-sm8250.c
+++ b/drivers/clk/qcom/dispcc-sm8250.c
@@ -955,7 +955,6 @@ static struct gdsc mdss_gdsc = {
 	},
 	.pwrsts = PWRSTS_OFF_ON,
 	.flags = HW_CTRL,
-	.supply = "mmcx",
 };
 
 static struct clk_regmap *disp_cc_sm8250_clocks[] = {
-- 
2.30.2

[RESEND PATCH v2 7/7] clk: qcom: videocc-sm8250: stop using mmcx regulator

[Dmitry Baryshkov]

Now as the common qcom clock controller code has been taught about power
domains, stop mentioning mmcx supply as a way to power up the clock
controller's gdscs.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/videocc-sm8250.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/clk/qcom/videocc-sm8250.c b/drivers/clk/qcom/videocc-sm8250.c
index 7b435a1c2c4b..eedef85d90e5 100644
--- a/drivers/clk/qcom/videocc-sm8250.c
+++ b/drivers/clk/qcom/videocc-sm8250.c
@@ -276,7 +276,6 @@ static struct gdsc mvs0c_gdsc = {
 	},
 	.flags = 0,
 	.pwrsts = PWRSTS_OFF_ON,
-	.supply = "mmcx",
 };
 
 static struct gdsc mvs1c_gdsc = {
@@ -286,7 +285,6 @@ static struct gdsc mvs1c_gdsc = {
 	},
 	.flags = 0,
 	.pwrsts = PWRSTS_OFF_ON,
-	.supply = "mmcx",
 };
 
 static struct gdsc mvs0_gdsc = {
@@ -296,7 +294,6 @@ static struct gdsc mvs0_gdsc = {
 	},
 	.flags = HW_CTRL,
 	.pwrsts = PWRSTS_OFF_ON,
-	.supply = "mmcx",
 };
 
 static struct gdsc mvs1_gdsc = {
@@ -306,7 +303,6 @@ static struct gdsc mvs1_gdsc = {
 	},
 	.flags = HW_CTRL,
 	.pwrsts = PWRSTS_OFF_ON,
-	.supply = "mmcx",
 };
 
 static struct clk_regmap *video_cc_sm8250_clocks[] = {
-- 
2.30.2