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

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

[Vipin Kumar]

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@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);
+
 	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);
 
-- 
1.8.0

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

[Igor Grinberg]

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

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

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

[Vipin Kumar]

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@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

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

[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@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.

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

[Vipin Kumar]

On 12/6/2012 1:06 PM, Igor Grinberg wrote:
> 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@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.

OK

> Where is this value come from? Any real justification? Or just: lets make it huge...

A bit of both. In routine usb_hub_power_on, we wait for max(pgood_delay, 
100) ms
for power to be stable. The pgood_delay can go max upto 512. This is
because max u8 can carry is 255 and pgood_delay is usb_hub_power_on * 2

A few pens request the maximum possible delay and are not stable even
after this. The new added delay is for power to be stable but I agree
that this value is more experimental and I do not have a real written
justification

> Also, as I understand from your commit message, this is needed only for broken pens...

Yes

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

The problem is that I do not know what to wait on in that case. Can you
point me to something

Regards
Vipin

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

[Igor Grinberg]

On 12/06/12 11:03, Vipin Kumar wrote:
> On 12/6/2012 1:06 PM, Igor Grinberg wrote:
>> 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@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.
> 
> OK
> 
>> Where is this value come from? Any real justification? Or just: lets make it huge...
> 
> A bit of both. In routine usb_hub_power_on, we wait for max(pgood_delay, 100) ms
> for power to be stable. The pgood_delay can go max upto 512. This is
> because max u8 can carry is 255 and pgood_delay is usb_hub_power_on * 2
> 
> A few pens request the maximum possible delay and are not stable even
> after this. The new added delay is for power to be stable but I agree
> that this value is more experimental and I do not have a real written
> justification

Ok. I think at least part of that information should be in the commit message.

> 
>> Also, as I understand from your commit message, this is needed only for broken pens...
> 
> Yes
> 
>> 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.
>>
> 
> The problem is that I do not know what to wait on in that case. Can you
> point me to something

Ok. I understand the problem.
I'm not a USB stack or USB HUB expert so,
unless someone (Marek?) can point to a right register or a way to handles this,
I suggest to introduce a kind of CONFIG_USB_HUB_PGOOD_MULT option
and have something like:
#ifndef CONFIG_USB_HUB_PGOOD_MULT
#define CONFIG_USB_HUB_PGOOD_MULT 2
#endif

and then in the usb_hub_power_on() function have something like:
- unsigned pgood_delay = hub->desc.bPwrOn2PwrGood * 2;
+ unsigned pgood_delay = hub->desc.bPwrOn2PwrGood * CONFIG_USB_HUB_PGOOD_MULT;

So if you need to raise the delay, you can do this through the config option
and only those that need this option will be affected.
Also, if there will be many users who need to define this option,
we can just increase the default.

Another  solution I'm thinking of is to use an environment variable for
the multiplier, so one can adjust the multiplier on the fly during runtime.
This solution is better at least during "debug and search" for the multiplier.

-- 
Regards,
Igor.

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

[Marek Vasut]

Dear Vipin Kumar,

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

Good, can you maybe poll the register instead of adding arbitrary delay which 
will cause trouble to everyone even if they never ever use such broken hardware?

Besides, I fail to understand the logic in this patch. The commit message 
doesn't explain it at all.

> 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@st.com>
[..]

Best regards,
Marek Vasut