[03/23] drm/i915: Move the TypeC port handling code to a separate file

Submitted by Imre Deak on June 4, 2019, 2:58 p.m.

Details

Message ID 20190604145826.16424-4-imre.deak@intel.com
State New
Headers show
Series "drm/i915: Fix TypeC port mode switching" ( rev: 2 1 ) in Intel GFX

Not browsing as part of any series.

Commit Message

Imre Deak June 4, 2019, 2:58 p.m.
Move the TypeC port handling functions to a new file for clarity.

While at it:
- s/icl_tc_port_connected()/intel_tc_port_connected()/
  icl_tc_phy_disconnect(), will be unexported later.

- s/intel_dp_get_fia_supported_lane_count()/intel_tc_port_fia_max_lane_count()/
  It's used for HDMI legacy mode too.

- Simplify function interfaces by passing only dig_port to them.

No functional changes.

Cc: Animesh Manna <animesh.manna@intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: José Roberto de Souza <jose.souza@intel.com>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/Makefile             |   3 +-
 drivers/gpu/drm/i915/Makefile.header-test |   1 +
 drivers/gpu/drm/i915/intel_ddi.c          |   6 +-
 drivers/gpu/drm/i915/intel_dp.c           | 227 +--------------------
 drivers/gpu/drm/i915/intel_dp.h           |   2 -
 drivers/gpu/drm/i915/intel_tc.c           | 230 ++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_tc.h           |  13 ++
 7 files changed, 252 insertions(+), 230 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_tc.c
 create mode 100644 drivers/gpu/drm/i915/intel_tc.h

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index c0a7b2994077..74c4d11d83eb 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -171,7 +171,8 @@  i915-y += intel_audio.o \
 	  intel_psr.o \
 	  intel_quirks.o \
 	  intel_sideband.o \
-	  intel_sprite.o
+	  intel_sprite.o \
+	  intel_tc.o
 i915-$(CONFIG_ACPI)		+= intel_acpi.o intel_opregion.o
 i915-$(CONFIG_DRM_FBDEV_EMULATION)	+= intel_fbdev.o
 
diff --git a/drivers/gpu/drm/i915/Makefile.header-test b/drivers/gpu/drm/i915/Makefile.header-test
index 6ef3b647ac65..e80e8e45b09c 100644
--- a/drivers/gpu/drm/i915/Makefile.header-test
+++ b/drivers/gpu/drm/i915/Makefile.header-test
@@ -58,6 +58,7 @@  header_test := \
 	intel_sdvo.h \
 	intel_sideband.h \
 	intel_sprite.h \
+	intel_tc.h \
 	intel_tv.h \
 	intel_uncore.h \
 	intel_vdsc.h \
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 350eaf54f01f..5a1c98438375 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -45,6 +45,7 @@ 
 #include "intel_lspcon.h"
 #include "intel_panel.h"
 #include "intel_psr.h"
+#include "intel_tc.h"
 #include "intel_vdsc.h"
 
 struct ddi_buf_trans {
@@ -3904,7 +3905,6 @@  static int intel_ddi_compute_config(struct intel_encoder *encoder,
 static void intel_ddi_encoder_suspend(struct intel_encoder *encoder)
 {
 	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
-	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
 
 	intel_dp_encoder_suspend(encoder);
 
@@ -3914,7 +3914,7 @@  static void intel_ddi_encoder_suspend(struct intel_encoder *encoder)
 	 * even if the sink has disappeared while being suspended.
 	 */
 	if (dig_port->tc_legacy_port)
-		icl_tc_phy_disconnect(i915, dig_port);
+		icl_tc_phy_disconnect(dig_port);
 }
 
 static void intel_ddi_encoder_reset(struct drm_encoder *drm_encoder)
@@ -3936,7 +3936,7 @@  static void intel_ddi_encoder_destroy(struct drm_encoder *encoder)
 	intel_dp_encoder_flush_work(encoder);
 
 	if (intel_port_is_tc(i915, dig_port->base.port))
-		icl_tc_phy_disconnect(i915, dig_port);
+		icl_tc_phy_disconnect(dig_port);
 
 	drm_encoder_cleanup(encoder);
 	kfree(dig_port);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 24b56b2a76c8..b69310bd9914 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -62,6 +62,7 @@ 
 #include "intel_panel.h"
 #include "intel_psr.h"
 #include "intel_sideband.h"
+#include "intel_tc.h"
 #include "intel_vdsc.h"
 
 #define DP_DPRX_ESI_LEN 14
@@ -211,46 +212,13 @@  static int intel_dp_max_common_rate(struct intel_dp *intel_dp)
 	return intel_dp->common_rates[intel_dp->num_common_rates - 1];
 }
 
-static int intel_dp_get_fia_supported_lane_count(struct intel_dp *intel_dp)
-{
-	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
-	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
-	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
-	intel_wakeref_t wakeref;
-	u32 lane_info;
-
-	if (tc_port == PORT_TC_NONE || dig_port->tc_type != TC_PORT_TYPEC)
-		return 4;
-
-	lane_info = 0;
-	with_intel_display_power(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref)
-		lane_info = (I915_READ(PORT_TX_DFLEXDPSP) &
-			     DP_LANE_ASSIGNMENT_MASK(tc_port)) >>
-				DP_LANE_ASSIGNMENT_SHIFT(tc_port);
-
-	switch (lane_info) {
-	default:
-		MISSING_CASE(lane_info);
-	case 1:
-	case 2:
-	case 4:
-	case 8:
-		return 1;
-	case 3:
-	case 12:
-		return 2;
-	case 15:
-		return 4;
-	}
-}
-
 /* Theoretical max between source and sink */
 static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	int source_max = intel_dig_port->max_lanes;
 	int sink_max = drm_dp_max_lane_count(intel_dp->dpcd);
-	int fia_max = intel_dp_get_fia_supported_lane_count(intel_dp);
+	int fia_max = intel_tc_port_fia_max_lane_count(intel_dig_port);
 
 	return min3(source_max, sink_max, fia_max);
 }
@@ -5231,195 +5199,6 @@  static bool icl_combo_port_connected(struct drm_i915_private *dev_priv,
 	return I915_READ(SDEISR) & SDE_DDI_HOTPLUG_ICP(port);
 }
 
-static const char *tc_type_name(enum tc_port_type type)
-{
-	static const char * const names[] = {
-		[TC_PORT_UNKNOWN] = "unknown",
-		[TC_PORT_LEGACY] = "legacy",
-		[TC_PORT_TYPEC] = "typec",
-		[TC_PORT_TBT] = "tbt",
-	};
-
-	if (WARN_ON(type >= ARRAY_SIZE(names)))
-		type = TC_PORT_UNKNOWN;
-
-	return names[type];
-}
-
-static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
-				    struct intel_digital_port *intel_dig_port,
-				    bool is_legacy, bool is_typec, bool is_tbt)
-{
-	enum port port = intel_dig_port->base.port;
-	enum tc_port_type old_type = intel_dig_port->tc_type;
-
-	WARN_ON(is_legacy + is_typec + is_tbt != 1);
-
-	if (is_legacy)
-		intel_dig_port->tc_type = TC_PORT_LEGACY;
-	else if (is_typec)
-		intel_dig_port->tc_type = TC_PORT_TYPEC;
-	else if (is_tbt)
-		intel_dig_port->tc_type = TC_PORT_TBT;
-	else
-		return;
-
-	/* Types are not supposed to be changed at runtime. */
-	WARN_ON(old_type != TC_PORT_UNKNOWN &&
-		old_type != intel_dig_port->tc_type);
-
-	if (old_type != intel_dig_port->tc_type)
-		DRM_DEBUG_KMS("Port %c has TC type %s\n", port_name(port),
-			      tc_type_name(intel_dig_port->tc_type));
-}
-
-/*
- * This function implements the first part of the Connect Flow described by our
- * specification, Gen11 TypeC Programming chapter. The rest of the flow (reading
- * lanes, EDID, etc) is done as needed in the typical places.
- *
- * Unlike the other ports, type-C ports are not available to use as soon as we
- * get a hotplug. The type-C PHYs can be shared between multiple controllers:
- * display, USB, etc. As a result, handshaking through FIA is required around
- * connect and disconnect to cleanly transfer ownership with the controller and
- * set the type-C power state.
- *
- * We could opt to only do the connect flow when we actually try to use the AUX
- * channels or do a modeset, then immediately run the disconnect flow after
- * usage, but there are some implications on this for a dynamic environment:
- * things may go away or change behind our backs. So for now our driver is
- * always trying to acquire ownership of the controller as soon as it gets an
- * interrupt (or polls state and sees a port is connected) and only gives it
- * back when it sees a disconnect. Implementation of a more fine-grained model
- * will require a lot of coordination with user space and thorough testing for
- * the extra possible cases.
- */
-static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
-			       struct intel_digital_port *dig_port)
-{
-	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
-	u32 val;
-
-	if (dig_port->tc_type != TC_PORT_LEGACY &&
-	    dig_port->tc_type != TC_PORT_TYPEC)
-		return true;
-
-	val = I915_READ(PORT_TX_DFLEXDPPMS);
-	if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) {
-		DRM_DEBUG_KMS("DP PHY for TC port %d not ready\n", tc_port);
-		WARN_ON(dig_port->tc_legacy_port);
-		return false;
-	}
-
-	/*
-	 * This function may be called many times in a row without an HPD event
-	 * in between, so try to avoid the write when we can.
-	 */
-	val = I915_READ(PORT_TX_DFLEXDPCSSS);
-	if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) {
-		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
-		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
-	}
-
-	/*
-	 * Now we have to re-check the live state, in case the port recently
-	 * became disconnected. Not necessary for legacy mode.
-	 */
-	if (dig_port->tc_type == TC_PORT_TYPEC &&
-	    !(I915_READ(PORT_TX_DFLEXDPSP) & TC_LIVE_STATE_TC(tc_port))) {
-		DRM_DEBUG_KMS("TC PHY %d sudden disconnect.\n", tc_port);
-		icl_tc_phy_disconnect(dev_priv, dig_port);
-		return false;
-	}
-
-	return true;
-}
-
-/*
- * See the comment at the connect function. This implements the Disconnect
- * Flow.
- */
-void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
-			   struct intel_digital_port *dig_port)
-{
-	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
-
-	if (dig_port->tc_type == TC_PORT_UNKNOWN)
-		return;
-
-	/*
-	 * TBT disconnection flow is read the live status, what was done in
-	 * caller.
-	 */
-	if (dig_port->tc_type == TC_PORT_TYPEC ||
-	    dig_port->tc_type == TC_PORT_LEGACY) {
-		u32 val;
-
-		val = I915_READ(PORT_TX_DFLEXDPCSSS);
-		val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
-		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
-	}
-
-	DRM_DEBUG_KMS("Port %c TC type %s disconnected\n",
-		      port_name(dig_port->base.port),
-		      tc_type_name(dig_port->tc_type));
-
-	dig_port->tc_type = TC_PORT_UNKNOWN;
-}
-
-/*
- * The type-C ports are different because even when they are connected, they may
- * not be available/usable by the graphics driver: see the comment on
- * icl_tc_phy_connect(). So in our driver instead of adding the additional
- * concept of "usable" and make everything check for "connected and usable" we
- * define a port as "connected" when it is not only connected, but also when it
- * is usable by the rest of the driver. That maintains the old assumption that
- * connected ports are usable, and avoids exposing to the users objects they
- * can't really use.
- */
-static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
-				  struct intel_digital_port *intel_dig_port)
-{
-	enum port port = intel_dig_port->base.port;
-	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
-	bool is_legacy, is_typec, is_tbt;
-	u32 dpsp;
-
-	/*
-	 * Complain if we got a legacy port HPD, but VBT didn't mark the port as
-	 * legacy. Treat the port as legacy from now on.
-	 */
-	if (!intel_dig_port->tc_legacy_port &&
-	    I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) {
-		DRM_ERROR("VBT incorrectly claims port %c is not TypeC legacy\n",
-			  port_name(port));
-		intel_dig_port->tc_legacy_port = true;
-	}
-	is_legacy = intel_dig_port->tc_legacy_port;
-
-	/*
-	 * The spec says we shouldn't be using the ISR bits for detecting
-	 * between TC and TBT. We should use DFLEXDPSP.
-	 */
-	dpsp = I915_READ(PORT_TX_DFLEXDPSP);
-	is_typec = dpsp & TC_LIVE_STATE_TC(tc_port);
-	is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port);
-
-	if (!is_legacy && !is_typec && !is_tbt) {
-		icl_tc_phy_disconnect(dev_priv, intel_dig_port);
-
-		return false;
-	}
-
-	icl_update_tc_port_type(dev_priv, intel_dig_port, is_legacy, is_typec,
-				is_tbt);
-
-	if (!icl_tc_phy_connect(dev_priv, intel_dig_port))
-		return false;
-
-	return true;
-}
-
 static bool icl_digital_port_connected(struct intel_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
@@ -5428,7 +5207,7 @@  static bool icl_digital_port_connected(struct intel_encoder *encoder)
 	if (intel_port_is_combophy(dev_priv, encoder->port))
 		return icl_combo_port_connected(dev_priv, dig_port);
 	else if (intel_port_is_tc(dev_priv, encoder->port))
-		return icl_tc_port_connected(dev_priv, dig_port);
+		return intel_tc_port_connected(dig_port);
 	else
 		MISSING_CASE(encoder->hpd_pin);
 
diff --git a/drivers/gpu/drm/i915/intel_dp.h b/drivers/gpu/drm/i915/intel_dp.h
index da70b1a41c83..657bbb1f5ed0 100644
--- a/drivers/gpu/drm/i915/intel_dp.h
+++ b/drivers/gpu/drm/i915/intel_dp.h
@@ -112,8 +112,6 @@  bool intel_dp_get_colorimetry_status(struct intel_dp *intel_dp);
 int intel_dp_link_required(int pixel_clock, int bpp);
 int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
 bool intel_digital_port_connected(struct intel_encoder *encoder);
-void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
-			   struct intel_digital_port *dig_port);
 
 static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
 {
diff --git a/drivers/gpu/drm/i915/intel_tc.c b/drivers/gpu/drm/i915/intel_tc.c
new file mode 100644
index 000000000000..7a1b5870945f
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_tc.c
@@ -0,0 +1,230 @@ 
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright © 2019 Intel Corporation
+ */
+#include "intel_display.h"
+#include "i915_drv.h"
+#include "intel_tc.h"
+
+static const char *tc_type_name(enum tc_port_type type)
+{
+	static const char * const names[] = {
+		[TC_PORT_UNKNOWN] = "unknown",
+		[TC_PORT_LEGACY] = "legacy",
+		[TC_PORT_TYPEC] = "typec",
+		[TC_PORT_TBT] = "tbt",
+	};
+
+	if (WARN_ON(type >= ARRAY_SIZE(names)))
+		type = TC_PORT_UNKNOWN;
+
+	return names[type];
+}
+
+int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port)
+{
+	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
+	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
+	intel_wakeref_t wakeref;
+	u32 lane_info;
+
+	if (tc_port == PORT_TC_NONE || dig_port->tc_type != TC_PORT_TYPEC)
+		return 4;
+
+	lane_info = 0;
+	with_intel_display_power(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref)
+		lane_info = (I915_READ(PORT_TX_DFLEXDPSP) &
+			     DP_LANE_ASSIGNMENT_MASK(tc_port)) >>
+				DP_LANE_ASSIGNMENT_SHIFT(tc_port);
+
+	switch (lane_info) {
+	default:
+		MISSING_CASE(lane_info);
+	case 1:
+	case 2:
+	case 4:
+	case 8:
+		return 1;
+	case 3:
+	case 12:
+		return 2;
+	case 15:
+		return 4;
+	}
+}
+
+/*
+ * This function implements the first part of the Connect Flow described by our
+ * specification, Gen11 TypeC Programming chapter. The rest of the flow (reading
+ * lanes, EDID, etc) is done as needed in the typical places.
+ *
+ * Unlike the other ports, type-C ports are not available to use as soon as we
+ * get a hotplug. The type-C PHYs can be shared between multiple controllers:
+ * display, USB, etc. As a result, handshaking through FIA is required around
+ * connect and disconnect to cleanly transfer ownership with the controller and
+ * set the type-C power state.
+ *
+ * We could opt to only do the connect flow when we actually try to use the AUX
+ * channels or do a modeset, then immediately run the disconnect flow after
+ * usage, but there are some implications on this for a dynamic environment:
+ * things may go away or change behind our backs. So for now our driver is
+ * always trying to acquire ownership of the controller as soon as it gets an
+ * interrupt (or polls state and sees a port is connected) and only gives it
+ * back when it sees a disconnect. Implementation of a more fine-grained model
+ * will require a lot of coordination with user space and thorough testing for
+ * the extra possible cases.
+ */
+static bool icl_tc_phy_connect(struct intel_digital_port *dig_port)
+{
+	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
+	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
+	u32 val;
+
+	if (dig_port->tc_type != TC_PORT_LEGACY &&
+	    dig_port->tc_type != TC_PORT_TYPEC)
+		return true;
+
+	val = I915_READ(PORT_TX_DFLEXDPPMS);
+	if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) {
+		DRM_DEBUG_KMS("DP PHY for TC port %d not ready\n", tc_port);
+		WARN_ON(dig_port->tc_legacy_port);
+		return false;
+	}
+
+	/*
+	 * This function may be called many times in a row without an HPD event
+	 * in between, so try to avoid the write when we can.
+	 */
+	val = I915_READ(PORT_TX_DFLEXDPCSSS);
+	if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) {
+		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
+		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
+	}
+
+	/*
+	 * Now we have to re-check the live state, in case the port recently
+	 * became disconnected. Not necessary for legacy mode.
+	 */
+	if (dig_port->tc_type == TC_PORT_TYPEC &&
+	    !(I915_READ(PORT_TX_DFLEXDPSP) & TC_LIVE_STATE_TC(tc_port))) {
+		DRM_DEBUG_KMS("TC PHY %d sudden disconnect.\n", tc_port);
+		icl_tc_phy_disconnect(dig_port);
+		return false;
+	}
+
+	return true;
+}
+
+/*
+ * See the comment at the connect function. This implements the Disconnect
+ * Flow.
+ */
+void icl_tc_phy_disconnect(struct intel_digital_port *dig_port)
+{
+	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
+	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
+
+	if (dig_port->tc_type == TC_PORT_UNKNOWN)
+		return;
+
+	/*
+	 * TBT disconnection flow is read the live status, what was done in
+	 * caller.
+	 */
+	if (dig_port->tc_type == TC_PORT_TYPEC ||
+	    dig_port->tc_type == TC_PORT_LEGACY) {
+		u32 val;
+
+		val = I915_READ(PORT_TX_DFLEXDPCSSS);
+		val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
+		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
+	}
+
+	DRM_DEBUG_KMS("Port %c TC type %s disconnected\n",
+		      port_name(dig_port->base.port),
+		      tc_type_name(dig_port->tc_type));
+
+	dig_port->tc_type = TC_PORT_UNKNOWN;
+}
+
+static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
+				    struct intel_digital_port *intel_dig_port,
+				    bool is_legacy, bool is_typec, bool is_tbt)
+{
+	enum port port = intel_dig_port->base.port;
+	enum tc_port_type old_type = intel_dig_port->tc_type;
+
+	WARN_ON(is_legacy + is_typec + is_tbt != 1);
+
+	if (is_legacy)
+		intel_dig_port->tc_type = TC_PORT_LEGACY;
+	else if (is_typec)
+		intel_dig_port->tc_type = TC_PORT_TYPEC;
+	else if (is_tbt)
+		intel_dig_port->tc_type = TC_PORT_TBT;
+	else
+		return;
+
+	/* Types are not supposed to be changed at runtime. */
+	WARN_ON(old_type != TC_PORT_UNKNOWN &&
+		old_type != intel_dig_port->tc_type);
+
+	if (old_type != intel_dig_port->tc_type)
+		DRM_DEBUG_KMS("Port %c has TC type %s\n", port_name(port),
+			      tc_type_name(intel_dig_port->tc_type));
+}
+
+
+/*
+ * The type-C ports are different because even when they are connected, they may
+ * not be available/usable by the graphics driver: see the comment on
+ * icl_tc_phy_connect(). So in our driver instead of adding the additional
+ * concept of "usable" and make everything check for "connected and usable" we
+ * define a port as "connected" when it is not only connected, but also when it
+ * is usable by the rest of the driver. That maintains the old assumption that
+ * connected ports are usable, and avoids exposing to the users objects they
+ * can't really use.
+ */
+bool intel_tc_port_connected(struct intel_digital_port *dig_port)
+{
+	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
+	enum port port = dig_port->base.port;
+	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
+	bool is_legacy, is_typec, is_tbt;
+	u32 dpsp;
+
+	/*
+	 * Complain if we got a legacy port HPD, but VBT didn't mark the port as
+	 * legacy. Treat the port as legacy from now on.
+	 */
+	if (!dig_port->tc_legacy_port &&
+	    I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) {
+		DRM_ERROR("VBT incorrectly claims port %c is not TypeC legacy\n",
+			  port_name(port));
+		dig_port->tc_legacy_port = true;
+	}
+	is_legacy = dig_port->tc_legacy_port;
+
+	/*
+	 * The spec says we shouldn't be using the ISR bits for detecting
+	 * between TC and TBT. We should use DFLEXDPSP.
+	 */
+	dpsp = I915_READ(PORT_TX_DFLEXDPSP);
+	is_typec = dpsp & TC_LIVE_STATE_TC(tc_port);
+	is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port);
+
+	if (!is_legacy && !is_typec && !is_tbt) {
+		icl_tc_phy_disconnect(dig_port);
+
+		return false;
+	}
+
+	icl_update_tc_port_type(dev_priv, dig_port, is_legacy, is_typec,
+				is_tbt);
+
+	if (!icl_tc_phy_connect(dig_port))
+		return false;
+
+	return true;
+}
+
diff --git a/drivers/gpu/drm/i915/intel_tc.h b/drivers/gpu/drm/i915/intel_tc.h
new file mode 100644
index 000000000000..94c62ac4a162
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_tc.h
@@ -0,0 +1,13 @@ 
+#ifndef __INTEL_TC_H__
+#define __INTEL_TC_H__
+
+#include <linux/types.h>
+
+struct intel_digital_port;
+
+void icl_tc_phy_disconnect(struct intel_digital_port *dig_port);
+
+bool intel_tc_port_connected(struct intel_digital_port *dig_port);
+int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port);
+
+#endif /* __INTEL_TC_H__ */

