[PATCH v2 0/4] usb: dwc3: dual-role support

[PATCH v2 0/4] usb: dwc3: dual-role support

[Roger Quadros]

Hi,

We rely on the OTG controller block or Extcon to provide us with
VBUS and ID line status via an interrupt.

This is then used to switch the controller between host, peripheral
and idle roles based on the following 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

Series depends on
[1] dwc3 trivial fixes - https://lkml.org/lkml/2017/2/15/204

Changelog:
v2:
- Use extcon device for VBUS/ID events if extcon phandle is present.
- Fix bugs during system suspend/resume.

cheers,
-roger

Roger Quadros (4):
  usb: dwc3: core.h: add some register definitions
  usb: dwc3: omap: don't miss events during suspend/resume
  usb: dwc3: add dual-role support
  usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

 Documentation/devicetree/bindings/usb/dwc3.txt |   2 +
 drivers/usb/dwc3/core.c                        | 752 ++++++++++++++++++++++++-
 drivers/usb/dwc3/core.h                        | 131 ++++-
 drivers/usb/dwc3/dwc3-omap.c                   |  16 +
 drivers/usb/dwc3/gadget.c                      |  18 +-
 5 files changed, 895 insertions(+), 24 deletions(-)

-- 
2.7.4

[PATCH v2 1/4] usb: dwc3: core.h: add some register definitions

[Roger Quadros]

Add OTG and GHWPARAMS6 register definitions

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/core.h | 82 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 82 insertions(+)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index d514dca..fc82d2e 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -204,6 +204,15 @@
 /* Global User Control 1 Register */
 #define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW	BIT(24)
 
+/* Global Status Register */
+#define DWC3_GSTS_OTG_IP	BIT(10)
+#define DWC3_GSTS_BC_IP		BIT(9)
+#define DWC3_GSTS_ADP_IP	BIT(8)
+#define DWC3_GSTS_HOST_IP	BIT(7)
+#define DWC3_GSTS_DEVICE_IP	BIT(6)
+#define DWC3_GSTS_CSR_TIMEOUT	BIT(5)
+#define DWC3_GSTS_BUS_ERR_ADDR_VLD	BIT(4)
+
 /* Global USB2 PHY Configuration Register */
 #define DWC3_GUSB2PHYCFG_PHYSOFTRST	BIT(31)
 #define DWC3_GUSB2PHYCFG_U2_FREECLK_EXISTS	BIT(30)
@@ -288,6 +297,11 @@
 #define DWC3_MAX_HIBER_SCRATCHBUFS		15
 
 /* Global HWPARAMS6 Register */
+#define DWC3_GHWPARAMS6_BCSUPPORT		BIT(14)
+#define DWC3_GHWPARAMS6_OTG3SUPPORT		BIT(13)
+#define DWC3_GHWPARAMS6_ADPSUPPORT		BIT(12)
+#define DWC3_GHWPARAMS6_HNPSUPPORT		BIT(11)
+#define DWC3_GHWPARAMS6_SRPSUPPORT		BIT(10)
 #define DWC3_GHWPARAMS6_EN_FPGA			BIT(7)
 
 /* Global HWPARAMS7 Register */
@@ -469,6 +483,74 @@
 #define DWC3_DEV_IMOD_INTERVAL_SHIFT	0
 #define DWC3_DEV_IMOD_INTERVAL_MASK	(0xffff << 0)
 
+/* OTG Configuration Register */
+#define DWC3_OCFG_DISPWRCUTTOFF		BIT(5)
+#define DWC3_OCFG_HIBDISMASK		BIT(4)
+#define DWC3_OCFG_SFTRSTMASK		BIT(3)
+#define DWC3_OCFG_OTGVERSION		BIT(2)
+#define DWC3_OCFG_HNPCAP		BIT(1)
+#define DWC3_OCFG_SRPCAP		BIT(0)
+
+/* OTG CTL Register */
+#define DWC3_OCTL_OTG3GOERR		BIT(7)
+#define DWC3_OCTL_PERIMODE		BIT(6)
+#define DWC3_OCTL_PRTPWRCTL		BIT(5)
+#define DWC3_OCTL_HNPREQ		BIT(4)
+#define DWC3_OCTL_SESREQ		BIT(3)
+#define DWC3_OCTL_TERMSELIDPULSE	BIT(2)
+#define DWC3_OCTL_DEVSETHNPEN		BIT(1)
+#define DWC3_OCTL_HSTSETHNPEN		BIT(0)
+
+/* OTG Event Register */
+#define DWC3_OEVT_DEVICEMODE		BIT(31)
+#define DWC3_OEVT_XHCIRUNSTPSET		BIT(27)
+#define DWC3_OEVT_DEVRUNSTPSET		BIT(26)
+#define DWC3_OEVT_HIBENTRY		BIT(25)
+#define DWC3_OEVT_CONIDSTSCHNG		BIT(24)
+#define DWC3_OEVT_HRRCONFNOTIF		BIT(23)
+#define DWC3_OEVT_HRRINITNOTIF		BIT(22)
+#define DWC3_OEVT_ADEVIDLE		BIT(21)
+#define DWC3_OEVT_ADEVBHOSTEND		BIT(20)
+#define DWC3_OEVT_ADEVHOST		BIT(19)
+#define DWC3_OEVT_ADEVHNPCHNG		BIT(18)
+#define DWC3_OEVT_ADEVSRPDET		BIT(17)
+#define DWC3_OEVT_ADEVSESSENDDET	BIT(16)
+#define DWC3_OEVT_BDEVBHOSTEND		BIT(11)
+#define DWC3_OEVT_BDEVHNPCHNG		BIT(10)
+#define DWC3_OEVT_BDEVSESSVLDDET	BIT(9)
+#define DWC3_OEVT_BDEVVBUSCHNG		BIT(8)
+#define DWC3_OEVT_BSESSVLD		BIT(3)
+#define DWC3_OEVT_HSTNEGSTS		BIT(2)
+#define DWC3_OEVT_SESREQSTS		BIT(1)
+#define DWC3_OEVT_ERROR			BIT(0)
+
+/* OTG Event Enable Register */
+#define DWC3_OEVTEN_XHCIRUNSTPSETEN	BIT(27)
+#define DWC3_OEVTEN_DEVRUNSTPSETEN	BIT(26)
+#define DWC3_OEVTEN_HIBENTRYEN		BIT(25)
+#define DWC3_OEVTEN_CONIDSTSCHNGEN	BIT(24)
+#define DWC3_OEVTEN_HRRCONFNOTIFEN	BIT(23)
+#define DWC3_OEVTEN_HRRINITNOTIFEN	BIT(22)
+#define DWC3_OEVTEN_ADEVIDLEEN		BIT(21)
+#define DWC3_OEVTEN_ADEVBHOSTENDEN	BIT(20)
+#define DWC3_OEVTEN_ADEVHOSTEN		BIT(19)
+#define DWC3_OEVTEN_ADEVHNPCHNGEN	BIT(18)
+#define DWC3_OEVTEN_ADEVSRPDETEN	BIT(17)
+#define DWC3_OEVTEN_ADEVSESSENDDETEN	BIT(16)
+#define DWC3_OEVTEN_BDEVHOSTENDEN	BIT(11)
+#define DWC3_OEVTEN_BDEVHNPCHNGEN	BIT(10)
+#define DWC3_OEVTEN_BDEVSESSVLDDETEN	BIT(9)
+#define DWC3_OEVTEN_BDEVVBUSCHNGE	BIT(8)
+
+/* OTG Status Register */
+#define DWC3_OSTS_DEVRUNSTP		BIT(13)
+#define DWC3_OSTS_XHCIRUNSTP		BIT(12)
+#define DWC3_OSTS_PERIPHERALSTATE	BIT(4)
+#define DWC3_OSTS_XHCIPRTPOWER		BIT(3)
+#define DWC3_OSTS_BSESVLD		BIT(2)
+#define DWC3_OSTS_VBUSVLD		BIT(1)
+#define DWC3_OSTS_CONIDSTS		BIT(0)
+
 /* Structures */
 
 struct dwc3_trb;
-- 
2.7.4

[PATCH v2 2/4] usb: dwc3: omap: don't miss events during suspend/resume

[Roger Quadros]

The USB cable state can change during suspend/resume
so be sure to check and update the extcon state.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/dwc3-omap.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
index fe4efb8..a7de60e 100644
--- a/drivers/usb/dwc3/dwc3-omap.c
+++ b/drivers/usb/dwc3/dwc3-omap.c
@@ -598,9 +598,25 @@ static int dwc3_omap_resume(struct device *dev)
 	return 0;
 }
 
+static void dwc3_omap_complete(struct device *dev)
+{
+	struct dwc3_omap	*omap = dev_get_drvdata(dev);
+
+	if (extcon_get_state(omap->edev, EXTCON_USB))
+		dwc3_omap_set_mailbox(omap, OMAP_DWC3_VBUS_VALID);
+	else
+		dwc3_omap_set_mailbox(omap, OMAP_DWC3_VBUS_OFF);
+
+	if (extcon_get_state(omap->edev, EXTCON_USB_HOST))
+		dwc3_omap_set_mailbox(omap, OMAP_DWC3_ID_GROUND);
+	else
+		dwc3_omap_set_mailbox(omap, OMAP_DWC3_ID_FLOAT);
+}
+
 static const struct dev_pm_ops dwc3_omap_dev_pm_ops = {
 
 	SET_SYSTEM_SLEEP_PM_OPS(dwc3_omap_suspend, dwc3_omap_resume)
+	.complete = dwc3_omap_complete,
 };
 
 #define DEV_PM_OPS	(&dwc3_omap_dev_pm_ops)
-- 
2.7.4

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

[Roger Quadros]

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@ti.com>
---
 drivers/usb/dwc3/core.c   | 595 ++++++++++++++++++++++++++++++++++++++++++++--
 drivers/usb/dwc3/core.h   |  44 +++-
 drivers/usb/dwc3/gadget.c |  18 +-
 3 files changed, 633 insertions(+), 24 deletions(-)

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>
 #include <linux/ioport.h>
 #include <linux/io.h>
 #include <linux/list.h>
@@ -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;
+}
+
+/* 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);
+}
+
+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);
+}
+
+static void dwc3_otg_disable_events(struct dwc3 *dwc, u32 disable_mask)
+{
+	dwc->oevten &= ~(disable_mask);
+	dwc3_writel(dwc->regs, DWC3_OEVTEN, dwc->oevten);
+}
+
+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);
+	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;
+	}
+
+	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);
+
+	/*
+	 * 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);
+	dwc3_writel(dwc->regs, DWC3_OCFG, reg);
+	/* OEVT = FFFF */
+	dwc3_writel(dwc->regs, DWC3_OEVT, ~0);
+	/* 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);
+	/*
+	 * 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);
+		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);
+		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
+
+		/*
+		 * OCFG.DisPrtPwrCutoff = 0/1
+		 */
+		reg = dwc3_readl(dwc->regs, DWC3_OCFG);
+		reg &= ~DWC3_OCFG_DISPWRCUTTOFF;
+		dwc3_writel(dwc->regs, DWC3_OCFG, reg);
+
+		/* start the xHCI host driver */
+		if (!skip) {
+			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);
+		}
+
+		/* 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);
+	} 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
+		 */
+
+		/* OCTL.HstSetHNPEn = 0, OCTL.PrtPwrCtl=0 */
+		reg = dwc3_readl(dwc->regs, DWC3_OCTL);
+		reg &= ~(DWC3_OCTL_HSTSETHNPEN | DWC3_OCTL_PRTPWRCTL);
+		dwc3_writel(dwc->regs, DWC3_OCTL, reg);
+
+		/* Initialize OTG registers */
+		dwc3_otgregs_init(dwc);
+	}
+
+	return 0;
+}
+
+/* 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) {
+		/* As per Figure 11-20 B-Device Flow Diagram */
+
+		/*
+		 * 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);
+		/* OCFG.OTGSftRstMsk = 0/1 */
+		reg |= DWC3_OCFG_SFTRSTMASK;
+		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);
+		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;
+		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);
+		}
+	} 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);
+		}
+
+		/*
+		 * 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);
+	}
+
+	return 0;
+}
+
+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;
+	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;
+}
+
+/* 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);
+}
+
+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);
+
+	irq_set_status_flags(dwc->otg_irq, IRQ_NOAUTOEN);
+	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);
+	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);
+
+	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);
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	dwc3_gadget_exit(dwc);
+}
+
+/* -------------------------------------------------------------------------- */
+
 static int dwc3_core_init_mode(struct dwc3 *dwc)
 {
 	struct device *dev = dwc->dev;
@@ -862,17 +1363,10 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 		}
 		break;
 	case USB_DR_MODE_OTG:
-		ret = dwc3_host_init(dwc);
-		if (ret) {
-			if (ret != -EPROBE_DEFER)
-				dev_err(dev, "failed to initialize host\n");
-			return ret;
-		}
-
-		ret = dwc3_gadget_init(dwc);
+		ret = dwc3_drd_init(dwc);
 		if (ret) {
 			if (ret != -EPROBE_DEFER)
-				dev_err(dev, "failed to initialize gadget\n");
+				dev_err(dev, "failed to initialize dual-role\n");
 			return ret;
 		}
 		break;
@@ -894,8 +1388,7 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc)
 		dwc3_host_exit(dwc);
 		break;
 	case USB_DR_MODE_OTG:
-		dwc3_host_exit(dwc);
-		dwc3_gadget_exit(dwc);
+		dwc3_drd_exit(dwc);
 		break;
 	default:
 		/* do nothing */
@@ -1207,19 +1700,35 @@ static int dwc3_suspend_common(struct dwc3 *dwc)
 {
 	unsigned long	flags;
 
+	spin_lock_irqsave(&dwc->lock, flags);
+
 	switch (dwc->dr_mode) {
 	case USB_DR_MODE_PERIPHERAL:
-	case USB_DR_MODE_OTG:
-		spin_lock_irqsave(&dwc->lock, flags);
 		dwc3_gadget_suspend(dwc);
-		spin_unlock_irqrestore(&dwc->lock, flags);
+		break;
+	case USB_DR_MODE_OTG:
+		switch (dwc->otg_fsm.protocol) {
+		case PROTO_GADGET:
+			dwc3_gadget_suspend(dwc);
+			break;
+		case PROTO_HOST:
+		case PROTO_UNDEF:
+		default:
+			/* nothing */
+			break;
+		}
 		break;
 	case USB_DR_MODE_HOST:
+	case USB_DR_MODE_UNKNOWN:
 	default:
 		/* do nothing */
 		break;
 	}
 
+	if (dwc->dr_mode == USB_DR_MODE_OTG)
+		dwc3_otg_core_exit(dwc);
+
+	spin_unlock_irqrestore(&dwc->lock, flags);
 	dwc3_core_exit(dwc);
 
 	return 0;
@@ -1234,19 +1743,40 @@ static int dwc3_resume_common(struct dwc3 *dwc)
 	if (ret)
 		return ret;
 
+	spin_lock_irqsave(&dwc->lock, flags);
+
 	switch (dwc->dr_mode) {
 	case USB_DR_MODE_PERIPHERAL:
-	case USB_DR_MODE_OTG:
-		spin_lock_irqsave(&dwc->lock, flags);
 		dwc3_gadget_resume(dwc);
-		spin_unlock_irqrestore(&dwc->lock, flags);
-		/* FALLTHROUGH */
+		break;
+	case USB_DR_MODE_OTG:
+		switch (dwc->otg_fsm.protocol) {
+		case PROTO_GADGET:
+			dwc3_gadget_resume(dwc);
+			break;
+		case PROTO_HOST:
+			break;
+		case PROTO_UNDEF:
+		default:
+			/* nothing */
+			break;
+		}
+		break;
 	case USB_DR_MODE_HOST:
+	case USB_DR_MODE_UNKNOWN:
 	default:
 		/* do nothing */
 		break;
 	}
 
+	if (dwc->dr_mode == USB_DR_MODE_OTG) {
+		dwc3_otg_core_init(dwc);
+		if (dwc->otg_fsm.protocol == PROTO_HOST)
+			dwc3_drd_start_host(dwc, true, 1);
+	}
+
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
 	return 0;
 }
 
