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.

Changes since v3:
https://lore.kernel.org/r/20250812-clk-tests-docs-v3-0-054aed58dcd3@redhat.com
- Update clk_core struct members (Maxime)
- Add v2 rate negotiation logic and additional kunit tests
- Drop clk_dummy_rate_mhz() in kunit tests; use HZ_PER_MHZ

[1] https://lore.kernel.org/lkml/20230825-pll-mipi_keep_rate-v1-0-35bc43570730@oltmanns.dev/
    https://lore.kernel.org/linux-kernel/20230807-pll-mipi_set_rate_parent-v6-0-f173239a4b59@oltmanns.dev/
    https://lore.kernel.org/all/20241114065759.3341908-1-victor.liu@nxp.com/
    https://lore.kernel.org/linux-clk/20241121-ge-ian-debug-imx8-clk-tree-v1-0-0f1b722588fe@bootlin.com/

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
Brian Masney (12):
      clk: add kernel docs for struct clk_core
      clk: test: convert constants to use HZ_PER_MHZ
      clk: test: introduce clk_dummy_div for a mock divider
      clk: test: introduce test suite for sibling rate changes on a divider
      clk: test: introduce clk_dummy_gate for a mock gate
      clk: test: introduce test suite for sibling rate changes on a gate
      clk: test: introduce helper to create a mock mux
      clk: test: introduce test variation for sibling rate changes on a mux
      clk: test: introduce test variation for sibling rate changes on a gate/mux
      clk: add support for v2 rate negotiation
      clk: test: introduce negotiate_rates() op for clk_dummy and clk_dummy_div
      clk: test: update divider kunit tests for v1 and v2 rate negotiation

 Documentation/admin-guide/kernel-parameters.txt |  15 +
 Documentation/driver-api/clk.rst                |   3 +
 drivers/clk/clk.c                               | 201 ++++++-
 drivers/clk/clk_test.c                          | 694 ++++++++++++++++++++----
 include/linux/clk-provider.h                    |   7 +
 include/linux/clk.h                             |  20 +
 6 files changed, 835 insertions(+), 105 deletions(-)
---
base-commit: 8f5ae30d69d7543eee0d70083daf4de8fe15d585
change-id: 20250806-clk-tests-docs-3c398759e998

Best regards,
-- 
Brian Masney <bmasney@redhat.com>

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

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

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?

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

Again, why can't we add a pointer to that array in clk_rate_request?

Maxime