[PATCH v11 0/9] Add tested id switch and vbus connect detect support for Chipidea

[PATCH v11 0/9] Add tested id switch and vbus connect detect support for Chipidea

[Peter Chen]

Changes for v11:
- mark ci_handle_vbus_change as static as it is only used at core.c
[3/9]
- Move the vbus operation for platform code to host code, as vbus
operation is common operation, and host is the only user for vbus.
When it is host mode, we need to open vbus, when it is out of host
mode, we need to close vbus. [6/9] [8/9]
- Delete the delayed work at core.c as it is not needed. [7/9]

Changes for v10:
- Delete [8/9] at v9, ci core's drvdata must be set for further operation.
[8/8]

Changes for v9:
- Some small comments from Alex like: variable comment for otg event
additional newline. [3/9]
- Import function tell show if the controller has otg capable, if
the controller supports both host and device, we think it is otg
capable, and can read OTGSC. [3/9]
- Merge two otg patches [v8 3/8] and [v8 4/8] to one [v9 3/9]. [3/9]
- Add inline to ci_hdrc_gadget_destroy if CONFIG_USB_CHIPIDEA_UDC
is not defined, it can fix one build warning "defined but not used"
[3/9]
- One comment from Felipe about changing calling gadget disconnect
API at chipidea's udc driver. I move calling ci->driver->disconnect
from _gadget_stop_activity to which calls _gadget_stop_activity except
ci13xxx_stop, as udc core will call disconnect when do rmmod gadget. [7/9]
- Add ci core probe's return value to ci's platform_data, we do this
for getting core's probe's result at platform layer, and quit it
if the core's probe fails. [8/9] [9/9]

Changes for v8:
- Add ci_supports_gadget helper to know if the controller
supports gadget, if the controller supports gadget, it
needs to read otgsc to know the vbus value, basically,
if the controller supports gadget, it will support host
as well [3/8]
- At ci_hdrc_probe, it needs to add free memory at error path
[3/8]
- Cosolidate ci->driver = NULL at ci13xxx_stop
[8/8]

Changes for v7:
For Patch 8/8, we only need to set ci->driver to NULL when usb cable
is not connected, for other changes, it will case some runtime pm
unmatch and un-align with udc-core & composite driver problems.

Changes for v6:
- Add Alex comments for init/destroy function [3/8] [4/8]
- Remove memset(&ci->gadget, 0, sizeof(ci->gadget)) at destory function [4/8]
- Add Kishon's comment: Change the format of struct usb_otg otg at drivers/usb/chipidea/ci.h
[1/8]
- Add comments for CI_VBUS_STABLE_TIMEOUT [3/8]
- Change the otg_set_peripheral return value check as the fully
chipidea driver users don't need it. [4/8]
- Fix one bug that the oops when re-plug in usb cable after
rmmod gadget [8/8]

Anyone who has tested this patchset, please give a tested-by, thanks.

This patchset adds 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 patch is fully tested at imx6 sabresd platform.
My chipidea repo: https://github.com/hzpeterchen/linux-usb.git
which is rebased of Sascha's git 
(git://git.pengutronix.de/git/imx/linux-2.6.git tags/usb-chipidea-otg-for-next)
and usb-3.9-rc1 from grep's usb-next.

At sascha's tag, it finishes some dr_mode and phy_type patches,
in that way, we can choose host-only and gadget-only function at
dts file as well as we can choose different phy setting.

Besides sascha's work, there are some Michael Grzeschik/Marc Kleine-Budde
usbmisc patch, imx chipidea suspend/resume patch, and one pin control
patch for imx6

Peter Chen (9):
  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: udc: add pullup/pulldown dp at hw_device_state
  usb: chipidea: udc: retire the flag CI13_PULLUP_ON_VBUS
  usb: chipidea: add vbus regulator control
  usb: chipidea: delete the delayed work
  usb: chipidea: imx: add getting vbus regulator code
  usb: chipidea: udc: fix the oops when plugs in usb cable after rmmod
    gadget

 drivers/usb/chipidea/Makefile      |    2 +-
 drivers/usb/chipidea/bits.h        |   10 ++
 drivers/usb/chipidea/ci.h          |   11 ++-
 drivers/usb/chipidea/ci13xxx_imx.c |   32 ++------
 drivers/usb/chipidea/ci13xxx_msm.c |    1 -
 drivers/usb/chipidea/core.c        |  166 +++++++++++++++++++++++++++++++-----
 drivers/usb/chipidea/host.c        |   29 ++++++-
 drivers/usb/chipidea/host.h        |    6 +-
 drivers/usb/chipidea/otg.c         |   68 +++++++++++++++
 drivers/usb/chipidea/otg.h         |    9 ++
 drivers/usb/chipidea/udc.c         |  118 ++++++++++++--------------
 drivers/usb/chipidea/udc.h         |    4 +
 include/linux/usb/chipidea.h       |    2 +-
 13 files changed, 341 insertions(+), 117 deletions(-)
 create mode 100644 drivers/usb/chipidea/otg.c
 create mode 100644 drivers/usb/chipidea/otg.h

[PATCH v11 1/9] 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>
---
 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 v11 2/9] 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..697e369 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 v11 3/9] 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.

- vbus interrupt needs to be handled when gadget function is enabled

- Create/destroy the gadget at udc's init and destory function

- start/stop API are used at otg id switch and probe routine

- Defer some gadget operations at ci's delayed work queue

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/bits.h        |   10 ++
 drivers/usb/chipidea/ci.h          |    7 +-
 drivers/usb/chipidea/ci13xxx_imx.c |    3 +-
 drivers/usb/chipidea/core.c        |  190 ++++++++++++++++++++++++++++++++----
 drivers/usb/chipidea/host.c        |    6 +
 drivers/usb/chipidea/host.h        |    6 +-
 drivers/usb/chipidea/otg.c         |   28 ++++--
 drivers/usb/chipidea/otg.h         |    3 +
 drivers/usb/chipidea/udc.c         |   39 ++++++--
 drivers/usb/chipidea/udc.h         |    4 +
 10 files changed, 254 insertions(+), 42 deletions(-)

diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
index d8ffc2f..58ef56c 100644
--- a/drivers/usb/chipidea/bits.h
+++ b/drivers/usb/chipidea/bits.h
@@ -77,11 +77,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 697e369..a3777a1 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -130,6 +130,9 @@ struct hw_bank {
  * @transceiver: pointer to USB PHY, if any
  * @hcd: pointer to usb_hcd for ehci host driver
  * @otg: for otg support
+ * @id_event: indicates there is a id event, and handled at ci_otg_work
+ * @b_sess_valid_event: indicates there is a vbus event, and handled
+ *@ci_otg_work
  */
 struct ci13xxx {
 	struct device			*dev;
@@ -140,6 +143,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 +170,8 @@ struct ci13xxx {
 	struct usb_phy			*transceiver;
 	struct usb_hcd			*hcd;
 	struct usb_otg      		otg;
+	bool				id_event;
+	bool				b_sess_valid_event;
 };
 
 static inline struct ci_role_driver *ci_role(struct ci13xxx *ci)
@@ -201,7 +207,6 @@ static inline void ci_role_stop(struct ci13xxx *ci)
 
 	ci->roles[role]->stop(ci);
 }
-
 /******************************************************************************
  * REGISTERS
  *****************************************************************************/
diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
index 136869b..793fdba 100644
--- a/drivers/usb/chipidea/ci13xxx_imx.c
+++ b/drivers/usb/chipidea/ci13xxx_imx.c
@@ -110,7 +110,8 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
 	pdata->capoffset = DEF_CAPOFFSET;
 	pdata->flags = CI13XXX_REQUIRE_TRANSCEIVER |
 		       CI13XXX_PULLUP_ON_VBUS |
-		       CI13XXX_DISABLE_STREAMING;
+		       CI13XXX_DISABLE_STREAMING | 
+		       CI13XXX_REGS_SHARED;
 
 	pdata->phy_mode = of_usb_get_phy_mode(pdev->dev.of_node);
 	pdata->dr_mode = of_usb_get_dr_mode(pdev->dev.of_node);
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index c89f2aa..c563e17 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -75,6 +75,7 @@
 #include "bits.h"
 #include "host.h"
 #include "debug.h"
+#include "otg.h"
 
 /* Controller register map */
 static uintptr_t ci_regs_nolpm[] = {
@@ -201,6 +202,14 @@ static int hw_device_init(struct ci13xxx *ci, void __iomem *base)
 	if (ci->hw_ep_max > ENDPT_MAX)
 		return -ENODEV;
 
+	/* Disable all interrupts bits */
+	hw_write(ci, OP_USBINTR, 0xffffffff, 0);
+	ci_disable_otg_interrupt(ci, OTGSC_INT_EN_BITS);
+
+	/* Clear all interrupts status bits*/
+	hw_write(ci, OP_USBSTS, 0xffffffff, 0xffffffff);
+	ci_clear_otg_interrupt(ci, OTGSC_INT_STATUS_BITS);
+
 	dev_dbg(ci->dev, "ChipIdea HDRC found, lpm: %d; cap: %p op: %p\n",
 		ci->hw_bank.lpm, ci->hw_bank.cap, ci->hw_bank.op);
 
@@ -302,24 +311,131 @@ 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;
+}
+
+/* 
+ * Since there are some capacitances at vbus, the vbus may not
+ * change very quickly when we stop/start internal 5v.
+ * Below is a vbus stable timeout value, the routine will quit
+ * if the vbus gets the required value, we can think there are some
+ * problems for hardware if the vbus can't get the required value
+ * within 5 seconds.
+ */
+#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);
 
+		/* 1. Finish the current role */
 		ci_role_stop(ci);
+		hw_device_reset(ci, USBMODE_CM_IDLE);
+
+		/* 2. Turn on/off vbus according to coming role */
+		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 (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,
+					CI_VBUS_STABLE_TIMEOUT);
+		}
+
+		/* 3. Begin the new role */
 		ci_role_start(ci, role);
-		enable_irq(ci->irq);
 	}
 }
 
