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

[Brian Masney <bmasney@redhat.com>]

On Thu, Sep 25, 2025 at 02:14:14PM +0200, Maxime Ripard wrote:
> 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?

Stephen asked me to not introduce a new clk op, however I don't see a
clean way to do this any other way. Personally, I think that we need a
new clk op for this use case for the reasons I outlined on the cover
letter. I am open for suggestions about alternative ways, and will
gladly make modifications. This is why I marked this series as RFC.
Patch 10 in this series is the main change of note here.

Additionally, the presence of the new op is a convenient way to also
signal to the clk core that these providers can use the v2 negotiation
logic. Otherwise, we'll need to introduce a flag somewhere else if we
want to support a v1/v2 negotiation logic across the clk tree.

As for 2), I negotiate the rate change from the top down. The new_rate
is propagated in the same manner as what's done today in the clk core
when a parent rate change occurs. I let it reuse the existing rate
change code that's currently in the clk core to minimize the change
that was introduced.

Regarding the clock table in 3), it could be done with what's there,
but there's the issue of the new clk_op. I posted this to get feedback
about that since I think we should settle on the core changes.

Brian