[v1,14/14] usb: dwc2: Add enter/exit hibernation from system issued suspend/resume

From: Doug Anderson <dianders@chromium.org>
To: Artur Petrosyan <Arthur.Petrosyan@synopsys.com>
Cc: Felipe Balbi <balbi@kernel.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Minas Harutyunyan <Minas.Harutyunyan@synopsys.com>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
	John Youn <John.Youn@synopsys.com>
Subject: [v1,14/14] usb: dwc2: Add enter/exit hibernation from system issued suspend/resume
Date: Fri, 26 Apr 2019 14:00:18 -0700	[thread overview]
Message-ID: <CAD=FV=VDVhA0qzBN13=3C44mAzhDaQBUia_QADqyggSSFwKXqQ@mail.gmail.com> (raw)

Hi,

On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan
<Arthur.Petrosyan@synopsys.com> wrote:
>
> Added a new flow of entering and exiting hibernation when PC is
> hibernated or suspended.
>
> Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
> ---
>  drivers/usb/dwc2/hcd.c | 128 +++++++++++++++++++++++++++++++------------------
>  1 file changed, 81 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 45d4a3e1ebd2..f1e92a287cb1 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -4510,35 +4510,54 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>         if (hsotg->op_state == OTG_STATE_B_PERIPHERAL)
>                 goto unlock;
>
> -       if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL ||
> +       if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_NONE ||
>             hsotg->flags.b.port_connect_status == 0)
>                 goto skip_power_saving;
>
> -       /*
> -        * Drive USB suspend and disable port Power
> -        * if usb bus is not suspended.
> -        */
> -       if (!hsotg->bus_suspended) {
> -               hprt0 = dwc2_read_hprt0(hsotg);
> -               hprt0 |= HPRT0_SUSP;
> -               hprt0 &= ~HPRT0_PWR;
> -               dwc2_writel(hsotg, hprt0, HPRT0);
> -               spin_unlock_irqrestore(&hsotg->lock, flags);
> -               dwc2_vbus_supply_exit(hsotg);
> -               spin_lock_irqsave(&hsotg->lock, flags);
> -       }
> +       switch (hsotg->params.power_down) {
> +       case DWC2_POWER_DOWN_PARAM_PARTIAL:
> +               /*
> +                * Drive USB suspend and disable port Power
> +                * if usb bus is not suspended.
> +                */
> +               if (!hsotg->bus_suspended) {
> +                       hprt0 = dwc2_read_hprt0(hsotg);
> +                       hprt0 |= HPRT0_SUSP;
> +                       hprt0 &= ~HPRT0_PWR;
> +                       dwc2_writel(hsotg, hprt0, HPRT0);
> +                       spin_unlock_irqrestore(&hsotg->lock, flags);
> +                       dwc2_vbus_supply_exit(hsotg);
> +                       spin_lock_irqsave(&hsotg->lock, flags);
> +               }
>
> -       /* Enter partial_power_down */
> -       ret = dwc2_enter_partial_power_down(hsotg);
> -       if (ret) {
> -               if (ret != -ENOTSUPP)
> -                       dev_err(hsotg->dev,
> -                               "enter partial_power_down failed\n");
> +               /* Enter partial_power_down */
> +               ret = dwc2_enter_partial_power_down(hsotg);
> +               if (ret) {
> +                       if (ret != -ENOTSUPP)
> +                               dev_err(hsotg->dev,
> +                                       "enter partial_power_down failed\n");
> +                       goto skip_power_saving;
> +               }
> +               hsotg->bus_suspended = true;
> +               break;
> +       case DWC2_POWER_DOWN_PARAM_HIBERNATION:
> +               if (!hsotg->bus_suspended) {

Do you have any idea why for DWC2_POWER_DOWN_PARAM_PARTIAL we still
call dwc2_enter_partial_power_down() even if bus_suspended is true,
but for hibernate you don't call dwc2_enter_hibernation()?

> +                       /* Enter hibernation */
> +                       spin_unlock_irqrestore(&hsotg->lock, flags);
> +                       ret = dwc2_enter_hibernation(hsotg, 1);
> +                       spin_lock_irqsave(&hsotg->lock, flags);
> +                       if (ret && ret != -ENOTSUPP)
> +                               dev_err(hsotg->dev,
> +                                       "%s: enter hibernation failed\n",
> +                                       __func__);

nit: no __func__ in dev_xxx() error messages.  The device plus the
message should be enough.  Only resort to __func__ if you're forced to
do an error message without a "struct device *".

nit: as per my comments in an earlier patch, remove special case for -ENOTSUPP

Also, presumably you want to match the error handling in
DWC2_POWER_DOWN_PARAM_PARTIAL and do a "goto skip_power_saving" when
you see an error?

> +               } else {
> +                       goto skip_power_saving;
> +               }
> +               break;
> +       default:
>                 goto skip_power_saving;
>         }
>
> -       hsotg->bus_suspended = true;
> -

It's a bit weird to remove this, but I guess it just got moved to the
partial power down case?  ...and in the hibernate case you're relying
on the hibernate function to set this?  Yet another frustratingly
asymmetric code structure...

>         /* Ask phy to be suspended */
>         if (!IS_ERR_OR_NULL(hsotg->uphy)) {
>                 spin_unlock_irqrestore(&hsotg->lock, flags);
> @@ -4564,17 +4583,17 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>         int ret = 0;
>         u32 hprt0;
>
> -       hprt0 = dwc2_read_hprt0(hsotg);
> -
>         spin_lock_irqsave(&hsotg->lock, flags);
>
> -       if (dwc2_is_device_mode(hsotg))
> +       if (!hsotg->bus_suspended)

As per my comments above I don't have a good grasp on what
"bus_suspended" is for.  ...that being said, if your change here is
actually correct then you probably (?) want to remove the "if
(hsotg->bus_suspended)" check later in this same function.

Said another way, you've now got code that looks like:

if (!hsotg->bus_suspended)
  goto exit;

/* code that I think doesn't touch bus_suspended */

if (hsotg->bus_suspended) {
  /* do something */
} else {
  /* do something else */
}

Presumably the "do something" is now dead code?

>                 goto unlock;
>
>         if (hsotg->lx_state != DWC2_L2)
>                 goto unlock;
>
> -       if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL ||
> +       hprt0 = dwc2_read_hprt0(hsotg);
> +
> +       if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_NONE ||
>             hprt0 & HPRT0_CONNSTS) {
>                 hsotg->lx_state = DWC2_L0;
>                 goto unlock;
> @@ -4597,36 +4616,51 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
>                 spin_lock_irqsave(&hsotg->lock, flags);
>         }
>
> -       /* Exit partial_power_down */
> -       ret = dwc2_exit_partial_power_down(hsotg, true);
> -       if (ret && (ret != -ENOTSUPP))
> -               dev_err(hsotg->dev, "exit partial_power_down failed\n");
> +       switch (hsotg->params.power_down) {
> +       case DWC2_POWER_DOWN_PARAM_PARTIAL:
>
> -       hsotg->lx_state = DWC2_L0;
> +               /* Exit partial_power_down */
> +               ret = dwc2_exit_partial_power_down(hsotg, true);
> +               if (ret && (ret != -ENOTSUPP))
> +                       dev_err(hsotg->dev, "exit partial_power_down failed\n");
>
> -       spin_unlock_irqrestore(&hsotg->lock, flags);
> +               hsotg->lx_state = DWC2_L0;
>
> -       if (hsotg->bus_suspended) {
> -               spin_lock_irqsave(&hsotg->lock, flags);
> -               hsotg->flags.b.port_suspend_change = 1;
>                 spin_unlock_irqrestore(&hsotg->lock, flags);
> -               dwc2_port_resume(hsotg);
> -       } else {
> -               dwc2_vbus_supply_init(hsotg);
>
> -               /* Wait for controller to correctly update D+/D- level */
> -               usleep_range(3000, 5000);
> +               if (hsotg->bus_suspended) {
> +                       spin_lock_irqsave(&hsotg->lock, flags);
> +                       hsotg->flags.b.port_suspend_change = 1;
> +                       spin_unlock_irqrestore(&hsotg->lock, flags);
> +                       dwc2_port_resume(hsotg);
> +               } else {
> +                       dwc2_vbus_supply_init(hsotg);
>
> -               /*
> -                * Clear Port Enable and Port Status changes.
> -                * Enable Port Power.
> -                */
> -               dwc2_writel(hsotg, HPRT0_PWR | HPRT0_CONNDET |
> +                       /* Wait for controller to correctly update D+/D- level */
> +                       usleep_range(3000, 5000);
> +
> +                       /*
> +                        * Clear Port Enable and Port Status changes.
> +                        * Enable Port Power.
> +                        */
> +                       dwc2_writel(hsotg, HPRT0_PWR | HPRT0_CONNDET |
>                                 HPRT0_ENACHG, HPRT0);
> -               /* Wait for controller to detect Port Connect */
> -               usleep_range(5000, 7000);
> +                       /* Wait for controller to detect Port Connect */
> +                       usleep_range(5000, 7000);
> +               }
> +               break;
> +       case DWC2_POWER_DOWN_PARAM_HIBERNATION:
> +
> +               /* Exit host hibernation. */
> +               if (hsotg->hibernated)
> +                       dwc2_exit_hibernation(hsotg, 0, 0, 1);
> +               break;
> +       default:
> +               goto unlock;
>         }
>
> +       spin_unlock_irqrestore(&hsotg->lock, flags);
> +

I'm pretty curious if you tested DWC2_POWER_DOWN_PARAM_PARTIAL after
applying your patch series.  As far as I can tell your switch
statement for DWC2_POWER_DOWN_PARAM_PARTIAL will "break" with the
spinlock already unlocked.  ...so you'll run spin_unlock_irqrestore
twice.  Is that really legit?

NOTE: in case it helps you, I've attempted to rebase this atop my
series.  Please double-check that I didn't mess anything up, though.

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

...with that a quick test seems to show that partial power down is
sorta working on rk3288 now.  I _think_ I saw one case where hotplug
failed but I've seen several where it works.  ...unfortunately it
seems to break when I do hotplug on the port where I have
"snps,reset-phy-on-wake" set.

-Doug