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

2015-07-27 Thread Alan Stern 
On Mon, 27 Jul 2015, Nikhil Badola wrote:

> As per USB specification, in the Suspend state, the status bit does
> not change until the port is suspended. However, there may be a delay
> in suspending a port if there is a transaction currently in progress
> on the bus.
> 
> In the USBDR controller, the PORTSCx[SUSP] bit changes immediately when
> the application sets it and not when the port is actually suspended
> 
> Workaround for this issue involves waiting for a minimum of 10ms to
> allow the controller to go into SUSPEND state before proceeding ahead

Why is this delay needed?  When ehci_bus_suspend() is called there are 
never any transactions in progress, except possibly for a SOF 
transaction (which takes less than 1 us, nowhere near 10 ms).

> Signed-off-by: Ramneek Mehresh 
> Signed-off-by: Nikhil Badola 
> ---
>  drivers/usb/host/ehci-fsl.c  |  3 +++
>  drivers/usb/host/ehci-hub.c  |  2 ++
>  drivers/usb/host/ehci.h  | 12 
>  drivers/usb/host/fsl-mph-dr-of.c |  5 +
>  include/linux/fsl_devices.h  |  1 +
>  5 files changed, 23 insertions(+)
> 
> diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
> index 202dafb..8904aae 100644
> --- a/drivers/usb/host/ehci-fsl.c
> +++ b/drivers/usb/host/ehci-fsl.c
> @@ -278,6 +278,9 @@ static int ehci_fsl_usb_setup(struct ehci_hcd *ehci)
>   out_be32(non_ehci + FSL_SOC_USB_SNOOP2, 0x8000 | 
> SNOOP_SIZE_2GB);
>   }
>  
> + 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 22abb68..7eac923 100644
> --- a/drivers/usb/host/ehci-hub.c
> +++ b/drivers/usb/host/ehci-hub.c
> @@ -303,6 +303,8 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
>   USB_PORT_STAT_HIGH_SPEED)
>   fs_idle_delay = true;
>   ehci_writel(ehci, t2, reg);
> + if (ehci_has_fsl_susp_errata(ehci))
> + usleep_range(1, 2);
>   changed = 1;
>   }
>   }

This is wrong for two reasons.  First, you must not sleep while holding 
a spinlock.  Second, your sleep is in the wrong place.  It should occur 
later, at this point:

if ((changed && ehci->has_tdi_phy_lpm) || fs_idle_delay) {
/*
 * Wait for HCD to enter low-power mode or for the bus
 * to switch to full-speed idle.
 */
usleep_range(5000, 5500);
}

You can check your quirk flag here and increase the length of this 
sleep to 10 ms.

Alan Stern

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-27 Thread Nikhil Badola 
As per USB specification, in the Suspend state, the status bit does
not change until the port is suspended. However, there may be a delay
in suspending a port if there is a transaction currently in progress
on the bus.

In the USBDR controller, the PORTSCx[SUSP] bit changes immediately when
the application sets it and not when the port is actually suspended

Workaround for this issue involves waiting for a minimum of 10ms to
allow the controller to go into SUSPEND state before proceeding ahead

Signed-off-by: Ramneek Mehresh 
Signed-off-by: Nikhil Badola 
---
 drivers/usb/host/ehci-fsl.c  |  3 +++
 drivers/usb/host/ehci-hub.c  |  2 ++
 drivers/usb/host/ehci.h  | 12 
 drivers/usb/host/fsl-mph-dr-of.c |  5 +
 include/linux/fsl_devices.h  |  1 +
 5 files changed, 23 insertions(+)

diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index 202dafb..8904aae 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -278,6 +278,9 @@ static int ehci_fsl_usb_setup(struct ehci_hcd *ehci)
out_be32(non_ehci + FSL_SOC_USB_SNOOP2, 0x8000 | 
SNOOP_SIZE_2GB);
}
 
