LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Artur Petrosyan <Arthur.Petrosyan@synopsys.com>
To: Doug Anderson <firstname.lastname@example.org>
Cc: Minas Harutyunyan <Minas.Harutyunyan@synopsys.com>,
Felipe Balbi <email@example.com>,
"Alan Stern" <firstname.lastname@example.org>,
William Wu <email@example.com>,
Stefan Wahren <firstname.lastname@example.org>,
Randy Li <email@example.com>,
Amelie Delaunay <firstname.lastname@example.org>,
"Elaine Zhang" <email@example.com>,
Greg Kroah-Hartman <firstname.lastname@example.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:20:09 +0000 [thread overview]
Message-ID: <SN1PR12MB243156982D5BD74F11680597A7350@SN1PR12MB2431.namprd12.prod.outlook.com> (raw)
On 5/1/2019 05:57, Doug Anderson wrote:
> On Mon, Apr 29, 2019 at 11:06 PM Artur Petrosyan
> <Arthur.Petrosyan@synopsys.com> wrote:
>> On 4/29/2019 21:34, Doug Anderson wrote:
>>> On Mon, Apr 29, 2019 at 1:43 AM Artur Petrosyan
>>> <Arthur.Petrosyan@synopsys.com> wrote:
>>>> 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
>>>>> 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 <email@example.com>
>>>>> This patch was last posted at:
>>>>> ...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
>> 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:
Here you are looking at the old code. After that there are several of
changes related to suspend/resume functions.
This is the link to the code with changes. Latest version of those
Your changes are sitting on that latest version of code. Not the old
version of it.
> 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().
I think you did this tests on the old version of the code
I have tested the flow myself with the mainline Kernel on
"torvalds/master" and not HPRT0_SUSP nor PCGCTL_STOPPCLK are not being set.
So here you need to review those things again.
> [ 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.
next prev parent reply other threads:[~2019-05-03 8:20 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 [this message]
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
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
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:
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--subject='Re: [PATCH v2 1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE' \
* 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).