Hi, John Youn <[email protected]> writes: > On 11/6/2015 9:55 AM, Felipe Balbi wrote: >> >> Hi, >> >> "McCauley, Ben" <[email protected]> writes: >>> Felipe, >>> >>> -----Original Message----- >>> From: Felipe Balbi [mailto:[email protected]] >>> Sent: Friday, November 06, 2015 10:06 AM >>> To: McCauley, Ben <[email protected]> >>> Cc: [email protected]; Schroeder, Jay <[email protected]> >>> Subject: Re: [PATCH] USB: DCW3: GADGET: Set Correct Max Speed >>>> >>>> >>>> Hi, >>>> >>>> "McCauley, Ben" <[email protected]> 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 <[email protected]> >>>>> --- >>>>> >>>>> 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
signature.asc
Description: PGP signature
