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

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

[Jack Pham]

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

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;
+
 	dwc3_writel(dwc->regs, DWC3_DEVTEN, reg);
 }
 
-- 
2.24.0

[PATCH 2/2] usb: dwc3: gadget: Rename EOPF event macros to Suspend

[Jack Pham]

The device event corresponding to End of Periodic Frame is only
found on older IP revisions (2.10a and prior, according to a
cursory SNPS databook search).  On revisions 2.30a and newer,
including DWC3.1, the same event value and corresponding DEVTEN
bit were repurposed to indicate that the link has gone into
suspend state (U3 or L2/L1).

EOPF events had never been enabled before in this driver, and
going forward we expect current and future DWC3-based devices
won't likely to be using such old DWC3 IP revisions either.
Hence rather than keeping the deprecated EOPF macro names let's
rename them to indicate their usage for suspend events.

Signed-off-by: Jack Pham <jackp@codeaurora.org>
---
 drivers/usb/dwc3/core.h   | 6 +++---
 drivers/usb/dwc3/debug.h  | 8 ++++----
 drivers/usb/dwc3/gadget.c | 4 ++--
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index b1e875c58f20..6091f63151d4 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -57,7 +57,7 @@
 #define DWC3_DEVICE_EVENT_LINK_STATUS_CHANGE	3
 #define DWC3_DEVICE_EVENT_WAKEUP		4
 #define DWC3_DEVICE_EVENT_HIBER_REQ		5
-#define DWC3_DEVICE_EVENT_EOPF			6
+#define DWC3_DEVICE_EVENT_SUSPEND		6
 #define DWC3_DEVICE_EVENT_SOF			7
 #define DWC3_DEVICE_EVENT_ERRATIC_ERROR		9
 #define DWC3_DEVICE_EVENT_CMD_CMPL		10
@@ -460,7 +460,7 @@
 #define DWC3_DEVTEN_CMDCMPLTEN		BIT(10)
 #define DWC3_DEVTEN_ERRTICERREN		BIT(9)
 #define DWC3_DEVTEN_SOFEN		BIT(7)
-#define DWC3_DEVTEN_EOPFEN		BIT(6)
+#define DWC3_DEVTEN_U3L2L1SUSPEN	BIT(6)
 #define DWC3_DEVTEN_HIBERNATIONREQEVTEN	BIT(5)
 #define DWC3_DEVTEN_WKUPEVTEN		BIT(4)
 #define DWC3_DEVTEN_ULSTCNGEN		BIT(3)
