Hi,
Jack Pham <jackp@xxxxxxxxxxxxxx> writes:
> Hi Felipe,
>
> On Wed, Apr 28, 2021 at 01:19:51PM +0300, Felipe Balbi wrote:
>> Jack Pham <jackp@xxxxxxxxxxxxxx> 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
Attachment:
signature.asc
Description: PGP signature
