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.ku...@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 <grinb...@compulab.co.il>
>
>>
>> - 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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
