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

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

[Marek Vasut]

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 <marex@denx.de>
---
Cc: Abel Vesa <abelvesa@kernel.org>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Lukas F. Hartmann <lukas@mntmn.com>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: imx@lists.linux.dev
Cc: kernel@dh-electronics.com
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-clk@vger.kernel.org
---
 drivers/clk/imx/clk-imx8mp.c | 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate

[Abel Vesa]

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 <marex@denx.de>

Reviewed-by: Abel Vesa <abel.vesa@linaro.org>

> ---
> Cc: Abel Vesa <abelvesa@kernel.org>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Lukas F. Hartmann <lukas@mntmn.com>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: imx@lists.linux.dev
> Cc: kernel@dh-electronics.com
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-clk@vger.kernel.org
> ---
>  drivers/clk/imx/clk-imx8mp.c | 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
> 

Re: [PATCH] clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate

[Abel Vesa]

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,
-- 
Abel Vesa <abel.vesa@linaro.org>

Re: [PATCH] clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate

[Adam Ford]

On Fri, May 31, 2024 at 3:36 PM Marek Vasut <marex@denx.de> 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 <marex@denx.de>
> ---
> Cc: Abel Vesa <abelvesa@kernel.org>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Lukas F. Hartmann <lukas@mntmn.com>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: imx@lists.linux.dev
> Cc: kernel@dh-electronics.com
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-clk@vger.kernel.org
> ---
>  drivers/clk/imx/clk-imx8mp.c | 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
>
>

Re: [PATCH] clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate

[Marek Vasut]

On 6/21/24 7:52 PM, Adam Ford wrote:
> On Fri, May 31, 2024 at 3:36 PM Marek Vasut <marex@denx.de> 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) .

Re: [PATCH] clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate

[Luca Ceresoli]

Hello Marek, Abel,

+Cc: Miquèl

On Fri, 31 May 2024 22:26:26 +0200
Marek Vasut <marex@denx.de> 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 <marex@denx.de>

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

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate

[Marek Vasut]

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 <marex@denx.de> 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 <marex@denx.de>
> 
> 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")

Re: [PATCH] clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate

[Luca Ceresoli]

Hi Marek,

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

On Wed, 13 Nov 2024 00:14:15 +0100
Marek Vasut <marex@denx.de> 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 <marex@denx.de> 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 <marex@denx.de>  
> > 
> > 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

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate

[Marek Vasut]

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 <marex@denx.de>
>>>
>>> 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

Re: [PATCH] clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate

[Luca Ceresoli]

Hi Marek,

just a quick feedback about my latest discoveries.

On Wed, 13 Nov 2024 22:19:02 +0100
Marek Vasut <marex@denx.de> 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 <marex@denx.de>  
> >>>
> >>> 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

-- 
Luca Ceresoli, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH] clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate

[Marek Vasut]

On 11/15/24 6:09 PM, Luca Ceresoli wrote:
> Hi Marek,

Hi,

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

Right, which is solved by configuring the Video PLL to the correct 
frequency in DT up front ... unless you have more than one output 
supplied by that Video PLL.

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

See the patch link in the previous email, if you can use one Audio PLL 
as PLL for the other DISP PIX, then you will be able to achieve accurate 
pixel clock on both outputs. Can you do that ?

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

Re: [PATCH] clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate

[Miquel Raynal]

Hi Marek,

>>> 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.
>
> Right, which is solved by configuring the Video PLL to the correct
> frequency in DT up front ... unless you have more than one output
> supplied by that Video PLL.

No, this looks like a bug in the imx8 clock driver. I would expect the
core to handle such case without DT hack. It is not okay to fix clock
frequencies in DT because drivers are failing to do it properly. I
understand there are advanced/dual cases with very specific frequencies
where you don't expect it to magically work and giving hints with DT
assigned-clocks* properties makes sense, but here I don't think we
should consider it as a proper fix.

If I may recap:
1- a simple display pipeline works
2- the pixel frequency could be more precise so the video_pll1 parent is
   used to dynamically compute a better frequency
3- the video_pll1 parent is too low in some cases which breaks the
   pipeline
4- we need to force video_pll1 to a value in DT

How possibly 4 could be a relevant answer to 2, seriously? May I return
you the advice, if you want a better video_pll1 value in the first
place, why not assigning it up front in DT?

I understand your goal, and I agree with it, but please acknowledge that
even though the current patch looks fine per-se, it is exposing a real
bug that is now visible. Hiding it with DT properties feels really wrong.

Thanks,
Miquèl

