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

Commit Message

Marek Vasut Oct. 8, 2024, 10:38 p.m. UTC

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 <[e-mail]>
---
Cc: Abel Vesa <[e-mail]>
Cc: Andrzej Hajda <[e-mail]>
Cc: David Airlie <[e-mail]>
Cc: Fabio Estevam <[e-mail]>
Cc: Isaac Scott <[e-mail]>
Cc: Jernej Skrabec <[e-mail]>
Cc: Jonas Karlman <[e-mail]>
Cc: Laurent Pinchart <[e-mail]>
Cc: Maarten Lankhorst <[e-mail]>
Cc: Maxime Ripard <[e-mail]>
Cc: Michael Turquette <[e-mail]>
Cc: Neil Armstrong <[e-mail]>
Cc: Peng Fan <[e-mail]>
Cc: Pengutronix Kernel Team <[e-mail]>
Cc: Robert Foss <[e-mail]>
Cc: Sascha Hauer <[e-mail]>
Cc: Shawn Guo <[e-mail]>
Cc: Simona Vetter <[e-mail]>
Cc: Stephen Boyd <[e-mail]>
Cc: Thomas Zimmermann <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
---
 drivers/clk/imx/clk-imx8mp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Abel Vesa Oct. 9, 2024, 11:40 a.m. UTC | #1

On 24-10-09 00:38:19, 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 <[e-mail]>

Any fixes tag needed ?

Otherwise, LGTM:

Reviewed-by: Abel Vesa <[e-mail]>

> ---
> Cc: Abel Vesa <[e-mail]>
> Cc: Andrzej Hajda <[e-mail]>
> Cc: David Airlie <[e-mail]>
> Cc: Fabio Estevam <[e-mail]>
> Cc: Isaac Scott <[e-mail]>
> Cc: Jernej Skrabec <[e-mail]>
> Cc: Jonas Karlman <[e-mail]>
> Cc: Laurent Pinchart <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Maxime Ripard <[e-mail]>
> Cc: Michael Turquette <[e-mail]>
> Cc: Neil Armstrong <[e-mail]>
> Cc: Peng Fan <[e-mail]>
> Cc: Pengutronix Kernel Team <[e-mail]>
> Cc: Robert Foss <[e-mail]>
> Cc: Sascha Hauer <[e-mail]>
> Cc: Shawn Guo <[e-mail]>
> Cc: Simona Vetter <[e-mail]>
> Cc: Stephen Boyd <[e-mail]>
> Cc: Thomas Zimmermann <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  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);
>  	hws[IMX8MP_CLK_MEMREPAIR] = imx8m_clk_hw_composite_critical("mem_repair", imx8mp_memrepair_sels, ccm_base + 0xbf80);
>  	hws[IMX8MP_CLK_MEDIA_MIPI_TEST_BYTE] = imx8m_clk_hw_composite("media_mipi_test_byte", imx8mp_media_mipi_test_byte_sels, ccm_base + 0xc100);
>  	hws[IMX8MP_CLK_ECSPI3] = imx8m_clk_hw_composite("ecspi3", imx8mp_ecspi3_sels, ccm_base + 0xc180);
> -- 
> 2.45.2
>

Marek Vasut Oct. 9, 2024, 3:43 p.m. UTC | #2

On 10/9/24 1:40 PM, Abel Vesa wrote:
> On 24-10-09 00:38:19, 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 <[e-mail]>
> 
> Any fixes tag needed ?
I now replied to 2/2 , I think this is feature development and should be 
applied to 6.13 cycle only. I would like to get input from Isaac on 
whether the DT fix I suggested to them in the original bug report also 
works, and if so, that should possibly be the Fixes: patch for 6.12 .

