Re: [PATCH 00/13] imx8mp: first clock propagation attempt (for LVDS)

[Benjamin Bara]

Hi Adam, Alexander!

On Wed, 4 Oct 2023 at 10:04, Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx> wrote:
> Am Dienstag, 3. Oktober 2023, 15:28:05 CEST schrieb Adam Ford:
> > On Sun, Sep 17, 2023 at 5:40 PM Benjamin Bara <bbara93@xxxxxxxxx> wrote:
> > > Hi!
> > >
> > > Target of this series is to dynamically set the rate of video_pll1 to
> > > the required LVDS clock rate(s), which are configured by the panel, and
> > > the lvds-bridge respectively.
> > >
> > > Some background:
> > > The LVDS panel requires two clocks: the crtc clock and the lvds clock.
> > > The lvds rate is always 7x the crtc rate. On the imx8mp, these are
> > > assigned to media_disp2_pix and media_ldb, which are both
> >
> > Could the LDB driver be updated to take in the crtc clock as a
> > parameter, then set the media_ldb to 7x crct clock.  I wonder if that
> > might simplify the task a bit.
>
> I'm not sure if you had something different in mind, but I guess this happens
> already in fsl_ldb_atomic_enable(), although using the mode clock.
> As this might not always be possible, commit bd43a9844bc6f ("drm: bridge: ldb:
> Warn if LDB clock does not match requested link frequency") was added to
> indicate something might be wrong.
> The main problem here is that both media_ldb and crct clock are not in a
> parent<->child relationship, but are siblings, configurable individually.

Yes, this already happens. First, the mode is set (which sets the CRTC
rate). Next, LDB sets the LVDS rate. Both do not have "access" to the
PLL, because the clocks haven't configured CLK_SET_RATE_PARENT. What
might be a working (but IMHO dirty) hack, is to give the LDB the PLL
clock as input too. Then it could set the PLL, LDB, CRTC rate (CRTC rate
must be set again after PLL is set!).

> > I still have concerns that the CLK_SET_RATE_PARENT may break the
> > media_disp1_pix if media_disp2_pix is changing it.
> > I think we should consider adding some sort of configurable flag to
> > the CCM that lets people choose  if CLK_SET_RATE_PARENT should be set
> > or not in the device tree instead of hard-coding it either on or off.
> > This would give people the flexibility of stating whether
> > media_disp1_pix, media_disp2_pix, both or neither could set
> > CLK_SET_RATE_PARENT.

Probably we could do that (for now) by adding a second (optional) clock
to LDB. If it is set, the LDB driver should also set the LVDS rate on
this clock. This would then be set to the parent PLL.

> > I believe the imx8mp-evk can support both LVDS-> HDMI and DSI->HDMI
> > bridges, and I fear that if they are trying to both set different
> > clock rates, this may break something and the clocks need to be
> > selected in advance to give people a bunch of HDMI options as well as
> > being able to divide down to support the LVDS.
> >
> > I think some of the displays could be tied to one of the Audio PLL's,
> > so I might experiment with splitting the media_disp1_pix and
> > media_disp2_pix from each other to see how well .

Yes, you probably could also tie them to one of the other available
PLLs. We "could" also do that automatically, by not setting
CLK_SET_RATE_REPARENT and adapting the clk-divider driver to look for a
better suitable parent. However, I guess the outcome is currently quite
unpredictable, so this would require a lot of additional work. Just to
mention it here too: I created a small spin-off of this series[1] with
the changes of this series which affect the core.

Probably using the optional clock for LDB is a suitable short-term
solution?

Regards
Benjamin