From mboxrd@z Thu Jan  1 00:00:00 1970
From: Hans de Goede <hdegoede@redhat.com>
Date: Wed, 5 Aug 2015 15:42:11 +0200
Subject: [U-Boot] [PATCH] usb: Fix device detection code
In-Reply-To: <1438737562-7692-1-git-send-email-marex@denx.de>
References: <1438737562-7692-1-git-send-email-marex@denx.de>
Message-ID: <55C212B3.3030001@redhat.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Hi,

On 05-08-15 03:19, Marek Vasut wrote:
> The code in question polls an USB port status via USB_REQ_GET_STATUS
> to determine whether there is a device on the port or not. The way to
> figure that out is to check two bits. Those are wPortChange[0] and
> wPortStatus[0].
>
> The wPortChange[0] indicates whether some kind of a connection status
> change happened on a port (a device was plugged or unplugged). The
> wPortStatus[0] bit indicates the status of the connection (plugged or
> unplugged).
>
> The current code tests whether wPortChange[0] == wPortStatus[0] and
> if that's the case, considers the loop polling for the presence of a
> USB device on port finished.
>
> This works for most USB sticks, since they come up really quickly and
> trigger the USB port change detection before the first iteration of the
> detection loop happens. Thus, both wPortChange[0] and wPortStatus[0]
> are set to 1 and thus equal. The loop is existed in it's first iteration
> and the stick is detected correctly.
>
> The problem is with some obscure USB sticks, which take some time before
> they pop up on the bus after the port was enabled. In this case, both
> the wPortChange[0] and wPortStatus[0] are 0. They are equal again, so
> the loop again exits in the first iteration, but this is incorrect, as
> such USB stick didn't have the opportunity to get detected on the bus.
>
> Rework the code such, that it checks for wPortChange[0] first to test
> if any connection change happened at all. If no change occured, keep
> polling. If a change did occur, test the wPortStatus[0] to see there is
> some device present on the port and only if this is the case, break out
> of the polling loop.
>
> This patch also trims down the duration of the polling loop from 10s
> per port to 1s per port. This is still annoyingly long, but there is
> no better option in case of U-Boot unfortunatelly. This change will
> most likely increase the duration of 'usb start' on some platforms,
> but this is needed to fix a bug.
>
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Hans de Goede <hdegoede@redhat.com>

Looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans


> ---
>   common/usb_hub.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/common/usb_hub.c b/common/usb_hub.c
> index f621ddb..652a104 100644
> --- a/common/usb_hub.c
> +++ b/common/usb_hub.c
> @@ -489,11 +489,15 @@ static int usb_hub_configure(struct usb_device *dev)
>   			portstatus = le16_to_cpu(portsts->wPortStatus);
>   			portchange = le16_to_cpu(portsts->wPortChange);
>
> -			if ((portchange & USB_PORT_STAT_C_CONNECTION) ==
> -				(portstatus & USB_PORT_STAT_CONNECTION))
> +			/* No connection change happened, wait a bit more. */
> +			if (!(portchange & USB_PORT_STAT_C_CONNECTION))
> +				continue;
> +
> +			/* Test if the connection came up, and if so, exit. */
> +			if (portstatus & USB_PORT_STAT_CONNECTION)
>   				break;
>
> -		} while (get_timer(start) < CONFIG_SYS_HZ * 10);
> +		} while (get_timer(start) < CONFIG_SYS_HZ * 1);
>
>   		if (ret < 0)
>   			continue;
>