Liu Ying Oct. 10, 2024, 5:22 a.m. UTC | #3

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 <[e-mail]>
> ---
> Cc: Abel Vesa <[e-mail]>
> Cc: Andrzej Hajda <[e-mail]>
> Cc: David Airlie <[e-mail]>
> Cc: Fabio Estevam <[e-mail]>
> Cc: Isaac Scott <[e-mail]>
> Cc: Jernej Skrabec <[e-mail]>
> Cc: Jonas Karlman <[e-mail]>
> Cc: Laurent Pinchart <[e-mail]>
> Cc: Maarten Lankhorst <[e-mail]>
> Cc: Maxime Ripard <[e-mail]>
> Cc: Michael Turquette <[e-mail]>
> Cc: Neil Armstrong <[e-mail]>
> Cc: Peng Fan <[e-mail]>
> Cc: Pengutronix Kernel Team <[e-mail]>
> Cc: Robert Foss <[e-mail]>
> Cc: Sascha Hauer <[e-mail]>
> Cc: Shawn Guo <[e-mail]>
> Cc: Simona Vetter <[e-mail]>
> Cc: Stephen Boyd <[e-mail]>
> Cc: Thomas Zimmermann <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  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.
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.

[1] https://patchwork.freedesktop.org/patch/616907/?series=139266&rev=1

>  	hws[IMX8MP_CLK_MEMREPAIR] = imx8m_clk_hw_composite_critical("mem_repair", imx8mp_memrepair_sels, ccm_base + 0xbf80);
>  	hws[IMX8MP_CLK_MEDIA_MIPI_TEST_BYTE] = imx8m_clk_hw_composite("media_mipi_test_byte", imx8mp_media_mipi_test_byte_sels, ccm_base + 0xc100);
>  	hws[IMX8MP_CLK_ECSPI3] = imx8m_clk_hw_composite("ecspi3", imx8mp_ecspi3_sels, ccm_base + 0xc180);

Marek Vasut Oct. 11, 2024, 1:55 a.m. UTC | #4

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 <[e-mail]>
>> ---
>> Cc: Abel Vesa <[e-mail]>
>> Cc: Andrzej Hajda <[e-mail]>
>> Cc: David Airlie <[e-mail]>
>> Cc: Fabio Estevam <[e-mail]>
>> Cc: Isaac Scott <[e-mail]>
>> Cc: Jernej Skrabec <[e-mail]>
>> Cc: Jonas Karlman <[e-mail]>
>> Cc: Laurent Pinchart <[e-mail]>
>> Cc: Maarten Lankhorst <[e-mail]>
>> Cc: Maxime Ripard <[e-mail]>
>> Cc: Michael Turquette <[e-mail]>
>> Cc: Neil Armstrong <[e-mail]>
>> Cc: Peng Fan <[e-mail]>
>> Cc: Pengutronix Kernel Team <[e-mail]>
>> Cc: Robert Foss <[e-mail]>
>> Cc: Sascha Hauer <[e-mail]>
>> Cc: Shawn Guo <[e-mail]>
>> Cc: Simona Vetter <[e-mail]>
>> Cc: Stephen Boyd <[e-mail]>
>> Cc: Thomas Zimmermann <[e-mail]>
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> Cc: [e-mail]
>> ---
>>   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 ?

Also, please Cc me on 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.

Liu Ying Oct. 11, 2024, 6:18 a.m. UTC | #5

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 <[e-mail]>
>>> ---
>>> Cc: Abel Vesa <[e-mail]>
>>> Cc: Andrzej Hajda <[e-mail]>
>>> Cc: David Airlie <[e-mail]>
>>> Cc: Fabio Estevam <[e-mail]>
>>> Cc: Isaac Scott <[e-mail]>
>>> Cc: Jernej Skrabec <[e-mail]>
>>> Cc: Jonas Karlman <[e-mail]>
>>> Cc: Laurent Pinchart <[e-mail]>
>>> Cc: Maarten Lankhorst <[e-mail]>
>>> Cc: Maxime Ripard <[e-mail]>
>>> Cc: Michael Turquette <[e-mail]>
>>> Cc: Neil Armstrong <[e-mail]>
>>> Cc: Peng Fan <[e-mail]>
>>> Cc: Pengutronix Kernel Team <[e-mail]>
>>> Cc: Robert Foss <[e-mail]>
>>> Cc: Sascha Hauer <[e-mail]>
>>> Cc: Shawn Guo <[e-mail]>
>>> Cc: Simona Vetter <[e-mail]>
>>> Cc: Stephen Boyd <[e-mail]>
>>> Cc: Thomas Zimmermann <[e-mail]>
>>> Cc: [e-mail]
>>> Cc: [e-mail]
>>> Cc: [e-mail]
>>> Cc: [e-mail]
>>> Cc: [e-mail]
>>> ---
>>>   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.

