Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
WARNING: multiple messages refer to this Message-ID
From: Doug Anderson <dianders@chromium.org>
To: "Minas Harutyunyan" <hminas@synopsys.com>,
	"Felipe Balbi" <felipe.balbi@linux.intel.com>,
	"Heiko Stübner" <heiko@sntech.de>,
	"Artur Petrosyan" <Arthur.Petrosyan@synopsys.com>
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, 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: [v2,1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE
Date: Wed, 1 May 2019 16:58:21 -0700
Message-ID: <CAD=FV=UGjQz9Di=NL_r_g1Hofqv-FWBywfSm9Vu6gGr22wzPrA@mail.gmail.com> (raw)

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://lkml.kernel.org/r/1446237173-15263-1-git-send-email-dianders@chromium.org
>
> ...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://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/306265/

...where I said that this fixes a resume speed regression.  More
details could be found at the linked bug, AKA:

https://bugs.chromium.org/p/chromium/issues/detail?id=548336

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

From: Doug Anderson <dianders@chromium.org>
To: "Minas Harutyunyan" <hminas@synopsys.com>,
	"Felipe Balbi" <felipe.balbi@linux.intel.com>,
	"Heiko Stübner" <heiko@sntech.de>,
	"Artur Petrosyan" <Arthur.Petrosyan@synopsys.com>
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, 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: Wed, 1 May 2019 16:58:21 -0700
Message-ID: <CAD=FV=UGjQz9Di=NL_r_g1Hofqv-FWBywfSm9Vu6gGr22wzPrA@mail.gmail.com> (raw)
Message-ID: <20190501235821.n6RsPHuez3w9ivajtM4XKrL99Zdlb0skTMcQhtcL1hc@z> (raw)
In-Reply-To: <20190418001356.124334-2-dianders@chromium.org>

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://lkml.kernel.org/r/1446237173-15263-1-git-send-email-dianders@chromium.org
>
> ...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://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/306265/

...where I said that this fixes a resume speed regression.  More
details could be found at the linked bug, AKA:

https://bugs.chromium.org/p/chromium/issues/detail?id=548336

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

         reply index

Thread overview: 50+ 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 ` [v2,1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE Doug Anderson
2019-04-18  0:13   ` [PATCH v2 1/5] " Douglas Anderson
2019-04-29  8:43   ` [v2,1/5] " Artur Petrosyan
2019-04-29  8:43     ` [PATCH v2 1/5] " Artur Petrosyan
2019-04-29 17:33     ` [v2,1/5] " Doug Anderson
2019-04-29 17:33       ` [PATCH v2 1/5] " Doug Anderson
2019-04-30  6:05       ` [v2,1/5] " Artur Petrosyan
2019-04-30  6:05         ` [PATCH v2 1/5] " Artur Petrosyan
2019-05-01  1:51         ` [v2,1/5] " Doug Anderson
2019-05-01  1:51           ` [PATCH v2 1/5] " Doug Anderson
2019-05-03  8:20           ` [v2,1/5] " Artur Petrosyan
2019-05-03  8:20             ` [PATCH v2 1/5] " Artur Petrosyan
2019-05-03 15:03             ` [v2,1/5] " Doug Anderson
2019-05-03 15:03               ` [PATCH v2 1/5] " Doug Anderson
2019-05-07  7:26               ` Artur Petrosyan
2019-05-01 23:58   ` Doug Anderson [this message]
2019-05-01 23:58     ` Doug Anderson
2019-05-03  8:25     ` [v2,1/5] " Artur Petrosyan
2019-05-03  8:25       ` [PATCH v2 1/5] " Artur Petrosyan
2019-05-03 15:07       ` [v2,1/5] " Doug Anderson
2019-05-03 15:07         ` [PATCH v2 1/5] " Doug Anderson
2019-05-07  7:05         ` Artur Petrosyan
2019-04-18  0:13 ` [v2,2/5] USB: Export usb_wakeup_enabled_descendants() Doug Anderson
2019-04-18  0:13   ` [PATCH v2 2/5] " Douglas Anderson
2019-04-18  0:13 ` [v2,3/5] Documentation: dt-bindings: Add snps,need-phy-for-wake for dwc2 USB Doug Anderson
2019-04-18  0:13   ` [PATCH v2 3/5] " Douglas Anderson
2019-04-25 12:40   ` [v2,3/5] " Felipe Balbi
2019-04-25 12:40     ` [PATCH v2 3/5] " Felipe Balbi
2019-04-25 18:09     ` [v2,3/5] " Doug Anderson
2019-04-25 18:09       ` [PATCH v2 3/5] " Doug Anderson
2019-04-25 19:58       ` [v2,3/5] " Doug Anderson
2019-04-25 19:58         ` [PATCH v2 3/5] " Doug Anderson
2019-05-02 18:36     ` [v2,3/5] " Doug Anderson
2019-05-02 18:36       ` [PATCH v2 3/5] " Doug Anderson
2019-04-30  1:23   ` [v2,3/5] " Rob Herring
2019-04-30  1:23     ` [PATCH v2 3/5] " Rob Herring
2019-04-30  5:25     ` [v2,3/5] " Doug Anderson
2019-04-30  5:25       ` [PATCH v2 3/5] " Doug Anderson
2019-04-18  0:13 ` [v2,4/5] USB: dwc2: Don't turn off the usbphy in suspend if wakeup is enabled Doug Anderson
2019-04-18  0:13   ` [PATCH v2 4/5] " Douglas Anderson
2019-04-25 12:41   ` [v2,4/5] " Felipe Balbi
2019-04-25 12:41     ` [PATCH v2 4/5] " Felipe Balbi
2019-04-18  0:13 ` [v2,5/5] ARM: dts: rockchip: Allow wakeup from rk3288-veyron's dwc2 USB ports Doug Anderson
2019-04-18  0:13   ` [PATCH v2 5/5] " 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 publically 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='CAD=FV=UGjQz9Di=NL_r_g1Hofqv-FWBywfSm9Vu6gGr22wzPrA@mail.gmail.com' \
    --to=dianders@chromium.org \
    --cc=Arthur.Petrosyan@synopsys.com \
    --cc=amelie.delaunay@st.com \
    --cc=amstan@chromium.org \
    --cc=ayaka@soulik.info \
    --cc=dinguyen@opensource.altera.com \
    --cc=felipe.balbi@linux.intel.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=heiko@sntech.de \
    --cc=hminas@synopsys.com \
    --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

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org linux-usb@archiver.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/ public-inbox