[PATCH 0/5] usb: port over some U-Boot fixes

[PATCH 0/5] usb: port over some U-Boot fixes

[Ahmad Fatoum]

A user reported on IRC that the USB keyboard fails to enumerate on an
MNT Pocket Reform, unless the laptop's power cable is connected.

This series collects a number of potential fixes for this issue.

Ahmad Fatoum (5):
  usb: hub: wait after port reset recovery
  usb: wait after initial device descriptor read
  usb: wait after setting device configuration
  usb: separate descriptor and config requests
  usb: hub: tolerate transient disconnects during reset

 drivers/usb/core/hub.c | 26 +++++++++++++++++++++++---
 drivers/usb/core/usb.c | 28 ++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 3 deletions(-)

-- 
2.47.3

[PATCH 1/5] usb: hub: wait after port reset recovery

[Ahmad Fatoum]

Port over U-Boot commit b5999f8f6ca89223b70ec3038fcfe9dbb6c67475:

|   usb: hub: Add missing reset recovery delay
|
|   Some devices like YubiKeys need more time before SET_ADDRESS. The spec
|   says we need to wait 10ms.
|
|   Signed-off-by: Hector Martin <marcan@marcan.st>
|   Reviewed-by: Marek Vasut <marex@denx.de>

Assisted-by: Codex:gpt-5.5
Signed-off-by: Ahmad Fatoum <a.fatoum@barebox.org>
---
 drivers/usb/core/hub.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 70da8566b289..e969fc466a8e 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -282,6 +282,13 @@ static void usb_hub_port_connect_change(struct usb_device *dev, int port,
 		return;
 	}
 
+	/*
+	 * USB 2.0 7.1.7.5: devices must be able to accept a SetAddress()
+	 * request (refer to Section 11.24.2 and Section 9.4 respectively)
+	 * after the reset recovery time 10 ms
+	 */
+	mdelay(10);
+
 	dev->children[port] = usb;
 	usb->parent = dev;
 	usb->portnr = port + 1;
-- 
2.47.3

[PATCH 2/5] usb: wait after initial device descriptor read

[Ahmad Fatoum]

Port over U-Boot commit 3406e9d8afcc8ade8cb3e697be2bcf9cc85c9d04:

|   usb: Add 1ms delay after first Get Descriptor request
|
|   Logitech Unifying Receiver 046d:c52b bcdDevice 12.10 seems
|   sensitive about the first Get Descriptor request. If there
|   are any other requests in the same microframe, the device
|   reports bogus data, first of the descriptor parts is not
|   sent to the host. Wait over one microframe duration before
|   issuing subsequent requests to avoid probe failure with
|   this device, since it can be used to connect USB keyboards.
|
|   Signed-off-by: Marek Vasut <marex@denx.de>
|   Tested-by: Janne Grunau <j@jannau.net>

Assisted-by: Codex:gpt-5.5
Signed-off-by: Ahmad Fatoum <a.fatoum@barebox.org>
---
 drivers/usb/core/usb.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index ae87137447c7..c6b3187de17d 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -403,6 +403,17 @@ static int usb_setup_descriptor(struct usb_device *dev, bool do_read)
 		if (err)
 			return err;
 
+		/*
+		 * Logitech Unifying Receiver 046d:c52b bcdDevice 12.10 seems
+		 * sensitive about the first Get Descriptor request. If there
+		 * are any other requests in the same microframe, the device
+		 * reports bogus data, first of the descriptor parts is not
+		 * sent to the host. Wait over one microframe duration here
+		 * (1mS for USB 1.x , 125uS for USB 2.0) to avoid triggering
+		 * the issue.
+		 */
+		mdelay(1);
+
 		dev->epmaxpacketin[0] = dev->descriptor->bMaxPacketSize0;
 		dev->epmaxpacketout[0] = dev->descriptor->bMaxPacketSize0;
 	}
-- 
2.47.3

[PATCH 3/5] usb: wait after setting device configuration

[Ahmad Fatoum]

Port over U-Boot commit f647bf0ba36a5236d4bc7f93d39bfacfb1cfe6c7:

