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

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
From 469c29a4d14a83155a280df4679a43ad4af2e1b4 Mon Sep 17 00:00:00 2001
From: Brian Masney <bmasney@redhat.com>
Date: Fri, 14 Nov 2025 18:45:06 -0500
Subject: [PATCH] HACK: clk: move negotiate_rates member from struct clk_ops to
 struct clk_rate_request
Content-type: text/plain

Demonstrate one way to move the negotiate_rates member from struct
clk_ops to struct clk_rate_request. Personally I think it's much cleaner
to have this on struct clk_ops.

Signed-off-by: Brian Masney <bmasney@redhat.com>
---
 Documentation/driver-api/clk.rst |  8 +++++---
 drivers/clk/clk.c                | 11 ++++++++---
 drivers/clk/clk_test.c           | 32 +++++++++++++++++++++++++-------
 include/linux/clk-provider.h     | 19 ++++++++++++-------
 4 files changed, 50 insertions(+), 20 deletions(-)

diff --git a/Documentation/driver-api/clk.rst b/Documentation/driver-api/clk.rst
index c46ee62ba5bd..47ed511432db 100644
--- a/Documentation/driver-api/clk.rst
+++ b/Documentation/driver-api/clk.rst
@@ -75,9 +75,6 @@ the operations defined in clk-provider.h::
 		void		(*disable)(struct clk_hw *hw);
 		int		(*is_enabled)(struct clk_hw *hw);
 		void		(*disable_unused)(struct clk_hw *hw);
-		bool            (*negotiate_rates)(struct clk_hw *hw,
-					           struct clk_rate_request *req,
-					           bool (*check_rate)(struct clk_core *, unsigned long));
 		unsigned long	(*recalc_rate)(struct clk_hw *hw,
 						unsigned long parent_rate);
 		long		(*round_rate)(struct clk_hw *hw,
@@ -103,6 +100,11 @@ the operations defined in clk-provider.h::
 					      struct dentry *dentry);
 	};
 
