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

From: Artur Petrosyan <Arthur.Petrosyan@synopsys.com>
To: Doug Anderson <dianders@chromium.org>
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: Mon, 29 Apr 2019 12:01:16 +0000	[thread overview]
Message-ID: <SN1PR12MB24313212A9F1574A4D00C3B3A7390@SN1PR12MB2431.namprd12.prod.outlook.com> (raw)

Hi,

On 4/27/2019 01:01, Doug Anderson wrote:
> 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()?
For Hibernation I do 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 *".
This code comes form previous implementations I have not touched it not 
to back anything.

> 
> 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?
When there is an error power_saving should be skipped.

> 
> 
>> +               } 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...Enter hibernation implements setting bus_suspend so I don't touch this.
Actually this patch set fixes issues it doesn't clean up everything 
related to hibernation or partial power down.

> 
> 
>>          /* 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?
> 
That part is not dad because if hsotg->bus_suspended is true 
resuming/exiting from suspend/partial power down/hibernation should be 
performed.
On the other hand if hsotg->bus_suspended is false there is no need to 
resume.

So of course if core is not suspended the code responsible for resuming 
should not be called. In that sense the code can be called dead.

> 
>>                  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?
I have tested the patches on HAPS-DX and Linaro HiKey 960 boards.

> 
> 
> 
> 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://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=2xJPvFyQhKsjBCMYd6_Kpb7_r7HPUAoeJHcQmBH80Gc&s=wEgbdpKgC9t9CfXpI8awXLos9GuRcsNEkpW09dgMxiw&e=
> 
> 
> ...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.
You can provide debug logs for that scenario I will try to help you fix 
issues with that.

> 
> -Doug
>