[PATCH V2] usb: musb: Fix unstable init of OTG_INTERFSEL.

[PATCH V2] usb: musb: Fix unstable init of OTG_INTERFSEL.

[anaumann]

From: Andreas Naumann <anaumann@ultratronik.de>

This is a hard to reproduce problem which leads to non-functional
USB-OTG port in 0.1%-1% of all boots. Tracked it down to commit
e25bec160158abe86c276d7d206264afc3646281, which introduces save/restore
of OTG_INTERFSEL over suspend.
Since the resume function is also called early in driver init, it uses a
non-initialized value (which is 0 and a non-supported setting in DM37xx
for INTERFSEL). Shortly after the correct value is set. Apparently this
works most time, but not always.

The fix is to initialize the value, BEFORE being written in the resume
function.


Signed-off-by: Andreas Naumann <anaumann@ultratronik.de>
---
 drivers/usb/musb/omap2430.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
index 4315d35..783547c 100644
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -337,7 +337,6 @@ static irqreturn_t omap2430_musb_interrupt(int irq, void *__hci)
 
 static int omap2430_musb_init(struct musb *musb)
 {
-	u32 l;
 	int status = 0;
 	struct device *dev = musb->controller;
 	struct omap2430_glue *glue = dev_get_drvdata(dev->parent);
@@ -366,23 +365,24 @@ static int omap2430_musb_init(struct musb *musb)
 
 	musb->isr = omap2430_musb_interrupt;
 
+	musb->context.otg_interfsel = musb_readl(musb->mregs, OTG_INTERFSEL);
+
+	if (data->interface_type == MUSB_INTERFACE_UTMI) {
+		/* OMAP4 uses Internal PHY GS70 which uses UTMI interface */
+		musb->context.otg_interfsel &= ~ULPI_12PIN;  /* Disable ULPI */
+		musb->context.otg_interfsel |= UTMI_8BIT;    /* Enable UTMI  */
+	} else {
+		musb->context.otg_interfsel |= ULPI_12PIN;
+	}
+
 	status = pm_runtime_get_sync(dev);
 	if (status < 0) {
 		dev_err(dev, "pm_runtime_get_sync FAILED %d\n", status);
 		goto err1;
 	}
 
-	l = musb_readl(musb->mregs, OTG_INTERFSEL);
-
-	if (data->interface_type == MUSB_INTERFACE_UTMI) {
-		/* OMAP4 uses Internal PHY GS70 which uses UTMI interface */
-		l &= ~ULPI_12PIN;       /* Disable ULPI */
-		l |= UTMI_8BIT;         /* Enable UTMI  */
-	} else {
-		l |= ULPI_12PIN;
-	}
 
-	musb_writel(musb->mregs, OTG_INTERFSEL, l);
+	musb_writel(musb->mregs, OTG_INTERFSEL, musb->context.otg_interfsel);
 
 	pr_debug("HS USB OTG: revision 0x%x, sysconfig 0x%02x, "
 			"sysstatus 0x%x, intrfsel 0x%x, simenable  0x%x\n",
-- 
1.8.4.1

Re: [PATCH V2] usb: musb: Fix unstable init of OTG_INTERFSEL.

[Grazvydas Ignotas]

On Thu, Nov 28, 2013 at 9:51 AM,  <anaumann-ZKHRqZ6+gQUX0D0ZMPkEVw@public.gmane.org> wrote:
> From: Andreas Naumann <anaumann-ZKHRqZ6+gQUX0D0ZMPkEVw@public.gmane.org>
>
> This is a hard to reproduce problem which leads to non-functional
> USB-OTG port in 0.1%-1% of all boots. Tracked it down to commit
> e25bec160158abe86c276d7d206264afc3646281, which introduces save/restore
> of OTG_INTERFSEL over suspend.
> Since the resume function is also called early in driver init, it uses a
> non-initialized value (which is 0 and a non-supported setting in DM37xx
> for INTERFSEL). Shortly after the correct value is set. Apparently this
> works most time, but not always.
>
> The fix is to initialize the value, BEFORE being written in the resume
> function.

Nice find, I think I'm also affected by this. However this crashes on
OMAP3530 pandora because you now access INTERFSEL before interface
clock is enabled.. This is not a problem on DM37xx because it uses
different interconnects. You should probably use "context_valid"
variable in omap2430_glue or similar to decide to write to register or
not.

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 V2] usb: musb: Fix unstable init of OTG_INTERFSEL.

[Andreas Naumann]

> Nice find, I think I'm also affected by this. However this crashes on
> OMAP3530 pandora because you now access INTERFSEL before interface
> clock is enabled.. This is not a problem on DM37xx because it uses
> different interconnects. You should probably use "context_valid"
> variable in omap2430_glue or similar to decide to write to register or
> not.
>
> Grazvydas

You mean introducing a "context_value"? I'm not sure if i want to add 
more code.
Do you think simple removing the first read is an option? So far we read 
back 0 anyway because the first write to INTERFSEL in resume() set it to 
uninitialized / 0 anyway.



> --
> 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
>

Re: [PATCH V2] usb: musb: Fix unstable init of OTG_INTERFSEL.

[Grazvydas Ignotas]

On Fri, Nov 29, 2013 at 10:34 AM, Andreas Naumann <dev@andin.de> wrote:
>> Nice find, I think I'm also affected by this. However this crashes on
>> OMAP3530 pandora because you now access INTERFSEL before interface
>> clock is enabled.. This is not a problem on DM37xx because it uses
>> different interconnects. You should probably use "context_valid"
>> variable in omap2430_glue or similar to decide to write to register or
>> not.
>>
>> Grazvydas
>
>
> You mean introducing a "context_value"? I'm not sure if i want to add more
> code.

What I meant is to have a flag that would be set after
omap2430_runtime_suspend() is called, and only write to INTERFSEL in
omap2430_runtime_resume() if that flag is set.

Drivers like drivers/tty/serial/omap-serial.c or
drivers/gpio/gpio-omap.c use get_context_loss_count() callbacks for
this, but it looks unnecessarily complicated.. Maybe Felipe has some
ideas on this?

> Do you think simple removing the first read is an option? So far we read
> back 0 anyway because the first write to INTERFSEL in resume() set it to
> uninitialized / 0 anyway.

It should be fine but I don't know if all OMAPs only have PHY
interface bits, there could be some that have some more bits there..
Yes current code is broken and writes 0 anyway, so any solution is
better, but ideally it should read-modify-write the register and only
change PHYSEL bits at probe.