linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 0/3] clk: qcom: handle power domains supplies for GDSC
@ 2020-10-05 22:59 Dmitry Baryshkov
  2020-10-05 22:59 ` [PATCH v1 1/3] dt-bindings: clock: qcom,dispcc: document power domain bindings Dmitry Baryshkov
                   ` (2 more replies)
  0 siblings, 3 replies; 12+ messages in thread
From: Dmitry Baryshkov @ 2020-10-05 22:59 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring, Jonathan Marek,
	Stephen Boyd, Michael Turquette
  Cc: linux-arm-msm, Manivannan Sadhasivam, devicetree, linux-clk

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/



^ permalink raw reply	[flat|nested] 12+ messages in thread

* [PATCH v1 1/3] dt-bindings: clock: qcom,dispcc: document power domain bindings
  2020-10-05 22:59 [PATCH v1 0/3] clk: qcom: handle power domains supplies for GDSC Dmitry Baryshkov
@ 2020-10-05 22:59 ` Dmitry Baryshkov
  2020-10-14  0:46   ` Stephen Boyd
  2020-10-14  2:17   ` Stephen Boyd
  2020-10-05 22:59 ` [PATCH v1 2/3] clk: qcom: gdsc: enable external switchable power domain Dmitry Baryshkov
  2020-10-05 22:59 ` [PATCH v1 3/3] clk: qcom: dispcc-sm8250: handle MMCX " Dmitry Baryshkov
  2 siblings, 2 replies; 12+ messages in thread
From: Dmitry Baryshkov @ 2020-10-05 22:59 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring, Jonathan Marek,
	Stephen Boyd, Michael Turquette
  Cc: linux-arm-msm, Manivannan Sadhasivam, devicetree, linux-clk, Rob Herring

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v1 2/3] clk: qcom: gdsc: enable external switchable power domain
  2020-10-05 22:59 [PATCH v1 0/3] clk: qcom: handle power domains supplies for GDSC Dmitry Baryshkov
  2020-10-05 22:59 ` [PATCH v1 1/3] dt-bindings: clock: qcom,dispcc: document power domain bindings Dmitry Baryshkov
@ 2020-10-05 22:59 ` Dmitry Baryshkov
  2020-10-14  2:15   ` Stephen Boyd
  2020-10-05 22:59 ` [PATCH v1 3/3] clk: qcom: dispcc-sm8250: handle MMCX " Dmitry Baryshkov
  2 siblings, 1 reply; 12+ messages in thread
From: Dmitry Baryshkov @ 2020-10-05 22:59 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring, Jonathan Marek,
	Stephen Boyd, Michael Turquette
  Cc: linux-arm-msm, Manivannan Sadhasivam, devicetree, linux-clk

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* [PATCH v1 3/3] clk: qcom: dispcc-sm8250: handle MMCX power domain
  2020-10-05 22:59 [PATCH v1 0/3] clk: qcom: handle power domains supplies for GDSC Dmitry Baryshkov
  2020-10-05 22:59 ` [PATCH v1 1/3] dt-bindings: clock: qcom,dispcc: document power domain bindings Dmitry Baryshkov
  2020-10-05 22:59 ` [PATCH v1 2/3] clk: qcom: gdsc: enable external switchable power domain Dmitry Baryshkov
@ 2020-10-05 22:59 ` Dmitry Baryshkov
  2020-10-14  2:16   ` Stephen Boyd
  2 siblings, 1 reply; 12+ messages in thread
From: Dmitry Baryshkov @ 2020-10-05 22:59 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring, Jonathan Marek,
	Stephen Boyd, Michael Turquette
  Cc: linux-arm-msm, Manivannan Sadhasivam, devicetree, linux-clk

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


^ permalink raw reply related	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 1/3] dt-bindings: clock: qcom,dispcc: document power domain bindings
  2020-10-05 22:59 ` [PATCH v1 1/3] dt-bindings: clock: qcom,dispcc: document power domain bindings Dmitry Baryshkov
@ 2020-10-14  0:46   ` Stephen Boyd
  2020-10-14  9:36     ` Dmitry Baryshkov
  2020-10-14  2:17   ` Stephen Boyd
  1 sibling, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2020-10-14  0:46 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Dmitry Baryshkov, Jonathan Marek,
	Michael Turquette, Rob Herring
  Cc: linux-arm-msm, Manivannan Sadhasivam, devicetree, linux-clk, Rob Herring

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 {

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 2/3] clk: qcom: gdsc: enable external switchable power domain
  2020-10-05 22:59 ` [PATCH v1 2/3] clk: qcom: gdsc: enable external switchable power domain Dmitry Baryshkov
@ 2020-10-14  2:15   ` Stephen Boyd
  2020-10-14  9:44     ` Dmitry Baryshkov
  0 siblings, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2020-10-14  2:15 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Dmitry Baryshkov, Jonathan Marek,
	Michael Turquette, Rob Herring
  Cc: linux-arm-msm, Manivannan Sadhasivam, devicetree, linux-clk

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.

