[PATCH v3 00/12] usb: Handle different sublink speeds

[PATCH v3 00/12] 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.

Changes in v3:
 - Remove "num-lanes" and "lane-speed-mantissa-gbps" common properties
 - Remove "num-lanes" and "lane-speed-mantissa-gbps" properties validation in dwc3
 - Update "maximum-speed" to support variations of SSP Gen X x Y
 - Update common function to parse new strings for "maximum-speed"
 - Update commit messages for the new changes

Changes in v2:
 - Move usb_sublink_speed attribute struct and enum to include/linux/usb/ch9.h
 - Use "num-lanes" and "lane-speed-mantissa-gbps" as common properties instead
 - Add common functions to get num-lanes and lsm properties
 - Fix missing gen1x2 sublink speed attribute check report in dwc3


Thinh Nguyen (12):
  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: Include USB SSP Gen X x Y
  usb: common: Add function to get num_lanes and transfer rate
  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/generic.txt |  11 +-
 drivers/usb/common/common.c                       |  47 ++++++-
 drivers/usb/dwc3/core.c                           |  31 ++++-
 drivers/usb/dwc3/core.h                           |  14 +++
 drivers/usb/dwc3/gadget.c                         | 143 +++++++++++++++++++++-
 drivers/usb/gadget/composite.c                    |  81 ++++++++----
 drivers/usb/gadget/udc/core.c                     |  24 +++-
 include/linux/usb/ch9.h                           |  68 ++++++++++
 include/linux/usb/gadget.h                        |  23 ++++
 9 files changed, 398 insertions(+), 44 deletions(-)


base-commit: a95fcda246dc3e9f5d00222c9a8f0a76aa0bb950
-- 
2.11.0

[PATCH v3 01/12] usb: ch9: Add sublink speed struct

[Thinh Nguyen]

USB 3.2 specification supports dual-lane for super-speed-plus. USB
devices may operate at different sublink speeds. To avoid using magic
numbers and capture the sublink speed better, introduce the
usb_sublink_speed structure and various sublink speed attribute enum.

See SSP BOS descriptor in USB 3.2 specification section 9.6.2.5

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
Changes in v3:
- None
Changes in v2:
- Move to include/linux/usb/ch9.h instead of under uapi

 include/linux/usb/ch9.h | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h
index 604c6c514a50..01191649a0ad 100644
--- a/include/linux/usb/ch9.h
+++ b/include/linux/usb/ch9.h
@@ -36,6 +36,49 @@
 #include <linux/device.h>
 #include <uapi/linux/usb/ch9.h>
 
+/* USB 3.2 sublink speed attributes */
+
+enum usb_lane_speed_exponent {
+	USB_LSE_BPS = 0,
+	USB_LSE_KBPS = 1,
+	USB_LSE_MBPS = 2,
+	USB_LSE_GBPS = 3,
+};
+
+enum usb_sublink_type {
+	USB_ST_SYMMETRIC_RX = 0,
+	USB_ST_ASYMMETRIC_RX = 1,
+	USB_ST_SYMMETRIC_TX = 2,
+	USB_ST_ASYMMETRIC_TX = 3,
+};
+
+enum usb_link_protocol {
+	USB_LP_SS = 0,
+	USB_LP_SSP = 1,
+};
+
+/**
+ * struct usb_sublink_speed - sublink speed attribute
+ * @id: sublink speed attribute ID (SSID)
+ * @mantissa: lane speed mantissa
+ * @exponent: lane speed exponent
+ * @type: sublink type
+ * @protocol: sublink protocol
+ *
+ * Super-speed-plus supports multiple lanes. Use the sublink speed attributes to
+ * describe the sublink speed.
+ *
+ * See USB 3.2 spec section 9.6.2.6 for super-speed-plus capability for more
+ * information.
+ */
+struct usb_sublink_speed {
+	u8				id;
+	u16				mantissa;
+	enum usb_lane_speed_exponent	exponent;
+	enum usb_sublink_type		type;
+	enum usb_link_protocol		protocol;
+};
+
 /**
  * usb_ep_type_string() - Returns human readable-name of the endpoint type.
  * @ep_type: The endpoint type to return human-readable name for.  If it's not
-- 
2.11.0

[PATCH v3 02/12] usb: gadget: composite: Avoid using magic numbers

[Thinh Nguyen]

Magic numbers are difficult to read. Use macros for super-speed-plus BOS
descriptor attributes in the composite driver. They're self-documented.
So there's no need to provide comments as we did previously for the
magic numbers.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 Changes in v3:
 - None
 Changes in v2:
 - None

 drivers/usb/gadget/composite.c | 41 ++++++++++++++++++++---------------------
 1 file changed, 20 insertions(+), 21 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 05b176c82cc5..0aa4cb49aa53 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -13,6 +13,7 @@
 #include <linux/module.h>
 #include <linux/device.h>
 #include <linux/utsname.h>
+#include <linux/bitfield.h>
 
 #include <linux/usb/composite.h>
 #include <linux/usb/otg.h>
@@ -743,32 +744,30 @@ static int bos_desc(struct usb_composite_dev *cdev)
 		ssp_cap->bReserved = 0;
 		ssp_cap->wReserved = 0;
 
-		/* SSAC = 1 (2 attributes) */
-		ssp_cap->bmAttributes = cpu_to_le32(1);
+		ssp_cap->bmAttributes =
+			cpu_to_le32(FIELD_PREP(USB_SSP_SUBLINK_SPEED_ATTRIBS, 1) |
+				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_IDS, 0));
 
-		/* Min RX/TX Lane Count = 1 */
 		ssp_cap->wFunctionalitySupport =
-			cpu_to_le16((1 << 8) | (1 << 12));
+			cpu_to_le16(FIELD_PREP(USB_SSP_MIN_SUBLINK_SPEED_ATTRIBUTE_ID, 0) |
+				    FIELD_PREP(USB_SSP_MIN_RX_LANE_COUNT, 1) |
+				    FIELD_PREP(USB_SSP_MIN_TX_LANE_COUNT, 1));
 
-		/*
-		 * bmSublinkSpeedAttr[0]:
-		 *   ST  = Symmetric, RX
-		 *   LSE =  3 (Gbps)
-		 *   LP  =  1 (SuperSpeedPlus)
-		 *   LSM = 10 (10 Gbps)
-		 */
 		ssp_cap->bmSublinkSpeedAttr[0] =
-			cpu_to_le32((3 << 4) | (1 << 14) | (0xa << 16));
-		/*
-		 * bmSublinkSpeedAttr[1] =
-		 *   ST  = Symmetric, TX
-		 *   LSE =  3 (Gbps)
-		 *   LP  =  1 (SuperSpeedPlus)
-		 *   LSM = 10 (10 Gbps)
-		 */
+			cpu_to_le32(FIELD_PREP(USB_SSP_SUBLINK_SPEED_SSID, 0) |
+				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSE, USB_LSE_GBPS) |
+				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_ST,
+					       USB_ST_SYMMETRIC_RX) |
+				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LP, USB_LP_SSP) |
+				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSM, 10));
+
 		ssp_cap->bmSublinkSpeedAttr[1] =
-			cpu_to_le32((3 << 4) | (1 << 14) |
-				    (0xa << 16) | (1 << 7));
+			cpu_to_le32(FIELD_PREP(USB_SSP_SUBLINK_SPEED_SSID, 0) |
+				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSE, USB_LSE_GBPS) |
+				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_ST,
+					       USB_ST_SYMMETRIC_TX) |
+				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LP, USB_LP_SSP) |
+				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSM, 10));
 	}
 
 	return le16_to_cpu(bos->wTotalLength);
-- 
2.11.0

[PATCH v3 03/12] usb: gadget: Expose sublink speed attributes

[Thinh Nguyen]

The USB 3.2 specification supports dual-lane and different transfer
rates for super-speed-plus. Devices operating in super-speed-plus can
be gen2x1, gen1x2, or gen2x2.

