[RFC 0/4] clk: qcom: handle power domains links for GDSC

[RFC 0/4] clk: qcom: handle power domains links for GDSC

[Dmitry Baryshkov]

On SM8250 MDSS_GDSC requires MMCX domain to be powered to access GDSC
registers. Handle this requirement in the gdsc code by binding the power
domain via dts file. The example in the schema file demonstrates this
binding (which is not required for SDM845).

[RFC 1/4] dt-bindings: clock: qcom,dispcc: document power domain bindings

[Dmitry Baryshkov]

SM8250 requires special power domain for accessing MMDS_GDSC registers.
Add bindings for the MMCX power domain.

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

diff --git a/Documentation/devicetree/bindings/clock/qcom,dispcc.yaml b/Documentation/devicetree/bindings/clock/qcom,dispcc.yaml
index 0b905a4e9ada..6325d9969913 100644
--- a/Documentation/devicetree/bindings/clock/qcom,dispcc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,dispcc.yaml
@@ -66,6 +66,16 @@ properties:
   reg:
     maxItems: 1
 
+  power-domains:
+    maxItems: 1
+
+  power-domain-names:
+    items:
+      - const: mmcx
+
+  required-opps:
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -81,6 +91,7 @@ examples:
   - |
     #include <dt-bindings/clock/qcom,gcc-sdm845.h>
     #include <dt-bindings/clock/qcom,rpmh.h>
+    #include <dt-bindings/power/qcom-rpmpd.h>
     clock-controller@af00000 {
       compatible = "qcom,sdm845-dispcc";
       reg = <0x0af00000 0x10000>;
@@ -105,5 +116,22 @@ examples:
       #clock-cells = <1>;
       #reset-cells = <1>;
       #power-domain-cells = <1>;
+      /* this is a part of sm8250 setup the power domain example */
+      power-domains = <&rpmhpd SDM845_CX>;
+      power-domain-names = "mmcx";
+      required-opps = <&rpmhpd_opp_low_svs>;
+    };
+    rpmhpd: power-controller {
+      compatible = "qcom,sdm845-rpmhpd";
+      #power-domain-cells = <1>;
+      operating-points-v2 = <&rpmhpd_opp_table>;
+
+      rpmhpd_opp_table: opp-table {
+        compatible = "operating-points-v2";
+
+        rpmhpd_opp_low_svs: opp3 {
+          opp-level = <RPMH_REGULATOR_LEVEL_LOW_SVS>;
+        };
+      };
     };
 ...
-- 
2.28.0

Re: [RFC 1/4] dt-bindings: clock: qcom,dispcc: document power domain bindings

[Rob Herring]

On Fri, 11 Sep 2020 16:09:47 +0300, Dmitry Baryshkov wrote:
> SM8250 requires special power domain for accessing MMDS_GDSC registers.
> Add bindings for the MMCX power domain.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  .../bindings/clock/qcom,dispcc.yaml           | 28 +++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[RFC 2/4] clk: qcom: gdsc: enable external switchable power domain

[Dmitry Baryshkov]

Some GDSCs (SM8250's MDSS_GDSC for example) need switchable power domain
to be on to be able to access hardware registers. Use dev_pm/opp to
enable corresponding power domain.

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

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index bfc4ac02f9ea..a522e062a79a 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_opp.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/reset-controller.h>
@@ -110,13 +111,31 @@ static int gdsc_poll_status(struct gdsc *sc, enum gdsc_status status)
 	return -ETIMEDOUT;
 }
 
