Dear Marek,
CCing Simon Glass.
On Tue, Oct 23, 2012 at 5:10 PM, Marek Vasut <ma...@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.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
>
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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
