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

[Vipin Kumar]

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<vipin.kumar at 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?
>
> 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<grinberg at compulab.co.il>
>

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