Hi, Jack Pham writes: > Hi Felipe, > > On Wed, Apr 28, 2021 at 01:19:51PM +0300, Felipe Balbi wrote: >> Jack Pham writes: >> > commit 72704f876f50 ("dwc3: gadget: Implement the suspend entry event >> > handler") introduced (nearly 5 years ago!) an interrupt handler for >> > U3/L1-L2 suspend events. The problem is that these events aren't >> >> look deeper. They *were* enabled. We've removed because, as it turns >> out, they just add a TON of interrupts and don't give us much extra >> information. The only reason why the state change interrupts are still >> there is because of a known silicon bug in versions < 2.50a. That's all >> documented in the driver itself. > > I did go through the commit history. Are you referring to your change > 799e9dc82968 ("usb: dwc3: gadget: conditionally disable Link State > change events")? If so then it sounds like you are talking about the > link state change event, defined as event value 3 and enabled with > DEVTEN bit 3. > > The "link state change event" is *not* the same as the one I'm referring > to in this patch which is documented in newer revisions of the databook > (both DWC3 and DWC3.1) as "USB Suspend Entry" (event 6). It's described > as only getting generated when the link enters U3, L2 or L1 states. heh, I need some sleep, apparently :-) >> > currently enabled in the DEVTEN register so the handler is never >> > even invoked. Fix this simply by enabling the corresponding bit >> > in dwc3_gadget_enable_irq() using the same revision check as found >> > in the handler. >> >> More importantly, *why* do you think you need these interrupts? > > Bus suspend and resume are useful conditions to be notified about-- > that's why we have the .suspend() & .resume() callbacks in struct > usb_gadget_driver. But currently the dwc3 gadget does not have any > interrupt generated for suspend, and as of now the dwc3_gadget_suspend() > does not get called, so it will never invoke the gadget driver's (let's > say composite.c) .suspend() routine. > > dwc3_gadget_suspend() is called from two places: understood. > 1. dwc3_gadget_linksts_change_interrupt() - which is the handler for > DWC3_DEVICE_EVENT_LINK_STATUS_CHANGE, the one I believe you are > referring to and is only enabled on revisions < 2.50a. > > 2. dwc3_gadget_suspend_interrupt() - which is the handler for the > DWC3_DEVICE_EVENT_EOPF (which I'm promptly renaming to > DWC3_DEVICE_EVENT_SUSPEND in patch 2/2) Right, now it all makes sense. Thanks for clarifying. -- balbi