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=-4.2 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS 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 511A7C43219 for ; Mon, 29 Apr 2019 17:34:06 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 146FD2075E for ; Mon, 29 Apr 2019 17:34:06 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=chromium.org header.i=@chromium.org header.b="SqTTfZ8x" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729049AbfD2ReE (ORCPT ); Mon, 29 Apr 2019 13:34:04 -0400 Received: from mail-vs1-f66.google.com ([209.85.217.66]:34813 "EHLO mail-vs1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728748AbfD2ReE (ORCPT ); Mon, 29 Apr 2019 13:34:04 -0400 Received: by mail-vs1-f66.google.com with SMTP id n17so6410254vsr.1 for ; Mon, 29 Apr 2019 10:34:03 -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=oRI/yWgrQc4mohswqM3B8q6VlrbjFTkbV3C0nqXm/N8=; b=SqTTfZ8xJPeqYmNsfZX4+7c7BOKZ+w14+xT/g9LA4jEOJ/AvzuukBOxc/w4W9/ar0o u8eRsIF7UgmNV6PzwCkEO/jDyOCF7AdfB53c0wLmmIMl3EBzBwHOvZCTQZZzvDgy3SjA fS3NhoZs4IM+X4Jzf5r4xVFETZvFgAFmXfZfg= 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=oRI/yWgrQc4mohswqM3B8q6VlrbjFTkbV3C0nqXm/N8=; b=tClXUb9TKrJfSSAgDwspUimYVWDHwTBLFM5wiTCDZMxaorCdcMswtQ4hRJGy+oOHgz VpLk4/P8IkuNWV2rTYTYGgpwcv27hDlqof3Hkd3sFRkyUxH9yhyzPn9ue0xV9wncY/QF otcjYeANyp0XRt601dvd6j42oDfgfokuRHyjNboxsvxXfebEERxMnLe/TOD7drEzpbyp bkgDYXpSqO6CgzUf8ixT228tVDP6SbCkMlbVOkE9jv/1boDzICe1eiruQSjmQjnVu5N8 TZUQR6QFW+f7cdviYiS6z34p1nEgWzVsHNjnhyc6OQ1JpAExK/4rQmhuxQb2hZJN/3p7 5SBg== X-Gm-Message-State: APjAAAVyIXFRYTI6ZeuP78cb1SiRD/r7tnCyraEoIReaZkVcXr0g+wIj U7+AjB93WxdufMDeEz8z814MhvheUcw= X-Google-Smtp-Source: APXvYqyhpZ3Rw/n1CShhoiHz5VDps20GNJRjC9Ofq9x2h/fTi2Exit+qhOtBePW4L3IgavevUByX/A== X-Received: by 2002:a67:78c5:: with SMTP id t188mr33183190vsc.107.1556559242049; Mon, 29 Apr 2019 10:34:02 -0700 (PDT) Received: from mail-vs1-f41.google.com (mail-vs1-f41.google.com. [209.85.217.41]) by smtp.gmail.com with ESMTPSA id s16sm4106577vks.39.2019.04.29.10.34.00 for (version=TLS1_3 cipher=AEAD-AES128-GCM-SHA256 bits=128/128); Mon, 29 Apr 2019 10:34:00 -0700 (PDT) Received: by mail-vs1-f41.google.com with SMTP id o10so6353272vsp.12 for ; Mon, 29 Apr 2019 10:34:00 -0700 (PDT) X-Received: by 2002:a67:bc01:: with SMTP id t1mr33891102vsn.149.1556559239606; Mon, 29 Apr 2019 10:33:59 -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: Mon, 29 Apr 2019 10:33:48 -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 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.org_= 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 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? > > @@ -4506,21 +4507,35 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hc= d) > > */ > > if (!hsotg->bus_suspended) { > > hprt0 =3D dwc2_read_hprt0(hsotg); > > - hprt0 |=3D HPRT0_SUSP; > > - hprt0 &=3D ~HPRT0_PWR; > > - dwc2_writel(hsotg, hprt0, HPRT0); > > - spin_unlock_irqrestore(&hsotg->lock, flags); > > - dwc2_vbus_supply_exit(hsotg); > > - spin_lock_irqsave(&hsotg->lock, flags); > > + if (hprt0 & HPRT0_CONNSTS) { > + h= prt0 |=3D HPRT0_SUSP; > Here you set "HPRT0_SUSP" bit but what if core doesn't support both > hibernation and Partial Power down assuming that > hsotg->params.power_down" value us equal to "DWC2_POWER_DOWN_PARAM_NONE" > which is 0. 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. 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 * It currently sets HPRT0_SUSP * It currently sets PCGCTL_STOPPCLK specifically in the case where power down is DWC2_POWER_DOWN_PARAM_NONE. ...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? > > + if (hsotg->params.power_down =3D=3D DWC2_POWER_DO= WN_PARAM_PARTIAL) > You make one checking of hsotg->params.power_down mode here. > > + hprt0 &=3D ~HPRT0_PWR; > > + dwc2_writel(hsotg, hprt0, HPRT0); > > + } > > + if (hsotg->params.power_down =3D=3D DWC2_POWER_DOWN_PARAM= _PARTIAL) { > another checking of power_down mode here. Yeah, we can debate about how to best share/split code. I'm not in love with the current structure either. When I rebased your patches atop mine I changed this to more fully split them and I agree that was better. > > @@ -4592,10 +4612,12 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd= ) > > spin_unlock_irqrestore(&hsotg->lock, flags); > > dwc2_port_resume(hsotg); > > } else { > > - dwc2_vbus_supply_init(hsotg); > > + if (hsotg->params.power_down =3D=3D DWC2_POWER_DOWN_PARAM= _PARTIAL) { > > + dwc2_vbus_supply_init(hsotg); > > > > - /* Wait for controller to correctly update D+/D- level */ > > - usleep_range(3000, 5000); > > + /* Wait for controller to correctly update D+/D- = level */ > > + usleep_range(3000, 5000); > > + } > > > > /* > > * Clear Port Enable and Port Status changes. > > > > I have tested the patch on HAPS-DX. With this patch or without it when I > have a device connected core enters to partial power down and doesn't > exit from it. So I cannot use the device. Can you explain what HAPS-DX is? -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: Mon, 29 Apr 2019 10:33:48 -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: SGksCgpPbiBNb24sIEFwciAyOSwgMjAxOSBhdCAxOjQzIEFNIEFydHVyIFBldHJvc3lhbgo8QXJ0 aHVyLlBldHJvc3lhbkBzeW5vcHN5cy5jb20+IHdyb3RlOgo+Cj4gSGksCj4KPiBPbiA0LzE4LzIw MTkgMDQ6MTUsIERvdWdsYXMgQW5kZXJzb24gd3JvdGU6Cj4gPiBUaGlzIGlzIGFuIGF0dGVtcHQg dG8gcmVoYXNoIGNvbW1pdCAwY2Y4ODRlODE5ZTAgKCJ1c2I6IGR3YzI6IGFkZCBidXMKPiA+IHN1 c3BlbmQvcmVzdW1lIGZvciBkd2MyIikgb24gVG9ULiAgVGhhdCBjb21taXQgd2FzIHJldmVydGVk IGluIGNvbW1pdAo+ID4gYjBiYjliYjZjZTAxICgiUmV2ZXJ0ICJ1c2I6IGR3YzI6IGFkZCBidXMg c3VzcGVuZC9yZXN1bWUgZm9yIGR3YzIiIikKPiA+IGJlY2F1c2UgYXBwYXJlbnRseSBpdCBicm9r ZSB0aGUgQWx0ZXJhIFNPQ0ZQR0EuCj4gPgo+ID4gV2l0aCBhbGwgdGhlIGNoYW5nZXMgdGhhdCBo YXZlIGhhcHBlbmVkIHRvIGR3YzIgaW4gdGhlIG1lYW50aW1lLCBpdCdzCj4gPiBwb3NzaWJsZSB0 aGF0IHRoZSBBbHRlcmEgU09DRlBHQSB3aWxsIGp1c3QgbWFnaWNhbGx5IHdvcmsgd2l0aCB0aGlz Cj4gPiBjaGFuZ2Ugbm93LiAgLi4uYW5kIGl0IHdvdWxkIGJlIGdvb2QgdG8gZ2V0IGJ1cyBzdXNw ZW5kL3Jlc3VtZQo+ID4gaW1wbGVtZW50ZWQuCj4gPgo+ID4gVGhpcyBjaGFuZ2UgaXMgYSBmb3J3 YXJkIHBvcnQgb2Ygb25lIHRoYXQncyBiZWVuIGxpdmluZyBpbiB0aGUgQ2hyb21lCj4gPiBPUyAz LjE0IGtlcm5lbCB0cmVlLgo+ID4KPiA+IFNpZ25lZC1vZmYtYnk6IERvdWdsYXMgQW5kZXJzb24g PGRpYW5kZXJzQGNocm9taXVtLm9yZz4KPiA+IC0tLQo+ID4gVGhpcyBwYXRjaCB3YXMgbGFzdCBw b3N0ZWQgYXQ6Cj4gPgo+ID4gaHR0cHM6Ly91cmxkZWZlbnNlLnByb29mcG9pbnQuY29tL3YyL3Vy bD91PWh0dHBzLTNBX19sa21sLmtlcm5lbC5vcmdfcl8xNDQ2MjM3MTczLTJEMTUyNjMtMkQxLTJE Z2l0LTJEc2VuZC0yRGVtYWlsLTJEZGlhbmRlcnMtNDBjaHJvbWl1bS5vcmcmZD1Ed0lEQWcmYz1E UEw2X1hfNkprWEZ4N0FYV3FCMHRnJnI9OWhQQkZLQ0pfbkJqSmhHVnJybFlPZU9RalBfSGxWellx ckNfRDduaU1KSSZtPU1NZmUtNGxaZVB5dHk2RjV6ZlE1NGtpWUd1SldOdWx5UmF0OTQ0TGtPc2Mm cz1uRXhGcEFQUF8wcGxaZk81TE1HMUItbXF0MXZ5Q3ZFMzVlbFZjeVZnczhZJmU9Cj4gPgo+ID4g Li4uYW5kIGFwcGVhcnMgdG8gaGF2ZSBkaWVkIHRoZSBkZWF0aCBvZiBzaWxlbmNlLiAgTWF5YmUg aXQgY291bGQgZ2V0Cj4gPiBzb21lIGJha2UgdGltZSBpbiBsaW51eG5leHQgaWYgd2UgY2FuJ3Qg ZmluZCBhbnkgcHJvYWN0aXZlIHRlc3Rpbmc/Cj4gPgo+ID4gSSB3aWxsIGFsc28gZnJlZWx5IGFk bWl0IHRoYXQgSSBkb24ndCBrbm93IHRvbnMgYWJvdXQgdGhlIHRoZW9yeQo+ID4gYmVoaW5kIHRo aXMgcGF0Y2guICBJJ20gbW9zdGx5IGp1c3QgcmUtaGFzaGluZyB0aGUgb3JpZ2luYWwgY29tbWl0 Cj4gPiBmcm9tIEtldmVyIHRoYXQgd2FzIHJldmVydGVkIHNpbmNlOgo+ID4gKiBUdXJuaW5nIG9u IHBhcnRpYWwgcG93ZXIgZG93biBvbiByazMyODggZG9lc24ndCAianVzdCB3b3JrIi4gIEkKPiA+ ICAgIGRvbid0IGdldCBob3RwbHVnIGV2ZW50cy4gIFRoaXMgaXMgZGVzcGl0ZSBkd2MyIGF1dG8t ZGV0ZWN0aW5nIHRoYXQKPiA+ICAgIHdlIGFyZSBwb3dlciBvcHRpbWl6ZWQuCj4gV2hhdCBkbyB5 b3UgbWVhbiBieSBkb2Vzbid0ICJqdXN0IHdvcmsiID8gSXQgc2VlbSB0byBtZSB0aGF0IGV2ZW4g YWZ0ZXIKPiBhZGRpbmcgdGhpcyBwYXRjaCB5b3UgZG9uJ3QgZ2V0IGlzc3VlcyBmaXhlZC4KPiBZ b3UgbWVudGlvbiB0aGF0IHlvdSBkb24ndCBnZXQgdGhlIGhvdHBsdWcgZXZlbnRzLiBQbGVhc2Ug cHJvdmlkZSBkd2MyCj4gZGVidWcgbG9ncyBhbmQgcmVnaXN0ZXIgZHVtcHMgb24gdGhpcyBpc3N1 ZS4KCkkgbWVhbiB0aGF0IHBhcnRpYWwgcG93ZXIgZG93biBpbiB0aGUgY3VycmVudGx5IHVwc3Ry ZWFtIGRyaXZlcgpkb2Vzbid0IHdvcmsuICBBS0E6IGlmIEkgdHVybiBvbiBwYXJ0aWFsIHBvd2Vy IGRvd24gaW4gdGhlIHVwc3RyZWFtCmRyaXZlciB0aGVuIGhvdHBsdWcgZXZlbnRzIGJyZWFrLiAg SSBjYW4gdHJ5IHRvIHByb3ZpZGUgc29tZSBsb2dzLiAgT24Kd2hhdCBleGFjdCB2ZXJzaW9uIG9m IHRoZSBjb2RlIGRvIHlvdSB3YW50IGxvZ3M/ICBKdXN0IHlvdXIgc2VyaWVzPwpKdXN0IG15IHNl cmllcz8gIE1haW5saW5lPyAgU29tZSBhdHRlbXB0IGF0IGNvbWJpbmluZyBib3RoIHNlcmllcz8g IEFzCkkgc2FpZCB0aGluZ3Mgc2VlbSB0byBzb3J0YSB3b3JrIHdpdGggdGhlIGNvbWJpbmVkIHNl cmllcy4gIEkgY2FuIHRyeQp0byBjbGFyaWZ5IGlmIHRoYXQncyB0aGUgc2VyaWVzIHlvdSB3YW50 IG1lIHRvIHRlc3Qgd2l0aC4gIC4uLm9yIEkgY2FuCndhaXQgZm9yIHlvdXIgbmV4dCB2ZXJzaW9u PwoKCj4gPiBAQCAtNDUwNiwyMSArNDUwNywzNSBAQCBzdGF0aWMgaW50IF9kd2MyX2hjZF9zdXNw ZW5kKHN0cnVjdCB1c2JfaGNkICpoY2QpCj4gPiAgICAgICAgKi8KPiA+ICAgICAgIGlmICghaHNv dGctPmJ1c19zdXNwZW5kZWQpIHsKPiA+ICAgICAgICAgICAgICAgaHBydDAgPSBkd2MyX3JlYWRf aHBydDAoaHNvdGcpOwo+ID4gLSAgICAgICAgICAgICBocHJ0MCB8PSBIUFJUMF9TVVNQOwo+ID4g LSAgICAgICAgICAgICBocHJ0MCAmPSB+SFBSVDBfUFdSOwo+ID4gLSAgICAgICAgICAgICBkd2My X3dyaXRlbChoc290ZywgaHBydDAsIEhQUlQwKTsKPiA+IC0gICAgICAgICAgICAgc3Bpbl91bmxv Y2tfaXJxcmVzdG9yZSgmaHNvdGctPmxvY2ssIGZsYWdzKTsKPiA+IC0gICAgICAgICAgICAgZHdj Ml92YnVzX3N1cHBseV9leGl0KGhzb3RnKTsKPiA+IC0gICAgICAgICAgICAgc3Bpbl9sb2NrX2ly cXNhdmUoJmhzb3RnLT5sb2NrLCBmbGFncyk7Cj4gPiArICAgICAgICAgICAgIGlmIChocHJ0MCAm IEhQUlQwX0NPTk5TVFMpIHsgPiArICAgICAgICAgICAgICAgICAgICAgICAgaHBydDAgfD0gSFBS VDBfU1VTUDsKPiBIZXJlIHlvdSBzZXQgIkhQUlQwX1NVU1AiIGJpdCBidXQgd2hhdCBpZiBjb3Jl IGRvZXNuJ3Qgc3VwcG9ydCBib3RoCj4gaGliZXJuYXRpb24gYW5kIFBhcnRpYWwgUG93ZXIgZG93 biBhc3N1bWluZyB0aGF0Cj4gaHNvdGctPnBhcmFtcy5wb3dlcl9kb3duIiB2YWx1ZSB1cyBlcXVh bCB0byAiRFdDMl9QT1dFUl9ET1dOX1BBUkFNX05PTkUiCj4gd2hpY2ggaXMgMC4KCkkgYW0gYnkg bm8gbWVhbnMgYW4gZXhwZXJ0IG9uIGR3YzIsIGJ1dCBhbiBhc3N1bXB0aW9uIG1hZGUgaW4gbXkg cGF0Y2gKaXMgdGhhdCBldmVuIGNvcmVzIHRoYXQgY2FuJ3Qgc3VwcG9ydCBwYXJ0aWFsIHBvd2Vy IGRvd24gY2FuIHN0aWxsCnNhdmUgc29tZSBhbW91bnQgb2YgcG93ZXIgd2hlbiBoY2Rfc3VzcGVu ZCBpcyBjYWxsZWQuCgpTb21lIGV2aWRlbmNlIHRoYXQgdGhpcyBzaG91bGQgYmUgcG9zc2libGU6 IGxvb2tpbmcgYXQgbWFpbmxpbmUgTGludXgKYW5kIGF0IGR3YzJfcG9ydF9zdXNwZW5kKCksIEkg c2VlOgoKKiBJdCBpcyBjdXJyZW50bHkgY2FsbGVkIGV2ZW4gd2hlbiB3ZSBoYXZlIERXQzJfUE9X RVJfRE9XTl9QQVJBTV9OT05FCgoqIEl0IGN1cnJlbnRseSBzZXRzIEhQUlQwX1NVU1AKCiogSXQg Y3VycmVudGx5IHNldHMgUENHQ1RMX1NUT1BQQ0xLIHNwZWNpZmljYWxseSBpbiB0aGUgY2FzZSB3 aGVyZQpwb3dlciBkb3duIGlzIERXQzJfUE9XRVJfRE9XTl9QQVJBTV9OT05FLgoKLi4uSSBiZWxp ZXZlIHRoYXQgdGhlIG5ldCBlZmZlY3Qgb2YgbXkgcGF0Y2ggZW5kcyB1cCBkb2luZyBib3RoIHRo b3NlCnNhbWUgdHdvIHRoaW5ncyBpbiBoY2Rfc3VzcGVuZC4gIFRoYXQgaXM6IHdoZW4gcG93ZXJf ZG93biBpcwpEV0MyX1BPV0VSX0RPV05fUEFSQU1fTk9ORSBJIGJlbGlldmUgbXkgcGF0Y2ggaXMg cmVhbGx5IGp1c3QgZG9pbmcgdGhlCnNhbWUgdGhpbmcgdGhhdCBkd2MyX3BvcnRfc3VzcGVuZCgp IHdvdWxkIGRvIGluIHRoZSBzYW1lIGNhc2UuICBJcwp0aGF0IG5vdCBPSz8KCgoKPiA+ICsgICAg ICAgICAgICAgICAgICAgICBpZiAoaHNvdGctPnBhcmFtcy5wb3dlcl9kb3duID09IERXQzJfUE9X RVJfRE9XTl9QQVJBTV9QQVJUSUFMKQo+IFlvdSBtYWtlIG9uZSBjaGVja2luZyBvZiBoc290Zy0+ cGFyYW1zLnBvd2VyX2Rvd24gbW9kZSBoZXJlLgo+ID4gKyAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgaHBydDAgJj0gfkhQUlQwX1BXUjsKPiA+ICsgICAgICAgICAgICAgICAgICAgICBkd2My X3dyaXRlbChoc290ZywgaHBydDAsIEhQUlQwKTsKPiA+ICsgICAgICAgICAgICAgfQo+ID4gKyAg ICAgICAgICAgICBpZiAoaHNvdGctPnBhcmFtcy5wb3dlcl9kb3duID09IERXQzJfUE9XRVJfRE9X Tl9QQVJBTV9QQVJUSUFMKSB7Cj4gYW5vdGhlciBjaGVja2luZyBvZiBwb3dlcl9kb3duIG1vZGUg aGVyZS4KClllYWgsIHdlIGNhbiBkZWJhdGUgYWJvdXQgaG93IHRvIGJlc3Qgc2hhcmUvc3BsaXQg Y29kZS4gIEknbSBub3QgaW4KbG92ZSB3aXRoIHRoZSBjdXJyZW50IHN0cnVjdHVyZSBlaXRoZXIu ICBXaGVuIEkgcmViYXNlZCB5b3VyIHBhdGNoZXMKYXRvcCBtaW5lIEkgY2hhbmdlZCB0aGlzIHRv IG1vcmUgZnVsbHkgc3BsaXQgdGhlbSBhbmQgSSBhZ3JlZSB0aGF0IHdhcwpiZXR0ZXIuCgoKPiA+ IEBAIC00NTkyLDEwICs0NjEyLDEyIEBAIHN0YXRpYyBpbnQgX2R3YzJfaGNkX3Jlc3VtZShzdHJ1 Y3QgdXNiX2hjZCAqaGNkKQo+ID4gICAgICAgICAgICAgICBzcGluX3VubG9ja19pcnFyZXN0b3Jl KCZoc290Zy0+bG9jaywgZmxhZ3MpOwo+ID4gICAgICAgICAgICAgICBkd2MyX3BvcnRfcmVzdW1l KGhzb3RnKTsKPiA+ICAgICAgIH0gZWxzZSB7Cj4gPiAtICAgICAgICAgICAgIGR3YzJfdmJ1c19z dXBwbHlfaW5pdChoc290Zyk7Cj4gPiArICAgICAgICAgICAgIGlmIChoc290Zy0+cGFyYW1zLnBv d2VyX2Rvd24gPT0gRFdDMl9QT1dFUl9ET1dOX1BBUkFNX1BBUlRJQUwpIHsKPiA+ICsgICAgICAg ICAgICAgICAgICAgICBkd2MyX3ZidXNfc3VwcGx5X2luaXQoaHNvdGcpOwo+ID4KPiA+IC0gICAg ICAgICAgICAgLyogV2FpdCBmb3IgY29udHJvbGxlciB0byBjb3JyZWN0bHkgdXBkYXRlIEQrL0Qt IGxldmVsICovCj4gPiAtICAgICAgICAgICAgIHVzbGVlcF9yYW5nZSgzMDAwLCA1MDAwKTsKPiA+ ICsgICAgICAgICAgICAgICAgICAgICAvKiBXYWl0IGZvciBjb250cm9sbGVyIHRvIGNvcnJlY3Rs eSB1cGRhdGUgRCsvRC0gbGV2ZWwgKi8KPiA+ICsgICAgICAgICAgICAgICAgICAgICB1c2xlZXBf cmFuZ2UoMzAwMCwgNTAwMCk7Cj4gPiArICAgICAgICAgICAgIH0KPiA+Cj4gPiAgICAgICAgICAg ICAgIC8qCj4gPiAgICAgICAgICAgICAgICAqIENsZWFyIFBvcnQgRW5hYmxlIGFuZCBQb3J0IFN0 YXR1cyBjaGFuZ2VzLgo+ID4KPgo+IEkgaGF2ZSB0ZXN0ZWQgdGhlIHBhdGNoIG9uIEhBUFMtRFgu IFdpdGggdGhpcyBwYXRjaCBvciB3aXRob3V0IGl0IHdoZW4gSQo+IGhhdmUgYSBkZXZpY2UgY29u bmVjdGVkIGNvcmUgIGVudGVycyB0byBwYXJ0aWFsIHBvd2VyIGRvd24gYW5kIGRvZXNuJ3QKPiBl eGl0IGZyb20gaXQuIFNvIEkgY2Fubm90IHVzZSB0aGUgZGV2aWNlLgoKQ2FuIHlvdSBleHBsYWlu IHdoYXQgSEFQUy1EWCBpcz8KCgotRG91Zwo= 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: Mon, 29 Apr 2019 10:33:48 -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 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.org_= 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 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? > > @@ -4506,21 +4507,35 @@ static int _dwc2_hcd_suspend(struct usb_hcd *hc= d) > > */ > > if (!hsotg->bus_suspended) { > > hprt0 =3D dwc2_read_hprt0(hsotg); > > - hprt0 |=3D HPRT0_SUSP; > > - hprt0 &=3D ~HPRT0_PWR; > > - dwc2_writel(hsotg, hprt0, HPRT0); > > - spin_unlock_irqrestore(&hsotg->lock, flags); > > - dwc2_vbus_supply_exit(hsotg); > > - spin_lock_irqsave(&hsotg->lock, flags); > > + if (hprt0 & HPRT0_CONNSTS) { > + h= prt0 |=3D HPRT0_SUSP; > Here you set "HPRT0_SUSP" bit but what if core doesn't support both > hibernation and Partial Power down assuming that > hsotg->params.power_down" value us equal to "DWC2_POWER_DOWN_PARAM_NONE" > which is 0. 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. 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 * It currently sets HPRT0_SUSP * It currently sets PCGCTL_STOPPCLK specifically in the case where power down is DWC2_POWER_DOWN_PARAM_NONE. ...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? > > + if (hsotg->params.power_down =3D=3D DWC2_POWER_DO= WN_PARAM_PARTIAL) > You make one checking of hsotg->params.power_down mode here. > > + hprt0 &=3D ~HPRT0_PWR; > > + dwc2_writel(hsotg, hprt0, HPRT0); > > + } > > + if (hsotg->params.power_down =3D=3D DWC2_POWER_DOWN_PARAM= _PARTIAL) { > another checking of power_down mode here. Yeah, we can debate about how to best share/split code. I'm not in love with the current structure either. When I rebased your patches atop mine I changed this to more fully split them and I agree that was better. > > @@ -4592,10 +4612,12 @@ static int _dwc2_hcd_resume(struct usb_hcd *hcd= ) > > spin_unlock_irqrestore(&hsotg->lock, flags); > > dwc2_port_resume(hsotg); > > } else { > > - dwc2_vbus_supply_init(hsotg); > > + if (hsotg->params.power_down =3D=3D DWC2_POWER_DOWN_PARAM= _PARTIAL) { > > + dwc2_vbus_supply_init(hsotg); > > > > - /* Wait for controller to correctly update D+/D- level */ > > - usleep_range(3000, 5000); > > + /* Wait for controller to correctly update D+/D- = level */ > > + usleep_range(3000, 5000); > > + } > > > > /* > > * Clear Port Enable and Port Status changes. > > > > I have tested the patch on HAPS-DX. With this patch or without it when I > have a device connected core enters to partial power down and doesn't > exit from it. So I cannot use the device. Can you explain what HAPS-DX is? -Doug