[PATCH] clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate

[Marek Vasut]

On 11/13/24 12:06 PM, Luca Ceresoli wrote:
> Hi Marek,

Hi,

>>>> The media_disp[12]_pix clock supply LCDIFv3 pixel clock output. These
>>>> clocks are usually the only downstream clock from Video PLL on i.MX8MP.
>>>> Allow these clocks to reconfigure the Video PLL, as that results in
>>>> accurate pixel clock. If the Video PLL is not reconfigured, the pixel
>>>> clock accuracy is low.
>>>>
>>>> Signed-off-by: Marek Vasut <marex at denx.de>
>>>
>>> I'm afraid I just found this patch broke my previously working setup
>>> with a panel connected on the LDB.
>> Do you need a fix similar to this one?
>>
>> 4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1
>> frequency to 506.8 MHz")
> 
> So, 4fbb73416b10 is adding an assigned-clock-rates to hardcode rates,
> especially the video_pll1 rate.

Nope.

See arch/arm64/boot/dts/freescale/imx8mp.dtsi

1891                         media_blk_ctrl: blk-ctrl at 32ec0000 {
...
1951                                 assigned-clock-rates = <500000000>, 
<200000000>,
1952                                                        <0>, <0>, 
<500000000>,
1953                                                        <1039500000>;

That imx8mp.dtsi preconfigures the Video PLL1 to some random clock 
frequency.

Commit 4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1 
frequency to 506.8 MHz") configures the Video PLL1 to a frequency from 
which both your panel pixel clock and LDB serializer clock can be 
successfully divided down.

Which panel do you use ?

Try this -- Revert this patch, check /sys/kernel/debug/clk/clk_summary 
and compare it with (I assume) panel-simple.c entry for the panel you 
use, and notice the disp_pix clock are likely a bit off. That's because 
the lcdif driver did its best to divide those pixel clock down from 
1039500000 set in imx8mp.dtsi .

If you really want accurate pixel clock for your panel, you need similar 
change to 4fbb73416b10 and configure the Video PLL such that the 
accurate pixel clock can be derived from it. The Video PLL cannot be set 
to pixel clock, because the LDB serializer clock are either 7x the pixel 
clock, or 3.5x the pixel clock (for dual link LVDS), so the Video PLL 
has to be set to 7x or 3.5x pixel clock of the panel, then you should 
get accurate pixel clock and a working panel again.

> However this is not fixing the problem I'm seeing. The existing
> assigned-clock-rates value for video_pll1 used to work because it is
> the media_ldb parent, and the parent wasn't recalculated. After this
> patch got applied the video_pll1 rate is computed at runtime and so the
> hardcoded value in assigned-clock-rates does not matter in the end.
> 
> I also tried a configuration that appears to me as the most optimal for
> managing both an LVDS panel on LDB and a DSI panel (which is also
> present in the more complete design I'm working on):
> 
>   * media_ldb and media_disp2 (the two clocks involved in LDB/LVDS
>     output) left as children of video_pll1 as per imx8mp.dtsi
>   * media_disp1 (used for DSI output) reparented to sys_pll3

I noticed that the PLL3 PLL1416x does not support dynamic rate 
configuration, I sent a patch for that a few days ago, but in the end I 
also found out that the PLL1416x is less accurate and cannot supply the 
LVDS side, so I gave up on that one.

I ended up using Audio PLL for one of the displays in similar setup 
(LVDS panel + DP monitor via DSI-to-DP bridge) , see this:

https://github.com/dh-electronics/meta-dhsom-imx-bsp/commit/fb3b1521bbe71b7ff91fe92be2e64a75104eb7d5

That way, I do get accurate pixel clock on both outputs, but I do have 
to sacrifice one of the Audio PLLs. On the other hand, if all you have 
is one audio output, the Audio PLL is PLL1443x and does support dynamic 
rate configuration, so one Audio PLL should be enough for such a use 
case. Also, upside is, PLL3 can supply the Audio DSP if needed.

> The above config assigns to each output (LVDS and DSI) an ad-hoc PLL.
> However the problem does not disappear, simply because the problem is
> that requesting a ~500 MHz rate to video_pll1 results in it to be
> configured at ~72 MHz.
>    
> This confirms the problem I reported appears to be an incorrect
> computation of the video_pll1 rate, which in turn looks like a bug
> (which as I said is exposed, not introduced, by this patch). If
> setting a hardcoded value could make it work, that would look like
> hiding a bug, wouldn't it?

I missed the part where you use both LVDS and DSI outputs , which both 
hang from the Video PLL1. I think that is what triggers the problem, and 
I think the link above might actually help with that.

> And at least with a single-panel setup the runtime computation should
> work just fine.
> 
> More generally speaking, I don't follow your approach: your patch
> enables runtime computation of the video_pll1 rate, but you now suggest
> to hardcode it. Am I missing something?
The patch I linked sets up the Video PLL1 frequency. When either LDB or 
LCDIF attempts to divide down the serializer or pixel clock from that 
Video PLL1, they can do so by setting a divider, without changing the 
Video PLL1 frequency. That's the crutch here.

I am looking for some better way to deal with it, see
[PATCH 1/2] clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure 
parent rate
[PATCH 2/2] drm: bridge: ldb: Configure LDB clock in .mode_set
and discussion with Isaac around
Re: [PATCH] drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock