Re: [PATCH 1/2] usb: dwc3: gadget: Enable suspend events

From: Felipe Balbi <balbi@kernel.org>
To: Jack Pham <jackp@codeaurora.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Thinh Nguyen <Thinh.Nguyen@synopsys.com>,
	linux-usb@vger.kernel.org, Wesley Cheng <wcheng@codeaurora.org>,
	Baolin Wang <baolin.wang7@gmail.com>
Subject: Re: [PATCH 1/2] usb: dwc3: gadget: Enable suspend events
Date: Fri, 30 Apr 2021 11:39:04 +0300	[thread overview]
Message-ID: <87wnski2tz.fsf@kernel.org> (raw)
In-Reply-To: <20210428153458.GD20698@jackp-linux.qualcomm.com>

[-- Attachment #1: Type: text/plain, Size: 2640 bytes --]

Hi,

Jack Pham <jackp@codeaurora.org> writes:
> Hi Felipe,
>
> On Wed, Apr 28, 2021 at 01:19:51PM +0300, Felipe Balbi wrote:
>> Jack Pham <jackp@codeaurora.org> 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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]