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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 5/1/2019 05:55, Doug Anderson wrote:
> Hi,
> 
> On Tue, Apr 30, 2019 at 12:11 AM Artur Petrosyan
> <Arthur.Petrosyan@xxxxxxxxxxxx> wrote:
>>
>> Hi,
>>
>> On 4/29/2019 21:35, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Mon, Apr 29, 2019 at 4:03 AM Artur Petrosyan
>>> <Arthur.Petrosyan@xxxxxxxxxxxx> wrote:
>>>>
>>>> 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.
>>>
>>> I was trying to say that your statement does not convey any
>>> information about the "why".  It just says: "I now set this variable
>>> because it needs to be set".  This tells me nothing useful because
>>> presumably if it did't need to be set then you wouldn't have set it.
>>> I want to know more information about how the code was broken before
>>> you did this.  What specifically requires this variable to be set?
>> Specifically that variable is set when core enters to hibernation or
>> partial power down.
>>>
>>>
>>>> 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.
>>>
>>> Perhaps you can help me understand what the difference is between
>>> "port suspend" and "bus suspend" on dwc2.  I think this is where my
>>> confusion lies since there are functions for both and they do subtly
>>> different things.  ...but both functions set bus_suspended.
>> dwc2_port_suspend() is a function which is called when set port feature
>> "USB_PORT_FEAT_SUSPEND" is asserted. Yet, bus_suspended is a variable.
>> That variable should be set any time that host enters to hibernation or
>> partial power down.
>>
>>>
>>>
>>>>>> @@ -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.
>>>
>>> Can you explain when it gets triggered, then?
>> _dwc2_hcd_resume() is triggered by the system.
>> As an example lets assume you have hibernated the PC and then you turn
>> the PC on. When you turn the PC back on in that case _dwc2_hcd_resume()
>> function is called to resume from suspended state (Hibernation/partial
>> power down)
> 
> I think you are still not understanding my question here.  Please
> re-read it again.
Your question is "Can you explain when it gets triggered, then?" so I 
have explained one case when it is triggered.
> 
> 
> -Doug
> 


-- 
Regards,
Artur




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux