Skip to contentSkip to conversation

usb: host: xhci: Fix Compliance Mode on SN65LVPE502CP Hardware

PATCH In review Alexis R. Cortes 3 participants 3 messages Aug 1, 2012

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 usb: host: xhci: Fix Compliance Mode on SN65LVPE502CP Hardware

This patch is intended to work around a known issue on the SN65LVPE502CP USB3.0 re-driver that can delay the negotiation between a device and the host past the usual handshake timeout, and if that happens on the first insertion, the host controller port will enter in Compliance Mode as per xHCI Spec, thus causing the port to become Unusable. This patch creates a timer which polls every 2 seconds the link state of each host controller's port (this by reading the PORTSC register) and recovers the port by issuing a Warm reset every time Compliance mode is detected. Since the issue is being caused by a pice of hardware, the timer will be enabled ONLY on those systems that have the SN65LVPE502CP installed (this patch uses DMI strings for detecting those systems), therefore making this patch to act as a quirk (XHCI_COMP_MODE_QUIRK has been added to the xhci stack).

---
 drivers/usb/host/xhci-hub.c |   28 +++++++++++++++
 drivers/usb/host/xhci.c     |   81 +++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/host/xhci.h     |    6 +++
 3 files changed, 115 insertions(+), 0 deletions(-)
drivers/usb/host/xhci-hub.c +28 -0
index 7b01094..ce4b181 100644
@@ -493,7 +493,19 @@ static void xhci_hub_report_link_state(u32 *status, u32 status_reg)
 		 * when this bit is set.
 		 */
 		pls |= USB_PORT_STAT_CONNECTION;
+	} else {
+		/*
+		 * If CAS bit isn't set but the Port is already at
+		 * Compliance Mode, fake a connection so the USB core
+		 * notices the Compliance state and resets the port.
+		 * This resolves an issue generated by the SN65LVPE502CP
+		 * in which sometimes the port enters compliance mode
+		 * caused by a delay on the host-device negotiation.
+		 */
+		if (pls == USB_SS_PORT_LS_COMP_MOD)
+			pls |= USB_PORT_STAT_CONNECTION;
 	}
+
 	/* update status field */
 	*status |= pls;
 }
@@ -645,6 +657,22 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 		/* Update Port Link State for super speed ports*/
 		if (hcd->speed == HCD_USB3) {
 			xhci_hub_report_link_state(&status, temp);
+			/*
+			 * Verify if all USB3 Ports Have entered U0. Delete compliance
+			 * mode timer if so.
+			 */
+			if (xhci->quirks & XHCI_COMP_MODE_QUIRK) {
+				if (xhci->port_status_u0 != ((1 << xhci->num_usb3_ports)-1)) {
+					if ((temp & PORT_PLS_MASK) == XDEV_U0) {
+						xhci->port_status_u0 |= 1 << wIndex;
+						if (xhci->port_status_u0 == ((1 << xhci->num_usb3_ports)-1)) {
+							del_timer_sync(&xhci->comp_mode_recovery_timer);
+							xhci_dbg(xhci, "Compliance Mode Recovery Timer Deleted. "
+									"All USB3 ports have entered U0 at least once.\n");
+						}
+					}
+				}
+			}
 		}
 		if (bus_state->port_c_suspend & (1 << wIndex))
 			status |= 1 << USB_PORT_FEAT_C_SUSPEND;
drivers/usb/host/xhci.c +81 -0

117 lines not on this page · raw message

