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

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

[Marek Vasut]

The media_ldb_root_clk supply LDB serializer. These clock are usually
shared with the LCDIFv3 pixel clock and supplied by the Video PLL on
i.MX8MP, but the LDB clock run at either x7 or x14 rate of the LCDIFv3
pixel clock. Allow the LDB to reconfigure Video PLL as needed, as that
results in accurate serializer clock.

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Abel Vesa <abelvesa@kernel.org>
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: David Airlie <airlied@gmail.com>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Isaac Scott <isaac.scott@ideasonboard.com>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: Robert Foss <rfoss@kernel.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: dri-devel@lists.freedesktop.org
Cc: imx@lists.linux.dev
Cc: kernel@dh-electronics.com
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-clk@vger.kernel.org
---
 drivers/clk/imx/clk-imx8mp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
index 516dbd170c8a3..2e61d340b8ab7 100644
--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -611,7 +611,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
 	hws[IMX8MP_CLK_MEDIA_MIPI_PHY1_REF] = imx8m_clk_hw_composite("media_mipi_phy1_ref", imx8mp_media_mipi_phy1_ref_sels, ccm_base + 0xbd80);
 	hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT);
 	hws[IMX8MP_CLK_MEDIA_CAM2_PIX] = imx8m_clk_hw_composite("media_cam2_pix", imx8mp_media_cam2_pix_sels, ccm_base + 0xbe80);
-	hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00);
+	hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite_bus_flags("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00, CLK_SET_RATE_PARENT);
 	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

[PATCH 2/2] drm: bridge: ldb: Configure LDB clock in .mode_set

[Marek Vasut]

The LDB serializer clock operate at either x7 or x14 rate of the input
LCDIFv3 scanout engine clock. Make sure the serializer clock and their
upstream Video PLL are configured early in .mode_set to the x7 or x14
rate of pixel clock, before LCDIFv3 .atomic_enable is called which would
configure the Video PLL to low x1 rate, which is unusable.

With this patch in place, the clock tree is correctly configured. The
example below is for a 71.1 MHz pixel clock panel, the LDB serializer
clock is then 497.7 MHz:

video_pll1_ref_sel                      1 1 0  24000000 0 0 50000
   video_pll1                           1 1 0 497700000 0 0 50000
      video_pll1_bypass                 1 1 0 497700000 0 0 50000
         video_pll1_out                 2 2 0 497700000 0 0 50000
            media_ldb                   1 1 0 497700000 0 0 50000
               media_ldb_root_clk       1 1 0 497700000 0 0 50000
            media_disp2_pix             1 1 0  71100000 0 0 50000
               media_disp2_pix_root_clk 1 1 0  71100000 0 0 50000

Signed-off-by: Marek Vasut <marex@denx.de>
---
Cc: Abel Vesa <abelvesa@kernel.org>
Cc: Andrzej Hajda <andrzej.hajda@intel.com>
Cc: David Airlie <airlied@gmail.com>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: Isaac Scott <isaac.scott@ideasonboard.com>
Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Michael Turquette <mturquette@baylibre.com>
Cc: Neil Armstrong <neil.armstrong@linaro.org>
Cc: Peng Fan <peng.fan@nxp.com>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: Robert Foss <rfoss@kernel.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Simona Vetter <simona@ffwll.ch>
Cc: Stephen Boyd <sboyd@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: dri-devel@lists.freedesktop.org
Cc: imx@lists.linux.dev
Cc: kernel@dh-electronics.com
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-clk@vger.kernel.org
---
 drivers/gpu/drm/bridge/fsl-ldb.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
index 0e4bac7dd04ff..a3a31467fcc85 100644
--- a/drivers/gpu/drm/bridge/fsl-ldb.c
+++ b/drivers/gpu/drm/bridge/fsl-ldb.c
@@ -278,6 +278,16 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
 	return MODE_OK;
 }
 
+static void fsl_ldb_mode_set(struct drm_bridge *bridge,
+			       const struct drm_display_mode *mode,
+			       const struct drm_display_mode *adj)
+{
+	struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
+	unsigned long requested_link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock);
+
+	clk_set_rate(fsl_ldb->clk, requested_link_freq);
+}
+
 static const struct drm_bridge_funcs funcs = {
 	.attach = fsl_ldb_attach,
 	.atomic_enable = fsl_ldb_atomic_enable,
@@ -287,6 +297,7 @@ static const struct drm_bridge_funcs funcs = {
 	.atomic_get_input_bus_fmts = fsl_ldb_atomic_get_input_bus_fmts,
 	.atomic_reset = drm_atomic_helper_bridge_reset,
 	.mode_valid = fsl_ldb_mode_valid,
+	.mode_set = fsl_ldb_mode_set,
 };
 
 static int fsl_ldb_probe(struct platform_device *pdev)
-- 
2.45.2

Re: [PATCH 2/2] drm: bridge: ldb: Configure LDB clock in .mode_set

[Isaac Scott]

On Wed, 2024-10-09 at 00:38 +0200, Marek Vasut wrote:
> The LDB serializer clock operate at either x7 or x14 rate of the
> input
> LCDIFv3 scanout engine clock. Make sure the serializer clock and
> their
> upstream Video PLL are configured early in .mode_set to the x7 or x14
> rate of pixel clock, before LCDIFv3 .atomic_enable is called which
> would
> configure the Video PLL to low x1 rate, which is unusable.
> 
> With this patch in place, the clock tree is correctly configured. The
> example below is for a 71.1 MHz pixel clock panel, the LDB serializer
> clock is then 497.7 MHz:

Awesome! Thank you for this, this seems to fix the regression and the
patches work as expected. I have tested both patches on v6.12-rc2 and
the display works well.

For both patches, 

Tested-by: Isaac Scott <isaac.scott@ideasonboard.com>
> 
> video_pll1_ref_sel                      1 1 0  24000000 0 0 50000
>    video_pll1                           1 1 0 497700000 0 0 50000
>       video_pll1_bypass                 1 1 0 497700000 0 0 50000
>          video_pll1_out                 2 2 0 497700000 0 0 50000
>             media_ldb                   1 1 0 497700000 0 0 50000
>                media_ldb_root_clk       1 1 0 497700000 0 0 50000
>             media_disp2_pix             1 1 0  71100000 0 0 50000
>                media_disp2_pix_root_clk 1 1 0  71100000 0 0 50000
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Abel Vesa <abelvesa@kernel.org>
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Isaac Scott <isaac.scott@ideasonboard.com>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Robert Foss <rfoss@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: imx@lists.linux.dev
> Cc: kernel@dh-electronics.com
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-clk@vger.kernel.org
> ---
>  drivers/gpu/drm/bridge/fsl-ldb.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c
> b/drivers/gpu/drm/bridge/fsl-ldb.c
> index 0e4bac7dd04ff..a3a31467fcc85 100644
> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> @@ -278,6 +278,16 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
>  	return MODE_OK;
>  }
>  
> +static void fsl_ldb_mode_set(struct drm_bridge *bridge,
> +			       const struct drm_display_mode *mode,
> +			       const struct drm_display_mode *adj)
> +{
> +	struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
> +	unsigned long requested_link_freq =
> fsl_ldb_link_frequency(fsl_ldb, mode->clock);
> +
> +	clk_set_rate(fsl_ldb->clk, requested_link_freq);
> +}
> +
>  static const struct drm_bridge_funcs funcs = {
>  	.attach = fsl_ldb_attach,
>  	.atomic_enable = fsl_ldb_atomic_enable,
> @@ -287,6 +297,7 @@ static const struct drm_bridge_funcs funcs = {
>  	.atomic_get_input_bus_fmts =
> fsl_ldb_atomic_get_input_bus_fmts,
>  	.atomic_reset = drm_atomic_helper_bridge_reset,
>  	.mode_valid = fsl_ldb_mode_valid,
> +	.mode_set = fsl_ldb_mode_set,
>  };
>  
>  static int fsl_ldb_probe(struct platform_device *pdev)

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

[Abel Vesa]

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

Any fixes tag needed ?

Otherwise, LGTM:

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

> ---
> Cc: Abel Vesa <abelvesa@kernel.org>
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Isaac Scott <isaac.scott@ideasonboard.com>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Robert Foss <rfoss@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: imx@lists.linux.dev
> Cc: kernel@dh-electronics.com
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-clk@vger.kernel.org
> ---
>  drivers/clk/imx/clk-imx8mp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
> index 516dbd170c8a3..2e61d340b8ab7 100644
> --- a/drivers/clk/imx/clk-imx8mp.c
> +++ b/drivers/clk/imx/clk-imx8mp.c
> @@ -611,7 +611,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
>  	hws[IMX8MP_CLK_MEDIA_MIPI_PHY1_REF] = imx8m_clk_hw_composite("media_mipi_phy1_ref", imx8mp_media_mipi_phy1_ref_sels, ccm_base + 0xbd80);
>  	hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT);
>  	hws[IMX8MP_CLK_MEDIA_CAM2_PIX] = imx8m_clk_hw_composite("media_cam2_pix", imx8mp_media_cam2_pix_sels, ccm_base + 0xbe80);
> -	hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00);
> +	hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite_bus_flags("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00, CLK_SET_RATE_PARENT);
>  	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
> 

Re: [PATCH 2/2] drm: bridge: ldb: Configure LDB clock in .mode_set

[Marek Vasut]

On 10/9/24 12:27 PM, Isaac Scott wrote:
> On Wed, 2024-10-09 at 00:38 +0200, Marek Vasut wrote:
>> The LDB serializer clock operate at either x7 or x14 rate of the
>> input
>> LCDIFv3 scanout engine clock. Make sure the serializer clock and
>> their
>> upstream Video PLL are configured early in .mode_set to the x7 or x14
>> rate of pixel clock, before LCDIFv3 .atomic_enable is called which
>> would
>> configure the Video PLL to low x1 rate, which is unusable.
>>
>> With this patch in place, the clock tree is correctly configured. The
>> example below is for a 71.1 MHz pixel clock panel, the LDB serializer
>> clock is then 497.7 MHz:
> 
> Awesome! Thank you for this, this seems to fix the regression and the
> patches work as expected. I have tested both patches on v6.12-rc2 and
> the display works well.
> 
> For both patches,
> 
> Tested-by: Isaac Scott <isaac.scott@ideasonboard.com>
Thank you for testing, but this patch feels too much like a feature 
development to me. Does the DT tweak I suggested also fix your issue? If 
so, I would really like that DT tweak to be the fix for current release 
and these two patches be feature development for 6.13 cycle. What do you 
think ?

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

[Marek Vasut]

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

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

[Liu Ying]

On 10/09/2024, Marek Vasut wrote:
> The media_ldb_root_clk supply LDB serializer. These clock are usually
> shared with the LCDIFv3 pixel clock and supplied by the Video PLL on
> i.MX8MP, but the LDB clock run at either x7 or x14 rate of the LCDIFv3
> pixel clock. Allow the LDB to reconfigure Video PLL as needed, as that
> results in accurate serializer clock.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Abel Vesa <abelvesa@kernel.org>
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Isaac Scott <isaac.scott@ideasonboard.com>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Robert Foss <rfoss@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: imx@lists.linux.dev
> Cc: kernel@dh-electronics.com
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-clk@vger.kernel.org
> ---
>  drivers/clk/imx/clk-imx8mp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
> index 516dbd170c8a3..2e61d340b8ab7 100644
> --- a/drivers/clk/imx/clk-imx8mp.c
> +++ b/drivers/clk/imx/clk-imx8mp.c
> @@ -611,7 +611,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
>  	hws[IMX8MP_CLK_MEDIA_MIPI_PHY1_REF] = imx8m_clk_hw_composite("media_mipi_phy1_ref", imx8mp_media_mipi_phy1_ref_sels, ccm_base + 0xbd80);
>  	hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT);
>  	hws[IMX8MP_CLK_MEDIA_CAM2_PIX] = imx8m_clk_hw_composite("media_cam2_pix", imx8mp_media_cam2_pix_sels, ccm_base + 0xbe80);
> -	hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00);
> +	hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite_bus_flags("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00, CLK_SET_RATE_PARENT);

This patch would cause the below in-flight LDB bridge driver
patch[1] fail to do display mode validation upon display modes
read from LVDS to HDMI converter IT6263's DDC I2C bus.
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);

-- 
Regards,
Liu Ying

Re: [PATCH 2/2] drm: bridge: ldb: Configure LDB clock in .mode_set

[Liu Ying]

On 10/09/2024, Marek Vasut wrote:
> The LDB serializer clock operate at either x7 or x14 rate of the input

Isn't it either x7 or 3.5x?

s/operate/operates/

