Message-ID: <SN1PR12MB2431C9312AD486935C9B728AA7390@SN1PR12MB2431.namprd12.prod.outlook.com>
Date:   Mon, 29 Apr 2019 12:05:29 +0000
From:   Artur Petrosyan <Arthur.Petrosyan@...opsys.com>
To:     Doug Anderson <dianders@...omium.org>
CC:     Felipe Balbi <balbi@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Minas Harutyunyan <Minas.Harutyunyan@...opsys.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        John Youn <John.Youn@...opsys.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v1 00/14] usb: dwc2: Fix and improve power saving modes.

Hi,

On 4/27/2019 01:06, Doug Anderson wrote:
> Hi,
> 
> On Fri, Apr 26, 2019 at 9:01 AM Doug Anderson <dianders@...omium.org> wrote:
>>
>> Hi,
>>
>> On Fri, Apr 26, 2019 at 12:11 AM Artur Petrosyan
>> <Arthur.Petrosyan@...opsys.com> 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@...opsys.com> wrote:
>>>>>
>>>>> Hi,
>>>>>
>>>>> On 4/25/2019 16:43, Felipe Balbi wrote:
>>>>>> Artur Petrosyan <Arthur.Petrosyan@...opsys.com> 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

Powered by blists - more mailing lists