| 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.
Avoiding phy powerdown in host mode when wakeup capable devices are connected, so that it can be wake up by devices. Set GENPD_FLAG_ACTIVE_WAKEUP flag to keep usb30_prim gdsc active when wakeup capable devices are connected to the host. Changes in v5: Added phy_power_off flag to check presence of wakeup capable devices. Dropped patch[v4,4/5] as it is present linux-next. Addressed comments in host.c and dwc3-qcom.c. Changes in v4: Addressed Matthias comments raised in v3. Changes in v3: Removed need_phy_for_wakeup flag and by default avoiding phy powerdown. Addressed Matthias comments and added entry for DEV_SUPERSPEED. Added suspend_quirk in dwc3 host and moved the dwc3_set_phy_speed_flags. Added wakeup-source dt entry and reading in dwc-qcom.c glue driver. Changes in v2: Dropped the patch in clock to set GENPD_FLAG_ACTIVE_WAKEUP flag and setting in usb dwc3 driver. Separated the core patch and glue driver patch. Made need_phy_for_wakeup flag part of dwc structure and hs_phy_flags as unsgined int. Adrressed the comment on device_init_wakeup call. Corrected offset for reading portsc register. Added pacth to support wakeup in xo shutdown case. Sandeep Maheswaram (4): usb: dwc3: core: Host wake up support from system suspend usb: dwc3: host: Add suspend_quirk for dwc3 host usb: dwc3: qcom: Configure wakeup interrupts and set genpd active wakeup flag arm64: dts: qcom: sc7180: Add wakeup-source property for USB node in IDP and trogdor arch/arm64/boot/dts/qcom/sc7180-idp.dts | 1 + arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 1 + drivers/usb/dwc3/core.c | 8 ++- drivers/usb/dwc3/core.h | 3 + drivers/usb/dwc3/dwc3-qcom.c | 87 ++++++++++++++++++---------- drivers/usb/dwc3/host.c | 58 +++++++++++++++++++ 6 files changed, 124 insertions(+), 34 deletions(-)