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 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);
+		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);
 
-	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);
 
 	tegra->otg_usb3_port = tegra_xusb_padctl_get_usb3_companion(tegra->padctl,
diff --git a/include/linux/phy/tegra/xusb.h b/include/linux/phy/tegra/xusb.h
index 6ca51e0080ec..a0d3d5b7cf33 100644
--- a/include/linux/phy/tegra/xusb.h
+++ b/include/linux/phy/tegra/xusb.h
@@ -33,5 +33,6 @@ int tegra_xusb_padctl_disable_phy_sleepwalk(struct tegra_xusb_padctl *padctl, st
 int tegra_xusb_padctl_enable_phy_wake(struct tegra_xusb_padctl *padctl, struct phy *phy);
 int tegra_xusb_padctl_disable_phy_wake(struct tegra_xusb_padctl *padctl, struct phy *phy);
 bool tegra_xusb_padctl_remote_wake_detected(struct tegra_xusb_padctl *padctl, struct phy *phy);
+bool tegra_xusb_usb2_port_wait_role_none(struct tegra_xusb_padctl *padctl, int index);
 
 #endif /* PHY_TEGRA_XUSB_H */

-- 
2.52.0

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

Hi Jonathan,

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.

I missed this review comment and I'm not sure what you mean here. Do you
want me to comment on the commit message on how I chose these
parameters? If so it's as stated in the current message, I simply tested
with these parameters and it worked and I really have no better basis
for choosing them. If you mean adding a comment in the code I can do
that for v2.

Thanks,
Diogo

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

On 15/01/2026 11:06, Diogo Ivo wrote:
> Hi Jonathan,
> 
> 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.
> 
> I missed this review comment and I'm not sure what you mean here. Do you
> want me to comment on the commit message on how I chose these
> parameters? If so it's as stated in the current message, I simply tested
> with these parameters and it worked and I really have no better basis
> for choosing them. If you mean adding a comment in the code I can do
> that for v2.

Yes please be explicit about how you arrived at these numbers. Ie. based 
upon your testing on what platform, etc.

Thanks
Jon

-- 
nvpublic

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

On 13/01/2026 14:05, Diogo Ivo wrote:
> 
> 
> 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?

Do you mean it will be longer than a few 100us?

Jon

-- 
nvpublic

On 1/13/26 14:48, Jon Hunter wrote:
> 
> On 13/01/2026 14:05, Diogo Ivo wrote:
>>
>>
>> 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?
> 
> Do you mean it will be longer than a few 100us?

Currently the worst case in wait_role_none() is around 300us but again
this is simply because I chose the values with no criteria except that
in my testing they have worked thus far. Do you have access to any
internal documentation where the transition length is documented?

In any case I think that the underlying principle of minimizing the time
we hold the mutex is solid, no?

Diogo

> Jon
> 

On 13/01/2026 15:10, Diogo Ivo wrote:

...

>>>>> -    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?
>>
>> Do you mean it will be longer than a few 100us?
> 
> Currently the worst case in wait_role_none() is around 300us but again
> this is simply because I chose the values with no criteria except that
> in my testing they have worked thus far. Do you have access to any
> internal documentation where the transition length is documented?
> 
> In any case I think that the underlying principle of minimizing the time
> we hold the mutex is solid, no?

Yes, but it was unclear to me if there could be a case where we are 
waiting for USB_ROLE_NONE but we have already transitioned to host. May 
be that will never happen, but it was not clear.

Jon

-- 
nvpublic

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 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);
> +		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;
> +}


This patch is causing the following build error today with -next ...

   MODPOST Module.symvers
ERROR: modpost: "tegra_xusb_usb2_port_wait_role_none" 
[drivers/usb/host/xhci-tegra.ko] undefined!

The above function symbol needs to be exported.

Jon

-- 
nvpublic

On 13/01/2026 11:35, 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 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);
>> +        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;
>> +}
> 
> 
> This patch is causing the following build error today with -next ...

Sorry this is not in -next, I had just applied locally on top!

>    MODPOST Module.symvers
> ERROR: modpost: "tegra_xusb_usb2_port_wait_role_none" [drivers/usb/host/ 
> xhci-tegra.ko] undefined!
> 
> The above function symbol needs to be exported.

Nonetheless this needs to be fixed.

Jon

-- 
nvpublic

On 1/13/26 11:44, Jon Hunter wrote:
> 
> On 13/01/2026 11:35, 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 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);
>>> +        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;
>>> +}
>>
>>
>> This patch is causing the following build error today with -next ...
> 
> Sorry this is not in -next, I had just applied locally on top!
> 
>>    MODPOST Module.symvers
>> ERROR: modpost: "tegra_xusb_usb2_port_wait_role_none" [drivers/usb/ 
>> host/ xhci-tegra.ko] undefined!
>>
>> The above function symbol needs to be exported.

You're right, thanks for catching this. I'll fix it in v2.

> 
> Nonetheless this needs to be fixed.
> 
> Jon
>