All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/6] clk: qcom: use power-domain for sm8250's clock controllers
@ 2021-06-30 13:31 Dmitry Baryshkov
  2021-06-30 13:31 ` [PATCH 1/6] dt-bindings: clock: qcom,dispcc-sm8x50: add mmcx power domain Dmitry Baryshkov
                   ` (5 more replies)
  0 siblings, 6 replies; 25+ messages in thread
From: Dmitry Baryshkov @ 2021-06-30 13:31 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring, Stephen Boyd,
	Taniya Das, Jonathan Marek, Michael Turquette
  Cc: linux-arm-msm, devicetree, linux-clk, Bryan O'Donoghue,
	Mark Brown, Ulf Hansson, linux-kernel

On SM8250 both the display and video clock controllers are powered up by
the MMCX power domain. Handle this link in GDSC code by using
pm_runtime_get/put to enable and disable the MMCX power domain.

----------------------------------------------------------------
Dmitry Baryshkov (6):
      dt-bindings: clock: qcom,dispcc-sm8x50: add mmcx power domain
      dt-bindings: clock: qcom,videocc: add mmcx power domain
      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         | 19 ++++++++
 .../devicetree/bindings/clock/qcom,videocc.yaml    | 19 ++++++++
 arch/arm64/boot/dts/qcom/sm8250.dtsi               | 13 ++---
 drivers/clk/qcom/common.c                          | 55 +++++++++++++++++++---
 drivers/clk/qcom/dispcc-sm8250.c                   |  1 -
 drivers/clk/qcom/gdsc.c                            |  6 +++
 drivers/clk/qcom/videocc-sm8250.c                  |  4 --
 7 files changed, 97 insertions(+), 20 deletions(-)



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

* [PATCH 1/6] dt-bindings: clock: qcom,dispcc-sm8x50: add mmcx power domain
  2021-06-30 13:31 [PATCH 0/6] clk: qcom: use power-domain for sm8250's clock controllers Dmitry Baryshkov
@ 2021-06-30 13:31 ` Dmitry Baryshkov
  2021-07-01 16:16   ` Ulf Hansson
  2021-06-30 13:31 ` [PATCH 2/6] dt-bindings: clock: qcom,videocc: " Dmitry Baryshkov
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 25+ messages in thread
From: Dmitry Baryshkov @ 2021-06-30 13:31 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring, Stephen Boyd,
	Taniya Das, Jonathan Marek, Michael Turquette
  Cc: linux-arm-msm, devicetree, linux-clk, Bryan O'Donoghue,
	Mark Brown, Ulf Hansson, linux-kernel

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>
---
 .../bindings/clock/qcom,dispcc-sm8x50.yaml    | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
index 0cdf53f41f84..48d86fb34fa7 100644
--- a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
@@ -55,6 +55,16 @@ properties:
   reg:
     maxItems: 1
 
+  power-domains:
+    description:
+      A phandle and PM domain specifier for the MMCX power domain.
+    maxItems: 1
+
+  required-opps:
+    description:
+      Performance state to use for MMCX to enable register access.
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -64,6 +74,15 @@ required:
   - '#reset-cells'
   - '#power-domain-cells'
 
+# Either both properties are present or both are absent
+dependencies:
+  power-domains:
+    required:
+      - required-opps
+  required-opps:
+    required:
+      - power-domains
+
 additionalProperties: false
 
 examples:
-- 
2.30.2


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

* [PATCH 2/6] dt-bindings: clock: qcom,videocc: add mmcx power domain
  2021-06-30 13:31 [PATCH 0/6] clk: qcom: use power-domain for sm8250's clock controllers Dmitry Baryshkov
  2021-06-30 13:31 ` [PATCH 1/6] dt-bindings: clock: qcom,dispcc-sm8x50: add mmcx power domain Dmitry Baryshkov
@ 2021-06-30 13:31 ` Dmitry Baryshkov
  2021-06-30 13:31 ` [PATCH 3/6] clk: qcom: gdsc: enable optional power domain support Dmitry Baryshkov
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 25+ messages in thread
From: Dmitry Baryshkov @ 2021-06-30 13:31 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring, Stephen Boyd,
	Taniya Das, Jonathan Marek, Michael Turquette
  Cc: linux-arm-msm, devicetree, linux-clk, Bryan O'Donoghue,
	Mark Brown, Ulf Hansson, linux-kernel

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>
---
 .../bindings/clock/qcom,videocc.yaml          | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/qcom,videocc.yaml b/Documentation/devicetree/bindings/clock/qcom,videocc.yaml
index 567202942b88..22421173e1ca 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:
+      Performance state to use for MMCX to enable register access.
+    maxItems: 1
+
 required:
   - compatible
   - reg
@@ -56,6 +66,15 @@ required:
   - '#reset-cells'
   - '#power-domain-cells'
 
+# Either both properties are present or both are absent
+dependencies:
+  power-domains:
+    required:
+      - required-opps
+  required-opps:
+    required:
+      - power-domains
+
 additionalProperties: false
 
 examples:
-- 
2.30.2


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

* [PATCH 3/6] clk: qcom: gdsc: enable optional power domain support
  2021-06-30 13:31 [PATCH 0/6] clk: qcom: use power-domain for sm8250's clock controllers Dmitry Baryshkov
  2021-06-30 13:31 ` [PATCH 1/6] dt-bindings: clock: qcom,dispcc-sm8x50: add mmcx power domain Dmitry Baryshkov
  2021-06-30 13:31 ` [PATCH 2/6] dt-bindings: clock: qcom,videocc: " Dmitry Baryshkov
@ 2021-06-30 13:31 ` Dmitry Baryshkov
  2021-06-30 15:00   ` Bjorn Andersson
  2021-06-30 13:31 ` [PATCH 4/6] arm64: dts: qcom: sm8250: remove mmcx regulator Dmitry Baryshkov
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 25+ messages in thread
From: Dmitry Baryshkov @ 2021-06-30 13:31 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring, Stephen Boyd,
	Taniya Das, Jonathan Marek, Michael Turquette
  Cc: linux-arm-msm, devicetree, linux-clk, Bryan O'Donoghue,
	Mark Brown, Ulf Hansson, linux-kernel

On sm8250 dispcc and videocc registers are powered up by the MMCX power
domain. Currently we used a regulator to enable this domain on demand,
however this has some consequences, as genpd code is not reentrant.

Teach Qualcomm clock controller code about setting up power domains and
using them for gdsc control.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/common.c | 55 ++++++++++++++++++++++++++++++++++-----
 drivers/clk/qcom/gdsc.c   |  6 +++++
 2 files changed, 55 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
index 60d2a78d1395..eeb5b8c93032 100644
--- a/drivers/clk/qcom/common.c
+++ b/drivers/clk/qcom/common.c
@@ -10,6 +10,8 @@
 #include <linux/clk-provider.h>
 #include <linux/reset-controller.h>
 #include <linux/of.h>
+#include <linux/pm_opp.h>
+#include <linux/pm_runtime.h>
 
 #include "common.h"
 #include "clk-rcg.h"
@@ -76,6 +78,16 @@ qcom_cc_map(struct platform_device *pdev, const struct qcom_cc_desc *desc)
 	struct resource *res;
 	struct device *dev = &pdev->dev;
 
+	if (of_find_property(dev->of_node, "required-opps", NULL)) {
+		int pd_opp;
+
+		pd_opp = of_get_required_opp_performance_state(dev->of_node, 0);
+		if (pd_opp < 0)
+			return ERR_PTR(pd_opp);
+
+		dev_pm_genpd_set_performance_state(dev, pd_opp);
+	}
+
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(base))
@@ -224,6 +236,11 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec,
 	return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
 }
 
+static void qcom_cc_pm_runtime_disable(void *data)
+{
+	pm_runtime_disable(data);
+}
+
 int qcom_cc_really_probe(struct platform_device *pdev,
 			 const struct qcom_cc_desc *desc, struct regmap *regmap)
 {
@@ -236,11 +253,28 @@ int qcom_cc_really_probe(struct platform_device *pdev,
 	struct clk_regmap **rclks = desc->clks;
 	size_t num_clk_hws = desc->num_clk_hws;
 	struct clk_hw **clk_hws = desc->clk_hws;
+	bool use_pm = false;
 
 	cc = devm_kzalloc(dev, sizeof(*cc), GFP_KERNEL);
 	if (!cc)
 		return -ENOMEM;
 
+	if (of_find_property(dev->of_node, "required-opps", NULL)) {
+		use_pm = true;
+
+		pm_runtime_enable(dev);
+		ret = pm_runtime_get_sync(dev);
+		if (ret < 0) {
+			pm_runtime_put(dev);
+			pm_runtime_disable(dev);
+			return ret;
+		}
+
+		ret = devm_add_action_or_reset(dev, qcom_cc_pm_runtime_disable, dev);
+		if (ret)
+			return ret;
+	}
+
 	reset = &cc->reset;
 	reset->rcdev.of_node = dev->of_node;
 	reset->rcdev.ops = &qcom_reset_ops;
@@ -251,7 +285,7 @@ int qcom_cc_really_probe(struct platform_device *pdev,
 
 	ret = devm_reset_controller_register(dev, &reset->rcdev);
 	if (ret)
-		return ret;
+		goto err;
 
 	if (desc->gdscs && desc->num_gdscs) {
 		scd = devm_kzalloc(dev, sizeof(*scd), GFP_KERNEL);
@@ -262,11 +296,11 @@ int qcom_cc_really_probe(struct platform_device *pdev,
 		scd->num = desc->num_gdscs;
 		ret = gdsc_register(scd, &reset->rcdev, regmap);
 		if (ret)
-			return ret;
+			goto err;
 		ret = devm_add_action_or_reset(dev, qcom_cc_gdsc_unregister,
 					       scd);
 		if (ret)
-			return ret;
+			goto err;
 	}
 
 	cc->rclks = rclks;
@@ -277,7 +311,7 @@ int qcom_cc_really_probe(struct platform_device *pdev,
 	for (i = 0; i < num_clk_hws; i++) {
 		ret = devm_clk_hw_register(dev, clk_hws[i]);
 		if (ret)
-			return ret;
+			goto err;
 	}
 
 	for (i = 0; i < num_clks; i++) {
@@ -286,14 +320,23 @@ int qcom_cc_really_probe(struct platform_device *pdev,
 
 		ret = devm_clk_register_regmap(dev, rclks[i]);
 		if (ret)
-			return ret;
+			goto err;
 	}
 
 	ret = devm_of_clk_add_hw_provider(dev, qcom_cc_clk_hw_get, cc);
 	if (ret)
-		return ret;
+		goto err;
+
+	if (use_pm)
+		pm_runtime_put(dev);
 
 	return 0;
+
+err:
+	if (use_pm)
+		pm_runtime_put(dev);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(qcom_cc_really_probe);
 
diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index 51ed640e527b..40c384bda4fc 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>
@@ -237,6 +238,8 @@ static int gdsc_enable(struct generic_pm_domain *domain)
 	struct gdsc *sc = domain_to_gdsc(domain);
 	int ret;
 
+	pm_runtime_get_sync(domain->dev.parent);
+
 	if (sc->pwrsts == PWRSTS_ON)
 		return gdsc_deassert_reset(sc);
 
@@ -326,6 +329,8 @@ static int gdsc_disable(struct generic_pm_domain *domain)
 	if (sc->flags & CLAMP_IO)
 		gdsc_assert_clamp_io(sc);
 
+	pm_runtime_put(domain->dev.parent);
+
 	return 0;
 }
 
@@ -427,6 +432,7 @@ int gdsc_register(struct gdsc_desc *desc,
 			continue;
 		scs[i]->regmap = regmap;
 		scs[i]->rcdev = rcdev;
+		scs[i]->pd.dev.parent = desc->dev;
 		ret = gdsc_init(scs[i]);
 		if (ret)
 			return ret;
-- 
2.30.2


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

* [PATCH 4/6] arm64: dts: qcom: sm8250: remove mmcx regulator
  2021-06-30 13:31 [PATCH 0/6] clk: qcom: use power-domain for sm8250's clock controllers Dmitry Baryshkov
                   ` (2 preceding siblings ...)
  2021-06-30 13:31 ` [PATCH 3/6] clk: qcom: gdsc: enable optional power domain support Dmitry Baryshkov
@ 2021-06-30 13:31 ` Dmitry Baryshkov
  2021-06-30 13:31 ` [PATCH 5/6] clk: qcom: dispcc-sm8250: stop using " Dmitry Baryshkov
  2021-06-30 13:31 ` [PATCH 6/6] clk: qcom: videocc-sm8250: " Dmitry Baryshkov
  5 siblings, 0 replies; 25+ messages in thread
From: Dmitry Baryshkov @ 2021-06-30 13:31 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring, Stephen Boyd,
	Taniya Das, Jonathan Marek, Michael Turquette
  Cc: linux-arm-msm, devicetree, linux-clk, Bryan O'Donoghue,
	Mark Brown, Ulf Hansson, linux-kernel

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

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@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 4c0de12aaba6..1c8478d1247d 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -271,13 +271,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>;
@@ -2362,7 +2355,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>;
@@ -2627,7 +2621,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.30.2


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

* [PATCH 5/6] clk: qcom: dispcc-sm8250: stop using mmcx regulator
  2021-06-30 13:31 [PATCH 0/6] clk: qcom: use power-domain for sm8250's clock controllers Dmitry Baryshkov
                   ` (3 preceding siblings ...)
  2021-06-30 13:31 ` [PATCH 4/6] arm64: dts: qcom: sm8250: remove mmcx regulator Dmitry Baryshkov
@ 2021-06-30 13:31 ` Dmitry Baryshkov
  2021-06-30 17:12   ` Bjorn Andersson
  2021-06-30 13:31 ` [PATCH 6/6] clk: qcom: videocc-sm8250: " Dmitry Baryshkov
  5 siblings, 1 reply; 25+ messages in thread
From: Dmitry Baryshkov @ 2021-06-30 13:31 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring, Stephen Boyd,
	Taniya Das, Jonathan Marek, Michael Turquette
  Cc: linux-arm-msm, devicetree, linux-clk, Bryan O'Donoghue,
	Mark Brown, Ulf Hansson, linux-kernel

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>
---
 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 de09cd5c209f..dfbfe64b12f6 100644
--- a/drivers/clk/qcom/dispcc-sm8250.c
+++ b/drivers/clk/qcom/dispcc-sm8250.c
@@ -955,7 +955,6 @@ static struct gdsc mdss_gdsc = {
 	},
 	.pwrsts = PWRSTS_OFF_ON,
 	.flags = HW_CTRL,
-	.supply = "mmcx",
 };
 
 static struct clk_regmap *disp_cc_sm8250_clocks[] = {
-- 
2.30.2


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

* [PATCH 6/6] clk: qcom: videocc-sm8250: stop using mmcx regulator
  2021-06-30 13:31 [PATCH 0/6] clk: qcom: use power-domain for sm8250's clock controllers Dmitry Baryshkov
                   ` (4 preceding siblings ...)
  2021-06-30 13:31 ` [PATCH 5/6] clk: qcom: dispcc-sm8250: stop using " Dmitry Baryshkov
@ 2021-06-30 13:31 ` Dmitry Baryshkov
  2021-06-30 17:13   ` Bjorn Andersson
  5 siblings, 1 reply; 25+ messages in thread
From: Dmitry Baryshkov @ 2021-06-30 13:31 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring, Stephen Boyd,
	Taniya Das, Jonathan Marek, Michael Turquette
  Cc: linux-arm-msm, devicetree, linux-clk, Bryan O'Donoghue,
	Mark Brown, Ulf Hansson, linux-kernel

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>
---
 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 7b435a1c2c4b..eedef85d90e5 100644
--- a/drivers/clk/qcom/videocc-sm8250.c
+++ b/drivers/clk/qcom/videocc-sm8250.c
@@ -276,7 +276,6 @@ static struct gdsc mvs0c_gdsc = {
 	},
 	.flags = 0,
 	.pwrsts = PWRSTS_OFF_ON,
-	.supply = "mmcx",
 };
 
 static struct gdsc mvs1c_gdsc = {
@@ -286,7 +285,6 @@ static struct gdsc mvs1c_gdsc = {
 	},
 	.flags = 0,
 	.pwrsts = PWRSTS_OFF_ON,
-	.supply = "mmcx",
 };
 
 static struct gdsc mvs0_gdsc = {
@@ -296,7 +294,6 @@ static struct gdsc mvs0_gdsc = {
 	},
 	.flags = HW_CTRL,
 	.pwrsts = PWRSTS_OFF_ON,
-	.supply = "mmcx",
 };
 
 static struct gdsc mvs1_gdsc = {
@@ -306,7 +303,6 @@ static struct gdsc mvs1_gdsc = {
 	},
 	.flags = HW_CTRL,
 	.pwrsts = PWRSTS_OFF_ON,
-	.supply = "mmcx",
 };
 
 static struct clk_regmap *video_cc_sm8250_clocks[] = {
-- 
2.30.2


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

* Re: [PATCH 3/6] clk: qcom: gdsc: enable optional power domain support
  2021-06-30 13:31 ` [PATCH 3/6] clk: qcom: gdsc: enable optional power domain support Dmitry Baryshkov
