Hi,
On 4/27/2019 00:45, Doug Anderson wrote:
> Hi,
>
> On Fri, Apr 19, 2019 at 1:05 PM Artur Petrosyan
> <Arthur.Petrosyan@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>> ---
>> 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
>
--
Regards,
Artur