Comments

On Tue, 04 Jun 2019, Imre Deak <imre.deak@intel.com> wrote:
> Move the TypeC port handling functions to a new file for clarity.
>
> While at it:
> - s/icl_tc_port_connected()/intel_tc_port_connected()/
>   icl_tc_phy_disconnect(), will be unexported later.
>
> - s/intel_dp_get_fia_supported_lane_count()/intel_tc_port_fia_max_lane_count()/
>   It's used for HDMI legacy mode too.
>
> - Simplify function interfaces by passing only dig_port to them.
>
> No functional changes.

Some nitpicks below.

BR,
Jani.


>
> Cc: Animesh Manna <animesh.manna@intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: José Roberto de Souza <jose.souza@intel.com>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile             |   3 +-
>  drivers/gpu/drm/i915/Makefile.header-test |   1 +
>  drivers/gpu/drm/i915/intel_ddi.c          |   6 +-
>  drivers/gpu/drm/i915/intel_dp.c           | 227 +--------------------
>  drivers/gpu/drm/i915/intel_dp.h           |   2 -
>  drivers/gpu/drm/i915/intel_tc.c           | 230 ++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_tc.h           |  13 ++
>  7 files changed, 252 insertions(+), 230 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/intel_tc.c
>  create mode 100644 drivers/gpu/drm/i915/intel_tc.h
>
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index c0a7b2994077..74c4d11d83eb 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -171,7 +171,8 @@ i915-y += intel_audio.o \
>  	  intel_psr.o \
>  	  intel_quirks.o \
>  	  intel_sideband.o \
> -	  intel_sprite.o
> +	  intel_sprite.o \
> +	  intel_tc.o
>  i915-$(CONFIG_ACPI)		+= intel_acpi.o intel_opregion.o
>  i915-$(CONFIG_DRM_FBDEV_EMULATION)	+= intel_fbdev.o
>  
> diff --git a/drivers/gpu/drm/i915/Makefile.header-test b/drivers/gpu/drm/i915/Makefile.header-test
> index 6ef3b647ac65..e80e8e45b09c 100644
> --- a/drivers/gpu/drm/i915/Makefile.header-test
> +++ b/drivers/gpu/drm/i915/Makefile.header-test
> @@ -58,6 +58,7 @@ header_test := \
>  	intel_sdvo.h \
>  	intel_sideband.h \
>  	intel_sprite.h \
> +	intel_tc.h \
>  	intel_tv.h \
>  	intel_uncore.h \
>  	intel_vdsc.h \
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 350eaf54f01f..5a1c98438375 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -45,6 +45,7 @@
>  #include "intel_lspcon.h"
>  #include "intel_panel.h"
>  #include "intel_psr.h"
> +#include "intel_tc.h"
>  #include "intel_vdsc.h"
>  
>  struct ddi_buf_trans {
> @@ -3904,7 +3905,6 @@ static int intel_ddi_compute_config(struct intel_encoder *encoder,
>  static void intel_ddi_encoder_suspend(struct intel_encoder *encoder)
>  {
>  	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> -	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>  
>  	intel_dp_encoder_suspend(encoder);
>  
> @@ -3914,7 +3914,7 @@ static void intel_ddi_encoder_suspend(struct intel_encoder *encoder)
>  	 * even if the sink has disappeared while being suspended.
>  	 */
>  	if (dig_port->tc_legacy_port)
> -		icl_tc_phy_disconnect(i915, dig_port);
> +		icl_tc_phy_disconnect(dig_port);
>  }
>  
>  static void intel_ddi_encoder_reset(struct drm_encoder *drm_encoder)
> @@ -3936,7 +3936,7 @@ static void intel_ddi_encoder_destroy(struct drm_encoder *encoder)
>  	intel_dp_encoder_flush_work(encoder);
>  
>  	if (intel_port_is_tc(i915, dig_port->base.port))
> -		icl_tc_phy_disconnect(i915, dig_port);
> +		icl_tc_phy_disconnect(dig_port);
>  
>  	drm_encoder_cleanup(encoder);
>  	kfree(dig_port);
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 24b56b2a76c8..b69310bd9914 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -62,6 +62,7 @@
>  #include "intel_panel.h"
>  #include "intel_psr.h"
>  #include "intel_sideband.h"
> +#include "intel_tc.h"
>  #include "intel_vdsc.h"
>  
>  #define DP_DPRX_ESI_LEN 14
> @@ -211,46 +212,13 @@ static int intel_dp_max_common_rate(struct intel_dp *intel_dp)
>  	return intel_dp->common_rates[intel_dp->num_common_rates - 1];
>  }
>  
> -static int intel_dp_get_fia_supported_lane_count(struct intel_dp *intel_dp)
> -{
> -	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> -	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> -	intel_wakeref_t wakeref;
> -	u32 lane_info;
> -
> -	if (tc_port == PORT_TC_NONE || dig_port->tc_type != TC_PORT_TYPEC)
> -		return 4;
> -
> -	lane_info = 0;
> -	with_intel_display_power(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref)
> -		lane_info = (I915_READ(PORT_TX_DFLEXDPSP) &
> -			     DP_LANE_ASSIGNMENT_MASK(tc_port)) >>
> -				DP_LANE_ASSIGNMENT_SHIFT(tc_port);
> -
> -	switch (lane_info) {
> -	default:
> -		MISSING_CASE(lane_info);
> -	case 1:
> -	case 2:
> -	case 4:
> -	case 8:
> -		return 1;
> -	case 3:
> -	case 12:
> -		return 2;
> -	case 15:
> -		return 4;
> -	}
> -}
> -
>  /* Theoretical max between source and sink */
>  static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp)
>  {
>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>  	int source_max = intel_dig_port->max_lanes;
>  	int sink_max = drm_dp_max_lane_count(intel_dp->dpcd);
> -	int fia_max = intel_dp_get_fia_supported_lane_count(intel_dp);
> +	int fia_max = intel_tc_port_fia_max_lane_count(intel_dig_port);
>  
>  	return min3(source_max, sink_max, fia_max);
>  }
> @@ -5231,195 +5199,6 @@ static bool icl_combo_port_connected(struct drm_i915_private *dev_priv,
>  	return I915_READ(SDEISR) & SDE_DDI_HOTPLUG_ICP(port);
>  }
>  
> -static const char *tc_type_name(enum tc_port_type type)
> -{
> -	static const char * const names[] = {
> -		[TC_PORT_UNKNOWN] = "unknown",
> -		[TC_PORT_LEGACY] = "legacy",
> -		[TC_PORT_TYPEC] = "typec",
> -		[TC_PORT_TBT] = "tbt",
> -	};
> -
> -	if (WARN_ON(type >= ARRAY_SIZE(names)))
> -		type = TC_PORT_UNKNOWN;
> -
> -	return names[type];
> -}
> -
> -static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
> -				    struct intel_digital_port *intel_dig_port,
> -				    bool is_legacy, bool is_typec, bool is_tbt)
> -{
> -	enum port port = intel_dig_port->base.port;
> -	enum tc_port_type old_type = intel_dig_port->tc_type;
> -
> -	WARN_ON(is_legacy + is_typec + is_tbt != 1);
> -
> -	if (is_legacy)
> -		intel_dig_port->tc_type = TC_PORT_LEGACY;
> -	else if (is_typec)
> -		intel_dig_port->tc_type = TC_PORT_TYPEC;
> -	else if (is_tbt)
> -		intel_dig_port->tc_type = TC_PORT_TBT;
> -	else
> -		return;
> -
> -	/* Types are not supposed to be changed at runtime. */
> -	WARN_ON(old_type != TC_PORT_UNKNOWN &&
> -		old_type != intel_dig_port->tc_type);
> -
> -	if (old_type != intel_dig_port->tc_type)
> -		DRM_DEBUG_KMS("Port %c has TC type %s\n", port_name(port),
> -			      tc_type_name(intel_dig_port->tc_type));
> -}
> -
> -/*
> - * This function implements the first part of the Connect Flow described by our
> - * specification, Gen11 TypeC Programming chapter. The rest of the flow (reading
> - * lanes, EDID, etc) is done as needed in the typical places.
> - *
> - * Unlike the other ports, type-C ports are not available to use as soon as we
> - * get a hotplug. The type-C PHYs can be shared between multiple controllers:
> - * display, USB, etc. As a result, handshaking through FIA is required around
> - * connect and disconnect to cleanly transfer ownership with the controller and
> - * set the type-C power state.
> - *
> - * We could opt to only do the connect flow when we actually try to use the AUX
> - * channels or do a modeset, then immediately run the disconnect flow after
> - * usage, but there are some implications on this for a dynamic environment:
> - * things may go away or change behind our backs. So for now our driver is
> - * always trying to acquire ownership of the controller as soon as it gets an
> - * interrupt (or polls state and sees a port is connected) and only gives it
> - * back when it sees a disconnect. Implementation of a more fine-grained model
> - * will require a lot of coordination with user space and thorough testing for
> - * the extra possible cases.
> - */
> -static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
> -			       struct intel_digital_port *dig_port)
> -{
> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> -	u32 val;
> -
> -	if (dig_port->tc_type != TC_PORT_LEGACY &&
> -	    dig_port->tc_type != TC_PORT_TYPEC)
> -		return true;
> -
> -	val = I915_READ(PORT_TX_DFLEXDPPMS);
> -	if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) {
> -		DRM_DEBUG_KMS("DP PHY for TC port %d not ready\n", tc_port);
> -		WARN_ON(dig_port->tc_legacy_port);
> -		return false;
> -	}
> -
> -	/*
> -	 * This function may be called many times in a row without an HPD event
> -	 * in between, so try to avoid the write when we can.
> -	 */
> -	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> -	if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) {
> -		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> -		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> -	}
> -
> -	/*
> -	 * Now we have to re-check the live state, in case the port recently
> -	 * became disconnected. Not necessary for legacy mode.
> -	 */
> -	if (dig_port->tc_type == TC_PORT_TYPEC &&
> -	    !(I915_READ(PORT_TX_DFLEXDPSP) & TC_LIVE_STATE_TC(tc_port))) {
> -		DRM_DEBUG_KMS("TC PHY %d sudden disconnect.\n", tc_port);
> -		icl_tc_phy_disconnect(dev_priv, dig_port);
> -		return false;
> -	}
> -
> -	return true;
> -}
> -
> -/*
> - * See the comment at the connect function. This implements the Disconnect
> - * Flow.
> - */
> -void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
> -			   struct intel_digital_port *dig_port)
> -{
> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> -
> -	if (dig_port->tc_type == TC_PORT_UNKNOWN)
> -		return;
> -
> -	/*
> -	 * TBT disconnection flow is read the live status, what was done in
> -	 * caller.
> -	 */
> -	if (dig_port->tc_type == TC_PORT_TYPEC ||
> -	    dig_port->tc_type == TC_PORT_LEGACY) {
> -		u32 val;
> -
> -		val = I915_READ(PORT_TX_DFLEXDPCSSS);
> -		val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> -		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> -	}
> -
> -	DRM_DEBUG_KMS("Port %c TC type %s disconnected\n",
> -		      port_name(dig_port->base.port),
> -		      tc_type_name(dig_port->tc_type));
> -
> -	dig_port->tc_type = TC_PORT_UNKNOWN;
> -}
> -
> -/*
> - * The type-C ports are different because even when they are connected, they may
> - * not be available/usable by the graphics driver: see the comment on
> - * icl_tc_phy_connect(). So in our driver instead of adding the additional
> - * concept of "usable" and make everything check for "connected and usable" we
> - * define a port as "connected" when it is not only connected, but also when it
> - * is usable by the rest of the driver. That maintains the old assumption that
> - * connected ports are usable, and avoids exposing to the users objects they
> - * can't really use.
> - */
> -static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
> -				  struct intel_digital_port *intel_dig_port)
> -{
> -	enum port port = intel_dig_port->base.port;
> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
> -	bool is_legacy, is_typec, is_tbt;
> -	u32 dpsp;
> -
> -	/*
> -	 * Complain if we got a legacy port HPD, but VBT didn't mark the port as
> -	 * legacy. Treat the port as legacy from now on.
> -	 */
> -	if (!intel_dig_port->tc_legacy_port &&
> -	    I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) {
> -		DRM_ERROR("VBT incorrectly claims port %c is not TypeC legacy\n",
> -			  port_name(port));
> -		intel_dig_port->tc_legacy_port = true;
> -	}
> -	is_legacy = intel_dig_port->tc_legacy_port;
> -
> -	/*
> -	 * The spec says we shouldn't be using the ISR bits for detecting
> -	 * between TC and TBT. We should use DFLEXDPSP.
> -	 */
> -	dpsp = I915_READ(PORT_TX_DFLEXDPSP);
> -	is_typec = dpsp & TC_LIVE_STATE_TC(tc_port);
> -	is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port);
> -
> -	if (!is_legacy && !is_typec && !is_tbt) {
> -		icl_tc_phy_disconnect(dev_priv, intel_dig_port);
> -
> -		return false;
> -	}
> -
> -	icl_update_tc_port_type(dev_priv, intel_dig_port, is_legacy, is_typec,
> -				is_tbt);
> -
> -	if (!icl_tc_phy_connect(dev_priv, intel_dig_port))
> -		return false;
> -
> -	return true;
> -}
> -
>  static bool icl_digital_port_connected(struct intel_encoder *encoder)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> @@ -5428,7 +5207,7 @@ static bool icl_digital_port_connected(struct intel_encoder *encoder)
>  	if (intel_port_is_combophy(dev_priv, encoder->port))
>  		return icl_combo_port_connected(dev_priv, dig_port);
>  	else if (intel_port_is_tc(dev_priv, encoder->port))
> -		return icl_tc_port_connected(dev_priv, dig_port);
> +		return intel_tc_port_connected(dig_port);
>  	else
>  		MISSING_CASE(encoder->hpd_pin);
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp.h b/drivers/gpu/drm/i915/intel_dp.h
> index da70b1a41c83..657bbb1f5ed0 100644
> --- a/drivers/gpu/drm/i915/intel_dp.h
> +++ b/drivers/gpu/drm/i915/intel_dp.h
> @@ -112,8 +112,6 @@ bool intel_dp_get_colorimetry_status(struct intel_dp *intel_dp);
>  int intel_dp_link_required(int pixel_clock, int bpp);
>  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
>  bool intel_digital_port_connected(struct intel_encoder *encoder);
> -void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
> -			   struct intel_digital_port *dig_port);
>  
>  static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
>  {
> diff --git a/drivers/gpu/drm/i915/intel_tc.c b/drivers/gpu/drm/i915/intel_tc.c
> new file mode 100644
> index 000000000000..7a1b5870945f
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_tc.c
> @@ -0,0 +1,230 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2019 Intel Corporation
> + */

From the Nitpicks Department, please add a blank line here.

> +#include "intel_display.h"
> +#include "i915_drv.h"
> +#include "intel_tc.h"

And sort the includes.

> +
> +static const char *tc_type_name(enum tc_port_type type)
> +{
> +	static const char * const names[] = {
> +		[TC_PORT_UNKNOWN] = "unknown",
> +		[TC_PORT_LEGACY] = "legacy",
> +		[TC_PORT_TYPEC] = "typec",
> +		[TC_PORT_TBT] = "tbt",
> +	};
> +
> +	if (WARN_ON(type >= ARRAY_SIZE(names)))
> +		type = TC_PORT_UNKNOWN;
> +
> +	return names[type];
> +}
> +
> +int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> +	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> +	intel_wakeref_t wakeref;
> +	u32 lane_info;
> +
> +	if (tc_port == PORT_TC_NONE || dig_port->tc_type != TC_PORT_TYPEC)
> +		return 4;
> +
> +	lane_info = 0;
> +	with_intel_display_power(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref)
> +		lane_info = (I915_READ(PORT_TX_DFLEXDPSP) &
> +			     DP_LANE_ASSIGNMENT_MASK(tc_port)) >>
> +				DP_LANE_ASSIGNMENT_SHIFT(tc_port);
> +
> +	switch (lane_info) {
> +	default:
> +		MISSING_CASE(lane_info);
> +	case 1:
> +	case 2:
> +	case 4:
> +	case 8:
> +		return 1;
> +	case 3:
> +	case 12:
> +		return 2;
> +	case 15:
> +		return 4;
> +	}
> +}
> +
> +/*
> + * This function implements the first part of the Connect Flow described by our
> + * specification, Gen11 TypeC Programming chapter. The rest of the flow (reading
> + * lanes, EDID, etc) is done as needed in the typical places.
> + *
> + * Unlike the other ports, type-C ports are not available to use as soon as we
> + * get a hotplug. The type-C PHYs can be shared between multiple controllers:
> + * display, USB, etc. As a result, handshaking through FIA is required around
> + * connect and disconnect to cleanly transfer ownership with the controller and
> + * set the type-C power state.
> + *
> + * We could opt to only do the connect flow when we actually try to use the AUX
> + * channels or do a modeset, then immediately run the disconnect flow after
> + * usage, but there are some implications on this for a dynamic environment:
> + * things may go away or change behind our backs. So for now our driver is
> + * always trying to acquire ownership of the controller as soon as it gets an
> + * interrupt (or polls state and sees a port is connected) and only gives it
> + * back when it sees a disconnect. Implementation of a more fine-grained model
> + * will require a lot of coordination with user space and thorough testing for
> + * the extra possible cases.
> + */
> +static bool icl_tc_phy_connect(struct intel_digital_port *dig_port)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> +	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> +	u32 val;
> +
> +	if (dig_port->tc_type != TC_PORT_LEGACY &&
> +	    dig_port->tc_type != TC_PORT_TYPEC)
> +		return true;
> +
> +	val = I915_READ(PORT_TX_DFLEXDPPMS);
> +	if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) {
> +		DRM_DEBUG_KMS("DP PHY for TC port %d not ready\n", tc_port);
> +		WARN_ON(dig_port->tc_legacy_port);
> +		return false;
> +	}
> +
> +	/*
> +	 * This function may be called many times in a row without an HPD event
> +	 * in between, so try to avoid the write when we can.
> +	 */
> +	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> +	if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) {
> +		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> +		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> +	}
> +
> +	/*
> +	 * Now we have to re-check the live state, in case the port recently
> +	 * became disconnected. Not necessary for legacy mode.
> +	 */
> +	if (dig_port->tc_type == TC_PORT_TYPEC &&
> +	    !(I915_READ(PORT_TX_DFLEXDPSP) & TC_LIVE_STATE_TC(tc_port))) {
> +		DRM_DEBUG_KMS("TC PHY %d sudden disconnect.\n", tc_port);
> +		icl_tc_phy_disconnect(dig_port);
> +		return false;
> +	}
> +
> +	return true;
> +}
> +
> +/*
> + * See the comment at the connect function. This implements the Disconnect
> + * Flow.
> + */
> +void icl_tc_phy_disconnect(struct intel_digital_port *dig_port)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> +	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> +
> +	if (dig_port->tc_type == TC_PORT_UNKNOWN)
> +		return;
> +
> +	/*
> +	 * TBT disconnection flow is read the live status, what was done in
> +	 * caller.
> +	 */
> +	if (dig_port->tc_type == TC_PORT_TYPEC ||
> +	    dig_port->tc_type == TC_PORT_LEGACY) {
> +		u32 val;
> +
> +		val = I915_READ(PORT_TX_DFLEXDPCSSS);
> +		val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> +		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> +	}
> +
> +	DRM_DEBUG_KMS("Port %c TC type %s disconnected\n",
> +		      port_name(dig_port->base.port),
> +		      tc_type_name(dig_port->tc_type));
> +
> +	dig_port->tc_type = TC_PORT_UNKNOWN;
> +}
> +
> +static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
> +				    struct intel_digital_port *intel_dig_port,
> +				    bool is_legacy, bool is_typec, bool is_tbt)
> +{
> +	enum port port = intel_dig_port->base.port;
> +	enum tc_port_type old_type = intel_dig_port->tc_type;
> +
> +	WARN_ON(is_legacy + is_typec + is_tbt != 1);
> +
> +	if (is_legacy)
> +		intel_dig_port->tc_type = TC_PORT_LEGACY;
> +	else if (is_typec)
> +		intel_dig_port->tc_type = TC_PORT_TYPEC;
> +	else if (is_tbt)
> +		intel_dig_port->tc_type = TC_PORT_TBT;
> +	else
> +		return;
> +
> +	/* Types are not supposed to be changed at runtime. */
> +	WARN_ON(old_type != TC_PORT_UNKNOWN &&
> +		old_type != intel_dig_port->tc_type);
> +
> +	if (old_type != intel_dig_port->tc_type)
> +		DRM_DEBUG_KMS("Port %c has TC type %s\n", port_name(port),
> +			      tc_type_name(intel_dig_port->tc_type));
> +}
> +
> +
> +/*
> + * The type-C ports are different because even when they are connected, they may
> + * not be available/usable by the graphics driver: see the comment on
> + * icl_tc_phy_connect(). So in our driver instead of adding the additional
> + * concept of "usable" and make everything check for "connected and usable" we
> + * define a port as "connected" when it is not only connected, but also when it
> + * is usable by the rest of the driver. That maintains the old assumption that
> + * connected ports are usable, and avoids exposing to the users objects they
> + * can't really use.
> + */
> +bool intel_tc_port_connected(struct intel_digital_port *dig_port)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> +	enum port port = dig_port->base.port;
> +	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
> +	bool is_legacy, is_typec, is_tbt;
> +	u32 dpsp;
> +
> +	/*
> +	 * Complain if we got a legacy port HPD, but VBT didn't mark the port as
> +	 * legacy. Treat the port as legacy from now on.
> +	 */
> +	if (!dig_port->tc_legacy_port &&
> +	    I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) {
> +		DRM_ERROR("VBT incorrectly claims port %c is not TypeC legacy\n",
> +			  port_name(port));
> +		dig_port->tc_legacy_port = true;
> +	}
> +	is_legacy = dig_port->tc_legacy_port;
> +
> +	/*
> +	 * The spec says we shouldn't be using the ISR bits for detecting
> +	 * between TC and TBT. We should use DFLEXDPSP.
> +	 */
> +	dpsp = I915_READ(PORT_TX_DFLEXDPSP);
> +	is_typec = dpsp & TC_LIVE_STATE_TC(tc_port);
> +	is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port);
> +
> +	if (!is_legacy && !is_typec && !is_tbt) {
> +		icl_tc_phy_disconnect(dig_port);
> +
> +		return false;
> +	}
> +
> +	icl_update_tc_port_type(dev_priv, dig_port, is_legacy, is_typec,
> +				is_tbt);
> +
> +	if (!icl_tc_phy_connect(dig_port))
> +		return false;
> +
> +	return true;
> +}
> +
> diff --git a/drivers/gpu/drm/i915/intel_tc.h b/drivers/gpu/drm/i915/intel_tc.h
> new file mode 100644
> index 000000000000..94c62ac4a162
> --- /dev/null
> +++ b/drivers/gpu/drm/i915/intel_tc.h
> @@ -0,0 +1,13 @@