@ 2021-06-30 15:00   ` Bjorn Andersson
  2021-06-30 15:47     ` Dmitry Baryshkov
  0 siblings, 1 reply; 25+ messages in thread
From: Bjorn Andersson @ 2021-06-30 15:00 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Andy Gross, Rob Herring, Stephen Boyd, Taniya Das,
	Jonathan Marek, Michael Turquette, linux-arm-msm, devicetree,
	linux-clk, Bryan O'Donoghue, Mark Brown, Ulf Hansson,
	linux-kernel

On Wed 30 Jun 08:31 CDT 2021, Dmitry Baryshkov wrote:

> On sm8250 dispcc and videocc registers are powered up by the MMCX power
> domain. Currently we used a regulator to enable this domain on demand,
> however this has some consequences, as genpd code is not reentrant.
> 
> Teach Qualcomm clock controller code about setting up power domains and
> using them for gdsc control.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

There's a proposal to add a generic binding for statically assigning a
performance states here:

https://lore.kernel.org/linux-arm-msm/1622095949-2014-1-git-send-email-rnayak@codeaurora.org/


But that said, do you really need this?

The requirement for driving MMCX to LOW_SVS on SM8250 (and NOM on
SM8150/SC8180x) seems to only come from the fact that you push MDP_CLK
to 460MHz in &mdss.

But then in &mdss_mdp you do the same using an opp-table based on the
actual MDP_CLK, which per its power-domains will scale MMCX accordingly.


So wouldn't it be sufficient to ensure that MDSS_GDSC is parented by
MMCX and then use opp-tables associated with the devices that scales the
clock and thereby actually carries the "required-opps".


I presume your testing indicates that it doesn't matter on sm8250, but
as stated above, 460MHz on sm8150/sc8180x requires nominal, so per your
suggestion we'd have to vote nominal in &mdss, which means that if the
DPU decides to go to 200MHz the &mdss will still keep the voltage at
NOM, even though the DPU's opp-table says that LOW_SVS is sufficient.

Regards,
Bjorn