+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 (ci->id_event) {
+		ci->id_event = false;
+		ci_handle_id_switch(ci);
+	} else if (ci->b_sess_valid_event) {
+		ci->b_sess_valid_event = false;
+		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);
+
+	if (ci->role == CI_ROLE_GADGET) {
+		/*
+		 * if it is device mode:
+		 * - Enable vbus detect
+		 * - If it has already connected to host, notify udc
+		 */
+		ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
+		ci_handle_vbus_change(ci);
+	} else if (ci->is_otg && (ci->role == CI_ROLE_HOST)) {
+		/* USB Device at the MicroB to A cable */
+		otg_set_vbus(&ci->otg, true);
+	}
+}
+
+static inline void ci_role_destroy(struct ci13xxx *ci)
+{
+	ci_hdrc_gadget_destroy(ci);
+	ci_hdrc_host_destroy(ci);
+}
+
 static ssize_t show_role(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
@@ -352,25 +468,50 @@ static ssize_t store_role(struct device *dev, struct device_attribute *attr,
 
 static DEVICE_ATTR(role, S_IRUSR | S_IWUSR, show_role, store_role);
 
+static bool ci_is_otg_capable(struct ci13xxx *ci)
+{
+	return hw_read(ci, CAP_DCCPARAMS, DCCPARAMS_DC | DCCPARAMS_HC)
+	       	== (DCCPARAMS_DC | DCCPARAMS_HC);
+}
+
 static irqreturn_t ci_irq(int irq, void *data)
 {
 	struct ci13xxx *ci = data;
 	irqreturn_t ret = IRQ_NONE;
 	u32 otgsc = 0;
 
-	if (ci->is_otg)
+	if (ci_is_otg_capable(ci))
 		otgsc = hw_read(ci, OP_OTGSC, ~0);
 
-	if (ci->role != CI_ROLE_END)
-		ret = ci_role(ci)->irq(ci);
+	/*
+	 * Handle id change interrupt, it indicates device/host function
+	 * switch.
+	 */
+	if (ci->is_otg && (otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS)) {
+		ci->id_event = true;
+		ci_clear_otg_interrupt(ci, OTGSC_IDIS);
+		disable_irq_nosync(ci->irq);
+		queue_work(ci->wq, &ci->work);
+		return IRQ_HANDLED;
+	}
 
-	if (ci->is_otg && (otgsc & OTGSC_IDIS)) {
-		hw_write(ci, OP_OTGSC, OTGSC_IDIS, OTGSC_IDIS);
+	/*
+	 * Handle vbus change interrupt, it indicates device connection
+	 * and disconnection events.
+	 */
+	if (ci_is_otg_capable(ci) && (otgsc & OTGSC_BSVIE)
+			&& (otgsc & OTGSC_BSVIS)) {
+		ci->b_sess_valid_event = true;
+		ci_clear_otg_interrupt(ci, OTGSC_BSVIS);
 		disable_irq_nosync(ci->irq);
 		queue_work(ci->wq, &ci->work);
-		ret = IRQ_HANDLED;
+		return IRQ_HANDLED;
 	}
 
+	/* Handle device/host interrupt */
+	if (ci->role != CI_ROLE_END)
+		ret = ci_role(ci)->irq(ci);
+
 	return ret;
 }
 
@@ -481,7 +622,8 @@ static int 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");
@@ -512,7 +654,10 @@ static int 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;
+		/* if otg is supported, create struct usb_otg */
+		ci_hdrc_otg_init(ci);
 		/* ID pin needs 1ms debouce time, we delay 2ms for safe */
 		mdelay(2);
 		ci->role = ci_otg_role(ci);
@@ -528,7 +673,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
 	if (ret) {
 		dev_err(dev, "can't start %s role\n", ci_role(ci)->name);
 		ret = -ENODEV;
-		goto rm_wq;
+		goto free_memory;
 	}
 
 	platform_set_drvdata(pdev, ci);
@@ -539,17 +684,22 @@ static int ci_hdrc_probe(struct platform_device *pdev)
 
 	ret = device_create_file(dev, &dev_attr_role);
 	if (ret)
-		goto rm_attr;
+		goto free_irq;
 
-	if (ci->is_otg)
-		hw_write(ci, OP_OTGSC, OTGSC_IDIE, OTGSC_IDIE);
+	/* Defer some operations */
+	queue_delayed_work(ci->wq, &ci->dwork, msecs_to_jiffies(200));
 
 	return ret;
 
-rm_attr:
-	device_remove_file(dev, &dev_attr_role);
+free_irq:
+	free_irq(ci->irq, ci);
 stop:
-	ci_role_stop(ci);
+	ci_role_destroy(ci);
+free_memory:
+	if (ci->roles[CI_ROLE_HOST])
+		devm_kfree(dev, ci->roles[CI_ROLE_HOST]);
+	if (ci->roles[CI_ROLE_GADGET])
+		devm_kfree(dev, ci->roles[CI_ROLE_GADGET]);
 rm_wq:
 	flush_workqueue(ci->wq);
 	destroy_workqueue(ci->wq);
@@ -565,7 +715,7 @@ static int 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 8e9d312..ead3158 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -105,3 +105,9 @@ int ci_hdrc_host_init(struct ci13xxx *ci)
 
 	return 0;
 }
+
+void ci_hdrc_host_destroy(struct ci13xxx *ci)
+{
+	if (ci->role == CI_ROLE_HOST)
+		host_stop(ci);
+}
diff --git a/drivers/usb/chipidea/host.h b/drivers/usb/chipidea/host.h
index 761fb1f..19cc72d 100644
--- a/drivers/usb/chipidea/host.h
+++ b/drivers/usb/chipidea/host.h
@@ -4,13 +4,17 @@
 #ifdef CONFIG_USB_CHIPIDEA_HOST
 
 int ci_hdrc_host_init(struct ci13xxx *ci);
-
+void ci_hdrc_host_destroy(struct ci13xxx *ci);
 #else
 
 static inline int ci_hdrc_host_init(struct ci13xxx *ci)
 {
 	return -ENXIO;
 }
+static inline void ci_hdrc_host_destroy(struct ci13xxx *ci)
+{
+
+}
 
 #endif
 
diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
index 7dea3b3..2986d91 100644
--- a/drivers/usb/chipidea/otg.c
+++ b/drivers/usb/chipidea/otg.c
@@ -10,21 +10,28 @@
  * 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/gadget.h>
 #include <linux/usb/chipidea.h>
 
 #include "ci.h"
-#include "udc.h"
 #include "bits.h"
-#include "host.h"
-#include "debug.h"
+
+void ci_clear_otg_interrupt(struct ci13xxx *ci, u32 bits)
+{
+	/* Only clear request bits */
+	hw_write(ci, OP_OTGSC, OTGSC_INT_STATUS_BITS, bits);
+}
+
+void ci_enable_otg_interrupt(struct ci13xxx *ci, u32 bits)
+{
+	hw_write(ci, OP_OTGSC, bits, bits);
+}
+
+void ci_disable_otg_interrupt(struct ci13xxx *ci, u32 bits)
+{
+	hw_write(ci, OP_OTGSC, bits, 0);
+}
 
 static int ci_otg_set_peripheral(struct usb_otg *otg,
 		struct usb_gadget *periph)
@@ -55,6 +62,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;
+	ci_enable_otg_interrupt(ci, OTGSC_IDIE);
 
 	return 0;
 }
diff --git a/drivers/usb/chipidea/otg.h b/drivers/usb/chipidea/otg.h
index b4c6b3e..fa30428 100644
--- a/drivers/usb/chipidea/otg.h
+++ b/drivers/usb/chipidea/otg.h
@@ -2,5 +2,8 @@
 #define __DRIVERS_USB_CHIPIDEA_OTG_H
 
 int ci_hdrc_otg_init(struct ci13xxx *ci);
+void ci_clear_otg_interrupt(struct ci13xxx *ci, u32 bits);
+void ci_enable_otg_interrupt(struct ci13xxx *ci, u32 bits);
+void ci_disable_otg_interrupt(struct ci13xxx *ci, u32 bits);
 
 #endif /* __DRIVERS_USB_CHIPIDEA_OTG_H */
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index d214448..e82dae4 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -31,6 +31,7 @@
 
 #include "ci.h"
 #include "udc.h"
+#include "otg.h"
 #include "bits.h"
 #include "debug.h"
 
@@ -1371,6 +1372,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 +1380,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");
 		}
 	}
 
@@ -1741,7 +1744,12 @@ static int udc_start(struct ci13xxx *ci)
 	if (!IS_ERR_OR_NULL(ci->transceiver)) {
 		retval = otg_set_peripheral(ci->transceiver->otg,
 						&ci->gadget);
-		if (retval)
+		/* 
+		 * If we implement all USB functions using chipidea drivers,
+		 * it doesn't need to call above API, meanwhile, if we only
+		 * use gadget function, calling above API is useless.
+		 */
+		if (retval && retval != -ENOTSUPP)
 			goto remove_dbg;
 	}
 
@@ -1778,12 +1786,27 @@ free_qh_pool:
 	return retval;
 }
 
+static int udc_id_switch_for_device(struct ci13xxx *ci)
+{
+	ci_clear_otg_interrupt(ci, OTGSC_BSVIS);
+	ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
+
+	return 0;
+}
+
+static void udc_id_switch_for_host(struct ci13xxx *ci)
+{
+	/* host doesn't care B_SESSION_VALID event */
+	ci_clear_otg_interrupt(ci, OTGSC_BSVIS);
+	ci_disable_otg_interrupt(ci, OTGSC_BSVIE);
+}
+
 /**
- * udc_remove: parent remove must call this to remove UDC
+ * ci_hdrc_gadget_destroy: parent remove must call this to remove UDC
  *
  * No interrupts active, the IRQ has been released
  */
-static void udc_stop(struct ci13xxx *ci)
+void ci_hdrc_gadget_destroy(struct ci13xxx *ci)
 {
 	if (ci == NULL)
 		return;
@@ -1802,15 +1825,13 @@ static void udc_stop(struct ci13xxx *ci)
 	}
 	dbg_remove_files(&ci->gadget.dev);
 	device_unregister(&ci->gadget.dev);
-	/* my kobject is dynamic, I swear! */
-	memset(&ci->gadget, 0, sizeof(ci->gadget));
 }
 
 /**
  * ci_hdrc_gadget_init - initialize device related bits
  * ci: the controller
  *
- * This function enables the gadget role, if the device is "device capable".
+ * This function initializes gadget, if the device is "device capable".
  */
 int ci_hdrc_gadget_init(struct ci13xxx *ci)
 {
@@ -1823,11 +1844,11 @@ int ci_hdrc_gadget_init(struct ci13xxx *ci)
 	if (!rdrv)
 		return -ENOMEM;
 
-	rdrv->start	= udc_start;
-	rdrv->stop	= udc_stop;
+	rdrv->start	= udc_id_switch_for_device;
+	rdrv->stop	= udc_id_switch_for_host;
 	rdrv->irq	= udc_irq;
 	rdrv->name	= "gadget";
 	ci->roles[CI_ROLE_GADGET] = rdrv;
 
-	return 0;
+	return udc_start(ci);
 }
diff --git a/drivers/usb/chipidea/udc.h b/drivers/usb/chipidea/udc.h
index 4ff2384..6553e4d 100644
--- a/drivers/usb/chipidea/udc.h
+++ b/drivers/usb/chipidea/udc.h
@@ -80,6 +80,7 @@ struct ci13xxx_req {
 #ifdef CONFIG_USB_CHIPIDEA_UDC
 
 int ci_hdrc_gadget_init(struct ci13xxx *ci);
+void ci_hdrc_gadget_destroy(struct ci13xxx *ci);
 
 #else
 
@@ -87,7 +88,10 @@ static inline int ci_hdrc_gadget_init(struct ci13xxx *ci)
 {
 	return -ENXIO;
 }
+static inline void ci_hdrc_gadget_destroy(struct ci13xxx *ci)
+{
 
+}
 #endif
 
 #endif /* __DRIVERS_USB_CHIPIDEA_UDC_H */
-- 
1.7.0.4

[PATCH v11 4/9] 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 e82dae4..70f9f2d 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -91,8 +91,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;
 }
@@ -1424,10 +1426,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 -EPERM;
+
 	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 v11 5/9] 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 793fdba..3ed119e 100644
--- a/drivers/usb/chipidea/ci13xxx_imx.c
+++ b/drivers/usb/chipidea/ci13xxx_imx.c
@@ -109,7 +109,6 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
 	pdata->name = "ci13xxx_imx";
 	pdata->capoffset = DEF_CAPOFFSET;
 	pdata->flags = CI13XXX_REQUIRE_TRANSCEIVER |
-		       CI13XXX_PULLUP_ON_VBUS |
 		       CI13XXX_DISABLE_STREAMING | 
 		       CI13XXX_REGS_SHARED;
 
diff --git a/drivers/usb/chipidea/ci13xxx_msm.c b/drivers/usb/chipidea/ci13xxx_msm.c
index 7d16681..5755ee8 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 70f9f2d..da504a8 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -1360,9 +1360,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)
@@ -1426,8 +1423,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 -EPERM;
 
 	if (is_on)
@@ -1551,14 +1547,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);
@@ -1581,8 +1575,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 b314647..d543e4a 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -18,7 +18,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)
 	enum usb_dr_mode	dr_mode;
 #define CI13XXX_CONTROLLER_RESET_EVENT		0
-- 
1.7.0.4

[PATCH v11 6/9] usb: chipidea: add vbus regulator control

[Peter Chen]

For boards which have board level vbus control (eg, gpio), we
need to operation vbus according to below rules:
- For host, the vbus should always be on.
- For otg, the vbus needs to be turned on/off when usb role switches.

We put vbus operation to host as host is the only vbus user,
When we are at host mode, the vbus is on, when we are not at
host mode, vbus should be off.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/ci.h    |    2 ++
 drivers/usb/chipidea/core.c  |   24 ++++++++----------------
 drivers/usb/chipidea/host.c  |   23 ++++++++++++++++++++++-
 include/linux/usb/chipidea.h |    1 +
 4 files changed, 33 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index a3777a1..8826cdb 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -133,6 +133,7 @@ struct hw_bank {
  * @id_event: indicates there is a id event, and handled at ci_otg_work
  * @b_sess_valid_event: indicates there is a vbus event, and handled
  * at ci_otg_work
+ * @reg_vbus: used to control internal vbus regulator
  */
 struct ci13xxx {
 	struct device			*dev;
@@ -172,6 +173,7 @@ struct ci13xxx {
 	struct usb_otg      		otg;
 	bool				id_event;
 	bool				b_sess_valid_event;
+	struct regulator		*reg_vbus;
 };
 
 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 c563e17..e0ff335 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -63,6 +63,7 @@
 #include <linux/kernel.h>
 #include <linux/slab.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb/otg.h>
@@ -364,17 +365,10 @@ static void ci_handle_id_switch(struct ci13xxx *ci)
 		hw_device_reset(ci, USBMODE_CM_IDLE);
 
 		/* 2. Turn on/off vbus according to coming role */
-		if (role == CI_ROLE_GADGET) {
-			otg_set_vbus(&ci->otg, false);
+		if (role == CI_ROLE_GADGET)
 			/* wait vbus lower than OTGSC_BSV */
 			hw_wait_reg(ci, OP_OTGSC, OTGSC_BSV, 0,
 					CI_VBUS_STABLE_TIMEOUT);
-		} 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,
-					CI_VBUS_STABLE_TIMEOUT);
-		}
 
 		/* 3. Begin the new role */
 		ci_role_start(ci, role);
@@ -416,17 +410,14 @@ 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);
 
+	/*
+	 * If it is gadget mode, the vbus operation should be done like below:
+	 * 1. Enable vbus detect
+	 * 2. If it has already connected to host, notify udc
+	 */
 	if (ci->role == CI_ROLE_GADGET) {
-		/*
-		 * if it is device mode:
-		 * - Enable vbus detect
-		 * - If it has already connected to host, notify udc
-		 */
 		ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
 		ci_handle_vbus_change(ci);
-	} else if (ci->is_otg && (ci->role == CI_ROLE_HOST)) {
-		/* USB Device at the MicroB to A cable */
-		otg_set_vbus(&ci->otg, true);
 	}
 }
 
@@ -603,6 +594,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
 
 	ci->dev = dev;
 	ci->platdata = dev->platform_data;
+	ci->reg_vbus = ci->platdata->reg_vbus;
 	if (ci->platdata->phy)
 		ci->transceiver = ci->platdata->phy;
 	else
diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index ead3158..8efbd44 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -21,6 +21,7 @@
 
 #include <linux/kernel.h>
 #include <linux/io.h>
+#include <linux/regulator/consumer.h>
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
 #include <linux/usb/chipidea.h>
@@ -64,9 +65,19 @@ static int host_start(struct ci13xxx *ci)
 	ehci->caps = ci->hw_bank.cap;
 	ehci->has_hostpc = ci->hw_bank.lpm;
 