SPDX header here please.

> +#ifndef __INTEL_TC_H__
> +#define __INTEL_TC_H__
> +
> +#include <linux/types.h>
> +
> +struct intel_digital_port;
> +
> +void icl_tc_phy_disconnect(struct intel_digital_port *dig_port);

I'd like to see this named intel_tc_* in the future.

BR,
Jani.


> +
> +bool intel_tc_port_connected(struct intel_digital_port *dig_port);
> +int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port);
> +
> +#endif /* __INTEL_TC_H__ */
On Thu, 06 Jun 2019, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Tue, 04 Jun 2019, Imre Deak <imre.deak@intel.com> wrote:
>> Move the TypeC port handling functions to a new file for clarity.
>>
>> While at it:
>> - s/icl_tc_port_connected()/intel_tc_port_connected()/
>>   icl_tc_phy_disconnect(), will be unexported later.
>>
>> - s/intel_dp_get_fia_supported_lane_count()/intel_tc_port_fia_max_lane_count()/
>>   It's used for HDMI legacy mode too.
>>
>> - Simplify function interfaces by passing only dig_port to them.
>>
>> No functional changes.
>
> Some nitpicks below.

Forgot,

Acked-by: Jani Nikula <jani.nikula@intel.com>

on the idea.

>
> BR,
> Jani.
>
>
>>
>> Cc: Animesh Manna <animesh.manna@intel.com>
>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> Cc: José Roberto de Souza <jose.souza@intel.com>
>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>> ---
>>  drivers/gpu/drm/i915/Makefile             |   3 +-
>>  drivers/gpu/drm/i915/Makefile.header-test |   1 +
>>  drivers/gpu/drm/i915/intel_ddi.c          |   6 +-
>>  drivers/gpu/drm/i915/intel_dp.c           | 227 +--------------------
>>  drivers/gpu/drm/i915/intel_dp.h           |   2 -
>>  drivers/gpu/drm/i915/intel_tc.c           | 230 ++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_tc.h           |  13 ++
>>  7 files changed, 252 insertions(+), 230 deletions(-)
>>  create mode 100644 drivers/gpu/drm/i915/intel_tc.c
>>  create mode 100644 drivers/gpu/drm/i915/intel_tc.h
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index c0a7b2994077..74c4d11d83eb 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -171,7 +171,8 @@ i915-y += intel_audio.o \
>>  	  intel_psr.o \
>>  	  intel_quirks.o \
>>  	  intel_sideband.o \
>> -	  intel_sprite.o
>> +	  intel_sprite.o \
>> +	  intel_tc.o
>>  i915-$(CONFIG_ACPI)		+= intel_acpi.o intel_opregion.o
>>  i915-$(CONFIG_DRM_FBDEV_EMULATION)	+= intel_fbdev.o
>>  
>> diff --git a/drivers/gpu/drm/i915/Makefile.header-test b/drivers/gpu/drm/i915/Makefile.header-test
>> index 6ef3b647ac65..e80e8e45b09c 100644
>> --- a/drivers/gpu/drm/i915/Makefile.header-test
>> +++ b/drivers/gpu/drm/i915/Makefile.header-test
>> @@ -58,6 +58,7 @@ header_test := \
>>  	intel_sdvo.h \
>>  	intel_sideband.h \
>>  	intel_sprite.h \
>> +	intel_tc.h \
>>  	intel_tv.h \
>>  	intel_uncore.h \
>>  	intel_vdsc.h \
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 350eaf54f01f..5a1c98438375 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -45,6 +45,7 @@
>>  #include "intel_lspcon.h"
>>  #include "intel_panel.h"
>>  #include "intel_psr.h"
>> +#include "intel_tc.h"
>>  #include "intel_vdsc.h"
>>  
>>  struct ddi_buf_trans {
>> @@ -3904,7 +3905,6 @@ static int intel_ddi_compute_config(struct intel_encoder *encoder,
>>  static void intel_ddi_encoder_suspend(struct intel_encoder *encoder)
>>  {
>>  	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
>> -	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>>  
>>  	intel_dp_encoder_suspend(encoder);
>>  
>> @@ -3914,7 +3914,7 @@ static void intel_ddi_encoder_suspend(struct intel_encoder *encoder)
>>  	 * even if the sink has disappeared while being suspended.
>>  	 */
>>  	if (dig_port->tc_legacy_port)
>> -		icl_tc_phy_disconnect(i915, dig_port);
>> +		icl_tc_phy_disconnect(dig_port);
>>  }
>>  
>>  static void intel_ddi_encoder_reset(struct drm_encoder *drm_encoder)
>> @@ -3936,7 +3936,7 @@ static void intel_ddi_encoder_destroy(struct drm_encoder *encoder)
>>  	intel_dp_encoder_flush_work(encoder);
>>  
>>  	if (intel_port_is_tc(i915, dig_port->base.port))
>> -		icl_tc_phy_disconnect(i915, dig_port);
>> +		icl_tc_phy_disconnect(dig_port);
>>  
>>  	drm_encoder_cleanup(encoder);
>>  	kfree(dig_port);
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 24b56b2a76c8..b69310bd9914 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -62,6 +62,7 @@
>>  #include "intel_panel.h"
>>  #include "intel_psr.h"
>>  #include "intel_sideband.h"
>> +#include "intel_tc.h"
>>  #include "intel_vdsc.h"
>>  
>>  #define DP_DPRX_ESI_LEN 14
>> @@ -211,46 +212,13 @@ static int intel_dp_max_common_rate(struct intel_dp *intel_dp)
>>  	return intel_dp->common_rates[intel_dp->num_common_rates - 1];
>>  }
>>  
>> -static int intel_dp_get_fia_supported_lane_count(struct intel_dp *intel_dp)
>> -{
>> -	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> -	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
>> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
>> -	intel_wakeref_t wakeref;
>> -	u32 lane_info;
>> -
>> -	if (tc_port == PORT_TC_NONE || dig_port->tc_type != TC_PORT_TYPEC)
>> -		return 4;
>> -
>> -	lane_info = 0;
>> -	with_intel_display_power(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref)
>> -		lane_info = (I915_READ(PORT_TX_DFLEXDPSP) &
>> -			     DP_LANE_ASSIGNMENT_MASK(tc_port)) >>
>> -				DP_LANE_ASSIGNMENT_SHIFT(tc_port);
>> -
>> -	switch (lane_info) {
>> -	default:
>> -		MISSING_CASE(lane_info);
>> -	case 1:
>> -	case 2:
>> -	case 4:
>> -	case 8:
>> -		return 1;
>> -	case 3:
>> -	case 12:
>> -		return 2;
>> -	case 15:
>> -		return 4;
>> -	}
>> -}
>> -
>>  /* Theoretical max between source and sink */
>>  static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp)
>>  {
>>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>>  	int source_max = intel_dig_port->max_lanes;
>>  	int sink_max = drm_dp_max_lane_count(intel_dp->dpcd);
>> -	int fia_max = intel_dp_get_fia_supported_lane_count(intel_dp);
>> +	int fia_max = intel_tc_port_fia_max_lane_count(intel_dig_port);
>>  
>>  	return min3(source_max, sink_max, fia_max);
>>  }
>> @@ -5231,195 +5199,6 @@ static bool icl_combo_port_connected(struct drm_i915_private *dev_priv,
>>  	return I915_READ(SDEISR) & SDE_DDI_HOTPLUG_ICP(port);
>>  }
>>  
>> -static const char *tc_type_name(enum tc_port_type type)
>> -{
>> -	static const char * const names[] = {
>> -		[TC_PORT_UNKNOWN] = "unknown",
>> -		[TC_PORT_LEGACY] = "legacy",
>> -		[TC_PORT_TYPEC] = "typec",
>> -		[TC_PORT_TBT] = "tbt",
>> -	};
>> -
>> -	if (WARN_ON(type >= ARRAY_SIZE(names)))
>> -		type = TC_PORT_UNKNOWN;
>> -
>> -	return names[type];
>> -}
>> -
>> -static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
>> -				    struct intel_digital_port *intel_dig_port,
>> -				    bool is_legacy, bool is_typec, bool is_tbt)
>> -{
>> -	enum port port = intel_dig_port->base.port;
>> -	enum tc_port_type old_type = intel_dig_port->tc_type;
>> -
>> -	WARN_ON(is_legacy + is_typec + is_tbt != 1);
>> -
>> -	if (is_legacy)
>> -		intel_dig_port->tc_type = TC_PORT_LEGACY;
>> -	else if (is_typec)
>> -		intel_dig_port->tc_type = TC_PORT_TYPEC;
>> -	else if (is_tbt)
>> -		intel_dig_port->tc_type = TC_PORT_TBT;
>> -	else
>> -		return;
>> -
>> -	/* Types are not supposed to be changed at runtime. */
>> -	WARN_ON(old_type != TC_PORT_UNKNOWN &&
>> -		old_type != intel_dig_port->tc_type);
>> -
>> -	if (old_type != intel_dig_port->tc_type)
>> -		DRM_DEBUG_KMS("Port %c has TC type %s\n", port_name(port),
>> -			      tc_type_name(intel_dig_port->tc_type));
>> -}
>> -
>> -/*
>> - * This function implements the first part of the Connect Flow described by our
>> - * specification, Gen11 TypeC Programming chapter. The rest of the flow (reading
>> - * lanes, EDID, etc) is done as needed in the typical places.
>> - *
>> - * Unlike the other ports, type-C ports are not available to use as soon as we
>> - * get a hotplug. The type-C PHYs can be shared between multiple controllers:
>> - * display, USB, etc. As a result, handshaking through FIA is required around
>> - * connect and disconnect to cleanly transfer ownership with the controller and
>> - * set the type-C power state.
>> - *
>> - * We could opt to only do the connect flow when we actually try to use the AUX
>> - * channels or do a modeset, then immediately run the disconnect flow after
>> - * usage, but there are some implications on this for a dynamic environment:
>> - * things may go away or change behind our backs. So for now our driver is
>> - * always trying to acquire ownership of the controller as soon as it gets an
>> - * interrupt (or polls state and sees a port is connected) and only gives it
>> - * back when it sees a disconnect. Implementation of a more fine-grained model
>> - * will require a lot of coordination with user space and thorough testing for
>> - * the extra possible cases.
>> - */
>> -static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
>> -			       struct intel_digital_port *dig_port)
>> -{
>> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
>> -	u32 val;
>> -
>> -	if (dig_port->tc_type != TC_PORT_LEGACY &&
>> -	    dig_port->tc_type != TC_PORT_TYPEC)
>> -		return true;
>> -
>> -	val = I915_READ(PORT_TX_DFLEXDPPMS);
>> -	if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) {
>> -		DRM_DEBUG_KMS("DP PHY for TC port %d not ready\n", tc_port);
>> -		WARN_ON(dig_port->tc_legacy_port);
>> -		return false;
>> -	}
>> -
>> -	/*
>> -	 * This function may be called many times in a row without an HPD event
>> -	 * in between, so try to avoid the write when we can.
>> -	 */
>> -	val = I915_READ(PORT_TX_DFLEXDPCSSS);
>> -	if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) {
>> -		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
>> -		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
>> -	}
>> -
>> -	/*
>> -	 * Now we have to re-check the live state, in case the port recently
>> -	 * became disconnected. Not necessary for legacy mode.
>> -	 */
>> -	if (dig_port->tc_type == TC_PORT_TYPEC &&
>> -	    !(I915_READ(PORT_TX_DFLEXDPSP) & TC_LIVE_STATE_TC(tc_port))) {
>> -		DRM_DEBUG_KMS("TC PHY %d sudden disconnect.\n", tc_port);
>> -		icl_tc_phy_disconnect(dev_priv, dig_port);
>> -		return false;
>> -	}
>> -
>> -	return true;
>> -}
>> -
>> -/*
>> - * See the comment at the connect function. This implements the Disconnect
>> - * Flow.
>> - */
>> -void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
>> -			   struct intel_digital_port *dig_port)
>> -{
>> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
>> -
>> -	if (dig_port->tc_type == TC_PORT_UNKNOWN)
>> -		return;
>> -
>> -	/*
>> -	 * TBT disconnection flow is read the live status, what was done in
>> -	 * caller.
>> -	 */
>> -	if (dig_port->tc_type == TC_PORT_TYPEC ||
>> -	    dig_port->tc_type == TC_PORT_LEGACY) {
>> -		u32 val;
>> -
>> -		val = I915_READ(PORT_TX_DFLEXDPCSSS);
>> -		val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
>> -		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
>> -	}
>> -
>> -	DRM_DEBUG_KMS("Port %c TC type %s disconnected\n",
>> -		      port_name(dig_port->base.port),
>> -		      tc_type_name(dig_port->tc_type));
>> -
>> -	dig_port->tc_type = TC_PORT_UNKNOWN;
>> -}
>> -
>> -/*
>> - * The type-C ports are different because even when they are connected, they may
>> - * not be available/usable by the graphics driver: see the comment on
>> - * icl_tc_phy_connect(). So in our driver instead of adding the additional
>> - * concept of "usable" and make everything check for "connected and usable" we
>> - * define a port as "connected" when it is not only connected, but also when it
>> - * is usable by the rest of the driver. That maintains the old assumption that
>> - * connected ports are usable, and avoids exposing to the users objects they
>> - * can't really use.
>> - */
>> -static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
>> -				  struct intel_digital_port *intel_dig_port)
>> -{
>> -	enum port port = intel_dig_port->base.port;
>> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
>> -	bool is_legacy, is_typec, is_tbt;
>> -	u32 dpsp;
>> -
>> -	/*
>> -	 * Complain if we got a legacy port HPD, but VBT didn't mark the port as
>> -	 * legacy. Treat the port as legacy from now on.
>> -	 */
>> -	if (!intel_dig_port->tc_legacy_port &&
>> -	    I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) {
>> -		DRM_ERROR("VBT incorrectly claims port %c is not TypeC legacy\n",
>> -			  port_name(port));
>> -		intel_dig_port->tc_legacy_port = true;
>> -	}
>> -	is_legacy = intel_dig_port->tc_legacy_port;
>> -
>> -	/*
>> -	 * The spec says we shouldn't be using the ISR bits for detecting
>> -	 * between TC and TBT. We should use DFLEXDPSP.
>> -	 */
>> -	dpsp = I915_READ(PORT_TX_DFLEXDPSP);
>> -	is_typec = dpsp & TC_LIVE_STATE_TC(tc_port);
>> -	is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port);
>> -
>> -	if (!is_legacy && !is_typec && !is_tbt) {
>> -		icl_tc_phy_disconnect(dev_priv, intel_dig_port);
>> -
>> -		return false;
>> -	}
>> -
>> -	icl_update_tc_port_type(dev_priv, intel_dig_port, is_legacy, is_typec,
>> -				is_tbt);
>> -
>> -	if (!icl_tc_phy_connect(dev_priv, intel_dig_port))
>> -		return false;
>> -
>> -	return true;
>> -}
>> -
>>  static bool icl_digital_port_connected(struct intel_encoder *encoder)
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> @@ -5428,7 +5207,7 @@ static bool icl_digital_port_connected(struct intel_encoder *encoder)
>>  	if (intel_port_is_combophy(dev_priv, encoder->port))
>>  		return icl_combo_port_connected(dev_priv, dig_port);
>>  	else if (intel_port_is_tc(dev_priv, encoder->port))
>> -		return icl_tc_port_connected(dev_priv, dig_port);
>> +		return intel_tc_port_connected(dig_port);
>>  	else
>>  		MISSING_CASE(encoder->hpd_pin);
>>  
>> diff --git a/drivers/gpu/drm/i915/intel_dp.h b/drivers/gpu/drm/i915/intel_dp.h
>> index da70b1a41c83..657bbb1f5ed0 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.h
>> +++ b/drivers/gpu/drm/i915/intel_dp.h
>> @@ -112,8 +112,6 @@ bool intel_dp_get_colorimetry_status(struct intel_dp *intel_dp);
>>  int intel_dp_link_required(int pixel_clock, int bpp);
>>  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
>>  bool intel_digital_port_connected(struct intel_encoder *encoder);
>> -void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
>> -			   struct intel_digital_port *dig_port);
>>  
>>  static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
>>  {
>> diff --git a/drivers/gpu/drm/i915/intel_tc.c b/drivers/gpu/drm/i915/intel_tc.c
>> new file mode 100644
>> index 000000000000..7a1b5870945f
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_tc.c
>> @@ -0,0 +1,230 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2019 Intel Corporation
>> + */
>
> From the Nitpicks Department, please add a blank line here.
>
>> +#include "intel_display.h"
>> +#include "i915_drv.h"
>> +#include "intel_tc.h"
>
> And sort the includes.
>
>> +
>> +static const char *tc_type_name(enum tc_port_type type)
>> +{
>> +	static const char * const names[] = {
>> +		[TC_PORT_UNKNOWN] = "unknown",
>> +		[TC_PORT_LEGACY] = "legacy",
>> +		[TC_PORT_TYPEC] = "typec",
>> +		[TC_PORT_TBT] = "tbt",
>> +	};
>> +
>> +	if (WARN_ON(type >= ARRAY_SIZE(names)))
>> +		type = TC_PORT_UNKNOWN;
>> +
>> +	return names[type];
>> +}
>> +
>> +int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port)
>> +{
>> +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
>> +	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
>> +	intel_wakeref_t wakeref;
>> +	u32 lane_info;
>> +
>> +	if (tc_port == PORT_TC_NONE || dig_port->tc_type != TC_PORT_TYPEC)
>> +		return 4;
>> +
>> +	lane_info = 0;
>> +	with_intel_display_power(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref)
>> +		lane_info = (I915_READ(PORT_TX_DFLEXDPSP) &
>> +			     DP_LANE_ASSIGNMENT_MASK(tc_port)) >>
>> +				DP_LANE_ASSIGNMENT_SHIFT(tc_port);
>> +
>> +	switch (lane_info) {
>> +	default:
>> +		MISSING_CASE(lane_info);
>> +	case 1:
>> +	case 2:
>> +	case 4:
>> +	case 8:
>> +		return 1;
>> +	case 3:
>> +	case 12:
>> +		return 2;
>> +	case 15:
>> +		return 4;
>> +	}
>> +}
>> +
>> +/*
>> + * This function implements the first part of the Connect Flow described by our
>> + * specification, Gen11 TypeC Programming chapter. The rest of the flow (reading
>> + * lanes, EDID, etc) is done as needed in the typical places.
>> + *
>> + * Unlike the other ports, type-C ports are not available to use as soon as we
>> + * get a hotplug. The type-C PHYs can be shared between multiple controllers:
>> + * display, USB, etc. As a result, handshaking through FIA is required around
>> + * connect and disconnect to cleanly transfer ownership with the controller and
>> + * set the type-C power state.
>> + *
>> + * We could opt to only do the connect flow when we actually try to use the AUX
>> + * channels or do a modeset, then immediately run the disconnect flow after
>> + * usage, but there are some implications on this for a dynamic environment:
>> + * things may go away or change behind our backs. So for now our driver is
>> + * always trying to acquire ownership of the controller as soon as it gets an
>> + * interrupt (or polls state and sees a port is connected) and only gives it
>> + * back when it sees a disconnect. Implementation of a more fine-grained model
>> + * will require a lot of coordination with user space and thorough testing for
>> + * the extra possible cases.
>> + */
>> +static bool icl_tc_phy_connect(struct intel_digital_port *dig_port)
>> +{
>> +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
>> +	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
>> +	u32 val;
>> +
>> +	if (dig_port->tc_type != TC_PORT_LEGACY &&
>> +	    dig_port->tc_type != TC_PORT_TYPEC)
>> +		return true;
>> +
>> +	val = I915_READ(PORT_TX_DFLEXDPPMS);
>> +	if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) {
>> +		DRM_DEBUG_KMS("DP PHY for TC port %d not ready\n", tc_port);
>> +		WARN_ON(dig_port->tc_legacy_port);
>> +		return false;
>> +	}
>> +
>> +	/*
>> +	 * This function may be called many times in a row without an HPD event
>> +	 * in between, so try to avoid the write when we can.
>> +	 */
>> +	val = I915_READ(PORT_TX_DFLEXDPCSSS);
>> +	if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) {
>> +		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
>> +		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
>> +	}
>> +
>> +	/*
>> +	 * Now we have to re-check the live state, in case the port recently
>> +	 * became disconnected. Not necessary for legacy mode.
>> +	 */
>> +	if (dig_port->tc_type == TC_PORT_TYPEC &&
>> +	    !(I915_READ(PORT_TX_DFLEXDPSP) & TC_LIVE_STATE_TC(tc_port))) {
>> +		DRM_DEBUG_KMS("TC PHY %d sudden disconnect.\n", tc_port);
>> +		icl_tc_phy_disconnect(dig_port);
>> +		return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +/*
>> + * See the comment at the connect function. This implements the Disconnect
>> + * Flow.
>> + */
>> +void icl_tc_phy_disconnect(struct intel_digital_port *dig_port)
>> +{
>> +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
>> +	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
>> +
>> +	if (dig_port->tc_type == TC_PORT_UNKNOWN)
>> +		return;
>> +
>> +	/*
>> +	 * TBT disconnection flow is read the live status, what was done in
>> +	 * caller.
>> +	 */
>> +	if (dig_port->tc_type == TC_PORT_TYPEC ||
>> +	    dig_port->tc_type == TC_PORT_LEGACY) {
>> +		u32 val;
>> +
>> +		val = I915_READ(PORT_TX_DFLEXDPCSSS);
>> +		val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
>> +		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
>> +	}
>> +
>> +	DRM_DEBUG_KMS("Port %c TC type %s disconnected\n",
>> +		      port_name(dig_port->base.port),
>> +		      tc_type_name(dig_port->tc_type));
>> +
>> +	dig_port->tc_type = TC_PORT_UNKNOWN;
>> +}
>> +
>> +static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
>> +				    struct intel_digital_port *intel_dig_port,
>> +				    bool is_legacy, bool is_typec, bool is_tbt)
>> +{
>> +	enum port port = intel_dig_port->base.port;
>> +	enum tc_port_type old_type = intel_dig_port->tc_type;
>> +
>> +	WARN_ON(is_legacy + is_typec + is_tbt != 1);
>> +
>> +	if (is_legacy)
>> +		intel_dig_port->tc_type = TC_PORT_LEGACY;
>> +	else if (is_typec)
>> +		intel_dig_port->tc_type = TC_PORT_TYPEC;
>> +	else if (is_tbt)
>> +		intel_dig_port->tc_type = TC_PORT_TBT;
>> +	else
>> +		return;
>> +
>> +	/* Types are not supposed to be changed at runtime. */
>> +	WARN_ON(old_type != TC_PORT_UNKNOWN &&
>> +		old_type != intel_dig_port->tc_type);
>> +
>> +	if (old_type != intel_dig_port->tc_type)
>> +		DRM_DEBUG_KMS("Port %c has TC type %s\n", port_name(port),
>> +			      tc_type_name(intel_dig_port->tc_type));
>> +}
>> +
>> +
>> +/*
>> + * The type-C ports are different because even when they are connected, they may
>> + * not be available/usable by the graphics driver: see the comment on
>> + * icl_tc_phy_connect(). So in our driver instead of adding the additional
>> + * concept of "usable" and make everything check for "connected and usable" we
>> + * define a port as "connected" when it is not only connected, but also when it
>> + * is usable by the rest of the driver. That maintains the old assumption that
>> + * connected ports are usable, and avoids exposing to the users objects they
>> + * can't really use.
>> + */
>> +bool intel_tc_port_connected(struct intel_digital_port *dig_port)
>> +{
>> +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
>> +	enum port port = dig_port->base.port;
>> +	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
>> +	bool is_legacy, is_typec, is_tbt;
>> +	u32 dpsp;
>> +
>> +	/*
>> +	 * Complain if we got a legacy port HPD, but VBT didn't mark the port as
>> +	 * legacy. Treat the port as legacy from now on.
>> +	 */
>> +	if (!dig_port->tc_legacy_port &&
>> +	    I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) {
>> +		DRM_ERROR("VBT incorrectly claims port %c is not TypeC legacy\n",
>> +			  port_name(port));
>> +		dig_port->tc_legacy_port = true;
>> +	}
>> +	is_legacy = dig_port->tc_legacy_port;
>> +
>> +	/*
>> +	 * The spec says we shouldn't be using the ISR bits for detecting
>> +	 * between TC and TBT. We should use DFLEXDPSP.
>> +	 */
>> +	dpsp = I915_READ(PORT_TX_DFLEXDPSP);
>> +	is_typec = dpsp & TC_LIVE_STATE_TC(tc_port);
>> +	is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port);
>> +
>> +	if (!is_legacy && !is_typec && !is_tbt) {
>> +		icl_tc_phy_disconnect(dig_port);
>> +
>> +		return false;
>> +	}
>> +
>> +	icl_update_tc_port_type(dev_priv, dig_port, is_legacy, is_typec,
>> +				is_tbt);
>> +
>> +	if (!icl_tc_phy_connect(dig_port))
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>> diff --git a/drivers/gpu/drm/i915/intel_tc.h b/drivers/gpu/drm/i915/intel_tc.h
>> new file mode 100644
>> index 000000000000..94c62ac4a162
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_tc.h
>> @@ -0,0 +1,13 @@
>
> SPDX header here please.
>
>> +#ifndef __INTEL_TC_H__
>> +#define __INTEL_TC_H__
>> +
>> +#include <linux/types.h>
>> +
>> +struct intel_digital_port;
>> +
>> +void icl_tc_phy_disconnect(struct intel_digital_port *dig_port);
>
> I'd like to see this named intel_tc_* in the future.
>
> BR,
> Jani.
>
>
>> +
>> +bool intel_tc_port_connected(struct intel_digital_port *dig_port);
>> +int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port);
>> +
>> +#endif /* __INTEL_TC_H__ */
On Thu, Jun 06, 2019 at 11:42:46AM +0300, Jani Nikula wrote:
> On Tue, 04 Jun 2019, Imre Deak <imre.deak@intel.com> wrote:
> > Move the TypeC port handling functions to a new file for clarity.
> >
> > While at it:
> > - s/icl_tc_port_connected()/intel_tc_port_connected()/
> >   icl_tc_phy_disconnect(), will be unexported later.
> >
> > - s/intel_dp_get_fia_supported_lane_count()/intel_tc_port_fia_max_lane_count()/
> >   It's used for HDMI legacy mode too.
> >
> > - Simplify function interfaces by passing only dig_port to them.
> >
> > No functional changes.
> 
> Some nitpicks below.
> 
> BR,
> Jani.
> 
> 
> >
> > Cc: Animesh Manna <animesh.manna@intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: José Roberto de Souza <jose.souza@intel.com>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/Makefile             |   3 +-
> >  drivers/gpu/drm/i915/Makefile.header-test |   1 +
> >  drivers/gpu/drm/i915/intel_ddi.c          |   6 +-
> >  drivers/gpu/drm/i915/intel_dp.c           | 227 +--------------------
> >  drivers/gpu/drm/i915/intel_dp.h           |   2 -
> >  drivers/gpu/drm/i915/intel_tc.c           | 230 ++++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_tc.h           |  13 ++
> >  7 files changed, 252 insertions(+), 230 deletions(-)
> >  create mode 100644 drivers/gpu/drm/i915/intel_tc.c
> >  create mode 100644 drivers/gpu/drm/i915/intel_tc.h
> >
> > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> > index c0a7b2994077..74c4d11d83eb 100644
> > --- a/drivers/gpu/drm/i915/Makefile
> > +++ b/drivers/gpu/drm/i915/Makefile
> > @@ -171,7 +171,8 @@ i915-y += intel_audio.o \
> >  	  intel_psr.o \
> >  	  intel_quirks.o \
> >  	  intel_sideband.o \
> > -	  intel_sprite.o
> > +	  intel_sprite.o \
> > +	  intel_tc.o
> >  i915-$(CONFIG_ACPI)		+= intel_acpi.o intel_opregion.o
> >  i915-$(CONFIG_DRM_FBDEV_EMULATION)	+= intel_fbdev.o
> >  
> > diff --git a/drivers/gpu/drm/i915/Makefile.header-test b/drivers/gpu/drm/i915/Makefile.header-test
> > index 6ef3b647ac65..e80e8e45b09c 100644
> > --- a/drivers/gpu/drm/i915/Makefile.header-test
> > +++ b/drivers/gpu/drm/i915/Makefile.header-test
> > @@ -58,6 +58,7 @@ header_test := \
> >  	intel_sdvo.h \
> >  	intel_sideband.h \
> >  	intel_sprite.h \
> > +	intel_tc.h \
> >  	intel_tv.h \
> >  	intel_uncore.h \
> >  	intel_vdsc.h \
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> > index 350eaf54f01f..5a1c98438375 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -45,6 +45,7 @@
> >  #include "intel_lspcon.h"
> >  #include "intel_panel.h"
> >  #include "intel_psr.h"
> > +#include "intel_tc.h"
> >  #include "intel_vdsc.h"
> >  
> >  struct ddi_buf_trans {
> > @@ -3904,7 +3905,6 @@ static int intel_ddi_compute_config(struct intel_encoder *encoder,
> >  static void intel_ddi_encoder_suspend(struct intel_encoder *encoder)
> >  {
> >  	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
> > -	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
> >  
> >  	intel_dp_encoder_suspend(encoder);
> >  
> > @@ -3914,7 +3914,7 @@ static void intel_ddi_encoder_suspend(struct intel_encoder *encoder)
> >  	 * even if the sink has disappeared while being suspended.
> >  	 */
> >  	if (dig_port->tc_legacy_port)
> > -		icl_tc_phy_disconnect(i915, dig_port);
> > +		icl_tc_phy_disconnect(dig_port);
> >  }
> >  
> >  static void intel_ddi_encoder_reset(struct drm_encoder *drm_encoder)
> > @@ -3936,7 +3936,7 @@ static void intel_ddi_encoder_destroy(struct drm_encoder *encoder)
> >  	intel_dp_encoder_flush_work(encoder);
> >  
> >  	if (intel_port_is_tc(i915, dig_port->base.port))
> > -		icl_tc_phy_disconnect(i915, dig_port);
> > +		icl_tc_phy_disconnect(dig_port);
> >  
> >  	drm_encoder_cleanup(encoder);
> >  	kfree(dig_port);
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 24b56b2a76c8..b69310bd9914 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -62,6 +62,7 @@
> >  #include "intel_panel.h"
> >  #include "intel_psr.h"
> >  #include "intel_sideband.h"
> > +#include "intel_tc.h"
> >  #include "intel_vdsc.h"
> >  
> >  #define DP_DPRX_ESI_LEN 14
> > @@ -211,46 +212,13 @@ static int intel_dp_max_common_rate(struct intel_dp *intel_dp)
> >  	return intel_dp->common_rates[intel_dp->num_common_rates - 1];
> >  }
> >  
> > -static int intel_dp_get_fia_supported_lane_count(struct intel_dp *intel_dp)
> > -{
> > -	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > -	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> > -	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> > -	intel_wakeref_t wakeref;
> > -	u32 lane_info;
> > -
> > -	if (tc_port == PORT_TC_NONE || dig_port->tc_type != TC_PORT_TYPEC)
> > -		return 4;
> > -
> > -	lane_info = 0;
> > -	with_intel_display_power(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref)
> > -		lane_info = (I915_READ(PORT_TX_DFLEXDPSP) &
> > -			     DP_LANE_ASSIGNMENT_MASK(tc_port)) >>
> > -				DP_LANE_ASSIGNMENT_SHIFT(tc_port);
> > -
> > -	switch (lane_info) {
> > -	default:
> > -		MISSING_CASE(lane_info);
> > -	case 1:
> > -	case 2:
> > -	case 4:
> > -	case 8:
> > -		return 1;
> > -	case 3:
> > -	case 12:
> > -		return 2;
> > -	case 15:
> > -		return 4;
> > -	}
> > -}
> > -
> >  /* Theoretical max between source and sink */
> >  static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp)
> >  {
> >  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> >  	int source_max = intel_dig_port->max_lanes;
> >  	int sink_max = drm_dp_max_lane_count(intel_dp->dpcd);
> > -	int fia_max = intel_dp_get_fia_supported_lane_count(intel_dp);
> > +	int fia_max = intel_tc_port_fia_max_lane_count(intel_dig_port);
> >  
> >  	return min3(source_max, sink_max, fia_max);
> >  }
> > @@ -5231,195 +5199,6 @@ static bool icl_combo_port_connected(struct drm_i915_private *dev_priv,
> >  	return I915_READ(SDEISR) & SDE_DDI_HOTPLUG_ICP(port);
> >  }
> >  
> > -static const char *tc_type_name(enum tc_port_type type)
> > -{
> > -	static const char * const names[] = {
> > -		[TC_PORT_UNKNOWN] = "unknown",
> > -		[TC_PORT_LEGACY] = "legacy",
> > -		[TC_PORT_TYPEC] = "typec",
> > -		[TC_PORT_TBT] = "tbt",
> > -	};
> > -
> > -	if (WARN_ON(type >= ARRAY_SIZE(names)))
> > -		type = TC_PORT_UNKNOWN;
> > -
> > -	return names[type];
> > -}
> > -
> > -static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
> > -				    struct intel_digital_port *intel_dig_port,
> > -				    bool is_legacy, bool is_typec, bool is_tbt)
> > -{
> > -	enum port port = intel_dig_port->base.port;
> > -	enum tc_port_type old_type = intel_dig_port->tc_type;
> > -
> > -	WARN_ON(is_legacy + is_typec + is_tbt != 1);
> > -
> > -	if (is_legacy)
> > -		intel_dig_port->tc_type = TC_PORT_LEGACY;
> > -	else if (is_typec)
> > -		intel_dig_port->tc_type = TC_PORT_TYPEC;
> > -	else if (is_tbt)
> > -		intel_dig_port->tc_type = TC_PORT_TBT;
> > -	else
> > -		return;
> > -
> > -	/* Types are not supposed to be changed at runtime. */
> > -	WARN_ON(old_type != TC_PORT_UNKNOWN &&
> > -		old_type != intel_dig_port->tc_type);
> > -
> > -	if (old_type != intel_dig_port->tc_type)
> > -		DRM_DEBUG_KMS("Port %c has TC type %s\n", port_name(port),
> > -			      tc_type_name(intel_dig_port->tc_type));
> > -}
> > -
> > -/*
> > - * This function implements the first part of the Connect Flow described by our
> > - * specification, Gen11 TypeC Programming chapter. The rest of the flow (reading
> > - * lanes, EDID, etc) is done as needed in the typical places.
> > - *
> > - * Unlike the other ports, type-C ports are not available to use as soon as we
> > - * get a hotplug. The type-C PHYs can be shared between multiple controllers:
> > - * display, USB, etc. As a result, handshaking through FIA is required around
> > - * connect and disconnect to cleanly transfer ownership with the controller and
> > - * set the type-C power state.
> > - *
> > - * We could opt to only do the connect flow when we actually try to use the AUX
> > - * channels or do a modeset, then immediately run the disconnect flow after
> > - * usage, but there are some implications on this for a dynamic environment:
> > - * things may go away or change behind our backs. So for now our driver is
> > - * always trying to acquire ownership of the controller as soon as it gets an
> > - * interrupt (or polls state and sees a port is connected) and only gives it
> > - * back when it sees a disconnect. Implementation of a more fine-grained model
> > - * will require a lot of coordination with user space and thorough testing for
> > - * the extra possible cases.
> > - */
> > -static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
> > -			       struct intel_digital_port *dig_port)
> > -{
> > -	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> > -	u32 val;
> > -
> > -	if (dig_port->tc_type != TC_PORT_LEGACY &&
> > -	    dig_port->tc_type != TC_PORT_TYPEC)
> > -		return true;
> > -
> > -	val = I915_READ(PORT_TX_DFLEXDPPMS);
> > -	if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) {
> > -		DRM_DEBUG_KMS("DP PHY for TC port %d not ready\n", tc_port);
> > -		WARN_ON(dig_port->tc_legacy_port);
> > -		return false;
> > -	}
> > -
> > -	/*
> > -	 * This function may be called many times in a row without an HPD event
> > -	 * in between, so try to avoid the write when we can.
> > -	 */
> > -	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > -	if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) {
> > -		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> > -		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > -	}
> > -
> > -	/*
> > -	 * Now we have to re-check the live state, in case the port recently
> > -	 * became disconnected. Not necessary for legacy mode.
> > -	 */
> > -	if (dig_port->tc_type == TC_PORT_TYPEC &&
> > -	    !(I915_READ(PORT_TX_DFLEXDPSP) & TC_LIVE_STATE_TC(tc_port))) {
> > -		DRM_DEBUG_KMS("TC PHY %d sudden disconnect.\n", tc_port);
> > -		icl_tc_phy_disconnect(dev_priv, dig_port);
> > -		return false;
> > -	}
> > -
> > -	return true;
> > -}
> > -
> > -/*
> > - * See the comment at the connect function. This implements the Disconnect
> > - * Flow.
> > - */
> > -void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
> > -			   struct intel_digital_port *dig_port)
> > -{
> > -	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> > -
> > -	if (dig_port->tc_type == TC_PORT_UNKNOWN)
> > -		return;
> > -
> > -	/*
> > -	 * TBT disconnection flow is read the live status, what was done in
> > -	 * caller.
> > -	 */
> > -	if (dig_port->tc_type == TC_PORT_TYPEC ||
> > -	    dig_port->tc_type == TC_PORT_LEGACY) {
> > -		u32 val;
> > -
> > -		val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > -		val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> > -		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > -	}
> > -
> > -	DRM_DEBUG_KMS("Port %c TC type %s disconnected\n",
> > -		      port_name(dig_port->base.port),
> > -		      tc_type_name(dig_port->tc_type));
> > -
> > -	dig_port->tc_type = TC_PORT_UNKNOWN;
> > -}
> > -
> > -/*
> > - * The type-C ports are different because even when they are connected, they may
> > - * not be available/usable by the graphics driver: see the comment on
> > - * icl_tc_phy_connect(). So in our driver instead of adding the additional
> > - * concept of "usable" and make everything check for "connected and usable" we
> > - * define a port as "connected" when it is not only connected, but also when it
> > - * is usable by the rest of the driver. That maintains the old assumption that
> > - * connected ports are usable, and avoids exposing to the users objects they
> > - * can't really use.
> > - */
> > -static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
> > -				  struct intel_digital_port *intel_dig_port)
> > -{
> > -	enum port port = intel_dig_port->base.port;
> > -	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
> > -	bool is_legacy, is_typec, is_tbt;
> > -	u32 dpsp;
> > -
> > -	/*
> > -	 * Complain if we got a legacy port HPD, but VBT didn't mark the port as
> > -	 * legacy. Treat the port as legacy from now on.
> > -	 */
> > -	if (!intel_dig_port->tc_legacy_port &&
> > -	    I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) {
> > -		DRM_ERROR("VBT incorrectly claims port %c is not TypeC legacy\n",
> > -			  port_name(port));
> > -		intel_dig_port->tc_legacy_port = true;
> > -	}
> > -	is_legacy = intel_dig_port->tc_legacy_port;
> > -
> > -	/*
> > -	 * The spec says we shouldn't be using the ISR bits for detecting
> > -	 * between TC and TBT. We should use DFLEXDPSP.
> > -	 */
> > -	dpsp = I915_READ(PORT_TX_DFLEXDPSP);
> > -	is_typec = dpsp & TC_LIVE_STATE_TC(tc_port);
> > -	is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port);
> > -
> > -	if (!is_legacy && !is_typec && !is_tbt) {
> > -		icl_tc_phy_disconnect(dev_priv, intel_dig_port);
> > -
> > -		return false;
> > -	}
> > -
> > -	icl_update_tc_port_type(dev_priv, intel_dig_port, is_legacy, is_typec,
> > -				is_tbt);
> > -
> > -	if (!icl_tc_phy_connect(dev_priv, intel_dig_port))
> > -		return false;
> > -
> > -	return true;
> > -}
> > -
> >  static bool icl_digital_port_connected(struct intel_encoder *encoder)
> >  {
> >  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > @@ -5428,7 +5207,7 @@ static bool icl_digital_port_connected(struct intel_encoder *encoder)
> >  	if (intel_port_is_combophy(dev_priv, encoder->port))
> >  		return icl_combo_port_connected(dev_priv, dig_port);
> >  	else if (intel_port_is_tc(dev_priv, encoder->port))
> > -		return icl_tc_port_connected(dev_priv, dig_port);
> > +		return intel_tc_port_connected(dig_port);
> >  	else
> >  		MISSING_CASE(encoder->hpd_pin);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_dp.h b/drivers/gpu/drm/i915/intel_dp.h
> > index da70b1a41c83..657bbb1f5ed0 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.h
> > +++ b/drivers/gpu/drm/i915/intel_dp.h
> > @@ -112,8 +112,6 @@ bool intel_dp_get_colorimetry_status(struct intel_dp *intel_dp);
> >  int intel_dp_link_required(int pixel_clock, int bpp);
> >  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
> >  bool intel_digital_port_connected(struct intel_encoder *encoder);
> > -void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
> > -			   struct intel_digital_port *dig_port);
> >  
> >  static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
> >  {
> > diff --git a/drivers/gpu/drm/i915/intel_tc.c b/drivers/gpu/drm/i915/intel_tc.c
> > new file mode 100644
> > index 000000000000..7a1b5870945f
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/intel_tc.c
> > @@ -0,0 +1,230 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright © 2019 Intel Corporation
> > + */
> 
> From the Nitpicks Department, please add a blank line here.

Ok.

> 
> > +#include "intel_display.h"
> > +#include "i915_drv.h"
> > +#include "intel_tc.h"
> 
> And sort the includes.

Ok, forgot that here..

> 
> > +
> > +static const char *tc_type_name(enum tc_port_type type)
> > +{
> > +	static const char * const names[] = {
> > +		[TC_PORT_UNKNOWN] = "unknown",
> > +		[TC_PORT_LEGACY] = "legacy",
> > +		[TC_PORT_TYPEC] = "typec",
> > +		[TC_PORT_TBT] = "tbt",
> > +	};
> > +
> > +	if (WARN_ON(type >= ARRAY_SIZE(names)))
> > +		type = TC_PORT_UNKNOWN;
> > +
> > +	return names[type];
> > +}
> > +
> > +int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> > +	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> > +	intel_wakeref_t wakeref;
> > +	u32 lane_info;
> > +
> > +	if (tc_port == PORT_TC_NONE || dig_port->tc_type != TC_PORT_TYPEC)
> > +		return 4;
> > +
> > +	lane_info = 0;
> > +	with_intel_display_power(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref)
> > +		lane_info = (I915_READ(PORT_TX_DFLEXDPSP) &
> > +			     DP_LANE_ASSIGNMENT_MASK(tc_port)) >>
> > +				DP_LANE_ASSIGNMENT_SHIFT(tc_port);
> > +
> > +	switch (lane_info) {
> > +	default:
> > +		MISSING_CASE(lane_info);
> > +	case 1:
> > +	case 2:
> > +	case 4:
> > +	case 8:
> > +		return 1;
> > +	case 3:
> > +	case 12:
> > +		return 2;
> > +	case 15:
> > +		return 4;
> > +	}
> > +}
> > +
> > +/*
> > + * This function implements the first part of the Connect Flow described by our
> > + * specification, Gen11 TypeC Programming chapter. The rest of the flow (reading
> > + * lanes, EDID, etc) is done as needed in the typical places.
> > + *
> > + * Unlike the other ports, type-C ports are not available to use as soon as we
> > + * get a hotplug. The type-C PHYs can be shared between multiple controllers:
> > + * display, USB, etc. As a result, handshaking through FIA is required around
> > + * connect and disconnect to cleanly transfer ownership with the controller and
> > + * set the type-C power state.
> > + *
> > + * We could opt to only do the connect flow when we actually try to use the AUX
> > + * channels or do a modeset, then immediately run the disconnect flow after
> > + * usage, but there are some implications on this for a dynamic environment:
> > + * things may go away or change behind our backs. So for now our driver is
> > + * always trying to acquire ownership of the controller as soon as it gets an
> > + * interrupt (or polls state and sees a port is connected) and only gives it
> > + * back when it sees a disconnect. Implementation of a more fine-grained model
> > + * will require a lot of coordination with user space and thorough testing for
> > + * the extra possible cases.
> > + */
> > +static bool icl_tc_phy_connect(struct intel_digital_port *dig_port)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> > +	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> > +	u32 val;
> > +
> > +	if (dig_port->tc_type != TC_PORT_LEGACY &&
> > +	    dig_port->tc_type != TC_PORT_TYPEC)
> > +		return true;
> > +
> > +	val = I915_READ(PORT_TX_DFLEXDPPMS);
> > +	if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) {
> > +		DRM_DEBUG_KMS("DP PHY for TC port %d not ready\n", tc_port);
> > +		WARN_ON(dig_port->tc_legacy_port);
> > +		return false;
> > +	}
> > +
> > +	/*
> > +	 * This function may be called many times in a row without an HPD event
> > +	 * in between, so try to avoid the write when we can.
> > +	 */
> > +	val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > +	if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) {
> > +		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> > +		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > +	}
> > +
> > +	/*
> > +	 * Now we have to re-check the live state, in case the port recently
> > +	 * became disconnected. Not necessary for legacy mode.
> > +	 */
> > +	if (dig_port->tc_type == TC_PORT_TYPEC &&
> > +	    !(I915_READ(PORT_TX_DFLEXDPSP) & TC_LIVE_STATE_TC(tc_port))) {
> > +		DRM_DEBUG_KMS("TC PHY %d sudden disconnect.\n", tc_port);
> > +		icl_tc_phy_disconnect(dig_port);
> > +		return false;
> > +	}
> > +
> > +	return true;
> > +}
> > +
> > +/*
> > + * See the comment at the connect function. This implements the Disconnect
> > + * Flow.
> > + */
> > +void icl_tc_phy_disconnect(struct intel_digital_port *dig_port)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> > +	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
> > +
> > +	if (dig_port->tc_type == TC_PORT_UNKNOWN)
> > +		return;
> > +
> > +	/*
> > +	 * TBT disconnection flow is read the live status, what was done in
> > +	 * caller.
> > +	 */
> > +	if (dig_port->tc_type == TC_PORT_TYPEC ||
> > +	    dig_port->tc_type == TC_PORT_LEGACY) {
> > +		u32 val;
> > +
> > +		val = I915_READ(PORT_TX_DFLEXDPCSSS);
> > +		val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
> > +		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
> > +	}
> > +
> > +	DRM_DEBUG_KMS("Port %c TC type %s disconnected\n",
> > +		      port_name(dig_port->base.port),
> > +		      tc_type_name(dig_port->tc_type));
> > +
> > +	dig_port->tc_type = TC_PORT_UNKNOWN;
> > +}
> > +
> > +static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
> > +				    struct intel_digital_port *intel_dig_port,
> > +				    bool is_legacy, bool is_typec, bool is_tbt)
> > +{
> > +	enum port port = intel_dig_port->base.port;
> > +	enum tc_port_type old_type = intel_dig_port->tc_type;
> > +
> > +	WARN_ON(is_legacy + is_typec + is_tbt != 1);
> > +
> > +	if (is_legacy)
> > +		intel_dig_port->tc_type = TC_PORT_LEGACY;
> > +	else if (is_typec)
> > +		intel_dig_port->tc_type = TC_PORT_TYPEC;
> > +	else if (is_tbt)
> > +		intel_dig_port->tc_type = TC_PORT_TBT;
> > +	else
> > +		return;
> > +
> > +	/* Types are not supposed to be changed at runtime. */
> > +	WARN_ON(old_type != TC_PORT_UNKNOWN &&
> > +		old_type != intel_dig_port->tc_type);
> > +
> > +	if (old_type != intel_dig_port->tc_type)
> > +		DRM_DEBUG_KMS("Port %c has TC type %s\n", port_name(port),
> > +			      tc_type_name(intel_dig_port->tc_type));
> > +}
> > +
> > +
> > +/*
> > + * The type-C ports are different because even when they are connected, they may
> > + * not be available/usable by the graphics driver: see the comment on
> > + * icl_tc_phy_connect(). So in our driver instead of adding the additional
> > + * concept of "usable" and make everything check for "connected and usable" we
> > + * define a port as "connected" when it is not only connected, but also when it
> > + * is usable by the rest of the driver. That maintains the old assumption that
> > + * connected ports are usable, and avoids exposing to the users objects they
> > + * can't really use.
> > + */
> > +bool intel_tc_port_connected(struct intel_digital_port *dig_port)
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
> > +	enum port port = dig_port->base.port;
> > +	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
> > +	bool is_legacy, is_typec, is_tbt;
> > +	u32 dpsp;
> > +
> > +	/*
> > +	 * Complain if we got a legacy port HPD, but VBT didn't mark the port as
> > +	 * legacy. Treat the port as legacy from now on.
> > +	 */
> > +	if (!dig_port->tc_legacy_port &&
> > +	    I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) {
> > +		DRM_ERROR("VBT incorrectly claims port %c is not TypeC legacy\n",
> > +			  port_name(port));
> > +		dig_port->tc_legacy_port = true;
> > +	}
> > +	is_legacy = dig_port->tc_legacy_port;
> > +
> > +	/*
> > +	 * The spec says we shouldn't be using the ISR bits for detecting
> > +	 * between TC and TBT. We should use DFLEXDPSP.
> > +	 */
> > +	dpsp = I915_READ(PORT_TX_DFLEXDPSP);
> > +	is_typec = dpsp & TC_LIVE_STATE_TC(tc_port);
> > +	is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port);
> > +
> > +	if (!is_legacy && !is_typec && !is_tbt) {
> > +		icl_tc_phy_disconnect(dig_port);
> > +
> > +		return false;
> > +	}
> > +
> > +	icl_update_tc_port_type(dev_priv, dig_port, is_legacy, is_typec,
> > +				is_tbt);
> > +
> > +	if (!icl_tc_phy_connect(dig_port))
> > +		return false;
> > +
> > +	return true;
> > +}
> > +
> > diff --git a/drivers/gpu/drm/i915/intel_tc.h b/drivers/gpu/drm/i915/intel_tc.h
> > new file mode 100644
> > index 000000000000..94c62ac4a162
> > --- /dev/null
> > +++ b/drivers/gpu/drm/i915/intel_tc.h
> > @@ -0,0 +1,13 @@
> 
> SPDX header here please.

