[PATCH 00/14] usb: dwc2: Fix and improve power saving modes.

[PATCH 00/14] usb: dwc2: Fix and improve power saving modes.

[Artur Petrosyan]

This patch set, fixes and improves partial power down and hibernation power
saving modes. Also, adds support for entering/exiting hibernation from
system issued suspend/resume.


Artur Petrosyan (14):
  usb: dwc2: Fix dwc2_restore_device_registers() function.
  usb: dwc2: Add descriptive debug messages for Partial Power Down mode.
  usb: dwc2: Fix wakeup detected and session request interrupt handlers.
  usb: dwc2: Fix suspend state in host mode for partial power down.
  usb: dwc2: Add port conn. sts. checking in _dwc2_hcd_resume()
    function.
  usb: dwc2: Add part. power down exit from
    dwc2_conn_id_status_change().
  usb: dwc2: Reset DEVADDR after exiting gadget hibernation.
  usb: dwc2: Add default param to control power optimization.
  usb: dwc2: Update dwc2_handle_usb_suspend_intr function.
  usb: dwc2: Fix hibernation between host and device modes.
  usb: dwc2: Allow exiting hibernation from gpwrdn rst detect
  usb: dwc2: Clear fifo_map when resetting core.
  usb: dwc2: Clear GINTSTS_RESTOREDONE bit after restore is generated.
  usb: dwc2: Add enter/exit hibernation from system issued
    suspend/resume

 drivers/usb/dwc2/core.c      |  20 +++++
 drivers/usb/dwc2/core.h      |   3 +
 drivers/usb/dwc2/core_intr.c | 178 ++++++++++++++++++++++++++-----------------
 drivers/usb/dwc2/debugfs.c   |   2 +
 drivers/usb/dwc2/gadget.c    |  16 +++-
 drivers/usb/dwc2/hcd.c       | 151 +++++++++++++++++++++++++-----------
 drivers/usb/dwc2/params.c    |  19 +++--
 7 files changed, 268 insertions(+), 121 deletions(-)

-- 
2.11.0

[01/14] usb: dwc2: Fix dwc2_restore_device_registers() function.

[Artur Petrosyan]

- Added backup of DCFG register.
- Added Set the Power-On Programming done bit.

Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
---
 drivers/usb/dwc2/gadget.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

-- 
2.11.0

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);
+	}
+
 	dwc2_writel(hsotg, dr->daintmsk, DAINTMSK);
 	dwc2_writel(hsotg, dr->diepmsk, DIEPMSK);
 	dwc2_writel(hsotg, dr->doepmsk, DOEPMSK);

[PATCH 01/14] usb: dwc2: Fix dwc2_restore_device_registers() function.

[Artur Petrosyan]

- Added backup of DCFG register.
- Added Set the Power-On Programming done bit.

Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
---
 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);
+	}
+
 	dwc2_writel(hsotg, dr->daintmsk, DAINTMSK);
 	dwc2_writel(hsotg, dr->diepmsk, DIEPMSK);
 	dwc2_writel(hsotg, dr->doepmsk, DOEPMSK);
-- 
2.11.0

[01/14] usb: dwc2: Fix dwc2_restore_device_registers() function.

[Felipe Balbi]

Artur Petrosyan <arthur.petrosyan@synopsys.com> writes:

> - Added backup of DCFG register.
> - Added Set the Power-On Programming done bit.
>
> Signed-off-by: Artur Petrosyan <arturp@synopsys.com>

won't apply.

Re: [PATCH 01/14] usb: dwc2: Fix dwc2_restore_device_registers() function.

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 236 bytes --]

Artur Petrosyan <arthur.petrosyan@synopsys.com> writes:

> - Added backup of DCFG register.
> - Added Set the Power-On Programming done bit.
>
> Signed-off-by: Artur Petrosyan <arturp@synopsys.com>

won't apply.


-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[01/14] usb: dwc2: Fix dwc2_restore_device_registers() function.

[Doug Anderson]

Hi,

On Fri, Apr 12, 2019 at 6:38 AM Artur Petrosyan
<arthur.petrosyan@synopsys.com> wrote:
>
> - Added backup of DCFG register.
> - Added Set the Power-On Programming done bit.
>
> Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
> ---
>  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

Re: [PATCH 01/14] usb: dwc2: Fix dwc2_restore_device_registers() function.

[Doug Anderson]

Hi,

On Fri, Apr 12, 2019 at 6:38 AM Artur Petrosyan
<arthur.petrosyan@synopsys.com> wrote:
>
> - Added backup of DCFG register.
> - Added Set the Power-On Programming done bit.
>
> Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
> ---
>  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

[01/14] usb: dwc2: Fix dwc2_restore_device_registers() function.

[Artur Petrosyan]

On 4/27/2019 00:43, Doug Anderson wrote:
> Hi,
> 
> On Fri, Apr 12, 2019 at 6:38 AM Artur Petrosyan
> <arthur.petrosyan@synopsys.com> wrote:
>>
>> - Added backup of DCFG register.
>> - Added Set the Power-On Programming done bit.
>>
>> Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
>> ---
>>   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
>

