clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate

Commit Message

Marek Vasut May 31, 2024, 8:26 p.m. UTC

The media_disp[12]_pix clock supply LCDIFv3 pixel clock output. These
clocks are usually the only downstream clock from Video PLL on i.MX8MP.
Allow these clocks to reconfigure the Video PLL, as that results in
accurate pixel clock. If the Video PLL is not reconfigured, the pixel
clock accuracy is low.

Signed-off-by: Marek Vasut <[e-mail]>
---
Cc: Abel Vesa <[e-mail]>
Cc: Fabio Estevam <[e-mail]>
Cc: Lukas F. Hartmann <[e-mail]>
Cc: Michael Turquette <[e-mail]>
Cc: Peng Fan <[e-mail]>
Cc: Pengutronix Kernel Team <[e-mail]>
Cc: Sascha Hauer <[e-mail]>
Cc: Shawn Guo <[e-mail]>
Cc: Stephen Boyd <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
---
 drivers/clk/imx/clk-imx8mp.c | 4 ++--
 drivers/clk/imx/clk.h        | 4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)

Comments

Abel Vesa June 21, 2024, 4:48 a.m. UTC | #1

On 24-05-31 22:26:26, Marek Vasut wrote:
> The media_disp[12]_pix clock supply LCDIFv3 pixel clock output. These
> clocks are usually the only downstream clock from Video PLL on i.MX8MP.
> Allow these clocks to reconfigure the Video PLL, as that results in
> accurate pixel clock. If the Video PLL is not reconfigured, the pixel
> clock accuracy is low.
> 
> Signed-off-by: Marek Vasut <[e-mail]>

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