Marek Vasut Oct. 12, 2024, 9:07 p.m. UTC | #6

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 <[e-mail]>
>>>> ---
>>>> Cc: Abel Vesa <[e-mail]>
>>>> Cc: Andrzej Hajda <[e-mail]>
>>>> Cc: David Airlie <[e-mail]>
>>>> Cc: Fabio Estevam <[e-mail]>
>>>> Cc: Isaac Scott <[e-mail]>
>>>> Cc: Jernej Skrabec <[e-mail]>
>>>> Cc: Jonas Karlman <[e-mail]>
>>>> Cc: Laurent Pinchart <[e-mail]>
>>>> Cc: Maarten Lankhorst <[e-mail]>
>>>> Cc: Maxime Ripard <[e-mail]>
>>>> Cc: Michael Turquette <[e-mail]>
>>>> Cc: Neil Armstrong <[e-mail]>
>>>> Cc: Peng Fan <[e-mail]>
>>>> Cc: Pengutronix Kernel Team <[e-mail]>
>>>> Cc: Robert Foss <[e-mail]>
>>>> Cc: Sascha Hauer <[e-mail]>
>>>> Cc: Shawn Guo <[e-mail]>
>>>> Cc: Simona Vetter <[e-mail]>
>>>> Cc: Stephen Boyd <[e-mail]>
>>>> Cc: Thomas Zimmermann <[e-mail]>
>>>> Cc: [e-mail]
>>>> Cc: [e-mail]
>>>> Cc: [e-mail]
>>>> Cc: [e-mail]
>>>> Cc: [e-mail]
>>>> ---
>>>>    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 ?

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 ?

>      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.

>> 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.

Liu Ying Oct. 22, 2024, 6:13 a.m. UTC | #7

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 <[e-mail]>
>>>>> ---
>>>>> Cc: Abel Vesa <[e-mail]>
>>>>> Cc: Andrzej Hajda <[e-mail]>
>>>>> Cc: David Airlie <[e-mail]>
>>>>> Cc: Fabio Estevam <[e-mail]>
>>>>> Cc: Isaac Scott <[e-mail]>
>>>>> Cc: Jernej Skrabec <[e-mail]>
>>>>> Cc: Jonas Karlman <[e-mail]>
>>>>> Cc: Laurent Pinchart <[e-mail]>
>>>>> Cc: Maarten Lankhorst <[e-mail]>
>>>>> Cc: Maxime Ripard <[e-mail]>
>>>>> Cc: Michael Turquette <[e-mail]>
>>>>> Cc: Neil Armstrong <[e-mail]>
>>>>> Cc: Peng Fan <[e-mail]>
>>>>> Cc: Pengutronix Kernel Team <[e-mail]>
>>>>> Cc: Robert Foss <[e-mail]>
>>>>> Cc: Sascha Hauer <[e-mail]>
>>>>> Cc: Shawn Guo <[e-mail]>
>>>>> Cc: Simona Vetter <[e-mail]>
>>>>> Cc: Stephen Boyd <[e-mail]>
>>>>> Cc: Thomas Zimmermann <[e-mail]>
>>>>> Cc: [e-mail]
>>>>> Cc: [e-mail]
>>>>> Cc: [e-mail]
>>>>> Cc: [e-mail]
>>>>> Cc: [e-mail]
>>>>> ---
>>>>>    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.

