Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756225AbbLHJKQ (ORCPT ); Tue, 8 Dec 2015 04:10:16 -0500 Received: from down.free-electrons.com ([37.187.137.238]:59394 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1756096AbbLHJKK convert rfc822-to-8bit (ORCPT ); Tue, 8 Dec 2015 04:10:10 -0500 From: Gregory CLEMENT To: Felipe Balbi Cc: , Greg Kroah-Hartman , , , , Subject: Re: [PATCH] usb: musb: dsps: handle the otg_state_a_wait_vrise_timeout case References: <1440087153-31084-1-git-send-email-gregory.clement@free-electrons.com> <55D719B1.8010700@free-electrons.com> <87twrfsbed.fsf@free-electrons.com> <87d1wtf5ir.fsf@saruman.tx.rr.com> <87a8rde96m.fsf@free-electrons.com> <87poyiegc7.fsf@free-electrons.com> <87poyinjpc.fsf@saruman.tx.rr.com> Date: Tue, 08 Dec 2015 10:07:58 +0100 In-Reply-To: <87poyinjpc.fsf@saruman.tx.rr.com> (Felipe Balbi's message of "Mon, 7 Dec 2015 13:26:55 -0600") Message-ID: <874mft9ukx.fsf@free-electrons.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5765 Lines: 159 Hi Felipe, On lun., déc. 07 2015, Felipe Balbi wrote: > Hi, > > Gregory CLEMENT writes: >> Hi Felipe, >> >> I am going back on this subject (again :) ) >> >> On mar., oct. 20 2015, Gregory CLEMENT wrote: >> >>> Hi Felipe, >>> >>> On lun., oct. 05 2015, Felipe Balbi wrote: >>> >>> >>>>> So after many tests on different devices, 200ms is enough for most of >>>>> them, but for one, 2000ms (2s) was needed! >>>>> >>>>> I see several option: >>>>> - adding a sysfs entry to setup the time >>>>> - adding a debugs entry entry >>>>> - adding configuration option in menuconfig >>>>> - using 2000ms and hopping it was enough for everyone >>>>> >>>>> My preference would go to the first option, what is your opinion? >>>> >>>> I'm not sure if either of them is good. man 2s is just too large. If the >> >> Well 2s is lon I agree, but currently instead of 2s we have an infinite >> wait, which is worse! >> >>>> device isn't following the specification, I'm afraid that device needs >>>> to be fixed. >> >> I agree but these devices are for most of them USB stick that we find in >> retail. We have no influence on them, so we have to do with them, even >> if they do not follow the sepc. >> >> So what about using configfs or sysfs to let the user confgiure this >> value if needed? > > iff we have to; I'm still not 100% convinced. > >> I go back on this because, your suggestion didn't work. At least, I >> didn't manage to make it improve the situation. >> >> Thanks, >> >>>> >>>> I think the real issue here is the lack of a disconnect IRQ from AM335x >>>> devices. But here's something I've been meaning to test but never had >>>> time. If you look into AM335x address space, there's a bit in the >>>> wrapper which tells it to use the standard MUSB registers for IRQ, >>>> instead of the TI-specific thing. Can you see if we get a disconnect IRQ >>>> with that ? >>>> >>>> TRM is at [1], see page 2566. Basically, if you set bit 3 in register >>>> offset 0x1014, then it should use Mentor IRQ registers. If that works, >>>> quite a few problems will actually vanish :-p >>> >>> So I tried it with the following patch: >>> >>> ------------------------------------- >>> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c >>> index c791ba5..c714500 100644 >>> --- a/drivers/usb/musb/musb_dsps.c >>> +++ b/drivers/usb/musb/musb_dsps.c >>> @@ -470,6 +470,7 @@ static int dsps_musb_init(struct musb *musb) >>> >>> /* Reset the musb */ >>> dsps_writel(reg_base, wrp->control, (1 << wrp->reset)); >>> + dsps_writel(musb->ctrl_base, wrp->control, BIT(3)); > > overwritting reset. > >>> musb->isr = dsps_interrupt; >>> >>> @@ -625,6 +626,7 @@ static int dsps_musb_reset(struct musb *musb) >>> if (session_restart || !glue->sw_babble_enabled) { >>> dev_info(musb->controller, "Restarting MUSB to recover from Babble\n"); >>> dsps_writel(musb->ctrl_base, wrp->control, (1 << wrp->reset)); >>> + dsps_writel(musb->ctrl_base, wrp->control, BIT(3)); > > here too. No wonder it won't work, right :-) > >>> I am not very familiar with that driver but my understanding was that >>> the Mentor IRQ registeres are managed by the musb_interrupt() function >>> which is called from the dsps_interrupt() interrupt handler. >>> >>> Am I right? > > yeah, however the way IRQs are reported is a different thing. AM335x > introduced its own IRQ reporting scheme which, basically, reads MUSB > generic IRQ status registers and reports on an AM335x specific > register. No idea why TI did that for AM335x devices. > >>> if it is the case then it didn't fix the issue I had. >>> >>> I activated the following debug line: >>> >>> [musb_hdrc]musb_interrupt =_ "** IRQ %s usb%04x tx%04x rx%04x\012" >>> [musb_dsps]dsps_interrupt =p "usbintr (%x) epintr(%x)\012" >>> >>> But I didn't get any interrupt while disconnecting the cable without any >>> device connected on it (whereas I got an interrupt when I connected it). >>> >>> Note that I applied this patch instead of the "usb: musb: dsps: handle >>> the otg_state_a_wait_vrise_timeout case", is what you had in mind ? > > yeah, that's what I had in mind. But your patch seems wrong :-) > > I tried writing a more correct version here and found 2 issues: > > a) bit 3 doesn't do anything :-p I cannot read IRQs from mentor's > registers > > b) when setting RESET_ISOLATION bit, reads of CTRL register hang. Note > that according to TRM, RESET_ISOLATION _must_ be set prior to a soft > reset and cleared afterwards. But right after setting RESET_ISOLATION, > if I try a read of CTRL, it'll hang forever. The datasheet seems not very coherent about it, on one side we have: "This bit should be set high prior to setting bit 0 and cleared after bit 0 is cleared." and on the other side: "Both the soft_reset and soft_reset_isolation bits should be asserted simultaneously." The hang you saw could be explained by the following: "Setting only the soft_reset_isolation bit will cause all USB0 output signals to go to a known constant value via multiplexers. This will prevent future access to USB0." page 2567 Gregory > > Bin, do you know about these problems ? They both seem rather alarming > to me. > > -- > balbi -- Gregory Clement, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/