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

[PATCH v6 0/6] 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-arm-msm/20210703005416.2668319-1-bjorn.andersson@linaro.org/
  (pending)

Changes since v5:
 - Dropped devm_pm_runtime_enable callback to remove extra dependency

Changes since v4:
 - Dropped pm_runtime handling from drivers/clk/qcom/common.c Moved the
   code into dispcc-sm8250.c and videocc-sm8250.c

Changes since v3:
 - Wrap gdsc_enable/gdsc_disable into pm_runtime_get/put calls rather
   than calling pm_runtime_get in gdsc_enabled and _put in gdsc_disable
 - Squash gdsc patches together to remove possible dependencies between
   two patches.

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 (8):
      dt-bindings: clock: qcom,dispcc-sm8x50: add mmcx power domain
      dt-bindings: clock: qcom,videocc: add mmcx power domain
      clk: qcom: dispcc-sm8250: use runtime PM for the clock controller
      clk: qcom: videocc-sm8250: use runtime PM for the clock controller
      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/clk/qcom/dispcc-sm8250.c                   | 28 ++++++++--
 drivers/clk/qcom/gdsc.c                            | 59 ++++++++++++++++++++--
 drivers/clk/qcom/gdsc.h                            |  2 +
 drivers/clk/qcom/videocc-sm8250.c                  | 31 +++++++++---
 7 files changed, 124 insertions(+), 21 deletions(-)

[PATCH v6 1/8] 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>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.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 6667261dc665..6b3380cf1a10 100644
--- a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
@@ -56,6 +56,11 @@ properties:
   reg:
     maxItems: 1
 
+  power-domains:
+    description:
+      A phandle and PM domain specifier for the MMCX power domain.
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -70,6 +75,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>;
@@ -90,5 +96,6 @@ examples:
       #clock-cells = <1>;
       #reset-cells = <1>;
       #power-domain-cells = <1>;
+      power-domains = <&rpmhpd SM8250_MMCX>;
     };
 ...
-- 
2.30.2

[PATCH v6 2/8] 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>
Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Reviewed-by: Rob Herring <robh@kernel.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 v6 3/8] clk: qcom: dispcc-sm8250: use runtime PM for the clock controller

[Dmitry Baryshkov]

On sm8250 dispcc and videocc registers are powered up by the MMCX power
domain. Use runtime PM calls to make sure that required power domain is
powered on while we access clock controller's registers.

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

diff --git a/drivers/clk/qcom/dispcc-sm8250.c b/drivers/clk/qcom/dispcc-sm8250.c
index 601c7c0ba483..108dd1249b6a 100644
--- a/drivers/clk/qcom/dispcc-sm8250.c
+++ b/drivers/clk/qcom/dispcc-sm8250.c
@@ -6,6 +6,7 @@
 #include <linux/clk-provider.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 #include <linux/reset-controller.h>
 
@@ -1226,13 +1227,31 @@ static const struct of_device_id disp_cc_sm8250_match_table[] = {
 };
 MODULE_DEVICE_TABLE(of, disp_cc_sm8250_match_table);
 
