Message-ID: <CAHCN7x+WesF5Dac=9THtHR7Y=LJwzAEsZ9aD1B7Lppc2vQSnJQ@mail.gmail.com>
Date:   Tue, 3 Oct 2023 08:28:05 -0500
From:   Adam Ford <aford173@...il.com>
To:     Benjamin Bara <bbara93@...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)

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.

> 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-35bc43570730@oltmanns.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>
>

Powered by blists - more mailing lists