[PATCH 00/11] usb: Handle different sublink speeds

[PATCH 00/11] usb: Handle different sublink speeds

[Thinh Nguyen]

A USB super-speed-plus device may operate at different sublink speed and lane
count (e.g. gen2x2, gen1x2, or gen2x1). The usb gadget stack needs to be able
to handle a couple things:

1) Report the sublink speed attributes the device support
2) Select the sublink speed attribute

This series introduces sublink speed attribute structure to ch9.h to capture
the device capability of the gadget. It also introduces a new gadget ops
udc_set_num_lanes_and_speed to select a specific sublink speed.

DWC3 needs this support for DWC_usb32 IP. Implement the new changes for DWC3.


Thinh Nguyen (11):
  usb: ch9: Add sublink speed struct
  usb: gadget: composite: Avoid using magic numbers
  usb: gadget: Expose sublink speed attributes
  usb: gadget: Set max speed for SSP devices
  usb: composite: Properly report sublink speed
  usb: devicetree: dwc3: Introduce num-lanes and lsm
  usb: dwc3: Initialize lane count and sublink speed
  usb: dwc3: gadget: Report sublink speed capability
  usb: dwc3: gadget: Implement setting of sublink speed
  usb: dwc3: gadget: Track connected lane and sublink speed
  usb: dwc3: gadget: Set speed only up to the max supported

 Documentation/devicetree/bindings/usb/dwc3.txt |   9 ++
 drivers/usb/dwc3/core.c                        |  64 ++++++++++++
 drivers/usb/dwc3/core.h                        |  18 ++++
 drivers/usb/dwc3/gadget.c                      | 135 ++++++++++++++++++++++++-
 drivers/usb/gadget/composite.c                 |  81 ++++++++++-----
 drivers/usb/gadget/udc/core.c                  |  24 ++++-
 include/linux/usb/gadget.h                     |  23 +++++
 include/uapi/linux/usb/ch9.h                   |  42 ++++++++
 8 files changed, 360 insertions(+), 36 deletions(-)

-- 
2.11.0

[PATCH 06/11] usb: devicetree: dwc3: Introduce num-lanes and lsm

[Thinh Nguyen]

Introduce num-lanes and lane-speed-mantissa-gbps for devices operating
in super-speed-plus. DWC_usb32 IP supports multiple lanes and can
operate in different sublink speeds. Currently the device controller
does not have the information of the phy's number of lanes supported. As
a result, the user can specify them through these properties if they are
different than the default setting.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 Documentation/devicetree/bindings/usb/dwc3.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index d03edf9d3935..4eba0615562f 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -86,6 +86,15 @@ Optional properties:
  - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
 	register for post-silicon frame length adjustment when the
 	fladj_30mhz_sdbnd signal is invalid or incorrect.
