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@xxxxxxxxx> 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@xxxxxxxxxxxx/
> >
> > ---
> >
> > 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@xxxxxxxxxxx>
--
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/
