[PATCH 0/2] musb runtime_pm fixes

[PATCH 0/2] musb runtime_pm fixes

[Grazvydas Ignotas]

These patches try address isp1704_charger crash in a different way
than 772aed45b604, which broke runtime_pm for me.

Felipe Contreras, oculd you test isp1704_charger with these?

Grazvydas Ignotas (2):
  usb: musb: fix some runtime_pm issues
  usb: musb: wake the device before ulpi transfers

 drivers/usb/musb/musb_core.c |   40 +++++++++++++++++++++++++++++++++-------
 drivers/usb/musb/omap2430.c  |    2 +-
 include/linux/usb/otg.h      |    1 +
 3 files changed, 35 insertions(+), 8 deletions(-)

[PATCH 1/2] usb: musb: fix some runtime_pm issues

[Grazvydas Ignotas]

When runtime_pm was originally added, it was done in rather confusing
way: omap2430_musb_init() (called from musb_init_controller) would do
runtime_pm_get_sync() and musb_init_controller() itself would do
runtime_pm_put to balance it out. This is not only confusing but also
wrong if non-omap2430 glue layer is used.

This confusion resulted in commit 772aed45b604 "usb: musb: fix
pm_runtime mismatch", that removed runtime_pm_put() from
musb_init_controller as that looked unbalanced, and also happened to
fix unrelated isp1704_charger crash. However this broke runtime PM
functionality (musb is now always powered, even without gadget active).

Avoid these confusing runtime pm dependences by making
musb_init_controller() and omap2430_musb_init() do their own runtime
get/put pairs; also cover error paths. Remove unneeded runtime_pm_put
in omap2430_remove too. isp1704_charger crash that motivated
772aed45b604 will be fixed by following patch.

Cc: Felipe Contreras <felipe.contreras@gmail.com>
Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
---
 drivers/usb/musb/musb_core.c |    9 ++++++++-
 drivers/usb/musb/omap2430.c  |    2 +-
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 3d11cf6..19543c9 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1904,7 +1904,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 
 	if (!musb->isr) {
 		status = -ENODEV;
-		goto fail3;
+		goto fail2;
 	}
 
 	if (!musb->xceiv->io_ops) {
@@ -1912,6 +1912,8 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 		musb->xceiv->io_ops = &musb_ulpi_access;
 	}
 
