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

[Jon Hunter <jonathanh@nvidia.com>]

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.


> +		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.

> -	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?

Thanks
Jon

-- 
nvpublic