[PATCH 00/13] imx8mp: first clock propagation attempt (for LVDS)

[PATCH 00/13] imx8mp: first clock propagation attempt (for LVDS)

[Benjamin Bara]

Hi!

Target of this series is to dynamically set the rate of video_pll1 to
the required LVDS clock rate(s), which are configured by the panel, and
the lvds-bridge respectively.

Some background:
The LVDS panel requires two clocks: the crtc clock and the lvds clock.
The lvds rate is always 7x the crtc rate. On the imx8mp, these are
assigned to media_disp2_pix and media_ldb, which are both
clk-composite-8m. The rates are set by drm_client_modeset_commit() (and
later by fsl_ldb_atomic_enable()), and the fsl-ldb driver, first crtc,
then lvds. The parent is typically assigned to video_pll1, which is a
clk-pll14xx (pll1443x).

The main problem:
As the clk-composite-8m currently doesn't support CLK_SET_RATE_PARENT,
the crtc rate is not propagated to video_pll1, and therefore must be
assigned in the device-tree manually.

The idea:
Enable CLK_SET_RATE_PARENT, at least for media_disp2_pix and media_ldb.
When this is done, ensure that the pll1443x can be re-configured,
meaning it ensures that an already configured rate (crtc rate) is still
supported when a second child requires a different rate (lvds rate). As
the children have divider, the current approach is straight forward by
calculating the LCM of the required rates. During the rate change of the
PLL, it must ensure that all children still have the configured rate at
the end (and maybe also bypass the clock while doing so?). This is done
by implementing a notifier function for the clk-composite-8m. The tricky
part is now to find out if the rate change was intentional or not. This
is done by adding the "change trigger" to the notify data. In our case,
we now can infer if we aren't the change trigger, we need to keep the
existing rate after the PLL's rate change. We keep the existing rate by
modifying the new_rate of the clock's core, as we are quite late in an
already ongoing clock change process.

Future work:
The re-configuration of the PLL can definitely be improved for other use
cases where the children have more fancy inter-dependencies. That's one
of the main reasons I currently only touched the mentioned clocks.
Additionally, it might make sense to automatically re-parent if a
different possible parent suits better.
For the core part, I thought about extending my "unintentional change
check" so that the core ensures that the children keep the configured
rate, which might not be easy as the parent could be allowed to "round",
but it's not clear (at least to me yet) how much rounding is allowed. I
found a similar discussion posted here[1], therefore added Frank and
Maxime.

Thanks & regards,
Benjamin

[1] https://lore.kernel.org/lkml/20230825-pll-mipi_keep_rate-v1-0-35bc43570730@oltmanns.dev/

---
Benjamin Bara (13):
      arm64: dts: imx8mp: lvds_bridge: use root instead of composite
      arm64: dts: imx8mp: re-parent IMX8MP_CLK_MEDIA_MIPI_PHY1_REF
      clk: implement clk_hw_set_rate()
      clk: print debug message if parent change is ignored
      clk: keep track of the trigger of an ongoing clk_set_rate
      clk: keep track if a clock is explicitly configured
      clk: detect unintended rate changes
      clk: divider: stop early if an optimal divider is found
      clk: imx: pll14xx: consider active rate for re-config
      clk: imx: composite-8m: convert compute_dividers to void
      clk: imx: composite-8m: implement CLK_SET_RATE_PARENT
      clk: imx: imx8mp: allow LVDS clocks to set parent rate
      arm64: dts: imx8mp: remove assigned-clock-rate of IMX8MP_VIDEO_PLL1

 arch/arm64/boot/dts/freescale/imx8mp.dtsi |  14 +--
 drivers/clk/clk-divider.c                 |   9 ++
 drivers/clk/clk.c                         | 146 +++++++++++++++++++++++++++++-
 drivers/clk/imx/clk-composite-8m.c        |  89 +++++++++++++++---
 drivers/clk/imx/clk-imx8mp.c              |   4 +-
 drivers/clk/imx/clk-pll14xx.c             |  20 ++++
 drivers/clk/imx/clk.h                     |   4 +
 include/linux/clk-provider.h              |   2 +
 include/linux/clk.h                       |   2 +
 9 files changed, 261 insertions(+), 29 deletions(-)
---
base-commit: e143016b56ecb0fcda5bb6026b0a25fe55274f56
change-id: 20230913-imx8mp-dtsi-7c6e25907e0e

Best regards,
-- 
Benjamin Bara <benjamin.bara@skidata.com>

[PATCH 01/13] arm64: dts: imx8mp: lvds_bridge: use root instead of composite

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

Use the actual root node of the media_ldb clock for the lvds_bridge.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 arch/arm64/boot/dts/freescale/imx8mp.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index 6f2f50e1639c..c946749a3d73 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -1760,7 +1760,7 @@ lvds_bridge: bridge@5c {
 					compatible = "fsl,imx8mp-ldb";
 					reg = <0x5c 0x4>, <0x128 0x4>;
 					reg-names = "ldb", "lvds";
-					clocks = <&clk IMX8MP_CLK_MEDIA_LDB>;
+					clocks = <&clk IMX8MP_CLK_MEDIA_LDB_ROOT>;
 					clock-names = "ldb";
 					assigned-clocks = <&clk IMX8MP_CLK_MEDIA_LDB>;
 					assigned-clock-parents = <&clk IMX8MP_VIDEO_PLL1_OUT>;

-- 
2.34.1

Re: [PATCH 01/13] arm64: dts: imx8mp: lvds_bridge: use root instead of composite

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 442 bytes --]

Hi,

On Mon, Sep 18, 2023 at 12:39:57AM +0200, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> Use the actual root node of the media_ldb clock for the lvds_bridge.
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>

DT is supposed to be hardware description, so an explanation about what
has changed or was wrong in that description to make that patch needed
would be welcome here

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 01/13] arm64: dts: imx8mp: lvds_bridge: use root instead of composite

[Benjamin Bara]

Hi!

On Tue, 19 Sept 2023 at 08:47, Maxime Ripard <mripard@kernel.org> wrote:
> On Mon, Sep 18, 2023 at 12:39:57AM +0200, Benjamin Bara wrote:
> > From: Benjamin Bara <benjamin.bara@skidata.com>
> >
> > Use the actual root node of the media_ldb clock for the lvds_bridge.
> >
> > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
>
> DT is supposed to be hardware description, so an explanation about what
> has changed or was wrong in that description to make that patch needed
> would be welcome here

Sure, sorry for that. In the imx8mp context, the _ROOT is the "leaf", so
the actual clock that is connected to the bridge. I will adapt and
clarify for V2.

Thanks

[PATCH 02/13] arm64: dts: imx8mp: re-parent IMX8MP_CLK_MEDIA_MIPI_PHY1_REF

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

