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

[Marek Vasut <marex@denx.de>]

On 11/18/24 9:15 AM, Miquel Raynal wrote:
> Hi Marek,

Hello Miquel,

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

It is not a proper fix, it is the best we can do right now. I already 
replied to Luca with a bunch of patches where I tried to come up with a 
way to negotiate the pixel clock in drivers ... I need to get back to those.

> 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?

Because I have DSI-to-(e)DP bridge on the DSI bus and I do not know the 
pixel clock needed by attached panel up front.

I already included a link to DTO which allowed me to operate both this 
DSI-to-(e)DP bridge and LVDS panel with accurate pixel clock, I was 
hoping that would also let you solve 3 and 4. 4fbb73416b10 ("arm64: dts: 
imx8mp-phyboard-pollux: Set Video PLL1 frequency to 506.8 MHz") fixed 3. 
for Isaac at least.

> 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.
I do fully agree the whole DT Video PLL1 clock frequency configuration 
is not good and it should not be in the DT at all. That is my goal in 
the very end.

The drivers (in this case, LCDIF1 + LCDIF2 + LDB) should negotiate the 
Video PLL1 frequency that fits them all best and configure it 
accordingly, without any DT assign-clock* workarounds.

I just didn't figure out a way to do that ^ yet.