Re: [PATCH RFC v4 00/12] clk: add support for v1 / v2 clock rate negotiation and kunit tests

[Maxime Ripard <mripard@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2822 bytes --]

Hi Brian,

On Tue, Sep 23, 2025 at 10:39:19AM -0400, Brian Masney wrote:
> The Common Clock Framework is expected to keep a clock’s rate stable
> after setting a new rate with:
> 
>     clk_set_rate(clk, NEW_RATE);
> 
> Clock consumers do not know about the clock hierarchy, sibling clocks,
> or the type of clocks involved. However, several longstanding issues
> affect how rate changes propagate through the clock tree when
> CLK_SET_RATE_PARENT is involved, and the parent's clock rate is changed:
> 
> - A clock in some cases can unknowingly change a sibling clock's rate.
>   More details about this particular case are documented at:
>   https://lore.kernel.org/linux-clk/20250528-clk-wip-v2-v2-2-0d2c2f220442@redhat.com/
> 
> - No negotiation is done with the sibling clocks, so an inappropriate
>   or less than ideal parent rate can be selected.
> 
> A selection of some real world examples of where this shows up is at
> [1]. DRM needs to run at precise clock rates, and this issue shows up
> there, however will also show up in other subsystems that require
> precise clock rates, such as sound.
> 
> An unknown subset of existing boards are unknowingly dependent on the
> existing behavior, so it's risky to change the way the rate negotiation
> logic is done in the clk core.
> 
> This series adds support for v1 and v2 rate negotiation logic to the clk
> core. When a child determines that a parent rate change needs to occur
> when the v2 logic is used, the parent negotiates with all nodes in that
> part of the clk subtree and picks the first rate that's acceptable to
> all nodes.
> 
> Kunit tests are introduced to illustrate the problem, and are updated
> later in the series to illustrate that the v2 negotiation logic works
> as expected, while keeping compatibility with v1.
> 
> I marked this as a RFC since Stephen asked me in a video call to not
> add a new member to struct clk_core, however I don't see how to do this
> any other way.
> 
> - The clk core doesn’t, and shouldn’t, know about the internal state the
>   various clk providers.
> - Child clks shouldn’t have to know the internal state of the parent clks.
> - Currently this information is not exposed in any way to the clk core.

I recall from that video call that Stephen asked:

- to indeed not introduce a new op
- to evaluate the change from top to bottom, but to set it bottom to top
- to evaluate the rate by letting child clocks expose an array of the
  parent rates they would like, and to intersect all of them to figure
  out the best parent rate.

It looks like you followed none of these suggestions, so explaining why
you couldn't implement them would be a great first step.

Also, you sent an RFC, on what would you like a comment exactly?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]