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

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

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

Hi,

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.

For anything that works, we *don't* want link state change interrupts.

> 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?

> Fixes: 72704f876f50 ("dwc3: gadget: Implement the suspend entry event handler")
> Signed-off-by: Jack Pham <jackp@codeaurora.org>
> ---
>  drivers/usb/dwc3/gadget.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index dd80e5ca8c78..cab3a9184068 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2323,6 +2323,10 @@ static void dwc3_gadget_enable_irq(struct dwc3 *dwc)
>  	if (DWC3_VER_IS_PRIOR(DWC3, 250A))
>  		reg |= DWC3_DEVTEN_ULSTCNGEN;
>  
> +	/* On 2.30a and above this bit enables U3/L2-L1 Suspend Events */
> +	if (!DWC3_VER_IS_PRIOR(DWC3, 230A))
> +		reg |= DWC3_DEVTEN_EOPFEN;

look at cpu usage for dwc3's interrupt before and after this
IRQ. Specially when connected to a host that fully supports LPM. IIRC,
recent xhci should trigger state changes fairly often.

Still, why do you think you need these events?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 857 bytes --]