Ok, added that like:
https://github.com/ideak/linux/blob/2dee09f687ba/drivers/gpu/drm/i915/intel_tc.h#L2

> 
> > +#ifndef __INTEL_TC_H__
> > +#define __INTEL_TC_H__
> > +
> > +#include <linux/types.h>
> > +
> > +struct intel_digital_port;
> > +
> > +void icl_tc_phy_disconnect(struct intel_digital_port *dig_port);
> 
> I'd like to see this named intel_tc_* in the future.

Yep, thought about that rule, which is good, but I kept this one
as-is, since later in this patchset it will get unexported.

> 
> BR,
> Jani.
> 
> 
> > +
> > +bool intel_tc_port_connected(struct intel_digital_port *dig_port);
> > +int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port);
> > +
> > +#endif /* __INTEL_TC_H__ */
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
On Tue, 2019-06-04 at 17:58 +0300, Imre Deak wrote:
> Move the TypeC port handling functions to a new file for clarity.

> 

> While at it:

> - s/icl_tc_port_connected()/intel_tc_port_connected()/

>   icl_tc_phy_disconnect(), will be unexported later.

> 

> -

> s/intel_dp_get_fia_supported_lane_count()/intel_tc_port_fia_max_lane_

> count()/

>   It's used for HDMI legacy mode too.

