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:

quoted

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

--