+   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 22abb68..7eac923 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -303,6 +303,8 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
USB_PORT_STAT_HIGH_SPEED)
fs_idle_delay = true;
ehci_writel(ehci, t2, reg);
+   if (ehci_has_fsl_susp_errata(ehci))
+   usleep_range(1, 2);
changed = 1;
}
}
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index f700157..817eab5 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -215,6 +215,7 @@ struct ehci_hcd {   /* one per controller */
/* SILICON QUIRKS */
unsignedno_selective_suspend:1;
unsignedhas_fsl_port_bug:1; /* FreeScale */
+   unsignedhas_fsl_susp_errata;/* Freescale SUSP quirk */
unsignedbig_endian_mmio:1;
unsignedbig_endian_desc:1;
unsignedbig_endian_capbase:1;
@@ -675,6 +676,17 @@ ehci_port_speed(struct ehci_hcd *ehci, unsigned int portsc)
 #defineehci_port_speed(ehci, portsc)   USB_PORT_STAT_HIGH_SPEED
 #endif
 
+#if defined(CONFIG_PPC_85xx)
+/*
+ * Some Freescale processors have an erratum (USB A-005697) in which
+ * we need to wait for 10ms for bus to go into suspend mode after
+ * setting SUSP bit
+ */
+#define ehci_has_fsl_susp_errata(e) ((e)->has_fsl_susp_errata)
+#else
+#define ehci_has_fsl_susp_errata(e) (0)
+#endif
+
 /*-*/
 
 #ifdef CONFIG_PPC_83xx
diff --git a/drivers/usb/host/fsl-mph-dr-of.c b/drivers/usb/host/fsl-mph-dr-of.c
index 9f73141..870b50a 100644
--- a/drivers/usb/host/fsl-mph-dr-of.c
+++ b/drivers/usb/host/fsl-mph-dr-of.c
@@ -222,6 +222,11 @@ static int fsl_usb2_mph_dr_of_probe(struct platform_device 
*ofdev)
else
pdata->has_fsl_erratum_a007792 = 0;
 
+   if (of_get_property(np, "fsl,usb-erratum-a005697", NULL))
+   pdata->has_fsl_erratum_a005697 = 1;
+   else
+   pdata->has_fsl_erratum_a005697 = 0;
+
/*
 * Determine whether phy_clk_valid needs to be checked
 * by reading property in device tree
diff --git a/include/linux/fsl_devices.h b/include/linux/fsl_devices.h
index cebdbbb..42bf841 100644
--- a/include/linux/fsl_devices.h
+++ b/include/linux/fsl_devices.h
@@ -100,6 +100,7 @@ struct fsl_usb2_platform_data {
unsignedalready_suspended:1;
unsignedhas_fsl_erratum_a007792:1;
unsignedcheck_phy_clk_valid:1;
+   unsignedhas_fsl_erratum_a005697:1;
 
/* register save area for suspend/resume */
u32 pm_command;
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-27 Thread Nikhil Badola 
As per USB specification, in the Suspend state, the status bit does
not change until the port is suspended. However, there may be a delay
in suspending a port if there is a transaction currently in progress
on the bus.

In the USBDR controller, the PORTSCx[SUSP] bit changes immediately when
the application sets it and not when the port is actually suspended

Workaround for this issue involves waiting for a minimum of 10ms to
allow the controller to go into SUSPEND state before proceeding ahead

Signed-off-by: Ramneek Mehresh ramneek.mehr...@freescale.com
Signed-off-by: Nikhil Badola nikhil.bad...@freescale.com
---
 drivers/usb/host/ehci-fsl.c  |  3 +++
 drivers/usb/host/ehci-hub.c  |  2 ++
 drivers/usb/host/ehci.h  | 12 
 drivers/usb/host/fsl-mph-dr-of.c |  5 +
 include/linux/fsl_devices.h  |  1 +
 5 files changed, 23 insertions(+)

diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index 202dafb..8904aae 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -278,6 +278,9 @@ static int ehci_fsl_usb_setup(struct ehci_hcd *ehci)
out_be32(non_ehci + FSL_SOC_USB_SNOOP2, 0x8000 | 
SNOOP_SIZE_2GB);
}
 
