Skip to contentSkip to conversation

dm: usb: Move descriptor setup code into its own function

PATCH New Simon Glass 3 participants 8 messages Apr 14, 2015

lore
Share & tools
lore
#0 lore raw

PATCH v3 1/3 dm: usb: Move descriptor setup code into its own function

usb_new_device() is far too long and does far too much. As a first step, move the code that does initial setup and reads a descriptor into its own function called usb_setup_descriptor().

For XHCI the init order is different - we set up the device but don't actually read the descriptor until after we set an address. Support this option as a parameter to usb_setup_descriptor().

Avoid changing this torturous code more than necessary to make it easy to review.

---

Changes in v3:
- Fix polarity of CONFIG_XHCI check (which broke USB keyboards)

 common/usb.c | 117 ++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 67 insertions(+), 50 deletions(-)
common/usb.c +67 -50
index 4ab2213..e5df98f 100644
@@ -891,35 +891,12 @@ int usb_legacy_port_reset(struct usb_device *hub, int portnr)
 	return 0;
 }
 
-/*
- * By the time we get here, the device has gotten a new device ID
- * and is in the default state. We need to identify the thing and
- * get the ball rolling..
- *
- * Returns 0 for success, != 0 for error.
- */
-int usb_new_device(struct usb_device *dev)
+static int usb_setup_descriptor(struct usb_device *dev, bool do_read)
 {
-	int addr, err;
-	int tmp;
+	__maybe_unused struct usb_device_descriptor *desc;
 	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, tmpbuf, USB_BUFSIZ);
 
 	/*
-	 * Allocate usb 3.0 device context.
-	 * USB 3.0 (xHCI) protocol tries to allocate device slot
-	 * and related data structures first. This call does that.
-	 * Refer to sec 4.3.2 in xHCI spec rev1.0
-	 */
-	if (usb_alloc_device(dev)) {
-		printf("Cannot allocate device context to get SLOT_ID\n");
-		return -1;
-	}
-
-	/* We still haven't set the Address yet */
-	addr = dev->devnum;
-	dev->devnum = 0;
-
-	/*
 	 * This is a Windows scheme of initialization sequence, with double
 	 * reset of the device (Linux uses the same sequence)
 	 * Some equipment is said to work only with such init sequence; this
@@ -934,40 +911,31 @@ int usb_new_device(struct usb_device *dev)
 	 * the maxpacket size is 8 or 16 the device may be waiting to transmit
 	 * some more, or keeps on retransmitting the 8 byte header. */
 
+	desc = (struct usb_device_descriptor *)tmpbuf;
 	dev->descriptor.bMaxPacketSize0 = 64;	    /* Start off at 64 bytes  */
 	/* Default to 64 byte max packet size */
 	dev->maxpacketsize = PACKET_SIZE_64;
 	dev->epmaxpacketin[0] = 64;
 	dev->epmaxpacketout[0] = 64;
 
-	/*
-	 * XHCI needs to issue a Address device command to setup
-	 * proper device context structures, before it can interact
-	 * with the device. So a get_descriptor will fail before any
-	 * of that is done for XHCI unlike EHCI.
-	 */
-#ifndef CONFIG_USB_XHCI
-	struct usb_device_descriptor *desc;
+	if (do_read) {
+		int err;
 
-	desc = (struct usb_device_descriptor *)tmpbuf;
-	err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, 64);
-	if (err < 0) {
-		debug("usb_new_device: usb_get_descriptor() failed\n");
-		return 1;
+		err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, 64);
+		if (err < sizeof(dev->descriptor)) {
+			if (err < 0) {
+				printf("unable to get device descriptor (error=%d)\n",
+				       err);
+				return err;
+			} else {
+				printf("USB device descriptor short read (expected %i, got %i)\n",
+				       (int)sizeof(dev->descriptor), err);
+				return -EIO;
+			}
+		}
+		memcpy(&dev->descriptor, tmpbuf, sizeof(dev->descriptor));
 	}
 