> ---
> Cc: Abel Vesa <[e-mail]>
> Cc: Fabio Estevam <[e-mail]>
> Cc: Lukas F. Hartmann <[e-mail]>
> Cc: Michael Turquette <[e-mail]>
> Cc: Peng Fan <[e-mail]>
> Cc: Pengutronix Kernel Team <[e-mail]>
> Cc: Sascha Hauer <[e-mail]>
> Cc: Shawn Guo <[e-mail]>
> Cc: Stephen Boyd <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  drivers/clk/imx/clk-imx8mp.c | 4 ++--
>  drivers/clk/imx/clk.h        | 4 ++++
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
> index 670aa2bab3017..f11dfe2028319 100644
> --- a/drivers/clk/imx/clk-imx8mp.c
> +++ b/drivers/clk/imx/clk-imx8mp.c
> @@ -547,7 +547,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
>  	hws[IMX8MP_CLK_AHB] = imx8m_clk_hw_composite_bus_critical("ahb_root", imx8mp_ahb_sels, ccm_base + 0x9000);
>  	hws[IMX8MP_CLK_AUDIO_AHB] = imx8m_clk_hw_composite_bus("audio_ahb", imx8mp_audio_ahb_sels, ccm_base + 0x9100);
>  	hws[IMX8MP_CLK_MIPI_DSI_ESC_RX] = imx8m_clk_hw_composite_bus("mipi_dsi_esc_rx", imx8mp_mipi_dsi_esc_rx_sels, ccm_base + 0x9200);
> -	hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300);
> +	hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300, CLK_SET_RATE_PARENT);
>  
>  	hws[IMX8MP_CLK_IPG_ROOT] = imx_clk_hw_divider2("ipg_root", "ahb_root", ccm_base + 0x9080, 0, 1);
>  
> @@ -609,7 +609,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
>  	hws[IMX8MP_CLK_USDHC3] = imx8m_clk_hw_composite("usdhc3", imx8mp_usdhc3_sels, ccm_base + 0xbc80);
>  	hws[IMX8MP_CLK_MEDIA_CAM1_PIX] = imx8m_clk_hw_composite("media_cam1_pix", imx8mp_media_cam1_pix_sels, ccm_base + 0xbd00);
>  	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("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00);
> +	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_MEMREPAIR] = imx8m_clk_hw_composite_critical("mem_repair", imx8mp_memrepair_sels, ccm_base + 0xbf80);
> diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
> index adb7ad649a0d2..aa5202f284f3d 100644
> --- a/drivers/clk/imx/clk.h
> +++ b/drivers/clk/imx/clk.h
> @@ -442,6 +442,10 @@ struct clk_hw *__imx8m_clk_hw_composite(const char *name,
>  	_imx8m_clk_hw_composite(name, parent_names, reg, \
>  			IMX_COMPOSITE_BUS, IMX_COMPOSITE_CLK_FLAGS_DEFAULT)
>  
> +#define imx8m_clk_hw_composite_bus_flags(name, parent_names, reg, flags) \
> +	_imx8m_clk_hw_composite(name, parent_names, reg, \
> +			IMX_COMPOSITE_BUS, IMX_COMPOSITE_CLK_FLAGS_DEFAULT | flags)
> +
>  #define imx8m_clk_hw_composite_bus_critical(name, parent_names, reg)	\
>  	_imx8m_clk_hw_composite(name, parent_names, reg, \
>  			IMX_COMPOSITE_BUS, IMX_COMPOSITE_CLK_FLAGS_CRITICAL)
> -- 
> 2.43.0
>

Abel Vesa June 21, 2024, 6:38 a.m. UTC | #2

On Fri, 31 May 2024 22:26:26 +0200, Marek Vasut wrote:
> The media_disp[12]_pix clock supply LCDIFv3 pixel clock output. These
> clocks are usually the only downstream clock from Video PLL on i.MX8MP.
> Allow these clocks to reconfigure the Video PLL, as that results in
> accurate pixel clock. If the Video PLL is not reconfigured, the pixel
> clock accuracy is low.
> 
> 
> [...]

Applied, thanks!

[1/1] clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate
      commit: ff06ea04e4cf3ba2f025024776e83bfbdfa05155

Best regards,

Adam Ford June 21, 2024, 5:52 p.m. UTC | #3

On Fri, May 31, 2024 at 3:36 PM Marek Vasut <[e-mail]> wrote:
>
> The media_disp[12]_pix clock supply LCDIFv3 pixel clock output. These
> clocks are usually the only downstream clock from Video PLL on i.MX8MP.
> Allow these clocks to reconfigure the Video PLL, as that results in
> accurate pixel clock. If the Video PLL is not reconfigured, the pixel
> clock accuracy is low.

What happens if you have both a DSI (IMX8MP_CLK_MEDIA_DISP1_PIX) and
an LVDS (IMX8MP_CLK_MEDIA_DISP2_PIX) display and both try to
reconfigure their shared parent clock when their resolutions/clocks
may not be the same?  I looked at doing that for the 8MP, but I was
running into display issues.

For example, I was trying to test an 800x480 LVDS display which needed
a pixel clock of 30MHz, and a DSI trying to run at 1920x1080 @
148.5MHz.

adam

>
> Signed-off-by: Marek Vasut <[e-mail]>
> ---
> Cc: Abel Vesa <[e-mail]>
> Cc: Fabio Estevam <[e-mail]>
> Cc: Lukas F. Hartmann <[e-mail]>
> Cc: Michael Turquette <[e-mail]>
> Cc: Peng Fan <[e-mail]>
> Cc: Pengutronix Kernel Team <[e-mail]>
> Cc: Sascha Hauer <[e-mail]>
> Cc: Shawn Guo <[e-mail]>
> Cc: Stephen Boyd <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  drivers/clk/imx/clk-imx8mp.c | 4 ++--
>  drivers/clk/imx/clk.h        | 4 ++++
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
> index 670aa2bab3017..f11dfe2028319 100644
> --- a/drivers/clk/imx/clk-imx8mp.c
> +++ b/drivers/clk/imx/clk-imx8mp.c
> @@ -547,7 +547,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
>         hws[IMX8MP_CLK_AHB] = imx8m_clk_hw_composite_bus_critical("ahb_root", imx8mp_ahb_sels, ccm_base + 0x9000);
>         hws[IMX8MP_CLK_AUDIO_AHB] = imx8m_clk_hw_composite_bus("audio_ahb", imx8mp_audio_ahb_sels, ccm_base + 0x9100);
>         hws[IMX8MP_CLK_MIPI_DSI_ESC_RX] = imx8m_clk_hw_composite_bus("mipi_dsi_esc_rx", imx8mp_mipi_dsi_esc_rx_sels, ccm_base + 0x9200);
> -       hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300);
> +       hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300, CLK_SET_RATE_PARENT);
>
>         hws[IMX8MP_CLK_IPG_ROOT] = imx_clk_hw_divider2("ipg_root", "ahb_root", ccm_base + 0x9080, 0, 1);
>
> @@ -609,7 +609,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
>         hws[IMX8MP_CLK_USDHC3] = imx8m_clk_hw_composite("usdhc3", imx8mp_usdhc3_sels, ccm_base + 0xbc80);
>         hws[IMX8MP_CLK_MEDIA_CAM1_PIX] = imx8m_clk_hw_composite("media_cam1_pix", imx8mp_media_cam1_pix_sels, ccm_base + 0xbd00);
>         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("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00);
> +       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_MEMREPAIR] = imx8m_clk_hw_composite_critical("mem_repair", imx8mp_memrepair_sels, ccm_base + 0xbf80);
> diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
> index adb7ad649a0d2..aa5202f284f3d 100644
> --- a/drivers/clk/imx/clk.h
> +++ b/drivers/clk/imx/clk.h
> @@ -442,6 +442,10 @@ struct clk_hw *__imx8m_clk_hw_composite(const char *name,
>         _imx8m_clk_hw_composite(name, parent_names, reg, \
>                         IMX_COMPOSITE_BUS, IMX_COMPOSITE_CLK_FLAGS_DEFAULT)
>
> +#define imx8m_clk_hw_composite_bus_flags(name, parent_names, reg, flags) \
> +       _imx8m_clk_hw_composite(name, parent_names, reg, \
> +                       IMX_COMPOSITE_BUS, IMX_COMPOSITE_CLK_FLAGS_DEFAULT | flags)
> +
>  #define imx8m_clk_hw_composite_bus_critical(name, parent_names, reg)   \
>         _imx8m_clk_hw_composite(name, parent_names, reg, \
>                         IMX_COMPOSITE_BUS, IMX_COMPOSITE_CLK_FLAGS_CRITICAL)
> --
> 2.43.0
>
>

Marek Vasut June 21, 2024, 8:22 p.m. UTC | #4

On 6/21/24 7:52 PM, Adam Ford wrote:
> On Fri, May 31, 2024 at 3:36 PM Marek Vasut <[e-mail]> wrote:
>>
>> The media_disp[12]_pix clock supply LCDIFv3 pixel clock output. These
>> clocks are usually the only downstream clock from Video PLL on i.MX8MP.
>> Allow these clocks to reconfigure the Video PLL, as that results in
>> accurate pixel clock. If the Video PLL is not reconfigured, the pixel
>> clock accuracy is low.
> 
> What happens if you have both a DSI (IMX8MP_CLK_MEDIA_DISP1_PIX) and
> an LVDS (IMX8MP_CLK_MEDIA_DISP2_PIX) display and both try to
> reconfigure their shared parent clock when their resolutions/clocks
> may not be the same?  I looked at doing that for the 8MP, but I was
> running into display issues.
> 
> For example, I was trying to test an 800x480 LVDS display which needed
> a pixel clock of 30MHz, and a DSI trying to run at 1920x1080 @
> 148.5MHz.

I have such a setup on my desk, I ended up putting one of the LCDIFs on 
Video PLL and the other on PLL3 (spare unused PLL) .

Luca Ceresoli Nov. 12, 2024, 10:42 p.m. UTC | #5

Hello Marek, Abel,

+Cc: Miquèl

On Fri, 31 May 2024 22:26:26 +0200
Marek Vasut <[e-mail]> wrote:

> The media_disp[12]_pix clock supply LCDIFv3 pixel clock output. These
> clocks are usually the only downstream clock from Video PLL on i.MX8MP.
> Allow these clocks to reconfigure the Video PLL, as that results in
> accurate pixel clock. If the Video PLL is not reconfigured, the pixel
> clock accuracy is low.
> 
> Signed-off-by: Marek Vasut <[e-mail]>

I'm afraid I just found this patch broke my previously working setup
with a panel connected on the LDB. As we are at 6.12-rc7, and this
patch got merged at 6.12-rc1, I'm reporting it immediately after a
quick preliminary analysis in order to hopefully find an appropriate
solution before 6.12.

Problem: with this patch, my LDB picture is horizontally shrunk by a
factor of about 7, measured empirically.

Configuration:
 - lcdif1 and lcdif3 disabled
 - a single-channel LVDS panel on lcdif2/ldb channel 2

So this problem looks like different from the one reported by Adam as
there a single panel and still it stops working.

The relevant rates in my case are as follows:

                            video_pll1   media_disp2_pix    media_ldb
                          ~~~~~~~~~~~~~  ~~~~~~~~~~~~~~~  ~~~~~~~~~~~~~
at boot, panel still off: 1.039.500.000    1.039.500.000  1.039.500.000
requested LDB clock:                                        503.300.000
before this patch:        1.039.500.000       74.250.000    519.750.000
after this patch:            71.900.000       71.900.000     71.900.000

Previously, the resulting media_ldb clock was not precise, but close
enough. Now the value is clearly too wrong to work. Also (but my memory
might be wrong here) there must be a ratio of 7 between media_ldb and
media_disp2_pix, which has become 1 after this patch, and this perhaps
explains the shrink factor of about 7.

I double checked the rate that fsl_ldb_atomic_enable() is requesting at
[0] and it is 503.3 MHz with and without the patch. This is proved by
the logged line, before and after the patch:

fsl-ldb 32ec0000.blk-ctrl:bridge@5c: Configured LDB clock (519750000 Hz) does not match requested LVDS clock: 503300000 Hz
fsl-ldb 32ec0000.blk-ctrl:bridge@5c: Configured LDB clock (71900000 Hz) does not match requested LVDS clock: 503300000 Hz

My preliminary conclusions:

 * it looks like a bug leading to an incorrect computation of the
   video_pll1 rate when media_ldb is requesting its rate
 * the bug does not look like due to this patch, but exposed by it

I still have no idea how the video_pll1 gets configured to such a low
value when its child needs a 500+ MHz clock.

Any clues or ideas would be welcome.

Best regards,
Luca

[0] https://elixir.bootlin.com/linux/v6.12-rc7/source/drivers/gpu/drm/bridge/fsl-ldb.c#L180

Marek Vasut Nov. 12, 2024, 11:14 p.m. UTC | #6

On 11/12/24 11:42 PM, Luca Ceresoli wrote:
> Hello Marek, Abel,

Hi,

> +Cc: Miquèl
> 
> On Fri, 31 May 2024 22:26:26 +0200
> Marek Vasut <[e-mail]> wrote:
> 
>> The media_disp[12]_pix clock supply LCDIFv3 pixel clock output. These
>> clocks are usually the only downstream clock from Video PLL on i.MX8MP.
>> Allow these clocks to reconfigure the Video PLL, as that results in
>> accurate pixel clock. If the Video PLL is not reconfigured, the pixel
>> clock accuracy is low.
>>
>> Signed-off-by: Marek Vasut <[e-mail]>
> 
> I'm afraid I just found this patch broke my previously working setup
> with a panel connected on the LDB.
Do you need a fix similar to this one?

4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1 
frequency to 506.8 MHz")

Luca Ceresoli Nov. 13, 2024, 11:06 a.m. UTC | #7

Hi Marek,

+Cc: Anson (initial author of clk-imx8mp.c)

On Wed, 13 Nov 2024 00:14:15 +0100
Marek Vasut <[e-mail]> wrote:

> On 11/12/24 11:42 PM, Luca Ceresoli wrote:
> > Hello Marek, Abel,  
> 
> Hi,
> 
> > +Cc: Miquèl
> > 
> > On Fri, 31 May 2024 22:26:26 +0200
> > Marek Vasut <[e-mail]> wrote:
> >   
> >> The media_disp[12]_pix clock supply LCDIFv3 pixel clock output. These
> >> clocks are usually the only downstream clock from Video PLL on i.MX8MP.
> >> Allow these clocks to reconfigure the Video PLL, as that results in
> >> accurate pixel clock. If the Video PLL is not reconfigured, the pixel
> >> clock accuracy is low.
> >>
> >> Signed-off-by: Marek Vasut <[e-mail]>  
> > 
> > I'm afraid I just found this patch broke my previously working setup
> > with a panel connected on the LDB.  
> Do you need a fix similar to this one?
> 
> 4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1 
> frequency to 506.8 MHz")

So, 4fbb73416b10 is adding an assigned-clock-rates to hardcode rates,
especially the video_pll1 rate.

However this is not fixing the problem I'm seeing. The existing
assigned-clock-rates value for video_pll1 used to work because it is
the media_ldb parent, and the parent wasn't recalculated. After this
patch got applied the video_pll1 rate is computed at runtime and so the
hardcoded value in assigned-clock-rates does not matter in the end.

I also tried a configuration that appears to me as the most optimal for
managing both an LVDS panel on LDB and a DSI panel (which is also
present in the more complete design I'm working on):

 * media_ldb and media_disp2 (the two clocks involved in LDB/LVDS
   output) left as children of video_pll1 as per imx8mp.dtsi
 * media_disp1 (used for DSI output) reparented to sys_pll3

The above config assigns to each output (LVDS and DSI) an ad-hoc PLL.
However the problem does not disappear, simply because the problem is
that requesting a ~500 MHz rate to video_pll1 results in it to be
configured at ~72 MHz.
  
This confirms the problem I reported appears to be an incorrect
computation of the video_pll1 rate, which in turn looks like a bug
(which as I said is exposed, not introduced, by this patch). If
setting a hardcoded value could make it work, that would look like
hiding a bug, wouldn't it?

And at least with a single-panel setup the runtime computation should
work just fine.

More generally speaking, I don't follow your approach: your patch
enables runtime computation of the video_pll1 rate, but you now suggest
to hardcode it. Am I missing something?

Luca

Marek Vasut Nov. 13, 2024, 9:19 p.m. UTC | #8

On 11/13/24 12:06 PM, Luca Ceresoli wrote:
> Hi Marek,

Hi,

>>>> The media_disp[12]_pix clock supply LCDIFv3 pixel clock output. These
>>>> clocks are usually the only downstream clock from Video PLL on i.MX8MP.
>>>> Allow these clocks to reconfigure the Video PLL, as that results in
>>>> accurate pixel clock. If the Video PLL is not reconfigured, the pixel
>>>> clock accuracy is low.
>>>>
>>>> Signed-off-by: Marek Vasut <[e-mail]>
>>>
>>> I'm afraid I just found this patch broke my previously working setup
>>> with a panel connected on the LDB.
>> Do you need a fix similar to this one?
>>
>> 4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1
>> frequency to 506.8 MHz")
> 
> So, 4fbb73416b10 is adding an assigned-clock-rates to hardcode rates,
> especially the video_pll1 rate.

Nope.

See arch/arm64/boot/dts/freescale/imx8mp.dtsi

1891                         media_blk_ctrl: blk-ctrl@32ec0000 {
...
1951                                 assigned-clock-rates = <500000000>, 
<200000000>,
1952                                                        <0>, <0>, 
<500000000>,
1953                                                        <1039500000>;

That imx8mp.dtsi preconfigures the Video PLL1 to some random clock 
frequency.

Commit 4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1 
frequency to 506.8 MHz") configures the Video PLL1 to a frequency from 
which both your panel pixel clock and LDB serializer clock can be 
successfully divided down.

Which panel do you use ?

Try this -- Revert this patch, check /sys/kernel/debug/clk/clk_summary 
and compare it with (I assume) panel-simple.c entry for the panel you 
use, and notice the disp_pix clock are likely a bit off. That's because 
the lcdif driver did its best to divide those pixel clock down from 
1039500000 set in imx8mp.dtsi .

If you really want accurate pixel clock for your panel, you need similar 
change to 4fbb73416b10 and configure the Video PLL such that the 
accurate pixel clock can be derived from it. The Video PLL cannot be set 
to pixel clock, because the LDB serializer clock are either 7x the pixel 
clock, or 3.5x the pixel clock (for dual link LVDS), so the Video PLL 
has to be set to 7x or 3.5x pixel clock of the panel, then you should 
get accurate pixel clock and a working panel again.

> However this is not fixing the problem I'm seeing. The existing
> assigned-clock-rates value for video_pll1 used to work because it is
> the media_ldb parent, and the parent wasn't recalculated. After this
> patch got applied the video_pll1 rate is computed at runtime and so the
> hardcoded value in assigned-clock-rates does not matter in the end.
> 
> I also tried a configuration that appears to me as the most optimal for
> managing both an LVDS panel on LDB and a DSI panel (which is also
> present in the more complete design I'm working on):
> 
>   * media_ldb and media_disp2 (the two clocks involved in LDB/LVDS
>     output) left as children of video_pll1 as per imx8mp.dtsi
>   * media_disp1 (used for DSI output) reparented to sys_pll3

I noticed that the PLL3 PLL1416x does not support dynamic rate 
configuration, I sent a patch for that a few days ago, but in the end I 
also found out that the PLL1416x is less accurate and cannot supply the 
LVDS side, so I gave up on that one.

I ended up using Audio PLL for one of the displays in similar setup 
(LVDS panel + DP monitor via DSI-to-DP bridge) , see this:

https://github.com/dh-electronics/meta-dhsom-imx-bsp/commit/fb3b1521bbe71b7ff91fe92be2e64a75104eb7d5

That way, I do get accurate pixel clock on both outputs, but I do have 
to sacrifice one of the Audio PLLs. On the other hand, if all you have 
is one audio output, the Audio PLL is PLL1443x and does support dynamic 
rate configuration, so one Audio PLL should be enough for such a use 
case. Also, upside is, PLL3 can supply the Audio DSP if needed.

> The above config assigns to each output (LVDS and DSI) an ad-hoc PLL.
> However the problem does not disappear, simply because the problem is
> that requesting a ~500 MHz rate to video_pll1 results in it to be
> configured at ~72 MHz.
>    
> This confirms the problem I reported appears to be an incorrect
> computation of the video_pll1 rate, which in turn looks like a bug
> (which as I said is exposed, not introduced, by this patch). If
> setting a hardcoded value could make it work, that would look like
> hiding a bug, wouldn't it?

I missed the part where you use both LVDS and DSI outputs , which both 
hang from the Video PLL1. I think that is what triggers the problem, and 
I think the link above might actually help with that.

> And at least with a single-panel setup the runtime computation should
> work just fine.
> 
> More generally speaking, I don't follow your approach: your patch
> enables runtime computation of the video_pll1 rate, but you now suggest
> to hardcode it. Am I missing something?
The patch I linked sets up the Video PLL1 frequency. When either LDB or 
LCDIF attempts to divide down the serializer or pixel clock from that 
Video PLL1, they can do so by setting a divider, without changing the 
Video PLL1 frequency. That's the crutch here.

I am looking for some better way to deal with it, see
[PATCH 1/2] clk: imx: clk-imx8mp: Allow LDB serializer clock reconfigure 
parent rate
[PATCH 2/2] drm: bridge: ldb: Configure LDB clock in .mode_set
and discussion with Isaac around
Re: [PATCH] drm: lcdif: Use adjusted_mode .clock instead of .crtc_clock

Luca Ceresoli Nov. 15, 2024, 5:09 p.m. UTC | #9

Hi Marek,

just a quick feedback about my latest discoveries.

On Wed, 13 Nov 2024 22:19:02 +0100
Marek Vasut <[e-mail]> wrote:

> On 11/13/24 12:06 PM, Luca Ceresoli wrote:
> > Hi Marek,  
> 
> Hi,
> 
> >>>> The media_disp[12]_pix clock supply LCDIFv3 pixel clock output. These
> >>>> clocks are usually the only downstream clock from Video PLL on i.MX8MP.
> >>>> Allow these clocks to reconfigure the Video PLL, as that results in
> >>>> accurate pixel clock. If the Video PLL is not reconfigured, the pixel
> >>>> clock accuracy is low.
> >>>>
> >>>> Signed-off-by: Marek Vasut <[e-mail]>  
> >>>
> >>> I'm afraid I just found this patch broke my previously working setup
> >>> with a panel connected on the LDB.  
> >> Do you need a fix similar to this one?
> >>
> >> 4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1
> >> frequency to 506.8 MHz")  
> > 
> > So, 4fbb73416b10 is adding an assigned-clock-rates to hardcode rates,
> > especially the video_pll1 rate.  
> 
> Nope.
> 
> See arch/arm64/boot/dts/freescale/imx8mp.dtsi
> 
> 1891                         media_blk_ctrl: blk-ctrl@32ec0000 {
> ...
> 1951                                 assigned-clock-rates = <500000000>, 
> <200000000>,
> 1952                                                        <0>, <0>, 
> <500000000>,
> 1953                                                        <1039500000>;
> 
> That imx8mp.dtsi preconfigures the Video PLL1 to some random clock 
> frequency.
> 
> Commit 4fbb73416b10 ("arm64: dts: imx8mp-phyboard-pollux: Set Video PLL1 
> frequency to 506.8 MHz") configures the Video PLL1 to a frequency from 
> which both your panel pixel clock and LDB serializer clock can be 
> successfully divided down.
> 
> Which panel do you use ?
> 
> Try this -- Revert this patch, check /sys/kernel/debug/clk/clk_summary 
> and compare it with (I assume) panel-simple.c entry for the panel you 
> use, and notice the disp_pix clock are likely a bit off. That's because 
> the lcdif driver did its best to divide those pixel clock down from 
> 1039500000 set in imx8mp.dtsi .
> 
> If you really want accurate pixel clock for your panel, you need similar 
> change to 4fbb73416b10 and configure the Video PLL such that the 
> accurate pixel clock can be derived from it. The Video PLL cannot be set 
> to pixel clock, because the LDB serializer clock are either 7x the pixel 
> clock, or 3.5x the pixel clock (for dual link LVDS), so the Video PLL 
> has to be set to 7x or 3.5x pixel clock of the panel, then you should 
> get accurate pixel clock and a working panel again.

I found that I'm having the same issue that has been discussed in some
related threads: the lcdif2 configures the video_pll1 to ~72 MHz, and
later LDB tries to set it to 7x that value, failing.

I would have guessed your "[PATCH 1/2] clk: imx: clk-imx8mp: Allow LDB
serializer clock reconfigure parent rate" would have fixed it, and it
actually allwos the LDB to set a proper (7x) rate for video_pll1, but
then also the media_disp2 goes to the same rate. Apparently the
video_pll1 is not propagated to media_disp2. I haven't dug into this.

So this is not the bug I had suspected about video_pll1 rate
computation.

For now my workaround is to set the exact rate to video_pll1 via
assigned-clock-rates.

However this breaks the case of using both lcdif1 and lcdif2. For that
I have added a reparenting of media_disp1 to sys_pll3 and it looks like
working.

Would you mind keeping Miquèl and me in Cc in following discussions
about the imx8mp video clocks? We are also facing this topic and we are
happy to contribute.

Luca

Patch

diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
index 670aa2bab3017..f11dfe2028319 100644
--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -547,7 +547,7 @@  static int imx8mp_clocks_probe(struct platform_device *pdev)
 	hws[IMX8MP_CLK_AHB] = imx8m_clk_hw_composite_bus_critical("ahb_root", imx8mp_ahb_sels, ccm_base + 0x9000);
 	hws[IMX8MP_CLK_AUDIO_AHB] = imx8m_clk_hw_composite_bus("audio_ahb", imx8mp_audio_ahb_sels, ccm_base + 0x9100);
 	hws[IMX8MP_CLK_MIPI_DSI_ESC_RX] = imx8m_clk_hw_composite_bus("mipi_dsi_esc_rx", imx8mp_mipi_dsi_esc_rx_sels, ccm_base + 0x9200);
-	hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300);
+	hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300, CLK_SET_RATE_PARENT);
 
 	hws[IMX8MP_CLK_IPG_ROOT] = imx_clk_hw_divider2("ipg_root", "ahb_root", ccm_base + 0x9080, 0, 1);
 
@@ -609,7 +609,7 @@  static int imx8mp_clocks_probe(struct platform_device *pdev)
 	hws[IMX8MP_CLK_USDHC3] = imx8m_clk_hw_composite("usdhc3", imx8mp_usdhc3_sels, ccm_base + 0xbc80);
 	hws[IMX8MP_CLK_MEDIA_CAM1_PIX] = imx8m_clk_hw_composite("media_cam1_pix", imx8mp_media_cam1_pix_sels, ccm_base + 0xbd00);
 	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("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00);
+	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_MEMREPAIR] = imx8m_clk_hw_composite_critical("mem_repair", imx8mp_memrepair_sels, ccm_base + 0xbf80);
diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
index adb7ad649a0d2..aa5202f284f3d 100644
--- a/drivers/clk/imx/clk.h
+++ b/drivers/clk/imx/clk.h
@@ -442,6 +442,10 @@  struct clk_hw *__imx8m_clk_hw_composite(const char *name,
 	_imx8m_clk_hw_composite(name, parent_names, reg, \
 			IMX_COMPOSITE_BUS, IMX_COMPOSITE_CLK_FLAGS_DEFAULT)
 
+#define imx8m_clk_hw_composite_bus_flags(name, parent_names, reg, flags) \
+	_imx8m_clk_hw_composite(name, parent_names, reg, \
+			IMX_COMPOSITE_BUS, IMX_COMPOSITE_CLK_FLAGS_DEFAULT | flags)
+
 #define imx8m_clk_hw_composite_bus_critical(name, parent_names, reg)	\
 	_imx8m_clk_hw_composite(name, parent_names, reg, \
 			IMX_COMPOSITE_BUS, IMX_COMPOSITE_CLK_FLAGS_CRITICAL)