+Note: The negotiate_rates callback is not part of struct clk_ops. Instead, it
+is specified in struct clk_init_data during clock registration and is copied
+to struct clk_rate_request for use during rate negotiations. It is invoked
+through the rate request structure rather than through ops.
+
 Hardware clk implementations
 ============================
 
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 926b7d4b9ab8..8014eb719266 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -130,6 +130,9 @@ struct clk_parent_map {
 struct clk_core {
 	const char		*name;
 	const struct clk_ops	*ops;
+	bool			(*negotiate_rates)(struct clk_hw *hw,
+						   struct clk_rate_request *req,
+						   bool (*check_rate)(struct clk_core *, unsigned long));
 	struct clk_hw		*hw;
 	struct module		*owner;
 	struct device		*dev;
@@ -427,8 +430,8 @@ static bool clk_core_is_enabled(struct clk_core *core)
 
 static int clk_core_use_v2_rate_negotiation(struct clk_core *core)
 {
-	bool has_v2_ops = core->ops->negotiate_rates ||
-	                  (core->parent && core->parent->ops->negotiate_rates);
+	bool has_v2_ops = core->negotiate_rates ||
+		(core->parent && core->parent->negotiate_rates);
 
 	return has_v2_ops && modparam_clk_v2_rate_negotiation;
 }
@@ -1713,6 +1716,7 @@ static void clk_core_init_rate_req(struct clk_core * const core,
 
 	req->core = core;
 	req->rate = rate;
+	req->negotiate_rates = core->negotiate_rates;
 	clk_core_get_boundaries(core, &req->min_rate, &req->max_rate);
 
 	parent = core->parent;
@@ -2504,7 +2508,7 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 		struct clk_rate_request req;
 
 		clk_core_init_rate_req(top, &req, top->new_rate);
-		if (!top->ops->negotiate_rates(top->hw, &req, clk_check_rate))
+		if (!req.negotiate_rates || !req.negotiate_rates(top->hw, &req, clk_check_rate))
 			return NULL;
 
 		clk_accept_rate_negotiations(top);
@@ -4511,6 +4515,7 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
 		goto fail_ops;
 	}
 	core->ops = init->ops;
+	core->negotiate_rates = init->negotiate_rates;
 
 	core->dev = dev;
 	clk_pm_runtime_init(core);
diff --git a/drivers/clk/clk_test.c b/drivers/clk/clk_test.c
index ff53b8bdf872..be2a49d2e446 100644
--- a/drivers/clk/clk_test.c
+++ b/drivers/clk/clk_test.c
@@ -122,7 +122,6 @@ static const struct clk_ops clk_dummy_rate_ops = {
 	.recalc_rate = clk_dummy_recalc_rate,
 	.determine_rate = clk_dummy_determine_rate,
 	.set_rate = clk_dummy_set_rate,
-	.negotiate_rates = clk_dummy_negotiate_rates,
 };
 
 static const struct clk_ops clk_dummy_maximize_rate_ops = {
@@ -218,7 +217,6 @@ static const struct clk_ops clk_dummy_div_ops = {
 	.recalc_rate = clk_dummy_div_recalc_rate,
 	.determine_rate = clk_dummy_div_determine_rate,
 	.set_rate = clk_dummy_div_set_rate,
-	.negotiate_rates = clk_dummy_div_negotiate_rates,
 };
 
 struct clk_dummy_gate {
@@ -755,6 +753,26 @@ struct clk_rate_change_sibling_div_div_context {
 static int clk_rate_change_sibling_div_div_test_init(struct kunit *test)
 {
 	struct clk_rate_change_sibling_div_div_context *ctx;
+	struct clk_init_data parent_init = {
+		.name = "parent",
+		.ops = &clk_dummy_rate_ops,
+		.negotiate_rates = clk_dummy_negotiate_rates,
+		.flags = 0,
+	};
+	struct clk_init_data child1_init = {
+		.name = "child1",
+		.ops = &clk_dummy_div_ops,
+		.negotiate_rates = clk_dummy_div_negotiate_rates,
+		.flags = CLK_SET_RATE_PARENT,
+		.num_parents = 1,
+	};
+	struct clk_init_data child2_init = {
+		.name = "child2",
+		.ops = &clk_dummy_div_ops,
+		.negotiate_rates = clk_dummy_div_negotiate_rates,
+		.flags = CLK_SET_RATE_PARENT,
+		.num_parents = 1,
+	};
 	int ret;
 
 	ctx = kunit_kzalloc(test, sizeof(*ctx), GFP_KERNEL);
@@ -762,22 +780,22 @@ static int clk_rate_change_sibling_div_div_test_init(struct kunit *test)
 		return -ENOMEM;
 	test->priv = ctx;
 
-	ctx->parent.hw.init = CLK_HW_INIT_NO_PARENT("parent", &clk_dummy_rate_ops, 0);
+	ctx->parent.hw.init = &parent_init;
 	ctx->parent.negotiate_step_size = 1 * HZ_PER_MHZ;
 	ctx->parent.rate = 24 * HZ_PER_MHZ;
 	ret = clk_hw_register_kunit(test, NULL, &ctx->parent.hw);
 	if (ret)
 		return ret;
 
-	ctx->child1.hw.init = CLK_HW_INIT_HW("child1", &ctx->parent.hw, &clk_dummy_div_ops,
-					     CLK_SET_RATE_PARENT);
+	child1_init.parent_hws = (const struct clk_hw*[]) { &ctx->parent.hw };
+	ctx->child1.hw.init = &child1_init;
 	ctx->child1.div = 1;
 	ret = clk_hw_register_kunit(test, NULL, &ctx->child1.hw);
 	if (ret)
 		return ret;
 
-	ctx->child2.hw.init = CLK_HW_INIT_HW("child2", &ctx->parent.hw, &clk_dummy_div_ops,
-					     CLK_SET_RATE_PARENT);
+	child2_init.parent_hws = (const struct clk_hw*[]) { &ctx->parent.hw };
+	ctx->child2.hw.init = &child2_init;
 	ctx->child2.div = 1;
 	ret = clk_hw_register_kunit(test, NULL, &ctx->child2.hw);
 	if (ret)
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 9041b17ba99e..bc162afc8cd8 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -53,6 +53,9 @@ struct dentry;
  *			requested constraints.
  * @best_parent_hw:	The most appropriate parent clock that fulfills the
  *			requested constraints.
+ * @negotiate_rates:	When a child clk requests a new rate that requires a rate
+ *			change from the parent, this negotiates a new parent rate that's
+ *			acceptable to all of the children.
  *
  */
 struct clk_rate_request {
@@ -62,6 +65,9 @@ struct clk_rate_request {
 	unsigned long max_rate;
 	unsigned long best_parent_rate;
 	struct clk_hw *best_parent_hw;
+	bool (*negotiate_rates)(struct clk_hw *hw,
+				struct clk_rate_request *req,
+				bool (*check_rate)(struct clk_core *, unsigned long));
 };
 
 void clk_hw_init_rate_request(const struct clk_hw *hw,
@@ -129,10 +135,6 @@ struct clk_duty {
  * @restore_context: Restore the context of the clock after a restoration
  *		of power.
  *
- * @negotiate_rates: When a child clk requests a new rate that requires a rate
- *		change from the parent, this negotiates a new parent rate that's
- *		acceptable to all of the children.
- *
  * @recalc_rate: Recalculate the rate of this clock, by querying hardware. The
  *		parent rate is an input parameter.  It is up to the caller to
  *		ensure that the prepare_mutex is held across this call. If the
@@ -246,9 +248,6 @@ struct clk_ops {
 	void		(*disable_unused)(struct clk_hw *hw);
 	int		(*save_context)(struct clk_hw *hw);
 	void		(*restore_context)(struct clk_hw *hw);
-	bool		(*negotiate_rates)(struct clk_hw *hw,
-					   struct clk_rate_request *req,
-					   bool (*check_rate)(struct clk_core *, unsigned long));
 	unsigned long	(*recalc_rate)(struct clk_hw *hw,
 					unsigned long parent_rate);
 	long		(*round_rate)(struct clk_hw *hw, unsigned long rate,
@@ -295,6 +294,9 @@ struct clk_parent_data {
  *
  * @name: clock name
  * @ops: operations this clock supports
+ * @negotiate_rates: When a child clk requests a new rate that requires a rate
+ *		change from the parent, this negotiates a new parent rate that's
+ *		acceptable to all of the children.
  * @parent_names: array of string names for all possible parents
  * @parent_data: array of parent data for all possible parents (when some
  *               parents are external to the clk controller)
@@ -306,6 +308,9 @@ struct clk_parent_data {
 struct clk_init_data {
 	const char		*name;
 	const struct clk_ops	*ops;
+	bool			(*negotiate_rates)(struct clk_hw *hw,
+						   struct clk_rate_request *req,
+						   bool (*check_rate)(struct clk_core *, unsigned long));
 	/* Only one of the following three should be assigned */
 	const char		* const *parent_names;
 	const struct clk_parent_data	*parent_data;
-- 
2.51.1

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

On Fri, Nov 21, 2025 at 11:09 AM Maxime Ripard <mripard@kernel.org> wrote:
> > 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?

The talk will be recorded, plus I'm sure there will be discussion
after my talk. I'll post a summary to this thread with the next steps
after Plumbers.

Brian

Hi all,

On Fri, Nov 21, 2025 at 03:35:10PM -0500, Brian Masney wrote:
> On Fri, Nov 21, 2025 at 11:09 AM Maxime Ripard <mripard@kernel.org> wrote:
> > > 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?
> 
> The talk will be recorded, plus I'm sure there will be discussion
> after my talk. I'll post a summary to this thread with the next steps
> after Plumbers.

I presented a talk at Linux Plumbers in Tokyo last week about this patch
set:

    Fixing Clock Tree Propagation in the Common Clk Framework
    https://www.youtube.com/watch?v=R8TytDzlcFs
    https://lpc.events/event/19/contributions/2152/
    My slide deck is on the second link. I didn't present all of the
    slides.

In summary, I made the scope of this patch set too big, and this is an
intractable problem the way I currently have this setup. Some boards
are currently unknowingly dependent on the existing behavior, and we
need to keep things the way they currently are. We can detect when rates
are unknowingly changed by a sibling, and log a warning, however some
systems are configured to panic the system on warning, so we don't want
to go that route.

Most clock rates are fine to change with the existing behavior. It's
just some clocks that are dependent on a precise clock rate, such as DRM
and sound, that are affected by this.

The way forward on this is to leave the existing behavior in the clock
core, and fix this in the clock providers themselves. When a rate change
occurs, it can walk down that portion of the subtree inside the clk
provider via some new helpers that will be added to the clk core, and
ensure that the clocks that need precise rates will have their rates,
and their parent (if needed), updated as necessary. An array of rate
changes can be added to struct clk_rate_request, and the clk core can
update the clocks in that order. So it'll be up to the clk providers to
ensure that the array is populated in the correct order.

I'm going to get this working first in kunit, and I'll post a new
version of this patch set with these changes. Once that's done, I'll
work with Maxime and some other folks to find a board that has this
problem, and I'll ensure my new clk patch set is able to fix the issue.

Thank you to everyone that attended and provided feedback. Please reply
here if I missed something.


Separately, I talked to Stephen about ways that I can help him with clk
maintenance. Here's some information from my notes:

- I converted from round_rate to determine_rate() across most of the
  kernel tree in over 200 patches. However, the only remaining patch
  set is to the phy subsystem. The patches have received Reviewed-by's,
  however I haven't been able to get the phy maintainer to pick up the
  patches. Stephen mentioned he can pick them up. There was a merge
  conflict against the latest linux-next, so I posted a new version that
  addresses the merge conflict.

  https://lore.kernel.org/linux-clk/20251212-phy-clk-round-rate-v3-0-beae3962f767@redhat.com/T/#t

  Here's the patch set that actually removes round_rate() from the clk
  core.

  https://lore.kernel.org/linux-clk/20251212-clk-remove-round-rate-v1-0-5c3d5f3edc78@redhat.com/

  We still occasionally get people that try to add new round_rate
  implementations. I try to catch them when the patches are posted,
  however I miss some across the tree and will post a patch when it hits
  linux-next. 

  Someone two days ago posted a patch that adds a new round_rate(), so
  it'd ideally be nice to get my two patch sets above into linux-next to
  put a stop to this.

  https://lore.kernel.org/linux-clk/20251216-dr1v90-cru-v3-3-52cc938d1db0@pigmoral.tech/

  I commented on that patch to drop the round_rate.

- There are maybe a dozen or so determine_rate() implementations where
  it just returns 0, and it lets the firmware deal with setting the
  appropriate rate. Stephen suggested that we add a new flag to the
  clk core so that we can drop these empty determine_rate()
  implementations. We didn't talk about a name, but at the moment I'm
  leaning towards CLK_IS_FW_MANAGED. If that's set, then the clk core
  will not allow determine_rate to be set. I'm open to other
  suggestions for a name.

- I will continue to help review changes to the clk core. As for the
  clk providers themselves, I don't have access to the data sheets, and
  I can't really review those in detail. However, I think it would be
  nice if we add some extra clk-specific checks that we can run against
  patches. Look for common issues that come up in review. Look for cases
  where some of the helpers in clk-provider.h can possibly be used.
  Chen-Yu Tsai pointed out that's may not be entirely accurate in all
  cases, but we can at least warn about it.

  Another case I thought of is if someone posts a patch set where the
  clocks referenced in the dt bindings don't match what's actually in
  the clock provider itself.

  Make this as a clk-specific check script, and ideally see if we can
  hook that script into checkpatch.pl for clk-specific patches. My
  perl days are long over though, and I'd like to make the clk check
  script in python.

  Anyways, with a pre flight check like this, I could help review clk
  drivers themselves to look for anything else that's out of the
  ordinary.

It was good to meet so many people in person at Japan.

Brian