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

[Doug Anderson]

Hi,

On Fri, Apr 26, 2019 at 9:01 AM Doug Anderson <dianders@xxxxxxxxxxxx> wrote:
>
> Hi,
>
> On Fri, Apr 26, 2019 at 12:11 AM Artur Petrosyan
> <Arthur.Petrosyan@xxxxxxxxxxxx> wrote:
> >
> > 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)"
>
> To be fair, the patch does not make entering hibernation worse, does
> it?  Specifically, I'll point to this part of the diff:
>
> - if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL) {
> + if (hsotg->params.power_down > DWC2_POWER_DOWN_PARAM_PARTIAL) {
>
> As you can see, if power_down == DWC2_POWER_DOWN_PARAM_HIBERNATION the
> flow for this "if" test is exactly the same before and after my patch.
>
>
> > . 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.
>
> I mentioned some of this in my cover letter [1].  To rehash, I said
> "Turning on partial power down on rk3288 doesn't "just work".  I don't
> get hotplug events.  This is despite dwc2 auto-detecting that we are
> power optimized."
>
> ...it is certainly possible that partial power down would work on
> rk3288 if someone had the time to debug it.
>
> NOTE: I don't have an Altera SOCFPGA, but I'll mention that a previous
> iteration of my patch (written by Kever Yang at Rockchip) was reverted
> because it _broke_ Altera SOCFPGAS.  Given my requests to test the new
> version have been met with silence, I'm inclined to land this and hope
> it's all good.  If there are problems then hopefully some actual
> details can be provided.  Last time there were none provided.
>
>
> > However, we don't just support the Partial Power Down feature enabled
> > cores. We also support Hibernation feature enabled cores.
>
> Sure, but the code that is actually landed upstream (even before my
> series) almost certainly doesn't function for Hibernation.  As pointed
> out above the entire "_dwc2_hcd_suspend()" function had a great big:
>
> if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL)
>   goto skip_power_saving;
>
> ...which, as far as I could tell, meant that Hibernation could not
> possible work.
>
>
> > 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.
>
> It may well fix my problems and maybe I can use partial power down
> now.  That'd be nice.  It was on my list and I would have worked on it
> last week except that your patches weren't on the mailing list then.
> ...so I moved on to some other work.  To avoid context switching too
> much I needed to get to a stopping point before testing your patches.
> I was hoping to have some nice rebased patches from you to test today,
> but maybe I'll try a hand at rebasing them myself.
>
> NOTE also that though I ported this change from the Chrome OS 3.14
> kernel tree, I'm actually currently working on the Chrome OS 4.19
> tree.  I also made sure to test the changes on mainline Linux.
>
>
> > 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.
>
> As per above, please have a careful look at my patches and you'll see
> that I was not introducing code that skipped hibernation.  I was
> keeping the same flow as the old code that skipped hibernation.  So if
> you are making hibernation work there should be no problems removing
> that.
>
>
> > 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.
>
> Personally I'd prefer if Felipe finished landing my series and then
> you rebased atop it.  As I said I'm convinced I'm not making your
> hibernation case any worse.  If you know of actual things that are
> made worse by my series then that would be a reason not to land it,
> but so far I haven't been convinced.
>
>
> [1] https://lkml.kernel.org/r/20190418001356.124334-2-dianders@xxxxxxxxxxxx

To add a bit of breadcrumbs, I did the rebase atop my patches and also
did some basic review of yours.  Other than a few nits I think I found
at least one bug where you're unlocking a spinlock twice in the
partial power down case.

I continue to be convinced that the right thing to do is to finish
landing my series and then once you've spun yours atop mine we can
look at landing it.

Here's all my picks atop Felipe's tree that show how I resolved
things:  https://chromium.googlesource.com/chromiumos/third_party/kernel/+log/refs/sandbox/dianders/190426-dwc2-stuff


-Doug