+ - snps,num-lanes: set to specify the number of lanes to use. Valid inputs are
+			1 or 2. Apply if the maximum-speed is super-speed-plus
+			only. Default value is 2 for DWC_usb32. For DWC_usb31,
+			it is always 1 at super-speed-plus.
+ - snps,lane-speed-mantissa-gbps: set to specify the symmetric lane speed
+			mantissa in Gbps. Valid inputs are 5 or 10. Apply if
+			the maximum-speed is super-speed-plus only. Default
+			value is 10. For DWC_usb31, it's always 10 at
+			super-speed-plus.
  - snps,rx-thr-num-pkt-prd: periodic ESS RX packet threshold count - host mode
 			only. Set this and rx-max-burst-prd to a valid,
 			non-zero value 1-16 (DWC_usb31 programming guide
-- 
2.11.0

Re: [PATCH 06/11] usb: devicetree: dwc3: Introduce num-lanes and lsm

[Rob Herring]

On Thu, Jul 16, 2020 at 02:59:08PM -0700, Thinh Nguyen wrote:
> Introduce num-lanes and lane-speed-mantissa-gbps for devices operating
> in super-speed-plus. DWC_usb32 IP supports multiple lanes and can
> operate in different sublink speeds. Currently the device controller
> does not have the information of the phy's number of lanes supported. As
> a result, the user can specify them through these properties if they are
> different than the default setting.
> 
> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> ---
>  Documentation/devicetree/bindings/usb/dwc3.txt | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index d03edf9d3935..4eba0615562f 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -86,6 +86,15 @@ Optional properties:
>   - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>  	register for post-silicon frame length adjustment when the
>  	fladj_30mhz_sdbnd signal is invalid or incorrect.
> + - snps,num-lanes: set to specify the number of lanes to use. Valid inputs are
> +			1 or 2. Apply if the maximum-speed is super-speed-plus
> +			only. Default value is 2 for DWC_usb32. For DWC_usb31,
> +			it is always 1 at super-speed-plus.
> + - snps,lane-speed-mantissa-gbps: set to specify the symmetric lane speed
> +			mantissa in Gbps. Valid inputs are 5 or 10. Apply if
> +			the maximum-speed is super-speed-plus only. Default
> +			value is 10. For DWC_usb31, it's always 10 at
> +			super-speed-plus.

This is all common USB things and should be common properties (which we 
may already have).

Rob

Re: [PATCH 06/11] usb: devicetree: dwc3: Introduce num-lanes and lsm

[Thinh Nguyen]

Rob Herring wrote:
> On Thu, Jul 16, 2020 at 02:59:08PM -0700, Thinh Nguyen wrote:
>> Introduce num-lanes and lane-speed-mantissa-gbps for devices operating
>> in super-speed-plus. DWC_usb32 IP supports multiple lanes and can
>> operate in different sublink speeds. Currently the device controller
>> does not have the information of the phy's number of lanes supported. As
>> a result, the user can specify them through these properties if they are
>> different than the default setting.
>>
>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>> ---
>>   Documentation/devicetree/bindings/usb/dwc3.txt | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>> index d03edf9d3935..4eba0615562f 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>> @@ -86,6 +86,15 @@ Optional properties:
>>    - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>>   	register for post-silicon frame length adjustment when the
>>   	fladj_30mhz_sdbnd signal is invalid or incorrect.
>> + - snps,num-lanes: set to specify the number of lanes to use. Valid inputs are
>> +			1 or 2. Apply if the maximum-speed is super-speed-plus
>> +			only. Default value is 2 for DWC_usb32. For DWC_usb31,
>> +			it is always 1 at super-speed-plus.
>> + - snps,lane-speed-mantissa-gbps: set to specify the symmetric lane speed
>> +			mantissa in Gbps. Valid inputs are 5 or 10. Apply if
>> +			the maximum-speed is super-speed-plus only. Default
>> +			value is 10. For DWC_usb31, it's always 10 at
>> +			super-speed-plus.
> This is all common USB things and should be common properties (which we
> may already have).

Sure. For "num-lanes" is simple, any objection if we use 
"lane-speed-mantissa-gbps"? Or should we add "lane-speed-exponent"?

Thanks,
Thinh

Re: [PATCH 06/11] usb: devicetree: dwc3: Introduce num-lanes and lsm

[Rob Herring]

On Mon, Jul 20, 2020 at 11:01 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>
> Rob Herring wrote:
> > On Thu, Jul 16, 2020 at 02:59:08PM -0700, Thinh Nguyen wrote:
> >> Introduce num-lanes and lane-speed-mantissa-gbps for devices operating
> >> in super-speed-plus. DWC_usb32 IP supports multiple lanes and can
> >> operate in different sublink speeds. Currently the device controller
> >> does not have the information of the phy's number of lanes supported. As
> >> a result, the user can specify them through these properties if they are
> >> different than the default setting.
> >>
> >> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> >> ---
> >>   Documentation/devicetree/bindings/usb/dwc3.txt | 9 +++++++++
> >>   1 file changed, 9 insertions(+)
> >>
> >> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> >> index d03edf9d3935..4eba0615562f 100644
> >> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> >> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> >> @@ -86,6 +86,15 @@ Optional properties:
> >>    - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
> >>      register for post-silicon frame length adjustment when the
> >>      fladj_30mhz_sdbnd signal is invalid or incorrect.
> >> + - snps,num-lanes: set to specify the number of lanes to use. Valid inputs are
> >> +                    1 or 2. Apply if the maximum-speed is super-speed-plus
> >> +                    only. Default value is 2 for DWC_usb32. For DWC_usb31,
> >> +                    it is always 1 at super-speed-plus.
> >> + - snps,lane-speed-mantissa-gbps: set to specify the symmetric lane speed
> >> +                    mantissa in Gbps. Valid inputs are 5 or 10. Apply if
> >> +                    the maximum-speed is super-speed-plus only. Default
> >> +                    value is 10. For DWC_usb31, it's always 10 at
> >> +                    super-speed-plus.
> > This is all common USB things and should be common properties (which we
> > may already have).
>
> Sure. For "num-lanes" is simple, any objection if we use
> "lane-speed-mantissa-gbps"? Or should we add "lane-speed-exponent"?

'num-lanes' is good as that's what PCIe uses. Document that with
'maximum-speed'.

I think 'super-speed-plus' should mean gen 2 10G per lane. Then
between num-lanes and maximum-speed you can define all 4 possible
rates.

Rob

Re: [PATCH 06/11] usb: devicetree: dwc3: Introduce num-lanes and lsm

[Thinh Nguyen]

Rob Herring wrote:
> On Mon, Jul 20, 2020 at 11:01 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>> Rob Herring wrote:
>>> On Thu, Jul 16, 2020 at 02:59:08PM -0700, Thinh Nguyen wrote:
>>>> Introduce num-lanes and lane-speed-mantissa-gbps for devices operating
>>>> in super-speed-plus. DWC_usb32 IP supports multiple lanes and can
>>>> operate in different sublink speeds. Currently the device controller
>>>> does not have the information of the phy's number of lanes supported. As
>>>> a result, the user can specify them through these properties if they are
>>>> different than the default setting.
>>>>
>>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>>> ---
>>>>    Documentation/devicetree/bindings/usb/dwc3.txt | 9 +++++++++
>>>>    1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> index d03edf9d3935..4eba0615562f 100644
>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> @@ -86,6 +86,15 @@ Optional properties:
>>>>     - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>>>>       register for post-silicon frame length adjustment when the
>>>>       fladj_30mhz_sdbnd signal is invalid or incorrect.
>>>> + - snps,num-lanes: set to specify the number of lanes to use. Valid inputs are
>>>> +                    1 or 2. Apply if the maximum-speed is super-speed-plus
>>>> +                    only. Default value is 2 for DWC_usb32. For DWC_usb31,
>>>> +                    it is always 1 at super-speed-plus.
>>>> + - snps,lane-speed-mantissa-gbps: set to specify the symmetric lane speed
>>>> +                    mantissa in Gbps. Valid inputs are 5 or 10. Apply if
>>>> +                    the maximum-speed is super-speed-plus only. Default
>>>> +                    value is 10. For DWC_usb31, it's always 10 at
>>>> +                    super-speed-plus.
>>> This is all common USB things and should be common properties (which we
>>> may already have).
>> Sure. For "num-lanes" is simple, any objection if we use
>> "lane-speed-mantissa-gbps"? Or should we add "lane-speed-exponent"?
> 'num-lanes' is good as that's what PCIe uses. Document that with
> 'maximum-speed'.
>
> I think 'super-speed-plus' should mean gen 2 10G per lane. Then
> between num-lanes and maximum-speed you can define all 4 possible
> rates.

That may confuse the user because now we'd use 'super-speed-plus' to 
define the speed of the lane rather than the device itself.

According to the USB 3.2 spec, super-speed-plus can mean gen2x1, gen1x2, 
or gen2x2.

BR,
Thinh

Re: [PATCH 06/11] usb: devicetree: dwc3: Introduce num-lanes and lsm

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2839 bytes --]


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> Rob Herring wrote:
>> On Mon, Jul 20, 2020 at 11:01 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>>> Rob Herring wrote:
>>>> On Thu, Jul 16, 2020 at 02:59:08PM -0700, Thinh Nguyen wrote:
>>>>> Introduce num-lanes and lane-speed-mantissa-gbps for devices operating
>>>>> in super-speed-plus. DWC_usb32 IP supports multiple lanes and can
>>>>> operate in different sublink speeds. Currently the device controller
>>>>> does not have the information of the phy's number of lanes supported. As
>>>>> a result, the user can specify them through these properties if they are
>>>>> different than the default setting.
>>>>>
>>>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>>>> ---
>>>>>    Documentation/devicetree/bindings/usb/dwc3.txt | 9 +++++++++
>>>>>    1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> index d03edf9d3935..4eba0615562f 100644
>>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>> @@ -86,6 +86,15 @@ Optional properties:
>>>>>     - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>>>>>       register for post-silicon frame length adjustment when the
>>>>>       fladj_30mhz_sdbnd signal is invalid or incorrect.
>>>>> + - snps,num-lanes: set to specify the number of lanes to use. Valid inputs are
>>>>> +                    1 or 2. Apply if the maximum-speed is super-speed-plus
>>>>> +                    only. Default value is 2 for DWC_usb32. For DWC_usb31,
>>>>> +                    it is always 1 at super-speed-plus.
>>>>> + - snps,lane-speed-mantissa-gbps: set to specify the symmetric lane speed
>>>>> +                    mantissa in Gbps. Valid inputs are 5 or 10. Apply if
>>>>> +                    the maximum-speed is super-speed-plus only. Default
>>>>> +                    value is 10. For DWC_usb31, it's always 10 at
>>>>> +                    super-speed-plus.
>>>> This is all common USB things and should be common properties (which we
>>>> may already have).
>>> Sure. For "num-lanes" is simple, any objection if we use
>>> "lane-speed-mantissa-gbps"? Or should we add "lane-speed-exponent"?
>> 'num-lanes' is good as that's what PCIe uses. Document that with
>> 'maximum-speed'.
>>
>> I think 'super-speed-plus' should mean gen 2 10G per lane. Then
>> between num-lanes and maximum-speed you can define all 4 possible
>> rates.
>
> That may confuse the user because now we'd use 'super-speed-plus' to 
> define the speed of the lane rather than the device itself.

