[PATCH v7 0/7] Add ITE IT6263 LVDS to HDMI converter support

[PATCH v7 0/7] Add ITE IT6263 LVDS to HDMI converter support

[Liu Ying]

This patch series aims to add ITE IT6263 LVDS to HDMI converter on
i.MX8MP EVK.

Since IT6263 DT binding and driver were picked up from v5 and landed
in drm-misc, this patch series contains patches almost all i.MX8MP
SoC/platform specific.

Patch 1 is a preparation patch to allow display mode of an existing
panel to pass the added mode validation logic in patch 3.

Patch 2 is a preparation patch to drop CLK_SET_RATE_PARENT flag for
media_disp{1,2}_pix clocks.  Patch 5 depends on patch 2.

Patch 3 allows i.MX8MP LVDS Display Bridge(LDB) bridge driver to find
the next non-panel bridge, that is the IT6263 in this case.

Patch 4 adds mode validation logic to i.MX8MP LDB bridge driver against
"ldb" clock so that it can filter out unsupported display modes read
from EDID.

Patch 5 adds mode validation logic to i.MX8MP LDB bridge driver against
"pix" clock so that it can filter out display modes which are not
supported by pixel clock tree.

Patch 6 adds DT overlays to support NXP adapter cards[1][2] with IT6263
populated.

Patch 7 enables the IT6263 bridge driver in defconfig.

Note that patch 3 and 4 depend on patch[3] in shawnguo/imx/fixes.

Since this patch series is related to another one[4] authored by Marek,
Maxime asked for a proper description[5] about the exact problem.

Admittedly, it's a bit complicated.  Here, I'm trying to do so and explain
a bit more.

[ Description ]
It's a clock problem about shared i.MX8MP video PLL between MIPI DSI and
LVDS display pipelines.  The pipelines are driven by separate DRM driver
instances, hence there is no way to negotiate a dynamically changeable
PLL rate to satisfy both of them.  The only solution is to assign a
sensible/unchangeable clock rate for the PLL in DT.

Admittedly, sys_pll3_out can be another clock source to derive pixel clock
for i.MX8MP MIPI DSI display pipeline if a particalur i.MX8MP platform
doesn't use audio(sys_pll3_out is supposed to derive audio AXI clock running
at nominal 600MHz).  However, for i.MX8MP platforms with audio features,
the shared video PLL case has to be handled and it determines that the above
solution(unchangeable PLL rate assigned in DT) has to be used no matter
sys_pll3_out is for display or audio, because the separate DRM driver
instances really don't know if they are sharing the video PLL or not.

[[ i.MX8MP Display Hardware ]]
i.MX8MP SoC supports three display pipelines:

 -----------------------------           ------------------------
| imx8mp_media_disp_pix_sels  |         | imx8mp_media_ldb_sels  |
 -----------------------------           ------------------------
|  osc_24m (fixed 24MHz)      |         |  osc_24m (fixed 24MHz) |
|*-video_pll1_out (video)     |         |  sys_pll2_333m (sys)   |
|  audio_pll2_out (audio)     |         |  sys_pll2_100m (sys)   |
|  audio_pll1_out (audio)     |         | -sys_pll1_800m (sys)   |
| -sys_pll1_800m (sys)        |         | -sys_pll2_1000m (sys)  |
| -sys_pll2_1000m (sys)       |         |  clk_ext2 (external)   |
|  sys_pll3_out (audio ?)     |         |  audio_pll2_out (audio)|
|  clk_ext4 (external)        |         |*-video_pll1_out (video)|
 -----------------------------           ------------------------
             ||                                     |
 -----------------------------           ------------------------
|   media_disp{1,2}_pix       |         |        media_ldb       |
 ----------------------------- mux+div   ------------------------ mux+div
             ||                                     |
 -----------------------------           ------------------------
| media_disp{1,2}_pix_root_clk|         |   media_ldb_root_clk   |
 ----------------------------- gate      ------------------------ gate
             ||                                     | (LVDS serial clock)
             ||                                     V
	     || (Disp2 Pclk)  --------      ------------------
	     | ------------> | LCDIF2 | -> |       LDB        | -> panel/bridge
	     |                --------      ------------------
	     |  (Disp1 Pclk)  --------      ------------------
	      -------------> | LCDIF1 | -> | Samsung MIPI DSI | -> panel/bridge
	                      --------      ------------------
                              --------      ------------------      ----------
                             | LCDIF3 | -> | Synopsys HDMI TX | -> | HDMI PHY |
                              --------      ------------------     |     +    |
                                 ^                                 |    PLL   |
                                 |                                  ----------
                                 | (Disp3 pclk)                         | |
                                  --------------------------------------  |
                                                                          V
                                                                    panel/bridge

* video_pll1_out is supposed to be used by video outputs.

- LCDIF2 + LDB can only use the *same* video_pll1_out, sys_pll1_800m or
  sys_pll2_1000m.

[[ i.MX8MP Display Drivers ]]
LCDIF: drivers/gpu/drm/mxsfb/lcdif_*.c
Three LCDIFv3 display controllers are driven by three imx-lcdif DRM instances
separately.

LDB: drivers/gpu/drm/bridge/fsl-ldb.c

Samsung MIPI DSI: drivers/gpu/drm/bridge/samsung-dsim.c

Synopsys HDMI TX: drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c

[[ Problem - Shared Video PLL Between Samsung MIPI DSI and LDB ]]
osc_24m, audio_pll*, sys_pll* and clk_ext* are not for video outputs,
because:
a. Aparently, osc_24m runs at fixed 24MHz which is too low for most displays.
b. Audio subsystem may consume all audio_pll*.
c. sys_pll* are system clocks which are supposed to run at fixed typical
   rates, e.g., sys_pll2_1000m runs at 1000MHz.
d. sys_pll3_out is supposed to derive audio AXI clock running at nominal
   600MHz(i.MX8MP data sheet specifies the rate), see NXP downstream kernel:
   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
e. clk_ext* are external clocks without known capabilities.

So, the only eligible video_pll1_out is supposed to be shared between LDB
and Samsung MIPI DSI in the two separate display pipelines if sys_pll3_out
is already used to derive the audio AXI clock.

With the shared video_pll1_out, drivers for the two display pipelines cannot
change the PLL clock rate in runtime, since the pipelines are driven by two
DRM driver instances.

[[ Solution ]]
Assign the PLL clock source(s) and the PLL clock rate(s) in DT.  Disallow
display drivers to change the PLL clock source(s) or rate(s) in runtime
including LCDIF driver and bridge drivers.  With sensible PLL clock rate(s),
typical display modes like 1920x1080@60 can be supported if external HDMI
bridges are connected, and panel display modes can be too.  Also the unneeded
CLK_SET_RATE_PARENT flag can be dropped for media_disp{1,2}_pix clocks.
If needed, bridge drivers just call clk_round_rate() to validate clocks so
that unsupported display modes can be filtered out.  Although the
unchangeable PLL clock rate disallows more potential display modes, the
solution works for single/dual/triple display pipelines(OFC, hardware designers
should pick panel/bridge display devices carefully first by considering clock
resources).

[1] https://www.nxp.com/part/IMX-LVDS-HDMI
[2] https://www.nxp.com/part/IMX-DLVDS-HDMI
[3] https://patchwork.kernel.org/project/linux-arm-kernel/patch/20241017031146.157996-1-marex@denx.de/
[4] https://patchwork.kernel.org/project/linux-arm-kernel/list/?series=896900&state=%2A&archive=both
[5] https://lore.kernel.org/linux-arm-kernel/3341a6a7-ac0e-4594-a670-b3a6d583b344@nxp.com/T/#m587e6a25bdab542d5d99abbf01caaca89495b1d5

v7:
* Put pixel clock properly by adding a dev managed action in fsl_ldb_probe()
  in patch 5.
* Collect R-b tag on patch 7.

v6:
* Drop CLK_SET_RATE_PARENT flag for media_disp{1,2}_pix clocks in patch 2.
* Get pixel clock from display controller's OF node and validate it's
  clock rate in patch 5 instead of taking the sibling "ldb "clock as
  pixel clock in patch 4.

Liu Ying (7):
  arm64: dts: imx8mp-skov-revb-mi1010ait-1cp1: Set "media_disp2_pix"
    clock rate to 70MHz
  Revert "clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure
    parent rate"
  drm/bridge: fsl-ldb: Get the next non-panel bridge
  drm/bridge: fsl-ldb: Use clk_round_rate() to validate "ldb" clock rate
  drm/bridge: fsl-ldb: Use clk_round_rate() to validate "pix" clock rate
  arm64: dts: imx8mp-evk: Add NXP LVDS to HDMI adapter cards
  arm64: defconfig: Enable ITE IT6263 driver

 arch/arm64/boot/dts/freescale/Makefile        |  8 ++
 .../imx8mp-evk-imx-lvds-hdmi-common.dtsi      | 29 +++++++
 ...8mp-evk-lvds0-imx-dlvds-hdmi-channel0.dtso | 44 ++++++++++
 ...imx8mp-evk-lvds0-imx-lvds-hdmi-common.dtsi | 43 ++++++++++
 .../imx8mp-evk-lvds0-imx-lvds-hdmi.dtso       | 28 ++++++
 ...8mp-evk-lvds1-imx-dlvds-hdmi-channel0.dtso | 44 ++++++++++
 ...imx8mp-evk-lvds1-imx-lvds-hdmi-common.dtsi | 43 ++++++++++
 .../imx8mp-evk-lvds1-imx-lvds-hdmi.dtso       | 28 ++++++
 arch/arm64/boot/dts/freescale/imx8mp-evk.dts  |  6 ++
 .../imx8mp-skov-revb-mi1010ait-1cp1.dts       |  8 +-
 arch/arm64/configs/defconfig                  |  1 +
 drivers/clk/imx/clk-imx8mp.c                  |  4 +-
 drivers/clk/imx/clk.h                         |  4 -
 drivers/gpu/drm/bridge/fsl-ldb.c              | 86 ++++++++++++-------
 14 files changed, 337 insertions(+), 39 deletions(-)
 create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-evk-imx-lvds-hdmi-common.dtsi
 create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-evk-lvds0-imx-dlvds-hdmi-channel0.dtso
 create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-evk-lvds0-imx-lvds-hdmi-common.dtsi
 create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-evk-lvds0-imx-lvds-hdmi.dtso
 create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-evk-lvds1-imx-dlvds-hdmi-channel0.dtso
 create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-evk-lvds1-imx-lvds-hdmi-common.dtsi
 create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-evk-lvds1-imx-lvds-hdmi.dtso

-- 
2.34.1

[PATCH v7 1/7] arm64: dts: imx8mp-skov-revb-mi1010ait-1cp1: Set "media_disp2_pix" clock rate to 70MHz

[Liu Ying]

The LVDS panel "multi-inno,mi1010ait-1cp" used on this platform has
a typical pixel clock rate of 70MHz.  Set "media_disp2_pix" clock rate
to that rate, instead of the original 68.9MHz.  The LVDS serial clock
is controlled by "media_ldb" clock.  It should run at 490MHz(7-fold the
pixel clock rate due to single LVDS link).  Set "video_pll1" clock rate
and "media_ldb" to 490MHz to achieve that.

This should be able to suppress this LDB driver warning:
[   17.206644] fsl-ldb 32ec0000.blk-ctrl:bridge@5c: Configured LDB clock (70000000 Hz) does not match requested LVDS clock: 490000000 Hz

This also makes the display mode used by the panel pass mode validation
against pixel clock rate and "media_ldb" clock rate in a certain display
driver.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
v7:
* No change.

v6:
* No change.

v5:
* No change.

v4:
* No change.

v3:
* New patch.

 .../dts/freescale/imx8mp-skov-revb-mi1010ait-1cp1.dts     | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mp-skov-revb-mi1010ait-1cp1.dts b/arch/arm64/boot/dts/freescale/imx8mp-skov-revb-mi1010ait-1cp1.dts
