[PATCH v3 0/7] Add fully tested id switch and vbus connect detect support for Chipidea

[PATCH v3 0/7] Add fully tested id switch and vbus connect detect support for Chipidea

[Peter Chen]

Changes for v3 mainly for 1/7, 3/7, 4/7, 7/7, see individual
patch commit for detail.

This patchset adds fully tested otg id switch function and
vbus connect/disconnect detection for chipidea driver.
The mainly design of id/vbus handling follows msm otg driver.
I hope the msm usb maintainer can have a look of this patchset,
and give some comments, and move the whole msm usb driver to
chipidea framework if possible in the future.

This patchset is fully tested at i.mx6Q saberlite board.

There is github repo for my chipdiea work:
https://github.com/hzpeterchen/linux-usb.git
branch: master

Peter Chen (7):
  Revert "USB: chipidea: add vbus detect for udc"
  usb: chipidea: add otg file
  usb: chipidea: add otg id switch and vbus connect/disconnect detect
  usb: chipidea: consolidate ci_role_driver's API for both roles
  usb: chipidea: udc: add pullup/pulldown dp at hw_device_state
  usb: chipidea: udc: retire the flag CI13_PULLUP_ON_VBUS
  usb: chipidea: imx: add internal vbus regulator control

 drivers/usb/chipidea/Makefile      |    2 +-
 drivers/usb/chipidea/bits.h        |   10 ++
 drivers/usb/chipidea/ci.h          |   29 +++++-
 drivers/usb/chipidea/ci13xxx_imx.c |   81 ++++++++++++----
 drivers/usb/chipidea/ci13xxx_msm.c |    1 -
 drivers/usb/chipidea/core.c        |  189 +++++++++++++++++++++++++++++++----
 drivers/usb/chipidea/host.c        |    2 +
 drivers/usb/chipidea/otg.c         |   61 ++++++++++++
 drivers/usb/chipidea/otg.h         |    6 +
 drivers/usb/chipidea/udc.c         |   90 +++++++----------
 include/linux/usb/chipidea.h       |    1 -
 11 files changed, 371 insertions(+), 101 deletions(-)
 create mode 100644 drivers/usb/chipidea/otg.c
 create mode 100644 drivers/usb/chipidea/otg.h

[PATCH v3 1/7] Revert "USB: chipidea: add vbus detect for udc"

[Peter Chen]

vbus register is at otgsc, and vbus detect does not belong
to device function. Revert this patch, and will move
vbus detect function to drivers/usb/chipidea/udc.c

This reverts commit 8c4fc031954b4eb72daf13d3c907a985a3eee208.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
Changes for v3:
Add Signed-off-by: Peter Chen <peter.chen@freescale.com>

 drivers/usb/chipidea/ci.h  |    1 -
 drivers/usb/chipidea/udc.c |   39 +--------------------------------------
 2 files changed, 1 insertions(+), 39 deletions(-)

diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index e25d126..d738603 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -139,7 +139,6 @@ struct ci13xxx {
 	enum ci_role			role;
 	bool				is_otg;
 	struct work_struct		work;
-	struct work_struct		vbus_work;
 	struct workqueue_struct		*wq;
 
 	struct dma_pool			*qh_pool;
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 2f45bba..d214448 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -305,18 +305,6 @@ static u32 hw_test_and_clear_intr_active(struct ci13xxx *ci)
 	return reg;
 }
 
-static void hw_enable_vbus_intr(struct ci13xxx *ci)
-{
-	hw_write(ci, OP_OTGSC, OTGSC_AVVIS, OTGSC_AVVIS);
-	hw_write(ci, OP_OTGSC, OTGSC_AVVIE, OTGSC_AVVIE);
-	queue_work(ci->wq, &ci->vbus_work);
-}
-
-static void hw_disable_vbus_intr(struct ci13xxx *ci)
-{
-	hw_write(ci, OP_OTGSC, OTGSC_AVVIE, 0);
-}
-
 /**
  * hw_test_and_clear_setup_guard: test & clear setup guard (execute without
  *                                interruption)
@@ -383,16 +371,6 @@ static int hw_usb_reset(struct ci13xxx *ci)
 	return 0;
 }
 
-static void vbus_work(struct work_struct *work)
-{
-	struct ci13xxx *ci = container_of(work, struct ci13xxx, vbus_work);
-
-	if (hw_read(ci, OP_OTGSC, OTGSC_AVV))
-		usb_gadget_vbus_connect(&ci->gadget);
-	else
-		usb_gadget_vbus_disconnect(&ci->gadget);
-}
-
 /******************************************************************************
  * UTIL block
  *****************************************************************************/