@@ -1301,6 +1831,7 @@ static int dwc3_runtime_resume(struct device *dev)
 		dwc3_gadget_process_pending_events(dwc);
 		break;
 	case USB_DR_MODE_HOST:
+	case USB_DR_MODE_UNKNOWN:
 	default:
 		/* do nothing */
 		break;
@@ -1336,6 +1867,30 @@ static int dwc3_runtime_idle(struct device *dev)
 #endif /* CONFIG_PM */
 
 #ifdef CONFIG_PM_SLEEP
+static int dwc3_prepare(struct device *dev)
+{
+	struct dwc3	*dwc = dev_get_drvdata(dev);
+	unsigned long	flags;
+
+	spin_lock_irqsave(&dwc->lock, flags);
+	dwc->otg_prevent_sync = true;
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	return 0;
+}
+
+static void dwc3_complete(struct device *dev)
+{
+	struct dwc3	*dwc = dev_get_drvdata(dev);
+	unsigned long	flags;
+
+	spin_lock_irqsave(&dwc->lock, flags);
+	dwc->otg_prevent_sync = false;
+	spin_unlock_irqrestore(&dwc->lock, flags);
+	if (dwc->dr_mode == USB_DR_MODE_OTG)
+		queue_work(system_power_efficient_wq, &dwc->otg_work);
+}
+
 static int dwc3_suspend(struct device *dev)
 {
 	struct dwc3	*dwc = dev_get_drvdata(dev);
@@ -1373,6 +1928,10 @@ static const struct dev_pm_ops dwc3_dev_pm_ops = {
 	SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume)
 	SET_RUNTIME_PM_OPS(dwc3_runtime_suspend, dwc3_runtime_resume,
 			dwc3_runtime_idle)
+#ifdef CONFIG_PM_SLEEP
+	.prepare = dwc3_prepare,
+	.complete = dwc3_complete,
+#endif
 };
 
 #ifdef CONFIG_OF
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index fc82d2e..bf8951d 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -35,6 +35,11 @@
 
 #include <linux/phy/phy.h>
 
+#include <linux/usb/otg.h>
+#include <linux/usb/otg-fsm.h>
+
+#include <linux/workqueue.h>
+
 #define DWC3_MSG_MAX	500
 
 /* Global constants */