@@ -1374,7 +1374,7 @@ struct dwc3_event_depevt {
  *	3	- ULStChng
  *	4	- WkUpEvt
  *	5	- Reserved
- *	6	- EOPF
+ *	6	- Suspend (EOPF on revisions 2.10a and prior)
  *	7	- SOF
  *	8	- Reserved
  *	9	- ErrticErr
diff --git a/drivers/usb/dwc3/debug.h b/drivers/usb/dwc3/debug.h
index db231de46bb3..d0ac89c5b317 100644
--- a/drivers/usb/dwc3/debug.h
+++ b/drivers/usb/dwc3/debug.h
@@ -221,8 +221,8 @@ static inline const char *dwc3_gadget_event_string(char *str, size_t size,
 		snprintf(str, size, "WakeUp [%s]",
 				dwc3_gadget_link_string(state));
 		break;
-	case DWC3_DEVICE_EVENT_EOPF:
-		snprintf(str, size, "End-Of-Frame [%s]",
+	case DWC3_DEVICE_EVENT_SUSPEND:
+		snprintf(str, size, "Suspend [%s]",
 				dwc3_gadget_link_string(state));
 		break;
 	case DWC3_DEVICE_EVENT_SOF:
@@ -353,8 +353,8 @@ static inline const char *dwc3_gadget_event_type_string(u8 event)
 		return "Wake-Up";
 	case DWC3_DEVICE_EVENT_HIBER_REQ:
 		return "Hibernation";
-	case DWC3_DEVICE_EVENT_EOPF:
-		return "End of Periodic Frame";
+	case DWC3_DEVICE_EVENT_SUSPEND:
+		return "Suspend";
 	case DWC3_DEVICE_EVENT_SOF:
 		return "Start of Frame";
 	case DWC3_DEVICE_EVENT_ERRATIC_ERROR:
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index cab3a9184068..6eab78f8a1a7 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2325,7 +2325,7 @@ static void dwc3_gadget_enable_irq(struct dwc3 *dwc)
 
 	/* On 2.30a and above this bit enables U3/L2-L1 Suspend Events */
 	if (!DWC3_VER_IS_PRIOR(DWC3, 230A))
-		reg |= DWC3_DEVTEN_EOPFEN;
+		reg |= DWC3_DEVTEN_U3L2L1SUSPEN;
 
 	dwc3_writel(dwc->regs, DWC3_DEVTEN, reg);
 }
@@ -3744,7 +3744,7 @@ static void dwc3_gadget_interrupt(struct dwc3 *dwc,
 	case DWC3_DEVICE_EVENT_LINK_STATUS_CHANGE:
 		dwc3_gadget_linksts_change_interrupt(dwc, event->event_info);
 		break;
-	case DWC3_DEVICE_EVENT_EOPF:
+	case DWC3_DEVICE_EVENT_SUSPEND:
 		/* It changed to be suspend event for version 2.30a and above */
 		if (!DWC3_VER_IS_PRIOR(DWC3, 230A)) {
 			/*
-- 
2.24.0

Re: [PATCH 2/2] usb: dwc3: gadget: Rename EOPF event macros to Suspend

[Felipe Balbi]

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


Hi,

Jack Pham <jackp@codeaurora.org> writes:
> The device event corresponding to End of Periodic Frame is only
> found on older IP revisions (2.10a and prior, according to a

you're reading databook for dwc3.1, right? Remember the original support
for the driver was on dwc3. This was always called EOPF back then. We
should maintain the name.

> cursory SNPS databook search).  On revisions 2.30a and newer,
> including DWC3.1, the same event value and corresponding DEVTEN
> bit were repurposed to indicate that the link has gone into
> suspend state (U3 or L2/L1).
>
> EOPF events had never been enabled before in this driver, and
> going forward we expect current and future DWC3-based devices
> won't likely to be using such old DWC3 IP revisions either.

We still have original omap5 devices, running on revision 1.73a
around. They'll remain supported for the time being.

> Hence rather than keeping the deprecated EOPF macro names let's
> rename them to indicate their usage for suspend events.

what do we gain from this change? I mean, in practice, what changes?
nothing realy, so why should we apply this?

-- 
balbi

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

Re: [PATCH 2/2] usb: dwc3: gadget: Rename EOPF event macros to Suspend

[Jack Pham]

Hi Felipe,

On Wed, Apr 28, 2021 at 01:28:16PM +0300, Felipe Balbi wrote:
> Jack Pham <jackp@codeaurora.org> writes:
> > The device event corresponding to End of Periodic Frame is only
> > found on older IP revisions (2.10a and prior, according to a
> 
> you're reading databook for dwc3.1, right? Remember the original support
> for the driver was on dwc3. This was always called EOPF back then. We
> should maintain the name.

I've looked through several revisions of the databook for both dwc3 and
dwc3.1. From what I can tell EOPF was nixed starting in DWC3 (not 3.1)
revision 2.20a. DWC3 revision 2.30a re-introduced event #6 for USB
suspend. And judging from the IP revision list in core.h, DWC3 is now
up to 3.30a (DWC3_REVISION_330A), so from number alone there are about
as many revisions that have this bit as EOPF as there are that use it
for SUSPEND. This carries over to DWC3.1 as well (not sure about DWC3.2)
so in fact there are probably more revisions of IP that no longer use
EOPF.

Hi Thinh, I'm wondering if you could please help corroborate the history
of this bit, and confirm whether it is also used as Suspend entry in DWC
3.2 IPs?

But I don't want to make it seem that I'm using revision history as a
gauge of how many real devices out there support EOPF vs Suspend. That
figure we'll never truly know.

> > cursory SNPS databook search).  On revisions 2.30a and newer,
> > including DWC3.1, the same event value and corresponding DEVTEN
> > bit were repurposed to indicate that the link has gone into
> > suspend state (U3 or L2/L1).
> >
> > EOPF events had never been enabled before in this driver, and
> > going forward we expect current and future DWC3-based devices
> > won't likely to be using such old DWC3 IP revisions either.
> 
> We still have original omap5 devices, running on revision 1.73a
> around. They'll remain supported for the time being.
> 
> > Hence rather than keeping the deprecated EOPF macro names let's
> > rename them to indicate their usage for suspend events.
> 
> what do we gain from this change? I mean, in practice, what changes?
> nothing realy, so why should we apply this?

I'm saying since this macro has never really been used to enable any
kind of event handling specifically for "End Of Periodic Frame", that
there is not much utility in keeping the name as EOPF. Instead as I
explained in patch 1, the same bit/event is used on newer revisions for
USB Suspend entry so assuming you accept that, then the purpose of this
follow-on patch is simply to make the code more readable by renaming the
macro to fit its usage.

Thanks,
Jack
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 2/2] usb: dwc3: gadget: Rename EOPF event macros to Suspend

[Thinh Nguyen]

Jack Pham wrote:
> Hi Felipe,
> 
> On Wed, Apr 28, 2021 at 01:28:16PM +0300, Felipe Balbi wrote:
>> Jack Pham <jackp@codeaurora.org> writes:
>>> The device event corresponding to End of Periodic Frame is only
>>> found on older IP revisions (2.10a and prior, according to a
>>
>> you're reading databook for dwc3.1, right? Remember the original support
>> for the driver was on dwc3. This was always called EOPF back then. We
>> should maintain the name.
> 
> I've looked through several revisions of the databook for both dwc3 and
> dwc3.1. From what I can tell EOPF was nixed starting in DWC3 (not 3.1)
> revision 2.20a. DWC3 revision 2.30a re-introduced event #6 for USB
> suspend. And judging from the IP revision list in core.h, DWC3 is now
> up to 3.30a (DWC3_REVISION_330A), so from number alone there are about
> as many revisions that have this bit as EOPF as there are that use it
> for SUSPEND. This carries over to DWC3.1 as well (not sure about DWC3.2)
> so in fact there are probably more revisions of IP that no longer use
> EOPF.
> 
> Hi Thinh, I'm wondering if you could please help corroborate the history
> of this bit, and confirm whether it is also used as Suspend entry in DWC
> 3.2 IPs?

DWC_usb32 IP is also the same as DWC_usb31 and uses it for suspend event.

BR,
Thinh

> 
> But I don't want to make it seem that I'm using revision history as a
> gauge of how many real devices out there support EOPF vs Suspend. That
> figure we'll never truly know.
> 
>>> cursory SNPS databook search).  On revisions 2.30a and newer,
>>> including DWC3.1, the same event value and corresponding DEVTEN
>>> bit were repurposed to indicate that the link has gone into
>>> suspend state (U3 or L2/L1).
>>>
>>> EOPF events had never been enabled before in this driver, and
>>> going forward we expect current and future DWC3-based devices
>>> won't likely to be using such old DWC3 IP revisions either.
>>
>> We still have original omap5 devices, running on revision 1.73a
>> around. They'll remain supported for the time being.
>>
>>> Hence rather than keeping the deprecated EOPF macro names let's
>>> rename them to indicate their usage for suspend events.
>>
>> what do we gain from this change? I mean, in practice, what changes?
>> nothing realy, so why should we apply this?
> 
> I'm saying since this macro has never really been used to enable any
> kind of event handling specifically for "End Of Periodic Frame", that
> there is not much utility in keeping the name as EOPF. Instead as I
> explained in patch 1, the same bit/event is used on newer revisions for
> USB Suspend entry so assuming you accept that, then the purpose of this
> follow-on patch is simply to make the code more readable by renaming the
> macro to fit its usage.
> 
> Thanks,
> Jack
> 

Re: [PATCH 2/2] usb: dwc3: gadget: Rename EOPF event macros to Suspend

[Felipe Balbi]

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

Jack Pham <jackp@codeaurora.org> writes:

> The device event corresponding to End of Periodic Frame is only
> found on older IP revisions (2.10a and prior, according to a
> cursory SNPS databook search).  On revisions 2.30a and newer,
> including DWC3.1, the same event value and corresponding DEVTEN
> bit were repurposed to indicate that the link has gone into
> suspend state (U3 or L2/L1).
>
> EOPF events had never been enabled before in this driver, and
> going forward we expect current and future DWC3-based devices
> won't likely to be using such old DWC3 IP revisions either.
> Hence rather than keeping the deprecated EOPF macro names let's
> rename them to indicate their usage for suspend events.
>
> Signed-off-by: Jack Pham <jackp@codeaurora.org>

Acked-by: Felipe Balbi <balbi@kernel.org>

-- 
balbi

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

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

[Felipe Balbi]

[-- 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 --]

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

[Jack Pham]

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.

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

Fully agree. I've seen the "link state change" interrupts in action and
they are very noisy as they get generated for every single link event,
particularly the USB 3.x LTSSM, including U0, U1, U2, Compliance,
Polling, etc. in addition to the above U3, L2 & L1.

But I am not proposing re-enabling that particular event type here.

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

  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)

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

In our experience for U3 & L2 they are only as frequent as the host
enters suspend, which is not that frequent unless the user is toggling
system suspend/resume like crazy or has autosuspend enabled with a very
short timeout.

You are right that LPM L1 might happen more often when connected to a
host that supports it. I'll try to collect some stats in that case to
see what is a typical count for a given connection. It's worth noting
that the dwc3_gadget_suspend_interrupt() handler specifically checks
whether it is U3 (L2 if 2.0) before calling dwc3_gadget_suspend()
otherwise it just stores the current link_state and exits.  So almost a
no-op but not quite since it does incur the cost of an interrupt every
time it happens.

> Still, why do you think you need these events?

Answered above. I'll add that specifically composite_suspend() also
invokes each function driver's .suspend() callback which is needed in
case functions have to stop any outstanding endpoint requests and
restart them in the .resume() handler. And finally composite_suspend()
also invokes the vbus_draw() callback which is needed to properly
inform a power supply or charger to stop drawing current to comply with
USB suspend power requirements.

Thanks,
Jack
-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

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

[Felipe Balbi]

[-- 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 --]

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

[Felipe Balbi]

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

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
> 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.
>
> Fixes: 72704f876f50 ("dwc3: gadget: Implement the suspend entry event handler")
> Signed-off-by: Jack Pham <jackp@codeaurora.org>

Acked-by: Felipe Balbi <balbi@kernel.org>

-- 
balbi

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