I agree. In USB land we should refer solely to the USB specification
naming schemes.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH 06/11] usb: devicetree: dwc3: Introduce num-lanes and lsm

[Rob Herring]

On Tue, Jul 21, 2020 at 10:42 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>
> Rob Herring wrote:
> > On Mon, Jul 20, 2020 at 11:01 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
> >> Rob Herring wrote:
> >>> On Thu, Jul 16, 2020 at 02:59:08PM -0700, Thinh Nguyen wrote:
> >>>> Introduce num-lanes and lane-speed-mantissa-gbps for devices operating
> >>>> in super-speed-plus. DWC_usb32 IP supports multiple lanes and can
> >>>> operate in different sublink speeds. Currently the device controller
> >>>> does not have the information of the phy's number of lanes supported. As
> >>>> a result, the user can specify them through these properties if they are
> >>>> different than the default setting.
> >>>>
> >>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> >>>> ---
> >>>>    Documentation/devicetree/bindings/usb/dwc3.txt | 9 +++++++++
> >>>>    1 file changed, 9 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> >>>> index d03edf9d3935..4eba0615562f 100644
> >>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> >>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> >>>> @@ -86,6 +86,15 @@ Optional properties:
> >>>>     - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
> >>>>       register for post-silicon frame length adjustment when the
> >>>>       fladj_30mhz_sdbnd signal is invalid or incorrect.
> >>>> + - snps,num-lanes: set to specify the number of lanes to use. Valid inputs are
> >>>> +                    1 or 2. Apply if the maximum-speed is super-speed-plus
> >>>> +                    only. Default value is 2 for DWC_usb32. For DWC_usb31,
> >>>> +                    it is always 1 at super-speed-plus.
> >>>> + - snps,lane-speed-mantissa-gbps: set to specify the symmetric lane speed
> >>>> +                    mantissa in Gbps. Valid inputs are 5 or 10. Apply if
> >>>> +                    the maximum-speed is super-speed-plus only. Default
> >>>> +                    value is 10. For DWC_usb31, it's always 10 at
> >>>> +                    super-speed-plus.
> >>> This is all common USB things and should be common properties (which we
> >>> may already have).
> >> Sure. For "num-lanes" is simple, any objection if we use
> >> "lane-speed-mantissa-gbps"? Or should we add "lane-speed-exponent"?
> > 'num-lanes' is good as that's what PCIe uses. Document that with
> > 'maximum-speed'.
> >
> > I think 'super-speed-plus' should mean gen 2 10G per lane. Then
> > between num-lanes and maximum-speed you can define all 4 possible
> > rates.
>
> That may confuse the user because now we'd use 'super-speed-plus' to
> define the speed of the lane rather than the device itself.
>
> According to the USB 3.2 spec, super-speed-plus can mean gen2x1, gen1x2,
> or gen2x2.

