From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.2 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,SIGNED_OFF_BY,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E4975C43219 for ; Wed, 1 May 2019 01:51:44 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 926C02087B for ; Wed, 1 May 2019 01:51:44 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="j6fmWbqC" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727447AbfEABvn (ORCPT ); Tue, 30 Apr 2019 21:51:43 -0400 Received: from mail-vs1-f66.google.com ([209.85.217.66]:45834 "EHLO mail-vs1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726123AbfEABvm (ORCPT ); Tue, 30 Apr 2019 21:51:42 -0400 Received: by mail-vs1-f66.google.com with SMTP id o10so9157860vsp.12 for ; Tue, 30 Apr 2019 18:51:42 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=06LICDfjCeCtvVUkBORNi2vtZVpAhIUAW1k7qKawkC4=; b=j6fmWbqC7a7BBUo2lPeQez2tJf5Zgp111u3x/f4c4UZKQzbMQLEf3aF5oaD85bXQdb t/dAWXeGctu5hwKcP/L+ocQoEikwxwmjBJ2FdoWCGDFdd0/2zAMxOD6oV/7oR8l9OgYn zCJFYMTm8yPHu7NOGDd9qCKPnJuut694tNiyY= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=06LICDfjCeCtvVUkBORNi2vtZVpAhIUAW1k7qKawkC4=; b=AkAZ7xMI5h8V6eUJm90i/AA1owLAK3IQ6pSwZSubAyszxsl7/d5rYtB+IkmLReuZRV Y211O/I9XnXK7w1B1kCIIpuGp10j+/u/MLVQ1eTEqjG/zrQmkiEdqLoFsGmkVDMH8ldp s97STA+GNg3eJ1taNtOyERMBxs3fZUgFJuJgus7arOclgyCafpWhfm0q0EOe3igXsrav 4iybh8EBVC7GO2sOZV9UTgvyF8QMBdau2KLJOmu1ModMDBs374WK+65KMIFLxA0hAjkD RpvDfiFYXeyidhQaQJAvsb45G7ccdvXf8E6fFUmaGR8rP/BlrTLu0FR5QJQEnmufVxs8 cHPg== X-Gm-Message-State: APjAAAVVLhqc20xqjbtzNks2aU2QmAgo3c+kTkkshkHCKFlUDgwa+LSl s+7z0HJE290eKHN1nWE3qYfoFhTzOvg= X-Google-Smtp-Source: APXvYqysvSzaadfrg4DEsvr8kODh0AJIkIeAI5L31zWe7EOFP9ENk4FXJhpDSjEyhDcwQuhUjRjh0A== X-Received: by 2002:a67:e3d1:: with SMTP id k17mr14234569vsm.86.1556675501409; Tue, 30 Apr 2019 18:51:41 -0700 (PDT) Received: from mail-vs1-f50.google.com (mail-vs1-f50.google.com. [209.85.217.50]) by smtp.gmail.com with ESMTPSA id 2sm44291813vke.27.2019.04.30.18.51.36 for (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Tue, 30 Apr 2019 18:51:37 -0700 (PDT) Received: by mail-vs1-f50.google.com with SMTP id b74so6054974vsd.9 for ; Tue, 30 Apr 2019 18:51:36 -0700 (PDT) X-Received: by 2002:a67:7cd1:: with SMTP id x200mr7091738vsc.144.1556675496267; Tue, 30 Apr 2019 18:51:36 -0700 (PDT) MIME-Version: 1.0 References: <20190418001356.124334-1-dianders@chromium.org> <20190418001356.124334-2-dianders@chromium.org> In-Reply-To: From: Doug Anderson Date: Tue, 30 Apr 2019 18:51:35 -0700 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2 1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE To: Artur Petrosyan Cc: Minas Harutyunyan , Felipe Balbi , "heiko@sntech.de" , Alan Stern , "amstan@chromium.org" , "linux-rockchip@lists.infradead.org" , William Wu , "linux-usb@vger.kernel.org" , Stefan Wahren , Randy Li , "zyw@rock-chips.com" , "mka@chromium.org" , "ryandcase@chromium.org" , Amelie Delaunay , "jwerner@chromium.org" , "dinguyen@opensource.altera.com" , Elaine Zhang , Greg Kroah-Hartman , "linux-kernel@vger.kernel.org" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On Mon, Apr 29, 2019 at 11:06 PM Artur Petrosyan wrote: > > Hi, > > On 4/29/2019 21:34, Doug Anderson wrote: > > Hi, > > > > On Mon, Apr 29, 2019 at 1:43 AM Artur Petrosyan > > 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 > >>> --- > >>> This patch was last posted at: > >>> > >>> https://urldefense.proofpoint.com/v2/url?u=3Dhttps-3A__lkml.kernel.or= g_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d= =3DDwIDAg&c=3DDPL6_X_6JkXFx7AXWqB0tg&r=3D9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqr= C_D7niMJI&m=3DMMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=3DnExFpAPP_0plZ= fO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e=3D > >>> > >>> ...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 th= at > >>> we are power optimized. > >> What do you mean by doesn't "just work" ? It seem to me that even afte= r > >> 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 =3D=3D 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 =3D=3D DWC2_POWER_DOWN_PARAM_PARTIAL. Because if you look at t= he > code more carefully you will see > > if (hsotg->params.power_down !=3D 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/dri= vers/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 From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Subject: [v2,1/5] usb: dwc2: bus suspend/resume for hosts with DWC2_POWER_DOWN_PARAM_NONE From: Doug Anderson Message-Id: Date: Tue, 30 Apr 2019 18:51:35 -0700 To: Artur Petrosyan Cc: Minas Harutyunyan , Felipe Balbi , "heiko@sntech.de" , Alan Stern , "amstan@chromium.org" , "linux-rockchip@lists.infradead.org" , William Wu , "linux-usb@vger.kernel.org" , Stefan Wahren , Randy Li , "zyw@rock-chips.com" , "mka@chromium.org" , "ryandcase@chromium.org" , Amelie Delaunay , "jwerner@chromium.org" , "dinguyen@opensource.altera.com" , Elaine Zhang , Greg Kroah-Hartman , "linux-kernel@vger.kernel.org" List-ID: SGksCgpPbiBNb24sIEFwciAyOSwgMjAxOSBhdCAxMTowNiBQTSBBcnR1ciBQZXRyb3N5YW4KPEFy dGh1ci5QZXRyb3N5YW5Ac3lub3BzeXMuY29tPiB3cm90ZToKPgo+IEhpLAo+Cj4gT24gNC8yOS8y MDE5IDIxOjM0LCBEb3VnIEFuZGVyc29uIHdyb3RlOgo+ID4gSGksCj4gPgo+ID4gT24gTW9uLCBB cHIgMjksIDIwMTkgYXQgMTo0MyBBTSBBcnR1ciBQZXRyb3N5YW4KPiA+IDxBcnRodXIuUGV0cm9z eWFuQHN5bm9wc3lzLmNvbT4gd3JvdGU6Cj4gPj4KPiA+PiBIaSwKPiA+Pgo+ID4+IE9uIDQvMTgv MjAxOSAwNDoxNSwgRG91Z2xhcyBBbmRlcnNvbiB3cm90ZToKPiA+Pj4gVGhpcyBpcyBhbiBhdHRl bXB0IHRvIHJlaGFzaCBjb21taXQgMGNmODg0ZTgxOWUwICgidXNiOiBkd2MyOiBhZGQgYnVzCj4g Pj4+IHN1c3BlbmQvcmVzdW1lIGZvciBkd2MyIikgb24gVG9ULiAgVGhhdCBjb21taXQgd2FzIHJl dmVydGVkIGluIGNvbW1pdAo+ID4+PiBiMGJiOWJiNmNlMDEgKCJSZXZlcnQgInVzYjogZHdjMjog YWRkIGJ1cyBzdXNwZW5kL3Jlc3VtZSBmb3IgZHdjMiIiKQo+ID4+PiBiZWNhdXNlIGFwcGFyZW50 bHkgaXQgYnJva2UgdGhlIEFsdGVyYSBTT0NGUEdBLgo+ID4+Pgo+ID4+PiBXaXRoIGFsbCB0aGUg Y2hhbmdlcyB0aGF0IGhhdmUgaGFwcGVuZWQgdG8gZHdjMiBpbiB0aGUgbWVhbnRpbWUsIGl0J3MK PiA+Pj4gcG9zc2libGUgdGhhdCB0aGUgQWx0ZXJhIFNPQ0ZQR0Egd2lsbCBqdXN0IG1hZ2ljYWxs eSB3b3JrIHdpdGggdGhpcwo+ID4+PiBjaGFuZ2Ugbm93LiAgLi4uYW5kIGl0IHdvdWxkIGJlIGdv b2QgdG8gZ2V0IGJ1cyBzdXNwZW5kL3Jlc3VtZQo+ID4+PiBpbXBsZW1lbnRlZC4KPiA+Pj4KPiA+ Pj4gVGhpcyBjaGFuZ2UgaXMgYSBmb3J3YXJkIHBvcnQgb2Ygb25lIHRoYXQncyBiZWVuIGxpdmlu ZyBpbiB0aGUgQ2hyb21lCj4gPj4+IE9TIDMuMTQga2VybmVsIHRyZWUuCj4gPj4+Cj4gPj4+IFNp Z25lZC1vZmYtYnk6IERvdWdsYXMgQW5kZXJzb24gPGRpYW5kZXJzQGNocm9taXVtLm9yZz4KPiA+ Pj4gLS0tCj4gPj4+IFRoaXMgcGF0Y2ggd2FzIGxhc3QgcG9zdGVkIGF0Ogo+ID4+Pgo+ID4+PiBo dHRwczovL3VybGRlZmVuc2UucHJvb2Zwb2ludC5jb20vdjIvdXJsP3U9aHR0cHMtM0FfX2xrbWwu a2VybmVsLm9yZ19yXzE0NDYyMzcxNzMtMkQxNTI2My0yRDEtMkRnaXQtMkRzZW5kLTJEZW1haWwt MkRkaWFuZGVycy00MGNocm9taXVtLm9yZyZkPUR3SURBZyZjPURQTDZfWF82SmtYRng3QVhXcUIw dGcmcj05aFBCRktDSl9uQmpKaEdWcnJsWU9lT1FqUF9IbFZ6WXFyQ19EN25pTUpJJm09TU1mZS00 bFplUHl0eTZGNXpmUTU0a2lZR3VKV051bHlSYXQ5NDRMa09zYyZzPW5FeEZwQVBQXzBwbFpmTzVM TUcxQi1tcXQxdnlDdkUzNWVsVmN5VmdzOFkmZT0KPiA+Pj4KPiA+Pj4gLi4uYW5kIGFwcGVhcnMg dG8gaGF2ZSBkaWVkIHRoZSBkZWF0aCBvZiBzaWxlbmNlLiAgTWF5YmUgaXQgY291bGQgZ2V0Cj4g Pj4+IHNvbWUgYmFrZSB0aW1lIGluIGxpbnV4bmV4dCBpZiB3ZSBjYW4ndCBmaW5kIGFueSBwcm9h Y3RpdmUgdGVzdGluZz8KPiA+Pj4KPiA+Pj4gSSB3aWxsIGFsc28gZnJlZWx5IGFkbWl0IHRoYXQg SSBkb24ndCBrbm93IHRvbnMgYWJvdXQgdGhlIHRoZW9yeQo+ID4+PiBiZWhpbmQgdGhpcyBwYXRj aC4gIEknbSBtb3N0bHkganVzdCByZS1oYXNoaW5nIHRoZSBvcmlnaW5hbCBjb21taXQKPiA+Pj4g ZnJvbSBLZXZlciB0aGF0IHdhcyByZXZlcnRlZCBzaW5jZToKPiA+Pj4gKiBUdXJuaW5nIG9uIHBh cnRpYWwgcG93ZXIgZG93biBvbiByazMyODggZG9lc24ndCAianVzdCB3b3JrIi4gIEkKPiA+Pj4g ICAgIGRvbid0IGdldCBob3RwbHVnIGV2ZW50cy4gIFRoaXMgaXMgZGVzcGl0ZSBkd2MyIGF1dG8t ZGV0ZWN0aW5nIHRoYXQKPiA+Pj4gICAgIHdlIGFyZSBwb3dlciBvcHRpbWl6ZWQuCj4gPj4gV2hh dCBkbyB5b3UgbWVhbiBieSBkb2Vzbid0ICJqdXN0IHdvcmsiID8gSXQgc2VlbSB0byBtZSB0aGF0 IGV2ZW4gYWZ0ZXIKPiA+PiBhZGRpbmcgdGhpcyBwYXRjaCB5b3UgZG9uJ3QgZ2V0IGlzc3VlcyBm aXhlZC4KPiA+PiBZb3UgbWVudGlvbiB0aGF0IHlvdSBkb24ndCBnZXQgdGhlIGhvdHBsdWcgZXZl bnRzLiBQbGVhc2UgcHJvdmlkZSBkd2MyCj4gPj4gZGVidWcgbG9ncyBhbmQgcmVnaXN0ZXIgZHVt cHMgb24gdGhpcyBpc3N1ZS4KPiA+Cj4gPiBJIG1lYW4gdGhhdCBwYXJ0aWFsIHBvd2VyIGRvd24g aW4gdGhlIGN1cnJlbnRseSB1cHN0cmVhbSBkcml2ZXIKPiA+IGRvZXNuJ3Qgd29yay4gIEFLQTog aWYgSSB0dXJuIG9uIHBhcnRpYWwgcG93ZXIgZG93biBpbiB0aGUgdXBzdHJlYW0KPiA+IGRyaXZl ciB0aGVuIGhvdHBsdWcgZXZlbnRzIGJyZWFrLiAgSSBjYW4gdHJ5IHRvIHByb3ZpZGUgc29tZSBs b2dzLiAgT24KPiA+IHdoYXQgZXhhY3QgdmVyc2lvbiBvZiB0aGUgY29kZSBkbyB5b3Ugd2FudCBs b2dzPyAgSnVzdCB5b3VyIHNlcmllcz8KPiA+IEp1c3QgbXkgc2VyaWVzPyAgTWFpbmxpbmU/ICBT b21lIGF0dGVtcHQgYXQgY29tYmluaW5nIGJvdGggc2VyaWVzPyAgQXMKPiA+IEkgc2FpZCB0aGlu Z3Mgc2VlbSB0byBzb3J0YSB3b3JrIHdpdGggdGhlIGNvbWJpbmVkIHNlcmllcy4gIEkgY2FuIHRy eQo+ID4gdG8gY2xhcmlmeSBpZiB0aGF0J3MgdGhlIHNlcmllcyB5b3Ugd2FudCBtZSB0byB0ZXN0 IHdpdGguICAuLi5vciBJIGNhbgo+ID4gd2FpdCBmb3IgeW91ciBuZXh0IHZlcnNpb24/Cj4gQXMg SSBzYWlkIHRoaXMgcGF0Y2ggZG9lc24ndCBmaXggdGhlIGlzc3VlIHdpdGggaG90cGx1Zy4gV2l0 aCB0aGlzIHBhdGNoCj4gb3Igd2l0aG91dCB0aGUgaG90cGx1ZyBiZWhhdmVzIGFzIGl0IHdhcy4g SSBoYXZlIHRlc3RlZCBpdCBvbiBvdXIgc2V0dXAuCj4KPiBIYXZlIHlvdSBkZWJ1Z2dlZCB5b3Vy IHBhdGNoPyBEb2VzIGl0IG1ha2UgYW55IGRpZmZlcmVuY2Ugb24geW91ciBzZXR1cAo+ID8gRG9l cyBpdCBmaXggdGhlIGlzc3VlIHdpdGggaG90cGx1Zz8KCkkgdGhpbmsgd2UncmUgc3RpbGwgbm90 IHRha2luZyBvbiB0aGUgc2FtZSBwYWdlLgoKTXkgcGF0Y2ggbWFrZXMgbm8gYXR0ZW1wdCB0byBt YWtlIHBhcnRpYWwgcG93ZXIgZG93biBtb2RlIHdvcmsuICBNeQpwYXRjaCBhdHRlbXB0cyB0byBt YWtlIHRoaW5ncyB3b3JrIGEgbGl0dGxlIGJldHRlciB3aGVuIHVzaW5nCkRXQzJfUE9XRVJfRE9X Tl9QQVJBTV9OT05FLiAgVGhlcmUgaXMgbm8gdXNlIHRlc3RpbmcgbXkgcGF0Y2ggd2l0aApwYXJ0 aWFsIHBvd2VyIGRvd24gYXMgaXQgc2hvdWxkbid0IGhhdmUgYW55IGltcGFjdCB0aGVyZS4KCgo+ ID4gSSBhbSBieSBubyBtZWFucyBhbiBleHBlcnQgb24gZHdjMiwgYnV0IGFuIGFzc3VtcHRpb24g bWFkZSBpbiBteSBwYXRjaAo+ID4gaXMgdGhhdCBldmVuIGNvcmVzIHRoYXQgY2FuJ3Qgc3VwcG9y dCBwYXJ0aWFsIHBvd2VyIGRvd24gY2FuIHN0aWxsCj4gPiBzYXZlIHNvbWUgYW1vdW50IG9mIHBv d2VyIHdoZW4gaGNkX3N1c3BlbmQgaXMgY2FsbGVkLgo+IEhhdmUgeW91IHRyaWVkIHRvIGRlYnVn IGR3YzIgd2l0aCBwb3dlcl9kb3duID09IERXQzJfUE9XRVJfRE9XTl9QQVJBTV9OT05FID8KPiA+ Cj4gPiBTb21lIGV2aWRlbmNlIHRoYXQgdGhpcyBzaG91bGQgYmUgcG9zc2libGU6IGxvb2tpbmcg YXQgbWFpbmxpbmUgTGludXgKPiA+IGFuZCBhdCBkd2MyX3BvcnRfc3VzcGVuZCgpLCBJIHNlZToK PiA+Cj4gPiAqIEl0IGlzIGN1cnJlbnRseSBjYWxsZWQgZXZlbiB3aGVuIHdlIGhhdmUgRFdDMl9Q T1dFUl9ET1dOX1BBUkFNX05PTkUKPiBDdXJyZW50bHkgKHdpdGhvdXQgeW91ciBhbmQgbXkgcGF0 Y2hlcykgKGxvb2tpbmcgYXQgbWFpbmxpbmUgTGludXgpIHRoZQo+IGZ1bmN0aW9uIGR3YzJfcG9y dF9zdXNwZW5kKCkgaXMgY2FsbGVkIGFueXdheSBiZWNhdXNlIGl0cyBjYWxsIGlzIGlzc3VlZAo+ IGJ5IHRoZSBzeXN0ZW0uIEJ1dCBpdCBwZXJmb3JtcyBlbnRlcmluZyB0byBzdXNwZW5kIG9ubHkg aW4gY2FzZSBvZgo+IERXQzJfUE9XRVJfRE9XTl9QQVJBTV9QQVJUSUFMLgo+Cj4gVGhpcyBpcyBu b3QgYW4gYXNzdW1wdGlvbi4gV2hhdCBJIGFtIHBvaW50aW5nIG91dCBpcyBiYXNlZCBvbiBkZWJ1 Z2dpbmcKPiBhbmQgYmVmb3JlIG1ha2luZyBhc3N1bXB0aW9ucyB3aXRob3V0IGRlYnVnZ2luZyBm b3IgbWUgc2VlbXMgbm90IG9rLgo+Cj4gQ3VycmVudGx5IHdpdGhvdXQgeW91ciBwYXRjaCBhbmQg d2l0aG91dCBteSBwYXRjaGVzLiBJbiB0aGUKPiBkd2MyX3BvcnRfc3VzcGVuZCgpIGl0IHdpbGwg ZW50ZXIgdG8gc3VzcGVuZCBvbmx5IGluIGNhc2UgdGhhdAo+IHBvd2VyX2Rvd24gPT0gRFdDMl9Q T1dFUl9ET1dOX1BBUkFNX1BBUlRJQUwuIEJlY2F1c2UgaWYgeW91IGxvb2sgYXQgdGhlCj4gY29k ZSBtb3JlIGNhcmVmdWxseSB5b3Ugd2lsbCBzZWUKPgo+ICAgICAgICAgaWYgKGhzb3RnLT5wYXJh bXMucG93ZXJfZG93biAhPSBEV0MyX1BPV0VSX0RPV05fUEFSQU1fUEFSVElBTCkKPiAgICAgICAg ICAgICAgICAgZ290byBza2lwX3Bvd2VyX3NhdmluZzsKPgo+IFRoaXMgc2F5cyBpZiBwb3dlcl9k b3duIGlzIG5vdCBEV0MyX1BPV0VSX0RPV05fUEFSQU1fUEFSVElBTCB0aGVuIHNraXAKPiBwb3dl ciBzYXZpbmcuCj4KPiBTbyBidXQgYWZ0ZXIgeW91ciBwYXRjaC4gSWYgcG93ZXJfZG93biBpcyBE V0MyX1BPV0VSX0RPV05fUEFSQU1fTk9ORSBpdAo+IHRyaWVzIHRvIHN1c3BlbmQuCgpXZSBtdXN0 IGJlIGxvb2tpbmcgYXQgZGlmZmVyZW50IGNvZGUuICBJJ20gbG9va2luZyBhdCBMaW51eCdzIHRy ZWUsIEFLQToKCmh0dHBzOi8vZ2l0Lmtlcm5lbC5vcmcvcHViL3NjbS9saW51eC9rZXJuZWwvZ2l0 L3RvcnZhbGRzL2xpbnV4LmdpdC90cmVlL2RyaXZlcnMvdXNiL2R3YzIvaGNkLmMjbjM0ODgKCkkg dG9vayBhIG1haW5saW5lIGtlcm5lbCAoInY1LjEtcmM3LTUtZzgzYTUwODQwZTcyYSIpIGFuZCBh ZGRlZApwcmludG91dHMgaW4gZHdjMl9wb3J0X3N1c3BlbmQoKSBuZXh0IHRvIHdoZXJlIGl0IHNl dCBIUFJUMF9TVVNQIGFuZApQQ0dDVExfU1RPUFBDTEsgaW4gZHdjMl9wb3J0X3N1c3BlbmQoKS4K ClsgIDQ1NC45MDYzNjRdIGR3YzIgZmY1NDAwMDAudXNiOiBJJ20gc2V0dGluZyBIUFJUMF9TVVNQ ClsgIDQ1NC45MDYzNjddIGR3YzIgZmY1NDAwMDAudXNiOiBJJ20gc2V0dGluZyBQQ0dDVExfU1RP UFBDTEsKCi4uLmFuZCBqdXN0IHRvIGNvbmZpcm06CgojIGdyZXAgJ15wb3dlcicgL3N5cy9rZXJu ZWwvZGVidWcvKi51c2IvcGFyYW1zCi9zeXMva2VybmVsL2RlYnVnL2ZmNTQwMDAwLnVzYi9wYXJh bXM6cG93ZXJfZG93biAgICAgICAgICAgICAgICAgICAgOiAwCi9zeXMva2VybmVsL2RlYnVnL2Zm NTgwMDAwLnVzYi9wYXJhbXM6cG93ZXJfZG93biAgICAgICAgICAgICAgICAgICAgOiAwCgpTbyBJ J20gcmVhbGx5IHF1aXRlIGNvbnZpbmNlZCB0aGF0IG9uIG1haW5saW5lIExpbnV4IHdpdGgKRFdD Ml9QT1dFUl9ET1dOX1BBUkFNX05PTkUgdGhhdCBkd2MyX3BvcnRfc3VzcGVuZCgpIHNldHMgSFBS VDBfU1VTUAphbmQgUENHQ1RMX1NUT1BQQ0xLLgoKCj4gPiAuLi5JIGJlbGlldmUgdGhhdCB0aGUg bmV0IGVmZmVjdCBvZiBteSBwYXRjaCBlbmRzIHVwIGRvaW5nIGJvdGggdGhvc2UKPiA+IHNhbWUg dHdvIHRoaW5ncyBpbiBoY2Rfc3VzcGVuZC4gIFRoYXQgaXM6IHdoZW4gcG93ZXJfZG93biBpcwo+ ID4gRFdDMl9QT1dFUl9ET1dOX1BBUkFNX05PTkUgSSBiZWxpZXZlIG15IHBhdGNoIGlzIHJlYWxs eSBqdXN0IGRvaW5nIHRoZQo+ID4gc2FtZSB0aGluZyB0aGF0IGR3YzJfcG9ydF9zdXNwZW5kKCkg d291bGQgZG8gaW4gdGhlIHNhbWUgY2FzZS4gIElzCj4gPiB0aGF0IG5vdCBPSz8KPiBObyBpZiB5 b3VyIHBhdGNoIGlzIGRvaW5nIHRoZSBzYW1lIHRoaW5nIGFzIGl0IHdhcyBkb2luZyBiZWZvcmUg d2hhdCBpcwo+IHRoZSBwdXJwb3NlIG9mIHRoZSBwYXRjaCA/CgpUaGUgcHVycG9zZSBpcyB0byBt YWtlIF9kd2MyX2hjZF9zdXNwZW5kKCkgd29yayBtb3JlIGNvcnJlY3RseSBpbiB0aGUKY2FzZSB3 aGVyZSBwb3dlcl9kb3duIGlzIERXQzJfUE9XRVJfRE9XTl9QQVJBTV9OT05FCgoKPiBNeSB0ZXN0 ZXMgc2hvdyB0aGF0IHlvdXIgcGF0Y2ggZG9lc24ndCBmaXggdGhlIGlzc3VlIHJlbGF0ZWQgcGFy dGlhbAo+IHBvd2VyIGRvd24uCgpSaWdodC4gIEkgaGF2ZSBiZWVuIHRyeWluZyB0byBzYXkgdGhh dCBteSBwYXRjaCBkb2Vzbid0IGRvIGFueXRoaW5nIGF0CmFsbCBmb3IgcGFydGlhbCBwb3dlciBk b3duLiAgSSBhbSBzaW1wbHkgdHJ5aW5nIHRvIG1ha2UKRFdDMl9QT1dFUl9ET1dOX1BBUkFNX05P TkUgd29yayBtb3JlIGNvcnJlY3RseS4KCkkgaGF2ZW4ndCBydW4gYWxsIHRoZSBwb3dlciBjb25z dW1wdGlvbiB0ZXN0cyBpbiBxdWl0ZSBhIGxvbmcgdGltZSBhbmQKSSdsbCB0cnkgdG8gZ2V0IGl0 IGhvb2tlZCB1cCB0b21vcnJvdyB0byBjb25maXJtIHRoYXQgbXkgcGF0Y2ggcmVhbGx5CnRydWx5 IGlzIHN0aWxsIG5lZWRlZCB0byBoZWxwIHdpdGggcG93ZXIgY29uc3VtcHRpb24uICBJIGRpZCBj b25maXJtCnRoYXQgYXQgbGVhc3QgdGhlcmUgYXJlIGNhc2VzIHdoZXJlIF9kd2MyX2hjZF9zdXNw ZW5kKCkgaXMgY2FsbGVkIGFuZApteSBwYXRjaCBpcyB3aGF0IHNldHMgdGhlIGltcG9ydGFudCBi aXRzLgoKLURvdWcK From mboxrd@z Thu Jan 1 00:00:00 1970 From: Doug Anderson 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 Message-ID: References: <20190418001356.124334-1-dianders@chromium.org> <20190418001356.124334-2-dianders@chromium.org> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org To: Artur Petrosyan Cc: Minas Harutyunyan , Felipe Balbi , "heiko@sntech.de" , Alan Stern , "amstan@chromium.org" , "linux-rockchip@lists.infradead.org" , William Wu , "linux-usb@vger.kernel.org" , Stefan Wahren , Randy Li , "zyw@rock-chips.com" , "mka@chromium.org" , "ryandcase@chromium.org" , Amelie Delaunay , "jwerner@chromium.org" , "dinguyen@opensource.altera.com" , Elaine List-Id: linux-rockchip.vger.kernel.org Hi, On Mon, Apr 29, 2019 at 11:06 PM Artur Petrosyan wrote: > > Hi, > > On 4/29/2019 21:34, Doug Anderson wrote: > > Hi, > > > > On Mon, Apr 29, 2019 at 1:43 AM Artur Petrosyan > > 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 > >>> --- > >>> This patch was last posted at: > >>> > >>> https://urldefense.proofpoint.com/v2/url?u=3Dhttps-3A__lkml.kernel.or= g_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org&d= =3DDwIDAg&c=3DDPL6_X_6JkXFx7AXWqB0tg&r=3D9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzYqr= C_D7niMJI&m=3DMMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=3DnExFpAPP_0plZ= fO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e=3D > >>> > >>> ...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 th= at > >>> we are power optimized. > >> What do you mean by doesn't "just work" ? It seem to me that even afte= r > >> 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 =3D=3D 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 =3D=3D DWC2_POWER_DOWN_PARAM_PARTIAL. Because if you look at t= he > code more carefully you will see > > if (hsotg->params.power_down !=3D 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/dri= vers/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