+	if (ci->reg_vbus) {
+		ret = regulator_enable(ci->reg_vbus);
+		if (ret) {
+			dev_err(ci->dev,
+				"Failed to enable vbus regulator, ret=%d\n",
+				ret);
+			goto put_hcd;
+		}
+	}
+
 	ret = usb_add_hcd(hcd, 0, 0);
 	if (ret)
-		usb_put_hcd(hcd);
+		goto disable_reg;
 	else
 		ci->hcd = hcd;
 
@@ -74,6 +85,14 @@ static int host_start(struct ci13xxx *ci)
 		hw_write(ci, OP_USBMODE, USBMODE_CI_SDIS, USBMODE_CI_SDIS);
 
 	return ret;
+
+disable_reg:
+	if (ci->reg_vbus)
+		regulator_disable(ci->reg_vbus);
+put_hcd:
+	usb_put_hcd(hcd);
+
+	return ret;
 }
 
 static void host_stop(struct ci13xxx *ci)
@@ -82,6 +101,8 @@ static void host_stop(struct ci13xxx *ci)
 
 	usb_remove_hcd(hcd);
 	usb_put_hcd(hcd);
+	if (ci->reg_vbus)
+		regulator_disable(ci->reg_vbus);
 }
 
 int ci_hdrc_host_init(struct ci13xxx *ci)
diff --git a/include/linux/usb/chipidea.h b/include/linux/usb/chipidea.h
index d543e4a..1430403 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -23,6 +23,7 @@ struct ci13xxx_platform_data {
 #define CI13XXX_CONTROLLER_RESET_EVENT		0
 #define CI13XXX_CONTROLLER_STOPPED_EVENT	1
 	void	(*notify_event) (struct ci13xxx *ci, unsigned event);
+	struct regulator *reg_vbus;
 };
 
 /* Default offset of capability registers */
-- 
1.7.0.4

[PATCH v11 7/9] usb: chipidea: delete the delayed work

[Peter Chen]

After moving vbus operation to host, we no matter need to delayed operation.
But, the connection notification to gadget is needed as gadget's start
is only enable vbus operation, but it doesn't need to delayed.

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/ci.h   |    1 -
 drivers/usb/chipidea/core.c |   22 +++-------------------
 2 files changed, 3 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index 8826cdb..35ca5ee 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -144,7 +144,6 @@ 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;
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index e0ff335..464e075 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -405,22 +405,6 @@ static void ci_otg_work(struct work_struct *work)
 	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);
-
-	/*
-	 * If it is gadget mode, the vbus operation should be done like below:
-	 * 1. Enable vbus detect
-	 * 2. If it has already connected to host, notify udc
-	 */
-	if (ci->role == CI_ROLE_GADGET) {
-		ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
-		ci_handle_vbus_change(ci);
-	}
-}
-
 static inline void ci_role_destroy(struct ci13xxx *ci)
 {
 	ci_hdrc_gadget_destroy(ci);
@@ -615,7 +599,6 @@ static int ci_hdrc_probe(struct platform_device *pdev)
 	}
 
 	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");
@@ -678,8 +661,9 @@ static int ci_hdrc_probe(struct platform_device *pdev)
 	if (ret)
 		goto free_irq;
 
-	/* Defer some operations */
-	queue_delayed_work(ci->wq, &ci->dwork, msecs_to_jiffies(200));
+	/* If it is connected to host, tell gadget the connection */
+	if (ci->role == CI_ROLE_GADGET)
+		ci_handle_vbus_change(ci);
 
 	return ret;
 
-- 
1.7.0.4

[PATCH v11 8/9] usb: chipidea: imx: add getting vbus regulator code

[Peter Chen]

Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
 drivers/usb/chipidea/ci13xxx_imx.c |   28 +++++-----------------------
 1 files changed, 5 insertions(+), 23 deletions(-)

diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
index 3ed119e..b2d8350 100644
--- a/drivers/usb/chipidea/ci13xxx_imx.c
+++ b/drivers/usb/chipidea/ci13xxx_imx.c
@@ -33,7 +33,6 @@ struct ci13xxx_imx_data {
 	struct usb_phy *phy;
 	struct platform_device *ci_pdev;
 	struct clk *clk;
-	struct regulator *reg_vbus;
 };
 
 static const struct usbmisc_ops *usbmisc_ops;
@@ -163,20 +162,9 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
 		data->phy = phy;
 	}
 
