Re: [PATCH 03/15] clk: qcom: gcc-sm6375: Unregister critical clocks

[Konrad Dybcio]

On 9.08.2023 18:52, Konrad Dybcio wrote:
> On 19.07.2023 10:43, Johan Hovold wrote:
>> On Tue, Jul 18, 2023 at 09:23:52AM -0700, Bjorn Andersson wrote:
>>> On Tue, Jul 18, 2023 at 03:26:51PM +0200, Konrad Dybcio wrote:
>>>> On 18.07.2023 15:20, Johan Hovold wrote:
>>>>> On Mon, Jul 17, 2023 at 05:19:10PM +0200, Konrad Dybcio wrote:
>>>>>> Some clocks need to be always-on, but we don't really do anything
>>>>>> with them, other than calling enable() once and telling Linux they're
>>>>>> enabled.
>>>>>>
>>>>>> Unregister them to save a couple of bytes and, perhaps more
>>>>>> importantly, allow for runtime suspend of the clock controller device,
>>>>>> as CLK_IS_CRITICAL prevents the latter.
>>>>>
>>>>> But this doesn't sound right. How can you disable a controller which
>>>>> still has clocks enabled?
>>>>>
>>>>> Shouldn't instead these clocks be modelled properly so that they are
>>>>> only enabled when actually needed?
>>>> Hm.. We do have clk_branch2_aon_ops, but something still needs to
>>>> toggle these clocks.
>>>>
>>>
>>> Before we started replacing these clocks with static votes, I handled
>>> exactly this problem in the turingcc-qcs404 driver by registering the
>>> ahb clock with a pm_clk_add(). The clock framework will then
>>> automagically keep the clock enabled around operations, but it will also
>>> keep the runtime state active as long as the clock is prepared.
>>>
>>> As mentioned in an earlier reply today, there's no similarity to this in
>>> the reset or gdsc code, so we'd need to add that in order to rely on
>>> such mechanism.
>>
>> This reminds me of:
>>
>> 	4cc47e8add63 ("clk: qcom: gdsc: Remove direct runtime PM calls")
>>
>> which recently removed a broken attempt to implement this for gdscs.
>>
>> Just stumbled over GENPD_FLAG_PM_CLK which may provide a way forward at
>> least for genpd (but see below).
>>
>>>> I *think* we could leave a permanent vote in probe() without breaking
>>>> runtime pm! I'll give it a spin bit later..
>>>>
>>>
>>> Modelling the AHB clock in DT and putting a devm_clk_get_enabled() would
>>> properly connect the two, and thereby handle probe order between the two
>>> clock controllers.
>>
>> Yeah, this dependency really should be described eventually.
>>
>>> But it would prevent the power-domain of the parent provider to ever
>>> suspending. Using pm_clk_add() this would at least depend on client
>>> votes.
>>
>> IIUC using pm_clk_add() would also prevent the parent from suspending
>> due to that resume call in clk_prepare().
>>
>> And this mechanism is also used for GENPD_FLAG_PM_CLK...
> So.. how do we go about solving the issue that this patch tried to
> address?
I see things this way:

- clock controllers (non-gcc) that use magic writes today,
  they should stay as they are to avoid dramatic spaghetti wrt
  dt backwards compatibility

- clock controllers (non-gcc) that use CLK_IS_CRITICAL are
  transitioned to magic writes to skip the PM code fluff which
  prevents shutting down the PDs if any clock is critical and
  for uniformity with point 1 (as the device trees still don't
  contain any references to the necessary clocks)

- new clock controllers are modeled with use of pm_clk and with
  proper device tree references

FWIW Qualcomm nowadays just keep these clocks always on (answering
Johan's question - they're either off-from-Linux-POV-not-really-in-
hardware or these clocks retain the enable bit, I don't know) in
their shipping downstream kernels, the power leak is probably
minimal, but if we can avoid it, every nanowatt is to our advantage

Konrad