| Message ID | 1528285308-25477-2-git-send-email-anischal@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Andy Gross |
| Headers | show |
Quoting Amit Nischal (2018-06-06 04:41:45) > For some of the GDSCs, there is a requirement to enable/disable the > few clocks before turning on/off the gdsc power domain. Add support Why is there a requirement? Do the clks need to be in hw control mode or they can't be turned off when the GDSC is off? It's hard for me to understand with these vague statements. > for the same by specifying a list of clk_hw pointers per gdsc and > enable/disable them along with power domain on/off callbacks. > > Signed-off-by: Amit Nischal <anischal@codeaurora.org> > --- > drivers/clk/qcom/gdsc.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ > drivers/clk/qcom/gdsc.h | 5 +++++ > 2 files changed, 49 insertions(+) > > diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c > index a077133..b6adca1 100644 > --- a/drivers/clk/qcom/gdsc.c > +++ b/drivers/clk/qcom/gdsc.c > @@ -12,6 +12,8 @@ > */ > > #include <linux/bitops.h> > +#include <linux/clk.h> Ugh. > +#include <linux/clk-provider.h> Both, really? > #include <linux/delay.h> > #include <linux/err.h> > #include <linux/jiffies.h> > @@ -208,11 +210,41 @@ static inline void gdsc_assert_reset_aon(struct gdsc *sc) > regmap_update_bits(sc->regmap, sc->clamp_io_ctrl, > GMEM_RESET_MASK, 0); > } > + > +static int gdsc_clk_prepare_enable(struct gdsc *sc) > +{ > + int i, ret; > + > + for (i = 0; i < sc->clk_count; i++) { > + ret = clk_prepare_enable(sc->clk_hws[i]->clk); > + if (ret) { > + for (i--; i >= 0; i--) > + clk_disable_unprepare(sc->clk_hws[i]->clk); > + return ret; > + } > + } > + return 0; > +} > + Looks an awful lot like bulk_enable clk API. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Stephen, Thanks for the review comments. Regards, Amit On 2018-07-09 11:04, Stephen Boyd wrote: > Quoting Amit Nischal (2018-06-06 04:41:45) >> For some of the GDSCs, there is a requirement to enable/disable the >> few clocks before turning on/off the gdsc power domain. Add support > > Why is there a requirement? Do the clks need to be in hw control mode > or > they can't be turned off when the GDSC is off? It's hard for me to > understand with these vague statements. > This requirement is primarily to turn on the GPU GX GDSC and these clocks do not need to be in HW control mode and clock disable is not related with the GDSC. To turn on the GX GDSC, root clock (GFX3D RCG) needs to be enabled first before writing to SW_COLLAPSE bit of the GDSC. The CLK_ON signal from RCG would power up the GPU GX GDSC. >> for the same by specifying a list of clk_hw pointers per gdsc and >> enable/disable them along with power domain on/off callbacks. >> >> Signed-off-by: Amit Nischal <anischal@codeaurora.org> >> --- >> drivers/clk/qcom/gdsc.c | 44 >> ++++++++++++++++++++++++++++++++++++++++++++ >> drivers/clk/qcom/gdsc.h | 5 +++++ >> 2 files changed, 49 insertions(+) >> >> diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c >> index a077133..b6adca1 100644 >> --- a/drivers/clk/qcom/gdsc.c >> +++ b/drivers/clk/qcom/gdsc.c >> @@ -12,6 +12,8 @@ >> */ >> >> #include <linux/bitops.h> >> +#include <linux/clk.h> > > Ugh. > >> +#include <linux/clk-provider.h> > > Both, really? > Above includes are required else we get a compilation error as below: error: dereferencing pointer to incomplete type ret = clk_prepare_enable(sc->clk_hws[i]->clk); ^ >> #include <linux/delay.h> >> #include <linux/err.h> >> #include <linux/jiffies.h> >> @@ -208,11 +210,41 @@ static inline void gdsc_assert_reset_aon(struct >> gdsc *sc) >> regmap_update_bits(sc->regmap, sc->clamp_io_ctrl, >> GMEM_RESET_MASK, 0); >> } >> + >> +static int gdsc_clk_prepare_enable(struct gdsc *sc) >> +{ >> + int i, ret; >> + >> + for (i = 0; i < sc->clk_count; i++) { >> + ret = clk_prepare_enable(sc->clk_hws[i]->clk); >> + if (ret) { >> + for (i--; i >= 0; i--) >> + >> clk_disable_unprepare(sc->clk_hws[i]->clk); >> + return ret; >> + } >> + } >> + return 0; >> +} >> + > > Looks an awful lot like bulk_enable clk API. As mentioned above, ROOT_EN bit of GFX3D RCG needs to be enabled first to turn on the GDSC. We want this enable to happen only through clock framework API in order to avoid stability issues. > -- > To unsubscribe from this list: send the line "unsubscribe linux-clk" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoting Amit Nischal (2018-07-12 05:23:48) > On 2018-07-09 11:04, Stephen Boyd wrote: > > Quoting Amit Nischal (2018-06-06 04:41:45) > >> For some of the GDSCs, there is a requirement to enable/disable the > >> few clocks before turning on/off the gdsc power domain. Add support > > > > Why is there a requirement? Do the clks need to be in hw control mode > > or > > they can't be turned off when the GDSC is off? It's hard for me to > > understand with these vague statements. > > > > This requirement is primarily to turn on the GPU GX GDSC and these > clocks > do not need to be in HW control mode and clock disable is not related > with the GDSC. Ok that's good to know. > > To turn on the GX GDSC, root clock (GFX3D RCG) needs to be enabled first > before writing to SW_COLLAPSE bit of the GDSC. The CLK_ON signal from > RCG > would power up the GPU GX GDSC. Can you please put this specific information in the commit text instead of making a vague statement about GDSC hardware configurations? Does anything go wrong if the GDSC is enabled from genpd but doesn't actually turn on until the GFX3D RCG root bit is enabled or disabled by the clk enable call? I suppose we won't know if the GDSC is enabled or not until the clk is enabled? Maybe we should make the clk enable of the RCG for GPU go check the GDSC status bit as well to make sure it's toggling on or off? Also, does the RCG turn on when the GX GDSC is off? I think we may be able to rely on the GPU driver to "do the right thing" and enable the GPU CX GDSC first, then the RCG and branch for the GFX3D clk, and then the GPU GX GDSC for the core GPU logic. Then we don't need to do anything special in the GDSC code for this. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2018-07-25 12:22, Stephen Boyd wrote: > Quoting Amit Nischal (2018-07-12 05:23:48) >> On 2018-07-09 11:04, Stephen Boyd wrote: >> > Quoting Amit Nischal (2018-06-06 04:41:45) >> >> For some of the GDSCs, there is a requirement to enable/disable the >> >> few clocks before turning on/off the gdsc power domain. Add support >> > >> > Why is there a requirement? Do the clks need to be in hw control mode >> > or >> > they can't be turned off when the GDSC is off? It's hard for me to >> > understand with these vague statements. >> > >> >> This requirement is primarily to turn on the GPU GX GDSC and these >> clocks >> do not need to be in HW control mode and clock disable is not related >> with the GDSC. > > Ok that's good to know. > >> >> To turn on the GX GDSC, root clock (GFX3D RCG) needs to be enabled >> first >> before writing to SW_COLLAPSE bit of the GDSC. The CLK_ON signal from >> RCG >> would power up the GPU GX GDSC. > > Can you please put this specific information in the commit text instead > of making a vague statement about GDSC hardware configurations? > > Does anything go wrong if the GDSC is enabled from genpd but doesn't > actually turn on until the GFX3D RCG root bit is enabled or disabled by > the clk enable call? I suppose we won't know if the GDSC is enabled or > not until the clk is enabled? Maybe we should make the clk enable of > the > RCG for GPU go check the GDSC status bit as well to make sure it's > toggling on or off? As per the current design, before turning on the GDSC(writing to the GDSCR register) we are setting the ROOT_EN bit of RCG and GDSC's status will be still off without clk_enable call to RCG even though we enable the GDSC. clk_enable for RCG is CLK_ON signal for the GPU_GX_GDSC. > > Also, does the RCG turn on when the GX GDSC is off? I think we may be > able to rely on the GPU driver to "do the right thing" and enable the > GPU CX GDSC first, then the RCG and branch for the GFX3D clk, and then > the GPU GX GDSC for the core GPU logic. Then we don't need to do > anything special in the GDSC code for this. Its a GPU_GX_GDSC requirement to enable the RCG first and then GX_GDSC. We want all of this sequence to be done by the GDSC driver so that client only call for clk_apis for the clock branch. For clients, It will be extra overhead to follow the below sequence. GPU_CX_GDSC enable -> Enable RCG -> Enable GPU_GX_GDSC -> Enable Branch. -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/clk/qcom/gdsc.c b/drivers/clk/qcom/gdsc.c index a077133..b6adca1 100644 --- a/drivers/clk/qcom/gdsc.c +++ b/drivers/clk/qcom/gdsc.c @@ -12,6 +12,8 @@ */ #include <linux/bitops.h> +#include <linux/clk.h> +#include <linux/clk-provider.h> #include <linux/delay.h> #include <linux/err.h> #include <linux/jiffies.h> @@ -208,11 +210,41 @@ static inline void gdsc_assert_reset_aon(struct gdsc *sc) regmap_update_bits(sc->regmap, sc->clamp_io_ctrl, GMEM_RESET_MASK, 0); } + +static int gdsc_clk_prepare_enable(struct gdsc *sc) +{ + int i, ret; + + for (i = 0; i < sc->clk_count; i++) { + ret = clk_prepare_enable(sc->clk_hws[i]->clk); + if (ret) { + for (i--; i >= 0; i--) + clk_disable_unprepare(sc->clk_hws[i]->clk); + return ret; + } + } + return 0; +} + +static void gdsc_clk_disable_unprepare(struct gdsc *sc) +{ + int i; + + for (i = 0; i < sc->clk_count; i++) + clk_disable_unprepare(sc->clk_hws[i]->clk); +} + static int gdsc_enable(struct generic_pm_domain *domain) { struct gdsc *sc = domain_to_gdsc(domain); int ret; + if (sc->clk_count) { + ret = gdsc_clk_prepare_enable(sc); + if (ret) + return ret; + } + if (sc->pwrsts == PWRSTS_ON) return gdsc_deassert_reset(sc); @@ -260,6 +292,9 @@ static int gdsc_enable(struct generic_pm_domain *domain) udelay(1); } + if (sc->clk_count) + gdsc_clk_disable_unprepare(sc); + return 0; } @@ -268,6 +303,12 @@ static int gdsc_disable(struct generic_pm_domain *domain) struct gdsc *sc = domain_to_gdsc(domain); int ret; + if (sc->clk_count) { + ret = gdsc_clk_prepare_enable(sc); + if (ret) + return ret; + } + if (sc->pwrsts == PWRSTS_ON) return gdsc_assert_reset(sc); @@ -299,6 +340,9 @@ static int gdsc_disable(struct generic_pm_domain *domain) if (sc->flags & CLAMP_IO) gdsc_assert_clamp_io(sc); + if (sc->clk_count) + gdsc_clk_disable_unprepare(sc); + return 0; } diff --git a/drivers/clk/qcom/gdsc.h b/drivers/clk/qcom/gdsc.h index bd1f2c7..59957d7 100644 --- a/drivers/clk/qcom/gdsc.h +++ b/drivers/clk/qcom/gdsc.h @@ -17,6 +17,7 @@ #include <linux/err.h> #include <linux/pm_domain.h> +struct clk_hw; struct regmap; struct reset_controller_dev; @@ -32,6 +33,8 @@ * @resets: ids of resets associated with this gdsc * @reset_count: number of @resets * @rcdev: reset controller + * @clk_count: number of associated clocks + * @clk_hws: clk_hw pointers for associated clocks with gdsc */ struct gdsc { struct generic_pm_domain pd; @@ -60,6 +63,8 @@ struct gdsc { struct reset_controller_dev *rcdev; unsigned int *resets; unsigned int reset_count; + unsigned int clk_count; + struct clk_hw *clk_hws[]; }; struct gdsc_desc {
For some of the GDSCs, there is a requirement to enable/disable the few clocks before turning on/off the gdsc power domain. Add support for the same by specifying a list of clk_hw pointers per gdsc and enable/disable them along with power domain on/off callbacks. Signed-off-by: Amit Nischal <anischal@codeaurora.org> --- drivers/clk/qcom/gdsc.c | 44 ++++++++++++++++++++++++++++++++++++++++++++ drivers/clk/qcom/gdsc.h | 5 +++++ 2 files changed, 49 insertions(+) -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html