Re: [RFC PATCH 0/5] clk: bcm: kona: Add bus clock support and prerequisite clocks

Alex Elder
2025-02-24T16:20:10Z

On 2/16/25 10:12 AM, Artur Weber wrote:
> This patchset does the following:
>
> - Introduce support for bus clocks. These are fairly similar to
> peripheral clocks, but only implement policy, gate and hyst.
>
> - Introduce support for prerequisite clocks; this way we can
> make peripheral clocks automatically enable their corresponding
> bus clocks.
>
> - Add matching bus clocks for BCM21664 peripheral clocks and update
> device tree bindings to match.
>
> The prerequisite clock portion of this patchset is adapted from an
> older attempt to add bus clocks[1], submitted by Alex Elder. I've
> retained his authorship on that commit.

That was a long time ago!

> Notably, Alex's patchset moved clock initialization to the prepare
> function. This seems to be incorrect; the prepare function gets called
> before the enable function, but not before "set rate"/"set parent"
> functions; thus, while clocks enabled fine, any configuration done
> before they were first enabled was broken. I ignored that part of
> the patchset and only kept the prerequisite clocks.

I think you're right.

> I would appreciate feedback on the prerequisite clock patch, hence
> why this patchset is marked as RFC.
>
> I wasn't able to find any other driver that does something like this,
> so I'm not sure if it's correct (especially since I had to switch from
> non-locking __clk_prepare and __clk_enable to the regular locking
> versions, as the non-locking versions are no longer public - they
> appear to have been replaced by clk_core counterparts, but those
> functions are not exported anywhere AFAICT).
>
> An alternative way to do this dependency would be to wrap every
> component with a relevant bus clock in a "simple-pm-bus" node
> with the bus clock in DT, but this seems rather unwieldy.

Yes I had the same thought, and ask about this on patch 3. I
can't comment on whether this notion of a prerequisite (that
is not its parent) makes sense though.

I didn't look at patch 1 or patch 5.

-Alex

>
> [1] https://lore.kernel.org/lkml/1402926007-4436-1-git-send-email-elder@linaro.org/
>
> Signed-off-by: Artur Weber <aweber.kernel@gmail.com>
> ---
> Alex Elder (1):
> clk: bcm281xx: implement prerequisite clocks
>
> Artur Weber (4):
> dt-bindings: clock: brcm,kona-ccu: Add BCM21664 bus clocks
> clk: bcm: kona: Add support for bus clocks
> clk: bcm21664: Add matching bus clocks for peripheral clocks
> ARM: dts: bcm2166x-common: Add matching bus clocks for peripheral clocks
>
> .../devicetree/bindings/clock/brcm,kona-ccu.yaml | 18 ++-
> arch/arm/boot/dts/broadcom/bcm2166x-common.dtsi | 28 +++--
> drivers/clk/bcm/clk-bcm21664.c | 107 ++++++++++++++++--
> drivers/clk/bcm/clk-kona-setup.c | 116 +++++++++++++++++++
> drivers/clk/bcm/clk-kona.c | 124 ++++++++++++++++++++-
> drivers/clk/bcm/clk-kona.h | 30 ++++-
> include/dt-bindings/clock/bcm21664.h | 19 +++-
> 7 files changed, 411 insertions(+), 31 deletions(-)
> ---
> base-commit: ffd294d346d185b70e28b1a28abe367bbfe53c04
> change-id: 20250212-kona-bus-clock-4297eefae940
>
> Best regards,