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 -- 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