-	/* 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 err_clk;
-		}
-		data->reg_vbus = reg_vbus;
-	} else {
-		reg_vbus = NULL;
-	}
+	if (!IS_ERR(reg_vbus))
+		pdata->reg_vbus = reg_vbus;
 
 	pdata->phy = data->phy;
 
@@ -186,7 +174,7 @@ static int 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 err_clk;
 		}
 		*pdev->dev.dma_mask = DMA_BIT_MASK(32);
 		dma_set_coherent_mask(&pdev->dev, *pdev->dev.dma_mask);
@@ -197,7 +185,7 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
 		if (ret) {
 			dev_err(&pdev->dev,
 				"usbmisc init failed, ret=%d\n", ret);
-			goto err;
+			goto err_clk;
 		}
 	}
 
@@ -209,7 +197,7 @@ static int 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 err_clk;
 	}
 
 	data->ci_pdev = plat_ci;
@@ -220,9 +208,6 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
 
 	return 0;
 
-err:
-	if (reg_vbus)
-		regulator_disable(reg_vbus);
 err_clk:
 	clk_disable_unprepare(data->clk);
 	return ret;
@@ -235,9 +220,6 @@ static int ci13xxx_imx_remove(struct platform_device *pdev)
 	pm_runtime_disable(&pdev->dev);
 	ci13xxx_remove_device(data->ci_pdev);
 
-	if (data->reg_vbus)
-		regulator_disable(data->reg_vbus);
-
 	if (data->phy) {
 		usb_phy_shutdown(data->phy);
 		module_put(data->phy->dev->driver->owner);
-- 
1.7.0.4

[PATCH v11 9/9] usb: chipidea: udc: fix the oops when plugs in usb cable after rmmod gadget

[Peter Chen]

When we rmmod gadget, the ci->driver needs to be cleared.
Otherwise, we plug in usb cable again, the driver will
consider gadget is there, in fact, it is removed.

Besides, consolidate the calling of ci->driver->disconnect, when
we do rmmod gadget, the gadget's disconnect should be called from
udc core.

Below is the oops this patch fixes.

root at freescale ~$ ci_hdrc ci_hdrc.0: Connected to host
Unable to handle kernel paging request at virtual address 7f01171c
pgd = 80004000
[7f01171c] *pgd=4fa1e811, *pte=00000000, *ppte=00000000
Internal error: Oops: 7 [#1] SMP ARM
Modules linked in: f_acm libcomposite u_serial [last unloaded: g_serial]
CPU: 0    Not tainted  (3.8.0-rc5+ #221)
	PC is at _gadget_stop_activity+0xbc/0x128
	LR is at ep_fifo_flush+0x68/0xa0
	pc : [<803634cc>]    lr : [<80363938>]    psr: 20000193
	sp : 806c7dc8  ip : 00000000  fp : 806c7de4
	r10: 00000000  r9 : 80710ea4  r8 : 00000000
	r7 : bf834094  r6 : bf834098  r5 : bf834010  r4 : bf8340a0
	r3 : 7f011708  r2 : 01940194  r1 : a0000193  r0 : bf834014
	Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment kernel
	Control: 10c53c7d  Table: 4f06404a  DAC: 00000017
	Process swapper/0 (pid: 0, stack limit = 0x806c6238)
	Stack: (0x806c7dc8 to 0x806c8000)
	7dc0:                   bf834010 bf809950 bf834010 0000004b 806c7e44 806c7de8
	7de0: 80365400 8036341c 0000ec1c 00000000 0000ec1c 00000040 fff5ede0 bf834014
	7e00: 000a1220 00000000 0000002e ffffd958 806c7e3c 806c7e20 8004e870 bf834010
	7e20: bf809950 0000004b 0000004b 00000000 80710ea4 00000000 806c7e5c 806c7e48
	7e40: 80362888 80365154 bfb35180 bf809950 806c7e9c 806c7e60 8008111c 803627f4
	7e60: 00989680 00000000 bf809900 bf809964 812df8c0 bf809900 bf809950 806c3f2c
	7e80: 0000004b 00000000 412fc09a 00000000 806c7eb4 806c7ea0 80081368 800810b8
	7ea0: bf809900 bf809950 806c7ecc 806c7eb8 800843c8 8008130c 0000004b 806cf748
	7ec0: 806c7ee4 806c7ed0 80081088 8008432c 806c6000 806cf748 806c7f0c 806c7ee8
	7ee0: 8000fa44 8008105c f400010c 806ce974 806c7f30 f4000110 806d29e8 412fc09a
	7f00: 806c7f2c 806c7f10 80008584 8000f9f4 8000fbd0 60000013 ffffffff 806c7f64
	7f20: 806c7f84 806c7f30 8000eac0 80008554 00000000 00000000 0000000f 8001aea0
	7f40: 806c6000 80712a48 804f0040 00000000 806d29e8 412fc09a 00000000 806c7f84
	7f60: 806c7f88 806c7f78 8000fbcc 8000fbd0 60000013 ffffffff 806c7fac 806c7f88
	7f80: 8001015c 8000fb9c 806cf5b0 80712980 806a4528 8fffffff 1000406a 412fc09a
	7fa0: 806c7fbc 806c7fb0 804e5334 800100ac 806c7ff4 806c7fc0 80668940 804e52d4
	7fc0: ffffffff ffffffff 806684bc 00000000 00000000 806a4528 10c53c7d 806ce94c
	7fe0: 806a4524 806d29dc 00000000 806c7ff8 10008078 806686b0 00000000 00000000
	Backtrace:
	[<80363410>] (_gadget_stop_activity+0x0/0x128) from [<80365400>] (udc_irq+0x2b8/0xf38)
	 r7:0000004b r6:bf834010 r5:bf809950 r4:bf834010
	 [<80365148>] (udc_irq+0x0/0xf38) from [<80362888>] (ci_irq+0xa0/0xf4)
	[<803627e8>] (ci_irq+0x0/0xf4) from [<8008111c>] (handle_irq_event_percpu+0x70/0x254)
	 r5:bf809950 r4:bfb35180
	 [<800810ac>] (handle_irq_event_percpu+0x0/0x254) from [<80081368>] (handle_irq_event+0x68/0x88)
	[<80081300>] (handle_irq_event+0x0/0x88) from [<800843c8>] (handle_fasteoi_irq+0xa8/0x178)
	 r5:bf809950 r4:bf809900
	 [<80084320>] (handle_fasteoi_irq+0x0/0x178) from [<80081088>] (generic_handle_irq+0x38/0x40)
	 r5:806cf748 r4:0000004b
	 [<80081050>] (generic_handle_irq+0x0/0x40) from [<8000fa44>] (handle_IRQ+0x5c/0xbc)
	 r5:806cf748 r4:806c6000
	 [<8000f9e8>] (handle_IRQ+0x0/0xbc) from [<80008584>] (gic_handle_irq+0x3c/0x70)
	 r9:412fc09a r8:806d29e8 r7:f4000110 r6:806c7f30 r5:806ce974
	 r4:f400010c
	 [<80008548>] (gic_handle_irq+0x0/0x70) from [<8000eac0>] (__irq_svc+0x40/0x54)
	Exception stack(0x806c7f30 to 0x806c7f78)
	7f20:                                     00000000 00000000 0000000f 8001aea0
	7f40: 806c6000 80712a48 804f0040 00000000 806d29e8 412fc09a 00000000 806c7f84
	7f60: 806c7f88 806c7f78 8000fbcc 8000fbd0 60000013 ffffffff
	 r7:806c7f64 r6:ffffffff r5:60000013 r4:8000fbd0
	 [<8000fb90>] (default_idle+0x0/0x4c) from [<8001015c>] (cpu_idle+0xbc/0xf8)
	[<800100a0>] (cpu_idle+0x0/0xf8) from [<804e5334>] (rest_init+0x6c/0x84)
	 r9:412fc09a r8:1000406a r7:8fffffff r6:806a4528 r5:80712980
	 r4:806cf5b0
	 [<804e52c8>] (rest_init+0x0/0x84) from [<80668940>] (start_kernel+0x29c/0x2ec)
	[<806686a4>] (start_kernel+0x0/0x2ec) from [<10008078>] (0x10008078)
	Code: e12fff33 e5953200 e3530000 0a000002 (e5933014)
	---[ end trace aade28ad434062bd ]---
	Kernel panic - not syncing: 0xbf8bdfa8)
	df60: 00000000 00000000 0000000f 8001aea0 bf8bc000 80712a48 804f0040 00000000
	df80: 806d29e8 412fc09a 00000000 bf8bdfb4 bf8bdfb8 bf8bdfa8 8000fbcc 8000fbd0
	dfa0: 60000013 ffffffff
	 r7:bf8bdf94 r6:ffffffff r5:60000013 r4:8000fbd0
	 [<8000fb90>] (default_idle+0x0/0x4c) from [<8001015c>] (cpu_idle+0xbc/0xf8)
	[<800100a0>] (cpu_idle+0x0/0xf8) from [<804e7930>] (secondary_start_kernel+0x120/0x148)
	 r9:412fc09a r8:1000406a r7:80712d20 r6:10c03c7d r5:00000002
	 r4:806e2008
	 [<804e7810>] (secondary_start_kernel+0x0/0x148) from [<104e7148>] (0x104e7148)
	 r5:0000001f r4:4f8a806a
	 CPU3: stopping
	 Backtrace:
	 [<800132e8>] (dump_backtrace+0x0/0x114) from [<804ea684>] (dump_stack+0x20/0x24)
	 r7:00000000 r6:806c3f2c r5:806cf748 r4:80712d18
	 [<804ea664>] (dump_stack+0x0/0x24) from [<80014adc>] (handle_IPI+0x140/0x18c)
	[<8001499c>] (handle_IPI+0x0/0x18c) from [<800085b0>] (gic_handle_irq+0x68/0x70)
	 r9:412fc09a r8:806d29e8 r7:f4000110 r6:bf8bff60 r5:806ce974
	 r4:f400010c
	 [<80008548>] (gic_handle_irq+0x0/0x70) from [<8000eac0>] (__irq_svc+0x40/0x54)
	Exception stack(0xbf8bff60 to 0xbf8bffa8)
	ff60: 00000000 00000000 0000000f 8001aea0 bf8be000 80712a48 804f0040 00000000
	ff80: 806d29e8 412fc09a 00000000 bf8bffb4 bf8bffb8 bf8bffa8 8000fbcc 8000fbd0
	ffa0: 60000013 ffffffff
	 r7:bf8bff94 r6:ffffffff r5:60000013 r4:8000fbd0
	 [<8000fb90>] (default_idle+0x0/0x4c) from [<8001015c>] (cpu_idle+0xbc/0xf8)
	[<800100a0>] (cpu_idle+0x0/0xf8) from [<804e7930>] (secondary_start_kernel+0x120/0x148)
	 r9:412fc09a r8:1000406a r7:80712d20 r6:10c03c7d r5:00000003
	 r4:806e2008
	 [<804e7810>] (secondary_start_kernel+0x0/0x148) from [<104e7148>] (0x104e7148)
	 r5:0000001f r4:4f8a806a
	 CPU1: stopping
	 Backtrace:
	 [<800132e8>] (dump_backtrace+0x0/0x114) from [<804ea684>] (dump_stack+0x20/0x24)
	 r7:00000000 r6:806c3f2c r5:806cf748 r4:80712d18
	 [<804ea664>] (dump_stack+0x0/0x24) from [<80014adc>] (handle_IPI+0x140/0x18c)
	[<8001499c>] (handle_IPI+0x0/0x18c) from [<800085b0>] (gic_handle_irq+0x68/0x70)
	 r9:412fc09a r8:806d29e8 r7:f4000110 r6:bf8bbf60 r5:806ce974
	 r4:f400010c
	 [<80008548>] (gic_handle_irq+0x0/0x70) from [<8000eac0>] (__irq_svc+0x40/0x54)
	Exception stack(0xbf8bbf60 to 0xbf8bbfa8)
	bf60: 00000000 00000000 0000000f 8001aea0 bf8ba000 80712a48 804f0040 00000000
	bf80: 806d29e8 412fc09a 00000000 bf8bbfb4 bf8bbfb8 bf8bbfa8 8000fbcc 8000fbd0
	bfa0: 60000013 ffffffff
	 r7:bf8bbf94 r6:ffffffff r5:60000013 r4:8000fbd0
	 [<8000fb90>] (default_idle+0x0/0x4c) from [<8001015c>] (cpu_idle+0xbc/0xf8)
	[<800100a0>] (cpu_idle+0x0/0xf8) from [<804e7930>] (secondary_start_kernel+0x120/0x148)
	 r9:412fc09a r8:1000406a r7:80712d20 r6:10c03c7d r5:00000001
	 r4:806e2008

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

diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index da504a8..339f8a1 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -579,9 +579,6 @@ static int _gadget_stop_activity(struct usb_gadget *gadget)
 	usb_ep_fifo_flush(&ci->ep0out->ep);
 	usb_ep_fifo_flush(&ci->ep0in->ep);
 
-	if (ci->driver)
-		ci->driver->disconnect(gadget);
-
 	/* make sure to disable all endpoints */
 	gadget_for_each_ep(ep, gadget) {
 		usb_ep_disable(ep);
@@ -612,6 +609,11 @@ __acquires(ci->lock)
 
 	dbg_event(0xFF, "BUS RST", 0);
 
+	if (ci->gadget.speed != USB_SPEED_UNKNOWN) {
+		if (ci->driver)
+			ci->driver->disconnect(&ci->gadget);
+	}
+
 	spin_unlock(&ci->lock);
 	retval = _gadget_stop_activity(&ci->gadget);
 	if (retval)
@@ -1373,6 +1375,8 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active)
 			hw_device_state(ci, ci->ep0out->qh.dma);
 			dev_dbg(ci->dev, "Connected to host\n");
 		} else {
+			if (ci->driver)
+				ci->driver->disconnect(&ci->gadget);
 			hw_device_state(ci, 0);
 			if (ci->platdata->notify_event)
 				ci->platdata->notify_event(ci,
@@ -1580,13 +1584,14 @@ static int ci13xxx_stop(struct usb_gadget *gadget,
 		if (ci->platdata->notify_event)
 			ci->platdata->notify_event(ci,
 			CI13XXX_CONTROLLER_STOPPED_EVENT);
-		ci->driver = NULL;
 		spin_unlock_irqrestore(&ci->lock, flags);
 		_gadget_stop_activity(&ci->gadget);
 		spin_lock_irqsave(&ci->lock, flags);
 		pm_runtime_put(&ci->gadget.dev);
 	}
 
+	ci->driver = NULL;
+
 	spin_unlock_irqrestore(&ci->lock, flags);
 
 	return 0;
-- 
1.7.0.4

[PATCH v11 8/9] usb: chipidea: imx: add getting vbus regulator code

[Felipe Balbi]

Hi,

On Wed, Mar 06, 2013 at 05:56:39PM +0800, Peter Chen wrote:
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
>  drivers/usb/chipidea/ci13xxx_imx.c |   28 +++++-----------------------
>  1 files changed, 5 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
> index 3ed119e..b2d8350 100644
> --- a/drivers/usb/chipidea/ci13xxx_imx.c
> +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> @@ -33,7 +33,6 @@ struct ci13xxx_imx_data {
>  	struct usb_phy *phy;
>  	struct platform_device *ci_pdev;
>  	struct clk *clk;
> -	struct regulator *reg_vbus;
>  };
>  
>  static const struct usbmisc_ops *usbmisc_ops;
> @@ -163,20 +162,9 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
>  		data->phy = phy;
>  	}
>  
> -	/* 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 err_clk;
> -		}
> -		data->reg_vbus = reg_vbus;
> -	} else {
> -		reg_vbus = NULL;
> -	}
> +	if (!IS_ERR(reg_vbus))
> +		pdata->reg_vbus = reg_vbus;

what you're doing here is wrong. chipidea core should be the one calling
devm_regulator_get().

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130306/14b1c271/attachment.sig>

[PATCH v11 4/9] usb: chipidea: udc: add pullup/pulldown dp at hw_device_state

[Felipe Balbi]

Hi,

On Wed, Mar 06, 2013 at 05:56:35PM +0800, Peter Chen wrote:
> - 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 e82dae4..70f9f2d 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -91,8 +91,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);

this patch doesn't make sense to me. What will happen is that you will
be enabling pullups when vbus_session() gets called and this might not
be what gadget driver wants.

You don't want to fiddle with that yourself since I'm changing the
framework so that gadget driver will always request pullups to be
enabled.

Alex, I don't think this patch is correct

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130306/5cf68fda/attachment.sig>

[PATCH v11 6/9] usb: chipidea: add vbus regulator control

[Felipe Balbi]

Hi,

On Wed, Mar 06, 2013 at 05:56:37PM +0800, Peter Chen wrote:
> For boards which have board level vbus control (eg, gpio), we
> need to operation vbus according to below rules:
> - For host, the vbus should always be on.
> - For otg, the vbus needs to be turned on/off when usb role switches.
> 
> We put vbus operation to host as host is the only vbus user,
> When we are at host mode, the vbus is on, when we are not at
> host mode, vbus should be off.
> 
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
>  drivers/usb/chipidea/ci.h    |    2 ++
>  drivers/usb/chipidea/core.c  |   24 ++++++++----------------
>  drivers/usb/chipidea/host.c  |   23 ++++++++++++++++++++++-
>  include/linux/usb/chipidea.h |    1 +
>  4 files changed, 33 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> index a3777a1..8826cdb 100644
> --- a/drivers/usb/chipidea/ci.h
> +++ b/drivers/usb/chipidea/ci.h
> @@ -133,6 +133,7 @@ struct hw_bank {
>   * @id_event: indicates there is a id event, and handled at ci_otg_work
>   * @b_sess_valid_event: indicates there is a vbus event, and handled
>   * at ci_otg_work
> + * @reg_vbus: used to control internal vbus regulator
>   */
>  struct ci13xxx {
>  	struct device			*dev;
> @@ -172,6 +173,7 @@ struct ci13xxx {
>  	struct usb_otg      		otg;
>  	bool				id_event;
>  	bool				b_sess_valid_event;
> +	struct regulator		*reg_vbus;
>  };
>  
>  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 c563e17..e0ff335 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -63,6 +63,7 @@
>  #include <linux/kernel.h>
>  #include <linux/slab.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/usb/ch9.h>
>  #include <linux/usb/gadget.h>
>  #include <linux/usb/otg.h>
> @@ -364,17 +365,10 @@ static void ci_handle_id_switch(struct ci13xxx *ci)
>  		hw_device_reset(ci, USBMODE_CM_IDLE);
>  
>  		/* 2. Turn on/off vbus according to coming role */
> -		if (role == CI_ROLE_GADGET) {
> -			otg_set_vbus(&ci->otg, false);
> +		if (role == CI_ROLE_GADGET)
>  			/* wait vbus lower than OTGSC_BSV */
>  			hw_wait_reg(ci, OP_OTGSC, OTGSC_BSV, 0,
>  					CI_VBUS_STABLE_TIMEOUT);
> -		} 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,
> -					CI_VBUS_STABLE_TIMEOUT);
> -		}
>  
>  		/* 3. Begin the new role */
>  		ci_role_start(ci, role);
> @@ -416,17 +410,14 @@ 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);
>  
> +	/*
> +	 * If it is gadget mode, the vbus operation should be done like below:
> +	 * 1. Enable vbus detect
> +	 * 2. If it has already connected to host, notify udc
> +	 */
>  	if (ci->role == CI_ROLE_GADGET) {
> -		/*
> -		 * if it is device mode:
> -		 * - Enable vbus detect
> -		 * - If it has already connected to host, notify udc
> -		 */
>  		ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
>  		ci_handle_vbus_change(ci);
> -	} else if (ci->is_otg && (ci->role == CI_ROLE_HOST)) {
> -		/* USB Device at the MicroB to A cable */
> -		otg_set_vbus(&ci->otg, true);
>  	}
>  }
>  
> @@ -603,6 +594,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>  
>  	ci->dev = dev;
>  	ci->platdata = dev->platform_data;
> +	ci->reg_vbus = ci->platdata->reg_vbus;

nak, teach ci_hdrc_probe() how to get its own regulator.

>  	if (ci->platdata->phy)
>  		ci->transceiver = ci->platdata->phy;

this should happen for PHYs as well btw.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130306/dc8a4ed3/attachment.sig>

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

[Alexander Shishkin]

On 6 March 2013 11:56, Peter Chen <peter.chen@freescale.com> wrote:
> 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.
>
> - vbus interrupt needs to be handled when gadget function is enabled
>
> - Create/destroy the gadget at udc's init and destory function
>
> - start/stop API are used at otg id switch and probe routine
>
> - Defer some gadget operations at ci's delayed work queue

When I asked you to reorder patches, I didn't mean squash them all
into one. Now you have a patch that does 5 different things and none
of them properly, because you still need to amend these changes later
in the patchset, like the patch 7, where you remove the delayed work,
which is added in this patch. At the same time, you have bits in this
patch that should be moved to other patches. See comments below.

