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

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);