drm: bridge: Samsung MIPI DSIM bridge

DRM bridge drivers have built-in handling of treating all display pipeline components as bridges.

Convert exynos_drm_dsi to a bridge driver with built-in encoder support for compatibility with existing component drivers.

This keeps switching the driver into the bridge in a more feasible manner.

Signed-off-by Jagan Teki <[email protected]>

---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 57 +++++++++++++++++--------
 1 file changed, 39 insertions(+), 18 deletions(-)

drivers/gpu/drm/exynos/exynos_drm_dsi.c +10 -11

index 1d777d8c1a83..90d2ce2c3055 100644
@@ -257,6 +257,7 @@ struct exynos_dsi {
 	struct drm_connector connector;
 	struct drm_panel *panel;
 	struct list_head bridge_chain;
+	struct drm_bridge bridge;
 	struct drm_bridge *out_bridge;
 	struct device *dev;
 

@@ -287,9 +288,9 @@ struct exynos_dsi {
 #define host_to_dsi(host) container_of(host, struct exynos_dsi, dsi_host)
 #define connector_to_dsi(c) container_of(c, struct exynos_dsi, connector)
 
-static inline struct exynos_dsi *encoder_to_dsi(struct drm_encoder *e)
+static inline struct exynos_dsi *bridge_to_dsi(struct drm_bridge *b)
 {
-	return container_of(e, struct exynos_dsi, encoder);
+	return container_of(b, struct exynos_dsi, bridge);
 }
 
 enum reg_idx {

@@ -1374,9 +1375,9 @@ static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi)
 	}
 }
 
-static void exynos_dsi_enable(struct drm_encoder *encoder)
+static void exynos_dsi_bridge_enable(struct drm_bridge *bridge)
 {
-	struct exynos_dsi *dsi = encoder_to_dsi(encoder);
+	struct exynos_dsi *dsi = bridge_to_dsi(bridge);
 	struct drm_bridge *iter;
 	int ret;
 

@@ -1429,9 +1430,9 @@ static void exynos_dsi_enable(struct drm_encoder *encoder)
 	pm_runtime_put(dsi->dev);
 }
 
-static void exynos_dsi_disable(struct drm_encoder *encoder)
+static void exynos_dsi_bridge_disable(struct drm_bridge *bridge)
 {
-	struct exynos_dsi *dsi = encoder_to_dsi(encoder);
+	struct exynos_dsi *dsi = bridge_to_dsi(bridge);
 	struct drm_bridge *iter;
 
 	if (!(dsi->state & DSIM_STATE_ENABLED))

@@ -1494,9 +1495,9 @@ static const struct drm_connector_helper_funcs exynos_dsi_connector_helper_funcs
 	.get_modes = exynos_dsi_get_modes,
 };
 
-static int exynos_dsi_create_connector(struct drm_encoder *encoder)
+static int exynos_dsi_create_connector(struct exynos_dsi *dsi)
 {
-	struct exynos_dsi *dsi = encoder_to_dsi(encoder);
+	struct drm_encoder *encoder = &dsi->encoder;
 	struct drm_connector *connector = &dsi->connector;
 	struct drm_device *drm = encoder->dev;
 	int ret;

@@ -1522,9 +1523,16 @@ static int exynos_dsi_create_connector(struct drm_encoder *encoder)
 	return 0;
 }
 
