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

[Marek Vasut <marex@denx.de>]

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@samsung.com>
> Signedoff-by: Vivek Gautam <gautam.vivek@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.