From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andreas Naumann Subject: Re: WG: [PATCH] usb: musb: omap2430: fix occasional musb breakage on boot Date: Fri, 13 Dec 2013 17:27:53 +0100 Message-ID: <52AB3589.6010606@andin.de> References: <52AA2AA8.4090402@andin.de> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-13; format=flowed Content-Transfer-Encoding: QUOTED-PRINTABLE Return-path: Received: from dd5612.kasserver.com ([85.13.130.143]:33821 "EHLO dd5612.kasserver.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751872Ab3LMQ16 (ORCPT ); Fri, 13 Dec 2013 11:27:58 -0500 In-Reply-To: Sender: linux-omap-owner@vger.kernel.org List-Id: linux-omap@vger.kernel.org To: Grazvydas Ignotas Cc: Linux USB Mailing List , fbalbi@ti.com, "linux-omap@vger.kernel.org" On 13.12.2013 13:34, Grazvydas Ignotas wrote: > On Thu, Dec 12, 2013 at 11:29 PM, Andreas Naumann wrot= e: >> 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; Graz= vydas >>> 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 u= ses a >>> non-initialized value (which is 0 and a non-supported setting in DM= 37xx >>> 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=3Dlinux-usb&m=3D138562574719654&w=3D2 >>> >>> Reported-and-bisected-by: Andreas Naumann >>> Signed-off-by: Grazvydas Ignotas >>> Cc: >>> --- >>> 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 behavio= r, >>> I've burned quite a lot of time on it myself over a year ago and ga= ve 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/omap243= 0.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 devic= e *dev) >>> >>> if (musb) { >>> omap2430_low_level_init(musb); >>> - musb_writel(musb->mregs, OTG_INTERFSEL, >>> + if (musb->context.otg_interfsel !=3D 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 c= omment >> on my original post, which was that initializing otg_interfsel to th= e 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 danger= ous? > > Well I was trying to do a minimal fix so that it could be suitable fo= r > 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_i= nterfsel >> 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 problem= atic >> first read in probe() while leaving other bits untouched. If you agr= ee 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=FEvydas > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" i= n the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html