USB: DCW3: GADGET: Set Correct Max Speed

Message ID 871tbzuufb.fsf@saruman.tx.rr.com
State New
Headers show

Commit Message

Felipe Balbi Nov. 9, 2015, 2:19 p.m.
Hi,

John Youn <John.Youn@synopsys.com> writes:
> On 11/6/2015 9:55 AM, Felipe Balbi wrote:

>> 

>> Hi,

>> 

>> "McCauley, Ben" <Ben.McCauley@garmin.com> writes:

>>> Felipe,

>>>

>>>  -----Original Message-----

>>>  From: Felipe Balbi [mailto:balbi@ti.com]

>>>  Sent: Friday, November 06, 2015 10:06 AM

>>>  To: McCauley, Ben <Ben.McCauley@garmin.com>

>>>  Cc: linux-usb@vger.kernel.org; Schroeder, Jay <Jay.Schroeder@garmin.com>

>>> Subject: Re: [PATCH] USB: DCW3: GADGET: Set Correct Max Speed

>>>>

>>>>

>>>> Hi,

>>>>

>>>> "McCauley, Ben" <Ben.McCauley@garmin.com> 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@garmin.com>

>>>>> ---

>>>>>

>>>>> 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

Comments

Felipe Balbi Nov. 9, 2015, 6:33 p.m. | #1
Hi,

"McCauley, Ben" <Ben.McCauley@garmin.com> 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 ?

-- 
balbi
Felipe Balbi Nov. 9, 2015, 9:09 p.m. | #2
Hi,

"McCauley, Ben" <Ben.McCauley@garmin.com> writes:
>> "McCauley, Ben" <Ben.McCauley@garmin.com> 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

-- 
balbi
Felipe Balbi Nov. 10, 2015, 3:10 p.m. | #3
Hi,

John Youn <John.Youn@synopsys.com> writes:
> On 11/9/2015 1:09 PM, Felipe Balbi wrote:

>> 

>> Hi,

>> 

>> "McCauley, Ben" <Ben.McCauley@garmin.com> writes:

>>>> "McCauley, Ben" <Ben.McCauley@garmin.com> 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.


right, this is what I tried to say :-) We just need the comment
explaining the situation and adding a FIXME note for the 2.1 thing.

> 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.


that's correct.

> 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?


interesting, let me think about that.

Ben, can you respin your patch to also add the comment explaining the
situation so we don't loose track of it ? Then I can apply once -rc1 is
out.

thanks

-- 
balbi
Felipe Balbi Nov. 16, 2015, 4:39 p.m. | #4
Hi,

"McCauley, Ben" <Ben.McCauley@garmin.com> writes:

<snip>

>> Ben, can you respin your patch to also add the comment explaining the

>> situation so we don't loose track of it ? Then I can apply once -rc1 is out.

>>

>> thanks

>>

>> --

>> Balbi

>

> The max speed was always being set to USB_SUPER_SPEED

> even when the phy was only capable of HIGH_SPEED. This

> 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. A

> follow on patch will be needed to add USB_SPEED_HIGH_LPM

> to support High Speed devices which also support LPM.

>

> Signed-off-by: McCauley, Ben <ben.mccauley@garmin.com>


if I were to apply this patch, all of those hundreds of lines of context
in the email would go into the git log. This is not how you send a patch.

> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c

> index 1eaf31c..342be2f 100644

> --- a/drivers/usb/dwc3/gadget.c

> +++ b/drivers/usb/dwc3/gadget.c

> @@ -2899,7 +2899,15 @@

>         }

>

>         dwc->gadget.ops                 = &dwc3_gadget_ops;

> -       dwc->gadget.max_speed           = USB_SPEED_SUPER;

> +       /*

> +        * FIXME A new enumeration USB_SPEED_HIGH_LPM needs to be added

> +        * to support USB 2.10. Setting max_speed to dwc->maximum_speed

> +        * disables LPM support for devices which do not support

> +        * SuperSpeed but are LPM capable.

> +        */

> +       if (dwc->maximum_speed < USB_SPEED_SUPER)

> +               dev_info(dwc->dev, "SuperSpeed not supported.\n");

> +       dwc->gadget.max_speed           = dwc->maximum_speed;

>         dwc->gadget.speed               = USB_SPEED_UNKNOWN;

>         dwc->gadget.sg_supported        = true;

>         dwc->gadget.name                = "dwc3-gadget"

>

> ________________________________

>

> 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.


this note, as I have said before, should not be in your email anymore,
much less in git's history. I'll all of these while applying, but it's
the only time; next time, sorry, I'll have to ignore your patch until
you manage to get it done correct.

regards

-- 
balbi

Patch

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);
 }