[PATCH v1 0/3] clk: qcom: handle power domains supplies for GDSC

[PATCH v1 0/3] clk: qcom: handle power domains supplies for GDSC

[Dmitry Baryshkov]

On SM8250 MDSS_GDSC (and the rest of display clock controller) is
supplied power by MMCX power domain. Handle this link in GDSC code by
binding the power domain in dts file.

This patchset depends on [1]

Changes since RFC:
 - Fix naming of gdsc_supply_on/gdsc_supply_off functions
 - Fix detaching of solo gdsc's power domain in error handling code
 - Drop the dts patch, as respective display nodes are still not
   submitted to the mailing list.

[1] https://lore.kernel.org/linux-arm-msm/20200927190653.13876-1-jonathan@marek.ca/

[PATCH v1 1/3] 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>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../bindings/clock/qcom,sdm845-dispcc.yaml    | 28 +++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/qcom,sdm845-dispcc.yaml b/Documentation/devicetree/bindings/clock/qcom,sdm845-dispcc.yaml
index 4a3be733d042..ff0db55470ac 100644
--- a/Documentation/devicetree/bindings/clock/qcom,sdm845-dispcc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,sdm845-dispcc.yaml
@@ -58,6 +58,16 @@ properties:
   reg:
     maxItems: 1
 
+  power-domains:
+    maxItems: 1
+
+  power-domain-names:
+    items:
+      - const: mmcx
+
+  required-opps:
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -73,6 +83,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>;
@@ -97,5 +108,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: [PATCH v1 1/3] dt-bindings: clock: qcom,dispcc: document power domain bindings

[Stephen Boyd]

Quoting Dmitry Baryshkov (2020-10-05 15:59:12)
> SM8250 requires special power domain for accessing MMDS_GDSC registers.

Heh, not sure it's special.

> Add bindings for the MMCX power domain.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
>  .../bindings/clock/qcom,sdm845-dispcc.yaml    | 28 +++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/clock/qcom,sdm845-dispcc.yaml b/Documentation/devicetree/bindings/clock/qcom,sdm845-dispcc.yaml
> index 4a3be733d042..ff0db55470ac 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,sdm845-dispcc.yaml
> +++ b/Documentation/devicetree/bindings/clock/qcom,sdm845-dispcc.yaml
> @@ -97,5 +108,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 {

Do we need this node in the example? I think it isn't required but I
guess it's OK.

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

Re: [PATCH v1 1/3] dt-bindings: clock: qcom,dispcc: document power domain bindings

[Dmitry Baryshkov]

On 14/10/2020 03:46, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2020-10-05 15:59:12)
>> SM8250 requires special power domain for accessing MMDS_GDSC registers.
> 
> Heh, not sure it's special.
> 
>> Add bindings for the MMCX power domain.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> Reviewed-by: Rob Herring <robh@kernel.org>
>> ---
>>   .../bindings/clock/qcom,sdm845-dispcc.yaml    | 28 +++++++++++++++++++
>>   1 file changed, 28 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/clock/qcom,sdm845-dispcc.yaml b/Documentation/devicetree/bindings/clock/qcom,sdm845-dispcc.yaml
>> index 4a3be733d042..ff0db55470ac 100644
>> --- a/Documentation/devicetree/bindings/clock/qcom,sdm845-dispcc.yaml
>> +++ b/Documentation/devicetree/bindings/clock/qcom,sdm845-dispcc.yaml
>> @@ -97,5 +108,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 {
> 
> Do we need this node in the example? I think it isn't required but I
> guess it's OK.

It is to be able to resolve "power-domains" and "required-opps" 
properties values.

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


-- 
With best wishes
Dmitry

Re: [PATCH v1 1/3] dt-bindings: clock: qcom,dispcc: document power domain bindings

[Stephen Boyd]

Quoting Dmitry Baryshkov (2020-10-05 15:59:12)
> @@ -97,5 +108,22 @@ examples:
>        #clock-cells = <1>;
>        #reset-cells = <1>;
>        #power-domain-cells = <1>;
> +      /* this is a part of sm8250 setup the power domain example */

What does this comment mean?

> +      power-domains = <&rpmhpd SDM845_CX>;
> +      power-domain-names = "mmcx";
> +      required-opps = <&rpmhpd_opp_low_svs>;
> +    };

[PATCH v1 2/3] clk: qcom: gdsc: enable external switchable power domain

[Dmitry Baryshkov]

On SM8250 MDSS_GDSC (and respective dispcc clocks) are children of MMCX
power domain. MMCX needs to be enabled to be able to access GDSC
registers and to enable display clocks. 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..c9e1619074f8 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;
 }
 
+static int gdsc_toggle_supply_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;
+}
+
+static int gdsc_toggle_supply_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_supply_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_supply_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 && scs[i]->pd_dev != 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: [PATCH v1 2/3] clk: qcom: gdsc: enable external switchable power domain

[Stephen Boyd]

Quoting Dmitry Baryshkov (2020-10-05 15:59:13)
> On SM8250 MDSS_GDSC (and respective dispcc clocks) are children of MMCX
> power domain. MMCX needs to be enabled to be able to access GDSC
> registers and to enable display clocks. Use dev_pm/opp to enable
> corresponding power domain.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---

