From: Jerry Huang <jerry.huang@nxp.com>
To: Alan Stern <stern@rowland.harvard.edu>
CC: "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        Ramneek Mehresh <ramneek.mehresh@nxp.com>,
        "julia.lawall@lip6.fr" <julia.lawall@lip6.fr>,
        Sriram Dash <sriram.dash@nxp.com>,
        "linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v2] fsl/usb: Workarourd for USB erratum-A005697
Thread-Topic: [PATCH v2] fsl/usb: Workarourd for USB erratum-A005697
Thread-Index: AQHSRs2gXD+2/7OzEUi53r13/5o7sqDpz3WAgAQjDHA=
Date: Mon, 28 Nov 2016 06:25:10 +0000
Message-ID: <DB5PR0401MB181328094228AC8BBE5D510AFE8A0@DB5PR0401MB1813.eurprd04.prod.outlook.com>
References: <1480044248-6365-1-git-send-email-jerry.huang@nxp.com>
 <Pine.LNX.4.44L0.1611251008030.1509-100000@netrider.rowland.org>
In-Reply-To: <Pine.LNX.4.44L0.1611251008030.1509-100000@netrider.rowland.org>
Accept-Language: en-US
Content-Language: en-US
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-originalarrivaltime: 28 Nov 2016 06:25:10.6868
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB5PR0401MB1925
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
Content-Transfer-Encoding: 8bit
X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id uAS7uYm6022688
Content-Length: 2770
Lines: 81

Thanks a lot, Alan,
I will send the v3 with your suggestion.

Best Regards
Jerry Huang


-----Original Message-----
From: Alan Stern [mailto:stern@rowland.harvard.edu] 
Sent: Friday, November 25, 2016 11:14 PM
To: Jerry Huang <jerry.huang@nxp.com>
Cc: gregkh@linuxfoundation.org; Ramneek Mehresh <ramneek.mehresh@nxp.com>; julia.lawall@lip6.fr; Sriram Dash <sriram.dash@nxp.com>; linux-usb@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.huang@nxp.com>
> Signed-off-by: Ramneek Mehresh <ramneek.mehresh@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