A gadget driver may need to know the gadget's sublink speeds to properly
setup its transfer requests and describe its capability in its
descriptors. To describe the transfer rate in super-speed-plus fully,
let's expose the lane count and sublink speed attributes when operating
in super-speed-plus.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 Changes in v3:
 - None
 Changes in v2:
 - None

 include/linux/usb/gadget.h | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 52ce1f6b8f83..bd982669609c 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -339,6 +339,15 @@ struct usb_gadget_ops {
  * @speed: Speed of current connection to USB host.
  * @max_speed: Maximal speed the UDC can handle.  UDC must support this
  *      and all slower speeds.
+ * @num_lanes: Number of lanes in use.
+ * @max_num_lanes: Maximum number of lanes the UDC supports.
+ * @ssac: Sublink speed attribute count. The number of sublink speed
+ *	attributes is ssac + 1.
+ * @sublink_speed: Array of sublink speed attributes the UDC supports. Sublink
+ *	speed attributes are paired, and an RX followed by a TX attribute.
+ * @speed_ssid: Current sublink speed attribute ID in use.
+ * @min_speed_ssid: Sublink speed attribute ID with the minimum speed.
+ * @max_speed_ssid: Sublink speed attribute ID with the maximum speed.
  * @state: the state we are now (attached, suspended, configured, etc)
  * @name: Identifies the controller hardware type.  Used in diagnostics
  *	and sometimes configuration.
@@ -406,6 +415,17 @@ struct usb_gadget {
 	struct list_head		ep_list;	/* of usb_ep */
 	enum usb_device_speed		speed;
 	enum usb_device_speed		max_speed;
+
+	/* SSP only */
+	unsigned			num_lanes;
+	unsigned			max_num_lanes;
+	unsigned			ssac;
+#define USB_GADGET_MAX_SSAC 3
+	struct usb_sublink_speed	sublink_speed[USB_GADGET_MAX_SSAC + 1];
+	unsigned			speed_ssid;
+	unsigned			min_speed_ssid;
+	unsigned			max_speed_ssid;
+
 	enum usb_device_state		state;
 	const char			*name;
 	struct device			dev;
-- 
2.11.0

[PATCH v3 04/12] usb: gadget: Set max speed for SSP devices

[Thinh Nguyen]

A super-speed-plus device may operate at different sublink speeds (e.g.
gen2x2, gen1x2, or gen2x1). If the USB device supports different sublink
speeds at super-speed-plus, set the device to operate at the maximum
number of lanes and sublink speed possible. Introduce gadget ops
udc_set_num_lanes_and_speed to set the lane count and sublink speed for
super-speed-plus capable devices.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 Changes in v3:
 - None
 Changes in v2:
 - None

 drivers/usb/gadget/udc/core.c | 24 +++++++++++++++++++-----
 include/linux/usb/gadget.h    |  3 +++
 2 files changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
index 4f82bcd31fd3..e5f569dc9ee0 100644
--- a/drivers/usb/gadget/udc/core.c
+++ b/drivers/usb/gadget/udc/core.c
@@ -1114,12 +1114,26 @@ static inline void usb_gadget_udc_stop(struct usb_udc *udc)
 static inline void usb_gadget_udc_set_speed(struct usb_udc *udc,
 					    enum usb_device_speed speed)
 {
-	if (udc->gadget->ops->udc_set_speed) {
-		enum usb_device_speed s;
+	struct usb_gadget *gadget = udc->gadget;
+	enum usb_device_speed s;
 
-		s = min(speed, udc->gadget->max_speed);
-		udc->gadget->ops->udc_set_speed(udc->gadget, s);
-	}
+	if (speed == USB_SPEED_UNKNOWN)
+		s = gadget->max_speed;
+	else
+		s = min(speed, gadget->max_speed);
+
+	/*
+	 * If the UDC supports super-speed-plus and different sublink speeds,
+	 * then set the gadget to the max possible sublink speed for
+	 * super-speed-plus symmetric lanes.
+	 */
+	if (s == USB_SPEED_SUPER_PLUS &&
+	    gadget->ops->udc_set_num_lanes_and_speed)
+		gadget->ops->udc_set_num_lanes_and_speed(gadget,
+							 gadget->max_num_lanes,
+							 gadget->max_speed_ssid);
+	else if (gadget->ops->udc_set_speed)
+		gadget->ops->udc_set_speed(gadget, s);
 }
 
 /**
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index bd982669609c..0a4f72cfe3c4 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -323,6 +323,9 @@ struct usb_gadget_ops {
 			struct usb_gadget_driver *);
 	int	(*udc_stop)(struct usb_gadget *);
 	void	(*udc_set_speed)(struct usb_gadget *, enum usb_device_speed);
+	void	(*udc_set_num_lanes_and_speed)(struct usb_gadget *,
+					       unsigned int num_lanes,
+					       unsigned int ssid);
 	struct usb_ep *(*match_ep)(struct usb_gadget *,
 			struct usb_endpoint_descriptor *,
 			struct usb_ss_ep_comp_descriptor *);
-- 
2.11.0

[PATCH v3 05/12] usb: composite: Properly report sublink speed

[Thinh Nguyen]

Use the max sublink speed attributes reported in the gadget structure
to write to the super-speed-plus BOS descriptor if available.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 Changes in v3:
 - None
 Changes in v2:
 - None

 drivers/usb/gadget/composite.c | 76 +++++++++++++++++++++++++++++-------------
 1 file changed, 52 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 0aa4cb49aa53..2d0b7af4b08f 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -729,45 +729,73 @@ static int bos_desc(struct usb_composite_dev *cdev)
 	/* The SuperSpeedPlus USB Device Capability descriptor */
 	if (gadget_is_superspeed_plus(cdev->gadget)) {
 		struct usb_ssp_cap_descriptor *ssp_cap;
+		unsigned int ssac = 1;
+		unsigned int ssic = 0;
+		unsigned int min_ssid = 0;
+		int i;
+
+		if (cdev->gadget->ssac) {
+			ssac = cdev->gadget->ssac;
+
+			/*
+			 * Paired RX and TX sublink speed attributes share
+			 * the same SSID.
+			 */
+			ssic = (ssac + 1) / 2 - 1;
+			min_ssid = cdev->gadget->min_speed_ssid;
+		}
 
 		ssp_cap = cdev->req->buf + le16_to_cpu(bos->wTotalLength);
 		bos->bNumDeviceCaps++;
 
-		/*
-		 * Report typical values.
-		 */
-
-		le16_add_cpu(&bos->wTotalLength, USB_DT_USB_SSP_CAP_SIZE(1));
-		ssp_cap->bLength = USB_DT_USB_SSP_CAP_SIZE(1);
+		le16_add_cpu(&bos->wTotalLength, USB_DT_USB_SSP_CAP_SIZE(ssac));
+		ssp_cap->bLength = USB_DT_USB_SSP_CAP_SIZE(ssac);
 		ssp_cap->bDescriptorType = USB_DT_DEVICE_CAPABILITY;
 		ssp_cap->bDevCapabilityType = USB_SSP_CAP_TYPE;
 		ssp_cap->bReserved = 0;
 		ssp_cap->wReserved = 0;
 
 		ssp_cap->bmAttributes =
-			cpu_to_le32(FIELD_PREP(USB_SSP_SUBLINK_SPEED_ATTRIBS, 1) |
-				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_IDS, 0));
+			cpu_to_le32(FIELD_PREP(USB_SSP_SUBLINK_SPEED_ATTRIBS, ssac) |
+				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_IDS, ssic));
 
 		ssp_cap->wFunctionalitySupport =
-			cpu_to_le16(FIELD_PREP(USB_SSP_MIN_SUBLINK_SPEED_ATTRIBUTE_ID, 0) |
+			cpu_to_le16(FIELD_PREP(USB_SSP_MIN_SUBLINK_SPEED_ATTRIBUTE_ID, min_ssid) |
 				    FIELD_PREP(USB_SSP_MIN_RX_LANE_COUNT, 1) |
 				    FIELD_PREP(USB_SSP_MIN_TX_LANE_COUNT, 1));
 
-		ssp_cap->bmSublinkSpeedAttr[0] =
-			cpu_to_le32(FIELD_PREP(USB_SSP_SUBLINK_SPEED_SSID, 0) |
-				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSE, USB_LSE_GBPS) |
-				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_ST,
-					       USB_ST_SYMMETRIC_RX) |
-				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LP, USB_LP_SSP) |
-				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSM, 10));
-
-		ssp_cap->bmSublinkSpeedAttr[1] =
-			cpu_to_le32(FIELD_PREP(USB_SSP_SUBLINK_SPEED_SSID, 0) |
-				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSE, USB_LSE_GBPS) |
-				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_ST,
-					       USB_ST_SYMMETRIC_TX) |
-				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LP, USB_LP_SSP) |
-				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSM, 10));
+		/*
+		 * If the sublink speed attributes are not specified, then the
+		 * default will be a pair symmetric RX/TX sublink speed
+		 * attributes of 10 Gbps.
+		 */
+		for (i = 0; i < ssac + 1; i++) {
+			struct usb_sublink_speed default_ssa;
+			struct usb_sublink_speed *ptr;
+
+			if (cdev->gadget->ssac) {
+				ptr = &cdev->gadget->sublink_speed[i];
+			} else {
+				default_ssa.id = i / 2;
+				default_ssa.protocol = USB_LP_SSP;
+				default_ssa.exponent = USB_LSE_GBPS;
+				default_ssa.mantissa = 10;
+
+				if (i % 2)
+					default_ssa.type = USB_ST_SYMMETRIC_TX;
+				else
+					default_ssa.type = USB_ST_SYMMETRIC_RX;
+
+				ptr = &default_ssa;
+			}
+
+			ssp_cap->bmSublinkSpeedAttr[i] =
+				cpu_to_le32(FIELD_PREP(USB_SSP_SUBLINK_SPEED_SSID, ptr->id) |
+					    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSE, ptr->exponent) |
+					    FIELD_PREP(USB_SSP_SUBLINK_SPEED_ST, ptr->type) |
+					    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LP, ptr->protocol) |
+					    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSM, ptr->mantissa));
+		}
 	}
 
 	return le16_to_cpu(bos->wTotalLength);
-- 
2.11.0

[PATCH v3 06/12] usb: devicetree: Include USB SSP Gen X x Y

[Thinh Nguyen]

According to the USB 3.2 spec, a super-speed-plus device can operate at
gen2x2, gen2x1, or gen1x2. If the USB controller device supports
multiple lanes at different transfer rates, the user can specify the HW
capability via these new speed strings:

"super-speed-plus-gen2x2"
"super-speed-plus-gen2x1"
"super-speed-plus-gen1x2"

If the argument is simply "super-speed-plus", USB controllers should
default to their maximum transfer rate and number of lanes.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
Changes in v3:
- Use "maximum-speed" to include both the num-lane and transfer rate for SSP
- Remove "num-lanes" and "lane-speed-mantissa-gbps" properties
Changes in v2:
- Make "num-lanes" and "lane-speed-mantissa-gbps" common USB properties

 Documentation/devicetree/bindings/usb/generic.txt | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/generic.txt b/Documentation/devicetree/bindings/usb/generic.txt
index ba472e7aefc9..8541b9571f2f 100644
--- a/Documentation/devicetree/bindings/usb/generic.txt
+++ b/Documentation/devicetree/bindings/usb/generic.txt
@@ -3,10 +3,13 @@ Generic USB Properties
 Optional properties:
  - maximum-speed: tells USB controllers we want to work up to a certain
 			speed. Valid arguments are "super-speed-plus",
-			"super-speed", "high-speed", "full-speed" and
-			"low-speed". In case this isn't passed via DT, USB
-			controllers should default to their maximum HW
-			capability.
+			"super-speed-plus-gen2x2", "super-speed-plus-gen2x1",
+			"super-speed-plus-gen1x2", "super-speed", "high-speed",
+			"full-speed" and "low-speed". In case this isn't passed
+			via DT, USB controllers should default to their maximum
+			HW capability. Similarly, if the argument is
+			"super-speed-plus", USB controllers should default to
+			their maximum transfer rate and number of lanes.
  - dr_mode: tells Dual-Role USB controllers that we want to work on a
 			particular mode. Valid arguments are "host",
 			"peripheral" and "otg". In case this attribute isn't
-- 
2.11.0

[PATCH v3 07/12] usb: common: Add function to get num_lanes and transfer rate

[Thinh Nguyen]

Add a new common function to parse maximum_speed property string for
the specified number of lanes and transfer rate.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
Changes in v3:
- Add new function to parse "maximum-speed" for lanes and transfer rate
- Remove separate functions getting num_lanes and transfer rate properties
Changes in v2:
- New commit

 drivers/usb/common/common.c | 47 ++++++++++++++++++++++++++++++++++++++++++---
 include/linux/usb/ch9.h     | 25 ++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
index 1433260d99b4..53b4950c49e4 100644
--- a/drivers/usb/common/common.c
+++ b/drivers/usb/common/common.c
@@ -77,18 +77,59 @@ const char *usb_speed_string(enum usb_device_speed speed)
 }
 EXPORT_SYMBOL_GPL(usb_speed_string);
 
-enum usb_device_speed usb_get_maximum_speed(struct device *dev)
+void usb_get_maximum_speed_and_num_lanes(struct device *dev,
+					 enum usb_device_speed *speed,
+					 enum usb_phy_gen *gen,
+					 u8 *num_lanes)
 {
 	const char *maximum_speed;
+	enum usb_device_speed matched_speed = USB_SPEED_UNKNOWN;
+	enum usb_phy_gen matched_gen = USB_PHY_GEN_UNKNOWN;
+	u8 matched_num_lanes = 0;
 	int ret;
 
 	ret = device_property_read_string(dev, "maximum-speed", &maximum_speed);
 	if (ret < 0)
-		return USB_SPEED_UNKNOWN;
+		goto done;
 
 	ret = match_string(speed_names, ARRAY_SIZE(speed_names), maximum_speed);
+	if (ret >= 0) {
+		matched_speed = ret;
+		goto done;
+	}
+
+	if (strncmp("super-speed-plus-gen2x2", maximum_speed, 23) == 0) {
+		matched_speed = USB_SPEED_SUPER_PLUS;
+		matched_gen = USB_PHY_GEN_2;
+		matched_num_lanes = 2;
+	} else if (strncmp("super-speed-plus-gen2x1", maximum_speed, 23) == 0) {
+		matched_speed = USB_SPEED_SUPER_PLUS;
+		matched_gen = USB_PHY_GEN_2;
+		matched_num_lanes = 1;
+	} else if (strncmp("super-speed-plus-gen1x2", maximum_speed, 23) == 0) {
+		matched_speed = USB_SPEED_SUPER_PLUS;
+		matched_gen = USB_PHY_GEN_1;
+		matched_num_lanes = 2;
+	}
+
+done:
+	if (speed)
+		*speed = matched_speed;
+
+	if (num_lanes)
+		*num_lanes = matched_num_lanes;
+
+	if (gen)
+		*gen = matched_gen;
+}
+EXPORT_SYMBOL_GPL(usb_get_maximum_speed_and_num_lanes);
+
+enum usb_device_speed usb_get_maximum_speed(struct device *dev)
+{
+	enum usb_device_speed speed;
 
-	return (ret < 0) ? USB_SPEED_UNKNOWN : ret;
+	usb_get_maximum_speed_and_num_lanes(dev, &speed, NULL, NULL);
+	return speed;
 }
 EXPORT_SYMBOL_GPL(usb_get_maximum_speed);
 
diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h
index 01191649a0ad..46cfd72e7082 100644
--- a/include/linux/usb/ch9.h
+++ b/include/linux/usb/ch9.h
@@ -57,6 +57,13 @@ enum usb_link_protocol {
 	USB_LP_SSP = 1,
 };
 
+/* USB phy signaling rate gen */
+enum usb_phy_gen {
+	USB_PHY_GEN_UNKNOWN,
+	USB_PHY_GEN_1,
+	USB_PHY_GEN_2,
+};
+
 /**
  * struct usb_sublink_speed - sublink speed attribute
  * @id: sublink speed attribute ID (SSID)
@@ -95,6 +102,24 @@ extern const char *usb_ep_type_string(int ep_type);
  */
 extern const char *usb_speed_string(enum usb_device_speed speed);
 
+/**
+ * usb_get_maximum_speed_and_num_lanes - Get maximum requested speed and number
+ * of lanes for a given USB controller.
+ * @dev: Pointer to the given USB controller device
+ * @speed: Where to output enum usb_device_speed
+ * @gen: Where to output phy signaling rate gen
+ * @num_lanes: Where to output number of requested lanes
+ *
+ * This function gets the maximum speed string from property "maximum-speed"
+ * and output the appropriate speed of the device. If the maximum-speed string
+ * is super-speed-plus-gen*, then it also outputs the number of lanes and phy
+ * signaling rate 'Gen' value.
+ */
+extern void usb_get_maximum_speed_and_num_lanes(struct device *dev,
+						enum usb_device_speed *speed,
+						enum usb_phy_gen *gen,
+						u8 *num_lanes);
+
 /**
  * usb_get_maximum_speed - Get maximum requested speed for a given USB
  * controller.
-- 
2.11.0

[PATCH v3 08/12] usb: dwc3: Initialize lane count and sublink speed

[Thinh Nguyen]

DWC_usb32 supports dual-lane operating at different sublink speeds.
Initialize and validate the maximum number of lanes and speed the
controller supports in the maximum_speed property.

Currently the controller has no visibility into the HW parameter to
determine the maximum number of lanes the HW supports. If the number of
lanes is not specified, then set the default to 2 for DWC_usb32 and 1
for DWC_usb31 for SSP.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
Changes in v3:
- Use new common function to get maximum-speed
- Remove num_lanes and lsm validation since they are no longer separate
  properties
- Replace dwc->maxmum_lsm field with dwc->maximum_ssp_rate for gen1/gen2
Changes in v2:
- Use common functions to get num_lanes and lsm properties

 drivers/usb/dwc3/core.c | 31 ++++++++++++++++++++++++++++++-
 drivers/usb/dwc3/core.h |  6 ++++++
 2 files changed, 36 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 422aea24afcd..4c62337e8765 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1274,7 +1274,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
 	 */
 	hird_threshold = 12;
 
-	dwc->maximum_speed = usb_get_maximum_speed(dev);
+	usb_get_maximum_speed_and_num_lanes(dev, &dwc->maximum_speed,
+					    &dwc->maximum_phy_gen,
+					    &dwc->maximum_num_lanes);
 	dwc->dr_mode = usb_get_dr_mode(dev);
 	dwc->hsphy_mode = of_usb_get_phy_mode(dev->of_node);
 
@@ -1426,6 +1428,33 @@ static void dwc3_check_params(struct dwc3 *dwc)
 
 		break;
 	}
+
+	/*
+	 * Currently the controller does not have visibility into the HW
+	 * parameter to determine the maximum number of lanes the phy
+	 * supports. If the number of lanes is not specified in the
+	 * device property, then set the default to 2 for DWC_usb32 and
+	 * 1 for DWC_usb31 for super-speed-plus.
+	 */
+	if (dwc->maximum_speed == USB_SPEED_SUPER_PLUS) {
+		if (DWC3_IP_IS(DWC32)) {
+			if (dwc->maximum_phy_gen == USB_PHY_GEN_UNKNOWN)
+				dwc->maximum_phy_gen = USB_PHY_GEN_2;
+
+			if (!dwc->maximum_num_lanes)
+				dwc->maximum_num_lanes = 2;
+
+		} else if (DWC3_IP_IS(DWC31)) {
+			if (dwc->maximum_num_lanes > 1)
+				dev_warn(dev, "UDC doesn't support multi-lanes\n");
+
+			dwc->maximum_phy_gen = USB_PHY_GEN_2;
+			dwc->maximum_num_lanes = 1;
+		}
+	} else {
+		dwc->maximum_phy_gen = USB_PHY_GEN_UNKNOWN;
+		dwc->maximum_num_lanes = 0;
+	}
 }
 
 static int dwc3_probe(struct platform_device *pdev)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 2f04b3e42bf1..3bbfbeaa67d8 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -958,6 +958,8 @@ struct dwc3_scratchpad_array {
  * @nr_scratch: number of scratch buffers
  * @u1u2: only used on revisions <1.83a for workaround
  * @maximum_speed: maximum speed requested (mainly for testing purposes)
+ * @maximum_phy_gen: maximum phy signaling rate
+ * @maximum_num_lanes: maximum number of lanes
  * @ip: controller's ID
  * @revision: controller's version of an IP
  * @version_type: VERSIONTYPE register contents, a sub release of a revision
@@ -988,6 +990,7 @@ struct dwc3_scratchpad_array {
  * @ep0state: state of endpoint zero
  * @link_state: link state
  * @speed: device speed (super, high, full, low)
+ * @num_lanes: number of connected lanes
  * @hwparams: copy of hwparams registers
  * @root: debugfs root folder pointer
  * @regset: debugfs pointer to regdump file
@@ -1119,6 +1122,8 @@ struct dwc3 {
 	u32			nr_scratch;
 	u32			u1u2;
 	u32			maximum_speed;
+	enum usb_phy_gen	maximum_phy_gen;
+	u8			maximum_num_lanes;
 
 	u32			ip;
 
@@ -1184,6 +1189,7 @@ struct dwc3 {
 	u8			u1pel;
 
 	u8			speed;
+	u8			num_lanes;
 
 	u8			num_eps;
 
-- 
2.11.0

[PATCH v3 09/12] usb: dwc3: gadget: Report sublink speed capability

[Thinh Nguyen]

Report the sublink speed attributes to the usb_gadget structure based on
the HW capability from the device maximum_speed property.

Only DWC_usb32 supports 2 sublink speeds if it can operate with 2 lanes.
(i.e. at SSP, it can operate as gen1x2)

Note: the SSID DWC3_SSP_SSID_GEN2 and DWC3_SSP_SSID_GEN1 are arbitrary.
There's no standard according to the USB 3.2 spec as long as they are
unique and within 0-15.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
Changes in v3:
- Update commit with updated field name
- No longer use DWC3_LSM_5/10_GBPS macros
Changes in v2:
- Fix missing check for gen1x2 when writing to sublink speed attributes
- Minor fix in commit message (first commit sentence ended with comma)

 drivers/usb/dwc3/core.h   |  4 ++++
 drivers/usb/dwc3/gadget.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 53 insertions(+)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 3bbfbeaa67d8..0ec675327014 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -40,6 +40,10 @@
 #define DWC3_XHCI_RESOURCES_NUM	2
 #define DWC3_ISOC_MAX_RETRIES	5
 
+/* Sublink Speed Attribute ID */
+#define DWC3_SSP_SSID_GEN2	2
+#define DWC3_SSP_SSID_GEN1	1
+
 #define DWC3_SCRATCHBUF_SIZE	4096	/* each buffer is assumed to be 4KiB */
 #define DWC3_EVENT_BUFFERS_SIZE	4096
 #define DWC3_EVENT_TYPE_MASK	0xfe
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index e44bfc3b5096..3038dae970f3 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -3686,6 +3686,55 @@ int dwc3_gadget_init(struct dwc3 *dwc)
 				dwc->revision);
 
 	dwc->gadget.max_speed		= dwc->maximum_speed;
+	dwc->gadget.max_num_lanes	= dwc->maximum_num_lanes;
+
+	if (dwc->maximum_speed == USB_SPEED_SUPER_PLUS) {
+		struct usb_sublink_speed *ssa;
+		int i;
+
+		/*
+		 * Multiple sublink speeds are only available to DWC_usb32
+		 * devices that can operate at gen2x2 max.
+		 */
+		if (dwc->maximum_phy_gen == USB_PHY_GEN_2 &&
+		    dwc->maximum_num_lanes == 2) {
+			dwc->gadget.ssac = 3;
+			dwc->gadget.min_speed_ssid = DWC3_SSP_SSID_GEN1;
+			dwc->gadget.max_speed_ssid = DWC3_SSP_SSID_GEN2;
+		} else if (dwc->maximum_phy_gen == USB_PHY_GEN_1 &&
+			   dwc->maximum_num_lanes == 2) {
+			dwc->gadget.ssac = 1;
+			dwc->gadget.min_speed_ssid = DWC3_SSP_SSID_GEN1;
+			dwc->gadget.max_speed_ssid = DWC3_SSP_SSID_GEN1;
+		} else {
+			dwc->gadget.ssac = 1;
+			dwc->gadget.min_speed_ssid = DWC3_SSP_SSID_GEN2;
+			dwc->gadget.max_speed_ssid = DWC3_SSP_SSID_GEN2;
+		}
+
+		for (i = 0; i < dwc->gadget.ssac + 1; i++) {
+			ssa = &dwc->gadget.sublink_speed[i];
+
+			if (dwc->gadget.ssac > 1 && i > 1)
+				ssa->id = dwc->gadget.max_speed_ssid;
+			else
+				ssa->id = dwc->gadget.min_speed_ssid;
+
+			if (ssa->id == DWC3_SSP_SSID_GEN1)
+				ssa->mantissa = 5;
+			else
+				ssa->mantissa = 10;
+
+			/* First attribute is RX followed by TX */
+			if (i % 2)
+				ssa->type = USB_ST_SYMMETRIC_TX;
+			else
+				ssa->type = USB_ST_SYMMETRIC_RX;
+
+			ssa->exponent = USB_LSE_GBPS;
+			ssa->protocol = USB_LP_SSP;
+		}
+	}
 
 	/*
 	 * REVISIT: Here we should clear all pending IRQs to be
-- 
2.11.0

[PATCH v3 10/12] usb: dwc3: gadget: Implement setting of sublink speed

[Thinh Nguyen]

Implement gadget ops udc_set_num_lanes_and_speed. This allows the
gadget/core driver to select number of lanes to use and the sublink
speed the controller supports.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 Changes in v3:
 - None
 Changes in v2:
 - None

 drivers/usb/dwc3/core.h   |  2 ++
 drivers/usb/dwc3/gadget.c | 64 ++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 0ec675327014..9466c4c36963 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -385,6 +385,8 @@
 #define DWC3_GUCTL2_RST_ACTBITLATER		BIT(14)
 
 /* Device Configuration Register */
+#define DWC3_DCFG_NUMLANES(n)	(((n) & 0x3) << 30) /* DWC_usb32 only */
+
 #define DWC3_DCFG_DEVADDR(addr)	((addr) << 3)
 #define DWC3_DCFG_DEVADDR_MASK	DWC3_DCFG_DEVADDR(0x7f)
 
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 3038dae970f3..b7da4e1c81aa 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2333,6 +2333,62 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
 	spin_unlock_irqrestore(&dwc->lock, flags);
 }
 
+static void dwc3_gadget_set_num_lanes_and_speed(struct usb_gadget *g,
+						unsigned int num_lanes,
+						unsigned int ssid)
+{
+	struct dwc3			*dwc = gadget_to_dwc(g);
+	struct usb_sublink_speed	*ssa = NULL;
+	unsigned int			lanes;
+	unsigned long			flags;
+	u32				reg;
+	int				i;
+
+	spin_lock_irqsave(&dwc->lock, flags);
+	if (dwc->maximum_speed < USB_SPEED_SUPER_PLUS)
+		goto done;
+
+	for (i = 0; i < g->ssac + 1; i++) {
+		if (g->sublink_speed[i].id == ssid) {
+			ssa = &g->sublink_speed[i];
+			break;
+		}
+	}
+
+	if (!ssa) {
+		dev_err(dwc->dev, "SSID not found (%d)\n", ssid);
+		goto done;
+	}
+
+	reg = dwc3_readl(dwc->regs, DWC3_DCFG);
+	reg &= ~DWC3_DCFG_SPEED_MASK;
+
+	switch (ssa->mantissa) {
+	case 5:
+		reg |= DWC3_DCFG_SUPERSPEED;
+		break;
+	case 10:
+		reg |= DWC3_DCFG_SUPERSPEED_PLUS;
+		break;
+	default:
+		dev_err(dwc->dev, "invalid lane speed mantissa (%d)\n",
+			ssa->mantissa);
+		goto done;
+	}
+
+	/* Lane configuration is only available to DWC_usb32 */
+	if (DWC3_IP_IS(DWC32)) {
+		lanes = clamp_t(unsigned int, num_lanes, 1,
+				dwc->maximum_num_lanes);
+		reg &= ~DWC3_DCFG_NUMLANES(~0);
+		reg |= DWC3_DCFG_NUMLANES(lanes - 1);
+	}
+
+	dwc3_writel(dwc->regs, DWC3_DCFG, reg);
+done:
+	spin_unlock_irqrestore(&dwc->lock, flags);
+}
+
 static const struct usb_gadget_ops dwc3_gadget_ops = {
 	.get_frame		= dwc3_gadget_get_frame,
 	.wakeup			= dwc3_gadget_wakeup,
@@ -2341,6 +2397,7 @@ static const struct usb_gadget_ops dwc3_gadget_ops = {
 	.udc_start		= dwc3_gadget_start,
 	.udc_stop		= dwc3_gadget_stop,
 	.udc_set_speed		= dwc3_gadget_set_speed,
+	.udc_set_num_lanes_and_speed = dwc3_gadget_set_num_lanes_and_speed,
 	.get_config_params	= dwc3_gadget_config_params,
 };
 
@@ -3751,7 +3808,12 @@ int dwc3_gadget_init(struct dwc3 *dwc)
 		goto err4;
 	}
 
-	dwc3_gadget_set_speed(&dwc->gadget, dwc->maximum_speed);
+	if (DWC3_IP_IS(DWC32) && dwc->maximum_speed == USB_SPEED_SUPER_PLUS)
+		dwc3_gadget_set_num_lanes_and_speed(&dwc->gadget,
+						    dwc->maximum_num_lanes,
+						    dwc->gadget.max_speed_ssid);
+	else
+		dwc3_gadget_set_speed(&dwc->gadget, dwc->maximum_speed);
 
 	return 0;
 
-- 
2.11.0

[PATCH v3 11/12] usb: dwc3: gadget: Track connected lane and sublink speed

[Thinh Nguyen]

Track the number of lanes connected in gadget->num_lanes and track the
current sublink speed attribute ID for super-speed-plus operations.

Note: if the device is running in gen1x2, set the gadget->speed to
USB_SPEED_SUPER_PLUS.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 Changes in v3:
 - None
 Changes in v2:
 - None

 drivers/usb/dwc3/core.h   |  2 ++
 drivers/usb/dwc3/gadget.c | 21 ++++++++++++++++++++-
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 9466c4c36963..4a70b1e4d2a7 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -460,6 +460,8 @@
 #define DWC3_DEVTEN_USBRSTEN		BIT(1)
 #define DWC3_DEVTEN_DISCONNEVTEN	BIT(0)
 
+#define DWC3_DSTS_CONNLANES(n)		(((n) >> 30) & 0x3) /* DWC_usb32 only */
+
 /* Device Status Register */
 #define DWC3_DSTS_DCNRD			BIT(29)
 
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index b7da4e1c81aa..05defaf0f745 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2328,6 +2328,10 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
 				reg |= DWC3_DCFG_SUPERSPEED_PLUS;
 		}
 	}
