Message-ID: <238b8798-1c6f-4f60-b30e-766142a09306@gmail.com>
Date: Tue, 25 Feb 2025 19:48:20 +0100
From: Artur Weber <aweber.kernel@...il.com>
To: Alex Elder <elder@...cstar.com>,
 Michael Turquette <mturquette@...libre.com>, Stephen Boyd
 <sboyd@...nel.org>, Florian Fainelli <florian.fainelli@...adcom.com>,
 Ray Jui <rjui@...adcom.com>, Scott Branden <sbranden@...adcom.com>,
 Broadcom internal kernel review list
 <bcm-kernel-feedback-list@...adcom.com>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>
Cc: Alex Elder <elder@...nel.org>, Stanislav Jakubek
 <stano.jakubek@...il.com>, linux-clk@...r.kernel.org,
 linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
 ~postmarketos/upstreaming@...ts.sr.ht
Subject: Re: [PATCH RFC 3/5] clk: bcm281xx: implement prerequisite clocks

On 24.02.2025 17:20, Alex Elder wrote:
> On 2/16/25 10:12 AM, Artur Weber wrote:
>> From: Alex Elder <elder@...nel.org>
>>
>> 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@...aro.org>
>> --- 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@...il.com>
> 
> 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
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?

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.

> 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.

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].)

- 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.

Best regards
Artur

[1] https://github.com/devicetree-org/dt-schema/blob/ed9190d20f146d13e262cc9138506326f7d4da91/dtschema/schemas/simple-bus.yaml#L60-L69

Powered by blists - more mailing lists