* Re: WG: [PATCH] usb: musb: omap2430: fix occasional musb breakage on boot
[not found] ` <EE4E4767751A8B40949306E403F4DDD8032AA82A-zDU4f0UzlK41cL/Vr7vMjYWky2maIgj/G9Ur7JDdleE@public.gmane.org>
@ 2013-12-12 21:29 ` Andreas Naumann
2013-12-13 12:34 ` Grazvydas Ignotas
0 siblings, 1 reply; 4+ messages in thread
From: Andreas Naumann @ 2013-12-12 21:29 UTC (permalink / raw)
To: linux-usb-u79uwXL29TY76Z2rM5mHXA
Cc: fbalbi-l0cyMroinI0, linux-omap-u79uwXL29TY76Z2rM5mHXA,
stable-u79uwXL29TY76Z2rM5mHXA, notasas-Re5JQEeQqe8AvxtiuMwx3w
Hi Grazvydas,
> Von: Grazvydas Ignotas [mailto:notasas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org]
> Gesendet: Donnerstag, 12. Dezember 2013 01:21
> An: linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: Felipe Balbi; linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; Naumann Andreas; Grazvydas Ignotas; stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Betreff: [PATCH] usb: musb: omap2430: fix occasional musb breakage on boot
>
> 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
> e25bec160158abe86c "omap2+: save and restore OTG_INTERFSEL",
> 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 is 0
> (0 should never be saved in the context as it's invalid value,
> so we use it as an indicator that context hasn't been saved yet).
>
> This issue was originally found by Andreas Naumann:
> http://marc.info/?l=linux-usb&m=138562574719654&w=2
>
> Reported-and-bisected-by: Andreas Naumann <anaumann-ZKHRqZ6+gQUX0D0ZMPkEVw@public.gmane.org>
> Signed-off-by: Grazvydas Ignotas <notasas-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: <stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>
> ---
> This is a regression from 3.2, so should go to -rc and stable, IMO.
> It's really annoying issue if you want to have a stable OTG behavior,
> I've burned quite a lot of time on it myself over a year ago and gave up
> eventually. Good thing Andreas finally found it, many thanks to him!
>
> drivers/usb/musb/omap2430.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
> index 2a408cd..737b3da 100644
> --- a/drivers/usb/musb/omap2430.c
> +++ b/drivers/usb/musb/omap2430.c
> @@ -672,7 +672,8 @@ static int omap2430_runtime_resume(struct device *dev)
>
> if (musb) {
> omap2430_low_level_init(musb);
> - musb_writel(musb->mregs, OTG_INTERFSEL,
> + if (musb->context.otg_interfsel != 0)
> + musb_writel(musb->mregs, OTG_INTERFSEL,
> musb->context.otg_interfsel);
> phy_power_on(musb->phy);
> }
>
Oh, easy way out. I like it but I've also been thinking about your
comment on my original post, which was that initializing otg_interfsel
to the PHYSEL bits only might be dangerous because we cant be sure that
there are other bits in the register.
However, isnt assuming that 0 is invalid on all OMAPs just as dangerous?
After thinking about my patch again, I would propose to change
otg_interfsel into otg_physel and read-modify-write only those bits in
resume() as you suggested in your first answer. That way I could discard
the problematic first read in probe() while leaving other bits
untouched. If you agree I post a patch for this tomorrow.
cheers,
Andreas
--
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: WG: [PATCH] usb: musb: omap2430: fix occasional musb breakage on boot
2013-12-12 21:29 ` WG: [PATCH] usb: musb: omap2430: fix occasional musb breakage on boot Andreas Naumann
@ 2013-12-13 12:34 ` Grazvydas Ignotas
2013-12-13 16:27 ` Andreas Naumann
0 siblings, 1 reply; 4+ messages in thread
From: Grazvydas Ignotas @ 2013-12-13 12:34 UTC (permalink / raw)
To: Andreas Naumann; +Cc: Linux USB Mailing List, fbalbi, linux-omap
On Thu, Dec 12, 2013 at 11:29 PM, Andreas Naumann <dev@andin.de> wrote:
> Hi Grazvydas,
>
>> Von: Grazvydas Ignotas [mailto:notasas@gmail.com]
>> Gesendet: Donnerstag, 12. Dezember 2013 01:21
>> An: linux-usb@vger.kernel.org
>> Cc: Felipe Balbi; linux-omap@vger.kernel.org; Naumann Andreas; Grazvydas
>> Ignotas; stable@vger.kernel.org
>> Betreff: [PATCH] usb: musb: omap2430: fix occasional musb breakage on boot
>>
>>
>> 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
>> e25bec160158abe86c "omap2+: save and restore OTG_INTERFSEL",
>> 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 is 0
>> (0 should never be saved in the context as it's invalid value,
>> so we use it as an indicator that context hasn't been saved yet).
>>
>> This issue was originally found by Andreas Naumann:
>> http://marc.info/?l=linux-usb&m=138562574719654&w=2
>>
>> Reported-and-bisected-by: Andreas Naumann <anaumann@ultratronik.de>
>> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
>> Cc: <stable@vger.kernel.org>
>> ---
>> This is a regression from 3.2, so should go to -rc and stable, IMO.
>> It's really annoying issue if you want to have a stable OTG behavior,
>> I've burned quite a lot of time on it myself over a year ago and gave up
>> eventually. Good thing Andreas finally found it, many thanks to him!
>>
>> drivers/usb/musb/omap2430.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
>> index 2a408cd..737b3da 100644
>> --- a/drivers/usb/musb/omap2430.c
>> +++ b/drivers/usb/musb/omap2430.c
>> @@ -672,7 +672,8 @@ static int omap2430_runtime_resume(struct device *dev)
>>
>> if (musb) {
>> omap2430_low_level_init(musb);
>> - musb_writel(musb->mregs, OTG_INTERFSEL,
>> + if (musb->context.otg_interfsel != 0)
>> + musb_writel(musb->mregs, OTG_INTERFSEL,
>> musb->context.otg_interfsel);
>> phy_power_on(musb->phy);
>> }
>>
>
> Oh, easy way out. I like it but I've also been thinking about your comment
> on my original post, which was that initializing otg_interfsel to the PHYSEL
> bits only might be dangerous because we cant be sure that there are other
> bits in the register.
> However, isnt assuming that 0 is invalid on all OMAPs just as dangerous?
Well I was trying to do a minimal fix so that it could be suitable for
merging to stable kernels. But yes you're right, I've just checked
OMAP4 TRM and 0 is actually valid value there..
> After thinking about my patch again, I would propose to change otg_interfsel
> into otg_physel and read-modify-write only those bits in resume() as you
> suggested in your first answer. That way I could discard the problematic
> first read in probe() while leaving other bits untouched. If you agree I
> post a patch for this tomorrow.
Hmm I don't know about that, this would be inconsistent with what all
other OMAP drivers do. Maybe we should do what musb_core.c does just
to be consistent and add a similar comment. Only the static variable
could be avoided in favor of struct omap2430_glue member.
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: WG: [PATCH] usb: musb: omap2430: fix occasional musb breakage on boot
2013-12-13 12:34 ` Grazvydas Ignotas
@ 2013-12-13 16:27 ` Andreas Naumann
2013-12-16 10:49 ` Grazvydas Ignotas
0 siblings, 1 reply; 4+ messages in thread
From: Andreas Naumann @ 2013-12-13 16:27 UTC (permalink / raw)
To: Grazvydas Ignotas; +Cc: Linux USB Mailing List, fbalbi, linux-omap
On 13.12.2013 13:34, Grazvydas Ignotas wrote:
> On Thu, Dec 12, 2013 at 11:29 PM, Andreas Naumann <dev@andin.de> wrote:
>> Hi Grazvydas,
>>
>>> Von: Grazvydas Ignotas [mailto:notasas@gmail.com]
>>> Gesendet: Donnerstag, 12. Dezember 2013 01:21
>>> An: linux-usb@vger.kernel.org
>>> Cc: Felipe Balbi; linux-omap@vger.kernel.org; Naumann Andreas; Grazvydas
>>> Ignotas; stable@vger.kernel.org
>>> Betreff: [PATCH] usb: musb: omap2430: fix occasional musb breakage on boot
>>>
>>>
>>> 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
>>> e25bec160158abe86c "omap2+: save and restore OTG_INTERFSEL",
>>> 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 is 0
>>> (0 should never be saved in the context as it's invalid value,
>>> so we use it as an indicator that context hasn't been saved yet).
>>>
>>> This issue was originally found by Andreas Naumann:
>>> http://marc.info/?l=linux-usb&m=138562574719654&w=2
>>>
>>> Reported-and-bisected-by: Andreas Naumann <anaumann@ultratronik.de>
>>> Signed-off-by: Grazvydas Ignotas <notasas@gmail.com>
>>> Cc: <stable@vger.kernel.org>
>>> ---
>>> This is a regression from 3.2, so should go to -rc and stable, IMO.
>>> It's really annoying issue if you want to have a stable OTG behavior,
>>> I've burned quite a lot of time on it myself over a year ago and gave up
>>> eventually. Good thing Andreas finally found it, many thanks to him!
>>>
>>> drivers/usb/musb/omap2430.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
>>> index 2a408cd..737b3da 100644
>>> --- a/drivers/usb/musb/omap2430.c
>>> +++ b/drivers/usb/musb/omap2430.c
>>> @@ -672,7 +672,8 @@ static int omap2430_runtime_resume(struct device *dev)
>>>
>>> if (musb) {
>>> omap2430_low_level_init(musb);
>>> - musb_writel(musb->mregs, OTG_INTERFSEL,
>>> + if (musb->context.otg_interfsel != 0)
>>> + musb_writel(musb->mregs, OTG_INTERFSEL,
>>> musb->context.otg_interfsel);
>>> phy_power_on(musb->phy);
>>> }
>>>
>>
>> Oh, easy way out. I like it but I've also been thinking about your comment
>> on my original post, which was that initializing otg_interfsel to the PHYSEL
>> bits only might be dangerous because we cant be sure that there are other
>> bits in the register.
>> However, isnt assuming that 0 is invalid on all OMAPs just as dangerous?
>
> Well I was trying to do a minimal fix so that it could be suitable for
> merging to stable kernels. But yes you're right, I've just checked
> OMAP4 TRM and 0 is actually valid value there..
>
>> After thinking about my patch again, I would propose to change otg_interfsel
>> into otg_physel and read-modify-write only those bits in resume() as you
>> suggested in your first answer. That way I could discard the problematic
>> first read in probe() while leaving other bits untouched. If you agree I
>> post a patch for this tomorrow.
>
> Hmm I don't know about that, this would be inconsistent with what all
> other OMAP drivers do. Maybe we should do what musb_core.c does just
Ok, thats cool.
> to be consistent and add a similar comment. Only the static variable
> could be avoided in favor of struct omap2430_glue member.
Whats wrong with the static?
cheers,
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: WG: [PATCH] usb: musb: omap2430: fix occasional musb breakage on boot
2013-12-13 16:27 ` Andreas Naumann
@ 2013-12-16 10:49 ` Grazvydas Ignotas
0 siblings, 0 replies; 4+ messages in thread
From: Grazvydas Ignotas @ 2013-12-16 10:49 UTC (permalink / raw)
To: Andreas Naumann; +Cc: Linux USB Mailing List, linux-omap
On Fri, Dec 13, 2013 at 6:27 PM, Andreas Naumann <dev@andin.de> wrote:
> On 13.12.2013 13:34, Grazvydas Ignotas wrote:
>>
>> Hmm I don't know about that, this would be inconsistent with what all
>> other OMAP drivers do. Maybe we should do what musb_core.c does just
>
> Ok, thats cool.
>
>> to be consistent and add a similar comment. Only the static variable
>> could be avoided in favor of struct omap2430_glue member.
>
> Whats wrong with the static?
Well besides minor memory wastage when the driver is compiled in but
not used, you'd cause problems if TI makes a SoC with multiple musb
instances. Yes this driver already contains global variables, but
still we can avoid adding more easily.
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2013-12-16 10:49 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <EE4E4767751A8B40949306E403F4DDD8032AA82A@sv03ultra002.ultratronik.de>
[not found] ` <EE4E4767751A8B40949306E403F4DDD8032AA82A-zDU4f0UzlK41cL/Vr7vMjYWky2maIgj/G9Ur7JDdleE@public.gmane.org>
2013-12-12 21:29 ` WG: [PATCH] usb: musb: omap2430: fix occasional musb breakage on boot Andreas Naumann
2013-12-13 12:34 ` Grazvydas Ignotas
2013-12-13 16:27 ` Andreas Naumann
2013-12-16 10:49 ` Grazvydas Ignotas
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.