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

[Marek Vasut <marex@denx.de>]

On 11/19/24 4:41 PM, Miquel Raynal wrote:

Hello Miquel,

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

Whew, glad I could help.

>>> 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 :-)

Good indeed :)

>> 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.
I CCed you on an ongoing conversation with Victor in

Re: [PATCH v7 2/7] Revert "clk: imx: clk-imx8mp: Allow media_disp pixel 
clock reconfigure parent rate"

let's continue the discussion there. I hope we can reach some sort of a 
way forward there, that works for everyone.