LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
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

  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 \
    --subject='Re: [PATCH v2 1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE' \
    /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

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