> 

> - Simplify function interfaces by passing only dig_port to them.

> 

> No functional changes.


Nice.

Reviewed-by: José Roberto de Souza <jose.souza@intel.com>


> 

> Cc: Animesh Manna <animesh.manna@intel.com>

> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>

> Cc: José Roberto de Souza <jose.souza@intel.com>

> Signed-off-by: Imre Deak <imre.deak@intel.com>

> ---

>  drivers/gpu/drm/i915/Makefile             |   3 +-

>  drivers/gpu/drm/i915/Makefile.header-test |   1 +

>  drivers/gpu/drm/i915/intel_ddi.c          |   6 +-

>  drivers/gpu/drm/i915/intel_dp.c           | 227 +-------------------

> -

>  drivers/gpu/drm/i915/intel_dp.h           |   2 -

>  drivers/gpu/drm/i915/intel_tc.c           | 230

> ++++++++++++++++++++++

>  drivers/gpu/drm/i915/intel_tc.h           |  13 ++

>  7 files changed, 252 insertions(+), 230 deletions(-)

>  create mode 100644 drivers/gpu/drm/i915/intel_tc.c

>  create mode 100644 drivers/gpu/drm/i915/intel_tc.h

> 

> diff --git a/drivers/gpu/drm/i915/Makefile

