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

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/1628767642-4008-1-git-send-email-rnayak@codeaurora.org/
(pending inclusion into 5.15)

Changes since v6:
 - Dropped dependency on Bjorn's patches
 - Restored required-opps properties
 - Held pm device state while gdsc is powered on, removing dependency on
   genpd's power_on() powering the domain into required state.

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         | 13 ++++++
 .../devicetree/bindings/clock/qcom,videocc.yaml    | 13 ++++++
 arch/arm64/boot/dts/qcom/sm8250.dtsi               | 13 ++----
 drivers/clk/qcom/dispcc-sm8250.c                   | 28 ++++++++++--
 drivers/clk/qcom/gdsc.c                            | 51 ++++++++++++++++++++--
 drivers/clk/qcom/gdsc.h                            |  2 +
 drivers/clk/qcom/videocc-sm8250.c                  | 31 ++++++++++---
 7 files changed, 130 insertions(+), 21 deletions(-)

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

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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/clock/qcom,videocc.yaml     | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/qcom,videocc.yaml b/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
index 567202942b88..5d7053503435 100644
--- a/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,videocc.yaml

@@ -47,6 +47,16 @@ properties:
   reg:
     maxItems: 1
 
+  power-domains:
+    description:
+      A phandle and PM domain specifier for the MMCX power domain.
+    maxItems: 1
+
+  required-opps:
+    description:
+      A phandle to an OPP node describing required MMCX performance point.
+    maxItems: 1
+
 required:
   - compatible
   - reg

@@ -61,6 +71,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 +80,7 @@ examples:
       #clock-cells = <1>;
       #reset-cells = <1>;
       #power-domain-cells = <1>;
+      power-domains = <&rpmhpd SM8250_MMCX>;
+      required-opps = <&rpmhpd_opp_low_svs>;
     };
 ...

-- 
2.33.0

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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/clock/qcom,dispcc-sm8x50.yaml          | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
index 6667261dc665..31497677e8de 100644
--- a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml

@@ -56,6 +56,16 @@ properties:
   reg:
     maxItems: 1
 
+  power-domains:
+    description:
+      A phandle and PM domain specifier for the MMCX power domain.
+    maxItems: 1
+
+  required-opps:
+    description:
+      A phandle to an OPP node describing required MMCX performance point.
+    maxItems: 1
+
 required:
   - compatible
   - reg

@@ -70,6 +80,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 +101,7 @@ examples:
       #clock-cells = <1>;
       #reset-cells = <1>;
       #power-domain-cells = <1>;
+      power-domains = <&rpmhpd SM8250_MMCX>;
+      required-opps = <&rpmhpd_opp_low_svs>;
     };
 ...

-- 
2.33.0

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 <redacted>
---
 drivers/clk/qcom/gdsc.c | 51 ++++++++++++++++++++++++++++++++++++++---
 drivers/clk/qcom/gdsc.h |  2 ++
 2 files changed, 50 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 4ece326ea233..7e1dd8ccfa38 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,22 @@ 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;
+
+	return _gdsc_enable(sc);
+}
+
+static int _gdsc_disable(struct gdsc *sc)
+{
+	int ret;
+
 	if (sc->pwrsts == PWRSTS_ON)
 		return gdsc_assert_reset(sc);
 

@@ -329,6 +356,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);
+
+	gdsc_pm_runtime_put(sc);
+
+	return ret;
+}
+
 static int gdsc_init(struct gdsc *sc)
 {
 	u32 mask, val;

@@ -443,6 +482,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]);

@@ -457,6 +498,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);

@@ -475,6 +518,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.33.0

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

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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>
---
 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.33.0

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

Signed-off-by: Dmitry Baryshkov <redacted>
Reviewed-by: Bjorn Andersson <redacted>
---
 arch/arm64/boot/dts/qcom/sm8250.dtsi | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index 4798368b02ef..0d35449fda02 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,8 @@ 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>;
+			required-opps = <&rpmhpd_opp_low_svs>;
 			clock-names = "iface", "bi_tcxo", "bi_tcxo_ao";
 			#clock-cells = <1>;
 			#reset-cells = <1>;

