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

[Artur Petrosyan]

Hi Doug,

On 4/26/2019 00:13, Doug Anderson wrote:
> Hi,
> 
> On Thu, Apr 25, 2019 at 7:01 AM Artur Petrosyan
> <Arthur.Petrosyan@xxxxxxxxxxxx> wrote:
>>
>> Hi,
>>
>> On 4/25/2019 16:43, Felipe Balbi wrote:
>>> Artur Petrosyan <Arthur.Petrosyan@xxxxxxxxxxxx> writes:
>>>> This patch set, fixes and improves partial power down and hibernation power
>>>> saving modes. Also, adds support for entering/exiting hibernation by
>>>> system issued suspend/resume.
>>>>
>>>> This series contains patches which were submitted to LKML. However, a part
>>>> of those patches didn't reach to LKML because of local issue related to
>>>> smtp server.
>>>>
>>>> The patches which reached to LKML are:
>>>>
>>>> - usb: dwc2: Add part. power down exit from dwc2_conn_id_status_change().
>>>> - usb: dwc2: Add port conn. sts. checking in _dwc2_hcd_resume() function.
>>>> - usb: dwc2: Fix suspend state in host mode for partial power down.
>>>> - usb: dwc2: Fix wakeup detected and session request interrupt handlers.
>>>> - usb: dwc2: Add descriptive debug messages for Partial Power Down mode.
>>>> - usb: dwc2: Fix dwc2_restore_device_registers() function.
>>>>
>>>> The patches which didn't reach to LKML are:
>>>>
>>>> - usb: dwc2: Add enter/exit hibernation from system issued suspend/resume
>>>> - usb: dwc2: Clear GINTSTS_RESTOREDONE bit after restore is generated.
>>>> - usb: dwc2: Clear fifo_map when resetting core.
>>>> - usb: dwc2: Allow exiting hibernation from gpwrdn rst detect
>>>> - usb: dwc2: Fix hibernation between host and device modes.
>>>> - usb: dwc2: Update dwc2_handle_usb_suspend_intr function.
>>>> - usb: dwc2: Add default param to control power optimization.
>>>> - usb: dwc2: Reset DEVADDR after exiting gadget hibernation.
>>>>
>>>> Submitting all of the patches together in this version.
>>>>
>>>> Changes from V0:
>>>>    - Replaced 1 with DWC2_POWER_DOWN_PARAM_PARTIAL in commit
>>>>      "9eed02b9fe96 usb: dwc2: Fix wakeup detected and session request
>>>>      interrupt handlers.
>>>>
>>>>
>>>> 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
>>>
>>> patches don't apply.
>>>
>>
>> Do we need to wait for Minas's acknowledge or there is problem related
>> to the patches?
> 
> It looks like the problem is that my patches won the race and Felipe
> applied them before yours.  Thus, presumably, it'll be up to you to
> rebase your patches atop mine and re-submit.  Specifically, you can
> do:
> 
> git remote add linux_usb_balbi
> git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git
> git fetch linux_usb_balbi
> git checkout linux_usb_balbi/testing/next
> 
> If you do that and then try to apply your patches you'll find that
> they no longer apply.  AKA try running:
> 
> for patch in 10909749 10909737 10909739 10909745 10909533 \
>     10909531 10909747 10909535 10909523 10909741 10909525 \
>     10909751 10909527 10909743; do
>    curl -L https://urldefense.proofpoint.com/v2/url?u=https-3A__patchwork.kernel.org_patch_-24-257Bpatch-257D_mbox&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=gzzEDEbxflLMgk9I-7Re9ytRMvyc_B8iZEmg2xGZN5E&s=aWT1hYtXeIeY8ClQ0sNYxwkmJFKDz4iaa4DchNwx3_w&e= | git am
> done
> 
> You'll see:
> 
>> Applying: usb: dwc2: Fix wakeup detected and session request interrupt handlers.
>> error: patch failed: drivers/usb/dwc2/core_intr.c:435
>> error: drivers/usb/dwc2/core_intr.c: patch does not apply
>> Patch failed at 0001 usb: dwc2: Fix wakeup detected and session request interrupt handlers.
> 
> NOTE: before reposting it might be a good idea to apply the last 3
> patches in my series as per [1] before sending up your series.  Since
> Felipe has already applied patches #1 and #2 in that series presumably
> he'll also apply #3 - #5.
> 
> I know it'a also up to me to try testing our your patches.  It's still
> on my list to give it a shot...
> 
> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_CAD-3DFV-3DWA07-2BgUkVvsikN-3DiDHZLUJQtzjkKtiBHAEDw4gLNWY7w-40mail.gmail.com&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=gzzEDEbxflLMgk9I-7Re9ytRMvyc_B8iZEmg2xGZN5E&s=9QP_h5ZlnT7ayUE1_6EVEgu8FaI3_kWm9xuzs1qrvdI&e=
> 
> P.S: It's helpful if you CC LKML on patches and discussions about
> them.  That allows the magic "permalink via message ID" on
> lkml.kernel.org and also allows your patches to be found on
> lore.kernel.org/patchwork/
> 
> -Doug
> 

Besides the issue that comes from the patch "usb: dwc2: Fix wakeup 
detected and session request interrupt handlers." there is one more 
serious conflict with one of your patches.

So the patch "usb: dwc2: bus suspend/resume for hosts with 
DWC2_POWER_DOWN_PARAM_NONE" have had also been added to the 
"balbi/testing/next" before my patch series which conflicts with two of 
my patches.

1. usb: dwc2: Fix suspend state in host mode for partial power down.
2. usb: dwc2: Add enter/exit hibernation from system issued suspend/resume

This patch introduced by you "usb: dwc2: bus suspend/resume for hosts 
with DWC2_POWER_DOWN_PARAM_NONE" got a little bit of issue. It 
eliminates entering hibernation through system issued suspend by 
checking "if (hsotg->params.power_down > DWC2_POWER_DOWN_PARAM_PARTIAL)" 
. As per the patch you mention that it fixes suspend/resume flow for 
Altera SOCFPGA and Chrome OS 3.14 kernel tree. I assume that the board 
has the Partial Power Down enabled core that is why it works out. 
However, we don't just support the Partial Power Down feature enabled 
cores. We also support Hibernation feature enabled cores.

The patch set that had been introduced by me which includes "usb: dwc2: 
Add enter/exit hibernation from system issued suspend/resume" patch adds 
support for both hibernation and Partial Power Down feature enabled 
cores and fixes several of Partial Power Down and hibernation related 
issues.

This patch set may fix all of the issues related with Altera SOCFPGA or
Chrome OS 3.14 kernel tree.

That is why we asked you to test the patch set before we could ACK or 
have chance to debug your patch deeper to see the help of it and to 
provide you information related to it.

So now I can rebase my changes to the "balbi/testing/next" but I will 
have to take the logic of skipping Hibernation out otherwise we will 
have problems with hibernation enabled cores.

We can ask Balbi to permanently suspend adding of the patch "usb: dwc2: 
bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE" and my 
patch series to his "testing/next". After you can have a chance to test 
my patch series we can see if the patches are acknowledged and ask Balbi 
  to add them.

-- 
Regards,
Artur