Hi Felipe,
On 7/12/2021 3:12 PM, Felipe Balbi wrote:
Hi,
Matthias Kaehlcke <mka@xxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
---
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@xxxxxxxxxxxxxx/)
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;
/* ... */
}
