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?
> + 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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
