Re: [PATCH 3/5] phy: tegra: xusb: Fix ordering issue when switching roles on USB2 ports

Diogo Ivo
2026-01-13T14:05:20Z

On 1/13/26 11:56, Jon Hunter wrote:
>
> On 04/12/2025 21:27, Diogo Ivo wrote:
>> The current implementation of USB2 role switching on Tegra relies on
>> whichever the previous USB controller driver was using the PHY to first
>> "yield" it back to USB_ROLE_NONE before the next controller configures
>> it for the new role. However, no mechanism to guarantee this ordering
>> was implemented, and currently, in the general case, the configuration
>> functions tegra_xhci_id_work() and tegra_xudc_usb_role_sw_work() end up
>> running in the same order regardless of the transition being HOST->DEVICE
>> or DEVICE->HOST, leading to one of these transitions ending up in a
>> non-working state due to the new configuration being clobbered by the
>> previous controller driver setting USB_ROLE_NONE after the fact.
>>
>> Fix this by introducing a helper that waits for the USB2 port’s current
>> role to become USB_ROLE_NONE and add it in the configuration functions
>> above before setting the role to either USB_ROLE_HOST or
>> USB_ROLE_DEVICE. The specific parameters of the helper function are
>> choices that seem reasonable in my testing and have no other basis.
>
> This is no information here about why 6 * 50/60us is deemed to be
> sufficient? May be it is, but a comment would be nice.
>
>> This was tested on a Tegra210 platform (Smaug). However, due to the
>> similar
>> approach in Tegra186 it is likely that not only this problem exists there
>> but that this patch also fixes it.
>>
>> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
>> ---
>> drivers/phy/tegra/xusb.c | 23 +++++++++++++++++++++++
>> drivers/usb/gadget/udc/tegra-xudc.c | 4 ++++
>> drivers/usb/host/xhci-tegra.c | 15 ++++++++++-----
>> include/linux/phy/tegra/xusb.h | 1 +
>> 4 files changed, 38 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/phy/tegra/xusb.c b/drivers/phy/tegra/xusb.c
>> index c89df95aa6ca..e05c3f2d1421 100644
>> --- a/drivers/phy/tegra/xusb.c
>> +++ b/drivers/phy/tegra/xusb.c
>> @@ -740,6 +740,29 @@ static void
>> tegra_xusb_parse_usb_role_default_mode(struct tegra_xusb_port *port)
>> }
>> }
>> +bool tegra_xusb_usb2_port_wait_role_none(struct tegra_xusb_padctl
>> *padctl, int index)
>> +{
>> + struct tegra_xusb_usb2_port *usb2 =
>> tegra_xusb_find_usb2_port(padctl,
>> + index);
>> + int retries = 5;
>> +
>> + if (!usb2) {
>> + dev_err(&usb2->base.dev, "no port found for USB2 lane %u\n",
>> index);
>
> This appears to be a bug. If !usb2 then dereference usb2->base anyway.

It is a bug, will fix in v2.

>> + return false;
>> + }
>> +
>> + do {
>> + if (usb2->role == USB_ROLE_NONE)
>> + return true;
>> +
>> + usleep_range(50, 60);
>> + } while (retries--);
>> +
>> + dev_err(&usb2->base.dev, "timed out waiting for USB_ROLE_NONE");
>> +
>> + return false;
>> +}
>> +
>> static int tegra_xusb_usb2_port_parse_dt(struct tegra_xusb_usb2_port
>> *usb2)
>> {
>> struct tegra_xusb_port *port = &usb2->base;
>> diff --git a/drivers/usb/gadget/udc/tegra-xudc.c b/drivers/usb/gadget/
>> udc/tegra-xudc.c
>> index 0c38fc37b6e6..72d725659e5f 100644
>> --- a/drivers/usb/gadget/udc/tegra-xudc.c
>> +++ b/drivers/usb/gadget/udc/tegra-xudc.c
>> @@ -698,8 +698,12 @@ static void tegra_xudc_restore_port_speed(struct
>> tegra_xudc *xudc)
>> static void tegra_xudc_device_mode_on(struct tegra_xudc *xudc)
>> {
>> + int port = tegra_xusb_padctl_get_port_number(xudc->curr_utmi_phy);
>> int err;
>> + if (!tegra_xusb_usb2_port_wait_role_none(xudc->padctl, port))
>> + return;
>> +
>> pm_runtime_get_sync(xudc->dev);
>> tegra_phy_xusb_utmi_pad_power_on(xudc->curr_utmi_phy);
>> diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-
>> tegra.c
>> index 9c69fccdc6e8..9944593166a3 100644
>> --- a/drivers/usb/host/xhci-tegra.c
>> +++ b/drivers/usb/host/xhci-tegra.c
>> @@ -1352,18 +1352,23 @@ static void tegra_xhci_id_work(struct
>> work_struct *work)
>> struct tegra_xusb_mbox_msg msg;
>> struct phy *phy = tegra_xusb_get_phy(tegra, "usb2",
>> tegra->otg_usb2_port);
>> + enum usb_role role = USB_ROLE_NONE;
>> u32 status;
>> int ret;
>> dev_dbg(tegra->dev, "host mode %s\n", str_on_off(tegra-
>> >host_mode));
>> - mutex_lock(&tegra->lock);
>
> Extra blank line here.

Will fix in v2.

>> - if (tegra->host_mode)
>> - phy_set_mode_ext(phy, PHY_MODE_USB_OTG, USB_ROLE_HOST);
>> - else
>> - phy_set_mode_ext(phy, PHY_MODE_USB_OTG, USB_ROLE_NONE);
>> + if (tegra->host_mode) {
>> + if (!tegra_xusb_usb2_port_wait_role_none(tegra->padctl,
>> + tegra->otg_usb2_port))
>> + return;
>> + role = USB_ROLE_HOST;
>> + }
>> +
>> + mutex_lock(&tegra->lock);
>> + phy_set_mode_ext(phy, PHY_MODE_USB_OTG, role);
>> mutex_unlock(&tegra->lock);
>
> I am trying to understand why you opted to implement it this way around
> and not add the wait loop after setting to the mode to USB_ROLE_NONE in
> the original code all within the context of the mutex?

I did that to minimize the amount of time we wait while holding the
mutex, as we can now possibly wait a significant amount of time for the
role switch. Is this an unneccessary optimization?

Thanks,
Diogo

> Thanks
> Jon