Re: [PATCH 1/2] clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure parent rate

[Liu Ying <victor.liu@nxp.com>]

On 11/18/2024, Maxime Ripard wrote:
> On Thu, Oct 31, 2024 at 10:35:27AM +0800, Liu Ying wrote:
>> 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 <marex@denx.de>
>>>>>>>>> ---
>>>>>>>>> Cc: Abel Vesa <abelvesa@kernel.org>
>>>>>>>>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>>>>>>>>> Cc: David Airlie <airlied@gmail.com>
>>>>>>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>>>>>>> Cc: Isaac Scott <isaac.scott@ideasonboard.com>
>>>>>>>>> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
>>>>>>>>> Cc: Jonas Karlman <jonas@kwiboo.se>
>>>>>>>>> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
>>>>>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>>>>> Cc: Maxime Ripard <mripard@kernel.org>
>>>>>>>>> Cc: Michael Turquette <mturquette@baylibre.com>
>>>>>>>>> Cc: Neil Armstrong <neil.armstrong@linaro.org>
>>>>>>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>>>>>>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
>>>>>>>>> Cc: Robert Foss <rfoss@kernel.org>
>>>>>>>>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>>>>>>>>> Cc: Shawn Guo <shawnguo@kernel.org>
>>>>>>>>> Cc: Simona Vetter <simona@ffwll.ch>
>>>>>>>>> Cc: Stephen Boyd <sboyd@kernel.org>
>>>>>>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>>>>> Cc: dri-devel@lists.freedesktop.org
>>>>>>>>> Cc: imx@lists.linux.dev
>>>>>>>>> Cc: kernel@dh-electronics.com
>>>>>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>>>>>> Cc: linux-clk@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?
> 
> Here, by mail will be good. Worst case scenario using a ascii art.

I have written a description in the cover letter of this patch series(v7):

https://patchwork.freedesktop.org/series/139266/#rev7

If you don't mind, please provide review comments there, thanks.

> 
> Maxime

-- 
Regards,
Liu Ying