| Message ID | 1534141987-29601-2-git-send-email-anischal@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
| Series | Add QCOM graphics clock controller driver for SDM845 | expand |
Quoting Amit Nischal (2018-08-12 23:33:04) > 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> In v1 of this patch series I asked for much more information in this commit text. Please add it here. I won't apply this patch until the justification is put into this commit text. > --- > 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> > +#include <linux/clk-provider.h> This makes me unhappy. It's almost always a problem when we see clk.h and clk-provider.h included in the same file, so if gdsc has to call clk APIs to operate correctly, then we should do that by having the gdsc code get clks properly instead of directly reaching into the clk_hw structure to get a clk pointer. This means we should have the gdsc code ask the clk framework to convert a clk_hw pointer into a clk pointer because of how so intimately connected the gdsc is to clks on this SoC. But given all that, I'm still trying to understand why we need to do this within the gdsc code. Adding these clk calls to the gdsc seems like we're attaching at the wrong abstraction level. Especially if the reason we do it is to make it easier for the GPU driver to handle dependencies. I hope that's not the case. Either way, it would make more sense to me if we made genpds for the clks and genpds for the gdscs and then associated the clk genpds with the gdsc genpds so that when a gdsc is enabled the clk domain that it depends on is enabled first. Then we have a generic solution for connecting clks to gdscs that doesn't require us to add more logic to the gdsc driver and avoids having clk providers do clk consumer things. Instead, it's all handled outside of this driver by specifying a domain dependency. It may turn out that such a solution would still need a way to make clk domains in the clk driver, and it will probably need to do that by converting clk_hw structures into clk pointers, but it would be good to do that anyway. So in summary, I believe we should end up at a point where we have clk domains and power domains (gdscs) all supported with genpds, and then we can connect them together however they're connected by linking the genpds to each other. Device drivers wouldn't really need to care how they're connected, as long as those genpds are attached to their device then the driver would be able to enable/disable them through runtime PM. But I can see how this may be hard to do for this patch series, so in the spirit of progress and getting things done, it would be OK with me if the gdsc code called some new clk API to convert a clk_hw pointer into a clk pointer and then did the same enable/disable things it's doing in this patch. This whole patch would need to be completely untangled and ripped out later when we have clk domains but at least we could get something working now while we work on making clk domains and plumbing them into genpds and runtime PM.
Hello Stephen, On 11/5/2018 12:04 PM, Stephen Boyd wrote: > Quoting Amit Nischal (2018-08-12 23:33:04) >> 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> > > In v1 of this patch series I asked for much more information in this > commit text. Please add it here. I won't apply this patch until the > justification is put into this commit text. > Would surely add more details. >> --- >> 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> >> +#include <linux/clk-provider.h> > > This makes me unhappy. It's almost always a problem when we see clk.h > and clk-provider.h included in the same file, so if gdsc has to call clk > APIs to operate correctly, then we should do that by having the gdsc > code get clks properly instead of directly reaching into the clk_hw > structure to get a clk pointer. This means we should have the gdsc code > ask the clk framework to convert a clk_hw pointer into a clk pointer > because of how so intimately connected the gdsc is to clks on this SoC. > But given all that, I'm still trying to understand why we need to do > this within the gdsc code. > > Adding these clk calls to the gdsc seems like we're attaching at the > wrong abstraction level. Especially if the reason we do it is to make it > easier for the GPU driver to handle dependencies. I hope that's not the > case. Either way, it would make more sense to me if we made genpds for > the clks and genpds for the gdscs and then associated the clk genpds > with the gdsc genpds so that when a gdsc is enabled the clk domain that > it depends on is enabled first. Then we have a generic solution for > connecting clks to gdscs that doesn't require us to add more logic to > the gdsc driver and avoids having clk providers do clk consumer things. > Instead, it's all handled outside of this driver by specifying a domain > dependency. It may turn out that such a solution would still need a way > to make clk domains in the clk driver, and it will probably need to do > that by converting clk_hw structures into clk pointers, but it would be > good to do that anyway. > > So in summary, I believe we should end up at a point where we have clk > domains and power domains (gdscs) all supported with genpds, and then we > can connect them together however they're connected by linking the > genpds to each other. Device drivers wouldn't really need to care how > they're connected, as long as those genpds are attached to their device > then the driver would be able to enable/disable them through runtime PM. > But I can see how this may be hard to do for this patch series, so in > the spirit of progress and getting things done, it would be OK with me > if the gdsc code called some new clk API to convert a clk_hw pointer > into a clk pointer and then did the same enable/disable things it's > doing in this patch. This whole patch would need to be completely > untangled and ripped out later when we have clk domains but at least we > could get something working now while we work on making clk domains and > plumbing them into genpds and runtime PM. > Yes, I agree with your points above, but as genpds currently cannot have a way to take in clock handles, this was the way we chose. I would add a new clock API as suggested and submit the next series.
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(+)