@@ -854,15 +859,23 @@ struct dwc3_scratchpad_array {
  * @event_buffer_list: a list of event buffers
  * @gadget: device side representation of the peripheral controller
  * @gadget_driver: pointer to the gadget driver
+ * @gadget_pullup: gadget driver's pullup request flag
  * @regs: base address for our registers
  * @regs_size: address space size
+ * @dr_mode: requested mode of operation
+ * @otg: usb otg data structure
+ * @otg-fsm: usb otg fsm data structure
+ * @otg_prevent_sync: flag to block events to otg fsm
+ * @current_mode: current mode of operation written to PRTCAPDIR
+ * @otg_work: work struct for otg/dual-role
+ * @otg_needs_host_start: flag that OTG controller needs to start host
  * @fladj: frame length adjustment
  * @irq_gadget: peripheral controller's IRQ number
+ * @otg_irq: IRQ number for OTG IRQs
  * @nr_scratch: number of scratch buffers
  * @u1u2: only used on revisions <1.83a for workaround
  * @maximum_speed: maximum speed requested (mainly for testing purposes)
  * @revision: revision register contents
- * @dr_mode: requested mode of operation
  * @hsphy_mode: UTMI phy mode, one of following:
  *		- USBPHY_INTERFACE_MODE_UTMI
  *		- USBPHY_INTERFACE_MODE_UTMIW
@@ -873,6 +886,9 @@ struct dwc3_scratchpad_array {
  * @ulpi: pointer to ulpi interface
  * @dcfg: saved contents of DCFG register
  * @gctl: saved contents of GCTL register
+ * @ocfg: saved contents of OCFG register
+ * @octl: saved contents of OCTL register
+ * @oevten: saved contents of OEVTEN register
  * @isoch_delay: wValue from Set Isochronous Delay request;
  * @u2sel: parameter from Set SEL request.
  * @u2pel: parameter from Set SEL request.
@@ -964,6 +980,7 @@ struct dwc3 {
 
 	struct usb_gadget	gadget;
 	struct usb_gadget_driver *gadget_driver;
+	bool			gadget_pullup;
 
 	struct usb_phy		*usb2_phy;
 	struct usb_phy		*usb3_phy;
@@ -973,14 +990,30 @@ struct dwc3 {
 
 	struct ulpi		*ulpi;
 
+	/* used for suspend/resume */
+	u32			dcfg;
+	u32			gctl;
+	u32			ocfg;
+	u32			octl;
+	u32			oevten;
+
 	void __iomem		*regs;
 	size_t			regs_size;
 
 	enum usb_dr_mode	dr_mode;
+	struct usb_otg		otg;
+	struct otg_fsm		otg_fsm;
+	bool			otg_prevent_sync;
+	u32			current_mode;
+	struct work_struct	otg_work;
+	bool			otg_needs_host_start;
+
 	enum usb_phy_interface	hsphy_mode;
 
 	u32			fladj;
 	u32			irq_gadget;
+	int			otg_irq;
+
 	u32			nr_scratch;
 	u32			u1u2;
 	u32			maximum_speed;
@@ -1277,6 +1310,9 @@ int dwc3_gadget_set_link_state(struct dwc3 *dwc, enum dwc3_link_state state);
 int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd,
 		struct dwc3_gadget_ep_cmd_params *params);
 int dwc3_send_gadget_generic_command(struct dwc3 *dwc, unsigned cmd, u32 param);
+int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend);
+int __dwc3_gadget_start(struct dwc3 *dwc);
+void __dwc3_gadget_stop(struct dwc3 *dwc);
 #else
 static inline int dwc3_gadget_init(struct dwc3 *dwc)
 { return 0; }
@@ -1296,6 +1332,12 @@ static inline int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd,
 static inline int dwc3_send_gadget_generic_command(struct dwc3 *dwc,
 		int cmd, u32 param)
 { return 0; }
+static inline int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
+{ return 0; }
+static inline int __dwc3_gadget_start(struct dwc3 *dwc)
+{ return 0; }
+static inline void __dwc3_gadget_stop(struct dwc3 *dwc)
+{ }
 #endif
 
 /* power management interface */
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 1341ed4..68a0e99 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1534,7 +1534,7 @@ static int dwc3_gadget_set_selfpowered(struct usb_gadget *g,
 	return 0;
 }
 
-static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
+int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
 {
 	u32			reg;
 	u32			timeout = 500;
@@ -1542,6 +1542,10 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int is_on, int suspend)
 	if (pm_runtime_suspended(dwc->dev))
 		return 0;
 
+	if (dwc->dr_mode == USB_DR_MODE_OTG &&
+	    dwc->otg_fsm.protocol != PROTO_GADGET)
+		return 0;
+
 	reg = dwc3_readl(dwc->regs, DWC3_DCTL);
 	if (is_on) {
 		if (dwc->revision <= DWC3_REVISION_187A) {
@@ -1603,6 +1607,7 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
 	}
 
 	spin_lock_irqsave(&dwc->lock, flags);
+	dwc->gadget_pullup = is_on;
 	ret = dwc3_gadget_run_stop(dwc, is_on, false);
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
@@ -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;
@@ -1812,8 +1817,11 @@ static int dwc3_gadget_start(struct usb_gadget *g,
 
 	dwc->gadget_driver	= driver;
 
-	if (pm_runtime_active(dwc->dev))
-		__dwc3_gadget_start(dwc);
+	if (pm_runtime_active(dwc->dev)) {
+		if (!(dwc->dr_mode == USB_DR_MODE_OTG &&
+		      dwc->otg_fsm.protocol != PROTO_GADGET))
+			__dwc3_gadget_start(dwc);
+	}
 
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
@@ -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)
 {
 	dwc3_gadget_disable_irq(dwc);
 	__dwc3_gadget_ep_disable(dwc->eps[0]);
-- 
2.7.4

[PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Roger Quadros]

dra7 OTG core limits the host controller to USB2.0 (high-speed) mode
when we're operating in dual-role.

We work around that by bypassing the OTG core and reading the
extcon framework directly for ID/VBUS events.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 Documentation/devicetree/bindings/usb/dwc3.txt |   2 +
 drivers/usb/dwc3/core.c                        | 169 ++++++++++++++++++++++++-
 drivers/usb/dwc3/core.h                        |   5 +
 3 files changed, 170 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index e3e6983..9955c0d 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -53,6 +53,8 @@ Optional properties:
  - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
 	register for post-silicon frame length adjustment when the
 	fladj_30mhz_sdbnd signal is invalid or incorrect.
+ - extcon: phandle to the USB connector extcon device. If present, extcon
+	device will be used to get USB cable events instead of OTG controller.
 
  - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.
 
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 619fa7c..b02d911 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -918,6 +918,19 @@ static void dwc3_otg_fsm_sync(struct dwc3 *dwc)
 	if (dwc->otg_prevent_sync)
 		return;
 
+	if (dwc->edev) {
+		/* get ID */
+		id = extcon_get_state(dwc->edev, EXTCON_USB_HOST);
+		/* Host means ID == 0 */
+		id = !id;
+
+		/* get VBUS */
+		vbus = extcon_get_state(dwc->edev, EXTCON_USB);
+		dwc3_drd_statemachine(dwc, id, vbus);
+
+		return;
+	}
+
 	reg = dwc3_readl(dwc->regs, DWC3_OSTS);
 	id = !!(reg & DWC3_OSTS_CONIDSTS);
 	vbus = !!(reg & DWC3_OSTS_BSESVLD);
@@ -934,6 +947,17 @@ static void dwc3_drd_work(struct work_struct *work)
 	spin_unlock(&dwc->lock);
 }
 
+static int dwc3_drd_notifier(struct notifier_block *nb,
+			     unsigned long event, void *ptr)
+{
+	struct dwc3 *dwc = container_of(nb, struct dwc3, edev_nb);
+
+	if (!dwc->otg_prevent_sync)
+		queue_work(system_power_efficient_wq, &dwc->otg_work);
+
+	return NOTIFY_DONE;
+}
+
 static void dwc3_otg_disable_events(struct dwc3 *dwc, u32 disable_mask)
 {
 	dwc->oevten &= ~(disable_mask);
@@ -1040,6 +1064,27 @@ static int dwc3_drd_start_host(struct dwc3 *dwc, int on, bool skip)
 {
 	u32 reg;
 
+	if (!dwc->edev)
+		goto otg;
+
+	if (on)
+		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
+
+	if (!skip) {
+		spin_unlock(&dwc->lock);
+
+		/* start or stop the HCD */
+		if (on)
+			dwc3_host_init(dwc);
+		else
+			dwc3_host_exit(dwc);
+
+		spin_lock(&dwc->lock);
+	}
+
+	return 0;
+
+otg:
 	/* switch OTG core */
 	if (on) {
 		/* As per Figure 11-10 A-Device Flow Diagram */
@@ -1133,6 +1178,33 @@ static int dwc3_drd_start_gadget(struct dwc3 *dwc, int on)
 	if (on)
 		dwc3_event_buffers_setup(dwc);
 
+	if (!dwc->edev)
+		goto otg;
+
+	if (on) {
+		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
+		/* start the Peripheral driver  */
+		if (dwc->gadget_driver) {
+			__dwc3_gadget_start(dwc);
+			if (dwc->gadget_pullup)
+				dwc3_gadget_run_stop(dwc, true, false);
+		}
+	} else {
+		/* 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);
+		}
+	}
+
+	return 0;
+
+otg:
 	if (on) {
 		/* As per Figure 11-20 B-Device Flow Diagram */
 
@@ -1251,6 +1323,13 @@ static void dwc3_otg_core_init(struct dwc3 *dwc)
 {
 	u32 reg;
 
+	/* force drd state machine update the first time */
+	dwc->otg_fsm.b_sess_vld = -1;
+	dwc->otg_fsm.id = -1;
+
+	if (dwc->edev)
+		return;
+
 	/*
 	 * As per Figure 11-4 OTG Driver Overall Programming Flow,
 	 * block "Initialize GCTL for OTG operation".
@@ -1264,15 +1343,14 @@ static void dwc3_otg_core_init(struct dwc3 *dwc)
 
 	/* 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;
 }
 
 /* dwc->lock must be held */
 static void dwc3_otg_core_exit(struct dwc3 *dwc)
 {
+	if (dwc->edev)
+		return;
+
 	/* disable all otg irqs */
 	dwc3_otg_disable_events(dwc, DWC3_OTG_ALL_EVENTS);
 	/* clear all events */
@@ -1286,6 +1364,57 @@ static int dwc3_drd_init(struct dwc3 *dwc)
 
 	INIT_WORK(&dwc->otg_work, dwc3_drd_work);
 
+	/* If extcon device is present we don't rely on OTG core for ID event */
+	if (dwc->edev) {
+		int id, vbus;
+
+		dwc->edev_nb.notifier_call = dwc3_drd_notifier;
+		ret = extcon_register_notifier(dwc->edev, EXTCON_USB,
+					       &dwc->edev_nb);
+		if (ret < 0) {
+			dev_err(dwc->dev, "Couldn't register USB cable notifier\n");
+			return -ENODEV;
+		}
+
+		ret = extcon_register_notifier(dwc->edev, EXTCON_USB_HOST,
+					       &dwc->edev_nb);
+		if (ret < 0) {
+			dev_err(dwc->dev, "Couldn't register USB-HOST cable notifier\n");
+			ret = -ENODEV;
+			goto extcon_fail;
+		}
+
+		/* sanity check id & vbus states */
+		id = extcon_get_state(dwc->edev, EXTCON_USB_HOST);
+		vbus = extcon_get_state(dwc->edev, EXTCON_USB);
+		if (id < 0 || vbus < 0) {
+			dev_err(dwc->dev, "Invalid USB cable state. id %d, vbus %d\n",
+				id, vbus);
+			ret = -ENODEV;
+			goto fail;
+		}
+
+		ret = dwc3_gadget_init(dwc);
+		if (ret)
+			goto fail;
+
+		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);
+
+		return 0;
+
+fail:
+		extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST,
+					   &dwc->edev_nb);
+extcon_fail:
+		extcon_unregister_notifier(dwc->edev, EXTCON_USB,
+					   &dwc->edev_nb);
+
+		return ret;
+	}
+
 	irq = dwc3_otg_get_irq(dwc);
 	if (irq < 0)
 		return irq;
@@ -1326,13 +1455,24 @@ static void dwc3_drd_exit(struct dwc3 *dwc)
 {
 	unsigned long flags;
 
+	spin_lock(&dwc->lock);
+	dwc->otg_prevent_sync = true;
+	spin_unlock(&dwc->lock);
 	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);
+	if (dwc->edev) {
+		extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST,
+					   &dwc->edev_nb);
+		extcon_unregister_notifier(dwc->edev, EXTCON_USB,
+					   &dwc->edev_nb);
+	} else {
+		free_irq(dwc->otg_irq, dwc);
+	}
 	spin_unlock_irqrestore(&dwc->lock, flags);
 
 	dwc3_gadget_exit(dwc);
@@ -1587,6 +1727,14 @@ static int dwc3_probe(struct platform_device *pdev)
 
 	dwc3_get_properties(dwc);
 
+	if (dev->of_node) {
+		if (of_property_read_bool(dev->of_node, "extcon"))
+			dwc->edev = extcon_get_edev_by_phandle(dev, 0);
+
+		if (IS_ERR(dwc->edev))
+			return PTR_ERR(dwc->edev);
+	}
+
 	platform_set_drvdata(pdev, dwc);
 	dwc3_cache_hwparams(dwc);
 
@@ -1743,6 +1891,13 @@ static int dwc3_resume_common(struct dwc3 *dwc)
 	if (ret)
 		return ret;
 
+	if (dwc->dr_mode == USB_DR_MODE_OTG &&
+	    dwc->edev) {
+		if (dwc->otg_fsm.protocol == PROTO_HOST)
+			dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
+		else if (dwc->otg_fsm.protocol == PROTO_GADGET)
+			dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
+	}
 	spin_lock_irqsave(&dwc->lock, flags);
 
 	switch (dwc->dr_mode) {
@@ -1771,8 +1926,10 @@ static int dwc3_resume_common(struct dwc3 *dwc)
 
 	if (dwc->dr_mode == USB_DR_MODE_OTG) {
 		dwc3_otg_core_init(dwc);
-		if (dwc->otg_fsm.protocol == PROTO_HOST)
+		if ((dwc->otg_fsm.protocol == PROTO_HOST) &&
+		    !dwc->edev) {
 			dwc3_drd_start_host(dwc, true, 1);
+		}
 	}
 
 	spin_unlock_irqrestore(&dwc->lock, flags);
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index bf8951d..fc060ae 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -38,6 +38,7 @@
 #include <linux/usb/otg.h>
 #include <linux/usb/otg-fsm.h>
 
+#include <linux/extcon.h>
 #include <linux/workqueue.h>
 
 #define DWC3_MSG_MAX	500
@@ -869,6 +870,8 @@ struct dwc3_scratchpad_array {
  * @current_mode: current mode of operation written to PRTCAPDIR
  * @otg_work: work struct for otg/dual-role
  * @otg_needs_host_start: flag that OTG controller needs to start host
+ * @edev: extcon handle
+ * @edev_nb: extcon notifier
  * @fladj: frame length adjustment
  * @irq_gadget: peripheral controller's IRQ number
  * @otg_irq: IRQ number for OTG IRQs
@@ -1007,6 +1010,8 @@ struct dwc3 {
 	u32			current_mode;
 	struct work_struct	otg_work;
 	bool			otg_needs_host_start;
+	struct extcon_dev	*edev;
+	struct notifier_block	edev_nb;
 
 	enum usb_phy_interface	hsphy_mode;
 
-- 
2.7.4

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Vivek Gautam]

On 02/16/2017 06:36 PM, Roger Quadros wrote:
> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode
> when we're operating in dual-role.
>
> We work around that by bypassing the OTG core and reading the
> extcon framework directly for ID/VBUS events.
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>
> ---
>   Documentation/devicetree/bindings/usb/dwc3.txt |   2 +
>   drivers/usb/dwc3/core.c                        | 169 ++++++++++++++++++++++++-
>   drivers/usb/dwc3/core.h                        |   5 +
>   3 files changed, 170 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index e3e6983..9955c0d 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -53,6 +53,8 @@ Optional properties:
>    - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>   	register for post-silicon frame length adjustment when the
>   	fladj_30mhz_sdbnd signal is invalid or incorrect.
> + - extcon: phandle to the USB connector extcon device. If present, extcon
> +	device will be used to get USB cable events instead of OTG controller.
>   
>    - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.
>   
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 619fa7c..b02d911 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c

[snip]

> @@ -1587,6 +1727,14 @@ static int dwc3_probe(struct platform_device *pdev)
>   
>   	dwc3_get_properties(dwc);
>   
> +	if (dev->of_node) {
> +		if (of_property_read_bool(dev->of_node, "extcon"))
> +			dwc->edev = extcon_get_edev_by_phandle(dev, 0);

Don't we want separate edev's for vbus and id ?
One can have separate pins connected to them and in that case
we can't get the events out of one pin only.

> +
> +		if (IS_ERR(dwc->edev))
> +			return PTR_ERR(dwc->edev);

  Took me a while to get to this. :)

                 if (IS_ERR(dwc->edev)) {
                        ret = PTR_ERR(dwc->edev);
                        goto err0;
                 }

We want to reset the res->start back to its original offset.

Testing this series currently. Will get back with my results.


Regards
Vivek

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Peter Chen]

On Thu, Feb 23, 2017 at 02:04:50PM +0530, Vivek Gautam wrote:
> 
> 
> On 02/16/2017 06:36 PM, Roger Quadros wrote:
> >dra7 OTG core limits the host controller to USB2.0 (high-speed) mode
> >when we're operating in dual-role.
> >
> >We work around that by bypassing the OTG core and reading the
> >extcon framework directly for ID/VBUS events.
> >
> >Signed-off-by: Roger Quadros <rogerq@ti.com>
> >---
> >  Documentation/devicetree/bindings/usb/dwc3.txt |   2 +
> >  drivers/usb/dwc3/core.c                        | 169 ++++++++++++++++++++++++-
> >  drivers/usb/dwc3/core.h                        |   5 +
> >  3 files changed, 170 insertions(+), 6 deletions(-)
> >
> >diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> >index e3e6983..9955c0d 100644
> >--- a/Documentation/devicetree/bindings/usb/dwc3.txt
> >+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> >@@ -53,6 +53,8 @@ Optional properties:
> >   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
> >  	register for post-silicon frame length adjustment when the
> >  	fladj_30mhz_sdbnd signal is invalid or incorrect.
> >+ - extcon: phandle to the USB connector extcon device. If present, extcon
> >+	device will be used to get USB cable events instead of OTG controller.
> >   - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.
> >diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> >index 619fa7c..b02d911 100644
> >--- a/drivers/usb/dwc3/core.c
> >+++ b/drivers/usb/dwc3/core.c
> 
> [snip]
> 
> >@@ -1587,6 +1727,14 @@ static int dwc3_probe(struct platform_device *pdev)
> >  	dwc3_get_properties(dwc);
> >+	if (dev->of_node) {
> >+		if (of_property_read_bool(dev->of_node, "extcon"))
> >+			dwc->edev = extcon_get_edev_by_phandle(dev, 0);
> 
> Don't we want separate edev's for vbus and id ?
> One can have separate pins connected to them and in that case
> we can't get the events out of one pin only.
> 

Current extcon-usb-gpio driver supports id and vbus at the same time,
that means there are two optional gpios under one extcon node.

-- 

Best Regards,
Peter Chen

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Vivek Gautam]

On 02/24/2017 06:27 AM, Peter Chen wrote:
> On Thu, Feb 23, 2017 at 02:04:50PM +0530, Vivek Gautam wrote:
>>
>> On 02/16/2017 06:36 PM, Roger Quadros wrote:
>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode
>>> when we're operating in dual-role.
>>>
>>> We work around that by bypassing the OTG core and reading the
>>> extcon framework directly for ID/VBUS events.
>>>
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>> ---
>>>   Documentation/devicetree/bindings/usb/dwc3.txt |   2 +
>>>   drivers/usb/dwc3/core.c                        | 169 ++++++++++++++++++++++++-
>>>   drivers/usb/dwc3/core.h                        |   5 +
>>>   3 files changed, 170 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> index e3e6983..9955c0d 100644
>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> @@ -53,6 +53,8 @@ Optional properties:
>>>    - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>>>   	register for post-silicon frame length adjustment when the
>>>   	fladj_30mhz_sdbnd signal is invalid or incorrect.
>>> + - extcon: phandle to the USB connector extcon device. If present, extcon
>>> +	device will be used to get USB cable events instead of OTG controller.
>>>    - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 619fa7c..b02d911 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>> [snip]
>>
>>> @@ -1587,6 +1727,14 @@ static int dwc3_probe(struct platform_device *pdev)
>>>   	dwc3_get_properties(dwc);
>>> +	if (dev->of_node) {
>>> +		if (of_property_read_bool(dev->of_node, "extcon"))
>>> +			dwc->edev = extcon_get_edev_by_phandle(dev, 0);
>> Don't we want separate edev's for vbus and id ?
>> One can have separate pins connected to them and in that case
>> we can't get the events out of one pin only.
>>
> Current extcon-usb-gpio driver supports id and vbus at the same time,
> that means there are two optional gpios under one extcon node.
>

Right, and we would want to leverage that by providing couple
of phandles for vbus and id, and requesting the two in driver.


Regards
Vivek

-- 
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Roger Quadros]

+Chanwoo

Hi Vivek,

On 23/02/17 10:34, Vivek Gautam wrote:
> 
> 
> On 02/16/2017 06:36 PM, Roger Quadros wrote:
>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode
>> when we're operating in dual-role.
>>
>> We work around that by bypassing the OTG core and reading the
>> extcon framework directly for ID/VBUS events.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> ---
>>   Documentation/devicetree/bindings/usb/dwc3.txt |   2 +
>>   drivers/usb/dwc3/core.c                        | 169 ++++++++++++++++++++++++-
>>   drivers/usb/dwc3/core.h                        |   5 +
>>   3 files changed, 170 insertions(+), 6 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>> index e3e6983..9955c0d 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>> @@ -53,6 +53,8 @@ Optional properties:
>>    - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>>       register for post-silicon frame length adjustment when the
>>       fladj_30mhz_sdbnd signal is invalid or incorrect.
>> + - extcon: phandle to the USB connector extcon device. If present, extcon
>> +    device will be used to get USB cable events instead of OTG controller.
>>      - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.
>>   diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 619fa7c..b02d911 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
> 
> [snip]
> 
>> @@ -1587,6 +1727,14 @@ static int dwc3_probe(struct platform_device *pdev)
>>         dwc3_get_properties(dwc);
>>   +    if (dev->of_node) {
>> +        if (of_property_read_bool(dev->of_node, "extcon"))
>> +            dwc->edev = extcon_get_edev_by_phandle(dev, 0);
> 
> Don't we want separate edev's for vbus and id ?
> One can have separate pins connected to them and in that case
> we can't get the events out of one pin only.

Is such kind of hardware really there? Ideally one extcon device
represents one connector. So VBUS and ID events of a single USB
port should come on one extcon device.
If it doesn't then you need to fix your platforms extcon driver
so that it does.
Chanwoo can correct me if I'm wrong on this understanding.

Currently, if VBUS and ID come on GPIOs the extcon-usb-gpio driver
takes care of both.

> 
>> +
>> +        if (IS_ERR(dwc->edev))
>> +            return PTR_ERR(dwc->edev);
> 
>  Took me a while to get to this. :)
> 
>                 if (IS_ERR(dwc->edev)) {
>                        ret = PTR_ERR(dwc->edev);
>                        goto err0;
>                 }
> 
> We want to reset the res->start back to its original offset.
> 
> Testing this series currently. Will get back with my results.
> 
> 
Thanks :)

-- 
cheers,
-roger

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Chanwoo Choi]

Hi Roger,

[snip]

>  /* dwc->lock must be held */
>  static void dwc3_otg_core_exit(struct dwc3 *dwc)
>  {
> +       if (dwc->edev)
> +               return;
> +
>         /* disable all otg irqs */
>         dwc3_otg_disable_events(dwc, DWC3_OTG_ALL_EVENTS);
>         /* clear all events */
> @@ -1286,6 +1364,57 @@ static int dwc3_drd_init(struct dwc3 *dwc)
>
>         INIT_WORK(&dwc->otg_work, dwc3_drd_work);
>
> +       /* If extcon device is present we don't rely on OTG core for ID event */
> +       if (dwc->edev) {
> +               int id, vbus;
> +
> +               dwc->edev_nb.notifier_call = dwc3_drd_notifier;
> +               ret = extcon_register_notifier(dwc->edev, EXTCON_USB,
> +                                              &dwc->edev_nb);

I recommend that you better to use the devm_extcon_register_notifier()

> +               if (ret < 0) {
> +                       dev_err(dwc->dev, "Couldn't register USB cable notifier\n");
> +                       return -ENODEV;
> +               }
> +
> +               ret = extcon_register_notifier(dwc->edev, EXTCON_USB_HOST,
> +                                              &dwc->edev_nb);

Ditto.

[snip]

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Chanwoo Choi]

Hi,

2017-02-24 21:02 GMT+09:00 Roger Quadros <rogerq@ti.com>:
> +Chanwoo
>
> Hi Vivek,
>
> On 23/02/17 10:34, Vivek Gautam wrote:
>>
>>
>> On 02/16/2017 06:36 PM, Roger Quadros wrote:
>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode
>>> when we're operating in dual-role.
>>>
>>> We work around that by bypassing the OTG core and reading the
>>> extcon framework directly for ID/VBUS events.
>>>
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>> ---
>>>   Documentation/devicetree/bindings/usb/dwc3.txt |   2 +
>>>   drivers/usb/dwc3/core.c                        | 169 ++++++++++++++++++++++++-
>>>   drivers/usb/dwc3/core.h                        |   5 +
>>>   3 files changed, 170 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> index e3e6983..9955c0d 100644
>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> @@ -53,6 +53,8 @@ Optional properties:
>>>    - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>>>       register for post-silicon frame length adjustment when the
>>>       fladj_30mhz_sdbnd signal is invalid or incorrect.
>>> + - extcon: phandle to the USB connector extcon device. If present, extcon
>>> +    device will be used to get USB cable events instead of OTG controller.
>>>      - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.
>>>   diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 619fa7c..b02d911 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>
>> [snip]
>>
>>> @@ -1587,6 +1727,14 @@ static int dwc3_probe(struct platform_device *pdev)
>>>         dwc3_get_properties(dwc);
>>>   +    if (dev->of_node) {
>>> +        if (of_property_read_bool(dev->of_node, "extcon"))
>>> +            dwc->edev = extcon_get_edev_by_phandle(dev, 0);
>>
>> Don't we want separate edev's for vbus and id ?
>> One can have separate pins connected to them and in that case
>> we can't get the events out of one pin only.
>
> Is such kind of hardware really there? Ideally one extcon device
> represents one connector. So VBUS and ID events of a single USB
> port should come on one extcon device.
> If it doesn't then you need to fix your platforms extcon driver
> so that it does.
> Chanwoo can correct me if I'm wrong on this understanding.
>
> Currently, if VBUS and ID come on GPIOs the extcon-usb-gpio driver
> takes care of both.

Basically, one extcon device driver can support the multiple
external connectors if the detection h/w or port is same.

One extcon device with extcon-usb-gpio.c can support the detection
of both ID and VBUS.

But, if there is any issue of h/w schematic, we need to consider
how to use the extcon device.

>
>>
>>> +
>>> +        if (IS_ERR(dwc->edev))
>>> +            return PTR_ERR(dwc->edev);
>>
>>  Took me a while to get to this. :)
>>
>>                 if (IS_ERR(dwc->edev)) {
>>                        ret = PTR_ERR(dwc->edev);
>>                        goto err0;
>>                 }
>>
>> We want to reset the res->start back to its original offset.
>>
>> Testing this series currently. Will get back with my results.
>>
>>
> Thanks :)
>
> --
> cheers,
> -roger

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Chanwoo Choi]

2017-02-25 12:46 GMT+09:00 Chanwoo Choi <cwchoi00@gmail.com>:
> Hi,
>
> 2017-02-24 21:02 GMT+09:00 Roger Quadros <rogerq@ti.com>:
>> +Chanwoo
>>
>> Hi Vivek,
>>
>> On 23/02/17 10:34, Vivek Gautam wrote:
>>>
>>>
>>> On 02/16/2017 06:36 PM, Roger Quadros wrote:
>>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode
>>>> when we're operating in dual-role.
>>>>
>>>> We work around that by bypassing the OTG core and reading the
>>>> extcon framework directly for ID/VBUS events.
>>>>
>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>> ---
>>>>   Documentation/devicetree/bindings/usb/dwc3.txt |   2 +
>>>>   drivers/usb/dwc3/core.c                        | 169 ++++++++++++++++++++++++-
>>>>   drivers/usb/dwc3/core.h                        |   5 +
>>>>   3 files changed, 170 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> index e3e6983..9955c0d 100644
>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> @@ -53,6 +53,8 @@ Optional properties:
>>>>    - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>>>>       register for post-silicon frame length adjustment when the
>>>>       fladj_30mhz_sdbnd signal is invalid or incorrect.
>>>> + - extcon: phandle to the USB connector extcon device. If present, extcon
>>>> +    device will be used to get USB cable events instead of OTG controller.
>>>>      - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.
>>>>   diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 619fa7c..b02d911 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>
>>> [snip]
>>>
>>>> @@ -1587,6 +1727,14 @@ static int dwc3_probe(struct platform_device *pdev)
>>>>         dwc3_get_properties(dwc);
>>>>   +    if (dev->of_node) {
>>>> +        if (of_property_read_bool(dev->of_node, "extcon"))
>>>> +            dwc->edev = extcon_get_edev_by_phandle(dev, 0);
>>>
>>> Don't we want separate edev's for vbus and id ?
>>> One can have separate pins connected to them and in that case
>>> we can't get the events out of one pin only.
>>
>> Is such kind of hardware really there? Ideally one extcon device
>> represents one connector. So VBUS and ID events of a single USB
>> port should come on one extcon device.
>> If it doesn't then you need to fix your platforms extcon driver
>> so that it does.
>> Chanwoo can correct me if I'm wrong on this understanding.
>>
>> Currently, if VBUS and ID come on GPIOs the extcon-usb-gpio driver
>> takes care of both.
>
> Basically, one extcon device driver can support the multiple
> external connectors if the detection h/w or port is same.
>
> One extcon device with extcon-usb-gpio.c can support the detection
> of both ID and VBUS.
>
> But, if there is any issue of h/w schematic, we need to consider
> how to use the extcon device.

Additionally, the extcon-usb-gpio.c use the two gpio pins
to detect both ID and VBUS pins. We can check it on documentation[1]
of extcon-usb-gpio driver.

[1] https://git.kernel.org/cgit/linux/kernel/git/chanwoo/extcon.git/tree/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Vivek Gautam]

On Sat, Feb 25, 2017 at 9:20 AM, Chanwoo Choi <cwchoi00@gmail.com> wrote:
> 2017-02-25 12:46 GMT+09:00 Chanwoo Choi <cwchoi00@gmail.com>:
>> Hi,
>>
>> 2017-02-24 21:02 GMT+09:00 Roger Quadros <rogerq@ti.com>:
>>> +Chanwoo
>>>
>>> Hi Vivek,
>>>
>>> On 23/02/17 10:34, Vivek Gautam wrote:
>>>>
>>>>
>>>> On 02/16/2017 06:36 PM, Roger Quadros wrote:
>>>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode
>>>>> when we're operating in dual-role.
>>>>>
>>>>> We work around that by bypassing the OTG core and reading the
>>>>> extcon framework directly for ID/VBUS events.
>>>>>
>>>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>>>>> ---
>>>>>   Documentation/devicetree/bindings/usb/dwc3.txt |   2 +
>>>>>   drivers/usb/dwc3/core.c                        | 169 ++++++++++++++++++++++++-
>>>>>   drivers/usb/dwc3/core.h                        |   5 +
>>>>>   3 files changed, 170 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> index e3e6983..9955c0d 100644
>>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> @@ -53,6 +53,8 @@ Optional properties:
>>>>>    - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>>>>>       register for post-silicon frame length adjustment when the
>>>>>       fladj_30mhz_sdbnd signal is invalid or incorrect.
>>>>> + - extcon: phandle to the USB connector extcon device. If present, extcon
>>>>> +    device will be used to get USB cable events instead of OTG controller.
>>>>>      - <DEPRECATED> tx-fifo-resize: determines if the FIFO *has* to be reallocated.
>>>>>   diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index 619fa7c..b02d911 100644
>>>>> --- a/drivers/usb/dwc3/core.c
>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>
>>>> [snip]
>>>>
>>>>> @@ -1587,6 +1727,14 @@ static int dwc3_probe(struct platform_device *pdev)
>>>>>         dwc3_get_properties(dwc);
>>>>>   +    if (dev->of_node) {
>>>>> +        if (of_property_read_bool(dev->of_node, "extcon"))
>>>>> +            dwc->edev = extcon_get_edev_by_phandle(dev, 0);
>>>>
>>>> Don't we want separate edev's for vbus and id ?
>>>> One can have separate pins connected to them and in that case
>>>> we can't get the events out of one pin only.
>>>
>>> Is such kind of hardware really there? Ideally one extcon device
>>> represents one connector. So VBUS and ID events of a single USB
>>> port should come on one extcon device.
>>> If it doesn't then you need to fix your platforms extcon driver
>>> so that it does.

Right, i see it now. One edev for one connector.

>>> Chanwoo can correct me if I'm wrong on this understanding.
>>>
>>> Currently, if VBUS and ID come on GPIOs the extcon-usb-gpio driver
>>> takes care of both.
>>
>> Basically, one extcon device driver can support the multiple
>> external connectors if the detection h/w or port is same.
>>
>> One extcon device with extcon-usb-gpio.c can support the detection
>> of both ID and VBUS.
>>
>> But, if there is any issue of h/w schematic, we need to consider
>> how to use the extcon device.
>
> Additionally, the extcon-usb-gpio.c use the two gpio pins
> to detect both ID and VBUS pins. We can check it on documentation[1]
> of extcon-usb-gpio driver.
>
> [1] https://git.kernel.org/cgit/linux/kernel/git/chanwoo/extcon.git/tree/Documentation/devicetree/bindings/extcon/extcon-usb-gpio.txt

Thanks Chanwoo, i understand it now. The extcon-usb-gpio supports
both vbus and id gpio, and updates the state of the edev likewise.

>
> --
> Best Regards,
> Chanwoo Choi
> Samsung Electronics
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


regards
Vivek
-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Roger Quadros]

On 25/02/17 05:35, Chanwoo Choi wrote:
> Hi Roger,
> 
> [snip]
> 
>>  /* dwc->lock must be held */
>>  static void dwc3_otg_core_exit(struct dwc3 *dwc)
>>  {
>> +       if (dwc->edev)
>> +               return;
>> +
>>         /* disable all otg irqs */
>>         dwc3_otg_disable_events(dwc, DWC3_OTG_ALL_EVENTS);
>>         /* clear all events */
>> @@ -1286,6 +1364,57 @@ static int dwc3_drd_init(struct dwc3 *dwc)
>>
>>         INIT_WORK(&dwc->otg_work, dwc3_drd_work);
>>
>> +       /* If extcon device is present we don't rely on OTG core for ID event */
>> +       if (dwc->edev) {
>> +               int id, vbus;
>> +
>> +               dwc->edev_nb.notifier_call = dwc3_drd_notifier;
>> +               ret = extcon_register_notifier(dwc->edev, EXTCON_USB,
>> +                                              &dwc->edev_nb);
> 
> I recommend that you better to use the devm_extcon_register_notifier()

Got it.

> 
>> +               if (ret < 0) {
>> +                       dev_err(dwc->dev, "Couldn't register USB cable notifier\n");
>> +                       return -ENODEV;
>> +               }
>> +
>> +               ret = extcon_register_notifier(dwc->edev, EXTCON_USB_HOST,
>> +                                              &dwc->edev_nb);
> 
> Ditto.
> 
> [snip]
> 

-- 
cheers,
-roger

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

[Roger Quadros]

Felipe,

On 16/02/17 15:06, Roger Quadros wrote:
> Hi,
> 
> We rely on the OTG controller block or Extcon to provide us with
> VBUS and ID line status via an interrupt.
> 
> This is then used to switch the controller between host, peripheral
> and idle roles based on the following 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
> 
> Series depends on
> [1] dwc3 trivial fixes - https://lkml.org/lkml/2017/2/15/204

Would appreciate any feedback on this series. Thanks.

> 
> Changelog:
> v2:
> - Use extcon device for VBUS/ID events if extcon phandle is present.
> - Fix bugs during system suspend/resume.
> 
> cheers,
> -roger
> 
> Roger Quadros (4):
>   usb: dwc3: core.h: add some register definitions
>   usb: dwc3: omap: don't miss events during suspend/resume
>   usb: dwc3: add dual-role support
>   usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode
> 
>  Documentation/devicetree/bindings/usb/dwc3.txt |   2 +
>  drivers/usb/dwc3/core.c                        | 752 ++++++++++++++++++++++++-
>  drivers/usb/dwc3/core.h                        | 131 ++++-
>  drivers/usb/dwc3/dwc3-omap.c                   |  16 +
>  drivers/usb/dwc3/gadget.c                      |  18 +-
>  5 files changed, 895 insertions(+), 24 deletions(-)
> 

-- 
cheers,
-roger

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 709 bytes --]

Roger Quadros <rogerq@ti.com> writes:

> Hi,
>
> We rely on the OTG controller block or Extcon to provide us with
> VBUS and ID line status via an interrupt.
>
> This is then used to switch the controller between host, peripheral
> and idle roles based on the following 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
>
> Series depends on
> [1] dwc3 trivial fixes - https://lkml.org/lkml/2017/2/15/204

right, you never sent v2 of that though, did you? :-s -ECONFUSED

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 22423 bytes --]


Hi,

Roger Quadros <rogerq@ti.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@ti.com>

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 638 bytes --]


Hi,

Roger Quadros <rogerq@ti.com> writes:
> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode
> when we're operating in dual-role.

yeah, that's not a quirk. DRA7 supports OTGv2, not OTGv3. There was no
USB3 when OTGv2 was written.

DRA7 just shouldn't use OTG core altogether. In fact, this is the very
thing I've been saying for a long time. Make the simplest implementation
possible. The dead simple, does-one-thing-only sort of implementation.

All we need for Dual-Role (without OTG extras) is some input for ID and
VBUS, then we add/remove HCD/UDC conditionally and set PRTCAPDIR.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Roger Quadros]

Hi,

On 28/03/17 13:27, Felipe Balbi wrote:
> Roger Quadros <rogerq@ti.com> writes:
> 
>> Hi,
>>
>> We rely on the OTG controller block or Extcon to provide us with
>> VBUS and ID line status via an interrupt.
>>
>> This is then used to switch the controller between host, peripheral
>> and idle roles based on the following 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
>>
>> Series depends on
>> [1] dwc3 trivial fixes - https://lkml.org/lkml/2017/2/15/204
> 
> right, you never sent v2 of that though, did you? :-s -ECONFUSED
> 

In that series, the 1st patch is already picked up for v4.11-rc.
I sent a v2 of the second patch but it seems it didn't apply cleanly.
https://lkml.org/lkml/2017/2/16/168

I'll send the last 2 patches again based off v4.11-rc4.

cheers,
-roger

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Roger Quadros]

On 28/03/17 14:10, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode
>> when we're operating in dual-role.
> 
> yeah, that's not a quirk. DRA7 supports OTGv2, not OTGv3. There was no
> USB3 when OTGv2 was written.
> 
> DRA7 just shouldn't use OTG core altogether. In fact, this is the very
> thing I've been saying for a long time. Make the simplest implementation
> possible. The dead simple, does-one-thing-only sort of implementation.
> 
> All we need for Dual-Role (without OTG extras) is some input for ID and
> VBUS, then we add/remove HCD/UDC conditionally and set PRTCAPDIR.
> 

The catch is that on AM437x there is no way to get ID and VBUS events other
than the OTG controller so we have to rely on the OTG controller for that. :(

I agree on the simplicity part. Let's do what minimal is necessary first.

cheers,
-roger

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3466 bytes --]


Hi,

Roger Quadros <rogerq@ti.com> writes:
>> Roger Quadros <rogerq@ti.com> writes:
>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode
>>> when we're operating in dual-role.
>> 
>> yeah, that's not a quirk. DRA7 supports OTGv2, not OTGv3. There was no
>> USB3 when OTGv2 was written.
>> 
>> DRA7 just shouldn't use OTG core altogether. In fact, this is the very
>> thing I've been saying for a long time. Make the simplest implementation
>> possible. The dead simple, does-one-thing-only sort of implementation.
>> 
>> All we need for Dual-Role (without OTG extras) is some input for ID and
>> VBUS, then we add/remove HCD/UDC conditionally and set PRTCAPDIR.
>> 
>
> The catch is that on AM437x there is no way to get ID and VBUS events other
> than the OTG controller so we have to rely on the OTG controller for that. :(

okay, so AM437x can get OTG interrupts properly. That's fine. We can
still do everything we need using code that's already existing in dwc3
if we refactor it a bit and hook it up to the OTG IRQ handler.

Here's what we do:

* First we re-factor all necessary code around so the API for OTG/DRD
  is resumed to calling:

	dwc3_add_udc(dwc);
        dwc3_del_udc(dwc);
        dwc3_add_hcd(dwc);
        dwc3_del_hcd(dwc);

the semantics of these should be easy to understand and you can
implement each in their respective host.c/gadget.c files.

* Second step is to modify our dwc3_init_mode() (or whatever that
  function was called, sorry, didn't check) to make sure we have
  something like:

	case OTG:
        	dwc3_add_udc(dwc);
                break;

We should *not* add HCD in this case yet.

* After that we add otg.c (or drd.c, no preference) and make that call
  dwc3_add_udc(dwc) and, also, provide
  dwc3_add_otg(dwc)/dwc3_del_otg(dwc) calls. Then patch the switch
  statement above to:

	case OTG:
        	dwc3_add_otg(dwc);
                break;

Note that at this point, this is simply a direct replacement of
dwc3_add_udc() to dwc3_add_otg(). This should maintain current behavior
(which is starting with peripheral mode by default), but it should also
add support for OTG interrupts to change the mode (from an interrupt
thead)

	otg_isr()
        {

		/* don't forget to remove preivous mode if necessary */
        	if (perimode)
                	dwc3_add_udc(dwc);
                else
                	dwc3_add_hcd(dwc);
	}

* The next patch would be to choose default conditionally based on
  PERIMODE or whatever.

Of course, this is an oversimplified view of reality. You still need to
poke around at PRTCAPDIR, etc. But all this can, actually, be prototyped
using our "mode" debugfs file. Just make that call
dwc3_add/del_udc/hcd() apart from fiddling with PRTCAPDIR in GCTL.

Your first implementation could be just that. Refactoring what needs to
be refactored, then patching "mode" debugfs to work properly in that
case. Only add otg.c/drd.c after "mode" debugfs file is stable, because
then you know what needs to be taken into consideration.

Just to be clear, I'm not saying we should *ONLY* get the debugfs
interface for v4.12, I'm saying you should start with that and get that
stable and working properly (make an infinite loop constantly changing
modes and keep it running over the weekend) before you add support for
OTG interrupts, which could come in the same series ;-)

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

[Roger Quadros]

Hi,

On 28/03/17 14:07, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.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@ti.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

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Roger Quadros]

On 29/03/17 13:32, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>>> Roger Quadros <rogerq@ti.com> writes:
>>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode
>>>> when we're operating in dual-role.
>>>
>>> yeah, that's not a quirk. DRA7 supports OTGv2, not OTGv3. There was no
>>> USB3 when OTGv2 was written.
>>>
>>> DRA7 just shouldn't use OTG core altogether. In fact, this is the very
>>> thing I've been saying for a long time. Make the simplest implementation
>>> possible. The dead simple, does-one-thing-only sort of implementation.
>>>
>>> All we need for Dual-Role (without OTG extras) is some input for ID and
>>> VBUS, then we add/remove HCD/UDC conditionally and set PRTCAPDIR.
>>>
>>
>> The catch is that on AM437x there is no way to get ID and VBUS events other
>> than the OTG controller so we have to rely on the OTG controller for that. :(
> 
> okay, so AM437x can get OTG interrupts properly. That's fine. We can
> still do everything we need using code that's already existing in dwc3
> if we refactor it a bit and hook it up to the OTG IRQ handler.
> 
> Here's what we do:
> 
> * First we re-factor all necessary code around so the API for OTG/DRD
>   is resumed to calling:
> 
> 	dwc3_add_udc(dwc);
>         dwc3_del_udc(dwc);
>         dwc3_add_hcd(dwc);
>         dwc3_del_hcd(dwc);
> 
> the semantics of these should be easy to understand and you can
> implement each in their respective host.c/gadget.c files.
> 
> * Second step is to modify our dwc3_init_mode() (or whatever that
>   function was called, sorry, didn't check) to make sure we have
>   something like:
> 
> 	case OTG:
>         	dwc3_add_udc(dwc);
>                 break;
> 
> We should *not* add HCD in this case yet.
> 
> * After that we add otg.c (or drd.c, no preference) and make that call
>   dwc3_add_udc(dwc) and, also, provide
>   dwc3_add_otg(dwc)/dwc3_del_otg(dwc) calls. Then patch the switch
>   statement above to:
> 
> 	case OTG:
>         	dwc3_add_otg(dwc);
>                 break;
> 
> Note that at this point, this is simply a direct replacement of
> dwc3_add_udc() to dwc3_add_otg(). This should maintain current behavior
> (which is starting with peripheral mode by default), but it should also
> add support for OTG interrupts to change the mode (from an interrupt
> thead)
> 
> 	otg_isr()
>         {
> 
> 		/* don't forget to remove preivous mode if necessary */
>         	if (perimode)
>                 	dwc3_add_udc(dwc);
>                 else
>                 	dwc3_add_hcd(dwc);
> 	}
> 
> * The next patch would be to choose default conditionally based on
>   PERIMODE or whatever.
> 
> Of course, this is an oversimplified view of reality. You still need to
> poke around at PRTCAPDIR, etc. But all this can, actually, be prototyped
> using our "mode" debugfs file. Just make that call
> dwc3_add/del_udc/hcd() apart from fiddling with PRTCAPDIR in GCTL.
> 
> Your first implementation could be just that. Refactoring what needs to
> be refactored, then patching "mode" debugfs to work properly in that
> case. Only add otg.c/drd.c after "mode" debugfs file is stable, because
> then you know what needs to be taken into consideration.
> 
> Just to be clear, I'm not saying we should *ONLY* get the debugfs
> interface for v4.12, I'm saying you should start with that and get that
> stable and working properly (make an infinite loop constantly changing
> modes and keep it running over the weekend) before you add support for
> OTG interrupts, which could come in the same series ;-)
> 

Agree with you. Moreover I could get rid of OTG controller related code
and have just debugfs and extcon implementation. We can add the OTG controller
bits later.

I agree with you on everything you said except using add/del_gadget_udc. :)
I've explained why we can't use del_gadget_udc in the other thread
but I'll explain it here again.

1) If we start in host role, usb_add_gadget_udc() won't be called. That means
no UDC and user can't load a gadget driver. Typical applications need to have
a gadget driver ready *before* the peripheral mode starts so that it can
enumerate immediately.

2) If we use usb_del_gadget_udc() when switching to host mode and
usb_add_gadget_udc() when switching back to peripheral mode, the previously
loaded gadget driver will not be assigned to this UDC. User has to unload
and reload the gadget driver.