-static const struct drm_encoder_helper_funcs exynos_dsi_encoder_helper_funcs = {
-	.enable = exynos_dsi_enable,
-	.disable = exynos_dsi_disable,
+static int exynos_dsi_bridge_attach(struct drm_bridge *bridge,

drivers/gpu/drm/exynos/exynos_drm_dsi.c +2 -2

 MODULE_DEVICE_TABLE(of, exynos_dsi_of_match);

@@ -1543,7 +1551,7 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 		dsi->out_bridge = out_bridge;
 		list_splice_init(&encoder->bridge_chain, &dsi->bridge_chain);
 	} else {
-		int ret = exynos_dsi_create_connector(encoder);
+		int ret = exynos_dsi_create_connector(dsi);
 
 		if (ret) {
 			DRM_DEV_ERROR(dsi->dev,

@@ -1596,7 +1604,7 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
 
 	if (dsi->panel) {
 		mutex_lock(&drm->mode_config.mutex);
-		exynos_dsi_disable(&dsi->encoder);
+		exynos_dsi_bridge_disable(&dsi->bridge);
 		dsi->panel = NULL;
 		dsi->connector.status = connector_status_disconnected;
 		mutex_unlock(&drm->mode_config.mutex);

drivers/gpu/drm/exynos/exynos_drm_dsi.c +17 -3

+	if (ret) {
+		drm_encoder_cleanup(&dsi->encoder);
+		return ret;
+	}
+
 	in_bridge_node = of_graph_get_remote_node(dev->of_node, DSI_PORT_IN, 0);
 	if (in_bridge_node) {
 		in_bridge = of_drm_find_bridge(in_bridge_node);

@@ -1723,10 +1735,9 @@ static void exynos_dsi_unbind(struct device *dev, struct device *master,
 				void *data)
 {
 	struct exynos_dsi *dsi = dev_get_drvdata(dev);
-	struct drm_encoder *encoder = &dsi->encoder;
-
-	exynos_dsi_disable(encoder);
 
+	exynos_dsi_bridge_disable(&dsi->bridge);
+	drm_encoder_cleanup(&dsi->encoder);
 	mipi_dsi_host_unregister(&dsi->dsi_host);
 }
 

@@ -1821,6 +1832,12 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 
 	pm_runtime_enable(dev);
 
+	dsi->bridge.funcs = &exynos_dsi_bridge_funcs;
+	dsi->bridge.of_node = dev->of_node;
+	dsi->bridge.type = DRM_MODE_CONNECTOR_DSI;
+
+	drm_bridge_add(&dsi->bridge);
+
 	ret = component_add(dev, &exynos_dsi_component_ops);
 	if (ret)
 		goto err_disable_runtime;

@@ -1835,6 +1852,10 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 
 static int exynos_dsi_remove(struct platform_device *pdev)
 {
+	struct exynos_dsi *dsi = platform_get_drvdata(pdev);
+
+	drm_bridge_remove(&dsi->bridge);
+
 	pm_runtime_disable(&pdev->dev);
 
 	component_del(&pdev->dev, &exynos_dsi_component_ops);

Signature · 1 line

2.25.1

Now the exynos dsi driver supports bridge and legacy encoder.

In order to support drm_device in both the implementations it is required to preserve the drm_device in a private structure and use it accordingly.

1. encoder case, the bind will assign the drm_device into encoder->dev, so preserve the drm_device in the bind call.

2. bridge case, the drm_bridge_attach will assign the drm_device into bridge->dev, so preserve the drm_device in the bridge_attach function call.

Signed-off-by Jagan Teki <[email protected]>

Use drm_panel_bridge to replace manual panel and
bridge_chain handling code.

This makes the driver simpler to allow all components in the display pipeline to be treated as bridges by cleaning the way to generic connector handling.

Signed-off-by Jagan Teki <[email protected]>

---
  drivers/gpu/drm/exynos/exynos_drm_dsi.c | 167 ++++--------------------
  1 file changed, 23 insertions(+), 144 deletions(-)

drivers/gpu/drm/exynos/exynos_drm_dsi.c +23 -143

index d7d60aee465b..24f0b082ac6d 100644
@@ -254,9 +254,6 @@ struct exynos_dsi_driver_data {
  struct exynos_dsi {
  	struct drm_encoder encoder;
  	struct mipi_dsi_host dsi_host;
-	struct drm_connector connector;
-	struct drm_panel *panel;
-	struct list_head bridge_chain;
  	struct drm_bridge bridge;
  	struct drm_bridge *out_bridge;
  	struct drm_device *drm;

@@ -287,7 +284,6 @@ struct exynos_dsi {
  };
 
  #define host_to_dsi(host) container_of(host, struct exynos_dsi, dsi_host)
-#define connector_to_dsi(c) container_of(c, struct exynos_dsi, connector)
 
  static inline struct exynos_dsi *bridge_to_dsi(struct drm_bridge *b)
  {

@@ -1379,7 +1375,6 @@ static void exynos_dsi_unregister_te_irq(struct exynos_dsi *dsi)
  static void exynos_dsi_bridge_enable(struct drm_bridge *bridge)
  {
  	struct exynos_dsi *dsi = bridge_to_dsi(bridge);
-	struct drm_bridge *iter;
  	int ret;
 
  	if (dsi->state & DSIM_STATE_ENABLED)

@@ -1393,134 +1388,51 @@ static void exynos_dsi_bridge_enable(struct drm_bridge *bridge)
 
  	dsi->state |= DSIM_STATE_ENABLED;
 
-	if (dsi->panel) {
-		ret = drm_panel_prepare(dsi->panel);
-		if (ret < 0)
-			goto err_put_sync;
-	} else {
-		list_for_each_entry_reverse(iter, &dsi->bridge_chain,
-					    chain_node) {
-			if (iter->funcs->pre_enable)
-				iter->funcs->pre_enable(iter);
-		}
-	}
-
  	exynos_dsi_set_display_mode(dsi);
  	exynos_dsi_set_display_enable(dsi, true);
 
-	if (dsi->panel) {
-		ret = drm_panel_enable(dsi->panel);
-		if (ret < 0)
-			goto err_display_disable;
-	} else {
-		list_for_each_entry(iter, &dsi->bridge_chain, chain_node) {
-			if (iter->funcs->enable)
-				iter->funcs->enable(iter);
-		}
-	}
-
  	dsi->state |= DSIM_STATE_VIDOUT_AVAILABLE;
  	return;
-
-err_display_disable:
-	exynos_dsi_set_display_enable(dsi, false);
-	drm_panel_unprepare(dsi->panel);
-
-err_put_sync:
-	dsi->state &= ~DSIM_STATE_ENABLED;
-	pm_runtime_put(dsi->dev);
  }
 
  static void exynos_dsi_bridge_disable(struct drm_bridge *bridge)
  {
  	struct exynos_dsi *dsi = bridge_to_dsi(bridge);
-	struct drm_bridge *iter;
 
  	if (!(dsi->state & DSIM_STATE_ENABLED))
  		return;
 
  	dsi->state &= ~DSIM_STATE_VIDOUT_AVAILABLE;
 
-	drm_panel_disable(dsi->panel);
-
-	list_for_each_entry_reverse(iter, &dsi->bridge_chain, chain_node) {
-		if (iter->funcs->disable)
-			iter->funcs->disable(iter);
-	}
-
  	exynos_dsi_set_display_enable(dsi, false);
-	drm_panel_unprepare(dsi->panel);
-
-	list_for_each_entry(iter, &dsi->bridge_chain, chain_node) {
-		if (iter->funcs->post_disable)
-			iter->funcs->post_disable(iter);
-	}
 
  	dsi->state &= ~DSIM_STATE_ENABLED;
  	pm_runtime_put_sync(dsi->dev);
  }
 
-static enum drm_connector_status
-exynos_dsi_detect(struct drm_connector *connector, bool force)
-{
-	return connector->status;
-}
-
-static void exynos_dsi_connector_destroy(struct drm_connector *connector)
+static int exynos_dsi_panel_or_bridge(struct exynos_dsi *dsi,
+				      struct device_node *node)
  {
-	drm_connector_unregister(connector);
-	drm_connector_cleanup(connector);
-	connector->dev = NULL;
-}
-
-static const struct drm_connector_funcs exynos_dsi_connector_funcs = {
-	.detect = exynos_dsi_detect,
-	.fill_modes = drm_helper_probe_single_connector_modes,
-	.destroy = exynos_dsi_connector_destroy,
-	.reset = drm_atomic_helper_connector_reset,
-	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
-	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
-};
-
-static int exynos_dsi_get_modes(struct drm_connector *connector)
-{
-	struct exynos_dsi *dsi = connector_to_dsi(connector);
-
-	if (dsi->panel)
-		return drm_panel_get_modes(dsi->panel, connector);
-
-	return 0;
-}
+	struct drm_bridge *panel_bridge;
+	struct drm_panel *panel;
 
-static const struct drm_connector_helper_funcs exynos_dsi_connector_helper_funcs = {
-	.get_modes = exynos_dsi_get_modes,
-};
+	panel_bridge = of_drm_find_bridge(node);
+	if (!panel_bridge) {
+		panel = of_drm_find_panel(node);
+		if (!IS_ERR(panel)) {
+			panel_bridge = drm_panel_bridge_add(panel);
+			if (IS_ERR(panel_bridge))
+				return PTR_ERR(panel_bridge);
+		}
+	}
 
-static int exynos_dsi_create_connector(struct exynos_dsi *dsi,
-				       struct drm_device *drm)
-{
-	struct drm_encoder *encoder = &dsi->encoder;
-	struct drm_connector *connector = &dsi->connector;
-	int ret;
+	of_node_put(node);
 
-	connector->polled = DRM_CONNECTOR_POLL_HPD;
+	dsi->out_bridge = panel_bridge;
 
-	ret = drm_connector_init(drm, connector, &exynos_dsi_connector_funcs,
-				 DRM_MODE_CONNECTOR_DSI);
-	if (ret) {
-		DRM_DEV_ERROR(dsi->dev,
-			      "Failed to initialize connector with drm\n");
-		return ret;
-	}
+	if (!dsi->out_bridge)
+		return -EPROBE_DEFER;
 
-	connector->status = connector_status_disconnected;
-	drm_connector_helper_add(connector, &exynos_dsi_connector_helper_funcs);
-	drm_connector_attach_encoder(connector, encoder);
-	if (!drm->registered)
-		return 0;
-
-	connector->funcs->reset(connector);
-	drm_connector_register(connector);
  	return 0;
  }
 

@@ -1531,7 +1443,8 @@ static int exynos_dsi_bridge_attach(struct drm_bridge *bridge,
 
  	dsi->drm = bridge->dev;
 
-	return 0;
+	return drm_bridge_attach(bridge->encoder, dsi->out_bridge, bridge,
+				 flags);
  }
 
  static const struct drm_bridge_funcs exynos_dsi_bridge_funcs = {

@@ -1546,32 +1459,12 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
  				  struct mipi_dsi_device *device)
  {
  	struct exynos_dsi *dsi = host_to_dsi(host);
-	struct drm_encoder *encoder = &dsi->encoder;
  	struct drm_device *drm = dsi->drm;
-	struct drm_bridge *out_bridge;
-
-	out_bridge  = of_drm_find_bridge(device->dev.of_node);
-	if (out_bridge) {
-		drm_bridge_attach(encoder, out_bridge, NULL, 0);
-		dsi->out_bridge = out_bridge;
-		list_splice_init(&encoder->bridge_chain, &dsi->bridge_chain);
-	} else {
-		int ret = exynos_dsi_create_connector(dsi, drm);
-
-		if (ret) {
-			DRM_DEV_ERROR(dsi->dev,
-				      "failed to create connector ret = %d\n",
-				      ret);
-			drm_encoder_cleanup(encoder);
-			return ret;
-		}
+	int ret;
 
-		dsi->panel = of_drm_find_panel(device->dev.of_node);
-		if (IS_ERR(dsi->panel))
-			dsi->panel = NULL;
-		else
-			dsi->connector.status = connector_status_connected;
-	}
+	ret = exynos_dsi_panel_or_bridge(dsi, device->dev.of_node);
+	if (ret)
+		return ret;
 
  	/*
  	 * This is a temporary solution and should be made by more generic way.

@@ -1607,19 +1500,6 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
  	struct exynos_dsi *dsi = host_to_dsi(host);
  	struct drm_device *drm = dsi->drm;
 
-	if (dsi->panel) {
-		mutex_lock(&drm->mode_config.mutex);
-		exynos_dsi_bridge_disable(&dsi->bridge);
-		dsi->panel = NULL;
-		dsi->connector.status = connector_status_disconnected;
-		mutex_unlock(&drm->mode_config.mutex);
-	} else {
-		if (dsi->out_bridge->funcs->detach)
-			dsi->out_bridge->funcs->detach(dsi->out_bridge);
-		dsi->out_bridge = NULL;
-		INIT_LIST_HEAD(&dsi->bridge_chain);
-	}
-
  	if (drm->mode_config.poll_enabled)
  		drm_kms_helper_hotplug_event(drm);
 

Quoted 8 lines · Jagan Teki

Use drm_panel_bridge to replace manual panel and
bridge_chain handling code.

This makes the driver simpler to allow all components in the display pipeline to be treated as bridges by cleaning the way to generic connector handling.

Signed-off-by Jagan Teki <[email protected]>

This breaks Exysos DSI driver operation (Trats board worked fine with only patches 1-2):

[    2.540066] exynos4-fb 11c00000.fimd: Adding to iommu group 0
[    2.554733] OF: graph: no port node found in /soc/fimd@11c00000
[    2.602819] [drm] Exynos DRM: using 11c00000.fimd device for DMA
mapping operations
[    2.609649] exynos-drm exynos-drm: bound 11c00000.fimd (ops
fimd_component_ops)
[    2.632558] exynos-drm exynos-drm: failed to bind 11c80000.dsi (ops
exynos_dsi_component_ops): -22
[    2.642263] exynos-drm exynos-drm: master bind failed: -22
[    2.651017] exynos-drm: probe of exynos-drm failed with error -22

Quoted 12 lines · Marek Szyprowski

Quoted 8 lines · Jagan Teki

Use drm_panel_bridge to replace manual panel and
bridge_chain handling code.

This makes the driver simpler to allow all components in the display pipeline to be treated as bridges by cleaning the way to generic connector handling.

Signed-off-by Jagan Teki <[email protected]>

This breaks Exysos DSI driver operation (Trats board worked fine with only patches 1-2):

Quoted 10 lines

[    2.540066] exynos4-fb 11c00000.fimd: Adding to iommu group 0
[    2.554733] OF: graph: no port node found in /soc/fimd@11c00000
[    2.602819] [drm] Exynos DRM: using 11c00000.fimd device for DMA
mapping operations
[    2.609649] exynos-drm exynos-drm: bound 11c00000.fimd (ops
fimd_component_ops)
[    2.632558] exynos-drm exynos-drm: failed to bind 11c80000.dsi (ops
exynos_dsi_component_ops): -22
[    2.642263] exynos-drm exynos-drm: master bind failed: -22
[    2.651017] exynos-drm: probe of exynos-drm failed with error -22

Thanks for testing it.

Can you check Squash of 3,4 or 3,4,5 will work or not?

Quoted 24 lines · Jagan Teki

Quoted 22 lines · Marek Szyprowski

Quoted 8 lines · Jagan Teki

Use drm_panel_bridge to replace manual panel and
bridge_chain handling code.

This makes the driver simpler to allow all components in the display pipeline to be treated as bridges by cleaning the way to generic connector handling.

Signed-off-by Jagan Teki <[email protected]>

This breaks Exysos DSI driver operation (Trats board worked fine with only patches 1-2):

[    2.540066] exynos4-fb 11c00000.fimd: Adding to iommu group 0
[    2.554733] OF: graph: no port node found in /soc/fimd@11c00000
[    2.602819] [drm] Exynos DRM: using 11c00000.fimd device for DMA
mapping operations
[    2.609649] exynos-drm exynos-drm: bound 11c00000.fimd (ops
fimd_component_ops)
[    2.632558] exynos-drm exynos-drm: failed to bind 11c80000.dsi (ops
exynos_dsi_component_ops): -22
[    2.642263] exynos-drm exynos-drm: master bind failed: -22
[    2.651017] exynos-drm: probe of exynos-drm failed with error -22

Quoted 3 lines

Thanks for testing it.

Can you check Squash of 3,4 or 3,4,5 will work or not?

I've check both sets: 1-4 and 1-5 and none of them works. The result is same as above. If I remember correctly, last time when I played with that code, there was a problem with DRM core calling bridge ops in different order than when they are used by the Exynos DSI driver.

Now the exynos dsi driver is fully aware of bridge handling, so get the display mode from bridge, mode_set API instead of legacy encoder crtc.

This makes bridge usage more efficient instead of relying on encoder stack.

Add mode_set in drm_bridge_funcs.

Signed-off-by Jagan Teki <[email protected]>

---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

drivers/gpu/drm/exynos/exynos_drm_dsi.c +12 -1

index d828be07c325..99a1b8c22313 100644
@@ -275,6 +275,7 @@ struct exynos_dsi {
 	u32 format;
 
 	int state;
+	struct drm_display_mode mode;
 	struct drm_property *brightness;
 	struct completion completed;
 

@@ -881,7 +882,7 @@ static int exynos_dsi_init_link(struct exynos_dsi *dsi)
 
 static void exynos_dsi_set_display_mode(struct exynos_dsi *dsi)
 {
-	struct drm_display_mode *m = &dsi->encoder.crtc->state->adjusted_mode;
+	struct drm_display_mode *m = &dsi->mode;
 	unsigned int num_bits_resol = dsi->driver_data->num_bits_resol;
 	u32 reg;
 

@@ -1411,6 +1412,15 @@ static void exynos_dsi_bridge_disable(struct drm_bridge *bridge)
 	pm_runtime_put_sync(dsi->dev);
 }
 
+static void exynos_dsi_bridge_mode_set(struct drm_bridge *bridge,
+				       const struct drm_display_mode *mode,
+				       const struct drm_display_mode *adjusted_mode)
+{
+	struct exynos_dsi *dsi = bridge_to_dsi(bridge);
+
+	drm_mode_copy(&dsi->mode, adjusted_mode);
+}
+
 static int exynos_dsi_panel_or_bridge(struct exynos_dsi *dsi,
 				      struct device_node *node)
 {

@@ -1451,6 +1461,7 @@ static int exynos_dsi_bridge_attach(struct drm_bridge *bridge,
 static const struct drm_bridge_funcs exynos_dsi_bridge_funcs = {
 	.enable = exynos_dsi_bridge_enable,
 	.disable = exynos_dsi_bridge_disable,
+	.mode_set = exynos_dsi_bridge_mode_set,
 	.attach = exynos_dsi_bridge_attach,
 };
 

Signature · 1 line

2.25.1

Now the exynos dsi driver is fully aware of bridge handling, so get the display mode from bridge, mode_set API instead of legacy encoder crtc.

This makes bridge usage more efficient instead of relying on encoder stack.

Add mode_set in drm_bridge_funcs.

Signed-off-by Jagan Teki <[email protected]>

---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

drivers/gpu/drm/exynos/exynos_drm_dsi.c +11 -1

index d828be07c325..99a1b8c22313 100644
@@ -275,6 +275,7 @@ struct exynos_dsi {
        u32 format;
 
        int state;
+       struct drm_display_mode mode;
        struct drm_property *brightness;
        struct completion completed;
 

@@ -881,7 +882,7 @@ static int exynos_dsi_init_link(struct exynos_dsi *dsi)
 
 static void exynos_dsi_set_display_mode(struct exynos_dsi *dsi)
 {
-       struct drm_display_mode *m = &dsi->encoder.crtc->state->adjusted_mode;
+       struct drm_display_mode *m = &dsi->mode;
        unsigned int num_bits_resol = dsi->driver_data->num_bits_resol;
        u32 reg;
 

@@ -1411,6 +1412,15 @@ static void exynos_dsi_bridge_disable(struct drm_bridge *bridge)
        pm_runtime_put_sync(dsi->dev);
 }
 
+static void exynos_dsi_bridge_mode_set(struct drm_bridge *bridge,
+                                      const struct drm_display_mode *mode,
+                                      const struct drm_display_mode *adjusted_mode)
+{
+       struct exynos_dsi *dsi = bridge_to_dsi(bridge);
+
+       drm_mode_copy(&dsi->mode, adjusted_mode);
+}
+
 static int exynos_dsi_panel_or_bridge(struct exynos_dsi *dsi,
                                      struct device_node *node)
 {

drivers/gpu/drm/exynos/exynos_drm_dsi.c +0 -1

        .attach = exynos_dsi_bridge_attach,
 };
 
--

2.25.1

Hey Jagan,

Quoted 53 lines · Jagan Teki

Now the exynos dsi driver is fully aware of bridge handling, so get the display mode from bridge, mode_set API instead of legacy encoder crtc.

This makes bridge usage more efficient instead of relying on encoder stack.

Add mode_set in drm_bridge_funcs.

Signed-off-by Jagan Teki <[email protected]>

---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

drivers/gpu/drm/exynos/exynos_drm_dsi.c +11 -1

index d828be07c325..99a1b8c22313 100644
@@ -275,6 +275,7 @@ struct exynos_dsi {
        u32 format;
 
        int state;
+       struct drm_display_mode mode;
        struct drm_property *brightness;
        struct completion completed;
 

@@ -881,7 +882,7 @@ static int exynos_dsi_init_link(struct exynos_dsi *dsi)
 
 static void exynos_dsi_set_display_mode(struct exynos_dsi *dsi)
 {
-       struct drm_display_mode *m = &dsi->encoder.crtc->state->adjusted_mode;
+       struct drm_display_mode *m = &dsi->mode;
        unsigned int num_bits_resol = dsi->driver_data->num_bits_resol;
        u32 reg;
 

@@ -1411,6 +1412,15 @@ static void exynos_dsi_bridge_disable(struct drm_bridge *bridge)
        pm_runtime_put_sync(dsi->dev);
 }
 
+static void exynos_dsi_bridge_mode_set(struct drm_bridge *bridge,
+                                      const struct drm_display_mode *mode,
+                                      const struct drm_display_mode *adjusted_mode)
+{
+       struct exynos_dsi *dsi = bridge_to_dsi(bridge);
+
+       drm_mode_copy(&dsi->mode, adjusted_mode);
+}
+
 static int exynos_dsi_panel_or_bridge(struct exynos_dsi *dsi,
                                      struct device_node *node)
 {

drivers/gpu/drm/exynos/exynos_drm_dsi.c static int exynos_dsi_bridge_attach(struct drm_bridge *bridge, · line 1464

Quoted 4 lines

drivers/gpu/drm/exynos/exynos_drm_dsi.c +1 -0

@@ -1451,6 +1461,7 @@ static int exynos_dsi_bridge_attach(struct drm_bridge *bridge,
 static const struct drm_bridge_funcs exynos_dsi_bridge_funcs = {
        .enable = exynos_dsi_bridge_enable,
        .disable = exynos_dsi_bridge_disable,
+       .mode_set = exynos_dsi_bridge_mode_set,

As far as I understand it, .enable(), .disable() &.mode_set() are deprecated[1] and should be replaced by atomic_enable(), atomic_disable() & atomic_enable() respectively.

[1] https://lore.kernel.org/dri-devel/[email protected]/

Exynos DSI driver is actually a Samsung MIPI DSIM bridge IP which is also used in i.MX8MM platforms.

Right now the existing driver has some exynos drm specific code bases like te_irq, crtc and component_ops.

In order to switch this driver into a common bridge driver We can see 2 options to handle the exynos specific code.

A. Drop the component_ops, and rework other specifics.
This may lead to more foundation work as it requires more changes in exynos drm drivers stack.

B. Handle the exynos specifics via driver data, and make
the common bridge work in different platforms and plan for option A in future.

So, this patch is trying to add option B) changes to handle exynos specifics via driver_data.

Signed-off-by Jagan Teki <[email protected]>

---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 37 +++++++++++++++++++------
 1 file changed, 29 insertions(+), 8 deletions(-)

drivers/gpu/drm/exynos/exynos_drm_dsi.c +29 -8

index 99a1b8c22313..53d878d4d2d7 100644
@@ -250,6 +250,7 @@ struct exynos_dsi_driver_data {
 	unsigned int wait_for_reset;
 	unsigned int num_bits_resol;
 	const unsigned int *reg_values;
+	bool exynos_specific;
 };
 
 struct exynos_dsi {

@@ -459,6 +460,7 @@ static const struct exynos_dsi_driver_data exynos3_dsi_driver_data = {
 	.wait_for_reset = 1,
 	.num_bits_resol = 11,
 	.reg_values = reg_values,
+	.exynos_specific = true,
 };
 
 static const struct exynos_dsi_driver_data exynos4_dsi_driver_data = {

@@ -471,6 +473,7 @@ static const struct exynos_dsi_driver_data exynos4_dsi_driver_data = {
 	.wait_for_reset = 1,
 	.num_bits_resol = 11,
 	.reg_values = reg_values,
+	.exynos_specific = true,
 };
 
 static const struct exynos_dsi_driver_data exynos5_dsi_driver_data = {

@@ -481,6 +484,7 @@ static const struct exynos_dsi_driver_data exynos5_dsi_driver_data = {
 	.wait_for_reset = 1,
 	.num_bits_resol = 11,
 	.reg_values = reg_values,
+	.exynos_specific = true,
 };
 
 static const struct exynos_dsi_driver_data exynos5433_dsi_driver_data = {

@@ -492,6 +496,7 @@ static const struct exynos_dsi_driver_data exynos5433_dsi_driver_data = {
 	.wait_for_reset = 0,
 	.num_bits_resol = 12,
 	.reg_values = exynos5433_reg_values,
+	.exynos_specific = true,
 };
 
 static const struct exynos_dsi_driver_data exynos5422_dsi_driver_data = {

@@ -503,6 +508,7 @@ static const struct exynos_dsi_driver_data exynos5422_dsi_driver_data = {
 	.wait_for_reset = 1,
 	.num_bits_resol = 12,
 	.reg_values = exynos5422_reg_values,
+	.exynos_specific = true,
 };
 
 static const struct of_device_id exynos_dsi_of_match[] = {

@@ -1484,7 +1490,8 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 	 * If attached panel device is for command mode one, dsi should register
 	 * TE interrupt handler.
 	 */
-	if (!(device->mode_flags & MIPI_DSI_MODE_VIDEO)) {
+	if (dsi->driver_data->exynos_specific &&
+	    !(device->mode_flags & MIPI_DSI_MODE_VIDEO)) {
 		int ret = exynos_dsi_register_te_irq(dsi, &device->dev);
 		if (ret)
 			return ret;

@@ -1495,8 +1502,9 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 	dsi->lanes = device->lanes;
 	dsi->format = device->format;
 	dsi->mode_flags = device->mode_flags;
-	exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode =
-			!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO);
+	if (dsi->driver_data->exynos_specific)
+		exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode =
+					    !(dsi->mode_flags & MIPI_DSI_MODE_VIDEO);
 
 	mutex_unlock(&drm->mode_config.mutex);
 

@@ -1515,7 +1523,8 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
 	if (drm->mode_config.poll_enabled)
 		drm_kms_helper_hotplug_event(drm);
 
-	exynos_dsi_unregister_te_irq(dsi);
+	if (dsi->driver_data->exynos_specific)
+		exynos_dsi_unregister_te_irq(dsi);
 
 	return 0;
 }

@@ -1737,6 +1746,15 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	if (!dsi->driver_data->exynos_specific) {
+		ret = mipi_dsi_host_register(&dsi->dsi_host);
+		if (ret) {
+			dev_err(dev, "failed to register mipi dsi host: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
 	platform_set_drvdata(pdev, dsi);
 
 	pm_runtime_enable(dev);

@@ -1747,9 +1765,11 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 
 	drm_bridge_add(&dsi->bridge);
 
-	ret = component_add(dev, &exynos_dsi_component_ops);
-	if (ret)
-		goto err_disable_runtime;
+	if (dsi->driver_data->exynos_specific) {
+		ret = component_add(dev, &exynos_dsi_component_ops);
+		if (ret)
+			goto err_disable_runtime;
+	}
 
 	return 0;
 

@@ -1767,7 +1787,8 @@ static int exynos_dsi_remove(struct platform_device *pdev)
 
 	pm_runtime_disable(&pdev->dev);
 
-	component_del(&pdev->dev, &exynos_dsi_component_ops);
+	if (dsi->driver_data->exynos_specific)
+		component_del(&pdev->dev, &exynos_dsi_component_ops);
 
 	return 0;
 }

Signature · 1 line

2.25.1

Quoted 13 lines · Jagan Teki

Exynos DSI driver is actually a Samsung MIPI DSIM bridge IP which is also used in i.MX8MM platforms.

Right now the existing driver has some exynos drm specific code bases like te_irq, crtc and component_ops.

In order to switch this driver into a common bridge driver We can see 2 options to handle the exynos specific code.

A. Drop the component_ops, and rework other specifics.
This may lead to more foundation work as it requires more changes in exynos drm drivers stack.

Quoted 6 lines

B. Handle the exynos specifics via driver data, and make
the common bridge work in different platforms and plan for option A in future.

So, this patch is trying to add option B) changes to handle exynos specifics via driver_data.

We really should find someone that has the time, energy, knowledge and hardware that can include device_link support once anf for all for bridges.
Then we would avoid hacks like this.

I see no other options at the moment, but look forward for a better solution.

Quoted 13 lines

Exynos DSI driver is actually a Samsung MIPI DSIM bridge IP which is also used in i.MX8MM platforms.

Right now the existing driver has some exynos drm specific code bases like te_irq, crtc and component_ops.

In order to switch this driver into a common bridge driver We can see 2 options to handle the exynos specific code.

A. Drop the component_ops, and rework other specifics.
This may lead to more foundation work as it requires more changes in exynos drm drivers stack.

Quoted 6 lines

B. Handle the exynos specifics via driver data, and make
the common bridge work in different platforms and plan for option A in future.

So, this patch is trying to add option B) changes to handle exynos specifics via driver_data.

We really should find someone that has the time, energy, knowledge and hardware that can include device_link support once anf for all for bridges.
Then we would avoid hacks like this.

I see no other options at the moment, but look forward for a better solution.

Sam

Quoted 134 lines

Signed-off-by Jagan Teki <[email protected]>

---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 37 +++++++++++++++++++------
 1 file changed, 29 insertions(+), 8 deletions(-)

drivers/gpu/drm/exynos/exynos_drm_dsi.c +29 -8

index 99a1b8c22313..53d878d4d2d7 100644
@@ -250,6 +250,7 @@ struct exynos_dsi_driver_data {
 	unsigned int wait_for_reset;
 	unsigned int num_bits_resol;
 	const unsigned int *reg_values;
+	bool exynos_specific;
 };
 
 struct exynos_dsi {

@@ -459,6 +460,7 @@ static const struct exynos_dsi_driver_data exynos3_dsi_driver_data = {
 	.wait_for_reset = 1,
 	.num_bits_resol = 11,
 	.reg_values = reg_values,
+	.exynos_specific = true,
 };
 
 static const struct exynos_dsi_driver_data exynos4_dsi_driver_data = {

@@ -471,6 +473,7 @@ static const struct exynos_dsi_driver_data exynos4_dsi_driver_data = {
 	.wait_for_reset = 1,
 	.num_bits_resol = 11,
 	.reg_values = reg_values,
+	.exynos_specific = true,
 };
 
 static const struct exynos_dsi_driver_data exynos5_dsi_driver_data = {

@@ -481,6 +484,7 @@ static const struct exynos_dsi_driver_data exynos5_dsi_driver_data = {
 	.wait_for_reset = 1,
 	.num_bits_resol = 11,
 	.reg_values = reg_values,
+	.exynos_specific = true,
 };
 
 static const struct exynos_dsi_driver_data exynos5433_dsi_driver_data = {

@@ -492,6 +496,7 @@ static const struct exynos_dsi_driver_data exynos5433_dsi_driver_data = {
 	.wait_for_reset = 0,
 	.num_bits_resol = 12,
 	.reg_values = exynos5433_reg_values,
+	.exynos_specific = true,
 };
 
 static const struct exynos_dsi_driver_data exynos5422_dsi_driver_data = {

@@ -503,6 +508,7 @@ static const struct exynos_dsi_driver_data exynos5422_dsi_driver_data = {
 	.wait_for_reset = 1,
 	.num_bits_resol = 12,
 	.reg_values = exynos5422_reg_values,
+	.exynos_specific = true,
 };
 
 static const struct of_device_id exynos_dsi_of_match[] = {

@@ -1484,7 +1490,8 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 	 * If attached panel device is for command mode one, dsi should register
 	 * TE interrupt handler.
 	 */
-	if (!(device->mode_flags & MIPI_DSI_MODE_VIDEO)) {
+	if (dsi->driver_data->exynos_specific &&
+	    !(device->mode_flags & MIPI_DSI_MODE_VIDEO)) {
 		int ret = exynos_dsi_register_te_irq(dsi, &device->dev);
 		if (ret)
 			return ret;

@@ -1495,8 +1502,9 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 	dsi->lanes = device->lanes;
 	dsi->format = device->format;
 	dsi->mode_flags = device->mode_flags;
-	exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode =
-			!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO);
+	if (dsi->driver_data->exynos_specific)
+		exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode =
+					    !(dsi->mode_flags & MIPI_DSI_MODE_VIDEO);
 
 	mutex_unlock(&drm->mode_config.mutex);
 

@@ -1515,7 +1523,8 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
 	if (drm->mode_config.poll_enabled)
 		drm_kms_helper_hotplug_event(drm);
 
-	exynos_dsi_unregister_te_irq(dsi);
+	if (dsi->driver_data->exynos_specific)
+		exynos_dsi_unregister_te_irq(dsi);
 
 	return 0;
 }

@@ -1737,6 +1746,15 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	if (!dsi->driver_data->exynos_specific) {
+		ret = mipi_dsi_host_register(&dsi->dsi_host);
+		if (ret) {
+			dev_err(dev, "failed to register mipi dsi host: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
 	platform_set_drvdata(pdev, dsi);
 
 	pm_runtime_enable(dev);

@@ -1747,9 +1765,11 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 
 	drm_bridge_add(&dsi->bridge);
 
-	ret = component_add(dev, &exynos_dsi_component_ops);
-	if (ret)
-		goto err_disable_runtime;
+	if (dsi->driver_data->exynos_specific) {
+		ret = component_add(dev, &exynos_dsi_component_ops);
+		if (ret)
+			goto err_disable_runtime;
+	}
 
 	return 0;
 

@@ -1767,7 +1787,8 @@ static int exynos_dsi_remove(struct platform_device *pdev)
 
 	pm_runtime_disable(&pdev->dev);
 
-	component_del(&pdev->dev, &exynos_dsi_component_ops);
+	if (dsi->driver_data->exynos_specific)
+		component_del(&pdev->dev, &exynos_dsi_component_ops);
 
 	return 0;
 }

Signature · 1 line

2.25.1

21. 7. 26. 오전 2:25에 Sam Ravnborg 이(가) 쓴 글:

Quoted 21 lines · Jagan Teki

Quoted 13 lines

Exynos DSI driver is actually a Samsung MIPI DSIM bridge IP which is also used in i.MX8MM platforms.

Right now the existing driver has some exynos drm specific code bases like te_irq, crtc and component_ops.

In order to switch this driver into a common bridge driver We can see 2 options to handle the exynos specific code.

A. Drop the component_ops, and rework other specifics.
This may lead to more foundation work as it requires more changes in exynos drm drivers stack.

Quoted 6 lines

B. Handle the exynos specifics via driver data, and make
the common bridge work in different platforms and plan for option A in future.

So, this patch is trying to add option B) changes to handle exynos specifics via driver_data.

Quoted 7 lines

We really should find someone that has the time, energy, knowledge and hardware that can include device_link support once anf for all for bridges.
Then we would avoid hacks like this.

I see no other options at the moment, but look forward for a better solution.

I'm not sure that it's correct to share this mipi dsi driver with I.MX8MM SoC even though it's a same IP because this MIPI DSI device isn't peripheral device but in SoC.

It would mean that Exynos MIPI DSI device depends on SoC architecture, and Exynos and I.MX series are totally different SoC. So if we share the same driver for the MIPI DSI device then many things we didn't predict may happen in the future.

I don't want to make Jagan's efforts
in vain for the community but clarify whether this is correct way or not. If this is only the way we have to go then we could more focus on actual solution not such hack. Impossible work with Jagan alone I think.

So let's get started with a question,
- Is MIPI-DSI bridge device or Encoder device? I think that MIPI-DSI is a Encoder device managed by atomic KMS. If MIPI-DSI should be handled as bridge device then does now drm bridge framework provide everything to share one driver with one more SoC? I mean something that drm bridge has to consider for such driver support, which is shared with one more SoC.

And Display mode - VIDEO and COMMAND mode - is generic type of MIPI DSI, and also componentised subsystem is a generic solution to resolve probing order issue not Exynos specific feature. These are driver specific ones not Exynos SoC I think. As SoC specific things should be considered, I think MIPI DSI Driver - interrupt handler and probing order things are really specific to device driver - should be separated but we could share the control part of the device.

I was busy with other projects so didn't care of Linux DRM world so there may be my missing something.

Quoted 130 lines

Quoted 130 lines

Signed-off-by Jagan Teki <[email protected]>

---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 37 +++++++++++++++++++------
 1 file changed, 29 insertions(+), 8 deletions(-)

drivers/gpu/drm/exynos/exynos_drm_dsi.c +29 -8

index 99a1b8c22313..53d878d4d2d7 100644
@@ -250,6 +250,7 @@ struct exynos_dsi_driver_data {
 	unsigned int wait_for_reset;
 	unsigned int num_bits_resol;
 	const unsigned int *reg_values;
+	bool exynos_specific;
 };
 
 struct exynos_dsi {

@@ -459,6 +460,7 @@ static const struct exynos_dsi_driver_data exynos3_dsi_driver_data = {
 	.wait_for_reset = 1,
 	.num_bits_resol = 11,
 	.reg_values = reg_values,
+	.exynos_specific = true,
 };
 
 static const struct exynos_dsi_driver_data exynos4_dsi_driver_data = {

@@ -471,6 +473,7 @@ static const struct exynos_dsi_driver_data exynos4_dsi_driver_data = {
 	.wait_for_reset = 1,
 	.num_bits_resol = 11,
 	.reg_values = reg_values,
+	.exynos_specific = true,
 };
 
 static const struct exynos_dsi_driver_data exynos5_dsi_driver_data = {

@@ -481,6 +484,7 @@ static const struct exynos_dsi_driver_data exynos5_dsi_driver_data = {
 	.wait_for_reset = 1,
 	.num_bits_resol = 11,
 	.reg_values = reg_values,
+	.exynos_specific = true,
 };
 
 static const struct exynos_dsi_driver_data exynos5433_dsi_driver_data = {

@@ -492,6 +496,7 @@ static const struct exynos_dsi_driver_data exynos5433_dsi_driver_data = {
 	.wait_for_reset = 0,
 	.num_bits_resol = 12,
 	.reg_values = exynos5433_reg_values,
+	.exynos_specific = true,
 };
 
 static const struct exynos_dsi_driver_data exynos5422_dsi_driver_data = {

@@ -503,6 +508,7 @@ static const struct exynos_dsi_driver_data exynos5422_dsi_driver_data = {
 	.wait_for_reset = 1,
 	.num_bits_resol = 12,
 	.reg_values = exynos5422_reg_values,
+	.exynos_specific = true,
 };
 
 static const struct of_device_id exynos_dsi_of_match[] = {

@@ -1484,7 +1490,8 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 	 * If attached panel device is for command mode one, dsi should register
 	 * TE interrupt handler.
 	 */
-	if (!(device->mode_flags & MIPI_DSI_MODE_VIDEO)) {
+	if (dsi->driver_data->exynos_specific &&
+	    !(device->mode_flags & MIPI_DSI_MODE_VIDEO)) {
 		int ret = exynos_dsi_register_te_irq(dsi, &device->dev);
 		if (ret)
 			return ret;

@@ -1495,8 +1502,9 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 	dsi->lanes = device->lanes;
 	dsi->format = device->format;
 	dsi->mode_flags = device->mode_flags;
-	exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode =
-			!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO);
+	if (dsi->driver_data->exynos_specific)
+		exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode =
+					    !(dsi->mode_flags & MIPI_DSI_MODE_VIDEO);
 
 	mutex_unlock(&drm->mode_config.mutex);
 

@@ -1515,7 +1523,8 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
 	if (drm->mode_config.poll_enabled)
 		drm_kms_helper_hotplug_event(drm);
 
-	exynos_dsi_unregister_te_irq(dsi);
+	if (dsi->driver_data->exynos_specific)
+		exynos_dsi_unregister_te_irq(dsi);
 
 	return 0;
 }

@@ -1737,6 +1746,15 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	if (!dsi->driver_data->exynos_specific) {
+		ret = mipi_dsi_host_register(&dsi->dsi_host);
+		if (ret) {
+			dev_err(dev, "failed to register mipi dsi host: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
 	platform_set_drvdata(pdev, dsi);
 
 	pm_runtime_enable(dev);

@@ -1747,9 +1765,11 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 
 	drm_bridge_add(&dsi->bridge);
 
-	ret = component_add(dev, &exynos_dsi_component_ops);
-	if (ret)
-		goto err_disable_runtime;
+	if (dsi->driver_data->exynos_specific) {
+		ret = component_add(dev, &exynos_dsi_component_ops);
+		if (ret)
+			goto err_disable_runtime;
+	}
 
 	return 0;
 

@@ -1767,7 +1787,8 @@ static int exynos_dsi_remove(struct platform_device *pdev)
 
 	pm_runtime_disable(&pdev->dev);
 
-	component_del(&pdev->dev, &exynos_dsi_component_ops);
+	if (dsi->driver_data->exynos_specific)
+		component_del(&pdev->dev, &exynos_dsi_component_ops);
 
 	return 0;
 }

Hi Inki,

Quoted 30 lines · Inki Dae

21. 7. 26. 오전 2:25에 Sam Ravnborg 이(가) 쓴 글:

Quoted 21 lines · Jagan Teki

Quoted 13 lines

Exynos DSI driver is actually a Samsung MIPI DSIM bridge IP which is also used in i.MX8MM platforms.

Right now the existing driver has some exynos drm specific code bases like te_irq, crtc and component_ops.

In order to switch this driver into a common bridge driver We can see 2 options to handle the exynos specific code.

A. Drop the component_ops, and rework other specifics.
This may lead to more foundation work as it requires more changes in exynos drm drivers stack.

Quoted 6 lines

B. Handle the exynos specifics via driver data, and make
the common bridge work in different platforms and plan for option A in future.

So, this patch is trying to add option B) changes to handle exynos specifics via driver_data.

Quoted 7 lines

We really should find someone that has the time, energy, knowledge and hardware that can include device_link support once anf for all for bridges.
Then we would avoid hacks like this.

I see no other options at the moment, but look forward for a better solution.

Quoted 8 lines

I'm not sure that it's correct to share this mipi dsi driver with I.MX8MM SoC even though it's a same IP because this MIPI DSI device isn't peripheral device but in SoC.

It would mean that Exynos MIPI DSI device depends on SoC architecture, and Exynos and I.MX series are totally different SoC. So if we share the same driver for the MIPI DSI device then many things we didn't predict may happen in the future.

Isn't that true for external components true thought ? Any driver shared by multiple systems will face this issue, where it will be developed with some use cases in mind, and regressions may happen when the driver is then extended to support other use cases not required for the original platform.

In general we don't want multiple drivers for the same IP core unless there are valid technical reasons for that. It's the whole point of the device tree, being able to describe how IP cores are integrated, so that code can be reused across platforms. Of course, integration differences between SoCs can sometimes vary wildly and require some amount of glue code.

Quoted 6 lines

I don't want to make Jagan's efforts
in vain for the community but clarify whether this is correct way or not. If this is only the way we have to go then we could more focus on actual solution not such hack. Impossible work with Jagan alone I think.

I do agree that we need more correct solutions and less hacks in general :-) The issues faced on Exynos also exist on other platforms, so it would be much better to solve them well once that duplicating implementations with less test coverage and reviews. There have been efforts in the past to address some of these issues, which have resulted in solutions such as the component framework. However, I'd argued that

we've never taken it to the last step, and have always stopped with half solutions. The component framework, for instance, is painful to use, and the handling of .remove() in most drivers is completely broken because of that (not just because of that though, we have issues in the DRM core that make hot-unplug just impossible to handle safetly).

Quoted 8 lines

So let's get started with a question,
- Is MIPI-DSI bridge device or Encoder device? I think that MIPI-DSI is a Encoder device managed by atomic KMS. If MIPI-DSI should be handled as bridge device then does now drm bridge framework provide everything to share one driver with one more SoC? I mean something that drm bridge has to consider for such driver support, which is shared with one more SoC.

The DRM "encoder" concept was a bit of a historical mistake that we are stuck with as drm_encoder is exposed to userspace. It comes from a time where nobody was envisioning chaining multiple encoders. DRM is moving to modelling every component after the CRTC as a bridge. This brings

much more flexibility, and in that model, the drm_encoder becomes more or less a stub.

The DRM bridge API has been extended in the past to support more features, and if anything is still missing that makes it difficult to move away from drm_encoder, we can of course address the issues in drm_bridge.

Quoted 141 lines

And Display mode - VIDEO and COMMAND mode - is generic type of MIPI DSI, and also componentised subsystem is a generic solution to resolve probing order issue not Exynos specific feature. These are driver specific ones not Exynos SoC I think. As SoC specific things should be considered, I think MIPI DSI Driver - interrupt handler and probing order things are really specific to device driver - should be separated but we could share the control part of the device.

I was busy with other projects so didn't care of Linux DRM world so there may be my missing something.

Quoted 130 lines

Quoted 130 lines

Signed-off-by Jagan Teki <[email protected]>

---
 drivers/gpu/drm/exynos/exynos_drm_dsi.c | 37 +++++++++++++++++++------
 1 file changed, 29 insertions(+), 8 deletions(-)

drivers/gpu/drm/exynos/exynos_drm_dsi.c +29 -8

index 99a1b8c22313..53d878d4d2d7 100644
@@ -250,6 +250,7 @@ struct exynos_dsi_driver_data {
 	unsigned int wait_for_reset;
 	unsigned int num_bits_resol;
 	const unsigned int *reg_values;
+	bool exynos_specific;
 };
 
 struct exynos_dsi {

@@ -459,6 +460,7 @@ static const struct exynos_dsi_driver_data exynos3_dsi_driver_data = {
 	.wait_for_reset = 1,
 	.num_bits_resol = 11,
 	.reg_values = reg_values,
+	.exynos_specific = true,
 };
 
 static const struct exynos_dsi_driver_data exynos4_dsi_driver_data = {

@@ -471,6 +473,7 @@ static const struct exynos_dsi_driver_data exynos4_dsi_driver_data = {
 	.wait_for_reset = 1,
 	.num_bits_resol = 11,
 	.reg_values = reg_values,
+	.exynos_specific = true,
 };
 
 static const struct exynos_dsi_driver_data exynos5_dsi_driver_data = {

@@ -481,6 +484,7 @@ static const struct exynos_dsi_driver_data exynos5_dsi_driver_data = {
 	.wait_for_reset = 1,
 	.num_bits_resol = 11,
 	.reg_values = reg_values,
+	.exynos_specific = true,
 };
 
 static const struct exynos_dsi_driver_data exynos5433_dsi_driver_data = {

@@ -492,6 +496,7 @@ static const struct exynos_dsi_driver_data exynos5433_dsi_driver_data = {
 	.wait_for_reset = 0,
 	.num_bits_resol = 12,
 	.reg_values = exynos5433_reg_values,
+	.exynos_specific = true,
 };
 
 static const struct exynos_dsi_driver_data exynos5422_dsi_driver_data = {

@@ -503,6 +508,7 @@ static const struct exynos_dsi_driver_data exynos5422_dsi_driver_data = {
 	.wait_for_reset = 1,
 	.num_bits_resol = 12,
 	.reg_values = exynos5422_reg_values,
+	.exynos_specific = true,
 };
 
 static const struct of_device_id exynos_dsi_of_match[] = {

@@ -1484,7 +1490,8 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 	 * If attached panel device is for command mode one, dsi should register
 	 * TE interrupt handler.
 	 */
-	if (!(device->mode_flags & MIPI_DSI_MODE_VIDEO)) {
+	if (dsi->driver_data->exynos_specific &&
+	    !(device->mode_flags & MIPI_DSI_MODE_VIDEO)) {
 		int ret = exynos_dsi_register_te_irq(dsi, &device->dev);
 		if (ret)
 			return ret;

@@ -1495,8 +1502,9 @@ static int exynos_dsi_host_attach(struct mipi_dsi_host *host,
 	dsi->lanes = device->lanes;
 	dsi->format = device->format;
 	dsi->mode_flags = device->mode_flags;
-	exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode =
-			!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO);
+	if (dsi->driver_data->exynos_specific)
+		exynos_drm_crtc_get_by_type(drm, EXYNOS_DISPLAY_TYPE_LCD)->i80_mode =
+					    !(dsi->mode_flags & MIPI_DSI_MODE_VIDEO);
 
 	mutex_unlock(&drm->mode_config.mutex);
 

@@ -1515,7 +1523,8 @@ static int exynos_dsi_host_detach(struct mipi_dsi_host *host,
 	if (drm->mode_config.poll_enabled)
 		drm_kms_helper_hotplug_event(drm);
 
-	exynos_dsi_unregister_te_irq(dsi);
+	if (dsi->driver_data->exynos_specific)
+		exynos_dsi_unregister_te_irq(dsi);
 
 	return 0;
 }

@@ -1737,6 +1746,15 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	if (!dsi->driver_data->exynos_specific) {
+		ret = mipi_dsi_host_register(&dsi->dsi_host);
+		if (ret) {
+			dev_err(dev, "failed to register mipi dsi host: %d\n",
+				ret);
+			return ret;
+		}
+	}
+
 	platform_set_drvdata(pdev, dsi);
 
 	pm_runtime_enable(dev);

@@ -1747,9 +1765,11 @@ static int exynos_dsi_probe(struct platform_device *pdev)
 
 	drm_bridge_add(&dsi->bridge);
 
-	ret = component_add(dev, &exynos_dsi_component_ops);
-	if (ret)
-		goto err_disable_runtime;
+	if (dsi->driver_data->exynos_specific) {
+		ret = component_add(dev, &exynos_dsi_component_ops);
+		if (ret)
+			goto err_disable_runtime;
+	}
 
 	return 0;
 

@@ -1767,7 +1787,8 @@ static int exynos_dsi_remove(struct platform_device *pdev)
 
 	pm_runtime_disable(&pdev->dev);
 
-	component_del(&pdev->dev, &exynos_dsi_component_ops);
+	if (dsi->driver_data->exynos_specific)
+		component_del(&pdev->dev, &exynos_dsi_component_ops);
 
 	return 0;
 }

Samsing MIPI DSIM bridge can be found on Exynos and NXP's i.MX8M Mini and Nano SoC's.

This dt-bindings replaces legacy exynos_dsim.txt.

Used the example node from latest Exynos SoC instead of the one used in legacy exynos_dsim.txt.

Add dt-bingings for it.

Cc Rob Herring <[email protected]>

Signed-off-by Jagan Teki <[email protected]>

---
 .../display/bridge/samsung,mipi-dsim.yaml     | 278 ++++++++++++++++++
 .../bindings/display/exynos/exynos_dsim.txt   |  90 ------
 MAINTAINERS                                   |   1 +
 3 files changed, 279 insertions(+), 90 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml
 delete mode 100644 Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt

Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml new file +18 -0

new file mode 100644
index 000000000000..b2970734ffd7
@@ -0,0 +1,278 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/samsung,mipi-dsim.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Samsung MIPI DSIM bridge controller
+
+maintainers:
+  - Inki Dae <[email protected]>
+  - Joonyoung Shim <[email protected]>
+  - Seung-Woo Kim <[email protected]>
+  - Kyungmin Park <[email protected]>
+  - Andrzej Hajda <[email protected]>
+  - Jagan Teki <[email protected]>
+
+description: |
+  Samsung MIPI DSIM bridge controller can be found it on Exynos

Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml new file +14 -0

+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  clocks:

Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml new file +24 -0

+    maxItems: 1
+
+  samsung,power-domain:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle to the associated samsung power domain
+    maxItems: 1
+
+  vddcore-supply:
+    description: MIPI DSIM Core voltage supply (e.g. 1.1V)
+
+  vddio-supply:
+    description: MIPI DSIM I/O and PLL voltage supply (e.g. 1.8V)
+
+  samsung,burst-clock-frequency:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      DSIM high speed burst mode frequency.
+
+  samsung,esc-clock-frequency:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      DSIM escape mode frequency.
+
+  samsung,pll-clock-frequency:

Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml new file +173 -0

+
+        unevaluatedProperties: false
+
+      port@1:
+        $ref: /schemas/graph.yaml#/properties/port
+        description:
+          DSI output port node to the panel or the next bridge
+          in the chain
+
+required:
+  - '#address-cells'
+  - '#size-cells'
+  - clock-names
+  - clocks
+  - compatible
+  - interrupts
+  - phy-names
+  - phys
+  - reg
+  - samsung,burst-clock-frequency
+  - samsung,esc-clock-frequency
+  - samsung,pll-clock-frequency
+
+allOf:
+  - $ref: ../dsi-controller.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: samsung,exynos5433-mipi-dsi
+
+    then:
+      properties:
+        clocks:
+          minItems: 5
+
+        clock-names:
+          items:
+            - const: bus_clk
+            - const: phyclk_mipidphy0_bitclkdiv8
+            - const: phyclk_mipidphy0_rxclkesc0
+            - const: sclk_rgb_vclk_to_dsim0
+            - const: sclk_mipi
+
+        ports:
+          required:
+            - port@0
+
+      required:
+        - ports
+        - vddcore-supply
+        - vddio-supply
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: samsung,exynos5410-mipi-dsi
+
+    then:
+      properties:
+        clocks:
+          minItems: 2
+
+        clock-names:
+          items:
+            - const: bus_clk
+            - const: pll_clk
+
+      required:
+        - vddcore-supply
+        - vddio-supply
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: samsung,exynos4210-mipi-dsi
+
+    then:
+      properties:
+        clocks:
+          minItems: 2
+
+        clock-names:
+          items:
+            - const: bus_clk
+            - const: sclk_mipi
+
+      required:
+        - vddcore-supply
+        - vddio-supply
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: samsung,exynos3250-mipi-dsi
+
+    then:
+      properties:
+        clocks:
+          minItems: 2
+
+        clock-names:
+          items:
+            - const: bus_clk
+            - const: pll_clk
+
+      required:
+        - vddcore-supply
+        - vddio-supply
+        - samsung,phy-type
+
+additionalProperties:
+  type: object
+
+examples:
+  - |
+    #include <dt-bindings/clock/exynos5433.h>
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    dsi@13900000 {
+       compatible = "samsung,exynos5433-mipi-dsi";
+       reg = <0x13900000 0xC0>;
+       interrupts = <GIC_SPI 205 IRQ_TYPE_LEVEL_HIGH>;
+       phys = <&mipi_phy 1>;
+       phy-names = "dsim";
+       clocks = <&cmu_disp CLK_PCLK_DSIM0>,
+                <&cmu_disp CLK_PHYCLK_MIPIDPHY0_BITCLKDIV8>,
+                <&cmu_disp CLK_PHYCLK_MIPIDPHY0_RXCLKESC0>,
+                <&cmu_disp CLK_SCLK_RGB_VCLK_TO_DSIM0>,
+                <&cmu_disp CLK_SCLK_DSIM0>;
+       clock-names = "bus_clk",
+                     "phyclk_mipidphy0_bitclkdiv8",
+                     "phyclk_mipidphy0_rxclkesc0",
+                     "sclk_rgb_vclk_to_dsim0",
+                     "sclk_mipi";
+       power-domains = <&pd_disp>;
+       vddcore-supply = <&ldo6_reg>;
+       vddio-supply = <&ldo7_reg>;
+       samsung,burst-clock-frequency = <512000000>;
+       samsung,esc-clock-frequency = <16000000>;
+       samsung,pll-clock-frequency = <24000000>;
+       pinctrl-names = "default";
+       pinctrl-0 = <&te_irq>;
+       status = "disabled";
+       #address-cells = <1>;
+       #size-cells = <0>;
+
+       panel@0 {
+          compatible = "samsung,s6e3ha2";
+          reg = <0>;
+          vdd3-supply = <&ldo27_reg>;
+          vci-supply = <&ldo28_reg>;
+          reset-gpios = <&gpg0 0 GPIO_ACTIVE_LOW>;
+          enable-gpios = <&gpf1 5 GPIO_ACTIVE_HIGH>;
+       };
+
+       ports {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          port@0 {
+             reg = <0>;
+
+             dsi_to_mic: endpoint {
+                remote-endpoint = <&mic_to_dsi>;
+             };
+          };
+       };
+    };

Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt deleted +0 -90

deleted file mode 100644
index be377786e8cd..000000000000
@@ -1,90 +0,0 @@
-Exynos MIPI DSI Master
-
-Required properties:
-  - compatible: value should be one of the following
-		"samsung,exynos3250-mipi-dsi" /* for Exynos3250/3472 SoCs */
-		"samsung,exynos4210-mipi-dsi" /* for Exynos4 SoCs */
-		"samsung,exynos5410-mipi-dsi" /* for Exynos5410/5420/5440 SoCs */
-		"samsung,exynos5422-mipi-dsi" /* for Exynos5422/5800 SoCs */
-		"samsung,exynos5433-mipi-dsi" /* for Exynos5433 SoCs */
-  - reg: physical base address and length of the registers set for the device
-  - interrupts: should contain DSI interrupt
-  - clocks: list of clock specifiers, must contain an entry for each required
-    entry in clock-names
-  - clock-names: should include "bus_clk"and "sclk_mipi" entries
-		 the use of "pll_clk" is deprecated
-  - phys: list of phy specifiers, must contain an entry for each required
-    entry in phy-names
-  - phy-names: should include "dsim" entry
-  - vddcore-supply: MIPI DSIM Core voltage supply (e.g. 1.1V)
-  - vddio-supply: MIPI DSIM I/O and PLL voltage supply (e.g. 1.8V)
-  - samsung,pll-clock-frequency: specifies frequency of the oscillator clock
-  - #address-cells, #size-cells: should be set respectively to <1> and <0>
-    according to DSI host bindings (see MIPI DSI bindings [1])
-  - samsung,burst-clock-frequency: specifies DSI frequency in high-speed burst
-    mode
-  - samsung,esc-clock-frequency: specifies DSI frequency in escape mode
-
-Optional properties:
-  - power-domains: a phandle to DSIM power domain node
-
-Child nodes:
-  Should contain DSI peripheral nodes (see MIPI DSI bindings [1]).
-
-Video interfaces:
-  Device node can contain following video interface port nodes according to [2]:
-  0: RGB input,
-  1: DSI output
-
-[1]: Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
-[2]: Documentation/devicetree/bindings/media/video-interfaces.txt
-
-Example:
-
-	dsi@11c80000 {
-		compatible = "samsung,exynos4210-mipi-dsi";
-		reg = <0x11C80000 0x10000>;
-		interrupts = <0 79 0>;
-		clocks = <&clock 286>, <&clock 143>;
-		clock-names = "bus_clk", "sclk_mipi";
-		phys = <&mipi_phy 1>;
-		phy-names = "dsim";
-		vddcore-supply = <&vusb_reg>;
-		vddio-supply = <&vmipi_reg>;
-		power-domains = <&pd_lcd0>;
-		#address-cells = <1>;
-		#size-cells = <0>;
-		samsung,pll-clock-frequency = <24000000>;
-
-		panel@1 {
-			reg = <0>;
-			...
-			port {
-				panel_ep: endpoint {
-					remote-endpoint = <&dsi_ep>;
-				};
-			};
-		};
-
-		ports {
-			#address-cells = <1>;
-			#size-cells = <0>;
-
-			port@0 {
-				reg = <0>;
-				decon_to_mic: endpoint {
-					remote-endpoint = <&mic_to_decon>;
-				};
-			};
-
-			port@1 {
-				reg = <1>;
-				dsi_ep: endpoint {
-					reg = <0>;
-					samsung,burst-clock-frequency = <500000000>;
-					samsung,esc-clock-frequency = <20000000>;
-					remote-endpoint = <&panel_ep>;
-				};
-			};
-		};
-	};

MAINTAINERS +1 -0

index 733c2ebc0393..e571fc7792aa 100644
@@ -5873,6 +5873,7 @@ M:	Andrzej Hajda <[email protected]>
 M:	Jagan Teki <[email protected]>
 S:	Maintained
 T:	git git://anongit.freedesktop.org/drm/drm-misc
+F:	Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml
 F:	drivers/gpu/drm/bridge/samsung-dsim.c
 
 DRM DRIVER FOR SITRONIX ST7703 PANELS

Signature · 1 line

2.25.1

Quoted 9 lines · Jagan Teki

Samsing MIPI DSIM bridge can be found on Exynos and NXP's i.MX8M Mini and Nano SoC's.

This dt-bindings replaces legacy exynos_dsim.txt.

Used the example node from latest Exynos SoC instead of the one used in legacy exynos_dsim.txt.

Add dt-bingings for it.

typo

Quoted 39 lines

Cc Rob Herring <[email protected]>

Signed-off-by Jagan Teki <[email protected]>

---
 .../display/bridge/samsung,mipi-dsim.yaml     | 278 ++++++++++++++++++
 .../bindings/display/exynos/exynos_dsim.txt   |  90 ------
 MAINTAINERS                                   |   1 +
 3 files changed, 279 insertions(+), 90 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml
 delete mode 100644 Documentation/devicetree/bindings/display/exynos/exynos_dsim.txt

Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml new file +22 -0

new file mode 100644
index 000000000000..b2970734ffd7
@@ -0,0 +1,278 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/display/bridge/samsung,mipi-dsim.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Samsung MIPI DSIM bridge controller
+
+maintainers:
+  - Inki Dae <[email protected]>
+  - Joonyoung Shim <[email protected]>
+  - Seung-Woo Kim <[email protected]>
+  - Kyungmin Park <[email protected]>
+  - Andrzej Hajda <[email protected]>
+  - Jagan Teki <[email protected]>
+
+description: |
+  Samsung MIPI DSIM bridge controller can be found it on Exynos
+  and i.MX8M Mini and Nano SoC's.
+
+properties:
+  compatible:

Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml line 28

Quoted 6 lines

Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml new file +6 -0

+    enum:
+      - samsung,exynos3250-mipi-dsi
+      - samsung,exynos4210-mipi-dsi
+      - samsung,exynos5410-mipi-dsi
+      - samsung,exynos5422-mipi-dsi
+      - samsung,exynos5433-mipi-dsi

What about i.MX compatibles?

Quoted 15 lines

Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml new file +14 -0

+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 0
+
+  clocks:

Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml line 52

Quoted 10 lines

Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml new file +10 -0

+    minItems: 2
+    maxItems: 5
+
+  clock-names:
+    minItems: 2
+    maxItems: 5
+
+  phys:
+    maxItems: 1
+    description: phandle to the phy module representing the DPHY

Drop

Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml line 63

Quoted 11 lines

Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml new file +11 -0

+
+  phy-names:
+    items:
+      - const: dsim
+
+  samsung,phy-type:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: phandle to the samsung phy-type
+
+  power-domains:
+    description: phandle to the associated power domain

Drop

Quoted 29 lines

Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml new file +29 -0

+    maxItems: 1
+
+  samsung,power-domain:
+    $ref: /schemas/types.yaml#/definitions/phandle
+    description: phandle to the associated samsung power domain
+    maxItems: 1
+
+  vddcore-supply:
+    description: MIPI DSIM Core voltage supply (e.g. 1.1V)
+
+  vddio-supply:
+    description: MIPI DSIM I/O and PLL voltage supply (e.g. 1.8V)
+
+  samsung,burst-clock-frequency:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      DSIM high speed burst mode frequency.
+
+  samsung,esc-clock-frequency:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      DSIM escape mode frequency.
+
+  samsung,pll-clock-frequency:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description:
+      DSIM oscillator clock frequency.
+
+  ports:

Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml line 97

Quoted 5 lines

Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml new file +5 -0

+    $ref: /schemas/graph.yaml#/properties/ports
+
+    properties:
+      port@0:
+        $ref: /schemas/graph.yaml#/$defs/port-base

If there are no extra endpoint properties, then use '/schemas/graph.yaml#/properties/port'.

Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml line 105

Quoted 9 lines

Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml new file +8 -0

+        description:
+          Input port node to receive pixel data from the
+          display controller. Exactly one endpoint must be
+          specified.
+        properties:
+          endpoint@0:
+            $ref: /schemas/graph.yaml#/properties/endpoint
+            description: sub-node describing the input from MIC

I'd assume i.MX has a different input than MIC?

Samsung MIPI DSIM bridge can also be found in i.MX8MM SoC.

Add dt-bingings for it.

Cc Rob Herring <[email protected]>

Signed-off-by Jagan Teki <[email protected]>

---
 .../display/bridge/samsung,mipi-dsim.yaml     | 84 ++++++++++++++++++-
 1 file changed, 83 insertions(+), 1 deletion(-)

Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml +1 -0

index b2970734ffd7..bd12d5706291 100644
@@ -26,6 +26,7 @@ properties:
       - samsung,exynos5410-mipi-dsi
       - samsung,exynos5422-mipi-dsi
       - samsung,exynos5433-mipi-dsi
+      - fsl,imx8mm-mipi-dsim
 
   reg:
     maxItems: 1

Documentation/devicetree/bindings/display/bridge/samsung,mipi-dsim.yaml +79 -1

+
   clocks:
     minItems: 2
     maxItems: 5

@@ -102,7 +107,7 @@ properties:
         properties:
           endpoint@0:
             $ref: /schemas/graph.yaml#/properties/endpoint
-            description: sub-node describing the input from MIC
+            description: sub-node describing the input from MIC or LCDIF
 
         unevaluatedProperties: false
 

@@ -128,6 +133,30 @@ required:
 
 allOf:
   - $ref: ../dsi-controller.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: fsl,imx8mm-mipi-dsim
+
+    then:
+      properties:
+        clocks:
+          minItems: 2
+
+        clock-names:
+          items:
+            - const: bus_clk
+            - const: sclk_mipi
+
+        ports:
+          required:
+            - port@0
+            - port@1
+
+      required:
+        - ports
+
   - if:
       properties:
         compatible:

@@ -221,6 +250,59 @@ additionalProperties:
   type: object
 
 examples:
+  - |
+    #include <dt-bindings/clock/imx8mm-clock.h>
+    #include <dt-bindings/power/imx8mm-power.h>
+    #include <dt-bindings/gpio/gpio.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    dsi@32e10000 {
+       compatible = "fsl,imx8mm-mipi-dsim";
+       reg = <0x32e10000 0x400>;
+       clocks = <&clk IMX8MM_CLK_DSI_CORE>,
+                <&clk IMX8MM_CLK_DSI_PHY_REF>;
+       clock-names = "bus_clk", "sclk_mipi";
+       assigned-clocks = <&clk IMX8MM_CLK_DSI_CORE>,
+                         <&clk IMX8MM_VIDEO_PLL1_OUT>,
+                         <&clk IMX8MM_CLK_DSI_PHY_REF>;
+       assigned-clock-parents = <&clk IMX8MM_SYS_PLL1_266M>,
+                         <&clk IMX8MM_VIDEO_PLL1_BYPASS>,
+                         <&clk IMX8MM_VIDEO_PLL1_OUT>;
+       assigned-clock-rates = <266000000>, <594000000>, <27000000>;
+       interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>;
+       phys = <&mipi_phy 0>;
+       phy-names = "dsim";
+       power-domains = <&dispmix_blk_ctl IMX8MM_BLK_CTL_PD_DISPMIX_MIPI_DSI>;
+       samsung,burst-clock-frequency = <891000000>;
+       samsung,esc-clock-frequency = <54000000>;
+       samsung,pll-clock-frequency = <27000000>;
+       status = "disabled";
+
+       ports {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          port@0 {
+             reg = <0>;
+             #address-cells = <1>;
+             #size-cells = <0>;
+
+             dsi_in_lcdif: endpoint@0 {
+                reg = <0>;
+                remote-endpoint = <&lcdif_out_dsi>;
+             };
+          };
+
+          port@1 {
+             reg = <1>;
+
+             dsi_out_panel: endpoint {
+                remote-endpoint = <&panel_in_dsi>;
+             };
+          };
+       };
+    };
+
   - |
     #include <dt-bindings/clock/exynos5433.h>
     #include <dt-bindings/gpio/gpio.h>

Signature · 1 line

2.25.1

PMS_P offset value in existing driver is not compatible with i.MX8MM.

However the i.MX8M Mini Application Reference manual shows the PMS_P offset is the same in the driver, but the i.MX8MM downstream driver uses a different one.

So, handle the PMS_P offset via driver_data and use the offset value for i.MX8MM from the downstream driver.

Signed-off-by Jagan Teki <[email protected]>

---
 drivers/gpu/drm/bridge/samsung-dsim.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

drivers/gpu/drm/bridge/samsung-dsim.c +10 -2

index 54767cbf231c..0ed218f5eefc 100644
@@ -184,7 +184,7 @@
 /* DSIM_PLLCTRL */
 #define DSIM_FREQ_BAND(x)		((x) << 24)
 #define DSIM_PLL_EN			(1 << 23)
-#define DSIM_PLL_P(x)			((x) << 13)
+#define DSIM_PLL_P(x, offset)		((x) << (offset))
 #define DSIM_PLL_M(x)			((x) << 4)
 #define DSIM_PLL_S(x)			((x) << 1)
 

@@ -259,6 +259,7 @@ struct samsung_dsim_driver_data {
 	unsigned int max_freq;
 	unsigned int wait_for_reset;
 	unsigned int num_bits_resol;
+	unsigned int pll_p_offset;
 	const unsigned int *reg_values;
 	bool exynos_specific;
 };

@@ -487,6 +488,7 @@ static const struct samsung_dsim_driver_data exynos3_dsi_driver_data = {
 	.max_freq = 1000,
 	.wait_for_reset = 1,
 	.num_bits_resol = 11,
+	.pll_p_offset = 13,
 	.reg_values = reg_values,
 	.exynos_specific = true,
 };

@@ -500,6 +502,7 @@ static const struct samsung_dsim_driver_data exynos4_dsi_driver_data = {
 	.max_freq = 1000,
 	.wait_for_reset = 1,
 	.num_bits_resol = 11,
+	.pll_p_offset = 13,
 	.reg_values = reg_values,
 	.exynos_specific = true,
 };

@@ -511,6 +514,7 @@ static const struct samsung_dsim_driver_data exynos5_dsi_driver_data = {
 	.max_freq = 1000,
 	.wait_for_reset = 1,
 	.num_bits_resol = 11,
+	.pll_p_offset = 13,
 	.reg_values = reg_values,
 	.exynos_specific = true,
 };

@@ -523,6 +527,7 @@ static const struct samsung_dsim_driver_data exynos5433_dsi_driver_data = {
 	.max_freq = 1500,
 	.wait_for_reset = 0,
 	.num_bits_resol = 12,
+	.pll_p_offset = 13,
 	.reg_values = exynos5433_reg_values,
 	.exynos_specific = true,
 };

@@ -535,6 +540,7 @@ static const struct samsung_dsim_driver_data exynos5422_dsi_driver_data = {
 	.max_freq = 1500,
 	.wait_for_reset = 1,
 	.num_bits_resol = 12,
+	.pll_p_offset = 13,
 	.reg_values = exynos5422_reg_values,
 	.exynos_specific = true,
 };

@@ -547,6 +553,7 @@ static const struct samsung_dsim_driver_data imx8mm_dsi_driver_data = {
 	.max_freq = 2100,
 	.wait_for_reset = 0,
 	.num_bits_resol = 12,
+	.pll_p_offset = 14,
 	.reg_values = imx8mm_dsim_reg_values,
 };
 

@@ -662,7 +669,8 @@ static unsigned long samsung_dsim_set_pll(struct samsung_dsim *dsi,
 	writel(driver_data->reg_values[PLL_TIMER],
 			dsi->reg_base + driver_data->plltmr_reg);
 
-	reg = DSIM_PLL_EN | DSIM_PLL_P(p) | DSIM_PLL_M(m) | DSIM_PLL_S(s);
+	reg = DSIM_PLL_EN | DSIM_PLL_P(p, driver_data->pll_p_offset) |
+	      DSIM_PLL_M(m) | DSIM_PLL_S(s);
 
 	if (driver_data->has_freqband) {
 		static const unsigned long freq_bands[] = {

Signature · 1 line

2.25.1

Fixing up the mode flags are required in order to correlate the correct sync flags in i.MX8MM eLCDIF.

So, handle the mode flags via bridge, mode_fixup.

Signed-off-by Jagan Teki <[email protected]>

---
 drivers/gpu/drm/bridge/samsung-dsim.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

drivers/gpu/drm/bridge/samsung-dsim.c +11 -0

index 0ed218f5eefc..c2a76ee5ac4e 100644
@@ -1474,6 +1474,16 @@ static void samsung_dsim_bridge_disable(struct drm_bridge *bridge)
 	pm_runtime_put_sync(dsi->dev);
 }
 
+static bool samsung_dsim_bridge_mode_fixup(struct drm_bridge *bridge,
+					   const struct drm_display_mode *mode,
+					   struct drm_display_mode *adjusted_mode)
+{
+	adjusted_mode->flags |= (DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_NVSYNC);
+	adjusted_mode->flags &= ~(DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_PVSYNC);
+
+	return true;
+}
+
 static void samsung_dsim_bridge_mode_set(struct drm_bridge *bridge,
 					 const struct drm_display_mode *mode,
 					 const struct drm_display_mode *adjusted_mode)

@@ -1542,6 +1552,7 @@ static const struct drm_bridge_funcs samsung_dsim_bridge_funcs = {
 	.enable = samsung_dsim_bridge_enable,
 	.disable = samsung_dsim_bridge_disable,
 	.mode_set = samsung_dsim_bridge_mode_set,
+	.mode_fixup = samsung_dsim_bridge_mode_fixup,
 	.attach = samsung_dsim_bridge_attach,
 };
 

Signature · 1 line

2.25.1

This series supports common bridge support for Samsung MIPI DSIM which is used in Exynos and i.MX8MM SoC's.

The final bridge supports both the Exynos and i.MX8MM DSI devices.

Right now bridge offers two sets of implementations.

A. With component_ops and exynos specific code exclusively for exynos dsi drivers and it's legacy bindings.

B. Without componenet_ops for newly implemented bridges and its users like i.MX8MM.

The future plan is to fix the implementation A) by dropping component_ops and fixing exynos specific code in order to make the bridge more mature to use and the same is mentioned in drivers TODO.

Patch 0001 - 0006: Bridge conversion
Patch 0007 - 0017: Samsung MIPI DSIM bridge fixes, additions

Tested in Engicam i.Core MX8M Mini SoM.

Anyone interest, please have a look on this repo
https://github.com/openedev/linux/tree/070421-imx8mm-dsim

Would appreciate anyone from the exynos team to test it on the exynos platform?

Any inputs?

Hi Jagan,

Quoted 29 lines · Jagan Teki

This series supports common bridge support for Samsung MIPI DSIM which is used in Exynos and i.MX8MM SoC's.

The final bridge supports both the Exynos and i.MX8MM DSI devices.

Right now bridge offers two sets of implementations.

A. With component_ops and exynos specific code exclusively for
exynos dsi drivers and it's legacy bindings.

B. Without componenet_ops for newly implemented bridges and its
users like i.MX8MM.

The future plan is to fix the implementation A) by dropping component_ops and fixing exynos specific code in order to make the bridge more mature to use and the same is mentioned in drivers TODO.

Patch 0001 - 0006: Bridge conversion
Patch 0007 - 0017: Samsung MIPI DSIM bridge fixes, additions

Tested in Engicam i.Core MX8M Mini SoM.

Anyone interest, please have a look on this repo
https://github.com/openedev/linux/tree/070421-imx8mm-dsim

Would appreciate anyone from the exynos team to test it on the exynos platform?

Quoted 1 line

Any inputs?

I really like where you are headign with this!
No testing - sorry. But I will try to provide a bit of feedback on the individual patches.

I hope you find a way to move forward with this.

Hi Sam,

Quoted 33 lines · Sam Ravnborg

Hi Jagan,

Quoted 29 lines · Jagan Teki

This series supports common bridge support for Samsung MIPI DSIM which is used in Exynos and i.MX8MM SoC's.

The final bridge supports both the Exynos and i.MX8MM DSI devices.

Right now bridge offers two sets of implementations.

A. With component_ops and exynos specific code exclusively for
exynos dsi drivers and it's legacy bindings.

B. Without componenet_ops for newly implemented bridges and its
users like i.MX8MM.

The future plan is to fix the implementation A) by dropping component_ops and fixing exynos specific code in order to make the bridge more mature to use and the same is mentioned in drivers TODO.

Patch 0001 - 0006: Bridge conversion
Patch 0007 - 0017: Samsung MIPI DSIM bridge fixes, additions

Tested in Engicam i.Core MX8M Mini SoM.

Anyone interest, please have a look on this repo
https://github.com/openedev/linux/tree/070421-imx8mm-dsim

Would appreciate anyone from the exynos team to test it on the exynos platform?

Quoted 7 lines

Quoted 1 line

Any inputs?

I really like where you are headign with this!
No testing - sorry. But I will try to provide a bit of feedback on the individual patches.

I hope you find a way to move forward with this.

Thanks for the response.

We have found some issues with Bridge conversion on existing exynos drivers. The component based DSI drivers(like exynos) are difficult to attach if it involves kms hotplug. kms hotplug would require drm pointer and that pointer would only available after the bind call finishes. But the bridge attach in bind call will defer till it find the attached bridge.

Right now I'm trying to find the proper way to attach the bridges for component based DSI drivers which involves kms hot-plug.

If you have any ideas on this, please let me know.

Quoted 54 lines · Jagan Teki

Hi Sam,

Quoted 33 lines · Sam Ravnborg

Hi Jagan,

Quoted 29 lines · Jagan Teki

This series supports common bridge support for Samsung MIPI DSIM which is used in Exynos and i.MX8MM SoC's.

The final bridge supports both the Exynos and i.MX8MM DSI devices.

Right now bridge offers two sets of implementations.

A. With component_ops and exynos specific code exclusively for
exynos dsi drivers and it's legacy bindings.

B. Without componenet_ops for newly implemented bridges and its
users like i.MX8MM.

The future plan is to fix the implementation A) by dropping component_ops and fixing exynos specific code in order to make the bridge more mature to use and the same is mentioned in drivers TODO.

Patch 0001 - 0006: Bridge conversion
Patch 0007 - 0017: Samsung MIPI DSIM bridge fixes, additions

Tested in Engicam i.Core MX8M Mini SoM.

Anyone interest, please have a look on this repo
https://github.com/openedev/linux/tree/070421-imx8mm-dsim

Would appreciate anyone from the exynos team to test it on the exynos platform?

Quoted 7 lines

Quoted 1 line

Any inputs?

I really like where you are headign with this!
No testing - sorry. But I will try to provide a bit of feedback on the individual patches.

I hope you find a way to move forward with this.

Thanks for the response.

We have found some issues with Bridge conversion on existing exynos drivers. The component based DSI drivers(like exynos) are difficult to attach if it involves kms hotplug. kms hotplug would require drm pointer and that pointer would only available after the bind call finishes. But the bridge attach in bind call will defer till it find the attached bridge.

Quoted 5 lines

Right now I'm trying to find the proper way to attach the bridges for component based DSI drivers which involves kms hot-plug.

If you have any ideas on this, please let me know.

Jagan,

How is your progress on this series? Looking at your repo it looks like you've rebased on top of 5.13-rc3 in your 070121-imx8mm-dsim branch but you've got a lot of things there that are likely not related to this series?

Best regards,

Tim

Hi Tim

Quoted 62 lines · Tim Harvey

Quoted 54 lines · Jagan Teki

Hi Sam,

Quoted 33 lines · Sam Ravnborg

Hi Jagan,

Quoted 29 lines · Jagan Teki

This series supports common bridge support for Samsung MIPI DSIM which is used in Exynos and i.MX8MM SoC's.

The final bridge supports both the Exynos and i.MX8MM DSI devices.

Right now bridge offers two sets of implementations.

A. With component_ops and exynos specific code exclusively for
exynos dsi drivers and it's legacy bindings.

B. Without componenet_ops for newly implemented bridges and its
users like i.MX8MM.

The future plan is to fix the implementation A) by dropping component_ops and fixing exynos specific code in order to make the bridge more mature to use and the same is mentioned in drivers TODO.

Patch 0001 - 0006: Bridge conversion
Patch 0007 - 0017: Samsung MIPI DSIM bridge fixes, additions

Tested in Engicam i.Core MX8M Mini SoM.

Anyone interest, please have a look on this repo
https://github.com/openedev/linux/tree/070421-imx8mm-dsim

Would appreciate anyone from the exynos team to test it on the exynos platform?

Quoted 7 lines

Quoted 1 line

Any inputs?

I really like where you are headign with this!
No testing - sorry. But I will try to provide a bit of feedback on the individual patches.

I hope you find a way to move forward with this.

Thanks for the response.

We have found some issues with Bridge conversion on existing exynos drivers. The component based DSI drivers(like exynos) are difficult to attach if it involves kms hotplug. kms hotplug would require drm pointer and that pointer would only available after the bind call finishes. But the bridge attach in bind call will defer till it find the attached bridge.

Quoted 5 lines

Right now I'm trying to find the proper way to attach the bridges for component based DSI drivers which involves kms hot-plug.

If you have any ideas on this, please let me know.

Quoted 6 lines

Jagan,

How is your progress on this series? Looking at your repo it looks like you've rebased on top of 5.13-rc3 in your 070121-imx8mm-dsim branch but you've got a lot of things there that are likely not related to this series?

I have a bit of work on those patches and tested on imx8mn. Basically:

- add the dsi timing calculation
- change few difference with samsung bridge
- fix crashes of my dsi panels
- compare with NXP driver the final results

I found that I have one problem that gives me some instability. In the NXP original driver the panel needs to be
enabled in bridge_enable before out the standby. If I understand correctly, our standby should be done after.
I would like to have some feedback and help and testing on other boards/devices and some suggestions on how to handle
some of the differences. Another big problem is etnavi that is not stable

Quoted 80 lines · Michael Nazzareno Trimarchi

Hi Tim

Quoted 62 lines · Tim Harvey

Quoted 54 lines · Jagan Teki

Hi Sam,

Quoted 33 lines · Sam Ravnborg

Hi Jagan,

Quoted 29 lines · Jagan Teki

This series supports common bridge support for Samsung MIPI DSIM which is used in Exynos and i.MX8MM SoC's.

The final bridge supports both the Exynos and i.MX8MM DSI devices.

Right now bridge offers two sets of implementations.

A. With component_ops and exynos specific code exclusively for
exynos dsi drivers and it's legacy bindings.

B. Without componenet_ops for newly implemented bridges and its
users like i.MX8MM.

The future plan is to fix the implementation A) by dropping component_ops and fixing exynos specific code in order to make the bridge more mature to use and the same is mentioned in drivers TODO.

Patch 0001 - 0006: Bridge conversion
Patch 0007 - 0017: Samsung MIPI DSIM bridge fixes, additions

Tested in Engicam i.Core MX8M Mini SoM.

Anyone interest, please have a look on this repo
https://github.com/openedev/linux/tree/070421-imx8mm-dsim

Would appreciate anyone from the exynos team to test it on the exynos platform?

Quoted 7 lines

Quoted 1 line

Any inputs?

I really like where you are headign with this!
No testing - sorry. But I will try to provide a bit of feedback on the individual patches.

I hope you find a way to move forward with this.

Thanks for the response.

We have found some issues with Bridge conversion on existing exynos drivers. The component based DSI drivers(like exynos) are difficult to attach if it involves kms hotplug. kms hotplug would require drm pointer and that pointer would only available after the bind call finishes. But the bridge attach in bind call will defer till it find the attached bridge.

Quoted 5 lines

Right now I'm trying to find the proper way to attach the bridges for component based DSI drivers which involves kms hot-plug.

If you have any ideas on this, please let me know.

Quoted 6 lines

Jagan,

How is your progress on this series? Looking at your repo it looks like you've rebased on top of 5.13-rc3 in your 070121-imx8mm-dsim branch but you've got a lot of things there that are likely not related to this series?

I have a bit of work on those patches and tested on imx8mn. Basically:

- add the dsi timing calculation
- change few difference with samsung bridge
- fix crashes of my dsi panels
- compare with NXP driver the final results

Quoted 8 lines

I found that I have one problem that gives me some instability. In the NXP original driver the panel needs to be
enabled in bridge_enable before out the standby. If I understand correctly, our standby should be done after.
I would like to have some feedback and help and testing on other boards/devices and some suggestions on how to handle
some of the differences. Another big problem is etnavi that is not stable