-	dev->descriptor.bMaxPacketSize0 = desc->bMaxPacketSize0;
-	/*
-	 * Fetch the device class, driver can use this info
-	 * to differentiate between HUB and DEVICE.
-	 */
-	dev->descriptor.bDeviceClass = desc->bDeviceClass;
-#endif
-
-	err = usb_legacy_port_reset(dev->parent, dev->portnr);
-	if (err)
-		return err;
-
 	dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0;
 	dev->epmaxpacketout[0] = dev->descriptor.bMaxPacketSize0;
 	switch (dev->descriptor.bMaxPacketSize0) {
@@ -984,6 +952,55 @@ int usb_new_device(struct usb_device *dev)
 		dev->maxpacketsize = PACKET_SIZE_64;
 		break;
 	}
+
+	return 0;
+}
+
+/*
+ * By the time we get here, the device has gotten a new device ID
+ * and is in the default state. We need to identify the thing and
+ * get the ball rolling..
+ *
+ * Returns 0 for success, != 0 for error.
+ */
+int usb_new_device(struct usb_device *dev)
+{
+	bool do_read = true;
+	int addr, err;
+	int tmp;
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, tmpbuf, USB_BUFSIZ);
+
+	/*
+	 * Allocate usb 3.0 device context.
+	 * USB 3.0 (xHCI) protocol tries to allocate device slot
+	 * and related data structures first. This call does that.
+	 * Refer to sec 4.3.2 in xHCI spec rev1.0
+	 */
+	if (usb_alloc_device(dev)) {
+		printf("Cannot allocate device context to get SLOT_ID\n");
+		return -1;
+	}
+
+	/* We still haven't set the Address yet */
+	addr = dev->devnum;
+	dev->devnum = 0;
+
+	/*
+	 * XHCI needs to issue a Address device command to setup
+	 * proper device context structures, before it can interact
+	 * with the device. So a get_descriptor will fail before any
+	 * of that is done for XHCI unlike EHCI.
+	 */
+#ifdef CONFIG_USB_XHCI
+	do_read = false;
+#endif
+	err = usb_setup_descriptor(dev, do_read);
+	if (err)
+		return err;
+	err = usb_legacy_port_reset(dev->parent, dev->portnr);
+	if (err)
+		return err;
+
 	dev->devnum = addr;
 
 	err = usb_set_address(dev); /* set address */
Signature · 1 line
2.2.0.rc0.207.ga3a616c
#1 lore raw

PATCH v3 2/3 dm: usb: Split out more code from usb_new_device()

Move the code that sets up the device with a new address into its own function, usb_prepare_device().

---

Changes in v3:
- Fix polarity of CONFIG_XHCI check (which broke USB keyboards)

 common/usb.c | 72 +++++++++++++++++++++++++++++++++++-------------------------
 1 file changed, 42 insertions(+), 30 deletions(-)
common/usb.c +42 -30
index e5df98f..f249acf 100644
@@ -956,19 +956,10 @@ static int usb_setup_descriptor(struct usb_device *dev, bool do_read)
 	return 0;
 }
 
-/*
- * By the time we get here, the device has gotten a new device ID
- * and is in the default state. We need to identify the thing and
- * get the ball rolling..
- *
- * Returns 0 for success, != 0 for error.
- */
-int usb_new_device(struct usb_device *dev)
+static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read,
+			      struct usb_device *parent, int portnr)
 {
-	bool do_read = true;
-	int addr, err;
-	int tmp;
-	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, tmpbuf, USB_BUFSIZ);
+	int err;
 
 	/*
 	 * Allocate usb 3.0 device context.
@@ -976,28 +967,15 @@ int usb_new_device(struct usb_device *dev)
 	 * and related data structures first. This call does that.
 	 * Refer to sec 4.3.2 in xHCI spec rev1.0
 	 */
