| Message ID | 1364824448-14732-2-git-send-email-gautam.vivek@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 01, 2013 at 07:24:00PM +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 <gautam.vivek@samsung.com> > --- > 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 available\n"); > + return; > + } wrong indentation, also, I'm not sure we should allow calls with NULL pointers. Perhaps a WARN() so we get API offenders early enough ?
Hi, On Tue, Apr 2, 2013 at 1:53 PM, Felipe Balbi <balbi@ti.com> wrote: > On Mon, Apr 01, 2013 at 07:24:00PM +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 <gautam.vivek@samsung.com> >> --- >> 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 available\n"); >> + return; >> + } > > wrong indentation, also, I'm not sure we should allow calls with NULL > pointers. Perhaps a WARN() so we get API offenders early enough ? True, bad coding style :-( We should be handling dev_err with a NULL pointer. Will just keep here: if (WARN_ON(!x->dev)) return .... ;
Hi, On Tue, Apr 02, 2013 at 04:04:01PM +0530, Vivek Gautam wrote: > > On Mon, Apr 01, 2013 at 07:24:00PM +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 <gautam.vivek@samsung.com> > >> --- > >> 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 available\n"); > >> + return; > >> + } > > > > wrong indentation, also, I'm not sure we should allow calls with NULL > > pointers. Perhaps a WARN() so we get API offenders early enough ? > > True, bad coding style :-( > We should be handling dev_err with a NULL pointer. > Will just keep here: > if (WARN_ON(!x->dev)) > return .... ; right, but I guess: if (WARN(!x || !x->dev, "Invalid parameters\n")) return -EINVAL; would be better ??
Hi, On Tue, Apr 2, 2013 at 5:40 PM, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Tue, Apr 02, 2013 at 04:04:01PM +0530, Vivek Gautam wrote: >> > On Mon, Apr 01, 2013 at 07:24:00PM +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 <gautam.vivek@samsung.com> >> >> --- >> >> 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 available\n"); >> >> + return; >> >> + } >> > >> > wrong indentation, also, I'm not sure we should allow calls with NULL >> > pointers. Perhaps a WARN() so we get API offenders early enough ? >> >> True, bad coding style :-( >> We should be handling dev_err with a NULL pointer. >> Will just keep here: >> if (WARN_ON(!x->dev)) >> return .... ; > > right, but I guess: > > if (WARN(!x || !x->dev, "Invalid parameters\n")) > return -EINVAL; > > would be better ?? Yea, better. Thanks Will amend this accordingly.
Hi, On Monday 01 April 2013 07:24 PM, 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 <gautam.vivek@samsung.com> > --- > 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 available\n"); > + return; > + } > + > + pm_runtime_enable(x->dev); > +} IMO we need not have wrapper APIs for runtime_enable and runtime_disable 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 than having an API for it to be done by *phy user* driver. Felipe, what do you think? Thanks Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Kishon, On Wed, Apr 3, 2013 at 10:38 AM, Kishon Vijay Abraham I <kishon@ti.com> wrote: > Hi, > > > On Monday 01 April 2013 07:24 PM, 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 <gautam.vivek@samsung.com> >> --- >> 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 available\n"); >> + return; >> + } >> + >> + pm_runtime_enable(x->dev); >> +} > > > IMO we need not have wrapper APIs for runtime_enable and runtime_disable > 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 than > having an API for it to be done by *phy user* driver. Felipe, what do you > think? Thanks!! That's very true, runtime_enable() and runtime_disable() calls are made by *phy_provider* only. But a querry here. Wouldn't in any case a PHY consumer might want to disable runtime_pm on 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 ?
Hi, On Wed, Apr 03, 2013 at 11:48:39AM +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 <gautam.vivek@samsung.com> > >> --- > >> 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 available\n"); > >> + return; > >> + } > >> + > >> + pm_runtime_enable(x->dev); > >> +} > > > > > > IMO we need not have wrapper APIs for runtime_enable and runtime_disable > > 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 than > > having an API for it to be done by *phy user* driver. Felipe, what do you > > think? > > Thanks!! > That's very true, runtime_enable() and runtime_disable() calls are made by > *phy_provider* only. But a querry here. > Wouldn't in any case a PHY consumer might want to disable runtime_pm on 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
Hi Felipe, On Wed, Apr 3, 2013 at 1:45 PM, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Wed, Apr 03, 2013 at 11:48:39AM +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 <gautam.vivek@samsung.com> >> >> --- >> >> 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 available\n"); >> >> + return; >> >> + } >> >> + >> >> + pm_runtime_enable(x->dev); >> >> +} >> > >> > >> > IMO we need not have wrapper APIs for runtime_enable and runtime_disable >> > 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 than >> > having an API for it to be done by *phy user* driver. Felipe, what do you >> > think? >> >> Thanks!! >> That's very true, runtime_enable() and runtime_disable() calls are made by >> *phy_provider* only. But a querry here. >> Wouldn't in any case a PHY consumer might want to disable runtime_pm on 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 Of course unless the PHY consumer can handle runtime PM for PHY, PHY should not ideally be going into runtime_suspend. Actually trying out few things, here are my observations 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 state. 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. 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 this. Aargh!! lot of confusion here :-( > > -- > balbi
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 <gautam.vivek@samsung.com> > >> >> --- > >> >> 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 available\n"); > >> >> + return; > >> >> + } > >> >> + > >> >> + pm_runtime_enable(x->dev); > >> >> +} > >> > > >> > > >> > IMO we need not have wrapper APIs for runtime_enable and runtime_disable > >> > 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 than > >> > having an API for it to be done by *phy user* driver. Felipe, what do you > >> > think? > >> > >> Thanks!! > >> That's very true, runtime_enable() and runtime_disable() calls are made by > >> *phy_provider* only. But a querry here. > >> Wouldn't in any case a PHY consumer might want to disable runtime_pm on 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 > > Of course unless the PHY consumer can handle runtime PM for PHY, > PHY should not ideally be going into runtime_suspend. > > Actually trying out few things, here are my observations > > 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 state. > 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. > > 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 this. > > 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 = true; return pm_runtime_enable(phy->dev); } ???
Hi, On Wed, Apr 03, 2013 at 04:54:14PM +0300, Felipe Balbi wrote: > > >> >> +static inline void usb_phy_autopm_enable(struct usb_phy *x) > > >> >> +{ > > >> >> + if (!x || !x->dev) { > > >> >> + dev_err(x->dev, "no PHY or attached device available\n"); > > >> >> + return; > > >> >> + } > > >> >> + > > >> >> + pm_runtime_enable(x->dev); > > >> >> +} > > >> > > > >> > > > >> > IMO we need not have wrapper APIs for runtime_enable and runtime_disable > > >> > 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 than > > >> > having an API for it to be done by *phy user* driver. Felipe, what do you > > >> > think? > > >> > > >> Thanks!! > > >> That's very true, runtime_enable() and runtime_disable() calls are made by > > >> *phy_provider* only. But a querry here. > > >> Wouldn't in any case a PHY consumer might want to disable runtime_pm on 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 > > > > Of course unless the PHY consumer can handle runtime PM for PHY, > > PHY should not ideally be going into runtime_suspend. > > > > Actually trying out few things, here are my observations > > > > 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 state. > > 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. > > > > 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 this. > > > > 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 = true; > return pm_runtime_enable(phy->dev); > } > > ??? and of course I missed the fact that pm_runtime_enable returns nothing :-) But you got my point.
Hi, On Wed, Apr 3, 2013 at 7:26 PM, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Wed, Apr 03, 2013 at 04:54:14PM +0300, Felipe Balbi wrote: >> > >> >> +static inline void usb_phy_autopm_enable(struct usb_phy *x) >> > >> >> +{ >> > >> >> + if (!x || !x->dev) { >> > >> >> + dev_err(x->dev, "no PHY or attached device available\n"); >> > >> >> + return; >> > >> >> + } >> > >> >> + >> > >> >> + pm_runtime_enable(x->dev); >> > >> >> +} >> > >> > >> > >> > >> > >> > IMO we need not have wrapper APIs for runtime_enable and runtime_disable >> > >> > 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 than >> > >> > having an API for it to be done by *phy user* driver. Felipe, what do you >> > >> > think? >> > >> >> > >> Thanks!! >> > >> That's very true, runtime_enable() and runtime_disable() calls are made by >> > >> *phy_provider* only. But a querry here. >> > >> Wouldn't in any case a PHY consumer might want to disable runtime_pm on 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 >> > >> > Of course unless the PHY consumer can handle runtime PM for PHY, >> > PHY should not ideally be going into runtime_suspend. >> > >> > Actually trying out few things, here are my observations >> > >> > 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 state. >> > 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. >> > >> > 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 this. >> > >> > 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. True >> >> So how about: >> >> usb_phy_autopm_enable() >> { >> if (!phy->suports_autosuspend) >> return -ENOSYS; >> >> if (phy->autosuspend_enabled) >> return 0; >> >> phy->autosuspend_enabled = true; >> return pm_runtime_enable(phy->dev); >> } >> >> ??? Great > > and of course I missed the fact that pm_runtime_enable returns nothing > :-) But you got my point. Yea, this is a way around this. :-) Just one more query :-) Lets suppose DWC3 enables runtime_pm on USB 2 type phy, it will try to go into suspend state and thereby call runtime_suspend(), if any. And PHY will come to active state only when its consumer wakes it up, and this consumer is operational only when its related PHY is in fully functional state. So do we have a situation in which this PHY goes into low power state in its runtime_suspend(), resulting in non-detection of devices on further attach (since PHY is in low power state) ? Will the controller (like EHCI/OHCI) be functional now ?
Hi, On Wed, Apr 03, 2013 at 07:40:44PM +0530, Vivek Gautam wrote: > >> > >> >> +static inline void usb_phy_autopm_enable(struct usb_phy *x) > >> > >> >> +{ > >> > >> >> + if (!x || !x->dev) { > >> > >> >> + dev_err(x->dev, "no PHY or attached device available\n"); > >> > >> >> + return; > >> > >> >> + } > >> > >> >> + > >> > >> >> + pm_runtime_enable(x->dev); > >> > >> >> +} > >> > >> > > >> > >> > > >> > >> > IMO we need not have wrapper APIs for runtime_enable and runtime_disable > >> > >> > 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 than > >> > >> > having an API for it to be done by *phy user* driver. Felipe, what do you > >> > >> > think? > >> > >> > >> > >> Thanks!! > >> > >> That's very true, runtime_enable() and runtime_disable() calls are made by > >> > >> *phy_provider* only. But a querry here. > >> > >> Wouldn't in any case a PHY consumer might want to disable runtime_pm on 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 > >> > > >> > Of course unless the PHY consumer can handle runtime PM for PHY, > >> > PHY should not ideally be going into runtime_suspend. > >> > > >> > Actually trying out few things, here are my observations > >> > > >> > 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 state. > >> > 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. > >> > > >> > 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 this. > >> > > >> > 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. > > True > > >> > >> So how about: > >> > >> usb_phy_autopm_enable() > >> { > >> if (!phy->suports_autosuspend) > >> return -ENOSYS; > >> > >> if (phy->autosuspend_enabled) > >> return 0; > >> > >> phy->autosuspend_enabled = true; > >> return pm_runtime_enable(phy->dev); > >> } > >> > >> ??? > > Great > > > > > and of course I missed the fact that pm_runtime_enable returns nothing > > :-) But you got my point. > > Yea, this is a way around this. :-) > > Just one more query :-) > > Lets suppose DWC3 enables runtime_pm on USB 2 type phy, > it will try to go into suspend state and thereby call runtime_suspend(), if any. > And PHY will come to active state only when its consumer wakes it up, > and this consumer is operational > only when its related PHY is in fully functional state. > So do we have a situation in which this PHY goes into low power state > in its runtime_suspend(), > resulting in non-detection of devices on further attach (since PHY is > in low power state) ? > > Will the controller (like EHCI/OHCI) be functional now ? ehci/ohci need to cope with that by calling usb_phy_autopm_get_sync(), right ? (so does DWC3 :-)
On Wed, Apr 3, 2013 at 7:48 PM, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Wed, Apr 03, 2013 at 07:40:44PM +0530, Vivek Gautam wrote: >> >> > >> >> +static inline void usb_phy_autopm_enable(struct usb_phy *x) >> >> > >> >> +{ >> >> > >> >> + if (!x || !x->dev) { >> >> > >> >> + dev_err(x->dev, "no PHY or attached device available\n"); >> >> > >> >> + return; >> >> > >> >> + } >> >> > >> >> + >> >> > >> >> + pm_runtime_enable(x->dev); >> >> > >> >> +} >> >> > >> > >> >> > >> > >> >> > >> > IMO we need not have wrapper APIs for runtime_enable and runtime_disable >> >> > >> > 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 than >> >> > >> > having an API for it to be done by *phy user* driver. Felipe, what do you >> >> > >> > think? >> >> > >> >> >> > >> Thanks!! >> >> > >> That's very true, runtime_enable() and runtime_disable() calls are made by >> >> > >> *phy_provider* only. But a querry here. >> >> > >> Wouldn't in any case a PHY consumer might want to disable runtime_pm on 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 >> >> > >> >> > Of course unless the PHY consumer can handle runtime PM for PHY, >> >> > PHY should not ideally be going into runtime_suspend. >> >> > >> >> > Actually trying out few things, here are my observations >> >> > >> >> > 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 state. >> >> > 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. >> >> > >> >> > 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 this. >> >> > >> >> > 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. >> >> True >> >> >> >> >> So how about: >> >> >> >> usb_phy_autopm_enable() >> >> { >> >> if (!phy->suports_autosuspend) >> >> return -ENOSYS; >> >> >> >> if (phy->autosuspend_enabled) >> >> return 0; >> >> >> >> phy->autosuspend_enabled = true; >> >> return pm_runtime_enable(phy->dev); >> >> } >> >> >> >> ??? >> >> Great >> >> > >> > and of course I missed the fact that pm_runtime_enable returns nothing >> > :-) But you got my point. >> >> Yea, this is a way around this. :-) >> >> Just one more query :-) >> >> Lets suppose DWC3 enables runtime_pm on USB 2 type phy, >> it will try to go into suspend state and thereby call runtime_suspend(), if any. >> And PHY will come to active state only when its consumer wakes it up, >> and this consumer is operational >> only when its related PHY is in fully functional state. >> So do we have a situation in which this PHY goes into low power state >> in its runtime_suspend(), >> resulting in non-detection of devices on further attach (since PHY is >> in low power state) ? >> >> Will the controller (like EHCI/OHCI) be functional now ? > > ehci/ohci need to cope with that by calling usb_phy_autopm_get_sync(), > right ? (so does DWC3 :-) Yes ofcourse. So PHYs (in their runtime_suspend) should not be pulled into a state wherein even the controllers become in-operational. Thereby no attach-detection, and controller doesn't wake up to be able to usb_phy_autopm_get_sync() Sorry for so much noise, i am acting like slow study ;-) > > -- > balbi
On Wed, 3 Apr 2013, Felipe Balbi wrote: > > Lets suppose DWC3 enables runtime_pm on USB 2 type phy, > > it will try to go into suspend state and thereby call runtime_suspend(), if any. > > And PHY will come to active state only when its consumer wakes it up, > > and this consumer is operational > > only when its related PHY is in fully functional state. > > So do we have a situation in which this PHY goes into low power state > > in its runtime_suspend(), > > resulting in non-detection of devices on further attach (since PHY is > > in low power state) ? > > > > Will the controller (like EHCI/OHCI) be functional now ? > > ehci/ohci need to cope with that by calling usb_phy_autopm_get_sync(), > right ? (so does DWC3 :-) Maybe you guys have already got this all figured out -- if so, feel free to ignore this email. Some subsystems handle this issue by calling pm_runtime_get_sync() before probing a driver and pm_runtime_put_sync() after unbinding the driver. If the driver is runtime-PM-enabled, it then does its own put_sync near the end of its probe routine and get_sync in its release routine. With PHYs you don't have probing and releasing, but you do have consumers registering and unregistering themselves, which is about the same thing. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wed, Apr 03, 2013 at 02:14:02PM -0400, Alan Stern wrote: > > > Lets suppose DWC3 enables runtime_pm on USB 2 type phy, > > > it will try to go into suspend state and thereby call runtime_suspend(), if any. > > > And PHY will come to active state only when its consumer wakes it up, > > > and this consumer is operational > > > only when its related PHY is in fully functional state. > > > So do we have a situation in which this PHY goes into low power state > > > in its runtime_suspend(), > > > resulting in non-detection of devices on further attach (since PHY is > > > in low power state) ? > > > > > > Will the controller (like EHCI/OHCI) be functional now ? > > > > ehci/ohci need to cope with that by calling usb_phy_autopm_get_sync(), > > right ? (so does DWC3 :-) > > Maybe you guys have already got this all figured out -- if so, feel > free to ignore this email. > > Some subsystems handle this issue by calling pm_runtime_get_sync() > before probing a driver and pm_runtime_put_sync() after unbinding the > driver. If the driver is runtime-PM-enabled, it then does its own > put_sync near the end of its probe routine and get_sync in its release > routine. sounds a bit 'fishy' to me... So a separate entity would call pm_runtime_get_sync(), even when we don't have registered dev_pm_ops, then drivers need to check if runtime_pm is enabled and call pm_runtime_put*() conditionally before returning from probe(). One remove, we might have another issue: device is already runtime_suspended (due to e.g. autosuspend) when module is removed, a call to pm_runtime_put_sync() will be unbalanced. No ?
Hi, On Thu, Apr 4, 2013 at 12:48 PM, Felipe Balbi <balbi@ti.com> wrote: > Hi, > > On Wed, Apr 03, 2013 at 02:14:02PM -0400, Alan Stern wrote: >> > > Lets suppose DWC3 enables runtime_pm on USB 2 type phy, >> > > it will try to go into suspend state and thereby call runtime_suspend(), if any. >> > > And PHY will come to active state only when its consumer wakes it up, >> > > and this consumer is operational >> > > only when its related PHY is in fully functional state. >> > > So do we have a situation in which this PHY goes into low power state >> > > in its runtime_suspend(), >> > > resulting in non-detection of devices on further attach (since PHY is >> > > in low power state) ? >> > > >> > > Will the controller (like EHCI/OHCI) be functional now ? >> > >> > ehci/ohci need to cope with that by calling usb_phy_autopm_get_sync(), >> > right ? (so does DWC3 :-) >> >> Maybe you guys have already got this all figured out -- if so, feel >> free to ignore this email. >> >> Some subsystems handle this issue by calling pm_runtime_get_sync() >> before probing a driver and pm_runtime_put_sync() after unbinding the >> driver. If the driver is runtime-PM-enabled, it then does its own >> put_sync near the end of its probe routine and get_sync in its release >> routine. > > sounds a bit 'fishy' to me... So a separate entity would call > pm_runtime_get_sync(), even when we don't have registered dev_pm_ops, > then drivers need to check if runtime_pm is enabled and call > pm_runtime_put*() conditionally before returning from probe(). One > remove, we might have another issue: device is already runtime_suspended > (due to e.g. autosuspend) when module is removed, a call to > pm_runtime_put_sync() will be unbalanced. No ? May be i am misinterpreting !! If PHYs are runtime-PM enabled (PHY probe calls *runtime_enable*), then the consumers need to call pm_runtime_get_sync whever they want to access PHY. Besides PHYs also need to *put_sync* just before their probe is finishing, so that it's availbale for autosuspend. I, however didn't understand the need of PHY to *get_sync* itself in release routine.
Hi, On Thu, Apr 04, 2013 at 02:26:51PM +0530, Vivek Gautam wrote: > >> > > Lets suppose DWC3 enables runtime_pm on USB 2 type phy, > >> > > it will try to go into suspend state and thereby call runtime_suspend(), if any. > >> > > And PHY will come to active state only when its consumer wakes it up, > >> > > and this consumer is operational > >> > > only when its related PHY is in fully functional state. > >> > > So do we have a situation in which this PHY goes into low power state > >> > > in its runtime_suspend(), > >> > > resulting in non-detection of devices on further attach (since PHY is > >> > > in low power state) ? > >> > > > >> > > Will the controller (like EHCI/OHCI) be functional now ? > >> > > >> > ehci/ohci need to cope with that by calling usb_phy_autopm_get_sync(), > >> > right ? (so does DWC3 :-) > >> > >> Maybe you guys have already got this all figured out -- if so, feel > >> free to ignore this email. > >> > >> Some subsystems handle this issue by calling pm_runtime_get_sync() > >> before probing a driver and pm_runtime_put_sync() after unbinding the > >> driver. If the driver is runtime-PM-enabled, it then does its own > >> put_sync near the end of its probe routine and get_sync in its release > >> routine. > > > > sounds a bit 'fishy' to me... So a separate entity would call > > pm_runtime_get_sync(), even when we don't have registered dev_pm_ops, > > then drivers need to check if runtime_pm is enabled and call > > pm_runtime_put*() conditionally before returning from probe(). One > > remove, we might have another issue: device is already runtime_suspended > > (due to e.g. autosuspend) when module is removed, a call to > > pm_runtime_put_sync() will be unbalanced. No ? > > May be i am misinterpreting !! > If PHYs are runtime-PM enabled (PHY probe calls *runtime_enable*), > then the consumers > need to call pm_runtime_get_sync whever they want to access PHY. Alright, so here's my understanding: I suggested letting e.g. DWC3 enable the PHY's runtime_pm; Alan said that it could be done before that so that DWC3 sees an enabled PHY during probe.
On Thu, 4 Apr 2013, Felipe Balbi wrote: > > >> Some subsystems handle this issue by calling pm_runtime_get_sync() > > >> before probing a driver and pm_runtime_put_sync() after unbinding the > > >> driver. If the driver is runtime-PM-enabled, it then does its own > > >> put_sync near the end of its probe routine and get_sync in its release > > >> routine. > > > > > > sounds a bit 'fishy' to me... So a separate entity would call > > > pm_runtime_get_sync(), even when we don't have registered dev_pm_ops, I don't know what you mean by "separate entity". The PHY's subsystem would handle this. After all, the subsystem has to handle registering the PHY in the first place. If the PHY doesn't have a dev_pm_ops, why are you talking about doing runtime PM on it? That doesn't make any sense. > > > then drivers need to check if runtime_pm is enabled and call > > > pm_runtime_put*() conditionally before returning from probe(). One They don't have to check. If CONFIG_PM_RUNTIME isn't set or the target is runtime-PM-disabled then pm_runtime_put* is essentially a no-op (in the disabled case it decrements the usage counter but doesn't do anything else). One possible complication I did not mention: The scheme described above assumes the device that uses the PHY will always be registered on the same type of bus. If the device can be used on multiple bus types (and hence in multiple subsystems) then things aren't so simple, because some of the subsystems might support runtime PM and others might not. (You may very well run into this problem with USB controllers that are sometimes on a PCI bus and sometimes on a platform bus.) In this case, the driver needs to adapt to the subsystem's capabilities. Presumably the bus-glue part of the driver takes care of this. > > > remove, we might have another issue: device is already runtime_suspended > > > (due to e.g. autosuspend) when module is removed, a call to > > > pm_runtime_put_sync() will be unbalanced. No ? No. I left out some of the details. For one thing, the subsystem is careful to do a runtime resume before calling the driver's remove method. (Also, if you look over my original description carefully, you'll see that there are no unbalanced calls -- even if the device is already runtime suspended when the driver is unbound.) For another, the subsystem needs to check before calling the driver's runtime-PM methods. There are two brief windows during which the driver isn't in charge of the device even though dev->driver is set. Those windows occur in the subsystem's probe routine (before it calls the driver's probe method) and in the subsystem's remove routine (after it calls the driver's remove method). At such times, the subsystem's runtime-PM handlers must be careful _not_ to call the driver's runtime-PM routines. > > May be i am misinterpreting !! > > If PHYs are runtime-PM enabled (PHY probe calls *runtime_enable*), > > then the consumers > > need to call pm_runtime_get_sync whever they want to access PHY. No, because in addition to being runtime-PM enabled, the PHY should automatically be runtime resumed when the consumer registers itself as a user of the PHY. Therefore the consumer doesn't need to do anything at all -- which is good for consumers that aren't runtime-PM aware. > Alright, so here's my understanding: > > I suggested letting e.g. DWC3 enable the PHY's runtime_pm; Alan said > that it could be done before that so that DWC3 sees an enabled PHY > during probe. Basically right. Help me to understand the overall situation a little better: What code registers the PHY initially? What routine does the DWC3 driver call to register itself as a consumer of the PHY? Likewise, what routine does it call to unregister itself? Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 02 April 2013 06:10 PM, Vivek Gautam wrote: > Hi, > > > On Tue, Apr 2, 2013 at 5:40 PM, Felipe Balbi <balbi@ti.com> wrote: >> Hi, >> >> On Tue, Apr 02, 2013 at 04:04:01PM +0530, Vivek Gautam wrote: >>>> On Mon, Apr 01, 2013 at 07:24:00PM +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 <gautam.vivek@samsung.com> >>>>> --- >>>>> 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 available\n"); >>>>> + return; >>>>> + } >>>> >>>> wrong indentation, also, I'm not sure we should allow calls with NULL >>>> pointers. Perhaps a WARN() so we get API offenders early enough ? >>> >>> True, bad coding style :-( >>> We should be handling dev_err with a NULL pointer. >>> Will just keep here: >>> if (WARN_ON(!x->dev)) >>> return .... ; >> >> right, but I guess: >> >> if (WARN(!x || !x->dev, "Invalid parameters\n")) >> return -EINVAL; >> >> would be better ?? btw, shouldn't it be IS_ERR(x)? Thanks Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Thu, Apr 18, 2013 at 05:20:11PM +0530, Kishon Vijay Abraham I 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 <gautam.vivek@samsung.com> > >>>>>--- > >>>>> 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 available\n"); > >>>>>+ return; > >>>>>+ } > >>>> > >>>>wrong indentation, also, I'm not sure we should allow calls with NULL > >>>>pointers. Perhaps a WARN() so we get API offenders early enough ? > >>> > >>>True, bad coding style :-( > >>>We should be handling dev_err with a NULL pointer. > >>>Will just keep here: > >>>if (WARN_ON(!x->dev)) > >>> return .... ; > >> > >>right, but I guess: > >> > >>if (WARN(!x || !x->dev, "Invalid parameters\n")) > >> return -EINVAL; > >> > >>would be better ?? > > btw, shouldn't it be IS_ERR(x)? not in this case, since we're trying to catch users passing NULL to as the phy argument.
Hi, On Thu, Apr 4, 2013 at 8:16 PM, Alan Stern <stern@rowland.harvard.edu> wrote: Apologies for delay in replying. > On Thu, 4 Apr 2013, Felipe Balbi wrote: > >> > >> Some subsystems handle this issue by calling pm_runtime_get_sync() >> > >> before probing a driver and pm_runtime_put_sync() after unbinding the >> > >> driver. If the driver is runtime-PM-enabled, it then does its own >> > >> put_sync near the end of its probe routine and get_sync in its release >> > >> routine. >> > > >> > > sounds a bit 'fishy' to me... So a separate entity would call >> > > pm_runtime_get_sync(), even when we don't have registered dev_pm_ops, > > I don't know what you mean by "separate entity". The PHY's subsystem > would handle this. After all, the subsystem has to handle registering > the PHY in the first place. > > If the PHY doesn't have a dev_pm_ops, why are you talking about doing > runtime PM on it? That doesn't make any sense. > >> > > then drivers need to check if runtime_pm is enabled and call >> > > pm_runtime_put*() conditionally before returning from probe(). One > > They don't have to check. If CONFIG_PM_RUNTIME isn't set or the target > is runtime-PM-disabled then pm_runtime_put* is essentially a no-op (in > the disabled case it decrements the usage counter but doesn't do > anything else). > > One possible complication I did not mention: The scheme described above > assumes the device that uses the PHY will always be registered on the > same type of bus. If the device can be used on multiple bus types (and > hence in multiple subsystems) then things aren't so simple, because > some of the subsystems might support runtime PM and others might not. > (You may very well run into this problem with USB controllers that are > sometimes on a PCI bus and sometimes on a platform bus.) In this case, > the driver needs to adapt to the subsystem's capabilities. Presumably > the bus-glue part of the driver takes care of this. > >> > > remove, we might have another issue: device is already runtime_suspended >> > > (due to e.g. autosuspend) when module is removed, a call to >> > > pm_runtime_put_sync() will be unbalanced. No ? > > No. I left out some of the details. For one thing, the subsystem is > careful to do a runtime resume before calling the driver's remove > method. (Also, if you look over my original description carefully, > you'll see that there are no unbalanced calls -- even if the device is > already runtime suspended when the driver is unbound.) > > For another, the subsystem needs to check before calling the driver's > runtime-PM methods. There are two brief windows during which the > driver isn't in charge of the device even though dev->driver is set. > Those windows occur in the subsystem's probe routine (before it calls > the driver's probe method) and in the subsystem's remove routine > (after it calls the driver's remove method). At such times, the > subsystem's runtime-PM handlers must be careful _not_ to call the > driver's runtime-PM routines. > >> > May be i am misinterpreting !! >> > If PHYs are runtime-PM enabled (PHY probe calls *runtime_enable*), >> > then the consumers >> > need to call pm_runtime_get_sync whever they want to access PHY. > > No, because in addition to being runtime-PM enabled, the PHY should > automatically be runtime resumed when the consumer registers itself as > a user of the PHY. Therefore the consumer doesn't need to do anything > at all -- which is good for consumers that aren't runtime-PM aware. > >> Alright, so here's my understanding: >> >> I suggested letting e.g. DWC3 enable the PHY's runtime_pm; Alan said >> that it could be done before that so that DWC3 sees an enabled PHY >> during probe. > > Basically right. Help me to understand the overall situation a little > better: > > What code registers the PHY initially? PHY is added to global list by PHY drivers (like phy-samsung-usb2.c/phy-omap-usb2.c) by usb_add_phy() API > > What routine does the DWC3 driver call to register itself > as a consumer of the PHY? I think DWC3 doesn't registers itself as consumer of PHY, rather it gets that PHY from the list using devm_usb_get_phy()/devm_usb_get_phy_by_phandle() API. DWC3 can now call PHY's initialization sequence using usb_phy_init(). So, before DWC3 initializes the PHY, PHYs should be in active state. > > Likewise, what routine does it call to unregister itself? Once DWC3's remove() is called PHYs are put.
On Tue, 23 Apr 2013, Vivek Gautam wrote: > >> Alright, so here's my understanding: > >> > >> I suggested letting e.g. DWC3 enable the PHY's runtime_pm; Alan said > >> that it could be done before that so that DWC3 sees an enabled PHY > >> during probe. > > > > Basically right. Help me to understand the overall situation a little > > better: > > > > What code registers the PHY initially? > PHY is added to global list by PHY drivers (like > phy-samsung-usb2.c/phy-omap-usb2.c) > by usb_add_phy() API Then this routine should initialize the PHY. The initialized state could be either active or suspended, your choice. Suspended would be best, in case the PHY never gets used. > > What routine does the DWC3 driver call to register itself > > as a consumer of the PHY? > I think DWC3 doesn't registers itself as consumer of PHY, > rather it gets that PHY from > the list using devm_usb_get_phy()/devm_usb_get_phy_by_phandle() API. > DWC3 can now call PHY's initialization sequence using usb_phy_init(). > So, before DWC3 initializes the PHY, PHYs should be in active state. Then usb_phy_init should make sure the PHY is in the active state. If usb_add_phy() left the PHY suspended, then this routine should call pm_runtime_get_sync(). After DWC3 (or any other driver) has acquired the PHY, it can call pm_runtime_put/get() however it likes, so long as the calls balance properly. If the driver isn't runtime-PM aware then it won't use any of these calls, and the PHY will remain active the entire time. > > Likewise, what routine does it call to unregister itself? > Once DWC3's remove() is called PHYs are put. Is there a routine analogous to usb_phy_init() that gets called when PHY is released? That routine would do the opposite of usb_phy_init(), putting the PHY back into its initialized state. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Tue, Apr 23, 2013 at 10:23 PM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Tue, 23 Apr 2013, Vivek Gautam wrote: > >> >> Alright, so here's my understanding: >> >> >> >> I suggested letting e.g. DWC3 enable the PHY's runtime_pm; Alan said >> >> that it could be done before that so that DWC3 sees an enabled PHY >> >> during probe. >> > >> > Basically right. Help me to understand the overall situation a little >> > better: >> > >> > What code registers the PHY initially? >> PHY is added to global list by PHY drivers (like >> phy-samsung-usb2.c/phy-omap-usb2.c) >> by usb_add_phy() API > > Then this routine should initialize the PHY. The initialized state > could be either active or suspended, your choice. Suspended would be > best, in case the PHY never gets used. Fair enough. > >> > What routine does the DWC3 driver call to register itself >> > as a consumer of the PHY? >> I think DWC3 doesn't registers itself as consumer of PHY, >> rather it gets that PHY from >> the list using devm_usb_get_phy()/devm_usb_get_phy_by_phandle() API. >> DWC3 can now call PHY's initialization sequence using usb_phy_init(). >> So, before DWC3 initializes the PHY, PHYs should be in active state. > > Then usb_phy_init should make sure the PHY is in the active state. If > usb_add_phy() left the PHY suspended, then this routine should call > pm_runtime_get_sync(). Right > > After DWC3 (or any other driver) has acquired the PHY, it can call > pm_runtime_put/get() however it likes, so long as the calls balance > properly. If the driver isn't runtime-PM aware then it won't use any > of these calls, and the PHY will remain active the entire time. Alright, so DWC3 (or any other consumer of PHY) should do minimal to handle runtime state of PHYs; get() when accessing PHY and put() when it's done with it. > >> > Likewise, what routine does it call to unregister itself? >> Once DWC3's remove() is called PHYs are put. > > Is there a routine analogous to usb_phy_init() that gets called when > PHY is released? That routine would do the opposite of usb_phy_init(), > putting the PHY back into its initialized state. Yes, ofcourse there's a routine usb_phy_shutdown(). So this will be calling put_sync() to put PHYs back to its initialized state. Right ?
On Tue, 23 Apr 2013, Vivek Gautam wrote: > Hi, > > > On Tue, Apr 23, 2013 at 10:23 PM, Alan Stern <stern@rowland.harvard.edu> wrote: > > On Tue, 23 Apr 2013, Vivek Gautam wrote: > > > >> >> Alright, so here's my understanding: > >> >> > >> >> I suggested letting e.g. DWC3 enable the PHY's runtime_pm; Alan said > >> >> that it could be done before that so that DWC3 sees an enabled PHY > >> >> during probe. > >> > > >> > Basically right. Help me to understand the overall situation a little > >> > better: > >> > > >> > What code registers the PHY initially? > >> PHY is added to global list by PHY drivers (like > >> phy-samsung-usb2.c/phy-omap-usb2.c) > >> by usb_add_phy() API > > > > Then this routine should initialize the PHY. The initialized state > > could be either active or suspended, your choice. Suspended would be > > best, in case the PHY never gets used. > > Fair enough. > > > > >> > What routine does the DWC3 driver call to register itself > >> > as a consumer of the PHY? > >> I think DWC3 doesn't registers itself as consumer of PHY, > >> rather it gets that PHY from > >> the list using devm_usb_get_phy()/devm_usb_get_phy_by_phandle() API. > >> DWC3 can now call PHY's initialization sequence using usb_phy_init(). > >> So, before DWC3 initializes the PHY, PHYs should be in active state. > > > > Then usb_phy_init should make sure the PHY is in the active state. If > > usb_add_phy() left the PHY suspended, then this routine should call > > pm_runtime_get_sync(). > > Right > > > > > After DWC3 (or any other driver) has acquired the PHY, it can call > > pm_runtime_put/get() however it likes, so long as the calls balance > > properly. If the driver isn't runtime-PM aware then it won't use any > > of these calls, and the PHY will remain active the entire time. > > Alright, so DWC3 (or any other consumer of PHY) should do minimal to > handle runtime state of PHYs; get() when accessing PHY and put() when it's done > with it. Yes. The first operation will generally be a put, because usb_phy_init() will leave the PHY in an active state. > >> > Likewise, what routine does it call to unregister itself? > >> Once DWC3's remove() is called PHYs are put. > > > > Is there a routine analogous to usb_phy_init() that gets called when > > PHY is released? That routine would do the opposite of usb_phy_init(), > > putting the PHY back into its initialized state. > > Yes, ofcourse there's a routine usb_phy_shutdown(). So this will be > calling put_sync() > to put PHYs back to its initialized state. Right ? Correct. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Apr 23, 2013 at 11:42 PM, Alan Stern <stern@rowland.harvard.edu> wrote: > On Tue, 23 Apr 2013, Vivek Gautam wrote: > >> Hi, >> >> >> On Tue, Apr 23, 2013 at 10:23 PM, Alan Stern <stern@rowland.harvard.edu> wrote: >> > On Tue, 23 Apr 2013, Vivek Gautam wrote: >> > >> >> >> Alright, so here's my understanding: >> >> >> >> >> >> I suggested letting e.g. DWC3 enable the PHY's runtime_pm; Alan said >> >> >> that it could be done before that so that DWC3 sees an enabled PHY >> >> >> during probe. >> >> > >> >> > Basically right. Help me to understand the overall situation a little >> >> > better: >> >> > >> >> > What code registers the PHY initially? >> >> PHY is added to global list by PHY drivers (like >> >> phy-samsung-usb2.c/phy-omap-usb2.c) >> >> by usb_add_phy() API >> > >> > Then this routine should initialize the PHY. The initialized state >> > could be either active or suspended, your choice. Suspended would be >> > best, in case the PHY never gets used. >> >> Fair enough. >> >> > >> >> > What routine does the DWC3 driver call to register itself >> >> > as a consumer of the PHY? >> >> I think DWC3 doesn't registers itself as consumer of PHY, >> >> rather it gets that PHY from >> >> the list using devm_usb_get_phy()/devm_usb_get_phy_by_phandle() API. >> >> DWC3 can now call PHY's initialization sequence using usb_phy_init(). >> >> So, before DWC3 initializes the PHY, PHYs should be in active state. >> > >> > Then usb_phy_init should make sure the PHY is in the active state. If >> > usb_add_phy() left the PHY suspended, then this routine should call >> > pm_runtime_get_sync(). >> >> Right >> >> > >> > After DWC3 (or any other driver) has acquired the PHY, it can call >> > pm_runtime_put/get() however it likes, so long as the calls balance >> > properly. If the driver isn't runtime-PM aware then it won't use any >> > of these calls, and the PHY will remain active the entire time. >> >> Alright, so DWC3 (or any other consumer of PHY) should do minimal to >> handle runtime state of PHYs; get() when accessing PHY and put() when it's done >> with it. > > Yes. The first operation will generally be a put, because > usb_phy_init() will leave the PHY in an active state. Alright. > >> >> > Likewise, what routine does it call to unregister itself? >> >> Once DWC3's remove() is called PHYs are put. >> > >> > Is there a routine analogous to usb_phy_init() that gets called when >> > PHY is released? That routine would do the opposite of usb_phy_init(), >> > putting the PHY back into its initialized state. >> >> Yes, ofcourse there's a routine usb_phy_shutdown(). So this will be >> calling put_sync() >> to put PHYs back to its initialized state. Right ? > > Correct. Hmm. Thanks for explaining things here and keeping patience to my queries :-) Felipe, thanks to you too for the discussion :-) I shall update the patchset asap.
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 available\n"); + return; + } + + pm_runtime_enable(x->dev); +} + +static inline void usb_phy_autopm_disable(struct usb_phy *x) +{ + if (!x || !x->dev) { + dev_err(x->dev, "no PHY or attached device available\n"); + return; + } + + pm_runtime_disable(x->dev); +} + +static inline int usb_phy_autopm_get(struct usb_phy *x) +{ + if (!x || !x->dev) { + dev_err(x->dev, "no PHY or attached device available\n"); + return -ENODEV; + } + + return pm_runtime_get(x->dev); +} + +static inline int usb_phy_autopm_get_sync(struct usb_phy *x) +{ + if (!x || !x->dev) { + dev_err(x->dev, "no PHY or attached device available\n"); + return -ENODEV; + } + + return pm_runtime_get_sync(x->dev); +} + +static inline int usb_phy_autopm_put(struct usb_phy *x) +{ + if (!x || !x->dev) { + dev_err(x->dev, "no PHY or attached device available\n"); + return -ENODEV; + } + + return pm_runtime_put(x->dev); +} + +static inline int usb_phy_autopm_put_sync(struct usb_phy *x) +{ + if (!x || !x->dev) { + dev_err(x->dev, "no PHY or attached device available\n"); + return -ENODEV; + } + + return pm_runtime_put_sync(x->dev); +} + +static inline void usb_phy_autopm_allow(struct usb_phy *x) +{ + if (!x || !x->dev) { + dev_err(x->dev, "no PHY or attached device available\n"); + return; + } + + pm_runtime_allow(x->dev); +} + +static inline void usb_phy_autopm_forbid(struct usb_phy *x) +{ + if (!x || !x->dev) { + dev_err(x->dev, "no PHY or attached device available\n"); + return; + } + + pm_runtime_forbid(x->dev); +} + +static inline int usb_phy_autopm_set_active(struct usb_phy *x) +{ + if (!x || !x->dev) { + dev_err(x->dev, "no PHY or attached device available\n"); + return -ENODEV; + } + + return pm_runtime_set_active(x->dev); +} + +static inline void usb_phy_autopm_set_suspended(struct usb_phy *x) +{ + if (!x || !x->dev) { + dev_err(x->dev, "no PHY or attached device available\n"); + return; + } + + pm_runtime_set_suspended(x->dev); +} + +static inline bool usb_phy_autopm_suspended(struct usb_phy *x) +{ + if (!x || !x->dev) { + dev_err(x->dev, "no PHY or attached device available\n"); + return 0; + } + + return pm_runtime_suspended(x->dev); +} + +static inline bool usb_phy_autopm_active(struct usb_phy *x) +{ + if (!x || !x->dev) { + dev_err(x->dev, "no PHY or attached device available\n"); + return 0; + } + + return pm_runtime_active(x->dev); +} + +static inline int usb_phy_autopm_suspend(struct usb_phy *x) +{ + if (!x || !x->dev) { + dev_err(x->dev, "no PHY or attached device available\n"); + return -ENODEV; + } + + return pm_runtime_suspend(x->dev); +} + +static inline int usb_phy_autopm_resume(struct usb_phy *x) +{ + if (!x || !x->dev) { + dev_err(x->dev, "no PHY or attached device available\n"); + return -ENODEV; + } + + return pm_runtime_resume(x->dev); +} + #endif /* __LINUX_USB_PHY_H */
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 <gautam.vivek@samsung.com> --- include/linux/usb/phy.h | 141 +++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 141 insertions(+), 0 deletions(-)