> b/drivers/gpu/drm/i915/Makefile

> index c0a7b2994077..74c4d11d83eb 100644

> --- a/drivers/gpu/drm/i915/Makefile

> +++ b/drivers/gpu/drm/i915/Makefile

> @@ -171,7 +171,8 @@ i915-y += intel_audio.o \

>  	  intel_psr.o \

>  	  intel_quirks.o \

>  	  intel_sideband.o \

> -	  intel_sprite.o

> +	  intel_sprite.o \

> +	  intel_tc.o

>  i915-$(CONFIG_ACPI)		+= intel_acpi.o intel_opregion.o

>  i915-$(CONFIG_DRM_FBDEV_EMULATION)	+= intel_fbdev.o

>  

> diff --git a/drivers/gpu/drm/i915/Makefile.header-test

> b/drivers/gpu/drm/i915/Makefile.header-test

> index 6ef3b647ac65..e80e8e45b09c 100644

> --- a/drivers/gpu/drm/i915/Makefile.header-test

> +++ b/drivers/gpu/drm/i915/Makefile.header-test

> @@ -58,6 +58,7 @@ header_test := \

>  	intel_sdvo.h \

>  	intel_sideband.h \

>  	intel_sprite.h \

> +	intel_tc.h \

>  	intel_tv.h \

>  	intel_uncore.h \

>  	intel_vdsc.h \

> diff --git a/drivers/gpu/drm/i915/intel_ddi.c

> b/drivers/gpu/drm/i915/intel_ddi.c

> index 350eaf54f01f..5a1c98438375 100644

> --- a/drivers/gpu/drm/i915/intel_ddi.c

> +++ b/drivers/gpu/drm/i915/intel_ddi.c

> @@ -45,6 +45,7 @@

>  #include "intel_lspcon.h"

>  #include "intel_panel.h"

>  #include "intel_psr.h"

> +#include "intel_tc.h"

>  #include "intel_vdsc.h"

>  

>  struct ddi_buf_trans {

> @@ -3904,7 +3905,6 @@ static int intel_ddi_compute_config(struct

> intel_encoder *encoder,

>  static void intel_ddi_encoder_suspend(struct intel_encoder *encoder)

>  {

>  	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder-

> >base);

> -	struct drm_i915_private *i915 = to_i915(encoder->base.dev);

>  

>  	intel_dp_encoder_suspend(encoder);

>  

> @@ -3914,7 +3914,7 @@ static void intel_ddi_encoder_suspend(struct

> intel_encoder *encoder)

>  	 * even if the sink has disappeared while being suspended.

>  	 */

>  	if (dig_port->tc_legacy_port)

> -		icl_tc_phy_disconnect(i915, dig_port);

> +		icl_tc_phy_disconnect(dig_port);

>  }

>  

>  static void intel_ddi_encoder_reset(struct drm_encoder *drm_encoder)

> @@ -3936,7 +3936,7 @@ static void intel_ddi_encoder_destroy(struct

> drm_encoder *encoder)

>  	intel_dp_encoder_flush_work(encoder);

>  

>  	if (intel_port_is_tc(i915, dig_port->base.port))

> -		icl_tc_phy_disconnect(i915, dig_port);

> +		icl_tc_phy_disconnect(dig_port);

>  

>  	drm_encoder_cleanup(encoder);

>  	kfree(dig_port);

> diff --git a/drivers/gpu/drm/i915/intel_dp.c

> b/drivers/gpu/drm/i915/intel_dp.c

> index 24b56b2a76c8..b69310bd9914 100644

> --- a/drivers/gpu/drm/i915/intel_dp.c

> +++ b/drivers/gpu/drm/i915/intel_dp.c

> @@ -62,6 +62,7 @@

>  #include "intel_panel.h"

>  #include "intel_psr.h"

>  #include "intel_sideband.h"

> +#include "intel_tc.h"

>  #include "intel_vdsc.h"

>  

>  #define DP_DPRX_ESI_LEN 14

> @@ -211,46 +212,13 @@ static int intel_dp_max_common_rate(struct

> intel_dp *intel_dp)

>  	return intel_dp->common_rates[intel_dp->num_common_rates - 1];

>  }

>  

> -static int intel_dp_get_fia_supported_lane_count(struct intel_dp

> *intel_dp)

> -{

> -	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);

> -	struct drm_i915_private *dev_priv = to_i915(dig_port-

> >base.base.dev);

> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port-

> >base.port);

> -	intel_wakeref_t wakeref;

> -	u32 lane_info;

> -

> -	if (tc_port == PORT_TC_NONE || dig_port->tc_type !=

> TC_PORT_TYPEC)

> -		return 4;

> -

> -	lane_info = 0;

> -	with_intel_display_power(dev_priv, POWER_DOMAIN_DISPLAY_CORE,

> wakeref)

> -		lane_info = (I915_READ(PORT_TX_DFLEXDPSP) &

> -			     DP_LANE_ASSIGNMENT_MASK(tc_port)) >>

> -				DP_LANE_ASSIGNMENT_SHIFT(tc_port);

> -

> -	switch (lane_info) {

> -	default:

> -		MISSING_CASE(lane_info);

> -	case 1:

> -	case 2:

> -	case 4:

> -	case 8:

> -		return 1;

> -	case 3:

> -	case 12:

> -		return 2;

> -	case 15:

> -		return 4;

> -	}

> -}

> -

>  /* Theoretical max between source and sink */

>  static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp)

>  {

>  	struct intel_digital_port *intel_dig_port =

> dp_to_dig_port(intel_dp);

>  	int source_max = intel_dig_port->max_lanes;

>  	int sink_max = drm_dp_max_lane_count(intel_dp->dpcd);

> -	int fia_max = intel_dp_get_fia_supported_lane_count(intel_dp);

> +	int fia_max = intel_tc_port_fia_max_lane_count(intel_dig_port);

>  

>  	return min3(source_max, sink_max, fia_max);

>  }

> @@ -5231,195 +5199,6 @@ static bool icl_combo_port_connected(struct

> drm_i915_private *dev_priv,

>  	return I915_READ(SDEISR) & SDE_DDI_HOTPLUG_ICP(port);

>  }

>  

> -static const char *tc_type_name(enum tc_port_type type)

> -{

> -	static const char * const names[] = {

> -		[TC_PORT_UNKNOWN] = "unknown",

> -		[TC_PORT_LEGACY] = "legacy",

> -		[TC_PORT_TYPEC] = "typec",

> -		[TC_PORT_TBT] = "tbt",

> -	};

> -

> -	if (WARN_ON(type >= ARRAY_SIZE(names)))

> -		type = TC_PORT_UNKNOWN;

> -

> -	return names[type];

> -}

> -

> -static void icl_update_tc_port_type(struct drm_i915_private

> *dev_priv,

> -				    struct intel_digital_port

> *intel_dig_port,

> -				    bool is_legacy, bool is_typec, bool

> is_tbt)

> -{

> -	enum port port = intel_dig_port->base.port;

> -	enum tc_port_type old_type = intel_dig_port->tc_type;

> -

> -	WARN_ON(is_legacy + is_typec + is_tbt != 1);

> -

> -	if (is_legacy)

> -		intel_dig_port->tc_type = TC_PORT_LEGACY;

> -	else if (is_typec)

> -		intel_dig_port->tc_type = TC_PORT_TYPEC;

> -	else if (is_tbt)

> -		intel_dig_port->tc_type = TC_PORT_TBT;

> -	else

> -		return;

> -

> -	/* Types are not supposed to be changed at runtime. */

> -	WARN_ON(old_type != TC_PORT_UNKNOWN &&

> -		old_type != intel_dig_port->tc_type);

> -

> -	if (old_type != intel_dig_port->tc_type)

> -		DRM_DEBUG_KMS("Port %c has TC type %s\n",

> port_name(port),

> -			      tc_type_name(intel_dig_port->tc_type));

> -}

> -

> -/*

> - * This function implements the first part of the Connect Flow

> described by our

> - * specification, Gen11 TypeC Programming chapter. The rest of the

> flow (reading

> - * lanes, EDID, etc) is done as needed in the typical places.

> - *

> - * Unlike the other ports, type-C ports are not available to use as

> soon as we

> - * get a hotplug. The type-C PHYs can be shared between multiple

> controllers:

> - * display, USB, etc. As a result, handshaking through FIA is

> required around

> - * connect and disconnect to cleanly transfer ownership with the

> controller and

> - * set the type-C power state.

> - *

> - * We could opt to only do the connect flow when we actually try to

> use the AUX

> - * channels or do a modeset, then immediately run the disconnect

> flow after

> - * usage, but there are some implications on this for a dynamic

> environment:

> - * things may go away or change behind our backs. So for now our

> driver is

> - * always trying to acquire ownership of the controller as soon as

> it gets an

> - * interrupt (or polls state and sees a port is connected) and only

> gives it

> - * back when it sees a disconnect. Implementation of a more fine-

> grained model

> - * will require a lot of coordination with user space and thorough

> testing for

> - * the extra possible cases.

> - */

> -static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,

> -			       struct intel_digital_port *dig_port)

> -{

> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port-

> >base.port);

> -	u32 val;

> -

> -	if (dig_port->tc_type != TC_PORT_LEGACY &&

> -	    dig_port->tc_type != TC_PORT_TYPEC)

> -		return true;

> -

> -	val = I915_READ(PORT_TX_DFLEXDPPMS);

> -	if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) {

> -		DRM_DEBUG_KMS("DP PHY for TC port %d not ready\n",

> tc_port);

> -		WARN_ON(dig_port->tc_legacy_port);

> -		return false;

> -	}

> -

> -	/*

> -	 * This function may be called many times in a row without an

> HPD event

> -	 * in between, so try to avoid the write when we can.

> -	 */

> -	val = I915_READ(PORT_TX_DFLEXDPCSSS);

> -	if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) {

> -		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);

> -		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);

> -	}

> -

> -	/*

> -	 * Now we have to re-check the live state, in case the port

> recently

> -	 * became disconnected. Not necessary for legacy mode.

> -	 */

> -	if (dig_port->tc_type == TC_PORT_TYPEC &&

> -	    !(I915_READ(PORT_TX_DFLEXDPSP) &

> TC_LIVE_STATE_TC(tc_port))) {

> -		DRM_DEBUG_KMS("TC PHY %d sudden disconnect.\n",

> tc_port);

> -		icl_tc_phy_disconnect(dev_priv, dig_port);

> -		return false;

> -	}

> -

> -	return true;

> -}

> -

> -/*

> - * See the comment at the connect function. This implements the

> Disconnect

> - * Flow.

> - */

> -void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,

> -			   struct intel_digital_port *dig_port)

> -{

> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port-

> >base.port);

> -

> -	if (dig_port->tc_type == TC_PORT_UNKNOWN)

> -		return;

> -

> -	/*

> -	 * TBT disconnection flow is read the live status, what was

> done in

> -	 * caller.

> -	 */

> -	if (dig_port->tc_type == TC_PORT_TYPEC ||

> -	    dig_port->tc_type == TC_PORT_LEGACY) {

> -		u32 val;

> -

> -		val = I915_READ(PORT_TX_DFLEXDPCSSS);

> -		val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);

> -		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);

> -	}

> -

> -	DRM_DEBUG_KMS("Port %c TC type %s disconnected\n",

> -		      port_name(dig_port->base.port),

> -		      tc_type_name(dig_port->tc_type));

> -

> -	dig_port->tc_type = TC_PORT_UNKNOWN;

> -}

> -

> -/*

> - * The type-C ports are different because even when they are

> connected, they may

> - * not be available/usable by the graphics driver: see the comment

> on

> - * icl_tc_phy_connect(). So in our driver instead of adding the

> additional

> - * concept of "usable" and make everything check for "connected and

> usable" we

> - * define a port as "connected" when it is not only connected, but

> also when it

> - * is usable by the rest of the driver. That maintains the old

> assumption that

> - * connected ports are usable, and avoids exposing to the users

> objects they

> - * can't really use.

> - */

> -static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,

> -				  struct intel_digital_port

> *intel_dig_port)

> -{

> -	enum port port = intel_dig_port->base.port;

> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);

> -	bool is_legacy, is_typec, is_tbt;

> -	u32 dpsp;

> -

> -	/*

> -	 * Complain if we got a legacy port HPD, but VBT didn't mark

> the port as

> -	 * legacy. Treat the port as legacy from now on.

> -	 */

> -	if (!intel_dig_port->tc_legacy_port &&

> -	    I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) {

> -		DRM_ERROR("VBT incorrectly claims port %c is not TypeC

> legacy\n",

> -			  port_name(port));

> -		intel_dig_port->tc_legacy_port = true;

> -	}

> -	is_legacy = intel_dig_port->tc_legacy_port;

> -

> -	/*

> -	 * The spec says we shouldn't be using the ISR bits for

> detecting

> -	 * between TC and TBT. We should use DFLEXDPSP.

> -	 */

> -	dpsp = I915_READ(PORT_TX_DFLEXDPSP);

> -	is_typec = dpsp & TC_LIVE_STATE_TC(tc_port);

> -	is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port);

> -

> -	if (!is_legacy && !is_typec && !is_tbt) {

> -		icl_tc_phy_disconnect(dev_priv, intel_dig_port);

> -

> -		return false;

> -	}

> -

> -	icl_update_tc_port_type(dev_priv, intel_dig_port, is_legacy,

> is_typec,

> -				is_tbt);

> -

> -	if (!icl_tc_phy_connect(dev_priv, intel_dig_port))

> -		return false;

> -

> -	return true;

> -}

> -

>  static bool icl_digital_port_connected(struct intel_encoder

> *encoder)

>  {

>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);

> @@ -5428,7 +5207,7 @@ static bool icl_digital_port_connected(struct

> intel_encoder *encoder)

>  	if (intel_port_is_combophy(dev_priv, encoder->port))

>  		return icl_combo_port_connected(dev_priv, dig_port);

>  	else if (intel_port_is_tc(dev_priv, encoder->port))

> -		return icl_tc_port_connected(dev_priv, dig_port);

> +		return intel_tc_port_connected(dig_port);

>  	else

>  		MISSING_CASE(encoder->hpd_pin);

>  

> diff --git a/drivers/gpu/drm/i915/intel_dp.h

> b/drivers/gpu/drm/i915/intel_dp.h

> index da70b1a41c83..657bbb1f5ed0 100644

> --- a/drivers/gpu/drm/i915/intel_dp.h

> +++ b/drivers/gpu/drm/i915/intel_dp.h

> @@ -112,8 +112,6 @@ bool intel_dp_get_colorimetry_status(struct

> intel_dp *intel_dp);

>  int intel_dp_link_required(int pixel_clock, int bpp);

>  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);

>  bool intel_digital_port_connected(struct intel_encoder *encoder);

> -void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,

> -			   struct intel_digital_port *dig_port);

>  

>  static inline unsigned int intel_dp_unused_lane_mask(int lane_count)

