linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] usb: dwc3: Decouple USB 2.0 L1 & L2 events
@ 2021-08-12  8:26 Jack Pham
  2021-08-18  1:28 ` [RFT][PATCH] " Jack Pham
  2021-08-18  5:08 ` [PATCH] " Felipe Balbi
  0 siblings, 2 replies; 11+ messages in thread
From: Jack Pham @ 2021-08-12  8:26 UTC (permalink / raw)
  To: Felipe Balbi, Greg Kroah-Hartman
  Cc: linux-usb, Wesley Cheng, Thinh Nguyen, 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.

Signed-off-by: Jack Pham <jackp@codeaurora.org>
---
 drivers/usb/dwc3/core.c | 9 +++++++++
 drivers/usb/dwc3/core.h | 5 +++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index ba74ad7f6995..719dac228703 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -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;
 
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 5991766239ba..356b94a7ec70 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -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)
-- 
2.24.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFT][PATCH] usb: dwc3: Decouple USB 2.0 L1 & L2 events
  2021-08-12  8:26 [PATCH] usb: dwc3: Decouple USB 2.0 L1 & L2 events Jack Pham
@ 2021-08-18  1:28 ` Jack Pham
  2021-08-18  2:07   ` John Stultz
                     ` (4 more replies)
  2021-08-18  5:08 ` [PATCH] " Felipe Balbi
  1 sibling, 5 replies; 11+ messages in thread
From: Jack Pham @ 2021-08-18  1:28 UTC (permalink / raw)
  To: Thinh Nguyen, John Stultz, Amit Pundir, Ray Chi, Ferry Toth,
	Chunfeng Yun, Andy Shevchenko, Marek Szyprowski, Li Jun
  Cc: Felipe Balbi, Greg Kroah-Hartman, linux-usb, Wesley Cheng

On Thu, Aug 12, 2021 at 01:26:35AM -0700, Jack Pham wrote:
> 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.

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

> Signed-off-by: Jack Pham <jackp@codeaurora.org>
> ---
>  drivers/usb/dwc3/core.c | 9 +++++++++
>  drivers/usb/dwc3/core.h | 5 +++--
>  2 files changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index ba74ad7f6995..719dac228703 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -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;
>  
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 5991766239ba..356b94a7ec70 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -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)
> -- 
> 2.24.0
> 

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFT][PATCH] usb: dwc3: Decouple USB 2.0 L1 & L2 events
  2021-08-18  1:28 ` [RFT][PATCH] " Jack Pham
@ 2021-08-18  2:07   ` John Stultz
  2021-08-18  7:52   ` Jun Li
                     ` (3 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: John Stultz @ 2021-08-18  2:07 UTC (permalink / raw)
  To: Jack Pham
  Cc: Thinh Nguyen, Amit Pundir, Ray Chi, Ferry Toth, Chunfeng Yun,
	Andy Shevchenko, Marek Szyprowski, Li Jun, Felipe Balbi,
	Greg Kroah-Hartman, Linux USB List, Wesley Cheng

On Tue, Aug 17, 2021 at 6:29 PM Jack Pham <jackp@codeaurora.org> wrote:
>
> On Thu, Aug 12, 2021 at 01:26:35AM -0700, Jack Pham wrote:
> > 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.
>
> 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.
>

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 <john.stultz@linaro.org> # for HiKey960 & db845c

thanks
-john

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH] usb: dwc3: Decouple USB 2.0 L1 & L2 events
  2021-08-12  8:26 [PATCH] usb: dwc3: Decouple USB 2.0 L1 & L2 events Jack Pham
  2021-08-18  1:28 ` [RFT][PATCH] " Jack Pham
@ 2021-08-18  5:08 ` Felipe Balbi
  1 sibling, 0 replies; 11+ messages in thread
From: Felipe Balbi @ 2021-08-18  5:08 UTC (permalink / raw)
  To: Jack Pham; +Cc: Greg Kroah-Hartman, linux-usb, Wesley Cheng, Thinh Nguyen