+   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 22abb68..7eac923 100644
--- a/drivers/usb/host/ehci-hub.c
+++ b/drivers/usb/host/ehci-hub.c
@@ -303,6 +303,8 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
USB_PORT_STAT_HIGH_SPEED)
fs_idle_delay = true;
ehci_writel(ehci, t2, reg);
+   if (ehci_has_fsl_susp_errata(ehci))
+   usleep_range(1, 2);
changed = 1;
}
}
diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h
index f700157..817eab5 100644
--- a/drivers/usb/host/ehci.h
+++ b/drivers/usb/host/ehci.h
@@ -215,6 +215,7 @@ struct ehci_hcd {   /* one per controller */
/* SILICON QUIRKS */
unsignedno_selective_suspend:1;
unsignedhas_fsl_port_bug:1; /* FreeScale */
+   unsignedhas_fsl_susp_errata;/* Freescale SUSP quirk */
unsignedbig_endian_mmio:1;
unsignedbig_endian_desc:1;
unsignedbig_endian_capbase:1;
@@ -675,6 +676,17 @@ ehci_port_speed(struct ehci_hcd *ehci, unsigned int portsc)
 #defineehci_port_speed(ehci, portsc)   USB_PORT_STAT_HIGH_SPEED
 #endif
 
+#if defined(CONFIG_PPC_85xx)
+/*
+ * Some Freescale processors have an erratum (USB A-005697) in which
+ * we need to wait for 10ms for bus to go into suspend mode after
+ * setting SUSP bit
+ */
+#define ehci_has_fsl_susp_errata(e) ((e)-has_fsl_susp_errata)
+#else
+#define ehci_has_fsl_susp_errata(e) (0)
+#endif
+
 /*-*/
 
 #ifdef CONFIG_PPC_83xx
diff --git a/drivers/usb/host/fsl-mph-dr-of.c b/drivers/usb/host/fsl-mph-dr-of.c
index 9f73141..870b50a 100644
--- a/drivers/usb/host/fsl-mph-dr-of.c
+++ b/drivers/usb/host/fsl-mph-dr-of.c
@@ -222,6 +222,11 @@ static int fsl_usb2_mph_dr_of_probe(struct platform_device 
*ofdev)
else
pdata-has_fsl_erratum_a007792 = 0;
 
+   if (of_get_property(np, fsl,usb-erratum-a005697, NULL))
+   pdata-has_fsl_erratum_a005697 = 1;
+   else
+   pdata-has_fsl_erratum_a005697 = 0;
+
/*
 * Determine whether phy_clk_valid needs to be checked
 * by reading property in device tree
diff --git a/include/linux/fsl_devices.h b/include/linux/fsl_devices.h
index cebdbbb..42bf841 100644
--- a/include/linux/fsl_devices.h
+++ b/include/linux/fsl_devices.h
@@ -100,6 +100,7 @@ struct fsl_usb2_platform_data {
unsignedalready_suspended:1;
unsignedhas_fsl_erratum_a007792:1;
unsignedcheck_phy_clk_valid:1;
+   unsignedhas_fsl_erratum_a005697:1;
 
/* register save area for suspend/resume */
u32 pm_command;
-- 
2.1.0

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

2015-07-27 Thread Alan Stern 
On Mon, 27 Jul 2015, Nikhil Badola wrote:

 As per USB specification, in the Suspend state, the status bit does
 not change until the port is suspended. However, there may be a delay
 in suspending a port if there is a transaction currently in progress
 on the bus.
 
 In the USBDR controller, the PORTSCx[SUSP] bit changes immediately when
 the application sets it and not when the port is actually suspended
 
 Workaround for this issue involves waiting for a minimum of 10ms to
 allow the controller to go into SUSPEND state before proceeding ahead

Why is this delay needed?  When ehci_bus_suspend() is called there are 
never any transactions in progress, except possibly for a SOF 
transaction (which takes less than 1 us, nowhere near 10 ms).

 Signed-off-by: Ramneek Mehresh ramneek.mehr...@freescale.com
 Signed-off-by: Nikhil Badola nikhil.bad...@freescale.com
 ---
  drivers/usb/host/ehci-fsl.c  |  3 +++
  drivers/usb/host/ehci-hub.c  |  2 ++
  drivers/usb/host/ehci.h  | 12 
  drivers/usb/host/fsl-mph-dr-of.c |  5 +
  include/linux/fsl_devices.h  |  1 +
  5 files changed, 23 insertions(+)
 
 diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
 index 202dafb..8904aae 100644
 --- a/drivers/usb/host/ehci-fsl.c
 +++ b/drivers/usb/host/ehci-fsl.c
 @@ -278,6 +278,9 @@ static int ehci_fsl_usb_setup(struct ehci_hcd *ehci)
   out_be32(non_ehci + FSL_SOC_USB_SNOOP2, 0x8000 | 
 SNOOP_SIZE_2GB);
   }
  
 + 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 22abb68..7eac923 100644
 --- a/drivers/usb/host/ehci-hub.c
 +++ b/drivers/usb/host/ehci-hub.c
 @@ -303,6 +303,8 @@ static int ehci_bus_suspend (struct usb_hcd *hcd)
   USB_PORT_STAT_HIGH_SPEED)
   fs_idle_delay = true;
   ehci_writel(ehci, t2, reg);
 + if (ehci_has_fsl_susp_errata(ehci))
 + usleep_range(1, 2);
   changed = 1;
   }
   }

This is wrong for two reasons.  First, you must not sleep while holding 
a spinlock.  Second, your sleep is in the wrong place.  It should occur 
later, at this point:

if ((changed  ehci-has_tdi_phy_lpm) || fs_idle_delay) {
/*
 * Wait for HCD to enter low-power mode or for the bus
 * to switch to full-speed idle.
 */
usleep_range(5000, 5500);
}

You can check your quirk flag here and increase the length of this 
sleep to 10 ms.

Alan Stern

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/