index 30962922b361..2c75da5f064f 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-skov-revb-mi1010ait-1cp1.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mp-skov-revb-mi1010ait-1cp1.dts
@@ -52,7 +52,7 @@ &lcdif2 {
 
 &lvds_bridge {
 	/* IMX8MP_CLK_MEDIA_LDB = IMX8MP_CLK_MEDIA_DISP2_PIX * 7 */
-	assigned-clock-rates = <482300000>;
+	assigned-clock-rates = <490000000>;
 	status = "okay";
 
 	ports {
@@ -70,10 +70,10 @@ &media_blk_ctrl {
 	 */
 	assigned-clock-rates = <500000000>, <200000000>, <0>,
 		/* IMX8MP_CLK_MEDIA_DISP2_PIX = pixelclk of lvds panel */
-		<68900000>,
+		<70000000>,
 		<500000000>,
-		/* IMX8MP_VIDEO_PLL1 = IMX8MP_CLK_MEDIA_LDB * 2 */
-		<964600000>;
+		/* IMX8MP_VIDEO_PLL1 = IMX8MP_CLK_MEDIA_LDB */
+		<490000000>;
 };
 
 &pwm4 {
-- 
2.34.1

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

[Liu Ying]

This reverts commit ff06ea04e4cf3ba2f025024776e83bfbdfa05155.

media_disp1_pix clock is the pixel clock of the first i.MX8MP LCDIFv3
display controller, while media_disp2_pix clock is the pixel clock of
the second i.MX8MP LCDIFv3 display controller.  The two display
controllers connect with Samsung MIPI DSI controller and LVDS Display
Bridge(LDB) respectively.  Since the two display controllers are driven
by separate DRM driver instances and the two pixel clocks may be derived
from the same video_pll1_out clock(sys_pll3_out clock could be already
used to derive audio_axi clock), there is no way to negotiate a dynamically
changeable video_pll1_out clock rate to satisfy both of the two display
controllers.  In this case, the only solution to drive them with the
single video_pll1_out clock is to assign a sensible/unchangeable clock
rate for video_pll1_out clock.  Thus, there is no need to set the
CLK_SET_RATE_PARENT flag for media_disp{1,2}_pix clocks, drop it then.

Fixes: ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
v7:
* No change.

v6:
* New patch.

 drivers/clk/imx/clk-imx8mp.c | 4 ++--
 drivers/clk/imx/clk.h        | 4 ----
 2 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/imx/clk-imx8mp.c b/drivers/clk/imx/clk-imx8mp.c
index 516dbd170c8a..e561ff7b135f 100644
--- a/drivers/clk/imx/clk-imx8mp.c
+++ b/drivers/clk/imx/clk-imx8mp.c
@@ -547,7 +547,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
 	hws[IMX8MP_CLK_AHB] = imx8m_clk_hw_composite_bus_critical("ahb_root", imx8mp_ahb_sels, ccm_base + 0x9000);
 	hws[IMX8MP_CLK_AUDIO_AHB] = imx8m_clk_hw_composite_bus("audio_ahb", imx8mp_audio_ahb_sels, ccm_base + 0x9100);
 	hws[IMX8MP_CLK_MIPI_DSI_ESC_RX] = imx8m_clk_hw_composite_bus("mipi_dsi_esc_rx", imx8mp_mipi_dsi_esc_rx_sels, ccm_base + 0x9200);
-	hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300, CLK_SET_RATE_PARENT);
+	hws[IMX8MP_CLK_MEDIA_DISP2_PIX] = imx8m_clk_hw_composite_bus("media_disp2_pix", imx8mp_media_disp_pix_sels, ccm_base + 0x9300);
 
 	hws[IMX8MP_CLK_IPG_ROOT] = imx_clk_hw_divider2("ipg_root", "ahb_root", ccm_base + 0x9080, 0, 1);
 
@@ -609,7 +609,7 @@ static int imx8mp_clocks_probe(struct platform_device *pdev)
 	hws[IMX8MP_CLK_USDHC3] = imx8m_clk_hw_composite("usdhc3", imx8mp_usdhc3_sels, ccm_base + 0xbc80);
 	hws[IMX8MP_CLK_MEDIA_CAM1_PIX] = imx8m_clk_hw_composite("media_cam1_pix", imx8mp_media_cam1_pix_sels, ccm_base + 0xbd00);
 	hws[IMX8MP_CLK_MEDIA_MIPI_PHY1_REF] = imx8m_clk_hw_composite("media_mipi_phy1_ref", imx8mp_media_mipi_phy1_ref_sels, ccm_base + 0xbd80);
-	hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite_bus_flags("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00, CLK_SET_RATE_PARENT);
+	hws[IMX8MP_CLK_MEDIA_DISP1_PIX] = imx8m_clk_hw_composite("media_disp1_pix", imx8mp_media_disp_pix_sels, ccm_base + 0xbe00);
 	hws[IMX8MP_CLK_MEDIA_CAM2_PIX] = imx8m_clk_hw_composite("media_cam2_pix", imx8mp_media_cam2_pix_sels, ccm_base + 0xbe80);
 	hws[IMX8MP_CLK_MEDIA_LDB] = imx8m_clk_hw_composite("media_ldb", imx8mp_media_ldb_sels, ccm_base + 0xbf00);
 	hws[IMX8MP_CLK_MEMREPAIR] = imx8m_clk_hw_composite_critical("mem_repair", imx8mp_memrepair_sels, ccm_base + 0xbf80);
diff --git a/drivers/clk/imx/clk.h b/drivers/clk/imx/clk.h
index aa5202f284f3..adb7ad649a0d 100644
--- a/drivers/clk/imx/clk.h
+++ b/drivers/clk/imx/clk.h
@@ -442,10 +442,6 @@ struct clk_hw *__imx8m_clk_hw_composite(const char *name,
 	_imx8m_clk_hw_composite(name, parent_names, reg, \
 			IMX_COMPOSITE_BUS, IMX_COMPOSITE_CLK_FLAGS_DEFAULT)
 
-#define imx8m_clk_hw_composite_bus_flags(name, parent_names, reg, flags) \
-	_imx8m_clk_hw_composite(name, parent_names, reg, \
-			IMX_COMPOSITE_BUS, IMX_COMPOSITE_CLK_FLAGS_DEFAULT | flags)
-
 #define imx8m_clk_hw_composite_bus_critical(name, parent_names, reg)	\
 	_imx8m_clk_hw_composite(name, parent_names, reg, \
 			IMX_COMPOSITE_BUS, IMX_COMPOSITE_CLK_FLAGS_CRITICAL)
-- 
2.34.1

[PATCH v7 3/7] drm/bridge: fsl-ldb: Get the next non-panel bridge

[Liu Ying]

The next bridge in bridge chain could be a panel bridge or a non-panel
bridge.  Use devm_drm_of_get_bridge() to replace the combination
function calls of of_drm_find_panel() and devm_drm_panel_bridge_add()
to get either a panel bridge or a non-panel bridge, instead of getting
a panel bridge only.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
v7:
* No change.

v6:
* No change.

v5:
* No change.

v4:
* No change.

v3:
* Collect Dmitry' R-b tag.

v2:
* No change.

 drivers/gpu/drm/bridge/fsl-ldb.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
index 0e4bac7dd04f..b559f3e0bef6 100644
--- a/drivers/gpu/drm/bridge/fsl-ldb.c
+++ b/drivers/gpu/drm/bridge/fsl-ldb.c
@@ -15,7 +15,6 @@
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
 #include <drm/drm_of.h>
-#include <drm/drm_panel.h>
 
 #define LDB_CTRL_CH0_ENABLE			BIT(0)
 #define LDB_CTRL_CH0_DI_SELECT			BIT(1)
@@ -86,7 +85,7 @@ static const struct fsl_ldb_devdata fsl_ldb_devdata[] = {
 struct fsl_ldb {
 	struct device *dev;
 	struct drm_bridge bridge;
-	struct drm_bridge *panel_bridge;
+	struct drm_bridge *next_bridge;
 	struct clk *clk;
 	struct regmap *regmap;
 	const struct fsl_ldb_devdata *devdata;
@@ -117,7 +116,7 @@ static int fsl_ldb_attach(struct drm_bridge *bridge,
 {
 	struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
 
-	return drm_bridge_attach(bridge->encoder, fsl_ldb->panel_bridge,
+	return drm_bridge_attach(bridge->encoder, fsl_ldb->next_bridge,
 				 bridge, flags);
 }
 
@@ -292,9 +291,7 @@ static const struct drm_bridge_funcs funcs = {
 static int fsl_ldb_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *panel_node;
 	struct device_node *remote1, *remote2;
-	struct drm_panel *panel;
 	struct fsl_ldb *fsl_ldb;
 	int dual_link;
 
@@ -318,33 +315,27 @@ static int fsl_ldb_probe(struct platform_device *pdev)
 	if (IS_ERR(fsl_ldb->regmap))
 		return PTR_ERR(fsl_ldb->regmap);
 
-	/* Locate the remote ports and the panel node */
+	/* Locate the remote ports. */
 	remote1 = of_graph_get_remote_node(dev->of_node, 1, 0);
 	remote2 = of_graph_get_remote_node(dev->of_node, 2, 0);
 	fsl_ldb->ch0_enabled = (remote1 != NULL);
 	fsl_ldb->ch1_enabled = (remote2 != NULL);
-	panel_node = of_node_get(remote1 ? remote1 : remote2);
 	of_node_put(remote1);
 	of_node_put(remote2);
 
-	if (!fsl_ldb->ch0_enabled && !fsl_ldb->ch1_enabled) {
-		of_node_put(panel_node);
-		return dev_err_probe(dev, -ENXIO, "No panel node found");
-	}
+	if (!fsl_ldb->ch0_enabled && !fsl_ldb->ch1_enabled)
+		return dev_err_probe(dev, -ENXIO, "No next bridge node found");
 
 	dev_dbg(dev, "Using %s\n",
 		fsl_ldb_is_dual(fsl_ldb) ? "dual-link mode" :
 		fsl_ldb->ch0_enabled ? "channel 0" : "channel 1");
 
-	panel = of_drm_find_panel(panel_node);
-	of_node_put(panel_node);
-	if (IS_ERR(panel))
-		return PTR_ERR(panel);
-
-	fsl_ldb->panel_bridge = devm_drm_panel_bridge_add(dev, panel);
-	if (IS_ERR(fsl_ldb->panel_bridge))
-		return PTR_ERR(fsl_ldb->panel_bridge);
-
+	fsl_ldb->next_bridge = devm_drm_of_get_bridge(dev, dev->of_node,
+						      fsl_ldb->ch0_enabled ? 1 : 2,
+						      0);
+	if (IS_ERR(fsl_ldb->next_bridge))
+		return dev_err_probe(dev, PTR_ERR(fsl_ldb->next_bridge),
+				     "failed to get next bridge\n");
 
 	if (fsl_ldb_is_dual(fsl_ldb)) {
 		struct device_node *port1, *port2;
-- 
2.34.1

[PATCH v7 4/7] drm/bridge: fsl-ldb: Use clk_round_rate() to validate "ldb" clock rate

[Liu Ying]

Multiple display modes could be read from a display device's EDID.
Use clk_round_rate() to validate the "ldb" clock rate for each mode
in drm_bridge_funcs::mode_valid() to filter unsupported modes out.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
Note that this patch depends on a patch in shawnguo/imx/fixes:
https://patchwork.kernel.org/project/linux-arm-kernel/patch/20241017031146.157996-1-marex@denx.de/

v7:
* No change.

v6:
* Drop pixel clock rate validation.

v5:
* No change.

v4:
* No change.

v3:
* No change.

v2:
* Add more comments in fsl-ldb.c and commit message about pixel clock
  rate validation.  (Maxime)

 drivers/gpu/drm/bridge/fsl-ldb.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
index b559f3e0bef6..d9436ff9ccc3 100644
--- a/drivers/gpu/drm/bridge/fsl-ldb.c
+++ b/drivers/gpu/drm/bridge/fsl-ldb.c
@@ -270,10 +270,16 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
 		   const struct drm_display_mode *mode)
 {
 	struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
+	unsigned long link_freq;
 
 	if (mode->clock > (fsl_ldb_is_dual(fsl_ldb) ? 160000 : 80000))
 		return MODE_CLOCK_HIGH;
 
+	/* Validate "ldb" clock rate. */
+	link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock);
+	if (link_freq != clk_round_rate(fsl_ldb->clk, link_freq))
+		return MODE_NOCLOCK;
+
 	return MODE_OK;
 }
 
-- 
2.34.1

[PATCH v7 5/7] drm/bridge: fsl-ldb: Use clk_round_rate() to validate "pix" clock rate

[Liu Ying]

Same to "ldb" clock rate validation, call clk_round_rate() to validate
"pix"(pixel clock) rate too.  This may filter modes out whose pixel
clock rates cannot be supported by the pixel clock tree.  For example,
when the pixel clock is derived from the i.MX8MP video_pll1_out clock
and video_pll1_out clock rate is 1.0395GHz, mode 720x576p@50Hz with
27MHz pixel clock rate will be filtered out in LDB split mode because
the PLL clock rate does satisfy the "ldb" clock rate(27MHz * 3.5 = 94.5MHz)
with 11 division ratio while it cannot satisfy the "pix" clock rate
with 38.5 division ratio(only integer division ratio is supported).

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
Note that this patch depends on a patch in shawnguo/imx/fixes:
https://patchwork.kernel.org/project/linux-arm-kernel/patch/20241017031146.157996-1-marex@denx.de/
Also, this patch depends on patch 2.

v7:
* Put pixel clock properly by adding a dev managed action in fsl_ldb_probe().

v6:
* New patch.

 drivers/gpu/drm/bridge/fsl-ldb.c | 53 +++++++++++++++++++++++++-------
 1 file changed, 42 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/bridge/fsl-ldb.c b/drivers/gpu/drm/bridge/fsl-ldb.c
index d9436ff9ccc3..9afd91c30c88 100644
--- a/drivers/gpu/drm/bridge/fsl-ldb.c
+++ b/drivers/gpu/drm/bridge/fsl-ldb.c
@@ -4,6 +4,7 @@
  */
 
 #include <linux/clk.h>
+#include <linux/device.h>
 #include <linux/media-bus-format.h>
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
@@ -11,6 +12,7 @@
 #include <linux/of_graph.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
+#include <linux/units.h>
 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
@@ -86,7 +88,8 @@ struct fsl_ldb {
 	struct device *dev;
 	struct drm_bridge bridge;
 	struct drm_bridge *next_bridge;
-	struct clk *clk;
+	struct clk *clk_ldb;
+	struct clk *clk_pixel;
 	struct regmap *regmap;
 	const struct fsl_ldb_devdata *devdata;
 	bool ch0_enabled;
@@ -176,15 +179,15 @@ static void fsl_ldb_atomic_enable(struct drm_bridge *bridge,
 	mode = &crtc_state->adjusted_mode;
 
 	requested_link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock);
-	clk_set_rate(fsl_ldb->clk, requested_link_freq);
+	clk_set_rate(fsl_ldb->clk_ldb, requested_link_freq);
 
-	configured_link_freq = clk_get_rate(fsl_ldb->clk);
+	configured_link_freq = clk_get_rate(fsl_ldb->clk_ldb);
 	if (configured_link_freq != requested_link_freq)
 		dev_warn(fsl_ldb->dev, "Configured LDB clock (%lu Hz) does not match requested LVDS clock: %lu Hz\n",
 			 configured_link_freq,
 			 requested_link_freq);
 
-	clk_prepare_enable(fsl_ldb->clk);
+	clk_prepare_enable(fsl_ldb->clk_ldb);
 
 	/* Program LDB_CTRL */
 	reg =	(fsl_ldb->ch0_enabled ? LDB_CTRL_CH0_ENABLE : 0) |
@@ -237,7 +240,7 @@ static void fsl_ldb_atomic_disable(struct drm_bridge *bridge,
 			regmap_write(fsl_ldb->regmap, fsl_ldb->devdata->lvds_ctrl, 0);
 	regmap_write(fsl_ldb->regmap, fsl_ldb->devdata->ldb_ctrl, 0);
 
-	clk_disable_unprepare(fsl_ldb->clk);
+	clk_disable_unprepare(fsl_ldb->clk_ldb);
 }
 
 #define MAX_INPUT_SEL_FORMATS 1
@@ -269,15 +272,21 @@ fsl_ldb_mode_valid(struct drm_bridge *bridge,
 		   const struct drm_display_info *info,
 		   const struct drm_display_mode *mode)
 {
+	unsigned long link_freq, pclk_rate, rounded_pclk_rate;
 	struct fsl_ldb *fsl_ldb = to_fsl_ldb(bridge);
-	unsigned long link_freq;
 
 	if (mode->clock > (fsl_ldb_is_dual(fsl_ldb) ? 160000 : 80000))
 		return MODE_CLOCK_HIGH;
 
 	/* Validate "ldb" clock rate. */
 	link_freq = fsl_ldb_link_frequency(fsl_ldb, mode->clock);
-	if (link_freq != clk_round_rate(fsl_ldb->clk, link_freq))
+	if (link_freq != clk_round_rate(fsl_ldb->clk_ldb, link_freq))
+		return MODE_NOCLOCK;
+
+	/* Validate pixel clock rate. */
+	pclk_rate = mode->clock * HZ_PER_KHZ;
+	rounded_pclk_rate = clk_round_rate(fsl_ldb->clk_pixel, pclk_rate);
+	if (rounded_pclk_rate != pclk_rate)
 		return MODE_NOCLOCK;
 
 	return MODE_OK;
@@ -294,12 +303,20 @@ static const struct drm_bridge_funcs funcs = {
 	.mode_valid = fsl_ldb_mode_valid,
 };
 
+static void fsl_ldb_clk_pixel_put(void *data)
+{
+	struct fsl_ldb *fsl_ldb = data;
+
+	clk_put(fsl_ldb->clk_pixel);
+}
+
 static int fsl_ldb_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct device_node *remote1, *remote2;
+	struct device_node *remote0, *remote1, *remote2;
 	struct fsl_ldb *fsl_ldb;
 	int dual_link;
+	int ret;
 
 	fsl_ldb = devm_kzalloc(dev, sizeof(*fsl_ldb), GFP_KERNEL);
 	if (!fsl_ldb)
@@ -313,9 +330,23 @@ static int fsl_ldb_probe(struct platform_device *pdev)
 	fsl_ldb->bridge.funcs = &funcs;
 	fsl_ldb->bridge.of_node = dev->of_node;
 
-	fsl_ldb->clk = devm_clk_get(dev, "ldb");
-	if (IS_ERR(fsl_ldb->clk))
-		return PTR_ERR(fsl_ldb->clk);
+	fsl_ldb->clk_ldb = devm_clk_get(dev, "ldb");
+	if (IS_ERR(fsl_ldb->clk_ldb))
+		return PTR_ERR(fsl_ldb->clk_ldb);
+
+	/* Get pixel clock from display controller's OF node. */
+	remote0 = of_graph_get_remote_node(dev->of_node, 0, 0);
+	fsl_ldb->clk_pixel = of_clk_get_by_name(remote0, "pix");
+	of_node_put(remote0);
+	if (IS_ERR(fsl_ldb->clk_pixel))
+		return PTR_ERR(fsl_ldb->clk_pixel);
+
+	ret = devm_add_action_or_reset(dev, fsl_ldb_clk_pixel_put, fsl_ldb);
+	if (ret) {
+		dev_err(dev, "Failed to add pixel clock put devm action: %d\n",
+			ret);
+		return ret;
+	}
 
 	fsl_ldb->regmap = syscon_node_to_regmap(dev->of_node->parent);
 	if (IS_ERR(fsl_ldb->regmap))
-- 
2.34.1

[PATCH v7 6/7] arm64: dts: imx8mp-evk: Add NXP LVDS to HDMI adapter cards

[Liu Ying]

One ITE IT6263 LVDS to HDMI converter is populated on NXP IMX-LVDS-HDMI
and IMX-DLVDS-HDMI adapter cards.

Card IMX-LVDS-HDMI supports single LVDS link(IT6263 link1).
Card IMX-DLVDS-HDMI supports dual LVDS links(IT6263 link1 and link2).

Only one card can be enabled with one i.MX8MP EVK.

Add dedicated overlays to support the below four connections:
1) imx8mp-evk-lvds0-imx-lvds-hdmi.dtso:
   i.MX8MP EVK LVDS0 connector <=> LVDS adapter card J6(IT6263 link1)

2) imx8mp-evk-lvds1-imx-lvds-hdmi.dtso:
   i.MX8MP EVK LVDS1 connector <=> LVDS adapter card J6(IT6263 link1)

3) imx8mp-evk-lvds0-imx-dlvds-hdmi-channel0.dtso:
   i.MX8MP EVK LVDS0 connector <=> DLVDS adapter card channel0(IT6263 link1)
   i.MX8MP EVK LVDS1 connector <=> DLVDS adapter card channel1(IT6263 link2)

4) imx8mp-evk-lvds1-imx-dlvds-hdmi-channel0.dtso:
   i.MX8MP EVK LVDS1 connector <=> DLVDS adapter card channel0(IT6263 link1)
   i.MX8MP EVK LVDS0 connector <=> DLVDS adapter card channel1(IT6263 link2)

Part links:
https://www.nxp.com/part/IMX-LVDS-HDMI
https://www.nxp.com/part/IMX-DLVDS-HDMI

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
v7:
* No change.

v6:
* No change.

v5:
* No change.

v4:
* Rebase this patch upon next-20241025 to resolve conflicts when apply.

v3:
* Use data-mapping DT property instead of ite,lvds-link-num-data-lanes.
  (Dmitry, Biju)

v2:
* Add ite,lvds-link-num-data-lanes properties.

 arch/arm64/boot/dts/freescale/Makefile        |  8 ++++
 .../imx8mp-evk-imx-lvds-hdmi-common.dtsi      | 29 ++++++++++++
 ...8mp-evk-lvds0-imx-dlvds-hdmi-channel0.dtso | 44 +++++++++++++++++++
 ...imx8mp-evk-lvds0-imx-lvds-hdmi-common.dtsi | 43 ++++++++++++++++++
 .../imx8mp-evk-lvds0-imx-lvds-hdmi.dtso       | 28 ++++++++++++
 ...8mp-evk-lvds1-imx-dlvds-hdmi-channel0.dtso | 44 +++++++++++++++++++
 ...imx8mp-evk-lvds1-imx-lvds-hdmi-common.dtsi | 43 ++++++++++++++++++
 .../imx8mp-evk-lvds1-imx-lvds-hdmi.dtso       | 28 ++++++++++++
 arch/arm64/boot/dts/freescale/imx8mp-evk.dts  |  6 +++
 9 files changed, 273 insertions(+)
 create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-evk-imx-lvds-hdmi-common.dtsi
 create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-evk-lvds0-imx-dlvds-hdmi-channel0.dtso
 create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-evk-lvds0-imx-lvds-hdmi-common.dtsi
 create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-evk-lvds0-imx-lvds-hdmi.dtso
 create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-evk-lvds1-imx-dlvds-hdmi-channel0.dtso
 create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-evk-lvds1-imx-lvds-hdmi-common.dtsi
 create mode 100644 arch/arm64/boot/dts/freescale/imx8mp-evk-lvds1-imx-lvds-hdmi.dtso

diff --git a/arch/arm64/boot/dts/freescale/Makefile b/arch/arm64/boot/dts/freescale/Makefile
index 42e6482a31cb..e2e828b352e7 100644
--- a/arch/arm64/boot/dts/freescale/Makefile
+++ b/arch/arm64/boot/dts/freescale/Makefile
@@ -211,8 +211,16 @@ dtb-$(CONFIG_ARCH_MXC) += imx8mp-verdin-wifi-ivy.dtb
 dtb-$(CONFIG_ARCH_MXC) += imx8mp-verdin-wifi-mallow.dtb
 dtb-$(CONFIG_ARCH_MXC) += imx8mp-verdin-wifi-yavia.dtb
 
+imx8mp-evk-lvds0-imx-dlvds-hdmi-channel0-dtbs += imx8mp-evk.dtb imx8mp-evk-lvds0-imx-dlvds-hdmi-channel0.dtbo
+imx8mp-evk-lvds0-imx-lvds-hdmi-dtbs += imx8mp-evk.dtb imx8mp-evk-lvds0-imx-lvds-hdmi.dtbo
+imx8mp-evk-lvds1-imx-dlvds-hdmi-channel0-dtbs += imx8mp-evk.dtb imx8mp-evk-lvds1-imx-dlvds-hdmi-channel0.dtbo
+imx8mp-evk-lvds1-imx-lvds-hdmi-dtbs += imx8mp-evk.dtb imx8mp-evk-lvds1-imx-lvds-hdmi.dtbo
 imx8mp-evk-mx8-dlvds-lcd1-dtbs += imx8mp-evk.dtb imx8mp-evk-mx8-dlvds-lcd1.dtbo
 imx8mp-evk-pcie-ep-dtbs += imx8mp-evk.dtb imx8mp-evk-pcie-ep.dtbo
+dtb-$(CONFIG_ARCH_MXC) += imx8mp-evk-lvds0-imx-dlvds-hdmi-channel0.dtb
+dtb-$(CONFIG_ARCH_MXC) += imx8mp-evk-lvds0-imx-lvds-hdmi.dtb
+dtb-$(CONFIG_ARCH_MXC) += imx8mp-evk-lvds1-imx-dlvds-hdmi-channel0.dtb
+dtb-$(CONFIG_ARCH_MXC) += imx8mp-evk-lvds1-imx-lvds-hdmi.dtb
 dtb-$(CONFIG_ARCH_MXC) += imx8mp-evk-mx8-dlvds-lcd1.dtb
 dtb-$(CONFIG_ARCH_MXC) += imx8mp-evk-pcie-ep.dtb
 
diff --git a/arch/arm64/boot/dts/freescale/imx8mp-evk-imx-lvds-hdmi-common.dtsi b/arch/arm64/boot/dts/freescale/imx8mp-evk-imx-lvds-hdmi-common.dtsi
new file mode 100644
index 000000000000..44b30e9b3fde
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/imx8mp-evk-imx-lvds-hdmi-common.dtsi
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright 2024 NXP
+ */
+
+/dts-v1/;
+/plugin/;
+
+&{/} {
+	lvds-hdmi-connector {
+		compatible = "hdmi-connector";
+		label = "J2";
+		type = "a";
+
+		port {
+			lvds2hdmi_connector_in: endpoint {
+				remote-endpoint = <&it6263_out>;
+			};
+		};
+	};
+};
+
+&lcdif2 {
+	status = "okay";
+};
+
+&lvds_bridge {
+	status = "okay";
+};
diff --git a/arch/arm64/boot/dts/freescale/imx8mp-evk-lvds0-imx-dlvds-hdmi-channel0.dtso b/arch/arm64/boot/dts/freescale/imx8mp-evk-lvds0-imx-dlvds-hdmi-channel0.dtso
new file mode 100644
index 000000000000..4008d2fd36d6
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/imx8mp-evk-lvds0-imx-dlvds-hdmi-channel0.dtso
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright 2024 NXP
+ */
+
+#include "imx8mp-evk-lvds0-imx-lvds-hdmi-common.dtsi"
+
+&it6263 {
+	ports {
+		port@0 {
+			reg = <0>;
+			dual-lvds-odd-pixels;
+
+			it6263_lvds_link1: endpoint {
+				remote-endpoint = <&ldb_lvds_ch0>;
+			};
+		};
+
+		port@1 {
+			reg = <1>;
+			dual-lvds-even-pixels;
+
+			it6263_lvds_link2: endpoint {
+				remote-endpoint = <&ldb_lvds_ch1>;
+			};
+		};
+	};
+};
+
+&lvds_bridge {
+	ports {
+		port@1 {
+			ldb_lvds_ch0: endpoint {
+				remote-endpoint = <&it6263_lvds_link1>;
+			};
+		};
+
+		port@2 {
+			ldb_lvds_ch1: endpoint {
+				remote-endpoint = <&it6263_lvds_link2>;
+			};
+		};
+	};
+};
diff --git a/arch/arm64/boot/dts/freescale/imx8mp-evk-lvds0-imx-lvds-hdmi-common.dtsi b/arch/arm64/boot/dts/freescale/imx8mp-evk-lvds0-imx-lvds-hdmi-common.dtsi
new file mode 100644
index 000000000000..6eae7477abf8
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/imx8mp-evk-lvds0-imx-lvds-hdmi-common.dtsi
@@ -0,0 +1,43 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright 2024 NXP
+ */
+
+#include <dt-bindings/gpio/gpio.h>
+#include "imx8mp-evk-imx-lvds-hdmi-common.dtsi"
+
+&i2c2 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	it6263: hdmi@4c {
+		compatible = "ite,it6263";
+		reg = <0x4c>;
+		data-mapping = "jeida-24";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_lvds_en>;
+		reset-gpios = <&gpio1 10 GPIO_ACTIVE_LOW>;
+		ivdd-supply = <&reg_buck5>;
+		ovdd-supply = <&reg_vext_3v3>;
+		txavcc18-supply = <&reg_buck5>;
+		txavcc33-supply = <&reg_vext_3v3>;
+		pvcc1-supply = <&reg_buck5>;
+		pvcc2-supply = <&reg_buck5>;
+		avcc-supply = <&reg_vext_3v3>;
+		anvdd-supply = <&reg_buck5>;
+		apvdd-supply = <&reg_buck5>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@2 {
+				reg = <2>;
+
+				it6263_out: endpoint {
+					remote-endpoint = <&lvds2hdmi_connector_in>;
+				};
+			};
+		};
+	};
+};
diff --git a/arch/arm64/boot/dts/freescale/imx8mp-evk-lvds0-imx-lvds-hdmi.dtso b/arch/arm64/boot/dts/freescale/imx8mp-evk-lvds0-imx-lvds-hdmi.dtso
new file mode 100644
index 000000000000..9e11f261ad13
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/imx8mp-evk-lvds0-imx-lvds-hdmi.dtso
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright 2024 NXP
+ */
+
+#include "imx8mp-evk-lvds0-imx-lvds-hdmi-common.dtsi"
+
+&it6263 {
+	ports {
+		port@0 {
+			reg = <0>;
+
+			it6263_lvds_link1: endpoint {
+				remote-endpoint = <&ldb_lvds_ch0>;
+			};
+		};
+	};
+};
+
+&lvds_bridge {
+	ports {
+		port@1 {
+			ldb_lvds_ch0: endpoint {
+				remote-endpoint = <&it6263_lvds_link1>;
+			};
+		};
+	};
+};
diff --git a/arch/arm64/boot/dts/freescale/imx8mp-evk-lvds1-imx-dlvds-hdmi-channel0.dtso b/arch/arm64/boot/dts/freescale/imx8mp-evk-lvds1-imx-dlvds-hdmi-channel0.dtso
new file mode 100644
index 000000000000..af2e73e36a1b
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/imx8mp-evk-lvds1-imx-dlvds-hdmi-channel0.dtso
@@ -0,0 +1,44 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright 2024 NXP
+ */
+
+#include "imx8mp-evk-lvds1-imx-lvds-hdmi-common.dtsi"
+
+&it6263 {
+	ports {
+		port@0 {
+			reg = <0>;
+			dual-lvds-even-pixels;
+
+			it6263_lvds_link1: endpoint {
+				remote-endpoint = <&ldb_lvds_ch1>;
+			};
+		};
+
+		port@1 {
+			reg = <1>;
+			dual-lvds-odd-pixels;
+
+			it6263_lvds_link2: endpoint {
+				remote-endpoint = <&ldb_lvds_ch0>;
+			};
+		};
+	};
+};
+
+&lvds_bridge {
+	ports {
+		port@1 {
+			ldb_lvds_ch0: endpoint {
+				remote-endpoint = <&it6263_lvds_link2>;
+			};
+		};
+
+		port@2 {
+			ldb_lvds_ch1: endpoint {
+				remote-endpoint = <&it6263_lvds_link1>;
+			};
+		};
+	};
+};
diff --git a/arch/arm64/boot/dts/freescale/imx8mp-evk-lvds1-imx-lvds-hdmi-common.dtsi b/arch/arm64/boot/dts/freescale/imx8mp-evk-lvds1-imx-lvds-hdmi-common.dtsi
new file mode 100644
index 000000000000..8cc9d361c2a4
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/imx8mp-evk-lvds1-imx-lvds-hdmi-common.dtsi
@@ -0,0 +1,43 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright 2024 NXP
+ */
+
+#include <dt-bindings/gpio/gpio.h>
+#include "imx8mp-evk-imx-lvds-hdmi-common.dtsi"
+
+&i2c3 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	it6263: hdmi@4c {
+		compatible = "ite,it6263";
+		reg = <0x4c>;
+		data-mapping = "jeida-24";
+		pinctrl-names = "default";
+		pinctrl-0 = <&pinctrl_lvds_en>;
+		reset-gpios = <&gpio1 10 GPIO_ACTIVE_LOW>;
+		ivdd-supply = <&reg_buck5>;
+		ovdd-supply = <&reg_vext_3v3>;
+		txavcc18-supply = <&reg_buck5>;
+		txavcc33-supply = <&reg_vext_3v3>;
+		pvcc1-supply = <&reg_buck5>;
+		pvcc2-supply = <&reg_buck5>;
+		avcc-supply = <&reg_vext_3v3>;
+		anvdd-supply = <&reg_buck5>;
+		apvdd-supply = <&reg_buck5>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@2 {
+				reg = <2>;
+
+				it6263_out: endpoint {
+					remote-endpoint = <&lvds2hdmi_connector_in>;
+				};
+			};
+		};
+	};
+};
diff --git a/arch/arm64/boot/dts/freescale/imx8mp-evk-lvds1-imx-lvds-hdmi.dtso b/arch/arm64/boot/dts/freescale/imx8mp-evk-lvds1-imx-lvds-hdmi.dtso
new file mode 100644
index 000000000000..527a893a71b2
--- /dev/null
+++ b/arch/arm64/boot/dts/freescale/imx8mp-evk-lvds1-imx-lvds-hdmi.dtso
@@ -0,0 +1,28 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright 2024 NXP
+ */
+
+#include "imx8mp-evk-lvds1-imx-lvds-hdmi-common.dtsi"
+
+&it6263 {
+	ports {
+		port@0 {
+			reg = <0>;
+
+			it6263_lvds_link1: endpoint {
+				remote-endpoint = <&ldb_lvds_ch1>;
+			};
+		};
+	};
+};
+
+&lvds_bridge {
+	ports {
+		port@2 {
+			ldb_lvds_ch1: endpoint {
+				remote-endpoint = <&it6263_lvds_link1>;
+			};
+		};
+	};
+};
diff --git a/arch/arm64/boot/dts/freescale/imx8mp-evk.dts b/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
index d26930f1a9e9..68e12a752edd 100644
--- a/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
+++ b/arch/arm64/boot/dts/freescale/imx8mp-evk.dts
@@ -938,6 +938,12 @@ MX8MP_IOMUXC_SPDIF_TX__I2C5_SCL         0x400001c2
 		>;
 	};
 
+	pinctrl_lvds_en: lvdsengrp {
+		fsl,pins = <
+			MX8MP_IOMUXC_GPIO1_IO10__GPIO1_IO10	0x1c0
+		>;
+	};
+
 	pinctrl_pcie0: pcie0grp {
 		fsl,pins = <
 			MX8MP_IOMUXC_I2C4_SCL__PCIE_CLKREQ_B	0x60 /* open drain, pull up */
-- 
2.34.1

[PATCH v7 7/7] arm64: defconfig: Enable ITE IT6263 driver

[Liu Ying]

ITE IT6263 LVDS to HDMI converter is populated on NXP IMX-LVDS-HDMI
and IMX-DLVDS-HDMI adapter cards.  The adapter cards can connect to
i.MX8MP EVK base board to support video output through HDMI connectors.
Build the ITE IT6263 driver as a module.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
---
v7:
* Collect Biju's R-b tag.

v6:
* No change.

v5:
* No change.

v4:
* No change.

v3:
* No change.

v2:
* No change.

 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index d13218d0c30f..9b20b75f82e2 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -901,6 +901,7 @@ CONFIG_DRM_PANEL_SITRONIX_ST7703=m
 CONFIG_DRM_PANEL_TRULY_NT35597_WQXGA=m
 CONFIG_DRM_PANEL_VISIONOX_VTDR6130=m
 CONFIG_DRM_FSL_LDB=m
+CONFIG_DRM_ITE_IT6263=m
 CONFIG_DRM_LONTIUM_LT8912B=m
 CONFIG_DRM_LONTIUM_LT9611=m
 CONFIG_DRM_LONTIUM_LT9611UXC=m
-- 
2.34.1

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

[Peng Fan]

> Subject: [PATCH v7 2/7] Revert "clk: imx: clk-imx8mp: Allow
> media_disp pixel clock reconfigure parent rate"
> 
> This reverts commit ff06ea04e4cf3ba2f025024776e83bfbdfa05155.
> 
> media_disp1_pix clock is the pixel clock of the first i.MX8MP LCDIFv3
> display controller, while media_disp2_pix clock is the pixel clock of the
> second i.MX8MP LCDIFv3 display controller.  The two display
> controllers connect with Samsung MIPI DSI controller and LVDS Display
> Bridge(LDB) respectively.  Since the two display controllers are driven
> by separate DRM driver instances and the two pixel clocks may be
> derived from the same video_pll1_out clock(sys_pll3_out clock could
> be already used to derive audio_axi clock), there is no way to negotiate
> a dynamically changeable video_pll1_out clock rate to satisfy both of
> the two display controllers.  In this case, the only solution to drive
> them with the single video_pll1_out clock is to assign a
> sensible/unchangeable clock rate for video_pll1_out clock.  Thus, there
> is no need to set the CLK_SET_RATE_PARENT flag for
> media_disp{1,2}_pix clocks, drop it then.
> 
> Fixes: ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel
> clock reconfigure parent rate")
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---

Acked-by: Peng Fan <peng.fan@nxp.com>

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

[Marek Vasut]

On 11/14/24 7:57 AM, Liu Ying wrote:
> This reverts commit ff06ea04e4cf3ba2f025024776e83bfbdfa05155.
> 
> media_disp1_pix clock is the pixel clock of the first i.MX8MP LCDIFv3
> display controller, while media_disp2_pix clock is the pixel clock of
> the second i.MX8MP LCDIFv3 display controller.  The two display
> controllers connect with Samsung MIPI DSI controller and LVDS Display
> Bridge(LDB) respectively.  Since the two display controllers are driven
> by separate DRM driver instances and the two pixel clocks may be derived
> from the same video_pll1_out clock(sys_pll3_out clock could be already
> used to derive audio_axi clock), there is no way to negotiate a dynamically
> changeable video_pll1_out clock rate to satisfy both of the two display
> controllers.  In this case, the only solution to drive them with the
> single video_pll1_out clock is to assign a sensible/unchangeable clock
> rate for video_pll1_out clock.  Thus, there is no need to set the
> CLK_SET_RATE_PARENT flag for media_disp{1,2}_pix clocks, drop it then.
> 
> Fixes: ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock reconfigure parent rate")
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
Uh, I almost missed this revert between all the LDB patches.

This revert will break my usecase on MX8MP where I need to operate two 
disparate panels attached to LVDS and TC358767 DSI-to-DP bridge and I 
need accurate pixel clock for both. Not being able to configure accurate 
pixel clock will make the displays not work, so from my side, this is a 
NAK, sorry.

There has to be some better solution which still allows the PLL 
reconfiguration to achieve accurate pixel clock.

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

[Ying Liu]

Hi Marek,

On 11/15/2024, Marek Vasut wrote:
> On 11/14/24 7:57 AM, Liu Ying wrote:
> > This reverts commit ff06ea04e4cf3ba2f025024776e83bfbdfa05155.
> >
> > media_disp1_pix clock is the pixel clock of the first i.MX8MP LCDIFv3
> > display controller, while media_disp2_pix clock is the pixel clock of
> > the second i.MX8MP LCDIFv3 display controller.  The two display
> > controllers connect with Samsung MIPI DSI controller and LVDS Display
> > Bridge(LDB) respectively.  Since the two display controllers are driven
> > by separate DRM driver instances and the two pixel clocks may be derived
> > from the same video_pll1_out clock(sys_pll3_out clock could be already
> > used to derive audio_axi clock), there is no way to negotiate a dynamically
> > changeable video_pll1_out clock rate to satisfy both of the two display
> > controllers.  In this case, the only solution to drive them with the
> > single video_pll1_out clock is to assign a sensible/unchangeable clock
> > rate for video_pll1_out clock.  Thus, there is no need to set the
> > CLK_SET_RATE_PARENT flag for media_disp{1,2}_pix clocks, drop it then.
> >
> > Fixes: ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock
> reconfigure parent rate")
> > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> Uh, I almost missed this revert between all the LDB patches.
> 
> This revert will break my usecase on MX8MP where I need to operate two
> disparate panels attached to LVDS and TC358767 DSI-to-DP bridge and I
> need accurate pixel clock for both. Not being able to configure accurate
> pixel clock will make the displays not work, so from my side, this is a
> NAK, sorry.

Is your usecase in upstream kernel? If yes, which DT file implements the
usecase?  I guess it's im8mp-dhcom-som.dtsi authored by you, but it only
contains the DT node for TC358767, but not LVDS panel.

Can you please elaborate about the failure case?

You still may assign an accurate PLL rate in DT.
This patch only makes the PLL rate be unchangeable dynamically in
runtime.  That means the existing imx8m-dhcom-som.dtsi would use
IMX8MP_VIDEO_PLL1_OUT(running at 1.0395GHz) as the parent clock
of IMX8MP_CLK_MEDIA_DISP1_PIX (for LCDIF1/DSI), since it includes
imx8mp.dsti.  I assume it should be able to support typical video modes
like 1080p60 video mode with 148.5MHz pixel clock at least with 1.0395GHz
PLL rate.  Granted that less video modes read from DP monitor would
be supported without dynamically changeable PLL rates, this is something
we have to accept because some i.MX8MP platforms(like i.MX8MP EVK)
have to share IMX8MP_VIDEO_PLL1_OUT between LVDS and MIPI DSI
display pipelines.  The missing part is that we need to do mode validation
for the MIPI DSI display pipeline either in samsung-dsim.c or lcdif_kms.c
to filter unsupported video mode out.  Is this missing mode validation
the cause of your failure case?

> 
> There has to be some better solution which still allows the PLL
> reconfiguration to achieve accurate pixel clock.

As I mentioned in cover letter, the only solution to support LVDS and
MIPI DSI displays on all i.MX8MP platforms is to assign a sensible and
unchangeable PLL rate in DT.  Some platforms may use two separate
PLLs for the LVDS and MIPI DSI display pipelines, while some others
have to use only the single IMX8MP_VIDEO_PLL1_OUT because
all other eligible PLLs are used up.  That's all fine, just being platforms
dependent.  The only limitation of the solution is that some platforms
couldn't support some particular LVDS and MIPI DSI displays at the
same time due to lack of PLLs, but this has to be accepted since
the shared IMX8MP_VIDEO_PLL1_OUT case needs to be supported and
the two display pipelines are not aware of each other from kernel's
point of view.

I hope that we can agree on this solution first before spreading
discussions across different threads and eventually the NAK can be
taken back.

Regards,
Liu Ying

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

[Marek Vasut]

On 11/18/24 4:54 AM, Ying Liu wrote:
> Hi Marek,

Hi,

>>> media_disp1_pix clock is the pixel clock of the first i.MX8MP LCDIFv3
>>> display controller, while media_disp2_pix clock is the pixel clock of
>>> the second i.MX8MP LCDIFv3 display controller.  The two display
>>> controllers connect with Samsung MIPI DSI controller and LVDS Display
>>> Bridge(LDB) respectively.  Since the two display controllers are driven
>>> by separate DRM driver instances and the two pixel clocks may be derived
>>> from the same video_pll1_out clock(sys_pll3_out clock could be already
>>> used to derive audio_axi clock), there is no way to negotiate a dynamically
>>> changeable video_pll1_out clock rate to satisfy both of the two display
>>> controllers.  In this case, the only solution to drive them with the
>>> single video_pll1_out clock is to assign a sensible/unchangeable clock
>>> rate for video_pll1_out clock.  Thus, there is no need to set the
>>> CLK_SET_RATE_PARENT flag for media_disp{1,2}_pix clocks, drop it then.
>>>
>>> Fixes: ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock
>> reconfigure parent rate")
>>> Signed-off-by: Liu Ying <victor.liu@nxp.com>
>> Uh, I almost missed this revert between all the LDB patches.
>>
>> This revert will break my usecase on MX8MP where I need to operate two
>> disparate panels attached to LVDS and TC358767 DSI-to-DP bridge and I
>> need accurate pixel clock for both. Not being able to configure accurate
>> pixel clock will make the displays not work, so from my side, this is a
>> NAK, sorry.
> 
> Is your usecase in upstream kernel? If yes, which DT file implements the
> usecase?  I guess it's im8mp-dhcom-som.dtsi authored by you, but it only
> contains the DT node for TC358767, but not LVDS panel.
> 
> Can you please elaborate about the failure case?

The TC9595 can drive an DP output, for that the clock which have to be 
set on the LCDIF cannot be predicted, as that information comes from the 
monitor EDID/DPCD. That is why the LCDIF has to be able to configure the 
Video PLL1 clock to accurate clock frequency.

For the LVDS LDB, the use case is the other way around -- the pixel 
clock which should be generated by LCDIF and fed to LDB are known from 
the panel type listed in DT, but they should still be accurate.

> You still may assign an accurate PLL rate in DT.
> This patch only makes the PLL rate be unchangeable dynamically in
> runtime.  That means the existing imx8m-dhcom-som.dtsi would use
> IMX8MP_VIDEO_PLL1_OUT(running at 1.0395GHz) as the parent clock
> of IMX8MP_CLK_MEDIA_DISP1_PIX (for LCDIF1/DSI), since it includes
> imx8mp.dsti.  I assume it should be able to support typical video modes
> like 1080p60 video mode with 148.5MHz pixel clock at least with 1.0395GHz
> PLL rate.

This will break multiple DP monitors I tested so far I'm afraid. And I 
spent a LOT of time wrestling with the TC9595 bridge to make sure it 
actually does work well.

> Granted that less video modes read from DP monitor would
> be supported without dynamically changeable PLL rates, this is something
> we have to accept because some i.MX8MP platforms(like i.MX8MP EVK)
> have to share IMX8MP_VIDEO_PLL1_OUT between LVDS and MIPI DSI
> display pipelines.

What I need is the use of two full PLL1443x (like Video PLL and Audio 
PLL1/2) , one for each display output, and those PLLs have to be fully 
configurable to produce accurate pixel clock for each connected panel. 
Otherwise I cannot make proper use of the video output capabilities of 
the MX8MP SoC.

> The missing part is that we need to do mode validation
> for the MIPI DSI display pipeline either in samsung-dsim.c or lcdif_kms.c
> to filter unsupported video mode out.  Is this missing mode validation
> the cause of your failure case?

I do want to support the various modes, I do not want to filter them 
out. They can be supported, the only "problem" is the shared Video PLL 
which is not really an actual problem in my case, because I do not use 
shared Video PLL, I use two separate PLLs.

I think what is needed is for the LCDIF1/LCDIF2/LDB to figure out 
whether they share the Video PLL at all (you already suggested the clock 
subsystem can provide that information), and then if:
- yes, agree on some sort of middle-ground frequency to configure into
   the Video PLL, frequency which somehow fits all three consumers
   (LCDIF1,LCDIF2,LDB)
- no, configure each consumer upstream clock to generate accurate pixel
   clock for that consumer

Something like ^ would make MX8MP EVK (the "yes" case) with shared Video 
PLL work, without breaking my use case (the "no" case), right ? (*)

>> There has to be some better solution which still allows the PLL
>> reconfiguration to achieve accurate pixel clock.
> 
> As I mentioned in cover letter, the only solution to support LVDS and
> MIPI DSI displays on all i.MX8MP platforms is to assign a sensible and
> unchangeable PLL rate in DT.

I am currently using Video PLL and Audio PLL to drive DSI and LVDS 
outputs from each, so no, fixed Video PLL assignment in DT is not the 
only solution.

> Some platforms may use two separate
> PLLs for the LVDS and MIPI DSI display pipelines, while some others
> have to use only the single IMX8MP_VIDEO_PLL1_OUT because
> all other eligible PLLs are used up.  That's all fine, just being platforms
> dependent.  The only limitation of the solution is that some platforms
> couldn't support some particular LVDS and MIPI DSI displays at the
> same time due to lack of PLLs, but this has to be accepted since
> the shared IMX8MP_VIDEO_PLL1_OUT case needs to be supported and
> the two display pipelines are not aware of each other from kernel's
> point of view.

Can something like (*) above be implemented instead, so both Shared and 
separate PLLs would be supported ? That should solve both of our use 
cases, right ?

> I hope that we can agree on this solution first before spreading
> discussions across different threads and eventually the NAK can be
> taken back.

I cannot really agree on a solution which breaks one of my use cases, 
but maybe there is an alternative how to support both options, see (*) 
above ?

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

[Ying Liu]

On 11/19/24, Marek Vasut wrote:
> On 11/18/24 4:54 AM, Ying Liu wrote:
> > Hi Marek,
> 
> Hi,
> 
> >>> media_disp1_pix clock is the pixel clock of the first i.MX8MP LCDIFv3
> >>> display controller, while media_disp2_pix clock is the pixel clock of
> >>> the second i.MX8MP LCDIFv3 display controller.  The two display
> >>> controllers connect with Samsung MIPI DSI controller and LVDS Display
> >>> Bridge(LDB) respectively.  Since the two display controllers are driven
> >>> by separate DRM driver instances and the two pixel clocks may be
> derived
> >>> from the same video_pll1_out clock(sys_pll3_out clock could be already
> >>> used to derive audio_axi clock), there is no way to negotiate a
> dynamically
> >>> changeable video_pll1_out clock rate to satisfy both of the two display
> >>> controllers.  In this case, the only solution to drive them with the
> >>> single video_pll1_out clock is to assign a sensible/unchangeable clock
> >>> rate for video_pll1_out clock.  Thus, there is no need to set the
> >>> CLK_SET_RATE_PARENT flag for media_disp{1,2}_pix clocks, drop it then.
> >>>
> >>> Fixes: ff06ea04e4cf ("clk: imx: clk-imx8mp: Allow media_disp pixel clock
> >> reconfigure parent rate")
> >>> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> >> Uh, I almost missed this revert between all the LDB patches.
> >>
> >> This revert will break my usecase on MX8MP where I need to operate two
> >> disparate panels attached to LVDS and TC358767 DSI-to-DP bridge and I
> >> need accurate pixel clock for both. Not being able to configure accurate
> >> pixel clock will make the displays not work, so from my side, this is a
> >> NAK, sorry.
> >
> > Is your usecase in upstream kernel? If yes, which DT file implements the
> > usecase?  I guess it's im8mp-dhcom-som.dtsi authored by you, but it only
> > contains the DT node for TC358767, but not LVDS panel.
> >
> > Can you please elaborate about the failure case?
> 
> The TC9595 can drive an DP output, for that the clock which have to be
> set on the LCDIF cannot be predicted, as that information comes from the
> monitor EDID/DPCD. That is why the LCDIF has to be able to configure the
> Video PLL1 clock to accurate clock frequency.
> 
> For the LVDS LDB, the use case is the other way around -- the pixel
> clock which should be generated by LCDIF and fed to LDB are known from
> the panel type listed in DT, but they should still be accurate.

Thanks for the information.  I think the key question is whether the
alternative solution(*) you mentioned below stands or not, in other words,
whether LCDIF1/LCDIF2/LDB drivers know that they are sharing a PLL
or not.

> 
> > You still may assign an accurate PLL rate in DT.
> > This patch only makes the PLL rate be unchangeable dynamically in
> > runtime.  That means the existing imx8m-dhcom-som.dtsi would use
> > IMX8MP_VIDEO_PLL1_OUT(running at 1.0395GHz) as the parent clock
> > of IMX8MP_CLK_MEDIA_DISP1_PIX (for LCDIF1/DSI), since it includes
> > imx8mp.dsti.  I assume it should be able to support typical video modes
> > like 1080p60 video mode with 148.5MHz pixel clock at least with 1.0395GHz
> > PLL rate.
> 
> This will break multiple DP monitors I tested so far I'm afraid. And I
> spent a LOT of time wrestling with the TC9595 bridge to make sure it
> actually does work well.

If the DP monitors support typical video modes like 1080p60 with
148.5MHz pixel clock rate, I assume these typical video modes work
still ok with this patch at least.  Please help confirm this, since if the
alternative solution(*) doesn't stand, we would know those video
modes still work ok with my solution(fixed PLL rate).

> 
> > Granted that less video modes read from DP monitor would
> > be supported without dynamically changeable PLL rates, this is something
> > we have to accept because some i.MX8MP platforms(like i.MX8MP EVK)
> > have to share IMX8MP_VIDEO_PLL1_OUT between LVDS and MIPI DSI
> > display pipelines.
> 
> What I need is the use of two full PLL1443x (like Video PLL and Audio
> PLL1/2) , one for each display output, and those PLLs have to be fully
> configurable to produce accurate pixel clock for each connected panel.
> Otherwise I cannot make proper use of the video output capabilities of
> the MX8MP SoC.

Yeah, I understand your requirements.  However, it still depends on
whether the alternative solution(*) stands or not.

> 
> > The missing part is that we need to do mode validation
> > for the MIPI DSI display pipeline either in samsung-dsim.c or lcdif_kms.c
> > to filter unsupported video mode out.  Is this missing mode validation
> > the cause of your failure case?
> 
> I do want to support the various modes, I do not want to filter them
> out. They can be supported, the only "problem" is the shared Video PLL
> which is not really an actual problem in my case, because I do not use
> shared Video PLL, I use two separate PLLs.
> 
> I think what is needed is for the LCDIF1/LCDIF2/LDB to figure out
> whether they share the Video PLL at all (you already suggested the clock
> subsystem can provide that information), and then if:

But, how to let LCDIF1/LCDIF2/LDB drivers to figure out that?

I didn't suggest that the clock subsystem can provide that information.

> - yes, agree on some sort of middle-ground frequency to configure into
>    the Video PLL, frequency which somehow fits all three consumers
>    (LCDIF1,LCDIF2,LDB)
> - no, configure each consumer upstream clock to generate accurate pixel
>    clock for that consumer
> 
> Something like ^ would make MX8MP EVK (the "yes" case) with shared Video
> PLL work, without breaking my use case (the "no" case), right ? (*)

In an ideal world, right.  But, again how to let LCDIF1/LCDIF2/LDB drivers
to figure out that they are sharing a PLL?  

> 
> >> There has to be some better solution which still allows the PLL
> >> reconfiguration to achieve accurate pixel clock.
> >
> > As I mentioned in cover letter, the only solution to support LVDS and
> > MIPI DSI displays on all i.MX8MP platforms is to assign a sensible and
> > unchangeable PLL rate in DT.
> 
> I am currently using Video PLL and Audio PLL to drive DSI and LVDS
> outputs from each, so no, fixed Video PLL assignment in DT is not the
> only solution.
> 
> > Some platforms may use two separate
> > PLLs for the LVDS and MIPI DSI display pipelines, while some others
> > have to use only the single IMX8MP_VIDEO_PLL1_OUT because
> > all other eligible PLLs are used up.  That's all fine, just being platforms
> > dependent.  The only limitation of the solution is that some platforms
> > couldn't support some particular LVDS and MIPI DSI displays at the
> > same time due to lack of PLLs, but this has to be accepted since
> > the shared IMX8MP_VIDEO_PLL1_OUT case needs to be supported and
> > the two display pipelines are not aware of each other from kernel's
> > point of view.
> 
> Can something like (*) above be implemented instead, so both Shared and
> separate PLLs would be supported ? That should solve both of our use
> cases, right ?

I don't see any clear way to implement something like(*).

Take the 3 i.MX8MP LCDIFs as one graphic card driven by one imx-lcdif
DRM instance?  Would it be too intrusive?

Use clk_get_parent() to determine if the pixel clocks of LCDIF1&2 are
sharing PLL(note clk_get_parent() implementation contains a TODO:
Create a per-user clk.)? 

How to do mode validation for the shared PLL case(note mode_valid()
callback is supposed to look at nothing more than passed-in mode)?
Use clk_set_rate_range() to fix the PLL rate(min == max)?

> 
> > I hope that we can agree on this solution first before spreading
> > discussions across different threads and eventually the NAK can be
> > taken back.
> 
> I cannot really agree on a solution which breaks one of my use cases,
> but maybe there is an alternative how to support both options, see (*)
> above ?

I tend to say there is no any alternative solution to satisfy both
separate PLLs and shared PLL use cases, or even if there is one, it won't
be easy to implement.  If you know one, please shout it out.

Regards,
Liu Ying

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

[Marek Vasut]

On 11/19/24 9:18 AM, Ying Liu wrote:

[...]

>> The TC9595 can drive an DP output, for that the clock which have to be
>> set on the LCDIF cannot be predicted, as that information comes from the
>> monitor EDID/DPCD. That is why the LCDIF has to be able to configure the
>> Video PLL1 clock to accurate clock frequency.
>>
>> For the LVDS LDB, the use case is the other way around -- the pixel
>> clock which should be generated by LCDIF and fed to LDB are known from
>> the panel type listed in DT, but they should still be accurate.
> 
> Thanks for the information.  I think the key question is whether the
> alternative solution(*) you mentioned below stands or not, in other words,
> whether LCDIF1/LCDIF2/LDB drivers know that they are sharing a PLL
> or not.

I'll continue at the end ...

>>> You still may assign an accurate PLL rate in DT.
>>> This patch only makes the PLL rate be unchangeable dynamically in
>>> runtime.  That means the existing imx8m-dhcom-som.dtsi would use
>>> IMX8MP_VIDEO_PLL1_OUT(running at 1.0395GHz) as the parent clock
>>> of IMX8MP_CLK_MEDIA_DISP1_PIX (for LCDIF1/DSI), since it includes
>>> imx8mp.dsti.  I assume it should be able to support typical video modes
>>> like 1080p60 video mode with 148.5MHz pixel clock at least with 1.0395GHz
>>> PLL rate.
>>
>> This will break multiple DP monitors I tested so far I'm afraid. And I
>> spent a LOT of time wrestling with the TC9595 bridge to make sure it
>> actually does work well.
> 
> If the DP monitors support typical video modes like 1080p60 with
> 148.5MHz pixel clock rate, I assume these typical video modes work
> still ok with this patch at least.  Please help confirm this, since if the
> alternative solution(*) doesn't stand, we would know those video
> modes still work ok with my solution(fixed PLL rate).

They do not work with the fixed PLL setting.

>>> Granted that less video modes read from DP monitor would
>>> be supported without dynamically changeable PLL rates, this is something
>>> we have to accept because some i.MX8MP platforms(like i.MX8MP EVK)
>>> have to share IMX8MP_VIDEO_PLL1_OUT between LVDS and MIPI DSI
>>> display pipelines.
>>
>> What I need is the use of two full PLL1443x (like Video PLL and Audio
>> PLL1/2) , one for each display output, and those PLLs have to be fully
>> configurable to produce accurate pixel clock for each connected panel.
>> Otherwise I cannot make proper use of the video output capabilities of
>> the MX8MP SoC.
> 
> Yeah, I understand your requirements.  However, it still depends on
> whether the alternative solution(*) stands or not.

I'll continue at the end ...

>>> The missing part is that we need to do mode validation
>>> for the MIPI DSI display pipeline either in samsung-dsim.c or lcdif_kms.c
>>> to filter unsupported video mode out.  Is this missing mode validation
>>> the cause of your failure case?
>>
>> I do want to support the various modes, I do not want to filter them
>> out. They can be supported, the only "problem" is the shared Video PLL
>> which is not really an actual problem in my case, because I do not use
>> shared Video PLL, I use two separate PLLs.
>>
>> I think what is needed is for the LCDIF1/LCDIF2/LDB to figure out
>> whether they share the Video PLL at all (you already suggested the clock
>> subsystem can provide that information), and then if:
> 
> But, how to let LCDIF1/LCDIF2/LDB drivers to figure out that?
> 
> I didn't suggest that the clock subsystem can provide that information.

... by end I mean here.

One really nasty way I can think of is -- use find_node_by_compatible(), 
look up all the relevant DT nodes, parse their clock properties, and 
check whether they all point to the Video PLL or not.

Maybe the clock subsystem has a better way, like list "neighbor" 
consumers of some specific parent clock or something like that.

[...]

>> Can something like (*) above be implemented instead, so both Shared and
>> separate PLLs would be supported ? That should solve both of our use
>> cases, right ?
> 
> I don't see any clear way to implement something like(*).
> 
> Take the 3 i.MX8MP LCDIFs as one graphic card driven by one imx-lcdif
> DRM instance?  Would it be too intrusive?

Yes, and I think unnecessary, one can simply traverse and parse the DT 
to determine the clock assignment?

> Use clk_get_parent() to determine if the pixel clocks of LCDIF1&2 are
> sharing PLL(note clk_get_parent() implementation contains a TODO:
> Create a per-user clk.)?

Maybe not necessary for this case.

> How to do mode validation for the shared PLL case(note mode_valid()
> callback is supposed to look at nothing more than passed-in mode)?
> Use clk_set_rate_range() to fix the PLL rate(min == max)?

This is a good question -- we can use fixed frequency set in DT for the 
PLL in case it is shared, and set whatever optimal frequency if the PLL 
is not shared. That would be a good first step I think (**).

The next step would be to find a way to negotiate acceptable PLL 
frequency between LCDIF1/LCDIF2/LDB in case the PLL is shared, but I do 
agree this is non-trivial, hence next step.

>>> I hope that we can agree on this solution first before spreading
>>> discussions across different threads and eventually the NAK can be
>>> taken back.
>>
>> I cannot really agree on a solution which breaks one of my use cases,
>> but maybe there is an alternative how to support both options, see (*)
>> above ?
> 
> I tend to say there is no any alternative solution to satisfy both
> separate PLLs and shared PLL use cases, or even if there is one, it won't
> be easy to implement.  If you know one, please shout it out.
Maybe (*) with first step (**) would be doable ?

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

[Ying Liu]

On 11/20/24, Marek Vasut wrote:
> On 11/19/24 9:18 AM, Ying Liu wrote:
> 
> [...]
> 
> >> The TC9595 can drive an DP output, for that the clock which have to be
> >> set on the LCDIF cannot be predicted, as that information comes from the
> >> monitor EDID/DPCD. That is why the LCDIF has to be able to configure the
> >> Video PLL1 clock to accurate clock frequency.
> >>
> >> For the LVDS LDB, the use case is the other way around -- the pixel
> >> clock which should be generated by LCDIF and fed to LDB are known from
> >> the panel type listed in DT, but they should still be accurate.
> >
> > Thanks for the information.  I think the key question is whether the
> > alternative solution(*) you mentioned below stands or not, in other words,
> > whether LCDIF1/LCDIF2/LDB drivers know that they are sharing a PLL
> > or not.
> 
> I'll continue at the end ...
> 
> >>> You still may assign an accurate PLL rate in DT.
> >>> This patch only makes the PLL rate be unchangeable dynamically in
> >>> runtime.  That means the existing imx8m-dhcom-som.dtsi would use
> >>> IMX8MP_VIDEO_PLL1_OUT(running at 1.0395GHz) as the parent clock
> >>> of IMX8MP_CLK_MEDIA_DISP1_PIX (for LCDIF1/DSI), since it includes
> >>> imx8mp.dsti.  I assume it should be able to support typical video modes
> >>> like 1080p60 video mode with 148.5MHz pixel clock at least with
> 1.0395GHz
> >>> PLL rate.
> >>
> >> This will break multiple DP monitors I tested so far I'm afraid. And I
> >> spent a LOT of time wrestling with the TC9595 bridge to make sure it
> >> actually does work well.
> >
> > If the DP monitors support typical video modes like 1080p60 with
> > 148.5MHz pixel clock rate, I assume these typical video modes work
> > still ok with this patch at least.  Please help confirm this, since if the
> > alternative solution(*) doesn't stand, we would know those video
> > modes still work ok with my solution(fixed PLL rate).
> 
> They do not work with the fixed PLL setting.

Why?  Did you assign a sensible fixed PLL rate in DT?
Can you please compare clk_summary output for the failing cases
before and after this patch is applied? I assume that if you use
the fixed PLL rate same to the rate which works before this patch is
applied, the typical video modes still just work after this patch is
applied.

> 
> >>> Granted that less video modes read from DP monitor would
> >>> be supported without dynamically changeable PLL rates, this is
> something
> >>> we have to accept because some i.MX8MP platforms(like i.MX8MP EVK)
> >>> have to share IMX8MP_VIDEO_PLL1_OUT between LVDS and MIPI DSI
> >>> display pipelines.
> >>
> >> What I need is the use of two full PLL1443x (like Video PLL and Audio
> >> PLL1/2) , one for each display output, and those PLLs have to be fully
> >> configurable to produce accurate pixel clock for each connected panel.
> >> Otherwise I cannot make proper use of the video output capabilities of
> >> the MX8MP SoC.
> >
> > Yeah, I understand your requirements.  However, it still depends on
> > whether the alternative solution(*) stands or not.
> 
> I'll continue at the end ...
> 
> >>> The missing part is that we need to do mode validation
> >>> for the MIPI DSI display pipeline either in samsung-dsim.c or lcdif_kms.c
> >>> to filter unsupported video mode out.  Is this missing mode validation
> >>> the cause of your failure case?
> >>
> >> I do want to support the various modes, I do not want to filter them
> >> out. They can be supported, the only "problem" is the shared Video PLL
> >> which is not really an actual problem in my case, because I do not use
> >> shared Video PLL, I use two separate PLLs.
> >>
> >> I think what is needed is for the LCDIF1/LCDIF2/LDB to figure out
> >> whether they share the Video PLL at all (you already suggested the clock
> >> subsystem can provide that information), and then if:
> >
> > But, how to let LCDIF1/LCDIF2/LDB drivers to figure out that?
> >
> > I didn't suggest that the clock subsystem can provide that information.
> 
> ... by end I mean here.
> 
> One really nasty way I can think of is -- use find_node_by_compatible(),
> look up all the relevant DT nodes, parse their clock properties, and
> check whether they all point to the Video PLL or not.

That's nasty.  It looks even more nasty when considering the fact that
i.MX93 LCDIF is also driven by imx-lcdif DRM while only i.MX8MP LCDIF
needs the nasty check, because i.MX93 SoC embeds only one LCDIF.

> 
> Maybe the clock subsystem has a better way, like list "neighbor"
> consumers of some specific parent clock or something like that.

What will imx-lcdif DRM look like by using this way? Get the ancestor PLL
clock of pixel clock(media_disp{1,2}_pix_root_clk), list all child clocks
(media_disp1_pix and/or media_disp2_pix + other possible clocks) of the
PLL clock in a string array and find media_disp1_pix + media_disp2_pix
in it?

Doesn't look nice, either.

> 
> [...]
> 
> >> Can something like (*) above be implemented instead, so both Shared
> and
> >> separate PLLs would be supported ? That should solve both of our use
> >> cases, right ?
> >
> > I don't see any clear way to implement something like(*).
> >
> > Take the 3 i.MX8MP LCDIFs as one graphic card driven by one imx-lcdif
> > DRM instance?  Would it be too intrusive?
> 
> Yes, and I think unnecessary, one can simply traverse and parse the DT
> to determine the clock assignment?

Yes, people can traverse and parse DT, but it's nasty.

In addition, one may argue that now that CLK_SET_RATE_PARENT flag
is set for the pixel clocks, all potential video modes read from EDID
should be supported when only either LVDS display pipeline or MIPI DSI
display pipeline is active in the shared PLL case.  This requires one
single DRM instance to detect single or dual active display pipelines
dynamically, hence this single DRM instance becomes necessary.

> 
> > Use clk_get_parent() to determine if the pixel clocks of LCDIF1&2 are
> > sharing PLL(note clk_get_parent() implementation contains a TODO:
> > Create a per-user clk.)?
> 
> Maybe not necessary for this case.
> 
> > How to do mode validation for the shared PLL case(note mode_valid()
> > callback is supposed to look at nothing more than passed-in mode)?
> > Use clk_set_rate_range() to fix the PLL rate(min == max)?
> 
> This is a good question -- we can use fixed frequency set in DT for the
> PLL in case it is shared, and set whatever optimal frequency if the PLL
> is not shared. That would be a good first step I think (**).

The above requirement of dynamical active display pipeline number
detection defeats the fixed PLL rate for in the shared PLL case. 
And, it makes mode validation kind of undoable, because mode_valid()
is not supposed to know that active number.

> 
> The next step would be to find a way to negotiate acceptable PLL
> frequency between LCDIF1/LCDIF2/LDB in case the PLL is shared, but I do
> agree this is non-trivial, hence next step.
> 
> >>> I hope that we can agree on this solution first before spreading
> >>> discussions across different threads and eventually the NAK can be
> >>> taken back.
> >>
> >> I cannot really agree on a solution which breaks one of my use cases,
> >> but maybe there is an alternative how to support both options, see (*)
> >> above ?
> >
> > I tend to say there is no any alternative solution to satisfy both
> > separate PLLs and shared PLL use cases, or even if there is one, it won't
> > be easy to implement.  If you know one, please shout it out.
> Maybe (*) with first step (**) would be doable ?

Maybe it's not doable if the above requirement of dynamical active display
pipeline number detection needs to be supported.

Considering 1) the way of getting separate/shared PLL information and
2) mode validation, I don't think your overall alternative solution is good.

Regards,
Liu Ying

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

[Marek Vasut]

On 11/20/24 7:38 AM, Ying Liu wrote:

[...]

>>> If the DP monitors support typical video modes like 1080p60 with
>>> 148.5MHz pixel clock rate, I assume these typical video modes work
>>> still ok with this patch at least.  Please help confirm this, since if the
>>> alternative solution(*) doesn't stand, we would know those video
>>> modes still work ok with my solution(fixed PLL rate).
>>
>> They do not work with the fixed PLL setting.
> 
> Why?  Did you assign a sensible fixed PLL rate in DT?

Whatever was in imx8mp.dtsi does not really work for all the panels.
Please keep in mind that the use case I have does not include only 
1920x1080 "standard" panels, but also other resolutions.

> Can you please compare clk_summary output for the failing cases
> before and after this patch is applied? I assume that if you use
> the fixed PLL rate same to the rate which works before this patch is
> applied, the typical video modes still just work after this patch is
> applied.

I'm afraid I do not need to support only typical video modes, but also 
the other "atypical" modes.

[...]

>> One really nasty way I can think of is -- use find_node_by_compatible(),
>> look up all the relevant DT nodes, parse their clock properties, and
>> check whether they all point to the Video PLL or not.
> 
> That's nasty.  It looks even more nasty when considering the fact that
> i.MX93 LCDIF is also driven by imx-lcdif DRM while only i.MX8MP LCDIF
> needs the nasty check, because i.MX93 SoC embeds only one LCDIF.

The check can be skipped based on compatible string.

I agree it is nasty, but it is a start. Are there better ideas ?

>> Maybe the clock subsystem has a better way, like list "neighbor"
>> consumers of some specific parent clock or something like that.
> 
> What will imx-lcdif DRM look like by using this way? Get the ancestor PLL
> clock of pixel clock(media_disp{1,2}_pix_root_clk), list all child clocks
> (media_disp1_pix and/or media_disp2_pix + other possible clocks) of the
> PLL clock in a string array and find media_disp1_pix + media_disp2_pix
> in it?
> 
> Doesn't look nice, either.

One other option came to my mind -- place a virtual clock between the 
Video PLL and consumers (LCDIF1/2/LDB), and then have the virtual clock 
driver do the clock rate negotiation in some .round_rate callback. That 
is also nasty, but it is another idea. If there is a clock specifically 
implemented to negotiate best upstream clock rate for all of its 
consumers, and it is aware of the consumer behavior details and 
requirements, maybe that could work ?

>> [...]
>>
>>>> Can something like (*) above be implemented instead, so both Shared
>> and
>>>> separate PLLs would be supported ? That should solve both of our use
>>>> cases, right ?
>>>
>>> I don't see any clear way to implement something like(*).
>>>
>>> Take the 3 i.MX8MP LCDIFs as one graphic card driven by one imx-lcdif
>>> DRM instance?  Would it be too intrusive?
>>
>> Yes, and I think unnecessary, one can simply traverse and parse the DT
>> to determine the clock assignment?
> 
> Yes, people can traverse and parse DT, but it's nasty.
> 
> In addition, one may argue that now that CLK_SET_RATE_PARENT flag
> is set for the pixel clocks, all potential video modes read from EDID
> should be supported when only either LVDS display pipeline or MIPI DSI
> display pipeline is active in the shared PLL case.  This requires one
> single DRM instance to detect single or dual active display pipelines
> dynamically, hence this single DRM instance becomes necessary.

Would single virtual clock which do the frequency negotiation between 
multiple DRM consumers work too ?

I do not have much to add to the points below.

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

[Ying Liu]

On 11/22/24, Marek Vasut wrote: 
> On 11/20/24 7:38 AM, Ying Liu wrote:
> 
> [...]
> 
> >>> If the DP monitors support typical video modes like 1080p60 with
> >>> 148.5MHz pixel clock rate, I assume these typical video modes work
> >>> still ok with this patch at least.  Please help confirm this, since if the
> >>> alternative solution(*) doesn't stand, we would know those video
> >>> modes still work ok with my solution(fixed PLL rate).
> >>
> >> They do not work with the fixed PLL setting.
> >
> > Why?  Did you assign a sensible fixed PLL rate in DT?
> 
> Whatever was in imx8mp.dtsi does not really work for all the panels.
> Please keep in mind that the use case I have does not include only
> 1920x1080 "standard" panels, but also other resolutions.

It looks like you are still sticking to the idea of supporting all potentially
valid video modes by trying to find an "alternative" solution, while
neglecting that the solution *could be* never working. 

> 
> > Can you please compare clk_summary output for the failing cases
> > before and after this patch is applied? I assume that if you use
> > the fixed PLL rate same to the rate which works before this patch is
> > applied, the typical video modes still just work after this patch is
> > applied.
> 
> I'm afraid I do not need to support only typical video modes, but also
> the other "atypical" modes.

If the "alternative" solution doesn't work, we'll end up using the "fixed
PLL rate" solution.  It that case, some video modes would be filtered
out as a sacrifice. 

> 
> [...]
> 
> >> One really nasty way I can think of is -- use find_node_by_compatible(),
> >> look up all the relevant DT nodes, parse their clock properties, and
> >> check whether they all point to the Video PLL or not.
> >
> > That's nasty.  It looks even more nasty when considering the fact that
> > i.MX93 LCDIF is also driven by imx-lcdif DRM while only i.MX8MP LCDIF
> > needs the nasty check, because i.MX93 SoC embeds only one LCDIF.
> 
> The check can be skipped based on compatible string.
> 
> I agree it is nasty, but it is a start. Are there better ideas ?

No good idea from me.

> 
> >> Maybe the clock subsystem has a better way, like list "neighbor"
> >> consumers of some specific parent clock or something like that.
> >
> > What will imx-lcdif DRM look like by using this way? Get the ancestor PLL
> > clock of pixel clock(media_disp{1,2}_pix_root_clk), list all child clocks
> > (media_disp1_pix and/or media_disp2_pix + other possible clocks) of the
> > PLL clock in a string array and find media_disp1_pix + media_disp2_pix
> > in it?
> >
> > Doesn't look nice, either.
> 
> One other option came to my mind -- place a virtual clock between the
> Video PLL and consumers (LCDIF1/2/LDB), and then have the virtual clock
> driver do the clock rate negotiation in some .round_rate callback. That
> is also nasty, but it is another idea. If there is a clock specifically
> implemented to negotiate best upstream clock rate for all of its
> consumers, and it is aware of the consumer behavior details and
> requirements, maybe that could work ?

A mighty virtual clock?  I'm not sure if that would work or not.

> 
> >> [...]
> >>
> >>>> Can something like (*) above be implemented instead, so both Shared
> >> and
> >>>> separate PLLs would be supported ? That should solve both of our use
> >>>> cases, right ?
> >>>
> >>> I don't see any clear way to implement something like(*).
> >>>
> >>> Take the 3 i.MX8MP LCDIFs as one graphic card driven by one imx-lcdif
> >>> DRM instance?  Would it be too intrusive?
> >>
> >> Yes, and I think unnecessary, one can simply traverse and parse the DT
> >> to determine the clock assignment?
> >
> > Yes, people can traverse and parse DT, but it's nasty.
> >
> > In addition, one may argue that now that CLK_SET_RATE_PARENT flag
> > is set for the pixel clocks, all potential video modes read from EDID
> > should be supported when only either LVDS display pipeline or MIPI DSI
> > display pipeline is active in the shared PLL case.  This requires one
> > single DRM instance to detect single or dual active display pipelines
> > dynamically, hence this single DRM instance becomes necessary.
> 
> Would single virtual clock which do the frequency negotiation between
> multiple DRM consumers work too ?

Not sure if it would work or not, but I'm sure that one single DRM instance
means atomic check/commit for the display pipelines as a whole, hence
awareness of active display pipeline number in an atomic way.

> 
> I do not have much to add to the points below.

Regards,
Liu Ying

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

[Adam Ford]

On Thu, Nov 21, 2024 at 9:39 PM Ying Liu <victor.liu@nxp.com> wrote:
>
> On 11/22/24, Marek Vasut wrote:
> > On 11/20/24 7:38 AM, Ying Liu wrote:
> >
> > [...]
> >
> > >>> If the DP monitors support typical video modes like 1080p60 with
> > >>> 148.5MHz pixel clock rate, I assume these typical video modes work
> > >>> still ok with this patch at least.  Please help confirm this, since if the
> > >>> alternative solution(*) doesn't stand, we would know those video
> > >>> modes still work ok with my solution(fixed PLL rate).
> > >>
> > >> They do not work with the fixed PLL setting.
> > >
> > > Why?  Did you assign a sensible fixed PLL rate in DT?
> >
> > Whatever was in imx8mp.dtsi does not really work for all the panels.
> > Please keep in mind that the use case I have does not include only
> > 1920x1080 "standard" panels, but also other resolutions.
>
> It looks like you are still sticking to the idea of supporting all potentially
> valid video modes by trying to find an "alternative" solution, while
> neglecting that the solution *could be* never working.
>
> >
> > > Can you please compare clk_summary output for the failing cases
> > > before and after this patch is applied? I assume that if you use
> > > the fixed PLL rate same to the rate which works before this patch is
> > > applied, the typical video modes still just work after this patch is
> > > applied.
> >
> > I'm afraid I do not need to support only typical video modes, but also
> > the other "atypical" modes.
>
> If the "alternative" solution doesn't work, we'll end up using the "fixed
> PLL rate" solution.  It that case, some video modes would be filtered
> out as a sacrifice.
>
> >
> > [...]
> >
> > >> One really nasty way I can think of is -- use find_node_by_compatible(),
> > >> look up all the relevant DT nodes, parse their clock properties, and
> > >> check whether they all point to the Video PLL or not.
> > >
> > > That's nasty.  It looks even more nasty when considering the fact that
> > > i.MX93 LCDIF is also driven by imx-lcdif DRM while only i.MX8MP LCDIF
> > > needs the nasty check, because i.MX93 SoC embeds only one LCDIF.
> >
> > The check can be skipped based on compatible string.
> >
> > I agree it is nasty, but it is a start. Are there better ideas ?
>
> No good idea from me.
>
> >
> > >> Maybe the clock subsystem has a better way, like list "neighbor"
> > >> consumers of some specific parent clock or something like that.
> > >
> > > What will imx-lcdif DRM look like by using this way? Get the ancestor PLL
> > > clock of pixel clock(media_disp{1,2}_pix_root_clk), list all child clocks
> > > (media_disp1_pix and/or media_disp2_pix + other possible clocks) of the
> > > PLL clock in a string array and find media_disp1_pix + media_disp2_pix
> > > in it?
> > >
> > > Doesn't look nice, either.
> >
> > One other option came to my mind -- place a virtual clock between the
> > Video PLL and consumers (LCDIF1/2/LDB), and then have the virtual clock
> > driver do the clock rate negotiation in some .round_rate callback. That
> > is also nasty, but it is another idea. If there is a clock specifically
> > implemented to negotiate best upstream clock rate for all of its
> > consumers, and it is aware of the consumer behavior details and
> > requirements, maybe that could work ?
>
> A mighty virtual clock?  I'm not sure if that would work or not.

From a power-consumption perspective, it seems to me like running the
clocks at the lowest value instead of setting a really high rate which
divides down would save power.

adam
>
> >
> > >> [...]
> > >>
> > >>>> Can something like (*) above be implemented instead, so both Shared
> > >> and
> > >>>> separate PLLs would be supported ? That should solve both of our use
> > >>>> cases, right ?
> > >>>
> > >>> I don't see any clear way to implement something like(*).
> > >>>
> > >>> Take the 3 i.MX8MP LCDIFs as one graphic card driven by one imx-lcdif
> > >>> DRM instance?  Would it be too intrusive?
> > >>
> > >> Yes, and I think unnecessary, one can simply traverse and parse the DT
> > >> to determine the clock assignment?
> > >
> > > Yes, people can traverse and parse DT, but it's nasty.
> > >
> > > In addition, one may argue that now that CLK_SET_RATE_PARENT flag
> > > is set for the pixel clocks, all potential video modes read from EDID
> > > should be supported when only either LVDS display pipeline or MIPI DSI
> > > display pipeline is active in the shared PLL case.  This requires one
> > > single DRM instance to detect single or dual active display pipelines
> > > dynamically, hence this single DRM instance becomes necessary.
> >
> > Would single virtual clock which do the frequency negotiation between
> > multiple DRM consumers work too ?
>
> Not sure if it would work or not, but I'm sure that one single DRM instance
> means atomic check/commit for the display pipelines as a whole, hence
> awareness of active display pipeline number in an atomic way.
>
> >
> > I do not have much to add to the points below.
>
> Regards,
> Liu Ying

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

[Marek Vasut]

On 11/22/24 4:39 AM, Ying Liu wrote:
> On 11/22/24, Marek Vasut wrote:
>> On 11/20/24 7:38 AM, Ying Liu wrote:
>>
>> [...]
>>
>>>>> If the DP monitors support typical video modes like 1080p60 with
>>>>> 148.5MHz pixel clock rate, I assume these typical video modes work
>>>>> still ok with this patch at least.  Please help confirm this, since if the
>>>>> alternative solution(*) doesn't stand, we would know those video
>>>>> modes still work ok with my solution(fixed PLL rate).
>>>>
>>>> They do not work with the fixed PLL setting.
>>>
>>> Why?  Did you assign a sensible fixed PLL rate in DT?
>>
>> Whatever was in imx8mp.dtsi does not really work for all the panels.
>> Please keep in mind that the use case I have does not include only
>> 1920x1080 "standard" panels, but also other resolutions.
> 
> It looks like you are still sticking to the idea of supporting all potentially
> valid video modes by trying to find an "alternative" solution, while
> neglecting that the solution *could be* never working.

Reconfiguring the PLL to achieve accurate pixel clock is working 
already, right now.

>>> Can you please compare clk_summary output for the failing cases
>>> before and after this patch is applied? I assume that if you use
>>> the fixed PLL rate same to the rate which works before this patch is
>>> applied, the typical video modes still just work after this patch is
>>> applied.
>>
>> I'm afraid I do not need to support only typical video modes, but also
>> the other "atypical" modes.
> 
> If the "alternative" solution doesn't work, we'll end up using the "fixed
> PLL rate" solution.  It that case, some video modes would be filtered
> out as a sacrifice.

Maybe it would be better to continue the discussion in

[PATCH 0/5] clk: Fix simple video pipelines on i.MX8

which seems close to the alternative solution I was looking for ?

Re: [PATCH v7 0/7] Add ITE IT6263 LVDS to HDMI converter support

[Maxime Ripard]

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

Hi,

Thanks for the description, I have several questions here.

On Thu, Nov 14, 2024 at 02:57:52PM +0800, Liu Ying wrote:
> This patch series aims to add ITE IT6263 LVDS to HDMI converter on
> i.MX8MP EVK.
> 
> Since IT6263 DT binding and driver were picked up from v5 and landed
> in drm-misc, this patch series contains patches almost all i.MX8MP
> SoC/platform specific.
> 
> Patch 1 is a preparation patch to allow display mode of an existing
> panel to pass the added mode validation logic in patch 3.
> 
> Patch 2 is a preparation patch to drop CLK_SET_RATE_PARENT flag for
> media_disp{1,2}_pix clocks.  Patch 5 depends on patch 2.
> 
> Patch 3 allows i.MX8MP LVDS Display Bridge(LDB) bridge driver to find
> the next non-panel bridge, that is the IT6263 in this case.
> 
> Patch 4 adds mode validation logic to i.MX8MP LDB bridge driver against
> "ldb" clock so that it can filter out unsupported display modes read
> from EDID.
> 
> Patch 5 adds mode validation logic to i.MX8MP LDB bridge driver against
> "pix" clock so that it can filter out display modes which are not
> supported by pixel clock tree.
> 
> Patch 6 adds DT overlays to support NXP adapter cards[1][2] with IT6263
> populated.
> 
> Patch 7 enables the IT6263 bridge driver in defconfig.
> 
> Note that patch 3 and 4 depend on patch[3] in shawnguo/imx/fixes.
> 
> Since this patch series is related to another one[4] authored by Marek,
> Maxime asked for a proper description[5] about the exact problem.
> 
> Admittedly, it's a bit complicated.  Here, I'm trying to do so and explain
> a bit more.
> 
> [ Description ]
> It's a clock problem about shared i.MX8MP video PLL between MIPI DSI and
> LVDS display pipelines.  The pipelines are driven by separate DRM driver
> instances, hence there is no way to negotiate a dynamically changeable
> PLL rate to satisfy both of them.  The only solution is to assign a
> sensible/unchangeable clock rate for the PLL in DT.
> 
> Admittedly, sys_pll3_out can be another clock source to derive pixel clock
> for i.MX8MP MIPI DSI display pipeline if a particalur i.MX8MP platform
> doesn't use audio(sys_pll3_out is supposed to derive audio AXI clock running
> at nominal 600MHz).  However, for i.MX8MP platforms with audio features,
> the shared video PLL case has to be handled and it determines that the above
> solution(unchangeable PLL rate assigned in DT) has to be used no matter
> sys_pll3_out is for display or audio, because the separate DRM driver
> instances really don't know if they are sharing the video PLL or not.
> 
> [[ i.MX8MP Display Hardware ]]
> i.MX8MP SoC supports three display pipelines:
> 
>  -----------------------------           ------------------------
> | imx8mp_media_disp_pix_sels  |         | imx8mp_media_ldb_sels  |
>  -----------------------------           ------------------------
> |  osc_24m (fixed 24MHz)      |         |  osc_24m (fixed 24MHz) |
> |*-video_pll1_out (video)     |         |  sys_pll2_333m (sys)   |
> |  audio_pll2_out (audio)     |         |  sys_pll2_100m (sys)   |
> |  audio_pll1_out (audio)     |         | -sys_pll1_800m (sys)   |
> | -sys_pll1_800m (sys)        |         | -sys_pll2_1000m (sys)  |
> | -sys_pll2_1000m (sys)       |         |  clk_ext2 (external)   |
> |  sys_pll3_out (audio ?)     |         |  audio_pll2_out (audio)|
> |  clk_ext4 (external)        |         |*-video_pll1_out (video)|
>  -----------------------------           ------------------------
>              ||                                     |
>  -----------------------------           ------------------------
> |   media_disp{1,2}_pix       |         |        media_ldb       |
>  ----------------------------- mux+div   ------------------------ mux+div
>              ||                                     |
>  -----------------------------           ------------------------
> | media_disp{1,2}_pix_root_clk|         |   media_ldb_root_clk   |
>  ----------------------------- gate      ------------------------ gate
>              ||                                     | (LVDS serial clock)
>              ||                                     V
> 	     || (Disp2 Pclk)  --------      ------------------
> 	     | ------------> | LCDIF2 | -> |       LDB        | -> panel/bridge
> 	     |                --------      ------------------
> 	     |  (Disp1 Pclk)  --------      ------------------
> 	      -------------> | LCDIF1 | -> | Samsung MIPI DSI | -> panel/bridge
> 	                      --------      ------------------
>                               --------      ------------------      ----------
>                              | LCDIF3 | -> | Synopsys HDMI TX | -> | HDMI PHY |
>                               --------      ------------------     |     +    |
>                                  ^                                 |    PLL   |
>                                  |                                  ----------
>                                  | (Disp3 pclk)                         | |
>                                   --------------------------------------  |
>                                                                           V
>                                                                     panel/bridge
> 
> * video_pll1_out is supposed to be used by video outputs.
> 
> - LCDIF2 + LDB can only use the *same* video_pll1_out, sys_pll1_800m or
>   sys_pll2_1000m.
> 
> [[ i.MX8MP Display Drivers ]]
> LCDIF: drivers/gpu/drm/mxsfb/lcdif_*.c
> Three LCDIFv3 display controllers are driven by three imx-lcdif DRM instances
> separately.
> 
> LDB: drivers/gpu/drm/bridge/fsl-ldb.c
> 
> Samsung MIPI DSI: drivers/gpu/drm/bridge/samsung-dsim.c
> 
> Synopsys HDMI TX: drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> 
> [[ Problem - Shared Video PLL Between Samsung MIPI DSI and LDB ]]
> osc_24m, audio_pll*, sys_pll* and clk_ext* are not for video outputs,
> because:
> a. Aparently, osc_24m runs at fixed 24MHz which is too low for most displays.
> b. Audio subsystem may consume all audio_pll*.
> c. sys_pll* are system clocks which are supposed to run at fixed typical
>    rates, e.g., sys_pll2_1000m runs at 1000MHz.
> d. sys_pll3_out is supposed to derive audio AXI clock running at nominal
>    600MHz(i.MX8MP data sheet specifies the rate), see NXP downstream kernel:
>    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

Is there any downside to using any of these clocks, aside from the fact
that their rate must not change?

Also, if they can't change their rate, why do they have
CLK_SET_RATE_PARENT (sys_pll* in particular) ?

> e. clk_ext* are external clocks without known capabilities.
> 
> So, the only eligible video_pll1_out is supposed to be shared between LDB
> and Samsung MIPI DSI in the two separate display pipelines if sys_pll3_out
> is already used to derive the audio AXI clock.
> 
> With the shared video_pll1_out, drivers for the two display pipelines cannot
> change the PLL clock rate in runtime, since the pipelines are driven by two
> DRM driver instances.

What is the typicall frequency on those pipelines? Could setting the PLL
high enough that any frequency required by any of these pipelines can be
accomodated through a divider work?

Something like you run the PLL at 594MHz, and then most HDMI frequencies
can be reached by a 1, 2 or 4 divider.

> [[ Solution ]]
> Assign the PLL clock source(s) and the PLL clock rate(s) in DT.  Disallow
> display drivers to change the PLL clock source(s) or rate(s) in runtime
> including LCDIF driver and bridge drivers.  With sensible PLL clock rate(s),
> typical display modes like 1920x1080@60 can be supported if external HDMI
> bridges are connected, and panel display modes can be too.  Also the unneeded
> CLK_SET_RATE_PARENT flag can be dropped for media_disp{1,2}_pix clocks.
> If needed, bridge drivers just call clk_round_rate() to validate clocks so
> that unsupported display modes can be filtered out.  Although the
> unchangeable PLL clock rate disallows more potential display modes, the
> solution works for single/dual/triple display pipelines(OFC, hardware designers
> should pick panel/bridge display devices carefully first by considering clock
> resources).

I think it's a reasonable idea, if not for the hardcode-it it DT stuff.
If we can manage to have a fixed setup work ok for all display use
cases, why would it be in DT? The clock driver seems like a much better
choice to me.

Maxime

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

Re: [PATCH v7 0/7] Add ITE IT6263 LVDS to HDMI converter support

[Liu Ying]

On 12/17/2024, Maxime Ripard wrote:
> Hi,

Hi,

> 
> Thanks for the description, I have several questions here.

Thanks for the feedback!

> 
> On Thu, Nov 14, 2024 at 02:57:52PM +0800, Liu Ying wrote:
>> This patch series aims to add ITE IT6263 LVDS to HDMI converter on
>> i.MX8MP EVK.
>>
>> Since IT6263 DT binding and driver were picked up from v5 and landed
>> in drm-misc, this patch series contains patches almost all i.MX8MP
>> SoC/platform specific.
>>
>> Patch 1 is a preparation patch to allow display mode of an existing
>> panel to pass the added mode validation logic in patch 3.
>>
>> Patch 2 is a preparation patch to drop CLK_SET_RATE_PARENT flag for
>> media_disp{1,2}_pix clocks.  Patch 5 depends on patch 2.
>>
>> Patch 3 allows i.MX8MP LVDS Display Bridge(LDB) bridge driver to find
>> the next non-panel bridge, that is the IT6263 in this case.
>>
>> Patch 4 adds mode validation logic to i.MX8MP LDB bridge driver against
>> "ldb" clock so that it can filter out unsupported display modes read
>> from EDID.
>>
>> Patch 5 adds mode validation logic to i.MX8MP LDB bridge driver against
>> "pix" clock so that it can filter out display modes which are not
>> supported by pixel clock tree.
>>
>> Patch 6 adds DT overlays to support NXP adapter cards[1][2] with IT6263
>> populated.
>>
>> Patch 7 enables the IT6263 bridge driver in defconfig.
>>
>> Note that patch 3 and 4 depend on patch[3] in shawnguo/imx/fixes.
>>
>> Since this patch series is related to another one[4] authored by Marek,
>> Maxime asked for a proper description[5] about the exact problem.
>>
>> Admittedly, it's a bit complicated.  Here, I'm trying to do so and explain
>> a bit more.
>>
>> [ Description ]
>> It's a clock problem about shared i.MX8MP video PLL between MIPI DSI and
>> LVDS display pipelines.  The pipelines are driven by separate DRM driver
>> instances, hence there is no way to negotiate a dynamically changeable
>> PLL rate to satisfy both of them.  The only solution is to assign a
>> sensible/unchangeable clock rate for the PLL in DT.
>>
>> Admittedly, sys_pll3_out can be another clock source to derive pixel clock
>> for i.MX8MP MIPI DSI display pipeline if a particalur i.MX8MP platform
>> doesn't use audio(sys_pll3_out is supposed to derive audio AXI clock running
>> at nominal 600MHz).  However, for i.MX8MP platforms with audio features,
>> the shared video PLL case has to be handled and it determines that the above
>> solution(unchangeable PLL rate assigned in DT) has to be used no matter
>> sys_pll3_out is for display or audio, because the separate DRM driver
>> instances really don't know if they are sharing the video PLL or not.
>>
>> [[ i.MX8MP Display Hardware ]]
>> i.MX8MP SoC supports three display pipelines:
>>
>>  -----------------------------           ------------------------
>> | imx8mp_media_disp_pix_sels  |         | imx8mp_media_ldb_sels  |
>>  -----------------------------           ------------------------
>> |  osc_24m (fixed 24MHz)      |         |  osc_24m (fixed 24MHz) |
>> |*-video_pll1_out (video)     |         |  sys_pll2_333m (sys)   |
>> |  audio_pll2_out (audio)     |         |  sys_pll2_100m (sys)   |
>> |  audio_pll1_out (audio)     |         | -sys_pll1_800m (sys)   |
>> | -sys_pll1_800m (sys)        |         | -sys_pll2_1000m (sys)  |
>> | -sys_pll2_1000m (sys)       |         |  clk_ext2 (external)   |
>> |  sys_pll3_out (audio ?)     |         |  audio_pll2_out (audio)|
>> |  clk_ext4 (external)        |         |*-video_pll1_out (video)|
>>  -----------------------------           ------------------------
>>              ||                                     |
>>  -----------------------------           ------------------------
>> |   media_disp{1,2}_pix       |         |        media_ldb       |
>>  ----------------------------- mux+div   ------------------------ mux+div
>>              ||                                     |
>>  -----------------------------           ------------------------
>> | media_disp{1,2}_pix_root_clk|         |   media_ldb_root_clk   |
>>  ----------------------------- gate      ------------------------ gate
>>              ||                                     | (LVDS serial clock)
>>              ||                                     V
>> 	     || (Disp2 Pclk)  --------      ------------------
>> 	     | ------------> | LCDIF2 | -> |       LDB        | -> panel/bridge
>> 	     |                --------      ------------------
>> 	     |  (Disp1 Pclk)  --------      ------------------
>> 	      -------------> | LCDIF1 | -> | Samsung MIPI DSI | -> panel/bridge
>> 	                      --------      ------------------
>>                               --------      ------------------      ----------
>>                              | LCDIF3 | -> | Synopsys HDMI TX | -> | HDMI PHY |
>>                               --------      ------------------     |     +    |
>>                                  ^                                 |    PLL   |
>>                                  |                                  ----------
>>                                  | (Disp3 pclk)                         | |
>>                                   --------------------------------------  |
>>                                                                           V
>>                                                                     panel/bridge
>>
>> * video_pll1_out is supposed to be used by video outputs.
>>
>> - LCDIF2 + LDB can only use the *same* video_pll1_out, sys_pll1_800m or
>>   sys_pll2_1000m.
>>
>> [[ i.MX8MP Display Drivers ]]
>> LCDIF: drivers/gpu/drm/mxsfb/lcdif_*.c
>> Three LCDIFv3 display controllers are driven by three imx-lcdif DRM instances
>> separately.
>>
>> LDB: drivers/gpu/drm/bridge/fsl-ldb.c
>>
>> Samsung MIPI DSI: drivers/gpu/drm/bridge/samsung-dsim.c
>>
>> Synopsys HDMI TX: drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
>>
>> [[ Problem - Shared Video PLL Between Samsung MIPI DSI and LDB ]]
>> osc_24m, audio_pll*, sys_pll* and clk_ext* are not for video outputs,
>> because:
>> a. Aparently, osc_24m runs at fixed 24MHz which is too low for most displays.
>> b. Audio subsystem may consume all audio_pll*.
>> c. sys_pll* are system clocks which are supposed to run at fixed typical
>>    rates, e.g., sys_pll2_1000m runs at 1000MHz.
>> d. sys_pll3_out is supposed to derive audio AXI clock running at nominal
>>    600MHz(i.MX8MP data sheet specifies the rate), see NXP downstream kernel:
>>    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
> 
> Is there any downside to using any of these clocks, aside from the fact
> that their rate must not change?