+
+	if (DWC3_IP_IS(DWC32) && speed < USB_SPEED_SUPER_PLUS)
+		reg &= ~DWC3_DCFG_NUMLANES(~0);
+
 	dwc3_writel(dwc->regs, DWC3_DCFG, reg);
 
 	spin_unlock_irqrestore(&dwc->lock, flags);
@@ -3199,12 +3203,19 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
 	struct dwc3_ep		*dep;
 	int			ret;
 	u32			reg;
+	u8			lanes = 1;
 	u8			speed;
 
 	reg = dwc3_readl(dwc->regs, DWC3_DSTS);
 	speed = reg & DWC3_DSTS_CONNECTSPD;
 	dwc->speed = speed;
 
+	if (DWC3_IP_IS(DWC32))
+		lanes = DWC3_DSTS_CONNLANES(reg) + 1;
+
+	dwc->gadget.num_lanes = lanes;
+	dwc->gadget.speed_ssid = 0;
+
 	/*
 	 * RAMClkSel is reset to 0 after USB reset, so it must be reprogrammed
 	 * each time on Connect Done.
@@ -3219,6 +3230,7 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
 		dwc3_gadget_ep0_desc.wMaxPacketSize = cpu_to_le16(512);
 		dwc->gadget.ep0->maxpacket = 512;
 		dwc->gadget.speed = USB_SPEED_SUPER_PLUS;
+		dwc->gadget.speed_ssid = DWC3_SSP_SSID_GEN2;
 		break;
 	case DWC3_DSTS_SUPERSPEED:
 		/*
@@ -3239,7 +3251,13 @@ static void dwc3_gadget_conndone_interrupt(struct dwc3 *dwc)
 
 		dwc3_gadget_ep0_desc.wMaxPacketSize = cpu_to_le16(512);
 		dwc->gadget.ep0->maxpacket = 512;
-		dwc->gadget.speed = USB_SPEED_SUPER;
+
+		if (lanes > 1) {
+			dwc->gadget.speed = USB_SPEED_SUPER_PLUS;
+			dwc->gadget.speed_ssid = DWC3_SSP_SSID_GEN1;
+		} else {
+			dwc->gadget.speed = USB_SPEED_SUPER;
+		}
 		break;
 	case DWC3_DSTS_HIGHSPEED:
 		dwc3_gadget_ep0_desc.wMaxPacketSize = cpu_to_le16(64);
@@ -3720,6 +3738,7 @@ int dwc3_gadget_init(struct dwc3 *dwc)
 	dwc->gadget.sg_supported	= true;
 	dwc->gadget.name		= "dwc3-gadget";
 	dwc->gadget.lpm_capable		= true;
+	dwc->gadget.num_lanes		= 1;
 
 	/*
 	 * FIXME We might be setting max_speed to <SUPER, however versions
-- 
2.11.0

[PATCH v3 12/12] usb: dwc3: gadget: Set speed only up to the max supported

[Thinh Nguyen]

The setting of device speed should be limited by the device's
maximum_speed. This patch adds a check and prevent the driver from
attempting to configure higher than the maximum_speed.

Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
---
 Changes in v3:
 - None
 Changes in v2:
 - None

 drivers/usb/dwc3/gadget.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 05defaf0f745..f62a9519287a 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -2276,6 +2276,7 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
 				  enum usb_device_speed speed)
 {
 	struct dwc3		*dwc = gadget_to_dwc(g);
+	enum usb_device_speed	selected_speed = speed;
 	unsigned long		flags;
 	u32			reg;
 
@@ -2300,7 +2301,10 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
 	    !dwc->dis_metastability_quirk) {
 		reg |= DWC3_DCFG_SUPERSPEED;
 	} else {
-		switch (speed) {
+		if (speed > dwc->maximum_speed)
+			selected_speed = dwc->maximum_speed;
+
+		switch (selected_speed) {
 		case USB_SPEED_LOW:
 			reg |= DWC3_DCFG_LOWSPEED;
 			break;
@@ -2320,7 +2324,8 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
 				reg |= DWC3_DCFG_SUPERSPEED_PLUS;
 			break;
 		default:
-			dev_err(dwc->dev, "invalid speed (%d)\n", speed);
+			dev_err(dwc->dev, "invalid speed (%d)\n",
+				selected_speed);
 
 			if (DWC3_IP_IS(DWC3))
 				reg |= DWC3_DCFG_SUPERSPEED;
@@ -2329,7 +2334,7 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
 		}
 	}
 
-	if (DWC3_IP_IS(DWC32) && speed < USB_SPEED_SUPER_PLUS)
+	if (DWC3_IP_IS(DWC32) && selected_speed < USB_SPEED_SUPER_PLUS)
 		reg &= ~DWC3_DCFG_NUMLANES(~0);
 
 	dwc3_writel(dwc->regs, DWC3_DCFG, reg);
-- 
2.11.0

Re: [PATCH v3 03/12] usb: gadget: Expose sublink speed attributes

[Chunfeng Yun]

On Fri, 2020-07-24 at 16:38 -0700, Thinh Nguyen wrote:
> The USB 3.2 specification supports dual-lane and different transfer
> rates for super-speed-plus. Devices operating in super-speed-plus can
> be gen2x1, gen1x2, or gen2x2.
> 
> A gadget driver may need to know the gadget's sublink speeds to properly
> setup its transfer requests and describe its capability in its
> descriptors. To describe the transfer rate in super-speed-plus fully,
> let's expose the lane count and sublink speed attributes when operating
> in super-speed-plus.
> 
> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> ---
>  Changes in v3:
>  - None
>  Changes in v2:
>  - None
> 
>  include/linux/usb/gadget.h | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 52ce1f6b8f83..bd982669609c 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -339,6 +339,15 @@ struct usb_gadget_ops {
>   * @speed: Speed of current connection to USB host.
>   * @max_speed: Maximal speed the UDC can handle.  UDC must support this
>   *      and all slower speeds.
> + * @num_lanes: Number of lanes in use.
> + * @max_num_lanes: Maximum number of lanes the UDC supports.
> + * @ssac: Sublink speed attribute count. The number of sublink speed
> + *	attributes is ssac + 1.
> + * @sublink_speed: Array of sublink speed attributes the UDC supports. Sublink
> + *	speed attributes are paired, and an RX followed by a TX attribute.
> + * @speed_ssid: Current sublink speed attribute ID in use.
> + * @min_speed_ssid: Sublink speed attribute ID with the minimum speed.
> + * @max_speed_ssid: Sublink speed attribute ID with the maximum speed.
>   * @state: the state we are now (attached, suspended, configured, etc)
>   * @name: Identifies the controller hardware type.  Used in diagnostics
>   *	and sometimes configuration.
> @@ -406,6 +415,17 @@ struct usb_gadget {
>  	struct list_head		ep_list;	/* of usb_ep */
>  	enum usb_device_speed		speed;
>  	enum usb_device_speed		max_speed;
> +
> +	/* SSP only */
> +	unsigned			num_lanes;
> +	unsigned			max_num_lanes;
> +	unsigned			ssac;
> +#define USB_GADGET_MAX_SSAC 3
> +	struct usb_sublink_speed	sublink_speed[USB_GADGET_MAX_SSAC + 1];
> +	unsigned			speed_ssid;
> +	unsigned			min_speed_ssid;
> +	unsigned			max_speed_ssid;
checkpatch warning:

WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'

> +
>  	enum usb_device_state		state;
>  	const char			*name;
>  	struct device			dev;

Re: [PATCH v3 03/12] usb: gadget: Expose sublink speed attributes

[Thinh Nguyen]

Chunfeng Yun wrote:
> On Fri, 2020-07-24 at 16:38 -0700, Thinh Nguyen wrote:
>> The USB 3.2 specification supports dual-lane and different transfer
>> rates for super-speed-plus. Devices operating in super-speed-plus can
>> be gen2x1, gen1x2, or gen2x2.
>>
>> A gadget driver may need to know the gadget's sublink speeds to properly
>> setup its transfer requests and describe its capability in its
>> descriptors. To describe the transfer rate in super-speed-plus fully,
>> let's expose the lane count and sublink speed attributes when operating
>> in super-speed-plus.
>>
>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>> ---
>>   Changes in v3:
>>   - None
>>   Changes in v2:
>>   - None
>>
>>   include/linux/usb/gadget.h | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>> index 52ce1f6b8f83..bd982669609c 100644
>> --- a/include/linux/usb/gadget.h
>> +++ b/include/linux/usb/gadget.h
>> @@ -339,6 +339,15 @@ struct usb_gadget_ops {
>>    * @speed: Speed of current connection to USB host.
>>    * @max_speed: Maximal speed the UDC can handle.  UDC must support this
>>    *      and all slower speeds.
>> + * @num_lanes: Number of lanes in use.
>> + * @max_num_lanes: Maximum number of lanes the UDC supports.
>> + * @ssac: Sublink speed attribute count. The number of sublink speed
>> + *	attributes is ssac + 1.
>> + * @sublink_speed: Array of sublink speed attributes the UDC supports. Sublink
>> + *	speed attributes are paired, and an RX followed by a TX attribute.
>> + * @speed_ssid: Current sublink speed attribute ID in use.
>> + * @min_speed_ssid: Sublink speed attribute ID with the minimum speed.
>> + * @max_speed_ssid: Sublink speed attribute ID with the maximum speed.
>>    * @state: the state we are now (attached, suspended, configured, etc)
>>    * @name: Identifies the controller hardware type.  Used in diagnostics
>>    *	and sometimes configuration.
>> @@ -406,6 +415,17 @@ struct usb_gadget {
>>   	struct list_head		ep_list;	/* of usb_ep */
>>   	enum usb_device_speed		speed;
>>   	enum usb_device_speed		max_speed;
>> +
>> +	/* SSP only */
>> +	unsigned			num_lanes;
>> +	unsigned			max_num_lanes;
>> +	unsigned			ssac;
>> +#define USB_GADGET_MAX_SSAC 3
>> +	struct usb_sublink_speed	sublink_speed[USB_GADGET_MAX_SSAC + 1];
>> +	unsigned			speed_ssid;
>> +	unsigned			min_speed_ssid;
>> +	unsigned			max_speed_ssid;
> checkpatch warning:
>
> WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'

Yes, but I'd like to keep them consistent with the rest of the fields in 
this structure.

>> +
>>   	enum usb_device_state		state;
>>   	const char			*name;
>>   	struct device			dev;

BR,
Thinh

Re: [PATCH v3 07/12] usb: common: Add function to get num_lanes and transfer rate

[Chunfeng Yun]

On Fri, 2020-07-24 at 16:39 -0700, Thinh Nguyen wrote:
> Add a new common function to parse maximum_speed property string for
> the specified number of lanes and transfer rate.
> 
> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> ---
> Changes in v3:
> - Add new function to parse "maximum-speed" for lanes and transfer rate
> - Remove separate functions getting num_lanes and transfer rate properties
> Changes in v2:
> - New commit
> 
>  drivers/usb/common/common.c | 47 ++++++++++++++++++++++++++++++++++++++++++---
>  include/linux/usb/ch9.h     | 25 ++++++++++++++++++++++++
>  2 files changed, 69 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
> index 1433260d99b4..53b4950c49e4 100644
> --- a/drivers/usb/common/common.c
> +++ b/drivers/usb/common/common.c
> @@ -77,18 +77,59 @@ const char *usb_speed_string(enum usb_device_speed speed)
>  }
>  EXPORT_SYMBOL_GPL(usb_speed_string);
>  
> -enum usb_device_speed usb_get_maximum_speed(struct device *dev)
> +void usb_get_maximum_speed_and_num_lanes(struct device *dev,
> +					 enum usb_device_speed *speed,
> +					 enum usb_phy_gen *gen,
> +					 u8 *num_lanes)
>  {
>  	const char *maximum_speed;
> +	enum usb_device_speed matched_speed = USB_SPEED_UNKNOWN;
> +	enum usb_phy_gen matched_gen = USB_PHY_GEN_UNKNOWN;
> +	u8 matched_num_lanes = 0;
>  	int ret;
>  
>  	ret = device_property_read_string(dev, "maximum-speed", &maximum_speed);
>  	if (ret < 0)
> -		return USB_SPEED_UNKNOWN;
> +		goto done;
>  
>  	ret = match_string(speed_names, ARRAY_SIZE(speed_names), maximum_speed);
> +	if (ret >= 0) {
> +		matched_speed = ret;
> +		goto done;
> +	}
> +
> +	if (strncmp("super-speed-plus-gen2x2", maximum_speed, 23) == 0) {
> +		matched_speed = USB_SPEED_SUPER_PLUS;
> +		matched_gen = USB_PHY_GEN_2;
> +		matched_num_lanes = 2;
> +	} else if (strncmp("super-speed-plus-gen2x1", maximum_speed, 23) == 0) {
> +		matched_speed = USB_SPEED_SUPER_PLUS;
> +		matched_gen = USB_PHY_GEN_2;
> +		matched_num_lanes = 1;
> +	} else if (strncmp("super-speed-plus-gen1x2", maximum_speed, 23) == 0) {
> +		matched_speed = USB_SPEED_SUPER_PLUS;
> +		matched_gen = USB_PHY_GEN_1;
> +		matched_num_lanes = 2;
> +	}
> +
> +done:
> +	if (speed)
> +		*speed = matched_speed;
> +
> +	if (num_lanes)
> +		*num_lanes = matched_num_lanes;
> +
> +	if (gen)
> +		*gen = matched_gen;
> +}
> +EXPORT_SYMBOL_GPL(usb_get_maximum_speed_and_num_lanes);
> +
> +enum usb_device_speed usb_get_maximum_speed(struct device *dev)
> +{
> +	enum usb_device_speed speed;
>  
> -	return (ret < 0) ? USB_SPEED_UNKNOWN : ret;
> +	usb_get_maximum_speed_and_num_lanes(dev, &speed, NULL, NULL);
> +	return speed;
>  }
>  EXPORT_SYMBOL_GPL(usb_get_maximum_speed);
>  
> diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h
> index 01191649a0ad..46cfd72e7082 100644
> --- a/include/linux/usb/ch9.h
> +++ b/include/linux/usb/ch9.h
> @@ -57,6 +57,13 @@ enum usb_link_protocol {
>  	USB_LP_SSP = 1,
>  };
>  
> +/* USB phy signaling rate gen */
> +enum usb_phy_gen {
> +	USB_PHY_GEN_UNKNOWN,
> +	USB_PHY_GEN_1,
> +	USB_PHY_GEN_2,
> +};
The GEN_1, GEN_2 will describe the capability of not only PHY but also
MAC, add _PHY_ seems a little ambiguous, I think
usb_get_maximum_speed_and_num_lanes() is mainly used to get the
capability of MAC. Another, not suitable to add property about PHY
capablity in MAC nodes.

> +
>  /**
>   * struct usb_sublink_speed - sublink speed attribute
>   * @id: sublink speed attribute ID (SSID)
> @@ -95,6 +102,24 @@ extern const char *usb_ep_type_string(int ep_type);
>   */
>  extern const char *usb_speed_string(enum usb_device_speed speed);
>  
> +/**
> + * usb_get_maximum_speed_and_num_lanes - Get maximum requested speed and number
> + * of lanes for a given USB controller.
> + * @dev: Pointer to the given USB controller device
> + * @speed: Where to output enum usb_device_speed
> + * @gen: Where to output phy signaling rate gen
> + * @num_lanes: Where to output number of requested lanes
> + *
> + * This function gets the maximum speed string from property "maximum-speed"
> + * and output the appropriate speed of the device. If the maximum-speed string
> + * is super-speed-plus-gen*, then it also outputs the number of lanes and phy
> + * signaling rate 'Gen' value.
> + */
> +extern void usb_get_maximum_speed_and_num_lanes(struct device *dev,
> +						enum usb_device_speed *speed,
> +						enum usb_phy_gen *gen,
> +						u8 *num_lanes);
> +
>  /**
>   * usb_get_maximum_speed - Get maximum requested speed for a given USB
>   * controller.

Re: [PATCH v3 07/12] usb: common: Add function to get num_lanes and transfer rate

[Thinh Nguyen]

Chunfeng Yun wrote:
> On Fri, 2020-07-24 at 16:39 -0700, Thinh Nguyen wrote:
>> Add a new common function to parse maximum_speed property string for
>> the specified number of lanes and transfer rate.
>>
>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>> ---
>> Changes in v3:
>> - Add new function to parse "maximum-speed" for lanes and transfer rate
>> - Remove separate functions getting num_lanes and transfer rate properties
>> Changes in v2:
>> - New commit
>>
>>   drivers/usb/common/common.c | 47 ++++++++++++++++++++++++++++++++++++++++++---
>>   include/linux/usb/ch9.h     | 25 ++++++++++++++++++++++++
>>   2 files changed, 69 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
>> index 1433260d99b4..53b4950c49e4 100644
>> --- a/drivers/usb/common/common.c
>> +++ b/drivers/usb/common/common.c
>> @@ -77,18 +77,59 @@ const char *usb_speed_string(enum usb_device_speed speed)
>>   }
>>   EXPORT_SYMBOL_GPL(usb_speed_string);
>>   
>> -enum usb_device_speed usb_get_maximum_speed(struct device *dev)
>> +void usb_get_maximum_speed_and_num_lanes(struct device *dev,
>> +					 enum usb_device_speed *speed,
>> +					 enum usb_phy_gen *gen,
>> +					 u8 *num_lanes)
>>   {
>>   	const char *maximum_speed;
>> +	enum usb_device_speed matched_speed = USB_SPEED_UNKNOWN;
>> +	enum usb_phy_gen matched_gen = USB_PHY_GEN_UNKNOWN;
>> +	u8 matched_num_lanes = 0;
>>   	int ret;
>>   
>>   	ret = device_property_read_string(dev, "maximum-speed", &maximum_speed);
>>   	if (ret < 0)
>> -		return USB_SPEED_UNKNOWN;
>> +		goto done;
>>   
>>   	ret = match_string(speed_names, ARRAY_SIZE(speed_names), maximum_speed);
>> +	if (ret >= 0) {
>> +		matched_speed = ret;
>> +		goto done;
>> +	}
>> +
>> +	if (strncmp("super-speed-plus-gen2x2", maximum_speed, 23) == 0) {
>> +		matched_speed = USB_SPEED_SUPER_PLUS;
>> +		matched_gen = USB_PHY_GEN_2;
>> +		matched_num_lanes = 2;
>> +	} else if (strncmp("super-speed-plus-gen2x1", maximum_speed, 23) == 0) {
>> +		matched_speed = USB_SPEED_SUPER_PLUS;
>> +		matched_gen = USB_PHY_GEN_2;
>> +		matched_num_lanes = 1;
>> +	} else if (strncmp("super-speed-plus-gen1x2", maximum_speed, 23) == 0) {
>> +		matched_speed = USB_SPEED_SUPER_PLUS;
>> +		matched_gen = USB_PHY_GEN_1;
>> +		matched_num_lanes = 2;
>> +	}
>> +
>> +done:
>> +	if (speed)
>> +		*speed = matched_speed;
>> +
>> +	if (num_lanes)
>> +		*num_lanes = matched_num_lanes;
>> +
>> +	if (gen)
>> +		*gen = matched_gen;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_get_maximum_speed_and_num_lanes);
>> +
>> +enum usb_device_speed usb_get_maximum_speed(struct device *dev)
>> +{
>> +	enum usb_device_speed speed;
>>   
>> -	return (ret < 0) ? USB_SPEED_UNKNOWN : ret;
>> +	usb_get_maximum_speed_and_num_lanes(dev, &speed, NULL, NULL);
>> +	return speed;
>>   }
>>   EXPORT_SYMBOL_GPL(usb_get_maximum_speed);
>>   
>> diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h
>> index 01191649a0ad..46cfd72e7082 100644
>> --- a/include/linux/usb/ch9.h
>> +++ b/include/linux/usb/ch9.h
>> @@ -57,6 +57,13 @@ enum usb_link_protocol {
>>   	USB_LP_SSP = 1,
>>   };
>>   
>> +/* USB phy signaling rate gen */
>> +enum usb_phy_gen {
>> +	USB_PHY_GEN_UNKNOWN,
>> +	USB_PHY_GEN_1,
>> +	USB_PHY_GEN_2,
>> +};
> The GEN_1, GEN_2 will describe the capability of not only PHY but also
> MAC, add _PHY_ seems a little ambiguous, I think
> usb_get_maximum_speed_and_num_lanes() is mainly used to get the
> capability of MAC. Another, not suitable to add property about PHY
> capablity in MAC nodes.
>

 From USB 3.2 spec:
"Gen 1 is an adjective used to refer to the Physical layer associated 
with a 5.0 Gbps signaling rate. The original USB SuperSpeed Phy and a 
Gen 1 Phy refer to the same Phy."
"Gen 2 is an adjective used to refer to the Physical layer associated 
with a 10 Gbps signaling rate."

If you have a better name, I'm open for suggestions.

BR,
Thinh

Re: [PATCH v3 07/12] usb: common: Add function to get num_lanes and transfer rate

[Greg Kroah-Hartman]

On Fri, Jul 24, 2020 at 04:39:11PM -0700, Thinh Nguyen wrote:
> Add a new common function to parse maximum_speed property string for
> the specified number of lanes and transfer rate.
> 
> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> ---
> Changes in v3:
> - Add new function to parse "maximum-speed" for lanes and transfer rate
> - Remove separate functions getting num_lanes and transfer rate properties

No, why have you split this into a single function that for some reason
"can not fail"?

> Changes in v2:
> - New commit
> 
>  drivers/usb/common/common.c | 47 ++++++++++++++++++++++++++++++++++++++++++---
>  include/linux/usb/ch9.h     | 25 ++++++++++++++++++++++++
>  2 files changed, 69 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
> index 1433260d99b4..53b4950c49e4 100644
> --- a/drivers/usb/common/common.c
> +++ b/drivers/usb/common/common.c
> @@ -77,18 +77,59 @@ const char *usb_speed_string(enum usb_device_speed speed)
>  }
>  EXPORT_SYMBOL_GPL(usb_speed_string);
>  
> -enum usb_device_speed usb_get_maximum_speed(struct device *dev)
> +void usb_get_maximum_speed_and_num_lanes(struct device *dev,
> +					 enum usb_device_speed *speed,
> +					 enum usb_phy_gen *gen,
> +					 u8 *num_lanes)

