| Message ID | 20200615142645.56209-10-mika.westerberg@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | None | expand |
Hi Mika, Sorry for the late comment.. On Mon, Jun 15, 2020 at 05:26:37PM +0300, Mika Westerberg wrote: > USB3 tunneling is possible only over USB4 link so don't create USB3 > tunnels if that's not the case. > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > drivers/thunderbolt/tb.c | 3 +++ > drivers/thunderbolt/tb.h | 2 ++ > drivers/thunderbolt/tb_regs.h | 1 + > drivers/thunderbolt/usb4.c | 24 +++++++++++++++++++++--- > 4 files changed, 27 insertions(+), 3 deletions(-) > > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c > index 55daa7f1a87d..2da82259e77c 100644 > --- a/drivers/thunderbolt/tb.c > +++ b/drivers/thunderbolt/tb.c > @@ -235,6 +235,9 @@ static int tb_tunnel_usb3(struct tb *tb, struct tb_switch *sw) > if (!up) > return 0; > > + if (!sw->link_usb4) > + return 0; On both here and the previous "up" check; should we be returning 0? Wouldn't it be better to return an appropriate error code? It sounds like 0 is considered a success.... Best regards, -Prashant > + > /* >
On Thu, Jul 16, 2020 at 11:16:00PM -0700, Prashant Malani wrote: > Hi Mika, > > Sorry for the late comment.. Sorry for the late reply, was on vacation ;-) > On Mon, Jun 15, 2020 at 05:26:37PM +0300, Mika Westerberg wrote: > > USB3 tunneling is possible only over USB4 link so don't create USB3 > > tunnels if that's not the case. > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > --- > > drivers/thunderbolt/tb.c | 3 +++ > > drivers/thunderbolt/tb.h | 2 ++ > > drivers/thunderbolt/tb_regs.h | 1 + > > drivers/thunderbolt/usb4.c | 24 +++++++++++++++++++++--- > > 4 files changed, 27 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c > > index 55daa7f1a87d..2da82259e77c 100644 > > --- a/drivers/thunderbolt/tb.c > > +++ b/drivers/thunderbolt/tb.c > > @@ -235,6 +235,9 @@ static int tb_tunnel_usb3(struct tb *tb, struct tb_switch *sw) > > if (!up) > > return 0; > > > > + if (!sw->link_usb4) > > + return 0; > On both here and the previous "up" check; should we be returning 0? > Wouldn't it be better to return an appropriate error code? It sounds > like 0 is considered a success.... The idea here is that you can call this function for every type of router (can be one without USB3 adapters so TBT 3,1,2) and it creates the tunnel if conditions for USB3 tunneling are met. It is not considered an error. However, if the operations fail for some reason we return appropriate error code.
Hi Mika, On Mon, Jul 20, 2020 at 2:02 AM Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > > On Thu, Jul 16, 2020 at 11:16:00PM -0700, Prashant Malani wrote: > > Hi Mika, > > > > Sorry for the late comment.. > > Sorry for the late reply, was on vacation ;-) > > > On Mon, Jun 15, 2020 at 05:26:37PM +0300, Mika Westerberg wrote: > > > USB3 tunneling is possible only over USB4 link so don't create USB3 > > > tunnels if that's not the case. > > > > > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > > --- > > > drivers/thunderbolt/tb.c | 3 +++ > > > drivers/thunderbolt/tb.h | 2 ++ > > > drivers/thunderbolt/tb_regs.h | 1 + > > > drivers/thunderbolt/usb4.c | 24 +++++++++++++++++++++--- > > > 4 files changed, 27 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c > > > index 55daa7f1a87d..2da82259e77c 100644 > > > --- a/drivers/thunderbolt/tb.c > > > +++ b/drivers/thunderbolt/tb.c > > > @@ -235,6 +235,9 @@ static int tb_tunnel_usb3(struct tb *tb, struct tb_switch *sw) > > > if (!up) > > > return 0; > > > > > > + if (!sw->link_usb4) > > > + return 0; > > On both here and the previous "up" check; should we be returning 0? > > Wouldn't it be better to return an appropriate error code? It sounds > > like 0 is considered a success.... > > The idea here is that you can call this function for every type of > router (can be one without USB3 adapters so TBT 3,1,2) and it creates > the tunnel if conditions for USB3 tunneling are met. It is not > considered an error. > > However, if the operations fail for some reason we return appropriate > error code. Got it. Thanks for the explanation! BR, -Prashant
diff --git a/drivers/thunderbolt/tb.c b/drivers/thunderbolt/tb.c index 55daa7f1a87d..2da82259e77c 100644 --- a/drivers/thunderbolt/tb.c +++ b/drivers/thunderbolt/tb.c @@ -235,6 +235,9 @@ static int tb_tunnel_usb3(struct tb *tb, struct tb_switch *sw) if (!up) return 0; + if (!sw->link_usb4) + return 0; + /* * Look up available down port. Since we are chaining it should * be found right above this switch. diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h index b53ef5be7263..de8124949eaf 100644 --- a/drivers/thunderbolt/tb.h +++ b/drivers/thunderbolt/tb.h @@ -97,6 +97,7 @@ struct tb_switch_tmu { * @device_name: Name of the device (or %NULL if not known) * @link_speed: Speed of the link in Gb/s * @link_width: Width of the link (1 or 2) + * @link_usb4: Upstream link is USB4 * @generation: Switch Thunderbolt generation * @cap_plug_events: Offset to the plug events capability (%0 if not found) * @cap_lc: Offset to the link controller capability (%0 if not found) @@ -136,6 +137,7 @@ struct tb_switch { const char *device_name; unsigned int link_speed; unsigned int link_width; + bool link_usb4; unsigned int generation; int cap_plug_events; int cap_lc; diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h index c29c5075525a..77d4b8598835 100644 --- a/drivers/thunderbolt/tb_regs.h +++ b/drivers/thunderbolt/tb_regs.h @@ -290,6 +290,7 @@ struct tb_regs_port_header { /* USB4 port registers */ #define PORT_CS_18 0x12 #define PORT_CS_18_BE BIT(8) +#define PORT_CS_18_TCM BIT(9) #define PORT_CS_19 0x13 #define PORT_CS_19_PC BIT(3) diff --git a/drivers/thunderbolt/usb4.c b/drivers/thunderbolt/usb4.c index 50c7534ba31e..393771d50962 100644 --- a/drivers/thunderbolt/usb4.c +++ b/drivers/thunderbolt/usb4.c @@ -192,6 +192,20 @@ static int usb4_switch_op(struct tb_switch *sw, u16 opcode, u8 *status) return 0; } +static bool link_is_usb4(struct tb_port *port) +{ + u32 val; + + if (!port->cap_usb4) + return false; + + if (tb_port_read(port, &val, TB_CFG_PORT, + port->cap_usb4 + PORT_CS_18, 1)) + return false; + + return !(val & PORT_CS_18_TCM); +} + /** * usb4_switch_setup() - Additional setup for USB4 device * @sw: USB4 router to setup @@ -205,6 +219,7 @@ static int usb4_switch_op(struct tb_switch *sw, u16 opcode, u8 *status) */ int usb4_switch_setup(struct tb_switch *sw) { + struct tb_port *downstream_port; struct tb_switch *parent; bool tbt3, xhci; u32 val = 0; @@ -217,6 +232,11 @@ int usb4_switch_setup(struct tb_switch *sw) if (ret) return ret; + parent = tb_switch_parent(sw); + downstream_port = tb_port_at(tb_route(sw), parent); + sw->link_usb4 = link_is_usb4(downstream_port); + tb_sw_dbg(sw, "link: %s\n", sw->link_usb4 ? "USB4" : "TBT3"); + xhci = val & ROUTER_CS_6_HCI; tbt3 = !(val & ROUTER_CS_6_TNS); @@ -227,9 +247,7 @@ int usb4_switch_setup(struct tb_switch *sw) if (ret) return ret; - parent = tb_switch_parent(sw); - - if (tb_switch_find_port(parent, TB_TYPE_USB3_DOWN)) { + if (sw->link_usb4 && tb_switch_find_port(parent, TB_TYPE_USB3_DOWN)) { val |= ROUTER_CS_5_UTO; xhci = false; }
USB3 tunneling is possible only over USB4 link so don't create USB3 tunnels if that's not the case. Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com> --- drivers/thunderbolt/tb.c | 3 +++ drivers/thunderbolt/tb.h | 2 ++ drivers/thunderbolt/tb_regs.h | 1 + drivers/thunderbolt/usb4.c | 24 +++++++++++++++++++++--- 4 files changed, 27 insertions(+), 3 deletions(-)