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@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.
>
> 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@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.
>
>>> 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.
>
> Thanks
>
>>>> 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.
> I am fine with that.
--
Regards,
Liu Ying
