Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934008AbdCVJBx (ORCPT ); Wed, 22 Mar 2017 05:01:53 -0400 Received: from mga04.intel.com ([192.55.52.120]:11624 "EHLO mga04.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933954AbdCVJBt (ORCPT ); Wed, 22 Mar 2017 05:01:49 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.36,204,1486454400"; d="asc'?scan'208";a="79321000" From: Felipe Balbi To: Baolin Wang Cc: Mathias Nyman , Mathias Nyman , Greg KH , Mark Brown , USB , LKML , Alan Stern Subject: Re: [PATCH v5 1/2] usb: host: plat: Enable xhci plat runtime PM In-Reply-To: References: <58908DBC.5010208@linux.intel.com> <58AB06EA.7040401@linux.intel.com> <87d1dbuk73.fsf@linux.intel.com> Date: Wed, 22 Mar 2017 11:00:41 +0200 Message-ID: <871stpvg7a.fsf@linux.intel.com> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="=-=-="; micalg=pgp-sha256; protocol="application/pgp-signature" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4310 Lines: 132 --=-=-= Content-Type: text/plain Content-Transfer-Encoding: quoted-printable Hi, Baolin Wang writes: >>>>> I don't yet understand why we can't just keep runtime pm disabled as a >>>>> default for xhci platform devices. >>>>> It could be enabled by whatever creates the platform device by settin= g some >>>>> device property >>>>> (or equivalent), which would be checked in xhci_plat_probe() before e= nabling >>>>> runtime pm. It >>>>> could then optionally be set in sysfs via power/control entry. >>>> >>>> I think runtime pm is not one hardware property, is it suitable if we >>>> introduce one device property to enable/disable runtime pm? >>> >>> As I said, runtime pm is not one hardware property, I think it is not >>> suitable if we introduce one device property to enable/disable runtime >>> pm. >> >> we already this functionality exposed on sysfs. > > From my understanding, Mathias suggested me to add one device property > (name like "usb-host-runtimePM") by platform_device_add_properties() > to enable/disable runtime PM when creating platform device, like > usb3_lpm_capable: > > if (dwc->usb3_lpm_capable) > props[prop_idx++].name =3D "usb3-lpm-capable"; > > ret =3D platform_device_add_properties(xhci, props); > if (ret) { > dev_err(dwc->dev, "failed to add properties to xHCI\n"); > goto err1; > } > > What I think It is not suitable to introduce one device property like > above to enable/disable runtime PM, it is not one hardware attribute. yeah, that's silly. We already have means for doing that: my_probe() { [...] pm_runtime_enable(dev); pm_runtime_forbid(dev); [...] return 0; } >>> Secondly, we only can resume the xhci platform device by getting the >>> xhci usage counter from gadget driver, since the cable plug in/out >>> events only can be notified to glue layer of gadget driver(like dwc3 >>> glue layer). That means if we want to suspend xhci platform device, we >> >> this is a problem with the glue layer, IMO. It should be something like >> so: >> >> static irqreturn_t dwc3_foobar_wakeup(int irq, void *_glue) >> { >> struct dwc3_foobar_glue *glue =3D _glue; >> u32 reg; >> >> reg =3D real(glue->base, OFFSET); >> if (reg & CONNECT) >> pm_runtime_resume(&glue->dwc3); >> >> return IRQ_HANDLED; >> } >> >> then dwc3's ->runtime_resume() should check if the event is supposed to >> be handled by host or peripheral by checking which mode it was before >> suspend and making the assumption that we don't change modes while >> suspended. Something like: >> >> static int dwc3_runtime_resume(struct device *dev) >> { >> struct dwc3 *dwc =3D dev_get_drvdata(dev); >> >> [...] >> >> if (dwc->is_host) >> pm_runtime_resume(dwc->xhci.dev); >> >> [...] >> >> return 0; >> } > > Yeah, if we don't need to get xhci usage counter in xhci_plat_probe() > to avoid affecting other controller's runtime PM, we can do like this > and do not need to get/put counter. why do you need to get xhci's usage counter in xhci_plat_probe() ? And why would one xhci affect the other? They are different struct device instances and, thus, have different pm usage counter. How would one xhci's pm_runtime affect another? PM runtime is not per driver, it's per device. =2D-=20 balbi --=-=-= Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAEBCAAdFiEElLzh7wn96CXwjh2IzL64meEamQYFAljSPTkACgkQzL64meEa mQZO/xAAqMEkHacsq5kzb75fjXoWN8SOl+/oCG9dytws6neXRxgw8VNKSltNDx/i Qf3KfAUUDFjAFWbQP72vZcClkKW/DbMKdWSejhm4Oc4JFU1gXmvjZ/6JXufUlg9D WWp1/uavGgpNbBrAmsLeqBaxP9JhrHmtS9sevQTU2YnwO8ZYWcVxKg9ngFyexijo oknHgtJK/2AyOaAmWs/X0/vEjV3/vY8k/QDPr8DckzCoV1h3Z0x2SxZgr81ivvz3 gTaFMrQaTB8zes3vMi3UBbllhke+Ql9MUQnMmyHMVBluoj2N6k4BC+ZcYHBFGIpw gszDArCuHZa2CyD50MPV0AR7qpM+WtKQhtt9eCojphYruSEuiiKKj8mnQbGxC1Zd 5NRiULslNTDPVMsbhE+v8BAys30XsxX5Ty9x4MpqCwYgMTrlyFmGKlhYjJDNKQ3D r9btiqBHE4aKynsriaK6jCYtTk1hgVv7Rk3HhYxb1mGrCd4qE5Aaj6GBRCBzPdBS xHD9M6oPY5SPR/C3mFKFPDUR49tX5T91UDz/cxWSpqmKyusHyTk0i3yBUwpEfURg i7ZAEfsLDrrTGCF3P5zixbRNyxJBQF6PlC3NQqarP6m1qlBv3mtVlDnRwY0rK6pm YxI4hukNZr4detUugz78o+XBq3jNHZX5cCBDyjXM57ae8zL2W+c= =6kZt -----END PGP SIGNATURE----- --=-=-=--