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