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

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Marek,

>>> 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.
>
> Right, which is solved by configuring the Video PLL to the correct
> frequency in DT up front ... unless you have more than one output
> supplied by that Video PLL.

No, this looks like a bug in the imx8 clock driver. I would expect the
core to handle such case without DT hack. It is not okay to fix clock
frequencies in DT because drivers are failing to do it properly. I
understand there are advanced/dual cases with very specific frequencies
where you don't expect it to magically work and giving hints with DT
assigned-clocks* properties makes sense, but here I don't think we
should consider it as a proper fix.

If I may recap:
1- a simple display pipeline works
2- the pixel frequency could be more precise so the video_pll1 parent is
   used to dynamically compute a better frequency
3- the video_pll1 parent is too low in some cases which breaks the
   pipeline
4- we need to force video_pll1 to a value in DT

How possibly 4 could be a relevant answer to 2, seriously? May I return
you the advice, if you want a better video_pll1 value in the first
place, why not assigning it up front in DT?

I understand your goal, and I agree with it, but please acknowledge that
even though the current patch looks fine per-se, it is exposing a real
bug that is now visible. Hiding it with DT properties feels really wrong.

Thanks,
Miquèl