> LCDIFv3 scanout engine clock. Make sure the serializer clock and their
> upstream Video PLL are configured early in .mode_set to the x7 or x14
> rate of pixel clock, before LCDIFv3 .atomic_enable is called which would
> configure the Video PLL to low x1 rate, which is unusable.
> 
> With this patch in place, the clock tree is correctly configured. The
> example below is for a 71.1 MHz pixel clock panel, the LDB serializer
> clock is then 497.7 MHz:
> 
> video_pll1_ref_sel                      1 1 0  24000000 0 0 50000
>    video_pll1                           1 1 0 497700000 0 0 50000
>       video_pll1_bypass                 1 1 0 497700000 0 0 50000
>          video_pll1_out                 2 2 0 497700000 0 0 50000
>             media_ldb                   1 1 0 497700000 0 0 50000
>                media_ldb_root_clk       1 1 0 497700000 0 0 50000
>             media_disp2_pix             1 1 0  71100000 0 0 50000
>                media_disp2_pix_root_clk 1 1 0  71100000 0 0 50000
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
> Cc: Abel Vesa <abelvesa@kernel.org>
> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> Cc: David Airlie <airlied@gmail.com>
> Cc: Fabio Estevam <festevam@gmail.com>
> Cc: Isaac Scott <isaac.scott@ideasonboard.com>
> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Cc: Maxime Ripard <mripard@kernel.org>
> Cc: Michael Turquette <mturquette@baylibre.com>
> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> Cc: Peng Fan <peng.fan@nxp.com>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> Cc: Robert Foss <rfoss@kernel.org>
> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> Cc: Shawn Guo <shawnguo@kernel.org>
> Cc: Simona Vetter <simona@ffwll.ch>
> Cc: Stephen Boyd <sboyd@kernel.org>
> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> Cc: dri-devel@lists.freedesktop.org
> Cc: imx@lists.linux.dev
> Cc: kernel@dh-electronics.com
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-clk@vger.kernel.org
> ---
>  drivers/gpu/drm/bridge/fsl-ldb.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
> index 0e4bac7dd04ff..a3a31467fcc85 100644
> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
> @@ -278,6 +278,16 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
>  	return MODE_OK;
>  }
>  
> +static void fsl_ldb_mode_set(struct drm_bridge *bridge,
> +			       const struct drm_display_mode *mode,
> +			       const struct drm_display_mode *adj)
> +{
> +	struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
> +	unsigned long requested_link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock);
> +
> +	clk_set_rate(fsl_ldb->clk, requested_link_freq);

The mode_set callback won't be called when only crtc_state->active
is changed from false to true in an atomic commit, e.g., blanking
the emulated fbdev first and then unblanking it.  So, in this case,
the clk_set_rate() in fsl_ldb_atomic_enable() is still called after
those from mxsfb_kms or lcdif_kms.

Also, it doesn't look neat to call clk_set_rate() from both mode_set
callback and atomic_enable callback.

The idea is to assign a reasonable PLL clock rate in DT to make
display drivers' life easier, especially for i.MX8MP where LDB,
Samsung MIPI DSI may use a single PLL at the same time.

> +}
> +
>  static const struct drm_bridge_funcs funcs = {
>  	.attach = fsl_ldb_attach,
>  	.atomic_enable = fsl_ldb_atomic_enable,
> @@ -287,6 +297,7 @@ static const struct drm_bridge_funcs funcs = {
>  	.atomic_get_input_bus_fmts = fsl_ldb_atomic_get_input_bus_fmts,
>  	.atomic_reset = drm_atomic_helper_bridge_reset,
>  	.mode_valid = fsl_ldb_mode_valid,
> +	.mode_set = fsl_ldb_mode_set,
>  };
>  
>  static int fsl_ldb_probe(struct platform_device *pdev)

-- 
Regards,
Liu Ying

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

[Marek Vasut]

On 10/10/24 7:22 AM, Liu Ying wrote:
> On 10/09/2024, Marek Vasut wrote:
>> The media_ldb_root_clk supply LDB serializer. These clock are usually
>> shared with the LCDIFv3 pixel clock and supplied by the Video PLL on
>> i.MX8MP, but the LDB clock run at either x7 or x14 rate of the LCDIFv3
>> pixel clock. Allow the LDB to reconfigure Video PLL as needed, as that
>> results in accurate serializer clock.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>> Cc: Abel Vesa <abelvesa@kernel.org>
>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>> Cc: David Airlie <airlied@gmail.com>
>> Cc: Fabio Estevam <festevam@gmail.com>
>> Cc: Isaac Scott <isaac.scott@ideasonboard.com>
>> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
>> Cc: Jonas Karlman <jonas@kwiboo.se>
>> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> Cc: Maxime Ripard <mripard@kernel.org>
>> Cc: Michael Turquette <mturquette@baylibre.com>
>> Cc: Neil Armstrong <neil.armstrong@linaro.org>
>> Cc: Peng Fan <peng.fan@nxp.com>
>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
>> Cc: Robert Foss <rfoss@kernel.org>
>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>> Cc: Shawn Guo <shawnguo@kernel.org>
>> Cc: Simona Vetter <simona@ffwll.ch>
>> Cc: Stephen Boyd <sboyd@kernel.org>
>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>> Cc: dri-devel@lists.freedesktop.org
>> Cc: imx@lists.linux.dev
>> Cc: kernel@dh-electronics.com
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-clk@vger.kernel.org
>> ---
>>   drivers/clk/imx/clk-imx8mp.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
>> index 516dbd170c8a3..2e61d340b8ab7 100644
>> --- a/drivers/clk/imx/clk-imx8mp.c
>> +++ b/drivers/clk/imx/clk-imx8mp.c
>> @@ -611,7 +611,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
>>   	hws[IMX8MP_CLK_MEDIA_MIPI_PHY1_REF] = imx8m_clk_hw_composite("media_mipi_phy1_ref", imx8mp_media_mipi_phy1_ref_sels, ccm_base + 0xbd80);
>>   	hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT);
>>   	hws[IMX8MP_CLK_MEDIA_CAM2_PIX] = imx8m_clk_hw_composite("media_cam2_pix", imx8mp_media_cam2_pix_sels, ccm_base + 0xbe80);
>> -	hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00);
>> +	hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite_bus_flags("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00, CLK_SET_RATE_PARENT);
> 
> This patch would cause the below in-flight LDB bridge driver
> patch[1] fail to do display mode validation upon display modes
> read from LVDS to HDMI converter IT6263's DDC I2C bus.

Why ?

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.

Re: [PATCH 2/2] drm: bridge: ldb: Configure LDB clock in .mode_set

[Marek Vasut]

On 10/10/24 9:15 AM, Liu Ying wrote:
> On 10/09/2024, Marek Vasut wrote:
>> The LDB serializer clock operate at either x7 or x14 rate of the input
> 
> Isn't it either x7 or 3.5x?

Is it 3.5 for the dual-link LVDS ?
I don't have such a panel right now to test.

[...]

>> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
>> index 0e4bac7dd04ff..a3a31467fcc85 100644
>> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
>> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
>> @@ -278,6 +278,16 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
>>   	return MODE_OK;
>>   }
>>   
>> +static void fsl_ldb_mode_set(struct drm_bridge *bridge,
>> +			       const struct drm_display_mode *mode,
>> +			       const struct drm_display_mode *adj)
>> +{
>> +	struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
>> +	unsigned long requested_link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock);
>> +
>> +	clk_set_rate(fsl_ldb->clk, requested_link_freq);
> 
> The mode_set callback won't be called when only crtc_state->active
> is changed from false to true in an atomic commit, e.g., blanking
> the emulated fbdev first and then unblanking it.  So, in this case,
> the clk_set_rate() in fsl_ldb_atomic_enable() is still called after
> those from mxsfb_kms or lcdif_kms.
> 
> Also, it doesn't look neat to call clk_set_rate() from both mode_set
> callback and atomic_enable callback.

I agree the mode_set callback is not the best place for this.
Do you know of a better callback where to do this ? I couldn't find one.

> The idea is to assign a reasonable PLL clock rate in DT to make
> display drivers' life easier, especially for i.MX8MP where LDB,
> Samsung MIPI DSI may use a single PLL at the same time.
I would really like to avoid setting arbitrary clock in DT, esp. if it 
can be avoided. And it surely can be avoided for this simple use case.

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

[Liu Ying]

On 10/11/2024, Marek Vasut wrote:
> On 10/10/24 7:22 AM, Liu Ying wrote:
>> On 10/09/2024, Marek Vasut wrote:
>>> The media_ldb_root_clk supply LDB serializer. These clock are usually
>>> shared with the LCDIFv3 pixel clock and supplied by the Video PLL on
>>> i.MX8MP, but the LDB clock run at either x7 or x14 rate of the LCDIFv3
>>> pixel clock. Allow the LDB to reconfigure Video PLL as needed, as that
>>> results in accurate serializer clock.
>>>
>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>> ---
>>> Cc: Abel Vesa <abelvesa@kernel.org>
>>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>>> Cc: David Airlie <airlied@gmail.com>
>>> Cc: Fabio Estevam <festevam@gmail.com>
>>> Cc: Isaac Scott <isaac.scott@ideasonboard.com>
>>> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
>>> Cc: Jonas Karlman <jonas@kwiboo.se>
>>> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Cc: Maxime Ripard <mripard@kernel.org>
>>> Cc: Michael Turquette <mturquette@baylibre.com>
>>> Cc: Neil Armstrong <neil.armstrong@linaro.org>
>>> Cc: Peng Fan <peng.fan@nxp.com>
>>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
>>> Cc: Robert Foss <rfoss@kernel.org>
>>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>>> Cc: Shawn Guo <shawnguo@kernel.org>
>>> Cc: Simona Vetter <simona@ffwll.ch>
>>> Cc: Stephen Boyd <sboyd@kernel.org>
>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>> Cc: dri-devel@lists.freedesktop.org
>>> Cc: imx@lists.linux.dev
>>> Cc: kernel@dh-electronics.com
>>> Cc: linux-arm-kernel@lists.infradead.org
>>> Cc: linux-clk@vger.kernel.org
>>> ---
>>>   drivers/clk/imx/clk-imx8mp.c | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
>>> index 516dbd170c8a3..2e61d340b8ab7 100644
>>> --- a/drivers/clk/imx/clk-imx8mp.c
>>> +++ b/drivers/clk/imx/clk-imx8mp.c
>>> @@ -611,7 +611,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
>>>       hws[IMX8MP_CLK_MEDIA_MIPI_PHY1_REF] = imx8m_clk_hw_composite("media_mipi_phy1_ref", imx8mp_media_mipi_phy1_ref_sels, ccm_base + 0xbd80);
>>>       hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT);
>>>       hws[IMX8MP_CLK_MEDIA_CAM2_PIX] = imx8m_clk_hw_composite("media_cam2_pix", imx8mp_media_cam2_pix_sels, ccm_base + 0xbe80);
>>> -    hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00);
>>> +    hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite_bus_flags("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00, CLK_SET_RATE_PARENT);
>>
>> This patch would cause the below in-flight LDB bridge driver
>> patch[1] fail to do display mode validation upon display modes
>> read from LVDS to HDMI converter IT6263's DDC I2C bus.
> 
> Why ?

Mode validation is affected only for dual LVDS link mode.
For single LVDS link mode, this patch does open more display
modes read from the DDC I2C bus.  The reason behind is that
LVDS serial clock rate/pixel clock rate = 3.5 for dual LVDS
link mode, while it's 7 for single LVDS link mode.

In my system, "video_pll1" clock rate is assigned to 1.0395GHz
in imx8mp.dtsi.  For 1920x1080-60.00Hz with 148.5MHz pixel
clock rate, "media_ldb" clock rate is 519.75MHz and
"media_disp2_pix" clock rate is 148.5MHz, which is fine for
dual LVDS link mode(x3.5).  For newly opened up 1920x1080-59.94Hz
with 148.352MHz pixel clock rate, "video_pll1" clock rate will
be changed to 519.232MHz, "media_ldb" clock rate is 519.232MHz
and "media_disp2_pix" clock rate is wrongly set to 519.232MHz
too because "media_disp2_pix" clock cannot handle the 3.5
division ratio from "video_pll1_out" clock running at
519.232MHz.  See the below clk_summary.

    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.

-- 
Regards,
Liu Ying

Re: [PATCH 2/2] drm: bridge: ldb: Configure LDB clock in .mode_set

[Liu Ying]

On 10/11/2024, Marek Vasut wrote:
> On 10/10/24 9:15 AM, Liu Ying wrote:
>> On 10/09/2024, Marek Vasut wrote:
>>> The LDB serializer clock operate at either x7 or x14 rate of the input
>>
>> Isn't it either x7 or 3.5x?
> 
> Is it 3.5 for the dual-link LVDS ?

Yes.

static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb, int clock)  
{                                                                                
        if (fsl_ldb_is_dual(fsl_ldb))                                            
                return clock * 3500;                                             
        else                                                                     
                return clock * 7000;                                             
}    

> I don't have such a panel right now to test.

