[v1,09/14] usb: dwc2: Update dwc2_handle_usb_suspend_intr function.

Commit Message

Artur Petrosyan April 19, 2019, 11:24 a.m. UTC

To avoid working in two modes (partial power down
and hibernation) changed conditions for entering
partial power down or hibernation.

Instead of checking hw_params.power_optimized and
hw_params.hibernation now checking power_down
param which already set to one of the options
(Hibernation or Partial Power Down) based on
OTG_EN_PWROPT.

Signed-off-by: Artur Petrosyan <[e-mail]>
Signed-off-by: Minas Harutyunyan <[e-mail]>
---
 drivers/usb/dwc2/core_intr.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Doug Anderson April 26, 2019, 8:51 p.m. UTC | #1

Hi,

On Fri, Apr 19, 2019 at 11:53 AM Artur Petrosyan
<[e-mail]> wrote:
>
> To avoid working in two modes (partial power down
> and hibernation) changed conditions for entering
> partial power down or hibernation.
>
> Instead of checking hw_params.power_optimized and
> hw_params.hibernation now checking power_down
> param which already set to one of the options
> (Hibernation or Partial Power Down) based on
> OTG_EN_PWROPT.
>
> Signed-off-by: Artur Petrosyan <[e-mail]>
> Signed-off-by: Minas Harutyunyan <[e-mail]>
> ---
>  drivers/usb/dwc2/core_intr.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)

In general I'm in support of this patch--it's cleaner and gets rid of
a needless goto \o/

...but you don't go far enough.  You can fully get rid of all of the
"-ENOTSUPP" stuff.  I've actually picked my patches and yours atop
Felipe's "testing/next" tree and you can find it here:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+log/refs/sandbox/dianders/190426-dwc2-stuff

...as part of that I've included a patch ("usb: dwc2: Get rid of
useless error checks for hibernation/partial power down"), AKA:

https://chromium.googlesource.com/chromiumos/third_party/kernel/+/0c924f736e2f7c1bb02531aa33c04a3ae5f4fc4c

Feel free to squash that into your patch or add it to your series if
you like it.  Note that patch points out that there's are still some
instances where calling dwc2_exit_partial_power_down() might still
happen in a case where it's not obvious if we were in partial power
down mode and made me wonder if there might be some bugs there.

-Doug

Artur Petrosyan April 29, 2019, 11:35 a.m. UTC | #2

On 4/27/2019 00:52, Doug Anderson wrote:
> Hi,
> 
> On Fri, Apr 19, 2019 at 11:53 AM Artur Petrosyan
> <[e-mail]> wrote:
>>
>> To avoid working in two modes (partial power down
>> and hibernation) changed conditions for entering
>> partial power down or hibernation.
>>
>> Instead of checking hw_params.power_optimized and
>> hw_params.hibernation now checking power_down
>> param which already set to one of the options
>> (Hibernation or Partial Power Down) based on
>> OTG_EN_PWROPT.
>>
>> Signed-off-by: Artur Petrosyan <[e-mail]>
>> Signed-off-by: Minas Harutyunyan <[e-mail]>
>> ---
>>   drivers/usb/dwc2/core_intr.c | 13 +++++++------
>>   1 file changed, 7 insertions(+), 6 deletions(-)
> 
> In general I'm in support of this patch--it's cleaner and gets rid of
> a needless goto \o/
> 
> ...but you don't go far enough.  You can fully get rid of all of the
> "-ENOTSUPP" stuff.  I've actually picked my patches and yours atop
> Felipe's "testing/next" tree and you can find it here:
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__chromium.googlesource.com_chromiumos_third-5Fparty_kernel_-2Blog_refs_sandbox_dianders_190426-2Ddwc2-2Dstuff&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=WBAPjgkB_xB8UlcsYvQdxyxg2a3wC70A-jrd4IucYKw&s=6HkWJc-CszWClXSA1Ja9AupZVe7Qb4VTMofH8yTmj0o&e=
> 
> ...as part of that I've included a patch ("usb: dwc2: Get rid of
> useless error checks for hibernation/partial power down"), AKA:
Have you tested the patch? on which platform?
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__chromium.googlesource.com_chromiumos_third-5Fparty_kernel_-2B_0c924f736e2f7c1bb02531aa33c04a3ae5f4fc4c&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=WBAPjgkB_xB8UlcsYvQdxyxg2a3wC70A-jrd4IucYKw&s=pm6jeDE--3WsqgUui0ZU15vcvHZRQ05jA8mvP1LohS0&e=
> 
> Feel free to squash that into your patch or add it to your series if
> you like it.  Note that patch points out that there's are still some
> instances where calling dwc2_exit_partial_power_down() might still
> happen in a case where it's not obvious if we were in partial power
> down mode and made me wonder if there might be some bugs there.
I will test it too. In case it is fully ok and has no issues. I will let 
you know.

> 
> -Doug
>

Patch

diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index 06f8022b1bdb..fc465707b7a1 100644
--- a/drivers/usb/dwc2/core_intr.c
+++ b/drivers/usb/dwc2/core_intr.c
@@ -517,14 +517,15 @@  static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
 			return;
 		}
 		if (dsts & DSTS_SUSPSTS) {
-			if (hsotg->hw_params.power_optimized) {
+			switch (hsotg->params.power_down) {
+			case DWC2_POWER_DOWN_PARAM_PARTIAL:
 				ret = dwc2_enter_partial_power_down(hsotg);
 				if (ret) {
 					if (ret != -ENOTSUPP)
 						dev_err(hsotg->dev,
 							"%s: enter partial_power_down failed\n",
 							__func__);
-					goto skip_power_saving;
+					break;
 				}
 
 				udelay(100);
@@ -532,16 +533,16 @@  static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
 				/* Ask phy to be suspended */
 				if (!IS_ERR_OR_NULL(hsotg->uphy))
 					usb_phy_set_suspend(hsotg->uphy, true);
-			}
-
-			if (hsotg->hw_params.hibernation) {
+				break;
+			case DWC2_POWER_DOWN_PARAM_HIBERNATION:
 				ret = dwc2_enter_hibernation(hsotg, 0);
 				if (ret && ret != -ENOTSUPP)
 					dev_err(hsotg->dev,
 						"%s: enter hibernation failed\n",
 						__func__);
+				break;
 			}
-skip_power_saving:
+
 			/*
 			 * Change to L2 (suspend) state before releasing
 			 * spinlock