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?
>
>
>>> 3. I see that dctl gets "DCTL_PWRONPRGDONE" set as part of the
>>> sequence in the "not remote wakeup" case before calling this function.
>>> ...but then part of the function (that you didn't change) clobbers it,
>>> I think.
>>>
>> Setting device programming done bit in dwc2_gadget_exit_hibernation()
>> comes from older code and from debugging I noticed that if it is not
>> done at that point then the flow brakes.
>>
>> So in partial power down flow we need to set that bit wile restoring
>> device registers. That is why the implementation is such.
>>
>>>
>>> I have no idea if any of that is a problem but the fact that the
>>> hibernate and partial power down code runs through such different
>>> paths makes it really hard to know / reason about. Many of those
>>> differences exist before your patch, but you're adding a new
>>> difference rather than trying to unify and that worries me.
>>>
>>>
>>
>> So to make it easy to reason about I think we should debug it. Please
>> point out where it fails. Have you tested this flow and did you see any
>> wrong behavior of hibernation or partial power down? if yes please
>> provide the debug logs so that they can be investigated.
>>
>> All of these patches have been tested on HAPS-DX and and Linaro HiKey
>> 960 board. These patches fix Hibernation and Partial Power down issues.
>
> I have no real way to test this code. I'm only setup to use dwc2 as a
> USB host since my target device is a laptop with type A ports on it.
> Although one of the ports could be made a gadget and I could force the
> mode and use an A-to-A cable, I don't have any use cases here nor do I
> really have any experience using dwc2 as a gadget.
>
> ...so if you and others are happy with the code I won't stand in the
> way--I was just reviewing the rest of the series so I figured I'd do a
> cursory pass on this patch too.
>
>
> -Doug
>
--
Regards,
Artur