|   usb: Wait after sending Set Configuration request
|
|   Some devices, like the SanDisk Cruzer Pop need some time to process
|   the Set Configuration request, so wait a little until they are ready.
|
|   Signed-off-by: Marek Vasut <marex@denx.de>
|   Cc: Chin Liang See <clsee@altera.com>
|   Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
|   Cc: Hans de Goede <hdegoede@redhat.com>
|   Cc: Stefan Roese <sr@denx.de>
|   Cc: Stephen Warren <swarren@nvidia.com>

Assisted-by: Codex:gpt-5.5
Signed-off-by: Ahmad Fatoum <a.fatoum@barebox.org>
---
 drivers/usb/core/usb.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index c6b3187de17d..145b85a28fab 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -539,6 +539,14 @@ int usb_new_device(struct usb_device *dev)
 		       dev->act_len, dev->status);
 		goto err_out;
 	}
+
+	/*
+	 * Wait until the Set Configuration request gets processed by the
+	 * device. This is required by at least SanDisk Cruzer Pop USB 2.0
+	 * and Kingston DT Ultimate 32GB USB 3.0 on DWC2 OTG controller.
+	 */
+	mdelay(10);
+
 	dev_dbg(&dev->dev, "new device: Mfr=%d, Product=%d, SerialNumber=%d\n",
 		   dev->descriptor->iManufacturer, dev->descriptor->iProduct,
 		   dev->descriptor->iSerialNumber);
-- 
2.47.3

[PATCH 4/5] usb: separate descriptor and config requests

[Ahmad Fatoum]

Port over U-Boot commit ef71290be9b70d8cfa63b506c7d93c5069f63c42:

|   usb: Assure Get Descriptor request is in separate microframe
|
|   The Kingston DT Ultimate USB 3.0 stick is sensitive to this first
|   Get Descriptor request and if the request is not in a separate
|   microframe, the stick refuses to operate. Add slight delay, which
|   is enough for one microframe to pass on any USB spec revision.
|
|   Signed-off-by: Marek Vasut <marex@denx.de>
|   Cc: Chin Liang See <clsee@altera.com>
|   Cc: Dinh Nguyen <dinguyen@opensource.altera.com>
|   Cc: Hans de Goede <hdegoede@redhat.com>
|   Cc: Stefan Roese <sr@denx.de>
|   Cc: Stephen Warren <swarren@nvidia.com>

Assisted-by: Codex:gpt-5.5
Signed-off-by: Ahmad Fatoum <a.fatoum@barebox.org>
---
 drivers/usb/core/usb.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/core/usb.c b/drivers/usb/core/usb.c
index 145b85a28fab..3f379197dc0c 100644
--- a/drivers/usb/core/usb.c
+++ b/drivers/usb/core/usb.c
@@ -522,6 +522,15 @@ int usb_new_device(struct usb_device *dev)
 	le16_to_cpus(&dev->descriptor->idVendor);
 	le16_to_cpus(&dev->descriptor->idProduct);
 	le16_to_cpus(&dev->descriptor->bcdDevice);
