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?
> > 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
