[PATCH v2] fsl/usb: Workarourd for USB erratum-A005697

[PATCH v2] fsl/usb: Workarourd for USB erratum-A005697

[Changming Huang]

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

 drivers/usb/host/ehci-fsl.c      |    3 +++
 drivers/usb/host/ehci-hub.c      |    7 +++++++
 drivers/usb/host/ehci.h          |    6 ++++++
 drivers/usb/host/fsl-mph-dr-of.c |    2 ++
 include/linux/fsl_devices.h      |    1 +
 5 files changed, 19 insertions(+)

diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index 9f5ffb6..91701cc 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -286,6 +286,9 @@ static int ehci_fsl_usb_setup(struct ehci_hcd *ehci)
 	if (pdata->has_fsl_erratum_a005275 == 1)
 		ehci->has_fsl_hs_errata = 1;
 
+	if (pdata->has_fsl_erratum_a005697 == 1)
+		ehci->has_fsl_susp_errata = 1;
+
 	if ((pdata->operating_mode == FSL_USB2_DR_HOST) ||
 			(pdata->operating_mode == FSL_USB2_DR_OTG))
 		if (ehci_fsl_setup_phy(hcd, pdata->phy_mode, 0))
diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
index 74f62d6..81e2310 100644
--- 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).
+		 */
+		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
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index 3f3b74a..7706e4a 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -219,6 +219,7 @@ struct ehci_hcd {			/* one per controller */
 	unsigned		no_selective_suspend:1;
 	unsigned		has_fsl_port_bug:1; /* FreeScale */
 	unsigned		has_fsl_hs_errata:1;	/* Freescale HS quirk */
+	unsigned		has_fsl_susp_errata:1;	/*Freescale SUSP quirk*/
 	unsigned		big_endian_mmio:1;
 	unsigned		big_endian_desc:1;
 	unsigned		big_endian_capbase:1;
@@ -703,10 +704,15 @@ struct ehci_tt {
 #if defined(CONFIG_PPC_85xx)
 /* Some Freescale processors have an erratum (USB A-005275) in which
  * incoming packets get corrupted in HS mode
+ * Some Freescale processors have an erratum (USB A-005697) in which
+ * we need to wait for 10ms for bus to fo into suspend mode after
+ * setting SUSP bit
  */
 #define ehci_has_fsl_hs_errata(e)	((e)->has_fsl_hs_errata)
+#define ehci_has_fsl_susp_errata(e)	((e)->has_fsl_susp_errata)
 #else
 #define ehci_has_fsl_hs_errata(e)	(0)
+#define ehci_has_fsl_susp_errata(e)	(0)
 #endif
 
 /*
diff --git a/drivers/usb/host/fsl-mph-dr-of.c b/drivers/usb/host/fsl-mph-dr-of.c
index f07ccb2..e90ddb5 100644
--- a/drivers/usb/host/fsl-mph-dr-of.c
+++ b/drivers/usb/host/fsl-mph-dr-of.c
@@ -226,6 +226,8 @@ static int fsl_usb2_mph_dr_of_probe(struct platform_device *ofdev)
 		of_property_read_bool(np, "fsl,usb-erratum-a007792");
 	pdata->has_fsl_erratum_a005275 =
 		of_property_read_bool(np, "fsl,usb-erratum-a005275");
+	pdata->has_fsl_erratum_a005697 =
+		of_property_read_bool(np, "fsl,usb_erratum-a005697");
 
 	/*
 	 * Determine whether phy_clk_valid needs to be checked
diff --git a/include/linux/fsl_devices.h b/include/linux/fsl_devices.h
index f291291..60cef82 100644
--- a/include/linux/fsl_devices.h
+++ b/include/linux/fsl_devices.h
@@ -100,6 +100,7 @@ struct fsl_usb2_platform_data {
 	unsigned	already_suspended:1;
 	unsigned        has_fsl_erratum_a007792:1;
 	unsigned        has_fsl_erratum_a005275:1;
+	unsigned	has_fsl_erratum_a005697:1;
 	unsigned        check_phy_clk_valid:1;
 
 	/* register save area for suspend/resume */
-- 
1.7.9.5

Re: [PATCH v2] fsl/usb: Workarourd for USB erratum-A005697

[Sergei Shtylyov]

Hello.

On 11/25/2016 06:24 AM, 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.

    Sensitive.

> Note that the bit status does not change untile the port is suspended and

    Until.

> that there may be a delay in susupending a port if there is a transaction

    Suspending.

> 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>

[...]

> diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c
> index 74f62d6..81e2310 100644
> --- 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

    Milliseconds.

> +		 * enter the suspend status before initiating a port resume
> +		 * using the Fore Port Resume bit (Not-EHCI compatible).

    Maybe force?
    s/Not/non/ also.

> +		 */
> +		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
[...]
> @@ -703,10 +704,15 @@ struct ehci_tt {
>  #if defined(CONFIG_PPC_85xx)
>  /* Some Freescale processors have an erratum (USB A-005275) in which
>   * incoming packets get corrupted in HS mode
> + * Some Freescale processors have an erratum (USB A-005697) in which
> + * we need to wait for 10ms for bus to fo into suspend mode after

    Fo?

[...]

MBR, Sergei

Re: [PATCH v2] fsl/usb: Workarourd for USB erratum-A005697

[Alan Stern]

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

RE: [PATCH v2] fsl/usb: Workarourd for USB erratum-A005697

[Jerry Huang]

Thanks a lot  for you look into this patch, I will fixed these typos.


Best Regards
Jerry Huang


-----Original Message-----
From: Sergei Shtylyov [mailto:sergei.shtylyov@cogentembedded.com] 
Sent: Friday, November 25, 2016 9:12 PM
To: Jerry Huang <jerry.huang@nxp.com>; stern@rowland.harvard.edu; gregkh@linuxfoundation.org
Cc: 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

Hello.

On 11/25/2016 06:24 AM, 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.

    Sensitive.

> Note that the bit status does not change untile the port is suspended 
> and

    Until.

> that there may be a delay in susupending a port if there is a 
> transaction

    Suspending.

> 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>

[...]

> diff --git a/drivers/usb/host/ehci-hub.c b/drivers/usb/host/ehci-hub.c 
> index 74f62d6..81e2310 100644
> --- 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

    Milliseconds.

> +		 * enter the suspend status before initiating a port resume
> +		 * using the Fore Port Resume bit (Not-EHCI compatible).

    Maybe force?
    s/Not/non/ also.

> +		 */
> +		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
[...]
> @@ -703,10 +704,15 @@ struct ehci_tt {  #if defined(CONFIG_PPC_85xx)
>  /* Some Freescale processors have an erratum (USB A-005275) in which
>   * incoming packets get corrupted in HS mode
> + * Some Freescale processors have an erratum (USB A-005697) in which
> + * we need to wait for 10ms for bus to fo into suspend mode after

    Fo?

[...]

MBR, Sergei

RE: [PATCH v2] fsl/usb: Workarourd for USB erratum-A005697

[Jerry Huang]

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