From: Artur Petrosyan <Arthur.Petrosyan@synopsys.com>
To: "Doug Anderson" <dianders@chromium.org>,
"Minas Harutyunyan" <Minas.Harutyunyan@synopsys.com>,
"Felipe Balbi" <felipe.balbi@linux.intel.com>,
"Heiko Stübner" <heiko@sntech.de>
Cc: Alan Stern <stern@rowland.harvard.edu>,
Alexandru M Stan <amstan@chromium.org>,
"open list:ARM/Rockchip SoC..."
<linux-rockchip@lists.infradead.org>,
William Wu <william.wu@rock-chips.com>,
"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
Stefan Wahren <stefan.wahren@i2se.com>,
Randy Li <ayaka@soulik.info>, Chris <zyw@rock-chips.com>,
Matthias Kaehlcke <mka@chromium.org>,
Ryan Case <ryandcase@chromium.org>,
Amelie Delaunay <amelie.delaunay@st.com>,
"Julius Werner" <jwerner@chromium.org>,
Dinh Nguyen <dinguyen@opensource.altera.com>,
Elaine Zhang <zhangqing@rock-chips.com>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
Date: Fri, 3 May 2019 08:25:48 +0000 [thread overview]
Message-ID: <SN1PR12MB243136608514210F3E3E536EA7350@SN1PR12MB2431.namprd12.prod.outlook.com> (raw)
In-Reply-To: CAD=FV=UGjQz9Di=NL_r_g1Hofqv-FWBywfSm9Vu6gGr22wzPrA@mail.gmail.com
On 5/2/2019 03:58, Doug Anderson wrote:
> Hi,
>
>
> On Wed, Apr 17, 2019 at 5:15 PM Douglas Anderson <dianders@chromium.org> wrote:
>>
>> This is an attempt to rehash commit 0cf884e819e0 ("usb: dwc2: add bus
>> suspend/resume for dwc2") on ToT. That commit was reverted in commit
>> b0bb9bb6ce01 ("Revert "usb: dwc2: add bus suspend/resume for dwc2"")
>> because apparently it broke the Altera SOCFPGA.
>>
>> With all the changes that have happened to dwc2 in the meantime, it's
>> possible that the Altera SOCFPGA will just magically work with this
>> change now. ...and it would be good to get bus suspend/resume
>> implemented.
>>
>> This change is a forward port of one that's been living in the Chrome
>> OS 3.14 kernel tree.
>>
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> ---
>> This patch was last posted at:
>>
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.kernel.org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=7rxT8EFX9mqUDtTL4P7iuzYNsYROe9rxHGCresSKPTg&s=lTaNUA2XIYPat417fkd1A4Zpvb5eyYtTc1H_NIfW8Vw&e=
>>
>> ...and appears to have died the death of silence. Maybe it could get
>> some bake time in linuxnext if we can't find any proactive testing?
>>
>> I will also freely admit that I don't know tons about the theory
>> behind this patch. I'm mostly just re-hashing the original commit
>> from Kever that was reverted since:
>> * 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.
>> * If we don't do something like this commit we don't get into as low
>> of a power mode.
>
> OK, I spent the day digging more into this patch to confirm that it's
> really the right thing to do. ...and it still seems to be.
>
> First off: I'm pretty sure the above sentence "If we don't do
> something like this commit we don't get into as low of a power mode."
> is totally wrong. Luckily it's "after the cut" and not part of the
> commit message. Specifically I did a bunch of power testing and I
> couldn't find any instance saving power after this patch.
>
> ...but, then I looked more carefully at all the history of this
> commit. I ended up at:
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__chromium-2Dreview.googlesource.com_c_chromiumos_third-5Fparty_kernel_-2B_306265_&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=7rxT8EFX9mqUDtTL4P7iuzYNsYROe9rxHGCresSKPTg&s=LiyyIyaCPmr88nJeI7TCGtoJBFLRWir_reikYtAHHDw&e=
Looking at this code review I see that this patch fixes whatever issues
you have on Chrome OS 3.14. But your patch has landed on the top of
latest Kernel version. With the latest version I think you would not
have the regression issue.
So you are fixing Chrome OS 3.14.
>
> ...where I said that this fixes a resume speed regression. More
> details could be found at the linked bug, AKA:
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.chromium.org_p_chromium_issues_detail-3Fid-3D548336&d=DwIBaQ&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=7rxT8EFX9mqUDtTL4P7iuzYNsYROe9rxHGCresSKPTg&s=7gK8ZGX2zZPqC98CDMhqxEY3Acm_TbYa3fpQjWtvexM&e=
>
> ...but, sadly, I wasn't as verbose as I usually am and didn't describe
> my exact testing setup. So I tried to reproduce. ...and I was able
> to. I tested on an rk3288-veyron-jerry with an empty USB hub plugged
> into the left port (the host port) and my "servo 2" debug board hooked
> up to the right port. The "power_Resume" test in Chrome OS certainly
> showed a regression in 3.14 when doing a suspend/resume cycle.
>
>
> Digging into the logs in 3.14, before this patch I saw this in the logs:
>
> usb 3-1: reset high-speed USB device number 2 using dwc2
> usb 3-1.7: reset high-speed USB device number 3 using dwc2
>
> ...after this patch:
>
> usb 3-1: USB disconnect, device number 2
> usb 3-1.7: USB disconnect, device number 3
> usb 3-1: new high-speed USB device number 4 using dwc2
> usb 3-1: New USB device found, idVendor=1a40, idProduct=0201, bcdDevice= 1.00
> usb 3-1: New USB device strings: Mfr=0, Product=1, SerialNumber=0
> usb 3-1: Product: USB 2.0 Hub [MTT]
> usb 3-1.7: new high-speed USB device number 5 using dwc2
> usb 3-1.7: New USB device found, idVendor=1a40, idProduct=0101, bcdDevice= 1.11
> usb 3-1.7: New USB device strings: Mfr=0, Product=1, SerialNumber=0
> usb 3-1.7: Product: USB 2.0 Hub
>
> ...so basically my belief is that without this patch we're just sorta
> leaving the device hanging and it get confused on resume. After this
> patch we behave slightly better.
>
> I tested on 4.19 and found much the same. There:
>
> usb 2-1: reset high-speed USB device number 2 using dwc2
> usb 2-1.7: reset high-speed USB device number 3 using dwc2
>
> vs.
>
> usb 2-1.7: USB disconnect, device number 3
> usb 2-1: USB disconnect, device number 2
> usb 2-1: new high-speed USB device number 4 using dwc2
> usb 2-1: New USB device found, idVendor=1a40, idProduct=0201, bcdDevice= 1.00
> usb 2-1: New USB device strings: Mfr=0, Product=1, SerialNumber=0
> usb 2-1: Product: USB 2.0 Hub [MTT]
> usb 2-1.7: new high-speed USB device number 5 using dwc2
> usb 2-1.7: New USB device found, idVendor=1a40, idProduct=0101, bcdDevice= 1.11
> usb 2-1.7: New USB device strings: Mfr=0, Product=1, SerialNumber=0
> usb 2-1.7: Product: USB 2.0 Hub
>
>
> On 4.19 I didn't actually notice a the same resume time regression,
> presumably because things are happening more asynchronously there (I
> didn't confirm this). ...but in any case it seems like the right
> thing to do to actually do the suspend.
>
>
> I'll also re-iterate once more that I'm not claiming that my patch
> helps with "partial power down". It merely makes the "power savings
> disabled" case work more properly.
>
>
> I'll also note that my patch is already in Felipe's "testing/next"
> branch which I continue to believe is correct and good.
>
> -Doug
>
--
Regards,
Artur
next prev parent reply other threads:[~2019-05-03 8:25 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-04-18 0:13 [PATCH v2 0/5] USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron Douglas Anderson
2019-04-18 0:13 ` [PATCH v2 1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE Douglas Anderson
2019-04-29 8:43 ` Artur Petrosyan
2019-04-29 17:33 ` Doug Anderson
2019-04-30 6:05 ` Artur Petrosyan
2019-05-01 1:51 ` Doug Anderson
2019-05-03 8:20 ` Artur Petrosyan
2019-05-03 15:03 ` Doug Anderson
2019-05-07 7:26 ` Artur Petrosyan
2019-05-01 23:58 ` Doug Anderson
2019-05-03 8:25 ` Artur Petrosyan [this message]
2019-05-03 15:07 ` Doug Anderson
2019-05-07 7:05 ` Artur Petrosyan
2019-04-18 0:13 ` [PATCH v2 2/5] USB: Export usb_wakeup_enabled_descendants() Douglas Anderson
2019-04-18 0:13 ` [PATCH v2 3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB Douglas Anderson
2019-04-25 12:40 ` Felipe Balbi
2019-04-25 18:09 ` Doug Anderson
2019-04-25 19:58 ` Doug Anderson
2019-05-02 18:36 ` Doug Anderson
2019-04-30 1:23 ` Rob Herring
2019-04-30 5:25 ` Doug Anderson
2019-04-18 0:13 ` [PATCH v2 4/5] USB: dwc2: Don't turn off the usbphy in suspend if wakeup is enabled Douglas Anderson
2019-04-25 12:41 ` Felipe Balbi
2019-04-18 0:13 ` [PATCH v2 5/5] ARM: dts: rockchip: Allow wakeup from rk3288-veyron's dwc2 USB ports Douglas Anderson
2019-04-18 12:40 ` [PATCH v2 0/5] USB: dwc2: Allow wakeup from suspend; enable for rk3288-veyron Minas Harutyunyan
2019-04-18 15:54 ` Doug Anderson
2019-04-19 11:43 ` Artur Petrosyan
2019-04-19 16:44 ` Artur Petrosyan
2019-04-22 15:50 ` Artur Petrosyan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=SN1PR12MB243136608514210F3E3E536EA7350@SN1PR12MB2431.namprd12.prod.outlook.com \
--to=arthur.petrosyan@synopsys.com \
--cc=Minas.Harutyunyan@synopsys.com \
--cc=amelie.delaunay@st.com \
--cc=amstan@chromium.org \
--cc=ayaka@soulik.info \
--cc=dianders@chromium.org \
--cc=dinguyen@opensource.altera.com \
--cc=felipe.balbi@linux.intel.com \
--cc=gregkh@linuxfoundation.org \
--cc=heiko@sntech.de \
--cc=jwerner@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-rockchip@lists.infradead.org \
--cc=linux-usb@vger.kernel.org \
--cc=mka@chromium.org \
--cc=ryandcase@chromium.org \
--cc=stefan.wahren@i2se.com \
--cc=stern@rowland.harvard.edu \
--cc=william.wu@rock-chips.com \
--cc=zhangqing@rock-chips.com \
--cc=zyw@rock-chips.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).