Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755271AbdC3GkQ (ORCPT ); Thu, 30 Mar 2017 02:40:16 -0400 Received: from lelnx193.ext.ti.com ([198.47.27.77]:23466 "EHLO lelnx193.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753284AbdC3GkO (ORCPT ); Thu, 30 Mar 2017 02:40:14 -0400 Subject: Re: [PATCH v2 3/4] usb: dwc3: add dual-role support To: Felipe Balbi , Mathias Nyman References: <1487250377-13653-1-git-send-email-rogerq@ti.com> <1487250377-13653-4-git-send-email-rogerq@ti.com> <87r31hhd7g.fsf@linux.intel.com> <46440b24-a2cd-93d6-e92e-2d80482f7d32@ti.com> <87a884kyva.fsf@linux.intel.com> CC: , , From: Roger Quadros Message-ID: Date: Thu, 30 Mar 2017 09:40:07 +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: <87a884kyva.fsf@linux.intel.com> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 23071 Lines: 672 Hi, On 29/03/17 16:15, Felipe Balbi wrote: > > Hi, > > Roger Quadros writes: >>>> @@ -839,6 +841,505 @@ static int dwc3_core_get_phy(struct dwc3 *dwc) >>>> return 0; >>>> } >>>> >>>> +static int dwc3_drd_start_host(struct dwc3 *dwc, int on, bool skip); >>>> +static int dwc3_drd_start_gadget(struct dwc3 *dwc, int on); >>>> + >>>> +/* dwc->lock must be held */ >>>> +static void dwc3_drd_statemachine(struct dwc3 *dwc, int id, int vbus) >>>> +{ >>>> + enum usb_otg_state new_state; >>>> + int protocol; >>>> + >>>> + if (id == dwc->otg_fsm.id && vbus == dwc->otg_fsm.b_sess_vld) >>>> + return; >>>> + >>>> + dwc->otg_fsm.id = id; >>>> + dwc->otg_fsm.b_sess_vld = vbus; >>>> + >>>> + if (!id) { >>>> + new_state = OTG_STATE_A_HOST; >>>> + } else{ >>>> + if (vbus) >>>> + new_state = OTG_STATE_B_PERIPHERAL; >>>> + else >>>> + new_state = OTG_STATE_B_IDLE; >>>> + } >>>> + >>>> + if (dwc->otg.state == new_state) >>>> + return; >>>> + >>>> + protocol = dwc->otg_fsm.protocol; >>>> + switch (new_state) { >>>> + case OTG_STATE_B_IDLE: >>>> + if (protocol == PROTO_GADGET) >>>> + dwc3_drd_start_gadget(dwc, 0); >>>> + else if (protocol == PROTO_HOST) >>>> + dwc3_drd_start_host(dwc, 0, 0); >>>> + dwc->otg_fsm.protocol = PROTO_UNDEF; >>>> + break; >>>> + case OTG_STATE_B_PERIPHERAL: >>>> + if (protocol == PROTO_HOST) >>>> + dwc3_drd_start_host(dwc, 0, 0); >>>> + >>>> + if (protocol != PROTO_GADGET) { >>>> + dwc->otg_fsm.protocol = PROTO_GADGET; >>>> + dwc3_drd_start_gadget(dwc, 1); >>>> + } >>>> + break; >>>> + case OTG_STATE_A_HOST: >>>> + if (protocol == PROTO_GADGET) >>>> + dwc3_drd_start_gadget(dwc, 0); >>>> + >>>> + if (protocol != PROTO_HOST) { >>>> + dwc->otg_fsm.protocol = PROTO_HOST; >>>> + dwc3_drd_start_host(dwc, 1, 0); >>>> + } >>>> + break; >>>> + default: >>>> + dev_err(dwc->dev, "drd: invalid usb-drd state: %s\n", >>>> + usb_otg_state_string(new_state)); >>>> + return; >>>> + } >>>> + >>>> + dwc->otg.state = new_state; >>>> +} >>> >>> I think I've mentioned this before. Why don't we start with the simplest >>> possible implementation? Something that *just* allows us to get ID pin >>> value and set the mode. After that's stable, then we add more pieces to >>> the mix. >> >> That is exactly what I'm doing. Maybe the switch case is making it look complicated. >> dwc3_drd_statemachine() has only 2 inputs VBUS and ID. >> >> I can change it to if/else if you prefer that. I like the way it is cause >> we can define 3 states IDLE, PERIPHERAL and HOST. > > Right, I like the three states, but somehow the code looks really > complex :-s > >>> For something that simple, we wouldn't even need to use OTG FSM layer >>> because that brings no benefit for such a simple requirement. >> >> no no. I think you got it wrong. I'm not using the OTG FSM layer at all :). > > what are all the otg_fsm mentions then? Also you have: > > + struct usb_otg otg; > + struct otg_fsm otg_fsm; > I'll get rid of them. They aren't really needed. Now I see why you thought I was using the otg_fsm layer. :) >>>> +/* dwc->lock must be held */ >>>> +static void dwc3_otg_fsm_sync(struct dwc3 *dwc) >>>> +{ >>>> + u32 reg; >>>> + int id, vbus; >>>> + >>>> + /* >>>> + * calling dwc3_otg_fsm_sync() during resume breaks host >>>> + * if adapter was removed during suspend as xhci driver >>>> + * is not prepared to see hcd removal before xhci_resume. >>>> + */ >>>> + if (dwc->otg_prevent_sync) >>>> + return; >>>> + >>>> + reg = dwc3_readl(dwc->regs, DWC3_OSTS); >>>> + id = !!(reg & DWC3_OSTS_CONIDSTS); >>>> + vbus = !!(reg & DWC3_OSTS_BSESVLD); >>>> + dwc3_drd_statemachine(dwc, id, vbus); >>>> +} >>> >>> Just consider this for a moment. Consider the steps taken to get here. >>> >>> - User plugs cable >>> - Hardirq handler run >>> - read register >>> - if (reg) return IRQ_WAKE_THREAD; >>> - schedule bottom half handler to sometime in the future >>> - scheduler runs our threaded handler >>> - lock dwc3 >>> - if (host) >>> - configure register >>> - add xHCI device >>> - else >>> - otg_fsm_sync() >>> - drd_statemachine() >>> - configure registers >>> - reimplement gadget initialization (same thing we do >>> when registering UDC >>> >>> I mean, just looking at this we can already see that it's really overly >>> complex. Right now we need simple, dead simple. This will limit the >>> possibility of errors. >> >> OK. I can probably get rid of the state machine model and just use if/else? >> Anything else you want me to get rid of? > > The workqueue, unless it's really, really necessary and it appears like > it shouldn't be. OK. > > We _can_ keep the switch statement. The problem is not the switch > statement, it's the reimplementation of code we already have. > > All you do with adding and removing UDC/HCD is already available > somewhere. Perhaps it just needs to be extracted to a function you can > call, but the code is already there, since we need it for > loading/unloading dwc3 itself. > >>>> +static void dwc3_drd_work(struct work_struct *work) >>>> +{ >>>> + struct dwc3 *dwc = container_of(work, struct dwc3, >>>> + otg_work); >>>> + >>>> + spin_lock(&dwc->lock); >>>> + dwc3_otg_fsm_sync(dwc); >>>> + spin_unlock(&dwc->lock); >>>> +} >>> >>> so this is only called from ->complete(). You mentioned your commit log >>> that calling dwc3_otg_fsm_sync() directly from ->complete() can lock up >>> the system. Why? I have a feeling your locking isn't proper and that's >>> why sometimes it locks up. You introduced a deadlock and to work around >>> it, the "solution" was to add a workqueue. >>> >>> Can you either confirm or refute the statement above? >> >> The real problem was that if host adapter was removed during a system suspend >> then while resuming XHCI was locking up like below. This is probably because >> we're trying to remove/Halt the HCD in the otg_irq_handler before XHCI driver has resumed? >> >> How can we ensure that we call dwc3_host_exit() only *after* XHCI driver has resumed? > > Well, xHCI is our child, so driver model should *already* be > guaranteeing that, no? Specially since you're calling this from > ->complete() which happens after ->resume() has been called for the > entire tree. It's true, however, that dwc3's ->complete() will be called > before xhci's ->complete(). Is this the problem you're pointing at? Or > do you mean xHCI is runtime suspended (or runtime resuming) while you > call dwc3_host_exit()? If that's the case, then there's a bug in xHCI > itself. Yes dwc3->complete() being called before xhci->complete(), and so HCD being removed before xhci->complete() causes the lockup. It could be a bug in xHCI driver as well. > > We should be able to remove a device from platform/pci bus at any time. > >> [ 1057.565573] PM: Syncing filesystems ... done. >> [ 1057.573986] PM: Preparing system for sleep (mem) >> [ 1057.580282] Freezing user space processes ... (elapsed 0.001 seconds) done. >> [ 1057.589626] Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done. >> [ 1057.598931] PM: Suspending system (mem) >> [ 1057.617852] PM: suspend of devices complete after 13.163 msecs >> [ 1057.628279] PM: late suspend of devices complete after 4.296 msecs >> [ 1057.635858] PM: noirq resume of devices complete after 0.178 msecs >> [ 1057.642783] PM: noirq suspend of devices failed >> [ 1057.649703] PM: early resume of devices complete after 2.134 msecs >> [ 1057.658046] net eth0: initializing cpsw version 1.15 (0) >> [ 1057.663634] cpsw 48484000.ethernet: initialized cpsw ale version 1.4 >> [ 1057.670325] cpsw 48484000.ethernet: ALE Table size 1024 >> [ 1057.683322] Generic PHY 48485000.mdio:02: attached PHY driver [Generic PHY] (mii_bus:phy_addr=48485000.mdio:02, irq=-1) >> [ 1057.706453] usb usb1: root hub lost power or was reset >> [ 1057.711847] usb usb2: root hub lost power or was reset >> [ 1057.987078] ata1: SATA link down (SStatus 0 SControl 300) >> [ 1059.762288] cpsw 48484000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx >> >> [ 1061.846695] PM: resume of devices complete after 4190.473 msecs >> [ 1061.853294] xhci-hcd xhci-hcd.0.auto: remove, state 1 >> [ 1061.858644] usb usb2: USB disconnect, device number 1 >> [ 1061.890701] xhci-hcd xhci-hcd.0.auto: Host halt failed, -110 >> [ 1061.896640] xhci-hcd xhci-hcd.0.auto: Host controller not halted, aborting reset. >> [ 1061.904535] xhci-hcd xhci-hcd.0.auto: USB bus 2 deregistered >> [ 1061.910514] xhci-hcd xhci-hcd.0.auto: remove, state 1 >> [ 1061.915848] usb usb1: USB disconnect, device number 1 >> [ 1061.921146] usb 1-1: USB disconnect, device number 2 >> >>> >>>> +static void dwc3_otg_disable_events(struct dwc3 *dwc, u32 disable_mask) >>>> +{ >>>> + dwc->oevten &= ~(disable_mask); >>>> + dwc3_writel(dwc->regs, DWC3_OEVTEN, dwc->oevten); >>>> +} >>> >>> we should disable OTG events from our top half handler, otherwise top >>> and bottom half can race with each other. >> >> If we disable OTG events then there is a chance that we miss the events that >> happen while they were disabled. > > no, they'll be in the register. Once we reenable them, then the IRQ line > will be raised once more and our handler will get scheduled. At least when I tested it by disabling events in OEVTEN, the events were missed. There should be another way to mask the interrupts than OEVTEN. > >> We need a way to mask the OTG events without loosing them while they are masked. >> Do you know how that could be achieved? > > masking doesn't clear events. It just masks them. Look at gadget.c for > how we do it. Basically, the code we have here is racy, really racy and > will cause hard-to-debug lockups. Your code can only work with > IRQF_ONESHOT, which we don't want to add back. > > In any case, to mask events, you set BIT 31 in GEVNTSIZ register. Just > copy it from gadget.c ;-) Isn't GEVNTSIZ specific to device side? We're talking about the OTG block here. Are you sure that setting bit 31 of GEVNTSIZ will mask OTG_irq? I don't think so. Note that OTG_IRQ is a separate IRQ line than PERIPHERAL_IRQ. > >>>> + spin_unlock(&dwc->lock); >>>> + >>>> + return IRQ_HANDLED; >>>> +} >>>> + >>>> +static irqreturn_t dwc3_otg_irq(int irq, void *_dwc) >>>> +{ >>>> + u32 reg; >>>> + struct dwc3 *dwc = _dwc; >>>> + irqreturn_t ret = IRQ_NONE; >>>> + >>>> + reg = dwc3_readl(dwc->regs, DWC3_OEVT); >>>> + if (reg) { >>>> + if ((dwc->otg_fsm.protocol == PROTO_HOST) && >>>> + !(reg & DWC3_OEVT_DEVICEMODE)) >>>> + dwc->otg_needs_host_start = 1; >>>> + dwc3_writel(dwc->regs, DWC3_OEVT, reg); >>>> + ret = IRQ_WAKE_THREAD; >>> >>> disable_events(); >> >> We can't disable events if we don't want to miss any events while they were disabled. > > right, disable events is not the best thing, sorry. We should set bit 31 > in GEVNTSIZ. > >> But I agree that we need to prevent them from firing another hard IRQ. >> I couldn't figure out how that can be done. > > gadget.c ;-) > >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +/* --------------------- Dual-Role management ------------------------------- */ >>>> +static void dwc3_otgregs_init(struct dwc3 *dwc) >>>> +{ >>>> + u32 reg; >>>> + >>>> + /* >>>> + * Prevent host/device reset from resetting OTG core. >>>> + * If we don't do this then xhci_reset (USBCMD.HCRST) will reset >>>> + * the signal outputs sent to the PHY, the OTG FSM logic of the >>>> + * core and also the resets to the VBUS filters inside the core. >>>> + */ >>>> + reg = dwc3_readl(dwc->regs, DWC3_OCFG); >>>> + reg |= DWC3_OCFG_SFTRSTMASK; >>>> + dwc3_writel(dwc->regs, DWC3_OCFG, reg); >>>> + >>>> + /* Disable hibernation for simplicity */ >>>> + reg = dwc3_readl(dwc->regs, DWC3_GCTL); >>>> + reg &= ~DWC3_GCTL_GBLHIBERNATIONEN; >>>> + dwc3_writel(dwc->regs, DWC3_GCTL, reg); >>> >>> no, don't do that. We support hibernation on some Intel devices. You'd >>> be regressing them, most likely. >> >> Do they support dual-role as well? Either ways I can omit this step. > > At least for now, let's skip it. OK. > >>>> + /* >>>> + * Initialize OTG registers as per >>>> + * Figure 11-4 OTG Driver Overall Programming Flow >>>> + */ >>>> + /* OCFG.SRPCap = 0, OCFG.HNPCap = 0 */ >>>> + reg = dwc3_readl(dwc->regs, DWC3_OCFG); >>>> + reg &= ~(DWC3_OCFG_SRPCAP | DWC3_OCFG_HNPCAP); >>> >>> are you sure *NO* DWC3 implementation out there is SRP capable and HNP >>> capable? >>> >>> HNP I understand that you want to disable because we're not support full >>> OTG, but SRP should be easy to support and it's also rather handy. In >>> any case, perhaps add a slightly longer comment explaining why you're >>> disabling these? >> >> This is done according to Fig 11.4 in the TRM. IMO it needs to be done >> even if the controller supports SRP and HNP. > > I see, fair enough. > >>>> + dwc3_writel(dwc->regs, DWC3_OCFG, reg); >>>> + /* OEVT = FFFF */ >>>> + dwc3_writel(dwc->regs, DWC3_OEVT, ~0); >>> >>> hmmm, flushing pending events. Are you sure you can even have them at >>> this point? This should be called after we reset the controller. >> >> This is again as per Fig 11.4 in TRM. > > documentation might have made assumptions which don't apply to us :-) > >>>> + /* OEVTEN = 0 */ >>>> + dwc3_otg_disable_events(dwc, DWC3_OTG_ALL_EVENTS); >>>> + /* OEVTEN.ConIDStsChngEn = 1. Instead we enable all events */ >>>> + dwc3_otg_enable_events(dwc, DWC3_OTG_ALL_EVENTS); >>> >>> oh, disable everything and enable everything right after. What gives? >> >> I did this following Fig 11.4. But there there don't enable all events, >> so it was a good idea to be on a clean slate by disabling all events first >> and then only enabling selected events. >> >> In any case I think it is good practice. i.e. clear before OR operation? >> FYI. dwc3_otg_enable_events doesn't clear the events that are not enabled so >> if some old event not part of enable_mask was enabled it will stay enabled. > > can't this result in IRQ triggering forever and us not handling it? ;-) Why should enabling events trigger IRQ? IRQ will trigger only when the event actually happens no? > >>>> + /* >>>> + * OCTL.PeriMode = 1, OCTL.DevSetHNPEn = 0, OCTL.HstSetHNPEn = 0, >>>> + * OCTL.HNPReq = 0 >>>> + */ >>>> + reg = dwc3_readl(dwc->regs, DWC3_OCTL); >>>> + reg |= DWC3_OCTL_PERIMODE; >>>> + reg &= ~(DWC3_OCTL_DEVSETHNPEN | DWC3_OCTL_HSTSETHNPEN | >>>> + DWC3_OCTL_HNPREQ); >>>> + dwc3_writel(dwc->regs, DWC3_OCTL, reg); >>>> +} >>>> + >>>> +/* dwc->lock must be held */ >>>> +static int dwc3_drd_start_host(struct dwc3 *dwc, int on, bool skip) >>>> +{ >>>> + u32 reg; >>>> + >>>> + /* switch OTG core */ >>>> + if (on) { >>>> + /* As per Figure 11-10 A-Device Flow Diagram */ >>>> + /* OCFG.HNPCap = 0, OCFG.SRPCap = 0 */ >>>> + reg = dwc3_readl(dwc->regs, DWC3_OCFG); >>>> + reg &= ~(DWC3_OCFG_SRPCAP | DWC3_OCFG_HNPCAP); >>> >>> didn't you do this already? Why do you need to do this again? >>> >> >> Was blindly following Fig 11-10. Might be necessary whenever we support HNP/SRP. >> I can get rid of it though if you prefer. > > please do, minimal support for now ;-) OK :). > >>>> + dwc3_writel(dwc->regs, DWC3_OCFG, reg); >>>> + >>>> + /* >>>> + * OCTL.PeriMode=0, OCTL.TermSelDLPulse = 0, >>>> + * OCTL.DevSetHNPEn = 0, OCTL.HstSetHNPEn = 0 >>>> + */ >>>> + reg = dwc3_readl(dwc->regs, DWC3_OCTL); >>>> + reg &= ~(DWC3_OCTL_PERIMODE | DWC3_OCTL_TERMSELIDPULSE | >>>> + DWC3_OCTL_DEVSETHNPEN | DWC3_OCTL_HSTSETHNPEN); >>> >>> HNP already disabled elsewhere. Why disable it again? >>> >> >> Strictly following TRM. nothing else. What do you want me to do here? > > assume your register writes actually stick :-) > >>>> + dwc3_writel(dwc->regs, DWC3_OCTL, reg); >>>> + >>>> + /* >>>> + * OCFG.DisPrtPwrCutoff = 0/1 >>>> + */ >>>> + reg = dwc3_readl(dwc->regs, DWC3_OCFG); >>>> + reg &= ~DWC3_OCFG_DISPWRCUTTOFF; >>> ^^ >>> one T enough? >>> >>>> + dwc3_writel(dwc->regs, DWC3_OCFG, reg); >>> >>> should you really always disable port power cutoff ? >> >> If I remember right this should be disabled for non OTG cases else >> core will turn off VBUS after A_WAIT_BCON timeout when port is >> disconnected. > > aha, good point :-) > >>>> + /* start the xHCI host driver */ >>>> + if (!skip) { >>> >>> when would skip be true? >>> >> >> only during system resume. > > hmmm, is there a reason for that? I mean, could we live without it for > the time being? Seems like all this achieves is avoiding reenumeration > of some devices during resume. Do we care from a starting > implementation? At least on AM43x, it was required. without that USB devices plugged in before a system suspend were lost after resume. I agree on dropping this for now and adding it later. > >>>> + } else { >>>> + /* >>>> + * Exit from A-device flow as per >>>> + * Figure 11-4 OTG Driver Overall Programming Flow >>>> + */ >>>> + /* stop the HCD */ >>>> + if (!skip) { >>>> + spin_unlock(&dwc->lock); >>>> + dwc3_host_exit(dwc); >>>> + spin_lock(&dwc->lock); >>>> + } >>>> + >>>> + /* >>>> + * OEVTEN.OTGADevBHostEndEvntEn=0, OEVTEN.OTGADevHNPChngEvntEn=0 >>>> + * OEVTEN.OTGADevSessEndDetEvntEn=0, >>>> + * OEVTEN.OTGADevHostEvntEn = 0 >>>> + * But we don't disable any OTG events >>> >>> why not? >> >> because we kept all of them enabled based on your suggestion last year >> (unlike what TRM says) > > Hmm, I see. I, clearly, forgot what I said. :-p Sorry np :) > >>>> + */ >>>> + >>>> + /* OCTL.HstSetHNPEn = 0, OCTL.PrtPwrCtl=0 */ >>>> + reg = dwc3_readl(dwc->regs, DWC3_OCTL); >>>> + reg &= ~(DWC3_OCTL_HSTSETHNPEN | DWC3_OCTL_PRTPWRCTL); >>> >>> disabled elsewhere. Why do it again? >> >> I can optimize it away if you prefer. > > we gotta start with an assumption that the HW works. If it doesn't, we > quirk it out. > >>>> + dwc3_writel(dwc->regs, DWC3_OCTL, reg); >>>> + >>>> + /* Initialize OTG registers */ >>>> + dwc3_otgregs_init(dwc); >>> >>> again you reinitialize everything. Why so many reinitializations? Seems >>> like you were having issues getting this to work and ended up with silly >>> reinitializations and workqueues in an effort to get it working. >> >> Felipe, last year you told me to strictly follow the TRM programming model. >> This is what it says to do. Please refer Fig 11.4 >> >> I know some things are silly but I deliberately didn't optimize them. >> If you want to now not strictly follow the TRM I'm fine with that as well. > > I see what you're doing now. > >>> This patch gives me the impression that the feature hasn't been tested >>> properly. :-s >> >> It is currently undergoing testing for TI release. So far there haven't been >> any surprises. > > good to know > >>>> + dwc3_writel(dwc->regs, DWC3_GCTL, reg); >>>> + >>>> + /* start the Peripheral driver */ >>>> + if (dwc->gadget_driver) { >>>> + __dwc3_gadget_start(dwc); >>>> + if (dwc->gadget_pullup) >>>> + dwc3_gadget_run_stop(dwc, true, false); >>> >>> why don't you add/remove the UDC just like you do for the HCD? (you >>> wouldn't add/remove a device, but rather call >>> usb_del_gadget_udc()/usb_add_gadget_udc() directly. Would that clean up >>> some of this? >> >> It causes more problems than solving anything. >> e.g. An already loaded gadget driver will have to be manually removed and re-loaded to >> work after a peripheral to host to peripheral mode switch. > > is that really still true? When we remove the UDC, the currently-loaded > gadget driver will be moved to the pending list. Once a UDC is added > back, udc-core will bind it again to the UDC. > OK. I need to test this again. As you say, the issue might already have been fixed. Good to know that. >>>> + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); >>>> + >>>> + /* Initialize OTG registers */ >>>> + dwc3_otgregs_init(dwc); >>>> + >>>> + /* force drd state machine update the first time */ >>>> + dwc->otg_fsm.b_sess_vld = -1; >>>> + dwc->otg_fsm.id = -1; >>> >>> Does this work if you boot with cable already attached? Both host and >>> peripheral cables? >> >> Yes. > > fair enough > >>>> +static int dwc3_drd_init(struct dwc3 *dwc) >>>> +{ >>>> + int ret, irq; >>>> + unsigned long flags; >>>> + >>>> + INIT_WORK(&dwc->otg_work, dwc3_drd_work); >>>> + >>>> + irq = dwc3_otg_get_irq(dwc); >>>> + if (irq < 0) >>>> + return irq; >>>> + >>>> + dwc->otg_irq = irq; >>>> + >>>> + /* disable all otg irqs */ >>>> + dwc3_otg_disable_events(dwc, DWC3_OTG_ALL_EVENTS); >>>> + /* clear all events */ >>>> + dwc3_writel(dwc->regs, DWC3_OEVT, ~0); >>> >>> >>> this is really odd. You have a bunch of these duplicated chunks of code >>> all over the place... >>> >>>> + irq_set_status_flags(dwc->otg_irq, IRQ_NOAUTOEN); >>> >>> why? >> >> I don't know how to fix this. I have to do this because dwc3_omap is doing it >> on the shared IRQ line and the flags must match if we need to share it. > > hmmm... Then why does dwc_omap IRQ have IRQ_NOAUTOEN and otg_irq doesn't? We're setting IRQ_NOAUTOEN for otg_irq above. But the problem is that other platforms might not have this set so it will break there. Need to think of a better way how to tackle this. > >>>> + ret = request_threaded_irq(dwc->otg_irq, dwc3_otg_irq, >>>> + dwc3_otg_thread_irq, >>>> + IRQF_SHARED, "dwc3-otg", dwc); >>>> + if (ret) { >>>> + dev_err(dwc->dev, "failed to request irq #%d --> %d\n", >>>> + dwc->otg_irq, ret); >>>> + ret = -ENODEV; >>>> + return ret; >>>> + } >>>> + >>>> + ret = dwc3_gadget_init(dwc); >>> >>> unconditionally? What if I booted with a micro-A plugged to my port and >>> a USB-stick attached to it? >> >> We are not starting the gadget controller though and we want UDC to be initialized >> anyways so users can load a gadget driver before hand. > > Users will be able to load gadget driver and that will be kept in the > pending list until a UDC is loaded. cool. > >> This is another point against using usb_del_gadget_udc(). The gadget controller >> is really there and user wants to have a persistant gadget driver loaded >> between host/peripheral mode switches. > > see above. > >>>> @@ -1827,7 +1835,7 @@ static int dwc3_gadget_start(struct usb_gadget *g, >>>> return ret; >>>> } >>>> >>>> -static void __dwc3_gadget_stop(struct dwc3 *dwc) >>>> +void __dwc3_gadget_stop(struct dwc3 *dwc) >>> >>> shouldn't have to. Just rely on usb_add/del_gadget_udc() >>> >> Please let's not use usb_add/del_gadget_udc(). It causes more trouble >> for user :) > > I can't see why it would :-s > >> gadget_start/stop has been working beautifully with the benefit of >> user being able to load gadget driver at any time (even when booted >> with host mode) and not worrying about re-loading it between >> host/peripheral role swithces. > > If that's still necessary, we have a bug in udc-core. That needs to be > fixed :-) > Understood. Thanks :) regards, -roger