Hi Liu,

quoted

quoted

The
pixel clock can be got from LDB's remote input LCDIF DT node by
calling of_clk_get_by_name() in fsl_ldb_probe() like the below patch
does. Similar to setting pixel clock rate, I think a chance is that
pixel clock enablement can be moved from LCDIF driver to
fsl_ldb_atomic_enable() to avoid on-the-fly division ratio change.

TBH, this sounds like a hack and is no longer required with this series.

Pixel clock is an input signal to LDB, which is reflected in LDB block
diagram in i.MX8MP TRM(see Figure 13-70) where "CLOCK" goes into LDB
along with "DATA", "HSYNC", "VSYNC", "DATA_EN" and "CONTROL".  So,
fsl,ldb.yaml should have documented the pixel clock in clocks and
clock-names properties, but unfortunately it doesn't and it's too late
to do so.  The workaround is to get the pixel clock from LCDIF DT node
in the LDB driver.  I would call it a workaround rather than a hack,
since fsl,ldb.yaml should have documented the pixel clock in the first
place.

quoted

You are just trying to circumvent the fact that until now, applying a
rate in an upper clock would unconfigure the downstream rates, and I
think this is our first real problem.

I'm still not a fan of setting CLK_SET_RATE_PARENT flag to the LDB clock
and pixel clocks.  If we look at the bigger picture, the first real
problem is probably how to support both separated video PLLs and shared
video PLL.

quoted

quoted

https://patchwork.kernel.org/project/linux-clk/patch/20241114065759.3341908-6-victor.liu@nxp.com/

Actually, one sibling patch of the above patch reverts ff06ea04e4cf
because I thought "fixed PLL rate" is the only solution, though I'm
discussing any alternative solution of "dynamically changeable PLL
rate" with Marek in the thread of the sibling patch.

I don't think we want fixed PLL rates. Especially if you start using

I don't want fixed PLL rates, either...

quoted

external (hot-pluggable) displays with different needs: it just does not

... but, considering the problem of how to support separated/shared video
PLLs, I think we have to accept the fixed PLL rates.  So, unfortunately
some video modes read from EDID cannot fly.  If there is an feasible
alternative solution, it will be good to implement it, but till now I
don't see any.

Can you please remind me what your exact display setup (and their
required pixel clocks) is?

AFAIU, you don't want to use dynamic calculations for the PLL because it
breaks your use case with HDMI. Of course this is a very limited use
case, because using a static rate means almost a single type of display
can be plugged.

The clock subsystem will not recalculate the video_pll1 if you can
achieve a perfect rate change using the LDB/PIX divisors. So let me
propose the below addition to this series. With the below diff, your
setup should still work with assigned clock rates, while letting us
handle our calculations dynamically.

The addition I am now proposing is to remove CLK_SET_RATE_PARENT to both
media_disp[12]_pix clocks. This should actually fix your situation while
keeping pixel clocks accurate as far it is possible as the LDB clock
will change video_pll1 only if the PLL rate is not suitable for it in
the first place. And then, there will be no other clock messing with
this PLL. This is probably a safer approach, which should still allow
accurate dynamic rate calculations for "simple" setups *and* let the
static assignations work otherwise:

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

Can you please give this proposal a try?

[...]

quoted

--- a/drivers/gpu/drm/bridge/fsl-ldb.c
+++ b/drivers/gpu/drm/bridge/fsl-ldb.c

@@ -177,6 +177,17 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
        mode = &crtc_state->adjusted_mode;
 
        requested_link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock);
+       /*
+        * Dual cases require a 3.5 rate division on the pixel clocks, which
+        * cannot be achieved with regular single divisors. Instead, double the
+        * parent PLL rate in the first place. In order to do that, we first
+        * require twice the target clock rate, which will program the upper
+        * PLL. Then, we ask for the actual targeted rate, which can be achieved
+        * by dividing by 2 the already configured upper PLL rate, without
+        * making further changes to it.
+        */
+       if (fsl_ldb_is_dual(fsl_ldb))
+               clk_set_rate(fsl_ldb->clk, requested_link_freq * 2);

I don't think i.MX6SX LDB needs this, because the "ldb" clock's parent
is a mux clock with "ldb_di0_div_3_5" or "ldb_di0_div_7" parents.

Ah, you mean we should only do this in the imx8 case, right?

Thanks,
Miquèl