[PATCH v3 0/6] thunderbolt: CL1 support for USB4 and Titan Ridge

[PATCH v3 0/6] thunderbolt: CL1 support for USB4 and Titan Ridge

[Gil Fine]

v1 can be found here:
  https://lore.kernel.org/linux-usb/20220501203321.19021-1-gil.fine@intel.com/
v2 can be found here:
  https://lore.kernel.org/linux-usb/20220509201656.502-1-gil.fine@intel.com/

Changes in v3:
* Fix to support the case of enabling CL1 entry after resume
  from runtime PM (if CL1 suppported in the connected device)

Changes in v2:
* Handle CL1 and CL0s together since on the hardware level they are
  supported and enabled together
* Use device_for_each_child() to set TMU mode of host router's 1st
  children
* Use FIELD_x macros from include/linux/bitfield.h
* Split single patch into two for clarity
* Fix commit message

In this series of patches, first, we address several issues in the CL0s
implementation.
Then, we add support for a second low power state of the
link: CL1. Low power states (called collectively CLx) are used to reduce
transmitter and receiver power when a high-speed lane is idle.
We enable it, if both sides of the link support it, and only for the
first hop router (i.e. the first device that connected to the
host router). This is needed for better thermal management.
CL1 improves power management that was intoduced by CL0s.
Also, we add support of dynamic change of TMU mode to HiFi uni-directional
once DisplayPort tunnel is created.
This enables CL0s while DP tunnel exists.
Due to Intel hardware limitation, once we changed the TMU mode to HiFi
uni-directional (when DP tunnel exists), we don't change TMU mode back
to normal uni-directional, even if DP tunnel is torn down later.
Though, after sleep or runtime PM resume, the TMU is changed to normal
uni-directional (if CLx suppported in the connected device) to enable
CL1 entry.

Gil Fine (6):
  thunderbolt: Silently ignore CLx enabling in case CLx is not supported
  thunderbolt: CLx disable before system suspend only if previously
    enabled
  thunderbolt: Fix typos in CLx enabling
  thunderbolt: Change downstream router's TMU rate in both TMU uni/bidir
    mode
  thunderbolt: Add CL1 support for USB4 and Titan Ridge routers
  thunderbolt: Change TMU mode to HiFi uni-directional once DisplayPort
    tunneled

 drivers/thunderbolt/switch.c  |  91 ++++++++--------
 drivers/thunderbolt/tb.c      |  83 +++++++++++---
 drivers/thunderbolt/tb.h      |  51 +++++----
 drivers/thunderbolt/tb_regs.h |   6 ++
 drivers/thunderbolt/tmu.c     | 198 ++++++++++++++++++++++++++++------
 5 files changed, 322 insertions(+), 107 deletions(-)

-- 
2.17.1

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH v3 1/6] thunderbolt: Silently ignore CLx enabling in case CLx is not supported

[Gil Fine]

We can't enable CLx if it is not supported either by the host or device,
or by the USB4/TBT link (e.g. when an optical cable is used).
We silently ignore CLx enabling in this case.

Signed-off-by: Gil Fine <gil.fine@intel.com>
---
 drivers/thunderbolt/tb.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index 44d04b651a8b..7419cd1aefba 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -581,6 +581,7 @@ static void tb_scan_port(struct tb_port *port)
 	struct tb_cm *tcm = tb_priv(port->sw->tb);
 	struct tb_port *upstream_port;
 	struct tb_switch *sw;
+	int ret;
 
 	if (tb_is_upstream_port(port))
 		return;
@@ -669,7 +670,9 @@ static void tb_scan_port(struct tb_port *port)
 	tb_switch_lane_bonding_enable(sw);
 	/* Set the link configured */
 	tb_switch_configure_link(sw);
-	if (tb_switch_enable_clx(sw, TB_CL0S))
+	/* Silently ignore CLx enabling in case CLx is not supported */
+	ret = tb_switch_enable_clx(sw, TB_CL0S);
+	if (ret && ret != -EOPNOTSUPP)
 		tb_sw_warn(sw, "failed to enable CLx on upstream port\n");
 
 	tb_switch_tmu_configure(sw, TB_SWITCH_TMU_RATE_HIFI,
@@ -1452,12 +1455,15 @@ static int tb_suspend_noirq(struct tb *tb)
 static void tb_restore_children(struct tb_switch *sw)
 {
 	struct tb_port *port;
+	int ret;
 
 	/* No need to restore if the router is already unplugged */
 	if (sw->is_unplugged)
 		return;
 
-	if (tb_switch_enable_clx(sw, TB_CL0S))
+	/* Silently ignore CLx re-enabling in case CLx is not supported */
+	ret = tb_switch_enable_clx(sw, TB_CL0S);
+	if (ret && ret != -EOPNOTSUPP)
 		tb_sw_warn(sw, "failed to re-enable CLx on upstream port\n");
 
 	/*
-- 
2.17.1

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH v3 2/6] thunderbolt: CLx disable before system suspend only if previously enabled

[Gil Fine]

Disable CLx before system suspended only if previously was enabled.

Signed-off-by: Gil Fine <gil.fine@intel.com>
---
 drivers/thunderbolt/switch.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index ac87e8b50e52..549523c9a543 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -3063,8 +3063,10 @@ void tb_switch_suspend(struct tb_switch *sw, bool runtime)
 	 * Actually only needed for Titan Ridge but for simplicity can be
 	 * done for USB4 device too as CLx is re-enabled at resume.
 	 */
-	if (tb_switch_disable_clx(sw, TB_CL0S))
-		tb_sw_warn(sw, "failed to disable CLx on upstream port\n");
+	if (tb_switch_is_clx_enabled(sw)) {
+		if (tb_switch_disable_clx(sw, TB_CL0S))
+			tb_sw_warn(sw, "failed to disable CLx on upstream port\n");
+	}
 
 	err = tb_plug_events_active(sw, false);
 	if (err)
-- 
2.17.1

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH v3 3/6] thunderbolt: Fix typos in CLx enabling

[Gil Fine]

Fix few typos in CLx enabling.

Signed-off-by: Gil Fine <gil.fine@intel.com>
---
 drivers/thunderbolt/switch.c | 2 +-
 drivers/thunderbolt/tmu.c    | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 549523c9a543..42b7daaf9c4d 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -3485,7 +3485,7 @@ static int tb_switch_enable_cl0s(struct tb_switch *sw)
  * to improve performance. CLx is enabled only if both sides of the link
  * support CLx, and if both sides of the link are not configured as two
  * single lane links and only if the link is not inter-domain link. The
- * complete set of conditions is descibed in CM Guide 1.0 section 8.1.
+ * complete set of conditions is described in CM Guide 1.0 section 8.1.
  *
  * Return: Returns 0 on success or an error code on failure.
  */
diff --git a/drivers/thunderbolt/tmu.c b/drivers/thunderbolt/tmu.c
index e4a07a26f693..b656659d02fb 100644
--- a/drivers/thunderbolt/tmu.c
+++ b/drivers/thunderbolt/tmu.c
@@ -606,7 +606,7 @@ int tb_switch_tmu_enable(struct tb_switch *sw)
 /**
  * tb_switch_tmu_configure() - Configure the TMU rate and directionality
  * @sw: Router whose mode to change
- * @rate: Rate to configure Off/LowRes/HiFi
+ * @rate: Rate to configure Off/Normal/HiFi
  * @unidirectional: If uni-directional (bi-directional otherwise)
  *
  * Selects the rate of the TMU and directionality (uni-directional or
-- 
2.17.1

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH v3 4/6] thunderbolt: Change downstream router's TMU rate in both TMU uni/bidir mode

[Gil Fine]

In case of uni-directional time sync, TMU handshake is
initiated by upstream router. In case of bi-directional
time sync, TMU handshake is initiated by downstream router.
In order to handle correctly the case of uni-directional mode,
we avoid changing the upstream router's rate to off,
because it might have another downstream router plugged that is set to
uni-directional mode (and we don't want to change its mode).
Instead, we always change downstream router's rate.

Signed-off-by: Gil Fine <gil.fine@intel.com>
---
 drivers/thunderbolt/tmu.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/thunderbolt/tmu.c b/drivers/thunderbolt/tmu.c
index b656659d02fb..985ca43b8f39 100644
--- a/drivers/thunderbolt/tmu.c
+++ b/drivers/thunderbolt/tmu.c
@@ -359,13 +359,14 @@ int tb_switch_tmu_disable(struct tb_switch *sw)
 		 * In case of uni-directional time sync, TMU handshake is
 		 * initiated by upstream router. In case of bi-directional
 		 * time sync, TMU handshake is initiated by downstream router.
-		 * Therefore, we change the rate to off in the respective
-		 * router.
+		 * We change downstream router's rate to off for both uni/bidir
+		 * cases although it is needed only for the bi-directional mode.
+		 * We avoid changing upstream router's mode since it might
+		 * have another downstream router plugged, that is set to
+		 * uni-directional mode and we don't want to change it's TMU
+		 * mode.
 		 */
-		if (unidirectional)
-			tb_switch_tmu_rate_write(parent, TB_SWITCH_TMU_RATE_OFF);
-		else
-			tb_switch_tmu_rate_write(sw, TB_SWITCH_TMU_RATE_OFF);
+		tb_switch_tmu_rate_write(sw, TB_SWITCH_TMU_RATE_OFF);
 
 		tb_port_tmu_time_sync_disable(up);
 		ret = tb_port_tmu_time_sync_disable(down);
-- 
2.17.1

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH v3 5/6] thunderbolt: Add CL1 support for USB4 and Titan Ridge routers