@@ -1392,7 +1370,6 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active)
 		if (is_active) {
 			pm_runtime_get_sync(&_gadget->dev);
 			hw_device_reset(ci, USBMODE_CM_DC);
-			hw_enable_vbus_intr(ci);
 			hw_device_state(ci, ci->ep0out->qh.dma);
 		} else {
 			hw_device_state(ci, 0);
@@ -1567,10 +1544,8 @@ static int ci13xxx_start(struct usb_gadget *gadget,
 	pm_runtime_get_sync(&ci->gadget.dev);
 	if (ci->platdata->flags & CI13XXX_PULLUP_ON_VBUS) {
 		if (ci->vbus_active) {
-			if (ci->platdata->flags & CI13XXX_REGS_SHARED) {
+			if (ci->platdata->flags & CI13XXX_REGS_SHARED)
 				hw_device_reset(ci, USBMODE_CM_DC);
-				hw_enable_vbus_intr(ci);
-			}
 		} else {
 			pm_runtime_put_sync(&ci->gadget.dev);
 			goto done;
@@ -1676,13 +1651,6 @@ static irqreturn_t udc_irq(struct ci13xxx *ci)
 	} else {
 		retval = IRQ_NONE;
 	}
-
-	intr = hw_read(ci, OP_OTGSC, ~0);
-	hw_write(ci, OP_OTGSC, ~0, intr);
-
-	if (intr & (OTGSC_AVVIE & OTGSC_AVVIS))
-		queue_work(ci->wq, &ci->vbus_work);
-
 	spin_unlock(&ci->lock);
 
 	return retval;
@@ -1758,7 +1726,6 @@ static int udc_start(struct ci13xxx *ci)
 		retval = hw_device_reset(ci, USBMODE_CM_DC);
 		if (retval)
 			goto put_transceiver;
-		hw_enable_vbus_intr(ci);
 	}
 
 	retval = device_register(&ci->gadget.dev);
@@ -1821,9 +1788,6 @@ static void udc_stop(struct ci13xxx *ci)
 	if (ci == NULL)
 		return;
 
-	hw_disable_vbus_intr(ci);
-	cancel_work_sync(&ci->vbus_work);
-
 	usb_del_gadget_udc(&ci->gadget);
 
 	destroy_eps(ci);
@@ -1864,7 +1828,6 @@ int ci_hdrc_gadget_init(struct ci13xxx *ci)
 	rdrv->irq	= udc_irq;
 	rdrv->name	= "gadget";
 	ci->roles[CI_ROLE_GADGET] = rdrv;
-	INIT_WORK(&ci->vbus_work, vbus_work);
 
 	return 0;
 }
-- 
1.7.0.4

[PATCH v3 2/7] usb: chipidea: add otg file

[Peter Chen]

Implement struct usb_otg, In that way, calling otg_set_peripheral
will not be failed at udc.c.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/Makefile |    2 +-
 drivers/usb/chipidea/ci.h     |    2 +
 drivers/usb/chipidea/otg.c    |   60 +++++++++++++++++++++++++++++++++++++++++
 drivers/usb/chipidea/otg.h    |    6 ++++
 4 files changed, 69 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
index d92ca32..11f513c 100644
--- a/drivers/usb/chipidea/Makefile
+++ b/drivers/usb/chipidea/Makefile
@@ -2,7 +2,7 @@ ccflags-$(CONFIG_USB_CHIPIDEA_DEBUG) := -DDEBUG
 
 obj-$(CONFIG_USB_CHIPIDEA)		+= ci_hdrc.o
 
-ci_hdrc-y				:= core.o
+ci_hdrc-y				:= core.o otg.o
 ci_hdrc-$(CONFIG_USB_CHIPIDEA_UDC)	+= udc.o
 ci_hdrc-$(CONFIG_USB_CHIPIDEA_HOST)	+= host.o
 ci_hdrc-$(CONFIG_USB_CHIPIDEA_DEBUG)	+= debug.o
diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index d738603..8702871 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -129,6 +129,7 @@ struct hw_bank {
  * @vbus_active: is VBUS active
  * @transceiver: pointer to USB PHY, if any
  * @hcd: pointer to usb_hcd for ehci host driver
+ * @otg: for otg support
  */
 struct ci13xxx {
 	struct device			*dev;
@@ -164,6 +165,7 @@ struct ci13xxx {
 	bool				global_phy;
 	struct usb_phy			*transceiver;
 	struct usb_hcd			*hcd;
+	struct usb_otg      otg;
 };
 
 static inline struct ci_role_driver *ci_role(struct ci13xxx *ci)
diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
new file mode 100644
index 0000000..7dea3b3
--- /dev/null
+++ b/drivers/usb/chipidea/otg.c
@@ -0,0 +1,60 @@
+/*
+ * otg.c - ChipIdea USB IP core OTG driver
+ *
+ * Copyright (C) 2012 Freescale Semiconductor, Inc.
+ *
+ * Author: Peter Chen
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/usb/gadget.h>
+#include <linux/usb/otg.h>
+#include <linux/usb/chipidea.h>
+
+#include "ci.h"
+#include "udc.h"
+#include "bits.h"
+#include "host.h"
+#include "debug.h"
+
+static int ci_otg_set_peripheral(struct usb_otg *otg,
+		struct usb_gadget *periph)
+{
+	otg->gadget = periph;
+
+	return 0;
+}
+
+static int ci_otg_set_host(struct usb_otg *otg, struct usb_bus *host)
+{
+	otg->host = host;
+
+	return 0;
+}
+
+/**
+ * ci_hdrc_otg_init - initialize device related bits
+ * ci: the controller
+ *
+ * This function create otg struct, if the device can switch between
+ * device and host.
+ */
+int ci_hdrc_otg_init(struct ci13xxx *ci)
+{
+	/* Useless at current */
+	ci->otg.set_peripheral = ci_otg_set_peripheral;
+	ci->otg.set_host = ci_otg_set_host;
+	if (!IS_ERR_OR_NULL(ci->transceiver))
+		ci->transceiver->otg = &ci->otg;
+
+	return 0;
+}
diff --git a/drivers/usb/chipidea/otg.h b/drivers/usb/chipidea/otg.h
new file mode 100644
index 0000000..b4c6b3e
--- /dev/null
+++ b/drivers/usb/chipidea/otg.h
@@ -0,0 +1,6 @@
+#ifndef __DRIVERS_USB_CHIPIDEA_OTG_H
+#define __DRIVERS_USB_CHIPIDEA_OTG_H
+
+int ci_hdrc_otg_init(struct ci13xxx *ci);
+
+#endif /* __DRIVERS_USB_CHIPIDEA_OTG_H */
-- 
1.7.0.4

[PATCH v3 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect

[Peter Chen]

The main design flow is the same with msm otg driver, that is the id and
vbus interrupt are handled at core driver, others are handled by
individual drivers.

- At former design, when switch usb role from device->host, it will call
udc_stop, it will remove the gadget driver, so when switch role
from host->device, it can't add gadget driver any more.
At new design, when role switch occurs, the gadget just calls
usb_gadget_vbus_disconnect/usb_gadget_vbus_connect as well as
reset controller, it will not free any device/gadget structure

- Add vbus connect and disconnect to core interrupt handler, it can
notify udc driver by calling usb_gadget_vbus_disconnect
/usb_gadget_vbus_connect.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
Changes for v3:
- Add prefix CI_ for ID and B_SESS_VALID
- Use new API hw_wait_reg which is alexander sugguested to wait vbus
voltage stable

 drivers/usb/chipidea/bits.h |   10 +++
 drivers/usb/chipidea/ci.h   |    5 +
 drivers/usb/chipidea/core.c |  182 ++++++++++++++++++++++++++++++++++++++----
 drivers/usb/chipidea/otg.c  |    1 +
 drivers/usb/chipidea/udc.c  |    2 +
 5 files changed, 182 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
index 050de85..ba9c6ef 100644
--- a/drivers/usb/chipidea/bits.h
+++ b/drivers/usb/chipidea/bits.h
@@ -65,11 +65,21 @@
 #define OTGSC_ASVIS	      BIT(18)
 #define OTGSC_BSVIS	      BIT(19)
 #define OTGSC_BSEIS	      BIT(20)
+#define OTGSC_1MSIS	      BIT(21)
+#define OTGSC_DPIS	      BIT(22)
 #define OTGSC_IDIE	      BIT(24)
 #define OTGSC_AVVIE	      BIT(25)
 #define OTGSC_ASVIE	      BIT(26)
 #define OTGSC_BSVIE	      BIT(27)
 #define OTGSC_BSEIE	      BIT(28)
+#define OTGSC_1MSIE	      BIT(29)
+#define OTGSC_DPIE	      BIT(30)
+#define OTGSC_INT_EN_BITS	(OTGSC_IDIE | OTGSC_AVVIE | OTGSC_ASVIE \
+				| OTGSC_BSVIE | OTGSC_BSEIE | OTGSC_1MSIE \
+				| OTGSC_DPIE)
+#define OTGSC_INT_STATUS_BITS	(OTGSC_IDIS | OTGSC_AVVIS | OTGSC_ASVIS	\
+				| OTGSC_BSVIS | OTGSC_BSEIS | OTGSC_1MSIS \
+				| OTGSC_DPIS)
 
 /* USBMODE */
 #define USBMODE_CM            (0x03UL <<  0)
diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index 8702871..12665fa 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -130,6 +130,7 @@ struct hw_bank {
  * @transceiver: pointer to USB PHY, if any
  * @hcd: pointer to usb_hcd for ehci host driver
  * @otg: for otg support
+ * @events: events for otg, and handled@ci_role_work
  */
 struct ci13xxx {
 	struct device			*dev;
@@ -140,6 +141,7 @@ struct ci13xxx {
 	enum ci_role			role;
 	bool				is_otg;
 	struct work_struct		work;
+	struct delayed_work		dwork;
 	struct workqueue_struct		*wq;
 
 	struct dma_pool			*qh_pool;
@@ -166,6 +168,9 @@ struct ci13xxx {
 	struct usb_phy			*transceiver;
 	struct usb_hcd			*hcd;
 	struct usb_otg      otg;
+#define CI_ID		0
+#define CI_B_SESS_VALID	1
+	unsigned long events;
 };
 
 static inline struct ci_role_driver *ci_role(struct ci13xxx *ci)
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index f69d029..e0392fa 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -73,6 +73,7 @@
 #include "bits.h"
 #include "host.h"
 #include "debug.h"
+#include "otg.h"
 
 /* Controller register map */
 static uintptr_t ci_regs_nolpm[] = {
@@ -265,24 +266,124 @@ static enum ci_role ci_otg_role(struct ci13xxx *ci)
 }
 
 /**
- * ci_role_work - perform role changing based on ID pin
- * @work: work struct
+ * hw_wait_reg: wait the register value
+ *
+ * Sometimes, it needs to wait register value before going on.
+ * Eg, when switch to device mode, the vbus value should be lower
+ * than OTGSC_BSV before connects to host.
+ *
+ * @ci: the controller
+ * @reg: register index
+ * @mask: mast bit
+ * @value: the bit value to wait
+ * @timeout: timeout to indicate an error
+ *
+ * This function returns an error code if timeout
  */
-static void ci_role_work(struct work_struct *work)
+static int hw_wait_reg(struct ci13xxx *ci, enum ci13xxx_regs reg, u32 mask,
+				u32 value, unsigned long timeout)
+{
+	unsigned long elapse = jiffies + timeout;
+
+	while (hw_read(ci, reg, mask) != value) {
+		if (time_after(jiffies, elapse)) {
+			dev_err(ci->dev, "timeout waiting for %08x in %d\n",
+					mask, reg);
+			return -ETIMEDOUT;
+		}
+		msleep(20);
+	}
+
+	return 0;
+}
+
+#define CI_VBUS_STABLE_TIMEOUT 500
+static void ci_handle_id_switch(struct ci13xxx *ci)
 {
-	struct ci13xxx *ci = container_of(work, struct ci13xxx, work);
 	enum ci_role role = ci_otg_role(ci);
 
 	if (role != ci->role) {
 		dev_dbg(ci->dev, "switching from %s to %s\n",
 			ci_role(ci)->name, ci->roles[role]->name);
 
-		ci_role_stop(ci);
-		ci_role_start(ci, role);
-		enable_irq(ci->irq);
+		/* 1. Finish the current role */
+		if (ci->role == CI_ROLE_GADGET) {
+			usb_gadget_vbus_disconnect(&ci->gadget);
+			/* host doesn't care B_SESSION_VALID event */
+			hw_write(ci, OP_OTGSC, OTGSC_BSVIE, ~OTGSC_BSVIE);
+			hw_write(ci, OP_OTGSC, OTGSC_BSVIS, OTGSC_BSVIS);
+			ci->role = CI_ROLE_END;
+			/* reset controller */
+			hw_device_reset(ci, USBMODE_CM_IDLE);
+		} else if (ci->role == CI_ROLE_HOST) {
+			ci_role_stop(ci);
+			/* reset controller */
+			hw_device_reset(ci, USBMODE_CM_IDLE);
+		}
+
+		/* 2. Turn on/off vbus according to coming role */
+		if (ci_otg_role(ci) == CI_ROLE_GADGET) {
+			otg_set_vbus(&ci->otg, false);
+			/* wait vbus lower than OTGSC_BSV */
+			hw_wait_reg(ci, OP_OTGSC, OTGSC_BSV, 0,
+					CI_VBUS_STABLE_TIMEOUT);
+		} else if (ci_otg_role(ci) == CI_ROLE_HOST) {
+			otg_set_vbus(&ci->otg, true);
+			/* wait vbus higher than OTGSC_AVV */
+			hw_wait_reg(ci, OP_OTGSC, OTGSC_AVV, OTGSC_AVV,
+					CI_VBUS_STABLE_TIMEOUT);
+		}
+
+		/* 3. Begin the new role */
+		if (ci_otg_role(ci) == CI_ROLE_GADGET) {
+			ci->role = role;
+			hw_write(ci, OP_OTGSC, OTGSC_BSVIS, OTGSC_BSVIS);
+			hw_write(ci, OP_OTGSC, OTGSC_BSVIE, OTGSC_BSVIE);
+		} else if (ci_otg_role(ci) == CI_ROLE_HOST) {
+			ci_role_start(ci, role);
+		}
 	}
 }
 
+static void ci_handle_vbus_change(struct ci13xxx *ci)
+{
+	u32 otgsc = hw_read(ci, OP_OTGSC, ~0);
+
+	if (otgsc & OTGSC_BSV)
+		usb_gadget_vbus_connect(&ci->gadget);
+	else
+		usb_gadget_vbus_disconnect(&ci->gadget);
+}
+
+/**
+ * ci_otg_work - perform otg (vbus/id) event handle
+ * @work: work struct
+ */
+static void ci_otg_work(struct work_struct *work)
+{
+	struct ci13xxx *ci = container_of(work, struct ci13xxx, work);
+
+	if (test_bit(CI_ID, &ci->events)) {
+		clear_bit(CI_ID, &ci->events);
+		ci_handle_id_switch(ci);
+	} else if (test_bit(CI_B_SESS_VALID, &ci->events)) {
+		clear_bit(CI_B_SESS_VALID, &ci->events);
+		ci_handle_vbus_change(ci);
+	} else
+		dev_err(ci->dev, "unexpected event occurs at %s\n", __func__);
+
+	enable_irq(ci->irq);
+}
+
+static void ci_delayed_work(struct work_struct *work)
+{
+	struct delayed_work *dwork = to_delayed_work(work);
+	struct ci13xxx *ci = container_of(dwork, struct ci13xxx, dwork);
+
+	otg_set_vbus(&ci->otg, true);
+
+}
+
 static ssize_t show_role(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
@@ -321,19 +422,36 @@ static irqreturn_t ci_irq(int irq, void *data)
 	irqreturn_t ret = IRQ_NONE;
 	u32 otgsc = 0;
 
-	if (ci->is_otg)
-		otgsc = hw_read(ci, OP_OTGSC, ~0);
-
-	if (ci->role != CI_ROLE_END)
-		ret = ci_role(ci)->irq(ci);
+	otgsc = hw_read(ci, OP_OTGSC, ~0);
 
-	if (ci->is_otg && (otgsc & OTGSC_IDIS)) {
+	/*
+	 * Handle id change interrupt, it indicates device/host function
+	 * switch.
+	 */
+	if (ci->is_otg && (otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS)) {
+		set_bit(CI_ID, &ci->events);
 		hw_write(ci, OP_OTGSC, OTGSC_IDIS, OTGSC_IDIS);
 		disable_irq_nosync(ci->irq);
 		queue_work(ci->wq, &ci->work);
-		ret = IRQ_HANDLED;
+		return IRQ_HANDLED;
+	}
+
+	/*
+	 * Handle vbus change interrupt, it indicates device connection
+	 * and disconnection events.
+	 */
+	if ((otgsc & OTGSC_BSVIE) && (otgsc & OTGSC_BSVIS)) {
+		set_bit(CI_B_SESS_VALID, &ci->events);
+		hw_write(ci, OP_OTGSC, OTGSC_BSVIS, OTGSC_BSVIS);
+		disable_irq_nosync(ci->irq);
+		queue_work(ci->wq, &ci->work);
+		return IRQ_HANDLED;
 	}
 
+	/* Handle device/host interrupt */
+	if (ci->role != CI_ROLE_END)
+		ret = ci_role(ci)->irq(ci);
+
 	return ret;
 }
 
@@ -397,6 +515,7 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
 	struct resource	*res;
 	void __iomem	*base;
 	int		ret;
+	u32		otgsc;
 
 	if (!dev->platform_data) {
 		dev_err(dev, "platform data missing\n");
@@ -421,6 +540,7 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 	}
 
+	ci->events = 0;
 	ci->dev = dev;
 	ci->platdata = dev->platform_data;
 	if (ci->platdata->phy)
@@ -442,12 +562,20 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
 		return -ENODEV;
 	}
 
-	INIT_WORK(&ci->work, ci_role_work);
+	INIT_WORK(&ci->work, ci_otg_work);
+	INIT_DELAYED_WORK(&ci->dwork, ci_delayed_work);
 	ci->wq = create_singlethread_workqueue("ci_otg");
 	if (!ci->wq) {
 		dev_err(dev, "can't create workqueue\n");
 		return -ENODEV;
 	}
+	/* Disable all interrupts bits */
+	hw_write(ci, OP_USBINTR, 0xffffffff, 0);
+	hw_write(ci, OP_OTGSC, OTGSC_INT_EN_BITS, 0);
+
+	/* Clear all interrupts status bits*/
+	hw_write(ci, OP_USBSTS, 0xffffffff, 0xffffffff);
+	hw_write(ci, OP_OTGSC, OTGSC_INT_STATUS_BITS, OTGSC_INT_STATUS_BITS);
 
 	/* initialize role(s) before the interrupt is requested */
 	ret = ci_hdrc_host_init(ci);
@@ -465,6 +593,7 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
 	}
 
 	if (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]) {
+		dev_dbg(dev, "support otg\n");
 		ci->is_otg = true;
 		/* ID pin needs 1ms debouce time, we delay 2ms for safe */
 		mdelay(2);
@@ -475,13 +604,29 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
 			: CI_ROLE_GADGET;
 	}
 
+	if (ci->is_otg)
+		/* if otg is supported, create struct usb_otg */
+		ci_hdrc_otg_init(ci);
+
 	ret = ci_role_start(ci, ci->role);
 	if (ret) {
-		dev_err(dev, "can't start %s role\n", ci_role(ci)->name);
+		dev_err(dev, "can't start %s role, ret=%d\n",
+				ci_role(ci)->name, ret);
 		ret = -ENODEV;
 		goto rm_wq;
 	}
 
+	otgsc = hw_read(ci, OP_OTGSC, ~0);
+	/*
+	 * if it is device mode:
+	 * - Enable vbus detect
+	 * - If it has already connected to host, notify udc
+	 */
+	if (ci->role == CI_ROLE_GADGET) {
+		hw_write(ci, OP_OTGSC, OTGSC_BSVIE, OTGSC_BSVIE);
+		ci_handle_vbus_change(ci);
+	}
+
 	platform_set_drvdata(pdev, ci);
 	ret = request_irq(ci->irq, ci_irq, IRQF_SHARED, ci->platdata->name,
 			  ci);
@@ -492,8 +637,9 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
 	if (ret)
 		goto rm_attr;
 
-	if (ci->is_otg)
-		hw_write(ci, OP_OTGSC, OTGSC_IDIE, OTGSC_IDIE);
+	/* Handle the situation that usb device at the MicroB to A cable */
+	if (ci->is_otg && !(otgsc & OTGSC_ID))
+		queue_delayed_work(ci->wq, &ci->dwork, msecs_to_jiffies(500));
 
 	return ret;
 
diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
index 7dea3b3..01ae162 100644
--- a/drivers/usb/chipidea/otg.c
+++ b/drivers/usb/chipidea/otg.c
@@ -55,6 +55,7 @@ int ci_hdrc_otg_init(struct ci13xxx *ci)
 	ci->otg.set_host = ci_otg_set_host;
 	if (!IS_ERR_OR_NULL(ci->transceiver))
 		ci->transceiver->otg = &ci->otg;
+	hw_write(ci, OP_OTGSC, OTGSC_IDIE, OTGSC_IDIE);
 
 	return 0;
 }
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index d214448..b52cb10 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -1371,6 +1371,7 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active)
 			pm_runtime_get_sync(&_gadget->dev);
 			hw_device_reset(ci, USBMODE_CM_DC);
 			hw_device_state(ci, ci->ep0out->qh.dma);
+			dev_dbg(ci->dev, "Connected to host\n");
 		} else {
 			hw_device_state(ci, 0);
 			if (ci->platdata->notify_event)
@@ -1378,6 +1379,7 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active)
 				CI13XXX_CONTROLLER_STOPPED_EVENT);
 			_gadget_stop_activity(&ci->gadget);
 			pm_runtime_put_sync(&_gadget->dev);
