On 2/25/25 12:48 PM, Artur Weber wrote:
On 24.02.2025 17:20, Alex Elder wrote:
On 2/16/25 10:12 AM, Artur Weber wrote:
From: Alex Elder <elder@xxxxxxxxxx>
Allow a clock to specify a "prerequisite" clock, identified by its
name. The prerequisite clock must be prepared and enabled before a
clock that depends on it is used. In order to simplify locking, we
require a clock and its prerequisite to be associated with the same
CCU. (We'll just trust--but not verify--that nobody defines a cycle
of prerequisite clocks.)
Rework the KONA_CLK() macro, and define a new KONA_CLK_PREREQ()
variant that allows a prerequisite clock to be specified.
Signed-off-by: Alex Elder <elder@xxxxxxxxxx>
--- Artur: rebase on v6.13, move prereq prepare/unprepare to main
prepare/unprepare functions, use locking versions of clk_prepare
and clk_enable since the non-locking versions are no longer
public ---
Signed-off-by: Artur Weber <aweber.kernel@xxxxxxxxx>
I'm surprised there is no prepare function for the peripheral
clocks.
Not sure I follow - there is a prepare function for peri clocks, the
I meant *before* your patch, there is no prepare function.
same one as for all Kona clocks - kona_clk_prepare. Though it only
enables the prereq clock... I assume you mean "prepare function specific
to peripheral clocks", in which case - what should a prepare function
specifically for peripheral clocks do?
It's just that the prepare function can block, but the
enable function cannot. I see __ccu_wait_bit() just
loops until the register indicates an update completed,
so I guess there's no blocking.
As I mentioned in the cover letter, clock initialization has to be done
explicitly at clock init time, since it's required for later operations
like setting the clock rate/parent that are done *before* the first call
to the prepare function happens; otherwise we get errors like this:
[ 4.740000] [ T39] kona_peri_clk_set_parent: trigger failed for sdio1
[ 4.760000] [ T39] kona_peri_clk_set_rate: gating failure for sdio1
[ 5.650000] [ T36] kona_peri_clk_set_parent: trigger failed for sdio3
[ 5.670000] [ T36] kona_peri_clk_set_rate: gating failure for sdio3
(I did consider moving the relevant clock initialization to a
"clk_ops.init" function, but left it out of this patchset for brevity.
Might consider actually doing that...)
The prequisite clock should separate the prepare and the
enable functionality. Right now you have kona_clk_prepare()
doing both. Instead, a clock's prepare function should
prepare its prerequisite (if any). Then its enable function
should enable its parent.
I copied this behavior from the original patch; there the
prereq_prepare function both prepared and enabled the relevant
prerequisite clock. Indeed doing each step (prepare/enable/disable/
unprepare) in the relevant functions would probably make more sense.
I hadn't worked on a clock driver before then, and I think I
got pulled off that project before this series could be accepted
(or even reviewed?). Maybe I just proposed what the vendor
kernel did. In any case, *today* I'd say they should be separated.
Should all the users of peripheral clocks just also be required
to specify the bus clocks as well? I suppose that doesn't
encode the prerequisite property (bus comes before peripheral);
is that truly a requirement?
I see a few problems with that:
- Most drivers only take one clock - that clock is pretty much always
the peripheral clock (with the only exception being usb_otg_ahb which
is used for the USB OTG controller).
- Even if they supported both clocks, they would likely just switch them
on/off at the same time as the equivalent peri clock.
I mostly figured a clock dependency mechanism would work here since it's
what the vendor kernel does - it allows for specifying a dependent clock
and enables/disables it whenever the clock with the dependency is
enabled/disabled.
I don't really expect that the bus clock *must* be enabled
before the peripheral clock. The requirement is probably
just that both must be (prepared and) enabled before using
the peripheral, and order doesn't matter. And in that case
there's no need to enforce the ordering.
An alternative option would be to handle the dependency in the device
tree, using "simple-pm-bus" nodes, either:
- Wrapping around each node for a subdevice that uses a peripheral
clock. This is rather unwieldy since it means a lot of subnodes for
the most basic of peripherals (sdio1-4, uartb(2,3), etc.). (Also,
managing the "ranges" properties for all of these sub-busses would
get annoying unless we specify empty ranges, which according to the
bindings "is only appropriate if the child node is another 'simple-
bus' or similar."[1].)
The above sounds wrong to me.
- Using the simple-bus nodes that the BCM2166x DTSI already uses, and
connecting the relevant bus clock for all the components to them,
and switching the compatible to "simple-pm-bus". It's less granular,
but is probably the most sensible option if we go the DT route.
As for whether enabling a bus clock before a peripheral clock is
required... I went ahead and tested it, and the results seem to hint
that it's not.
- I seem to remember that not having the bus clock initialized caused
some failures on peri clocks, but I can't reproduce this anymore.
Most likely I'm mistaking it for policy bit setup, which is required
for gating to work (I have another commit ready to add policy bits
to all BCM21664 clocks, which I'm planning to send after this
patchset). And besides, just initializing bus clocks after peri
clocks seems to work given that's how it's set up right now.
- Also, it seems that all clocks defined with HW_SW_GATE are gated
on by default, and this state is never cleared afterwards, so all the
bus clocks I added in the BCM21664 bus clock commit are already
enabled at startup... after the peri clocks. And given that everything
works fine, it's probably OK.
Now that I think about it - maybe it would be easier to just keep things
the way they are; drop the prerequisite clock mechanism, let the
driver enable bus clocks by default, then if we wanted to explicitly
control the bus clocks, use the second DT approach I mentioned. If we
ever want to squeeze every last bit of power savings or for any other
potential benefit of keeping bus clocks off, we can revisit this
mechanism later.
I'm far out of the loop on the Broadcom drivers. If the hardware
requires no actual hierarchy/ordering of these clocks, then none
should be implied (by DT, or by the code). And if that's the case,
having the peripherals depend on both the bus and peripheral clocks
probably makes sense.
-Alex
Best regards
Artur
[1] https://github.com/devicetree-org/dt-schema/blob/
ed9190d20f146d13e262cc9138506326f7d4da91/dtschema/schemas/simple-
bus.yaml#L60-L69