[Gil Fine]

In this patch we add support for a second low power state of the link: CL1.
Low power states (called collectively CLx) are used to reduce
transmitter and receiver power when a high-speed lane is idle.
We enable it, if both sides of the link support it,
and only for the first hop router (i.e. the first device that connected
to the host router). This is needed for better thermal management.

Signed-off-by: Gil Fine <gil.fine@intel.com>
---
 drivers/thunderbolt/switch.c  |  89 ++++++++---------
 drivers/thunderbolt/tb.c      |  55 ++++++++---
 drivers/thunderbolt/tb.h      |  46 ++++-----
 drivers/thunderbolt/tb_regs.h |   6 ++
 drivers/thunderbolt/tmu.c     | 177 ++++++++++++++++++++++++++++------
 5 files changed, 266 insertions(+), 107 deletions(-)

diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c
index 42b7daaf9c4d..7ce2ac96da23 100644
--- a/drivers/thunderbolt/switch.c
+++ b/drivers/thunderbolt/switch.c
@@ -3062,10 +3062,12 @@ void tb_switch_suspend(struct tb_switch *sw, bool runtime)
 	/*
 	 * Actually only needed for Titan Ridge but for simplicity can be
 	 * done for USB4 device too as CLx is re-enabled at resume.
+	 * CL0s and CL1 are enabled and supported together.
 	 */
-	if (tb_switch_is_clx_enabled(sw)) {
-		if (tb_switch_disable_clx(sw, TB_CL0S))
-			tb_sw_warn(sw, "failed to disable CLx on upstream port\n");
+	if (tb_switch_is_clx_enabled(sw, TB_CL1)) {
+		if (tb_switch_disable_clx(sw, TB_CL1))
+			tb_sw_warn(sw, "failed to disable %s on upstream port\n",
+				   tb_switch_clx_name(TB_CL1));
 	}
 
 	err = tb_plug_events_active(sw, false);
@@ -3357,13 +3359,12 @@ static bool tb_port_clx_supported(struct tb_port *port, enum tb_clx clx)
 	}
 
 	switch (clx) {
-	case TB_CL0S:
-		/* CL0s support requires also CL1 support */
+	case TB_CL1:
+		/* CL0s and CL1 are enabled and supported together */
 		mask = LANE_ADP_CS_0_CL0S_SUPPORT | LANE_ADP_CS_0_CL1_SUPPORT;
 		break;
 
-	/* For now we support only CL0s. Not CL1, CL2 */
-	case TB_CL1:
+	/* For now we support only CL0s and CL1. Not CL2 */
 	case TB_CL2:
 	default:
 		return false;
@@ -3377,18 +3378,18 @@ static bool tb_port_clx_supported(struct tb_port *port, enum tb_clx clx)
 	return !!(val & mask);
 }
 
-static inline bool tb_port_cl0s_supported(struct tb_port *port)
-{
-	return tb_port_clx_supported(port, TB_CL0S);
-}
-
-static int __tb_port_cl0s_set(struct tb_port *port, bool enable)
+static int __tb_port_clx_set(struct tb_port *port, enum tb_clx clx, bool enable)
 {
 	u32 phy, mask;
 	int ret;
 
-	/* To enable CL0s also required to enable CL1 */
-	mask = LANE_ADP_CS_1_CL0S_ENABLE | LANE_ADP_CS_1_CL1_ENABLE;
+	/* CL0s and CL1 are enabled and supported together */
+	if (clx == TB_CL1)
+		mask = LANE_ADP_CS_1_CL0S_ENABLE | LANE_ADP_CS_1_CL1_ENABLE;
+	else
+		/* For now we support only CL0s and CL1. Not CL2 */
+		return -EOPNOTSUPP;
+
 	ret = tb_port_read(port, &phy, TB_CFG_PORT,
 			   port->cap_phy + LANE_ADP_CS_1, 1);
 	if (ret)
@@ -3403,20 +3404,20 @@ static int __tb_port_cl0s_set(struct tb_port *port, bool enable)
 			     port->cap_phy + LANE_ADP_CS_1, 1);
 }
 
-static int tb_port_cl0s_disable(struct tb_port *port)
+static int tb_port_clx_disable(struct tb_port *port, enum tb_clx clx)
 {
-	return __tb_port_cl0s_set(port, false);
+	return __tb_port_clx_set(port, clx, false);
 }
 
-static int tb_port_cl0s_enable(struct tb_port *port)
+static int tb_port_clx_enable(struct tb_port *port, enum tb_clx clx)
 {
-	return __tb_port_cl0s_set(port, true);
+	return __tb_port_clx_set(port, clx, true);
 }
 
-static int tb_switch_enable_cl0s(struct tb_switch *sw)
+static int __tb_switch_enable_clx(struct tb_switch *sw, enum tb_clx clx)
 {
 	struct tb_switch *parent = tb_switch_parent(sw);
-	bool up_cl0s_support, down_cl0s_support;
+	bool up_clx_support, down_clx_support;
 	struct tb_port *up, *down;
 	int ret;
 
@@ -3441,37 +3442,37 @@ static int tb_switch_enable_cl0s(struct tb_switch *sw)
 	up = tb_upstream_port(sw);
 	down = tb_port_at(tb_route(sw), parent);
 
-	up_cl0s_support = tb_port_cl0s_supported(up);
-	down_cl0s_support = tb_port_cl0s_supported(down);
+	up_clx_support = tb_port_clx_supported(up, clx);
+	down_clx_support = tb_port_clx_supported(down, clx);
 
-	tb_port_dbg(up, "CL0s %ssupported\n",
-		    up_cl0s_support ? "" : "not ");
-	tb_port_dbg(down, "CL0s %ssupported\n",
-		    down_cl0s_support ? "" : "not ");
+	tb_port_dbg(up, "%s %ssupported\n", tb_switch_clx_name(clx),
+		    up_clx_support ? "" : "not ");
+	tb_port_dbg(down, "%s %ssupported\n", tb_switch_clx_name(clx),
+		    down_clx_support ? "" : "not ");
 
-	if (!up_cl0s_support || !down_cl0s_support)
+	if (!up_clx_support || !down_clx_support)
 		return -EOPNOTSUPP;
 
-	ret = tb_port_cl0s_enable(up);
+	ret = tb_port_clx_enable(up, clx);
 	if (ret)
 		return ret;
 
-	ret = tb_port_cl0s_enable(down);
+	ret = tb_port_clx_enable(down, clx);
 	if (ret) {
-		tb_port_cl0s_disable(up);
+		tb_port_clx_disable(up, clx);
 		return ret;
 	}
 
 	ret = tb_switch_mask_clx_objections(sw);
 	if (ret) {
-		tb_port_cl0s_disable(up);
-		tb_port_cl0s_disable(down);
+		tb_port_clx_disable(up, clx);
+		tb_port_clx_disable(down, clx);
 		return ret;
 	}
 
-	sw->clx = TB_CL0S;
+	sw->clx = clx;
 
-	tb_port_dbg(up, "CL0s enabled\n");
+	tb_port_dbg(up, "%s enabled\n", tb_switch_clx_name(clx));
 	return 0;
 }
 
@@ -3504,15 +3505,16 @@ int tb_switch_enable_clx(struct tb_switch *sw, enum tb_clx clx)
 		return 0;
 
 	switch (clx) {
-	case TB_CL0S:
-		return tb_switch_enable_cl0s(sw);
+	case TB_CL1:
+		/* CL0s and CL1 are enabled and supported together */
+		return __tb_switch_enable_clx(sw, clx);
 
 	default:
 		return -EOPNOTSUPP;
 	}
 }
 