+static void disp_cc_sm8250_pm_runtime_disable(void *data)
+{
+	pm_runtime_disable(data);
+}
+
 static int disp_cc_sm8250_probe(struct platform_device *pdev)
 {
 	struct regmap *regmap;
+	int ret;
+
+	pm_runtime_enable(&pdev->dev);
+
+	ret = devm_add_action_or_reset(&pdev->dev, disp_cc_sm8250_pm_runtime_disable, &pdev->dev);
+	if (ret)
+		return ret;
+
+	ret = pm_runtime_resume_and_get(&pdev->dev);
+	if (ret)
+		return ret;
 
 	regmap = qcom_cc_map(pdev, &disp_cc_sm8250_desc);
-	if (IS_ERR(regmap))
+	if (IS_ERR(regmap)) {
+		pm_runtime_put(&pdev->dev);
 		return PTR_ERR(regmap);
+	}
 
 	/* note: trion == lucid, except for the prepare() op */
 	BUILD_BUG_ON(CLK_ALPHA_PLL_TYPE_TRION != CLK_ALPHA_PLL_TYPE_LUCID);
@@ -1257,7 +1276,11 @@ static int disp_cc_sm8250_probe(struct platform_device *pdev)
 	/* DISP_CC_XO_CLK always-on */
 	regmap_update_bits(regmap, 0x605c, BIT(0), BIT(0));
 
-	return qcom_cc_really_probe(pdev, &disp_cc_sm8250_desc, regmap);
+	ret = qcom_cc_really_probe(pdev, &disp_cc_sm8250_desc, regmap);
+
+	pm_runtime_put(&pdev->dev);
+
+	return ret;
 }
 
 static struct platform_driver disp_cc_sm8250_driver = {
-- 
2.30.2

[PATCH v6 4/8] clk: qcom: videocc-sm8250: use runtime PM for the clock controller

[Dmitry Baryshkov]

On sm8250 dispcc and videocc registers are powered up by the MMCX power
domain. Use runtime PM calls to make sure that required power domain is
powered on while we access clock controller's registers.

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

diff --git a/drivers/clk/qcom/videocc-sm8250.c b/drivers/clk/qcom/videocc-sm8250.c
index 7b435a1c2c4b..8617454e4a77 100644
--- a/drivers/clk/qcom/videocc-sm8250.c
+++ b/drivers/clk/qcom/videocc-sm8250.c
@@ -6,6 +6,7 @@
 #include <linux/clk-provider.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include <linux/regmap.h>
 
 #include <dt-bindings/clock/qcom,videocc-sm8250.h>
@@ -364,13 +365,31 @@ static const struct of_device_id video_cc_sm8250_match_table[] = {
 };
 MODULE_DEVICE_TABLE(of, video_cc_sm8250_match_table);
 
+static void video_cc_sm8250_pm_runtime_disable(void *data)
+{
+	pm_runtime_disable(data);
+}
+
 static int video_cc_sm8250_probe(struct platform_device *pdev)
 {
 	struct regmap *regmap;
+	int ret;
+
+	pm_runtime_enable(&pdev->dev);
+
+	ret = devm_add_action_or_reset(&pdev->dev, video_cc_sm8250_pm_runtime_disable, &pdev->dev);
+	if (ret)
+		return ret;
+
+	ret = pm_runtime_resume_and_get(&pdev->dev);
+	if (ret)
+		return ret;
 
 	regmap = qcom_cc_map(pdev, &video_cc_sm8250_desc);
-	if (IS_ERR(regmap))
+	if (IS_ERR(regmap)) {
+		pm_runtime_put(&pdev->dev);
 		return PTR_ERR(regmap);
+	}
 
 	clk_lucid_pll_configure(&video_pll0, regmap, &video_pll0_config);
 	clk_lucid_pll_configure(&video_pll1, regmap, &video_pll1_config);
@@ -379,7 +398,11 @@ static int video_cc_sm8250_probe(struct platform_device *pdev)
 	regmap_update_bits(regmap, 0xe58, BIT(0), BIT(0));
 	regmap_update_bits(regmap, 0xeec, BIT(0), BIT(0));
 
-	return qcom_cc_really_probe(pdev, &video_cc_sm8250_desc, regmap);
+	ret = qcom_cc_really_probe(pdev, &video_cc_sm8250_desc, regmap);
+
+	pm_runtime_put(&pdev->dev);
+
+	return ret;
 }
 
 static struct platform_driver video_cc_sm8250_driver = {
-- 
2.30.2

[PATCH v6 5/8] 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.

Make gdsc code also use pm_runtime calls to ensure that registers are
accessible during the gdsc_enable/gdsc_disable operations.

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

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 51ed640e527b..a0781d7284cc 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,22 @@ enum gdsc_status {
 	GDSC_ON
 };
 
+static int gdsc_pm_runtime_get(struct gdsc *sc)
+{
+	if (!sc->dev)
+		return 0;
+
+	return pm_runtime_resume_and_get(sc->dev);
+}
+
+static int gdsc_pm_runtime_put(struct gdsc *sc)
+{
+	if (!sc->dev)
+		return 0;
+
+	return pm_runtime_put_sync(sc->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 +249,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 +306,26 @@ 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);
+
+	gdsc_pm_runtime_put(sc);
+
+	return ret;
+}
+
+static int _gdsc_disable(struct gdsc *sc)
+{
+	int ret;
+
 	if (sc->pwrsts == PWRSTS_ON)
 		return gdsc_assert_reset(sc);
 
@@ -329,6 +360,22 @@ 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_pm_runtime_get(sc);
+	if (ret)
+		return ret;
+
+	ret = _gdsc_disable(sc);
+
+	gdsc_pm_runtime_put(sc);
+
+	return ret;
+}
+
 static int gdsc_init(struct gdsc *sc)
 {
 	u32 mask, val;
@@ -425,6 +472,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]->dev = dev;
 		scs[i]->regmap = regmap;
 		scs[i]->rcdev = rcdev;
 		ret = gdsc_init(scs[i]);
@@ -439,6 +488,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 +508,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);
 }
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index 5bb396b344d1..702d47a87af6 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
+ * @dev: the device holding the GDSC, used for pm_runtime calls
  */
 struct gdsc {
 	struct generic_pm_domain	pd;
@@ -58,6 +59,7 @@ struct gdsc {
 
 	const char 			*supply;
 	struct regulator		*rsupply;
+	struct device 			*dev;
 };
 
 struct gdsc_desc {
-- 
2.30.2

[PATCH v6 6/8] 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>
Reviewed-by: Bjorn Andersson <bjorn.andersson@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 4798368b02ef..bce5e9a9dd84 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -273,13 +273,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>;
@@ -2451,7 +2444,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>;
@@ -2720,7 +2713,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 v6 7/8] 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>
Reviewed-by: Bjorn Andersson <bjorn.andersson@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 108dd1249b6a..cf0bb12eb6e1 100644
--- a/drivers/clk/qcom/dispcc-sm8250.c
+++ b/drivers/clk/qcom/dispcc-sm8250.c
@@ -1129,7 +1129,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 v6 8/8] 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>
Reviewed-by: Bjorn Andersson <bjorn.andersson@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 8617454e4a77..f28f2cb051d7 100644
--- a/drivers/clk/qcom/videocc-sm8250.c
+++ b/drivers/clk/qcom/videocc-sm8250.c
@@ -277,7 +277,6 @@ static struct gdsc mvs0c_gdsc = {
 	},
 	.flags = 0,
 	.pwrsts = PWRSTS_OFF_ON,
