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

[Felipe Balbi]

Hi,

Roger Quadros <rogerq@xxxxxx> writes:
> If dr_mode is "otg" then support dual role mode of operation.
>
> Get ID and VBUS information from the OTG controller
> and put the controller in the appropriate state.
>
> This is our dual-role state table.
>
> ID	VBUS	dual-role state
> --	----	---------------
> 0	x	A_HOST - Host controller active
> 1	0	B_IDLE - Both Host and Gadget controllers inactive
> 1	1	B_PERIPHERAL - Gadget controller active
>
> Calling dwc3_otg_fsm_sync() directly from dwc3_complete() can
> lock up the system at xHCI add/remove so we use a work queue for it.
>
> Signed-off-by: Roger Quadros <rogerq@xxxxxx>

it still seems to me that you're adding too much code for something that
should be darn simple. Please, read on.

> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 369bab1..619fa7c 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -27,6 +27,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/interrupt.h>
> +#include <linux/irq.h>

as a cosmetic change, it would be nicer to have a drd.c or otg.c which
exposes dwc3_otg_start()/stop() like we do for gadget.c and host.c

> @@ -107,6 +108,7 @@ void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
>  	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
>  	reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
>  	reg |= DWC3_GCTL_PRTCAPDIR(mode);
> +	dwc->current_mode = mode;
>  	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
>  }
>  
> @@ -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.

For something that simple, we wouldn't even need to use OTG FSM layer
because that brings no benefit for such a simple requirement.

Once there's a *real* need for OTG FSM, then we can add support for it,
but then we add support to something we know to be working.

> +/* 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.

> +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?

> +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.

> +static void dwc3_otg_enable_events(struct dwc3 *dwc, u32 enable_mask)
> +{
> +	dwc->oevten |= (enable_mask);
> +	dwc3_writel(dwc->regs, DWC3_OEVTEN, dwc->oevten);
> +}
> +
> +#define DWC3_OTG_ALL_EVENTS	(DWC3_OEVTEN_XHCIRUNSTPSETEN | \
> +		DWC3_OEVTEN_DEVRUNSTPSETEN | DWC3_OEVTEN_HIBENTRYEN | \
> +		DWC3_OEVTEN_CONIDSTSCHNGEN | DWC3_OEVTEN_HRRCONFNOTIFEN | \
> +		DWC3_OEVTEN_HRRINITNOTIFEN | DWC3_OEVTEN_ADEVIDLEEN | \
> +		DWC3_OEVTEN_ADEVBHOSTENDEN | DWC3_OEVTEN_ADEVHOSTEN | \
> +		DWC3_OEVTEN_ADEVHNPCHNGEN | DWC3_OEVTEN_ADEVSRPDETEN | \
> +		DWC3_OEVTEN_ADEVSESSENDDETEN | DWC3_OEVTEN_BDEVHOSTENDEN | \
> +		DWC3_OEVTEN_BDEVHNPCHNGEN | DWC3_OEVTEN_BDEVSESSVLDDETEN | \
> +		DWC3_OEVTEN_BDEVVBUSCHNGE)
> +
> +static irqreturn_t dwc3_otg_thread_irq(int irq, void *_dwc)
> +{
> +	struct dwc3 *dwc = _dwc;
> +
> +	spin_lock(&dwc->lock);
> +	if (dwc->otg_needs_host_start) {
> +		dwc3_drd_start_host(dwc, true, 1);
> +		dwc->otg_needs_host_start = 0;
> +	}
> +
> +	dwc3_otg_fsm_sync(dwc);

enable_events();

> +	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();

> +	}

There's one step missing here. Where do you mask OTG interrupts?

> +
> +	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.

> +	/*
> +	 * 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?

> +	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.

> +	/* 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?

> +	/*
> +	 * 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?

> +		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?

> +		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 ?

> +		/* start the xHCI host driver */
> +		if (!skip) {

when would skip be true?

> +			spin_unlock(&dwc->lock);
> +			dwc3_host_init(dwc);
> +			spin_lock(&dwc->lock);
> +		}
> +
> +		/*
> +		 * OCFG.SRPCap = 1, OCFG.HNPCap = GHWPARAMS6.HNP_CAP
> +		 * We don't want SRP/HNP for simple dual-role so leave
> +		 * these disabled.
> +		 */
> +
> +		/*
> +		 * OEVTEN.OTGADevHostEvntEn = 1
> +		 * OEVTEN.OTGADevSessEndDetEvntEn = 1
> +		 * We don't want HNP/role-swap so leave these disabled.
> +		 */
> +
> +		/* GUSB2PHYCFG.ULPIAutoRes = 1/0, GUSB2PHYCFG.SusPHY = 1 */
> +		if (!dwc->dis_u2_susphy_quirk) {
> +			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> +			reg |= DWC3_GUSB2PHYCFG_SUSPHY;
> +			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);

