Thanks a lot, Alan, I will send the v3 with your suggestion. Best Regards Jerry Huang
-----Original Message----- From: Alan Stern [mailto:st...@rowland.harvard.edu] Sent: Friday, November 25, 2016 11:14 PM To: Jerry Huang <jerry.hu...@nxp.com> Cc: gre...@linuxfoundation.org; Ramneek Mehresh <ramneek.mehr...@nxp.com>; julia.law...@lip6.fr; Sriram Dash <sriram.d...@nxp.com>; linux-...@vger.kernel.org; linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] fsl/usb: Workarourd for USB erratum-A005697 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 <jerry.hu...@nxp.com> > Signed-off-by: Ramneek Mehresh <ramneek.mehr...@nxp.com> > --- > 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
