Messages in this thread | | | Subject | Re: [PATCH v3 1/4] clk: qcom: gdsc: Add support to enable/disable the clocks with GDSC | From | Taniya Das <> | Date | Mon, 19 Nov 2018 16:51:56 +0530 |
| |
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.
-- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation.
--
| |