-	.supply = "mmcx",
 };
 
 static struct gdsc mvs1c_gdsc = {
@@ -287,7 +286,6 @@ static struct gdsc mvs1c_gdsc = {
 	},
 	.flags = 0,
 	.pwrsts = PWRSTS_OFF_ON,
-	.supply = "mmcx",
 };
 
 static struct gdsc mvs0_gdsc = {
@@ -297,7 +295,6 @@ static struct gdsc mvs0_gdsc = {
 	},
 	.flags = HW_CTRL,
 	.pwrsts = PWRSTS_OFF_ON,
-	.supply = "mmcx",
 };
 
 static struct gdsc mvs1_gdsc = {
@@ -307,7 +304,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 v6 0/6] clk: qcom: use power-domain for sm8250's clock controllers

[Ulf Hansson]

On Tue, 27 Jul 2021 at 22:20, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> 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-arm-msm/20210703005416.2668319-1-bjorn.andersson@linaro.org/
>   (pending)
>
> Changes since v5:
>  - Dropped devm_pm_runtime_enable callback to remove extra dependency
>
> Changes since v4:
>  - Dropped pm_runtime handling from drivers/clk/qcom/common.c Moved the
>    code into dispcc-sm8250.c and videocc-sm8250.c
>
> Changes since v3:
>  - Wrap gdsc_enable/gdsc_disable into pm_runtime_get/put calls rather
>    than calling pm_runtime_get in gdsc_enabled and _put in gdsc_disable
>  - Squash gdsc patches together to remove possible dependencies between
>    two patches.
>
> 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 (8):
>       dt-bindings: clock: qcom,dispcc-sm8x50: add mmcx power domain
>       dt-bindings: clock: qcom,videocc: add mmcx power domain
>       clk: qcom: dispcc-sm8250: use runtime PM for the clock controller
>       clk: qcom: videocc-sm8250: use runtime PM for the clock controller
>       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/clk/qcom/dispcc-sm8250.c                   | 28 ++++++++--
>  drivers/clk/qcom/gdsc.c                            | 59 ++++++++++++++++++++--
>  drivers/clk/qcom/gdsc.h                            |  2 +
>  drivers/clk/qcom/videocc-sm8250.c                  | 31 +++++++++---
>  7 files changed, 124 insertions(+), 21 deletions(-)
>