>
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
>  drivers/usb/chipidea/bits.h        |   10 ++
>  drivers/usb/chipidea/ci.h          |    7 +-
>  drivers/usb/chipidea/ci13xxx_imx.c |    3 +-
>  drivers/usb/chipidea/core.c        |  190 ++++++++++++++++++++++++++++++++----
>  drivers/usb/chipidea/host.c        |    6 +
>  drivers/usb/chipidea/host.h        |    6 +-
>  drivers/usb/chipidea/otg.c         |   28 ++++--
>  drivers/usb/chipidea/otg.h         |    3 +
>  drivers/usb/chipidea/udc.c         |   39 ++++++--
>  drivers/usb/chipidea/udc.h         |    4 +
>  10 files changed, 254 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
> index d8ffc2f..58ef56c 100644
> --- a/drivers/usb/chipidea/bits.h
> +++ b/drivers/usb/chipidea/bits.h
> @@ -77,11 +77,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 697e369..a3777a1 100644
> --- a/drivers/usb/chipidea/ci.h
> +++ b/drivers/usb/chipidea/ci.h
> @@ -130,6 +130,9 @@ struct hw_bank {
>   * @transceiver: pointer to USB PHY, if any
>   * @hcd: pointer to usb_hcd for ehci host driver
>   * @otg: for otg support
> + * @id_event: indicates there is a id event, and handled at ci_otg_work
> + * @b_sess_valid_event: indicates there is a vbus event, and handled
> + * at ci_otg_work
>   */
>  struct ci13xxx {
>         struct device                   *dev;
> @@ -140,6 +143,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 +170,8 @@ struct ci13xxx {
>         struct usb_phy                  *transceiver;
>         struct usb_hcd                  *hcd;
>         struct usb_otg                  otg;
> +       bool                            id_event;
> +       bool                            b_sess_valid_event;
>  };
>
>  static inline struct ci_role_driver *ci_role(struct ci13xxx *ci)
> @@ -201,7 +207,6 @@ static inline void ci_role_stop(struct ci13xxx *ci)
>
>         ci->roles[role]->stop(ci);
>  }
> -
>  /******************************************************************************
>   * REGISTERS
>   *****************************************************************************/
> diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
> index 136869b..793fdba 100644
> --- a/drivers/usb/chipidea/ci13xxx_imx.c
> +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> @@ -110,7 +110,8 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
>         pdata->capoffset = DEF_CAPOFFSET;
>         pdata->flags = CI13XXX_REQUIRE_TRANSCEIVER |
>                        CI13XXX_PULLUP_ON_VBUS |
> -                      CI13XXX_DISABLE_STREAMING;
> +                      CI13XXX_DISABLE_STREAMING |
> +                      CI13XXX_REGS_SHARED;
>
>         pdata->phy_mode = of_usb_get_phy_mode(pdev->dev.of_node);
>         pdata->dr_mode = of_usb_get_dr_mode(pdev->dev.of_node);
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index c89f2aa..c563e17 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -75,6 +75,7 @@
>  #include "bits.h"
>  #include "host.h"
>  #include "debug.h"
> +#include "otg.h"
>
>  /* Controller register map */
>  static uintptr_t ci_regs_nolpm[] = {
> @@ -201,6 +202,14 @@ static int hw_device_init(struct ci13xxx *ci, void __iomem *base)
>         if (ci->hw_ep_max > ENDPT_MAX)
>                 return -ENODEV;
>
> +       /* Disable all interrupts bits */
> +       hw_write(ci, OP_USBINTR, 0xffffffff, 0);
> +       ci_disable_otg_interrupt(ci, OTGSC_INT_EN_BITS);
> +
> +       /* Clear all interrupts status bits*/
> +       hw_write(ci, OP_USBSTS, 0xffffffff, 0xffffffff);
> +       ci_clear_otg_interrupt(ci, OTGSC_INT_STATUS_BITS);
> +
>         dev_dbg(ci->dev, "ChipIdea HDRC found, lpm: %d; cap: %p op: %p\n",
>                 ci->hw_bank.lpm, ci->hw_bank.cap, ci->hw_bank.op);
>
> @@ -302,24 +311,131 @@ 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;
> +}
> +
> +/*
> + * Since there are some capacitances at vbus, the vbus may not
> + * change very quickly when we stop/start internal 5v.
> + * Below is a vbus stable timeout value, the routine will quit
> + * if the vbus gets the required value, we can think there are some
> + * problems for hardware if the vbus can't get the required value
> + * within 5 seconds.
> + */
> +#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);
>
> +               /* 1. Finish the current role */
>                 ci_role_stop(ci);
> +               hw_device_reset(ci, USBMODE_CM_IDLE);
> +
> +               /* 2. Turn on/off vbus according to coming role */
> +               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 (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,
> +                                       CI_VBUS_STABLE_TIMEOUT);
> +               }
> +
> +               /* 3. Begin the new role */
>                 ci_role_start(ci, role);
> -               enable_irq(ci->irq);
>         }
>  }
>
> +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 (ci->id_event) {
> +               ci->id_event = false;
> +               ci_handle_id_switch(ci);
> +       } else if (ci->b_sess_valid_event) {
> +               ci->b_sess_valid_event = false;
> +               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);
> +
> +       if (ci->role == CI_ROLE_GADGET) {
> +               /*
> +                * if it is device mode:
> +                * - Enable vbus detect
> +                * - If it has already connected to host, notify udc
> +                */
> +               ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
> +               ci_handle_vbus_change(ci);
> +       } else if (ci->is_otg && (ci->role == CI_ROLE_HOST)) {
> +               /* USB Device at the MicroB to A cable */
> +               otg_set_vbus(&ci->otg, true);
> +       }
> +}

You remove this function in patch 7. Why add it in the first place?

> +
> +static inline void ci_role_destroy(struct ci13xxx *ci)
> +{
> +       ci_hdrc_gadget_destroy(ci);
> +       ci_hdrc_host_destroy(ci);
> +}

You shouldn't use "inline" here. And the name of the function is also
ambiguous, since it destroys all roles. It would be better to just
call both _destroys(), like I suggested in the diff I gave you some
time ago.

> +
>  static ssize_t show_role(struct device *dev, struct device_attribute *attr,
>                          char *buf)
>  {
> @@ -352,25 +468,50 @@ static ssize_t store_role(struct device *dev, struct device_attribute *attr,
>
>  static DEVICE_ATTR(role, S_IRUSR | S_IWUSR, show_role, store_role);
>
> +static bool ci_is_otg_capable(struct ci13xxx *ci)
> +{
> +       return hw_read(ci, CAP_DCCPARAMS, DCCPARAMS_DC | DCCPARAMS_HC)
> +               == (DCCPARAMS_DC | DCCPARAMS_HC);
> +}

Can't we
a) remember this value so that we don't have to read the register at
every interrupt
b) also take into account the fact that DCCPARAMS doesn't read
correctly on all chipideas? (see dr_mode patches)

> +
>  static irqreturn_t ci_irq(int irq, void *data)
>  {
>         struct ci13xxx *ci = data;
>         irqreturn_t ret = IRQ_NONE;
>         u32 otgsc = 0;
>
> -       if (ci->is_otg)
> +       if (ci_is_otg_capable(ci))

ci->is_otg is actually the right thing to use, we should instead make
sure that it's set properly

>                 otgsc = hw_read(ci, OP_OTGSC, ~0);
>
> -       if (ci->role != CI_ROLE_END)
> -               ret = ci_role(ci)->irq(ci);
> +       /*
> +        * Handle id change interrupt, it indicates device/host function
> +        * switch.
> +        */
> +       if (ci->is_otg && (otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS)) {
> +               ci->id_event = true;
> +               ci_clear_otg_interrupt(ci, OTGSC_IDIS);
> +               disable_irq_nosync(ci->irq);
> +               queue_work(ci->wq, &ci->work);
> +               return IRQ_HANDLED;
> +       }
>
> -       if (ci->is_otg && (otgsc & OTGSC_IDIS)) {
> -               hw_write(ci, OP_OTGSC, OTGSC_IDIS, OTGSC_IDIS);
> +       /*
> +        * Handle vbus change interrupt, it indicates device connection
> +        * and disconnection events.
> +        */
> +       if (ci_is_otg_capable(ci) && (otgsc & OTGSC_BSVIE)
> +                       && (otgsc & OTGSC_BSVIS)) {
> +               ci->b_sess_valid_event = true;
> +               ci_clear_otg_interrupt(ci, OTGSC_BSVIS);
>                 disable_irq_nosync(ci->irq);
>                 queue_work(ci->wq, &ci->work);
> -               ret = IRQ_HANDLED;
> +               return IRQ_HANDLED;
>         }
>
> +       /* Handle device/host interrupt */
> +       if (ci->role != CI_ROLE_END)
> +               ret = ci_role(ci)->irq(ci);
> +
>         return ret;
>  }
>
> @@ -481,7 +622,8 @@ static int 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");
> @@ -512,7 +654,10 @@ static int 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;
> +               /* if otg is supported, create struct usb_otg */
> +               ci_hdrc_otg_init(ci);
>                 /* ID pin needs 1ms debouce time, we delay 2ms for safe */
>                 mdelay(2);
>                 ci->role = ci_otg_role(ci);

I'm thinking that this should be based on dr_mode patches and not the
other way around.

> @@ -528,7 +673,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>         if (ret) {
>                 dev_err(dev, "can't start %s role\n", ci_role(ci)->name);
>                 ret = -ENODEV;
> -               goto rm_wq;
> +               goto free_memory;
>         }
>
>         platform_set_drvdata(pdev, ci);
> @@ -539,17 +684,22 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>
>         ret = device_create_file(dev, &dev_attr_role);
>         if (ret)
> -               goto rm_attr;
> +               goto free_irq;
>
> -       if (ci->is_otg)
> -               hw_write(ci, OP_OTGSC, OTGSC_IDIE, OTGSC_IDIE);
> +       /* Defer some operations */
> +       queue_delayed_work(ci->wq, &ci->dwork, msecs_to_jiffies(200));
>
>         return ret;
>
> -rm_attr:
> -       device_remove_file(dev, &dev_attr_role);
> +free_irq:
> +       free_irq(ci->irq, ci);
>  stop:
> -       ci_role_stop(ci);
> +       ci_role_destroy(ci);
> +free_memory:
> +       if (ci->roles[CI_ROLE_HOST])
> +               devm_kfree(dev, ci->roles[CI_ROLE_HOST]);
> +       if (ci->roles[CI_ROLE_GADGET])
> +               devm_kfree(dev, ci->roles[CI_ROLE_GADGET]);
>  rm_wq:
>         flush_workqueue(ci->wq);
>         destroy_workqueue(ci->wq);
> @@ -565,7 +715,7 @@ static int 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 8e9d312..ead3158 100644
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -105,3 +105,9 @@ int ci_hdrc_host_init(struct ci13xxx *ci)
>
>         return 0;
>  }
> +
> +void ci_hdrc_host_destroy(struct ci13xxx *ci)
> +{
> +       if (ci->role == CI_ROLE_HOST)
> +               host_stop(ci);
> +}
> diff --git a/drivers/usb/chipidea/host.h b/drivers/usb/chipidea/host.h
> index 761fb1f..19cc72d 100644
> --- a/drivers/usb/chipidea/host.h
> +++ b/drivers/usb/chipidea/host.h
> @@ -4,13 +4,17 @@
>  #ifdef CONFIG_USB_CHIPIDEA_HOST
>
>  int ci_hdrc_host_init(struct ci13xxx *ci);
> -
> +void ci_hdrc_host_destroy(struct ci13xxx *ci);
>  #else
>
>  static inline int ci_hdrc_host_init(struct ci13xxx *ci)
>  {
>         return -ENXIO;
>  }
> +static inline void ci_hdrc_host_destroy(struct ci13xxx *ci)
> +{
> +
> +}
>
>  #endif
>
> diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
> index 7dea3b3..2986d91 100644
> --- a/drivers/usb/chipidea/otg.c
> +++ b/drivers/usb/chipidea/otg.c
> @@ -10,21 +10,28 @@
>   * 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/gadget.h>
>  #include <linux/usb/chipidea.h>
>
>  #include "ci.h"
> -#include "udc.h"
>  #include "bits.h"
> -#include "host.h"
> -#include "debug.h"
> +
> +void ci_clear_otg_interrupt(struct ci13xxx *ci, u32 bits)
> +{
> +       /* Only clear request bits */
> +       hw_write(ci, OP_OTGSC, OTGSC_INT_STATUS_BITS, bits);
> +}
> +
> +void ci_enable_otg_interrupt(struct ci13xxx *ci, u32 bits)
> +{
> +       hw_write(ci, OP_OTGSC, bits, bits);
> +}
> +
> +void ci_disable_otg_interrupt(struct ci13xxx *ci, u32 bits)
> +{
> +       hw_write(ci, OP_OTGSC, bits, 0);
> +}
>
>  static int ci_otg_set_peripheral(struct usb_otg *otg,
>                 struct usb_gadget *periph)
> @@ -55,6 +62,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;
> +       ci_enable_otg_interrupt(ci, OTGSC_IDIE);

There should be a counter-action to ci_hdrc_otg_init().
Btw, why can't clear_otg_interrupt() be called from here as well
instead of hw_device_init()? This function should only be called for
otg capable devices, so it should be safe. And while at it, I think
this whole otg.c/otg.h part of this patch should be moved to patch 2.

>
>         return 0;
>  }
> diff --git a/drivers/usb/chipidea/otg.h b/drivers/usb/chipidea/otg.h
> index b4c6b3e..fa30428 100644
> --- a/drivers/usb/chipidea/otg.h
> +++ b/drivers/usb/chipidea/otg.h
> @@ -2,5 +2,8 @@
>  #define __DRIVERS_USB_CHIPIDEA_OTG_H
>
>  int ci_hdrc_otg_init(struct ci13xxx *ci);
> +void ci_clear_otg_interrupt(struct ci13xxx *ci, u32 bits);
> +void ci_enable_otg_interrupt(struct ci13xxx *ci, u32 bits);
> +void ci_disable_otg_interrupt(struct ci13xxx *ci, u32 bits);
>
>  #endif /* __DRIVERS_USB_CHIPIDEA_OTG_H */
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index d214448..e82dae4 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -31,6 +31,7 @@
>
>  #include "ci.h"
>  #include "udc.h"
> +#include "otg.h"
>  #include "bits.h"
>  #include "debug.h"
>
> @@ -1371,6 +1372,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 +1380,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");
>                 }
>         }
>
> @@ -1741,7 +1744,12 @@ static int udc_start(struct ci13xxx *ci)
>         if (!IS_ERR_OR_NULL(ci->transceiver)) {
>                 retval = otg_set_peripheral(ci->transceiver->otg,
>                                                 &ci->gadget);
> -               if (retval)
> +               /*
> +                * If we implement all USB functions using chipidea drivers,
> +                * it doesn't need to call above API, meanwhile, if we only
> +                * use gadget function, calling above API is useless.
> +                */
> +               if (retval && retval != -ENOTSUPP)
>                         goto remove_dbg;
>         }
>
> @@ -1778,12 +1786,27 @@ free_qh_pool:
>         return retval;
>  }
>
> +static int udc_id_switch_for_device(struct ci13xxx *ci)