drivers/usb/host/xhci.h +6 -0
index 55c0785..f1e7874 100644
@@ -1494,6 +1494,7 @@ struct xhci_hcd {
 #define XHCI_TRUST_TX_LENGTH	(1 << 10)
 #define XHCI_LPM_SUPPORT	(1 << 11)
 #define XHCI_INTEL_HOST		(1 << 12)
+#define XHCI_COMP_MODE_QUIRK	(1 << 13)
 	unsigned int		num_active_eps;
 	unsigned int		limit_active_eps;
 	/* There are two roothubs to keep track of bus suspend info for */
@@ -1510,6 +1511,11 @@ struct xhci_hcd {
 	unsigned		sw_lpm_support:1;
 	/* support xHCI 1.0 spec USB2 hardware LPM */
 	unsigned		hw_lpm_support:1;
+	/* Compliance Mode Recovery Data */
+	struct timer_list	comp_mode_recovery_timer;
+	u32			port_status_u0;
+/* Compliance Mode Timer Triggered every 2 seconds */
+#define COMP_MODE_RCVRY_MSECS 2000
 };
 
 /* convert between an HCD pointer and the corresponding EHCI_HCD */
Signature · 1 line
1.7.1
#1 lore raw
Quoted 10 lines · Alexis R. Cortes

This patch is intended to work around a known issue on the SN65LVPE502CP USB3.0 re-driver that can delay the negotiation between a device and the host past the usual handshake timeout, and if that happens on the first insertion, the host controller port will enter in Compliance Mode as per xHCI Spec, thus causing the port to become Unusable. This patch creates a timer which polls every 2 seconds the link state of each host controller's port (this by reading the PORTSC register) and recovers the port by issuing a Warm reset every time Compliance mode is detected. Since the issue is being caused by a pice of hardware, the timer will be enabled

Typo, insert an 'e' here ^

Quoted 1 line

+++ b/drivers/usb/host/xhci.c

..

drivers/usb/host/xhci.c int xhci_init(struct usb_hcd *hcd) · line 487

Quoted 12 lines
drivers/usb/host/xhci.c +6 -0
@@ -420,6 +477,12 @@ int xhci_init(struct usb_hcd *hcd)
 	retval = xhci_mem_init(xhci, GFP_KERNEL);
 	xhci_dbg(xhci, "Finished xhci_init\n");
 
+	/* Initializing Compliance Mode Recovery Data If Needed */
+	if (compliance_mode_recovery_timer_quirk_check()) {
+		xhci->quirks |= XHCI_COMP_MODE_QUIRK;
+		compliance_mode_recovery_timer_init(xhci);
+	}
+
 	return retval;
 }

Maybe add the code just *before* the "Finished" debug message rather than after.

//Peter

#2 lore raw
In reply to Alexis R. Cortes
Quoted 3 lines · Alexis R. Cortes

This patch is intended to work around a known issue on the SN65LVPE502CP USB3.0 re-driver that can delay the negotiation between a device and the host past the usual handshake timeout,

<br> Paragraphs, please. :)

Quoted 3 lines

if that happens on the first insertion, the host controller port will enter in Compliance Mode as per xHCI Spec, thus causing the port to become Unusable.

Please describe what you mean by "unusable", and what the visible signs to a user would be when they hit this bug. That will help anyone who may be encountering this bug on a system different than the HP ones. For example:

When the port goes into compliance mode, the xHCI driver doesn't get a port status event. The port will remain in compliance mode, and no device connections or disconnections will be detected. The port will seem "dead" to the user. lsusb will report the port is in compliance mode.

<br>

Quoted 4 lines

This patch creates a timer which polls
every 2 seconds the link state of each host controller's port (this by reading the PORTSC register) and recovers the port by issuing a Warm reset every time Compliance mode is detected.

<br>

Quoted 6 lines

Since the issue
is being caused by a pice of hardware, the timer will be enabled ONLY on those systems that have the SN65LVPE502CP installed (this patch uses DMI strings for detecting those systems), therefore making this patch to act as a quirk (XHCI_COMP_MODE_QUIRK has been added to the xhci stack).

Please mention the exact systems you're enabling them for; don't just say the DMI strings are in the code. After all, you could have meant a different system than you coded for. Plus, then people will be able to quickly know whether this patch applies to their system just by looking at the patch description.

More coding style comments below.