You can add a panel DT node for test to see the relationship
between the clocks, without a real dual-link LVDS panel.

> 
> [...]
> 
>>> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
>>> index 0e4bac7dd04ff..a3a31467fcc85 100644
>>> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
>>> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
>>> @@ -278,6 +278,16 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
>>>       return MODE_OK;
>>>   }
>>>   +static void fsl_ldb_mode_set(struct drm_bridge *bridge,
>>> +                   const struct drm_display_mode *mode,
>>> +                   const struct drm_display_mode *adj)
>>> +{
>>> +    struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
>>> +    unsigned long requested_link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock);
>>> +
>>> +    clk_set_rate(fsl_ldb->clk, requested_link_freq);
>>
>> The mode_set callback won't be called when only crtc_state->active
>> is changed from false to true in an atomic commit, e.g., blanking
>> the emulated fbdev first and then unblanking it.  So, in this case,
>> the clk_set_rate() in fsl_ldb_atomic_enable() is still called after
>> those from mxsfb_kms or lcdif_kms.
>>
>> Also, it doesn't look neat to call clk_set_rate() from both mode_set
>> callback and atomic_enable callback.
> 
> I agree the mode_set callback is not the best place for this.
> Do you know of a better callback where to do this ? I couldn't find one.

A wild idea is to change the order between the CRTC atomic_enable
callback and the bridge one by implementing your own
atomic_commit_tail...  I don't think there is any place to do this
other than atomic_enable callback.

Anyway, I don't think it is necessary to manage the clk_set_rate()
function calls between this driver and mxsfb_kms or lcdif_kms
because "video_pll1" clock rate is supposed to be assigned in DT...

> 
>> The idea is to assign a reasonable PLL clock rate in DT to make
>> display drivers' life easier, especially for i.MX8MP where LDB,
>> Samsung MIPI DSI may use a single PLL at the same time.
> I would really like to avoid setting arbitrary clock in DT, esp. if it can be avoided. And it surely can be avoided for this simple use case.

... just like I said in patch 1/2, "video_pll1" clock rate needs
to be x2 "media_ldb" clock rate for dual LVDS link mode. Without
an assigned "video_pll1" clock rate in DT, this driver cannot
achieve that.  And, the i.MX8MP LDB + Samsung MIPI DSI case is
not simple considering using one single PLL and display modes
read from EDID.  

-- 
Regards,
Liu Ying

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

[Marek Vasut]

On 10/11/24 8:18 AM, Liu Ying wrote:
> On 10/11/2024, Marek Vasut wrote:
>> On 10/10/24 7:22 AM, Liu Ying wrote:
>>> On 10/09/2024, Marek Vasut wrote:
>>>> The media_ldb_root_clk supply LDB serializer. These clock are usually
>>>> shared with the LCDIFv3 pixel clock and supplied by the Video PLL on
>>>> i.MX8MP, but the LDB clock run at either x7 or x14 rate of the LCDIFv3
>>>> pixel clock. Allow the LDB to reconfigure Video PLL as needed, as that
>>>> results in accurate serializer clock.
>>>>
>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>> ---
>>>> Cc: Abel Vesa <abelvesa@kernel.org>
>>>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>>>> Cc: David Airlie <airlied@gmail.com>
>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>> Cc: Isaac Scott <isaac.scott@ideasonboard.com>
>>>> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
>>>> Cc: Jonas Karlman <jonas@kwiboo.se>
>>>> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Cc: Maxime Ripard <mripard@kernel.org>
>>>> Cc: Michael Turquette <mturquette@baylibre.com>
>>>> Cc: Neil Armstrong <neil.armstrong@linaro.org>
>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
>>>> Cc: Robert Foss <rfoss@kernel.org>
>>>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>>>> Cc: Shawn Guo <shawnguo@kernel.org>
>>>> Cc: Simona Vetter <simona@ffwll.ch>
>>>> Cc: Stephen Boyd <sboyd@kernel.org>
>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>> Cc: dri-devel@lists.freedesktop.org
>>>> Cc: imx@lists.linux.dev
>>>> Cc: kernel@dh-electronics.com
>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>> Cc: linux-clk@vger.kernel.org
>>>> ---
>>>>    drivers/clk/imx/clk-imx8mp.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
>>>> index 516dbd170c8a3..2e61d340b8ab7 100644
>>>> --- a/drivers/clk/imx/clk-imx8mp.c
>>>> +++ b/drivers/clk/imx/clk-imx8mp.c
>>>> @@ -611,7 +611,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
>>>>        hws[IMX8MP_CLK_MEDIA_MIPI_PHY1_REF] = imx8m_clk_hw_composite("media_mipi_phy1_ref", imx8mp_media_mipi_phy1_ref_sels, ccm_base + 0xbd80);
>>>>        hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT);
>>>>        hws[IMX8MP_CLK_MEDIA_CAM2_PIX] = imx8m_clk_hw_composite("media_cam2_pix", imx8mp_media_cam2_pix_sels, ccm_base + 0xbe80);
>>>> -    hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00);
>>>> +    hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite_bus_flags("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00, CLK_SET_RATE_PARENT);
>>>
>>> This patch would cause the below in-flight LDB bridge driver
>>> patch[1] fail to do display mode validation upon display modes
>>> read from LVDS to HDMI converter IT6263's DDC I2C bus.
>>
>> Why ?
> 
> Mode validation is affected only for dual LVDS link mode.
> For single LVDS link mode, this patch does open more display
> modes read from the DDC I2C bus.  The reason behind is that
> LVDS serial clock rate/pixel clock rate = 3.5 for dual LVDS
> link mode, while it's 7 for single LVDS link mode.
> 
> In my system, "video_pll1" clock rate is assigned to 1.0395GHz
> in imx8mp.dtsi.  For 1920x1080-60.00Hz with 148.5MHz pixel
> clock rate, "media_ldb" clock rate is 519.75MHz and
> "media_disp2_pix" clock rate is 148.5MHz, which is fine for
> dual LVDS link mode(x3.5).  For newly opened up 1920x1080-59.94Hz
> with 148.352MHz pixel clock rate, "video_pll1" clock rate will
> be changed to 519.232MHz, "media_ldb" clock rate is 519.232MHz
> and "media_disp2_pix" clock rate is wrongly set to 519.232MHz
> too because "media_disp2_pix" clock cannot handle the 3.5
> division ratio from "video_pll1_out" clock running at
> 519.232MHz.  See the below clk_summary.

Shouldn't this patch help exactly with that ?

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.

Re: [PATCH 2/2] drm: bridge: ldb: Configure LDB clock in .mode_set

[Marek Vasut]

On 10/11/24 8:49 AM, Liu Ying wrote:
> On 10/11/2024, Marek Vasut wrote:
>> On 10/10/24 9:15 AM, Liu Ying wrote:
>>> On 10/09/2024, Marek Vasut wrote:
>>>> The LDB serializer clock operate at either x7 or x14 rate of the input
>>>
>>> Isn't it either x7 or 3.5x?
>>
>> Is it 3.5 for the dual-link LVDS ?
> 
> Yes.
> 
> static unsigned long fsl_ldb_link_frequency(struct fsl_ldb *fsl_ldb, int clock)
> {
>          if (fsl_ldb_is_dual(fsl_ldb))
>                  return clock * 3500;
>          else
>                  return clock * 7000;
> }
> 
>> I don't have such a panel right now to test.
> 
> You can add a panel DT node for test to see the relationship
> between the clocks, without a real dual-link LVDS panel.

I'll take your word for this.

>> [...]
>>
>>>> diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
>>>> index 0e4bac7dd04ff..a3a31467fcc85 100644
>>>> --- a/drivers/gpu/drm/bridge/fsl-ldb.c
>>>> +++ b/drivers/gpu/drm/bridge/fsl-ldb.c
>>>> @@ -278,6 +278,16 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
>>>>        return MODE_OK;
>>>>    }
>>>>    +static void fsl_ldb_mode_set(struct drm_bridge *bridge,
>>>> +                   const struct drm_display_mode *mode,
>>>> +                   const struct drm_display_mode *adj)
>>>> +{
>>>> +    struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
>>>> +    unsigned long requested_link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock);
>>>> +
>>>> +    clk_set_rate(fsl_ldb->clk, requested_link_freq);
>>>
>>> The mode_set callback won't be called when only crtc_state->active
>>> is changed from false to true in an atomic commit, e.g., blanking
>>> the emulated fbdev first and then unblanking it.  So, in this case,
>>> the clk_set_rate() in fsl_ldb_atomic_enable() is still called after
>>> those from mxsfb_kms or lcdif_kms.
>>>
>>> Also, it doesn't look neat to call clk_set_rate() from both mode_set
>>> callback and atomic_enable callback.
>>
>> I agree the mode_set callback is not the best place for this.
>> Do you know of a better callback where to do this ? I couldn't find one.
> 
> A wild idea is to change the order between the CRTC atomic_enable
> callback and the bridge one by implementing your own
> atomic_commit_tail...  I don't think there is any place to do this
> other than atomic_enable callback.

I will give that a try, thanks.

> Anyway, I don't think it is necessary to manage the clk_set_rate()
> function calls between this driver and mxsfb_kms or lcdif_kms
> because "video_pll1" clock rate is supposed to be assigned in DT...

I disagree with this part. I believe the assignment of clock in DT is 
only a temporary workaround which should be removed. The drivers should 
be able to figure out and set the clock tree configuration.

>>> The idea is to assign a reasonable PLL clock rate in DT to make
>>> display drivers' life easier, especially for i.MX8MP where LDB,
>>> Samsung MIPI DSI may use a single PLL at the same time.
>> I would really like to avoid setting arbitrary clock in DT, esp. if it can be avoided. And it surely can be avoided for this simple use case.
> 
> ... just like I said in patch 1/2, "video_pll1" clock rate needs
> to be x2 "media_ldb" clock rate for dual LVDS link mode. Without
> an assigned "video_pll1" clock rate in DT, this driver cannot
> achieve that.

This is something the LDB driver can infer from DT and configure the 
clock tree accordingly.

> And, the i.MX8MP LDB + Samsung MIPI DSI case is
> not simple considering using one single PLL and display modes
> read from EDID.
You could use separate PLLs for each LCDIF scanout engine in such a 
deployment, I already ran into that, so I am aware of it. That is 
probably the best way out of such a problem, esp. if accurate pixel 
clock are the requirement.

[...]

Re: [PATCH 2/2] drm: bridge: ldb: Configure LDB clock in .mode_set

[Liu Ying]

On 10/13/2024, Marek Vasut wrote:
> On 10/11/24 8:49 AM, Liu Ying wrote:
>> On 10/11/2024, Marek Vasut wrote:
>>> On 10/10/24 9:15 AM, Liu Ying wrote:
>>>> On 10/09/2024, Marek Vasut wrote:

[...]

>> Anyway, I don't think it is necessary to manage the clk_set_rate()
>> function calls between this driver and mxsfb_kms or lcdif_kms
>> because "video_pll1" clock rate is supposed to be assigned in DT...
> 
> I disagree with this part. I believe the assignment of clock in DT is only a temporary workaround which should be removed. The drivers should be able to figure out and set the clock tree configuration.

I think the clock rate assignment in DT is still needed.
A good reason is that again we need to share one video PLL
between MIPI DSI and LDB display pipelines for i.MX8MP.

> 
>>>> The idea is to assign a reasonable PLL clock rate in DT to make
>>>> display drivers' life easier, especially for i.MX8MP where LDB,
>>>> Samsung MIPI DSI may use a single PLL at the same time.
>>> I would really like to avoid setting arbitrary clock in DT, esp. if it can be avoided. And it surely can be avoided for this simple use case.
>>
>> ... just like I said in patch 1/2, "video_pll1" clock rate needs
>> to be x2 "media_ldb" clock rate for dual LVDS link mode. Without
>> an assigned "video_pll1" clock rate in DT, this driver cannot
>> achieve that.
> 
> This is something the LDB driver can infer from DT and configure the clock tree accordingly.

Well, the LDB driver only controls the "ldb" clock rate. It doesn't
magically set the parent "video_pll1" clock's rate to 2x it's rate,
unless the driver gets "video_pll1_out" clock by calling
clk_get_parent() and directly controls the PLL clock rate which
doesn't look neat. 

> 
>> And, the i.MX8MP LDB + Samsung MIPI DSI case is
>> not simple considering using one single PLL and display modes
>> read from EDID.
> You could use separate PLLs for each LCDIF scanout engine in such a deployment, I already ran into that, so I am aware of it. That is probably the best way out of such a problem, esp. if accurate pixel clock are the requirement.

