[PATCH 1/2] clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate
Maxime Ripard
mripard at kernel.org
Tue Oct 22 07:50:24 UTC 2024
On Tue, Oct 22, 2024 at 02:13:57PM +0800, Liu Ying wrote:
> On 10/13/2024, Marek Vasut wrote:
> > On 10/11/24 8:18 AM, Liu Ying wrote:
> >> On 10/11/2024, Marek Vasut wrote:
> >>> On 10/10/24 7:22 AM, Liu Ying wrote:
> >>>> On 10/09/2024, Marek Vasut wrote:
> >>>>> The media_ldb_root_clk supply LDB serializer. These clock are usually
> >>>>> shared with the LCDIFv3 pixel clock and supplied by the Video PLL on
> >>>>> i.MX8MP, but the LDB clock run at either x7 or x14 rate of the LCDIFv3
> >>>>> pixel clock. Allow the LDB to reconfigure Video PLL as needed, as that
> >>>>> results in accurate serializer clock.
> >>>>>
> >>>>> Signed-off-by: Marek Vasut <marex at denx.de>
> >>>>> ---
> >>>>> Cc: Abel Vesa <abelvesa at kernel.org>
> >>>>> Cc: Andrzej Hajda <andrzej.hajda at intel.com>
> >>>>> Cc: David Airlie <airlied at gmail.com>
> >>>>> Cc: Fabio Estevam <festevam at gmail.com>
> >>>>> Cc: Isaac Scott <isaac.scott at ideasonboard.com>
> >>>>> Cc: Jernej Skrabec <jernej.skrabec at gmail.com>
> >>>>> Cc: Jonas Karlman <jonas at kwiboo.se>
> >>>>> Cc: Laurent Pinchart <Laurent.pinchart at ideasonboard.com>
> >>>>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> >>>>> Cc: Maxime Ripard <mripard at kernel.org>
> >>>>> Cc: Michael Turquette <mturquette at baylibre.com>
> >>>>> Cc: Neil Armstrong <neil.armstrong at linaro.org>
> >>>>> Cc: Peng Fan <peng.fan at nxp.com>
> >>>>> Cc: Pengutronix Kernel Team <kernel at pengutronix.de>
> >>>>> Cc: Robert Foss <rfoss at kernel.org>
> >>>>> Cc: Sascha Hauer <s.hauer at pengutronix.de>
> >>>>> Cc: Shawn Guo <shawnguo at kernel.org>
> >>>>> Cc: Simona Vetter <simona at ffwll.ch>
> >>>>> Cc: Stephen Boyd <sboyd at kernel.org>
> >>>>> Cc: Thomas Zimmermann <tzimmermann at suse.de>
> >>>>> Cc: dri-devel at lists.freedesktop.org
> >>>>> Cc: imx at lists.linux.dev
> >>>>> Cc: kernel at dh-electronics.com
> >>>>> Cc: linux-arm-kernel at lists.infradead.org
> >>>>> Cc: linux-clk at vger.kernel.org
> >>>>> ---
> >>>>> drivers/clk/imx/clk-imx8mp.c | 2 +-
> >>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
> >>>>> index 516dbd170c8a3..2e61d340b8ab7 100644
> >>>>> --- a/drivers/clk/imx/clk-imx8mp.c
> >>>>> +++ b/drivers/clk/imx/clk-imx8mp.c
> >>>>> @@ -611,7 +611,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
> >>>>> hws[IMX8MP_CLK_MEDIA_MIPI_PHY1_REF] = imx8m_clk_hw_composite("media_mipi_phy1_ref", imx8mp_media_mipi_phy1_ref_sels, ccm_base + 0xbd80);
> >>>>> hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT);
> >>>>> hws[IMX8MP_CLK_MEDIA_CAM2_PIX] = imx8m_clk_hw_composite("media_cam2_pix", imx8mp_media_cam2_pix_sels, ccm_base + 0xbe80);
> >>>>> - hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00);
> >>>>> + hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite_bus_flags("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00, CLK_SET_RATE_PARENT);
> >>>>
> >>>> This patch would cause the below in-flight LDB bridge driver
> >>>> patch[1] fail to do display mode validation upon display modes
> >>>> read from LVDS to HDMI converter IT6263's DDC I2C bus.
> >>>
> >>> Why ?
> >>
> >> Mode validation is affected only for dual LVDS link mode.
> >> For single LVDS link mode, this patch does open more display
> >> modes read from the DDC I2C bus. The reason behind is that
> >> LVDS serial clock rate/pixel clock rate = 3.5 for dual LVDS
> >> link mode, while it's 7 for single LVDS link mode.
> >>
> >> In my system, "video_pll1" clock rate is assigned to 1.0395GHz
> >> in imx8mp.dtsi. For 1920x1080-60.00Hz with 148.5MHz pixel
> >> clock rate, "media_ldb" clock rate is 519.75MHz and
> >> "media_disp2_pix" clock rate is 148.5MHz, which is fine for
> >> dual LVDS link mode(x3.5). For newly opened up 1920x1080-59.94Hz
> >> with 148.352MHz pixel clock rate, "video_pll1" clock rate will
> >> be changed to 519.232MHz, "media_ldb" clock rate is 519.232MHz
> >> and "media_disp2_pix" clock rate is wrongly set to 519.232MHz
> >> too because "media_disp2_pix" clock cannot handle the 3.5
> >> division ratio from "video_pll1_out" clock running at
> >> 519.232MHz. See the below clk_summary.
> >
> > Shouldn't this patch help exactly with that ?
>
> No, it doesn't help but only makes clk_round_rate() called in
> LDB driver's .mode_valid() against 148.352MHz return 148.352MHz
> which allows the unexpected 1920x1080-59.94Hz display mode.
>
> >
> > It should allow you to set video_pll1_out to whatever is necessary by LDB first, fixate that frequency, and the LCDIFv3 would then be forced to use /7 divider from faster Video PLL1 , right ?
>
> Yes, it allows that for single-link LVDS use cases.
> And, __no__, for dual-link LVDS use cases because the
> video_pll1_out clock rate needs to be 2x the LVDS serial clock
> rate.
>
> >
> >> video_pll1_ref_sel 1 1 0 24000000 0 0 50000 Y deviceless no_connection_id
> >> video_pll1 1 1 0 519232000 0 0 50000 Y deviceless no_connection_id
> >> video_pll1_bypass 1 1 0 519232000 0 0 50000 Y deviceless no_connection_id
> >> video_pll1_out 2 2 0 519232000 0 0 50000 Y deviceless no_connection_id
> >> media_ldb 1 1 0 519232000 0 0 50000 Y deviceless no_connection_id
> >> media_ldb_root_clk 1 1 0 519232000 0 0 50000 Y 32ec0000.blk-ctrl:bridge at 5c ldb
> >> deviceless no_connection_id
> >> media_disp1_pix 0 0 0 129808000 0 0 50000 N deviceless no_connection_id
> >> media_disp1_pix_root_clk 0 0 0 129808000 0 0 50000 N 32e80000.display-controller pix
> >> 32ec0000.blk-ctrl disp1
> >> deviceless no_connection_id
> >> media_disp2_pix 1 1 0 519232000 0 0 50000 Y deviceless no_connection_id
> >> media_disp2_pix_root_clk 1 1 0 519232000 0 0 50000 Y 32e90000.display-controller pix
> >> 32ec0000.blk-ctrl disp2
> >> deviceless no_connection_id
> >>
> >> Single LVDS link mode is not affected because "media_disp2_pix"
> >> clock can handle the 7 division ratio.
> >>
> >> To support the dual LVDS link mode, "video_pll1" clock rate needs
> >> to be x2 "media_ldb" clock rate so that "media_disp2_pix" clock
> >> can use 7 division ratio to achieve the /3.5 clock rate comparing
> >> to "media_ldb" clock rate. However, "video_pll1" is not seen by
> >> LDB driver thus not directly controlled by it. This is another
> >> reason why assigning a reasonable "video_pll1" clock rate in DT
> >> makes sense.
> >
> > I agree that _right_now_, the DT clock assignment is the only option.
> > I would like to see that DT part disappear and instead would prefer if the LDB/LCDIF could figure out the clock tree configuration themselves.
>
> I think we'll live with the assigned clock rate in DT, because the
> i.MX8MP LDB and Samsung MIPI DSI display pipelines need to share a
> video PLL, like I mentioned in comments for patch 2.
Guys. There's 4 different discussions that look to be on the same topic,
and doing workarounds in the DT, DRM driver and clock driver for
something that looks like a broken clock.
Could we have *somewhere* a proper description of what the problem is
exactly, so we can review it? Because at the moment, it's certainly not
helping.
Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 273 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20241022/50222b20/attachment.sig>
More information about the dri-devel
mailing list