Message-ID: <568634dc-9b4c-4ba1-84fb-0433a34262e0@riscstar.com>
Date: Tue, 25 Feb 2025 13:35:33 -0600
From: Alex Elder <elder@...cstar.com>
To: Artur Weber <aweber.kernel@...il.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 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@...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

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

Powered by blists - more mailing lists