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