[PATCH 0/9] drm/bridge: imx: Add i.MX93 MIPI DSI support

[PATCH 0/9] drm/bridge: imx: Add i.MX93 MIPI DSI support

[Liu Ying]

Hi,

This series aims to add MIPI DSI support for Freescale i.MX93 SoC.

There is a Synopsys DesignWare MIPI DSI host controller and a Synopsys
Designware MIPI DPHY embedded in i.MX93.  Some configurations and
extensions to them are controlled by i.MX93 media blk-ctrl.

Add a DRM bridge for i.MX93 MIPI DSI by using existing DW MIPI DSI
bridge helpers and implementing i.MX93 MIPI DSI specific extensions.

Note that since this series touches the dw-mipi-dsi driver, tests are
needed to be done for meson, rockchip and stm.

Patch 1 ~ 7 do preparation work for adding i.MX93 MIPI DSI DRM bridge driver.

Patch 8 adds DT-binding documentation for i.MX93 MIPI DSI.

Patch 9 adds i.MX93 MIPI DSI DRM bridge.

Liu Ying (9):
  drm/bridge: synopsys: dw-mipi-dsi: Add dw_mipi_dsi_get_bridge() helper
  drm/bridge: synopsys: dw-mipi-dsi: Add input bus format negotiation
    support
  drm/bridge: synopsys: dw-mipi-dsi: Force input bus flags
  drm/bridge: synopsys: dw-mipi-dsi: Add mode fixup support
  drm/bridge: synopsys: dw-mipi-dsi: Use pixel clock rate to calculate
    lbcc
  drm/bridge: synopsys: dw-mipi-dsi: Set minimum lane byte clock cycles
    for HSA and HBP
  drm/bridge: synopsys: dw-mipi-dsi: Disable HSTX and LPRX timeout check
  dt-bindings: display: bridge: Document Freescale i.MX93 MIPI DSI
  drm/bridge: imx: Add i.MX93 MIPI DSI support

 .../display/bridge/fsl,imx93-mipi-dsi.yaml    | 115 +++
 drivers/gpu/drm/bridge/imx/Kconfig            |  10 +
 drivers/gpu/drm/bridge/imx/Makefile           |   1 +
 drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c   | 934 ++++++++++++++++++
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c |  91 +-
 include/drm/bridge/dw_mipi_dsi.h              |  16 +
 6 files changed, 1163 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/fsl,imx93-mipi-dsi.yaml
 create mode 100644 drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c

-- 
2.37.1

[PATCH 1/9] drm/bridge: synopsys: dw-mipi-dsi: Add dw_mipi_dsi_get_bridge() helper

[Liu Ying]

Add dw_mipi_dsi_get_bridge() helper so that it can be used by vendor
drivers which implement vendor specific extensions to Synopsys DW MIPI DSI.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 6 ++++++
 include/drm/bridge/dw_mipi_dsi.h              | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index b2efecf7d160..57eae0fdd970 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -1207,6 +1207,12 @@ void dw_mipi_dsi_set_slave(struct dw_mipi_dsi *dsi, struct dw_mipi_dsi *slave)
 }
 EXPORT_SYMBOL_GPL(dw_mipi_dsi_set_slave);
 
+struct drm_bridge *dw_mipi_dsi_get_bridge(struct dw_mipi_dsi *dsi)
+{
+	return &dsi->bridge;
+}
+EXPORT_SYMBOL_GPL(dw_mipi_dsi_get_bridge);
+
 /*
  * Probe/remove API, used from platforms based on the DRM bridge API.
  */
diff --git a/include/drm/bridge/dw_mipi_dsi.h b/include/drm/bridge/dw_mipi_dsi.h
index 5286a53a1875..f54621b17a69 100644
--- a/include/drm/bridge/dw_mipi_dsi.h
+++ b/include/drm/bridge/dw_mipi_dsi.h
@@ -11,6 +11,7 @@
 
 #include <linux/types.h>
 
+#include <drm/drm_bridge.h>
 #include <drm/drm_modes.h>
 
 struct drm_display_mode;
@@ -68,5 +69,6 @@ void dw_mipi_dsi_remove(struct dw_mipi_dsi *dsi);
 int dw_mipi_dsi_bind(struct dw_mipi_dsi *dsi, struct drm_encoder *encoder);
 void dw_mipi_dsi_unbind(struct dw_mipi_dsi *dsi);
 void dw_mipi_dsi_set_slave(struct dw_mipi_dsi *dsi, struct dw_mipi_dsi *slave);
+struct drm_bridge *dw_mipi_dsi_get_bridge(struct dw_mipi_dsi *dsi);
 
 #endif /* __DW_MIPI_DSI__ */
-- 
2.37.1

[PATCH 2/9] drm/bridge: synopsys: dw-mipi-dsi: Add input bus format negotiation support

[Liu Ying]

Introduce ->get_input_bus_fmts() callback to struct dw_mipi_dsi_plat_data
so that vendor drivers can implement specific methods to get input bus
formats for Synopsys DW MIPI DSI.

While at it, implement a generic callback for ->atomic_get_input_bus_fmts(),
where we try to get the input bus formats through pdata->get_input_bus_fmts()
first.  If it's unavailable, fall back to the only format - MEDIA_BUS_FMT_FIXED,
which matches the default behavior if ->atomic_get_input_bus_fmts() is not
implemented as ->atomic_get_input_bus_fmts()'s kerneldoc indicates.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 30 +++++++++++++++++++
 include/drm/bridge/dw_mipi_dsi.h              | 11 +++++++
 2 files changed, 41 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index 57eae0fdd970..8580b8a97fb1 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -12,6 +12,7 @@
 #include <linux/component.h>
 #include <linux/debugfs.h>
 #include <linux/iopoll.h>
+#include <linux/media-bus-format.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
 #include <linux/pm_runtime.h>
@@ -536,6 +537,34 @@ static const struct mipi_dsi_host_ops dw_mipi_dsi_host_ops = {
 	.transfer = dw_mipi_dsi_host_transfer,
 };
 
