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

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

[anaumann-ZKHRqZ6+gQUX0D0ZMPkEVw]

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.

Fix it by not writing the value on runtime resume if it has not been
initialized yet.

Signed-off-by: Andreas Naumann <anaumann-ZKHRqZ6+gQUX0D0ZMPkEVw@public.gmane.org>
---
Even though I find the implementation a bit awkward this should fix
the issue without breaking anything else. Hope everyone is happy 
with this.

 drivers/usb/musb/omap2430.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
index 4315d35..fbe2c08 100644
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -48,6 +48,7 @@ struct omap2430_glue {
 	enum omap_musb_vbus_id_status status;
 	struct work_struct	omap_musb_mailbox_work;
 	struct device		*control_otghs;
+	u8 	initialized;
 };
 #define glue_to_musb(g)		platform_get_drvdata(g->musb)
 
@@ -383,6 +384,7 @@ static int omap2430_musb_init(struct musb *musb)
 	}
 
 	musb_writel(musb->mregs, OTG_INTERFSEL, l);
+	glue->initialized = 1;
 
 	pr_debug("HS USB OTG: revision 0x%x, sysconfig 0x%02x, "
 			"sysstatus 0x%x, intrfsel 0x%x, simenable  0x%x\n",
@@ -509,6 +511,7 @@ static int omap2430_probe(struct platform_device *pdev)
 	glue->dev			= &pdev->dev;
 	glue->musb			= musb;
 	glue->status			= OMAP_MUSB_UNKNOWN;
+	glue->initialized	= 0;
 
 	if (np) {
 		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
@@ -646,7 +649,8 @@ static int omap2430_runtime_resume(struct device *dev)
 
 	if (musb) {
 		omap2430_low_level_init(musb);
-		musb_writel(musb->mregs, OTG_INTERFSEL,
+		if(glue->initialized)
+			musb_writel(musb->mregs, OTG_INTERFSEL,
 				musb->context.otg_interfsel);
 
 		usb_phy_set_suspend(musb->xceiv, 0);
-- 
1.8.4.1

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

[David Cohen]

On Tue, Dec 17, 2013 at 05:48:33PM +0100, anaumann@ultratronik.de wrote:
> 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.
> 
> Fix it by not writing the value on runtime resume if it has not been
> initialized yet.
> 
> Signed-off-by: Andreas Naumann <anaumann@ultratronik.de>
> ---
> Even though I find the implementation a bit awkward this should fix
> the issue without breaking anything else. Hope everyone is happy 
> with this.
> 
>  drivers/usb/musb/omap2430.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
> index 4315d35..fbe2c08 100644
> --- a/drivers/usb/musb/omap2430.c
> +++ b/drivers/usb/musb/omap2430.c
> @@ -48,6 +48,7 @@ struct omap2430_glue {
>  	enum omap_musb_vbus_id_status status;
>  	struct work_struct	omap_musb_mailbox_work;
>  	struct device		*control_otghs;
> +	u8 	initialized;
>  };
>  #define glue_to_musb(g)		platform_get_drvdata(g->musb)
>  
> @@ -383,6 +384,7 @@ static int omap2430_musb_init(struct musb *musb)
>  	}
>  
>  	musb_writel(musb->mregs, OTG_INTERFSEL, l);
> +	glue->initialized = 1;
>  
>  	pr_debug("HS USB OTG: revision 0x%x, sysconfig 0x%02x, "
>  			"sysstatus 0x%x, intrfsel 0x%x, simenable  0x%x\n",
> @@ -509,6 +511,7 @@ static int omap2430_probe(struct platform_device *pdev)
>  	glue->dev			= &pdev->dev;
>  	glue->musb			= musb;
>  	glue->status			= OMAP_MUSB_UNKNOWN;
> +	glue->initialized	= 0;

You don't need to do this. 'glue' was already allocated with kzalloc().

>  
>  	if (np) {
>  		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> @@ -646,7 +649,8 @@ static int omap2430_runtime_resume(struct device *dev)
>  
>  	if (musb) {
>  		omap2430_low_level_init(musb);
> -		musb_writel(musb->mregs, OTG_INTERFSEL,
> +		if(glue->initialized)

Are you sure this is thread safe?
If you're sending this patch it means runtime_resume can be called
before omap2430_must_init(), but how about at the same time?
You defined 'initialized' as u8 type, then read/write operations won't
be atomic in ARM.

Br, David Cohen

> +			musb_writel(musb->mregs, OTG_INTERFSEL,
>  				musb->context.otg_interfsel);
>  
>  		usb_phy_set_suspend(musb->xceiv, 0);
> -- 
> 1.8.4.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Andreas Naumann]

Am 17.12.2013 18:22, schrieb David Cohen:
> On Tue, Dec 17, 2013 at 05:48:33PM +0100, anaumann@ultratronik.de wrote:
>> 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.
>>
>> Fix it by not writing the value on runtime resume if it has not been
>> initialized yet.
>>
>> Signed-off-by: Andreas Naumann <anaumann@ultratronik.de>
>> ---
>> Even though I find the implementation a bit awkward this should fix
>> the issue without breaking anything else. Hope everyone is happy
>> with this.
>>
>>   drivers/usb/musb/omap2430.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
>> index 4315d35..fbe2c08 100644
>> --- a/drivers/usb/musb/omap2430.c
>> +++ b/drivers/usb/musb/omap2430.c
>> @@ -48,6 +48,7 @@ struct omap2430_glue {
>>   	enum omap_musb_vbus_id_status status;
>>   	struct work_struct	omap_musb_mailbox_work;
>>   	struct device		*control_otghs;
>> +	u8 	initialized;
>>   };
>>   #define glue_to_musb(g)		platform_get_drvdata(g->musb)
>>
>> @@ -383,6 +384,7 @@ static int omap2430_musb_init(struct musb *musb)
>>   	}
>>
>>   	musb_writel(musb->mregs, OTG_INTERFSEL, l);
>> +	glue->initialized = 1;
>>
>>   	pr_debug("HS USB OTG: revision 0x%x, sysconfig 0x%02x, "
>>   			"sysstatus 0x%x, intrfsel 0x%x, simenable  0x%x\n",
>> @@ -509,6 +511,7 @@ static int omap2430_probe(struct platform_device *pdev)
>>   	glue->dev			= &pdev->dev;
>>   	glue->musb			= musb;
>>   	glue->status			= OMAP_MUSB_UNKNOWN;
>> +	glue->initialized	= 0;
>
> You don't need to do this. 'glue' was already allocated with kzalloc().

ok

>
>>
>>   	if (np) {
>>   		pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>> @@ -646,7 +649,8 @@ static int omap2430_runtime_resume(struct device *dev)
>>
>>   	if (musb) {
>>   		omap2430_low_level_init(musb);
>> -		musb_writel(musb->mregs, OTG_INTERFSEL,
>> +		if(glue->initialized)
>
> Are you sure this is thread safe?
> If you're sending this patch it means runtime_resume can be called
> before omap2430_must_init(), but how about at the same time?
> You defined 'initialized' as u8 type, then read/write operations won't
> be atomic in ARM.

You're right, wasnt thinking of that. Shall I use atomic_t and helpers?

>
> Br, David Cohen
>
>> +			musb_writel(musb->mregs, OTG_INTERFSEL,
>>   				musb->context.otg_interfsel);
>>
>>   		usb_phy_set_suspend(musb->xceiv, 0);
>> --
>> 1.8.4.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[Grazvydas Ignotas]

On Wed, Dec 18, 2013 at 9:41 AM, Andreas Naumann <dev@andin.de> wrote:
> Am 17.12.2013 18:22, schrieb David Cohen:
>
>> On Tue, Dec 17, 2013 at 05:48:33PM +0100, anaumann@ultratronik.de wrote:
>>>
>>> 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.
>>>
>>> Fix it by not writing the value on runtime resume if it has not been
>>> initialized yet.
>>>
>>> Signed-off-by: Andreas Naumann <anaumann@ultratronik.de>
>>> ---
>>> Even though I find the implementation a bit awkward this should fix
>>> the issue without breaking anything else. Hope everyone is happy
>>> with this.
>>>
>>>   drivers/usb/musb/omap2430.c | 6 +++++-
>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
>>> index 4315d35..fbe2c08 100644
>>> --- a/drivers/usb/musb/omap2430.c
>>> +++ b/drivers/usb/musb/omap2430.c
>>> @@ -48,6 +48,7 @@ struct omap2430_glue {
>>>         enum omap_musb_vbus_id_status status;
>>>         struct work_struct      omap_musb_mailbox_work;
>>>         struct device           *control_otghs;
>>> +       u8      initialized;
>>>   };
>>>   #define glue_to_musb(g)               platform_get_drvdata(g->musb)
>>>
>>> @@ -383,6 +384,7 @@ static int omap2430_musb_init(struct musb *musb)
>>>         }
>>>
>>>         musb_writel(musb->mregs, OTG_INTERFSEL, l);
>>> +       glue->initialized = 1;
>>>
>>>         pr_debug("HS USB OTG: revision 0x%x, sysconfig 0x%02x, "
>>>                         "sysstatus 0x%x, intrfsel 0x%x, simenable
>>> 0x%x\n",
>>> @@ -509,6 +511,7 @@ static int omap2430_probe(struct platform_device
>>> *pdev)
>>>         glue->dev                       = &pdev->dev;
>>>         glue->musb                      = musb;
>>>         glue->status                    = OMAP_MUSB_UNKNOWN;
>>> +       glue->initialized       = 0;
>>
>>
>> You don't need to do this. 'glue' was already allocated with kzalloc().
>
>
> ok
>
>
>>
>>>
>>>         if (np) {
>>>                 pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata),
>>> GFP_KERNEL);
>>> @@ -646,7 +649,8 @@ static int omap2430_runtime_resume(struct device
>>> *dev)
>>>
>>>         if (musb) {
>>>                 omap2430_low_level_init(musb);
>>> -               musb_writel(musb->mregs, OTG_INTERFSEL,
>>> +               if(glue->initialized)
>>
>>
>> Are you sure this is thread safe?
>> If you're sending this patch it means runtime_resume can be called
>> before omap2430_must_init(), but how about at the same time?

No, the problem is that omap2430_runtime_resume() is called _during_
omap2430_must_init(), when pm_runtime_get_sync() is called. We can't
read the initial register value before pm_runtime_get_sync() returns
because the hardware is not powered up, and from pm_runtime_get_sync()
omap2430_runtime_resume() is called, where the cached register value
is needed.

>> You defined 'initialized' as u8 type, then read/write operations won't
>> be atomic in ARM.

We would only have problems if runtime_suspend() and runtime_resume()
are called at the same time, can this really happed?

Grazvydas

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

[Felipe Balbi]

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

Hi,

On Tue, Dec 17, 2013 at 05:48:33PM +0100, anaumann@ultratronik.de wrote:
> 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.

yeah, but the problem is not on the glue layer. The bug is omap_device
and pm_runtime not agreeing on device's state. I suppose there was a fix
for that recently in linux-omap@vger mailing list.

-- 
balbi

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

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

[Grazvydas Ignotas]

On Wed, Dec 18, 2013 at 5:35 PM, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote:
> Hi,
>
> On Tue, Dec 17, 2013 at 05:48:33PM +0100, 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.
>
> yeah, but the problem is not on the glue layer. The bug is omap_device
> and pm_runtime not agreeing on device's state. I suppose there was a fix
> for that recently in linux-omap@vger mailing list.

You mean this: http://marc.info/?t=138444882600003&r=1&w=2 ?

This looks like a different issue during suspend, this problem is at
startup. Both musb_core and omap2430.c expect hardware to be disabled
on startup, and that works as expected. The problem is on first
pm_runtime_get_sync(), which results in first runtime_resume() call,
musb_core checks for first resume and doesn't load yet-unset context
in that case, however glue does and breaks things. We have this
problem since 3.2.


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

[Andreas Naumann]

Am 19.12.2013 00:40, schrieb Grazvydas Ignotas:
> On Wed, Dec 18, 2013 at 5:35 PM, Felipe Balbi <balbi@ti.com> wrote:
>> Hi,
>>
>> On Tue, Dec 17, 2013 at 05:48:33PM +0100, anaumann@ultratronik.de wrote:
>>> 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.
>>
>> yeah, but the problem is not on the glue layer. The bug is omap_device
>> and pm_runtime not agreeing on device's state. I suppose there was a fix
>> for that recently in linux-omap@vger mailing list.
>
> You mean this: http://marc.info/?t=138444882600003&r=1&w=2 ?
>
> This looks like a different issue during suspend, this problem is at
> startup. Both musb_core and omap2430.c expect hardware to be disabled
> on startup, and that works as expected. The problem is on first
> pm_runtime_get_sync(), which results in first runtime_resume() call,
> musb_core checks for first resume and doesn't load yet-unset context
> in that case, however glue does and breaks things. We have this
> problem since 3.2.

I'm a bit short on time at the moment, but shall I have a go on another 
version of the patch with the init-variable as atomic_t?

Andreas

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