On Wed, Jan 31, 2024, Guilherme G. Piccoli wrote:
> On 29/01/2024 23:17, Thinh Nguyen wrote:
> > [...]
> >> 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?
> >
>
> Hi Thinh, indeed - this a bit odd right? I guess this is maybe related
> with the way kernel keeps track of clocks on suspend - despite TSC keeps
> accounting on suspend (at least for all modern x86 processors IIUC), the
> timestamps in both tracing buffer or dmesg do not reflect suspend time.
>
> Is it different in your x86 systems? Or maybe in other architectures you
> have experience with?
I just expected this to happen for S4 and not S3. Our test environment
is different. I guess the "local" clock is turned off for S3. Perhaps we
should change the trace_clock for one that doesn't stop on suspend. If
you're using x86 TSC clock, maybe try this next time?
# echo x86-tsc > /sys/kernel/debug/tracing/trace_clock
>
> I've done a test on Steam Deck, take a look in both attachments - seems
> to be aligned with my perception. Also checked dmesg on my Intel laptop
> (i5-6300U, with "nonstop_tsc" capability) and the timestamps do not
> reflect the time spent in S3 suspend...
Ok. I'll keep this in mind for future debugs also. Thanks.
>
>
> >>> [...]
> >>> 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?)
>
> Thanks for the clarification! I don't have it at hands anyway,
> unfortunately heh
>
>
> > [...]
> > 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.
>
> OK, tried with this hunk alone, and this is the result:
>
> $ dmesg | grep "suspend\|xhci"
> [227.990149] PM: suspend entry (deep)
> [228.012255] printk: Suspending console(s) (use no_console_suspend to debug)
> [228.041056] xhci-hcd xhci-hcd.2.auto: xhci_plat_suspend:
> device_may_wakeup: 0
This confirms that the xhci platform device created by dwc3 doesn't
enable wakeup. This actually inline with what we thought in the
beginning. But from the discussion and tests you did, we can deduce
further what had happened now.
Can you test another scenario. Connect an HID device such as a keyboard
to the STEAM DECK. Have the DECK run as host. Put it to sleep, check to
see if the keyboard can remote wakeup the DECK. If remote wakeup doesn't
work, can you try this in addition to the previous patch?
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index 4957b9765dc5..f9361e995f5b 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -166,6 +166,9 @@ int dwc3_host_init(struct dwc3 *dwc)
}
}
+ if (dwc->sys_wakeup)
+ device_init_wakeup(&xhci->dev, true);
+
ret = platform_device_add(xhci);
if (ret) {
dev_err(dwc->dev, "failed to register xHCI device\n");
> [228.878517] usb 1-3: reset full-speed USB device number 2 using xhci_hcd
> [229.150502] usb 1-5: reset full-speed USB device number 3 using xhci_hcd
> [229.318882] PM: suspend exit
>
>
> > 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):
> >
> > <patch>
>
> Thanks a bunch Thinh, with this patch applied (plus the above hunk on
> xhci-plat), things work both in host or device mode - I could properly
> suspend and resume after pressing the power button in the Steam Deck.
>
> So, how should we proceed now? Could you send a final/official version
> of the patch? I could test and provide my Tested-by (and proceed with
> backporting to Deck's kernel). Or let me know if you want/need more tests.
>
> Thanks again for all the help/support in this issue =)
No problem. I'm glad we can debug this without probing into the
hardward. I'll rework the patch so that it can be submitted. Your
Tested-by will be very helpful.
Thanks,
Thinh