Then add new strings as needed to make it clear: super-speed-plus-gen1x2

It's obvious that what 'super-speed-plus' means is not clear since
USB-IF extended its meaning.

Rob

Re: [PATCH 06/11] usb: devicetree: dwc3: Introduce num-lanes and lsm

[Thinh Nguyen]

Rob Herring wrote:
> On Tue, Jul 21, 2020 at 10:42 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>> Rob Herring wrote:
>>> On Mon, Jul 20, 2020 at 11:01 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>>>> Rob Herring wrote:
>>>>> On Thu, Jul 16, 2020 at 02:59:08PM -0700, Thinh Nguyen wrote:
>>>>>> Introduce num-lanes and lane-speed-mantissa-gbps for devices operating
>>>>>> in super-speed-plus. DWC_usb32 IP supports multiple lanes and can
>>>>>> operate in different sublink speeds. Currently the device controller
>>>>>> does not have the information of the phy's number of lanes supported. As
>>>>>> a result, the user can specify them through these properties if they are
>>>>>> different than the default setting.
>>>>>>
>>>>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>>>>> ---
>>>>>>     Documentation/devicetree/bindings/usb/dwc3.txt | 9 +++++++++
>>>>>>     1 file changed, 9 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>> index d03edf9d3935..4eba0615562f 100644
>>>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>> @@ -86,6 +86,15 @@ Optional properties:
>>>>>>      - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>>>>>>        register for post-silicon frame length adjustment when the
>>>>>>        fladj_30mhz_sdbnd signal is invalid or incorrect.
>>>>>> + - snps,num-lanes: set to specify the number of lanes to use. Valid inputs are
>>>>>> +                    1 or 2. Apply if the maximum-speed is super-speed-plus
>>>>>> +                    only. Default value is 2 for DWC_usb32. For DWC_usb31,
>>>>>> +                    it is always 1 at super-speed-plus.
>>>>>> + - snps,lane-speed-mantissa-gbps: set to specify the symmetric lane speed
>>>>>> +                    mantissa in Gbps. Valid inputs are 5 or 10. Apply if
>>>>>> +                    the maximum-speed is super-speed-plus only. Default
>>>>>> +                    value is 10. For DWC_usb31, it's always 10 at
>>>>>> +                    super-speed-plus.
>>>>> This is all common USB things and should be common properties (which we
>>>>> may already have).
>>>> Sure. For "num-lanes" is simple, any objection if we use
>>>> "lane-speed-mantissa-gbps"? Or should we add "lane-speed-exponent"?
>>> 'num-lanes' is good as that's what PCIe uses. Document that with
>>> 'maximum-speed'.
>>>
>>> I think 'super-speed-plus' should mean gen 2 10G per lane. Then
>>> between num-lanes and maximum-speed you can define all 4 possible
>>> rates.
>> That may confuse the user because now we'd use 'super-speed-plus' to
>> define the speed of the lane rather than the device itself.
>>
>> According to the USB 3.2 spec, super-speed-plus can mean gen2x1, gen1x2,
>> or gen2x2.
> Then add new strings as needed to make it clear: super-speed-plus-gen1x2
>
> It's obvious that what 'super-speed-plus' means is not clear since
> USB-IF extended its meaning.
>
> Rob

