Re: [RFC/PATCH 2/2] usb:gadget: Add SuperSpeed support to the Gadget Framework

[tlinder]

> Hi Tatyana,
>
> Comments inline.  I'm not familiar with the gadget framework; I'm just
> curious about some descriptor choices.
>
> On Sun, Oct 03, 2010 at 10:02:15AM +0200, tlinder wrote:
>> diff --git a/drivers/usb/gadget/composite.c
>> b/drivers/usb/gadget/composite.c
>> index c619c80..a5dcfe1 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -70,6 +70,102 @@ static char *iSerialNumber;
>>  module_param(iSerialNumber, charp, 0);
>>  MODULE_PARM_DESC(iSerialNumber, "SerialNumber string");
>>
>> +/** Default endpoint companion descriptor */
>> +static struct usb_ss_ep_comp_descriptor ep_comp_desc = {
>> +		.bDescriptorType = USB_DT_SS_ENDPOINT_COMP,
>> +		.bLength = 0x06,
>> +		.bMaxBurst = 0, /*the default is we don't support bursting*/
>> +		.bmAttributes = 0, /*2^0 streams supported*/
>> +		.wBytesPerInterval = 0,
>> +};
>
> Can you please set wBytesPerInterval to something sane for periodic
> endpoints?  Perhaps have it set to the maximum packet size times the max
> burst size times Mult plus one, or less if the device *knows* it's going
> to send less data.  It's used for xHC host controller scheduling, so
> it's important to get right for maximum bandwidth usage.

This descriptor holds default values so both bMaxBurst and bmAttributes
are set to 0, meaning bursting and streaming are not not supported. So
Mult will be set to 0 as well. Mult defined only for iso endpoints and not
for interrupt.
Due to the above I propose setting wBytesPerInterval to maxpacketsize for
periodic endpoints.

