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.