thunderbolt: CL1 support for USB4 and Titan Ridge

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 <[email protected]>

---
 drivers/thunderbolt/tb.c  | 28 ++++++++++++++++++++++++++++
 drivers/thunderbolt/tb.h  |  5 +++++
 drivers/thunderbolt/tmu.c | 14 ++++++++++++++
 3 files changed, 47 insertions(+)

drivers/thunderbolt/tb.c +0 -0

index f512197e719b..d0f85a8c56de 100644

drivers/thunderbolt/tb.c +26 -0

 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:

drivers/thunderbolt/tb.h +0 -0

index a16fffba9dd2..3dbd9d919d5f 100644

drivers/thunderbolt/tb.h +2 -0

+
 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

drivers/thunderbolt/tmu.c +0 -0

index e822ab90338b..b8ff9f64a71e 100644

drivers/thunderbolt/tmu.c +5 -0

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

Signature · 1 line

2.17.1

Hi Gil,

Quoted 21 lines · 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 <[email protected]>

---
 drivers/thunderbolt/tb.c  | 28 ++++++++++++++++++++++++++++
 drivers/thunderbolt/tb.h  |  5 +++++
 drivers/thunderbolt/tmu.c | 14 ++++++++++++++
 3 files changed, 47 insertions(+)

drivers/thunderbolt/tb.c +0 -0

index f512197e719b..d0f85a8c56de 100644
@@ -50,6 +50,8 @@ struct tb_hotplug_event {
 };
 

drivers/thunderbolt/tb.c struct tb_hotplug_event { · line 54

Quoted 3 lines

drivers/thunderbolt/tb.c +2 -0

 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.

Quoted 58 lines

drivers/thunderbolt/tb.c +26 -0

 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:

drivers/thunderbolt/tb.h +0 -0

index a16fffba9dd2..3dbd9d919d5f 100644
@@ -110,6 +110,10 @@ struct tb_switch_tmu {
 	enum tb_switch_tmu_rate rate_request;
 };
 

drivers/thunderbolt/tb.h struct tb_switch_tmu { · line 115

Quoted 3 lines

drivers/thunderbolt/tb.h +3 -0

+struct tb_sw_tmu_config {
+	enum tb_switch_tmu_rate rate;
+};

Is this wrapper structure really needed?

Quoted 14 lines

drivers/thunderbolt/tb.h +2 -0

+
 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

drivers/thunderbolt/tmu.c +0 -0

drivers/thunderbolt/tmu.c int tb_switch_tmu_enable(struct tb_switch *sw) · line 728

Quoted 6 lines

index e822ab90338b..b8ff9f64a71e 100644

drivers/thunderbolt/tmu.c +0 -0

@@ -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.

drivers/thunderbolt/tmu.c int tb_switch_tmu_enable(struct tb_switch *sw) · line 730

Quoted 3 lines

drivers/thunderbolt/tmu.c +1 -0

+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?

drivers/thunderbolt/tmu.c int tb_switch_tmu_enable(struct tb_switch *sw) · line 738

Quoted 9 lines

drivers/thunderbolt/tmu.c +8 -0

+{
+	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.

Quoted 11 lines

drivers/thunderbolt/tmu.c +5 -0

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

Signature · 1 line

2.17.1

Quoted 4 lines

Quoted 1 line

+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

Quoted 2 lines

Can it be const too?

IIUC, it shall be a function pointer with specified signature otherwise it will fail at compilation

Hi Gil,

Quoted 8 lines · Gil Fine

Quoted 4 lines

Quoted 1 line

+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 *.

Quoted 1 line

Quoted 1 line

Quoted 3 lines

Quoted 1 line

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.

Hi Mika,

Quoted 15 lines · Mika Westerberg

Hi Gil,

Quoted 8 lines · Gil Fine

Quoted 4 lines

Quoted 1 line

+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 *.

Quoted 1 line

Quoted 1 line

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

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...

Quoted 47 lines · Gil Fine

Hi Mika,

Quoted 15 lines · Mika Westerberg

Hi Gil,

Quoted 8 lines · Gil Fine

Quoted 4 lines

Quoted 1 line

+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 *.

Quoted 1 line

Quoted 1 line

Quoted 7 lines

Quoted 3 lines

Quoted 1 line

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;

Quoted 7 lines

»·······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.

Quoted 3 lines

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.

Hi Gil,

Quoted 4 lines · Gil Fine

Quoted 3 lines

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.

Quoted 2 lines

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.

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.

Quoted 10 lines · Mika Westerberg

Hi Gil,

Quoted 6 lines · Gil Fine

Quoted 3 lines

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.

Quoted 3 lines

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...?