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@xxxxxxxxxx> 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
