[PATCH V2 00/11] drm/bridge: tc358767: Add DSI-to-DPI mode support

[PATCH V2 00/11] drm/bridge: tc358767: Add DSI-to-DPI mode support

[Marek Vasut]

The TC358767/TC358867/TC9595 are all capable of operating in multiple
modes, DPI-to-(e)DP, DSI-to-(e)DP, DSI-to-DPI. Clean up the driver,
switch to atomic ops, and add support for the DSI-to-DPI mode in
addition to already supported DPI-to-(e)DP mode.

Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
To: dri-devel@lists.freedesktop.org

Marek Vasut (11):
  dt-bindings: display: bridge: tc358867: Document DPI output support
  drm/bridge: tc358767: Change tc_ prefix to tc_edp_ for (e)DP specific
    functions
  drm/bridge: tc358767: Convert to atomic ops
  drm/bridge: tc358767: Implement atomic_check callback
  drm/bridge: tc358767: Move hardware init to enable callback
  drm/bridge: tc358767: Move (e)DP bridge endpoint parsing into
    dedicated function
  drm/bridge: tc358767: Wrap (e)DP aux I2C registration into
    tc_aux_link_setup()
  drm/bridge: tc358767: Move bridge ops setup into
    tc_probe_edp_bridge_endpoint()
  drm/bridge: tc358767: Detect bridge mode from connected endpoints in
    DT
  drm/bridge: tc358767: Split tc_set_video_mode() into common and (e)DP
    part
  drm/bridge: tc358767: Add DSI-to-DPI mode support

 .../display/bridge/toshiba,tc358767.yaml      |   4 +-
 drivers/gpu/drm/bridge/tc358767.c             | 654 +++++++++++++++---
 2 files changed, 557 insertions(+), 101 deletions(-)

-- 
2.34.1

[PATCH V2 01/11] dt-bindings: display: bridge: tc358867: Document DPI output support

[Marek Vasut]

The TC358767/TC358867/TC9595 are all capable of operating in multiple
modes, DPI-to-(e)DP, DSI-to-(e)DP, DSI-to-DPI. Document support for the
DPI output port, which can now be connected both as input and output.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: devicetree@vger.kernel.org
To: dri-devel@lists.freedesktop.org
---
V2: - Rebase on next-20220217
---
 .../devicetree/bindings/display/bridge/toshiba,tc358767.yaml  | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
index f1541cc052977..5cfda6f2ba69c 100644
--- a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
@@ -61,8 +61,8 @@ properties:
       port@1:
         $ref: /schemas/graph.yaml#/properties/port
         description: |
-            DPI input port. The remote endpoint phandle should be a
-            reference to a valid DPI output endpoint node
+            DPI input/output port. The remote endpoint phandle should be a
+            reference to a valid DPI output or input endpoint node.
 
       port@2:
         $ref: /schemas/graph.yaml#/properties/port
-- 
2.34.1

[PATCH V2 01/11] dt-bindings: display: bridge: tc358867: Document DPI output support

[Marek Vasut]

The TC358767/TC358867/TC9595 are all capable of operating in multiple
modes, DPI-to-(e)DP, DSI-to-(e)DP, DSI-to-DPI. Document support for the
DPI output port, which can now be connected both as input and output.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: devicetree@vger.kernel.org
To: dri-devel@lists.freedesktop.org
---
V2: - Rebase on next-20220217
---
 .../devicetree/bindings/display/bridge/toshiba,tc358767.yaml  | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
index f1541cc052977..5cfda6f2ba69c 100644
--- a/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/toshiba,tc358767.yaml
@@ -61,8 +61,8 @@ properties:
       port@1:
         $ref: /schemas/graph.yaml#/properties/port
         description: |
-            DPI input port. The remote endpoint phandle should be a
-            reference to a valid DPI output endpoint node
+            DPI input/output port. The remote endpoint phandle should be a
+            reference to a valid DPI output or input endpoint node.
 
       port@2:
         $ref: /schemas/graph.yaml#/properties/port
-- 
2.34.1

[PATCH V2 02/11] drm/bridge: tc358767: Change tc_ prefix to tc_edp_ for (e)DP specific functions

[Marek Vasut]

These functions are specific to (e)DP output initialization and
operation, add specific tc_edp_ prefix to those functions to
discern them from DPI output functions that will be added later
in this series. No functional change.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
V2: - New patch
---
 drivers/gpu/drm/bridge/tc358767.c | 35 ++++++++++++++++---------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index c23e0abc65e8f..4b8ea0db2a5e8 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1164,7 +1164,7 @@ static int tc_main_link_disable(struct tc_data *tc)
 	return regmap_write(tc->regmap, DP0CTL, 0);
 }
 
-static int tc_stream_enable(struct tc_data *tc)
+static int tc_edp_stream_enable(struct tc_data *tc)
 {
 	int ret;
 	u32 value;
@@ -1219,7 +1219,7 @@ static int tc_stream_enable(struct tc_data *tc)
 	return 0;
 }
 
-static int tc_stream_disable(struct tc_data *tc)
+static int tc_edp_stream_disable(struct tc_data *tc)
 {
 	int ret;
 
@@ -1234,7 +1234,7 @@ static int tc_stream_disable(struct tc_data *tc)
 	return 0;
 }
 
-static void tc_bridge_enable(struct drm_bridge *bridge)
+static void tc_edp_bridge_enable(struct drm_bridge *bridge)
 {
 	struct tc_data *tc = bridge_to_tc(bridge);
 	int ret;
@@ -1251,7 +1251,7 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
 		return;
 	}
 
-	ret = tc_stream_enable(tc);
+	ret = tc_edp_stream_enable(tc);
 	if (ret < 0) {
 		dev_err(tc->dev, "main link stream start error: %d\n", ret);
 		tc_main_link_disable(tc);
@@ -1259,12 +1259,12 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
 	}
 }
 