If we introduce a new enum for gen1x2, now we'd have to go back and 
inspect all the checks for all the drivers where for example speed == 
USB_SPEED_SUPER_PLUS. It seems to be more clunky and may introduce more 
bugs.

BR,
Thinh

Re: [PATCH 06/11] usb: devicetree: dwc3: Introduce num-lanes and lsm

[Thinh Nguyen]

Thinh Nguyen wrote:
> Rob Herring wrote:
>> On Tue, Jul 21, 2020 at 10:42 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>>> Rob Herring wrote:
>>>> On Mon, Jul 20, 2020 at 11:01 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>>>>> Rob Herring wrote:
>>>>>> On Thu, Jul 16, 2020 at 02:59:08PM -0700, Thinh Nguyen wrote:
>>>>>>> Introduce num-lanes and lane-speed-mantissa-gbps for devices operating
>>>>>>> in super-speed-plus. DWC_usb32 IP supports multiple lanes and can
>>>>>>> operate in different sublink speeds. Currently the device controller
>>>>>>> does not have the information of the phy's number of lanes supported. As
>>>>>>> a result, the user can specify them through these properties if they are
>>>>>>> different than the default setting.
>>>>>>>
>>>>>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>>>>>> ---
>>>>>>>      Documentation/devicetree/bindings/usb/dwc3.txt | 9 +++++++++
>>>>>>>      1 file changed, 9 insertions(+)
>>>>>>>
>>>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>>> index d03edf9d3935..4eba0615562f 100644
>>>>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>>> @@ -86,6 +86,15 @@ Optional properties:
>>>>>>>       - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>>>>>>>         register for post-silicon frame length adjustment when the
>>>>>>>         fladj_30mhz_sdbnd signal is invalid or incorrect.
>>>>>>> + - snps,num-lanes: set to specify the number of lanes to use. Valid inputs are
>>>>>>> +                    1 or 2. Apply if the maximum-speed is super-speed-plus
>>>>>>> +                    only. Default value is 2 for DWC_usb32. For DWC_usb31,
>>>>>>> +                    it is always 1 at super-speed-plus.
>>>>>>> + - snps,lane-speed-mantissa-gbps: set to specify the symmetric lane speed
>>>>>>> +                    mantissa in Gbps. Valid inputs are 5 or 10. Apply if
>>>>>>> +                    the maximum-speed is super-speed-plus only. Default
>>>>>>> +                    value is 10. For DWC_usb31, it's always 10 at
>>>>>>> +                    super-speed-plus.
>>>>>> This is all common USB things and should be common properties (which we
>>>>>> may already have).
>>>>> Sure. For "num-lanes" is simple, any objection if we use
>>>>> "lane-speed-mantissa-gbps"? Or should we add "lane-speed-exponent"?
>>>> 'num-lanes' is good as that's what PCIe uses. Document that with
>>>> 'maximum-speed'.
>>>>
>>>> I think 'super-speed-plus' should mean gen 2 10G per lane. Then
>>>> between num-lanes and maximum-speed you can define all 4 possible
>>>> rates.
>>> That may confuse the user because now we'd use 'super-speed-plus' to
>>> define the speed of the lane rather than the device itself.
>>>
>>> According to the USB 3.2 spec, super-speed-plus can mean gen2x1, gen1x2,
>>> or gen2x2.
>> Then add new strings as needed to make it clear: super-speed-plus-gen1x2
>>
>> It's obvious that what 'super-speed-plus' means is not clear since
>> USB-IF extended its meaning.
>>
>> Rob
> If we introduce a new enum for gen1x2, now we'd have to go back and
> inspect all the checks for all the drivers where for example speed ==
> USB_SPEED_SUPER_PLUS. It seems to be more clunky and may introduce more
> bugs.
>