I cannot use separate PLLs for the i.MX8MP LDB and Samsung MIPI
DSI display pipelines on i.MX8MP EVK, because the PLLs are limited
resources and we are running out of it.  Because LDB needs the pixel
clock and LVDS serial clock to be derived from a same PLL, the only
valid PLLs(see imx8mp_media_disp_pix_sels[] and
imx8mp_media_ldb_sels[]) are "video_pll1_out", "audio_pll2_out",
"sys_pll2_1000m" and "sys_pll1_800m".  All are used as either audio
clock or system clocks on i.MX8MP EVK, except "video_pll1_out".

You probably may use separate PLLs for a particular i.MX8MP platform
with limited features, but not for i.MX8MP EVK which is supposed to
evaluate all SoC features.

> 
> [...]

-- 
Regards,
Liu Ying

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

[Liu Ying]

On 10/13/2024, Marek Vasut wrote:
> On 10/11/24 8:18 AM, Liu Ying wrote:
>> On 10/11/2024, Marek Vasut wrote:
>>> On 10/10/24 7:22 AM, Liu Ying wrote:
>>>> On 10/09/2024, Marek Vasut wrote:
>>>>> The media_ldb_root_clk supply LDB serializer. These clock are usually
>>>>> shared with the LCDIFv3 pixel clock and supplied by the Video PLL on
>>>>> i.MX8MP, but the LDB clock run at either x7 or x14 rate of the LCDIFv3
>>>>> pixel clock. Allow the LDB to reconfigure Video PLL as needed, as that
>>>>> results in accurate serializer clock.
>>>>>
>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>> ---
>>>>> Cc: Abel Vesa <abelvesa@kernel.org>
>>>>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>>>>> Cc: David Airlie <airlied@gmail.com>
>>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>>> Cc: Isaac Scott <isaac.scott@ideasonboard.com>
>>>>> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
>>>>> Cc: Jonas Karlman <jonas@kwiboo.se>
>>>>> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>> Cc: Maxime Ripard <mripard@kernel.org>
>>>>> Cc: Michael Turquette <mturquette@baylibre.com>
>>>>> Cc: Neil Armstrong <neil.armstrong@linaro.org>
>>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
>>>>> Cc: Robert Foss <rfoss@kernel.org>
>>>>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>>>>> Cc: Shawn Guo <shawnguo@kernel.org>
>>>>> Cc: Simona Vetter <simona@ffwll.ch>
>>>>> Cc: Stephen Boyd <sboyd@kernel.org>
>>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>>> Cc: dri-devel@lists.freedesktop.org
>>>>> Cc: imx@lists.linux.dev
>>>>> Cc: kernel@dh-electronics.com
>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>> Cc: linux-clk@vger.kernel.org
>>>>> ---
>>>>>    drivers/clk/imx/clk-imx8mp.c | 2 +-
>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
>>>>> index 516dbd170c8a3..2e61d340b8ab7 100644
>>>>> --- a/drivers/clk/imx/clk-imx8mp.c
>>>>> +++ b/drivers/clk/imx/clk-imx8mp.c
>>>>> @@ -611,7 +611,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
>>>>>        hws[IMX8MP_CLK_MEDIA_MIPI_PHY1_REF] = imx8m_clk_hw_composite("media_mipi_phy1_ref", imx8mp_media_mipi_phy1_ref_sels, ccm_base + 0xbd80);
>>>>>        hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT);
>>>>>        hws[IMX8MP_CLK_MEDIA_CAM2_PIX] = imx8m_clk_hw_composite("media_cam2_pix", imx8mp_media_cam2_pix_sels, ccm_base + 0xbe80);
>>>>> -    hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00);
>>>>> +    hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite_bus_flags("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00, CLK_SET_RATE_PARENT);
>>>>
>>>> This patch would cause the below in-flight LDB bridge driver
>>>> patch[1] fail to do display mode validation upon display modes
>>>> read from LVDS to HDMI converter IT6263's DDC I2C bus.
>>>
>>> Why ?
>>
>> Mode validation is affected only for dual LVDS link mode.
>> For single LVDS link mode, this patch does open more display
>> modes read from the DDC I2C bus.  The reason behind is that
>> LVDS serial clock rate/pixel clock rate = 3.5 for dual LVDS
>> link mode, while it's 7 for single LVDS link mode.
>>
>> In my system, "video_pll1" clock rate is assigned to 1.0395GHz
>> in imx8mp.dtsi.  For 1920x1080-60.00Hz with 148.5MHz pixel
>> clock rate, "media_ldb" clock rate is 519.75MHz and
>> "media_disp2_pix" clock rate is 148.5MHz, which is fine for
>> dual LVDS link mode(x3.5).  For newly opened up 1920x1080-59.94Hz
>> with 148.352MHz pixel clock rate, "video_pll1" clock rate will
>> be changed to 519.232MHz, "media_ldb" clock rate is 519.232MHz
>> and "media_disp2_pix" clock rate is wrongly set to 519.232MHz
>> too because "media_disp2_pix" clock cannot handle the 3.5
>> division ratio from "video_pll1_out" clock running at
>> 519.232MHz.  See the below clk_summary.
> 
> Shouldn't this patch help exactly with that ?

No, it doesn't help but only makes clk_round_rate() called in
LDB driver's .mode_valid() against 148.352MHz return 148.352MHz
which allows the unexpected 1920x1080-59.94Hz display mode.

> 
> It should allow you to set video_pll1_out to whatever is necessary by LDB first, fixate that frequency, and the LCDIFv3 would then be forced to use /7 divider from faster Video PLL1 , right ?

Yes, it allows that for single-link LVDS use cases.
And, __no__, for dual-link LVDS use cases because the
video_pll1_out clock rate needs to be 2x the LVDS serial clock
rate. 

> 
>>      video_pll1_ref_sel               1       1        0        24000000    0          0     50000      Y      deviceless                      no_connection_id
>>         video_pll1                    1       1        0        519232000   0          0     50000      Y         deviceless                      no_connection_id
>>            video_pll1_bypass          1       1        0        519232000   0          0     50000      Y            deviceless                      no_connection_id
>>               video_pll1_out          2       2        0        519232000   0          0     50000      Y               deviceless                      no_connection_id
>>                  media_ldb            1       1        0        519232000   0          0     50000      Y                  deviceless                      no_connection_id
>>                     media_ldb_root_clk 1       1        0        519232000   0          0     50000      Y                     32ec0000.blk-ctrl:bridge@5c     ldb
>>                                                                                                                               deviceless                      no_connection_id
>>                  media_disp1_pix      0       0        0        129808000   0          0     50000      N                  deviceless                      no_connection_id
>>                     media_disp1_pix_root_clk 0       0        0        129808000   0          0     50000      N                     32e80000.display-controller     pix
>>                                                                                                                               32ec0000.blk-ctrl               disp1
>>                                                                                                                               deviceless                      no_connection_id
>>                  media_disp2_pix      1       1        0        519232000   0          0     50000      Y                  deviceless                      no_connection_id
>>                     media_disp2_pix_root_clk 1       1        0        519232000   0          0     50000      Y                     32e90000.display-controller     pix
>>                                                                                                                               32ec0000.blk-ctrl               disp2
>>                                                                                                                               deviceless                      no_connection_id
>>
>> Single LVDS link mode is not affected because "media_disp2_pix"
>> clock can handle the 7 division ratio.
>>
>> To support the dual LVDS link mode, "video_pll1" clock rate needs
>> to be x2 "media_ldb" clock rate so that "media_disp2_pix" clock
>> can use 7 division ratio to achieve the /3.5 clock rate comparing
>> to "media_ldb" clock rate.  However, "video_pll1" is not seen by
>> LDB driver thus not directly controlled by it.  This is another
>> reason why assigning a reasonable "video_pll1" clock rate in DT
>> makes sense.
> 
> I agree that _right_now_, the DT clock assignment is the only option.
> I would like to see that DT part disappear and instead would prefer if the LDB/LCDIF could figure out the clock tree configuration themselves.

I think we'll live with the assigned clock rate in DT, because the
i.MX8MP LDB and Samsung MIPI DSI display pipelines need to share a
video PLL, like I mentioned in comments for patch 2.

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

-- 
Regards,
Liu Ying

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 9012 bytes --]

On Tue, Oct 22, 2024 at 02:13:57PM +0800, Liu Ying wrote:
> On 10/13/2024, Marek Vasut wrote:
> > On 10/11/24 8:18 AM, Liu Ying wrote:
> >> On 10/11/2024, Marek Vasut wrote:
> >>> On 10/10/24 7:22 AM, Liu Ying wrote:
> >>>> On 10/09/2024, Marek Vasut wrote:
> >>>>> The media_ldb_root_clk supply LDB serializer. These clock are usually
> >>>>> shared with the LCDIFv3 pixel clock and supplied by the Video PLL on
> >>>>> i.MX8MP, but the LDB clock run at either x7 or x14 rate of the LCDIFv3
> >>>>> pixel clock. Allow the LDB to reconfigure Video PLL as needed, as that
> >>>>> results in accurate serializer clock.
> >>>>>
> >>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>> ---
> >>>>> Cc: Abel Vesa <abelvesa@kernel.org>
> >>>>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> >>>>> Cc: David Airlie <airlied@gmail.com>
> >>>>> Cc: Fabio Estevam <festevam@gmail.com>
> >>>>> Cc: Isaac Scott <isaac.scott@ideasonboard.com>
> >>>>> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> >>>>> Cc: Jonas Karlman <jonas@kwiboo.se>
> >>>>> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> >>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>> Cc: Maxime Ripard <mripard@kernel.org>
> >>>>> Cc: Michael Turquette <mturquette@baylibre.com>
> >>>>> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> >>>>> Cc: Peng Fan <peng.fan@nxp.com>
> >>>>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> >>>>> Cc: Robert Foss <rfoss@kernel.org>
> >>>>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> >>>>> Cc: Shawn Guo <shawnguo@kernel.org>
> >>>>> Cc: Simona Vetter <simona@ffwll.ch>
> >>>>> Cc: Stephen Boyd <sboyd@kernel.org>
> >>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>> Cc: dri-devel@lists.freedesktop.org
> >>>>> Cc: imx@lists.linux.dev
> >>>>> Cc: kernel@dh-electronics.com
> >>>>> Cc: linux-arm-kernel@lists.infradead.org
> >>>>> Cc: linux-clk@vger.kernel.org
> >>>>> ---
> >>>>>    drivers/clk/imx/clk-imx8mp.c | 2 +-
> >>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
> >>>>> index 516dbd170c8a3..2e61d340b8ab7 100644
> >>>>> --- a/drivers/clk/imx/clk-imx8mp.c
> >>>>> +++ b/drivers/clk/imx/clk-imx8mp.c
> >>>>> @@ -611,7 +611,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
> >>>>>        hws[IMX8MP_CLK_MEDIA_MIPI_PHY1_REF] = imx8m_clk_hw_composite("media_mipi_phy1_ref", imx8mp_media_mipi_phy1_ref_sels, ccm_base + 0xbd80);
> >>>>>        hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT);
> >>>>>        hws[IMX8MP_CLK_MEDIA_CAM2_PIX] = imx8m_clk_hw_composite("media_cam2_pix", imx8mp_media_cam2_pix_sels, ccm_base + 0xbe80);
> >>>>> -    hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00);
> >>>>> +    hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite_bus_flags("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00, CLK_SET_RATE_PARENT);
> >>>>
> >>>> This patch would cause the below in-flight LDB bridge driver
> >>>> patch[1] fail to do display mode validation upon display modes
> >>>> read from LVDS to HDMI converter IT6263's DDC I2C bus.
> >>>
> >>> Why ?
> >>
> >> Mode validation is affected only for dual LVDS link mode.
> >> For single LVDS link mode, this patch does open more display
> >> modes read from the DDC I2C bus.  The reason behind is that
> >> LVDS serial clock rate/pixel clock rate = 3.5 for dual LVDS
> >> link mode, while it's 7 for single LVDS link mode.
> >>
> >> In my system, "video_pll1" clock rate is assigned to 1.0395GHz
> >> in imx8mp.dtsi.  For 1920x1080-60.00Hz with 148.5MHz pixel
> >> clock rate, "media_ldb" clock rate is 519.75MHz and
> >> "media_disp2_pix" clock rate is 148.5MHz, which is fine for
> >> dual LVDS link mode(x3.5).  For newly opened up 1920x1080-59.94Hz
> >> with 148.352MHz pixel clock rate, "video_pll1" clock rate will
> >> be changed to 519.232MHz, "media_ldb" clock rate is 519.232MHz
> >> and "media_disp2_pix" clock rate is wrongly set to 519.232MHz
> >> too because "media_disp2_pix" clock cannot handle the 3.5
> >> division ratio from "video_pll1_out" clock running at
> >> 519.232MHz.  See the below clk_summary.
> > 
> > Shouldn't this patch help exactly with that ?
> 
> No, it doesn't help but only makes clk_round_rate() called in
> LDB driver's .mode_valid() against 148.352MHz return 148.352MHz
> which allows the unexpected 1920x1080-59.94Hz display mode.
> 
> > 
> > It should allow you to set video_pll1_out to whatever is necessary by LDB first, fixate that frequency, and the LCDIFv3 would then be forced to use /7 divider from faster Video PLL1 , right ?
> 
> Yes, it allows that for single-link LVDS use cases.
> And, __no__, for dual-link LVDS use cases because the
> video_pll1_out clock rate needs to be 2x the LVDS serial clock
> rate. 
> 
> > 
> >>      video_pll1_ref_sel               1       1        0        24000000    0          0     50000      Y      deviceless                      no_connection_id
> >>         video_pll1                    1       1        0        519232000   0          0     50000      Y         deviceless                      no_connection_id
> >>            video_pll1_bypass          1       1        0        519232000   0          0     50000      Y            deviceless                      no_connection_id
> >>               video_pll1_out          2       2        0        519232000   0          0     50000      Y               deviceless                      no_connection_id
> >>                  media_ldb            1       1        0        519232000   0          0     50000      Y                  deviceless                      no_connection_id
> >>                     media_ldb_root_clk 1       1        0        519232000   0          0     50000      Y                     32ec0000.blk-ctrl:bridge@5c     ldb
> >>                                                                                                                               deviceless                      no_connection_id
> >>                  media_disp1_pix      0       0        0        129808000   0          0     50000      N                  deviceless                      no_connection_id
> >>                     media_disp1_pix_root_clk 0       0        0        129808000   0          0     50000      N                     32e80000.display-controller     pix
> >>                                                                                                                               32ec0000.blk-ctrl               disp1
> >>                                                                                                                               deviceless                      no_connection_id
> >>                  media_disp2_pix      1       1        0        519232000   0          0     50000      Y                  deviceless                      no_connection_id
> >>                     media_disp2_pix_root_clk 1       1        0        519232000   0          0     50000      Y                     32e90000.display-controller     pix
> >>                                                                                                                               32ec0000.blk-ctrl               disp2
> >>                                                                                                                               deviceless                      no_connection_id
> >>
> >> Single LVDS link mode is not affected because "media_disp2_pix"
> >> clock can handle the 7 division ratio.
> >>
> >> To support the dual LVDS link mode, "video_pll1" clock rate needs
> >> to be x2 "media_ldb" clock rate so that "media_disp2_pix" clock
> >> can use 7 division ratio to achieve the /3.5 clock rate comparing
> >> to "media_ldb" clock rate.  However, "video_pll1" is not seen by
> >> LDB driver thus not directly controlled by it.  This is another
> >> reason why assigning a reasonable "video_pll1" clock rate in DT
> >> makes sense.
> > 
> > I agree that _right_now_, the DT clock assignment is the only option.
> > I would like to see that DT part disappear and instead would prefer if the LDB/LCDIF could figure out the clock tree configuration themselves.
> 
> I think we'll live with the assigned clock rate in DT, because the
> i.MX8MP LDB and Samsung MIPI DSI display pipelines need to share a
> video PLL, like I mentioned in comments for patch 2.

Guys. There's 4 different discussions that look to be on the same topic,
and doing workarounds in the DT, DRM driver and clock driver for
something that looks like a broken clock.

Could we have *somewhere* a proper description of what the problem is
exactly, so we can review it? Because at the moment, it's certainly not
helping.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Marek Vasut]

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

