From: Doug Anderson <dianders@chromium.org> To: Artur Petrosyan <Arthur.Petrosyan@synopsys.com> Cc: Minas Harutyunyan <Minas.Harutyunyan@synopsys.com>, Felipe Balbi <felipe.balbi@linux.intel.com>, "heiko@sntech.de" <heiko@sntech.de>, Alan Stern <stern@rowland.harvard.edu>, "amstan@chromium.org" <amstan@chromium.org>, "linux-rockchip@lists.infradead.org" <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>, "zyw@rock-chips.com" <zyw@rock-chips.com>, "mka@chromium.org" <mka@chromium.org>, "ryandcase@chromium.org" <ryandcase@chromium.org>, Amelie Delaunay <amelie.delaunay@st.com>, "jwerner@chromium.org" <jwerner@chromium.org>, "dinguyen@opensource.altera.com" <dinguyen@opensource.altera.com>, Elaine Zhang <zhangqing@rock-chips.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> Subject: [v2,1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE Date: Tue, 30 Apr 2019 18:51:35 -0700 [thread overview] Message-ID: <CAD=FV=UWGOWiCpy5qUY8oFHPyxdYJe7OsdWBDuCNcXBu5BJoGA@mail.gmail.com> (raw) Hi, On Mon, Apr 29, 2019 at 11:06 PM Artur Petrosyan <Arthur.Petrosyan@synopsys.com> wrote: > > Hi, > > On 4/29/2019 21:34, Doug Anderson wrote: > > Hi, > > > > On Mon, Apr 29, 2019 at 1:43 AM Artur Petrosyan > > <Arthur.Petrosyan@synopsys.com> wrote: > >> > >> Hi, > >> > >> On 4/18/2019 04:15, Douglas Anderson 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=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&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. > >> What do you mean by doesn't "just work" ? It seem to me that even after > >> adding this patch you don't get issues fixed. > >> You mention that you don't get the hotplug events. Please provide dwc2 > >> debug logs and register dumps on this issue. > > > > I mean that partial power down in the currently upstream driver > > doesn't work. AKA: if I turn on partial power down in the upstream > > driver then hotplug events break. I can try to provide some logs. On > > what exact version of the code do you want logs? Just your series? > > Just my series? Mainline? Some attempt at combining both series? As > > I said things seem to sorta work with the combined series. I can try > > to clarify if that's the series you want me to test with. ...or I can > > wait for your next version? > As I said this patch doesn't fix the issue with hotplug. With this patch > or without the hotplug behaves as it was. I have tested it on our setup. > > Have you debugged your patch? Does it make any difference on your setup > ? Does it fix the issue with hotplug? I think we're still not taking on the same page. My patch makes no attempt to make partial power down mode work. My patch attempts to make things work a little better when using DWC2_POWER_DOWN_PARAM_NONE. There is no use testing my patch with partial power down as it shouldn't have any impact there. > > I am by no means an expert on dwc2, but an assumption made in my patch > > is that even cores that can't support partial power down can still > > save some amount of power when hcd_suspend is called. > Have you tried to debug dwc2 with power_down == DWC2_POWER_DOWN_PARAM_NONE ? > > > > Some evidence that this should be possible: looking at mainline Linux > > and at dwc2_port_suspend(), I see: > > > > * It is currently called even when we have DWC2_POWER_DOWN_PARAM_NONE > Currently (without your and my patches) (looking at mainline Linux) the > function dwc2_port_suspend() is called anyway because its call is issued > by the system. But it performs entering to suspend only in case of > DWC2_POWER_DOWN_PARAM_PARTIAL. > > This is not an assumption. What I am pointing out is based on debugging > and before making assumptions without debugging for me seems not ok. > > Currently without your patch and without my patches. In the > dwc2_port_suspend() it will enter to suspend only in case that > power_down == DWC2_POWER_DOWN_PARAM_PARTIAL. Because if you look at the > code more carefully you will see > > if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL) > goto skip_power_saving; > > This says if power_down is not DWC2_POWER_DOWN_PARAM_PARTIAL then skip > power saving. > > So but after your patch. If power_down is DWC2_POWER_DOWN_PARAM_NONE it > tries to suspend. We must be looking at different code. I'm looking at Linux's tree, AKA: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/dwc2/hcd.c#n3488 I took a mainline kernel ("v5.1-rc7-5-g83a50840e72a") and added printouts in dwc2_port_suspend() next to where it set HPRT0_SUSP and PCGCTL_STOPPCLK in dwc2_port_suspend(). [ 454.906364] dwc2 ff540000.usb: I'm setting HPRT0_SUSP [ 454.906367] dwc2 ff540000.usb: I'm setting PCGCTL_STOPPCLK ...and just to confirm: # grep '^power' /sys/kernel/debug/*.usb/params /sys/kernel/debug/ff540000.usb/params:power_down : 0 /sys/kernel/debug/ff580000.usb/params:power_down : 0 So I'm really quite convinced that on mainline Linux with DWC2_POWER_DOWN_PARAM_NONE that dwc2_port_suspend() sets HPRT0_SUSP and PCGCTL_STOPPCLK. > > ...I believe that the net effect of my patch ends up doing both those > > same two things in hcd_suspend. That is: when power_down is > > DWC2_POWER_DOWN_PARAM_NONE I believe my patch is really just doing the > > same thing that dwc2_port_suspend() would do in the same case. Is > > that not OK? > No if your patch is doing the same thing as it was doing before what is > the purpose of the patch ? The purpose is to make _dwc2_hcd_suspend() work more correctly in the case where power_down is DWC2_POWER_DOWN_PARAM_NONE > My testes show that your patch doesn't fix the issue related partial > power down. Right. I have been trying to say that my patch doesn't do anything at all for partial power down. I am simply trying to make DWC2_POWER_DOWN_PARAM_NONE work more correctly. I haven't run all the power consumption tests in quite a long time and I'll try to get it hooked up tomorrow to confirm that my patch really truly is still needed to help with power consumption. I did confirm that at least there are cases where _dwc2_hcd_suspend() is called and my patch is what sets the important bits. -Doug
WARNING: multiple messages have this Message-ID (diff)
From: Doug Anderson <dianders@chromium.org> To: Artur Petrosyan <Arthur.Petrosyan@synopsys.com> Cc: Minas Harutyunyan <Minas.Harutyunyan@synopsys.com>, Felipe Balbi <felipe.balbi@linux.intel.com>, "heiko@sntech.de" <heiko@sntech.de>, Alan Stern <stern@rowland.harvard.edu>, "amstan@chromium.org" <amstan@chromium.org>, "linux-rockchip@lists.infradead.org" <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>, "zyw@rock-chips.com" <zyw@rock-chips.com>, "mka@chromium.org" <mka@chromium.org>, "ryandcase@chromium.org" <ryandcase@chromium.org>, Amelie Delaunay <amelie.delaunay@st.com>, "jwerner@chromium.org" <jwerner@chromium.org>, "dinguyen@opensource.altera.com" <dinguyen@opensource.altera.com>, Elaine Zhang <zhangqing@rock-chips.com>, Greg Kroah-Hartman <gregkh@linuxfoundation.org>, "linux-kernel@vger.kernel.org" <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: Tue, 30 Apr 2019 18:51:35 -0700 [thread overview] Message-ID: <CAD=FV=UWGOWiCpy5qUY8oFHPyxdYJe7OsdWBDuCNcXBu5BJoGA@mail.gmail.com> (raw) Message-ID: <20190501015135.-yAyPgSa_5frLtkedmKGxLmH3giFPW7cMiXhkrX6GBM@z> (raw) In-Reply-To: <SN1PR12MB24317F315C5A4DFE70F9CB0DA73A0@SN1PR12MB2431.namprd12.prod.outlook.com> Hi, On Mon, Apr 29, 2019 at 11:06 PM Artur Petrosyan <Arthur.Petrosyan@synopsys.com> wrote: > > Hi, > > On 4/29/2019 21:34, Doug Anderson wrote: > > Hi, > > > > On Mon, Apr 29, 2019 at 1:43 AM Artur Petrosyan > > <Arthur.Petrosyan@synopsys.com> wrote: > >> > >> Hi, > >> > >> On 4/18/2019 04:15, Douglas Anderson 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=DwIDAg&c=DPL6_X_6JkXFx7AXWqB0tg&r=9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqrC_D7niMJI&m=MMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=nExFpAPP_0plZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&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. > >> What do you mean by doesn't "just work" ? It seem to me that even after > >> adding this patch you don't get issues fixed. > >> You mention that you don't get the hotplug events. Please provide dwc2 > >> debug logs and register dumps on this issue. > > > > I mean that partial power down in the currently upstream driver > > doesn't work. AKA: if I turn on partial power down in the upstream > > driver then hotplug events break. I can try to provide some logs. On > > what exact version of the code do you want logs? Just your series? > > Just my series? Mainline? Some attempt at combining both series? As > > I said things seem to sorta work with the combined series. I can try > > to clarify if that's the series you want me to test with. ...or I can > > wait for your next version? > As I said this patch doesn't fix the issue with hotplug. With this patch > or without the hotplug behaves as it was. I have tested it on our setup. > > Have you debugged your patch? Does it make any difference on your setup > ? Does it fix the issue with hotplug? I think we're still not taking on the same page. My patch makes no attempt to make partial power down mode work. My patch attempts to make things work a little better when using DWC2_POWER_DOWN_PARAM_NONE. There is no use testing my patch with partial power down as it shouldn't have any impact there. > > I am by no means an expert on dwc2, but an assumption made in my patch > > is that even cores that can't support partial power down can still > > save some amount of power when hcd_suspend is called. > Have you tried to debug dwc2 with power_down == DWC2_POWER_DOWN_PARAM_NONE ? > > > > Some evidence that this should be possible: looking at mainline Linux > > and at dwc2_port_suspend(), I see: > > > > * It is currently called even when we have DWC2_POWER_DOWN_PARAM_NONE > Currently (without your and my patches) (looking at mainline Linux) the > function dwc2_port_suspend() is called anyway because its call is issued > by the system. But it performs entering to suspend only in case of > DWC2_POWER_DOWN_PARAM_PARTIAL. > > This is not an assumption. What I am pointing out is based on debugging > and before making assumptions without debugging for me seems not ok. > > Currently without your patch and without my patches. In the > dwc2_port_suspend() it will enter to suspend only in case that > power_down == DWC2_POWER_DOWN_PARAM_PARTIAL. Because if you look at the > code more carefully you will see > > if (hsotg->params.power_down != DWC2_POWER_DOWN_PARAM_PARTIAL) > goto skip_power_saving; > > This says if power_down is not DWC2_POWER_DOWN_PARAM_PARTIAL then skip > power saving. > > So but after your patch. If power_down is DWC2_POWER_DOWN_PARAM_NONE it > tries to suspend. We must be looking at different code. I'm looking at Linux's tree, AKA: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/dwc2/hcd.c#n3488 I took a mainline kernel ("v5.1-rc7-5-g83a50840e72a") and added printouts in dwc2_port_suspend() next to where it set HPRT0_SUSP and PCGCTL_STOPPCLK in dwc2_port_suspend(). [ 454.906364] dwc2 ff540000.usb: I'm setting HPRT0_SUSP [ 454.906367] dwc2 ff540000.usb: I'm setting PCGCTL_STOPPCLK ...and just to confirm: # grep '^power' /sys/kernel/debug/*.usb/params /sys/kernel/debug/ff540000.usb/params:power_down : 0 /sys/kernel/debug/ff580000.usb/params:power_down : 0 So I'm really quite convinced that on mainline Linux with DWC2_POWER_DOWN_PARAM_NONE that dwc2_port_suspend() sets HPRT0_SUSP and PCGCTL_STOPPCLK. > > ...I believe that the net effect of my patch ends up doing both those > > same two things in hcd_suspend. That is: when power_down is > > DWC2_POWER_DOWN_PARAM_NONE I believe my patch is really just doing the > > same thing that dwc2_port_suspend() would do in the same case. Is > > that not OK? > No if your patch is doing the same thing as it was doing before what is > the purpose of the patch ? The purpose is to make _dwc2_hcd_suspend() work more correctly in the case where power_down is DWC2_POWER_DOWN_PARAM_NONE > My testes show that your patch doesn't fix the issue related partial > power down. Right. I have been trying to say that my patch doesn't do anything at all for partial power down. I am simply trying to make DWC2_POWER_DOWN_PARAM_NONE work more correctly. I haven't run all the power consumption tests in quite a long time and I'll try to get it hooked up tomorrow to confirm that my patch really truly is still needed to help with power consumption. I did confirm that at least there are cases where _dwc2_hcd_suspend() is called and my patch is what sets the important bits. -Doug
next prev reply other threads:[~2019-05-01 1:51 UTC|newest] 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 ` Doug Anderson [this message] 2019-05-01 1:51 ` 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 ` [v2,1/5] " Doug Anderson 2019-05-01 23:58 ` [PATCH v2 1/5] " 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 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='CAD=FV=UWGOWiCpy5qUY8oFHPyxdYJe7OsdWBDuCNcXBu5BJoGA@mail.gmail.com' \ --to=dianders@chromium.org \ --cc=Arthur.Petrosyan@synopsys.com \ --cc=Minas.Harutyunyan@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=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: linkBe 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).