Paul, Thank you for catching the bug. Sarah, Thank you for the detailed description and explanation on the changes. I will send the patch shortly with the updated changes and test. Alexandra. > On Sat, Aug 10, 2013 at 04:04:02AM +0000, Paul Zimmerman wrote: >> > From: linux-usb-owner@xxxxxxxxxxxxxxx >> [mailto:linux-usb-owner@xxxxxxxxxxxxxxx] On Behalf Of Alexandra Yates >> > Sent: Friday, August 09, 2013 12:21 PM >> >> Hi Alexandra, >> >> <snip> >> >> > >> > +/* USB 2.0 BOS descriptor and a capability descriptor, combined */ >> > +static u8 usb2_bos_descriptor[] = { >> > + USB_DT_BOS_SIZE, /* __u8 bLength, 5 bytes */ >> > + USB_DT_BOS, /* __u8 bDescriptorType */ >> > + 0x0c, 0x00, /* __le16 wTotalLength, 15 bytes */ >> > + 0x1, /* __u8 bNumDeviceCaps */ >> > + /* First device capability */ >> > + USB_DT_USB_EXT_CAP_SIZE, /* 7 bits USB2 ext */ >> > + USB_DT_DEVICE_CAPABILITY, /* Device Capability */ >> > + USB_CAP_TYPE_EXT, /* DevCapability Type, USB2.0 ext */ >> > + 0x1e, /* bmAttributes first byte: bit1:LPM >> > + Supported, bit2: BESL supported, >> > + bit3:valid baseline BESL, bit4: >> > + valid Deep BESL, bits5-7 */ >> >> Here you hard-code the first byte of bmAttributes, so LPM Supported, >> BESL >> Supported etc. are already set... > > Yes, Paul is right, you need to set this value to 0x00 in the initial > structure. > >> > + 0xff, /* bmAttributes - second byte: >> > + 8-11bit:BESL and 12-16bit:DBESL*/ > > You also need to set this to 0x00. > >> > + 0x00, /* bmAttribute - third byte: >> reserved, >> > + must be zero */ >> > + 0x00, /* bmAttribute - fourth byte: >> reserved, >> > + must be zero */ >> > +}; >> > + >> > >> > static void xhci_common_hub_descriptor(struct xhci_hcd *xhci, >> > struct usb_hub_descriptor *desc, int ports) >> > @@ -577,12 +599,33 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 >> typeReq, u16 wValue, >> > if ((wValue & 0xff00) != (USB_DT_BOS << 8)) >> > goto error; >> > >> > - if (hcd->speed != HCD_USB3) >> > + if (hcd->speed == HCD_USB3) { >> > + /* Set the U1 and U2 exit latencies. */ >> > + memcpy(buf, &usb3_bos_descriptor, >> > + USB_DT_BOS_SIZE + USB_DT_USB_SS_CAP_SIZE); >> > + } else if (hcd ->speed == HCD_USB2) { >> > + memcpy(buf, &usb2_bos_descriptor, >> > + USB_DT_BOS_SIZE + USB_DT_USB_EXT_CAP_SIZE); >> > + >> > + /* Set first byte of bmAttributes in the >> > + * usb2_bos_descriptor */ >> > + if (xhci->hw_lpm_support) >> > + buf[8] |= USB_LPM_SUPPORT; >> >> ...so these two lines have no effect, the bit is already set. > > Then OR'ing in the USB_LPM_SUPPORT bit will work here. > > Further, we want to tell lsusb that the USB 2.0 roothub has Link PM > support if the host controller supports either software-controlled Link > PM, or hardware-controlled link PM. The USB core only works with > hardware-controlled Link PM, but we could add software controlled link > PM in the future. > > In xhci-mem.c:xhci_add_in_port(), xhci->sw_lpm_support is set if the > host supports USB 2.0 link PM. If the host supports hardware-controlled > Link PM, xhci->hw_lpm_support is additionally set. > > Your code currently only works for hosts that support > hardware-controlled Link PM. You should change this line: > > + if (xhci->hw_lpm_support) > + buf[8] |= USB_LPM_SUPPORT; > > To > > + if (xhci->sw_lpm_support) > + buf[8] |= USB_LPM_SUPPORT; > > >> > + /* Set the BESL support bit in bmAttributes >> first >> > + * byte */ >> > + if (XHCI_BLC) >> > + buf[8] |= USB_BESL_SUPPORT; >> > + if (xhci->dbesl) { >> > + buf[8] = USB_BESL_DEEP_VALID; >> > + buf[9] = xhci->dbesl; >> > + } >> > + if (xhci->dbesld) { >> > + buf[8] = USB_BESL_DEEP_VALID; >> > + buf[9] = xhci->dbesl << 4; >> > + } > > The math here is off, and your initialization of the structure hid those > issues. You should have used USB_BESL_BASELINE_VALID in the second if > block instead of using USB_BESL_DEEP_VALID twice. In the third if > statement block, I think you meant to use xhci->dbesld not xhci->dbesl. > > Finally, you overwrote the contents of buf[8] and buf[9] in the two if > statements, which means USB_LPM_SUPPORT and USB_BESL_SUPPORT would never > be set in buf[8], and if xhci->dbesl and xhci->dbesld were both set, > buf[9] would only contain the deep BESL value. You probably meant to > use |= instead of = for all the assignments in those last two if > blocks. > >> > diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h >> > index c338741..90edeed 100644 >> > --- a/drivers/usb/host/xhci.h >> > +++ b/drivers/usb/host/xhci.h >> > @@ -1564,6 +1564,9 @@ struct xhci_hcd { >> > /* Compliance Mode Recovery Data */ >> > struct timer_list comp_mode_recovery_timer; >> > u32 port_status_u0; >> > + /* LPM DBESL and BESL USB 2.0 Errata support */ >> > + u32 dbesl; >> > + u32 dbesld; >> > /* Compliance Mode Timer Triggered every 2 seconds */ >> > #define COMP_MODE_RCVRY_MSECS 2000 >> > }; > > You defined these new values, but you don't set them any where in the > code. I think this was one of the areas where you were stuck, when we > discussed this last. Sorry for not getting back to you on this sooner. > Here's what I would like you to do. > > First, the xHCI driver ignores any BESL deep values from the device, and > only uses the BESL values. Therefore, we shouldn't report the deep BESL > value in the USB 2.0 BOS descriptor, and we shouldn't set > USB_BESL_DEEP_VALID. We should only report the BESL value if the host > supports BESL for any of the roothub ports. > > So, how do we know if the host supports the BESL encoding for the > roothub ports? To do that, we need to look at the port's extended > capabilities register. The code in xhci_add_in_port() caches those > capabilities in xhci->ext_caps: > > /* cache usb2 port capabilities */ > if (major_revision < 0x03 && xhci->num_ext_caps < max_caps) > xhci->ext_caps[xhci->num_ext_caps++] = temp; > > Code in xhci_update_device() later checks the extended capabilities for > the port to see if it supports the BESL encoding: > > if (xhci_check_usb2_port_capability(xhci, portnum, > XHCI_BLC)) > > That function checks the xhci->ext_caps for that port to see if the bit > (XHCI_BLC) is set: > > /* check if a usb2 port supports a given extened capability protocol > * only USB2 ports extended protocol capability values are cached. > * Return 1 if capability is supported > */ > static int xhci_check_usb2_port_capability(struct xhci_hcd *xhci, int > port, > unsigned capability) > { > u32 port_offset, port_count; > int i; > > for (i = 0; i < xhci->num_ext_caps; i++) { > if (xhci->ext_caps[i] & capability) { > /* port offsets starts at 1 */ > port_offset = XHCI_EXT_PORT_OFF(xhci->ext_caps[i]) > - 1; > port_count = > XHCI_EXT_PORT_COUNT(xhci->ext_caps[i]); > if (port >= port_offset && > port < port_offset + port_count) > return 1; > } > } > return 0; > } > > Basically, if any of the USB 2.0 ports support the BESL encoding, some > array entry in xhci->ext_caps will have XHCI_BLC set. If that bit is > set for any of the USB 2.0 registers, the roothub descriptor should have > USB_BESL_BASELINE_VALID set. > > Now, what do we set the value of the baseline BESL to be in the hub > descriptor? > > The xHCI BESL and DSBESL values are actually in the PCI configuration > registers. However, the xHCI driver doesn't actually use those values > from the PCI registers. Instead, it ignores the deep BESL values, and > uses a default BESL value of XHCI_DEFAULT_BESL. XHCI_DEFAULT_BESL is > defined as 4, which means a BESL value of 400 microseconds (see the > Table X-X1. BESL & HIRD Encodings). > > We only use the XHCI_DEFAULT_BESL if the USB 2.0 port register in the > extended capabilities section has BLC set. > > So here's what you should do: > > 1. Delete xhci->dbesld from your patch. > > 2. Change the name from xhci->dbesl to just xhci->besl. The USB spec > refers to it as baseline BESL, even though the PCI spec refers to it as > default BESL. It's confusing, so let's stick with the USB version, > since this is a USB driver. > > 2. In xhci_add_in_port(), if any port capability has the XHCI_BLC set, > set xhci->besl to XHCI_DEFAULT_BESL, i.e.: > > if (xhci->ext_caps[i] & XHCI_BLC) > xhci->besl = XHCI_DEFAULT_BESL; > > 3. Where XHCI_DEFAULT_BESL is used in xhci_update_device() in xhci.c, > change it to use xhci->besl instead. > > 4. Zero the USB 2.0 BOS descriptors as I mentioned above. > > 5. Change your math in the BOS descriptor to be: > > + /* Set the BESL support bit in bmAttributes */ > + if (xhci->besl) { > + buf[8] |= USB_BESL_SUPPORT; > + buf[8] |= USB_BESL_BASELINE_VALID; > + buf[9] = xhci->besl; > + } > > Does that make sense? If you have any questions, you can get me on IRC. > I may not be in the office tomorrow, but I will definitely be in the > office on Thursday. > > Sarah Sharp > Thank you, Alexandra. -- 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
