[PATCH 1/1] usb: dwc3: let role switch handle initial state

[PATCH 1/1] usb: dwc3: let role switch handle initial state

[Alexander Stein]

From: Markus Niebel <Markus.Niebel@ew.tq-group.com>

When we have a role switch device attached, we should not configure our
initial role. Leave this up to the role switch device, that should
detect and signal the initial role.

This fixes situations where a Type-A plug is connected already when the
driver is loaded but the default role is set to none or device. In this
case only an disconnect / reconnect gets the correct mode.

Signed-off-by: Markus Niebel <Markus.Niebel@ew.tq-group.com>
Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
---
I couldn't find any information whether USB role switch drivers are
supposed to call usb_role_switch_set_role() during their probe.
But this seems sensible, otherwise the actual/initial state is unknown.

 drivers/usb/dwc3/drd.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index 039bf241769af..1c2e504a5d8ba 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -496,15 +496,8 @@ static enum usb_role dwc3_usb_role_switch_get(struct usb_role_switch *sw)
 static int dwc3_setup_role_switch(struct dwc3 *dwc)
 {
 	struct usb_role_switch_desc dwc3_role_switch = {NULL};
-	u32 mode;
 
 	dwc->role_switch_default_mode = usb_get_role_switch_default_mode(dwc->dev);
-	if (dwc->role_switch_default_mode == USB_DR_MODE_HOST) {
-		mode = DWC3_GCTL_PRTCAP_HOST;
-	} else {
-		dwc->role_switch_default_mode = USB_DR_MODE_PERIPHERAL;
-		mode = DWC3_GCTL_PRTCAP_DEVICE;
-	}
 
 	dwc3_role_switch.fwnode = dev_fwnode(dwc->dev);
 	dwc3_role_switch.set = dwc3_usb_role_switch_set;
@@ -526,7 +519,11 @@ static int dwc3_setup_role_switch(struct dwc3 *dwc)
 		}
 	}
 
-	dwc3_set_mode(dwc, mode);
+	/*
+	 * usb_role_switch should implement initial detection and call
+	 * dwc3_usb_role_switch_set to get the state machine running
+	 */
+
 	return 0;
 }
 #else
-- 
2.34.1

Re: [PATCH 1/1] usb: dwc3: let role switch handle initial state

[Thinh Nguyen]

Hi,

On Tue, Oct 17, 2023, Alexander Stein wrote:
> From: Markus Niebel <Markus.Niebel@ew.tq-group.com>
> 
> When we have a role switch device attached, we should not configure our
> initial role. Leave this up to the role switch device, that should
> detect and signal the initial role.
> 
> This fixes situations where a Type-A plug is connected already when the
> driver is loaded but the default role is set to none or device. In this
> case only an disconnect / reconnect gets the correct mode.

If the default role is none, why isn't there a notification to update
the role on initialization from the connector? The current role should
not be none.

BR,
Thinh

> 
> Signed-off-by: Markus Niebel <Markus.Niebel@ew.tq-group.com>
> Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> ---
> I couldn't find any information whether USB role switch drivers are
> supposed to call usb_role_switch_set_role() during their probe.
> But this seems sensible, otherwise the actual/initial state is unknown.
> 
>  drivers/usb/dwc3/drd.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
> index 039bf241769af..1c2e504a5d8ba 100644
> --- a/drivers/usb/dwc3/drd.c
> +++ b/drivers/usb/dwc3/drd.c
> @@ -496,15 +496,8 @@ static enum usb_role dwc3_usb_role_switch_get(struct usb_role_switch *sw)
>  static int dwc3_setup_role_switch(struct dwc3 *dwc)
>  {
>  	struct usb_role_switch_desc dwc3_role_switch = {NULL};
> -	u32 mode;
>  
>  	dwc->role_switch_default_mode = usb_get_role_switch_default_mode(dwc->dev);
> -	if (dwc->role_switch_default_mode == USB_DR_MODE_HOST) {
> -		mode = DWC3_GCTL_PRTCAP_HOST;
> -	} else {
> -		dwc->role_switch_default_mode = USB_DR_MODE_PERIPHERAL;
> -		mode = DWC3_GCTL_PRTCAP_DEVICE;
> -	}
>  
>  	dwc3_role_switch.fwnode = dev_fwnode(dwc->dev);
>  	dwc3_role_switch.set = dwc3_usb_role_switch_set;
> @@ -526,7 +519,11 @@ static int dwc3_setup_role_switch(struct dwc3 *dwc)
>  		}
>  	}
>  
> -	dwc3_set_mode(dwc, mode);
> +	/*
> +	 * usb_role_switch should implement initial detection and call
> +	 * dwc3_usb_role_switch_set to get the state machine running
> +	 */
> +
>  	return 0;
>  }
>  #else
> -- 
> 2.34.1
> 

Re: [PATCH 1/1] usb: dwc3: let role switch handle initial state

[Alexander Stein]

Hi,

