| Message ID | 20191113141521.1696-1-b-liu@ti.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: dwc3: turn off VBUS when leaving host mode | expand |
Bin, On 13/11/2019 16:15, Bin Liu wrote: > VBUS should be turned off when leaving the host mode. > Set GCTL_PRTCAP to device mode in teardown to de-assert DRVVBUS pin to > turn off VBUS power. Doesn't DRVVBUS pin de-assert when root hub driver turns off port power? Which should happen when XHCI host driver is unregistered as part of dwc3_host_exit()? cheers, -roger > > Fixes: 5f94adfeed97 ("usb: dwc3: core: refactor mode initialization to its own function") > Signed-off-by: Bin Liu <b-liu@ti.com> > --- > drivers/usb/dwc3/core.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 97d6ae3c4df2..76ac9cd54e64 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1201,6 +1201,7 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc) > break; > case USB_DR_MODE_HOST: > dwc3_host_exit(dwc); > + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > break; > case USB_DR_MODE_OTG: > dwc3_drd_exit(dwc); >
Roger, On Thu, Nov 14, 2019 at 10:48:36AM +0200, Roger Quadros wrote: > Bin, > > On 13/11/2019 16:15, Bin Liu wrote: > > VBUS should be turned off when leaving the host mode. > > Set GCTL_PRTCAP to device mode in teardown to de-assert DRVVBUS pin to > > turn off VBUS power. > > Doesn't DRVVBUS pin de-assert when root hub driver turns off port power? > Which should happen when XHCI host driver is unregistered as part of > dwc3_host_exit()? No, DRVVBUS is still 3.3v after 'modprbe -r dwc3-omap' on TI AM57x platform if this patch is not applied. -Bin.
Hi, Bin Liu <b-liu@ti.com> writes: > VBUS should be turned off when leaving the host mode. > Set GCTL_PRTCAP to device mode in teardown to de-assert DRVVBUS pin to > turn off VBUS power. > > Fixes: 5f94adfeed97 ("usb: dwc3: core: refactor mode initialization to its own function") > Signed-off-by: Bin Liu <b-liu@ti.com> > --- > drivers/usb/dwc3/core.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 97d6ae3c4df2..76ac9cd54e64 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1201,6 +1201,7 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc) > break; > case USB_DR_MODE_HOST: > dwc3_host_exit(dwc); > + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); seems like this should be done as part of dwc3_host_exit()
On 18/11/2019 09:07, Felipe Balbi wrote: > Hi, > > Bin Liu <b-liu@ti.com> writes: > >> VBUS should be turned off when leaving the host mode. >> Set GCTL_PRTCAP to device mode in teardown to de-assert DRVVBUS pin to >> turn off VBUS power. >> >> Fixes: 5f94adfeed97 ("usb: dwc3: core: refactor mode initialization to its own function") >> Signed-off-by: Bin Liu <b-liu@ti.com> >> --- >> drivers/usb/dwc3/core.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 97d6ae3c4df2..76ac9cd54e64 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -1201,6 +1201,7 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc) >> break; >> case USB_DR_MODE_HOST: >> dwc3_host_exit(dwc); >> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > > seems like this should be done as part of dwc3_host_exit() > That wouldn't work well with OTG case as dwc3_host_exit(dwc) is called when switching roles and we don't want PRTCAP to change from DWC3_GCTL_PRTCAP_OTG. cheers, -roger
Hi, Roger Quadros <rogerq@ti.com> writes: > On 18/11/2019 09:07, Felipe Balbi wrote: >> Hi, >> >> Bin Liu <b-liu@ti.com> writes: >> >>> VBUS should be turned off when leaving the host mode. >>> Set GCTL_PRTCAP to device mode in teardown to de-assert DRVVBUS pin to >>> turn off VBUS power. >>> >>> Fixes: 5f94adfeed97 ("usb: dwc3: core: refactor mode initialization to its own function") >>> Signed-off-by: Bin Liu <b-liu@ti.com> >>> --- >>> drivers/usb/dwc3/core.c | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >>> index 97d6ae3c4df2..76ac9cd54e64 100644 >>> --- a/drivers/usb/dwc3/core.c >>> +++ b/drivers/usb/dwc3/core.c >>> @@ -1201,6 +1201,7 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc) >>> break; >>> case USB_DR_MODE_HOST: >>> dwc3_host_exit(dwc); >>> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); >> >> seems like this should be done as part of dwc3_host_exit() >> > > That wouldn't work well with OTG case as dwc3_host_exit(dwc) is > called when switching roles and we don't want PRTCAP > to change from DWC3_GCTL_PRTCAP_OTG. if (port != OTG) set_prtcap(DEVICE)
Hi Felipe, On Mon, Dec 02, 2019 at 09:41:42AM +0200, Felipe Balbi wrote: > > Hi, > > Roger Quadros <rogerq@ti.com> writes: > > On 18/11/2019 09:07, Felipe Balbi wrote: > >> Hi, > >> > >> Bin Liu <b-liu@ti.com> writes: > >> > >>> VBUS should be turned off when leaving the host mode. > >>> Set GCTL_PRTCAP to device mode in teardown to de-assert DRVVBUS pin to > >>> turn off VBUS power. > >>> > >>> Fixes: 5f94adfeed97 ("usb: dwc3: core: refactor mode initialization to its own function") > >>> Signed-off-by: Bin Liu <b-liu@ti.com> > >>> --- > >>> drivers/usb/dwc3/core.c | 1 + > >>> 1 file changed, 1 insertion(+) > >>> > >>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > >>> index 97d6ae3c4df2..76ac9cd54e64 100644 > >>> --- a/drivers/usb/dwc3/core.c > >>> +++ b/drivers/usb/dwc3/core.c > >>> @@ -1201,6 +1201,7 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc) > >>> break; > >>> case USB_DR_MODE_HOST: > >>> dwc3_host_exit(dwc); > >>> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > >> > >> seems like this should be done as part of dwc3_host_exit() > >> > > > > That wouldn't work well with OTG case as dwc3_host_exit(dwc) is > > called when switching roles and we don't want PRTCAP > > to change from DWC3_GCTL_PRTCAP_OTG. > > if (port != OTG) > set_prtcap(DEVICE) During init, the PRTCAP is set in dwc3_core_init_mode() besides dwc3_{host,gadget,drd}_init(). So for tearing down setting it in dwc3_core_exit_mode() would make the code logic symmetric and easy to understand. Also it turns out that setting PRTCAP is required for OTG mode too to de-assert DRVVBUS. If left GCTL[PRTCAP] to OTG, grounding the ID pin would make the controller to assert DRVVBUS without any software involved. So the fix should be the following. Please let me know you comments. -Bin. -----------8<------------ diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 97d6ae3c4df2..cede7a8e3605 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -1209,6 +1209,9 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc) /* do nothing */ break; } + + /* de-assert DRVVBUS for HOST and OTG mode */ + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); } static void dwc3_get_properties(struct dwc3 *dwc) > > -- > balbi
Hi, Bin Liu <b-liu@ti.com> writes: >> >>> VBUS should be turned off when leaving the host mode. >> >>> Set GCTL_PRTCAP to device mode in teardown to de-assert DRVVBUS pin to >> >>> turn off VBUS power. >> >>> >> >>> Fixes: 5f94adfeed97 ("usb: dwc3: core: refactor mode initialization to its own function") >> >>> Signed-off-by: Bin Liu <b-liu@ti.com> >> >>> --- >> >>> drivers/usb/dwc3/core.c | 1 + >> >>> 1 file changed, 1 insertion(+) >> >>> >> >>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> >>> index 97d6ae3c4df2..76ac9cd54e64 100644 >> >>> --- a/drivers/usb/dwc3/core.c >> >>> +++ b/drivers/usb/dwc3/core.c >> >>> @@ -1201,6 +1201,7 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc) >> >>> break; >> >>> case USB_DR_MODE_HOST: >> >>> dwc3_host_exit(dwc); >> >>> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); >> >> >> >> seems like this should be done as part of dwc3_host_exit() >> >> >> > >> > That wouldn't work well with OTG case as dwc3_host_exit(dwc) is >> > called when switching roles and we don't want PRTCAP >> > to change from DWC3_GCTL_PRTCAP_OTG. >> >> if (port != OTG) >> set_prtcap(DEVICE) > > During init, the PRTCAP is set in dwc3_core_init_mode() besides > dwc3_{host,gadget,drd}_init(). So for tearing down setting it in > dwc3_core_exit_mode() would make the code logic symmetric and easy to > understand. Fair enough, makes sense. > Also it turns out that setting PRTCAP is required for OTG mode too to But, then, how do we go back to OTG? > de-assert DRVVBUS. If left GCTL[PRTCAP] to OTG, grounding the ID pin > would make the controller to assert DRVVBUS without any software > involved. So the fix should be the following. Please let me know you > comments. But that's expected, no? If port is OTG, then it must obey ID pin rules specified by OTG. IIRC, dwc3 is OTG 2.x compliant, at least the version TI uses.
On Tue, Dec 10, 2019 at 01:50:58PM +0200, Felipe Balbi wrote: > > Hi, > > Bin Liu <b-liu@ti.com> writes: > >> >>> VBUS should be turned off when leaving the host mode. > >> >>> Set GCTL_PRTCAP to device mode in teardown to de-assert DRVVBUS pin to > >> >>> turn off VBUS power. > >> >>> > >> >>> Fixes: 5f94adfeed97 ("usb: dwc3: core: refactor mode initialization to its own function") > >> >>> Signed-off-by: Bin Liu <b-liu@ti.com> > >> >>> --- > >> >>> drivers/usb/dwc3/core.c | 1 + > >> >>> 1 file changed, 1 insertion(+) > >> >>> > >> >>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > >> >>> index 97d6ae3c4df2..76ac9cd54e64 100644 > >> >>> --- a/drivers/usb/dwc3/core.c > >> >>> +++ b/drivers/usb/dwc3/core.c > >> >>> @@ -1201,6 +1201,7 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc) > >> >>> break; > >> >>> case USB_DR_MODE_HOST: > >> >>> dwc3_host_exit(dwc); > >> >>> + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > >> >> > >> >> seems like this should be done as part of dwc3_host_exit() > >> >> > >> > > >> > That wouldn't work well with OTG case as dwc3_host_exit(dwc) is > >> > called when switching roles and we don't want PRTCAP > >> > to change from DWC3_GCTL_PRTCAP_OTG. > >> > >> if (port != OTG) > >> set_prtcap(DEVICE) > > > > During init, the PRTCAP is set in dwc3_core_init_mode() besides > > dwc3_{host,gadget,drd}_init(). So for tearing down setting it in > > dwc3_core_exit_mode() would make the code logic symmetric and easy to > > understand. > > Fair enough, makes sense. > > > Also it turns out that setting PRTCAP is required for OTG mode too to > > But, then, how do we go back to OTG? This new setting (PRTCAP = DEVICE) is in dwc3_core_exit_mode() which is called in platform driver .remove(). So PRTCAP will be set to OTG for dr_mode = 'otg' in dwc3_core_init_mode() at the next time in the platform driver .probe(). > > > de-assert DRVVBUS. If left GCTL[PRTCAP] to OTG, grounding the ID pin > > would make the controller to assert DRVVBUS without any software > > involved. So the fix should be the following. Please let me know you > > comments. > > But that's expected, no? If port is OTG, then it must obey ID pin rules > specified by OTG. IIRC, dwc3 is OTG 2.x compliant, at least the version > TI uses. It is expected when the usb drivers are bind to the controller. I am trying to solve the issue that DRVVBUS is still asserted on both host port and otg port with ID pin grounded when the drivers are unbind from the controller (after modprobe -r dwc3-omap, DRVVBUS should be de-asserted). -Bin.
Hi, Bin Liu <b-liu@ti.com> writes: >> > de-assert DRVVBUS. If left GCTL[PRTCAP] to OTG, grounding the ID pin >> > would make the controller to assert DRVVBUS without any software >> > involved. So the fix should be the following. Please let me know you >> > comments. >> >> But that's expected, no? If port is OTG, then it must obey ID pin rules >> specified by OTG. IIRC, dwc3 is OTG 2.x compliant, at least the version >> TI uses. > > It is expected when the usb drivers are bind to the controller. I am > trying to solve the issue that DRVVBUS is still asserted on both host > port and otg port with ID pin grounded when the drivers are unbind from > the controller (after modprobe -r dwc3-omap, DRVVBUS should be > de-asserted). I see. Now I understand the scenario. Thanks for the explanation, Bin. Care to send an up-to-date patch so I can queue it?
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 97d6ae3c4df2..76ac9cd54e64 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -1201,6 +1201,7 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc) break; case USB_DR_MODE_HOST: dwc3_host_exit(dwc); + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); break; case USB_DR_MODE_OTG: dwc3_drd_exit(dwc);
VBUS should be turned off when leaving the host mode. Set GCTL_PRTCAP to device mode in teardown to de-assert DRVVBUS pin to turn off VBUS power. Fixes: 5f94adfeed97 ("usb: dwc3: core: refactor mode initialization to its own function") Signed-off-by: Bin Liu <b-liu@ti.com> --- drivers/usb/dwc3/core.c | 1 + 1 file changed, 1 insertion(+)