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

[PATCH v3 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)

Changes since v2:
 - Move pm_runtime calls from generic genpd code to the gdsc code for
   now (as suggested by Ulf & Bjorn)

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
      clk: qcom: gdsc: enable optional power domain support
      clk: qcom: gdsc: call runtime PM functions for the provider device
      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/clk/qcom/common.c                          | 37 ++++++++++--
 drivers/clk/qcom/dispcc-sm8250.c                   |  1 -
 drivers/clk/qcom/gdsc.c                            | 70 ++++++++++++++++++++--
 drivers/clk/qcom/gdsc.h                            |  2 +
 drivers/clk/qcom/videocc-sm8250.c                  |  4 --
 8 files changed, 115 insertions(+), 24 deletions(-)

[PATCH v3 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

[PATCH v3 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

[PATCH v3 3/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 use 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 runtime PM and
using specified for gdsc powerup.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/common.c | 37 +++++++++++++++++++++++++++++++------
 drivers/clk/qcom/gdsc.c   |  4 ++++
 2 files changed, 35 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..ccd36617d067 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -439,6 +439,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 +459,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

[PATCH v3 4/7] clk: qcom: gdsc: call runtime PM functions for the provider device

[Dmitry Baryshkov]

In order to properly handle runtime PM status of the provider device,
call pm_runtime_get/pm_runtime_put on the clock controller device.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/gdsc.c | 66 ++++++++++++++++++++++++++++++++++++++---
 drivers/clk/qcom/gdsc.h |  2 ++
 2 files changed, 64 insertions(+), 4 deletions(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index ccd36617d067..6bec31fccb09 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -11,6 +11,7 @@
 #include <linux/kernel.h>
 #include <linux/ktime.h>
 #include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/reset-controller.h>
@@ -50,6 +51,30 @@ enum gdsc_status {
 	GDSC_ON
 };
 
+static int gdsc_pm_runtime_get(struct gdsc *sc)
+{
+	int ret;
+
+	if (!sc->rpm_dev)
+		return 0;
+
+	ret = pm_runtime_get_sync(sc->rpm_dev);
+	if (ret < 0) {
+		pm_runtime_put_noidle(sc->rpm_dev);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int gdsc_pm_runtime_put(struct gdsc *sc)
+{
+	if (!sc->rpm_dev)
+		return 0;
+
+	return pm_runtime_put_sync(sc->rpm_dev);
+}
+
 /* Returns 1 if GDSC status is status, 0 if not, and < 0 on error */
 static int gdsc_check_status(struct gdsc *sc, enum gdsc_status status)
 {
@@ -232,9 +257,8 @@ static void gdsc_retain_ff_on(struct gdsc *sc)
 	regmap_update_bits(sc->regmap, sc->gdscr, mask, mask);
 }
 
-static int gdsc_enable(struct generic_pm_domain *domain)
+static int _gdsc_enable(struct gdsc *sc)
 {
-	struct gdsc *sc = domain_to_gdsc(domain);
 	int ret;
 
 	if (sc->pwrsts == PWRSTS_ON)
@@ -290,11 +314,28 @@ static int gdsc_enable(struct generic_pm_domain *domain)
 	return 0;
 }
 
-static int gdsc_disable(struct generic_pm_domain *domain)
+static int gdsc_enable(struct generic_pm_domain *domain)
 {
 	struct gdsc *sc = domain_to_gdsc(domain);
 	int ret;
 
+	ret = gdsc_pm_runtime_get(sc);
+	if (ret)
+		return ret;
+
+	ret = _gdsc_enable(sc);
+	if (ret) {
+		gdsc_pm_runtime_put(sc);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int _gdsc_disable(struct gdsc *sc)
+{
+	int ret;
+
 	if (sc->pwrsts == PWRSTS_ON)
 		return gdsc_assert_reset(sc);
 
@@ -329,6 +370,18 @@ static int gdsc_disable(struct generic_pm_domain *domain)
 	return 0;
 }
 
+static int gdsc_disable(struct generic_pm_domain *domain)
+{
+	struct gdsc *sc = domain_to_gdsc(domain);
+	int ret;
+
+	ret = _gdsc_disable(sc);
+	if (ret)
+		return ret;
+
+	return gdsc_pm_runtime_put(sc);
+}
+
 static int gdsc_init(struct gdsc *sc)
 {
 	u32 mask, val;
@@ -425,6 +478,8 @@ int gdsc_register(struct gdsc_desc *desc,
 	for (i = 0; i < num; i++) {
 		if (!scs[i])
 			continue;
+		if (pm_runtime_enabled(dev))
+			scs[i]->rpm_dev = dev;
 		scs[i]->regmap = regmap;
 		scs[i]->rcdev = rcdev;
 		ret = gdsc_init(scs[i]);
@@ -486,7 +541,10 @@ void gdsc_unregister(struct gdsc_desc *desc)
  */
 int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain)
 {
+	struct gdsc *sc = domain_to_gdsc(domain);
+
 	/* Do nothing but give genpd the impression that we were successful */
-	return 0;
+	/* Get the runtime PM device only */
+	return gdsc_pm_runtime_get(sc);
 }
 EXPORT_SYMBOL_GPL(gdsc_gx_do_nothing_enable);
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index 5bb396b344d1..a82982df0a55 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -25,6 +25,7 @@ struct reset_controller_dev;
  * @resets: ids of resets associated with this gdsc
  * @reset_count: number of @resets
  * @rcdev: reset controller
+ * @rpm_dev: runtime PM device
  */
 struct gdsc {
 	struct generic_pm_domain	pd;
@@ -58,6 +59,7 @@ struct gdsc {
 
 	const char 			*supply;
 	struct regulator		*rsupply;
+	struct device 			*rpm_dev;
 };
 
 struct gdsc_desc {
-- 
2.30.2

[PATCH v3 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

[PATCH v3 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

[PATCH v3 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

Re: [PATCH v3 3/7] clk: qcom: gdsc: enable optional power domain support

[Bjorn Andersson]

On Fri 09 Jul 12:31 CDT 2021, Dmitry Baryshkov wrote:

> On sm8250 dispcc and videocc registers are powered up by the MMCX power
> domain. Currently we use 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 runtime PM and
> using specified for gdsc powerup.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/clk/qcom/common.c | 37 +++++++++++++++++++++++++++++++------
>  drivers/clk/qcom/gdsc.c   |  4 ++++
>  2 files changed, 35 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);

In turingcc-qcs404.c I'm using pm_runtime to have the clock framework
ensure that the iface clock is enabled during clock operations, so this
will result in a "unbalanced enable" warning.

> +	ret = pm_runtime_get_sync(dev);

I don't think you should wrap the entire initialization in a
pm_runtime_get_sync()/put() region. Instead follow the clock framework
and wrap gdsc initialization that needs to touch the hardware in:

	if (pm_runtime_enabled())
		pm_runtime_get_sync();

I do however think that as of this patch, when probe returns MMCX might
very well be turned off, as the only user (this driver) has pm_runtime
enabled and it's idle. So I think you should introduce the
pm_runtime_get()/put() in the gdsc functions before this patch.


To summarize, I think you should rely on the individual clock drivers to
pm_runtime_enable()/disable().

> +	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..ccd36617d067 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -439,6 +439,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);

Nice, I didn't know that we could fish it out of the dev.

Regards,
Bjorn

>  	}
>  
>  	return of_genpd_add_provider_onecell(dev->of_node, data);
> @@ -457,6 +459,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: [PATCH v3 4/7] clk: qcom: gdsc: call runtime PM functions for the provider device

[Bjorn Andersson]

On Fri 09 Jul 12:31 CDT 2021, Dmitry Baryshkov wrote:

> In order to properly handle runtime PM status of the provider device,
> call pm_runtime_get/pm_runtime_put on the clock controller device.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/clk/qcom/gdsc.c | 66 ++++++++++++++++++++++++++++++++++++++---
>  drivers/clk/qcom/gdsc.h |  2 ++
>  2 files changed, 64 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index ccd36617d067..6bec31fccb09 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -11,6 +11,7 @@
>  #include <linux/kernel.h>
>  #include <linux/ktime.h>
>  #include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/reset-controller.h>
> @@ -50,6 +51,30 @@ enum gdsc_status {
>  	GDSC_ON
>  };
>  
> +static int gdsc_pm_runtime_get(struct gdsc *sc)
> +{
> +	int ret;
> +
> +	if (!sc->rpm_dev)
> +		return 0;
> +
> +	ret = pm_runtime_get_sync(sc->rpm_dev);
> +	if (ret < 0) {
> +		pm_runtime_put_noidle(sc->rpm_dev);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int gdsc_pm_runtime_put(struct gdsc *sc)
> +{
> +	if (!sc->rpm_dev)
> +		return 0;
> +
> +	return pm_runtime_put_sync(sc->rpm_dev);
> +}
> +
>  /* Returns 1 if GDSC status is status, 0 if not, and < 0 on error */
>  static int gdsc_check_status(struct gdsc *sc, enum gdsc_status status)
>  {
> @@ -232,9 +257,8 @@ static void gdsc_retain_ff_on(struct gdsc *sc)
>  	regmap_update_bits(sc->regmap, sc->gdscr, mask, mask);
>  }
>  
> -static int gdsc_enable(struct generic_pm_domain *domain)
> +static int _gdsc_enable(struct gdsc *sc)
>  {
> -	struct gdsc *sc = domain_to_gdsc(domain);
>  	int ret;
>  
>  	if (sc->pwrsts == PWRSTS_ON)
> @@ -290,11 +314,28 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>  	return 0;
>  }
>  
> -static int gdsc_disable(struct generic_pm_domain *domain)
> +static int gdsc_enable(struct generic_pm_domain *domain)
>  {
>  	struct gdsc *sc = domain_to_gdsc(domain);
>  	int ret;
>  
> +	ret = gdsc_pm_runtime_get(sc);
> +	if (ret)
> +		return ret;
> +
> +	ret = _gdsc_enable(sc);
> +	if (ret) {
> +		gdsc_pm_runtime_put(sc);

I presume what you do here is to leave the pm_runtime state of dispcc
active if we succeeded in enabling the gdsc. But the gdsc is a subdomain
of the parent domain, so the framework should take case of its
dependency.

So the reason for gdsc_pm_runtime_get()/put() in this code path is so
that you can access the dispcc registers, i.e. I think you should
get()/put() regardless of the return value.

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int _gdsc_disable(struct gdsc *sc)
> +{
> +	int ret;
> +
>  	if (sc->pwrsts == PWRSTS_ON)
>  		return gdsc_assert_reset(sc);
>  
> @@ -329,6 +370,18 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>  	return 0;
>  }
>  
> +static int gdsc_disable(struct generic_pm_domain *domain)
> +{
> +	struct gdsc *sc = domain_to_gdsc(domain);
> +	int ret;
> +

If the gdsc is found to be on at initialization, the next operation that
will happen is gdsc_disable() and as you didn't activate the pm_runtime
state in gdsc_init() you would in theory get here with registers
unaccessible.

In practice though, the active gdsc should through the being a subdomain
of the parent domain keep power on for you, so you won't notice this
issue.

But as above, I think you should wrap _gdsc_disable() in a get()/put()
pair.

> +	ret = _gdsc_disable(sc);
> +	if (ret)
> +		return ret;
> +
> +	return gdsc_pm_runtime_put(sc);
> +}
> +
>  static int gdsc_init(struct gdsc *sc)
>  {
>  	u32 mask, val;
> @@ -425,6 +478,8 @@ int gdsc_register(struct gdsc_desc *desc,
>  	for (i = 0; i < num; i++) {
>  		if (!scs[i])
>  			continue;
> +		if (pm_runtime_enabled(dev))
> +			scs[i]->rpm_dev = dev;
>  		scs[i]->regmap = regmap;
>  		scs[i]->rcdev = rcdev;
>  		ret = gdsc_init(scs[i]);
> @@ -486,7 +541,10 @@ void gdsc_unregister(struct gdsc_desc *desc)
>   */
>  int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain)
>  {
> +	struct gdsc *sc = domain_to_gdsc(domain);
> +
>  	/* Do nothing but give genpd the impression that we were successful */
> -	return 0;
> +	/* Get the runtime PM device only */
> +	return gdsc_pm_runtime_get(sc);

Per above, if you let the framework deal with the gdsc's dependencies on
the parent domain and you only get()/put() for the sake of dispcc then
you don't need you don't need to do this to keep the subsequent
gdsc_disable() in balance.

>  }
>  EXPORT_SYMBOL_GPL(gdsc_gx_do_nothing_enable);
> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> index 5bb396b344d1..a82982df0a55 100644
> --- a/drivers/clk/qcom/gdsc.h
> +++ b/drivers/clk/qcom/gdsc.h
> @@ -25,6 +25,7 @@ struct reset_controller_dev;
>   * @resets: ids of resets associated with this gdsc
>   * @reset_count: number of @resets
>   * @rcdev: reset controller
> + * @rpm_dev: runtime PM device
>   */
>  struct gdsc {
>  	struct generic_pm_domain	pd;
> @@ -58,6 +59,7 @@ struct gdsc {
>  
>  	const char 			*supply;
>  	struct regulator		*rsupply;
> +	struct device 			*rpm_dev;

This isn't just the "runtime pm device", it's the device this gdsc is
associated with. So "dev" sounds sufficient to me, but that requires
that you have a separate bool rpm_enabled to remember if
pm_runtime_enabled() was true during probe.

So unless we need "dev" for something else this might be sufficient.

Regards,
Bjorn

>  };
>  
>  struct gdsc_desc {
> -- 
> 2.30.2
> 

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

[Bjorn Andersson]

On Fri 09 Jul 12:31 CDT 2021, Dmitry Baryshkov wrote:

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

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

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

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

[Bjorn Andersson]

On Fri 09 Jul 12:31 CDT 2021, Dmitry Baryshkov wrote:

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

Reviewed-by: Bjorn Andersson <bjorn.andersson@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
> 

Re: [PATCH v3 5/7] arm64: dts: qcom: sm8250: remove mmcx regulator

[Bjorn Andersson]

On Fri 09 Jul 12:32 CDT 2021, Dmitry Baryshkov wrote:

> Switch dispcc and videocc into using MMCX domain directly. Drop the now
> unused mmcx regulator.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

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

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

[Bjorn Andersson]

On Fri 09 Jul 12:32 CDT 2021, Dmitry Baryshkov wrote:

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

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

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

Re: [PATCH v3 7/7] clk: qcom: videocc-sm8250: stop using mmcx regulator

[Bjorn Andersson]

On Fri 09 Jul 12:32 CDT 2021, Dmitry Baryshkov wrote:

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

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

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

Re: [PATCH v3 3/7] clk: qcom: gdsc: enable optional power domain support

[Dmitry Baryshkov]

On 09/07/2021 21:38, Bjorn Andersson wrote:
> On Fri 09 Jul 12:31 CDT 2021, Dmitry Baryshkov wrote:
> 
>> On sm8250 dispcc and videocc registers are powered up by the MMCX power
>> domain. Currently we use 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 runtime PM and
>> using specified for gdsc powerup.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/clk/qcom/common.c | 37 +++++++++++++++++++++++++++++++------
>>   drivers/clk/qcom/gdsc.c   |  4 ++++
>>   2 files changed, 35 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);
> 
> In turingcc-qcs404.c I'm using pm_runtime to have the clock framework
> ensure that the iface clock is enabled during clock operations, so this
> will result in a "unbalanced enable" warning.

And later I register the disabler:

  ret = devm_add_action_or_reset(dev, qcom_cc_pm_runtime_disable, dev);

You might want to add this to qcs404 code.

> 
>> +	ret = pm_runtime_get_sync(dev);
> 
> I don't think you should wrap the entire initialization in a
> pm_runtime_get_sync()/put() region. Instead follow the clock framework
> and wrap gdsc initialization that needs to touch the hardware in:

Init should be wrapped in the pm_runtime_get/put calls, so that the MMCX 
domain is on through the fall init sequence. Otherwise it can get turned 
off during it, boom, failed register access and reboot.

> 
> 	if (pm_runtime_enabled())
> 		pm_runtime_get_sync();

I don't think it's worth doing that. Having single lock for the whole 
init sequence is safer (and cleaner).

If you check other pm-enabled drivers, they would either call 
pm_runtime_enable at the end of the probe or get_sync in the beginning 
of the probe and put_FOO in the end. In this driver calling 
pm_runtime_enable() at the end of the probe function will not work, 
since this way clk subsystem will not pick up the device for runtime 
power management (as pm_runtime_enabled() would return false).

> 
> I do however think that as of this patch, when probe returns MMCX might
> very well be turned off, as the only user (this driver) has pm_runtime
> enabled and it's idle. So I think you should introduce the
> pm_runtime_get()/put() in the gdsc functions before this patch.

Maybe I'd just squash them together.

> 
> 
> To summarize, I think you should rely on the individual clock drivers to
> pm_runtime_enable()/disable().
> 
>> +	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..ccd36617d067 100644
>> --- a/drivers/clk/qcom/gdsc.c
>> +++ b/drivers/clk/qcom/gdsc.c
>> @@ -439,6 +439,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);
> 
> Nice, I didn't know that we could fish it out of the dev.
> 
> Regards,
> Bjorn
> 
>>   	}
>>   
>>   	return of_genpd_add_provider_onecell(dev->of_node, data);
>> @@ -457,6 +459,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
>>


-- 
With best wishes
Dmitry

Re: [PATCH v3 4/7] clk: qcom: gdsc: call runtime PM functions for the provider device

[Dmitry Baryshkov]

On 09/07/2021 21:54, Bjorn Andersson wrote:
> On Fri 09 Jul 12:31 CDT 2021, Dmitry Baryshkov wrote:
> 
>> In order to properly handle runtime PM status of the provider device,
>> call pm_runtime_get/pm_runtime_put on the clock controller device.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/clk/qcom/gdsc.c | 66 ++++++++++++++++++++++++++++++++++++++---
>>   drivers/clk/qcom/gdsc.h |  2 ++
>>   2 files changed, 64 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
>> index ccd36617d067..6bec31fccb09 100644
>> --- a/drivers/clk/qcom/gdsc.c
>> +++ b/drivers/clk/qcom/gdsc.c
>> @@ -11,6 +11,7 @@
>>   #include <linux/kernel.h>
>>   #include <linux/ktime.h>
>>   #include <linux/pm_domain.h>
>> +#include <linux/pm_runtime.h>
>>   #include <linux/regmap.h>
>>   #include <linux/regulator/consumer.h>
>>   #include <linux/reset-controller.h>
>> @@ -50,6 +51,30 @@ enum gdsc_status {
>>   	GDSC_ON
>>   };
>>   
>> +static int gdsc_pm_runtime_get(struct gdsc *sc)
>> +{
>> +	int ret;
>> +
>> +	if (!sc->rpm_dev)
>> +		return 0;
>> +
>> +	ret = pm_runtime_get_sync(sc->rpm_dev);
>> +	if (ret < 0) {
>> +		pm_runtime_put_noidle(sc->rpm_dev);
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int gdsc_pm_runtime_put(struct gdsc *sc)
>> +{
>> +	if (!sc->rpm_dev)
>> +		return 0;
>> +
>> +	return pm_runtime_put_sync(sc->rpm_dev);
>> +}
>> +
>>   /* Returns 1 if GDSC status is status, 0 if not, and < 0 on error */
>>   static int gdsc_check_status(struct gdsc *sc, enum gdsc_status status)
>>   {
>> @@ -232,9 +257,8 @@ static void gdsc_retain_ff_on(struct gdsc *sc)
>>   	regmap_update_bits(sc->regmap, sc->gdscr, mask, mask);
>>   }
>>   
>> -static int gdsc_enable(struct generic_pm_domain *domain)
>> +static int _gdsc_enable(struct gdsc *sc)
>>   {
>> -	struct gdsc *sc = domain_to_gdsc(domain);
>>   	int ret;
>>   
>>   	if (sc->pwrsts == PWRSTS_ON)
>> @@ -290,11 +314,28 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>>   	return 0;
>>   }
>>   
>> -static int gdsc_disable(struct generic_pm_domain *domain)
>> +static int gdsc_enable(struct generic_pm_domain *domain)
>>   {
>>   	struct gdsc *sc = domain_to_gdsc(domain);
>>   	int ret;
>>   
>> +	ret = gdsc_pm_runtime_get(sc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = _gdsc_enable(sc);
>> +	if (ret) {
>> +		gdsc_pm_runtime_put(sc);
> 
> I presume what you do here is to leave the pm_runtime state of dispcc
> active if we succeeded in enabling the gdsc. But the gdsc is a subdomain
> of the parent domain, so the framework should take case of its
> dependency.
> 
> So the reason for gdsc_pm_runtime_get()/put() in this code path is so
> that you can access the dispcc registers, i.e. I think you should
> get()/put() regardless of the return value.

pm domain code will handle enabling MMCX, so this code is not required 
strictly speaking. Ulf suggested adding it back, so I followed the 
suggestion. Maybe I misunderstood his suggestion.

putting pm_runtime after gdsc_enable does not sound like a logical case. 
However it would simplify code a bit. Let me try...

> 
>> +		return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int _gdsc_disable(struct gdsc *sc)
>> +{
>> +	int ret;
>> +
>>   	if (sc->pwrsts == PWRSTS_ON)
>>   		return gdsc_assert_reset(sc);
>>   
>> @@ -329,6 +370,18 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>>   	return 0;
>>   }
>>   
>> +static int gdsc_disable(struct generic_pm_domain *domain)
>> +{
>> +	struct gdsc *sc = domain_to_gdsc(domain);
>> +	int ret;
>> +
> 
> If the gdsc is found to be on at initialization, the next operation that
> will happen is gdsc_disable() and as you didn't activate the pm_runtime
> state in gdsc_init() you would in theory get here with registers
> unaccessible.
> 
> In practice though, the active gdsc should through the being a subdomain
> of the parent domain keep power on for you, so you won't notice this
> issue.

Nice catch.

> 
> But as above, I think you should wrap _gdsc_disable() in a get()/put()
> pair.
> 
>> +	ret = _gdsc_disable(sc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return gdsc_pm_runtime_put(sc);
>> +}
>> +
>>   static int gdsc_init(struct gdsc *sc)
>>   {
>>   	u32 mask, val;
>> @@ -425,6 +478,8 @@ int gdsc_register(struct gdsc_desc *desc,
>>   	for (i = 0; i < num; i++) {
>>   		if (!scs[i])
>>   			continue;
>> +		if (pm_runtime_enabled(dev))
>> +			scs[i]->rpm_dev = dev;
>>   		scs[i]->regmap = regmap;
>>   		scs[i]->rcdev = rcdev;
>>   		ret = gdsc_init(scs[i]);
>> @@ -486,7 +541,10 @@ void gdsc_unregister(struct gdsc_desc *desc)
>>    */
>>   int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain)
>>   {
>> +	struct gdsc *sc = domain_to_gdsc(domain);
>> +
>>   	/* Do nothing but give genpd the impression that we were successful */
>> -	return 0;
>> +	/* Get the runtime PM device only */
>> +	return gdsc_pm_runtime_get(sc);
> 
> Per above, if you let the framework deal with the gdsc's dependencies on
> the parent domain and you only get()/put() for the sake of dispcc then
> you don't need you don't need to do this to keep the subsequent
> gdsc_disable() in balance.
> 
>>   }
>>   EXPORT_SYMBOL_GPL(gdsc_gx_do_nothing_enable);
>> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
>> index 5bb396b344d1..a82982df0a55 100644
>> --- a/drivers/clk/qcom/gdsc.h
>> +++ b/drivers/clk/qcom/gdsc.h
>> @@ -25,6 +25,7 @@ struct reset_controller_dev;
>>    * @resets: ids of resets associated with this gdsc
>>    * @reset_count: number of @resets
>>    * @rcdev: reset controller
>> + * @rpm_dev: runtime PM device
>>    */
>>   struct gdsc {
>>   	struct generic_pm_domain	pd;
>> @@ -58,6 +59,7 @@ struct gdsc {
>>   
>>   	const char 			*supply;
>>   	struct regulator		*rsupply;
>> +	struct device 			*rpm_dev;
> 
> This isn't just the "runtime pm device", it's the device this gdsc is
> associated with. So "dev" sounds sufficient to me, but that requires
> that you have a separate bool rpm_enabled to remember if
> pm_runtime_enabled() was true during probe.
> 
> So unless we need "dev" for something else this might be sufficient.
> 
> Regards,
> Bjorn
> 
>>   };
>>   
>>   struct gdsc_desc {
>> -- 
>> 2.30.2
>>


-- 
With best wishes
Dmitry

Re: [PATCH v3 3/7] clk: qcom: gdsc: enable optional power domain support

[Bjorn Andersson]

On Fri 09 Jul 16:28 CDT 2021, Dmitry Baryshkov wrote:

> On 09/07/2021 21:38, Bjorn Andersson wrote:
> > On Fri 09 Jul 12:31 CDT 2021, Dmitry Baryshkov wrote:
> > 
> > > On sm8250 dispcc and videocc registers are powered up by the MMCX power
> > > domain. Currently we use 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 runtime PM and
> > > using specified for gdsc powerup.
> > > 
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >   drivers/clk/qcom/common.c | 37 +++++++++++++++++++++++++++++++------
> > >   drivers/clk/qcom/gdsc.c   |  4 ++++
> > >   2 files changed, 35 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);
> > 
> > In turingcc-qcs404.c I'm using pm_runtime to have the clock framework
> > ensure that the iface clock is enabled during clock operations, so this
> > will result in a "unbalanced enable" warning.
> 
> And later I register the disabler:
> 
>  ret = devm_add_action_or_reset(dev, qcom_cc_pm_runtime_disable, dev);
> 
> You might want to add this to qcs404 code.
> 

Let's land it using the apis that exist, then I think there's plenty of
examples throughout the kernel to make a case for introducing
devm_pm_clk_create() and devm_pm_runtime_enable().

But introducing that would complicate the path your patches would have
to take towards mainline.

> > 
> > > +	ret = pm_runtime_get_sync(dev);
> > 
> > I don't think you should wrap the entire initialization in a
> > pm_runtime_get_sync()/put() region. Instead follow the clock framework
> > and wrap gdsc initialization that needs to touch the hardware in:
> 
> Init should be wrapped in the pm_runtime_get/put calls, so that the MMCX
> domain is on through the fall init sequence. Otherwise it can get turned off
> during it, boom, failed register access and reboot.
> 

Right, we need to wrap init in pm_runtime_get()/put(), to ensure that
the registers are accessible. But the clock code has these surrounding
the initialization and therefor I think you should do the same in
gdsc_init() - instead of here.

> > 
> > 	if (pm_runtime_enabled())
> > 		pm_runtime_get_sync();
> 
> I don't think it's worth doing that. Having single lock for the whole init
> sequence is safer (and cleaner).
> 

Right, you need to cache the pm_runtime_enabled(), just as is done with
core->rpm_enabled in the clock framework.

The majority of the code involved in dispcc's initialization already has
more granular pm_runtime_get()/put() sections, so I don't think you
should override that with a big section here.

> If you check other pm-enabled drivers, they would either call
> pm_runtime_enable at the end of the probe or get_sync in the beginning of
> the probe and put_FOO in the end. In this driver calling pm_runtime_enable()
> at the end of the probe function will not work, since this way clk subsystem
> will not pick up the device for runtime power management (as
> pm_runtime_enabled() would return false).
> 

Right, just as turingcc does, we need to call pm_runtime_enable() early
and gdsc needs to check during initialization if pm_runtime should be
used for the particular clock controller.

> > 
> > I do however think that as of this patch, when probe returns MMCX might
> > very well be turned off, as the only user (this driver) has pm_runtime
> > enabled and it's idle. So I think you should introduce the
> > pm_runtime_get()/put() in the gdsc functions before this patch.
> 
> Maybe I'd just squash them together.
> 

With what I suggest the two patches are reduced to adding pm_runtime
support in the gdsc driver, and then a separate patch adding
pm_runtime_enable() in dispcc and videocc.

> > 
> > 
> > To summarize, I think you should rely on the individual clock drivers to
> > pm_runtime_enable()/disable().
> > 
> > > +	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..ccd36617d067 100644
> > > --- a/drivers/clk/qcom/gdsc.c
> > > +++ b/drivers/clk/qcom/gdsc.c
> > > @@ -439,6 +439,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);
> > 
> > Nice, I didn't know that we could fish it out of the dev.
> > 
> > Regards,
> > Bjorn
> > 
> > >   	}
> > >   	return of_genpd_add_provider_onecell(dev->of_node, data);
> > > @@ -457,6 +459,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
> > > 
> 
> 
> -- 
> With best wishes
> Dmitry

Re: [PATCH v3 4/7] clk: qcom: gdsc: call runtime PM functions for the provider device

[Bjorn Andersson]

On Fri 09 Jul 17:10 CDT 2021, Dmitry Baryshkov wrote:

> On 09/07/2021 21:54, Bjorn Andersson wrote:
> > On Fri 09 Jul 12:31 CDT 2021, Dmitry Baryshkov wrote:
> > 
> > > In order to properly handle runtime PM status of the provider device,
> > > call pm_runtime_get/pm_runtime_put on the clock controller device.
> > > 
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >   drivers/clk/qcom/gdsc.c | 66 ++++++++++++++++++++++++++++++++++++++---
> > >   drivers/clk/qcom/gdsc.h |  2 ++
> > >   2 files changed, 64 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > > index ccd36617d067..6bec31fccb09 100644
> > > --- a/drivers/clk/qcom/gdsc.c
> > > +++ b/drivers/clk/qcom/gdsc.c
> > > @@ -11,6 +11,7 @@
> > >   #include <linux/kernel.h>
> > >   #include <linux/ktime.h>
> > >   #include <linux/pm_domain.h>
> > > +#include <linux/pm_runtime.h>
> > >   #include <linux/regmap.h>
> > >   #include <linux/regulator/consumer.h>
> > >   #include <linux/reset-controller.h>
> > > @@ -50,6 +51,30 @@ enum gdsc_status {
> > >   	GDSC_ON
> > >   };
> > > +static int gdsc_pm_runtime_get(struct gdsc *sc)
> > > +{
> > > +	int ret;
> > > +
> > > +	if (!sc->rpm_dev)
> > > +		return 0;
> > > +
> > > +	ret = pm_runtime_get_sync(sc->rpm_dev);
> > > +	if (ret < 0) {
> > > +		pm_runtime_put_noidle(sc->rpm_dev);
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int gdsc_pm_runtime_put(struct gdsc *sc)
> > > +{
> > > +	if (!sc->rpm_dev)
> > > +		return 0;
> > > +
> > > +	return pm_runtime_put_sync(sc->rpm_dev);
> > > +}
> > > +
> > >   /* Returns 1 if GDSC status is status, 0 if not, and < 0 on error */
> > >   static int gdsc_check_status(struct gdsc *sc, enum gdsc_status status)
> > >   {
> > > @@ -232,9 +257,8 @@ static void gdsc_retain_ff_on(struct gdsc *sc)
> > >   	regmap_update_bits(sc->regmap, sc->gdscr, mask, mask);
> > >   }
> > > -static int gdsc_enable(struct generic_pm_domain *domain)
> > > +static int _gdsc_enable(struct gdsc *sc)
> > >   {
> > > -	struct gdsc *sc = domain_to_gdsc(domain);
> > >   	int ret;
> > >   	if (sc->pwrsts == PWRSTS_ON)
> > > @@ -290,11 +314,28 @@ static int gdsc_enable(struct generic_pm_domain *domain)
> > >   	return 0;
> > >   }
> > > -static int gdsc_disable(struct generic_pm_domain *domain)
> > > +static int gdsc_enable(struct generic_pm_domain *domain)
> > >   {
> > >   	struct gdsc *sc = domain_to_gdsc(domain);
> > >   	int ret;
> > > +	ret = gdsc_pm_runtime_get(sc);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = _gdsc_enable(sc);
> > > +	if (ret) {
> > > +		gdsc_pm_runtime_put(sc);
> > 
> > I presume what you do here is to leave the pm_runtime state of dispcc
> > active if we succeeded in enabling the gdsc. But the gdsc is a subdomain
> > of the parent domain, so the framework should take case of its
> > dependency.
> > 
> > So the reason for gdsc_pm_runtime_get()/put() in this code path is so
> > that you can access the dispcc registers, i.e. I think you should
> > get()/put() regardless of the return value.
> 
> pm domain code will handle enabling MMCX, so this code is not required
> strictly speaking. Ulf suggested adding it back, so I followed the
> suggestion. Maybe I misunderstood his suggestion.
> 
> putting pm_runtime after gdsc_enable does not sound like a logical case.
> However it would simplify code a bit. Let me try...
> 

If you consider this device's and the individual gdsc's power state as
separate consumers of MMCX, then it perhaps makes more sense?

Similar to how a regulator driver would rely on the regulator framework
to deal with parent regulators...

Regards,
Bjorn

> > 
> > > +		return ret;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int _gdsc_disable(struct gdsc *sc)
> > > +{
> > > +	int ret;
> > > +
> > >   	if (sc->pwrsts == PWRSTS_ON)
> > >   		return gdsc_assert_reset(sc);
> > > @@ -329,6 +370,18 @@ static int gdsc_disable(struct generic_pm_domain *domain)
> > >   	return 0;
> > >   }
> > > +static int gdsc_disable(struct generic_pm_domain *domain)
> > > +{
> > > +	struct gdsc *sc = domain_to_gdsc(domain);
> > > +	int ret;
> > > +
> > 
> > If the gdsc is found to be on at initialization, the next operation that
> > will happen is gdsc_disable() and as you didn't activate the pm_runtime
> > state in gdsc_init() you would in theory get here with registers
> > unaccessible.
> > 
> > In practice though, the active gdsc should through the being a subdomain
> > of the parent domain keep power on for you, so you won't notice this
> > issue.
> 
> Nice catch.
> 
> > 
> > But as above, I think you should wrap _gdsc_disable() in a get()/put()
> > pair.
> > 
> > > +	ret = _gdsc_disable(sc);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return gdsc_pm_runtime_put(sc);
> > > +}
> > > +
> > >   static int gdsc_init(struct gdsc *sc)
> > >   {
> > >   	u32 mask, val;
> > > @@ -425,6 +478,8 @@ int gdsc_register(struct gdsc_desc *desc,
> > >   	for (i = 0; i < num; i++) {
> > >   		if (!scs[i])
> > >   			continue;
> > > +		if (pm_runtime_enabled(dev))
> > > +			scs[i]->rpm_dev = dev;
> > >   		scs[i]->regmap = regmap;
> > >   		scs[i]->rcdev = rcdev;
> > >   		ret = gdsc_init(scs[i]);
> > > @@ -486,7 +541,10 @@ void gdsc_unregister(struct gdsc_desc *desc)
> > >    */
> > >   int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain)
> > >   {
> > > +	struct gdsc *sc = domain_to_gdsc(domain);
> > > +
> > >   	/* Do nothing but give genpd the impression that we were successful */
> > > -	return 0;
> > > +	/* Get the runtime PM device only */
> > > +	return gdsc_pm_runtime_get(sc);
> > 
> > Per above, if you let the framework deal with the gdsc's dependencies on
> > the parent domain and you only get()/put() for the sake of dispcc then
> > you don't need you don't need to do this to keep the subsequent
> > gdsc_disable() in balance.
> > 
> > >   }
> > >   EXPORT_SYMBOL_GPL(gdsc_gx_do_nothing_enable);
> > > diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> > > index 5bb396b344d1..a82982df0a55 100644
> > > --- a/drivers/clk/qcom/gdsc.h
> > > +++ b/drivers/clk/qcom/gdsc.h
> > > @@ -25,6 +25,7 @@ struct reset_controller_dev;
> > >    * @resets: ids of resets associated with this gdsc
> > >    * @reset_count: number of @resets
> > >    * @rcdev: reset controller
> > > + * @rpm_dev: runtime PM device
> > >    */
> > >   struct gdsc {
> > >   	struct generic_pm_domain	pd;
> > > @@ -58,6 +59,7 @@ struct gdsc {
> > >   	const char 			*supply;
> > >   	struct regulator		*rsupply;
> > > +	struct device 			*rpm_dev;
> > 
> > This isn't just the "runtime pm device", it's the device this gdsc is
> > associated with. So "dev" sounds sufficient to me, but that requires
> > that you have a separate bool rpm_enabled to remember if
> > pm_runtime_enabled() was true during probe.
> > 
> > So unless we need "dev" for something else this might be sufficient.
> > 
> > Regards,
> > Bjorn
> > 
> > >   };
> > >   struct gdsc_desc {
> > > -- 
> > > 2.30.2
> > > 
> 
> 
> -- 
> With best wishes
> Dmitry