I would use _to_ instead of _for_, for clarity.

> +{
> +       ci_clear_otg_interrupt(ci, OTGSC_BSVIS);
> +       ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
> +
> +       return 0;
> +}
> +
> +static void udc_id_switch_for_host(struct ci13xxx *ci)

Same here.

> +{
> +       /* host doesn't care B_SESSION_VALID event */
> +       ci_clear_otg_interrupt(ci, OTGSC_BSVIS);
> +       ci_disable_otg_interrupt(ci, OTGSC_BSVIE);
> +}
> +
>  /**
> - * udc_remove: parent remove must call this to remove UDC
> + * ci_hdrc_gadget_destroy: parent remove must call this to remove UDC
>   *
>   * No interrupts active, the IRQ has been released
>   */
> -static void udc_stop(struct ci13xxx *ci)
> +void ci_hdrc_gadget_destroy(struct ci13xxx *ci)
>  {
>         if (ci == NULL)
>                 return;
> @@ -1802,15 +1825,13 @@ static void udc_stop(struct ci13xxx *ci)
>         }
>         dbg_remove_files(&ci->gadget.dev);
>         device_unregister(&ci->gadget.dev);
> -       /* my kobject is dynamic, I swear! */
> -       memset(&ci->gadget, 0, sizeof(ci->gadget));
>  }
>
>  /**
>   * ci_hdrc_gadget_init - initialize device related bits
>   * ci: the controller
>   *
> - * This function enables the gadget role, if the device is "device capable".
> + * This function initializes gadget, if the device is "device capable".
>   */
>  int ci_hdrc_gadget_init(struct ci13xxx *ci)
>  {
> @@ -1823,11 +1844,11 @@ int ci_hdrc_gadget_init(struct ci13xxx *ci)
>         if (!rdrv)
>                 return -ENOMEM;
>
> -       rdrv->start     = udc_start;
> -       rdrv->stop      = udc_stop;
> +       rdrv->start     = udc_id_switch_for_device;
> +       rdrv->stop      = udc_id_switch_for_host;
>         rdrv->irq       = udc_irq;
>         rdrv->name      = "gadget";
>         ci->roles[CI_ROLE_GADGET] = rdrv;
>
> -       return 0;
> +       return udc_start(ci);
>  }
> diff --git a/drivers/usb/chipidea/udc.h b/drivers/usb/chipidea/udc.h
> index 4ff2384..6553e4d 100644
> --- a/drivers/usb/chipidea/udc.h
> +++ b/drivers/usb/chipidea/udc.h
> @@ -80,6 +80,7 @@ struct ci13xxx_req {
>  #ifdef CONFIG_USB_CHIPIDEA_UDC
>
>  int ci_hdrc_gadget_init(struct ci13xxx *ci);
> +void ci_hdrc_gadget_destroy(struct ci13xxx *ci);
>
>  #else
>
> @@ -87,7 +88,10 @@ static inline int ci_hdrc_gadget_init(struct ci13xxx *ci)
>  {
>         return -ENXIO;
>  }
> +static inline void ci_hdrc_gadget_destroy(struct ci13xxx *ci)
> +{
>
> +}

Newlines are wrong here as well as in the udc.h bit. Which is weird,
because they were ok in the original patch I sent you
http://www.mail-archive.com/linux-usb at vger.kernel.org/msg13668.html

>  #endif
>
>  #endif /* __DRIVERS_USB_CHIPIDEA_UDC_H */
> --
> 1.7.0.4
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v11 9/9] usb: chipidea: udc: fix the oops when plugs in usb cable after rmmod gadget

[Russell King - ARM Linux]

On Wed, Mar 06, 2013 at 05:56:40PM +0800, Peter Chen wrote:
> root at freescale ~$ ci_hdrc ci_hdrc.0: Connected to host
> Unable to handle kernel paging request at virtual address 7f01171c
> pgd = 80004000
> [7f01171c] *pgd=4fa1e811, *pte=00000000, *ppte=00000000
> Internal error: Oops: 7 [#1] SMP ARM
> Modules linked in: f_acm libcomposite u_serial [last unloaded: g_serial]
> CPU: 0    Not tainted  (3.8.0-rc5+ #221)
> 	PC is at _gadget_stop_activity+0xbc/0x128
> 	LR is at ep_fifo_flush+0x68/0xa0


Why much of the oops dump indented?

[PATCH v11 8/9] usb: chipidea: imx: add getting vbus regulator code

[Peter Chen]

On Wed, Mar 06, 2013 at 12:11:46PM +0200, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Mar 06, 2013 at 05:56:39PM +0800, Peter Chen wrote:
> > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > ---
> >  drivers/usb/chipidea/ci13xxx_imx.c |   28 +++++-----------------------
> >  1 files changed, 5 insertions(+), 23 deletions(-)
> > 
> > diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
> > index 3ed119e..b2d8350 100644
> > --- a/drivers/usb/chipidea/ci13xxx_imx.c
> > +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> > @@ -33,7 +33,6 @@ struct ci13xxx_imx_data {
> >  	struct usb_phy *phy;
> >  	struct platform_device *ci_pdev;
> >  	struct clk *clk;
> > -	struct regulator *reg_vbus;
> >  };
> >  
> >  static const struct usbmisc_ops *usbmisc_ops;
> > @@ -163,20 +162,9 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
> >  		data->phy = phy;
> >  	}
> >  
> > -	/* 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 err_clk;
> > -		}
> > -		data->reg_vbus = reg_vbus;
> > -	} else {
> > -		reg_vbus = NULL;
> > -	}
> > +	if (!IS_ERR(reg_vbus))
> > +		pdata->reg_vbus = reg_vbus;
> 
> what you're doing here is wrong. chipidea core should be the one calling
> devm_regulator_get().

chipidea core has not dts entry, is created by platform layer.
Regulator info is passed from dts.

> 
> -- 
> balbi



-- 

Best Regards,
Peter Chen

[PATCH v11 4/9] usb: chipidea: udc: add pullup/pulldown dp at hw_device_state

[Peter Chen]

On Wed, Mar 06, 2013 at 01:26:33PM +0200, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Mar 06, 2013 at 05:56:35PM +0800, Peter Chen wrote:
> > - 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 e82dae4..70f9f2d 100644
> > --- a/drivers/usb/chipidea/udc.c
> > +++ b/drivers/usb/chipidea/udc.c
> > @@ -91,8 +91,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);
> 
> this patch doesn't make sense to me. What will happen is that you will
> be enabling pullups when vbus_session() gets called and this might not
> be what gadget driver wants.
> 
> You don't want to fiddle with that yourself since I'm changing the
> framework so that gadget driver will always request pullups to be
> enabled.

Hi Felipe,

Do you think pullup dp without vbus is a valid operation?
Current udc core code makes that possible.
But I think current your udc core code (add pullup after loading gadget)
make benefit for udc driver who has not vbus operation.

For chipidea driver:

- If vbus is there before load gadget module, the pullup dp is done by udc
core code.
- If vbus is not there before load gadget module, the pullup will be done
when the vbus is there.

> 
> Alex, I don't think this patch is correct
> 
> -- 
> balbi



-- 

Best Regards,
Peter Chen

[PATCH v11 6/9] usb: chipidea: add vbus regulator control

[Peter Chen]

On Wed, Mar 06, 2013 at 01:29:16PM +0200, Felipe Balbi wrote:
> Hi,
> 
> On Wed, Mar 06, 2013 at 05:56:37PM +0800, Peter Chen wrote:
> > For boards which have board level vbus control (eg, gpio), we
> > need to operation vbus according to below rules:
> > - For host, the vbus should always be on.
> > - For otg, the vbus needs to be turned on/off when usb role switches.
> > 
> > We put vbus operation to host as host is the only vbus user,
> > When we are at host mode, the vbus is on, when we are not at
> > host mode, vbus should be off.
> > 
> > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > ---
> >  
> > @@ -603,6 +594,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> >  
> >  	ci->dev = dev;
> >  	ci->platdata = dev->platform_data;
> > +	ci->reg_vbus = ci->platdata->reg_vbus;
> 
> nak, teach ci_hdrc_probe() how to get its own regulator.
> 
> >  	if (ci->platdata->phy)
> >  		ci->transceiver = ci->platdata->phy;
> 
> this should happen for PHYs as well btw.

Like I said at previous email, core code has NO DTS entry.

> 
> -- 
> balbi



-- 

Best Regards,
Peter Chen

[PATCH v11 9/9] usb: chipidea: udc: fix the oops when plugs in usb cable after rmmod gadget

[Peter Chen]

On Wed, Mar 06, 2013 at 06:46:21PM +0000, Russell King - ARM Linux wrote:
> On Wed, Mar 06, 2013 at 05:56:40PM +0800, Peter Chen wrote:
> > root at freescale ~$ ci_hdrc ci_hdrc.0: Connected to host
> > Unable to handle kernel paging request at virtual address 7f01171c
> > pgd = 80004000
> > [7f01171c] *pgd=4fa1e811, *pte=00000000, *ppte=00000000
> > Internal error: Oops: 7 [#1] SMP ARM
> > Modules linked in: f_acm libcomposite u_serial [last unloaded: g_serial]
> > CPU: 0    Not tainted  (3.8.0-rc5+ #221)
> > 	PC is at _gadget_stop_activity+0xbc/0x128
> > 	LR is at ep_fifo_flush+0x68/0xa0
> 
> 
> Why much of the oops dump indented?
> 

Maybe it occurs when I paste it, I will change it.
Thanks.

-- 

Best Regards,
Peter Chen

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

[Peter Chen]

On Wed, Mar 06, 2013 at 07:09:34PM +0200, Alexander Shishkin wrote:
> > - start/stop API are used at otg id switch and probe routine
> >
> > - Defer some gadget operations at ci's delayed work queue
> 
> When I asked you to reorder patches, I didn't mean squash them all
> into one. Now you have a patch that does 5 different things and none
> of them properly, because you still need to amend these changes later
> in the patchset, like the patch 7, where you remove the delayed work,
> which is added in this patch. At the same time, you have bits in this
> patch that should be moved to other patches. See comments below.
> 

I am sorry to let you review difficult, as I changed/added patch according
to function change, but not updated previous patches.

I will re-organize all patches according to functionalities.
Besides, can you help review all patches during next time, in that case,
I can know which patch is ok.

> 
> You remove this function in patch 7. Why add it in the first place?
> 

As it is needed at patch 7 to let the function work

> > +
> > +static inline void ci_role_destroy(struct ci13xxx *ci)
> > +{
> > +       ci_hdrc_gadget_destroy(ci);
> > +       ci_hdrc_host_destroy(ci);
> > +}
> 
> You shouldn't use "inline" here. And the name of the function is also
> ambiguous, since it destroys all roles. It would be better to just
> call both _destroys(), like I suggested in the diff I gave you some
> time ago.

Will change.

> 
> > +
> >  static ssize_t show_role(struct device *dev, struct device_attribute *attr,
> >                          char *buf)
> >  {
> > @@ -352,25 +468,50 @@ static ssize_t store_role(struct device *dev, struct device_attribute *attr,
> >
> >  static DEVICE_ATTR(role, S_IRUSR | S_IWUSR, show_role, store_role);
> >
> > +static bool ci_is_otg_capable(struct ci13xxx *ci)
> > +{
> > +       return hw_read(ci, CAP_DCCPARAMS, DCCPARAMS_DC | DCCPARAMS_HC)
> > +               == (DCCPARAMS_DC | DCCPARAMS_HC);
> > +}
> 
> Can't we
> a) remember this value so that we don't have to read the register at
> every interrupt

Yes.

> b) also take into account the fact that DCCPARAMS doesn't read
> correctly on all chipideas? (see dr_mode patches)
> 

We have discussed it, but you have not given me exactly answer
"Which situation we can read OTGSC"?

1. We Can't read OTGSC if DCCPARAMS shows it is only device-capable
2. DCCPARAMS doesn't correctly for some chips
3. If we depend on dr_mode, OTGSC should be read when dr_mode = "peripheral",
in that case, the condition is ci->roles[CI_ROLE_GADGET].

If the dr_mode is not set by DT, the 1 and 3 is conflict.


> >
> > -       if (ci->is_otg)
> > +       if (ci_is_otg_capable(ci))
> 
> ci->is_otg is actually the right thing to use, we should instead make
> sure that it's set properly
> 

Yes, what is ci->is_otg stands for in your opinion?
In my patchset, it means it supportes partial OTG (id switch) function.

> >         if (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]) {
> > +               dev_dbg(dev, "support otg\n");
> >                 ci->is_otg = true;
> > +               /* if otg is supported, create struct usb_otg */
> > +               ci_hdrc_otg_init(ci);
> >                 /* ID pin needs 1ms debouce time, we delay 2ms for safe */
> >                 mdelay(2);
> >                 ci->role = ci_otg_role(ci);
> 
> I'm thinking that this should be based on dr_mode patches and not the
> other way around.
> 

No, ci->role means which role is currently.
And it is otg mode already as 
(ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]).

> >
> >  static int ci_otg_set_peripheral(struct usb_otg *otg,
> >                 struct usb_gadget *periph)
> > @@ -55,6 +62,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;
> > +       ci_enable_otg_interrupt(ci, OTGSC_IDIE);
> 
> There should be a counter-action to ci_hdrc_otg_init().
> Btw, why can't clear_otg_interrupt() be called from here as well
> instead of hw_device_init()? 