Maxime Ripard Oct. 22, 2024, 7:50 a.m. UTC | #8

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 <[e-mail]>
> >>>>> ---
> >>>>> Cc: Abel Vesa <[e-mail]>
> >>>>> Cc: Andrzej Hajda <[e-mail]>
> >>>>> Cc: David Airlie <[e-mail]>
> >>>>> Cc: Fabio Estevam <[e-mail]>
> >>>>> Cc: Isaac Scott <[e-mail]>
> >>>>> Cc: Jernej Skrabec <[e-mail]>
> >>>>> Cc: Jonas Karlman <[e-mail]>
> >>>>> Cc: Laurent Pinchart <[e-mail]>
> >>>>> Cc: Maarten Lankhorst <[e-mail]>
> >>>>> Cc: Maxime Ripard <[e-mail]>
> >>>>> Cc: Michael Turquette <[e-mail]>
> >>>>> Cc: Neil Armstrong <[e-mail]>
> >>>>> Cc: Peng Fan <[e-mail]>
> >>>>> Cc: Pengutronix Kernel Team <[e-mail]>
> >>>>> Cc: Robert Foss <[e-mail]>
> >>>>> Cc: Sascha Hauer <[e-mail]>
> >>>>> Cc: Shawn Guo <[e-mail]>
> >>>>> Cc: Simona Vetter <[e-mail]>
> >>>>> Cc: Stephen Boyd <[e-mail]>
> >>>>> Cc: Thomas Zimmermann <[e-mail]>
> >>>>> Cc: [e-mail]
> >>>>> Cc: [e-mail]
> >>>>> Cc: [e-mail]
> >>>>> Cc: [e-mail]
> >>>>> Cc: [e-mail]
> >>>>> ---
> >>>>>    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.

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

Marek Vasut Oct. 23, 2024, 12:50 a.m. UTC | #9

On 10/22/24 8:13 AM, Liu Ying wrote:

[...]

>>>>> 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.

Why is 1920x1080-59.94Hz mode unexpected in the first place ?
I assume your display device reports that it supports this mode, and now 
the scanout engine and LDB can generate this mode too ? Or does the 
display device NOT support this 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.

Can't the LDB still set the Video PLL frequency to whatever it needs 
first, fixate it, and only then let the LCDIFv3 divide the frequency 
down ? (sorry, I am a bit tired today, maybe I am missing the obvious)

>>>       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.

They do NOT need to share a PLL, you can use e.g. PLL3 for one and Video 
PLL for the other if the requirement is accurate pixel clock .

[...]

Liu Ying Oct. 23, 2024, 5:25 a.m. UTC | #10

On 10/23/2024, Marek Vasut wrote:
> On 10/22/24 8:13 AM, Liu Ying wrote:
> 
> [...]
> 
>>>>>> 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.
> 
> Why is 1920x1080-59.94Hz mode unexpected in the first place ?

Because video PLL1 is supposed to be shared by LDB and Samsung
MIPI DSI display pipelines on i.MX8MP EVK and video PLL1 clock
rate won't be changed once it is assigned to 1.0395GHz in
imx8mp.dtsi at least for i.MX8MP EVK.  This 1.0395GHz satisfies
the need to drive typical 1920x1080@60 display mode read from
HDMI monitors connected to LVDS to HDMI(IT6263) and
MIPI DSI to HDMI(ADV7535) transmitters.  The transmitters can
be connected to i.MX8MP EVK through MINI-SAS connectors.  If
the MINI-SAS connectors can also connect to a LVDS panel or
MIPI DSI panel if the transmitters are not connected.

This 1.0395GHz just doesn't satisfy 1920x1080-59.94Hz display
mode with 148.352MHz pixel clock rate.

> I assume your display device reports that it supports this mode, and now the scanout engine and LDB can generate this mode too ? Or does the display device NOT support this mode ?

This display mode is read from a HDMI monitor's EDID, so the
display device supports it for sure.