3) All this becomes even more complex for configfs based gadget driver.

So using stop/start gadget is a much simpler solution really as UDC software
side of things remain unchanged and the gadget driver can persist between
role switches.

cheers,
-roger

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

[Felipe Balbi]

Hi,

Roger Quadros <rogerq@ti.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;

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

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.

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.

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

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

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

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

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

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

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

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

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

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

-- 
balbi

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Felipe Balbi]

Hi,

Roger Quadros <rogerq@ti.com> writes:
>> Roger Quadros <rogerq@ti.com> writes:
>>>> Roger Quadros <rogerq@ti.com> writes:
>>>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode
>>>>> when we're operating in dual-role.
>>>>
>>>> yeah, that's not a quirk. DRA7 supports OTGv2, not OTGv3. There was no
>>>> USB3 when OTGv2 was written.
>>>>
>>>> DRA7 just shouldn't use OTG core altogether. In fact, this is the very
>>>> thing I've been saying for a long time. Make the simplest implementation
>>>> possible. The dead simple, does-one-thing-only sort of implementation.
>>>>
>>>> All we need for Dual-Role (without OTG extras) is some input for ID and
>>>> VBUS, then we add/remove HCD/UDC conditionally and set PRTCAPDIR.
>>>>
>>>
>>> The catch is that on AM437x there is no way to get ID and VBUS events other
>>> than the OTG controller so we have to rely on the OTG controller for that. :(
>> 
>> okay, so AM437x can get OTG interrupts properly. That's fine. We can
>> still do everything we need using code that's already existing in dwc3
>> if we refactor it a bit and hook it up to the OTG IRQ handler.
>> 
>> Here's what we do:
>> 
>> * First we re-factor all necessary code around so the API for OTG/DRD
>>   is resumed to calling:
>> 
>> 	dwc3_add_udc(dwc);
>>         dwc3_del_udc(dwc);
>>         dwc3_add_hcd(dwc);
>>         dwc3_del_hcd(dwc);
>> 
>> the semantics of these should be easy to understand and you can
>> implement each in their respective host.c/gadget.c files.
>> 
>> * Second step is to modify our dwc3_init_mode() (or whatever that
>>   function was called, sorry, didn't check) to make sure we have
>>   something like:
>> 
>> 	case OTG:
>>         	dwc3_add_udc(dwc);
>>                 break;
>> 
>> We should *not* add HCD in this case yet.
>> 
>> * After that we add otg.c (or drd.c, no preference) and make that call
>>   dwc3_add_udc(dwc) and, also, provide
>>   dwc3_add_otg(dwc)/dwc3_del_otg(dwc) calls. Then patch the switch
>>   statement above to:
>> 
>> 	case OTG:
>>         	dwc3_add_otg(dwc);
>>                 break;
>> 
>> Note that at this point, this is simply a direct replacement of
>> dwc3_add_udc() to dwc3_add_otg(). This should maintain current behavior
>> (which is starting with peripheral mode by default), but it should also
>> add support for OTG interrupts to change the mode (from an interrupt
>> thead)
>> 
>> 	otg_isr()
>>         {
>> 
>> 		/* don't forget to remove preivous mode if necessary */
>>         	if (perimode)
>>                 	dwc3_add_udc(dwc);
>>                 else
>>                 	dwc3_add_hcd(dwc);
>> 	}
>> 
>> * The next patch would be to choose default conditionally based on
>>   PERIMODE or whatever.
>> 
>> Of course, this is an oversimplified view of reality. You still need to
>> poke around at PRTCAPDIR, etc. But all this can, actually, be prototyped
>> using our "mode" debugfs file. Just make that call
>> dwc3_add/del_udc/hcd() apart from fiddling with PRTCAPDIR in GCTL.
>> 
>> Your first implementation could be just that. Refactoring what needs to
>> be refactored, then patching "mode" debugfs to work properly in that
>> case. Only add otg.c/drd.c after "mode" debugfs file is stable, because
>> then you know what needs to be taken into consideration.
>> 
>> Just to be clear, I'm not saying we should *ONLY* get the debugfs
>> interface for v4.12, I'm saying you should start with that and get that
>> stable and working properly (make an infinite loop constantly changing
>> modes and keep it running over the weekend) before you add support for
>> OTG interrupts, which could come in the same series ;-)
>> 
>
> Agree with you. Moreover I could get rid of OTG controller related code
> and have just debugfs and extcon implementation. We can add the OTG controller
> bits later.
>
> I agree with you on everything you said except using add/del_gadget_udc. :)
> I've explained why we can't use del_gadget_udc in the other thread
> but I'll explain it here again.
>
> 1) If we start in host role, usb_add_gadget_udc() won't be called. That means
> no UDC and user can't load a gadget driver. Typical applications need to have
> a gadget driver ready *before* the peripheral mode starts so that it can
> enumerate immediately.

