Message-ID: <mbbwnxoik3qhy6gcwglfdch2v2gdhz3uqoaeu3xujnec6uwnoy@lqexuvwyjyny>
Date: Fri, 21 Nov 2025 17:09:36 +0100
From: Maxime Ripard <mripard@...nel.org>
To: Brian Masney <bmasney@...hat.com>
Cc: Stephen Boyd <sboyd@...nel.org>, 
	Michael Turquette <mturquette@...libre.com>, Jonathan Corbet <corbet@....net>, 
	Russell King <linux@...linux.org.uk>, linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-doc@...r.kernel.org
Subject: Re: [PATCH RFC v4 00/12] clk: add support for v1 / v2 clock rate
 negotiation and kunit tests

On Fri, Nov 14, 2025 at 07:22:55PM -0500, Brian Masney wrote:
> 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 think I'm still confused about why we need negociate_rates in the
first place.

IIRC, Stephen's suggestion was something along the lines of "let's make
determine_rate optionally return a list of possible parent rates in
clk_rate_request and make the core find the intersection".

Why do we need to involve the driver in such a scenario, and using
negociate_rate in particular?

> 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.

Yeah, discussing it at plumbers would probably be a good idea, and maybe
try to record / transcribe the meeting so we can have the minutes too
somewhere?
Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)

Powered by blists - more mailing lists