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@xxxxxxx>
>>> ---
>>> Cc: Abel Vesa <abelvesa@xxxxxxxxxx>
>>> Cc: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
>>> Cc: David Airlie <airlied@xxxxxxxxx>
>>> Cc: Fabio Estevam <festevam@xxxxxxxxx>
>>> Cc: Isaac Scott <isaac.scott@xxxxxxxxxxxxxxxx>
>>> Cc: Jernej Skrabec <jernej.skrabec@xxxxxxxxx>
>>> Cc: Jonas Karlman <jonas@xxxxxxxxx>
>>> Cc: Laurent Pinchart <Laurent.pinchart@xxxxxxxxxxxxxxxx>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
>>> Cc: Maxime Ripard <mripard@xxxxxxxxxx>
>>> Cc: Michael Turquette <mturquette@xxxxxxxxxxxx>
>>> Cc: Neil Armstrong <neil.armstrong@xxxxxxxxxx>
>>> Cc: Peng Fan <peng.fan@xxxxxxx>
>>> Cc: Pengutronix Kernel Team <kernel@xxxxxxxxxxxxxx>
>>> Cc: Robert Foss <rfoss@xxxxxxxxxx>
>>> Cc: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
>>> Cc: Shawn Guo <shawnguo@xxxxxxxxxx>
>>> Cc: Simona Vetter <simona@xxxxxxxx>
>>> Cc: Stephen Boyd <sboyd@xxxxxxxxxx>
>>> Cc: Thomas Zimmermann <tzimmermann@xxxxxxx>
>>> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
>>> Cc: imx@xxxxxxxxxxxxxxx
>>> Cc: kernel@xxxxxxxxxxxxxxxxxx
>>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>>> Cc: linux-clk@xxxxxxxxxxxxxxx
>>> ---
>>> 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.
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@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.
>
> Also, please Cc me on fsl-ldb.c patches.
Ok, will do. BTW, if MAINTAINERS is updated, then you'll always
receive fsl-ldb.c patches.
>
>> Unsupported display modes cannot be ruled out. Note that
>> "media_ldb" is derived from "video_pll1_out" by default as the
>> parent is set in imx8mp.dtsi. And, the only 4 rates supported
>> by "video_pll1" are listed in imx_pll1443x_tbl[] - 1.0395GHz,
>> 650MHz, 594MHz and 519.75MHz.
> I disagree with this part, since commit b09c68dc57c9 ("clk: imx: pll14xx: Support dynamic rates") the 1443x PLLs can be configured to arbitrary rates which for video PLL is desirable as those should produce accurate clock.
Kinda ack - that commit does open up many more clock rates.
But, the commit just says dynamic rates, not arbitrary rates or
any rate.
--
Regards,
Liu Ying
