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

[Luca Ceresoli <luca.ceresoli@bootlin.com>]

Hi Marek,

just a quick feedback about my latest discoveries.

On Wed, 13 Nov 2024 22:19:02 +0100
Marek Vasut <marex@denx.de> wrote:

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

I found that I'm having the same issue that has been discussed in some
related threads: the lcdif2 configures the video_pll1 to ~72 MHz, and
later LDB tries to set it to 7x that value, failing.

I would have guessed your "[PATCH 1/2] clk: imx: clk-imx8mp: Allow LDB
serializer clock reconfigure parent rate" would have fixed it, and it
actually allwos the LDB to set a proper (7x) rate for video_pll1, but
then also the media_disp2 goes to the same rate. Apparently the
video_pll1 is not propagated to media_disp2. I haven't dug into this.

So this is not the bug I had suspected about video_pll1 rate
computation.

For now my workaround is to set the exact rate to video_pll1 via
assigned-clock-rates.

However this breaks the case of using both lcdif1 and lcdif2. For that
I have added a reparenting of media_disp1 to sys_pll3 and it looks like
working.

Would you mind keeping Miquèl and me in Cc in following discussions
about the imx8mp video clocks? We are also facing this topic and we are
happy to contribute.

Luca

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com