@@ -2720,7 +2714,8 @@ opp-358000000 {
 		dispcc: clock-controller@af00000 {
 			compatible = "qcom,sm8250-dispcc";
 			reg = <0 0x0af00000 0 0x10000>;
-			mmcx-supply = <&mmcx_reg>;
+			power-domains = <&rpmhpd SM8250_MMCX>;
+			required-opps = <&rpmhpd_opp_low_svs>;
 			clocks = <&rpmhcc RPMH_CXO_CLK>,
 				 <&dsi0_phy 0>,
 				 <&dsi0_phy 1>,

-- 
2.33.0

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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>
---
 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.33.0

On Sun, 29 Aug 2021 at 17:47, Dmitry Baryshkov
[off-list ref] 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-pm/1628767642-4008-1-git-send-email-rnayak@codeaurora.org/
(pending inclusion into 5.15)

I think I already reviewed v6, but perhaps you made some bigger
changes. Anyway, feel free to add, for the series:

Reviewed-by: Ulf Hansson <redacted>

Kind regards
Uffe

Changes since v6:
 - Dropped dependency on Bjorn's patches
 - Restored required-opps properties
 - Held pm device state while gdsc is powered on, removing dependency on
   genpd's power_on() powering the domain into required state.

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         | 13 ++++++
 .../devicetree/bindings/clock/qcom,videocc.yaml    | 13 ++++++
 arch/arm64/boot/dts/qcom/sm8250.dtsi               | 13 ++----
 drivers/clk/qcom/dispcc-sm8250.c                   | 28 ++++++++++--
 drivers/clk/qcom/gdsc.c                            | 51 ++++++++++++++++++++--
 drivers/clk/qcom/gdsc.h                            |  2 +
 drivers/clk/qcom/videocc-sm8250.c                  | 31 ++++++++++---
 7 files changed, 130 insertions(+), 21 deletions(-)

Hi,

On 29/08/2021 18:47, Dmitry Baryshkov 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-pm/1628767642-4008-1-git-send-email-rnayak@codeaurora.org/
(pending inclusion into 5.15)

Gracious ping for this patch series.

Changes since v6:
  - Dropped dependency on Bjorn's patches
  - Restored required-opps properties
  - Held pm device state while gdsc is powered on, removing dependency on
    genpd's power_on() powering the domain into required state.

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         | 13 ++++++
  .../devicetree/bindings/clock/qcom,videocc.yaml    | 13 ++++++
  arch/arm64/boot/dts/qcom/sm8250.dtsi               | 13 ++----
  drivers/clk/qcom/dispcc-sm8250.c                   | 28 ++++++++++--
  drivers/clk/qcom/gdsc.c                            | 51 ++++++++++++++++++++--
  drivers/clk/qcom/gdsc.h                            |  2 +
  drivers/clk/qcom/videocc-sm8250.c                  | 31 ++++++++++---
  7 files changed, 130 insertions(+), 21 deletions(-)

-- 
With best wishes
Dmitry

On Sun 29 Aug 08:47 PDT 2021, Dmitry Baryshkov wrote:

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.

As I said on previous iterations, the clock framework will ensure that
the power domain is powered up around all calls back into the clock
driver, so for clocks this isn't needed.

But after digging some more, the gdsc registration needs this and I
don't mind keeping it on for the duration of the registration, rather
than having the clock framework turn it on and off.

As far as I can see the reset_controller won't actually access the
hardware during registration.

Reviewed-by: Bjorn Andersson <redacted>



That said, qcom_reset_{assert,deassert}() doesn't seem to be performed
under a pm_runtime_get() section, so I believe that should be addressed
as well.  I don't see that we have a problem with this in practice as of
today, but it seems worthy to correct - unless I'm just missing
something.

Regards,
Bjorn

On Sun 29 Aug 08:47 PDT 2021, Dmitry Baryshkov wrote:

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.

Reviewed-by: Bjorn Andersson <redacted>

Regards,
Bjorn

On Sun 29 Aug 08:47 PDT 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.

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

Reviewed-by: Bjorn Andersson <redacted>

Regards,
Bjorn

On Sun 29 Aug 08:47 PDT 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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>

Once we merge these, I expect that the boards will start crashing if
the kernel is booted using an existing DTB?

Is it okay to just merge the first 6 patches in the series now and
postpone these two until we've had the dts change sitting for a while?

Regards,
Bjorn

On 07/10/2021 18:48, Bjorn Andersson wrote:

On Sun 29 Aug 08:47 PDT 2021, Dmitry Baryshkov wrote:

quoted

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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>

Once we merge these, I expect that the boards will start crashing if
the kernel is booted using an existing DTB?

Is it okay to just merge the first 6 patches in the series now and
postpone these two until we've had the dts change sitting for a while?

Sure it is.

Regards,
Bjorn

quoted

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

-- 
With best wishes
Dmitry

Quoting Dmitry Baryshkov (2021-10-07 09:16:13)

On 07/10/2021 18:48, Bjorn Andersson wrote:

quoted

On Sun 29 Aug 08:47 PDT 2021, Dmitry Baryshkov wrote:

quoted

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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>

Once we merge these, I expect that the boards will start crashing if
the kernel is booted using an existing DTB?

Is it okay to just merge the first 6 patches in the series now and
postpone these two until we've had the dts change sitting for a while?

Sure it is.

What's the merge strategy? It goes through arm-soc?

On 13/10/2021 22:50, Stephen Boyd wrote:

Quoting Dmitry Baryshkov (2021-10-07 09:16:13)

quoted

On 07/10/2021 18:48, Bjorn Andersson wrote:

quoted

On Sun 29 Aug 08:47 PDT 2021, Dmitry Baryshkov wrote:

quoted

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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>

Once we merge these, I expect that the boards will start crashing if
the kernel is booted using an existing DTB?

Is it okay to just merge the first 6 patches in the series now and
postpone these two until we've had the dts change sitting for a while?

Sure it is.

What's the merge strategy? It goes through arm-soc?

I think this should go through the clk tree. There is little chance of conflicts.


-- 
With best wishes
Dmitry

Quoting Dmitry Baryshkov (2021-10-14 02:53:41)

On 13/10/2021 22:50, Stephen Boyd wrote:

quoted

Quoting Dmitry Baryshkov (2021-10-07 09:16:13)

quoted

On 07/10/2021 18:48, Bjorn Andersson wrote:

quoted

On Sun 29 Aug 08:47 PDT 2021, Dmitry Baryshkov wrote:

quoted

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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>

Once we merge these, I expect that the boards will start crashing if
the kernel is booted using an existing DTB?

Is it okay to just merge the first 6 patches in the series now and
postpone these two until we've had the dts change sitting for a while?

Sure it is.

What's the merge strategy? It goes through arm-soc?

I think this should go through the clk tree. There is little chance of 
conflicts.

Ok. So I take the first 6 through clk tree and then we wait for the last
two?

Quoting Dmitry Baryshkov (2021-10-14 02:53:41)

On 13/10/2021 22:50, Stephen Boyd wrote:

quoted

Quoting Dmitry Baryshkov (2021-10-07 09:16:13)

quoted

On 07/10/2021 18:48, Bjorn Andersson wrote:

quoted

On Sun 29 Aug 08:47 PDT 2021, Dmitry Baryshkov wrote:

quoted

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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>

Once we merge these, I expect that the boards will start crashing if
the kernel is booted using an existing DTB?

Is it okay to just merge the first 6 patches in the series now and
postpone these two until we've had the dts change sitting for a while?

Sure it is.

What's the merge strategy? It goes through arm-soc?

I think this should go through the clk tree. There is little chance of 
conflicts.

The other thing that concerns me is that we don't have backwards
compat code. If things are going to start crashing that's not very nice.
Is there some way to make it work with old and new DTB for one release
so that we don't have to worry about this problem?

On Fri, 15 Oct 2021 at 03:04, Stephen Boyd [off-list ref] wrote:

Quoting Dmitry Baryshkov (2021-10-14 02:53:41)

quoted

On 13/10/2021 22:50, Stephen Boyd wrote:

quoted

Quoting Dmitry Baryshkov (2021-10-07 09:16:13)

quoted

On 07/10/2021 18:48, Bjorn Andersson wrote:

quoted

On Sun 29 Aug 08:47 PDT 2021, Dmitry Baryshkov wrote:

quoted

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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>

Once we merge these, I expect that the boards will start crashing if
the kernel is booted using an existing DTB?

Is it okay to just merge the first 6 patches in the series now and
postpone these two until we've had the dts change sitting for a while?

Sure it is.

What's the merge strategy? It goes through arm-soc?

I think this should go through the clk tree. There is little chance of
conflicts.

The other thing that concerns me is that we don't have backwards
compat code. If things are going to start crashing that's not very nice.
Is there some way to make it work with old and new DTB for one release
so that we don't have to worry about this problem?

I have to admit that I did not check that, but without the patch 7 the
dispcc and videocc would be compatible with the old DTB. The 'supply =
"mmcx"' would ensure that it is used if it is defined.

-- 
With best wishes
Dmitry

Quoting Dmitry Baryshkov (2021-08-29 08:47:50)

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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>
Reviewed-by: Rob Herring <robh@kernel.org>
---

Applied to clk-next

Quoting Dmitry Baryshkov (2021-08-29 08:47:51)

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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>
Reviewed-by: Rob Herring <robh@kernel.org>
---

Applied to clk-next

Quoting Dmitry Baryshkov (2021-08-29 08:47:52)

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

Applied to clk-next

Quoting Dmitry Baryshkov (2021-08-29 08:47:53)

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

Applied to clk-next

Quoting Dmitry Baryshkov (2021-08-29 08:47:54)

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

Applied to clk-next

On Sun, 29 Aug 2021 18:47:49 +0300, Dmitry Baryshkov 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-pm/1628767642-4008-1-git-send-email-rnayak@codeaurora.org/
(pending inclusion into 5.15)

[...]

Applied, thanks!

[6/8] arm64: dts: qcom: sm8250: remove mmcx regulator
      commit: 266e5cf39a0f25787cb66a36dde50799194062c6

Best regards,
-- 
Bjorn Andersson [off-list ref]

On 07/10/2021 18:48, Bjorn Andersson wrote:

On Sun 29 Aug 08:47 PDT 2021, Dmitry Baryshkov wrote:

quoted

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 <redacted>
Reviewed-by: Bjorn Andersson <redacted>

Once we merge these, I expect that the boards will start crashing if
the kernel is booted using an existing DTB?

Is it okay to just merge the first 6 patches in the series now and
postpone these two until we've had the dts change sitting for a while?

Bjorn, the changes 1-6 went into 5.15. I suppose it's time to merge the last two patches?

Regards,
Bjorn

quoted

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

-- 
With best wishes
Dmitry