On Sun, Jan 28, 2024, Guilherme G. Piccoli wrote:
> Hi Thinh, thanks again for the help and apologies for my delay.
> Responses inline below:
>
>
> On 22/01/2024 23:12, Thinh Nguyen wrote:
> > [...]
> >
> > The tracepoints indicated that no gadget driver was binded. So the
> > controller actually never started (ie. soft-connection never happened if
> > the controller doesn't start).
> >
> >>
> >> (2) Plugged the USB cable connecting the Deck to my laptop - results at
> >> {trace,regdump}.2 and as we can see, traces are empty.
> >
> > Right, because as noted above, no activity is seen.
> >
>
> Okay, that makes sense then and explains why I see nothing related to
> the Deck on my laptop! Do you know a SW way to measure that the USB port
> is providing power / "energy"? It's just out of curiosity, not that we
> need that in this case (knowing the lack of a gadget driver).
>
No, not that I'm aware. You'd need some analyzer/instrumentation for
this.
>
> >> [...]
> >
> > I can see the device was resumed right after.
> >
> > kworker/u32:10-1078 [002] ...1. 179.453703: dwc3_pci_suspend <-pci_pm_suspend
> > kworker/u32:10-1078 [002] ...1. 179.453704: dwc3_pci_dsm <-pci_pm_suspend
> > kworker/u32:19-1087 [005] ...1. 179.939836: dwc3_pci_resume <-dpm_run_callback
> >
> >>
> >> (4) [bonus] Collected the same results of 3 (after rebooting the Deck)
> >> but running dwc3 with this patch/quirk - it's easy to notice in the
> >> trace, as we can see the extras readl/writel prior to suspend. In this
> >> case, suspend works...and results are on {trace,regdump}.4-PATCHED
> >
> > Even in the patched version, device was resumed right after. The resume
> > here may not trigger the system resume, but it resumed nonetheless.
> >
> > kworker/u32:6-356 [006] ...1. 69.600400: dwc3_pci_suspend <-pci_pm_suspend
> > kworker/u32:6-356 [006] ...1. 69.600401: dwc3_pci_dsm <-pci_pm_suspend
> > kworker/u32:22-1028 [001] ...1. 70.125294: dwc3_pci_resume <-dpm_run_callback
> >
>
> Yeah, in both cases the resume happened. But they differ: without the
> patch, resume was automatic - the device effectively can't suspend since
> it auto-resumes instantly. Whereas with the patch (scenario 4), I've
> triggered the resume by pressing the power button on Steam Deck.
>
> And ftrace timestamps unfortunately don't help with that, it's not
> showing how much time the device stay suspended, so it might be
> confusing and both cases could appear as the same exact scenario, even
> if they are completely different (fail vs success cases).
That's odd.. my assumption was the timestamps to be valid. Were you able
to confirm that it's the issue with the timestamps? Perhaps check if the
other devices in the system also wakeup at the approximately same
time printed in the timestamps?
>
>
> >> [...]
> > Thanks for the logs and data points. The tracepoints indicate that the
> > workaround _only_ prevented the system wakeup, not the controller.
> > The wakeup still happen there as you can see the controller got woken up
> > immediately after request to go into suspend in both cases, patched or
> > not. So the workaround you provided doesn't help keeping the controller
> > in suspend.
>
> Yeah, this is not really the case - as mentioned above, though they
> appear the same in traces, without the workaround we have an immediate
> auto-resume, preventing the suspend. With the patch, device suspends and
> we can keep it this way for the amount of time we want, only resuming
> when we press the power button (which is exactly the expected behavior).
Ok
>
>
> >
> > We need to look into why that's the case, and we need to figure out
> > where the source of the wake came from. Do you have a connector
> > controller that can also wakeup the system?
>
> I'm not sure about this, what do you mean by a connector controller?!
I mean connector controller such as Type-C Port Controller (TCPC) or
some specific phy that can detect and report to a driver whether a
connection event occurs. For the lack of better term, I used connector
controller... (perhaps just connector?)
>
>
> >
> > As for the workaround, if the wakeup source is the usb controller, did
> > you try to disable wakeup?
> >
> > diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
> > index 6604845c397c..e395d7518022 100644
> > --- a/drivers/usb/dwc3/dwc3-pci.c
> > +++ b/drivers/usb/dwc3/dwc3-pci.c
> > @@ -359,7 +359,7 @@ static int dwc3_pci_probe(struct pci_dev *pci, const struct pci_device_id *id)
> > goto err;
> > }
> >
> > - device_init_wakeup(dev, true);
> > + device_init_wakeup(dev, false);
> > pci_set_drvdata(pci, dwc);
> > pm_runtime_put(dev);
> > #ifdef CONFIG_PM
> >
> > If it works, you can fine tune to only disable wakeup when in device
> > mode and enable when in host mode with device_set_wakeup_enable().
> >
>
> Oh, great suggestion - thanks! And...it worked!
Great! This means that the wakeup source is from the usb controller.
>
> So, with your patch, I'm able to properly suspend the Deck, even with
> dwc3 in device/gadget mode.
>
> I'll try to cook a patch with this approach but restricted to this
> specific USB controller and only when in gadget mode - do you think this
> is the way to go?
Perhaps we can make this a generic change across different devices. See
below.
>
> Any other debug / logs that might be useful?
In your setup, can you check if host mode wakeup is enabled:
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index c5d9ac67e612..716b05ff0384 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -472,6 +472,10 @@ static int xhci_plat_suspend(struct device *dev)
ret = xhci_priv_suspend_quirk(hcd);
if (ret)
return ret;
+
+ dev_info(dev, "%s: device_may_wakeup: %d\n",
+ __func__, !!device_may_wakeup(dev));
+
/*
* xhci_suspend() needs `do_wakeup` to know whether host is allowed
* to do wakeup during suspend.
When you go through the dwc3-pci code path, the dwc3 creates the
platform device from the pci device. Perhaps it doesn't enable wakeup.
Let's confirm that.
My suspicion is that the power management is mapped to the same PCI
PMCSR for both the host and device mode. When the device is in suspend
(D3), it gives control the the PMUs. If the PME is enabled, the PMUs
will generate PME on connection detection if the PME is enabled. When
you go through the xhci code path, wakeup may not be enabled.
For device mode, we can handle generically by only enabling wakeup if
the following conditions are met:
if (dwc->gadget_driver && dwc->softconnect)
Try this (totally untested):
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 3b68e8e45b8b..8a13fd6c813a 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1980,6 +1980,8 @@ static int dwc3_probe(struct platform_device *pdev)
pm_runtime_forbid(dev);
+ dwc->sys_wakeup = !!device_may_wakeup(dwc->sysdev);
+
ret = dwc3_alloc_event_buffers(dwc, DWC3_EVENT_BUFFERS_SIZE);
if (ret) {
dev_err(dwc->dev, "failed to allocate event buffers\n");
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index df544ec730d2..39f7bf068b1e 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1134,6 +1134,7 @@ struct dwc3_scratchpad_array {
* @dis_metastability_quirk: set to disable metastability quirk.
* @host_vbus_glitches_quirk: set to avoid vbus glitch during xhci reset.
* @dis_split_quirk: set to disable split boundary.
+ * @sys_wakeup: set if the device is configured for system wakeup.
* @wakeup_configured: set if the device is configured for remote wakeup.
* @suspended: set to track suspend event due to U3/L2.
* @imod_interval: set the interrupt moderation interval in 250ns
@@ -1358,6 +1359,7 @@ struct dwc3 {
unsigned dis_split_quirk:1;
unsigned async_callbacks:1;
+ unsigned sys_wakeup:1;
unsigned wakeup_configured:1;
unsigned suspended:1;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 019368f8e9c4..96960570de31 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2783,6 +2783,9 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
pm_runtime_put(dwc->dev);
+ if (dwc->sys_wakeup)
+ device_set_wakeup_enable(dwc->sysdev, is_on);
+
return ret;
}
@@ -4665,6 +4668,9 @@ int dwc3_gadget_init(struct dwc3 *dwc)
else
dwc3_gadget_set_speed(dwc->gadget, dwc->maximum_speed);
+ if (dwc->sys_wakeup)
+ device_wakeup_disable(dwc->sysdev);
+
return 0;
err5:
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index ee1ffe150056..5509b0355d58 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -163,6 +163,10 @@ int dwc3_host_init(struct dwc3 *dwc)
}
}
+ /* Perhaps we need to enable wakeup for xhci->dev? */
+ if (dwc->sys_wakeup)
+ device_wakeup_enable(dwc->sysdev);
+
ret = platform_device_add(xhci);
if (ret) {
dev_err(dwc->dev, "failed to register xHCI device\n");
BR,
Thinh
