lkml.org 
[lkml]   [2019]   [Apr]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v1 00/14] usb: dwc2: Fix and improve power saving modes.
Date
Hi,

On 4/26/2019 20:02, Doug Anderson wrote:
> Hi,
>
> On Fri, Apr 26, 2019 at 12:11 AM Artur Petrosyan
> <Arthur.Petrosyan@synopsys.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@synopsys.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 4/25/2019 16:43, Felipe Balbi wrote:
>>>>> Artur Petrosyan <Arthur.Petrosyan@synopsys.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.
>
>

From the point of making hibernation worse no it doesn't.

>> . 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."
>

Have you tested your patch on any board?


> ...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.
>
>
Ok so after rebase it may be removed.

>> 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=Bgo5YsUKytageORysPEnHFDC2KH68gUT5GSuZFXYBiU&s=5X1sj6qNMNW85zQbTzXJuKIgH74L-P8LsGfSi66MjDE&e=
>
I have had a look on your patch and made some comments.

Also, tested your patch "usb: dwc2: bus suspend/resume for hosts with
DWC2_POWER_DOWN_PARAM_NONE" on HAPS-DX. With this patch or without it
when I have a device connected core enters to partial power down and
doesn't exit from it. The attached device cannot be used.



--
Regards,
Artur

\
 
 \ /
  Last update: 2019-04-29 10:44    [W:0.092 / U:0.088 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site