Quoted 73 lines
---
 drivers/usb/host/xhci-hub.c |   28 +++++++++++++++
 drivers/usb/host/xhci.c     |   81 +++++++++++++++++++++++++++++++++++++++++++
 drivers/usb/host/xhci.h     |    6 +++
 3 files changed, 115 insertions(+), 0 deletions(-)
drivers/usb/host/xhci-hub.c +28 -0
index 7b01094..ce4b181 100644
@@ -493,7 +493,19 @@ static void xhci_hub_report_link_state(u32 *status, u32 status_reg)
 		 * when this bit is set.
 		 */
 		pls |= USB_PORT_STAT_CONNECTION;
+	} else {
+		/*
+		 * If CAS bit isn't set but the Port is already at
+		 * Compliance Mode, fake a connection so the USB core
+		 * notices the Compliance state and resets the port.
+		 * This resolves an issue generated by the SN65LVPE502CP
+		 * in which sometimes the port enters compliance mode
+		 * caused by a delay on the host-device negotiation.
+		 */
+		if (pls == USB_SS_PORT_LS_COMP_MOD)
+			pls |= USB_PORT_STAT_CONNECTION;
 	}
+
 	/* update status field */
 	*status |= pls;
 }
@@ -645,6 +657,22 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 		/* Update Port Link State for super speed ports*/
 		if (hcd->speed == HCD_USB3) {
 			xhci_hub_report_link_state(&status, temp);
+			/*
+			 * Verify if all USB3 Ports Have entered U0. Delete compliance
+			 * mode timer if so.
+			 */
+			if (xhci->quirks & XHCI_COMP_MODE_QUIRK) {
+				if (xhci->port_status_u0 != ((1 << xhci->num_usb3_ports)-1)) {
+					if ((temp & PORT_PLS_MASK) == XDEV_U0) {
+						xhci->port_status_u0 |= 1 << wIndex;
+						if (xhci->port_status_u0 == ((1 << xhci->num_usb3_ports)-1)) {
+							del_timer_sync(&xhci->comp_mode_recovery_timer);
+							xhci_dbg(xhci, "Compliance Mode Recovery Timer Deleted. "
+									"All USB3 ports have entered U0 at least once.\n");
+						}
+					}
+				}
+			}
 		}
 		if (bus_state->port_c_suspend & (1 << wIndex))
 			status |= 1 << USB_PORT_FEAT_C_SUSPEND;
drivers/usb/host/xhci.c +4 -0
index a979cd0..fea738b 100644
@@ -26,6 +26,7 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/slab.h>
+#include <linux/dmi.h>
 
 #include "xhci.h"
 
@@ -397,6 +398,62 @@ static void xhci_msix_sync_irqs(struct xhci_hcd *xhci)
 
 #endif
 
