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. > 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@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> >
