Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932311AbcKYPOX (ORCPT ); Fri, 25 Nov 2016 10:14:23 -0500 Received: from netrider.rowland.org ([192.131.102.5]:35555 "HELO netrider.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1755302AbcKYPNz (ORCPT ); Fri, 25 Nov 2016 10:13:55 -0500 Date: Fri, 25 Nov 2016 10:13:53 -0500 (EST) From: Alan Stern X-X-Sender: stern@netrider.rowland.org To: Changming Huang cc: gregkh@linuxfoundation.org, , , , , Subject: Re: [PATCH v2] fsl/usb: Workarourd for USB erratum-A005697 In-Reply-To: <1480044248-6365-1-git-send-email-jerry.huang@nxp.com> Message-ID: MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2258 Lines: 66 On Fri, 25 Nov 2016, Changming Huang wrote: > The EHCI specification states the following in the SUSP bit description: > In the Suspend state, the port is senstive to resume detection. > Note that the bit status does not change untile the port is suspended and > that there may be a delay in susupending a port if there is a transaction > currently in progress on the USB. > > However, in NXP USBDR controller, the PORTSCx[SUSP] bit changes immediately > when the application sets it and not when the port is actually suspended. > > So the application must wait for at least 10 milliseconds after a port > indicates that it is suspended, to make sure this port has entered > suspended state before initiating this port resume using the Force Port > Resume bit. This bit is for NXP controller, not EHCI compatible. > > Signed-off-by: Changming Huang > Signed-off-by: Ramneek Mehresh > --- > Change in v2: > - move sleep out of spin-lock and add more comment for this workaround This patch is incomplete. > --- a/drivers/usb/host/ehci-hub.c > +++ b/drivers/usb/host/ehci-hub.c > @@ -310,6 +310,13 @@ static int ehci_bus_suspend (struct usb_hcd *hcd) > } > spin_unlock_irq(&ehci->lock); > > + if (changed && ehci_has_fsl_susp_errata(ehci)) > + /* Wait for at least 10 millisecondes to ensure the controller > + * enter the suspend status before initiating a port resume > + * using the Fore Port Resume bit (Not-EHCI compatible). > + */ The proper style for multi-line comments is: /* * Wait for ... * ... */ Also, "Force" is misspelled. > + usleep_range(10000, 20000); > + > if ((changed && ehci->has_tdi_phy_lpm) || fs_idle_delay) { > /* > * Wait for HCD to enter low-power mode or for the bus The patch does not change the code around line 1190 in the original file: /* After above check the port must be connected. * Set appropriate bit thus could put phy into low power * mode if we have tdi_phy_lpm feature */ temp &= ~PORT_WKCONN_E; temp |= PORT_WKDISC_E | PORT_WKOC_E; ehci_writel(ehci, temp | PORT_SUSPEND, status_reg); This code sets the PORT_SUSPEND bit but does not have a 10-ms delay. You need to add a delay here. Alan Stern