osc_24m and sys_pll* don't support spread spectrum while audio_pll* and
video_pll1_out support it. Other than that, I don't see any downside to
use any of these clocks, if their clock rates happen to meet the clock
rate requirements of the MIPI DSI and LDB display pipelines.

> 
> Also, if they can't change their rate, why do they have
> CLK_SET_RATE_PARENT (sys_pll* in particular) ?

If media_disp{1,2}_pix and media_ldb clocks have no CLK_SET_RATE_PARENT,
it doesn't mattter whether their parent clocks(these clocks) have it or not.
Note that patch 2 drops CLK_SET_RATE_PARENT for media_disp{1,2}_pix clocks.

Anyway, why have sys_pll* clocks got CLK_SET_RATE_PARENT? The reason I can
think of is that it makes some potential i.MX8MP platforms possible to set
the sys_pll* rates with the parent clock rates(sys_pll1/2/3) updated
accordingly, e.g., if sys_pll2_1000m is the only active child of sys_pll2_out,
sys_pll2_1000m's rate can be assigned to 800MHz(not typical 1000MHz) in DT.
Or, maybe, the sys_pll* rates can be assigned to some particular values to
support nominal/overdrive modes of various i.MX8MP clock roots(some are
derived from the sys_pll* clocks).

> 
>> e. clk_ext* are external clocks without known capabilities.
>>
>> So, the only eligible video_pll1_out is supposed to be shared between LDB
>> and Samsung MIPI DSI in the two separate display pipelines if sys_pll3_out
>> is already used to derive the audio AXI clock.
>>
>> With the shared video_pll1_out, drivers for the two display pipelines cannot
>> change the PLL clock rate in runtime, since the pipelines are driven by two
>> DRM driver instances.
> 
> What is the typicall frequency on those pipelines? Could setting the PLL