Jack Pham <jackp@codeaurora.org> writes:

> 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 <jackp@codeaurora.org>

Looks okay to me:

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

-- 
balbi

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [RFT][PATCH] usb: dwc3: Decouple USB 2.0 L1 & L2 events
  2021-08-18  1:28 ` [RFT][PATCH] " Jack Pham
  2021-08-18  2:07   ` John Stultz
@ 2021-08-18  7:52   ` Jun Li
  2021-08-18  9:09   ` Amit Pundir
                     ` (2 subsequent siblings)
  4 siblings, 0 replies; 11+ messages in thread
From: Jun Li @ 2021-08-18  7:52 UTC (permalink / raw)
  To: Jack Pham, Thinh Nguyen, John Stultz, Amit Pundir, Ray Chi,
	Ferry Toth, Chunfeng Yun, Andy Shevchenko, Marek Szyprowski
  Cc: Felipe Balbi, Greg Kroah-Hartman, linux-usb, Wesley Cheng



> -----Original Message-----
> From: jackp=codeaurora.org@mg.codeaurora.org
> <jackp=codeaurora.org@mg.codeaurora.org> On Behalf Of Jack Pham
> Sent: Wednesday, August 18, 2021 9:29 AM
> To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>; John Stultz
> <john.stultz@linaro.org>; Amit Pundir <amit.pundir@linaro.org>; Ray Chi
> <raychi@google.com>; Ferry Toth <ftoth@exalondelft.nl>; Chunfeng Yun
> <chunfeng.yun@mediatek.com>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Marek Szyprowski
> <m.szyprowski@samsung.com>; Jun Li <jun.li@nxp.com>
> Cc: Felipe Balbi <balbi@kernel.org>; Greg Kroah-Hartman
> <gregkh@linuxfoundation.org>; linux-usb@vger.kernel.org; Wesley Cheng
> <wcheng@codeaurora.org>
> Subject: Re: [RFT][PATCH] usb: dwc3: Decouple USB 2.0 L1 & L2 events
> 
> On Thu, Aug 12, 2021 at 01:26:35AM -0700, Jack Pham wrote:
> > 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.
> 
> 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!

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 <jun.li@nxp.com>
Reviewed-by: Jun Li <jun.li@nxp.com>

> Thanks,
> Jack
> 
> > Signed-off-by: Jack Pham <jackp@codeaurora.org>
> > ---
> >  drivers/usb/dwc3/core.c | 9 +++++++++  drivers/usb/dwc3/core.h | 5
> > +++--
> >  2 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index
> > ba74ad7f6995..719dac228703 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -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;
> >
> > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index
> > 5991766239ba..356b94a7ec70 100644
> > --- a/drivers/usb/dwc3/core.h
> > +++ b/drivers/usb/dwc3/core.h
> > @@ -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)
> > --
> > 2.24.0
> >
> 
> --
> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFT][PATCH] usb: dwc3: Decouple USB 2.0 L1 & L2 events
  2021-08-18  1:28 ` [RFT][PATCH] " Jack Pham
  2021-08-18  2:07   ` John Stultz
  2021-08-18  7:52   ` Jun Li
