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

[Vivek Gautam]

Hi Marek,

On Fri, Oct 26, 2012 at 3:48 PM, Marek Vasut <marex at denx.de> wrote:
> 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.
>
Sure, will split this small change.

>> >>                       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().
>
Ok, will switch to debug(...).
And may be in that case we may try to eliminate USB _PRINTF and
other such macro altogether from sub framework ?

>> >> +     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.
>
Yeah, sure will keep this in common.h and update ehci-hcd.

>> > [...]
>> >
>> > Rest is just great.
>>
>> Thanks for reviewing this Marek. I shall update the patchset soon.
>
> Welcome, it's pleasure to work with you ;-)


-- 
Thanks & Regards
Vivek