Hi Maxime,
On 10/22/2024, Maxime Ripard wrote:
> 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 <ma...@denx.de>
>>>>>>> ---
>>>>>>> Cc: Abel Vesa <abelv...@kernel.org>
>>>>>>> Cc: Andrzej Hajda <andrzej.ha...@intel.com>
>>>>>>> Cc: David Airlie <airl...@gmail.com>
>>>>>>> Cc: Fabio Estevam <feste...@gmail.com>
>>>>>>> Cc: Isaac Scott <isaac.sc...@ideasonboard.com>
>>>>>>> Cc: Jernej Skrabec <jernej.skra...@gmail.com>
>>>>>>> Cc: Jonas Karlman <jo...@kwiboo.se>
>>>>>>> Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
>>>>>>> Cc: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
>>>>>>> Cc: Maxime Ripard <mrip...@kernel.org>
>>>>>>> Cc: Michael Turquette <mturque...@baylibre.com>
>>>>>>> Cc: Neil Armstrong <neil.armstr...@linaro.org>
>>>>>>> Cc: Peng Fan <peng....@nxp.com>
>>>>>>> Cc: Pengutronix Kernel Team <ker...@pengutronix.de>
>>>>>>> Cc: Robert Foss <rf...@kernel.org>
>>>>>>> Cc: Sascha Hauer <s.ha...@pengutronix.de>
>>>>>>> Cc: Shawn Guo <shawn...@kernel.org>
>>>>>>> Cc: Simona Vetter <sim...@ffwll.ch>
>>>>>>> Cc: Stephen Boyd <sb...@kernel.org>
>>>>>>> Cc: Thomas Zimmermann <tzimmerm...@suse.de>
>>>>>>> Cc: dri-devel@lists.freedesktop.org
>>>>>>> Cc: i...@lists.linux.dev
>>>>>>> Cc: ker...@dh-electronics.com
>>>>>>> Cc: linux-arm-ker...@lists.infradead.org
>>>>>>> Cc: linux-...@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@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.
This is a bit complicated, because it is related to i.MX8MP MIPI DSI/
LVDS/HDMI, i.MX93 MIPI DSI/LVDS/parallel display pipelines. Even
i.MX6SX LVDS display pipeline is a bit related, since i.MX8MP/i.MX93/
i.MX6SX LDBs share the same fsl-ldb.c driver.
>
> 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.
Can you please suggest a place where this could happen?
>
> Maxime
--
Regards,
Liu Ying
