Patchwork [U-Boot,v2,1/1] tegra: usb: Fix device enumeration problem of USB1

login
register
mail settings
Submitter Jim Lin
Date June 19, 2012, 8:47 a.m.
Message ID <4B9C9637D5087840A465BDCB251780E9E2D629AF74@HKMAIL02.nvidia.com>
Download mbox | patch
Permalink /patch/165696/
State Superseded
Headers show

Comments

Jim Lin - June 19, 2012, 8:47 a.m.
A known hardware issue of USB1 port where bit 1 (connect status
change) of PORTSC register will be set after issuing Port Reset
(like "usb reset" in u-boot command line).
This will be treated as an error and stops later device enumeration.

Therefore we add a definition in header file and a callback function
to clear that bit after Port Reset in order to proceed later device
enumeration.

CONFIG_USB_EHCI_SUBMIT_ROOT_POST_CALLBACK

Signed-off-by: Jim Lin <jilin@nvidia.com>
---
To reproduce this issue, you can modify board .dts file to set
as the following to build u-boot binary.
"
 usb0 = "/usb@c5000000";
 usb1 = "/usb@c5008000";
"
Install device on USB1 port (address at 0xc5000000).
And run "usb reset" in u-boot console to enumerate device.

Changes in v2:
- Change config name
- Add a callback function at the end of ehci_submit_root() function

 drivers/usb/host/ehci-hcd.c     |    4 ++++
 drivers/usb/host/ehci-tegra.c   |   34 ++++++++++++++++++++++++++++++++++
 drivers/usb/host/ehci.h         |    4 ++++
 include/configs/tegra2-common.h |    8 ++++++++
 4 files changed, 50 insertions(+), 0 deletions(-)
Stephen Warren - June 19, 2012, 5:13 p.m.
On 06/19/2012 02:47 AM, Jim Lin wrote:
> A known hardware issue of USB1 port where bit 1 (connect status
> change) of PORTSC register will be set after issuing Port Reset
> (like "usb reset" in u-boot command line).
> This will be treated as an error and stops later device enumeration.
> 
> Therefore we add a definition in header file and a callback function
> to clear that bit after Port Reset in order to proceed later device
> enumeration.
> 
> CONFIG_USB_EHCI_SUBMIT_ROOT_POST_CALLBACK

This style of fix (adding an optional callback from the USB core) seems
like the idea to me. Thanks.

However, I'm a little worried that it implements the bug workaround at a
different time to the Linux kernel. The kernel does:

* Assert reset to the port
* Poll the port for enable (any time delays are here)
* Clear the CSC bit

So, the CSC bit is cleared a long time after the port is reset.

However, this U-Boot patch does:

* Assert reset to the port
* Clear the CSC bit
* Wait a long time; 200mS
* Check the port is enabled

So, the CSC bit is cleared immediately after the port is reset.

Is this U-Boot fix still guaranteed to solve the problem? In your
testing, how many times did you test without this fix, and what was the
failure percentage. How many times did you test with the fix, and what
was the failure percentage?

I think it'd be safer to execute the callback from inside
hub_port_reset() right before the call to usb_get_port_status(). That
would make the time when CSC gets cleared much more equivalent between
the Linux kernel and U-Boot. The implementation of the callback function
could probably be a lot simpler to (e.g. doesn't need to check which
feature is being set etc.)
Marek Vasut - June 19, 2012, 9:16 p.m.
Dear Stephen Warren,

> On 06/19/2012 02:47 AM, Jim Lin wrote:
> > A known hardware issue of USB1 port where bit 1 (connect status
> > change) of PORTSC register will be set after issuing Port Reset
> > (like "usb reset" in u-boot command line).
> > This will be treated as an error and stops later device enumeration.
> > 
> > Therefore we add a definition in header file and a callback function
> > to clear that bit after Port Reset in order to proceed later device
> > enumeration.
> > 
> > CONFIG_USB_EHCI_SUBMIT_ROOT_POST_CALLBACK

We did such fix in the past by adding weak function. Check ehci-hcd.c