[...]

>>>>> This patch would cause the below in-flight LDB bridge driver
>>>>> patch[1] fail to do display mode validation upon display modes
>>>>> read from LVDS to HDMI converter IT6263's DDC I2C bus.
>>>>
>>>> Why ?
>>>
>>> Mode validation is affected only for dual LVDS link mode.
>>> For single LVDS link mode, this patch does open more display
>>> modes read from the DDC I2C bus.  The reason behind is that
>>> LVDS serial clock rate/pixel clock rate = 3.5 for dual LVDS
>>> link mode, while it's 7 for single LVDS link mode.
>>>
>>> In my system, "video_pll1" clock rate is assigned to 1.0395GHz
>>> in imx8mp.dtsi.  For 1920x1080-60.00Hz with 148.5MHz pixel
>>> clock rate, "media_ldb" clock rate is 519.75MHz and
>>> "media_disp2_pix" clock rate is 148.5MHz, which is fine for
>>> dual LVDS link mode(x3.5).  For newly opened up 1920x1080-59.94Hz
>>> with 148.352MHz pixel clock rate, "video_pll1" clock rate will
>>> be changed to 519.232MHz, "media_ldb" clock rate is 519.232MHz
>>> and "media_disp2_pix" clock rate is wrongly set to 519.232MHz
>>> too because "media_disp2_pix" clock cannot handle the 3.5
>>> division ratio from "video_pll1_out" clock running at
>>> 519.232MHz.  See the below clk_summary.
>>
>> Shouldn't this patch help exactly with that ?
> 
> No, it doesn't help but only makes clk_round_rate() called in
> LDB driver's .mode_valid() against 148.352MHz return 148.352MHz
> which allows the unexpected 1920x1080-59.94Hz display mode.

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

>> It should allow you to set video_pll1_out to whatever is necessary by LDB first, fixate that frequency, and the LCDIFv3 would then be forced to use /7 divider from faster Video PLL1 , right ?
> 
> Yes, it allows that for single-link LVDS use cases.
> And, __no__, for dual-link LVDS use cases because the
> video_pll1_out clock rate needs to be 2x the LVDS serial clock
> rate.

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

>>>       video_pll1_ref_sel               1       1        0        24000000    0          0     50000      Y      deviceless                      no_connection_id
>>>          video_pll1                    1       1        0        519232000   0          0     50000      Y         deviceless                      no_connection_id
>>>             video_pll1_bypass          1       1        0        519232000   0          0     50000      Y            deviceless                      no_connection_id
>>>                video_pll1_out          2       2        0        519232000   0          0     50000      Y               deviceless                      no_connection_id
>>>                   media_ldb            1       1        0        519232000   0          0     50000      Y                  deviceless                      no_connection_id
>>>                      media_ldb_root_clk 1       1        0        519232000   0          0     50000      Y                     32ec0000.blk-ctrl:bridge@5c     ldb
>>>                                                                                                                                deviceless                      no_connection_id
>>>                   media_disp1_pix      0       0        0        129808000   0          0     50000      N                  deviceless                      no_connection_id
>>>                      media_disp1_pix_root_clk 0       0        0        129808000   0          0     50000      N                     32e80000.display-controller     pix
>>>                                                                                                                                32ec0000.blk-ctrl               disp1
>>>                                                                                                                                deviceless                      no_connection_id
>>>                   media_disp2_pix      1       1        0        519232000   0          0     50000      Y                  deviceless                      no_connection_id
>>>                      media_disp2_pix_root_clk 1       1        0        519232000   0          0     50000      Y                     32e90000.display-controller     pix
>>>                                                                                                                                32ec0000.blk-ctrl               disp2
>>>                                                                                                                                deviceless                      no_connection_id
>>>
>>> Single LVDS link mode is not affected because "media_disp2_pix"
>>> clock can handle the 7 division ratio.
>>>
>>> To support the dual LVDS link mode, "video_pll1" clock rate needs
>>> to be x2 "media_ldb" clock rate so that "media_disp2_pix" clock
>>> can use 7 division ratio to achieve the /3.5 clock rate comparing
>>> to "media_ldb" clock rate.  However, "video_pll1" is not seen by
>>> LDB driver thus not directly controlled by it.  This is another
>>> reason why assigning a reasonable "video_pll1" clock rate in DT
>>> makes sense.
>>
>> I agree that _right_now_, the DT clock assignment is the only option.
>> I would like to see that DT part disappear and instead would prefer if the LDB/LCDIF could figure out the clock tree configuration themselves.
> 
> I think we'll live with the assigned clock rate in DT, because the
> i.MX8MP LDB and Samsung MIPI DSI display pipelines need to share a
> video PLL, like I mentioned in comments for patch 2.

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

[...]

Re: [PATCH 2/2] drm: bridge: ldb: Configure LDB clock in .mode_set

[Marek Vasut]

On 10/22/24 7:59 AM, Liu Ying wrote:

[...]

>>> Anyway, I don't think it is necessary to manage the clk_set_rate()
>>> function calls between this driver and mxsfb_kms or lcdif_kms
>>> because "video_pll1" clock rate is supposed to be assigned in DT...
>>
>> I disagree with this part. I believe the assignment of clock in DT is only a temporary workaround which should be removed. The drivers should be able to figure out and set the clock tree configuration.
> 
> I think the clock rate assignment in DT is still needed.
> A good reason is that again we need to share one video PLL
> between MIPI DSI and LDB display pipelines for i.MX8MP.

You don't really need to share the Video PLL , you can free up e.g. PLL3 
and use it for one video output pipeline, and use the Video PLL for the 
other video pipeline, and then you get accurate pixel clock in both 
pipelines.

>>>>> The idea is to assign a reasonable PLL clock rate in DT to make
>>>>> display drivers' life easier, especially for i.MX8MP where LDB,
>>>>> Samsung MIPI DSI may use a single PLL at the same time.
>>>> I would really like to avoid setting arbitrary clock in DT, esp. if it can be avoided. And it surely can be avoided for this simple use case.
>>>
>>> ... just like I said in patch 1/2, "video_pll1" clock rate needs
>>> to be x2 "media_ldb" clock rate for dual LVDS link mode. Without
>>> an assigned "video_pll1" clock rate in DT, this driver cannot
>>> achieve that.
>>
>> This is something the LDB driver can infer from DT and configure the clock tree accordingly.
> 
> Well, the LDB driver only controls the "ldb" clock rate. It doesn't
> magically set the parent "video_pll1" clock's rate to 2x it's rate,
> unless the driver gets "video_pll1_out" clock by calling
> clk_get_parent() and directly controls the PLL clock rate which
> doesn't look neat.

It isn't nice, but it actually may solve this problem, no ?

>>> And, the i.MX8MP LDB + Samsung MIPI DSI case is
>>> not simple considering using one single PLL and display modes
>>> read from EDID.
>> You could use separate PLLs for each LCDIF scanout engine in such a deployment, I already ran into that, so I am aware of it. That is probably the best way out of such a problem, esp. if accurate pixel clock are the requirement.
> 
> I cannot use separate PLLs for the i.MX8MP LDB and Samsung MIPI
> DSI display pipelines on i.MX8MP EVK, because the PLLs are limited
> resources and we are running out of it.  Because LDB needs the pixel
> clock and LVDS serial clock to be derived from a same PLL, the only
> valid PLLs(see imx8mp_media_disp_pix_sels[] and
> imx8mp_media_ldb_sels[]) are "video_pll1_out", "audio_pll2_out",
> "sys_pll2_1000m" and "sys_pll1_800m".  All are used as either audio
> clock or system clocks on i.MX8MP EVK, except "video_pll1_out".

Could you use Video PLL for LDB and PLL3 for DSI then ?

I think this could still be configurable per board, it shouldn't be such 
that one board which attempts to showcase everything would prevent other 
boards with specific requirements from achieving those.

> You probably may use separate PLLs for a particular i.MX8MP platform
> with limited features, but not for i.MX8MP EVK which is supposed to
> evaluate all SoC features.
Right, that, exactly.

[...]

Re: [PATCH 2/2] drm: bridge: ldb: Configure LDB clock in .mode_set

[Liu Ying]

On 10/23/2024, Marek Vasut wrote:
> On 10/22/24 7:59 AM, Liu Ying wrote:
> 
> [...]
> 
>>>> Anyway, I don't think it is necessary to manage the clk_set_rate()
>>>> function calls between this driver and mxsfb_kms or lcdif_kms
>>>> because "video_pll1" clock rate is supposed to be assigned in DT...
>>>
>>> I disagree with this part. I believe the assignment of clock in DT is only a temporary workaround which should be removed. The drivers should be able to figure out and set the clock tree configuration.
>>
>> I think the clock rate assignment in DT is still needed.
>> A good reason is that again we need to share one video PLL
>> between MIPI DSI and LDB display pipelines for i.MX8MP.
> 
> You don't really need to share the Video PLL , you can free up e.g. PLL3 and use it for one video output pipeline, and use the Video PLL for the other video pipeline, and then you get accurate pixel clock in both pipelines.

I need to share the video PLL. PLL3 is used as audio AXI's parent
in NXP downstream kernel for i.MX8MP EVK(Nominal Drive Mode) and
derives a audio AXI clock running at 600MHz which is the nominal
audio AXI clock rate mentioned in i.MX8MP chip data sheet.

https://github.com/nxp-imx/linux-imx/blob/lf-6.6.y/arch/arm64/boot/dts/freescale/imx8mp-evk-ndm.dts#L19
https://github.com/nxp-imx/linux-imx/blob/lf-6.6.y/arch/arm64/boot/dts/freescale/imx8mp-ddr4-evk.dts#L25

