Hi, John Youn <John.Youn@xxxxxxxxxxxx> writes: > On 11/6/2015 9:55 AM, Felipe Balbi wrote: >> >> Hi, >> >> "McCauley, Ben" <Ben.McCauley@xxxxxxxxxx> writes: >>> Felipe, >>> >>> -----Original Message----- >>> From: Felipe Balbi [mailto:balbi@xxxxxx] >>> Sent: Friday, November 06, 2015 10:06 AM >>> To: McCauley, Ben <Ben.McCauley@xxxxxxxxxx> >>> 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: >>>>> The max speed was always being set to USB_SUPER_SPEED even when the >>>>> phy was only capable of HIGH_SPEED. This >>>> >>>> this statement is untrue >>>> >>>>> uses the value set for the phy to set the max for the gadget. It >>>>> resolves an issue with Window PCs where they report that using a >>>>> USB3.0 port would result in better performance even when connecting >>>>> through a 3.0 port. >>>> >>>> man, who gave you this kernel ? Which kernel version are you using ? >>>> Have you even tested mainline ? >>> >>> This Kernel is from http://omapzoom.org/?p=kernel/omap.git;a=shortlog;h=refs/heads/p-ti-linux-3.14.y-common >>> at branch "development/omapzoom/glsdk-7.-2.00.02". >>> I have not tested it on main line. >> >> seems like you should be talking to your FAE or using TI's e2e forum for >> this. >> >>>>> Signed-off-by: McCauley, Ben <ben.mccauley@xxxxxxxxxx> >>>>> --- >>>>> >>>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c >>>>> index 1eaf31c..0a388e3 100644 >>>>> --- a/drivers/usb/dwc3/gadget.c >>>>> +++ b/drivers/usb/dwc3/gadget.c >>>>> @@ -2899,7 +2899,7 @@ >>>> >>>> which kernel are you using ? This file is 2868 lines, so you're patching outside >>>> the file. >>>> >>>>> } >>>>> >>>>> dwc->gadget.ops = &dwc3_gadget_ops; >>>>> - dwc->gadget.max_speed = USB_SPEED_SUPER; >>>>> + dwc->gadget.max_speed = dwc->maximum_speed; >>>> >>>> this is corrected at dwc3_gadget_start(): >> >> and I see that in that kernel, this is corrected at >> dwc3_gadget_restart() instead. >> >>>> /** >>>> * WORKAROUND: DWC3 revision < 2.20a have an issue >>>> * which would cause metastability state on Run/Stop >>>> * bit if we try to force the IP to USB2-only mode. >>>> * >>>> * Because of that, we cannot configure the IP to any >>>> * speed other than the SuperSpeed >>>> * >>>> * Refers to: >>>> * >>>> * STAR#9000525659: Clock Domain Crossing on DCTL in >>>> * USB 2.0 Mode >>>> */ >>>> if (dwc->revision < DWC3_REVISION_220A) { >>>> reg |= DWC3_DCFG_SUPERSPEED; >>>> } else { >>>> switch (dwc->maximum_speed) { >>>> case USB_SPEED_LOW: >>>> reg |= DWC3_DSTS_LOWSPEED; >>>> break; >>>> case USB_SPEED_FULL: >>>> reg |= DWC3_DSTS_FULLSPEED1; >>>> break; >>>> case USB_SPEED_HIGH: >>>> reg |= DWC3_DSTS_HIGHSPEED; >>>> break; >>>> case USB_SPEED_SUPER: /* FALLTHROUGH */ >>>> case USB_SPEED_UNKNOWN: /* FALTHROUGH */ >>>> default: >>>> reg |= DWC3_DSTS_SUPERSPEED; >>>> } >>>> } >>>> >>> >>> I am on 3.14 LTS though even on 4.3 I do not see dwc->gadget.max_speed >>> being updated to reflect the actual max speed as set in >>> drivers/usb/dwc3/core.c [dwc3_probe()]. dwc->gadget.max_speed is used >>> in a number of locations to determine what response is sent to the >>> host. >>> >>> dwc3_gadget_start() only appears to set the max speed to the register >>> and not update the gadget. >> >> but the IP _is_ super-speed capable. Max speed should only be used to >> determine if $this gadget can run with $this controller; speed, instead, >> is used for all sorts of different things and _that_ is updated on >> Connection Done IRQ. >> >> Can you point one place where you think it's wrong ? And, please, refer >> to mainline code. We can't support a v3.14 kernel which might contain a >> ton of additions which are not in mainline (for example, mainline >> doesn't have a dwc3_gadget_restart() function). >> > > > Hi Felipe, > > There are 3 related speed settings which may be confusing the > matter. > > dwc->maximum_speed > > A user-supplied parameter that determines how to program the > DCFG.speed. This is the code you're referring to above. > > gadget->speed > > The current connected speed of the gadget. Used in various ways > by the upper layer. > > gadget->max_speed > > Reports the maximum speed supported by the gadget. > > This is used by upper layer to determine which BOS descriptors to > send and the settings within those descriptors. It's also used to > set the bcdUSB appropriately. no, bcdUSB will be set to 0x0210 if max_speed == SUPER & speed == HIGH: case USB_DT_DEVICE: cdev->desc.bNumConfigurations = count_configs(cdev, USB_DT_DEVICE); cdev->desc.bMaxPacketSize0 = cdev->gadget->ep0->maxpacket; if (gadget_is_superspeed(gadget)) { if (gadget->speed >= USB_SPEED_SUPER) { cdev->desc.bcdUSB = cpu_to_le16(0x0300); cdev->desc.bMaxPacketSize0 = 9; } else { cdev->desc.bcdUSB = cpu_to_le16(0x0210); } } else { cdev->desc.bcdUSB = cpu_to_le16(0x0200); } > 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 ? 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 ? -- balbi
Attachment:
signature.asc
Description: PGP signature
