Hi,
On Thu, Jan 30, 2025 at 09:51:09AM +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."
Looks good in general few comments still see below.
> Signed-off-by: Mohammad Rahimi <rahimi.mhmmd@xxxxxxxxx>
> ---
> drivers/thunderbolt/tb.c | 28 +++++++++++++++++--
> drivers/thunderbolt/tb.h | 3 ++
> drivers/thunderbolt/tb_regs.h | 1 +
> drivers/thunderbolt/usb4.c | 52 +++++++++++++++++++++++++++++++++++
> 4 files changed, 81 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c
> index a7c6919fbf97..31a8269a5156 100644
> --- a/drivers/thunderbolt/tb.c
> +++ b/drivers/thunderbolt/tb.c
> @@ -1013,6 +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 link_recovery_up = false, link_recovery_down = false;
> bool clx = false, clx_disabled = false, downstream;
> struct tb_switch *sw;
> struct tb_port *up;
> @@ -1075,15 +1076,29 @@ 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;
> }
>
> + ret = usb4_port_link_recovery_disable(up);
> + if (ret < 0) {
> + tb_port_warn(up, "failed to disable the link recovery\n");
failed to disable link recovery
also should we still continue? How catastrophic this actually is? I mean
this now prevents the user from using her new UHBR monitor even though it
probably works just fine even if we don't do this step.
> + break;
> + }
> + link_recovery_up = ret > 0;
> +
> + ret = usb4_port_link_recovery_disable(down);
> + if (ret < 0) {
> + tb_port_warn(down, "failed to disable the link recovery\n");
failed to disable link recovery
and actually you can move this message into the function itself and drop
these messages here.
> + break;
> + }
> + link_recovery_down = ret > 0;
> +
> tb_sw_dbg(up->sw, "configuring asymmetric link\n");
>
> /*
> @@ -1091,6 +1106,13 @@ static int tb_configure_asym(struct tb *tb, struct tb_port *src_port,
> * transtion the link into asymmetric now.
> */
> ret = tb_switch_set_link_width(up->sw, width_up);
> +
> + /* Re-enable Link Recovery flow if they were previosly enabled */
previously
> + if (link_recovery_up)
> + usb4_port_link_recovery_enable(up);
> + if (link_recovery_down)
> + usb4_port_link_recovery_enable(down);
> +
> if (ret) {
> tb_sw_warn(up->sw, "failed to set link width\n");
> break;
> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h
> index ddbf0cd78377..d37d778082fc 100644
> --- a/drivers/thunderbolt/tb.h
> +++ b/drivers/thunderbolt/tb.h
> @@ -1332,6 +1332,9 @@ int usb4_port_router_online(struct tb_port *port);
> int usb4_port_enumerate_retimers(struct tb_port *port);
> bool usb4_port_clx_supported(struct tb_port *port);
>
> +int usb4_port_link_recovery_enable(struct tb_port *port);
> +int usb4_port_link_recovery_disable(struct tb_port *port);
> +
> 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);
> 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..99fd6aa1704e 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,57 @@ bool usb4_port_clx_supported(struct tb_port *port)
> return !!(val & PORT_CS_18_CPS);
> }
>
> +static int __usb4_port_link_recovery_enable(struct tb_port *port, bool enable)
> +{
> + bool was_enable;
> + int ret;
> + u32 val;
I think you should check here
if (!port->cap_usb4)
return -EINVAL;
> +
> + 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, "link recovery %s\n", str_enabled_disabled(enable));
> + return was_enable ? 1 : 0;
> +}
> +
> +/**
> + * usb4_port_link_recovery_enable() - Enable the Link Recovery
> + * @port: USB4 port
> + *
> + * Enables the Link Recovery for @port.
> + * Returns -ERRNO on failure, otherwise the previous state of the Link Recovery.
Returns errno on failure..
or you can also specify which errors but then use %-EINVAL and so on.
Ditto to the other function.
> + */
> +int usb4_port_link_recovery_enable(struct tb_port *port)
> +{
> + return __usb4_port_link_recovery_enable(port, true);
> +}
> +
> +/**
> + * usb4_port_link_recovery_disable() - Disable the Link Recovery
> + * @port: USB4 port
> + *
> + * Disables the Link Recovery for @port.
> + * Returns -ERRNO on failure, otherwise the previous state of the Link Recovery.
> + */
> +int usb4_port_link_recovery_disable(struct tb_port *port)
> +{
> + return __usb4_port_link_recovery_enable(port, false);
> +}
> +
> /**
> * usb4_port_asym_supported() - If the port supports asymmetric link
> * @port: USB4 port
> --
> 2.45.2
