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

[Marek Vasut]

Dear Vivek Gautam,

[...]
> 
> 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".

I see, can we split this? I really enjoy small incremental patches, it's much 
easier to bisect issues then.

> >>                       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 ?

Yes, but I'd prefer to kill USB_PRINTF altogether in favor of debug().

> >> +     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=0b405d78ea34df1c528fbc4e24ed2a
> > ad756ac4a2;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.

_AWESOME_ !

> >> +     return res;
> >> +}
> >> 

[...]

> >> 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 :(

ICK, so we have ad-hoc implementation of this :-( I'd say, put it into common.h 
and remove the ehci's ad-hoc implementation.

> > [...]
> > 
> > Rest is just great.
> 
> Thanks for reviewing this Marek. I shall update the patchset soon.

Welcome, it's pleasure to work with you ;-)