+	pm_runtime_get_sync(musb->controller);
+
 #ifndef CONFIG_MUSB_PIO_ONLY
 	if (use_dma && dev->dma_mask) {
 		struct dma_controller	*c;
@@ -2023,6 +2025,8 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 		goto fail5;
 #endif
 
+	pm_runtime_put(musb->controller);
+
 	dev_info(dev, "USB %s mode controller at %p using %s, IRQ %d\n",
 			({char *s;
 			 switch (musb->board_mode) {
@@ -2047,6 +2051,9 @@ fail4:
 		musb_gadget_cleanup(musb);
 
 fail3:
+	pm_runtime_put_sync(musb->controller);
+
+fail2:
 	if (musb->irq_wake)
 		device_init_wakeup(dev, 0);
 	musb_platform_exit(musb);
diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
index df719ea..b105d46 100644
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -332,6 +332,7 @@ static int omap2430_musb_init(struct musb *musb)
 
 	setup_timer(&musb_idle_timer, musb_do_idle, (unsigned long) musb);
 
+	pm_runtime_put_noidle(musb->controller);
 	return 0;
 
 err1:
@@ -477,7 +478,6 @@ static int __exit omap2430_remove(struct platform_device *pdev)
 
 	platform_device_del(glue->musb);
 	platform_device_put(glue->musb);
-	pm_runtime_put(&pdev->dev);
 	kfree(glue);
 
 	return 0;
-- 
1.7.0.4

Re: [PATCH 1/2] usb: musb: fix some runtime_pm issues

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1214 bytes --]

On Sat, Feb 04, 2012 at 07:43:51PM +0200, Grazvydas Ignotas wrote:
> When runtime_pm was originally added, it was done in rather confusing
> way: omap2430_musb_init() (called from musb_init_controller) would do
> runtime_pm_get_sync() and musb_init_controller() itself would do
> runtime_pm_put to balance it out. This is not only confusing but also
> wrong if non-omap2430 glue layer is used.
> 
> This confusion resulted in commit 772aed45b604 "usb: musb: fix
> pm_runtime mismatch", that removed runtime_pm_put() from
> musb_init_controller as that looked unbalanced, and also happened to
> fix unrelated isp1704_charger crash. However this broke runtime PM
> functionality (musb is now always powered, even without gadget active).
> 
> Avoid these confusing runtime pm dependences by making
> musb_init_controller() and omap2430_musb_init() do their own runtime
> get/put pairs; also cover error paths. Remove unneeded runtime_pm_put
> in omap2430_remove too. isp1704_charger crash that motivated
> 772aed45b604 will be fixed by following patch.
> 
> Cc: Felipe Contreras <felipe.contreras@gmail.com>
> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>

applied, thanks

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 2/2] usb: musb: wake the device before ulpi transfers

[Grazvydas Ignotas]

musb can be suspended at the time some other driver wants to do ulpi
transfers using otg_io_* functions, and that can cause data abort,
as it happened with isp1704_charger:
http://article.gmane.org/gmane.linux.kernel/1226122

Add pm_runtime to ulpi functions to rectify this. This also adds io_dev
to otg_transceiver so that pm_runtime can be used.

Cc: Felipe Contreras <felipe.contreras-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Grazvydas Ignotas <notasas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/usb/musb/musb_core.c |   31 +++++++++++++++++++++++++------
 include/linux/usb/otg.h      |    1 +
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 19543c9..ca9d74a 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -137,6 +137,9 @@ static int musb_ulpi_read(struct otg_transceiver *otg, u32 offset)
 	int	i = 0;
 	u8	r;
 	u8	power;
+	int	ret;
+
+	pm_runtime_get_sync(otg->io_dev);
 
 	/* Make sure the transceiver is not in low power mode */
 	power = musb_readb(addr, MUSB_POWER);
@@ -154,15 +157,22 @@ static int musb_ulpi_read(struct otg_transceiver *otg, u32 offset)
 	while (!(musb_readb(addr, MUSB_ULPI_REG_CONTROL)
 				& MUSB_ULPI_REG_CMPLT)) {
 		i++;
-		if (i == 10000)
-			return -ETIMEDOUT;
+		if (i == 10000) {
+			ret = -ETIMEDOUT;
+			goto out;
+		}
 
 	}
 	r = musb_readb(addr, MUSB_ULPI_REG_CONTROL);
 	r &= ~MUSB_ULPI_REG_CMPLT;
 	musb_writeb(addr, MUSB_ULPI_REG_CONTROL, r);
 
-	return musb_readb(addr, MUSB_ULPI_REG_DATA);
+	ret = musb_readb(addr, MUSB_ULPI_REG_DATA);
+
+out:
+	pm_runtime_put(otg->io_dev);
+
+	return ret;
 }
 
 static int musb_ulpi_write(struct otg_transceiver *otg,
@@ -172,6 +182,9 @@ static int musb_ulpi_write(struct otg_transceiver *otg,
 	int	i = 0;
 	u8	r = 0;
 	u8	power;
+	int	ret = 0;
+
+	pm_runtime_get_sync(otg->io_dev);
 
 	/* Make sure the transceiver is not in low power mode */
 	power = musb_readb(addr, MUSB_POWER);
@@ -185,15 +198,20 @@ static int musb_ulpi_write(struct otg_transceiver *otg,
 	while (!(musb_readb(addr, MUSB_ULPI_REG_CONTROL)
 				& MUSB_ULPI_REG_CMPLT)) {
 		i++;
-		if (i == 10000)
-			return -ETIMEDOUT;
+		if (i == 10000) {
+			ret = -ETIMEDOUT;
+			goto out;
+		}
 	}
 
 	r = musb_readb(addr, MUSB_ULPI_REG_CONTROL);
 	r &= ~MUSB_ULPI_REG_CMPLT;
 	musb_writeb(addr, MUSB_ULPI_REG_CONTROL, r);
 
-	return 0;
+out:
+	pm_runtime_put(otg->io_dev);
+
+	return ret;
 }
 #else
 #define musb_ulpi_read		NULL
@@ -1908,6 +1926,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 	}
 
 	if (!musb->xceiv->io_ops) {
+		musb->xceiv->io_dev = musb->controller;
 		musb->xceiv->io_priv = musb->mregs;
 		musb->xceiv->io_ops = &musb_ulpi_access;
 	}
diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
index d87f44f..308b699 100644
--- a/include/linux/usb/otg.h
+++ b/include/linux/usb/otg.h
@@ -71,6 +71,7 @@ struct otg_transceiver {
 	struct usb_bus		*host;
 	struct usb_gadget	*gadget;
 
+	struct device			*io_dev;
 	struct otg_io_access_ops	*io_ops;
 	void __iomem			*io_priv;
 
-- 
1.7.0.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] usb: musb: wake the device before ulpi transfers

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]

On Sat, Feb 04, 2012 at 07:43:52PM +0200, Grazvydas Ignotas wrote:
> musb can be suspended at the time some other driver wants to do ulpi
> transfers using otg_io_* functions, and that can cause data abort,
> as it happened with isp1704_charger:
> http://article.gmane.org/gmane.linux.kernel/1226122
> 
> Add pm_runtime to ulpi functions to rectify this. This also adds io_dev
> to otg_transceiver so that pm_runtime can be used.
> 
> Cc: Felipe Contreras <felipe.contreras-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Grazvydas Ignotas <notasas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

doesn't apply, please refresh.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 2/2] usb: musb: wake the device before ulpi transfers

[Grazvydas Ignotas]

On Wed, Mar 21, 2012 at 12:10 PM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote:
> On Sat, Feb 04, 2012 at 07:43:52PM +0200, Grazvydas Ignotas wrote:
>> musb can be suspended at the time some other driver wants to do ulpi
>> transfers using otg_io_* functions, and that can cause data abort,
>> as it happened with isp1704_charger:
>> http://article.gmane.org/gmane.linux.kernel/1226122
>>
>> Add pm_runtime to ulpi functions to rectify this. This also adds io_dev
>> to otg_transceiver so that pm_runtime can be used.
>>
>> Cc: Felipe Contreras <felipe.contreras-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Signed-off-by: Grazvydas Ignotas <notasas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> doesn't apply, please refresh.

done.


-- 
Gražvydas
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] musb runtime_pm fixes

[Grazvydas Ignotas]

On Sat, Feb 4, 2012 at 7:43 PM, Grazvydas Ignotas <notasas@gmail.com> wrote:
> These patches try address isp1704_charger crash in a different way
> than 772aed45b604, which broke runtime_pm for me.
>
> Felipe Contreras, oculd you test isp1704_charger with these?

Ping. These still apply cleanly and are needed to solve musb runtime
PM issues on OMAP3.
I've also done a random ULPI transfer while musb was suspended and it worked.

>
> Grazvydas Ignotas (2):
>  usb: musb: fix some runtime_pm issues
>  usb: musb: wake the device before ulpi transfers
>
>  drivers/usb/musb/musb_core.c |   40 +++++++++++++++++++++++++++++++++-------
>  drivers/usb/musb/omap2430.c  |    2 +-
>  include/linux/usb/otg.h      |    1 +
>  3 files changed, 35 insertions(+), 8 deletions(-)
>

-- 
Gražvydas
--
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