Re: [PATCH v3 1/4] clk: qcom: gdsc: Add support to enable/disable the clocks with GDSC

[Taniya Das <tdas@codeaurora.org>]

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.

--