[U-Boot] [RFC PATCH 1/2] USB: SS: Add support for Super Speed USB interface

Vivek Gautam gautamvivek1987 at gmail.com
Fri Oct 26 12:07:59 CEST 2012


Dear Marek,

CCing Simon Glass.

On Tue, Oct 23, 2012 at 5:10 PM, Marek Vasut <marex at denx.de> wrote:
> 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.sajjan at samsung.com>
>> Signedoff-by: Vivek Gautam <gautam.vivek at 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
>
Sure, will split these fixes in a 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?
>
Sure, will make a switch for this. That whould be better.

>> 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.
>
Shall put all these fixes in a separate patch.

>>                               &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 ?
>
This comes as an affect of introduction of "struct usb_ep_desc"
which eventually contains "struct usb_endpoint_descriptor" and
"struct usb_ss_ep_comp_descriptor".

>>                       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 &(...) ?
>
True, we can remove these braces.

>> +                             &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.
>
Isn't USB_PRINTF itself a conditional 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!
>
Yes, thanks for pointing out. Will document it properly to make things
more understandable.

>> +     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 ?
>
Right. will change this accordingly.

>> +#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?
>
Sure, will correct it.

>> +#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.
>
Ok, will do this.

>>  {
>>       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.
>
Will try to fix this.

>>       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
>
Sure, missed the proper macro here. ;) Will amend it.

>> +                     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?
>
Ok, will try to fix this chunk.

>>               /* 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.
>
Sure, fixes in a separate patch :-)

>>               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?
>
This is again a fix. As pointed here "ep_desc =
&iface->ep_desc[i].ep_desc", fixes
things around introduction of "struct usb_ep_desc".

>>               }
>>       }
>>       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?
>
Can you please guide me here, i can see a similar inline function in
ehci-hcd only :(

> [...]
>
> Rest is just great.

Thanks for reviewing this Marek. I shall update the patchset soon.

Thanks & Regards
Vivek


More information about the U-Boot mailing list