Hello, thank you for your prompt reply! On Wed, Jan 29, 2025 at 01:48:04PM GMT, Mika Westerberg wrote: > Hi, > > On Wed, Jan 29, 2025 at 11:24:33AM +0000, Mohammad Rahimi wrote: > > Hello! Thanks for the thorough review. > > > > On Wed, Jan 29, 2025 at 08:31:48AM GMT, Mika Westerberg wrote: > > > Hi, > > > > > > On Tue, Jan 28, 2025 at 04:36:05PM +0000, Mohammad Rahimi wrote: > > > > Updates the Connection Manager to disable the Gen 4 Link Recovery > > > > flow before transitioning from a Symmetric Link to an Asymmetric > > > > Link, as specified in recent changes to the USB4 v2 specification. > > > > > > > > According to the "USB4 2.0 ENGINEERING CHANGE NOTICE FORM" > > > > published in September 2024, the rationale for this change is: > > > > > > > > "Since the default value of the Target Asymmetric Link might be > > > > different than Symmetric Link and Gen 4 Link Recovery flow checks > > > > this field to make sure it matched the actual Negotiated Link Width, > > > > we’re removing the condition for a Disconnect in the Gen 4 Link > > > > Recovery flow when Target Asymmetric Link doesn’t match the actual > > > > Link width and adding a Connection Manager note to Disable Gen 4 Link > > > > Recovery flow before doing Asymmetric Transitions." > > > > > > > > Signed-off-by: Mohammad Rahimi <rahimi.mhmmd@xxxxxxxxx> > > > > --- > > > > drivers/thunderbolt/tb.c | 23 ++++--- > > > > drivers/thunderbolt/tb.h | 3 + > > > > drivers/thunderbolt/tb_regs.h | 1 + > > > > drivers/thunderbolt/usb4.c | 125 ++++++++++++++++++++++++++++++++++ > > > > 4 files changed, 142 insertions(+), 10 deletions(-) > > > > > > > > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c > > > > index a7c6919fbf97..da53e4619eca 100644 > > > > --- a/drivers/thunderbolt/tb.c > > > > +++ b/drivers/thunderbolt/tb.c > > > > @@ -1013,7 +1013,7 @@ static int tb_configure_asym(struct tb *tb, struct tb_port *src_port, > > > > struct tb_port *dst_port, int requested_up, > > > > int requested_down) > > > > { > > > > - bool clx = false, clx_disabled = false, downstream; > > > > + bool clx_was_enable = false, lrf_was_enable = false, downstream; > > > > > > Let's call it link_recovery instead of lrf. > > > > > > > struct tb_switch *sw; > > > > struct tb_port *up; > > > > int ret = 0; > > > > @@ -1075,14 +1075,12 @@ static int tb_configure_asym(struct tb *tb, struct tb_port *src_port, > > > > continue; > > > > > > > > /* > > > > - * Disable CL states before doing any transitions. We > > > > - * delayed it until now that we know there is a real > > > > - * transition taking place. > > > > + * Disable CL states and Link Recovery flow before doing any > > > > + * transitions. We delayed it until now that we know there is > > > > + * a real transition taking place. > > > > */ > > > > - if (!clx_disabled) { > > > > - clx = tb_disable_clx(sw); > > > > - clx_disabled = true; > > > > - } > > > > + clx_was_enable = tb_disable_clx(sw); > > > > + lrf_was_enable = usb4_disable_lrf(sw); > > > > > > The naming is off here and I suggest to do this per USB4 port not per > > > router. See below. > > > > > > > Sure, I'll handle it per port. Please see my question below. > > > > > > tb_sw_dbg(up->sw, "configuring asymmetric link\n"); > > > > > > > > @@ -1097,9 +1095,14 @@ static int tb_configure_asym(struct tb *tb, struct tb_port *src_port, > > > > } > > > > } > > > > > > > > - /* Re-enable CL states if they were previosly enabled */ > > > > - if (clx) > > > > + /* > > > > + * Re-enable CL states and Link Recovery flow if > > > > + * they were previosly enabled > > > > + */ > > > > + if (clx_was_enable) > > > > tb_enable_clx(sw); > > > > + if (lrf_was_enable) > > > > + usb4_enable_lrf(sw); > > > > > > > > return ret; > > > > } > > > > diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h > > > > index ddbf0cd78377..3bec35f78d51 100644 > > > > --- a/drivers/thunderbolt/tb.h > > > > +++ b/drivers/thunderbolt/tb.h > > > > @@ -1336,6 +1336,9 @@ bool usb4_port_asym_supported(struct tb_port *port); > > > > int usb4_port_asym_set_link_width(struct tb_port *port, enum tb_link_width width); > > > > int usb4_port_asym_start(struct tb_port *port); > > > > > > > > +bool usb4_enable_lrf(struct tb_switch *sw); > > > > > > int usb4_port_link_recovery_enable(struct tb_port *port); > > > > > > > +bool usb4_disable_lrf(struct tb_switch *sw); > > > > > > void usb4_port_link_recovery_disable(struct tb_port *port); > > > > > > > Here, you have suggested to return int for enabling but void for disabling. > > Let’s also check my next comment. > > > > > and move these above the asym port functions. > > > > > > > + > > > > /** > > > > * enum tb_sb_target - Sideband transaction target > > > > * @USB4_SB_TARGET_ROUTER: Target is the router itself > > > > diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h > > > > index 4e43b47f9f11..085139e1a958 100644 > > > > --- a/drivers/thunderbolt/tb_regs.h > > > > +++ b/drivers/thunderbolt/tb_regs.h > > > > @@ -398,6 +398,7 @@ struct tb_regs_port_header { > > > > #define PORT_CS_19_WOD BIT(17) > > > > #define PORT_CS_19_WOU4 BIT(18) > > > > #define PORT_CS_19_START_ASYM BIT(24) > > > > +#define PORT_CS_19_ELR BIT(31) > > > > > > > > /* Display Port adapter registers */ > > > > #define ADP_DP_CS_0 0x00 > > > > diff --git a/drivers/thunderbolt/usb4.c b/drivers/thunderbolt/usb4.c > > > > index e51d01671d8e..49dd3d201617 100644 > > > > --- a/drivers/thunderbolt/usb4.c > > > > +++ b/drivers/thunderbolt/usb4.c > > > > @@ -10,6 +10,7 @@ > > > > #include <linux/delay.h> > > > > #include <linux/ktime.h> > > > > #include <linux/units.h> > > > > +#include <linux/string_helpers.h> > > > > > > > > #include "sb_regs.h" > > > > #include "tb.h" > > > > @@ -1518,6 +1519,130 @@ bool usb4_port_clx_supported(struct tb_port *port) > > > > return !!(val & PORT_CS_18_CPS); > > > > } > > > > > > > > +static int __usb4_port_lrf_enable(struct tb_port *port, > > > > + bool enable, bool *was_enable) > > > > +{ > > > > + int ret; > > > > + u32 val; > > > > + > > > > + ret = tb_port_read(port, &val, TB_CFG_PORT, > > > > + port->cap_usb4 + PORT_CS_19, 1); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + *was_enable |= !!(val & PORT_CS_19_ELR); > > > > + > > > > + if (enable) > > > > + val |= PORT_CS_19_ELR; > > > > + else > > > > + val &= ~PORT_CS_19_ELR; > > > > + > > > > + ret = tb_port_write(port, &val, TB_CFG_PORT, > > > > + port->cap_usb4 + PORT_CS_19, 1); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + tb_port_dbg(port, "ELR %s\n", str_enabled_disabled(enable)); > > > > > > tb_port_dbg(port, "link recovery %s\n", ...) > > > > > > Ditto below. > > > > > > > + return 0; > > > > +} > > > > + > > > > +static int usb4_switch_lrf_enable(struct tb_switch *sw) > > > > +{ > > > > + bool was_enable = false; > > > > + struct tb_port *up, *down; > > > > + int ret; > > > > > > Reverse christmas tree: > > > > > > struct tb_port *up, *down; > > > bool was_enable = false; > > > int ret; > > > > > > Ditto everywhere. > > > > > > > + > > > > + up = tb_upstream_port(sw); > > > > + down = tb_switch_downstream_port(sw); > > > > + > > > > + ret = __usb4_port_lrf_enable(up, true, &was_enable); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + ret = __usb4_port_lrf_enable(down, true, &was_enable); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + tb_sw_dbg(sw, "ELR %s\n", str_enabled_disabled(true)); > > > > + > > > > + return 0; > > > > +} > > > > + > > > > +static int usb4_switch_lrf_disable(struct tb_switch *sw) > > > > +{ > > > > + bool was_enable = false; > > > > + struct tb_port *up, *down; > > > > + int ret; > > > > + > > > > + up = tb_upstream_port(sw); > > > > + down = tb_switch_downstream_port(sw); > > > > + > > > > + ret = __usb4_port_lrf_enable(up, false, &was_enable); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + ret = __usb4_port_lrf_enable(down, false, &was_enable); > > > > + if (ret) > > > > + return ret; > > > > + > > > > + tb_sw_dbg(sw, "ELR %s\n", str_enabled_disabled(false)); > > > > + > > > > + /* At least one ELR has been disabled */ > > > > + return was_enable ? 1 : 0; > > > > > > Just return 0 in case of success. > > > > > > > If we return 0 on success when disabling Link Recovery, we won’t be able > > to restore ELR to its previous state before the asymmetric transition. > > As a result, enabling Link Recovery will become a side effect of > > asymmetric transitions. I’m not sure—is that acceptable? > > I missed the fact that you are using the positive return value to indicated > whether link recovery was enabled. > > The port functions can return the previous value like: > > ret = usb4_port_link_recovery_enable(up); > > then ret contains < 0 in case of error, 0 if it was not enabled > 0 if it > was. > > > > > +} > > > > + > > > > +/** > > > > + * usb4_disable_lrf() - Enables Link Recovery flow up to host router > > > > + * @sw: Router to start > > > > + * > > > > + * Enables Link Recovery flow from @sw up to the host router. > > > > + * Returns true if every Link Recovery flow was successfully enabled. > > > > + */ > > > > +bool usb4_enable_lrf(struct tb_switch *sw) > > > > +{ > > > > + bool enabled = true; > > > > + > > > > + do { > > > > + if (usb4_switch_lrf_enable(sw) < 0) { > > > > + tb_sw_warn(sw, "failed to enable Link Recovery flow\n"); > > > > + enabled = false; > > > > + } > > > > + > > > > + sw = tb_switch_parent(sw); > > > > + } while (sw); > > > > + > > > > + return enabled; > > > > +} > > > > > > This should go to tb.c and called tb_enable_link_recovery() analogous to > > > tb_enable_clx() and it calls the low-level usb4_port_link_recovery.. > > > functions. > > > > > > Ditto below. > > > > > > > Just to confirm my understanding, here’s what I plan to do based on > > your instructions: > > > > I’ll add two port-level functions in usb4.c: > > > > int usb4_port_link_recovery_enable(struct tb_port *port); > > void usb4_port_link_recovery_disable(struct tb_port *port); > > Yes. > > > And two functions in tb.c: > > > > int tb_enable_link_recovery(struct tb_switch *sw); > > int tb_disable_link_recovery(struct tb_switch *sw); > > > > It can even be one function to avoid duplicating code. > > > Both will return 0 on success. > > > > In tb_configure_asym(), I won’t be using the return value from > > either of these two functions. > > It should be checking the the return value and act accordingly. For example > what happens if disabling link recovery fails? Is that error situation or > should we continue configuring asymmetric link? Think also from users' > perspective. > Thanks again for the clear explanation. I will do that. > > > > + > > > > +/** > > > > + * usb4_disable_lrf() - Disable Link Recovery flow up to host router > > > > + * @sw: Router to start > > > > + * > > > > + * Disables Link Recovery flow from @sw up to the host router. > > > > + * Returns true if any Link Recovery flow was disabled. This > > > > + * can be used to figure out whether the link was setup by us > > > > + * or the boot firmware so we don't accidentally enable them if > > > > + * they were not enabled during discovery. > > > > > > Okay I think you copied the CLx part here, no? How did you test this? > > > > > > > The way we should handle Link Recovery seems quite similar to CL states. > > Yeah but you copied > > * can be used to figure out whether the link was setup by us > * or the boot firmware so we don't accidentally enable them if > * they were not enabled during discovery. > > and that does not even apply for USB4 v2 host routers by default since we > don't do discovery by default. > My bad. Didn't know. > > I should have mentioned this in my first email, unfortunately, > > I don’t have access to any Gen 4 hosts or devices for testing. > > I think this needs to get some testing on real hardware. I have here some > so I can try the next version. Great! Could you let me know what you are using for testing?