Message ID | 1616434280-32635-1-git-send-email-sanm@codeaurora.org |
---|---|
Headers | show |
Series | USB DWC3 host wake up support from system suspend | expand |
On Mon, Mar 22, 2021 at 11:01:17PM +0530, Sandeep Maheswaram wrote:
> Avoiding phy powerdown when wakeup capable devices are connected.
That says "what" (in a very abbreviated way), but not _WHY_ you want to
do this. Please fix this up.
thanks,
greg k-h
On Mon, Mar 22, 2021 at 11:01:19PM +0530, Sandeep Maheswaram wrote: > Configure interrupts based on hs_phy_mode to avoid triggering of > interrupts during system suspend and suspends successfully. > Set genpd active wakeup flag for usb gdsc if wakeup capable devices > are connected so that wake up happens without reenumeration. > Add helper functions to enable,disable wake irqs. That feels like a lot of different things all in one patch. What are you trying to fix here? I can't figure it out at all... Please work on your changelog text for these patches when you resend them. thanks, greg k-h
On Mon, Mar 22, 2021 at 11:01:17PM +0530, Sandeep Maheswaram wrote: > Avoiding phy powerdown when wakeup capable devices are connected. > > Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org> > --- > drivers/usb/dwc3/core.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 94fdbe5..9ecd7ac 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1702,7 +1702,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) > dwc3_core_exit(dwc); > break; > case DWC3_GCTL_PRTCAP_HOST: > - if (!PMSG_IS_AUTO(msg)) { > + if (!PMSG_IS_AUTO(msg) && dwc->phy_power_off) { This is the first patch of the series, but the 'phy_power_off' flag is only added by '[2/4] usb: dwc3: host: Add suspend_quirk for dwc3 host'. Patches should not rely on later patches in the series in order to build error/warning free. It seems you need to swap the order of patch 1 and 2. > dwc3_core_exit(dwc); > break; > } > @@ -1763,13 +1763,15 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg) > spin_unlock_irqrestore(&dwc->lock, flags); > break; > case DWC3_GCTL_PRTCAP_HOST: > - if (!PMSG_IS_AUTO(msg)) { > + if (!PMSG_IS_AUTO(msg) && dwc->phy_power_off) { > ret = dwc3_core_init_for_resume(dwc); > if (ret) > return ret; > dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST); > break; > - } > + } else > + dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST); > + nit: use curly braces since the 'if' block has them.
On Tue, Mar 23, 2021 at 01:11:18PM +0100, Greg Kroah-Hartman wrote: > On Mon, Mar 22, 2021 at 11:01:19PM +0530, Sandeep Maheswaram wrote: > > Configure interrupts based on hs_phy_mode to avoid triggering of > > interrupts during system suspend and suspends successfully. > > Set genpd active wakeup flag for usb gdsc if wakeup capable devices > > are connected so that wake up happens without reenumeration. > > Add helper functions to enable,disable wake irqs. > > That feels like a lot of different things all in one patch. Sandeep: one thing you could do to reduce the churn is to add dwc3_qcom_enable/disable_wakeup_irq() in a separate patch, without any functional changes. Then this patch would only add the different branches based on the PHY mode. The handling of the power domain could probably also be done in a separate patch, if I recall correctly it is only an optimization.
On Tue, Mar 23, 2021 at 05:49:14PM -0700, Matthias Kaehlcke wrote: > On Tue, Mar 23, 2021 at 01:11:18PM +0100, Greg Kroah-Hartman wrote: > > On Mon, Mar 22, 2021 at 11:01:19PM +0530, Sandeep Maheswaram wrote: > > > Configure interrupts based on hs_phy_mode to avoid triggering of > > > interrupts during system suspend and suspends successfully. > > > Set genpd active wakeup flag for usb gdsc if wakeup capable devices > > > are connected so that wake up happens without reenumeration. > > > Add helper functions to enable,disable wake irqs. > > > > That feels like a lot of different things all in one patch. > > Sandeep: one thing you could do to reduce the churn is to add > dwc3_qcom_enable/disable_wakeup_irq() in a separate patch, without > any functional changes. Then this patch would only add the different > branches based on the PHY mode. > > The handling of the power domain could probably also be done in a > separate patch, if I recall correctly it is only an optimization. Actually another thing that could be in a separate patch is enabling wakeup support based on 'wakeup-source'. That's not even directly related with this series. With all that you'd have fairly atomic patches and it should be easy to write meaningful commit messages.