Re: [PATCH] clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate

[Marek Vasut]

On 11/18/24 9:15 AM, Miquel Raynal wrote:
> Hi Marek,

Hello Miquel,

>>>> 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.
>>
>> Right, which is solved by configuring the Video PLL to the correct
>> frequency in DT up front ... unless you have more than one output
>> supplied by that Video PLL.
> 
> No, this looks like a bug in the imx8 clock driver. I would expect the
> core to handle such case without DT hack. It is not okay to fix clock
> frequencies in DT because drivers are failing to do it properly. I
> understand there are advanced/dual cases with very specific frequencies
> where you don't expect it to magically work and giving hints with DT
> assigned-clocks* properties makes sense, but here I don't think we
> should consider it as a proper fix.

It is not a proper fix, it is the best we can do right now. I already 
replied to Luca with a bunch of patches where I tried to come up with a 
way to negotiate the pixel clock in drivers ... I need to get back to those.

> If I may recap:
> 1- a simple display pipeline works
> 2- the pixel frequency could be more precise so the video_pll1 parent is
>     used to dynamically compute a better frequency
> 3- the video_pll1 parent is too low in some cases which breaks the
>     pipeline
> 4- we need to force video_pll1 to a value in DT
> 
> How possibly 4 could be a relevant answer to 2, seriously? May I return
> you the advice, if you want a better video_pll1 value in the first
> place, why not assigning it up front in DT?

Because I have DSI-to-(e)DP bridge on the DSI bus and I do not know the 
pixel clock needed by attached panel up front.

