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.
Signed-off-by: Alexis R. Cortes <[email protected]>
---
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(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 7b01094..ce4b181 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -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;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index a979cd0..fea738b 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -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;
+ 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 */
+ xhci_dbg(xhci, "Compliance Mode Detected on port %d! Attempting recovery routine.\n", i + 1);
+ 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));
+}
+
+static void compliance_mode_recovery_timer_init(struct xhci_hcd *xhci)
+{
+ xhci->port_status_u0 = 0;
+ init_timer(&xhci->comp_mode_recovery_timer);
+ 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);
+ 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");
+}
+
+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;
}
@@ -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)))
+ del_timer_sync(&xhci->comp_mode_recovery_timer);
+
if (xhci->quirks & XHCI_AMD_PLL_FIX)
usb_amd_dev_put();
@@ -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))) {
+ 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 */
+ if (xhci->quirks & XHCI_COMP_MODE_QUIRK)
+ compliance_mode_recovery_timer_init(xhci);
+
return retval;
}
#endif /* CONFIG_PM */
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 55c0785..f1e7874 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -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.
Alexis R. Cortes wrote:
> 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.
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.
> @@ -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");
> + }
> + }
> + }
> + }
> }
Is this style consistent with the surrounding code? I would be
surprised. The kernel frequently uses the "early exits" concept.
Please try to embrace it, I think it makes for lovely readable code.
A quote from
http://www.cranked.me/2008/07/spartan-programming-real-man-way-to-do.html
--8<--
Spartan programming strives for simultaneous minimization of all of
the following measures of code complexity:
1. horizontal complexity, that is, the depth of nesting of control
structures, just as the total line length.
..
8. conditionals, that is the number of if and multiple branch
switch statements.
-->8--
This may not be in http://kernel.org/doc/Documentation/CodingStyle
per se, but Chapter 7: Centralized exiting of functions touches on
the issue, includes an example, and I've found it used a lot in the
kernel code. It's a very good idea.
> +++ b/drivers/usb/host/xhci.h
> @@ -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
I'm surprised how this get hardcoded from a header file.
I for one would like it to be settable.
Thanks
//Peter
On Wed, Aug 01, 2012 at 10:01:31PM +0200, Peter Stuge wrote:
> 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.
>
>
> Alexis R. Cortes wrote:
> > 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.
>
> 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.
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.
> 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.
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