[v1,04/14] usb: dwc2: Fix suspend state in host mode for partial power down.

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,04/14] usb: dwc2: Fix suspend state in host mode for partial power down.
Date: Mon, 29 Apr 2019 11:03:16 +0000	[thread overview]
Message-ID: <SN1PR12MB24316A556FF0EC6E7899A9F0A7390@SN1PR12MB2431.namprd12.prod.outlook.com> (raw)

Hi,

On 4/27/2019 00:45, Doug Anderson wrote:
> Hi,
> 
> On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan
> <Arthur.Petrosyan@synopsys.com> wrote:
>>
>> - In dwc2_port_suspend() function added waiting for the
>>    HPRT0.PrtSusp register field to be set.
>>
>> - In _dwc2_hcd_suspend() function added checking of
>>    "hsotg->flags.b.port_connect_status" port connection
>>    status if port connection status is 0 then skipping
>>    power saving (entering partial power down mode).
>>    Because if there is no device connected there would
>>    be no need to enter partial power down mode.
>>
>> - Added "hsotg->bus_suspended = true" beceuse after
> 
> s/beceuse/because
> 
>>    entering partial power down in host mode the
>>    bus_suspended flag must be set.
> 
> The above statement sounds to me like trying to win an argument by
> saying "I'm right because I'm right."  Can you give more details about
> why "bus_suspended" must be set?  See below also.
> 
There is no intention of winning any argument.
Are you familiar with "bus_suspended" flag ? have you looked at what is 
it used for ?

  * @bus_suspended:	True if bus is suspended

So when entering to hibernation is performed bus is suspended. To 
indicate that "hsotg->bus_suspended" is assigned to true.

> 
>> Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
>> ---
>>   drivers/usb/dwc2/hcd.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index dd82fa516f3f..1d18258b5ff8 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -3479,6 +3479,10 @@ static void dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex)
>>          hprt0 |= HPRT0_SUSP;
>>          dwc2_writel(hsotg, hprt0, HPRT0);
>>
>> +       /* Wait for the HPRT0.PrtSusp register field to be set */
>> +       if (dwc2_hsotg_wait_bit_set(hsotg, HPRT0, HPRT0_SUSP, 3000))
>> +               dev_warn(hsotg->dev, "Suspend wasn't generated\n");
>> +
>>          hsotg->bus_suspended = true;
>>
>>          /*
>> @@ -4488,7 +4492,8 @@ 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_PARTIAL ||
>> +           hsotg->flags.b.port_connect_status == 0)
>>                  goto skip_power_saving;
>>
>>          /*
>> @@ -4514,6 +4519,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
>>                  goto skip_power_saving;
>>          }
>>
>> +       hsotg->bus_suspended = true;
>> +
> 
> I'm a bit unsure about this.  Specifically I note that
> _dwc2_hcd_resume() has a special case "if (hsotg->bus_suspended)".
> Does that now become dead code?
No it doesn't became a dead code.
> 
> ...I talk about this a bit more in my review of ("usb: dwc2: Add
> enter/exit hibernation from system issued suspend/resume")
> 
> 
> -Doug
>