@ 2021-08-18  9:09   ` Amit Pundir
  2021-08-18  9:33   ` Andy Shevchenko
  2021-08-19  2:01   ` Thinh Nguyen
  4 siblings, 0 replies; 11+ messages in thread
From: Amit Pundir @ 2021-08-18  9:09 UTC (permalink / raw)
  To: Jack Pham
  Cc: Thinh Nguyen, John Stultz, Ray Chi, Ferry Toth, Chunfeng Yun,
	Andy Shevchenko, Marek Szyprowski, Li Jun, Felipe Balbi,
	Greg Kroah-Hartman, linux-usb, Wesley Cheng

On Wed, 18 Aug 2021 at 06:59, Jack Pham <jackp@codeaurora.org> wrote:
>
> On Thu, Aug 12, 2021 at 01:26:35AM -0700, Jack Pham wrote:
> > 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.
>
> 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!

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 <amit.pundir@linaro.org> # for RB5 (sm8250)

Regards,
Amit Pundir

> Thanks,
> Jack
>
> > Signed-off-by: Jack Pham <jackp@codeaurora.org>
> > ---
> >  drivers/usb/dwc3/core.c | 9 +++++++++
> >  drivers/usb/dwc3/core.h | 5 +++--
> >  2 files changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index ba74ad7f6995..719dac228703 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -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;
> >
> > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> > index 5991766239ba..356b94a7ec70 100644
> > --- a/drivers/usb/dwc3/core.h
> > +++ b/drivers/usb/dwc3/core.h
> > @@ -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)
> > --
> > 2.24.0
> >
>
> --
> The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFT][PATCH] usb: dwc3: Decouple USB 2.0 L1 & L2 events
  2021-08-18  1:28 ` [RFT][PATCH] " Jack Pham
                     ` (2 preceding siblings ...)
  2021-08-18  9:09   ` Amit Pundir
@ 2021-08-18  9:33   ` Andy Shevchenko
  2021-08-18 19:48     ` Ferry Toth
  2021-08-19  2:01   ` Thinh Nguyen
  4 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2021-08-18  9:33 UTC (permalink / raw)
  To: Jack Pham
  Cc: Thinh Nguyen, John Stultz, Amit Pundir, Ray Chi, Ferry Toth,
	Chunfeng Yun, Marek Szyprowski, Li Jun, Felipe Balbi,
	Greg Kroah-Hartman, linux-usb, Wesley Cheng

On Tue, Aug 17, 2021 at 06:28:59PM -0700, Jack Pham wrote:
> On Thu, Aug 12, 2021 at 01:26:35AM -0700, Jack Pham wrote:
> > 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.
> 
> 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.

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.

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFT][PATCH] usb: dwc3: Decouple USB 2.0 L1 & L2 events
  2021-08-18  9:33   ` Andy Shevchenko
@ 2021-08-18 19:48     ` Ferry Toth
  2021-08-19 12:26       ` Andy Shevchenko
  2021-08-20 12:17       ` Jack Pham
  0 siblings, 2 replies; 11+ messages in thread
From: Ferry Toth @ 2021-08-18 19:48 UTC (permalink / raw)
  To: Andy Shevchenko, Jack Pham
  Cc: Thinh Nguyen, John Stultz, Amit Pundir, Ray Chi, Ferry Toth,
	Chunfeng Yun, Marek Szyprowski, Li Jun, Felipe Balbi,
	Greg Kroah-Hartman, linux-usb, Wesley Cheng

Op 18-08-2021 om 11:33 schreef Andy Shevchenko:
> On Tue, Aug 17, 2021 at 06:28:59PM -0700, Jack Pham wrote:
>> On Thu, Aug 12, 2021 at 01:26:35AM -0700, Jack Pham wrote:
>>> 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.
>>
>> 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.

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

Should I retest with this reverted?

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

> 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 <fntoth@gmail.com> # for Merrifield

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFT][PATCH] usb: dwc3: Decouple USB 2.0 L1 & L2 events
  2021-08-18  1:28 ` [RFT][PATCH] " Jack Pham
                     ` (3 preceding siblings ...)
  2021-08-18  9:33   ` Andy Shevchenko