>  };

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 3/3] clk: qcom: dispcc-sm8250: handle MMCX power domain
  2020-10-05 22:59 ` [PATCH v1 3/3] clk: qcom: dispcc-sm8250: handle MMCX " Dmitry Baryshkov
@ 2020-10-14  2:16   ` Stephen Boyd
  2020-10-14  9:46     ` Dmitry Baryshkov
  0 siblings, 1 reply; 12+ messages in thread
From: Stephen Boyd @ 2020-10-14  2:16 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Dmitry Baryshkov, Jonathan Marek,
	Michael Turquette, Rob Herring
  Cc: linux-arm-msm, Manivannan Sadhasivam, devicetree, linux-clk

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 1/3] dt-bindings: clock: qcom,dispcc: document power domain bindings
  2020-10-05 22:59 ` [PATCH v1 1/3] dt-bindings: clock: qcom,dispcc: document power domain bindings Dmitry Baryshkov
  2020-10-14  0:46   ` Stephen Boyd
@ 2020-10-14  2:17   ` Stephen Boyd
  1 sibling, 0 replies; 12+ messages in thread
From: Stephen Boyd @ 2020-10-14  2:17 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Dmitry Baryshkov, Jonathan Marek,
	Michael Turquette, Rob Herring
  Cc: linux-arm-msm, Manivannan Sadhasivam, devicetree, linux-clk, Rob Herring

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>;
> +    };

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 1/3] dt-bindings: clock: qcom,dispcc: document power domain bindings
  2020-10-14  0:46   ` Stephen Boyd
@ 2020-10-14  9:36     ` Dmitry Baryshkov
  0 siblings, 0 replies; 12+ messages in thread
From: Dmitry Baryshkov @ 2020-10-14  9:36 UTC (permalink / raw)
  To: Stephen Boyd, Andy Gross, Bjorn Andersson, Jonathan Marek,
	Michael Turquette, Rob Herring
  Cc: linux-arm-msm, Manivannan Sadhasivam, devicetree, linux-clk, Rob Herring

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 2/3] clk: qcom: gdsc: enable external switchable power domain
  2020-10-14  2:15   ` Stephen Boyd
@ 2020-10-14  9:44     ` Dmitry Baryshkov
  2020-10-14 17:29       ` Stephen Boyd
  0 siblings, 1 reply; 12+ messages in thread
From: Dmitry Baryshkov @ 2020-10-14  9:44 UTC (permalink / raw)
  To: Stephen Boyd, Andy Gross, Bjorn Andersson, Jonathan Marek,
	Michael Turquette, Rob Herring
  Cc: linux-arm-msm, Manivannan Sadhasivam, devicetree, linux-clk

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 3/3] clk: qcom: dispcc-sm8250: handle MMCX power domain
  2020-10-14  2:16   ` Stephen Boyd
@ 2020-10-14  9:46     ` Dmitry Baryshkov
  0 siblings, 0 replies; 12+ messages in thread
From: Dmitry Baryshkov @ 2020-10-14  9:46 UTC (permalink / raw)
  To: Stephen Boyd, Andy Gross, Bjorn Andersson, Jonathan Marek,
	Michael Turquette, Rob Herring
  Cc: linux-arm-msm, Manivannan Sadhasivam, devicetree, linux-clk

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

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH v1 2/3] clk: qcom: gdsc: enable external switchable power domain
  2020-10-14  9:44     ` Dmitry Baryshkov
@ 2020-10-14 17:29       ` Stephen Boyd
  0 siblings, 0 replies; 12+ messages in thread
From: Stephen Boyd @ 2020-10-14 17:29 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Dmitry Baryshkov, Jonathan Marek,
	Michael Turquette, Rob Herring
  Cc: linux-arm-msm, Manivannan Sadhasivam, devicetree, linux-clk

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.

^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2020-10-14 17:29 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-05 22:59 [PATCH v1 0/3] clk: qcom: handle power domains supplies for GDSC Dmitry Baryshkov
2020-10-05 22:59 ` [PATCH v1 1/3] dt-bindings: clock: qcom,dispcc: document power domain bindings Dmitry Baryshkov
2020-10-14  0:46   ` Stephen Boyd
2020-10-14  9:36     ` Dmitry Baryshkov
2020-10-14  2:17   ` Stephen Boyd
2020-10-05 22:59 ` [PATCH v1 2/3] clk: qcom: gdsc: enable external switchable power domain Dmitry Baryshkov
2020-10-14  2:15   ` Stephen Boyd
2020-10-14  9:44     ` Dmitry Baryshkov
2020-10-14 17:29       ` Stephen Boyd
2020-10-05 22:59 ` [PATCH v1 3/3] clk: qcom: dispcc-sm8250: handle MMCX " Dmitry Baryshkov
2020-10-14  2:16   ` Stephen Boyd
2020-10-14  9:46     ` Dmitry Baryshkov

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).