-static int tb_switch_disable_cl0s(struct tb_switch *sw)
+static int __tb_switch_disable_clx(struct tb_switch *sw, enum tb_clx clx)
 {
 	struct tb_switch *parent = tb_switch_parent(sw);
 	struct tb_port *up, *down;
@@ -3534,17 +3536,17 @@ static int tb_switch_disable_cl0s(struct tb_switch *sw)
 
 	up = tb_upstream_port(sw);
 	down = tb_port_at(tb_route(sw), parent);
-	ret = tb_port_cl0s_disable(up);
+	ret = tb_port_clx_disable(up, clx);
 	if (ret)
 		return ret;
 
-	ret = tb_port_cl0s_disable(down);
+	ret = tb_port_clx_disable(down, clx);
 	if (ret)
 		return ret;
 
 	sw->clx = TB_CLX_DISABLE;
 
-	tb_port_dbg(up, "CL0s disabled\n");
+	tb_port_dbg(up, "%s disabled\n", tb_switch_clx_name(clx));
 	return 0;
 }
 
@@ -3561,8 +3563,9 @@ int tb_switch_disable_clx(struct tb_switch *sw, enum tb_clx clx)
 		return 0;
 
 	switch (clx) {
-	case TB_CL0S:
-		return tb_switch_disable_cl0s(sw);
+	case TB_CL1:
+		/* CL0s and CL1 are enabled and supported together */
+		return __tb_switch_disable_clx(sw, clx);
 
 	default:
 		return -EOPNOTSUPP;
diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index 7419cd1aefba..f512197e719b 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -221,7 +221,7 @@ static int tb_enable_tmu(struct tb_switch *sw)
 	int ret;
 
 	/* If it is already enabled in correct mode, don't touch it */
-	if (tb_switch_tmu_hifi_is_enabled(sw, sw->tmu.unidirectional_request))
+	if (tb_switch_tmu_is_enabled(sw, sw->tmu.unidirectional_request))
 		return 0;
 
 	ret = tb_switch_tmu_disable(sw);
@@ -670,13 +670,24 @@ static void tb_scan_port(struct tb_port *port)
 	tb_switch_lane_bonding_enable(sw);
 	/* Set the link configured */
 	tb_switch_configure_link(sw);
-	/* Silently ignore CLx enabling in case CLx is not supported */
-	ret = tb_switch_enable_clx(sw, TB_CL0S);
+	/*
+	 * CL0s and CL1 are enabled and supported together.
+	 * Silently ignore CLx enabling in case CLx is not supported.
+	 */
+	ret = tb_switch_enable_clx(sw, TB_CL1);
 	if (ret && ret != -EOPNOTSUPP)
-		tb_sw_warn(sw, "failed to enable CLx on upstream port\n");
+		tb_sw_warn(sw, "failed to enable %s on upstream port\n",
+			   tb_switch_clx_name(TB_CL1));
 
-	tb_switch_tmu_configure(sw, TB_SWITCH_TMU_RATE_HIFI,
-				tb_switch_is_clx_enabled(sw));
+	if (tb_switch_is_clx_enabled(sw, TB_CL1))
+		/*
+		 * To support highest CLx state, we set router's TMU to
+		 * Normal-Uni mode.
+		 */
+		tb_switch_tmu_configure(sw, TB_SWITCH_TMU_RATE_NORMAL, true);
+	else
+		/* If CLx disabled, configure router's TMU to HiFi-Bidir mode*/
+		tb_switch_tmu_configure(sw, TB_SWITCH_TMU_RATE_HIFI, false);
 
 	if (tb_enable_tmu(sw))
 		tb_sw_warn(sw, "failed to enable TMU\n");
@@ -1416,7 +1427,12 @@ static int tb_start(struct tb *tb)
 		return ret;
 	}
 
-	tb_switch_tmu_configure(tb->root_switch, TB_SWITCH_TMU_RATE_HIFI, false);
+	/*
+	 * To support highest CLx state, we set host router's TMU to
+	 * Normal mode.
+	 */
+	tb_switch_tmu_configure(tb->root_switch, TB_SWITCH_TMU_RATE_NORMAL,
+				false);
 	/* Enable TMU if it is off */
 	tb_switch_tmu_enable(tb->root_switch);
 	/* Full scan to discover devices added before the driver was loaded. */
@@ -1461,16 +1477,25 @@ static void tb_restore_children(struct tb_switch *sw)
 	if (sw->is_unplugged)
 		return;
 
-	/* Silently ignore CLx re-enabling in case CLx is not supported */
-	ret = tb_switch_enable_clx(sw, TB_CL0S);
-	if (ret && ret != -EOPNOTSUPP)
-		tb_sw_warn(sw, "failed to re-enable CLx on upstream port\n");
-
 	/*
-	 * tb_switch_tmu_configure() was already called when the switch was
-	 * added before entering system sleep or runtime suspend,
-	 * so no need to call it again before enabling TMU.
+	 * CL0s and CL1 are enabled and supported together.
+	 * Silently ignore CLx re-enabling in case CLx is not supported.
 	 */
+	ret = tb_switch_enable_clx(sw, TB_CL1);
+	if (ret && ret != -EOPNOTSUPP)
+		tb_sw_warn(sw, "failed to re-enable %s on upstream port\n",
+			   tb_switch_clx_name(TB_CL1));
+
+	if (tb_switch_is_clx_enabled(sw, TB_CL1))
+		/*
+		 * To support highest CLx state, we set router's TMU to
+		 * Normal-Uni mode.
+		 */
+		tb_switch_tmu_configure(sw, TB_SWITCH_TMU_RATE_NORMAL, true);
+	else
+		/* If CLx disabled, configure router's TMU to HiFi-Bidir mode*/
+		tb_switch_tmu_configure(sw, TB_SWITCH_TMU_RATE_HIFI, false);
+
 	if (tb_enable_tmu(sw))
 		tb_sw_warn(sw, "failed to restore TMU configuration\n");
 
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index ad025ff142ba..a16fffba9dd2 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -13,6 +13,7 @@
 #include <linux/pci.h>
 #include <linux/thunderbolt.h>
 #include <linux/uuid.h>
+#include <linux/bitfield.h>
 
 #include "tb_regs.h"
 #include "ctl.h"
