| Message ID | 1506511163-26026-1-git-send-email-mgautam@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | Andy Gross |
| Headers | show |
Hi Felipe, Let me know if patches in this series look fine to you. On 9/27/2017 4:49 PM, Manu Gautam wrote: > Driver powers-off PHYs and reinitializes DWC3 core and gadget on > resume. While this works fine for gadget mode but in host > mode there is not re-initialization of host stack. Also, resetting > bus as part of bus_suspend/resume is not correct which could affect > (or disconnect) connected devices. > Fix this by not reinitializing core on suspend/resume in host mode > for HOST only and OTG/drd configurations. > > Signed-off-by: Manu Gautam <mgautam@codeaurora.org> > --- > drivers/usb/dwc3/core.c | 43 ++++++++++++++++++++----------------------- > 1 file changed, 20 insertions(+), 23 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 03474d3..f75613f 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -927,6 +927,7 @@ static int dwc3_core_init_mode(struct dwc3 *dwc) > > switch (dwc->dr_mode) { > case USB_DR_MODE_PERIPHERAL: > + dwc->current_dr_role = DWC3_GCTL_PRTCAP_DEVICE; > dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > > if (dwc->usb2_phy) > @@ -942,6 +943,7 @@ static int dwc3_core_init_mode(struct dwc3 *dwc) > } > break; > case USB_DR_MODE_HOST: > + dwc->current_dr_role = DWC3_GCTL_PRTCAP_HOST; > dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST); > > if (dwc->usb2_phy) > @@ -1293,21 +1295,19 @@ static int dwc3_suspend_common(struct dwc3 *dwc) > { > unsigned long flags; > > - switch (dwc->dr_mode) { > - case USB_DR_MODE_PERIPHERAL: > - case USB_DR_MODE_OTG: > + switch (dwc->current_dr_role) { > + case DWC3_GCTL_PRTCAP_DEVICE: > spin_lock_irqsave(&dwc->lock, flags); > dwc3_gadget_suspend(dwc); > spin_unlock_irqrestore(&dwc->lock, flags); > + dwc3_core_exit(dwc); > break; > - case USB_DR_MODE_HOST: > + case DWC3_GCTL_PRTCAP_HOST: > default: > /* do nothing */ > break; > } > > - dwc3_core_exit(dwc); > - > return 0; > } > > @@ -1316,18 +1316,17 @@ static int dwc3_resume_common(struct dwc3 *dwc) > unsigned long flags; > int ret; > > - ret = dwc3_core_init(dwc); > - if (ret) > - return ret; > + switch (dwc->current_dr_role) { > + case DWC3_GCTL_PRTCAP_DEVICE: > + ret = dwc3_core_init(dwc); > + if (ret) > + return ret; > > - switch (dwc->dr_mode) { > - case USB_DR_MODE_PERIPHERAL: > - case USB_DR_MODE_OTG: > spin_lock_irqsave(&dwc->lock, flags); > dwc3_gadget_resume(dwc); > spin_unlock_irqrestore(&dwc->lock, flags); > - /* FALLTHROUGH */ > - case USB_DR_MODE_HOST: > + break; > + case DWC3_GCTL_PRTCAP_HOST: > default: > /* do nothing */ > break; > @@ -1338,7 +1337,7 @@ static int dwc3_resume_common(struct dwc3 *dwc) > > static int dwc3_runtime_checks(struct dwc3 *dwc) > { > - switch (dwc->dr_mode) { > + switch (dwc->current_dr_role) { > case USB_DR_MODE_PERIPHERAL: > case USB_DR_MODE_OTG: > if (dwc->connected) > @@ -1381,12 +1380,11 @@ static int dwc3_runtime_resume(struct device *dev) > if (ret) > return ret; > > - switch (dwc->dr_mode) { > - case USB_DR_MODE_PERIPHERAL: > - case USB_DR_MODE_OTG: > + switch (dwc->current_dr_role) { > + case DWC3_GCTL_PRTCAP_DEVICE: > dwc3_gadget_process_pending_events(dwc); > break; > - case USB_DR_MODE_HOST: > + case DWC3_GCTL_PRTCAP_HOST: > default: > /* do nothing */ > break; > @@ -1402,13 +1400,12 @@ static int dwc3_runtime_idle(struct device *dev) > { > struct dwc3 *dwc = dev_get_drvdata(dev); > > - switch (dwc->dr_mode) { > - case USB_DR_MODE_PERIPHERAL: > - case USB_DR_MODE_OTG: > + switch (dwc->current_dr_role) { > + case DWC3_GCTL_PRTCAP_DEVICE: > if (dwc3_runtime_checks(dwc)) > return -EBUSY; > break; > - case USB_DR_MODE_HOST: > + case DWC3_GCTL_PRTCAP_HOST: > default: > /* do nothing */ > break;
Hi, Manu Gautam <mgautam@codeaurora.org> writes: > Hi Felipe, > > Let me know if patches in this series look fine to you. It does, I just don't have means to test this as intel's platform doesn't give SW access to PHYs. I was hoping someone from TI would give a tested-by, but it's too late. We'll just take this series and fix any possible bugs during the -rc cycle. Just one question below > On 9/27/2017 4:49 PM, Manu Gautam wrote: >> Driver powers-off PHYs and reinitializes DWC3 core and gadget on >> resume. While this works fine for gadget mode but in host >> mode there is not re-initialization of host stack. Also, resetting >> bus as part of bus_suspend/resume is not correct which could affect >> (or disconnect) connected devices. >> Fix this by not reinitializing core on suspend/resume in host mode >> for HOST only and OTG/drd configurations. >> >> Signed-off-by: Manu Gautam <mgautam@codeaurora.org> >> --- >> drivers/usb/dwc3/core.c | 43 ++++++++++++++++++++----------------------- >> 1 file changed, 20 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c >> index 03474d3..f75613f 100644 >> --- a/drivers/usb/dwc3/core.c >> +++ b/drivers/usb/dwc3/core.c >> @@ -927,6 +927,7 @@ static int dwc3_core_init_mode(struct dwc3 *dwc) >> >> switch (dwc->dr_mode) { >> case USB_DR_MODE_PERIPHERAL: >> + dwc->current_dr_role = DWC3_GCTL_PRTCAP_DEVICE; seems like this could be done inside dwc3_set_prtcap(), no? We can do that as a separate patch too. No issues.
Hi Manu, On 27/09/17 14:19, Manu Gautam wrote: > Driver powers-off PHYs and reinitializes DWC3 core and gadget on > resume. While this works fine for gadget mode but in host > mode there is not re-initialization of host stack. Also, resetting > bus as part of bus_suspend/resume is not correct which could affect > (or disconnect) connected devices. > Fix this by not reinitializing core on suspend/resume in host mode > for HOST only and OTG/drd configurations. > All this seems correct but we (TI) were relying on dwc3_core_exit() to be called during dwc3_suspend() to have the lowest power state for our platforms. After this patch, DWC3 controller and PHYs won't be turned off thus preventing our platform from reaching low power levels. So this is a regression for us (TI) in v4.15-rc. Felipe, do you agree? If yes I can send a patch which fixes the regression and also makes USB host work after suspend/resume. > Signed-off-by: Manu Gautam <mgautam@codeaurora.org> > --- > drivers/usb/dwc3/core.c | 43 ++++++++++++++++++++----------------------- > 1 file changed, 20 insertions(+), 23 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 03474d3..f75613f 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -927,6 +927,7 @@ static int dwc3_core_init_mode(struct dwc3 *dwc) > > switch (dwc->dr_mode) { > case USB_DR_MODE_PERIPHERAL: > + dwc->current_dr_role = DWC3_GCTL_PRTCAP_DEVICE; > dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); > > if (dwc->usb2_phy) > @@ -942,6 +943,7 @@ static int dwc3_core_init_mode(struct dwc3 *dwc) > } > break; > case USB_DR_MODE_HOST: > + dwc->current_dr_role = DWC3_GCTL_PRTCAP_HOST; > dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST); > > if (dwc->usb2_phy) > @@ -1293,21 +1295,19 @@ static int dwc3_suspend_common(struct dwc3 *dwc) > { > unsigned long flags; > > - switch (dwc->dr_mode) { > - case USB_DR_MODE_PERIPHERAL: > - case USB_DR_MODE_OTG: > + switch (dwc->current_dr_role) { > + case DWC3_GCTL_PRTCAP_DEVICE: > spin_lock_irqsave(&dwc->lock, flags); > dwc3_gadget_suspend(dwc); > spin_unlock_irqrestore(&dwc->lock, flags); > + dwc3_core_exit(dwc); > break; > - case USB_DR_MODE_HOST: > + case DWC3_GCTL_PRTCAP_HOST: > default: > /* do nothing */ > break; > } > > - dwc3_core_exit(dwc); > - > return 0; > } > > @@ -1316,18 +1316,17 @@ static int dwc3_resume_common(struct dwc3 *dwc) > unsigned long flags; > int ret; > > - ret = dwc3_core_init(dwc); > - if (ret) > - return ret; > + switch (dwc->current_dr_role) { > + case DWC3_GCTL_PRTCAP_DEVICE: > + ret = dwc3_core_init(dwc); > + if (ret) > + return ret; > > - switch (dwc->dr_mode) { > - case USB_DR_MODE_PERIPHERAL: > - case USB_DR_MODE_OTG: > spin_lock_irqsave(&dwc->lock, flags); > dwc3_gadget_resume(dwc); > spin_unlock_irqrestore(&dwc->lock, flags); > - /* FALLTHROUGH */ > - case USB_DR_MODE_HOST: > + break; > + case DWC3_GCTL_PRTCAP_HOST: > default: > /* do nothing */ > break; > @@ -1338,7 +1337,7 @@ static int dwc3_resume_common(struct dwc3 *dwc) > > static int dwc3_runtime_checks(struct dwc3 *dwc) > { > - switch (dwc->dr_mode) { > + switch (dwc->current_dr_role) { > case USB_DR_MODE_PERIPHERAL: > case USB_DR_MODE_OTG: > if (dwc->connected) > @@ -1381,12 +1380,11 @@ static int dwc3_runtime_resume(struct device *dev) > if (ret) > return ret; > > - switch (dwc->dr_mode) { > - case USB_DR_MODE_PERIPHERAL: > - case USB_DR_MODE_OTG: > + switch (dwc->current_dr_role) { > + case DWC3_GCTL_PRTCAP_DEVICE: > dwc3_gadget_process_pending_events(dwc); > break; > - case USB_DR_MODE_HOST: > + case DWC3_GCTL_PRTCAP_HOST: > default: > /* do nothing */ > break; > @@ -1402,13 +1400,12 @@ static int dwc3_runtime_idle(struct device *dev) > { > struct dwc3 *dwc = dev_get_drvdata(dev); > > - switch (dwc->dr_mode) { > - case USB_DR_MODE_PERIPHERAL: > - case USB_DR_MODE_OTG: > + switch (dwc->current_dr_role) { > + case DWC3_GCTL_PRTCAP_DEVICE: > if (dwc3_runtime_checks(dwc)) > return -EBUSY; > break; > - case USB_DR_MODE_HOST: > + case DWC3_GCTL_PRTCAP_HOST: > default: > /* do nothing */ > break; >
Hi, Roger Quadros <rogerq@ti.com> writes: > Hi Manu, > > On 27/09/17 14:19, Manu Gautam wrote: >> Driver powers-off PHYs and reinitializes DWC3 core and gadget on >> resume. While this works fine for gadget mode but in host >> mode there is not re-initialization of host stack. Also, resetting >> bus as part of bus_suspend/resume is not correct which could affect >> (or disconnect) connected devices. >> Fix this by not reinitializing core on suspend/resume in host mode >> for HOST only and OTG/drd configurations. >> > > All this seems correct but we (TI) were relying on dwc3_core_exit() to be called > during dwc3_suspend() to have the lowest power state for our platforms. > > After this patch, DWC3 controller and PHYs won't be turned off thus > preventing our platform from reaching low power levels. > > So this is a regression for us (TI) in v4.15-rc. > > Felipe, do you agree? > > If yes I can send a patch which fixes the regression > and also makes USB host work after suspend/resume. A power consumption regression is still a regression. But it's super late in the -rc cycle, I think we need to get this merged after 4.16-rc1 and make sure to Cc stable. Should be more than enough time to review and test patches.
On 10/01/18 14:57, Felipe Balbi wrote: > > Hi, > > Roger Quadros <rogerq@ti.com> writes: >> Hi Manu, >> >> On 27/09/17 14:19, Manu Gautam wrote: >>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on >>> resume. While this works fine for gadget mode but in host >>> mode there is not re-initialization of host stack. Also, resetting >>> bus as part of bus_suspend/resume is not correct which could affect >>> (or disconnect) connected devices. >>> Fix this by not reinitializing core on suspend/resume in host mode >>> for HOST only and OTG/drd configurations. >>> >> >> All this seems correct but we (TI) were relying on dwc3_core_exit() to be called >> during dwc3_suspend() to have the lowest power state for our platforms. >> >> After this patch, DWC3 controller and PHYs won't be turned off thus >> preventing our platform from reaching low power levels. >> >> So this is a regression for us (TI) in v4.15-rc. >> >> Felipe, do you agree? >> >> If yes I can send a patch which fixes the regression >> and also makes USB host work after suspend/resume. > > A power consumption regression is still a regression. But it's super > late in the -rc cycle, I think we need to get this merged after 4.16-rc1 > and make sure to Cc stable. > > Should be more than enough time to review and test patches. > Fine with me.
Hi, On 1/10/2018 6:18 PM, Roger Quadros wrote: > Hi Manu, > > On 27/09/17 14:19, Manu Gautam wrote: >> Driver powers-off PHYs and reinitializes DWC3 core and gadget on >> resume. While this works fine for gadget mode but in host >> mode there is not re-initialization of host stack. Also, resetting >> bus as part of bus_suspend/resume is not correct which could affect >> (or disconnect) connected devices. >> Fix this by not reinitializing core on suspend/resume in host mode >> for HOST only and OTG/drd configurations. >> > All this seems correct but we (TI) were relying on dwc3_core_exit() to be called > during dwc3_suspend() to have the lowest power state for our platforms. > > After this patch, DWC3 controller and PHYs won't be turned off thus > preventing our platform from reaching low power levels. > > So this is a regression for us (TI) in v4.15-rc. > > Felipe, do you agree? > > If yes I can send a patch which fixes the regression > and also makes USB host work after suspend/resume. > I think it will be better to separate runtime_suspend and pm_suspend handling for host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across system suspend-resume should be ok but doing that for runtime suspend-resume is not correct. Let me know if that sounds ok, I can provide a patch for same instead of reverting this which affects runtime PM with dwc3 host. Also, we need to consider dwc3 in Host mode with dr_mode as DRD/OTG similar to dr_mode as HOST.
Hi, Manu Gautam <mgautam@codeaurora.org> writes: >> On 27/09/17 14:19, Manu Gautam wrote: >>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on >>> resume. While this works fine for gadget mode but in host >>> mode there is not re-initialization of host stack. Also, resetting >>> bus as part of bus_suspend/resume is not correct which could affect >>> (or disconnect) connected devices. >>> Fix this by not reinitializing core on suspend/resume in host mode >>> for HOST only and OTG/drd configurations. >>> >> All this seems correct but we (TI) were relying on dwc3_core_exit() to be called >> during dwc3_suspend() to have the lowest power state for our platforms. >> >> After this patch, DWC3 controller and PHYs won't be turned off thus >> preventing our platform from reaching low power levels. >> >> So this is a regression for us (TI) in v4.15-rc. >> >> Felipe, do you agree? >> >> If yes I can send a patch which fixes the regression >> and also makes USB host work after suspend/resume. >> > > I think it will be better to separate runtime_suspend and pm_suspend handling for > host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across system > suspend-resume should be ok but doing that for runtime suspend-resume is not > correct. it sure is. It's part of hibernation-while-disconnected programming sequence > Let me know if that sounds ok, I can provide a patch for same instead of > reverting this which affects runtime PM with dwc3 host. nope, that would break platforms using hibernation
Hi Felipe, On 1/11/2018 1:44 PM, Felipe Balbi wrote: > Hi, > > Manu Gautam <mgautam@codeaurora.org> writes: >>> On 27/09/17 14:19, Manu Gautam wrote: >>>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on >>>> resume. While this works fine for gadget mode but in host >>>> mode there is not re-initialization of host stack. Also, resetting >>>> bus as part of bus_suspend/resume is not correct which could affect >>>> (or disconnect) connected devices. >>>> Fix this by not reinitializing core on suspend/resume in host mode >>>> for HOST only and OTG/drd configurations. >>>> >>> All this seems correct but we (TI) were relying on dwc3_core_exit() to be called >>> during dwc3_suspend() to have the lowest power state for our platforms. >>> >>> After this patch, DWC3 controller and PHYs won't be turned off thus >>> preventing our platform from reaching low power levels. >>> >>> So this is a regression for us (TI) in v4.15-rc. >>> >>> Felipe, do you agree? >>> >>> If yes I can send a patch which fixes the regression >>> and also makes USB host work after suspend/resume. >>> >> I think it will be better to separate runtime_suspend and pm_suspend handling for >> host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across system >> suspend-resume should be ok but doing that for runtime suspend-resume is not >> correct. > it sure is. It's part of hibernation-while-disconnected programming sequence > >> Let me know if that sounds ok, I can provide a patch for same instead of >> reverting this which affects runtime PM with dwc3 host. > nope, that would break platforms using hibernation Please don't mind me asking this if it is very basic, I am probably missing something there We should be able to distinguish between runtime_pm vs system_suspend/hibernation and then process accordingly. In host mode runtime suspend/resume could happen very often with device connected, and resetting h/w on every runtime_resume might not be desired. And PHYs drivers can also support runtime_suspend which would be preferred instead of shutting down phy.
Hi, Manu Gautam <mgautam@codeaurora.org> writes: >>>> On 27/09/17 14:19, Manu Gautam wrote: >>>>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on >>>>> resume. While this works fine for gadget mode but in host >>>>> mode there is not re-initialization of host stack. Also, resetting >>>>> bus as part of bus_suspend/resume is not correct which could affect >>>>> (or disconnect) connected devices. >>>>> Fix this by not reinitializing core on suspend/resume in host mode >>>>> for HOST only and OTG/drd configurations. >>>>> >>>> All this seems correct but we (TI) were relying on dwc3_core_exit() to be called >>>> during dwc3_suspend() to have the lowest power state for our platforms. >>>> >>>> After this patch, DWC3 controller and PHYs won't be turned off thus >>>> preventing our platform from reaching low power levels. >>>> >>>> So this is a regression for us (TI) in v4.15-rc. >>>> >>>> Felipe, do you agree? >>>> >>>> If yes I can send a patch which fixes the regression >>>> and also makes USB host work after suspend/resume. >>>> >>> I think it will be better to separate runtime_suspend and pm_suspend handling for >>> host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across system >>> suspend-resume should be ok but doing that for runtime suspend-resume is not >>> correct. >> it sure is. It's part of hibernation-while-disconnected programming sequence >> >>> Let me know if that sounds ok, I can provide a patch for same instead of >>> reverting this which affects runtime PM with dwc3 host. >> nope, that would break platforms using hibernation > > Please don't mind me asking this if it is very basic, I am probably > missing something there > > We should be able to distinguish between runtime_pm vs > system_suspend/hibernation and then process accordingly. I'm not talking about Linux suspend to disk; I'm talking about Synopsys' Hibernation feature (open up your databook and have a read ;-) > In host mode runtime suspend/resume could happen very often with > device connected, and resetting h/w on every runtime_resume might not > be desired. And PHYs drivers can also support runtime_suspend which > would be preferred instead of shutting down phy. We don't do anything when dwc3 is working as a host, we simply assume if we reach dwc3.ko, xhci has done its part. Here's what our suspend_common looks like: static int dwc3_suspend_common(struct dwc3 *dwc) { unsigned long flags; switch (dwc->current_dr_role) { case DWC3_GCTL_PRTCAP_DEVICE: spin_lock_irqsave(&dwc->lock, flags); dwc3_gadget_suspend(dwc); spin_unlock_irqrestore(&dwc->lock, flags); dwc3_core_exit(dwc); break; case DWC3_GCTL_PRTCAP_HOST: default: /* do nothing */ break; } return 0; } We're not resetting anything, not tearing down anything. No idea why you're saying that in host mode we're breaking things apart. If you have out-of-tree patches on top of v4.15-rc7, fix them instead of claiming mainline is at fault.
Felipe, On 11/01/18 11:04, Felipe Balbi wrote: > > Hi, > > Manu Gautam <mgautam@codeaurora.org> writes: >>>>> On 27/09/17 14:19, Manu Gautam wrote: >>>>>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on >>>>>> resume. While this works fine for gadget mode but in host >>>>>> mode there is not re-initialization of host stack. Also, resetting >>>>>> bus as part of bus_suspend/resume is not correct which could affect >>>>>> (or disconnect) connected devices. >>>>>> Fix this by not reinitializing core on suspend/resume in host mode >>>>>> for HOST only and OTG/drd configurations. >>>>>> >>>>> All this seems correct but we (TI) were relying on dwc3_core_exit() to be called >>>>> during dwc3_suspend() to have the lowest power state for our platforms. >>>>> >>>>> After this patch, DWC3 controller and PHYs won't be turned off thus >>>>> preventing our platform from reaching low power levels. >>>>> >>>>> So this is a regression for us (TI) in v4.15-rc. >>>>> >>>>> Felipe, do you agree? >>>>> >>>>> If yes I can send a patch which fixes the regression >>>>> and also makes USB host work after suspend/resume. >>>>> >>>> I think it will be better to separate runtime_suspend and pm_suspend handling for >>>> host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across system >>>> suspend-resume should be ok but doing that for runtime suspend-resume is not >>>> correct. >>> it sure is. It's part of hibernation-while-disconnected programming sequence >>> >>>> Let me know if that sounds ok, I can provide a patch for same instead of >>>> reverting this which affects runtime PM with dwc3 host. >>> nope, that would break platforms using hibernation >> >> Please don't mind me asking this if it is very basic, I am probably >> missing something there >> >> We should be able to distinguish between runtime_pm vs >> system_suspend/hibernation and then process accordingly. > > I'm not talking about Linux suspend to disk; I'm talking about Synopsys' > Hibernation feature (open up your databook and have a read ;-) > >> In host mode runtime suspend/resume could happen very often with >> device connected, and resetting h/w on every runtime_resume might not >> be desired. And PHYs drivers can also support runtime_suspend which >> would be preferred instead of shutting down phy. > > We don't do anything when dwc3 is working as a host, we simply assume if > we reach dwc3.ko, xhci has done its part. Here's what our > suspend_common looks like: > > static int dwc3_suspend_common(struct dwc3 *dwc) > { > unsigned long flags; > > switch (dwc->current_dr_role) { > case DWC3_GCTL_PRTCAP_DEVICE: > spin_lock_irqsave(&dwc->lock, flags); > dwc3_gadget_suspend(dwc); > spin_unlock_irqrestore(&dwc->lock, flags); > dwc3_core_exit(dwc); > break; > case DWC3_GCTL_PRTCAP_HOST: > default: > /* do nothing */ > break; > } > > return 0; > } > > We're not resetting anything, not tearing down anything. No idea why > you're saying that in host mode we're breaking things apart. If you have > out-of-tree patches on top of v4.15-rc7, fix them instead of claiming > mainline is at fault. > This is the case after commit 689bf72c6e0d ("usb: dwc3: Don't reinitialize core during host bus-suspend/resume") which is breaking low power for TI platforms in Host mode. If we revert that commit we will be doing dwc3_core_exit() for host mode as well. Which is what we want for system suspend but probably not for runtime suspend in host case. This is why Manu wants to differentiate runtime vs system suspend.
Hi, Roger Quadros <rogerq@ti.com> writes: >>> In host mode runtime suspend/resume could happen very often with >>> device connected, and resetting h/w on every runtime_resume might not >>> be desired. And PHYs drivers can also support runtime_suspend which >>> would be preferred instead of shutting down phy. >> >> We don't do anything when dwc3 is working as a host, we simply assume if >> we reach dwc3.ko, xhci has done its part. Here's what our >> suspend_common looks like: >> >> static int dwc3_suspend_common(struct dwc3 *dwc) >> { >> unsigned long flags; >> >> switch (dwc->current_dr_role) { >> case DWC3_GCTL_PRTCAP_DEVICE: >> spin_lock_irqsave(&dwc->lock, flags); >> dwc3_gadget_suspend(dwc); >> spin_unlock_irqrestore(&dwc->lock, flags); >> dwc3_core_exit(dwc); >> break; >> case DWC3_GCTL_PRTCAP_HOST: >> default: >> /* do nothing */ >> break; >> } >> >> return 0; >> } >> >> We're not resetting anything, not tearing down anything. No idea why >> you're saying that in host mode we're breaking things apart. If you have >> out-of-tree patches on top of v4.15-rc7, fix them instead of claiming >> mainline is at fault. >> > > This is the case after commit 689bf72c6e0d ("usb: dwc3: Don't > reinitialize core during host bus-suspend/resume") which is breaking > low power for TI platforms in Host mode. > > If we revert that commit we will be doing dwc3_core_exit() for host > mode as well. Which is what we want for system suspend but probably > not for runtime suspend in host case. > > This is why Manu wants to differentiate runtime vs system suspend. We already differentiate them. Maybe the only thing we need is to *not* call core_exit() in host-mode during runtime_suspend, but call it if mode is device or during system sleep.
Hi Manu, On 11/01/18 03:41, Manu Gautam wrote: > Hi, > > > On 1/10/2018 6:18 PM, Roger Quadros wrote: >> Hi Manu, >> >> On 27/09/17 14:19, Manu Gautam wrote: >>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on >>> resume. While this works fine for gadget mode but in host >>> mode there is not re-initialization of host stack. Also, resetting >>> bus as part of bus_suspend/resume is not correct which could affect >>> (or disconnect) connected devices. >>> Fix this by not reinitializing core on suspend/resume in host mode >>> for HOST only and OTG/drd configurations. >>> >> All this seems correct but we (TI) were relying on dwc3_core_exit() to be called >> during dwc3_suspend() to have the lowest power state for our platforms. >> >> After this patch, DWC3 controller and PHYs won't be turned off thus >> preventing our platform from reaching low power levels. >> >> So this is a regression for us (TI) in v4.15-rc. >> >> Felipe, do you agree? >> >> If yes I can send a patch which fixes the regression >> and also makes USB host work after suspend/resume. >> > > I think it will be better to separate runtime_suspend and pm_suspend handling for > host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across system > suspend-resume should be ok but doing that for runtime suspend-resume is not > correct. > Let me know if that sounds ok, I can provide a patch for same instead of > reverting this which affects runtime PM with dwc3 host. > Also, we need to consider dwc3 in Host mode with dr_mode as DRD/OTG similar to > dr_mode as HOST. > > Are you going to provide a patch for this any time soon? FYI, suspend/resume is broken on DRA7x with Dual-role while in host mode.
Hi Roger, On 1/15/2018 9:10 PM, Roger Quadros wrote: > Hi Manu, [snip] >> I think it will be better to separate runtime_suspend and pm_suspend handling for >> host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across system >> suspend-resume should be ok but doing that for runtime suspend-resume is not >> correct. >> Let me know if that sounds ok, I can provide a patch for same instead of >> reverting this which affects runtime PM with dwc3 host. >> Also, we need to consider dwc3 in Host mode with dr_mode as DRD/OTG similar to >> dr_mode as HOST. >> >> > Are you going to provide a patch for this any time soon? > > FYI, suspend/resume is broken on DRA7x with Dual-role while in host mode. > Posted following patch: https://patchwork.kernel.org/patch/10166077/ Let me know if this works for you.
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 03474d3..f75613f 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -927,6 +927,7 @@ static int dwc3_core_init_mode(struct dwc3 *dwc) switch (dwc->dr_mode) { case USB_DR_MODE_PERIPHERAL: + dwc->current_dr_role = DWC3_GCTL_PRTCAP_DEVICE; dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); if (dwc->usb2_phy) @@ -942,6 +943,7 @@ static int dwc3_core_init_mode(struct dwc3 *dwc) } break; case USB_DR_MODE_HOST: + dwc->current_dr_role = DWC3_GCTL_PRTCAP_HOST; dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST); if (dwc->usb2_phy) @@ -1293,21 +1295,19 @@ static int dwc3_suspend_common(struct dwc3 *dwc) { unsigned long flags; - switch (dwc->dr_mode) { - case USB_DR_MODE_PERIPHERAL: - case USB_DR_MODE_OTG: + switch (dwc->current_dr_role) { + case DWC3_GCTL_PRTCAP_DEVICE: spin_lock_irqsave(&dwc->lock, flags); dwc3_gadget_suspend(dwc); spin_unlock_irqrestore(&dwc->lock, flags); + dwc3_core_exit(dwc); break; - case USB_DR_MODE_HOST: + case DWC3_GCTL_PRTCAP_HOST: default: /* do nothing */ break; } - dwc3_core_exit(dwc); - return 0; } @@ -1316,18 +1316,17 @@ static int dwc3_resume_common(struct dwc3 *dwc) unsigned long flags; int ret; - ret = dwc3_core_init(dwc); - if (ret) - return ret; + switch (dwc->current_dr_role) { + case DWC3_GCTL_PRTCAP_DEVICE: + ret = dwc3_core_init(dwc); + if (ret) + return ret; - switch (dwc->dr_mode) { - case USB_DR_MODE_PERIPHERAL: - case USB_DR_MODE_OTG: spin_lock_irqsave(&dwc->lock, flags); dwc3_gadget_resume(dwc); spin_unlock_irqrestore(&dwc->lock, flags); - /* FALLTHROUGH */ - case USB_DR_MODE_HOST: + break; + case DWC3_GCTL_PRTCAP_HOST: default: /* do nothing */ break; @@ -1338,7 +1337,7 @@ static int dwc3_resume_common(struct dwc3 *dwc) static int dwc3_runtime_checks(struct dwc3 *dwc) { - switch (dwc->dr_mode) { + switch (dwc->current_dr_role) { case USB_DR_MODE_PERIPHERAL: case USB_DR_MODE_OTG: if (dwc->connected) @@ -1381,12 +1380,11 @@ static int dwc3_runtime_resume(struct device *dev) if (ret) return ret; - switch (dwc->dr_mode) { - case USB_DR_MODE_PERIPHERAL: - case USB_DR_MODE_OTG: + switch (dwc->current_dr_role) { + case DWC3_GCTL_PRTCAP_DEVICE: dwc3_gadget_process_pending_events(dwc); break; - case USB_DR_MODE_HOST: + case DWC3_GCTL_PRTCAP_HOST: default: /* do nothing */ break; @@ -1402,13 +1400,12 @@ static int dwc3_runtime_idle(struct device *dev) { struct dwc3 *dwc = dev_get_drvdata(dev); - switch (dwc->dr_mode) { - case USB_DR_MODE_PERIPHERAL: - case USB_DR_MODE_OTG: + switch (dwc->current_dr_role) { + case DWC3_GCTL_PRTCAP_DEVICE: if (dwc3_runtime_checks(dwc)) return -EBUSY; break; - case USB_DR_MODE_HOST: + case DWC3_GCTL_PRTCAP_HOST: default: /* do nothing */ break;
Driver powers-off PHYs and reinitializes DWC3 core and gadget on resume. While this works fine for gadget mode but in host mode there is not re-initialization of host stack. Also, resetting bus as part of bus_suspend/resume is not correct which could affect (or disconnect) connected devices. Fix this by not reinitializing core on suspend/resume in host mode for HOST only and OTG/drd configurations. Signed-off-by: Manu Gautam <mgautam@codeaurora.org> --- drivers/usb/dwc3/core.c | 43 ++++++++++++++++++++----------------------- 1 file changed, 20 insertions(+), 23 deletions(-)