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

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.