-	if (usb_alloc_device(dev)) {
+	err = usb_alloc_device(dev);
+	if (err) {
 		printf("Cannot allocate device context to get SLOT_ID\n");
-		return -1;
+		return err;
 	}
-
-	/* We still haven't set the Address yet */
-	addr = dev->devnum;
-	dev->devnum = 0;
-
-	/*
-	 * XHCI needs to issue a Address device command to setup
-	 * proper device context structures, before it can interact
-	 * with the device. So a get_descriptor will fail before any
-	 * of that is done for XHCI unlike EHCI.
-	 */
-#ifdef CONFIG_USB_XHCI
-	do_read = false;
-#endif
 	err = usb_setup_descriptor(dev, do_read);
 	if (err)
 		return err;
-	err = usb_legacy_port_reset(dev->parent, dev->portnr);
+	err = usb_legacy_port_reset(parent, portnr);
 	if (err)
 		return err;
 
@@ -1008,11 +986,45 @@ int usb_new_device(struct usb_device *dev)
 	if (err < 0) {
 		printf("\n      USB device not accepting new address " \
 			"(error=%lX)\n", dev->status);
-		return 1;
+		return err;
 	}
 
 	mdelay(10);	/* Let the SET_ADDRESS settle */
 
+	return 0;
+}
+
+/*
+ * By the time we get here, the device has gotten a new device ID
+ * and is in the default state. We need to identify the thing and
+ * get the ball rolling..
+ *
+ * Returns 0 for success, != 0 for error.
+ */
+int usb_new_device(struct usb_device *dev)
+{
+	bool do_read = true;
+	int addr, err;
+	int tmp, ret;
+	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, tmpbuf, USB_BUFSIZ);
+
+	/* We still haven't set the Address yet */
+	addr = dev->devnum;
+	dev->devnum = 0;
+
+	/*
+	 * XHCI needs to issue a Address device command to setup
+	 * proper device context structures, before it can interact
+	 * with the device. So a get_descriptor will fail before any
+	 * of that is done for XHCI unlike EHCI.
+	 */
+#ifdef CONFIG_USB_XHCI
+	do_read = false;
+#endif
+	ret = usb_prepare_device(dev, addr, do_read, dev->parent, dev->portnr);
+	if (ret)
+		return ret;
+
 	tmp = sizeof(dev->descriptor);
 
 	err = usb_get_descriptor(dev, USB_DT_DEVICE, 0,
Signature · 1 line
2.2.0.rc0.207.ga3a616c
#2 lore raw

PATCH v3 2/3 dm: usb: Split out more code from usb_new_device()

Quoted 9 lines · Simon Glass

Move the code that sets up the device with a new address into its own function, usb_prepare_device().

---

Changes in v3:
- Fix polarity of CONFIG_XHCI check (which broke USB keyboards)

Best regards,
Marek Vasut

#3 lore raw

PATCH v3 3/3 dm: usb: Complete the splitting up of usb_new_device()

In reply to Simon Glass

This function now calls usb_setup_device() to set up the device and usb_hub_probe() to check if it is a hub. The XHCI special case is now a parameter to usb_setup_device(). The latter will be used by the USB uclass when it is added, since it does not rely on any CONFIGs or legacy data structures.

---

Changes in v3:
- Use the max packet size info when reading the descriptor the second time

 common/usb.c | 160 ++++++++++++++++++++++++++++++++++++-----------------------
 1 file changed, 98 insertions(+), 62 deletions(-)
common/usb.c +98 -62
index f249acf..c5b9876 100644
@@ -891,10 +891,34 @@ int usb_legacy_port_reset(struct usb_device *hub, int portnr)
 	return 0;
 }
 
-static int usb_setup_descriptor(struct usb_device *dev, bool do_read)
+static int get_descriptor_len(struct usb_device *dev, int len)
 {
 	__maybe_unused struct usb_device_descriptor *desc;
 	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, tmpbuf, USB_BUFSIZ);
+	int err;
+
+	desc = (struct usb_device_descriptor *)tmpbuf;
+
+	err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, len);
+	if (err < sizeof(dev->descriptor)) {
+		if (err < 0) {
+			printf("unable to get device descriptor (error=%d)\n",
+			       err);
+			return err;
+		} else {
+			printf("USB device descriptor short read (expected %i, got %i)\n",
+			       (int)sizeof(dev->descriptor), err);
+			return -EIO;
+		}
+	}
+	memcpy(&dev->descriptor, tmpbuf, sizeof(dev->descriptor));
+
+	return 0;
+}
+
+static int usb_setup_descriptor(struct usb_device *dev, bool do_read)
+{
+	__maybe_unused struct usb_device_descriptor *desc;
 
 	/*
 	 * This is a Windows scheme of initialization sequence, with double
@@ -911,7 +935,6 @@ static int usb_setup_descriptor(struct usb_device *dev, bool do_read)
 	 * the maxpacket size is 8 or 16 the device may be waiting to transmit
 	 * some more, or keeps on retransmitting the 8 byte header. */
 
-	desc = (struct usb_device_descriptor *)tmpbuf;
 	dev->descriptor.bMaxPacketSize0 = 64;	    /* Start off at 64 bytes  */
 	/* Default to 64 byte max packet size */
 	dev->maxpacketsize = PACKET_SIZE_64;
@@ -921,19 +944,9 @@ static int usb_setup_descriptor(struct usb_device *dev, bool do_read)
 	if (do_read) {
 		int err;
 
-		err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, 64);
-		if (err < sizeof(dev->descriptor)) {
-			if (err < 0) {
-				printf("unable to get device descriptor (error=%d)\n",
-				       err);
-				return err;
-			} else {
-				printf("USB device descriptor short read (expected %i, got %i)\n",
-				       (int)sizeof(dev->descriptor), err);
-				return -EIO;
-			}
-		}
-		memcpy(&dev->descriptor, tmpbuf, sizeof(dev->descriptor));
+		err = get_descriptor_len(dev, 64);
+		if (err)
+			return err;
 	}
 
 	dev->epmaxpacketin[0] = dev->descriptor.bMaxPacketSize0;