> ---
>  drivers/clk/qcom/common.c | 55 ++++++++++++++++++++++++++++++++++-----
>  drivers/clk/qcom/gdsc.c   |  6 +++++
>  2 files changed, 55 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> index 60d2a78d1395..eeb5b8c93032 100644
> --- a/drivers/clk/qcom/common.c
> +++ b/drivers/clk/qcom/common.c
> @@ -10,6 +10,8 @@
>  #include <linux/clk-provider.h>
>  #include <linux/reset-controller.h>
>  #include <linux/of.h>
> +#include <linux/pm_opp.h>
> +#include <linux/pm_runtime.h>
>  
>  #include "common.h"
>  #include "clk-rcg.h"
> @@ -76,6 +78,16 @@ qcom_cc_map(struct platform_device *pdev, const struct qcom_cc_desc *desc)
>  	struct resource *res;
>  	struct device *dev = &pdev->dev;
>  
> +	if (of_find_property(dev->of_node, "required-opps", NULL)) {
> +		int pd_opp;
> +
> +		pd_opp = of_get_required_opp_performance_state(dev->of_node, 0);
> +		if (pd_opp < 0)
> +			return ERR_PTR(pd_opp);
> +
> +		dev_pm_genpd_set_performance_state(dev, pd_opp);
> +	}
> +
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	base = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(base))
> @@ -224,6 +236,11 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec,
>  	return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
>  }
>  
> +static void qcom_cc_pm_runtime_disable(void *data)
> +{
> +	pm_runtime_disable(data);
> +}
> +
>  int qcom_cc_really_probe(struct platform_device *pdev,
>  			 const struct qcom_cc_desc *desc, struct regmap *regmap)
>  {
> @@ -236,11 +253,28 @@ int qcom_cc_really_probe(struct platform_device *pdev,
>  	struct clk_regmap **rclks = desc->clks;
>  	size_t num_clk_hws = desc->num_clk_hws;
>  	struct clk_hw **clk_hws = desc->clk_hws;
> +	bool use_pm = false;
>  
>  	cc = devm_kzalloc(dev, sizeof(*cc), GFP_KERNEL);
>  	if (!cc)
>  		return -ENOMEM;
>  
> +	if (of_find_property(dev->of_node, "required-opps", NULL)) {
> +		use_pm = true;
> +
> +		pm_runtime_enable(dev);
> +		ret = pm_runtime_get_sync(dev);
> +		if (ret < 0) {
> +			pm_runtime_put(dev);
> +			pm_runtime_disable(dev);
> +			return ret;
> +		}
> +
> +		ret = devm_add_action_or_reset(dev, qcom_cc_pm_runtime_disable, dev);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	reset = &cc->reset;
>  	reset->rcdev.of_node = dev->of_node;
>  	reset->rcdev.ops = &qcom_reset_ops;
> @@ -251,7 +285,7 @@ int qcom_cc_really_probe(struct platform_device *pdev,
>  
>  	ret = devm_reset_controller_register(dev, &reset->rcdev);
>  	if (ret)
> -		return ret;
> +		goto err;
>  
>  	if (desc->gdscs && desc->num_gdscs) {
>  		scd = devm_kzalloc(dev, sizeof(*scd), GFP_KERNEL);
> @@ -262,11 +296,11 @@ int qcom_cc_really_probe(struct platform_device *pdev,
>  		scd->num = desc->num_gdscs;
>  		ret = gdsc_register(scd, &reset->rcdev, regmap);
>  		if (ret)
> -			return ret;
> +			goto err;
>  		ret = devm_add_action_or_reset(dev, qcom_cc_gdsc_unregister,
>  					       scd);
>  		if (ret)
> -			return ret;
> +			goto err;
>  	}
>  
>  	cc->rclks = rclks;
> @@ -277,7 +311,7 @@ int qcom_cc_really_probe(struct platform_device *pdev,
>  	for (i = 0; i < num_clk_hws; i++) {
>  		ret = devm_clk_hw_register(dev, clk_hws[i]);
>  		if (ret)
> -			return ret;
> +			goto err;
>  	}
>  
>  	for (i = 0; i < num_clks; i++) {
> @@ -286,14 +320,23 @@ int qcom_cc_really_probe(struct platform_device *pdev,
>  
>  		ret = devm_clk_register_regmap(dev, rclks[i]);
>  		if (ret)
> -			return ret;
> +			goto err;
>  	}
>  
>  	ret = devm_of_clk_add_hw_provider(dev, qcom_cc_clk_hw_get, cc);
>  	if (ret)
> -		return ret;
> +		goto err;
> +
> +	if (use_pm)
> +		pm_runtime_put(dev);
>  
>  	return 0;
> +
> +err:
> +	if (use_pm)
> +		pm_runtime_put(dev);
> +
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(qcom_cc_really_probe);
>  
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index 51ed640e527b..40c384bda4fc 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>
> @@ -237,6 +238,8 @@ static int gdsc_enable(struct generic_pm_domain *domain)
>  	struct gdsc *sc = domain_to_gdsc(domain);
>  	int ret;
>  
> +	pm_runtime_get_sync(domain->dev.parent);
> +
>  	if (sc->pwrsts == PWRSTS_ON)
>  		return gdsc_deassert_reset(sc);
>  
> @@ -326,6 +329,8 @@ static int gdsc_disable(struct generic_pm_domain *domain)
>  	if (sc->flags & CLAMP_IO)
>  		gdsc_assert_clamp_io(sc);
>  
> +	pm_runtime_put(domain->dev.parent);
> +
>  	return 0;
>  }
>  
> @@ -427,6 +432,7 @@ int gdsc_register(struct gdsc_desc *desc,
>  			continue;
>  		scs[i]->regmap = regmap;
>  		scs[i]->rcdev = rcdev;
> +		scs[i]->pd.dev.parent = desc->dev;
>  		ret = gdsc_init(scs[i]);
>  		if (ret)
>  			return ret;
> -- 
> 2.30.2
> 

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

* Re: [PATCH 3/6] clk: qcom: gdsc: enable optional power domain support
  2021-06-30 15:00   ` Bjorn Andersson
@ 2021-06-30 15:47     ` Dmitry Baryshkov
  2021-06-30 17:11       ` Bjorn Andersson
  0 siblings, 1 reply; 25+ messages in thread
From: Dmitry Baryshkov @ 2021-06-30 15:47 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Andy Gross, Rob Herring, Stephen Boyd, Taniya Das,
	Jonathan Marek, Michael Turquette,
	open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:COMMON CLK FRAMEWORK, Bryan O'Donoghue, Mark Brown,
	Ulf Hansson, open list

Hi,

On Wed, 30 Jun 2021 at 18:00, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Wed 30 Jun 08:31 CDT 2021, Dmitry Baryshkov wrote:
>
> > On sm8250 dispcc and videocc registers are powered up by the MMCX power
> > domain. Currently we used a regulator to enable this domain on demand,
> > however this has some consequences, as genpd code is not reentrant.
> >
> > Teach Qualcomm clock controller code about setting up power domains and
> > using them for gdsc control.
> >
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
> There's a proposal to add a generic binding for statically assigning a
> performance states here:
>
> https://lore.kernel.org/linux-arm-msm/1622095949-2014-1-git-send-email-rnayak@codeaurora.org/
>
>
> But that said, do you really need this?
>
> The requirement for driving MMCX to LOW_SVS on SM8250 (and NOM on
> SM8150/SC8180x) seems to only come from the fact that you push MDP_CLK
> to 460MHz in &mdss.
>
> But then in &mdss_mdp you do the same using an opp-table based on the
> actual MDP_CLK, which per its power-domains will scale MMCX accordingly.

MDSS and DSI would bump up MMCX performance state requirements on
their own, depending on the frequency being selected.

> So wouldn't it be sufficient to ensure that MDSS_GDSC is parented by
> MMCX and then use opp-tables associated with the devices that scales the
> clock and thereby actually carries the "required-opps".

Actually no. I set the performance state in the qcom_cc_map, so that
further register access is possible. Initially I was doing this in the
qcom_cc_really_probe() and it was already too late.
Just to remind: this patchset is not about MDSS_GDSC being parented by
MMCX, it is about dispcc/videocc registers being gated with MMCX.

> I presume your testing indicates that it doesn't matter on sm8250, but
> as stated above, 460MHz on sm8150/sc8180x requires nominal, so per your
> suggestion we'd have to vote nominal in &mdss, which means that if the
> DPU decides to go to 200MHz the &mdss will still keep the voltage at
> NOM, even though the DPU's opp-table says that LOW_SVS is sufficient.

Let me check whether LOW_SVS is really a requirement or if setting
MIN_SVS would also be sufficient for that. Interesting enough, from
the downstream drivers it looks like dispcc should be able to work
with MIN_SVS, while videocc would require LOW_SVS.

>
> Regards,
> Bjorn
>
> > ---
> >  drivers/clk/qcom/common.c | 55 ++++++++++++++++++++++++++++++++++-----
> >  drivers/clk/qcom/gdsc.c   |  6 +++++
> >  2 files changed, 55 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> > index 60d2a78d1395..eeb5b8c93032 100644
> > --- a/drivers/clk/qcom/common.c
> > +++ b/drivers/clk/qcom/common.c
> > @@ -10,6 +10,8 @@
> >  #include <linux/clk-provider.h>
> >  #include <linux/reset-controller.h>
> >  #include <linux/of.h>
> > +#include <linux/pm_opp.h>
> > +#include <linux/pm_runtime.h>
> >
> >  #include "common.h"
> >  #include "clk-rcg.h"
> > @@ -76,6 +78,16 @@ qcom_cc_map(struct platform_device *pdev, const struct qcom_cc_desc *desc)
> >       struct resource *res;
> >       struct device *dev = &pdev->dev;
> >
> > +     if (of_find_property(dev->of_node, "required-opps", NULL)) {
> > +             int pd_opp;
> > +
> > +             pd_opp = of_get_required_opp_performance_state(dev->of_node, 0);
> > +             if (pd_opp < 0)
> > +                     return ERR_PTR(pd_opp);
> > +
> > +             dev_pm_genpd_set_performance_state(dev, pd_opp);
> > +     }
> > +
> >       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >       base = devm_ioremap_resource(dev, res);
> >       if (IS_ERR(base))
> > @@ -224,6 +236,11 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec,
> >       return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
> >  }
> >
> > +static void qcom_cc_pm_runtime_disable(void *data)
> > +{
> > +     pm_runtime_disable(data);
> > +}
> > +
> >  int qcom_cc_really_probe(struct platform_device *pdev,
> >                        const struct qcom_cc_desc *desc, struct regmap *regmap)
> >  {
> > @@ -236,11 +253,28 @@ int qcom_cc_really_probe(struct platform_device *pdev,
> >       struct clk_regmap **rclks = desc->clks;
> >       size_t num_clk_hws = desc->num_clk_hws;
> >       struct clk_hw **clk_hws = desc->clk_hws;
> > +     bool use_pm = false;
> >
> >       cc = devm_kzalloc(dev, sizeof(*cc), GFP_KERNEL);
> >       if (!cc)
> >               return -ENOMEM;
> >
> > +     if (of_find_property(dev->of_node, "required-opps", NULL)) {
> > +             use_pm = true;
> > +
> > +             pm_runtime_enable(dev);
> > +             ret = pm_runtime_get_sync(dev);
> > +             if (ret < 0) {
> > +                     pm_runtime_put(dev);
> > +                     pm_runtime_disable(dev);
> > +                     return ret;
> > +             }
> > +
> > +             ret = devm_add_action_or_reset(dev, qcom_cc_pm_runtime_disable, dev);
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> >       reset = &cc->reset;
> >       reset->rcdev.of_node = dev->of_node;
> >       reset->rcdev.ops = &qcom_reset_ops;
> > @@ -251,7 +285,7 @@ int qcom_cc_really_probe(struct platform_device *pdev,
> >
> >       ret = devm_reset_controller_register(dev, &reset->rcdev);
> >       if (ret)
> > -             return ret;
> > +             goto err;
> >
> >       if (desc->gdscs && desc->num_gdscs) {
> >               scd = devm_kzalloc(dev, sizeof(*scd), GFP_KERNEL);
> > @@ -262,11 +296,11 @@ int qcom_cc_really_probe(struct platform_device *pdev,
> >               scd->num = desc->num_gdscs;
> >               ret = gdsc_register(scd, &reset->rcdev, regmap);
> >               if (ret)
> > -                     return ret;
> > +                     goto err;
> >               ret = devm_add_action_or_reset(dev, qcom_cc_gdsc_unregister,
> >                                              scd);
> >               if (ret)
> > -                     return ret;
> > +                     goto err;
> >       }
> >
> >       cc->rclks = rclks;
> > @@ -277,7 +311,7 @@ int qcom_cc_really_probe(struct platform_device *pdev,
> >       for (i = 0; i < num_clk_hws; i++) {
> >               ret = devm_clk_hw_register(dev, clk_hws[i]);
> >               if (ret)
> > -                     return ret;
> > +                     goto err;
> >       }
> >
> >       for (i = 0; i < num_clks; i++) {
> > @@ -286,14 +320,23 @@ int qcom_cc_really_probe(struct platform_device *pdev,
> >
> >               ret = devm_clk_register_regmap(dev, rclks[i]);
> >               if (ret)
> > -                     return ret;
> > +                     goto err;
> >       }
> >
> >       ret = devm_of_clk_add_hw_provider(dev, qcom_cc_clk_hw_get, cc);
> >       if (ret)
> > -             return ret;
> > +             goto err;
> > +
> > +     if (use_pm)
> > +             pm_runtime_put(dev);
> >
> >       return 0;
> > +
> > +err:
> > +     if (use_pm)
> > +             pm_runtime_put(dev);
> > +
> > +     return ret;
> >  }
> >  EXPORT_SYMBOL_GPL(qcom_cc_really_probe);
> >
> > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > index 51ed640e527b..40c384bda4fc 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>
> > @@ -237,6 +238,8 @@ static int gdsc_enable(struct generic_pm_domain *domain)
> >       struct gdsc *sc = domain_to_gdsc(domain);
> >       int ret;
> >
> > +     pm_runtime_get_sync(domain->dev.parent);
> > +
> >       if (sc->pwrsts == PWRSTS_ON)
> >               return gdsc_deassert_reset(sc);
> >
> > @@ -326,6 +329,8 @@ static int gdsc_disable(struct generic_pm_domain *domain)
> >       if (sc->flags & CLAMP_IO)
> >               gdsc_assert_clamp_io(sc);
> >
> > +     pm_runtime_put(domain->dev.parent);
> > +
> >       return 0;
> >  }
> >
> > @@ -427,6 +432,7 @@ int gdsc_register(struct gdsc_desc *desc,
> >                       continue;
> >               scs[i]->regmap = regmap;
> >               scs[i]->rcdev = rcdev;
> > +             scs[i]->pd.dev.parent = desc->dev;
> >               ret = gdsc_init(scs[i]);
> >               if (ret)
> >                       return ret;
> > --
> > 2.30.2
> >



-- 
With best wishes
Dmitry

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

* Re: [PATCH 3/6] clk: qcom: gdsc: enable optional power domain support
  2021-06-30 15:47     ` Dmitry Baryshkov
@ 2021-06-30 17:11       ` Bjorn Andersson
  2021-06-30 20:29         ` Dmitry Baryshkov
  0 siblings, 1 reply; 25+ messages in thread
From: Bjorn Andersson @ 2021-06-30 17:11 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Andy Gross, Rob Herring, Stephen Boyd, Taniya Das,
	Jonathan Marek, Michael Turquette,
	open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:COMMON CLK FRAMEWORK, Bryan O'Donoghue, Mark Brown,
	Ulf Hansson, open list

On Wed 30 Jun 10:47 CDT 2021, Dmitry Baryshkov wrote:

> Hi,
> 
> On Wed, 30 Jun 2021 at 18:00, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Wed 30 Jun 08:31 CDT 2021, Dmitry Baryshkov wrote:
> >
> > > On sm8250 dispcc and videocc registers are powered up by the MMCX power
> > > domain. Currently we used a regulator to enable this domain on demand,
> > > however this has some consequences, as genpd code is not reentrant.
> > >
> > > Teach Qualcomm clock controller code about setting up power domains and
> > > using them for gdsc control.
> > >
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> >
> > There's a proposal to add a generic binding for statically assigning a
> > performance states here:
> >
> > https://lore.kernel.org/linux-arm-msm/1622095949-2014-1-git-send-email-rnayak@codeaurora.org/
> >
> >
> > But that said, do you really need this?
> >
> > The requirement for driving MMCX to LOW_SVS on SM8250 (and NOM on
> > SM8150/SC8180x) seems to only come from the fact that you push MDP_CLK
> > to 460MHz in &mdss.
> >
> > But then in &mdss_mdp you do the same using an opp-table based on the
> > actual MDP_CLK, which per its power-domains will scale MMCX accordingly.
> 
> MDSS and DSI would bump up MMCX performance state requirements on
> their own, depending on the frequency being selected.
> 

Right, but as I copied things from the sm8250.dtsi to come up with
sm8150/sc8180x.dtsi I concluded that as soon as the assigned-clockrate
in &mdss kicks in I need the performance state to be at NOM.

So keeping the assigned-clockrate in &mdss means that MMCX will never go
below NOM.

> > So wouldn't it be sufficient to ensure that MDSS_GDSC is parented by
> > MMCX and then use opp-tables associated with the devices that scales the
> > clock and thereby actually carries the "required-opps".
> 
> Actually no. I set the performance state in the qcom_cc_map, so that
> further register access is possible. Initially I was doing this in the
> qcom_cc_really_probe() and it was already too late.
> Just to remind: this patchset is not about MDSS_GDSC being parented by
> MMCX, it is about dispcc/videocc registers being gated with MMCX.
> 

So you're saying that just enabling MMCX isn't enough to touch the
dispcc/videocc registers? If that's the case it seems like MMCX's
definition of "on" needs to be adjusted - because just specifying MMCX
as the power-domain for dispcc/videocc and enabling pm_runtime should
ensure that MMCX is enabled when the clock registers are accessed (I
don't see anything like that for the GDSC part though).

I thought our problem you had was that you need to set a
performance_state in order to clock up some of the clocks - e.g.
MDP_CLK.

> > I presume your testing indicates that it doesn't matter on sm8250, but
> > as stated above, 460MHz on sm8150/sc8180x requires nominal, so per your
> > suggestion we'd have to vote nominal in &mdss, which means that if the
> > DPU decides to go to 200MHz the &mdss will still keep the voltage at
> > NOM, even though the DPU's opp-table says that LOW_SVS is sufficient.
> 
> Let me check whether LOW_SVS is really a requirement or if setting
> MIN_SVS would also be sufficient for that. Interesting enough, from
> the downstream drivers it looks like dispcc should be able to work
> with MIN_SVS, while videocc would require LOW_SVS.
> 

LOW_SVS is the documented requirement for ticking MDP_CLK at 460MHz on
SM8250. But I would expect we don't need LOW_SVS in order to poke the
registers in dispcc/videocc.

Regards,
Bjorn

> >
> > Regards,
> > Bjorn
> >
> > > ---
> > >  drivers/clk/qcom/common.c | 55 ++++++++++++++++++++++++++++++++++-----
> > >  drivers/clk/qcom/gdsc.c   |  6 +++++
> > >  2 files changed, 55 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> > > index 60d2a78d1395..eeb5b8c93032 100644
> > > --- a/drivers/clk/qcom/common.c
> > > +++ b/drivers/clk/qcom/common.c
> > > @@ -10,6 +10,8 @@
> > >  #include <linux/clk-provider.h>
> > >  #include <linux/reset-controller.h>
> > >  #include <linux/of.h>
> > > +#include <linux/pm_opp.h>
> > > +#include <linux/pm_runtime.h>
> > >
> > >  #include "common.h"
> > >  #include "clk-rcg.h"
> > > @@ -76,6 +78,16 @@ qcom_cc_map(struct platform_device *pdev, const struct qcom_cc_desc *desc)
> > >       struct resource *res;
> > >       struct device *dev = &pdev->dev;
> > >
> > > +     if (of_find_property(dev->of_node, "required-opps", NULL)) {
> > > +             int pd_opp;
> > > +
> > > +             pd_opp = of_get_required_opp_performance_state(dev->of_node, 0);
> > > +             if (pd_opp < 0)
> > > +                     return ERR_PTR(pd_opp);
> > > +
> > > +             dev_pm_genpd_set_performance_state(dev, pd_opp);
> > > +     }
> > > +
> > >       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > >       base = devm_ioremap_resource(dev, res);
> > >       if (IS_ERR(base))
> > > @@ -224,6 +236,11 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec,
> > >       return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
> > >  }
> > >
> > > +static void qcom_cc_pm_runtime_disable(void *data)
> > > +{
> > > +     pm_runtime_disable(data);
> > > +}
> > > +
> > >  int qcom_cc_really_probe(struct platform_device *pdev,
> > >                        const struct qcom_cc_desc *desc, struct regmap *regmap)
> > >  {
> > > @@ -236,11 +253,28 @@ int qcom_cc_really_probe(struct platform_device *pdev,
> > >       struct clk_regmap **rclks = desc->clks;
> > >       size_t num_clk_hws = desc->num_clk_hws;
> > >       struct clk_hw **clk_hws = desc->clk_hws;
> > > +     bool use_pm = false;
> > >
> > >       cc = devm_kzalloc(dev, sizeof(*cc), GFP_KERNEL);
> > >       if (!cc)
> > >               return -ENOMEM;
> > >
> > > +     if (of_find_property(dev->of_node, "required-opps", NULL)) {
> > > +             use_pm = true;
> > > +
> > > +             pm_runtime_enable(dev);
> > > +             ret = pm_runtime_get_sync(dev);
> > > +             if (ret < 0) {
> > > +                     pm_runtime_put(dev);
> > > +                     pm_runtime_disable(dev);
> > > +                     return ret;
> > > +             }
> > > +
> > > +             ret = devm_add_action_or_reset(dev, qcom_cc_pm_runtime_disable, dev);
> > > +             if (ret)
> > > +                     return ret;
> > > +     }
> > > +
> > >       reset = &cc->reset;
> > >       reset->rcdev.of_node = dev->of_node;
> > >       reset->rcdev.ops = &qcom_reset_ops;
> > > @@ -251,7 +285,7 @@ int qcom_cc_really_probe(struct platform_device *pdev,
> > >
> > >       ret = devm_reset_controller_register(dev, &reset->rcdev);
> > >       if (ret)
> > > -             return ret;
> > > +             goto err;
> > >
> > >       if (desc->gdscs && desc->num_gdscs) {
> > >               scd = devm_kzalloc(dev, sizeof(*scd), GFP_KERNEL);
> > > @@ -262,11 +296,11 @@ int qcom_cc_really_probe(struct platform_device *pdev,
> > >               scd->num = desc->num_gdscs;
> > >               ret = gdsc_register(scd, &reset->rcdev, regmap);
> > >               if (ret)
> > > -                     return ret;
> > > +                     goto err;
> > >               ret = devm_add_action_or_reset(dev, qcom_cc_gdsc_unregister,
> > >                                              scd);
> > >               if (ret)
> > > -                     return ret;
> > > +                     goto err;
> > >       }
> > >
> > >       cc->rclks = rclks;
> > > @@ -277,7 +311,7 @@ int qcom_cc_really_probe(struct platform_device *pdev,
> > >       for (i = 0; i < num_clk_hws; i++) {
> > >               ret = devm_clk_hw_register(dev, clk_hws[i]);
> > >               if (ret)
> > > -                     return ret;
> > > +                     goto err;
> > >       }
> > >
> > >       for (i = 0; i < num_clks; i++) {
> > > @@ -286,14 +320,23 @@ int qcom_cc_really_probe(struct platform_device *pdev,
> > >
> > >               ret = devm_clk_register_regmap(dev, rclks[i]);
> > >               if (ret)
> > > -                     return ret;
> > > +                     goto err;
> > >       }
> > >
> > >       ret = devm_of_clk_add_hw_provider(dev, qcom_cc_clk_hw_get, cc);
> > >       if (ret)
> > > -             return ret;
> > > +             goto err;
> > > +
> > > +     if (use_pm)
> > > +             pm_runtime_put(dev);
> > >
> > >       return 0;
> > > +
> > > +err:
> > > +     if (use_pm)
> > > +             pm_runtime_put(dev);
> > > +
> > > +     return ret;
> > >  }
> > >  EXPORT_SYMBOL_GPL(qcom_cc_really_probe);
> > >
> > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > > index 51ed640e527b..40c384bda4fc 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>
> > > @@ -237,6 +238,8 @@ static int gdsc_enable(struct generic_pm_domain *domain)
> > >       struct gdsc *sc = domain_to_gdsc(domain);
> > >       int ret;
> > >
> > > +     pm_runtime_get_sync(domain->dev.parent);
> > > +
> > >       if (sc->pwrsts == PWRSTS_ON)
> > >               return gdsc_deassert_reset(sc);
> > >
> > > @@ -326,6 +329,8 @@ static int gdsc_disable(struct generic_pm_domain *domain)
> > >       if (sc->flags & CLAMP_IO)
> > >               gdsc_assert_clamp_io(sc);
> > >
> > > +     pm_runtime_put(domain->dev.parent);
> > > +
> > >       return 0;
> > >  }
> > >
> > > @@ -427,6 +432,7 @@ int gdsc_register(struct gdsc_desc *desc,
> > >                       continue;
> > >               scs[i]->regmap = regmap;
> > >               scs[i]->rcdev = rcdev;
> > > +             scs[i]->pd.dev.parent = desc->dev;
> > >               ret = gdsc_init(scs[i]);
> > >               if (ret)
> > >                       return ret;
> > > --
> > > 2.30.2
> > >
> 
> 
> 
> -- 
> With best wishes
> Dmitry

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

* Re: [PATCH 5/6] clk: qcom: dispcc-sm8250: stop using mmcx regulator
  2021-06-30 13:31 ` [PATCH 5/6] clk: qcom: dispcc-sm8250: stop using " Dmitry Baryshkov
@ 2021-06-30 17:12   ` Bjorn Andersson
  0 siblings, 0 replies; 25+ messages in thread
From: Bjorn Andersson @ 2021-06-30 17:12 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Andy Gross, Rob Herring, Stephen Boyd, Taniya Das,
	Jonathan Marek, Michael Turquette, linux-arm-msm, devicetree,
	linux-clk, Bryan O'Donoghue, Mark Brown, Ulf Hansson,
	linux-kernel

On Wed 30 Jun 08:31 CDT 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>

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 de09cd5c209f..dfbfe64b12f6 100644
> --- a/drivers/clk/qcom/dispcc-sm8250.c
> +++ b/drivers/clk/qcom/dispcc-sm8250.c
> @@ -955,7 +955,6 @@ static struct gdsc mdss_gdsc = {
>  	},
>  	.pwrsts = PWRSTS_OFF_ON,
>  	.flags = HW_CTRL,
> -	.supply = "mmcx",
>  };
>  
>  static struct clk_regmap *disp_cc_sm8250_clocks[] = {
> -- 
> 2.30.2
> 

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

* Re: [PATCH 6/6] clk: qcom: videocc-sm8250: stop using mmcx regulator
  2021-06-30 13:31 ` [PATCH 6/6] clk: qcom: videocc-sm8250: " Dmitry Baryshkov
@ 2021-06-30 17:13   ` Bjorn Andersson
  0 siblings, 0 replies; 25+ messages in thread
From: Bjorn Andersson @ 2021-06-30 17:13 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Andy Gross, Rob Herring, Stephen Boyd, Taniya Das,
	Jonathan Marek, Michael Turquette, linux-arm-msm, devicetree,
	linux-clk, Bryan O'Donoghue, Mark Brown, Ulf Hansson,
	linux-kernel

On Wed 30 Jun 08:31 CDT 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 gdscs.
> 
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  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 7b435a1c2c4b..eedef85d90e5 100644
> --- a/drivers/clk/qcom/videocc-sm8250.c
> +++ b/drivers/clk/qcom/videocc-sm8250.c
> @@ -276,7 +276,6 @@ static struct gdsc mvs0c_gdsc = {
>  	},
>  	.flags = 0,
>  	.pwrsts = PWRSTS_OFF_ON,
> -	.supply = "mmcx",
>  };
>  
>  static struct gdsc mvs1c_gdsc = {
> @@ -286,7 +285,6 @@ static struct gdsc mvs1c_gdsc = {
>  	},
>  	.flags = 0,
>  	.pwrsts = PWRSTS_OFF_ON,
> -	.supply = "mmcx",
>  };
>  
>  static struct gdsc mvs0_gdsc = {
> @@ -296,7 +294,6 @@ static struct gdsc mvs0_gdsc = {
>  	},
>  	.flags = HW_CTRL,
>  	.pwrsts = PWRSTS_OFF_ON,
> -	.supply = "mmcx",
>  };
>  
>  static struct gdsc mvs1_gdsc = {
> @@ -306,7 +303,6 @@ static struct gdsc mvs1_gdsc = {
>  	},
>  	.flags = HW_CTRL,
>  	.pwrsts = PWRSTS_OFF_ON,
> -	.supply = "mmcx",
>  };
>  
>  static struct clk_regmap *video_cc_sm8250_clocks[] = {
> -- 
> 2.30.2
> 

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

* Re: [PATCH 3/6] clk: qcom: gdsc: enable optional power domain support
  2021-06-30 17:11       ` Bjorn Andersson
@ 2021-06-30 20:29         ` Dmitry Baryshkov
  2021-07-01  4:22           ` Bjorn Andersson
  0 siblings, 1 reply; 25+ messages in thread
From: Dmitry Baryshkov @ 2021-06-30 20:29 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Andy Gross, Rob Herring, Stephen Boyd, Taniya Das,
	Jonathan Marek, Michael Turquette,
	open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:COMMON CLK FRAMEWORK, Bryan O'Donoghue, Mark Brown,
	Ulf Hansson, open list

On Wed, 30 Jun 2021 at 20:11, Bjorn Andersson
<bjorn.andersson@linaro.org> wrote:
>
> On Wed 30 Jun 10:47 CDT 2021, Dmitry Baryshkov wrote:
>
> > Hi,
> >
> > On Wed, 30 Jun 2021 at 18:00, Bjorn Andersson
> > <bjorn.andersson@linaro.org> wrote:
> > >
> > > On Wed 30 Jun 08:31 CDT 2021, Dmitry Baryshkov wrote:
> > >
> > > > On sm8250 dispcc and videocc registers are powered up by the MMCX power
> > > > domain. Currently we used a regulator to enable this domain on demand,
> > > > however this has some consequences, as genpd code is not reentrant.
> > > >
> > > > Teach Qualcomm clock controller code about setting up power domains and
> > > > using them for gdsc control.
> > > >
> > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > >
> > > There's a proposal to add a generic binding for statically assigning a
> > > performance states here:
> > >
> > > https://lore.kernel.org/linux-arm-msm/1622095949-2014-1-git-send-email-rnayak@codeaurora.org/

I checked this thread. It looks like Rajendra will also switch to the
"required-opps" property. So if that series goes in first, we can drop
the call to set_performance_state. If this one goes in first, we can
drop the set_performance_state call after getting Rajendra's work in.

> > >
> > >
> > > But that said, do you really need this?
> > >
> > > The requirement for driving MMCX to LOW_SVS on SM8250 (and NOM on
> > > SM8150/SC8180x) seems to only come from the fact that you push MDP_CLK
> > > to 460MHz in &mdss.
> > >
> > > But then in &mdss_mdp you do the same using an opp-table based on the
> > > actual MDP_CLK, which per its power-domains will scale MMCX accordingly.
> >
> > MDSS and DSI would bump up MMCX performance state requirements on
> > their own, depending on the frequency being selected.
> >
>
> Right, but as I copied things from the sm8250.dtsi to come up with
> sm8150/sc8180x.dtsi I concluded that as soon as the assigned-clockrate
> in &mdss kicks in I need the performance state to be at NOM.
>
> So keeping the assigned-clockrate in &mdss means that MMCX will never go
> below NOM.

No, because once MDP is fully running, it will lower the clock frequency:

# grep mdp_clk /sys/kernel/debug/clk/clk_summary
          disp_cc_mdss_mdp_clk_src       1        1        0
150000000          0     0  50000         ?
             disp_cc_mdss_mdp_clk       2        2        0
150000000          0     0  50000         Y

>
> > > So wouldn't it be sufficient to ensure that MDSS_GDSC is parented by
> > > MMCX and then use opp-tables associated with the devices that scales the
> > > clock and thereby actually carries the "required-opps".
> >
> > Actually no. I set the performance state in the qcom_cc_map, so that
> > further register access is possible. Initially I was doing this in the
> > qcom_cc_really_probe() and it was already too late.
> > Just to remind: this patchset is not about MDSS_GDSC being parented by
> > MMCX, it is about dispcc/videocc registers being gated with MMCX.
> >
>
> So you're saying that just enabling MMCX isn't enough to touch the
> dispcc/videocc registers? If that's the case it seems like MMCX's
> definition of "on" needs to be adjusted - because just specifying MMCX
> as the power-domain for dispcc/videocc and enabling pm_runtime should
> ensure that MMCX is enabled when the clock registers are accessed (I
> don't see anything like that for the GDSC part though).

No, it is not enough. If I comment out the set_performance_state call,
the board reboots.

However I can set the opps as low as RET and register access will work.
I'll run more experiments and if everything works as expected, I can
use retention or min_svs level in the next iteration.
Just note that downstream specifies low_svs as minimum voltage level
for MMCX regulator.

> I thought our problem you had was that you need to set a
> performance_state in order to clock up some of the clocks - e.g.
> MDP_CLK.

No, even register access needs proper perf state.

>
> > > I presume your testing indicates that it doesn't matter on sm8250, but
> > > as stated above, 460MHz on sm8150/sc8180x requires nominal, so per your
> > > suggestion we'd have to vote nominal in &mdss, which means that if the
> > > DPU decides to go to 200MHz the &mdss will still keep the voltage at
> > > NOM, even though the DPU's opp-table says that LOW_SVS is sufficient.
> >
> > Let me check whether LOW_SVS is really a requirement or if setting
> > MIN_SVS would also be sufficient for that. Interesting enough, from
> > the downstream drivers it looks like dispcc should be able to work
> > with MIN_SVS, while videocc would require LOW_SVS.
> >
>
> LOW_SVS is the documented requirement for ticking MDP_CLK at 460MHz on
> SM8250. But I would expect we don't need LOW_SVS in order to poke the
> registers in dispcc/videocc.
>
> Regards,
> Bjorn
>
> > >
> > > Regards,
> > > Bjorn
> > >
> > > > ---
> > > >  drivers/clk/qcom/common.c | 55 ++++++++++++++++++++++++++++++++++-----
> > > >  drivers/clk/qcom/gdsc.c   |  6 +++++
> > > >  2 files changed, 55 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/drivers/clk/qcom/common.c b/drivers/clk/qcom/common.c
> > > > index 60d2a78d1395..eeb5b8c93032 100644
> > > > --- a/drivers/clk/qcom/common.c
> > > > +++ b/drivers/clk/qcom/common.c
> > > > @@ -10,6 +10,8 @@
> > > >  #include <linux/clk-provider.h>
> > > >  #include <linux/reset-controller.h>
> > > >  #include <linux/of.h>
> > > > +#include <linux/pm_opp.h>
> > > > +#include <linux/pm_runtime.h>
> > > >
> > > >  #include "common.h"
> > > >  #include "clk-rcg.h"
> > > > @@ -76,6 +78,16 @@ qcom_cc_map(struct platform_device *pdev, const struct qcom_cc_desc *desc)
> > > >       struct resource *res;
> > > >       struct device *dev = &pdev->dev;
> > > >
> > > > +     if (of_find_property(dev->of_node, "required-opps", NULL)) {
> > > > +             int pd_opp;
> > > > +
> > > > +             pd_opp = of_get_required_opp_performance_state(dev->of_node, 0);
> > > > +             if (pd_opp < 0)
> > > > +                     return ERR_PTR(pd_opp);
> > > > +
> > > > +             dev_pm_genpd_set_performance_state(dev, pd_opp);
> > > > +     }
> > > > +
> > > >       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > >       base = devm_ioremap_resource(dev, res);
> > > >       if (IS_ERR(base))
> > > > @@ -224,6 +236,11 @@ static struct clk_hw *qcom_cc_clk_hw_get(struct of_phandle_args *clkspec,
> > > >       return cc->rclks[idx] ? &cc->rclks[idx]->hw : NULL;
> > > >  }
> > > >
> > > > +static void qcom_cc_pm_runtime_disable(void *data)
> > > > +{
> > > > +     pm_runtime_disable(data);
> > > > +}
> > > > +
> > > >  int qcom_cc_really_probe(struct platform_device *pdev,
> > > >                        const struct qcom_cc_desc *desc, struct regmap *regmap)
> > > >  {
> > > > @@ -236,11 +253,28 @@ int qcom_cc_really_probe(struct platform_device *pdev,
> > > >       struct clk_regmap **rclks = desc->clks;
> > > >       size_t num_clk_hws = desc->num_clk_hws;
> > > >       struct clk_hw **clk_hws = desc->clk_hws;
> > > > +     bool use_pm = false;
> > > >
> > > >       cc = devm_kzalloc(dev, sizeof(*cc), GFP_KERNEL);
> > > >       if (!cc)
> > > >               return -ENOMEM;
> > > >
> > > > +     if (of_find_property(dev->of_node, "required-opps", NULL)) {
> > > > +             use_pm = true;
> > > > +
> > > > +             pm_runtime_enable(dev);
> > > > +             ret = pm_runtime_get_sync(dev);
> > > > +             if (ret < 0) {
> > > > +                     pm_runtime_put(dev);
> > > > +                     pm_runtime_disable(dev);
> > > > +                     return ret;
> > > > +             }
> > > > +
> > > > +             ret = devm_add_action_or_reset(dev, qcom_cc_pm_runtime_disable, dev);
> > > > +             if (ret)
> > > > +                     return ret;
> > > > +     }
> > > > +
> > > >       reset = &cc->reset;
> > > >       reset->rcdev.of_node = dev->of_node;
> > > >       reset->rcdev.ops = &qcom_reset_ops;
> > > > @@ -251,7 +285,7 @@ int qcom_cc_really_probe(struct platform_device *pdev,
> > > >
> > > >       ret = devm_reset_controller_register(dev, &reset->rcdev);
> > > >       if (ret)
> > > > -             return ret;
> > > > +             goto err;
> > > >
> > > >       if (desc->gdscs && desc->num_gdscs) {
> > > >               scd = devm_kzalloc(dev, sizeof(*scd), GFP_KERNEL);
> > > > @@ -262,11 +296,11 @@ int qcom_cc_really_probe(struct platform_device *pdev,
> > > >               scd->num = desc->num_gdscs;
> > > >               ret = gdsc_register(scd, &reset->rcdev, regmap);
> > > >               if (ret)
> > > > -                     return ret;
> > > > +                     goto err;
> > > >               ret = devm_add_action_or_reset(dev, qcom_cc_gdsc_unregister,
> > > >                                              scd);
> > > >               if (ret)
> > > > -                     return ret;
> > > > +                     goto err;
> > > >       }
> > > >
> > > >       cc->rclks = rclks;
> > > > @@ -277,7 +311,7 @@ int qcom_cc_really_probe(struct platform_device *pdev,
> > > >       for (i = 0; i < num_clk_hws; i++) {
> > > >               ret = devm_clk_hw_register(dev, clk_hws[i]);
> > > >               if (ret)
> > > > -                     return ret;
> > > > +                     goto err;
> > > >       }
> > > >
> > > >       for (i = 0; i < num_clks; i++) {
> > > > @@ -286,14 +320,23 @@ int qcom_cc_really_probe(struct platform_device *pdev,
> > > >
> > > >               ret = devm_clk_register_regmap(dev, rclks[i]);
> > > >               if (ret)
> > > > -                     return ret;
> > > > +                     goto err;
> > > >       }
> > > >
> > > >       ret = devm_of_clk_add_hw_provider(dev, qcom_cc_clk_hw_get, cc);
> > > >       if (ret)
> > > > -             return ret;
> > > > +             goto err;
> > > > +
> > > > +     if (use_pm)
> > > > +             pm_runtime_put(dev);
> > > >
> > > >       return 0;
> > > > +
> > > > +err:
> > > > +     if (use_pm)
> > > > +             pm_runtime_put(dev);
> > > > +
> > > > +     return ret;
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(qcom_cc_really_probe);
> > > >
> > > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > > > index 51ed640e527b..40c384bda4fc 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>
> > > > @@ -237,6 +238,8 @@ static int gdsc_enable(struct generic_pm_domain *domain)
> > > >       struct gdsc *sc = domain_to_gdsc(domain);
> > > >       int ret;
> > > >
> > > > +     pm_runtime_get_sync(domain->dev.parent);
> > > > +
> > > >       if (sc->pwrsts == PWRSTS_ON)
> > > >               return gdsc_deassert_reset(sc);
> > > >
> > > > @@ -326,6 +329,8 @@ static int gdsc_disable(struct generic_pm_domain *domain)
> > > >       if (sc->flags & CLAMP_IO)
> > > >               gdsc_assert_clamp_io(sc);
> > > >
> > > > +     pm_runtime_put(domain->dev.parent);
> > > > +
> > > >       return 0;
> > > >  }
> > > >
> > > > @@ -427,6 +432,7 @@ int gdsc_register(struct gdsc_desc *desc,
> > > >                       continue;
> > > >               scs[i]->regmap = regmap;
> > > >               scs[i]->rcdev = rcdev;
> > > > +             scs[i]->pd.dev.parent = desc->dev;
> > > >               ret = gdsc_init(scs[i]);
> > > >               if (ret)
> > > >                       return ret;
> > > > --
> > > > 2.30.2
> > > >
> >
> >
> >
> > --
> > With best wishes
> > Dmitry



-- 
With best wishes
Dmitry

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

* Re: [PATCH 3/6] clk: qcom: gdsc: enable optional power domain support
  2021-06-30 20:29         ` Dmitry Baryshkov
@ 2021-07-01  4:22           ` Bjorn Andersson
  2021-07-01 20:12             ` Dmitry Baryshkov
  0 siblings, 1 reply; 25+ messages in thread
From: Bjorn Andersson @ 2021-07-01  4:22 UTC (permalink / raw)
  To: Dmitry Baryshkov, Stephen Boyd, rnayak
  Cc: Andy Gross, Rob Herring, Jonathan Marek, Taniya Das,
	Michael Turquette, open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:COMMON CLK FRAMEWORK, Bryan O'Donoghue, Mark Brown,
	Ulf Hansson, open list

On Wed 30 Jun 15:29 CDT 2021, Dmitry Baryshkov wrote:

> On Wed, 30 Jun 2021 at 20:11, Bjorn Andersson
> <bjorn.andersson@linaro.org> wrote:
> >
> > On Wed 30 Jun 10:47 CDT 2021, Dmitry Baryshkov wrote:
> >
> > > Hi,
> > >
> > > On Wed, 30 Jun 2021 at 18:00, Bjorn Andersson
> > > <bjorn.andersson@linaro.org> wrote:
> > > >
> > > > On Wed 30 Jun 08:31 CDT 2021, Dmitry Baryshkov wrote:
> > > >
> > > > > On sm8250 dispcc and videocc registers are powered up by the MMCX power
> > > > > domain. Currently we used a regulator to enable this domain on demand,
> > > > > however this has some consequences, as genpd code is not reentrant.
> > > > >
> > > > > Teach Qualcomm clock controller code about setting up power domains and
> > > > > using them for gdsc control.
> > > > >
> > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > >
> > > > There's a proposal to add a generic binding for statically assigning a
> > > > performance states here:
> > > >
> > > > https://lore.kernel.org/linux-arm-msm/1622095949-2014-1-git-send-email-rnayak@codeaurora.org/
> 
> I checked this thread. It looks like Rajendra will also switch to the
> "required-opps" property. So if that series goes in first, we can drop
> the call to set_performance_state. If this one goes in first, we can
> drop the set_performance_state call after getting Rajendra's work in.
> 
> > > >
> > > >
> > > > But that said, do you really need this?
> > > >
> > > > The requirement for driving MMCX to LOW_SVS on SM8250 (and NOM on
> > > > SM8150/SC8180x) seems to only come from the fact that you push MDP_CLK
> > > > to 460MHz in &mdss.
> > > >
> > > > But then in &mdss_mdp you do the same using an opp-table based on the
> > > > actual MDP_CLK, which per its power-domains will scale MMCX accordingly.
> > >
> > > MDSS and DSI would bump up MMCX performance state requirements on
> > > their own, depending on the frequency being selected.
> > >
> >
> > Right, but as I copied things from the sm8250.dtsi to come up with
> > sm8150/sc8180x.dtsi I concluded that as soon as the assigned-clockrate
> > in &mdss kicks in I need the performance state to be at NOM.
> >
> > So keeping the assigned-clockrate in &mdss means that MMCX will never go
> > below NOM.
> 
> No, because once MDP is fully running, it will lower the clock frequency:
> 
> # grep mdp_clk /sys/kernel/debug/clk/clk_summary
>           disp_cc_mdss_mdp_clk_src       1        1        0
> 150000000          0     0  50000         ?
>              disp_cc_mdss_mdp_clk       2        2        0
> 150000000          0     0  50000         Y
> 

But won't that just lower the performance state requested by the
&mdss_mdp, while the &mdss still votes for NOM - with the outcome being
that we maintain NOM even if the clock goes down?

> >
> > > > So wouldn't it be sufficient to ensure that MDSS_GDSC is parented by
> > > > MMCX and then use opp-tables associated with the devices that scales the
> > > > clock and thereby actually carries the "required-opps".
> > >
> > > Actually no. I set the performance state in the qcom_cc_map, so that
> > > further register access is possible. Initially I was doing this in the
> > > qcom_cc_really_probe() and it was already too late.
> > > Just to remind: this patchset is not about MDSS_GDSC being parented by
> > > MMCX, it is about dispcc/videocc registers being gated with MMCX.
> > >
> >
> > So you're saying that just enabling MMCX isn't enough to touch the
> > dispcc/videocc registers? If that's the case it seems like MMCX's
> > definition of "on" needs to be adjusted - because just specifying MMCX
> > as the power-domain for dispcc/videocc and enabling pm_runtime should
> > ensure that MMCX is enabled when the clock registers are accessed (I
> > don't see anything like that for the GDSC part though).
> 
> No, it is not enough. If I comment out the set_performance_state call,
> the board reboots.
> 
> However I can set the opps as low as RET and register access will work.
> I'll run more experiments and if everything works as expected, I can
> use retention or min_svs level in the next iteration.
> Just note that downstream specifies low_svs as minimum voltage level
> for MMCX regulator.
> 

It doesn't make sense to me that a lone power_on on the power-domain
wouldn't give us enough juice to poke the registers.

But digging into the rpmhpd implementation answers the question, simply
invoking rpmhpd_power_on() is a nop, unless
rpmhpd_set_performance_state() has previously been called, because
pd->corner is 0. So this explains why enable isn't sufficient.

Compare this with the rpmpd implementation that will send an
enable request to the RPM in this case.

> > I thought our problem you had was that you need to set a
> > performance_state in order to clock up some of the clocks - e.g.
> > MDP_CLK.
> 
> No, even register access needs proper perf state.
> 

Per above finding you're right, enabling a rpmhpd power-domain doesn't
do anything. And I don't find this intuitive or even in line with the
expectations of the api...



A quick test booting rb3 and rb5 seems to indicate that it's possible to
initialize pd->corner to 1 (to ensure that enable at least gives us the
lowest level).

set_performance_state(0) will however then result in voting for "off",
rather than the lowest enabled level.


Rajendra, Stephen, is this really how rpmhpd is supposed to work?!

Regards,
Bjorn

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

* Re: [PATCH 1/6] dt-bindings: clock: qcom,dispcc-sm8x50: add mmcx power domain
  2021-06-30 13:31 ` [PATCH 1/6] dt-bindings: clock: qcom,dispcc-sm8x50: add mmcx power domain Dmitry Baryshkov
@ 2021-07-01 16:16   ` Ulf Hansson
  2021-07-01 16:39     ` Dmitry Baryshkov
  0 siblings, 1 reply; 25+ messages in thread
From: Ulf Hansson @ 2021-07-01 16:16 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Andy Gross, Bjorn Andersson, Rob Herring, Stephen Boyd,
	Taniya Das, Jonathan Marek, Michael Turquette, linux-arm-msm,
	DTML, linux-clk, Bryan O'Donoghue, Mark Brown,
	Linux Kernel Mailing List

On Wed, 30 Jun 2021 at 15:31, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> 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>
> ---
>  .../bindings/clock/qcom,dispcc-sm8x50.yaml    | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
> index 0cdf53f41f84..48d86fb34fa7 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
> +++ b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
> @@ -55,6 +55,16 @@ properties:
>    reg:
>      maxItems: 1
>
> +  power-domains:
> +    description:
> +      A phandle and PM domain specifier for the MMCX power domain.
> +    maxItems: 1
> +

Should you perhaps state that this is a parent domain? Or it isn't?

Related to this and because this is a power domain provider, you
should probably reference the common power-domain bindings somewhere
here. Along the lines of this:

- $ref: power-domain.yaml#

As an example, you could have a look at
Documentation/devicetree/bindings/power/pd-samsung.yaml.

> +  required-opps:
> +    description:
> +      Performance state to use for MMCX to enable register access.
> +    maxItems: 1

According to the previous discussions, I was under the assumption that
this property belongs to a consumer node rather than in the provider
node, no?

> +
>  required:
>    - compatible
>    - reg
> @@ -64,6 +74,15 @@ required:
>    - '#reset-cells'
>    - '#power-domain-cells'
>
> +# Either both properties are present or both are absent
> +dependencies:
> +  power-domains:
> +    required:
> +      - required-opps
> +  required-opps:
> +    required:
> +      - power-domains
> +
>  additionalProperties: false
>
>  examples:
> --
> 2.30.2
>

Kind regards
Uffe

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

* Re: [PATCH 1/6] dt-bindings: clock: qcom,dispcc-sm8x50: add mmcx power domain
  2021-07-01 16:16   ` Ulf Hansson
@ 2021-07-01 16:39     ` Dmitry Baryshkov
  2021-07-01 16:58       ` Ulf Hansson
  0 siblings, 1 reply; 25+ messages in thread
From: Dmitry Baryshkov @ 2021-07-01 16:39 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Andy Gross, Bjorn Andersson, Rob Herring, Stephen Boyd,
	Taniya Das, Jonathan Marek, Michael Turquette, linux-arm-msm,
	DTML, linux-clk, Bryan O'Donoghue, Mark Brown,
	Linux Kernel Mailing List

On Thu, 1 Jul 2021 at 19:17, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>
> On Wed, 30 Jun 2021 at 15:31, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > 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>
> > ---
> >  .../bindings/clock/qcom,dispcc-sm8x50.yaml    | 19 +++++++++++++++++++
> >  1 file changed, 19 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
> > index 0cdf53f41f84..48d86fb34fa7 100644
> > --- a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
> > +++ b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
> > @@ -55,6 +55,16 @@ properties:
> >    reg:
> >      maxItems: 1
> >
> > +  power-domains:
> > +    description:
> > +      A phandle and PM domain specifier for the MMCX power domain.
> > +    maxItems: 1
> > +
>
> Should you perhaps state that this is a parent domain? Or it isn't?
>
> Related to this and because this is a power domain provider, you
> should probably reference the common power-domain bindings somewhere
> here. Along the lines of this:
>
> - $ref: power-domain.yaml#
>
> As an example, you could have a look at
> Documentation/devicetree/bindings/power/pd-samsung.yaml.

I'll take a look.

>
> > +  required-opps:
> > +    description:
> > +      Performance state to use for MMCX to enable register access.
> > +    maxItems: 1
>
> According to the previous discussions, I was under the assumption that
> this property belongs to a consumer node rather than in the provider
> node, no?

It is both a consumer and a provider. It consumes SM8250_MMCX from
rpmhpd and provides MMSC_GDSC.

>
> > +
> >  required:
> >    - compatible
> >    - reg
> > @@ -64,6 +74,15 @@ required:
> >    - '#reset-cells'
> >    - '#power-domain-cells'
> >
> > +# Either both properties are present or both are absent
> > +dependencies:
> > +  power-domains:
> > +    required:
> > +      - required-opps
> > +  required-opps:
> > +    required:
> > +      - power-domains
> > +
> >  additionalProperties: false
> >
> >  examples:
> > --
> > 2.30.2
> >
>
> Kind regards
> Uffe



-- 
With best wishes
Dmitry

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

* Re: [PATCH 1/6] dt-bindings: clock: qcom,dispcc-sm8x50: add mmcx power domain
  2021-07-01 16:39     ` Dmitry Baryshkov
@ 2021-07-01 16:58       ` Ulf Hansson
  2021-07-01 19:26         ` Bjorn Andersson
  0 siblings, 1 reply; 25+ messages in thread
From: Ulf Hansson @ 2021-07-01 16:58 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Andy Gross, Bjorn Andersson, Rob Herring, Stephen Boyd,
	Taniya Das, Jonathan Marek, Michael Turquette, linux-arm-msm,
	DTML, linux-clk, Bryan O'Donoghue, Mark Brown,
	Linux Kernel Mailing List

On Thu, 1 Jul 2021 at 18:39, Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> On Thu, 1 Jul 2021 at 19:17, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >
> > On Wed, 30 Jun 2021 at 15:31, Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > 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>
> > > ---
> > >  .../bindings/clock/qcom,dispcc-sm8x50.yaml    | 19 +++++++++++++++++++
> > >  1 file changed, 19 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
> > > index 0cdf53f41f84..48d86fb34fa7 100644
> > > --- a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
> > > +++ b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
> > > @@ -55,6 +55,16 @@ properties:
> > >    reg:
> > >      maxItems: 1
> > >
> > > +  power-domains:
> > > +    description:
> > > +      A phandle and PM domain specifier for the MMCX power domain.
> > > +    maxItems: 1
> > > +
> >
> > Should you perhaps state that this is a parent domain? Or it isn't?
> >
> > Related to this and because this is a power domain provider, you
> > should probably reference the common power-domain bindings somewhere
> > here. Along the lines of this:
> >
> > - $ref: power-domain.yaml#
> >
> > As an example, you could have a look at
> > Documentation/devicetree/bindings/power/pd-samsung.yaml.
>
> I'll take a look.
>
> >
> > > +  required-opps:
> > > +    description:
> > > +      Performance state to use for MMCX to enable register access.
> > > +    maxItems: 1
> >
> > According to the previous discussions, I was under the assumption that
> > this property belongs to a consumer node rather than in the provider
> > node, no?
>
> It is both a consumer and a provider. It consumes SM8250_MMCX from
> rpmhpd and provides MMSC_GDSC.

That sounds a bit weird to me.

In my view and per the common power domain bindings (as pointed to
above): If a power domain provider is a consumer of another power
domain, that per definition means that there is a parent domain
specified.

[...]

Kind regards
Uffe

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

* Re: [PATCH 1/6] dt-bindings: clock: qcom,dispcc-sm8x50: add mmcx power domain
  2021-07-01 16:58       ` Ulf Hansson
@ 2021-07-01 19:26         ` Bjorn Andersson
  2021-07-06  7:23           ` Ulf Hansson
  0 siblings, 1 reply; 25+ messages in thread
From: Bjorn Andersson @ 2021-07-01 19:26 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Dmitry Baryshkov, Andy Gross, Rob Herring, Stephen Boyd,
	Taniya Das, Jonathan Marek, Michael Turquette, linux-arm-msm,
	DTML, linux-clk, Bryan O'Donoghue, Mark Brown,
	Linux Kernel Mailing List

On Thu 01 Jul 11:58 CDT 2021, Ulf Hansson wrote:

> On Thu, 1 Jul 2021 at 18:39, Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > On Thu, 1 Jul 2021 at 19:17, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >
> > > On Wed, 30 Jun 2021 at 15:31, Dmitry Baryshkov
> > > <dmitry.baryshkov@linaro.org> wrote:
> > > >
> > > > 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>
> > > > ---
> > > >  .../bindings/clock/qcom,dispcc-sm8x50.yaml    | 19 +++++++++++++++++++
> > > >  1 file changed, 19 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
> > > > index 0cdf53f41f84..48d86fb34fa7 100644
> > > > --- a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
> > > > +++ b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
> > > > @@ -55,6 +55,16 @@ properties:
> > > >    reg:
> > > >      maxItems: 1
> > > >
> > > > +  power-domains:
> > > > +    description:
> > > > +      A phandle and PM domain specifier for the MMCX power domain.
> > > > +    maxItems: 1
> > > > +
> > >
> > > Should you perhaps state that this is a parent domain? Or it isn't?
> > >
> > > Related to this and because this is a power domain provider, you
> > > should probably reference the common power-domain bindings somewhere
> > > here. Along the lines of this:
> > >
> > > - $ref: power-domain.yaml#
> > >
> > > As an example, you could have a look at
> > > Documentation/devicetree/bindings/power/pd-samsung.yaml.
> >
> > I'll take a look.
> >
> > >
> > > > +  required-opps:
> > > > +    description:
> > > > +      Performance state to use for MMCX to enable register access.
> > > > +    maxItems: 1
> > >
> > > According to the previous discussions, I was under the assumption that
> > > this property belongs to a consumer node rather than in the provider
> > > node, no?
> >
> > It is both a consumer and a provider. It consumes SM8250_MMCX from
> > rpmhpd and provides MMSC_GDSC.
> 
> That sounds a bit weird to me.
> 

dispcc is a hardware block powered by MMCX, so it is a consumer of it
and needs to control MMCX.

> In my view and per the common power domain bindings (as pointed to
> above): If a power domain provider is a consumer of another power
> domain, that per definition means that there is a parent domain
> specified.
> 

And in addition to needing MMCX to access the dispcc, the exposed
power-domain "MDSS_GDSC" is powered by the same MMCX and as such
MDSS_GDSC should be a subdomain of MMCX.


But what I was trying to say yesterday is that the power-domain property
should be sufficient and that we shouldn't need to drive MMCX to a
particular performance_state in order to access the registers.

Then as clients make votes on clock rates that requires higher
performance_state, they would describe this in their opp-tables etc.


But without any performance_state requests, pd->corner will in
rpmhpd_power_on() be 0 and as such powering on the power-domain won't
actually do anything. Similarly dev_pm_genpd_set_performance_state(dev,
0) on an active power-domain from rpmhpd will turn it off.


So the reason why Dmitry is adding the required-opps to the binding is
to get rpmhpd to actually tell the hardware to turn on the power domain.
And I don't think this is in accordance with the framework's
expectations.

Regards,
Bjorn

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

* Re: [PATCH 3/6] clk: qcom: gdsc: enable optional power domain support
  2021-07-01  4:22           ` Bjorn Andersson
@ 2021-07-01 20:12             ` Dmitry Baryshkov
  2021-07-01 20:57               ` Bjorn Andersson
  0 siblings, 1 reply; 25+ messages in thread
From: Dmitry Baryshkov @ 2021-07-01 20:12 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Stephen Boyd, Rajendra Nayak, Andy Gross, Rob Herring,
	Jonathan Marek, Taniya Das, Michael Turquette,
	open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:COMMON CLK FRAMEWORK, Bryan O'Donoghue, Mark Brown,
	Ulf Hansson, open list

On Thu, 1 Jul 2021 at 07:23, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
>
> On Wed 30 Jun 15:29 CDT 2021, Dmitry Baryshkov wrote:
>
> > On Wed, 30 Jun 2021 at 20:11, Bjorn Andersson
> > <bjorn.andersson@linaro.org> wrote:
> > >
> > > On Wed 30 Jun 10:47 CDT 2021, Dmitry Baryshkov wrote:
> > >
> > > > Hi,
> > > >
> > > > On Wed, 30 Jun 2021 at 18:00, Bjorn Andersson
> > > > <bjorn.andersson@linaro.org> wrote:
> > > > >
> > > > > On Wed 30 Jun 08:31 CDT 2021, Dmitry Baryshkov wrote:
> > > > >
> > > > > > On sm8250 dispcc and videocc registers are powered up by the MMCX power
> > > > > > domain. Currently we used a regulator to enable this domain on demand,
> > > > > > however this has some consequences, as genpd code is not reentrant.
> > > > > >
> > > > > > Teach Qualcomm clock controller code about setting up power domains and
> > > > > > using them for gdsc control.
> > > > > >
> > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > >
> > > > > There's a proposal to add a generic binding for statically assigning a
> > > > > performance states here:
> > > > >
> > > > > https://lore.kernel.org/linux-arm-msm/1622095949-2014-1-git-send-email-rnayak@codeaurora.org/
> >
> > I checked this thread. It looks like Rajendra will also switch to the
> > "required-opps" property. So if that series goes in first, we can drop
> > the call to set_performance_state. If this one goes in first, we can
> > drop the set_performance_state call after getting Rajendra's work in.
> >
> > > > >
> > > > >
> > > > > But that said, do you really need this?
> > > > >
> > > > > The requirement for driving MMCX to LOW_SVS on SM8250 (and NOM on
> > > > > SM8150/SC8180x) seems to only come from the fact that you push MDP_CLK
> > > > > to 460MHz in &mdss.
> > > > >
> > > > > But then in &mdss_mdp you do the same using an opp-table based on the
> > > > > actual MDP_CLK, which per its power-domains will scale MMCX accordingly.
> > > >
> > > > MDSS and DSI would bump up MMCX performance state requirements on
> > > > their own, depending on the frequency being selected.
> > > >
> > >
> > > Right, but as I copied things from the sm8250.dtsi to come up with
> > > sm8150/sc8180x.dtsi I concluded that as soon as the assigned-clockrate
> > > in &mdss kicks in I need the performance state to be at NOM.
> > >
> > > So keeping the assigned-clockrate in &mdss means that MMCX will never go
> > > below NOM.
> >
> > No, because once MDP is fully running, it will lower the clock frequency:
> >
> > # grep mdp_clk /sys/kernel/debug/clk/clk_summary
> >           disp_cc_mdss_mdp_clk_src       1        1        0
> > 150000000          0     0  50000         ?
> >              disp_cc_mdss_mdp_clk       2        2        0
> > 150000000          0     0  50000         Y
> >
>
> But won't that just lower the performance state requested by the
> &mdss_mdp, while the &mdss still votes for NOM - with the outcome being
> that we maintain NOM even if the clock goes down?

&mdss doesn't vote on performance state. At least it does not on
msm/msm-next which I have at hand right now.
&mdss toggles mdss_gdsc, but does not assign any performance state.

On the other hand &mdss_mdp and &dsi0 clearly vote on mmcx's performance state.

>
> > >
> > > > > So wouldn't it be sufficient to ensure that MDSS_GDSC is parented by
> > > > > MMCX and then use opp-tables associated with the devices that scales the
> > > > > clock and thereby actually carries the "required-opps".
> > > >
> > > > Actually no. I set the performance state in the qcom_cc_map, so that
> > > > further register access is possible. Initially I was doing this in the
> > > > qcom_cc_really_probe() and it was already too late.
> > > > Just to remind: this patchset is not about MDSS_GDSC being parented by
> > > > MMCX, it is about dispcc/videocc registers being gated with MMCX.
> > > >
> > >
> > > So you're saying that just enabling MMCX isn't enough to touch the
> > > dispcc/videocc registers? If that's the case it seems like MMCX's
> > > definition of "on" needs to be adjusted - because just specifying MMCX
> > > as the power-domain for dispcc/videocc and enabling pm_runtime should
> > > ensure that MMCX is enabled when the clock registers are accessed (I
> > > don't see anything like that for the GDSC part though).
> >
> > No, it is not enough. If I comment out the set_performance_state call,
> > the board reboots.
> >
> > However I can set the opps as low as RET and register access will work.
> > I'll run more experiments and if everything works as expected, I can
> > use retention or min_svs level in the next iteration.
> > Just note that downstream specifies low_svs as minimum voltage level
> > for MMCX regulator.
> >
>
> It doesn't make sense to me that a lone power_on on the power-domain
> wouldn't give us enough juice to poke the registers.
>
> But digging into the rpmhpd implementation answers the question, simply
> invoking rpmhpd_power_on() is a nop, unless
> rpmhpd_set_performance_state() has previously been called, because
> pd->corner is 0. So this explains why enable isn't sufficient.
>
> Compare this with the rpmpd implementation that will send an
> enable request to the RPM in this case.

Do you think that we should change that to:

rpmhpd_aggregate_corner(pd, max(pd->corner, 1)) ?

Or

rpmhpd_aggregate_corner(pd, max(pd->corner, pd->levels[1])) ?

>
> > > I thought our problem you had was that you need to set a
> > > performance_state in order to clock up some of the clocks - e.g.
> > > MDP_CLK.
> >
> > No, even register access needs proper perf state.
> >
>
> Per above finding you're right, enabling a rpmhpd power-domain doesn't
> do anything. And I don't find this intuitive or even in line with the
> expectations of the api...
>
>
>
> A quick test booting rb3 and rb5 seems to indicate that it's possible to
> initialize pd->corner to 1 (to ensure that enable at least gives us the
> lowest level).
>
> set_performance_state(0) will however then result in voting for "off",
> rather than the lowest enabled level.

Well, set_performance_state(0) means that "the device wouldn't
participate anymore to find the target performance state of the
genpd". Strictly speaking it does not specify whether it is ok to turn
it off or not. (like the regulator with the voltage set to 0V).
But I'd also like to hear a comment from Stephen here.


> Rajendra, Stephen, is this really how rpmhpd is supposed to work?!



-- 
With best wishes
Dmitry

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

* Re: [PATCH 3/6] clk: qcom: gdsc: enable optional power domain support
  2021-07-01 20:12             ` Dmitry Baryshkov
@ 2021-07-01 20:57               ` Bjorn Andersson
  2021-07-02  7:35                 ` Rajendra Nayak
  0 siblings, 1 reply; 25+ messages in thread
From: Bjorn Andersson @ 2021-07-01 20:57 UTC (permalink / raw)
  To: Dmitry Baryshkov
  Cc: Stephen Boyd, Rajendra Nayak, Andy Gross, Rob Herring,
	Jonathan Marek, Taniya Das, Michael Turquette,
	open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:COMMON CLK FRAMEWORK, Bryan O'Donoghue, Mark Brown,
	Ulf Hansson, open list

On Thu 01 Jul 15:12 CDT 2021, Dmitry Baryshkov wrote:

> On Thu, 1 Jul 2021 at 07:23, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
> >
> > On Wed 30 Jun 15:29 CDT 2021, Dmitry Baryshkov wrote:
> >
> > > On Wed, 30 Jun 2021 at 20:11, Bjorn Andersson
> > > <bjorn.andersson@linaro.org> wrote:
> > > >
> > > > On Wed 30 Jun 10:47 CDT 2021, Dmitry Baryshkov wrote:
> > > >
> > > > > Hi,
> > > > >
> > > > > On Wed, 30 Jun 2021 at 18:00, Bjorn Andersson
> > > > > <bjorn.andersson@linaro.org> wrote:
> > > > > >
> > > > > > On Wed 30 Jun 08:31 CDT 2021, Dmitry Baryshkov wrote:
> > > > > >
> > > > > > > On sm8250 dispcc and videocc registers are powered up by the MMCX power
> > > > > > > domain. Currently we used a regulator to enable this domain on demand,
> > > > > > > however this has some consequences, as genpd code is not reentrant.
> > > > > > >
> > > > > > > Teach Qualcomm clock controller code about setting up power domains and
> > > > > > > using them for gdsc control.
> > > > > > >
> > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > >
> > > > > > There's a proposal to add a generic binding for statically assigning a
> > > > > > performance states here:
> > > > > >
> > > > > > https://lore.kernel.org/linux-arm-msm/1622095949-2014-1-git-send-email-rnayak@codeaurora.org/
> > >
> > > I checked this thread. It looks like Rajendra will also switch to the
> > > "required-opps" property. So if that series goes in first, we can drop
> > > the call to set_performance_state. If this one goes in first, we can
> > > drop the set_performance_state call after getting Rajendra's work in.
> > >
> > > > > >
> > > > > >
> > > > > > But that said, do you really need this?
> > > > > >
> > > > > > The requirement for driving MMCX to LOW_SVS on SM8250 (and NOM on
> > > > > > SM8150/SC8180x) seems to only come from the fact that you push MDP_CLK
> > > > > > to 460MHz in &mdss.
> > > > > >
> > > > > > But then in &mdss_mdp you do the same using an opp-table based on the
> > > > > > actual MDP_CLK, which per its power-domains will scale MMCX accordingly.
> > > > >
> > > > > MDSS and DSI would bump up MMCX performance state requirements on
> > > > > their own, depending on the frequency being selected.
> > > > >
> > > >
> > > > Right, but as I copied things from the sm8250.dtsi to come up with
> > > > sm8150/sc8180x.dtsi I concluded that as soon as the assigned-clockrate
> > > > in &mdss kicks in I need the performance state to be at NOM.
> > > >
> > > > So keeping the assigned-clockrate in &mdss means that MMCX will never go
> > > > below NOM.
> > >
> > > No, because once MDP is fully running, it will lower the clock frequency:
> > >
> > > # grep mdp_clk /sys/kernel/debug/clk/clk_summary
> > >           disp_cc_mdss_mdp_clk_src       1        1        0
> > > 150000000          0     0  50000         ?
> > >              disp_cc_mdss_mdp_clk       2        2        0
> > > 150000000          0     0  50000         Y
> > >
> >
> > But won't that just lower the performance state requested by the
> > &mdss_mdp, while the &mdss still votes for NOM - with the outcome being
> > that we maintain NOM even if the clock goes down?
> 
> &mdss doesn't vote on performance state. At least it does not on
> msm/msm-next which I have at hand right now.
> &mdss toggles mdss_gdsc, but does not assign any performance state.
> 

Right, but per the upstream implementation, enabling MDSS_GDSC could in
itself fail, because unless something else has driven up the performance
state the enable that trickles up won't actually turn on the supply.

> On the other hand &mdss_mdp and &dsi0 clearly vote on mmcx's performance state.
> 

Right, but it does so as part of its clock scaling, so this makes
perfect sense to me.

> >
> > > >
> > > > > > So wouldn't it be sufficient to ensure that MDSS_GDSC is parented by
> > > > > > MMCX and then use opp-tables associated with the devices that scales the
> > > > > > clock and thereby actually carries the "required-opps".
> > > > >
> > > > > Actually no. I set the performance state in the qcom_cc_map, so that
> > > > > further register access is possible. Initially I was doing this in the
> > > > > qcom_cc_really_probe() and it was already too late.
> > > > > Just to remind: this patchset is not about MDSS_GDSC being parented by
> > > > > MMCX, it is about dispcc/videocc registers being gated with MMCX.
> > > > >
> > > >
> > > > So you're saying that just enabling MMCX isn't enough to touch the
> > > > dispcc/videocc registers? If that's the case it seems like MMCX's
> > > > definition of "on" needs to be adjusted - because just specifying MMCX
> > > > as the power-domain for dispcc/videocc and enabling pm_runtime should
> > > > ensure that MMCX is enabled when the clock registers are accessed (I
> > > > don't see anything like that for the GDSC part though).
> > >
> > > No, it is not enough. If I comment out the set_performance_state call,
> > > the board reboots.
> > >
> > > However I can set the opps as low as RET and register access will work.
> > > I'll run more experiments and if everything works as expected, I can
> > > use retention or min_svs level in the next iteration.
> > > Just note that downstream specifies low_svs as minimum voltage level
> > > for MMCX regulator.
> > >
> >
> > It doesn't make sense to me that a lone power_on on the power-domain
> > wouldn't give us enough juice to poke the registers.
> >
> > But digging into the rpmhpd implementation answers the question, simply
> > invoking rpmhpd_power_on() is a nop, unless
> > rpmhpd_set_performance_state() has previously been called, because
> > pd->corner is 0. So this explains why enable isn't sufficient.
> >
> > Compare this with the rpmpd implementation that will send an
> > enable request to the RPM in this case.
> 
> Do you think that we should change that to:
> 
> rpmhpd_aggregate_corner(pd, max(pd->corner, 1)) ?
> 
> Or
> 
> rpmhpd_aggregate_corner(pd, max(pd->corner, pd->levels[1])) ?
> 

In rpmhpd_power_on() and rpmhpd_set_performance_state() we pass the
index of the entry in pd->levels[] that we want, but in
rpmhpd_power_off() we pass the value of pd->levels[0].

So I would suggest dropping the if (pd->corner) and doing:

  rpmhpd_aggregate_corner(pd, max(pd->corner, 1));

And it seems both rb3 and rb5 still boots with this change (but I need
to do some more testing to know for sure).

> >
> > > > I thought our problem you had was that you need to set a
> > > > performance_state in order to clock up some of the clocks - e.g.
> > > > MDP_CLK.
> > >
> > > No, even register access needs proper perf state.
> > >
> >
> > Per above finding you're right, enabling a rpmhpd power-domain doesn't
> > do anything. And I don't find this intuitive or even in line with the
> > expectations of the api...
> >
> >
> >
> > A quick test booting rb3 and rb5 seems to indicate that it's possible to
> > initialize pd->corner to 1 (to ensure that enable at least gives us the
> > lowest level).
> >
> > set_performance_state(0) will however then result in voting for "off",
> > rather than the lowest enabled level.
> 
> Well, set_performance_state(0) means that "the device wouldn't
> participate anymore to find the target performance state of the
> genpd".

I agree.

> Strictly speaking it does not specify whether it is ok to turn
> it off or not. (like the regulator with the voltage set to 0V).
> But I'd also like to hear a comment from Stephen here.
> 

Looking at other power-domains (e.g. gdsc and rpmpd) enabling the
power-domain means it is no longer off and if you need some specific
performance state you have to vote for that.

So I'm also interested in hearing if there's any reasoning behind how
this was written.

Regards,
Bjorn

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

* Re: [PATCH 3/6] clk: qcom: gdsc: enable optional power domain support
  2021-07-01 20:57               ` Bjorn Andersson
@ 2021-07-02  7:35                 ` Rajendra Nayak
  2021-07-03  3:20                   ` Bjorn Andersson
  0 siblings, 1 reply; 25+ messages in thread
From: Rajendra Nayak @ 2021-07-02  7:35 UTC (permalink / raw)
  To: Bjorn Andersson, Dmitry Baryshkov
  Cc: Stephen Boyd, Andy Gross, Rob Herring, Jonathan Marek,
	Taniya Das, Michael Turquette,
	open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:COMMON CLK FRAMEWORK, Bryan O'Donoghue, Mark Brown,
	Ulf Hansson, open list



On 7/2/2021 2:27 AM, Bjorn Andersson wrote:
> On Thu 01 Jul 15:12 CDT 2021, Dmitry Baryshkov wrote:
> 
>> On Thu, 1 Jul 2021 at 07:23, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
>>>
>>> On Wed 30 Jun 15:29 CDT 2021, Dmitry Baryshkov wrote:
>>>
>>>> On Wed, 30 Jun 2021 at 20:11, Bjorn Andersson
>>>> <bjorn.andersson@linaro.org> wrote:
>>>>>
>>>>> On Wed 30 Jun 10:47 CDT 2021, Dmitry Baryshkov wrote:
>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On Wed, 30 Jun 2021 at 18:00, Bjorn Andersson
>>>>>> <bjorn.andersson@linaro.org> wrote:
>>>>>>>
>>>>>>> On Wed 30 Jun 08:31 CDT 2021, Dmitry Baryshkov wrote:
>>>>>>>
>>>>>>>> On sm8250 dispcc and videocc registers are powered up by the MMCX power
>>>>>>>> domain. Currently we used a regulator to enable this domain on demand,
>>>>>>>> however this has some consequences, as genpd code is not reentrant.
>>>>>>>>
>>>>>>>> Teach Qualcomm clock controller code about setting up power domains and
>>>>>>>> using them for gdsc control.
>>>>>>>>
>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>
>>>>>>> There's a proposal to add a generic binding for statically assigning a
>>>>>>> performance states here:
>>>>>>>
>>>>>>> https://lore.kernel.org/linux-arm-msm/1622095949-2014-1-git-send-email-rnayak@codeaurora.org/
>>>>
>>>> I checked this thread. It looks like Rajendra will also switch to the
>>>> "required-opps" property. So if that series goes in first, we can drop
>>>> the call to set_performance_state. If this one goes in first, we can
>>>> drop the set_performance_state call after getting Rajendra's work in.
>>>>
>>>>>>>
>>>>>>>
>>>>>>> But that said, do you really need this?
>>>>>>>
>>>>>>> The requirement for driving MMCX to LOW_SVS on SM8250 (and NOM on
>>>>>>> SM8150/SC8180x) seems to only come from the fact that you push MDP_CLK
>>>>>>> to 460MHz in &mdss.
>>>>>>>
>>>>>>> But then in &mdss_mdp you do the same using an opp-table based on the
>>>>>>> actual MDP_CLK, which per its power-domains will scale MMCX accordingly.
>>>>>>
>>>>>> MDSS and DSI would bump up MMCX performance state requirements on
>>>>>> their own, depending on the frequency being selected.
>>>>>>
>>>>>
>>>>> Right, but as I copied things from the sm8250.dtsi to come up with
>>>>> sm8150/sc8180x.dtsi I concluded that as soon as the assigned-clockrate
>>>>> in &mdss kicks in I need the performance state to be at NOM.
>>>>>
>>>>> So keeping the assigned-clockrate in &mdss means that MMCX will never go
>>>>> below NOM.
>>>>
>>>> No, because once MDP is fully running, it will lower the clock frequency:
>>>>
>>>> # grep mdp_clk /sys/kernel/debug/clk/clk_summary
>>>>            disp_cc_mdss_mdp_clk_src       1        1        0
>>>> 150000000          0     0  50000         ?
>>>>               disp_cc_mdss_mdp_clk       2        2        0
>>>> 150000000          0     0  50000         Y
>>>>
>>>
>>> But won't that just lower the performance state requested by the
>>> &mdss_mdp, while the &mdss still votes for NOM - with the outcome being
>>> that we maintain NOM even if the clock goes down?
>>
>> &mdss doesn't vote on performance state. At least it does not on
>> msm/msm-next which I have at hand right now.
>> &mdss toggles mdss_gdsc, but does not assign any performance state.
>>
> 
> Right, but per the upstream implementation, enabling MDSS_GDSC could in
> itself fail, because unless something else has driven up the performance
> state the enable that trickles up won't actually turn on the supply.
> 
>> On the other hand &mdss_mdp and &dsi0 clearly vote on mmcx's performance state.
>>
> 
> Right, but it does so as part of its clock scaling, so this makes
> perfect sense to me.
> 
>>>
>>>>>
>>>>>>> So wouldn't it be sufficient to ensure that MDSS_GDSC is parented by
>>>>>>> MMCX and then use opp-tables associated with the devices that scales the
>>>>>>> clock and thereby actually carries the "required-opps".
>>>>>>
>>>>>> Actually no. I set the performance state in the qcom_cc_map, so that
>>>>>> further register access is possible. Initially I was doing this in the
>>>>>> qcom_cc_really_probe() and it was already too late.
>>>>>> Just to remind: this patchset is not about MDSS_GDSC being parented by
>>>>>> MMCX, it is about dispcc/videocc registers being gated with MMCX.
>>>>>>
>>>>>
>>>>> So you're saying that just enabling MMCX isn't enough to touch the
>>>>> dispcc/videocc registers? If that's the case it seems like MMCX's
>>>>> definition of "on" needs to be adjusted - because just specifying MMCX
>>>>> as the power-domain for dispcc/videocc and enabling pm_runtime should
>>>>> ensure that MMCX is enabled when the clock registers are accessed (I
>>>>> don't see anything like that for the GDSC part though).
>>>>
>>>> No, it is not enough. If I comment out the set_performance_state call,
>>>> the board reboots.
>>>>
>>>> However I can set the opps as low as RET and register access will work.
>>>> I'll run more experiments and if everything works as expected, I can
>>>> use retention or min_svs level in the next iteration.
>>>> Just note that downstream specifies low_svs as minimum voltage level
>>>> for MMCX regulator.
>>>>
>>>
>>> It doesn't make sense to me that a lone power_on on the power-domain
>>> wouldn't give us enough juice to poke the registers.
>>>
>>> But digging into the rpmhpd implementation answers the question, simply
>>> invoking rpmhpd_power_on() is a nop, unless
>>> rpmhpd_set_performance_state() has previously been called, because
>>> pd->corner is 0. So this explains why enable isn't sufficient.
>>>
>>> Compare this with the rpmpd implementation that will send an
>>> enable request to the RPM in this case.

Right, in case of RPMh, there was no separate 'enable' request which
could be sent, there was just a 'corner' request.

I don't completely recall, but the reason to not send a 'default corner'
on enable was perhaps to keep the enable and set_performance orthogonal.

However, given we then decided to send the lowest possible corner
in disable, it perhaps makes sense to send a 'lowest non-zero corner' on enable
as well.

>>
>> Do you think that we should change that to:
>>
>> rpmhpd_aggregate_corner(pd, max(pd->corner, 1)) ?
>>
>> Or
>>
>> rpmhpd_aggregate_corner(pd, max(pd->corner, pd->levels[1])) ?
>>
> 
> In rpmhpd_power_on() and rpmhpd_set_performance_state() we pass the
> index of the entry in pd->levels[] that we want, but in
> rpmhpd_power_off() we pass the value of pd->levels[0].
> 
> So I would suggest dropping the if (pd->corner) and doing:
> 
>    rpmhpd_aggregate_corner(pd, max(pd->corner, 1));

So the index value represents the hlvl (0-15) that eventually gets sent to
rpmh, the pd->levels are the sparse vlvl values that come from the command
DB mappings.

What seems sane is to sent the lowest non-zero vlvl. That in most cases
would be at index 1, but for some which do not support complete off,
it could be at index 0.

> 
> And it seems both rb3 and rb5 still boots with this change (but I need
> to do some more testing to know for sure).
> 
>>>
>>>>> I thought our problem you had was that you need to set a
>>>>> performance_state in order to clock up some of the clocks - e.g.
>>>>> MDP_CLK.
>>>>
>>>> No, even register access needs proper perf state.
>>>>
>>>
>>> Per above finding you're right, enabling a rpmhpd power-domain doesn't
>>> do anything. And I don't find this intuitive or even in line with the
>>> expectations of the api...
>>>
>>>
>>>
>>> A quick test booting rb3 and rb5 seems to indicate that it's possible to
>>> initialize pd->corner to 1 (to ensure that enable at least gives us the
>>> lowest level).
>>>
>>> set_performance_state(0) will however then result in voting for "off",
>>> rather than the lowest enabled level.
>>
>> Well, set_performance_state(0) means that "the device wouldn't
>> participate anymore to find the target performance state of the
>> genpd".
> 
> I agree.
> 
>> Strictly speaking it does not specify whether it is ok to turn
>> it off or not. (like the regulator with the voltage set to 0V).
>> But I'd also like to hear a comment from Stephen here.
>>
> 
> Looking at other power-domains (e.g. gdsc and rpmpd) enabling the
> power-domain means it is no longer off and if you need some specific
> performance state you have to vote for that.
> 
> So I'm also interested in hearing if there's any reasoning behind how
> this was written.
> 
> Regards,
> Bjorn
> 

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

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

* Re: [PATCH 3/6] clk: qcom: gdsc: enable optional power domain support
  2021-07-02  7:35                 ` Rajendra Nayak
@ 2021-07-03  3:20                   ` Bjorn Andersson
  2021-07-05  4:33                     ` Rajendra Nayak
  0 siblings, 1 reply; 25+ messages in thread
From: Bjorn Andersson @ 2021-07-03  3:20 UTC (permalink / raw)
  To: Rajendra Nayak
  Cc: Dmitry Baryshkov, Stephen Boyd, Andy Gross, Rob Herring,
	Jonathan Marek, Taniya Das, Michael Turquette,
	open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:COMMON CLK FRAMEWORK, Bryan O'Donoghue, Mark Brown,
	Ulf Hansson, open list

On Fri 02 Jul 02:35 CDT 2021, Rajendra Nayak wrote:

> 
> 
> On 7/2/2021 2:27 AM, Bjorn Andersson wrote:
> > On Thu 01 Jul 15:12 CDT 2021, Dmitry Baryshkov wrote:
> > 
> > > On Thu, 1 Jul 2021 at 07:23, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
> > > > 
> > > > On Wed 30 Jun 15:29 CDT 2021, Dmitry Baryshkov wrote:
> > > > 
> > > > > On Wed, 30 Jun 2021 at 20:11, Bjorn Andersson
> > > > > <bjorn.andersson@linaro.org> wrote:
> > > > > > 
> > > > > > On Wed 30 Jun 10:47 CDT 2021, Dmitry Baryshkov wrote:
> > > > > > 
> > > > > > > Hi,
> > > > > > > 
> > > > > > > On Wed, 30 Jun 2021 at 18:00, Bjorn Andersson
> > > > > > > <bjorn.andersson@linaro.org> wrote:
> > > > > > > > 
> > > > > > > > On Wed 30 Jun 08:31 CDT 2021, Dmitry Baryshkov wrote:
> > > > > > > > 
> > > > > > > > > On sm8250 dispcc and videocc registers are powered up by the MMCX power
> > > > > > > > > domain. Currently we used a regulator to enable this domain on demand,
> > > > > > > > > however this has some consequences, as genpd code is not reentrant.
> > > > > > > > > 
> > > > > > > > > Teach Qualcomm clock controller code about setting up power domains and
> > > > > > > > > using them for gdsc control.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > > > > > > 
> > > > > > > > There's a proposal to add a generic binding for statically assigning a
> > > > > > > > performance states here:
> > > > > > > > 
> > > > > > > > https://lore.kernel.org/linux-arm-msm/1622095949-2014-1-git-send-email-rnayak@codeaurora.org/
> > > > > 
> > > > > I checked this thread. It looks like Rajendra will also switch to the
> > > > > "required-opps" property. So if that series goes in first, we can drop
> > > > > the call to set_performance_state. If this one goes in first, we can
> > > > > drop the set_performance_state call after getting Rajendra's work in.
> > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > But that said, do you really need this?
> > > > > > > > 
> > > > > > > > The requirement for driving MMCX to LOW_SVS on SM8250 (and NOM on
> > > > > > > > SM8150/SC8180x) seems to only come from the fact that you push MDP_CLK
> > > > > > > > to 460MHz in &mdss.
> > > > > > > > 
> > > > > > > > But then in &mdss_mdp you do the same using an opp-table based on the
> > > > > > > > actual MDP_CLK, which per its power-domains will scale MMCX accordingly.
> > > > > > > 
> > > > > > > MDSS and DSI would bump up MMCX performance state requirements on
> > > > > > > their own, depending on the frequency being selected.
> > > > > > > 
> > > > > > 
> > > > > > Right, but as I copied things from the sm8250.dtsi to come up with
> > > > > > sm8150/sc8180x.dtsi I concluded that as soon as the assigned-clockrate
> > > > > > in &mdss kicks in I need the performance state to be at NOM.
> > > > > > 
> > > > > > So keeping the assigned-clockrate in &mdss means that MMCX will never go
> > > > > > below NOM.
> > > > > 
> > > > > No, because once MDP is fully running, it will lower the clock frequency:
> > > > > 
> > > > > # grep mdp_clk /sys/kernel/debug/clk/clk_summary
> > > > >            disp_cc_mdss_mdp_clk_src       1        1        0
> > > > > 150000000          0     0  50000         ?
> > > > >               disp_cc_mdss_mdp_clk       2        2        0
> > > > > 150000000          0     0  50000         Y
> > > > > 
> > > > 
> > > > But won't that just lower the performance state requested by the
> > > > &mdss_mdp, while the &mdss still votes for NOM - with the outcome being
> > > > that we maintain NOM even if the clock goes down?
> > > 
> > > &mdss doesn't vote on performance state. At least it does not on
> > > msm/msm-next which I have at hand right now.
> > > &mdss toggles mdss_gdsc, but does not assign any performance state.
> > > 
> > 
> > Right, but per the upstream implementation, enabling MDSS_GDSC could in
> > itself fail, because unless something else has driven up the performance
> > state the enable that trickles up won't actually turn on the supply.
> > 
> > > On the other hand &mdss_mdp and &dsi0 clearly vote on mmcx's performance state.
> > > 
> > 
> > Right, but it does so as part of its clock scaling, so this makes
> > perfect sense to me.
> > 
> > > > 
> > > > > > 
> > > > > > > > So wouldn't it be sufficient to ensure that MDSS_GDSC is parented by
> > > > > > > > MMCX and then use opp-tables associated with the devices that scales the
> > > > > > > > clock and thereby actually carries the "required-opps".
> > > > > > > 
> > > > > > > Actually no. I set the performance state in the qcom_cc_map, so that
> > > > > > > further register access is possible. Initially I was doing this in the
> > > > > > > qcom_cc_really_probe() and it was already too late.
> > > > > > > Just to remind: this patchset is not about MDSS_GDSC being parented by
> > > > > > > MMCX, it is about dispcc/videocc registers being gated with MMCX.
> > > > > > > 
> > > > > > 
> > > > > > So you're saying that just enabling MMCX isn't enough to touch the
> > > > > > dispcc/videocc registers? If that's the case it seems like MMCX's
> > > > > > definition of "on" needs to be adjusted - because just specifying MMCX
> > > > > > as the power-domain for dispcc/videocc and enabling pm_runtime should
> > > > > > ensure that MMCX is enabled when the clock registers are accessed (I
> > > > > > don't see anything like that for the GDSC part though).
> > > > > 
> > > > > No, it is not enough. If I comment out the set_performance_state call,
> > > > > the board reboots.
> > > > > 
> > > > > However I can set the opps as low as RET and register access will work.
> > > > > I'll run more experiments and if everything works as expected, I can
> > > > > use retention or min_svs level in the next iteration.
> > > > > Just note that downstream specifies low_svs as minimum voltage level
> > > > > for MMCX regulator.
> > > > > 
> > > > 
> > > > It doesn't make sense to me that a lone power_on on the power-domain
> > > > wouldn't give us enough juice to poke the registers.
> > > > 
> > > > But digging into the rpmhpd implementation answers the question, simply
> > > > invoking rpmhpd_power_on() is a nop, unless
> > > > rpmhpd_set_performance_state() has previously been called, because
> > > > pd->corner is 0. So this explains why enable isn't sufficient.
> > > > 
> > > > Compare this with the rpmpd implementation that will send an
> > > > enable request to the RPM in this case.
> 
> Right, in case of RPMh, there was no separate 'enable' request which
> could be sent, there was just a 'corner' request.
> 
> I don't completely recall, but the reason to not send a 'default corner'
> on enable was perhaps to keep the enable and set_performance orthogonal.
> 
> However, given we then decided to send the lowest possible corner
> in disable, it perhaps makes sense to send a 'lowest non-zero corner' on enable
> as well.
> 

I was slightly worries that the change would dump cx and mx from
whatever level the bootloader put it at down to LOW_SVS during boot.

But both rb3 and rb5 boots fine with this change, so I posted it here:
https://lore.kernel.org/linux-arm-msm/20210703025449.2687201-1-bjorn.andersson@linaro.org/

> > > 
> > > Do you think that we should change that to:
> > > 
> > > rpmhpd_aggregate_corner(pd, max(pd->corner, 1)) ?
> > > 
> > > Or
> > > 
> > > rpmhpd_aggregate_corner(pd, max(pd->corner, pd->levels[1])) ?
> > > 
> > 
> > In rpmhpd_power_on() and rpmhpd_set_performance_state() we pass the
> > index of the entry in pd->levels[] that we want, but in
> > rpmhpd_power_off() we pass the value of pd->levels[0].
> > 
> > So I would suggest dropping the if (pd->corner) and doing:
> > 
> >    rpmhpd_aggregate_corner(pd, max(pd->corner, 1));
> 
> So the index value represents the hlvl (0-15) that eventually gets sent to
> rpmh, the pd->levels are the sparse vlvl values that come from the command
> DB mappings.
> 
> What seems sane is to sent the lowest non-zero vlvl. That in most cases
> would be at index 1, but for some which do not support complete off,
> it could be at index 0.
> 

I took this into consideration in above patch, keeping track of the
first non-zero corner and using this when the domain is enabled.

Unfortunately, if the first entry would be say LOW_SVS power_off would
request corner (hlvl) 64. So I fixed that in patch 1/2 in above series.

Regards,
Bjorn

> > 
> > And it seems both rb3 and rb5 still boots with this change (but I need
> > to do some more testing to know for sure).
> > 
> > > > 
> > > > > > I thought our problem you had was that you need to set a
> > > > > > performance_state in order to clock up some of the clocks - e.g.
> > > > > > MDP_CLK.
> > > > > 
> > > > > No, even register access needs proper perf state.
> > > > > 
> > > > 
> > > > Per above finding you're right, enabling a rpmhpd power-domain doesn't
> > > > do anything. And I don't find this intuitive or even in line with the
> > > > expectations of the api...
> > > > 
> > > > 
> > > > 
> > > > A quick test booting rb3 and rb5 seems to indicate that it's possible to
> > > > initialize pd->corner to 1 (to ensure that enable at least gives us the
> > > > lowest level).
> > > > 
> > > > set_performance_state(0) will however then result in voting for "off",
> > > > rather than the lowest enabled level.
> > > 
> > > Well, set_performance_state(0) means that "the device wouldn't
> > > participate anymore to find the target performance state of the
> > > genpd".
> > 
> > I agree.
> > 
> > > Strictly speaking it does not specify whether it is ok to turn
> > > it off or not. (like the regulator with the voltage set to 0V).
> > > But I'd also like to hear a comment from Stephen here.
> > > 
> > 
> > Looking at other power-domains (e.g. gdsc and rpmpd) enabling the
> > power-domain means it is no longer off and if you need some specific
> > performance state you have to vote for that.
> > 
> > So I'm also interested in hearing if there's any reasoning behind how
> > this was written.
> > 
> > Regards,
> > Bjorn
> > 
> 
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation

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

* Re: [PATCH 3/6] clk: qcom: gdsc: enable optional power domain support
  2021-07-03  3:20                   ` Bjorn Andersson
@ 2021-07-05  4:33                     ` Rajendra Nayak
  0 siblings, 0 replies; 25+ messages in thread
From: Rajendra Nayak @ 2021-07-05  4:33 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Dmitry Baryshkov, Stephen Boyd, Andy Gross, Rob Herring,
	Jonathan Marek, Taniya Das, Michael Turquette,
	open list:DRM DRIVER FOR MSM ADRENO GPU,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	open list:COMMON CLK FRAMEWORK, Bryan O'Donoghue, Mark Brown,
	Ulf Hansson, open list


On 7/3/2021 8:50 AM, Bjorn Andersson wrote:
> On Fri 02 Jul 02:35 CDT 2021, Rajendra Nayak wrote:
> 
>>
>>
>> On 7/2/2021 2:27 AM, Bjorn Andersson wrote:
>>> On Thu 01 Jul 15:12 CDT 2021, Dmitry Baryshkov wrote:
>>>
>>>> On Thu, 1 Jul 2021 at 07:23, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
>>>>>
>>>>> On Wed 30 Jun 15:29 CDT 2021, Dmitry Baryshkov wrote:
>>>>>
>>>>>> On Wed, 30 Jun 2021 at 20:11, Bjorn Andersson
>>>>>> <bjorn.andersson@linaro.org> wrote:
>>>>>>>
>>>>>>> On Wed 30 Jun 10:47 CDT 2021, Dmitry Baryshkov wrote:
>>>>>>>
>>>>>>>> Hi,
>>>>>>>>
>>>>>>>> On Wed, 30 Jun 2021 at 18:00, Bjorn Andersson
>>>>>>>> <bjorn.andersson@linaro.org> wrote:
>>>>>>>>>
>>>>>>>>> On Wed 30 Jun 08:31 CDT 2021, Dmitry Baryshkov wrote:
>>>>>>>>>
>>>>>>>>>> On sm8250 dispcc and videocc registers are powered up by the MMCX power
>>>>>>>>>> domain. Currently we used a regulator to enable this domain on demand,
>>>>>>>>>> however this has some consequences, as genpd code is not reentrant.
>>>>>>>>>>
>>>>>>>>>> Teach Qualcomm clock controller code about setting up power domains and
>>>>>>>>>> using them for gdsc control.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>>>>>>>
>>>>>>>>> There's a proposal to add a generic binding for statically assigning a
>>>>>>>>> performance states here:
>>>>>>>>>
>>>>>>>>> https://lore.kernel.org/linux-arm-msm/1622095949-2014-1-git-send-email-rnayak@codeaurora.org/
>>>>>>
>>>>>> I checked this thread. It looks like Rajendra will also switch to the
>>>>>> "required-opps" property. So if that series goes in first, we can drop
>>>>>> the call to set_performance_state. If this one goes in first, we can
>>>>>> drop the set_performance_state call after getting Rajendra's work in.
>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> But that said, do you really need this?
>>>>>>>>>
>>>>>>>>> The requirement for driving MMCX to LOW_SVS on SM8250 (and NOM on
>>>>>>>>> SM8150/SC8180x) seems to only come from the fact that you push MDP_CLK
>>>>>>>>> to 460MHz in &mdss.
>>>>>>>>>
>>>>>>>>> But then in &mdss_mdp you do the same using an opp-table based on the
>>>>>>>>> actual MDP_CLK, which per its power-domains will scale MMCX accordingly.
>>>>>>>>
>>>>>>>> MDSS and DSI would bump up MMCX performance state requirements on
>>>>>>>> their own, depending on the frequency being selected.
>>>>>>>>
>>>>>>>
>>>>>>> Right, but as I copied things from the sm8250.dtsi to come up with
>>>>>>> sm8150/sc8180x.dtsi I concluded that as soon as the assigned-clockrate
>>>>>>> in &mdss kicks in I need the performance state to be at NOM.
>>>>>>>
>>>>>>> So keeping the assigned-clockrate in &mdss means that MMCX will never go
>>>>>>> below NOM.
>>>>>>
>>>>>> No, because once MDP is fully running, it will lower the clock frequency:
>>>>>>
>>>>>> # grep mdp_clk /sys/kernel/debug/clk/clk_summary
>>>>>>             disp_cc_mdss_mdp_clk_src       1        1        0
>>>>>> 150000000          0     0  50000         ?
>>>>>>                disp_cc_mdss_mdp_clk       2        2        0
>>>>>> 150000000          0     0  50000         Y
>>>>>>
>>>>>
>>>>> But won't that just lower the performance state requested by the
>>>>> &mdss_mdp, while the &mdss still votes for NOM - with the outcome being
>>>>> that we maintain NOM even if the clock goes down?
>>>>
>>>> &mdss doesn't vote on performance state. At least it does not on
>>>> msm/msm-next which I have at hand right now.
>>>> &mdss toggles mdss_gdsc, but does not assign any performance state.
>>>>
>>>
>>> Right, but per the upstream implementation, enabling MDSS_GDSC could in
>>> itself fail, because unless something else has driven up the performance
>>> state the enable that trickles up won't actually turn on the supply.
>>>
>>>> On the other hand &mdss_mdp and &dsi0 clearly vote on mmcx's performance state.
>>>>
>>>
>>> Right, but it does so as part of its clock scaling, so this makes
>>> perfect sense to me.
>>>
>>>>>
>>>>>>>
>>>>>>>>> So wouldn't it be sufficient to ensure that MDSS_GDSC is parented by
>>>>>>>>> MMCX and then use opp-tables associated with the devices that scales the
>>>>>>>>> clock and thereby actually carries the "required-opps".
>>>>>>>>
>>>>>>>> Actually no. I set the performance state in the qcom_cc_map, so that
>>>>>>>> further register access is possible. Initially I was doing this in the
>>>>>>>> qcom_cc_really_probe() and it was already too late.
>>>>>>>> Just to remind: this patchset is not about MDSS_GDSC being parented by
>>>>>>>> MMCX, it is about dispcc/videocc registers being gated with MMCX.
>>>>>>>>
>>>>>>>
>>>>>>> So you're saying that just enabling MMCX isn't enough to touch the
>>>>>>> dispcc/videocc registers? If that's the case it seems like MMCX's
>>>>>>> definition of "on" needs to be adjusted - because just specifying MMCX
>>>>>>> as the power-domain for dispcc/videocc and enabling pm_runtime should
>>>>>>> ensure that MMCX is enabled when the clock registers are accessed (I
>>>>>>> don't see anything like that for the GDSC part though).
>>>>>>
>>>>>> No, it is not enough. If I comment out the set_performance_state call,
>>>>>> the board reboots.
>>>>>>
>>>>>> However I can set the opps as low as RET and register access will work.
>>>>>> I'll run more experiments and if everything works as expected, I can
>>>>>> use retention or min_svs level in the next iteration.
>>>>>> Just note that downstream specifies low_svs as minimum voltage level
>>>>>> for MMCX regulator.
>>>>>>
>>>>>
>>>>> It doesn't make sense to me that a lone power_on on the power-domain
>>>>> wouldn't give us enough juice to poke the registers.
>>>>>
>>>>> But digging into the rpmhpd implementation answers the question, simply
>>>>> invoking rpmhpd_power_on() is a nop, unless
>>>>> rpmhpd_set_performance_state() has previously been called, because
>>>>> pd->corner is 0. So this explains why enable isn't sufficient.
>>>>>
>>>>> Compare this with the rpmpd implementation that will send an
>>>>> enable request to the RPM in this case.
>>
>> Right, in case of RPMh, there was no separate 'enable' request which
>> could be sent, there was just a 'corner' request.
>>
>> I don't completely recall, but the reason to not send a 'default corner'
>> on enable was perhaps to keep the enable and set_performance orthogonal.
>>
>> However, given we then decided to send the lowest possible corner
>> in disable, it perhaps makes sense to send a 'lowest non-zero corner' on enable
>> as well.
>>
> 
> I was slightly worries that the change would dump cx and mx from
> whatever level the bootloader put it at down to LOW_SVS during boot.
> 
> But both rb3 and rb5 boots fine with this change, so I posted it here:

That seems to be a valid concern, perhaps this needs a little more wider testing on
more platforms to really make sure it isn;t causing some regression.

> https://lore.kernel.org/linux-arm-msm/20210703025449.2687201-1-bjorn.andersson@linaro.org/
> 
>>>>
>>>> Do you think that we should change that to:
>>>>
>>>> rpmhpd_aggregate_corner(pd, max(pd->corner, 1)) ?
>>>>
>>>> Or
>>>>
>>>> rpmhpd_aggregate_corner(pd, max(pd->corner, pd->levels[1])) ?
>>>>
>>>
>>> In rpmhpd_power_on() and rpmhpd_set_performance_state() we pass the
>>> index of the entry in pd->levels[] that we want, but in
>>> rpmhpd_power_off() we pass the value of pd->levels[0].
>>>
>>> So I would suggest dropping the if (pd->corner) and doing:
>>>
>>>     rpmhpd_aggregate_corner(pd, max(pd->corner, 1));
>>
>> So the index value represents the hlvl (0-15) that eventually gets sent to
>> rpmh, the pd->levels are the sparse vlvl values that come from the command
>> DB mappings.
>>
>> What seems sane is to sent the lowest non-zero vlvl. That in most cases
>> would be at index 1, but for some which do not support complete off,
>> it could be at index 0.
>>
> 
> I took this into consideration in above patch, keeping track of the
> first non-zero corner and using this when the domain is enabled.
> 
> Unfortunately, if the first entry would be say LOW_SVS power_off would
> request corner (hlvl) 64. So I fixed that in patch 1/2 in above series.

That was by design to make sure rpmh does not ignore your request to 'turn off'
a resource (since it really does not allow clients to dictate when to turn off)
and keep it at the same level as before.

> 
> Regards,
> Bjorn
> 
>>>
>>> And it seems both rb3 and rb5 still boots with this change (but I need
>>> to do some more testing to know for sure).
>>>
>>>>>
>>>>>>> I thought our problem you had was that you need to set a
>>>>>>> performance_state in order to clock up some of the clocks - e.g.
>>>>>>> MDP_CLK.
>>>>>>
>>>>>> No, even register access needs proper perf state.
>>>>>>
>>>>>
>>>>> Per above finding you're right, enabling a rpmhpd power-domain doesn't
>>>>> do anything. And I don't find this intuitive or even in line with the
>>>>> expectations of the api...
>>>>>
>>>>>
>>>>>
>>>>> A quick test booting rb3 and rb5 seems to indicate that it's possible to
>>>>> initialize pd->corner to 1 (to ensure that enable at least gives us the
>>>>> lowest level).
>>>>>
>>>>> set_performance_state(0) will however then result in voting for "off",
>>>>> rather than the lowest enabled level.
>>>>
>>>> Well, set_performance_state(0) means that "the device wouldn't
>>>> participate anymore to find the target performance state of the
>>>> genpd".
>>>
>>> I agree.
>>>
>>>> Strictly speaking it does not specify whether it is ok to turn
>>>> it off or not. (like the regulator with the voltage set to 0V).
>>>> But I'd also like to hear a comment from Stephen here.
>>>>
>>>
>>> Looking at other power-domains (e.g. gdsc and rpmpd) enabling the
>>> power-domain means it is no longer off and if you need some specific
>>> performance state you have to vote for that.
>>>
>>> So I'm also interested in hearing if there's any reasoning behind how
>>> this was written.
>>>
>>> Regards,
>>> Bjorn
>>>
>>
>> -- 
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
>> of Code Aurora Forum, hosted by The Linux Foundation

-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
of Code Aurora Forum, hosted by The Linux Foundation

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

* Re: [PATCH 1/6] dt-bindings: clock: qcom,dispcc-sm8x50: add mmcx power domain
  2021-07-01 19:26         ` Bjorn Andersson
@ 2021-07-06  7:23           ` Ulf Hansson
  2021-07-07  5:03             ` Bjorn Andersson
  0 siblings, 1 reply; 25+ messages in thread
From: Ulf Hansson @ 2021-07-06  7:23 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Dmitry Baryshkov, Andy Gross, Rob Herring, Stephen Boyd,
	Taniya Das, Jonathan Marek, Michael Turquette, linux-arm-msm,
	DTML, linux-clk, Bryan O'Donoghue, Mark Brown,
	Linux Kernel Mailing List

On Thu, 1 Jul 2021 at 21:26, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
>
> On Thu 01 Jul 11:58 CDT 2021, Ulf Hansson wrote:
>
> > On Thu, 1 Jul 2021 at 18:39, Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > On Thu, 1 Jul 2021 at 19:17, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > >
> > > > On Wed, 30 Jun 2021 at 15:31, Dmitry Baryshkov
> > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > >
> > > > > 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>
> > > > > ---
> > > > >  .../bindings/clock/qcom,dispcc-sm8x50.yaml    | 19 +++++++++++++++++++
> > > > >  1 file changed, 19 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
> > > > > index 0cdf53f41f84..48d86fb34fa7 100644
> > > > > --- a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
> > > > > +++ b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
> > > > > @@ -55,6 +55,16 @@ properties:
> > > > >    reg:
> > > > >      maxItems: 1
> > > > >
> > > > > +  power-domains:
> > > > > +    description:
> > > > > +      A phandle and PM domain specifier for the MMCX power domain.
> > > > > +    maxItems: 1
> > > > > +
> > > >
> > > > Should you perhaps state that this is a parent domain? Or it isn't?
> > > >
> > > > Related to this and because this is a power domain provider, you
> > > > should probably reference the common power-domain bindings somewhere
> > > > here. Along the lines of this:
> > > >
> > > > - $ref: power-domain.yaml#
> > > >
> > > > As an example, you could have a look at
> > > > Documentation/devicetree/bindings/power/pd-samsung.yaml.
> > >
> > > I'll take a look.
> > >
> > > >
> > > > > +  required-opps:
> > > > > +    description:
> > > > > +      Performance state to use for MMCX to enable register access.
> > > > > +    maxItems: 1
> > > >
> > > > According to the previous discussions, I was under the assumption that
> > > > this property belongs to a consumer node rather than in the provider
> > > > node, no?
> > >
> > > It is both a consumer and a provider. It consumes SM8250_MMCX from
> > > rpmhpd and provides MMSC_GDSC.
> >
> > That sounds a bit weird to me.
> >
>
> dispcc is a hardware block powered by MMCX, so it is a consumer of it
> and needs to control MMCX.

Right, that sounds reasonable.

>
> > In my view and per the common power domain bindings (as pointed to
> > above): If a power domain provider is a consumer of another power
> > domain, that per definition means that there is a parent domain
> > specified.
> >
>
> And in addition to needing MMCX to access the dispcc, the exposed
> power-domain "MDSS_GDSC" is powered by the same MMCX and as such
> MDSS_GDSC should be a subdomain of MMCX.

What do you mean by "exposed"? It sounds like you are saying that
"MDSS_GDSC" is an artificial power domain, no?

If that's the case, more exactly, why is it like this?

My apologies if I bother you with details, but as a maintainer of
genpd, it is very useful to me to have the complete picture.

>
>
> But what I was trying to say yesterday is that the power-domain property
> should be sufficient and that we shouldn't need to drive MMCX to a
> particular performance_state in order to access the registers.
>
> Then as clients make votes on clock rates that requires higher
> performance_state, they would describe this in their opp-tables etc.
>
>
> But without any performance_state requests, pd->corner will in
> rpmhpd_power_on() be 0 and as such powering on the power-domain won't
> actually do anything. Similarly dev_pm_genpd_set_performance_state(dev,
> 0) on an active power-domain from rpmhpd will turn it off.

Yes, I noticed the patches you posted. Thanks for helping out here!

>
>
> So the reason why Dmitry is adding the required-opps to the binding is
> to get rpmhpd to actually tell the hardware to turn on the power domain.
> And I don't think this is in accordance with the framework's
> expectations.

I agree!

>
> Regards,
> Bjorn

Kind regards
Uffe

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

* Re: [PATCH 1/6] dt-bindings: clock: qcom,dispcc-sm8x50: add mmcx power domain
  2021-07-06  7:23           ` Ulf Hansson
@ 2021-07-07  5:03             ` Bjorn Andersson
  0 siblings, 0 replies; 25+ messages in thread
From: Bjorn Andersson @ 2021-07-07  5:03 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Dmitry Baryshkov, Andy Gross, Rob Herring, Stephen Boyd,
	Taniya Das, Jonathan Marek, Michael Turquette, linux-arm-msm,
	DTML, linux-clk, Bryan O'Donoghue, Mark Brown,
	Linux Kernel Mailing List

On Tue 06 Jul 02:23 CDT 2021, Ulf Hansson wrote:

> On Thu, 1 Jul 2021 at 21:26, Bjorn Andersson <bjorn.andersson@linaro.org> wrote:
> >
> > On Thu 01 Jul 11:58 CDT 2021, Ulf Hansson wrote:
> >
> > > On Thu, 1 Jul 2021 at 18:39, Dmitry Baryshkov
> > > <dmitry.baryshkov@linaro.org> wrote:
> > > >
> > > > On Thu, 1 Jul 2021 at 19:17, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > > > >
> > > > > On Wed, 30 Jun 2021 at 15:31, Dmitry Baryshkov
> > > > > <dmitry.baryshkov@linaro.org> wrote:
> > > > > >
> > > > > > 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>
> > > > > > ---
> > > > > >  .../bindings/clock/qcom,dispcc-sm8x50.yaml    | 19 +++++++++++++++++++
> > > > > >  1 file changed, 19 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
> > > > > > index 0cdf53f41f84..48d86fb34fa7 100644
> > > > > > --- a/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/clock/qcom,dispcc-sm8x50.yaml
> > > > > > @@ -55,6 +55,16 @@ properties:
> > > > > >    reg:
> > > > > >      maxItems: 1
> > > > > >
> > > > > > +  power-domains:
> > > > > > +    description:
> > > > > > +      A phandle and PM domain specifier for the MMCX power domain.
> > > > > > +    maxItems: 1
> > > > > > +
> > > > >
> > > > > Should you perhaps state that this is a parent domain? Or it isn't?
> > > > >
> > > > > Related to this and because this is a power domain provider, you
> > > > > should probably reference the common power-domain bindings somewhere
> > > > > here. Along the lines of this:
> > > > >
> > > > > - $ref: power-domain.yaml#
> > > > >
> > > > > As an example, you could have a look at
> > > > > Documentation/devicetree/bindings/power/pd-samsung.yaml.
> > > >
> > > > I'll take a look.
> > > >
> > > > >
> > > > > > +  required-opps:
> > > > > > +    description:
> > > > > > +      Performance state to use for MMCX to enable register access.
> > > > > > +    maxItems: 1
> > > > >
> > > > > According to the previous discussions, I was under the assumption that
> > > > > this property belongs to a consumer node rather than in the provider
> > > > > node, no?
> > > >
> > > > It is both a consumer and a provider. It consumes SM8250_MMCX from
> > > > rpmhpd and provides MMSC_GDSC.
> > >
> > > That sounds a bit weird to me.
> > >
> >
> > dispcc is a hardware block powered by MMCX, so it is a consumer of it
> > and needs to control MMCX.
> 
> Right, that sounds reasonable.
> 
> >
> > > In my view and per the common power domain bindings (as pointed to
> > > above): If a power domain provider is a consumer of another power
> > > domain, that per definition means that there is a parent domain
> > > specified.
> > >
> >
> > And in addition to needing MMCX to access the dispcc, the exposed
> > power-domain "MDSS_GDSC" is powered by the same MMCX and as such
> > MDSS_GDSC should be a subdomain of MMCX.
> 
> What do you mean by "exposed"? It sounds like you are saying that
> "MDSS_GDSC" is an artificial power domain, no?
> 
> If that's the case, more exactly, why is it like this?
> 
> My apologies if I bother you with details, but as a maintainer of
> genpd, it is very useful to me to have the complete picture.
> 

The display hardware blocks are powered by the MDSS_GDSC power-domain,
which is a subdomain to the MMCX power domain.

MDSS_GDSC is controlled by registers in the display clock controller
block, which is also powered by the MMCX power domain.

Lastly the MMCX power domain is controlled through the RPMh, using the
rpmhpd driver.



As such, specifying MMCX as the power-domain for the dispcc block and
making the dispcc driver use that same power-domain as parent for the
MDSS_GDSC seems to accurately depict these relationships.

Regards,
Bjorn

> >
> >
> > But what I was trying to say yesterday is that the power-domain property
> > should be sufficient and that we shouldn't need to drive MMCX to a
> > particular performance_state in order to access the registers.
> >
> > Then as clients make votes on clock rates that requires higher
> > performance_state, they would describe this in their opp-tables etc.
> >
> >
> > But without any performance_state requests, pd->corner will in
> > rpmhpd_power_on() be 0 and as such powering on the power-domain won't
> > actually do anything. Similarly dev_pm_genpd_set_performance_state(dev,
> > 0) on an active power-domain from rpmhpd will turn it off.
> 
> Yes, I noticed the patches you posted. Thanks for helping out here!
> 
> >
> >
> > So the reason why Dmitry is adding the required-opps to the binding is
> > to get rpmhpd to actually tell the hardware to turn on the power domain.
> > And I don't think this is in accordance with the framework's
> > expectations.
> 
> I agree!
> 
> >
> > Regards,
> > Bjorn
> 
> Kind regards
> Uffe

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

end of thread, other threads:[~2021-07-07  5:04 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-30 13:31 [PATCH 0/6] clk: qcom: use power-domain for sm8250's clock controllers Dmitry Baryshkov
2021-06-30 13:31 ` [PATCH 1/6] dt-bindings: clock: qcom,dispcc-sm8x50: add mmcx power domain Dmitry Baryshkov
2021-07-01 16:16   ` Ulf Hansson
2021-07-01 16:39     ` Dmitry Baryshkov
2021-07-01 16:58       ` Ulf Hansson
2021-07-01 19:26         ` Bjorn Andersson
2021-07-06  7:23           ` Ulf Hansson
2021-07-07  5:03             ` Bjorn Andersson
2021-06-30 13:31 ` [PATCH 2/6] dt-bindings: clock: qcom,videocc: " Dmitry Baryshkov
2021-06-30 13:31 ` [PATCH 3/6] clk: qcom: gdsc: enable optional power domain support Dmitry Baryshkov
2021-06-30 15:00   ` Bjorn Andersson
2021-06-30 15:47     ` Dmitry Baryshkov
2021-06-30 17:11       ` Bjorn Andersson
2021-06-30 20:29         ` Dmitry Baryshkov
2021-07-01  4:22           ` Bjorn Andersson
2021-07-01 20:12             ` Dmitry Baryshkov
2021-07-01 20:57               ` Bjorn Andersson
2021-07-02  7:35                 ` Rajendra Nayak
2021-07-03  3:20                   ` Bjorn Andersson
2021-07-05  4:33                     ` Rajendra Nayak
2021-06-30 13:31 ` [PATCH 4/6] arm64: dts: qcom: sm8250: remove mmcx regulator Dmitry Baryshkov
2021-06-30 13:31 ` [PATCH 5/6] clk: qcom: dispcc-sm8250: stop using " Dmitry Baryshkov
2021-06-30 17:12   ` Bjorn Andersson
2021-06-30 13:31 ` [PATCH 6/6] clk: qcom: videocc-sm8250: " Dmitry Baryshkov
2021-06-30 17:13   ` Bjorn Andersson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.