Upstream kernel currently uses PLL1 800M as audio AXI's parent.
Although audio AXI clock is assigned to 600MHz, the actual rate
reported by clock summary is 400MHz(not the nominal rate). So,
audio AXI clock's parent is supposed to be changed to PLL3.

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/freescale/imx8mp.dtsi?h=v6.12-rc4#n815

    sys_pll1_ref_sel                 1       1        0        24000000    0          0     50000      Y      deviceless                      no_connection_id         
       sys_pll1                      1       1        0        800000000   0          0     50000      Y         deviceless                      no_connection_id         
          sys_pll1_bypass            1       1        0        800000000   0          0     50000      Y            deviceless                      no_connection_id         
             sys_pll1_out            4       4        0        800000000   0          0     50000      Y               deviceless                      no_connection_id         
                sys_pll1_800m        5       6        0        800000000   0          0     50000      Y                  deviceless                      no_connection_id             
                   audio_axi         1       1        0        400000000   0          0     50000      Y                     power-domain@5                  no_connection_id         
                                                                                                                             deviceless                      no_connection_id         
                      audio_axi_root 0       0        0        400000000   0          0     50000      Y                        deviceless                      no_connection_id            

All other clocks in imx8mp_media_disp_pix_sels[] are not appropriate
to be used by display pipelines, except "video_pll1_out", at least
for i.MX8MP EVK.

> 
>>>>>> The idea is to assign a reasonable PLL clock rate in DT to make
>>>>>> display drivers' life easier, especially for i.MX8MP where LDB,
>>>>>> Samsung MIPI DSI may use a single PLL at the same time.
>>>>> I would really like to avoid setting arbitrary clock in DT, esp. if it can be avoided. And it surely can be avoided for this simple use case.
>>>>
>>>> ... just like I said in patch 1/2, "video_pll1" clock rate needs
>>>> to be x2 "media_ldb" clock rate for dual LVDS link mode. Without
>>>> an assigned "video_pll1" clock rate in DT, this driver cannot
>>>> achieve that.
>>>
>>> This is something the LDB driver can infer from DT and configure the clock tree accordingly.
>>
>> Well, the LDB driver only controls the "ldb" clock rate. It doesn't
>> magically set the parent "video_pll1" clock's rate to 2x it's rate,
>> unless the driver gets "video_pll1_out" clock by calling
>> clk_get_parent() and directly controls the PLL clock rate which
>> doesn't look neat.
> 
> It isn't nice, but it actually may solve this problem, no ?

Not nice, but it may actually call clk_set_rate() directly
for "video_pll1_out".

> 
>>>> And, the i.MX8MP LDB + Samsung MIPI DSI case is
>>>> not simple considering using one single PLL and display modes
>>>> read from EDID.
>>> You could use separate PLLs for each LCDIF scanout engine in such a deployment, I already ran into that, so I am aware of it. That is probably the best way out of such a problem, esp. if accurate pixel clock are the requirement.
>>
>> I cannot use separate PLLs for the i.MX8MP LDB and Samsung MIPI
>> DSI display pipelines on i.MX8MP EVK, because the PLLs are limited
>> resources and we are running out of it.  Because LDB needs the pixel
>> clock and LVDS serial clock to be derived from a same PLL, the only
>> valid PLLs(see imx8mp_media_disp_pix_sels[] and
>> imx8mp_media_ldb_sels[]) are "video_pll1_out", "audio_pll2_out",
>> "sys_pll2_1000m" and "sys_pll1_800m".  All are used as either audio
>> clock or system clocks on i.MX8MP EVK, except "video_pll1_out".
> 
> Could you use Video PLL for LDB and PLL3 for DSI then ?

No, I can't, as I explained above - PLL3 is supposed to be used as
audio AXI clock's parent to achieve the nominal 600MHz clock rate
for audio AXI clock.

> 
> I think this could still be configurable per board, it shouldn't be such that one board which attempts to showcase everything would prevent other boards with specific requirements from achieving those.

You probably may set "ldb" clock rate in this driver and
additionally/un-nicely set it's parent clock rate(the video PLL
rate for i.MX8MP) for dual-link LVDS use cases.  But, due to the
shared PLL, it doesn't look ok to set CLK_SET_RATE_PARENT flag
for "media_ldb" as patch 1 does.

> 
>> You probably may use separate PLLs for a particular i.MX8MP platform
>> with limited features, but not for i.MX8MP EVK which is supposed to
>> evaluate all SoC features.
> Right, that, exactly.
> 
> [...]
> 

-- 
Regards,
Liu Ying

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

[Liu Ying]

On 10/23/2024, Marek Vasut wrote:
> On 10/22/24 8:13 AM, Liu Ying wrote:
> 
> [...]
> 
>>>>>> This patch would cause the below in-flight LDB bridge driver
>>>>>> patch[1] fail to do display mode validation upon display modes
>>>>>> read from LVDS to HDMI converter IT6263's DDC I2C bus.
>>>>>
>>>>> Why ?
>>>>
>>>> Mode validation is affected only for dual LVDS link mode.
>>>> For single LVDS link mode, this patch does open more display
>>>> modes read from the DDC I2C bus.  The reason behind is that
>>>> LVDS serial clock rate/pixel clock rate = 3.5 for dual LVDS
>>>> link mode, while it's 7 for single LVDS link mode.
>>>>
>>>> In my system, "video_pll1" clock rate is assigned to 1.0395GHz
>>>> in imx8mp.dtsi.  For 1920x1080-60.00Hz with 148.5MHz pixel
>>>> clock rate, "media_ldb" clock rate is 519.75MHz and
>>>> "media_disp2_pix" clock rate is 148.5MHz, which is fine for
>>>> dual LVDS link mode(x3.5).  For newly opened up 1920x1080-59.94Hz
>>>> with 148.352MHz pixel clock rate, "video_pll1" clock rate will
>>>> be changed to 519.232MHz, "media_ldb" clock rate is 519.232MHz
>>>> and "media_disp2_pix" clock rate is wrongly set to 519.232MHz
>>>> too because "media_disp2_pix" clock cannot handle the 3.5
>>>> division ratio from "video_pll1_out" clock running at
>>>> 519.232MHz.  See the below clk_summary.
>>>
>>> Shouldn't this patch help exactly with that ?
>>
>> No, it doesn't help but only makes clk_round_rate() called in
>> LDB driver's .mode_valid() against 148.352MHz return 148.352MHz
>> which allows the unexpected 1920x1080-59.94Hz display mode.
> 
> Why is 1920x1080-59.94Hz mode unexpected in the first place ?

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

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

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

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

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

> 
>>> It should allow you to set video_pll1_out to whatever is necessary by LDB first, fixate that frequency, and the LCDIFv3 would then be forced to use /7 divider from faster Video PLL1 , right ?
>>
>> Yes, it allows that for single-link LVDS use cases.
>> And, __no__, for dual-link LVDS use cases because the
>> video_pll1_out clock rate needs to be 2x the LVDS serial clock
>> rate.
> 
> Can't the LDB still set the Video PLL frequency to whatever it needs first, fixate it, and only then let the LCDIFv3 divide the frequency down ? (sorry, I am a bit tired today, maybe I am missing the obvious)

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

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

> 
>>>>       video_pll1_ref_sel               1       1        0        24000000    0          0     50000      Y      deviceless                      no_connection_id
>>>>          video_pll1                    1       1        0        519232000   0          0     50000      Y         deviceless                      no_connection_id
>>>>             video_pll1_bypass          1       1        0        519232000   0          0     50000      Y            deviceless                      no_connection_id
>>>>                video_pll1_out          2       2        0        519232000   0          0     50000      Y               deviceless                      no_connection_id
>>>>                   media_ldb            1       1        0        519232000   0          0     50000      Y                  deviceless                      no_connection_id
>>>>                      media_ldb_root_clk 1       1        0        519232000   0          0     50000      Y                     32ec0000.blk-ctrl:bridge@5c     ldb
>>>>                                                                                                                                deviceless                      no_connection_id
>>>>                   media_disp1_pix      0       0        0        129808000   0          0     50000      N                  deviceless                      no_connection_id
>>>>                      media_disp1_pix_root_clk 0       0        0        129808000   0          0     50000      N                     32e80000.display-controller     pix
>>>>                                                                                                                                32ec0000.blk-ctrl               disp1
>>>>                                                                                                                                deviceless                      no_connection_id
>>>>                   media_disp2_pix      1       1        0        519232000   0          0     50000      Y                  deviceless                      no_connection_id
>>>>                      media_disp2_pix_root_clk 1       1        0        519232000   0          0     50000      Y                     32e90000.display-controller     pix
>>>>                                                                                                                                32ec0000.blk-ctrl               disp2
>>>>                                                                                                                                deviceless                      no_connection_id
>>>>
>>>> Single LVDS link mode is not affected because "media_disp2_pix"
>>>> clock can handle the 7 division ratio.
>>>>
>>>> To support the dual LVDS link mode, "video_pll1" clock rate needs
>>>> to be x2 "media_ldb" clock rate so that "media_disp2_pix" clock
>>>> can use 7 division ratio to achieve the /3.5 clock rate comparing
>>>> to "media_ldb" clock rate.  However, "video_pll1" is not seen by
>>>> LDB driver thus not directly controlled by it.  This is another
>>>> reason why assigning a reasonable "video_pll1" clock rate in DT
>>>> makes sense.
>>>
>>> I agree that _right_now_, the DT clock assignment is the only option.
>>> I would like to see that DT part disappear and instead would prefer if the LDB/LCDIF could figure out the clock tree configuration themselves.
>>
>> I think we'll live with the assigned clock rate in DT, because the
>> i.MX8MP LDB and Samsung MIPI DSI display pipelines need to share a
>> video PLL, like I mentioned in comments for patch 2.
> 
> They do NOT need to share a PLL, you can use e.g. PLL3 for one and Video PLL for the other if the requirement is accurate pixel clock .

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

> 
> [...]

-- 
Regards,
Liu Ying

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

[Liu Ying]

Hi Maxime,