The scanout engine and LDB cannot support this display mode
with the fixed 1.0395GHz video PLL1 clock rate.  With other
particular video PLL1 clock rate, they can.

> 
>>> 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.
> 
> Can't the LDB still set the Video PLL frequency to whatever it needs first, fixate it, and only then let the LCDIFv3 divide the frequency down ? (sorry, I am a bit tired today, maybe I am missing the obvious)

Yes, LDB driver can set video PLL clock rate directly, but it
needs to get the video PLL clock first by calling clk_get_parent(),
which doesn't look nice.

Let LCDIFv3 driver divide the rate down?  Not easy... You know
it needs to make LDB driver's atomic_enable() happen before
LCDIFv3 driver's atomic_enable().

> 
>>>>       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.
> 
> They do NOT need to share a PLL, you can use e.g. PLL3 for one and Video PLL for the other if the requirement is accurate pixel clock .

I need to share the video PLL clock on i.MX8MP EVK for LDB and Samsung
MIPI DSI display pipelines.  PLL3 is supposed to be the parent clock of
audio AXI clock at least on i.MX8MP EVK.  All other clocks in
imx8mp_media_disp_pix_sels[] are not appropriate to be used by the
display pipelines, except "video_pll1_out", at least for i.MX8MP EVK.

> 
> [...]

Liu Ying Oct. 31, 2024, 2:35 a.m. UTC | #11

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 <[e-mail]>
>>>>>>> ---
>>>>>>> Cc: Abel Vesa <[e-mail]>
>>>>>>> Cc: Andrzej Hajda <[e-mail]>
>>>>>>> Cc: David Airlie <[e-mail]>
>>>>>>> Cc: Fabio Estevam <[e-mail]>
>>>>>>> Cc: Isaac Scott <[e-mail]>
>>>>>>> Cc: Jernej Skrabec <[e-mail]>
>>>>>>> Cc: Jonas Karlman <[e-mail]>
>>>>>>> Cc: Laurent Pinchart <[e-mail]>
>>>>>>> Cc: Maarten Lankhorst <[e-mail]>
>>>>>>> Cc: Maxime Ripard <[e-mail]>
>>>>>>> Cc: Michael Turquette <[e-mail]>
>>>>>>> Cc: Neil Armstrong <[e-mail]>
>>>>>>> Cc: Peng Fan <[e-mail]>
>>>>>>> Cc: Pengutronix Kernel Team <[e-mail]>
>>>>>>> Cc: Robert Foss <[e-mail]>
>>>>>>> Cc: Sascha Hauer <[e-mail]>
>>>>>>> Cc: Shawn Guo <[e-mail]>
>>>>>>> Cc: Simona Vetter <[e-mail]>
>>>>>>> Cc: Stephen Boyd <[e-mail]>
>>>>>>> Cc: Thomas Zimmermann <[e-mail]>
>>>>>>> Cc: [e-mail]
>>>>>>> Cc: [e-mail]
>>>>>>> Cc: [e-mail]
>>>>>>> Cc: [e-mail]
>>>>>>> Cc: [e-mail]
>>>>>>> ---
>>>>>>>    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