+			dev_dbg(ci->dev, "disconnected from host\n");
 		}
 	}
 
-- 
1.7.0.4

[PATCH v3 4/7] usb: chipidea: consolidate ci_role_driver's API for both roles

[Peter Chen]

- Create init/destroy API for probe and remove
- start/stop API are only used otg id switch process
- Create the gadget at ci_hdrc_probe if the gadget is supported
at that port, the main purpose for this is to avoid gadget module
load fail at init.rc

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
Changes for v3:
- Create init/destroy API for probe and remove
- start/stop API are only used otg id switch process

 drivers/usb/chipidea/ci.h   |   19 ++++++++++-
 drivers/usb/chipidea/core.c |   75 +++++++++++++++++++++----------------------
 drivers/usb/chipidea/host.c |    2 +
 drivers/usb/chipidea/udc.c  |   22 +++++++++++-
 4 files changed, 76 insertions(+), 42 deletions(-)

diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index 12665fa..bc9e4e1 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -67,14 +67,18 @@ enum ci_role {
 
 /**
  * struct ci_role_driver - host/gadget role driver
- * start: start this role
- * stop: stop this role
+ * init: init this role (used at module probe)
+ * start: start this role (used at id switch)
+ * stop: stop this role (used at id switch)
+ * destroy: destroy this role (used at module remove)
  * irq: irq handler for this role
  * name: role name string (host/gadget)
  */
 struct ci_role_driver {
+	int		(*init)(struct ci13xxx *);
 	int		(*start)(struct ci13xxx *);
 	void		(*stop)(struct ci13xxx *);
+	void		(*destroy)(struct ci13xxx *);
 	irqreturn_t	(*irq)(struct ci13xxx *);
 	const char	*name;
 };
@@ -207,6 +211,17 @@ static inline void ci_role_stop(struct ci13xxx *ci)
 	ci->roles[role]->stop(ci);
 }
 