Similar to commit 07bb2e368820 ("arm64: dts: imx8mp: Fix video clock
parents") the parent of IMX8MP_CLK_MEDIA_MIPI_PHY1_REF should be set in
the media_blk_ctrl. Currently, if mipi_dsi is not in use, its parent is
set to IMX8MP_VIDEO_PLL1_OUT, and might therefore clash with the
constraints coming from a panel.

Cc: Adam Ford <aford173@gmail.com>
Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 arch/arm64/boot/dts/freescale/imx8mp.dtsi | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index c946749a3d73..9539d747e28e 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -1640,11 +1640,6 @@ mipi_dsi: dsi@32e60000 {
 				clocks = <&clk IMX8MP_CLK_MEDIA_APB_ROOT>,
 					 <&clk IMX8MP_CLK_MEDIA_MIPI_PHY1_REF>;
 				clock-names = "bus_clk", "sclk_mipi";
-				assigned-clocks = <&clk IMX8MP_CLK_MEDIA_APB>,
-						  <&clk IMX8MP_CLK_MEDIA_MIPI_PHY1_REF>;
-				assigned-clock-parents = <&clk IMX8MP_SYS_PLL1_800M>,
-							 <&clk IMX8MP_CLK_24M>;
-				assigned-clock-rates = <200000000>, <24000000>;
 				samsung,pll-clock-frequency = <24000000>;
 				interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
 				power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_DSI_1>;
@@ -1747,13 +1742,16 @@ media_blk_ctrl: blk-ctrl@32ec0000 {
 						  <&clk IMX8MP_CLK_MEDIA_APB>,
 						  <&clk IMX8MP_CLK_MEDIA_DISP1_PIX>,
 						  <&clk IMX8MP_CLK_MEDIA_DISP2_PIX>,
-						  <&clk IMX8MP_VIDEO_PLL1>;
+						  <&clk IMX8MP_VIDEO_PLL1>,
+						  <&clk IMX8MP_CLK_MEDIA_MIPI_PHY1_REF>;
 				assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>,
 							 <&clk IMX8MP_SYS_PLL1_800M>,
 							 <&clk IMX8MP_VIDEO_PLL1_OUT>,
-							 <&clk IMX8MP_VIDEO_PLL1_OUT>;
+							 <&clk IMX8MP_VIDEO_PLL1_OUT>,
+							 <&clk IMX8MP_CLK_24M>;
 				assigned-clock-rates = <500000000>, <200000000>,
-						       <0>, <0>, <1039500000>;
+						       <0>, <0>, <1039500000>,
+						       <24000000>;
 				#power-domain-cells = <1>;
 
 				lvds_bridge: bridge@5c {

-- 
2.34.1

Re: [PATCH 02/13] arm64: dts: imx8mp: re-parent IMX8MP_CLK_MEDIA_MIPI_PHY1_REF

[Adam Ford]

On Sun, Sep 17, 2023 at 5:40 PM Benjamin Bara <bbara93@gmail.com> wrote:
>
> From: Benjamin Bara <benjamin.bara@skidata.com>
>
> Similar to commit 07bb2e368820 ("arm64: dts: imx8mp: Fix video clock
> parents") the parent of IMX8MP_CLK_MEDIA_MIPI_PHY1_REF should be set in
> the media_blk_ctrl. Currently, if mipi_dsi is not in use, its parent is
> set to IMX8MP_VIDEO_PLL1_OUT, and might therefore clash with the
> constraints coming from a panel.

From what I can see, it looks like the IMX8MP_CLK_MEDIA_MIPI_PHY1_REF
parent is being set to IMX8MP_CLK_24M.  Isn't that the default? I also
don't think we need to set a 24MHz clock to 24MHz if that's the
default.

If that is the case, I would suggest we try to remove the assignment
altogether to make the device tree simpler and less to untangle if a
board needs to manually manipulate the clocks for some specific
reason.

adam

>
> Cc: Adam Ford <aford173@gmail.com>
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 14 ++++++--------
>  1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> index c946749a3d73..9539d747e28e 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> @@ -1640,11 +1640,6 @@ mipi_dsi: dsi@32e60000 {
>                                 clocks = <&clk IMX8MP_CLK_MEDIA_APB_ROOT>,
>                                          <&clk IMX8MP_CLK_MEDIA_MIPI_PHY1_REF>;
>                                 clock-names = "bus_clk", "sclk_mipi";
> -                               assigned-clocks = <&clk IMX8MP_CLK_MEDIA_APB>,
> -                                                 <&clk IMX8MP_CLK_MEDIA_MIPI_PHY1_REF>;
> -                               assigned-clock-parents = <&clk IMX8MP_SYS_PLL1_800M>,
> -                                                        <&clk IMX8MP_CLK_24M>;
> -                               assigned-clock-rates = <200000000>, <24000000>;
>                                 samsung,pll-clock-frequency = <24000000>;
>                                 interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
>                                 power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_DSI_1>;
> @@ -1747,13 +1742,16 @@ media_blk_ctrl: blk-ctrl@32ec0000 {
>                                                   <&clk IMX8MP_CLK_MEDIA_APB>,
>                                                   <&clk IMX8MP_CLK_MEDIA_DISP1_PIX>,
>                                                   <&clk IMX8MP_CLK_MEDIA_DISP2_PIX>,
> -                                                 <&clk IMX8MP_VIDEO_PLL1>;
> +                                                 <&clk IMX8MP_VIDEO_PLL1>,
> +                                                 <&clk IMX8MP_CLK_MEDIA_MIPI_PHY1_REF>;
>                                 assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>,
>                                                          <&clk IMX8MP_SYS_PLL1_800M>,
>                                                          <&clk IMX8MP_VIDEO_PLL1_OUT>,
> -                                                        <&clk IMX8MP_VIDEO_PLL1_OUT>;
> +                                                        <&clk IMX8MP_VIDEO_PLL1_OUT>,
> +                                                        <&clk IMX8MP_CLK_24M>;
>                                 assigned-clock-rates = <500000000>, <200000000>,
> -                                                      <0>, <0>, <1039500000>;
> +                                                      <0>, <0>, <1039500000>,
> +                                                      <24000000>;
>                                 #power-domain-cells = <1>;
>
>                                 lvds_bridge: bridge@5c {
>
> --
> 2.34.1
>

Re: [PATCH 02/13] arm64: dts: imx8mp: re-parent IMX8MP_CLK_MEDIA_MIPI_PHY1_REF

[Benjamin Bara]

Hi Adam,

thanks for the feedback!

On Tue, 3 Oct 2023 at 15:02, Adam Ford <aford173@gmail.com> wrote:
> From what I can see, it looks like the IMX8MP_CLK_MEDIA_MIPI_PHY1_REF
> parent is being set to IMX8MP_CLK_24M.  Isn't that the default? I also
> don't think we need to set a 24MHz clock to 24MHz if that's the
> default.

I can retry (have the patch applied since then), but as far as I
remember, it was not. What was even funnier was that media_mipi_phy1_ref
hat a divider != 1 set (it is a composite), so it wasn't sufficient to
just re-parent it to OSC_24M - probably because set_rate() was called
before it was re-parented to OSC_24M. But thanks for the catch, I will
take a look again and adapt it if possible.

Regards
Benjamin

> If that is the case, I would suggest we try to remove the assignment
> altogether to make the device tree simpler and less to untangle if a
> board needs to manually manipulate the clocks for some specific
> reason.
>
> adam
>
> >
> > Cc: Adam Ford <aford173@gmail.com>
> > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > ---
> >  arch/arm64/boot/dts/freescale/imx8mp.dtsi | 14 ++++++--------
> >  1 file changed, 6 insertions(+), 8 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > index c946749a3d73..9539d747e28e 100644
> > --- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > +++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
> > @@ -1640,11 +1640,6 @@ mipi_dsi: dsi@32e60000 {
> >                                 clocks = <&clk IMX8MP_CLK_MEDIA_APB_ROOT>,
> >                                          <&clk IMX8MP_CLK_MEDIA_MIPI_PHY1_REF>;
> >                                 clock-names = "bus_clk", "sclk_mipi";
> > -                               assigned-clocks = <&clk IMX8MP_CLK_MEDIA_APB>,
> > -                                                 <&clk IMX8MP_CLK_MEDIA_MIPI_PHY1_REF>;
> > -                               assigned-clock-parents = <&clk IMX8MP_SYS_PLL1_800M>,
> > -                                                        <&clk IMX8MP_CLK_24M>;
> > -                               assigned-clock-rates = <200000000>, <24000000>;
> >                                 samsung,pll-clock-frequency = <24000000>;
> >                                 interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
> >                                 power-domains = <&media_blk_ctrl IMX8MP_MEDIABLK_PD_MIPI_DSI_1>;
> > @@ -1747,13 +1742,16 @@ media_blk_ctrl: blk-ctrl@32ec0000 {
> >                                                   <&clk IMX8MP_CLK_MEDIA_APB>,
> >                                                   <&clk IMX8MP_CLK_MEDIA_DISP1_PIX>,
> >                                                   <&clk IMX8MP_CLK_MEDIA_DISP2_PIX>,
> > -                                                 <&clk IMX8MP_VIDEO_PLL1>;
> > +                                                 <&clk IMX8MP_VIDEO_PLL1>,
> > +                                                 <&clk IMX8MP_CLK_MEDIA_MIPI_PHY1_REF>;
> >                                 assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>,
> >                                                          <&clk IMX8MP_SYS_PLL1_800M>,
> >                                                          <&clk IMX8MP_VIDEO_PLL1_OUT>,
> > -                                                        <&clk IMX8MP_VIDEO_PLL1_OUT>;
> > +                                                        <&clk IMX8MP_VIDEO_PLL1_OUT>,
> > +                                                        <&clk IMX8MP_CLK_24M>;
> >                                 assigned-clock-rates = <500000000>, <200000000>,
> > -                                                      <0>, <0>, <1039500000>;
> > +                                                      <0>, <0>, <1039500000>,
> > +                                                      <24000000>;
> >                                 #power-domain-cells = <1>;
> >
> >                                 lvds_bridge: bridge@5c {
> >
> > --
> > 2.34.1
> >

[PATCH 03/13] clk: implement clk_hw_set_rate()

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

This function can be used to set the rate of a clock hardware from a
driver, e.g. to adapt the rate to a clock change coming from the parent.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/clk/clk.c            | 15 +++++++++++++++
 include/linux/clk-provider.h |  1 +
 2 files changed, 16 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index c249f9791ae8..3e222802b712 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2462,6 +2462,21 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 	return ret;
 }
 
+int clk_hw_set_rate(struct clk_hw *hw, unsigned long req_rate)
+{
+	/* A rate change is ongoing, so just target the required rate.
+	 * Note: this does not work if one clock along the line has
+	 * CLK_RECALC_NEW_RATES active, as this overwrites the new_rate again.
+	 */
+	if (hw->core->new_rate != hw->core->rate) {
+		hw->core->new_rate = req_rate;
+		return 0;
+	}
+
+	return clk_core_set_rate_nolock(hw->core, req_rate);
+}
+EXPORT_SYMBOL_GPL(clk_hw_set_rate);
+
 /**
  * clk_set_rate - specify a new rate for clk
  * @clk: the clk whose rate is being changed
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index ec32ec58c59f..3fb99ed5e8d9 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -1316,6 +1316,7 @@ int clk_hw_get_parent_index(struct clk_hw *hw);
 int clk_hw_set_parent(struct clk_hw *hw, struct clk_hw *new_parent);
 unsigned int __clk_get_enable_count(struct clk *clk);
 unsigned long clk_hw_get_rate(const struct clk_hw *hw);
+int clk_hw_set_rate(struct clk_hw *hw, unsigned long req_rate);
 unsigned long clk_hw_get_flags(const struct clk_hw *hw);
 #define clk_hw_can_set_rate_parent(hw) \
 	(clk_hw_get_flags((hw)) & CLK_SET_RATE_PARENT)

-- 
2.34.1

Re: [PATCH 03/13] clk: implement clk_hw_set_rate()

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1540 bytes --]

Hi,

On Mon, Sep 18, 2023 at 12:39:59AM +0200, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> This function can be used to set the rate of a clock hardware from a
> driver, e.g. to adapt the rate to a clock change coming from the parent.
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
>  drivers/clk/clk.c            | 15 +++++++++++++++
>  include/linux/clk-provider.h |  1 +
>  2 files changed, 16 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index c249f9791ae8..3e222802b712 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2462,6 +2462,21 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
>  	return ret;
>  }
>  
> +int clk_hw_set_rate(struct clk_hw *hw, unsigned long req_rate)
> +{
> +	/* A rate change is ongoing, so just target the required rate.
> +	 * Note: this does not work if one clock along the line has
> +	 * CLK_RECALC_NEW_RATES active, as this overwrites the new_rate again.
> +	 */
> +	if (hw->core->new_rate != hw->core->rate) {
> +		hw->core->new_rate = req_rate;
> +		return 0;
> +	}
> +
> +	return clk_core_set_rate_nolock(hw->core, req_rate);
> +}
> +EXPORT_SYMBOL_GPL(clk_hw_set_rate);

So, we discussed it recently, and it's non-obvious to see how it would
work if you're in a set_rate change that modifies the tree and the new
call to clk_hw_set_rate modifies the tree too. Some explanation on how
it's handled, and some unit tests are required here imo.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 04/13] clk: print debug message if parent change is ignored

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

Print a debug message if the determination of the best clock rate
suggests a re-config of the parent (which means the actual driver
considers doing so), but the clock is not configured with
CLK_SET_PARENT_RATE.

This should give a good hint for clock config improvement potential.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/clk/clk.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 3e222802b712..4954d31899ce 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2241,9 +2241,14 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 		}
 	}
 
-	if ((core->flags & CLK_SET_RATE_PARENT) && parent &&
-	    best_parent_rate != parent->rate)
-		top = clk_calc_new_rates(parent, best_parent_rate);
+	if (parent && best_parent_rate != parent->rate) {
+		if (core->flags & CLK_SET_RATE_PARENT)
+			top = clk_calc_new_rates(parent, best_parent_rate);
+		else
+			pr_debug("%s: ignore parent %s re-config from %lu to %lu\n",
+				 core->name, parent->name, parent->rate,
+				 best_parent_rate);
+	}
 
 out:
 	clk_calc_subtree(core, new_rate, parent, p_index);

-- 
2.34.1

[PATCH 05/13] clk: keep track of the trigger of an ongoing clk_set_rate

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

When we keep track of the rate change trigger, we can easily check if an
affected clock is affiliated with the trigger. Additionally, the trigger
is added to the notify data, so that drivers can implement workarounds
that might be necessary if a shared parent changes.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/clk/clk.c   | 12 ++++++++++++
 include/linux/clk.h |  2 ++
 2 files changed, 14 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 4954d31899ce..8f4f92547768 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -33,6 +33,9 @@ static struct task_struct *enable_owner;
 static int prepare_refcnt;
 static int enable_refcnt;
 
+/* responsible for ongoing rate change, protected by prepare_lock */
+static struct clk *rate_trigger_clk;
+
 static HLIST_HEAD(clk_root_list);
 static HLIST_HEAD(clk_orphan_list);
 static LIST_HEAD(clk_notifier_list);
@@ -1742,6 +1745,7 @@ static int __clk_notify(struct clk_core *core, unsigned long msg,
 
 	cnd.old_rate = old_rate;
 	cnd.new_rate = new_rate;
+	cnd.trigger = rate_trigger_clk ? : core->parent->hw->clk;
 
 	list_for_each_entry(cn, &clk_notifier_list, node) {
 		if (cn->clk->core == core) {
@@ -2513,6 +2517,8 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
 	/* prevent racing with updates to the clock topology */
 	clk_prepare_lock();
 
+	rate_trigger_clk = clk;
+
 	if (clk->exclusive_count)
 		clk_core_rate_unprotect(clk->core);
 
@@ -2521,6 +2527,8 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
 	if (clk->exclusive_count)
 		clk_core_rate_protect(clk->core);
 
+	rate_trigger_clk = NULL;
+
 	clk_prepare_unlock();
 
 	return ret;
@@ -2556,6 +2564,8 @@ int clk_set_rate_exclusive(struct clk *clk, unsigned long rate)
 	/* prevent racing with updates to the clock topology */
 	clk_prepare_lock();
 
+	rate_trigger_clk = clk;
+
 	/*
 	 * The temporary protection removal is not here, on purpose
 	 * This function is meant to be used instead of clk_rate_protect,
@@ -2568,6 +2578,8 @@ int clk_set_rate_exclusive(struct clk *clk, unsigned long rate)
 		clk->exclusive_count++;
 	}
 
+	rate_trigger_clk = NULL;
+
 	clk_prepare_unlock();
 
 	return ret;
diff --git a/include/linux/clk.h b/include/linux/clk.h
index 06f1b292f8a0..f0fe78c7a0f1 100644
--- a/include/linux/clk.h
+++ b/include/linux/clk.h
@@ -60,6 +60,7 @@ struct clk_notifier {
 /**
  * struct clk_notifier_data - rate data to pass to the notifier callback
  * @clk: struct clk * being changed
+ * @trigger: struct clk * being responsible for the change
  * @old_rate: previous rate of this clk
  * @new_rate: new rate of this clk
  *
@@ -70,6 +71,7 @@ struct clk_notifier {
  */
 struct clk_notifier_data {
 	struct clk		*clk;
+	struct clk		*trigger;
 	unsigned long		old_rate;
 	unsigned long		new_rate;
 };

-- 
2.34.1

Re: [PATCH 05/13] clk: keep track of the trigger of an ongoing clk_set_rate

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2535 bytes --]

On Mon, Sep 18, 2023 at 12:40:01AM +0200, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> When we keep track of the rate change trigger, we can easily check if an
> affected clock is affiliated with the trigger. Additionally, the trigger
> is added to the notify data, so that drivers can implement workarounds
> that might be necessary if a shared parent changes.
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
>  drivers/clk/clk.c   | 12 ++++++++++++
>  include/linux/clk.h |  2 ++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 4954d31899ce..8f4f92547768 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -33,6 +33,9 @@ static struct task_struct *enable_owner;
>  static int prepare_refcnt;
>  static int enable_refcnt;
>  
> +/* responsible for ongoing rate change, protected by prepare_lock */
> +static struct clk *rate_trigger_clk;
> +
>  static HLIST_HEAD(clk_root_list);
>  static HLIST_HEAD(clk_orphan_list);
>  static LIST_HEAD(clk_notifier_list);
> @@ -1742,6 +1745,7 @@ static int __clk_notify(struct clk_core *core, unsigned long msg,
>  
>  	cnd.old_rate = old_rate;
>  	cnd.new_rate = new_rate;
> +	cnd.trigger = rate_trigger_clk ? : core->parent->hw->clk;
>  
>  	list_for_each_entry(cn, &clk_notifier_list, node) {
>  		if (cn->clk->core == core) {
> @@ -2513,6 +2517,8 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
>  	/* prevent racing with updates to the clock topology */
>  	clk_prepare_lock();
>  
> +	rate_trigger_clk = clk;
> +

So I don't think that interacts very well with the clk_hw_set_rate
function you introduced. It looks like you only consider the initial
clock here so you wouldn't update rate_trigger_clk on a clk_hw_set_rate
call, but that creates some inconsistencies:

  - If we call clk_hw_set_rate outside of the set_rate path (but in
    .init for example), then we end up with a notifier without a trigger
    clock set.

  - More generally, depending on the path we're currently in, a call to
    clk_hw_set_rate will notify a clock in different ways which is a bit
    weird to me. The trigger clock can also be any clock, parent or
    child, at any level, which definitely complicates things at the
    driver level.

The rate propagation is top-down, so could be get away with just setting
the parent clock that triggered the notification?

Either way, we need unit tests for that too.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 05/13] clk: keep track of the trigger of an ongoing clk_set_rate

[Benjamin Bara]

Hi!

On Tue, 19 Sept 2023 at 09:06, Maxime Ripard <mripard@kernel.org> wrote:
> On Mon, Sep 18, 2023 at 12:40:01AM +0200, Benjamin Bara wrote:
> > From: Benjamin Bara <benjamin.bara@skidata.com>
> >
> > When we keep track of the rate change trigger, we can easily check if an
> > affected clock is affiliated with the trigger. Additionally, the trigger
> > is added to the notify data, so that drivers can implement workarounds
> > that might be necessary if a shared parent changes.
> >
> > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > ---
> >  drivers/clk/clk.c   | 12 ++++++++++++
> >  include/linux/clk.h |  2 ++
> >  2 files changed, 14 insertions(+)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 4954d31899ce..8f4f92547768 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -33,6 +33,9 @@ static struct task_struct *enable_owner;
> >  static int prepare_refcnt;
> >  static int enable_refcnt;
> >
> > +/* responsible for ongoing rate change, protected by prepare_lock */
> > +static struct clk *rate_trigger_clk;
> > +
> >  static HLIST_HEAD(clk_root_list);
> >  static HLIST_HEAD(clk_orphan_list);
> >  static LIST_HEAD(clk_notifier_list);
> > @@ -1742,6 +1745,7 @@ static int __clk_notify(struct clk_core *core, unsigned long msg,
> >
> >       cnd.old_rate = old_rate;
> >       cnd.new_rate = new_rate;
> > +     cnd.trigger = rate_trigger_clk ? : core->parent->hw->clk;
> >
> >       list_for_each_entry(cn, &clk_notifier_list, node) {
> >               if (cn->clk->core == core) {
> > @@ -2513,6 +2517,8 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
> >       /* prevent racing with updates to the clock topology */
> >       clk_prepare_lock();
> >
> > +     rate_trigger_clk = clk;
> > +
>
> So I don't think that interacts very well with the clk_hw_set_rate
> function you introduced. It looks like you only consider the initial
> clock here so you wouldn't update rate_trigger_clk on a clk_hw_set_rate
> call, but that creates some inconsistencies:
>
>   - If we call clk_hw_set_rate outside of the set_rate path (but in
>     .init for example), then we end up with a notifier without a trigger
>     clock set.
>
>   - More generally, depending on the path we're currently in, a call to
>     clk_hw_set_rate will notify a clock in different ways which is a bit
>     weird to me. The trigger clock can also be any clock, parent or
>     child, at any level, which definitely complicates things at the
>     driver level.
>
> The rate propagation is top-down, so could be get away with just setting
> the parent clock that triggered the notification?

As I mentioned in the other response, this implementation seems to be
just a hack to get additional context in the notifier. I think that's
also a problem Frank had in his approach. Inside the notifier, it's not
clear what to do with the incoming change. Because it could be either
"intended", meaning a sub-clock of the current clock has triggered the
change, or "unintended" (e.g. a sibling has triggered the change, but
the subtree beyond the current clock still requires the old rate, and
therefore the clock needs to adapt). Therefore I think if we use
req_rate here, we might be able to achieve the same thing in a better
way.

Thanks!

[PATCH 06/13] clk: keep track if a clock is explicitly configured

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

When we keep track if a clock has a given rate explicitly set by a
consumer, we can identify unintentional clock rate changes in an easy
way. This also helps during debugging, as one can see if a rate is set
by accident or due to a consumer-related change.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/clk/clk.c            | 25 +++++++++++++++++++++++++
 include/linux/clk-provider.h |  1 +
 2 files changed, 26 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 8f4f92547768..82c65ed432c5 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -70,6 +70,7 @@ struct clk_core {
 	unsigned long		rate;
 	unsigned long		req_rate;
 	unsigned long		new_rate;
+	unsigned long		set_rate;
 	struct clk_core		*new_parent;
 	struct clk_core		*new_child;
 	unsigned long		flags;
@@ -541,6 +542,12 @@ bool __clk_is_enabled(struct clk *clk)
 }
 EXPORT_SYMBOL_GPL(__clk_is_enabled);
 
+bool __clk_is_rate_set(struct clk *clk)
+{
+	return clk->core->set_rate > 0;
+}
+EXPORT_SYMBOL_GPL(__clk_is_rate_set);
+
 static bool mux_is_better_rate(unsigned long rate, unsigned long now,
 			   unsigned long best, unsigned long flags)
 {
@@ -578,6 +585,19 @@ static bool clk_core_has_parent(struct clk_core *core, const struct clk_core *pa
 	return false;
 }
 
+static bool clk_core_is_ancestor(struct clk_core *core, const struct clk_core *ancestor)
+{
+	struct clk_core *tmp = core->parent;
+
+	while (tmp) {
+		if (tmp == ancestor)
+			return true;
+		tmp = tmp->parent;
+	}
+
+	return false;
+}
+
 static void
 clk_core_forward_rate_req(struct clk_core *core,
 			  const struct clk_rate_request *old_req,
@@ -2358,6 +2378,9 @@ static void clk_change_rate(struct clk_core *core)
 
 	trace_clk_set_rate_complete(core, core->new_rate);
 
+	if (rate_trigger_clk && clk_core_is_ancestor(rate_trigger_clk->core, core))
+		core->set_rate = core->new_rate;
+
 	core->rate = clk_recalc(core, best_parent_rate);
 
 	if (core->flags & CLK_SET_RATE_UNGATE) {
@@ -2528,6 +2551,7 @@ int clk_set_rate(struct clk *clk, unsigned long rate)
 		clk_core_rate_protect(clk->core);
 
 	rate_trigger_clk = NULL;
+	clk->core->set_rate = rate;
 
 	clk_prepare_unlock();
 
@@ -2579,6 +2603,7 @@ int clk_set_rate_exclusive(struct clk *clk, unsigned long rate)
 	}
 
 	rate_trigger_clk = NULL;
+	clk->core->set_rate = rate;
 
 	clk_prepare_unlock();
 
diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
index 3fb99ed5e8d9..e3732e0bbed9 100644
--- a/include/linux/clk-provider.h
+++ b/include/linux/clk-provider.h
@@ -1325,6 +1325,7 @@ bool clk_hw_is_prepared(const struct clk_hw *hw);
 bool clk_hw_rate_is_protected(const struct clk_hw *hw);
 bool clk_hw_is_enabled(const struct clk_hw *hw);
 bool __clk_is_enabled(struct clk *clk);
+bool __clk_is_rate_set(struct clk *hw);
 struct clk *__clk_lookup(const char *name);
 int __clk_mux_determine_rate(struct clk_hw *hw,
 			     struct clk_rate_request *req);

-- 
2.34.1

Re: [PATCH 06/13] clk: keep track if a clock is explicitly configured

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1024 bytes --]

Hi,

On Mon, Sep 18, 2023 at 12:40:02AM +0200, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> When we keep track if a clock has a given rate explicitly set by a
> consumer, we can identify unintentional clock rate changes in an easy
> way. This also helps during debugging, as one can see if a rate is set
> by accident or due to a consumer-related change.
> 
> Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> ---
>  drivers/clk/clk.c            | 25 +++++++++++++++++++++++++
>  include/linux/clk-provider.h |  1 +
>  2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 8f4f92547768..82c65ed432c5 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -70,6 +70,7 @@ struct clk_core {
>  	unsigned long		rate;
>  	unsigned long		req_rate;
>  	unsigned long		new_rate;
> +	unsigned long		set_rate;

This is pretty much what req_rate is supposed to be about. Why didn't it
work in your case?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 06/13] clk: keep track if a clock is explicitly configured

[Benjamin Bara]

Hi Maxime!

thanks for taking the time to look through :)

On Tue, 19 Sept 2023 at 09:07, Maxime Ripard <mripard@kernel.org> wrote:
> On Mon, Sep 18, 2023 at 12:40:02AM +0200, Benjamin Bara wrote:
> > From: Benjamin Bara <benjamin.bara@skidata.com>
> >
> > When we keep track if a clock has a given rate explicitly set by a
> > consumer, we can identify unintentional clock rate changes in an easy
> > way. This also helps during debugging, as one can see if a rate is set
> > by accident or due to a consumer-related change.
> >
> > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > ---
> >  drivers/clk/clk.c            | 25 +++++++++++++++++++++++++
> >  include/linux/clk-provider.h |  1 +
> >  2 files changed, 26 insertions(+)
> >
> > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > index 8f4f92547768..82c65ed432c5 100644
> > --- a/drivers/clk/clk.c
> > +++ b/drivers/clk/clk.c
> > @@ -70,6 +70,7 @@ struct clk_core {
> >       unsigned long           rate;
> >       unsigned long           req_rate;
> >       unsigned long           new_rate;
> > +     unsigned long           set_rate;
>
> This is pretty much what req_rate is supposed to be about. Why didn't it
> work in your case?

I picked this one to respond first because I think some of the
implemented stuff just workarounds the current req_rate behaviour.

Currently, I have two "problems" with it:
1. It's set during initialization[1]. In this phase, the *required* rate
   isn't known yet, so it should be 0 imo.
2. It's set during re-parenting[2,3]. Also here, just because we
   re-parent, the active consumer (which set the req_rate to a valid
   value) still requires the clock to have the same rate.

That is basically the reason why we have no info if the req_rate is
really "required" by a consumer or if it is just set because the parent
had it at some time. It's only usage is here[4], which IMO doesn't
really depends on the wrong behaviour I described above.

The respective sub-tree we talk about on the imx8mp looks like this (one
example for the the LVDS-only case):
video_pll1 (pll; 7x crtc rate - currently, rate is assigned via dt)
  video_pll1_bypass (mux; 7x crtc rate)
    video_pll1_out (gate; 7x crtc rate)
      media_ldb (divider; 7x crtc rate)
        media_ldb_root_clk (gate; 7x crtc rate)
      media_disp2_pix (divider; 1x crtc rate)
        media_disp2_pix_root_clk (gate; 1x crtc rate)
      media_disp1_pix (divider; unused for now)
        media_disp1_pix_root_clk (gate; unused for now)

The problem is that the panel driver sets media_disp1_pix_root_clk,
ldb-bridge driver sets media_ldb_root_clk. All the others have a
req_rate of the rate video_pll1 had when they got initialized or
re-parented.

My idea was, that when media_disp2_pix_root_clk is set to the CRTC rate,
IMO all clocks along the line (especially media_disp1_pix, which is
"seen" as child of the PLL, and the actual divider for
media_disp2_pix_root_clk) need to set their new rate as "required",
because the subtree below them relies on it. This might be a wrong
approach. It might be sufficient to have a req_rate only on the nodes
that actually require it. However, IMHO we need to make sure that *all*
required rates (especially the ones of leaves!) are respected after a
change. Meaning if we e.g. request video_pll1 to change again (this time
by media_ldb_root_clk), we have to ensure that media_disp2_pix_root_clk
has still the rate which has been set as req_rate before.

Ultimately, my trigger patch is also just a really bad workaround for a
new_rate != req_rate check, so I want to re-build the idea behind it
based on a differently defined req_rate. Need to take a deeper look on
that.

Thanks & regards
Benjamin

[1] https://elixir.bootlin.com/linux/v6.5.3/source/drivers/clk/clk.c#L3891
[2] https://elixir.bootlin.com/linux/v6.5.3/source/drivers/clk/clk.c#L2726
[3] https://elixir.bootlin.com/linux/v6.5.3/source/drivers/clk/clk.c#L2812
[4] https://elixir.bootlin.com/linux/v6.5.3/source/drivers/clk/clk.c#L2592

Re: [PATCH 06/13] clk: keep track if a clock is explicitly configured

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5425 bytes --]

Hi Benjamin,

On Wed, Sep 20, 2023 at 09:22:16AM +0200, Benjamin Bara wrote:
> On Tue, 19 Sept 2023 at 09:07, Maxime Ripard <mripard@kernel.org> wrote:
> > On Mon, Sep 18, 2023 at 12:40:02AM +0200, Benjamin Bara wrote:
> > > From: Benjamin Bara <benjamin.bara@skidata.com>
> > >
> > > When we keep track if a clock has a given rate explicitly set by a
> > > consumer, we can identify unintentional clock rate changes in an easy
> > > way. This also helps during debugging, as one can see if a rate is set
> > > by accident or due to a consumer-related change.
> > >
> > > Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
> > > ---
> > >  drivers/clk/clk.c            | 25 +++++++++++++++++++++++++
> > >  include/linux/clk-provider.h |  1 +
> > >  2 files changed, 26 insertions(+)
> > >
> > > diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> > > index 8f4f92547768..82c65ed432c5 100644
> > > --- a/drivers/clk/clk.c
> > > +++ b/drivers/clk/clk.c
> > > @@ -70,6 +70,7 @@ struct clk_core {
> > >       unsigned long           rate;
> > >       unsigned long           req_rate;
> > >       unsigned long           new_rate;
> > > +     unsigned long           set_rate;
> >
> > This is pretty much what req_rate is supposed to be about. Why didn't it
> > work in your case?
> 
> I picked this one to respond first because I think some of the
> implemented stuff just workarounds the current req_rate behaviour.
> 
> Currently, I have two "problems" with it:
> 1. It's set during initialization[1]. In this phase, the *required* rate
>    isn't known yet, so it should be 0 imo.

Agreed. Ideally, it should be another value (like -1) since 0 is also
used for rates in some drivers, but that's a separate story :)

> 2. It's set during re-parenting[2,3]. Also here, just because we
>    re-parent, the active consumer (which set the req_rate to a valid
>    value) still requires the clock to have the same rate.
>
> That is basically the reason why we have no info if the req_rate is
> really "required" by a consumer or if it is just set because the parent
> had it at some time. It's only usage is here[4], which IMO doesn't
> really depends on the wrong behaviour I described above.

Ah, right.

> The respective sub-tree we talk about on the imx8mp looks like this (one
> example for the the LVDS-only case):
> video_pll1 (pll; 7x crtc rate - currently, rate is assigned via dt)
>   video_pll1_bypass (mux; 7x crtc rate)
>     video_pll1_out (gate; 7x crtc rate)
>       media_ldb (divider; 7x crtc rate)
>         media_ldb_root_clk (gate; 7x crtc rate)
>       media_disp2_pix (divider; 1x crtc rate)
>         media_disp2_pix_root_clk (gate; 1x crtc rate)
>       media_disp1_pix (divider; unused for now)
>         media_disp1_pix_root_clk (gate; unused for now)
> 
> The problem is that the panel driver sets media_disp1_pix_root_clk,
> ldb-bridge driver sets media_ldb_root_clk. All the others have a
> req_rate of the rate video_pll1 had when they got initialized or
> re-parented.

So we have only dividers, but what is the range of those? ie, could we
get away with running the video-pll1 at 297/594MHz (or a multiple of it)
and cover most of the pixel rates for LVDS?

> My idea was, that when media_disp2_pix_root_clk is set to the CRTC rate,
> IMO all clocks along the line (especially media_disp1_pix, which is
> "seen" as child of the PLL, and the actual divider for
> media_disp2_pix_root_clk) need to set their new rate as "required",
> because the subtree below them relies on it. This might be a wrong
> approach. It might be sufficient to have a req_rate only on the nodes
> that actually require it.

That makes total sense. However, the clock framework hasn't been
designed around modifying the rate of multiple clocks in one go, which
is pretty much what you want to achieve at the moment.

You're already reaching those limits in your patches since, for example,
you kind of hardcode the tolerance the clocks consider to be ok within
the framework, which something that really belongs to each clock driver.

This is why I'm insisting in figuring out whether we can run the main
PLL at a frequency that is good enough for each use-case. That way it
doesn't have to change, you don't have to propagate anything, the
problem becomes much simpler :)

> However, IMHO we need to make sure that *all* required rates
> (especially the ones of leaves!) are respected after a change.

Part of the issue I was telling you about is that clk_set_rate never
really expressed any time duration, it's very much a fire and forget
call, so for all the CCF cares the rate could change on the very next
instruction and it would be ok.

Doing so would also introduce some subtle corner-cases, like what is
happening if cpufreq set your CPU frequency to (for example) 1GHz, but
the firmware lowered it to 600MHz for thermal throttling. What happens
then? Which rate do you consider the required rate?

This would effectively mean merging clk_set_rate with
clk_set_rate_exclusive, but the latter never really caught up because
most clocks don't care, and it's fairly inconvenient to use.

If there is an effort to be made (and I still don't believe we need to),
then I think we should put in into improving clk_set_rate_exclusive()
rather than changing the semantics of clk_set_rate().

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 07/13] clk: detect unintended rate changes

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

As we now keep track of the clocks which are allowed to change - namely
the ones which are along the ancestor line between the rate trigger and
the top-most changed clock, we can run through the subtree of changes
and look for unexpected ones. Shared parents must set their rate in a
way, that all consumer-configured rates are respected. As this is
sometimes not possible and clocks sometime doesn't require the *exact*
rate, we might have to find a way to find out if it is *exact enough*.
Then we could fix it in the core.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/clk/clk.c | 83 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 83 insertions(+)

diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 82c65ed432c5..faececc44c28 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2280,6 +2280,74 @@ static struct clk_core *clk_calc_new_rates(struct clk_core *core,
 	return top;
 }
 
+/*
+ * If the changed clock is consumer-configured, but not an ancestor of the
+ * trigger, it is most likely an unintended change. As a workaround, we try to
+ * set the rate back to the old without changing the parent. If this is not
+ * possible, the change should not have been suggested in the first place.
+ */
+static struct clk_core *clk_detect_unintended_rate_changes(struct clk_core *core,
+							   bool fix)
+{
+	struct clk_core *child, *tmp_clk;
+
+	if (core->rate == core->new_rate)
+		return NULL;
+
+	if (core->set_rate && core != rate_trigger_clk->core &&
+	    !clk_core_is_ancestor(rate_trigger_clk->core, core)) {
+		struct clk_core *parent = core->new_parent ? : core->parent;
+		struct clk_rate_request req;
+
+		pr_debug("%s: unintended change by %s (%lu -> %lu)\n", core->name,
+			 rate_trigger_clk->core->name, core->rate, core->new_rate);
+
+		if (fix) {
+			clk_hw_init_rate_request(core->hw, &req, core->rate);
+			req.best_parent_rate = parent->new_rate;
+			req.best_parent_hw = parent->hw;
+
+			if (clk_core_round_rate_nolock(core, &req))
+				return core;
+
+			/* TODO: how close is close enough? */
+			if (req.rate != core->rate) {
+				pr_debug("%s: %s fix failed, req=%lu, sugg=%lu\n",
+					 __func__, core->name, core->rate, req.rate);
+				return core;
+			}
+			if (req.best_parent_rate != parent->new_rate ||
+			    req.best_parent_hw != parent->hw) {
+				pr_debug("%s: %s fix failed, req=%s@%lu, sugg=%s@%lu\n",
+					 __func__, core->name, parent->name,
+					 parent->new_rate,
+					 req.best_parent_hw->core->name,
+					 req.best_parent_rate);
+				return core;
+			}
+
+			core->new_rate = core->rate;
+		}
+		return NULL;
+	}
+
+	hlist_for_each_entry(child, &core->children, child_node) {
+		if (child->new_parent && child->new_parent != core)
+			continue;
+		tmp_clk = clk_detect_unintended_rate_changes(child, fix);
+		if (tmp_clk)
+			return tmp_clk;
+	}
+
+	if (core->new_child) {
+		tmp_clk = clk_detect_unintended_rate_changes(core->new_child, fix);
+		if (tmp_clk)
+			return tmp_clk;
+	}
+
+	return NULL;
+}
+
 /*
  * Notify about rate changes in a subtree. Always walk down the whole tree
  * so that in case of an error we can walk down the whole tree again and
@@ -2484,6 +2552,21 @@ static int clk_core_set_rate_nolock(struct clk_core *core,
 		goto err;
 	}
 
+	/*
+	 * The notifying process offers the possibility to fix the rates of
+	 * unrelated clocks along the tree. After that, run a detection to find
+	 * clocks which are potentially wrongly configured now. These might be
+	 * fixed by the core in the future.
+	 */
+	fail_clk = clk_detect_unintended_rate_changes(top, false);
+	if (fail_clk) {
+		pr_err("%s: unintended rate change cannot be fixed\n",
+		       fail_clk->name);
+		ret = -EINVAL;
+		goto err;
+	}
+
+
 	/* change the rates */
 	clk_change_rate(top);
 

-- 
2.34.1

Re: [PATCH 07/13] clk: detect unintended rate changes

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2082 bytes --]

Hi,

On Mon, Sep 18, 2023 at 12:40:03AM +0200, Benjamin Bara wrote:
> From: Benjamin Bara <benjamin.bara@skidata.com>
> 
> As we now keep track of the clocks which are allowed to change - namely
> the ones which are along the ancestor line between the rate trigger and
> the top-most changed clock

I'm not sure that the fact that only a clock between the clock
triggering the rate change and the top-most changed clock was allowed to
change has ever been a thing.

This puts a fairly big pressure on the tree propagation code, and
whether or not that can be done is completely context-dependent.

Devices like UART, I2C or audio devices are rate change sensitive, and
yet usually have internal dividers to accomodate for their parent rate
so don't usually care as long as they are notified.

Similarly, all the non-rate-sensitive devices (like pretty much all
bus/registers clocks) don't care at all about what the rate is, so that
requirement is making a rate change going through less likely, without a
particular reason only for a handful of devices (display in this case).

Also, this rules out any child of a clock from being allowed to change?
That looks wild to me :)

> we can run through the subtree of changes and look for unexpected
> ones.

Again, I'm not sure we can say that those changes were unexpected. They
very much were expected to change to the CCF so far.

> Shared parents must set their rate in a way, that all
> consumer-configured rates are respected.

Again, that entirely depends on the context: the clock tree topology,
the devices involved, what their drivers support, etc. I'm sure it's
true in your case, but I'm not sure we can make that a generic
statement.

> As this is sometimes not possible and clocks sometime doesn't require
> the *exact* rate, we might have to find a way to find out if it is
> *exact enough*. Then we could fix it in the core.

And "exact enough" entirely depends on the context again, so really, I'm
not sure we can (and should!) fix that at the framework level.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH 08/13] clk: divider: stop early if an optimal divider is found

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

During finding the best divider, the current implementation asks the
parent for the best rate for all its available dividers. If there are a
lot of supported divider values and the maximum divider is far from the
target rate, this can lead to many iterations. Depending on the parent,
the process of calculating the best fitting rate can be quite complex.

Therefore, return early if an optimal divider has been found.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/clk/clk-divider.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/clk/clk-divider.c b/drivers/clk/clk-divider.c
index a2c2b5203b0a..61b40dfb4e6f 100644
--- a/drivers/clk/clk-divider.c
+++ b/drivers/clk/clk-divider.c
@@ -332,6 +332,15 @@ static int clk_divider_bestdiv(struct clk_hw *hw, struct clk_hw *parent,
 			bestdiv = i;
 			best = now;
 			*best_parent_rate = parent_rate;
+			if (now == rate)
+				/*
+				 * Calculating fitting PLL parameters, which
+				 * might be done in parent's round_rate, can be
+				 * time-consuming. Therefore, the lowest parent
+				 * rate which gives us the exact required rate
+				 * is already optimal.
+				 */
+				return i;
 		}
 	}
 

-- 
2.34.1

[PATCH 09/13] clk: imx: pll14xx: consider active rate for re-config

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

As the pll14xx might be the parent of multiple child clocks, the active
config is most likely still required by one of them. As the children
have divider, use the LCM of the old and the new rate to target for an
integer multiple of the active rate.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/clk/imx/clk-pll14xx.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/clk/imx/clk-pll14xx.c b/drivers/clk/imx/clk-pll14xx.c
index 0d58d85c375e..803e8b0a7a31 100644
--- a/drivers/clk/imx/clk-pll14xx.c
+++ b/drivers/clk/imx/clk-pll14xx.c
@@ -12,6 +12,7 @@
 #include <linux/export.h>
 #include <linux/io.h>
 #include <linux/iopoll.h>
+#include <linux/lcm.h>
 #include <linux/slab.h>
 #include <linux/jiffies.h>
 
@@ -36,6 +37,7 @@
 
 struct clk_pll14xx {
 	struct clk_hw			hw;
+	unsigned long			rate;
 	void __iomem			*base;
 	enum imx_pll14xx_type		type;
 	const struct imx_pll14xx_rate_table *rate_table;
@@ -235,6 +237,22 @@ static long clk_pll1443x_round_rate(struct clk_hw *hw, unsigned long rate,
 	struct clk_pll14xx *pll = to_clk_pll14xx(hw);
 	struct imx_pll14xx_rate_table t;
 
+	/*
+	 * If the PLL is configured more than once, we have to consider the
+	 * active config for the new rate. As the children have divider, also
+	 * allow multiples of the already configured rate. This is a simple
+	 * approach to enable dynamic re-config via SET_CLK_RATE_PARENT for more
+	 * than one consumer. E.g. on the imx8mp, when video_pll1 is parent of
+	 * media_ldb and media_disp2_pix (always 7:1).
+	 */
+	if (pll->rate) {
+		unsigned long want = rate;
+
+		rate = lcm(pll->rate, rate);
+		pr_debug("%s: old=%ld, want=%ld, new=%ld\n", clk_hw_get_name(hw),
+			 pll->rate, want, rate);
+	}
+
 	imx_pll14xx_calc_settings(pll, rate, *prate, &t);
 
 	return t.rate;
@@ -343,6 +361,8 @@ static int clk_pll1416x_set_rate(struct clk_hw *hw, unsigned long drate,
 	tmp &= ~BYPASS_MASK;
 	writel_relaxed(tmp, pll->base + GNRL_CTL);
 
+	pll->rate = rate->rate;
+
 	return 0;
 }
 

-- 
2.34.1

[PATCH 10/13] clk: imx: composite-8m: convert compute_dividers to void

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

As the current implementation cannot fail, drop the return value.

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/clk/imx/clk-composite-8m.c | 18 ++++++------------
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
index 27a08c50ac1d..a121f1285110 100644
--- a/drivers/clk/imx/clk-composite-8m.c
+++ b/drivers/clk/imx/clk-composite-8m.c
@@ -47,13 +47,12 @@ static unsigned long imx8m_clk_composite_divider_recalc_rate(struct clk_hw *hw,
 				   divider->flags, PCG_DIV_WIDTH);
 }
 
-static int imx8m_clk_composite_compute_dividers(unsigned long rate,
+static void imx8m_clk_composite_compute_dividers(unsigned long rate,
 						unsigned long parent_rate,
 						int *prediv, int *postdiv)
 {
 	int div1, div2;
 	int error = INT_MAX;
-	int ret = -EINVAL;
 
 	*prediv = 1;
 	*postdiv = 1;
@@ -66,11 +65,9 @@ static int imx8m_clk_composite_compute_dividers(unsigned long rate,
 				*prediv = div1;
 				*postdiv = div2;
 				error = new_error;
-				ret = 0;
 			}
 		}
 	}
-	return ret;
 }
 
 static long imx8m_clk_composite_divider_round_rate(struct clk_hw *hw,
@@ -80,8 +77,8 @@ static long imx8m_clk_composite_divider_round_rate(struct clk_hw *hw,
 	int prediv_value;
 	int div_value;
 
-	imx8m_clk_composite_compute_dividers(rate, *prate,
-						&prediv_value, &div_value);
+	imx8m_clk_composite_compute_dividers(rate, *prate, &prediv_value,
+					     &div_value);
 	rate = DIV_ROUND_UP(*prate, prediv_value);
 
 	return DIV_ROUND_UP(rate, div_value);
@@ -96,13 +93,10 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw,
 	unsigned long flags;
 	int prediv_value;
 	int div_value;
-	int ret;
 	u32 orig, val;
 
-	ret = imx8m_clk_composite_compute_dividers(rate, parent_rate,
-						&prediv_value, &div_value);
-	if (ret)
-		return -EINVAL;
+	imx8m_clk_composite_compute_dividers(rate, parent_rate,	&prediv_value,
+					     &div_value);
 
 	spin_lock_irqsave(divider->lock, flags);
 
@@ -118,7 +112,7 @@ static int imx8m_clk_composite_divider_set_rate(struct clk_hw *hw,
 
 	spin_unlock_irqrestore(divider->lock, flags);
 
-	return ret;
+	return 0;
 }
 
 static int imx8m_divider_determine_rate(struct clk_hw *hw,

-- 
2.34.1

[PATCH 11/13] clk: imx: composite-8m: implement CLK_SET_RATE_PARENT

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

One of the key parts to enable dynamic clock propagation on the imx8m,
are the consumer-facing composites. They currently only divide,
therefore the parent must be already quite good in shape to provide a
close enough rate. Therefore, the parents are usually hard-assigned in
the dt. To workaround that, this commit enables propagation to the
parent of the composite.

If a rate cannot be reached exactly by only dividing, the parent is
asked (for now simply for the exact required rate - no dividers taken
into account). If the parent already has a configured rate, it's the
parent's job to ensure that all children are satisfied.

By using a notifier, the propagation-enabled clocks listen to clock
changes coming from the parent. If one is happening, it's the composites
job to verify if the rate is satisfying and if it is an intended change.

Otherwise, countermeasures have to be taken into account (e.g. setting
the rate back or aborting the change).

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/clk/imx/clk-composite-8m.c | 71 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 68 insertions(+), 3 deletions(-)

diff --git a/drivers/clk/imx/clk-composite-8m.c b/drivers/clk/imx/clk-composite-8m.c
index a121f1285110..068f61df28b1 100644
--- a/drivers/clk/imx/clk-composite-8m.c
+++ b/drivers/clk/imx/clk-composite-8m.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/clk-provider.h>
+#include <linux/clk.h>
 #include <linux/errno.h>
 #include <linux/export.h>
 #include <linux/io.h>
@@ -119,8 +120,12 @@ static int imx8m_divider_determine_rate(struct clk_hw *hw,
 				      struct clk_rate_request *req)
 {
 	struct clk_divider *divider = to_clk_divider(hw);
+	struct clk_hw *parent = clk_hw_get_parent(hw);
 	int prediv_value;
 	int div_value;
+	unsigned long target_rate;
+	struct clk_rate_request req_parent;
+	int ret;
 
 	/* if read only, just return current value */
 	if (divider->flags & CLK_DIVIDER_READ_ONLY) {
@@ -140,9 +145,29 @@ static int imx8m_divider_determine_rate(struct clk_hw *hw,
 						 divider->flags, prediv_value * div_value);
 	}
 
-	return divider_determine_rate(hw, req, divider->table,
-				      PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
-				      divider->flags);
+	target_rate = req->rate;
+	ret = divider_determine_rate(hw, req, divider->table,
+				     PCG_PREDIV_WIDTH + PCG_DIV_WIDTH,
+				     divider->flags);
+	if (ret || req->rate == target_rate)
+		return ret;
+
+	/*
+	 * If re-configuring the parent gives a better rate, do this instead.
+	 * Avoid re-parenting for now, which could be done first if a possible
+	 * parent already has a satisfying rate. The implementation does not
+	 * consider the dividers between the parent and the current clock.
+	 */
+	clk_hw_forward_rate_request(hw, req, parent, &req_parent, target_rate);
+	if (__clk_determine_rate(parent, &req_parent))
+		return 0;
+
+	if (abs(req_parent.rate - target_rate) < abs(req->rate - target_rate)) {
+		req->rate = req_parent.rate;
+		req->best_parent_rate = req_parent.rate;
+	}
+
+	return 0;
 }
 
 static const struct clk_ops imx8m_clk_composite_divider_ops = {
@@ -198,6 +223,33 @@ static const struct clk_ops imx8m_clk_composite_mux_ops = {
 	.determine_rate = imx8m_clk_composite_mux_determine_rate,
 };
 
+static int imx8m_clk_composite_notifier_fn(struct notifier_block *notifier,
+					   unsigned long code, void *data)
+{
+	struct clk_notifier_data *cnd = data;
+	struct clk_hw *hw = __clk_get_hw(cnd->clk);
+
+	if (code != PRE_RATE_CHANGE)
+		return NOTIFY_OK;
+
+	if (!__clk_is_rate_set(cnd->clk))
+		return NOTIFY_OK;
+
+	/*
+	 * Consumer of a composite-m8 clock usually use the root clk, a gate
+	 * connected to the composite (e.g. media_ldb and media_ldb_root).
+	 * Therefore, evaluate the trigger's parent too.
+	 */
+	if (cnd->clk != cnd->trigger && cnd->clk != clk_get_parent(cnd->trigger))
+		return notifier_from_errno(clk_hw_set_rate(hw, cnd->old_rate));
+
+	return NOTIFY_OK;
+}
+
+static struct notifier_block imx8m_clk_composite_notifier = {
+	.notifier_call = imx8m_clk_composite_notifier_fn,
+};
+
 struct clk_hw *__imx8m_clk_hw_composite(const char *name,
 					const char * const *parent_names,
 					int num_parents, void __iomem *reg,
@@ -211,6 +263,7 @@ struct clk_hw *__imx8m_clk_hw_composite(const char *name,
 	struct clk_mux *mux = NULL;
 	const struct clk_ops *divider_ops;
 	const struct clk_ops *mux_ops;
+	int ret;
 
 	mux = kzalloc(sizeof(*mux), GFP_KERNEL);
 	if (!mux)
@@ -268,6 +321,18 @@ struct clk_hw *__imx8m_clk_hw_composite(const char *name,
 	if (IS_ERR(hw))
 		goto fail;
 
+	/*
+	 * register a notifier which should switch back to the configured rate
+	 * if the rate is going to be changed unintentionally.
+	 */
+	if (flags & CLK_SET_RATE_PARENT) {
+		ret = clk_notifier_register(hw->clk, &imx8m_clk_composite_notifier);
+		if (ret) {
+			hw = ERR_PTR(ret);
+			goto fail;
+		}
+	}
+
 	return hw;
 
 fail:

-- 
2.34.1

[PATCH 12/13] clk: imx: imx8mp: allow LVDS clocks to set parent rate

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

As composit-8m are now ready to dynamically propagate the
required clock rate and pll14xx is able to keep its children in the
configured state, allow media_ldb and media_disp2_pix to set the rate of
their parent (which is usually video_pll for both).

Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 drivers/clk/imx/clk-imx8mp.c | 4 ++--
 drivers/clk/imx/clk.h        | 4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
index 670aa2bab301..fbc63012dab9 100644
--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -547,7 +547,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
 	hws[IMX8MP_CLK_AHB] = imx8m_clk_hw_composite_bus_critical("ahb_root", imx8mp_ahb_sels, ccm_base + 0x9000);
 	hws[IMX8MP_CLK_AUDIO_AHB] = imx8m_clk_hw_composite_bus("audio_ahb", imx8mp_audio_ahb_sels, ccm_base + 0x9100);
 	hws[IMX8MP_CLK_MIPI_DSI_ESC_RX] = imx8m_clk_hw_composite_bus("mipi_dsi_esc_rx", imx8mp_mipi_dsi_esc_rx_sels, ccm_base + 0x9200);
-	hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300);
+	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_IPG_ROOT] = imx_clk_hw_divider2("ipg_root", "ahb_root", ccm_base + 0x9080, 0, 1);
 
@@ -611,7 +611,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
 	hws[IMX8MP_CLK_MEDIA_MIPI_PHY1_REF] = imx8m_clk_hw_composite("media_mipi_phy1_ref", imx8mp_media_mipi_phy1_ref_sels, ccm_base + 0xbd80);
 	hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00);
 	hws[IMX8MP_CLK_MEDIA_CAM2_PIX] = imx8m_clk_hw_composite("media_cam2_pix", imx8mp_media_cam2_pix_sels, ccm_base + 0xbe80);
-	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_flags("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00, CLK_SET_RATE_PARENT);
 	hws[IMX8MP_CLK_MEMREPAIR] = imx8m_clk_hw_composite_critical("mem_repair", imx8mp_memrepair_sels, ccm_base + 0xbf80);
 	hws[IMX8MP_CLK_MEDIA_MIPI_TEST_BYTE] = imx8m_clk_hw_composite("media_mipi_test_byte", imx8mp_media_mipi_test_byte_sels, ccm_base + 0xc100);
 	hws[IMX8MP_CLK_ECSPI3] = imx8m_clk_hw_composite("ecspi3", imx8mp_ecspi3_sels, ccm_base + 0xc180);
diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
index adb7ad649a0d..aa5202f284f3 100644
--- a/drivers/clk/imx/clk.h
+++ b/drivers/clk/imx/clk.h
@@ -442,6 +442,10 @@ struct clk_hw *__imx8m_clk_hw_composite(const char *name,
 	_imx8m_clk_hw_composite(name, parent_names, reg, \
 			IMX_COMPOSITE_BUS, IMX_COMPOSITE_CLK_FLAGS_DEFAULT)
 
+#define imx8m_clk_hw_composite_bus_flags(name, parent_names, reg, flags) \
+	_imx8m_clk_hw_composite(name, parent_names, reg, \
+			IMX_COMPOSITE_BUS, IMX_COMPOSITE_CLK_FLAGS_DEFAULT | flags)
+
 #define imx8m_clk_hw_composite_bus_critical(name, parent_names, reg)	\
 	_imx8m_clk_hw_composite(name, parent_names, reg, \
 			IMX_COMPOSITE_BUS, IMX_COMPOSITE_CLK_FLAGS_CRITICAL)

-- 
2.34.1

[PATCH 13/13] arm64: dts: imx8mp: remove assigned-clock-rate of IMX8MP_VIDEO_PLL1

[Benjamin Bara]

From: Benjamin Bara <benjamin.bara@skidata.com>

Similar to commit 16c984524862 ("arm64: dts: imx8mp: don't initialize
audio clocks from CCM node").

With commit b09c68dc57c9 ("clk: imx: pll14xx: Support dynamic rates") in
place, the clock consumer (e.g. a panel) is able to set a more suitable
rate for the IMX8MP_VIDEO_PLL1. As composite-8m is now able to propagate
the rate through, avoid setting a rate in the dtsi.

Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Signed-off-by: Benjamin Bara <benjamin.bara@skidata.com>
---
 arch/arm64/boot/dts/freescale/imx8mp.dtsi | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp.dtsi b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
index 9539d747e28e..f40b40ee8f9e 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mp.dtsi
@@ -1742,7 +1742,6 @@ media_blk_ctrl: blk-ctrl@32ec0000 {
 						  <&clk IMX8MP_CLK_MEDIA_APB>,
 						  <&clk IMX8MP_CLK_MEDIA_DISP1_PIX>,
 						  <&clk IMX8MP_CLK_MEDIA_DISP2_PIX>,
-						  <&clk IMX8MP_VIDEO_PLL1>,
 						  <&clk IMX8MP_CLK_MEDIA_MIPI_PHY1_REF>;
 				assigned-clock-parents = <&clk IMX8MP_SYS_PLL2_1000M>,
 							 <&clk IMX8MP_SYS_PLL1_800M>,
@@ -1750,8 +1749,7 @@ media_blk_ctrl: blk-ctrl@32ec0000 {
 							 <&clk IMX8MP_VIDEO_PLL1_OUT>,
 							 <&clk IMX8MP_CLK_24M>;
 				assigned-clock-rates = <500000000>, <200000000>,
-						       <0>, <0>, <1039500000>,
-						       <24000000>;
+						       <0>, <0>, <24000000>;
 				#power-domain-cells = <1>;
 
 				lvds_bridge: bridge@5c {

-- 
2.34.1

Re: [PATCH 00/13] imx8mp: first clock propagation attempt (for LVDS)

[Adam Ford]

On Sun, Sep 17, 2023 at 3:40 PM Benjamin Bara <bbara93@gmail.com> wrote:
>
> Hi!
>
> Target of this series is to dynamically set the rate of video_pll1 to
> the required LVDS clock rate(s), which are configured by the panel, and
> the lvds-bridge respectively.
>
> Some background:
> The LVDS panel requires two clocks: the crtc clock and the lvds clock.
> The lvds rate is always 7x the crtc rate. On the imx8mp, these are
> assigned to media_disp2_pix and media_ldb, which are both
> clk-composite-8m. The rates are set by drm_client_modeset_commit() (and
> later by fsl_ldb_atomic_enable()), and the fsl-ldb driver, first crtc,
> then lvds. The parent is typically assigned to video_pll1, which is a
> clk-pll14xx (pll1443x).
>
> The main problem:
> As the clk-composite-8m currently doesn't support CLK_SET_RATE_PARENT,
> the crtc rate is not propagated to video_pll1, and therefore must be
> assigned in the device-tree manually.
>
> The idea:
> Enable CLK_SET_RATE_PARENT, at least for media_disp2_pix and media_ldb.
> When this is done, ensure that the pll1443x can be re-configured,
> meaning it ensures that an already configured rate (crtc rate) is still
> supported when a second child requires a different rate (lvds rate). As

Have you tested with the DSI as well?  If memory servers, the DSI
clock and the LVDS clock are both clocked from the same video_pll.  At
one time, I had done some experimentation with trying the DSI
connected to an HDMI bridge chip connected to a monitor and the LVDS
was connected to a display panel with a static resolution and refresh
rate.  For my LVDS display, it needs 30MHz to display properly, but
various HDMI resolutions needed values that were not evenly divisible
by 30MHz which appeared to cause display sync issues when trying to
share a clock that was trying to dynamically adjust for two different
displays especially when trying to change the resoltuion of the HDMI
display to various values for different resolutions.

> the children have divider, the current approach is straight forward by
> calculating the LCM of the required rates. During the rate change of the
> PLL, it must ensure that all children still have the configured rate at
> the end (and maybe also bypass the clock while doing so?). This is done
> by implementing a notifier function for the clk-composite-8m. The tricky
> part is now to find out if the rate change was intentional or not. This
> is done by adding the "change trigger" to the notify data. In our case,
> we now can infer if we aren't the change trigger, we need to keep the
> existing rate after the PLL's rate change. We keep the existing rate by
> modifying the new_rate of the clock's core, as we are quite late in an
> already ongoing clock change process.
>
> Future work:
> The re-configuration of the PLL can definitely be improved for other use
> cases where the children have more fancy inter-dependencies. That's one
> of the main reasons I currently only touched the mentioned clocks.
> Additionally, it might make sense to automatically re-parent if a
> different possible parent suits better.
> For the core part, I thought about extending my "unintentional change
> check" so that the core ensures that the children keep the configured
> rate, which might not be easy as the parent could be allowed to "round",
> but it's not clear (at least to me yet) how much rounding is allowed. I
> found a similar discussion posted here[1], therefore added Frank and
> Maxime.
>
> Thanks & regards,
> Benjamin
>
> [1] https://lore.kernel.org/lkml/20230825-pll-mipi_keep_rate-v1-0-35bc43570730@oltmanns.dev/
>
> ---
> Benjamin Bara (13):
>       arm64: dts: imx8mp: lvds_bridge: use root instead of composite
>       arm64: dts: imx8mp: re-parent IMX8MP_CLK_MEDIA_MIPI_PHY1_REF
>       clk: implement clk_hw_set_rate()
>       clk: print debug message if parent change is ignored
>       clk: keep track of the trigger of an ongoing clk_set_rate
>       clk: keep track if a clock is explicitly configured
>       clk: detect unintended rate changes
>       clk: divider: stop early if an optimal divider is found
>       clk: imx: pll14xx: consider active rate for re-config
>       clk: imx: composite-8m: convert compute_dividers to void
>       clk: imx: composite-8m: implement CLK_SET_RATE_PARENT
>       clk: imx: imx8mp: allow LVDS clocks to set parent rate
>       arm64: dts: imx8mp: remove assigned-clock-rate of IMX8MP_VIDEO_PLL1
>
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi |  14 +--
>  drivers/clk/clk-divider.c                 |   9 ++
>  drivers/clk/clk.c                         | 146 +++++++++++++++++++++++++++++-
>  drivers/clk/imx/clk-composite-8m.c        |  89 +++++++++++++++---
>  drivers/clk/imx/clk-imx8mp.c              |   4 +-
>  drivers/clk/imx/clk-pll14xx.c             |  20 ++++
>  drivers/clk/imx/clk.h                     |   4 +
>  include/linux/clk-provider.h              |   2 +
>  include/linux/clk.h                       |   2 +
>  9 files changed, 261 insertions(+), 29 deletions(-)
> ---
> base-commit: e143016b56ecb0fcda5bb6026b0a25fe55274f56
> change-id: 20230913-imx8mp-dtsi-7c6e25907e0e
>
> Best regards,
> --
> Benjamin Bara <benjamin.bara@skidata.com>
>

Re: [PATCH 00/13] imx8mp: first clock propagation attempt (for LVDS)

[Benjamin Bara]

Hi Adam!

On Mon, 18 Sept 2023 at 07:00, Adam Ford <aford173@gmail.com> wrote:
> On Sun, Sep 17, 2023 at 3:40 PM Benjamin Bara <bbara93@gmail.com> wrote:
> > The idea:
> > Enable CLK_SET_RATE_PARENT, at least for media_disp2_pix and media_ldb.
> > When this is done, ensure that the pll1443x can be re-configured,
> > meaning it ensures that an already configured rate (crtc rate) is still
> > supported when a second child requires a different rate (lvds rate). As
>
> Have you tested with the DSI as well?  If memory servers, the DSI
> clock and the LVDS clock are both clocked from the same video_pll.  At
> one time, I had done some experimentation with trying the DSI
> connected to an HDMI bridge chip connected to a monitor and the LVDS
> was connected to a display panel with a static resolution and refresh
> rate.  For my LVDS display, it needs 30MHz to display properly, but
> various HDMI resolutions needed values that were not evenly divisible
> by 30MHz which appeared to cause display sync issues when trying to
> share a clock that was trying to dynamically adjust for two different
> displays especially when trying to change the resoltuion of the HDMI
> display to various values for different resolutions.

Unfortunately I haven't. I think if you have the use case to support
different "run-time-dynamic" (HDMI) rates in parallel with a static
(LVDS) rate, it probably makes sense (for now) to just use a LVDS panel
which can be feeded from one of the static PLLs directly and do a manual
re-parenting in the dt. The manual re-parenting could be replaced by an
automated re-parenting in the composite driver. When I think about it,
it might make sense to extend clk-divider's clk_divider_bestdiv()[1]
(which is currently used by the composite-8m) with a "find the best
parent" implementation, something like:
1. are we in range if we divide the active parent with all possible
   dividers? (already existing)
2. are we in range if we switch to a different parent and divide it with
   all possible dividers?
3. are we in range if we re-configure a possible parent (and switch to
   it)?

Steps 2 & 3 are e.g. implemented by at91's clk-master[2]. There are
maybe also "smarter" solutions to the problem beside trying every
possibility. Anyways, we already have a CLK_SET_RATE_NO_REPARENT which
would indicate if we are allowed to do so.

For static use cases involving both, I would probably (for now) go with
a hard-assigned, tested clock rate in the dt. IMHO, this should always
work as fall-back.

Regards,
Benjamin

[1] https://elixir.bootlin.com/linux/v6.5.3/source/drivers/clk/clk-divider.c#L304
[2] https://elixir.bootlin.com/linux/v6.5.3/source/drivers/clk/at91/clk-master.c#L586

Re: [PATCH 00/13] imx8mp: first clock propagation attempt (for LVDS)

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1877 bytes --]

On Mon, Sep 18, 2023 at 07:59:16PM +0200, Benjamin Bara wrote:
> Hi Adam!
> 
> On Mon, 18 Sept 2023 at 07:00, Adam Ford <aford173@gmail.com> wrote:
> > On Sun, Sep 17, 2023 at 3:40 PM Benjamin Bara <bbara93@gmail.com> wrote:
> > > The idea:
> > > Enable CLK_SET_RATE_PARENT, at least for media_disp2_pix and media_ldb.
> > > When this is done, ensure that the pll1443x can be re-configured,
> > > meaning it ensures that an already configured rate (crtc rate) is still
> > > supported when a second child requires a different rate (lvds rate). As
> >
> > Have you tested with the DSI as well?  If memory servers, the DSI
> > clock and the LVDS clock are both clocked from the same video_pll.  At
> > one time, I had done some experimentation with trying the DSI
> > connected to an HDMI bridge chip connected to a monitor and the LVDS
> > was connected to a display panel with a static resolution and refresh
> > rate.  For my LVDS display, it needs 30MHz to display properly, but
> > various HDMI resolutions needed values that were not evenly divisible
> > by 30MHz which appeared to cause display sync issues when trying to
> > share a clock that was trying to dynamically adjust for two different
> > displays especially when trying to change the resoltuion of the HDMI
> > display to various values for different resolutions.
> 
> Unfortunately I haven't. I think if you have the use case to support
> different "run-time-dynamic" (HDMI) rates in parallel with a static
> (LVDS) rate

If anything, LVDS is harder to deal with than HDMI. HDMI only has a
handful of clock rates (74.250, 148.5, 297 and 594MHz mostly) while LVDS
is more freeform.

We are more likely to change the rate on an HDMI device though, but a
rate change from 1080p to 720p would only require a divide by two (from
148.5 to 74.250) so fairly easy to do.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 00/13] imx8mp: first clock propagation attempt (for LVDS)

[Frank Oltmanns]

Hi Benjamin!

On 2023-09-18 at 00:39:56 +0200, Benjamin Bara <bbara93@gmail.com> wrote:
> Hi!
>
> Target of this series is to dynamically set the rate of video_pll1 to
> the required LVDS clock rate(s), which are configured by the panel, and
> the lvds-bridge respectively.
>
> Some background:
> The LVDS panel requires two clocks: the crtc clock and the lvds clock.
> The lvds rate is always 7x the crtc rate. On the imx8mp, these are
> assigned to media_disp2_pix and media_ldb, which are both
> clk-composite-8m. The rates are set by drm_client_modeset_commit() (and
> later by fsl_ldb_atomic_enable()), and the fsl-ldb driver, first crtc,
> then lvds. The parent is typically assigned to video_pll1, which is a
> clk-pll14xx (pll1443x).
>
> The main problem:
> As the clk-composite-8m currently doesn't support CLK_SET_RATE_PARENT,
> the crtc rate is not propagated to video_pll1, and therefore must be
> assigned in the device-tree manually.
>
> The idea:
> Enable CLK_SET_RATE_PARENT, at least for media_disp2_pix and media_ldb.
> When this is done, ensure that the pll1443x can be re-configured,
> meaning it ensures that an already configured rate (crtc rate) is still
> supported when a second child requires a different rate (lvds rate). As
> the children have divider, the current approach is straight forward by
> calculating the LCM of the required rates. During the rate change of the
> PLL, it must ensure that all children still have the configured rate at
> the end (and maybe also bypass the clock while doing so?). This is done
> by implementing a notifier function for the clk-composite-8m. The tricky
> part is now to find out if the rate change was intentional or not. This
> is done by adding the "change trigger" to the notify data. In our case,
> we now can infer if we aren't the change trigger, we need to keep the
> existing rate after the PLL's rate change. We keep the existing rate by
> modifying the new_rate of the clock's core, as we are quite late in an
> already ongoing clock change process.
>
> Future work:
> The re-configuration of the PLL can definitely be improved for other use
> cases where the children have more fancy inter-dependencies. That's one
> of the main reasons I currently only touched the mentioned clocks.
> Additionally, it might make sense to automatically re-parent if a
> different possible parent suits better.
> For the core part, I thought about extending my "unintentional change
> check" so that the core ensures that the children keep the configured
> rate, which might not be easy as the parent could be allowed to "round",
> but it's not clear (at least to me yet) how much rounding is allowed. I
> found a similar discussion posted here[1], therefore added Frank and
> Maxime.

Thank you very much for including me in the discussion. If I understood
Maxime correctly, your proposal is close to what he was suggesting in
the discussion you referenced. Unfortunately, it doesn't cover the
rounding aspect (which you also mentioned in your cover letter and the
description for clk_detect_unintended_rate_changes in patch 7. I've been
pondering the last three weeks how to find a good solution to this
problem, but so far haven't found any.

Hopefully, your suggestion spurs some new thoughts.

Thanks,
  Frank

>
> Thanks & regards,
> Benjamin
>
> [1] https://lore.kernel.org/lkml/20230825-pll-mipi_keep_rate-v1-0-35bc43570730@oltmanns.dev/
>
> ---
> Benjamin Bara (13):
>       arm64: dts: imx8mp: lvds_bridge: use root instead of composite
>       arm64: dts: imx8mp: re-parent IMX8MP_CLK_MEDIA_MIPI_PHY1_REF
>       clk: implement clk_hw_set_rate()
>       clk: print debug message if parent change is ignored
>       clk: keep track of the trigger of an ongoing clk_set_rate
>       clk: keep track if a clock is explicitly configured
>       clk: detect unintended rate changes
>       clk: divider: stop early if an optimal divider is found
>       clk: imx: pll14xx: consider active rate for re-config
>       clk: imx: composite-8m: convert compute_dividers to void
>       clk: imx: composite-8m: implement CLK_SET_RATE_PARENT
>       clk: imx: imx8mp: allow LVDS clocks to set parent rate
>       arm64: dts: imx8mp: remove assigned-clock-rate of IMX8MP_VIDEO_PLL1
>
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi |  14 +--
>  drivers/clk/clk-divider.c                 |   9 ++
>  drivers/clk/clk.c                         | 146 +++++++++++++++++++++++++++++-
>  drivers/clk/imx/clk-composite-8m.c        |  89 +++++++++++++++---
>  drivers/clk/imx/clk-imx8mp.c              |   4 +-
>  drivers/clk/imx/clk-pll14xx.c             |  20 ++++
>  drivers/clk/imx/clk.h                     |   4 +
>  include/linux/clk-provider.h              |   2 +
>  include/linux/clk.h                       |   2 +
>  9 files changed, 261 insertions(+), 29 deletions(-)
> ---
> base-commit: e143016b56ecb0fcda5bb6026b0a25fe55274f56
> change-id: 20230913-imx8mp-dtsi-7c6e25907e0e
>
> Best regards,

Re: [PATCH 00/13] imx8mp: first clock propagation attempt (for LVDS)

[Benjamin Bara]

Hi Frank!

On Mon, 18 Sept 2023 at 19:24, Frank Oltmanns <frank@oltmanns.dev> wrote:
> On 2023-09-18 at 00:39:56 +0200, Benjamin Bara <bbara93@gmail.com> wrote:
> Thank you very much for including me in the discussion. If I understood
> Maxime correctly, your proposal is close to what he was suggesting in
> the discussion you referenced. Unfortunately, it doesn't cover the
> rounding aspect (which you also mentioned in your cover letter and the
> description for clk_detect_unintended_rate_changes in patch 7. I've been
> pondering the last three weeks how to find a good solution to this
> problem, but so far haven't found any.

I think if we stick to the idea of always enforcing the exact "typical
rate", we cannot avoid physically impossible cases. IMHO, it might make
sense to add a set_rate() function with a "timing_entry" (e.g. used by
display_timing.h[1]) to the clock API, which gives a suggestion but also
defines the "real" boundaries. This would provide a shared parent PLL
more freedom to provide a satisfying rate for all its children.

Regards
Benjamin

[1] https://elixir.bootlin.com/linux/v6.5.3/source/include/video/display_timing.h#L64

Re: [PATCH 00/13] imx8mp: first clock propagation attempt (for LVDS)

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1514 bytes --]

On Mon, Sep 18, 2023 at 08:05:48PM +0200, Benjamin Bara wrote:
> Hi Frank!
> 
> On Mon, 18 Sept 2023 at 19:24, Frank Oltmanns <frank@oltmanns.dev> wrote:
> > On 2023-09-18 at 00:39:56 +0200, Benjamin Bara <bbara93@gmail.com> wrote:
> > Thank you very much for including me in the discussion. If I understood
> > Maxime correctly, your proposal is close to what he was suggesting in
> > the discussion you referenced. Unfortunately, it doesn't cover the
> > rounding aspect (which you also mentioned in your cover letter and the
> > description for clk_detect_unintended_rate_changes in patch 7. I've been
> > pondering the last three weeks how to find a good solution to this
> > problem, but so far haven't found any.
> 
> I think if we stick to the idea of always enforcing the exact "typical
> rate", we cannot avoid physically impossible cases. IMHO, it might make
> sense to add a set_rate() function with a "timing_entry" (e.g. used by
> display_timing.h[1]) to the clock API, which gives a suggestion but also
> defines the "real" boundaries. This would provide a shared parent PLL
> more freedom to provide a satisfying rate for all its children.

It's definitely something we should do, and I've wanted to do that for a
while.

The clock rate is not the only thing we can change though. The usual
trick is to modify the blanking areas to come up with a rate that
matches what the hardware can provide without modifying the framerate.

It belongs more in a KMS helper

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 00/13] imx8mp: first clock propagation attempt (for LVDS)

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2714 bytes --]

Hi,

On Mon, Sep 18, 2023 at 12:39:56AM +0200, Benjamin Bara wrote:
> Target of this series is to dynamically set the rate of video_pll1 to
> the required LVDS clock rate(s), which are configured by the panel, and
> the lvds-bridge respectively.
> 
> Some background:
> The LVDS panel requires two clocks: the crtc clock and the lvds clock.
> The lvds rate is always 7x the crtc rate. On the imx8mp, these are
> assigned to media_disp2_pix and media_ldb, which are both
> clk-composite-8m. The rates are set by drm_client_modeset_commit() (and
> later by fsl_ldb_atomic_enable()), and the fsl-ldb driver, first crtc,
> then lvds. The parent is typically assigned to video_pll1, which is a
> clk-pll14xx (pll1443x).
> 
> The main problem:
> As the clk-composite-8m currently doesn't support CLK_SET_RATE_PARENT,
> the crtc rate is not propagated to video_pll1, and therefore must be
> assigned in the device-tree manually.
> 
> The idea:
> Enable CLK_SET_RATE_PARENT, at least for media_disp2_pix and media_ldb.
> When this is done, ensure that the pll1443x can be re-configured,
> meaning it ensures that an already configured rate (crtc rate) is still
> supported when a second child requires a different rate (lvds rate). As
> the children have divider, the current approach is straight forward by
> calculating the LCM of the required rates. During the rate change of the
> PLL, it must ensure that all children still have the configured rate at
> the end (and maybe also bypass the clock while doing so?). This is done
> by implementing a notifier function for the clk-composite-8m. The tricky
> part is now to find out if the rate change was intentional or not. This
> is done by adding the "change trigger" to the notify data. In our case,
> we now can infer if we aren't the change trigger, we need to keep the
> existing rate after the PLL's rate change. We keep the existing rate by
> modifying the new_rate of the clock's core, as we are quite late in an
> already ongoing clock change process.

So just like the discussion we had on the Allwinner stuff, I don't think
you can cover it completely within the framework. If we take a step
backward, I guess what you want is that you have multiple clocks,
feeding multiple displays at varying clock rates depending on the
resolution, and the parent needs to accomodate all of them, right?

Could you share the clock tree and the capability of each clocks (range
of the multipliers / dividers mostly)?

I'm wondering if we couldn't set the parent clock to a fairly high rate
that would be high enough for each child to reach whatever rate it needs
to have without the need for CLK_SET_RATE_PARENT.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 00/13] imx8mp: first clock propagation attempt (for LVDS)

[Adam Ford]

On Sun, Sep 17, 2023 at 5:40 PM Benjamin Bara <bbara93@gmail.com> wrote:
>
> Hi!
>
> Target of this series is to dynamically set the rate of video_pll1 to
> the required LVDS clock rate(s), which are configured by the panel, and
> the lvds-bridge respectively.
>
> Some background:
> The LVDS panel requires two clocks: the crtc clock and the lvds clock.
> The lvds rate is always 7x the crtc rate. On the imx8mp, these are
> assigned to media_disp2_pix and media_ldb, which are both

Could the LDB driver be updated to take in the crtc clock as a
parameter, then set the media_ldb to 7x crct clock.  I wonder if that
might simplify the task a bit.

> clk-composite-8m. The rates are set by drm_client_modeset_commit() (and
> later by fsl_ldb_atomic_enable()), and the fsl-ldb driver, first crtc,
> then lvds. The parent is typically assigned to video_pll1, which is a
> clk-pll14xx (pll1443x).
>
> The main problem:
> As the clk-composite-8m currently doesn't support CLK_SET_RATE_PARENT,
> the crtc rate is not propagated to video_pll1, and therefore must be
> assigned in the device-tree manually.
>
> The idea:
> Enable CLK_SET_RATE_PARENT, at least for media_disp2_pix and media_ldb.
> When this is done, ensure that the pll1443x can be re-configured,
> meaning it ensures that an already configured rate (crtc rate) is still
> supported when a second child requires a different rate (lvds rate). As

I still have concerns that the CLK_SET_RATE_PARENT may break the
media_disp1_pix if media_disp2_pix is changing it.

I think we should consider adding some sort of configurable flag to
the CCM that lets people choose  if CLK_SET_RATE_PARENT should be set
or not in the device tree instead of hard-coding it either on or off.
This would give people the flexibility of stating whether
media_disp1_pix, media_disp2_pix, both or neither could set
CLK_SET_RATE_PARENT.

I believe the imx8mp-evk can support both LVDS-> HDMI and DSI->HDMI
bridges, and I fear that if they are trying to both set different
clock rates, this may break something and the clocks need to be
selected in advance to give people a bunch of HDMI options as well as
being able to divide down to support the LVDS.

I think some of the displays could be tied to one of the Audio PLL's,
so I might experiment with splitting the media_disp1_pix and
media_disp2_pix from each other to see how well .


> the children have divider, the current approach is straight forward by
> calculating the LCM of the required rates. During the rate change of the
> PLL, it must ensure that all children still have the configured rate at
> the end (and maybe also bypass the clock while doing so?). This is done
> by implementing a notifier function for the clk-composite-8m. The tricky
> part is now to find out if the rate change was intentional or not. This
> is done by adding the "change trigger" to the notify data. In our case,
> we now can infer if we aren't the change trigger, we need to keep the
> existing rate after the PLL's rate change. We keep the existing rate by
> modifying the new_rate of the clock's core, as we are quite late in an
> already ongoing clock change process.
>
> Future work:
> The re-configuration of the PLL can definitely be improved for other use
> cases where the children have more fancy inter-dependencies. That's one
> of the main reasons I currently only touched the mentioned clocks.
> Additionally, it might make sense to automatically re-parent if a
> different possible parent suits better.
> For the core part, I thought about extending my "unintentional change
> check" so that the core ensures that the children keep the configured
> rate, which might not be easy as the parent could be allowed to "round",
> but it's not clear (at least to me yet) how much rounding is allowed. I
> found a similar discussion posted here[1], therefore added Frank and
> Maxime.
>
> Thanks & regards,
> Benjamin
>
> [1] https://lore.kernel.org/lkml/20230825-pll-mipi_keep_rate-v1-0-35bc43570730@oltmanns.dev/
>
> ---
> Benjamin Bara (13):
>       arm64: dts: imx8mp: lvds_bridge: use root instead of composite
>       arm64: dts: imx8mp: re-parent IMX8MP_CLK_MEDIA_MIPI_PHY1_REF
>       clk: implement clk_hw_set_rate()
>       clk: print debug message if parent change is ignored
>       clk: keep track of the trigger of an ongoing clk_set_rate
>       clk: keep track if a clock is explicitly configured
>       clk: detect unintended rate changes
>       clk: divider: stop early if an optimal divider is found
>       clk: imx: pll14xx: consider active rate for re-config
>       clk: imx: composite-8m: convert compute_dividers to void
>       clk: imx: composite-8m: implement CLK_SET_RATE_PARENT
>       clk: imx: imx8mp: allow LVDS clocks to set parent rate
>       arm64: dts: imx8mp: remove assigned-clock-rate of IMX8MP_VIDEO_PLL1
>
>  arch/arm64/boot/dts/freescale/imx8mp.dtsi |  14 +--
>  drivers/clk/clk-divider.c                 |   9 ++
>  drivers/clk/clk.c                         | 146 +++++++++++++++++++++++++++++-
>  drivers/clk/imx/clk-composite-8m.c        |  89 +++++++++++++++---
>  drivers/clk/imx/clk-imx8mp.c              |   4 +-
>  drivers/clk/imx/clk-pll14xx.c             |  20 ++++
>  drivers/clk/imx/clk.h                     |   4 +
>  include/linux/clk-provider.h              |   2 +
>  include/linux/clk.h                       |   2 +
>  9 files changed, 261 insertions(+), 29 deletions(-)
> ---
> base-commit: e143016b56ecb0fcda5bb6026b0a25fe55274f56
> change-id: 20230913-imx8mp-dtsi-7c6e25907e0e
>
> Best regards,
> --
> Benjamin Bara <benjamin.bara@skidata.com>
>

Re: [PATCH 00/13] imx8mp: first clock propagation attempt (for LVDS)

[Alexander Stein]

Hi Adam,

Am Dienstag, 3. Oktober 2023, 15:28:05 CEST schrieb Adam Ford:
> On Sun, Sep 17, 2023 at 5:40 PM Benjamin Bara <bbara93@gmail.com> wrote:
> > Hi!
> > 
> > Target of this series is to dynamically set the rate of video_pll1 to
> > the required LVDS clock rate(s), which are configured by the panel, and
> > the lvds-bridge respectively.
> > 
> > Some background:
> > The LVDS panel requires two clocks: the crtc clock and the lvds clock.
> > The lvds rate is always 7x the crtc rate. On the imx8mp, these are
> > assigned to media_disp2_pix and media_ldb, which are both
> 
> Could the LDB driver be updated to take in the crtc clock as a
> parameter, then set the media_ldb to 7x crct clock.  I wonder if that
> might simplify the task a bit.

I'm not sure if you had something different in mind, but I guess this happens 
already in fsl_ldb_atomic_enable(), although using the mode clock.
As this might not always be possible, commit bd43a9844bc6f ("drm: bridge: ldb: 
Warn if LDB clock does not match requested link frequency") was added to 
indicate something might be wrong.
The main problem here is that both media_ldb and crct clock are not in a 
parent<->child relationship, but are siblings, configurable individually.

Best regards,
Alexander

> > clk-composite-8m. The rates are set by drm_client_modeset_commit() (and
> > later by fsl_ldb_atomic_enable()), and the fsl-ldb driver, first crtc,
> > then lvds. The parent is typically assigned to video_pll1, which is a
> > clk-pll14xx (pll1443x).
> > 
> > The main problem:
> > As the clk-composite-8m currently doesn't support CLK_SET_RATE_PARENT,
> > the crtc rate is not propagated to video_pll1, and therefore must be
> > assigned in the device-tree manually.
> > 
> > The idea:
> > Enable CLK_SET_RATE_PARENT, at least for media_disp2_pix and media_ldb.
> > When this is done, ensure that the pll1443x can be re-configured,
> > meaning it ensures that an already configured rate (crtc rate) is still
> > supported when a second child requires a different rate (lvds rate). As
> 
> I still have concerns that the CLK_SET_RATE_PARENT may break the
> media_disp1_pix if media_disp2_pix is changing it.
> 
> I think we should consider adding some sort of configurable flag to
> the CCM that lets people choose  if CLK_SET_RATE_PARENT should be set
> or not in the device tree instead of hard-coding it either on or off.
> This would give people the flexibility of stating whether
> media_disp1_pix, media_disp2_pix, both or neither could set
> CLK_SET_RATE_PARENT.
> 
> I believe the imx8mp-evk can support both LVDS-> HDMI and DSI->HDMI
> bridges, and I fear that if they are trying to both set different
> clock rates, this may break something and the clocks need to be
> selected in advance to give people a bunch of HDMI options as well as
> being able to divide down to support the LVDS.
> 
> I think some of the displays could be tied to one of the Audio PLL's,
> so I might experiment with splitting the media_disp1_pix and
> media_disp2_pix from each other to see how well .
> 
> > the children have divider, the current approach is straight forward by
> > calculating the LCM of the required rates. During the rate change of the
> > PLL, it must ensure that all children still have the configured rate at
> > the end (and maybe also bypass the clock while doing so?). This is done
> > by implementing a notifier function for the clk-composite-8m. The tricky
> > part is now to find out if the rate change was intentional or not. This
> > is done by adding the "change trigger" to the notify data. In our case,
> > we now can infer if we aren't the change trigger, we need to keep the
> > existing rate after the PLL's rate change. We keep the existing rate by
> > modifying the new_rate of the clock's core, as we are quite late in an
> > already ongoing clock change process.
> > 
> > Future work:
> > The re-configuration of the PLL can definitely be improved for other use
> > cases where the children have more fancy inter-dependencies. That's one
> > of the main reasons I currently only touched the mentioned clocks.
> > Additionally, it might make sense to automatically re-parent if a
> > different possible parent suits better.
> > For the core part, I thought about extending my "unintentional change
> > check" so that the core ensures that the children keep the configured
> > rate, which might not be easy as the parent could be allowed to "round",
> > but it's not clear (at least to me yet) how much rounding is allowed. I
> > found a similar discussion posted here[1], therefore added Frank and
> > Maxime.
> > 
> > Thanks & regards,
> > Benjamin
> > 
> > [1]
> > https://lore.kernel.org/lkml/20230825-pll-mipi_keep_rate-v1-0-35bc4357073
> > 0@oltmanns.dev/
> > 
> > ---
> > 
> > Benjamin Bara (13):
> >       arm64: dts: imx8mp: lvds_bridge: use root instead of composite
> >       arm64: dts: imx8mp: re-parent IMX8MP_CLK_MEDIA_MIPI_PHY1_REF
> >       clk: implement clk_hw_set_rate()
> >       clk: print debug message if parent change is ignored
> >       clk: keep track of the trigger of an ongoing clk_set_rate
> >       clk: keep track if a clock is explicitly configured
> >       clk: detect unintended rate changes
> >       clk: divider: stop early if an optimal divider is found
> >       clk: imx: pll14xx: consider active rate for re-config
> >       clk: imx: composite-8m: convert compute_dividers to void
> >       clk: imx: composite-8m: implement CLK_SET_RATE_PARENT
> >       clk: imx: imx8mp: allow LVDS clocks to set parent rate
> >       arm64: dts: imx8mp: remove assigned-clock-rate of IMX8MP_VIDEO_PLL1
> >  
> >  arch/arm64/boot/dts/freescale/imx8mp.dtsi |  14 +--
> >  drivers/clk/clk-divider.c                 |   9 ++
> >  drivers/clk/clk.c                         | 146
> >  +++++++++++++++++++++++++++++- drivers/clk/imx/clk-composite-8m.c       
> >  |  89 +++++++++++++++--- drivers/clk/imx/clk-imx8mp.c              |   4
> >  +-
> >  drivers/clk/imx/clk-pll14xx.c             |  20 ++++
> >  drivers/clk/imx/clk.h                     |   4 +
> >  include/linux/clk-provider.h              |   2 +
> >  include/linux/clk.h                       |   2 +
> >  9 files changed, 261 insertions(+), 29 deletions(-)
> > 
> > ---
> > base-commit: e143016b56ecb0fcda5bb6026b0a25fe55274f56
> > change-id: 20230913-imx8mp-dtsi-7c6e25907e0e
> > 
> > Best regards,
> > --
> > Benjamin Bara <benjamin.bara@skidata.com>


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH 00/13] imx8mp: first clock propagation attempt (for LVDS)

[Benjamin Bara]

Hi Adam, Alexander!

On Wed, 4 Oct 2023 at 10:04, Alexander Stein <alexander.stein@ew.tq-group.com> wrote:
> Am Dienstag, 3. Oktober 2023, 15:28:05 CEST schrieb Adam Ford:
> > On Sun, Sep 17, 2023 at 5:40 PM Benjamin Bara <bbara93@gmail.com> wrote:
> > > Hi!
> > >
> > > Target of this series is to dynamically set the rate of video_pll1 to
> > > the required LVDS clock rate(s), which are configured by the panel, and
> > > the lvds-bridge respectively.
> > >
> > > Some background:
> > > The LVDS panel requires two clocks: the crtc clock and the lvds clock.
> > > The lvds rate is always 7x the crtc rate. On the imx8mp, these are
> > > assigned to media_disp2_pix and media_ldb, which are both
> >
> > Could the LDB driver be updated to take in the crtc clock as a
> > parameter, then set the media_ldb to 7x crct clock.  I wonder if that
> > might simplify the task a bit.
>
> I'm not sure if you had something different in mind, but I guess this happens
> already in fsl_ldb_atomic_enable(), although using the mode clock.
> As this might not always be possible, commit bd43a9844bc6f ("drm: bridge: ldb:
> Warn if LDB clock does not match requested link frequency") was added to
> indicate something might be wrong.
> The main problem here is that both media_ldb and crct clock are not in a
> parent<->child relationship, but are siblings, configurable individually.

Yes, this already happens. First, the mode is set (which sets the CRTC
rate). Next, LDB sets the LVDS rate. Both do not have "access" to the
PLL, because the clocks haven't configured CLK_SET_RATE_PARENT. What
might be a working (but IMHO dirty) hack, is to give the LDB the PLL
clock as input too. Then it could set the PLL, LDB, CRTC rate (CRTC rate
must be set again after PLL is set!).

> > I still have concerns that the CLK_SET_RATE_PARENT may break the
> > media_disp1_pix if media_disp2_pix is changing it.
> > I think we should consider adding some sort of configurable flag to
> > the CCM that lets people choose  if CLK_SET_RATE_PARENT should be set
> > or not in the device tree instead of hard-coding it either on or off.
> > This would give people the flexibility of stating whether
> > media_disp1_pix, media_disp2_pix, both or neither could set
> > CLK_SET_RATE_PARENT.

Probably we could do that (for now) by adding a second (optional) clock
to LDB. If it is set, the LDB driver should also set the LVDS rate on
this clock. This would then be set to the parent PLL.

> > I believe the imx8mp-evk can support both LVDS-> HDMI and DSI->HDMI
> > bridges, and I fear that if they are trying to both set different
> > clock rates, this may break something and the clocks need to be
> > selected in advance to give people a bunch of HDMI options as well as
> > being able to divide down to support the LVDS.
> >
> > I think some of the displays could be tied to one of the Audio PLL's,
> > so I might experiment with splitting the media_disp1_pix and
> > media_disp2_pix from each other to see how well .

Yes, you probably could also tie them to one of the other available
PLLs. We "could" also do that automatically, by not setting
CLK_SET_RATE_REPARENT and adapting the clk-divider driver to look for a
better suitable parent. However, I guess the outcome is currently quite
unpredictable, so this would require a lot of additional work. Just to
mention it here too: I created a small spin-off of this series[1] with
the changes of this series which affect the core.

Probably using the optional clock for LDB is a suitable short-term
solution?

Regards
Benjamin