> 
> This style of fix (adding an optional callback from the USB core) seems
> like the idea to me. Thanks.
> 
> However, I'm a little worried that it implements the bug workaround at a
> different time to the Linux kernel. The kernel does:
> 
> * Assert reset to the port
> * Poll the port for enable (any time delays are here)
> * Clear the CSC bit
> 
> So, the CSC bit is cleared a long time after the port is reset.
> 
> However, this U-Boot patch does:
> 
> * Assert reset to the port
> * Clear the CSC bit
> * Wait a long time; 200mS
> * Check the port is enabled
> 
> So, the CSC bit is cleared immediately after the port is reset.
> 
> Is this U-Boot fix still guaranteed to solve the problem? In your
> testing, how many times did you test without this fix, and what was the
> failure percentage. How many times did you test with the fix, and what
> was the failure percentage?
> 
> I think it'd be safer to execute the callback from inside
> hub_port_reset() right before the call to usb_get_port_status(). That
> would make the time when CSC gets cleared much more equivalent between
> the Linux kernel and U-Boot. The implementation of the callback function
> could probably be a lot simpler to (e.g. doesn't need to check which
> feature is being set etc.)

Best regards,
Marek Vasut

Patch

diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 38d6ae0..0b6b656 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -792,7 +792,11 @@  ehci_submit_root(struct usb_device *dev, unsigned long pipe, void *buffer,
 
 	dev->act_len = len;
 	dev->status = 0;
+#ifdef CONFIG_USB_EHCI_SUBMIT_ROOT_POST_CALLBACK
+	return ehci_submit_root_post_callback(dev, pipe, buffer, length, req);
+#else
 	return 0;
+#endif
 
 unknown:
 	debug("requesttype=%x, request=%x, value=%x, index=%x, length=%x\n",
diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index a7e105b..3dd18d3 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -29,6 +29,40 @@ 
 #include <asm/errno.h>
 #include <asm/arch/usb.h>
 
+#ifdef CONFIG_USB_EHCI_SUBMIT_ROOT_POST_CALLBACK
+int
+ehci_submit_root_post_callback(struct usb_device *dev, unsigned long pipe,
+	void *buffer, int length, struct devrequest *req)
+{
+	u16 typeReq;
+	uint32_t reg;
+	uint32_t *status_reg;
+
+	status_reg = (uint32_t *)&hcor->or_portsc[
+						le16_to_cpu(req->index) - 1];
+	if (((u32) status_reg & 0xFFFFC000) != TEGRA_USB1_BASE)
+		return 0;
+	typeReq = req->request | req->requesttype << 8;
+	switch (typeReq) {
+	case USB_REQ_SET_FEATURE | ((USB_DIR_OUT | USB_RT_PORT) << 8):
+		switch (le16_to_cpu(req->value)) {
+		/*
+		 * A known HW issue on USB1 port where bit 1 (Connect Status
+		 * Change) of PORTSC register will be set after issuing Port
+		 * Reset. Clear that bit for later device enumeration.
+		 */
+		case USB_PORT_FEAT_RESET:
+			reg = ehci_readl(status_reg);
+			reg &= ~EHCI_PS_CLEAR;
+			reg |= EHCI_PS_CSC;
+			ehci_writel(status_reg, reg);
+			break;
+		}
+		break;
+	}
+	return 0;
+}
+#endif
 
 /*
  * Create the appropriate control structures to manage
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index cc00ce4..6c5d8f3 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -204,4 +204,8 @@  struct QH {
 int ehci_hcd_init(void);
 int ehci_hcd_stop(void);
 
+#ifdef CONFIG_USB_EHCI_SUBMIT_ROOT_POST_CALLBACK
+int ehci_submit_root_post_callback(struct usb_device *dev, unsigned long pipe,
+	void *buffer, int length, struct devrequest *req);
+#endif
 #endif /* USB_EHCI_H */
diff --git a/include/configs/tegra2-common.h b/include/configs/tegra2-common.h
index 1931179..b4822d4 100644
--- a/include/configs/tegra2-common.h
+++ b/include/configs/tegra2-common.h
@@ -111,6 +111,14 @@ 
 #define CONFIG_EHCI_IS_TDI
 #define CONFIG_EHCI_DCACHE
 
+/*
+ * A known HW issue on USB1 port where bit 1 (Connect Status Change) of PORTSC
+ * register will be set after issuing Port Reset.
+ * This setting is to add a callback function to clear that bit for later
+ * device enumeration.
+ */
+#define CONFIG_USB_EHCI_SUBMIT_ROOT_POST_CALLBACK
+
 /* Total I2C ports on Tegra2 */
 #define TEGRA_I2C_NUM_CONTROLLERS	4