Maxime Ripard Nov. 18, 2024, 3:46 p.m. UTC | #12

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 <[e-mail]>
> >>>>>>> ---
> >>>>>>> Cc: Abel Vesa <[e-mail]>
> >>>>>>> Cc: Andrzej Hajda <[e-mail]>
> >>>>>>> Cc: David Airlie <[e-mail]>
> >>>>>>> Cc: Fabio Estevam <[e-mail]>
> >>>>>>> Cc: Isaac Scott <[e-mail]>
> >>>>>>> Cc: Jernej Skrabec <[e-mail]>
> >>>>>>> Cc: Jonas Karlman <[e-mail]>
> >>>>>>> Cc: Laurent Pinchart <[e-mail]>
> >>>>>>> Cc: Maarten Lankhorst <[e-mail]>
> >>>>>>> Cc: Maxime Ripard <[e-mail]>
> >>>>>>> Cc: Michael Turquette <[e-mail]>
> >>>>>>> Cc: Neil Armstrong <[e-mail]>
> >>>>>>> Cc: Peng Fan <[e-mail]>
> >>>>>>> Cc: Pengutronix Kernel Team <[e-mail]>
> >>>>>>> Cc: Robert Foss <[e-mail]>
> >>>>>>> Cc: Sascha Hauer <[e-mail]>
> >>>>>>> Cc: Shawn Guo <[e-mail]>
> >>>>>>> Cc: Simona Vetter <[e-mail]>
> >>>>>>> Cc: Stephen Boyd <[e-mail]>
> >>>>>>> Cc: Thomas Zimmermann <[e-mail]>
> >>>>>>> Cc: [e-mail]
> >>>>>>> Cc: [e-mail]
> >>>>>>> Cc: [e-mail]
> >>>>>>> Cc: [e-mail]
> >>>>>>> Cc: [e-mail]
> >>>>>>> ---
> >>>>>>>    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.

Maxime

Liu Ying Nov. 19, 2024, 2:09 a.m. UTC | #13

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 <[e-mail]>
>>>>>>>>> ---
>>>>>>>>> Cc: Abel Vesa <[e-mail]>
>>>>>>>>> Cc: Andrzej Hajda <[e-mail]>
>>>>>>>>> Cc: David Airlie <[e-mail]>
>>>>>>>>> Cc: Fabio Estevam <[e-mail]>
>>>>>>>>> Cc: Isaac Scott <[e-mail]>
>>>>>>>>> Cc: Jernej Skrabec <[e-mail]>
>>>>>>>>> Cc: Jonas Karlman <[e-mail]>
>>>>>>>>> Cc: Laurent Pinchart <[e-mail]>
>>>>>>>>> Cc: Maarten Lankhorst <[e-mail]>
>>>>>>>>> Cc: Maxime Ripard <[e-mail]>
>>>>>>>>> Cc: Michael Turquette <[e-mail]>
>>>>>>>>> Cc: Neil Armstrong <[e-mail]>
>>>>>>>>> Cc: Peng Fan <[e-mail]>
>>>>>>>>> Cc: Pengutronix Kernel Team <[e-mail]>
>>>>>>>>> Cc: Robert Foss <[e-mail]>
>>>>>>>>> Cc: Sascha Hauer <[e-mail]>
>>>>>>>>> Cc: Shawn Guo <[e-mail]>
>>>>>>>>> Cc: Simona Vetter <[e-mail]>
>>>>>>>>> Cc: Stephen Boyd <[e-mail]>
>>>>>>>>> Cc: Thomas Zimmermann <[e-mail]>
>>>>>>>>> Cc: [e-mail]
>>>>>>>>> Cc: [e-mail]
>>>>>>>>> Cc: [e-mail]
>>>>>>>>> Cc: [e-mail]
>>>>>>>>> Cc: [e-mail]
>>>>>>>>> ---
>>>>>>>>>    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

Marek Vasut Nov. 21, 2024, 2:47 a.m. UTC | #14

I'll stop this thread, let's continue the discussion in one place in:

[PATCH v7 2/7] Revert "clk: imx: clk-imx8mp: Allow media_disp pixel 
clock reconfigure parent rate"

Patch

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);
 	hws[IMX8MP_CLK_MEMREPAIR] = imx8m_clk_hw_composite_critical("mem_repair", imx8mp_memrepair_sels, ccm_base + 0xbf80);
 	hws[IMX8MP_CLK_MEDIA_MIPI_TEST_BYTE] = imx8m_clk_hw_composite("media_mipi_test_byte", imx8mp_media_mipi_test_byte_sels, ccm_base + 0xc100);
 	hws[IMX8MP_CLK_ECSPI3] = imx8m_clk_hw_composite("ecspi3", imx8mp_ecspi3_sels, ccm_base + 0xc180);