Re: [PATCH 01/14] usb: dwc2: Fix dwc2_restore_device_registers() function.

[Artur Petrosyan]

On 4/27/2019 00:43, Doug Anderson wrote:
> Hi,
> 
> On Fri, Apr 12, 2019 at 6:38 AM Artur Petrosyan
> <arthur.petrosyan@synopsys.com> wrote:
>>
>> - Added backup of DCFG register.
>> - Added Set the Power-On Programming done bit.
>>
>> Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
>> ---
>>   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

[01/14] usb: dwc2: Fix dwc2_restore_device_registers() function.

[Doug Anderson]

Hi,

On Mon, Apr 29, 2019 at 3:51 AM Artur Petrosyan
<Arthur.Petrosyan@synopsys.com> wrote:
>
> On 4/27/2019 00:43, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Apr 12, 2019 at 6:38 AM Artur Petrosyan
> > <arthur.petrosyan@synopsys.com> wrote:
> >>
> >> - Added backup of DCFG register.
> >> - Added Set the Power-On Programming done bit.
> >>
> >> Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
> >> ---
> >>   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

Re: [PATCH 01/14] usb: dwc2: Fix dwc2_restore_device_registers() function.

[Doug Anderson]

Hi,

On Mon, Apr 29, 2019 at 3:51 AM Artur Petrosyan
<Arthur.Petrosyan@synopsys.com> wrote:
>
> On 4/27/2019 00:43, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Apr 12, 2019 at 6:38 AM Artur Petrosyan
> > <arthur.petrosyan@synopsys.com> wrote:
> >>
> >> - Added backup of DCFG register.
> >> - Added Set the Power-On Programming done bit.
> >>
> >> Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
> >> ---
> >>   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

[01/14] usb: dwc2: Fix dwc2_restore_device_registers() function.

[Artur Petrosyan]

On 4/29/2019 21:34, Doug Anderson wrote:
> Hi,
> 
> On Mon, Apr 29, 2019 at 3:51 AM Artur Petrosyan
> <Arthur.Petrosyan@synopsys.com> wrote:
>>
>> On 4/27/2019 00:43, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Fri, Apr 12, 2019 at 6:38 AM Artur Petrosyan
>>> <arthur.petrosyan@synopsys.com> wrote:
>>>>
>>>> - Added backup of DCFG register.
>>>> - Added Set the Power-On Programming done bit.
>>>>
>>>> Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
>>>> ---
>>>>    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
>

Re: [PATCH 01/14] usb: dwc2: Fix dwc2_restore_device_registers() function.

[Artur Petrosyan]

On 4/29/2019 21:34, Doug Anderson wrote:
> Hi,
> 
> On Mon, Apr 29, 2019 at 3:51 AM Artur Petrosyan
> <Arthur.Petrosyan@synopsys.com> wrote:
>>
>> On 4/27/2019 00:43, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Fri, Apr 12, 2019 at 6:38 AM Artur Petrosyan
>>> <arthur.petrosyan@synopsys.com> wrote:
>>>>
>>>> - Added backup of DCFG register.
>>>> - Added Set the Power-On Programming done bit.
>>>>
>>>> Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
>>>> ---
>>>>    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

[01/14] usb: dwc2: Fix dwc2_restore_device_registers() function.

[Doug Anderson]

Hi,

On Mon, Apr 29, 2019 at 11:59 PM Artur Petrosyan
<Arthur.Petrosyan@synopsys.com> wrote:
>
> On 4/29/2019 21:34, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Apr 29, 2019 at 3:51 AM Artur Petrosyan
> > <Arthur.Petrosyan@synopsys.com> wrote:
> >>
> >> On 4/27/2019 00:43, Doug Anderson wrote:
> >>> Hi,
> >>>
> >>> On Fri, Apr 12, 2019 at 6:38 AM Artur Petrosyan
> >>> <arthur.petrosyan@synopsys.com> wrote:
> >>>>
> >>>> - Added backup of DCFG register.
> >>>> - Added Set the Power-On Programming done bit.
> >>>>
> >>>> Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
> >>>> ---
> >>>>    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

Re: [PATCH 01/14] usb: dwc2: Fix dwc2_restore_device_registers() function.

[Doug Anderson]

Hi,

On Mon, Apr 29, 2019 at 11:59 PM Artur Petrosyan
<Arthur.Petrosyan@synopsys.com> wrote:
>
> On 4/29/2019 21:34, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Apr 29, 2019 at 3:51 AM Artur Petrosyan
> > <Arthur.Petrosyan@synopsys.com> wrote:
> >>
> >> On 4/27/2019 00:43, Doug Anderson wrote:
> >>> Hi,
> >>>
> >>> On Fri, Apr 12, 2019 at 6:38 AM Artur Petrosyan
> >>> <arthur.petrosyan@synopsys.com> wrote:
> >>>>
> >>>> - Added backup of DCFG register.
> >>>> - Added Set the Power-On Programming done bit.
> >>>>
> >>>> Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
> >>>> ---
> >>>>    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

