[U-Boot] [PATCH 1/1] tegra: usb: Fix device enumeration problem of USB1
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@c5000000"; usb1 = "/usb@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
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?
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. -----------------------------------------------------------------------------------
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.
participants (2)
-
Jim Lin -
Stephen Warren