From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vipin Kumar Date: Fri, 7 Dec 2012 16:02:18 +0530 Subject: [U-Boot] [PATCH v2] usbh/ehci: Increase timeout for enumeration In-Reply-To: <50C1C275.1090900@compulab.co.il> References: <50C1BEDD.6040202@compulab.co.il> <50C1C275.1090900@compulab.co.il> Message-ID: <50C1C5B2.1000908@st.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On 12/7/2012 3:48 PM, Igor Grinberg wrote: > > > On 12/07/12 12:03, Igor Grinberg wrote: >> On 12/07/12 10:58, Vipin Kumar wrote: >>> The current logic reads the port status just once after usb_hub_power_on and >>> expects the portstatus and portchange to report the connection status >>> immediately and correctly. >>> >>> Few pen drives are not able to report both of them immediately ie. those pens >>> report the connection change but not the connected state after the first read. >>> This opportunity once lost is gone for ever because the u-boot, unlike linux or >>> any other OS, works in polling mode. >>> >>> This patch modifies the logic to read the port status continuously until the >>> portstatus and portchange both report a connection change as well as a connected >>> state or no connection change and no connection. This logic is placed in a >>> timeout of 10 sec. At the end of it, the pen drive would have either reported a >>> ONE or a ZERO in bit 1 of portstatus as well as portchange. >>> >>> It enhances the set of pen drives which can eventually be detected by u-boot >>> >>> Signed-off-by: Vipin Kumar >>> --- >>> Hello Marek, Igor, >>> >>> I found another way to handle it. Please let me know if it is OK from the USB >>> stack poit of view. The fact is that a few pens do not report a connected status >>> in portstatus while they report a connection change in portchange after a >>> usb_hub_power_on. >>> >>> In this patch, I have tried to compare the connection bit from portstatus and >>> portchange for a timeout of 10 seconds. The situation is asumed to be stable >>> once both of them report the same. This seems to have increased the set of pens >>> supported by u-boot without any apparent side effect >>> >>> Please let me know if this is OK from your side >> >> Basically, this one looks fine, although I have two minor concerns below. >> >>> >>> Regards >>> Vipin >>> >>> common/usb_hub.c | 25 ++++++++++++++++++++----- >>> 1 file changed, 20 insertions(+), 5 deletions(-) >>> >>> diff --git a/common/usb_hub.c b/common/usb_hub.c >>> index e4a1201..3a66b0e 100644 >>> --- a/common/usb_hub.c >>> +++ b/common/usb_hub.c >>> @@ -396,14 +396,29 @@ static int usb_hub_configure(struct usb_device *dev) >>> for (i = 0; i< dev->maxchild; i++) { >>> ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1); >>> unsigned short portstatus, portchange; >>> + int ret; >>> + ulong start = get_timer(0); >>> + >>> + do { >>> + ret = usb_get_port_status(dev, i + 1, portsts); >>> + if (ret< 0) { >>> + USB_HUB_PRINTF("get_port_status failed\n"); >>> + break; >>> + } >>> + >>> + portstatus = le16_to_cpu(portsts->wPortStatus); >>> + portchange = le16_to_cpu(portsts->wPortChange); >>> + >>> + if ((portchange& USB_PORT_STAT_C_CONNECTION) == >>> + (portstatus& USB_PORT_STAT_CONNECTION)) >> >> I don't know if there is any corner case when the above check >> will always fail and so it will always wait a maximal delay time. >> Are those registers that identical, or can there be differences? > > Never mind, my mistake, USB_PORT_STAT_C_CONNECTION and USB_PORT_STAT_CONNECTION > are the same bit in the register. > >> >>> + break; >>> + >>> + mdelay(100); >>> + } while (get_timer(start)< CONFIG_SYS_HZ * 10); >> >> Is there any justification for the CONFIG_SYS_HZ * 10? >> I would be much more fine with this patch if there were any >> (even just test based * 2) reason for that number. > > Once you address this one, feel free to add: > Acked-by: Igor Grinberg > Thanks Igor, let me wait for Marek's comments also >> >>> >>> - if (usb_get_port_status(dev, i + 1, portsts)< 0) { >>> - USB_HUB_PRINTF("get_port_status failed\n"); >>> + if (ret< 0) >>> continue; >>> - } >>> >>> - portstatus = le16_to_cpu(portsts->wPortStatus); >>> - portchange = le16_to_cpu(portsts->wPortChange); >>> USB_HUB_PRINTF("Port %d Status %X Change %X\n", >>> i + 1, portstatus, portchange); >>> >> >