[01/14] usb: dwc2: Fix dwc2_restore_device_registers() function.

[Artur Petrosyan]

On 4/30/2019 19:29, Doug Anderson wrote:
> Hi,
> 
> On Mon, Apr 29, 2019 at 11:59 PM Artur Petrosyan
> <Arthur.Petrosyan@synopsys.com> wrote:
>>
>> On 4/29/2019 21:34, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Mon, Apr 29, 2019 at 3:51 AM Artur Petrosyan
>>> <Arthur.Petrosyan@synopsys.com> wrote:
>>>>
>>>> On 4/27/2019 00:43, Doug Anderson wrote:
>>>>> Hi,
>>>>>
>>>>> On Fri, Apr 12, 2019 at 6:38 AM Artur Petrosyan
>>>>> <arthur.petrosyan@synopsys.com> wrote:
>>>>>>
>>>>>> - Added backup of DCFG register.
>>>>>> - Added Set the Power-On Programming done bit.
>>>>>>
>>>>>> Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
>>>>>> ---
>>>>>>     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.
Ok let's forget about that.
> 
> -Doug
>

Re: [PATCH 01/14] usb: dwc2: Fix dwc2_restore_device_registers() function.

[Artur Petrosyan]

On 4/30/2019 19:29, Doug Anderson wrote:
> Hi,
> 
> On Mon, Apr 29, 2019 at 11:59 PM Artur Petrosyan
> <Arthur.Petrosyan@synopsys.com> wrote:
>>
>> On 4/29/2019 21:34, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Mon, Apr 29, 2019 at 3:51 AM Artur Petrosyan
>>> <Arthur.Petrosyan@synopsys.com> wrote:
>>>>
>>>> On 4/27/2019 00:43, Doug Anderson wrote:
>>>>> Hi,
>>>>>
>>>>> On Fri, Apr 12, 2019 at 6:38 AM Artur Petrosyan
>>>>> <arthur.petrosyan@synopsys.com> wrote:
>>>>>>
>>>>>> - Added backup of DCFG register.
>>>>>> - Added Set the Power-On Programming done bit.
>>>>>>
>>>>>> Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
>>>>>> ---
>>>>>>     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.
Ok let's forget about that.
> 
> -Doug
> 


-- 
Regards,
Artur

[02/14] usb: dwc2: Add descriptive debug messages for Partial Power Down mode.

[Artur Petrosyan]

Added dev_dbg() messages when entering and exiting from
partial power down. It is now more visible when core
enters partial power down and when exits form it.

Debug messages are added in the following functions.
- dwc2_exit_partial_power_down()
- dwc2_enter_partial_power_down()

Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
---
 drivers/usb/dwc2/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

-- 
2.11.0

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index 55d5ae2a7ec7..fb471d18a3de 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -141,6 +141,8 @@ int dwc2_exit_partial_power_down(struct dwc2_hsotg *hsotg, bool restore)
 	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
 		return -ENOTSUPP;
 
+	dev_dbg(hsotg->dev, "Exiting of Partial Power Down started.\n");
+
 	pcgcctl = dwc2_readl(hsotg, PCGCTL);
 	pcgcctl &= ~PCGCTL_STOPPCLK;
 	dwc2_writel(hsotg, pcgcctl, PCGCTL);
@@ -178,6 +180,8 @@ int dwc2_exit_partial_power_down(struct dwc2_hsotg *hsotg, bool restore)
 		}
 	}
 
+	dev_dbg(hsotg->dev, "Exit Partial Power Down completes here.\n");
+
 	return ret;
 }
 
@@ -194,6 +198,8 @@ int dwc2_enter_partial_power_down(struct dwc2_hsotg *hsotg)
 	if (!hsotg->params.power_down)
 		return -ENOTSUPP;
 
+	dev_dbg(hsotg->dev, "Start of Partial Power Down completed\n");
+
 	/* Backup all registers */
 	ret = dwc2_backup_global_registers(hsotg);
 	if (ret) {
@@ -238,6 +244,8 @@ int dwc2_enter_partial_power_down(struct dwc2_hsotg *hsotg)
 	pcgcctl |= PCGCTL_STOPPCLK;
 	dwc2_writel(hsotg, pcgcctl, PCGCTL);
 
+	dev_dbg(hsotg->dev, "Partial Power Down completed\n");
+
 	return ret;
 }
 

[PATCH 02/14] usb: dwc2: Add descriptive debug messages for Partial Power Down mode.

[Artur Petrosyan]

Added dev_dbg() messages when entering and exiting from
partial power down. It is now more visible when core
enters partial power down and when exits form it.

Debug messages are added in the following functions.
- dwc2_exit_partial_power_down()
- dwc2_enter_partial_power_down()

Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
---
 drivers/usb/dwc2/core.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c
index 55d5ae2a7ec7..fb471d18a3de 100644
--- a/drivers/usb/dwc2/core.c
+++ b/drivers/usb/dwc2/core.c
@@ -141,6 +141,8 @@ int dwc2_exit_partial_power_down(struct dwc2_hsotg *hsotg, bool restore)
 	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
 		return -ENOTSUPP;
 
