| Message ID | 1624882097-23265-7-git-send-email-sanm@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | USB DWC3 host wake up support from system suspend | expand |
On Mon, Jun 28, 2021 at 05:38:17PM +0530, Sandeep Maheswaram wrote: > 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> > --- > Checking phy_power_off flag instead of usb_wakeup_enabled_descendants > to keep gdsc active. > > drivers/usb/dwc3/dwc3-qcom.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+) > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c > index 82125bc..ba31aa3 100644 > --- a/drivers/usb/dwc3/dwc3-qcom.c > +++ b/drivers/usb/dwc3/dwc3-qcom.c > @@ -17,6 +17,7 @@ > #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> > @@ -355,9 +356,15 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) > u32 val; > int i, ret; > > + 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->phy_power_off && dwc->xhci) > + 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 +389,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); > This is essentially the same as v7, which Felipe NAKed (https://patchwork.kernel.org/project/linux-arm-msm/patch/1619586716-8687-6-git-send-email-sanm@codeaurora.org/) I think Felipe wants to see the handling of the power domain in the xhci-plat driver. One problem here is that the power domain is owned by the glue driver. For dwc3 the glue device is the parent of the xHCI device, this is also the case for some other drivers like histb or cdns3, but I'm not sure if it is universally true. If it isn't xhci-plat could only make use of dev->parent->pm_domain for certain compatible strings. One could argue that it isn't very clean either if xhci-plat manipulates a resource of it's parent. At the same time the glue driver isn't supposed to check for the wakeup capable devices, so I guess some kind of trade-off needs to be made.
Hi, Matthias Kaehlcke <mka@chromium.org> writes: > On Mon, Jun 28, 2021 at 05:38:17PM +0530, Sandeep Maheswaram wrote: >> 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> >> --- >> Checking phy_power_off flag instead of usb_wakeup_enabled_descendants >> to keep gdsc active. >> >> drivers/usb/dwc3/dwc3-qcom.c | 13 +++++++++++++ >> 1 file changed, 13 insertions(+) >> >> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c >> index 82125bc..ba31aa3 100644 >> --- a/drivers/usb/dwc3/dwc3-qcom.c >> +++ b/drivers/usb/dwc3/dwc3-qcom.c >> @@ -17,6 +17,7 @@ >> #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> >> @@ -355,9 +356,15 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) >> u32 val; >> int i, ret; >> >> + 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->phy_power_off && dwc->xhci) >> + 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 +389,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); >> > > This is essentially the same as v7, which Felipe NAKed > (https://patchwork.kernel.org/project/linux-arm-msm/patch/1619586716-8687-6-git-send-email-sanm@codeaurora.org/) > > I think Felipe wants to see the handling of the power domain in the > xhci-plat driver. One problem here is that the power domain is owned this is not exactly what I meant to say, though. I want drivers to be self-contained. I.e. dwc3 doesn't modify xhci data and vice-versa. There are a few assummpmtions that we can make, though. The structure is usually like this: glue { dwc3 { xhci }; }; This means that in order for glue_suspend() to run, dwc3 has to suspend first and xhci has to suspend before dwc3. For example, in the suspend call above, qcom (the glue) is directly accessing dwc3 core data, which is incorrect. It looks like we want to know if the PHY is not powered off and if it isn't, then we want to change the power domain ACTIVE_WAKEUP flag. Now, phy_power_off is false whenever any of xHCI's children enable USB wakeup. It seems like we need to way to generically propagate that knowledge up the parent tree. I.e., a parent needs to know if its child is wakeup capable, then dwc3 could, in its suspend routine: static int dwc3_suspend(struct device *dev) { /* ... */ if (device_children_wakeup_capable(dev)) device_enable_wakeup(dev); /* ... */ } and similarly for qcom glue: static int dwc3_qcom_suspend(struct device *dev) { /* ... */ if (device_children_wakeup_capable(dev)) { device_enable_wakeup(dev); genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP; } /* ... */ } It also seems plausible that this could be done at driver core and completely hidden away from drivers.
Hi Felipe, On 7/12/2021 3:12 PM, Felipe Balbi wrote: > Hi, > > Matthias Kaehlcke <mka@chromium.org> writes: >> On Mon, Jun 28, 2021 at 05:38:17PM +0530, Sandeep Maheswaram wrote: >>> 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> >>> --- >>> Checking phy_power_off flag instead of usb_wakeup_enabled_descendants >>> to keep gdsc active. >>> >>> drivers/usb/dwc3/dwc3-qcom.c | 13 +++++++++++++ >>> 1 file changed, 13 insertions(+) >>> >>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c >>> index 82125bc..ba31aa3 100644 >>> --- a/drivers/usb/dwc3/dwc3-qcom.c >>> +++ b/drivers/usb/dwc3/dwc3-qcom.c >>> @@ -17,6 +17,7 @@ >>> #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> >>> @@ -355,9 +356,15 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) >>> u32 val; >>> int i, ret; >>> >>> + 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->phy_power_off && dwc->xhci) >>> + 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 +389,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); >>> >> This is essentially the same as v7, which Felipe NAKed >> (https://patchwork.kernel.org/project/linux-arm-msm/patch/1619586716-8687-6-git-send-email-sanm@codeaurora.org/) >> >> I think Felipe wants to see the handling of the power domain in the >> xhci-plat driver. One problem here is that the power domain is owned > this is not exactly what I meant to say, though. I want drivers to be > self-contained. I.e. dwc3 doesn't modify xhci data and vice-versa. There > are a few assummpmtions that we can make, though. The structure is > usually like this: > > glue { > dwc3 { > xhci > }; > }; > > This means that in order for glue_suspend() to run, dwc3 has to suspend > first and xhci has to suspend before dwc3. > > For example, in the suspend call above, qcom (the glue) is directly > accessing dwc3 core data, which is incorrect. It looks like we want to > know if the PHY is not powered off and if it isn't, then we want to > change the power domain ACTIVE_WAKEUP flag. Now, phy_power_off is false > whenever any of xHCI's children enable USB wakeup. > > It seems like we need to way to generically propagate that knowledge up > the parent tree. I.e., a parent needs to know if its child is wakeup > capable, then dwc3 could, in its suspend routine: > > static int dwc3_suspend(struct device *dev) > { > /* ... */ > > if (device_children_wakeup_capable(dev)) > device_enable_wakeup(dev); > > /* ... */ > } Can we use like this device_may_wakeup(&dwc->xhci->dev) to check if children is wakeup capable like below ? static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg) { /* ... */ if (!PMSG_IS_AUTO(msg) && !device_may_wakeup(&dwc->xhci->dev)) { dwc3_core_exit(dwc); break; } /* ... */ } > and similarly for qcom glue: > > static int dwc3_qcom_suspend(struct device *dev) > { > /* ... */ > > > if (device_children_wakeup_capable(dev)) { > device_enable_wakeup(dev); > genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP; > } > > /* ... */ > } > > It also seems plausible that this could be done at driver core and > completely hidden away from drivers. And in qcom glue like this static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) { /* ... */ struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); if (device_may_wakeup(&dwc->xhci->dev) && dwc->xhci) genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP; /* ... */ }
Hi, Sandeep Maheswaram <sanm@codeaurora.org> writes: >> This means that in order for glue_suspend() to run, dwc3 has to suspend >> first and xhci has to suspend before dwc3. >> >> For example, in the suspend call above, qcom (the glue) is directly >> accessing dwc3 core data, which is incorrect. It looks like we want to >> know if the PHY is not powered off and if it isn't, then we want to >> change the power domain ACTIVE_WAKEUP flag. Now, phy_power_off is false >> whenever any of xHCI's children enable USB wakeup. >> >> It seems like we need to way to generically propagate that knowledge up >> the parent tree. I.e., a parent needs to know if its child is wakeup >> capable, then dwc3 could, in its suspend routine: >> >> static int dwc3_suspend(struct device *dev) >> { >> /* ... */ >> >> if (device_children_wakeup_capable(dev)) >> device_enable_wakeup(dev); >> >> /* ... */ >> } > > Can we use like this device_may_wakeup(&dwc->xhci->dev) to check if > children is wakeup capable like below ? that really doesn't sound like a good idea, IMHO. We're still passing through layers of abstraction without anyone's knowledge :-) It looks to me like we're missing some infrastructure in the wakeup code so parents can make decisions based on the state of their children. >> and similarly for qcom glue: >> >> static int dwc3_qcom_suspend(struct device *dev) >> { >> /* ... */ >> >> >> if (device_children_wakeup_capable(dev)) { >> device_enable_wakeup(dev); >> genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP; >> } >> >> /* ... */ >> } >> >> It also seems plausible that this could be done at driver core and >> completely hidden away from drivers. > > And in qcom glue like this > > static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) > { > > /* ... */ > > struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3); you see, here there's an assumption that the platform data is still valid and not some bogus dangling pointer. There's also an assumption that the type is struct dwc3 (which is unlikely to change, but still).
+linux-pm and Rafael Hi Balbi, On Mon, Jul 12, 2021 at 12:42:17PM +0300, Felipe Balbi wrote: > > >> @@ -355,9 +356,15 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) > >> u32 val; > >> int i, ret; > >> > >> + 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->phy_power_off && dwc->xhci) > >> + 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 +389,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); > >> > > > > This is essentially the same as v7, which Felipe NAKed > > (https://patchwork.kernel.org/project/linux-arm-msm/patch/1619586716-8687-6-git-send-email-sanm@codeaurora.org/) > > > > I think Felipe wants to see the handling of the power domain in the > > xhci-plat driver. One problem here is that the power domain is owned > > this is not exactly what I meant to say, though. I want drivers to be > self-contained. I.e. dwc3 doesn't modify xhci data and vice-versa. There > are a few assummpmtions that we can make, though. The structure is > usually like this: > > glue { > dwc3 { > xhci > }; > }; > > This means that in order for glue_suspend() to run, dwc3 has to suspend > first and xhci has to suspend before dwc3. > > For example, in the suspend call above, qcom (the glue) is directly > accessing dwc3 core data, which is incorrect. It looks like we want to > know if the PHY is not powered off and if it isn't, then we want to > change the power domain ACTIVE_WAKEUP flag. Now, phy_power_off is false > whenever any of xHCI's children enable USB wakeup. > > It seems like we need to way to generically propagate that knowledge up > the parent tree. I.e., a parent needs to know if its child is wakeup > capable, then dwc3 could, in its suspend routine: > > static int dwc3_suspend(struct device *dev) > { > /* ... */ > > if (device_children_wakeup_capable(dev)) > device_enable_wakeup(dev); > > /* ... */ > } > > and similarly for qcom glue: > > static int dwc3_qcom_suspend(struct device *dev) > { > /* ... */ > > if (device_children_wakeup_capable(dev)) { > device_enable_wakeup(dev); > genpd->flags |= GENPD_FLAG_ACTIVE_WAKEUP; > } > > /* ... */ > } > > It also seems plausible that this could be done at driver core and > completely hidden away from drivers. > device_children_wakeup_capable() idea sounds good. Added linux-pm and Rafael for more inputs on this. AFAICT, device wakeup settings are NOT propagated to the parent. Ideally one expects the parent to be wake up capable and enabled when any of its children is enabling the wakeup. If that had been propagated, we could simply check device_may_wakeup() and take an action. I am sure there are good reason why and how this propgation may not work as we expect it to work for all devices. Can we make something like below to make sure that DWC3 core respects its child wakeup setting? This is inline with your suggestion of propagating it through layers. we should probably enable wakeup on dwc3 dev so that the glue drivers can take the action appropriately. diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 172dfd2..9c43d37 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -1749,7 +1749,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) && !device_may_wakeup(&dwc->xhci->dev)) { dwc3_core_exit(dwc); break; } @@ -1810,7 +1810,7 @@ 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) && !device_may_wakeup(&dwc->xhci->dev)) { ret = dwc3_core_init_for_resume(dwc); if (ret) return ret; Thanks, Pavan
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c index 82125bc..ba31aa3 100644 --- a/drivers/usb/dwc3/dwc3-qcom.c +++ b/drivers/usb/dwc3/dwc3-qcom.c @@ -17,6 +17,7 @@ #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> @@ -355,9 +356,15 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom) u32 val; int i, ret; + 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->phy_power_off && dwc->xhci) + 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 +389,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> --- Checking phy_power_off flag instead of usb_wakeup_enabled_descendants to keep gdsc active. drivers/usb/dwc3/dwc3-qcom.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)