Dear Vivek Gautam, > This adds usb framework support for super-speed usb, which will > further facilitate to add stack support for xHCI. > > Signed-off-by: Vikas C Sajjan <vikas.saj...@samsung.com> > Signedoff-by: Vivek Gautam <gautam.vi...@samsung.com>
CCing Benoit, (help me! ;-) ) I'll be blunt and technical below, try not to take it personally please. > --- > common/cmd_usb.c | 6 +- > common/usb.c | 41 ++++++++- > common/usb_hub.c | 26 +++++- > common/usb_storage.c | 35 +++++---- > include/common.h | 2 + > include/linux/usb/ch9.h | 2 +- > include/usb.h | 15 +++- > include/usb_defs.h | 26 ++++++- > include/usbdescriptors.h | 201 > ++++++++++++++++++++++++++++++++++++++++++++++ 9 files changed, 322 > insertions(+), 32 deletions(-) > > diff --git a/common/cmd_usb.c b/common/cmd_usb.c > index c128455..013b2e8 100644 > --- a/common/cmd_usb.c > +++ b/common/cmd_usb.c > @@ -262,7 +262,7 @@ void usb_display_config(struct usb_device *dev) > ifdesc = &config->if_desc[i]; > usb_display_if_desc(&ifdesc->desc, dev); > for (ii = 0; ii < ifdesc->no_of_ep; ii++) { > - epdesc = &ifdesc->ep_desc[ii]; > + epdesc = &ifdesc->ep_desc[ii].ep_desc; Fix, split into separate patch > usb_display_ep_desc(epdesc); > } > } > @@ -271,7 +271,9 @@ void usb_display_config(struct usb_device *dev) > > static inline char *portspeed(int speed) > { > - if (speed == USB_SPEED_HIGH) > + if (speed == USB_SPEED_SUPER) > + return "5 Gb/s"; > + else if (speed == USB_SPEED_HIGH) > return "480 Mb/s"; > else if (speed == USB_SPEED_LOW) > return "1.5 Mb/s"; Can you convert this into switch please? > diff --git a/common/usb.c b/common/usb.c > index 50b8175..691d9ac 100644 > --- a/common/usb.c > +++ b/common/usb.c > @@ -304,7 +304,7 @@ usb_set_maxpacket_ep(struct usb_device *dev, int > if_idx, int ep_idx) struct usb_endpoint_descriptor *ep; > u16 ep_wMaxPacketSize; > > - ep = &dev->config.if_desc[if_idx].ep_desc[ep_idx]; > + ep = &dev->config.if_desc[if_idx].ep_desc[ep_idx].ep_desc; > > b = ep->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK; > ep_wMaxPacketSize = get_unaligned(&ep->wMaxPacketSize); > @@ -360,6 +360,7 @@ static int usb_parse_config(struct usb_device *dev, > int index, ifno, epno, curr_if_num; > int i; > u16 ep_wMaxPacketSize; > + struct usb_interface *if_desc = NULL; > > ifno = -1; > epno = -1; > @@ -401,21 +402,27 @@ static int usb_parse_config(struct usb_device *dev, > break; > case USB_DT_ENDPOINT: > epno = dev->config.if_desc[ifno].no_of_ep; > + if_desc = &dev->config.if_desc[ifno]; > /* found an endpoint */ > - dev->config.if_desc[ifno].no_of_ep++; > - memcpy(&dev->config.if_desc[ifno].ep_desc[epno], > + if_desc->no_of_ep++; > + memcpy(&if_desc->ep_desc[epno].ep_desc, This change is irrelevant, it's a fix so put it into separate patch with proper explanation. > &buffer[index], buffer[index]); > ep_wMaxPacketSize = get_unaligned(&dev->config.\ > if_desc[ifno].\ > ep_desc[epno].\ > - wMaxPacketSize); > + ep_desc.wMaxPacketSize); What does this change do/fix ? > put_unaligned(le16_to_cpu(ep_wMaxPacketSize), > &dev->config.\ > if_desc[ifno].\ > ep_desc[epno].\ > - wMaxPacketSize); > + ep_desc.wMaxPacketSize); > USB_PRINTF("if %d, ep %d\n", ifno, epno); > break; > + case USB_DT_SS_ENDPOINT_COMP: > + if_desc = &dev->config.if_desc[ifno]; > + memcpy(&(if_desc->ep_desc[epno].ss_ep_comp), Do you need these braces in &(...) ? > + &buffer[index], buffer[index]); > + break; > default: > if (head->bLength == 0) > return 1; > @@ -659,6 +666,18 @@ static int usb_get_string(struct usb_device *dev, > unsigned short langid, return result; > } > > +/* Allocate usb device */ > +int usb_alloc_dev(struct usb_device *dev) > +{ > + int res; > + > + USB_PRINTF("alloc device\n"); this is a debug print. > + res = usb_control_msg(dev, usb_sndctrlpipe(dev->parent, 0), > + USB_REQ_ALLOC_DEV, 0, 0, 0, > + NULL, 0, USB_CNTL_TIMEOUT); How does this "allocate" a device? Please, do a proper documentation. Actually, take a look at include/linker_lists.h Or see here: http://git.denx.de/?p=u- boot.git;a=blob;f=include/linker_lists.h;h=0b405d78ea34df1c528fbc4e24ed2aad756ac4a2;hb=HEAD And here (U-Boot Code Documentation): http://www.denx.de/wiki/U-Boot/CodingStyle It'd be really nice if you could follow such pattern of documentation! > + return res; > +} > > static void usb_try_string_workarounds(unsigned char *buf, int *length) > { > @@ -852,7 +871,10 @@ int usb_new_device(struct usb_device *dev) > struct usb_device_descriptor *desc; > int port = -1; > struct usb_device *parent = dev->parent; > + > +#ifndef CONFIG_USB_XHCI > unsigned short portstatus; Use __maybe_unused instead so it's not poluted with these #ifdefs ? > +#endif > > /* send 64-byte GET-DEVICE-DESCRIPTOR request. Since the descriptor is > * only 18 bytes long, this will terminate with a short packet. But if > @@ -889,12 +911,21 @@ int usb_new_device(struct usb_device *dev) > return 1; > } > > + /* > + * Putting up the code here for slot assignment and initialization: > + * xHCI spec sec 4.3.2, 4.3.3 > + */ Misaligned comment? > +#ifdef CONFIG_USB_XHCI > + /* Call if and only if device is attached and detected */ > + usb_alloc_dev(dev); > +#else > /* reset the port for the second time */ > err = hub_port_reset(dev->parent, port, &portstatus); > if (err < 0) { > printf("\n Couldn't reset port %i\n", port); > return 1; > } > +#endif > } > #endif > > diff --git a/common/usb_hub.c b/common/usb_hub.c > index e4a1201..8a00894 100644 > --- a/common/usb_hub.c > +++ b/common/usb_hub.c > @@ -204,7 +204,12 @@ int hub_port_reset(struct usb_device *dev, int port, > } > > > -void usb_hub_port_connect_change(struct usb_device *dev, int port) > +/* > + * Adding the flag do_port_reset: USB 2.0 device requires port reset > + * while USB 3.0 device does not. > + */ > +void usb_hub_port_connect_change(struct usb_device *dev, int port, > + int do_port_reset) Make it uint32_t flags so once usb4.0 (or MS's magic USB port where you can connect joystick :D ) arrives and it needs two and half resets of a port, we just add another flag. > { > struct usb_device *usb; > ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1); > @@ -235,11 +240,21 @@ void usb_hub_port_connect_change(struct usb_device > *dev, int port) } > mdelay(200); > > +#ifdef CONFIG_USB_XHCI > + /* Reset the port */ > + if (do_port_reset) { > + if (hub_port_reset(dev, port, &portstatus) < 0) { > + printf("cannot reset port %i!?\n", port + 1); > + return; > + } > + } > +#else > /* Reset the port */ > if (hub_port_reset(dev, port, &portstatus) < 0) { > printf("cannot reset port %i!?\n", port + 1); > return; > } > +#endif I'm sure #ifdef ... do_port_reset = 1 #endif would work just fine ... then you'd avoid such massive blocks of ifdef'd code. > mdelay(200); > > @@ -409,7 +424,10 @@ static int usb_hub_configure(struct usb_device *dev) > > if (portchange & USB_PORT_STAT_C_CONNECTION) { > USB_HUB_PRINTF("port %d connection change\n", i + 1); > - usb_hub_port_connect_change(dev, i); > + usb_hub_port_connect_change(dev, i, 0); > + } else if ((portstatus & 0x1) == 1) { Magic constant ... NAK > + USB_HUB_PRINTF("port %d connection change\n", i + 1); > + usb_hub_port_connect_change(dev, i, 1); > } > if (portchange & USB_PORT_STAT_C_ENABLE) { > USB_HUB_PRINTF("port %d enable change, status %x\n", [...] > @@ -278,10 +278,11 @@ int usb_stor_scan(int mode) > lun++) { > usb_dev_desc[usb_max_devs].lun = lun; > if (usb_stor_get_info(dev, &usb_stor[start], > - &usb_dev_desc[usb_max_devs]) == 1) { > - usb_max_devs++; > - } > + &usb_dev_desc[usb_max_devs]) == 1) { > + usb_max_devs++; > + } > } > + > } Can we fix this depth of indend somehow? > /* if storage device */ > if (usb_max_devs == USB_MAX_STOR_DEV) { > @@ -511,7 +512,7 @@ int usb_stor_BBB_comdat(ccb *srb, struct us_data *us) > dir_in = US_DIRECTION(srb->cmd[0]); > > #ifdef BBB_COMDAT_TRACE > - printf("dir %d lun %d cmdlen %d cmd %p datalen %d pdata %p\n", > + printf("dir %d lun %d cmdlen %d cmd %p datalen %lu pdata %p\n", Fixes should certainly be submitted as separate patches prior to feature additions. > dir_in, srb->lun, srb->cmdlen, srb->cmd, srb->datalen, > srb->pdata); > if (srb->cmdlen) { > @@ -1208,6 +1209,7 @@ int usb_storage_probe(struct usb_device *dev, > unsigned int ifnum, { > struct usb_interface *iface; > int i; > + struct usb_endpoint_descriptor *ep_desc; > unsigned int flags = 0; > > int protocol = 0; > @@ -1290,24 +1292,25 @@ int usb_storage_probe(struct usb_device *dev, > unsigned int ifnum, * We will ignore any others. > */ > for (i = 0; i < iface->desc.bNumEndpoints; i++) { > + ep_desc = &iface->ep_desc[i].ep_desc; > /* is it an BULK endpoint? */ > - if ((iface->ep_desc[i].bmAttributes & > - USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_BULK) { > - if (iface->ep_desc[i].bEndpointAddress & USB_DIR_IN) > - ss->ep_in = iface->ep_desc[i].bEndpointAddress & > - USB_ENDPOINT_NUMBER_MASK; > + if ((ep_desc->bmAttributes & > + USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_BULK) { > + if (ep_desc->bEndpointAddress & USB_DIR_IN) > + ss->ep_in = ep_desc->bEndpointAddress & > + USB_ENDPOINT_NUMBER_MASK; > else > ss->ep_out = > - iface->ep_desc[i].bEndpointAddress & > + ep_desc->bEndpointAddress & > USB_ENDPOINT_NUMBER_MASK; > } > > /* is it an interrupt endpoint? */ > - if ((iface->ep_desc[i].bmAttributes & > - USB_ENDPOINT_XFERTYPE_MASK) == USB_ENDPOINT_XFER_INT) { > - ss->ep_int = iface->ep_desc[i].bEndpointAddress & > - USB_ENDPOINT_NUMBER_MASK; > - ss->irqinterval = iface->ep_desc[i].bInterval; > + if ((ep_desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) > + == USB_ENDPOINT_XFER_INT) { > + ss->ep_int = ep_desc->bEndpointAddress & > + USB_ENDPOINT_NUMBER_MASK; > + ss->irqinterval = ep_desc->bInterval; Are you just introducing new variable here? > } > } > USB_STOR_PRINTF("Endpoints In %d Out %d Int %d\n", > diff --git a/include/common.h b/include/common.h > index b23e90b..ef5f687 100644 > --- a/include/common.h > +++ b/include/common.h > @@ -211,6 +211,8 @@ typedef void (interrupt_handler_t)(void *); > #define MIN(x, y) min(x, y) > #define MAX(x, y) max(x, y) > > +#define min3(a, b, c) min(min(a, b), c) > + Isn't this defined somewhere already? [...] Rest is just great. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot