[PATCHv0 0/5] usb: chipidea: clean up debuggery
This patchset cleans up the remaining debugging bits in the driver by either conventing them to trace points or moving to debugfs, all the while getting rid of home-grown tracing facilities. Trace events are accessible through perf tool. If you don't yet have it in your testing environment, now is the good time. Alexander Shishkin (5): usb: chipidea: convert events to tracepoints usb: chipidea: replace interrupt accounting with tracepoints usb: chipidea: convert debug entries in sysfs to debugfs usb: chipidea: move role to debugfs usb: chipidea: move debug files creation/removal to the core drivers/usb/chipidea/ci.h | 15 + drivers/usb/chipidea/core.c | 47 +-- drivers/usb/chipidea/debug.c| 867 +-- drivers/usb/chipidea/debug.h| 34 +- drivers/usb/chipidea/udc.c | 66 ++- drivers/usb/chipidea/udc.h |2 - include/trace/events/chipidea.h | 151 +++ 7 files changed, 392 insertions(+), 790 deletions(-) create mode 100644 include/trace/events/chipidea.h -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv0 1/5] usb: chipidea: convert events to tracepoints
As part of the legacy from the original driver design, we retain home-grown
tracing infrastructure, complete with own ring buffer and timestamps. While
it is useful for debugging certain cases, it's a lot of extra code, which
these days is rather redundant.
This patch replaces local tracing functionality with kernel tracepoints,
thus getting rid of the ring buffer and all the maintenance code, while
making use of standard kernel infrastructure.
Signed-off-by: Alexander Shishkin
---
drivers/usb/chipidea/ci.h | 13 +++
drivers/usb/chipidea/core.c |2 +
drivers/usb/chipidea/debug.c| 202 +--
drivers/usb/chipidea/debug.h| 20
drivers/usb/chipidea/udc.c | 46 -
drivers/usb/chipidea/udc.h |2 -
include/trace/events/chipidea.h | 135 ++
7 files changed, 169 insertions(+), 251 deletions(-)
create mode 100644 include/trace/events/chipidea.h
diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index e25d126..f67d101 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -23,6 +23,8 @@
*/
#define CI13XXX_PAGE_SIZE 4096ul /* page size for TD's */
#define ENDPT_MAX 32
+#define RX0 /* similar to USB_DIR_OUT but can be used as an index */
+#define TX1 /* similar to USB_DIR_IN but can be used as an index */
/**
* STRUCTURES
@@ -59,6 +61,15 @@ struct ci13xxx_ep {
struct dma_pool *td_pool;
};
+/**
+ * ci_ep_addr: calculates endpoint address from direction & number
+ * @ep: endpoint
+ */
+static inline u8 ci_ep_addr(struct ci13xxx_ep *ep)
+{
+ return ((ep->dir == TX) ? USB_ENDPOINT_DIR_MASK : 0) | ep->num;
+}
+
enum ci_role {
CI_ROLE_HOST = 0,
CI_ROLE_GADGET,
@@ -313,4 +324,6 @@ int hw_port_test_set(struct ci13xxx *ci, u8 mode);
u8 hw_port_test_get(struct ci13xxx *ci);
+#include
+
#endif /* __DRIVERS_USB_CHIPIDEA_CI_H */
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index f69d029..ef965ad 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -68,6 +68,8 @@
#include
#include
+#define CREATE_TRACE_POINTS
+
#include "ci.h"
#include "udc.h"
#include "bits.h"
diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c
index 3bc244d..8937f83 100644
--- a/drivers/usb/chipidea/debug.c
+++ b/drivers/usb/chipidea/debug.c
@@ -207,200 +207,6 @@ static ssize_t show_driver(struct device *dev, struct
device_attribute *attr,
}
static DEVICE_ATTR(driver, S_IRUSR, show_driver, NULL);
-/* Maximum event message length */
-#define DBG_DATA_MSG 64UL
-
-/* Maximum event messages */
-#define DBG_DATA_MAX 128UL
-
-/* Event buffer descriptor */
-static struct {
- char (buf[DBG_DATA_MAX])[DBG_DATA_MSG]; /* buffer */
- unsigned idx; /* index */
- unsigned tty; /* print to console? */
- rwlock_t lck; /* lock */
-} dbg_data = {
- .idx = 0,
- .tty = 0,
- .lck = __RW_LOCK_UNLOCKED(lck)
-};
-
-/**
- * dbg_dec: decrements debug event index
- * @idx: buffer index
- */
-static void dbg_dec(unsigned *idx)
-{
- *idx = (*idx - 1) & (DBG_DATA_MAX-1);
-}
-
-/**
- * dbg_inc: increments debug event index
- * @idx: buffer index
- */
-static void dbg_inc(unsigned *idx)
-{
- *idx = (*idx + 1) & (DBG_DATA_MAX-1);
-}
-
-/**
- * dbg_print: prints the common part of the event
- * @addr: endpoint address
- * @name: event name
- * @status: status
- * @extra: extra information
- */
-static void dbg_print(u8 addr, const char *name, int status, const char *extra)
-{
- struct timeval tval;
- unsigned int stamp;
- unsigned long flags;
-
- write_lock_irqsave(&dbg_data.lck, flags);
-
- do_gettimeofday(&tval);
- stamp = tval.tv_sec & 0x; /* 2^32 = 4294967296. Limit to 4096s */
- stamp = stamp * 100 + tval.tv_usec;
-
- scnprintf(dbg_data.buf[dbg_data.idx], DBG_DATA_MSG,
- "%04X\t? %02X %-7.7s %4i ?\t%s\n",
- stamp, addr, name, status, extra);
-
- dbg_inc(&dbg_data.idx);
-
- write_unlock_irqrestore(&dbg_data.lck, flags);
-
- if (dbg_data.tty != 0)
- pr_notice("%04X\t? %02X %-7.7s %4i ?\t%s\n",
- stamp, addr, name, status, extra);
-}
-
-/**
- * dbg_done: prints a DONE event
- * @addr: endpoint address
- * @td: transfer descriptor
- * @status: status
- */
-void dbg_done(u8 addr, const u32 token, int status)
-{
- char msg[DBG_DATA_MSG];
-
- scnprintf(msg, sizeof(msg), "%d %02X",
- (int)(token & TD_TOTAL_BYTES) >> ffs_nr(TD_TOTAL_BYTE
[PATCHv0 2/5] usb: chipidea: replace interrupt accounting with tracepoints
The driver also has interrupt counters and another ring buffer for keeping
track of the order in which they arrive. This patch converts these counters
to trace points. Userspace tools such as perf can provide information on both
order and stats of the interrupts.
Signed-off-by: Alexander Shishkin
---
drivers/usb/chipidea/debug.c| 160 +--
drivers/usb/chipidea/debug.h|5 --
drivers/usb/chipidea/udc.c | 11 ++-
include/trace/events/chipidea.h | 18 -
4 files changed, 26 insertions(+), 168 deletions(-)
diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c
index 8937f83..dcbfb2b 100644
--- a/drivers/usb/chipidea/debug.c
+++ b/drivers/usb/chipidea/debug.c
@@ -21,46 +21,6 @@
#include "bits.h"
#include "debug.h"
-/* Interrupt statistics */
-#define ISR_MASK 0x1F
-static struct isr_statistics {
- u32 test;
- u32 ui;
- u32 uei;
- u32 pci;
- u32 uri;
- u32 sli;
- u32 none;
- struct {
- u32 cnt;
- u32 buf[ISR_MASK+1];
- u32 idx;
- } hndl;
-} isr_statistics;
-
-void dbg_interrupt(u32 intmask)
-{
- if (!intmask) {
- isr_statistics.none++;
- return;
- }
-
- isr_statistics.hndl.buf[isr_statistics.hndl.idx++] = intmask;
- isr_statistics.hndl.idx &= ISR_MASK;
- isr_statistics.hndl.cnt++;
-
- if (USBi_URI & intmask)
- isr_statistics.uri++;
- if (USBi_PCI & intmask)
- isr_statistics.pci++;
- if (USBi_UEI & intmask)
- isr_statistics.uei++;
- if (USBi_UI & intmask)
- isr_statistics.ui++;
- if (USBi_SLI & intmask)
- isr_statistics.sli++;
-}
-
/**
* hw_register_read: reads all device registers (execute without interruption)
* @buf: destination buffer
@@ -208,118 +168,6 @@ static ssize_t show_driver(struct device *dev, struct
device_attribute *attr,
static DEVICE_ATTR(driver, S_IRUSR, show_driver, NULL);
/**
- * show_inters: interrupt status, enable status and historic
- *
- * Check "device.h" for details
- */
-static ssize_t show_inters(struct device *dev, struct device_attribute *attr,
- char *buf)
-{
- struct ci13xxx *ci = container_of(dev, struct ci13xxx, gadget.dev);
- unsigned long flags;
- u32 intr;
- unsigned i, j, n = 0;
-
- if (attr == NULL || buf == NULL) {
- dev_err(ci->dev, "[%s] EINVAL\n", __func__);
- return 0;
- }
-
- spin_lock_irqsave(&ci->lock, flags);
-
- /*n += scnprintf(buf + n, PAGE_SIZE - n,
- "status = %08x\n", hw_read_intr_status(ci));
- n += scnprintf(buf + n, PAGE_SIZE - n,
- "enable = %08x\n", hw_read_intr_enable(ci));*/
-
- n += scnprintf(buf + n, PAGE_SIZE - n, "*test = %d\n",
- isr_statistics.test);
- n += scnprintf(buf + n, PAGE_SIZE - n, "? ui = %d\n",
- isr_statistics.ui);
- n += scnprintf(buf + n, PAGE_SIZE - n, "? uei = %d\n",
- isr_statistics.uei);
- n += scnprintf(buf + n, PAGE_SIZE - n, "? pci = %d\n",
- isr_statistics.pci);
- n += scnprintf(buf + n, PAGE_SIZE - n, "? uri = %d\n",
- isr_statistics.uri);
- n += scnprintf(buf + n, PAGE_SIZE - n, "? sli = %d\n",
- isr_statistics.sli);
- n += scnprintf(buf + n, PAGE_SIZE - n, "*none = %d\n",
- isr_statistics.none);
- n += scnprintf(buf + n, PAGE_SIZE - n, "*hndl = %d\n",
- isr_statistics.hndl.cnt);
-
- for (i = isr_statistics.hndl.idx, j = 0; j <= ISR_MASK; j++, i++) {
- i &= ISR_MASK;
- intr = isr_statistics.hndl.buf[i];
-
- if (USBi_UI & intr)
- n += scnprintf(buf + n, PAGE_SIZE - n, "ui ");
- intr &= ~USBi_UI;
- if (USBi_UEI & intr)
- n += scnprintf(buf + n, PAGE_SIZE - n, "uei ");
- intr &= ~USBi_UEI;
- if (USBi_PCI & intr)
- n += scnprintf(buf + n, PAGE_SIZE - n, "pci ");
- intr &= ~USBi_PCI;
- if (USBi_URI & intr)
- n += scnprintf(buf + n, PAGE_SIZE - n, "uri ");
- intr &= ~USBi_URI;
- if (USBi_SLI & intr)
- n += scnprintf(buf + n, PAGE_SIZE - n, "sli ");
- intr &= ~USBi_SLI;
- if (intr)
- n += scnprintf(buf + n, PAGE_SIZE - n, "??? "
[PATCHv0 4/5] usb: chipidea: move role to debugfs
Manual role switching function is there for debugging purposes, so has
to move to debugfs.
Signed-off-by: Alexander Shishkin
---
drivers/usb/chipidea/core.c | 39 --
drivers/usb/chipidea/debug.c | 54 ++
2 files changed, 54 insertions(+), 39 deletions(-)
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index ef965ad..56d41f9 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -285,38 +285,6 @@ static void ci_role_work(struct work_struct *work)
}
}
-static ssize_t show_role(struct device *dev, struct device_attribute *attr,
-char *buf)
-{
- struct ci13xxx *ci = dev_get_drvdata(dev);
-
- return sprintf(buf, "%s\n", ci_role(ci)->name);
-}
-
-static ssize_t store_role(struct device *dev, struct device_attribute *attr,
- const char *buf, size_t count)
-{
- struct ci13xxx *ci = dev_get_drvdata(dev);
- enum ci_role role;
- int ret;
-
- for (role = CI_ROLE_HOST; role < CI_ROLE_END; role++)
- if (ci->roles[role] && !strcmp(buf, ci->roles[role]->name))
- break;
-
- if (role == CI_ROLE_END || role == ci->role)
- return -EINVAL;
-
- ci_role_stop(ci);
- ret = ci_role_start(ci, role);
- if (ret)
- return ret;
-
- return count;
-}
-
-static DEVICE_ATTR(role, S_IRUSR | S_IWUSR, show_role, store_role);
-
static irqreturn_t ci_irq(int irq, void *data)
{
struct ci13xxx *ci = data;
@@ -490,17 +458,11 @@ static int __devinit ci_hdrc_probe(struct platform_device
*pdev)
if (ret)
goto stop;
- ret = device_create_file(dev, &dev_attr_role);
- if (ret)
- goto rm_attr;
-
if (ci->is_otg)
hw_write(ci, OP_OTGSC, OTGSC_IDIE, OTGSC_IDIE);
return ret;
-rm_attr:
- device_remove_file(dev, &dev_attr_role);
stop:
ci_role_stop(ci);
rm_wq:
@@ -516,7 +478,6 @@ static int __devexit ci_hdrc_remove(struct platform_device
*pdev)
flush_workqueue(ci->wq);
destroy_workqueue(ci->wq);
- device_remove_file(ci->dev, &dev_attr_role);
free_irq(ci->irq, ci);
ci_role_stop(ci);
diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c
index 668465c..a87689a 100644
--- a/drivers/usb/chipidea/debug.c
+++ b/drivers/usb/chipidea/debug.c
@@ -208,6 +208,55 @@ static struct file_operations ci_requests_fops = {
.release= single_release,
};
+static int ci_role_show(struct seq_file *s, void *data)
+{
+ struct ci13xxx *ci = s->private;
+
+ seq_printf(s, "%s\n", ci_role(ci)->name);
+
+ return 0;
+}
+
+static ssize_t ci_role_write(struct file *file, const char __user *ubuf,
+size_t count, loff_t *ppos)
+{
+ struct seq_file *s = file->private_data;
+ struct ci13xxx *ci = s->private;
+ enum ci_role role;
+ char buf[8];
+ int ret;
+
+ if (copy_from_user(buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
+return -EFAULT;
+
+ for (role = CI_ROLE_HOST; role < CI_ROLE_END; role++)
+ if (ci->roles[role] &&
+ !strncmp(buf, ci->roles[role]->name,
+strlen(ci->roles[role]->name)))
+ break;
+
+ if (role == CI_ROLE_END || role == ci->role)
+ return -EINVAL;
+
+ ci_role_stop(ci);
+ ret = ci_role_start(ci, role);
+
+ return ret ? ret : count;
+}
+
+static int ci_role_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, ci_role_show, inode->i_private);
+}
+
+static struct file_operations ci_role_fops = {
+ .open = ci_role_open,
+ .write = ci_role_write,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release= single_release,
+};
+
/**
* dbg_create_files: initializes the attribute interface
* @ci: device
@@ -239,6 +288,11 @@ int dbg_create_files(struct ci13xxx *ci)
dent = debugfs_create_file("requests", S_IRUGO, ci->debugfs, ci,
&ci_requests_fops);
+ if (!dent)
+ goto err;
+
+ dent = debugfs_create_file("role", S_IRUGO | S_IWUSR, ci->debugfs, ci,
+ &ci_role_fops);
if (dent)
return 0;
err:
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv0 5/5] usb: chipidea: move debug files creation/removal to the core
Create and remove debugfs entries in hdrc probe/remove instead of
start/stop of the device controller. Gadget specific will not export
anything while the controller is in host mode.
Signed-off-by: Alexander Shishkin
---
drivers/usb/chipidea/core.c |6 +-
drivers/usb/chipidea/udc.c |9 +
2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 56d41f9..0e9c4a8 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -461,8 +461,11 @@ static int __devinit ci_hdrc_probe(struct platform_device
*pdev)
if (ci->is_otg)
hw_write(ci, OP_OTGSC, OTGSC_IDIE, OTGSC_IDIE);
- return ret;
+ ret = dbg_create_files(ci);
+ if (!ret)
+ return 0;
+ free_irq(ci->irq, ci);
stop:
ci_role_stop(ci);
rm_wq:
@@ -476,6 +479,7 @@ static int __devexit ci_hdrc_remove(struct platform_device
*pdev)
{
struct ci13xxx *ci = platform_get_drvdata(pdev);
+ dbg_remove_files(ci);
flush_workqueue(ci->wq);
destroy_workqueue(ci->wq);
free_irq(ci->irq, ci);
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 48adead..36fce40 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -1762,15 +1762,11 @@ static int udc_start(struct ci13xxx *ci)
goto put_transceiver;
}
- retval = dbg_create_files(ci);
- if (retval)
- goto unreg_device;
-
if (!IS_ERR_OR_NULL(ci->transceiver)) {
retval = otg_set_peripheral(ci->transceiver->otg,
&ci->gadget);
if (retval)
- goto remove_dbg;
+ goto unreg_device;
}
retval = usb_add_gadget_udc(dev, &ci->gadget);
@@ -1790,8 +1786,6 @@ remove_trans:
}
dev_err(dev, "error = %i\n", retval);
-remove_dbg:
- dbg_remove_files(ci);
unreg_device:
device_unregister(&ci->gadget.dev);
put_transceiver:
@@ -1831,7 +1825,6 @@ static void udc_stop(struct ci13xxx *ci)
if (ci->global_phy)
usb_put_phy(ci->transceiver);
}
- dbg_remove_files(ci);
device_unregister(&ci->gadget.dev);
/* my kobject is dynamic, I swear! */
memset(&ci->gadget, 0, sizeof(ci->gadget));
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv0 3/5] usb: chipidea: convert debug entries in sysfs to debugfs
Currently, we have a bunch of files in sysfs that display all sorts of
debugging information for the device controller, so they have to move to
debugfs where they belong. The "registers" interface have been removed,
since it doesn't fit into the current driver design as is and it's hardly
a good idea to touch the registers from userspace anyway.
Signed-off-by: Alexander Shishkin
---
drivers/usb/chipidea/ci.h|2 +
drivers/usb/chipidea/debug.c | 463 +-
drivers/usb/chipidea/debug.h |9 +-
drivers/usb/chipidea/udc.c |6 +-
4 files changed, 147 insertions(+), 333 deletions(-)
diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index f67d101..fc8fc06 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -140,6 +140,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
+ * @debugfs: root dentry for this controller in debugfs
*/
struct ci13xxx {
struct device *dev;
@@ -176,6 +177,7 @@ struct ci13xxx {
boolglobal_phy;
struct usb_phy *transceiver;
struct usb_hcd *hcd;
+ struct dentry *debugfs;
};
static inline struct ci_role_driver *ci_role(struct ci13xxx *ci)
diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c
index dcbfb2b..668465c 100644
--- a/drivers/usb/chipidea/debug.c
+++ b/drivers/usb/chipidea/debug.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -22,223 +23,113 @@
#include "debug.h"
/**
- * hw_register_read: reads all device registers (execute without interruption)
- * @buf: destination buffer
- * @size: buffer size
- *
- * This function returns number of registers read
+ * ci_device_show: prints information about device capabilities and status
*/
-static size_t hw_register_read(struct ci13xxx *ci, u32 *buf, size_t size)
+static int ci_device_show(struct seq_file *s, void *data)
{
- unsigned i;
-
- if (size > ci->hw_bank.size)
- size = ci->hw_bank.size;
-
- for (i = 0; i < size; i++)
- buf[i] = hw_read(ci, i * sizeof(u32), ~0);
-
- return size;
-}
-
-/**
- * hw_register_write: writes to register
- * @addr: register address
- * @data: register value
- *
- * This function returns an error code
- */
-static int hw_register_write(struct ci13xxx *ci, u16 addr, u32 data)
-{
- /* align */
- addr /= sizeof(u32);
+ struct ci13xxx *ci = s->private;
+ struct usb_gadget *gadget = &ci->gadget;
- if (addr >= ci->hw_bank.size)
- return -EINVAL;
+ seq_printf(s, "speed = %d\n", gadget->speed);
+ seq_printf(s, "max_speed = %d\n", gadget->max_speed);
+ seq_printf(s, "is_otg= %d\n", gadget->is_otg);
+ seq_printf(s, "is_a_peripheral = %d\n", gadget->is_a_peripheral);
+ seq_printf(s, "b_hnp_enable = %d\n", gadget->b_hnp_enable);
+ seq_printf(s, "a_hnp_support = %d\n", gadget->a_hnp_support);
+ seq_printf(s, "a_alt_hnp_support = %d\n", gadget->a_alt_hnp_support);
+ seq_printf(s, "name = %s\n",
+ (gadget->name ? gadget->name : ""));
+
+ if (!ci->driver)
+ return 0;
- /* align */
- addr *= sizeof(u32);
+ seq_printf(s, "gadget function = %s\n",
+ (ci->driver->function ? ci->driver->function : ""));
+ seq_printf(s, "gadget max speed = %d\n", ci->driver->max_speed);
- hw_write(ci, addr, ~0, data);
return 0;
}
-/**
- * hw_intr_clear: disables interrupt & clears interrupt status (execute without
- *interruption)
- * @n: interrupt bit
- *
- * This function returns an error code
- */
-static int hw_intr_clear(struct ci13xxx *ci, int n)
+static int ci_device_open(struct inode *inode, struct file *file)
{
- if (n >= REG_BITS)
- return -EINVAL;
-
- hw_write(ci, OP_USBINTR, BIT(n), 0);
- hw_write(ci, OP_USBSTS, BIT(n), BIT(n));
- return 0;
+ return single_open(file, ci_device_show, inode->i_private);
}
-/**
- * hw_intr_force: enables interrupt & forces interrupt status (execute without
- *interruption)
- * @n: interrupt bit
- *
- * This function returns an error code
- */
-static int hw_intr_force(struct ci13xxx *ci, int n)
-{
- if (n >= REG_BITS)
- return -EINVAL;
-
- hw_write(ci, CAP_TESTMODE, TESTMODE_FORCE, TESTMODE_FORCE);
- hw_write(ci, OP_USBINTR, BIT(n), BIT(n));
Re: [PATCH v3 0/7] support other fsl SoCs with usbmisc + small fixes
Michael Grzeschik writes: > Nearly every SoC from Freescale has this non-core usb registers. This series > adds support for more users of this driver. > > This series is based on Peter Chen's work. Its needed to merge his master > branch > before applying this series: > > https://github.com/hzpeterchen/linux-usb.git > > Marc Kleine-Budde (4): > usb: chipidea: usbmisc: unset global varibale usbmisc on driver > remove > usb: chipidea: usbmisc: fix a potential race condition > usb: chipidea: usbmisc: prepare driver to handle more than one soc > usb: chipidea: usbmisc: add support for ahb, ipg and per clock > > Michael Grzeschik (3): > usb: chipidea: usbmisc: rename file, struct and functions to > usbmisc_imx > usb: chipidea: usbmisc: add mx53 support > usb: chipidea: usbmisc: add post handling and errata fix for mx25 Cc'ing Peter Chen, in case he didn't see this. Peter, can you have a look and give your Tested-by/Reviewed-by where appropriate? Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUGFIX PATCH] USB: chipidea: fix use after free bug
Lothar Waßmann writes:
> The pointer to a platform_device struct must not be dereferenced after
> the device has been unregistered.
>
> This bug produces a crash when unloading the ci13xxx kernel module
> compiled with CONFIG_PAGE_POISONING enabled.
>
> Signed-off-by: Lothar Waßmann
Good one, thanks.
Acked-by: Alexander Shishkin
Greg, this is -stable material, applicable starting from v3.6.
> ---
> drivers/usb/chipidea/core.c |3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index f69d029..b726c49 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -385,8 +385,9 @@ EXPORT_SYMBOL_GPL(ci13xxx_add_device);
>
> void ci13xxx_remove_device(struct platform_device *pdev)
> {
> + int id = pdev->id;
> platform_device_unregister(pdev);
> - ida_simple_remove(&ci_ida, pdev->id);
> + ida_simple_remove(&ci_ida, id);
> }
> EXPORT_SYMBOL_GPL(ci13xxx_remove_device);
>
> --
> 1.7.2.5
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/7] usb: chipidea: usbmisc: fix a potential race condition
Sascha Hauer writes: > On Fri, Nov 23, 2012 at 01:36:36PM +0800, Peter Chen wrote: >> On Wed, Nov 21, 2012 at 03:06:29PM +0100, Michael Grzeschik wrote: >> > From: Marc Kleine-Budde >> > >> > This fixes a potential race condition where the ci13xxx_imx glue code >> > could be fast enough to call one of the usbmisc_ops before he got a >> > valid value on the static usbmisc pointer. To fix that we first set >> > usbmisc, then call usbmisc_set_ops(). >> >> usbmisc is subsys_initcall, and cil13xxx_imx is module_init. Any >> potential situation that the ci13xxx_imx's probe is ran before the >> usbmisc's probe is completed? > > Not having looked at the code you are referring to at all I just want > to say that: drivers can be modules (don't know if that's true for > chipidea) and sooner or later we'll probably get devicetree overlays, ChipIdea can be not even one, but two modules (ci_hdrc, the actual controller driver, always a platform_driver) and platform bindings like ci13xxx_imx, ci13xxx_pci, ci13xxx_msm, which can be platform or pci or whatever else drivers. > so the devicetree nodes might just appear during runtime. Depending on > initcall order is generally not a good idea. That's very true. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/9] usb: chipidea: ci13xxx-imx: add "dr_mode" property to device tree bindings
Peter Chen writes: > On Fri, Nov 16, 2012 at 01:53:09PM +0200, Alexander Shishkin wrote: >> Michael Grzeschik writes: >> I'd prefer this function to live in ci13xxx_imx, since that's where it's >> used and it doesn't really need anything from core.c anyway. Or maybe it >> would make sense to make it even more generic (for other devitetree >> users), since you're saying that other drivers are using this already. >> >> Looks good to me otherwise. > > I think it supplies a way that the platform can override the usb role, > only freescale uses it, but also other platforms may use it. It is a > generic feature, other chipidea users may need it. I'm not objecting against the flags, I just don't like the function in chipidea core. Platform code can just use the flags. It can be either part of the platform driver or a generic dt helper, in case there are more users, it doesn't belong to chipidea core. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/7] usb: chipidea: add otg file
Peter Chen writes:
> diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> index d738603..8702871 100644
> --- a/drivers/usb/chipidea/ci.h
> +++ b/drivers/usb/chipidea/ci.h
> @@ -129,6 +129,7 @@ struct hw_bank {
> * @vbus_active: is VBUS active
> * @transceiver: pointer to USB PHY, if any
> * @hcd: pointer to usb_hcd for ehci host driver
> + * @otg: for otg support
> */
> struct ci13xxx {
> struct device *dev;
> @@ -164,6 +165,7 @@ struct ci13xxx {
> boolglobal_phy;
> struct usb_phy *transceiver;
> struct usb_hcd *hcd;
> + struct usb_otg otg;
Can you indent the "otg" so that it's aligned with the rest?
> };
>
> static inline struct ci_role_driver *ci_role(struct ci13xxx *ci)
> diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
> new file mode 100644
> index 000..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
> +#include
> +#include
> +#include
> +#include
> +#include
Most of these look unnecessary.
> +#include
> +#include
> +#include
> +
> +#include "ci.h"
> +#include "udc.h"
> +#include "bits.h"
> +#include "host.h"
> +#include "debug.h"
And these.
> +
> +static int ci_otg_set_peripheral(struct usb_otg *otg,
> + struct usb_gadget *periph)
> +{
> + otg->gadget = periph;
> +
> + return 0;
> +}
> +
> +static int ci_otg_set_host(struct usb_otg *otg, struct usb_bus *host)
> +{
> + otg->host = host;
> +
> + return 0;
> +}
> +
> +/**
> + * ci_hdrc_otg_init - initialize device related bits
> + * ci: the controller
> + *
> + * This function create otg struct, if the device can switch between
> + * device and host.
> + */
> +int ci_hdrc_otg_init(struct ci13xxx *ci)
> +{
> + /* Useless at current */
> + ci->otg.set_peripheral = ci_otg_set_peripheral;
> + ci->otg.set_host = ci_otg_set_host;
> + if (!IS_ERR_OR_NULL(ci->transceiver))
> + ci->transceiver->otg = &ci->otg;
> +
> + return 0;
> +}
> diff --git a/drivers/usb/chipidea/otg.h b/drivers/usb/chipidea/otg.h
> new file mode 100644
> index 000..b4c6b3e
> --- /dev/null
> +++ b/drivers/usb/chipidea/otg.h
> @@ -0,0 +1,6 @@
> +#ifndef __DRIVERS_USB_CHIPIDEA_OTG_H
> +#define __DRIVERS_USB_CHIPIDEA_OTG_H
> +
> +int ci_hdrc_otg_init(struct ci13xxx *ci);
Can you put it to ci.h instead?
Thanks,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect
Peter Chen writes:
> +static void ci_otg_work(struct work_struct *work)
> +{
> + struct ci13xxx *ci = container_of(work, struct ci13xxx, work);
> +
> + if (test_bit(CI_ID, &ci->events)) {
> + clear_bit(CI_ID, &ci->events);
> + ci_handle_id_switch(ci);
> + } else if (test_bit(CI_B_SESS_VALID, &ci->events)) {
> + clear_bit(CI_B_SESS_VALID, &ci->events);
> + ci_handle_vbus_change(ci);
> + } else
> + dev_err(ci->dev, "unexpected event occurs at %s\n", __func__);
> +
> + enable_irq(ci->irq);
> +}
if (ci->id) {
ci->id = false;
ci_handle_id_switch(ci);
} else if (ci->vbus_valid) {
ci->vbus_valid = false;
ci_handle_vbus_change(ci);
} ...
is so much easier on the eyes. We really don't have any reasons to have
events in a bitmask.
> +
> +static void ci_delayed_work(struct work_struct *work)
> +{
> + struct delayed_work *dwork = to_delayed_work(work);
> + struct ci13xxx *ci = container_of(dwork, struct ci13xxx, dwork);
> +
> + otg_set_vbus(&ci->otg, true);
> +
> +}
> +
> static ssize_t show_role(struct device *dev, struct device_attribute *attr,
>char *buf)
> {
> @@ -321,19 +422,36 @@ static irqreturn_t ci_irq(int irq, void *data)
> irqreturn_t ret = IRQ_NONE;
> u32 otgsc = 0;
>
> - if (ci->is_otg)
> - otgsc = hw_read(ci, OP_OTGSC, ~0);
> -
> - if (ci->role != CI_ROLE_END)
> - ret = ci_role(ci)->irq(ci);
> + otgsc = hw_read(ci, OP_OTGSC, ~0);
>
> - if (ci->is_otg && (otgsc & OTGSC_IDIS)) {
> + /*
> + * Handle id change interrupt, it indicates device/host function
> + * switch.
> + */
> + if (ci->is_otg && (otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS)) {
> + set_bit(CI_ID, &ci->events);
> hw_write(ci, OP_OTGSC, OTGSC_IDIS, OTGSC_IDIS);
A thought: we're setting and unsetting IDIS/IDIE and BSVIS/BSVIE that we
might use a helper function for it.
> disable_irq_nosync(ci->irq);
> queue_work(ci->wq, &ci->work);
> - ret = IRQ_HANDLED;
> + return IRQ_HANDLED;
> + }
> +
> + /*
> + * Handle vbus change interrupt, it indicates device connection
> + * and disconnection events.
> + */
> + if ((otgsc & OTGSC_BSVIE) && (otgsc & OTGSC_BSVIS)) {
> + set_bit(CI_B_SESS_VALID, &ci->events);
> + hw_write(ci, OP_OTGSC, OTGSC_BSVIS, OTGSC_BSVIS);
> + disable_irq_nosync(ci->irq);
> + queue_work(ci->wq, &ci->work);
> + return IRQ_HANDLED;
> }
>
> + /* Handle device/host interrupt */
> + if (ci->role != CI_ROLE_END)
> + ret = ci_role(ci)->irq(ci);
> +
> return ret;
> }
>
> @@ -397,6 +515,7 @@ static int __devinit ci_hdrc_probe(struct platform_device
> *pdev)
> struct resource *res;
> void __iomem*base;
> int ret;
> + u32 otgsc;
>
> if (!dev->platform_data) {
> dev_err(dev, "platform data missing\n");
> @@ -421,6 +540,7 @@ static int __devinit ci_hdrc_probe(struct platform_device
> *pdev)
> return -ENOMEM;
> }
>
> + ci->events = 0;
"ci" has just been kzalloc'ed.
> ci->dev = dev;
> ci->platdata = dev->platform_data;
> if (ci->platdata->phy)
> @@ -442,12 +562,20 @@ static int __devinit ci_hdrc_probe(struct
> platform_device *pdev)
> return -ENODEV;
> }
>
> - INIT_WORK(&ci->work, ci_role_work);
> + INIT_WORK(&ci->work, ci_otg_work);
> + INIT_DELAYED_WORK(&ci->dwork, ci_delayed_work);
> ci->wq = create_singlethread_workqueue("ci_otg");
> if (!ci->wq) {
> dev_err(dev, "can't create workqueue\n");
> return -ENODEV;
> }
> + /* Disable all interrupts bits */
> + hw_write(ci, OP_USBINTR, 0x, 0);
> + hw_write(ci, OP_OTGSC, OTGSC_INT_EN_BITS, 0);
> +
> + /* Clear all interrupts status bits*/
> + hw_write(ci, OP_USBSTS, 0x, 0x);
> + hw_write(ci, OP_OTGSC, OTGSC_INT_STATUS_BITS, OTGSC_INT_STATUS_BITS);
How about moving these to hw_device_init()?
>
> /* initialize role(s) before the interrupt is requested */
> ret = ci_hdrc_host_init(ci);
> @@ -465,6 +593,7 @@ static int __devinit ci_hdrc_probe(struct platform_device
> *pdev)
> }
>
> if (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]) {
> + dev_dbg(dev, "support otg\n");
> ci->is_otg = true;
> /* ID pin needs 1ms debouce time, we delay 2ms for safe */
> mdelay(2);
> @@ -475,13 +604,29 @@ static int __devinit ci_hdrc_probe(struct
> platform_device *pdev)
> : CI_ROLE_GADGET;
> }
>
> + if (ci->is_otg)
> + /* if otg is supported, create struct usb_otg */
> + ci_hdrc_otg_init(ci);
Can this ju
Re: Need review for chipidea driver
On 24 January 2013 05:15, Chen Peter-B29397 wrote: > Sorry, forget to cc the list > >> >> Hi Greg, >> >> Alex has no response for chipidea driver review for long time >> (more than 1 month), below are some links about patchset. >> Does anyone else can help do it? Sorry for that. Looking into it asap. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 0/6] support other fsl SoCs with usbmisc + small fixes
Peter Chen writes: > On Tue, Nov 27, 2012 at 05:16:55PM +0100, Michael Grzeschik wrote: >> Nearly every SoC from Freescale has this non-core usb registers. This series >> adds support for more users of this driver. >> >> This series is based on Peter Chen's work. Its needed to merge his master >> branch >> before applying this series: >> >> https://github.com/hzpeterchen/linux-usb.git > > I have tested it at i.mx6q sabrelite board, it works good. > > I have pushed your commit to my git, please cc me > your coming chipidea patches, thanks. > > Alex, please add: > > Reviewed-by: Peter Chen > Tested-by: Peter Chen Looks good, queueing this one for submission. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv1 0/9] usb: chipidea: clean up debuggery
This patchset cleans up the remaining debugging bits in the driver by either conventing them to trace points or moving to debugfs, all the while getting rid of home-grown tracing facilities. Trace events are accessible through perf tool. If you don't yet have it in your testing environment, now is the good time. Alexander Shishkin (8): usb: chipidea: drop redundant includes usb: chipidea: trim include list in udc code usb: chipidea: trim include list in the core usb: chipidea: convert events to tracepoints usb: chipidea: replace interrupt accounting with tracepoints usb: chipidea: convert debug entries in sysfs to debugfs usb: chipidea: move role to debugfs usb: chipidea: move debug files creation/removal to the core Dan Carpenter (1): usb: chipidea: fix precedence bug in ci_requests_show() drivers/usb/chipidea/ci.h | 15 + drivers/usb/chipidea/core.c | 50 +-- drivers/usb/chipidea/debug.c| 888 +-- drivers/usb/chipidea/debug.h| 34 +- drivers/usb/chipidea/udc.c | 74 ++-- drivers/usb/chipidea/udc.h |2 - include/trace/events/chipidea.h | 151 +++ 7 files changed, 399 insertions(+), 815 deletions(-) create mode 100644 include/trace/events/chipidea.h -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/9] usb: chipidea: drop redundant includes
debug.c is carrying a lot of includes that aren't needed there, drop them. Signed-off-by: Alexander Shishkin --- drivers/usb/chipidea/debug.c | 17 +++-- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c index 3bc244d..117910f 100644 --- a/drivers/usb/chipidea/debug.c +++ b/drivers/usb/chipidea/debug.c @@ -1,20 +1,9 @@ -#include -#include -#include -#include -#include -#include -#include -#include -#include -#include #include -#include -#include +#include +#include +#include #include #include -#include -#include #include "ci.h" #include "udc.h" -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/9] usb: chipidea: trim include list in udc code
Some headers included in udc core code are not actually needed, remove them. Signed-off-by: Alexander Shishkin --- drivers/usb/chipidea/udc.c |8 +--- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c index 2f45bba..aea09d2 100644 --- a/drivers/usb/chipidea/udc.c +++ b/drivers/usb/chipidea/udc.c @@ -13,14 +13,8 @@ #include #include #include -#include #include -#include -#include -#include -#include -#include -#include +#include #include #include #include -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 4/9] usb: chipidea: convert events to tracepoints
As part of the legacy from the original driver design, we retain home-grown
tracing infrastructure, complete with own ring buffer and timestamps. While
it is useful for debugging certain cases, it's a lot of extra code, which
these days is rather redundant.
This patch replaces local tracing functionality with kernel tracepoints,
thus getting rid of the ring buffer and all the maintenance code, while
making use of standard kernel infrastructure.
Signed-off-by: Alexander Shishkin
---
drivers/usb/chipidea/ci.h | 13 +++
drivers/usb/chipidea/core.c |2 +
drivers/usb/chipidea/debug.c| 202 +--
drivers/usb/chipidea/debug.h| 20
drivers/usb/chipidea/udc.c | 46 -
drivers/usb/chipidea/udc.h |2 -
include/trace/events/chipidea.h | 135 ++
7 files changed, 169 insertions(+), 251 deletions(-)
create mode 100644 include/trace/events/chipidea.h
diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index e25d126..f67d101 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -23,6 +23,8 @@
*/
#define CI13XXX_PAGE_SIZE 4096ul /* page size for TD's */
#define ENDPT_MAX 32
+#define RX0 /* similar to USB_DIR_OUT but can be used as an index */
+#define TX1 /* similar to USB_DIR_IN but can be used as an index */
/**
* STRUCTURES
@@ -59,6 +61,15 @@ struct ci13xxx_ep {
struct dma_pool *td_pool;
};
+/**
+ * ci_ep_addr: calculates endpoint address from direction & number
+ * @ep: endpoint
+ */
+static inline u8 ci_ep_addr(struct ci13xxx_ep *ep)
+{
+ return ((ep->dir == TX) ? USB_ENDPOINT_DIR_MASK : 0) | ep->num;
+}
+
enum ci_role {
CI_ROLE_HOST = 0,
CI_ROLE_GADGET,
@@ -313,4 +324,6 @@ int hw_port_test_set(struct ci13xxx *ci, u8 mode);
u8 hw_port_test_get(struct ci13xxx *ci);
+#include
+
#endif /* __DRIVERS_USB_CHIPIDEA_CI_H */
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index 77963b6..d0aa172 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -65,6 +65,8 @@
#include
#include
+#define CREATE_TRACE_POINTS
+
#include "ci.h"
#include "udc.h"
#include "bits.h"
diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c
index 117910f..9e9caa8 100644
--- a/drivers/usb/chipidea/debug.c
+++ b/drivers/usb/chipidea/debug.c
@@ -196,200 +196,6 @@ static ssize_t show_driver(struct device *dev, struct
device_attribute *attr,
}
static DEVICE_ATTR(driver, S_IRUSR, show_driver, NULL);
-/* Maximum event message length */
-#define DBG_DATA_MSG 64UL
-
-/* Maximum event messages */
-#define DBG_DATA_MAX 128UL
-
-/* Event buffer descriptor */
-static struct {
- char (buf[DBG_DATA_MAX])[DBG_DATA_MSG]; /* buffer */
- unsigned idx; /* index */
- unsigned tty; /* print to console? */
- rwlock_t lck; /* lock */
-} dbg_data = {
- .idx = 0,
- .tty = 0,
- .lck = __RW_LOCK_UNLOCKED(lck)
-};
-
-/**
- * dbg_dec: decrements debug event index
- * @idx: buffer index
- */
-static void dbg_dec(unsigned *idx)
-{
- *idx = (*idx - 1) & (DBG_DATA_MAX-1);
-}
-
-/**
- * dbg_inc: increments debug event index
- * @idx: buffer index
- */
-static void dbg_inc(unsigned *idx)
-{
- *idx = (*idx + 1) & (DBG_DATA_MAX-1);
-}
-
-/**
- * dbg_print: prints the common part of the event
- * @addr: endpoint address
- * @name: event name
- * @status: status
- * @extra: extra information
- */
-static void dbg_print(u8 addr, const char *name, int status, const char *extra)
-{
- struct timeval tval;
- unsigned int stamp;
- unsigned long flags;
-
- write_lock_irqsave(&dbg_data.lck, flags);
-
- do_gettimeofday(&tval);
- stamp = tval.tv_sec & 0x; /* 2^32 = 4294967296. Limit to 4096s */
- stamp = stamp * 100 + tval.tv_usec;
-
- scnprintf(dbg_data.buf[dbg_data.idx], DBG_DATA_MSG,
- "%04X\t? %02X %-7.7s %4i ?\t%s\n",
- stamp, addr, name, status, extra);
-
- dbg_inc(&dbg_data.idx);
-
- write_unlock_irqrestore(&dbg_data.lck, flags);
-
- if (dbg_data.tty != 0)
- pr_notice("%04X\t? %02X %-7.7s %4i ?\t%s\n",
- stamp, addr, name, status, extra);
-}
-
-/**
- * dbg_done: prints a DONE event
- * @addr: endpoint address
- * @td: transfer descriptor
- * @status: status
- */
-void dbg_done(u8 addr, const u32 token, int status)
-{
- char msg[DBG_DATA_MSG];
-
- scnprintf(msg, sizeof(msg), "%d %02X",
- (int)(token & TD_TOTAL_BYTES) >> ffs_nr(TD_TOTAL_BYTE
[PATCH 3/9] usb: chipidea: trim include list in the core
Some headers included in the chipidea controller core are not needed, remove them. Signed-off-by: Alexander Shishkin --- drivers/usb/chipidea/core.c |3 --- 1 file changed, 3 deletions(-) diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index 57cae1f..77963b6 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -51,15 +51,12 @@ */ #include #include -#include #include -#include #include #include #include #include #include -#include #include #include #include -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/9] usb: chipidea: replace interrupt accounting with tracepoints
The driver also has interrupt counters and another ring buffer for keeping
track of the order in which they arrive. This patch converts these counters
to trace points. Userspace tools such as perf can provide information on both
order and stats of the interrupts.
Signed-off-by: Alexander Shishkin
---
drivers/usb/chipidea/debug.c| 160 +--
drivers/usb/chipidea/debug.h|5 --
drivers/usb/chipidea/udc.c | 11 ++-
include/trace/events/chipidea.h | 18 -
4 files changed, 26 insertions(+), 168 deletions(-)
diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c
index 9e9caa8..898aca5 100644
--- a/drivers/usb/chipidea/debug.c
+++ b/drivers/usb/chipidea/debug.c
@@ -10,46 +10,6 @@
#include "bits.h"
#include "debug.h"
-/* Interrupt statistics */
-#define ISR_MASK 0x1F
-static struct isr_statistics {
- u32 test;
- u32 ui;
- u32 uei;
- u32 pci;
- u32 uri;
- u32 sli;
- u32 none;
- struct {
- u32 cnt;
- u32 buf[ISR_MASK+1];
- u32 idx;
- } hndl;
-} isr_statistics;
-
-void dbg_interrupt(u32 intmask)
-{
- if (!intmask) {
- isr_statistics.none++;
- return;
- }
-
- isr_statistics.hndl.buf[isr_statistics.hndl.idx++] = intmask;
- isr_statistics.hndl.idx &= ISR_MASK;
- isr_statistics.hndl.cnt++;
-
- if (USBi_URI & intmask)
- isr_statistics.uri++;
- if (USBi_PCI & intmask)
- isr_statistics.pci++;
- if (USBi_UEI & intmask)
- isr_statistics.uei++;
- if (USBi_UI & intmask)
- isr_statistics.ui++;
- if (USBi_SLI & intmask)
- isr_statistics.sli++;
-}
-
/**
* hw_register_read: reads all device registers (execute without interruption)
* @buf: destination buffer
@@ -197,118 +157,6 @@ static ssize_t show_driver(struct device *dev, struct
device_attribute *attr,
static DEVICE_ATTR(driver, S_IRUSR, show_driver, NULL);
/**
- * show_inters: interrupt status, enable status and historic
- *
- * Check "device.h" for details
- */
-static ssize_t show_inters(struct device *dev, struct device_attribute *attr,
- char *buf)
-{
- struct ci13xxx *ci = container_of(dev, struct ci13xxx, gadget.dev);
- unsigned long flags;
- u32 intr;
- unsigned i, j, n = 0;
-
- if (attr == NULL || buf == NULL) {
- dev_err(ci->dev, "[%s] EINVAL\n", __func__);
- return 0;
- }
-
- spin_lock_irqsave(&ci->lock, flags);
-
- /*n += scnprintf(buf + n, PAGE_SIZE - n,
- "status = %08x\n", hw_read_intr_status(ci));
- n += scnprintf(buf + n, PAGE_SIZE - n,
- "enable = %08x\n", hw_read_intr_enable(ci));*/
-
- n += scnprintf(buf + n, PAGE_SIZE - n, "*test = %d\n",
- isr_statistics.test);
- n += scnprintf(buf + n, PAGE_SIZE - n, "? ui = %d\n",
- isr_statistics.ui);
- n += scnprintf(buf + n, PAGE_SIZE - n, "? uei = %d\n",
- isr_statistics.uei);
- n += scnprintf(buf + n, PAGE_SIZE - n, "? pci = %d\n",
- isr_statistics.pci);
- n += scnprintf(buf + n, PAGE_SIZE - n, "? uri = %d\n",
- isr_statistics.uri);
- n += scnprintf(buf + n, PAGE_SIZE - n, "? sli = %d\n",
- isr_statistics.sli);
- n += scnprintf(buf + n, PAGE_SIZE - n, "*none = %d\n",
- isr_statistics.none);
- n += scnprintf(buf + n, PAGE_SIZE - n, "*hndl = %d\n",
- isr_statistics.hndl.cnt);
-
- for (i = isr_statistics.hndl.idx, j = 0; j <= ISR_MASK; j++, i++) {
- i &= ISR_MASK;
- intr = isr_statistics.hndl.buf[i];
-
- if (USBi_UI & intr)
- n += scnprintf(buf + n, PAGE_SIZE - n, "ui ");
- intr &= ~USBi_UI;
- if (USBi_UEI & intr)
- n += scnprintf(buf + n, PAGE_SIZE - n, "uei ");
- intr &= ~USBi_UEI;
- if (USBi_PCI & intr)
- n += scnprintf(buf + n, PAGE_SIZE - n, "pci ");
- intr &= ~USBi_PCI;
- if (USBi_URI & intr)
- n += scnprintf(buf + n, PAGE_SIZE - n, "uri ");
- intr &= ~USBi_URI;
- if (USBi_SLI & intr)
- n += scnprintf(buf + n, PAGE_SIZE - n, "sli ");
- intr &= ~USBi_SLI;
- if (intr)
- n += scnprintf(buf + n, PAGE_SIZE - n, "??? "
[PATCH 7/9] usb: chipidea: move role to debugfs
Manual role switching function is there for debugging purposes, so has
to move to debugfs.
Signed-off-by: Alexander Shishkin
---
drivers/usb/chipidea/core.c | 39 --
drivers/usb/chipidea/debug.c | 54 ++
2 files changed, 54 insertions(+), 39 deletions(-)
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index d0aa172..ab7861d 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -282,38 +282,6 @@ static void ci_role_work(struct work_struct *work)
}
}
-static ssize_t show_role(struct device *dev, struct device_attribute *attr,
-char *buf)
-{
- struct ci13xxx *ci = dev_get_drvdata(dev);
-
- return sprintf(buf, "%s\n", ci_role(ci)->name);
-}
-
-static ssize_t store_role(struct device *dev, struct device_attribute *attr,
- const char *buf, size_t count)
-{
- struct ci13xxx *ci = dev_get_drvdata(dev);
- enum ci_role role;
- int ret;
-
- for (role = CI_ROLE_HOST; role < CI_ROLE_END; role++)
- if (ci->roles[role] && !strcmp(buf, ci->roles[role]->name))
- break;
-
- if (role == CI_ROLE_END || role == ci->role)
- return -EINVAL;
-
- ci_role_stop(ci);
- ret = ci_role_start(ci, role);
- if (ret)
- return ret;
-
- return count;
-}
-
-static DEVICE_ATTR(role, S_IRUSR | S_IWUSR, show_role, store_role);
-
static irqreturn_t ci_irq(int irq, void *data)
{
struct ci13xxx *ci = data;
@@ -488,17 +456,11 @@ static int ci_hdrc_probe(struct platform_device *pdev)
if (ret)
goto stop;
- ret = device_create_file(dev, &dev_attr_role);
- if (ret)
- goto rm_attr;
-
if (ci->is_otg)
hw_write(ci, OP_OTGSC, OTGSC_IDIE, OTGSC_IDIE);
return ret;
-rm_attr:
- device_remove_file(dev, &dev_attr_role);
stop:
ci_role_stop(ci);
rm_wq:
@@ -514,7 +476,6 @@ static int ci_hdrc_remove(struct platform_device *pdev)
flush_workqueue(ci->wq);
destroy_workqueue(ci->wq);
- device_remove_file(ci->dev, &dev_attr_role);
free_irq(ci->irq, ci);
ci_role_stop(ci);
diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c
index 057ae09..5738079 100644
--- a/drivers/usb/chipidea/debug.c
+++ b/drivers/usb/chipidea/debug.c
@@ -199,6 +199,55 @@ static const struct file_operations ci_requests_fops = {
.release= single_release,
};
+static int ci_role_show(struct seq_file *s, void *data)
+{
+ struct ci13xxx *ci = s->private;
+
+ seq_printf(s, "%s\n", ci_role(ci)->name);
+
+ return 0;
+}
+
+static ssize_t ci_role_write(struct file *file, const char __user *ubuf,
+size_t count, loff_t *ppos)
+{
+ struct seq_file *s = file->private_data;
+ struct ci13xxx *ci = s->private;
+ enum ci_role role;
+ char buf[8];
+ int ret;
+
+ if (copy_from_user(buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
+ return -EFAULT;
+
+ for (role = CI_ROLE_HOST; role < CI_ROLE_END; role++)
+ if (ci->roles[role] &&
+ !strncmp(buf, ci->roles[role]->name,
+strlen(ci->roles[role]->name)))
+ break;
+
+ if (role == CI_ROLE_END || role == ci->role)
+ return -EINVAL;
+
+ ci_role_stop(ci);
+ ret = ci_role_start(ci, role);
+
+ return ret ? ret : count;
+}
+
+static int ci_role_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, ci_role_show, inode->i_private);
+}
+
+static const struct file_operations ci_role_fops = {
+ .open = ci_role_open,
+ .write = ci_role_write,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release= single_release,
+};
+
/**
* dbg_create_files: initializes the attribute interface
* @ci: device
@@ -230,6 +279,11 @@ int dbg_create_files(struct ci13xxx *ci)
dent = debugfs_create_file("requests", S_IRUGO, ci->debugfs, ci,
&ci_requests_fops);
+ if (!dent)
+ goto err;
+
+ dent = debugfs_create_file("role", S_IRUGO | S_IWUSR, ci->debugfs, ci,
+ &ci_role_fops);
if (dent)
return 0;
err:
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 8/9] usb: chipidea: move debug files creation/removal to the core
Create and remove debugfs entries in hdrc probe/remove instead of
start/stop of the device controller. Gadget specific will not export
anything while the controller is in host mode.
Signed-off-by: Alexander Shishkin
---
drivers/usb/chipidea/core.c |6 +-
drivers/usb/chipidea/udc.c |9 +
2 files changed, 6 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
index ab7861d..8f1b083 100644
--- a/drivers/usb/chipidea/core.c
+++ b/drivers/usb/chipidea/core.c
@@ -459,8 +459,11 @@ static int ci_hdrc_probe(struct platform_device *pdev)
if (ci->is_otg)
hw_write(ci, OP_OTGSC, OTGSC_IDIE, OTGSC_IDIE);
- return ret;
+ ret = dbg_create_files(ci);
+ if (!ret)
+ return 0;
+ free_irq(ci->irq, ci);
stop:
ci_role_stop(ci);
rm_wq:
@@ -474,6 +477,7 @@ static int ci_hdrc_remove(struct platform_device *pdev)
{
struct ci13xxx *ci = platform_get_drvdata(pdev);
+ dbg_remove_files(ci);
flush_workqueue(ci->wq);
destroy_workqueue(ci->wq);
free_irq(ci->irq, ci);
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 6527f5c..58c8164 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -1756,15 +1756,11 @@ static int udc_start(struct ci13xxx *ci)
goto put_transceiver;
}
- retval = dbg_create_files(ci);
- if (retval)
- goto unreg_device;
-
if (!IS_ERR_OR_NULL(ci->transceiver)) {
retval = otg_set_peripheral(ci->transceiver->otg,
&ci->gadget);
if (retval)
- goto remove_dbg;
+ goto unreg_device;
}
retval = usb_add_gadget_udc(dev, &ci->gadget);
@@ -1784,8 +1780,6 @@ remove_trans:
}
dev_err(dev, "error = %i\n", retval);
-remove_dbg:
- dbg_remove_files(ci);
unreg_device:
device_unregister(&ci->gadget.dev);
put_transceiver:
@@ -1825,7 +1819,6 @@ static void udc_stop(struct ci13xxx *ci)
if (ci->global_phy)
usb_put_phy(ci->transceiver);
}
- dbg_remove_files(ci);
device_unregister(&ci->gadget.dev);
/* my kobject is dynamic, I swear! */
memset(&ci->gadget, 0, sizeof(ci->gadget));
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 6/9] usb: chipidea: convert debug entries in sysfs to debugfs
Currently, we have a bunch of files in sysfs that display all sorts of
debugging information for the device controller, so they have to move to
debugfs where they belong. The "registers" interface have been removed,
since it doesn't fit into the current driver design as is and it's hardly
a good idea to touch the registers from userspace anyway.
Signed-off-by: Alexander Shishkin
---
drivers/usb/chipidea/ci.h|2 +
drivers/usb/chipidea/debug.c | 469 +-
drivers/usb/chipidea/debug.h |9 +-
drivers/usb/chipidea/udc.c |6 +-
4 files changed, 151 insertions(+), 335 deletions(-)
diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
index f67d101..fc8fc06 100644
--- a/drivers/usb/chipidea/ci.h
+++ b/drivers/usb/chipidea/ci.h
@@ -140,6 +140,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
+ * @debugfs: root dentry for this controller in debugfs
*/
struct ci13xxx {
struct device *dev;
@@ -176,6 +177,7 @@ struct ci13xxx {
boolglobal_phy;
struct usb_phy *transceiver;
struct usb_hcd *hcd;
+ struct dentry *debugfs;
};
static inline struct ci_role_driver *ci_role(struct ci13xxx *ci)
diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c
index 898aca5..057ae09 100644
--- a/drivers/usb/chipidea/debug.c
+++ b/drivers/usb/chipidea/debug.c
@@ -2,6 +2,9 @@
#include
#include
#include
+#include
+#include
+#include
#include
#include
@@ -11,223 +14,113 @@
#include "debug.h"
/**
- * hw_register_read: reads all device registers (execute without interruption)
- * @buf: destination buffer
- * @size: buffer size
- *
- * This function returns number of registers read
+ * ci_device_show: prints information about device capabilities and status
*/
-static size_t hw_register_read(struct ci13xxx *ci, u32 *buf, size_t size)
+static int ci_device_show(struct seq_file *s, void *data)
{
- unsigned i;
-
- if (size > ci->hw_bank.size)
- size = ci->hw_bank.size;
-
- for (i = 0; i < size; i++)
- buf[i] = hw_read(ci, i * sizeof(u32), ~0);
-
- return size;
-}
-
-/**
- * hw_register_write: writes to register
- * @addr: register address
- * @data: register value
- *
- * This function returns an error code
- */
-static int hw_register_write(struct ci13xxx *ci, u16 addr, u32 data)
-{
- /* align */
- addr /= sizeof(u32);
+ struct ci13xxx *ci = s->private;
+ struct usb_gadget *gadget = &ci->gadget;
- if (addr >= ci->hw_bank.size)
- return -EINVAL;
+ seq_printf(s, "speed = %d\n", gadget->speed);
+ seq_printf(s, "max_speed = %d\n", gadget->max_speed);
+ seq_printf(s, "is_otg= %d\n", gadget->is_otg);
+ seq_printf(s, "is_a_peripheral = %d\n", gadget->is_a_peripheral);
+ seq_printf(s, "b_hnp_enable = %d\n", gadget->b_hnp_enable);
+ seq_printf(s, "a_hnp_support = %d\n", gadget->a_hnp_support);
+ seq_printf(s, "a_alt_hnp_support = %d\n", gadget->a_alt_hnp_support);
+ seq_printf(s, "name = %s\n",
+ (gadget->name ? gadget->name : ""));
+
+ if (!ci->driver)
+ return 0;
- /* align */
- addr *= sizeof(u32);
+ seq_printf(s, "gadget function = %s\n",
+ (ci->driver->function ? ci->driver->function : ""));
+ seq_printf(s, "gadget max speed = %d\n", ci->driver->max_speed);
- hw_write(ci, addr, ~0, data);
return 0;
}
-/**
- * hw_intr_clear: disables interrupt & clears interrupt status (execute without
- *interruption)
- * @n: interrupt bit
- *
- * This function returns an error code
- */
-static int hw_intr_clear(struct ci13xxx *ci, int n)
+static int ci_device_open(struct inode *inode, struct file *file)
{
- if (n >= REG_BITS)
- return -EINVAL;
-
- hw_write(ci, OP_USBINTR, BIT(n), 0);
- hw_write(ci, OP_USBSTS, BIT(n), BIT(n));
- return 0;
+ return single_open(file, ci_device_show, inode->i_private);
}
-/**
- * hw_intr_force: enables interrupt & forces interrupt status (execute without
- *interruption)
- * @n: interrupt bit
- *
- * This function returns an error code
- */
-static int hw_intr_force(struct ci13xxx *ci, int n)
-{
- if (n >= REG_BITS)
- return -EINVAL;
-
- hw_write(ci, CAP_TESTMODE, TESTMODE_FORCE, TESTMODE_FORCE);
- hw_write(ci, OP_USBINTR, BIT(n), B
[PATCH 9/9] usb: chipidea: fix precedence bug in ci_requests_show()
From: Dan Carpenter The intent here was to have parenthesis around the (ci->hw_ep_max / 2) so that it counts like "0 1 2 0 1 2". In the current code, the mod operation happens first so it counts like "0 0 1 1 2 2". Signed-off-by: Dan Carpenter [rebased on top of debug.c changes] Signed-off-by: Alexander Shishkin --- drivers/usb/chipidea/debug.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/usb/chipidea/debug.c b/drivers/usb/chipidea/debug.c index 5738079..36a7063 100644 --- a/drivers/usb/chipidea/debug.c +++ b/drivers/usb/chipidea/debug.c @@ -175,7 +175,7 @@ static int ci_requests_show(struct seq_file *s, void *data) req = list_entry(ptr, struct ci13xxx_req, queue); seq_printf(s, "EP=%02i: TD=%08X %s\n", - i % ci->hw_ep_max/2, (u32)req->dma, + i % (ci->hw_ep_max / 2), (u32)req->dma, ((i < ci->hw_ep_max/2) ? "RX" : "TX")); for (j = 0; j < qsize; j++) -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch v2] usb: gadget: precedence bug in show_requests()
Dan Carpenter writes: > The intent here was to have parenthesis around the (ci->hw_ep_max / 2) > so that it counts like "0 1 2 0 1 2". In the current code, the mod > operation happens first so it counts like "0 0 1 1 2 2". > > Signed-off-by: Dan Carpenter Thanks, I added this one on top of my debug cleanup patchset [1]. [1] http://marc.info/?l=linux-usb&m=135902438408860&w=2 Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/9] usb: chipidea: drop redundant includes
Sergei Shtylyov writes: >But you also add some #include's, and don't document this in the changelog. Nice catch, thanks! Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH v5 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect
Peter Chen 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
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_ASVISBIT(18)
> #define OTGSC_BSVISBIT(19)
> #define OTGSC_BSEISBIT(20)
> +#define OTGSC_1MSISBIT(21)
> +#define OTGSC_DPIS BIT(22)
> #define OTGSC_IDIE BIT(24)
> #define OTGSC_AVVIEBIT(25)
> #define OTGSC_ASVIEBIT(26)
> #define OTGSC_BSVIEBIT(27)
> #define OTGSC_BSEIEBIT(28)
> +#define OTGSC_1MSIEBIT(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_rolerole;
> boolis_otg;
> struct work_struct work;
> + struct delayed_work dwork;
> struct workqueue_struct *wq;
>
> struct dma_pool *qh_pool;
> @@ -165,7 +167,9 @@ struct ci13xxx {
> boolglobal_phy;
> struct usb_phy *transceiver;
> struct usb_hcd *hcd;
> - struct usb_otg otg;
> + struct usb_otg otg;
> + boolid_event;
> + boolb_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, 0x, 0);
> + ci_disable_otg_interrupt(ci, OTGSC_INT_EN_BITS);
> +
> + /* Clear all interrupts status bits*/
> + hw_write(ci, OP_USBSTS, 0x, 0x);
> + 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 wa
Re: [RESEND PATCH v5 4/7] usb: chipidea: consolidate ci_role_driver's API for both roles
Peter Chen 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
> ---
> 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);
> -
Re: [RESEND PATCH v5 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect
Peter Chen 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
[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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH v5 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect
Peter Chen writes:
> On Thu, Jan 24, 2013 at 05:25:17PM +0200, Alexander Shishkin wrote:
>> Peter Chen 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
>>
>> [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);
-
Re: [RESEND PATCH v5 4/7] usb: chipidea: consolidate ci_role_driver's API for both roles
Peter Chen writes:
> On Thu, Jan 24, 2013 at 04:35:30PM +0200, Alexander Shishkin wrote:
>> Peter Chen 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
Re: [RESEND PATCH v5 4/7] usb: chipidea: consolidate ci_role_driver's API for both roles
Peter Chen writes: > On Fri, Jan 25, 2013 at 02:12:20PM +0200, Alexander Shishkin wrote: >> Peter Chen writes: >> >> > On Thu, Jan 24, 2013 at 04:35:30PM +0200, Alexander Shishkin wrote: >> >> Peter Chen 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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH,RFC] usb: add devicetree helpers for determining dr_mode and phy_type
Sascha Hauer writes:
> From: Michael Grzeschik
>
> This adds two little devicetree helper functions for determining the
> dr_mode (host, peripheral, otg) and phy_type (utmi, ulpi,...) from
> the devicetree.
>
> Signed-off-by: Michael Grzeschik
> Signed-off-by: Marc Kleine-Budde
> ---
>
> The properties and their values have been taken from the fsl-mph-dr driver.
> This binding is also documented (though currently not used) for the tegra
> ehci driver (Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt).
> This is a first attempt to parse these bindings at a common place so that
> others can make use of it.
>
> Basically I want to know whether this binding is recommended for new drivers
> since normally the devicetree uses '-' instead of '_', and maybe there are
> other problems with it.
>
> I need this binding for the chipidea driver. I suspect that the fsl-mph-dr
> driver also really handles a chipidea core.
As far as I know, it is a chipidea core. Adding Peter to Cc list, he can
probably confirm.
> Should we agree on this I would convert the fsl-mph-dr driver to use these
> helpers.
>
> Sascha
>
> drivers/usb/core/Makefile |1 +
> drivers/usb/core/of.c | 76
> +
> include/linux/usb/of.h| 22 +
> include/linux/usb/phy.h |9 ++
> 4 files changed, 108 insertions(+)
> create mode 100644 drivers/usb/core/of.c
> create mode 100644 include/linux/usb/of.h
>
> diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
> index 26059b9..5378add 100644
> --- a/drivers/usb/core/Makefile
> +++ b/drivers/usb/core/Makefile
> @@ -10,5 +10,6 @@ usbcore-y += devio.o notify.o generic.o quirks.o devices.o
>
> usbcore-$(CONFIG_PCI)+= hcd-pci.o
> usbcore-$(CONFIG_ACPI) += usb-acpi.o
> +usbcore-$(CONFIG_OF) += of.o
>
> obj-$(CONFIG_USB)+= usbcore.o
> diff --git a/drivers/usb/core/of.c b/drivers/usb/core/of.c
> new file mode 100644
> index 000..d000d9f
> --- /dev/null
> +++ b/drivers/usb/core/of.c
> @@ -0,0 +1,76 @@
> +/*
> + * OF helpers for usb devices.
> + *
> + * This file is released under the GPLv2
> + *
> + * Initially copied out of drivers/of/of_net.c
> + */
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +static const char *usbphy_modes[] = {
> + [USBPHY_INTERFACE_MODE_NA] = "",
> + [USBPHY_INTERFACE_MODE_UTMI]= "utmi",
> + [USBPHY_INTERFACE_MODE_UTMIW] = "utmi_wide",
> + [USBPHY_INTERFACE_MODE_ULPI]= "ulpi",
> + [USBPHY_INTERFACE_MODE_SERIAL] = "serial",
> + [USBPHY_INTERFACE_MODE_HSIC]= "hsic",
> +};
> +
> +/**
> + * of_get_usbphy_mode - Get phy mode for given device_node
> + * @np: Pointer to the given device_node
> + *
> + * The function gets phy interface string from property 'phy_type',
> + * and returns the correspondig enum usb_phy_interface
> + */
> +enum usb_phy_interface of_usb_get_phy_mode(struct device_node *np)
> +{
> + const char *phy_type;
> + int err, i;
> +
> + err = of_property_read_string(np, "phy_type", &phy_type);
> + if (err < 0)
> + return USBPHY_INTERFACE_MODE_NA;
> +
> + for (i = 0; i < ARRAY_SIZE(usbphy_modes); i++)
> + if (!strcasecmp(phy_type, usbphy_modes[i]))
> + return i;
> +
> + return USBPHY_INTERFACE_MODE_NA;
> +}
> +EXPORT_SYMBOL_GPL(of_usb_get_phy_mode);
> +
> +static const char *usb_dr_modes[] = {
> + [USB_DR_MODE_UNKNOWN] = "",
> + [USB_DR_MODE_HOST] = "host",
> + [USB_DR_MODE_PERIPHERAL]= "peripheral",
> + [USB_DR_MODE_OTG] = "otg",
> +};
> +
> +/**
> + * of_usb_get_dr_mode - Get dual role mode for given device_node
> + * @np: Pointer to the given device_node
> + *
> + * The function gets phy interface string from property 'dr_mode',
> + * and returns the correspondig enum usb_phy_dr_mode
> + */
> +enum usb_phy_dr_mode of_usb_get_dr_mode(struct device_node *np)
> +{
> + const char *dr_mode;
> + int err, i;
> +
> + err = of_property_read_string(np, "dr_mode", &dr_mode);
> + if (err < 0)
> + return USB_DR_MODE_UNKNOWN;
> +
> + for (i = 0; i < ARRAY_SIZE(usb_dr_modes); i++)
> + if (!strcasecmp(dr_mode, usb_dr_modes[i]))
> + return i;
> +
> + return USB_DR_MODE_UNKNOWN;
> +}
> +EXPORT_SYMBOL_GPL(of_usb_get_dr_mode);
> diff --git a/include/linux/usb/of.h b/include/linux/usb/of.h
> new file mode 100644
> index 000..582ba96
> --- /dev/null
> +++ b/include/linux/usb/of.h
> @@ -0,0 +1,22 @@
> +/*
> + * OF helpers for usb devices.
> + *
> + * This file is released under the GPLv2
> + */
> +
> +#ifndef __LINUX_USB_OF_H
> +#define __LINUX_USB_OF_H
> +
> +#include
> +
> +enum usb_phy_dr_mode {
> + USB_DR_MODE_UNKNOWN,
> + USB_DR_MODE_HOST,
> + USB_DR_MODE_PERIPHERAL,
> + USB_DR_MODE_OTG,
> +};
> +
> +enum usb_phy_interface of_usb_get_
Re: [RESEND PATCH v5 3/7] usb: chipidea: add otg id switch and vbus connect/disconnect detect
Peter Chen writes: > On Wed, Jan 30, 2013 at 11:36:42AM +0530, kishon wrote: >> Hi, >> >> >boolglobal_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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/9] USB: chipidea: add PTW and PTS handling
Michael Grzeschik writes:
> On Fri, Feb 01, 2013 at 08:52:07AM +0100, Sascha Hauer wrote:
>> From: Michael Grzeschik
>>
>> This patch makes it possible to configure the PTW and PTS bits inside
>> the portsc register for host and device mode before the driver starts
>> and the phy can be addressed as hardware implementation is designed.
>>
>> Signed-off-by: Michael Grzeschik
>> Signed-off-by: Marc Kleine-Budde
>> Signed-off-by: Sascha Hauer
>> ---
>> .../devicetree/bindings/usb/ci13xxx-imx.txt|5 +++
>> drivers/usb/chipidea/bits.h| 14 ++-
>> drivers/usb/chipidea/ci13xxx_imx.c |3 ++
>> drivers/usb/chipidea/core.c| 39
>>
>> include/linux/usb/chipidea.h |1 +
>> 5 files changed, 61 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>> b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>> index 5778b9c..dd42ccd 100644
>> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
>> @@ -5,6 +5,11 @@ Required properties:
>> - reg: Should contain registers location and length
>> - interrupts: Should contain controller interrupt
>>
>> +Recommended properies:
>> +- phy_type: the type of the phy connected to the core. Should be one
>> + of "utmi", "utmi_wide", "ulpi", "serial" or "hsic". Without this
>> + property the PORTSC register won't be touched
>> +
>> Optional properties:
>> - fsl,usbphy: phandler of usb phy that connects to the only one port
>> - fsl,usbmisc: phandler of non-core register device, with one argument
>> diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
>> index 050de85..d8ffc2f 100644
>> --- a/drivers/usb/chipidea/bits.h
>> +++ b/drivers/usb/chipidea/bits.h
>> @@ -48,10 +48,22 @@
>> #define PORTSC_SUSP BIT(7)
>> #define PORTSC_HSPBIT(9)
>> #define PORTSC_PTC(0x0FUL << 16)
>> +/* PTS and PTW for non lpm version only */
>> +#define PORTSC_PTS(d) d) & 0x3) << 30) | (((d) & 0x4) ? BIT(25)
>> : 0))
>> +#define PORTSC_PTWBIT(28)
>>
>> /* DEVLC */
>> #define DEVLC_PSPD(0x03UL << 25)
>> -#defineDEVLC_PSPD_HS (0x02UL << 25)
>> +#define DEVLC_PSPD_HS (0x02UL << 25)
>> +#define DEVLC_PTW BIT(27)
>> +#define DEVLC_STS BIT(28)
>> +#define DEVLC_PTS(d) (((d) & 0x7) << 29)
>> +
>> +/* Encoding for DEVLC_PTS and PORTSC_PTS */
>> +#define PTS_UTMI 0
>> +#define PTS_ULPI 2
>> +#define PTS_SERIAL3
>> +#define PTS_HSIC 4
>>
>> /* OTGSC */
>> #define OTGSC_IDPUBIT(5)
>> diff --git a/drivers/usb/chipidea/ci13xxx_imx.c
>> b/drivers/usb/chipidea/ci13xxx_imx.c
>> index 69024e0..ebc1148 100644
>> --- a/drivers/usb/chipidea/ci13xxx_imx.c
>> +++ b/drivers/usb/chipidea/ci13xxx_imx.c
>> @@ -21,6 +21,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include "ci.h"
>> #include "ci13xxx_imx.h"
>> @@ -112,6 +113,8 @@ static int ci13xxx_imx_probe(struct platform_device
>> *pdev)
>> CI13XXX_PULLUP_ON_VBUS |
>> CI13XXX_DISABLE_STREAMING;
>>
>> +pdata->phy_mode = of_usb_get_phy_mode(pdev->dev.of_node);
>> +
>> data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> if (!data) {
>> dev_err(&pdev->dev, "Failed to allocate CI13xxx-IMX data!\n");
>> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
>> index 57cae1f..a3ec29d 100644
>> --- a/drivers/usb/chipidea/core.c
>> +++ b/drivers/usb/chipidea/core.c
>> @@ -67,6 +67,8 @@
>> #include
>> #include
>> #include
>> +#include
>> +#include
>>
>> #include "ci.h"
>> #include "udc.h"
>> @@ -211,6 +213,41 @@ static int hw_device_init(struct ci13xxx *ci, void
>> __iomem *base)
>> return 0;
>> }
>>
>> +static void hw_phymode_configure(struct ci13xxx *ci)
>> +{
>> +u32 portsc, lpm;
>> +
>> +switch (ci->platdata->phy_mode) {
>> +case USBPHY_INTERFACE_MODE_UTMI:
>> +portsc = PORTSC_PTS(PTS_UTMI);
>> +lpm = DEVLC_PTS(PTS_UTMI);
>> +break;
>> +case USBPHY_INTERFACE_MODE_UTMIW:
>> +portsc = PORTSC_PTS(PTS_UTMI) | PORTSC_PTW;
>> +lpm = DEVLC_PTS(PTS_UTMI) | DEVLC_PTW;
>> +break;
>> +case USBPHY_INTERFACE_MODE_ULPI:
>> +portsc = PORTSC_PTS(PTS_ULPI);
>> +lpm = DEVLC_PTS(PTS_ULPI);
>> +break;
>> +case USBPHY_INTERFACE_MODE_SERIAL:
>> +portsc = PORTSC_PTS(PTS_SERIAL);
>> +lpm = DEVLC_PTS(PTS_SERIAL);
>> +break;
>> +case USBPHY_INTERFACE_MODE_HSIC:
>> +portsc = PORTSC_PTS(PTS_HSIC);
>> +lpm = DEVLC_PTS(PTS_HSIC);
>> +break;
>> +default:
>> +return;
>> +}
>> +
>> +
Re: [PATCH 3/9] USB: add devicetree helpers for determining dr_mode and phy_type
Sascha Hauer writes:
> From: Michael Grzeschik
>
> This adds two little devicetree helper functions for determining the
> dr_mode (host, peripheral, otg) and phy_type (utmi, ulpi,...) from
> the devicetree.
>
> Signed-off-by: Michael Grzeschik
> Signed-off-by: Marc Kleine-Budde
> Signed-off-by: Sascha Hauer
> ---
> drivers/usb/phy/Makefile |1 +
> drivers/usb/phy/of.c | 47
> ++
> drivers/usb/usb-common.c | 36 +++
> include/linux/usb/of.h | 27 ++
> include/linux/usb/otg.h |7 +++
> include/linux/usb/phy.h |9 +
> 6 files changed, 127 insertions(+)
> create mode 100644 drivers/usb/phy/of.c
> create mode 100644 include/linux/usb/of.h
>
> diff --git a/drivers/usb/phy/Makefile b/drivers/usb/phy/Makefile
> index 9fa6327..e1be1e8 100644
> --- a/drivers/usb/phy/Makefile
> +++ b/drivers/usb/phy/Makefile
> @@ -5,6 +5,7 @@
> ccflags-$(CONFIG_USB_DEBUG) := -DDEBUG
>
> obj-$(CONFIG_USB_OTG_UTILS) += phy.o
> +obj-$(CONFIG_OF) += of.o
> obj-$(CONFIG_OMAP_USB2) += omap-usb2.o
> obj-$(CONFIG_OMAP_USB3) += omap-usb3.o
> obj-$(CONFIG_OMAP_CONTROL_USB) += omap-control-usb.o
> diff --git a/drivers/usb/phy/of.c b/drivers/usb/phy/of.c
> new file mode 100644
> index 000..e6f3b74
> --- /dev/null
> +++ b/drivers/usb/phy/of.c
> @@ -0,0 +1,47 @@
> +/*
> + * USB of helper code
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +static const char *usbphy_modes[] = {
> + [USBPHY_INTERFACE_MODE_UNKNOWN] = "",
> + [USBPHY_INTERFACE_MODE_UTMI]= "utmi",
> + [USBPHY_INTERFACE_MODE_UTMIW] = "utmi_wide",
> + [USBPHY_INTERFACE_MODE_ULPI]= "ulpi",
> + [USBPHY_INTERFACE_MODE_SERIAL] = "serial",
> + [USBPHY_INTERFACE_MODE_HSIC]= "hsic",
> +};
> +
> +/**
> + * of_usb_get_phy_mode - Get phy mode for given device_node
> + * @np: Pointer to the given device_node
> + *
> + * The function gets phy interface string from property 'phy_type',
> + * and returns the correspondig enum usb_phy_interface
> + */
> +enum usb_phy_interface of_usb_get_phy_mode(struct device_node *np)
> +{
> + const char *phy_type;
> + int err, i;
> +
> + err = of_property_read_string(np, "phy_type", &phy_type);
> + if (err < 0)
> + return USBPHY_INTERFACE_MODE_UNKNOWN;
> +
> + for (i = 0; i < ARRAY_SIZE(usbphy_modes); i++)
> + if (!strcmp(phy_type, usbphy_modes[i]))
> + return i;
> +
> + return USBPHY_INTERFACE_MODE_UNKNOWN;
> +}
> +EXPORT_SYMBOL_GPL(of_usb_get_phy_mode);
> diff --git a/drivers/usb/usb-common.c b/drivers/usb/usb-common.c
> index d29503e..ad4d87d 100644
> --- a/drivers/usb/usb-common.c
> +++ b/drivers/usb/usb-common.c
> @@ -14,6 +14,9 @@
> #include
> #include
> #include
> +#include
> +#include
> +#include
>
> const char *usb_speed_string(enum usb_device_speed speed)
> {
> @@ -32,4 +35,37 @@ const char *usb_speed_string(enum usb_device_speed speed)
> }
> EXPORT_SYMBOL_GPL(usb_speed_string);
>
> +#ifdef CONFIG_OF
> +static const char *usb_dr_modes[] = {
> + [USB_DR_MODE_UNKNOWN] = "",
> + [USB_DR_MODE_HOST] = "host",
> + [USB_DR_MODE_PERIPHERAL]= "peripheral",
> + [USB_DR_MODE_OTG] = "otg",
> +};
It turns out this is a problem, especially since this is generic usb
code: we have a chipidea controller (a patchset just arrived) that does
both host and peripheral, but not otg. And I'm told now that dwc3
controller can be synthesized like that too.
Summoning Felipe to the thread, this patch is largely his call anyway.
Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] usb: chipidea: Fix incorrect check of function return value
Svetoslav Neykov writes: > Use the correct variable to check for the return value of the last function. This one is fixed already by Julia Lawall: 5c6e9bf0 Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/5] usb: chipidea: AR933x platform support for the chipidea driver
Svetoslav Neykov writes:
> Support host and device usb modes for the chipidea controller in AR933x.
> The controller doesn't support OTG functionality so the platform code
> forces one of the modes based on the state of GPIO13 pin at startup.
Some comments below.
>
> Signed-off-by: Svetoslav Neykov
> ---
> arch/mips/ath79/dev-usb.c | 19 ++
> arch/mips/include/asm/mach-ath79/ar71xx_regs.h |3 +
> drivers/usb/chipidea/Makefile |1 +
> drivers/usb/chipidea/ci13xxx_ar933x.c | 73
>
> 4 files changed, 96 insertions(+)
> create mode 100644 drivers/usb/chipidea/ci13xxx_ar933x.c
>
> diff --git a/arch/mips/ath79/dev-usb.c b/arch/mips/ath79/dev-usb.c
> index bd2bc10..52966b3 100644
> --- a/arch/mips/ath79/dev-usb.c
> +++ b/arch/mips/ath79/dev-usb.c
> @@ -174,8 +174,27 @@ static void __init ar913x_usb_setup(void)
> platform_device_register(&ath79_ehci_device);
> }
>
> +static void __init ar933x_usb_setup_ctrl_config(void)
> +{
> + void __iomem *usb_ctrl_base, *usb_config_reg;
> + u32 usb_config;
> +
> + usb_ctrl_base = ioremap(AR71XX_USB_CTRL_BASE, AR71XX_USB_CTRL_SIZE);
> + usb_config_reg = usb_ctrl_base + AR71XX_USB_CTRL_REG_CONFIG;
> + usb_config = __raw_readl(usb_config_reg);
> + usb_config &= ~AR933X_USB_CONFIG_HOST_ONLY;
> + __raw_writel(usb_config, usb_config_reg);
> + iounmap(usb_ctrl_base);
> +}
> +
> static void __init ar933x_usb_setup(void)
> {
> + u32 bootstrap;
> +
> + bootstrap = ath79_reset_rr(AR933X_RESET_REG_BOOTSTRAP);
> + if (!(bootstrap & AR933X_BOOTSTRAP_USB_MODE_HOST))
> + ar933x_usb_setup_ctrl_config();
> +
> ath79_device_reset_set(AR933X_RESET_USBSUS_OVERRIDE);
> mdelay(10);
>
> diff --git a/arch/mips/include/asm/mach-ath79/ar71xx_regs.h
> b/arch/mips/include/asm/mach-ath79/ar71xx_regs.h
> index a5e0f17..13eb2d9 100644
> --- a/arch/mips/include/asm/mach-ath79/ar71xx_regs.h
> +++ b/arch/mips/include/asm/mach-ath79/ar71xx_regs.h
> @@ -297,6 +297,7 @@
> #define AR934X_RESET_USB_PHY BIT(4)
> #define AR934X_RESET_USBSUS_OVERRIDE BIT(3)
>
> +#define AR933X_BOOTSTRAP_USB_MODE_HOST BIT(3)
> #define AR933X_BOOTSTRAP_REF_CLK_40 BIT(0)
>
> #define AR934X_BOOTSTRAP_SW_OPTION8 BIT(23)
> @@ -315,6 +316,8 @@
> #define AR934X_BOOTSTRAP_SDRAM_DISABLED BIT(1)
> #define AR934X_BOOTSTRAP_DDR1BIT(0)
>
> +#define AR933X_USB_CONFIG_HOST_ONLY BIT(8)
> +
> #define AR934X_PCIE_WMAC_INT_WMAC_MISC BIT(0)
> #define AR934X_PCIE_WMAC_INT_WMAC_TX BIT(1)
> #define AR934X_PCIE_WMAC_INT_WMAC_RXLP BIT(2)
> diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
> index d92ca32..196b7b4 100644
> --- a/drivers/usb/chipidea/Makefile
> +++ b/drivers/usb/chipidea/Makefile
> @@ -10,6 +10,7 @@ ci_hdrc-$(CONFIG_USB_CHIPIDEA_DEBUG)+= debug.o
> # Glue/Bridge layers go here
>
> obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_msm.o
> +obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_ar933x.o
The problem is that this will only compile for mips target, so you
should probably change the Makefile to only compile it for mips.
>
> # PCI doesn't provide stubs, need to check
> ifneq ($(CONFIG_PCI),)
> diff --git a/drivers/usb/chipidea/ci13xxx_ar933x.c
> b/drivers/usb/chipidea/ci13xxx_ar933x.c
> new file mode 100644
> index 000..046a4b6
> --- /dev/null
> +++ b/drivers/usb/chipidea/ci13xxx_ar933x.c
> @@ -0,0 +1,73 @@
> +/* Copyright (c) 2010, Code Aurora Forum. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 and
> + * only version 2 as published by the Free Software Foundation.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "ci.h"
> +
> +static struct ci13xxx_platform_data ci13xxx_ar933x_platdata = {
> + .name = "ci13xxx_ar933x",
> + .flags = 0,
> + .capoffset = DEF_CAPOFFSET
> +};
> +
> +static int __devinit ci13xxx_ar933x_probe(struct platform_device *pdev)
__devinit/__devexit/__devexit_p() are gone from the kernel.
Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] usb: chipidea: Don't access OTG related registers
Svetoslav Neykov writes:
> According to the datasheet the chipidea controller in AR933x doesn't expose
> OTG and TEST registers.
> If no OTG support is detected don't call functions which access those
> registers.
>
> Signed-off-by: Svetoslav Neykov
> ---
> drivers/usb/chipidea/udc.c | 24
> 1 file changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index 78ac5e5..9fda4d8 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -1395,7 +1395,10 @@ 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);
> +
> + if (ci->is_otg)
> + hw_enable_vbus_intr(ci);
It might be easier on the eyes if you move the "if (ci->is_otg)" check
inside hw_enable_vbus_intr(). But otherwise looks sensible.
Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 0/8] Add tested id switch and vbus connect detect support for Chipidea
Peter Chen writes: > Changes for v7: > - Add ci_supports_gadget helper to know if the controller > supports gadget, if the controller supports gadget, it > needs to read otgsc to know the vbus value, basically, > if the controller supports gadget, it will support host > as well [3/8] > - At ci_hdrc_probe, it needs to add free memory at error path > [3/8] > - Cosolidate ci->driver = NULL at ci13xxx_stop > [8/8] Michael, Marc, could you give your Tested-by's and/or Reviewed-by's for this one? Thanks, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v8 3/8] usb: chipidea: add otg id switch and vbus connect/disconnect detect
Peter Chen 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.
>
> - vbus interrupt needs to be handled when gadget function is enabled
Please find some comments below.
>
> Signed-off-by: Peter Chen
>
> diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
> index d8ffc2f..58ef56c 100644
> --- a/drivers/usb/chipidea/bits.h
> +++ b/drivers/usb/chipidea/bits.h
> @@ -77,11 +77,21 @@
> #define OTGSC_ASVISBIT(18)
> #define OTGSC_BSVISBIT(19)
> #define OTGSC_BSEISBIT(20)
> +#define OTGSC_1MSISBIT(21)
> +#define OTGSC_DPIS BIT(22)
> #define OTGSC_IDIE BIT(24)
> #define OTGSC_AVVIEBIT(25)
> #define OTGSC_ASVIEBIT(26)
> #define OTGSC_BSVIEBIT(27)
> #define OTGSC_BSEIEBIT(28)
> +#define OTGSC_1MSIEBIT(29)
> +#define OTGSC_DPIE BIT(30)
> +#define OTGSC_INT_EN_BITS(OTGSC_IDIE | OTGSC_AVVIE | OTGSC_ASVIE \
> + | OTGSC_BSVIE | OTGSC_BSEIE | OTGSC_1MSIE \
> + | OTGSC_DPIE)
> +#define OTGSC_INT_STATUS_BITS(OTGSC_IDIS | OTGSC_AVVIS | OTGSC_ASVIS
> \
> + | OTGSC_BSVIS | OTGSC_BSEIS | OTGSC_1MSIS \
> + | OTGSC_DPIS)
>
> /* USBMODE */
> #define USBMODE_CM(0x03UL << 0)
> diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> index 697e369..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
Should be id_event and b_sess_valid_event.
> */
> struct ci13xxx {
> struct device *dev;
> @@ -140,6 +141,7 @@ struct ci13xxx {
> enum ci_rolerole;
> boolis_otg;
> struct work_struct work;
> + struct delayed_work dwork;
> struct workqueue_struct *wq;
>
> struct dma_pool *qh_pool;
> @@ -166,6 +168,8 @@ struct ci13xxx {
> struct usb_phy *transceiver;
> struct usb_hcd *hcd;
> struct usb_otg otg;
> + boolid_event;
> + boolb_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/ci13xxx_imx.c
> b/drivers/usb/chipidea/ci13xxx_imx.c
> index 136869b..793fdba 100644
> --- a/drivers/usb/chipidea/ci13xxx_imx.c
> +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> @@ -110,7 +110,8 @@ static int ci13xxx_imx_probe(struct platform_device *pdev)
> pdata->capoffset = DEF_CAPOFFSET;
> pdata->flags = CI13XXX_REQUIRE_TRANSCEIVER |
> CI13XXX_PULLUP_ON_VBUS |
> -CI13XXX_DISABLE_STREAMING;
> +CI13XXX_DISABLE_STREAMING |
> +CI13XXX_REGS_SHARED;
>
> pdata->phy_mode = of_usb_get_phy_mode(pdev->dev.of_node);
> pdata->dr_mode = of_usb_get_dr_mode(pdev->dev.of_node);
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index c89f2aa..fbb6984 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -75,6 +75,7 @@
> #include "bits.h"
> #include "host.h"
> #include "debug.h"
> +#include "otg.h"
>
> /* Controller register map */
> static uintptr_t ci_regs_nolpm[] = {
> @@ -201,6 +202,14 @@ static int hw_device_init(struct ci13xxx *ci, void
> __iomem *base)
> if (ci->hw_ep_max > ENDPT_MAX)
> return -ENODEV;
>
> + /* Disable all interrupts bits */
> + hw_write(ci, OP_USBINTR, 0x, 0);
> + ci_disable_otg_interrupt(ci, OTGSC_INT_EN_BITS);
> +
> + /* Clear all interrupts status bits*/
> + hw_write(ci, OP_USBSTS, 0x, 0x);
> + ci_clear_otg_interrupt(ci, OTGSC_INT_STATUS_BITS);
> +
Re: [PATCH 5/9] USB: chipidea: add PTW and PTS handling
Sascha Hauer writes: > From: Michael Grzeschik > > This patch makes it possible to configure the PTW and PTS bits inside > the portsc register for host and device mode before the driver starts > and the phy can be addressed as hardware implementation is designed. > > Signed-off-by: Michael Grzeschik > Signed-off-by: Marc Kleine-Budde > Signed-off-by: Sascha Hauer > --- > .../devicetree/bindings/usb/ci13xxx-imx.txt|5 +++ > drivers/usb/chipidea/bits.h| 14 ++- > drivers/usb/chipidea/ci13xxx_imx.c |3 ++ > drivers/usb/chipidea/core.c| 39 > > include/linux/usb/chipidea.h |1 + > 5 files changed, 61 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt > b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt > index 5778b9c..dd42ccd 100644 > --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt > +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt > @@ -5,6 +5,11 @@ Required properties: > - reg: Should contain registers location and length > - interrupts: Should contain controller interrupt > > +Recommended properies: > +- phy_type: the type of the phy connected to the core. Should be one > + of "utmi", "utmi_wide", "ulpi", "serial" or "hsic". Without this > + property the PORTSC register won't be touched > + Looks like this bit belongs to patch 3/9, where you're adding devicetree hooks. Otherwise looks good. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4] USB: add devicetree helpers for determining dr_mode and phy_type
Sascha Hauer writes: > 4th round of patches. > > Peter, I would be glad if you could test them before your holiday. I rebased > your last round of Chipidea OTG patches onto this series which you can pull > here: > > git://git.pengutronix.de/git/imx/linux-2.6.git tags/usb-chipidea-otg-for-next > > I couldn't really test the otg patches since my current hardware does not have > the ID pin connected, but I can verify that my usecase still works with your > patches applied. > > Alex, should the patches work for you and are fine otherwise, could you apply > them for v3.9? Looks good in general. I just realised the comment I made on 5/9 is not entirely valid, but the dr_mode setting needs one extra option as was discussed. Also, all the phy/otg patches (1/9, 2/9, 3/9, 8/9) need Felipe's ack, then I can apply the series. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] USB: chipidea: fix & vs | bug
Dan Carpenter writes: > On Fri, Feb 15, 2013 at 01:26:29PM +0300, Dan Carpenter wrote: >> On Fri, Sep 14, 2012 at 02:58:20PM +0800, Richard Zhao wrote: >> > On Fri, Sep 14, 2012 at 09:50:56AM +0300, Dan Carpenter wrote: >> > > There is a '&' vs '|' typo in the original code so the condition is >> > > never true and we don't queue the work. >> > > >> > > Signed-off-by: Dan Carpenter >> > > --- >> > > >> > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c >> > > index 2f45bba..5294f81 100644 >> > > --- a/drivers/usb/chipidea/udc.c >> > > +++ b/drivers/usb/chipidea/udc.c >> > > @@ -1680,7 +1680,7 @@ static irqreturn_t udc_irq(struct ci13xxx *ci) >> > > intr = hw_read(ci, OP_OTGSC, ~0); >> > > hw_write(ci, OP_OTGSC, ~0, intr); >> > > >> > > -if (intr & (OTGSC_AVVIE & OTGSC_AVVIS)) >> > > +if (intr & (OTGSC_AVVIE | OTGSC_AVVIS)) >> > It's not what I meant. Should be >> > if ((intr & OTGSC_AVVIE) && (intr & OTGSC_AVVIS)) >> > I'm still testing it. >> >> How did the testing go? >> > > Richard's email is bouncing. Probably I should just send this > myself. We're reverting this commit for 3.10 as part of Peter's vbus detection patchset. If it seems more urgent, I guess we can revert it earlier as a bugfix. Otherwise, I'd just leave it till 3.10. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch] USB: chipidea: fix & vs | bug
Dan Carpenter writes: > On Fri, Feb 15, 2013 at 02:54:56PM +0200, Alexander Shishkin wrote: >> Dan Carpenter writes: >> >> > On Fri, Feb 15, 2013 at 01:26:29PM +0300, Dan Carpenter wrote: >> >> On Fri, Sep 14, 2012 at 02:58:20PM +0800, Richard Zhao wrote: >> >> > On Fri, Sep 14, 2012 at 09:50:56AM +0300, Dan Carpenter wrote: >> >> > > There is a '&' vs '|' typo in the original code so the condition is >> >> > > never true and we don't queue the work. >> >> > > >> >> > > Signed-off-by: Dan Carpenter >> >> > > --- >> >> > > >> >> > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c >> >> > > index 2f45bba..5294f81 100644 >> >> > > --- a/drivers/usb/chipidea/udc.c >> >> > > +++ b/drivers/usb/chipidea/udc.c >> >> > > @@ -1680,7 +1680,7 @@ static irqreturn_t udc_irq(struct ci13xxx *ci) >> >> > > intr = hw_read(ci, OP_OTGSC, ~0); >> >> > > hw_write(ci, OP_OTGSC, ~0, intr); >> >> > > >> >> > > - if (intr & (OTGSC_AVVIE & OTGSC_AVVIS)) >> >> > > + if (intr & (OTGSC_AVVIE | OTGSC_AVVIS)) >> >> > It's not what I meant. Should be >> >> > if ((intr & OTGSC_AVVIE) && (intr & OTGSC_AVVIS)) >> >> > I'm still testing it. >> >> >> >> How did the testing go? >> >> >> > >> > Richard's email is bouncing. Probably I should just send this >> > myself. >> >> We're reverting this commit for 3.10 as part of Peter's vbus detection >> patchset. If it seems more urgent, I guess we can revert it earlier as a >> bugfix. Otherwise, I'd just leave it till 3.10. > > No rush. It's just a static checker thing. Ok, thanks. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: About chipidea tree
Peter Chen writes: > Hi Alex, Hi, > Do we have a chipidea repo which is queued for mainline? > We have several patchsets for chipidea these monthes, I > don't know their status. For me, I would like based > on your tree if it exists. I thought about it, but then it seems like having a separate branch is bound to be confusing to most people. I'd much rather prefer everything go to usb-next, and this is my current plan. Since Greg will start applying new stuff to usb-next after -rc1 is tagged, I'll send my current stash of patches for inclusion then. If your patchset, for example, has conflicts with my stuff that's not merged, I'll try to take care of resolving the conflicts and submit everything to Greg. In other words, it should be always ok to base your chipidea patchsets on usb-next. Let me know if this sounds reasonable to you. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: About chipidea tree
Greg KH writes: > On Thu, Feb 21, 2013 at 05:12:45PM +0800, Peter Chen wrote: >> Hi Alex, >> >> Do we have a chipidea repo which is queued for mainline? >> We have several patchsets for chipidea these monthes, I >> don't know their status. For me, I would like based >> on your tree if it exists. > > Yeah, I would like to know what is going on here as well. I've seen a > ton of chipidea patches floating around, but none seem to ever make it > to me. Any thoughts as to how to resolve this? I did miss the merge window for 3.9, that's a fact. It is my current intention to send all the pending patches that I have stacked and that have been reviewed to you after -rc1 is tagged, because right now you probably won't apply them to usb-next anyway. There are a few patchsets floating around that still require more review and potentially more work, but with any luck we can have those sorted out by the next merge window. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: About chipidea tree
Marc Kleine-Budde writes: > On 02/26/2013 11:56 AM, Alexander Shishkin wrote: >> Peter Chen writes: >> >>> Hi Alex, >> >> Hi, >> >>> Do we have a chipidea repo which is queued for mainline? >>> We have several patchsets for chipidea these monthes, I >>> don't know their status. For me, I would like based >>> on your tree if it exists. >> >> I thought about it, but then it seems like having a separate branch is >> bound to be confusing to most people. I'd much rather prefer everything >> go to usb-next, and this is my current plan. Since Greg will start >> applying new stuff to usb-next after -rc1 is tagged, I'll send my >> current stash of patches for inclusion then. If your patchset, for > > Can we have a look at your queued patches? Sure, http://marc.info/?l=linux-usb&m=135902434508839 >> example, has conflicts with my stuff that's not merged, I'll try to take >> care of resolving the conflicts and submit everything to Greg. In other >> words, it should be always ok to base your chipidea patchsets on >> usb-next. >> >> Let me know if this sounds reasonable to you. > > Michael has already done that work (some S-o-b form Michael are missing) > and rebased Sascha's and Peter's patches to current linus/master, see > this tree : > > http://git.pengutronix.de/?p=mgr/linux.git;a=shortlog;h=refs/heads/chipidea-for-v3.10 Taking a quick look, quite some of those patches are not ready for inclusion yet. So if the question is, do we need a -next tree for all the chipidea patchsets floating around, it might be a good thing. But it's not what Peter was asking in the first place. > The tree includes my patch we resent yesterday, as Sascha's series > depends on this. Great, I'll take a look. Thanks, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: chipidea: register debugging sysfs on our device
Felipe Balbi writes: > Don't register anything non-generic under > the gadget's device as we don't really *own* > it. > > Signed-off-by: Felipe Balbi > --- > > Hi Alex, > > please take a look at this patch, if you don't reply to it > I will take it through my tree for this -rc cycle. I have sent > few weeks ago and didn't get a review. Looks ok. Have you seen my debug cleanup patches, in particular [1]? That patchset removes everything from sysfs and takes some of those bits to debugfs instead. I'm going to send it to Greg after -rc1, unless you have objections. [1] http://marc.info/?l=linux-usb&m=135902438108859&w=2 Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 0/5] Chipidea driver support for the AR933x platform
Svetoslav Neykov writes: > Add support for the usb controller in AR933x platform. > The processor is big-endian so all multi-byte values of the usb > descriptors must be converted explicitly. Another difference is that > the controller supports both host and device modes but not OTG. > The patches are tested on WR703n router running OpenWRT trunk. Svetoslav, are you working on these or do you have time to address the issues raised in review? Thanks, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 0/5] Chipidea driver support for the AR933x platform
Svetoslav Neykov writes: > Hi Alex, > I am working on the incorporating all received comments - thanks to all for > taking their time to comment. > Apologies for not replying to the received mails, didn't want to spam with > OK replies to each separately. Great, thanks! Looks like this patchset will need some synchronization with Sacha's dr_mode/phy_mode patchset, but I presume you're aware of that. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: About chipidea tree
Greg KH writes: > On Tue, Feb 26, 2013 at 01:01:48PM +0200, Alexander Shishkin wrote: >> Greg KH writes: >> >> > On Thu, Feb 21, 2013 at 05:12:45PM +0800, Peter Chen wrote: >> >> Hi Alex, >> >> >> >> Do we have a chipidea repo which is queued for mainline? >> >> We have several patchsets for chipidea these monthes, I >> >> don't know their status. For me, I would like based >> >> on your tree if it exists. >> > >> > Yeah, I would like to know what is going on here as well. I've seen a >> > ton of chipidea patches floating around, but none seem to ever make it >> > to me. Any thoughts as to how to resolve this? >> >> I did miss the merge window for 3.9, that's a fact. > > I think you also missed 3.8, right? I don't think we had anything that was ready for inclusion at that moment besides a few bugfixes which made it to 3.8. >> It is my current intention to send all the pending patches that I have >> stacked and that have been reviewed to you after -rc1 is tagged, >> because right now you probably won't apply them to usb-next anyway. > > That is correct, I just want to make sure this doesn't happen again for > 3.10. Yes, we definitely don't want that. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 8/9] usb: chipidea: tell platform layer the ci core probe's result
Felipe Balbi writes: > Hi, > > On Thu, Feb 28, 2013 at 04:31:44PM +0800, Peter Chen wrote: >> > > > > > > If the probe fails, the ci13xxx_add_device will not return error, >> > > > > > > (bus_probe_device doesn't has return value) >> > > > > > > therefore, the platform layer can't know whether core's probe >> > > > > > > is successful or not, if platform layer goes on using core's >> > > > > > > struct >> > > > > > > which is initialized at core's probe, the error will occur. >> > > > > > > >> > > > > > > This error is showed when I only compile gadget, the host-only >> > > > > > > controller reports "no supported roles", and fails probe, but imx >> > > > > > > platform code doesn't know it, and goes on using core's private >> > > > > > > data. >> > > > > > > >> > > > > > > Signed-off-by: Peter Chen >> > > > > > >> > > > > > this just tells you that platform code shouldn't be using the >> > > > > > driver >> > > > > > directly. passing probe_retval via platform_data is an >> > > > > > abomination, fix >> > > > > > the real problem instead, whatever it is. >> > > > > >> > > > > So you suggest the platform glue layer should not use core driver's >> > > > > data >> > > > > directly, eg, for your dwc3, the platform glue layer should not use >> > > > > struct dwc3 *dwc directly? >> > > > >> > > > yes, and it doesn't. Ever. >> > > > >> > > >> > > If the dwc3 core fails to probe, but controller core clk is still on, is >> > > it >> > > a valid case? >> > >> > of course not, but then again, core clk shouldn't be handled by glue >> > layer. You need to figure out who owns the clock, if it feeds DWC3 why >> > would you clk_get() and clk_prepare_enable() from glue ? Makes no sense. >> > >> >> Sorry? I can't find clk_prepare_enable at dwc3/core.c, but at dwc3 core, it >> try to access register at probe, unless platform layer open the clock, how >> can the core visit the core register. > > Is it really this difficult to figure out ? Fair enough, below are all > the details: > > To understand the reason why dwc3/core.c doesn't know about struct clk, > you need to consider where the driver was originally written; it was > written on an OMAP platform (actually first on a virtual model OMAP - > somewhat like QEMU -, than on a PCIe FPGA prototype of the IP, then > ARM-based FPGA prototype, then OMAP5, none of which needed explicit > clock control, see below). > > OMAP's PM is written in such a way that a pm_runtime_get() will enable > the device the all clocks necessary to be usable. Since OMAP would never > need to use clocks directly and I would never be able to test that code, > I decided not to add it. > > Now, if dwc3-exynos needs it, the sane thing to do would be add struct > clk knowledge to dwc3/core.c but make it optional. If there are no > clocks available, don't bail out. I'm not too familiar with the multitudes of platforms out there, but my simple question is: why can't we have pm runtime take care of enabling/disabling the clocks so that we don't have to do it in drivers? Seems obvious that a platform/SoC/board should know about it's clock tree structure, so why doesn't the platform code then take care of all the dirty details? It seems totally unreasonable and messy to add notion of clocks to drivers just because some platforms can't get their PM right. > Just because dwc3/core.c doesn't know about clocks, it doesn't mean it's > correct to hack it into the glue layer if that doesn't need the clock. > > Now that we know that's a bug, who's going to send me tested patches to > teach dwc3/core.c about struct clk in a way that doesn't break PCIe, nor > OMAP5 ? So are you sure that's what you want? Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] USB mxs-phy: Register phy with framework
Marc Kleine-Budde writes:
> On 02/28/2013 09:01 AM, Felipe Balbi wrote:
>> hi,
>>
>> On Wed, Feb 27, 2013 at 03:16:30PM +0100, Marc Kleine-Budde wrote:
>>> From: Sascha Hauer
>>>
>>> We now have usb_add_phy_dev(), so use it to register with the framework
>>> to be able to find the phy from the USB driver.
>>>
>>> Reviewed-by: Kishon Vijay Abraham I
>>> Reviewed-by: Peter Chen
>>> Acked-by: Felipe Balbi
>>> Signed-off-by: Sascha Hauer
>>> Signed-off-by: Marc Kleine-Budde
>>
>> any chance you can move away from {write,read}[bwl]_relaxed() so we can
>> build this driver on other architectures ?
>
> The hardware is in the ARM imx2{2,8} only. Another option would be to
> add an "depends on ARCH_ARM".
Doesn't mean that we shouldn't be able to compile-test the driver.
Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: About chipidea tree
Peter Chen writes: > On Tue, Feb 26, 2013 at 02:47:34PM +0100, Marc Kleine-Budde wrote: >> On 02/26/2013 02:25 PM, Alexander Shishkin wrote: >> > Marc Kleine-Budde writes: >> > >> >> On 02/26/2013 11:56 AM, Alexander Shishkin wrote: >> >>> Peter Chen writes: >> >>> >> >>>> Hi Alex, >> >>> >> >>> Hi, >> >>> >> >>>> Do we have a chipidea repo which is queued for mainline? >> >>>> We have several patchsets for chipidea these monthes, I >> >>>> don't know their status. For me, I would like based >> >>>> on your tree if it exists. >> >>> >> >>> I thought about it, but then it seems like having a separate branch is >> >>> bound to be confusing to most people. I'd much rather prefer everything >> >>> go to usb-next, and this is my current plan. Since Greg will start >> >>> applying new stuff to usb-next after -rc1 is tagged, I'll send my >> >>> current stash of patches for inclusion then. If your patchset, for >> >> >> >> Can we have a look at your queued patches? >> > >> > Sure, >> > http://marc.info/?l=linux-usb&m=135902434508839 >> > >> >>> example, has conflicts with my stuff that's not merged, I'll try to take >> >>> care of resolving the conflicts and submit everything to Greg. In other >> >>> words, it should be always ok to base your chipidea patchsets on >> >>> usb-next. >> >>> >> >>> Let me know if this sounds reasonable to you. >> >> >> >> Michael has already done that work (some S-o-b form Michael are missing) >> >> and rebased Sascha's and Peter's patches to current linus/master, see >> >> this tree : >> >> >> >> http://git.pengutronix.de/?p=mgr/linux.git;a=shortlog;h=refs/heads/chipidea-for-v3.10 >> > >> > Taking a quick look, quite some of those patches are not ready for >> > inclusion yet. So if the question is, do we need a -next tree for all >> > the chipidea patchsets floating around, it might be a good thing. But >> > it's not what Peter was asking in the first place. >> >> I suggest that we have a branch that holds all chipidea patches that are >> ready for mainline. Otherwise it's really hard to code any new features > > I agree. > > Alex, you can have a repo at github or any other places which is based > on usb-next, and add it to MAINTAINERS. We can develop the new feature > based on your repo. Greg can pull it directly if he agrees or you can send > your queued patchset before every merge windows. Ok, let's try this. I have a linux-ci repo on github, might as well do something useful with it [1], [2]. Currently, the branch called "ci-for-greg" is where I stack patches that I'm planning to be sending (via email) to Greg when the time is right. The "policy" is such that it'll be rebased on top of Greg's usb-next and probably often, so no fast forwards. Also patches may be dropped from it if necessary. Since the branch is not for pulling, the "no rebase" rule doesn't apply. If you have comments/suggestions/etc for a patch that is in this branch, please reply to the email with that patch on the mailing list and not via github infrastructure. Sounds reasonable? I'm now scouting my inbox for more candidates for this branch. Please don't re-send the patches that have already been sent, unless you have new versions of those, I have them all. Do send new versions, though. [1] git://github.com/virtuoso/linux-ci.git ci-for-greg [2] https://github.com/virtuoso/linux-ci/commits/ci-for-greg Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] USB mxs-phy: use readl(), writel() instead of the _relaxed() versions
Marc Kleine-Budde writes: > This patch converts the mxs-phy driver from readl_relaxed(), writel_relaxed() > to the plain readl(), writel() functions, which are available on all > platforms. > This is done to enable compile time testing on non ARM platforms. > > Reported-by: Alexander Shishkin I think it was Felipe who reported this actually. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v9 8/9] usb: chipidea: tell platform layer the ci core probe's result
Felipe Balbi writes: > Hi, > > On Thu, Feb 28, 2013 at 11:32:09AM +0200, Alexander Shishkin wrote: >> >> > > > > > > If the probe fails, the ci13xxx_add_device will not return >> >> > > > > > > error, >> >> > > > > > > (bus_probe_device doesn't has return value) >> >> > > > > > > therefore, the platform layer can't know whether core's probe >> >> > > > > > > is successful or not, if platform layer goes on using core's >> >> > > > > > > struct >> >> > > > > > > which is initialized at core's probe, the error will occur. >> >> > > > > > > >> >> > > > > > > This error is showed when I only compile gadget, the host-only >> >> > > > > > > controller reports "no supported roles", and fails probe, but >> >> > > > > > > imx >> >> > > > > > > platform code doesn't know it, and goes on using core's >> >> > > > > > > private data. >> >> > > > > > > >> >> > > > > > > Signed-off-by: Peter Chen >> >> > > > > > >> >> > > > > > this just tells you that platform code shouldn't be using the >> >> > > > > > driver >> >> > > > > > directly. passing probe_retval via platform_data is an >> >> > > > > > abomination, fix >> >> > > > > > the real problem instead, whatever it is. >> >> > > > > >> >> > > > > So you suggest the platform glue layer should not use core >> >> > > > > driver's data >> >> > > > > directly, eg, for your dwc3, the platform glue layer should not >> >> > > > > use >> >> > > > > struct dwc3 *dwc directly? >> >> > > > >> >> > > > yes, and it doesn't. Ever. >> >> > > > >> >> > > >> >> > > If the dwc3 core fails to probe, but controller core clk is still on, >> >> > > is it >> >> > > a valid case? >> >> > >> >> > of course not, but then again, core clk shouldn't be handled by glue >> >> > layer. You need to figure out who owns the clock, if it feeds DWC3 why >> >> > would you clk_get() and clk_prepare_enable() from glue ? Makes no sense. >> >> > >> >> >> >> Sorry? I can't find clk_prepare_enable at dwc3/core.c, but at dwc3 core, >> >> it >> >> try to access register at probe, unless platform layer open the clock, how >> >> can the core visit the core register. >> > >> > Is it really this difficult to figure out ? Fair enough, below are all >> > the details: >> > >> > To understand the reason why dwc3/core.c doesn't know about struct clk, >> > you need to consider where the driver was originally written; it was >> > written on an OMAP platform (actually first on a virtual model OMAP - >> > somewhat like QEMU -, than on a PCIe FPGA prototype of the IP, then >> > ARM-based FPGA prototype, then OMAP5, none of which needed explicit >> > clock control, see below). >> > >> > OMAP's PM is written in such a way that a pm_runtime_get() will enable >> > the device the all clocks necessary to be usable. Since OMAP would never >> > need to use clocks directly and I would never be able to test that code, >> > I decided not to add it. >> > >> > Now, if dwc3-exynos needs it, the sane thing to do would be add struct >> > clk knowledge to dwc3/core.c but make it optional. If there are no >> > clocks available, don't bail out. >> >> I'm not too familiar with the multitudes of platforms out there, but my >> simple question is: why can't we have pm runtime take care of >> enabling/disabling the clocks so that we don't have to do it in drivers? > > that's what OMAP does. > >> Seems obvious that a platform/SoC/board should know about it's clock >> tree structure, so why doesn't the platform code then take care of all >> the dirty details? > > it might seem that way, but it's not that obvious ;-) Some platforms > have a single clock, some will split in
Re: v3.9-rc1: swapper/0 [ INFO: possible circular locking dependency detected ]
On 6 March 2013 12:33, Maxime Ripard wrote: > Hi, > > Just noticed this in 3.9-rc1 on an iMX28 (ARM) board with a config > based on mxs_defconfig. I'm using the patchset "Add tested id switch > and vbus connect detect support for Chipidea" from Peter Chen in its > 10th version [1], rebased on top of 3.9-rc1, but since this doesn't > modify the locks present in the udc_irq function, I think the problem > is elsewhere. >From a quick look at the current chipidea code, I don't see any place where we might try to take cdev->lock under ci->lock. I should have a better look at Peter's patchset to see if it can trigger that. Peter, have you seen anything like that with your code? Is this easy to trigger (I would assume it is)? Can you post the contents of "events" debug file in sysfs? Thanks, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v11 3/9] usb: chipidea: add otg id switch and vbus connect/disconnect detect
On 6 March 2013 11:56, 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.
>
> - vbus interrupt needs to be handled when gadget function is enabled
>
> - Create/destroy the gadget at udc's init and destory function
>
> - start/stop API are used at otg id switch and probe routine
>
> - Defer some gadget operations at ci's delayed work queue
When I asked you to reorder patches, I didn't mean squash them all
into one. Now you have a patch that does 5 different things and none
of them properly, because you still need to amend these changes later
in the patchset, like the patch 7, where you remove the delayed work,
which is added in this patch. At the same time, you have bits in this
patch that should be moved to other patches. See comments below.
>
> Signed-off-by: Peter Chen
> ---
> drivers/usb/chipidea/bits.h| 10 ++
> drivers/usb/chipidea/ci.h |7 +-
> drivers/usb/chipidea/ci13xxx_imx.c |3 +-
> drivers/usb/chipidea/core.c| 190
>
> drivers/usb/chipidea/host.c|6 +
> drivers/usb/chipidea/host.h|6 +-
> drivers/usb/chipidea/otg.c | 28 --
> drivers/usb/chipidea/otg.h |3 +
> drivers/usb/chipidea/udc.c | 39 ++--
> drivers/usb/chipidea/udc.h |4 +
> 10 files changed, 254 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
> index d8ffc2f..58ef56c 100644
> --- a/drivers/usb/chipidea/bits.h
> +++ b/drivers/usb/chipidea/bits.h
> @@ -77,11 +77,21 @@
> #define OTGSC_ASVIS BIT(18)
> #define OTGSC_BSVIS BIT(19)
> #define OTGSC_BSEIS BIT(20)
> +#define OTGSC_1MSIS BIT(21)
> +#define OTGSC_DPIS BIT(22)
> #define OTGSC_IDIE BIT(24)
> #define OTGSC_AVVIE BIT(25)
> #define OTGSC_ASVIE BIT(26)
> #define OTGSC_BSVIE BIT(27)
> #define OTGSC_BSEIE BIT(28)
> +#define OTGSC_1MSIE BIT(29)
> +#define OTGSC_DPIE BIT(30)
> +#define OTGSC_INT_EN_BITS (OTGSC_IDIE | OTGSC_AVVIE | OTGSC_ASVIE \
> + | OTGSC_BSVIE | OTGSC_BSEIE | OTGSC_1MSIE \
> + | OTGSC_DPIE)
> +#define OTGSC_INT_STATUS_BITS (OTGSC_IDIS | OTGSC_AVVIS | OTGSC_ASVIS \
> + | OTGSC_BSVIS | OTGSC_BSEIS | OTGSC_1MSIS \
> + | OTGSC_DPIS)
>
> /* USBMODE */
> #define USBMODE_CM(0x03UL << 0)
> diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> index 697e369..a3777a1 100644
> --- a/drivers/usb/chipidea/ci.h
> +++ b/drivers/usb/chipidea/ci.h
> @@ -130,6 +130,9 @@ struct hw_bank {
> * @transceiver: pointer to USB PHY, if any
> * @hcd: pointer to usb_hcd for ehci host driver
> * @otg: for otg support
> + * @id_event: indicates there is a id event, and handled at ci_otg_work
> + * @b_sess_valid_event: indicates there is a vbus event, and handled
> + * at ci_otg_work
> */
> struct ci13xxx {
> struct device *dev;
> @@ -140,6 +143,7 @@ struct ci13xxx {
> enum ci_rolerole;
> boolis_otg;
> struct work_struct work;
> + struct delayed_work dwork;
> struct workqueue_struct *wq;
>
> struct dma_pool *qh_pool;
> @@ -166,6 +170,8 @@ struct ci13xxx {
> struct usb_phy *transceiver;
> struct usb_hcd *hcd;
> struct usb_otg otg;
> + boolid_event;
> + boolb_sess_valid_event;
> };
>
> static inline struct ci_role_driver *ci_role(struct ci13xxx *ci)
> @@ -201,7 +207,6 @@ static inline void ci_role_stop(struct ci13xxx *ci)
>
> ci->roles[role]->stop(ci);
> }
> -
>
> /**
> * REGISTERS
>
> */
> diff --git a/drivers/usb/chipidea/ci13xxx_imx.c
> b/drivers/usb/chipidea/ci13xxx_imx.c
> index 136869b..793fdba 100644
> --- a/drivers/usb/chipidea/ci13xxx_imx.c
> +++ b/drivers/usb/chip
Re: [PATCH] usb: chipidea: don't redefine __ffs()
On 7 March 2013 18:47, Felipe Balbi wrote: > Hi, > > On Fri, Feb 22, 2013 at 10:24:41AM +0200, Felipe Balbi wrote: >> chipidea's ffs_nr() is pretty much what __ffs() does. >> >> Use that one instead. >> >> Signed-off-by: Felipe Balbi > > it has been almost 2 weeks, just a gentle ping here. It's been sitting in my github branch [1] for a week, too. Forgot to reply here, sorry It's obviously the right thing, thanks. [1] https://github.com/virtuoso/linux-ci/commits/ci-for-greg -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: About USBADRA bit at DEVICEADDR for chipidea driver
Peter Chen writes: > Hi David Hi, > I notice at your code for hw_usb_set_address, there is a comment for it > /** > * This function explicitly sets the address, without the "USBADRA" (advance) > * feature, which is not supported by older versions of the controller. > */ That's mine. David's original driver did use USBADRA. I removed it because it's not supported by some versions of chipidea, for example the one that Marvell integrated in their kirkwood SoCs. > If we use USB3.0 host for CV test, we must use this bit, or the host > may send GET_DESCRIPTOR before the controller set address. That's because we don't have the state machine for ep0 currently, USBADRA has nothing to do with it. > My question is how can we know if the chipidea controller supports > USBADRA feature or not? We probably can figure it out, but in the end it just adds more complication. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: About USBADRA bit at DEVICEADDR for chipidea driver
Peter Chen writes: > On Fri, Mar 08, 2013 at 12:39:04PM +0200, Alexander Shishkin wrote: >> Peter Chen writes: >> >> > Hi David >> >> Hi, >> >> > I notice at your code for hw_usb_set_address, there is a comment for it >> > /** >> > * This function explicitly sets the address, without the "USBADRA" >> > (advance) >> > * feature, which is not supported by older versions of the controller. >> > */ >> >> That's mine. David's original driver did use USBADRA. I removed it >> because it's not supported by some versions of chipidea, for example the >> one that Marvell integrated in their kirkwood SoCs. >> >> > If we use USB3.0 host for CV test, we must use this bit, or the host >> > may send GET_DESCRIPTOR before the controller set address. >> >> That's because we don't have the state machine for ep0 currently, >> USBADRA has nothing to do with it. > > Can you explain more? As far as I know, it is the completion of set_address > cost too much time that the host sends next command before device > sets address. USBADRA doesn't magically speed up anything. It's a convenience thing that lets you set the controller's address register value straight away instead of deferring it till after status phase of SET_ADDRESS. The status phase won't happen any sooner than normally. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: fsl_udc_core: set address request aren't handled like expected. usb30cv test suite fails.
Peter Chen writes: > On Thu, Mar 07, 2013 at 09:58:52AM +0100, Peter Bestler wrote: >> Hi, >> >> We try to get our device (based on p2020rdb) usb 2.0 compliant. We ran the >> usb30cv test suite (version 1.0.1.2, chapter 9 tests for usb 2.0 devices) on >> win7 with g_zero and g_serial. We access the device via an usb 3.0 hcd from >> intel. Our device runs the 3.2.35-rt52 kernel. I spotted the following >> problem with ch9setaddress in fsl_udc_core.c. >> >> All tests passed on single execution. At running it in batch mode the first >> test after switching from default to adressed state failed. The subsequent >> tests passed. It doesn't depend on the selected tests, only on the switch >> over from default to adressed. It fails with our custom gadgetfs driver too. >> Another device with Intel PXA25x and the same setup passes all tests. >> >> With the total phase usbsniffer i spotted the following behavior: >> The test issues a setAddress and receives an ACK, 125 us afterwards the host >> issues a getDescriptor (setuptx) request, which fails at setuptx. The USB >> sniffer reports invalid PID sequence. >> >> For debugging purpose I delayed the dma_map_single in ep0_prime_status >> (which does the ACK, right?) by 2 milliseconds. And all tests are passing in >> batch mode. It's quite the same sequence on the bus, but between setAdress >> and its ACK is a delay of 3 ms. >> >> I think delaying the ACK in set request isn't the way to go. I think we set >> the address too early; we have to wait until the status phase of the set >> address has finished. My understanding is that the device has to respond to >> address 0 until the complete status phase of setAddress is passed (is this >> correct). >> >> Has anybody ran the usb30cv on fsl_udc recently? >> >> After applying the patch f79a60b8785 none of the tests run anymore. Did I >> miss anything here? How can we fix this issue? >> >> Best regards >> >> Peter >> >> --- Patch for debugging --- >> diff --git a/drivers/usb/gadget/fsl_udc_core.c >> b/drivers/usb/gadget/fsl_udc_core.c >> index 55abfb6..fdbfd25 100644 >> --- a/drivers/usb/gadget/fsl_udc_core.c >> +++ b/drivers/usb/gadget/fsl_udc_core.c >> @@ -1285,6 +1285,7 @@ static int ep0_prime_status(struct fsl_udc *udc, int >> direction) >> req->req.complete = NULL; >> req->dtd_count = 0; >> >> + udelay(2000); >> req->req.dma = dma_map_single(ep->udc->gadget.dev.parent, >> req->req.buf, req->req.length, >> ep_is_in(ep) ? DMA_TO_DEVICE : DMA_FROM_DEVICE); >> >> >> >> > > Please check your datasheet if your controller has USBADRA bit > at DEVICEADDR, if it exists, it means your controller supports > advance store device address function. Please have a try for > below change, if it fixes your problem, please give a tested-by, > then, I can make change for chipidea and fsl-core driver. Ah, now I see where the confusion comes from. No, chipidea driver already gets it right, no changes needed. Even better, we do it without the USBADRA crutch, since not all chipidea cores support it. Maybe this is our reminder that it's about time we started moving all chipidea users to use the chipidea driver. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] usb: chipidea: udc: add the define TD_COUNT and fix all users
Michael Grzeschik writes: > A static count of transfer descriptors was used everywhere in the driver > with the fixed number 4. This patch adds a define, named TD_COUNT, and > replaces all users of this value. This way its possible to have only one > parameter to change and limit the amount of tds per transfer. I think Svetoslav made exactly the same patch in his patchset, but I think this patchset will go first. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] USB chipidea: introduce dual role mode pdata flags
On 8 March 2013 18:33, Felipe Balbi wrote:
> Hi,
>
> On Thu, Feb 28, 2013 at 11:57:02AM +0100, Marc Kleine-Budde wrote:
>> @@ -487,14 +488,23 @@ static int ci_hdrc_probe(struct platform_device *pdev)
>> return -ENODEV;
>> }
>>
>> + /* For now we treat dual-role as otg */
>> + dr_mode = ci->platdata->dr_mode;
>> + if (dr_mode == USB_DR_MODE_UNKNOWN || dr_mode == USB_DR_MODE_DUAL_ROLE)
>> + dr_mode = USB_DR_MODE_OTG;
>> +
>> /* initialize role(s) before the interrupt is requested */
>> - ret = ci_hdrc_host_init(ci);
>> - if (ret)
>> - dev_info(dev, "doesn't support host\n");
>> + if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>
> this is not something you should be passing via pdata; chipidea core
> should know how to read this data by itself. Meaning that chipidea core
> should be taught about devicetree. But make it optional since now all
> users use DT.
And I don't think I like the idea of chipidea core calling into device
tree code directly.
Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/5] USB chipidea: introduce dual role mode pdata flags
On 8 March 2013 18:52, Marc Kleine-Budde wrote:
> On 03/08/2013 05:46 PM, Alexander Shishkin wrote:
>> On 8 March 2013 18:33, Felipe Balbi wrote:
>>> Hi,
>>>
>>> On Thu, Feb 28, 2013 at 11:57:02AM +0100, Marc Kleine-Budde wrote:
>>>> @@ -487,14 +488,23 @@ static int ci_hdrc_probe(struct platform_device
>>>> *pdev)
>>>> return -ENODEV;
>>>> }
>>>>
>>>> + /* For now we treat dual-role as otg */
Btw, if we do this, Peter's otg code will try to access OTGSC, which
is not what we want on non-otg devices, so we'll need a clear
distinction between the two.
>>>> + dr_mode = ci->platdata->dr_mode;
>>>> + if (dr_mode == USB_DR_MODE_UNKNOWN || dr_mode ==
>>>> USB_DR_MODE_DUAL_ROLE)
>>>> + dr_mode = USB_DR_MODE_OTG;
>>>> +
>>>> /* initialize role(s) before the interrupt is requested */
>>>> - ret = ci_hdrc_host_init(ci);
>>>> - if (ret)
>>>> - dev_info(dev, "doesn't support host\n");
>>>> + if (dr_mode == USB_DR_MODE_OTG || dr_mode == USB_DR_MODE_HOST) {
>>>
>>> this is not something you should be passing via pdata; chipidea core
>>> should know how to read this data by itself. Meaning that chipidea core
>>> should be taught about devicetree. But make it optional since now all
>>> users use DT.
>>
>> And I don't think I like the idea of chipidea core calling into device
>> tree code directly.
>
> Hmmmthis means draw :)
Well, we could go for something like
ci_hdrc-$(CONFIG_OF) += of.o
and try to contain the damage there, maybe? Ideas? I would very much
like to keep the clutter away from the core probe if possible.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: About USBADRA bit at DEVICEADDR for chipidea driver
Peter Chen writes: > On Fri, Mar 08, 2013 at 04:06:30PM +0200, Alexander Shishkin wrote: >> Peter Chen writes: >> >> > On Fri, Mar 08, 2013 at 12:39:04PM +0200, Alexander Shishkin wrote: >> >> Peter Chen writes: >> >> >> >> > Hi David >> >> >> >> Hi, >> >> >> >> > I notice at your code for hw_usb_set_address, there is a comment for it >> >> > /** >> >> > * This function explicitly sets the address, without the "USBADRA" >> >> > (advance) >> >> > * feature, which is not supported by older versions of the controller. >> >> > */ >> >> >> >> That's mine. David's original driver did use USBADRA. I removed it >> >> because it's not supported by some versions of chipidea, for example the >> >> one that Marvell integrated in their kirkwood SoCs. >> >> >> >> > If we use USB3.0 host for CV test, we must use this bit, or the host >> >> > may send GET_DESCRIPTOR before the controller set address. >> >> >> >> That's because we don't have the state machine for ep0 currently, >> >> USBADRA has nothing to do with it. >> > >> > Can you explain more? As far as I know, it is the completion of set_address >> > cost too much time that the host sends next command before device >> > sets address. >> >> USBADRA doesn't magically speed up anything. It's a convenience thing >> that lets you set the controller's address register value straight away >> instead of deferring it till after status phase of >> SET_ADDRESS. The status phase won't happen any sooner than normally. >> > > Correct, but the problem is after the status phase finished, when > the software can set address without using USBADRA? Assuming the busy system, > the interrupt latecy, etc? > > For USB 3.0 host CV test, the host sends GET_DESCRIPTOR very quick (<500us) > once it accept the status of SET_ADDRESS The USB 2.0 spec says the recovery period after the status phase of SetAddress is 2ms. That should be enough to process the transfer completion interrupt, shouldn't it? Why is USB 3 CV violating this and why should we care? I guess, if we really want to, we can make USBADRA usage conditional, BUT something tells me that there will be more arbitrary timing expectations then that we won't necessarily be able to meet. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Two remain problems at chipidea driver
Peter Chen writes: > Hi Alex and all, > > Currently, we have two problems to block chipidea driver coming > development. > > As there are so many chipidea versions, we impossible to collect > all to make a decision, it is better to cover most of the cases, > and using device tree (or platform data) to cover exceptions > if they exist. > > 1. USB Mode Problem > How can we decide USB mode (gadget, host and otg) at driver, and > how to read OTGSC register? Below is my pinion. > > - We get gadget or host support from CAP_DCCPARAMS(DCCPARAMS_DC or > DCCPARAMS_HC), > If both DCCPARAMS_DC and DCCPARAMS_HC are 1, then the mode is "otg". > If DCCPARAMS_DC = 1 and DCCPARAMS_HC = 0, the mode is "gadget". > If DCCPARAMS_DC = 0 and DCCPARAMS_HC = 1, the mode is "host". > If DCCPARAMS_DC = 0 and DCCPARAMS_HC = 0, prompt an error > and try to get it from DT. > > - Override the value using DT, please do not consider too much between > dule_role and otg. We just consider all controllers which supports host > and gadget at the same time as otg device. The exception may not be existed > or be too long to use. The mips platform support patchset that Svetoslav is doing is for such a device. In any case, I see no reason to not support all the possible devices. > - For how to read OTGSC register, we need another flag to indicate > it is otg capable (DCCPARAMS_DC = 1 and DCCPARAMS_HC = 1), if it is > otg capable, read OTGSC is allowed. Here, OTG capable device can work > as gadget only mode. I'd say, in the core driver: 1) get dr_mode from platform data 2) only if that's DR_MODE_UNKNOWN (iirc), read DCCPARAMS, since it's not guaranteed to work across all chipideas; 3) if dr_mode == OTG or dr_mode == UNKNOWN and DCCPARAMS has both DC and HC, set ci->otg, which determines if we can access otgsc 4) if dr_mode == DUAL_ROLE, don't set ci->otg. Do you see any problems with this? > 2. Chipidea Core Driver DT Support > I agree we move some core things, like vbus, operation_mode, phy to core > driver using DT. But for clock, it is better still exist at glue > layer, clock is input for chipidea core, chipidea core doesn't need > to know its clock from IC point. Like clock, the wakeup setting, low > power sequence are platform specific, they are designed by individual > companies. Hmm, if the clock is external to chipidea, I guess it should be ok to keep it in the platform code as long as it is entirely contained in the platform code, that is, no awkward passing *clk pointers to the core and back. > Let me know your opinion about these two problems and your plan for them. > Thanks. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Two remain problems at chipidea driver
Peter Chen writes: > On Thu, Mar 14, 2013 at 12:31:38PM +0200, Alexander Shishkin wrote: >> Peter Chen writes: >> >> > Hi Alex and all, >> > >> > Currently, we have two problems to block chipidea driver coming >> > development. >> > >> > As there are so many chipidea versions, we impossible to collect >> > all to make a decision, it is better to cover most of the cases, >> > and using device tree (or platform data) to cover exceptions >> > if they exist. >> > >> > 1. USB Mode Problem >> > How can we decide USB mode (gadget, host and otg) at driver, and >> > how to read OTGSC register? Below is my pinion. >> > >> > - We get gadget or host support from CAP_DCCPARAMS(DCCPARAMS_DC or >> > DCCPARAMS_HC), >> > If both DCCPARAMS_DC and DCCPARAMS_HC are 1, then the mode is "otg". >> > If DCCPARAMS_DC = 1 and DCCPARAMS_HC = 0, the mode is "gadget". >> > If DCCPARAMS_DC = 0 and DCCPARAMS_HC = 1, the mode is "host". >> > If DCCPARAMS_DC = 0 and DCCPARAMS_HC = 0, prompt an error >> > and try to get it from DT. >> > >> > - Override the value using DT, please do not consider too much between >> > dule_role and otg. We just consider all controllers which supports host >> > and gadget at the same time as otg device. The exception may not be existed >> > or be too long to use. >> >> The mips platform support patchset that Svetoslav is doing is for such a >> device. In any case, I see no reason to not support all the possible >> devices. > > Oh, I find Svetoslav's patch, and know the situation. > >> >> > - For how to read OTGSC register, we need another flag to indicate >> > it is otg capable (DCCPARAMS_DC = 1 and DCCPARAMS_HC = 1), if it is >> > otg capable, read OTGSC is allowed. Here, OTG capable device can work >> > as gadget only mode. >> >> I'd say, in the core driver: >> 1) get dr_mode from platform data >> 2) only if that's DR_MODE_UNKNOWN (iirc), read DCCPARAMS, since it's >> not guaranteed to work across all chipideas; >> 3) if dr_mode == OTG or dr_mode == UNKNOWN and DCCPARAMS has both DC >> and HC, set ci->otg, which determines if we can access otgsc >> 4) if dr_mode == DUAL_ROLE, don't set ci->otg. >> >> Do you see any problems with this? > > How to know vbus status if dr_mode == gadget, we need to indicate connection > and disconnection? We don't. Do we need to indicate vbus session valid for gadget only devices? > if dr_mode == DUAL_ROLE, do we support ID switch? How can we know ID which > ID pins connects to controller? In this case we can support gpio-based or debugfs file-based role switching. > I think we need to split the relationship between dr_mode and OTG capable > to cover this problem, eg, using another platform data to indicate if > the controller supports OTG or not, since now we can't know if the controller > supports OTG or not from registers. And only OTG capable device can access > OTGSC. So that's what dr_mode already does in my suggestion above, isn't it? dr_mode != OTG => no OTG; why do we need another platform field for? >> > 2. Chipidea Core Driver DT Support >> > I agree we move some core things, like vbus, operation_mode, phy to core >> > driver using DT. But for clock, it is better still exist at glue >> > layer, clock is input for chipidea core, chipidea core doesn't need >> > to know its clock from IC point. Like clock, the wakeup setting, low >> > power sequence are platform specific, they are designed by individual >> > companies. >> >> Hmm, if the clock is external to chipidea, I guess it should be ok to >> keep it in the platform code as long as it is entirely contained in the >> platform code, that is, no awkward passing *clk pointers to the core and >> back. > > Do you agree you convert DT right now, seems Felipe insists on it? You mean, move phy to the core instead of passing the pointer and such? Yes, we should *carefully* do that. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Two remain problems at chipidea driver
Peter Chen writes: > On Fri, Mar 15, 2013 at 12:38:07PM +0200, Alexander Shishkin wrote: >> >> >> >> I'd say, in the core driver: >> >> 1) get dr_mode from platform data >> >> 2) only if that's DR_MODE_UNKNOWN (iirc), read DCCPARAMS, since it's >> >> not guaranteed to work across all chipideas; >> >> 3) if dr_mode == OTG or dr_mode == UNKNOWN and DCCPARAMS has both DC >> >> and HC, set ci->otg, which determines if we can access otgsc >> >> 4) if dr_mode == DUAL_ROLE, don't set ci->otg. >> >> >> >> Do you see any problems with this? >> > >> > How to know vbus status if dr_mode == gadget, we need to indicate >> > connection >> > and disconnection? >> >> We don't. Do we need to indicate vbus session valid for gadget only >> devices? > > Of course, eg,, how android phone know it connects to pc or not? If nothing else, then at least it should be able to tell from the usb phy. But I would be surprised if somebody uses a gadget-only usb controller in a SoC that they use for an android phone or tablet. Most probably it'll be dr_mode==USB_DR_MODE_OTG. >> >> > if dr_mode == DUAL_ROLE, do we support ID switch? How can we know ID which >> > ID pins connects to controller? >> >> In this case we can support gpio-based or debugfs file-based role >> switching. >> >> > I think we need to split the relationship between dr_mode and OTG capable >> > to cover this problem, eg, using another platform data to indicate if >> > the controller supports OTG or not, since now we can't know if the >> > controller >> > supports OTG or not from registers. And only OTG capable device can access >> > OTGSC. >> >> So that's what dr_mode already does in my suggestion above, isn't it? >> dr_mode != OTG => no OTG; why do we need another platform field for? > > dr_mode stands for which controller operation mode currently What do you mean, "currently"? It *should* define what the controller is capable of. Then, the current mode of operation can be either detected from OTGSC or will simply default to HOST or PERIPHERAL, according to dr_mode. IOW, dr_mode is a capability of the controller. > otg_capable stands for whether the controller supports OTG operation It is still redundant as otg_capable = dr_mode == USB_DR_MODE_OTG (same as ci->otg should be). > Eg, for tablet or phone, the dr_mode may be "gadget", but the > otg_capable = 1. No, because dr_mode indicates controller's capability, and not the "current" mode of operation. Why would anyone want to put *that* in a DT? Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Two remain problems at chipidea driver
Peter Chen writes: > On Thu, Mar 14, 2013 at 09:53:55AM +0100, Marc Kleine-Budde wrote: >> On 03/14/2013 09:34 AM, Peter Chen wrote: >> > Hi Alex and all, >> > >> > Currently, we have two problems to block chipidea driver coming >> > development. >> > >> > As there are so many chipidea versions, we impossible to collect >> > all to make a decision, it is better to cover most of the cases, >> > and using device tree (or platform data) to cover exceptions >> > if they exist. >> > >> > 1. USB Mode Problem >> > How can we decide USB mode (gadget, host and otg) at driver, and >> > how to read OTGSC register? Below is my pinion. >> > >> > - We get gadget or host support from CAP_DCCPARAMS(DCCPARAMS_DC or >> > DCCPARAMS_HC), >> >> IIRC This is broken on mx25. The host only port gets into an error state >> if you read this register. :( > > If there is an exception, you can use DT to override it. Eg: > > if (ci->dccparams_is_error) > do not read DCCPARAMS; /* using DT dr_mode setting */ I know it's tempting to add more phandles to the device tree for every possible exception, but let's try to resist the temptation. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Two remain problems at chipidea driver
Peter Chen writes: > On Fri, Mar 15, 2013 at 05:17:08PM +0200, Alexander Shishkin wrote: >> >> > Eg, for tablet or phone, the dr_mode may be "gadget", but the >> > otg_capable = 1. >> >> No, because dr_mode indicates controller's capability, and not the >> "current" mode of operation. Why would anyone want to put *that* in a >> DT? >> > > OK, now I totally understand your mind of this problem. In fact, dr_mode > is NOT controller's capability, even at its original place: > (Documentation/devicetree/bindings/usb/fsl-usb.txt or nvidia, > tegra20-ehci.txt) > dr_mode is the usb working mode. > > When we design USB system, the requirements are differ from products > to products. > The phone/tablet device may only wants itself as gadget > device, it needs to be no-response when there is a usb device plug in > (eg usb keyboard with Micro B-to-A cable). > > The car entertainment system or other Standard-A port system do not want > to be enumerated when it plugs to notebook using Standard A-to-A cable. Bah. Of course, you're right. We're stuck with dr_mode till people learn to design middleware stacks that can handle being both host and peripheral. > So, currently, even most of controllers are otg-capable, still most > of designs are one working mode designed. The reason why we design > the dr_mode is that we want controller working mode to be decided > by DT without re-compile the kernel by build out the host/gadget driver. Ok, so then how about introducing *one* more parameter, something like "dr_cap", which 1) when specified, supersedes DCCPARAMS, so no need to read that register any more; 2) when unspecified, use DCCPARAMS; 3) can be one of "host", "peripheral", "otg", "dual_role": - host, peripheral: initialize one role only, stick to that, no otg; - dual_role: initialize both roles, no otg; - otg: both roles, ci->is_otg == true. Another question now is, do we need "dual_role" variant for the dr_mode parameter? Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Two remain problems at chipidea driver
Marc Kleine-Budde writes: > On 03/20/2013 12:04 PM, Alexander Shishkin wrote: >> Peter Chen writes: >> >>> On Fri, Mar 15, 2013 at 05:17:08PM +0200, Alexander Shishkin wrote: >>>> >>>>> Eg, for tablet or phone, the dr_mode may be "gadget", but the >>>>> otg_capable = 1. >>>> >>>> No, because dr_mode indicates controller's capability, and not the >>>> "current" mode of operation. Why would anyone want to put *that* in a >>>> DT? >>>> >>> >>> OK, now I totally understand your mind of this problem. In fact, dr_mode >>> is NOT controller's capability, even at its original place: >>> (Documentation/devicetree/bindings/usb/fsl-usb.txt or nvidia, >>> tegra20-ehci.txt) >>> dr_mode is the usb working mode. >>> >>> When we design USB system, the requirements are differ from products >>> to products. >>> The phone/tablet device may only wants itself as gadget >>> device, it needs to be no-response when there is a usb device plug in >>> (eg usb keyboard with Micro B-to-A cable). >>> >>> The car entertainment system or other Standard-A port system do not want >>> to be enumerated when it plugs to notebook using Standard A-to-A cable. >> >> Bah. Of course, you're right. We're stuck with dr_mode till people learn >> to design middleware stacks that can handle being both host and >> peripheral. >> >>> So, currently, even most of controllers are otg-capable, still most >>> of designs are one working mode designed. The reason why we design >>> the dr_mode is that we want controller working mode to be decided >>> by DT without re-compile the kernel by build out the host/gadget driver. >> >> Ok, so then how about introducing *one* more parameter, something like >> "dr_cap", which >> 1) when specified, supersedes DCCPARAMS, so no need to read that >> register any more; >> 2) when unspecified, use DCCPARAMS; >> 3) can be one of "host", "peripheral", "otg", "dual_role": >>- host, peripheral: initialize one role only, stick to that, no otg; >>- dual_role: initialize both roles, no otg; >>- otg: both roles, ci->is_otg == true. >> >> Another question now is, do we need "dual_role" variant for the dr_mode >> parameter? > > What's the difference between the newly proposed dr_cap and the dr_mode > parameter? See the discussion above. dr_cap is what the device can actually do (host, peripheral, etc). Tells us which roles to initialize and wether we can access OTGSC on this device. dr_mode is what function of the device we'll be using on this particular board. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Two remain problems at chipidea driver
Marc Kleine-Budde writes: > On 03/20/2013 12:23 PM, Alexander Shishkin wrote: >> Marc Kleine-Budde writes: >> >>> On 03/20/2013 12:04 PM, Alexander Shishkin wrote: >>>> Peter Chen writes: >>>> >>>>> On Fri, Mar 15, 2013 at 05:17:08PM +0200, Alexander Shishkin wrote: >>>>>> >>>>>>> Eg, for tablet or phone, the dr_mode may be "gadget", but the >>>>>>> otg_capable = 1. >>>>>> >>>>>> No, because dr_mode indicates controller's capability, and not the >>>>>> "current" mode of operation. Why would anyone want to put *that* in a >>>>>> DT? >>>>>> >>>>> >>>>> OK, now I totally understand your mind of this problem. In fact, dr_mode >>>>> is NOT controller's capability, even at its original place: >>>>> (Documentation/devicetree/bindings/usb/fsl-usb.txt or nvidia, >>>>> tegra20-ehci.txt) >>>>> dr_mode is the usb working mode. >>>>> >>>>> When we design USB system, the requirements are differ from products >>>>> to products. >>>>> The phone/tablet device may only wants itself as gadget >>>>> device, it needs to be no-response when there is a usb device plug in >>>>> (eg usb keyboard with Micro B-to-A cable). >>>>> >>>>> The car entertainment system or other Standard-A port system do not want >>>>> to be enumerated when it plugs to notebook using Standard A-to-A cable. >>>> >>>> Bah. Of course, you're right. We're stuck with dr_mode till people learn >>>> to design middleware stacks that can handle being both host and >>>> peripheral. >>>> >>>>> So, currently, even most of controllers are otg-capable, still most >>>>> of designs are one working mode designed. The reason why we design >>>>> the dr_mode is that we want controller working mode to be decided >>>>> by DT without re-compile the kernel by build out the host/gadget driver. >>>> >>>> Ok, so then how about introducing *one* more parameter, something like >>>> "dr_cap", which >>>> 1) when specified, supersedes DCCPARAMS, so no need to read that >>>> register any more; >>>> 2) when unspecified, use DCCPARAMS; >>>> 3) can be one of "host", "peripheral", "otg", "dual_role": >>>>- host, peripheral: initialize one role only, stick to that, no otg; >>>>- dual_role: initialize both roles, no otg; >>>>- otg: both roles, ci->is_otg == true. >>>> >>>> Another question now is, do we need "dual_role" variant for the dr_mode >>>> parameter? >>> >>> What's the difference between the newly proposed dr_cap and the dr_mode >>> parameter? >> >> See the discussion above. >> >> dr_cap is what the device can actually do (host, peripheral, etc). Tells >> us which roles to initialize and wether we can access OTGSC on this >> device. >> dr_mode is what function of the device we'll be using on this particular >> board. > > Sorry, I don't get why the driver needs to know what the chipidea can do > in theory (dr_cap). IMHO it should be sufficient to tell the driver what > that exact hardware it runs on can do (dr_mode). What the hardware can > do depends on the actual chipidea implementation used in that SoC and > the board the SoC is soldered on. Again, see the discussion above. In real world products (that is, phones and tablets as opposed to jolly fun development boards), vendors will want to limit the usb functionality to peripheral only or host only or whatever, because the middleware stack can only do one thing or because they don't want to go through with otg certification or you name it. Meanwhile, the controller and the whole device can still support otg. And we need to know that if we're to try to detect vbus session, because that is done via OTGSC which is only available in otg configurations. The alternative is to have N more platform tweaks for * dccparams_is_borked, * otgsc_is_borked, and a lot of luck trying to get that right in the probe(). This is, of course a preferred approach for anyone who supports phandle creep in DT. I don't like either approach, but the former seems a tad less damaging. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Two remain problems at chipidea driver
Felipe Balbi writes: > Hi, > > On Wed, Mar 20, 2013 at 03:37:01PM +0200, Alexander Shishkin wrote: >> >> dr_cap is what the device can actually do (host, peripheral, etc). Tells >> >> us which roles to initialize and wether we can access OTGSC on this >> >> device. >> >> dr_mode is what function of the device we'll be using on this particular >> >> board. >> > >> > Sorry, I don't get why the driver needs to know what the chipidea can do >> > in theory (dr_cap). IMHO it should be sufficient to tell the driver what >> > that exact hardware it runs on can do (dr_mode). What the hardware can >> > do depends on the actual chipidea implementation used in that SoC and >> > the board the SoC is soldered on. >> >> Again, see the discussion above. >> >> In real world products (that is, phones and tablets as opposed to jolly >> fun development boards), vendors will want to limit the usb >> functionality to peripheral only or host only or whatever, because the >> middleware stack can only do one thing or because they don't want to go >> through with otg certification or you name it. Meanwhile, the controller > > that's not entirely true. A manufacturer can decide to skip OTG > certification but still support Dual Role. Look at the whole Android > Accessory Kit, for example. Sure, I was just making an example of how device capabilities can differ from device's intended function. >> and the whole device can still support otg. And we need to know that if >> we're to try to detect vbus session, because that is done via OTGSC >> which is only available in otg configurations. > > well, if it's only available in OTG configurations, then you make the > same assumption in driver. If driver was compiled with OTG, you check > OTGSC; otherwise don't. I'd kind of like to support different configurations in runtime and have as few compilation options as possible. Of course, if it means extra spaghetti, there's a trade off right there. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: chipidea driver
Richard Zhao writes: > On Thu, Aug 23, 2012 at 06:57:03PM +0200, Marc Kleine-Budde wrote: >> Hello, Hi, >> Michael and I have a bunch of updates and improvement for the chipidea >> driver. They apply to Richard's tree: >> >> https://github.com/riczhao/kernel-imx/commits/topics/usb-driver >> >> What's the status of these patches? It would be fine if someone queues >> them for upstream. > My patches is pending on Alex to review. The otg patch series was sent > on Jul 12. I don't know whether Alex has been back from vacation, or > what else I can do. > > otg patch: > http://lists.infradead.org/pipermail/linux-arm-kernel/2012-July/109020.html > usbmisc: > http://lists.infradead.org/pipermail/linux-arm-kernel/2012-July/111945.html Thanks for the ping, will have a look soon. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/11] USB: chipidea: clear gadget struct at udc_start fail path
Richard Zhao writes: > States in gadget are not needed any more, set it to zero. It's generally a good practice to mention in the commit message what is it that you are fixing with this patch. > Signed-off-by: Richard Zhao > --- > drivers/usb/chipidea/udc.c |1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > index c7a032a..9fb6394 100644 > --- a/drivers/usb/chipidea/udc.c > +++ b/drivers/usb/chipidea/udc.c > @@ -1746,6 +1746,7 @@ free_pools: > dma_pool_destroy(ci->td_pool); > free_qh_pool: > dma_pool_destroy(ci->qh_pool); > + memset(&ci->gadget, 0, sizeof(ci->gadget)); I understand that you're probably hitting "kobject already initialized" warning here, but I think the real problem is that this function gets called twice and fails the first time. Why does that happen? In any case, please elaborate on the problem. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 05/11] USB: chipidea: don't let probe fail if otg controller start one role failed
Richard Zhao writes:
> One role failed, but the other role will possiblly still work.
> For example, when udc start failed, if plug in a host device,
> it works.
If you're a host device, ci->role should be HOST at this point and
ci_role_start() shouldn't fail. If ci->role is detected wrongly, the
role detection must be fixed. If ci_role_start() fails for host, but it
still works when it's called again after the id pin change is detected,
again, the problem is elsewhere.
>
> Signed-off-by: Richard Zhao
> ---
> drivers/usb/chipidea/core.c |7 +--
> 1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index 19ef324..8fd390a 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -473,8 +473,11 @@ static int __devinit ci_hdrc_probe(struct
> platform_device *pdev)
> 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;
> + ci->role = CI_ROLE_END;
So are you relying on id pin interrupt for initializing the role after
this? Can you provide more details?
Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/3] USB: chipidea: add imx usbmisc support
Richard Zhao writes:
> i.MX usb controllers shares non-core registers, which may include
> SoC specific controls. We take it as a usbmisc device and usbmisc
> driver set operations needed by ci13xxx_imx driver.
>
> For example, Sabrelite board has bad over-current design, we can
> usbmisc to disable over-current detect.
Why does this have to be part of the usb driver instead of SoC specific
code? It looks like you've created a whole new device/driver
infrastructure just to disable overcurrent for a specific board.
And the infrastructure boils down to a complex way of passing a callback
from imx driver to another imx driver, that only works if they are
probed in the right order. I don't see any point in doing it like this
other than inflating the device tree tables even further.
Why can't this be part of the SoC code like it is done, for example in
arch/arm/mach-omap2/control.c?
Regards,
--
Alex
> Signed-off-by: Richard Zhao
> Acked-by: Sascha Hauer
> ---
> .../devicetree/bindings/usb/ci13xxx-imx.txt|5 +
> .../devicetree/bindings/usb/usbmisc-imx.txt| 14 ++
> drivers/usb/chipidea/Makefile |2 +-
> drivers/usb/chipidea/ci13xxx_imx.c | 64
> drivers/usb/chipidea/ci13xxx_imx.h | 28
> drivers/usb/chipidea/usbmisc_imx6q.c | 162
>
> 6 files changed, 274 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> create mode 100644 drivers/usb/chipidea/ci13xxx_imx.h
> create mode 100644 drivers/usb/chipidea/usbmisc_imx6q.c
>
> diff --git a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> index 2c29041..5778b9c 100644
> --- a/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> +++ b/Documentation/devicetree/bindings/usb/ci13xxx-imx.txt
> @@ -7,7 +7,10 @@ Required properties:
>
> Optional properties:
> - fsl,usbphy: phandler of usb phy that connects to the only one port
> +- fsl,usbmisc: phandler of non-core register device, with one argument
> + that indicate usb controller index
> - vbus-supply: regulator for vbus
> +- disable-over-current: disable over current detect
>
> Examples:
> usb@02184000 { /* USB OTG */
> @@ -15,4 +18,6 @@ usb@02184000 { /* USB OTG */
> reg = <0x02184000 0x200>;
> interrupts = <0 43 0x04>;
> fsl,usbphy = <&usbphy1>;
> + fsl,usbmisc = <&usbmisc 0>;
> + disable-over-current;
> };
> diff --git a/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> new file mode 100644
> index 000..97ce94e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/usbmisc-imx.txt
> @@ -0,0 +1,14 @@
> +* Freescale i.MX non-core registers
> +
> +Required properties:
> +- #index-cells: Cells used to descibe usb controller index. Should be <1>
> +- compatible: Should be one of below:
> + "fsl,imx6q-usbmisc" for imx6q
> +- reg: Should contain registers location and length
> +
> +Examples:
> +usbmisc@02184800 {
> + #index-cells = <1>;
> + compatible = "fsl,imx6q-usbmisc";
> + reg = <0x02184800 0x200>;
> +};
> diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
> index 5c66d9c..57e510f 100644
> --- a/drivers/usb/chipidea/Makefile
> +++ b/drivers/usb/chipidea/Makefile
> @@ -15,5 +15,5 @@ ifneq ($(CONFIG_PCI),)
> endif
>
> ifneq ($(CONFIG_OF_DEVICE),)
> - obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o
> + obj-$(CONFIG_USB_CHIPIDEA) += ci13xxx_imx.o usbmisc_imx6q.o
> endif
> diff --git a/drivers/usb/chipidea/ci13xxx_imx.c
> b/drivers/usb/chipidea/ci13xxx_imx.c
> index ef60d06..96ac67b 100644
> --- a/drivers/usb/chipidea/ci13xxx_imx.c
> +++ b/drivers/usb/chipidea/ci13xxx_imx.c
> @@ -22,6 +22,7 @@
> #include
>
> #include "ci.h"
> +#include "ci13xxx_imx.h"
>
> #define pdev_to_phy(pdev) \
> ((struct usb_phy *)platform_get_drvdata(pdev))
> @@ -34,6 +35,55 @@ struct ci13xxx_imx_data {
> struct regulator *reg_vbus;
> };
>
> +static const struct usbmisc_ops *usbmisc_ops;
> +
> +/* Common functions shared by usbmisc drivers */
> +
> +int usbmisc_set_ops(const struct usbmisc_ops *ops)
> +{
> + if (usbmisc_ops)
> + return -EBUSY;
> +
> + usbmisc_ops = ops;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(usbmisc_set_ops);
> +
> +void usbmisc_unset_ops(const struct usbmisc_ops *ops)
> +{
> + usbmisc_ops = NULL;
> +}
> +EXPORT_SYMBOL_GPL(usbmisc_unset_ops);
> +
> +int usbmisc_get_init_data(struct device *dev, struct usbmisc_usb_device
> *usbdev)
> +{
> + struct device_node *np = dev->of_node;
> + struct of_phandle_args args;
> + int ret;
> +
> + usbdev->dev = dev;
> +
> + ret = of_parse_phandle_with_args(np, "fsl,usbmisc", "#index-cells",
> + 0, &args);
> + if (ret) {
> + dev_err(dev, "Faile
Re: [PATCH v2 04/11] USB: chipidea: clear gadget struct at udc_start fail path
Richard Zhao writes: > On Tue, Aug 28, 2012 at 11:29:35AM +0300, Alexander Shishkin wrote: >> Richard Zhao writes: >> >> > States in gadget are not needed any more, set it to zero. >> >> It's generally a good practice to mention in the commit message what is >> it that you are fixing with this patch. > It fixes the following dump if udc_start register gadget->dev but fail > the start process: > kobject (bf8330a0): tried to init an initialized object, something is > seriously wrong. > [<80011d90>] (dump_backtrace+0x0/0x10c) from [<804b65ac>] > (dump_stack+0x18/0x1c) > [<804b6594>] (dump_stack+0x0/0x1c) from [<80247d98>] (kobject_init+0x7c/0x9c) > [<80247d1c>] (kobject_init+0x0/0x9c) from [<802a3870>] > (device_initialize+0x28/0x70) > [<802a3848>] (device_initialize+0x0/0x70) from [<802a4798>] > (device_register+0x14/0x20) > [<802a4784>] (device_register+0x0/0x20) from [<80359a70>] > (udc_start+0x180/0x2c8) > [<803598f0>] (udc_start+0x0/0x2c8) from [<80356e9c>] (ci_role_work+0xc8/0xec) > [<80356dd4>] (ci_role_work+0x0/0xec) from [<8003a8a0>] > (process_one_work+0x158/0x474) > [<8003a748>] (process_one_work+0x0/0x474) from [<8003af6c>] > (worker_thread+0x140/0x3b4) > [<8003ae2c>] (worker_thread+0x0/0x3b4) from [<8003fe60>] (kthread+0x90/0x9c) > [<8003fdd0>] (kthread+0x0/0x9c) from [<8002774c>] (do_exit+0x0/0x350) >> >> > Signed-off-by: Richard Zhao >> > --- >> > drivers/usb/chipidea/udc.c |1 + >> > 1 file changed, 1 insertion(+) >> > >> > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c >> > index c7a032a..9fb6394 100644 >> > --- a/drivers/usb/chipidea/udc.c >> > +++ b/drivers/usb/chipidea/udc.c >> > @@ -1746,6 +1746,7 @@ free_pools: >> >dma_pool_destroy(ci->td_pool); >> > free_qh_pool: >> >dma_pool_destroy(ci->qh_pool); >> > + memset(&ci->gadget, 0, sizeof(ci->gadget)); >> >> I understand that you're probably hitting "kobject already initialized" >> warning here, but I think the real problem is that this function gets >> called twice and fails the first time. Why does that happen? > I saw the dump at early stage of adding imx otg support, when there's > things not ready. I think it's less important why udc_start fail, > because no one enable imx otg before. It's important when it fails, > the dump shows up. I'd prefer that whenever udc_start() fails it doesn't go unnoticed, except probably for the cases when it fails due to transceiver not being ready at the probe time, which needs to be handled separately. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 05/11] USB: chipidea: don't let probe fail if otg controller start one role failed
Richard Zhao writes:
> On Tue, Aug 28, 2012 at 11:38:23AM +0300, Alexander Shishkin wrote:
>> Richard Zhao writes:
>>
>> > One role failed, but the other role will possiblly still work.
>> > For example, when udc start failed, if plug in a host device,
>> > it works.
>>
>> If you're a host device, ci->role should be HOST at this point and
>> ci_role_start() shouldn't fail. If ci->role is detected wrongly, the
>> role detection must be fixed. If ci_role_start() fails for host, but it
>> still works when it's called again after the id pin change is detected,
>> again, the problem is elsewhere.
> The check is only for OTG device. For single role device, it just fail
> if it start the role failed.
Sorry, can you rephrase?
>>
>> >
>> > Signed-off-by: Richard Zhao
>> > ---
>> > drivers/usb/chipidea/core.c |7 +--
>> > 1 file changed, 5 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
>> > index 19ef324..8fd390a 100644
>> > --- a/drivers/usb/chipidea/core.c
>> > +++ b/drivers/usb/chipidea/core.c
>> > @@ -473,8 +473,11 @@ static int __devinit ci_hdrc_probe(struct
>> > platform_device *pdev)
>> >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;
>> > + ci->role = CI_ROLE_END;
>>
>> So are you relying on id pin interrupt for initializing the role after
>> this? Can you provide more details?
> Yes. The ID pin detect still works. My case is, gadget role failed,
> host role works. I was trying not to ruin host function.
But it shouldn't even try to start the gadget, because ci_otg_role()
should set ci->role to HOST before ci_role_start() happens.
Another question is, why does gadget start fail?
Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 04/11] USB: chipidea: clear gadget struct at udc_start fail path
Richard Zhao writes: > On Wed, Aug 29, 2012 at 11:03:48AM +0300, Alexander Shishkin wrote: >> Richard Zhao writes: >> >> > On Tue, Aug 28, 2012 at 11:29:35AM +0300, Alexander Shishkin wrote: >> >> Richard Zhao writes: >> >> >> >> > States in gadget are not needed any more, set it to zero. >> >> >> >> It's generally a good practice to mention in the commit message what is >> >> it that you are fixing with this patch. >> > It fixes the following dump if udc_start register gadget->dev but fail >> > the start process: >> > kobject (bf8330a0): tried to init an initialized object, something is >> > seriously wrong. >> > [<80011d90>] (dump_backtrace+0x0/0x10c) from [<804b65ac>] >> > (dump_stack+0x18/0x1c) >> > [<804b6594>] (dump_stack+0x0/0x1c) from [<80247d98>] >> > (kobject_init+0x7c/0x9c) >> > [<80247d1c>] (kobject_init+0x0/0x9c) from [<802a3870>] >> > (device_initialize+0x28/0x70) >> > [<802a3848>] (device_initialize+0x0/0x70) from [<802a4798>] >> > (device_register+0x14/0x20) >> > [<802a4784>] (device_register+0x0/0x20) from [<80359a70>] >> > (udc_start+0x180/0x2c8) >> > [<803598f0>] (udc_start+0x0/0x2c8) from [<80356e9c>] >> > (ci_role_work+0xc8/0xec) >> > [<80356dd4>] (ci_role_work+0x0/0xec) from [<8003a8a0>] >> > (process_one_work+0x158/0x474) >> > [<8003a748>] (process_one_work+0x0/0x474) from [<8003af6c>] >> > (worker_thread+0x140/0x3b4) >> > [<8003ae2c>] (worker_thread+0x0/0x3b4) from [<8003fe60>] >> > (kthread+0x90/0x9c) >> > [<8003fdd0>] (kthread+0x0/0x9c) from [<8002774c>] (do_exit+0x0/0x350) >> >> >> >> > Signed-off-by: Richard Zhao >> >> > --- >> >> > drivers/usb/chipidea/udc.c |1 + >> >> > 1 file changed, 1 insertion(+) >> >> > >> >> > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c >> >> > index c7a032a..9fb6394 100644 >> >> > --- a/drivers/usb/chipidea/udc.c >> >> > +++ b/drivers/usb/chipidea/udc.c >> >> > @@ -1746,6 +1746,7 @@ free_pools: >> >> > dma_pool_destroy(ci->td_pool); >> >> > free_qh_pool: >> >> > dma_pool_destroy(ci->qh_pool); >> >> > + memset(&ci->gadget, 0, sizeof(ci->gadget)); >> >> >> >> I understand that you're probably hitting "kobject already initialized" >> >> warning here, but I think the real problem is that this function gets >> >> called twice and fails the first time. Why does that happen? >> > I saw the dump at early stage of adding imx otg support, when there's >> > things not ready. I think it's less important why udc_start fail, >> > because no one enable imx otg before. It's important when it fails, >> > the dump shows up. >> >> I'd prefer that whenever udc_start() fails it doesn't go unnoticed, >> except probably for the cases when it fails due to transceiver not being >> ready at the probe time, which needs to be handled separately. > Is it related to the patch? The patch is logically right that reset the > status, agree? No. This warning means that we have called udc_start() once, failed (thus didn't call udc_stop()) due to some unknown reason, and called it again at a later point in time. This -- failing for unknown reason -- should not happen. If it does, we should fix the reason, not paper over the warning that is telling us that something is wrong. If we decide that we want to allow udc_start to fail for *certain* reasons, we should do it explicitly, not by shutting up all warnings for good. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 05/11] USB: chipidea: don't let probe fail if otg controller start one role failed
Richard Zhao writes:
> On Wed, Aug 29, 2012 at 11:10:33AM +0300, Alexander Shishkin wrote:
>> Richard Zhao writes:
>>
>> > On Tue, Aug 28, 2012 at 11:38:23AM +0300, Alexander Shishkin wrote:
>> >> Richard Zhao writes:
>> >>
>> >> > One role failed, but the other role will possiblly still work.
>> >> > For example, when udc start failed, if plug in a host device,
>> >> > it works.
>> >>
>> >> If you're a host device, ci->role should be HOST at this point and
>> >> ci_role_start() shouldn't fail. If ci->role is detected wrongly, the
>> >> role detection must be fixed. If ci_role_start() fails for host, but it
>> >> still works when it's called again after the id pin change is detected,
>> >> again, the problem is elsewhere.
>> > The check is only for OTG device. For single role device, it just fail
>> > if it start the role failed.
>>
>> Sorry, can you rephrase?
>>
>> >>
>> >> >
>> >> > Signed-off-by: Richard Zhao
>> >> > ---
>> >> > drivers/usb/chipidea/core.c |7 +--
>> >> > 1 file changed, 5 insertions(+), 2 deletions(-)
>> >> >
>> >> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
>> >> > index 19ef324..8fd390a 100644
>> >> > --- a/drivers/usb/chipidea/core.c
>> >> > +++ b/drivers/usb/chipidea/core.c
>> >> > @@ -473,8 +473,11 @@ static int __devinit ci_hdrc_probe(struct
>> >> > platform_device *pdev)
>> >> > 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;
>> >> > + ci->role = CI_ROLE_END;
>> >>
>> >> So are you relying on id pin interrupt for initializing the role after
>> >> this? Can you provide more details?
>> > Yes. The ID pin detect still works. My case is, gadget role failed,
>> > host role works. I was trying not to ruin host function.
>>
>> But it shouldn't even try to start the gadget, because ci_otg_role()
>> should set ci->role to HOST before ci_role_start() happens.
> It depends on ID pin. If ID pin is gadget and gadget is not support well,
> ci_role_start will fail. See below example.
>>
>> Another question is, why does gadget start fail?
> There's one example:
> If usb phy don't support otg yet, otg_set_peripheral will fail, and
> then cause udc_start fail.
So you should drop the CI13XXX_REQUIRE_TRANSCEIVER flag from imx
platform data till the phy is fully implemented.
Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/3] USB: chipidea: add imx usbmisc support
Sascha Hauer writes: > On Wed, Aug 29, 2012 at 10:50:08AM +0300, Alexander Shishkin wrote: >> Richard Zhao writes: >> >> > i.MX usb controllers shares non-core registers, which may include >> > SoC specific controls. We take it as a usbmisc device and usbmisc >> > driver set operations needed by ci13xxx_imx driver. >> > >> > For example, Sabrelite board has bad over-current design, we can >> > usbmisc to disable over-current detect. >> >> Why does this have to be part of the usb driver instead of SoC specific >> code? It looks like you've created a whole new device/driver >> infrastructure just to disable overcurrent for a specific board. > > Richards code indeed only handles overcurrent for a specific board, but > there are more bits to configure in the longer run: power pin > polarities, ULPI/serial mode select and some more. Sounds to me like these things that should be taken care of by the phy driver, which will likely be simpler from both driver's and devicetree's perspective. > >> >> And the infrastructure boils down to a complex way of passing a callback >> from imx driver to another imx driver, that only works if they are >> probed in the right order. I don't see any point in doing it like this >> other than inflating the device tree tables even further. >> >> Why can't this be part of the SoC code like it is done, for example in >> arch/arm/mach-omap2/control.c? > > The settings are board specific, so there must be some way to configure > them in the devicetree. But I'm sure there's a way to control board-specific settings/kludges from devicetree? Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 1/5] usb: chipidea: udc: fix setup of endpoint maxpacket size
Marc Kleine-Budde writes: > From: Michael Grzeschik > > This patch changes the setup of the endpoint maxpacket size. All non control > endpoints are initialized with an undefined ((unsigned short)~0) maxpacket > size. The maxpacket size of Endpoint 0 will be kept at CTRL_PAYLOAD_MAX. > > Some gadget drivers check for the maxpacket size before they enable the > endpoint, which leads to a wrong state in these drivers. > > Cc: > Signed-off-by: Michael Grzeschik > Signed-off-by: Marc Kleine-Budde > Acked-by: Felipe Balbi Acked-by: Alexander Shishkin > --- > changes since v1: > - reworded patch description > - Added Felipe's Ack > > drivers/usb/chipidea/udc.c |8 +++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > index c7a032a..7801a3f 100644 > --- a/drivers/usb/chipidea/udc.c > +++ b/drivers/usb/chipidea/udc.c > @@ -1455,7 +1455,12 @@ static int init_eps(struct ci13xxx *ci) > > mEp->ep.name = mEp->name; > mEp->ep.ops = &usb_ep_ops; > - mEp->ep.maxpacket = CTRL_PAYLOAD_MAX; > + /* > + * for ep0: maxP defined in desc, for other > + * eps, maxP is set by epautoconfig() called > + * by gadget layer > + */ > + mEp->ep.maxpacket = (unsigned short)~0; > > INIT_LIST_HEAD(&mEp->qh.queue); > mEp->qh.ptr = dma_pool_alloc(ci->qh_pool, GFP_KERNEL, > @@ -1475,6 +1480,7 @@ static int init_eps(struct ci13xxx *ci) > else > ci->ep0in = mEp; > > + mEp->ep.maxpacket = CTRL_PAYLOAD_MAX; > continue; > } > > -- > 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/5] usb: chipidea: udc: add pullup fuction, needed by the uvc gadget
Marc Kleine-Budde writes:
> From: Michael Grzeschik
>
> Add function to physicaly enable or disable of pullup connection on the USB-D+
> line. The uvc gaget will fail, if this function is not implemented.
>
> Signed-off-by: Michael Grzeschik
> Signed-off-by: Marc Kleine-Budde
> Acked-by: Felipe Balbi
Acked-by: Alexander Shishkin
> ---
> drivers/usb/chipidea/udc.c | 21 +
> 1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index 7801a3f..32ee870 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -78,8 +78,7 @@ static inline int ep_to_bit(struct ci13xxx *ci, int n)
> }
>
> /**
> - * hw_device_state: enables/disables interrupts & starts/stops device
> (execute
> - * without interruption)
> + * hw_device_state: enables/disables interrupts (execute without
> interruption)
> * @dma: 0 => disable, !0 => enable and set dma engine
> *
> * This function returns an error code
> @@ -91,9 +90,7 @@ 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_USBCMD, USBCMD_RS, 0);
> hw_write(ci, OP_USBINTR, ~0, 0);
> }
> return 0;
> @@ -1420,6 +1417,21 @@ static int ci13xxx_vbus_draw(struct usb_gadget
> *_gadget, unsigned mA)
> return -ENOTSUPP;
> }
>
> +/* Change Data+ pullup status
> + * this func is used by usb_gadget_connect/disconnet
> + */
> +static int ci13xxx_pullup(struct usb_gadget *_gadget, int is_on)
> +{
> + struct ci13xxx *ci = container_of(_gadget, struct ci13xxx, gadget);
> +
> + if (is_on)
> + hw_write(ci, OP_USBCMD, USBCMD_RS, USBCMD_RS);
> + else
> + hw_write(ci, OP_USBCMD, USBCMD_RS, 0);
> +
> + return 0;
> +}
> +
> static int ci13xxx_start(struct usb_gadget *gadget,
>struct usb_gadget_driver *driver);
> static int ci13xxx_stop(struct usb_gadget *gadget,
> @@ -1432,6 +1444,7 @@ static int ci13xxx_stop(struct usb_gadget *gadget,
> static const struct usb_gadget_ops usb_gadget_ops = {
> .vbus_session = ci13xxx_vbus_session,
> .wakeup = ci13xxx_wakeup,
> + .pullup = ci13xxx_pullup,
> .vbus_draw = ci13xxx_vbus_draw,
> .udc_start = ci13xxx_start,
> .udc_stop = ci13xxx_stop,
> --
> 1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/5] usb: chipidea: udc: fix error path in udc_start()
Marc Kleine-Budde writes:
> This patch fixes the error path of udc_start(). Now NULL is used to
> unset the peripheral with otg_set_peripheral().
>
> Signed-off-by: Marc Kleine-Budde
> Reviewed-by: Richard Zhao
Acked-by: Alexander Shishkin
> ---
> drivers/usb/chipidea/udc.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index 32ee870..3a755e5 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -1748,7 +1748,7 @@ static int udc_start(struct ci13xxx *ci)
>
> remove_trans:
> if (!IS_ERR_OR_NULL(ci->transceiver)) {
> - otg_set_peripheral(ci->transceiver->otg, &ci->gadget);
> + otg_set_peripheral(ci->transceiver->otg, NULL);
> if (ci->global_phy)
> usb_put_phy(ci->transceiver);
> }
> --
> 1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/5] usb: chipidea: cleanup dma_pool if udc_start() fails
Marc Kleine-Budde writes:
> If udc_start() fails the qh_pool dma-pool cannot be closed because
> it's still in use. This patch factors out the dma_pool_free() loop
> into destroy_eps() and calls it in the error path of udc_start(),
> too.
>
> Cc: Richard Zhao
> Signed-off-by: Marc Kleine-Budde
Acked-by: Alexander Shishkin
> ---
> drivers/usb/chipidea/udc.c | 23 +++
> 1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index 3a755e5..2d8b609 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -1503,6 +1503,17 @@ static int init_eps(struct ci13xxx *ci)
> return retval;
> }
>
> +static void destroy_eps(struct ci13xxx *ci)
> +{
> + int i;
> +
> + for (i = 0; i < ci->hw_ep_max; i++) {
> + struct ci13xxx_ep *mEp = &ci->ci13xxx_ep[i];
> +
> + dma_pool_free(ci->qh_pool, mEp->qh.ptr, mEp->qh.dma);
> + }
> +}
> +
> /**
> * ci13xxx_start: register a gadget driver
> * @gadget: our gadget
> @@ -1710,7 +1721,7 @@ static int udc_start(struct ci13xxx *ci)
> if (ci->platdata->flags & CI13XXX_REQUIRE_TRANSCEIVER) {
> if (ci->transceiver == NULL) {
> retval = -ENODEV;
> - goto free_pools;
> + goto destroy_eps;
> }
> }
>
> @@ -1761,6 +1772,8 @@ unreg_device:
> put_transceiver:
> if (!IS_ERR_OR_NULL(ci->transceiver) && ci->global_phy)
> usb_put_phy(ci->transceiver);
> +destroy_eps:
> + destroy_eps(ci);
> free_pools:
> dma_pool_destroy(ci->td_pool);
> free_qh_pool:
> @@ -1775,18 +1788,12 @@ free_qh_pool:
> */
> static void udc_stop(struct ci13xxx *ci)
> {
> - int i;
> -
> if (ci == NULL)
> return;
>
> usb_del_gadget_udc(&ci->gadget);
>
> - for (i = 0; i < ci->hw_ep_max; i++) {
> - struct ci13xxx_ep *mEp = &ci->ci13xxx_ep[i];
> -
> - dma_pool_free(ci->qh_pool, mEp->qh.ptr, mEp->qh.dma);
> - }
> + destroy_eps(ci);
>
> dma_pool_destroy(ci->td_pool);
> dma_pool_destroy(ci->qh_pool);
> --
> 1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 5/5] usb: chipidea: udc: don't stall endpoint if request list is empty in isr_tr_complete_low
Marc Kleine-Budde writes:
> From: Michael Grzeschik
>
> When attaching an imx28 or imx53 in USB gadget mode to a Windows host and
> starting a rndis connection we see this message every 4-10 seconds:
>
> g_ether gadget: high speed config #2: RNDIS
>
> Analysis shows that each time this message is printed, the rndis connection is
> re-establish due to a reset because of a stalled endpoint (ep 0, dir 1). The
> endpoint is stalled because the reqeust complete bit on that endpoint is set,
> but in isr_tr_complete_low() the endpoint request list (mEp->qh.queue) is
> empty.
>
> This patch removed this check, because the code doesn't take the following
> situation into account:
>
> The loop over all endpoints in isr_tr_complete_handler() will call ep_nuke()
> on
> both ep0/dir0 and ep/dir1 in the first loop. Pending reqeusts will be flushed
> and completed here. There seems to be a race condition, the request is nuked,
> but the request complete bit will be set, too. The subsequent check (in
> ep0/dir1's loop cycle) for endpoint request list (mEp->qh.queue) empty will
> fail.
>
> Both other mainline chipidea drivers (mv_udc_core.c and fsl_udc_core.c) don't
> have this check.
>
> Signed-off-by: Michael Grzeschik
> Signed-off-by: Marc Kleine-Budde
Makes sense to me,
Acked-by: Alexander Shishkin
> ---
> drivers/usb/chipidea/udc.c |5 +
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index 2d8b609..d214448 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -771,10 +771,7 @@ __acquires(mEp->lock)
> {
> struct ci13xxx_req *mReq, *mReqTemp;
> struct ci13xxx_ep *mEpTemp = mEp;
> - int uninitialized_var(retval);
> -
> - if (list_empty(&mEp->qh.queue))
> - return -EINVAL;
> + int retval = 0;
>
> list_for_each_entry_safe(mReq, mReqTemp, &mEp->qh.queue,
> queue) {
> --
> 1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 2/5] usb: chipidea: udc: add pullup fuction, needed by the uvc gadget
Marc Kleine-Budde writes: > On 09/06/2012 02:26 PM, Alexander Shishkin wrote: >> Marc Kleine-Budde writes: >> >>> From: Michael Grzeschik >>> >>> Add function to physicaly enable or disable of pullup connection on the >>> USB-D+ >>> line. The uvc gaget will fail, if this function is not implemented. >>> >>> Signed-off-by: Michael Grzeschik >>> Signed-off-by: Marc Kleine-Budde >>> Acked-by: Felipe Balbi >> >> Acked-by: Alexander Shishkin > > Thanks. Should we put stable@v.k.o on Cc? I certainly don't see why not. Btw, are you ok with sending these patches to Greg or should I take care of them? My recent awol seems to have caused a pile of patches on Richard's side too, but I don't think they conflict with yours. Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: chipidea driver
Greg KH writes: > On Fri, Aug 24, 2012 at 10:09:11AM +0800, Richard Zhao wrote: >> On Thu, Aug 23, 2012 at 06:57:03PM +0200, Marc Kleine-Budde wrote: >> > Hello, >> > >> > Michael and I have a bunch of updates and improvement for the chipidea >> > driver. They apply to Richard's tree: >> > >> > https://github.com/riczhao/kernel-imx/commits/topics/usb-driver >> > >> > What's the status of these patches? It would be fine if someone queues >> > them for upstream. >> My patches is pending on Alex to review. The otg patch series was sent >> on Jul 12. I don't know whether Alex has been back from vacation, or >> what else I can do. >> >> otg patch: >> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-July/109020.html >> usbmisc: >> http://lists.infradead.org/pipermail/linux-arm-kernel/2012-July/111945.html > > Alex, what is going on with the pending chipidea patches? There seem to > be a lot of them, and I haven't seen any response from you in quite some > time. Indeed there's a pile of patches, I'm going through them right now, Marc already has my acks. We should probably agree if I should collect them all before sending to you or if guys can send them to you directly (as soon as we agree on acks). Regards, -- Alex -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 05/11] USB: chipidea: don't let probe fail if otg controller start one role failed
Richard Zhao writes:
> On Wed, Aug 29, 2012 at 12:48:15PM +0300, Alexander Shishkin wrote:
>> Richard Zhao writes:
>>
>> > On Wed, Aug 29, 2012 at 11:10:33AM +0300, Alexander Shishkin wrote:
>> >> Richard Zhao writes:
>> >>
>> >> > On Tue, Aug 28, 2012 at 11:38:23AM +0300, Alexander Shishkin wrote:
>> >> >> Richard Zhao writes:
>> >> >>
>> >> >> > One role failed, but the other role will possiblly still work.
>> >> >> > For example, when udc start failed, if plug in a host device,
>> >> >> > it works.
>> >> >>
>> >> >> If you're a host device, ci->role should be HOST at this point and
>> >> >> ci_role_start() shouldn't fail. If ci->role is detected wrongly, the
>> >> >> role detection must be fixed. If ci_role_start() fails for host, but it
>> >> >> still works when it's called again after the id pin change is detected,
>> >> >> again, the problem is elsewhere.
>> >> > The check is only for OTG device. For single role device, it just fail
>> >> > if it start the role failed.
>> >>
>> >> Sorry, can you rephrase?
>> >>
>> >> >>
>> >> >> >
>> >> >> > Signed-off-by: Richard Zhao
>> >> >> > ---
>> >> >> > drivers/usb/chipidea/core.c |7 +--
>> >> >> > 1 file changed, 5 insertions(+), 2 deletions(-)
>> >> >> >
>> >> >> > diff --git a/drivers/usb/chipidea/core.c
>> >> >> > b/drivers/usb/chipidea/core.c
>> >> >> > index 19ef324..8fd390a 100644
>> >> >> > --- a/drivers/usb/chipidea/core.c
>> >> >> > +++ b/drivers/usb/chipidea/core.c
>> >> >> > @@ -473,8 +473,11 @@ static int __devinit ci_hdrc_probe(struct
>> >> >> > platform_device *pdev)
>> >> >> > 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;
>> >> >> > +ci->role = CI_ROLE_END;
>> >> >>
>> >> >> So are you relying on id pin interrupt for initializing the role after
>> >> >> this? Can you provide more details?
>> >> > Yes. The ID pin detect still works. My case is, gadget role failed,
>> >> > host role works. I was trying not to ruin host function.
>> >>
>> >> But it shouldn't even try to start the gadget, because ci_otg_role()
>> >> should set ci->role to HOST before ci_role_start() happens.
>> > It depends on ID pin. If ID pin is gadget and gadget is not support well,
>> > ci_role_start will fail. See below example.
>> >>
>> >> Another question is, why does gadget start fail?
>> > There's one example:
>> > If usb phy don't support otg yet, otg_set_peripheral will fail, and
>> > then cause udc_start fail.
>>
>> So you should drop the CI13XXX_REQUIRE_TRANSCEIVER flag from imx
>> platform data till the phy is fully implemented.
> It's just an example. We can not assume udc_start always success. If it
> fails, isn't it better to continue support of host than say game over?
Ok, I think what we need here is to rework error handling around role
switching, so that we can allow certain things to fail and retry at a
later point (say at id pin change), while being careful about the other
failures.
If this patch is not crucial for imx right now, I'd prefer to leave it
out.
Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 1/3] USB: chipidea: add imx usbmisc support
Richard Zhao writes:
> On Wed, Aug 29, 2012 at 10:00:32PM +0200, Marc Kleine-Budde wrote:
>> On 08/29/2012 01:01 PM, Sascha Hauer wrote:
>> > On Wed, Aug 29, 2012 at 01:18:10PM +0300, Alexander Shishkin wrote:
>> >> Sascha Hauer writes:
>> >>
>> >>> On Wed, Aug 29, 2012 at 10:50:08AM +0300, Alexander Shishkin wrote:
>> >>>> Richard Zhao writes:
>> >>>>
>> >>>>> i.MX usb controllers shares non-core registers, which may include
>> >>>>> SoC specific controls. We take it as a usbmisc device and usbmisc
>> >>>>> driver set operations needed by ci13xxx_imx driver.
>> >>>>>
>> >>>>> For example, Sabrelite board has bad over-current design, we can
>> >>>>> usbmisc to disable over-current detect.
>> >>>>
>> >>>> Why does this have to be part of the usb driver instead of SoC specific
>> >>>> code? It looks like you've created a whole new device/driver
>> >>>> infrastructure just to disable overcurrent for a specific board.
>> >>>
>> >>> Richards code indeed only handles overcurrent for a specific board, but
>> >>> there are more bits to configure in the longer run: power pin
>> >>> polarities, ULPI/serial mode select and some more.
>>
>> We already have a patch adding a usbmisc_imx53_init() function to that
>> driver.
>>
>> >> Sounds to me like these things that should be taken care of by the phy
>> >> driver, which will likely be simpler from both driver's and devicetree's
>> >> perspective.
>> >
>> > Most i.MX SoCs have three instances of the chipidea core. These cores
>> > share a single register space for controlling the mentioned bits (the
>> > usbmisc register space). The usbmisc looks different on the different
>> > SoCs.
>> > Indeed they control some phy specific aspects, but the phy itself may
>> > also be an external ULPI or UTMI phy with a separate driver. So if we
>> > integrate the usbmisc into the phy wouldn't that mean that it has to
>> > be integrated into all possible phy drivers?
>> >
>> > From a devicetrees perspective it makes sense to integrate the flags
>> > into the chipidea nodes, because there is one node per chipidea core,
>> > but only one usbmisc unit for all ports on the SoC. So we can do a:
>> >
>> >chipidea@ofs {
>> >disable-overcurrent = <1>;
>> >};
>> >
>> > instead of
>> >
>> >usbmisc@ofs {
>> >disable-overcurrent-port0 = <1>;
>> >disable-overcurrent-port1 = <0>;
>> >...
>> >};
>>
>> +1
>>
>> IMHO looks much cleaner.
> So, Marc agree on the patch too. Maybe you can give a reviewed-by? :)
>
> Hi Alex,
>
> What do you think?
Weeell, as long as this is contained in platform specific code and arm
bits, I can tolerate it.
What about the other two patches? I guess they can go through arm tree
so we don't have to send it through Greg, right?
Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 05/11] USB: chipidea: don't let probe fail if otg controller start one role failed
Richard Zhao writes:
> On Tue, Sep 11, 2012 at 10:23:55AM +0300, Alexander Shishkin wrote:
>> Richard Zhao writes:
>>
>> > On Wed, Aug 29, 2012 at 12:48:15PM +0300, Alexander Shishkin wrote:
>> >> Richard Zhao writes:
>> >>
>> >> > On Wed, Aug 29, 2012 at 11:10:33AM +0300, Alexander Shishkin wrote:
>> >> >> Richard Zhao writes:
>> >> >>
>> >> >> > On Tue, Aug 28, 2012 at 11:38:23AM +0300, Alexander Shishkin wrote:
>> >> >> >> Richard Zhao writes:
>> >> >> >>
>> >> >> >> > One role failed, but the other role will possiblly still work.
>> >> >> >> > For example, when udc start failed, if plug in a host device,
>> >> >> >> > it works.
>> >> >> >>
>> >> >> >> If you're a host device, ci->role should be HOST at this point and
>> >> >> >> ci_role_start() shouldn't fail. If ci->role is detected wrongly, the
>> >> >> >> role detection must be fixed. If ci_role_start() fails for host,
>> >> >> >> but it
>> >> >> >> still works when it's called again after the id pin change is
>> >> >> >> detected,
>> >> >> >> again, the problem is elsewhere.
>> >> >> > The check is only for OTG device. For single role device, it just
>> >> >> > fail
>> >> >> > if it start the role failed.
>> >> >>
>> >> >> Sorry, can you rephrase?
>> >> >>
>> >> >> >>
>> >> >> >> >
>> >> >> >> > Signed-off-by: Richard Zhao
>> >> >> >> > ---
>> >> >> >> > drivers/usb/chipidea/core.c |7 +--
>> >> >> >> > 1 file changed, 5 insertions(+), 2 deletions(-)
>> >> >> >> >
>> >> >> >> > diff --git a/drivers/usb/chipidea/core.c
>> >> >> >> > b/drivers/usb/chipidea/core.c
>> >> >> >> > index 19ef324..8fd390a 100644
>> >> >> >> > --- a/drivers/usb/chipidea/core.c
>> >> >> >> > +++ b/drivers/usb/chipidea/core.c
>> >> >> >> > @@ -473,8 +473,11 @@ static int __devinit ci_hdrc_probe(struct
>> >> >> >> > platform_device *pdev)
>> >> >> >> > 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;
>> >> >> >> > + ci->role = CI_ROLE_END;
>> >> >> >>
>> >> >> >> So are you relying on id pin interrupt for initializing the role
>> >> >> >> after
>> >> >> >> this? Can you provide more details?
>> >> >> > Yes. The ID pin detect still works. My case is, gadget role failed,
>> >> >> > host role works. I was trying not to ruin host function.
>> >> >>
>> >> >> But it shouldn't even try to start the gadget, because ci_otg_role()
>> >> >> should set ci->role to HOST before ci_role_start() happens.
>> >> > It depends on ID pin. If ID pin is gadget and gadget is not support
>> >> > well,
>> >> > ci_role_start will fail. See below example.
>> >> >>
>> >> >> Another question is, why does gadget start fail?
>> >> > There's one example:
>> >> > If usb phy don't support otg yet, otg_set_peripheral will fail, and
>> >> > then cause udc_start fail.
>> >>
>> >> So you should drop the CI13XXX_REQUIRE_TRANSCEIVER flag from imx
>> >> platform data till the phy is fully implemented.
>> > It's just an example. We can not assume udc_start always success. If it
>> > fails, isn't it better to continue support of host than say game over?
>>
>> Ok, I think what we need here is to rework error handling around role
>> switching, so that we can allow certain things to fail and retry at a
>> later point (say at id pin change), while being careful about the other
>> failures.
>>
>> If this patch is not crucial for imx right now, I'd prefer to leave it
>> out.
> That's OK. but I hope other function related patch can be merged for
> 3.7 :)
Also 6/11 seems quite pointless to me. Otherwise, I'll send them to Greg
today.
Regards,
--
Alex
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: move children deallocation after quiescing the hub
Commit ff823c79a5c33194c2e5594f7c4686ea3547910c ("usb: move children
to struct usb_port") forgot to consider the hub_disconnect sequence,
which releases ports before quiescing the hub, which will lead to a
use-after-free, since hub_quiesce() will try to disconnect ports'
children, which are already deallocated. Simple modprobe dummy_hcd &&
rmmod dummy_hcd will illustrate the problem.
This patch moves deallocation of hub's ports after hub_quiesce() call
in hub_disconnect().
Cc: Lan Tianyu
Signed-off-by: Alexander Shishkin
---
drivers/usb/core/hub.c |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index aa45e43..6dc41c6 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -1584,9 +1584,6 @@ static void hub_disconnect(struct usb_interface *intf)
struct usb_device *hdev = interface_to_usbdev(intf);
int i;
- for (i = 0; i < hdev->maxchild; i++)
- usb_hub_remove_port_device(hub, i + 1);
-
/* Take the hub off the event list and don't let it be added again */
spin_lock_irq(&hub_event_lock);
if (!list_empty(&hub->event_list)) {
@@ -1601,6 +1598,9 @@ static void hub_disconnect(struct usb_interface *intf)
hub_quiesce(hub, HUB_DISCONNECT);
usb_set_intfdata (intf, NULL);
+
+ for (i = 0; i < hdev->maxchild; i++)
+ usb_hub_remove_port_device(hub, i + 1);
hub->hdev->maxchild = 0;
if (hub->hdev->speed == USB_SPEED_HIGH)
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 00/12] usb: chipidea updates for v3.7
Hi, This bunch of patches contains fixes in the core driver and updates for chipidea integrated in imx chips. Marc Kleine-Budde (2): usb: chipidea: udc: fix error path in udc_start() usb: chipidea: cleanup dma_pool if udc_start() fails Michael Grzeschik (3): usb: chipidea: udc: fix setup of endpoint maxpacket size usb: chipidea: udc: add pullup fuction, needed by the uvc gadget usb: chipidea: udc: don't stall endpoint if request list is empty in isr_tr_complete_low Richard Zhao (7): USB: chipidea: add imx usbmisc support USB: chipidea: imx: add pinctrl support USB: chipidea: delay 2ms before read ID status at probe time USB: chipidea: acknowledge ID change interrupt in irq handler USB: chipidea: add vbus detect for udc USB: chipidea: add -DDEBUG if debugging is enabled USB: chipidea: re-order irq handling to avoid unhandled irqs .../devicetree/bindings/usb/ci13xxx-imx.txt|5 + .../devicetree/bindings/usb/usbmisc-imx.txt| 14 ++ drivers/usb/chipidea/Makefile |4 +- drivers/usb/chipidea/ci.h |1 + drivers/usb/chipidea/ci13xxx_imx.c | 71 + drivers/usb/chipidea/ci13xxx_imx.h | 28 drivers/usb/chipidea/core.c| 24 +-- drivers/usb/chipidea/udc.c | 98 +--- drivers/usb/chipidea/usbmisc_imx6q.c | 162 9 files changed, 378 insertions(+), 29 deletions(-) create mode 100644 Documentation/devicetree/bindings/usb/usbmisc-imx.txt create mode 100644 drivers/usb/chipidea/ci13xxx_imx.h create mode 100644 drivers/usb/chipidea/usbmisc_imx6q.c -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 01/12] usb: chipidea: udc: fix setup of endpoint maxpacket size
From: Michael Grzeschik This patch changes the setup of the endpoint maxpacket size. All non control endpoints are initialized with an undefined ((unsigned short)~0) maxpacket size. The maxpacket size of Endpoint 0 will be kept at CTRL_PAYLOAD_MAX. Some gadget drivers check for the maxpacket size before they enable the endpoint, which leads to a wrong state in these drivers. Cc: Signed-off-by: Michael Grzeschik Acked-by: Felipe Balbi Signed-off-by: Marc Kleine-Budde Signed-off-by: Alexander Shishkin --- drivers/usb/chipidea/udc.c |8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c index c7a032a..7801a3f 100644 --- a/drivers/usb/chipidea/udc.c +++ b/drivers/usb/chipidea/udc.c @@ -1455,7 +1455,12 @@ static int init_eps(struct ci13xxx *ci) mEp->ep.name = mEp->name; mEp->ep.ops = &usb_ep_ops; - mEp->ep.maxpacket = CTRL_PAYLOAD_MAX; + /* +* for ep0: maxP defined in desc, for other +* eps, maxP is set by epautoconfig() called +* by gadget layer +*/ + mEp->ep.maxpacket = (unsigned short)~0; INIT_LIST_HEAD(&mEp->qh.queue); mEp->qh.ptr = dma_pool_alloc(ci->qh_pool, GFP_KERNEL, @@ -1475,6 +1480,7 @@ static int init_eps(struct ci13xxx *ci) else ci->ep0in = mEp; + mEp->ep.maxpacket = CTRL_PAYLOAD_MAX; continue; } -- 1.7.10.4 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 02/12] usb: chipidea: udc: add pullup fuction, needed by the uvc gadget
From: Michael Grzeschik
Add function to physicaly enable or disable of pullup connection on the USB-D+
line. The uvc gaget will fail, if this function is not implemented.
Cc:
Signed-off-by: Michael Grzeschik
Acked-by: Felipe Balbi
Signed-off-by: Marc Kleine-Budde
Signed-off-by: Alexander Shishkin
---
drivers/usb/chipidea/udc.c | 21 +
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 7801a3f..32ee870 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -78,8 +78,7 @@ static inline int ep_to_bit(struct ci13xxx *ci, int n)
}
/**
- * hw_device_state: enables/disables interrupts & starts/stops device (execute
- * without interruption)
+ * hw_device_state: enables/disables interrupts (execute without interruption)
* @dma: 0 => disable, !0 => enable and set dma engine
*
* This function returns an error code
@@ -91,9 +90,7 @@ 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_USBCMD, USBCMD_RS, 0);
hw_write(ci, OP_USBINTR, ~0, 0);
}
return 0;
@@ -1420,6 +1417,21 @@ static int ci13xxx_vbus_draw(struct usb_gadget *_gadget,
unsigned mA)
return -ENOTSUPP;
}
+/* Change Data+ pullup status
+ * this func is used by usb_gadget_connect/disconnet
+ */
+static int ci13xxx_pullup(struct usb_gadget *_gadget, int is_on)
+{
+ struct ci13xxx *ci = container_of(_gadget, struct ci13xxx, gadget);
+
+ if (is_on)
+ hw_write(ci, OP_USBCMD, USBCMD_RS, USBCMD_RS);
+ else
+ hw_write(ci, OP_USBCMD, USBCMD_RS, 0);
+
+ return 0;
+}
+
static int ci13xxx_start(struct usb_gadget *gadget,
struct usb_gadget_driver *driver);
static int ci13xxx_stop(struct usb_gadget *gadget,
@@ -1432,6 +1444,7 @@ static int ci13xxx_stop(struct usb_gadget *gadget,
static const struct usb_gadget_ops usb_gadget_ops = {
.vbus_session = ci13xxx_vbus_session,
.wakeup = ci13xxx_wakeup,
+ .pullup = ci13xxx_pullup,
.vbus_draw = ci13xxx_vbus_draw,
.udc_start = ci13xxx_start,
.udc_stop = ci13xxx_stop,
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 03/12] usb: chipidea: udc: fix error path in udc_start()
From: Marc Kleine-Budde
This patch fixes the error path of udc_start(). Now NULL is used to
unset the peripheral with otg_set_peripheral().
Cc:
Reviewed-by: Richard Zhao
Signed-off-by: Marc Kleine-Budde
Signed-off-by: Alexander Shishkin
---
drivers/usb/chipidea/udc.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 32ee870..3a755e5 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -1748,7 +1748,7 @@ static int udc_start(struct ci13xxx *ci)
remove_trans:
if (!IS_ERR_OR_NULL(ci->transceiver)) {
- otg_set_peripheral(ci->transceiver->otg, &ci->gadget);
+ otg_set_peripheral(ci->transceiver->otg, NULL);
if (ci->global_phy)
usb_put_phy(ci->transceiver);
}
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 05/12] usb: chipidea: udc: don't stall endpoint if request list is empty in isr_tr_complete_low
From: Michael Grzeschik
When attaching an imx28 or imx53 in USB gadget mode to a Windows host and
starting a rndis connection we see this message every 4-10 seconds:
g_ether gadget: high speed config #2: RNDIS
Analysis shows that each time this message is printed, the rndis connection is
re-establish due to a reset because of a stalled endpoint (ep 0, dir 1). The
endpoint is stalled because the reqeust complete bit on that endpoint is set,
but in isr_tr_complete_low() the endpoint request list (mEp->qh.queue) is
empty.
This patch removed this check, because the code doesn't take the following
situation into account:
The loop over all endpoints in isr_tr_complete_handler() will call ep_nuke() on
both ep0/dir0 and ep/dir1 in the first loop. Pending reqeusts will be flushed
and completed here. There seems to be a race condition, the request is nuked,
but the request complete bit will be set, too. The subsequent check (in
ep0/dir1's loop cycle) for endpoint request list (mEp->qh.queue) empty will
fail.
Both other mainline chipidea drivers (mv_udc_core.c and fsl_udc_core.c) don't
have this check.
Cc:
Signed-off-by: Michael Grzeschik
Signed-off-by: Marc Kleine-Budde
Signed-off-by: Alexander Shishkin
---
drivers/usb/chipidea/udc.c |5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
index 2d8b609..d214448 100644
--- a/drivers/usb/chipidea/udc.c
+++ b/drivers/usb/chipidea/udc.c
@@ -771,10 +771,7 @@ __acquires(mEp->lock)
{
struct ci13xxx_req *mReq, *mReqTemp;
struct ci13xxx_ep *mEpTemp = mEp;
- int uninitialized_var(retval);
-
- if (list_empty(&mEp->qh.queue))
- return -EINVAL;
+ int retval = 0;
list_for_each_entry_safe(mReq, mReqTemp, &mEp->qh.queue,
queue) {
--
1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