that has changed since you started writing this series :-) gadget
drivers are kept in pending list until a UDC is around. I'll get
information on that tomorrow, if you require.

> 2) If we use usb_del_gadget_udc() when switching to host mode and
> usb_add_gadget_udc() when switching back to peripheral mode, the previously
> loaded gadget driver will not be assigned to this UDC. User has to unload
> and reload the gadget driver.

that should not be the case anymore, if it is we have a bug in udc-core

> 3) All this becomes even more complex for configfs based gadget driver.
>
> So using stop/start gadget is a much simpler solution really as UDC software
> side of things remain unchanged and the gadget driver can persist between
> role switches.

I hadn't considered configfs, I'll try this out tomorrow as well.

-- 
balbi

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Roger Quadros]

On 29/03/17 16:21, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>>> Roger Quadros <rogerq@ti.com> writes:
>>>>> Roger Quadros <rogerq@ti.com> writes:
>>>>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode
>>>>>> when we're operating in dual-role.
>>>>>
>>>>> yeah, that's not a quirk. DRA7 supports OTGv2, not OTGv3. There was no
>>>>> USB3 when OTGv2 was written.
>>>>>
>>>>> DRA7 just shouldn't use OTG core altogether. In fact, this is the very
>>>>> thing I've been saying for a long time. Make the simplest implementation
>>>>> possible. The dead simple, does-one-thing-only sort of implementation.
>>>>>
>>>>> All we need for Dual-Role (without OTG extras) is some input for ID and
>>>>> VBUS, then we add/remove HCD/UDC conditionally and set PRTCAPDIR.
>>>>>
>>>>
>>>> The catch is that on AM437x there is no way to get ID and VBUS events other
>>>> than the OTG controller so we have to rely on the OTG controller for that. :(
>>>
>>> okay, so AM437x can get OTG interrupts properly. That's fine. We can
>>> still do everything we need using code that's already existing in dwc3
>>> if we refactor it a bit and hook it up to the OTG IRQ handler.
>>>
>>> Here's what we do:
>>>
>>> * First we re-factor all necessary code around so the API for OTG/DRD
>>>   is resumed to calling:
>>>
>>> 	dwc3_add_udc(dwc);
>>>         dwc3_del_udc(dwc);
>>>         dwc3_add_hcd(dwc);
>>>         dwc3_del_hcd(dwc);
>>>
>>> the semantics of these should be easy to understand and you can
>>> implement each in their respective host.c/gadget.c files.
>>>
>>> * Second step is to modify our dwc3_init_mode() (or whatever that
>>>   function was called, sorry, didn't check) to make sure we have
>>>   something like:
>>>
>>> 	case OTG:
>>>         	dwc3_add_udc(dwc);
>>>                 break;
>>>
>>> We should *not* add HCD in this case yet.
>>>
>>> * After that we add otg.c (or drd.c, no preference) and make that call
>>>   dwc3_add_udc(dwc) and, also, provide
>>>   dwc3_add_otg(dwc)/dwc3_del_otg(dwc) calls. Then patch the switch
>>>   statement above to:
>>>
>>> 	case OTG:
>>>         	dwc3_add_otg(dwc);
>>>                 break;
>>>
>>> Note that at this point, this is simply a direct replacement of
>>> dwc3_add_udc() to dwc3_add_otg(). This should maintain current behavior
>>> (which is starting with peripheral mode by default), but it should also
>>> add support for OTG interrupts to change the mode (from an interrupt
>>> thead)
>>>
>>> 	otg_isr()
>>>         {
>>>
>>> 		/* don't forget to remove preivous mode if necessary */
>>>         	if (perimode)
>>>                 	dwc3_add_udc(dwc);
>>>                 else
>>>                 	dwc3_add_hcd(dwc);
>>> 	}
>>>
>>> * The next patch would be to choose default conditionally based on
>>>   PERIMODE or whatever.
>>>
>>> Of course, this is an oversimplified view of reality. You still need to
>>> poke around at PRTCAPDIR, etc. But all this can, actually, be prototyped
>>> using our "mode" debugfs file. Just make that call
>>> dwc3_add/del_udc/hcd() apart from fiddling with PRTCAPDIR in GCTL.
>>>
>>> Your first implementation could be just that. Refactoring what needs to
>>> be refactored, then patching "mode" debugfs to work properly in that
>>> case. Only add otg.c/drd.c after "mode" debugfs file is stable, because
>>> then you know what needs to be taken into consideration.
>>>
>>> Just to be clear, I'm not saying we should *ONLY* get the debugfs
>>> interface for v4.12, I'm saying you should start with that and get that
>>> stable and working properly (make an infinite loop constantly changing
>>> modes and keep it running over the weekend) before you add support for
>>> OTG interrupts, which could come in the same series ;-)
>>>
>>
>> Agree with you. Moreover I could get rid of OTG controller related code
>> and have just debugfs and extcon implementation. We can add the OTG controller
>> bits later.
>>
>> I agree with you on everything you said except using add/del_gadget_udc. :)
>> I've explained why we can't use del_gadget_udc in the other thread
>> but I'll explain it here again.
>>
>> 1) If we start in host role, usb_add_gadget_udc() won't be called. That means
>> no UDC and user can't load a gadget driver. Typical applications need to have
>> a gadget driver ready *before* the peripheral mode starts so that it can
>> enumerate immediately.
> 
> that has changed since you started writing this series :-) gadget
> drivers are kept in pending list until a UDC is around. I'll get
> information on that tomorrow, if you require.

"until a UDC is around" is the key point. If we never call usb_add_gadget_udc()
or we call usb_del_gadget_udc() then the UDC is not around right?
> 
>> 2) If we use usb_del_gadget_udc() when switching to host mode and
>> usb_add_gadget_udc() when switching back to peripheral mode, the previously
>> loaded gadget driver will not be assigned to this UDC. User has to unload
>> and reload the gadget driver.
> 
> that should not be the case anymore, if it is we have a bug in udc-core

OK. good to know.
> 
>> 3) All this becomes even more complex for configfs based gadget driver.
>>
>> So using stop/start gadget is a much simpler solution really as UDC software
>> side of things remain unchanged and the gadget driver can persist between
>> role switches.
> 
> I hadn't considered configfs, I'll try this out tomorrow as well.
> 
Al-right, thanks.

cheers,
-roger

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

[Roger Quadros]

Hi,

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

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

[Felipe Balbi]

Hi

Roger Quadros <rogerq@ti.com> writes:
>>>> 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. :)

fair enough

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

I see...

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

that's true, sorry.

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

man, there's really no way to mask OTG events. This can be bad :-)

John, can you confirm if there's really no way of masking OTG events
without loosing them?

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

heh, right :-) What I mean is that you might enable an interrupt event
which you don't clear, because you don't support it or don't handle it,
or whatever.

Reserved bits might become non-reserved in the future and so on.

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

looks like we have another problem which needs to be investigated ;-)

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

okay

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

exactly :-)

> Need to think of a better way how to tackle this.

okay

-- 
balbi

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 517 bytes --]


Hi,

Roger Quadros <rogerq@ti.com> writes:
>>> 3) All this becomes even more complex for configfs based gadget driver.
>>>
>>> So using stop/start gadget is a much simpler solution really as UDC software
>>> side of things remain unchanged and the gadget driver can persist between
>>> role switches.
>> 
>> I hadn't considered configfs, I'll try this out tomorrow as well.
>> 
> Al-right, thanks.

just tested with g_zero.ko and a configfs-based f_mass_storage
gadget. All works fine.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Roger Quadros]

On 30/03/17 12:32, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>>>> 3) All this becomes even more complex for configfs based gadget driver.
>>>>
>>>> So using stop/start gadget is a much simpler solution really as UDC software
>>>> side of things remain unchanged and the gadget driver can persist between
>>>> role switches.
>>>
>>> I hadn't considered configfs, I'll try this out tomorrow as well.
>>>
>> Al-right, thanks.
> 
> just tested with g_zero.ko and a configfs-based f_mass_storage
> gadget. All works fine.
> 
Good to know. Thanks for the test.

cheers,
-roger

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Roger Quadros]

Hi,