On 10/22/2024, Maxime Ripard wrote:
> On Tue, Oct 22, 2024 at 02:13:57PM +0800, Liu Ying wrote:
>> On 10/13/2024, Marek Vasut wrote:
>>> On 10/11/24 8:18 AM, Liu Ying wrote:
>>>> On 10/11/2024, Marek Vasut wrote:
>>>>> On 10/10/24 7:22 AM, Liu Ying wrote:
>>>>>> On 10/09/2024, Marek Vasut wrote:
>>>>>>> The media_ldb_root_clk supply LDB serializer. These clock are usually
>>>>>>> shared with the LCDIFv3 pixel clock and supplied by the Video PLL on
>>>>>>> i.MX8MP, but the LDB clock run at either x7 or x14 rate of the LCDIFv3
>>>>>>> pixel clock. Allow the LDB to reconfigure Video PLL as needed, as that
>>>>>>> results in accurate serializer clock.
>>>>>>>
>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>> ---
>>>>>>> Cc: Abel Vesa <abelvesa@kernel.org>
>>>>>>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>>>>>>> Cc: David Airlie <airlied@gmail.com>
>>>>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>>>>> Cc: Isaac Scott <isaac.scott@ideasonboard.com>
>>>>>>> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
>>>>>>> Cc: Jonas Karlman <jonas@kwiboo.se>
>>>>>>> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
>>>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>>> Cc: Maxime Ripard <mripard@kernel.org>
>>>>>>> Cc: Michael Turquette <mturquette@baylibre.com>
>>>>>>> Cc: Neil Armstrong <neil.armstrong@linaro.org>
>>>>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>>>>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
>>>>>>> Cc: Robert Foss <rfoss@kernel.org>
>>>>>>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>>>>>>> Cc: Shawn Guo <shawnguo@kernel.org>
>>>>>>> Cc: Simona Vetter <simona@ffwll.ch>
>>>>>>> Cc: Stephen Boyd <sboyd@kernel.org>
>>>>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>>> Cc: dri-devel@lists.freedesktop.org
>>>>>>> Cc: imx@lists.linux.dev
>>>>>>> Cc: kernel@dh-electronics.com
>>>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>>>> Cc: linux-clk@vger.kernel.org
>>>>>>> ---
>>>>>>>    drivers/clk/imx/clk-imx8mp.c | 2 +-
>>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>
>>>>>>> diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
>>>>>>> index 516dbd170c8a3..2e61d340b8ab7 100644
>>>>>>> --- a/drivers/clk/imx/clk-imx8mp.c
>>>>>>> +++ b/drivers/clk/imx/clk-imx8mp.c
>>>>>>> @@ -611,7 +611,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
>>>>>>>        hws[IMX8MP_CLK_MEDIA_MIPI_PHY1_REF] = imx8m_clk_hw_composite("media_mipi_phy1_ref", imx8mp_media_mipi_phy1_ref_sels, ccm_base + 0xbd80);
>>>>>>>        hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT);
>>>>>>>        hws[IMX8MP_CLK_MEDIA_CAM2_PIX] = imx8m_clk_hw_composite("media_cam2_pix", imx8mp_media_cam2_pix_sels, ccm_base + 0xbe80);
>>>>>>> -    hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00);
>>>>>>> +    hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite_bus_flags("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00, CLK_SET_RATE_PARENT);
>>>>>>
>>>>>> This patch would cause the below in-flight LDB bridge driver
>>>>>> patch[1] fail to do display mode validation upon display modes
>>>>>> read from LVDS to HDMI converter IT6263's DDC I2C bus.
>>>>>
>>>>> Why ?
>>>>
>>>> Mode validation is affected only for dual LVDS link mode.
>>>> For single LVDS link mode, this patch does open more display
>>>> modes read from the DDC I2C bus.  The reason behind is that
>>>> LVDS serial clock rate/pixel clock rate = 3.5 for dual LVDS
>>>> link mode, while it's 7 for single LVDS link mode.
>>>>
>>>> In my system, "video_pll1" clock rate is assigned to 1.0395GHz
>>>> in imx8mp.dtsi.  For 1920x1080-60.00Hz with 148.5MHz pixel
>>>> clock rate, "media_ldb" clock rate is 519.75MHz and
>>>> "media_disp2_pix" clock rate is 148.5MHz, which is fine for
>>>> dual LVDS link mode(x3.5).  For newly opened up 1920x1080-59.94Hz
>>>> with 148.352MHz pixel clock rate, "video_pll1" clock rate will
>>>> be changed to 519.232MHz, "media_ldb" clock rate is 519.232MHz
>>>> and "media_disp2_pix" clock rate is wrongly set to 519.232MHz
>>>> too because "media_disp2_pix" clock cannot handle the 3.5
>>>> division ratio from "video_pll1_out" clock running at
>>>> 519.232MHz.  See the below clk_summary.
>>>
>>> Shouldn't this patch help exactly with that ?
>>
>> No, it doesn't help but only makes clk_round_rate() called in
>> LDB driver's .mode_valid() against 148.352MHz return 148.352MHz
>> which allows the unexpected 1920x1080-59.94Hz display mode.
>>
>>>
>>> It should allow you to set video_pll1_out to whatever is necessary by LDB first, fixate that frequency, and the LCDIFv3 would then be forced to use /7 divider from faster Video PLL1 , right ?
>>
>> Yes, it allows that for single-link LVDS use cases.
>> And, __no__, for dual-link LVDS use cases because the
>> video_pll1_out clock rate needs to be 2x the LVDS serial clock
>> rate. 
>>
>>>
>>>>      video_pll1_ref_sel               1       1        0        24000000    0          0     50000      Y      deviceless                      no_connection_id
>>>>         video_pll1                    1       1        0        519232000   0          0     50000      Y         deviceless                      no_connection_id
>>>>            video_pll1_bypass          1       1        0        519232000   0          0     50000      Y            deviceless                      no_connection_id
>>>>               video_pll1_out          2       2        0        519232000   0          0     50000      Y               deviceless                      no_connection_id
>>>>                  media_ldb            1       1        0        519232000   0          0     50000      Y                  deviceless                      no_connection_id
>>>>                     media_ldb_root_clk 1       1        0        519232000   0          0     50000      Y                     32ec0000.blk-ctrl:bridge@5c     ldb
>>>>                                                                                                                               deviceless                      no_connection_id
>>>>                  media_disp1_pix      0       0        0        129808000   0          0     50000      N                  deviceless                      no_connection_id
>>>>                     media_disp1_pix_root_clk 0       0        0        129808000   0          0     50000      N                     32e80000.display-controller     pix
>>>>                                                                                                                               32ec0000.blk-ctrl               disp1
>>>>                                                                                                                               deviceless                      no_connection_id
>>>>                  media_disp2_pix      1       1        0        519232000   0          0     50000      Y                  deviceless                      no_connection_id
>>>>                     media_disp2_pix_root_clk 1       1        0        519232000   0          0     50000      Y                     32e90000.display-controller     pix
>>>>                                                                                                                               32ec0000.blk-ctrl               disp2
>>>>                                                                                                                               deviceless                      no_connection_id
>>>>
>>>> Single LVDS link mode is not affected because "media_disp2_pix"
>>>> clock can handle the 7 division ratio.
>>>>
>>>> To support the dual LVDS link mode, "video_pll1" clock rate needs
>>>> to be x2 "media_ldb" clock rate so that "media_disp2_pix" clock
>>>> can use 7 division ratio to achieve the /3.5 clock rate comparing
>>>> to "media_ldb" clock rate.  However, "video_pll1" is not seen by
>>>> LDB driver thus not directly controlled by it.  This is another
>>>> reason why assigning a reasonable "video_pll1" clock rate in DT
>>>> makes sense.
>>>
>>> I agree that _right_now_, the DT clock assignment is the only option.
>>> I would like to see that DT part disappear and instead would prefer if the LDB/LCDIF could figure out the clock tree configuration themselves.
>>
>> I think we'll live with the assigned clock rate in DT, because the
>> i.MX8MP LDB and Samsung MIPI DSI display pipelines need to share a
>> video PLL, like I mentioned in comments for patch 2.
> 
> Guys. There's 4 different discussions that look to be on the same topic,
> and doing workarounds in the DT, DRM driver and clock driver for
> something that looks like a broken clock.

This is a bit complicated, because it is related to i.MX8MP MIPI DSI/
LVDS/HDMI, i.MX93 MIPI DSI/LVDS/parallel display pipelines.  Even
i.MX6SX LVDS display pipeline is a bit related, since i.MX8MP/i.MX93/
i.MX6SX LDBs share the same fsl-ldb.c driver.

> 
> Could we have *somewhere* a proper description of what the problem is
> exactly, so we can review it? Because at the moment, it's certainly not
> helping.

Can you please suggest a place where this could happen?

> 
> Maxime

-- 
Regards,
Liu Ying

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 9826 bytes --]

On Thu, Oct 31, 2024 at 10:35:27AM +0800, Liu Ying wrote:
> Hi Maxime,
> 
> On 10/22/2024, Maxime Ripard wrote:
> > On Tue, Oct 22, 2024 at 02:13:57PM +0800, Liu Ying wrote:
> >> On 10/13/2024, Marek Vasut wrote:
> >>> On 10/11/24 8:18 AM, Liu Ying wrote:
> >>>> On 10/11/2024, Marek Vasut wrote:
> >>>>> On 10/10/24 7:22 AM, Liu Ying wrote:
> >>>>>> On 10/09/2024, Marek Vasut wrote:
> >>>>>>> The media_ldb_root_clk supply LDB serializer. These clock are usually
> >>>>>>> shared with the LCDIFv3 pixel clock and supplied by the Video PLL on
> >>>>>>> i.MX8MP, but the LDB clock run at either x7 or x14 rate of the LCDIFv3
> >>>>>>> pixel clock. Allow the LDB to reconfigure Video PLL as needed, as that
> >>>>>>> results in accurate serializer clock.
> >>>>>>>
> >>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>>>> ---
> >>>>>>> Cc: Abel Vesa <abelvesa@kernel.org>
> >>>>>>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
> >>>>>>> Cc: David Airlie <airlied@gmail.com>
> >>>>>>> Cc: Fabio Estevam <festevam@gmail.com>
> >>>>>>> Cc: Isaac Scott <isaac.scott@ideasonboard.com>
> >>>>>>> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
> >>>>>>> Cc: Jonas Karlman <jonas@kwiboo.se>
> >>>>>>> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> >>>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>>>>> Cc: Maxime Ripard <mripard@kernel.org>
> >>>>>>> Cc: Michael Turquette <mturquette@baylibre.com>
> >>>>>>> Cc: Neil Armstrong <neil.armstrong@linaro.org>
> >>>>>>> Cc: Peng Fan <peng.fan@nxp.com>
> >>>>>>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
> >>>>>>> Cc: Robert Foss <rfoss@kernel.org>
> >>>>>>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
> >>>>>>> Cc: Shawn Guo <shawnguo@kernel.org>
> >>>>>>> Cc: Simona Vetter <simona@ffwll.ch>
> >>>>>>> Cc: Stephen Boyd <sboyd@kernel.org>
> >>>>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
> >>>>>>> Cc: dri-devel@lists.freedesktop.org
> >>>>>>> Cc: imx@lists.linux.dev
> >>>>>>> Cc: kernel@dh-electronics.com
> >>>>>>> Cc: linux-arm-kernel@lists.infradead.org
> >>>>>>> Cc: linux-clk@vger.kernel.org
> >>>>>>> ---
> >>>>>>>    drivers/clk/imx/clk-imx8mp.c | 2 +-
> >>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
> >>>>>>> index 516dbd170c8a3..2e61d340b8ab7 100644
> >>>>>>> --- a/drivers/clk/imx/clk-imx8mp.c
> >>>>>>> +++ b/drivers/clk/imx/clk-imx8mp.c
> >>>>>>> @@ -611,7 +611,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
> >>>>>>>        hws[IMX8MP_CLK_MEDIA_MIPI_PHY1_REF] = imx8m_clk_hw_composite("media_mipi_phy1_ref", imx8mp_media_mipi_phy1_ref_sels, ccm_base + 0xbd80);
> >>>>>>>        hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT);
> >>>>>>>        hws[IMX8MP_CLK_MEDIA_CAM2_PIX] = imx8m_clk_hw_composite("media_cam2_pix", imx8mp_media_cam2_pix_sels, ccm_base + 0xbe80);
> >>>>>>> -    hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00);
> >>>>>>> +    hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite_bus_flags("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00, CLK_SET_RATE_PARENT);
> >>>>>>
> >>>>>> This patch would cause the below in-flight LDB bridge driver
> >>>>>> patch[1] fail to do display mode validation upon display modes
> >>>>>> read from LVDS to HDMI converter IT6263's DDC I2C bus.
> >>>>>
> >>>>> Why ?
> >>>>
> >>>> Mode validation is affected only for dual LVDS link mode.
> >>>> For single LVDS link mode, this patch does open more display
> >>>> modes read from the DDC I2C bus.  The reason behind is that
> >>>> LVDS serial clock rate/pixel clock rate = 3.5 for dual LVDS
> >>>> link mode, while it's 7 for single LVDS link mode.
> >>>>
> >>>> In my system, "video_pll1" clock rate is assigned to 1.0395GHz
> >>>> in imx8mp.dtsi.  For 1920x1080-60.00Hz with 148.5MHz pixel
> >>>> clock rate, "media_ldb" clock rate is 519.75MHz and
> >>>> "media_disp2_pix" clock rate is 148.5MHz, which is fine for
> >>>> dual LVDS link mode(x3.5).  For newly opened up 1920x1080-59.94Hz
> >>>> with 148.352MHz pixel clock rate, "video_pll1" clock rate will
> >>>> be changed to 519.232MHz, "media_ldb" clock rate is 519.232MHz
> >>>> and "media_disp2_pix" clock rate is wrongly set to 519.232MHz
> >>>> too because "media_disp2_pix" clock cannot handle the 3.5
> >>>> division ratio from "video_pll1_out" clock running at
> >>>> 519.232MHz.  See the below clk_summary.
> >>>
> >>> Shouldn't this patch help exactly with that ?
> >>
> >> No, it doesn't help but only makes clk_round_rate() called in
> >> LDB driver's .mode_valid() against 148.352MHz return 148.352MHz
> >> which allows the unexpected 1920x1080-59.94Hz display mode.
> >>
> >>>
> >>> It should allow you to set video_pll1_out to whatever is necessary by LDB first, fixate that frequency, and the LCDIFv3 would then be forced to use /7 divider from faster Video PLL1 , right ?
> >>
> >> Yes, it allows that for single-link LVDS use cases.
> >> And, __no__, for dual-link LVDS use cases because the
> >> video_pll1_out clock rate needs to be 2x the LVDS serial clock
> >> rate. 
> >>
> >>>
> >>>>      video_pll1_ref_sel               1       1        0        24000000    0          0     50000      Y      deviceless                      no_connection_id
> >>>>         video_pll1                    1       1        0        519232000   0          0     50000      Y         deviceless                      no_connection_id
> >>>>            video_pll1_bypass          1       1        0        519232000   0          0     50000      Y            deviceless                      no_connection_id
> >>>>               video_pll1_out          2       2        0        519232000   0          0     50000      Y               deviceless                      no_connection_id
> >>>>                  media_ldb            1       1        0        519232000   0          0     50000      Y                  deviceless                      no_connection_id
> >>>>                     media_ldb_root_clk 1       1        0        519232000   0          0     50000      Y                     32ec0000.blk-ctrl:bridge@5c     ldb
> >>>>                                                                                                                               deviceless                      no_connection_id
> >>>>                  media_disp1_pix      0       0        0        129808000   0          0     50000      N                  deviceless                      no_connection_id
> >>>>                     media_disp1_pix_root_clk 0       0        0        129808000   0          0     50000      N                     32e80000.display-controller     pix
> >>>>                                                                                                                               32ec0000.blk-ctrl               disp1
> >>>>                                                                                                                               deviceless                      no_connection_id
> >>>>                  media_disp2_pix      1       1        0        519232000   0          0     50000      Y                  deviceless                      no_connection_id
> >>>>                     media_disp2_pix_root_clk 1       1        0        519232000   0          0     50000      Y                     32e90000.display-controller     pix
> >>>>                                                                                                                               32ec0000.blk-ctrl               disp2
> >>>>                                                                                                                               deviceless                      no_connection_id
> >>>>
> >>>> Single LVDS link mode is not affected because "media_disp2_pix"
> >>>> clock can handle the 7 division ratio.
> >>>>
> >>>> To support the dual LVDS link mode, "video_pll1" clock rate needs
> >>>> to be x2 "media_ldb" clock rate so that "media_disp2_pix" clock
> >>>> can use 7 division ratio to achieve the /3.5 clock rate comparing
> >>>> to "media_ldb" clock rate.  However, "video_pll1" is not seen by
> >>>> LDB driver thus not directly controlled by it.  This is another
> >>>> reason why assigning a reasonable "video_pll1" clock rate in DT
> >>>> makes sense.
> >>>
> >>> I agree that _right_now_, the DT clock assignment is the only option.
> >>> I would like to see that DT part disappear and instead would prefer if the LDB/LCDIF could figure out the clock tree configuration themselves.
> >>
> >> I think we'll live with the assigned clock rate in DT, because the
> >> i.MX8MP LDB and Samsung MIPI DSI display pipelines need to share a
> >> video PLL, like I mentioned in comments for patch 2.
> > 
> > Guys. There's 4 different discussions that look to be on the same topic,
> > and doing workarounds in the DT, DRM driver and clock driver for
> > something that looks like a broken clock.
> 
> This is a bit complicated, because it is related to i.MX8MP MIPI DSI/
> LVDS/HDMI, i.MX93 MIPI DSI/LVDS/parallel display pipelines.  Even
> i.MX6SX LVDS display pipeline is a bit related, since i.MX8MP/i.MX93/
> i.MX6SX LDBs share the same fsl-ldb.c driver.
> 
> > 
> > Could we have *somewhere* a proper description of what the problem is
> > exactly, so we can review it? Because at the moment, it's certainly not
> > helping.
> 
> Can you please suggest a place where this could happen?

Here, by mail will be good. Worst case scenario using a ascii art.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Liu Ying]

