Message-ID: <12810050.O9o76ZdvQC@steina-w>
Date:   Wed, 04 Oct 2023 10:04:41 +0200
From:   Alexander Stein <alexander.stein@...tq-group.com>
To:     Benjamin Bara <bbara93@...il.com>, Adam Ford <aford173@...il.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>,
        Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        NXP Linux Team <linux-imx@....com>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Russell King <linux@...linux.org.uk>,
        Abel Vesa <abelvesa@...nel.org>, Peng Fan <peng.fan@....com>,
        Frank Oltmanns <frank@...manns.dev>,
        Maxime Ripard <mripard@...nel.org>, devicetree@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-clk@...r.kernel.org,
        Benjamin Bara <benjamin.bara@...data.com>,
        Lucas Stach <l.stach@...gutronix.de>
Subject: Re: [PATCH 00/13] imx8mp: first clock propagation attempt (for LVDS)

Hi Adam,

Am Dienstag, 3. Oktober 2023, 15:28:05 CEST schrieb Adam Ford:
> On Sun, Sep 17, 2023 at 5:40 PM Benjamin Bara <bbara93@...il.com> 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.

Best regards,
Alexander

> > clk-composite-8m. The rates are set by drm_client_modeset_commit() (and
> > later by fsl_ldb_atomic_enable()), and the fsl-ldb driver, first crtc,
> > then lvds. The parent is typically assigned to video_pll1, which is a
> > clk-pll14xx (pll1443x).
> > 
> > The main problem:
> > As the clk-composite-8m currently doesn't support CLK_SET_RATE_PARENT,
> > the crtc rate is not propagated to video_pll1, and therefore must be
> > assigned in the device-tree manually.
> > 
> > The idea:
> > Enable CLK_SET_RATE_PARENT, at least for media_disp2_pix and media_ldb.
> > When this is done, ensure that the pll1443x can be re-configured,
> > meaning it ensures that an already configured rate (crtc rate) is still
> > supported when a second child requires a different rate (lvds rate). As
> 
> 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.
> 
> 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 .
> 
> > the children have divider, the current approach is straight forward by
> > calculating the LCM of the required rates. During the rate change of the
> > PLL, it must ensure that all children still have the configured rate at
> > the end (and maybe also bypass the clock while doing so?). This is done
> > by implementing a notifier function for the clk-composite-8m. The tricky
> > part is now to find out if the rate change was intentional or not. This
> > is done by adding the "change trigger" to the notify data. In our case,
> > we now can infer if we aren't the change trigger, we need to keep the
> > existing rate after the PLL's rate change. We keep the existing rate by
> > modifying the new_rate of the clock's core, as we are quite late in an
> > already ongoing clock change process.
> > 
> > Future work:
> > The re-configuration of the PLL can definitely be improved for other use
> > cases where the children have more fancy inter-dependencies. That's one
> > of the main reasons I currently only touched the mentioned clocks.
> > Additionally, it might make sense to automatically re-parent if a
> > different possible parent suits better.
> > For the core part, I thought about extending my "unintentional change
> > check" so that the core ensures that the children keep the configured
> > rate, which might not be easy as the parent could be allowed to "round",
> > but it's not clear (at least to me yet) how much rounding is allowed. I
> > found a similar discussion posted here[1], therefore added Frank and
> > Maxime.
> > 
> > Thanks & regards,
> > Benjamin
> > 
> > [1]
> > https://lore.kernel.org/lkml/20230825-pll-mipi_keep_rate-v1-0-35bc4357073
> > 0@...manns.dev/
> > 
> > ---
> > 
> > Benjamin Bara (13):
> >       arm64: dts: imx8mp: lvds_bridge: use root instead of composite
> >       arm64: dts: imx8mp: re-parent IMX8MP_CLK_MEDIA_MIPI_PHY1_REF
> >       clk: implement clk_hw_set_rate()
> >       clk: print debug message if parent change is ignored
> >       clk: keep track of the trigger of an ongoing clk_set_rate
> >       clk: keep track if a clock is explicitly configured
> >       clk: detect unintended rate changes
> >       clk: divider: stop early if an optimal divider is found
> >       clk: imx: pll14xx: consider active rate for re-config
> >       clk: imx: composite-8m: convert compute_dividers to void
> >       clk: imx: composite-8m: implement CLK_SET_RATE_PARENT
> >       clk: imx: imx8mp: allow LVDS clocks to set parent rate
> >       arm64: dts: imx8mp: remove assigned-clock-rate of IMX8MP_VIDEO_PLL1
> >  
> >  arch/arm64/boot/dts/freescale/imx8mp.dtsi |  14 +--
> >  drivers/clk/clk-divider.c                 |   9 ++
> >  drivers/clk/clk.c                         | 146
> >  +++++++++++++++++++++++++++++- drivers/clk/imx/clk-composite-8m.c       
> >  |  89 +++++++++++++++--- drivers/clk/imx/clk-imx8mp.c              |   4
> >  +-
> >  drivers/clk/imx/clk-pll14xx.c             |  20 ++++
> >  drivers/clk/imx/clk.h                     |   4 +
> >  include/linux/clk-provider.h              |   2 +
> >  include/linux/clk.h                       |   2 +
> >  9 files changed, 261 insertions(+), 29 deletions(-)
> > 
> > ---
> > base-commit: e143016b56ecb0fcda5bb6026b0a25fe55274f56
> > change-id: 20230913-imx8mp-dtsi-7c6e25907e0e
> > 
> > Best regards,
> > --
> > Benjamin Bara <benjamin.bara@...data.com>


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Powered by blists - more mailing lists