| Message ID | 20231017055726.904133-1-alexander.stein@ew.tq-group.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] usb: dwc3: let role switch handle initial state | expand |
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 >
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
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
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