On 11/9/2015 1:09 PM, Felipe Balbi wrote:
>
> 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.
>
>> 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).
>
>> 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.
>
> 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.
>
> regards
>
I think we need a way to set gadget->max_speed in any
case. Especially with 3.1 where it might be HS, SS, or SSP. I
think setting via the maximum_speed param seems the best
option. Perhaps as an override in case the GHWPARAMS3 is wrong.
If the PHY is restricted to HS-only then it shouldn't matter that
DCFG.speed is programmed to SUPERSPEED. So the workaround for
controllers < 2.20a can remain as-is.
The LPM in high speed is a separate issue that exists even
now. We address this issue by overriding bcdUSB to 0x0201 for our
HS product, as specified in USB LPM ECN section 3.
I think a possible solution to this is to use the
gadget->get_config_params(). We can add a field in the
usb_dcd_config_params struct to say whether the gadget is LPM
capable.
Then if gadget->max_speed == HIGH_SPEED, and the device is LPM
capable, we can return bcdUSB = 0x201.
Any thoughts on that?
Regards,
John
--
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
