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

[Igor Grinberg]

On 12/06/12 08:58, Vipin Kumar wrote:
> On 12/6/2012 12:17 PM, Igor Grinberg wrote:
>> On 12/06/12 08:30, Vipin Kumar wrote:
>>> Few pen drives take longer than usual for enumeration. The u-boot unlike linux
>>> does not depend on interrupts and works in polling and timeout mode.
>>>
>>> This patch increases this timeout to increase the set of usb sticks that can be
>>> enumerated by u-boot
>>>
>>> Signed-off-by: Vipin Kumar<vipin.kumar at st.com>
>>> ---
>>>   common/usb_hub.c | 27 ++++++++++++++++++++++-----
>>>   1 file changed, 22 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/common/usb_hub.c b/common/usb_hub.c
>>> index e4a1201..24de9b7 100644
>>> --- a/common/usb_hub.c
>>> +++ b/common/usb_hub.c
>>> @@ -393,17 +393,34 @@ static int usb_hub_configure(struct usb_device *dev)
>>>           "" : "no ");
>>>       usb_hub_power_on(hub);
>>>
>>> +    mdelay(1500);
>>
>> a 1.5 seconds? This looks like a huge overkill...
>> Even for broken usb sticks...
>>
> 
> Yes, but we are not talking about performance in u-boot. And since we are working in a polling mode, we only have 1 chance to detect the pen-drive

Of course we _do care_ about performance and 1.5 seconds is huge and not justified impact.
Where is this value come from? Any real justification? Or just: lets make it huge...
Also, as I understand from your commit message, this is needed only for broken pens...
Why should all others suffer?

If this is really needed, I think you can do better then this.
For example instead of waiting 1.5 seconds no meter what each time,
make it a busy/wait loop (like you do below) and expire after a timeout.

> 
>>> +
>>>       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))
>>> +                break;
>>>
>>> -        if (usb_get_port_status(dev, i + 1, portsts)<  0) {
>>> -            USB_HUB_PRINTF("get_port_status failed\n");
>>> +            mdelay(100);
>>> +        } while (get_timer(start)<  CONFIG_SYS_HZ * 10);
>>> +
>>> +        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.