[U-Boot] [PATCH 1/1] tegra: usb: Fix device enumeration problem of USB1

[U-Boot] [PATCH 1/1] tegra: usb: Fix device enumeration problem of USB1

[Jim Lin]

For some reason, bit 1 (connect status change) of PORTSC will be set
after issuing Port Reset (like "usb reset" in u-boot command line).
This will be treated as an error and stops later device enumeration.

Therefore we add a definition in header file to ignore checking of that bit
after Port Reset.
CONFIG_USB_RESET_IGNORE_CONNECT_CHANGE

Signed-off-by: Jim Lin <jilin@nvidia.com>
---
To reproduce this issue, you can modify board .dts file to set
as the following to build u-boot binary.
"
 usb0 = "/usb at c5000000";
 usb1 = "/usb at c5008000";
"
Install device on USB1 port (address at 0xc5000000).
And run "usb reset" in u-boot console to enumerate device.

 common/usb_hub.c                |    4 ++++
 include/configs/tegra2-common.h |    7 +++++++
 2 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/common/usb_hub.c b/common/usb_hub.c
index e0edaad..8e6bdd8 100644
--- a/common/usb_hub.c
+++ b/common/usb_hub.c
@@ -180,8 +180,12 @@ int hub_port_reset(struct usb_device *dev, int port,
 			(portstatus & USB_PORT_STAT_CONNECTION) ? 1 : 0,
 			(portstatus & USB_PORT_STAT_ENABLE) ? 1 : 0);
 
+#ifdef CONFIG_USB_RESET_IGNORE_CONNECT_CHANGE
+		if (!(portstatus & USB_PORT_STAT_CONNECTION))
+#else
 		if ((portchange & USB_PORT_STAT_C_CONNECTION) ||
 		    !(portstatus & USB_PORT_STAT_CONNECTION))
+#endif
 			return -1;
 
 		if (portstatus & USB_PORT_STAT_ENABLE)
diff --git a/include/configs/tegra2-common.h b/include/configs/tegra2-common.h
index 1931179..904c17e 100644
--- a/include/configs/tegra2-common.h
+++ b/include/configs/tegra2-common.h
@@ -111,6 +111,13 @@
 #define CONFIG_EHCI_IS_TDI
 #define CONFIG_EHCI_DCACHE
 
+/*
+ * For some reason, bit 1 (Connect Status Change) of PORTSC register will be
+ * set after issuing Port Reset. This setting is to ignore checking of that
+ * bit after reset.
+ */
+#define CONFIG_USB_RESET_IGNORE_CONNECT_CHANGE
+
 /* Total I2C ports on Tegra2 */
 #define TEGRA_I2C_NUM_CONTROLLERS	4
 
-- 
1.7.3

nvpublic

[U-Boot] [PATCH 1/1] tegra: usb: Fix device enumeration problem of USB1

[Stephen Warren]

On 06/14/2012 04:40 AM, Jim Lin wrote:
> For some reason, bit 1 (connect status change) of PORTSC will be set
> after issuing Port Reset (like "usb reset" in u-boot command line).
> This will be treated as an error and stops later device enumeration.
> 
> Therefore we add a definition in header file to ignore checking of that bit
> after Port Reset.
> CONFIG_USB_RESET_IGNORE_CONNECT_CHANGE

(Again, I'm CC'ing the USB maintainer here)

Looking at the Linux kernel's Tegra EHCI driver:
a) This WAR is only needed on the first USB port, not all of them.
b) This WAR is not complete; there's a loop in the kernel that resets
the port twice in order to guarantee that the port will become enabled.
c) The kernel driver actively clears this CSC bit rather than leaving it
set and ignoring it. Is there any implication to this difference?

So, rather than just ifdef'ing this fix into the driver, wouldn't it be
better to add a callback from the USB core into the USB driver, so that
the Tegra EHCI driver could choose to only implement this WAR for port
1, and also do the multiple-reset-loop thing.

Finally, in the change description, the text "for some reason" is quite
unclear; it sounds like you have absolutely no idea why this happens. Is
this a known and root-caused HW bug for which this fix has been fully
validated? Or, is this patch just some random hack that seems to work
for you?

[U-Boot] [PATCH 1/1] tegra: usb: Fix device enumeration problem of USB1

[Jim Lin]

On 06/14/2012 04:40 AM, Jim Lin wrote:
>> For some reason, bit 1 (connect status change) of PORTSC will be set
>> after issuing Port Reset (like "usb reset" in u-boot command line).
>> This will be treated as an error and stops later device enumeration.
>> 
>> Therefore we add a definition in header file to ignore checking of that bit
>> after Port Reset.
>> CONFIG_USB_RESET_IGNORE_CONNECT_CHANGE