+int gdsc_toggle_on(struct gdsc *sc)
+{
+	if (sc->rsupply)
+		return regulator_enable(sc->rsupply);
+	if (sc->pd_dev)
+		return dev_pm_genpd_set_performance_state(sc->pd_dev, sc->pd_opp);
+	return 0;
+}
+
+int gdsc_toggle_off(struct gdsc *sc)
+{
+	if (sc->pd_dev)
+		return dev_pm_genpd_set_performance_state(sc->pd_dev, 0);
+	if (sc->rsupply)
+		return regulator_disable(sc->rsupply);
+	return 0;
+}
+
 static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
 {
 	int ret;
 	u32 val = (status == GDSC_ON) ? 0 : SW_COLLAPSE_MASK;
 
-	if (status == GDSC_ON && sc->rsupply) {
-		ret = regulator_enable(sc->rsupply);
+	if (status == GDSC_ON) {
+		ret = gdsc_toggle_on(sc);
 		if (ret < 0)
 			return ret;
 	}
@@ -153,8 +172,8 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
 	ret = gdsc_poll_status(sc, status);
 	WARN(ret, "%s status stuck at 'o%s'", sc->pd.name, status ? "ff" : "n");
 
-	if (!ret && status == GDSC_OFF && sc->rsupply) {
-		ret = regulator_disable(sc->rsupply);
+	if (!ret && status == GDSC_OFF) {
+		ret = gdsc_toggle_off(sc);
 		if (ret < 0)
 			return ret;
 	}
@@ -407,6 +426,27 @@ int gdsc_register(struct gdsc_desc *desc,
 			return PTR_ERR(scs[i]->rsupply);
 	}
 
+	for (i = 0; i < num; i++) {
+		if (!scs[i] || !scs[i]->domain)
+			continue;
+
+		scs[i]->pd_opp = of_get_required_opp_performance_state(dev->of_node, scs[i]->perf_idx);
+		if (scs[i]->pd_opp < 0)
+			return scs[i]->pd_opp;
+
+		scs[i]->pd_dev = dev_pm_domain_attach_by_name(dev, scs[i]->domain);
+		if (IS_ERR(scs[i]->pd_dev)) {
+			ret = PTR_ERR(scs[i]->pd_dev);
+			/* Single domain has been already attached, so reuse dev */
+			if (ret == -EEXIST) {
+				scs[i]->pd_dev = dev;
+			} else {
+				scs[i]->pd_dev = NULL;
+				goto pm_detach;
+			}
+		}
+	}
+
 	data->num_domains = num;
 	for (i = 0; i < num; i++) {
 		if (!scs[i])
@@ -428,6 +468,12 @@ int gdsc_register(struct gdsc_desc *desc,
 	}
 
 	return of_genpd_add_provider_onecell(dev->of_node, data);
+
+pm_detach:
+	for (i = 0; i < num; i++)
+		if (scs[i]->pd_dev)
+			dev_pm_domain_detach(scs[i]->pd_dev, false);
+	return ret;
 }
 
 void gdsc_unregister(struct gdsc_desc *desc)
@@ -443,6 +489,8 @@ void gdsc_unregister(struct gdsc_desc *desc)
 			continue;
 		if (scs[i]->parent)
 			pm_genpd_remove_subdomain(scs[i]->parent, &scs[i]->pd);
+		if (scs[i]->pd_dev && scs[i]->pd_dev != dev)
+			dev_pm_domain_detach(scs[i]->pd_dev, true);
 	}
 	of_genpd_del_provider(dev->of_node);
 }
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index bd537438c793..d58575f8f25f 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -57,6 +57,11 @@ struct gdsc {
 
 	const char 			*supply;
 	struct regulator		*rsupply;
+
+	const char			*domain;
+	unsigned int			perf_idx;
+	struct device			*pd_dev;
+	int				pd_opp;
 };
 
 struct gdsc_desc {
-- 
2.28.0

Re: [RFC 2/4] clk: qcom: gdsc: enable external switchable power domain

[Bjorn Andersson]

On Fri 11 Sep 08:09 CDT 2020, Dmitry Baryshkov wrote:

> Some GDSCs (SM8250's MDSS_GDSC for example) need switchable power domain
> to be on to be able to access hardware registers. Use dev_pm/opp to
                           ^
What you describe here ----+ sounds like the GDSC controller is part of
the power-domain specified and hence needs to be enabled in order to
control the GDSC.

But in contrast what the patch implements is a mechanism where the
GDSC power-domain is a child of some other power-domain. So the commit
message needs to better reflect what's implemented.

Then looking at the DT representation I think it says that the
controller sits in the specified power-domain, rather than the exposed
power-domain...

> enable corresponding power domain.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/clk/qcom/gdsc.c | 56 ++++++++++++++++++++++++++++++++++++++---
>  drivers/clk/qcom/gdsc.h |  5 ++++
>  2 files changed, 57 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index bfc4ac02f9ea..a522e062a79a 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_opp.h>
>  #include <linux/regmap.h>
>  #include <linux/regulator/consumer.h>
>  #include <linux/reset-controller.h>
> @@ -110,13 +111,31 @@ static int gdsc_poll_status(struct gdsc *sc, enum gdsc_status status)
>  	return -ETIMEDOUT;
>  }
>  
> +int gdsc_toggle_on(struct gdsc *sc)

This should be "static" and I think you should include "supply" in the
name to denote that it doesn't turn on the gdsc, but rather its supply.

> +{
> +	if (sc->rsupply)
> +		return regulator_enable(sc->rsupply);
> +	if (sc->pd_dev)
> +		return dev_pm_genpd_set_performance_state(sc->pd_dev, sc->pd_opp);
> +	return 0;
> +}
> +
> +int gdsc_toggle_off(struct gdsc *sc)
> +{
> +	if (sc->pd_dev)
> +		return dev_pm_genpd_set_performance_state(sc->pd_dev, 0);
> +	if (sc->rsupply)
> +		return regulator_disable(sc->rsupply);
> +	return 0;
> +}
> +
>  static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
>  {
>  	int ret;
>  	u32 val = (status == GDSC_ON) ? 0 : SW_COLLAPSE_MASK;
>  
> -	if (status == GDSC_ON && sc->rsupply) {
> -		ret = regulator_enable(sc->rsupply);
> +	if (status == GDSC_ON) {
> +		ret = gdsc_toggle_on(sc);
>  		if (ret < 0)
>  			return ret;
>  	}
> @@ -153,8 +172,8 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
>  	ret = gdsc_poll_status(sc, status);
>  	WARN(ret, "%s status stuck at 'o%s'", sc->pd.name, status ? "ff" : "n");
>  
> -	if (!ret && status == GDSC_OFF && sc->rsupply) {
> -		ret = regulator_disable(sc->rsupply);
> +	if (!ret && status == GDSC_OFF) {
> +		ret = gdsc_toggle_off(sc);
>  		if (ret < 0)
>  			return ret;
>  	}
> @@ -407,6 +426,27 @@ int gdsc_register(struct gdsc_desc *desc,
>  			return PTR_ERR(scs[i]->rsupply);
>  	}
>  
> +	for (i = 0; i < num; i++) {
> +		if (!scs[i] || !scs[i]->domain)
> +			continue;
> +
> +		scs[i]->pd_opp = of_get_required_opp_performance_state(dev->of_node, scs[i]->perf_idx);
> +		if (scs[i]->pd_opp < 0)
> +			return scs[i]->pd_opp;
> +
> +		scs[i]->pd_dev = dev_pm_domain_attach_by_name(dev, scs[i]->domain);
> +		if (IS_ERR(scs[i]->pd_dev)) {
> +			ret = PTR_ERR(scs[i]->pd_dev);
> +			/* Single domain has been already attached, so reuse dev */
> +			if (ret == -EEXIST) {
> +				scs[i]->pd_dev = dev;
> +			} else {
> +				scs[i]->pd_dev = NULL;
> +				goto pm_detach;
> +			}
> +		}
> +	}
> +
>  	data->num_domains = num;
>  	for (i = 0; i < num; i++) {
>  		if (!scs[i])
> @@ -428,6 +468,12 @@ int gdsc_register(struct gdsc_desc *desc,
>  	}
>  
>  	return of_genpd_add_provider_onecell(dev->of_node, data);
> +
> +pm_detach:
> +	for (i = 0; i < num; i++)
> +		if (scs[i]->pd_dev)
> +			dev_pm_domain_detach(scs[i]->pd_dev, false);

I think that if dev_pm_domain_attach_by_name() returned -EEXIST you
will attempt to detach the main device's domain here.

> +	return ret;
>  }
>  
>  void gdsc_unregister(struct gdsc_desc *desc)
> @@ -443,6 +489,8 @@ void gdsc_unregister(struct gdsc_desc *desc)
>  			continue;
>  		if (scs[i]->parent)
>  			pm_genpd_remove_subdomain(scs[i]->parent, &scs[i]->pd);
> +		if (scs[i]->pd_dev && scs[i]->pd_dev != dev)
> +			dev_pm_domain_detach(scs[i]->pd_dev, true);

Ditto

Regards,
Bjorn

>  	}
>  	of_genpd_del_provider(dev->of_node);
>  }
> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> index bd537438c793..d58575f8f25f 100644
> --- a/drivers/clk/qcom/gdsc.h
> +++ b/drivers/clk/qcom/gdsc.h
> @@ -57,6 +57,11 @@ struct gdsc {
>  
>  	const char 			*supply;
>  	struct regulator		*rsupply;
> +
> +	const char			*domain;
> +	unsigned int			perf_idx;
> +	struct device			*pd_dev;
> +	int				pd_opp;
>  };
>  
>  struct gdsc_desc {
> -- 
> 2.28.0
> 

[RFC 3/4] clk: qcom: dispcc-sm8250: handle MMCX power domain

[Dmitry Baryshkov]

On SM8250 MMCX power domain is required to access MMDS_GDSC registers.
Enable using this power domain for the gdsc.

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

diff --git a/drivers/clk/qcom/dispcc-sm8250.c b/drivers/clk/qcom/dispcc-sm8250.c
index 7c0f384a3a42..fc5fb2b2fe5e 100644
--- a/drivers/clk/qcom/dispcc-sm8250.c
+++ b/drivers/clk/qcom/dispcc-sm8250.c
@@ -970,6 +970,8 @@ static struct gdsc mdss_gdsc = {
 	},
 	.pwrsts = PWRSTS_OFF_ON,
 	.flags = HW_CTRL,
+	.domain = "mmcx",
+	.perf_idx = 0,
 };
 
 static struct clk_regmap *disp_cc_sm8250_clocks[] = {
-- 
2.28.0

[RFC 4/4] arm64: dts: qcom: sm8250: pin MMCX to stop the board from crashing

[Dmitry Baryshkov]

Use MMCX domain to power up MDSS_GDSC.

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

diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index 5045fe89a494..c42640228ceb 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -1732,6 +1732,9 @@ dp_out: endpoint {
 		dispcc: clock-controller@af00000 {
 			compatible = "qcom,sm8250-dispcc";
 			reg = <0 0x0af00000 0 0x10000>;
+			power-domains = <&rpmhpd SM8250_MMCX>;
+			power-domain-names = "mmcx";
+			required-opps = <&rpmhpd_opp_low_svs>;
 			clocks = <&rpmhcc RPMH_CXO_CLK>,
 				 <&dsi0_phy 0>,
 				 <&dsi0_phy 1>,
-- 
2.28.0

Re: [RFC 0/4] clk: qcom: handle power domains links for GDSC

[Stephen Boyd]

Quoting Dmitry Baryshkov (2020-09-11 06:09:46)
> On SM8250 MDSS_GDSC requires MMCX domain to be powered to access GDSC
> registers. Handle this requirement in the gdsc code by binding the power
> domain via dts file. The example in the schema file demonstrates this
> binding (which is not required for SDM845).
> 

Please Cc the linux-clk list on clk patches. It might be obvious now
that I don't always look at this email pile to fish out clk things.