On 29/03/17 13:32, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>>> Roger Quadros <rogerq@ti.com> writes:
>>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode
>>>> when we're operating in dual-role.
>>>
>>> yeah, that's not a quirk. DRA7 supports OTGv2, not OTGv3. There was no
>>> USB3 when OTGv2 was written.
>>>
>>> DRA7 just shouldn't use OTG core altogether. In fact, this is the very
>>> thing I've been saying for a long time. Make the simplest implementation
>>> possible. The dead simple, does-one-thing-only sort of implementation.
>>>
>>> All we need for Dual-Role (without OTG extras) is some input for ID and
>>> VBUS, then we add/remove HCD/UDC conditionally and set PRTCAPDIR.
>>>
>>
>> The catch is that on AM437x there is no way to get ID and VBUS events other
>> than the OTG controller so we have to rely on the OTG controller for that. :(
> 
> okay, so AM437x can get OTG interrupts properly. That's fine. We can
> still do everything we need using code that's already existing in dwc3
> if we refactor it a bit and hook it up to the OTG IRQ handler.
> 
> Here's what we do:
> 
> * First we re-factor all necessary code around so the API for OTG/DRD
>   is resumed to calling:
> 
> 	dwc3_add_udc(dwc);
>         dwc3_del_udc(dwc);
>         dwc3_add_hcd(dwc);
>         dwc3_del_hcd(dwc);

Why do we need these new APIs? don't these suffice?
	dwc3_gadget_init(dwc);
	dwc3_gadget_exit(dwc);
	dwc3_host_init(dwc);
	dwc3_host_exit(dwc);
> 
> the semantics of these should be easy to understand and you can
> implement each in their respective host.c/gadget.c files.
> 
> * Second step is to modify our dwc3_init_mode() (or whatever that
>   function was called, sorry, didn't check) to make sure we have
>   something like:
> 
> 	case OTG:
>         	dwc3_add_udc(dwc);
>                 break;
> 
> We should *not* add HCD in this case yet.
> 
> * After that we add otg.c (or drd.c, no preference) and make that call
>   dwc3_add_udc(dwc) and, also, provide
>   dwc3_add_otg(dwc)/dwc3_del_otg(dwc) calls. Then patch the switch
>   statement above to:
> 
> 	case OTG:
>         	dwc3_add_otg(dwc);
>                 break;
> 
> Note that at this point, this is simply a direct replacement of
> dwc3_add_udc() to dwc3_add_otg(). This should maintain current behavior
> (which is starting with peripheral mode by default), but it should also
> add support for OTG interrupts to change the mode (from an interrupt
> thead)
> 
> 	otg_isr()
>         {
> 
> 		/* don't forget to remove preivous mode if necessary */
>         	if (perimode)
>                 	dwc3_add_udc(dwc);
>                 else
>                 	dwc3_add_hcd(dwc);
> 	}
> 
> * The next patch would be to choose default conditionally based on
>   PERIMODE or whatever.
> 
> Of course, this is an oversimplified view of reality. You still need to
> poke around at PRTCAPDIR, etc. But all this can, actually, be prototyped
> using our "mode" debugfs file. Just make that call
> dwc3_add/del_udc/hcd() apart from fiddling with PRTCAPDIR in GCTL.

We also need to ensure that system suspend/resume doesn't break.
Mainly if we suspend/resume with UDC removed.
> 
> Your first implementation could be just that. Refactoring what needs to
> be refactored, then patching "mode" debugfs to work properly in that
> case. Only add otg.c/drd.c after "mode" debugfs file is stable, because
> then you know what needs to be taken into consideration.
> 
> Just to be clear, I'm not saying we should *ONLY* get the debugfs
> interface for v4.12, I'm saying you should start with that and get that
> stable and working properly (make an infinite loop constantly changing
> modes and keep it running over the weekend) before you add support for
> OTG interrupts, which could come in the same series ;-)
> 

Just to clarify debugfs mode behaviour.

Currently it is just changing PRTCAPDIR. What we need to do is that if
dr_mode == "otg", then we call dwc3_host/gadget_init/exit() accordingly as well.

Does this make sense?

cheers,
-roger

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 4359 bytes --]


Hi,

Roger Quadros <rogerq@ti.com> writes:
>>>> Roger Quadros <rogerq@ti.com> writes:
>>>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode
>>>>> when we're operating in dual-role.
>>>>
>>>> yeah, that's not a quirk. DRA7 supports OTGv2, not OTGv3. There was no
>>>> USB3 when OTGv2 was written.
>>>>
>>>> DRA7 just shouldn't use OTG core altogether. In fact, this is the very
>>>> thing I've been saying for a long time. Make the simplest implementation
>>>> possible. The dead simple, does-one-thing-only sort of implementation.
>>>>
>>>> All we need for Dual-Role (without OTG extras) is some input for ID and
>>>> VBUS, then we add/remove HCD/UDC conditionally and set PRTCAPDIR.
>>>>
>>>
>>> The catch is that on AM437x there is no way to get ID and VBUS events other
>>> than the OTG controller so we have to rely on the OTG controller for that. :(
>> 
>> okay, so AM437x can get OTG interrupts properly. That's fine. We can
>> still do everything we need using code that's already existing in dwc3
>> if we refactor it a bit and hook it up to the OTG IRQ handler.
>> 
>> Here's what we do:
>> 
>> * First we re-factor all necessary code around so the API for OTG/DRD
>>   is resumed to calling:
>> 
>> 	dwc3_add_udc(dwc);
>>         dwc3_del_udc(dwc);
>>         dwc3_add_hcd(dwc);
>>         dwc3_del_hcd(dwc);
>
> Why do we need these new APIs? don't these suffice?
> 	dwc3_gadget_init(dwc);
> 	dwc3_gadget_exit(dwc);
> 	dwc3_host_init(dwc);
> 	dwc3_host_exit(dwc);

well, if they do what we want, sure. They suffice.

>> the semantics of these should be easy to understand and you can
>> implement each in their respective host.c/gadget.c files.
>> 
>> * Second step is to modify our dwc3_init_mode() (or whatever that
>>   function was called, sorry, didn't check) to make sure we have
>>   something like:
>> 
>> 	case OTG:
>>         	dwc3_add_udc(dwc);
>>                 break;
>> 
>> We should *not* add HCD in this case yet.
>> 
>> * After that we add otg.c (or drd.c, no preference) and make that call
>>   dwc3_add_udc(dwc) and, also, provide
>>   dwc3_add_otg(dwc)/dwc3_del_otg(dwc) calls. Then patch the switch
>>   statement above to:
>> 
>> 	case OTG:
>>         	dwc3_add_otg(dwc);
>>                 break;
>> 
>> Note that at this point, this is simply a direct replacement of
>> dwc3_add_udc() to dwc3_add_otg(). This should maintain current behavior
>> (which is starting with peripheral mode by default), but it should also
>> add support for OTG interrupts to change the mode (from an interrupt
>> thead)
>> 
>> 	otg_isr()
>>         {
>> 
>> 		/* don't forget to remove preivous mode if necessary */
>>         	if (perimode)
>>                 	dwc3_add_udc(dwc);
>>                 else
>>                 	dwc3_add_hcd(dwc);
>> 	}
>> 
>> * The next patch would be to choose default conditionally based on
>>   PERIMODE or whatever.
>> 
>> Of course, this is an oversimplified view of reality. You still need to
>> poke around at PRTCAPDIR, etc. But all this can, actually, be prototyped
>> using our "mode" debugfs file. Just make that call
>> dwc3_add/del_udc/hcd() apart from fiddling with PRTCAPDIR in GCTL.
>
> We also need to ensure that system suspend/resume doesn't break.
> Mainly if we suspend/resume with UDC removed.

right, why would it break in that case? I'm missing something...

>> Your first implementation could be just that. Refactoring what needs to
>> be refactored, then patching "mode" debugfs to work properly in that
>> case. Only add otg.c/drd.c after "mode" debugfs file is stable, because
>> then you know what needs to be taken into consideration.
>> 
>> Just to be clear, I'm not saying we should *ONLY* get the debugfs
>> interface for v4.12, I'm saying you should start with that and get that
>> stable and working properly (make an infinite loop constantly changing
>> modes and keep it running over the weekend) before you add support for
>> OTG interrupts, which could come in the same series ;-)
>> 
>
> Just to clarify debugfs mode behaviour.
>
> Currently it is just changing PRTCAPDIR. What we need to do is that if
> dr_mode == "otg", then we call dwc3_host/gadget_init/exit() accordingly as well.
>
> Does this make sense?

it does.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Roger Quadros]

+Mathias

On 31/03/17 10:46, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>>>>> Roger Quadros <rogerq@ti.com> writes:
>>>>>> dra7 OTG core limits the host controller to USB2.0 (high-speed) mode
>>>>>> when we're operating in dual-role.
>>>>>
>>>>> yeah, that's not a quirk. DRA7 supports OTGv2, not OTGv3. There was no
>>>>> USB3 when OTGv2 was written.
>>>>>
>>>>> DRA7 just shouldn't use OTG core altogether. In fact, this is the very
>>>>> thing I've been saying for a long time. Make the simplest implementation
>>>>> possible. The dead simple, does-one-thing-only sort of implementation.
>>>>>
>>>>> All we need for Dual-Role (without OTG extras) is some input for ID and
>>>>> VBUS, then we add/remove HCD/UDC conditionally and set PRTCAPDIR.
>>>>>
>>>>
>>>> The catch is that on AM437x there is no way to get ID and VBUS events other
>>>> than the OTG controller so we have to rely on the OTG controller for that. :(
>>>
>>> okay, so AM437x can get OTG interrupts properly. That's fine. We can
>>> still do everything we need using code that's already existing in dwc3
>>> if we refactor it a bit and hook it up to the OTG IRQ handler.
>>>
>>> Here's what we do:
>>>
>>> * First we re-factor all necessary code around so the API for OTG/DRD
>>>   is resumed to calling:
>>>
>>> 	dwc3_add_udc(dwc);
>>>         dwc3_del_udc(dwc);
>>>         dwc3_add_hcd(dwc);
>>>         dwc3_del_hcd(dwc);
>>
>> Why do we need these new APIs? don't these suffice?
>> 	dwc3_gadget_init(dwc);
>> 	dwc3_gadget_exit(dwc);
>> 	dwc3_host_init(dwc);
>> 	dwc3_host_exit(dwc);
> 
> well, if they do what we want, sure. They suffice.
> 
>>> the semantics of these should be easy to understand and you can
>>> implement each in their respective host.c/gadget.c files.
>>>
>>> * Second step is to modify our dwc3_init_mode() (or whatever that
>>>   function was called, sorry, didn't check) to make sure we have
>>>   something like:
>>>
>>> 	case OTG:
>>>         	dwc3_add_udc(dwc);
>>>                 break;
>>>
>>> We should *not* add HCD in this case yet.
>>>
>>> * After that we add otg.c (or drd.c, no preference) and make that call
>>>   dwc3_add_udc(dwc) and, also, provide
>>>   dwc3_add_otg(dwc)/dwc3_del_otg(dwc) calls. Then patch the switch
>>>   statement above to:
>>>
>>> 	case OTG:
>>>         	dwc3_add_otg(dwc);
>>>                 break;
>>>
>>> Note that at this point, this is simply a direct replacement of
>>> dwc3_add_udc() to dwc3_add_otg(). This should maintain current behavior
>>> (which is starting with peripheral mode by default), but it should also
>>> add support for OTG interrupts to change the mode (from an interrupt
>>> thead)
>>>
>>> 	otg_isr()
>>>         {
>>>
>>> 		/* don't forget to remove preivous mode if necessary */
>>>         	if (perimode)
>>>                 	dwc3_add_udc(dwc);
>>>                 else
>>>                 	dwc3_add_hcd(dwc);
>>> 	}
>>>
>>> * The next patch would be to choose default conditionally based on
>>>   PERIMODE or whatever.
>>>
>>> Of course, this is an oversimplified view of reality. You still need to
>>> poke around at PRTCAPDIR, etc. But all this can, actually, be prototyped
>>> using our "mode" debugfs file. Just make that call
>>> dwc3_add/del_udc/hcd() apart from fiddling with PRTCAPDIR in GCTL.
>>
>> We also need to ensure that system suspend/resume doesn't break.
>> Mainly if we suspend/resume with UDC removed.
> 
> right, why would it break in that case? I'm missing something...
> 
>>> Your first implementation could be just that. Refactoring what needs to
>>> be refactored, then patching "mode" debugfs to work properly in that
>>> case. Only add otg.c/drd.c after "mode" debugfs file is stable, because
>>> then you know what needs to be taken into consideration.
>>>
>>> Just to be clear, I'm not saying we should *ONLY* get the debugfs
>>> interface for v4.12, I'm saying you should start with that and get that
>>> stable and working properly (make an infinite loop constantly changing
>>> modes and keep it running over the weekend) before you add support for
>>> OTG interrupts, which could come in the same series ;-)
>>>
>>
>> Just to clarify debugfs mode behaviour.
>>
>> Currently it is just changing PRTCAPDIR. What we need to do is that if
>> dr_mode == "otg", then we call dwc3_host/gadget_init/exit() accordingly as well.
>>
>> Does this make sense?
> 
> it does.
> 

OK. Below is a patch that allows us to use debugfs/mode to do the role switch.
Switching from device to host worked fine but I get the following error when
switching from host to device.

https://hastebin.com/liluqosewe.xml

cheers,
-roger

---
>From 50c49f18474b388d10533eb9f6d04f454fabf687 Mon Sep 17 00:00:00 2001
From: Roger Quadros <rogerq@ti.com>
Date: Fri, 31 Mar 2017 12:54:13 +0300
Subject: [PATCH] usb: dwc3: make role-switching work with debugfs/mode

If dr_mode == "otg", we start by default in PERIPHERAL mode.
Keep track of current role in "current_dr_role" whenever dwc3_set_mode()
is called.

When debugfs/mode is changed AND we're in dual-role mode,
handle the switch by stopping and starting the respective
host/gadget controllers.

Signed-off-by: Roger Quadros <rogerq@ti.com>
---
 drivers/usb/dwc3/core.c    | 38 +++++++++++++++++++++++------------
 drivers/usb/dwc3/core.h    |  2 ++
 drivers/usb/dwc3/debugfs.c | 49 +++++++++++++++++++++++++++++++++++++++-------
 3 files changed, 70 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 369bab1..e2d36ba 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -108,6 +108,8 @@ void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
 	reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
 	reg |= DWC3_GCTL_PRTCAPDIR(mode);
 	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
+
+	dwc->current_dr_role = mode;
 }
 
 u32 dwc3_core_fifo_space(struct dwc3_ep *dep, u8 type)
@@ -862,13 +864,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
 		}
 		break;
 	case USB_DR_MODE_OTG:
-		ret = dwc3_host_init(dwc);
-		if (ret) {
-			if (ret != -EPROBE_DEFER)
-				dev_err(dev, "failed to initialize host\n");
-			return ret;
-		}
-
+		/* start in peripheral role by default */
+		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
 		ret = dwc3_gadget_init(dwc);
 		if (ret) {
 			if (ret != -EPROBE_DEFER)
@@ -894,8 +891,11 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc)
 		dwc3_host_exit(dwc);
 		break;
 	case USB_DR_MODE_OTG:
-		dwc3_host_exit(dwc);
-		dwc3_gadget_exit(dwc);
+		/* role might have changed since start */
+		if (dwc->current_dr_role ==  DWC3_GCTL_PRTCAP_DEVICE)
+			dwc3_gadget_exit(dwc);
+		else if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)
+			dwc3_host_exit(dwc);
 		break;
 	default:
 		/* do nothing */
@@ -1209,11 +1209,18 @@ static int dwc3_suspend_common(struct dwc3 *dwc)
 
 	switch (dwc->dr_mode) {
 	case USB_DR_MODE_PERIPHERAL:
-	case USB_DR_MODE_OTG:
 		spin_lock_irqsave(&dwc->lock, flags);
 		dwc3_gadget_suspend(dwc);
 		spin_unlock_irqrestore(&dwc->lock, flags);
 		break;
+	case USB_DR_MODE_OTG:
+		/* gadget might not be always present */
+		if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE) {
+			spin_lock_irqsave(&dwc->lock, flags);
+			dwc3_gadget_suspend(dwc);
+			spin_unlock_irqrestore(&dwc->lock, flags);
+		}
+		break;
 	case USB_DR_MODE_HOST:
 	default:
 		/* do nothing */
