linux-arm-msm.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] clk: qcom: gdsc: Handle supply regulators
@ 2020-03-19  5:38 Bjorn Andersson
  2020-03-19  5:38 ` [PATCH 1/4] clk: qcom: gdsc: Handle GDSC regulator supplies Bjorn Andersson
                   ` (3 more replies)
  0 siblings, 4 replies; 14+ messages in thread
From: Bjorn Andersson @ 2020-03-19  5:38 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Michael Turquette, Stephen Boyd,
	Rob Herring
  Cc: linux-arm-msm, linux-clk, devicetree, linux-kernel, Rob Clark

Handle supply regulators for GDSCs to allow probe deferral when regulators are
not yet present/enabled and to allow the GDSC to enable/disable dependencies as
needed.

Bjorn Andersson (3):
  clk: qcom: gdsc: Handle GDSC regulator supplies
  clk: qcom: mmcc-msm8996: Properly describe GPU_GX gdsc
  arm64: dts: qcom: msm8996: Make GPU node control GPU_GX GDSC

Rajendra Nayak (1):
  arm64: dts: qcom: db820c: Add s2 regulator in pmi8994

 .../devicetree/bindings/clock/qcom,mmcc.yaml  |  4 ++++
 arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi  | 14 +++++++++++
 arch/arm64/boot/dts/qcom/msm8996.dtsi         |  2 +-
 arch/arm64/boot/dts/qcom/pmi8994.dtsi         |  6 +++++
 drivers/clk/qcom/gdsc.c                       | 24 +++++++++++++++++++
 drivers/clk/qcom/gdsc.h                       |  4 ++++
 drivers/clk/qcom/mmcc-msm8996.c               |  2 ++
 7 files changed, 55 insertions(+), 1 deletion(-)

-- 
2.24.0


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

* [PATCH 1/4] clk: qcom: gdsc: Handle GDSC regulator supplies
  2020-03-19  5:38 [PATCH 0/4] clk: qcom: gdsc: Handle supply regulators Bjorn Andersson
@ 2020-03-19  5:38 ` Bjorn Andersson
  2020-03-20 23:31   ` Stephen Boyd
  2020-03-31  5:35   ` Taniya Das
  2020-03-19  5:39 ` [PATCH 2/4] clk: qcom: mmcc-msm8996: Properly describe GPU_GX gdsc Bjorn Andersson
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 14+ messages in thread
From: Bjorn Andersson @ 2020-03-19  5:38 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd
  Cc: Andy Gross, Rob Herring, linux-arm-msm, linux-clk, devicetree,
	linux-kernel, Rob Clark

Certain GDSCs, such as the GPU_GX on MSM8996, requires that the upstream
regulator supply is powered in order to be turned on.

It's not guaranteed that the bootloader will leave these supplies on and
the driver core will attempt to enable any GDSCs before allowing the
individual drivers to probe defer on the PMIC regulator driver not yet
being present.

So the gdsc driver needs to be made aware of supplying regulators and
probe defer on their absence, and it needs to enable and disable the
regulator accordingly.

Voltage adjustments of the supplying regulator are deferred to the
client drivers themselves.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/clk/qcom/gdsc.c | 24 ++++++++++++++++++++++++
 drivers/clk/qcom/gdsc.h |  4 ++++
 2 files changed, 28 insertions(+)

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index a250f59708d8..3528789cc9d0 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -13,6 +13,7 @@
 #include <linux/regmap.h>
 #include <linux/reset-controller.h>
 #include <linux/slab.h>
+#include <linux/regulator/consumer.h>
 #include "gdsc.h"
 
 #define PWR_ON_MASK		BIT(31)
@@ -112,6 +113,12 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
 	int ret;
 	u32 val = (status == GDSC_ON) ? 0 : SW_COLLAPSE_MASK;
 
+	if (status == GDSC_ON && sc->rsupply) {
+		ret = regulator_enable(sc->rsupply);
+		if (ret < 0)
+			return ret;
+	}
+
 	ret = regmap_update_bits(sc->regmap, sc->gdscr, SW_COLLAPSE_MASK, val);
 	if (ret)
 		return ret;
@@ -143,6 +150,13 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
 
 	ret = gdsc_poll_status(sc, status);
 	WARN(ret, "%s status stuck at 'o%s'", sc->pd.name, status ? "ff" : "n");
+
+	if (!ret && status == GDSC_OFF && sc->rsupply) {
+		ret = regulator_disable(sc->rsupply);
+		if (ret < 0)
+			return ret;
+	}
+
 	return ret;
 }
 