>  {

> diff --git a/drivers/gpu/drm/i915/intel_tc.c

> b/drivers/gpu/drm/i915/intel_tc.c

> new file mode 100644

> index 000000000000..7a1b5870945f

> --- /dev/null

> +++ b/drivers/gpu/drm/i915/intel_tc.c

> @@ -0,0 +1,230 @@

> +// SPDX-License-Identifier: MIT

> +/*

> + * Copyright © 2019 Intel Corporation

> + */

> +#include "intel_display.h"

> +#include "i915_drv.h"

> +#include "intel_tc.h"

> +

> +static const char *tc_type_name(enum tc_port_type type)

> +{

> +	static const char * const names[] = {

> +		[TC_PORT_UNKNOWN] = "unknown",

> +		[TC_PORT_LEGACY] = "legacy",

> +		[TC_PORT_TYPEC] = "typec",

> +		[TC_PORT_TBT] = "tbt",

> +	};

> +

> +	if (WARN_ON(type >= ARRAY_SIZE(names)))

> +		type = TC_PORT_UNKNOWN;

> +

> +	return names[type];

> +}

> +

> +int intel_tc_port_fia_max_lane_count(struct intel_digital_port

> *dig_port)

> +{

> +	struct drm_i915_private *dev_priv = to_i915(dig_port-

> >base.base.dev);

> +	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port-

> >base.port);

> +	intel_wakeref_t wakeref;

> +	u32 lane_info;

> +

> +	if (tc_port == PORT_TC_NONE || dig_port->tc_type !=

> TC_PORT_TYPEC)

> +		return 4;

> +

> +	lane_info = 0;

> +	with_intel_display_power(dev_priv, POWER_DOMAIN_DISPLAY_CORE,

> wakeref)

> +		lane_info = (I915_READ(PORT_TX_DFLEXDPSP) &

> +			     DP_LANE_ASSIGNMENT_MASK(tc_port)) >>

> +				DP_LANE_ASSIGNMENT_SHIFT(tc_port);

> +

> +	switch (lane_info) {

> +	default:

> +		MISSING_CASE(lane_info);

> +	case 1:

> +	case 2:

> +	case 4:

> +	case 8:

> +		return 1;

> +	case 3:

> +	case 12:

> +		return 2;

> +	case 15:

> +		return 4;

> +	}

> +}

> +

> +/*

> + * This function implements the first part of the Connect Flow

> described by our

> + * specification, Gen11 TypeC Programming chapter. The rest of the

> flow (reading

> + * lanes, EDID, etc) is done as needed in the typical places.

> + *

> + * Unlike the other ports, type-C ports are not available to use as

> soon as we

> + * get a hotplug. The type-C PHYs can be shared between multiple

> controllers:

> + * display, USB, etc. As a result, handshaking through FIA is

> required around

> + * connect and disconnect to cleanly transfer ownership with the

> controller and

> + * set the type-C power state.

> + *

> + * We could opt to only do the connect flow when we actually try to

> use the AUX

> + * channels or do a modeset, then immediately run the disconnect

> flow after

> + * usage, but there are some implications on this for a dynamic

> environment:

> + * things may go away or change behind our backs. So for now our

> driver is

> + * always trying to acquire ownership of the controller as soon as

> it gets an

> + * interrupt (or polls state and sees a port is connected) and only

> gives it

> + * back when it sees a disconnect. Implementation of a more fine-

> grained model

> + * will require a lot of coordination with user space and thorough

> testing for

> + * the extra possible cases.

> + */

> +static bool icl_tc_phy_connect(struct intel_digital_port *dig_port)

> +{

> +	struct drm_i915_private *dev_priv = to_i915(dig_port-

> >base.base.dev);

> +	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port-

> >base.port);

> +	u32 val;

> +

> +	if (dig_port->tc_type != TC_PORT_LEGACY &&

> +	    dig_port->tc_type != TC_PORT_TYPEC)

> +		return true;

> +

> +	val = I915_READ(PORT_TX_DFLEXDPPMS);

> +	if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) {

> +		DRM_DEBUG_KMS("DP PHY for TC port %d not ready\n",

> tc_port);

> +		WARN_ON(dig_port->tc_legacy_port);

> +		return false;

> +	}

> +

> +	/*

> +	 * This function may be called many times in a row without an

> HPD event

> +	 * in between, so try to avoid the write when we can.

> +	 */

> +	val = I915_READ(PORT_TX_DFLEXDPCSSS);

> +	if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) {

> +		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);

> +		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);

> +	}

> +

> +	/*

> +	 * Now we have to re-check the live state, in case the port

> recently

> +	 * became disconnected. Not necessary for legacy mode.

> +	 */

> +	if (dig_port->tc_type == TC_PORT_TYPEC &&

> +	    !(I915_READ(PORT_TX_DFLEXDPSP) &

> TC_LIVE_STATE_TC(tc_port))) {

> +		DRM_DEBUG_KMS("TC PHY %d sudden disconnect.\n",

> tc_port);

> +		icl_tc_phy_disconnect(dig_port);

> +		return false;

> +	}

> +

> +	return true;

> +}

> +

> +/*

> + * See the comment at the connect function. This implements the

> Disconnect

> + * Flow.

> + */

> +void icl_tc_phy_disconnect(struct intel_digital_port *dig_port)

> +{

> +	struct drm_i915_private *dev_priv = to_i915(dig_port-

> >base.base.dev);

> +	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port-

> >base.port);

> +

> +	if (dig_port->tc_type == TC_PORT_UNKNOWN)

> +		return;

> +

> +	/*

> +	 * TBT disconnection flow is read the live status, what was

> done in

> +	 * caller.

> +	 */

> +	if (dig_port->tc_type == TC_PORT_TYPEC ||

> +	    dig_port->tc_type == TC_PORT_LEGACY) {

> +		u32 val;

> +

> +		val = I915_READ(PORT_TX_DFLEXDPCSSS);

> +		val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);

> +		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);

> +	}

> +

> +	DRM_DEBUG_KMS("Port %c TC type %s disconnected\n",

> +		      port_name(dig_port->base.port),

> +		      tc_type_name(dig_port->tc_type));

> +

> +	dig_port->tc_type = TC_PORT_UNKNOWN;

> +}

> +

> +static void icl_update_tc_port_type(struct drm_i915_private

> *dev_priv,

> +				    struct intel_digital_port

> *intel_dig_port,

> +				    bool is_legacy, bool is_typec, bool

> is_tbt)

> +{

> +	enum port port = intel_dig_port->base.port;

> +	enum tc_port_type old_type = intel_dig_port->tc_type;

> +

> +	WARN_ON(is_legacy + is_typec + is_tbt != 1);

> +

> +	if (is_legacy)

> +		intel_dig_port->tc_type = TC_PORT_LEGACY;

> +	else if (is_typec)

> +		intel_dig_port->tc_type = TC_PORT_TYPEC;

> +	else if (is_tbt)

> +		intel_dig_port->tc_type = TC_PORT_TBT;

> +	else

> +		return;

> +

> +	/* Types are not supposed to be changed at runtime. */

> +	WARN_ON(old_type != TC_PORT_UNKNOWN &&

> +		old_type != intel_dig_port->tc_type);

> +

> +	if (old_type != intel_dig_port->tc_type)

> +		DRM_DEBUG_KMS("Port %c has TC type %s\n",

> port_name(port),

> +			      tc_type_name(intel_dig_port->tc_type));

> +}

> +

> +

> +/*

> + * The type-C ports are different because even when they are

> connected, they may

> + * not be available/usable by the graphics driver: see the comment

> on

> + * icl_tc_phy_connect(). So in our driver instead of adding the

> additional

> + * concept of "usable" and make everything check for "connected and

> usable" we

> + * define a port as "connected" when it is not only connected, but

> also when it

> + * is usable by the rest of the driver. That maintains the old

> assumption that

> + * connected ports are usable, and avoids exposing to the users

> objects they

> + * can't really use.

> + */

> +bool intel_tc_port_connected(struct intel_digital_port *dig_port)

> +{

> +	struct drm_i915_private *dev_priv = to_i915(dig_port-

> >base.base.dev);

> +	enum port port = dig_port->base.port;

> +	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);

> +	bool is_legacy, is_typec, is_tbt;

> +	u32 dpsp;

> +

> +	/*

> +	 * Complain if we got a legacy port HPD, but VBT didn't mark

> the port as

> +	 * legacy. Treat the port as legacy from now on.

> +	 */

> +	if (!dig_port->tc_legacy_port &&

> +	    I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) {

> +		DRM_ERROR("VBT incorrectly claims port %c is not TypeC

> legacy\n",

> +			  port_name(port));

> +		dig_port->tc_legacy_port = true;

> +	}

> +	is_legacy = dig_port->tc_legacy_port;

> +

> +	/*

> +	 * The spec says we shouldn't be using the ISR bits for

> detecting

> +	 * between TC and TBT. We should use DFLEXDPSP.

> +	 */

> +	dpsp = I915_READ(PORT_TX_DFLEXDPSP);

> +	is_typec = dpsp & TC_LIVE_STATE_TC(tc_port);

> +	is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port);

> +

> +	if (!is_legacy && !is_typec && !is_tbt) {

> +		icl_tc_phy_disconnect(dig_port);

> +

> +		return false;

> +	}

> +

> +	icl_update_tc_port_type(dev_priv, dig_port, is_legacy,

> is_typec,

> +				is_tbt);

> +

> +	if (!icl_tc_phy_connect(dig_port))

> +		return false;

> +

> +	return true;

> +}

> +

> diff --git a/drivers/gpu/drm/i915/intel_tc.h

> b/drivers/gpu/drm/i915/intel_tc.h

> new file mode 100644

> index 000000000000..94c62ac4a162

> --- /dev/null

> +++ b/drivers/gpu/drm/i915/intel_tc.h

> @@ -0,0 +1,13 @@

> +#ifndef __INTEL_TC_H__

> +#define __INTEL_TC_H__

> +

> +#include <linux/types.h>

> +

> +struct intel_digital_port;

> +

> +void icl_tc_phy_disconnect(struct intel_digital_port *dig_port);

> +

> +bool intel_tc_port_connected(struct intel_digital_port *dig_port);

> +int intel_tc_port_fia_max_lane_count(struct intel_digital_port

> *dig_port);

> +