+static u32 *
+dw_mipi_dsi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
+					     struct drm_bridge_state *bridge_state,
+					     struct drm_crtc_state *crtc_state,
+					     struct drm_connector_state *conn_state,
+					     u32 output_fmt,
+					     unsigned int *num_input_fmts)
+{
+	struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge);
+	const struct dw_mipi_dsi_plat_data *pdata = dsi->plat_data;
+	u32 *input_fmts;
+
+	if (pdata->get_input_bus_fmts)
+		return pdata->get_input_bus_fmts(pdata->priv_data,
+						 bridge, bridge_state,
+						 crtc_state, conn_state,
+						 output_fmt, num_input_fmts);
+
+	/* Fall back to MEDIA_BUS_FMT_FIXED as the only input format. */
+	input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
+	if (!input_fmts)
+		return NULL;
+	input_fmts[0] = MEDIA_BUS_FMT_FIXED;
+	*num_input_fmts = 1;
+
+	return input_fmts;
+}
+
 static void dw_mipi_dsi_video_mode_config(struct dw_mipi_dsi *dsi)
 {
 	u32 val;
@@ -1003,6 +1032,7 @@ static int dw_mipi_dsi_bridge_attach(struct drm_bridge *bridge,
 static const struct drm_bridge_funcs dw_mipi_dsi_bridge_funcs = {
 	.atomic_duplicate_state	= drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state	= drm_atomic_helper_bridge_destroy_state,
+	.atomic_get_input_bus_fmts = dw_mipi_dsi_bridge_atomic_get_input_bus_fmts,
 	.atomic_reset		= drm_atomic_helper_bridge_reset,
 	.atomic_enable		= dw_mipi_dsi_bridge_atomic_enable,
 	.atomic_post_disable	= dw_mipi_dsi_bridge_post_atomic_disable,
diff --git a/include/drm/bridge/dw_mipi_dsi.h b/include/drm/bridge/dw_mipi_dsi.h
index f54621b17a69..246650f2814f 100644
--- a/include/drm/bridge/dw_mipi_dsi.h
+++ b/include/drm/bridge/dw_mipi_dsi.h
@@ -11,7 +11,10 @@
 
 #include <linux/types.h>
 
+#include <drm/drm_atomic.h>
 #include <drm/drm_bridge.h>
+#include <drm/drm_connector.h>
+#include <drm/drm_crtc.h>
 #include <drm/drm_modes.h>
 
 struct drm_display_mode;
@@ -56,6 +59,14 @@ struct dw_mipi_dsi_plat_data {
 					   unsigned long mode_flags,
 					   u32 lanes, u32 format);
 
+	u32 *(*get_input_bus_fmts)(void *priv_data,
+				   struct drm_bridge *bridge,
+				   struct drm_bridge_state *bridge_state,
+				   struct drm_crtc_state *crtc_state,
+				   struct drm_connector_state *conn_state,
+				   u32 output_fmt,
+				   unsigned int *num_input_fmts);
+
 	const struct dw_mipi_dsi_phy_ops *phy_ops;
 	const struct dw_mipi_dsi_host_ops *host_ops;
 
-- 
2.37.1

[PATCH 3/9] drm/bridge: synopsys: dw-mipi-dsi: Force input bus flags

[Liu Ying]

The DATAEN_ACTIVE_LOW bit in DSI_DPI_CFG_POL register is set to zero,
so set the DRM_BUS_FLAG_DE_HIGH flag in input_bus_cfg.flags.  It appears
that the DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE flag also makes sense, so
set it in input_bus_cfg.flags too.  With this patch, the flags set by
drm_atomic_bridge_propagate_bus_flags() are overridden (see comment in
that function) in case any downstream bridges propagates invalid flags
to this bridge.  A real problematic case is to connect a RM67191 MIPI
DSI panel whose driver sets DRM_BUS_FLAG_DE_LOW and
DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE bus flags.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index 8580b8a97fb1..8cd89a63b5f2 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -23,6 +23,7 @@
 #include <drm/bridge/dw_mipi_dsi.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
+#include <drm/drm_connector.h>
 #include <drm/drm_crtc.h>
 #include <drm/drm_mipi_dsi.h>
 #include <drm/drm_modes.h>
@@ -565,6 +566,17 @@ dw_mipi_dsi_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
 	return input_fmts;
 }
 
+static int dw_mipi_dsi_bridge_atomic_check(struct drm_bridge *bridge,
+					   struct drm_bridge_state *bridge_state,
+					   struct drm_crtc_state *crtc_state,
+					   struct drm_connector_state *conn_state)
+{
+	bridge_state->input_bus_cfg.flags =
+		DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE;
+
+	return 0;
+}
+
 static void dw_mipi_dsi_video_mode_config(struct dw_mipi_dsi *dsi)
 {
 	u32 val;
@@ -1033,6 +1045,7 @@ static const struct drm_bridge_funcs dw_mipi_dsi_bridge_funcs = {
 	.atomic_duplicate_state	= drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state	= drm_atomic_helper_bridge_destroy_state,
 	.atomic_get_input_bus_fmts = dw_mipi_dsi_bridge_atomic_get_input_bus_fmts,
+	.atomic_check		= dw_mipi_dsi_bridge_atomic_check,
 	.atomic_reset		= drm_atomic_helper_bridge_reset,
 	.atomic_enable		= dw_mipi_dsi_bridge_atomic_enable,
 	.atomic_post_disable	= dw_mipi_dsi_bridge_post_atomic_disable,
-- 
2.37.1

[PATCH 4/9] drm/bridge: synopsys: dw-mipi-dsi: Add mode fixup support

[Liu Ying]

Vendor drivers may need to fixup mode due to pixel clock tree limitation,
so introduce the ->mode_fixup() callcack to struct dw_mipi_dsi_plat_data
and call it at atomic check stage if available.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 14 ++++++++++++++
 include/drm/bridge/dw_mipi_dsi.h              |  3 +++
 2 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index 8cd89a63b5f2..c754d55f71d1 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -571,9 +571,23 @@ static int dw_mipi_dsi_bridge_atomic_check(struct drm_bridge *bridge,
 					   struct drm_crtc_state *crtc_state,
 					   struct drm_connector_state *conn_state)
 {
+	struct dw_mipi_dsi *dsi = bridge_to_dsi(bridge);
+	const struct dw_mipi_dsi_plat_data *pdata = dsi->plat_data;
+	bool ret;
+
 	bridge_state->input_bus_cfg.flags =
 		DRM_BUS_FLAG_DE_HIGH | DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE;
 
+	if (pdata->mode_fixup) {
+		ret = pdata->mode_fixup(pdata->priv_data, &crtc_state->mode,
+					&crtc_state->adjusted_mode);
+		if (!ret) {
+			DRM_DEBUG_DRIVER("failed to fixup mode " DRM_MODE_FMT "\n",
+					 DRM_MODE_ARG(&crtc_state->mode));
+			return -EINVAL;
+		}
+	}
+
 	return 0;
 }
 
diff --git a/include/drm/bridge/dw_mipi_dsi.h b/include/drm/bridge/dw_mipi_dsi.h
index 246650f2814f..65d5e68065e3 100644
--- a/include/drm/bridge/dw_mipi_dsi.h
+++ b/include/drm/bridge/dw_mipi_dsi.h
@@ -59,6 +59,9 @@ struct dw_mipi_dsi_plat_data {
 					   unsigned long mode_flags,
 					   u32 lanes, u32 format);
 
+	bool (*mode_fixup)(void *priv_data, const struct drm_display_mode *mode,
+			   struct drm_display_mode *adjusted_mode);
+
 	u32 *(*get_input_bus_fmts)(void *priv_data,
 				   struct drm_bridge *bridge,
 				   struct drm_bridge_state *bridge_state,
-- 
2.37.1

[PATCH 5/9] drm/bridge: synopsys: dw-mipi-dsi: Use pixel clock rate to calculate lbcc

[Liu Ying]

To get better accuration, use pixel clock rate to calculate lbcc instead of
lane_mbps since the pixel clock rate is in KHz while lane_mbps is in MHz.
Without this, distorted image can be seen on a HDMI monitor connected with
i.MX93 11x11 EVK through ADV7535 DSI to HDMI bridge in 1920x1080p@60 video
mode.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index c754d55f71d1..332388fd86da 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -12,6 +12,7 @@
 #include <linux/component.h>
 #include <linux/debugfs.h>
 #include <linux/iopoll.h>
+#include <linux/math64.h>
 #include <linux/media-bus-format.h>
 #include <linux/module.h>
 #include <linux/of_device.h>
@@ -762,8 +763,15 @@ static u32 dw_mipi_dsi_get_hcomponent_lbcc(struct dw_mipi_dsi *dsi,
 					   u32 hcomponent)
 {
 	u32 frac, lbcc;
+	int bpp;
 
-	lbcc = hcomponent * dsi->lane_mbps * MSEC_PER_SEC / 8;
+	bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
+	if (bpp < 0) {
+		dev_err(dsi->dev, "failed to get bpp\n");
+		return 0;
+	}
+
+	lbcc = div_u64((u64)hcomponent * mode->clock * bpp, dsi->lanes * 8);
 
 	frac = lbcc % mode->clock;
 	lbcc = lbcc / mode->clock;
-- 
2.37.1

[PATCH 6/9] drm/bridge: synopsys: dw-mipi-dsi: Set minimum lane byte clock cycles for HSA and HBP

[Liu Ying]

According to Synopsys support channel, each region of HSA, HBP and HFP must
have minimum number of 10 bytes where constant 4 bytes are for HSS or HSE
and 6 bytes are for blanking packet(header + CRC).  Hence, the below table
comes in.

+------------+----------+-------+
| data lanes | min lbcc | bytes |
+------------+----------+-------+
|     1      |    10    |  1*10 |
+------------+----------+-------+
|     2      |    5     |  2*5  |
+------------+----------+-------+
|     3      |    4     |  3*4  |
+------------+----------+-------+
|     4      |    3     |  4*3  |
+------------+----------+-------+

Implement the minimum lbcc numbers to make sure that the values programmed
into DSI_VID_HSA_TIME and DSI_VID_HBP_TIME registers meet the minimum
number requirement.  For DSI_VID_HLINE_TIME register, it seems that the
value programmed should be based on mode->htotal as-is, instead of sum up
HSA, HBP, HFP and HDISPLAY.

This helps the case where Raydium RM67191 DSI panel is connected, since
it's video timing for hsync length is only 2 pixels and without this patch
the programmed value for DSI_VID_HSA_TIME is only 2 with 4 data lanes.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index 332388fd86da..536306ccea5a 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -757,12 +757,19 @@ static void dw_mipi_dsi_command_mode_config(struct dw_mipi_dsi *dsi)
 	dsi_write(dsi, DSI_MODE_CFG, ENABLE_CMD_MODE);
 }
 
+static const u32 minimum_lbccs[] = {10, 5, 4, 3};
+
+static inline u32 dw_mipi_dsi_get_minimum_lbcc(struct dw_mipi_dsi *dsi)
+{
+	return minimum_lbccs[dsi->lanes - 1];
+}
+
 /* Get lane byte clock cycles. */
 static u32 dw_mipi_dsi_get_hcomponent_lbcc(struct dw_mipi_dsi *dsi,
 					   const struct drm_display_mode *mode,
 					   u32 hcomponent)
 {
-	u32 frac, lbcc;
+	u32 frac, lbcc, minimum_lbcc;
 	int bpp;
 
 	bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
@@ -778,6 +785,11 @@ static u32 dw_mipi_dsi_get_hcomponent_lbcc(struct dw_mipi_dsi *dsi,
 	if (frac)
 		lbcc++;
 
+	minimum_lbcc = dw_mipi_dsi_get_minimum_lbcc(dsi);
+
+	if (lbcc < minimum_lbcc)
+		lbcc = minimum_lbcc;
+
 	return lbcc;
 }
 
-- 
2.37.1

[PATCH 7/9] drm/bridge: synopsys: dw-mipi-dsi: Disable HSTX and LPRX timeout check

[Liu Ying]

According to Synopsys DW MIPI DSI host databook, HSTX and LPRX timeout
contention detections are measured in TO_CLK_DIVISION cycles.  However,
the current driver programs magic values to TO_CLK_DIVISION, HSTX_TO_CNT
and LPRX_TO_CNT register fields, which makes timeout error event wrongly
happen for some video modes, at least for the typical 1920x1080p@60 video
mode read from a HDMI monitor driven by ADV7535 DSI to HDMI bridge.
While at it, the current driver doesn't enable interrupt to handle or
complain about the error status, so true error just happens silently
except for display distortions by visual check.

Disable the timeout check by setting those timeout register fields to
zero for now until someone comes along with better computations for the
timeout values.  Although the databook doesn't mention what happens when
they are set to zero, it turns out the false error doesn't happen for
the 1920x1080p@60 video mode at least.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index 536306ccea5a..0fcadf99e783 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -684,7 +684,7 @@ static void dw_mipi_dsi_init(struct dw_mipi_dsi *dsi)
 	 * timeout clock division should be computed with the
 	 * high speed transmission counter timeout and byte lane...
 	 */
-	dsi_write(dsi, DSI_CLKMGR_CFG, TO_CLK_DIVISION(10) |
+	dsi_write(dsi, DSI_CLKMGR_CFG, TO_CLK_DIVISION(0) |
 		  TX_ESC_CLK_DIVISION(esc_clk_division));
 }
 
@@ -747,7 +747,7 @@ static void dw_mipi_dsi_command_mode_config(struct dw_mipi_dsi *dsi)
 	 * compute high speed transmission counter timeout according
 	 * to the timeout clock division (TO_CLK_DIVISION) and byte lane...
 	 */
-	dsi_write(dsi, DSI_TO_CNT_CFG, HSTX_TO_CNT(1000) | LPRX_TO_CNT(1000));
+	dsi_write(dsi, DSI_TO_CNT_CFG, HSTX_TO_CNT(0) | LPRX_TO_CNT(0));
 	/*
 	 * TODO dw drv improvements
 	 * the Bus-Turn-Around Timeout Counter should be computed
-- 
2.37.1

[PATCH 8/9] dt-bindings: display: bridge: Document Freescale i.MX93 MIPI DSI

[Liu Ying]

Freescale i.MX93 SoC embeds a Synopsys Designware MIPI DSI host
controller and a Synopsys Designware MIPI DPHY.  Some configurations
and extensions to them are controlled by i.MX93 media blk-ctrl.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 .../display/bridge/fsl,imx93-mipi-dsi.yaml    | 115 ++++++++++++++++++
 1 file changed, 115 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/fsl,imx93-mipi-dsi.yaml

diff --git a/Documentation/devicetree/bindings/display/bridge/fsl,imx93-mipi-dsi.yaml b/Documentation/devicetree/bindings/display/bridge/fsl,imx93-mipi-dsi.yaml
new file mode 100644
index 000000000000..d6e51d0cf546
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/bridge/fsl,imx93-mipi-dsi.yaml
@@ -0,0 +1,115 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/fsl,imx93-mipi-dsi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Freescale i.MX93 specific extensions to Synopsys Designware MIPI DSI
+
+maintainers:
+  - Liu Ying <victor.liu@nxp.com>
+
+description: |
+  There is a Synopsys Designware MIPI DSI Host Controller and a Synopsys
+  Designware MIPI DPHY embedded in Freescale i.MX93 SoC.  Some configurations
+  and extensions to them are controlled by i.MX93 media blk-ctrl.
+
+allOf:
+  - $ref: snps,dw-mipi-dsi.yaml#
+
+properties:
+  compatible:
+    const: fsl,imx93-mipi-dsi
+
+  clocks:
+    items:
+      - description: apb clock
+      - description: pixel clock
+      - description: PHY configuration clock
+      - description: PHY reference clock
+
+  clock-names:
+    items:
+      - const: pclk
+      - const: pix
+      - const: phy_cfg
+      - const: phy_ref
+
+  interrupts:
+    maxItems: 1
+
+  fsl,media-blk-ctrl:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description:
+      i.MX93 media blk-ctrl, as a syscon, controls pixel component bit map
+      configurations from LCDIF display controller to the MIPI DSI host
+      controller and MIPI DPHY PLL related configurations through PLL SoC
+      interface.
+
+  power-domains:
+    maxItems: 1
+
+required:
+  - compatible
+  - interrupts
+  - fsl,media-blk-ctrl
+  - power-domains
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/imx93-clock.h>
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/power/fsl,imx93-power.h>
+
+    dsi@4ae10000 {
+        compatible = "fsl,imx93-mipi-dsi";
+        reg = <0x4ae10000 0x10000>;
+        interrupts = <GIC_SPI 177 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&clk IMX93_CLK_MIPI_DSI_GATE>,
+                 <&clk IMX93_CLK_MEDIA_DISP_PIX>,
+                 <&clk IMX93_CLK_MIPI_PHY_CFG>,
+                 <&clk IMX93_CLK_24M>;
+        clock-names = "pclk", "pix", "phy_cfg", "phy_ref";
+        fsl,media-blk-ctrl = <&media_blk_ctrl>;
+        power-domains = <&media_blk_ctrl IMX93_MEDIABLK_PD_MIPI_DSI>;
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        panel@0 {
+            compatible = "raydium,rm67191";
+            reg = <0>;
+            reset-gpios = <&adp5585gpio 6 GPIO_ACTIVE_LOW>;
+            dsi-lanes = <4>;
+            video-mode = <2>;
+
+            port {
+                panel_in: endpoint {
+                    remote-endpoint = <&dsi_out>;
+                };
+            };
+        };
+
+        ports {
+            #address-cells = <1>;
+            #size-cells = <0>;
+
+            port@0 {
+                reg = <0>;
+
+                dsi_to_lcdif: endpoint {
+                    remote-endpoint = <&lcdif_to_dsi>;
+                };
+            };
+
+            port@1 {
+                reg = <1>;
+
+                dsi_out: endpoint {
+                    remote-endpoint = <&panel_in>;
+                };
+            };
+        };
+    };
-- 
2.37.1

[PATCH 9/9] drm/bridge: imx: Add i.MX93 MIPI DSI support

[Liu Ying]

Freescale i.MX93 SoC embeds a Synopsys Designware MIPI DSI host
controller and a Synopsys Designware MIPI DPHY.  Some configurations
and extensions to them are controlled by i.MX93 media blk-ctrl.

Add a DRM bridge for i.MX93 MIPI DSI by using existing DW MIPI DSI
bridge helpers and implementing i.MX93 MIPI DSI specific extensions.

Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
 drivers/gpu/drm/bridge/imx/Kconfig          |  10 +
 drivers/gpu/drm/bridge/imx/Makefile         |   1 +
 drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c | 934 ++++++++++++++++++++
 3 files changed, 945 insertions(+)
 create mode 100644 drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c

diff --git a/drivers/gpu/drm/bridge/imx/Kconfig b/drivers/gpu/drm/bridge/imx/Kconfig
index 9fae28db6aa7..5182298c7182 100644
--- a/drivers/gpu/drm/bridge/imx/Kconfig
+++ b/drivers/gpu/drm/bridge/imx/Kconfig
@@ -49,4 +49,14 @@ config DRM_IMX8QXP_PIXEL_LINK_TO_DPI
 	  Choose this to enable pixel link to display pixel interface(PXL2DPI)
 	  found in Freescale i.MX8qxp processor.
 
+config DRM_IMX93_MIPI_DSI
+	tristate "Freescale i.MX93 specific extensions for Synopsys DW MIPI DSI"
+	depends on OF
+	depends on COMMON_CLK
+	select DRM_DW_MIPI_DSI
+	select GENERIC_PHY_MIPI_DPHY
+	help
+	  Choose this to enable MIPI DSI controller found in Freescale i.MX93
+	  processor.
+
 endif # ARCH_MXC || COMPILE_TEST
diff --git a/drivers/gpu/drm/bridge/imx/Makefile b/drivers/gpu/drm/bridge/imx/Makefile
index 8e2ebf3399a1..2b0c2e44aa1b 100644
--- a/drivers/gpu/drm/bridge/imx/Makefile
+++ b/drivers/gpu/drm/bridge/imx/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_DRM_IMX8QXP_LDB) += imx8qxp-ldb.o
 obj-$(CONFIG_DRM_IMX8QXP_PIXEL_COMBINER) += imx8qxp-pixel-combiner.o
 obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK) += imx8qxp-pixel-link.o
 obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK_TO_DPI) += imx8qxp-pxl2dpi.o
+obj-$(CONFIG_DRM_IMX93_MIPI_DSI) += imx93-mipi-dsi.o
diff --git a/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c b/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c
new file mode 100644
index 000000000000..77f59e3407a0
--- /dev/null
+++ b/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c
@@ -0,0 +1,934 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+/*
+ * Copyright 2022,2023 NXP
+ */
+
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/math.h>
+#include <linux/media-bus-format.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/phy/phy.h>
+#include <linux/phy/phy-mipi-dphy.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#include <drm/bridge/dw_mipi_dsi.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_print.h>
+
+/* DPHY PLL configuration registers */
+#define DSI_REG				0x4c
+#define  CFGCLKFREQRANGE_MASK		GENMASK(5, 0)
+#define  CFGCLKFREQRANGE(x)		FIELD_PREP(CFGCLKFREQRANGE_MASK, (x))
+#define  CLKSEL_MASK			GENMASK(7, 6)
+#define  CLKSEL_STOP			FIELD_PREP(CLKSEL_MASK, 0)
+#define  CLKSEL_GEN			FIELD_PREP(CLKSEL_MASK, 1)
+#define  CLKSEL_EXT			FIELD_PREP(CLKSEL_MASK, 2)
+#define  HSFREQRANGE_MASK		GENMASK(14, 8)
+#define  HSFREQRANGE(x)			FIELD_PREP(HSFREQRANGE_MASK, (x))
+#define  UPDATE_PLL			BIT(17)
+#define  SHADOW_CLR			BIT(18)
+#define  CLK_EXT			BIT(19)
+
+#define DSI_WRITE_REG0			0x50
+#define  M_MASK				GENMASK(9, 0)
+#define  M(x)				FIELD_PREP(M_MASK, ((x) - 2))
+#define  N_MASK				GENMASK(13, 10)
+#define  N(x)				FIELD_PREP(N_MASK, ((x) - 1))
+#define  VCO_CTRL_MASK			GENMASK(19, 14)
+#define  VCO_CTRL(x)			FIELD_PREP(VCO_CTRL_MASK, (x))
+#define  PROP_CTRL_MASK			GENMASK(25, 20)
+#define  PROP_CTRL(x)			FIELD_PREP(PROP_CTRL_MASK, (x))
+#define  INT_CTRL_MASK			GENMASK(31, 26)
+#define  INT_CTRL(x)			FIELD_PREP(INT_CTRL_MASK, (x))
+
+#define DSI_WRITE_REG1			0x54
+#define  GMP_CTRL_MASK			GENMASK(1, 0)
+#define  GMP_CTRL(x)			FIELD_PREP(GMP_CTRL_MASK, (x))
+#define  CPBIAS_CTRL_MASK		GENMASK(8, 2)
+#define  CPBIAS_CTRL(x)			FIELD_PREP(CPBIAS_CTRL_MASK, (x))
+#define  PLL_SHADOW_CTRL		BIT(9)
+
+/* display mux control register */
+#define DISPLAY_MUX			0x60
+#define  MIPI_DSI_RGB666_MAP_CFG	GENMASK(7, 6)
+#define  RGB666_CONFIG1			FIELD_PREP(MIPI_DSI_RGB666_MAP_CFG, 0)
+#define  RGB666_CONFIG2			FIELD_PREP(MIPI_DSI_RGB666_MAP_CFG, 1)
+#define  MIPI_DSI_RGB565_MAP_CFG	GENMASK(5, 4)
+#define  RGB565_CONFIG1			FIELD_PREP(MIPI_DSI_RGB565_MAP_CFG, 0)
+#define  RGB565_CONFIG2			FIELD_PREP(MIPI_DSI_RGB565_MAP_CFG, 1)
+#define  RGB565_CONFIG3			FIELD_PREP(MIPI_DSI_RGB565_MAP_CFG, 2)
+#define  LCDIF_CROSS_LINE_PATTERN	GENMASK(3, 0)
+#define  RGB888_TO_RGB888		FIELD_PREP(LCDIF_CROSS_LINE_PATTERN, 0)
+#define  RGB888_TO_RGB666		FIELD_PREP(LCDIF_CROSS_LINE_PATTERN, 6)
+#define  RGB565_TO_RGB565		FIELD_PREP(LCDIF_CROSS_LINE_PATTERN, 7)
+
+#define MHZ(x)				((x) * 1000000UL)
+
+#define REF_CLK_RATE_MAX		MHZ(64)
+#define REF_CLK_RATE_MIN		MHZ(2)
+#define FOUT_MAX			MHZ(1250)
+#define FOUT_MIN			MHZ(40)
+#define FVCO_DIV_FACTOR			MHZ(80)
+
+#define MBPS(x)				((x) * 1000000UL)
+
+#define DATA_RATE_MAX_SPEED		MBPS(2500)
+#define DATA_RATE_MIN_SPEED		MBPS(80)
+
+#define M_MAX				625UL
+#define M_MIN				64UL
+
+#define N_MAX				16U
+#define N_MIN				1U
+
+struct imx93_dsi {
+	struct device *dev;
+	struct regmap *regmap;
+	struct clk *clk_pixel;
+	struct clk *clk_ref;
+	struct clk *clk_cfg;
+	struct dw_mipi_dsi *dmd;
+	struct dw_mipi_dsi_plat_data pdata;
+	union phy_configure_opts phy_cfg;
+	unsigned long ref_clk_rate;
+	u32 format;
+};
+
+struct dphy_pll_cfg {
+	u32 m;	/* PLL Feedback Multiplication Ratio */
+	u32 n;	/* PLL Input Frequency Division Ratio */
+};
+
+struct dphy_pll_vco_prop {
+	unsigned long max_fout;
+	u8 vco_cntl;
+	u8 prop_cntl;
+};
+
+struct dphy_pll_hsfreqrange {
+	unsigned long max_mbps;
+	u8 hsfreqrange;
+};
+
+/* DPHY Databook Table 3-13 Charge-pump Programmability */
+static const struct dphy_pll_vco_prop vco_prop_map[] = {
+	{   55, 0x3f, 0x0d },
+	{   82, 0x37, 0x0d },
+	{  110, 0x2f, 0x0d },
+	{  165, 0x27, 0x0d },
+	{  220, 0x1f, 0x0d },
+	{  330, 0x17, 0x0d },
+	{  440, 0x0f, 0x0d },
+	{  660, 0x07, 0x0d },
+	{ 1149, 0x03, 0x0d },
+	{ 1152, 0x01, 0x0d },
+	{ 1250, 0x01, 0x0e },
+};
+
+/* DPHY Databook Table 5-7 Frequency Ranges and Defaults */
+static const struct dphy_pll_hsfreqrange hsfreqrange_map[] = {
+	{   89, 0x00 },
+	{   99, 0x10 },
+	{  109, 0x20 },
+	{  119, 0x30 },
+	{  129, 0x01 },
+	{  139, 0x11 },
+	{  149, 0x21 },
+	{  159, 0x31 },
+	{  169, 0x02 },
+	{  179, 0x12 },
+	{  189, 0x22 },
+	{  204, 0x32 },
+	{  219, 0x03 },
+	{  234, 0x13 },
+	{  249, 0x23 },
+	{  274, 0x33 },
+	{  299, 0x04 },
+	{  324, 0x14 },
+	{  349, 0x25 },
+	{  399, 0x35 },
+	{  449, 0x05 },
+	{  499, 0x16 },
+	{  549, 0x26 },
+	{  599, 0x37 },
+	{  649, 0x07 },
+	{  699, 0x18 },
+	{  749, 0x28 },
+	{  799, 0x39 },
+	{  849, 0x09 },
+	{  899, 0x19 },
+	{  949, 0x29 },
+	{  999, 0x3a },
+	{ 1049, 0x0a },
+	{ 1099, 0x1a },
+	{ 1149, 0x2a },
+	{ 1199, 0x3b },
+	{ 1249, 0x0b },
+	{ 1299, 0x1b },
+	{ 1349, 0x2b },
+	{ 1399, 0x3c },
+	{ 1449, 0x0c },
+	{ 1499, 0x1c },
+	{ 1549, 0x2c },
+	{ 1599, 0x3d },
+	{ 1649, 0x0d },
+	{ 1699, 0x1d },
+	{ 1749, 0x2e },
+	{ 1799, 0x3e },
+	{ 1849, 0x0e },
+	{ 1899, 0x1e },
+	{ 1949, 0x2f },
+	{ 1999, 0x3f },
+	{ 2049, 0x0f },
+	{ 2099, 0x40 },
+	{ 2149, 0x41 },
+	{ 2199, 0x42 },
+	{ 2249, 0x43 },
+	{ 2299, 0x44 },
+	{ 2349, 0x45 },
+	{ 2399, 0x46 },
+	{ 2449, 0x47 },
+	{ 2499, 0x48 },
+	{ 2500, 0x49 },
+};
+
+static void dphy_pll_write(struct imx93_dsi *dsi, unsigned int reg, u32 value)
+{
+	int ret;
+
+	ret = regmap_write(dsi->regmap, reg, value);
+	if (ret < 0)
+		DRM_DEV_ERROR(dsi->dev,
+			      "failed to write 0x%08x to pll reg 0x%x: %d\n",
+			      value, reg, ret);
+}
+
+static inline unsigned long data_rate_to_fout(unsigned long data_rate)
+{
+	/* Fout is half of data rate */
+	return data_rate / 2;
+}
+
+static int
+dphy_pll_get_configure_from_opts(struct imx93_dsi *dsi,
+				 struct phy_configure_opts_mipi_dphy *dphy_opts,
+				 struct dphy_pll_cfg *cfg)
+{
+	struct device *dev = dsi->dev;
+	unsigned long fin = dsi->ref_clk_rate;
+	unsigned long fout;
+	unsigned long best_fout = 0;
+	unsigned int fvco_div;
+	unsigned int min_n, max_n, n, best_n;
+	unsigned long m, best_m;
+	unsigned long min_delta = ULONG_MAX;
+	unsigned long tmp, delta;
+
+	if (dphy_opts->hs_clk_rate < DATA_RATE_MIN_SPEED ||
+	    dphy_opts->hs_clk_rate > DATA_RATE_MAX_SPEED) {
+		DRM_DEV_DEBUG_DRIVER(dev, "invalid data rate per lane: %lu\n",
+				     dphy_opts->hs_clk_rate);
+		return -EINVAL;
+	}
+
+	fout = data_rate_to_fout(dphy_opts->hs_clk_rate);
+
+	/* DPHY Databook 3.3.6.1 Output Frequency */
+	/* Fout = Fvco / Fvco_div = (Fin * M) / (Fvco_div * N) */
+	/* Fvco_div could be 1/2/4/8 according to Fout range. */
+	fvco_div = 8UL / min(DIV_ROUND_UP(fout, FVCO_DIV_FACTOR), 8UL);
+
+	/* limitation: 2MHz <= Fin / N <= 8MHz */
+	min_n = DIV_ROUND_UP_ULL((u64)fin, MHZ(8));
+	max_n = DIV_ROUND_DOWN_ULL((u64)fin, MHZ(2));
+
+	/* clamp possible N(s) */
+	min_n = clamp(min_n, N_MIN, N_MAX);
+	max_n = clamp(max_n, N_MIN, N_MAX);
+
+	DRM_DEV_DEBUG_DRIVER(dev, "Fout = %lu, Fvco_div = %u, n_range = [%u, %u]\n",
+			     fout, fvco_div, min_n, max_n);
+
+	for (n = min_n; n <= max_n; n++) {
+		/* M = (Fout * N * Fvco_div) / Fin */
+		tmp = fout * n * fvco_div;
+		m = DIV_ROUND_CLOSEST(tmp, fin);
+
+		/* check M range */
+		if (m < M_MIN || m > M_MAX)
+			continue;
+
+		/* calculate temporary Fout */
+		tmp = m * fin;
+		do_div(tmp, n * fvco_div);
+		if (tmp < FOUT_MIN || tmp > FOUT_MAX)
+			continue;
+
+		delta = abs(fout - tmp);
+		if (delta < min_delta) {
+			best_n = n;
+			best_m = m;
+			min_delta = delta;
+			best_fout = tmp;
+		}
+	}
+
+	if (best_fout) {
+		cfg->m = best_m;
+		cfg->n = best_n;
+		DRM_DEV_DEBUG_DRIVER(dev, "best Fout = %lu, m = %u, n = %u\n",
+				     best_fout, cfg->m, cfg->n);
+	} else {
+		DRM_DEV_DEBUG_DRIVER(dev, "failed to find best Fout\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static void dphy_pll_clear_shadow(struct imx93_dsi *dsi)
+{
+	/* Reference DPHY Databook Figure 3-3 Initialization Timing Diagram. */
+	/* Select clock generation first. */
+	dphy_pll_write(dsi, DSI_REG, CLKSEL_GEN);
+
+	/* Clear shadow after clock selection is done a while. */
+	fsleep(1);
+	dphy_pll_write(dsi, DSI_REG, CLKSEL_GEN | SHADOW_CLR);
+
+	/* A minimum pulse of 5ns on shadow_clear signal. */
+	fsleep(1);
+	dphy_pll_write(dsi, DSI_REG, CLKSEL_GEN);
+}
+
+static unsigned long dphy_pll_get_cfgclkrange(struct imx93_dsi *dsi)
+{
+	/*
+	 * DPHY Databook Table 4-4 System Control Signals mentions an equation
+	 * for cfgclkfreqrange[5:0].
+	 */
+	return (clk_get_rate(dsi->clk_cfg) / MHZ(1) - 17) * 4;
+}
+
+static u8
+dphy_pll_get_hsfreqrange(struct phy_configure_opts_mipi_dphy *dphy_opts)
+{
+	unsigned long mbps = dphy_opts->hs_clk_rate / MHZ(1);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(hsfreqrange_map); i++)
+		if (mbps <= hsfreqrange_map[i].max_mbps)
+			return hsfreqrange_map[i].hsfreqrange;
+
+	return 0;
+}
+
+static u8 dphy_pll_get_vco(struct phy_configure_opts_mipi_dphy *dphy_opts)
+{
+	unsigned long fout = data_rate_to_fout(dphy_opts->hs_clk_rate) / MHZ(1);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(vco_prop_map); i++)
+		if (fout <= vco_prop_map[i].max_fout)
+			return vco_prop_map[i].vco_cntl;
+
+	return 0;
+}
+
+static u8 dphy_pll_get_prop(struct phy_configure_opts_mipi_dphy *dphy_opts)
+{
+	unsigned long fout = data_rate_to_fout(dphy_opts->hs_clk_rate) / MHZ(1);
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(vco_prop_map); i++)
+		if (fout <= vco_prop_map[i].max_fout)
+			return vco_prop_map[i].prop_cntl;
+
+	return 0;
+}
+
+static int dphy_pll_update(struct imx93_dsi *dsi)
+{
+	int ret;
+
+	ret = regmap_update_bits(dsi->regmap, DSI_REG, UPDATE_PLL, UPDATE_PLL);
+	if (ret < 0) {
+		DRM_DEV_ERROR(dsi->dev, "failed to set UPDATE_PLL: %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * The updatepll signal should be asserted for a minimum of four clkin
+	 * cycles, according to DPHY Databook Figure 3-3 Initialization Timing
+	 * Diagram.
+	 */
+	fsleep(10);
+
+	ret = regmap_update_bits(dsi->regmap, DSI_REG, UPDATE_PLL, 0);
+	if (ret < 0) {
+		DRM_DEV_ERROR(dsi->dev, "failed to clear UPDATE_PLL: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int dphy_pll_configure(struct imx93_dsi *dsi, union phy_configure_opts *opts)
+{
+	struct dphy_pll_cfg cfg = { 0 };
+	u32 val;
+	int ret;
+
+	ret = dphy_pll_get_configure_from_opts(dsi, &opts->mipi_dphy, &cfg);
+	if (ret) {
+		DRM_DEV_ERROR(dsi->dev, "failed to get phy pll cfg %d\n", ret);
+		return ret;
+	}
+
+	dphy_pll_clear_shadow(dsi);
+
+	/* DSI_REG */
+	val = CLKSEL_GEN |
+	      CFGCLKFREQRANGE(dphy_pll_get_cfgclkrange(dsi)) |
+	      HSFREQRANGE(dphy_pll_get_hsfreqrange(&opts->mipi_dphy));
+	dphy_pll_write(dsi, DSI_REG, val);
+
+	/* DSI_WRITE_REG0 */
+	val = M(cfg.m) | N(cfg.n) | INT_CTRL(0) |
+	      VCO_CTRL(dphy_pll_get_vco(&opts->mipi_dphy)) |
+	      PROP_CTRL(dphy_pll_get_prop(&opts->mipi_dphy));
+	dphy_pll_write(dsi, DSI_WRITE_REG0, val);
+
+	/* DSI_WRITE_REG1 */
+	dphy_pll_write(dsi, DSI_WRITE_REG1, GMP_CTRL(1) | CPBIAS_CTRL(0x10));
+
+	ret = clk_prepare_enable(dsi->clk_ref);
+	if (ret < 0) {
+		DRM_DEV_ERROR(dsi->dev, "failed to enable ref clock: %d\n", ret);
+		return ret;
+	}
+
+	/*
+	 * At least 10 refclk cycles are required before updatePLL assertion,
+	 * according to DPHY Databook Figure 3-3 Initialization Timing Diagram.
+	 */
+	fsleep(10);
+
+	ret = dphy_pll_update(dsi);
+	if (ret < 0) {
+		clk_disable_unprepare(dsi->clk_ref);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void dphy_pll_clear_reg(struct imx93_dsi *dsi)
+{
+	dphy_pll_write(dsi, DSI_REG, 0);
+	dphy_pll_write(dsi, DSI_WRITE_REG0, 0);
+	dphy_pll_write(dsi, DSI_WRITE_REG1, 0);
+}
+
+static int dphy_pll_init(struct imx93_dsi *dsi)
+{
+	int ret;
+
+	ret = clk_prepare_enable(dsi->clk_cfg);
+	if (ret < 0) {
+		DRM_DEV_ERROR(dsi->dev, "failed to enable config clock: %d\n", ret);
+		return ret;
+	}
+
+	dphy_pll_clear_reg(dsi);
+
+	return 0;
+}
+
+static void dphy_pll_uninit(struct imx93_dsi *dsi)
+{
+	dphy_pll_clear_reg(dsi);
+	clk_disable_unprepare(dsi->clk_cfg);
+}
+
+static void dphy_pll_power_off(struct imx93_dsi *dsi)
+{
+	dphy_pll_clear_reg(dsi);
+	clk_disable_unprepare(dsi->clk_ref);
+}
+
+static int imx93_dsi_get_phy_configure_opts(struct imx93_dsi *dsi,
+					    const struct drm_display_mode *mode,
+					    union phy_configure_opts *phy_cfg,
+					    u32 lanes, u32 format)
+{
+	struct device *dev = dsi->dev;
+	int bpp;
+	int ret;
+
+	bpp = mipi_dsi_pixel_format_to_bpp(format);
+	if (bpp < 0) {
+		DRM_DEV_DEBUG_DRIVER(dev, "failed to get bpp for pixel format %d\n",
+				     format);
+		return -EINVAL;
+	}
+
+	ret = phy_mipi_dphy_get_default_config(mode->clock * MSEC_PER_SEC, bpp,
+					       lanes, &phy_cfg->mipi_dphy);
+	if (ret < 0) {
+		DRM_DEV_DEBUG_DRIVER(dev, "failed to get default phy cfg %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static enum drm_mode_status
+imx93_dsi_validate_mode(struct imx93_dsi *dsi, const struct drm_display_mode *mode)
+{
+	struct drm_bridge *bridge = dw_mipi_dsi_get_bridge(dsi->dmd);
+
+	/* Get the last bridge */
+	while (drm_bridge_get_next_bridge(bridge))
+		bridge = drm_bridge_get_next_bridge(bridge);
+
+	if ((bridge->ops & DRM_BRIDGE_OP_DETECT) &&
+	    (bridge->ops & DRM_BRIDGE_OP_EDID)) {
+		unsigned long pixel_clock_rate = mode->clock * 1000;
+		unsigned long rounded_rate;
+
+		/* Allow +/-0.5% pixel clock rate deviation */
+		rounded_rate = clk_round_rate(dsi->clk_pixel, pixel_clock_rate);
+		if (rounded_rate < pixel_clock_rate * 995 / 1000 ||
+		    rounded_rate > pixel_clock_rate * 1005 / 1000) {
+			DRM_DEV_DEBUG_DRIVER(dsi->dev,
+					     "failed to round clock for mode " DRM_MODE_FMT "\n",
+					     DRM_MODE_ARG(mode));
+			return MODE_NOCLOCK;
+		}
+	}
+
+	return MODE_OK;
+}
+
+static enum drm_mode_status
+imx93_dsi_validate_phy(struct imx93_dsi *dsi, const struct drm_display_mode *mode,
+		       unsigned long mode_flags, u32 lanes, u32 format)
+{
+	union phy_configure_opts phy_cfg;
+	struct dphy_pll_cfg cfg = { 0 };
+	struct device *dev = dsi->dev;
+	int ret;
+
+	ret = imx93_dsi_get_phy_configure_opts(dsi, mode, &phy_cfg, lanes,
+					       format);
+	if (ret < 0) {
+		DRM_DEV_DEBUG_DRIVER(dev, "failed to get phy cfg opts %d\n", ret);
+		return MODE_ERROR;
+	}
+
+	ret = dphy_pll_get_configure_from_opts(dsi, &phy_cfg.mipi_dphy, &cfg);
+	if (ret < 0) {
+		DRM_DEV_DEBUG_DRIVER(dev, "failed to get phy pll cfg %d\n", ret);
+		return MODE_NOCLOCK;
+	}
+
+	return MODE_OK;
+}
+
+static enum drm_mode_status
+imx93_dsi_mode_valid(void *priv_data, const struct drm_display_mode *mode,
+		     unsigned long mode_flags, u32 lanes, u32 format)
+{
+	struct imx93_dsi *dsi = priv_data;
+	struct device *dev = dsi->dev;
+	enum drm_mode_status ret;
+
+	ret = imx93_dsi_validate_mode(dsi, mode);
+	if (ret != MODE_OK) {
+		DRM_DEV_DEBUG_DRIVER(dev, "failed to validate mode " DRM_MODE_FMT "\n",
+				     DRM_MODE_ARG(mode));
+		return ret;
+	}
+
+	ret = imx93_dsi_validate_phy(dsi, mode, mode_flags, lanes, format);
+	if (ret != MODE_OK) {
+		DRM_DEV_DEBUG_DRIVER(dev,
+				     "failed to validate phy for mode " DRM_MODE_FMT "\n",
+				     DRM_MODE_ARG(mode));
+		return ret;
+	}
+
+	return MODE_OK;
+}
+
+static bool imx93_dsi_mode_fixup(void *priv_data,
+				 const struct drm_display_mode *mode,
+				 struct drm_display_mode *adjusted_mode)
+{
+	struct imx93_dsi *dsi = priv_data;
+	unsigned long pixel_clock_rate;
+	unsigned long rounded_rate;
+
+	pixel_clock_rate = mode->clock * 1000;
+	rounded_rate = clk_round_rate(dsi->clk_pixel, pixel_clock_rate);
+
+	memcpy(adjusted_mode, mode, sizeof(*mode));
+	adjusted_mode->clock = rounded_rate / 1000;
+
+	DRM_DEV_DEBUG_DRIVER(dsi->dev, "adj clock %d for mode " DRM_MODE_FMT "\n",
+			     adjusted_mode->clock, DRM_MODE_ARG(mode));
+
+	return true;
+}
+
+static u32 *imx93_dsi_get_input_bus_fmts(void *priv_data,
+					 struct drm_bridge *bridge,
+					 struct drm_bridge_state *bridge_state,
+					 struct drm_crtc_state *crtc_state,
+					 struct drm_connector_state *conn_state,
+					 u32 output_fmt,
+					 unsigned int *num_input_fmts)
+{
+	u32 *input_fmts, input_fmt;
+
+	*num_input_fmts = 0;
+
+	switch (output_fmt) {
+	case MEDIA_BUS_FMT_RGB888_1X24:
+	case MEDIA_BUS_FMT_RGB666_1X18:
+	case MEDIA_BUS_FMT_FIXED:
+		input_fmt = MEDIA_BUS_FMT_RGB888_1X24;
+		break;
+	case MEDIA_BUS_FMT_RGB565_1X16:
+		input_fmt = output_fmt;
+		break;
+	default:
+		return NULL;
+	}
+
+	input_fmts = kmalloc(sizeof(*input_fmts), GFP_KERNEL);
+	if (!input_fmts)
+		return NULL;
+	input_fmts[0] = input_fmt;
+	*num_input_fmts = 1;
+
+	return input_fmts;
+}
+
+static int imx93_dsi_phy_init(void *priv_data)
+{
+	struct imx93_dsi *dsi = priv_data;
+	unsigned int fmt = 0;
+	int ret;
+
+	switch (dsi->format) {
+	case MIPI_DSI_FMT_RGB888:
+		fmt = RGB888_TO_RGB888;
+		break;
+	case MIPI_DSI_FMT_RGB666:
+		fmt = RGB888_TO_RGB666;
+		regmap_update_bits(dsi->regmap, DISPLAY_MUX,
+				   MIPI_DSI_RGB666_MAP_CFG, RGB666_CONFIG2);
+		break;
+	case MIPI_DSI_FMT_RGB666_PACKED:
+		fmt = RGB888_TO_RGB666;
+		regmap_update_bits(dsi->regmap, DISPLAY_MUX,
+				   MIPI_DSI_RGB666_MAP_CFG, RGB666_CONFIG1);
+		break;
+	case MIPI_DSI_FMT_RGB565:
+		fmt = RGB565_TO_RGB565;
+		regmap_update_bits(dsi->regmap, DISPLAY_MUX,
+				   MIPI_DSI_RGB565_MAP_CFG, RGB565_CONFIG1);
+		break;
+	}
+
+	regmap_update_bits(dsi->regmap, DISPLAY_MUX, LCDIF_CROSS_LINE_PATTERN, fmt);
+
+	ret = dphy_pll_init(dsi);
+	if (ret < 0) {
+		DRM_DEV_ERROR(dsi->dev, "failed to init phy: %d\n", ret);
+		return ret;
+	}
+
+	ret = dphy_pll_configure(dsi, &dsi->phy_cfg);
+	if (ret < 0) {
+		DRM_DEV_ERROR(dsi->dev, "failed to configure phy: %d\n", ret);
+		dphy_pll_uninit(dsi);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void imx93_dsi_phy_power_off(void *priv_data)
+{
+	struct imx93_dsi *dsi = priv_data;
+
+	dphy_pll_power_off(dsi);
+	dphy_pll_uninit(dsi);
+}
+
+static int
+imx93_dsi_get_lane_mbps(void *priv_data, const struct drm_display_mode *mode,
+			unsigned long mode_flags, u32 lanes, u32 format,
+			unsigned int *lane_mbps)
+{
+	struct imx93_dsi *dsi = priv_data;
+	union phy_configure_opts phy_cfg;
+	struct device *dev = dsi->dev;
+	int ret;
+
+	ret = imx93_dsi_get_phy_configure_opts(dsi, mode, &phy_cfg, lanes,
+					       format);
+	if (ret < 0) {
+		DRM_DEV_DEBUG_DRIVER(dev, "failed to get phy cfg opts %d\n", ret);
+		return ret;
+	}
+
+	*lane_mbps = DIV_ROUND_UP(phy_cfg.mipi_dphy.hs_clk_rate, USEC_PER_SEC);
+
+	memcpy(&dsi->phy_cfg, &phy_cfg, sizeof(phy_cfg));
+
+	DRM_DEV_DEBUG_DRIVER(dev, "get lane_mbps %u for mode " DRM_MODE_FMT "\n",
+			     *lane_mbps, DRM_MODE_ARG(mode));
+
+	return 0;
+}
+
+/* High-Speed Transition Times */
+struct hstt {
+	unsigned int maxfreq;
+	struct dw_mipi_dsi_dphy_timing timing;
+};
+
+#define HSTT(_maxfreq, _c_lp2hs, _c_hs2lp, _d_lp2hs, _d_hs2lp)	\
+{								\
+	.maxfreq = (_maxfreq),					\
+	.timing = {						\
+		.clk_lp2hs = (_c_lp2hs),			\
+		.clk_hs2lp = (_c_hs2lp),			\
+		.data_lp2hs = (_d_lp2hs),			\
+		.data_hs2lp = (_d_hs2lp),			\
+	}							\
+}
+
+/* DPHY Databook Table A-4 High-Speed Transition Times */
+static const struct hstt hstt_table[] = {
+	HSTT(80,    21,  17,  15, 10),
+	HSTT(90,    23,  17,  16, 10),
+	HSTT(100,   22,  17,  16, 10),
+	HSTT(110,   25,  18,  17, 11),
+	HSTT(120,   26,  20,  18, 11),
+	HSTT(130,   27,  19,  19, 11),
+	HSTT(140,   27,  19,  19, 11),
+	HSTT(150,   28,  20,  20, 12),
+	HSTT(160,   30,  21,  22, 13),
+	HSTT(170,   30,  21,  23, 13),
+	HSTT(180,   31,  21,  23, 13),
+	HSTT(190,   32,  22,  24, 13),
+	HSTT(205,   35,  22,  25, 13),
+	HSTT(220,   37,  26,  27, 15),
+	HSTT(235,   38,  28,  27, 16),
+	HSTT(250,   41,  29,  30, 17),
+	HSTT(275,   43,  29,  32, 18),
+	HSTT(300,   45,  32,  35, 19),
+	HSTT(325,   48,  33,  36, 18),
+	HSTT(350,   51,  35,  40, 20),
+	HSTT(400,   59,  37,  44, 21),
+	HSTT(450,   65,  40,  49, 23),
+	HSTT(500,   71,  41,  54, 24),
+	HSTT(550,   77,  44,  57, 26),
+	HSTT(600,   82,  46,  64, 27),
+	HSTT(650,   87,  48,  67, 28),
+	HSTT(700,   94,  52,  71, 29),
+	HSTT(750,   99,  52,  75, 31),
+	HSTT(800,  105,  55,  82, 32),
+	HSTT(850,  110,  58,  85, 32),
+	HSTT(900,  115,  58,  88, 35),
+	HSTT(950,  120,  62,  93, 36),
+	HSTT(1000, 128,  63,  99, 38),
+	HSTT(1050, 132,  65, 102, 38),
+	HSTT(1100, 138,  67, 106, 39),
+	HSTT(1150, 146,  69, 112, 42),
+	HSTT(1200, 151,  71, 117, 43),
+	HSTT(1250, 153,  74, 120, 45),
+	HSTT(1300, 160,  73, 124, 46),
+	HSTT(1350, 165,  76, 130, 47),
+	HSTT(1400, 172,  78, 134, 49),
+	HSTT(1450, 177,  80, 138, 49),
+	HSTT(1500, 183,  81, 143, 52),
+	HSTT(1550, 191,  84, 147, 52),
+	HSTT(1600, 194,  85, 152, 52),
+	HSTT(1650, 201,  86, 155, 53),
+	HSTT(1700, 208,  88, 161, 53),
+	HSTT(1750, 212,  89, 165, 53),
+	HSTT(1800, 220,  90, 171, 54),
+	HSTT(1850, 223,  92, 175, 54),
+	HSTT(1900, 231,  91, 180, 55),
+	HSTT(1950, 236,  95, 185, 56),
+	HSTT(2000, 243,  97, 190, 56),
+	HSTT(2050, 248,  99, 194, 58),
+	HSTT(2100, 252, 100, 199, 59),
+	HSTT(2150, 259, 102, 204, 61),
+	HSTT(2200, 266, 105, 210, 62),
+	HSTT(2250, 269, 109, 213, 63),
+	HSTT(2300, 272, 109, 217, 65),
+	HSTT(2350, 281, 112, 225, 66),
+	HSTT(2400, 283, 115, 226, 66),
+	HSTT(2450, 282, 115, 226, 67),
+	HSTT(2500, 281, 118, 227, 67),
+};
+
+static int imx93_dsi_phy_get_timing(void *priv_data, unsigned int lane_mbps,
+				    struct dw_mipi_dsi_dphy_timing *timing)
+{
+	struct imx93_dsi *dsi = priv_data;
+	struct device *dev = dsi->dev;
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(hstt_table); i++)
+		if (lane_mbps <= hstt_table[i].maxfreq)
+			break;
+
+	if (i == ARRAY_SIZE(hstt_table)) {
+		DRM_DEV_ERROR(dev, "failed to get phy timing for lane_mbps %u\n",
+			      lane_mbps);
+		return -EINVAL;
+	}
+
+	*timing = hstt_table[i].timing;
+
+	DRM_DEV_DEBUG_DRIVER(dev, "get phy timing for %u <= %u (lane_mbps)\n",
+			     lane_mbps, hstt_table[i].maxfreq);
+
+	return 0;
+}
+
+static const struct dw_mipi_dsi_phy_ops imx93_dsi_phy_ops = {
+	.init = imx93_dsi_phy_init,
+	.power_off = imx93_dsi_phy_power_off,
+	.get_lane_mbps = imx93_dsi_get_lane_mbps,
+	.get_timing = imx93_dsi_phy_get_timing,
+};
+
+static int imx93_dsi_host_attach(void *priv_data, struct mipi_dsi_device *device)
+{
+	struct imx93_dsi *dsi = priv_data;
+
+	dsi->format = device->format;
+
+	return 0;
+}
+
+static const struct dw_mipi_dsi_host_ops imx93_dsi_host_ops = {
+	.attach = imx93_dsi_host_attach,
+};
+
+static int imx93_dsi_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct imx93_dsi *dsi;
+	int ret;
+
+	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
+	if (!dsi)
+		return -ENOMEM;
+
+	dsi->regmap = syscon_regmap_lookup_by_phandle(np, "fsl,media-blk-ctrl");
+	if (IS_ERR(dsi->regmap)) {
+		ret = PTR_ERR(dsi->regmap);
+		DRM_DEV_ERROR(dev, "failed to get block ctrl regmap: %d\n", ret);
+		return ret;
+	}
+
+	dsi->clk_pixel = devm_clk_get(dev, "pix");
+	if (IS_ERR(dsi->clk_pixel)) {
+		ret = PTR_ERR(dsi->clk_pixel);
+		if (ret != -EPROBE_DEFER)
+			DRM_DEV_ERROR(dev, "failed to get pixel clock: %d\n", ret);
+		return ret;
+	}
+
+	dsi->clk_cfg = devm_clk_get(dev, "phy_cfg");
+	if (IS_ERR(dsi->clk_cfg)) {
+		ret = PTR_ERR(dsi->clk_cfg);
+		if (ret != -EPROBE_DEFER)
+			DRM_DEV_ERROR(dev, "failed to get phy cfg clock: %d\n", ret);
+		return ret;
+	}
+
+	dsi->clk_ref = devm_clk_get(dev, "phy_ref");
+	if (IS_ERR(dsi->clk_ref)) {
+		ret = PTR_ERR(dsi->clk_ref);
+		if (ret != -EPROBE_DEFER)
+			DRM_DEV_ERROR(dev, "failed to get phy ref clock: %d\n", ret);
+		return ret;
+	}
+
+	dsi->ref_clk_rate = clk_get_rate(dsi->clk_ref);
+	if (dsi->ref_clk_rate < REF_CLK_RATE_MIN ||
+	    dsi->ref_clk_rate > REF_CLK_RATE_MAX) {
+		DRM_DEV_ERROR(dev, "invalid phy ref clock rate %lu\n",
+			      dsi->ref_clk_rate);
+		return -EINVAL;
+	}
+	DRM_DEV_DEBUG_DRIVER(dev, "phy ref clock rate: %lu\n", dsi->ref_clk_rate);
+
+	dsi->dev = dev;
+	dsi->pdata.max_data_lanes = 4;
+	dsi->pdata.mode_valid = imx93_dsi_mode_valid;
+	dsi->pdata.mode_fixup = imx93_dsi_mode_fixup;
+	dsi->pdata.get_input_bus_fmts = imx93_dsi_get_input_bus_fmts;
+	dsi->pdata.phy_ops = &imx93_dsi_phy_ops;
+	dsi->pdata.host_ops = &imx93_dsi_host_ops;
+	dsi->pdata.priv_data = dsi;
+	platform_set_drvdata(pdev, dsi);
+
+	dsi->dmd = dw_mipi_dsi_probe(pdev, &dsi->pdata);
+	if (IS_ERR(dsi->dmd)) {
+		ret = PTR_ERR(dsi->dmd);
+		if (ret != -EPROBE_DEFER)
+			DRM_DEV_ERROR(dev, "failed to probe dw_mipi_dsi: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void imx93_dsi_remove(struct platform_device *pdev)
+{
+	struct imx93_dsi *dsi = platform_get_drvdata(pdev);
+
+	dw_mipi_dsi_remove(dsi->dmd);
+}
+
+static const struct of_device_id imx93_dsi_dt_ids[] = {
+	{ .compatible = "fsl,imx93-mipi-dsi", },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, imx93_dsi_dt_ids);
+
+static struct platform_driver imx93_dsi_driver = {
+	.probe	= imx93_dsi_probe,
+	.remove_new = imx93_dsi_remove,
+	.driver	= {
+		.of_match_table = imx93_dsi_dt_ids,
+		.name = "imx93_mipi_dsi",
+	},
+};
+module_platform_driver(imx93_dsi_driver);
+
+MODULE_DESCRIPTION("Freescale i.MX93 MIPI DSI driver");
+MODULE_AUTHOR("Liu Ying <victor.liu@nxp.com>");
+MODULE_LICENSE("GPL");
-- 
2.37.1

Re: [PATCH 9/9] drm/bridge: imx: Add i.MX93 MIPI DSI support

[Jagan Teki]

On Mon, Jul 17, 2023 at 11:44 AM Liu Ying <victor.liu@nxp.com> wrote:
>
> Freescale i.MX93 SoC embeds a Synopsys Designware MIPI DSI host
> controller and a Synopsys Designware MIPI DPHY.  Some configurations
> and extensions to them are controlled by i.MX93 media blk-ctrl.
>
> Add a DRM bridge for i.MX93 MIPI DSI by using existing DW MIPI DSI
> bridge helpers and implementing i.MX93 MIPI DSI specific extensions.

I think the better way would add compatibility to be part of existing
dw-mipi-dsi.c with specific driver data. This way it avoids all the
platform-related helpers(extensions) and makes the driver generic to
all SoCs which use DW DSI IP. It would be a straightforward change as
the imx93 drm pipeline already supports bridge topology.

Thanks,
Jagan.

RE: [PATCH 9/9] drm/bridge: imx: Add i.MX93 MIPI DSI support

[Ying Liu]

Hi Jagan,

On Monday, July 17, 2023 2:44 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> On Mon, Jul 17, 2023 at 11:44 AM Liu Ying <victor.liu@nxp.com> wrote:
> >
> > Freescale i.MX93 SoC embeds a Synopsys Designware MIPI DSI host
> > controller and a Synopsys Designware MIPI DPHY.  Some configurations
> > and extensions to them are controlled by i.MX93 media blk-ctrl.
> >
> > Add a DRM bridge for i.MX93 MIPI DSI by using existing DW MIPI DSI
> > bridge helpers and implementing i.MX93 MIPI DSI specific extensions.
> 
> I think the better way would add compatibility to be part of existing
> dw-mipi-dsi.c with specific driver data. This way it avoids all the
> platform-related helpers(extensions) and makes the driver generic to
> all SoCs which use DW DSI IP. It would be a straightforward change as
> the imx93 drm pipeline already supports bridge topology.

The platform-related stuff is handed over to dw-mipi-dsi.c via struct
dw_mipi_dsi_plat_data as an argument of dw_mipi_dsi_probe().  It looks
ok for vendor drivers to call dw_mipi_dsi_probe() to set the platform-related
information(rockchip, meson and stm do that), like pdata.phy_ops and
pdata.host_ops.

dw-mipi-dsi.c is generic w/wo this patch series.

Can you elaborate more about adding compatibility to be part of existing
dw-mipi-dsi.c with specific driver data?  I don't see clear approach to do
that.

Regards,
Liu Ying

> 
> Thanks,
> Jagan.

Re: [PATCH 0/9] drm/bridge: imx: Add i.MX93 MIPI DSI support

[Alexander Stein]

Hi,

Am Montag, 17. Juli 2023, 08:18:22 CEST schrieb Liu Ying:
> Hi,
> 
> This series aims to add MIPI DSI support for Freescale i.MX93 SoC.
> 
> There is a Synopsys DesignWare MIPI DSI host controller and a Synopsys
> Designware MIPI DPHY embedded in i.MX93.  Some configurations and
> extensions to them are controlled by i.MX93 media blk-ctrl.
> 
> Add a DRM bridge for i.MX93 MIPI DSI by using existing DW MIPI DSI
> bridge helpers and implementing i.MX93 MIPI DSI specific extensions.
> 
> Note that since this series touches the dw-mipi-dsi driver, tests are
> needed to be done for meson, rockchip and stm.
> 
> Patch 1 ~ 7 do preparation work for adding i.MX93 MIPI DSI DRM bridge
> driver.
> 
> Patch 8 adds DT-binding documentation for i.MX93 MIPI DSI.
> 
> Patch 9 adds i.MX93 MIPI DSI DRM bridge.
> 
> Liu Ying (9):
>   drm/bridge: synopsys: dw-mipi-dsi: Add dw_mipi_dsi_get_bridge() helper
>   drm/bridge: synopsys: dw-mipi-dsi: Add input bus format negotiation
>     support
>   drm/bridge: synopsys: dw-mipi-dsi: Force input bus flags
>   drm/bridge: synopsys: dw-mipi-dsi: Add mode fixup support
>   drm/bridge: synopsys: dw-mipi-dsi: Use pixel clock rate to calculate
>     lbcc
>   drm/bridge: synopsys: dw-mipi-dsi: Set minimum lane byte clock cycles
>     for HSA and HBP
>   drm/bridge: synopsys: dw-mipi-dsi: Disable HSTX and LPRX timeout check
>   dt-bindings: display: bridge: Document Freescale i.MX93 MIPI DSI
>   drm/bridge: imx: Add i.MX93 MIPI DSI support
> 
>  .../display/bridge/fsl,imx93-mipi-dsi.yaml    | 115 +++
>  drivers/gpu/drm/bridge/imx/Kconfig            |  10 +
>  drivers/gpu/drm/bridge/imx/Makefile           |   1 +
>  drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c   | 934 ++++++++++++++++++
>  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c |  91 +-
>  include/drm/bridge/dw_mipi_dsi.h              |  16 +
>  6 files changed, 1163 insertions(+), 4 deletions(-)
>  create mode 100644
> Documentation/devicetree/bindings/display/bridge/fsl,imx93-mipi-dsi.yaml
> create mode 100644 drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c

Thanks for posting this patch series. I was trying to use this driver on our 
TQMa93xxLA platform where the DSI signals are connected to a TC9595 (driver 
tc358767) DSI-to-DP bridge.
Unfortunately this bridge requires the DSI signals to be in LP-11 upon reset 
and while in idle, otherwise not even DP AUX channel is functional.
Apparently DSI is currently not in LP-11. But reading the RM I have no idea 
how to configure the DSI host to achieve that. Do you have additional 
information which might help me here?
Also could you provide your DT configuration?

Thanks and best regards,
Alexander
-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH 9/9] drm/bridge: imx: Add i.MX93 MIPI DSI support

[Alexander Stein]

Hi,

thanks for the patch.

Am Montag, 17. Juli 2023, 08:18:31 CEST schrieb Liu Ying:
> Freescale i.MX93 SoC embeds a Synopsys Designware MIPI DSI host
> controller and a Synopsys Designware MIPI DPHY.  Some configurations
> and extensions to them are controlled by i.MX93 media blk-ctrl.
> 
> Add a DRM bridge for i.MX93 MIPI DSI by using existing DW MIPI DSI
> bridge helpers and implementing i.MX93 MIPI DSI specific extensions.
> 
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
>  drivers/gpu/drm/bridge/imx/Kconfig          |  10 +
>  drivers/gpu/drm/bridge/imx/Makefile         |   1 +
>  drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c | 934 ++++++++++++++++++++
>  3 files changed, 945 insertions(+)
>  create mode 100644 drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c
> 
> diff --git a/drivers/gpu/drm/bridge/imx/Kconfig
> b/drivers/gpu/drm/bridge/imx/Kconfig index 9fae28db6aa7..5182298c7182
> 100644
> --- a/drivers/gpu/drm/bridge/imx/Kconfig
> +++ b/drivers/gpu/drm/bridge/imx/Kconfig
> @@ -49,4 +49,14 @@ config DRM_IMX8QXP_PIXEL_LINK_TO_DPI
>  	  Choose this to enable pixel link to display pixel 
interface(PXL2DPI)
>  	  found in Freescale i.MX8qxp processor.
> 
> +config DRM_IMX93_MIPI_DSI
> +	tristate "Freescale i.MX93 specific extensions for Synopsys DW MIPI 
DSI"
> +	depends on OF
> +	depends on COMMON_CLK
> +	select DRM_DW_MIPI_DSI
> +	select GENERIC_PHY_MIPI_DPHY
> +	help
> +	  Choose this to enable MIPI DSI controller found in Freescale 
i.MX93
> +	  processor.
> +
>  endif # ARCH_MXC || COMPILE_TEST
> diff --git a/drivers/gpu/drm/bridge/imx/Makefile
> b/drivers/gpu/drm/bridge/imx/Makefile index 8e2ebf3399a1..2b0c2e44aa1b
> 100644
> --- a/drivers/gpu/drm/bridge/imx/Makefile
> +++ b/drivers/gpu/drm/bridge/imx/Makefile
> @@ -4,3 +4,4 @@ obj-$(CONFIG_DRM_IMX8QXP_LDB) += imx8qxp-ldb.o
>  obj-$(CONFIG_DRM_IMX8QXP_PIXEL_COMBINER) += imx8qxp-pixel-combiner.o
>  obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK) += imx8qxp-pixel-link.o
>  obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK_TO_DPI) += imx8qxp-pxl2dpi.o
> +obj-$(CONFIG_DRM_IMX93_MIPI_DSI) += imx93-mipi-dsi.o
> diff --git a/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c
> b/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c new file mode 100644
> index 000000000000..77f59e3407a0
> --- /dev/null
> +++ b/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c

> [snip]

> +static int imx93_dsi_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct imx93_dsi *dsi;
> +	int ret;
> +
> +	dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
> +	if (!dsi)
> +		return -ENOMEM;
> +
> +	dsi->regmap = syscon_regmap_lookup_by_phandle(np, "fsl,media-blk-
ctrl");
> +	if (IS_ERR(dsi->regmap)) {
> +		ret = PTR_ERR(dsi->regmap);
> +		DRM_DEV_ERROR(dev, "failed to get block ctrl regmap: 
%d\n", ret);

Could you use dev_err_probe here instead?

> +		return ret;
> +	}
> +
> +	dsi->clk_pixel = devm_clk_get(dev, "pix");
> +	if (IS_ERR(dsi->clk_pixel)) {
> +		ret = PTR_ERR(dsi->clk_pixel);
> +		if (ret != -EPROBE_DEFER)
> +			DRM_DEV_ERROR(dev, "failed to get pixel clock: 
%d\n", ret);

Could you use dev_err_probe here instead?

> +		return ret;
> +	}
> +
> +	dsi->clk_cfg = devm_clk_get(dev, "phy_cfg");
> +	if (IS_ERR(dsi->clk_cfg)) {
> +		ret = PTR_ERR(dsi->clk_cfg);
> +		if (ret != -EPROBE_DEFER)
> +			DRM_DEV_ERROR(dev, "failed to get phy cfg clock: 
%d\n", ret);
> +		return ret;
> +	}
> +
> +	dsi->clk_ref = devm_clk_get(dev, "phy_ref");
> +	if (IS_ERR(dsi->clk_ref)) {
> +		ret = PTR_ERR(dsi->clk_ref);
> +		if (ret != -EPROBE_DEFER)
> +			DRM_DEV_ERROR(dev, "failed to get phy ref clock: 
%d\n", ret);

Could you use dev_err_probe here instead?

> +		return ret;
> +	}
> +
> +	dsi->ref_clk_rate = clk_get_rate(dsi->clk_ref);
> +	if (dsi->ref_clk_rate < REF_CLK_RATE_MIN ||
> +	    dsi->ref_clk_rate > REF_CLK_RATE_MAX) {
> +		DRM_DEV_ERROR(dev, "invalid phy ref clock rate %lu\n",
> +			      dsi->ref_clk_rate);
> +		return -EINVAL;
> +	}
> +	DRM_DEV_DEBUG_DRIVER(dev, "phy ref clock rate: %lu\n", dsi-
>ref_clk_rate);
> +
> +	dsi->dev = dev;
> +	dsi->pdata.max_data_lanes = 4;
> +	dsi->pdata.mode_valid = imx93_dsi_mode_valid;
> +	dsi->pdata.mode_fixup = imx93_dsi_mode_fixup;
> +	dsi->pdata.get_input_bus_fmts = imx93_dsi_get_input_bus_fmts;
> +	dsi->pdata.phy_ops = &imx93_dsi_phy_ops;
> +	dsi->pdata.host_ops = &imx93_dsi_host_ops;
> +	dsi->pdata.priv_data = dsi;
> +	platform_set_drvdata(pdev, dsi);
> +
> +	dsi->dmd = dw_mipi_dsi_probe(pdev, &dsi->pdata);
> +	if (IS_ERR(dsi->dmd)) {
> +		ret = PTR_ERR(dsi->dmd);
> +		if (ret != -EPROBE_DEFER)
> +			DRM_DEV_ERROR(dev, "failed to probe dw_mipi_dsi: 
%d\n", ret);

Could you use dev_err_probe here instead?

Best regards,
Alexander

> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void imx93_dsi_remove(struct platform_device *pdev)
> +{
> +	struct imx93_dsi *dsi = platform_get_drvdata(pdev);
> +
> +	dw_mipi_dsi_remove(dsi->dmd);
> +}
> +
> +static const struct of_device_id imx93_dsi_dt_ids[] = {
> +	{ .compatible = "fsl,imx93-mipi-dsi", },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, imx93_dsi_dt_ids);
> +
> +static struct platform_driver imx93_dsi_driver = {
> +	.probe	= imx93_dsi_probe,
> +	.remove_new = imx93_dsi_remove,
> +	.driver	= {
> +		.of_match_table = imx93_dsi_dt_ids,
> +		.name = "imx93_mipi_dsi",
> +	},
> +};
> +module_platform_driver(imx93_dsi_driver);
> +
> +MODULE_DESCRIPTION("Freescale i.MX93 MIPI DSI driver");
> +MODULE_AUTHOR("Liu Ying <victor.liu@nxp.com>");
> +MODULE_LICENSE("GPL");


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

RE: [PATCH 0/9] drm/bridge: imx: Add i.MX93 MIPI DSI support

[Ying Liu]

On Tuesday, July 18, 2023 3:40 PM Alexander Stein <alexander.stein@ew.tq-group.com> wrote:
>
> Hi,

Hi,

>
> Am Montag, 17. Juli 2023, 08:18:22 CEST schrieb Liu Ying:
> > Hi,
> >
> > This series aims to add MIPI DSI support for Freescale i.MX93 SoC.
> >
> > There is a Synopsys DesignWare MIPI DSI host controller and a Synopsys
> > Designware MIPI DPHY embedded in i.MX93.  Some configurations and
> > extensions to them are controlled by i.MX93 media blk-ctrl.
> >
> > Add a DRM bridge for i.MX93 MIPI DSI by using existing DW MIPI DSI
> > bridge helpers and implementing i.MX93 MIPI DSI specific extensions.
> >
> > Note that since this series touches the dw-mipi-dsi driver, tests are
> > needed to be done for meson, rockchip and stm.
> >
> > Patch 1 ~ 7 do preparation work for adding i.MX93 MIPI DSI DRM bridge
> > driver.
> >
> > Patch 8 adds DT-binding documentation for i.MX93 MIPI DSI.
> >
> > Patch 9 adds i.MX93 MIPI DSI DRM bridge.
> >
> > Liu Ying (9):
> >   drm/bridge: synopsys: dw-mipi-dsi: Add dw_mipi_dsi_get_bridge() helper
> >   drm/bridge: synopsys: dw-mipi-dsi: Add input bus format negotiation
> >     support
> >   drm/bridge: synopsys: dw-mipi-dsi: Force input bus flags
> >   drm/bridge: synopsys: dw-mipi-dsi: Add mode fixup support
> >   drm/bridge: synopsys: dw-mipi-dsi: Use pixel clock rate to calculate
> >     lbcc
> >   drm/bridge: synopsys: dw-mipi-dsi: Set minimum lane byte clock cycles
> >     for HSA and HBP
> >   drm/bridge: synopsys: dw-mipi-dsi: Disable HSTX and LPRX timeout check
> >   dt-bindings: display: bridge: Document Freescale i.MX93 MIPI DSI
> >   drm/bridge: imx: Add i.MX93 MIPI DSI support
> >
> >  .../display/bridge/fsl,imx93-mipi-dsi.yaml    | 115 +++
> >  drivers/gpu/drm/bridge/imx/Kconfig            |  10 +
> >  drivers/gpu/drm/bridge/imx/Makefile           |   1 +
> >  drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c   | 934
> ++++++++++++++++++
> >  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c |  91 +-
> >  include/drm/bridge/dw_mipi_dsi.h              |  16 +
> >  6 files changed, 1163 insertions(+), 4 deletions(-)
> >  create mode 100644
> > Documentation/devicetree/bindings/display/bridge/fsl,imx93-mipi-
> dsi.yaml
> > create mode 100644 drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c
>
> Thanks for posting this patch series. I was trying to use this driver on our
> TQMa93xxLA platform where the DSI signals are connected to a TC9595
> (driver
> tc358767) DSI-to-DP bridge.

Thanks for trying to test/use this driver.

I don't have TC9595 bridge to test, unfortunately.

> Unfortunately this bridge requires the DSI signals to be in LP-11 upon reset
> and while in idle, otherwise not even DP AUX channel is functional.
> Apparently DSI is currently not in LP-11. But reading the RM I have no idea
> how to configure the DSI host to achieve that. Do you have additional
> information which might help me here?

Hmm, probably no.
But, I tested ADV7535 DSI to HDMI bridge or RM67191 DSI panel on
i.MX93 11x11 EVK with this series, which works.

> Also could you provide your DT configuration?

For media blk-ctrl, dsi and lcdif, see:
https://pastebin.mozilla.org/aP8tFrPM

For adv7535 display pipeline, see:
https://pastebin.mozilla.org/89zwvr9Y

Note assigned-clock-rates is needed in lcdif DT node to suggest video pll rate.

Regards,
Liu Ying

>
> Thanks and best regards,
> Alexander
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> http://www.t/
> q-
> group.com%2F&data=05%7C01%7Cvictor.liu%40nxp.com%7Cbf329d1b4d704
> 801a94f08db876225f6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
> 7C638252627845769696%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7
> C%7C&sdata=Q4wBL6Ji0wcMVIJpuR1gNAqBtFgUiJAwA5QvesFoGLc%3D&rese
> rved=0
>

RE: [PATCH 9/9] drm/bridge: imx: Add i.MX93 MIPI DSI support

[Ying Liu]

On Tuesday, July 18, 2023 3:49 PM Alexander Stein <alexander.stein@ew.tq-group.com> wrote:
>
> Hi,

Hi,

>
> thanks for the patch.

Thanks for your review.

>
> Am Montag, 17. Juli 2023, 08:18:31 CEST schrieb Liu Ying:
> > Freescale i.MX93 SoC embeds a Synopsys Designware MIPI DSI host
> > controller and a Synopsys Designware MIPI DPHY.  Some configurations
> > and extensions to them are controlled by i.MX93 media blk-ctrl.
> >
> > Add a DRM bridge for i.MX93 MIPI DSI by using existing DW MIPI DSI
> > bridge helpers and implementing i.MX93 MIPI DSI specific extensions.
> >
> > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > ---
> >  drivers/gpu/drm/bridge/imx/Kconfig          |  10 +
> >  drivers/gpu/drm/bridge/imx/Makefile         |   1 +
> >  drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c | 934
> ++++++++++++++++++++
> >  3 files changed, 945 insertions(+)
> >  create mode 100644 drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c
> >
> > diff --git a/drivers/gpu/drm/bridge/imx/Kconfig
> > b/drivers/gpu/drm/bridge/imx/Kconfig index
> 9fae28db6aa7..5182298c7182
> > 100644
> > --- a/drivers/gpu/drm/bridge/imx/Kconfig
> > +++ b/drivers/gpu/drm/bridge/imx/Kconfig
> > @@ -49,4 +49,14 @@ config DRM_IMX8QXP_PIXEL_LINK_TO_DPI
> >       Choose this to enable pixel link to display pixel
> interface(PXL2DPI)
> >       found in Freescale i.MX8qxp processor.
> >
> > +config DRM_IMX93_MIPI_DSI
> > +   tristate "Freescale i.MX93 specific extensions for Synopsys DW MIPI
> DSI"
> > +   depends on OF
> > +   depends on COMMON_CLK
> > +   select DRM_DW_MIPI_DSI
> > +   select GENERIC_PHY_MIPI_DPHY
> > +   help
> > +     Choose this to enable MIPI DSI controller found in Freescale
> i.MX93
> > +     processor.
> > +
> >  endif # ARCH_MXC || COMPILE_TEST
> > diff --git a/drivers/gpu/drm/bridge/imx/Makefile
> > b/drivers/gpu/drm/bridge/imx/Makefile index
> 8e2ebf3399a1..2b0c2e44aa1b
> > 100644
> > --- a/drivers/gpu/drm/bridge/imx/Makefile
> > +++ b/drivers/gpu/drm/bridge/imx/Makefile
> > @@ -4,3 +4,4 @@ obj-$(CONFIG_DRM_IMX8QXP_LDB) += imx8qxp-ldb.o
> >  obj-$(CONFIG_DRM_IMX8QXP_PIXEL_COMBINER) += imx8qxp-pixel-
> combiner.o
> >  obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK) += imx8qxp-pixel-link.o
> >  obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK_TO_DPI) += imx8qxp-pxl2dpi.o
> > +obj-$(CONFIG_DRM_IMX93_MIPI_DSI) += imx93-mipi-dsi.o
> > diff --git a/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c
> > b/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c new file mode 100644
> > index 000000000000..77f59e3407a0
> > --- /dev/null
> > +++ b/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c
>
> > [snip]
>
> > +static int imx93_dsi_probe(struct platform_device *pdev)
> > +{
> > +   struct device *dev = &pdev->dev;
> > +   struct device_node *np = dev->of_node;
> > +   struct imx93_dsi *dsi;
> > +   int ret;
> > +
> > +   dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
> > +   if (!dsi)
> > +           return -ENOMEM;
> > +
> > +   dsi->regmap = syscon_regmap_lookup_by_phandle(np, "fsl,media-
> blk-
> ctrl");
> > +   if (IS_ERR(dsi->regmap)) {
> > +           ret = PTR_ERR(dsi->regmap);
> > +           DRM_DEV_ERROR(dev, "failed to get block ctrl regmap:
> %d\n", ret);
>
> Could you use dev_err_probe here instead?

Maybe, it's better to keep using DRM_DEV_ERROR to achieve consistent
error log format across the driver which is implemented in drm_dev_printk().
I see other DRM drivers do the same.

>
> > +           return ret;
> > +   }
> > +
> > +   dsi->clk_pixel = devm_clk_get(dev, "pix");
> > +   if (IS_ERR(dsi->clk_pixel)) {
> > +           ret = PTR_ERR(dsi->clk_pixel);
> > +           if (ret != -EPROBE_DEFER)
> > +                   DRM_DEV_ERROR(dev, "failed to get pixel clock:
> %d\n", ret);
>
> Could you use dev_err_probe here instead?

Ditto.

>
> > +           return ret;
> > +   }
> > +
> > +   dsi->clk_cfg = devm_clk_get(dev, "phy_cfg");
> > +   if (IS_ERR(dsi->clk_cfg)) {
> > +           ret = PTR_ERR(dsi->clk_cfg);
> > +           if (ret != -EPROBE_DEFER)
> > +                   DRM_DEV_ERROR(dev, "failed to get phy cfg clock:
> %d\n", ret);
> > +           return ret;
> > +   }
> > +
> > +   dsi->clk_ref = devm_clk_get(dev, "phy_ref");
> > +   if (IS_ERR(dsi->clk_ref)) {
> > +           ret = PTR_ERR(dsi->clk_ref);
> > +           if (ret != -EPROBE_DEFER)
> > +                   DRM_DEV_ERROR(dev, "failed to get phy ref clock:
> %d\n", ret);
>
> Could you use dev_err_probe here instead?

Ditto.

>
> > +           return ret;
> > +   }
> > +
> > +   dsi->ref_clk_rate = clk_get_rate(dsi->clk_ref);
> > +   if (dsi->ref_clk_rate < REF_CLK_RATE_MIN ||
> > +       dsi->ref_clk_rate > REF_CLK_RATE_MAX) {
> > +           DRM_DEV_ERROR(dev, "invalid phy ref clock rate %lu\n",
> > +                         dsi->ref_clk_rate);
> > +           return -EINVAL;
> > +   }
> > +   DRM_DEV_DEBUG_DRIVER(dev, "phy ref clock rate: %lu\n", dsi-
> >ref_clk_rate);
> > +
> > +   dsi->dev = dev;
> > +   dsi->pdata.max_data_lanes = 4;
> > +   dsi->pdata.mode_valid = imx93_dsi_mode_valid;
> > +   dsi->pdata.mode_fixup = imx93_dsi_mode_fixup;
> > +   dsi->pdata.get_input_bus_fmts = imx93_dsi_get_input_bus_fmts;
> > +   dsi->pdata.phy_ops = &imx93_dsi_phy_ops;
> > +   dsi->pdata.host_ops = &imx93_dsi_host_ops;
> > +   dsi->pdata.priv_data = dsi;
> > +   platform_set_drvdata(pdev, dsi);
> > +
> > +   dsi->dmd = dw_mipi_dsi_probe(pdev, &dsi->pdata);
> > +   if (IS_ERR(dsi->dmd)) {
> > +           ret = PTR_ERR(dsi->dmd);
> > +           if (ret != -EPROBE_DEFER)
> > +                   DRM_DEV_ERROR(dev, "failed to probe dw_mipi_dsi:
> %d\n", ret);
>
> Could you use dev_err_probe here instead?

Ditto.

Regards,
Liu Ying

>
> Best regards,
> Alexander
>
> > +           return ret;
> > +   }
> > +
> > +   return 0;
> > +}
> > +
> > +static void imx93_dsi_remove(struct platform_device *pdev)
> > +{
> > +   struct imx93_dsi *dsi = platform_get_drvdata(pdev);
> > +
> > +   dw_mipi_dsi_remove(dsi->dmd);
> > +}
> > +
> > +static const struct of_device_id imx93_dsi_dt_ids[] = {
> > +   { .compatible = "fsl,imx93-mipi-dsi", },
> > +   { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(of, imx93_dsi_dt_ids);
> > +
> > +static struct platform_driver imx93_dsi_driver = {
> > +   .probe  = imx93_dsi_probe,
> > +   .remove_new = imx93_dsi_remove,
> > +   .driver = {
> > +           .of_match_table = imx93_dsi_dt_ids,
> > +           .name = "imx93_mipi_dsi",
> > +   },
> > +};
> > +module_platform_driver(imx93_dsi_driver);
> > +
> > +MODULE_DESCRIPTION("Freescale i.MX93 MIPI DSI driver");
> > +MODULE_AUTHOR("Liu Ying <victor.liu@nxp.com>");
> > +MODULE_LICENSE("GPL");
>
>
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> http://www.t/
> q-
> group.com%2F&data=05%7C01%7Cvictor.liu%40nxp.com%7Cc42417f9a9484
> 3ead2b808db876380d3%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C638252633665634690%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
> AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%
> 7C%7C&sdata=GUWOZgHFFp0nKImw2aIAsaqMv9KtgI6%2BD%2BaOdDhJ%2B
> tU%3D&reserved=0
>

Re: [PATCH 9/9] drm/bridge: imx: Add i.MX93 MIPI DSI support

[Alexander Stein]

Hi,

Am Dienstag, 18. Juli 2023, 11:00:25 CEST schrieb Ying Liu:
> On Tuesday, July 18, 2023 3:49 PM Alexander Stein <alexander.stein@ew.tq-
group.com> wrote:
> > Hi,
> 
> Hi,
> 
> > thanks for the patch.
> 
> Thanks for your review.
> 
> > Am Montag, 17. Juli 2023, 08:18:31 CEST schrieb Liu Ying:
> > > Freescale i.MX93 SoC embeds a Synopsys Designware MIPI DSI host
> > > controller and a Synopsys Designware MIPI DPHY.  Some configurations
> > > and extensions to them are controlled by i.MX93 media blk-ctrl.
> > > 
> > > Add a DRM bridge for i.MX93 MIPI DSI by using existing DW MIPI DSI
> > > bridge helpers and implementing i.MX93 MIPI DSI specific extensions.
> > > 
> > > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > > ---
> > > 
> > >  drivers/gpu/drm/bridge/imx/Kconfig          |  10 +
> > >  drivers/gpu/drm/bridge/imx/Makefile         |   1 +
> > >  drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c | 934
> > 
> > ++++++++++++++++++++
> > 
> > >  3 files changed, 945 insertions(+)
> > >  create mode 100644 drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c
> > > 
> > > diff --git a/drivers/gpu/drm/bridge/imx/Kconfig
> > > b/drivers/gpu/drm/bridge/imx/Kconfig index
> > 
> > 9fae28db6aa7..5182298c7182
> > 
> > > 100644
> > > --- a/drivers/gpu/drm/bridge/imx/Kconfig
> > > +++ b/drivers/gpu/drm/bridge/imx/Kconfig
> > > @@ -49,4 +49,14 @@ config DRM_IMX8QXP_PIXEL_LINK_TO_DPI
> > > 
> > >       Choose this to enable pixel link to display pixel
> > 
> > interface(PXL2DPI)
> > 
> > >       found in Freescale i.MX8qxp processor.
> > > 
> > > +config DRM_IMX93_MIPI_DSI
> > > +   tristate "Freescale i.MX93 specific extensions for Synopsys DW MIPI
> > 
> > DSI"
> > 
> > > +   depends on OF
> > > +   depends on COMMON_CLK
> > > +   select DRM_DW_MIPI_DSI
> > > +   select GENERIC_PHY_MIPI_DPHY
> > > +   help
> > > +     Choose this to enable MIPI DSI controller found in Freescale
> > 
> > i.MX93
> > 
> > > +     processor.
> > > +
> > > 
> > >  endif # ARCH_MXC || COMPILE_TEST
> > > 
> > > diff --git a/drivers/gpu/drm/bridge/imx/Makefile
> > > b/drivers/gpu/drm/bridge/imx/Makefile index
> > 
> > 8e2ebf3399a1..2b0c2e44aa1b
> > 
> > > 100644
> > > --- a/drivers/gpu/drm/bridge/imx/Makefile
> > > +++ b/drivers/gpu/drm/bridge/imx/Makefile
> > > @@ -4,3 +4,4 @@ obj-$(CONFIG_DRM_IMX8QXP_LDB) += imx8qxp-ldb.o
> > > 
> > >  obj-$(CONFIG_DRM_IMX8QXP_PIXEL_COMBINER) += imx8qxp-pixel-
> > 
> > combiner.o
> > 
> > >  obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK) += imx8qxp-pixel-link.o
> > >  obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK_TO_DPI) += imx8qxp-pxl2dpi.o
> > > 
> > > +obj-$(CONFIG_DRM_IMX93_MIPI_DSI) += imx93-mipi-dsi.o
> > > diff --git a/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c
> > > b/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c new file mode 100644
> > > index 000000000000..77f59e3407a0
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c
> > > 
> > > [snip]
> > > 
> > > +static int imx93_dsi_probe(struct platform_device *pdev)
> > > +{
> > > +   struct device *dev = &pdev->dev;
> > > +   struct device_node *np = dev->of_node;
> > > +   struct imx93_dsi *dsi;
> > > +   int ret;
> > > +
> > > +   dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
> > > +   if (!dsi)
> > > +           return -ENOMEM;
> > > +
> > > +   dsi->regmap = syscon_regmap_lookup_by_phandle(np, "fsl,media-
> > 
> > blk-
> > ctrl");
> > 
> > > +   if (IS_ERR(dsi->regmap)) {
> > > +           ret = PTR_ERR(dsi->regmap);
> > 
> > > +           DRM_DEV_ERROR(dev, "failed to get block ctrl regmap:
> > %d\n", ret);
> > 
> > Could you use dev_err_probe here instead?
> 
> Maybe, it's better to keep using DRM_DEV_ERROR to achieve consistent
> error log format across the driver which is implemented in drm_dev_printk().
> I see other DRM drivers do the same.

I see your point. On the other hand the benefit of dev_err_probe() is that the 
message of deferred probe can be seen in /sys/kernel/debug/devices_deferred.
Your check against EPROBE_DEFER will hide the message if something is not 
correct.

Maybe a to be introduced DRM_DEV_ERROR_PROBE might be useful.

Best regards,
Alexander

> > > +           return ret;
> > > +   }
> > > +
> > > +   dsi->clk_pixel = devm_clk_get(dev, "pix");
> > > +   if (IS_ERR(dsi->clk_pixel)) {
> > > +           ret = PTR_ERR(dsi->clk_pixel);
> > > +           if (ret != -EPROBE_DEFER)
> > 
> > > +                   DRM_DEV_ERROR(dev, "failed to get pixel clock:
> > %d\n", ret);
> > 
> > Could you use dev_err_probe here instead?
> 
> Ditto.
> 
> > > +           return ret;
> > > +   }
> > > +
> > > +   dsi->clk_cfg = devm_clk_get(dev, "phy_cfg");
> > > +   if (IS_ERR(dsi->clk_cfg)) {
> > > +           ret = PTR_ERR(dsi->clk_cfg);
> > > +           if (ret != -EPROBE_DEFER)
> > 
> > > +                   DRM_DEV_ERROR(dev, "failed to get phy cfg clock:
> > %d\n", ret);
> > 
> > > +           return ret;
> > > +   }
> > > +
> > > +   dsi->clk_ref = devm_clk_get(dev, "phy_ref");
> > > +   if (IS_ERR(dsi->clk_ref)) {
> > > +           ret = PTR_ERR(dsi->clk_ref);
> > > +           if (ret != -EPROBE_DEFER)
> > 
> > > +                   DRM_DEV_ERROR(dev, "failed to get phy ref clock:
> > %d\n", ret);
> > 
> > Could you use dev_err_probe here instead?
> 
> Ditto.
> 
> > > +           return ret;
> > > +   }
> > > +
> > > +   dsi->ref_clk_rate = clk_get_rate(dsi->clk_ref);
> > > +   if (dsi->ref_clk_rate < REF_CLK_RATE_MIN ||
> > > +       dsi->ref_clk_rate > REF_CLK_RATE_MAX) {
> > > +           DRM_DEV_ERROR(dev, "invalid phy ref clock rate %lu\n",
> > > +                         dsi->ref_clk_rate);
> > > +           return -EINVAL;
> > > +   }
> > > +   DRM_DEV_DEBUG_DRIVER(dev, "phy ref clock rate: %lu\n", dsi-
> > >
> > >ref_clk_rate);
> > >
> > > +
> > > +   dsi->dev = dev;
> > > +   dsi->pdata.max_data_lanes = 4;
> > > +   dsi->pdata.mode_valid = imx93_dsi_mode_valid;
> > > +   dsi->pdata.mode_fixup = imx93_dsi_mode_fixup;
> > > +   dsi->pdata.get_input_bus_fmts = imx93_dsi_get_input_bus_fmts;
> > > +   dsi->pdata.phy_ops = &imx93_dsi_phy_ops;
> > > +   dsi->pdata.host_ops = &imx93_dsi_host_ops;
> > > +   dsi->pdata.priv_data = dsi;
> > > +   platform_set_drvdata(pdev, dsi);
> > > +
> > > +   dsi->dmd = dw_mipi_dsi_probe(pdev, &dsi->pdata);
> > > +   if (IS_ERR(dsi->dmd)) {
> > > +           ret = PTR_ERR(dsi->dmd);
> > > +           if (ret != -EPROBE_DEFER)
> > 
> > > +                   DRM_DEV_ERROR(dev, "failed to probe dw_mipi_dsi:
> > %d\n", ret);
> > 
> > Could you use dev_err_probe here instead?
> 
> Ditto.
> 
> Regards,
> Liu Ying
> 
> > Best regards,
> > Alexander
> > 
> > > +           return ret;
> > > +   }
> > > +
> > > +   return 0;
> > > +}
> > > +
> > > +static void imx93_dsi_remove(struct platform_device *pdev)
> > > +{
> > > +   struct imx93_dsi *dsi = platform_get_drvdata(pdev);
> > > +
> > > +   dw_mipi_dsi_remove(dsi->dmd);
> > > +}
> > > +
> > > +static const struct of_device_id imx93_dsi_dt_ids[] = {
> > > +   { .compatible = "fsl,imx93-mipi-dsi", },
> > > +   { /* sentinel */ }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, imx93_dsi_dt_ids);
> > > +
> > > +static struct platform_driver imx93_dsi_driver = {
> > > +   .probe  = imx93_dsi_probe,
> > > +   .remove_new = imx93_dsi_remove,
> > > +   .driver = {
> > > +           .of_match_table = imx93_dsi_dt_ids,
> > > +           .name = "imx93_mipi_dsi",
> > > +   },
> > > +};
> > > +module_platform_driver(imx93_dsi_driver);
> > > +
> > > +MODULE_DESCRIPTION("Freescale i.MX93 MIPI DSI driver");
> > > +MODULE_AUTHOR("Liu Ying <victor.liu@nxp.com>");
> > > +MODULE_LICENSE("GPL");
> > 
> > --
> > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > Amtsgericht München, HRB 105018
> > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > http://www.t/
> > q-
> > group.com%2F&data=05%7C01%7Cvictor.liu%40nxp.com%7Cc42417f9a9484
> > 3ead2b808db876380d3%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> > %7C638252633665634690%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
> > AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%
> > 7C%7C&sdata=GUWOZgHFFp0nKImw2aIAsaqMv9KtgI6%2BD%2BaOdDhJ%2B
> > tU%3D&reserved=0


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH 9/9] drm/bridge: imx: Add i.MX93 MIPI DSI support

[Jagan Teki]

On Tue, Jul 18, 2023 at 8:28 AM Ying Liu <victor.liu@nxp.com> wrote:
>
> Hi Jagan,
>
> On Monday, July 17, 2023 2:44 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> > On Mon, Jul 17, 2023 at 11:44 AM Liu Ying <victor.liu@nxp.com> wrote:
> > >
> > > Freescale i.MX93 SoC embeds a Synopsys Designware MIPI DSI host
> > > controller and a Synopsys Designware MIPI DPHY.  Some configurations
> > > and extensions to them are controlled by i.MX93 media blk-ctrl.
> > >
> > > Add a DRM bridge for i.MX93 MIPI DSI by using existing DW MIPI DSI
> > > bridge helpers and implementing i.MX93 MIPI DSI specific extensions.
> >
> > I think the better way would add compatibility to be part of existing
> > dw-mipi-dsi.c with specific driver data. This way it avoids all the
> > platform-related helpers(extensions) and makes the driver generic to
> > all SoCs which use DW DSI IP. It would be a straightforward change as
> > the imx93 drm pipeline already supports bridge topology.
>
> The platform-related stuff is handed over to dw-mipi-dsi.c via struct
> dw_mipi_dsi_plat_data as an argument of dw_mipi_dsi_probe().  It looks
> ok for vendor drivers to call dw_mipi_dsi_probe() to set the platform-related
> information(rockchip, meson and stm do that), like pdata.phy_ops and
> pdata.host_ops.

I understand this topology of having soc-platform drivers with
dw-mipi-dsi bridge. What I'm suggesting is to not add a soc-platform
driver for imx93 instead add add support directly on dw-mipi-dsi
bridge.

>
> dw-mipi-dsi.c is generic w/wo this patch series.
>
> Can you elaborate more about adding compatibility to be part of existing
> dw-mipi-dsi.c with specific driver data?  I don't see clear approach to do
> that.

Please check the above comments, an example of samsung-dsim.c

Thanks,
Jagan.

RE: [PATCH 9/9] drm/bridge: imx: Add i.MX93 MIPI DSI support

[Ying Liu]

On Tuesday, July 18, 2023 5:35 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> 
> >
> > Hi Jagan,
> >
> > On Monday, July 17, 2023 2:44 PM Jagan Teki
> <jagan@amarulasolutions.com> wrote:
> > > On Mon, Jul 17, 2023 at 11:44 AM Liu Ying <victor.liu@nxp.com> wrote:
> > > >
> > > > Freescale i.MX93 SoC embeds a Synopsys Designware MIPI DSI host
> > > > controller and a Synopsys Designware MIPI DPHY.  Some configurations
> > > > and extensions to them are controlled by i.MX93 media blk-ctrl.
> > > >
> > > > Add a DRM bridge for i.MX93 MIPI DSI by using existing DW MIPI DSI
> > > > bridge helpers and implementing i.MX93 MIPI DSI specific extensions.
> > >
> > > I think the better way would add compatibility to be part of existing
> > > dw-mipi-dsi.c with specific driver data. This way it avoids all the
> > > platform-related helpers(extensions) and makes the driver generic to
> > > all SoCs which use DW DSI IP. It would be a straightforward change as
> > > the imx93 drm pipeline already supports bridge topology.
> >
> > The platform-related stuff is handed over to dw-mipi-dsi.c via struct
> > dw_mipi_dsi_plat_data as an argument of dw_mipi_dsi_probe().  It looks
> > ok for vendor drivers to call dw_mipi_dsi_probe() to set the platform-
> related
> > information(rockchip, meson and stm do that), like pdata.phy_ops and
> > pdata.host_ops.
> 
> I understand this topology of having soc-platform drivers with
> dw-mipi-dsi bridge. What I'm suggesting is to not add a soc-platform
> driver for imx93 instead add add support directly on dw-mipi-dsi
> bridge.

It seems that directly supporting i.MX93 MIPI DSI in dw-mipi-dsi.c is
not feasible.  The main reason is that struct dw_mipi_dsi_plat_data
contains phy_ops and each vendor driver provides very different
methods for phy, while...

> 
> >
> > dw-mipi-dsi.c is generic w/wo this patch series.
> >
> > Can you elaborate more about adding compatibility to be part of existing
> > dw-mipi-dsi.c with specific driver data?  I don't see clear approach to do
> > that.
> 
> Please check the above comments, an example of samsung-dsim.c

... it seems that samsung-dsim.c uses struct samsung_dsim_driver_data to
differential platform information and it doesn't contain any callback, which
means comparing to DW MIPI DSI, Samsung DSIM shows more consistency
across vendor SoCs from HW IP/SoC integration PoV.

Regards,
Liu Ying

> 
> Thanks,
> Jagan.

RE: [PATCH 9/9] drm/bridge: imx: Add i.MX93 MIPI DSI support

[Ying Liu]

On Tuesday, July 18, 2023 5:31 PM Alexander Stein <alexander.stein@ew.tq-group.com> wrote:
>
> Hi,

Hi,

>
> Am Dienstag, 18. Juli 2023, 11:00:25 CEST schrieb Ying Liu:
> > On Tuesday, July 18, 2023 3:49 PM Alexander Stein
> <alexander.stein@ew.tq-
> group.com> wrote:
> > > Hi,
> >
> > Hi,
> >
> > > thanks for the patch.
> >
> > Thanks for your review.
> >
> > > Am Montag, 17. Juli 2023, 08:18:31 CEST schrieb Liu Ying:
> > > > Freescale i.MX93 SoC embeds a Synopsys Designware MIPI DSI host
> > > > controller and a Synopsys Designware MIPI DPHY.  Some configurations
> > > > and extensions to them are controlled by i.MX93 media blk-ctrl.
> > > >
> > > > Add a DRM bridge for i.MX93 MIPI DSI by using existing DW MIPI DSI
> > > > bridge helpers and implementing i.MX93 MIPI DSI specific extensions.
> > > >
> > > > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> > > > ---
> > > >
> > > >  drivers/gpu/drm/bridge/imx/Kconfig          |  10 +
> > > >  drivers/gpu/drm/bridge/imx/Makefile         |   1 +
> > > >  drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c | 934
> > >
> > > ++++++++++++++++++++
> > >
> > > >  3 files changed, 945 insertions(+)
> > > >  create mode 100644 drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/imx/Kconfig
> > > > b/drivers/gpu/drm/bridge/imx/Kconfig index
> > >
> > > 9fae28db6aa7..5182298c7182
> > >
> > > > 100644
> > > > --- a/drivers/gpu/drm/bridge/imx/Kconfig
> > > > +++ b/drivers/gpu/drm/bridge/imx/Kconfig
> > > > @@ -49,4 +49,14 @@ config DRM_IMX8QXP_PIXEL_LINK_TO_DPI
> > > >
> > > >       Choose this to enable pixel link to display pixel
> > >
> > > interface(PXL2DPI)
> > >
> > > >       found in Freescale i.MX8qxp processor.
> > > >
> > > > +config DRM_IMX93_MIPI_DSI
> > > > +   tristate "Freescale i.MX93 specific extensions for Synopsys DW MIPI
> > >
> > > DSI"
> > >
> > > > +   depends on OF
> > > > +   depends on COMMON_CLK
> > > > +   select DRM_DW_MIPI_DSI
> > > > +   select GENERIC_PHY_MIPI_DPHY
> > > > +   help
> > > > +     Choose this to enable MIPI DSI controller found in Freescale
> > >
> > > i.MX93
> > >
> > > > +     processor.
> > > > +
> > > >
> > > >  endif # ARCH_MXC || COMPILE_TEST
> > > >
> > > > diff --git a/drivers/gpu/drm/bridge/imx/Makefile
> > > > b/drivers/gpu/drm/bridge/imx/Makefile index
> > >
> > > 8e2ebf3399a1..2b0c2e44aa1b
> > >
> > > > 100644
> > > > --- a/drivers/gpu/drm/bridge/imx/Makefile
> > > > +++ b/drivers/gpu/drm/bridge/imx/Makefile
> > > > @@ -4,3 +4,4 @@ obj-$(CONFIG_DRM_IMX8QXP_LDB) += imx8qxp-
> ldb.o
> > > >
> > > >  obj-$(CONFIG_DRM_IMX8QXP_PIXEL_COMBINER) += imx8qxp-pixel-
> > >
> > > combiner.o
> > >
> > > >  obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK) += imx8qxp-pixel-link.o
> > > >  obj-$(CONFIG_DRM_IMX8QXP_PIXEL_LINK_TO_DPI) += imx8qxp-
> pxl2dpi.o
> > > >
> > > > +obj-$(CONFIG_DRM_IMX93_MIPI_DSI) += imx93-mipi-dsi.o
> > > > diff --git a/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c
> > > > b/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c new file mode 100644
> > > > index 000000000000..77f59e3407a0
> > > > --- /dev/null
> > > > +++ b/drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c
> > > >
> > > > [snip]
> > > >
> > > > +static int imx93_dsi_probe(struct platform_device *pdev)
> > > > +{
> > > > +   struct device *dev = &pdev->dev;
> > > > +   struct device_node *np = dev->of_node;
> > > > +   struct imx93_dsi *dsi;
> > > > +   int ret;
> > > > +
> > > > +   dsi = devm_kzalloc(dev, sizeof(*dsi), GFP_KERNEL);
> > > > +   if (!dsi)
> > > > +           return -ENOMEM;
> > > > +
> > > > +   dsi->regmap = syscon_regmap_lookup_by_phandle(np, "fsl,media-
> > >
> > > blk-
> > > ctrl");
> > >
> > > > +   if (IS_ERR(dsi->regmap)) {
> > > > +           ret = PTR_ERR(dsi->regmap);
> > >
> > > > +           DRM_DEV_ERROR(dev, "failed to get block ctrl regmap:
> > > %d\n", ret);
> > >
> > > Could you use dev_err_probe here instead?
> >
> > Maybe, it's better to keep using DRM_DEV_ERROR to achieve consistent
> > error log format across the driver which is implemented in
> drm_dev_printk().
> > I see other DRM drivers do the same.
>
> I see your point. On the other hand the benefit of dev_err_probe() is that the
> message of deferred probe can be seen in
> /sys/kernel/debug/devices_deferred.
> Your check against EPROBE_DEFER will hide the message if something is not
> correct.
>
> Maybe a to be introduced DRM_DEV_ERROR_PROBE might be useful.

Maybe.  I assume this is not a must-have for this series - maybe someone may
come along to introduce it in future.

Regards,
Liu Ying

>
> Best regards,
> Alexander
>
> > > > +           return ret;
> > > > +   }
> > > > +
> > > > +   dsi->clk_pixel = devm_clk_get(dev, "pix");
> > > > +   if (IS_ERR(dsi->clk_pixel)) {
> > > > +           ret = PTR_ERR(dsi->clk_pixel);
> > > > +           if (ret != -EPROBE_DEFER)
> > >
> > > > +                   DRM_DEV_ERROR(dev, "failed to get pixel clock:
> > > %d\n", ret);
> > >
> > > Could you use dev_err_probe here instead?
> >
> > Ditto.
> >
> > > > +           return ret;
> > > > +   }
> > > > +
> > > > +   dsi->clk_cfg = devm_clk_get(dev, "phy_cfg");
> > > > +   if (IS_ERR(dsi->clk_cfg)) {
> > > > +           ret = PTR_ERR(dsi->clk_cfg);
> > > > +           if (ret != -EPROBE_DEFER)
> > >
> > > > +                   DRM_DEV_ERROR(dev, "failed to get phy cfg clock:
> > > %d\n", ret);
> > >
> > > > +           return ret;
> > > > +   }
> > > > +
> > > > +   dsi->clk_ref = devm_clk_get(dev, "phy_ref");
> > > > +   if (IS_ERR(dsi->clk_ref)) {
> > > > +           ret = PTR_ERR(dsi->clk_ref);
> > > > +           if (ret != -EPROBE_DEFER)
> > >
> > > > +                   DRM_DEV_ERROR(dev, "failed to get phy ref clock:
> > > %d\n", ret);
> > >
> > > Could you use dev_err_probe here instead?
> >
> > Ditto.
> >
> > > > +           return ret;
> > > > +   }
> > > > +
> > > > +   dsi->ref_clk_rate = clk_get_rate(dsi->clk_ref);
> > > > +   if (dsi->ref_clk_rate < REF_CLK_RATE_MIN ||
> > > > +       dsi->ref_clk_rate > REF_CLK_RATE_MAX) {
> > > > +           DRM_DEV_ERROR(dev, "invalid phy ref clock rate %lu\n",
> > > > +                         dsi->ref_clk_rate);
> > > > +           return -EINVAL;
> > > > +   }
> > > > +   DRM_DEV_DEBUG_DRIVER(dev, "phy ref clock rate: %lu\n", dsi-
> > > >
> > > >ref_clk_rate);
> > > >
> > > > +
> > > > +   dsi->dev = dev;
> > > > +   dsi->pdata.max_data_lanes = 4;
> > > > +   dsi->pdata.mode_valid = imx93_dsi_mode_valid;
> > > > +   dsi->pdata.mode_fixup = imx93_dsi_mode_fixup;
> > > > +   dsi->pdata.get_input_bus_fmts = imx93_dsi_get_input_bus_fmts;
> > > > +   dsi->pdata.phy_ops = &imx93_dsi_phy_ops;
> > > > +   dsi->pdata.host_ops = &imx93_dsi_host_ops;
> > > > +   dsi->pdata.priv_data = dsi;
> > > > +   platform_set_drvdata(pdev, dsi);
> > > > +
> > > > +   dsi->dmd = dw_mipi_dsi_probe(pdev, &dsi->pdata);
> > > > +   if (IS_ERR(dsi->dmd)) {
> > > > +           ret = PTR_ERR(dsi->dmd);
> > > > +           if (ret != -EPROBE_DEFER)
> > >
> > > > +                   DRM_DEV_ERROR(dev, "failed to probe dw_mipi_dsi:
> > > %d\n", ret);
> > >
> > > Could you use dev_err_probe here instead?
> >
> > Ditto.
> >
> > Regards,
> > Liu Ying
> >
> > > Best regards,
> > > Alexander
> > >
> > > > +           return ret;
> > > > +   }
> > > > +
> > > > +   return 0;
> > > > +}
> > > > +
> > > > +static void imx93_dsi_remove(struct platform_device *pdev)
> > > > +{
> > > > +   struct imx93_dsi *dsi = platform_get_drvdata(pdev);
> > > > +
> > > > +   dw_mipi_dsi_remove(dsi->dmd);
> > > > +}
> > > > +
> > > > +static const struct of_device_id imx93_dsi_dt_ids[] = {
> > > > +   { .compatible = "fsl,imx93-mipi-dsi", },
> > > > +   { /* sentinel */ }
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, imx93_dsi_dt_ids);
> > > > +
> > > > +static struct platform_driver imx93_dsi_driver = {
> > > > +   .probe  = imx93_dsi_probe,
> > > > +   .remove_new = imx93_dsi_remove,
> > > > +   .driver = {
> > > > +           .of_match_table = imx93_dsi_dt_ids,
> > > > +           .name = "imx93_mipi_dsi",
> > > > +   },
> > > > +};
> > > > +module_platform_driver(imx93_dsi_driver);
> > > > +
> > > > +MODULE_DESCRIPTION("Freescale i.MX93 MIPI DSI driver");
> > > > +MODULE_AUTHOR("Liu Ying <victor.liu@nxp.com>");
> > > > +MODULE_LICENSE("GPL");
> > >
> > > --
> > > TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> > > Amtsgericht München, HRB 105018
> > > Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> > >
> http://www.t/
> %2F&data=05%7C01%7Cvictor.liu%40nxp.com%7Cefbbc19ba3414f89448308
> db8771b5df%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638252
> 694676247773%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJ
> QIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdat
> a=BCL6xs5i8jZZDFC2ONKMv5%2B1o0mpuD0ocxC3BnUa2To%3D&reserved=0
> > > q-
> > >
> group.com%2F&data=05%7C01%7Cvictor.liu%40nxp.com%7Cc42417f9a9484
> > >
> 3ead2b808db876380d3%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> > > %7C638252633665634690%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4
> wLj
> > >
> AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%
> > >
> 7C%7C&sdata=GUWOZgHFFp0nKImw2aIAsaqMv9KtgI6%2BD%2BaOdDhJ%2B
> > > tU%3D&reserved=0
>
>
> --
> TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
> Amtsgericht München, HRB 105018
> Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
> http://www.t/
> q-
> group.com%2F&data=05%7C01%7Cvictor.liu%40nxp.com%7Cefbbc19ba3414
> f89448308db8771b5df%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
> 7C638252694676247773%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjA
> wMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7
> C%7C&sdata=%2FCtQd5%2BYCDCjOKmsbiO2LSeE1hqQsrmhepmSGalydhc%3
> D&reserved=0
>

Re: [PATCH 9/9] drm/bridge: imx: Add i.MX93 MIPI DSI support

[Jagan Teki]

Hi,

On Tue, Jul 18, 2023 at 3:19 PM Ying Liu <victor.liu@nxp.com> wrote:
>
> On Tuesday, July 18, 2023 5:35 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > >
> > > Hi Jagan,
> > >
> > > On Monday, July 17, 2023 2:44 PM Jagan Teki
> > <jagan@amarulasolutions.com> wrote:
> > > > On Mon, Jul 17, 2023 at 11:44 AM Liu Ying <victor.liu@nxp.com> wrote:
> > > > >
> > > > > Freescale i.MX93 SoC embeds a Synopsys Designware MIPI DSI host
> > > > > controller and a Synopsys Designware MIPI DPHY.  Some configurations
> > > > > and extensions to them are controlled by i.MX93 media blk-ctrl.
> > > > >
> > > > > Add a DRM bridge for i.MX93 MIPI DSI by using existing DW MIPI DSI
> > > > > bridge helpers and implementing i.MX93 MIPI DSI specific extensions.
> > > >
> > > > I think the better way would add compatibility to be part of existing
> > > > dw-mipi-dsi.c with specific driver data. This way it avoids all the
> > > > platform-related helpers(extensions) and makes the driver generic to
> > > > all SoCs which use DW DSI IP. It would be a straightforward change as
> > > > the imx93 drm pipeline already supports bridge topology.
> > >
> > > The platform-related stuff is handed over to dw-mipi-dsi.c via struct
> > > dw_mipi_dsi_plat_data as an argument of dw_mipi_dsi_probe().  It looks
> > > ok for vendor drivers to call dw_mipi_dsi_probe() to set the platform-
> > related
> > > information(rockchip, meson and stm do that), like pdata.phy_ops and
> > > pdata.host_ops.
> >
> > I understand this topology of having soc-platform drivers with
> > dw-mipi-dsi bridge. What I'm suggesting is to not add a soc-platform
> > driver for imx93 instead add add support directly on dw-mipi-dsi
> > bridge.
>
> It seems that directly supporting i.MX93 MIPI DSI in dw-mipi-dsi.c is
> not feasible.  The main reason is that struct dw_mipi_dsi_plat_data
> contains phy_ops and each vendor driver provides very different
> methods for phy, while...

Cannot this phy_ops goes to PHY core somewhere around and even it is
possible to add via driver data for imx93 by untouching existing
handling? I know it is not as direct as I describe but it is worth
maintaining the driver this way to keep control of the future
soc-drivers to include in dw-mipi-dsi instead of handling platform
code separately.

>
> >
> > >
> > > dw-mipi-dsi.c is generic w/wo this patch series.
> > >
> > > Can you elaborate more about adding compatibility to be part of existing
> > > dw-mipi-dsi.c with specific driver data?  I don't see clear approach to do
> > > that.
> >
> > Please check the above comments, an example of samsung-dsim.c
>
> ... it seems that samsung-dsim.c uses struct samsung_dsim_driver_data to
> differential platform information and it doesn't contain any callback, which
> means comparing to DW MIPI DSI, Samsung DSIM shows more consistency
> across vendor SoCs from HW IP/SoC integration PoV.

Yes, but is it possible to adjust struct according to DW MIPI DSI.

Thanks
Jagan.

Re: [PATCH 9/9] drm/bridge: imx: Add i.MX93 MIPI DSI support

[Sam Ravnborg]

Hi Ying Liu,

On Tue, Jul 18, 2023 at 09:00:25AM +0000, Ying Liu wrote:
> > > +   if (IS_ERR(dsi->regmap)) {
> > > +           ret = PTR_ERR(dsi->regmap);
> > > +           DRM_DEV_ERROR(dev, "failed to get block ctrl regmap:
> > %d\n", ret);
> >
> > Could you use dev_err_probe here instead?
> 
> Maybe, it's better to keep using DRM_DEV_ERROR to achieve consistent
> error log format across the driver which is implemented in drm_dev_printk().
> I see other DRM drivers do the same.

All the DRM_* macros are deprecated.
New code shall use drm_*, dev_* or pr_ as appropriate.

The appropriate variant here is dev_err_probe().

	Sam

Re: [PATCH 8/9] dt-bindings: display: bridge: Document Freescale i.MX93 MIPI DSI

[Rob Herring]

On Mon, 17 Jul 2023 14:18:30 +0800, Liu Ying wrote:
> Freescale i.MX93 SoC embeds a Synopsys Designware MIPI DSI host
> controller and a Synopsys Designware MIPI DPHY.  Some configurations
> and extensions to them are controlled by i.MX93 media blk-ctrl.
> 
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
>  .../display/bridge/fsl,imx93-mipi-dsi.yaml    | 115 ++++++++++++++++++
>  1 file changed, 115 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/bridge/fsl,imx93-mipi-dsi.yaml
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH 9/9] drm/bridge: imx: Add i.MX93 MIPI DSI support

[kernel test robot]

Hi Liu,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-exynos/exynos-drm-next]
[also build test ERROR on drm-intel/for-linux-next drm-intel/for-linux-next-fixes drm-tip/drm-tip linus/master v6.5-rc2 next-20230718]
[cannot apply to drm-misc/drm-misc-next drm/drm-next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Liu-Ying/drm-bridge-synopsys-dw-mipi-dsi-Add-dw_mipi_dsi_get_bridge-helper/20230718-172247
base:   https://git.kernel.org/pub/scm/linux/kernel/git/daeinki/drm-exynos.git exynos-drm-next
patch link:    https://lore.kernel.org/r/20230717061831.1826878-10-victor.liu%40nxp.com
patch subject: [PATCH 9/9] drm/bridge: imx: Add i.MX93 MIPI DSI support
config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20230719/202307190811.jOcroxF5-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230719/202307190811.jOcroxF5-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202307190811.jOcroxF5-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   In file included from arch/arm/include/asm/div64.h:107,
                    from include/linux/math.h:6,
                    from include/linux/kernel.h:26,
                    from include/linux/clk.h:13,
                    from drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c:9:
   drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c: In function 'dphy_pll_get_configure_from_opts':
   include/asm-generic/div64.h:222:35: warning: comparison of distinct pointer types lacks a cast
     222 |         (void)(((typeof((n)) *)0) == ((uint64_t *)0));  \
         |                                   ^~
   drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c:272:17: note: in expansion of macro 'do_div'
     272 |                 do_div(tmp, n * fvco_div);
         |                 ^~~~~~
   In file included from include/linux/build_bug.h:5,
                    from include/linux/bitfield.h:10,
                    from drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c:7:
>> include/asm-generic/div64.h:234:32: warning: right shift count >= width of type [-Wshift-count-overflow]
     234 |         } else if (likely(((n) >> 32) == 0)) {          \
         |                                ^~
   include/linux/compiler.h:76:45: note: in definition of macro 'likely'
      76 | # define likely(x)      __builtin_expect(!!(x), 1)
         |                                             ^
   drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c:272:17: note: in expansion of macro 'do_div'
     272 |                 do_div(tmp, n * fvco_div);
         |                 ^~~~~~
>> include/asm-generic/div64.h:238:36: error: passing argument 1 of '__div64_32' from incompatible pointer type [-Werror=incompatible-pointer-types]
     238 |                 __rem = __div64_32(&(n), __base);       \
         |                                    ^~~~
         |                                    |
         |                                    long unsigned int *
   drivers/gpu/drm/bridge/imx/imx93-mipi-dsi.c:272:17: note: in expansion of macro 'do_div'
     272 |                 do_div(tmp, n * fvco_div);
         |                 ^~~~~~
   arch/arm/include/asm/div64.h:24:45: note: expected 'uint64_t *' {aka 'long long unsigned int *'} but argument is of type 'long unsigned int *'
      24 | static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
         |                                   ~~~~~~~~~~^
   cc1: some warnings being treated as errors


vim +/__div64_32 +238 include/asm-generic/div64.h

^1da177e4c3f41 Linus Torvalds     2005-04-16  215  
^1da177e4c3f41 Linus Torvalds     2005-04-16  216  /* The unnecessary pointer compare is there
^1da177e4c3f41 Linus Torvalds     2005-04-16  217   * to check for type safety (n must be 64bit)
^1da177e4c3f41 Linus Torvalds     2005-04-16  218   */
^1da177e4c3f41 Linus Torvalds     2005-04-16  219  # define do_div(n,base) ({				\
^1da177e4c3f41 Linus Torvalds     2005-04-16  220  	uint32_t __base = (base);			\
^1da177e4c3f41 Linus Torvalds     2005-04-16  221  	uint32_t __rem;					\
^1da177e4c3f41 Linus Torvalds     2005-04-16  222  	(void)(((typeof((n)) *)0) == ((uint64_t *)0));	\
911918aa7ef6f8 Nicolas Pitre      2015-11-02  223  	if (__builtin_constant_p(__base) &&		\
911918aa7ef6f8 Nicolas Pitre      2015-11-02  224  	    is_power_of_2(__base)) {			\
911918aa7ef6f8 Nicolas Pitre      2015-11-02  225  		__rem = (n) & (__base - 1);		\
911918aa7ef6f8 Nicolas Pitre      2015-11-02  226  		(n) >>= ilog2(__base);			\
c747ce4706190e Geert Uytterhoeven 2021-08-11  227  	} else if (__builtin_constant_p(__base) &&	\
461a5e51060c93 Nicolas Pitre      2015-10-30  228  		   __base != 0) {			\
461a5e51060c93 Nicolas Pitre      2015-10-30  229  		uint32_t __res_lo, __n_lo = (n);	\
461a5e51060c93 Nicolas Pitre      2015-10-30  230  		(n) = __div64_const32(n, __base);	\
461a5e51060c93 Nicolas Pitre      2015-10-30  231  		/* the remainder can be computed with 32-bit regs */ \
461a5e51060c93 Nicolas Pitre      2015-10-30  232  		__res_lo = (n);				\
461a5e51060c93 Nicolas Pitre      2015-10-30  233  		__rem = __n_lo - __res_lo * __base;	\
911918aa7ef6f8 Nicolas Pitre      2015-11-02 @234  	} else if (likely(((n) >> 32) == 0)) {		\
^1da177e4c3f41 Linus Torvalds     2005-04-16  235  		__rem = (uint32_t)(n) % __base;		\
^1da177e4c3f41 Linus Torvalds     2005-04-16  236  		(n) = (uint32_t)(n) / __base;		\
c747ce4706190e Geert Uytterhoeven 2021-08-11  237  	} else {					\
^1da177e4c3f41 Linus Torvalds     2005-04-16 @238  		__rem = __div64_32(&(n), __base);	\
c747ce4706190e Geert Uytterhoeven 2021-08-11  239  	}						\
^1da177e4c3f41 Linus Torvalds     2005-04-16  240  	__rem;						\
^1da177e4c3f41 Linus Torvalds     2005-04-16  241   })
^1da177e4c3f41 Linus Torvalds     2005-04-16  242  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

RE: [PATCH 9/9] drm/bridge: imx: Add i.MX93 MIPI DSI support

[Ying Liu]

On Tuesday, July 18, 2023 6:51 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> 
> Hi,

Hi,

> 
> On Tue, Jul 18, 2023 at 3:19 PM Ying Liu <victor.liu@nxp.com> wrote:
> >
> > On Tuesday, July 18, 2023 5:35 PM Jagan Teki
> <jagan@amarulasolutions.com> wrote:
> > >
> > > >
> > > > Hi Jagan,
> > > >
> > > > On Monday, July 17, 2023 2:44 PM Jagan Teki
> > > <jagan@amarulasolutions.com> wrote:
> > > > > On Mon, Jul 17, 2023 at 11:44 AM Liu Ying <victor.liu@nxp.com>
> wrote:
> > > > > >
> > > > > > Freescale i.MX93 SoC embeds a Synopsys Designware MIPI DSI host
> > > > > > controller and a Synopsys Designware MIPI DPHY.  Some
> configurations
> > > > > > and extensions to them are controlled by i.MX93 media blk-ctrl.
> > > > > >
> > > > > > Add a DRM bridge for i.MX93 MIPI DSI by using existing DW MIPI DSI
> > > > > > bridge helpers and implementing i.MX93 MIPI DSI specific
> extensions.
> > > > >
> > > > > I think the better way would add compatibility to be part of existing
> > > > > dw-mipi-dsi.c with specific driver data. This way it avoids all the
> > > > > platform-related helpers(extensions) and makes the driver generic to
> > > > > all SoCs which use DW DSI IP. It would be a straightforward change as
> > > > > the imx93 drm pipeline already supports bridge topology.
> > > >
> > > > The platform-related stuff is handed over to dw-mipi-dsi.c via struct
> > > > dw_mipi_dsi_plat_data as an argument of dw_mipi_dsi_probe().  It
> looks
> > > > ok for vendor drivers to call dw_mipi_dsi_probe() to set the platform-
> > > related
> > > > information(rockchip, meson and stm do that), like pdata.phy_ops and
> > > > pdata.host_ops.
> > >
> > > I understand this topology of having soc-platform drivers with
> > > dw-mipi-dsi bridge. What I'm suggesting is to not add a soc-platform
> > > driver for imx93 instead add add support directly on dw-mipi-dsi
> > > bridge.
> >
> > It seems that directly supporting i.MX93 MIPI DSI in dw-mipi-dsi.c is
> > not feasible.  The main reason is that struct dw_mipi_dsi_plat_data
> > contains phy_ops and each vendor driver provides very different
> > methods for phy, while...
> 
> Cannot this phy_ops goes to PHY core somewhere around and even it is
> possible to add via driver data for imx93 by untouching existing
> handling? I know it is not as direct as I describe but it is worth
> maintaining the driver this way to keep control of the future
> soc-drivers to include in dw-mipi-dsi instead of handling platform
> code separately.

Like I said, each vendor driver provides very different methods for phy.
The reason is that phy IPs are different and SoC integration things are
handled via struct dw_mipi_dsi_phy_ops.  Meson calls devm_phy_get()
to get a phy.  Rockchip calls devm_phy_create() to create a phy.  Meson,
rockchip and stm have their own struct dw_mipi_dsi_phy_ops
implementations, same to i.MX93.  Different pixel format control is
implemented in meson's and i.MX93's ->init() callback. Meson additionally
handles clocks in ->init() callback.

Generally speaking, it's a no-go to put phy_ops into PHY core for all the
vendors, IMHO.

In particular, i.MX93 MIPI DPHY's PLL can be fully controlled by media
blk-ctrl(as a syscon) thru the PLL's SoC control interface, while PHY
registers can be controlled by DW MIPI DSI testdin/out control interface
alternatively including a part of the PLL registers.  So, adding a PHY
driver for i.MX93 MIPI DPHY doesn't make sense since the PLL controlled
by media blk-ctrl doesn't constitute a PHY.  Instead, dw_mipi_dsi_phy_ops
may cover all the PHY control well.

From my PoV, w/wo this series, dw-mipi-dsi.c looks fine to keep being
generic and easy to maintain.  All vendor drivers do is to handle platform
specific stuff, which is good.

> 
> >
> > >
> > > >
> > > > dw-mipi-dsi.c is generic w/wo this patch series.
> > > >
> > > > Can you elaborate more about adding compatibility to be part of
> existing
> > > > dw-mipi-dsi.c with specific driver data?  I don't see clear approach to do
> > > > that.
> > >
> > > Please check the above comments, an example of samsung-dsim.c
> >
> > ... it seems that samsung-dsim.c uses struct samsung_dsim_driver_data to
> > differential platform information and it doesn't contain any callback, which
> > means comparing to DW MIPI DSI, Samsung DSIM shows more consistency
> > across vendor SoCs from HW IP/SoC integration PoV.
> 
> Yes, but is it possible to adjust struct according to DW MIPI DSI.

Looking at the vendor drivers, they show a lot diversity, which cannot be
parameterized into a struct like the samsung dsim driver does.

struct dw_mipi_dsi_plat_data hides all platform specific information from
dw-mipi-dsi.c well, IMHO.

Regards,
Liu Ying

> 
> Thanks
> Jagan.

RE: [PATCH 9/9] drm/bridge: imx: Add i.MX93 MIPI DSI support

[Ying Liu]

On Tuesday, July 18, 2023 11:47 PM  Sam Ravnborg <sam@ravnborg.org> wrote:
> 
> Hi Ying Liu,

Hi Sam,

> 
> On Tue, Jul 18, 2023 at 09:00:25AM +0000, Ying Liu wrote:
> > > > +   if (IS_ERR(dsi->regmap)) {
> > > > +           ret = PTR_ERR(dsi->regmap);
> > > > +           DRM_DEV_ERROR(dev, "failed to get block ctrl regmap:
> > > %d\n", ret);
> > >
> > > Could you use dev_err_probe here instead?
> >
> > Maybe, it's better to keep using DRM_DEV_ERROR to achieve consistent
> > error log format across the driver which is implemented in
> drm_dev_printk().
> > I see other DRM drivers do the same.
> 
> All the DRM_* macros are deprecated.
> New code shall use drm_*, dev_* or pr_ as appropriate.

Ok, will use dev_* in v2.

> 
> The appropriate variant here is dev_err_probe().

Ok, will use dev_err_probe() here in v2.

Regards,
Liu Ying

> 
> 	Sam

Re: [PATCH 9/9] drm/bridge: imx: Add i.MX93 MIPI DSI support

[Jagan Teki]

On Wed, Jul 19, 2023 at 2:05 PM Ying Liu <victor.liu@nxp.com> wrote:
>
> On Tuesday, July 18, 2023 6:51 PM Jagan Teki <jagan@amarulasolutions.com> wrote:
> >
> > Hi,
>
> Hi,
>
> >
> > On Tue, Jul 18, 2023 at 3:19 PM Ying Liu <victor.liu@nxp.com> wrote:
> > >
> > > On Tuesday, July 18, 2023 5:35 PM Jagan Teki
> > <jagan@amarulasolutions.com> wrote:
> > > >
> > > > >
> > > > > Hi Jagan,
> > > > >
> > > > > On Monday, July 17, 2023 2:44 PM Jagan Teki
> > > > <jagan@amarulasolutions.com> wrote:
> > > > > > On Mon, Jul 17, 2023 at 11:44 AM Liu Ying <victor.liu@nxp.com>
> > wrote:
> > > > > > >
> > > > > > > Freescale i.MX93 SoC embeds a Synopsys Designware MIPI DSI host
> > > > > > > controller and a Synopsys Designware MIPI DPHY.  Some
> > configurations
> > > > > > > and extensions to them are controlled by i.MX93 media blk-ctrl.
> > > > > > >
> > > > > > > Add a DRM bridge for i.MX93 MIPI DSI by using existing DW MIPI DSI
> > > > > > > bridge helpers and implementing i.MX93 MIPI DSI specific
> > extensions.
> > > > > >
> > > > > > I think the better way would add compatibility to be part of existing
> > > > > > dw-mipi-dsi.c with specific driver data. This way it avoids all the
> > > > > > platform-related helpers(extensions) and makes the driver generic to
> > > > > > all SoCs which use DW DSI IP. It would be a straightforward change as
> > > > > > the imx93 drm pipeline already supports bridge topology.
> > > > >
> > > > > The platform-related stuff is handed over to dw-mipi-dsi.c via struct
> > > > > dw_mipi_dsi_plat_data as an argument of dw_mipi_dsi_probe().  It
> > looks
> > > > > ok for vendor drivers to call dw_mipi_dsi_probe() to set the platform-
> > > > related
> > > > > information(rockchip, meson and stm do that), like pdata.phy_ops and
> > > > > pdata.host_ops.
> > > >
> > > > I understand this topology of having soc-platform drivers with
> > > > dw-mipi-dsi bridge. What I'm suggesting is to not add a soc-platform
> > > > driver for imx93 instead add add support directly on dw-mipi-dsi
> > > > bridge.
> > >
> > > It seems that directly supporting i.MX93 MIPI DSI in dw-mipi-dsi.c is
> > > not feasible.  The main reason is that struct dw_mipi_dsi_plat_data
> > > contains phy_ops and each vendor driver provides very different
> > > methods for phy, while...
> >
> > Cannot this phy_ops goes to PHY core somewhere around and even it is
> > possible to add via driver data for imx93 by untouching existing
> > handling? I know it is not as direct as I describe but it is worth
> > maintaining the driver this way to keep control of the future
> > soc-drivers to include in dw-mipi-dsi instead of handling platform
> > code separately.
>
> Like I said, each vendor driver provides very different methods for phy.
> The reason is that phy IPs are different and SoC integration things are
> handled via struct dw_mipi_dsi_phy_ops.  Meson calls devm_phy_get()
> to get a phy.  Rockchip calls devm_phy_create() to create a phy.  Meson,
> rockchip and stm have their own struct dw_mipi_dsi_phy_ops
> implementations, same to i.MX93.  Different pixel format control is
> implemented in meson's and i.MX93's ->init() callback. Meson additionally
> handles clocks in ->init() callback.
>
> Generally speaking, it's a no-go to put phy_ops into PHY core for all the
> vendors, IMHO.
>
> In particular, i.MX93 MIPI DPHY's PLL can be fully controlled by media
> blk-ctrl(as a syscon) thru the PLL's SoC control interface, while PHY
> registers can be controlled by DW MIPI DSI testdin/out control interface
> alternatively including a part of the PLL registers.  So, adding a PHY
> driver for i.MX93 MIPI DPHY doesn't make sense since the PLL controlled
> by media blk-ctrl doesn't constitute a PHY.  Instead, dw_mipi_dsi_phy_ops
> may cover all the PHY control well.
>
> From my PoV, w/wo this series, dw-mipi-dsi.c looks fine to keep being
> generic and easy to maintain.  All vendor drivers do is to handle platform
> specific stuff, which is good.

Thanks for the explanation. I agreed with on you most of the points
from the perspective of PoV which are difficult to FIT into a common
bridge. However, I'm still believing that the having bridge does only
the bridge job, and keeping the PHY or other PoV-specific stuff on
respective driver subsystems would be great synergy for the bridge
drivers or any common IP driver's point-of-view.

Anyway, I might not control i.MX93 PoV stuff, but I can try on it for
the new DSI which uses this IP, and keep you in CC if it is a possible
land in the near future.

>
> >
> > >
> > > >
> > > > >
> > > > > dw-mipi-dsi.c is generic w/wo this patch series.
> > > > >
> > > > > Can you elaborate more about adding compatibility to be part of
> > existing
> > > > > dw-mipi-dsi.c with specific driver data?  I don't see clear approach to do
> > > > > that.
> > > >
> > > > Please check the above comments, an example of samsung-dsim.c
> > >
> > > ... it seems that samsung-dsim.c uses struct samsung_dsim_driver_data to
> > > differential platform information and it doesn't contain any callback, which
> > > means comparing to DW MIPI DSI, Samsung DSIM shows more consistency
> > > across vendor SoCs from HW IP/SoC integration PoV.
> >
> > Yes, but is it possible to adjust struct according to DW MIPI DSI.
>
> Looking at the vendor drivers, they show a lot diversity, which cannot be
> parameterized into a struct like the samsung dsim driver does.
>
> struct dw_mipi_dsi_plat_data hides all platform specific information from
> dw-mipi-dsi.c well, IMHO.

Okay.

Thanks,
Jagan.

Re: [PATCH 5/9] drm/bridge: synopsys: dw-mipi-dsi: Use pixel clock rate to calculate lbcc

[neil.armstrong]

On 17/07/2023 08:18, Liu Ying wrote:
> To get better accuration, use pixel clock rate to calculate lbcc instead of
> lane_mbps since the pixel clock rate is in KHz while lane_mbps is in MHz.
> Without this, distorted image can be seen on a HDMI monitor connected with
> i.MX93 11x11 EVK through ADV7535 DSI to HDMI bridge in 1920x1080p@60 video
> mode.
> 
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
>   drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 10 +++++++++-
>   1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index c754d55f71d1..332388fd86da 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -12,6 +12,7 @@
>   #include <linux/component.h>
>   #include <linux/debugfs.h>
>   #include <linux/iopoll.h>
> +#include <linux/math64.h>
>   #include <linux/media-bus-format.h>
>   #include <linux/module.h>
>   #include <linux/of_device.h>
> @@ -762,8 +763,15 @@ static u32 dw_mipi_dsi_get_hcomponent_lbcc(struct dw_mipi_dsi *dsi,
>   					   u32 hcomponent)
>   {
>   	u32 frac, lbcc;
> +	int bpp;
>   
> -	lbcc = hcomponent * dsi->lane_mbps * MSEC_PER_SEC / 8;
> +	bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> +	if (bpp < 0) {
> +		dev_err(dsi->dev, "failed to get bpp\n");
> +		return 0;
> +	}
> +
> +	lbcc = div_u64((u64)hcomponent * mode->clock * bpp, dsi->lanes * 8);
>   
>   	frac = lbcc % mode->clock;
>   	lbcc = lbcc / mode->clock;

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH 6/9] drm/bridge: synopsys: dw-mipi-dsi: Set minimum lane byte clock cycles for HSA and HBP

[neil.armstrong]

On 17/07/2023 08:18, Liu Ying wrote:
> According to Synopsys support channel, each region of HSA, HBP and HFP must
> have minimum number of 10 bytes where constant 4 bytes are for HSS or HSE
> and 6 bytes are for blanking packet(header + CRC).  Hence, the below table
> comes in.
> 
> +------------+----------+-------+
> | data lanes | min lbcc | bytes |
> +------------+----------+-------+
> |     1      |    10    |  1*10 |
> +------------+----------+-------+
> |     2      |    5     |  2*5  |
> +------------+----------+-------+
> |     3      |    4     |  3*4  |
> +------------+----------+-------+
> |     4      |    3     |  4*3  |
> +------------+----------+-------+
> 
> Implement the minimum lbcc numbers to make sure that the values programmed
> into DSI_VID_HSA_TIME and DSI_VID_HBP_TIME registers meet the minimum
> number requirement.  For DSI_VID_HLINE_TIME register, it seems that the
> value programmed should be based on mode->htotal as-is, instead of sum up
> HSA, HBP, HFP and HDISPLAY.
> 
> This helps the case where Raydium RM67191 DSI panel is connected, since
> it's video timing for hsync length is only 2 pixels and without this patch
> the programmed value for DSI_VID_HSA_TIME is only 2 with 4 data lanes.
> 
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
>   drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 14 +++++++++++++-
>   1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index 332388fd86da..536306ccea5a 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -757,12 +757,19 @@ static void dw_mipi_dsi_command_mode_config(struct dw_mipi_dsi *dsi)
>   	dsi_write(dsi, DSI_MODE_CFG, ENABLE_CMD_MODE);
>   }
>   
> +static const u32 minimum_lbccs[] = {10, 5, 4, 3};
> +
> +static inline u32 dw_mipi_dsi_get_minimum_lbcc(struct dw_mipi_dsi *dsi)
> +{
> +	return minimum_lbccs[dsi->lanes - 1];
> +}
> +
>   /* Get lane byte clock cycles. */
>   static u32 dw_mipi_dsi_get_hcomponent_lbcc(struct dw_mipi_dsi *dsi,
>   					   const struct drm_display_mode *mode,
>   					   u32 hcomponent)
>   {
> -	u32 frac, lbcc;
> +	u32 frac, lbcc, minimum_lbcc;
>   	int bpp;
>   
>   	bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> @@ -778,6 +785,11 @@ static u32 dw_mipi_dsi_get_hcomponent_lbcc(struct dw_mipi_dsi *dsi,
>   	if (frac)
>   		lbcc++;
>   
> +	minimum_lbcc = dw_mipi_dsi_get_minimum_lbcc(dsi);
> +
> +	if (lbcc < minimum_lbcc)
> +		lbcc = minimum_lbcc;
> +
>   	return lbcc;
>   }
>   

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH 7/9] drm/bridge: synopsys: dw-mipi-dsi: Disable HSTX and LPRX timeout check

[neil.armstrong]

On 17/07/2023 08:18, Liu Ying wrote:
> According to Synopsys DW MIPI DSI host databook, HSTX and LPRX timeout
> contention detections are measured in TO_CLK_DIVISION cycles.  However,
> the current driver programs magic values to TO_CLK_DIVISION, HSTX_TO_CNT
> and LPRX_TO_CNT register fields, which makes timeout error event wrongly
> happen for some video modes, at least for the typical 1920x1080p@60 video
> mode read from a HDMI monitor driven by ADV7535 DSI to HDMI bridge.
> While at it, the current driver doesn't enable interrupt to handle or
> complain about the error status, so true error just happens silently
> except for display distortions by visual check.
> 
> Disable the timeout check by setting those timeout register fields to
> zero for now until someone comes along with better computations for the
> timeout values.  Although the databook doesn't mention what happens when
> they are set to zero, it turns out the false error doesn't happen for
> the 1920x1080p@60 video mode at least.
> 
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
>   drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index 536306ccea5a..0fcadf99e783 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -684,7 +684,7 @@ static void dw_mipi_dsi_init(struct dw_mipi_dsi *dsi)
>   	 * timeout clock division should be computed with the
>   	 * high speed transmission counter timeout and byte lane...
>   	 */
> -	dsi_write(dsi, DSI_CLKMGR_CFG, TO_CLK_DIVISION(10) |
> +	dsi_write(dsi, DSI_CLKMGR_CFG, TO_CLK_DIVISION(0) |
>   		  TX_ESC_CLK_DIVISION(esc_clk_division));
>   }
>   
> @@ -747,7 +747,7 @@ static void dw_mipi_dsi_command_mode_config(struct dw_mipi_dsi *dsi)
>   	 * compute high speed transmission counter timeout according
>   	 * to the timeout clock division (TO_CLK_DIVISION) and byte lane...
>   	 */
> -	dsi_write(dsi, DSI_TO_CNT_CFG, HSTX_TO_CNT(1000) | LPRX_TO_CNT(1000));
> +	dsi_write(dsi, DSI_TO_CNT_CFG, HSTX_TO_CNT(0) | LPRX_TO_CNT(0));
>   	/*
>   	 * TODO dw drv improvements
>   	 * the Bus-Turn-Around Timeout Counter should be computed

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH 5/9] drm/bridge: synopsys: dw-mipi-dsi: Use pixel clock rate to calculate lbcc

[Heiko Stübner]

Hi,

Am Montag, 17. Juli 2023, 08:18:27 CEST schrieb Liu Ying:
> To get better accuration, use pixel clock rate to calculate lbcc instead of
> lane_mbps since the pixel clock rate is in KHz while lane_mbps is in MHz.
> Without this, distorted image can be seen on a HDMI monitor connected with
> i.MX93 11x11 EVK through ADV7535 DSI to HDMI bridge in 1920x1080p@60 video
> mode.
> 
> Signed-off-by: Liu Ying <victor.liu@nxp.com>

looks like I'm late to the party, but this change breaks the display output
my px30 minievb with the xinpeng xpp055c272 dsi display [0].

Found this commit via git bisection and added a bit of debug output to
compare the value differences for the old and new calculation:

[   34.810722] dw_mipi_dsi_get_hcomponent_lbcc: old lbcc: 810 * 480 * 1000 / 8
[   34.810749] dw_mipi_dsi_get_hcomponent_lbcc: new lbcc: 810 * 64000 * 24 / (4 * 8)
[   34.810756] dw_mipi_dsi_get_hcomponent_lbcc: old lbcc: 48600000, new lbcc: 38880000
[   34.810762] dw_mipi_dsi_get_hcomponent_lbcc: old lbcc: 10 * 480 * 1000 / 8
[   34.810767] dw_mipi_dsi_get_hcomponent_lbcc: new lbcc: 10 * 64000 * 24 / (4 * 8)
[   34.810773] dw_mipi_dsi_get_hcomponent_lbcc: old lbcc: 600000, new lbcc: 480000
[   34.810778] dw_mipi_dsi_get_hcomponent_lbcc: old lbcc: 40 * 480 * 1000 / 8
[   34.810783] dw_mipi_dsi_get_hcomponent_lbcc: new lbcc: 40 * 64000 * 24 / (4 * 8)
[   34.810789] dw_mipi_dsi_get_hcomponent_lbcc: old lbcc: 2400000, new lbcc: 1920000

With the new lbcc I get a blank dsi panel and just going back to the old
calculation of lbcc restores the image.

I don't have that much in-depth knowledge about dsi stuff and the original
panel times also "just" came from the vendor tree, but I really would like
to keep that display working ;-) .

Do you have any idea which way to go to fix this?


Thanks
Heiko


[0]
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/rockchip/px30-evb.dts#n138
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/panel/panel-xinpeng-xpp055c272.c

> ---
>  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index c754d55f71d1..332388fd86da 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -12,6 +12,7 @@
>  #include <linux/component.h>
>  #include <linux/debugfs.h>
>  #include <linux/iopoll.h>
> +#include <linux/math64.h>
>  #include <linux/media-bus-format.h>
>  #include <linux/module.h>
>  #include <linux/of_device.h>
> @@ -762,8 +763,15 @@ static u32 dw_mipi_dsi_get_hcomponent_lbcc(struct dw_mipi_dsi *dsi,
>  					   u32 hcomponent)
>  {
>  	u32 frac, lbcc;
> +	int bpp;
>  
> -	lbcc = hcomponent * dsi->lane_mbps * MSEC_PER_SEC / 8;
> +	bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> +	if (bpp < 0) {
> +		dev_err(dsi->dev, "failed to get bpp\n");
> +		return 0;
> +	}
> +
> +	lbcc = div_u64((u64)hcomponent * mode->clock * bpp, dsi->lanes * 8);

RE: [PATCH 5/9] drm/bridge: synopsys: dw-mipi-dsi: Use pixel clock rate to calculate lbcc

[Ying Liu]

On Tuesday, October 17, 2023 2:15 AM, Heiko Stübner <heiko@sntech.de> wrote:
> Hi,

Hi,

>
> Am Montag, 17. Juli 2023, 08:18:27 CEST schrieb Liu Ying:
> > To get better accuration, use pixel clock rate to calculate lbcc instead of
> > lane_mbps since the pixel clock rate is in KHz while lane_mbps is in MHz.
> > Without this, distorted image can be seen on a HDMI monitor connected
> with
> > i.MX93 11x11 EVK through ADV7535 DSI to HDMI bridge in 1920x1080p@60
> video
> > mode.
> >
> > Signed-off-by: Liu Ying <victor.liu@nxp.com>
>
> looks like I'm late to the party, but this change breaks the display output
> my px30 minievb with the xinpeng xpp055c272 dsi display [0].

Hmm, I asked for a test, but anyway sorry for the breakage.

The panel driver sets MIPI_DSI_MODE_VIDEO_BURST.
And, it seems that rockchip dsi driver [1] only supports the burst mode,
because it takes 1/0.8 = 1.25 faster lane_mbps than "bandwidth of RGB".

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c#n568

>
> Found this commit via git bisection and added a bit of debug output to
> compare the value differences for the old and new calculation:
>
> [   34.810722] dw_mipi_dsi_get_hcomponent_lbcc: old lbcc: 810 * 480 * 1000
> / 8
> [   34.810749] dw_mipi_dsi_get_hcomponent_lbcc: new lbcc: 810 * 64000 *
> 24 / (4 * 8)
> [   34.810756] dw_mipi_dsi_get_hcomponent_lbcc: old lbcc: 48600000, new
> lbcc: 38880000
> [   34.810762] dw_mipi_dsi_get_hcomponent_lbcc: old lbcc: 10 * 480 * 1000 /
> 8
> [   34.810767] dw_mipi_dsi_get_hcomponent_lbcc: new lbcc: 10 * 64000 * 24
> / (4 * 8)
> [   34.810773] dw_mipi_dsi_get_hcomponent_lbcc: old lbcc: 600000, new lbcc:
> 480000
> [   34.810778] dw_mipi_dsi_get_hcomponent_lbcc: old lbcc: 40 * 480 * 1000 /
> 8
> [   34.810783] dw_mipi_dsi_get_hcomponent_lbcc: new lbcc: 40 * 64000 * 24
> / (4 * 8)
> [   34.810789] dw_mipi_dsi_get_hcomponent_lbcc: old lbcc: 2400000, new
> lbcc: 1920000

Old lbcc / new lbcc is always 1.25.

The new lbcc is for non-burst modes(sync pulse/sync event), IIUC.
At least, it works for i.MX93 with the RM67191 panel and ADV7535 in
sync pulse mode.

>
> With the new lbcc I get a blank dsi panel and just going back to the old
> calculation of lbcc restores the image.
>
> I don't have that much in-depth knowledge about dsi stuff and the original
> panel times also "just" came from the vendor tree, but I really would like
> to keep that display working ;-) .
>
> Do you have any idea which way to go to fix this?

Can you please test the below patch for your case?

--------------------------------------------------------8<---------------------------------------------------------------------
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -774,13 +774,19 @@ static u32 dw_mipi_dsi_get_hcomponent_lbcc(struct dw_mipi_dsi *dsi,
        u32 frac, lbcc, minimum_lbcc;
        int bpp;

-       bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
-       if (bpp < 0) {
-               dev_err(dsi->dev, "failed to get bpp\n");
-               return 0;
-       }
+       if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
+               /* lbcc based on lane_mbps */
+               lbcc = hcomponent * dsi->lane_mbps * MSEC_PER_SEC / 8;
+       } else {
+               /* lbcc based on pixel clock */
+               bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
+               if (bpp < 0) {
+                       dev_err(dsi->dev, "failed to get bpp\n");
+                       return 0;
+               }

-       lbcc = div_u64((u64)hcomponent * mode->clock * bpp, dsi->lanes * 8);
+               lbcc = div_u64((u64)hcomponent * mode->clock * bpp, dsi->lanes * 8);
+       }

        frac = lbcc % mode->clock;
        lbcc = lbcc / mode->clock;
--------------------------------------------------------8<---------------------------------------------------------------------

It kind of keeps the old lbcc for burst mode, except for the minimum lbcc check
I introduced.

It seems that meson supports non-burst modes only and stm supports both
non-burst modes and burst mode.  With the patch, I still worry about non-burst
modes for stm, assuming the minimum lbcc check is ok and everything works
for meson since I guess Neil has already tested the patch set on meson.

Should we go with the above patch?  If yes, I may send it out.

Regards,
Liu Ying

>
>
> Thanks
> Heiko
>
>
> [0]
> https://git.ker/
> nel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2F
> tree%2Farch%2Farm64%2Fboot%2Fdts%2Frockchip%2Fpx30-
> evb.dts%23n138&data=05%7C01%7Cvictor.liu%40nxp.com%7C8f712ad41720
> 4ba7411808dbce73ce63%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C638330769044424464%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
> AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%
> 7C%7C&sdata=effPCbPOk3GGuO8mR%2FSlcjFJfDUEZmq082simvjkux0%3D&r
> eserved=0
> https://git.ker/
> nel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2F
> tree%2Fdrivers%2Fgpu%2Fdrm%2Fpanel%2Fpanel-xinpeng-
> xpp055c272.c&data=05%7C01%7Cvictor.liu%40nxp.com%7C8f712ad417204b
> a7411808dbce73ce63%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
> C638330769044424464%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
> %7C&sdata=bAgcLnJpIEQaYZZUI1CnUsgP7rMiNV6wKKg%2Bl8%2FlN40%3D&r
> eserved=0
>
> > ---
> >  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> > index c754d55f71d1..332388fd86da 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/component.h>
> >  #include <linux/debugfs.h>
> >  #include <linux/iopoll.h>
> > +#include <linux/math64.h>
> >  #include <linux/media-bus-format.h>
> >  #include <linux/module.h>
> >  #include <linux/of_device.h>
> > @@ -762,8 +763,15 @@ static u32
> dw_mipi_dsi_get_hcomponent_lbcc(struct dw_mipi_dsi *dsi,
> >                                        u32 hcomponent)
> >  {
> >     u32 frac, lbcc;
> > +   int bpp;
> >
> > -   lbcc = hcomponent * dsi->lane_mbps * MSEC_PER_SEC / 8;
> > +   bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> > +   if (bpp < 0) {
> > +           dev_err(dsi->dev, "failed to get bpp\n");
> > +           return 0;
> > +   }
> > +
> > +   lbcc = div_u64((u64)hcomponent * mode->clock * bpp, dsi->lanes *
> 8);
>
>
>

Re: [PATCH 5/9] drm/bridge: synopsys: dw-mipi-dsi: Use pixel clock rate to calculate lbcc

[Heiko Stübner]

Hi,

Am Dienstag, 17. Oktober 2023, 12:15:11 CEST schrieb Ying Liu:
> On Tuesday, October 17, 2023 2:15 AM, Heiko Stübner <heiko@sntech.de> wrote:
> > Am Montag, 17. Juli 2023, 08:18:27 CEST schrieb Liu Ying:
> > > To get better accuration, use pixel clock rate to calculate lbcc instead of
> > > lane_mbps since the pixel clock rate is in KHz while lane_mbps is in MHz.
> > > Without this, distorted image can be seen on a HDMI monitor connected
> > with
> > > i.MX93 11x11 EVK through ADV7535 DSI to HDMI bridge in 1920x1080p@60
> > video
> > > mode.
> > >
> > > Signed-off-by: Liu Ying <victor.liu@nxp.com>
> >
> > looks like I'm late to the party, but this change breaks the display output
> > my px30 minievb with the xinpeng xpp055c272 dsi display [0].
> 
> Hmm, I asked for a test, but anyway sorry for the breakage.

I'm often way behind with looking at drm-related changes, sorry about that.

So thanks a lot for taking the time to look into the problem.


> The panel driver sets MIPI_DSI_MODE_VIDEO_BURST.
> And, it seems that rockchip dsi driver [1] only supports the burst mode,
> because it takes 1/0.8 = 1.25 faster lane_mbps than "bandwidth of RGB".
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c#n568
> 
> >
> > Found this commit via git bisection and added a bit of debug output to
> > compare the value differences for the old and new calculation:
> >
> > [   34.810722] dw_mipi_dsi_get_hcomponent_lbcc: old lbcc: 810 * 480 * 1000
> > / 8
> > [   34.810749] dw_mipi_dsi_get_hcomponent_lbcc: new lbcc: 810 * 64000 *
> > 24 / (4 * 8)
> > [   34.810756] dw_mipi_dsi_get_hcomponent_lbcc: old lbcc: 48600000, new
> > lbcc: 38880000
> > [   34.810762] dw_mipi_dsi_get_hcomponent_lbcc: old lbcc: 10 * 480 * 1000 /
> > 8
> > [   34.810767] dw_mipi_dsi_get_hcomponent_lbcc: new lbcc: 10 * 64000 * 24
> > / (4 * 8)
> > [   34.810773] dw_mipi_dsi_get_hcomponent_lbcc: old lbcc: 600000, new lbcc:
> > 480000
> > [   34.810778] dw_mipi_dsi_get_hcomponent_lbcc: old lbcc: 40 * 480 * 1000 /
> > 8
> > [   34.810783] dw_mipi_dsi_get_hcomponent_lbcc: new lbcc: 40 * 64000 * 24
> > / (4 * 8)
> > [   34.810789] dw_mipi_dsi_get_hcomponent_lbcc: old lbcc: 2400000, new
> > lbcc: 1920000
> 
> Old lbcc / new lbcc is always 1.25.
> 
> The new lbcc is for non-burst modes(sync pulse/sync event), IIUC.
> At least, it works for i.MX93 with the RM67191 panel and ADV7535 in
> sync pulse mode.
> 
> >
> > With the new lbcc I get a blank dsi panel and just going back to the old
> > calculation of lbcc restores the image.
> >
> > I don't have that much in-depth knowledge about dsi stuff and the original
> > panel times also "just" came from the vendor tree, but I really would like
> > to keep that display working ;-) .
> >
> > Do you have any idea which way to go to fix this?
> 
> Can you please test the below patch for your case?

The patch below does fix the display on the device. After applying it
I do get a working display again.


> --------------------------------------------------------8<---------------------------------------------------------------------
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -774,13 +774,19 @@ static u32 dw_mipi_dsi_get_hcomponent_lbcc(struct dw_mipi_dsi *dsi,
>         u32 frac, lbcc, minimum_lbcc;
>         int bpp;
> 
> -       bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> -       if (bpp < 0) {
> -               dev_err(dsi->dev, "failed to get bpp\n");
> -               return 0;
> -       }
> +       if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
> +               /* lbcc based on lane_mbps */
> +               lbcc = hcomponent * dsi->lane_mbps * MSEC_PER_SEC / 8;
> +       } else {
> +               /* lbcc based on pixel clock */
> +               bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> +               if (bpp < 0) {
> +                       dev_err(dsi->dev, "failed to get bpp\n");
> +                       return 0;
> +               }
> 
> -       lbcc = div_u64((u64)hcomponent * mode->clock * bpp, dsi->lanes * 8);
> +               lbcc = div_u64((u64)hcomponent * mode->clock * bpp, dsi->lanes * 8);
> +       }
> 
>         frac = lbcc % mode->clock;
>         lbcc = lbcc / mode->clock;
> --------------------------------------------------------8<---------------------------------------------------------------------
> 
> It kind of keeps the old lbcc for burst mode, except for the minimum lbcc check
> I introduced.
> 
> It seems that meson supports non-burst modes only and stm supports both
> non-burst modes and burst mode.  With the patch, I still worry about non-burst
> modes for stm, assuming the minimum lbcc check is ok and everything works
> for meson since I guess Neil has already tested the patch set on meson.
> 
> Should we go with the above patch?  If yes, I may send it out.

In my mind, definitly :-) .

But maybe Neil as the other reviewer also wants to chime in.


So again thanks for looking into the issue.
Heiko


> >
> > [0]
> > https://git.ker/
> > nel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2F
> > tree%2Farch%2Farm64%2Fboot%2Fdts%2Frockchip%2Fpx30-
> > evb.dts%23n138&data=05%7C01%7Cvictor.liu%40nxp.com%7C8f712ad41720
> > 4ba7411808dbce73ce63%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> > %7C638330769044424464%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLj
> > AwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%
> > 7C%7C&sdata=effPCbPOk3GGuO8mR%2FSlcjFJfDUEZmq082simvjkux0%3D&r
> > eserved=0
> > https://git.ker/
> > nel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Ftorvalds%2Flinux.git%2F
> > tree%2Fdrivers%2Fgpu%2Fdrm%2Fpanel%2Fpanel-xinpeng-
> > xpp055c272.c&data=05%7C01%7Cvictor.liu%40nxp.com%7C8f712ad417204b
> > a7411808dbce73ce63%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
> > C638330769044424464%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> > MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
> > %7C&sdata=bAgcLnJpIEQaYZZUI1CnUsgP7rMiNV6wKKg%2Bl8%2FlN40%3D&r
> > eserved=0
> >
> > > ---
> > >  drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> > b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> > > index c754d55f71d1..332388fd86da 100644
> > > --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> > > +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> > > @@ -12,6 +12,7 @@
> > >  #include <linux/component.h>
> > >  #include <linux/debugfs.h>
> > >  #include <linux/iopoll.h>
> > > +#include <linux/math64.h>
> > >  #include <linux/media-bus-format.h>
> > >  #include <linux/module.h>
> > >  #include <linux/of_device.h>
> > > @@ -762,8 +763,15 @@ static u32
> > dw_mipi_dsi_get_hcomponent_lbcc(struct dw_mipi_dsi *dsi,
> > >                                        u32 hcomponent)
> > >  {
> > >     u32 frac, lbcc;
> > > +   int bpp;
> > >
> > > -   lbcc = hcomponent * dsi->lane_mbps * MSEC_PER_SEC / 8;
> > > +   bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> > > +   if (bpp < 0) {
> > > +           dev_err(dsi->dev, "failed to get bpp\n");
> > > +           return 0;
> > > +   }
> > > +
> > > +   lbcc = div_u64((u64)hcomponent * mode->clock * bpp, dsi->lanes *
> > 8);
> >
> >
> >
> 
>