Am Samstag, 21. Oktober 2023, 00:43:39 CEST schrieb Thinh Nguyen:
> Hi,
> 
> On Tue, Oct 17, 2023, Alexander Stein wrote:
> 
> > From: Markus Niebel <Markus.Niebel@ew.tq-group.com>
> > 
> > When we have a role switch device attached, we should not configure our
> > initial role. Leave this up to the role switch device, that should
> > detect and signal the initial role.
> > 
> > This fixes situations where a Type-A plug is connected already when the
> > driver is loaded but the default role is set to none or device. In this
> > case only an disconnect / reconnect gets the correct mode.
> 
> 
> If the default role is none, why isn't there a notification to update
> the role on initialization from the connector? The current role should
> not be none.

dwc->role_switch_default_mode can only be none if the DT is misconfigured, 
e.g. role-switch-default-mode = "";

Calls to usb_role_switch_set_role() from usb role switch drivers will not 
affect dwc->role_switch_default_mode.
I'm wondering if checking for a misconfigured DT is sensible. But this will be 
detected by 'make dtbs_check'.

Best regards,
Alexander

> 
> BR,
> Thinh
> 
> 
> > 
> > Signed-off-by: Markus Niebel <Markus.Niebel@ew.tq-group.com>
> > Signed-off-by: Alexander Stein <alexander.stein@ew.tq-group.com>
> > ---
> > I couldn't find any information whether USB role switch drivers are
> > supposed to call usb_role_switch_set_role() during their probe.
> > But this seems sensible, otherwise the actual/initial state is unknown.
> > 
> > 
> >  drivers/usb/dwc3/drd.c | 13 +++++--------
> >  1 file changed, 5 insertions(+), 8 deletions(-)
> > 
> > 
> > diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
> > index 039bf241769af..1c2e504a5d8ba 100644
> > --- a/drivers/usb/dwc3/drd.c
> > +++ b/drivers/usb/dwc3/drd.c
> > @@ -496,15 +496,8 @@ static enum usb_role dwc3_usb_role_switch_get(struct
> > usb_role_switch *sw)
> 
> >  static int dwc3_setup_role_switch(struct dwc3 *dwc)
> >  {
> >  
> >  	struct usb_role_switch_desc dwc3_role_switch = {NULL};
> > 
> > -	u32 mode;
> > 
> >  
> >  
> >  	dwc->role_switch_default_mode =
> >  	usb_get_role_switch_default_mode(dwc->dev);
> 
> > -	if (dwc->role_switch_default_mode == USB_DR_MODE_HOST) {
> > -		mode = DWC3_GCTL_PRTCAP_HOST;
> > -	} else {
> > -		dwc->role_switch_default_mode = USB_DR_MODE_PERIPHERAL;
> > -		mode = DWC3_GCTL_PRTCAP_DEVICE;
> > -	}
> > 
> >  
> >  
> >  	dwc3_role_switch.fwnode = dev_fwnode(dwc->dev);
> >  	dwc3_role_switch.set = dwc3_usb_role_switch_set;
> > 
> > @@ -526,7 +519,11 @@ static int dwc3_setup_role_switch(struct dwc3 *dwc)
> > 
> >  		}
> >  	
> >  	}
> >  
> >  
> > 
> > -	dwc3_set_mode(dwc, mode);
> > +	/*
> > +	 * usb_role_switch should implement initial detection and call
> > +	 * dwc3_usb_role_switch_set to get the state machine running
> > +	 */
> > +
> > 
> >  	return 0;
> >  
> >  }
> >  #else
> > 
> > -- 
> > 2.34.1


-- 
TQ-Systems GmbH | Mühlstraße 2, Gut Delling | 82229 Seefeld, Germany
Amtsgericht München, HRB 105018
Geschäftsführer: Detlef Schneider, Rüdiger Stahl, Stefan Schneider
http://www.tq-group.com/

Re: [PATCH 1/1] usb: dwc3: let role switch handle initial state

[Thinh Nguyen]

On Wed, Oct 25, 2023, Alexander Stein wrote:
> Hi,
> 
> Am Samstag, 21. Oktober 2023, 00:43:39 CEST schrieb Thinh Nguyen:
> > Hi,
> > 
> > On Tue, Oct 17, 2023, Alexander Stein wrote:
> > 
> > > From: Markus Niebel <Markus.Niebel@ew.tq-group.com>
> > > 
> > > When we have a role switch device attached, we should not configure our
> > > initial role. Leave this up to the role switch device, that should
> > > detect and signal the initial role.
> > > 
> > > This fixes situations where a Type-A plug is connected already when the
> > > driver is loaded but the default role is set to none or device. In this
> > > case only an disconnect / reconnect gets the correct mode.
> > 
> > 
> > If the default role is none, why isn't there a notification to update
> > the role on initialization from the connector? The current role should
> > not be none.
> 
> dwc->role_switch_default_mode can only be none if the DT is misconfigured, 
> e.g. role-switch-default-mode = "";

No, I was referring to the current role, which is detected after
initialization. The current role should not be none. If the connector
has not notified the controller of the current role, the dwc3 driver
would set the controller to either host or device mode. The connector
can notify the dwc3 driver if it detected the current role on its
initialization.

> 
> Calls to usb_role_switch_set_role() from usb role switch drivers will not 
> affect dwc->role_switch_default_mode.
> I'm wondering if checking for a misconfigured DT is sensible. But this will be 
> detected by 'make dtbs_check'.
> 

I see that you just pushed out the fix patch for the actual issue, which
looks more reasonable.

Thanks,
Thinh