> +#endif /* __INTEL_TC_H__ */
On Thu, Jun 06, 2019 at 11:42:46AM +0300, Jani Nikula wrote:
>On Tue, 04 Jun 2019, Imre Deak <imre.deak@intel.com> wrote:
>> Move the TypeC port handling functions to a new file for clarity.
>>
>> While at it:
>> - s/icl_tc_port_connected()/intel_tc_port_connected()/
>>   icl_tc_phy_disconnect(), will be unexported later.
>>
>> - s/intel_dp_get_fia_supported_lane_count()/intel_tc_port_fia_max_lane_count()/
>>   It's used for HDMI legacy mode too.
>>
>> - Simplify function interfaces by passing only dig_port to them.
>>
>> No functional changes.
>
>Some nitpicks below.
>
>BR,
>Jani.
>
>
>>
>> Cc: Animesh Manna <animesh.manna@intel.com>
>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> Cc: José Roberto de Souza <jose.souza@intel.com>
>> Signed-off-by: Imre Deak <imre.deak@intel.com>
>> ---
>>  drivers/gpu/drm/i915/Makefile             |   3 +-
>>  drivers/gpu/drm/i915/Makefile.header-test |   1 +
>>  drivers/gpu/drm/i915/intel_ddi.c          |   6 +-
>>  drivers/gpu/drm/i915/intel_dp.c           | 227 +--------------------
>>  drivers/gpu/drm/i915/intel_dp.h           |   2 -
>>  drivers/gpu/drm/i915/intel_tc.c           | 230 ++++++++++++++++++++++
>>  drivers/gpu/drm/i915/intel_tc.h           |  13 ++
>>  7 files changed, 252 insertions(+), 230 deletions(-)
>>  create mode 100644 drivers/gpu/drm/i915/intel_tc.c
>>  create mode 100644 drivers/gpu/drm/i915/intel_tc.h
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
>> index c0a7b2994077..74c4d11d83eb 100644
>> --- a/drivers/gpu/drm/i915/Makefile
>> +++ b/drivers/gpu/drm/i915/Makefile
>> @@ -171,7 +171,8 @@ i915-y += intel_audio.o \
>>  	  intel_psr.o \
>>  	  intel_quirks.o \
>>  	  intel_sideband.o \
>> -	  intel_sprite.o
>> +	  intel_sprite.o \
>> +	  intel_tc.o
>>  i915-$(CONFIG_ACPI)		+= intel_acpi.o intel_opregion.o
>>  i915-$(CONFIG_DRM_FBDEV_EMULATION)	+= intel_fbdev.o
>>
>> diff --git a/drivers/gpu/drm/i915/Makefile.header-test b/drivers/gpu/drm/i915/Makefile.header-test
>> index 6ef3b647ac65..e80e8e45b09c 100644
>> --- a/drivers/gpu/drm/i915/Makefile.header-test
>> +++ b/drivers/gpu/drm/i915/Makefile.header-test
>> @@ -58,6 +58,7 @@ header_test := \
>>  	intel_sdvo.h \
>>  	intel_sideband.h \
>>  	intel_sprite.h \
>> +	intel_tc.h \
>>  	intel_tv.h \
>>  	intel_uncore.h \
>>  	intel_vdsc.h \
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index 350eaf54f01f..5a1c98438375 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -45,6 +45,7 @@
>>  #include "intel_lspcon.h"
>>  #include "intel_panel.h"
>>  #include "intel_psr.h"
>> +#include "intel_tc.h"
>>  #include "intel_vdsc.h"
>>
>>  struct ddi_buf_trans {
>> @@ -3904,7 +3905,6 @@ static int intel_ddi_compute_config(struct intel_encoder *encoder,
>>  static void intel_ddi_encoder_suspend(struct intel_encoder *encoder)
>>  {
>>  	struct intel_digital_port *dig_port = enc_to_dig_port(&encoder->base);
>> -	struct drm_i915_private *i915 = to_i915(encoder->base.dev);
>>
>>  	intel_dp_encoder_suspend(encoder);
>>
>> @@ -3914,7 +3914,7 @@ static void intel_ddi_encoder_suspend(struct intel_encoder *encoder)
>>  	 * even if the sink has disappeared while being suspended.
>>  	 */
>>  	if (dig_port->tc_legacy_port)
>> -		icl_tc_phy_disconnect(i915, dig_port);
>> +		icl_tc_phy_disconnect(dig_port);
>>  }
>>
>>  static void intel_ddi_encoder_reset(struct drm_encoder *drm_encoder)
>> @@ -3936,7 +3936,7 @@ static void intel_ddi_encoder_destroy(struct drm_encoder *encoder)
>>  	intel_dp_encoder_flush_work(encoder);
>>
>>  	if (intel_port_is_tc(i915, dig_port->base.port))
>> -		icl_tc_phy_disconnect(i915, dig_port);
>> +		icl_tc_phy_disconnect(dig_port);
>>
>>  	drm_encoder_cleanup(encoder);
>>  	kfree(dig_port);
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 24b56b2a76c8..b69310bd9914 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -62,6 +62,7 @@
>>  #include "intel_panel.h"
>>  #include "intel_psr.h"
>>  #include "intel_sideband.h"
>> +#include "intel_tc.h"
>>  #include "intel_vdsc.h"
>>
>>  #define DP_DPRX_ESI_LEN 14
>> @@ -211,46 +212,13 @@ static int intel_dp_max_common_rate(struct intel_dp *intel_dp)
>>  	return intel_dp->common_rates[intel_dp->num_common_rates - 1];
>>  }
>>
>> -static int intel_dp_get_fia_supported_lane_count(struct intel_dp *intel_dp)
>> -{
>> -	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> -	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
>> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
>> -	intel_wakeref_t wakeref;
>> -	u32 lane_info;
>> -
>> -	if (tc_port == PORT_TC_NONE || dig_port->tc_type != TC_PORT_TYPEC)
>> -		return 4;
>> -
>> -	lane_info = 0;
>> -	with_intel_display_power(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref)
>> -		lane_info = (I915_READ(PORT_TX_DFLEXDPSP) &
>> -			     DP_LANE_ASSIGNMENT_MASK(tc_port)) >>
>> -				DP_LANE_ASSIGNMENT_SHIFT(tc_port);
>> -
>> -	switch (lane_info) {
>> -	default:
>> -		MISSING_CASE(lane_info);
>> -	case 1:
>> -	case 2:
>> -	case 4:
>> -	case 8:
>> -		return 1;
>> -	case 3:
>> -	case 12:
>> -		return 2;
>> -	case 15:
>> -		return 4;
>> -	}
>> -}
>> -
>>  /* Theoretical max between source and sink */
>>  static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp)
>>  {
>>  	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
>>  	int source_max = intel_dig_port->max_lanes;
>>  	int sink_max = drm_dp_max_lane_count(intel_dp->dpcd);
>> -	int fia_max = intel_dp_get_fia_supported_lane_count(intel_dp);
>> +	int fia_max = intel_tc_port_fia_max_lane_count(intel_dig_port);
>>
>>  	return min3(source_max, sink_max, fia_max);
>>  }
>> @@ -5231,195 +5199,6 @@ static bool icl_combo_port_connected(struct drm_i915_private *dev_priv,
>>  	return I915_READ(SDEISR) & SDE_DDI_HOTPLUG_ICP(port);
>>  }
>>
>> -static const char *tc_type_name(enum tc_port_type type)
>> -{
>> -	static const char * const names[] = {
>> -		[TC_PORT_UNKNOWN] = "unknown",
>> -		[TC_PORT_LEGACY] = "legacy",
>> -		[TC_PORT_TYPEC] = "typec",
>> -		[TC_PORT_TBT] = "tbt",
>> -	};
>> -
>> -	if (WARN_ON(type >= ARRAY_SIZE(names)))
>> -		type = TC_PORT_UNKNOWN;
>> -
>> -	return names[type];
>> -}
>> -
>> -static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
>> -				    struct intel_digital_port *intel_dig_port,
>> -				    bool is_legacy, bool is_typec, bool is_tbt)
>> -{
>> -	enum port port = intel_dig_port->base.port;
>> -	enum tc_port_type old_type = intel_dig_port->tc_type;
>> -
>> -	WARN_ON(is_legacy + is_typec + is_tbt != 1);
>> -
>> -	if (is_legacy)
>> -		intel_dig_port->tc_type = TC_PORT_LEGACY;
>> -	else if (is_typec)
>> -		intel_dig_port->tc_type = TC_PORT_TYPEC;
>> -	else if (is_tbt)
>> -		intel_dig_port->tc_type = TC_PORT_TBT;
>> -	else
>> -		return;
>> -
>> -	/* Types are not supposed to be changed at runtime. */
>> -	WARN_ON(old_type != TC_PORT_UNKNOWN &&
>> -		old_type != intel_dig_port->tc_type);
>> -
>> -	if (old_type != intel_dig_port->tc_type)
>> -		DRM_DEBUG_KMS("Port %c has TC type %s\n", port_name(port),
>> -			      tc_type_name(intel_dig_port->tc_type));
>> -}
>> -
>> -/*
>> - * This function implements the first part of the Connect Flow described by our
>> - * specification, Gen11 TypeC Programming chapter. The rest of the flow (reading
>> - * lanes, EDID, etc) is done as needed in the typical places.
>> - *
>> - * Unlike the other ports, type-C ports are not available to use as soon as we
>> - * get a hotplug. The type-C PHYs can be shared between multiple controllers:
>> - * display, USB, etc. As a result, handshaking through FIA is required around
>> - * connect and disconnect to cleanly transfer ownership with the controller and
>> - * set the type-C power state.
>> - *
>> - * We could opt to only do the connect flow when we actually try to use the AUX
>> - * channels or do a modeset, then immediately run the disconnect flow after
>> - * usage, but there are some implications on this for a dynamic environment:
>> - * things may go away or change behind our backs. So for now our driver is
>> - * always trying to acquire ownership of the controller as soon as it gets an
>> - * interrupt (or polls state and sees a port is connected) and only gives it
>> - * back when it sees a disconnect. Implementation of a more fine-grained model
>> - * will require a lot of coordination with user space and thorough testing for
>> - * the extra possible cases.
>> - */
>> -static bool icl_tc_phy_connect(struct drm_i915_private *dev_priv,
>> -			       struct intel_digital_port *dig_port)
>> -{
>> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
>> -	u32 val;
>> -
>> -	if (dig_port->tc_type != TC_PORT_LEGACY &&
>> -	    dig_port->tc_type != TC_PORT_TYPEC)
>> -		return true;
>> -
>> -	val = I915_READ(PORT_TX_DFLEXDPPMS);
>> -	if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) {
>> -		DRM_DEBUG_KMS("DP PHY for TC port %d not ready\n", tc_port);
>> -		WARN_ON(dig_port->tc_legacy_port);
>> -		return false;
>> -	}
>> -
>> -	/*
>> -	 * This function may be called many times in a row without an HPD event
>> -	 * in between, so try to avoid the write when we can.
>> -	 */
>> -	val = I915_READ(PORT_TX_DFLEXDPCSSS);
>> -	if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) {
>> -		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
>> -		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
>> -	}
>> -
>> -	/*
>> -	 * Now we have to re-check the live state, in case the port recently
>> -	 * became disconnected. Not necessary for legacy mode.
>> -	 */
>> -	if (dig_port->tc_type == TC_PORT_TYPEC &&
>> -	    !(I915_READ(PORT_TX_DFLEXDPSP) & TC_LIVE_STATE_TC(tc_port))) {
>> -		DRM_DEBUG_KMS("TC PHY %d sudden disconnect.\n", tc_port);
>> -		icl_tc_phy_disconnect(dev_priv, dig_port);
>> -		return false;
>> -	}
>> -
>> -	return true;
>> -}
>> -
>> -/*
>> - * See the comment at the connect function. This implements the Disconnect
>> - * Flow.
>> - */
>> -void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
>> -			   struct intel_digital_port *dig_port)
>> -{
>> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
>> -
>> -	if (dig_port->tc_type == TC_PORT_UNKNOWN)
>> -		return;
>> -
>> -	/*
>> -	 * TBT disconnection flow is read the live status, what was done in
>> -	 * caller.
>> -	 */
>> -	if (dig_port->tc_type == TC_PORT_TYPEC ||
>> -	    dig_port->tc_type == TC_PORT_LEGACY) {
>> -		u32 val;
>> -
>> -		val = I915_READ(PORT_TX_DFLEXDPCSSS);
>> -		val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
>> -		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
>> -	}
>> -
>> -	DRM_DEBUG_KMS("Port %c TC type %s disconnected\n",
>> -		      port_name(dig_port->base.port),
>> -		      tc_type_name(dig_port->tc_type));
>> -
>> -	dig_port->tc_type = TC_PORT_UNKNOWN;
>> -}
>> -
>> -/*
>> - * The type-C ports are different because even when they are connected, they may
>> - * not be available/usable by the graphics driver: see the comment on
>> - * icl_tc_phy_connect(). So in our driver instead of adding the additional
>> - * concept of "usable" and make everything check for "connected and usable" we
>> - * define a port as "connected" when it is not only connected, but also when it
>> - * is usable by the rest of the driver. That maintains the old assumption that
>> - * connected ports are usable, and avoids exposing to the users objects they
>> - * can't really use.
>> - */
>> -static bool icl_tc_port_connected(struct drm_i915_private *dev_priv,
>> -				  struct intel_digital_port *intel_dig_port)
>> -{
>> -	enum port port = intel_dig_port->base.port;
>> -	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
>> -	bool is_legacy, is_typec, is_tbt;
>> -	u32 dpsp;
>> -
>> -	/*
>> -	 * Complain if we got a legacy port HPD, but VBT didn't mark the port as
>> -	 * legacy. Treat the port as legacy from now on.
>> -	 */
>> -	if (!intel_dig_port->tc_legacy_port &&
>> -	    I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) {
>> -		DRM_ERROR("VBT incorrectly claims port %c is not TypeC legacy\n",
>> -			  port_name(port));
>> -		intel_dig_port->tc_legacy_port = true;
>> -	}
>> -	is_legacy = intel_dig_port->tc_legacy_port;
>> -
>> -	/*
>> -	 * The spec says we shouldn't be using the ISR bits for detecting
>> -	 * between TC and TBT. We should use DFLEXDPSP.
>> -	 */
>> -	dpsp = I915_READ(PORT_TX_DFLEXDPSP);
>> -	is_typec = dpsp & TC_LIVE_STATE_TC(tc_port);
>> -	is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port);
>> -
>> -	if (!is_legacy && !is_typec && !is_tbt) {
>> -		icl_tc_phy_disconnect(dev_priv, intel_dig_port);
>> -
>> -		return false;
>> -	}
>> -
>> -	icl_update_tc_port_type(dev_priv, intel_dig_port, is_legacy, is_typec,
>> -				is_tbt);
>> -
>> -	if (!icl_tc_phy_connect(dev_priv, intel_dig_port))
>> -		return false;
>> -
>> -	return true;
>> -}
>> -
>>  static bool icl_digital_port_connected(struct intel_encoder *encoder)
>>  {
>>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>> @@ -5428,7 +5207,7 @@ static bool icl_digital_port_connected(struct intel_encoder *encoder)
>>  	if (intel_port_is_combophy(dev_priv, encoder->port))
>>  		return icl_combo_port_connected(dev_priv, dig_port);
>>  	else if (intel_port_is_tc(dev_priv, encoder->port))
>> -		return icl_tc_port_connected(dev_priv, dig_port);
>> +		return intel_tc_port_connected(dig_port);
>>  	else
>>  		MISSING_CASE(encoder->hpd_pin);
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.h b/drivers/gpu/drm/i915/intel_dp.h
>> index da70b1a41c83..657bbb1f5ed0 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.h
>> +++ b/drivers/gpu/drm/i915/intel_dp.h
>> @@ -112,8 +112,6 @@ bool intel_dp_get_colorimetry_status(struct intel_dp *intel_dp);
>>  int intel_dp_link_required(int pixel_clock, int bpp);
>>  int intel_dp_max_data_rate(int max_link_clock, int max_lanes);
>>  bool intel_digital_port_connected(struct intel_encoder *encoder);
>> -void icl_tc_phy_disconnect(struct drm_i915_private *dev_priv,
>> -			   struct intel_digital_port *dig_port);
>>
>>  static inline unsigned int intel_dp_unused_lane_mask(int lane_count)
>>  {
>> diff --git a/drivers/gpu/drm/i915/intel_tc.c b/drivers/gpu/drm/i915/intel_tc.c
>> new file mode 100644
>> index 000000000000..7a1b5870945f
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_tc.c
>> @@ -0,0 +1,230 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2019 Intel Corporation
>> + */
>
>From the Nitpicks Department, please add a blank line here.
>
>> +#include "intel_display.h"
>> +#include "i915_drv.h"
>> +#include "intel_tc.h"
>
>And sort the includes.

humn... In some other projects I use the convention of always having the
correspondent header to be included first and the rest sorted. Here it
would be like:

#include "intel_tc.h"

#include "intel_display.h"
#include "intel_drv.h"

This way we guarantee that intel_tc.h header is self-contained: anything
it needs, it includes or declares.  If we adopted such convention I don't
see why we would still need Makefile.header-test for example.

Lucas De Marchi

>
>> +
>> +static const char *tc_type_name(enum tc_port_type type)
>> +{
>> +	static const char * const names[] = {
>> +		[TC_PORT_UNKNOWN] = "unknown",
>> +		[TC_PORT_LEGACY] = "legacy",
>> +		[TC_PORT_TYPEC] = "typec",
>> +		[TC_PORT_TBT] = "tbt",
>> +	};
>> +
>> +	if (WARN_ON(type >= ARRAY_SIZE(names)))
>> +		type = TC_PORT_UNKNOWN;
>> +
>> +	return names[type];
>> +}
>> +
>> +int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port)
>> +{
>> +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
>> +	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
>> +	intel_wakeref_t wakeref;
>> +	u32 lane_info;
>> +
>> +	if (tc_port == PORT_TC_NONE || dig_port->tc_type != TC_PORT_TYPEC)
>> +		return 4;
>> +
>> +	lane_info = 0;
>> +	with_intel_display_power(dev_priv, POWER_DOMAIN_DISPLAY_CORE, wakeref)
>> +		lane_info = (I915_READ(PORT_TX_DFLEXDPSP) &
>> +			     DP_LANE_ASSIGNMENT_MASK(tc_port)) >>
>> +				DP_LANE_ASSIGNMENT_SHIFT(tc_port);
>> +
>> +	switch (lane_info) {
>> +	default:
>> +		MISSING_CASE(lane_info);
>> +	case 1:
>> +	case 2:
>> +	case 4:
>> +	case 8:
>> +		return 1;
>> +	case 3:
>> +	case 12:
>> +		return 2;
>> +	case 15:
>> +		return 4;
>> +	}
>> +}
>> +
>> +/*
>> + * This function implements the first part of the Connect Flow described by our
>> + * specification, Gen11 TypeC Programming chapter. The rest of the flow (reading
>> + * lanes, EDID, etc) is done as needed in the typical places.
>> + *
>> + * Unlike the other ports, type-C ports are not available to use as soon as we
>> + * get a hotplug. The type-C PHYs can be shared between multiple controllers:
>> + * display, USB, etc. As a result, handshaking through FIA is required around
>> + * connect and disconnect to cleanly transfer ownership with the controller and
>> + * set the type-C power state.
>> + *
>> + * We could opt to only do the connect flow when we actually try to use the AUX
>> + * channels or do a modeset, then immediately run the disconnect flow after
>> + * usage, but there are some implications on this for a dynamic environment:
>> + * things may go away or change behind our backs. So for now our driver is
>> + * always trying to acquire ownership of the controller as soon as it gets an
>> + * interrupt (or polls state and sees a port is connected) and only gives it
>> + * back when it sees a disconnect. Implementation of a more fine-grained model
>> + * will require a lot of coordination with user space and thorough testing for
>> + * the extra possible cases.
>> + */
>> +static bool icl_tc_phy_connect(struct intel_digital_port *dig_port)
>> +{
>> +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
>> +	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
>> +	u32 val;
>> +
>> +	if (dig_port->tc_type != TC_PORT_LEGACY &&
>> +	    dig_port->tc_type != TC_PORT_TYPEC)
>> +		return true;
>> +
>> +	val = I915_READ(PORT_TX_DFLEXDPPMS);
>> +	if (!(val & DP_PHY_MODE_STATUS_COMPLETED(tc_port))) {
>> +		DRM_DEBUG_KMS("DP PHY for TC port %d not ready\n", tc_port);
>> +		WARN_ON(dig_port->tc_legacy_port);
>> +		return false;
>> +	}
>> +
>> +	/*
>> +	 * This function may be called many times in a row without an HPD event
>> +	 * in between, so try to avoid the write when we can.
>> +	 */
>> +	val = I915_READ(PORT_TX_DFLEXDPCSSS);
>> +	if (!(val & DP_PHY_MODE_STATUS_NOT_SAFE(tc_port))) {
>> +		val |= DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
>> +		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
>> +	}
>> +
>> +	/*
>> +	 * Now we have to re-check the live state, in case the port recently
>> +	 * became disconnected. Not necessary for legacy mode.
>> +	 */
>> +	if (dig_port->tc_type == TC_PORT_TYPEC &&
>> +	    !(I915_READ(PORT_TX_DFLEXDPSP) & TC_LIVE_STATE_TC(tc_port))) {
>> +		DRM_DEBUG_KMS("TC PHY %d sudden disconnect.\n", tc_port);
>> +		icl_tc_phy_disconnect(dig_port);
>> +		return false;
>> +	}
>> +
>> +	return true;
>> +}
>> +
>> +/*
>> + * See the comment at the connect function. This implements the Disconnect
>> + * Flow.
>> + */
>> +void icl_tc_phy_disconnect(struct intel_digital_port *dig_port)
>> +{
>> +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
>> +	enum tc_port tc_port = intel_port_to_tc(dev_priv, dig_port->base.port);
>> +
>> +	if (dig_port->tc_type == TC_PORT_UNKNOWN)
>> +		return;
>> +
>> +	/*
>> +	 * TBT disconnection flow is read the live status, what was done in
>> +	 * caller.
>> +	 */
>> +	if (dig_port->tc_type == TC_PORT_TYPEC ||
>> +	    dig_port->tc_type == TC_PORT_LEGACY) {
>> +		u32 val;
>> +
>> +		val = I915_READ(PORT_TX_DFLEXDPCSSS);
>> +		val &= ~DP_PHY_MODE_STATUS_NOT_SAFE(tc_port);
>> +		I915_WRITE(PORT_TX_DFLEXDPCSSS, val);
>> +	}
>> +
>> +	DRM_DEBUG_KMS("Port %c TC type %s disconnected\n",
>> +		      port_name(dig_port->base.port),
>> +		      tc_type_name(dig_port->tc_type));
>> +
>> +	dig_port->tc_type = TC_PORT_UNKNOWN;
>> +}
>> +
>> +static void icl_update_tc_port_type(struct drm_i915_private *dev_priv,
>> +				    struct intel_digital_port *intel_dig_port,
>> +				    bool is_legacy, bool is_typec, bool is_tbt)
>> +{
>> +	enum port port = intel_dig_port->base.port;
>> +	enum tc_port_type old_type = intel_dig_port->tc_type;
>> +
>> +	WARN_ON(is_legacy + is_typec + is_tbt != 1);
>> +
>> +	if (is_legacy)
>> +		intel_dig_port->tc_type = TC_PORT_LEGACY;
>> +	else if (is_typec)
>> +		intel_dig_port->tc_type = TC_PORT_TYPEC;
>> +	else if (is_tbt)
>> +		intel_dig_port->tc_type = TC_PORT_TBT;
>> +	else
>> +		return;
>> +
>> +	/* Types are not supposed to be changed at runtime. */
>> +	WARN_ON(old_type != TC_PORT_UNKNOWN &&
>> +		old_type != intel_dig_port->tc_type);
>> +
>> +	if (old_type != intel_dig_port->tc_type)
>> +		DRM_DEBUG_KMS("Port %c has TC type %s\n", port_name(port),
>> +			      tc_type_name(intel_dig_port->tc_type));
>> +}
>> +
>> +
>> +/*
>> + * The type-C ports are different because even when they are connected, they may
>> + * not be available/usable by the graphics driver: see the comment on
>> + * icl_tc_phy_connect(). So in our driver instead of adding the additional
>> + * concept of "usable" and make everything check for "connected and usable" we
>> + * define a port as "connected" when it is not only connected, but also when it
>> + * is usable by the rest of the driver. That maintains the old assumption that
>> + * connected ports are usable, and avoids exposing to the users objects they
>> + * can't really use.
>> + */
>> +bool intel_tc_port_connected(struct intel_digital_port *dig_port)
>> +{
>> +	struct drm_i915_private *dev_priv = to_i915(dig_port->base.base.dev);
>> +	enum port port = dig_port->base.port;
>> +	enum tc_port tc_port = intel_port_to_tc(dev_priv, port);
>> +	bool is_legacy, is_typec, is_tbt;
>> +	u32 dpsp;
>> +
>> +	/*
>> +	 * Complain if we got a legacy port HPD, but VBT didn't mark the port as
>> +	 * legacy. Treat the port as legacy from now on.
>> +	 */
>> +	if (!dig_port->tc_legacy_port &&
>> +	    I915_READ(SDEISR) & SDE_TC_HOTPLUG_ICP(tc_port)) {
>> +		DRM_ERROR("VBT incorrectly claims port %c is not TypeC legacy\n",
>> +			  port_name(port));
>> +		dig_port->tc_legacy_port = true;
>> +	}
>> +	is_legacy = dig_port->tc_legacy_port;
>> +
>> +	/*
>> +	 * The spec says we shouldn't be using the ISR bits for detecting
>> +	 * between TC and TBT. We should use DFLEXDPSP.
>> +	 */
>> +	dpsp = I915_READ(PORT_TX_DFLEXDPSP);
>> +	is_typec = dpsp & TC_LIVE_STATE_TC(tc_port);
>> +	is_tbt = dpsp & TC_LIVE_STATE_TBT(tc_port);
>> +
>> +	if (!is_legacy && !is_typec && !is_tbt) {
>> +		icl_tc_phy_disconnect(dig_port);
>> +
>> +		return false;
>> +	}
>> +
>> +	icl_update_tc_port_type(dev_priv, dig_port, is_legacy, is_typec,
>> +				is_tbt);
>> +
>> +	if (!icl_tc_phy_connect(dig_port))
>> +		return false;
>> +
>> +	return true;
>> +}
>> +
>> diff --git a/drivers/gpu/drm/i915/intel_tc.h b/drivers/gpu/drm/i915/intel_tc.h
>> new file mode 100644
>> index 000000000000..94c62ac4a162
>> --- /dev/null
>> +++ b/drivers/gpu/drm/i915/intel_tc.h
>> @@ -0,0 +1,13 @@
>
>SPDX header here please.
>
>> +#ifndef __INTEL_TC_H__
>> +#define __INTEL_TC_H__
>> +
>> +#include <linux/types.h>
>> +
>> +struct intel_digital_port;
>> +
>> +void icl_tc_phy_disconnect(struct intel_digital_port *dig_port);
>
>I'd like to see this named intel_tc_* in the future.
>
>BR,
>Jani.
>
>
>> +
>> +bool intel_tc_port_connected(struct intel_digital_port *dig_port);
>> +int intel_tc_port_fia_max_lane_count(struct intel_digital_port *dig_port);
>> +
>> +#endif /* __INTEL_TC_H__ */
>
>-- 
>Jani Nikula, Intel Open Source Graphics Center
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, 10 Jun 2019, Lucas De Marchi <lucas.de.marchi@gmail.com> wrote:
> On Thu, Jun 06, 2019 at 11:42:46AM +0300, Jani Nikula wrote:
>>On Tue, 04 Jun 2019, Imre Deak <imre.deak@intel.com> wrote:
>>From the Nitpicks Department, please add a blank line here.
>>
>>> +#include "intel_display.h"
>>> +#include "i915_drv.h"
>>> +#include "intel_tc.h"
>>
>>And sort the includes.
>
> humn... In some other projects I use the convention of always having the
> correspondent header to be included first and the rest sorted. Here it
> would be like:
>
> #include "intel_tc.h"
>
> #include "intel_display.h"
> #include "intel_drv.h"
>
> This way we guarantee that intel_tc.h header is self-contained: anything
> it needs, it includes or declares.  If we adopted such convention I don't
> see why we would still need Makefile.header-test for example.

I think the long standing convention is to have <asm>, <linux>, and
<drm> headers included before "local" headers.

Eventually the version of header tests that I've submitted to kbuild
upstream will obsolete the Makefile.header-test files, and I expect us
to have simply

header-test-y := $(notdir $(wildcard $(src)/*.h))

in there, testing everything.

BR,
Jani.