[U-Boot] [PATCH v2] usbh/ehci: Increase timeout for enumeration

[Igor Grinberg <grinberg@compulab.co.il>]

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 <vipin.kumar@st.com>
> ---
> 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?

> +				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.

>  
> -		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);
>  

-- 
Regards,
Igor.