| Message ID | 1619586716-8687-6-git-send-email-sanm@codeaurora.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | USB DWC3 host wake up support from system suspend | expand |
Hi, Sandeep Maheswaram <sanm@codeaurora.org> writes: > If wakeup capable devices are connected to the controller (directly > or through hubs) at suspend time keep the power domain on in order > to support wakeup from these devices. > > Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org> > --- > drivers/usb/dwc3/dwc3-qcom.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index 82125bc..1e220af 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -17,9 +17,11 @@ > #include <linux/of_platform.h> > #include <linux/platform_device.h> > #include <linux/phy/phy.h> > +#include <linux/pm_domain.h> > #include <linux/usb/of.h> > #include <linux/reset.h> > #include <linux/iopoll.h> > +#include <linux/usb/hcd.h> > > #include "core.h" > > @@ -354,10 +356,19 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) > { > u32 val; > int i, ret; > + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); > + struct usb_hcd *hcd; > + struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain); > > if (qcom->is_suspended) > return 0; > > + if (dwc->xhci) { > + hcd = platform_get_drvdata(dwc->xhci); > + if (usb_wakeup_enabled_descendants(hcd->self.root_hub)) > + genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP; > + } wow, you really need to find a way to do these things generically instead of bypassing a bunch of layers and access stuff $this doesn't directly own. I'm gonna say 'no' to this, sorry. It looks like xhci should, directly, learn about much of this instead of hiding it 3-layers deep into the dwc3 glue layer for your specific SoC.
On Wed, Apr 28, 2021 at 01:04:43PM +0300, Felipe Balbi wrote: > > Hi, > > Sandeep Maheswaram <sanm@codeaurora.org> writes: > > If wakeup capable devices are connected to the controller (directly > > or through hubs) at suspend time keep the power domain on in order > > to support wakeup from these devices. > > > > Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org> > > --- > > drivers/usb/dwc3/dwc3-qcom.c | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > > index 82125bc..1e220af 100644 > > --- a/drivers/usb/dwc3/dwc3-qcom.c > > +++ b/drivers/usb/dwc3/dwc3-qcom.c > > @@ -17,9 +17,11 @@ > > #include <linux/of_platform.h> > > #include <linux/platform_device.h> > > #include <linux/phy/phy.h> > > +#include <linux/pm_domain.h> > > #include <linux/usb/of.h> > > #include <linux/reset.h> > > #include <linux/iopoll.h> > > +#include <linux/usb/hcd.h> > > > > #include "core.h" > > > > @@ -354,10 +356,19 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) > > { > > u32 val; > > int i, ret; > > + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); > > + struct usb_hcd *hcd; > > + struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain); > > > > if (qcom->is_suspended) > > return 0; > > > > + if (dwc->xhci) { > > + hcd = platform_get_drvdata(dwc->xhci); > > + if (usb_wakeup_enabled_descendants(hcd->self.root_hub)) > > + genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP; > > + } > > wow, you really need to find a way to do these things generically > instead of bypassing a bunch of layers and access stuff $this doesn't > directly own. > > I'm gonna say 'no' to this, sorry. It looks like xhci should, directly, > learn about much of this instead of hiding it 3-layers deep into the > dwc3 glue layer for your specific SoC. Maybe this could be addressed with a pair of wakeup quirks, one for suspend, another for resume. An optional genpd field could be added to struct xhci_plat_priv. The wakeup quirks would set/clear GENPD_FLAG_ACTIVE_WAKEUP of the genpd. Does the above sound more palatable?
Hi, Matthias Kaehlcke <mka@chromium.org> writes: >> Sandeep Maheswaram <sanm@codeaurora.org> writes: >> > If wakeup capable devices are connected to the controller (directly >> > or through hubs) at suspend time keep the power domain on in order >> > to support wakeup from these devices. >> > >> > Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org> >> > --- >> > drivers/usb/dwc3/dwc3-qcom.c | 17 +++++++++++++++++ >> > 1 file changed, 17 insertions(+) >> > >> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c >> > index 82125bc..1e220af 100644 >> > --- a/drivers/usb/dwc3/dwc3-qcom.c >> > +++ b/drivers/usb/dwc3/dwc3-qcom.c >> > @@ -17,9 +17,11 @@ >> > #include <linux/of_platform.h> >> > #include <linux/platform_device.h> >> > #include <linux/phy/phy.h> >> > +#include <linux/pm_domain.h> >> > #include <linux/usb/of.h> >> > #include <linux/reset.h> >> > #include <linux/iopoll.h> >> > +#include <linux/usb/hcd.h> >> > >> > #include "core.h" >> > >> > @@ -354,10 +356,19 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) >> > { >> > u32 val; >> > int i, ret; >> > + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); >> > + struct usb_hcd *hcd; >> > + struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain); >> > >> > if (qcom->is_suspended) >> > return 0; >> > >> > + if (dwc->xhci) { >> > + hcd = platform_get_drvdata(dwc->xhci); >> > + if (usb_wakeup_enabled_descendants(hcd->self.root_hub)) >> > + genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP; >> > + } >> >> wow, you really need to find a way to do these things generically >> instead of bypassing a bunch of layers and access stuff $this doesn't >> directly own. >> >> I'm gonna say 'no' to this, sorry. It looks like xhci should, directly, >> learn about much of this instead of hiding it 3-layers deep into the >> dwc3 glue layer for your specific SoC. > > Maybe this could be addressed with a pair of wakeup quirks, one for suspend, > another for resume. An optional genpd field could be added to struct > xhci_plat_priv. The wakeup quirks would set/clear GENPD_FLAG_ACTIVE_WAKEUP > of the genpd. > > Does the above sound more palatable? I don't get why you need quirk flags. All you're doing here is: if (usb_wakeup_enabled_descendants(hcd->self.root_hub)) genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP; If you move this test to xhci_suspend(), you shouldn't need all the magic, right?
On Thu, May 13, 2021 at 04:49:19PM +0300, Felipe Balbi wrote: > > Hi, > > Matthias Kaehlcke <mka@chromium.org> writes: > >> Sandeep Maheswaram <sanm@codeaurora.org> writes: > >> > If wakeup capable devices are connected to the controller (directly > >> > or through hubs) at suspend time keep the power domain on in order > >> > to support wakeup from these devices. > >> > > >> > Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org> > >> > --- > >> > drivers/usb/dwc3/dwc3-qcom.c | 17 +++++++++++++++++ > >> > 1 file changed, 17 insertions(+) > >> > > >> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > >> > index 82125bc..1e220af 100644 > >> > --- a/drivers/usb/dwc3/dwc3-qcom.c > >> > +++ b/drivers/usb/dwc3/dwc3-qcom.c > >> > @@ -17,9 +17,11 @@ > >> > #include <linux/of_platform.h> > >> > #include <linux/platform_device.h> > >> > #include <linux/phy/phy.h> > >> > +#include <linux/pm_domain.h> > >> > #include <linux/usb/of.h> > >> > #include <linux/reset.h> > >> > #include <linux/iopoll.h> > >> > +#include <linux/usb/hcd.h> > >> > > >> > #include "core.h" > >> > > >> > @@ -354,10 +356,19 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) > >> > { > >> > u32 val; > >> > int i, ret; > >> > + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); > >> > + struct usb_hcd *hcd; > >> > + struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain); > >> > > >> > if (qcom->is_suspended) > >> > return 0; > >> > > >> > + if (dwc->xhci) { > >> > + hcd = platform_get_drvdata(dwc->xhci); > >> > + if (usb_wakeup_enabled_descendants(hcd->self.root_hub)) > >> > + genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP; > >> > + } > >> > >> wow, you really need to find a way to do these things generically > >> instead of bypassing a bunch of layers and access stuff $this doesn't > >> directly own. > >> > >> I'm gonna say 'no' to this, sorry. It looks like xhci should, directly, > >> learn about much of this instead of hiding it 3-layers deep into the > >> dwc3 glue layer for your specific SoC. > > > > Maybe this could be addressed with a pair of wakeup quirks, one for suspend, > > another for resume. An optional genpd field could be added to struct > > xhci_plat_priv. The wakeup quirks would set/clear GENPD_FLAG_ACTIVE_WAKEUP > > of the genpd. > > > > Does the above sound more palatable? > > I don't get why you need quirk flags. All you're doing here is: > > if (usb_wakeup_enabled_descendants(hcd->self.root_hub)) > genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP; > > If you move this test to xhci_suspend(), you shouldn't need all the > magic, right? Right, the quirks shouldn't be necessary if setting/clearing the genpd flag is the right thing to do for any controller that sets the genpd field in _priv, which probably is the case.
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index 82125bc..1e220af 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -17,9 +17,11 @@ #include <linux/of_platform.h> #include <linux/platform_device.h> #include <linux/phy/phy.h> +#include <linux/pm_domain.h> #include <linux/usb/of.h> #include <linux/reset.h> #include <linux/iopoll.h> +#include <linux/usb/hcd.h> #include "core.h" @@ -354,10 +356,19 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) { u32 val; int i, ret; + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); + struct usb_hcd *hcd; + struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain); if (qcom->is_suspended) return 0; + if (dwc->xhci) { + hcd = platform_get_drvdata(dwc->xhci); + if (usb_wakeup_enabled_descendants(hcd->self.root_hub)) + genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP; + } + val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG); if (!(val & PWR_EVNT_LPM_IN_L2_MASK)) dev_err(qcom->dev, "HS-PHY not in L2\n"); @@ -382,9 +393,15 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom) int ret; int i; + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); + struct generic_pm_domain *genpd = pd_to_genpd(qcom->dev->pm_domain); + if (!qcom->is_suspended) return 0; + if (dwc->xhci) + genpd->flags &= ~GENPD_FLAG_ACTIVE_WAKEUP; + if (device_may_wakeup(qcom->dev)) dwc3_qcom_disable_interrupts(qcom);
If wakeup capable devices are connected to the controller (directly or through hubs) at suspend time keep the power domain on in order to support wakeup from these devices. Signed-off-by: Sandeep Maheswaram <sanm@codeaurora.org> --- drivers/usb/dwc3/dwc3-qcom.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)