| Message ID | 20201005225914.315852-3-dmitry.baryshkov@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/3] dt-bindings: clock: qcom,dispcc: document power domain bindings | expand |
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. > };
On 14/10/2020 05:15, Stephen Boyd wrote: > Quoting Dmitry Baryshkov (2020-10-05 15:59:13) >> On SM8250 MDSS_GDSC (and respective dispcc clocks) are children of MMCX >> power domain. MMCX needs to be enabled to be able to access GDSC >> registers and to enable display clocks. Use dev_pm/opp to enable >> corresponding power domain. >> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- > > A general question is why is this done in the gdsc code instead of > somewhere generic? It seems that genpds may need to change the > performance state of other genpds. I vaguely recall that genpd supports > connecting different power domains together so maybe this could all be > handled in the genpd layer instead of here? Then a regulator could be > put behind a genpd and similarly be connected to the gdsc and turned on > before turning on the gdsc? Basically because we need not only to enable the genpd, but also to set performance state. This would mean creating a separate regulator driver calling dev_pm_genpd_set_performance_state() from enable/disable paths. Does that seem like a better solution to you? > >> drivers/clk/qcom/gdsc.c | 56 ++++++++++++++++++++++++++++++++++++++--- >> drivers/clk/qcom/gdsc.h | 5 ++++ >> 2 files changed, 57 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h >> index bd537438c793..d58575f8f25f 100644 >> --- a/drivers/clk/qcom/gdsc.h >> +++ b/drivers/clk/qcom/gdsc.h >> @@ -57,6 +57,11 @@ struct gdsc { >> >> const char *supply; >> struct regulator *rsupply; >> + >> + const char *domain; >> + unsigned int perf_idx; >> + struct device *pd_dev; >> + int pd_opp; > > Please document these fields. Will do. -- With best wishes Dmitry
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 --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 {
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(-) -- 2.28.0