On 4/30/2019 19:29, Doug Anderson wrote:
> Hi,
>
> On Mon, Apr 29, 2019 at 11:59 PM Artur Petrosyan
> <Arthur.Petrosyan@xxxxxxxxxxxx> wrote:
>>
>> On 4/29/2019 21:34, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Mon, Apr 29, 2019 at 3:51 AM Artur Petrosyan
>>> <Arthur.Petrosyan@xxxxxxxxxxxx> wrote:
>>>>
>>>> On 4/27/2019 00:43, Doug Anderson wrote:
>>>>> Hi,
>>>>>
>>>>> On Fri, Apr 12, 2019 at 6:38 AM Artur Petrosyan
>>>>> <arthur.petrosyan@xxxxxxxxxxxx> wrote:
>>>>>>
>>>>>> - Added backup of DCFG register.
>>>>>> - Added Set the Power-On Programming done bit.
>>>>>>
>>>>>> Signed-off-by: Artur Petrosyan <arturp@xxxxxxxxxxxx>
>>>>>> ---
>>>>>> drivers/usb/dwc2/gadget.c | 10 ++++++++++
>>>>>> 1 file changed, 10 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
>>>>>> index 6812a8a3a98b..dcb0fbb8bc42 100644
>>>>>> --- a/drivers/usb/dwc2/gadget.c
>>>>>> +++ b/drivers/usb/dwc2/gadget.c
>>>>>> @@ -5004,6 +5004,7 @@ int dwc2_restore_device_registers(struct dwc2_hsotg *hsotg, int remote_wakeup)
>>>>>> {
>>>>>> struct dwc2_dregs_backup *dr;
>>>>>> int i;
>>>>>> + u32 dctl;
>>>>>>
>>>>>> dev_dbg(hsotg->dev, "%s\n", __func__);
>>>>>>
>>>>>> @@ -5019,6 +5020,15 @@ int dwc2_restore_device_registers(struct dwc2_hsotg *hsotg, int remote_wakeup)
>>>>>> if (!remote_wakeup)
>>>>>> dwc2_writel(hsotg, dr->dctl, DCTL);
>>>>>>
>>>>>> + if (hsotg->params.power_down == DWC2_POWER_DOWN_PARAM_PARTIAL) {
>>>>>> + dwc2_writel(hsotg, dr->dcfg, DCFG);
>>>>>> +
>>>>>> + /* Set the Power-On Programming done bit */
>>>>>> + dctl = dwc2_readl(hsotg, DCTL);
>>>>>> + dctl |= DCTL_PWRONPRGDONE;
>>>>>> + dwc2_writel(hsotg, dctl, DCTL);
>>>>>> + }
>>>>>
>>>>> I can't vouch one way or the other for the correctness of this change,
>>>>> but I will say that it's frustrating how asymmetric hibernate and
>>>>> partial power down are. It makes things really hard to reason about.
>>>>> Is there any way you could restructure this so it happens in the same
>>>>> way between hibernate and partial power down?
>>>>>
>>>>
>>>>> Specifically looking at the similar sequence in
>>>>> dwc2_gadget_exit_hibernation() (which calls this function), I see:
>>>>>
>>>>> 1. There are some extra delays. Are those needed for partial power down?
>>>> Do you mean delays in dwc2_gadget_exit_hibernation() ? If yes they are
>>>> needed for hibernation flow. What relates to delays in hibernation
>>>> needed for partial power down. Anything that is implemented in the
>>>> hibernation delays or other things are part of hibernation and are not
>>>> used in partial power down because they are two different flows of power
>>>> saving.
>>>
>>> OK, if they aren't needed for partial power down then that's fine.
>>> When I see two functions doing nearly the same sets of writes but one
>>> has delays and the other doesn't it makes me wonder if that was on
>>> purpose or not.
>>>
>>>
>>>>> 2. For exiting hibernation the setting of "DCTL_PWRONPRGDONE" only
>>>>> happens if "not remote wakeup". Is it truly on purpose that you don't
>>>>> do that?
>>>> Currently partial power down doesn't support remote wakeup flow.
>>>
>>> Oh. What happens if you have partial power down enabled and try to
>>> enable remote wakeup? Does it give an error?
>> As far as I have been debugging I have not seen error in that case.
>>
>> Do you mean like it should print a message saying that current partial
>> power down code doesn't support remote wakeup?
>
> Not sure. ...why don't we just forget about this question? I don't
> have enough gadget knowledge nor any way to test.
Ok let's forget about that.
>
> -Doug
>
--
Regards,
Artur