What is wrong with just having multiple different functions instead?

>  {
>  	const char *maximum_speed;
> +	enum usb_device_speed matched_speed = USB_SPEED_UNKNOWN;
> +	enum usb_phy_gen matched_gen = USB_PHY_GEN_UNKNOWN;
> +	u8 matched_num_lanes = 0;
>  	int ret;
>  
>  	ret = device_property_read_string(dev, "maximum-speed", &maximum_speed);
>  	if (ret < 0)
> -		return USB_SPEED_UNKNOWN;
> +		goto done;
>  
>  	ret = match_string(speed_names, ARRAY_SIZE(speed_names), maximum_speed);
> +	if (ret >= 0) {
> +		matched_speed = ret;
> +		goto done;
> +	}
> +
> +	if (strncmp("super-speed-plus-gen2x2", maximum_speed, 23) == 0) {
> +		matched_speed = USB_SPEED_SUPER_PLUS;
> +		matched_gen = USB_PHY_GEN_2;
> +		matched_num_lanes = 2;
> +	} else if (strncmp("super-speed-plus-gen2x1", maximum_speed, 23) == 0) {
> +		matched_speed = USB_SPEED_SUPER_PLUS;
> +		matched_gen = USB_PHY_GEN_2;
> +		matched_num_lanes = 1;
> +	} else if (strncmp("super-speed-plus-gen1x2", maximum_speed, 23) == 0) {

Where are all of these device properties documented?



> +		matched_speed = USB_SPEED_SUPER_PLUS;
> +		matched_gen = USB_PHY_GEN_1;
> +		matched_num_lanes = 2;
> +	}
> +
> +done:
> +	if (speed)
> +		*speed = matched_speed;
> +
> +	if (num_lanes)
> +		*num_lanes = matched_num_lanes;
> +
> +	if (gen)
> +		*gen = matched_gen;



> +}
> +EXPORT_SYMBOL_GPL(usb_get_maximum_speed_and_num_lanes);
> +
> +enum usb_device_speed usb_get_maximum_speed(struct device *dev)
> +{
> +	enum usb_device_speed speed;
>  
> -	return (ret < 0) ? USB_SPEED_UNKNOWN : ret;
> +	usb_get_maximum_speed_and_num_lanes(dev, &speed, NULL, NULL);

Here's an example of why this isn't a good function.

It isn't self-describing.  Why do you pass in 3 pointers yet the name
only contains 2 things?

usb_get_maximum_speed_and_num_lanes_and_generation() shows that this is
not a good thing, right?

Again, 3 different functions please.

> +	return speed;
>  }
>  EXPORT_SYMBOL_GPL(usb_get_maximum_speed);
>  
> diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h
> index 01191649a0ad..46cfd72e7082 100644
> --- a/include/linux/usb/ch9.h
> +++ b/include/linux/usb/ch9.h
> @@ -57,6 +57,13 @@ enum usb_link_protocol {
>  	USB_LP_SSP = 1,
>  };
>  
> +/* USB phy signaling rate gen */
> +enum usb_phy_gen {
> +	USB_PHY_GEN_UNKNOWN,
> +	USB_PHY_GEN_1,
> +	USB_PHY_GEN_2,
> +};
> +
>  /**
>   * struct usb_sublink_speed - sublink speed attribute
>   * @id: sublink speed attribute ID (SSID)
> @@ -95,6 +102,24 @@ extern const char *usb_ep_type_string(int ep_type);
>   */
>  extern const char *usb_speed_string(enum usb_device_speed speed);
>  
> +/**
> + * usb_get_maximum_speed_and_num_lanes - Get maximum requested speed and number
> + * of lanes for a given USB controller.

You forgot that it also determines the "gen".

thanks,

greg k-h

Re: [PATCH v3 03/12] usb: gadget: Expose sublink speed attributes

[Greg Kroah-Hartman]

On Sat, Jul 25, 2020 at 03:33:39AM +0000, Thinh Nguyen wrote:
> Chunfeng Yun wrote:
> > On Fri, 2020-07-24 at 16:38 -0700, Thinh Nguyen wrote:
> >> The USB 3.2 specification supports dual-lane and different transfer
> >> rates for super-speed-plus. Devices operating in super-speed-plus can
> >> be gen2x1, gen1x2, or gen2x2.
> >>
> >> A gadget driver may need to know the gadget's sublink speeds to properly
> >> setup its transfer requests and describe its capability in its
> >> descriptors. To describe the transfer rate in super-speed-plus fully,
> >> let's expose the lane count and sublink speed attributes when operating
> >> in super-speed-plus.
> >>
> >> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> >> ---
> >>   Changes in v3:
> >>   - None
> >>   Changes in v2:
> >>   - None
> >>
> >>   include/linux/usb/gadget.h | 20 ++++++++++++++++++++
> >>   1 file changed, 20 insertions(+)
> >>
> >> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> >> index 52ce1f6b8f83..bd982669609c 100644
> >> --- a/include/linux/usb/gadget.h
> >> +++ b/include/linux/usb/gadget.h
> >> @@ -339,6 +339,15 @@ struct usb_gadget_ops {
> >>    * @speed: Speed of current connection to USB host.
> >>    * @max_speed: Maximal speed the UDC can handle.  UDC must support this
> >>    *      and all slower speeds.
> >> + * @num_lanes: Number of lanes in use.
> >> + * @max_num_lanes: Maximum number of lanes the UDC supports.
> >> + * @ssac: Sublink speed attribute count. The number of sublink speed
> >> + *	attributes is ssac + 1.
> >> + * @sublink_speed: Array of sublink speed attributes the UDC supports. Sublink
> >> + *	speed attributes are paired, and an RX followed by a TX attribute.
> >> + * @speed_ssid: Current sublink speed attribute ID in use.
> >> + * @min_speed_ssid: Sublink speed attribute ID with the minimum speed.
> >> + * @max_speed_ssid: Sublink speed attribute ID with the maximum speed.
> >>    * @state: the state we are now (attached, suspended, configured, etc)
> >>    * @name: Identifies the controller hardware type.  Used in diagnostics
> >>    *	and sometimes configuration.
> >> @@ -406,6 +415,17 @@ struct usb_gadget {
> >>   	struct list_head		ep_list;	/* of usb_ep */
> >>   	enum usb_device_speed		speed;
> >>   	enum usb_device_speed		max_speed;
> >> +
> >> +	/* SSP only */
> >> +	unsigned			num_lanes;
> >> +	unsigned			max_num_lanes;
> >> +	unsigned			ssac;
> >> +#define USB_GADGET_MAX_SSAC 3
> >> +	struct usb_sublink_speed	sublink_speed[USB_GADGET_MAX_SSAC + 1];
> >> +	unsigned			speed_ssid;
> >> +	unsigned			min_speed_ssid;
> >> +	unsigned			max_speed_ssid;
> > checkpatch warning:
> >
> > WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
> 
> Yes, but I'd like to keep them consistent with the rest of the fields in 
> this structure.