For MIPI DSI to HDMI(ADV7535) and LVDS to HDMI(IT6263), the typical pixel
rates are 148.5MHz(1080p60Hz) and 74.25MHz(720p60Hz) and the typical LDB
clock rate is 519.75MHz.

For MIPI DSI panel and LVDS panel, there no typical pixel rates. It depends
on individual panels. However, it's possible to override panel's pixel clock
rate in DT to use a fixed PLL clock rate if the pixel clock rate deviation
is still acceptable by the panel.

> high enough that any frequency required by any of these pipelines can be
> accomodated through a divider work?

Yes, that's why media_blk_ctrl node in imx8mp.dtsi assigns video_pll1 clock
rate to 1.0395GHz. That rate supports the typical 148.5MHz and 74.25MHz
pixel clock rates. With this patch series applied, i.MX8MP EVK would use
this fixed 1.0395GHz video_pll1 clock to drive both MIPI DSI to HDMI and
LVDS to HDMI simultaneously.

> 
> Something like you run the PLL at 594MHz, and then most HDMI frequencies
> can be reached by a 1, 2 or 4 divider.

PLL running at 594MHz does support the typical pixel clock rates for MIPI
DSI to HDMI, but not for LVDS to HDMI due to the 7x(single-LVDS link) or
3.5x(dual-LVDS link) folder between LDB clock rate and pixel clock rate.
PLL running at 1.0395GHz is the one chosen to support both MIPI DSI to
HDMI and LVDS to HDMI, e.g., with dual-LVDS link, 148.5MHz pixel clock rate
= 1.0395GHz / 7 and 519.75MHz LDB clock rate(148.5MHz * 3.5) = 1.0395GHz / 2.