For the series:

Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>

Kind regards
Uffe

Re: [PATCH v6 0/6] clk: qcom: use power-domain for sm8250's clock controllers

[Stephen Boyd]

Quoting Dmitry Baryshkov (2021-07-27 13:19:56)
> 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-arm-msm/20210703005416.2668319-1-bjorn.andersson@linaro.org/
>   (pending)

Does this patch series need to go through the qcom tree? Presumably the
dependency is going through qcom -> arm-soc

> 
> Changes since v5:
>  - Dropped devm_pm_runtime_enable callback to remove extra dependency
> 
> Changes since v4:
>  - Dropped pm_runtime handling from drivers/clk/qcom/common.c Moved the
>    code into dispcc-sm8250.c and videocc-sm8250.c
> 
> Changes since v3:
>  - Wrap gdsc_enable/gdsc_disable into pm_runtime_get/put calls rather
>    than calling pm_runtime_get in gdsc_enabled and _put in gdsc_disable
>  - Squash gdsc patches together to remove possible dependencies between
>    two patches.
>

Re: [PATCH v6 0/6] clk: qcom: use power-domain for sm8250's clock controllers

[Dmitry Baryshkov]

On 26/08/2021 21:31, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2021-07-27 13:19:56)
>> 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-arm-msm/20210703005416.2668319-1-bjorn.andersson@linaro.org/
>>    (pending)
> 
> Does this patch series need to go through the qcom tree? Presumably the
> dependency is going through qcom -> arm-soc

It looks like Bjorn did not apply his patches in the for-5.15 series, so 
we'd have to wait anyway. Probably I should rebase these patches instead 
on Rajendra's required-opps patch (which is going in this window).

> 
>>
>> Changes since v5:
>>   - Dropped devm_pm_runtime_enable callback to remove extra dependency
>>
>> Changes since v4:
>>   - Dropped pm_runtime handling from drivers/clk/qcom/common.c Moved the
>>     code into dispcc-sm8250.c and videocc-sm8250.c
>>
>> Changes since v3:
>>   - Wrap gdsc_enable/gdsc_disable into pm_runtime_get/put calls rather
>>     than calling pm_runtime_get in gdsc_enabled and _put in gdsc_disable
>>   - Squash gdsc patches together to remove possible dependencies between
>>     two patches.
>>


-- 
With best wishes
Dmitry

Re: [PATCH v6 0/6] clk: qcom: use power-domain for sm8250's clock controllers

[Stephen Boyd]

