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.
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>
---
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;
}
--
1.7.4.4
--
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
