usb: host: xhci: Fix Compliance Mode on SN65LVPE502CP Hardware
A message that is not in the archive (sent off-list or not yet received): 00c901cd71a1$4f337c60$ed9a7520$@[email protected]
Hi Sarah,
I'll perform the code style changes you mentioned and will submit the patch.
Regarding your remark about deleting the timer on suspend and re-initializing on resume 'always' (only if the system is subject to the quirk), the reason of doing this is that when the system resumes from suspend, it doesn't matter if all ports had entered U0 before the system suspension, the Compliance Mode issue could hit again after resume, so it is like starting everything again, that is why on the compliance_mode_recovery_timer_init() function I'm reinitializing the 'xhci->port_status_u0' to 0. I'll add a comment mentioning this behavior on the description of the patch and also on the code.
Best Regards,
Alexis Cortes.
Quoted 361 lines
-----Original Message-----
From: Sarah Sharp [mailto:[email protected]]
Sent: Friday, August 03, 2012 12:22 PM
To: Alexis R. Cortes
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: FW: [PATCH] usb: host: xhci: Fix Compliance Mode on SN65LVPE502CP HardwareQuoted 3 lines · Alexis R. Cortes
Hi Sarah,
I have rewritten my code following your recommendations. Could you please
review it and give your comments? If everything is fine I'll proceed to generate the patch and submit it.
I have a couple style comments and one potential code behavior issue.
drivers/usb/host/xhci-hub.c static void xhci_hub_report_link_state(u32 *status,
Quoted 41 lines
The description of my patch is:
----------------------------------------------------------------------
-----
usb: host: xhci: Fix Compliance Mode on SN65LVPE502CP HardwareThis 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.
If that happens on the first insertion, the host controller port will enter in Compliance Mode and NO port status event will be generated (as per xHCI Spec) making impossible to detect this event by software. The port will remain in compliance mode until a warm reset is applied to it.
As a result of this, the port will seem "dead" to the user and no device connections or disconnections will be detected.
For solving this, 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.
Since the issue is being caused by a piece 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_QUIRKhas been added to the xhci stack).This patch applies for these systems:
Vendor: Hewlett-Packard. System Models: Z420, Z620 and Z820.
----------------------------------------------------------------------
-----The 'git diff' of my change is as follows:
drivers/usb/host/xhci-hub.c +0
-0@@ -493,11 +493,47 @@ static void xhci_hub_report_link_state(u32 *status,u32
status_reg)Quoted 10 lines
drivers/usb/host/xhci-hub.c +7
-0* 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 drivers/usb/host/xhci-hub.c static void xhci_hub_report_link_state(u32 *status, · line 512
Quoted 10 lines
drivers/usb/host/xhci-hub.c +5
-0+* 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; }Function comments need a blank line at the top, like so:
/*
* Function comment.
*/
voidxhci_del_comp_mod_timer(struct xhci_hcd *xhci, u32 status, u16 wIndex)Quoted 10 lines
drivers/usb/host/xhci-hub.c +10
-0+/* Function for Compliance Mode Quirk. +* +* This Function verifies if all xhc USB3 ports have entered U0, if +so, +* the compliance mode timer is deleted. A port won't enter +* compliance mode if it has previously entered U0. +*/ +void xhci_del_comp_mod_timer (struct xhci_hcd *xhci,u32 status,u16 +wIndex) { +u32 all_ports_seen_u0 = ((1 << xhci->num_usb3_ports)-1 );drivers/usb/host/xhci-hub.c static void xhci_hub_report_link_state(u32 *status, · line 532
Quoted 10 lines
drivers/usb/host/xhci-hub.c +10
-0+bool port_in_u0 = ((status &PORT_PLS_MASK ) ==XDEV_U0 ); + +if (!(xhci->quirks &XHCI_COMP_MODE_QUIRK )) +return ; + +if ((xhci->port_status_u0 != all_ports_seen_u0) && 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 U0already!\n");
drivers/usb/host/xhci-hub.c static void xhci_hub_report_link_state(u32 *status, · line 533
Quoted 1 line
drivers/usb/host/xhci-hub.c +1
-0+ xhci_dbg(xhci, "Compliance Mode Recovery TimerDeleted.\n");
drivers/usb/host/xhci-hub.c static void xhci_hub_report_link_state(u32 *status,
Quoted 7 lines
drivers/usb/host/xhci-hub.c +5
-0+ } + } +} +int xhci_hub_control (struct usb_hcd *hcd,u16 typeReq,u16 wValue, u16 wIndex, char *buf, u16 wLength) { @@ -645,6 +681,11 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16wValue,
drivers/usb/host/xhci-hub.c
Quoted 5 lines
/* Update Port Link State for super speed ports*/ if (hcd->speed == HCD_USB3) { xhci_hub_report_link_state(&status, temp);drivers/usb/host/xhci-hub.c +2
-0+/* +* Verify if all USB3 Ports Have entered U0 already.
Quoted 29 lines
drivers/usb/host/xhci-hub.c +3
-1+* Delete Compliance Mode Timer if so. +*/ +xhci_del_comp_mod_timer (xhci, temp, wIndex); }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 indexa979cd0..9c18375 100644
drivers/usb/host/xhci.c +7
-0@@ -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,97 @@ 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; +drivers/usb/host/xhci.c static void xhci_msix_sync_irqs(struct xhci_hcd · line 417
Quoted 10 lines
drivers/usb/host/xhci.c +10
-0+ 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->Port%d!\n",
Quoted 11 lines
drivers/usb/host/xhci.c +11
-0+ i + 1); +xhci_dbg (xhci,"Attempting Recovery routine!\n" ); + hcd = xhci->shared_hcd; + +if (hcd->state ==HC_STATE_SUSPENDED ) +usb_hcd_resume_root_hub (hcd); + +usb_hcd_poll_rh_status (hcd); + } + } +drivers/usb/host/xhci.c static void xhci_msix_sync_irqs(struct xhci_hcd · line 438
Quoted 10 lines
drivers/usb/host/xhci.c +10
-0+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 )); +} + +/* +* Quirk to work around issue generated by the SN65LVPE502CP USB3.0 +re-driver +* that causes ports behind that hardware to enter compliance mode sometimes.
drivers/usb/host/xhci.c static void xhci_msix_sync_irqs(struct xhci_hcd · line 449
Quoted 11 lines
drivers/usb/host/xhci.c +11
-0+ * The quirk creates a timer that polls every 2 seconds the link +state of + * each host controller's port and recovers it by issuing a Warm +reset + * if Compliance mode is detected, otherwise the port will become +"dead" (no + * device connections or disconnections will be detected anymore). +Becasue no + * status event is generated when entering compliance mode (per xhci +spec), + * this quirk is needed on systems that have the failing hardwareinstalled.
drivers/usb/host/xhci.c static void xhci_msix_sync_irqs(struct xhci_hcd · line 451
Quoted 2 lines
drivers/usb/host/xhci.c +2
-0+*/ +Don't put a newline between the comment and the function. If someone converts the function comment to kernel doc style, I don't think the tools will pick up the comment properly.
drivers/usb/host/xhci.c static void xhci_msix_sync_irqs(struct xhci_hcd · line 458
Quoted 7 lines
drivers/usb/host/xhci.c +7
-0+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;Quoted 6 lines
drivers/usb/host/xhci.c +6
-0+ 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);drivers/usb/host/xhci.c static void xhci_msix_sync_irqs(struct xhci_hcd · line 474
Quoted 10 lines
drivers/usb/host/xhci.c +10
-0+ xhci_dbg(xhci, "Compliance Mode Recovery Timer +Initialized.\n"); } + +/* +* This function identifies the systems that have installed the +SN65LVPE502CP +* USB3.0 re-driver and that need the Compliance Mode Quirk. +* Systems: +* Vendor: Hewlett-Packard -> System Models: Z420, Z620 and Z820 */ +No newline here either, please.
Quoted 53 lines
drivers/usb/host/xhci.c +34
-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" ))) +return false; + +if (strstr (dmi_product_name,"Z420" ) || +strstr (dmi_product_name,"Z620" ) || +strstr (dmi_product_name,"Z820" )) +return true; + +return false; +} + +static int xhci_all_ports_seen_u0 (struct xhci_hcd *xhci) { +return (xhci->port_status_u0 == ((1 << +xhci->num_usb3_ports)-1 )); } + +/* * Initialize memory for HCD and xHC (one-time init). * @@ -420,6 +512,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 +726,11 @@ 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_all_ports_seen_u0 (xhci)))) +del_timer_sync (&xhci->comp_mode_recovery_timer); +if (xhci->quirks &XHCI_AMD_PLL_FIX )usb_amd_dev_put ();@@ -802,6 +905,16 @@ int xhci_suspend(struct xhci_hcd *xhci) }drivers/usb/host/xhci.c int xhci_suspend(struct xhci_hcd *xhci) · line 915
Quoted 10 lines
drivers/usb/host/xhci.c +8
-0spin_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_all_ports_seen_u0 (xhci)))) { +del_timer_sync (&xhci->comp_mode_recovery_timer); + xhci_dbg(xhci, "Compliance Mode Recovery TimerDeleted!\n");
drivers/usb/host/xhci.c int xhci_suspend(struct xhci_hcd *xhci) · line 917
Quoted 2 lines
drivers/usb/host/xhci.c +2
-0+ } +So you conditionally delete the compliance mode timer here, if there's still a port that hasn't seen U0 (meaning there is a compliance timer running), but...
drivers/usb/host/xhci.c int xhci_resume(struct xhci_hcd *xhci, bool
Quoted 4 lines
drivers/usb/host/xhci.c +0
-0/* step 5: remove core well power */ /* synchronize irq when using MSI-X */ xhci_msix_sync_irqs (xhci);@@ -934,6 +1047,10 @@ int xhci_resume(struct xhci_hcd *xhci, boolhibernated)
drivers/usb/host/xhci.c int xhci_resume(struct xhci_hcd *xhci, bool · line 1053
Quoted 7 lines
drivers/usb/host/xhci.c +4
-0usb_hcd_resume_root_hub (hcd);usb_hcd_resume_root_hub (xhci->shared_hcd); } + +if (xhci->quirks &XHCI_COMP_MODE_QUIRK ) +compliance_mode_recovery_timer_init (xhci); +..you unconditionally re-initialize it here. That means on resume, the timer will always get re-started. I think you want to only reinitialize it if there are ports that haven't seen U0.
Or was that part of the workaround? Do you have to re-start the timer after resume from suspend or hibernate? If ports need to see U0 after resume or they might go into compliance mode, wouldn't you want to reset the
xhci->port_stateto zero?If you do need to re-initialize the timer unconditionally, please add a comment here, and in the description.
Quoted 27 lines
drivers/usb/host/xhci.c +0
-0return retval; }#endif /* CONFIG_PM */ drivers/usb/host/xhci.h index renamed from drivers/usb/host/xhci.h +0
-055c0785..f1e7874 100644
drivers/usb/host/xhci.h +1
-0@@ -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;drivers/usb/host/xhci.h +5
-0+/* 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 */ Quoted 1 line
(END)
Sarah Sharp