+static inline void ci_role_destroy(struct ci13xxx *ci)
+{
+	enum ci_role role = ci->role;
+
+	if (role == CI_ROLE_END)
+		return;
+
+	ci->role = CI_ROLE_END;
+
+	ci->roles[role]->destroy(ci);
+}
 /******************************************************************************
  * REGISTERS
  *****************************************************************************/
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index e0392fa..f0d3691 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -307,27 +307,16 @@ static void ci_handle_id_switch(struct ci13xxx *ci)
 			ci_role(ci)->name, ci->roles[role]->name);
 
 		/* 1. Finish the current role */
-		if (ci->role == CI_ROLE_GADGET) {
-			usb_gadget_vbus_disconnect(&ci->gadget);
-			/* host doesn't care B_SESSION_VALID event */
-			hw_write(ci, OP_OTGSC, OTGSC_BSVIE, ~OTGSC_BSVIE);
-			hw_write(ci, OP_OTGSC, OTGSC_BSVIS, OTGSC_BSVIS);
-			ci->role = CI_ROLE_END;
-			/* reset controller */
-			hw_device_reset(ci, USBMODE_CM_IDLE);
-		} else if (ci->role == CI_ROLE_HOST) {
-			ci_role_stop(ci);
-			/* reset controller */
-			hw_device_reset(ci, USBMODE_CM_IDLE);
-		}
+		ci_role_stop(ci);
+		hw_device_reset(ci, USBMODE_CM_IDLE);
 
 		/* 2. Turn on/off vbus according to coming role */
-		if (ci_otg_role(ci) == CI_ROLE_GADGET) {
+		if (role == CI_ROLE_GADGET) {
 			otg_set_vbus(&ci->otg, false);
 			/* wait vbus lower than OTGSC_BSV */
 			hw_wait_reg(ci, OP_OTGSC, OTGSC_BSV, 0,
 					CI_VBUS_STABLE_TIMEOUT);
-		} else if (ci_otg_role(ci) == CI_ROLE_HOST) {
+		} else if (role == CI_ROLE_HOST) {
 			otg_set_vbus(&ci->otg, true);
 			/* wait vbus higher than OTGSC_AVV */
 			hw_wait_reg(ci, OP_OTGSC, OTGSC_AVV, OTGSC_AVV,
@@ -335,13 +324,7 @@ static void ci_handle_id_switch(struct ci13xxx *ci)
 		}
 
 		/* 3. Begin the new role */
-		if (ci_otg_role(ci) == CI_ROLE_GADGET) {
-			ci->role = role;
-			hw_write(ci, OP_OTGSC, OTGSC_BSVIS, OTGSC_BSVIS);
-			hw_write(ci, OP_OTGSC, OTGSC_BSVIE, OTGSC_BSVIE);
-		} else if (ci_otg_role(ci) == CI_ROLE_HOST) {
-			ci_role_start(ci, role);
-		}
+		ci_role_start(ci, role);
 	}
 }
 
@@ -584,7 +567,7 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
 
 	ret = ci_hdrc_gadget_init(ci);
 	if (ret)
-		dev_info(dev, "doesn't support gadget\n");
+		dev_info(dev, "doesn't support gadget, ret=%d\n", ret);
 
 	if (!ci->roles[CI_ROLE_HOST] && !ci->roles[CI_ROLE_GADGET]) {
 		dev_err(dev, "no supported roles\n");
@@ -608,23 +591,39 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
 		/* if otg is supported, create struct usb_otg */
 		ci_hdrc_otg_init(ci);
 
-	ret = ci_role_start(ci, ci->role);
-	if (ret) {
-		dev_err(dev, "can't start %s role, ret=%d\n",
-				ci_role(ci)->name, ret);
-		ret = -ENODEV;
-		goto rm_wq;
-	}
-
 	otgsc = hw_read(ci, OP_OTGSC, ~0);
+
 	/*
-	 * if it is device mode:
-	 * - Enable vbus detect
-	 * - If it has already connected to host, notify udc
+	 * If the gadget is supported, call its init unconditionally,
+	 * We need to support load gadget module at init.rc.
 	 */
-	if (ci->role == CI_ROLE_GADGET) {
-		hw_write(ci, OP_OTGSC, OTGSC_BSVIE, OTGSC_BSVIE);
-		ci_handle_vbus_change(ci);
+	if (ci->roles[CI_ROLE_GADGET]) {
+		ret = ci->roles[CI_ROLE_GADGET]->init(ci);
+		if (ret) {
+			dev_err(dev, "can't init %s role, ret=%d\n",
+					ci_role(ci)->name, ret);
+			ret = -ENODEV;
+			goto rm_wq;
+		}
+		/*
+		 * if it is device mode:
+		 * - Enable vbus detect
+		 * - If it has already connected to host, notify udc
+		 */
+		if (ci->role == CI_ROLE_GADGET) {
+			hw_write(ci, OP_OTGSC, OTGSC_BSVIE, OTGSC_BSVIE);
+			ci_handle_vbus_change(ci);
+		}
+	}
+
+	if (ci->role == CI_ROLE_HOST) {
+		ret = ci->roles[CI_ROLE_HOST]->init(ci);
+		if (ret) {
+			dev_err(dev, "can't init %s role, ret=%d\n",
+					ci_role(ci)->name, ret);
+			ret = -ENODEV;
+			goto rm_wq;
+		}
 	}
 
 	platform_set_drvdata(pdev, ci);
@@ -662,7 +661,7 @@ static int __devexit ci_hdrc_remove(struct platform_device *pdev)
 	destroy_workqueue(ci->wq);
 	device_remove_file(ci->dev, &dev_attr_role);
 	free_irq(ci->irq, ci);
-	ci_role_stop(ci);
+	ci_role_destroy(ci);
 
 	return 0;
 }
diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index caecad9..6024a4f 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -92,8 +92,10 @@ int ci_hdrc_host_init(struct ci13xxx *ci)
 	if (!rdrv)
 		return -ENOMEM;
 
+	rdrv->init	= host_start;
 	rdrv->start	= host_start;
 	rdrv->stop	= host_stop;
+	rdrv->destroy	= host_stop;
 	rdrv->irq	= host_irq;
 	rdrv->name	= "host";
 	ci->roles[CI_ROLE_HOST] = rdrv;
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index b52cb10..ae4755c 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -1780,6 +1780,22 @@ free_qh_pool:
 	return retval;
 }
 
+static int udc_id_switch_for_device(struct ci13xxx *ci)
+{
+	hw_write(ci, OP_OTGSC, OTGSC_BSVIS, OTGSC_BSVIS);
+	hw_write(ci, OP_OTGSC, OTGSC_BSVIE, OTGSC_BSVIE);
+
+	return 0;
+}
+
+static void udc_id_switch_for_host(struct ci13xxx *ci)
+{
+	usb_gadget_vbus_disconnect(&ci->gadget);
+	/* host doesn't care B_SESSION_VALID event */
+	hw_write(ci, OP_OTGSC, OTGSC_BSVIE, ~OTGSC_BSVIE);
+	hw_write(ci, OP_OTGSC, OTGSC_BSVIS, OTGSC_BSVIS);
+}
+
 /**
  * udc_remove: parent remove must call this to remove UDC
  *
@@ -1825,8 +1841,10 @@ int ci_hdrc_gadget_init(struct ci13xxx *ci)
 	if (!rdrv)
 		return -ENOMEM;
 
-	rdrv->start	= udc_start;
-	rdrv->stop	= udc_stop;
+	rdrv->init	= udc_start;
+	rdrv->start	= udc_id_switch_for_device;
+	rdrv->stop	= udc_id_switch_for_host;
+	rdrv->destroy	= udc_stop;
 	rdrv->irq	= udc_irq;
 	rdrv->name	= "gadget";
 	ci->roles[CI_ROLE_GADGET] = rdrv;
-- 
1.7.0.4

[PATCH v3 5/7] usb: chipidea: udc: add pullup/pulldown dp at hw_device_state

[Peter Chen]

- During the connect/disconnect host, we need to pullup
and pulldown dp
- Make sure the dp is not pullup until the vbus is on when
flag CI13XXX_PULLUP_ON_VBUS is set
- Using hw_device_state when set run/stop bit

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/udc.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index ae4755c..f27507f 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -90,8 +90,10 @@ static int hw_device_state(struct ci13xxx *ci, u32 dma)
 		/* interrupt, error, port change, reset, sleep/suspend */
 		hw_write(ci, OP_USBINTR, ~0,
 			     USBi_UI|USBi_UEI|USBi_PCI|USBi_URI|USBi_SLI);
