From patchwork Mon Nov 9 14:19:36 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Felipe Balbi X-Patchwork-Id: 56222 Delivered-To: patch@linaro.org Received: by 10.112.155.196 with SMTP id vy4csp206405lbb; Mon, 9 Nov 2015 06:19:51 -0800 (PST) X-Received: by 10.66.218.170 with SMTP id ph10mr12347458pac.58.1447078791598; Mon, 09 Nov 2015 06:19:51 -0800 (PST) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id yj10si22639713pab.237.2015.11.09.06.19.51; Mon, 09 Nov 2015 06:19:51 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-usb-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-usb-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-usb-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751855AbbKIOTt (ORCPT + 4 others); Mon, 9 Nov 2015 09:19:49 -0500 Received: from arroyo.ext.ti.com ([192.94.94.40]:40821 "EHLO arroyo.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751620AbbKIOTs (ORCPT ); Mon, 9 Nov 2015 09:19:48 -0500 Received: from dlelxv90.itg.ti.com ([172.17.2.17]) by arroyo.ext.ti.com (8.13.7/8.13.7) with ESMTP id tA9EJfC4008512; Mon, 9 Nov 2015 08:19:41 -0600 Received: from DLEE71.ent.ti.com (dlee71.ent.ti.com [157.170.170.114]) by dlelxv90.itg.ti.com (8.14.3/8.13.8) with ESMTP id tA9EJedo027741; Mon, 9 Nov 2015 08:19:40 -0600 Received: from dflp33.itg.ti.com (10.64.6.16) by DLEE71.ent.ti.com (157.170.170.114) with Microsoft SMTP Server id 14.3.224.2; Mon, 9 Nov 2015 08:19:40 -0600 Received: from localhost (ileax41-snat.itg.ti.com [10.172.224.153]) by dflp33.itg.ti.com (8.14.3/8.13.8) with ESMTP id tA9EJdSe015301; Mon, 9 Nov 2015 08:19:40 -0600 From: Felipe Balbi To: John Youn , "McCauley, Ben" , "gregkh@linuxfoundation.org" CC: "linux-usb@vger.kernel.org" , "Schroeder, Jay" Subject: Re: [PATCH] USB: DCW3: GADGET: Set Correct Max Speed In-Reply-To: <2B3535C5ECE8B5419E3ECBE30077290901DC3BF2F3@US01WEMBX2.internal.synopsys.com> References: <163A5C2B44493946932B998BBAD7C38F54B7E14A@OLAWPA-EXMB05.ad.garmin.com> <87k2pvyuww.fsf@saruman.tx.rr.com> <163A5C2B44493946932B998BBAD7C38F54B7EBFF@OLAWPA-EXMB05.ad.garmin.com> <87lhabxbck.fsf@saruman.tx.rr.com> <2B3535C5ECE8B5419E3ECBE30077290901DC3BF2F3@US01WEMBX2.internal.synopsys.com> User-Agent: Notmuch/0.21 (http://notmuchmail.org) Emacs/24.5.1 (x86_64-pc-linux-gnu) Date: Mon, 9 Nov 2015 08:19:36 -0600 Message-ID: <871tbzuufb.fsf@saruman.tx.rr.com> MIME-Version: 1.0 Sender: linux-usb-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-usb@vger.kernel.org Hi, John Youn writes: > On 11/6/2015 9:55 AM, Felipe Balbi wrote: >> >> Hi, >> >> "McCauley, Ben" writes: >>> Felipe, >>> >>> -----Original Message----- >>> From: Felipe Balbi [mailto:balbi@ti.com] >>> Sent: Friday, November 06, 2015 10:06 AM >>> To: McCauley, Ben >>> Cc: linux-usb@vger.kernel.org; Schroeder, Jay >>> Subject: Re: [PATCH] USB: DCW3: GADGET: Set Correct Max Speed >>>> >>>> >>>> Hi, >>>> >>>> "McCauley, Ben" 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 >>>>> --- >>>>> >>>>> 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: Can you see if this solves the issue you're having ? -- balbi 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); }