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@xxxxxxxxxxxxxx> 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.