No, do not do things that you know are wrong and will have to be cleaned
up in the future.  Unless you are trying to increase your patch count
for some reason, this is not ok.

thanks,

greg k-h

Re: [PATCH v3 07/12] usb: common: Add function to get num_lanes and transfer rate

[Thinh Nguyen]

Greg Kroah-Hartman wrote:
> On Fri, Jul 24, 2020 at 04:39:11PM -0700, Thinh Nguyen wrote:
>> Add a new common function to parse maximum_speed property string for
>> the specified number of lanes and transfer rate.
>>
>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>> ---
>> Changes in v3:
>> - Add new function to parse "maximum-speed" for lanes and transfer rate
>> - Remove separate functions getting num_lanes and transfer rate properties
> No, why have you split this into a single function that for some reason
> "can not fail"?

This patch was updated to read from a single property "maximum-speed" to 
get the device speed, gen, and number of lanes. So we use a single 
function to parse this property.

This came up from on Rob's comments:
https://lore.kernel.org/linux-usb/f68f395c-0821-9e34-f6cf-75fa60c9a35b@synopsys.com/T/#mac3a4d0b1c02866e3bdbd6809506fbbee8bf84c2

https://lore.kernel.org/linux-usb/0818e876-ea5b-9ebc-7427-8e26b627e6b4@synopsys.com/T/#m87191333cb10a7f0caaf533bfa198724d33c2519


>
>> Changes in v2:
>> - New commit
>>
>>   drivers/usb/common/common.c | 47 ++++++++++++++++++++++++++++++++++++++++++---
>>   include/linux/usb/ch9.h     | 25 ++++++++++++++++++++++++
>>   2 files changed, 69 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
>> index 1433260d99b4..53b4950c49e4 100644
>> --- a/drivers/usb/common/common.c
>> +++ b/drivers/usb/common/common.c
>> @@ -77,18 +77,59 @@ const char *usb_speed_string(enum usb_device_speed speed)
>>   }
>>   EXPORT_SYMBOL_GPL(usb_speed_string);
>>   
>> -enum usb_device_speed usb_get_maximum_speed(struct device *dev)
>> +void usb_get_maximum_speed_and_num_lanes(struct device *dev,
>> +					 enum usb_device_speed *speed,
>> +					 enum usb_phy_gen *gen,
>> +					 u8 *num_lanes)
> What is wrong with just having multiple different functions instead?

See my comment above.


>
>>   {
>>   	const char *maximum_speed;
>> +	enum usb_device_speed matched_speed = USB_SPEED_UNKNOWN;
>> +	enum usb_phy_gen matched_gen = USB_PHY_GEN_UNKNOWN;
>> +	u8 matched_num_lanes = 0;
>>   	int ret;
>>   
>>   	ret = device_property_read_string(dev, "maximum-speed", &maximum_speed);
>>   	if (ret < 0)
>> -		return USB_SPEED_UNKNOWN;
>> +		goto done;
>>   
>>   	ret = match_string(speed_names, ARRAY_SIZE(speed_names), maximum_speed);
>> +	if (ret >= 0) {
>> +		matched_speed = ret;
>> +		goto done;
>> +	}
>> +
>> +	if (strncmp("super-speed-plus-gen2x2", maximum_speed, 23) == 0) {
>> +		matched_speed = USB_SPEED_SUPER_PLUS;
>> +		matched_gen = USB_PHY_GEN_2;
>> +		matched_num_lanes = 2;
>> +	} else if (strncmp("super-speed-plus-gen2x1", maximum_speed, 23) == 0) {
>> +		matched_speed = USB_SPEED_SUPER_PLUS;
>> +		matched_gen = USB_PHY_GEN_2;
>> +		matched_num_lanes = 1;
>> +	} else if (strncmp("super-speed-plus-gen1x2", maximum_speed, 23) == 0) {
> Where are all of these device properties documented?

It's coming from a single property "maximum-speed", documented in patch 
6/12  "usb: devicetree: Include USB SSP Gen X x Y"

https://lore.kernel.org/linux-usb/3bc4fb6a0d7c1a9ea4d5d9384ade26c9066c87d0.1595631457.git.thinhn@synopsys.com/T/#u

>
>
>
>> +		matched_speed = USB_SPEED_SUPER_PLUS;
>> +		matched_gen = USB_PHY_GEN_1;
>> +		matched_num_lanes = 2;
>> +	}
>> +
>> +done:
>> +	if (speed)
>> +		*speed = matched_speed;
>> +
>> +	if (num_lanes)
>> +		*num_lanes = matched_num_lanes;
>> +
>> +	if (gen)
>> +		*gen = matched_gen;
>
>
>> +}
>> +EXPORT_SYMBOL_GPL(usb_get_maximum_speed_and_num_lanes);
>> +
>> +enum usb_device_speed usb_get_maximum_speed(struct device *dev)
>> +{
>> +	enum usb_device_speed speed;
>>   
>> -	return (ret < 0) ? USB_SPEED_UNKNOWN : ret;
>> +	usb_get_maximum_speed_and_num_lanes(dev, &speed, NULL, NULL);
> Here's an example of why this isn't a good function.
>
> It isn't self-describing.  Why do you pass in 3 pointers yet the name
> only contains 2 things?

Right... I can revise.

>
> usb_get_maximum_speed_and_num_lanes_and_generation() shows that this is
> not a good thing, right?
>
> Again, 3 different functions please.

Should we have 3 separate properties to describe the device? If so, this 
was done in v2 of this series, but Rob has different ideas. Please advise.

>
>> +	return speed;
>>   }
>>   EXPORT_SYMBOL_GPL(usb_get_maximum_speed);
>>   
>> diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h
>> index 01191649a0ad..46cfd72e7082 100644
>> --- a/include/linux/usb/ch9.h
>> +++ b/include/linux/usb/ch9.h
>> @@ -57,6 +57,13 @@ enum usb_link_protocol {
>>   	USB_LP_SSP = 1,
>>   };
>>   
>> +/* USB phy signaling rate gen */
>> +enum usb_phy_gen {
>> +	USB_PHY_GEN_UNKNOWN,
>> +	USB_PHY_GEN_1,
>> +	USB_PHY_GEN_2,
>> +};
>> +
>>   /**
>>    * struct usb_sublink_speed - sublink speed attribute
>>    * @id: sublink speed attribute ID (SSID)
>> @@ -95,6 +102,24 @@ extern const char *usb_ep_type_string(int ep_type);
>>    */
>>   extern const char *usb_speed_string(enum usb_device_speed speed);
>>   
>> +/**
>> + * usb_get_maximum_speed_and_num_lanes - Get maximum requested speed and number
>> + * of lanes for a given USB controller.
> You forgot that it also determines the "gen".

Ok. Will fix.

Thanks!
Thinh

Re: [PATCH v3 03/12] usb: gadget: Expose sublink speed attributes

[Thinh Nguyen]

Greg Kroah-Hartman wrote:
> On Sat, Jul 25, 2020 at 03:33:39AM +0000, Thinh Nguyen wrote:
>> Chunfeng Yun wrote:
>>> On Fri, 2020-07-24 at 16:38 -0700, Thinh Nguyen wrote:
>>>> The USB 3.2 specification supports dual-lane and different transfer
>>>> rates for super-speed-plus. Devices operating in super-speed-plus can
>>>> be gen2x1, gen1x2, or gen2x2.
>>>>
>>>> A gadget driver may need to know the gadget's sublink speeds to properly
>>>> setup its transfer requests and describe its capability in its
>>>> descriptors. To describe the transfer rate in super-speed-plus fully,
>>>> let's expose the lane count and sublink speed attributes when operating
>>>> in super-speed-plus.
>>>>
>>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>>> ---
>>>>    Changes in v3:
>>>>    - None
>>>>    Changes in v2:
>>>>    - None
>>>>
>>>>    include/linux/usb/gadget.h | 20 ++++++++++++++++++++
>>>>    1 file changed, 20 insertions(+)
>>>>
>>>> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
>>>> index 52ce1f6b8f83..bd982669609c 100644
>>>> --- a/include/linux/usb/gadget.h
>>>> +++ b/include/linux/usb/gadget.h
>>>> @@ -339,6 +339,15 @@ struct usb_gadget_ops {
>>>>     * @speed: Speed of current connection to USB host.
>>>>     * @max_speed: Maximal speed the UDC can handle.  UDC must support this
>>>>     *      and all slower speeds.
>>>> + * @num_lanes: Number of lanes in use.
>>>> + * @max_num_lanes: Maximum number of lanes the UDC supports.
>>>> + * @ssac: Sublink speed attribute count. The number of sublink speed
>>>> + *	attributes is ssac + 1.
>>>> + * @sublink_speed: Array of sublink speed attributes the UDC supports. Sublink
>>>> + *	speed attributes are paired, and an RX followed by a TX attribute.
>>>> + * @speed_ssid: Current sublink speed attribute ID in use.
>>>> + * @min_speed_ssid: Sublink speed attribute ID with the minimum speed.
>>>> + * @max_speed_ssid: Sublink speed attribute ID with the maximum speed.
>>>>     * @state: the state we are now (attached, suspended, configured, etc)
>>>>     * @name: Identifies the controller hardware type.  Used in diagnostics
>>>>     *	and sometimes configuration.
>>>> @@ -406,6 +415,17 @@ struct usb_gadget {
>>>>    	struct list_head		ep_list;	/* of usb_ep */
>>>>    	enum usb_device_speed		speed;
>>>>    	enum usb_device_speed		max_speed;
>>>> +
>>>> +	/* SSP only */
>>>> +	unsigned			num_lanes;
>>>> +	unsigned			max_num_lanes;
>>>> +	unsigned			ssac;
>>>> +#define USB_GADGET_MAX_SSAC 3
>>>> +	struct usb_sublink_speed	sublink_speed[USB_GADGET_MAX_SSAC + 1];
>>>> +	unsigned			speed_ssid;
>>>> +	unsigned			min_speed_ssid;
>>>> +	unsigned			max_speed_ssid;
>>> checkpatch warning:
>>>
>>> WARNING:UNSPECIFIED_INT: Prefer 'unsigned int' to bare use of 'unsigned'
>> Yes, but I'd like to keep them consistent with the rest of the fields in
>> this structure.
> No, do not do things that you know are wrong and will have to be cleaned
> up in the future.  Unless you are trying to increase your patch count
> for some reason, this is not ok.
>

Ok. Will fix this.

Thanks,
Thinh

Re: [PATCH v3 07/12] usb: common: Add function to get num_lanes and transfer rate

[Greg Kroah-Hartman]

