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

Reply via email to