@@ -371,6 +385,16 @@ int gdsc_register(struct gdsc_desc *desc,
 	if (!data->domains)
 		return -ENOMEM;
 
+	/* Resolve any regulator supplies */
+	for (i = 0; i < num; i++) {
+		if (!scs[i] || !scs[i]->supply)
+			continue;
+
+		scs[i]->rsupply = devm_regulator_get(dev, scs[i]->supply);
+		if (IS_ERR(scs[i]->rsupply))
+			return PTR_ERR(scs[i]->rsupply);
+	}
+
 	data->num_domains = num;
 	for (i = 0; i < num; i++) {
 		if (!scs[i])
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index 64cdc8cf0d4d..c36fc26dcdff 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -10,6 +10,7 @@
 #include <linux/pm_domain.h>
 
 struct regmap;
+struct regulator;
 struct reset_controller_dev;
 
 /**
@@ -52,6 +53,9 @@ struct gdsc {
 	struct reset_controller_dev	*rcdev;
 	unsigned int			*resets;
 	unsigned int			reset_count;
+
+	const char 			*supply;
+	struct regulator		*rsupply;
 };
 
 struct gdsc_desc {
-- 
2.24.0


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

* [PATCH 2/4] clk: qcom: mmcc-msm8996: Properly describe GPU_GX gdsc
  2020-03-19  5:38 [PATCH 0/4] clk: qcom: gdsc: Handle supply regulators Bjorn Andersson
  2020-03-19  5:38 ` [PATCH 1/4] clk: qcom: gdsc: Handle GDSC regulator supplies Bjorn Andersson
@ 2020-03-19  5:39 ` Bjorn Andersson
  2020-03-20 23:31   ` Stephen Boyd
  2020-03-19  5:39 ` [PATCH 3/4] arm64: dts: qcom: db820c: Add s2 regulator in pmi8994 Bjorn Andersson
  2020-03-19  5:39 ` [PATCH 4/4] arm64: dts: qcom: msm8996: Make GPU node control GPU_GX GDSC Bjorn Andersson
  3 siblings, 1 reply; 14+ messages in thread
From: Bjorn Andersson @ 2020-03-19  5:39 UTC (permalink / raw)
  To: Michael Turquette, Stephen Boyd, Rob Herring
  Cc: Andy Gross, linux-arm-msm, linux-clk, devicetree, linux-kernel,
	Rob Clark

The GPU_GX GDSC depends on both GPU GDSC being enabled and that the
VDD_GX rail is powered, so update the description of the node to cover
these requirements.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 Documentation/devicetree/bindings/clock/qcom,mmcc.yaml | 4 ++++
 drivers/clk/qcom/mmcc-msm8996.c                        | 2 ++
 2 files changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml
index 85518494ce43..65d9aa790581 100644
--- a/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml
+++ b/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml
@@ -67,6 +67,10 @@ properties:
     description:
        Protected clock specifier list as per common clock binding
 
+  vdd_gfx-supply:
+    description:
+      Regulator supply for the GPU_GX GDSC
+
 required:
   - compatible
   - reg
diff --git a/drivers/clk/qcom/mmcc-msm8996.c b/drivers/clk/qcom/mmcc-msm8996.c
index 6c7592ddf8bb..fd43a35db13b 100644
--- a/drivers/clk/qcom/mmcc-msm8996.c
+++ b/drivers/clk/qcom/mmcc-msm8996.c
@@ -3064,7 +3064,9 @@ static struct gdsc gpu_gx_gdsc = {
 		.name = "gpu_gx",
 	},
 	.pwrsts = PWRSTS_OFF_ON,
+	.parent = &gpu_gdsc.pd,
 	.flags = CLAMP_IO,
+	.supply = "vdd_gfx",
 };
 
 static struct clk_regmap *mmcc_msm8996_clocks[] = {
-- 
2.24.0


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

* [PATCH 3/4] arm64: dts: qcom: db820c: Add s2 regulator in pmi8994
  2020-03-19  5:38 [PATCH 0/4] clk: qcom: gdsc: Handle supply regulators Bjorn Andersson
  2020-03-19  5:38 ` [PATCH 1/4] clk: qcom: gdsc: Handle GDSC regulator supplies Bjorn Andersson
  2020-03-19  5:39 ` [PATCH 2/4] clk: qcom: mmcc-msm8996: Properly describe GPU_GX gdsc Bjorn Andersson
@ 2020-03-19  5:39 ` Bjorn Andersson
  2020-03-20 23:33   ` Stephen Boyd
  2020-03-19  5:39 ` [PATCH 4/4] arm64: dts: qcom: msm8996: Make GPU node control GPU_GX GDSC Bjorn Andersson
  3 siblings, 1 reply; 14+ messages in thread
From: Bjorn Andersson @ 2020-03-19  5:39 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring
  Cc: Michael Turquette, Stephen Boyd, linux-arm-msm, linux-clk,
	devicetree, linux-kernel, Rob Clark

From: Rajendra Nayak <rnayak@codeaurora.org>

Add the SPMI regulator node in the PMI8994, use it to give us VDD_GX
at a fixed max nominal voltage for the db820c and specify this as supply
for the MMSS GPU_GX GDSC.

With the introduction of CPR support the range for VDD_GX should be
expanded.

Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
[bjorn: Split between pmi8994 and db820c, changed voltage, rewrote commit message]
Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi | 14 ++++++++++++++
 arch/arm64/boot/dts/qcom/pmi8994.dtsi        |  6 ++++++
 2 files changed, 20 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
index 4692b7ad16b7..075cebaec3f3 100644
--- a/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
+++ b/arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi
@@ -251,6 +251,10 @@ &mdss {
 	status = "okay";
 };
 
+&mmcc {
+	vdd_gfx-supply = <&vdd_gfx>;
+};
+
 &msmgpio {
 	gpio-line-names =
 		"[SPI0_DOUT]", /* GPIO_0, BLSP1_SPI_MOSI, LSEC pin 14 */
@@ -688,6 +692,16 @@ pinconf {
 	};
 };
 
+
+&pmi8994_spmi_regulators {
+	vdd_gfx: s2@1700 {
+		reg = <0x1700 0x100>;
+		regulator-name = "VDD_GFX";
+		regulator-min-microvolt = <980000>;
+		regulator-max-microvolt = <980000>;
+	};
+};
+
 &rpm_requests {
 	pm8994-regulators {
 		compatible = "qcom,rpm-pm8994-regulators";
diff --git a/arch/arm64/boot/dts/qcom/pmi8994.dtsi b/arch/arm64/boot/dts/qcom/pmi8994.dtsi
index 21e05215abe4..e5ed28ab9b2d 100644
--- a/arch/arm64/boot/dts/qcom/pmi8994.dtsi
+++ b/arch/arm64/boot/dts/qcom/pmi8994.dtsi
@@ -26,5 +26,11 @@ pmic@3 {
 		reg = <0x3 SPMI_USID>;
 		#address-cells = <1>;
 		#size-cells = <0>;
+
+		pmi8994_spmi_regulators: regulators {
+			compatible = "qcom,pmi8994-regulators";
+			#address-cells = <1>;
+			#size-cells = <1>;
+		};
 	};
 };
-- 
2.24.0


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

* [PATCH 4/4] arm64: dts: qcom: msm8996: Make GPU node control GPU_GX GDSC
  2020-03-19  5:38 [PATCH 0/4] clk: qcom: gdsc: Handle supply regulators Bjorn Andersson
                   ` (2 preceding siblings ...)
  2020-03-19  5:39 ` [PATCH 3/4] arm64: dts: qcom: db820c: Add s2 regulator in pmi8994 Bjorn Andersson
@ 2020-03-19  5:39 ` Bjorn Andersson
  3 siblings, 0 replies; 14+ messages in thread
From: Bjorn Andersson @ 2020-03-19  5:39 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring
  Cc: Michael Turquette, Stephen Boyd, linux-arm-msm, linux-clk,
	devicetree, linux-kernel, Rob Clark

Presumably the GPU node needs to control both the GPU and GPU GX power
domains, but given that GPU GX now depends on the GPU GDSC both can
effectively be controlled by controlling GPU GX. So use this instead.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 arch/arm64/boot/dts/qcom/msm8996.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/msm8996.dtsi b/arch/arm64/boot/dts/qcom/msm8996.dtsi
index 14827adebd94..f29f45e9737b 100644
--- a/arch/arm64/boot/dts/qcom/msm8996.dtsi
+++ b/arch/arm64/boot/dts/qcom/msm8996.dtsi
@@ -639,7 +639,7 @@ gpu@b00000 {
 				"mem",
 				"mem_iface";
 
-			power-domains = <&mmcc GPU_GDSC>;
+			power-domains = <&mmcc GPU_GX_GDSC>;
 			iommus = <&adreno_smmu 0>;
 
 			nvmem-cells = <&gpu_speed_bin>;
-- 
2.24.0


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

* Re: [PATCH 1/4] clk: qcom: gdsc: Handle GDSC regulator supplies
  2020-03-19  5:38 ` [PATCH 1/4] clk: qcom: gdsc: Handle GDSC regulator supplies Bjorn Andersson
@ 2020-03-20 23:31   ` Stephen Boyd
  2020-03-31  5:35   ` Taniya Das
  1 sibling, 0 replies; 14+ messages in thread
From: Stephen Boyd @ 2020-03-20 23:31 UTC (permalink / raw)
  To: Bjorn Andersson, Michael Turquette
  Cc: Andy Gross, Rob Herring, linux-arm-msm, linux-clk, devicetree,
	linux-kernel, Rob Clark

Quoting Bjorn Andersson (2020-03-18 22:38:59)
> Certain GDSCs, such as the GPU_GX on MSM8996, requires that the upstream
> regulator supply is powered in order to be turned on.
> 
> It's not guaranteed that the bootloader will leave these supplies on and
> the driver core will attempt to enable any GDSCs before allowing the
> individual drivers to probe defer on the PMIC regulator driver not yet
> being present.
> 
> So the gdsc driver needs to be made aware of supplying regulators and
> probe defer on their absence, and it needs to enable and disable the
> regulator accordingly.
> 
> Voltage adjustments of the supplying regulator are deferred to the
> client drivers themselves.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---

Ok. Looks mostly fine to me.

>  drivers/clk/qcom/gdsc.c | 24 ++++++++++++++++++++++++
>  drivers/clk/qcom/gdsc.h |  4 ++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index a250f59708d8..3528789cc9d0 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -13,6 +13,7 @@
>  #include <linux/regmap.h>
>  #include <linux/reset-controller.h>
>  #include <linux/slab.h>
> +#include <linux/regulator/consumer.h>

Alphabetical?

>  #include "gdsc.h"
>  
>  #define PWR_ON_MASK            BIT(31)
> @@ -371,6 +385,16 @@ int gdsc_register(struct gdsc_desc *desc,
>         if (!data->domains)
>                 return -ENOMEM;
>  
> +       /* Resolve any regulator supplies */

Drop the comment please, it's just saying what the code is doing.

> +       for (i = 0; i < num; i++) {
> +               if (!scs[i] || !scs[i]->supply)
> +                       continue;
> +
> +               scs[i]->rsupply = devm_regulator_get(dev, scs[i]->supply);
> +               if (IS_ERR(scs[i]->rsupply))
> +                       return PTR_ERR(scs[i]->rsupply);
> +       }
> +
>         data->num_domains = num;
>         for (i = 0; i < num; i++) {
>                 if (!scs[i])

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

* Re: [PATCH 2/4] clk: qcom: mmcc-msm8996: Properly describe GPU_GX gdsc
  2020-03-19  5:39 ` [PATCH 2/4] clk: qcom: mmcc-msm8996: Properly describe GPU_GX gdsc Bjorn Andersson
@ 2020-03-20 23:31   ` Stephen Boyd
  2020-03-21  5:16     ` Bjorn Andersson
  0 siblings, 1 reply; 14+ messages in thread
From: Stephen Boyd @ 2020-03-20 23:31 UTC (permalink / raw)
  To: Bjorn Andersson, Michael Turquette, Rob Herring
  Cc: Andy Gross, linux-arm-msm, linux-clk, devicetree, linux-kernel,
	Rob Clark

Quoting Bjorn Andersson (2020-03-18 22:39:00)
> diff --git a/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml
> index 85518494ce43..65d9aa790581 100644
> --- a/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml
> +++ b/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml
> @@ -67,6 +67,10 @@ properties:
>      description:
>         Protected clock specifier list as per common clock binding
>  
> +  vdd_gfx-supply:

Why not vdd-gfx-supply? What's with the underscore?

> +    description:
> +      Regulator supply for the GPU_GX GDSC
> +
>  required:
>    - compatible
>    - reg

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

* Re: [PATCH 3/4] arm64: dts: qcom: db820c: Add s2 regulator in pmi8994
  2020-03-19  5:39 ` [PATCH 3/4] arm64: dts: qcom: db820c: Add s2 regulator in pmi8994 Bjorn Andersson
@ 2020-03-20 23:33   ` Stephen Boyd
  2020-03-21  5:19     ` Bjorn Andersson
  0 siblings, 1 reply; 14+ messages in thread
From: Stephen Boyd @ 2020-03-20 23:33 UTC (permalink / raw)
  To: Andy Gross, Bjorn Andersson, Rob Herring
  Cc: Michael Turquette, linux-arm-msm, linux-clk, devicetree,
	linux-kernel, Rob Clark

Quoting Bjorn Andersson (2020-03-18 22:39:01)
> From: Rajendra Nayak <rnayak@codeaurora.org>
> 
> Add the SPMI regulator node in the PMI8994, use it to give us VDD_GX
> at a fixed max nominal voltage for the db820c and specify this as supply
> for the MMSS GPU_GX GDSC.
> 
> With the introduction of CPR support the range for VDD_GX should be
> expanded.
> 
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> [bjorn: Split between pmi8994 and db820c, changed voltage, rewrote commit message]
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---

These two dts patches don't need to go through clk tree right? And the
first patch can be applied and regulator core will just return us a
dummy supply so it's safe to apply now?

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

* Re: [PATCH 2/4] clk: qcom: mmcc-msm8996: Properly describe GPU_GX gdsc
  2020-03-20 23:31   ` Stephen Boyd
@ 2020-03-21  5:16     ` Bjorn Andersson
  2020-03-21 18:43       ` Stephen Boyd
  0 siblings, 1 reply; 14+ messages in thread
From: Bjorn Andersson @ 2020-03-21  5:16 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Michael Turquette, Rob Herring, Andy Gross, linux-arm-msm,
	linux-clk, devicetree, linux-kernel, Rob Clark

On Fri 20 Mar 16:31 PDT 2020, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2020-03-18 22:39:00)
> > diff --git a/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml
> > index 85518494ce43..65d9aa790581 100644
> > --- a/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml
> > +++ b/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml
> > @@ -67,6 +67,10 @@ properties:
> >      description:
> >         Protected clock specifier list as per common clock binding
> >  
> > +  vdd_gfx-supply:
> 
> Why not vdd-gfx-supply? What's with the underscore?
> 

The pad is named "VDD_GFX" in the datasheet and the schematics. I see
that we've started y/_/-/ in some of the newly added bindings, would you
prefer I follow this?

Regards,
Bjorn

> > +    description:
> > +      Regulator supply for the GPU_GX GDSC
> > +
> >  required:
> >    - compatible
> >    - reg

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

* Re: [PATCH 3/4] arm64: dts: qcom: db820c: Add s2 regulator in pmi8994
  2020-03-20 23:33   ` Stephen Boyd
@ 2020-03-21  5:19     ` Bjorn Andersson
  0 siblings, 0 replies; 14+ messages in thread
From: Bjorn Andersson @ 2020-03-21  5:19 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Andy Gross, Rob Herring, Michael Turquette, linux-arm-msm,
	linux-clk, devicetree, linux-kernel, Rob Clark

On Fri 20 Mar 16:33 PDT 2020, Stephen Boyd wrote:

> Quoting Bjorn Andersson (2020-03-18 22:39:01)
> > From: Rajendra Nayak <rnayak@codeaurora.org>
> > 
> > Add the SPMI regulator node in the PMI8994, use it to give us VDD_GX
> > at a fixed max nominal voltage for the db820c and specify this as supply
> > for the MMSS GPU_GX GDSC.
> > 
> > With the introduction of CPR support the range for VDD_GX should be
> > expanded.
> > 
> > Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> > [bjorn: Split between pmi8994 and db820c, changed voltage, rewrote commit message]
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> 
> These two dts patches don't need to go through clk tree right? And the
> first patch can be applied and regulator core will just return us a
> dummy supply so it's safe to apply now?

Right, the two pairs can be merged completely independently; we just
need to agree on the _ vs -

Regards,
Bjorn

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

* Re: [PATCH 2/4] clk: qcom: mmcc-msm8996: Properly describe GPU_GX gdsc
  2020-03-21  5:16     ` Bjorn Andersson
@ 2020-03-21 18:43       ` Stephen Boyd
  2020-03-30 23:10         ` Rob Herring
  0 siblings, 1 reply; 14+ messages in thread
From: Stephen Boyd @ 2020-03-21 18:43 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: Michael Turquette, Rob Herring, Andy Gross, linux-arm-msm,
	linux-clk, devicetree, linux-kernel, Rob Clark

Quoting Bjorn Andersson (2020-03-20 22:16:12)
> On Fri 20 Mar 16:31 PDT 2020, Stephen Boyd wrote:
> 
> > Quoting Bjorn Andersson (2020-03-18 22:39:00)
> > > diff --git a/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml
> > > index 85518494ce43..65d9aa790581 100644
> > > --- a/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml
> > > +++ b/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml
> > > @@ -67,6 +67,10 @@ properties:
> > >      description:
> > >         Protected clock specifier list as per common clock binding
> > >  
> > > +  vdd_gfx-supply:
> > 
> > Why not vdd-gfx-supply? What's with the underscore?
> > 
> 
> The pad is named "VDD_GFX" in the datasheet and the schematics. I see
> that we've started y/_/-/ in some of the newly added bindings, would you
> prefer I follow this?

If the datasheet has this then I guess it's fine. I'll wait for Rob to
ack.

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

* Re: [PATCH 2/4] clk: qcom: mmcc-msm8996: Properly describe GPU_GX gdsc
  2020-03-21 18:43       ` Stephen Boyd
@ 2020-03-30 23:10         ` Rob Herring
  0 siblings, 0 replies; 14+ messages in thread
From: Rob Herring @ 2020-03-30 23:10 UTC (permalink / raw)
  To: Stephen Boyd
  Cc: Bjorn Andersson, Michael Turquette, Andy Gross, linux-arm-msm,
	linux-clk, devicetree, linux-kernel, Rob Clark

On Sat, Mar 21, 2020 at 11:43:12AM -0700, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2020-03-20 22:16:12)
> > On Fri 20 Mar 16:31 PDT 2020, Stephen Boyd wrote:
> > 
> > > Quoting Bjorn Andersson (2020-03-18 22:39:00)
> > > > diff --git a/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml b/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml
> > > > index 85518494ce43..65d9aa790581 100644
> > > > --- a/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml
> > > > +++ b/Documentation/devicetree/bindings/clock/qcom,mmcc.yaml
> > > > @@ -67,6 +67,10 @@ properties:
> > > >      description:
> > > >         Protected clock specifier list as per common clock binding
> > > >  
> > > > +  vdd_gfx-supply:
> > > 
> > > Why not vdd-gfx-supply? What's with the underscore?
> > > 
> > 
> > The pad is named "VDD_GFX" in the datasheet and the schematics. I see
> > that we've started y/_/-/ in some of the newly added bindings, would you
> > prefer I follow this?
> 
> If the datasheet has this then I guess it's fine. I'll wait for Rob to
> ack.

vddgfx or vdd-gfx.

Rob


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

* Re: [PATCH 1/4] clk: qcom: gdsc: Handle GDSC regulator supplies
  2020-03-19  5:38 ` [PATCH 1/4] clk: qcom: gdsc: Handle GDSC regulator supplies Bjorn Andersson
  2020-03-20 23:31   ` Stephen Boyd
@ 2020-03-31  5:35   ` Taniya Das
  2020-03-31  6:08     ` Bjorn Andersson
  1 sibling, 1 reply; 14+ messages in thread
From: Taniya Das @ 2020-03-31  5:35 UTC (permalink / raw)
  To: Bjorn Andersson, Michael Turquette, Stephen Boyd
  Cc: Andy Gross, Rob Herring, linux-arm-msm, linux-clk, devicetree,
	linux-kernel, Rob Clark

Hi Stephen,

I think the upstream design always wanted the client/consumer to enable 
the GPU Rail and then turn ON the GDSC?

Why are we going ahead with adding the support of regulator in the GDSC 
driver?

On 3/19/2020 11:08 AM, Bjorn Andersson wrote:
> Certain GDSCs, such as the GPU_GX on MSM8996, requires that the upstream
> regulator supply is powered in order to be turned on.
> 
> It's not guaranteed that the bootloader will leave these supplies on and
> the driver core will attempt to enable any GDSCs before allowing the
> individual drivers to probe defer on the PMIC regulator driver not yet
> being present.
> 
> So the gdsc driver needs to be made aware of supplying regulators and
> probe defer on their absence, and it needs to enable and disable the
> regulator accordingly.
> 
> Voltage adjustments of the supplying regulator are deferred to the
> client drivers themselves.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>   drivers/clk/qcom/gdsc.c | 24 ++++++++++++++++++++++++
>   drivers/clk/qcom/gdsc.h |  4 ++++
>   2 files changed, 28 insertions(+)
> 
> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> index a250f59708d8..3528789cc9d0 100644
> --- a/drivers/clk/qcom/gdsc.c
> +++ b/drivers/clk/qcom/gdsc.c
> @@ -13,6 +13,7 @@
>   #include <linux/regmap.h>
>   #include <linux/reset-controller.h>
>   #include <linux/slab.h>
> +#include <linux/regulator/consumer.h>
>   #include "gdsc.h"
>   
>   #define PWR_ON_MASK		BIT(31)
> @@ -112,6 +113,12 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
>   	int ret;
>   	u32 val = (status == GDSC_ON) ? 0 : SW_COLLAPSE_MASK;
>   
> +	if (status == GDSC_ON && sc->rsupply) {
> +		ret = regulator_enable(sc->rsupply);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>   	ret = regmap_update_bits(sc->regmap, sc->gdscr, SW_COLLAPSE_MASK, val);
>   	if (ret)
>   		return ret;
> @@ -143,6 +150,13 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
>   
>   	ret = gdsc_poll_status(sc, status);
>   	WARN(ret, "%s status stuck at 'o%s'", sc->pd.name, status ? "ff" : "n");
> +
> +	if (!ret && status == GDSC_OFF && sc->rsupply) {
> +		ret = regulator_disable(sc->rsupply);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
>   	return ret;
>   }
>   
> @@ -371,6 +385,16 @@ int gdsc_register(struct gdsc_desc *desc,
>   	if (!data->domains)
>   		return -ENOMEM;
>   
> +	/* Resolve any regulator supplies */
> +	for (i = 0; i < num; i++) {
> +		if (!scs[i] || !scs[i]->supply)
> +			continue;
> +
> +		scs[i]->rsupply = devm_regulator_get(dev, scs[i]->supply);
> +		if (IS_ERR(scs[i]->rsupply))
> +			return PTR_ERR(scs[i]->rsupply);
> +	}
> +
>   	data->num_domains = num;
>   	for (i = 0; i < num; i++) {
>   		if (!scs[i])
> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> index 64cdc8cf0d4d..c36fc26dcdff 100644
> --- a/drivers/clk/qcom/gdsc.h
> +++ b/drivers/clk/qcom/gdsc.h
> @@ -10,6 +10,7 @@
>   #include <linux/pm_domain.h>
>   
>   struct regmap;
> +struct regulator;
>   struct reset_controller_dev;
>   
>   /**
> @@ -52,6 +53,9 @@ struct gdsc {
>   	struct reset_controller_dev	*rcdev;
>   	unsigned int			*resets;
>   	unsigned int			reset_count;
> +
> +	const char 			*supply;
> +	struct regulator		*rsupply;
>   };
>   
>   struct gdsc_desc {
> 

-- 
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] 14+ messages in thread

* Re: [PATCH 1/4] clk: qcom: gdsc: Handle GDSC regulator supplies
  2020-03-31  5:35   ` Taniya Das
@ 2020-03-31  6:08     ` Bjorn Andersson
  0 siblings, 0 replies; 14+ messages in thread
From: Bjorn Andersson @ 2020-03-31  6:08 UTC (permalink / raw)
  To: Taniya Das
  Cc: Michael Turquette, Stephen Boyd, Andy Gross, Rob Herring,
	linux-arm-msm, linux-clk, devicetree, linux-kernel, Rob Clark

On Mon 30 Mar 22:35 PDT 2020, Taniya Das wrote:

> Hi Stephen,
> 
> I think the upstream design always wanted the client/consumer to enable the
> GPU Rail and then turn ON the GDSC?
> 
> Why are we going ahead with adding the support of regulator in the GDSC
> driver?
> 

As I (partially) describe below the mdss driver on 8996 doesn't probe
because the GDSC fails to enable, because the upstream supply is not
enabled, so the mdss driver can't turn on the regulator needed by the
GDSC.

I don't see any other way to handle this than extending the gdsc
implementation, hence my proposal to change the design.
Suggestions/feedback are welcome though.

Regards,
Bjorn

> On 3/19/2020 11:08 AM, Bjorn Andersson wrote:
> > Certain GDSCs, such as the GPU_GX on MSM8996, requires that the upstream
> > regulator supply is powered in order to be turned on.
> > 
> > It's not guaranteed that the bootloader will leave these supplies on and
> > the driver core will attempt to enable any GDSCs before allowing the
> > individual drivers to probe defer on the PMIC regulator driver not yet
> > being present.
> > 
> > So the gdsc driver needs to be made aware of supplying regulators and
> > probe defer on their absence, and it needs to enable and disable the
> > regulator accordingly.
> > 
> > Voltage adjustments of the supplying regulator are deferred to the
> > client drivers themselves.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > ---
> >   drivers/clk/qcom/gdsc.c | 24 ++++++++++++++++++++++++
> >   drivers/clk/qcom/gdsc.h |  4 ++++
> >   2 files changed, 28 insertions(+)
> > 
> > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
> > index a250f59708d8..3528789cc9d0 100644
> > --- a/drivers/clk/qcom/gdsc.c
> > +++ b/drivers/clk/qcom/gdsc.c
> > @@ -13,6 +13,7 @@
> >   #include <linux/regmap.h>
> >   #include <linux/reset-controller.h>
> >   #include <linux/slab.h>
> > +#include <linux/regulator/consumer.h>
> >   #include "gdsc.h"
> >   #define PWR_ON_MASK		BIT(31)
> > @@ -112,6 +113,12 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
> >   	int ret;
> >   	u32 val = (status == GDSC_ON) ? 0 : SW_COLLAPSE_MASK;
> > +	if (status == GDSC_ON && sc->rsupply) {
> > +		ret = regulator_enable(sc->rsupply);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> >   	ret = regmap_update_bits(sc->regmap, sc->gdscr, SW_COLLAPSE_MASK, val);
> >   	if (ret)
> >   		return ret;
> > @@ -143,6 +150,13 @@ static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
> >   	ret = gdsc_poll_status(sc, status);
> >   	WARN(ret, "%s status stuck at 'o%s'", sc->pd.name, status ? "ff" : "n");
> > +
> > +	if (!ret && status == GDSC_OFF && sc->rsupply) {
> > +		ret = regulator_disable(sc->rsupply);
> > +		if (ret < 0)
> > +			return ret;
> > +	}
> > +
> >   	return ret;
> >   }
> > @@ -371,6 +385,16 @@ int gdsc_register(struct gdsc_desc *desc,
> >   	if (!data->domains)
> >   		return -ENOMEM;
> > +	/* Resolve any regulator supplies */
> > +	for (i = 0; i < num; i++) {
> > +		if (!scs[i] || !scs[i]->supply)
> > +			continue;
> > +
> > +		scs[i]->rsupply = devm_regulator_get(dev, scs[i]->supply);
> > +		if (IS_ERR(scs[i]->rsupply))
> > +			return PTR_ERR(scs[i]->rsupply);
> > +	}
> > +
> >   	data->num_domains = num;
> >   	for (i = 0; i < num; i++) {
> >   		if (!scs[i])
> > diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> > index 64cdc8cf0d4d..c36fc26dcdff 100644
> > --- a/drivers/clk/qcom/gdsc.h
> > +++ b/drivers/clk/qcom/gdsc.h
> > @@ -10,6 +10,7 @@
> >   #include <linux/pm_domain.h>
> >   struct regmap;
> > +struct regulator;
> >   struct reset_controller_dev;
> >   /**
> > @@ -52,6 +53,9 @@ struct gdsc {
> >   	struct reset_controller_dev	*rcdev;
> >   	unsigned int			*resets;
> >   	unsigned int			reset_count;
> > +
> > +	const char 			*supply;
> > +	struct regulator		*rsupply;
> >   };
> >   struct gdsc_desc {
> > 
> 
> -- 
> 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] 14+ messages in thread

end of thread, other threads:[~2020-03-31  6:08 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-19  5:38 [PATCH 0/4] clk: qcom: gdsc: Handle supply regulators Bjorn Andersson
2020-03-19  5:38 ` [PATCH 1/4] clk: qcom: gdsc: Handle GDSC regulator supplies Bjorn Andersson
2020-03-20 23:31   ` Stephen Boyd
2020-03-31  5:35   ` Taniya Das
2020-03-31  6:08     ` Bjorn Andersson
2020-03-19  5:39 ` [PATCH 2/4] clk: qcom: mmcc-msm8996: Properly describe GPU_GX gdsc Bjorn Andersson
2020-03-20 23:31   ` Stephen Boyd
2020-03-21  5:16     ` Bjorn Andersson
2020-03-21 18:43       ` Stephen Boyd
2020-03-30 23:10         ` Rob Herring
2020-03-19  5:39 ` [PATCH 3/4] arm64: dts: qcom: db820c: Add s2 regulator in pmi8994 Bjorn Andersson
2020-03-20 23:33   ` Stephen Boyd
2020-03-21  5:19     ` Bjorn Andersson
2020-03-19  5:39 ` [PATCH 4/4] arm64: dts: qcom: msm8996: Make GPU node control GPU_GX GDSC Bjorn Andersson

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