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.
>
> 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.
>
> 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.
>
> -Doug
>
--
Regards,
Artur
