usb: dwc3: Decouple USB 2.0 L1 & L2 events

PATCH In review Jack Pham 8 participants 11 messages Aug 12, 2021 – Aug 20, 2021

Subsystems (2)

USB DesignWare USB3 DRD IP driver

lore

Share & tools

lore

Jack Pham

author

2021-08-12 08:26 UTC #0 lore raw

PATCH 1/1 usb: dwc3: Decouple USB 2.0 L1 & L2 events

On DWC_usb3 revisions 3.00a and newer (including DWC_usb31 and DWC_usb32) the GUCTL1 register gained the DEV_DECOUPLE_L1L2_EVT field (bit 31) which when enabled allows the controller in device mode to treat USB 2.0 L1 LPM & L2 events separately.

After commit d1d90dd27254 ("usb: dwc3: gadget: Enable suspend events") the controller will now receive events (and therefore interrupts) for every state change when entering/exiting either L1 or L2 states. Since L1 is handled entirely by the hardware and requires no software intervention, there is no need to even enable these events and unnecessarily notify the gadget driver. Enable the aforementioned bit to help reduce the overall interrupt count for these L1 events that don't need to be handled while retaining the events for full L2 suspend/wakeup.

Signed-off-by Jack Pham <[email protected]>

---
 drivers/usb/dwc3/core.c | 9 +++++++++
 drivers/usb/dwc3/core.h | 5 +++--
 2 files changed, 12 insertions(+), 2 deletions(-)

drivers/usb/dwc3/core.c +9 -0

index ba74ad7f6995..719dac228703 100644
@@ -1050,6 +1050,15 @@ static int dwc3_core_init(struct dwc3 *dwc)
 		if (!DWC3_VER_IS_PRIOR(DWC3, 290A))
 			reg |= DWC3_GUCTL1_DEV_L1_EXIT_BY_HW;
 
+		/*
+		 * Decouple USB 2.0 L1 & L2 events which will allow for
+		 * gadget driver to only receive U3/L2 suspend & wakeup
+		 * events and prevent the more frequent L1 LPM transitions
+		 * from interrupting the driver.
+		 */
+		if (!DWC3_VER_IS_PRIOR(DWC3, 300A))
+			reg |= DWC3_GUCTL1_DEV_DECOUPLE_L1L2_EVT;
+
 		if (dwc->dis_tx_ipgap_linecheck_quirk)
 			reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;

drivers/usb/dwc3/core.h +3 -2

index 5991766239ba..356b94a7ec70 100644
@@ -256,9 +256,10 @@
 #define DWC3_GUCTL_HSTINAUTORETRY	BIT(14)
 
 /* Global User Control 1 Register */
-#define DWC3_GUCTL1_PARKMODE_DISABLE_SS	BIT(17)
+#define DWC3_GUCTL1_DEV_DECOUPLE_L1L2_EVT	BIT(31)
 #define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS	BIT(28)
-#define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW	BIT(24)
+#define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW		BIT(24)
+#define DWC3_GUCTL1_PARKMODE_DISABLE_SS		BIT(17)
 
 /* Global Status Register */
 #define DWC3_GSTS_OTG_IP	BIT(10)

Signature · 1 line

2.24.0

Jack Pham

author

2021-08-18 01:29 UTC #1 lore raw

Quoted 5 lines · Jack Pham

On DWC_usb3 revisions 3.00a and newer (including DWC_usb31 and DWC_usb32) the GUCTL1 register gained the DEV_DECOUPLE_L1L2_EVT field (bit 31) which when enabled allows the controller in device mode to treat USB 2.0 L1 LPM & L2 events separately.

Quoted 9 lines

After commit d1d90dd27254 ("usb: dwc3: gadget: Enable suspend events") the controller will now receive events (and therefore interrupts) for every state change when entering/exiting either L1 or L2 states. Since L1 is handled entirely by the hardware and requires no software intervention, there is no need to even enable these events and unnecessarily notify the gadget driver. Enable the aforementioned bit to help reduce the overall interrupt count for these L1 events that don't need to be handled while retaining the events for full L2 suspend/wakeup.