already done elsewhere. Why do you need to do it again?

> +		}
> +
> +		/* Set Port Power to enable VBUS: OCTL.PrtPwrCtl = 1 */
> +		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
> +		reg |= DWC3_OCTL_PRTPWRCTL;
> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);

I wonder if you have a race here with xHCI. You're essentially enable
port power after xHCI has been loaded already. Mathias, do you see any
problems with this? Could we confuse xHCI with this? I'm assuming there
are no issues since without VBUS xHCI wouldn't see any port status
change events, but thought I'd ask anyway.

> +	} 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?

> +		 */
> +
> +		/* 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?

> +		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.

This patch gives me the impression that the feature hasn't been tested
properly. :-s

> +/* dwc->lock must be held */
> +static int dwc3_drd_start_gadget(struct dwc3 *dwc, int on)
> +{
> +	u32 reg;
> +
> +	if (on)
> +		dwc3_event_buffers_setup(dwc);
> +
> +	if (on) {

if (on)
	[..]

if (on)
	[..]

:-s

> +		/*
> +		 * OCFG.HNPCap = GHWPARAMS6.HNP_CAP, OCFG.SRPCap = 1
> +		 * but we set them to 0 for simple dual-role operation.
> +		 */
> +		reg = dwc3_readl(dwc->regs, DWC3_OCFG);
> +		reg &= ~(DWC3_OCFG_SRPCAP | DWC3_OCFG_HNPCAP);

and again clearing bits that have already been cleared multiple times.

> +		/* OCFG.OTGSftRstMsk = 0/1 */
> +		reg |= DWC3_OCFG_SFTRSTMASK;

do you need to set this again?

> +		dwc3_writel(dwc->regs, DWC3_OCFG, reg);
> +		/*
> +		 * OCTL.PeriMode = 1
> +		 * OCTL.TermSelDLPulse = 0/1, OCTL.HNPReq = 0
> +		 * OCTL.DevSetHNPEn = 0, OCTL.HstSetHNPEn = 0
> +		 */
> +		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
> +		reg |= DWC3_OCTL_PERIMODE;
> +		reg &= ~(DWC3_OCTL_TERMSELIDPULSE | DWC3_OCTL_HNPREQ |
> +			 DWC3_OCTL_DEVSETHNPEN | DWC3_OCTL_HSTSETHNPEN);

clearing bits that were already cleared.

> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
> +		/* OEVTEN.OTGBDevSesVldDetEvntEn = 1 */
> +		dwc3_otg_enable_events(dwc, DWC3_OEVT_BDEVSESSVLDDET);
> +		/* GUSB2PHYCFG.ULPIAutoRes = 0, GUSB2PHYCFG0.SusPHY = 1 */
> +		if (!dwc->dis_u2_susphy_quirk) {
> +			reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> +			reg |= DWC3_GUSB2PHYCFG_SUSPHY;
> +			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> +		}
> +		/* GCTL.GblHibernationEn = 0 */
> +		reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> +		reg &= ~DWC3_GCTL_GBLHIBERNATIONEN;

possibly breaking other users.

> +		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?

> +		}
> +	} else {
> +		/*
> +		 * Exit from B-device flow as per
> +		 * Figure 11-4 OTG Driver Overall Programming Flow
> +		 */
> +		/* stop the Peripheral driver */
> +		if (dwc->gadget_driver) {
> +			if (dwc->gadget_pullup)
> +				dwc3_gadget_run_stop(dwc, false, false);
> +			spin_unlock(&dwc->lock);
> +			if (dwc->gadget_driver->disconnect)
> +				dwc->gadget_driver->disconnect(&dwc->gadget);
> +			spin_lock(&dwc->lock);
> +			__dwc3_gadget_stop(dwc);

usb_del_gadget_udc()

> +		}
> +
> +		/*
> +		 * OEVTEN.OTGBDevHNPChngEvntEn = 0
> +		 * OEVTEN.OTGBDevVBusChngEvntEn = 0
> +		 * OEVTEN.OTGBDevBHostEndEvntEn = 0
> +		 */
> +		reg = dwc3_readl(dwc->regs, DWC3_OEVTEN);
> +		reg &= ~(DWC3_OEVT_BDEVHNPCHNG | DWC3_OEVT_BDEVVBUSCHNG |
> +			 DWC3_OEVT_BDEVBHOSTEND);
> +		dwc3_writel(dwc->regs, DWC3_OEVTEN, reg);
> +
> +		/* OCTL.DevSetHNPEn = 0, OCTL.HNPReq = 0, OCTL.PeriMode=1 */
> +		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
> +		reg &= ~(DWC3_OCTL_DEVSETHNPEN | DWC3_OCTL_HNPREQ);

:-)

