Re: [PATCH v2 3/4] usb: dwc3: add dual-role support

From: Roger Quadros
Date: Thu Mar 30 2017 - 02:40:20 EST

Next message: James Morris: "Re: [RESEND][PATCH] TOMOYO: Use designated initializers"
Previous message: Amir Goldstein: "Re: [REGRESSION 4.11] Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE") breaks ubifs"
In reply to: Felipe Balbi: "Re: [PATCH v2 3/4] usb: dwc3: add dual-role support"
Next in thread: Felipe Balbi: "Re: [PATCH v2 3/4] usb: dwc3: add dual-role support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

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

Next message: James Morris: "Re: [RESEND][PATCH] TOMOYO: Use designated initializers"
Previous message: Amir Goldstein: "Re: [REGRESSION 4.11] Commit d8514d8edb5b ("ovl: copy up regular file using O_TMPFILE") breaks ubifs"
In reply to: Felipe Balbi: "Re: [PATCH v2 3/4] usb: dwc3: add dual-role support"
Next in thread: Felipe Balbi: "Re: [PATCH v2 3/4] usb: dwc3: add dual-role support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]