Quoting Dmitry Baryshkov (2021-08-26 14:56:23)
> On 26/08/2021 21:31, Stephen Boyd wrote:
> > Quoting Dmitry Baryshkov (2021-07-27 13:19:56)
> >> 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-arm-msm/20210703005416.2668319-1-bjorn.andersson@linaro.org/
> >>    (pending)
> > 
> > Does this patch series need to go through the qcom tree? Presumably the
> > dependency is going through qcom -> arm-soc
> 
> It looks like Bjorn did not apply his patches in the for-5.15 series, so 
> we'd have to wait anyway. Probably I should rebase these patches instead 
> on Rajendra's required-opps patch (which is going in this window).
> 

Ok. Thanks. I'll drop it from my queue for now.

Re: [PATCH v6 0/6] clk: qcom: use power-domain for sm8250's clock controllers

[Dmitry Baryshkov]

On Sun, 29 Aug 2021 at 06:51, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Dmitry Baryshkov (2021-08-26 14:56:23)
> > On 26/08/2021 21:31, Stephen Boyd wrote:
> > > Quoting Dmitry Baryshkov (2021-07-27 13:19:56)
> > >> 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-arm-msm/20210703005416.2668319-1-bjorn.andersson@linaro.org/
> > >>    (pending)
> > >
> > > Does this patch series need to go through the qcom tree? Presumably the
> > > dependency is going through qcom -> arm-soc
> >
> > It looks like Bjorn did not apply his patches in the for-5.15 series, so
> > we'd have to wait anyway. Probably I should rebase these patches instead
> > on Rajendra's required-opps patch (which is going in this window).
> >
>
> Ok. Thanks. I'll drop it from my queue for now.