On 11/18/2024, Maxime Ripard wrote:
> On Thu, Oct 31, 2024 at 10:35:27AM +0800, Liu Ying wrote:
>> Hi Maxime,
>>
>> On 10/22/2024, Maxime Ripard wrote:
>>> On Tue, Oct 22, 2024 at 02:13:57PM +0800, Liu Ying wrote:
>>>> On 10/13/2024, Marek Vasut wrote:
>>>>> On 10/11/24 8:18 AM, Liu Ying wrote:
>>>>>> On 10/11/2024, Marek Vasut wrote:
>>>>>>> On 10/10/24 7:22 AM, Liu Ying wrote:
>>>>>>>> On 10/09/2024, Marek Vasut wrote:
>>>>>>>>> The media_ldb_root_clk supply LDB serializer. These clock are usually
>>>>>>>>> shared with the LCDIFv3 pixel clock and supplied by the Video PLL on
>>>>>>>>> i.MX8MP, but the LDB clock run at either x7 or x14 rate of the LCDIFv3
>>>>>>>>> pixel clock. Allow the LDB to reconfigure Video PLL as needed, as that
>>>>>>>>> results in accurate serializer clock.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Marek Vasut <marex@denx.de>
>>>>>>>>> ---
>>>>>>>>> Cc: Abel Vesa <abelvesa@kernel.org>
>>>>>>>>> Cc: Andrzej Hajda <andrzej.hajda@intel.com>
>>>>>>>>> Cc: David Airlie <airlied@gmail.com>
>>>>>>>>> Cc: Fabio Estevam <festevam@gmail.com>
>>>>>>>>> Cc: Isaac Scott <isaac.scott@ideasonboard.com>
>>>>>>>>> Cc: Jernej Skrabec <jernej.skrabec@gmail.com>
>>>>>>>>> Cc: Jonas Karlman <jonas@kwiboo.se>
>>>>>>>>> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
>>>>>>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>>>>> Cc: Maxime Ripard <mripard@kernel.org>
>>>>>>>>> Cc: Michael Turquette <mturquette@baylibre.com>
>>>>>>>>> Cc: Neil Armstrong <neil.armstrong@linaro.org>
>>>>>>>>> Cc: Peng Fan <peng.fan@nxp.com>
>>>>>>>>> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
>>>>>>>>> Cc: Robert Foss <rfoss@kernel.org>
>>>>>>>>> Cc: Sascha Hauer <s.hauer@pengutronix.de>
>>>>>>>>> Cc: Shawn Guo <shawnguo@kernel.org>
>>>>>>>>> Cc: Simona Vetter <simona@ffwll.ch>
>>>>>>>>> Cc: Stephen Boyd <sboyd@kernel.org>
>>>>>>>>> Cc: Thomas Zimmermann <tzimmermann@suse.de>
>>>>>>>>> Cc: dri-devel@lists.freedesktop.org
>>>>>>>>> Cc: imx@lists.linux.dev
>>>>>>>>> Cc: kernel@dh-electronics.com
>>>>>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>>>>>> Cc: linux-clk@vger.kernel.org
>>>>>>>>> ---
>>>>>>>>>    drivers/clk/imx/clk-imx8mp.c | 2 +-
>>>>>>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
>>>>>>>>> index 516dbd170c8a3..2e61d340b8ab7 100644
>>>>>>>>> --- a/drivers/clk/imx/clk-imx8mp.c
>>>>>>>>> +++ b/drivers/clk/imx/clk-imx8mp.c
>>>>>>>>> @@ -611,7 +611,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
>>>>>>>>>        hws[IMX8MP_CLK_MEDIA_MIPI_PHY1_REF] = imx8m_clk_hw_composite("media_mipi_phy1_ref", imx8mp_media_mipi_phy1_ref_sels, ccm_base + 0xbd80);
>>>>>>>>>        hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT);
>>>>>>>>>        hws[IMX8MP_CLK_MEDIA_CAM2_PIX] = imx8m_clk_hw_composite("media_cam2_pix", imx8mp_media_cam2_pix_sels, ccm_base + 0xbe80);
>>>>>>>>> -    hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00);
>>>>>>>>> +    hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite_bus_flags("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00, CLK_SET_RATE_PARENT);
>>>>>>>>
>>>>>>>> This patch would cause the below in-flight LDB bridge driver
>>>>>>>> patch[1] fail to do display mode validation upon display modes
>>>>>>>> read from LVDS to HDMI converter IT6263's DDC I2C bus.
>>>>>>>
>>>>>>> Why ?
>>>>>>
>>>>>> Mode validation is affected only for dual LVDS link mode.
>>>>>> For single LVDS link mode, this patch does open more display
>>>>>> modes read from the DDC I2C bus.  The reason behind is that
>>>>>> LVDS serial clock rate/pixel clock rate = 3.5 for dual LVDS
>>>>>> link mode, while it's 7 for single LVDS link mode.
>>>>>>
>>>>>> In my system, "video_pll1" clock rate is assigned to 1.0395GHz
>>>>>> in imx8mp.dtsi.  For 1920x1080-60.00Hz with 148.5MHz pixel
>>>>>> clock rate, "media_ldb" clock rate is 519.75MHz and
>>>>>> "media_disp2_pix" clock rate is 148.5MHz, which is fine for
>>>>>> dual LVDS link mode(x3.5).  For newly opened up 1920x1080-59.94Hz
>>>>>> with 148.352MHz pixel clock rate, "video_pll1" clock rate will
>>>>>> be changed to 519.232MHz, "media_ldb" clock rate is 519.232MHz
>>>>>> and "media_disp2_pix" clock rate is wrongly set to 519.232MHz
>>>>>> too because "media_disp2_pix" clock cannot handle the 3.5
>>>>>> division ratio from "video_pll1_out" clock running at
>>>>>> 519.232MHz.  See the below clk_summary.
>>>>>
>>>>> Shouldn't this patch help exactly with that ?
>>>>
>>>> No, it doesn't help but only makes clk_round_rate() called in
>>>> LDB driver's .mode_valid() against 148.352MHz return 148.352MHz
>>>> which allows the unexpected 1920x1080-59.94Hz display mode.
>>>>
>>>>>
>>>>> It should allow you to set video_pll1_out to whatever is necessary by LDB first, fixate that frequency, and the LCDIFv3 would then be forced to use /7 divider from faster Video PLL1 , right ?
>>>>
>>>> Yes, it allows that for single-link LVDS use cases.
>>>> And, __no__, for dual-link LVDS use cases because the
>>>> video_pll1_out clock rate needs to be 2x the LVDS serial clock
>>>> rate. 
>>>>
>>>>>
>>>>>>      video_pll1_ref_sel               1       1        0        24000000    0          0     50000      Y      deviceless                      no_connection_id
>>>>>>         video_pll1                    1       1        0        519232000   0          0     50000      Y         deviceless                      no_connection_id
>>>>>>            video_pll1_bypass          1       1        0        519232000   0          0     50000      Y            deviceless                      no_connection_id
>>>>>>               video_pll1_out          2       2        0        519232000   0          0     50000      Y               deviceless                      no_connection_id
>>>>>>                  media_ldb            1       1        0        519232000   0          0     50000      Y                  deviceless                      no_connection_id
>>>>>>                     media_ldb_root_clk 1       1        0        519232000   0          0     50000      Y                     32ec0000.blk-ctrl:bridge@5c     ldb
>>>>>>                                                                                                                               deviceless                      no_connection_id
>>>>>>                  media_disp1_pix      0       0        0        129808000   0          0     50000      N                  deviceless                      no_connection_id
>>>>>>                     media_disp1_pix_root_clk 0       0        0        129808000   0          0     50000      N                     32e80000.display-controller     pix
>>>>>>                                                                                                                               32ec0000.blk-ctrl               disp1
>>>>>>                                                                                                                               deviceless                      no_connection_id
>>>>>>                  media_disp2_pix      1       1        0        519232000   0          0     50000      Y                  deviceless                      no_connection_id
>>>>>>                     media_disp2_pix_root_clk 1       1        0        519232000   0          0     50000      Y                     32e90000.display-controller     pix
>>>>>>                                                                                                                               32ec0000.blk-ctrl               disp2
>>>>>>                                                                                                                               deviceless                      no_connection_id
>>>>>>
>>>>>> Single LVDS link mode is not affected because "media_disp2_pix"
>>>>>> clock can handle the 7 division ratio.
>>>>>>
>>>>>> To support the dual LVDS link mode, "video_pll1" clock rate needs
>>>>>> to be x2 "media_ldb" clock rate so that "media_disp2_pix" clock
>>>>>> can use 7 division ratio to achieve the /3.5 clock rate comparing
>>>>>> to "media_ldb" clock rate.  However, "video_pll1" is not seen by
>>>>>> LDB driver thus not directly controlled by it.  This is another
>>>>>> reason why assigning a reasonable "video_pll1" clock rate in DT
>>>>>> makes sense.
>>>>>
>>>>> I agree that _right_now_, the DT clock assignment is the only option.
>>>>> I would like to see that DT part disappear and instead would prefer if the LDB/LCDIF could figure out the clock tree configuration themselves.
>>>>
>>>> I think we'll live with the assigned clock rate in DT, because the
>>>> i.MX8MP LDB and Samsung MIPI DSI display pipelines need to share a
>>>> video PLL, like I mentioned in comments for patch 2.
>>>
>>> Guys. There's 4 different discussions that look to be on the same topic,
>>> and doing workarounds in the DT, DRM driver and clock driver for
>>> something that looks like a broken clock.
>>
>> This is a bit complicated, because it is related to i.MX8MP MIPI DSI/
>> LVDS/HDMI, i.MX93 MIPI DSI/LVDS/parallel display pipelines.  Even
>> i.MX6SX LVDS display pipeline is a bit related, since i.MX8MP/i.MX93/
>> i.MX6SX LDBs share the same fsl-ldb.c driver.
>>
>>>
>>> Could we have *somewhere* a proper description of what the problem is
>>> exactly, so we can review it? Because at the moment, it's certainly not
>>> helping.
>>
>> Can you please suggest a place where this could happen?
> 
> Here, by mail will be good. Worst case scenario using a ascii art.

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

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

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

> 
> Maxime

-- 
Regards,
Liu Ying

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

[Marek Vasut]

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

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