lkml.org 
[lkml]   [2018]   [Nov]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 1/4] clk: qcom: gdsc: Add support to enable/disable the clocks with GDSC
From
Date
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.

--

\
 
 \ /
  Last update: 2018-11-19 12:23    [W:0.184 / U:6.648 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site