thunderbolt: A couple of fixes
Subsystems (2)
cover thunderbolt: A couple of fixes
Hi,
This series includes a couple of fixes for issues found. I'm planning to get these into v6.3-rc.
The first one is a memory leak fix.
The second one is fixing a reboot time issue reported by Christian that we finally were able to reproduce in the lab. The reason this happened only in Linux is that we enumerate retimers during link bring up and there is certain step missing after the enumeration that leads the link to not come up properly after soft-reboot. This should be fixed with issuing UNSET_INBOUND_SBTX after retimer access and this works in the lab.
@Christian, can you check that it solves the issue for you too?
The third one is a quirk that is needed for Intel hardware to limit the USB3 bandwidth accordingly.
Gil Fine (2):
thunderbolt: Add missing UNSET_INBOUND_SBTX for retimer access
thunderbolt: Limit USB3 bandwidth of certain Intel USB4 host routers
Mika Westerberg (3):
thunderbolt: Fix memory leak in margining
thunderbolt: Call tb_check_quirks() after initializing adapters
thunderbolt: Use scale field when allocating USB3 bandwidth
drivers/thunderbolt/debugfs.c | 12 ++++---- drivers/thunderbolt/quirks.c | 31 ++++++++++++++++++++ drivers/thunderbolt/retimer.c | 23 +++++++++++++-- drivers/thunderbolt/sb_regs.h | 1 + drivers/thunderbolt/switch.c | 4 +-- drivers/thunderbolt/tb.h | 4 +++ drivers/thunderbolt/usb4.c | 53 ++++++++++++++++++++++++++++++----- 7 files changed, 111 insertions(+), 17 deletions(-)
Signature · 1 line
2.39.1
PATCH 1/5 thunderbolt: Fix memory leak in margining
Memory for the usb4->margining needs to be relased for the upstream port of the router as well, even though the debugfs directory gets released with the router device removal. Fix this.
--- drivers/thunderbolt/debugfs.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/thunderbolt/debugfs.c +6 -6
@@ -942,7 +942,8 @@ static void margining_port_remove(struct tb_port *port)snprintf (dir_name,sizeof (dir_name),"port%d" , port->port); parent =debugfs_lookup (dir_name, port->sw->debugfs_dir); -debugfs_remove_recursive (debugfs_lookup ("margining" , parent)); +if (parent) +debugfs_remove_recursive (debugfs_lookup ("margining" , parent));kfree (port->usb4->margining); port->usb4->margining =NULL ;
@@ -967,19 +968,18 @@ static void margining_switch_init(struct tb_switch *sw)static void margining_switch_remove (struct tb_switch *sw) { +struct tb_port *upstream, *downstream;struct tb_switch *parent_sw; -struct tb_port *downstream;u64 route =tb_route (sw);if (!route)return ; -/* -* Upstream is removed with the router itself but we need to -* remove the downstream port margining directory. -*/ + upstream =tb_upstream_port (sw); parent_sw =tb_switch_parent (sw); downstream =tb_port_at (route, parent_sw); + +margining_port_remove (upstream);margining_port_remove (downstream); }
Signature · 1 line
2.39.1
PATCH 2/5 thunderbolt: Add missing UNSET_INBOUND_SBTX for retimer access
In reply to Mika WesterbergFrom: Gil Fine <[email protected]>
According to USB4 retimer specification, the process of firmware update sequence requires issuing a SET_INBOUND_SBTX port operation that later shall be followed by UNSET_INBOUND_SBTX port operation. This last step is not currently issued by the driver but it is necessary to make sure the retimers are put back to passthrough mode even during enumeration.
If this step is missing the link may not come up properly after soft-reboot for example.
For this reason issue UNSET_INBOUND_SBTX after SET_INBOUND_SBTX for enumeration and also when the NVM upgrade is run.
--- drivers/thunderbolt/retimer.c | 23 +++++++++++++++++++++-- drivers/thunderbolt/sb_regs.h | 1 + drivers/thunderbolt/tb.h | 1 + drivers/thunderbolt/usb4.c | 14 ++++++++++++++ 4 files changed, 37 insertions(+), 2 deletions(-)
drivers/thunderbolt/retimer.c +21 -2
@@ -187,6 +187,22 @@ static ssize_t nvm_authenticate_show(struct device *dev,return ret; } +static void tb_retimer_set_inbound_sbtx (struct tb_port *port) +{ +int i; + +for (i =1 ; i <=TB_MAX_RETIMER_INDEX ; i++) +usb4_port_retimer_set_inbound_sbtx (port, i); +} + +static void tb_retimer_unset_inbound_sbtx (struct tb_port *port) +{ +int i; + +for (i =TB_MAX_RETIMER_INDEX ; i >=1 ; i--) +usb4_port_retimer_unset_inbound_sbtx (port, i); +} +static ssize_t nvm_authenticate_store (struct device *dev,struct device_attribute *attr,const char *buf,size_t count) {
@@ -213,6 +229,7 @@ static ssize_t nvm_authenticate_store(struct device *dev, rt->auth_status =0 ;if (val) { +tb_retimer_set_inbound_sbtx (rt->port);if (val ==AUTHENTICATE_ONLY ) { ret =tb_retimer_nvm_authenticate (rt, true); }else {
@@ -232,6 +249,7 @@ static ssize_t nvm_authenticate_store(struct device *dev, } exit_unlock: +tb_retimer_unset_inbound_sbtx (rt->port);mutex_unlock (&rt->tb->lock); exit_rpm:pm_runtime_mark_last_busy (&rt->dev);
@@ -440,8 +458,7 @@ int tb_retimer_scan(struct tb_port *port, bool add)* Enable sideband channel for each retimer. We can do this * regardless whether there is device connected or not. */ -for (i =1 ; i <=TB_MAX_RETIMER_INDEX ; i++) -usb4_port_retimer_set_inbound_sbtx (port, i); +tb_retimer_set_inbound_sbtx (port);/* * Before doing anything else, read the authentication status.
@@ -464,6 +481,8 @@ int tb_retimer_scan(struct tb_port *port, bool add)break ; } +tb_retimer_unset_inbound_sbtx (port); +if (!last_idx)return 0 ;
drivers/thunderbolt/sb_regs.h +1 -0
@@ -20,6 +20,7 @@ enum usb4_sb_opcode {USB4_SB_OPCODE_ROUTER_OFFLINE =0x4e45534c ,/* "LSEN" */ USB4_SB_OPCODE_ENUMERATE_RETIMERS =0x4d554e45 ,/* "ENUM" */ USB4_SB_OPCODE_SET_INBOUND_SBTX =0x5055534c ,/* "LSUP" */ +USB4_SB_OPCODE_UNSET_INBOUND_SBTX =0x50555355 ,/* "USUP" */ USB4_SB_OPCODE_QUERY_LAST_RETIMER =0x5453414c ,/* "LAST" */ USB4_SB_OPCODE_GET_NVM_SECTOR_SIZE =0x53534e47 ,/* "GNSS" */ USB4_SB_OPCODE_NVM_SET_OFFSET = 0x53504f42, /* "BOPS" */
drivers/thunderbolt/tb.h +1 -0
@@ -1242,6 +1242,7 @@ int usb4_port_sw_margin(struct tb_port *port, unsigned int lanes, bool timing,int usb4_port_sw_margin_errors (struct tb_port *port,u32 *errors);int usb4_port_retimer_set_inbound_sbtx (struct tb_port *port,u8 index); +int usb4_port_retimer_unset_inbound_sbtx (struct tb_port *port,u8 index);int usb4_port_retimer_read (struct tb_port *port,u8 index,u8 reg,void *buf,u8 size); int usb4_port_retimer_write(struct tb_port *port, u8 index, u8 reg,
drivers/thunderbolt/usb4.c +14 -0
@@ -1578,6 +1578,20 @@ int usb4_port_retimer_set_inbound_sbtx(struct tb_port *port, u8 index) 500); } +/** +* usb4_port_retimer_unset_inbound_sbtx() - Disable sideband channel transactions +* @port: USB4 port +* @index: Retimer index +* +* Disables sideband channel transations on SBTX. The reverse of +* usb4_port_retimer_set_inbound_sbtx(). +*/ +int usb4_port_retimer_unset_inbound_sbtx (struct tb_port *port,u8 index) +{ +return usb4_port_retimer_op (port, index, +USB4_SB_OPCODE_UNSET_INBOUND_SBTX ,500 ); +} +/** * usb4_port_retimer_read() - Read from retimer sideband registers * @port: USB4 port
Signature · 1 line
2.39.1
PATCH 3/5 thunderbolt: Call tb_check_quirks() after initializing adapters
In reply to Mika WesterbergIn order to apply quirks based on certain adapter types move call to tb_check_quirks() happen after the adapters are initialized. This should not affect the existing quirks.
--- drivers/thunderbolt/switch.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/thunderbolt/switch.c +2 -2
@@ -2968,8 +2968,6 @@ int tb_switch_add(struct tb_switch *sw)dev_warn (&sw->dev,"reading DROM failed: %d\n" , ret);tb_sw_dbg (sw,"uid: %#llx\n" , sw->uid); -tb_check_quirks (sw); - ret =tb_switch_set_uuid (sw);if (ret) {dev_err (&sw->dev,"failed to set UUID\n" );
@@ -2988,6 +2986,8 @@ int tb_switch_add(struct tb_switch *sw) } } +tb_check_quirks (sw); +tb_switch_default_link_ports (sw); ret =tb_switch_update_link_attributes (sw);
Signature · 1 line
2.39.1
PATCH 4/5 thunderbolt: Limit USB3 bandwidth of certain Intel USB4 host routers
In reply to Mika WesterbergFrom: Gil Fine <[email protected]>
Current Intel USB4 host routers have hardware limitation that the USB3 bandwidth cannot go higher than 16376 Mb/s. Work this around by adding a new quirk that limits the bandwidth for the affected host routers.
--- drivers/thunderbolt/quirks.c | 31 +++++++++++++++++++++++++++++++ drivers/thunderbolt/tb.h | 3 +++ drivers/thunderbolt/usb4.c | 17 +++++++++++++++-- 3 files changed, 49 insertions(+), 2 deletions(-)
drivers/thunderbolt/quirks.c +31 -0
@@ -26,6 +26,19 @@ static void quirk_clx_disable(struct tb_switch *sw)tb_sw_dbg (sw,"disabling CL states\n" ); } +static void quirk_usb3_maximum_bandwidth (struct tb_switch *sw) +{ +struct tb_port *port; + +tb_switch_for_each_port (sw, port) { +if (!tb_port_is_usb3_down (port)) +continue ; + port->max_bw =16376 ; +tb_port_dbg (port,"USB3 maximum bandwidth limited to %u Mb/s\n" , + port->max_bw); + } +} +struct tb_quirk {u16 hw_vendor_id;u16 hw_device_id;
@@ -43,6 +56,24 @@ static const struct tb_quirk tb_quirks[] = {* DP buffers. */ { 0x8087, 0x0b26, 0x0000, 0x0000, quirk_dp_credit_allocation }, +/* +* Limit the maximum USB3 bandwidth for the following Intel USB4 +* host routers due to a hardware issue. +*/ + { 0x8087, PCI_DEVICE_ID_INTEL_ADL_NHI0, 0x0000, 0x0000, + quirk_usb3_maximum_bandwidth }, + { 0x8087, PCI_DEVICE_ID_INTEL_ADL_NHI1, 0x0000, 0x0000, + quirk_usb3_maximum_bandwidth }, + { 0x8087, PCI_DEVICE_ID_INTEL_RPL_NHI0, 0x0000, 0x0000, + quirk_usb3_maximum_bandwidth }, + { 0x8087, PCI_DEVICE_ID_INTEL_RPL_NHI1, 0x0000, 0x0000, + quirk_usb3_maximum_bandwidth }, + { 0x8087, PCI_DEVICE_ID_INTEL_MTL_M_NHI0, 0x0000, 0x0000, + quirk_usb3_maximum_bandwidth }, + { 0x8087, PCI_DEVICE_ID_INTEL_MTL_P_NHI0, 0x0000, 0x0000, + quirk_usb3_maximum_bandwidth }, + { 0x8087, PCI_DEVICE_ID_INTEL_MTL_P_NHI1, 0x0000, 0x0000, + quirk_usb3_maximum_bandwidth },/* * CLx is not supported on AMD USB4 Yellow Carp and Pink Sardine platforms. */
drivers/thunderbolt/tb.h +3 -0
@@ -272,6 +272,8 @@ struct tb_bandwidth_group {* @group: Bandwidth allocation group the adapter is assigned to. Only * used for DP IN adapters for now. * @group_list: The adapter is linked to the group's list of ports through this +* @max_bw: Maximum possible bandwidth through this adapter if set to +* non-zero. * * In USB4 terminology this structure represents an adapter (protocol or * lane adapter).
@@ -299,6 +301,7 @@ struct tb_port {unsigned int dma_credits;struct tb_bandwidth_group *group;struct list_head group_list; +unsigned int max_bw; };/**
drivers/thunderbolt/usb4.c +15 -2
@@ -1882,6 +1882,15 @@ int usb4_port_retimer_nvm_read(struct tb_port *port, u8 index, usb4_port_retimer_nvm_read_block, &info); } +static inline unsigned int +usb4_usb3_port_max_bandwidth (const struct tb_port *port,unsigned int bw) +{ +/* Take the possible bandwidth limitation into account */ +if (port->max_bw) +return min (bw, port->max_bw); +return bw; +} +/** * usb4_usb3_port_max_link_rate() - Maximum support USB3 link rate * @port: USB3 adapter port
@@ -1903,7 +1912,9 @@ int usb4_usb3_port_max_link_rate(struct tb_port *port)return ret; lr = (val &ADP_USB3_CS_4_MSLR_MASK ) >>ADP_USB3_CS_4_MSLR_SHIFT ; -return lr ==ADP_USB3_CS_4_MSLR_20G ?20000 :10000 ; + ret = lr ==ADP_USB3_CS_4_MSLR_20G ?20000 :10000 ; + +return usb4_usb3_port_max_bandwidth (port, ret); }/**
@@ -1930,7 +1941,9 @@ int usb4_usb3_port_actual_link_rate(struct tb_port *port)return 0 ; lr = val &ADP_USB3_CS_4_ALR_MASK ; -return lr ==ADP_USB3_CS_4_ALR_20G ?20000 :10000 ; + ret = lr ==ADP_USB3_CS_4_ALR_20G ?20000 :10000 ; + +return usb4_usb3_port_max_bandwidth (port, ret); }static int usb4_usb3_port_cm_request (struct tb_port *port,bool request)
Signature · 1 line
2.39.1
PATCH 5/5 thunderbolt: Use scale field when allocating USB3 bandwidth
In reply to Mika WesterbergWhen tunneling aggregated USB3 (20 Gb/s) the bandwidth values that are programmed to the ADP_USB3_CS_2 go higher than 4096 and that does not fit anymmore to the 12-bit field. Fix this by scaling the value using the scale field accordingly.
--- drivers/thunderbolt/usb4.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)
drivers/thunderbolt/usb4.c +17 -5
@@ -2094,18 +2094,30 @@ static int usb4_usb3_port_write_allocated_bandwidth(struct tb_port *port, int downstream_bw) {u32 val, ubw, dbw, scale; -int ret; +int ret, max_bw; -/* Read the used scale, hardware default is 0 */ - ret =tb_port_read (port, &scale,TB_CFG_PORT , - port->cap_adap +ADP_USB3_CS_3 ,1 ); +/* Figure out suitable scale */ + scale =0 ; + max_bw =max (upstream_bw, downstream_bw); +while (scale <64 ) { +if (mbps_to_usb3_bw (max_bw, scale) <4096 ) +break ; + scale++; + } + +if (WARN_ON (scale >=64 )) +return -EINVAL ; + + ret =tb_port_write (port, &scale,TB_CFG_PORT , + port->cap_adap +ADP_USB3_CS_3 ,1 );if (ret)return ret; - scale &=ADP_USB3_CS_3_SCALE_MASK ; ubw =mbps_to_usb3_bw (upstream_bw, scale); dbw =mbps_to_usb3_bw (downstream_bw, scale); +tb_port_dbg (port,"scaled bandwidth %u/%u, scale %u\n" , ubw, dbw, scale); + ret =tb_port_read (port, &val,TB_CFG_PORT , port->cap_adap +ADP_USB3_CS_2 ,1 );if (ret)
Signature · 1 line
2.39.1
Yehezkel Bernat
maintainerRe: [PATCH 5/5] thunderbolt: Use scale field when allocating USB3 bandwidth
Quoted 4 lines · Mika Westerberg
When tunneling aggregated USB3 (20 Gb/s) the bandwidth values that are programmed to the
ADP_USB3_CS_2go higher than 4096 and that does not fit anymmore to the 12-bit field. Fix this by scaling the value using
typo: anymore
Quoted 20 lines · Mika Westerberg
Hi,
This series includes a couple of fixes for issues found. I'm planning to get these into v6.3-rc.
The first one is a memory leak fix.
The second one is fixing a reboot time issue reported by Christian that we finally were able to reproduce in the lab. The reason this happened only in Linux is that we enumerate retimers during link bring up and there is certain step missing after the enumeration that leads the link to not come up properly after soft-reboot. This should be fixed with issuing
UNSET_INBOUND_SBTXafter retimer access and this works in the lab.@Christian, can you check that it solves the issue for you too?
The third one is a quirk that is needed for Intel hardware to limit the USB3 bandwidth accordingly.
Quoted 8 lines
Gil Fine (2):
thunderbolt: Add missingUNSET_INBOUND_SBTXfor retimer access
thunderbolt: Limit USB3 bandwidth of certain Intel USB4 host routersMika Westerberg (3):
thunderbolt: Fix memory leak in margining
thunderbolt: Calltb_check_quirks()after initializing adapters
thunderbolt: Use scale field when allocating USB3 bandwidth
Fixed the typo reported by Yehezkel and applied all to thunderbolt.git/fixes.
Hi Mika,
Quoted 9 lines
Quoted 9 lines
The second one is fixing a reboot time issue reported by Christian that we finally were able to reproduce in the lab. The reason this happened only in Linux is that we enumerate retimers during link bring up and there is certain step missing after the enumeration that leads the link to not come up properly after soft-reboot. This should be fixed with issuing
UNSET_INBOUND_SBTXafter retimer access and this works in the lab.@Christian, can you check that it solves the issue for you too?
I was told today that the patch solves the issue on the Elitebook! (don't have the hardware here in my office any more...).
Thanks for fixing this!
Best regards,
Christian
Quoted 11 lines · Christian Schaubschläger
Hi Mika,
Quoted 8 lines
Quoted 8 lines
The second one is fixing a reboot time issue reported by Christian that we finally were able to reproduce in the lab. The reason this happened only in Linux is that we enumerate retimers during link bring up and there is certain step missing after the enumeration that leads the link to not come up properly after soft-reboot. This should be fixed with issuing
UNSET_INBOUND_SBTXafter retimer access and this works in the lab.
Quoted 3 lines
Quoted 1 line
Quoted 1 line
@Christian, can you check that it solves the issue for you too?
I was told today that the patch solves the issue on the Elitebook!
Okay good to know :)