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

Message

Benjamin Bara Sept. 17, 2023, 10:39 p.m. UTC

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/[e-mail]/

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

Comments

Adam Ford Sept. 18, 2023, 5 a.m. UTC | #1

On Sun, Sep 17, 2023 at 3:40 PM Benjamin Bara <[e-mail]> 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/[e-mail]/
>
> ---
> 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 <[e-mail]>
>

Frank Oltmanns Sept. 18, 2023, 5:24 p.m. UTC | #2

Hi Benjamin!

On 2023-09-18 at 00:39:56 +0200, Benjamin Bara <[e-mail]> 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/[e-mail]/
>
> ---
> 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 Sept. 18, 2023, 5:59 p.m. UTC | #3

Hi Adam!

On Mon, 18 Sept 2023 at 07:00, Adam Ford <[e-mail]> wrote:
> On Sun, Sep 17, 2023 at 3:40 PM Benjamin Bara <[e-mail]> 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

Benjamin Bara Sept. 18, 2023, 6:05 p.m. UTC | #4

Hi Frank!

On Mon, 18 Sept 2023 at 19:24, Frank Oltmanns <[e-mail]> wrote:
> On 2023-09-18 at 00:39:56 +0200, Benjamin Bara <[e-mail]> 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

Maxime Ripard Sept. 19, 2023, 7:31 a.m. UTC | #5

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

Maxime Ripard Sept. 19, 2023, 7:37 a.m. UTC | #6

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 <[e-mail]> wrote:
> > On Sun, Sep 17, 2023 at 3:40 PM Benjamin Bara <[e-mail]> 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

Maxime Ripard Sept. 19, 2023, 7:39 a.m. UTC | #7

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 <[e-mail]> wrote:
> > On 2023-09-18 at 00:39:56 +0200, Benjamin Bara <[e-mail]> 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

Adam Ford Oct. 3, 2023, 1:28 p.m. UTC | #8

On Sun, Sep 17, 2023 at 5:40 PM Benjamin Bara <[e-mail]> 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/[e-mail]/
>
> ---
> 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 <[e-mail]>
>

Alexander Stein Oct. 4, 2023, 8:04 a.m. UTC | #9

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 <[e-mail]> 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
> > [e-mail]/
> > 
> > ---
> > 
> > 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 <[e-mail]>

Benjamin Bara Oct. 4, 2023, 8:28 a.m. UTC | #10

Hi Adam, Alexander!

On Wed, 4 Oct 2023 at 10:04, Alexander Stein <[e-mail]> wrote:
> Am Dienstag, 3. Oktober 2023, 15:28:05 CEST schrieb Adam Ford:
> > On Sun, Sep 17, 2023 at 5:40 PM Benjamin Bara <[e-mail]> 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