>(Again, I'm CC'ing the USB maintainer here)

>Looking at the Linux kernel's Tegra EHCI driver:
>a) This WAR is only needed on the first USB port, not all of them.

  [Jim] No penalty for USB2 and USB3 ports. Because u-boot hub_port_reset
  has at most 5 times of retry for a port.
  USB2 and USB3 ports will reset once in retry loop
  ('port connect change' bit will not set after reset).
  USB1 will have at most 2 times of reset in the retry loop after adding
  this patch.

>b) This WAR is not complete; there's a loop in the kernel that resets
>the port twice in order to guarantee that the port will become enabled.

  [Jim] Disagree. Because u-boot hub_port_reset has at most 5 times of retry
  for a port. U-boot and kernel code are not entirely same.

>c) The kernel driver actively clears this CSC bit rather than leaving it
>set and ignoring it. Is there any implication to this difference?

  [Jim] I can make next change to be consistent with kernel driver to clear
  CSC bit after Port Reset.

>So, rather than just ifdef'ing this fix into the driver, wouldn't it be
>better to add a callback from the USB core into the USB driver, so that
>the Tegra EHCI driver could choose to only implement this WAR for port
>1, and also do the multiple-reset-loop thing.

  [Jim] No callback for me to cut in. Also no penalty for other ports.

>Finally, in the change description, the text "for some reason" is quite
>unclear; it sounds like you have absolutely no idea why this happens. Is
>this a known and root-caused HW bug for which this fix has been fully
>validated? Or, is this patch just some random hack that seems to work
>for you?

  [Jim] This is a known and root-caused HW bug.
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

[U-Boot] [PATCH 1/1] tegra: usb: Fix device enumeration problem of USB1

[Stephen Warren]

On 06/15/2012 03:38 AM, Jim Lin wrote:
> On 06/14/2012 04:40 AM, Jim Lin wrote:
>>> For some reason, bit 1 (connect status change) of PORTSC will be set
>>> after issuing Port Reset (like "usb reset" in u-boot command line).
>>> This will be treated as an error and stops later device enumeration.
>>>
>>> Therefore we add a definition in header file to ignore checking of that bit
>>> after Port Reset.
>>> CONFIG_USB_RESET_IGNORE_CONNECT_CHANGE
> 
>> (Again, I'm CC'ing the USB maintainer here)
> 
>> Looking at the Linux kernel's Tegra EHCI driver:
>> a) This WAR is only needed on the first USB port, not all of them.
> 
>   [Jim] No penalty for USB2 and USB3 ports. Because u-boot hub_port_reset
>   has at most 5 times of retry for a port.
>   USB2 and USB3 ports will reset once in retry loop
>   ('port connect change' bit will not set after reset).
>   USB1 will have at most 2 times of reset in the retry loop after adding
>   this patch.

Yes, there is no time penalty, but presumably there's a reason that
hub_port_reset() explicitly checks whether PORT_STAT_C_CONNECTION is set
and fails if it is. This change will remove that check for USB2 and USB3
and thus change the behavior of those ports which are not affected by
the bug this patch is trying to fix.

>> b) This WAR is not complete; there's a loop in the kernel that resets
>> the port twice in order to guarantee that the port will become enabled.
> 
>   [Jim] Disagree. Because u-boot hub_port_reset has at most 5 times of retry
>   for a port. U-boot and kernel code are not entirely same.

OK, I see the loop. It's not doing exactly what the kernel is to the HW,
but it's hopefully close enough.

>> So, rather than just ifdef'ing this fix into the driver, wouldn't it be
>> better to add a callback from the USB core into the USB driver, so that
>> the Tegra EHCI driver could choose to only implement this WAR for port
>> 1, and also do the multiple-reset-loop thing.
> 
>   [Jim] No callback for me to cut in. Also no penalty for other ports.

My point was that a callback (or perhaps some kind of per-port flag if
not an actual callback) could be added, not that there was one already
that should be used.

>> Finally, in the change description, the text "for some reason" is quite
>> unclear; it sounds like you have absolutely no idea why this happens. Is
>> this a known and root-caused HW bug for which this fix has been fully
>> validated? Or, is this patch just some random hack that seems to work
>> for you?
> 
>   [Jim] This is a known and root-caused HW bug.

That's good to know. It'd be great if the commit description could be
more assertive on this point.