@ 2021-08-19  2:01   ` Thinh Nguyen
  4 siblings, 0 replies; 11+ messages in thread
From: Thinh Nguyen @ 2021-08-19  2:01 UTC (permalink / raw)
  To: Jack Pham, Thinh Nguyen, John Stultz, Amit Pundir, Ray Chi,
	Ferry Toth, Chunfeng Yun, Andy Shevchenko, Marek Szyprowski,
	Li Jun
  Cc: Felipe Balbi, Greg Kroah-Hartman, linux-usb, Wesley Cheng

Jack Pham wrote:
> On Thu, Aug 12, 2021 at 01:26:35AM -0700, Jack Pham wrote:
>> 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.
> 

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

> 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
> 
>> Signed-off-by: Jack Pham <jackp@codeaurora.org>
>> ---
>>  drivers/usb/dwc3/core.c | 9 +++++++++
>>  drivers/usb/dwc3/core.h | 5 +++--
>>  2 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index ba74ad7f6995..719dac228703 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -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;
>>  
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 5991766239ba..356b94a7ec70 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -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)
>> -- 
>> 2.24.0
>>
> 


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFT][PATCH] usb: dwc3: Decouple USB 2.0 L1 & L2 events
  2021-08-18 19:48     ` Ferry Toth
@ 2021-08-19 12:26       ` Andy Shevchenko
  2021-08-20 12:17       ` Jack Pham
  1 sibling, 0 replies; 11+ messages in thread
From: Andy Shevchenko @ 2021-08-19 12:26 UTC (permalink / raw)
  To: Ferry Toth
  Cc: Jack Pham, Thinh Nguyen, John Stultz, Amit Pundir, Ray Chi,
	Ferry Toth, Chunfeng Yun, Marek Szyprowski, Li Jun, Felipe Balbi,
	Greg Kroah-Hartman, linux-usb, Wesley Cheng

On Wed, Aug 18, 2021 at 09:48:18PM +0200, Ferry Toth wrote:
> Op 18-08-2021 om 11:33 schreef Andy Shevchenko:
> > On Tue, Aug 17, 2021 at 06:28:59PM -0700, Jack Pham wrote:
> > > On Thu, Aug 12, 2021 at 01:26:35AM -0700, Jack Pham wrote:

...

> > > 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
> 
> > 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 <fntoth@gmail.com> # for Merrifield

Thank you, Ferry!

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [RFT][PATCH] usb: dwc3: Decouple USB 2.0 L1 & L2 events
  2021-08-18 19:48     ` Ferry Toth
  2021-08-19 12:26       ` Andy Shevchenko
@ 2021-08-20 12:17       ` Jack Pham
  1 sibling, 0 replies; 11+ messages in thread
From: Jack Pham @ 2021-08-20 12:17 UTC (permalink / raw)
  To: Ferry Toth
  Cc: Andy Shevchenko, Thinh Nguyen, John Stultz, Amit Pundir, Ray Chi,
	Ferry Toth, Chunfeng Yun, Marek Szyprowski, Li Jun, Felipe Balbi,
	Greg Kroah-Hartman, linux-usb, Wesley Cheng

On Wed, Aug 18, 2021 at 09:48:18PM +0200, Ferry Toth wrote:
> Op 18-08-2021 om 11:33 schreef Andy Shevchenko:
> > On Tue, Aug 17, 2021 at 06:28:59PM -0700, Jack Pham wrote:
> > > On Thu, Aug 12, 2021 at 01:26:35AM -0700, Jack Pham wrote:
> > > > 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.
> > > 
> > > 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.
> 
> 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.

> > > 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
> 
> > 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 <fntoth@gmail.com> # for Merrifield

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

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2021-08-20 12:18 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-12  8:26 [PATCH] usb: dwc3: Decouple USB 2.0 L1 & L2 events Jack Pham
2021-08-18  1:28 ` [RFT][PATCH] " Jack Pham
2021-08-18  2:07   ` John Stultz
2021-08-18  7:52   ` Jun Li
2021-08-18  9:09   ` Amit Pundir
2021-08-18  9:33   ` Andy Shevchenko
2021-08-18 19:48     ` Ferry Toth
2021-08-19 12:26       ` Andy Shevchenko
2021-08-20 12:17       ` Jack Pham
2021-08-19  2:01   ` Thinh Nguyen
2021-08-18  5:08 ` [PATCH] " Felipe Balbi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).