@@ -1236,11 +1243,18 @@ static int dwc3_resume_common(struct dwc3 *dwc)
 
 	switch (dwc->dr_mode) {
 	case USB_DR_MODE_PERIPHERAL:
-	case USB_DR_MODE_OTG:
 		spin_lock_irqsave(&dwc->lock, flags);
 		dwc3_gadget_resume(dwc);
 		spin_unlock_irqrestore(&dwc->lock, flags);
-		/* FALLTHROUGH */
+		break;
+	case USB_DR_MODE_OTG:
+		/* gadget might not be always present */
+		if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_DEVICE) {
+			spin_lock_irqsave(&dwc->lock, flags);
+			dwc3_gadget_resume(dwc);
+			spin_unlock_irqrestore(&dwc->lock, flags);
+		}
+		break;
 	case USB_DR_MODE_HOST:
 	default:
 		/* do nothing */
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 7ffdee5..f45ff44 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -785,6 +785,7 @@ struct dwc3_scratchpad_array {
  * @maximum_speed: maximum speed requested (mainly for testing purposes)
  * @revision: revision register contents
  * @dr_mode: requested mode of operation
+ * @current_dr_role: current role of operation when in dual-role mode
  * @hsphy_mode: UTMI phy mode, one of following:
  *		- USBPHY_INTERFACE_MODE_UTMI
  *		- USBPHY_INTERFACE_MODE_UTMIW
@@ -901,6 +902,7 @@ struct dwc3 {
 	size_t			regs_size;
 
 	enum usb_dr_mode	dr_mode;
+	u32			current_dr_role;
 	enum usb_phy_interface	hsphy_mode;
 
 	u32			fladj;
diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
index 31926dd..a101b14 100644
--- a/drivers/usb/dwc3/debugfs.c
+++ b/drivers/usb/dwc3/debugfs.c
@@ -327,19 +327,54 @@ static ssize_t dwc3_mode_write(struct file *file,
 		return -EFAULT;
 
 	if (!strncmp(buf, "host", 4))
-		mode |= DWC3_GCTL_PRTCAP_HOST;
+		mode = DWC3_GCTL_PRTCAP_HOST;
 
 	if (!strncmp(buf, "device", 6))
-		mode |= DWC3_GCTL_PRTCAP_DEVICE;
+		mode = DWC3_GCTL_PRTCAP_DEVICE;
 
 	if (!strncmp(buf, "otg", 3))
-		mode |= DWC3_GCTL_PRTCAP_OTG;
+		mode = DWC3_GCTL_PRTCAP_OTG;
 
-	if (mode) {
-		spin_lock_irqsave(&dwc->lock, flags);
-		dwc3_set_mode(dwc, mode);
-		spin_unlock_irqrestore(&dwc->lock, flags);
+	if (!mode)
+		return -EINVAL;
+
+	if (mode == dwc->current_dr_role)
+		goto exit;
+
+	/* prevent role switching if we're not dual-role */
+	if (dwc->dr_mode != USB_DR_MODE_OTG)
+		return -EINVAL;
+
+	/* stop old role */
+	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)
+	switch (dwc->current_dr_role) {
+	case DWC3_GCTL_PRTCAP_HOST:
+		dwc3_host_exit(dwc);
+		break;
+	case DWC3_GCTL_PRTCAP_DEVICE:
+		dwc3_gadget_exit(dwc);
+		break;
+	default:
+		break;
+	}
+
+	/* switch PRTCAP mode. updates current_dr_role */
+	spin_lock_irqsave(&dwc->lock, flags);
+	dwc3_set_mode(dwc, mode);
+	spin_unlock_irqrestore(&dwc->lock, flags);
+
+	/* start new role */
+	switch (dwc->current_dr_role) {
+	case DWC3_GCTL_PRTCAP_HOST:
+		dwc3_host_init(dwc);
+		break;
+	case DWC3_GCTL_PRTCAP_DEVICE:
+		dwc3_gadget_init(dwc);
+		break;
+	default:
+		break;
 	}
+exit:
 	return count;
 }
 
-- 
2.7.4

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 5926 bytes --]


Hi,

Roger Quadros <rogerq@ti.com> writes:
>>>> Your first implementation could be just that. Refactoring what needs to
>>>> be refactored, then patching "mode" debugfs to work properly in that
>>>> case. Only add otg.c/drd.c after "mode" debugfs file is stable, because
>>>> then you know what needs to be taken into consideration.
>>>>
>>>> Just to be clear, I'm not saying we should *ONLY* get the debugfs
>>>> interface for v4.12, I'm saying you should start with that and get that
>>>> stable and working properly (make an infinite loop constantly changing
>>>> modes and keep it running over the weekend) before you add support for
>>>> OTG interrupts, which could come in the same series ;-)
>>>>
>>>
>>> Just to clarify debugfs mode behaviour.
>>>
>>> Currently it is just changing PRTCAPDIR. What we need to do is that if
>>> dr_mode == "otg", then we call dwc3_host/gadget_init/exit() accordingly as well.
>>>
>>> Does this make sense?
>> 
>> it does.
>> 
>
> OK. Below is a patch that allows us to use debugfs/mode to do the role switch.
> Switching from device to host worked fine but I get the following error when
> switching from host to device.
>
> https://hastebin.com/liluqosewe.xml
>
> cheers,
> -roger
>
> ---
> From 50c49f18474b388d10533eb9f6d04f454fabf687 Mon Sep 17 00:00:00 2001
> From: Roger Quadros <rogerq@ti.com>
> Date: Fri, 31 Mar 2017 12:54:13 +0300
> Subject: [PATCH] usb: dwc3: make role-switching work with debugfs/mode
>
> If dr_mode == "otg", we start by default in PERIPHERAL mode.
> Keep track of current role in "current_dr_role" whenever dwc3_set_mode()
> is called.
>
> When debugfs/mode is changed AND we're in dual-role mode,
> handle the switch by stopping and starting the respective
> host/gadget controllers.
>
> Signed-off-by: Roger Quadros <rogerq@ti.com>

I'm assuming you also plan on breaking this down further ;-)

> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 369bab1..e2d36ba 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -108,6 +108,8 @@ void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
>  	reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
>  	reg |= DWC3_GCTL_PRTCAPDIR(mode);
>  	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> +
> +	dwc->current_dr_role = mode;
>  }
>  
>  u32 dwc3_core_fifo_space(struct dwc3_ep *dep, u8 type)
> @@ -862,13 +864,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>  		}
>  		break;
>  	case USB_DR_MODE_OTG:
> -		ret = dwc3_host_init(dwc);
> -		if (ret) {
> -			if (ret != -EPROBE_DEFER)
> -				dev_err(dev, "failed to initialize host\n");
> -			return ret;
> -		}
> -
> +		/* start in peripheral role by default */
> +		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
>  		ret = dwc3_gadget_init(dwc);
>  		if (ret) {
>  			if (ret != -EPROBE_DEFER)
> @@ -894,8 +891,11 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc)
>  		dwc3_host_exit(dwc);
>  		break;
>  	case USB_DR_MODE_OTG:
> -		dwc3_host_exit(dwc);
> -		dwc3_gadget_exit(dwc);
> +		/* role might have changed since start */
> +		if (dwc->current_dr_role ==  DWC3_GCTL_PRTCAP_DEVICE)
> +			dwc3_gadget_exit(dwc);
> +		else if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)
> +			dwc3_host_exit(dwc);

how about patching the respective exit/init functions with something
like:

if (dwc->current_dr_role != $my_expected_role)
	return 0;

then you can call them without any checks.

> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
> index 31926dd..a101b14 100644
> --- a/drivers/usb/dwc3/debugfs.c
> +++ b/drivers/usb/dwc3/debugfs.c
> @@ -327,19 +327,54 @@ static ssize_t dwc3_mode_write(struct file *file,
>  		return -EFAULT;
>  
>  	if (!strncmp(buf, "host", 4))
> -		mode |= DWC3_GCTL_PRTCAP_HOST;
> +		mode = DWC3_GCTL_PRTCAP_HOST;
>  
>  	if (!strncmp(buf, "device", 6))
> -		mode |= DWC3_GCTL_PRTCAP_DEVICE;
> +		mode = DWC3_GCTL_PRTCAP_DEVICE;
>  
>  	if (!strncmp(buf, "otg", 3))
> -		mode |= DWC3_GCTL_PRTCAP_OTG;
> +		mode = DWC3_GCTL_PRTCAP_OTG;
>  
> -	if (mode) {
> -		spin_lock_irqsave(&dwc->lock, flags);
> -		dwc3_set_mode(dwc, mode);
> -		spin_unlock_irqrestore(&dwc->lock, flags);
> +	if (!mode)
> +		return -EINVAL;
> +
> +	if (mode == dwc->current_dr_role)
> +		goto exit;
> +
> +	/* prevent role switching if we're not dual-role */
> +	if (dwc->dr_mode != USB_DR_MODE_OTG)
> +		return -EINVAL;
> +
> +	/* stop old role */
> +	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)

is this your bug? This switch statement only executes when we're in host
mode. This means that when you switch to peripheral, you don't exit
host. Then when you switch back from peripheral to host, you're going to
add the same platform_device again. We're going to have TWO xHCI
platform device with the exact same name. When you finally switch again
from host to device, then you have issues.

Can you confirm?

> +	switch (dwc->current_dr_role) {
> +	case DWC3_GCTL_PRTCAP_HOST:
> +		dwc3_host_exit(dwc);
> +		break;
> +	case DWC3_GCTL_PRTCAP_DEVICE:
> +		dwc3_gadget_exit(dwc);
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	/* switch PRTCAP mode. updates current_dr_role */
> +	spin_lock_irqsave(&dwc->lock, flags);
> +	dwc3_set_mode(dwc, mode);
> +	spin_unlock_irqrestore(&dwc->lock, flags);
> +
> +	/* start new role */
> +	switch (dwc->current_dr_role) {
> +	case DWC3_GCTL_PRTCAP_HOST:
> +		dwc3_host_init(dwc);
> +		break;
> +	case DWC3_GCTL_PRTCAP_DEVICE:
> +		dwc3_gadget_init(dwc);
> +		break;
> +	default:
> +		break;
>  	}
> +exit:
>  	return count;
>  }
>  
> -- 
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Roger Quadros]

On 31/03/17 15:00, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros <rogerq@ti.com> writes:
>>>>> Your first implementation could be just that. Refactoring what needs to
>>>>> be refactored, then patching "mode" debugfs to work properly in that
>>>>> case. Only add otg.c/drd.c after "mode" debugfs file is stable, because
>>>>> then you know what needs to be taken into consideration.
>>>>>
>>>>> Just to be clear, I'm not saying we should *ONLY* get the debugfs
>>>>> interface for v4.12, I'm saying you should start with that and get that
>>>>> stable and working properly (make an infinite loop constantly changing
>>>>> modes and keep it running over the weekend) before you add support for
>>>>> OTG interrupts, which could come in the same series ;-)
>>>>>
>>>>
>>>> Just to clarify debugfs mode behaviour.
>>>>
>>>> Currently it is just changing PRTCAPDIR. What we need to do is that if
>>>> dr_mode == "otg", then we call dwc3_host/gadget_init/exit() accordingly as well.
>>>>
>>>> Does this make sense?
>>>
>>> it does.
>>>
>>
>> OK. Below is a patch that allows us to use debugfs/mode to do the role switch.
>> Switching from device to host worked fine but I get the following error when
>> switching from host to device.
>>
>> https://hastebin.com/liluqosewe.xml
>>
>> cheers,
>> -roger
>>
>> ---
>> From 50c49f18474b388d10533eb9f6d04f454fabf687 Mon Sep 17 00:00:00 2001
>> From: Roger Quadros <rogerq@ti.com>
>> Date: Fri, 31 Mar 2017 12:54:13 +0300
>> Subject: [PATCH] usb: dwc3: make role-switching work with debugfs/mode
>>
>> If dr_mode == "otg", we start by default in PERIPHERAL mode.
>> Keep track of current role in "current_dr_role" whenever dwc3_set_mode()
>> is called.
>>
>> When debugfs/mode is changed AND we're in dual-role mode,
>> handle the switch by stopping and starting the respective
>> host/gadget controllers.
>>
>> Signed-off-by: Roger Quadros <rogerq@ti.com>
> 
> I'm assuming you also plan on breaking this down further ;-)

Did you mean I must split this patch into smaller ones?

> 
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 369bab1..e2d36ba 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -108,6 +108,8 @@ void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
>>  	reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
>>  	reg |= DWC3_GCTL_PRTCAPDIR(mode);
>>  	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
>> +
>> +	dwc->current_dr_role = mode;
>>  }
>>  
>>  u32 dwc3_core_fifo_space(struct dwc3_ep *dep, u8 type)
>> @@ -862,13 +864,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>>  		}
>>  		break;
>>  	case USB_DR_MODE_OTG:
>> -		ret = dwc3_host_init(dwc);
>> -		if (ret) {
>> -			if (ret != -EPROBE_DEFER)
>> -				dev_err(dev, "failed to initialize host\n");
>> -			return ret;
>> -		}
>> -
>> +		/* start in peripheral role by default */
>> +		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
>>  		ret = dwc3_gadget_init(dwc);
>>  		if (ret) {
>>  			if (ret != -EPROBE_DEFER)
>> @@ -894,8 +891,11 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc)
>>  		dwc3_host_exit(dwc);
>>  		break;
>>  	case USB_DR_MODE_OTG:
>> -		dwc3_host_exit(dwc);
>> -		dwc3_gadget_exit(dwc);
>> +		/* role might have changed since start */
>> +		if (dwc->current_dr_role ==  DWC3_GCTL_PRTCAP_DEVICE)
>> +			dwc3_gadget_exit(dwc);
>> +		else if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)
>> +			dwc3_host_exit(dwc);
> 
> how about patching the respective exit/init functions with something
> like:
> 
> if (dwc->current_dr_role != $my_expected_role)
> 	return 0;
> 
> then you can call them without any checks.

OK.

