Re: [PATCH v8 0/8] clk: add support for v1 / v2 clock rate negotiation and kunit tests

[Miquel Raynal <miquel.raynal@bootlin.com>]

On 27/03/2026 at 15:31:55 -04, Brian Masney <bmasney@redhat.com> 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.
>
> - 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.
>
> This series introduces kunit tests to illustrate the current behavior in
> the clk core. As discussed at Linux Plumbers Conference 2025 in Tokyo
> [2], it was suggested to move the negotiation logic into the clk
> providers themselves so that only the clks with this issue can have
> their rate preserved, and add some common helpers to the clk core.

I could succesfully test this series with minor adjustments:

* The LCM calculation must happen on core clocks instead of hw clocks
  (see my other answer in this thread).

* The imx8mp LDB clock shall set the CLK_SET_RATE_PARENT flag, otherwise
  it does not forward the rate change:

--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -751,9 +751,9 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
-       hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00);
+       hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite_bus_flags("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00, CLK_SET_RATE_PARENT);

* The imx8mp LDB and media_pix[1,2] clocks need to be flagged as
  supporting the new v2 rate calculation introduced by Brian.

--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -689,7 +689,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)

-       hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300, CLK_SET_RATE_PARENT);
+       hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300, CLK_SET_RATE_PARENT | CLK_V2_RATE_NEGOTIATION);
 
-       hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT);
+       hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT | CLK_V2_RATE_NEGOTIATION);

-       hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite_bus_flags("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00, CLK_SET_RATE_PARENT);
+       hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite_bus_flags("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00, CLK_SET_RATE_PARENT | CLK_V2_RATE_NEGOTIATION);

To be fairly honest, the discussion at LPC about how to handle this case
(and the transition) did not exist because time was flying but I do not
generally like the approaches were we introduce "the good" alternative
without enabling it by default. It just makes the transitions endless
and more painful for people who already suffer from a problem. I am
fine keeping the old behaviour available, but I would be in favour of
making the legacy way available with an explicity flag, rather than the
opposite. We may break users, yes, but otherwise we will have to support
the two calculations forever and nobody will ever notice the new
alternative.

That being said, it works! So:

Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
Tested-by: Miquel Raynal <miquel.raynal@bootlin.com>

Thanks,
Miquèl