+
+	/*
+	 * Kingston DT Ultimate 32GB USB 3.0 seems to be extremely sensitive
+	 * about this first Get Descriptor request. If there are any other
+	 * requests in the first microframe, the stick crashes. Wait about
+	 * one microframe duration here (1mS for USB 1.x , 125uS for USB 2.0).
+	 */
+	mdelay(1);
+
 	/* only support for one config for now */
 	length = usb_get_configuration_no(dev, buf, 0);
 	if (length < 0) {
-- 
2.47.3

[PATCH 5/5] usb: hub: tolerate transient disconnects during reset

[Ahmad Fatoum]

Port over U-Boot commit 74c0d756dea14855bec47f380b6ccb557be5db7c:

|   usb: hub: don't check CONNECTION in hub_port_reset()
|
|   One specific USB 3.0 device behaves strangely when reset by
|   usb_new_device()'s call to hub_port_reset(). For some reason, the device
|   appears to briefly drop off the bus when this second bus reset is
|   executed, yet if we retry this loop, it'll eventually come back after
|   another two resets.
|
|   If USB bus reset is executed over and over within usb_new_device()'s call
|   to hub_port_reset(), I see the following sequence of results, which
|   repeats as long as you want:
|
|   1) STAT_C_CONNECTION = 1 STAT_CONNECTION = 0  USB_PORT_STAT_ENABLE 0
|   2) STAT_C_CONNECTION = 1 STAT_CONNECTION = 1  USB_PORT_STAT_ENABLE 0
|   3) STAT_C_CONNECTION = 1 STAT_CONNECTION = 1  USB_PORT_STAT_ENABLE 1
|
|   The device in question is a SanDisk Ultra USB 3.0 16GB memory stick with
|   USB VID/PID 0x0781/0x5581.
|
|   In order to allow this device to work with U-Boot, ignore the
|   {C_,}CONNECTION bits in the status/change registers, and only use the
|   ENABLE bit to determine if the reset was successful.
|
|   To be honest, extensive investigation has failed to determine why this
|   problem occurs. I'd love to know! I don't know if it's caused by:
|   * A HW bug in the device
|   * A HW bug in the Tegra USB controller
|   * A SW bug in the U-Boot Tegra USB driver
|   * A SW bug in the U-Boot USB core
|
|   This issue only occurs when the device's USB3 pins are attached to the
|   host; if only the USB2 pins are connected the issue does not occur. The
|   USB3 controller on Tegra is in reset, so is not actively communicating
|   with the device at all - a USB3 analyzer confirms this. Slightly
|   unplugging the device (so the USB3 pins don't contact) or using a USB2
|   cable or hub as an intermediary avoids the problem. For some reason,
|   the Linux kernel (either on the same Tegra board, or on an x86 host)
|   has no issue with the device, and I observe no disconnections during
|   reset.
|
|   This change won't affect any USB device that already works, since such
|   devices could not currently be triggering the error return this patch
|   removes, or they wouldn't be working currently.
|
|   However, this patch is quite reliable in practice, hence I hope it's
|   acceptable to solve the problem.
|
|   The only potential fallout I can see from this patch is:
|
|   * A broken device that triggers C_CONNECTION/!CONNECTION now causes the
|     loop in hub_port_reset() to run multiple times. If it never succeeds,
|     this will cause "usb start" to take roughly 1s extra to execute.
|
|   * If the user unplugs a device while hub_port_reset() is executing, and
|     very quickly swaps in a new device, hub_port_reset() might succeed on
|     the new device. This would mean that any information cached about the
|     original device (from the descriptor read in usb_new_device(), which
|     simply caches the max packet size) might be invalid, which would cause
|     problems talking to the new device. However, without this change, the
|     new device wouldn't work anyway, so this is probably not much of a
|     loss.
|
|   Signed-off-by: Stephen Warren <swarren@nvidia.com>

Assisted-by: Codex:gpt-5.5
Signed-off-by: Ahmad Fatoum <a.fatoum@barebox.org>
---
 drivers/usb/core/hub.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index e969fc466a8e..3820b4cc9067 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -208,9 +208,22 @@ static int hub_port_reset(struct usb_device *hub, int port,
 			(portstatus & USB_PORT_STAT_CONNECTION) ? 1 : 0,
 			(portstatus & USB_PORT_STAT_ENABLE) ? 1 : 0);
 
-		if ((portchange & USB_PORT_STAT_C_CONNECTION) ||
-		    !(portstatus & USB_PORT_STAT_CONNECTION))
-			return -1;
+		/*
+		 * Perhaps we should check for the following here:
+		 * - C_CONNECTION hasn't been set.
+		 * - CONNECTION is still set.
+		 *
+		 * Doing so would ensure that the device is still connected
+		 * to the bus, and hasn't been unplugged or replaced while the
+		 * USB bus reset was going on.
+		 *
+		 * However, if we do that, then (at least) a San Disk Ultra
+		 * USB 3.0 16GB device fails to reset on (at least) an NVIDIA
+		 * Tegra Jetson TK1 board. For some reason, the device appears
+		 * to briefly drop off the bus when this second bus reset is
+		 * executed, yet if we retry this loop, it'll eventually come
+		 * back after another reset or two.
+		 */
 
 		if (portstatus & USB_PORT_STAT_ENABLE)
 			break;
-- 
2.47.3