[Freedreno] [RFC PATCH 1/2] clk: qcom: implement RCG2 'parked' clock support

[Dmitry Baryshkov]

On 07/10/2023 13:08, Bryan O'Donoghue wrote:
> On 07/10/2023 00:45, Konrad Dybcio wrote:
>> On 4.10.2023 14:52, Bryan O'Donoghue wrote:
>>> On 04/10/2023 13:08, Dmitry Baryshkov wrote:
>>>> On Wed, 4 Oct 2023 at 12:27, Bryan O'Donoghue
>>>> <bryan.odonoghue at linaro.org> wrote:
>>>>>
>>>>> On 04/10/2023 01:31, Dmitry Baryshkov wrote:
>>>>>> clk_rcg2_shared_ops implements support for the case of the RCG which
>>>>>> must not be completely turned off. However its design has one major
>>>>>> drawback: it doesn't allow us to properly implement the is_enabled
>>>>>> callback, which causes different kinds of misbehaviour from the CCF.
>>>>>>
>>>>>> Follow the idea behind clk_regmap_phy_mux_ops and implement the new
>>>>>> clk_rcg2_parked_ops. It also targets the clocks which must not be 
>>>>>> fully
>>>>>> switched off (and shared most of the implementation with
>>>>>> clk_rcg2_shared_ops). The major difference is that it requires 
>>>>>> that the
>>>>>> parent map doesn't conain the safe (parked) clock source. Instead 
>>>>>> if the
>>>>>> CFG_REG register points to the safe source, the clock is 
>>>>>> considered to
>>>>>> be disabled.
>>>>>
>>>>> Why not have a new bit in .flags ?
>>>>>
>>>>> Instead of lying about the clock being off, mark the clock as 
>>>>> "parked",
>>>>> or "safe parked" or whatever term we choose for it ?
>>>>
>>>> The main problem with adding flags doesn't fully scale. From the CCF
>>>> perspective, what should be the difference between parked and disabled
>>>> clocks? How should it treat the parked one?
>>>
>>> Exactly the same as a disabled clock, except you get a "parked" 
>>> instead of a "disabled" when looking up its state and you don't have to
>> The thing is that currently there's only the notion of "enabled"
>> or "not enabled".. Introducing a third state here would be the
>> jump from boolean to quantum logic!
>>
>> I think that abstracting this information away from Linux is not
>> an issue.. These clocks "can't be any more off", or the SoC will
>> explode badly and Linux will be unusable..
>>
>> Think of it like CPUs with a hypervisor, you shut them down, but
>> the physical number crunchers on the host CPU may not actually
>> get cut off from their power source, but there's no reason for
>> the VM to know that. That's probably what happens on our little
>> virtualized snapdragons anyway..
>>
>> Konrad
> 
> So not a state but a flag.
> 
> 1. The clock tree we declare _should_ be a fair and complete description
>     of the hardware clock tree.

Yes and no. We already have clocks not present in the tree for different 
reasons: being handled by the RPM(h), being critical to the platform 
integrity, being useless for Linux, etc.

> 
> 2. If we remove XO from some trees with the only indication of
>     differentiation being the callback you bind the tree to you need
>     someone reading the code both know about parking, derive that
>     information from reading the callback, which means you require that
>     person to read the code in detail and understand it.
> 
>     That's alot of tribal knowledge we are storing up there.

I think adding a huge comment should help. Because otherwise you sound 
like 'we should not expect kernel developers to read the code', which is 
not true.

> 
> 3. A different approach is to add a new CLK_DISABLE_PARKS_TO_XO - or
>     whatever name makes sense.
> 
>     a) The clock tree declared in the gcc, camcc, dispcc, videocc or
>        is correct and aligned with the documentation and silicon.
>        Right away this avoids patches sent to 'fixup' incomplete trees.
> 
>     b) When you look at a clock struct clk_branch_gcc.clk.hw.init.flags
>        there is a big dirty CLK_DOES_THIS_THING flag which doesn't
>        require a developer to have tribal knowledge about how we've
>        hacked up clock parking.

But the problem is that this flag is not generic at all. I think we will 
be damned and prosecuted if we try adding anything about PARK_TO_XO to 
<linux/clk-provider.h>.

And also there is always a question on the state integrity: if the clock 
is parented with the XO on the driver bootstrap, should CCF treat it as 
'parked' or as 'enabled, clocked by XO'?

> 
> My basic point here is the declaration of a parked clock should be 
> obvious, easy to understand and not lend itself to "helpful" patches to 
> "fix" the clock tree.

We already tried doing that... And it failed. For the PHY PIPE clocks we 
ended up doing exactly the same thing, because it simplified the code 
_a_lot_.

> Also consider precedent. When you want to quickly get your clock 
> controller up and running - you generally open existing upstream stuff 
> to clone and own as much as possible. A BIT_DIRTY_FLAG transmits more 
> information than a small callback with esoteric logic buried inside of 
> the disable path.
> 
> I agree with your point on a new state but similarly I think the 
> callback buries too much information away. IMO the top level clock 
> declaration - rather like the DT should as closely as possible declare 
> an accurate clock tree.
> 
> If we need to do special stuff to an individual tree, then CLK_FLAG it. 
> Are qcom clocks really the only clocks in the world that need to park to 
> XO on the disable path ?
> 
> Alternatively continue on with the callback but make the name more 
> instructive not "park" since we are dealing with people who have English 
> as a second language, third language. English is my first language but 
> still a "parked" clock means little to me except that like you and 
> Dmitry I work with qcom stuff so I understand it.
> 
> "disable_park_xo->clk_disable" or something - even still I think 
> removing XO from the clock tree is asking for trouble.

clk_rcg2_disable_parks_to_tcxo_ops ? Slightly ugly but I'm fine with that.

> 
> Start from the principle that gcc/camcc/dispcc clock trees should be 
> complete and work from there.
> 
> That's my 0.02 anyway.
> 

-- 
With best wishes
Dmitry