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

[Alex Elder <elder@riscstar.com>]

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,