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

Thinh Nguyen wrote:

Thinh Nguyen wrote:

quoted

Rob Herring wrote:

quoted

On Tue, Jul 21, 2020 at 10:42 AM Thinh Nguyen [off-list ref] wrote:

quoted

Rob Herring wrote:

quoted

On Mon, Jul 20, 2020 at 11:01 PM Thinh Nguyen [off-list ref] wrote:

quoted

Rob Herring wrote:

quoted

On Thu, Jul 16, 2020 at 02:59:08PM -0700, Thinh Nguyen wrote:

quoted

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