> -----Original Message----- > From: Felipe Balbi [mailto:balbi@xxxxxx] > Sent: Monday, November 09, 2015 3:09 PM > To: McCauley, Ben <Ben.McCauley@xxxxxxxxxx>; John Youn > <John.Youn@xxxxxxxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx > Cc: linux-usb@xxxxxxxxxxxxxxx; Schroeder, Jay <Jay.Schroeder@xxxxxxxxxx> > Subject: RE: [PATCH] USB: DCW3: GADGET: Set Correct Max Speed > > > Hi, > > "McCauley, Ben" <Ben.McCauley@xxxxxxxxxx> writes: > >> "McCauley, Ben" <Ben.McCauley@xxxxxxxxxx> writes: > >> >> > Ben is talking about setting the gadget->max_speed, which is > >> >> > hard-coded in dwc3 to USB_SPEED_SUPER. > >> >> > > >> >> > For this core, the maximum supported speed may not actually be > >> >> > super speed. Many customers choose to use the 3.0 core in > >> >> > 2.0-only mode with a HS phy. > >> >> > >> >> yeah, I got all that. But we still have the problem of > >> >> metastability for anything <2.20a. For those cores, we just cannot > >> >> mess around with any of these settings (STAR 9000525659, if you > wanna check). > >> >> > >> >> > And with the 3.1 core, it my be integrated in a system that is > >> >> > HS-only, SS (gen1), or SSP (gen2) capable. So the > >> >> > gadget->max_speed needs to be set correctly based on some > >> >> > user-defined or detected value. > >> >> > > >> >> > We have a similar change in our local tree for 3.1, except we > >> >> > set it based on GHWPARAMS3.SSPHY_INTERFACE. But I think it > makes > >> sense > >> >> > to > >> >> set > >> >> > it from dwc->maximum_speed as well, for testing or to override it. > >> >> > >> >> My other fear is about people setting this wrongly and later > >> >> complaining that it's not working. I would very much prefer your > >> >> patch which detects whether SSPHY_INTERFACE is around, however > >> >> that > >> won't always be correct. > >> >> > >> >> I just noticed that in our USB2-only SoC (AM437x), SSPHY_INTERFACE > >> >> is set to > >> >> 1 even though this SoC does _not_ have any USB3 capabilities. > >> >> > >> >> GHWPARAMS3 = 0x10420085 > >> >> > >> >> I actually tested this problem as listed above and connected my > >> >> USB2-only SoC to a windows box (both USB2 and USB3 hosts) and did > >> >> not see any such error messages. Any ideas why you have problems > >> >> an I don't > >> ? > >> > > >> > Using an actual 3.0 device I have been able to generate the > >> > following message "This device can perform faster", when connecting > >> > to one of the 2.0 ports on the same PC. It might have something to > >> > do with the organization of the USB hubs In my computer making > >> > windows think that a 3.0 device was connected to a 2.0 when the > >> > reality is swapped when connecting the gadget. > >> > >> if you're connecting a USB 3.0 device to a USB 2.0 port, windows is > >> doing the right thing. It's telling you "if you want to use the full > >> potential of this device, you should connect it to one of your available USB > 3.0 ports". > >> > >> Neither Windows nor dwc3 are wrong in this case. > >> > >> >> Maybe, all we have to do is avoid adding a SuperSpeed USB > >> >> Capability descriptor if we didn't negotiate for superspeed. All > >> >> the rest can remain the > >> >> same: > >> >> > >> >> diff --git a/drivers/usb/gadget/composite.c > >> >> b/drivers/usb/gadget/composite.c index 8b14c2a13ac5..672ae6bfcad8 > >> >> 100644 > >> >> --- a/drivers/usb/gadget/composite.c > >> >> +++ b/drivers/usb/gadget/composite.c > >> >> @@ -547,9 +547,8 @@ static int count_configs(struct > >> >> usb_composite_dev *cdev, unsigned type) static int > >> >> bos_desc(struct usb_composite_dev > >> >> *cdev) { > >> >> struct usb_ext_cap_descriptor *usb_ext; > >> >> - struct usb_ss_cap_descriptor *ss_cap; > >> >> - struct usb_dcd_config_params dcd_config_params; > >> >> struct usb_bos_descriptor *bos = cdev->req->buf; > >> >> + struct usb_gadget *gadget = cdev->gadget; > >> >> > >> >> bos->bLength = USB_DT_BOS_SIZE; > >> >> bos->bDescriptorType = USB_DT_BOS; @@ -569,33 +568,38 @@ > >> >> static int bos_desc(struct usb_composite_dev > >> >> *cdev) > >> >> usb_ext->bDevCapabilityType = USB_CAP_TYPE_EXT; > >> >> usb_ext->bmAttributes = cpu_to_le32(USB_LPM_SUPPORT | > >> >> USB_BESL_SUPPORT); > >> >> > >> >> - /* > >> >> - * The Superspeed USB Capability descriptor shall be implemented > by > >> >> all > >> >> - * SuperSpeed devices. > >> >> - */ > >> >> - ss_cap = cdev->req->buf + le16_to_cpu(bos->wTotalLength); > >> >> - bos->bNumDeviceCaps++; > >> >> - le16_add_cpu(&bos->wTotalLength, USB_DT_USB_SS_CAP_SIZE); > >> >> - ss_cap->bLength = USB_DT_USB_SS_CAP_SIZE; > >> >> - ss_cap->bDescriptorType = USB_DT_DEVICE_CAPABILITY; > >> >> - ss_cap->bDevCapabilityType = USB_SS_CAP_TYPE; > >> >> - ss_cap->bmAttributes = 0; /* LTM is not supported yet */ > >> >> - ss_cap->wSpeedSupported = > >> >> cpu_to_le16(USB_LOW_SPEED_OPERATION | > >> >> + if (gadget->speed >= USB_SPEED_SUPER) { > >> >> + struct usb_ss_cap_descriptor *ss_cap; > >> >> + struct usb_dcd_config_params dcd_config_params; > >> >> + > >> >> + /* > >> >> + * The Superspeed USB Capability descriptor shall be > >> >> implemented > >> >> + * by all SuperSpeed devices. > >> >> + */ > >> >> + ss_cap = cdev->req->buf + le16_to_cpu(bos->wTotalLength); > >> >> + bos->bNumDeviceCaps++; > >> >> + le16_add_cpu(&bos->wTotalLength, > >> >> USB_DT_USB_SS_CAP_SIZE); > >> >> + ss_cap->bLength = USB_DT_USB_SS_CAP_SIZE; > >> >> + ss_cap->bDescriptorType = USB_DT_DEVICE_CAPABILITY; > >> >> + ss_cap->bDevCapabilityType = USB_SS_CAP_TYPE; > >> >> + ss_cap->bmAttributes = 0; /* LTM is not supported yet */ > >> >> + ss_cap->wSpeedSupported = > >> >> cpu_to_le16(USB_LOW_SPEED_OPERATION | > >> >> USB_FULL_SPEED_OPERATION | > >> >> USB_HIGH_SPEED_OPERATION | > >> >> USB_5GBPS_OPERATION); > >> >> - ss_cap->bFunctionalitySupport = USB_LOW_SPEED_OPERATION; > >> >> + ss_cap->bFunctionalitySupport = > >> >> USB_LOW_SPEED_OPERATION; > >> >> > >> >> - /* Get Controller configuration */ > >> >> - if (cdev->gadget->ops->get_config_params) > >> >> - cdev->gadget->ops- > >> >> >get_config_params(&dcd_config_params); > >> >> - else { > >> >> + /* Get Controller configuration */ > >> >> dcd_config_params.bU1devExitLat = > >> >> USB_DEFAULT_U1_DEV_EXIT_LAT; > >> >> dcd_config_params.bU2DevExitLat = > >> >> cpu_to_le16(USB_DEFAULT_U2_DEV_EXIT_LAT); > >> >> + > >> >> + if (cdev->gadget->ops->get_config_params) > >> >> + cdev->gadget->ops- > >> >> >get_config_params(&dcd_config_params); > >> >> + > >> >> + ss_cap->bU1devExitLat = dcd_config_params.bU1devExitLat; > >> >> + ss_cap->bU2DevExitLat = > >> >> + dcd_config_params.bU2DevExitLat; > >> >> } > >> >> - ss_cap->bU1devExitLat = dcd_config_params.bU1devExitLat; > >> >> - ss_cap->bU2DevExitLat = dcd_config_params.bU2DevExitLat; > >> >> > >> >> return le16_to_cpu(bos->wTotalLength); } > >> >> > >> >> Can you see if this solves the issue you're having ? > >> >> > >> > > >> > I can confirm that this does resolve Windows reporting a that a 3.0 > >> > device was connected to a non 3.0 port. > >> > > >> > I am concerned that though this fixes the issue, it could result in > >> > 3.0 devices not reporting themselves as a 3.0 device when connected > >> > to a 2.0 port. > >> > >> right, I was thinking about the same thing after I sent this. But can > >> you really describe the problem you're having ? Is your device > >> SuperSpeed capable or not ? Are you connecting it to a Highspeed port or > Superspeed port ? > >> > > > > The device is not SuperSpeed capable and the issue comes up when > > connecting the device to a > > a couple emails back you stated: > > 'Using an actual 3.0 device I have been able to generate the following > message "This device can perform faster", when connecting to one of the 2.0 > ports on the same PC. It might have something to do with the organization of > the USB hubs In my computer making windows think that a 3.0 device was > connected to a 2.0 when the reality is swapped when connecting the gadget.' > > and now you say again that the device is _not_ Superspeed capable. Please > make up your mind otherwise I really cannot help you. Sorry for the confusion I meant to indicate that using an off the shelf 3.0 device I was able to generate the message on Windows as would be expected. The device I am developing does not support SuperSpeed. > > > SuperSpeed port. The crux of the issue is that I have a coworker > > laptop which when connected to the device the device receives a U2 > > command multiple times and after cycling through suspend and resume a > > couple times finalizes in the suspend state. So that reality is that > > there are two issues, > > what runs on this desktop ? Linux ? Windows ? Which version ? (Linux 4.0 ? > Windows 7 ?). What's the model of that laptop (although this is less > important). The laptop is a Dell Latitude E7440 running windows 7. > > > 1. PC reports 3.0 when the device does not support 3.0. This results > > in a confusing user experience and worth resolving. > > The real thing is that we still want to report our BOS descriptor, just skip > Superspeed capability descriptor if this instance of dwc3 really can't do > superspeed due to missing PIPE3 PHY or whatever. However, we can't use > negotiated speed for that because we still want that warning for the case > where the device _is_ superspeed capable (i.e. dwc3 with superspeed PHY) > but is connected to USB 2.0 port. > > > 2. Device initialized in suspend resulting in the g_hid module not > > functioning. The first patch fixed this as a side effect. This is the > > actual issue I am working to fix. This does not happen on all SS > > capable computers. I have only found the one model so far. > > Wich SoC are you using ? Based on the fact that you pointed me to > omapzoom.org, I'll assume you're using something from the DRA7x family, > am I right ? I'll think of a solution for the mainline kernel, but I'll ask again that > you get in touch with your FAE for support with your v3.14 kernel. Yes I am working with the DRA7x. the direction we have been given is to take Kernel fixes to the community so that they can be picked up off the 3.14 LTS. > > It seems like we need a new USB_SPEED_HIGH_LPM added so we have > means to tell composite.c that we don't support Superspeed but support > highspeed _with_ LPM. > > Meanwhile, I guess the quickest "solution" _is_ your patch, but I need a very > large FIXME comment explaining that we would be better off with a new > USB_SPEED_HIGH_LPM because of the whole bcdUSB 0x0210 thing. Also, > when adding the comment, I need a dev_info() message to show up on all > cores <2.20a which makes it clear that we're changing max_speed, but we > can't change DCFG programming because that would cause the metastability > error. The more I understand the issue might not be an bug but an incorrect configuration of LPM. Without knowing how LPM was implemented on the different Windows PC I have tested with It is hard to determine what the correct behavior is though I suspect that LPM is not valid on HID devices. It is possible that the issue is that the device is reporting that LPM is supported which while true, it is not desirable for a HID device. It might be better to have an option to disable reporting LPM for gadgets which generate events like an HID device. It would make sense to allow differentiating between a 2.0, 2.1, and a 3.0 device and if the intermediate solution is a FIXME it would probably be better for me to make a change locally and wait for the final solution to come down the pipeline. Especially if no one else is having an issue with this. > > regards > > -- > balbi ________________________________ CONFIDENTIALITY NOTICE: This email and any attachments are for the sole use of the intended recipient(s) and contain information that may be confidential and/or legally privileged. If you have received this email in error, please notify the sender by reply email and delete the message. Any disclosure, copying, distribution or use of this communication (including attachments) by someone other than the intended recipient is prohibited. Thank you. -- 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
