| Message ID | 4B9C9637D5087840A465BDCB251780E9E2D629AF74@HKMAIL02.nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
On 06/19/2012 02:47 AM, Jim Lin wrote: > A known hardware issue of USB1 port where bit 1 (connect status > change) of PORTSC register 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 and a callback function > to clear that bit after Port Reset in order to proceed later device > enumeration. > > CONFIG_USB_EHCI_SUBMIT_ROOT_POST_CALLBACK This style of fix (adding an optional callback from the USB core) seems like the idea to me. Thanks. However, I'm a little worried that it implements the bug workaround at a different time to the Linux kernel. The kernel does: * Assert reset to the port * Poll the port for enable (any time delays are here) * Clear the CSC bit So, the CSC bit is cleared a long time after the port is reset. However, this U-Boot patch does: * Assert reset to the port * Clear the CSC bit * Wait a long time; 200mS * Check the port is enabled So, the CSC bit is cleared immediately after the port is reset. Is this U-Boot fix still guaranteed to solve the problem? In your testing, how many times did you test without this fix, and what was the failure percentage. How many times did you test with the fix, and what was the failure percentage? I think it'd be safer to execute the callback from inside hub_port_reset() right before the call to usb_get_port_status(). That would make the time when CSC gets cleared much more equivalent between the Linux kernel and U-Boot. The implementation of the callback function could probably be a lot simpler to (e.g. doesn't need to check which feature is being set etc.)
Dear Stephen Warren, > On 06/19/2012 02:47 AM, Jim Lin wrote: > > A known hardware issue of USB1 port where bit 1 (connect status > > change) of PORTSC register 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 and a callback function > > to clear that bit after Port Reset in order to proceed later device > > enumeration. > > > > CONFIG_USB_EHCI_SUBMIT_ROOT_POST_CALLBACK We did such fix in the past by adding weak function. Check ehci-hcd.c > > This style of fix (adding an optional callback from the USB core) seems > like the idea to me. Thanks. > > However, I'm a little worried that it implements the bug workaround at a > different time to the Linux kernel. The kernel does: > > * Assert reset to the port > * Poll the port for enable (any time delays are here) > * Clear the CSC bit > > So, the CSC bit is cleared a long time after the port is reset. > > However, this U-Boot patch does: > > * Assert reset to the port > * Clear the CSC bit > * Wait a long time; 200mS > * Check the port is enabled > > So, the CSC bit is cleared immediately after the port is reset. > > Is this U-Boot fix still guaranteed to solve the problem? In your > testing, how many times did you test without this fix, and what was the > failure percentage. How many times did you test with the fix, and what > was the failure percentage? > > I think it'd be safer to execute the callback from inside > hub_port_reset() right before the call to usb_get_port_status(). That > would make the time when CSC gets cleared much more equivalent between > the Linux kernel and U-Boot. The implementation of the callback function > could probably be a lot simpler to (e.g. doesn't need to check which > feature is being set etc.) Best regards, Marek Vasut
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index 38d6ae0..0b6b656 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -792,7 +792,11 @@ ehci_submit_root(struct usb_device *dev, unsigned long pipe, void *buffer, dev->act_len = len; dev->status = 0; +#ifdef CONFIG_USB_EHCI_SUBMIT_ROOT_POST_CALLBACK + return ehci_submit_root_post_callback(dev, pipe, buffer, length, req); +#else return 0; +#endif unknown: debug("requesttype=%x, request=%x, value=%x, index=%x, length=%x\n", diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c index a7e105b..3dd18d3 100644 --- a/drivers/usb/host/ehci-tegra.c +++ b/drivers/usb/host/ehci-tegra.c @@ -29,6 +29,40 @@ #include <asm/errno.h> #include <asm/arch/usb.h> +#ifdef CONFIG_USB_EHCI_SUBMIT_ROOT_POST_CALLBACK +int +ehci_submit_root_post_callback(struct usb_device *dev, unsigned long pipe, + void *buffer, int length, struct devrequest *req) +{ + u16 typeReq; + uint32_t reg; + uint32_t *status_reg; + + status_reg = (uint32_t *)&hcor->or_portsc[ + le16_to_cpu(req->index) - 1]; + if (((u32) status_reg & 0xFFFFC000) != TEGRA_USB1_BASE) + return 0; + typeReq = req->request | req->requesttype << 8; + switch (typeReq) { + case USB_REQ_SET_FEATURE | ((USB_DIR_OUT | USB_RT_PORT) << 8): + switch (le16_to_cpu(req->value)) { + /* + * A known HW issue on USB1 port where bit 1 (Connect Status + * Change) of PORTSC register will be set after issuing Port + * Reset. Clear that bit for later device enumeration. + */ + case USB_PORT_FEAT_RESET: + reg = ehci_readl(status_reg); + reg &= ~EHCI_PS_CLEAR; + reg |= EHCI_PS_CSC; + ehci_writel(status_reg, reg); + break; + } + break; + } + return 0; +} +#endif /* * Create the appropriate control structures to manage diff --git a/drivers/usb/host/ehci.h b/drivers/usb/host/ehci.h index cc00ce4..6c5d8f3 100644 --- a/drivers/usb/host/ehci.h +++ b/drivers/usb/host/ehci.h @@ -204,4 +204,8 @@ struct QH { int ehci_hcd_init(void); int ehci_hcd_stop(void); +#ifdef CONFIG_USB_EHCI_SUBMIT_ROOT_POST_CALLBACK +int ehci_submit_root_post_callback(struct usb_device *dev, unsigned long pipe, + void *buffer, int length, struct devrequest *req); +#endif #endif /* USB_EHCI_H */ diff --git a/include/configs/tegra2-common.h b/include/configs/tegra2-common.h index 1931179..b4822d4 100644 --- a/include/configs/tegra2-common.h +++ b/include/configs/tegra2-common.h @@ -111,6 +111,14 @@ #define CONFIG_EHCI_IS_TDI #define CONFIG_EHCI_DCACHE +/* + * A known HW issue on USB1 port where bit 1 (Connect Status Change) of PORTSC + * register will be set after issuing Port Reset. + * This setting is to add a callback function to clear that bit for later + * device enumeration. + */ +#define CONFIG_USB_EHCI_SUBMIT_ROOT_POST_CALLBACK + /* Total I2C ports on Tegra2 */ #define TEGRA_I2C_NUM_CONTROLLERS 4
A known hardware issue of USB1 port where bit 1 (connect status change) of PORTSC register 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 and a callback function to clear that bit after Port Reset in order to proceed later device enumeration. CONFIG_USB_EHCI_SUBMIT_ROOT_POST_CALLBACK 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. Changes in v2: - Change config name - Add a callback function at the end of ehci_submit_root() function drivers/usb/host/ehci-hcd.c | 4 ++++ drivers/usb/host/ehci-tegra.c | 34 ++++++++++++++++++++++++++++++++++ drivers/usb/host/ehci.h | 4 ++++ include/configs/tegra2-common.h | 8 ++++++++ 4 files changed, 50 insertions(+), 0 deletions(-)