On Sat, Jul 25, 2020 at 10:51:07AM +0000, Thinh Nguyen wrote:
> Greg Kroah-Hartman wrote:
> > On Fri, Jul 24, 2020 at 04:39:11PM -0700, Thinh Nguyen wrote:
> >> Add a new common function to parse maximum_speed property string for
> >> the specified number of lanes and transfer rate.
> >>
> >> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
> >> ---
> >> Changes in v3:
> >> - Add new function to parse "maximum-speed" for lanes and transfer rate
> >> - Remove separate functions getting num_lanes and transfer rate properties
> > No, why have you split this into a single function that for some reason
> > "can not fail"?
> 
> This patch was updated to read from a single property "maximum-speed" to 
> get the device speed, gen, and number of lanes. So we use a single 
> function to parse this property.
> 
> This came up from on Rob's comments:
> https://lore.kernel.org/linux-usb/f68f395c-0821-9e34-f6cf-75fa60c9a35b@synopsys.com/T/#mac3a4d0b1c02866e3bdbd6809506fbbee8bf84c2
> 
> https://lore.kernel.org/linux-usb/0818e876-ea5b-9ebc-7427-8e26b627e6b4@synopsys.com/T/#m87191333cb10a7f0caaf533bfa198724d33c2519
> 
> 
> >
> >> Changes in v2:
> >> - New commit
> >>
> >>   drivers/usb/common/common.c | 47 ++++++++++++++++++++++++++++++++++++++++++---
> >>   include/linux/usb/ch9.h     | 25 ++++++++++++++++++++++++
> >>   2 files changed, 69 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
> >> index 1433260d99b4..53b4950c49e4 100644
> >> --- a/drivers/usb/common/common.c
> >> +++ b/drivers/usb/common/common.c
> >> @@ -77,18 +77,59 @@ const char *usb_speed_string(enum usb_device_speed speed)
> >>   }
> >>   EXPORT_SYMBOL_GPL(usb_speed_string);
> >>   
> >> -enum usb_device_speed usb_get_maximum_speed(struct device *dev)
> >> +void usb_get_maximum_speed_and_num_lanes(struct device *dev,
> >> +					 enum usb_device_speed *speed,
> >> +					 enum usb_phy_gen *gen,
> >> +					 u8 *num_lanes)
> > What is wrong with just having multiple different functions instead?
> 
> See my comment above.
> 
> 
> >
> >>   {
> >>   	const char *maximum_speed;
> >> +	enum usb_device_speed matched_speed = USB_SPEED_UNKNOWN;
> >> +	enum usb_phy_gen matched_gen = USB_PHY_GEN_UNKNOWN;
> >> +	u8 matched_num_lanes = 0;
> >>   	int ret;
> >>   
> >>   	ret = device_property_read_string(dev, "maximum-speed", &maximum_speed);
> >>   	if (ret < 0)
> >> -		return USB_SPEED_UNKNOWN;
> >> +		goto done;
> >>   
> >>   	ret = match_string(speed_names, ARRAY_SIZE(speed_names), maximum_speed);
> >> +	if (ret >= 0) {
> >> +		matched_speed = ret;
> >> +		goto done;
> >> +	}
> >> +
> >> +	if (strncmp("super-speed-plus-gen2x2", maximum_speed, 23) == 0) {
> >> +		matched_speed = USB_SPEED_SUPER_PLUS;
> >> +		matched_gen = USB_PHY_GEN_2;
> >> +		matched_num_lanes = 2;
> >> +	} else if (strncmp("super-speed-plus-gen2x1", maximum_speed, 23) == 0) {
> >> +		matched_speed = USB_SPEED_SUPER_PLUS;
> >> +		matched_gen = USB_PHY_GEN_2;
> >> +		matched_num_lanes = 1;
> >> +	} else if (strncmp("super-speed-plus-gen1x2", maximum_speed, 23) == 0) {
> > Where are all of these device properties documented?
> 
> It's coming from a single property "maximum-speed", documented in patch 
> 6/12  "usb: devicetree: Include USB SSP Gen X x Y"
> 
> https://lore.kernel.org/linux-usb/3bc4fb6a0d7c1a9ea4d5d9384ade26c9066c87d0.1595631457.git.thinhn@synopsys.com/T/#u
> 
> >
> >
> >
> >> +		matched_speed = USB_SPEED_SUPER_PLUS;
> >> +		matched_gen = USB_PHY_GEN_1;
> >> +		matched_num_lanes = 2;
> >> +	}
> >> +
> >> +done:
> >> +	if (speed)
> >> +		*speed = matched_speed;
> >> +
> >> +	if (num_lanes)
> >> +		*num_lanes = matched_num_lanes;
> >> +
> >> +	if (gen)
> >> +		*gen = matched_gen;
> >
> >
> >> +}
> >> +EXPORT_SYMBOL_GPL(usb_get_maximum_speed_and_num_lanes);
> >> +
> >> +enum usb_device_speed usb_get_maximum_speed(struct device *dev)
> >> +{
> >> +	enum usb_device_speed speed;
> >>   
> >> -	return (ret < 0) ? USB_SPEED_UNKNOWN : ret;
> >> +	usb_get_maximum_speed_and_num_lanes(dev, &speed, NULL, NULL);
> > Here's an example of why this isn't a good function.
> >
> > It isn't self-describing.  Why do you pass in 3 pointers yet the name
> > only contains 2 things?
> 
> Right... I can revise.
> 
> >
> > usb_get_maximum_speed_and_num_lanes_and_generation() shows that this is
> > not a good thing, right?
> >
> > Again, 3 different functions please.
> 
> Should we have 3 separate properties to describe the device? If so, this 
> was done in v2 of this series, but Rob has different ideas. Please advise.

I don't care about the properties, that should be independant of the
function call made to determine this information.  Don't let the data
formats force you to write horrible code :)

thanks,

greg k-h

Re: [PATCH v3 07/12] usb: common: Add function to get num_lanes and transfer rate

[Thinh Nguyen]

Greg Kroah-Hartman wrote:
> On Sat, Jul 25, 2020 at 10:51:07AM +0000, Thinh Nguyen wrote:
>> Greg Kroah-Hartman wrote:
>>> On Fri, Jul 24, 2020 at 04:39:11PM -0700, Thinh Nguyen wrote:
>>>> Add a new common function to parse maximum_speed property string for
>>>> the specified number of lanes and transfer rate.
>>>>
>>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>>>> ---
>>>> Changes in v3:
>>>> - Add new function to parse "maximum-speed" for lanes and transfer rate
>>>> - Remove separate functions getting num_lanes and transfer rate properties
>>> No, why have you split this into a single function that for some reason
>>> "can not fail"?
>> This patch was updated to read from a single property "maximum-speed" to
>> get the device speed, gen, and number of lanes. So we use a single
>> function to parse this property.
>>
>> This came up from on Rob's comments:
>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/f68f395c-0821-9e34-f6cf-75fa60c9a35b@synopsys.com/T/*mac3a4d0b1c02866e3bdbd6809506fbbee8bf84c2__;Iw!!A4F2R9G_pg!JvGMuHeacSdTRkN-1SCUsebqfPskVCdtFj6xUkMuvtkwBcGK5N4v5nukhHZKI-lKdFEE$
>>
>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/0818e876-ea5b-9ebc-7427-8e26b627e6b4@synopsys.com/T/*m87191333cb10a7f0caaf533bfa198724d33c2519__;Iw!!A4F2R9G_pg!JvGMuHeacSdTRkN-1SCUsebqfPskVCdtFj6xUkMuvtkwBcGK5N4v5nukhHZKI0SFG1Fk$
>>
>>
>>>> Changes in v2:
>>>> - New commit
>>>>
>>>>    drivers/usb/common/common.c | 47 ++++++++++++++++++++++++++++++++++++++++++---
>>>>    include/linux/usb/ch9.h     | 25 ++++++++++++++++++++++++
>>>>    2 files changed, 69 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
>>>> index 1433260d99b4..53b4950c49e4 100644
>>>> --- a/drivers/usb/common/common.c
>>>> +++ b/drivers/usb/common/common.c
>>>> @@ -77,18 +77,59 @@ const char *usb_speed_string(enum usb_device_speed speed)
>>>>    }
>>>>    EXPORT_SYMBOL_GPL(usb_speed_string);
>>>>    
>>>> -enum usb_device_speed usb_get_maximum_speed(struct device *dev)
>>>> +void usb_get_maximum_speed_and_num_lanes(struct device *dev,
>>>> +					 enum usb_device_speed *speed,
>>>> +					 enum usb_phy_gen *gen,
>>>> +					 u8 *num_lanes)
>>> What is wrong with just having multiple different functions instead?
>> See my comment above.
>>
>>
>>>>    {
>>>>    	const char *maximum_speed;
>>>> +	enum usb_device_speed matched_speed = USB_SPEED_UNKNOWN;
>>>> +	enum usb_phy_gen matched_gen = USB_PHY_GEN_UNKNOWN;
>>>> +	u8 matched_num_lanes = 0;
>>>>    	int ret;
>>>>    
>>>>    	ret = device_property_read_string(dev, "maximum-speed", &maximum_speed);
>>>>    	if (ret < 0)
>>>> -		return USB_SPEED_UNKNOWN;
>>>> +		goto done;
>>>>    
>>>>    	ret = match_string(speed_names, ARRAY_SIZE(speed_names), maximum_speed);
>>>> +	if (ret >= 0) {
>>>> +		matched_speed = ret;
>>>> +		goto done;
>>>> +	}
>>>> +
>>>> +	if (strncmp("super-speed-plus-gen2x2", maximum_speed, 23) == 0) {
>>>> +		matched_speed = USB_SPEED_SUPER_PLUS;
>>>> +		matched_gen = USB_PHY_GEN_2;
>>>> +		matched_num_lanes = 2;
>>>> +	} else if (strncmp("super-speed-plus-gen2x1", maximum_speed, 23) == 0) {
>>>> +		matched_speed = USB_SPEED_SUPER_PLUS;
>>>> +		matched_gen = USB_PHY_GEN_2;
>>>> +		matched_num_lanes = 1;
>>>> +	} else if (strncmp("super-speed-plus-gen1x2", maximum_speed, 23) == 0) {
>>> Where are all of these device properties documented?
>> It's coming from a single property "maximum-speed", documented in patch
>> 6/12  "usb: devicetree: Include USB SSP Gen X x Y"
>>
>> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/3bc4fb6a0d7c1a9ea4d5d9384ade26c9066c87d0.1595631457.git.thinhn@synopsys.com/T/*u__;Iw!!A4F2R9G_pg!JvGMuHeacSdTRkN-1SCUsebqfPskVCdtFj6xUkMuvtkwBcGK5N4v5nukhHZKI6d-EKTE$
>>
>>>
>>>
>>>> +		matched_speed = USB_SPEED_SUPER_PLUS;
>>>> +		matched_gen = USB_PHY_GEN_1;
>>>> +		matched_num_lanes = 2;
>>>> +	}
>>>> +
>>>> +done:
>>>> +	if (speed)
>>>> +		*speed = matched_speed;
>>>> +
>>>> +	if (num_lanes)
>>>> +		*num_lanes = matched_num_lanes;
>>>> +
>>>> +	if (gen)
>>>> +		*gen = matched_gen;
>>>
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(usb_get_maximum_speed_and_num_lanes);
>>>> +
>>>> +enum usb_device_speed usb_get_maximum_speed(struct device *dev)
>>>> +{
>>>> +	enum usb_device_speed speed;
>>>>    
>>>> -	return (ret < 0) ? USB_SPEED_UNKNOWN : ret;
>>>> +	usb_get_maximum_speed_and_num_lanes(dev, &speed, NULL, NULL);
>>> Here's an example of why this isn't a good function.
>>>
>>> It isn't self-describing.  Why do you pass in 3 pointers yet the name
>>> only contains 2 things?
>> Right... I can revise.
>>
>>> usb_get_maximum_speed_and_num_lanes_and_generation() shows that this is
>>> not a good thing, right?
>>>
>>> Again, 3 different functions please.
>> Should we have 3 separate properties to describe the device? If so, this
>> was done in v2 of this series, but Rob has different ideas. Please advise.
> I don't care about the properties, that should be independant of the
> function call made to determine this information.  Don't let the data
> formats force you to write horrible code :)

Ok. Will revise.

Thanks,
Thinh