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

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

Hi Marek,

+Cc: Anson (initial author of clk-imx8mp.c)

On Wed, 13 Nov 2024 00:14:15 +0100
Marek Vasut <marex@denx.de> wrote:

> On 11/12/24 11:42 PM, Luca Ceresoli wrote:
> > Hello Marek, Abel,  
> 
> Hi,
> 
> > +Cc: Miquèl
> > 
> > On Fri, 31 May 2024 22:26:26 +0200
> > Marek Vasut <marex@denx.de> wrote:
> >   
> >> 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.

However this is not fixing the problem I'm seeing. The existing
assigned-clock-rates value for video_pll1 used to work because it is
the media_ldb parent, and the parent wasn't recalculated. After this
patch got applied the video_pll1 rate is computed at runtime and so the
hardcoded value in assigned-clock-rates does not matter in the end.

I also tried a configuration that appears to me as the most optimal for
managing both an LVDS panel on LDB and a DSI panel (which is also
present in the more complete design I'm working on):

 * media_ldb and media_disp2 (the two clocks involved in LDB/LVDS
   output) left as children of video_pll1 as per imx8mp.dtsi
 * media_disp1 (used for DSI output) reparented to sys_pll3

The above config assigns to each output (LVDS and DSI) an ad-hoc PLL.
However the problem does not disappear, simply because the problem is
that requesting a ~500 MHz rate to video_pll1 results in it to be
configured at ~72 MHz.
  
This confirms the problem I reported appears to be an incorrect
computation of the video_pll1 rate, which in turn looks like a bug
(which as I said is exposed, not introduced, by this patch). If
setting a hardcoded value could make it work, that would look like
hiding a bug, wouldn't it?

And at least with a single-panel setup the runtime computation should
work just fine.

More generally speaking, I don't follow your approach: your patch
enables runtime computation of the video_pll1 rate, but you now suggest
to hardcode it. Am I missing something?

Luca

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