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. The 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.

---
 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(-)

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;

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 */

1.7.1

Hi Alexis,

Did you run the patch through checkpatch.pl before submitting it?

I think you will get a bunch of important and completely automatic feedback when you do that. Please fix everything that the script mentions.

This is a pretty awful workaround for a teeny tiny hardware error. You're making systems wake up every two seconds. I don't want that on my system. I think making the timer settable would be nice.

Also, the patch does more things than what you describe. It adds a new quirk, and it adds checks to set said quirk for various different laptop models. Each of those changes (add timer+quirk, and add checks to set quirk for laptops) should rather be a separate commit.

Yep, it's not fun, but the timer should only run on particular HP systems. The alternative is "dead" ports. Alex, can you update your description to include that fact?

Greg and I already argued about making the timer settable via a module parameter, and I think the basic answer was that he doesn't want a new modparam for this.

I would really rather the quirk addition and application to some machine be in the same patch. That's how the other quirk patches have been applied in the past.

Sarah Sharp

Can you break the code up a bit so the line limit warnings don't appear? The long line warnings are really indicative of a deeper problem, usually that the parent function has gotten much too long, and should be broken into smaller functions.

The only exception to the long line rule (and one that checkpatch doesn't usually complain about) is that we don't break up lines in the middle of strings. It makes searching for the strings later much harder. So either leave the really long string line, or break it up into two xhci_dbg statements.

In general, warnings from checkpatch.pl stop my patch application and submission work flow, so I don't accept patches with warnings.

For instance:

			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");
                                               }
                                       }
                               }
                       }

Could be broken out into a new function, like so:

void xhci_del_comp_mod_timer(struct xhci_hcd *xhci)

{
	unsigned int all_ports_seen_u0 = ((1 << xhci->num_usb3_ports)-1));
	boolean this_port_in_u0 = ((temp & PORT_PLS_MASK) == XDEV_U0);

	if (!(xhci->quirks & XHCI_COMP_MODE_QUIRK))
		return;

	if (xhci->port_status_u0 != all_ports_seen_u0 && this_port_in_u0) {
		xhci->port_status_u0 |= 1 << wIndex;
		if (xhci->port_status_u0 == all_ports_seen_u0) {
			del_timer_sync(&xhci->comp_mode_recovery_timer);
			xhci_dbg(xhci, "All USB3 ports have entered U0 at least once.\n");
			xhci_dbg(xhci, "Compliance Mode Recovery Timer Deleted.\n");
		}
	}
}

Then you can call that new function in the ugly long indent-heavy function in xhci-hub.c.

Sarah Sharp