> 
>> [[ Solution ]]
>> Assign the PLL clock source(s) and the PLL clock rate(s) in DT.  Disallow
>> display drivers to change the PLL clock source(s) or rate(s) in runtime
>> including LCDIF driver and bridge drivers.  With sensible PLL clock rate(s),
>> typical display modes like 1920x1080@60 can be supported if external HDMI
>> bridges are connected, and panel display modes can be too.  Also the unneeded
>> CLK_SET_RATE_PARENT flag can be dropped for media_disp{1,2}_pix clocks.
>> If needed, bridge drivers just call clk_round_rate() to validate clocks so
>> that unsupported display modes can be filtered out.  Although the
>> unchangeable PLL clock rate disallows more potential display modes, the
>> solution works for single/dual/triple display pipelines(OFC, hardware designers
>> should pick panel/bridge display devices carefully first by considering clock
>> resources).
> 
> I think it's a reasonable idea, if not for the hardcode-it it DT stuff.
> If we can manage to have a fixed setup work ok for all display use
> cases, why would it be in DT? The clock driver seems like a much better
> choice to me.

Different i.MX8MP platforms may have different display devices(panel(s)
and/or bridge(s)). It's flexible to allow each platform to assign the PLL
rate(s) in DT. It doesn't look doable with clock driver, does it?

