[U-Boot] [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.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v3: - Fix polarity of CONFIG_XHCI check (which broke USB keyboards)
common/usb.c | 117 ++++++++++++++++++++++++++++++++++------------------------- 1 file changed, 67 insertions(+), 50 deletions(-)
diff --git a/common/usb.c b/common/usb.c index 4ab2213..e5df98f 100644 --- a/common/usb.c +++ b/common/usb.c @@ -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 */
Move the code that sets up the device with a new address into its own function, usb_prepare_device().
Signed-off-by: Simon Glass sjg@chromium.org
---
Changes in v3: - Fix polarity of CONFIG_XHCI check (which broke USB keyboards)
common/usb.c | 72 +++++++++++++++++++++++++++++++++++------------------------- 1 file changed, 42 insertions(+), 30 deletions(-)
diff --git a/common/usb.c b/common/usb.c index e5df98f..f249acf 100644 --- a/common/usb.c +++ b/common/usb.c @@ -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,
On Tuesday, April 14, 2015 at 05:19:03 AM, Simon Glass wrote:
Move the code that sets up the device with a new address into its own function, usb_prepare_device().
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v3:
- Fix polarity of CONFIG_XHCI check (which broke USB keyboards)
Reviewed-by: Marek Vasut marex@denx.de
Best regards, Marek Vasut
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.
Signed-off-by: Simon Glass sjg@chromium.org ---
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(-)
diff --git a/common/usb.c b/common/usb.c index f249acf..c5b9876 100644 --- a/common/usb.c +++ b/common/usb.c @@ -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) {
On Tuesday, April 14, 2015 at 05:19:04 AM, Simon Glass wrote:
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.
Signed-off-by: Simon Glass sjg@chromium.org
Reviewed-by: Marek Vasut marex@denx.de
Best regards, Marek Vasut
On Tuesday, April 14, 2015 at 05:19:02 AM, Simon Glass wrote:
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.
Signed-off-by: Simon Glass sjg@chromium.org
Reviewed-by: Marek Vasut marex@denx.de
Best regards, Marek Vasut
On 04/13/2015 09:19 PM, Simon Glass wrote:
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,
Tested-by: Stephen Warren swarren@nvidia.com
Note that what I actually tested was the commits in u-boot-dm/next, which in another email you mentioned included these patches.
Hi Stephen,
On 14 April 2015 at 08:56, Stephen Warren swarren@wwwdotorg.org wrote:
On 04/13/2015 09:19 PM, Simon Glass wrote:
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,
Tested-by: Stephen Warren swarren@nvidia.com
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
participants (3)
-
Marek Vasut -
Simon Glass -
Stephen Warren