@@ -994,71 +1007,41 @@ static int usb_prepare_device(struct usb_device *dev, int addr, bool do_read,
 	return 0;
 }
 
-/*
- * By the time we get here, the device has gotten a new device ID
- * and is in the default state. We need to identify the thing and
- * get the ball rolling..
- *
- * Returns 0 for success, != 0 for error.
- */
-int usb_new_device(struct usb_device *dev)
+static int usb_select_config(struct usb_device *dev)
 {
-	bool do_read = true;
-	int addr, err;
-	int tmp, ret;
 	ALLOC_CACHE_ALIGN_BUFFER(unsigned char, tmpbuf, USB_BUFSIZ);
+	int err;
 
-	/* We still haven't set the Address yet */
-	addr = dev->devnum;
-	dev->devnum = 0;
-
-	/*
-	 * XHCI needs to issue a Address device command to setup
-	 * proper device context structures, before it can interact
-	 * with the device. So a get_descriptor will fail before any
-	 * of that is done for XHCI unlike EHCI.
-	 */
-#ifdef CONFIG_USB_XHCI
-	do_read = false;
-#endif
-	ret = usb_prepare_device(dev, addr, do_read, dev->parent, dev->portnr);
-	if (ret)
-		return ret;
-
-	tmp = sizeof(dev->descriptor);
+	err = get_descriptor_len(dev, 64);
+	if (err)
+		return err;
 
-	err = usb_get_descriptor(dev, USB_DT_DEVICE, 0,
-				 tmpbuf, sizeof(dev->descriptor));
-	if (err < tmp) {
-		if (err < 0)
-			printf("unable to get device descriptor (error=%d)\n",
-			       err);
-		else
-			printf("USB device descriptor short read " \
-				"(expected %i, got %i)\n", tmp, err);
-		return 1;
-	}
-	memcpy(&dev->descriptor, tmpbuf, sizeof(dev->descriptor));
 	/* correct le values */
 	le16_to_cpus(&dev->descriptor.bcdUSB);
 	le16_to_cpus(&dev->descriptor.idVendor);
 	le16_to_cpus(&dev->descriptor.idProduct);
 	le16_to_cpus(&dev->descriptor.bcdDevice);
+
 	/* only support for one config for now */
 	err = usb_get_configuration_no(dev, tmpbuf, 0);
 	if (err < 0) {
 		printf("usb_new_device: Cannot read configuration, " \
 		       "skipping device %04x:%04x\n",
 		       dev->descriptor.idVendor, dev->descriptor.idProduct);
-		return -1;
+		return err;
 	}
 	usb_parse_config(dev, tmpbuf, 0);
 	usb_set_maxpacket(dev);
-	/* we set the default configuration here */
-	if (usb_set_configuration(dev, dev->config.desc.bConfigurationValue)) {
+	/*
+	 * we set the default configuration here
+	 * This seems premature. If the driver wants a different configuration
+	 * it will need to select itself.
+	 */
+	err = usb_set_configuration(dev, dev->config.desc.bConfigurationValue);
+	if (err < 0) {
 		printf("failed to set default configuration " \
 			"len %d, status %lX\n", dev->act_len, dev->status);
-		return -1;
+		return err;
 	}
 	debug("new device strings: Mfr=%d, Product=%d, SerialNumber=%d\n",
 	      dev->descriptor.iManufacturer, dev->descriptor.iProduct,
@@ -1078,11 +1061,64 @@ int usb_new_device(struct usb_device *dev)
 	debug("Manufacturer %s\n", dev->mf);
 	debug("Product      %s\n", dev->prod);
 	debug("SerialNumber %s\n", dev->serial);
-	/* now prode if the device is a hub */
-	usb_hub_probe(dev, 0);
+
 	return 0;
 }
 
+static int usb_setup_device(struct usb_device *dev, bool do_read,
+			    struct usb_device *parent, int portnr)
+{
+	int addr;
+	int ret;
+
+	/* We still haven't set the Address yet */
+	addr = dev->devnum;
+	dev->devnum = 0;
+
+	ret = usb_prepare_device(dev, addr, do_read, parent, portnr);
+	if (ret)
+		return ret;
+	ret = usb_select_config(dev);
+
+	return ret;
+}
+
+/*
+ * By the time we get here, the device has gotten a new device ID
+ * and is in the default state. We need to identify the thing and
+ * get the ball rolling..
+ *
+ * Returns 0 for success, != 0 for error.
+ */
+int usb_new_device(struct usb_device *dev)
+{
+	bool do_read = true;
+	int count;
+	int err;
+
+	/*
+	 * XHCI needs to issue a Address device command to setup
+	 * proper device context structures, before it can interact
+	 * with the device. So a get_descriptor will fail before any
+	 * of that is done for XHCI unlike EHCI.
+	 */
+#ifdef CONFIG_USB_XHCI
+	do_read = false;
+#endif
+	err = usb_setup_device(dev, do_read, dev->parent, dev->portnr);
+	if (err)
+		return err;
+
+	count = 1;
+	/* now prode if the device is a hub */
+	err = usb_hub_probe(dev, 0);
+	if (err < 0)
+		return err;
+	count += err;
+
+	return count;
+}
+
 __weak
 int board_usb_init(int index, enum usb_init_type init)
 {
Signature · 1 line
2.2.0.rc0.207.ga3a616c
#4 lore raw

PATCH v3 3/3 dm: usb: Complete the splitting up of usb_new_device()

Quoted 7 lines · Simon Glass

This function now calls usb_setup_device() to set up the device and usb_hub_probe() to check if it is a hub. The XHCI special case is now a parameter to usb_setup_device(). The latter will be used by the USB uclass when it is added, since it does not rely on any CONFIGs or legacy data structures.

Best regards,
Marek Vasut

#5 lore raw

PATCH v3 1/3 dm: usb: Move descriptor setup code into its own function

In reply to Simon Glass
Quoted 4 lines · Simon Glass

usb_new_device() is far too long and does far too much. As a first step, move the code that does initial setup and reads a descriptor into its own function called usb_setup_descriptor().

Quoted 8 lines

For XHCI the init order is different - we set up the device but don't actually read the descriptor until after we set an address. Support this option as a parameter to usb_setup_descriptor().

Avoid changing this torturous code more than necessary to make it easy to review.

Best regards,
Marek Vasut

#6 lore raw

PATCH v3 1/3 dm: usb: Move descriptor setup code into its own function

In reply to Simon Glass
Quoted 10 lines · Simon Glass

usb_new_device() is far too long and does far too much. As a first step, move the code that does initial setup and reads a descriptor into its own function called usb_setup_descriptor().

For XHCI the init order is different - we set up the device but don't actually read the descriptor until after we set an address. Support this option as a parameter to usb_setup_descriptor().

Avoid changing this torturous code more than necessary to make it easy to review.

The series,

Note that what I actually tested was the commits in u-boot-dm/next, which in another email you mentioned included these patches.

#7 lore raw

PATCH v3 1/3 dm: usb: Move descriptor setup code into its own function

Hi Stephen,

Quoted 16 lines · Stephen Warren
Quoted 7 lines · Simon Glass

usb_new_device() is far too long and does far too much. As a first step, move
the code that does initial setup and reads a descriptor into its own function
called usb_setup_descriptor().

Quoted 6 lines

For XHCI the init order is different - we set up the device but don't actually read the descriptor until after we set an address. Support this option as a parameter to usb_setup_descriptor().

Avoid changing this torturous code more than necessary to make it easy to review.

Quoted 6 lines

The series,

Note that what I actually tested was the commits in u-boot-dm/next, which in another email you mentioned included these patches.

That's right. Thanks - I'll work on getting a pull request for this lot soon.

Regards,
Simon