dm: usb: Move descriptor setup code into its own function
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
@@ -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
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
@@ -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
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().Signed-off-by Simon Glass <[email protected]>---
Changes in v3:
- Fix polarity ofCONFIG_XHCIcheck (which broke USB keyboards)
Best regards,
Marek Vasut
PATCH v3 3/3 dm: usb: Complete the splitting up of usb_new_device()
In reply to Simon GlassThis 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
@@ -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
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 andusb_hub_probe()to check if it is a hub. The XHCI special case is now a parameter tousb_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 <[email protected]>
Best regards,
Marek Vasut
PATCH v3 1/3 dm: usb: Move descriptor setup code into its own function
In reply to Simon GlassQuoted 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 calledusb_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.
Signed-off-by Simon Glass <[email protected]>
Best regards,
Marek Vasut
PATCH v3 1/3 dm: usb: Move descriptor setup code into its own function
In reply to Simon GlassQuoted 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 calledusb_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.
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
calledusb_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,
Tested-by Stephen Warren <[email protected]>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