Re: [PATCH 5/5 v4] mfd: omap: usb: Runtime PM support
On Tue, Aug 30, 2011 at 2:17 AM, Kevin Hilman khil...@ti.com wrote: Kevin Hilman khil...@ti.com writes: Keshava Munegowda keshava_mgo...@ti.com writes: From: Keshava Munegowda keshava_mgo...@ti.com The usbhs core driver does not enable/disable the intefrace and fucntional clocks; These clocks are handled by hwmod and runtime pm, hence insted of the clock enable/disable, the runtime pm APIS are used. however,the port clocks are handled by the usbhs core. Signed-off-by: Keshava Munegowda keshava_mgo...@ti.com General comment: all usage of pm_runtime_put_sync() can likely be replaced by the asynchronous versions. I don't currently see why the synchronous verions are needed here. Other than that, the runtime PM parts of this look OK to me. After changing to asynchonous puts, feel free to add: Reviewed-by: Kevin Hilman khil...@ti.com oops, this should've been in response to your v8 version. Kevin Also, after a quick glance, it looks like this version of the series addresses the problems seen by Jassi Brar with the TLL reset[1]. Please confirm. Speaking of which, it's helpful to Cc folks who have had comments on previous versions of your series so they are sure they're previous issues are addressed. I've Cc'd Jassi Brar. Thanks, Kevin [1] http://marc.info/?l=linux-omapm=130921260703865w=2 Thanks for review; yes, its reworks of complete runtime pm of usbhs as suggested by Jassi Brar. balbi, sameo this patch series available at the branch kmg-usbhs-pm of code repository : git://gitorious.org/~kmg/mirrors/kmg-usbhs-pm.git Regards Keshava -- 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 5/5 v4] mfd: omap: usb: Runtime PM support
Todd Poynor toddpoy...@google.com writes:
On Fri, Aug 12, 2011 at 12:20:21PM +0530, Munegowda, Keshava wrote:
On Wed, Aug 10, 2011 at 10:01 PM, Todd Poynor toddpoy...@google.com wrote:
@@ -913,12 +598,15 @@ static int usbhs_enable(struct device *dev)
(pdata-ehci_data-reset_gpio_port[1], 1);
}
-end_count:
- omap-count++;
+ pm_runtime_put_sync(dev);
spin_unlock_irqrestore(omap-lock, flags);
Is pm_runtime_irq_safe() needed (else I think runtime PM callbacks may
re-enable IRQs... or there's the new *_suspend runtime PM calls that
may avoid this)?
pm_runtime_irq_safe() is not required; usbhs does not have a parent
and it is the parent driver of
ehci and ohci drivers.
But the above expects IRQs to be disabled during the
pm_runtime_put_sync, and synchronous calls can turn IRQs back on in
rpm_idle:
if (callback) {
spin_unlock_irq(dev-power.lock);
callback(dev);
I see other folks who know this better than me are discussing USB run
time PM and might_sleep contexts, so I'll note this concern and let
others chime in if they think there's a real problem here.
FYI... The commit below fixes this mainline (merged as of v3.1-rc4).
Kevin
commit 02b26774afebb2d62695ba3230319d70d8c6cc2d
Author: Kevin Hilman khil...@ti.com
Date: Fri Aug 5 21:45:20 2011 +0200
PM / Runtime: Allow _put_sync() from interrupts-disabled context
Currently the use of pm_runtime_put_sync() is not safe from
interrupts-disabled context because rpm_idle() will release the
spinlock and enable interrupts for the idle callbacks. This enables
interrupts during a time where interrupts were expected to be
disabled, and can have strange side effects on drivers that expected
interrupts to be disabled.
This is not a bug since the documentation clearly states that only
_put_sync_suspend() is safe in IRQ-safe mode.
However, pm_runtime_put_sync() could be made safe when in IRQ-safe
mode by releasing the spinlock but not re-enabling interrupts, which
is what this patch aims to do.
Problem was found when using some buggy drivers that set
pm_runtime_irq_safe() and used _put_sync() in interrupts-disabled
context.
Reported-by: Colin Cross ccr...@google.com
Tested-by: Nishanth Menon n...@ti.com
Signed-off-by: Kevin Hilman khil...@ti.com
Signed-off-by: Rafael J. Wysocki r...@sisk.pl
--
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 5/5 v4] mfd: omap: usb: Runtime PM support
Keshava Munegowda keshava_mgo...@ti.com writes: From: Keshava Munegowda keshava_mgo...@ti.com The usbhs core driver does not enable/disable the intefrace and fucntional clocks; These clocks are handled by hwmod and runtime pm, hence insted of the clock enable/disable, the runtime pm APIS are used. however,the port clocks are handled by the usbhs core. Signed-off-by: Keshava Munegowda keshava_mgo...@ti.com General comment: all usage of pm_runtime_put_sync() can likely be replaced by the asynchronous versions. I don't currently see why the synchronous verions are needed here. Other than that, the runtime PM parts of this look OK to me. After changing to asynchonous puts, feel free to add: Reviewed-by: Kevin Hilman khil...@ti.com Also, after a quick glance, it looks like this version of the series addresses the problems seen by Jassi Brar with the TLL reset[1]. Please confirm. Speaking of which, it's helpful to Cc folks who have had comments on previous versions of your series so they are sure they're previous issues are addressed. I've Cc'd Jassi Brar. Thanks, Kevin [1] http://marc.info/?l=linux-omapm=130921260703865w=2 -- 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 5/5 v4] mfd: omap: usb: Runtime PM support
Kevin Hilman khil...@ti.com writes: Keshava Munegowda keshava_mgo...@ti.com writes: From: Keshava Munegowda keshava_mgo...@ti.com The usbhs core driver does not enable/disable the intefrace and fucntional clocks; These clocks are handled by hwmod and runtime pm, hence insted of the clock enable/disable, the runtime pm APIS are used. however,the port clocks are handled by the usbhs core. Signed-off-by: Keshava Munegowda keshava_mgo...@ti.com General comment: all usage of pm_runtime_put_sync() can likely be replaced by the asynchronous versions. I don't currently see why the synchronous verions are needed here. Other than that, the runtime PM parts of this look OK to me. After changing to asynchonous puts, feel free to add: Reviewed-by: Kevin Hilman khil...@ti.com oops, this should've been in response to your v8 version. Kevin Also, after a quick glance, it looks like this version of the series addresses the problems seen by Jassi Brar with the TLL reset[1]. Please confirm. Speaking of which, it's helpful to Cc folks who have had comments on previous versions of your series so they are sure they're previous issues are addressed. I've Cc'd Jassi Brar. Thanks, Kevin [1] http://marc.info/?l=linux-omapm=130921260703865w=2 -- 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 5/5 v4] mfd: omap: usb: Runtime PM support
On Sat, Aug 13, 2011 at 3:00 AM, Todd Poynor toddpoy...@google.com wrote:
On Fri, Aug 12, 2011 at 12:20:21PM +0530, Munegowda, Keshava wrote:
On Wed, Aug 10, 2011 at 10:01 PM, Todd Poynor toddpoy...@google.com wrote:
@@ -913,12 +598,15 @@ static int usbhs_enable(struct device *dev)
(pdata-ehci_data-reset_gpio_port[1], 1);
}
-end_count:
- omap-count++;
+ pm_runtime_put_sync(dev);
spin_unlock_irqrestore(omap-lock, flags);
Is pm_runtime_irq_safe() needed (else I think runtime PM callbacks may
re-enable IRQs... or there's the new *_suspend runtime PM calls that
may avoid this)?
pm_runtime_irq_safe() is not required; usbhs does not have a parent
and it is the parent driver of
ehci and ohci drivers.
But the above expects IRQs to be disabled during the
pm_runtime_put_sync, and synchronous calls can turn IRQs back on in
rpm_idle:
if (callback) {
spin_unlock_irq(dev-power.lock);
callback(dev);
I see other folks who know this better than me are discussing USB run
time PM and might_sleep contexts, so I'll note this concern and let
others chime in if they think there's a real problem here.
Thanks, I think I should protect the critical section of call backs here.
Todd
--
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 5/5 v4] mfd: omap: usb: Runtime PM support
On Wed, Aug 10, 2011 at 10:01 PM, Todd Poynor toddpoy...@google.com wrote: On Tue, Aug 09, 2011 at 07:45:09PM +0530, Keshava Munegowda wrote: From: Keshava Munegowda keshava_mgo...@ti.com The usbhs core driver does not enable/disable the intefrace and typo: interface fucntional clocks; These clocks are handled by hwmod and runtime pm, typo: functional hence insted of the clock enable/disable, the runtime pm APIS are used. however,the port clocks are handled by the usbhs core. Signed-off-by: Keshava Munegowda keshava_mgo...@ti.com ... @@ -913,12 +598,15 @@ static int usbhs_enable(struct device *dev) (pdata-ehci_data-reset_gpio_port[1], 1); } -end_count: - omap-count++; + pm_runtime_put_sync(dev); spin_unlock_irqrestore(omap-lock, flags); Is pm_runtime_irq_safe() needed (else I think runtime PM callbacks may re-enable IRQs... or there's the new *_suspend runtime PM calls that may avoid this)? pm_runtime_irq_safe() is not required; usbhs does not have a parent and it is the parent driver of ehci and ohci drivers. ... @@ -266,10 +261,12 @@ static int ehci_hcd_omap_remove(struct platform_device *pdev) struct usb_hcd *hcd = dev_get_drvdata(dev); usb_remove_hcd(hcd); - omap_usbhs_disable(dev); disable_put_regulator(dev-platform_data); - iounmap(hcd-regs); usb_put_hcd(hcd); + iounmap(hcd-regs); yes , I will do this. usb_put_hcd may release the hcd, needs to be after the deref for iounmap. + pm_runtime_put_sync(dev); + pm_runtime_disable(dev); Todd -- 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 5/5 v4] mfd: omap: usb: Runtime PM support
On Fri, Aug 12, 2011 at 12:20:21PM +0530, Munegowda, Keshava wrote:
On Wed, Aug 10, 2011 at 10:01 PM, Todd Poynor toddpoy...@google.com wrote:
@@ -913,12 +598,15 @@ static int usbhs_enable(struct device *dev)
(pdata-ehci_data-reset_gpio_port[1], 1);
}
-end_count:
- omap-count++;
+ pm_runtime_put_sync(dev);
spin_unlock_irqrestore(omap-lock, flags);
Is pm_runtime_irq_safe() needed (else I think runtime PM callbacks may
re-enable IRQs... or there's the new *_suspend runtime PM calls that
may avoid this)?
pm_runtime_irq_safe() is not required; usbhs does not have a parent
and it is the parent driver of
ehci and ohci drivers.
But the above expects IRQs to be disabled during the
pm_runtime_put_sync, and synchronous calls can turn IRQs back on in
rpm_idle:
if (callback) {
spin_unlock_irq(dev-power.lock);
callback(dev);
I see other folks who know this better than me are discussing USB run
time PM and might_sleep contexts, so I'll note this concern and let
others chime in if they think there's a real problem here.
Todd
--
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 5/5 v4] mfd: omap: usb: Runtime PM support
On Fri, Aug 12, 2011 at 16:30, Todd Poynor toddpoy...@google.com wrote:
On Fri, Aug 12, 2011 at 12:20:21PM +0530, Munegowda, Keshava wrote:
On Wed, Aug 10, 2011 at 10:01 PM, Todd Poynor toddpoy...@google.com wrote:
@@ -913,12 +598,15 @@ static int usbhs_enable(struct device *dev)
(pdata-ehci_data-reset_gpio_port[1], 1);
}
-end_count:
- omap-count++;
+ pm_runtime_put_sync(dev);
spin_unlock_irqrestore(omap-lock, flags);
Is pm_runtime_irq_safe() needed (else I think runtime PM callbacks may
re-enable IRQs... or there's the new *_suspend runtime PM calls that
may avoid this)?
pm_runtime_irq_safe() is not required; usbhs does not have a parent
and it is the parent driver of
ehci and ohci drivers.
But the above expects IRQs to be disabled during the
pm_runtime_put_sync, and synchronous calls can turn IRQs back on in
rpm_idle:
if (callback) {
spin_unlock_irq(dev-power.lock);
callback(dev);
I see other folks who know this better than me are discussing USB run
time PM and might_sleep contexts, so I'll note this concern and let
others chime in if they think there's a real problem here.
It is rather easy with the following patch applied to pull this bug out!
https://patchwork.kernel.org/patch/1001572/
Regards,
Nishanth Menon
--
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 5/5 v4] mfd: omap: usb: Runtime PM support
Hi,
On Tue, Aug 9, 2011 at 10:15 PM, Keshava Munegowda
keshava_mgo...@ti.com wrote:
From: Keshava Munegowda keshava_mgo...@ti.com
The usbhs core driver does not enable/disable the intefrace and
fucntional clocks; These clocks are handled by hwmod and runtime pm,
hence insted of the clock enable/disable, the runtime pm APIS are
used. however,the port clocks are handled by the usbhs core.
Signed-off-by: Keshava Munegowda keshava_mgo...@ti.com
---
arch/arm/plat-omap/include/plat/usb.h | 3 -
drivers/mfd/omap-usb-host.c | 722
+
drivers/usb/host/ehci-omap.c | 19 +-
drivers/usb/host/ohci-omap3.c | 14 +-
4 files changed, 286 insertions(+), 472 deletions(-)
diff --git a/arch/arm/plat-omap/include/plat/usb.h
b/arch/arm/plat-omap/include/plat/usb.h
index 17d3c93..2b66dc2 100644
--- a/arch/arm/plat-omap/include/plat/usb.h
+++ b/arch/arm/plat-omap/include/plat/usb.h
@@ -100,9 +100,6 @@ extern void usb_musb_init(struct omap_musb_board_data
*board_data);
extern void usbhs_init(const struct usbhs_omap_board_data *pdata);
-extern int omap_usbhs_enable(struct device *dev);
-extern void omap_usbhs_disable(struct device *dev);
-
extern int omap4430_phy_power(struct device *dev, int ID, int on);
extern int omap4430_phy_set_clk(struct device *dev, int on);
extern int omap4430_phy_init(struct device *dev);
diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
index 5def51c..39cfae6 100644
--- a/drivers/mfd/omap-usb-host.c
+++ b/drivers/mfd/omap-usb-host.c
@@ -26,6 +26,7 @@
#include linux/spinlock.h
#include linux/gpio.h
#include plat/usb.h
+#include linux/pm_runtime.h
#define USBHS_DRIVER_NAME usbhs_omap
#define OMAP_EHCI_DEVICE ehci-omap
@@ -146,9 +147,6 @@
struct usbhs_hcd_omap {
- struct clk *usbhost_ick;
- struct clk *usbhost_hs_fck;
- struct clk *usbhost_fs_fck;
struct clk *xclk60mhsp1_ck;
struct clk *xclk60mhsp2_ck;
struct clk *utmi_p1_fck;
@@ -158,8 +156,6 @@ struct usbhs_hcd_omap {
struct clk *usbhost_p2_fck;
struct clk *usbtll_p2_fck;
struct clk *init_60m_fclk;
- struct clk *usbtll_fck;
- struct clk *usbtll_ick;
void __iomem *uhh_base;
void __iomem *tll_base;
@@ -168,7 +164,6 @@ struct usbhs_hcd_omap {
u32 usbhs_rev;
spinlock_t lock;
- int count;
};
/*-*/
@@ -318,269 +313,6 @@ err_end:
return ret;
}
-/**
- * usbhs_omap_probe - initialize TI-based HCDs
- *
- * Allocates basic resources for this USB host controller.
- */
-static int __devinit usbhs_omap_probe(struct platform_device *pdev)
-{
- struct device *dev = pdev-dev;
- struct usbhs_omap_platform_data *pdata = dev-platform_data;
- struct usbhs_hcd_omap *omap;
- struct resource *res;
- int ret = 0;
- int i;
-
- if (!pdata) {
- dev_err(dev, Missing platform data\n);
- ret = -ENOMEM;
- goto end_probe;
- }
-
- omap = kzalloc(sizeof(*omap), GFP_KERNEL);
- if (!omap) {
- dev_err(dev, Memory allocation failed\n);
- ret = -ENOMEM;
- goto end_probe;
- }
-
- spin_lock_init(omap-lock);
-
- for (i = 0; i OMAP3_HS_USB_PORTS; i++)
- omap-platdata.port_mode[i] = pdata-port_mode[i];
-
- omap-platdata.ehci_data = pdata-ehci_data;
- omap-platdata.ohci_data = pdata-ohci_data;
-
- omap-usbhost_ick = clk_get(dev, usbhost_ick);
- if (IS_ERR(omap-usbhost_ick)) {
- ret = PTR_ERR(omap-usbhost_ick);
- dev_err(dev, usbhost_ick failed error:%d\n, ret);
- goto err_end;
- }
-
- omap-usbhost_hs_fck = clk_get(dev, hs_fck);
- if (IS_ERR(omap-usbhost_hs_fck)) {
- ret = PTR_ERR(omap-usbhost_hs_fck);
- dev_err(dev, usbhost_hs_fck failed error:%d\n, ret);
- goto err_usbhost_ick;
- }
-
- omap-usbhost_fs_fck = clk_get(dev, fs_fck);
- if (IS_ERR(omap-usbhost_fs_fck)) {
- ret = PTR_ERR(omap-usbhost_fs_fck);
- dev_err(dev, usbhost_fs_fck failed error:%d\n, ret);
- goto err_usbhost_hs_fck;
- }
-
- omap-usbtll_fck = clk_get(dev, usbtll_fck);
- if
Re: [PATCH 5/5 v4] mfd: omap: usb: Runtime PM support
On Tue, Aug 09, 2011 at 07:45:09PM +0530, Keshava Munegowda wrote: From: Keshava Munegowda keshava_mgo...@ti.com The usbhs core driver does not enable/disable the intefrace and typo: interface fucntional clocks; These clocks are handled by hwmod and runtime pm, typo: functional hence insted of the clock enable/disable, the runtime pm APIS are used. however,the port clocks are handled by the usbhs core. Signed-off-by: Keshava Munegowda keshava_mgo...@ti.com ... @@ -913,12 +598,15 @@ static int usbhs_enable(struct device *dev) (pdata-ehci_data-reset_gpio_port[1], 1); } -end_count: - omap-count++; + pm_runtime_put_sync(dev); spin_unlock_irqrestore(omap-lock, flags); Is pm_runtime_irq_safe() needed (else I think runtime PM callbacks may re-enable IRQs... or there's the new *_suspend runtime PM calls that may avoid this)? ... @@ -266,10 +261,12 @@ static int ehci_hcd_omap_remove(struct platform_device *pdev) struct usb_hcd *hcd = dev_get_drvdata(dev); usb_remove_hcd(hcd); - omap_usbhs_disable(dev); disable_put_regulator(dev-platform_data); - iounmap(hcd-regs); usb_put_hcd(hcd); + iounmap(hcd-regs); usb_put_hcd may release the hcd, needs to be after the deref for iounmap. + pm_runtime_put_sync(dev); + pm_runtime_disable(dev); Todd -- 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
