Hi Dmitry,
On 01/06/11 07:37, Dmitry Torokhov wrote:
> On Tue, May 31, 2011 at 02:00:46PM -0700, Sarah Sharp wrote:
>> On Tue, May 31, 2011 at 12:08:14PM -0700, Dmitry Torokhov wrote:
>>> On Tue, May 31, 2011 at 11:34:58AM -0700, Sarah Sharp wrote:
>>>> On Tue, May 31, 2011 at 09:49:41AM -0700, Dmitry Torokhov wrote:
>>>>> Hi Matt,
>>>>>
>>>>> On Tue, May 31, 2011 at 03:52:16PM +1000, Matt Evans wrote:
>>>>>> Commit dfa49c4ad120a784ef1ff0717168aa79f55a483a
>>>>>> "USB: xhci - fix math in xhci_get_endpoint_interval()"
>>>>>> expands the clamping of the interval value to 0-15, which is only legal
>>>>>> for HS/SS endpoints. For FS, it must be 3-11. When testing with FS
>>>>>> isochronous endpoints (an audio device), values outside this range caused
>>>>>> the following error 0x11 (again) on an NEC xHCI controller:
>>>>>>
>>>>>> xhci_hcd 0000:01:00.0: ERROR: unexpected command completion code 0x11.
>>>>>> usb 1-1: Not enough bandwidth for altsetting 1
>>>>>>
>>>>>
>>>>> Hmm, however USB2.0 spec (to whcih high/full/low-speed devices should conform
>>>>> states:
>>>>>
>>>>> "For full-/high-speed isochronous endpoints, this value must be in the
>>>>> range from 1 to 16. The bInterval value bInterval-1 is used as the
>>>>> exponent for a 2 value; e.g., a 4-1 bInterval of 4 means a period of 8
>>>>> (2 )."
>>>>
>>>> It doesn't really matter what the USB 2.0 specification says. If the
>>>> xHCI specification says that xHCI hosts can only handle FS intervals of
>>>> 3-11, and host controllers are going to reject Configure Endpoint
>>>> commands based on what the spec says, then the xHCI driver needs to
>>>> limit the interval in the same way.
>>>>
>>>> It could be that the xHCI spec architect chose to limit the FS intervals
>>>> for a reason, much the same way they chose to limit the interval to
>>>> powers of two. Or it could have just been a typo.
>>>>
>>>> If the xHCI spec is wrong, then you should email xhcispec@xxxxxxxxx and
>>>> try to get an errata put in for it. But even then, we won't be able to
>>>> tell which hosts implement the errata, so we'll probably have to stick
>>>> with the lower interval limit.
>>>
>>> xHCI spec seems to be confused indeed. Compare:
>>>
>>> Table 56:
>>> "The legal range of values is 3 to 10 for Full- or Low-speed Interrupt
>>> endpoints (1 to 256 ms.) and 0 to 15 for all other endpoint types (125
>>> us to 4096 us)."
>>>
>>> This suggests that full- and low-speed interrupt endpoints specify
>>> interval in exponent-based form.
>>>
>>> However 6.2.3.6 states:
>>>
>>> For low-speed Interrupt and full-speed Interrupt and Isoch endpoints the
>>> bInterval field declared by a Full-or Low-speed device is computed as
>>> bInterval * 1ms., where bInterval = 1 to 255."
>>
>> My updated version of the 1.0 spec (with all the errata up to Jan. 11,
>> 2011) has a nice table in that section instead. I'm just going to
>> attach a screenshot of it. Basically, it says to clamp FS devices at
>> 10, with the exception of FS isochronous devices, which can go up to 18.
>>
>> Of course, it's possible that the NEC host controller is doing some
>> internal checking of the interval, and won't accept FS isochronous
>> intervals greater than 10. I guess we would need to clamp to 18 only
>> for xHCI 1.0 hosts.
>
> No, what we were missingis that units of measurement for FS isoc
> endpoints is frames, not microframes. I think the following patch is
> what we need.
Aha, great! I missed that unit-change subtlety in my patch. Though my test
machine's unavailable ATM, the code will give the right result (3) for the
bInterval (1) in my USB audio isoch endpoints. Happy for my patch to be kicked
down the well in favour of this. :)
> Thanks,
> Dmitry
>
>
> Subject: USB: xhci - fix interval calculation for FS isoc endpoints
>
> Full-speed isoc endpoints specify interval in exponent based form
> in frames, not microframes, so we need to adjust accordingly.
>
> Reported-by: Matt Evans <matt@xxxxxxxxxx>
> Signed-off-by: Dmitry Torokhov <dtor@xxxxxxxxxx>
Acked-by: Matt Evans <matt@xxxxxxxxxx>
Should also CC stable to follow your original patch, no?
Thanks,
Matt
> ---
> drivers/usb/host/xhci-mem.c | 14 ++++++++++++--
> 1 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index 26caba4..0f8e1d2 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -985,9 +985,19 @@ static unsigned int xhci_parse_exponent_interval(struct usb_device *udev,
> interval = clamp_val(ep->desc.bInterval, 1, 16) - 1;
> if (interval != ep->desc.bInterval - 1)
> dev_warn(&udev->dev,
> - "ep %#x - rounding interval to %d microframes\n",
> + "ep %#x - rounding interval to %d %sframes\n",
> ep->desc.bEndpointAddress,
> - 1 << interval);
> + 1 << interval,
> + udev->speed == USB_SPEED_FULL ? "" : "micro");
> +
> + if (udev->speed == USB_SPEED_FULL) {
> + /*
> + * Full speed isoc endpoints specify interval in frames,
> + * not microframes. We are using microframes everywhere,
> + * so adjust accordingly.
> + */
> + interval += 3; /* 1 frame = 2^3 uframes */
> + }
>
> return interval;
> }
--
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