A general question is why is this done in the gdsc code instead of
somewhere generic? It seems that genpds may need to change the
performance state of other genpds. I vaguely recall that genpd supports
connecting different power domains together so maybe this could all be
handled in the genpd layer instead of here? Then a regulator could be
put behind a genpd and similarly be connected to the gdsc and turned on
before turning on the gdsc?

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

Please document these fields.

>  };

Re: [PATCH v1 2/3] clk: qcom: gdsc: enable external switchable power domain

[Dmitry Baryshkov]

On 14/10/2020 05:15, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2020-10-05 15:59:13)
>> On SM8250 MDSS_GDSC (and respective dispcc clocks) are children of MMCX
>> power domain. MMCX needs to be enabled to be able to access GDSC
>> registers and to enable display clocks. Use dev_pm/opp to enable
>> corresponding power domain.
>>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
> 
> A general question is why is this done in the gdsc code instead of
> somewhere generic? It seems that genpds may need to change the
> performance state of other genpds. I vaguely recall that genpd supports
> connecting different power domains together so maybe this could all be
> handled in the genpd layer instead of here? Then a regulator could be
> put behind a genpd and similarly be connected to the gdsc and turned on
> before turning on the gdsc?

Basically because we need not only to enable the genpd, but also to set 
performance state. This would mean creating a separate regulator driver 
calling dev_pm_genpd_set_performance_state() from enable/disable paths.
Does that seem like a better solution to you?

> 
>>   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.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;
> 
> Please document these fields.

Will do.

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/3] clk: qcom: gdsc: enable external switchable power domain

[Stephen Boyd]

Quoting Dmitry Baryshkov (2020-10-14 02:44:31)
> On 14/10/2020 05:15, Stephen Boyd wrote:
> > Quoting Dmitry Baryshkov (2020-10-05 15:59:13)
> >> On SM8250 MDSS_GDSC (and respective dispcc clocks) are children of MMCX
> >> power domain. MMCX needs to be enabled to be able to access GDSC
> >> registers and to enable display clocks. Use dev_pm/opp to enable
> >> corresponding power domain.
> >>
> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >> ---
> > 
> > A general question is why is this done in the gdsc code instead of
> > somewhere generic? It seems that genpds may need to change the
> > performance state of other genpds. I vaguely recall that genpd supports
> > connecting different power domains together so maybe this could all be
> > handled in the genpd layer instead of here? Then a regulator could be
> > put behind a genpd and similarly be connected to the gdsc and turned on
> > before turning on the gdsc?
> 
> Basically because we need not only to enable the genpd, but also to set 
> performance state. This would mean creating a separate regulator driver 
> calling dev_pm_genpd_set_performance_state() from enable/disable paths.
> Does that seem like a better solution to you?

It does sound like a better solution to me. Unfortunately we already
have that generic code here in the gdsc file so lifting it up into the
genpd layer is a bunch of work. It is certainly the end goal.

[PATCH v1 3/3] 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 07a98d3f882d..3941054a7b07 100644
--- a/drivers/clk/qcom/dispcc-sm8250.c
+++ b/drivers/clk/qcom/dispcc-sm8250.c
@@ -963,6 +963,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

Re: [PATCH v1 3/3] clk: qcom: dispcc-sm8250: handle MMCX power domain

[Stephen Boyd]

Quoting Dmitry Baryshkov (2020-10-05 15:59:14)
> diff --git a/drivers/clk/qcom/dispcc-sm8250.c b/drivers/clk/qcom/dispcc-sm8250.c
> index 07a98d3f882d..3941054a7b07 100644
> --- a/drivers/clk/qcom/dispcc-sm8250.c
> +++ b/drivers/clk/qcom/dispcc-sm8250.c
> @@ -963,6 +963,8 @@ static struct gdsc mdss_gdsc = {
>         },
>         .pwrsts = PWRSTS_OFF_ON,
>         .flags = HW_CTRL,
> +       .domain = "mmcx",
> +       .perf_idx = 0,

Does a perf_idx of 0 mean off? Or just "not off"?

Re: [PATCH v1 3/3] clk: qcom: dispcc-sm8250: handle MMCX power domain

[Dmitry Baryshkov]

On 14/10/2020 05:16, Stephen Boyd wrote:
> Quoting Dmitry Baryshkov (2020-10-05 15:59:14)
>> diff --git a/drivers/clk/qcom/dispcc-sm8250.c b/drivers/clk/qcom/dispcc-sm8250.c
>> index 07a98d3f882d..3941054a7b07 100644
>> --- a/drivers/clk/qcom/dispcc-sm8250.c
>> +++ b/drivers/clk/qcom/dispcc-sm8250.c
>> @@ -963,6 +963,8 @@ static struct gdsc mdss_gdsc = {
>>          },
>>          .pwrsts = PWRSTS_OFF_ON,
>>          .flags = HW_CTRL,
>> +       .domain = "mmcx",
>> +       .perf_idx = 0,
> 
> Does a perf_idx of 0 mean off? Or just "not off"?


It means 'use performance state with index 0 declared in the DTS'.


-- 
With best wishes
Dmitry