Hi Maxime (and Stephen),

On Tue, Sep 30, 2025 at 01:28:52PM +0200, Maxime Ripard wrote:
> On Thu, Sep 25, 2025 at 10:20:24AM -0400, Brian Masney wrote:
> > 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.
> 
> So his suggestion was to base the whole logic on clk_ops.determine_rate.
> You're saying that it would violate parent/child abstraction. Can you
> explain why you think that is the case, because it's really not obvious
> to me.
> 
> Additionally, and assuming that we indeed need something similar to your
> suggestion, determinate_rate takes a pointer to struct clk_rate_request.
> Why did you choose to create a new op instead of adding the check_rate
> pointer to clk_rate_request?

Sorry about the delayed response. I've been busy with other projects at
work.

I attached a patch that puts the negotiate_rates member on struct
clk_rate_request instead of struct clk_ops. In order to get this to
work, it also required adding it to struct clk_core and
struct clk_init_data as well. I made this so that this patch applies
on top of this series.

I think the clk_rate_request approach is very ugly, and adding it to
struct clk_ops like I have it in this series is the way to go.

I'm giving a talk at Linux Plumbers in Tokyo next month:

    Fixing Clock Tree Propagation in the Common Clk Framework 
    https://lpc.events/event/19/contributions/2152/

Stephen will be there as well, and hopefully we can reach consensus
about an acceptable approach to fix this.

My round_rate to determine_rate conversion will drop one member from
struct clk_ops, so maybe that'll help, and we can have a trade?
There's currently only one outstanding patch series remaining in
drivers/phy that's blocking me from posting what I have to remove
round_rate from the clk core:

https://lore.kernel.org/linux-clk/20251106-phy-clk-route-rate-v2-resend-v1-0-e2058963bfb1@redhat.com/T/

I'll bug Vinod on email again so that we can hopefully get that in for
v6.19. (No response on IRC yesterday.) If not, I see that he's giving a
talk at Plumbers and I'll bug him in person after his talk.

Brian