usb: Handle different sublink speeds

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 <[email protected]>

---
Changes in v3:
- None
Changes in v2:
- None

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

include/linux/usb/gadget.h +9 -0

index 52ce1f6b8f83..bd982669609c 100644
@@ -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;

include/linux/usb/gadget.h +1 -0

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

Quoted 44 lines · 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 <[email protected]>

---
Changes in v3:
- None
Changes in v2:
- None

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

include/linux/usb/gadget.h +9 -0

index 52ce1f6b8f83..bd982669609c 100644
@@ -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;

include/linux/usb/gadget.h struct usb_gadget { · line 427

Quoted 10 lines

include/linux/usb/gadget.h +10 -0

+
+	/* 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'

include/linux/usb/gadget.h struct usb_gadget { · line 431

Quoted 4 lines

include/linux/usb/gadget.h +1 -0

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

Quoted 57 lines · Chunfeng Yun

Quoted 44 lines · 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 <[email protected]>

---
Changes in v3:
- None
Changes in v2:
- None

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

include/linux/usb/gadget.h +9 -0

index 52ce1f6b8f83..bd982669609c 100644
@@ -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;

include/linux/usb/gadget.h struct usb_gadget { · line 427

Quoted 10 lines

include/linux/usb/gadget.h +10 -0

+
+	/* 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:

Quoted 1 line

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.

Quoted 58 lines · Thinh Nguyen

Quoted 57 lines · Chunfeng Yun

Quoted 44 lines · 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 <[email protected]>

---
Changes in v3:
- None
Changes in v2:
- None

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

include/linux/usb/gadget.h +9 -0

index 52ce1f6b8f83..bd982669609c 100644
@@ -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;

include/linux/usb/gadget.h struct usb_gadget { · line 427

Quoted 10 lines

include/linux/usb/gadget.h +10 -0

+
+	/* 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:

Quoted 3 lines

Quoted 1 line

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.

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 <[email protected]>

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

drivers/usb/common/common.c +44 -3

index 1433260d99b4..53b4950c49e4 100644
@@ -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);
 

include/linux/usb/ch9.h +0 -0

index 01191649a0ad..46cfd72e7082 100644

include/linux/usb/ch9.h +19 -0

+
 /**
  * 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.

Quoted 90 lines · 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 <[email protected]>

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

drivers/usb/common/common.c +44 -3

index 1433260d99b4..53b4950c49e4 100644
@@ -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);
 

include/linux/usb/ch9.h +0 -0

index 01191649a0ad..46cfd72e7082 100644
@@ -57,6 +57,13 @@ enum usb_link_protocol {
  	USB_LP_SSP = 1,
  };
 

include/linux/usb/ch9.h enum usb_link_protocol { · line 65

Quoted 6 lines

include/linux/usb/ch9.h +6 -0

+/* 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.

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 <[email protected]>

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

drivers/usb/common/common.c +0 -0

index 1433260d99b4..53b4950c49e4 100644

drivers/usb/common/common.c +8 -1

 {
 	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;
+	}

drivers/usb/common/common.c +14 -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;
 
 

drivers/usb/common/common.c +1 -0

+	return speed;

}
EXPORT_SYMBOL_GPL(usb_get_maximum_speed);

include/linux/usb/ch9.h +7 -0

index 01191649a0ad..46cfd72e7082 100644
@@ -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)

Quoted 8 lines · 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 <[email protected]>

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

Quoted 16 lines

Changes in v2:
- New commit

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

drivers/usb/common/common.c +0 -0

index 1433260d99b4..53b4950c49e4 100644
@@ -77,18 +77,59 @@ const char *usb_speed_string(enum usb_device_speed speed)
  }
  EXPORT_SYMBOL_GPL(usb_speed_string);
 

drivers/usb/common/common.c const char *usb_speed_string(enum usb_device_speed speed) · line 83

Quoted 5 lines

drivers/usb/common/common.c +4 -1

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

Quoted 13 lines

drivers/usb/common/common.c +4 -1

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

Quoted 5 lines

drivers/usb/common/common.c +4 -0

  	ret = match_string(speed_names, ARRAY_SIZE(speed_names), maximum_speed);
+	if (ret >= 0) {
+		matched_speed = ret;
+		goto done;
+	}

drivers/usb/common/common.c const char *usb_speed_string(enum usb_device_speed speed) · line 109

Quoted 10 lines

drivers/usb/common/common.c +10 -0

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

Quoted 26 lines

drivers/usb/common/common.c +20 -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;
 

drivers/usb/common/common.c const char *usb_speed_string(enum usb_device_speed speed) · line 131

Quoted 2 lines

drivers/usb/common/common.c +1 -1

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

Quoted 27 lines

drivers/usb/common/common.c +1 -0

+	return speed;
  }

  EXPORT_SYMBOL_GPL(usb_get_maximum_speed);

include/linux/usb/ch9.h +7 -0

index 01191649a0ad..46cfd72e7082 100644
@@ -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);
 

include/linux/usb/ch9.h extern const char *usb_ep_type_string(int ep_type); · line 107

Quoted 3 lines

include/linux/usb/ch9.h +3 -0

+/**
+ * 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".

Quoted 11 lines · Greg Kroah-Hartman

Quoted 8 lines · 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 <[email protected]>

---
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/[email protected]/T/#mac3a4d0b1c02866e3bdbd6809506fbbee8bf84c2

https://lore.kernel.org/linux-usb/[email protected]/T/#m87191333cb10a7f0caaf533bfa198724d33c2519

Quoted 16 lines

Quoted 16 lines

Changes in v2:
- New commit

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

drivers/usb/common/common.c +0 -0

index 1433260d99b4..53b4950c49e4 100644
@@ -77,18 +77,59 @@ const char *usb_speed_string(enum usb_device_speed speed)
  }
  EXPORT_SYMBOL_GPL(usb_speed_string);
 

Quoted 6 lines

drivers/usb/common/common.c const char *usb_speed_string(enum usb_device_speed speed) · line 83

Quoted 5 lines

drivers/usb/common/common.c +4 -1

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

Quoted 13 lines

Quoted 13 lines

drivers/usb/common/common.c +4 -1

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

Quoted 16 lines

Quoted 5 lines

drivers/usb/common/common.c +4 -0

  	ret = match_string(speed_names, ARRAY_SIZE(speed_names), maximum_speed);
+	if (ret >= 0) {
+		matched_speed = ret;
+		goto done;
+	}

drivers/usb/common/common.c const char *usb_speed_string(enum usb_device_speed speed) · line 109

Quoted 10 lines

drivers/usb/common/common.c +10 -0

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

Quoted 26 lines

Quoted 26 lines

drivers/usb/common/common.c +20 -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;
 

Quoted 6 lines

drivers/usb/common/common.c const char *usb_speed_string(enum usb_device_speed speed) · line 131

Quoted 2 lines

drivers/usb/common/common.c +1 -1

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

Quoted 1 line

Quoted 4 lines

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.

Quoted 114 lines · Thinh Nguyen

Quoted 11 lines · Greg Kroah-Hartman

Quoted 8 lines · 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 <[email protected]>

---
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/[email protected]/T/*mac3a4d0b1c02866e3bdbd6809506fbbee8bf84c2__;Iw!!A4F2R9G_pg!JvGMuHeacSdTRkN-1SCUsebqfPskVCdtFj6xUkMuvtkwBcGK5N4v5nukhHZKI-lKdFEE$

https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/[email protected]/T/*m87191333cb10a7f0caaf533bfa198724d33c2519__;Iw!!A4F2R9G_pg!JvGMuHeacSdTRkN-1SCUsebqfPskVCdtFj6xUkMuvtkwBcGK5N4v5nukhHZKI0SFG1Fk$

Quoted 15 lines

Quoted 15 lines

Changes in v2:
- New commit

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

drivers/usb/common/common.c +0 -0

index 1433260d99b4..53b4950c49e4 100644
@@ -77,18 +77,59 @@ const char *usb_speed_string(enum usb_device_speed speed)
   }
   EXPORT_SYMBOL_GPL(usb_speed_string);
 

Quoted 6 lines

drivers/usb/common/common.c const char *usb_speed_string(enum usb_device_speed speed) · line 83

Quoted 5 lines

drivers/usb/common/common.c +4 -1

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

Quoted 12 lines

Quoted 12 lines

drivers/usb/common/common.c +4 -1

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

Quoted 16 lines

Quoted 5 lines

drivers/usb/common/common.c +4 -0

   	ret = match_string(speed_names, ARRAY_SIZE(speed_names), maximum_speed);
+	if (ret >= 0) {
+		matched_speed = ret;
+		goto done;
+	}

drivers/usb/common/common.c const char *usb_speed_string(enum usb_device_speed speed) · line 109

Quoted 10 lines

drivers/usb/common/common.c +10 -0

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

Quoted 24 lines

Quoted 24 lines

drivers/usb/common/common.c +20 -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;
 

Quoted 6 lines

drivers/usb/common/common.c const char *usb_speed_string(enum usb_device_speed speed) · line 131

Quoted 2 lines

drivers/usb/common/common.c +1 -1

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

Quoted 3 lines

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

Quoted 3 lines

Quoted 1 line

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