> 
>> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
>> index 31926dd..a101b14 100644
>> --- a/drivers/usb/dwc3/debugfs.c
>> +++ b/drivers/usb/dwc3/debugfs.c
>> @@ -327,19 +327,54 @@ static ssize_t dwc3_mode_write(struct file *file,
>>  		return -EFAULT;
>>  
>>  	if (!strncmp(buf, "host", 4))
>> -		mode |= DWC3_GCTL_PRTCAP_HOST;
>> +		mode = DWC3_GCTL_PRTCAP_HOST;
>>  
>>  	if (!strncmp(buf, "device", 6))
>> -		mode |= DWC3_GCTL_PRTCAP_DEVICE;
>> +		mode = DWC3_GCTL_PRTCAP_DEVICE;
>>  
>>  	if (!strncmp(buf, "otg", 3))
>> -		mode |= DWC3_GCTL_PRTCAP_OTG;
>> +		mode = DWC3_GCTL_PRTCAP_OTG;
>>  
>> -	if (mode) {
>> -		spin_lock_irqsave(&dwc->lock, flags);
>> -		dwc3_set_mode(dwc, mode);
>> -		spin_unlock_irqrestore(&dwc->lock, flags);
>> +	if (!mode)
>> +		return -EINVAL;
>> +
>> +	if (mode == dwc->current_dr_role)
>> +		goto exit;
>> +
>> +	/* prevent role switching if we're not dual-role */
>> +	if (dwc->dr_mode != USB_DR_MODE_OTG)
>> +		return -EINVAL;
>> +
>> +	/* stop old role */
>> +	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)
> 
> is this your bug? This switch statement only executes when we're in host
> mode. This means that when you switch to peripheral, you don't exit
> host. Then when you switch back from peripheral to host, you're going to
> add the same platform_device again. We're going to have TWO xHCI
> platform device with the exact same name. When you finally switch again
> from host to device, then you have issues.
> 
> Can you confirm?

That was a bug but I still see the issue although only when a mass storage
device was plugged in.

I see this other new issue when not using a mass storage device.

root@rockdesk:/sys/kernel/debug/48890000.usb# echo device > mode
[  218.226104] xhci-hcd xhci-hcd.1.auto: remove, state 4
[  218.231822] usb usb4: USB disconnect, device number 1
[  218.246973] xhci-hcd xhci-hcd.1.auto: USB bus 4 deregistered
[  218.252961] xhci-hcd xhci-hcd.1.auto: remove, state 4
[  218.258347] usb usb3: USB disconnect, device number 1
[  218.265858] xhci-hcd xhci-hcd.1.auto: USB bus 3 deregistered
[  218.274312] dwc3 48890000.usb: changing max_speed on rev 5533202a
[  218.282108] kobject (ed120208): tried to init an initialized object, something is seriously wrong.
[  218.291553] CPU: 1 PID: 2025 Comm: bash Not tainted 4.11.0-rc4-00004-g559b2c9 #1285
[  218.299590] Hardware name: Generic DRA74X (Flattened Device Tree)
[  218.306002] [<c01101ec>] (unwind_backtrace) from [<c010c328>] (show_stack+0x10/0x14)
[  218.314133] [<c010c328>] (show_stack) from [<c04b46b8>] (dump_stack+0xac/0xe0)
[  218.321716] [<c04b46b8>] (dump_stack) from [<c04b5dc8>] (kobject_init+0x78/0x94)
[  218.329484] [<c04b5dc8>] (kobject_init) from [<c0570c98>] (device_initialize+0x20/0xe4)
[  218.337891] [<c0570c98>] (device_initialize) from [<c0573280>] (device_register+0xc/0x18)
[  218.346502] [<c0573280>] (device_register) from [<bf298ce8>] (usb_add_gadget_udc_release+0x88/0x1e8 [udc_core])
[  218.357153] [<bf298ce8>] (usb_add_gadget_udc_release [udc_core]) from [<bf2c6d68>] (dwc3_gadget_init+0x278/0x6c0 [dwc3])
[  218.368603] [<bf2c6d68>] (dwc3_gadget_init [dwc3]) from [<bf2cab54>] (dwc3_mode_write+0x178/0x1c4 [dwc3])
[  218.378668] [<bf2cab54>] (dwc3_mode_write [dwc3]) from [<c04557c4>] (full_proxy_write+0x4c/0x64)
[  218.387897] [<c04557c4>] (full_proxy_write) from [<c02b29d8>] (__vfs_write+0x1c/0x114)
[  218.396206] [<c02b29d8>] (__vfs_write) from [<c02b421c>] (vfs_write+0xa0/0x168)
[  218.403877] [<c02b421c>] (vfs_write) from [<c02b50b4>] (SyS_write+0x44/0x9c)
[  218.411276] [<c02b50b4>] (SyS_write) from [<c0107880>] (ret_fast_syscall+0x0/0x1c)
[  218.419347] ------------[ cut here ]------------
[  218.424208] WARNING: CPU: 1 PID: 2025 at lib/kobject.c:597 kobject_get+0x48/0x58
[  218.432018] kobject: '(null)' (ed379018): is not initialized, yet kobject_get() is being called.
[  218.441272] Modules linked in: usb_f_ss_lb g_zero libcomposite xhci_plat_hcd xhci_hcd usbcore dwc3 udc_core evdev snd_soc_davinci_mcasp usb_common snd_soc_edma snd_soc_simple_card m25p80 snd_soc_tlv320aic3x e
[  218.488436] CPU: 1 PID: 2025 Comm: bash Not tainted 4.11.0-rc4-00004-g559b2c9 #1285
[  218.496470] Hardware name: Generic DRA74X (Flattened Device Tree)
[  218.502870] [<c01101ec>] (unwind_backtrace) from [<c010c328>] (show_stack+0x10/0x14)
[  218.510996] [<c010c328>] (show_stack) from [<c04b46b8>] (dump_stack+0xac/0xe0)
[  218.518579] [<c04b46b8>] (dump_stack) from [<c0136cf0>] (__warn+0xd8/0x104)
[  218.525895] [<c0136cf0>] (__warn) from [<c0136d50>] (warn_slowpath_fmt+0x34/0x44)
[  218.533756] [<c0136d50>] (warn_slowpath_fmt) from [<c04b5e38>] (kobject_get+0x48/0x58)
[  218.542065] [<c04b5e38>] (kobject_get) from [<c0800ef0>] (klist_add_tail+0x18/0x44)
[  218.550114] [<c0800ef0>] (klist_add_tail) from [<c05730dc>] (device_add+0x3d8/0x570)
[  218.558258] [<c05730dc>] (device_add) from [<bf298ce8>] (usb_add_gadget_udc_release+0x88/0x1e8 [udc_core])
[  218.568428] [<bf298ce8>] (usb_add_gadget_udc_release [udc_core]) from [<bf2c6d68>] (dwc3_gadget_init+0x278/0x6c0 [dwc3])
[  218.579872] [<bf2c6d68>] (dwc3_gadget_init [dwc3]) from [<bf2cab54>] (dwc3_mode_write+0x178/0x1c4 [dwc3])
[  218.589939] [<bf2cab54>] (dwc3_mode_write [dwc3]) from [<c04557c4>] (full_proxy_write+0x4c/0x64)
[  218.599165] [<c04557c4>] (full_proxy_write) from [<c02b29d8>] (__vfs_write+0x1c/0x114)
[  218.607480] [<c02b29d8>] (__vfs_write) from [<c02b421c>] (vfs_write+0xa0/0x168)
[  218.615147] [<c02b421c>] (vfs_write) from [<c02b50b4>] (SyS_write+0x44/0x9c)
[  218.622549] [<c02b50b4>] (SyS_write) from [<c0107880>] (ret_fast_syscall+0x0/0x1c)
[  218.630543] ---[ end trace 9b9aa5ff9aaa9cf9 ]---
[  218.635433] ------------[ cut here ]------------
[  218.640321] WARNING: CPU: 1 PID: 2025 at lib/refcount.c:114 kobject_get+0x24/0x58
[  218.648232] refcount_t: increment on 0; use-after-free.
[  218.653718] Modules linked in: usb_f_ss_lb g_zero libcomposite xhci_plat_hcd xhci_hcd usbcore dwc3 udc_core evdev snd_soc_davinci_mcasp usb_common snd_soc_edma snd_soc_simple_card m25p80 snd_soc_tlv320aic3x e
[  218.700873] CPU: 1 PID: 2025 Comm: bash Tainted: G        W       4.11.0-rc4-00004-g559b2c9 #1285
[  218.710180] Hardware name: Generic DRA74X (Flattened Device Tree)
[  218.716583] [<c01101ec>] (unwind_backtrace) from [<c010c328>] (show_stack+0x10/0x14)
[  218.724710] [<c010c328>] (show_stack) from [<c04b46b8>] (dump_stack+0xac/0xe0)
[  218.732296] [<c04b46b8>] (dump_stack) from [<c0136cf0>] (__warn+0xd8/0x104)
[  218.739605] [<c0136cf0>] (__warn) from [<c0136d50>] (warn_slowpath_fmt+0x34/0x44)
[  218.747467] [<c0136d50>] (warn_slowpath_fmt) from [<c04b5e14>] (kobject_get+0x24/0x58)
[  218.755778] [<c04b5e14>] (kobject_get) from [<c0800ef0>] (klist_add_tail+0x18/0x44)
[  218.763820] [<c0800ef0>] (klist_add_tail) from [<c05730dc>] (device_add+0x3d8/0x570)
[  218.771963] [<c05730dc>] (device_add) from [<bf298ce8>] (usb_add_gadget_udc_release+0x88/0x1e8 [udc_core])
[  218.782128] [<bf298ce8>] (usb_add_gadget_udc_release [udc_core]) from [<bf2c6d68>] (dwc3_gadget_init+0x278/0x6c0 [dwc3])
[  218.793573] [<bf2c6d68>] (dwc3_gadget_init [dwc3]) from [<bf2cab54>] (dwc3_mode_write+0x178/0x1c4 [dwc3])
[  218.803634] [<bf2cab54>] (dwc3_mode_write [dwc3]) from [<c04557c4>] (full_proxy_write+0x4c/0x64)
[  218.812862] [<c04557c4>] (full_proxy_write) from [<c02b29d8>] (__vfs_write+0x1c/0x114)
[  218.821166] [<c02b29d8>] (__vfs_write) from [<c02b421c>] (vfs_write+0xa0/0x168)
[  218.828848] [<c02b421c>] (vfs_write) from [<c02b50b4>] (SyS_write+0x44/0x9c)
[  218.836251] [<c02b50b4>] (SyS_write) from [<c0107880>] (ret_fast_syscall+0x0/0x1c)
[  218.844240] ---[ end trace 9b9aa5ff9aaa9cfa ]---
[  218.850118] zero gadget: Gadget Zero, version: Cinco de Mayo 2008
[  218.856622] zero gadget: zero ready
[  218.861206] omap_l3_noc 44000000.ocp: L3 application error: target 5 mod:1 (unclearable)
[  218.869779] omap_l3_noc 44000000.ocp: L3 debug error: target 5 mod:1 (unclearable)

> 
>> +	switch (dwc->current_dr_role) {
>> +	case DWC3_GCTL_PRTCAP_HOST:
>> +		dwc3_host_exit(dwc);
>> +		break;
>> +	case DWC3_GCTL_PRTCAP_DEVICE:
>> +		dwc3_gadget_exit(dwc);
>> +		break;
>> +	default:
>> +		break;
>> +	}
>> +
>> +	/* switch PRTCAP mode. updates current_dr_role */
>> +	spin_lock_irqsave(&dwc->lock, flags);
>> +	dwc3_set_mode(dwc, mode);
>> +	spin_unlock_irqrestore(&dwc->lock, flags);
>> +
>> +	/* start new role */
>> +	switch (dwc->current_dr_role) {
>> +	case DWC3_GCTL_PRTCAP_HOST:
>> +		dwc3_host_init(dwc);
>> +		break;
>> +	case DWC3_GCTL_PRTCAP_DEVICE:
>> +		dwc3_gadget_init(dwc);
>> +		break;
>> +	default:
>> +		break;
>>  	}
>> +exit:
>>  	return count;
>>  }
>>  
>> -- 
>> 2.7.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

cheers,
-roger

Re: [PATCH v2 4/4] usb: dwc3: Workaround for super-speed host on dra7 in dual-role mode

[Felipe Balbi]

Hi,

Roger Quadros <rogerq@ti.com> writes:
> On 31/03/17 15:00, Felipe Balbi wrote:
>> 
>> Hi,
>> 
>> Roger Quadros <rogerq@ti.com> writes:
>>>>>> Your first implementation could be just that. Refactoring what needs to
>>>>>> be refactored, then patching "mode" debugfs to work properly in that
>>>>>> case. Only add otg.c/drd.c after "mode" debugfs file is stable, because
>>>>>> then you know what needs to be taken into consideration.
>>>>>>
>>>>>> Just to be clear, I'm not saying we should *ONLY* get the debugfs
>>>>>> interface for v4.12, I'm saying you should start with that and get that
>>>>>> stable and working properly (make an infinite loop constantly changing
>>>>>> modes and keep it running over the weekend) before you add support for
>>>>>> OTG interrupts, which could come in the same series ;-)
>>>>>>
>>>>>
>>>>> Just to clarify debugfs mode behaviour.
>>>>>
>>>>> Currently it is just changing PRTCAPDIR. What we need to do is that if
>>>>> dr_mode == "otg", then we call dwc3_host/gadget_init/exit() accordingly as well.
>>>>>
>>>>> Does this make sense?
>>>>
>>>> it does.
>>>>
>>>
>>> OK. Below is a patch that allows us to use debugfs/mode to do the role switch.
>>> Switching from device to host worked fine but I get the following error when
>>> switching from host to device.
>>>
>>> https://hastebin.com/liluqosewe.xml
>>>
>>> cheers,
>>> -roger
>>>
>>> ---
>>> From 50c49f18474b388d10533eb9f6d04f454fabf687 Mon Sep 17 00:00:00 2001
>>> From: Roger Quadros <rogerq@ti.com>
>>> Date: Fri, 31 Mar 2017 12:54:13 +0300
>>> Subject: [PATCH] usb: dwc3: make role-switching work with debugfs/mode
>>>
>>> If dr_mode == "otg", we start by default in PERIPHERAL mode.
>>> Keep track of current role in "current_dr_role" whenever dwc3_set_mode()
>>> is called.
>>>
>>> When debugfs/mode is changed AND we're in dual-role mode,
>>> handle the switch by stopping and starting the respective
>>> host/gadget controllers.
>>>
>>> Signed-off-by: Roger Quadros <rogerq@ti.com>
>> 
>> I'm assuming you also plan on breaking this down further ;-)
>
> Did you mean I must split this patch into smaller ones?
>
>> 
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 369bab1..e2d36ba 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -108,6 +108,8 @@ void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
>>>  	reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
>>>  	reg |= DWC3_GCTL_PRTCAPDIR(mode);
>>>  	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
>>> +
>>> +	dwc->current_dr_role = mode;
>>>  }
>>>  
>>>  u32 dwc3_core_fifo_space(struct dwc3_ep *dep, u8 type)
>>> @@ -862,13 +864,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
>>>  		}
>>>  		break;
>>>  	case USB_DR_MODE_OTG:
>>> -		ret = dwc3_host_init(dwc);
>>> -		if (ret) {
>>> -			if (ret != -EPROBE_DEFER)
>>> -				dev_err(dev, "failed to initialize host\n");
>>> -			return ret;
>>> -		}
>>> -
>>> +		/* start in peripheral role by default */
>>> +		dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
>>>  		ret = dwc3_gadget_init(dwc);
>>>  		if (ret) {
>>>  			if (ret != -EPROBE_DEFER)
>>> @@ -894,8 +891,11 @@ static void dwc3_core_exit_mode(struct dwc3 *dwc)
>>>  		dwc3_host_exit(dwc);
>>>  		break;
>>>  	case USB_DR_MODE_OTG:
>>> -		dwc3_host_exit(dwc);
>>> -		dwc3_gadget_exit(dwc);
>>> +		/* role might have changed since start */
>>> +		if (dwc->current_dr_role ==  DWC3_GCTL_PRTCAP_DEVICE)
>>> +			dwc3_gadget_exit(dwc);
>>> +		else if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)
>>> +			dwc3_host_exit(dwc);
>> 
>> how about patching the respective exit/init functions with something
>> like:
>> 
>> if (dwc->current_dr_role != $my_expected_role)
>> 	return 0;
>> 
>> then you can call them without any checks.
>
> OK.
>
>> 
>>> diff --git a/drivers/usb/dwc3/debugfs.c b/drivers/usb/dwc3/debugfs.c
>>> index 31926dd..a101b14 100644
>>> --- a/drivers/usb/dwc3/debugfs.c
>>> +++ b/drivers/usb/dwc3/debugfs.c
>>> @@ -327,19 +327,54 @@ static ssize_t dwc3_mode_write(struct file *file,
>>>  		return -EFAULT;
>>>  
>>>  	if (!strncmp(buf, "host", 4))
>>> -		mode |= DWC3_GCTL_PRTCAP_HOST;
>>> +		mode = DWC3_GCTL_PRTCAP_HOST;
>>>  
>>>  	if (!strncmp(buf, "device", 6))
>>> -		mode |= DWC3_GCTL_PRTCAP_DEVICE;
>>> +		mode = DWC3_GCTL_PRTCAP_DEVICE;
>>>  
>>>  	if (!strncmp(buf, "otg", 3))
>>> -		mode |= DWC3_GCTL_PRTCAP_OTG;
>>> +		mode = DWC3_GCTL_PRTCAP_OTG;
>>>  
>>> -	if (mode) {
>>> -		spin_lock_irqsave(&dwc->lock, flags);
>>> -		dwc3_set_mode(dwc, mode);
>>> -		spin_unlock_irqrestore(&dwc->lock, flags);
>>> +	if (!mode)
>>> +		return -EINVAL;
>>> +
>>> +	if (mode == dwc->current_dr_role)
>>> +		goto exit;
>>> +
>>> +	/* prevent role switching if we're not dual-role */
>>> +	if (dwc->dr_mode != USB_DR_MODE_OTG)
>>> +		return -EINVAL;
>>> +
>>> +	/* stop old role */
>>> +	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST)
>> 
>> is this your bug? This switch statement only executes when we're in host
>> mode. This means that when you switch to peripheral, you don't exit
>> host. Then when you switch back from peripheral to host, you're going to
>> add the same platform_device again. We're going to have TWO xHCI
>> platform device with the exact same name. When you finally switch again
>> from host to device, then you have issues.
>> 
>> Can you confirm?
>
> That was a bug but I still see the issue although only when a mass storage
> device was plugged in.
>
> I see this other new issue when not using a mass storage device.
>
> root@rockdesk:/sys/kernel/debug/48890000.usb# echo device > mode
> [  218.226104] xhci-hcd xhci-hcd.1.auto: remove, state 4
> [  218.231822] usb usb4: USB disconnect, device number 1
> [  218.246973] xhci-hcd xhci-hcd.1.auto: USB bus 4 deregistered
> [  218.252961] xhci-hcd xhci-hcd.1.auto: remove, state 4
> [  218.258347] usb usb3: USB disconnect, device number 1
> [  218.265858] xhci-hcd xhci-hcd.1.auto: USB bus 3 deregistered
> [  218.274312] dwc3 48890000.usb: changing max_speed on rev 5533202a
> [  218.282108] kobject (ed120208): tried to init an initialized object, something is seriously wrong.

kobj->state_initialized is left set. Need to find a clean way to clear
it. As a quick test, you could memset gadget->dev to zero from usb_del_gadget_udc()

-- 
balbi

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

[John Youn]

On 03/30/2017 02:27 AM, Felipe Balbi wrote:
>
> Hi
>
> Roger Quadros <rogerq@ti.com> writes:
>>>>> 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. :)
>
> fair enough
>
>>>>> 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.
>
> I see...
>
>>>> 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.
>
> that's true, sorry.
>
>> 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.
>
> man, there's really no way to mask OTG events. This can be bad :-)
>
> John, can you confirm if there's really no way of masking OTG events
> without loosing them?

Not sure. I'll have to verify.

If the IP version is 3.00a or higher you could experiment with using
interrupt moderation as a global mask.

John