On Tue, Oct 05, 2010 at 04:53:40AM -0700, tlinder@xxxxxxxxxxxxxx wrote:
> > 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:
> >> +/** 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.
Sounds good.
> >> + 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.
Ok. I suppose whatever gadget application is being used can reset these
values later? So that if you had a gadget webcam, it could set the
wMaxPacketSize to the frame size or whatever it needed?
> >> + 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)
That will give you *horrible* power management. The whole point of the
link power management is to allow the device to go to sleep between
packets. Pick some non-zero, lower default.
How are you going to implement link power management on the gadget side,
btw? I know that the Linux USB host side doesn't support link PM yet,
but if it did, how would the gadget power down pieces of itself when it
receives a link PM request? Do you need some hooks that specific
hardware implementations can register with the gadget interface?
Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