-static void tc_bridge_disable(struct drm_bridge *bridge)
+static void tc_edp_bridge_disable(struct drm_bridge *bridge)
 {
 	struct tc_data *tc = bridge_to_tc(bridge);
 	int ret;
 
-	ret = tc_stream_disable(tc);
+	ret = tc_edp_stream_disable(tc);
 	if (ret < 0)
 		dev_err(tc->dev, "main link stream stop error: %d\n", ret);
 
@@ -1285,9 +1285,10 @@ static bool tc_bridge_mode_fixup(struct drm_bridge *bridge,
 	return true;
 }
 
-static enum drm_mode_status tc_mode_valid(struct drm_bridge *bridge,
-					  const struct drm_display_info *info,
-					  const struct drm_display_mode *mode)
+static enum drm_mode_status
+tc_edp_mode_valid(struct drm_bridge *bridge,
+		  const struct drm_display_info *info,
+		  const struct drm_display_mode *mode)
 {
 	struct tc_data *tc = bridge_to_tc(bridge);
 	u32 req, avail;
@@ -1395,8 +1396,8 @@ static const struct drm_connector_funcs tc_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
-static int tc_bridge_attach(struct drm_bridge *bridge,
-			    enum drm_bridge_attach_flags flags)
+static int tc_edp_bridge_attach(struct drm_bridge *bridge,
+				enum drm_bridge_attach_flags flags)
 {
 	u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
 	struct tc_data *tc = bridge_to_tc(bridge);
@@ -1448,18 +1449,18 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
 	return ret;
 }
 
-static void tc_bridge_detach(struct drm_bridge *bridge)
+static void tc_edp_bridge_detach(struct drm_bridge *bridge)
 {
 	drm_dp_aux_unregister(&bridge_to_tc(bridge)->aux);
 }
 
 static const struct drm_bridge_funcs tc_bridge_funcs = {
-	.attach = tc_bridge_attach,
-	.detach = tc_bridge_detach,
-	.mode_valid = tc_mode_valid,
+	.attach = tc_edp_bridge_attach,
+	.detach = tc_edp_bridge_detach,
+	.mode_valid = tc_edp_mode_valid,
 	.mode_set = tc_bridge_mode_set,
-	.enable = tc_bridge_enable,
-	.disable = tc_bridge_disable,
+	.enable = tc_edp_bridge_enable,
+	.disable = tc_edp_bridge_disable,
 	.mode_fixup = tc_bridge_mode_fixup,
 	.detect = tc_bridge_detect,
 	.get_edid = tc_get_edid,
-- 
2.34.1

Re: [PATCH V2 02/11] drm/bridge: tc358767: Change tc_ prefix to tc_edp_ for (e)DP specific functions

[Lucas Stach]

Am Freitag, dem 18.02.2022 um 02:00 +0100 schrieb Marek Vasut:
> These functions are specific to (e)DP output initialization and
> operation, add specific tc_edp_ prefix to those functions to
> discern them from DPI output functions that will be added later
> in this series. No functional change.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
> V2: - New patch
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 35 ++++++++++++++++---------------
>  1 file changed, 18 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index c23e0abc65e8f..4b8ea0db2a5e8 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1164,7 +1164,7 @@ static int tc_main_link_disable(struct tc_data *tc)
>  	return regmap_write(tc->regmap, DP0CTL, 0);
>  }
>  
> -static int tc_stream_enable(struct tc_data *tc)
> +static int tc_edp_stream_enable(struct tc_data *tc)
>  {
>  	int ret;
>  	u32 value;
> @@ -1219,7 +1219,7 @@ static int tc_stream_enable(struct tc_data *tc)
>  	return 0;
>  }
>  
> -static int tc_stream_disable(struct tc_data *tc)
> +static int tc_edp_stream_disable(struct tc_data *tc)
>  {
>  	int ret;
>  
> @@ -1234,7 +1234,7 @@ static int tc_stream_disable(struct tc_data *tc)
>  	return 0;
>  }
>  
> -static void tc_bridge_enable(struct drm_bridge *bridge)
> +static void tc_edp_bridge_enable(struct drm_bridge *bridge)
>  {
>  	struct tc_data *tc = bridge_to_tc(bridge);
>  	int ret;
> @@ -1251,7 +1251,7 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
>  		return;
>  	}
>  
> -	ret = tc_stream_enable(tc);
> +	ret = tc_edp_stream_enable(tc);
>  	if (ret < 0) {
>  		dev_err(tc->dev, "main link stream start error: %d\n", ret);
>  		tc_main_link_disable(tc);
> @@ -1259,12 +1259,12 @@ static void tc_bridge_enable(struct drm_bridge *bridge)
>  	}
>  }
>  
> -static void tc_bridge_disable(struct drm_bridge *bridge)
> +static void tc_edp_bridge_disable(struct drm_bridge *bridge)
>  {
>  	struct tc_data *tc = bridge_to_tc(bridge);
>  	int ret;
>  
> -	ret = tc_stream_disable(tc);
> +	ret = tc_edp_stream_disable(tc);
>  	if (ret < 0)
>  		dev_err(tc->dev, "main link stream stop error: %d\n", ret);
>  
> @@ -1285,9 +1285,10 @@ static bool tc_bridge_mode_fixup(struct drm_bridge *bridge,
>  	return true;
>  }
>  
> -static enum drm_mode_status tc_mode_valid(struct drm_bridge *bridge,
> -					  const struct drm_display_info *info,
> -					  const struct drm_display_mode *mode)
> +static enum drm_mode_status
> +tc_edp_mode_valid(struct drm_bridge *bridge,
> +		  const struct drm_display_info *info,
> +		  const struct drm_display_mode *mode)
>  {
>  	struct tc_data *tc = bridge_to_tc(bridge);
>  	u32 req, avail;
> @@ -1395,8 +1396,8 @@ static const struct drm_connector_funcs tc_connector_funcs = {
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>  };
>  
> -static int tc_bridge_attach(struct drm_bridge *bridge,
> -			    enum drm_bridge_attach_flags flags)
> +static int tc_edp_bridge_attach(struct drm_bridge *bridge,
> +				enum drm_bridge_attach_flags flags)
>  {
>  	u32 bus_format = MEDIA_BUS_FMT_RGB888_1X24;
>  	struct tc_data *tc = bridge_to_tc(bridge);
> @@ -1448,18 +1449,18 @@ static int tc_bridge_attach(struct drm_bridge *bridge,
>  	return ret;
>  }
>  
> -static void tc_bridge_detach(struct drm_bridge *bridge)
> +static void tc_edp_bridge_detach(struct drm_bridge *bridge)
>  {
>  	drm_dp_aux_unregister(&bridge_to_tc(bridge)->aux);
>  }
>  
>  static const struct drm_bridge_funcs tc_bridge_funcs = {
> -	.attach = tc_bridge_attach,
> -	.detach = tc_bridge_detach,
> -	.mode_valid = tc_mode_valid,
> +	.attach = tc_edp_bridge_attach,
> +	.detach = tc_edp_bridge_detach,
> +	.mode_valid = tc_edp_mode_valid,
>  	.mode_set = tc_bridge_mode_set,
> -	.enable = tc_bridge_enable,
> -	.disable = tc_bridge_disable,
> +	.enable = tc_edp_bridge_enable,
> +	.disable = tc_edp_bridge_disable,
>  	.mode_fixup = tc_bridge_mode_fixup,
>  	.detect = tc_bridge_detect,
>  	.get_edid = tc_get_edid,

[PATCH V2 03/11] drm/bridge: tc358767: Convert to atomic ops

[Marek Vasut]

Use the atomic version of the enable/disable operations to continue the
transition to the atomic API. This will be needed to access the mode
from the atomic state.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
V2: - New patch
---
 drivers/gpu/drm/bridge/tc358767.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 4b8ea0db2a5e8..522c2c4d8514f 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1234,7 +1234,9 @@ static int tc_edp_stream_disable(struct tc_data *tc)
 	return 0;
 }
 
-static void tc_edp_bridge_enable(struct drm_bridge *bridge)
+static void
+tc_edp_bridge_atomic_enable(struct drm_bridge *bridge,
+			    struct drm_bridge_state *old_bridge_state)
 {
 	struct tc_data *tc = bridge_to_tc(bridge);
 	int ret;
@@ -1259,7 +1261,9 @@ static void tc_edp_bridge_enable(struct drm_bridge *bridge)
 	}
 }
 
-static void tc_edp_bridge_disable(struct drm_bridge *bridge)
+static void
+tc_edp_bridge_atomic_disable(struct drm_bridge *bridge,
+			     struct drm_bridge_state *old_bridge_state)
 {
 	struct tc_data *tc = bridge_to_tc(bridge);
 	int ret;
@@ -1459,11 +1463,14 @@ static const struct drm_bridge_funcs tc_bridge_funcs = {
 	.detach = tc_edp_bridge_detach,
 	.mode_valid = tc_edp_mode_valid,
 	.mode_set = tc_bridge_mode_set,
-	.enable = tc_edp_bridge_enable,
-	.disable = tc_edp_bridge_disable,
+	.atomic_enable = tc_edp_bridge_atomic_enable,
+	.atomic_disable = tc_edp_bridge_atomic_disable,
 	.mode_fixup = tc_bridge_mode_fixup,
 	.detect = tc_bridge_detect,
 	.get_edid = tc_get_edid,
+	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+	.atomic_reset = drm_atomic_helper_bridge_reset,
 };
 
 static bool tc_readable_reg(struct device *dev, unsigned int reg)
-- 
2.34.1

Re: [PATCH V2 03/11] drm/bridge: tc358767: Convert to atomic ops

[Lucas Stach]

Am Freitag, dem 18.02.2022 um 02:00 +0100 schrieb Marek Vasut:
> Use the atomic version of the enable/disable operations to continue the
> transition to the atomic API. This will be needed to access the mode
> from the atomic state.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
> V2: - New patch
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 4b8ea0db2a5e8..522c2c4d8514f 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1234,7 +1234,9 @@ static int tc_edp_stream_disable(struct tc_data *tc)
>  	return 0;
>  }
>  
> -static void tc_edp_bridge_enable(struct drm_bridge *bridge)
> +static void
> +tc_edp_bridge_atomic_enable(struct drm_bridge *bridge,
> +			    struct drm_bridge_state *old_bridge_state)
>  {
>  	struct tc_data *tc = bridge_to_tc(bridge);
>  	int ret;
> @@ -1259,7 +1261,9 @@ static void tc_edp_bridge_enable(struct drm_bridge *bridge)
>  	}
>  }
>  
> -static void tc_edp_bridge_disable(struct drm_bridge *bridge)
> +static void
> +tc_edp_bridge_atomic_disable(struct drm_bridge *bridge,
> +			     struct drm_bridge_state *old_bridge_state)
>  {
>  	struct tc_data *tc = bridge_to_tc(bridge);
>  	int ret;
> @@ -1459,11 +1463,14 @@ static const struct drm_bridge_funcs tc_bridge_funcs = {
>  	.detach = tc_edp_bridge_detach,
>  	.mode_valid = tc_edp_mode_valid,
>  	.mode_set = tc_bridge_mode_set,
> -	.enable = tc_edp_bridge_enable,
> -	.disable = tc_edp_bridge_disable,
> +	.atomic_enable = tc_edp_bridge_atomic_enable,
> +	.atomic_disable = tc_edp_bridge_atomic_disable,
>  	.mode_fixup = tc_bridge_mode_fixup,
>  	.detect = tc_bridge_detect,
>  	.get_edid = tc_get_edid,
> +	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> +	.atomic_reset = drm_atomic_helper_bridge_reset,
>  };
>  
>  static bool tc_readable_reg(struct device *dev, unsigned int reg)

[PATCH V2 04/11] drm/bridge: tc358767: Implement atomic_check callback

[Marek Vasut]

Implement .atomic_check callback which prevents user space from setting
unsupported mode. The tc_edp_common_atomic_check() variant is already
prepared for DSI-to-DPI mode addition, which has different frequency
limits.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
V2: - New patch
---
 drivers/gpu/drm/bridge/tc358767.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 522c2c4d8514f..01d11adee6c74 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1289,6 +1289,31 @@ static bool tc_bridge_mode_fixup(struct drm_bridge *bridge,
 	return true;
 }
 
+static int tc_edp_common_atomic_check(struct drm_bridge *bridge,
+				      struct drm_bridge_state *bridge_state,
+				      struct drm_crtc_state *crtc_state,
+				      struct drm_connector_state *conn_state,
+				      const unsigned int max_khz)
+{
+	tc_bridge_mode_fixup(bridge, &crtc_state->mode,
+			     &crtc_state->adjusted_mode);
+
+	if (crtc_state->adjusted_mode.clock > max_khz)
+		crtc_state->adjusted_mode.clock = max_khz;
+
+	return 0;
+}
+
+static int tc_edp_atomic_check(struct drm_bridge *bridge,
+			       struct drm_bridge_state *bridge_state,
+			       struct drm_crtc_state *crtc_state,
+			       struct drm_connector_state *conn_state)
+{
+	/* DPI->(e)DP interface clock limitation: upto 154 MHz */
+	return tc_edp_common_atomic_check(bridge, bridge_state, crtc_state,
+					  conn_state, 154000);
+}
+
 static enum drm_mode_status
 tc_edp_mode_valid(struct drm_bridge *bridge,
 		  const struct drm_display_info *info,
@@ -1463,6 +1488,7 @@ static const struct drm_bridge_funcs tc_bridge_funcs = {
 	.detach = tc_edp_bridge_detach,
 	.mode_valid = tc_edp_mode_valid,
 	.mode_set = tc_bridge_mode_set,
+	.atomic_check = tc_edp_atomic_check,
 	.atomic_enable = tc_edp_bridge_atomic_enable,
 	.atomic_disable = tc_edp_bridge_atomic_disable,
 	.mode_fixup = tc_bridge_mode_fixup,
-- 
2.34.1

Re: [PATCH V2 04/11] drm/bridge: tc358767: Implement atomic_check callback

[Lucas Stach]

Am Freitag, dem 18.02.2022 um 02:00 +0100 schrieb Marek Vasut:
> Implement .atomic_check callback which prevents user space from setting
> unsupported mode. The tc_edp_common_atomic_check() variant is already
> prepared for DSI-to-DPI mode addition, which has different frequency
> limits.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> ---
> V2: - New patch
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 522c2c4d8514f..01d11adee6c74 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1289,6 +1289,31 @@ static bool tc_bridge_mode_fixup(struct drm_bridge *bridge,
>  	return true;
>  }
>  
> +static int tc_edp_common_atomic_check(struct drm_bridge *bridge,

Drop the edp in the name here? Later in the series you call this
function from the DPI code, so this breaks the nice clean naming
separation from patch 1.

> +				      struct drm_bridge_state *bridge_state,
> +				      struct drm_crtc_state *crtc_state,
> +				      struct drm_connector_state *conn_state,
> +				      const unsigned int max_khz)
> +{
> +	tc_bridge_mode_fixup(bridge, &crtc_state->mode,
> +			     &crtc_state->adjusted_mode);
> +
> +	if (crtc_state->adjusted_mode.clock > max_khz)
> +		crtc_state->adjusted_mode.clock = max_khz;

I don't think this is correct. The adjusted most is just for minor
adjustments if the bridge can not fully match the mode. If userspace
supplies a invalid high modeclock I think it would be better to fail
the atomic check -> return -EINVAL

Regards,
Lucas

> +
> +	return 0;
> +}
> +
> +static int tc_edp_atomic_check(struct drm_bridge *bridge,
> +			       struct drm_bridge_state *bridge_state,
> +			       struct drm_crtc_state *crtc_state,
> +			       struct drm_connector_state *conn_state)
> +{
> +	/* DPI->(e)DP interface clock limitation: upto 154 MHz */
> +	return tc_edp_common_atomic_check(bridge, bridge_state, crtc_state,
> +					  conn_state, 154000);
> +}
> +
>  static enum drm_mode_status
>  tc_edp_mode_valid(struct drm_bridge *bridge,
>  		  const struct drm_display_info *info,
> @@ -1463,6 +1488,7 @@ static const struct drm_bridge_funcs tc_bridge_funcs = {
>  	.detach = tc_edp_bridge_detach,
>  	.mode_valid = tc_edp_mode_valid,
>  	.mode_set = tc_bridge_mode_set,
> +	.atomic_check = tc_edp_atomic_check,
>  	.atomic_enable = tc_edp_bridge_atomic_enable,
>  	.atomic_disable = tc_edp_bridge_atomic_disable,
>  	.mode_fixup = tc_bridge_mode_fixup,

Re: [PATCH V2 04/11] drm/bridge: tc358767: Implement atomic_check callback

[Marek Vasut]

On 2/18/22 18:34, Lucas Stach wrote:

Hi

[...]

>>   drivers/gpu/drm/bridge/tc358767.c | 26 ++++++++++++++++++++++++++
>>   1 file changed, 26 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
>> index 522c2c4d8514f..01d11adee6c74 100644
>> --- a/drivers/gpu/drm/bridge/tc358767.c
>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>> @@ -1289,6 +1289,31 @@ static bool tc_bridge_mode_fixup(struct drm_bridge *bridge,
>>   	return true;
>>   }
>>   
>> +static int tc_edp_common_atomic_check(struct drm_bridge *bridge,
> 
> Drop the edp in the name here? Later in the series you call this
> function from the DPI code, so this breaks the nice clean naming
> separation from patch 1.
> 
>> +				      struct drm_bridge_state *bridge_state,
>> +				      struct drm_crtc_state *crtc_state,
>> +				      struct drm_connector_state *conn_state,
>> +				      const unsigned int max_khz)
>> +{
>> +	tc_bridge_mode_fixup(bridge, &crtc_state->mode,
>> +			     &crtc_state->adjusted_mode);
>> +
>> +	if (crtc_state->adjusted_mode.clock > max_khz)
>> +		crtc_state->adjusted_mode.clock = max_khz;
> 
> I don't think this is correct. The adjusted most is just for minor
> adjustments if the bridge can not fully match the mode. If userspace
> supplies a invalid high modeclock I think it would be better to fail
> the atomic check -> return -EINVAL

Maxime was telling me that returning -EINVAL from atomic_check is weird, 
so maybe we should also wait for his opinion on this part.

Re: [PATCH V2 04/11] drm/bridge: tc358767: Implement atomic_check callback

[Maxime Ripard]

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

On Sat, Feb 19, 2022 at 03:26:40AM +0100, Marek Vasut wrote:
> On 2/18/22 18:34, Lucas Stach wrote:
> 
> Hi
> 
> [...]
> 
> > >   drivers/gpu/drm/bridge/tc358767.c | 26 ++++++++++++++++++++++++++
> > >   1 file changed, 26 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > > index 522c2c4d8514f..01d11adee6c74 100644
> > > --- a/drivers/gpu/drm/bridge/tc358767.c
> > > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > > @@ -1289,6 +1289,31 @@ static bool tc_bridge_mode_fixup(struct drm_bridge *bridge,
> > >   	return true;
> > >   }
> > > +static int tc_edp_common_atomic_check(struct drm_bridge *bridge,
> > 
> > Drop the edp in the name here? Later in the series you call this
> > function from the DPI code, so this breaks the nice clean naming
> > separation from patch 1.
> > 
> > > +				      struct drm_bridge_state *bridge_state,
> > > +				      struct drm_crtc_state *crtc_state,
> > > +				      struct drm_connector_state *conn_state,
> > > +				      const unsigned int max_khz)
> > > +{
> > > +	tc_bridge_mode_fixup(bridge, &crtc_state->mode,
> > > +			     &crtc_state->adjusted_mode);
> > > +
> > > +	if (crtc_state->adjusted_mode.clock > max_khz)
> > > +		crtc_state->adjusted_mode.clock = max_khz;
> > 
> > I don't think this is correct. The adjusted most is just for minor
> > adjustments if the bridge can not fully match the mode. If userspace
> > supplies a invalid high modeclock I think it would be better to fail
> > the atomic check -> return -EINVAL
> 
> Maxime was telling me that returning -EINVAL from atomic_check is weird, so
> maybe we should also wait for his opinion on this part.

That was in a completely different context?

Our discussion was about how you would propagate clock constraints
across a pipeline, and I was telling you that it would be weird to
return -EINVAL for a mode that was reported on a connector as supported
(or even preferred).

My argument was for mode_valid to filter them out.

If your clock is way above what you can support on your device, then
returning an error in atomic_check is the right thing to do.

Maxime

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

Re: [PATCH V2 04/11] drm/bridge: tc358767: Implement atomic_check callback

[Marek Vasut]

On 2/21/22 10:01, Maxime Ripard wrote:
> On Sat, Feb 19, 2022 at 03:26:40AM +0100, Marek Vasut wrote:
>> On 2/18/22 18:34, Lucas Stach wrote:
>>
>> Hi
>>
>> [...]
>>
>>>>    drivers/gpu/drm/bridge/tc358767.c | 26 ++++++++++++++++++++++++++
>>>>    1 file changed, 26 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
>>>> index 522c2c4d8514f..01d11adee6c74 100644
>>>> --- a/drivers/gpu/drm/bridge/tc358767.c
>>>> +++ b/drivers/gpu/drm/bridge/tc358767.c
>>>> @@ -1289,6 +1289,31 @@ static bool tc_bridge_mode_fixup(struct drm_bridge *bridge,
>>>>    	return true;
>>>>    }
>>>> +static int tc_edp_common_atomic_check(struct drm_bridge *bridge,
>>>
>>> Drop the edp in the name here? Later in the series you call this
>>> function from the DPI code, so this breaks the nice clean naming
>>> separation from patch 1.
>>>
>>>> +				      struct drm_bridge_state *bridge_state,
>>>> +				      struct drm_crtc_state *crtc_state,
>>>> +				      struct drm_connector_state *conn_state,
>>>> +				      const unsigned int max_khz)
>>>> +{
>>>> +	tc_bridge_mode_fixup(bridge, &crtc_state->mode,
>>>> +			     &crtc_state->adjusted_mode);
>>>> +
>>>> +	if (crtc_state->adjusted_mode.clock > max_khz)
>>>> +		crtc_state->adjusted_mode.clock = max_khz;
>>>
>>> I don't think this is correct. The adjusted most is just for minor
>>> adjustments if the bridge can not fully match the mode. If userspace
>>> supplies a invalid high modeclock I think it would be better to fail
>>> the atomic check -> return -EINVAL
>>
>> Maxime was telling me that returning -EINVAL from atomic_check is weird, so
>> maybe we should also wait for his opinion on this part.
> 
> That was in a completely different context?
> 
> Our discussion was about how you would propagate clock constraints
> across a pipeline, and I was telling you that it would be weird to
> return -EINVAL for a mode that was reported on a connector as supported
> (or even preferred).
> 
> My argument was for mode_valid to filter them out.
> 
> If your clock is way above what you can support on your device, then
> returning an error in atomic_check is the right thing to do.

Ah OK

[PATCH V2 05/11] drm/bridge: tc358767: Move hardware init to enable callback

[Marek Vasut]

The TC358767/TC358867/TC9595 are all capable of operating either from
attached Xtal or from DSI clock lane clock. In case the later is used,
all I2C accesses will fail until the DSI clock lane is running and
supplying clock to the chip.

Move all hardware initialization to enable callback to guarantee the
DSI clock lane is running before accessing the hardware. In case Xtal
is attached to the chip, this change has no effect.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
V2: - Rebase on next-20220217
---
 drivers/gpu/drm/bridge/tc358767.c | 111 +++++++++++++++++-------------
 1 file changed, 63 insertions(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 01d11adee6c74..134c4d8621236 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1234,6 +1234,63 @@ static int tc_edp_stream_disable(struct tc_data *tc)
 	return 0;
 }
 
+static int tc_hardware_init(struct tc_data *tc)
+{
+	int ret;
+
+	ret = regmap_read(tc->regmap, TC_IDREG, &tc->rev);
+	if (ret) {
+		dev_err(tc->dev, "can not read device ID: %d\n", ret);
+		return ret;
+	}
+
+	if ((tc->rev != 0x6601) && (tc->rev != 0x6603)) {
+		dev_err(tc->dev, "invalid device ID: 0x%08x\n", tc->rev);
+		return -EINVAL;
+	}
+
+	tc->assr = (tc->rev == 0x6601); /* Enable ASSR for eDP panels */
+
+	if (!tc->reset_gpio) {
+		/*
+		 * If the reset pin isn't present, do a software reset. It isn't
+		 * as thorough as the hardware reset, as we can't reset the I2C
+		 * communication block for obvious reasons, but it's getting the
+		 * chip into a defined state.
+		 */
+		regmap_update_bits(tc->regmap, SYSRSTENB,
+				ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP,
+				0);
+		regmap_update_bits(tc->regmap, SYSRSTENB,
+				ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP,
+				ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP);
+		usleep_range(5000, 10000);
+	}
+
+	if (tc->hpd_pin >= 0) {
+		u32 lcnt_reg = tc->hpd_pin == 0 ? INT_GP0_LCNT : INT_GP1_LCNT;
+		u32 h_lc = INT_GPIO_H(tc->hpd_pin) | INT_GPIO_LC(tc->hpd_pin);
+
+		/* Set LCNT to 2ms */
+		regmap_write(tc->regmap, lcnt_reg,
+			     clk_get_rate(tc->refclk) * 2 / 1000);
+		/* We need the "alternate" mode for HPD */
+		regmap_write(tc->regmap, GPIOM, BIT(tc->hpd_pin));
+
+		if (tc->have_irq) {
+			/* enable H & LC */
+			regmap_update_bits(tc->regmap, INTCTL_G, h_lc, h_lc);
+		}
+	}
+
+	if (tc->have_irq) {
+		/* enable SysErr */
+		regmap_write(tc->regmap, INTCTL_G, INT_SYSERR);
+	}
+
+	return 0;
+}
+
 static void
 tc_edp_bridge_atomic_enable(struct drm_bridge *bridge,
 			    struct drm_bridge_state *old_bridge_state)
@@ -1241,6 +1298,12 @@ tc_edp_bridge_atomic_enable(struct drm_bridge *bridge,
 	struct tc_data *tc = bridge_to_tc(bridge);
 	int ret;
 
+	ret = tc_hardware_init(tc);
+	if (ret < 0) {
+		dev_err(tc->dev, "failed to initialize bridge: %d\n", ret);
+		return;
+	}
+
 	ret = tc_get_display_props(tc);
 	if (ret < 0) {
 		dev_err(tc->dev, "failed to read display props: %d\n", ret);
@@ -1660,9 +1723,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	}
 
 	if (client->irq > 0) {
-		/* enable SysErr */
-		regmap_write(tc->regmap, INTCTL_G, INT_SYSERR);
-
 		ret = devm_request_threaded_irq(dev, client->irq,
 						NULL, tc_irq_handler,
 						IRQF_ONESHOT,
@@ -1675,51 +1735,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		tc->have_irq = true;
 	}
 
-	ret = regmap_read(tc->regmap, TC_IDREG, &tc->rev);
-	if (ret) {
-		dev_err(tc->dev, "can not read device ID: %d\n", ret);
-		return ret;
-	}
-
-	if ((tc->rev != 0x6601) && (tc->rev != 0x6603)) {
-		dev_err(tc->dev, "invalid device ID: 0x%08x\n", tc->rev);
-		return -EINVAL;
-	}
-
-	tc->assr = (tc->rev == 0x6601); /* Enable ASSR for eDP panels */
-
-	if (!tc->reset_gpio) {
-		/*
-		 * If the reset pin isn't present, do a software reset. It isn't
-		 * as thorough as the hardware reset, as we can't reset the I2C
-		 * communication block for obvious reasons, but it's getting the
-		 * chip into a defined state.
-		 */
-		regmap_update_bits(tc->regmap, SYSRSTENB,
-				ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP,
-				0);
-		regmap_update_bits(tc->regmap, SYSRSTENB,
-				ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP,
-				ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP);
-		usleep_range(5000, 10000);
-	}
-
-	if (tc->hpd_pin >= 0) {
-		u32 lcnt_reg = tc->hpd_pin == 0 ? INT_GP0_LCNT : INT_GP1_LCNT;
-		u32 h_lc = INT_GPIO_H(tc->hpd_pin) | INT_GPIO_LC(tc->hpd_pin);
-
-		/* Set LCNT to 2ms */
-		regmap_write(tc->regmap, lcnt_reg,
-			     clk_get_rate(tc->refclk) * 2 / 1000);
-		/* We need the "alternate" mode for HPD */
-		regmap_write(tc->regmap, GPIOM, BIT(tc->hpd_pin));
-
-		if (tc->have_irq) {
-			/* enable H & LC */
-			regmap_update_bits(tc->regmap, INTCTL_G, h_lc, h_lc);
-		}
-	}
-
 	ret = tc_aux_link_setup(tc);
 	if (ret)
 		return ret;
-- 
2.34.1

Re: [PATCH V2 05/11] drm/bridge: tc358767: Move hardware init to enable callback

[Lucas Stach]

Am Freitag, dem 18.02.2022 um 02:00 +0100 schrieb Marek Vasut:
> The TC358767/TC358867/TC9595 are all capable of operating either from
> attached Xtal or from DSI clock lane clock. In case the later is used,
> all I2C accesses will fail until the DSI clock lane is running and
> supplying clock to the chip.
> 
> Move all hardware initialization to enable callback to guarantee the
> DSI clock lane is running before accessing the hardware. In case Xtal
> is attached to the chip, this change has no effect.

I'm not sure if that last statement is correct. When the chip is
bridging to eDP, the aux channel and HPD handling is needed to be
functional way before the atomic enable happen. I have no idea how this
would interact with the clock supplied from the DSI lanes. Maybe it
doesn't work at all and proper eDP support always needs a external
reference clock?

I think we should make the "ref" clock a optional clock to properly
describe the fact that the chip can operate without this clock in DSI
input mode and then either do the chip init in the probe routine when
the ref clock is present, or defer it to atomic enable when the ref
clock is absent.

> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> ---
> V2: - Rebase on next-20220217
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 111 +++++++++++++++++-------------
>  1 file changed, 63 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 01d11adee6c74..134c4d8621236 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1234,6 +1234,63 @@ static int tc_edp_stream_disable(struct tc_data *tc)
>  	return 0;
>  }
>  
> +static int tc_hardware_init(struct tc_data *tc)
> +{
> +	int ret;
> +
> +	ret = regmap_read(tc->regmap, TC_IDREG, &tc->rev);
> +	if (ret) {
> +		dev_err(tc->dev, "can not read device ID: %d\n", ret);
> +		return ret;
> +	}
> +
> +	if ((tc->rev != 0x6601) && (tc->rev != 0x6603)) {
> +		dev_err(tc->dev, "invalid device ID: 0x%08x\n", tc->rev);
> +		return -EINVAL;
> +	}
> +
> +	tc->assr = (tc->rev == 0x6601); /* Enable ASSR for eDP panels */
> +
> +	if (!tc->reset_gpio) {
> +		/*
> +		 * If the reset pin isn't present, do a software reset. It isn't
> +		 * as thorough as the hardware reset, as we can't reset the I2C
> +		 * communication block for obvious reasons, but it's getting the
> +		 * chip into a defined state.
> +		 */
> +		regmap_update_bits(tc->regmap, SYSRSTENB,
> +				ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP,
> +				0);
> +		regmap_update_bits(tc->regmap, SYSRSTENB,
> +				ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP,
> +				ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP);
> +		usleep_range(5000, 10000);
> +	}
> +
> +	if (tc->hpd_pin >= 0) {
> +		u32 lcnt_reg = tc->hpd_pin == 0 ? INT_GP0_LCNT : INT_GP1_LCNT;
> +		u32 h_lc = INT_GPIO_H(tc->hpd_pin) | INT_GPIO_LC(tc->hpd_pin);
> +
> +		/* Set LCNT to 2ms */
> +		regmap_write(tc->regmap, lcnt_reg,
> +			     clk_get_rate(tc->refclk) * 2 / 1000);
> +		/* We need the "alternate" mode for HPD */
> +		regmap_write(tc->regmap, GPIOM, BIT(tc->hpd_pin));
> +
> +		if (tc->have_irq) {
> +			/* enable H & LC */
> +			regmap_update_bits(tc->regmap, INTCTL_G, h_lc, h_lc);
> +		}
> +	}
> +
> +	if (tc->have_irq) {
> +		/* enable SysErr */
> +		regmap_write(tc->regmap, INTCTL_G, INT_SYSERR);
> +	}
> +
> +	return 0;
> +}
> +
>  static void
>  tc_edp_bridge_atomic_enable(struct drm_bridge *bridge,
>  			    struct drm_bridge_state *old_bridge_state)
> @@ -1241,6 +1298,12 @@ tc_edp_bridge_atomic_enable(struct drm_bridge *bridge,
>  	struct tc_data *tc = bridge_to_tc(bridge);
>  	int ret;
>  
> +	ret = tc_hardware_init(tc);
> +	if (ret < 0) {
> +		dev_err(tc->dev, "failed to initialize bridge: %d\n", ret);
> +		return;
> +	}
> +
>  	ret = tc_get_display_props(tc);
>  	if (ret < 0) {
>  		dev_err(tc->dev, "failed to read display props: %d\n", ret);
> @@ -1660,9 +1723,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	}
>  
>  	if (client->irq > 0) {
> -		/* enable SysErr */
> -		regmap_write(tc->regmap, INTCTL_G, INT_SYSERR);
> -
>  		ret = devm_request_threaded_irq(dev, client->irq,
>  						NULL, tc_irq_handler,
>  						IRQF_ONESHOT,
> @@ -1675,51 +1735,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  		tc->have_irq = true;
>  	}
>  
> -	ret = regmap_read(tc->regmap, TC_IDREG, &tc->rev);
> -	if (ret) {
> -		dev_err(tc->dev, "can not read device ID: %d\n", ret);
> -		return ret;
> -	}
> -
> -	if ((tc->rev != 0x6601) && (tc->rev != 0x6603)) {
> -		dev_err(tc->dev, "invalid device ID: 0x%08x\n", tc->rev);
> -		return -EINVAL;
> -	}
> -
> -	tc->assr = (tc->rev == 0x6601); /* Enable ASSR for eDP panels */
> -
> -	if (!tc->reset_gpio) {
> -		/*
> -		 * If the reset pin isn't present, do a software reset. It isn't
> -		 * as thorough as the hardware reset, as we can't reset the I2C
> -		 * communication block for obvious reasons, but it's getting the
> -		 * chip into a defined state.
> -		 */
> -		regmap_update_bits(tc->regmap, SYSRSTENB,
> -				ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP,
> -				0);
> -		regmap_update_bits(tc->regmap, SYSRSTENB,
> -				ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP,
> -				ENBLCD0 | ENBBM | ENBDSIRX | ENBREG | ENBHDCP);
> -		usleep_range(5000, 10000);
> -	}
> -
> -	if (tc->hpd_pin >= 0) {
> -		u32 lcnt_reg = tc->hpd_pin == 0 ? INT_GP0_LCNT : INT_GP1_LCNT;
> -		u32 h_lc = INT_GPIO_H(tc->hpd_pin) | INT_GPIO_LC(tc->hpd_pin);
> -
> -		/* Set LCNT to 2ms */
> -		regmap_write(tc->regmap, lcnt_reg,
> -			     clk_get_rate(tc->refclk) * 2 / 1000);
> -		/* We need the "alternate" mode for HPD */
> -		regmap_write(tc->regmap, GPIOM, BIT(tc->hpd_pin));
> -
> -		if (tc->have_irq) {
> -			/* enable H & LC */
> -			regmap_update_bits(tc->regmap, INTCTL_G, h_lc, h_lc);
> -		}
> -	}
> -
>  	ret = tc_aux_link_setup(tc);

The above function is also poking i2c regs, as will any DP AUX
transaction in case of eDP.

Regards,
Lucas

>  	if (ret)
>  		return ret;

Re: [PATCH V2 05/11] drm/bridge: tc358767: Move hardware init to enable callback

[Marek Vasut]

On 2/18/22 18:49, Lucas Stach wrote:
> Am Freitag, dem 18.02.2022 um 02:00 +0100 schrieb Marek Vasut:
>> The TC358767/TC358867/TC9595 are all capable of operating either from
>> attached Xtal or from DSI clock lane clock. In case the later is used,
>> all I2C accesses will fail until the DSI clock lane is running and
>> supplying clock to the chip.
>>
>> Move all hardware initialization to enable callback to guarantee the
>> DSI clock lane is running before accessing the hardware. In case Xtal
>> is attached to the chip, this change has no effect.
> 
> I'm not sure if that last statement is correct. When the chip is
> bridging to eDP, the aux channel and HPD handling is needed to be
> functional way before the atomic enable happen. I have no idea how this
> would interact with the clock supplied from the DSI lanes. Maybe it
> doesn't work at all and proper eDP support always needs a external
> reference clock?

The driver currently assumes the TC358767 always gets RefClk from Xtal.

There is subsequent series which adds support for deriving clock which 
drive the TC358767 PLLs from DSI HS clock instead of Xtal in case the 
bridge operates in DSI-to-DPI mode. That needs additional plumbing, as 
the TC358767 must be able to select specific clock frequency on the DSI 
HS clock lane, because its PLLs need specific frequencies, see:

[RFC][PATCH 0/7] drm/bridge: Add support for selecting DSI host HS clock 
from DSI bridge

If someone needs to implement DSI-to-(e)DP mode without Xtal, ugh, that 
would likely need to have a way to figure out the DSI HS clock frequency 
already in probe and then enable those DSI HS clock very early on too ?

> I think we should make the "ref" clock a optional clock to properly
> describe the fact that the chip can operate without this clock in DSI
> input mode and then either do the chip init in the probe routine when
> the ref clock is present, or defer it to atomic enable when the ref
> clock is absent.

See the RFC patchset above, that patchset does exactly that, it makes 
RefClk optional.

[...]

Re: [PATCH V2 05/11] drm/bridge: tc358767: Move hardware init to enable callback

[Lucas Stach]

Am Samstag, dem 19.02.2022 um 03:39 +0100 schrieb Marek Vasut:
> On 2/18/22 18:49, Lucas Stach wrote:
> > Am Freitag, dem 18.02.2022 um 02:00 +0100 schrieb Marek Vasut:
> > > The TC358767/TC358867/TC9595 are all capable of operating either from
> > > attached Xtal or from DSI clock lane clock. In case the later is used,
> > > all I2C accesses will fail until the DSI clock lane is running and
> > > supplying clock to the chip.
> > > 
> > > Move all hardware initialization to enable callback to guarantee the
> > > DSI clock lane is running before accessing the hardware. In case Xtal
> > > is attached to the chip, this change has no effect.
> > 
> > I'm not sure if that last statement is correct. When the chip is
> > bridging to eDP, the aux channel and HPD handling is needed to be
> > functional way before the atomic enable happen. I have no idea how this
> > would interact with the clock supplied from the DSI lanes. Maybe it
> > doesn't work at all and proper eDP support always needs a external
> > reference clock?
> 
> The driver currently assumes the TC358767 always gets RefClk from Xtal.
> 
> There is subsequent series which adds support for deriving clock which 
> drive the TC358767 PLLs from DSI HS clock instead of Xtal in case the 
> bridge operates in DSI-to-DPI mode. That needs additional plumbing, as 
> the TC358767 must be able to select specific clock frequency on the DSI 
> HS clock lane, because its PLLs need specific frequencies, see:
> 
> [RFC][PATCH 0/7] drm/bridge: Add support for selecting DSI host HS clock 
> from DSI bridge
> 
> If someone needs to implement DSI-to-(e)DP mode without Xtal, ugh, that 
> would likely need to have a way to figure out the DSI HS clock frequency 
> already in probe and then enable those DSI HS clock very early on too ?
> 
Probably, but that was really just something I wondered about while
passing by.

The real issue is that I think _this_ patch breaks eDP operation, as
now the chip is initialized way too late, as the AUX channel
functionality needs to be available long before the atomic bridge
enable callback is called. The AUX channel is used to fetch the display
EDID, so before you can even set a mode it needs to be functional.
Unconditionally moving the chip init is probably (I haven't tested it
yet) going to break this.

Regards,
Lucas

> > I think we should make the "ref" clock a optional clock to properly
> > describe the fact that the chip can operate without this clock in DSI
> > input mode and then either do the chip init in the probe routine when
> > the ref clock is present, or defer it to atomic enable when the ref
> > clock is absent.
> 
> See the RFC patchset above, that patchset does exactly that, it makes 
> RefClk optional.
> 
> [...]

Re: [PATCH V2 05/11] drm/bridge: tc358767: Move hardware init to enable callback

[Marek Vasut]

On 2/21/22 10:12, Lucas Stach wrote:
> Am Samstag, dem 19.02.2022 um 03:39 +0100 schrieb Marek Vasut:
>> On 2/18/22 18:49, Lucas Stach wrote:
>>> Am Freitag, dem 18.02.2022 um 02:00 +0100 schrieb Marek Vasut:
>>>> The TC358767/TC358867/TC9595 are all capable of operating either from
>>>> attached Xtal or from DSI clock lane clock. In case the later is used,
>>>> all I2C accesses will fail until the DSI clock lane is running and
>>>> supplying clock to the chip.
>>>>
>>>> Move all hardware initialization to enable callback to guarantee the
>>>> DSI clock lane is running before accessing the hardware. In case Xtal
>>>> is attached to the chip, this change has no effect.
>>>
>>> I'm not sure if that last statement is correct. When the chip is
>>> bridging to eDP, the aux channel and HPD handling is needed to be
>>> functional way before the atomic enable happen. I have no idea how this
>>> would interact with the clock supplied from the DSI lanes. Maybe it
>>> doesn't work at all and proper eDP support always needs a external
>>> reference clock?
>>
>> The driver currently assumes the TC358767 always gets RefClk from Xtal.
>>
>> There is subsequent series which adds support for deriving clock which
>> drive the TC358767 PLLs from DSI HS clock instead of Xtal in case the
>> bridge operates in DSI-to-DPI mode. That needs additional plumbing, as
>> the TC358767 must be able to select specific clock frequency on the DSI
>> HS clock lane, because its PLLs need specific frequencies, see:
>>
>> [RFC][PATCH 0/7] drm/bridge: Add support for selecting DSI host HS clock
>> from DSI bridge
>>
>> If someone needs to implement DSI-to-(e)DP mode without Xtal, ugh, that
>> would likely need to have a way to figure out the DSI HS clock frequency
>> already in probe and then enable those DSI HS clock very early on too ?
>>
> Probably, but that was really just something I wondered about while
> passing by.
> 
> The real issue is that I think _this_ patch breaks eDP operation, as
> now the chip is initialized way too late, as the AUX channel
> functionality needs to be available long before the atomic bridge
> enable callback is called.

I don't think that's correct -- look at the tc_hardware_init() function, 
all it does is it reads the chip ID, optionally does software reset if 
there is no reset GPIO connected, and then sets up hotplug detect IRQ. 
There is nothing specific to bringing up the AUX channel in that 
function, so the AUX channel should work even before tc_hardware_init() 
is called.

> The AUX channel is used to fetch the display
> EDID, so before you can even set a mode it needs to be functional.
> Unconditionally moving the chip init is probably (I haven't tested it
> yet) going to break this.

If you have such a setup with eDP, please test it, I don't think this is 
going to break it.

[PATCH V2 06/11] drm/bridge: tc358767: Move (e)DP bridge endpoint parsing into dedicated function

[Marek Vasut]

The TC358767/TC358867/TC9595 are all capable of operating in multiple
modes, DPI-to-(e)DP, DSI-to-(e)DP, DSI-to-DPI. Only the first mode is
currently supported. In order to support the rest of the modes without
making the tc_probe() overly long, split the bridge endpoint parsing
into dedicated function, where the necessary logic to detect the bridge
mode based on which endpoints are connected, can be implemented.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
V2: - Rename tc_probe_bridge_mode() to tc_probe_edp_bridge_endpoint()
      to better reflect that it parses the (e)DP output endpoint
---
 drivers/gpu/drm/bridge/tc358767.c | 30 +++++++++++++++++++++---------
 1 file changed, 21 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 134c4d8621236..450a472888ba9 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1646,19 +1646,12 @@ static irqreturn_t tc_irq_handler(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
-static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
+static int tc_probe_edp_bridge_endpoint(struct tc_data *tc)
 {
-	struct device *dev = &client->dev;
+	struct device *dev = tc->dev;
 	struct drm_panel *panel;
-	struct tc_data *tc;
 	int ret;
 
-	tc = devm_kzalloc(dev, sizeof(*tc), GFP_KERNEL);
-	if (!tc)
-		return -ENOMEM;
-
-	tc->dev = dev;
-
 	/* port@2 is the output port */
 	ret = drm_of_find_panel_or_bridge(dev->of_node, 2, 0, &panel, NULL);
 	if (ret && ret != -ENODEV)
@@ -1677,6 +1670,25 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		tc->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
 	}
 
+	return ret;
+}
+
+static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
+{
+	struct device *dev = &client->dev;
+	struct tc_data *tc;
+	int ret;
+
+	tc = devm_kzalloc(dev, sizeof(*tc), GFP_KERNEL);
+	if (!tc)
+		return -ENOMEM;
+
+	tc->dev = dev;
+
+	ret = tc_probe_edp_bridge_endpoint(tc);
+	if (ret)
+		return ret;
+
 	/* Shut down GPIO is optional */
 	tc->sd_gpio = devm_gpiod_get_optional(dev, "shutdown", GPIOD_OUT_HIGH);
 	if (IS_ERR(tc->sd_gpio))
-- 
2.34.1

Re: [PATCH V2 06/11] drm/bridge: tc358767: Move (e)DP bridge endpoint parsing into dedicated function

[Lucas Stach]

Am Freitag, dem 18.02.2022 um 02:00 +0100 schrieb Marek Vasut:
> The TC358767/TC358867/TC9595 are all capable of operating in multiple
> modes, DPI-to-(e)DP, DSI-to-(e)DP, DSI-to-DPI. Only the first mode is
> currently supported. In order to support the rest of the modes without
> making the tc_probe() overly long, split the bridge endpoint parsing
> into dedicated function, where the necessary logic to detect the bridge
> mode based on which endpoints are connected, can be implemented.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
> V2: - Rename tc_probe_bridge_mode() to tc_probe_edp_bridge_endpoint()
>       to better reflect that it parses the (e)DP output endpoint
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 30 +++++++++++++++++++++---------
>  1 file changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 134c4d8621236..450a472888ba9 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1646,19 +1646,12 @@ static irqreturn_t tc_irq_handler(int irq, void *arg)
>  	return IRQ_HANDLED;
>  }
>  
> -static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +static int tc_probe_edp_bridge_endpoint(struct tc_data *tc)
>  {
> -	struct device *dev = &client->dev;
> +	struct device *dev = tc->dev;
>  	struct drm_panel *panel;
> -	struct tc_data *tc;
>  	int ret;
>  
> -	tc = devm_kzalloc(dev, sizeof(*tc), GFP_KERNEL);
> -	if (!tc)
> -		return -ENOMEM;
> -
> -	tc->dev = dev;
> -
>  	/* port@2 is the output port */
>  	ret = drm_of_find_panel_or_bridge(dev->of_node, 2, 0, &panel, NULL);
>  	if (ret && ret != -ENODEV)
> @@ -1677,6 +1670,25 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  		tc->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
>  	}
>  
> +	return ret;
> +}
> +
> +static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
> +{
> +	struct device *dev = &client->dev;
> +	struct tc_data *tc;
> +	int ret;
> +
> +	tc = devm_kzalloc(dev, sizeof(*tc), GFP_KERNEL);
> +	if (!tc)
> +		return -ENOMEM;
> +
> +	tc->dev = dev;
> +
> +	ret = tc_probe_edp_bridge_endpoint(tc);
> +	if (ret)
> +		return ret;
> +
>  	/* Shut down GPIO is optional */
>  	tc->sd_gpio = devm_gpiod_get_optional(dev, "shutdown", GPIOD_OUT_HIGH);
>  	if (IS_ERR(tc->sd_gpio))

[PATCH V2 07/11] drm/bridge: tc358767: Wrap (e)DP aux I2C registration into tc_aux_link_setup()

[Marek Vasut]

This bit of code is (e)DP and aux I2C link specific, move it into
tc_aux_link_setup() to permit cleaner addition of DSI-to-DPI mode.
No functional change.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
V2: - New patch
---
 drivers/gpu/drm/bridge/tc358767.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 450a472888ba9..55b7f3fb9eec9 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -656,6 +656,12 @@ static int tc_aux_link_setup(struct tc_data *tc)
 	if (ret)
 		goto err;
 
+	/* Register DP AUX channel */
+	tc->aux.name = "TC358767 AUX i2c adapter";
+	tc->aux.dev = tc->dev;
+	tc->aux.transfer = tc_aux_transfer;
+	drm_dp_aux_init(&tc->aux);
+
 	return 0;
 err:
 	dev_err(tc->dev, "tc_aux_link_setup failed: %d\n", ret);
@@ -1751,12 +1757,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	if (ret)
 		return ret;
 
-	/* Register DP AUX channel */
-	tc->aux.name = "TC358767 AUX i2c adapter";
-	tc->aux.dev = tc->dev;
-	tc->aux.transfer = tc_aux_transfer;
-	drm_dp_aux_init(&tc->aux);
-
 	tc->bridge.funcs = &tc_bridge_funcs;
 	if (tc->hpd_pin >= 0)
 		tc->bridge.ops |= DRM_BRIDGE_OP_DETECT;
-- 
2.34.1

Re: [PATCH V2 07/11] drm/bridge: tc358767: Wrap (e)DP aux I2C registration into tc_aux_link_setup()

[Lucas Stach]

Am Freitag, dem 18.02.2022 um 02:00 +0100 schrieb Marek Vasut:
> This bit of code is (e)DP and aux I2C link specific, move it into
> tc_aux_link_setup() to permit cleaner addition of DSI-to-DPI mode.
> No functional change.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
> V2: - New patch
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 450a472888ba9..55b7f3fb9eec9 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -656,6 +656,12 @@ static int tc_aux_link_setup(struct tc_data *tc)
>  	if (ret)
>  		goto err;
>  
> +	/* Register DP AUX channel */
> +	tc->aux.name = "TC358767 AUX i2c adapter";
> +	tc->aux.dev = tc->dev;
> +	tc->aux.transfer = tc_aux_transfer;
> +	drm_dp_aux_init(&tc->aux);
> +
>  	return 0;
>  err:
>  	dev_err(tc->dev, "tc_aux_link_setup failed: %d\n", ret);
> @@ -1751,12 +1757,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	if (ret)
>  		return ret;
>  
> -	/* Register DP AUX channel */
> -	tc->aux.name = "TC358767 AUX i2c adapter";
> -	tc->aux.dev = tc->dev;
> -	tc->aux.transfer = tc_aux_transfer;
> -	drm_dp_aux_init(&tc->aux);
> -
>  	tc->bridge.funcs = &tc_bridge_funcs;
>  	if (tc->hpd_pin >= 0)
>  		tc->bridge.ops |= DRM_BRIDGE_OP_DETECT;

[PATCH V2 08/11] drm/bridge: tc358767: Move bridge ops setup into tc_probe_edp_bridge_endpoint()

[Marek Vasut]

The bridge ops are specific to the bridge configuration, move them
into tc_probe_edp_bridge_endpoint() to permit cleaner addition of
DSI-to-DPI mode. No functional change.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
V2: - New patch
---
 drivers/gpu/drm/bridge/tc358767.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 55b7f3fb9eec9..7dae18de76c97 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1676,6 +1676,11 @@ static int tc_probe_edp_bridge_endpoint(struct tc_data *tc)
 		tc->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
 	}
 
+	tc->bridge.funcs = &tc_bridge_funcs;
+	if (tc->hpd_pin >= 0)
+		tc->bridge.ops |= DRM_BRIDGE_OP_DETECT;
+	tc->bridge.ops |= DRM_BRIDGE_OP_EDID;
+
 	return ret;
 }
 
@@ -1757,11 +1762,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 	if (ret)
 		return ret;
 
-	tc->bridge.funcs = &tc_bridge_funcs;
-	if (tc->hpd_pin >= 0)
-		tc->bridge.ops |= DRM_BRIDGE_OP_DETECT;
-	tc->bridge.ops |= DRM_BRIDGE_OP_EDID;
-
 	tc->bridge.of_node = dev->of_node;
 	drm_bridge_add(&tc->bridge);
 
-- 
2.34.1

Re: [PATCH V2 08/11] drm/bridge: tc358767: Move bridge ops setup into tc_probe_edp_bridge_endpoint()

[Lucas Stach]

Am Freitag, dem 18.02.2022 um 02:00 +0100 schrieb Marek Vasut:
> The bridge ops are specific to the bridge configuration, move them
> into tc_probe_edp_bridge_endpoint() to permit cleaner addition of
> DSI-to-DPI mode. No functional change.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> ---
> V2: - New patch
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 55b7f3fb9eec9..7dae18de76c97 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1676,6 +1676,11 @@ static int tc_probe_edp_bridge_endpoint(struct tc_data *tc)
>  		tc->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
>  	}
>  
> +	tc->bridge.funcs = &tc_bridge_funcs;

Could you please also rename those to tc_edp_bridge_funcs? Otherwise I
agree with this patch.

> +	if (tc->hpd_pin >= 0)
> +		tc->bridge.ops |= DRM_BRIDGE_OP_DETECT;
> +	tc->bridge.ops |= DRM_BRIDGE_OP_EDID;
> +
>  	return ret;
>  }
>  
> @@ -1757,11 +1762,6 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  	if (ret)
>  		return ret;
>  
> -	tc->bridge.funcs = &tc_bridge_funcs;
> -	if (tc->hpd_pin >= 0)
> -		tc->bridge.ops |= DRM_BRIDGE_OP_DETECT;
> -	tc->bridge.ops |= DRM_BRIDGE_OP_EDID;
> -
>  	tc->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&tc->bridge);
>  

[PATCH V2 09/11] drm/bridge: tc358767: Detect bridge mode from connected endpoints in DT

[Marek Vasut]

The TC358767/TC358867/TC9595 are all capable of operating in multiple
modes, DPI-to-(e)DP, DSI-to-(e)DP, DSI-to-DPI. Only the first mode is
currently supported. It is possible to find out the mode in which the
bridge should be operated by testing connected endpoints in DT.

Port allocation:
port@0 - DSI input
port@1 - DPI input/output
port@2 - eDP output

Possible connections:
DPI -> port@1 -> port@2 -> eDP :: [port@0 is not connected]
DSI -> port@0 -> port@2 -> eDP :: [port@1 is not connected]
DSI -> port@0 -> port@1 -> DPI :: [port@2 is not connected]

Add function to determine the bridge mode based on connected endpoints.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
V2: - New patch
---
 drivers/gpu/drm/bridge/tc358767.c | 46 ++++++++++++++++++++++++++++++-
 1 file changed, 45 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 7dae18de76c97..4af0ad5db2148 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1684,6 +1684,50 @@ static int tc_probe_edp_bridge_endpoint(struct tc_data *tc)
 	return ret;
 }
 
+static int tc_probe_bridge_endpoint(struct tc_data *tc)
+{
+	struct device *dev = tc->dev;
+	struct of_endpoint endpoint;
+	struct device_node *node = NULL;
+	const u8 mode_dpi_to_edp = BIT(1) | BIT(2);
+	const u8 mode_dsi_to_edp = BIT(0) | BIT(2);
+	const u8 mode_dsi_to_dpi = BIT(0) | BIT(1);
+	u8 mode = 0;
+
+	/*
+	 * Determine bridge configuration.
+	 *
+	 * Port allocation:
+	 * port@0 - DSI input
+	 * port@1 - DPI input/output
+	 * port@2 - eDP output
+	 *
+	 * Possible connections:
+	 * DPI -> port@1 -> port@2 -> eDP :: [port@0 is not connected]
+	 * DSI -> port@0 -> port@2 -> eDP :: [port@1 is not connected]
+	 * DSI -> port@0 -> port@1 -> DPI :: [port@2 is not connected]
+	 */
+
+	for_each_endpoint_of_node(dev->of_node, node) {
+		of_graph_parse_endpoint(node, &endpoint);
+		if (endpoint.port > 2)
+			return -EINVAL;
+
+		mode |= BIT(endpoint.port);
+	}
+
+	if (mode == mode_dpi_to_edp)
+		return tc_probe_edp_bridge_endpoint(tc);
+	else if (mode == mode_dsi_to_dpi)
+		dev_warn(dev, "The mode DSI-to-DPI is not supported!\n");
+	else if (mode == mode_dsi_to_edp)
+		dev_warn(dev, "The mode DSI-to-(e)DP is not supported!\n");
+	else
+		dev_warn(dev, "Invalid mode (0x%x) is not supported!\n", mode);
+
+	return -EINVAL;
+}
+
 static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 {
 	struct device *dev = &client->dev;
@@ -1696,7 +1740,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 
 	tc->dev = dev;
 
-	ret = tc_probe_edp_bridge_endpoint(tc);
+	ret = tc_probe_bridge_endpoint(tc);
 	if (ret)
 		return ret;
 
-- 
2.34.1

Re: [PATCH V2 09/11] drm/bridge: tc358767: Detect bridge mode from connected endpoints in DT

[Lucas Stach]

Am Freitag, dem 18.02.2022 um 02:00 +0100 schrieb Marek Vasut:
> The TC358767/TC358867/TC9595 are all capable of operating in multiple
> modes, DPI-to-(e)DP, DSI-to-(e)DP, DSI-to-DPI. Only the first mode is
> currently supported. It is possible to find out the mode in which the
> bridge should be operated by testing connected endpoints in DT.
> 
> Port allocation:
> port@0 - DSI input
> port@1 - DPI input/output
> port@2 - eDP output
> 
> Possible connections:
> DPI -> port@1 -> port@2 -> eDP :: [port@0 is not connected]
> DSI -> port@0 -> port@2 -> eDP :: [port@1 is not connected]
> DSI -> port@0 -> port@1 -> DPI :: [port@2 is not connected]
> 
> Add function to determine the bridge mode based on connected endpoints.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
> V2: - New patch
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 46 ++++++++++++++++++++++++++++++-
>  1 file changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 7dae18de76c97..4af0ad5db2148 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1684,6 +1684,50 @@ static int tc_probe_edp_bridge_endpoint(struct tc_data *tc)
>  	return ret;
>  }
>  
> +static int tc_probe_bridge_endpoint(struct tc_data *tc)
> +{
> +	struct device *dev = tc->dev;
> +	struct of_endpoint endpoint;
> +	struct device_node *node = NULL;
> +	const u8 mode_dpi_to_edp = BIT(1) | BIT(2);
> +	const u8 mode_dsi_to_edp = BIT(0) | BIT(2);
> +	const u8 mode_dsi_to_dpi = BIT(0) | BIT(1);
> +	u8 mode = 0;
> +
> +	/*
> +	 * Determine bridge configuration.
> +	 *
> +	 * Port allocation:
> +	 * port@0 - DSI input
> +	 * port@1 - DPI input/output
> +	 * port@2 - eDP output
> +	 *
> +	 * Possible connections:
> +	 * DPI -> port@1 -> port@2 -> eDP :: [port@0 is not connected]
> +	 * DSI -> port@0 -> port@2 -> eDP :: [port@1 is not connected]
> +	 * DSI -> port@0 -> port@1 -> DPI :: [port@2 is not connected]
> +	 */
> +
> +	for_each_endpoint_of_node(dev->of_node, node) {
> +		of_graph_parse_endpoint(node, &endpoint);
> +		if (endpoint.port > 2)
> +			return -EINVAL;
> +
> +		mode |= BIT(endpoint.port);
> +	}
> +
> +	if (mode == mode_dpi_to_edp)
> +		return tc_probe_edp_bridge_endpoint(tc);
> +	else if (mode == mode_dsi_to_dpi)
> +		dev_warn(dev, "The mode DSI-to-DPI is not supported!\n");
> +	else if (mode == mode_dsi_to_edp)
> +		dev_warn(dev, "The mode DSI-to-(e)DP is not supported!\n");
> +	else
> +		dev_warn(dev, "Invalid mode (0x%x) is not supported!\n", mode);
> +
> +	return -EINVAL;
> +}
> +
>  static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  {
>  	struct device *dev = &client->dev;
> @@ -1696,7 +1740,7 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  
>  	tc->dev = dev;
>  
> -	ret = tc_probe_edp_bridge_endpoint(tc);
> +	ret = tc_probe_bridge_endpoint(tc);
>  	if (ret)
>  		return ret;
>  

[PATCH V2 10/11] drm/bridge: tc358767: Split tc_set_video_mode() into common and (e)DP part

[Marek Vasut]

The tc_set_video_mode() sets up both common and (e)DP video mode settings of
the bridge chip. Split the function into tc_set_common_video_mode() to set
the common settings and tc_set_edp_video_mode() to set the (e)DP specific
settings. No functional change.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
V2: - New patch
---
 drivers/gpu/drm/bridge/tc358767.c | 48 ++++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 4af0ad5db2148..091c969a36ab7 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -734,11 +734,10 @@ static int tc_get_display_props(struct tc_data *tc)
 	return ret;
 }
 
-static int tc_set_video_mode(struct tc_data *tc,
-			     const struct drm_display_mode *mode)
+static int tc_set_common_video_mode(struct tc_data *tc,
+				    const struct drm_display_mode *mode)
 {
 	int ret;
-	int vid_sync_dly;
 	int max_tu_symbol;
 
 	int left_margin = mode->htotal - mode->hsync_end;
@@ -747,7 +746,6 @@ static int tc_set_video_mode(struct tc_data *tc,
 	int upper_margin = mode->vtotal - mode->vsync_end;
 	int lower_margin = mode->vsync_start - mode->vdisplay;
 	int vsync_len = mode->vsync_end - mode->vsync_start;
-	u32 dp0_syncval;
 	u32 bits_per_pixel = 24;
 	u32 in_bw, out_bw;
 
@@ -818,8 +816,35 @@ static int tc_set_video_mode(struct tc_data *tc,
 			   FIELD_PREP(COLOR_B, 99) |
 			   ENI2CFILTER |
 			   FIELD_PREP(COLOR_BAR_MODE, COLOR_BAR_MODE_BARS));
-	if (ret)
-		return ret;
+
+	return ret;
+}
+
+static int tc_set_edp_video_mode(struct tc_data *tc,
+				 const struct drm_display_mode *mode)
+{
+	int ret;
+	int vid_sync_dly;
+	int max_tu_symbol;
+
+	int left_margin = mode->htotal - mode->hsync_end;
+	int hsync_len = mode->hsync_end - mode->hsync_start;
+	int upper_margin = mode->vtotal - mode->vsync_end;
+	int vsync_len = mode->vsync_end - mode->vsync_start;
+	u32 dp0_syncval;
+	u32 bits_per_pixel = 24;
+	u32 in_bw, out_bw;
+
+	/*
+	 * Recommended maximum number of symbols transferred in a transfer unit:
+	 * DIV_ROUND_UP((input active video bandwidth in bytes) * tu_size,
+	 *              (output active video bandwidth in bytes))
+	 * Must be less than tu_size.
+	 */
+
+	in_bw = mode->clock * bits_per_pixel / 8;
+	out_bw = tc->link.num_lanes * tc->link.rate;
+	max_tu_symbol = DIV_ROUND_UP(in_bw * TU_SIZE_RECOMMENDED, out_bw);
 
 	/* DP Main Stream Attributes */
 	vid_sync_dly = hsync_len + left_margin + mode->hdisplay;
@@ -869,10 +894,7 @@ static int tc_set_video_mode(struct tc_data *tc,
 			   FIELD_PREP(MAX_TU_SYMBOL, max_tu_symbol) |
 			   FIELD_PREP(TU_SIZE, TU_SIZE_RECOMMENDED) |
 			   BPC_8);
-	if (ret)
-		return ret;
-
-	return 0;
+	return ret;
 }
 
 static int tc_wait_link_training(struct tc_data *tc)
@@ -1185,7 +1207,11 @@ static int tc_edp_stream_enable(struct tc_data *tc)
 			return ret;
 	}
 
-	ret = tc_set_video_mode(tc, &tc->mode);
+	ret = tc_set_common_video_mode(tc, &tc->mode);
+	if (ret)
+		return ret;
+
+	ret = tc_set_edp_video_mode(tc, &tc->mode);
 	if (ret)
 		return ret;
 
-- 
2.34.1

Re: [PATCH V2 10/11] drm/bridge: tc358767: Split tc_set_video_mode() into common and (e)DP part

[Lucas Stach]

Am Freitag, dem 18.02.2022 um 02:00 +0100 schrieb Marek Vasut:
> The tc_set_video_mode() sets up both common and (e)DP video mode settings of
> the bridge chip. Split the function into tc_set_common_video_mode() to set
> the common settings and tc_set_edp_video_mode() to set the (e)DP specific
> settings. No functional change.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>

Reviewed-by: Lucas Stach <l.stach@pengutronix.de>

> ---
> V2: - New patch
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 48 ++++++++++++++++++++++++-------
>  1 file changed, 37 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 4af0ad5db2148..091c969a36ab7 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -734,11 +734,10 @@ static int tc_get_display_props(struct tc_data *tc)
>  	return ret;
>  }
>  
> -static int tc_set_video_mode(struct tc_data *tc,
> -			     const struct drm_display_mode *mode)
> +static int tc_set_common_video_mode(struct tc_data *tc,
> +				    const struct drm_display_mode *mode)
>  {
>  	int ret;
> -	int vid_sync_dly;
>  	int max_tu_symbol;
>  
>  	int left_margin = mode->htotal - mode->hsync_end;
> @@ -747,7 +746,6 @@ static int tc_set_video_mode(struct tc_data *tc,
>  	int upper_margin = mode->vtotal - mode->vsync_end;
>  	int lower_margin = mode->vsync_start - mode->vdisplay;
>  	int vsync_len = mode->vsync_end - mode->vsync_start;
> -	u32 dp0_syncval;
>  	u32 bits_per_pixel = 24;
>  	u32 in_bw, out_bw;
>  
> @@ -818,8 +816,35 @@ static int tc_set_video_mode(struct tc_data *tc,
>  			   FIELD_PREP(COLOR_B, 99) |
>  			   ENI2CFILTER |
>  			   FIELD_PREP(COLOR_BAR_MODE, COLOR_BAR_MODE_BARS));
> -	if (ret)
> -		return ret;
> +
> +	return ret;
> +}
> +
> +static int tc_set_edp_video_mode(struct tc_data *tc,
> +				 const struct drm_display_mode *mode)
> +{
> +	int ret;
> +	int vid_sync_dly;
> +	int max_tu_symbol;
> +
> +	int left_margin = mode->htotal - mode->hsync_end;
> +	int hsync_len = mode->hsync_end - mode->hsync_start;
> +	int upper_margin = mode->vtotal - mode->vsync_end;
> +	int vsync_len = mode->vsync_end - mode->vsync_start;
> +	u32 dp0_syncval;
> +	u32 bits_per_pixel = 24;
> +	u32 in_bw, out_bw;
> +
> +	/*
> +	 * Recommended maximum number of symbols transferred in a transfer unit:
> +	 * DIV_ROUND_UP((input active video bandwidth in bytes) * tu_size,
> +	 *              (output active video bandwidth in bytes))
> +	 * Must be less than tu_size.
> +	 */
> +
> +	in_bw = mode->clock * bits_per_pixel / 8;
> +	out_bw = tc->link.num_lanes * tc->link.rate;
> +	max_tu_symbol = DIV_ROUND_UP(in_bw * TU_SIZE_RECOMMENDED, out_bw);
>  
>  	/* DP Main Stream Attributes */
>  	vid_sync_dly = hsync_len + left_margin + mode->hdisplay;
> @@ -869,10 +894,7 @@ static int tc_set_video_mode(struct tc_data *tc,
>  			   FIELD_PREP(MAX_TU_SYMBOL, max_tu_symbol) |
>  			   FIELD_PREP(TU_SIZE, TU_SIZE_RECOMMENDED) |
>  			   BPC_8);
> -	if (ret)
> -		return ret;
> -
> -	return 0;
> +	return ret;
>  }
>  
>  static int tc_wait_link_training(struct tc_data *tc)
> @@ -1185,7 +1207,11 @@ static int tc_edp_stream_enable(struct tc_data *tc)
>  			return ret;
>  	}
>  
> -	ret = tc_set_video_mode(tc, &tc->mode);
> +	ret = tc_set_common_video_mode(tc, &tc->mode);
> +	if (ret)
> +		return ret;
> +
> +	ret = tc_set_edp_video_mode(tc, &tc->mode);
>  	if (ret)
>  		return ret;
>  

[PATCH V2 11/11] drm/bridge: tc358767: Add DSI-to-DPI mode support

[Marek Vasut]

The TC358767/TC358867/TC9595 are all capable of operating in multiple
modes, DPI-to-(e)DP, DSI-to-(e)DP, DSI-to-DPI. Add support for the
DSI-to-DPI mode.

This requires skipping most of the (e)DP initialization code, which is
currently a large part of this driver, hence it is better to have far
simpler separate tc_dpi_bridge_funcs and their implementation.

The configuration of DPI output is also much simpler. The configuration
of the DSI input is rather similar to the other TC bridge chips.

The Pixel PLL in DPI output mode does not have the 65..150 MHz limitation
imposed on the (e)DP output mode, so this limitation is skipped to permit
operating panels with far slower pixel clock, even below 9 MHz. This mode
of operation of the PLL is valid and tested.

The detection of bridge mode is now added into tc_probe_bridge_mode(),
where in case a DPI panel is found on port@1 endpoint@1, the mode is
assumed to be DSI-to-DPI. If (e)DP is detected on port@2, the mode is
assumed to be DPI-to-(e)DP.

The DSI-to-(e)DP mode is not supported due to lack of proper hardware,
but this would be some sort of mix between the two aforementioned modes.

Signed-off-by: Marek Vasut <marex@denx.de>
Cc: Jonas Karlman <jonas@kwiboo.se>
Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
Cc: Maxime Ripard <maxime@cerno.tech>
Cc: Neil Armstrong <narmstrong@baylibre.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
---
V2: - Rebase on next-20220217 and new patches in this series
---
 drivers/gpu/drm/bridge/tc358767.c | 339 +++++++++++++++++++++++++++++-
 1 file changed, 332 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
index 091c969a36ab7..2b9fceb1333fa 100644
--- a/drivers/gpu/drm/bridge/tc358767.c
+++ b/drivers/gpu/drm/bridge/tc358767.c
@@ -1,6 +1,12 @@
 // SPDX-License-Identifier: GPL-2.0-or-later
 /*
- * tc358767 eDP bridge driver
+ * TC358767/TC358867/TC9595 DSI/DPI-to-DPI/(e)DP bridge driver
+ *
+ * The TC358767/TC358867/TC9595 can operate in multiple modes.
+ * The following modes are supported:
+ *   DPI->(e)DP -- supported
+ *   DSI->DPI .... supported
+ *   DSI->(e)DP .. NOT supported
  *
  * Copyright (C) 2016 CogentEmbedded Inc
  * Author: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
@@ -29,6 +35,7 @@
 #include <drm/drm_bridge.h>
 #include <drm/dp/drm_dp_helper.h>
 #include <drm/drm_edid.h>
+#include <drm/drm_mipi_dsi.h>
 #include <drm/drm_of.h>
 #include <drm/drm_panel.h>
 #include <drm/drm_print.h>
@@ -36,7 +43,35 @@
 
 /* Registers */
 
-/* Display Parallel Interface */
+/* PPI layer registers */
+#define PPI_STARTPPI		0x0104 /* START control bit */
+#define PPI_LPTXTIMECNT		0x0114 /* LPTX timing signal */
+#define LPX_PERIOD			3
+#define PPI_LANEENABLE		0x0134
+#define PPI_TX_RX_TA		0x013c
+#define TTA_GET				0x40000
+#define TTA_SURE			6
+#define PPI_D0S_ATMR		0x0144
+#define PPI_D1S_ATMR		0x0148
+#define PPI_D0S_CLRSIPOCOUNT	0x0164 /* Assertion timer for Lane 0 */
+#define PPI_D1S_CLRSIPOCOUNT	0x0168 /* Assertion timer for Lane 1 */
+#define PPI_D2S_CLRSIPOCOUNT	0x016c /* Assertion timer for Lane 2 */
+#define PPI_D3S_CLRSIPOCOUNT	0x0170 /* Assertion timer for Lane 3 */
+#define PPI_START_FUNCTION		BIT(0)
+
+/* DSI layer registers */
+#define DSI_STARTDSI		0x0204 /* START control bit of DSI-TX */
+#define DSI_LANEENABLE		0x0210 /* Enables each lane */
+#define DSI_RX_START			BIT(0)
+
+/* Lane enable PPI and DSI register bits */
+#define LANEENABLE_CLEN		BIT(0)
+#define LANEENABLE_L0EN		BIT(1)
+#define LANEENABLE_L1EN		BIT(2)
+#define LANEENABLE_L2EN		BIT(1)
+#define LANEENABLE_L3EN		BIT(2)
+
+/* Display Parallel Input Interface */
 #define DPIPXLFMT		0x0440
 #define VS_POL_ACTIVE_LOW		(1 << 10)
 #define HS_POL_ACTIVE_LOW		(1 << 9)
@@ -48,6 +83,14 @@
 #define DPI_BPP_RGB666			(1 << 0)
 #define DPI_BPP_RGB565			(2 << 0)
 
+/* Display Parallel Output Interface */
+#define POCTRL			0x0448
+#define POCTRL_S2P			BIT(7)
+#define POCTRL_PCLK_POL			BIT(3)
+#define POCTRL_VS_POL			BIT(2)
+#define POCTRL_HS_POL			BIT(1)
+#define POCTRL_DE_POL			BIT(0)
+
 /* Video Path */
 #define VPCTRL0			0x0450
 #define VSDELAY			GENMASK(31, 20)
@@ -247,6 +290,9 @@ struct tc_data {
 	struct drm_bridge	*panel_bridge;
 	struct drm_connector	connector;
 
+	struct mipi_dsi_device	*dsi;
+	u8			dsi_lanes;
+
 	/* link settings */
 	struct tc_edp_link	link;
 
@@ -502,8 +548,10 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
 				/*
 				 * refclk * mul / (ext_pre_div * pre_div)
 				 * should be in the 150 to 650 MHz range
+				 * for (e)DP
 				 */
-				if ((clk > 650000000) || (clk < 150000000))
+				if ((tc->bridge.type != DRM_MODE_CONNECTOR_DPI) &&
+				    ((clk > 650000000) || (clk < 150000000)))
 					continue;
 
 				clk = clk / ext_div[i_post];
@@ -820,6 +868,20 @@ static int tc_set_common_video_mode(struct tc_data *tc,
 	return ret;
 }
 
+static int tc_set_dpi_video_mode(struct tc_data *tc,
+				 const struct drm_display_mode *mode)
+{
+	u32 value = POCTRL_S2P;
+
+	if (tc->mode.flags & DRM_MODE_FLAG_NHSYNC)
+		value |= POCTRL_HS_POL;
+
+	if (tc->mode.flags & DRM_MODE_FLAG_NVSYNC)
+		value |= POCTRL_VS_POL;
+
+	return regmap_write(tc->regmap, POCTRL, value);
+}
+
 static int tc_set_edp_video_mode(struct tc_data *tc,
 				 const struct drm_display_mode *mode)
 {
@@ -1192,6 +1254,79 @@ static int tc_main_link_disable(struct tc_data *tc)
 	return regmap_write(tc->regmap, DP0CTL, 0);
 }
 
+static int tc_dpi_stream_enable(struct tc_data *tc)
+{
+	int ret;
+	u32 value;
+
+	dev_dbg(tc->dev, "enable video stream\n");
+
+	/* Setup PLL */
+	ret = tc_set_syspllparam(tc);
+	if (ret)
+		return ret;
+
+	/* Pixel PLL must always be enabled for DPI mode */
+	ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
+			    1000 * tc->mode.clock);
+	if (ret)
+		return ret;
+
+	regmap_write(tc->regmap, PPI_D0S_CLRSIPOCOUNT, 3);
+	regmap_write(tc->regmap, PPI_D1S_CLRSIPOCOUNT, 3);
+	regmap_write(tc->regmap, PPI_D2S_CLRSIPOCOUNT, 3);
+	regmap_write(tc->regmap, PPI_D3S_CLRSIPOCOUNT, 3);
+	regmap_write(tc->regmap, PPI_D0S_ATMR, 0);
+	regmap_write(tc->regmap, PPI_D1S_ATMR, 0);
+	regmap_write(tc->regmap, PPI_TX_RX_TA, TTA_GET | TTA_SURE);
+	regmap_write(tc->regmap, PPI_LPTXTIMECNT, LPX_PERIOD);
+
+	value = ((LANEENABLE_L0EN << tc->dsi_lanes) - LANEENABLE_L0EN) |
+		LANEENABLE_CLEN;
+	regmap_write(tc->regmap, PPI_LANEENABLE, value);
+	regmap_write(tc->regmap, DSI_LANEENABLE, value);
+
+	ret = tc_set_common_video_mode(tc, &tc->mode);
+	if (ret)
+		return ret;
+
+	ret = tc_set_dpi_video_mode(tc, &tc->mode);
+	if (ret)
+		return ret;
+
+	/* Set input interface */
+	value = DP0_AUDSRC_NO_INPUT;
+	if (tc_test_pattern)
+		value |= DP0_VIDSRC_COLOR_BAR;
+	else
+		value |= DP0_VIDSRC_DSI_RX;
+	ret = regmap_write(tc->regmap, SYSCTRL, value);
+	if (ret)
+		return ret;
+
+	msleep(100);
+
+	regmap_write(tc->regmap, PPI_STARTPPI, PPI_START_FUNCTION);
+	regmap_write(tc->regmap, DSI_STARTDSI, DSI_RX_START);
+
+	return 0;
+}
+
+static int tc_dpi_stream_disable(struct tc_data *tc)
+{
+	int ret;
+
+	dev_dbg(tc->dev, "disable video stream\n");
+
+	ret = regmap_update_bits(tc->regmap, DP0CTL, VID_EN, 0);
+	if (ret)
+		return ret;
+
+	tc_pxl_pll_dis(tc);
+
+	return 0;
+}
+
 static int tc_edp_stream_enable(struct tc_data *tc)
 {
 	int ret;
@@ -1323,6 +1458,40 @@ static int tc_hardware_init(struct tc_data *tc)
 	return 0;
 }
 
+static void
+tc_dpi_bridge_atomic_enable(struct drm_bridge *bridge,
+			    struct drm_bridge_state *old_bridge_state)
+
+{
+	struct tc_data *tc = bridge_to_tc(bridge);
+	int ret;
+
+	ret = tc_hardware_init(tc);
+	if (ret < 0) {
+		dev_err(tc->dev, "failed to initialize bridge: %d\n", ret);
+		return;
+	}
+
+	ret = tc_dpi_stream_enable(tc);
+	if (ret < 0) {
+		dev_err(tc->dev, "main link stream start error: %d\n", ret);
+		tc_main_link_disable(tc);
+		return;
+	}
+}
+
+static void
+tc_dpi_bridge_atomic_disable(struct drm_bridge *bridge,
+			     struct drm_bridge_state *old_bridge_state)
+{
+	struct tc_data *tc = bridge_to_tc(bridge);
+	int ret;
+
+	ret = tc_dpi_stream_disable(tc);
+	if (ret < 0)
+		dev_err(tc->dev, "main link stream stop error: %d\n", ret);
+}
+
 static void
 tc_edp_bridge_atomic_enable(struct drm_bridge *bridge,
 			    struct drm_bridge_state *old_bridge_state)
@@ -1399,6 +1568,16 @@ static int tc_edp_common_atomic_check(struct drm_bridge *bridge,
 	return 0;
 }
 
+static int tc_dpi_atomic_check(struct drm_bridge *bridge,
+			       struct drm_bridge_state *bridge_state,
+			       struct drm_crtc_state *crtc_state,
+			       struct drm_connector_state *conn_state)
+{
+	/* DSI->DPI interface clock limitation: upto 100 MHz */
+	return tc_edp_common_atomic_check(bridge, bridge_state, crtc_state,
+					  conn_state, 100000);
+}
+
 static int tc_edp_atomic_check(struct drm_bridge *bridge,
 			       struct drm_bridge_state *bridge_state,
 			       struct drm_crtc_state *crtc_state,
@@ -1409,6 +1588,18 @@ static int tc_edp_atomic_check(struct drm_bridge *bridge,
 					  conn_state, 154000);
 }
 
+static enum drm_mode_status
+tc_dpi_mode_valid(struct drm_bridge *bridge,
+		  const struct drm_display_info *info,
+		  const struct drm_display_mode *mode)
+{
+	/* DPI interface clock limitation: upto 100 MHz */
+	if (mode->clock > 100000)
+		return MODE_CLOCK_HIGH;
+
+	return MODE_OK;
+}
+
 static enum drm_mode_status
 tc_edp_mode_valid(struct drm_bridge *bridge,
 		  const struct drm_display_info *info,
@@ -1520,6 +1711,18 @@ static const struct drm_connector_funcs tc_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
+static int tc_dpi_bridge_attach(struct drm_bridge *bridge,
+				enum drm_bridge_attach_flags flags)
+{
+	struct tc_data *tc = bridge_to_tc(bridge);
+
+	if (!tc->panel_bridge)
+		return 0;
+
+	return drm_bridge_attach(tc->bridge.encoder, tc->panel_bridge,
+				 &tc->bridge, flags);
+}
+
 static int tc_edp_bridge_attach(struct drm_bridge *bridge,
 				enum drm_bridge_attach_flags flags)
 {
@@ -1578,6 +1781,45 @@ static void tc_edp_bridge_detach(struct drm_bridge *bridge)
 	drm_dp_aux_unregister(&bridge_to_tc(bridge)->aux);
 }
 
+#define MAX_INPUT_SEL_FORMATS	1
+
+static u32 *
+tc_dpi_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)
+{
+	u32 *input_fmts;
+
+	*num_input_fmts = 0;
+
+	input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),
+			     GFP_KERNEL);
+	if (!input_fmts)
+		return NULL;
+
+	/* This is the DSI-end bus format */
+	input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
+	*num_input_fmts = 1;
+
+	return input_fmts;
+}
+
+static const struct drm_bridge_funcs tc_dpi_bridge_funcs = {
+	.attach = tc_dpi_bridge_attach,
+	.mode_valid = tc_dpi_mode_valid,
+	.mode_set = tc_bridge_mode_set,
+	.atomic_check = tc_dpi_atomic_check,
+	.atomic_enable = tc_dpi_bridge_atomic_enable,
+	.atomic_disable = tc_dpi_bridge_atomic_disable,
+	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+	.atomic_reset = drm_atomic_helper_bridge_reset,
+	.atomic_get_input_bus_fmts = tc_dpi_atomic_get_input_bus_fmts,
+};
+
 static const struct drm_bridge_funcs tc_bridge_funcs = {
 	.attach = tc_edp_bridge_attach,
 	.detach = tc_edp_bridge_detach,
@@ -1678,6 +1920,81 @@ static irqreturn_t tc_irq_handler(int irq, void *arg)
 	return IRQ_HANDLED;
 }
 
+static int tc_mipi_dsi_host_attach(struct tc_data *tc)
+{
+	struct device *dev = tc->dev;
+	struct device_node *host_node;
+	struct device_node *endpoint;
+	struct mipi_dsi_device *dsi;
+	struct mipi_dsi_host *host;
+	const struct mipi_dsi_device_info info = {
+		.type = "tc358767",
+		.channel = 0,
+		.node = NULL,
+	};
+	int ret;
+
+	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
+	tc->dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
+	host_node = of_graph_get_remote_port_parent(endpoint);
+	host = of_find_mipi_dsi_host_by_node(host_node);
+	of_node_put(host_node);
+	of_node_put(endpoint);
+
+	if (tc->dsi_lanes < 0 || tc->dsi_lanes > 4)
+		return -EINVAL;
+
+	if (!host)
+		return -EPROBE_DEFER;
+
+	dsi = mipi_dsi_device_register_full(host, &info);
+	if (IS_ERR(dsi))
+		return dev_err_probe(dev, PTR_ERR(dsi),
+				     "failed to create dsi device\n");
+
+	tc->dsi = dsi;
+
+	dsi->lanes = tc->dsi_lanes;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret < 0) {
+		dev_err(dev, "failed to attach dsi to host: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int tc_probe_dpi_bridge_endpoint(struct tc_data *tc)
+{
+	struct device *dev = tc->dev;
+	struct drm_panel *panel;
+	int ret;
+
+	/* port@1 is the DPI input/output port */
+	ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
+	if (ret && ret != -ENODEV)
+		return ret;
+
+	if (panel) {
+		struct drm_bridge *panel_bridge;
+
+		panel_bridge = devm_drm_panel_bridge_add(dev, panel);
+		if (IS_ERR(panel_bridge))
+			return PTR_ERR(panel_bridge);
+
+		tc->panel_bridge = panel_bridge;
+		tc->bridge.type = DRM_MODE_CONNECTOR_DPI;
+		tc->bridge.funcs = &tc_dpi_bridge_funcs;
+
+		return 0;
+	}
+
+	return ret;
+}
+
 static int tc_probe_edp_bridge_endpoint(struct tc_data *tc)
 {
 	struct device *dev = tc->dev;
@@ -1745,7 +2062,7 @@ static int tc_probe_bridge_endpoint(struct tc_data *tc)
 	if (mode == mode_dpi_to_edp)
 		return tc_probe_edp_bridge_endpoint(tc);
 	else if (mode == mode_dsi_to_dpi)
-		dev_warn(dev, "The mode DSI-to-DPI is not supported!\n");
+		return tc_probe_dpi_bridge_endpoint(tc);
 	else if (mode == mode_dsi_to_edp)
 		dev_warn(dev, "The mode DSI-to-(e)DP is not supported!\n");
 	else
@@ -1828,15 +2145,23 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
 		tc->have_irq = true;
 	}
 
-	ret = tc_aux_link_setup(tc);
-	if (ret)
-		return ret;
+	if (tc->bridge.type != DRM_MODE_CONNECTOR_DPI) { /* (e)DP output */
+		ret = tc_aux_link_setup(tc);
+		if (ret)
+			return ret;
+	}
 
 	tc->bridge.of_node = dev->of_node;
 	drm_bridge_add(&tc->bridge);
 
 	i2c_set_clientdata(client, tc);
 
+	if (tc->bridge.type == DRM_MODE_CONNECTOR_DPI) { /* DPI output */
+		ret = tc_mipi_dsi_host_attach(tc);
+		if (ret)
+			return ret;
+	}
+
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH V2 11/11] drm/bridge: tc358767: Add DSI-to-DPI mode support

[Lucas Stach]

Am Freitag, dem 18.02.2022 um 02:00 +0100 schrieb Marek Vasut:
> The TC358767/TC358867/TC9595 are all capable of operating in multiple
> modes, DPI-to-(e)DP, DSI-to-(e)DP, DSI-to-DPI. Add support for the
> DSI-to-DPI mode.
> 
> This requires skipping most of the (e)DP initialization code, which is
> currently a large part of this driver, hence it is better to have far
> simpler separate tc_dpi_bridge_funcs and their implementation.
> 
> The configuration of DPI output is also much simpler. The configuration
> of the DSI input is rather similar to the other TC bridge chips.
> 
> The Pixel PLL in DPI output mode does not have the 65..150 MHz limitation
> imposed on the (e)DP output mode, so this limitation is skipped to permit
> operating panels with far slower pixel clock, even below 9 MHz. This mode
> of operation of the PLL is valid and tested.
> 
> The detection of bridge mode is now added into tc_probe_bridge_mode(),
> where in case a DPI panel is found on port@1 endpoint@1, the mode is
> assumed to be DSI-to-DPI. If (e)DP is detected on port@2, the mode is
> assumed to be DPI-to-(e)DP.
> 
> The DSI-to-(e)DP mode is not supported due to lack of proper hardware,
> but this would be some sort of mix between the two aforementioned modes.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Jonas Karlman <jonas@kwiboo.se>
> Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> Cc: Maxime Ripard <maxime@cerno.tech>
> Cc: Neil Armstrong <narmstrong@baylibre.com>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> ---
> V2: - Rebase on next-20220217 and new patches in this series
> ---
>  drivers/gpu/drm/bridge/tc358767.c | 339 +++++++++++++++++++++++++++++-
>  1 file changed, 332 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> index 091c969a36ab7..2b9fceb1333fa 100644
> --- a/drivers/gpu/drm/bridge/tc358767.c
> +++ b/drivers/gpu/drm/bridge/tc358767.c
> @@ -1,6 +1,12 @@
>  // SPDX-License-Identifier: GPL-2.0-or-later
>  /*
> - * tc358767 eDP bridge driver
> + * TC358767/TC358867/TC9595 DSI/DPI-to-DPI/(e)DP bridge driver
> + *
> + * The TC358767/TC358867/TC9595 can operate in multiple modes.
> + * The following modes are supported:
> + *   DPI->(e)DP -- supported
> + *   DSI->DPI .... supported
> + *   DSI->(e)DP .. NOT supported
>   *
>   * Copyright (C) 2016 CogentEmbedded Inc
>   * Author: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
> @@ -29,6 +35,7 @@
>  #include <drm/drm_bridge.h>
>  #include <drm/dp/drm_dp_helper.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_mipi_dsi.h>
>  #include <drm/drm_of.h>
>  #include <drm/drm_panel.h>
>  #include <drm/drm_print.h>
> @@ -36,7 +43,35 @@
>  
>  /* Registers */
>  
> -/* Display Parallel Interface */
> +/* PPI layer registers */
> +#define PPI_STARTPPI		0x0104 /* START control bit */
> +#define PPI_LPTXTIMECNT		0x0114 /* LPTX timing signal */
> +#define LPX_PERIOD			3
> +#define PPI_LANEENABLE		0x0134
> +#define PPI_TX_RX_TA		0x013c
> +#define TTA_GET				0x40000
> +#define TTA_SURE			6
> +#define PPI_D0S_ATMR		0x0144
> +#define PPI_D1S_ATMR		0x0148
> +#define PPI_D0S_CLRSIPOCOUNT	0x0164 /* Assertion timer for Lane 0 */
> +#define PPI_D1S_CLRSIPOCOUNT	0x0168 /* Assertion timer for Lane 1 */
> +#define PPI_D2S_CLRSIPOCOUNT	0x016c /* Assertion timer for Lane 2 */
> +#define PPI_D3S_CLRSIPOCOUNT	0x0170 /* Assertion timer for Lane 3 */
> +#define PPI_START_FUNCTION		BIT(0)
> +
> +/* DSI layer registers */
> +#define DSI_STARTDSI		0x0204 /* START control bit of DSI-TX */
> +#define DSI_LANEENABLE		0x0210 /* Enables each lane */
> +#define DSI_RX_START			BIT(0)
> +
> +/* Lane enable PPI and DSI register bits */
> +#define LANEENABLE_CLEN		BIT(0)
> +#define LANEENABLE_L0EN		BIT(1)
> +#define LANEENABLE_L1EN		BIT(2)
> +#define LANEENABLE_L2EN		BIT(1)
> +#define LANEENABLE_L3EN		BIT(2)
> +
> +/* Display Parallel Input Interface */
>  #define DPIPXLFMT		0x0440
>  #define VS_POL_ACTIVE_LOW		(1 << 10)
>  #define HS_POL_ACTIVE_LOW		(1 << 9)
> @@ -48,6 +83,14 @@
>  #define DPI_BPP_RGB666			(1 << 0)
>  #define DPI_BPP_RGB565			(2 << 0)
>  
> +/* Display Parallel Output Interface */
> +#define POCTRL			0x0448
> +#define POCTRL_S2P			BIT(7)
> +#define POCTRL_PCLK_POL			BIT(3)
> +#define POCTRL_VS_POL			BIT(2)
> +#define POCTRL_HS_POL			BIT(1)
> +#define POCTRL_DE_POL			BIT(0)
> +
>  /* Video Path */
>  #define VPCTRL0			0x0450
>  #define VSDELAY			GENMASK(31, 20)
> @@ -247,6 +290,9 @@ struct tc_data {
>  	struct drm_bridge	*panel_bridge;
>  	struct drm_connector	connector;
>  
> +	struct mipi_dsi_device	*dsi;
> +	u8			dsi_lanes;
> +
>  	/* link settings */
>  	struct tc_edp_link	link;
>  
> @@ -502,8 +548,10 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
>  				/*
>  				 * refclk * mul / (ext_pre_div * pre_div)
>  				 * should be in the 150 to 650 MHz range
> +				 * for (e)DP
>  				 */
> -				if ((clk > 650000000) || (clk < 150000000))
> +				if ((tc->bridge.type != DRM_MODE_CONNECTOR_DPI) &&
> +				    ((clk > 650000000) || (clk < 150000000)))
>  					continue;

Is there any indication what the bounds are for DPI mode? Can we
replace this with a better check, instead of just disabling it?

>  
>  				clk = clk / ext_div[i_post];
> @@ -820,6 +868,20 @@ static int tc_set_common_video_mode(struct tc_data *tc,
>  	return ret;
>  }
>  
> +static int tc_set_dpi_video_mode(struct tc_data *tc,
> +				 const struct drm_display_mode *mode)
> +{
> +	u32 value = POCTRL_S2P;
> +
> +	if (tc->mode.flags & DRM_MODE_FLAG_NHSYNC)
> +		value |= POCTRL_HS_POL;
> +
> +	if (tc->mode.flags & DRM_MODE_FLAG_NVSYNC)
> +		value |= POCTRL_VS_POL;
> +
> +	return regmap_write(tc->regmap, POCTRL, value);
> +}
> +
>  static int tc_set_edp_video_mode(struct tc_data *tc,
>  				 const struct drm_display_mode *mode)
>  {
> @@ -1192,6 +1254,79 @@ static int tc_main_link_disable(struct tc_data *tc)
>  	return regmap_write(tc->regmap, DP0CTL, 0);
>  }
>  
> +static int tc_dpi_stream_enable(struct tc_data *tc)
> +{
> +	int ret;
> +	u32 value;
> +
> +	dev_dbg(tc->dev, "enable video stream\n");
> +
> +	/* Setup PLL */
> +	ret = tc_set_syspllparam(tc);
> +	if (ret)
> +		return ret;
> +
> +	/* Pixel PLL must always be enabled for DPI mode */
> +	ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
> +			    1000 * tc->mode.clock);
> +	if (ret)
> +		return ret;
> +
> +	regmap_write(tc->regmap, PPI_D0S_CLRSIPOCOUNT, 3);
> +	regmap_write(tc->regmap, PPI_D1S_CLRSIPOCOUNT, 3);
> +	regmap_write(tc->regmap, PPI_D2S_CLRSIPOCOUNT, 3);
> +	regmap_write(tc->regmap, PPI_D3S_CLRSIPOCOUNT, 3);

Hm, those hardcoded always seem kind of fishy, as AFAIK those
parameters are dependent on land frequency and some other things. But
I'm also not sure if we have all the information available to
dynamically calculate them.

> +	regmap_write(tc->regmap, PPI_D0S_ATMR, 0);
> +	regmap_write(tc->regmap, PPI_D1S_ATMR, 0);
> +	regmap_write(tc->regmap, PPI_TX_RX_TA, TTA_GET | TTA_SURE);
> +	regmap_write(tc->regmap, PPI_LPTXTIMECNT, LPX_PERIOD);
> +
> +	value = ((LANEENABLE_L0EN << tc->dsi_lanes) - LANEENABLE_L0EN) |
> +		LANEENABLE_CLEN;
> +	regmap_write(tc->regmap, PPI_LANEENABLE, value);
> +	regmap_write(tc->regmap, DSI_LANEENABLE, value);
> +
> +	ret = tc_set_common_video_mode(tc, &tc->mode);
> +	if (ret)
> +		return ret;
> +
> +	ret = tc_set_dpi_video_mode(tc, &tc->mode);
> +	if (ret)
> +		return ret;
> +
> +	/* Set input interface */
> +	value = DP0_AUDSRC_NO_INPUT;
> +	if (tc_test_pattern)
> +		value |= DP0_VIDSRC_COLOR_BAR;
> +	else
> +		value |= DP0_VIDSRC_DSI_RX;
> +	ret = regmap_write(tc->regmap, SYSCTRL, value);
> +	if (ret)
> +		return ret;
> +
> +	msleep(100);

What is that used for? PLL stabilization? Some other purpose?

> +
> +	regmap_write(tc->regmap, PPI_STARTPPI, PPI_START_FUNCTION);
> +	regmap_write(tc->regmap, DSI_STARTDSI, DSI_RX_START);
> +
> +	return 0;
> +}
> +
> +static int tc_dpi_stream_disable(struct tc_data *tc)
> +{
> +	int ret;
> +
> +	dev_dbg(tc->dev, "disable video stream\n");
> +
> +	ret = regmap_update_bits(tc->regmap, DP0CTL, VID_EN, 0);

If I'm not mistaken than VID_EN isn't set in the DPI stream enable, so
clearing it here seems wrong.

> +	if (ret)
> +		return ret;
> +
> +	tc_pxl_pll_dis(tc);
> +
> +	return 0;
> +}
> +
>  static int tc_edp_stream_enable(struct tc_data *tc)
>  {
>  	int ret;
> @@ -1323,6 +1458,40 @@ static int tc_hardware_init(struct tc_data *tc)
>  	return 0;
>  }
>  
> +static void
> +tc_dpi_bridge_atomic_enable(struct drm_bridge *bridge,
> +			    struct drm_bridge_state *old_bridge_state)
> +
> +{
> +	struct tc_data *tc = bridge_to_tc(bridge);
> +	int ret;
> +
> +	ret = tc_hardware_init(tc);
> +	if (ret < 0) {
> +		dev_err(tc->dev, "failed to initialize bridge: %d\n", ret);
> +		return;
> +	}
> +
> +	ret = tc_dpi_stream_enable(tc);
> +	if (ret < 0) {
> +		dev_err(tc->dev, "main link stream start error: %d\n", ret);
> +		tc_main_link_disable(tc);
> +		return;
> +	}
> +}
> +
> +static void
> +tc_dpi_bridge_atomic_disable(struct drm_bridge *bridge,
> +			     struct drm_bridge_state *old_bridge_state)
> +{
> +	struct tc_data *tc = bridge_to_tc(bridge);
> +	int ret;
> +
> +	ret = tc_dpi_stream_disable(tc);
> +	if (ret < 0)
> +		dev_err(tc->dev, "main link stream stop error: %d\n", ret);
> +}
> +
>  static void
>  tc_edp_bridge_atomic_enable(struct drm_bridge *bridge,
>  			    struct drm_bridge_state *old_bridge_state)
> @@ -1399,6 +1568,16 @@ static int tc_edp_common_atomic_check(struct drm_bridge *bridge,
>  	return 0;
>  }
>  
> +static int tc_dpi_atomic_check(struct drm_bridge *bridge,
> +			       struct drm_bridge_state *bridge_state,
> +			       struct drm_crtc_state *crtc_state,
> +			       struct drm_connector_state *conn_state)
> +{
> +	/* DSI->DPI interface clock limitation: upto 100 MHz */
> +	return tc_edp_common_atomic_check(bridge, bridge_state, crtc_state,
> +					  conn_state, 100000);
> +}
> +
>  static int tc_edp_atomic_check(struct drm_bridge *bridge,
>  			       struct drm_bridge_state *bridge_state,
>  			       struct drm_crtc_state *crtc_state,
> @@ -1409,6 +1588,18 @@ static int tc_edp_atomic_check(struct drm_bridge *bridge,
>  					  conn_state, 154000);
>  }
>  
> +static enum drm_mode_status
> +tc_dpi_mode_valid(struct drm_bridge *bridge,
> +		  const struct drm_display_info *info,
> +		  const struct drm_display_mode *mode)
> +{
> +	/* DPI interface clock limitation: upto 100 MHz */
> +	if (mode->clock > 100000)
> +		return MODE_CLOCK_HIGH;
> +
> +	return MODE_OK;
> +}
> +
>  static enum drm_mode_status
>  tc_edp_mode_valid(struct drm_bridge *bridge,
>  		  const struct drm_display_info *info,
> @@ -1520,6 +1711,18 @@ static const struct drm_connector_funcs tc_connector_funcs = {
>  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
>  };
>  
> +static int tc_dpi_bridge_attach(struct drm_bridge *bridge,
> +				enum drm_bridge_attach_flags flags)
> +{
> +	struct tc_data *tc = bridge_to_tc(bridge);
> +
> +	if (!tc->panel_bridge)
> +		return 0;
> +
> +	return drm_bridge_attach(tc->bridge.encoder, tc->panel_bridge,
> +				 &tc->bridge, flags);
> +}
> +
>  static int tc_edp_bridge_attach(struct drm_bridge *bridge,
>  				enum drm_bridge_attach_flags flags)
>  {
> @@ -1578,6 +1781,45 @@ static void tc_edp_bridge_detach(struct drm_bridge *bridge)
>  	drm_dp_aux_unregister(&bridge_to_tc(bridge)->aux);
>  }
>  
> +#define MAX_INPUT_SEL_FORMATS	1
> +
> +static u32 *
> +tc_dpi_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)
> +{
> +	u32 *input_fmts;
> +
> +	*num_input_fmts = 0;
> +
> +	input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),
> +			     GFP_KERNEL);
> +	if (!input_fmts)
> +		return NULL;
> +
> +	/* This is the DSI-end bus format */
> +	input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
> +	*num_input_fmts = 1;
> +
> +	return input_fmts;
> +}
> +
> +static const struct drm_bridge_funcs tc_dpi_bridge_funcs = {
> +	.attach = tc_dpi_bridge_attach,
> +	.mode_valid = tc_dpi_mode_valid,
> +	.mode_set = tc_bridge_mode_set,
> +	.atomic_check = tc_dpi_atomic_check,
> +	.atomic_enable = tc_dpi_bridge_atomic_enable,
> +	.atomic_disable = tc_dpi_bridge_atomic_disable,
> +	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> +	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> +	.atomic_reset = drm_atomic_helper_bridge_reset,
> +	.atomic_get_input_bus_fmts = tc_dpi_atomic_get_input_bus_fmts,
> +};
> +
>  static const struct drm_bridge_funcs tc_bridge_funcs = {
>  	.attach = tc_edp_bridge_attach,
>  	.detach = tc_edp_bridge_detach,
> @@ -1678,6 +1920,81 @@ static irqreturn_t tc_irq_handler(int irq, void *arg)
>  	return IRQ_HANDLED;
>  }
>  
> +static int tc_mipi_dsi_host_attach(struct tc_data *tc)
> +{
> +	struct device *dev = tc->dev;
> +	struct device_node *host_node;
> +	struct device_node *endpoint;
> +	struct mipi_dsi_device *dsi;
> +	struct mipi_dsi_host *host;
> +	const struct mipi_dsi_device_info info = {
> +		.type = "tc358767",
> +		.channel = 0,
> +		.node = NULL,
> +	};
> +	int ret;
> +
> +	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> +	tc->dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");

The data-lanes property isn't documented in the DT binding. Please add.

> +	host_node = of_graph_get_remote_port_parent(endpoint);
> +	host = of_find_mipi_dsi_host_by_node(host_node);
> +	of_node_put(host_node);
> +	of_node_put(endpoint);
> +
> +	if (tc->dsi_lanes < 0 || tc->dsi_lanes > 4)
> +		return -EINVAL;
> +
> +	if (!host)
> +		return -EPROBE_DEFER;
> +
> +	dsi = mipi_dsi_device_register_full(host, &info);
> +	if (IS_ERR(dsi))
> +		return dev_err_probe(dev, PTR_ERR(dsi),
> +				     "failed to create dsi device\n");
> +
> +	tc->dsi = dsi;
> +
> +	dsi->lanes = tc->dsi_lanes;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
> +
> +	ret = mipi_dsi_attach(dsi);
> +	if (ret < 0) {
> +		dev_err(dev, "failed to attach dsi to host: %d\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tc_probe_dpi_bridge_endpoint(struct tc_data *tc)
> +{
> +	struct device *dev = tc->dev;
> +	struct drm_panel *panel;
> +	int ret;
> +
> +	/* port@1 is the DPI input/output port */
> +	ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
> +	if (ret && ret != -ENODEV)
> +		return ret;
> +
> +	if (panel) {
> +		struct drm_bridge *panel_bridge;
> +
> +		panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> +		if (IS_ERR(panel_bridge))
> +			return PTR_ERR(panel_bridge);
> +
> +		tc->panel_bridge = panel_bridge;
> +		tc->bridge.type = DRM_MODE_CONNECTOR_DPI;
> +		tc->bridge.funcs = &tc_dpi_bridge_funcs;
> +
> +		return 0;
> +	}
> +
> +	return ret;
> +}
> +
>  static int tc_probe_edp_bridge_endpoint(struct tc_data *tc)
>  {
>  	struct device *dev = tc->dev;
> @@ -1745,7 +2062,7 @@ static int tc_probe_bridge_endpoint(struct tc_data *tc)
>  	if (mode == mode_dpi_to_edp)
>  		return tc_probe_edp_bridge_endpoint(tc);
>  	else if (mode == mode_dsi_to_dpi)
> -		dev_warn(dev, "The mode DSI-to-DPI is not supported!\n");
> +		return tc_probe_dpi_bridge_endpoint(tc);
>  	else if (mode == mode_dsi_to_edp)
>  		dev_warn(dev, "The mode DSI-to-(e)DP is not supported!\n");
>  	else
> @@ -1828,15 +2145,23 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>  		tc->have_irq = true;
>  	}
>  
> -	ret = tc_aux_link_setup(tc);
> -	if (ret)
> -		return ret;
> +	if (tc->bridge.type != DRM_MODE_CONNECTOR_DPI) { /* (e)DP output */
> +		ret = tc_aux_link_setup(tc);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	tc->bridge.of_node = dev->of_node;
>  	drm_bridge_add(&tc->bridge);
>  
>  	i2c_set_clientdata(client, tc);
>  
> +	if (tc->bridge.type == DRM_MODE_CONNECTOR_DPI) { /* DPI output */
> +		ret = tc_mipi_dsi_host_attach(tc);
> +		if (ret)
> +			return ret;
> +	}

If tc_mipi_dsi_host_attach fails the drm bridge registered a few lines
above isn't cleaned up properly.

Regards,
Lucas

Re: [PATCH V2 11/11] drm/bridge: tc358767: Add DSI-to-DPI mode support

[Marek Vasut]

On 2/18/22 19:38, Lucas Stach wrote:

[...]

>> @@ -502,8 +548,10 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
>>   				/*
>>   				 * refclk * mul / (ext_pre_div * pre_div)
>>   				 * should be in the 150 to 650 MHz range
>> +				 * for (e)DP
>>   				 */
>> -				if ((clk > 650000000) || (clk < 150000000))
>> +				if ((tc->bridge.type != DRM_MODE_CONNECTOR_DPI) &&
>> +				    ((clk > 650000000) || (clk < 150000000)))
>>   					continue;
> 
> Is there any indication what the bounds are for DPI mode? Can we
> replace this with a better check, instead of just disabling it?

Apparently the DPI pixel PLL output is limited to 100 MHz, so yes, this 
can be improved.

[...]

>> +static int tc_dpi_stream_enable(struct tc_data *tc)
>> +{
>> +	int ret;
>> +	u32 value;
>> +
>> +	dev_dbg(tc->dev, "enable video stream\n");
>> +
>> +	/* Setup PLL */
>> +	ret = tc_set_syspllparam(tc);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Pixel PLL must always be enabled for DPI mode */
>> +	ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
>> +			    1000 * tc->mode.clock);
>> +	if (ret)
>> +		return ret;
>> +
>> +	regmap_write(tc->regmap, PPI_D0S_CLRSIPOCOUNT, 3);
>> +	regmap_write(tc->regmap, PPI_D1S_CLRSIPOCOUNT, 3);
>> +	regmap_write(tc->regmap, PPI_D2S_CLRSIPOCOUNT, 3);
>> +	regmap_write(tc->regmap, PPI_D3S_CLRSIPOCOUNT, 3);
> 
> Hm, those hardcoded always seem kind of fishy, as AFAIK those
> parameters are dependent on land frequency and some other things. But
> I'm also not sure if we have all the information available to
> dynamically calculate them.

We have multiple copies of the same ^ code in other TC358nnn drivers 
too, it seems like some de-duplication could be done too. I have this 
feeling the Toshiba bridges are all glued together from two (or more) 
halves and there is large potential for de-duplication in all those TC 
drivers. But does anyone really have all the chips to test, except for 
Toshiba ?

>> +	regmap_write(tc->regmap, PPI_D0S_ATMR, 0);
>> +	regmap_write(tc->regmap, PPI_D1S_ATMR, 0);
>> +	regmap_write(tc->regmap, PPI_TX_RX_TA, TTA_GET | TTA_SURE);
>> +	regmap_write(tc->regmap, PPI_LPTXTIMECNT, LPX_PERIOD);
>> +
>> +	value = ((LANEENABLE_L0EN << tc->dsi_lanes) - LANEENABLE_L0EN) |
>> +		LANEENABLE_CLEN;
>> +	regmap_write(tc->regmap, PPI_LANEENABLE, value);
>> +	regmap_write(tc->regmap, DSI_LANEENABLE, value);
>> +
>> +	ret = tc_set_common_video_mode(tc, &tc->mode);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = tc_set_dpi_video_mode(tc, &tc->mode);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Set input interface */
>> +	value = DP0_AUDSRC_NO_INPUT;
>> +	if (tc_test_pattern)
>> +		value |= DP0_VIDSRC_COLOR_BAR;
>> +	else
>> +		value |= DP0_VIDSRC_DSI_RX;
>> +	ret = regmap_write(tc->regmap, SYSCTRL, value);
>> +	if (ret)
>> +		return ret;
>> +
>> +	msleep(100);
> 
> What is that used for? PLL stabilization? Some other purpose?

Yes, except that should've been microseconds ... fixed

[...]

>> +static int tc_mipi_dsi_host_attach(struct tc_data *tc)
>> +{
>> +	struct device *dev = tc->dev;
>> +	struct device_node *host_node;
>> +	struct device_node *endpoint;
>> +	struct mipi_dsi_device *dsi;
>> +	struct mipi_dsi_host *host;
>> +	const struct mipi_dsi_device_info info = {
>> +		.type = "tc358767",
>> +		.channel = 0,
>> +		.node = NULL,
>> +	};
>> +	int ret;
>> +
>> +	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
>> +	tc->dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> 
> The data-lanes property isn't documented in the DT binding. Please add.

Fixed in a separate patch.

[...]

>> @@ -1828,15 +2145,23 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
>>   		tc->have_irq = true;
>>   	}
>>   
>> -	ret = tc_aux_link_setup(tc);
>> -	if (ret)
>> -		return ret;
>> +	if (tc->bridge.type != DRM_MODE_CONNECTOR_DPI) { /* (e)DP output */
>> +		ret = tc_aux_link_setup(tc);
>> +		if (ret)
>> +			return ret;
>> +	}
>>   
>>   	tc->bridge.of_node = dev->of_node;
>>   	drm_bridge_add(&tc->bridge);
>>   
>>   	i2c_set_clientdata(client, tc);
>>   
>> +	if (tc->bridge.type == DRM_MODE_CONNECTOR_DPI) { /* DPI output */
>> +		ret = tc_mipi_dsi_host_attach(tc);
>> +		if (ret)
>> +			return ret;
>> +	}
> 
> If tc_mipi_dsi_host_attach fails the drm bridge registered a few lines
> above isn't cleaned up properly.

Fixed

Re: [PATCH V2 11/11] drm/bridge: tc358767: Add DSI-to-DPI mode support

[Lucas Stach]

Hi Marek,

two more things I noticed.

Am Freitag, dem 18.02.2022 um 19:38 +0100 schrieb Lucas Stach:
> Am Freitag, dem 18.02.2022 um 02:00 +0100 schrieb Marek Vasut:
> > The TC358767/TC358867/TC9595 are all capable of operating in multiple
> > modes, DPI-to-(e)DP, DSI-to-(e)DP, DSI-to-DPI. Add support for the
> > DSI-to-DPI mode.
> > 
> > This requires skipping most of the (e)DP initialization code, which is
> > currently a large part of this driver, hence it is better to have far
> > simpler separate tc_dpi_bridge_funcs and their implementation.
> > 
> > The configuration of DPI output is also much simpler. The configuration
> > of the DSI input is rather similar to the other TC bridge chips.
> > 
> > The Pixel PLL in DPI output mode does not have the 65..150 MHz limitation
> > imposed on the (e)DP output mode, so this limitation is skipped to permit
> > operating panels with far slower pixel clock, even below 9 MHz. This mode
> > of operation of the PLL is valid and tested.
> > 
> > The detection of bridge mode is now added into tc_probe_bridge_mode(),
> > where in case a DPI panel is found on port@1 endpoint@1, the mode is
> > assumed to be DSI-to-DPI. If (e)DP is detected on port@2, the mode is
> > assumed to be DPI-to-(e)DP.
> > 
> > The DSI-to-(e)DP mode is not supported due to lack of proper hardware,
> > but this would be some sort of mix between the two aforementioned modes.
> > 
> > Signed-off-by: Marek Vasut <marex@denx.de>
> > Cc: Jonas Karlman <jonas@kwiboo.se>
> > Cc: Laurent Pinchart <Laurent.pinchart@ideasonboard.com>
> > Cc: Maxime Ripard <maxime@cerno.tech>
> > Cc: Neil Armstrong <narmstrong@baylibre.com>
> > Cc: Sam Ravnborg <sam@ravnborg.org>
> > ---
> > V2: - Rebase on next-20220217 and new patches in this series
> > ---
> >  drivers/gpu/drm/bridge/tc358767.c | 339 +++++++++++++++++++++++++++++-
> >  1 file changed, 332 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/tc358767.c b/drivers/gpu/drm/bridge/tc358767.c
> > index 091c969a36ab7..2b9fceb1333fa 100644
> > --- a/drivers/gpu/drm/bridge/tc358767.c
> > +++ b/drivers/gpu/drm/bridge/tc358767.c
> > @@ -1,6 +1,12 @@
> >  // SPDX-License-Identifier: GPL-2.0-or-later
> >  /*
> > - * tc358767 eDP bridge driver
> > + * TC358767/TC358867/TC9595 DSI/DPI-to-DPI/(e)DP bridge driver
> > + *
> > + * The TC358767/TC358867/TC9595 can operate in multiple modes.
> > + * The following modes are supported:
> > + *   DPI->(e)DP -- supported
> > + *   DSI->DPI .... supported
> > + *   DSI->(e)DP .. NOT supported
> >   *
> >   * Copyright (C) 2016 CogentEmbedded Inc
> >   * Author: Andrey Gusakov <andrey.gusakov@cogentembedded.com>
> > @@ -29,6 +35,7 @@
> >  #include <drm/drm_bridge.h>
> >  #include <drm/dp/drm_dp_helper.h>
> >  #include <drm/drm_edid.h>
> > +#include <drm/drm_mipi_dsi.h>
> >  #include <drm/drm_of.h>
> >  #include <drm/drm_panel.h>
> >  #include <drm/drm_print.h>
> > @@ -36,7 +43,35 @@
> >  
> >  /* Registers */
> >  
> > -/* Display Parallel Interface */
> > +/* PPI layer registers */
> > +#define PPI_STARTPPI		0x0104 /* START control bit */
> > +#define PPI_LPTXTIMECNT		0x0114 /* LPTX timing signal */
> > +#define LPX_PERIOD			3
> > +#define PPI_LANEENABLE		0x0134
> > +#define PPI_TX_RX_TA		0x013c
> > +#define TTA_GET				0x40000
> > +#define TTA_SURE			6
> > +#define PPI_D0S_ATMR		0x0144
> > +#define PPI_D1S_ATMR		0x0148
> > +#define PPI_D0S_CLRSIPOCOUNT	0x0164 /* Assertion timer for Lane 0 */
> > +#define PPI_D1S_CLRSIPOCOUNT	0x0168 /* Assertion timer for Lane 1 */
> > +#define PPI_D2S_CLRSIPOCOUNT	0x016c /* Assertion timer for Lane 2 */
> > +#define PPI_D3S_CLRSIPOCOUNT	0x0170 /* Assertion timer for Lane 3 */
> > +#define PPI_START_FUNCTION		BIT(0)
> > +
> > +/* DSI layer registers */
> > +#define DSI_STARTDSI		0x0204 /* START control bit of DSI-TX */
> > +#define DSI_LANEENABLE		0x0210 /* Enables each lane */
> > +#define DSI_RX_START			BIT(0)
> > +
> > +/* Lane enable PPI and DSI register bits */
> > +#define LANEENABLE_CLEN		BIT(0)
> > +#define LANEENABLE_L0EN		BIT(1)
> > +#define LANEENABLE_L1EN		BIT(2)
> > +#define LANEENABLE_L2EN		BIT(1)
> > +#define LANEENABLE_L3EN		BIT(2)
> > +
> > +/* Display Parallel Input Interface */
> >  #define DPIPXLFMT		0x0440
> >  #define VS_POL_ACTIVE_LOW		(1 << 10)
> >  #define HS_POL_ACTIVE_LOW		(1 << 9)
> > @@ -48,6 +83,14 @@
> >  #define DPI_BPP_RGB666			(1 << 0)
> >  #define DPI_BPP_RGB565			(2 << 0)
> >  
> > +/* Display Parallel Output Interface */
> > +#define POCTRL			0x0448
> > +#define POCTRL_S2P			BIT(7)
> > +#define POCTRL_PCLK_POL			BIT(3)
> > +#define POCTRL_VS_POL			BIT(2)
> > +#define POCTRL_HS_POL			BIT(1)
> > +#define POCTRL_DE_POL			BIT(0)
> > +
> >  /* Video Path */
> >  #define VPCTRL0			0x0450
> >  #define VSDELAY			GENMASK(31, 20)
> > @@ -247,6 +290,9 @@ struct tc_data {
> >  	struct drm_bridge	*panel_bridge;
> >  	struct drm_connector	connector;
> >  
> > +	struct mipi_dsi_device	*dsi;
> > +	u8			dsi_lanes;
> > +
> >  	/* link settings */
> >  	struct tc_edp_link	link;
> >  
> > @@ -502,8 +548,10 @@ static int tc_pxl_pll_en(struct tc_data *tc, u32 refclk, u32 pixelclock)
> >  				/*
> >  				 * refclk * mul / (ext_pre_div * pre_div)
> >  				 * should be in the 150 to 650 MHz range
> > +				 * for (e)DP
> >  				 */
> > -				if ((clk > 650000000) || (clk < 150000000))
> > +				if ((tc->bridge.type != DRM_MODE_CONNECTOR_DPI) &&
> > +				    ((clk > 650000000) || (clk < 150000000)))
> >  					continue;
> 
> Is there any indication what the bounds are for DPI mode? Can we
> replace this with a better check, instead of just disabling it?
> 
> >  
> >  				clk = clk / ext_div[i_post];
> > @@ -820,6 +868,20 @@ static int tc_set_common_video_mode(struct tc_data *tc,
> >  	return ret;
> >  }
> >  
> > +static int tc_set_dpi_video_mode(struct tc_data *tc,
> > +				 const struct drm_display_mode *mode)
> > +{
> > +	u32 value = POCTRL_S2P;
> > +
> > +	if (tc->mode.flags & DRM_MODE_FLAG_NHSYNC)
> > +		value |= POCTRL_HS_POL;
> > +
> > +	if (tc->mode.flags & DRM_MODE_FLAG_NVSYNC)
> > +		value |= POCTRL_VS_POL;
> > +
> > +	return regmap_write(tc->regmap, POCTRL, value);
> > +}
> > +
> >  static int tc_set_edp_video_mode(struct tc_data *tc,
> >  				 const struct drm_display_mode *mode)
> >  {
> > @@ -1192,6 +1254,79 @@ static int tc_main_link_disable(struct tc_data *tc)
> >  	return regmap_write(tc->regmap, DP0CTL, 0);
> >  }
> >  
> > +static int tc_dpi_stream_enable(struct tc_data *tc)
> > +{
> > +	int ret;
> > +	u32 value;
> > +
> > +	dev_dbg(tc->dev, "enable video stream\n");
> > +
> > +	/* Setup PLL */
> > +	ret = tc_set_syspllparam(tc);
> > +	if (ret)
> > +		return ret;

If SYSCLK isn't derived from the DSI lanes, but from the refclk input,
you need to enable the DP PLL. Otherwise it stays in bypass running
SYSCLK way too slow. As the video buffer write clock is derived from
SYSCLK, that causes constant FIFO write issues.

+       /* PLL setup */
+       ret = tc_pllupdate(tc, DP0_PLLCTRL);
+       if (ret)
+               return ret;
+
+       ret = tc_pllupdate(tc, DP1_PLLCTRL);
+       if (ret)
+               return ret;
+

> > +
> > +	/* Pixel PLL must always be enabled for DPI mode */
> > +	ret = tc_pxl_pll_en(tc, clk_get_rate(tc->refclk),
> > +			    1000 * tc->mode.clock);
> > +	if (ret)
> > +		return ret;
> > +
> > +	regmap_write(tc->regmap, PPI_D0S_CLRSIPOCOUNT, 3);
> > +	regmap_write(tc->regmap, PPI_D1S_CLRSIPOCOUNT, 3);
> > +	regmap_write(tc->regmap, PPI_D2S_CLRSIPOCOUNT, 3);
> > +	regmap_write(tc->regmap, PPI_D3S_CLRSIPOCOUNT, 3);
> 
> Hm, those hardcoded always seem kind of fishy, as AFAIK those
> parameters are dependent on land frequency and some other things. But
> I'm also not sure if we have all the information available to
> dynamically calculate them.
> 
> > +	regmap_write(tc->regmap, PPI_D0S_ATMR, 0);
> > +	regmap_write(tc->regmap, PPI_D1S_ATMR, 0);
> > +	regmap_write(tc->regmap, PPI_TX_RX_TA, TTA_GET | TTA_SURE);
> > +	regmap_write(tc->regmap, PPI_LPTXTIMECNT, LPX_PERIOD);
> > +
> > +	value = ((LANEENABLE_L0EN << tc->dsi_lanes) - LANEENABLE_L0EN) |
> > +		LANEENABLE_CLEN;
> > +	regmap_write(tc->regmap, PPI_LANEENABLE, value);
> > +	regmap_write(tc->regmap, DSI_LANEENABLE, value);
> > +
> > +	ret = tc_set_common_video_mode(tc, &tc->mode);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = tc_set_dpi_video_mode(tc, &tc->mode);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/* Set input interface */
> > +	value = DP0_AUDSRC_NO_INPUT;
> > +	if (tc_test_pattern)
> > +		value |= DP0_VIDSRC_COLOR_BAR;
> > +	else
> > +		value |= DP0_VIDSRC_DSI_RX;
> > +	ret = regmap_write(tc->regmap, SYSCTRL, value);
> > +	if (ret)
> > +		return ret;
> > +
> > +	msleep(100);
> 
> What is that used for? PLL stabilization? Some other purpose?
> 
> > +
> > +	regmap_write(tc->regmap, PPI_STARTPPI, PPI_START_FUNCTION);
> > +	regmap_write(tc->regmap, DSI_STARTDSI, DSI_RX_START);
> > +
> > +	return 0;
> > +}
> > +
> > +static int tc_dpi_stream_disable(struct tc_data *tc)
> > +{
> > +	int ret;
> > +
> > +	dev_dbg(tc->dev, "disable video stream\n");
> > +
> > +	ret = regmap_update_bits(tc->regmap, DP0CTL, VID_EN, 0);
> 
> If I'm not mistaken than VID_EN isn't set in the DPI stream enable, so
> clearing it here seems wrong.
> 
> > +	if (ret)
> > +		return ret;
> > +
> > +	tc_pxl_pll_dis(tc);
> > +
> > +	return 0;
> > +}
> > +
> >  static int tc_edp_stream_enable(struct tc_data *tc)
> >  {
> >  	int ret;
> > @@ -1323,6 +1458,40 @@ static int tc_hardware_init(struct tc_data *tc)
> >  	return 0;
> >  }
> >  
> > +static void
> > +tc_dpi_bridge_atomic_enable(struct drm_bridge *bridge,
> > +			    struct drm_bridge_state *old_bridge_state)
> > +
> > +{
> > +	struct tc_data *tc = bridge_to_tc(bridge);
> > +	int ret;
> > +
> > +	ret = tc_hardware_init(tc);
> > +	if (ret < 0) {
> > +		dev_err(tc->dev, "failed to initialize bridge: %d\n", ret);
> > +		return;
> > +	}
> > +
> > +	ret = tc_dpi_stream_enable(tc);
> > +	if (ret < 0) {
> > +		dev_err(tc->dev, "main link stream start error: %d\n", ret);
> > +		tc_main_link_disable(tc);
> > +		return;
> > +	}
> > +}
> > +
> > +static void
> > +tc_dpi_bridge_atomic_disable(struct drm_bridge *bridge,
> > +			     struct drm_bridge_state *old_bridge_state)
> > +{
> > +	struct tc_data *tc = bridge_to_tc(bridge);
> > +	int ret;
> > +
> > +	ret = tc_dpi_stream_disable(tc);
> > +	if (ret < 0)
> > +		dev_err(tc->dev, "main link stream stop error: %d\n", ret);
> > +}
> > +
> >  static void
> >  tc_edp_bridge_atomic_enable(struct drm_bridge *bridge,
> >  			    struct drm_bridge_state *old_bridge_state)
> > @@ -1399,6 +1568,16 @@ static int tc_edp_common_atomic_check(struct drm_bridge *bridge,
> >  	return 0;
> >  }
> >  
> > +static int tc_dpi_atomic_check(struct drm_bridge *bridge,
> > +			       struct drm_bridge_state *bridge_state,
> > +			       struct drm_crtc_state *crtc_state,
> > +			       struct drm_connector_state *conn_state)
> > +{
> > +	/* DSI->DPI interface clock limitation: upto 100 MHz */
> > +	return tc_edp_common_atomic_check(bridge, bridge_state, crtc_state,
> > +					  conn_state, 100000);
> > +}
> > +
> >  static int tc_edp_atomic_check(struct drm_bridge *bridge,
> >  			       struct drm_bridge_state *bridge_state,
> >  			       struct drm_crtc_state *crtc_state,
> > @@ -1409,6 +1588,18 @@ static int tc_edp_atomic_check(struct drm_bridge *bridge,
> >  					  conn_state, 154000);
> >  }
> >  
> > +static enum drm_mode_status
> > +tc_dpi_mode_valid(struct drm_bridge *bridge,
> > +		  const struct drm_display_info *info,
> > +		  const struct drm_display_mode *mode)
> > +{
> > +	/* DPI interface clock limitation: upto 100 MHz */
> > +	if (mode->clock > 100000)
> > +		return MODE_CLOCK_HIGH;
> > +
> > +	return MODE_OK;
> > +}
> > +
> >  static enum drm_mode_status
> >  tc_edp_mode_valid(struct drm_bridge *bridge,
> >  		  const struct drm_display_info *info,
> > @@ -1520,6 +1711,18 @@ static const struct drm_connector_funcs tc_connector_funcs = {
> >  	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> >  };
> >  
> > +static int tc_dpi_bridge_attach(struct drm_bridge *bridge,
> > +				enum drm_bridge_attach_flags flags)
> > +{
> > +	struct tc_data *tc = bridge_to_tc(bridge);
> > +
> > +	if (!tc->panel_bridge)
> > +		return 0;
> > +
> > +	return drm_bridge_attach(tc->bridge.encoder, tc->panel_bridge,
> > +				 &tc->bridge, flags);
> > +}
> > +
> >  static int tc_edp_bridge_attach(struct drm_bridge *bridge,
> >  				enum drm_bridge_attach_flags flags)
> >  {
> > @@ -1578,6 +1781,45 @@ static void tc_edp_bridge_detach(struct drm_bridge *bridge)
> >  	drm_dp_aux_unregister(&bridge_to_tc(bridge)->aux);
> >  }
> >  
> > +#define MAX_INPUT_SEL_FORMATS	1
> > +
> > +static u32 *
> > +tc_dpi_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)
> > +{
> > +	u32 *input_fmts;
> > +
> > +	*num_input_fmts = 0;
> > +
> > +	input_fmts = kcalloc(MAX_INPUT_SEL_FORMATS, sizeof(*input_fmts),
> > +			     GFP_KERNEL);
> > +	if (!input_fmts)
> > +		return NULL;
> > +
> > +	/* This is the DSI-end bus format */
> > +	input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
> > +	*num_input_fmts = 1;
> > +
> > +	return input_fmts;
> > +}
> > +
> > +static const struct drm_bridge_funcs tc_dpi_bridge_funcs = {
> > +	.attach = tc_dpi_bridge_attach,
> > +	.mode_valid = tc_dpi_mode_valid,
> > +	.mode_set = tc_bridge_mode_set,
> > +	.atomic_check = tc_dpi_atomic_check,
> > +	.atomic_enable = tc_dpi_bridge_atomic_enable,
> > +	.atomic_disable = tc_dpi_bridge_atomic_disable,
> > +	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> > +	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> > +	.atomic_reset = drm_atomic_helper_bridge_reset,
> > +	.atomic_get_input_bus_fmts = tc_dpi_atomic_get_input_bus_fmts,
> > +};
> > +
> >  static const struct drm_bridge_funcs tc_bridge_funcs = {
> >  	.attach = tc_edp_bridge_attach,
> >  	.detach = tc_edp_bridge_detach,
> > @@ -1678,6 +1920,81 @@ static irqreturn_t tc_irq_handler(int irq, void *arg)
> >  	return IRQ_HANDLED;
> >  }
> >  
> > +static int tc_mipi_dsi_host_attach(struct tc_data *tc)
> > +{
> > +	struct device *dev = tc->dev;
> > +	struct device_node *host_node;
> > +	struct device_node *endpoint;
> > +	struct mipi_dsi_device *dsi;
> > +	struct mipi_dsi_host *host;
> > +	const struct mipi_dsi_device_info info = {
> > +		.type = "tc358767",
> > +		.channel = 0,
> > +		.node = NULL,
> > +	};
> > +	int ret;
> > +
> > +	endpoint = of_graph_get_endpoint_by_regs(dev->of_node, 0, 0);
> > +	tc->dsi_lanes = of_property_count_u32_elems(endpoint, "data-lanes");
> 
> The data-lanes property isn't documented in the DT binding. Please add.

> 
> > +	host_node = of_graph_get_remote_port_parent(endpoint);
> > +	host = of_find_mipi_dsi_host_by_node(host_node);
> > +	of_node_put(host_node);
> > +	of_node_put(endpoint);
> > +
> > +	if (tc->dsi_lanes < 0 || tc->dsi_lanes > 4)
> > +		return -EINVAL;

tc->dsi_lanes is of type u8, so it will never be negative. Better check
the raw int return value first?

Regards,
Lucas

> > +
> > +	if (!host)
> > +		return -EPROBE_DEFER;
> > +
> > +	dsi = mipi_dsi_device_register_full(host, &info);
> > +	if (IS_ERR(dsi))
> > +		return dev_err_probe(dev, PTR_ERR(dsi),
> > +				     "failed to create dsi device\n");
> > +
> > +	tc->dsi = dsi;
> > +
> > +	dsi->lanes = tc->dsi_lanes;
> > +	dsi->format = MIPI_DSI_FMT_RGB888;
> > +	dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_VIDEO_SYNC_PULSE;
> > +
> > +	ret = mipi_dsi_attach(dsi);
> > +	if (ret < 0) {
> > +		dev_err(dev, "failed to attach dsi to host: %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int tc_probe_dpi_bridge_endpoint(struct tc_data *tc)
> > +{
> > +	struct device *dev = tc->dev;
> > +	struct drm_panel *panel;
> > +	int ret;
> > +
> > +	/* port@1 is the DPI input/output port */
> > +	ret = drm_of_find_panel_or_bridge(dev->of_node, 1, 0, &panel, NULL);
> > +	if (ret && ret != -ENODEV)
> > +		return ret;
> > +
> > +	if (panel) {
> > +		struct drm_bridge *panel_bridge;
> > +
> > +		panel_bridge = devm_drm_panel_bridge_add(dev, panel);
> > +		if (IS_ERR(panel_bridge))
> > +			return PTR_ERR(panel_bridge);
> > +
> > +		tc->panel_bridge = panel_bridge;
> > +		tc->bridge.type = DRM_MODE_CONNECTOR_DPI;
> > +		tc->bridge.funcs = &tc_dpi_bridge_funcs;
> > +
> > +		return 0;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  static int tc_probe_edp_bridge_endpoint(struct tc_data *tc)
> >  {
> >  	struct device *dev = tc->dev;
> > @@ -1745,7 +2062,7 @@ static int tc_probe_bridge_endpoint(struct tc_data *tc)
> >  	if (mode == mode_dpi_to_edp)
> >  		return tc_probe_edp_bridge_endpoint(tc);
> >  	else if (mode == mode_dsi_to_dpi)
> > -		dev_warn(dev, "The mode DSI-to-DPI is not supported!\n");
> > +		return tc_probe_dpi_bridge_endpoint(tc);
> >  	else if (mode == mode_dsi_to_edp)
> >  		dev_warn(dev, "The mode DSI-to-(e)DP is not supported!\n");
> >  	else
> > @@ -1828,15 +2145,23 @@ static int tc_probe(struct i2c_client *client, const struct i2c_device_id *id)
> >  		tc->have_irq = true;
> >  	}
> >  
> > -	ret = tc_aux_link_setup(tc);
> > -	if (ret)
> > -		return ret;
> > +	if (tc->bridge.type != DRM_MODE_CONNECTOR_DPI) { /* (e)DP output */
> > +		ret = tc_aux_link_setup(tc);
> > +		if (ret)
> > +			return ret;
> > +	}
> >  
> >  	tc->bridge.of_node = dev->of_node;
> >  	drm_bridge_add(&tc->bridge);
> >  
> >  	i2c_set_clientdata(client, tc);
> >  
> > +	if (tc->bridge.type == DRM_MODE_CONNECTOR_DPI) { /* DPI output */
> > +		ret = tc_mipi_dsi_host_attach(tc);
> > +		if (ret)
> > +			return ret;
> > +	}
> 
> If tc_mipi_dsi_host_attach fails the drm bridge registered a few lines
> above isn't cleaned up properly.
> 
> Regards,
> Lucas