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.1 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,SIGNED_OFF_BY,SPF_PASS,T_DKIMWL_WL_HIGH 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 AD536C43219 for ; Fri, 3 May 2019 15:03:48 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 6FFF62070B for ; Fri, 3 May 2019 15:03:48 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="Mx0fWWvV" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728175AbfECPDr (ORCPT ); Fri, 3 May 2019 11:03:47 -0400 Received: from mail-vs1-f65.google.com ([209.85.217.65]:34820 "EHLO mail-vs1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727586AbfECPDr (ORCPT ); Fri, 3 May 2019 11:03:47 -0400 Received: by mail-vs1-f65.google.com with SMTP id r23so1624950vsq.2 for ; Fri, 03 May 2019 08:03:46 -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=/2zB92Wp7OJKMoHTQAGZBoeHpTeZv+wAfkk5Wp8/82I=; b=Mx0fWWvVhpczsqJaUFU1eRmw+T3uPX1WqlKj2E7W8b/n99kkVbfP9JspiOpaZLUVky in5uLVHMBmxQRyX50+F0j6bFQTxfVZ/jJoVP2mwPVT0u36RZoq28wxcG52d6z1fhpbD4 Ow0i4UBsPq0npH8UFycS+tjrjwkRFr5GtuViw= 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=/2zB92Wp7OJKMoHTQAGZBoeHpTeZv+wAfkk5Wp8/82I=; b=gqyDQmQr0ZOP5Gq1iNCbuSduBOTbxLhrLYhldNs/pesemgFs9xthb+DM4GAF4z5sDN /LQVTscYYduXBZUH24PnySn8Kwtub7mWymrLfReKf2RhLZ94uxki9ELsLGp3wLHKKTGZ AqR+oy+9fG9+38KX9qmedH5PJg2taors3zSn9u1ypCzNa+/t87338mJ0xidfa+T1/FW8 y2ESSXOMuSUGx2TOTHfrQUzAj55kLF08/wj9kXrGJdJxpDR4qjrxsqntmOOC9sPlHvrg CxSU3BySCBbQZAcnTqGTouSMXFdjUAaBEIjyMFLSApKq+j7zCXamUs23cr8labSXkluy 4egw== X-Gm-Message-State: APjAAAUCCPF70Us/cJ9ny8pj5e7m/K6Wk9vaA0LapAc24G8MwK128ubQ 6eQTniQtLYqBliczyQtWM2LZqU1G30g= X-Google-Smtp-Source: APXvYqwOjb2Xu292XQ69owbM2tf26N39M+roXIYVjHaIGZXe2rokVQIoOg38ih96CbBl9lOAIGMOhg== X-Received: by 2002:a67:7f85:: with SMTP id a127mr5727777vsd.141.1556895823498; Fri, 03 May 2019 08:03:43 -0700 (PDT) Received: from mail-vs1-f44.google.com (mail-vs1-f44.google.com. [209.85.217.44]) by smtp.gmail.com with ESMTPSA id c192sm1735121vka.10.2019.05.03.08.03.37 for (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Fri, 03 May 2019 08:03:39 -0700 (PDT) Received: by mail-vs1-f44.google.com with SMTP id g187so3777484vsc.8 for ; Fri, 03 May 2019 08:03:37 -0700 (PDT) X-Received: by 2002:a67:d29e:: with SMTP id z30mr5820475vsi.111.1556895816252; Fri, 03 May 2019 08:03: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: Fri, 3 May 2019 08:03:24 -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 Fri, May 3, 2019 at 1:20 AM Artur Petrosyan wrote: > > On 5/1/2019 05:57, Doug Anderson wrote: > > 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 b= us > >>>>> suspend/resume for dwc2") on ToT. That commit was reverted in comm= it > >>>>> 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 Chro= me > >>>>> 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.= org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org= &d=3DDwIDAg&c=3DDPL6_X_6JkXFx7AXWqB0tg&r=3D9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzY= qrC_D7niMJI&m=3DMMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=3DnExFpAPP_0p= lZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e=3D > >>>>> > >>>>> ...and appears to have died the death of silence. Maybe it could g= et > >>>>> 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 af= ter > >>>> adding this patch you don't get issues fixed. > >>>> You mention that you don't get the hotplug events. Please provide dw= c2 > >>>> 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. O= n > >>> what exact version of the code do you want logs? Just your series? > >>> Just my series? Mainline? Some attempt at combining both series? A= s > >>> 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 ca= n > >>> wait for your next version? > >> As I said this patch doesn't fix the issue with hotplug. With this pat= ch > >> or without the hotplug behaves as it was. I have tested it on our setu= p. > >> > >> Have you debugged your patch? Does it make any difference on your setu= p > >> ? 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 patc= h > >>> 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_PA= RAM_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) th= e > >> function dwc2_port_suspend() is called anyway because its call is issu= ed > >> 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 debuggin= g > >> 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 a= t the > >> code more carefully you will see > >> > >> if (hsotg->params.power_down !=3D DWC2_POWER_DOWN_PARAM_PARTI= AL) > >> 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 i= t > >> tries to suspend. > > > > We must be looking at different code. I'm looking at Linux's tree, AKA= : > > > > https://urldefense.proofpoint.com/v2/url?u=3Dhttps-3A__git.kernel.org_p= ub_scm_linux_kernel_git_torvalds_linux.git_tree_drivers_usb_dwc2_hcd.c-23n3= 488&d=3DDwIFaQ&c=3DDPL6_X_6JkXFx7AXWqB0tg&r=3D9hPBFKCJ_nBjJhGVrrlYOeOQjP_Hl= VzYqrC_D7niMJI&m=3DIWkDOOGTr0q-H1piDv2KOZe_Hnrz18g6rXFx-DsTuv4&s=3DAHu2iOKk= ybliRGtIfN7cF5p070UdvUKTYJsyAKYojis&e=3D > Here you are looking at the old code. After that there are several of > changes related to suspend/resume functions. In my email, see that I said I actually checked out mainline kernel (and I gave you the exact version: "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 The version "v5.1-rc7-5-g83a50840e72a" is not old code. > This is the link to the code with changes. Latest version of those > functions. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/d= rivers/usb/dwc2/hcd.c#n4489 > > Your changes are sitting on that latest version of code. Not the old > version of it. You are pointing me at _dwc2_hcd_suspend() whereas I pointed at dwc2_port_suspend(). Why? I am saying that dwc2_port_suspend() _does_ set "HPRT0_SUSP" and "PCGCTL_STOPPCLK" even with DWC2_POWER_DOWN_PARAM_NONE. Do you disagree? I completely agree that on mainline _dwc2_hcd_suspend() _does not_ set these bits with DWC2_POWER_DOWN_PARAM_NONE. That is what my patch fixes. -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: Fri, 3 May 2019 08:03:24 -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: SGksCgpPbiBGcmksIE1heSAzLCAyMDE5IGF0IDE6MjAgQU0gQXJ0dXIgUGV0cm9zeWFuCjxBcnRo dXIuUGV0cm9zeWFuQHN5bm9wc3lzLmNvbT4gd3JvdGU6Cj4KPiBPbiA1LzEvMjAxOSAwNTo1Nywg RG91ZyBBbmRlcnNvbiB3cm90ZToKPiA+IEhpLAo+ID4KPiA+IE9uIE1vbiwgQXByIDI5LCAyMDE5 IGF0IDExOjA2IFBNIEFydHVyIFBldHJvc3lhbgo+ID4gPEFydGh1ci5QZXRyb3N5YW5Ac3lub3Bz eXMuY29tPiB3cm90ZToKPiA+Pgo+ID4+IEhpLAo+ID4+Cj4gPj4gT24gNC8yOS8yMDE5IDIxOjM0 LCBEb3VnIEFuZGVyc29uIHdyb3RlOgo+ID4+PiBIaSwKPiA+Pj4KPiA+Pj4gT24gTW9uLCBBcHIg MjksIDIwMTkgYXQgMTo0MyBBTSBBcnR1ciBQZXRyb3N5YW4KPiA+Pj4gPEFydGh1ci5QZXRyb3N5 YW5Ac3lub3BzeXMuY29tPiB3cm90ZToKPiA+Pj4+Cj4gPj4+PiBIaSwKPiA+Pj4+Cj4gPj4+PiBP biA0LzE4LzIwMTkgMDQ6MTUsIERvdWdsYXMgQW5kZXJzb24gd3JvdGU6Cj4gPj4+Pj4gVGhpcyBp cyBhbiBhdHRlbXB0IHRvIHJlaGFzaCBjb21taXQgMGNmODg0ZTgxOWUwICgidXNiOiBkd2MyOiBh ZGQgYnVzCj4gPj4+Pj4gc3VzcGVuZC9yZXN1bWUgZm9yIGR3YzIiKSBvbiBUb1QuICBUaGF0IGNv bW1pdCB3YXMgcmV2ZXJ0ZWQgaW4gY29tbWl0Cj4gPj4+Pj4gYjBiYjliYjZjZTAxICgiUmV2ZXJ0 ICJ1c2I6IGR3YzI6IGFkZCBidXMgc3VzcGVuZC9yZXN1bWUgZm9yIGR3YzIiIikKPiA+Pj4+PiBi ZWNhdXNlIGFwcGFyZW50bHkgaXQgYnJva2UgdGhlIEFsdGVyYSBTT0NGUEdBLgo+ID4+Pj4+Cj4g Pj4+Pj4gV2l0aCBhbGwgdGhlIGNoYW5nZXMgdGhhdCBoYXZlIGhhcHBlbmVkIHRvIGR3YzIgaW4g dGhlIG1lYW50aW1lLCBpdCdzCj4gPj4+Pj4gcG9zc2libGUgdGhhdCB0aGUgQWx0ZXJhIFNPQ0ZQ R0Egd2lsbCBqdXN0IG1hZ2ljYWxseSB3b3JrIHdpdGggdGhpcwo+ID4+Pj4+IGNoYW5nZSBub3cu ICAuLi5hbmQgaXQgd291bGQgYmUgZ29vZCB0byBnZXQgYnVzIHN1c3BlbmQvcmVzdW1lCj4gPj4+ Pj4gaW1wbGVtZW50ZWQuCj4gPj4+Pj4KPiA+Pj4+PiBUaGlzIGNoYW5nZSBpcyBhIGZvcndhcmQg cG9ydCBvZiBvbmUgdGhhdCdzIGJlZW4gbGl2aW5nIGluIHRoZSBDaHJvbWUKPiA+Pj4+PiBPUyAz LjE0IGtlcm5lbCB0cmVlLgo+ID4+Pj4+Cj4gPj4+Pj4gU2lnbmVkLW9mZi1ieTogRG91Z2xhcyBB bmRlcnNvbiA8ZGlhbmRlcnNAY2hyb21pdW0ub3JnPgo+ID4+Pj4+IC0tLQo+ID4+Pj4+IFRoaXMg cGF0Y2ggd2FzIGxhc3QgcG9zdGVkIGF0Ogo+ID4+Pj4+Cj4gPj4+Pj4gaHR0cHM6Ly91cmxkZWZl bnNlLnByb29mcG9pbnQuY29tL3YyL3VybD91PWh0dHBzLTNBX19sa21sLmtlcm5lbC5vcmdfcl8x NDQ2MjM3MTczLTJEMTUyNjMtMkQxLTJEZ2l0LTJEc2VuZC0yRGVtYWlsLTJEZGlhbmRlcnMtNDBj aHJvbWl1bS5vcmcmZD1Ed0lEQWcmYz1EUEw2X1hfNkprWEZ4N0FYV3FCMHRnJnI9OWhQQkZLQ0pf bkJqSmhHVnJybFlPZU9RalBfSGxWellxckNfRDduaU1KSSZtPU1NZmUtNGxaZVB5dHk2RjV6ZlE1 NGtpWUd1SldOdWx5UmF0OTQ0TGtPc2Mmcz1uRXhGcEFQUF8wcGxaZk81TE1HMUItbXF0MXZ5Q3ZF MzVlbFZjeVZnczhZJmU9Cj4gPj4+Pj4KPiA+Pj4+PiAuLi5hbmQgYXBwZWFycyB0byBoYXZlIGRp ZWQgdGhlIGRlYXRoIG9mIHNpbGVuY2UuICBNYXliZSBpdCBjb3VsZCBnZXQKPiA+Pj4+PiBzb21l IGJha2UgdGltZSBpbiBsaW51eG5leHQgaWYgd2UgY2FuJ3QgZmluZCBhbnkgcHJvYWN0aXZlIHRl c3Rpbmc/Cj4gPj4+Pj4KPiA+Pj4+PiBJIHdpbGwgYWxzbyBmcmVlbHkgYWRtaXQgdGhhdCBJIGRv bid0IGtub3cgdG9ucyBhYm91dCB0aGUgdGhlb3J5Cj4gPj4+Pj4gYmVoaW5kIHRoaXMgcGF0Y2gu ICBJJ20gbW9zdGx5IGp1c3QgcmUtaGFzaGluZyB0aGUgb3JpZ2luYWwgY29tbWl0Cj4gPj4+Pj4g ZnJvbSBLZXZlciB0aGF0IHdhcyByZXZlcnRlZCBzaW5jZToKPiA+Pj4+PiAqIFR1cm5pbmcgb24g cGFydGlhbCBwb3dlciBkb3duIG9uIHJrMzI4OCBkb2Vzbid0ICJqdXN0IHdvcmsiLiAgSQo+ID4+ Pj4+ICAgICAgZG9uJ3QgZ2V0IGhvdHBsdWcgZXZlbnRzLiAgVGhpcyBpcyBkZXNwaXRlIGR3YzIg YXV0by1kZXRlY3RpbmcgdGhhdAo+ID4+Pj4+ICAgICAgd2UgYXJlIHBvd2VyIG9wdGltaXplZC4K PiA+Pj4+IFdoYXQgZG8geW91IG1lYW4gYnkgZG9lc24ndCAianVzdCB3b3JrIiA/IEl0IHNlZW0g dG8gbWUgdGhhdCBldmVuIGFmdGVyCj4gPj4+PiBhZGRpbmcgdGhpcyBwYXRjaCB5b3UgZG9uJ3Qg Z2V0IGlzc3VlcyBmaXhlZC4KPiA+Pj4+IFlvdSBtZW50aW9uIHRoYXQgeW91IGRvbid0IGdldCB0 aGUgaG90cGx1ZyBldmVudHMuIFBsZWFzZSBwcm92aWRlIGR3YzIKPiA+Pj4+IGRlYnVnIGxvZ3Mg YW5kIHJlZ2lzdGVyIGR1bXBzIG9uIHRoaXMgaXNzdWUuCj4gPj4+Cj4gPj4+IEkgbWVhbiB0aGF0 IHBhcnRpYWwgcG93ZXIgZG93biBpbiB0aGUgY3VycmVudGx5IHVwc3RyZWFtIGRyaXZlcgo+ID4+ PiBkb2Vzbid0IHdvcmsuICBBS0E6IGlmIEkgdHVybiBvbiBwYXJ0aWFsIHBvd2VyIGRvd24gaW4g dGhlIHVwc3RyZWFtCj4gPj4+IGRyaXZlciB0aGVuIGhvdHBsdWcgZXZlbnRzIGJyZWFrLiAgSSBj YW4gdHJ5IHRvIHByb3ZpZGUgc29tZSBsb2dzLiAgT24KPiA+Pj4gd2hhdCBleGFjdCB2ZXJzaW9u IG9mIHRoZSBjb2RlIGRvIHlvdSB3YW50IGxvZ3M/ICBKdXN0IHlvdXIgc2VyaWVzPwo+ID4+PiBK dXN0IG15IHNlcmllcz8gIE1haW5saW5lPyAgU29tZSBhdHRlbXB0IGF0IGNvbWJpbmluZyBib3Ro IHNlcmllcz8gIEFzCj4gPj4+IEkgc2FpZCB0aGluZ3Mgc2VlbSB0byBzb3J0YSB3b3JrIHdpdGgg dGhlIGNvbWJpbmVkIHNlcmllcy4gIEkgY2FuIHRyeQo+ID4+PiB0byBjbGFyaWZ5IGlmIHRoYXQn cyB0aGUgc2VyaWVzIHlvdSB3YW50IG1lIHRvIHRlc3Qgd2l0aC4gIC4uLm9yIEkgY2FuCj4gPj4+ IHdhaXQgZm9yIHlvdXIgbmV4dCB2ZXJzaW9uPwo+ID4+IEFzIEkgc2FpZCB0aGlzIHBhdGNoIGRv ZXNuJ3QgZml4IHRoZSBpc3N1ZSB3aXRoIGhvdHBsdWcuIFdpdGggdGhpcyBwYXRjaAo+ID4+IG9y IHdpdGhvdXQgdGhlIGhvdHBsdWcgYmVoYXZlcyBhcyBpdCB3YXMuIEkgaGF2ZSB0ZXN0ZWQgaXQg b24gb3VyIHNldHVwLgo+ID4+Cj4gPj4gSGF2ZSB5b3UgZGVidWdnZWQgeW91ciBwYXRjaD8gRG9l cyBpdCBtYWtlIGFueSBkaWZmZXJlbmNlIG9uIHlvdXIgc2V0dXAKPiA+PiA/IERvZXMgaXQgZml4 IHRoZSBpc3N1ZSB3aXRoIGhvdHBsdWc/Cj4gPgo+ID4gSSB0aGluayB3ZSdyZSBzdGlsbCBub3Qg dGFraW5nIG9uIHRoZSBzYW1lIHBhZ2UuCj4gPgo+ID4gTXkgcGF0Y2ggbWFrZXMgbm8gYXR0ZW1w dCB0byBtYWtlIHBhcnRpYWwgcG93ZXIgZG93biBtb2RlIHdvcmsuICBNeQo+ID4gcGF0Y2ggYXR0 ZW1wdHMgdG8gbWFrZSB0aGluZ3Mgd29yayBhIGxpdHRsZSBiZXR0ZXIgd2hlbiB1c2luZwo+ID4g RFdDMl9QT1dFUl9ET1dOX1BBUkFNX05PTkUuICBUaGVyZSBpcyBubyB1c2UgdGVzdGluZyBteSBw YXRjaCB3aXRoCj4gPiBwYXJ0aWFsIHBvd2VyIGRvd24gYXMgaXQgc2hvdWxkbid0IGhhdmUgYW55 IGltcGFjdCB0aGVyZS4KPiA+Cj4gPgo+ID4+PiBJIGFtIGJ5IG5vIG1lYW5zIGFuIGV4cGVydCBv biBkd2MyLCBidXQgYW4gYXNzdW1wdGlvbiBtYWRlIGluIG15IHBhdGNoCj4gPj4+IGlzIHRoYXQg ZXZlbiBjb3JlcyB0aGF0IGNhbid0IHN1cHBvcnQgcGFydGlhbCBwb3dlciBkb3duIGNhbiBzdGls bAo+ID4+PiBzYXZlIHNvbWUgYW1vdW50IG9mIHBvd2VyIHdoZW4gaGNkX3N1c3BlbmQgaXMgY2Fs bGVkLgo+ID4+IEhhdmUgeW91IHRyaWVkIHRvIGRlYnVnIGR3YzIgd2l0aCBwb3dlcl9kb3duID09 IERXQzJfUE9XRVJfRE9XTl9QQVJBTV9OT05FID8KPiA+Pj4KPiA+Pj4gU29tZSBldmlkZW5jZSB0 aGF0IHRoaXMgc2hvdWxkIGJlIHBvc3NpYmxlOiBsb29raW5nIGF0IG1haW5saW5lIExpbnV4Cj4g Pj4+IGFuZCBhdCBkd2MyX3BvcnRfc3VzcGVuZCgpLCBJIHNlZToKPiA+Pj4KPiA+Pj4gKiBJdCBp cyBjdXJyZW50bHkgY2FsbGVkIGV2ZW4gd2hlbiB3ZSBoYXZlIERXQzJfUE9XRVJfRE9XTl9QQVJB TV9OT05FCj4gPj4gQ3VycmVudGx5ICh3aXRob3V0IHlvdXIgYW5kIG15IHBhdGNoZXMpIChsb29r aW5nIGF0IG1haW5saW5lIExpbnV4KSB0aGUKPiA+PiBmdW5jdGlvbiBkd2MyX3BvcnRfc3VzcGVu ZCgpIGlzIGNhbGxlZCBhbnl3YXkgYmVjYXVzZSBpdHMgY2FsbCBpcyBpc3N1ZWQKPiA+PiBieSB0 aGUgc3lzdGVtLiBCdXQgaXQgcGVyZm9ybXMgZW50ZXJpbmcgdG8gc3VzcGVuZCBvbmx5IGluIGNh c2Ugb2YKPiA+PiBEV0MyX1BPV0VSX0RPV05fUEFSQU1fUEFSVElBTC4KPiA+Pgo+ID4+IFRoaXMg aXMgbm90IGFuIGFzc3VtcHRpb24uIFdoYXQgSSBhbSBwb2ludGluZyBvdXQgaXMgYmFzZWQgb24g ZGVidWdnaW5nCj4gPj4gYW5kIGJlZm9yZSBtYWtpbmcgYXNzdW1wdGlvbnMgd2l0aG91dCBkZWJ1 Z2dpbmcgZm9yIG1lIHNlZW1zIG5vdCBvay4KPiA+Pgo+ID4+IEN1cnJlbnRseSB3aXRob3V0IHlv dXIgcGF0Y2ggYW5kIHdpdGhvdXQgbXkgcGF0Y2hlcy4gSW4gdGhlCj4gPj4gZHdjMl9wb3J0X3N1 c3BlbmQoKSBpdCB3aWxsIGVudGVyIHRvIHN1c3BlbmQgb25seSBpbiBjYXNlIHRoYXQKPiA+PiBw b3dlcl9kb3duID09IERXQzJfUE9XRVJfRE9XTl9QQVJBTV9QQVJUSUFMLiBCZWNhdXNlIGlmIHlv dSBsb29rIGF0IHRoZQo+ID4+IGNvZGUgbW9yZSBjYXJlZnVsbHkgeW91IHdpbGwgc2VlCj4gPj4K PiA+PiAgICAgICAgICBpZiAoaHNvdGctPnBhcmFtcy5wb3dlcl9kb3duICE9IERXQzJfUE9XRVJf RE9XTl9QQVJBTV9QQVJUSUFMKQo+ID4+ICAgICAgICAgICAgICAgICAgZ290byBza2lwX3Bvd2Vy X3NhdmluZzsKPiA+Pgo+ID4+IFRoaXMgc2F5cyBpZiBwb3dlcl9kb3duIGlzIG5vdCBEV0MyX1BP V0VSX0RPV05fUEFSQU1fUEFSVElBTCB0aGVuIHNraXAKPiA+PiBwb3dlciBzYXZpbmcuCj4gPj4K PiA+PiBTbyBidXQgYWZ0ZXIgeW91ciBwYXRjaC4gSWYgcG93ZXJfZG93biBpcyBEV0MyX1BPV0VS X0RPV05fUEFSQU1fTk9ORSBpdAo+ID4+IHRyaWVzIHRvIHN1c3BlbmQuCj4gPgo+ID4gV2UgbXVz dCBiZSBsb29raW5nIGF0IGRpZmZlcmVudCBjb2RlLiAgSSdtIGxvb2tpbmcgYXQgTGludXgncyB0 cmVlLCBBS0E6Cj4gPgo+ID4gaHR0cHM6Ly91cmxkZWZlbnNlLnByb29mcG9pbnQuY29tL3YyL3Vy bD91PWh0dHBzLTNBX19naXQua2VybmVsLm9yZ19wdWJfc2NtX2xpbnV4X2tlcm5lbF9naXRfdG9y dmFsZHNfbGludXguZ2l0X3RyZWVfZHJpdmVyc191c2JfZHdjMl9oY2QuYy0yM24zNDg4JmQ9RHdJ RmFRJmM9RFBMNl9YXzZKa1hGeDdBWFdxQjB0ZyZyPTloUEJGS0NKX25CakpoR1ZycmxZT2VPUWpQ X0hsVnpZcXJDX0Q3bmlNSkkmbT1JV2tET09HVHIwcS1IMXBpRHYyS09aZV9IbnJ6MThnNnJYRngt RHNUdXY0JnM9QUh1MmlPS2t5YmxpUkd0SWZON2NGNXAwNzBVZHZVS1RZSnN5QUtZb2ppcyZlPQo+ IEhlcmUgeW91IGFyZSBsb29raW5nIGF0IHRoZSBvbGQgY29kZS4gQWZ0ZXIgdGhhdCB0aGVyZSBh cmUgc2V2ZXJhbCBvZgo+IGNoYW5nZXMgcmVsYXRlZCB0byBzdXNwZW5kL3Jlc3VtZSBmdW5jdGlv bnMuCgpJbiBteSBlbWFpbCwgc2VlIHRoYXQgSSBzYWlkIEkgYWN0dWFsbHkgY2hlY2tlZCBvdXQg bWFpbmxpbmUga2VybmVsCihhbmQgSSBnYXZlIHlvdSB0aGUgZXhhY3QgdmVyc2lvbjogInY1LjEt cmM3LTUtZzgzYTUwODQwZTcyYSIpIGFuZAphZGRlZCBwcmludG91dHMgaW4gZHdjMl9wb3J0X3N1 c3BlbmQoKSBuZXh0IHRvIHdoZXJlIGl0IHNldCBIUFJUMF9TVVNQCmFuZCBQQ0dDVExfU1RPUFBD TEsgaW4gZHdjMl9wb3J0X3N1c3BlbmQoKS4KClsgIDQ1NC45MDYzNjRdIGR3YzIgZmY1NDAwMDAu dXNiOiBJJ20gc2V0dGluZyBIUFJUMF9TVVNQClsgIDQ1NC45MDYzNjddIGR3YzIgZmY1NDAwMDAu dXNiOiBJJ20gc2V0dGluZyBQQ0dDVExfU1RPUFBDTEsKClRoZSB2ZXJzaW9uICJ2NS4xLXJjNy01 LWc4M2E1MDg0MGU3MmEiIGlzIG5vdCBvbGQgY29kZS4KCgo+IFRoaXMgaXMgdGhlIGxpbmsgdG8g dGhlIGNvZGUgd2l0aCBjaGFuZ2VzLiBMYXRlc3QgdmVyc2lvbiBvZiB0aG9zZQo+IGZ1bmN0aW9u cy4KPgo+IGh0dHBzOi8vZ2l0Lmtlcm5lbC5vcmcvcHViL3NjbS9saW51eC9rZXJuZWwvZ2l0L3Rv cnZhbGRzL2xpbnV4LmdpdC90cmVlL2RyaXZlcnMvdXNiL2R3YzIvaGNkLmMjbjQ0ODkKPgo+IFlv dXIgY2hhbmdlcyBhcmUgc2l0dGluZyBvbiB0aGF0IGxhdGVzdCB2ZXJzaW9uIG9mIGNvZGUuIE5v dCB0aGUgb2xkCj4gdmVyc2lvbiBvZiBpdC4KCllvdSBhcmUgcG9pbnRpbmcgbWUgYXQgX2R3YzJf aGNkX3N1c3BlbmQoKSB3aGVyZWFzIEkgcG9pbnRlZCBhdApkd2MyX3BvcnRfc3VzcGVuZCgpLiAg V2h5PwoKSSBhbSBzYXlpbmcgdGhhdCBkd2MyX3BvcnRfc3VzcGVuZCgpIF9kb2VzXyBzZXQgIkhQ UlQwX1NVU1AiIGFuZAoiUENHQ1RMX1NUT1BQQ0xLIiBldmVuIHdpdGggRFdDMl9QT1dFUl9ET1dO X1BBUkFNX05PTkUuICBEbyB5b3UKZGlzYWdyZWU/CgpJIGNvbXBsZXRlbHkgYWdyZWUgdGhhdCBv biBtYWlubGluZSBfZHdjMl9oY2Rfc3VzcGVuZCgpIF9kb2VzIG5vdF8gc2V0CnRoZXNlIGJpdHMg d2l0aCBEV0MyX1BPV0VSX0RPV05fUEFSQU1fTk9ORS4gIFRoYXQgaXMgd2hhdCBteSBwYXRjaApm aXhlcy4KCgotRG91Zwo= 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: Fri, 3 May 2019 08:03:24 -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 Fri, May 3, 2019 at 1:20 AM Artur Petrosyan wrote: > > On 5/1/2019 05:57, Doug Anderson wrote: > > 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 b= us > >>>>> suspend/resume for dwc2") on ToT. That commit was reverted in comm= it > >>>>> 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 Chro= me > >>>>> 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.= org_r_1446237173-2D15263-2D1-2Dgit-2Dsend-2Demail-2Ddianders-40chromium.org= &d=3DDwIDAg&c=3DDPL6_X_6JkXFx7AXWqB0tg&r=3D9hPBFKCJ_nBjJhGVrrlYOeOQjP_HlVzY= qrC_D7niMJI&m=3DMMfe-4lZePyty6F5zfQ54kiYGuJWNulyRat944LkOsc&s=3DnExFpAPP_0p= lZfO5LMG1B-mqt1vyCvE35elVcyVgs8Y&e=3D > >>>>> > >>>>> ...and appears to have died the death of silence. Maybe it could g= et > >>>>> 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 af= ter > >>>> adding this patch you don't get issues fixed. > >>>> You mention that you don't get the hotplug events. Please provide dw= c2 > >>>> 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. O= n > >>> what exact version of the code do you want logs? Just your series? > >>> Just my series? Mainline? Some attempt at combining both series? A= s > >>> 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 ca= n > >>> wait for your next version? > >> As I said this patch doesn't fix the issue with hotplug. With this pat= ch > >> or without the hotplug behaves as it was. I have tested it on our setu= p. > >> > >> Have you debugged your patch? Does it make any difference on your setu= p > >> ? 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 patc= h > >>> 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_PA= RAM_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) th= e > >> function dwc2_port_suspend() is called anyway because its call is issu= ed > >> 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 debuggin= g > >> 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 a= t the > >> code more carefully you will see > >> > >> if (hsotg->params.power_down !=3D DWC2_POWER_DOWN_PARAM_PARTI= AL) > >> 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 i= t > >> tries to suspend. > > > > We must be looking at different code. I'm looking at Linux's tree, AKA= : > > > > https://urldefense.proofpoint.com/v2/url?u=3Dhttps-3A__git.kernel.org_p= ub_scm_linux_kernel_git_torvalds_linux.git_tree_drivers_usb_dwc2_hcd.c-23n3= 488&d=3DDwIFaQ&c=3DDPL6_X_6JkXFx7AXWqB0tg&r=3D9hPBFKCJ_nBjJhGVrrlYOeOQjP_Hl= VzYqrC_D7niMJI&m=3DIWkDOOGTr0q-H1piDv2KOZe_Hnrz18g6rXFx-DsTuv4&s=3DAHu2iOKk= ybliRGtIfN7cF5p070UdvUKTYJsyAKYojis&e=3D > Here you are looking at the old code. After that there are several of > changes related to suspend/resume functions. In my email, see that I said I actually checked out mainline kernel (and I gave you the exact version: "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 The version "v5.1-rc7-5-g83a50840e72a" is not old code. > This is the link to the code with changes. Latest version of those > functions. > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/d= rivers/usb/dwc2/hcd.c#n4489 > > Your changes are sitting on that latest version of code. Not the old > version of it. You are pointing me at _dwc2_hcd_suspend() whereas I pointed at dwc2_port_suspend(). Why? I am saying that dwc2_port_suspend() _does_ set "HPRT0_SUSP" and "PCGCTL_STOPPCLK" even with DWC2_POWER_DOWN_PARAM_NONE. Do you disagree? I completely agree that on mainline _dwc2_hcd_suspend() _does not_ set these bits with DWC2_POWER_DOWN_PARAM_NONE. That is what my patch fixes. -Doug