| Message ID | 20210428090111.3370-2-jackp@codeaurora.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6f26ebb79a84bcad211cb2d8a2ef74dfc427322d |
| Headers | show |
| Series | [1/2] usb: dwc3: gadget: Enable suspend events | expand |
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?
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
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 >
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>
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)) { /*
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(-)