Messages in this thread | | | Date | Fri, 23 Sep 2022 11:08:30 -0500 | Subject | Re: [PATCH v2] thunderbolt: Explicitly enable lane adapter hotplug events at startup | From | "Limonciello, Mario" <> |
| |
On 9/23/2022 04:16, Mika Westerberg wrote: > Hi Mario, > > On Thu, Sep 22, 2022 at 11:07:29AM -0500, Mario Limonciello wrote: >> Software that has run before the USB4 CM in Linux runs may have disabled >> hotplug events for a given lane adapter. >> >> Other CMs such as that one distributed with Windows 11 will enable hotplug >> events. Do the same thing in the Linux CM which fixes hotplug events on >> "AMD Pink Sardine". >> >> Cc: stable@vger.kernel.org >> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com> >> --- >> v1->v2: >> * Only send second patch as first was merged already >> * s/usb4_enable_hotplug/usb4_port_hotplug_enable/ >> * Clarify intended users in documentation comment >> * Only call for lane adapters >> * Add stable tag >> >> drivers/thunderbolt/switch.c | 4 ++++ >> drivers/thunderbolt/tb.h | 1 + >> drivers/thunderbolt/tb_regs.h | 1 + >> drivers/thunderbolt/usb4.c | 20 ++++++++++++++++++++ >> 4 files changed, 26 insertions(+) >> >> diff --git a/drivers/thunderbolt/switch.c b/drivers/thunderbolt/switch.c >> index 77d7f07ca075..3213239d12c8 100644 >> --- a/drivers/thunderbolt/switch.c >> +++ b/drivers/thunderbolt/switch.c >> @@ -778,6 +778,10 @@ static int tb_init_port(struct tb_port *port) >> >> if (!tb_port_read(port, &hop, TB_CFG_HOPS, 0, 2)) >> port->ctl_credits = hop.initial_credits; >> + >> + res = usb4_port_hotplug_enable(port); >> + if (res) > > I think this does not belong here in tb_init_port(). This is called from > both FW and SW CM paths and we don't want to confuse the FW CM more than > necessary ;-) > > So instead I think this should be added to tb_plug_events_active(). >
The problem with that location is that tb_plug_events_active() is called from tb_switch_configure() which is before tb_switch_add() is called. tb_switch_add() calls tb_init_port() which reads port->config for the first time.
So if this is only to be called in tb_switch_configure() it means reading port->config "earlier" too.
So it definitely needs to be called in tb_init_port() or a later function but before the device is announced to satisfy only running on the appropriate port types.
tb_init_port() or tb_switch_add feels like the right place to me. How about leaving it where it is but guarding with a "if (!tb_switch_is_icm())" to avoid the risk to the FW CM case?
> Otherwise looks good to me. > >> + return res; >> } >> if (!port->ctl_credits) >> port->ctl_credits = 2; >> diff --git a/drivers/thunderbolt/tb.h b/drivers/thunderbolt/tb.h >> index 5db76de40cc1..332159f984fc 100644 >> --- a/drivers/thunderbolt/tb.h >> +++ b/drivers/thunderbolt/tb.h >> @@ -1174,6 +1174,7 @@ int usb4_switch_add_ports(struct tb_switch *sw); >> void usb4_switch_remove_ports(struct tb_switch *sw); >> >> int usb4_port_unlock(struct tb_port *port); >> +int usb4_port_hotplug_enable(struct tb_port *port); >> int usb4_port_configure(struct tb_port *port); >> void usb4_port_unconfigure(struct tb_port *port); >> int usb4_port_configure_xdomain(struct tb_port *port); >> diff --git a/drivers/thunderbolt/tb_regs.h b/drivers/thunderbolt/tb_regs.h >> index 166054110388..bbe38b2d9057 100644 >> --- a/drivers/thunderbolt/tb_regs.h >> +++ b/drivers/thunderbolt/tb_regs.h >> @@ -308,6 +308,7 @@ struct tb_regs_port_header { >> #define ADP_CS_5 0x05 >> #define ADP_CS_5_LCA_MASK GENMASK(28, 22) >> #define ADP_CS_5_LCA_SHIFT 22 >> +#define ADP_CS_5_DHP BIT(31) >> >> /* TMU adapter registers */ >> #define TMU_ADP_CS_3 0x03 >> diff --git a/drivers/thunderbolt/usb4.c b/drivers/thunderbolt/usb4.c >> index 3a2e7126db9d..f0b5a8f1ed3a 100644 >> --- a/drivers/thunderbolt/usb4.c >> +++ b/drivers/thunderbolt/usb4.c >> @@ -1046,6 +1046,26 @@ int usb4_port_unlock(struct tb_port *port) >> return tb_port_write(port, &val, TB_CFG_PORT, ADP_CS_4, 1); >> } >> >> +/** >> + * usb4_port_hotplug_enable() - Enables hotplug for a port >> + * @port: USB4 port to operate on >> + * >> + * Enables hot plug events on a given port. This is only intended >> + * to be used on lane, DP-IN, and DP-OUT adapters. >> + */ >> +int usb4_port_hotplug_enable(struct tb_port *port) >> +{ >> + int ret; >> + u32 val; >> + >> + ret = tb_port_read(port, &val, TB_CFG_PORT, ADP_CS_5, 1); >> + if (ret) >> + return ret; >> + >> + val &= ~ADP_CS_5_DHP; >> + return tb_port_write(port, &val, TB_CFG_PORT, ADP_CS_5, 1); >> +} >> + >> static int usb4_port_set_configured(struct tb_port *port, bool configured) >> { >> int ret; >> -- >> 2.34.1
| |