From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932851AbdC2N6y (ORCPT ); Wed, 29 Mar 2017 09:58:54 -0400 Received: from fllnx210.ext.ti.com ([198.47.19.17]:35377 "EHLO fllnx210.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932302AbdC2N6w (ORCPT ); Wed, 29 Mar 2017 09:58:52 -0400 Subject: Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode To: Felipe Balbi References: <1487250377-13653-1-git-send-email-rogerq@ti.com> <1487250377-13653-5-git-send-email-rogerq@ti.com> <87o9wlhd1j.fsf@linux.intel.com> <87d1d0l6f4.fsf@linux.intel.com> <1e4bc161-7f21-1969-8471-9674020c9ae1@ti.com> <877f38kylf.fsf@linux.intel.com> CC: , , From: Roger Quadros Message-ID: Date: Wed, 29 Mar 2017 16:58:44 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <877f38kylf.fsf@linux.intel.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 29/03/17 16:21, Felipe Balbi wrote: > > Hi, > > Roger Quadros writes: >>> Roger Quadros writes: >>>>> Roger Quadros writes: >>>>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode >>>>>> when we're operating in dual-role. >>>>> >>>>> yeah, that's not a quirk. DRA7 supports OTGv2, not OTGv3. There was no >>>>> USB3 when OTGv2 was written. >>>>> >>>>> DRA7 just shouldn't use OTG core altogether. In fact, this is the very >>>>> thing I've been saying for a long time. Make the simplest implementation >>>>> possible. The dead simple, does-one-thing-only sort of implementation. >>>>> >>>>> All we need for Dual-Role (without OTG extras) is some input for ID and >>>>> VBUS, then we add/remove HCD/UDC conditionally and set PRTCAPDIR. >>>>> >>>> >>>> The catch is that on AM437x there is no way to get ID and VBUS events other >>>> than the OTG controller so we have to rely on the OTG controller for that. :( >>> >>> okay, so AM437x can get OTG interrupts properly. That's fine. We can >>> still do everything we need using code that's already existing in dwc3 >>> if we refactor it a bit and hook it up to the OTG IRQ handler. >>> >>> Here's what we do: >>> >>> * First we re-factor all necessary code around so the API for OTG/DRD >>> is resumed to calling: >>> >>> dwc3_add_udc(dwc); >>> dwc3_del_udc(dwc); >>> dwc3_add_hcd(dwc); >>> dwc3_del_hcd(dwc); >>> >>> the semantics of these should be easy to understand and you can >>> implement each in their respective host.c/gadget.c files. >>> >>> * Second step is to modify our dwc3_init_mode() (or whatever that >>> function was called, sorry, didn't check) to make sure we have >>> something like: >>> >>> case OTG: >>> dwc3_add_udc(dwc); >>> break; >>> >>> We should *not* add HCD in this case yet. >>> >>> * After that we add otg.c (or drd.c, no preference) and make that call >>> dwc3_add_udc(dwc) and, also, provide >>> dwc3_add_otg(dwc)/dwc3_del_otg(dwc) calls. Then patch the switch >>> statement above to: >>> >>> case OTG: >>> dwc3_add_otg(dwc); >>> break; >>> >>> Note that at this point, this is simply a direct replacement of >>> dwc3_add_udc() to dwc3_add_otg(). This should maintain current behavior >>> (which is starting with peripheral mode by default), but it should also >>> add support for OTG interrupts to change the mode (from an interrupt >>> thead) >>> >>> otg_isr() >>> { >>> >>> /* don't forget to remove preivous mode if necessary */ >>> if (perimode) >>> dwc3_add_udc(dwc); >>> else >>> dwc3_add_hcd(dwc); >>> } >>> >>> * The next patch would be to choose default conditionally based on >>> PERIMODE or whatever. >>> >>> Of course, this is an oversimplified view of reality. You still need to >>> poke around at PRTCAPDIR, etc. But all this can, actually, be prototyped >>> using our "mode" debugfs file. Just make that call >>> dwc3_add/del_udc/hcd() apart from fiddling with PRTCAPDIR in GCTL. >>> >>> Your first implementation could be just that. Refactoring what needs to >>> be refactored, then patching "mode" debugfs to work properly in that >>> case. Only add otg.c/drd.c after "mode" debugfs file is stable, because >>> then you know what needs to be taken into consideration. >>> >>> Just to be clear, I'm not saying we should *ONLY* get the debugfs >>> interface for v4.12, I'm saying you should start with that and get that >>> stable and working properly (make an infinite loop constantly changing >>> modes and keep it running over the weekend) before you add support for >>> OTG interrupts, which could come in the same series ;-) >>> >> >> Agree with you. Moreover I could get rid of OTG controller related code >> and have just debugfs and extcon implementation. We can add the OTG controller >> bits later. >> >> I agree with you on everything you said except using add/del_gadget_udc. :) >> I've explained why we can't use del_gadget_udc in the other thread >> but I'll explain it here again. >> >> 1) If we start in host role, usb_add_gadget_udc() won't be called. That means >> no UDC and user can't load a gadget driver. Typical applications need to have >> a gadget driver ready *before* the peripheral mode starts so that it can >> enumerate immediately. > > that has changed since you started writing this series :-) gadget > drivers are kept in pending list until a UDC is around. I'll get > information on that tomorrow, if you require. "until a UDC is around" is the key point. If we never call usb_add_gadget_udc() or we call usb_del_gadget_udc() then the UDC is not around right? > >> 2) If we use usb_del_gadget_udc() when switching to host mode and >> usb_add_gadget_udc() when switching back to peripheral mode, the previously >> loaded gadget driver will not be assigned to this UDC. User has to unload >> and reload the gadget driver. > > that should not be the case anymore, if it is we have a bug in udc-core OK. good to know. > >> 3) All this becomes even more complex for configfs based gadget driver. >> >> So using stop/start gadget is a much simpler solution really as UDC software >> side of things remain unchanged and the gadget driver can persist between >> role switches. > > I hadn't considered configfs, I'll try this out tomorrow as well. > Al-right, thanks. cheers, -roger