I already included a link to DTO which allowed me to operate both this 
DSI-to-(e)DP bridge and LVDS panel with accurate pixel clock, I was 
hoping that would also let you solve 3 and 4. 4fbb73416b10 ("arm64: dts: 
imx8mp-phyboard-pollux: Set Video PLL1 frequency to 506.8 MHz") fixed 3. 
for Isaac at least.

> I understand your goal, and I agree with it, but please acknowledge that
> even though the current patch looks fine per-se, it is exposing a real
> bug that is now visible. Hiding it with DT properties feels really wrong.
I do fully agree the whole DT Video PLL1 clock frequency configuration 
is not good and it should not be in the DT at all. That is my goal in 
the very end.

The drivers (in this case, LCDIF1 + LCDIF2 + LDB) should negotiate the 
Video PLL1 frequency that fits them all best and configure it 
accordingly, without any DT assign-clock* workarounds.

I just didn't figure out a way to do that ^ yet.

Re: [PATCH] clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate

[Miquel Raynal]

Hi Marek,

On 18/11/2024 at 15:30:04 +01, Marek Vasut <marex@denx.de> wrote:

> On 11/18/24 9:15 AM, Miquel Raynal wrote:
>> Hi Marek,
>
> Hello Miquel,
>
>>>>> 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.
>>>
>>> Right, which is solved by configuring the Video PLL to the correct
>>> frequency in DT up front ... unless you have more than one output
>>> supplied by that Video PLL.
>> No, this looks like a bug in the imx8 clock driver. I would expect the
>> core to handle such case without DT hack. It is not okay to fix clock
>> frequencies in DT because drivers are failing to do it properly. I
>> understand there are advanced/dual cases with very specific frequencies
>> where you don't expect it to magically work and giving hints with DT
>> assigned-clocks* properties makes sense, but here I don't think we
>> should consider it as a proper fix.
>
> It is not a proper fix, it is the best we can do right now. I already

I am sorry I probably misunderstood your previous reply then. I am fine
with the assigned-clocks workaround.

> replied to Luca with a bunch of patches where I tried to come up with a
> way to negotiate the pixel clock in drivers ... I need to get back to
> those.

Indeed, thanks to your feedback we got it fixed locally, so short term
is okay for us (but people not reading this thread might still suffer
from the problem though).

>> If I may recap:
>> 1- a simple display pipeline works
>> 2- the pixel frequency could be more precise so the video_pll1 parent is
>>     used to dynamically compute a better frequency
>> 3- the video_pll1 parent is too low in some cases which breaks the
>>     pipeline
>> 4- we need to force video_pll1 to a value in DT
>> How possibly 4 could be a relevant answer to 2, seriously? May I
>> return
>> you the advice, if you want a better video_pll1 value in the first
>> place, why not assigning it up front in DT?
>
> Because I have DSI-to-(e)DP bridge on the DSI bus and I do not know the
> pixel clock needed by attached panel up front.
>
> I already included a link to DTO which allowed me to operate both this
> DSI-to-(e)DP bridge and LVDS panel with accurate pixel clock, I was
> hoping that would also let you solve 3 and 4. 4fbb73416b10 ("arm64: dts:
> imx8mp-phyboard-pollux: Set Video PLL1 frequency to 506.8 MHz") fixed
> 3. for Isaac at least.
>
>> I understand your goal, and I agree with it, but please acknowledge that
>> even though the current patch looks fine per-se, it is exposing a real
>> bug that is now visible. Hiding it with DT properties feels really wrong.
> I do fully agree the whole DT Video PLL1 clock frequency configuration
> is not good and it should not be in the DT at all. That is my goal in
> the very end.
>
> The drivers (in this case, LCDIF1 + LCDIF2 + LDB) should negotiate the
> Video PLL1 frequency that fits them all best and configure it
> accordingly, without any DT assign-clock* workarounds.

Ok, good to know we are aligned :-)

> I just didn't figure out a way to do that ^ yet.

Of course, getting rid of the DT workarounds is probably a long term
goal, unlike the mid-term goal which is: "fixing" today's situation for
"everyone with a simple setup". We are also looking into this and
willing to find a proper solution.

Cheers,
Miquèl

Re: [PATCH] clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate

[Marek Vasut]

On 11/19/24 4:41 PM, Miquel Raynal wrote:

Hello Miquel,

>>>> Right, which is solved by configuring the Video PLL to the correct
>>>> frequency in DT up front ... unless you have more than one output
>>>> supplied by that Video PLL.
>>> No, this looks like a bug in the imx8 clock driver. I would expect the
>>> core to handle such case without DT hack. It is not okay to fix clock
>>> frequencies in DT because drivers are failing to do it properly. I
>>> understand there are advanced/dual cases with very specific frequencies
>>> where you don't expect it to magically work and giving hints with DT
>>> assigned-clocks* properties makes sense, but here I don't think we
>>> should consider it as a proper fix.
>>
>> It is not a proper fix, it is the best we can do right now. I already
> 
> I am sorry I probably misunderstood your previous reply then. I am fine
> with the assigned-clocks workaround.
> 
>> replied to Luca with a bunch of patches where I tried to come up with a
>> way to negotiate the pixel clock in drivers ... I need to get back to
>> those.
> 
> Indeed, thanks to your feedback we got it fixed locally, so short term
> is okay for us (but people not reading this thread might still suffer
> from the problem though).

Whew, glad I could help.

>>> If I may recap:
>>> 1- a simple display pipeline works
>>> 2- the pixel frequency could be more precise so the video_pll1 parent is
>>>      used to dynamically compute a better frequency
>>> 3- the video_pll1 parent is too low in some cases which breaks the
>>>      pipeline
>>> 4- we need to force video_pll1 to a value in DT
>>> How possibly 4 could be a relevant answer to 2, seriously? May I
>>> return
>>> you the advice, if you want a better video_pll1 value in the first
>>> place, why not assigning it up front in DT?
>>
>> Because I have DSI-to-(e)DP bridge on the DSI bus and I do not know the
>> pixel clock needed by attached panel up front.
>>
>> I already included a link to DTO which allowed me to operate both this
>> DSI-to-(e)DP bridge and LVDS panel with accurate pixel clock, I was
>> hoping that would also let you solve 3 and 4. 4fbb73416b10 ("arm64: dts:
>> imx8mp-phyboard-pollux: Set Video PLL1 frequency to 506.8 MHz") fixed
>> 3. for Isaac at least.
>>
>>> I understand your goal, and I agree with it, but please acknowledge that
>>> even though the current patch looks fine per-se, it is exposing a real
>>> bug that is now visible. Hiding it with DT properties feels really wrong.
>> I do fully agree the whole DT Video PLL1 clock frequency configuration
>> is not good and it should not be in the DT at all. That is my goal in
>> the very end.
>>
>> The drivers (in this case, LCDIF1 + LCDIF2 + LDB) should negotiate the
>> Video PLL1 frequency that fits them all best and configure it
>> accordingly, without any DT assign-clock* workarounds.
> 
> Ok, good to know we are aligned :-)

Good indeed :)

>> I just didn't figure out a way to do that ^ yet.
> 
> Of course, getting rid of the DT workarounds is probably a long term
> goal, unlike the mid-term goal which is: "fixing" today's situation for
> "everyone with a simple setup". We are also looking into this and
> willing to find a proper solution.
I CCed you on an ongoing conversation with Victor in

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

let's continue the discussion there. I hope we can reach some sort of a 
way forward there, that works for everyone.