Message-ID: <46440b24-a2cd-93d6-e92e-2d80482f7d32@ti.com>
Date:   Wed, 29 Mar 2017 14:33:43 +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 28/03/17 14:07, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@...com> 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@...com>
> 
> it still seems to me that you're adding too much code for something that
> should be darn simple. Please, read on.

Sure, I'm all ears for simplification :).

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

Agreed.
> 
>> @@ -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.

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.

> 
> 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 :).

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

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

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

We need a way to mask the OTG events without loosing them while they are masked.
Do you know how that could be achieved?

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

We can't disable events if we don't want to miss any events while they were disabled.
But I agree that we need to prevent them from firing another hard IRQ.
I couldn't figure out how that can be done.
> 
>> +	}
> 
> There's one step missing here. Where do you mask OTG interrupts?

What is the way to mask OTG interrupts without loosing them.

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

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

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

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

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

>> +		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.
> 
>> +		/* start the xHCI host driver */
>> +		if (!skip) {
> 
> when would skip be true?
> 

only during system resume.

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

Indeed. I'll get rid if this.
> 
>> +		}
>> +
>> +		/* 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.

We haven't seen any problems with XHCI with our testing yet.
> 
>> +	} 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)
> 
>> +		 */
>> +
>> +		/* 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.

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

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

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

:P

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

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.

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

Right. Will get rid of this.

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

OK.

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

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

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.

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

alright.
> 
> /**
>  *	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()
> 
Please let's not use usb_add/del_gadget_udc(). It causes more trouble for user :)
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.

cheers,
-roger

Powered by blists - more mailing lists