Hi, On 4/27/2019 01:06, Doug Anderson wrote: > 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://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_20190418001356.124334-2D2-2Ddianders-40chromium.org&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=mdz-R9O5TUR_zXEeeCZO2341dvcwZro2cedCZzIIans&s=6qAU20vyvZqIWdkRGSUWEdiTr57arbJf2uHECkCuwQg&e= > > 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. Yeah thank you so much for the review it really helps to make conclusions on the implementations. I have tested those patches on HAPS-DX platform and have not got any problem. Hibernation and partial power down flows are working ok. > > 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://urldefense.proofpoint.com/v2/url?u=https-3A__chromium.googlesource.com_chromiumos_third-5Fparty_kernel_-2Blog_refs_sandbox_dianders_190426-2Ddwc2-2Dstuff&d=DwIFaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=mdz-R9O5TUR_zXEeeCZO2341dvcwZro2cedCZzIIans&s=mVfBGtiMQg2XVHXmGCWcd584g0DYRH1JDVnEnJWE6P8&e= > > > -Doug > I will make my changes then will go ahead and rebase. -- Regards, Artur