+static void compliance_mode_recovery(unsigned long arg)
+{
+	struct xhci_hcd *xhci;

drivers/usb/host/xhci.c static void xhci_msix_sync_irqs(struct xhci_hcd *xhci) · line 413

Quoted 10 lines
drivers/usb/host/xhci.c +10 -0
+	struct usb_hcd *hcd;
+	u32 temp;
+	int i;
+
+	xhci = (struct xhci_hcd *)arg;
+
+	for (i = 0; i < xhci->num_usb3_ports; i++) {
+		temp = xhci_readl(xhci, xhci->usb3_ports[i]);
+		if ((temp & PORT_PLS_MASK) == USB_SS_PORT_LS_COMP_MOD) {
+			/* Compliance Mode Detected. Letting USB Core handle the Warm Reset */

Comment too long, please break it up like so:

			/*
			 * This is a really long comment.
			 * With two lines.
			 */

drivers/usb/host/xhci.c static void xhci_msix_sync_irqs(struct xhci_hcd *xhci) · line 414

Quoted 1 line
drivers/usb/host/xhci.c +1 -0
+			xhci_dbg(xhci, "Compliance Mode Detected on port %d! Attempting recovery routine.\n", i + 1);

dmesg output should be short and sweet, hopefully less than 80 characters long. Try breaking it up into separate xhci_dbg statements.

drivers/usb/host/xhci.c static void xhci_msix_sync_irqs(struct xhci_hcd *xhci) · line 425

Quoted 11 lines
drivers/usb/host/xhci.c +11 -0
+			hcd = xhci->shared_hcd;
+
+			if (hcd->state == HC_STATE_SUSPENDED)
+				usb_hcd_resume_root_hub(hcd);
+
+			usb_hcd_poll_rh_status(hcd);
+		}
+	}
+
+	if (xhci->port_status_u0 != ((1 << xhci->num_usb3_ports)-1))
+		mod_timer(&xhci->comp_mode_recovery_timer, jiffies + msecs_to_jiffies(COMP_MODE_RCVRY_MSECS));

Please break this function call into multiple lines.

drivers/usb/host/xhci.c static void xhci_msix_sync_irqs(struct xhci_hcd *xhci) · line 431

Quoted 6 lines
drivers/usb/host/xhci.c +6 -0
+}
+
+static void compliance_mode_recovery_timer_init(struct xhci_hcd *xhci)
+{
+	xhci->port_status_u0 = 0;
+	init_timer(&xhci->comp_mode_recovery_timer);

<br>

drivers/usb/host/xhci.c static void xhci_msix_sync_irqs(struct xhci_hcd *xhci) · line 434

Quoted 3 lines
drivers/usb/host/xhci.c +3 -0
+	xhci->comp_mode_recovery_timer.data = (unsigned long) xhci;
+	xhci->comp_mode_recovery_timer.function = compliance_mode_recovery;
+	xhci->comp_mode_recovery_timer.expires = jiffies + msecs_to_jiffies(COMP_MODE_RCVRY_MSECS);

<br>

drivers/usb/host/xhci.c static void xhci_msix_sync_irqs(struct xhci_hcd *xhci) · line 439

Quoted 5 lines
drivers/usb/host/xhci.c +5 -0
+	set_timer_slack(&xhci->comp_mode_recovery_timer, msecs_to_jiffies(COMP_MODE_RCVRY_MSECS));
+	add_timer(&xhci->comp_mode_recovery_timer);
+	xhci_dbg(xhci, "Compliance Mode Recovery Timer Initialized.\n");
+}
+

Please add a function comment to briefly describe what this quirk does, and why you need it. Then someone that's just reading the code won't have to dig out the commit that added it.

Quoted 34 lines
drivers/usb/host/xhci.c +23 -0
+static bool compliance_mode_recovery_timer_quirk_check(void)
+{
+	const char *dmi_product_name, *dmi_sys_vendor;
+
+	dmi_product_name = dmi_get_system_info(DMI_PRODUCT_NAME);
+	dmi_sys_vendor = dmi_get_system_info(DMI_SYS_VENDOR);
+
+	if (strstr(dmi_sys_vendor, "Hewlett-Packard")) {
+		if (strstr(dmi_product_name, "Z420") || strstr(dmi_product_name, "Z620") ||
+				strstr(dmi_product_name, "Z820")) {
+			return true;
+		}
+	}
+
+	return false;
+}
+
 /*
  * Initialize memory for HCD and xHC (one-time init).
  *
@@ -420,6 +477,12 @@ int xhci_init(struct usb_hcd *hcd)
 	retval = xhci_mem_init(xhci, GFP_KERNEL);
 	xhci_dbg(xhci, "Finished xhci_init\n");
 
+	/* Initializing Compliance Mode Recovery Data If Needed */
+	if (compliance_mode_recovery_timer_quirk_check()) {
+		xhci->quirks |= XHCI_COMP_MODE_QUIRK;
+		compliance_mode_recovery_timer_init(xhci);
+	}
+
 	return retval;
 }
 

drivers/usb/host/xhci.c void xhci_stop(struct usb_hcd *hcd) · line 695