+	dev_dbg(hsotg->dev, "Exiting of Partial Power Down started.\n");
+
 	pcgcctl = dwc2_readl(hsotg, PCGCTL);
 	pcgcctl &= ~PCGCTL_STOPPCLK;
 	dwc2_writel(hsotg, pcgcctl, PCGCTL);
@@ -178,6 +180,8 @@ int dwc2_exit_partial_power_down(struct dwc2_hsotg *hsotg, bool restore)
 		}
 	}
 
+	dev_dbg(hsotg->dev, "Exit Partial Power Down completes here.\n");
+
 	return ret;
 }
 
@@ -194,6 +198,8 @@ int dwc2_enter_partial_power_down(struct dwc2_hsotg *hsotg)
 	if (!hsotg->params.power_down)
 		return -ENOTSUPP;
 
+	dev_dbg(hsotg->dev, "Start of Partial Power Down completed\n");
+
 	/* Backup all registers */
 	ret = dwc2_backup_global_registers(hsotg);
 	if (ret) {
@@ -238,6 +244,8 @@ int dwc2_enter_partial_power_down(struct dwc2_hsotg *hsotg)
 	pcgcctl |= PCGCTL_STOPPCLK;
 	dwc2_writel(hsotg, pcgcctl, PCGCTL);
 
+	dev_dbg(hsotg->dev, "Partial Power Down completed\n");
+
 	return ret;
 }
 
-- 
2.11.0

[03/14] usb: dwc2: Fix wakeup detected and session request interrupt handlers.

[Artur Petrosyan]

In host mode port power must be turned on when wakeup
detected or session request interrupt is detected.
Because, otherwise core wouldn't exit form partial
power down.

- Turned on the port power by setting HPRT0_PWR bit.

- Called dwc2_hcd_connect() function after enabling
  the power of the port.

Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
---
 drivers/usb/dwc2/core_intr.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

-- 
2.11.0

diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index 19ae2595f1c3..ce523fd2b1b4 100644
--- a/drivers/usb/dwc2/core_intr.c
+++ b/drivers/usb/dwc2/core_intr.c
@@ -312,6 +312,7 @@ static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg)
 static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
 {
 	int ret;
+	u32 hprt0;
 
 	/* Clear interrupt */
 	dwc2_writel(hsotg, GINTSTS_SESSREQINT, GINTSTS);
@@ -320,7 +321,8 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
 		hsotg->lx_state);
 
 	if (dwc2_is_device_mode(hsotg)) {
-		if (hsotg->lx_state == DWC2_L2) {
+		if (hsotg->lx_state == DWC2_L2 &&
+		    hsotg->params.power_down == 1) {
 			ret = dwc2_exit_partial_power_down(hsotg, true);
 			if (ret && (ret != -ENOTSUPP))
 				dev_err(hsotg->dev,
@@ -332,6 +334,14 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
 		 * established
 		 */
 		dwc2_hsotg_disconnect(hsotg);
+	} else {
+		/* Turn on the port power bit. */
+		hprt0 = dwc2_read_hprt0(hsotg);
+		hprt0 |= HPRT0_PWR;
+		dwc2_writel(hsotg, hprt0, HPRT0);
+
+		/* Connect hcd after port power is set. */
+		dwc2_hcd_connect(hsotg);
 	}
 }
 
@@ -396,6 +406,7 @@ static void dwc2_wakeup_from_lpm_l1(struct dwc2_hsotg *hsotg)
 static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
 {
 	int ret;
+	u32 hprt0;
 
 	/* Clear interrupt */
 	dwc2_writel(hsotg, GINTSTS_WKUPINT, GINTSTS);
@@ -426,8 +437,6 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
 		/* Change to L0 state */
 		hsotg->lx_state = DWC2_L0;
 	} else {
-		if (hsotg->params.power_down)
-			return;
 
 		if (hsotg->lx_state != DWC2_L1) {
 			u32 pcgcctl = dwc2_readl(hsotg, PCGCTL);
@@ -435,6 +444,15 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
 			/* Restart the Phy Clock */
 			pcgcctl &= ~PCGCTL_STOPPCLK;
 			dwc2_writel(hsotg, pcgcctl, PCGCTL);
+
+			/* Turn on the port power bit. */
+			hprt0 = dwc2_read_hprt0(hsotg);
+			hprt0 |= HPRT0_PWR;
+			dwc2_writel(hsotg, hprt0, HPRT0);
+
+			/* Connect hcd. */
+			dwc2_hcd_connect(hsotg);
+
 			mod_timer(&hsotg->wkp_timer,
 				  jiffies + msecs_to_jiffies(71));
 		} else {

[PATCH 03/14] usb: dwc2: Fix wakeup detected and session request interrupt handlers.

[Artur Petrosyan]

In host mode port power must be turned on when wakeup
detected or session request interrupt is detected.
Because, otherwise core wouldn't exit form partial
power down.

- Turned on the port power by setting HPRT0_PWR bit.

- Called dwc2_hcd_connect() function after enabling
  the power of the port.

Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
---
 drivers/usb/dwc2/core_intr.c | 24 +++++++++++++++++++++---
 1 file changed, 21 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
index 19ae2595f1c3..ce523fd2b1b4 100644
--- a/drivers/usb/dwc2/core_intr.c
+++ b/drivers/usb/dwc2/core_intr.c
@@ -312,6 +312,7 @@ static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg)
 static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
 {
 	int ret;
+	u32 hprt0;
 
 	/* Clear interrupt */
 	dwc2_writel(hsotg, GINTSTS_SESSREQINT, GINTSTS);
@@ -320,7 +321,8 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
 		hsotg->lx_state);
 
 	if (dwc2_is_device_mode(hsotg)) {
-		if (hsotg->lx_state == DWC2_L2) {
+		if (hsotg->lx_state == DWC2_L2 &&
+		    hsotg->params.power_down == 1) {
 			ret = dwc2_exit_partial_power_down(hsotg, true);
 			if (ret && (ret != -ENOTSUPP))
 				dev_err(hsotg->dev,
@@ -332,6 +334,14 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
 		 * established
 		 */
 		dwc2_hsotg_disconnect(hsotg);
+	} else {
+		/* Turn on the port power bit. */
+		hprt0 = dwc2_read_hprt0(hsotg);
+		hprt0 |= HPRT0_PWR;
+		dwc2_writel(hsotg, hprt0, HPRT0);
+
+		/* Connect hcd after port power is set. */
+		dwc2_hcd_connect(hsotg);
 	}
 }
 
@@ -396,6 +406,7 @@ static void dwc2_wakeup_from_lpm_l1(struct dwc2_hsotg *hsotg)
 static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
 {
 	int ret;
+	u32 hprt0;
 
 	/* Clear interrupt */
 	dwc2_writel(hsotg, GINTSTS_WKUPINT, GINTSTS);
@@ -426,8 +437,6 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
 		/* Change to L0 state */
 		hsotg->lx_state = DWC2_L0;
 	} else {
-		if (hsotg->params.power_down)
-			return;
 
 		if (hsotg->lx_state != DWC2_L1) {
 			u32 pcgcctl = dwc2_readl(hsotg, PCGCTL);
@@ -435,6 +444,15 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
 			/* Restart the Phy Clock */
 			pcgcctl &= ~PCGCTL_STOPPCLK;
 			dwc2_writel(hsotg, pcgcctl, PCGCTL);
+
+			/* Turn on the port power bit. */
+			hprt0 = dwc2_read_hprt0(hsotg);
+			hprt0 |= HPRT0_PWR;
+			dwc2_writel(hsotg, hprt0, HPRT0);
+
+			/* Connect hcd. */
+			dwc2_hcd_connect(hsotg);
+
 			mod_timer(&hsotg->wkp_timer,
 				  jiffies + msecs_to_jiffies(71));
 		} else {
-- 
2.11.0

[03/14] usb: dwc2: Fix wakeup detected and session request interrupt handlers.

[Jules Maselbas]

Hi Artur,

On Fri, Apr 12, 2019 at 01:38:56PM +0000, Artur Petrosyan wrote:
> In host mode port power must be turned on when wakeup
> detected or session request interrupt is detected.
> Because, otherwise core wouldn't exit form partial
> power down.
> 
> - Turned on the port power by setting HPRT0_PWR bit.
> 
> - Called dwc2_hcd_connect() function after enabling
>   the power of the port.
> 
> Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
> ---
>  drivers/usb/dwc2/core_intr.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index 19ae2595f1c3..ce523fd2b1b4 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -312,6 +312,7 @@ static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg)
>  static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
>  {
>  	int ret;
> +	u32 hprt0;
>  
>  	/* Clear interrupt */
>  	dwc2_writel(hsotg, GINTSTS_SESSREQINT, GINTSTS);
> @@ -320,7 +321,8 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
>  		hsotg->lx_state);
>  
>  	if (dwc2_is_device_mode(hsotg)) {
> -		if (hsotg->lx_state == DWC2_L2) {
> +		if (hsotg->lx_state == DWC2_L2 &&
> +		    hsotg->params.power_down == 1) {
I think you can replace 1 with DWC2_POWER_DOWN_PARAM_PARTIAL.

[snip]
---
Best,
Jules

Re: [PATCH 03/14] usb: dwc2: Fix wakeup detected and session request interrupt handlers.

[Jules Maselbas]

Hi Artur,

On Fri, Apr 12, 2019 at 01:38:56PM +0000, Artur Petrosyan wrote:
> In host mode port power must be turned on when wakeup
> detected or session request interrupt is detected.
> Because, otherwise core wouldn't exit form partial
> power down.
> 
> - Turned on the port power by setting HPRT0_PWR bit.
> 
> - Called dwc2_hcd_connect() function after enabling
>   the power of the port.
> 
> Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
> ---
>  drivers/usb/dwc2/core_intr.c | 24 +++++++++++++++++++++---
>  1 file changed, 21 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index 19ae2595f1c3..ce523fd2b1b4 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -312,6 +312,7 @@ static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg)
>  static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
>  {
>  	int ret;
> +	u32 hprt0;
>  
>  	/* Clear interrupt */
>  	dwc2_writel(hsotg, GINTSTS_SESSREQINT, GINTSTS);
> @@ -320,7 +321,8 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
>  		hsotg->lx_state);
>  
>  	if (dwc2_is_device_mode(hsotg)) {
> -		if (hsotg->lx_state == DWC2_L2) {
> +		if (hsotg->lx_state == DWC2_L2 &&
> +		    hsotg->params.power_down == 1) {
I think you can replace 1 with DWC2_POWER_DOWN_PARAM_PARTIAL.

[snip]

---
Best,
Jules

[03/14] usb: dwc2: Fix wakeup detected and session request interrupt handlers.

[Artur Petrosyan]

Hi Jules,

On 4/12/2019 18:21, Jules Maselbas wrote:
> Hi Artur,
> 
> On Fri, Apr 12, 2019 at 01:38:56PM +0000, Artur Petrosyan wrote:
>> In host mode port power must be turned on when wakeup
>> detected or session request interrupt is detected.
>> Because, otherwise core wouldn't exit form partial
>> power down.
>>
>> - Turned on the port power by setting HPRT0_PWR bit.
>>
>> - Called dwc2_hcd_connect() function after enabling
>>    the power of the port.
>>
>> Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
>> ---
>>   drivers/usb/dwc2/core_intr.c | 24 +++++++++++++++++++++---
>>   1 file changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>> index 19ae2595f1c3..ce523fd2b1b4 100644
>> --- a/drivers/usb/dwc2/core_intr.c
>> +++ b/drivers/usb/dwc2/core_intr.c
>> @@ -312,6 +312,7 @@ static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg)
>>   static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
>>   {
>>   	int ret;
>> +	u32 hprt0;
>>   
>>   	/* Clear interrupt */
>>   	dwc2_writel(hsotg, GINTSTS_SESSREQINT, GINTSTS);
>> @@ -320,7 +321,8 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
>>   		hsotg->lx_state);
>>   
>>   	if (dwc2_is_device_mode(hsotg)) {
>> -		if (hsotg->lx_state == DWC2_L2) {
>> +		if (hsotg->lx_state == DWC2_L2 &&
>> +		    hsotg->params.power_down == 1) {
> I think you can replace 1 with DWC2_POWER_DOWN_PARAM_PARTIAL.

Thank you for the review. Yeah, you are right it should be updated.

> 
> [snip]
> 
> ---
> Best,
> Jules
> 
>

Re: [PATCH 03/14] usb: dwc2: Fix wakeup detected and session request interrupt handlers.

[Artur Petrosyan]

Hi Jules,

On 4/12/2019 18:21, Jules Maselbas wrote:
> Hi Artur,
> 
> On Fri, Apr 12, 2019 at 01:38:56PM +0000, Artur Petrosyan wrote:
>> In host mode port power must be turned on when wakeup
>> detected or session request interrupt is detected.
>> Because, otherwise core wouldn't exit form partial
>> power down.
>>
>> - Turned on the port power by setting HPRT0_PWR bit.
>>
>> - Called dwc2_hcd_connect() function after enabling
>>    the power of the port.
>>
>> Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
>> ---
>>   drivers/usb/dwc2/core_intr.c | 24 +++++++++++++++++++++---
>>   1 file changed, 21 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>> index 19ae2595f1c3..ce523fd2b1b4 100644
>> --- a/drivers/usb/dwc2/core_intr.c
>> +++ b/drivers/usb/dwc2/core_intr.c
>> @@ -312,6 +312,7 @@ static void dwc2_handle_conn_id_status_change_intr(struct dwc2_hsotg *hsotg)
>>   static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
>>   {
>>   	int ret;
>> +	u32 hprt0;
>>   
>>   	/* Clear interrupt */
>>   	dwc2_writel(hsotg, GINTSTS_SESSREQINT, GINTSTS);
>> @@ -320,7 +321,8 @@ static void dwc2_handle_session_req_intr(struct dwc2_hsotg *hsotg)
>>   		hsotg->lx_state);
>>   
>>   	if (dwc2_is_device_mode(hsotg)) {
>> -		if (hsotg->lx_state == DWC2_L2) {
>> +		if (hsotg->lx_state == DWC2_L2 &&
>> +		    hsotg->params.power_down == 1) {
> I think you can replace 1 with DWC2_POWER_DOWN_PARAM_PARTIAL.

Thank you for the review. Yeah, you are right it should be updated.

> 
> [snip]
> 
> ---
> Best,
> Jules
> 
> 

[04/14] usb: dwc2: Fix suspend state in host mode for partial power down.

[Artur Petrosyan]

- In dwc2_port_suspend() function added waiting for the
  HPRT0.PrtSusp register field to be set.

- In _dwc2_hcd_suspend() function added checking of
  "hsotg->flags.b.port_connect_status" port connection
  status if port connection status is 0 then skipping
  power saving (entering partial power down mode).
  Because if there is no device connected there would
  be no need to enter partial power down mode.

- Added "hsotg->bus_suspended = true" beceuse after
  entering partial power down in host mode the
  bus_suspended flag must be set.

Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
---
 drivers/usb/dwc2/hcd.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

-- 
2.11.0

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index dd82fa516f3f..1d18258b5ff8 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -3479,6 +3479,10 @@ static void dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex)
 	hprt0 |= HPRT0_SUSP;
 	dwc2_writel(hsotg, hprt0, HPRT0);
 
+	/* Wait for the HPRT0.PrtSusp register field to be set */
+	if (dwc2_hsotg_wait_bit_set(hsotg, HPRT0, HPRT0_SUSP, 3000))
+		dev_warn(hsotg->dev, "Suspend wasn't generated\n");
+
 	hsotg->bus_suspended = true;
 
 	/*
@@ -4488,7 +4492,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
 	if (hsotg->op_state == OTG_STATE_B_PERIPHERAL)
 		goto unlock;
 
-	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
+	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL ||
+	    hsotg->flags.b.port_connect_status == 0)
 		goto skip_power_saving;
 
 	/*
@@ -4514,6 +4519,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
 		goto skip_power_saving;
 	}
 
+	hsotg->bus_suspended = true;
+
 	/* Ask phy to be suspended */
 	if (!IS_ERR_OR_NULL(hsotg->uphy)) {
 		spin_unlock_irqrestore(&hsotg->lock, flags);

[PATCH 04/14] usb: dwc2: Fix suspend state in host mode for partial power down.

[Artur Petrosyan]

- In dwc2_port_suspend() function added waiting for the
  HPRT0.PrtSusp register field to be set.

- In _dwc2_hcd_suspend() function added checking of
  "hsotg->flags.b.port_connect_status" port connection
  status if port connection status is 0 then skipping
  power saving (entering partial power down mode).
  Because if there is no device connected there would
  be no need to enter partial power down mode.

- Added "hsotg->bus_suspended = true" beceuse after
  entering partial power down in host mode the
  bus_suspended flag must be set.

Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
---
 drivers/usb/dwc2/hcd.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index dd82fa516f3f..1d18258b5ff8 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -3479,6 +3479,10 @@ static void dwc2_port_suspend(struct dwc2_hsotg *hsotg, u16 windex)
 	hprt0 |= HPRT0_SUSP;
 	dwc2_writel(hsotg, hprt0, HPRT0);
 
+	/* Wait for the HPRT0.PrtSusp register field to be set */
+	if (dwc2_hsotg_wait_bit_set(hsotg, HPRT0, HPRT0_SUSP, 3000))
+		dev_warn(hsotg->dev, "Suspend wasn't generated\n");
+
 	hsotg->bus_suspended = true;
 
 	/*
@@ -4488,7 +4492,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
 	if (hsotg->op_state == OTG_STATE_B_PERIPHERAL)
 		goto unlock;
 
-	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
+	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL ||
+	    hsotg->flags.b.port_connect_status == 0)
 		goto skip_power_saving;
 
 	/*
@@ -4514,6 +4519,8 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hcd)
 		goto skip_power_saving;
 	}
 
+	hsotg->bus_suspended = true;
+
 	/* Ask phy to be suspended */
 	if (!IS_ERR_OR_NULL(hsotg->uphy)) {
 		spin_unlock_irqrestore(&hsotg->lock, flags);
-- 
2.11.0

[05/14] usb: dwc2: Add port conn. sts. checking in _dwc2_hcd_resume() function.

[Artur Petrosyan]

Added port connection status checking which prevents exiting from
Partial Power Down mode from _dwc2_hcd_resume() when no entering
to Partial Power Down mode happened.

Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
---
 drivers/usb/dwc2/hcd.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

-- 
2.11.0

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 1d18258b5ff8..8367902a47eb 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4544,6 +4544,9 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
 	struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
 	unsigned long flags;
 	int ret = 0;
+	u32 hprt0;
+
+	hprt0 = dwc2_read_hprt0(hsotg);
 
 	spin_lock_irqsave(&hsotg->lock, flags);
 
@@ -4553,7 +4556,8 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
 	if (hsotg->lx_state != DWC2_L2)
 		goto unlock;
 
-	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL) {
+	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL ||
+	    hprt0 & HPRT0_CONNSTS) {
 		hsotg->lx_state = DWC2_L0;
 		goto unlock;
 	}

[PATCH 05/14] usb: dwc2: Add port conn. sts. checking in _dwc2_hcd_resume() function.

[Artur Petrosyan]

Added port connection status checking which prevents exiting from
Partial Power Down mode from _dwc2_hcd_resume() when no entering
to Partial Power Down mode happened.

Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
---
 drivers/usb/dwc2/hcd.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 1d18258b5ff8..8367902a47eb 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4544,6 +4544,9 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
 	struct dwc2_hsotg *hsotg = dwc2_hcd_to_hsotg(hcd);
 	unsigned long flags;
 	int ret = 0;
+	u32 hprt0;
+
+	hprt0 = dwc2_read_hprt0(hsotg);
 
 	spin_lock_irqsave(&hsotg->lock, flags);
 
@@ -4553,7 +4556,8 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd)
 	if (hsotg->lx_state != DWC2_L2)
 		goto unlock;
 
-	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL) {
+	if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL ||
+	    hprt0 & HPRT0_CONNSTS) {
 		hsotg->lx_state = DWC2_L0;
 		goto unlock;
 	}
-- 
2.11.0

[06/14] usb: dwc2: Add part. power down exit from dwc2_conn_id_status_change().

[Artur Petrosyan]

Before changing to connector B exiting from Partial
Power Down is required.

- Added exiting from Partial Power Down mode when
  connector ID status changes to "connId B".
  Because if connector ID status changed to B connector
  while core was in partial power down mode, HANG would
  accrue from a soft reset.

Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
---
 drivers/usb/dwc2/hcd.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

-- 
2.11.0

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 8367902a47eb..54450fa352cf 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -3348,6 +3348,7 @@ static void dwc2_conn_id_status_change(struct work_struct *work)
 						wf_otg);
 	u32 count = 0;
 	u32 gotgctl;
+	u32 pcgcctl;
 	unsigned long flags;
 
 	dev_dbg(hsotg->dev, "%s()\n", __func__);
@@ -3387,6 +3388,23 @@ static void dwc2_conn_id_status_change(struct work_struct *work)
 		if (count > 250)
 			dev_err(hsotg->dev,
 				"Connection id status change timed out\n");
+
+		if (hsotg->params.power_down ==
+		    DWC2_POWER_DOWN_PARAM_PARTIAL &&
+		    hsotg->lx_state == DWC2_L2) {
+			pcgcctl = dwc2_readl(hsotg, PCGCTL);
+			pcgcctl &= ~PCGCTL_STOPPCLK;
+			dwc2_writel(hsotg, pcgcctl, PCGCTL);
+
+			pcgcctl = dwc2_readl(hsotg, PCGCTL);
+			pcgcctl &= ~PCGCTL_PWRCLMP;
+			dwc2_writel(hsotg, pcgcctl, PCGCTL);
+
+			pcgcctl = dwc2_readl(hsotg, PCGCTL);
+			pcgcctl &= ~PCGCTL_RSTPDWNMODULE;
+			dwc2_writel(hsotg, pcgcctl, PCGCTL);
+		}
+
 		hsotg->op_state = OTG_STATE_B_PERIPHERAL;
 		dwc2_core_init(hsotg, false);
 		dwc2_enable_global_interrupts(hsotg);

[PATCH 06/14] usb: dwc2: Add part. power down exit from dwc2_conn_id_status_change().

[Artur Petrosyan]

Before changing to connector B exiting from Partial
Power Down is required.

- Added exiting from Partial Power Down mode when
  connector ID status changes to "connId B".
  Because if connector ID status changed to B connector
  while core was in partial power down mode, HANG would
  accrue from a soft reset.

Signed-off-by: Artur Petrosyan <arturp@synopsys.com>
---
 drivers/usb/dwc2/hcd.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 8367902a47eb..54450fa352cf 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -3348,6 +3348,7 @@ static void dwc2_conn_id_status_change(struct work_struct *work)
 						wf_otg);
 	u32 count = 0;
 	u32 gotgctl;
+	u32 pcgcctl;
 	unsigned long flags;
 
 	dev_dbg(hsotg->dev, "%s()\n", __func__);
@@ -3387,6 +3388,23 @@ static void dwc2_conn_id_status_change(struct work_struct *work)
 		if (count > 250)
 			dev_err(hsotg->dev,
 				"Connection id status change timed out\n");
+
+		if (hsotg->params.power_down ==
+		    DWC2_POWER_DOWN_PARAM_PARTIAL &&
+		    hsotg->lx_state == DWC2_L2) {
+			pcgcctl = dwc2_readl(hsotg, PCGCTL);
+			pcgcctl &= ~PCGCTL_STOPPCLK;
+			dwc2_writel(hsotg, pcgcctl, PCGCTL);
+
+			pcgcctl = dwc2_readl(hsotg, PCGCTL);
+			pcgcctl &= ~PCGCTL_PWRCLMP;
+			dwc2_writel(hsotg, pcgcctl, PCGCTL);
+
+			pcgcctl = dwc2_readl(hsotg, PCGCTL);
+			pcgcctl &= ~PCGCTL_RSTPDWNMODULE;
+			dwc2_writel(hsotg, pcgcctl, PCGCTL);
+		}
+
 		hsotg->op_state = OTG_STATE_B_PERIPHERAL;
 		dwc2_core_init(hsotg, false);
 		dwc2_enable_global_interrupts(hsotg);
-- 
2.11.0