[U-Boot] [RFC PATCH 1/2] USB: SS: Add support for Super Speed USB interface
Marek Vasut
marex at denx.de
Tue Oct 23 13:40:11 CEST 2012
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
> 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.
More information about the U-Boot
mailing list