* [RESEND PATCH v5 1/7] Revert "USB: chipidea: add vbus detect for udc"
2013-01-21 1:56 [RESEND PATCH v5 0/7] Add fully tested id switch and vbus connect detect support for Chipidea Peter Chen
@ 2013-01-21 1:56 ` Peter Chen
2013-01-21 1:56 ` [RESEND PATCH v5 2/7] usb: chipidea: add otg file Peter Chen
` (5 subsequent siblings)
6 siblings, 0 replies; 24+ messages in thread
From: Peter Chen @ 2013-01-21 1:56 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RESEND PATCH v5 2/7] usb: chipidea: add otg file
2013-01-21 1:56 [RESEND PATCH v5 0/7] Add fully tested id switch and vbus connect detect support for Chipidea Peter Chen
2013-01-21 1:56 ` [RESEND PATCH v5 1/7] Revert "USB: chipidea: add vbus detect for udc" Peter Chen
@ 2013-01-21 1:56 ` Peter Chen
2013-01-21 1:56 ` [RESEND PATCH v5 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect Peter Chen
` (4 subsequent siblings)
6 siblings, 0 replies; 24+ messages in thread
From: Peter Chen @ 2013-01-21 1:56 UTC (permalink / raw)
To: linux-arm-kernel
Implement struct usb_otg, In that way, calling otg_set_peripheral
will not be failed at udc.c.
Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
drivers/usb/chipidea/Makefile | 2 +-
drivers/usb/chipidea/ci.h | 2 +
drivers/usb/chipidea/otg.c | 60 +++++++++++++++++++++++++++++++++++++++++
drivers/usb/chipidea/otg.h | 6 ++++
4 files changed, 69 insertions(+), 1 deletions(-)
diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
index d92ca32..11f513c 100644
--- a/drivers/usb/chipidea/Makefile
+++ b/drivers/usb/chipidea/Makefile
@@ -2,7 +2,7 @@ ccflags-$(CONFIG_USB_CHIPIDEA_DEBUG) := -DDEBUG
obj-$(CONFIG_USB_CHIPIDEA) += ci_hdrc.o
-ci_hdrc-y := core.o
+ci_hdrc-y := core.o otg.o
ci_hdrc-$(CONFIG_USB_CHIPIDEA_UDC) += udc.o
ci_hdrc-$(CONFIG_USB_CHIPIDEA_HOST) += host.o
ci_hdrc-$(CONFIG_USB_CHIPIDEA_DEBUG) += debug.o
diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index d738603..8702871 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -129,6 +129,7 @@ struct hw_bank {
* @vbus_active: is VBUS active
* @transceiver: pointer to USB PHY, if any
* @hcd: pointer to usb_hcd for ehci host driver
+ * @otg: for otg support
*/
struct ci13xxx {
struct device *dev;
@@ -164,6 +165,7 @@ struct ci13xxx {
bool global_phy;
struct usb_phy *transceiver;
struct usb_hcd *hcd;
+ struct usb_otg otg;
};
static inline struct ci_role_driver *ci_role(struct ci13xxx *ci)
diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
new file mode 100644
index 0000000..7dea3b3
--- /dev/null
+++ b/drivers/usb/chipidea/otg.c
@@ -0,0 +1,60 @@
+/*
+ * otg.c - ChipIdea USB IP core OTG driver
+ *
+ * Copyright (C) 2012 Freescale Semiconductor, Inc.
+ *
+ * Author: Peter Chen
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/usb/gadget.h>
+#include <linux/usb/otg.h>
+#include <linux/usb/chipidea.h>
+
+#include "ci.h"
+#include "udc.h"
+#include "bits.h"
+#include "host.h"
+#include "debug.h"
+
+static int ci_otg_set_peripheral(struct usb_otg *otg,
+ struct usb_gadget *periph)
+{
+ otg->gadget = periph;
+
+ return 0;
+}
+
+static int ci_otg_set_host(struct usb_otg *otg, struct usb_bus *host)
+{
+ otg->host = host;
+
+ return 0;
+}
+
+/**
+ * ci_hdrc_otg_init - initialize device related bits
+ * ci: the controller
+ *
+ * This function create otg struct, if the device can switch between
+ * device and host.
+ */
+int ci_hdrc_otg_init(struct ci13xxx *ci)
+{
+ /* Useless at current */
+ ci->otg.set_peripheral = ci_otg_set_peripheral;
+ ci->otg.set_host = ci_otg_set_host;
+ if (!IS_ERR_OR_NULL(ci->transceiver))
+ ci->transceiver->otg = &ci->otg;
+
+ return 0;
+}
diff --git a/drivers/usb/chipidea/otg.h b/drivers/usb/chipidea/otg.h
new file mode 100644
index 0000000..b4c6b3e
--- /dev/null
+++ b/drivers/usb/chipidea/otg.h
@@ -0,0 +1,6 @@
+#ifndef __DRIVERS_USB_CHIPIDEA_OTG_H
+#define __DRIVERS_USB_CHIPIDEA_OTG_H
+
+int ci_hdrc_otg_init(struct ci13xxx *ci);
+
+#endif /* __DRIVERS_USB_CHIPIDEA_OTG_H */
--
1.7.0.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RESEND PATCH v5 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect
2013-01-21 1:56 [RESEND PATCH v5 0/7] Add fully tested id switch and vbus connect detect support for Chipidea Peter Chen
2013-01-21 1:56 ` [RESEND PATCH v5 1/7] Revert "USB: chipidea: add vbus detect for udc" Peter Chen
2013-01-21 1:56 ` [RESEND PATCH v5 2/7] usb: chipidea: add otg file Peter Chen
@ 2013-01-21 1:56 ` Peter Chen
2013-01-24 14:06 ` Alexander Shishkin
` (2 more replies)
2013-01-21 1:56 ` [RESEND PATCH v5 4/7] usb: chipidea: consolidate ci_role_driver's API for both roles Peter Chen
` (3 subsequent siblings)
6 siblings, 3 replies; 24+ messages in thread
From: Peter Chen @ 2013-01-21 1:56 UTC (permalink / raw)
To: linux-arm-kernel
The main design flow is the same with msm otg driver, that is the id and
vbus interrupt are handled at core driver, others are handled by
individual drivers.
- At former design, when switch usb role from device->host, it will call
udc_stop, it will remove the gadget driver, so when switch role
from host->device, it can't add gadget driver any more.
At new design, when role switch occurs, the gadget just calls
usb_gadget_vbus_disconnect/usb_gadget_vbus_connect as well as
reset controller, it will not free any device/gadget structure
- Add vbus connect and disconnect to core interrupt handler, it can
notify udc driver by calling usb_gadget_vbus_disconnect
/usb_gadget_vbus_connect.
Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
drivers/usb/chipidea/bits.h | 10 +++
drivers/usb/chipidea/ci.h | 8 ++-
drivers/usb/chipidea/core.c | 177 ++++++++++++++++++++++++++++++++++++++----
drivers/usb/chipidea/otg.c | 28 +++++---
drivers/usb/chipidea/otg.h | 3 +
drivers/usb/chipidea/udc.c | 2 +
6 files changed, 200 insertions(+), 28 deletions(-)
diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
index 050de85..ba9c6ef 100644
--- a/drivers/usb/chipidea/bits.h
+++ b/drivers/usb/chipidea/bits.h
@@ -65,11 +65,21 @@
#define OTGSC_ASVIS BIT(18)
#define OTGSC_BSVIS BIT(19)
#define OTGSC_BSEIS BIT(20)
+#define OTGSC_1MSIS BIT(21)
+#define OTGSC_DPIS BIT(22)
#define OTGSC_IDIE BIT(24)
#define OTGSC_AVVIE BIT(25)
#define OTGSC_ASVIE BIT(26)
#define OTGSC_BSVIE BIT(27)
#define OTGSC_BSEIE BIT(28)
+#define OTGSC_1MSIE BIT(29)
+#define OTGSC_DPIE BIT(30)
+#define OTGSC_INT_EN_BITS (OTGSC_IDIE | OTGSC_AVVIE | OTGSC_ASVIE \
+ | OTGSC_BSVIE | OTGSC_BSEIE | OTGSC_1MSIE \
+ | OTGSC_DPIE)
+#define OTGSC_INT_STATUS_BITS (OTGSC_IDIS | OTGSC_AVVIS | OTGSC_ASVIS \
+ | OTGSC_BSVIS | OTGSC_BSEIS | OTGSC_1MSIS \
+ | OTGSC_DPIS)
/* USBMODE */
#define USBMODE_CM (0x03UL << 0)
diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index 8702871..325d790 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -130,6 +130,7 @@ struct hw_bank {
* @transceiver: pointer to USB PHY, if any
* @hcd: pointer to usb_hcd for ehci host driver
* @otg: for otg support
+ * @events: events for otg, and handled@ci_role_work
*/
struct ci13xxx {
struct device *dev;
@@ -140,6 +141,7 @@ struct ci13xxx {
enum ci_role role;
bool is_otg;
struct work_struct work;
+ struct delayed_work dwork;
struct workqueue_struct *wq;
struct dma_pool *qh_pool;
@@ -165,7 +167,9 @@ struct ci13xxx {
bool global_phy;
struct usb_phy *transceiver;
struct usb_hcd *hcd;
- struct usb_otg otg;
+ struct usb_otg otg;
+ bool id_event;
+ bool b_sess_valid_event;
};
static inline struct ci_role_driver *ci_role(struct ci13xxx *ci)
@@ -314,4 +318,6 @@ int hw_port_test_set(struct ci13xxx *ci, u8 mode);
u8 hw_port_test_get(struct ci13xxx *ci);
+void ci_handle_vbus_change(struct ci13xxx *ci);
+
#endif /* __DRIVERS_USB_CHIPIDEA_CI_H */
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index aebf695..f8f8484 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -73,6 +73,7 @@
#include "bits.h"
#include "host.h"
#include "debug.h"
+#include "otg.h"
/* Controller register map */
static uintptr_t ci_regs_nolpm[] = {
@@ -199,6 +200,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);
@@ -265,24 +274,124 @@ static enum ci_role ci_otg_role(struct ci13xxx *ci)
}
/**
- * ci_role_work - perform role changing based on ID pin
- * @work: work struct
+ * hw_wait_reg: wait the register value
+ *
+ * Sometimes, it needs to wait register value before going on.
+ * Eg, when switch to device mode, the vbus value should be lower
+ * than OTGSC_BSV before connects to host.
+ *
+ * @ci: the controller
+ * @reg: register index
+ * @mask: mast bit
+ * @value: the bit value to wait
+ * @timeout: timeout to indicate an error
+ *
+ * This function returns an error code if timeout
*/
-static void ci_role_work(struct work_struct *work)
+static int hw_wait_reg(struct ci13xxx *ci, enum ci13xxx_regs reg, u32 mask,
+ u32 value, unsigned long timeout)
+{
+ unsigned long elapse = jiffies + timeout;
+
+ while (hw_read(ci, reg, mask) != value) {
+ if (time_after(jiffies, elapse)) {
+ dev_err(ci->dev, "timeout waiting for %08x in %d\n",
+ mask, reg);
+ return -ETIMEDOUT;
+ }
+ msleep(20);
+ }
+
+ return 0;
+}
+
+#define CI_VBUS_STABLE_TIMEOUT 500
+static void ci_handle_id_switch(struct ci13xxx *ci)
{
- struct ci13xxx *ci = container_of(work, struct ci13xxx, work);
enum ci_role role = ci_otg_role(ci);
if (role != ci->role) {
dev_dbg(ci->dev, "switching from %s to %s\n",
ci_role(ci)->name, ci->roles[role]->name);
- ci_role_stop(ci);
- ci_role_start(ci, role);
- enable_irq(ci->irq);
+ /* 1. Finish the current role */
+ if (ci->role == CI_ROLE_GADGET) {
+ usb_gadget_vbus_disconnect(&ci->gadget);
+ /* host doesn't care B_SESSION_VALID event */
+ ci_clear_otg_interrupt(ci, OTGSC_BSVIS);
+ ci_disable_otg_interrupt(ci, OTGSC_BSVIE);
+ ci->role = CI_ROLE_END;
+ /* reset controller */
+ hw_device_reset(ci, USBMODE_CM_IDLE);
+ } else if (ci->role == CI_ROLE_HOST) {
+ ci_role_stop(ci);
+ /* reset controller */
+ hw_device_reset(ci, USBMODE_CM_IDLE);
+ }
+
+ /* 2. Turn on/off vbus according to coming role */
+ if (ci_otg_role(ci) == CI_ROLE_GADGET) {
+ otg_set_vbus(&ci->otg, false);
+ /* wait vbus lower than OTGSC_BSV */
+ hw_wait_reg(ci, OP_OTGSC, OTGSC_BSV, 0,
+ CI_VBUS_STABLE_TIMEOUT);
+ } else if (ci_otg_role(ci) == CI_ROLE_HOST) {
+ otg_set_vbus(&ci->otg, true);
+ /* wait vbus higher than OTGSC_AVV */
+ hw_wait_reg(ci, OP_OTGSC, OTGSC_AVV, OTGSC_AVV,
+ CI_VBUS_STABLE_TIMEOUT);
+ }
+
+ /* 3. Begin the new role */
+ if (ci_otg_role(ci) == CI_ROLE_GADGET) {
+ ci->role = role;
+ ci_clear_otg_interrupt(ci, OTGSC_BSVIS);
+ ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
+ } else if (ci_otg_role(ci) == CI_ROLE_HOST) {
+ ci_role_start(ci, role);
+ }
}
}
+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);
+
+ otg_set_vbus(&ci->otg, true);
+
+}
+
static ssize_t show_role(struct device *dev, struct device_attribute *attr,
char *buf)
{
@@ -321,19 +430,36 @@ static irqreturn_t ci_irq(int irq, void *data)
irqreturn_t ret = IRQ_NONE;
u32 otgsc = 0;
- if (ci->is_otg)
- otgsc = hw_read(ci, OP_OTGSC, ~0);
+ 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 ((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;
}
@@ -398,6 +524,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
struct resource *res;
void __iomem *base;
int ret;
+ u32 otgsc;
if (!dev->platform_data) {
dev_err(dev, "platform data missing\n");
@@ -443,7 +570,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");
@@ -466,7 +594,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);
@@ -483,6 +614,17 @@ static int ci_hdrc_probe(struct platform_device *pdev)
goto rm_wq;
}
+ otgsc = hw_read(ci, OP_OTGSC, ~0);
+ /*
+ * if it is device mode:
+ * - Enable vbus detect
+ * - If it has already connected to host, notify udc
+ */
+ if (ci->role == CI_ROLE_GADGET) {
+ ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
+ ci_handle_vbus_change(ci);
+ }
+
platform_set_drvdata(pdev, ci);
ret = request_irq(ci->irq, ci_irq, IRQF_SHARED, ci->platdata->name,
ci);
@@ -493,8 +635,9 @@ static int ci_hdrc_probe(struct platform_device *pdev)
if (ret)
goto rm_attr;
- if (ci->is_otg)
- hw_write(ci, OP_OTGSC, OTGSC_IDIE, OTGSC_IDIE);
+ /* Handle the situation that usb device at the MicroB to A cable */
+ if (ci->is_otg && !(otgsc & OTGSC_ID))
+ queue_delayed_work(ci->wq, &ci->dwork, msecs_to_jiffies(500));
return ret;
diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
index 7dea3b3..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..83e54bb 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -1371,6 +1371,7 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active)
pm_runtime_get_sync(&_gadget->dev);
hw_device_reset(ci, USBMODE_CM_DC);
hw_device_state(ci, ci->ep0out->qh.dma);
+ dev_dbg(ci->dev, "Connected to host\n");
} else {
hw_device_state(ci, 0);
if (ci->platdata->notify_event)
@@ -1378,6 +1379,7 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active)
CI13XXX_CONTROLLER_STOPPED_EVENT);
_gadget_stop_activity(&ci->gadget);
pm_runtime_put_sync(&_gadget->dev);
+ dev_dbg(ci->dev, "Disconnected from host\n");
}
}
--
1.7.0.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RESEND PATCH v5 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect
2013-01-21 1:56 ` [RESEND PATCH v5 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect Peter Chen
@ 2013-01-24 14:06 ` Alexander Shishkin
2013-01-25 6:13 ` Peter Chen
2013-01-24 15:25 ` Alexander Shishkin
2013-01-30 6:06 ` kishon
2 siblings, 1 reply; 24+ messages in thread
From: Alexander Shishkin @ 2013-01-24 14:06 UTC (permalink / raw)
To: linux-arm-kernel
Peter Chen <peter.chen@freescale.com> writes:
> The main design flow is the same with msm otg driver, that is the id and
> vbus interrupt are handled at core driver, others are handled by
> individual drivers.
>
> - At former design, when switch usb role from device->host, it will call
> udc_stop, it will remove the gadget driver, so when switch role
> from host->device, it can't add gadget driver any more.
> At new design, when role switch occurs, the gadget just calls
> usb_gadget_vbus_disconnect/usb_gadget_vbus_connect as well as
> reset controller, it will not free any device/gadget structure
>
> - Add vbus connect and disconnect to core interrupt handler, it can
> notify udc driver by calling usb_gadget_vbus_disconnect
> /usb_gadget_vbus_connect.
>
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
A few comments below.
> ---
> drivers/usb/chipidea/bits.h | 10 +++
> drivers/usb/chipidea/ci.h | 8 ++-
> drivers/usb/chipidea/core.c | 177 ++++++++++++++++++++++++++++++++++++++----
> drivers/usb/chipidea/otg.c | 28 +++++---
> drivers/usb/chipidea/otg.h | 3 +
> drivers/usb/chipidea/udc.c | 2 +
> 6 files changed, 200 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
> index 050de85..ba9c6ef 100644
> --- a/drivers/usb/chipidea/bits.h
> +++ b/drivers/usb/chipidea/bits.h
> @@ -65,11 +65,21 @@
> #define OTGSC_ASVIS BIT(18)
> #define OTGSC_BSVIS BIT(19)
> #define OTGSC_BSEIS BIT(20)
> +#define OTGSC_1MSIS BIT(21)
> +#define OTGSC_DPIS BIT(22)
> #define OTGSC_IDIE BIT(24)
> #define OTGSC_AVVIE BIT(25)
> #define OTGSC_ASVIE BIT(26)
> #define OTGSC_BSVIE BIT(27)
> #define OTGSC_BSEIE BIT(28)
> +#define OTGSC_1MSIE BIT(29)
> +#define OTGSC_DPIE BIT(30)
> +#define OTGSC_INT_EN_BITS (OTGSC_IDIE | OTGSC_AVVIE | OTGSC_ASVIE \
> + | OTGSC_BSVIE | OTGSC_BSEIE | OTGSC_1MSIE \
> + | OTGSC_DPIE)
> +#define OTGSC_INT_STATUS_BITS (OTGSC_IDIS | OTGSC_AVVIS | OTGSC_ASVIS \
> + | OTGSC_BSVIS | OTGSC_BSEIS | OTGSC_1MSIS \
> + | OTGSC_DPIS)
>
> /* USBMODE */
> #define USBMODE_CM (0x03UL << 0)
> diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> index 8702871..325d790 100644
> --- a/drivers/usb/chipidea/ci.h
> +++ b/drivers/usb/chipidea/ci.h
> @@ -130,6 +130,7 @@ struct hw_bank {
> * @transceiver: pointer to USB PHY, if any
> * @hcd: pointer to usb_hcd for ehci host driver
> * @otg: for otg support
> + * @events: events for otg, and handled at ci_role_work
> */
> struct ci13xxx {
> struct device *dev;
> @@ -140,6 +141,7 @@ struct ci13xxx {
> enum ci_role role;
> bool is_otg;
> struct work_struct work;
> + struct delayed_work dwork;
> struct workqueue_struct *wq;
>
> struct dma_pool *qh_pool;
> @@ -165,7 +167,9 @@ struct ci13xxx {
> bool global_phy;
> struct usb_phy *transceiver;
> struct usb_hcd *hcd;
> - struct usb_otg otg;
> + struct usb_otg otg;
> + bool id_event;
> + bool b_sess_valid_event;
> };
>
> static inline struct ci_role_driver *ci_role(struct ci13xxx *ci)
> @@ -314,4 +318,6 @@ int hw_port_test_set(struct ci13xxx *ci, u8 mode);
>
> u8 hw_port_test_get(struct ci13xxx *ci);
>
> +void ci_handle_vbus_change(struct ci13xxx *ci);
> +
> #endif /* __DRIVERS_USB_CHIPIDEA_CI_H */
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index aebf695..f8f8484 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -73,6 +73,7 @@
> #include "bits.h"
> #include "host.h"
> #include "debug.h"
> +#include "otg.h"
>
> /* Controller register map */
> static uintptr_t ci_regs_nolpm[] = {
> @@ -199,6 +200,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);
>
> @@ -265,24 +274,124 @@ static enum ci_role ci_otg_role(struct ci13xxx *ci)
> }
>
> /**
> - * ci_role_work - perform role changing based on ID pin
> - * @work: work struct
> + * hw_wait_reg: wait the register value
> + *
> + * Sometimes, it needs to wait register value before going on.
> + * Eg, when switch to device mode, the vbus value should be lower
> + * than OTGSC_BSV before connects to host.
> + *
> + * @ci: the controller
> + * @reg: register index
> + * @mask: mast bit
> + * @value: the bit value to wait
> + * @timeout: timeout to indicate an error
> + *
> + * This function returns an error code if timeout
> */
> -static void ci_role_work(struct work_struct *work)
> +static int hw_wait_reg(struct ci13xxx *ci, enum ci13xxx_regs reg, u32 mask,
> + u32 value, unsigned long timeout)
> +{
> + unsigned long elapse = jiffies + timeout;
> +
> + while (hw_read(ci, reg, mask) != value) {
> + if (time_after(jiffies, elapse)) {
> + dev_err(ci->dev, "timeout waiting for %08x in %d\n",
> + mask, reg);
> + return -ETIMEDOUT;
> + }
> + msleep(20);
> + }
> +
> + return 0;
> +}
> +
> +#define CI_VBUS_STABLE_TIMEOUT 500
> +static void ci_handle_id_switch(struct ci13xxx *ci)
> {
> - struct ci13xxx *ci = container_of(work, struct ci13xxx, work);
> enum ci_role role = ci_otg_role(ci);
>
> if (role != ci->role) {
> dev_dbg(ci->dev, "switching from %s to %s\n",
> ci_role(ci)->name, ci->roles[role]->name);
>
> - ci_role_stop(ci);
> - ci_role_start(ci, role);
> - enable_irq(ci->irq);
> + /* 1. Finish the current role */
> + if (ci->role == CI_ROLE_GADGET) {
> + usb_gadget_vbus_disconnect(&ci->gadget);
> + /* host doesn't care B_SESSION_VALID event */
> + ci_clear_otg_interrupt(ci, OTGSC_BSVIS);
> + ci_disable_otg_interrupt(ci, OTGSC_BSVIE);
> + ci->role = CI_ROLE_END;
> + /* reset controller */
> + hw_device_reset(ci, USBMODE_CM_IDLE);
> + } else if (ci->role == CI_ROLE_HOST) {
> + ci_role_stop(ci);
> + /* reset controller */
> + hw_device_reset(ci, USBMODE_CM_IDLE);
> + }
> +
> + /* 2. Turn on/off vbus according to coming role */
> + if (ci_otg_role(ci) == CI_ROLE_GADGET) {
> + otg_set_vbus(&ci->otg, false);
> + /* wait vbus lower than OTGSC_BSV */
> + hw_wait_reg(ci, OP_OTGSC, OTGSC_BSV, 0,
> + CI_VBUS_STABLE_TIMEOUT);
> + } else if (ci_otg_role(ci) == CI_ROLE_HOST) {
> + otg_set_vbus(&ci->otg, true);
> + /* wait vbus higher than OTGSC_AVV */
> + hw_wait_reg(ci, OP_OTGSC, OTGSC_AVV, OTGSC_AVV,
> + CI_VBUS_STABLE_TIMEOUT);
> + }
> +
> + /* 3. Begin the new role */
> + if (ci_otg_role(ci) == CI_ROLE_GADGET) {
> + ci->role = role;
> + ci_clear_otg_interrupt(ci, OTGSC_BSVIS);
> + ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
> + } else if (ci_otg_role(ci) == CI_ROLE_HOST) {
> + ci_role_start(ci, role);
> + }
> }
> }
>
> +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);
> +
> + otg_set_vbus(&ci->otg, true);
> +
> +}
> +
> static ssize_t show_role(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> @@ -321,19 +430,36 @@ static irqreturn_t ci_irq(int irq, void *data)
> irqreturn_t ret = IRQ_NONE;
> u32 otgsc = 0;
>
> - if (ci->is_otg)
> - otgsc = hw_read(ci, OP_OTGSC, ~0);
> + otgsc = hw_read(ci, OP_OTGSC, ~0);
My spec says that in non-otg implementations OTGSC is reserved. We
probably shouldn't try any of this for such devices.
>
> - 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 ((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;
> }
>
> @@ -398,6 +524,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> struct resource *res;
> void __iomem *base;
> int ret;
> + u32 otgsc;
>
> if (!dev->platform_data) {
> dev_err(dev, "platform data missing\n");
> @@ -443,7 +570,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");
> @@ -466,7 +594,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);
> @@ -483,6 +614,17 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> goto rm_wq;
> }
>
> + otgsc = hw_read(ci, OP_OTGSC, ~0);
> + /*
> + * if it is device mode:
> + * - Enable vbus detect
> + * - If it has already connected to host, notify udc
> + */
> + if (ci->role == CI_ROLE_GADGET) {
> + ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
We should only do this for otg capable devices, otherwise OTGSC is
reserved.
> + ci_handle_vbus_change(ci);
Shouldn't this be part of the gadget role start like I suggested a
couple of months back? Maybe ci_enable_otg_interrupt() can go there too.
> + }
> +
> platform_set_drvdata(pdev, ci);
> ret = request_irq(ci->irq, ci_irq, IRQF_SHARED, ci->platdata->name,
> ci);
> @@ -493,8 +635,9 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> if (ret)
> goto rm_attr;
>
> - if (ci->is_otg)
> - hw_write(ci, OP_OTGSC, OTGSC_IDIE, OTGSC_IDIE);
> + /* Handle the situation that usb device at the MicroB to A cable */
> + if (ci->is_otg && !(otgsc & OTGSC_ID))
> + queue_delayed_work(ci->wq, &ci->dwork, msecs_to_jiffies(500));
>
> return ret;
>
> diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
> index 7dea3b3..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..83e54bb 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -1371,6 +1371,7 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active)
> pm_runtime_get_sync(&_gadget->dev);
> hw_device_reset(ci, USBMODE_CM_DC);
> hw_device_state(ci, ci->ep0out->qh.dma);
> + dev_dbg(ci->dev, "Connected to host\n");
> } else {
> hw_device_state(ci, 0);
> if (ci->platdata->notify_event)
> @@ -1378,6 +1379,7 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active)
> CI13XXX_CONTROLLER_STOPPED_EVENT);
> _gadget_stop_activity(&ci->gadget);
> pm_runtime_put_sync(&_gadget->dev);
> + dev_dbg(ci->dev, "Disconnected from host\n");
> }
> }
>
> --
> 1.7.0.4
^ permalink raw reply [flat|nested] 24+ messages in thread* [RESEND PATCH v5 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect
2013-01-24 14:06 ` Alexander Shishkin
@ 2013-01-25 6:13 ` Peter Chen
0 siblings, 0 replies; 24+ messages in thread
From: Peter Chen @ 2013-01-25 6:13 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jan 24, 2013 at 04:06:48PM +0200, Alexander Shishkin wrote:
> Peter Chen <peter.chen@freescale.com> writes:
>
> > static ssize_t show_role(struct device *dev, struct device_attribute *attr,
> > char *buf)
> > {
> > @@ -321,19 +430,36 @@ static irqreturn_t ci_irq(int irq, void *data)
> > irqreturn_t ret = IRQ_NONE;
> > u32 otgsc = 0;
> >
> > - if (ci->is_otg)
> > - otgsc = hw_read(ci, OP_OTGSC, ~0);
> > + otgsc = hw_read(ci, OP_OTGSC, ~0);
>
> My spec says that in non-otg implementations OTGSC is reserved. We
> probably shouldn't try any of this for such devices.
OK, will change.
> >
> > + otgsc = hw_read(ci, OP_OTGSC, ~0);
> > + /*
> > + * if it is device mode:
> > + * - Enable vbus detect
> > + * - If it has already connected to host, notify udc
> > + */
> > + if (ci->role == CI_ROLE_GADGET) {
> > + ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
>
> We should only do this for otg capable devices, otherwise OTGSC is
> reserved.
We can consider all device mode supported device is otg capable
devices for chipidea controller, do you think so? There should be
no devcie-only capable device. Besides, vbus is indicated at OTGSC,
we need to use it to know connect and disconnect.
>
> > + ci_handle_vbus_change(ci);
>
> Shouldn't this be part of the gadget role start like I suggested a
> couple of months back? Maybe ci_enable_otg_interrupt() can go there too.
Role start/stop will only be called during role switch (ID switch interrupt).
This code is used to handle the case that boots up board with usb cable
connects to pc, then insmod gadget module. In this case, the vbus interrupt
will not occur due to no vbus change, so the ci->vbus_active is not set,
then, after insmod gadget module, the hw_device_reset will not be called,
then, the enumeration will not be begun due to run/stop bit is not set.
In fact, it only do set ci->vbus_active. This code can be replaced with:
ci->vbus_active = 1.
If you prefer this way, I will change.
--
Best Regards,
Peter Chen
^ permalink raw reply [flat|nested] 24+ messages in thread
* [RESEND PATCH v5 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect
2013-01-21 1:56 ` [RESEND PATCH v5 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect Peter Chen
2013-01-24 14:06 ` Alexander Shishkin
@ 2013-01-24 15:25 ` Alexander Shishkin
2013-01-25 6:28 ` Peter Chen
2013-01-30 6:06 ` kishon
2 siblings, 1 reply; 24+ messages in thread
From: Alexander Shishkin @ 2013-01-24 15:25 UTC (permalink / raw)
To: linux-arm-kernel
Peter Chen <peter.chen@freescale.com> writes:
> The main design flow is the same with msm otg driver, that is the id and
> vbus interrupt are handled at core driver, others are handled by
> individual drivers.
>
> - At former design, when switch usb role from device->host, it will call
> udc_stop, it will remove the gadget driver, so when switch role
> from host->device, it can't add gadget driver any more.
> At new design, when role switch occurs, the gadget just calls
> usb_gadget_vbus_disconnect/usb_gadget_vbus_connect as well as
> reset controller, it will not free any device/gadget structure
>
> - Add vbus connect and disconnect to core interrupt handler, it can
> notify udc driver by calling usb_gadget_vbus_disconnect
> /usb_gadget_vbus_connect.
>
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
[snip]
> @@ -483,6 +614,17 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> goto rm_wq;
> }
>
> + otgsc = hw_read(ci, OP_OTGSC, ~0);
> + /*
> + * if it is device mode:
> + * - Enable vbus detect
> + * - If it has already connected to host, notify udc
> + */
> + if (ci->role == CI_ROLE_GADGET) {
> + ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
> + ci_handle_vbus_change(ci);
> + }
> +
Actually, this doesn't work, neither here, nor in udc_start(), where the
next patch moves it. If you start in host role with A plug, unplug it,
plug B and load a gadget module, it won't start till you replug the
cable, which is where vbus detection happens. So, when you say that vbus
detection is "fully tested", what is the test case set that you're
using?
One obvious fix is to move ci_handle_vbus_change() call to
ci13xxx_start(), but I need to think about the whole thing a bit more.
Regards,
--
Alex
^ permalink raw reply [flat|nested] 24+ messages in thread* [RESEND PATCH v5 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect
2013-01-24 15:25 ` Alexander Shishkin
@ 2013-01-25 6:28 ` Peter Chen
2013-01-25 9:40 ` Alexander Shishkin
0 siblings, 1 reply; 24+ messages in thread
From: Peter Chen @ 2013-01-25 6:28 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jan 24, 2013 at 05:25:17PM +0200, Alexander Shishkin wrote:
> Peter Chen <peter.chen@freescale.com> writes:
>
> > The main design flow is the same with msm otg driver, that is the id and
> > vbus interrupt are handled at core driver, others are handled by
> > individual drivers.
> >
> > - At former design, when switch usb role from device->host, it will call
> > udc_stop, it will remove the gadget driver, so when switch role
> > from host->device, it can't add gadget driver any more.
> > At new design, when role switch occurs, the gadget just calls
> > usb_gadget_vbus_disconnect/usb_gadget_vbus_connect as well as
> > reset controller, it will not free any device/gadget structure
> >
> > - Add vbus connect and disconnect to core interrupt handler, it can
> > notify udc driver by calling usb_gadget_vbus_disconnect
> > /usb_gadget_vbus_connect.
> >
> > Signed-off-by: Peter Chen <peter.chen@freescale.com>
>
> [snip]
>
> > @@ -483,6 +614,17 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> > goto rm_wq;
> > }
> >
> > + otgsc = hw_read(ci, OP_OTGSC, ~0);
> > + /*
> > + * if it is device mode:
> > + * - Enable vbus detect
> > + * - If it has already connected to host, notify udc
> > + */
> > + if (ci->role == CI_ROLE_GADGET) {
> > + ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
> > + ci_handle_vbus_change(ci);
> > + }
> > +
>
> Actually, this doesn't work, neither here, nor in udc_start(), where the
> next patch moves it. If you start in host role with A plug, unplug it,
> plug B and load a gadget module,
When we unplug A, device's role start will be called, that is
udc_id_switch_for_device in my patch, it will enable vbus interrupt.
Then, when we plug B, there will be an vbus interrupt, then the
ci->vbus_active will be set, then when we load a gadget module,
the enumeration will begin like I said at last email.
Ok, I say "fully tested" means I tested cases for my development, which
includes: host/device is plugged during boots up, device/host switch,
host only controller, load gadget before/next cable plugs in. If you
think it is not enough, I will skip "fully" when sends next version patch.
--
Best Regards,
Peter Chen
^ permalink raw reply [flat|nested] 24+ messages in thread* [RESEND PATCH v5 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect
2013-01-25 6:28 ` Peter Chen
@ 2013-01-25 9:40 ` Alexander Shishkin
2013-01-28 3:32 ` Peter Chen
0 siblings, 1 reply; 24+ messages in thread
From: Alexander Shishkin @ 2013-01-25 9:40 UTC (permalink / raw)
To: linux-arm-kernel
Peter Chen <peter.chen@freescale.com> writes:
> On Thu, Jan 24, 2013 at 05:25:17PM +0200, Alexander Shishkin wrote:
>> Peter Chen <peter.chen@freescale.com> writes:
>>
>> > The main design flow is the same with msm otg driver, that is the id and
>> > vbus interrupt are handled at core driver, others are handled by
>> > individual drivers.
>> >
>> > - At former design, when switch usb role from device->host, it will call
>> > udc_stop, it will remove the gadget driver, so when switch role
>> > from host->device, it can't add gadget driver any more.
>> > At new design, when role switch occurs, the gadget just calls
>> > usb_gadget_vbus_disconnect/usb_gadget_vbus_connect as well as
>> > reset controller, it will not free any device/gadget structure
>> >
>> > - Add vbus connect and disconnect to core interrupt handler, it can
>> > notify udc driver by calling usb_gadget_vbus_disconnect
>> > /usb_gadget_vbus_connect.
>> >
>> > Signed-off-by: Peter Chen <peter.chen@freescale.com>
>>
>> [snip]
>>
>> > @@ -483,6 +614,17 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>> > goto rm_wq;
>> > }
>> >
>> > + otgsc = hw_read(ci, OP_OTGSC, ~0);
>> > + /*
>> > + * if it is device mode:
>> > + * - Enable vbus detect
>> > + * - If it has already connected to host, notify udc
>> > + */
>> > + if (ci->role == CI_ROLE_GADGET) {
>> > + ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
>> > + ci_handle_vbus_change(ci);
>> > + }
>> > +
>>
>> Actually, this doesn't work, neither here, nor in udc_start(), where the
>> next patch moves it. If you start in host role with A plug, unplug it,
>> plug B and load a gadget module,
>
> When we unplug A, device's role start will be called, that is
> udc_id_switch_for_device in my patch, it will enable vbus interrupt.
> Then, when we plug B, there will be an vbus interrupt, then the
> ci->vbus_active will be set, then when we load a gadget module,
> the enumeration will begin like I said at last email.
What doesn't happen is the reset into device mode (unless you have
_REGS_SHARED set, which by the way seems a bit misleading) and we're
still doing nothing till the vbus interrupt comes. So the REGS_SHARED is
the real problem in this case.
Now, looking at ci13xxx_{start,stop}(), I'm seeing some more code
duplication. What do think about something like this (untested):
---cut---
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index dcac77c..38446de 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -1351,6 +1351,28 @@ static const struct usb_ep_ops usb_ep_ops = {
.fifo_flush = ep_fifo_flush,
};
+static int hw_device_connect(struct ci13xxx *ci, int connect)
+{
+ int ret;
+
+ if (connect) {
+ pm_runtime_get_sync(&ci->gadget.dev);
+ ret = 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)
+ ci->platdata->notify_event(ci,
+ CI13XXX_CONTROLLER_STOPPED_EVENT);
+ ret = _gadget_stop_activity(&ci->gadget);
+ pm_runtime_put_sync(&ci->gadget.dev);
+ dev_dbg(ci->dev, "Disconnected from host\n");
+ }
+
+ return ret;
+}
+
/******************************************************************************
* GADGET block
*****************************************************************************/
@@ -1358,7 +1380,7 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active)
{
struct ci13xxx *ci = container_of(_gadget, struct ci13xxx, gadget);
unsigned long flags;
- int gadget_ready = 0;
+ int gadget_ready = 0, ret = 0;
spin_lock_irqsave(&ci->lock, flags);
ci->vbus_active = is_active;
@@ -1366,24 +1388,10 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active)
gadget_ready = 1;
spin_unlock_irqrestore(&ci->lock, flags);
- if (gadget_ready) {
- if (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)
- ci->platdata->notify_event(ci,
- CI13XXX_CONTROLLER_STOPPED_EVENT);
- _gadget_stop_activity(&ci->gadget);
- pm_runtime_put_sync(&_gadget->dev);
- dev_dbg(ci->dev, "Disconnected from host\n");
- }
- }
+ if (gadget_ready)
+ ret = hw_device_connect(ci, is_active);
- return 0;
+ return ret;
}
static int ci13xxx_wakeup(struct usb_gadget *_gadget)
@@ -1546,20 +1554,9 @@ static int ci13xxx_start(struct usb_gadget *gadget,
spin_lock_irqsave(&ci->lock, flags);
ci->driver = driver;
- pm_runtime_get_sync(&ci->gadget.dev);
- 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)
+ retval = hw_device_connect(ci, 1);
- retval = hw_device_state(ci, ci->ep0out->qh.dma);
- if (retval)
- pm_runtime_put_sync(&ci->gadget.dev);
-
- done:
spin_unlock_irqrestore(&ci->lock, flags);
return retval;
}
@@ -1572,24 +1569,18 @@ static int ci13xxx_stop(struct usb_gadget *gadget,
{
struct ci13xxx *ci = container_of(gadget, struct ci13xxx, gadget);
unsigned long flags;
+ int ret = 0;
spin_lock_irqsave(&ci->lock, flags);
if (ci->vbus_active) {
- hw_device_state(ci, 0);
- 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);
- }
-
- spin_unlock_irqrestore(&ci->lock, flags);
+ ret = hw_device_connect(ci, 0);
+ } else
+ spin_unlock_irqrestore(&ci->lock, flags);
- return 0;
+ return ret;
}
/******************************************************************************
---cut---
That's on top of your patchset. Then, I notice there's a mess with
locking around _gadget_stop_activity(), but that's out of scope of this
patchset, of course.
> Ok, I say "fully tested" means I tested cases for my development, which
> includes: host/device is plugged during boots up, device/host switch,
> host only controller, load gadget before/next cable plugs in. If you
> think it is not enough, I will skip "fully" when sends next version patch.
Sounds good, but it makes sense to mention that it's fully tested on
imx.
Regards,
--
Alex
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RESEND PATCH v5 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect
2013-01-25 9:40 ` Alexander Shishkin
@ 2013-01-28 3:32 ` Peter Chen
0 siblings, 0 replies; 24+ messages in thread
From: Peter Chen @ 2013-01-28 3:32 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jan 25, 2013 at 11:40:10AM +0200, Alexander Shishkin wrote:
> Peter Chen <peter.chen@freescale.com> writes:
>
> > On Thu, Jan 24, 2013 at 05:25:17PM +0200, Alexander Shishkin wrote:
>
> What doesn't happen is the reset into device mode (unless you have
> _REGS_SHARED set, which by the way seems a bit misleading) and we're
> still doing nothing till the vbus interrupt comes. So the REGS_SHARED is
> the real problem in this case.
Yes, at imx it sets REGS_SHARD. The REGS_SHARED is needed at below
conditions:
- System boots up with usb device at OTG port using A-plug cable, at this
situation, any gadget initialization should not do any registers operation,
the related functions are udc_start and ci13xxx_start.
- At udc_irq, we do not want any wakeup, id, and host interrupt to call
internal code.
>
--
Best Regards,
Peter Chen
^ permalink raw reply [flat|nested] 24+ messages in thread
* [RESEND PATCH v5 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect
2013-01-21 1:56 ` [RESEND PATCH v5 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect Peter Chen
2013-01-24 14:06 ` Alexander Shishkin
2013-01-24 15:25 ` Alexander Shishkin
@ 2013-01-30 6:06 ` kishon
2013-01-30 6:57 ` Peter Chen
2 siblings, 1 reply; 24+ messages in thread
From: kishon @ 2013-01-30 6:06 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Monday 21 January 2013 07:26 AM, Peter Chen 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.
>
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
> drivers/usb/chipidea/bits.h | 10 +++
> drivers/usb/chipidea/ci.h | 8 ++-
> drivers/usb/chipidea/core.c | 177 ++++++++++++++++++++++++++++++++++++++----
> drivers/usb/chipidea/otg.c | 28 +++++---
> drivers/usb/chipidea/otg.h | 3 +
> drivers/usb/chipidea/udc.c | 2 +
> 6 files changed, 200 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
> index 050de85..ba9c6ef 100644
> --- a/drivers/usb/chipidea/bits.h
> +++ b/drivers/usb/chipidea/bits.h
> @@ -65,11 +65,21 @@
> #define OTGSC_ASVIS BIT(18)
> #define OTGSC_BSVIS BIT(19)
> #define OTGSC_BSEIS BIT(20)
> +#define OTGSC_1MSIS BIT(21)
> +#define OTGSC_DPIS BIT(22)
> #define OTGSC_IDIE BIT(24)
> #define OTGSC_AVVIE BIT(25)
> #define OTGSC_ASVIE BIT(26)
> #define OTGSC_BSVIE BIT(27)
> #define OTGSC_BSEIE BIT(28)
> +#define OTGSC_1MSIE BIT(29)
> +#define OTGSC_DPIE BIT(30)
> +#define OTGSC_INT_EN_BITS (OTGSC_IDIE | OTGSC_AVVIE | OTGSC_ASVIE \
> + | OTGSC_BSVIE | OTGSC_BSEIE | OTGSC_1MSIE \
> + | OTGSC_DPIE)
> +#define OTGSC_INT_STATUS_BITS (OTGSC_IDIS | OTGSC_AVVIS | OTGSC_ASVIS \
> + | OTGSC_BSVIS | OTGSC_BSEIS | OTGSC_1MSIS \
> + | OTGSC_DPIS)
>
> /* USBMODE */
> #define USBMODE_CM (0x03UL << 0)
> diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> index 8702871..325d790 100644
> --- a/drivers/usb/chipidea/ci.h
> +++ b/drivers/usb/chipidea/ci.h
> @@ -130,6 +130,7 @@ struct hw_bank {
> * @transceiver: pointer to USB PHY, if any
> * @hcd: pointer to usb_hcd for ehci host driver
> * @otg: for otg support
> + * @events: events for otg, and handled at ci_role_work
> */
> struct ci13xxx {
> struct device *dev;
> @@ -140,6 +141,7 @@ struct ci13xxx {
> enum ci_role role;
> bool is_otg;
> struct work_struct work;
> + struct delayed_work dwork;
> struct workqueue_struct *wq;
>
> struct dma_pool *qh_pool;
> @@ -165,7 +167,9 @@ struct ci13xxx {
> bool global_phy;
> struct usb_phy *transceiver;
> struct usb_hcd *hcd;
> - struct usb_otg otg;
> + struct usb_otg otg;
You have added *otg* in previous patch and added a tab for *otg* in this
patch.
> + bool id_event;
> + bool b_sess_valid_event;
> };
>
> static inline struct ci_role_driver *ci_role(struct ci13xxx *ci)
> @@ -314,4 +318,6 @@ int hw_port_test_set(struct ci13xxx *ci, u8 mode);
>
> u8 hw_port_test_get(struct ci13xxx *ci);
>
> +void ci_handle_vbus_change(struct ci13xxx *ci);
> +
> #endif /* __DRIVERS_USB_CHIPIDEA_CI_H */
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index aebf695..f8f8484 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -73,6 +73,7 @@
> #include "bits.h"
> #include "host.h"
> #include "debug.h"
> +#include "otg.h"
>
> /* Controller register map */
> static uintptr_t ci_regs_nolpm[] = {
> @@ -199,6 +200,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);
>
> @@ -265,24 +274,124 @@ static enum ci_role ci_otg_role(struct ci13xxx *ci)
> }
>
> /**
> - * ci_role_work - perform role changing based on ID pin
> - * @work: work struct
> + * hw_wait_reg: wait the register value
> + *
> + * Sometimes, it needs to wait register value before going on.
> + * Eg, when switch to device mode, the vbus value should be lower
> + * than OTGSC_BSV before connects to host.
> + *
> + * @ci: the controller
> + * @reg: register index
> + * @mask: mast bit
> + * @value: the bit value to wait
> + * @timeout: timeout to indicate an error
> + *
> + * This function returns an error code if timeout
> */
> -static void ci_role_work(struct work_struct *work)
> +static int hw_wait_reg(struct ci13xxx *ci, enum ci13xxx_regs reg, u32 mask,
> + u32 value, unsigned long timeout)
> +{
> + unsigned long elapse = jiffies + timeout;
> +
> + while (hw_read(ci, reg, mask) != value) {
> + if (time_after(jiffies, elapse)) {
> + dev_err(ci->dev, "timeout waiting for %08x in %d\n",
> + mask, reg);
> + return -ETIMEDOUT;
> + }
> + msleep(20);
> + }
> +
> + return 0;
> +}
> +
> +#define CI_VBUS_STABLE_TIMEOUT 500
Just curious.. how was this timeout value obtained?
Thanks
Kishon
^ permalink raw reply [flat|nested] 24+ messages in thread* [RESEND PATCH v5 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect
2013-01-30 6:06 ` kishon
@ 2013-01-30 6:57 ` Peter Chen
2013-01-30 10:31 ` Alexander Shishkin
0 siblings, 1 reply; 24+ messages in thread
From: Peter Chen @ 2013-01-30 6:57 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jan 30, 2013 at 11:36:42AM +0530, kishon wrote:
> Hi,
>
> > bool global_phy;
> > struct usb_phy *transceiver;
> > struct usb_hcd *hcd;
> >- struct usb_otg otg;
> >+ struct usb_otg otg;
> You have added *otg* in previous patch and added a tab for *otg* in
> this patch.
thanks, I will change at previous patch
>
> >+
> >+#define CI_VBUS_STABLE_TIMEOUT 500
>
> Just curious.. how was this timeout value obtained?
Just a timeout value, if the vbus goes to required value, it will quit.
Besides, 5s for vbus stable should be enough for an well behaviour hardware.
>
> Thanks
> Kishon
>
--
Best Regards,
Peter Chen
^ permalink raw reply [flat|nested] 24+ messages in thread
* [RESEND PATCH v5 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect
2013-01-30 6:57 ` Peter Chen
@ 2013-01-30 10:31 ` Alexander Shishkin
2013-01-31 2:02 ` Peter Chen
0 siblings, 1 reply; 24+ messages in thread
From: Alexander Shishkin @ 2013-01-30 10:31 UTC (permalink / raw)
To: linux-arm-kernel
Peter Chen <peter.chen@freescale.com> writes:
> On Wed, Jan 30, 2013 at 11:36:42AM +0530, kishon wrote:
>> Hi,
>>
>> > bool global_phy;
>> > struct usb_phy *transceiver;
>> > struct usb_hcd *hcd;
>> >- struct usb_otg otg;
>> >+ struct usb_otg otg;
>> You have added *otg* in previous patch and added a tab for *otg* in
>> this patch.
> thanks, I will change at previous patch
>>
>> >+
>> >+#define CI_VBUS_STABLE_TIMEOUT 500
>>
>> Just curious.. how was this timeout value obtained?
>
> Just a timeout value, if the vbus goes to required value, it will quit.
> Besides, 5s for vbus stable should be enough for an well behaviour hardware.
Can you also mention this in the patch, since it raises questions?
Thanks,
--
Alex
^ permalink raw reply [flat|nested] 24+ messages in thread
* [RESEND PATCH v5 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect
2013-01-30 10:31 ` Alexander Shishkin
@ 2013-01-31 2:02 ` Peter Chen
0 siblings, 0 replies; 24+ messages in thread
From: Peter Chen @ 2013-01-31 2:02 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Jan 30, 2013 at 12:31:04PM +0200, Alexander Shishkin wrote:
> Peter Chen <peter.chen@freescale.com> writes:
>
> > On Wed, Jan 30, 2013 at 11:36:42AM +0530, kishon wrote:
> >> Hi,
> >>
> >> > bool global_phy;
> >> > struct usb_phy *transceiver;
> >> > struct usb_hcd *hcd;
> >> >- struct usb_otg otg;
> >> >+ struct usb_otg otg;
> >> You have added *otg* in previous patch and added a tab for *otg* in
> >> this patch.
> > thanks, I will change at previous patch
> >>
> >> >+
> >> >+#define CI_VBUS_STABLE_TIMEOUT 500
> >>
> >> Just curious.. how was this timeout value obtained?
> >
> > Just a timeout value, if the vbus goes to required value, it will quit.
> > Besides, 5s for vbus stable should be enough for an well behaviour hardware.
>
> Can you also mention this in the patch, since it raises questions?
Sure, do you have other ideas about next version patch?
>
> Thanks,
> --
> Alex
>
--
Best Regards,
Peter Chen
^ permalink raw reply [flat|nested] 24+ messages in thread
* [RESEND PATCH v5 4/7] usb: chipidea: consolidate ci_role_driver's API for both roles
2013-01-21 1:56 [RESEND PATCH v5 0/7] Add fully tested id switch and vbus connect detect support for Chipidea Peter Chen
` (2 preceding siblings ...)
2013-01-21 1:56 ` [RESEND PATCH v5 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect Peter Chen
@ 2013-01-21 1:56 ` Peter Chen
2013-01-24 14:35 ` Alexander Shishkin
2013-01-21 1:56 ` [RESEND PATCH v5 5/7] usb: chipidea: udc: add pullup/pulldown dp at hw_device_state Peter Chen
` (2 subsequent siblings)
6 siblings, 1 reply; 24+ messages in thread
From: Peter Chen @ 2013-01-21 1:56 UTC (permalink / raw)
To: linux-arm-kernel
- Create init/destroy API for probe and remove
- start/stop API are only used otg id switch process
- Create the gadget at ci_hdrc_probe if the gadget is supported
at that port, the main purpose for this is to avoid gadget module
load fail at init.rc
Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
drivers/usb/chipidea/ci.h | 19 +++++++++++-
drivers/usb/chipidea/core.c | 65 ++++++++++++++++++------------------------
drivers/usb/chipidea/host.c | 2 +
drivers/usb/chipidea/udc.c | 33 ++++++++++++++++++++-
4 files changed, 78 insertions(+), 41 deletions(-)
diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index 325d790..00939e6 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -67,14 +67,18 @@ enum ci_role {
/**
* struct ci_role_driver - host/gadget role driver
- * start: start this role
- * stop: stop this role
+ * init: init this role (used at module probe)
+ * start: start this role (used at id switch)
+ * stop: stop this role (used at id switch)
+ * destroy: destroy this role (used at module remove)
* irq: irq handler for this role
* name: role name string (host/gadget)
*/
struct ci_role_driver {
+ int (*init)(struct ci13xxx *);
int (*start)(struct ci13xxx *);
void (*stop)(struct ci13xxx *);
+ void (*destroy)(struct ci13xxx *);
irqreturn_t (*irq)(struct ci13xxx *);
const char *name;
};
@@ -206,6 +210,17 @@ static inline void ci_role_stop(struct ci13xxx *ci)
ci->roles[role]->stop(ci);
}
+static inline void ci_role_destroy(struct ci13xxx *ci)
+{
+ enum ci_role role = ci->role;
+
+ if (role == CI_ROLE_END)
+ return;
+
+ ci->role = CI_ROLE_END;
+
+ ci->roles[role]->destroy(ci);
+}
/******************************************************************************
* REGISTERS
*****************************************************************************/
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index f8f8484..a5adf1a 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -315,27 +315,16 @@ static void ci_handle_id_switch(struct ci13xxx *ci)
ci_role(ci)->name, ci->roles[role]->name);
/* 1. Finish the current role */
- if (ci->role == CI_ROLE_GADGET) {
- usb_gadget_vbus_disconnect(&ci->gadget);
- /* host doesn't care B_SESSION_VALID event */
- ci_clear_otg_interrupt(ci, OTGSC_BSVIS);
- ci_disable_otg_interrupt(ci, OTGSC_BSVIE);
- ci->role = CI_ROLE_END;
- /* reset controller */
- hw_device_reset(ci, USBMODE_CM_IDLE);
- } else if (ci->role == CI_ROLE_HOST) {
- ci_role_stop(ci);
- /* reset controller */
- hw_device_reset(ci, USBMODE_CM_IDLE);
- }
+ ci_role_stop(ci);
+ hw_device_reset(ci, USBMODE_CM_IDLE);
/* 2. Turn on/off vbus according to coming role */
- if (ci_otg_role(ci) == CI_ROLE_GADGET) {
+ if (role == CI_ROLE_GADGET) {
otg_set_vbus(&ci->otg, false);
/* wait vbus lower than OTGSC_BSV */
hw_wait_reg(ci, OP_OTGSC, OTGSC_BSV, 0,
CI_VBUS_STABLE_TIMEOUT);
- } else if (ci_otg_role(ci) == CI_ROLE_HOST) {
+ } else if (role == CI_ROLE_HOST) {
otg_set_vbus(&ci->otg, true);
/* wait vbus higher than OTGSC_AVV */
hw_wait_reg(ci, OP_OTGSC, OTGSC_AVV, OTGSC_AVV,
@@ -343,13 +332,7 @@ static void ci_handle_id_switch(struct ci13xxx *ci)
}
/* 3. Begin the new role */
- if (ci_otg_role(ci) == CI_ROLE_GADGET) {
- ci->role = role;
- ci_clear_otg_interrupt(ci, OTGSC_BSVIS);
- ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
- } else if (ci_otg_role(ci) == CI_ROLE_HOST) {
- ci_role_start(ci, role);
- }
+ ci_role_start(ci, role);
}
}
@@ -585,7 +568,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
ret = ci_hdrc_gadget_init(ci);
if (ret)
- dev_info(dev, "doesn't support gadget\n");
+ dev_info(dev, "doesn't support gadget, ret=%d\n", ret);
if (!ci->roles[CI_ROLE_HOST] && !ci->roles[CI_ROLE_GADGET]) {
dev_err(dev, "no supported roles\n");
@@ -607,22 +590,30 @@ static int ci_hdrc_probe(struct platform_device *pdev)
: CI_ROLE_GADGET;
}
- ret = ci_role_start(ci, ci->role);
- if (ret) {
- dev_err(dev, "can't start %s role\n", ci_role(ci)->name);
- ret = -ENODEV;
- goto rm_wq;
- }
-
otgsc = hw_read(ci, OP_OTGSC, ~0);
+
/*
- * if it is device mode:
- * - Enable vbus detect
- * - If it has already connected to host, notify udc
+ * If the gadget is supported, call its init unconditionally,
+ * We need to support load gadget module at init.rc.
*/
- if (ci->role == CI_ROLE_GADGET) {
- ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
- ci_handle_vbus_change(ci);
+ if (ci->roles[CI_ROLE_GADGET]) {
+ ret = ci->roles[CI_ROLE_GADGET]->init(ci);
+ if (ret) {
+ dev_err(dev, "can't init %s role, ret=%d\n",
+ ci_role(ci)->name, ret);
+ ret = -ENODEV;
+ goto rm_wq;
+ }
+ }
+
+ if (ci->role == CI_ROLE_HOST) {
+ ret = ci->roles[CI_ROLE_HOST]->init(ci);
+ if (ret) {
+ dev_err(dev, "can't init %s role, ret=%d\n",
+ ci_role(ci)->name, ret);
+ ret = -ENODEV;
+ goto rm_wq;
+ }
}
platform_set_drvdata(pdev, ci);
@@ -660,7 +651,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 caecad9..6024a4f 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -92,8 +92,10 @@ int ci_hdrc_host_init(struct ci13xxx *ci)
if (!rdrv)
return -ENOMEM;
+ rdrv->init = host_start;
rdrv->start = host_start;
rdrv->stop = host_stop;
+ rdrv->destroy = host_stop;
rdrv->irq = host_irq;
rdrv->name = "host";
ci->roles[CI_ROLE_HOST] = rdrv;
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 83e54bb..e6a4ec0 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"
@@ -1754,6 +1755,16 @@ static int udc_start(struct ci13xxx *ci)
pm_runtime_no_callbacks(&ci->gadget.dev);
pm_runtime_enable(&ci->gadget.dev);
+ /*
+ * if it is device mode:
+ * - Enable vbus detect
+ * - 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);
+ }
+
return retval;
remove_trans:
@@ -1780,6 +1791,22 @@ 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)
+{
+ usb_gadget_vbus_disconnect(&ci->gadget);
+ /* 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
*
@@ -1825,8 +1852,10 @@ int ci_hdrc_gadget_init(struct ci13xxx *ci)
if (!rdrv)
return -ENOMEM;
- rdrv->start = udc_start;
- rdrv->stop = udc_stop;
+ rdrv->init = udc_start;
+ rdrv->start = udc_id_switch_for_device;
+ rdrv->stop = udc_id_switch_for_host;
+ rdrv->destroy = udc_stop;
rdrv->irq = udc_irq;
rdrv->name = "gadget";
ci->roles[CI_ROLE_GADGET] = rdrv;
--
1.7.0.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RESEND PATCH v5 4/7] usb: chipidea: consolidate ci_role_driver's API for both roles
2013-01-21 1:56 ` [RESEND PATCH v5 4/7] usb: chipidea: consolidate ci_role_driver's API for both roles Peter Chen
@ 2013-01-24 14:35 ` Alexander Shishkin
2013-01-25 3:30 ` Peter Chen
0 siblings, 1 reply; 24+ messages in thread
From: Alexander Shishkin @ 2013-01-24 14:35 UTC (permalink / raw)
To: linux-arm-kernel
Peter Chen <peter.chen@freescale.com> writes:
> - Create init/destroy API for probe and remove
> - start/stop API are only used otg id switch process
> - Create the gadget at ci_hdrc_probe if the gadget is supported
> at that port, the main purpose for this is to avoid gadget module
> load fail at init.rc
I don't think it's necessary to mention android-specific init scripts
here in our patches.
> Signed-off-by: Peter Chen <peter.chen@freescale.com>
> ---
> drivers/usb/chipidea/ci.h | 19 +++++++++++-
> drivers/usb/chipidea/core.c | 65 ++++++++++++++++++------------------------
> drivers/usb/chipidea/host.c | 2 +
> drivers/usb/chipidea/udc.c | 33 ++++++++++++++++++++-
> 4 files changed, 78 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> index 325d790..00939e6 100644
> --- a/drivers/usb/chipidea/ci.h
> +++ b/drivers/usb/chipidea/ci.h
> @@ -67,14 +67,18 @@ enum ci_role {
>
> /**
> * struct ci_role_driver - host/gadget role driver
> - * start: start this role
> - * stop: stop this role
> + * init: init this role (used at module probe)
> + * start: start this role (used at id switch)
> + * stop: stop this role (used at id switch)
> + * destroy: destroy this role (used at module remove)
> * irq: irq handler for this role
> * name: role name string (host/gadget)
> */
> struct ci_role_driver {
> + int (*init)(struct ci13xxx *);
> int (*start)(struct ci13xxx *);
> void (*stop)(struct ci13xxx *);
> + void (*destroy)(struct ci13xxx *);
> irqreturn_t (*irq)(struct ci13xxx *);
> const char *name;
> };
> @@ -206,6 +210,17 @@ static inline void ci_role_stop(struct ci13xxx *ci)
> ci->roles[role]->stop(ci);
> }
>
> +static inline void ci_role_destroy(struct ci13xxx *ci)
> +{
> + enum ci_role role = ci->role;
> +
> + if (role == CI_ROLE_END)
> + return;
> +
> + ci->role = CI_ROLE_END;
> +
> + ci->roles[role]->destroy(ci);
> +}
What does this do? It should take role an a parameter, at least. Or it
can be called ci_roles_destroy() and, well, destroy all the roles. Now
we're in a situation where we destroy one.
The idea is that, with this api, we can (and should) have both roles
allocated all the time, but only one role start()'ed.
What we can do here is move the udc's .init() code to
ci_hdrc_gadget_init() and add a complementary ci_hdrc_gadget_destroy(),
which we call in ci_hdrc_remove() and probe's error path. And we can
probably do something similar for the host, or just leave it as it is
now. Seems simpler to me.
> /******************************************************************************
> * REGISTERS
> *****************************************************************************/
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index f8f8484..a5adf1a 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -315,27 +315,16 @@ static void ci_handle_id_switch(struct ci13xxx *ci)
> ci_role(ci)->name, ci->roles[role]->name);
>
> /* 1. Finish the current role */
> - if (ci->role == CI_ROLE_GADGET) {
> - usb_gadget_vbus_disconnect(&ci->gadget);
> - /* host doesn't care B_SESSION_VALID event */
> - ci_clear_otg_interrupt(ci, OTGSC_BSVIS);
> - ci_disable_otg_interrupt(ci, OTGSC_BSVIE);
> - ci->role = CI_ROLE_END;
> - /* reset controller */
> - hw_device_reset(ci, USBMODE_CM_IDLE);
> - } else if (ci->role == CI_ROLE_HOST) {
> - ci_role_stop(ci);
> - /* reset controller */
> - hw_device_reset(ci, USBMODE_CM_IDLE);
> - }
> + ci_role_stop(ci);
> + hw_device_reset(ci, USBMODE_CM_IDLE);
>
> /* 2. Turn on/off vbus according to coming role */
> - if (ci_otg_role(ci) == CI_ROLE_GADGET) {
> + if (role == CI_ROLE_GADGET) {
> otg_set_vbus(&ci->otg, false);
> /* wait vbus lower than OTGSC_BSV */
> hw_wait_reg(ci, OP_OTGSC, OTGSC_BSV, 0,
> CI_VBUS_STABLE_TIMEOUT);
> - } else if (ci_otg_role(ci) == CI_ROLE_HOST) {
> + } else if (role == CI_ROLE_HOST) {
> otg_set_vbus(&ci->otg, true);
> /* wait vbus higher than OTGSC_AVV */
> hw_wait_reg(ci, OP_OTGSC, OTGSC_AVV, OTGSC_AVV,
> @@ -343,13 +332,7 @@ static void ci_handle_id_switch(struct ci13xxx *ci)
> }
>
> /* 3. Begin the new role */
> - if (ci_otg_role(ci) == CI_ROLE_GADGET) {
> - ci->role = role;
> - ci_clear_otg_interrupt(ci, OTGSC_BSVIS);
> - ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
> - } else if (ci_otg_role(ci) == CI_ROLE_HOST) {
> - ci_role_start(ci, role);
> - }
> + ci_role_start(ci, role);
> }
> }
>
> @@ -585,7 +568,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>
> ret = ci_hdrc_gadget_init(ci);
> if (ret)
> - dev_info(dev, "doesn't support gadget\n");
> + dev_info(dev, "doesn't support gadget, ret=%d\n", ret);
>
> if (!ci->roles[CI_ROLE_HOST] && !ci->roles[CI_ROLE_GADGET]) {
> dev_err(dev, "no supported roles\n");
> @@ -607,22 +590,30 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> : CI_ROLE_GADGET;
> }
>
> - ret = ci_role_start(ci, ci->role);
> - if (ret) {
> - dev_err(dev, "can't start %s role\n", ci_role(ci)->name);
> - ret = -ENODEV;
> - goto rm_wq;
> - }
> -
> otgsc = hw_read(ci, OP_OTGSC, ~0);
> +
> /*
> - * if it is device mode:
> - * - Enable vbus detect
> - * - If it has already connected to host, notify udc
> + * If the gadget is supported, call its init unconditionally,
> + * We need to support load gadget module at init.rc.
> */
> - if (ci->role == CI_ROLE_GADGET) {
> - ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
> - ci_handle_vbus_change(ci);
> + if (ci->roles[CI_ROLE_GADGET]) {
> + ret = ci->roles[CI_ROLE_GADGET]->init(ci);
> + if (ret) {
> + dev_err(dev, "can't init %s role, ret=%d\n",
> + ci_role(ci)->name, ret);
> + ret = -ENODEV;
> + goto rm_wq;
> + }
> + }
> +
> + if (ci->role == CI_ROLE_HOST) {
> + ret = ci->roles[CI_ROLE_HOST]->init(ci);
> + if (ret) {
> + dev_err(dev, "can't init %s role, ret=%d\n",
> + ci_role(ci)->name, ret);
> + ret = -ENODEV;
> + goto rm_wq;
> + }
> }
>
> platform_set_drvdata(pdev, ci);
> @@ -660,7 +651,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 caecad9..6024a4f 100644
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -92,8 +92,10 @@ int ci_hdrc_host_init(struct ci13xxx *ci)
> if (!rdrv)
> return -ENOMEM;
>
> + rdrv->init = host_start;
> rdrv->start = host_start;
> rdrv->stop = host_stop;
> + rdrv->destroy = host_stop;
Will this allocate the hcd twice if we start in host role? Looks like that.
> rdrv->irq = host_irq;
> rdrv->name = "host";
> ci->roles[CI_ROLE_HOST] = rdrv;
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index 83e54bb..e6a4ec0 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"
>
> @@ -1754,6 +1755,16 @@ static int udc_start(struct ci13xxx *ci)
> pm_runtime_no_callbacks(&ci->gadget.dev);
> pm_runtime_enable(&ci->gadget.dev);
>
> + /*
> + * if it is device mode:
> + * - Enable vbus detect
> + * - 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);
> + }
> +
> return retval;
>
> remove_trans:
> @@ -1780,6 +1791,22 @@ 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)
> +{
> + usb_gadget_vbus_disconnect(&ci->gadget);
> + /* 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
> *
> @@ -1825,8 +1852,10 @@ int ci_hdrc_gadget_init(struct ci13xxx *ci)
> if (!rdrv)
> return -ENOMEM;
>
> - rdrv->start = udc_start;
> - rdrv->stop = udc_stop;
> + rdrv->init = udc_start;
> + rdrv->start = udc_id_switch_for_device;
> + rdrv->stop = udc_id_switch_for_host;
> + rdrv->destroy = udc_stop;
> rdrv->irq = udc_irq;
> rdrv->name = "gadget";
> ci->roles[CI_ROLE_GADGET] = rdrv;
> --
> 1.7.0.4
^ permalink raw reply [flat|nested] 24+ messages in thread* [RESEND PATCH v5 4/7] usb: chipidea: consolidate ci_role_driver's API for both roles
2013-01-24 14:35 ` Alexander Shishkin
@ 2013-01-25 3:30 ` Peter Chen
2013-01-25 12:12 ` Alexander Shishkin
0 siblings, 1 reply; 24+ messages in thread
From: Peter Chen @ 2013-01-25 3:30 UTC (permalink / raw)
To: linux-arm-kernel
On Thu, Jan 24, 2013 at 04:35:30PM +0200, Alexander Shishkin wrote:
> Peter Chen <peter.chen@freescale.com> writes:
>
> > - Create init/destroy API for probe and remove
> > - start/stop API are only used otg id switch process
> > - Create the gadget at ci_hdrc_probe if the gadget is supported
> > at that port, the main purpose for this is to avoid gadget module
> > load fail at init.rc
>
> I don't think it's necessary to mention android-specific init scripts
> here in our patches.
Ok, will just mention init script.
>
> >
> > +static inline void ci_role_destroy(struct ci13xxx *ci)
> > +{
> > + enum ci_role role = ci->role;
> > +
> > + if (role == CI_ROLE_END)
> > + return;
> > +
> > + ci->role = CI_ROLE_END;
> > +
> > + ci->roles[role]->destroy(ci);
> > +}
>
> What does this do? It should take role an a parameter, at least. Or it
> can be called ci_roles_destroy() and, well, destroy all the roles. Now
> we're in a situation where we destroy one.
Yes, this function has some problems, I suggest just call two lines at
ci_role_destory, do you think so?
ci->roles[role]->destroy(ci);
ci->role = CI_ROLE_END;
>
> The idea is that, with this api, we can (and should) have both roles
> allocated all the time, but only one role start()'ed.
>
> What we can do here is move the udc's .init() code to
> ci_hdrc_gadget_init() and add a complementary ci_hdrc_gadget_destroy(),
> which we call in ci_hdrc_remove() and probe's error path. And we can
> probably do something similar for the host, or just leave it as it is
> now. Seems simpler to me.
Yes, current code has bug that it should call ci_role_destroy at probe's
error path.
For your comments, it still needs to add destory function for udc which will
be called at core.c. I still prefer current way due to below reasons:
- It can keep ci_hdrc_gadget_init() clean
- .init and .remove are different logical function with .start and .stop.
The .init and .remove are used for create and destroy, .start and .stop
are used at id role switch.
> > }
> > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> > index caecad9..6024a4f 100644
> > --- a/drivers/usb/chipidea/host.c
> > +++ b/drivers/usb/chipidea/host.c
> > @@ -92,8 +92,10 @@ int ci_hdrc_host_init(struct ci13xxx *ci)
> > if (!rdrv)
> > return -ENOMEM;
> >
> > + rdrv->init = host_start;
> > rdrv->start = host_start;
> > rdrv->stop = host_stop;
> > + rdrv->destroy = host_stop;
>
> Will this allocate the hcd twice if we start in host role? Looks like that.
No, if we start host role, the host will be allocated once at probe.
host_start is only called when the id value from 1 to 0.
>
--
Best Regards,
Peter Chen
^ permalink raw reply [flat|nested] 24+ messages in thread* [RESEND PATCH v5 4/7] usb: chipidea: consolidate ci_role_driver's API for both roles
2013-01-25 3:30 ` Peter Chen
@ 2013-01-25 12:12 ` Alexander Shishkin
2013-01-28 5:21 ` Peter Chen
0 siblings, 1 reply; 24+ messages in thread
From: Alexander Shishkin @ 2013-01-25 12:12 UTC (permalink / raw)
To: linux-arm-kernel
Peter Chen <peter.chen@freescale.com> writes:
> On Thu, Jan 24, 2013 at 04:35:30PM +0200, Alexander Shishkin wrote:
>> Peter Chen <peter.chen@freescale.com> writes:
>>
>> > - Create init/destroy API for probe and remove
>> > - start/stop API are only used otg id switch process
>> > - Create the gadget at ci_hdrc_probe if the gadget is supported
>> > at that port, the main purpose for this is to avoid gadget module
>> > load fail at init.rc
>>
>> I don't think it's necessary to mention android-specific init scripts
>> here in our patches.
>
> Ok, will just mention init script.
>>
>> >
>> > +static inline void ci_role_destroy(struct ci13xxx *ci)
>> > +{
>> > + enum ci_role role = ci->role;
>> > +
>> > + if (role == CI_ROLE_END)
>> > + return;
>> > +
>> > + ci->role = CI_ROLE_END;
>> > +
>> > + ci->roles[role]->destroy(ci);
>> > +}
>>
>> What does this do? It should take role an a parameter, at least. Or it
>> can be called ci_roles_destroy() and, well, destroy all the roles. Now
>> we're in a situation where we destroy one.
> Yes, this function has some problems, I suggest just call two lines at
> ci_role_destory, do you think so?
>
> ci->roles[role]->destroy(ci);
> ci->role = CI_ROLE_END;
I just don't see why it's needed at all. See below.
>>
>> The idea is that, with this api, we can (and should) have both roles
>> allocated all the time, but only one role start()'ed.
>>
>> What we can do here is move the udc's .init() code to
>> ci_hdrc_gadget_init() and add a complementary ci_hdrc_gadget_destroy(),
>> which we call in ci_hdrc_remove() and probe's error path. And we can
>> probably do something similar for the host, or just leave it as it is
>> now. Seems simpler to me.
> Yes, current code has bug that it should call ci_role_destroy at probe's
> error path.
> For your comments, it still needs to add destory function for udc which will
> be called at core.c. I still prefer current way due to below reasons:
>
> - It can keep ci_hdrc_gadget_init() clean
> - .init and .remove are different logical function with .start and .stop.
> The .init and .remove are used for create and destroy, .start and .stop
> are used at id role switch.
True, and we can keep it that way: (again, untested)
---cut---
diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index 342b430..36f495a 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -67,18 +67,14 @@ enum ci_role {
/**
* struct ci_role_driver - host/gadget role driver
- * init: init this role (used at module probe)
* start: start this role (used at id switch)
* stop: stop this role (used at id switch)
- * destroy: destroy this role (used at module remove)
* irq: irq handler for this role
* name: role name string (host/gadget)
*/
struct ci_role_driver {
- int (*init)(struct ci13xxx *);
int (*start)(struct ci13xxx *);
void (*stop)(struct ci13xxx *);
- void (*destroy)(struct ci13xxx *);
irqreturn_t (*irq)(struct ci13xxx *);
const char *name;
};
@@ -212,17 +208,6 @@ static inline void ci_role_stop(struct ci13xxx *ci)
ci->roles[role]->stop(ci);
}
-static inline void ci_role_destroy(struct ci13xxx *ci)
-{
- enum ci_role role = ci->role;
-
- if (role == CI_ROLE_END)
- return;
-
- ci->role = CI_ROLE_END;
-
- ci->roles[role]->destroy(ci);
-}
/******************************************************************************
* REGISTERS
*****************************************************************************/
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index a61e759..83f35fa 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -402,6 +402,12 @@ static ssize_t store_role(struct device *dev, struct device_attribute *attr,
if (ret)
return ret;
+ /*
+ * there won't be an interrupt in case of manual switching,
+ * so we need to check vbus session manually
+ */
+ ci_handle_vbus_change(ci);
+
return count;
}
@@ -592,30 +598,6 @@ static int ci_hdrc_probe(struct platform_device *pdev)
otgsc = hw_read(ci, OP_OTGSC, ~0);
- /*
- * If the gadget is supported, call its init unconditionally,
- * We need to support load gadget module at init.rc.
- */
- if (ci->roles[CI_ROLE_GADGET]) {
- ret = ci->roles[CI_ROLE_GADGET]->init(ci);
- if (ret) {
- dev_err(dev, "can't init %s role, ret=%d\n",
- ci_role(ci)->name, ret);
- ret = -ENODEV;
- goto rm_wq;
- }
- }
-
- if (ci->role == CI_ROLE_HOST) {
- ret = ci->roles[CI_ROLE_HOST]->init(ci);
- if (ret) {
- dev_err(dev, "can't init %s role, ret=%d\n",
- ci_role(ci)->name, ret);
- ret = -ENODEV;
- goto rm_wq;
- }
- }
-
platform_set_drvdata(pdev, ci);
ret = request_irq(ci->irq, ci_irq, IRQF_SHARED, ci->platdata->name,
ci);
@@ -626,6 +608,8 @@ static int ci_hdrc_probe(struct platform_device *pdev)
if (ret)
goto rm_attr;
+ ci_role_start(ci, ci->role);
+
/* Handle the situation that usb device at the MicroB to A cable */
if (ci->is_otg && !(otgsc & OTGSC_ID))
queue_delayed_work(ci->wq, &ci->dwork, msecs_to_jiffies(500));
@@ -651,7 +635,8 @@ 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_destroy(ci);
+ ci_hdrc_gadget_destroy(ci);
+ ci_hdrc_host_destroy(ci);
return 0;
}
diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index 7f4538c..ead3158 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -95,10 +95,8 @@ int ci_hdrc_host_init(struct ci13xxx *ci)
if (!rdrv)
return -ENOMEM;
- rdrv->init = host_start;
rdrv->start = host_start;
rdrv->stop = host_stop;
- rdrv->destroy = host_stop;
rdrv->irq = host_irq;
rdrv->name = "host";
ci->roles[CI_ROLE_HOST] = rdrv;
@@ -107,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..2e529be 100644
--- a/drivers/usb/chipidea/host.h
+++ b/drivers/usb/chipidea/host.h
@@ -4,6 +4,7 @@
#ifdef CONFIG_USB_CHIPIDEA_HOST
int ci_hdrc_host_init(struct ci13xxx *ci);
+void ci_hdrc_host_destroy(struct ci13xxx *ci);
#else
@@ -12,6 +13,10 @@ static inline int ci_hdrc_host_init(struct ci13xxx *ci)
return -ENXIO;
}
+static void ci_hdrc_host_destroy(struct ci13xxx *ci)
+{
+}
+
#endif
#endif /* __DRIVERS_USB_CHIPIDEA_HOST_H */
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 38446de..38b06ac 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -1750,10 +1750,10 @@ static int udc_start(struct ci13xxx *ci)
* - Enable vbus detect
* - If it has already connected to host, notify udc
*/
- if (ci->role == CI_ROLE_GADGET) {
+ //if (ci->role == CI_ROLE_GADGET) {
ci_enable_otg_interrupt(ci, OTGSC_BSVIE);
ci_handle_vbus_change(ci);
- }
+ //}
return retval;
@@ -1798,11 +1798,10 @@ static void udc_id_switch_for_host(struct ci13xxx *ci)
}
/**
- * udc_remove: parent remove must call this to remove UDC
- *
- * No interrupts active, the IRQ has been released
+ * ci_hdrc_gadget_init - initialize device related bits
+ * ci: the controller
*/
-static void udc_stop(struct ci13xxx *ci)
+void ci_hdrc_gadget_destroy(struct ci13xxx *ci)
{
if (ci == NULL)
return;
@@ -1821,8 +1820,6 @@ 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));
}
/**
@@ -1842,13 +1839,11 @@ int ci_hdrc_gadget_init(struct ci13xxx *ci)
if (!rdrv)
return -ENOMEM;
- rdrv->init = udc_start;
rdrv->start = udc_id_switch_for_device;
rdrv->stop = udc_id_switch_for_host;
- rdrv->destroy = udc_stop;
rdrv->irq = udc_irq;
rdrv->name = "gadget";
ci->roles[CI_ROLE_GADGET] = rdrv;
- return 0;
+ return udc_start(ci);
}
diff --git a/drivers/usb/chipidea/udc.h b/drivers/usb/chipidea/udc.h
index 4ff2384d..12fd7cf 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
@@ -88,6 +89,10 @@ static inline int ci_hdrc_gadget_init(struct ci13xxx *ci)
return -ENXIO;
}
+static void ci_hdrc_gadget_destroy(struct ci13xxx *ci)
+{
+}
+
#endif
#endif /* __DRIVERS_USB_CHIPIDEA_UDC_H */
---cut---
Which is shorter (32 insertions(+), 53 deletions(-)) and makes the main
probe easier on the eyes. What do you think?
Regards,
--
Alex
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RESEND PATCH v5 4/7] usb: chipidea: consolidate ci_role_driver's API for both roles
2013-01-25 12:12 ` Alexander Shishkin
@ 2013-01-28 5:21 ` Peter Chen
2013-01-29 9:37 ` Alexander Shishkin
0 siblings, 1 reply; 24+ messages in thread
From: Peter Chen @ 2013-01-28 5:21 UTC (permalink / raw)
To: linux-arm-kernel
On Fri, Jan 25, 2013 at 02:12:20PM +0200, Alexander Shishkin wrote:
> Peter Chen <peter.chen@freescale.com> writes:
>
> > On Thu, Jan 24, 2013 at 04:35:30PM +0200, Alexander Shishkin wrote:
> >> Peter Chen <peter.chen@freescale.com> writes:
> >>
> >> > - Create init/destroy API for probe and remove
> >> > - start/stop API are only used otg id switch process
> >> > - Create the gadget at ci_hdrc_probe if the gadget is supported
> >> > at that port, the main purpose for this is to avoid gadget module
> >> > load fail at init.rc
> >>
> @@ -402,6 +402,12 @@ static ssize_t store_role(struct device *dev, struct device_attribute *attr,
> if (ret)
> return ret;
>
> + /*
> + * there won't be an interrupt in case of manual switching,
> + * so we need to check vbus session manually
> + */
> + ci_handle_vbus_change(ci);
> +
It may not be used as there will be a vbus interrupt.
> return count;
> }
>
> }
> dbg_remove_files(&ci->gadget.dev);
> device_unregister(&ci->gadget.dev);
> - /* my kobject is dynamic, I swear! */
> - memset(&ci->gadget, 0, sizeof(ci->gadget));
Any reason to delete it, another fix?
> }
>
> /**
> @@ -1842,13 +1839,11 @@ int ci_hdrc_gadget_init(struct ci13xxx *ci)
> if (!rdrv)
> return -ENOMEM;
>
> - rdrv->init = udc_start;
> rdrv->start = udc_id_switch_for_device;
> rdrv->stop = udc_id_switch_for_host;
> - rdrv->destroy = udc_stop;
Where we call udc_start and udc_stop? And the udc_start should only be called
one time.
>
> Which is shorter (32 insertions(+), 53 deletions(-)) and makes the main
> probe easier on the eyes. What do you think?
OK, not matter how to change it, it needs to cover below things:
(Covers last email)
- Gadget needs to be only initialized one time.
- Host/device function should be OK when the device on it or
the usb cable connects to host during the boots up.
- When the OTG port is at host mode, it should not call any
register writing operations at gadget's API.
--
Best Regards,
Peter Chen
^ permalink raw reply [flat|nested] 24+ messages in thread
* [RESEND PATCH v5 4/7] usb: chipidea: consolidate ci_role_driver's API for both roles
2013-01-28 5:21 ` Peter Chen
@ 2013-01-29 9:37 ` Alexander Shishkin
2013-01-30 6:52 ` Peter Chen
0 siblings, 1 reply; 24+ messages in thread
From: Alexander Shishkin @ 2013-01-29 9:37 UTC (permalink / raw)
To: linux-arm-kernel
Peter Chen <peter.chen@freescale.com> writes:
> On Fri, Jan 25, 2013 at 02:12:20PM +0200, Alexander Shishkin wrote:
>> Peter Chen <peter.chen@freescale.com> writes:
>>
>> > On Thu, Jan 24, 2013 at 04:35:30PM +0200, Alexander Shishkin wrote:
>> >> Peter Chen <peter.chen@freescale.com> writes:
>> >>
>> >> > - Create init/destroy API for probe and remove
>> >> > - start/stop API are only used otg id switch process
>> >> > - Create the gadget at ci_hdrc_probe if the gadget is supported
>> >> > at that port, the main purpose for this is to avoid gadget module
>> >> > load fail at init.rc
>> >>
>> @@ -402,6 +402,12 @@ static ssize_t store_role(struct device *dev, struct device_attribute *attr,
>> if (ret)
>> return ret;
>>
>> + /*
>> + * there won't be an interrupt in case of manual switching,
>> + * so we need to check vbus session manually
>> + */
>> + ci_handle_vbus_change(ci);
>> +
> It may not be used as there will be a vbus interrupt.
Not if you write gadget to "role" file.
>> return count;
>> }
>>
>> }
>> dbg_remove_files(&ci->gadget.dev);
>> device_unregister(&ci->gadget.dev);
>> - /* my kobject is dynamic, I swear! */
>> - memset(&ci->gadget, 0, sizeof(ci->gadget));
> Any reason to delete it, another fix?
It is currently like this because start and stop functions are called
multiple times during the devices lifetime. This patch converts the
driver to only call start at driver's probe() and stop at driver's
remove().
>> }
>>
>> /**
>> @@ -1842,13 +1839,11 @@ int ci_hdrc_gadget_init(struct ci13xxx *ci)
>> if (!rdrv)
>> return -ENOMEM;
>>
>> - rdrv->init = udc_start;
>> rdrv->start = udc_id_switch_for_device;
>> rdrv->stop = udc_id_switch_for_host;
>> - rdrv->destroy = udc_stop;
> Where we call udc_start and udc_stop? And the udc_start should only be called
> one time.
ci_hdrc_gadget_init() and ci_hdrc_gadget_destroy(). Look closer at the
patch.
>>
>> Which is shorter (32 insertions(+), 53 deletions(-)) and makes the main
>> probe easier on the eyes. What do you think?
> OK, not matter how to change it, it needs to cover below things:
> (Covers last email)
> - Gadget needs to be only initialized one time.
Yes, that's what we agreed on and that's what I'm suggesting in the
above patch.
> - Host/device function should be OK when the device on it or
> the usb cable connects to host during the boots up.
Should still work.
> - When the OTG port is at host mode, it should not call any
> register writing operations at gadget's API.
Furthermore, there shouldn't be any calls to the gadget api.
Regards,
--
Alex
^ permalink raw reply [flat|nested] 24+ messages in thread
* [RESEND PATCH v5 4/7] usb: chipidea: consolidate ci_role_driver's API for both roles
2013-01-29 9:37 ` Alexander Shishkin
@ 2013-01-30 6:52 ` Peter Chen
0 siblings, 0 replies; 24+ messages in thread
From: Peter Chen @ 2013-01-30 6:52 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jan 29, 2013 at 11:37:22AM +0200, Alexander Shishkin wrote:
> Peter Chen <peter.chen@freescale.com> writes:
>
> > On Fri, Jan 25, 2013 at 02:12:20PM +0200, Alexander Shishkin wrote:
> >> Peter Chen <peter.chen@freescale.com> writes:
> >>
> >> > On Thu, Jan 24, 2013 at 04:35:30PM +0200, Alexander Shishkin wrote:
> >> >> Peter Chen <peter.chen@freescale.com> writes:
> >> >>
> >> >> > - Create init/destroy API for probe and remove
> >> >> > - start/stop API are only used otg id switch process
> >> >> > - Create the gadget at ci_hdrc_probe if the gadget is supported
> >> >> > at that port, the main purpose for this is to avoid gadget module
> >> >> > load fail at init.rc
> >> >>
> >> @@ -402,6 +402,12 @@ static ssize_t store_role(struct device *dev, struct device_attribute *attr,
> >> if (ret)
> >> return ret;
> >>
> >> + /*
> >> + * there won't be an interrupt in case of manual switching,
> >> + * so we need to check vbus session manually
> >> + */
> >> + ci_handle_vbus_change(ci);
> >> +
> > It may not be used as there will be a vbus interrupt.
>
> Not if you write gadget to "role" file.
Let me see, we will only use it for standard-A receptacle port soldered
at OTG port, there is no ID manual switch, and ID pin is grounded.
When the user wants to use it at gadget mode, eg, update image.
1. Plug cable, then write "gadget" to "role" file
It will work as there will be ci_handle_vbus_change(ci) at role_start,
just like we insmod gadget mode when the cable is connecting to host.
2. Write "gadget" to "role" file first, then plug cable
After stopping host, it should close vbus.
After gadget role starts, it will enable vbus interrupt.
Then, when we plug in A-to-A cable, there will be a vbus interrupt, then
active gadget.
> >> return -ENOMEM;
> >>
> >> - rdrv->init = udc_start;
> >> rdrv->start = udc_id_switch_for_device;
> >> rdrv->stop = udc_id_switch_for_host;
> >> - rdrv->destroy = udc_stop;
> > Where we call udc_start and udc_stop? And the udc_start should only be called
> > one time.
>
> ci_hdrc_gadget_init() and ci_hdrc_gadget_destroy(). Look closer at the
> patch.
I am sorry, I still not see udc_stop at ci_hdrc_gadget_destroy().
It doesn't matter. I know what you want to do, let me summery
- Call udc_start at ci_hdrc_gadget_init, but vbus enable function should
only be called if the role is gadget, or vbus interrupt will occur
when the host switches gadget (vbus is off), besides, the ci->vbus_active
will be set at host mode, and ci13xxx_start will operate register
with function hw_device_state(ci, ci->ep0out->qh.dma) if we load module
at host mode. Besides, if we set REG_SHARED, it will reset controller.
- Create two destroy functions for gadget and host, and call them at ci
remove.
Anything I forget, or do you think anything else I need to change?
If you agree, I will send patch for above change.
Do you want this change is on the top of my v5 patch
or just a new v6 patch with above change?
> > - When the OTG port is at host mode, it should not call any
> > register writing operations at gadget's API.
>
> Furthermore, there shouldn't be any calls to the gadget api.
The user may load gadget module when it is at host mode, eg, at
their init script.
--
Best Regards,
Peter Chen
^ permalink raw reply [flat|nested] 24+ messages in thread
* [RESEND PATCH v5 5/7] usb: chipidea: udc: add pullup/pulldown dp at hw_device_state
2013-01-21 1:56 [RESEND PATCH v5 0/7] Add fully tested id switch and vbus connect detect support for Chipidea Peter Chen
` (3 preceding siblings ...)
2013-01-21 1:56 ` [RESEND PATCH v5 4/7] usb: chipidea: consolidate ci_role_driver's API for both roles Peter Chen
@ 2013-01-21 1:56 ` Peter Chen
2013-01-21 1:56 ` [RESEND PATCH v5 6/7] usb: chipidea: udc: retire the flag CI13_PULLUP_ON_VBUS Peter Chen
2013-01-21 1:56 ` [RESEND PATCH v5 7/7] usb: chipidea: imx: add internal vbus regulator control Peter Chen
6 siblings, 0 replies; 24+ messages in thread
From: Peter Chen @ 2013-01-21 1:56 UTC (permalink / raw)
To: linux-arm-kernel
- 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 e6a4ec0..aab20bb 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 -ENOTSUPP;
+
if (is_on)
- hw_write(ci, OP_USBCMD, USBCMD_RS, USBCMD_RS);
+ hw_device_state(ci, ci->ep0out->qh.dma);
else
- hw_write(ci, OP_USBCMD, USBCMD_RS, 0);
+ hw_device_state(ci, 0);
return 0;
}
--
1.7.0.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RESEND PATCH v5 6/7] usb: chipidea: udc: retire the flag CI13_PULLUP_ON_VBUS
2013-01-21 1:56 [RESEND PATCH v5 0/7] Add fully tested id switch and vbus connect detect support for Chipidea Peter Chen
` (4 preceding siblings ...)
2013-01-21 1:56 ` [RESEND PATCH v5 5/7] usb: chipidea: udc: add pullup/pulldown dp at hw_device_state Peter Chen
@ 2013-01-21 1:56 ` Peter Chen
2013-01-21 1:56 ` [RESEND PATCH v5 7/7] usb: chipidea: imx: add internal vbus regulator control Peter Chen
6 siblings, 0 replies; 24+ messages in thread
From: Peter Chen @ 2013-01-21 1:56 UTC (permalink / raw)
To: linux-arm-kernel
(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 8c29122..3b91ff4 100644
--- a/drivers/usb/chipidea/ci13xxx_imx.c
+++ b/drivers/usb/chipidea/ci13xxx_imx.c
@@ -88,7 +88,6 @@ EXPORT_SYMBOL_GPL(usbmisc_get_init_data);
static struct ci13xxx_platform_data ci13xxx_imx_platdata = {
.name = "ci13xxx_imx",
.flags = CI13XXX_REQUIRE_TRANSCEIVER |
- CI13XXX_PULLUP_ON_VBUS |
CI13XXX_DISABLE_STREAMING,
.capoffset = DEF_CAPOFFSET,
};
diff --git a/drivers/usb/chipidea/ci13xxx_msm.c b/drivers/usb/chipidea/ci13xxx_msm.c
index 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 aab20bb..dcac77c 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 -ENOTSUPP;
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 544825d..37821b3 100644
--- a/include/linux/usb/chipidea.h
+++ b/include/linux/usb/chipidea.h
@@ -17,7 +17,6 @@ struct ci13xxx_platform_data {
unsigned long flags;
#define CI13XXX_REGS_SHARED BIT(0)
#define CI13XXX_REQUIRE_TRANSCEIVER BIT(1)
-#define CI13XXX_PULLUP_ON_VBUS BIT(2)
#define CI13XXX_DISABLE_STREAMING BIT(3)
#define CI13XXX_CONTROLLER_RESET_EVENT 0
--
1.7.0.4
^ permalink raw reply related [flat|nested] 24+ messages in thread* [RESEND PATCH v5 7/7] usb: chipidea: imx: add internal vbus regulator control
2013-01-21 1:56 [RESEND PATCH v5 0/7] Add fully tested id switch and vbus connect detect support for Chipidea Peter Chen
` (5 preceding siblings ...)
2013-01-21 1:56 ` [RESEND PATCH v5 6/7] usb: chipidea: udc: retire the flag CI13_PULLUP_ON_VBUS Peter Chen
@ 2013-01-21 1:56 ` Peter Chen
6 siblings, 0 replies; 24+ messages in thread
From: Peter Chen @ 2013-01-21 1:56 UTC (permalink / raw)
To: linux-arm-kernel
- For host, the vbus should always be on.
- For otg, the vbus is off defaultly, the vbus needs to be
turned on/off when usb role switches.
Signed-off-by: Peter Chen <peter.chen@freescale.com>
---
drivers/usb/chipidea/ci.h | 2 +
drivers/usb/chipidea/ci13xxx_imx.c | 80 ++++++++++++++++++++++++++++--------
2 files changed, 64 insertions(+), 18 deletions(-)
diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index 00939e6..342b430 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -135,6 +135,7 @@ struct hw_bank {
* @hcd: pointer to usb_hcd for ehci host driver
* @otg: for otg support
* @events: events for otg, and handled at ci_role_work
+ * @reg_vbus: used to control internal vbus regulator
*/
struct ci13xxx {
struct device *dev;
@@ -174,6 +175,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/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c
index 3b91ff4..4c9df98 100644
--- a/drivers/usb/chipidea/ci13xxx_imx.c
+++ b/drivers/usb/chipidea/ci13xxx_imx.c
@@ -88,14 +88,47 @@ EXPORT_SYMBOL_GPL(usbmisc_get_init_data);
static struct ci13xxx_platform_data ci13xxx_imx_platdata = {
.name = "ci13xxx_imx",
.flags = CI13XXX_REQUIRE_TRANSCEIVER |
- CI13XXX_DISABLE_STREAMING,
+ CI13XXX_DISABLE_STREAMING |
+ CI13XXX_REGS_SHARED,
.capoffset = DEF_CAPOFFSET,
};
+static int ci13xxx_otg_set_vbus(struct usb_otg *otg, bool enabled)
+{
+ struct ci13xxx *ci = container_of(otg, struct ci13xxx, otg);
+ struct regulator *reg_vbus = ci->reg_vbus;
+ int ret;
+
+ WARN_ON(!reg_vbus);
+
+ if (reg_vbus) {
+ if (enabled) {
+ ret = regulator_enable(reg_vbus);
+ if (ret) {
+ dev_err(ci->dev,
+ "Failed to enable vbus regulator, ret=%d\n",
+ ret);
+ return ret;
+ }
+ } else {
+ ret = regulator_disable(reg_vbus);
+ if (ret) {
+ dev_err(ci->dev,
+ "Failed to disable vbus regulator, ret=%d\n",
+ ret);
+ return ret;
+ }
+ }
+ }
+
+ return 0;
+}
+
static int ci13xxx_imx_probe(struct platform_device *pdev)
{
struct ci13xxx_imx_data *data;
struct platform_device *plat_ci, *phy_pdev;
+ struct ci13xxx *ci;
struct device_node *phy_np;
struct resource *res;
struct regulator *reg_vbus;
@@ -152,20 +185,11 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
}
}
- /* we only support host now, so enable vbus here */
reg_vbus = devm_regulator_get(&pdev->dev, "vbus");
- if (!IS_ERR(reg_vbus)) {
- ret = regulator_enable(reg_vbus);
- if (ret) {
- dev_err(&pdev->dev,
- "Failed to enable vbus regulator, err=%d\n",
- ret);
- goto put_np;
- }
+ if (!IS_ERR(reg_vbus))
data->reg_vbus = reg_vbus;
- } else {
+ else
reg_vbus = NULL;
- }
ci13xxx_imx_platdata.phy = data->phy;
@@ -175,7 +199,7 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
if (!pdev->dev.dma_mask) {
ret = -ENOMEM;
dev_err(&pdev->dev, "Failed to alloc dma_mask!\n");
- goto err;
+ goto put_np;
}
*pdev->dev.dma_mask = DMA_BIT_MASK(32);
dma_set_coherent_mask(&pdev->dev, *pdev->dev.dma_mask);
@@ -186,7 +210,7 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
if (ret) {
dev_err(&pdev->dev,
"usbmisc init failed, ret=%d\n", ret);
- goto err;
+ goto put_np;
}
}
@@ -198,24 +222,44 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
dev_err(&pdev->dev,
"Can't register ci_hdrc platform device, err=%d\n",
ret);
- goto err;
+ goto put_np;
}
data->ci_pdev = plat_ci;
platform_set_drvdata(pdev, data);
+ ci = platform_get_drvdata(plat_ci);
+ /*
+ * Internal vbus on/off policy
+ * - Always on for host only function
+ * - Always off for gadget only function
+ * - call otg.set_vbus to control on/off according usb role
+ */
+
+ if (ci->roles[CI_ROLE_HOST] && !ci->roles[CI_ROLE_GADGET]
+ && reg_vbus) {
+ ret = regulator_enable(reg_vbus);
+ if (ret) {
+ dev_err(&pdev->dev,
+ "Failed to enable vbus regulator, ret=%d\n",
+ ret);
+ goto put_np;
+ }
+ } else if (ci->is_otg) {
+ ci->otg.set_vbus = ci13xxx_otg_set_vbus;
+ ci->reg_vbus = data->reg_vbus;
+ }
+
pm_runtime_no_callbacks(&pdev->dev);
pm_runtime_enable(&pdev->dev);
return 0;
-err:
- if (reg_vbus)
- regulator_disable(reg_vbus);
put_np:
if (phy_np)
of_node_put(phy_np);
clk_disable_unprepare(data->clk);
+
return ret;
}
--
1.7.0.4
^ permalink raw reply related [flat|nested] 24+ messages in thread