+		hw_write(ci, OP_USBCMD, USBCMD_RS, USBCMD_RS);
 	} else {
 		hw_write(ci, OP_USBINTR, ~0, 0);
+		hw_write(ci, OP_USBCMD, USBCMD_RS, 0);
 	}
 	return 0;
 }
@@ -1423,10 +1425,14 @@ static int ci13xxx_pullup(struct usb_gadget *_gadget, int is_on)
 {
 	struct ci13xxx *ci = container_of(_gadget, struct ci13xxx, gadget);
 
+	if ((ci->platdata->flags & CI13XXX_PULLUP_ON_VBUS) &&
+			!ci->vbus_active)
+		return -ENOTSUPP;
+
 	if (is_on)
-		hw_write(ci, OP_USBCMD, USBCMD_RS, USBCMD_RS);
+		hw_device_state(ci, ci->ep0out->qh.dma);
 	else
-		hw_write(ci, OP_USBCMD, USBCMD_RS, 0);
+		hw_device_state(ci, 0);
 
 	return 0;
 }
-- 
1.7.0.4

[PATCH v3 6/7] usb: chipidea: udc: retire the flag CI13_PULLUP_ON_VBUS

[Peter Chen]

(change CI13XXX to CI13 to avoid junk email check)
Now, we have handled vbus session in core driver when the
vbus interrupt occurs, so our pullup operations are all
according to vbus.
Of cource, the software can still call .pullup when device connects
to host if it wants to connect/disconnect with host.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/ci13xxx_imx.c |    1 -
 drivers/usb/chipidea/ci13xxx_msm.c |    1 -
 drivers/usb/chipidea/udc.c         |   23 ++++++++---------------
 include/linux/usb/chipidea.h       |    1 -
 4 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
index 0f5ca4b..86671a3 100644
--- a/drivers/usb/chipidea/ci13xxx_imx.c
+++ b/drivers/usb/chipidea/ci13xxx_imx.c
@@ -88,7 +88,6 @@ EXPORT_SYMBOL_GPL(usbmisc_get_init_data);
 static struct ci13xxx_platform_data ci13xxx_imx_platdata __devinitdata  = {
 	.name			= "ci13xxx_imx",
 	.flags			= CI13XXX_REQUIRE_TRANSCEIVER |
-				  CI13XXX_PULLUP_ON_VBUS |
 				  CI13XXX_DISABLE_STREAMING,
 	.capoffset		= DEF_CAPOFFSET,
 };
diff --git a/drivers/usb/chipidea/ci13xxx_msm.c b/drivers/usb/chipidea/ci13xxx_msm.c
index b01feb3..cf74b62 100644
--- a/drivers/usb/chipidea/ci13xxx_msm.c
+++ b/drivers/usb/chipidea/ci13xxx_msm.c
@@ -49,7 +49,6 @@ static struct ci13xxx_platform_data ci13xxx_msm_platdata = {
 	.name			= "ci13xxx_msm",
 	.flags			= CI13XXX_REGS_SHARED |
 				  CI13XXX_REQUIRE_TRANSCEIVER |
-				  CI13XXX_PULLUP_ON_VBUS |
 				  CI13XXX_DISABLE_STREAMING,
 
 	.notify_event		= ci13xxx_msm_notify_event,
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index f27507f..f4796c1 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -1359,9 +1359,6 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active)
 	unsigned long flags;
 	int gadget_ready = 0;
 
-	if (!(ci->platdata->flags & CI13XXX_PULLUP_ON_VBUS))
-		return -EOPNOTSUPP;
-
 	spin_lock_irqsave(&ci->lock, flags);
 	ci->vbus_active = is_active;
 	if (ci->driver)
