Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
On Tue, Apr 23, 2013 at 11:42 PM, Alan Stern st...@rowland.harvard.edu wrote: On Tue, 23 Apr 2013, Vivek Gautam wrote: Hi, On Tue, Apr 23, 2013 at 10:23 PM, Alan Stern st...@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. -- Best Regards Vivek -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
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.vi...@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.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
Hi, On Thu, Apr 4, 2013 at 8:16 PM, Alan Stern st...@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. -- Best Regards Vivek -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
Hi, On Tue, Apr 23, 2013 at 10:23 PM, Alan Stern st...@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 ? -- Best Regards Vivek -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
On Tue, 23 Apr 2013, Vivek Gautam wrote: Hi, On Tue, Apr 23, 2013 at 10:23 PM, Alan Stern st...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
On Tuesday 02 April 2013 06:10 PM, Vivek Gautam wrote:
Hi,
On Tue, Apr 2, 2013 at 5:40 PM, Felipe Balbi ba...@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.vi...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
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 ? -- balbi signature.asc Description: Digital signature
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
Hi, On Thu, Apr 4, 2013 at 12:48 PM, Felipe Balbi ba...@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. -- Thanks Regards Vivek -- To unsubscribe from this list: send the line unsubscribe linux-omap in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
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. -- balbi signature.asc Description: Digital signature
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
Hi Kishon,
On Wed, Apr 3, 2013 at 10:38 AM, Kishon Vijay Abraham I kis...@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.vi...@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 ?
--
Thanks Regards
Vivek
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
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.vi...@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
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
Hi Felipe,
On Wed, Apr 3, 2013 at 1:45 PM, Felipe Balbi ba...@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.vi...@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
--
Thanks Regards
Vivek
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
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.vi...@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);
}
???
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
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.
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
Hi,
On Wed, Apr 3, 2013 at 7:26 PM, Felipe Balbi ba...@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 ?
--
Thanks Regards
Vivek
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
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 :-)
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
On Wed, Apr 3, 2013 at 7:48 PM, Felipe Balbi ba...@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
--
Thanks Regards
Vivek
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
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.vi...@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 ?
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
Hi,
On Tue, Apr 2, 2013 at 1:53 PM, Felipe Balbi ba...@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.vi...@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 ;
--
Thanks Regards
Vivek
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
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.vi...@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 ??
--
balbi
signature.asc
Description: Digital signature
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
Hi,
On Tue, Apr 2, 2013 at 5:40 PM, Felipe Balbi ba...@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.vi...@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.
--
Thanks Regards
Vivek
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 01/11] usb: phy: Add APIs for runtime power management
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.vi...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v3 01/11] usb: phy: Add APIs for runtime power management
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.vi...@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);
+}
+
+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 */
--
1.7.6.5
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
