Date:   Thu, 30 Mar 2017 09:40:07 +0300
From:   Roger Quadros <rogerq@...com>
To:     Felipe Balbi <felipe.balbi@...ux.intel.com>,
        Mathias Nyman <mathias.nyman@...ux.intel.com>
CC:     <vivek.gautam@...eaurora.org>, <linux-usb@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/4] usb: dwc3: add dual-role support

Hi,

On 29/03/17 16:15, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@...com> 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

Powered by blists - more mailing lists