What the criterion for reading OTGSC?
What means otg capable and how do we know it is otg capable?

> This function should only be called for
> otg capable devices, so it should be safe. And while at it, I think
> this whole otg.c/otg.h part of this patch should be moved to patch 2.
> 

OK.

> >
> > +static int udc_id_switch_for_device(struct ci13xxx *ci)
> 
> I would use _to_ instead of _for_, for clarity.
> 
> > +{
> > +       ci_clear_otg_interrupt(ci, OTGSC_BSVIS);
> > +       ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
> > +
> > +       return 0;
> > +}
> > +
> > +static void udc_id_switch_for_host(struct ci13xxx *ci)
> 
> Same here.
> 

Will change

> 
> Newlines are wrong here as well as in the udc.h bit. Which is weird,
> because they were ok in the original patch I sent you
> http://www.mail-archive.com/linux-usb at vger.kernel.org/msg13668.html
> 

Will change

-- 

Best Regards,
Peter Chen

[PATCH v11 4/9] usb: chipidea: udc: add pullup/pulldown dp at hw_device_state

[Felipe Balbi]

Hi,

On Thu, Mar 07, 2013 at 10:36:13AM +0800, Peter Chen wrote:
> > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> > > index e82dae4..70f9f2d 100644
> > > --- a/drivers/usb/chipidea/udc.c
> > > +++ b/drivers/usb/chipidea/udc.c
> > > @@ -91,8 +91,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);
> > 
> > this patch doesn't make sense to me. What will happen is that you will
> > be enabling pullups when vbus_session() gets called and this might not
> > be what gadget driver wants.
> > 
> > You don't want to fiddle with that yourself since I'm changing the
> > framework so that gadget driver will always request pullups to be
> > enabled.
> 
> Hi Felipe,
> 
> Do you think pullup dp without vbus is a valid operation?

why not ? What I want to connect pullups first and only then issue SRP ?

> Current udc core code makes that possible.

so ?

> But I think current your udc core code (add pullup after loading
> gadget) make benefit for udc driver who has not vbus operation.
> 
> For chipidea driver:
> 
> - If vbus is there before load gadget module, the pullup dp is done by
> udc core code.
> - If vbus is not there before load gadget module, the pullup will be
> done when the vbus is there.

This isn't legal. If you want to make sure vbus is alive before
connecting pullups, then do it generically. Modify udc-core.c to make
those checks for you. Bypassing the framework is dangerous because
whenever I wanna change something, I might miss your private details and
end up causing regressions.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130307/89379368/attachment.sig>

[PATCH v11 6/9] usb: chipidea: add vbus regulator control

[Felipe Balbi]

On Thu, Mar 07, 2013 at 10:41:03AM +0800, Peter Chen wrote:
> On Wed, Mar 06, 2013 at 01:29:16PM +0200, Felipe Balbi wrote:
> > Hi,
> > 
> > On Wed, Mar 06, 2013 at 05:56:37PM +0800, Peter Chen wrote:
> > > For boards which have board level vbus control (eg, gpio), we
> > > need to operation vbus according to below rules:
> > > - For host, the vbus should always be on.
> > > - For otg, the vbus needs to be turned on/off when usb role switches.
> > > 
> > > We put vbus operation to host as host is the only vbus user,
> > > When we are at host mode, the vbus is on, when we are not at
> > > host mode, vbus should be off.
> > > 
> > > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > > ---
> > >  
> > > @@ -603,6 +594,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> > >  
> > >  	ci->dev = dev;
> > >  	ci->platdata = dev->platform_data;
> > > +	ci->reg_vbus = ci->platdata->reg_vbus;
> > 
> > nak, teach ci_hdrc_probe() how to get its own regulator.
> > 
> > >  	if (ci->platdata->phy)
> > >  		ci->transceiver = ci->platdata->phy;
> > 
> > this should happen for PHYs as well btw.
> 
> Like I said at previous email, core code has NO DTS entry.

that's your problem :-)

You can just as well create both nodes in DTS file:

ci13xx_fsl at foo {
	compatible = "fsl,chipidea";
	reg = <foo size>;
	blablabla
	ranges;

	chipidea_core at bar {
		compatible = "chipidea";
		reg <bar size>;
		blablabla
	};
};

then assign the regulator to chipidea itself, not to the glue.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130307/e43cfb62/attachment.sig>

[PATCH v11 4/9] usb: chipidea: udc: add pullup/pulldown dp at hw_device_state

[Peter Chen]

On Thu, Mar 07, 2013 at 11:50:54AM +0200, Felipe Balbi wrote:
> Hi,
> 
> On Thu, Mar 07, 2013 at 10:36:13AM +0800, Peter Chen wrote:
> > > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> > > > index e82dae4..70f9f2d 100644
> > > > --- a/drivers/usb/chipidea/udc.c
> > > > +++ b/drivers/usb/chipidea/udc.c
> > > > @@ -91,8 +91,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);
> > > 
> > > this patch doesn't make sense to me. What will happen is that you will
> > > be enabling pullups when vbus_session() gets called and this might not
> > > be what gadget driver wants.
> > > 
> > > You don't want to fiddle with that yourself since I'm changing the
> > > framework so that gadget driver will always request pullups to be
> > > enabled.
> > 
> > Hi Felipe,
> > 
> > Do you think pullup dp without vbus is a valid operation?
> 
> why not ? What I want to connect pullups first and only then issue SRP ?

I am not familiar with OTG, but it only stands for special case, right?

> 
> > Current udc core code makes that possible.
> 
> so ?

Without vbus, but pullup dp, it will cause more power.

> 
> > But I think current your udc core code (add pullup after loading
> > gadget) make benefit for udc driver who has not vbus operation.
> > 
> > For chipidea driver:
> > 
> > - If vbus is there before load gadget module, the pullup dp is done by
> > udc core code.
> > - If vbus is not there before load gadget module, the pullup will be
> > done when the vbus is there.
> 
> This isn't legal. If you want to make sure vbus is alive before
> connecting pullups, then do it generically. Modify udc-core.c to make
> those checks for you. Bypassing the framework is dangerous because
> whenever I wanna change something, I might miss your private details and
> end up causing regressions.

Let thing be generic is a good idea. Then, is it ok I post a patch let
udc manage pullup by itself (through a flag) just you did for uvc?
UDC core doesn't know VBUS, so the pullup can't be managed by udc core
totally.

Besides, I looked four udc drivers (fsl_udc_core.c, at91_udc.c, mv_udc_core.c
and bcm63xx_udc.c), the first three manage pullup by itself, ony
bcm doesn't control it by itself.

> -- 
> balbi



-- 

Best Regards,
Peter Chen

[PATCH v11 6/9] usb: chipidea: add vbus regulator control

[Peter Chen]

On Thu, Mar 07, 2013 at 11:52:48AM +0200, Felipe Balbi wrote:
> On Thu, Mar 07, 2013 at 10:41:03AM +0800, Peter Chen wrote:
> > On Wed, Mar 06, 2013 at 01:29:16PM +0200, Felipe Balbi wrote:
> > > Hi,
> > > 
> > > On Wed, Mar 06, 2013 at 05:56:37PM +0800, Peter Chen wrote:
> > > > For boards which have board level vbus control (eg, gpio), we
> > > > need to operation vbus according to below rules:
> > > > - For host, the vbus should always be on.
> > > > - For otg, the vbus needs to be turned on/off when usb role switches.
> > > > 
> > > > We put vbus operation to host as host is the only vbus user,
> > > > When we are at host mode, the vbus is on, when we are not at
> > > > host mode, vbus should be off.
> > > > 
> > > > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > > > ---
> > > >  
> > > > @@ -603,6 +594,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> > > >  
> > > >  	ci->dev = dev;
> > > >  	ci->platdata = dev->platform_data;
> > > > +	ci->reg_vbus = ci->platdata->reg_vbus;
> > > 
> > > nak, teach ci_hdrc_probe() how to get its own regulator.
> > > 
> > > >  	if (ci->platdata->phy)
> > > >  		ci->transceiver = ci->platdata->phy;
> > > 
> > > this should happen for PHYs as well btw.
> > 
> > Like I said at previous email, core code has NO DTS entry.
> 
> that's your problem :-)
> 
> You can just as well create both nodes in DTS file:
> 
> ci13xx_fsl at foo {
> 	compatible = "fsl,chipidea";
> 	reg = <foo size>;
> 	blablabla
> 	ranges;
> 
> 	chipidea_core at bar {
> 		compatible = "chipidea";
> 		reg <bar size>;
> 		blablabla
> 	};
> };
> 
> then assign the regulator to chipidea itself, not to the glue.

No, that will make things more complicated at current chipidea driver.

Differ with dwc3, chipidea created its platform data at glue layer,
and chipidea core owing this data when platform device is created.

If we also want get things from DT node, we need to get node from
glue layer as this node is NOT belonged to chipidea core's pdev.
(We can't get it through compatible string as compatible string
 is the same from every chipidea device)

Convert platform data to DT for chipidea core is a big job,
it is out of scope of this patch. If Alex also thinks we
are deserved to do it, I can do it later.

> 
> -- 
> balbi



-- 

Best Regards,
Peter Chen

[PATCH v11 4/9] usb: chipidea: udc: add pullup/pulldown dp at hw_device_state

[Felipe Balbi]

Hi,

On Fri, Mar 08, 2013 at 09:28:34AM +0800, Peter Chen wrote:
> > > > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> > > > > index e82dae4..70f9f2d 100644
> > > > > --- a/drivers/usb/chipidea/udc.c
> > > > > +++ b/drivers/usb/chipidea/udc.c
> > > > > @@ -91,8 +91,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);
> > > > 
> > > > this patch doesn't make sense to me. What will happen is that you will
> > > > be enabling pullups when vbus_session() gets called and this might not
> > > > be what gadget driver wants.
> > > > 
> > > > You don't want to fiddle with that yourself since I'm changing the
> > > > framework so that gadget driver will always request pullups to be
> > > > enabled.
> > > 
> > > Hi Felipe,
> > > 
> > > Do you think pullup dp without vbus is a valid operation?
> > 
> > why not ? What I want to connect pullups first and only then issue SRP ?
> 
> I am not familiar with OTG, but it only stands for special case, right?

SRP is not OTG-specific. Any host is allowed to support it and any
device is allowed to initiate it.

> > > Current udc core code makes that possible.
> > 
> > so ?
> 
> Without vbus, but pullup dp, it will cause more power.

a fair concern.

> > > But I think current your udc core code (add pullup after loading
> > > gadget) make benefit for udc driver who has not vbus operation.
> > > 
> > > For chipidea driver:
> > > 
> > > - If vbus is there before load gadget module, the pullup dp is done by
> > > udc core code.
> > > - If vbus is not there before load gadget module, the pullup will be
> > > done when the vbus is there.
> > 
> > This isn't legal. If you want to make sure vbus is alive before
> > connecting pullups, then do it generically. Modify udc-core.c to make
> > those checks for you. Bypassing the framework is dangerous because
> > whenever I wanna change something, I might miss your private details and
> > end up causing regressions.
> 
> Let thing be generic is a good idea. Then, is it ok I post a patch let
> udc manage pullup by itself (through a flag) just you did for uvc?
> UDC core doesn't know VBUS, so the pullup can't be managed by udc core
> totally.

then teach UDC core about VBUS. Send the patches and we will figure out
if they can be applied or not.

> Besides, I looked four udc drivers (fsl_udc_core.c, at91_udc.c, mv_udc_core.c
> and bcm63xx_udc.c), the first three manage pullup by itself, ony
> bcm doesn't control it by itself.

those are all bugs that need to be sorted out. I don't have that HW so I
can't test any of my changes.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130308/0336a16e/attachment.sig>

[PATCH v11 6/9] usb: chipidea: add vbus regulator control

[Felipe Balbi]

Hi,

On Fri, Mar 08, 2013 at 02:27:58PM +0800, Peter Chen wrote:
> > > > > For boards which have board level vbus control (eg, gpio), we
> > > > > need to operation vbus according to below rules:
> > > > > - For host, the vbus should always be on.
> > > > > - For otg, the vbus needs to be turned on/off when usb role switches.
> > > > > 
> > > > > We put vbus operation to host as host is the only vbus user,
> > > > > When we are at host mode, the vbus is on, when we are not at
> > > > > host mode, vbus should be off.
> > > > > 
> > > > > Signed-off-by: Peter Chen <peter.chen@freescale.com>
> > > > > ---
> > > > >  
> > > > > @@ -603,6 +594,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> > > > >  
> > > > >  	ci->dev = dev;
> > > > >  	ci->platdata = dev->platform_data;
> > > > > +	ci->reg_vbus = ci->platdata->reg_vbus;
> > > > 
> > > > nak, teach ci_hdrc_probe() how to get its own regulator.
> > > > 
> > > > >  	if (ci->platdata->phy)
> > > > >  		ci->transceiver = ci->platdata->phy;
> > > > 
> > > > this should happen for PHYs as well btw.
> > > 
> > > Like I said at previous email, core code has NO DTS entry.
> > 
> > that's your problem :-)
> > 
> > You can just as well create both nodes in DTS file:
> > 
> > ci13xx_fsl at foo {
> > 	compatible = "fsl,chipidea";
> > 	reg = <foo size>;
> > 	blablabla
> > 	ranges;
> > 
> > 	chipidea_core at bar {
> > 		compatible = "chipidea";
> > 		reg <bar size>;
> > 		blablabla
> > 	};
> > };
> > 
> > then assign the regulator to chipidea itself, not to the glue.
> 
> No, that will make things more complicated at current chipidea driver.

