From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758616Ab3DCNyj (ORCPT ); Wed, 3 Apr 2013 09:54:39 -0400 Received: from arroyo.ext.ti.com ([192.94.94.40]:38627 "EHLO arroyo.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757497Ab3DCNyh (ORCPT ); Wed, 3 Apr 2013 09:54:37 -0400 Date: Wed, 3 Apr 2013 16:54:14 +0300 From: Felipe Balbi To: Vivek Gautam CC: , Kishon Vijay Abraham I , Vivek Gautam , , , , , , , , , , , , Subject: Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management Message-ID: <20130403135414.GG14680@arwen.pp.htv.fi> Reply-To: References: <1364824448-14732-1-git-send-email-gautam.vivek@samsung.com> <1364824448-14732-2-git-send-email-gautam.vivek@samsung.com> <515BB951.40702@ti.com> <20130403081532.GE25837@arwen.pp.htv.fi> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="N8NGGaQn1mzfvaPg" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --N8NGGaQn1mzfvaPg Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable HI, On Wed, Apr 03, 2013 at 06:42:48PM +0530, Vivek Gautam wrote: > >> >> Adding APIs to handle runtime power management on PHY > >> >> devices. PHY consumers may need to wake-up/suspend PHYs > >> >> when they work across autosuspend. > >> >> > >> >> Signed-off-by: Vivek Gautam > >> >> --- > >> >> include/linux/usb/phy.h | 141 > >> >> +++++++++++++++++++++++++++++++++++++++++++++++ > >> >> 1 files changed, 141 insertions(+), 0 deletions(-) > >> >> > >> >> diff --git a/include/linux/usb/phy.h b/include/linux/usb/phy.h > >> >> index 6b5978f..01bf9c1 100644 > >> >> --- a/include/linux/usb/phy.h > >> >> +++ b/include/linux/usb/phy.h > >> >> @@ -297,4 +297,145 @@ static inline const char *usb_phy_type_string= (enum > >> >> usb_phy_type type) > >> >> return "UNKNOWN PHY TYPE"; > >> >> } > >> >> } > >> >> + > >> >> +static inline void usb_phy_autopm_enable(struct usb_phy *x) > >> >> +{ > >> >> + if (!x || !x->dev) { > >> >> + dev_err(x->dev, "no PHY or attached device availabl= e\n"); > >> >> + return; > >> >> + } > >> >> + > >> >> + pm_runtime_enable(x->dev); > >> >> +} > >> > > >> > > >> > IMO we need not have wrapper APIs for runtime_enable and runtime_dis= able > >> > here. Generally runtime_enable and runtime_disable is done in probe = and > >> > remove of a driver respectively. So it's better to leave the > >> > runtime_enable/runtime_disable to be done in *phy provider* driver t= han > >> > having an API for it to be done by *phy user* driver. Felipe, what d= o you > >> > think? > >> > >> Thanks!! > >> That's very true, runtime_enable() and runtime_disable() calls are mad= e by > >> *phy_provider* only. But a querry here. > >> Wouldn't in any case a PHY consumer might want to disable runtime_pm o= n PHY ? > >> Say, when consumer failed to suspend the PHY properly > >> (*put_sync(phy->dev)* fails), how much sure is the consumer about the > >> state of PHY ? > > > > no no, wait a minute. We might not want to enable runtime pm for the PHY > > until the UDC says it can handle runtime pm, no ? I guess this makes a > > bit of sense (at least in my head :-p). > > > > Imagine if PHY is runtime suspended but e.g. DWC3 isn't runtime pm > > enabled... Does it make sense to leave that control to the USB > > controller drivers ? > > > > I'm open for suggestions >=20 > Of course unless the PHY consumer can handle runtime PM for PHY, > PHY should not ideally be going into runtime_suspend. >=20 > Actually trying out few things, here are my observations >=20 > Enabling runtime_pm on PHY pushes PHY to go into runtime_suspend state. > But a device detection wakes up DWC3 controller, and if i don't wake > up PHY (using get_sync(phy->dev)) here > in runtime_resume() callback of DWC3, i don't get PHY back in active stat= e. > So it becomes the duty of DWC3 controller to handle PHY's sleep and wake-= up. > Thereby it becomes logical that DWC3 controller has the right to > enable runtime_pm > of PHY. >=20 > But there's a catch here. if there are multiple consumers of PHY (like > USB2 type PHY can > have DWC3 controller as well as EHCI/OHCI or even HSGadget) then in that = case, > only one of the consumer can enable runtime_pm on PHY. So who decides thi= s. >=20 > Aargh!! lot of confusion here :-( hmmm, maybe add a flag to struct usb_phy and check it on usb_phy_autopm_enable() ?? How does usbcore handle it ? They request class drivers to pass supports_autosuspend, but while we should have a similar flag, that's not enough. We also need a flag to tell us when pm_runtime has already been enabled. So how about: usb_phy_autopm_enable() { if (!phy->suports_autosuspend) return -ENOSYS; if (phy->autosuspend_enabled) return 0; phy->autosuspend_enabled =3D true; return pm_runtime_enable(phy->dev); } ??? --=20 balbi --N8NGGaQn1mzfvaPg Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) iQIcBAEBAgAGBQJRXDSGAAoJEIaOsuA1yqREVDQQAIr+4rqnr5SvAfNJTzELP4a/ A42ZFuDjrFIrIMPtqGZZkjXGkQ98sTAaIER0D+rXXDeV7bJLNN+8g2yfszO2jrAF bATNzFLfA6PH3zl2OpAXwMe/Hk6b0uzndBmSwrr7IeeEvBy5v7myFp7TSf4e4lKF Ki4dumLXOl4p1HBsmm7QpF6jH2+MIvCS9oWXwvqSfL6apFakLasYO+CVtvZmhXy7 BCbzL361pHm/JhwOnoTgIWy7lySNxdLloAfQ4vqzemI16/oTlev1wnIL8iZDG9u1 ANTimhXCE9z0RZSj2SRsqugAppULeZkYsmrYMX5DQMEK0CDr5OGSuV0QQMvhkLi/ wjTmOosajail3pT7Gr4AUMhi+wXcGHtiKZAl68Rv42W8M0s2Jpc4Ho2jlIVOFdlA gzQZw1juLW+qKycx20IsK8WWghHaM8tQ56rvlyI94d90mbuwnEeDLGCaoHVMFT2D AlicnpsWFrNUqRQIzx8uvbrXdQJcUkpaWPV/9BA9KlwgSXtoB1TI/9pmjjvx9xWI kzikaLMG21IVee7ymmsGbB88K515tOyIU9+P5RT7YKGMgow22brs9dZP64GBJ2CS ZlePYZaWagQNwV1FSUrDZ58BN/O8xy/8KhM2tn84dq07JeSEDTUScYW3GqnSHwRZ 3obrP50rqiKKG8oXfhKB =WX/N -----END PGP SIGNATURE----- --N8NGGaQn1mzfvaPg--