> 
> Maxime

-- 
Regards,
Liu Ying

Re: [PATCH v7 0/7] Add ITE IT6263 LVDS to HDMI converter support

[Maxime Ripard]

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

On Wed, Dec 18, 2024 at 02:02:18PM +0800, Liu Ying wrote:
> On 12/17/2024, Maxime Ripard wrote:
> > On Thu, Nov 14, 2024 at 02:57:52PM +0800, Liu Ying wrote:
> >> This patch series aims to add ITE IT6263 LVDS to HDMI converter on
> >> i.MX8MP EVK.
> >>
> >> Since IT6263 DT binding and driver were picked up from v5 and landed
> >> in drm-misc, this patch series contains patches almost all i.MX8MP
> >> SoC/platform specific.
> >>
> >> Patch 1 is a preparation patch to allow display mode of an existing
> >> panel to pass the added mode validation logic in patch 3.
> >>
> >> Patch 2 is a preparation patch to drop CLK_SET_RATE_PARENT flag for
> >> media_disp{1,2}_pix clocks.  Patch 5 depends on patch 2.
> >>
> >> Patch 3 allows i.MX8MP LVDS Display Bridge(LDB) bridge driver to find
> >> the next non-panel bridge, that is the IT6263 in this case.
> >>
> >> Patch 4 adds mode validation logic to i.MX8MP LDB bridge driver against
> >> "ldb" clock so that it can filter out unsupported display modes read
> >> from EDID.
> >>
> >> Patch 5 adds mode validation logic to i.MX8MP LDB bridge driver against
> >> "pix" clock so that it can filter out display modes which are not
> >> supported by pixel clock tree.
> >>
> >> Patch 6 adds DT overlays to support NXP adapter cards[1][2] with IT6263
> >> populated.
> >>
> >> Patch 7 enables the IT6263 bridge driver in defconfig.
> >>
> >> Note that patch 3 and 4 depend on patch[3] in shawnguo/imx/fixes.
> >>
> >> Since this patch series is related to another one[4] authored by Marek,
> >> Maxime asked for a proper description[5] about the exact problem.
> >>
> >> Admittedly, it's a bit complicated.  Here, I'm trying to do so and explain
> >> a bit more.
> >>
> >> [ Description ]
> >> It's a clock problem about shared i.MX8MP video PLL between MIPI DSI and
> >> LVDS display pipelines.  The pipelines are driven by separate DRM driver
> >> instances, hence there is no way to negotiate a dynamically changeable
> >> PLL rate to satisfy both of them.  The only solution is to assign a
> >> sensible/unchangeable clock rate for the PLL in DT.
> >>
> >> Admittedly, sys_pll3_out can be another clock source to derive pixel clock
> >> for i.MX8MP MIPI DSI display pipeline if a particalur i.MX8MP platform
> >> doesn't use audio(sys_pll3_out is supposed to derive audio AXI clock running
> >> at nominal 600MHz).  However, for i.MX8MP platforms with audio features,
> >> the shared video PLL case has to be handled and it determines that the above
> >> solution(unchangeable PLL rate assigned in DT) has to be used no matter
> >> sys_pll3_out is for display or audio, because the separate DRM driver
> >> instances really don't know if they are sharing the video PLL or not.
> >>
> >> [[ i.MX8MP Display Hardware ]]
> >> i.MX8MP SoC supports three display pipelines:
> >>
> >>  -----------------------------           ------------------------
> >> | imx8mp_media_disp_pix_sels  |         | imx8mp_media_ldb_sels  |
> >>  -----------------------------           ------------------------
> >> |  osc_24m (fixed 24MHz)      |         |  osc_24m (fixed 24MHz) |
> >> |*-video_pll1_out (video)     |         |  sys_pll2_333m (sys)   |
> >> |  audio_pll2_out (audio)     |         |  sys_pll2_100m (sys)   |
> >> |  audio_pll1_out (audio)     |         | -sys_pll1_800m (sys)   |
> >> | -sys_pll1_800m (sys)        |         | -sys_pll2_1000m (sys)  |
> >> | -sys_pll2_1000m (sys)       |         |  clk_ext2 (external)   |
> >> |  sys_pll3_out (audio ?)     |         |  audio_pll2_out (audio)|
> >> |  clk_ext4 (external)        |         |*-video_pll1_out (video)|
> >>  -----------------------------           ------------------------
> >>              ||                                     |
> >>  -----------------------------           ------------------------
> >> |   media_disp{1,2}_pix       |         |        media_ldb       |
> >>  ----------------------------- mux+div   ------------------------ mux+div
> >>              ||                                     |
> >>  -----------------------------           ------------------------
> >> | media_disp{1,2}_pix_root_clk|         |   media_ldb_root_clk   |
> >>  ----------------------------- gate      ------------------------ gate
> >>              ||                                     | (LVDS serial clock)
> >>              ||                                     V
> >> 	     || (Disp2 Pclk)  --------      ------------------
> >> 	     | ------------> | LCDIF2 | -> |       LDB        | -> panel/bridge
> >> 	     |                --------      ------------------
> >> 	     |  (Disp1 Pclk)  --------      ------------------
> >> 	      -------------> | LCDIF1 | -> | Samsung MIPI DSI | -> panel/bridge
> >> 	                      --------      ------------------
> >>                               --------      ------------------      ----------
> >>                              | LCDIF3 | -> | Synopsys HDMI TX | -> | HDMI PHY |
> >>                               --------      ------------------     |     +    |
> >>                                  ^                                 |    PLL   |
> >>                                  |                                  ----------
> >>                                  | (Disp3 pclk)                         | |
> >>                                   --------------------------------------  |
> >>                                                                           V
> >>                                                                     panel/bridge
> >>
> >> * video_pll1_out is supposed to be used by video outputs.
> >>
> >> - LCDIF2 + LDB can only use the *same* video_pll1_out, sys_pll1_800m or
> >>   sys_pll2_1000m.
> >>
> >> [[ i.MX8MP Display Drivers ]]
> >> LCDIF: drivers/gpu/drm/mxsfb/lcdif_*.c
> >> Three LCDIFv3 display controllers are driven by three imx-lcdif DRM instances
> >> separately.
> >>
> >> LDB: drivers/gpu/drm/bridge/fsl-ldb.c
> >>
> >> Samsung MIPI DSI: drivers/gpu/drm/bridge/samsung-dsim.c
> >>
> >> Synopsys HDMI TX: drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
> >>
> >> [[ Problem - Shared Video PLL Between Samsung MIPI DSI and LDB ]]
> >> osc_24m, audio_pll*, sys_pll* and clk_ext* are not for video outputs,
> >> because:
> >> a. Aparently, osc_24m runs at fixed 24MHz which is too low for most displays.
> >> b. Audio subsystem may consume all audio_pll*.
> >> c. sys_pll* are system clocks which are supposed to run at fixed typical
> >>    rates, e.g., sys_pll2_1000m runs at 1000MHz.
> >> d. sys_pll3_out is supposed to derive audio AXI clock running at nominal
> >>    600MHz(i.MX8MP data sheet specifies the rate), see NXP downstream kernel:
> >>    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
> > 
> > Is there any downside to using any of these clocks, aside from the fact
> > that their rate must not change?
> 
> osc_24m and sys_pll* don't support spread spectrum while audio_pll* and
> video_pll1_out support it. Other than that, I don't see any downside to
> use any of these clocks, if their clock rates happen to meet the clock
> rate requirements of the MIPI DSI and LDB display pipelines.
> 
> > 
> > Also, if they can't change their rate, why do they have
> > CLK_SET_RATE_PARENT (sys_pll* in particular) ?
> 
> If media_disp{1,2}_pix and media_ldb clocks have no CLK_SET_RATE_PARENT,
> it doesn't mattter whether their parent clocks(these clocks) have it or not.
> Note that patch 2 drops CLK_SET_RATE_PARENT for media_disp{1,2}_pix clocks.
> 
> Anyway, why have sys_pll* clocks got CLK_SET_RATE_PARENT? The reason I can
> think of is that it makes some potential i.MX8MP platforms possible to set
> the sys_pll* rates with the parent clock rates(sys_pll1/2/3) updated
> accordingly, e.g., if sys_pll2_1000m is the only active child of sys_pll2_out,
> sys_pll2_1000m's rate can be assigned to 800MHz(not typical 1000MHz) in DT.
> Or, maybe, the sys_pll* rates can be assigned to some particular values to
> support nominal/overdrive modes of various i.MX8MP clock roots(some are
> derived from the sys_pll* clocks).
> 
> > 
> >> e. clk_ext* are external clocks without known capabilities.
> >>
> >> So, the only eligible video_pll1_out is supposed to be shared between LDB
> >> and Samsung MIPI DSI in the two separate display pipelines if sys_pll3_out
> >> is already used to derive the audio AXI clock.
> >>
> >> With the shared video_pll1_out, drivers for the two display pipelines cannot
> >> change the PLL clock rate in runtime, since the pipelines are driven by two
> >> DRM driver instances.
> > 
> > What is the typicall frequency on those pipelines? Could setting the PLL
> 
> For MIPI DSI to HDMI(ADV7535) and LVDS to HDMI(IT6263), the typical pixel
> rates are 148.5MHz(1080p60Hz) and 74.25MHz(720p60Hz) and the typical LDB
> clock rate is 519.75MHz.
> 
> For MIPI DSI panel and LVDS panel, there no typical pixel rates. It depends
> on individual panels. However, it's possible to override panel's pixel clock
> rate in DT to use a fixed PLL clock rate if the pixel clock rate deviation
> is still acceptable by the panel.
> 
> > high enough that any frequency required by any of these pipelines can be
> > accomodated through a divider work?
> 
> Yes, that's why media_blk_ctrl node in imx8mp.dtsi assigns video_pll1 clock
> rate to 1.0395GHz. That rate supports the typical 148.5MHz and 74.25MHz
> pixel clock rates. With this patch series applied, i.MX8MP EVK would use
> this fixed 1.0395GHz video_pll1 clock to drive both MIPI DSI to HDMI and
> LVDS to HDMI simultaneously.
> 
> > 
> > Something like you run the PLL at 594MHz, and then most HDMI frequencies
> > can be reached by a 1, 2 or 4 divider.
> 
> PLL running at 594MHz does support the typical pixel clock rates for MIPI
> DSI to HDMI, but not for LVDS to HDMI due to the 7x(single-LVDS link) or
> 3.5x(dual-LVDS link) folder between LDB clock rate and pixel clock rate.
> PLL running at 1.0395GHz is the one chosen to support both MIPI DSI to
> HDMI and LVDS to HDMI, e.g., with dual-LVDS link, 148.5MHz pixel clock rate
> = 1.0395GHz / 7 and 519.75MHz LDB clock rate(148.5MHz * 3.5) = 1.0395GHz / 2.
> 
> > 
> >> [[ Solution ]]
> >> Assign the PLL clock source(s) and the PLL clock rate(s) in DT.  Disallow
> >> display drivers to change the PLL clock source(s) or rate(s) in runtime
> >> including LCDIF driver and bridge drivers.  With sensible PLL clock rate(s),
> >> typical display modes like 1920x1080@60 can be supported if external HDMI
> >> bridges are connected, and panel display modes can be too.  Also the unneeded
> >> CLK_SET_RATE_PARENT flag can be dropped for media_disp{1,2}_pix clocks.
> >> If needed, bridge drivers just call clk_round_rate() to validate clocks so
> >> that unsupported display modes can be filtered out.  Although the
> >> unchangeable PLL clock rate disallows more potential display modes, the
> >> solution works for single/dual/triple display pipelines(OFC, hardware designers
> >> should pick panel/bridge display devices carefully first by considering clock
> >> resources).
> > 
> > I think it's a reasonable idea, if not for the hardcode-it it DT stuff.
> > If we can manage to have a fixed setup work ok for all display use
> > cases, why would it be in DT? The clock driver seems like a much better
> > choice to me.
> 
> Different i.MX8MP platforms may have different display devices(panel(s)
> and/or bridge(s)). It's flexible to allow each platform to assign the PLL
> rate(s) in DT. It doesn't look doable with clock driver, does it?