Just for the reference. I've sent v7 of this patchset. After thinking
more about power domains relationship, I think we have a hole in the
abstraction here. Currently subdomains cause power domains to be
powered up, but do not dictate the performance level the parent domain
should be working in. While this does not look like an issue for the
gdsc (and thus it can be easily solved by the Bjorn's patches, which
enforce rpmhpd to be powered on to 'at least lowest possible'
performance state, this might be not the case for the future links. I
think at some point the pd_add_subdomain() interface should be
extended with the ability to specify minimum required performance
state when the link becomes on. Until that time I have changed code to
enforce having clock controller in pm resume state when gdsc is
enabled, thus CC itself votes on parent's (rpmhpd) performance state.


-- 
With best wishes
Dmitry

Re: [PATCH v6 0/6] clk: qcom: use power-domain for sm8250's clock controllers

[Ulf Hansson]

On Sun, 29 Aug 2021 at 17:54, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Sun, 29 Aug 2021 at 06:51, Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Dmitry Baryshkov (2021-08-26 14:56:23)
> > > On 26/08/2021 21:31, Stephen Boyd wrote:
> > > > Quoting Dmitry Baryshkov (2021-07-27 13:19:56)
> > > >> 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-arm-msm/20210703005416.2668319-1-bjorn.andersson@linaro.org/
> > > >>    (pending)
> > > >
> > > > Does this patch series need to go through the qcom tree? Presumably the
> > > > dependency is going through qcom -> arm-soc
> > >
> > > It looks like Bjorn did not apply his patches in the for-5.15 series, so
> > > we'd have to wait anyway. Probably I should rebase these patches instead
> > > on Rajendra's required-opps patch (which is going in this window).
> > >
> >
> > Ok. Thanks. I'll drop it from my queue for now.
>
> Just for the reference. I've sent v7 of this patchset. After thinking
> more about power domains relationship, I think we have a hole in the
> abstraction here. Currently subdomains cause power domains to be
> powered up, but do not dictate the performance level the parent domain
> should be working in.

That's not entirely true. In genpd_add_subdomain() we verify that if
the child is powered on, the parent must already be powered on,
otherwise we treat this a bad setup and return an error code.

What seems to be missing though, is that if there is a performance
state applied for the child domain, that should be propagated to the
parent domain too. Right?

> While this does not look like an issue for the
> gdsc (and thus it can be easily solved by the Bjorn's patches, which
> enforce rpmhpd to be powered on to 'at least lowest possible'
> performance state, this might be not the case for the future links. I
> think at some point the pd_add_subdomain() interface should be
> extended with the ability to specify minimum required performance
> state when the link becomes on.

I guess that minimum performance state could be considered as a
"required-opp" in the DT node for the power-domain provider, no?

Another option would simply be to manage this solely in the
platform/soc specific genpd provider. Would that work?

> Until that time I have changed code to
> enforce having clock controller in pm resume state when gdsc is
> enabled, thus CC itself votes on parent's (rpmhpd) performance state.
>
>
> --
> With best wishes
> Dmitry

Kind regards
Uffe

Re: [PATCH v6 0/6] clk: qcom: use power-domain for sm8250's clock controllers

[Dmitry Baryshkov]

On Tue, 7 Sept 2021 at 17:34, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Sun, 29 Aug 2021 at 17:54, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Sun, 29 Aug 2021 at 06:51, Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Dmitry Baryshkov (2021-08-26 14:56:23)
> > > > On 26/08/2021 21:31, Stephen Boyd wrote:
> > > > > Quoting Dmitry Baryshkov (2021-07-27 13:19:56)
> > > > >> 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-arm-msm/20210703005416.2668319-1-bjorn.andersson@linaro.org/
> > > > >>    (pending)
> > > > >
> > > > > Does this patch series need to go through the qcom tree? Presumably the
> > > > > dependency is going through qcom -> arm-soc
> > > >
> > > > It looks like Bjorn did not apply his patches in the for-5.15 series, so
> > > > we'd have to wait anyway. Probably I should rebase these patches instead
> > > > on Rajendra's required-opps patch (which is going in this window).
> > > >
> > >
> > > Ok. Thanks. I'll drop it from my queue for now.
> >
> > Just for the reference. I've sent v7 of this patchset. After thinking
> > more about power domains relationship, I think we have a hole in the
> > abstraction here. Currently subdomains cause power domains to be
> > powered up, but do not dictate the performance level the parent domain
> > should be working in.
>
> That's not entirely true. In genpd_add_subdomain() we verify that if
> the child is powered on, the parent must already be powered on,
> otherwise we treat this a bad setup and return an error code.
>
> What seems to be missing though, is that if there is a performance
> state applied for the child domain, that should be propagated to the
> parent domain too. Right?
>
> > While this does not look like an issue for the
> > gdsc (and thus it can be easily solved by the Bjorn's patches, which
> > enforce rpmhpd to be powered on to 'at least lowest possible'
> > performance state, this might be not the case for the future links. I
> > think at some point the pd_add_subdomain() interface should be
> > extended with the ability to specify minimum required performance
> > state when the link becomes on.
>
> I guess that minimum performance state could be considered as a
> "required-opp" in the DT node for the power-domain provider, no?

Yes, up to some point. But this enforces a particular driver code
(that I've had to change from v6 to v7).

In v6 the gdsc's power_on code would pm_runtime_get() the provider
device, power on the domain and the pm_runtime_put() the provider
device. Thus the gdsc genpd would be powered on (keeping parent
domains in the on state), but the provider device itself would be
runtime-suspended (neat idea by Bjorn). However this relied on changes
in rpmhpd behaviour (which still did not make it to linux-next).

In v7 we have to keep the provider device in resumed state while the
gdsc genpd is powered on (to keep the required-opps vote in place).

I suppose that 'child requires minimum parent's performance state'
might become common property at some point, allowing us to drop this
pm_runtime handling.

> Another option would simply be to manage this solely in the
> platform/soc specific genpd provider. Would that work?

Yes, I've had this in the very old iteration of mmcx fixup patchset
(even before mmcx-regulator came into play). It ended up with quite an
ugly piece of code.

>
> > Until that time I have changed code to
> > enforce having clock controller in pm resume state when gdsc is
> > enabled, thus CC itself votes on parent's (rpmhpd) performance state.
> >
> >
> > --
> > With best wishes
> > Dmitry
>
> Kind regards
> Uffe



-- 
With best wishes
Dmitry