@@ -1425,8 +1422,7 @@ static int ci13xxx_pullup(struct usb_gadget *_gadget, int is_on)
 {
 	struct ci13xxx *ci = container_of(_gadget, struct ci13xxx, gadget);
 
-	if ((ci->platdata->flags & CI13XXX_PULLUP_ON_VBUS) &&
-			!ci->vbus_active)
+	if (!ci->vbus_active)
 		return -ENOTSUPP;
 
 	if (is_on)
@@ -1550,14 +1546,12 @@ static int ci13xxx_start(struct usb_gadget *gadget,
 
 	ci->driver = driver;
 	pm_runtime_get_sync(&ci->gadget.dev);
-	if (ci->platdata->flags & CI13XXX_PULLUP_ON_VBUS) {
-		if (ci->vbus_active) {
-			if (ci->platdata->flags & CI13XXX_REGS_SHARED)
-				hw_device_reset(ci, USBMODE_CM_DC);
-		} else {
-			pm_runtime_put_sync(&ci->gadget.dev);
-			goto done;
-		}
+	if (ci->vbus_active) {
+		if (ci->platdata->flags & CI13XXX_REGS_SHARED)
+			hw_device_reset(ci, USBMODE_CM_DC);
+	} else {
+		pm_runtime_put_sync(&ci->gadget.dev);
+		goto done;
 	}
 
 	retval = hw_device_state(ci, ci->ep0out->qh.dma);
@@ -1580,8 +1574,7 @@ static int ci13xxx_stop(struct usb_gadget *gadget,
 
 	spin_lock_irqsave(&ci->lock, flags);
 
-	if (!(ci->platdata->flags & CI13XXX_PULLUP_ON_VBUS) ||
-			ci->vbus_active) {
+	if (ci->vbus_active) {
 		hw_device_state(ci, 0);
 		if (ci->platdata->notify_event)
 			ci->platdata->notify_event(ci,
diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
index 544825d..37821b3 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -17,7 +17,6 @@ struct ci13xxx_platform_data {
 	unsigned long	 flags;
 #define CI13XXX_REGS_SHARED		BIT(0)
 #define CI13XXX_REQUIRE_TRANSCEIVER	BIT(1)
-#define CI13XXX_PULLUP_ON_VBUS		BIT(2)
 #define CI13XXX_DISABLE_STREAMING	BIT(3)
 
 #define CI13XXX_CONTROLLER_RESET_EVENT		0
-- 
1.7.0.4

[PATCH v3 7/7] usb: chipidea: imx: add internal vbus regulator control

[Peter Chen]

- For host, the vbus should always be on.
- For otg, the vbus is off defaultly, the vbus needs to be
turned on/off when usb role switches.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
Changes for v3:
- Add return value check for regulator_enable/regulator_disable
- typo error

 drivers/usb/chipidea/ci.h          |    6 ++-
 drivers/usb/chipidea/ci13xxx_imx.c |   80 ++++++++++++++++++++++++++++--------
 2 files changed, 66 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index bc9e4e1..9528f6a 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -135,6 +135,7 @@ struct hw_bank {
  * @hcd: pointer to usb_hcd for ehci host driver
  * @otg: for otg support
  * @events: events for otg, and handled at ci_role_work
+ * @reg_vbus: used to control internal vbus regulator
  */
 struct ci13xxx {
 	struct device			*dev;
@@ -171,10 +172,11 @@ struct ci13xxx {
 	bool				global_phy;
 	struct usb_phy			*transceiver;
 	struct usb_hcd			*hcd;
-	struct usb_otg      otg;
+	struct usb_otg			otg;
 #define CI_ID		0
 #define CI_B_SESS_VALID	1
-	unsigned long events;
+	unsigned long			events;
+	struct regulator		*reg_vbus;
 };
 
 static inline struct ci_role_driver *ci_role(struct ci13xxx *ci)
diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
index 86671a3..b50cfd7 100644
--- a/drivers/usb/chipidea/ci13xxx_imx.c
+++ b/drivers/usb/chipidea/ci13xxx_imx.c
@@ -88,14 +88,47 @@ EXPORT_SYMBOL_GPL(usbmisc_get_init_data);
 static struct ci13xxx_platform_data ci13xxx_imx_platdata __devinitdata  = {
 	.name			= "ci13xxx_imx",
 	.flags			= CI13XXX_REQUIRE_TRANSCEIVER |
-				  CI13XXX_DISABLE_STREAMING,
+				  CI13XXX_DISABLE_STREAMING |
+				  CI13XXX_REGS_SHARED,
 	.capoffset		= DEF_CAPOFFSET,
 };
 
+static int ci13xxx_otg_set_vbus(struct usb_otg *otg, bool enabled)
+{
+	struct ci13xxx	*ci = container_of(otg, struct ci13xxx, otg);
+	struct regulator *reg_vbus = ci->reg_vbus;
+	int ret;
+
+	WARN_ON(!reg_vbus);
+
+	if (reg_vbus) {
+		if (enabled) {
+			ret = regulator_enable(reg_vbus);
+			if (ret) {
+				dev_err(ci->dev,
+				"Failed to enable vbus regulator, ret=%d\n",
+				ret);
+				return ret;
+			}
+		} else {
+			ret = regulator_disable(reg_vbus);
+			if (ret) {
+				dev_err(ci->dev,
+				"Failed to disable vbus regulator, ret=%d\n",
+				ret);
+				return ret;
+			}
+		}
+	}
+
+	return 0;
+}
+
 static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
 {
 	struct ci13xxx_imx_data *data;
 	struct platform_device *plat_ci, *phy_pdev;
+	struct ci13xxx	*ci;
 	struct device_node *phy_np;
 	struct resource *res;
 	struct regulator *reg_vbus;
@@ -152,20 +185,11 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
 		}
 	}
 
-	/* we only support host now, so enable vbus here */
 	reg_vbus = devm_regulator_get(&pdev->dev, "vbus");
-	if (!IS_ERR(reg_vbus)) {
-		ret = regulator_enable(reg_vbus);
-		if (ret) {
-			dev_err(&pdev->dev,
-				"Failed to enable vbus regulator, err=%d\n",
-				ret);
-			goto put_np;
-		}
+	if (!IS_ERR(reg_vbus))
 		data->reg_vbus = reg_vbus;
-	} else {
+	else
 		reg_vbus = NULL;
-	}
 
 	ci13xxx_imx_platdata.phy = data->phy;
 
@@ -175,7 +199,7 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
 		if (!pdev->dev.dma_mask) {
 			ret = -ENOMEM;
 			dev_err(&pdev->dev, "Failed to alloc dma_mask!\n");
-			goto err;
+			goto put_np;
 		}
 		*pdev->dev.dma_mask = DMA_BIT_MASK(32);
 		dma_set_coherent_mask(&pdev->dev, *pdev->dev.dma_mask);
@@ -186,7 +210,7 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
 		if (ret) {
 			dev_err(&pdev->dev,
 				"usbmisc init failed, ret=%d\n", ret);
-			goto err;
+			goto put_np;
 		}
 	}
 
@@ -198,24 +222,44 @@ static int __devinit ci13xxx_imx_probe(struct platform_device *pdev)
 		dev_err(&pdev->dev,
 			"Can't register ci_hdrc platform device, err=%d\n",
 			ret);
-		goto err;
+		goto put_np;
 	}
 
 	data->ci_pdev = plat_ci;
 	platform_set_drvdata(pdev, data);
 
+	ci = platform_get_drvdata(plat_ci);
+	/*
+	 * Internal vbus on/off policy
+	 * - Always on for host only function
+	 * - Always off for gadget only function
+	 * - call otg.set_vbus to control on/off according usb role
+	 */
+
+	if (ci->roles[CI_ROLE_HOST] && !ci->roles[CI_ROLE_GADGET]
+			&& reg_vbus) {
+		ret = regulator_enable(reg_vbus);
+		if (ret) {
+			dev_err(&pdev->dev,
+				"Failed to enable vbus regulator, ret=%d\n",
+				ret);
+			goto put_np;
+		}
+	} else if (ci->is_otg) {
+		ci->otg.set_vbus = ci13xxx_otg_set_vbus;
+		ci->reg_vbus = data->reg_vbus;
+	}
+
 	pm_runtime_no_callbacks(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 
 	return 0;
 
-err:
-	if (reg_vbus)
-		regulator_disable(reg_vbus);
 put_np:
 	if (phy_np)
 		of_node_put(phy_np);
 	clk_disable_unprepare(data->clk);
+
 	return ret;
 }
 
-- 
1.7.0.4

[PATCH v3 2/7] usb: chipidea: add otg file

[Alexander Shishkin]

Peter Chen <peter.chen@freescale.com> writes:

> diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> index d738603..8702871 100644
> --- a/drivers/usb/chipidea/ci.h
> +++ b/drivers/usb/chipidea/ci.h
> @@ -129,6 +129,7 @@ struct hw_bank {
>   * @vbus_active: is VBUS active
>   * @transceiver: pointer to USB PHY, if any
>   * @hcd: pointer to usb_hcd for ehci host driver
> + * @otg: for otg support
>   */
>  struct ci13xxx {
>  	struct device			*dev;
> @@ -164,6 +165,7 @@ struct ci13xxx {
>  	bool				global_phy;
>  	struct usb_phy			*transceiver;
>  	struct usb_hcd			*hcd;
> +	struct usb_otg      otg;

Can you indent the "otg" so that it's aligned with the rest?

>  };
>  
>  static inline struct ci_role_driver *ci_role(struct ci13xxx *ci)
> diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
> new file mode 100644
> index 0000000..7dea3b3
> --- /dev/null
> +++ b/drivers/usb/chipidea/otg.c
> @@ -0,0 +1,60 @@
> +/*
> + * otg.c - ChipIdea USB IP core OTG driver
> + *
> + * Copyright (C) 2012 Freescale Semiconductor, Inc.
> + *
> + * Author: Peter Chen
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/module.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>

Most of these look unnecessary.

> +#include <linux/usb/gadget.h>
> +#include <linux/usb/otg.h>
> +#include <linux/usb/chipidea.h>
> +
> +#include "ci.h"
> +#include "udc.h"
> +#include "bits.h"
> +#include "host.h"
> +#include "debug.h"

And these.

> +
> +static int ci_otg_set_peripheral(struct usb_otg *otg,
> +		struct usb_gadget *periph)
> +{
> +	otg->gadget = periph;
> +
> +	return 0;
> +}
> +
> +static int ci_otg_set_host(struct usb_otg *otg, struct usb_bus *host)
> +{
> +	otg->host = host;
> +
> +	return 0;
> +}
> +
> +/**
> + * ci_hdrc_otg_init - initialize device related bits
> + * ci: the controller
> + *
> + * This function create otg struct, if the device can switch between
> + * device and host.
> + */
> +int ci_hdrc_otg_init(struct ci13xxx *ci)
> +{
> +	/* Useless at current */
> +	ci->otg.set_peripheral = ci_otg_set_peripheral;
> +	ci->otg.set_host = ci_otg_set_host;
> +	if (!IS_ERR_OR_NULL(ci->transceiver))
> +		ci->transceiver->otg = &ci->otg;
> +
> +	return 0;
> +}
> diff --git a/drivers/usb/chipidea/otg.h b/drivers/usb/chipidea/otg.h
> new file mode 100644
> index 0000000..b4c6b3e
> --- /dev/null
> +++ b/drivers/usb/chipidea/otg.h
> @@ -0,0 +1,6 @@
> +#ifndef __DRIVERS_USB_CHIPIDEA_OTG_H
> +#define __DRIVERS_USB_CHIPIDEA_OTG_H
> +
> +int ci_hdrc_otg_init(struct ci13xxx *ci);

Can you put it to ci.h instead?

Thanks,
--
Alex

[PATCH v3 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect

[Alexander Shishkin]

Peter Chen <peter.chen@freescale.com> writes:

> +static void ci_otg_work(struct work_struct *work)
> +{
> +	struct ci13xxx *ci = container_of(work, struct ci13xxx, work);
> +
> +	if (test_bit(CI_ID, &ci->events)) {
> +		clear_bit(CI_ID, &ci->events);
> +		ci_handle_id_switch(ci);
> +	} else if (test_bit(CI_B_SESS_VALID, &ci->events)) {
> +		clear_bit(CI_B_SESS_VALID, &ci->events);
> +		ci_handle_vbus_change(ci);
> +	} else
> +		dev_err(ci->dev, "unexpected event occurs at %s\n", __func__);
> +
> +	enable_irq(ci->irq);
> +}

if (ci->id) {
	ci->id = false;
	ci_handle_id_switch(ci);
} else if (ci->vbus_valid) {
	ci->vbus_valid = false;
	ci_handle_vbus_change(ci);
} ...

is so much easier on the eyes. We really don't have any reasons to have
events in a bitmask.

> +
> +static void ci_delayed_work(struct work_struct *work)
> +{
> +	struct delayed_work *dwork = to_delayed_work(work);
> +	struct ci13xxx *ci = container_of(dwork, struct ci13xxx, dwork);
> +
> +	otg_set_vbus(&ci->otg, true);
> +
> +}
> +
>  static ssize_t show_role(struct device *dev, struct device_attribute *attr,
>  			 char *buf)
>  {
> @@ -321,19 +422,36 @@ static irqreturn_t ci_irq(int irq, void *data)
>  	irqreturn_t ret = IRQ_NONE;
>  	u32 otgsc = 0;
>  
> -	if (ci->is_otg)
> -		otgsc = hw_read(ci, OP_OTGSC, ~0);
> -
> -	if (ci->role != CI_ROLE_END)
> -		ret = ci_role(ci)->irq(ci);
> +	otgsc = hw_read(ci, OP_OTGSC, ~0);
>  
> -	if (ci->is_otg && (otgsc & OTGSC_IDIS)) {
> +	/*
> +	 * Handle id change interrupt, it indicates device/host function
> +	 * switch.
> +	 */
> +	if (ci->is_otg && (otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS)) {
> +		set_bit(CI_ID, &ci->events);
>  		hw_write(ci, OP_OTGSC, OTGSC_IDIS, OTGSC_IDIS);

A thought: we're setting and unsetting IDIS/IDIE and BSVIS/BSVIE that we
might use a helper function for it.

>  		disable_irq_nosync(ci->irq);
>  		queue_work(ci->wq, &ci->work);
> -		ret = IRQ_HANDLED;
> +		return IRQ_HANDLED;
> +	}
> +
> +	/*
> +	 * Handle vbus change interrupt, it indicates device connection
> +	 * and disconnection events.
> +	 */
> +	if ((otgsc & OTGSC_BSVIE) && (otgsc & OTGSC_BSVIS)) {
> +		set_bit(CI_B_SESS_VALID, &ci->events);
> +		hw_write(ci, OP_OTGSC, OTGSC_BSVIS, OTGSC_BSVIS);
> +		disable_irq_nosync(ci->irq);
> +		queue_work(ci->wq, &ci->work);
> +		return IRQ_HANDLED;
>  	}
>  
> +	/* Handle device/host interrupt */
> +	if (ci->role != CI_ROLE_END)
> +		ret = ci_role(ci)->irq(ci);
> +
>  	return ret;
>  }
>  
> @@ -397,6 +515,7 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
>  	struct resource	*res;
>  	void __iomem	*base;
>  	int		ret;
> +	u32		otgsc;
>  
>  	if (!dev->platform_data) {
>  		dev_err(dev, "platform data missing\n");
> @@ -421,6 +540,7 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	}
>  
> +	ci->events = 0;

"ci" has just been kzalloc'ed.

>  	ci->dev = dev;
>  	ci->platdata = dev->platform_data;
>  	if (ci->platdata->phy)
> @@ -442,12 +562,20 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
>  		return -ENODEV;
>  	}
>  
> -	INIT_WORK(&ci->work, ci_role_work);
> +	INIT_WORK(&ci->work, ci_otg_work);
> +	INIT_DELAYED_WORK(&ci->dwork, ci_delayed_work);
>  	ci->wq = create_singlethread_workqueue("ci_otg");
>  	if (!ci->wq) {
>  		dev_err(dev, "can't create workqueue\n");
>  		return -ENODEV;
>  	}
> +	/* Disable all interrupts bits */
> +	hw_write(ci, OP_USBINTR, 0xffffffff, 0);
> +	hw_write(ci, OP_OTGSC, OTGSC_INT_EN_BITS, 0);
> +
> +	/* Clear all interrupts status bits*/
> +	hw_write(ci, OP_USBSTS, 0xffffffff, 0xffffffff);
> +	hw_write(ci, OP_OTGSC, OTGSC_INT_STATUS_BITS, OTGSC_INT_STATUS_BITS);

How about moving these to hw_device_init()?

>  
>  	/* initialize role(s) before the interrupt is requested */
>  	ret = ci_hdrc_host_init(ci);
> @@ -465,6 +593,7 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
>  	}
>  
>  	if (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]) {
> +		dev_dbg(dev, "support otg\n");
>  		ci->is_otg = true;
>  		/* ID pin needs 1ms debouce time, we delay 2ms for safe */
>  		mdelay(2);
> @@ -475,13 +604,29 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
>  			: CI_ROLE_GADGET;
>  	}
>  
> +	if (ci->is_otg)
> +		/* if otg is supported, create struct usb_otg */
> +		ci_hdrc_otg_init(ci);

Can this just go the same block where is_otg is set?

> +
>  	ret = ci_role_start(ci, ci->role);
>  	if (ret) {
> -		dev_err(dev, "can't start %s role\n", ci_role(ci)->name);
> +		dev_err(dev, "can't start %s role, ret=%d\n",
> +				ci_role(ci)->name, ret);

This change is not really related to the rest of the patch.

>  		ret = -ENODEV;
>  		goto rm_wq;
>  	}
>  
> +	otgsc = hw_read(ci, OP_OTGSC, ~0);
> +	/*
> +	 * if it is device mode:
> +	 * - Enable vbus detect
> +	 * - If it has already connected to host, notify udc
> +	 */
> +	if (ci->role == CI_ROLE_GADGET) {
> +		hw_write(ci, OP_OTGSC, OTGSC_BSVIE, OTGSC_BSVIE);
> +		ci_handle_vbus_change(ci);
> +	}

This looks like something that belongs to gadget role start, doesn't it?

> +
>  	platform_set_drvdata(pdev, ci);
>  	ret = request_irq(ci->irq, ci_irq, IRQF_SHARED, ci->platdata->name,
>  			  ci);
> @@ -492,8 +637,9 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto rm_attr;
>  
> -	if (ci->is_otg)
> -		hw_write(ci, OP_OTGSC, OTGSC_IDIE, OTGSC_IDIE);
> +	/* Handle the situation that usb device at the MicroB to A cable */
> +	if (ci->is_otg && !(otgsc & OTGSC_ID))
> +		queue_delayed_work(ci->wq, &ci->dwork, msecs_to_jiffies(500));
>  
>  	return ret;
>  
> diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
> index 7dea3b3..01ae162 100644
> --- a/drivers/usb/chipidea/otg.c
> +++ b/drivers/usb/chipidea/otg.c
> @@ -55,6 +55,7 @@ int ci_hdrc_otg_init(struct ci13xxx *ci)
>  	ci->otg.set_host = ci_otg_set_host;
>  	if (!IS_ERR_OR_NULL(ci->transceiver))
>  		ci->transceiver->otg = &ci->otg;
> +	hw_write(ci, OP_OTGSC, OTGSC_IDIE, OTGSC_IDIE);
>  
>  	return 0;
>  }
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index d214448..b52cb10 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -1371,6 +1371,7 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active)
>  			pm_runtime_get_sync(&_gadget->dev);
>  			hw_device_reset(ci, USBMODE_CM_DC);
>  			hw_device_state(ci, ci->ep0out->qh.dma);
> +			dev_dbg(ci->dev, "Connected to host\n");
>  		} else {
>  			hw_device_state(ci, 0);
>  			if (ci->platdata->notify_event)
> @@ -1378,6 +1379,7 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active)
>  				CI13XXX_CONTROLLER_STOPPED_EVENT);
>  			_gadget_stop_activity(&ci->gadget);
>  			pm_runtime_put_sync(&_gadget->dev);
> +			dev_dbg(ci->dev, "disconnected from host\n");
>  		}
>  	}
>  
> -- 
> 1.7.0.4

[PATCH v3 2/7] usb: chipidea: add otg file

[Peter Chen]

On Thu, Nov 29, 2012 at 03:53:26PM +0200, Alexander Shishkin wrote:
> Peter Chen <peter.chen@freescale.com> writes:
> 
> > diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> > index d738603..8702871 100644
> > --- a/drivers/usb/chipidea/ci.h
> > +++ b/drivers/usb/chipidea/ci.h
> > @@ -129,6 +129,7 @@ struct hw_bank {
> >   * @vbus_active: is VBUS active
> >   * @transceiver: pointer to USB PHY, if any
> >   * @hcd: pointer to usb_hcd for ehci host driver
> > + * @otg: for otg support
> >   */
> >  struct ci13xxx {
> >  	struct device			*dev;
> > @@ -164,6 +165,7 @@ struct ci13xxx {
> >  	bool				global_phy;
> >  	struct usb_phy			*transceiver;
> >  	struct usb_hcd			*hcd;
> > +	struct usb_otg      otg;
> 
> Can you indent the "otg" so that it's aligned with the rest?
will change
> 
> >  };
> >  
> >  static inline struct ci_role_driver *ci_role(struct ci13xxx *ci)
> > diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
> > new file mode 100644
> > index 0000000..7dea3b3
> > --- /dev/null
> > +++ b/drivers/usb/chipidea/otg.c
> > @@ -0,0 +1,60 @@
> > +/*
> > + * otg.c - ChipIdea USB IP core OTG driver
> > + *
> > + * Copyright (C) 2012 Freescale Semiconductor, Inc.
> > + *
> > + * Author: Peter Chen
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/platform_device.h>
> > +#include <linux/module.h>
> > +#include <linux/io.h>
> > +#include <linux/irq.h>
> > +#include <linux/kernel.h>
> > +#include <linux/slab.h>
> 
> Most of these look unnecessary.
> 
> > +#include <linux/usb/gadget.h>
> > +#include <linux/usb/otg.h>
> > +#include <linux/usb/chipidea.h>
> > +
> > +#include "ci.h"
> > +#include "udc.h"
> > +#include "bits.h"
> > +#include "host.h"
> > +#include "debug.h"
> 
> And these.
will change
> 
> > +
> > +static int ci_otg_set_peripheral(struct usb_otg *otg,
> > +		struct usb_gadget *periph)
> > +{
> > +	otg->gadget = periph;
> > +
> > +	return 0;
> > +}
> > +
> > +static int ci_otg_set_host(struct usb_otg *otg, struct usb_bus *host)
> > +{
> > +	otg->host = host;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * ci_hdrc_otg_init - initialize device related bits
> > + * ci: the controller
> > + *
> > + * This function create otg struct, if the device can switch between
> > + * device and host.
> > + */
> > +int ci_hdrc_otg_init(struct ci13xxx *ci)
> > +{
> > +	/* Useless at current */
> > +	ci->otg.set_peripheral = ci_otg_set_peripheral;
> > +	ci->otg.set_host = ci_otg_set_host;
> > +	if (!IS_ERR_OR_NULL(ci->transceiver))
> > +		ci->transceiver->otg = &ci->otg;
> > +
> > +	return 0;
> > +}
> > diff --git a/drivers/usb/chipidea/otg.h b/drivers/usb/chipidea/otg.h
> > new file mode 100644
> > index 0000000..b4c6b3e
> > --- /dev/null
> > +++ b/drivers/usb/chipidea/otg.h
> > @@ -0,0 +1,6 @@
> > +#ifndef __DRIVERS_USB_CHIPIDEA_OTG_H
> > +#define __DRIVERS_USB_CHIPIDEA_OTG_H
> > +
> > +int ci_hdrc_otg_init(struct ci13xxx *ci);
> 
> Can you put it to ci.h instead?

I think we may have more complicated operation for otg, so keep one
otg header is accepted, like host.h, udc.h.
> 
> Thanks,
> --
> Alex
> 

-- 

Best Regards,
Peter Chen

[PATCH v3 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect

[Peter Chen]

On Thu, Nov 29, 2012 at 04:30:10PM +0200, Alexander Shishkin wrote:
> Peter Chen <peter.chen@freescale.com> writes:
> 
> > +static void ci_otg_work(struct work_struct *work)
> > +{
> > +	struct ci13xxx *ci = container_of(work, struct ci13xxx, work);
> > +
> > +	if (test_bit(CI_ID, &ci->events)) {
> > +		clear_bit(CI_ID, &ci->events);
> > +		ci_handle_id_switch(ci);
> > +	} else if (test_bit(CI_B_SESS_VALID, &ci->events)) {
> > +		clear_bit(CI_B_SESS_VALID, &ci->events);
> > +		ci_handle_vbus_change(ci);
> > +	} else
> > +		dev_err(ci->dev, "unexpected event occurs at %s\n", __func__);
> > +
> > +	enable_irq(ci->irq);
> > +}
> 
> if (ci->id) {
> 	ci->id = false;
> 	ci_handle_id_switch(ci);
> } else if (ci->vbus_valid) {
> 	ci->vbus_valid = false;
> 	ci_handle_vbus_change(ci);
> } ...
> 
> is so much easier on the eyes. We really don't have any reasons to have
> events in a bitmask.

Ok, then I keep several flags (currently, it is two) as otg events.
> > @@ -321,19 +422,36 @@ static irqreturn_t ci_irq(int irq, void *data)
> >  	irqreturn_t ret = IRQ_NONE;
> >  	u32 otgsc = 0;
> >  
> > -	if (ci->is_otg)
> > -		otgsc = hw_read(ci, OP_OTGSC, ~0);
> > -
> > -	if (ci->role != CI_ROLE_END)
> > -		ret = ci_role(ci)->irq(ci);
> > +	otgsc = hw_read(ci, OP_OTGSC, ~0);
> >  
> > -	if (ci->is_otg && (otgsc & OTGSC_IDIS)) {
> > +	/*
> > +	 * Handle id change interrupt, it indicates device/host function
> > +	 * switch.
> > +	 */
> > +	if (ci->is_otg && (otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS)) {
> > +		set_bit(CI_ID, &ci->events);
> >  		hw_write(ci, OP_OTGSC, OTGSC_IDIS, OTGSC_IDIS);
> 
> A thought: we're setting and unsetting IDIS/IDIE and BSVIS/BSVIE that we
> might use a helper function for it.
ci_clear_otg_status/ci_enable_otg_interrupt/ci_disable_otg_interrupt?

> 
> >  		disable_irq_nosync(ci->irq);
> >  		queue_work(ci->wq, &ci->work);
> > -		ret = IRQ_HANDLED;
> > +		return IRQ_HANDLED;
> > +	}
> > +
> > +	/*
> > +	 * Handle vbus change interrupt, it indicates device connection
> > +	 * and disconnection events.
> > +	 */
> > +	if ((otgsc & OTGSC_BSVIE) && (otgsc & OTGSC_BSVIS)) {
> > +		set_bit(CI_B_SESS_VALID, &ci->events);
> > +		hw_write(ci, OP_OTGSC, OTGSC_BSVIS, OTGSC_BSVIS);
> > +		disable_irq_nosync(ci->irq);
> > +		queue_work(ci->wq, &ci->work);
> > +		return IRQ_HANDLED;
> >  	}
> >  
> > +	/* Handle device/host interrupt */
> > +	if (ci->role != CI_ROLE_END)
> > +		ret = ci_role(ci)->irq(ci);
> > +
> >  	return ret;
> >  }
> >  
> > @@ -397,6 +515,7 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
> >  	struct resource	*res;
> >  	void __iomem	*base;
> >  	int		ret;
> > +	u32		otgsc;
> >  
> >  	if (!dev->platform_data) {
> >  		dev_err(dev, "platform data missing\n");
> > @@ -421,6 +540,7 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
> >  		return -ENOMEM;
> >  	}
> >  
> > +	ci->events = 0;
> 
> "ci" has just been kzalloc'ed.
will change.
> 
> >  	ci->dev = dev;
> >  	ci->platdata = dev->platform_data;
> >  	if (ci->platdata->phy)
> > @@ -442,12 +562,20 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
> >  		return -ENODEV;
> >  	}
> >  
> > -	INIT_WORK(&ci->work, ci_role_work);
> > +	INIT_WORK(&ci->work, ci_otg_work);
> > +	INIT_DELAYED_WORK(&ci->dwork, ci_delayed_work);
> >  	ci->wq = create_singlethread_workqueue("ci_otg");
> >  	if (!ci->wq) {
> >  		dev_err(dev, "can't create workqueue\n");
> >  		return -ENODEV;
> >  	}
> > +	/* Disable all interrupts bits */
> > +	hw_write(ci, OP_USBINTR, 0xffffffff, 0);
> > +	hw_write(ci, OP_OTGSC, OTGSC_INT_EN_BITS, 0);
> > +
> > +	/* Clear all interrupts status bits*/
> > +	hw_write(ci, OP_USBSTS, 0xffffffff, 0xffffffff);
> > +	hw_write(ci, OP_OTGSC, OTGSC_INT_STATUS_BITS, OTGSC_INT_STATUS_BITS);
> 
> How about moving these to hw_device_init()?

Good idea
> 
> >  
> >  	/* initialize role(s) before the interrupt is requested */
> >  	ret = ci_hdrc_host_init(ci);
> > @@ -465,6 +593,7 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
> >  	}
> >  
> >  	if (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]) {
> > +		dev_dbg(dev, "support otg\n");
> >  		ci->is_otg = true;
> >  		/* ID pin needs 1ms debouce time, we delay 2ms for safe */
> >  		mdelay(2);
> > @@ -475,13 +604,29 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
> >  			: CI_ROLE_GADGET;
> >  	}
> >  
> > +	if (ci->is_otg)
> > +		/* if otg is supported, create struct usb_otg */
> > +		ci_hdrc_otg_init(ci);
> 
> Can this just go the same block where is_otg is set?
Will change
> 
> > +
> >  	ret = ci_role_start(ci, ci->role);
> >  	if (ret) {
> > -		dev_err(dev, "can't start %s role\n", ci_role(ci)->name);
> > +		dev_err(dev, "can't start %s role, ret=%d\n",
> > +				ci_role(ci)->name, ret);
> 
> This change is not really related to the rest of the patch.
It just adds return value output, it can let the user know what kinds of
error occurs during the ci_role_start, meanwhile, it is a small change,
and not deserved a patch.
> 
> >  		ret = -ENODEV;
> >  		goto rm_wq;
> >  	}
> >  
> > +	otgsc = hw_read(ci, OP_OTGSC, ~0);
> > +	/*
> > +	 * if it is device mode:
> > +	 * - Enable vbus detect
> > +	 * - If it has already connected to host, notify udc
> > +	 */
> > +	if (ci->role == CI_ROLE_GADGET) {
> > +		hw_write(ci, OP_OTGSC, OTGSC_BSVIE, OTGSC_BSVIE);
> > +		ci_handle_vbus_change(ci);
> > +	}
> 
> This looks like something that belongs to gadget role start, doesn't it?
Yes, will change.
> 

-- 

Best Regards,
Peter Chen