Messages in this thread | | | Subject | Re: [PATCH v8 6/6] usb: dwc3: qcom: Keep power domain on to support wakeup | From | Sandeep Maheswaram <> | Date | Wed, 18 Aug 2021 14:44:13 +0530 |
| |
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;
/* ... */
}
| |