diff mbox series

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

Message ID 20201005225914.315852-3-dmitry.baryshkov@linaro.org
State New
Headers show
Series clk: qcom: handle power domains supplies for GDSC | expand

Commit Message

Dmitry Baryshkov Oct. 5, 2020, 10:59 p.m. UTC
On SM8250 MDSS_GDSC (and respective dispcc clocks) are children of MMCX
power domain. MMCX needs to be enabled to be able to access GDSC
registers and to enable display clocks. Use dev_pm/opp to enable
corresponding power domain.

Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/clk/qcom/gdsc.c | 56 ++++++++++++++++++++++++++++++++++++++---
 drivers/clk/qcom/gdsc.h |  5 ++++
 2 files changed, 57 insertions(+), 4 deletions(-)

Comments

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

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

>  drivers/clk/qcom/gdsc.c | 56 ++++++++++++++++++++++++++++++++++++++---
>  drivers/clk/qcom/gdsc.h |  5 ++++
>  2 files changed, 57 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
> index bd537438c793..d58575f8f25f 100644
> --- a/drivers/clk/qcom/gdsc.h
> +++ b/drivers/clk/qcom/gdsc.h
> @@ -57,6 +57,11 @@ struct gdsc {
>  
>         const char                      *supply;
>         struct regulator                *rsupply;
> +
> +       const char                      *domain;
> +       unsigned int                    perf_idx;
> +       struct device                   *pd_dev;
> +       int                             pd_opp;

Please document these fields.

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

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

> 
>>   drivers/clk/qcom/gdsc.c | 56 ++++++++++++++++++++++++++++++++++++++---
>>   drivers/clk/qcom/gdsc.h |  5 ++++
>>   2 files changed, 57 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
>> index bd537438c793..d58575f8f25f 100644
>> --- a/drivers/clk/qcom/gdsc.h
>> +++ b/drivers/clk/qcom/gdsc.h
>> @@ -57,6 +57,11 @@ struct gdsc {
>>   
>>          const char                      *supply;
>>          struct regulator                *rsupply;
>> +
>> +       const char                      *domain;
>> +       unsigned int                    perf_idx;
>> +       struct device                   *pd_dev;
>> +       int                             pd_opp;
> 
> Please document these fields.

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

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

Patch

diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c
index bfc4ac02f9ea..c9e1619074f8 100644
--- a/drivers/clk/qcom/gdsc.c
+++ b/drivers/clk/qcom/gdsc.c
@@ -11,6 +11,7 @@ 
 #include <linux/kernel.h>
 #include <linux/ktime.h>
 #include <linux/pm_domain.h>
+#include <linux/pm_opp.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/reset-controller.h>
@@ -110,13 +111,31 @@  static int gdsc_poll_status(struct gdsc *sc, enum gdsc_status status)
 	return -ETIMEDOUT;
 }
 
+static int gdsc_toggle_supply_on(struct gdsc *sc)
+{
+	if (sc->rsupply)
+		return regulator_enable(sc->rsupply);
+	if (sc->pd_dev)
+		return dev_pm_genpd_set_performance_state(sc->pd_dev, sc->pd_opp);
+	return 0;
+}
+
+static int gdsc_toggle_supply_off(struct gdsc *sc)
+{
+	if (sc->pd_dev)
+		return dev_pm_genpd_set_performance_state(sc->pd_dev, 0);
+	if (sc->rsupply)
+		return regulator_disable(sc->rsupply);
+	return 0;
+}
+
 static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
 {
 	int ret;
 	u32 val = (status == GDSC_ON) ? 0 : SW_COLLAPSE_MASK;
 
-	if (status == GDSC_ON && sc->rsupply) {
-		ret = regulator_enable(sc->rsupply);
+	if (status == GDSC_ON) {
+		ret = gdsc_toggle_supply_on(sc);
 		if (ret < 0)
 			return ret;
 	}
@@ -153,8 +172,8 @@  static int gdsc_toggle_logic(struct gdsc *sc, enum gdsc_status status)
 	ret = gdsc_poll_status(sc, status);
 	WARN(ret, "%s status stuck at 'o%s'", sc->pd.name, status ? "ff" : "n");
 
-	if (!ret && status == GDSC_OFF && sc->rsupply) {
-		ret = regulator_disable(sc->rsupply);
+	if (!ret && status == GDSC_OFF) {
+		ret = gdsc_toggle_supply_off(sc);
 		if (ret < 0)
 			return ret;
 	}
@@ -407,6 +426,27 @@  int gdsc_register(struct gdsc_desc *desc,
 			return PTR_ERR(scs[i]->rsupply);
 	}
 
+	for (i = 0; i < num; i++) {
+		if (!scs[i] || !scs[i]->domain)
+			continue;
+
+		scs[i]->pd_opp = of_get_required_opp_performance_state(dev->of_node, scs[i]->perf_idx);
+		if (scs[i]->pd_opp < 0)
+			return scs[i]->pd_opp;
+
+		scs[i]->pd_dev = dev_pm_domain_attach_by_name(dev, scs[i]->domain);
+		if (IS_ERR(scs[i]->pd_dev)) {
+			ret = PTR_ERR(scs[i]->pd_dev);
+			/* Single domain has been already attached, so reuse dev */
+			if (ret == -EEXIST) {
+				scs[i]->pd_dev = dev;
+			} else {
+				scs[i]->pd_dev = NULL;
+				goto pm_detach;
+			}
+		}
+	}
+
 	data->num_domains = num;
 	for (i = 0; i < num; i++) {
 		if (!scs[i])
@@ -428,6 +468,12 @@  int gdsc_register(struct gdsc_desc *desc,
 	}
 
 	return of_genpd_add_provider_onecell(dev->of_node, data);
+
+pm_detach:
+	for (i = 0; i < num; i++)
+		if (scs[i]->pd_dev && scs[i]->pd_dev != dev)
+			dev_pm_domain_detach(scs[i]->pd_dev, false);
+	return ret;
 }
 
 void gdsc_unregister(struct gdsc_desc *desc)
@@ -443,6 +489,8 @@  void gdsc_unregister(struct gdsc_desc *desc)
 			continue;
 		if (scs[i]->parent)
 			pm_genpd_remove_subdomain(scs[i]->parent, &scs[i]->pd);
+		if (scs[i]->pd_dev && scs[i]->pd_dev != dev)
+			dev_pm_domain_detach(scs[i]->pd_dev, true);
 	}
 	of_genpd_del_provider(dev->of_node);
 }
diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h
index bd537438c793..d58575f8f25f 100644
--- a/drivers/clk/qcom/gdsc.h
+++ b/drivers/clk/qcom/gdsc.h
@@ -57,6 +57,11 @@  struct gdsc {
 
 	const char 			*supply;
 	struct regulator		*rsupply;
+
+	const char			*domain;
+	unsigned int			perf_idx;
+	struct device			*pd_dev;
+	int				pd_opp;
 };
 
 struct gdsc_desc {