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

[PATCH v7 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-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(-)

[PATCH v7 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>
---
 .../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

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

[Stephen Boyd]

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 <dmitry.baryshkov@linaro.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---

Applied to clk-next

[PATCH v7 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>
---
 .../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

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

[Stephen Boyd]

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 <dmitry.baryshkov@linaro.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---

Applied to clk-next

[PATCH v7 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.33.0

Re: [PATCH v7 3/8] clk: qcom: dispcc-sm8250: use runtime PM for the clock controller

[Bjorn Andersson]

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 <bjorn.andersson@linaro.org>



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

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

Re: [PATCH v7 3/8] clk: qcom: dispcc-sm8250: use runtime PM for the clock controller

[Stephen Boyd]

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 <dmitry.baryshkov@linaro.org>
> ---

Applied to clk-next

[PATCH v7 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.33.0

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

[Bjorn Andersson]

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 <bjorn.andersson@linaro.org>

Regards,
Bjorn

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

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

[Stephen Boyd]

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 <dmitry.baryshkov@linaro.org>
> ---

Applied to clk-next

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

Re: [PATCH v7 5/8] clk: qcom: gdsc: enable optional power domain support

[Bjorn Andersson]

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 <bjorn.andersson@linaro.org>

Regards,
Bjorn

> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  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
> 

Re: [PATCH v7 5/8] clk: qcom: gdsc: enable optional power domain support

[Stephen Boyd]

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 <dmitry.baryshkov@linaro.org>
> ---

Applied to clk-next

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

[PATCH v7 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.33.0

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

[Bjorn Andersson]

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

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

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

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

[Dmitry Baryshkov]

On 07/10/2021 18:48, Bjorn Andersson wrote:
> 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 <dmitry.baryshkov@linaro.org>
>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> 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
> 
>> ---
>>   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

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

[Stephen Boyd]

Quoting Dmitry Baryshkov (2021-10-07 09:16:13)
> On 07/10/2021 18:48, Bjorn Andersson wrote:
> > 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 <dmitry.baryshkov@linaro.org>
> >> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > 
> > 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?

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

[Dmitry Baryshkov]

On 13/10/2021 22:50, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2021-10-07 09:16:13)
>> On 07/10/2021 18:48, Bjorn Andersson wrote:
>>> 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 <dmitry.baryshkov@linaro.org>
>>>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
>>>
>>> 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

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

[Stephen Boyd]

Quoting Dmitry Baryshkov (2021-10-14 02:53:41)
> On 13/10/2021 22:50, Stephen Boyd wrote:
> > Quoting Dmitry Baryshkov (2021-10-07 09:16:13)
> >> On 07/10/2021 18:48, Bjorn Andersson wrote:
> >>> 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 <dmitry.baryshkov@linaro.org>
> >>>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> >>>
> >>> 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?

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

[Stephen Boyd]

Quoting Dmitry Baryshkov (2021-10-14 02:53:41)
> On 13/10/2021 22:50, Stephen Boyd wrote:
> > Quoting Dmitry Baryshkov (2021-10-07 09:16:13)
> >> On 07/10/2021 18:48, Bjorn Andersson wrote:
> >>> 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 <dmitry.baryshkov@linaro.org>
> >>>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> >>>
> >>> 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?

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

[Dmitry Baryshkov]

On Fri, 15 Oct 2021 at 03:04, Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Dmitry Baryshkov (2021-10-14 02:53:41)
> > On 13/10/2021 22:50, Stephen Boyd wrote:
> > > Quoting Dmitry Baryshkov (2021-10-07 09:16:13)
> > >> On 07/10/2021 18:48, Bjorn Andersson wrote:
> > >>> 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 <dmitry.baryshkov@linaro.org>
> > >>>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > >>>
> > >>> 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

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

[Bjorn Andersson]

On Thu 14 Oct 17:25 PDT 2021, Stephen Boyd wrote:

> Quoting Dmitry Baryshkov (2021-10-14 17:15:39)
> > On Fri, 15 Oct 2021 at 03:04, Stephen Boyd <sboyd@kernel.org> wrote:
> > >
> > > Quoting Dmitry Baryshkov (2021-10-14 02:53:41)
> > > > On 13/10/2021 22:50, Stephen Boyd wrote:
> > > > > Quoting Dmitry Baryshkov (2021-10-07 09:16:13)
> > > > >> On 07/10/2021 18:48, Bjorn Andersson wrote:
> > > > >>> 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 <dmitry.baryshkov@linaro.org>
> > > > >>>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > >>>
> > > > >>> 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.
> > 
> 
> So I can skip patch 7 and take everything else?

If you take patch 1-5 and I pick up patch 6 for v5.16 then we should
have transitioned with backwards compatibility.

Then we can resubmit 7 & 8 in 1-2 releases.

Regards,
Bjorn

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

[Stephen Boyd]

Quoting Bjorn Andersson (2021-10-14 17:46:51)
> On Thu 14 Oct 17:25 PDT 2021, Stephen Boyd wrote:
> 
> > Quoting Dmitry Baryshkov (2021-10-14 17:15:39)
> > > On Fri, 15 Oct 2021 at 03:04, Stephen Boyd <sboyd@kernel.org> wrote:
> > > >
> > > > Quoting Dmitry Baryshkov (2021-10-14 02:53:41)
> > > > > On 13/10/2021 22:50, Stephen Boyd wrote:
> > > > > > Quoting Dmitry Baryshkov (2021-10-07 09:16:13)
> > > > > >> On 07/10/2021 18:48, Bjorn Andersson wrote:
> > > > > >>> 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 <dmitry.baryshkov@linaro.org>
> > > > > >>>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > > > >>>
> > > > > >>> 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.
> > > 
> > 
> > So I can skip patch 7 and take everything else?
> 
> If you take patch 1-5 and I pick up patch 6 for v5.16 then we should
> have transitioned with backwards compatibility.
> 

Ok. Sounds like a deal.

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

[Dmitry Baryshkov]

On 07/10/2021 18:48, Bjorn Andersson wrote:
> 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 <dmitry.baryshkov@linaro.org>
>> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> 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
> 
>> ---
>>   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

[PATCH v7 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.33.0

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

[Ulf Hansson]

On Sun, 29 Aug 2021 at 17:47, 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-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 <ulf.hansson@linaro.org>

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

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

[Dmitry Baryshkov]

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

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

[Bjorn Andersson]

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 <bjorn.andersson@linaro.org>