| Message ID | 20210709173202.667820-1-dmitry.baryshkov@linaro.org |
|---|---|
| Headers | show |
| Series | clk: qcom: use power-domain for sm8250's clock controllers | expand |
On Fri 09 Jul 12:31 CDT 2021, Dmitry Baryshkov wrote: > In order to properly handle runtime PM status of the provider device, > call pm_runtime_get/pm_runtime_put on the clock controller device. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > --- > drivers/clk/qcom/gdsc.c | 66 ++++++++++++++++++++++++++++++++++++++--- > drivers/clk/qcom/gdsc.h | 2 ++ > 2 files changed, 64 insertions(+), 4 deletions(-) > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > index ccd36617d067..6bec31fccb09 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> > @@ -50,6 +51,30 @@ enum gdsc_status { > GDSC_ON > }; > > +static int gdsc_pm_runtime_get(struct gdsc *sc) > +{ > + int ret; > + > + if (!sc->rpm_dev) > + return 0; > + > + ret = pm_runtime_get_sync(sc->rpm_dev); > + if (ret < 0) { > + pm_runtime_put_noidle(sc->rpm_dev); > + return ret; > + } > + > + return 0; > +} > + > +static int gdsc_pm_runtime_put(struct gdsc *sc) > +{ > + if (!sc->rpm_dev) > + return 0; > + > + return pm_runtime_put_sync(sc->rpm_dev); > +} > + > /* Returns 1 if GDSC status is status, 0 if not, and < 0 on error */ > static int gdsc_check_status(struct gdsc *sc, enum gdsc_status status) > { > @@ -232,9 +257,8 @@ static void gdsc_retain_ff_on(struct gdsc *sc) > regmap_update_bits(sc->regmap, sc->gdscr, mask, mask); > } > > -static int gdsc_enable(struct generic_pm_domain *domain) > +static int _gdsc_enable(struct gdsc *sc) > { > - struct gdsc *sc = domain_to_gdsc(domain); > int ret; > > if (sc->pwrsts == PWRSTS_ON) > @@ -290,11 +314,28 @@ static int gdsc_enable(struct generic_pm_domain *domain) > return 0; > } > > -static int gdsc_disable(struct generic_pm_domain *domain) > +static int gdsc_enable(struct generic_pm_domain *domain) > { > struct gdsc *sc = domain_to_gdsc(domain); > int ret; > > + ret = gdsc_pm_runtime_get(sc); > + if (ret) > + return ret; > + > + ret = _gdsc_enable(sc); > + if (ret) { > + gdsc_pm_runtime_put(sc); I presume what you do here is to leave the pm_runtime state of dispcc active if we succeeded in enabling the gdsc. But the gdsc is a subdomain of the parent domain, so the framework should take case of its dependency. So the reason for gdsc_pm_runtime_get()/put() in this code path is so that you can access the dispcc registers, i.e. I think you should get()/put() regardless of the return value. > + return ret; > + } > + > + return 0; > +} > + > +static int _gdsc_disable(struct gdsc *sc) > +{ > + int ret; > + > if (sc->pwrsts == PWRSTS_ON) > return gdsc_assert_reset(sc); > > @@ -329,6 +370,18 @@ static int gdsc_disable(struct generic_pm_domain *domain) > return 0; > } > > +static int gdsc_disable(struct generic_pm_domain *domain) > +{ > + struct gdsc *sc = domain_to_gdsc(domain); > + int ret; > + If the gdsc is found to be on at initialization, the next operation that will happen is gdsc_disable() and as you didn't activate the pm_runtime state in gdsc_init() you would in theory get here with registers unaccessible. In practice though, the active gdsc should through the being a subdomain of the parent domain keep power on for you, so you won't notice this issue. But as above, I think you should wrap _gdsc_disable() in a get()/put() pair. > + ret = _gdsc_disable(sc); > + if (ret) > + return ret; > + > + return gdsc_pm_runtime_put(sc); > +} > + > static int gdsc_init(struct gdsc *sc) > { > u32 mask, val; > @@ -425,6 +478,8 @@ int gdsc_register(struct gdsc_desc *desc, > for (i = 0; i < num; i++) { > if (!scs[i]) > continue; > + if (pm_runtime_enabled(dev)) > + scs[i]->rpm_dev = dev; > scs[i]->regmap = regmap; > scs[i]->rcdev = rcdev; > ret = gdsc_init(scs[i]); > @@ -486,7 +541,10 @@ void gdsc_unregister(struct gdsc_desc *desc) > */ > int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain) > { > + struct gdsc *sc = domain_to_gdsc(domain); > + > /* Do nothing but give genpd the impression that we were successful */ > - return 0; > + /* Get the runtime PM device only */ > + return gdsc_pm_runtime_get(sc); Per above, if you let the framework deal with the gdsc's dependencies on the parent domain and you only get()/put() for the sake of dispcc then you don't need you don't need to do this to keep the subsequent gdsc_disable() in balance. > } > EXPORT_SYMBOL_GPL(gdsc_gx_do_nothing_enable); > diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h > index 5bb396b344d1..a82982df0a55 100644 > --- a/drivers/clk/qcom/gdsc.h > +++ b/drivers/clk/qcom/gdsc.h > @@ -25,6 +25,7 @@ struct reset_controller_dev; > * @resets: ids of resets associated with this gdsc > * @reset_count: number of @resets > * @rcdev: reset controller > + * @rpm_dev: runtime PM device > */ > struct gdsc { > struct generic_pm_domain pd; > @@ -58,6 +59,7 @@ struct gdsc { > > const char *supply; > struct regulator *rsupply; > + struct device *rpm_dev; This isn't just the "runtime pm device", it's the device this gdsc is associated with. So "dev" sounds sufficient to me, but that requires that you have a separate bool rpm_enabled to remember if pm_runtime_enabled() was true during probe. So unless we need "dev" for something else this might be sufficient. Regards, Bjorn > }; > > struct gdsc_desc { > -- > 2.30.2 >
On Fri 09 Jul 12:32 CDT 2021, Dmitry Baryshkov wrote: > Switch dispcc and videocc into using MMCX domain directly. Drop the now > unused mmcx regulator. > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org> Regards, Bjorn > --- > arch/arm64/boot/dts/qcom/sm8250.dtsi | 11 ++--------- > 1 file changed, 2 insertions(+), 9 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi > index 4c0de12aaba6..2a468b85dc09 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,7 @@ 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>; > clock-names = "iface", "bi_tcxo", "bi_tcxo_ao"; > #clock-cells = <1>; > #reset-cells = <1>; > @@ -2627,7 +2620,7 @@ opp-358000000 { > dispcc: clock-controller@af00000 { > compatible = "qcom,sm8250-dispcc"; > reg = <0 0x0af00000 0 0x10000>; > - mmcx-supply = <&mmcx_reg>; > + power-domains = <&rpmhpd SM8250_MMCX>; > clocks = <&rpmhcc RPMH_CXO_CLK>, > <&dsi0_phy 0>, > <&dsi0_phy 1>, > -- > 2.30.2 >
On Fri 09 Jul 12:32 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 >
On 09/07/2021 21:54, Bjorn Andersson wrote: > On Fri 09 Jul 12:31 CDT 2021, Dmitry Baryshkov wrote: > >> In order to properly handle runtime PM status of the provider device, >> call pm_runtime_get/pm_runtime_put on the clock controller device. >> >> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> >> --- >> drivers/clk/qcom/gdsc.c | 66 ++++++++++++++++++++++++++++++++++++++--- >> drivers/clk/qcom/gdsc.h | 2 ++ >> 2 files changed, 64 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c >> index ccd36617d067..6bec31fccb09 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> >> @@ -50,6 +51,30 @@ enum gdsc_status { >> GDSC_ON >> }; >> >> +static int gdsc_pm_runtime_get(struct gdsc *sc) >> +{ >> + int ret; >> + >> + if (!sc->rpm_dev) >> + return 0; >> + >> + ret = pm_runtime_get_sync(sc->rpm_dev); >> + if (ret < 0) { >> + pm_runtime_put_noidle(sc->rpm_dev); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static int gdsc_pm_runtime_put(struct gdsc *sc) >> +{ >> + if (!sc->rpm_dev) >> + return 0; >> + >> + return pm_runtime_put_sync(sc->rpm_dev); >> +} >> + >> /* Returns 1 if GDSC status is status, 0 if not, and < 0 on error */ >> static int gdsc_check_status(struct gdsc *sc, enum gdsc_status status) >> { >> @@ -232,9 +257,8 @@ static void gdsc_retain_ff_on(struct gdsc *sc) >> regmap_update_bits(sc->regmap, sc->gdscr, mask, mask); >> } >> >> -static int gdsc_enable(struct generic_pm_domain *domain) >> +static int _gdsc_enable(struct gdsc *sc) >> { >> - struct gdsc *sc = domain_to_gdsc(domain); >> int ret; >> >> if (sc->pwrsts == PWRSTS_ON) >> @@ -290,11 +314,28 @@ static int gdsc_enable(struct generic_pm_domain *domain) >> return 0; >> } >> >> -static int gdsc_disable(struct generic_pm_domain *domain) >> +static int gdsc_enable(struct generic_pm_domain *domain) >> { >> struct gdsc *sc = domain_to_gdsc(domain); >> int ret; >> >> + ret = gdsc_pm_runtime_get(sc); >> + if (ret) >> + return ret; >> + >> + ret = _gdsc_enable(sc); >> + if (ret) { >> + gdsc_pm_runtime_put(sc); > > I presume what you do here is to leave the pm_runtime state of dispcc > active if we succeeded in enabling the gdsc. But the gdsc is a subdomain > of the parent domain, so the framework should take case of its > dependency. > > So the reason for gdsc_pm_runtime_get()/put() in this code path is so > that you can access the dispcc registers, i.e. I think you should > get()/put() regardless of the return value. pm domain code will handle enabling MMCX, so this code is not required strictly speaking. Ulf suggested adding it back, so I followed the suggestion. Maybe I misunderstood his suggestion. putting pm_runtime after gdsc_enable does not sound like a logical case. However it would simplify code a bit. Let me try... > >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> +static int _gdsc_disable(struct gdsc *sc) >> +{ >> + int ret; >> + >> if (sc->pwrsts == PWRSTS_ON) >> return gdsc_assert_reset(sc); >> >> @@ -329,6 +370,18 @@ static int gdsc_disable(struct generic_pm_domain *domain) >> return 0; >> } >> >> +static int gdsc_disable(struct generic_pm_domain *domain) >> +{ >> + struct gdsc *sc = domain_to_gdsc(domain); >> + int ret; >> + > > If the gdsc is found to be on at initialization, the next operation that > will happen is gdsc_disable() and as you didn't activate the pm_runtime > state in gdsc_init() you would in theory get here with registers > unaccessible. > > In practice though, the active gdsc should through the being a subdomain > of the parent domain keep power on for you, so you won't notice this > issue. Nice catch. > > But as above, I think you should wrap _gdsc_disable() in a get()/put() > pair. > >> + ret = _gdsc_disable(sc); >> + if (ret) >> + return ret; >> + >> + return gdsc_pm_runtime_put(sc); >> +} >> + >> static int gdsc_init(struct gdsc *sc) >> { >> u32 mask, val; >> @@ -425,6 +478,8 @@ int gdsc_register(struct gdsc_desc *desc, >> for (i = 0; i < num; i++) { >> if (!scs[i]) >> continue; >> + if (pm_runtime_enabled(dev)) >> + scs[i]->rpm_dev = dev; >> scs[i]->regmap = regmap; >> scs[i]->rcdev = rcdev; >> ret = gdsc_init(scs[i]); >> @@ -486,7 +541,10 @@ void gdsc_unregister(struct gdsc_desc *desc) >> */ >> int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain) >> { >> + struct gdsc *sc = domain_to_gdsc(domain); >> + >> /* Do nothing but give genpd the impression that we were successful */ >> - return 0; >> + /* Get the runtime PM device only */ >> + return gdsc_pm_runtime_get(sc); > > Per above, if you let the framework deal with the gdsc's dependencies on > the parent domain and you only get()/put() for the sake of dispcc then > you don't need you don't need to do this to keep the subsequent > gdsc_disable() in balance. > >> } >> EXPORT_SYMBOL_GPL(gdsc_gx_do_nothing_enable); >> diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h >> index 5bb396b344d1..a82982df0a55 100644 >> --- a/drivers/clk/qcom/gdsc.h >> +++ b/drivers/clk/qcom/gdsc.h >> @@ -25,6 +25,7 @@ struct reset_controller_dev; >> * @resets: ids of resets associated with this gdsc >> * @reset_count: number of @resets >> * @rcdev: reset controller >> + * @rpm_dev: runtime PM device >> */ >> struct gdsc { >> struct generic_pm_domain pd; >> @@ -58,6 +59,7 @@ struct gdsc { >> >> const char *supply; >> struct regulator *rsupply; >> + struct device *rpm_dev; > > This isn't just the "runtime pm device", it's the device this gdsc is > associated with. So "dev" sounds sufficient to me, but that requires > that you have a separate bool rpm_enabled to remember if > pm_runtime_enabled() was true during probe. > > So unless we need "dev" for something else this might be sufficient. > > Regards, > Bjorn > >> }; >> >> struct gdsc_desc { >> -- >> 2.30.2 >>
On Fri 09 Jul 17:10 CDT 2021, Dmitry Baryshkov wrote: > On 09/07/2021 21:54, Bjorn Andersson wrote: > > On Fri 09 Jul 12:31 CDT 2021, Dmitry Baryshkov wrote: > > > > > In order to properly handle runtime PM status of the provider device, > > > call pm_runtime_get/pm_runtime_put on the clock controller device. > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > > --- > > > drivers/clk/qcom/gdsc.c | 66 ++++++++++++++++++++++++++++++++++++++--- > > > drivers/clk/qcom/gdsc.h | 2 ++ > > > 2 files changed, 64 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > > > index ccd36617d067..6bec31fccb09 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> > > > @@ -50,6 +51,30 @@ enum gdsc_status { > > > GDSC_ON > > > }; > > > +static int gdsc_pm_runtime_get(struct gdsc *sc) > > > +{ > > > + int ret; > > > + > > > + if (!sc->rpm_dev) > > > + return 0; > > > + > > > + ret = pm_runtime_get_sync(sc->rpm_dev); > > > + if (ret < 0) { > > > + pm_runtime_put_noidle(sc->rpm_dev); > > > + return ret; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static int gdsc_pm_runtime_put(struct gdsc *sc) > > > +{ > > > + if (!sc->rpm_dev) > > > + return 0; > > > + > > > + return pm_runtime_put_sync(sc->rpm_dev); > > > +} > > > + > > > /* Returns 1 if GDSC status is status, 0 if not, and < 0 on error */ > > > static int gdsc_check_status(struct gdsc *sc, enum gdsc_status status) > > > { > > > @@ -232,9 +257,8 @@ static void gdsc_retain_ff_on(struct gdsc *sc) > > > regmap_update_bits(sc->regmap, sc->gdscr, mask, mask); > > > } > > > -static int gdsc_enable(struct generic_pm_domain *domain) > > > +static int _gdsc_enable(struct gdsc *sc) > > > { > > > - struct gdsc *sc = domain_to_gdsc(domain); > > > int ret; > > > if (sc->pwrsts == PWRSTS_ON) > > > @@ -290,11 +314,28 @@ static int gdsc_enable(struct generic_pm_domain *domain) > > > return 0; > > > } > > > -static int gdsc_disable(struct generic_pm_domain *domain) > > > +static int gdsc_enable(struct generic_pm_domain *domain) > > > { > > > struct gdsc *sc = domain_to_gdsc(domain); > > > int ret; > > > + ret = gdsc_pm_runtime_get(sc); > > > + if (ret) > > > + return ret; > > > + > > > + ret = _gdsc_enable(sc); > > > + if (ret) { > > > + gdsc_pm_runtime_put(sc); > > > > I presume what you do here is to leave the pm_runtime state of dispcc > > active if we succeeded in enabling the gdsc. But the gdsc is a subdomain > > of the parent domain, so the framework should take case of its > > dependency. > > > > So the reason for gdsc_pm_runtime_get()/put() in this code path is so > > that you can access the dispcc registers, i.e. I think you should > > get()/put() regardless of the return value. > > pm domain code will handle enabling MMCX, so this code is not required > strictly speaking. Ulf suggested adding it back, so I followed the > suggestion. Maybe I misunderstood his suggestion. > > putting pm_runtime after gdsc_enable does not sound like a logical case. > However it would simplify code a bit. Let me try... > If you consider this device's and the individual gdsc's power state as separate consumers of MMCX, then it perhaps makes more sense? Similar to how a regulator driver would rely on the regulator framework to deal with parent regulators... Regards, Bjorn > > > > > + return ret; > > > + } > > > + > > > + return 0; > > > +} > > > + > > > +static int _gdsc_disable(struct gdsc *sc) > > > +{ > > > + int ret; > > > + > > > if (sc->pwrsts == PWRSTS_ON) > > > return gdsc_assert_reset(sc); > > > @@ -329,6 +370,18 @@ static int gdsc_disable(struct generic_pm_domain *domain) > > > return 0; > > > } > > > +static int gdsc_disable(struct generic_pm_domain *domain) > > > +{ > > > + struct gdsc *sc = domain_to_gdsc(domain); > > > + int ret; > > > + > > > > If the gdsc is found to be on at initialization, the next operation that > > will happen is gdsc_disable() and as you didn't activate the pm_runtime > > state in gdsc_init() you would in theory get here with registers > > unaccessible. > > > > In practice though, the active gdsc should through the being a subdomain > > of the parent domain keep power on for you, so you won't notice this > > issue. > > Nice catch. > > > > > But as above, I think you should wrap _gdsc_disable() in a get()/put() > > pair. > > > > > + ret = _gdsc_disable(sc); > > > + if (ret) > > > + return ret; > > > + > > > + return gdsc_pm_runtime_put(sc); > > > +} > > > + > > > static int gdsc_init(struct gdsc *sc) > > > { > > > u32 mask, val; > > > @@ -425,6 +478,8 @@ int gdsc_register(struct gdsc_desc *desc, > > > for (i = 0; i < num; i++) { > > > if (!scs[i]) > > > continue; > > > + if (pm_runtime_enabled(dev)) > > > + scs[i]->rpm_dev = dev; > > > scs[i]->regmap = regmap; > > > scs[i]->rcdev = rcdev; > > > ret = gdsc_init(scs[i]); > > > @@ -486,7 +541,10 @@ void gdsc_unregister(struct gdsc_desc *desc) > > > */ > > > int gdsc_gx_do_nothing_enable(struct generic_pm_domain *domain) > > > { > > > + struct gdsc *sc = domain_to_gdsc(domain); > > > + > > > /* Do nothing but give genpd the impression that we were successful */ > > > - return 0; > > > + /* Get the runtime PM device only */ > > > + return gdsc_pm_runtime_get(sc); > > > > Per above, if you let the framework deal with the gdsc's dependencies on > > the parent domain and you only get()/put() for the sake of dispcc then > > you don't need you don't need to do this to keep the subsequent > > gdsc_disable() in balance. > > > > > } > > > EXPORT_SYMBOL_GPL(gdsc_gx_do_nothing_enable); > > > diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h > > > index 5bb396b344d1..a82982df0a55 100644 > > > --- a/drivers/clk/qcom/gdsc.h > > > +++ b/drivers/clk/qcom/gdsc.h > > > @@ -25,6 +25,7 @@ struct reset_controller_dev; > > > * @resets: ids of resets associated with this gdsc > > > * @reset_count: number of @resets > > > * @rcdev: reset controller > > > + * @rpm_dev: runtime PM device > > > */ > > > struct gdsc { > > > struct generic_pm_domain pd; > > > @@ -58,6 +59,7 @@ struct gdsc { > > > const char *supply; > > > struct regulator *rsupply; > > > + struct device *rpm_dev; > > > > This isn't just the "runtime pm device", it's the device this gdsc is > > associated with. So "dev" sounds sufficient to me, but that requires > > that you have a separate bool rpm_enabled to remember if > > pm_runtime_enabled() was true during probe. > > > > So unless we need "dev" for something else this might be sufficient. > > > > Regards, > > Bjorn > > > > > }; > > > struct gdsc_desc { > > > -- > > > 2.30.2 > > > > > > -- > With best wishes > Dmitry
On SM8250 both the display and video clock controllers are powered up by the MMCX power domain. Handle this by linking clock controllers to the proper power domain, and using runtime power management to enable and disable the MMCX power domain. Dependencies: - https://lore.kernel.org/linux-pm/20210603093438.138705-1-ulf.hansson@linaro.org/ (merged in 5.14) - https://lore.kernel.org/linux-arm-msm/20210703005416.2668319-1-bjorn.andersson@linaro.org/ (pending) Changes since v2: - Move pm_runtime calls from generic genpd code to the gdsc code for now (as suggested by Ulf & Bjorn) Changes since v1: - Rebase on top of Bjorn's patches, removing the need for setting performance state directly. - Move runtime PM calls from GDSC code to generic genpd code. - Always call pm_runtime_enable in the Qualcomm generic clock controller code. - Register GDSC power domains as subdomains of the domain powering the clock controller if there is one. ---------------------------------------------------------------- Dmitry Baryshkov (7): 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 clk: qcom: gdsc: call runtime PM functions for the provider device 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 | 7 +++ .../devicetree/bindings/clock/qcom,videocc.yaml | 7 +++ arch/arm64/boot/dts/qcom/sm8250.dtsi | 11 +--- drivers/clk/qcom/common.c | 37 ++++++++++-- drivers/clk/qcom/dispcc-sm8250.c | 1 - drivers/clk/qcom/gdsc.c | 70 ++++++++++++++++++++-- drivers/clk/qcom/gdsc.h | 2 + drivers/clk/qcom/videocc-sm8250.c | 4 -- 8 files changed, 115 insertions(+), 24 deletions(-)