Quoted 5 lines
drivers/usb/host/xhci.c +2 -0
@@ -628,6 +691,10 @@ void xhci_stop(struct usb_hcd *hcd)
 	del_timer_sync(&xhci->event_ring_timer);
 #endif
 
+	/* Deleting Compliance Mode Recovery Timer */
+	if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) && (xhci->port_status_u0 != ((1 << xhci->num_usb3_ports)-1)))

Ok, you're using this conditional enough times that you need a separate function call for it. E.g.

int xhci_all_ports_seen_u0(struct xhci_hcd *xhci)

{
	return xhci->port_status_u0 == ((1 << xhci->num_usb3_ports) - 1);
}

Then if you find later you've gotten the math wrong, you only have to change it in one place.

Quoted 6 lines
drivers/usb/host/xhci.c +2 -0
+		del_timer_sync(&xhci->comp_mode_recovery_timer);
+
 	if (xhci->quirks & XHCI_AMD_PLL_FIX)
 		usb_amd_dev_put();
 

drivers/usb/host/xhci.c int xhci_suspend(struct xhci_hcd *xhci) · line 876

Quoted 8 lines
drivers/usb/host/xhci.c +5 -0
@@ -802,6 +869,15 @@ int xhci_suspend(struct xhci_hcd *xhci)
 	}
 	spin_unlock_irq(&xhci->lock);
 
+	/*
+	 * Deleting Compliance Mode Recovery Timer because the xHCI Host
+	 * is about to be suspended.
+	 */
+	if ((xhci->quirks & XHCI_COMP_MODE_QUIRK) && (xhci->port_status_u0 != ((1 << xhci->num_usb3_ports)-1))) {

Another instance where you use that conditional.

drivers/usb/host/xhci.c int xhci_resume(struct xhci_hcd *xhci, bool hibernated) · line 1014

Quoted 13 lines
drivers/usb/host/xhci.c +6 -0
+		del_timer_sync(&xhci->comp_mode_recovery_timer);
+		xhci_dbg(xhci, "Compliance Mode Recovery Timer Deleted - xHC suspended\n");
+	}
+
 	/* step 5: remove core well power */
 	/* synchronize irq when using MSI-X */
 	xhci_msix_sync_irqs(xhci);
@@ -934,6 +1010,11 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
 		usb_hcd_resume_root_hub(hcd);
 		usb_hcd_resume_root_hub(xhci->shared_hcd);
 	}
+
+	/* Initializing Compliance Mode Recovery Timer */

The function name is pretty self explanatory, so you don't need this comment.

Quoted 33 lines
drivers/usb/host/xhci.c +3 -0
+	if (xhci->quirks & XHCI_COMP_MODE_QUIRK)
+		compliance_mode_recovery_timer_init(xhci);
+
 	return retval;
 }
 #endif	/* CONFIG_PM */
drivers/usb/host/xhci.h +6 -0
index 55c0785..f1e7874 100644
@@ -1494,6 +1494,7 @@ struct xhci_hcd {
 #define XHCI_TRUST_TX_LENGTH	(1 << 10)
 #define XHCI_LPM_SUPPORT	(1 << 11)
 #define XHCI_INTEL_HOST		(1 << 12)
+#define XHCI_COMP_MODE_QUIRK	(1 << 13)
 	unsigned int		num_active_eps;
 	unsigned int		limit_active_eps;
 	/* There are two roothubs to keep track of bus suspend info for */
@@ -1510,6 +1511,11 @@ struct xhci_hcd {
 	unsigned		sw_lpm_support:1;
 	/* support xHCI 1.0 spec USB2 hardware LPM */
 	unsigned		hw_lpm_support:1;
+	/* Compliance Mode Recovery Data */
+	struct timer_list	comp_mode_recovery_timer;
+	u32			port_status_u0;
+/* Compliance Mode Timer Triggered every 2 seconds */
+#define COMP_MODE_RCVRY_MSECS 2000
 };
 
 /* convert between an HCD pointer and the corresponding EHCI_HCD */
Signature · 1 line
1.7.1

Sarah Sharp