Hi folks in To:

I'd like to request if any of you could help test this patch on your boards to help make sure it doesn't cause any regressions since I know some of the recent dwc3 patches from Qualcomm have been found to break other devices :(. So I'm hoping to avoid that even for a patch as small as this.

Hoping this could be tried out on boards/SoCs such as db845c, hikey960, Exynos, the Intel "lakes", etc. Ideally this needs validation with a high-speed connection to a USB 3.x host, which increases the chances that USB 2.0 Link Power Management is supported.

The overall goal of this patch is to eliminate events generated for L1 entry/exit, so we should see a slight reduction in interrupt counts when checking grep dwc3 /proc/interrupts for comparable traffic.

Appreciate any feedback and help in testing!
Thanks,
Jack

Quoted 46 lines

Signed-off-by Jack Pham <[email protected]>

---
 drivers/usb/dwc3/core.c | 9 +++++++++
 drivers/usb/dwc3/core.h | 5 +++--
 2 files changed, 12 insertions(+), 2 deletions(-)

drivers/usb/dwc3/core.c +9 -0

index ba74ad7f6995..719dac228703 100644
@@ -1050,6 +1050,15 @@ static int dwc3_core_init(struct dwc3 *dwc)
 		if (!DWC3_VER_IS_PRIOR(DWC3, 290A))
 			reg |= DWC3_GUCTL1_DEV_L1_EXIT_BY_HW;
 
+		/*
+		 * Decouple USB 2.0 L1 & L2 events which will allow for
+		 * gadget driver to only receive U3/L2 suspend & wakeup
+		 * events and prevent the more frequent L1 LPM transitions
+		 * from interrupting the driver.
+		 */
+		if (!DWC3_VER_IS_PRIOR(DWC3, 300A))
+			reg |= DWC3_GUCTL1_DEV_DECOUPLE_L1L2_EVT;
+
 		if (dwc->dis_tx_ipgap_linecheck_quirk)
 			reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;

drivers/usb/dwc3/core.h +3 -2

index 5991766239ba..356b94a7ec70 100644
@@ -256,9 +256,10 @@
 #define DWC3_GUCTL_HSTINAUTORETRY	BIT(14)
 
 /* Global User Control 1 Register */
-#define DWC3_GUCTL1_PARKMODE_DISABLE_SS	BIT(17)
+#define DWC3_GUCTL1_DEV_DECOUPLE_L1L2_EVT	BIT(31)
 #define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS	BIT(28)
-#define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW	BIT(24)
+#define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW		BIT(24)
+#define DWC3_GUCTL1_PARKMODE_DISABLE_SS		BIT(17)
 
 /* Global Status Register */
 #define DWC3_GSTS_OTG_IP	BIT(10)

Signature · 1 line

2.24.0

Signature · 2 lines

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

John Stultz

Tested-by

2021-08-18 02:08 UTC #2 lore raw

Quoted 25 lines · Jack Pham

Quoted 5 lines · Jack Pham
On DWC_usb3 revisions 3.00a and newer (including DWC_usb31 and DWC_usb32) the GUCTL1 register gained the DEV_DECOUPLE_L1L2_EVT field (bit 31) which when enabled allows the controller in device mode to treat USB 2.0 L1 LPM & L2 events separately.
Quoted 9 lines
After commit d1d90dd27254 ("usb: dwc3: gadget: Enable suspend events") the controller will now receive events (and therefore interrupts) for every state change when entering/exiting either L1 or L2 states. Since L1 is handled entirely by the hardware and requires no software intervention, there is no need to even enable these events and unnecessarily notify the gadget driver. Enable the aforementioned bit to help reduce the overall interrupt count for these L1 events that don't need to be handled while retaining the events for full L2 suspend/wakeup.
Hi folks in To:
I'd like to request if any of you could help test this patch on your boards to help make sure it doesn't cause any regressions since I know some of the recent dwc3 patches from Qualcomm have been found to break other devices :(. So I'm hoping to avoid that even for a patch as small as this.

Quoted 9 lines

Hoping this could be tried out on boards/SoCs such as db845c, hikey960, Exynos, the Intel "lakes", etc. Ideally this needs validation with a high-speed connection to a USB 3.x host, which increases the chances that USB 2.0 Link Power Management is supported.
The overall goal of this patch is to eliminate events generated for L1 entry/exit, so we should see a slight reduction in interrupt counts when checking grep dwc3 /proc/interrupts for comparable traffic.

I don't have a ton of before/after interrupts data, but I applied this and booted on both db845c and HiKey960 and haven't seen any negative effects so far.

HiKey960 only connects USB2 to the usb-c gadget port so I can't really test with a USB3 host.

On db845c, I'm also only seeing high-speed connections but I'm not sure if that's because the usb3 labeled port on my build server isn't really usb3, or if its due to something else being off (I did double check I've got a proper usb3 A/C cable, as a number of my cables are usb2 A/C).

But I've pushed and pulled a few files, run logcat for awhile and unplugged and re-pluggeded the cable a few times on both devices and it seems ok to me.

So for what it's worth:

Tested-by John Stultz <[email protected]> # for HiKey960 & db845c

thanks
-john

Jun Li

Tested-by Reviewed-by

2021-08-18 07:52 UTC #3 lore raw

In reply to Jack Pham

Quoted 45 lines

-----Original Message-----
From: [email protected] <[email protected]> On Behalf Of Jack Pham
Sent: Wednesday, August 18, 2021 9:29 AM
To: Thinh Nguyen <[email protected]>; John Stultz <[email protected]>; Amit Pundir <[email protected]>; Ray Chi <[email protected]>; Ferry Toth <[email protected]>; Chunfeng Yun <[email protected]>; Andy Shevchenko <[email protected]>; Marek Szyprowski <[email protected]>; Jun Li <[email protected]>
Cc: Felipe Balbi <[email protected]>; Greg Kroah-Hartman <[email protected]>; [email protected]; Wesley Cheng <[email protected]>
Subject: Re: [RFT][PATCH] usb: dwc3: Decouple USB 2.0 L1 & L2 events
Quoted 5 lines · Jack Pham
On DWC_usb3 revisions 3.00a and newer (including DWC_usb31 and DWC_usb32) the GUCTL1 register gained the DEV_DECOUPLE_L1L2_EVT field (bit 31) which when enabled allows the controller in device mode to treat USB 2.0 L1 LPM & L2 events separately.
Quoted 9 lines
After commit d1d90dd27254 ("usb: dwc3: gadget: Enable suspend events") the controller will now receive events (and therefore interrupts) for every state change when entering/exiting either L1 or L2 states. Since L1 is handled entirely by the hardware and requires no software intervention, there is no need to even enable these events and unnecessarily notify the gadget driver. Enable the aforementioned bit to help reduce the overall interrupt count for these L1 events that don't need to be handled while retaining the events for full L2 suspend/wakeup.
Hi folks in To:
I'd like to request if any of you could help test this patch on your boards to help make sure it doesn't cause any regressions since I know some of the recent dwc3 patches from Qualcomm have been found to break other devices :(. So I'm hoping to avoid that even for a patch as small as this.
Hoping this could be tried out on boards/SoCs such as db845c, hikey960, Exynos, the Intel "lakes", etc. Ideally this needs validation with a high-speed connection to a USB 3.x host, which increases the chances that USB 2.0 Link Power Management is supported.

Quoted 5 lines

The overall goal of this patch is to eliminate events generated for L1 entry/exit, so we should see a slight reduction in interrupt counts when checking grep dwc3 /proc/interrupts for comparable traffic.
Appreciate any feedback and help in testing!

Applied the patch and tested on iMX8MP, can't see L1 sleep event of below in ftrace:
"irq/48-dwc3-557 [000] d..1 264.478691: dwc3_event: event (00020601): Suspend [U2]"
by connecting to USB2 port of my Dell host, so

Tested-by Jun Li <[email protected]>

Reviewed-by Jun Li <[email protected]>

Quoted 53 lines

Thanks,
Jack

Quoted 44 lines

Signed-off-by Jack Pham <[email protected]>

---
 drivers/usb/dwc3/core.c | 9 +++++++++  drivers/usb/dwc3/core.h | 5
+++--
 2 files changed, 12 insertions(+), 2 deletions(-)

drivers/usb/dwc3/core.c index renamed from drivers/usb/dwc3/core.c +0 -0

ba74ad7f6995..719dac228703 100644

drivers/usb/dwc3/core.c +9 -0

@@ -1050,6 +1050,15 @@ static int dwc3_core_init(struct dwc3 *dwc)
 		if (!DWC3_VER_IS_PRIOR(DWC3, 290A))
 			reg |= DWC3_GUCTL1_DEV_L1_EXIT_BY_HW;
 
+		/*
+		 * Decouple USB 2.0 L1 & L2 events which will allow for
+		 * gadget driver to only receive U3/L2 suspend & wakeup
+		 * events and prevent the more frequent L1 LPM transitions
+		 * from interrupting the driver.
+		 */
+		if (!DWC3_VER_IS_PRIOR(DWC3, 300A))
+			reg |= DWC3_GUCTL1_DEV_DECOUPLE_L1L2_EVT;
+
 		if (dwc->dis_tx_ipgap_linecheck_quirk)
 			reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;

drivers/usb/dwc3/core.h index renamed from drivers/usb/dwc3/core.h +0 -0

5991766239ba..356b94a7ec70 100644

drivers/usb/dwc3/core.h +3 -3

@@ -256,9 +256,10 @@
 #define DWC3_GUCTL_HSTINAUTORETRY	BIT(14)
 
 /* Global User Control 1 Register */
-#define DWC3_GUCTL1_PARKMODE_DISABLE_SS	BIT(17)
+#define DWC3_GUCTL1_DEV_DECOUPLE_L1L2_EVT	BIT(31)
 #define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS	BIT(28)
-#define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW	BIT(24)
+#define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW		BIT(24)
+#define DWC3_GUCTL1_PARKMODE_DISABLE_SS		BIT(17)
 
 /* Global Status Register */
 #define DWC3_GSTS_OTG_IP	BIT(10)
--

Quoted 2 lines

2.24.0

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

Amit Pundir

Tested-by

2021-08-18 09:09 UTC #4 lore raw

In reply to Jack Pham

Quoted 30 lines · Jack Pham

Quoted 5 lines · Jack Pham
On DWC_usb3 revisions 3.00a and newer (including DWC_usb31 and DWC_usb32) the GUCTL1 register gained the DEV_DECOUPLE_L1L2_EVT field (bit 31) which when enabled allows the controller in device mode to treat USB 2.0 L1 LPM & L2 events separately.
Quoted 9 lines
After commit d1d90dd27254 ("usb: dwc3: gadget: Enable suspend events") the controller will now receive events (and therefore interrupts) for every state change when entering/exiting either L1 or L2 states. Since L1 is handled entirely by the hardware and requires no software intervention, there is no need to even enable these events and unnecessarily notify the gadget driver. Enable the aforementioned bit to help reduce the overall interrupt count for these L1 events that don't need to be handled while retaining the events for full L2 suspend/wakeup.
Hi folks in To:
I'd like to request if any of you could help test this patch on your boards to help make sure it doesn't cause any regressions since I know some of the recent dwc3 patches from Qualcomm have been found to break other devices :(. So I'm hoping to avoid that even for a patch as small as this.
Hoping this could be tried out on boards/SoCs such as db845c, hikey960, Exynos, the Intel "lakes", etc. Ideally this needs validation with a high-speed connection to a USB 3.x host, which increases the chances that USB 2.0 Link Power Management is supported.

Quoted 5 lines

The overall goal of this patch is to eliminate events generated for L1 entry/exit, so we should see a slight reduction in interrupt counts when checking grep dwc3 /proc/interrupts for comparable traffic.
Appreciate any feedback and help in testing!

Smoke tested this patch on RB5, running android-mainline (v5.14-rc6) kernel, with a USB 3.x host.
Pushed/Pulled couple of large files over ADB, hotplugged the cable a few times and found no obvious regressions.

Tested-by Amit Pundir <[email protected]> # for RB5 (sm8250)

Regards,
Amit Pundir

Quoted 53 lines

Thanks,
Jack

Quoted 44 lines

Signed-off-by Jack Pham <[email protected]>

---
 drivers/usb/dwc3/core.c | 9 +++++++++
 drivers/usb/dwc3/core.h | 5 +++--
 2 files changed, 12 insertions(+), 2 deletions(-)

drivers/usb/dwc3/core.c +9 -0

index ba74ad7f6995..719dac228703 100644
@@ -1050,6 +1050,15 @@ static int dwc3_core_init(struct dwc3 *dwc)
              if (!DWC3_VER_IS_PRIOR(DWC3, 290A))
                      reg |= DWC3_GUCTL1_DEV_L1_EXIT_BY_HW;
 
+             /*
+              * Decouple USB 2.0 L1 & L2 events which will allow for
+              * gadget driver to only receive U3/L2 suspend & wakeup
+              * events and prevent the more frequent L1 LPM transitions
+              * from interrupting the driver.
+              */
+             if (!DWC3_VER_IS_PRIOR(DWC3, 300A))
+                     reg |= DWC3_GUCTL1_DEV_DECOUPLE_L1L2_EVT;
+
              if (dwc->dis_tx_ipgap_linecheck_quirk)
                      reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;

drivers/usb/dwc3/core.h +3 -3

index 5991766239ba..356b94a7ec70 100644
@@ -256,9 +256,10 @@
 #define DWC3_GUCTL_HSTINAUTORETRY    BIT(14)
 
 /* Global User Control 1 Register */
-#define DWC3_GUCTL1_PARKMODE_DISABLE_SS      BIT(17)
+#define DWC3_GUCTL1_DEV_DECOUPLE_L1L2_EVT    BIT(31)
 #define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS   BIT(28)
-#define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW        BIT(24)
+#define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW                BIT(24)
+#define DWC3_GUCTL1_PARKMODE_DISABLE_SS              BIT(17)
 
 /* Global Status Register */
 #define DWC3_GSTS_OTG_IP     BIT(10)
--

Quoted 2 lines

2.24.0

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

Andy Shevchenko

2021-08-18 09:35 UTC #5 lore raw

In reply to Jack Pham

Quoted 24 lines · Jack Pham

Quoted 5 lines · Jack Pham
On DWC_usb3 revisions 3.00a and newer (including DWC_usb31 and DWC_usb32) the GUCTL1 register gained the DEV_DECOUPLE_L1L2_EVT field (bit 31) which when enabled allows the controller in device mode to treat USB 2.0 L1 LPM & L2 events separately.
Quoted 9 lines
After commit d1d90dd27254 ("usb: dwc3: gadget: Enable suspend events") the controller will now receive events (and therefore interrupts) for every state change when entering/exiting either L1 or L2 states. Since L1 is handled entirely by the hardware and requires no software intervention, there is no need to even enable these events and unnecessarily notify the gadget driver. Enable the aforementioned bit to help reduce the overall interrupt count for these L1 events that don't need to be handled while retaining the events for full L2 suspend/wakeup.
Hi folks in To:
I'd like to request if any of you could help test this patch on your boards to help make sure it doesn't cause any regressions since I know some of the recent dwc3 patches from Qualcomm have been found to break other devices :(. So I'm hoping to avoid that even for a patch as small as this.

Quoted 8 lines

Hoping this could be tried out on boards/SoCs such as db845c, hikey960, Exynos, the Intel "lakes", etc. Ideally this needs validation with a high-speed connection to a USB 3.x host, which increases the chances that USB 2.0 Link Power Management is supported.
The overall goal of this patch is to eliminate events generated for L1 entry/exit, so we should see a slight reduction in interrupt counts when checking grep dwc3 /proc/interrupts for comparable traffic.

Unfortunately I'm quite busy lately with more important stuff and I dunno if I will be able to test this in reasonable time. So, if Ferry volunteers, then we can cover Intel Merrifield platform as well.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Ferry Toth

Tested-by

2021-08-18 19:49 UTC #6 lore raw

Op 18-08-2021 om 11:33 schreef Andy Shevchenko:

Quoted 25 lines · Jack Pham

Quoted 24 lines · Jack Pham
Quoted 5 lines
On DWC_usb3 revisions 3.00a and newer (including DWC_usb31 and DWC_usb32) the GUCTL1 register gained the DEV_DECOUPLE_L1L2_EVT field (bit 31) which when enabled allows the controller in device mode to treat USB 2.0 L1 LPM & L2 events separately.
Quoted 9 lines
After commit d1d90dd27254 ("usb: dwc3: gadget: Enable suspend events") the controller will now receive events (and therefore interrupts) for every state change when entering/exiting either L1 or L2 states. Since L1 is handled entirely by the hardware and requires no software intervention, there is no need to even enable these events and unnecessarily notify the gadget driver. Enable the aforementioned bit to help reduce the overall interrupt count for these L1 events that don't need to be handled while retaining the events for full L2 suspend/wakeup.
Hi folks in To:
I'd like to request if any of you could help test this patch on your boards to help make sure it doesn't cause any regressions since I know some of the recent dwc3 patches from Qualcomm have been found to break other devices :(. So I'm hoping to avoid that even for a patch as small as this.

Quoted 4 lines

Quoted 4 lines
Hoping this could be tried out on boards/SoCs such as db845c, hikey960, Exynos, the Intel "lakes", etc. Ideally this needs validation with a high-speed connection to a USB 3.x host, which increases the chances that USB 2.0 Link Power Management is supported.

Merrifield: We currently have PROPERTY_ENTRY_BOOL("snps,usb2-gadget-lpm-disable")

Should I retest with this reverted?

Quoted 3 lines

Quoted 3 lines
The overall goal of this patch is to eliminate events generated for L1 entry/exit, so we should see a slight reduction in interrupt counts when checking grep dwc3 /proc/interrupts for comparable traffic.

I didn't compare interrupts

Quoted 4 lines

Unfortunately I'm quite busy lately with more important stuff and I dunno if I will be able to test this in reasonable time. So, if Ferry volunteers, then we can cover Intel Merrifield platform as well.

Performance unchanged, no regressions found.

Tested-by Ferry Toth <[email protected]> # for Merrifield

Andy Shevchenko

2021-08-19 12:26 UTC #7 lore raw

Quoted 3 lines · Ferry Toth

Op 18-08-2021 om 11:33 schreef Andy Shevchenko:
Quoted 2 lines · Jack Pham
Quoted 1 line · Jack Pham

...

Quoted 4 lines

Quoted 3 lines
Quoted 3 lines
The overall goal of this patch is to eliminate events generated for L1 entry/exit, so we should see a slight reduction in interrupt counts when checking grep dwc3 /proc/interrupts for comparable traffic.

Quoted 8 lines

I didn't compare interrupts
Quoted 3 lines
Unfortunately I'm quite busy lately with more important stuff and I dunno if I will be able to test this in reasonable time. So, if Ferry volunteers, then we can cover Intel Merrifield platform as well.
Performance unchanged, no regressions found.
Tested-by Ferry Toth <[email protected]> # for Merrifield

Thank you, Ferry!

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Jack Pham

author

2021-08-20 12:18 UTC #8 lore raw

In reply to Ferry Toth

Quoted 31 lines · Ferry Toth

Op 18-08-2021 om 11:33 schreef Andy Shevchenko:
Quoted 25 lines · Jack Pham
Quoted 24 lines · Jack Pham
Quoted 5 lines
On DWC_usb3 revisions 3.00a and newer (including DWC_usb31 and DWC_usb32) the GUCTL1 register gained the DEV_DECOUPLE_L1L2_EVT field (bit 31) which when enabled allows the controller in device mode to treat USB 2.0 L1 LPM & L2 events separately.
Quoted 9 lines
After commit d1d90dd27254 ("usb: dwc3: gadget: Enable suspend events") the controller will now receive events (and therefore interrupts) for every state change when entering/exiting either L1 or L2 states. Since L1 is handled entirely by the hardware and requires no software intervention, there is no need to even enable these events and unnecessarily notify the gadget driver. Enable the aforementioned bit to help reduce the overall interrupt count for these L1 events that don't need to be handled while retaining the events for full L2 suspend/wakeup.
Hi folks in To:
I'd like to request if any of you could help test this patch on your boards to help make sure it doesn't cause any regressions since I know some of the recent dwc3 patches from Qualcomm have been found to break other devices :(. So I'm hoping to avoid that even for a patch as small as this.
Quoted 4 lines
Quoted 4 lines
Hoping this could be tried out on boards/SoCs such as db845c, hikey960, Exynos, the Intel "lakes", etc. Ideally this needs validation with a high-speed connection to a USB 3.x host, which increases the chances that USB 2.0 Link Power Management is supported.

Quoted 4 lines

Merrifield: We currently have PROPERTY_ENTRY_BOOL("snps,usb2-gadget-lpm-disable")
Should I retest with this reverted?

Maybe best to leave it, since it looks like you had added this quirk for other L1 LPM incompatibility issues you faced on this platform.

Quoted 6 lines

Quoted 3 lines
Quoted 3 lines
The overall goal of this patch is to eliminate events generated for L1 entry/exit, so we should see a slight reduction in interrupt counts when checking grep dwc3 /proc/interrupts for comparable traffic.
I didn't compare interrupts

Quoted 7 lines

Quoted 4 lines
Unfortunately I'm quite busy lately with more important stuff and I dunno if I will be able to test this in reasonable time. So, if Ferry volunteers, then we can cover Intel Merrifield platform as well.
Performance unchanged, no regressions found.
Tested-by Ferry Toth <[email protected]> # for Merrifield

At least it is a no-op for you. Thanks for verifying!

Jack

Signature · 2 lines

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

Thinh Nguyen

maintainer

2021-08-19 02:01 UTC #9 lore raw

In reply to Jack Pham

Quoted 6 lines · Jack Pham

Quoted 5 lines · Jack Pham
On DWC_usb3 revisions 3.00a and newer (including DWC_usb31 and DWC_usb32) the GUCTL1 register gained the DEV_DECOUPLE_L1L2_EVT field (bit 31) which when enabled allows the controller in device mode to treat USB 2.0 L1 LPM & L2 events separately.

Quoted 10 lines

Quoted 9 lines
After commit d1d90dd27254 ("usb: dwc3: gadget: Enable suspend events") the controller will now receive events (and therefore interrupts) for every state change when entering/exiting either L1 or L2 states. Since L1 is handled entirely by the hardware and requires no software intervention, there is no need to even enable these events and unnecessarily notify the gadget driver. Enable the aforementioned bit to help reduce the overall interrupt count for these L1 events that don't need to be handled while retaining the events for full L2 suspend/wakeup.

Currently dwc3 doesn't handle or do anything more for L1 suspend/resume, so it doesn't care about L1 suspend/wake events. For your design, there maybe no more power saving can be done at L1 from the driver, but it may not apply to all others. Other than that, the change is fine.

BR,
Thinh

Quoted 68 lines

Hi folks in To:
I'd like to request if any of you could help test this patch on your boards to help make sure it doesn't cause any regressions since I know some of the recent dwc3 patches from Qualcomm have been found to break other devices :(. So I'm hoping to avoid that even for a patch as small as this.
Hoping this could be tried out on boards/SoCs such as db845c, hikey960, Exynos, the Intel "lakes", etc. Ideally this needs validation with a high-speed connection to a USB 3.x host, which increases the chances that USB 2.0 Link Power Management is supported.
The overall goal of this patch is to eliminate events generated for L1 entry/exit, so we should see a slight reduction in interrupt counts when checking grep dwc3 /proc/interrupts for comparable traffic.
Appreciate any feedback and help in testing!
Thanks,
Jack
Quoted 46 lines
Signed-off-by Jack Pham <[email protected]>
---
 drivers/usb/dwc3/core.c | 9 +++++++++
 drivers/usb/dwc3/core.h | 5 +++--
 2 files changed, 12 insertions(+), 2 deletions(-)
drivers/usb/dwc3/core.c +9 -0
index ba74ad7f6995..719dac228703 100644
@@ -1050,6 +1050,15 @@ static int dwc3_core_init(struct dwc3 *dwc)
 		if (!DWC3_VER_IS_PRIOR(DWC3, 290A))
 			reg |= DWC3_GUCTL1_DEV_L1_EXIT_BY_HW;
 
+		/*
+		 * Decouple USB 2.0 L1 & L2 events which will allow for
+		 * gadget driver to only receive U3/L2 suspend & wakeup
+		 * events and prevent the more frequent L1 LPM transitions
+		 * from interrupting the driver.
+		 */
+		if (!DWC3_VER_IS_PRIOR(DWC3, 300A))
+			reg |= DWC3_GUCTL1_DEV_DECOUPLE_L1L2_EVT;
+
 		if (dwc->dis_tx_ipgap_linecheck_quirk)
 			reg |= DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS;
 
drivers/usb/dwc3/core.h +3 -2
index 5991766239ba..356b94a7ec70 100644
@@ -256,9 +256,10 @@
 #define DWC3_GUCTL_HSTINAUTORETRY	BIT(14)
 
 /* Global User Control 1 Register */
-#define DWC3_GUCTL1_PARKMODE_DISABLE_SS	BIT(17)
+#define DWC3_GUCTL1_DEV_DECOUPLE_L1L2_EVT	BIT(31)
 #define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS	BIT(28)
-#define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW	BIT(24)
+#define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW		BIT(24)
+#define DWC3_GUCTL1_PARKMODE_DISABLE_SS		BIT(17)
 
 /* Global Status Register */
 #define DWC3_GSTS_OTG_IP	BIT(10)
Signature · 1 line
2.24.0

Felipe Balbi

Acked-by on v1 1/1

2021-08-18 05:12 UTC #10 lore raw

In reply to Jack Pham

Quoted 15 lines · Jack Pham

On DWC_usb3 revisions 3.00a and newer (including DWC_usb31 and DWC_usb32) the GUCTL1 register gained the DEV_DECOUPLE_L1L2_EVT field (bit 31) which when enabled allows the controller in device mode to treat USB 2.0 L1 LPM & L2 events separately.
After commit d1d90dd27254 ("usb: dwc3: gadget: Enable suspend events") the controller will now receive events (and therefore interrupts) for every state change when entering/exiting either L1 or L2 states. Since L1 is handled entirely by the hardware and requires no software intervention, there is no need to even enable these events and unnecessarily notify the gadget driver. Enable the aforementioned bit to help reduce the overall interrupt count for these L1 events that don't need to be handled while retaining the events for full L2 suspend/wakeup.

Quoted 1 line

Signed-off-by Jack Pham <[email protected]>

Looks okay to me:

Acked-by Felipe Balbi <[email protected]>

Signature · 1 line

balbi