>> +/**This function receives a pointer to usb_function and adds
>> + * missing super speed descriptors in the ss_descriptor field
>> + * according to its hs_descriptors field.
>> + *
>> + * In more details: this function copies f->hs_descriptors while
>> + * updating the endpoint descriptor and adding endpoint
>> + * companion descriptor
>> + */
>> +static void create_ss_descriptors(struct usb_function *f)
>> +{
>> +	unsigned bytes; /*number of bytes to allocate*/
>> +	unsigned n_desc;    /* number of descriptors*/
>> +	void *mem; /*allocated memory to copy to*/
>> +	struct usb_descriptor_header **tmp;
>> +	struct usb_endpoint_descriptor	*ep_desc ;
>> +	struct usb_descriptor_header **src = f->hs_descriptors;
>> +
>> +	if (!f->hs_descriptors)
>> +		return;
>> +
>> +	/* Count number of EPs (in order to know how many SS_EP_COMPANION
>> +	   descriptors to add), the total number of descriptors and the sum of
>> +	   each descriptor bLength field in order to know how much memory to
>> +	   allocate*/
>> +	for (bytes = 0, n_desc = 0, tmp = src; *tmp; tmp++, n_desc++) {
>> +		if ((*tmp)->bDescriptorType == USB_DT_ENDPOINT) {
>> +			bytes += ep_comp_desc.bLength;
>> +			n_desc++;
>> +		}
>> +		bytes += (*tmp)->bLength;
>> +	}
>> +
>> +	bytes += (n_desc + 1) * sizeof(*tmp);
>> +	mem = kmalloc(bytes, GFP_KERNEL);
>> +	if (!mem)
>> +		return;
>> +
>> +	/* fill in pointers starting at "tmp",
>> +	 * to descriptors copied starting at "mem";
>> +	 * and return "ret"
>> +	 */
>> +	tmp = mem;
>> +	f->ss_descriptors  = mem;
>> +	mem += (n_desc + 1) * sizeof(*tmp);
>> +	while (*src) {
>> +		/*Copy the original descriptor*/
>> +		memcpy(mem, *src, (*src)->bLength);
>> +		switch ((*src)->bDescriptorType) {
>> +		case USB_DT_ENDPOINT:
>> +			/*update ep descriptor*/
>> +			ep_desc = (struct usb_endpoint_descriptor *)mem;
>> +			switch (ep_desc->bmAttributes &
>> +				USB_ENDPOINT_XFERTYPE_MASK) {
>> +			case USB_ENDPOINT_XFER_CONTROL:
>> +				ep_desc->wMaxPacketSize = 512;
>> +				ep_desc->bInterval = 0;
>> +				break;
>> +			case USB_ENDPOINT_XFER_BULK:
>> +				ep_desc->wMaxPacketSize = 1024;
>> +				ep_desc->bInterval = 0;
>> +				break;
>> +			case USB_ENDPOINT_XFER_INT:
>> +			case USB_ENDPOINT_XFER_ISOC:
>
> Why are you not setting wMaxPacketSize for periodic endpoints?  Does it
> get set later?  (I can't tell from this snippet.)

It's not set later. According to the USB30 Spec Table 9-18, the
description of wMaxPacketSize for interrupt and iso endpoints:
"..if bMuxBurst field is set to zero then this field can have any value
from 0..1024 for isochronous endpoints and 1..1042 for an interrupt
endpoint." Since bMuxBurst default is 0 we decided to leave this fields
value as it was in the HighSpeed descriptor.

>> +				break;
>> +			}
>> +			*tmp = mem;
>> +			tmp++;
>> +			mem += (*src)->bLength;
>> +			/*add ep companion descriptor*/
>> +			memcpy(mem, &ep_comp_desc, ep_comp_desc.bLength);
>> +			*tmp = mem;
>> +			tmp++;
>> +			mem += ep_comp_desc.bLength;
>> +			break;
>> +		default:
>> +			*tmp = mem;
>> +			tmp++;
>> +			mem += (*src)->bLength;
>> +			break;
>> +		}
>> +		src++;
>> +	}
>> +	*tmp = NULL; /*The last (struct usb_descriptor_header *) in the
>> +			descriptors vector is NULL*/
>> +	f->ss_desc_allocated = true;
>> +}
>> +
>
> ...
>
>> +/**
>> + * bos() - prepares the BOS (Binary Device Object) descriptor
>> + * and its device capabilities descriptors. The bos descriptor
>> + * should be supported by a Superspeed device.
>> + */
>> +static int bos(struct usb_composite_dev *cdev)
>> +{
>> +	struct usb_bos_descriptor	*bos = cdev->req->buf;
>> +	struct usb_ext_cap_descriptor	*usb_ext = NULL;
>> +	struct ss_usb_cap_descriptor	*ss_cap = NULL;
>> +
>> +	bos->bLength = USB_DT_BOS_SIZE;
>> +	bos->bDescriptorType = USB_DT_BOS;
>> +
>> +	bos->wTotalLength = USB_DT_BOS_SIZE;
>> +	bos->bNumDeviceCaps = 0;
>> +
>> +	/* A SuperSpeed device shall include the USB2.0 extension descriptor
>> and
>> +	   shall support LPM when operating in USB2.0 HS mode.*/
>> +	usb_ext = (struct usb_ext_cap_descriptor *)
>> +			(cdev->req->buf+bos->wTotalLength);
>> +	bos->bNumDeviceCaps++;
>> +	bos->wTotalLength += USB_DT_USB_EXT_CAP_SIZE;
>> +	usb_ext->bLength = USB_DT_USB_EXT_CAP_SIZE;
>> +	usb_ext->bDescriptorType = USB_DT_DEVICE_CAPABILITY;
>> +	usb_ext->bDevCapabilityType = USB_CAP_TYPE_EXT;
>> +	usb_ext->bmAttributes = USB_LPM_SUPPORT;
>> +
>> +	/* The Superspeed USB Capability descriptor shall be implemented by
>> all
>> +	   Superspeed devices. */
>> +	ss_cap = (struct ss_usb_cap_descriptor *)
>> +		(cdev->req->buf+bos->wTotalLength);
>> +	bos->bNumDeviceCaps++;
>> +	bos->wTotalLength += USB_DT_SS_USB_CAP_SIZE;
>> +	ss_cap->bLength = USB_DT_SS_USB_CAP_SIZE;
>> +	ss_cap->bDescriptorType = USB_DT_DEVICE_CAPABILITY;
>> +	ss_cap->bDevCapabilityType = SS_USB_CAP_TYPE;
>> +	ss_cap->bmAttributes = 0; /*LTM is not supported yet*/
>> +	ss_cap->wSpeedSupported = USB_LOW_SPEED_OPERATION |
>> +				USB_FULL_SPEED_OPERATION |
>> +				USB_HIGH_SPEED_OPERATION |
>> +				USB_5GBPS_OPERATION;
>
> So this gadget will be able to support all speeds?

Yes, that's correct.

>
>> +	ss_cap->bFunctionalitySupport = USB_LOW_SPEED_OPERATION;
>> +	ss_cap->bU1devExitLat = 0;
>> +	ss_cap->bU2DevExitLat = 0;
>
> Are you really sure you want to set the exit latency for low power
> states to less than 1 microsecond?  Without real hardware it would be
> difficult to test, but this seems overly optimistic.

We will set it to the maximum value according to the USB30 spec:
ss_cap->bU1devExitLat = 0x0A (less then 10 microsec)
ss_cap->bU2DevExitLat = 0x07FF (less then 2047 microsec)

>
>
>> diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h
>> index da2ed77..69e528a 100644
>> --- a/include/linux/usb/ch9.h
>> +++ b/include/linux/usb/ch9.h
>
> ...
>
>> +/*Container ID Capability descriptor: Defines the instance unique ID
>> used to
>> +  identify the instance across all operating modes*/
>> +#define	CONTAINER_ID_TYPE	4
>> +struct ss_usb_container_id_descriptor {
>> +	__u8  bLength;
>> +	__u8  bDescriptorType;
>> +	__u8  bDevCapabilityType;
>> +	__u8  bReserved;
>> +	__u8  ContainerID[16]; /*128-bit number*/
>> +} __attribute__((packed));
>> +
>> +#define USB_DT_SS_CONTN_ID_SIZE	20
>
> I don't see generation of a container ID in the code.  Are you going to
> implement it?  It's only required for USB 3.0 hubs, and I'm not sure if
> the gadget framework supports hubs.  Nothing else in your code seems to
> use this definition.

This definition was moved to another patch that includes only USB30 ch9
definitions.

> Sarah
>


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/