@@ -111,7 +112,7 @@ struct tb_switch_tmu {
 
 enum tb_clx {
 	TB_CLX_DISABLE,
-	TB_CL0S,
+	/* CL0s and CL1 are enabled and supported together */
 	TB_CL1,
 	TB_CL2,
 };
@@ -934,45 +935,46 @@ void tb_switch_tmu_configure(struct tb_switch *sw,
 			     enum tb_switch_tmu_rate rate,
 			     bool unidirectional);
 /**
- * tb_switch_tmu_hifi_is_enabled() - Checks if the specified TMU mode is enabled
+ * tb_switch_tmu_is_enabled() - Checks if the specified TMU mode is enabled
  * @sw: Router whose TMU mode to check
  * @unidirectional: If uni-directional (bi-directional otherwise)
  *
  * Return true if hardware TMU configuration matches the one passed in
- * as parameter. That is HiFi and either uni-directional or bi-directional.
+ * as parameter. That is HiFi/Normal and either uni-directional or bi-directional.
  */
-static inline bool tb_switch_tmu_hifi_is_enabled(const struct tb_switch *sw,
-						 bool unidirectional)
+static inline bool tb_switch_tmu_is_enabled(const struct tb_switch *sw,
+					    bool unidirectional)
 {
-	return sw->tmu.rate == TB_SWITCH_TMU_RATE_HIFI &&
+	return sw->tmu.rate == sw->tmu.rate_request &&
 	       sw->tmu.unidirectional == unidirectional;
 }
 
+static inline const char *tb_switch_clx_name(enum tb_clx clx)
+{
+	switch (clx) {
+	/* CL0s and CL1 are enabled and supported together */
+	case TB_CL1:
+		return "CL0s/CL1";
+	default:
+		return "Unknown";
+	}
+}
+
 int tb_switch_enable_clx(struct tb_switch *sw, enum tb_clx clx);
 int tb_switch_disable_clx(struct tb_switch *sw, enum tb_clx clx);
 
 /**
  * tb_switch_is_clx_enabled() - Checks if the CLx is enabled
- * @sw: Router to check the CLx state for
- *
- * Checks if the CLx is enabled on the router upstream link.
- * Not applicable for a host router.
- */
-static inline bool tb_switch_is_clx_enabled(const struct tb_switch *sw)
-{
-	return sw->clx != TB_CLX_DISABLE;
-}
-
-/**
- * tb_switch_is_cl0s_enabled() - Checks if the CL0s is enabled
- * @sw: Router to check for the CL0s
+ * @sw: Router to check for the CLx
+ * @clx: The CLx state to check for
  *
- * Checks if the CL0s is enabled on the router upstream link.
+ * Checks if the specified CLx is enabled on the router upstream link.
  * Not applicable for a host router.
  */
-static inline bool tb_switch_is_cl0s_enabled(const struct tb_switch *sw)
+static inline bool tb_switch_is_clx_enabled(const struct tb_switch *sw,
+					    enum tb_clx clx)
 {
-	return sw->clx == TB_CL0S;
+	return sw->clx == clx;
 }
 
 /**
diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h
index b301eeb0c89b..dd85aac5ced3 100644
--- a/drivers/thunderbolt/tb_regs.h
+++ b/drivers/thunderbolt/tb_regs.h
@@ -234,6 +234,7 @@ enum usb4_switch_op {
 
 /* Router TMU configuration */
 #define TMU_RTR_CS_0				0x00
+#define TMU_RTR_CS_0_FREQ_WIND_MASK		GENMASK(26, 16)
 #define TMU_RTR_CS_0_TD				BIT(27)
 #define TMU_RTR_CS_0_UCAP			BIT(30)
 #define TMU_RTR_CS_1				0x01
@@ -244,6 +245,11 @@ enum usb4_switch_op {
 #define TMU_RTR_CS_3_LOCAL_TIME_NS_MASK		GENMASK(15, 0)
 #define TMU_RTR_CS_3_TS_PACKET_INTERVAL_MASK	GENMASK(31, 16)
 #define TMU_RTR_CS_3_TS_PACKET_INTERVAL_SHIFT	16
+#define TMU_RTR_CS_15				0xf
+#define TMU_RTR_CS_15_FREQ_AVG_MASK		GENMASK(5, 0)
+#define TMU_RTR_CS_15_DELAY_AVG_MASK		GENMASK(11, 6)
+#define TMU_RTR_CS_15_OFFSET_AVG_MASK		GENMASK(17, 12)
+#define TMU_RTR_CS_15_ERROR_AVG_MASK		GENMASK(23, 18)
 #define TMU_RTR_CS_22				0x16
 #define TMU_RTR_CS_24				0x18
 #define TMU_RTR_CS_25				0x19
diff --git a/drivers/thunderbolt/tmu.c b/drivers/thunderbolt/tmu.c
index 985ca43b8f39..e822ab90338b 100644
--- a/drivers/thunderbolt/tmu.c
+++ b/drivers/thunderbolt/tmu.c
@@ -11,6 +11,55 @@
 
 #include "tb.h"
 
+static int tb_switch_set_tmu_mode_params(struct tb_switch *sw,
+					 enum tb_switch_tmu_rate rate)
+{
+	u32 freq_meas_wind[2] = { 30, 800 };
+	u32 avg_const[2] = { 4, 8 };
+	u32 freq, avg, val;
+	int ret;
+
+	if (rate == TB_SWITCH_TMU_RATE_NORMAL) {
+		freq = freq_meas_wind[0];
+		avg = avg_const[0];
+	} else if (rate == TB_SWITCH_TMU_RATE_HIFI) {
+		freq = freq_meas_wind[1];
+		avg = avg_const[1];
+	} else {
+		return 0;
+	}
+
+	ret = tb_sw_read(sw, &val, TB_CFG_SWITCH,
+			 sw->tmu.cap + TMU_RTR_CS_0, 1);
+	if (ret)
+		return ret;
+
+	val &= ~TMU_RTR_CS_0_FREQ_WIND_MASK;
+	val |= FIELD_PREP(TMU_RTR_CS_0_FREQ_WIND_MASK, freq);
+
+	ret = tb_sw_write(sw, &val, TB_CFG_SWITCH,
+			  sw->tmu.cap + TMU_RTR_CS_0, 1);
+	if (ret)
+		return ret;
+
+	ret = tb_sw_read(sw, &val, TB_CFG_SWITCH,
+			 sw->tmu.cap + TMU_RTR_CS_15, 1);
+	if (ret)
+		return ret;
+
+	val &= ~TMU_RTR_CS_15_FREQ_AVG_MASK &
+		~TMU_RTR_CS_15_DELAY_AVG_MASK &
+		~TMU_RTR_CS_15_OFFSET_AVG_MASK &
+		~TMU_RTR_CS_15_ERROR_AVG_MASK;
+	val |=  FIELD_PREP(TMU_RTR_CS_15_FREQ_AVG_MASK, avg) |
+		FIELD_PREP(TMU_RTR_CS_15_DELAY_AVG_MASK, avg) |
+		FIELD_PREP(TMU_RTR_CS_15_OFFSET_AVG_MASK, avg) |
+		FIELD_PREP(TMU_RTR_CS_15_ERROR_AVG_MASK, avg);
+
+	return tb_sw_write(sw, &val, TB_CFG_SWITCH,
+			   sw->tmu.cap + TMU_RTR_CS_15, 1);
+}
+
 static const char *tb_switch_tmu_mode_name(const struct tb_switch *sw)
 {
 	bool root_switch = !tb_route(sw);
@@ -348,7 +397,7 @@ int tb_switch_tmu_disable(struct tb_switch *sw)
 
 
 	if (tb_route(sw)) {
-		bool unidirectional = tb_switch_tmu_hifi_is_enabled(sw, true);
+		bool unidirectional = sw->tmu.unidirectional;
 		struct tb_switch *parent = tb_switch_parent(sw);
 		struct tb_port *down, *up;
 		int ret;
@@ -412,6 +461,7 @@ static void __tb_switch_tmu_off(struct tb_switch *sw, bool unidirectional)
 	else
 		tb_switch_tmu_rate_write(sw, TB_SWITCH_TMU_RATE_OFF);
 
+	tb_switch_set_tmu_mode_params(sw, sw->tmu.rate);
 	tb_port_tmu_unidirectional_disable(down);
 	tb_port_tmu_unidirectional_disable(up);
 }
@@ -493,7 +543,11 @@ static int __tb_switch_tmu_enable_unidirectional(struct tb_switch *sw)
 
 	up = tb_upstream_port(sw);
 	down = tb_port_at(tb_route(sw), parent);
-	ret = tb_switch_tmu_rate_write(parent, TB_SWITCH_TMU_RATE_HIFI);
+	ret = tb_switch_tmu_rate_write(parent, sw->tmu.rate_request);
+	if (ret)
+		return ret;
+
+	ret = tb_switch_set_tmu_mode_params(sw, sw->tmu.rate_request);
 	if (ret)
 		return ret;
 
@@ -520,7 +574,83 @@ static int __tb_switch_tmu_enable_unidirectional(struct tb_switch *sw)
 	return ret;
 }
 
-static int tb_switch_tmu_hifi_enable(struct tb_switch *sw)
+static void __tb_switch_tmu_change_mode_prev(struct tb_switch *sw)
+{
+	struct tb_switch *parent = tb_switch_parent(sw);
+	struct tb_port *down, *up;
+
+	down = tb_port_at(tb_route(sw), parent);
+	up = tb_upstream_port(sw);
+	/*
+	 * In case of any failure in one of the steps when change mode,
+	 * get back to the TMU configurations in previous mode.
+	 * In case of additional failures in the functions below,
+	 * ignore them since the caller shall already report a failure.
+	 */
+	tb_port_tmu_set_unidirectional(down, sw->tmu.unidirectional);
+	if (sw->tmu.unidirectional_request)
+		tb_switch_tmu_rate_write(parent, sw->tmu.rate);
+	else
+		tb_switch_tmu_rate_write(sw, sw->tmu.rate);
+
+	tb_switch_set_tmu_mode_params(sw, sw->tmu.rate);
+	tb_port_tmu_set_unidirectional(up, sw->tmu.unidirectional);
+}
+
+static int __tb_switch_tmu_change_mode(struct tb_switch *sw)
+{
+	struct tb_switch *parent = tb_switch_parent(sw);
+	struct tb_port *up, *down;
+	int ret;
+
+	up = tb_upstream_port(sw);
+	down = tb_port_at(tb_route(sw), parent);
+	ret = tb_port_tmu_set_unidirectional(down, sw->tmu.unidirectional_request);
+	if (ret)
+		goto out;
+
+	if (sw->tmu.unidirectional_request)
+		ret = tb_switch_tmu_rate_write(parent, sw->tmu.rate_request);
+	else
+		ret = tb_switch_tmu_rate_write(sw, sw->tmu.rate_request);
+	if (ret)
+		return ret;
+
+	ret = tb_switch_set_tmu_mode_params(sw, sw->tmu.rate_request);
+	if (ret)
+		return ret;
+
+	ret = tb_port_tmu_set_unidirectional(up, sw->tmu.unidirectional_request);
+	if (ret)
+		goto out;
+
+	ret = tb_port_tmu_time_sync_enable(down);
+	if (ret)
+		goto out;
+
+	ret = tb_port_tmu_time_sync_enable(up);
+	if (ret)
+		goto out;
+
+	return 0;
+
+out:
+	__tb_switch_tmu_change_mode_prev(sw);
+	return ret;
+}
+
+/**
+ * tb_switch_tmu_enable() - Enable TMU on a router
+ * @sw: Router whose TMU to enable
+ *
+ * Enables TMU of a router to be in uni-directional Normal/HiFi
+ * or bi-directional HiFi mode. Calling tb_switch_tmu_configure() is required
+ * before calling this function, to select the mode Normal/HiFi and
+ * directionality (uni-directional/bi-directional).
+ * In HiFi mode all tunneling should work. In Normal mode, DP tunneling can't
+ * work. Uni-directional mode is required for CLx (Link Low-Power) to work.
+ */
+int tb_switch_tmu_enable(struct tb_switch *sw)
 {
 	bool unidirectional = sw->tmu.unidirectional_request;
 	int ret;
@@ -536,12 +666,15 @@ static int tb_switch_tmu_hifi_enable(struct tb_switch *sw)
 	if (!tb_switch_is_clx_supported(sw))
 		return 0;
 
-	if (tb_switch_tmu_hifi_is_enabled(sw, sw->tmu.unidirectional_request))
+	if (tb_switch_tmu_is_enabled(sw, sw->tmu.unidirectional_request))
 		return 0;
 
 	if (tb_switch_is_titan_ridge(sw) && unidirectional) {
-		/* Titan Ridge supports only CL0s */
-		if (!tb_switch_is_cl0s_enabled(sw))
+		/*
+		 * Titan Ridge supports CL0s and CL1 only. CL0s and CL1 are
+		 * enabled and supported together.
+		 */
+		if (!tb_switch_is_clx_enabled(sw, TB_CL1))
 			return -EOPNOTSUPP;
 
 		ret = tb_switch_tmu_objection_mask(sw);
@@ -558,7 +691,11 @@ static int tb_switch_tmu_hifi_enable(struct tb_switch *sw)
 		return ret;
 
 	if (tb_route(sw)) {
-		/* The used mode changes are from OFF to HiFi-Uni/HiFi-BiDir */
+		/*
+		 * The used mode changes are from OFF to
+		 * HiFi-Uni/HiFi-BiDir/Normal-Uni or from Normal-Uni to
+		 * HiFi-Uni.
+		 */
 		if (sw->tmu.rate == TB_SWITCH_TMU_RATE_OFF) {
 			if (unidirectional)
 				ret = __tb_switch_tmu_enable_unidirectional(sw);
@@ -566,6 +703,10 @@ static int tb_switch_tmu_hifi_enable(struct tb_switch *sw)
 				ret = __tb_switch_tmu_enable_bidirectional(sw);
 			if (ret)
 				return ret;
+		} else if (sw->tmu.rate == TB_SWITCH_TMU_RATE_NORMAL) {
+			ret = __tb_switch_tmu_change_mode(sw);
+			if (ret)
+				return ret;
 		}
 		sw->tmu.unidirectional = unidirectional;
 	} else {
@@ -575,35 +716,17 @@ static int tb_switch_tmu_hifi_enable(struct tb_switch *sw)
 		 * of the child node - see above.
 		 * Here only the host router' rate configuration is written.
 		 */
-		ret = tb_switch_tmu_rate_write(sw, TB_SWITCH_TMU_RATE_HIFI);
+		ret = tb_switch_tmu_rate_write(sw, sw->tmu.rate_request);
 		if (ret)
 			return ret;
 	}
 
-	sw->tmu.rate = TB_SWITCH_TMU_RATE_HIFI;
+	sw->tmu.rate = sw->tmu.rate_request;
 
 	tb_sw_dbg(sw, "TMU: mode set to: %s\n", tb_switch_tmu_mode_name(sw));
 	return tb_switch_tmu_set_time_disruption(sw, false);
 }
 
-/**
- * tb_switch_tmu_enable() - Enable TMU on a router
- * @sw: Router whose TMU to enable
- *
- * Enables TMU of a router to be in uni-directional or bi-directional HiFi mode.
- * Calling tb_switch_tmu_configure() is required before calling this function,
- * to select the mode HiFi and directionality (uni-directional/bi-directional).
- * In both modes all tunneling should work. Uni-directional mode is required for
- * CLx (Link Low-Power) to work.
- */
-int tb_switch_tmu_enable(struct tb_switch *sw)
-{
-	if (sw->tmu.rate_request == TB_SWITCH_TMU_RATE_NORMAL)
-		return -EOPNOTSUPP;
-
-	return tb_switch_tmu_hifi_enable(sw);
-}
-
 /**
  * tb_switch_tmu_configure() - Configure the TMU rate and directionality
  * @sw: Router whose mode to change
-- 
2.17.1

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

[PATCH v3 6/6] thunderbolt: Change TMU mode to HiFi uni-directional once DisplayPort tunneled

[Gil Fine]

Here we configure TMU mode to HiFi uni-directional once DP tunnel
is created. This is due to accuracy requirement for DP tunneling
as appears in CM guide 1.0, section 7.3.2.
Due to Intel hardware limitation, once we changed the TMU mode to HiFi
uni-directional (when DP tunnel exists), we don't change TMU mode back to
normal uni-directional, even if DP tunnel is torn down later.

Signed-off-by: Gil Fine <gil.fine@intel.com>
---
 drivers/thunderbolt/tb.c  | 28 ++++++++++++++++++++++++++++
 drivers/thunderbolt/tb.h  |  5 +++++
 drivers/thunderbolt/tmu.c | 14 ++++++++++++++
 3 files changed, 47 insertions(+)

diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
index f512197e719b..d0f85a8c56de 100644
--- a/drivers/thunderbolt/tb.c
+++ b/drivers/thunderbolt/tb.c
@@ -50,6 +50,8 @@ struct tb_hotplug_event {
 };
 
 static void tb_handle_hotplug(struct work_struct *work);
+static void tb_enable_tmu_1st_child(struct tb *tb,
+				    enum tb_switch_tmu_rate rate);
 
 static void tb_queue_hotplug(struct tb *tb, u64 route, u8 port, bool unplug)
 {
@@ -118,6 +120,13 @@ static void tb_switch_discover_tunnels(struct tb_switch *sw,
 		switch (port->config.type) {
 		case TB_TYPE_DP_HDMI_IN:
 			tunnel = tb_tunnel_discover_dp(tb, port, alloc_hopids);
+			/*
+			 * In case of DP tunnel exists, change TMU mode to
+			 * HiFi for CL0s to work.
+			 */
+			if (tunnel)
+				tb_enable_tmu_1st_child(tb,
+						TB_SWITCH_TMU_RATE_HIFI);
 			break;
 
 		case TB_TYPE_PCIE_DOWN:
@@ -235,6 +244,19 @@ static int tb_enable_tmu(struct tb_switch *sw)
 	return tb_switch_tmu_enable(sw);
 }
 
+/*
+ * Once a DP tunnel exists in the domain, we set the TMU mode so that
+ * it meets the accuracy requirements and also enables CLx entry (CL0s).
+ * We set the TMU mode of the first depth router(s) for CL0s to work.
+ */
+static void tb_enable_tmu_1st_child(struct tb *tb, enum tb_switch_tmu_rate rate)
+{
+	struct tb_sw_tmu_config tmu = { .rate = rate };
+
+	device_for_each_child(&tb->root_switch->dev, &tmu,
+			      tb_switch_tmu_config_enable);
+}
+
 /**
  * tb_find_unused_port() - return the first inactive port on @sw
  * @sw: Switch to find the port on
@@ -985,6 +1007,12 @@ static void tb_tunnel_dp(struct tb *tb)
 
 	list_add_tail(&tunnel->list, &tcm->tunnel_list);
 	tb_reclaim_usb3_bandwidth(tb, in, out);
+	/*
+	 * In case of DP tunnel exists, change TMU mode to
+	 * HiFi for CL0s to work.
+	 */
+	tb_enable_tmu_1st_child(tb, TB_SWITCH_TMU_RATE_HIFI);
+
 	return;
 
 err_free:
diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
index a16fffba9dd2..3dbd9d919d5f 100644
--- a/drivers/thunderbolt/tb.h
+++ b/drivers/thunderbolt/tb.h
@@ -110,6 +110,10 @@ struct tb_switch_tmu {
 	enum tb_switch_tmu_rate rate_request;
 };
 
+struct tb_sw_tmu_config {
+	enum tb_switch_tmu_rate rate;
+};
+
 enum tb_clx {
 	TB_CLX_DISABLE,
 	/* CL0s and CL1 are enabled and supported together */
@@ -934,6 +938,7 @@ int tb_switch_tmu_enable(struct tb_switch *sw);
 void tb_switch_tmu_configure(struct tb_switch *sw,
 			     enum tb_switch_tmu_rate rate,
 			     bool unidirectional);
+int tb_switch_tmu_config_enable(struct device *dev, void *data);
 /**
  * tb_switch_tmu_is_enabled() - Checks if the specified TMU mode is enabled
  * @sw: Router whose TMU mode to check
diff --git a/drivers/thunderbolt/tmu.c b/drivers/thunderbolt/tmu.c
index e822ab90338b..b8ff9f64a71e 100644
--- a/drivers/thunderbolt/tmu.c
+++ b/drivers/thunderbolt/tmu.c
@@ -727,6 +727,20 @@ int tb_switch_tmu_enable(struct tb_switch *sw)
 	return tb_switch_tmu_set_time_disruption(sw, false);
 }
 
+int tb_switch_tmu_config_enable(struct device *dev, void *data)
+{
+	if (tb_is_switch(dev)) {
+		struct tb_switch *sw = tb_to_switch(dev);
+		struct tb_sw_tmu_config *tmu = data;
+
+		tb_switch_tmu_configure(sw, tmu->rate, tb_switch_is_clx_enabled(sw, TB_CL1));
+		if (tb_switch_tmu_enable(sw))
+			tb_sw_dbg(sw, "Fail switching TMU to HiFi for 1st depth router\n");
+	}
+
+	return 0;
+}
+
 /**
  * tb_switch_tmu_configure() - Configure the TMU rate and directionality
  * @sw: Router whose mode to change
-- 
2.17.1

---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH v3 6/6] thunderbolt: Change TMU mode to HiFi uni-directional once DisplayPort tunneled

[Mika Westerberg]

Hi Gil,

On Wed, May 11, 2022 at 05:05:49PM +0300, Gil Fine wrote:
> Here we configure TMU mode to HiFi uni-directional once DP tunnel
> is created. This is due to accuracy requirement for DP tunneling
> as appears in CM guide 1.0, section 7.3.2.
> Due to Intel hardware limitation, once we changed the TMU mode to HiFi
> uni-directional (when DP tunnel exists), we don't change TMU mode back to
> normal uni-directional, even if DP tunnel is torn down later.
> 
> Signed-off-by: Gil Fine <gil.fine@intel.com>
> ---
>  drivers/thunderbolt/tb.c  | 28 ++++++++++++++++++++++++++++
>  drivers/thunderbolt/tb.h  |  5 +++++
>  drivers/thunderbolt/tmu.c | 14 ++++++++++++++
>  3 files changed, 47 insertions(+)
> 
> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index f512197e719b..d0f85a8c56de 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
> @@ -50,6 +50,8 @@ struct tb_hotplug_event {
>  };
>  
>  static void tb_handle_hotplug(struct work_struct *work);
> +static void tb_enable_tmu_1st_child(struct tb *tb,
> +				    enum tb_switch_tmu_rate rate);

This forward declaration is not needed. You can just move the
implementation before you call it first time.

>  static void tb_queue_hotplug(struct tb *tb, u64 route, u8 port, bool unplug)
>  {
> @@ -118,6 +120,13 @@ static void tb_switch_discover_tunnels(struct tb_switch *sw,
>  		switch (port->config.type) {
>  		case TB_TYPE_DP_HDMI_IN:
>  			tunnel = tb_tunnel_discover_dp(tb, port, alloc_hopids);
> +			/*
> +			 * In case of DP tunnel exists, change TMU mode to
> +			 * HiFi for CL0s to work.
> +			 */
> +			if (tunnel)
> +				tb_enable_tmu_1st_child(tb,
> +						TB_SWITCH_TMU_RATE_HIFI);
>  			break;
>  
>  		case TB_TYPE_PCIE_DOWN:
> @@ -235,6 +244,19 @@ static int tb_enable_tmu(struct tb_switch *sw)
>  	return tb_switch_tmu_enable(sw);
>  }
>  
> +/*
> + * Once a DP tunnel exists in the domain, we set the TMU mode so that
> + * it meets the accuracy requirements and also enables CLx entry (CL0s).
> + * We set the TMU mode of the first depth router(s) for CL0s to work.
> + */
> +static void tb_enable_tmu_1st_child(struct tb *tb, enum tb_switch_tmu_rate rate)
> +{
> +	struct tb_sw_tmu_config tmu = { .rate = rate };
> +
> +	device_for_each_child(&tb->root_switch->dev, &tmu,
> +			      tb_switch_tmu_config_enable);
> +}
> +
>  /**
>   * tb_find_unused_port() - return the first inactive port on @sw
>   * @sw: Switch to find the port on
> @@ -985,6 +1007,12 @@ static void tb_tunnel_dp(struct tb *tb)
>  
>  	list_add_tail(&tunnel->list, &tcm->tunnel_list);
>  	tb_reclaim_usb3_bandwidth(tb, in, out);
> +	/*
> +	 * In case of DP tunnel exists, change TMU mode to
> +	 * HiFi for CL0s to work.
> +	 */
> +	tb_enable_tmu_1st_child(tb, TB_SWITCH_TMU_RATE_HIFI);
> +
>  	return;
>  
>  err_free:
> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> index a16fffba9dd2..3dbd9d919d5f 100644
> --- a/drivers/thunderbolt/tb.h
> +++ b/drivers/thunderbolt/tb.h
> @@ -110,6 +110,10 @@ struct tb_switch_tmu {
>  	enum tb_switch_tmu_rate rate_request;
>  };
>  
> +struct tb_sw_tmu_config {
> +	enum tb_switch_tmu_rate rate;
> +};

Is this wrapper structure really needed?

> +
>  enum tb_clx {
>  	TB_CLX_DISABLE,
>  	/* CL0s and CL1 are enabled and supported together */
> @@ -934,6 +938,7 @@ int tb_switch_tmu_enable(struct tb_switch *sw);
>  void tb_switch_tmu_configure(struct tb_switch *sw,
>  			     enum tb_switch_tmu_rate rate,
>  			     bool unidirectional);
> +int tb_switch_tmu_config_enable(struct device *dev, void *data);
>  /**
>   * tb_switch_tmu_is_enabled() - Checks if the specified TMU mode is enabled
>   * @sw: Router whose TMU mode to check
> diff --git a/drivers/thunderbolt/tmu.c b/drivers/thunderbolt/tmu.c
> index e822ab90338b..b8ff9f64a71e 100644
> --- a/drivers/thunderbolt/tmu.c
> +++ b/drivers/thunderbolt/tmu.c
> @@ -727,6 +727,20 @@ int tb_switch_tmu_enable(struct tb_switch *sw)
>  	return tb_switch_tmu_set_time_disruption(sw, false);
>  }

You are missing kernel-doc for the non-static function.

>  
> +int tb_switch_tmu_config_enable(struct device *dev, void *data)

Also can we please make it take some real type and not something
arbitrary?

Can it be const too?

> +{
> +	if (tb_is_switch(dev)) {
> +		struct tb_switch *sw = tb_to_switch(dev);
> +		struct tb_sw_tmu_config *tmu = data;
> +
> +		tb_switch_tmu_configure(sw, tmu->rate, tb_switch_is_clx_enabled(sw, TB_CL1));
> +		if (tb_switch_tmu_enable(sw))
> +			tb_sw_dbg(sw, "Fail switching TMU to HiFi for 1st depth router\n");

Be consistent with the messaging so don't start with capital letter
here.

> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * tb_switch_tmu_configure() - Configure the TMU rate and directionality
>   * @sw: Router whose mode to change
> -- 
> 2.17.1

Re: [PATCH v3 6/6] thunderbolt: Change TMU mode to HiFi uni-directional once DisplayPort tunneled

[Gil Fine]

Hi Mika,

On Fri, May 13, 2022 at 12:53:25PM +0300, Mika Westerberg wrote:
> Hi Gil,
> 
> On Wed, May 11, 2022 at 05:05:49PM +0300, Gil Fine wrote:
> > Here we configure TMU mode to HiFi uni-directional once DP tunnel
> > is created. This is due to accuracy requirement for DP tunneling
> > as appears in CM guide 1.0, section 7.3.2.
> > Due to Intel hardware limitation, once we changed the TMU mode to HiFi
> > uni-directional (when DP tunnel exists), we don't change TMU mode back to
> > normal uni-directional, even if DP tunnel is torn down later.
> > 
> > Signed-off-by: Gil Fine <gil.fine@intel.com>
> > ---
> >  drivers/thunderbolt/tb.c  | 28 ++++++++++++++++++++++++++++
> >  drivers/thunderbolt/tb.h  |  5 +++++
> >  drivers/thunderbolt/tmu.c | 14 ++++++++++++++
> >  3 files changed, 47 insertions(+)
> > 
> > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> > index f512197e719b..d0f85a8c56de 100644
> > --- a/drivers/thunderbolt/tb.c
> > +++ b/drivers/thunderbolt/tb.c
> > @@ -50,6 +50,8 @@ struct tb_hotplug_event {
> >  };
> >  
> >  static void tb_handle_hotplug(struct work_struct *work);
> > +static void tb_enable_tmu_1st_child(struct tb *tb,
> > +				    enum tb_switch_tmu_rate rate);
> 
> This forward declaration is not needed. You can just move the
> implementation before you call it first time.

Sure, right.

> 
> >  static void tb_queue_hotplug(struct tb *tb, u64 route, u8 port, bool unplug)
> >  {
> > @@ -118,6 +120,13 @@ static void tb_switch_discover_tunnels(struct tb_switch *sw,
> >  		switch (port->config.type) {
> >  		case TB_TYPE_DP_HDMI_IN:
> >  			tunnel = tb_tunnel_discover_dp(tb, port, alloc_hopids);
> > +			/*
> > +			 * In case of DP tunnel exists, change TMU mode to
> > +			 * HiFi for CL0s to work.
> > +			 */
> > +			if (tunnel)
> > +				tb_enable_tmu_1st_child(tb,
> > +						TB_SWITCH_TMU_RATE_HIFI);
> >  			break;
> >  
> >  		case TB_TYPE_PCIE_DOWN:
> > @@ -235,6 +244,19 @@ static int tb_enable_tmu(struct tb_switch *sw)
> >  	return tb_switch_tmu_enable(sw);
> >  }
> >  
> > +/*
> > + * Once a DP tunnel exists in the domain, we set the TMU mode so that
> > + * it meets the accuracy requirements and also enables CLx entry (CL0s).
> > + * We set the TMU mode of the first depth router(s) for CL0s to work.
> > + */
> > +static void tb_enable_tmu_1st_child(struct tb *tb, enum tb_switch_tmu_rate rate)
> > +{
> > +	struct tb_sw_tmu_config tmu = { .rate = rate };
> > +
> > +	device_for_each_child(&tb->root_switch->dev, &tmu,
> > +			      tb_switch_tmu_config_enable);
> > +}
> > +
> >  /**
> >   * tb_find_unused_port() - return the first inactive port on @sw
> >   * @sw: Switch to find the port on
> > @@ -985,6 +1007,12 @@ static void tb_tunnel_dp(struct tb *tb)
> >  
> >  	list_add_tail(&tunnel->list, &tcm->tunnel_list);
> >  	tb_reclaim_usb3_bandwidth(tb, in, out);
> > +	/*
> > +	 * In case of DP tunnel exists, change TMU mode to
> > +	 * HiFi for CL0s to work.
> > +	 */
> > +	tb_enable_tmu_1st_child(tb, TB_SWITCH_TMU_RATE_HIFI);
> > +
> >  	return;
> >  
> >  err_free:
> > diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> > index a16fffba9dd2..3dbd9d919d5f 100644
> > --- a/drivers/thunderbolt/tb.h
> > +++ b/drivers/thunderbolt/tb.h
> > @@ -110,6 +110,10 @@ struct tb_switch_tmu {
> >  	enum tb_switch_tmu_rate rate_request;
> >  };
> >  
> > +struct tb_sw_tmu_config {
> > +	enum tb_switch_tmu_rate rate;
> > +};
> 
> Is this wrapper structure really needed?

Right, will fix

> 
> > +
> >  enum tb_clx {
> >  	TB_CLX_DISABLE,
> >  	/* CL0s and CL1 are enabled and supported together */
> > @@ -934,6 +938,7 @@ int tb_switch_tmu_enable(struct tb_switch *sw);
> >  void tb_switch_tmu_configure(struct tb_switch *sw,
> >  			     enum tb_switch_tmu_rate rate,
> >  			     bool unidirectional);
> > +int tb_switch_tmu_config_enable(struct device *dev, void *data);
> >  /**
> >   * tb_switch_tmu_is_enabled() - Checks if the specified TMU mode is enabled
> >   * @sw: Router whose TMU mode to check
> > diff --git a/drivers/thunderbolt/tmu.c b/drivers/thunderbolt/tmu.c
> > index e822ab90338b..b8ff9f64a71e 100644
> > --- a/drivers/thunderbolt/tmu.c
> > +++ b/drivers/thunderbolt/tmu.c
> > @@ -727,6 +727,20 @@ int tb_switch_tmu_enable(struct tb_switch *sw)
> >  	return tb_switch_tmu_set_time_disruption(sw, false);
> >  }
> 
> You are missing kernel-doc for the non-static function.

Right, will add
> 
> >  
> > +int tb_switch_tmu_config_enable(struct device *dev, void *data)
> 
> Also can we please make it take some real type and not something
> arbitrary?
You mean the names, right?
Something like:
int tb_switch_tmu_config_enable(struct device *parent, void *rate)
If so, yes, I will

> 
> Can it be const too?
IIUC, it shall be a function pointer with specified signature otherwise it will fail
at compilation

> 
> > +{
> > +	if (tb_is_switch(dev)) {
> > +		struct tb_switch *sw = tb_to_switch(dev);
> > +		struct tb_sw_tmu_config *tmu = data;
> > +
> > +		tb_switch_tmu_configure(sw, tmu->rate, tb_switch_is_clx_enabled(sw, TB_CL1));
> > +		if (tb_switch_tmu_enable(sw))
> > +			tb_sw_dbg(sw, "Fail switching TMU to HiFi for 1st depth router\n");
> 
> Be consistent with the messaging so don't start with capital letter
> here.

OK

> 
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  /**
> >   * tb_switch_tmu_configure() - Configure the TMU rate and directionality
> >   * @sw: Router whose mode to change
> > -- 
> > 2.17.1

-- 
Thanks,
Gil
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH v3 6/6] thunderbolt: Change TMU mode to HiFi uni-directional once DisplayPort tunneled

[Mika Westerberg]

Hi Gil,

On Sun, May 15, 2022 at 11:27:46PM +0300, Gil Fine wrote:
> > > +int tb_switch_tmu_config_enable(struct device *dev, void *data)
> > 
> > Also can we please make it take some real type and not something
> > arbitrary?
> You mean the names, right?
> Something like:
> int tb_switch_tmu_config_enable(struct device *parent, void *rate)
> If so, yes, I will

I mean use a real type, not void *.

> > 
> > Can it be const too?
> IIUC, it shall be a function pointer with specified signature otherwise it will fail
> at compilation

Okay then I suggest to make a reasonable "API" function that handles
all this internally that does not take arbitrary pointers. Remember to
document it in kernel-doc too.

Re: [PATCH v3 6/6] thunderbolt: Change TMU mode to HiFi uni-directional once DisplayPort tunneled

[Gil Fine]

Hi Mika,

On Mon, May 16, 2022 at 11:34:15AM +0300, Mika Westerberg wrote:
> Hi Gil,
> 
> On Sun, May 15, 2022 at 11:27:46PM +0300, Gil Fine wrote:
> > > > +int tb_switch_tmu_config_enable(struct device *dev, void *data)
> > > 
> > > Also can we please make it take some real type and not something
> > > arbitrary?
> > You mean the names, right?
> > Something like:
> > int tb_switch_tmu_config_enable(struct device *parent, void *rate)
> > If so, yes, I will
> 
> I mean use a real type, not void *.
> 
> > > 
> > > Can it be const too?
> > IIUC, it shall be a function pointer with specified signature otherwise it will fail
> > at compilation
> 
> Okay then I suggest to make a reasonable "API" function that handles
> all this internally that does not take arbitrary pointers. Remember to
> document it in kernel-doc too.

This is a function pointer that shall be passed to device_for_each_child()
And it has to be defined as:

int (*fn)(struct device *dev, void *data)

Similar as here e.g.:

static int remove_retimer(struct device *dev, void *data)
{
»·······struct tb_retimer *rt = tb_to_retimer(dev);
»·······struct tb_port *port = data;

»·······if (rt && rt->port == port)
»·······»·······tb_retimer_remove(rt);
»·······return 0;
}

void tb_retimer_remove_all(struct tb_port *port)
{
»·······struct usb4_port *usb4;

»·······usb4 = port->usb4;
»·······if (usb4)
»·······»·······device_for_each_child_reverse(&usb4->dev, port,
»·······»·······»·······»·······»·······      remove_retimer);
}

So not sure I get you...

-- 
Thanks,
Gil
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH v3 6/6] thunderbolt: Change TMU mode to HiFi uni-directional once DisplayPort tunneled

[Mika Westerberg]

On Mon, May 16, 2022 at 11:59:03AM +0300, Gil Fine wrote:
> Hi Mika,
> 
> On Mon, May 16, 2022 at 11:34:15AM +0300, Mika Westerberg wrote:
> > Hi Gil,
> > 
> > On Sun, May 15, 2022 at 11:27:46PM +0300, Gil Fine wrote:
> > > > > +int tb_switch_tmu_config_enable(struct device *dev, void *data)
> > > > 
> > > > Also can we please make it take some real type and not something
> > > > arbitrary?
> > > You mean the names, right?
> > > Something like:
> > > int tb_switch_tmu_config_enable(struct device *parent, void *rate)
> > > If so, yes, I will
> > 
> > I mean use a real type, not void *.
> > 
> > > > 
> > > > Can it be const too?
> > > IIUC, it shall be a function pointer with specified signature otherwise it will fail
> > > at compilation
> > 
> > Okay then I suggest to make a reasonable "API" function that handles
> > all this internally that does not take arbitrary pointers. Remember to
> > document it in kernel-doc too.
> 
> This is a function pointer that shall be passed to device_for_each_child()
> And it has to be defined as:
> 
> int (*fn)(struct device *dev, void *data)
> 
> Similar as here e.g.:
> 
> static int remove_retimer(struct device *dev, void *data)
> {
> »·······struct tb_retimer *rt = tb_to_retimer(dev);
> »·······struct tb_port *port = data;
> 
> »·······if (rt && rt->port == port)
> »·······»·······tb_retimer_remove(rt);
> »·······return 0;
> }
> 
> void tb_retimer_remove_all(struct tb_port *port)
> {
> »·······struct usb4_port *usb4;
> 
> »·······usb4 = port->usb4;
> »·······if (usb4)
> »·······»·······device_for_each_child_reverse(&usb4->dev, port,
> »·······»·······»·······»·······»·······      remove_retimer);
> }
> 
> So not sure I get you...

The difference is that above it is static function not exposed outside
of that file and used directly below its implementation.

In your case you make it non-static "API" function exported from tmu.c
and called from tb.c.

So instead I suggest to put the device_for_each_child() in tmu.c and
then the tb_switch_tmu_config_enable() static right above it. Please
also name the resulting API function consistently.

Re: [PATCH v3 6/6] thunderbolt: Change TMU mode to HiFi uni-directional once DisplayPort tunneled

[Mika Westerberg]

Hi Gil,

On Mon, May 16, 2022 at 04:21:41PM +0300, Gil Fine wrote:
> > So instead I suggest to put the device_for_each_child() in tmu.c and
> > then the tb_switch_tmu_config_enable() static right above it. Please
> > also name the resulting API function consistently.
> 
> OK, got you and fixed that.
> Please let me know when you think that I can send out the v4 series.

You can send it whenever you want but I think it can go to v5.20 and not
v5.19 since I was planning to send out my pull request for Greg
tomorrow. I can pick it up after v5.19-rc1 is released.

Re: [PATCH v3 6/6] thunderbolt: Change TMU mode to HiFi uni-directional once DisplayPort tunneled

[Gil Fine]

Hi Mika,

On Mon, May 16, 2022 at 12:34:06PM +0300, Mika Westerberg wrote:
> On Mon, May 16, 2022 at 11:59:03AM +0300, Gil Fine wrote:
> > Hi Mika,
> > 
> > On Mon, May 16, 2022 at 11:34:15AM +0300, Mika Westerberg wrote:
> > > Hi Gil,
> > > 
> > > On Sun, May 15, 2022 at 11:27:46PM +0300, Gil Fine wrote:
> > > > > > +int tb_switch_tmu_config_enable(struct device *dev, void *data)
> > > > > 
> > > > > Also can we please make it take some real type and not something
> > > > > arbitrary?
> > > > You mean the names, right?
> > > > Something like:
> > > > int tb_switch_tmu_config_enable(struct device *parent, void *rate)
> > > > If so, yes, I will
> > > 
> > > I mean use a real type, not void *.
> > > 
> > > > > 
> > > > > Can it be const too?
> > > > IIUC, it shall be a function pointer with specified signature otherwise it will fail
> > > > at compilation
> > > 
> > > Okay then I suggest to make a reasonable "API" function that handles
> > > all this internally that does not take arbitrary pointers. Remember to
> > > document it in kernel-doc too.
> > 
> > This is a function pointer that shall be passed to device_for_each_child()
> > And it has to be defined as:
> > 
> > int (*fn)(struct device *dev, void *data)
> > 
> > Similar as here e.g.:
> > 
> > static int remove_retimer(struct device *dev, void *data)
> > {
> > »·······struct tb_retimer *rt = tb_to_retimer(dev);
> > »·······struct tb_port *port = data;
> > 
> > »·······if (rt && rt->port == port)
> > »·······»·······tb_retimer_remove(rt);
> > »·······return 0;
> > }
> > 
> > void tb_retimer_remove_all(struct tb_port *port)
> > {
> > »·······struct usb4_port *usb4;
> > 
> > »·······usb4 = port->usb4;
> > »·······if (usb4)
> > »·······»·······device_for_each_child_reverse(&usb4->dev, port,
> > »·······»·······»·······»·······»·······      remove_retimer);
> > }
> > 
> > So not sure I get you...
> 
> The difference is that above it is static function not exposed outside
> of that file and used directly below its implementation.
> 
> In your case you make it non-static "API" function exported from tmu.c
> and called from tb.c.
> 
> So instead I suggest to put the device_for_each_child() in tmu.c and
> then the tb_switch_tmu_config_enable() static right above it. Please
> also name the resulting API function consistently.

OK, got you and fixed that.
Please let me know when you think that I can send out the v4 series.

-- 
Thanks,
Gil
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH v3 6/6] thunderbolt: Change TMU mode to HiFi uni-directional once DisplayPort tunneled

[Greg KH]

On Mon, May 16, 2022 at 04:45:48PM +0300, Gil Fine wrote:
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
> 

Now deleted.  This footer is not compatible with kernel development,
sorry.

Re: [PATCH v3 6/6] thunderbolt: Change TMU mode to HiFi uni-directional once DisplayPort tunneled

[Gil Fine]

On Mon, May 16, 2022 at 04:19:46PM +0300, Mika Westerberg wrote:
> Hi Gil,
> 
> On Mon, May 16, 2022 at 04:21:41PM +0300, Gil Fine wrote:
> > > So instead I suggest to put the device_for_each_child() in tmu.c and
> > > then the tb_switch_tmu_config_enable() static right above it. Please
> > > also name the resulting API function consistently.
> > 
> > OK, got you and fixed that.
> > Please let me know when you think that I can send out the v4 series.
> 
> You can send it whenever you want but I think it can go to v5.20 and not
> v5.19 since I was planning to send out my pull request for Greg
> tomorrow. I can pick it up after v5.19-rc1 is released.

Ohh I see, I was hoping to meet v5.19
Can't you send it also as part of your your pull request tomorrow?
Or it is not mature enough yet...?

-- 
Thanks,
Gil
---------------------------------------------------------------------
Intel Israel (74) Limited

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

Re: [PATCH v3 6/6] thunderbolt: Change TMU mode to HiFi uni-directional once DisplayPort tunneled

[Mika Westerberg]

On Mon, May 16, 2022 at 04:45:48PM +0300, Gil Fine wrote:
> On Mon, May 16, 2022 at 04:19:46PM +0300, Mika Westerberg wrote:
> > Hi Gil,
> > 
> > On Mon, May 16, 2022 at 04:21:41PM +0300, Gil Fine wrote:
> > > > So instead I suggest to put the device_for_each_child() in tmu.c and
> > > > then the tb_switch_tmu_config_enable() static right above it. Please
> > > > also name the resulting API function consistently.
> > > 
> > > OK, got you and fixed that.
> > > Please let me know when you think that I can send out the v4 series.
> > 
> > You can send it whenever you want but I think it can go to v5.20 and not
> > v5.19 since I was planning to send out my pull request for Greg
> > tomorrow. I can pick it up after v5.19-rc1 is released.
> 
> Ohh I see, I was hoping to meet v5.19
> Can't you send it also as part of your your pull request tomorrow?
> Or it is not mature enough yet...?

Well typically it is expected that patches have been sitting in
linux-next for a while before going to the upstream maintainer (Greg in
this case). This is to ensure they get at least some attention from the
various build robots and also people running linux-next on their
hardware.