In my opinion, the better option would be to introduce a new property 
for lane speed such as "lane-speed-mantissa-gbps" because:

1) It still follows the spec, easier for the user to understand
2) We only need to update the drivers where the number of lanes and lane 
speed matter
3) Easier speed comparison between usb_device_speed enum
4) Easier to backport new code where there's speed comparison
5) Easily extendable to new/different lane speeds

BR,
Thinh

Re: [PATCH 06/11] usb: devicetree: dwc3: Introduce num-lanes and lsm

[Thinh Nguyen]

Thinh Nguyen wrote:
> Thinh Nguyen wrote:
>> Rob Herring wrote:
>>> On Tue, Jul 21, 2020 at 10:42 AM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>>>> Rob Herring wrote:
>>>>> On Mon, Jul 20, 2020 at 11:01 PM Thinh Nguyen <Thinh.Nguyen@synopsys.com> wrote:
>>>>>> Rob Herring wrote:
>>>>>>> On Thu, Jul 16, 2020 at 02:59:08PM -0700, Thinh Nguyen wrote:
>>>>>>>> Introduce num-lanes and lane-speed-mantissa-gbps for devices operating
>>>>>>>> in super-speed-plus. DWC_usb32 IP supports multiple lanes and can
>>>>>>>> operate in different sublink speeds. Currently the device controller
>>>>>>>> does not have the information of the phy's number of lanes supported. As
>>>>>>>> a result, the user can specify them through these properties if they are
>>>>>>>> different than the default setting.
>>>>>>>>
>>>>>>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>>>>>>> ---
>>>>>>>>       Documentation/devicetree/bindings/usb/dwc3.txt | 9 +++++++++
>>>>>>>>       1 file changed, 9 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>>>> index d03edf9d3935..4eba0615562f 100644
>>>>>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>>>>>> @@ -86,6 +86,15 @@ Optional properties:
>>>>>>>>        - snps,quirk-frame-length-adjustment: Value for GFLADJ_30MHZ field of GFLADJ
>>>>>>>>          register for post-silicon frame length adjustment when the
>>>>>>>>          fladj_30mhz_sdbnd signal is invalid or incorrect.
>>>>>>>> + - snps,num-lanes: set to specify the number of lanes to use. Valid inputs are
>>>>>>>> +                    1 or 2. Apply if the maximum-speed is super-speed-plus
>>>>>>>> +                    only. Default value is 2 for DWC_usb32. For DWC_usb31,
>>>>>>>> +                    it is always 1 at super-speed-plus.
>>>>>>>> + - snps,lane-speed-mantissa-gbps: set to specify the symmetric lane speed
>>>>>>>> +                    mantissa in Gbps. Valid inputs are 5 or 10. Apply if
>>>>>>>> +                    the maximum-speed is super-speed-plus only. Default
>>>>>>>> +                    value is 10. For DWC_usb31, it's always 10 at
>>>>>>>> +                    super-speed-plus.
>>>>>>> This is all common USB things and should be common properties (which we
>>>>>>> may already have).
>>>>>> Sure. For "num-lanes" is simple, any objection if we use
>>>>>> "lane-speed-mantissa-gbps"? Or should we add "lane-speed-exponent"?
>>>>> 'num-lanes' is good as that's what PCIe uses. Document that with
>>>>> 'maximum-speed'.
>>>>>
>>>>> I think 'super-speed-plus' should mean gen 2 10G per lane. Then
>>>>> between num-lanes and maximum-speed you can define all 4 possible
>>>>> rates.
>>>> That may confuse the user because now we'd use 'super-speed-plus' to
>>>> define the speed of the lane rather than the device itself.
>>>>
>>>> According to the USB 3.2 spec, super-speed-plus can mean gen2x1, gen1x2,
>>>> or gen2x2.
>>> Then add new strings as needed to make it clear: super-speed-plus-gen1x2
>>>
>>> It's obvious that what 'super-speed-plus' means is not clear since
>>> USB-IF extended its meaning.
>>>
>>> Rob
>> If we introduce a new enum for gen1x2, now we'd have to go back and
>> inspect all the checks for all the drivers where for example speed ==
>> USB_SPEED_SUPER_PLUS. It seems to be more clunky and may introduce more
>> bugs.
>>
> In my opinion, the better option would be to introduce a new property
> for lane speed such as "lane-speed-mantissa-gbps" because:
>
> 1) It still follows the spec, easier for the user to understand
> 2) We only need to update the drivers where the number of lanes and lane
> speed matter
> 3) Easier speed comparison between usb_device_speed enum
> 4) Easier to backport new code where there's speed comparison
> 5) Easily extendable to new/different lane speeds
>

Let me send out v2 of this series so that others can also provide more 
feedback on other patches. We can continue with this discussion as 
needed in the meanwhile.

BR,
Thinh