> +		reg |= DWC3_OCTL_PERIMODE;
> +		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
> +
> +		/* Initialize OTG registers */
> +		dwc3_otgregs_init(dwc);

:-)

> +static int dwc3_otg_get_irq(struct dwc3 *dwc)
> +{
> +	struct platform_device *dwc3_pdev = to_platform_device(dwc->dev);
> +	int irq;
> +
> +	irq = platform_get_irq_byname(dwc3_pdev, "otg");
> +	if (irq > 0)
> +		goto out;
> +
> +	if (irq == -EPROBE_DEFER)
> +		goto out;
> +
> +	irq = platform_get_irq_byname(dwc3_pdev, "dwc_usb3");
> +	if (irq > 0)
> +		goto out;
> +
> +	if (irq == -EPROBE_DEFER)
> +		goto out;
> +
> +	irq = platform_get_irq(dwc3_pdev, 0);
> +	if (irq > 0)
> +		goto out;
> +
> +	if (irq != -EPROBE_DEFER)
> +		dev_err(dwc->dev, "missing otg IRQ\n");
> +
> +	if (!irq)
> +		irq = -EINVAL;
> +
> +out:
> +	return irq;
> +}
> +
> +/* dwc->lock must be held */
> +static void dwc3_otg_core_init(struct dwc3 *dwc)
> +{
> +	u32 reg;
> +
> +	/*
> +	 * As per Figure 11-4 OTG Driver Overall Programming Flow,
> +	 * block "Initialize GCTL for OTG operation".
> +	 */
> +	/* GCTL.PrtCapDir=2'b11 */
> +	dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
> +	/* GUSB2PHYCFG0.SusPHY=0 */
> +	reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> +	reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;

why here? We only need this if the quirk flag is set. If that flag has
been set, this bit should have been cleared already.

> +	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?

> +}
> +
> +/* dwc->lock must be held */
> +static void dwc3_otg_core_exit(struct dwc3 *dwc)
> +{
> +	/* disable all otg irqs */
> +	dwc3_otg_disable_events(dwc, DWC3_OTG_ALL_EVENTS);
> +	/* clear all events */
> +	dwc3_writel(dwc->regs, DWC3_OEVT, ~0);

when free_irq() is called, it will wait for the current interrupt to
finish being processed, this means OEVT will already be zero but the
time free_irq() finishes running. I think you shouldn't care about this.

Just mask all events and that should be 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?

> +	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?

I think this should be conditional on extcon cable state.

> +	if (ret) {
> +		free_irq(dwc->otg_irq, dwc);
> +		return ret;
> +	}
> +
> +	spin_lock_irqsave(&dwc->lock, flags);
> +	dwc3_otg_core_init(dwc);
> +	spin_unlock_irqrestore(&dwc->lock, flags);
> +	queue_work(system_power_efficient_wq, &dwc->otg_work);

:-s

> +	return 0;
> +}
> +
> +static void dwc3_drd_exit(struct dwc3 *dwc)
> +{
> +	unsigned long flags;
> +
> +	cancel_work_sync(&dwc->otg_work);
> +	spin_lock_irqsave(&dwc->lock, flags);
> +	dwc3_otg_core_exit(dwc);
> +	if (dwc->otg_fsm.protocol == PROTO_HOST)
> +		dwc3_drd_start_host(dwc, 0, 0);
> +	dwc->otg_fsm.protocol = PROTO_UNDEF;
> +	free_irq(dwc->otg_irq, dwc);

free_irq() might sleep, no?

/**
 *	free_irq - free an interrupt allocated with request_irq
 *	@irq: Interrupt line to free
 *	@dev_id: Device identity to free
 *
 *	Remove an interrupt handler. The handler is removed and if the
 *	interrupt line is no longer in use by any driver it is disabled.
 *	On a shared IRQ the caller must ensure the interrupt is disabled
 *	on the card it drives before calling this function. The function
 *	does not return until any executing interrupts for this IRQ
 *	have completed.
 *
 *	This function must not be called from interrupt context.
 */
void free_irq(unsigned int irq, void *dev_id)

> @@ -1207,19 +1700,35 @@ static int dwc3_suspend_common(struct dwc3 *dwc)
>  {
>  	unsigned long	flags;
>  
> +	spin_lock_irqsave(&dwc->lock, flags);

looks like it should be in a separate patch.

> @@ -1679,7 +1684,7 @@ static void dwc3_gadget_setup_nump(struct dwc3 *dwc)
>  	dwc3_writel(dwc->regs, DWC3_DCFG, reg);
>  }
>  
> -static int __dwc3_gadget_start(struct dwc3 *dwc)
> +int __dwc3_gadget_start(struct dwc3 *dwc)
>  {
>  	struct dwc3_ep		*dep;
>  	int			ret = 0;
> @@ -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()

-- 
balbi

Attachment: signature.asc
Description: PGP signature