Why not? Plenty of platforms are doing it. If anything, it's more
flexible to deal with it in the clock driver, because then you don't
have to filter out modes that aren't compatible with the frequency that
got assigned in the DT.

Also, it's more robust since that frequency isn't guaranteed to be
constant across the system life.

Maxime

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

Re: [PATCH v7 0/7] Add ITE IT6263 LVDS to HDMI converter support

[Liu Ying]

On 01/28/2025, Maxime Ripard wrote:
> On Wed, Dec 18, 2024 at 02:02:18PM +0800, Liu Ying wrote:
>> On 12/17/2024, Maxime Ripard wrote:
>>> On Thu, Nov 14, 2024 at 02:57:52PM +0800, Liu Ying wrote:
>>>> This patch series aims to add ITE IT6263 LVDS to HDMI converter on
>>>> i.MX8MP EVK.
>>>>
>>>> Since IT6263 DT binding and driver were picked up from v5 and landed
>>>> in drm-misc, this patch series contains patches almost all i.MX8MP
>>>> SoC/platform specific.
>>>>
>>>> Patch 1 is a preparation patch to allow display mode of an existing
>>>> panel to pass the added mode validation logic in patch 3.
>>>>
>>>> Patch 2 is a preparation patch to drop CLK_SET_RATE_PARENT flag for
>>>> media_disp{1,2}_pix clocks.  Patch 5 depends on patch 2.
>>>>
>>>> Patch 3 allows i.MX8MP LVDS Display Bridge(LDB) bridge driver to find
>>>> the next non-panel bridge, that is the IT6263 in this case.
>>>>
>>>> Patch 4 adds mode validation logic to i.MX8MP LDB bridge driver against
>>>> "ldb" clock so that it can filter out unsupported display modes read
>>>> from EDID.
>>>>
>>>> Patch 5 adds mode validation logic to i.MX8MP LDB bridge driver against
>>>> "pix" clock so that it can filter out display modes which are not
>>>> supported by pixel clock tree.
>>>>
>>>> Patch 6 adds DT overlays to support NXP adapter cards[1][2] with IT6263
>>>> populated.
>>>>
>>>> Patch 7 enables the IT6263 bridge driver in defconfig.
>>>>
>>>> Note that patch 3 and 4 depend on patch[3] in shawnguo/imx/fixes.
>>>>
>>>> Since this patch series is related to another one[4] authored by Marek,
>>>> Maxime asked for a proper description[5] about the exact problem.
>>>>
>>>> Admittedly, it's a bit complicated.  Here, I'm trying to do so and explain
>>>> a bit more.
>>>>
>>>> [ Description ]
>>>> It's a clock problem about shared i.MX8MP video PLL between MIPI DSI and
>>>> LVDS display pipelines.  The pipelines are driven by separate DRM driver
>>>> instances, hence there is no way to negotiate a dynamically changeable
>>>> PLL rate to satisfy both of them.  The only solution is to assign a
>>>> sensible/unchangeable clock rate for the PLL in DT.
>>>>
>>>> Admittedly, sys_pll3_out can be another clock source to derive pixel clock
>>>> for i.MX8MP MIPI DSI display pipeline if a particalur i.MX8MP platform
>>>> doesn't use audio(sys_pll3_out is supposed to derive audio AXI clock running
>>>> at nominal 600MHz).  However, for i.MX8MP platforms with audio features,
>>>> the shared video PLL case has to be handled and it determines that the above
>>>> solution(unchangeable PLL rate assigned in DT) has to be used no matter
>>>> sys_pll3_out is for display or audio, because the separate DRM driver
>>>> instances really don't know if they are sharing the video PLL or not.
>>>>
>>>> [[ i.MX8MP Display Hardware ]]
>>>> i.MX8MP SoC supports three display pipelines:
>>>>
>>>>  -----------------------------           ------------------------
>>>> | imx8mp_media_disp_pix_sels  |         | imx8mp_media_ldb_sels  |
>>>>  -----------------------------           ------------------------
>>>> |  osc_24m (fixed 24MHz)      |         |  osc_24m (fixed 24MHz) |
>>>> |*-video_pll1_out (video)     |         |  sys_pll2_333m (sys)   |
>>>> |  audio_pll2_out (audio)     |         |  sys_pll2_100m (sys)   |
>>>> |  audio_pll1_out (audio)     |         | -sys_pll1_800m (sys)   |
>>>> | -sys_pll1_800m (sys)        |         | -sys_pll2_1000m (sys)  |
>>>> | -sys_pll2_1000m (sys)       |         |  clk_ext2 (external)   |
>>>> |  sys_pll3_out (audio ?)     |         |  audio_pll2_out (audio)|
>>>> |  clk_ext4 (external)        |         |*-video_pll1_out (video)|
>>>>  -----------------------------           ------------------------
>>>>              ||                                     |
>>>>  -----------------------------           ------------------------
>>>> |   media_disp{1,2}_pix       |         |        media_ldb       |
>>>>  ----------------------------- mux+div   ------------------------ mux+div
>>>>              ||                                     |
>>>>  -----------------------------           ------------------------
>>>> | media_disp{1,2}_pix_root_clk|         |   media_ldb_root_clk   |
>>>>  ----------------------------- gate      ------------------------ gate
>>>>              ||                                     | (LVDS serial clock)
>>>>              ||                                     V
>>>> 	     || (Disp2 Pclk)  --------      ------------------
>>>> 	     | ------------> | LCDIF2 | -> |       LDB        | -> panel/bridge
>>>> 	     |                --------      ------------------
>>>> 	     |  (Disp1 Pclk)  --------      ------------------
>>>> 	      -------------> | LCDIF1 | -> | Samsung MIPI DSI | -> panel/bridge
>>>> 	                      --------      ------------------
>>>>                               --------      ------------------      ----------
>>>>                              | LCDIF3 | -> | Synopsys HDMI TX | -> | HDMI PHY |
>>>>                               --------      ------------------     |     +    |
>>>>                                  ^                                 |    PLL   |
>>>>                                  |                                  ----------
>>>>                                  | (Disp3 pclk)                         | |
>>>>                                   --------------------------------------  |
>>>>                                                                           V
>>>>                                                                     panel/bridge
>>>>
>>>> * video_pll1_out is supposed to be used by video outputs.
>>>>
>>>> - LCDIF2 + LDB can only use the *same* video_pll1_out, sys_pll1_800m or
>>>>   sys_pll2_1000m.
>>>>
>>>> [[ i.MX8MP Display Drivers ]]
>>>> LCDIF: drivers/gpu/drm/mxsfb/lcdif_*.c
>>>> Three LCDIFv3 display controllers are driven by three imx-lcdif DRM instances
>>>> separately.
>>>>
>>>> LDB: drivers/gpu/drm/bridge/fsl-ldb.c
>>>>
>>>> Samsung MIPI DSI: drivers/gpu/drm/bridge/samsung-dsim.c
>>>>
>>>> Synopsys HDMI TX: drivers/gpu/drm/bridge/imx/imx8mp-hdmi-tx.c
>>>>
>>>> [[ Problem - Shared Video PLL Between Samsung MIPI DSI and LDB ]]
>>>> osc_24m, audio_pll*, sys_pll* and clk_ext* are not for video outputs,
>>>> because:
>>>> a. Aparently, osc_24m runs at fixed 24MHz which is too low for most displays.
>>>> b. Audio subsystem may consume all audio_pll*.
>>>> c. sys_pll* are system clocks which are supposed to run at fixed typical
>>>>    rates, e.g., sys_pll2_1000m runs at 1000MHz.
>>>> d. sys_pll3_out is supposed to derive audio AXI clock running at nominal
>>>>    600MHz(i.MX8MP data sheet specifies the rate), see NXP downstream kernel:
>>>>    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
>>>
>>> Is there any downside to using any of these clocks, aside from the fact
>>> that their rate must not change?
>>
>> osc_24m and sys_pll* don't support spread spectrum while audio_pll* and
>> video_pll1_out support it. Other than that, I don't see any downside to
>> use any of these clocks, if their clock rates happen to meet the clock
>> rate requirements of the MIPI DSI and LDB display pipelines.
>>
>>>
>>> Also, if they can't change their rate, why do they have
>>> CLK_SET_RATE_PARENT (sys_pll* in particular) ?
>>
>> If media_disp{1,2}_pix and media_ldb clocks have no CLK_SET_RATE_PARENT,
>> it doesn't mattter whether their parent clocks(these clocks) have it or not.
>> Note that patch 2 drops CLK_SET_RATE_PARENT for media_disp{1,2}_pix clocks.
>>
>> Anyway, why have sys_pll* clocks got CLK_SET_RATE_PARENT? The reason I can
>> think of is that it makes some potential i.MX8MP platforms possible to set
>> the sys_pll* rates with the parent clock rates(sys_pll1/2/3) updated
>> accordingly, e.g., if sys_pll2_1000m is the only active child of sys_pll2_out,
>> sys_pll2_1000m's rate can be assigned to 800MHz(not typical 1000MHz) in DT.
>> Or, maybe, the sys_pll* rates can be assigned to some particular values to
>> support nominal/overdrive modes of various i.MX8MP clock roots(some are
>> derived from the sys_pll* clocks).
>>
>>>
>>>> e. clk_ext* are external clocks without known capabilities.
>>>>
>>>> So, the only eligible video_pll1_out is supposed to be shared between LDB
>>>> and Samsung MIPI DSI in the two separate display pipelines if sys_pll3_out
>>>> is already used to derive the audio AXI clock.
>>>>
>>>> With the shared video_pll1_out, drivers for the two display pipelines cannot
>>>> change the PLL clock rate in runtime, since the pipelines are driven by two
>>>> DRM driver instances.
>>>
>>> What is the typicall frequency on those pipelines? Could setting the PLL
>>
>> For MIPI DSI to HDMI(ADV7535) and LVDS to HDMI(IT6263), the typical pixel
>> rates are 148.5MHz(1080p60Hz) and 74.25MHz(720p60Hz) and the typical LDB
>> clock rate is 519.75MHz.
>>
>> For MIPI DSI panel and LVDS panel, there no typical pixel rates. It depends
>> on individual panels. However, it's possible to override panel's pixel clock
>> rate in DT to use a fixed PLL clock rate if the pixel clock rate deviation
>> is still acceptable by the panel.
>>
>>> high enough that any frequency required by any of these pipelines can be
>>> accomodated through a divider work?
>>
>> Yes, that's why media_blk_ctrl node in imx8mp.dtsi assigns video_pll1 clock
>> rate to 1.0395GHz. That rate supports the typical 148.5MHz and 74.25MHz
>> pixel clock rates. With this patch series applied, i.MX8MP EVK would use
>> this fixed 1.0395GHz video_pll1 clock to drive both MIPI DSI to HDMI and
>> LVDS to HDMI simultaneously.
>>
>>>
>>> Something like you run the PLL at 594MHz, and then most HDMI frequencies
>>> can be reached by a 1, 2 or 4 divider.
>>
>> PLL running at 594MHz does support the typical pixel clock rates for MIPI
>> DSI to HDMI, but not for LVDS to HDMI due to the 7x(single-LVDS link) or
>> 3.5x(dual-LVDS link) folder between LDB clock rate and pixel clock rate.
>> PLL running at 1.0395GHz is the one chosen to support both MIPI DSI to
>> HDMI and LVDS to HDMI, e.g., with dual-LVDS link, 148.5MHz pixel clock rate
>> = 1.0395GHz / 7 and 519.75MHz LDB clock rate(148.5MHz * 3.5) = 1.0395GHz / 2.
>>
>>>
>>>> [[ Solution ]]
>>>> Assign the PLL clock source(s) and the PLL clock rate(s) in DT.  Disallow
>>>> display drivers to change the PLL clock source(s) or rate(s) in runtime
>>>> including LCDIF driver and bridge drivers.  With sensible PLL clock rate(s),
>>>> typical display modes like 1920x1080@60 can be supported if external HDMI
>>>> bridges are connected, and panel display modes can be too.  Also the unneeded
>>>> CLK_SET_RATE_PARENT flag can be dropped for media_disp{1,2}_pix clocks.
>>>> If needed, bridge drivers just call clk_round_rate() to validate clocks so
>>>> that unsupported display modes can be filtered out.  Although the
>>>> unchangeable PLL clock rate disallows more potential display modes, the
>>>> solution works for single/dual/triple display pipelines(OFC, hardware designers
>>>> should pick panel/bridge display devices carefully first by considering clock
>>>> resources).
>>>
>>> I think it's a reasonable idea, if not for the hardcode-it it DT stuff.
>>> If we can manage to have a fixed setup work ok for all display use
>>> cases, why would it be in DT? The clock driver seems like a much better
>>> choice to me.
>>
>> Different i.MX8MP platforms may have different display devices(panel(s)
>> and/or bridge(s)). It's flexible to allow each platform to assign the PLL
>> rate(s) in DT. It doesn't look doable with clock driver, does it?
> 
> Why not? Plenty of platforms are doing it. If anything, it's more

