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

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

Hi Marek,

On 18/11/2024 at 15:30:04 +01, Marek Vasut <marex@denx.de> wrote:

> 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

I am sorry I probably misunderstood your previous reply then. I am fine
with the assigned-clocks workaround.

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

Indeed, thanks to your feedback we got it fixed locally, so short term
is okay for us (but people not reading this thread might still suffer
from the problem though).

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

Ok, good to know we are aligned :-)

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

Of course, getting rid of the DT workarounds is probably a long term
goal, unlike the mid-term goal which is: "fixing" today's situation for
"everyone with a simple setup". We are also looking into this and
willing to find a proper solution.

Cheers,
Miquèl