so what ? We're not here to choose the easy approach, we're here to
choose the 'right' approach. If that means rewritting a bunch of
chipidea core code, so be it.

> Differ with dwc3, chipidea created its platform data at glue layer,
> and chipidea core owing this data when platform device is created.

yeah yeah, but that's just because chipidea core doesn't understand DT.

It should take you no more than a day to teach it about DT, though. Not
sure what you're complaining about.

> If we also want get things from DT node, we need to get node from
> glue layer as this node is NOT belonged to chipidea core's pdev.

which DT node are you talking about ? you want chipidea core to have
access to glue's DT node ? Are you insane ?

Currently your regulator only belongs to glue because chipidea core
doesn't know about DT, if you teach it about DT, then you move the
regulator to chipidea core and it will not need access to glue's DT
node.

> (We can't get it through compatible string as compatible string
>  is the same from every chipidea device)

you're not making sense.

If you wanna add DT support for chipidea core, you need to come up with
a compatible that makes sense. It won't be fsl,* because chipidea core
doesn't belong to fsl.

> Convert platform data to DT for chipidea core is a big job,

no it's not.

> it is out of scope of this patch. If Alex also thinks we

yeah, but this patch makes no sense. The right thing (TM) to do is, as I
have said multiple times before, to teach chipidea core about DT. Please
stop trying to find excuses for not doing the work you need to do; You
would've already done it if you had spent so much of everybody's time
trying to find excuses why not to do it and why Alex should accept your
broken patches.

Have you already forgotten how many issues were found in each and every
version of your patchset ? There's a reason why we're revieweing v11,
right ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130308/c4c78e1f/attachment.sig>

[PATCH v11 4/9] usb: chipidea: udc: add pullup/pulldown dp at hw_device_state

[Peter Chen]

On Fri, Mar 08, 2013 at 09:18:32AM +0200, Felipe Balbi wrote:
> > > 
> > > This isn't legal. If you want to make sure vbus is alive before
> > > connecting pullups, then do it generically. Modify udc-core.c to make
> > > those checks for you. Bypassing the framework is dangerous because
> > > whenever I wanna change something, I might miss your private details and
> > > end up causing regressions.
> > 
> > Let thing be generic is a good idea. Then, is it ok I post a patch let
> > udc manage pullup by itself (through a flag) just you did for uvc?
> > UDC core doesn't know VBUS, so the pullup can't be managed by udc core
> > totally.
> 
> then teach UDC core about VBUS. Send the patches and we will figure out
> if they can be applied or not.

I will do it

> 
> -- 
> balbi



-- 

Best Regards,
Peter Chen

[PATCH v11 6/9] usb: chipidea: add vbus regulator control

[Peter Chen]

On Fri, Mar 08, 2013 at 09:26:24AM +0200, Felipe Balbi wrote:
> 
> > Differ with dwc3, chipidea created its platform data at glue layer,
> > and chipidea core owing this data when platform device is created.
> 
> yeah yeah, but that's just because chipidea core doesn't understand DT.
> 
> It should take you no more than a day to teach it about DT, though. Not
> sure what you're complaining about.

I have not complained about it, it is out of scope of this patchset.
This patchset is just adds ID/vbus support for chipidea.
For other enhancements, it needs another patchset. 

> 
> > If we also want get things from DT node, we need to get node from
> > glue layer as this node is NOT belonged to chipidea core's pdev.
> 
> which DT node are you talking about ? you want chipidea core to have
> access to glue's DT node ? Are you insane ?

Please read chipidea code, the chipidea core (pdev & platform data)
is not created by DT, it is created by glue layer.
If we added dts entry like you showed me, we can't get node by
pdev->dev.of_node.

> 
> If you wanna add DT support for chipidea core, you need to come up with
> a compatible that makes sense. It won't be fsl,* because chipidea core
> doesn't belong to fsl.

I know it. 

> 
> > Convert platform data to DT for chipidea core is a big job,
> 
> no it's not.

I need to change all msm and pci board related files.

> 
> yeah, but this patch makes no sense. The right thing (TM) to do is, as I
> have said multiple times before, to teach chipidea core about DT. Please
> stop trying to find excuses for not doing the work you need to do; You
> would've already done it if you had spent so much of everybody's time
> trying to find excuses why not to do it and why Alex should accept your
> broken patches.

First let chipidea core know about DT is out of this patchset.

Second, why you think it is a broken patch, it just follows current
framework. Besides, if you have concerns about DT stuff for chipidea
core, why not mention it when sasche added dr_mode and phy_mode at that time?

> 
> Have you already forgotten how many issues were found in each and every
> version of your patchset ? There's a reason why we're revieweing v11,
> right ?

I just want to limit this patchset to id/vbus enable feature, 
I admit I am less upstream experience, but if new requirements
new features and new bugs always coming, things can't be ended up
easily.


-- 

Best Regards,
Peter Chen

[PATCH v11 6/9] usb: chipidea: add vbus regulator control

[Felipe Balbi]

Hi,

On Fri, Mar 08, 2013 at 04:32:46PM +0800, Peter Chen wrote:
> > > If we also want get things from DT node, we need to get node from
> > > glue layer as this node is NOT belonged to chipidea core's pdev.
> > 
> > which DT node are you talking about ? you want chipidea core to have
> > access to glue's DT node ? Are you insane ?
> 
> Please read chipidea code, the chipidea core (pdev & platform data)
> is not created by DT, it is created by glue layer.

I know that, what I'm arguing is that instead of going through all loops
and hoops as you're doing, make DTS create the pdev.

> > > Convert platform data to DT for chipidea core is a big job,
> > 
> > no it's not.
> 
> I need to change all msm and pci board related files.

that's just life. Or, you could meanwhile use OF_DEV_AUX_DATA()

> > yeah, but this patch makes no sense. The right thing (TM) to do is, as I
> > have said multiple times before, to teach chipidea core about DT. Please
> > stop trying to find excuses for not doing the work you need to do; You
> > would've already done it if you had spent so much of everybody's time
> > trying to find excuses why not to do it and why Alex should accept your
> > broken patches.
> 
> First let chipidea core know about DT is out of this patchset.

not entirely, since you need access to the regulator from chipidea core
and your regulator bindings are all passed via DT.

There's a clear dependency there.

> Second, why you think it is a broken patch, it just follows current
> framework. Besides, if you have concerns about DT stuff for chipidea
> core, why not mention it when sasche added dr_mode and phy_mode at
> that time?

Well, people make mistakes, right ? That was my mistake, but it doesn't
we have to let the mistake proliferate.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130308/f4ce513d/attachment.sig>

[PATCH v11 6/9] usb: chipidea: add vbus regulator control

[Peter Chen]

On Fri, Mar 08, 2013 at 10:42:35AM +0200, Felipe Balbi wrote:
> 
> > Second, why you think it is a broken patch, it just follows current
> > framework. Besides, if you have concerns about DT stuff for chipidea
> > core, why not mention it when sasche added dr_mode and phy_mode at
> > that time?
> 
> Well, people make mistakes, right ? That was my mistake, but it doesn't
> we have to let the mistake proliferate.
> 

That means I need to post below two patches before this patchset can
be accepted.

- Let udc-core know vbus
- Convert platform data to DT for chipidea core driver

-- 

Best Regards,
Peter Chen

[PATCH v11 6/9] usb: chipidea: add vbus regulator control

[Felipe Balbi]

On Fri, Mar 08, 2013 at 04:52:02PM +0800, Peter Chen wrote:
> On Fri, Mar 08, 2013 at 10:42:35AM +0200, Felipe Balbi wrote:
> > 
> > > Second, why you think it is a broken patch, it just follows current
> > > framework. Besides, if you have concerns about DT stuff for chipidea
> > > core, why not mention it when sasche added dr_mode and phy_mode at
> > > that time?
> > 
> > Well, people make mistakes, right ? That was my mistake, but it doesn't
> > we have to let the mistake proliferate.
> > 
> 
> That means I need to post below two patches before this patchset can
> be accepted.
> 
> - Let udc-core know vbus
> - Convert platform data to DT for chipidea core driver

right, ask for help if you need but don't try to deliver patches which
are conceptually wrong.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130308/87a201c3/attachment-0001.sig>

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

[Peter Chen]

On Thu, Mar 07, 2013 at 01:50:24PM +0800, Peter Chen wrote:
> On Wed, Mar 06, 2013 at 07:09:34PM +0200, Alexander Shishkin wrote:
> > > - start/stop API are used at otg id switch and probe routine
> > >
> > > - Defer some gadget operations at ci's delayed work queue
> > 
> > When I asked you to reorder patches, I didn't mean squash them all
> > into one. Now you have a patch that does 5 different things and none
> > of them properly, because you still need to amend these changes later
> > in the patchset, like the patch 7, where you remove the delayed work,
> > which is added in this patch. At the same time, you have bits in this
> > patch that should be moved to other patches. See comments below.
> > 
> 
> I am sorry to let you review difficult, as I changed/added patch according
> to function change, but not updated previous patches.
> 
> I will re-organize all patches according to functionalities.
> Besides, can you help review all patches during next time, in that case,
> I can know which patch is ok.
> 
> > 
> > You remove this function in patch 7. Why add it in the first place?
> > 
> 
> As it is needed at patch 7 to let the function work
> 
> > > +
> > > +static inline void ci_role_destroy(struct ci13xxx *ci)
> > > +{
> > > +       ci_hdrc_gadget_destroy(ci);
> > > +       ci_hdrc_host_destroy(ci);
> > > +}
> > 
> > You shouldn't use "inline" here. And the name of the function is also
> > ambiguous, since it destroys all roles. It would be better to just
> > call both _destroys(), like I suggested in the diff I gave you some
> > time ago.
> 
> Will change.
> 
> > 
> > > +
> > >  static ssize_t show_role(struct device *dev, struct device_attribute *attr,
> > >                          char *buf)
> > >  {
> > > @@ -352,25 +468,50 @@ static ssize_t store_role(struct device *dev, struct device_attribute *attr,
> > >
> > >  static DEVICE_ATTR(role, S_IRUSR | S_IWUSR, show_role, store_role);
> > >
> > > +static bool ci_is_otg_capable(struct ci13xxx *ci)
> > > +{
> > > +       return hw_read(ci, CAP_DCCPARAMS, DCCPARAMS_DC | DCCPARAMS_HC)
> > > +               == (DCCPARAMS_DC | DCCPARAMS_HC);
> > > +}
> > 
> > Can't we
> > a) remember this value so that we don't have to read the register at
> > every interrupt
> 
> Yes.
> 
> > b) also take into account the fact that DCCPARAMS doesn't read
> > correctly on all chipideas? (see dr_mode patches)
> > 
> 
> We have discussed it, but you have not given me exactly answer
> "Which situation we can read OTGSC"?
> 
> 1. We Can't read OTGSC if DCCPARAMS shows it is only device-capable
> 2. DCCPARAMS doesn't correctly for some chips
> 3. If we depend on dr_mode, OTGSC should be read when dr_mode = "peripheral",
> in that case, the condition is ci->roles[CI_ROLE_GADGET].
> 
> If the dr_mode is not set by DT, the 1 and 3 is conflict.
> 
> 
> > >
> > > -       if (ci->is_otg)
> > > +       if (ci_is_otg_capable(ci))
> > 
> > ci->is_otg is actually the right thing to use, we should instead make
> > sure that it's set properly
> > 
> 
> Yes, what is ci->is_otg stands for in your opinion?
> In my patchset, it means it supportes partial OTG (id switch) function.
> 
> > >         if (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]) {
> > > +               dev_dbg(dev, "support otg\n");
> > >                 ci->is_otg = true;
> > > +               /* if otg is supported, create struct usb_otg */
> > > +               ci_hdrc_otg_init(ci);
> > >                 /* ID pin needs 1ms debouce time, we delay 2ms for safe */
> > >                 mdelay(2);
> > >                 ci->role = ci_otg_role(ci);
> > 
> > I'm thinking that this should be based on dr_mode patches and not the
> > other way around.
> > 
> 
> No, ci->role means which role is currently.
> And it is otg mode already as 
> (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]).
> 
> > >
> > >  static int ci_otg_set_peripheral(struct usb_otg *otg,
> > >                 struct usb_gadget *periph)
> > > @@ -55,6 +62,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;
> > > +       ci_enable_otg_interrupt(ci, OTGSC_IDIE);
> > 
> > There should be a counter-action to ci_hdrc_otg_init().
> > Btw, why can't clear_otg_interrupt() be called from here as well
> > instead of hw_device_init()? 
> 
> What the criterion for reading OTGSC?
> What means otg capable and how do we know it is otg capable?
> 
> > This function should only be called for
> > otg capable devices, so it should be safe. And while at it, I think
> > this whole otg.c/otg.h part of this patch should be moved to patch 2.
> > 
> 
> OK.
> 
> > >
> > > +static int udc_id_switch_for_device(struct ci13xxx *ci)
> > 
> > I would use _to_ instead of _for_, for clarity.
> > 
> > > +{
> > > +       ci_clear_otg_interrupt(ci, OTGSC_BSVIS);
> > > +       ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static void udc_id_switch_for_host(struct ci13xxx *ci)
> > 

Hi Alan, 

Would you like to answer my questions, esp for
"the situation to read OTGSC", we discussed it lots of times,
but seems has no conclusion.

Thanks.
-- 


Best Regards,
Peter Chen