Keep CLK_SET_RATE_PARENT flag for IMX8MP_CLK_MEDIA_DISP{1,2}_PIX clocks, check
compatible machines and call clk_set_rate_exclusive() in imx8mp_clocks_probe()
like the below snippet ?  This is the only way I can think of to fix the
video_pll1 clock rate in the clock driver.

-8<-
if (of_machine_is_compatible("fsl,imx8mp-evk")) {
	err = clk_set_rate_exclusive(hws[IMX8MP_VIDEO_PLL1]->clk, 1039500000);
	if (err < 0) {
		dev_err(dev, "failed to set exclusive rate for video_pll1: %d\n", err);
			return err;
 	}
}
-8<-

However, it's not as flexible as assigning the video_pll1 clock rate in DT,
because the i.MX8MP EVK board exposes it's MIPI DSI and LVDS connectors through
MINI-SAS connectors(find MINI-SAS connectors in the board image at
https://www.nxp.com/design/design-center/development-boards-and-designs/8MPLUSLPD4-EVK)
and people may connect various sensible MIPI DSI/LVDS panels or bridges with
MIPI DSI/LVDS input interfaces as they want(not necessarily the panels and
bridges provided by NXP).  Hardcoding the PLL clock rate in the
clk_set_rate_exclusive() function call is really not flexible.  Instead, people
may flexibly assign a sensible PLL clock rate according to the actual connected
display devices.

> flexible to deal with it in the clock driver, because then you don't
> have to filter out modes that aren't compatible with the frequency that
> got assigned in the DT.

I still need to filter out unsupported modes(due to clock constraints) in DRM
driver even if I use the above snippet in imx8mp_clocks_probe(), e.g., the below
two modes with 27MHz pixel clock rate need to be filtered out for IT6263 LVDS to
HDMI bridge with dual LVDS links because the 1.0395GHz PLL rate cannot satisfy
the 27MHZ pixel clock rate.

#7 720x576 50.00 720 732 796 864 576 581 586 625 27000 flags: nhsync, nvsync; type: driver
#8 720x480 59.94 720 736 798 858 480 489 495 525 27000 flags: nhsync, nvsync; type: driver

> 
> Also, it's more robust since that frequency isn't guaranteed to be
> constant across the system life.

If we drop CLK_SET_RATE_PARENT flag for IMX8MP_CLK_MEDIA_DISP{1,2}_PIX clocks
and assign the PLL rate(s) in DT, I don't think the PLL rate(s) would change
across the system life.

> 
> Maxime

-- 
Regards,
Liu Ying