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.
-Doug
