[RFC PATCH 0/8] usb: Check for genXxY on host

[RFC PATCH 0/8] usb: Check for genXxY on host

[Thinh Nguyen]

This series add some missing support for USB 3.2 SuperSpeed Plus detection on
the host side. A SuperSpeed Plus device can operate in gen2x2, gen2x1, or
gen1x2. The current implementation can't detect whether the device is in Gen 1
or Gen 2 speed. We can do this by matching for the lane speed exponent and
mantissa of the SSP sublink speed capability descriptor from the hub driver.

Also, the current xHCI driver is missing some reports for the default SSP
Sublink Speed capability for USB 3.2 roothub. This series also add some support
for xHCI driver detecting various SuperSpeed Plus GenXxY.


Thinh Nguyen (8):
  usb: core: Track SuperSpeed Plus GenXxY
  usb: core: hub: Remove port_speed_is_ssp()
  usb: core: hub: Print speed name based on ssp rate
  usb: core: sysfs: Check for SSP rate in speed attr
  usb: xhci: Init root hub SSP rate
  usb: xhci: Fix port minor revision
  usb: xhci: Rewrite xhci_create_usb3_bos_desc()
  usb: xhci: Remove unused function

 drivers/usb/core/hcd.c      |   6 +-
 drivers/usb/core/hub.c      |  97 +++++++---
 drivers/usb/core/sysfs.c    |   5 +-
 drivers/usb/host/xhci-hub.c | 368 ++++++++++++++++++++++++------------
 drivers/usb/host/xhci-mem.c |   9 +
 drivers/usb/host/xhci.c     |   2 +
 include/linux/usb.h         |   2 +
 7 files changed, 345 insertions(+), 144 deletions(-)


base-commit: 29b01295a829fba7399ee84afff4e64660e49f04
-- 
2.28.0

[RFC PATCH 1/8] usb: core: Track SuperSpeed Plus GenXxY

[Thinh Nguyen]

Introduce ssp_rate field to usb_device structure to capture the
connected SuperSpeed Plus signaling rate generation and lane count with
the corresponding usb_ssp_rate enum.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/core/hcd.c |  6 +++-
 drivers/usb/core/hub.c | 78 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb.h    |  2 ++
 3 files changed, 85 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index ad5a0f405a75..55de04df3022 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2721,6 +2721,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
 
 	rhdev->rx_lanes = 1;
 	rhdev->tx_lanes = 1;
+	rhdev->ssp_rate = USB_SSP_GEN_UNKNOWN;
 
 	switch (hcd->speed) {
 	case HCD_USB11:
@@ -2738,8 +2739,11 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	case HCD_USB32:
 		rhdev->rx_lanes = 2;
 		rhdev->tx_lanes = 2;
-		fallthrough;
+		rhdev->ssp_rate = USB_SSP_GEN_2x2;
+		rhdev->speed = USB_SPEED_SUPER_PLUS;
+		break;
 	case HCD_USB31:
+		rhdev->ssp_rate = USB_SSP_GEN_2x1;
 		rhdev->speed = USB_SPEED_SUPER_PLUS;
 		break;
 	default:
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 7f71218cc1e5..e78b2dd7801a 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -31,6 +31,7 @@
 #include <linux/pm_qos.h>
 #include <linux/kobject.h>
 
+#include <linux/bitfield.h>
 #include <linux/uaccess.h>
 #include <asm/byteorder.h>
 
@@ -2668,6 +2669,81 @@ int usb_authorize_device(struct usb_device *usb_dev)
 	return result;
 }
 
+/**
+ * get_port_ssp_rate - Match the extended port status to SSP rate
+ * @hdev: The hub device
+ * @ext_portstatus: extended port status
+ *
+ * Match the extended port status speed id to the SuperSpeed Plus sublink speed
+ * capability attributes. Base on the number of connected lanes and speed,
+ * return the corresponding enum usb_ssp_rate.
+ */
+static enum usb_ssp_rate get_port_ssp_rate(struct usb_device *hdev,
+					   u32 ext_portstatus)
+{
+	struct usb_ssp_cap_descriptor *ssp_cap = hdev->bos->ssp_cap;
+	u32 attr;
+	u8 speed_id;
+	u8 ssac;
+	u8 lanes;
+	int i;
+
+	if (!ssp_cap)
+		goto out;
+
+	speed_id = ext_portstatus & USB_EXT_PORT_STAT_RX_SPEED_ID;
+	lanes = USB_EXT_PORT_RX_LANES(ext_portstatus) + 1;
+
+	ssac = le32_to_cpu(ssp_cap->bmAttributes) &
+		USB_SSP_SUBLINK_SPEED_ATTRIBS;
+
+	for (i = 0; i <= ssac; i++) {
+		u8 ssid;
+
+		attr = le32_to_cpu(ssp_cap->bmSublinkSpeedAttr[i]);
+		ssid = FIELD_GET(USB_SSP_SUBLINK_SPEED_SSID, attr);
+		if (speed_id == ssid) {
+			u16 mantissa;
+			u8 lse;
+			u8 type;
+
+			/*
+			 * Note: currently asymmetric lane types are only
+			 * applicable for SSIC operate in SuperSpeed protocol
+			 */
+			type = FIELD_GET(USB_SSP_SUBLINK_SPEED_ST, attr);
+			if (type == USB_SSP_SUBLINK_SPEED_ST_ASYM_RX ||
+			    type == USB_SSP_SUBLINK_SPEED_ST_ASYM_TX)
+				goto out;
+
+			if (FIELD_GET(USB_SSP_SUBLINK_SPEED_LP, attr) !=
+			    USB_SSP_SUBLINK_SPEED_LP_SSP)
+				goto out;
+
+			lse = FIELD_GET(USB_SSP_SUBLINK_SPEED_LSE, attr);
+			mantissa = FIELD_GET(USB_SSP_SUBLINK_SPEED_LSM, attr);
+
+			/* Convert to Gbps */
+			for (; lse < USB_SSP_SUBLINK_SPEED_LSE_GBPS; lse++)
+				mantissa /= 1000;
+
+			if (mantissa >= 10 && lanes == 1)
+				return USB_SSP_GEN_2x1;
+
+			if (mantissa >= 10 && lanes == 2)
+				return USB_SSP_GEN_2x2;
+
+			if (mantissa >= 5 && lanes == 2)
+				return USB_SSP_GEN_1x2;
+
+			goto out;
+		}
+	}
+
+out:
+	return USB_SSP_GEN_UNKNOWN;
+}
+
 /*
  * Return 1 if port speed is SuperSpeedPlus, 0 otherwise
  * check it from the link protocol field of the current speed ID attribute.
@@ -2850,9 +2926,11 @@ static int hub_port_wait_reset(struct usb_hub *hub, int port1,
 		/* extended portstatus Rx and Tx lane count are zero based */
 		udev->rx_lanes = USB_EXT_PORT_RX_LANES(ext_portstatus) + 1;
 		udev->tx_lanes = USB_EXT_PORT_TX_LANES(ext_portstatus) + 1;
+		udev->ssp_rate = get_port_ssp_rate(hub->hdev, ext_portstatus);
 	} else {
 		udev->rx_lanes = 1;
 		udev->tx_lanes = 1;
+		udev->ssp_rate = USB_SSP_GEN_UNKNOWN;
 	}
 	if (hub_is_wusb(hub))
 		udev->speed = USB_SPEED_WIRELESS;
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 7d72c4e0713c..c334387f950e 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -560,6 +560,7 @@ struct usb3_lpm_parameters {
  * @speed: device speed: high/full/low (or error)
  * @rx_lanes: number of rx lanes in use, USB 3.2 adds dual-lane support
  * @tx_lanes: number of tx lanes in use, USB 3.2 adds dual-lane support
+ * @ssp_rate: SuperSpeed Plus phy signaling rate and lane count
  * @tt: Transaction Translator info; used with low/full speed dev, highspeed hub
  * @ttport: device port on that tt hub
  * @toggle: one bit for each endpoint, with ([0] = IN, [1] = OUT) endpoints
@@ -636,6 +637,7 @@ struct usb_device {
 	enum usb_device_speed	speed;
 	unsigned int		rx_lanes;
 	unsigned int		tx_lanes;
+	enum usb_ssp_rate	ssp_rate;
 
 	struct usb_tt	*tt;
 	int		ttport;
-- 
2.28.0

[RFC PATCH 2/8] usb: core: hub: Remove port_speed_is_ssp()

[Thinh Nguyen]

The get_port_ssp_rate() can replace port_speed_is_ssp(). If the port
speed is detected to be in gen2x1, gen1x2, or gen2x2, then the port is
operating at SuperSpeed Plus.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/core/hub.c | 31 +------------------------------
 1 file changed, 1 insertion(+), 30 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index e78b2dd7801a..823470607d58 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2744,33 +2744,6 @@ static enum usb_ssp_rate get_port_ssp_rate(struct usb_device *hdev,
 	return USB_SSP_GEN_UNKNOWN;
 }
 
-/*
- * Return 1 if port speed is SuperSpeedPlus, 0 otherwise
- * check it from the link protocol field of the current speed ID attribute.
- * current speed ID is got from ext port status request. Sublink speed attribute
- * table is returned with the hub BOS SSP device capability descriptor
- */
-static int port_speed_is_ssp(struct usb_device *hdev, int speed_id)
-{
-	int ssa_count;
-	u32 ss_attr;
-	int i;
-	struct usb_ssp_cap_descriptor *ssp_cap = hdev->bos->ssp_cap;
-
-	if (!ssp_cap)
-		return 0;
-
-	ssa_count = le32_to_cpu(ssp_cap->bmAttributes) &
-		USB_SSP_SUBLINK_SPEED_ATTRIBS;
-
-	for (i = 0; i <= ssa_count; i++) {
-		ss_attr = le32_to_cpu(ssp_cap->bmSublinkSpeedAttr[i]);
-		if (speed_id == (ss_attr & USB_SSP_SUBLINK_SPEED_SSID))
-			return !!(ss_attr & USB_SSP_SUBLINK_SPEED_LP);
-	}
-	return 0;
-}
-
 /* Returns 1 if @hub is a WUSB root hub, 0 otherwise */
 static unsigned hub_is_wusb(struct usb_hub *hub)
 {
@@ -2934,9 +2907,7 @@ static int hub_port_wait_reset(struct usb_hub *hub, int port1,
 	}
 	if (hub_is_wusb(hub))
 		udev->speed = USB_SPEED_WIRELESS;
-	else if (hub_is_superspeedplus(hub->hdev) &&
-		 port_speed_is_ssp(hub->hdev, ext_portstatus &
-				   USB_EXT_PORT_STAT_RX_SPEED_ID))
+	else if (udev->ssp_rate != USB_SSP_GEN_UNKNOWN)
 		udev->speed = USB_SPEED_SUPER_PLUS;
 	else if (hub_is_superspeed(hub->hdev))
 		udev->speed = USB_SPEED_SUPER;
-- 
2.28.0

[RFC PATCH 3/8] usb: core: hub: Print speed name based on ssp rate

[Thinh Nguyen]

Check for usb_device->ssp_rate to print the SuperSpeed Plus signaling
rate generation and lane count.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/core/hub.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 823470607d58..6dc4e7385e75 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -4830,9 +4830,13 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
 						"%s SuperSpeed%s%s USB device number %d using %s\n",
 						(udev->config) ? "reset" : "new",
 					 (udev->speed == USB_SPEED_SUPER_PLUS) ?
-							"Plus Gen 2" : " Gen 1",
-					 (udev->rx_lanes == 2 && udev->tx_lanes == 2) ?
-							"x2" : "",
+							" Plus" : "",
+					 (udev->ssp_rate == USB_SSP_GEN_2x2) ?
+							" gen2x2" :
+					 (udev->ssp_rate == USB_SSP_GEN_2x1) ?
+							" gen2x1" :
+					 (udev->ssp_rate == USB_SSP_GEN_1x2) ?
+							" gen1x2" : "",
 					 devnum, driver_name);
 			}
 
-- 
2.28.0

[RFC PATCH 4/8] usb: core: sysfs: Check for SSP rate in speed attr

[Thinh Nguyen]

Check for usb_device->ssp_rate to output the signaling rate for genXxY.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/core/sysfs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/sysfs.c b/drivers/usb/core/sysfs.c
index d85699bee671..5a168ba9fc51 100644
--- a/drivers/usb/core/sysfs.c
+++ b/drivers/usb/core/sysfs.c
@@ -167,7 +167,10 @@ static ssize_t speed_show(struct device *dev, struct device_attribute *attr,
 		speed = "5000";
 		break;
 	case USB_SPEED_SUPER_PLUS:
-		speed = "10000";
+		if (udev->ssp_rate == USB_SSP_GEN_2x2)
+			speed = "20000";
+		else
+			speed = "10000";
 		break;
 	default:
 		speed = "unknown";
-- 
2.28.0

[RFC PATCH 5/8] usb: xhci: Init root hub SSP rate

[Thinh Nguyen]

Initialize USB 3.x root hub SuperSpeed Plus rate.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/host/xhci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index f232dc9c172d..7da03a1a5084 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -5226,10 +5226,12 @@ int xhci_gen_setup(struct usb_hcd *hcd, xhci_get_quirks_t get_quirks)
 			hcd->self.root_hub->speed = USB_SPEED_SUPER_PLUS;
 			hcd->self.root_hub->rx_lanes = 2;
 			hcd->self.root_hub->tx_lanes = 2;
+			hcd->self.root_hub->ssp_rate = USB_SSP_GEN_2x2;
 			break;
 		case 1:
 			hcd->speed = HCD_USB31;
 			hcd->self.root_hub->speed = USB_SPEED_SUPER_PLUS;
+			hcd->self.root_hub->ssp_rate = USB_SSP_GEN_2x1;
 			break;
 		}
 		xhci_info(xhci, "Host supports USB 3.%x %sSuperSpeed\n",
-- 
2.28.0

[RFC PATCH 6/8] usb: xhci: Fix port minor revision

[Thinh Nguyen]

Some hosts incorrectly use sub-minor version for minor version (i.e.
0x02 instead of 0x20 for bcdUSB 0x320 and 0x01 for bcdUSB 0x310).
Currently the xHCI driver works around this by just checking for minor
revision > 0x01 for USB 3.1 everywhere. With the addition of USB 3.2,
checking this gets a bit cumbersome. Since there is no USB release with
bcdUSB 0x301 to 0x309, we can assume that sub-minor version 01 to 09 is
incorrect. Let's try to fix this and use the minor revision that matches
with the USB/xHCI spec to help with the version checking within the
driver.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/host/xhci-mem.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index f2c4ee7c4786..34105b477c62 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -2129,6 +2129,15 @@ static void xhci_add_in_port(struct xhci_hcd *xhci, unsigned int num_ports,
 
 	if (major_revision == 0x03) {
 		rhub = &xhci->usb3_rhub;
+		/*
+		 * Some hosts incorrectly use sub-minor version for minor
+		 * version (i.e. 0x02 instead of 0x20 for bcdUSB 0x320 and 0x01
+		 * for bcdUSB 0x310). Since there is no USB release with sub
+		 * minor version 0x301 to 0x309, we can assume that they are
+		 * incorrect and fix it here.
+		 */
+		if (!(minor_revision & 0xf0) && (minor_revision & 0x0f))
+			minor_revision = minor_revision << 4;
 	} else if (major_revision <= 0x02) {
 		rhub = &xhci->usb2_rhub;
 	} else {
-- 
2.28.0

[RFC PATCH 7/8] usb: xhci: Rewrite xhci_create_usb3_bos_desc()

[Thinh Nguyen]

The current xhci_create_usb3_bos_desc() uses a static bos structure and
various magic numbers and offset making it difficult to extend support
for USB 3.2. Let's rewrite this entire function to support dual-lane in
USB 3.2.

The hub driver matches the port speed ID from the extended port status
to the SSID of the sublink speed attributes to detect if the device
supports SuperSpeed Plus. Currently we don't provide the default gen1x2
and gen2x2 sublink speed capability descriptor for USB 3.2 roothub. The
USB stack depends on this to detect and match the correct speed.
In addition, if the xHCI host provides Protocol Speed ID (PSI)
capability, then make sure to convert Protocol Speed ID Mantissa and
Exponent (PSIM & PSIE) to lane speed for gen1x2 and gen2x2.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/host/xhci-hub.c | 279 +++++++++++++++++++++++++++++++++++-
 1 file changed, 277 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 74c497fd3476..c095c30212e5 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -11,6 +11,7 @@
 
 #include <linux/slab.h>
 #include <asm/unaligned.h>
+#include <linux/bitfield.h>
 
 #include "xhci.h"
 #include "xhci-trace.h"
@@ -52,7 +53,281 @@ static u8 usb_bos_descriptor [] = {
 	0xb5, 0x40, 0x0a, 0x00,		/* 10Gbps, SSP, symmetric, tx, ID = 5 */
 };
 
-static int xhci_create_usb3_bos_desc(struct xhci_hcd *xhci, char *buf,
+static int xhci_fill_default_ssp_attr(struct xhci_hcd *xhci,
+		struct usb_ssp_cap_descriptor *ssp_cap)
+{
+	u32 attr;
+	u8 ssac;
+	int i;
+
+	attr = le32_to_cpu(ssp_cap->bmAttributes);
+	ssac = FIELD_GET(USB_SSP_SUBLINK_SPEED_ATTRIBS, attr);
+
+	/* Currently we only support USB 3.1 and 3.2 */
+	if (ssac != 3 && ssac != 7)
+		return -EINVAL;
+
+	/*
+	 * Map default xHCI port speed ID to SSID:
+	 *  SSID 4 = Symmetric SSP Gen1x1
+	 *  SSID 5 = Symmetric SSP Gen2x1
+	 *  SSID 6 = Symmetric SSP Gen1x2
+	 *  SSID 7 = Symmetric SSP Gen2x2
+	 */
+	for (i = 0; i < ssac + 1; i++) {
+		u8 ssid;
+		u8 type;
+		u8 lp;
+		u16 mantissa;
+
+		ssid = (i >> 1) + 4;
+
+		if (ssid > 4)
+			lp = USB_SSP_SUBLINK_SPEED_LP_SSP;
+		else
+			lp = USB_SSP_SUBLINK_SPEED_LP_SS;
+
+		if (ssid == 5 || ssid == 7)
+			mantissa = 10;
+		else
+			mantissa = 5;
+
+		if (i % 2)
+			type = USB_SSP_SUBLINK_SPEED_ST_SYM_TX;
+		else
+			type = USB_SSP_SUBLINK_SPEED_ST_SYM_RX;
+
+		ssp_cap->bmSublinkSpeedAttr[i] =
+			cpu_to_le32(FIELD_PREP(USB_SSP_SUBLINK_SPEED_SSID,
+					       ssid) |
+				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSE,
+					       USB_SSP_SUBLINK_SPEED_LSE_GBPS) |
+				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_ST, type) |
+				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LP, lp) |
+				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSM,
+					       mantissa));
+	}
+
+	return 0;
+}
+
+static int xhci_create_usb3x_bos_desc(struct xhci_hcd *xhci, char *buf,
+				      u16 wLength)
+{
+	struct usb_bos_descriptor	*bos;
+	struct usb_ss_cap_descriptor	*ss_cap;
+	struct usb_ssp_cap_descriptor	*ssp_cap;
+	struct xhci_port_cap		*port_cap = NULL;
+	u16				bcdUSB;
+	u32				reg;
+	u32				min_rate = 0;
+	u8				min_ssid;
+	u8				ssac;
+	u8				ssic;
+	int				offset;
+	int				i;
+
+	/* BOS descriptor */
+	bos = (struct usb_bos_descriptor *)buf;
+	bos->bLength = USB_DT_BOS_SIZE;
+	bos->bDescriptorType = USB_DT_BOS;
+	bos->wTotalLength = cpu_to_le16(USB_DT_BOS_SIZE +
+					USB_DT_USB_SS_CAP_SIZE);
+	bos->bNumDeviceCaps = 1;
+
+	/* Create the descriptor for port with the highest revision */
+	for (i = 0; i < xhci->num_port_caps; i++) {
+		u8 major = xhci->port_caps[i].maj_rev;
+		u8 minor = xhci->port_caps[i].min_rev;
+		u16 rev = (major << 8) | minor;
+
+		if (i == 0 || bcdUSB < rev) {
+			bcdUSB = rev;
+			port_cap = &xhci->port_caps[i];
+		}
+	}
+
+	if (bcdUSB >= 0x0310) {
+		/* Two SSA entries for each unique PSI ID, RX and TX */
+		if (port_cap->psi_count)
+			ssac = port_cap->psi_uid_count * 2 - 1;
+		else if (bcdUSB == 0x0320)
+			ssac = 7;
+		else
+			ssac = 3;
+
+		if (port_cap->psi_count)
+			ssic = port_cap->psi_count - 1;
+		else
+			ssic = (ssac + 1) / 2 - 1;
+
+		bos->bNumDeviceCaps++;
+		bos->wTotalLength = cpu_to_le16(USB_DT_BOS_SIZE +
+						USB_DT_USB_SS_CAP_SIZE +
+						USB_DT_USB_SSP_CAP_SIZE(ssac));
+	}
+
+	if (wLength < USB_DT_BOS_SIZE + USB_DT_USB_SS_CAP_SIZE)
+		return wLength;
+
+	/* SuperSpeed USB Device Capability */
+	ss_cap = (struct usb_ss_cap_descriptor *)&buf[USB_DT_BOS_SIZE];
+	ss_cap->bLength = USB_DT_USB_SS_CAP_SIZE;
+	ss_cap->bDescriptorType = USB_DT_DEVICE_CAPABILITY;
+	ss_cap->bDevCapabilityType = USB_SS_CAP_TYPE;
+	ss_cap->bmAttributes = 0; /* set later */
+	ss_cap->wSpeedSupported = cpu_to_le16(USB_5GBPS_OPERATION);
+	ss_cap->bFunctionalitySupport = USB_LOW_SPEED_OPERATION;
+	ss_cap->bU1devExitLat = 0; /* set later */
+	ss_cap->bU2DevExitLat = 0; /* set later */
+
+	reg = readl(&xhci->cap_regs->hcc_params);
+	if (HCC_LTC(reg))
+		ss_cap->bmAttributes |= USB_LTM_SUPPORT;
+
+	if ((xhci->quirks & XHCI_LPM_SUPPORT)) {
+		reg = readl(&xhci->cap_regs->hcs_params3);
+		ss_cap->bU1devExitLat = HCS_U1_LATENCY(reg);
+		ss_cap->bU2DevExitLat = cpu_to_le16(HCS_U2_LATENCY(reg));
+	}
+
+	if (wLength < le16_to_cpu(bos->wTotalLength))
+		return wLength;
+
+	if (bcdUSB < 0x0310)
+		return le16_to_cpu(bos->wTotalLength);
+
+	ssp_cap = (struct usb_ssp_cap_descriptor *)&buf[USB_DT_BOS_SIZE +
+		USB_DT_USB_SS_CAP_SIZE];
+	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, ssac) |
+			    FIELD_PREP(USB_SSP_SUBLINK_SPEED_IDS, ssic));
+
+	if (!port_cap->psi_count) {
+		int ret;
+
+		ret = xhci_fill_default_ssp_attr(xhci, ssp_cap);
+		if (ret)
+			return ret;
+
+		min_ssid = 4;
+		goto out;
+	}
+
+	offset = 0;
+	for (i = 0; i < port_cap->psi_count; i++) {
+		u32 psi;
+		u32 attr;
+		u8 ssid;
+		u8 lp;
+		u8 lse;
+		u8 psie;
+		u16 lane_mantissa;
+		u16 psim;
+		u16 plt;
+
+		psi = port_cap->psi[i];
+		ssid = XHCI_EXT_PORT_PSIV(psi);
+		lp = XHCI_EXT_PORT_LP(psi);
+		psie = XHCI_EXT_PORT_PSIE(psi);
+		psim = XHCI_EXT_PORT_PSIM(psi);
+		plt = psi & PLT_MASK;
+
+		lse = psie;
+		lane_mantissa = psim;
+
+		/* Shift to Gbps and set SSP Link Protocol if 10Gpbs */
+		for (; psie < USB_SSP_SUBLINK_SPEED_LSE_GBPS; psie++)
+			psim /= 1000;
+
+		if (!min_rate || psim < min_rate) {
+			min_ssid = ssid;
+			min_rate = psim;
+		}
+
+		/* Some host controllers don't set the link protocol for SSP */
+		if (psim >= 10)
+			lp = USB_SSP_SUBLINK_SPEED_LP_SSP;
+
+		/*
+		 * PSIM and PSIE represent the total speed of PSI. The BOS
+		 * descriptor SSP sublink speed attribute lane mantissa
+		 * describes the lane speed. E.g. PSIM and PSIE for gen2x2
+		 * is 20Gbps, but the BOS descriptor lane speed mantissa is
+		 * 10Gbps. Check and modify the mantissa value to match the
+		 * lane speed.
+		 */
+		if (bcdUSB == 0x0320 && plt == PLT_SYM) {
+			/*
+			 * The PSI dword for gen1x2 and gen2x1 share the same
+			 * values. But the lane speed for gen1x2 is 5Gbps while
+			 * gen2x1 is 10Gbps. If the previous PSI dword SSID is
+			 * 5 and the PSIE and PSIM match with SSID 6, let's
+			 * assume that the controller follows the default speed
+			 * id with SSID 6 for gen1x2.
+			 */
+			if (ssid == 6 && psie == 3 && psim == 10 && i) {
+				u32 prev = port_cap->psi[i - 1];
+
+				if ((prev & PLT_MASK) == PLT_SYM &&
+				    XHCI_EXT_PORT_PSIV(prev) == 5 &&
+				    XHCI_EXT_PORT_PSIE(prev) == 3 &&
+				    XHCI_EXT_PORT_PSIM(prev) == 10) {
+					lse = USB_SSP_SUBLINK_SPEED_LSE_GBPS;
+					lane_mantissa = 5;
+				}
+			}
+
+			if (psie == 3 && psim > 10) {
+				lse = USB_SSP_SUBLINK_SPEED_LSE_GBPS;
+				lane_mantissa = 10;
+			}
+		}
+
+		attr = (FIELD_PREP(USB_SSP_SUBLINK_SPEED_SSID, ssid) |
+			FIELD_PREP(USB_SSP_SUBLINK_SPEED_LP, lp) |
+			FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSE, lse) |
+			FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSM, lane_mantissa));
+
+		switch (plt) {
+		case PLT_SYM:
+			attr |= FIELD_PREP(USB_SSP_SUBLINK_SPEED_ST,
+					   USB_SSP_SUBLINK_SPEED_ST_SYM_RX);
+			ssp_cap->bmSublinkSpeedAttr[offset++] = cpu_to_le32(attr);
+
+			attr &= ~USB_SSP_SUBLINK_SPEED_ST;
+			attr |= FIELD_PREP(USB_SSP_SUBLINK_SPEED_ST,
+					   USB_SSP_SUBLINK_SPEED_ST_SYM_TX);
+			ssp_cap->bmSublinkSpeedAttr[offset++] = cpu_to_le32(attr);
+			break;
+		case PLT_ASYM_RX:
+			attr |= FIELD_PREP(USB_SSP_SUBLINK_SPEED_ST,
+					   USB_SSP_SUBLINK_SPEED_ST_ASYM_RX);
+			ssp_cap->bmSublinkSpeedAttr[offset++] = cpu_to_le32(attr);
+			break;
+		case PLT_ASYM_TX:
+			attr |= FIELD_PREP(USB_SSP_SUBLINK_SPEED_ST,
+					   USB_SSP_SUBLINK_SPEED_ST_ASYM_TX);
+			ssp_cap->bmSublinkSpeedAttr[offset++] = cpu_to_le32(attr);
+			break;
+		}
+	}
+out:
+	ssp_cap->wFunctionalitySupport =
+		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));
+
+	return le16_to_cpu(bos->wTotalLength);
+}
+
+static __maybe_unused int xhci_create_usb3_bos_desc(struct xhci_hcd *xhci, char *buf,
 				     u16 wLength)
 {
 	struct xhci_port_cap *port_cap = NULL;
@@ -1137,7 +1412,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 		if (hcd->speed < HCD_USB3)
 			goto error;
 
-		retval = xhci_create_usb3_bos_desc(xhci, buf, wLength);
+		retval = xhci_create_usb3x_bos_desc(xhci, buf, wLength);
 		spin_unlock_irqrestore(&xhci->lock, flags);
 		return retval;
 	case GetPortStatus:
-- 
2.28.0

[RFC PATCH 8/8] usb: xhci: Remove unused function

[Thinh Nguyen]

Now that we replaced the xhci_create_usb3_bos_desc() function. We can
remove it along with the static usb_bos_descriptor structure.

Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
---
 drivers/usb/host/xhci-hub.c | 147 ------------------------------------
 1 file changed, 147 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index c095c30212e5..411d5ae9f501 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -20,39 +20,6 @@
 #define	PORT_RWC_BITS	(PORT_CSC | PORT_PEC | PORT_WRC | PORT_OCC | \
 			 PORT_RC | PORT_PLC | PORT_PE)
 
-/* USB 3 BOS descriptor and a capability descriptors, combined.
- * Fields will be adjusted and added later in xhci_create_usb3_bos_desc()
- */
-static u8 usb_bos_descriptor [] = {
-	USB_DT_BOS_SIZE,		/*  __u8 bLength, 5 bytes */
-	USB_DT_BOS,			/*  __u8 bDescriptorType */
-	0x0F, 0x00,			/*  __le16 wTotalLength, 15 bytes */
-	0x1,				/*  __u8 bNumDeviceCaps */
-	/* First device capability, SuperSpeed */
-	USB_DT_USB_SS_CAP_SIZE,		/*  __u8 bLength, 10 bytes */
-	USB_DT_DEVICE_CAPABILITY,	/* Device Capability */
-	USB_SS_CAP_TYPE,		/* bDevCapabilityType, SUPERSPEED_USB */
-	0x00,				/* bmAttributes, LTM off by default */
-	USB_5GBPS_OPERATION, 0x00,	/* wSpeedsSupported, 5Gbps only */
-	0x03,				/* bFunctionalitySupport,
-					   USB 3.0 speed only */
-	0x00,				/* bU1DevExitLat, set later. */
-	0x00, 0x00,			/* __le16 bU2DevExitLat, set later. */
-	/* Second device capability, SuperSpeedPlus */
-	0x1c,				/* bLength 28, will be adjusted later */
-	USB_DT_DEVICE_CAPABILITY,	/* Device Capability */
-	USB_SSP_CAP_TYPE,		/* bDevCapabilityType SUPERSPEED_PLUS */
-	0x00,				/* bReserved 0 */
-	0x23, 0x00, 0x00, 0x00,		/* bmAttributes, SSAC=3 SSIC=1 */
-	0x01, 0x00,			/* wFunctionalitySupport */
-	0x00, 0x00,			/* wReserved 0 */
-	/* Default Sublink Speed Attributes, overwrite if custom PSI exists */
-	0x34, 0x00, 0x05, 0x00,		/* 5Gbps, symmetric, rx, ID = 4 */
-	0xb4, 0x00, 0x05, 0x00,		/* 5Gbps, symmetric, tx, ID = 4 */
-	0x35, 0x40, 0x0a, 0x00,		/* 10Gbps, SSP, symmetric, rx, ID = 5 */
-	0xb5, 0x40, 0x0a, 0x00,		/* 10Gbps, SSP, symmetric, tx, ID = 5 */
-};
-
 static int xhci_fill_default_ssp_attr(struct xhci_hcd *xhci,
 		struct usb_ssp_cap_descriptor *ssp_cap)
 {
@@ -327,120 +294,6 @@ static int xhci_create_usb3x_bos_desc(struct xhci_hcd *xhci, char *buf,
 	return le16_to_cpu(bos->wTotalLength);
 }
 
-static __maybe_unused int xhci_create_usb3_bos_desc(struct xhci_hcd *xhci, char *buf,
-				     u16 wLength)
-{
-	struct xhci_port_cap *port_cap = NULL;
-	int i, ssa_count;
-	u32 temp;
-	u16 desc_size, ssp_cap_size, ssa_size = 0;
-	bool usb3_1 = false;
-
-	desc_size = USB_DT_BOS_SIZE + USB_DT_USB_SS_CAP_SIZE;
-	ssp_cap_size = sizeof(usb_bos_descriptor) - desc_size;
-
-	/* does xhci support USB 3.1 Enhanced SuperSpeed */
-	for (i = 0; i < xhci->num_port_caps; i++) {
-		if (xhci->port_caps[i].maj_rev == 0x03 &&
-		    xhci->port_caps[i].min_rev >= 0x01) {
-			usb3_1 = true;
-			port_cap = &xhci->port_caps[i];
-			break;
-		}
-	}
-
-	if (usb3_1) {
-		/* does xhci provide a PSI table for SSA speed attributes? */
-		if (port_cap->psi_count) {
-			/* two SSA entries for each unique PSI ID, RX and TX */
-			ssa_count = port_cap->psi_uid_count * 2;
-			ssa_size = ssa_count * sizeof(u32);
-			ssp_cap_size -= 16; /* skip copying the default SSA */
-		}
-		desc_size += ssp_cap_size;
-	}
-	memcpy(buf, &usb_bos_descriptor, min(desc_size, wLength));
-
-	if (usb3_1) {
-		/* modify bos descriptor bNumDeviceCaps and wTotalLength */
-		buf[4] += 1;
-		put_unaligned_le16(desc_size + ssa_size, &buf[2]);
-	}
-
-	if (wLength < USB_DT_BOS_SIZE + USB_DT_USB_SS_CAP_SIZE)
-		return wLength;
-
-	/* Indicate whether the host has LTM support. */
-	temp = readl(&xhci->cap_regs->hcc_params);
-	if (HCC_LTC(temp))
-		buf[8] |= USB_LTM_SUPPORT;
-
-	/* Set the U1 and U2 exit latencies. */
-	if ((xhci->quirks & XHCI_LPM_SUPPORT)) {
-		temp = readl(&xhci->cap_regs->hcs_params3);
-		buf[12] = HCS_U1_LATENCY(temp);
-		put_unaligned_le16(HCS_U2_LATENCY(temp), &buf[13]);
-	}
-
-	/* If PSI table exists, add the custom speed attributes from it */
-	if (usb3_1 && port_cap->psi_count) {
-		u32 ssp_cap_base, bm_attrib, psi, psi_mant, psi_exp;
-		int offset;
-
-		ssp_cap_base = USB_DT_BOS_SIZE + USB_DT_USB_SS_CAP_SIZE;
-
-		if (wLength < desc_size)
-			return wLength;
-		buf[ssp_cap_base] = ssp_cap_size + ssa_size;
-
-		/* attribute count SSAC bits 4:0 and ID count SSIC bits 8:5 */
-		bm_attrib = (ssa_count - 1) & 0x1f;
-		bm_attrib |= (port_cap->psi_uid_count - 1) << 5;
-		put_unaligned_le32(bm_attrib, &buf[ssp_cap_base + 4]);
-
-		if (wLength < desc_size + ssa_size)
-			return wLength;
-		/*
-		 * Create the Sublink Speed Attributes (SSA) array.
-		 * The xhci PSI field and USB 3.1 SSA fields are very similar,
-		 * but link type bits 7:6 differ for values 01b and 10b.
-		 * xhci has also only one PSI entry for a symmetric link when
-		 * USB 3.1 requires two SSA entries (RX and TX) for every link
-		 */
-		offset = desc_size;
-		for (i = 0; i < port_cap->psi_count; i++) {
-			psi = port_cap->psi[i];
-			psi &= ~USB_SSP_SUBLINK_SPEED_RSVD;
-			psi_exp = XHCI_EXT_PORT_PSIE(psi);
-			psi_mant = XHCI_EXT_PORT_PSIM(psi);
-
-			/* Shift to Gbps and set SSP Link BIT(14) if 10Gpbs */
-			for (; psi_exp < 3; psi_exp++)
-				psi_mant /= 1000;
-			if (psi_mant >= 10)
-				psi |= BIT(14);
-
-			if ((psi & PLT_MASK) == PLT_SYM) {
-			/* Symmetric, create SSA RX and TX from one PSI entry */
-				put_unaligned_le32(psi, &buf[offset]);
-				psi |= 1 << 7;  /* turn entry to TX */
-				offset += 4;
-				if (offset >= desc_size + ssa_size)
-					return desc_size + ssa_size;
-			} else if ((psi & PLT_MASK) == PLT_ASYM_RX) {
-				/* Asymetric RX, flip bits 7:6 for SSA */
-				psi ^= PLT_MASK;
-			}
-			put_unaligned_le32(psi, &buf[offset]);
-			offset += 4;
-			if (offset >= desc_size + ssa_size)
-				return desc_size + ssa_size;
-		}
-	}
-	/* ssa_size is 0 for other than usb 3.1 hosts */
-	return desc_size + ssa_size;
-}
-
 static void xhci_common_hub_descriptor(struct xhci_hcd *xhci,
 		struct usb_hub_descriptor *desc, int ports)
 {
-- 
2.28.0

Re: [RFC PATCH 1/8] usb: core: Track SuperSpeed Plus GenXxY

[Thinh Nguyen]

Gustavo A. R. Silva wrote:
> Hi,
>
> On 2/1/21 21:42, Thinh Nguyen wrote:
>> Introduce ssp_rate field to usb_device structure to capture the
>> connected SuperSpeed Plus signaling rate generation and lane count with
>> the corresponding usb_ssp_rate enum.
>>
>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>> ---
>>  drivers/usb/core/hcd.c |  6 +++-
>>  drivers/usb/core/hub.c | 78 ++++++++++++++++++++++++++++++++++++++++++
>>  include/linux/usb.h    |  2 ++
>>  3 files changed, 85 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index ad5a0f405a75..55de04df3022 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -2721,6 +2721,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>>  
>>  	rhdev->rx_lanes = 1;
>>  	rhdev->tx_lanes = 1;
>> +	rhdev->ssp_rate = USB_SSP_GEN_UNKNOWN;
>>  
>>  	switch (hcd->speed) {
>>  	case HCD_USB11:
>> @@ -2738,8 +2739,11 @@ int usb_add_hcd(struct usb_hcd *hcd,
>>  	case HCD_USB32:
>>  		rhdev->rx_lanes = 2;
>>  		rhdev->tx_lanes = 2;
>> -		fallthrough;
>> +		rhdev->ssp_rate = USB_SSP_GEN_2x2;
>> +		rhdev->speed = USB_SPEED_SUPER_PLUS;
>> +		break;
>>  	case HCD_USB31:
>> +		rhdev->ssp_rate = USB_SSP_GEN_2x1;
>>  		rhdev->speed = USB_SPEED_SUPER_PLUS;
>>  		break;
>>  	default:
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index 7f71218cc1e5..e78b2dd7801a 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -31,6 +31,7 @@
>>  #include <linux/pm_qos.h>
>>  #include <linux/kobject.h>
>>  
>> +#include <linux/bitfield.h>
>>  #include <linux/uaccess.h>
>>  #include <asm/byteorder.h>
>>  
>> @@ -2668,6 +2669,81 @@ int usb_authorize_device(struct usb_device *usb_dev)
>>  	return result;
>>  }
>>  
>> +/**
>> + * get_port_ssp_rate - Match the extended port status to SSP rate
>> + * @hdev: The hub device
>> + * @ext_portstatus: extended port status
>> + *
>> + * Match the extended port status speed id to the SuperSpeed Plus sublink speed
>> + * capability attributes. Base on the number of connected lanes and speed,
>> + * return the corresponding enum usb_ssp_rate.
>> + */
>> +static enum usb_ssp_rate get_port_ssp_rate(struct usb_device *hdev,
>> +					   u32 ext_portstatus)
>> +{
>> +	struct usb_ssp_cap_descriptor *ssp_cap = hdev->bos->ssp_cap;
>> +	u32 attr;
>> +	u8 speed_id;
>> +	u8 ssac;
>> +	u8 lanes;
>> +	int i;
>> +
>> +	if (!ssp_cap)
>> +		goto out;
>> +
>> +	speed_id = ext_portstatus & USB_EXT_PORT_STAT_RX_SPEED_ID;
>> +	lanes = USB_EXT_PORT_RX_LANES(ext_portstatus) + 1;
>> +
>> +	ssac = le32_to_cpu(ssp_cap->bmAttributes) &
>> +		USB_SSP_SUBLINK_SPEED_ATTRIBS;
>> +
>> +	for (i = 0; i <= ssac; i++) {
> Why a less than or equal to comparison here?
> Why not just a less than comparison (i < ssac) ?
>
> Thanks
> --
> Gustavo

The SSAC here matches with the SSAC (Sublink Speed Attribute Count) from
the USB 3.2 spec. It's zero based. E.g. so SSAC of 3 is 4 number of
Sublink Speed Attributes. That's why "<=".

Thinh

>
>> +		u8 ssid;
>> +
>> +		attr = le32_to_cpu(ssp_cap->bmSublinkSpeedAttr[i]);
>> +		ssid = FIELD_GET(USB_SSP_SUBLINK_SPEED_SSID, attr);
>> +		if (speed_id == ssid) {
>> +			u16 mantissa;
>> +			u8 lse;
>> +			u8 type;
>> +
>> +			/*
>> +			 * Note: currently asymmetric lane types are only
>> +			 * applicable for SSIC operate in SuperSpeed protocol
>> +			 */
>> +			type = FIELD_GET(USB_SSP_SUBLINK_SPEED_ST, attr);
>> +			if (type == USB_SSP_SUBLINK_SPEED_ST_ASYM_RX ||
>> +			    type == USB_SSP_SUBLINK_SPEED_ST_ASYM_TX)
>> +				goto out;
>> +
>> +			if (FIELD_GET(USB_SSP_SUBLINK_SPEED_LP, attr) !=
>> +			    USB_SSP_SUBLINK_SPEED_LP_SSP)
>> +				goto out;
>> +
>> +			lse = FIELD_GET(USB_SSP_SUBLINK_SPEED_LSE, attr);
>> +			mantissa = FIELD_GET(USB_SSP_SUBLINK_SPEED_LSM, attr);
>> +
>> +			/* Convert to Gbps */
>> +			for (; lse < USB_SSP_SUBLINK_SPEED_LSE_GBPS; lse++)
>> +				mantissa /= 1000;
>> +
>> +			if (mantissa >= 10 && lanes == 1)
>> +				return USB_SSP_GEN_2x1;
>> +
>> +			if (mantissa >= 10 && lanes == 2)
>> +				return USB_SSP_GEN_2x2;
>> +
>> +			if (mantissa >= 5 && lanes == 2)
>> +				return USB_SSP_GEN_1x2;
>> +
>> +			goto out;
>> +		}
>> +	}
>> +
>> +out:
>> +	return USB_SSP_GEN_UNKNOWN;
>> +}
>> +
>>  /*
>>   * Return 1 if port speed is SuperSpeedPlus, 0 otherwise
>>   * check it from the link protocol field of the current speed ID attribute.
>> @@ -2850,9 +2926,11 @@ static int hub_port_wait_reset(struct usb_hub *hub, int port1,
>>  		/* extended portstatus Rx and Tx lane count are zero based */
>>  		udev->rx_lanes = USB_EXT_PORT_RX_LANES(ext_portstatus) + 1;
>>  		udev->tx_lanes = USB_EXT_PORT_TX_LANES(ext_portstatus) + 1;
>> +		udev->ssp_rate = get_port_ssp_rate(hub->hdev, ext_portstatus);
>>  	} else {
>>  		udev->rx_lanes = 1;
>>  		udev->tx_lanes = 1;
>> +		udev->ssp_rate = USB_SSP_GEN_UNKNOWN;
>>  	}
>>  	if (hub_is_wusb(hub))
>>  		udev->speed = USB_SPEED_WIRELESS;
>> diff --git a/include/linux/usb.h b/include/linux/usb.h
>> index 7d72c4e0713c..c334387f950e 100644
>> --- a/include/linux/usb.h
>> +++ b/include/linux/usb.h
>> @@ -560,6 +560,7 @@ struct usb3_lpm_parameters {
>>   * @speed: device speed: high/full/low (or error)
>>   * @rx_lanes: number of rx lanes in use, USB 3.2 adds dual-lane support
>>   * @tx_lanes: number of tx lanes in use, USB 3.2 adds dual-lane support
>> + * @ssp_rate: SuperSpeed Plus phy signaling rate and lane count
>>   * @tt: Transaction Translator info; used with low/full speed dev, highspeed hub
>>   * @ttport: device port on that tt hub
>>   * @toggle: one bit for each endpoint, with ([0] = IN, [1] = OUT) endpoints
>> @@ -636,6 +637,7 @@ struct usb_device {
>>  	enum usb_device_speed	speed;
>>  	unsigned int		rx_lanes;
>>  	unsigned int		tx_lanes;
>> +	enum usb_ssp_rate	ssp_rate;
>>  
>>  	struct usb_tt	*tt;
>>  	int		ttport;
>>

Re: [RFC PATCH 1/8] usb: core: Track SuperSpeed Plus GenXxY

[Thinh Nguyen]

Gustavo A. R. Silva wrote:
>
> On 2/1/21 21:58, Thinh Nguyen wrote:
> [..]
>
>>>> +/**
>>>> + * get_port_ssp_rate - Match the extended port status to SSP rate
>>>> + * @hdev: The hub device
>>>> + * @ext_portstatus: extended port status
>>>> + *
>>>> + * Match the extended port status speed id to the SuperSpeed Plus sublink speed
>>>> + * capability attributes. Base on the number of connected lanes and speed,
>>>> + * return the corresponding enum usb_ssp_rate.
>>>> + */
>>>> +static enum usb_ssp_rate get_port_ssp_rate(struct usb_device *hdev,
>>>> +					   u32 ext_portstatus)
>>>> +{
>>>> +	struct usb_ssp_cap_descriptor *ssp_cap = hdev->bos->ssp_cap;
>>>> +	u32 attr;
>>>> +	u8 speed_id;
>>>> +	u8 ssac;
>>>> +	u8 lanes;
>>>> +	int i;
>>>> +
>>>> +	if (!ssp_cap)
>>>> +		goto out;
>>>> +
>>>> +	speed_id = ext_portstatus & USB_EXT_PORT_STAT_RX_SPEED_ID;
>>>> +	lanes = USB_EXT_PORT_RX_LANES(ext_portstatus) + 1;
>>>> +
>>>> +	ssac = le32_to_cpu(ssp_cap->bmAttributes) &
>>>> +		USB_SSP_SUBLINK_SPEED_ATTRIBS;
>>>> +
>>>> +	for (i = 0; i <= ssac; i++) {
>>> Why a less than or equal to comparison here?
>>> Why not just a less than comparison (i < ssac) ?
>>>
>>> Thanks
>>> --
>>> Gustavo
>> The SSAC here matches with the SSAC (Sublink Speed Attribute Count) from
>> the USB 3.2 spec. It's zero based. E.g. so SSAC of 3 is 4 number of
>> Sublink Speed Attributes. That's why "<=".
> I see, what's worthy of attention is that _i_ is being used as an index
> for bmSublinkSpeedAttr[], just a couple of lines below:
>
>>>> +		u8 ssid;
>>>> +
>>>> +		attr = le32_to_cpu(ssp_cap->bmSublinkSpeedAttr[i]);
> 							^^^
> 							here
>
> are we sure that this doesn't cause an out-of-bounds read?
> 						
> --
> Gustavo

It can if the hub output some bogus value for SSAC. Please note that I'm
using the same logic as the previous implementation for port_speed_is_ssp().

Thanks,
Thinh

>
>>>> +		ssid = FIELD_GET(USB_SSP_SUBLINK_SPEED_SSID, attr);
>>>> +		if (speed_id == ssid) {
>>>> +			u16 mantissa;
>>>> +			u8 lse;
>>>> +			u8 type;
>>>> +
>>>> +			/*
>>>> +			 * Note: currently asymmetric lane types are only
>>>> +			 * applicable for SSIC operate in SuperSpeed protocol
>>>> +			 */
>>>> +			type = FIELD_GET(USB_SSP_SUBLINK_SPEED_ST, attr);
>>>> +			if (type == USB_SSP_SUBLINK_SPEED_ST_ASYM_RX ||
>>>> +			    type == USB_SSP_SUBLINK_SPEED_ST_ASYM_TX)
>>>> +				goto out;
>>>> +
>>>> +			if (FIELD_GET(USB_SSP_SUBLINK_SPEED_LP, attr) !=
>>>> +			    USB_SSP_SUBLINK_SPEED_LP_SSP)
>>>> +				goto out;
>>>> +
>>>> +			lse = FIELD_GET(USB_SSP_SUBLINK_SPEED_LSE, attr);
>>>> +			mantissa = FIELD_GET(USB_SSP_SUBLINK_SPEED_LSM, attr);
>>>> +
>>>> +			/* Convert to Gbps */
>>>> +			for (; lse < USB_SSP_SUBLINK_SPEED_LSE_GBPS; lse++)
>>>> +				mantissa /= 1000;
>>>> +
>>>> +			if (mantissa >= 10 && lanes == 1)
>>>> +				return USB_SSP_GEN_2x1;
>>>> +
>>>> +			if (mantissa >= 10 && lanes == 2)
>>>> +				return USB_SSP_GEN_2x2;
>>>> +
>>>> +			if (mantissa >= 5 && lanes == 2)
>>>> +				return USB_SSP_GEN_1x2;
>>>> +
>>>> +			goto out;
>>>> +		}
>>>> +	}
>>>> +
>>>> +out:
>>>> +	return USB_SSP_GEN_UNKNOWN;
>>>> +}
>>>> +
>>>>  /*
>>>>   * Return 1 if port speed is SuperSpeedPlus, 0 otherwise
>>>>   * check it from the link protocol field of the current speed ID attribute.
>>>> @@ -2850,9 +2926,11 @@ static int hub_port_wait_reset(struct usb_hub *hub, int port1,
>>>>  		/* extended portstatus Rx and Tx lane count are zero based */
>>>>  		udev->rx_lanes = USB_EXT_PORT_RX_LANES(ext_portstatus) + 1;
>>>>  		udev->tx_lanes = USB_EXT_PORT_TX_LANES(ext_portstatus) + 1;
>>>> +		udev->ssp_rate = get_port_ssp_rate(hub->hdev, ext_portstatus);
>>>>  	} else {
>>>>  		udev->rx_lanes = 1;
>>>>  		udev->tx_lanes = 1;
>>>> +		udev->ssp_rate = USB_SSP_GEN_UNKNOWN;
>>>>  	}
>>>>  	if (hub_is_wusb(hub))
>>>>  		udev->speed = USB_SPEED_WIRELESS;

Re: [RFC PATCH 6/8] usb: xhci: Fix port minor revision

[Sergei Shtylyov]

Hello!

On 02.02.2021 6:42, Thinh Nguyen wrote:

> Some hosts incorrectly use sub-minor version for minor version (i.e.
> 0x02 instead of 0x20 for bcdUSB 0x320 and 0x01 for bcdUSB 0x310).
> Currently the xHCI driver works around this by just checking for minor
> revision > 0x01 for USB 3.1 everywhere. With the addition of USB 3.2,
> checking this gets a bit cumbersome. Since there is no USB release with
> bcdUSB 0x301 to 0x309, we can assume that sub-minor version 01 to 09 is
> incorrect. Let's try to fix this and use the minor revision that matches
> with the USB/xHCI spec to help with the version checking within the
> driver.
> 
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> ---
>   drivers/usb/host/xhci-mem.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> index f2c4ee7c4786..34105b477c62 100644
> --- a/drivers/usb/host/xhci-mem.c
> +++ b/drivers/usb/host/xhci-mem.c
> @@ -2129,6 +2129,15 @@ static void xhci_add_in_port(struct xhci_hcd *xhci, unsigned int num_ports,
>   
>   	if (major_revision == 0x03) {
>   		rhub = &xhci->usb3_rhub;
> +		/*
> +		 * Some hosts incorrectly use sub-minor version for minor
> +		 * version (i.e. 0x02 instead of 0x20 for bcdUSB 0x320 and 0x01
> +		 * for bcdUSB 0x310). Since there is no USB release with sub
> +		 * minor version 0x301 to 0x309, we can assume that they are
> +		 * incorrect and fix it here.
> +		 */
> +		if (!(minor_revision & 0xf0) && (minor_revision & 0x0f))
> +			minor_revision = minor_revision << 4;

    Why not:

			minor_revision <<= 4;

[...]

MBR, Sergei

Re: [RFC PATCH 7/8] usb: xhci: Rewrite xhci_create_usb3_bos_desc()

[Mathias Nyman]

Hi

On 2.2.2021 5.42, Thinh Nguyen wrote:
> The current xhci_create_usb3_bos_desc() uses a static bos structure and
> various magic numbers and offset making it difficult to extend support
> for USB 3.2. Let's rewrite this entire function to support dual-lane in
> USB 3.2.

Agree, it's time to get rid of the static u8 array used for bos.
 
> 
> The hub driver matches the port speed ID from the extended port status
> to the SSID of the sublink speed attributes to detect if the device
> supports SuperSpeed Plus. Currently we don't provide the default gen1x2
> and gen2x2 sublink speed capability descriptor for USB 3.2 roothub. The
> USB stack depends on this to detect and match the correct speed.
> In addition, if the xHCI host provides Protocol Speed ID (PSI)
> capability, then make sure to convert Protocol Speed ID Mantissa and
> Exponent (PSIM & PSIE) to lane speed for gen1x2 and gen2x2.
> 
> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> ---
>  drivers/usb/host/xhci-hub.c | 279 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 277 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index 74c497fd3476..c095c30212e5 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -11,6 +11,7 @@
>  
>  #include <linux/slab.h>
>  #include <asm/unaligned.h>
> +#include <linux/bitfield.h>
>  
>  #include "xhci.ch"
>  #include "xhci-trace.h"
> @@ -52,7 +53,281 @@ static u8 usb_bos_descriptor [] = {
>  	0xb5, 0x40, 0x0a, 0x00,		/* 10Gbps, SSP, symmetric, tx, ID = 5 */
>  };
>  
> -static int xhci_create_usb3_bos_desc(struct xhci_hcd *xhci, char *buf,
> +static int xhci_fill_default_ssp_attr(struct xhci_hcd *xhci,
> +		struct usb_ssp_cap_descriptor *ssp_cap)
> +{
> +	u32 attr;
> +	u8 ssac;
> +	int i;
> +
> +	attr = le32_to_cpu(ssp_cap->bmAttributes);
> +	ssac = FIELD_GET(USB_SSP_SUBLINK_SPEED_ATTRIBS, attr);
> +
> +	/* Currently we only support USB 3.1 and 3.2 */
> +	if (ssac != 3 && ssac != 7)
> +		return -EINVAL;
> +
> +	/*
> +	 * Map default xHCI port speed ID to SSID:
> +	 *  SSID 4 = Symmetric SSP Gen1x1
> +	 *  SSID 5 = Symmetric SSP Gen2x1
> +	 *  SSID 6 = Symmetric SSP Gen1x2
> +	 *  SSID 7 = Symmetric SSP Gen2x2
> +	 */
> +	for (i = 0; i < ssac + 1; i++) {
> +		u8 ssid;
> +		u8 type;
> +		u8 lp;
> +		u16 mantissa;
> +
> +		ssid = (i >> 1) + 4;
> +
> +		if (ssid > 4)
> +			lp = USB_SSP_SUBLINK_SPEED_LP_SSP;
> +		else
> +			lp = USB_SSP_SUBLINK_SPEED_LP_SS;
> +
> +		if (ssid == 5 || ssid == 7)
> +			mantissa = 10;
> +		else
> +			mantissa = 5;
> +
> +		if (i % 2)
> +			type = USB_SSP_SUBLINK_SPEED_ST_SYM_TX;
> +		else
> +			type = USB_SSP_SUBLINK_SPEED_ST_SYM_RX;
> +
> +		ssp_cap->bmSublinkSpeedAttr[i] =
> +			cpu_to_le32(FIELD_PREP(USB_SSP_SUBLINK_SPEED_SSID,
> +					       ssid) |
> +				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSE,
> +					       USB_SSP_SUBLINK_SPEED_LSE_GBPS) |
> +				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_ST, type) |
> +				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LP, lp) |
> +				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSM,
> +					       mantissa));
> +	}
> +
> +	return 0;
> +}

I don't think it makes sense to generates the default sublinkSpeedAttr[] entries like
the function above does.
The content is static and defined in xhci spec.
For these I'd just make a static u32 array. (minding endianness)

static u32 ssp_cap_default_ssa[] = {
	0xaaaaaaaa, /* USB 3.0 SS 5Gbps Id:4, rx */ 
	0xbbbbbbbb, /* USB 3.0 SS 5Gbps Id:4, tx */ 
	0xcccccccc, /* USB 3.1 SSP 10Gbps Id:5  rx */ 
	0xdddddddd, /* USB 3.1 SSP 10Gbps Id:5, tx */ 
	0xeeeeeeee, /* USB 3.2 Gen1x2 10Gbps Id:6, rx */ 
	...
}

Then when creating the ssp cap descriptor set the attribute count (ssac) based on
bcdUSB as you do, and assign the default values if custom ones aren't given (!psi_count).

for (i = 0; i < ssac + 1; i++)
	ssp_cap->mbSublinkSpeedAttr[i] = ssp_capdefault_ssa[i]; 
> +
> +static int xhci_create_usb3x_bos_desc(struct xhci_hcd *xhci, char *buf,
> +				      u16 wLength)
> +{
> +	struct usb_bos_descriptor	*bos;
> +	struct usb_ss_cap_descriptor	*ss_cap;
> +	struct usb_ssp_cap_descriptor	*ssp_cap;
> +	struct xhci_port_cap		*port_cap = NULL;
> +	u16				bcdUSB;
> +	u32				reg;
> +	u32				min_rate = 0;
> +	u8				min_ssid;
> +	u8				ssac;
> +	u8				ssic;
> +	int				offset;
> +	int				i;
> +
> +	/* BOS descriptor */
> +	bos = (struct usb_bos_descriptor *)buf;
> +	bos->bLength = USB_DT_BOS_SIZE;
> +	bos->bDescriptorType = USB_DT_BOS;
> +	bos->wTotalLength = cpu_to_le16(USB_DT_BOS_SIZE +
> +					USB_DT_USB_SS_CAP_SIZE);
> +	bos->bNumDeviceCaps = 1;

This is much better than the current way we fill the descriptor, much more readable.

> +
> +	/* Create the descriptor for port with the highest revision */
> +	for (i = 0; i < xhci->num_port_caps; i++) {
> +		u8 major = xhci->port_caps[i].maj_rev;
> +		u8 minor = xhci->port_caps[i].min_rev;
> +		u16 rev = (major << 8) | minor;
> +
> +		if (i == 0 || bcdUSB < rev) {
> +			bcdUSB = rev;
> +			port_cap = &xhci->port_caps[i];
> +		}
> +	}
> +
> +	if (bcdUSB >= 0x0310) {
> +		/* Two SSA entries for each unique PSI ID, RX and TX */
> +		if (port_cap->psi_count)
> +			ssac = port_cap->psi_uid_count * 2 - 1;
> +		else if (bcdUSB == 0x0320)

maybe (bcdUSB >= 0x320), allows old driver to handle future hardware.

> +			ssac = 7;
> +		else
> +			ssac = 3;
> +
> +		if (port_cap->psi_count)
> +			ssic = port_cap->psi_count - 1;
> +		else
> +			ssic = (ssac + 1) / 2 - 1;
> +
> +		bos->bNumDeviceCaps++;
> +		bos->wTotalLength = cpu_to_le16(USB_DT_BOS_SIZE +
> +						USB_DT_USB_SS_CAP_SIZE +
> +						USB_DT_USB_SSP_CAP_SIZE(ssac));
> +	}
> +
> +	if (wLength < USB_DT_BOS_SIZE + USB_DT_USB_SS_CAP_SIZE)
> +		return wLength;
> +
> +	/* SuperSpeed USB Device Capability */
> +	ss_cap = (struct usb_ss_cap_descriptor *)&buf[USB_DT_BOS_SIZE];
> +	ss_cap->bLength = USB_DT_USB_SS_CAP_SIZE;
> +	ss_cap->bDescriptorType = USB_DT_DEVICE_CAPABILITY;
> +	ss_cap->bDevCapabilityType = USB_SS_CAP_TYPE;
> +	ss_cap->bmAttributes = 0; /* set later */
> +	ss_cap->wSpeedSupported = cpu_to_le16(USB_5GBPS_OPERATION);
> +	ss_cap->bFunctionalitySupport = USB_LOW_SPEED_OPERATION;
> +	ss_cap->bU1devExitLat = 0; /* set later */
> +	ss_cap->bU2DevExitLat = 0; /* set later */
> +
> +	reg = readl(&xhci->cap_regs->hcc_params);
> +	if (HCC_LTC(reg))
> +		ss_cap->bmAttributes |= USB_LTM_SUPPORT;
> +
> +	if ((xhci->quirks & XHCI_LPM_SUPPORT)) {
> +		reg = readl(&xhci->cap_regs->hcs_params3);
> +		ss_cap->bU1devExitLat = HCS_U1_LATENCY(reg);
> +		ss_cap->bU2DevExitLat = cpu_to_le16(HCS_U2_LATENCY(reg));
> +	}
> +
> +	if (wLength < le16_to_cpu(bos->wTotalLength))
> +		return wLength;
> +
> +	if (bcdUSB < 0x0310)
> +		return le16_to_cpu(bos->wTotalLength);
> +
> +	ssp_cap = (struct usb_ssp_cap_descriptor *)&buf[USB_DT_BOS_SIZE +
> +		USB_DT_USB_SS_CAP_SIZE];
> +	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, ssac) |
> +			    FIELD_PREP(USB_SSP_SUBLINK_SPEED_IDS, ssic));
> +
> +	if (!port_cap->psi_count) {

for (i = 0; i < ssac + 1; i++)
	ssp_cap->mbSublinkSpeedAttr[i] = ssp_cap_default_ssa[i];  //mind endianness

> +		int ret;
> +
> +		ret = xhci_fill_default_ssp_attr(xhci, ssp_cap);
> +		if (ret)
> +			return ret;
> +
> +		min_ssid = 4;
> +		goto out;
> +	}
> +
> +	offset = 0;
> +	for (i = 0; i < port_cap->psi_count; i++) {

I need to take a better look at this sublinkSpeedAttr[] creation from custom PSIC table a bit later.

Thanks
-Mathias

Re: [RFC PATCH 6/8] usb: xhci: Fix port minor revision

[Thinh Nguyen]

Sergei Shtylyov wrote:
> Hello!
>
> On 02.02.2021 6:42, Thinh Nguyen wrote:
>
>> Some hosts incorrectly use sub-minor version for minor version (i.e.
>> 0x02 instead of 0x20 for bcdUSB 0x320 and 0x01 for bcdUSB 0x310).
>> Currently the xHCI driver works around this by just checking for minor
>> revision > 0x01 for USB 3.1 everywhere. With the addition of USB 3.2,
>> checking this gets a bit cumbersome. Since there is no USB release with
>> bcdUSB 0x301 to 0x309, we can assume that sub-minor version 01 to 09 is
>> incorrect. Let's try to fix this and use the minor revision that matches
>> with the USB/xHCI spec to help with the version checking within the
>> driver.
>>
>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>> ---
>>   drivers/usb/host/xhci-mem.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>> index f2c4ee7c4786..34105b477c62 100644
>> --- a/drivers/usb/host/xhci-mem.c
>> +++ b/drivers/usb/host/xhci-mem.c
>> @@ -2129,6 +2129,15 @@ static void xhci_add_in_port(struct xhci_hcd
>> *xhci, unsigned int num_ports,
>>         if (major_revision == 0x03) {
>>           rhub = &xhci->usb3_rhub;
>> +        /*
>> +         * Some hosts incorrectly use sub-minor version for minor
>> +         * version (i.e. 0x02 instead of 0x20 for bcdUSB 0x320 and 0x01
>> +         * for bcdUSB 0x310). Since there is no USB release with sub
>> +         * minor version 0x301 to 0x309, we can assume that they are
>> +         * incorrect and fix it here.
>> +         */
>> +        if (!(minor_revision & 0xf0) && (minor_revision & 0x0f))
>> +            minor_revision = minor_revision << 4;
>
>    Why not:
>
>             minor_revision <<= 4;
>
> [...]
>
> MBR, Sergei

Sure, we can do that.

Thanks,
Thinh

Re: [RFC PATCH 7/8] usb: xhci: Rewrite xhci_create_usb3_bos_desc()

[Thinh Nguyen]

Mathias Nyman wrote:
> Hi
>
> On 2.2.2021 5.42, Thinh Nguyen wrote:
>> The current xhci_create_usb3_bos_desc() uses a static bos structure and
>> various magic numbers and offset making it difficult to extend support
>> for USB 3.2. Let's rewrite this entire function to support dual-lane in
>> USB 3.2.
> Agree, it's time to get rid of the static u8 array used for bos.
>  
>> The hub driver matches the port speed ID from the extended port status
>> to the SSID of the sublink speed attributes to detect if the device
>> supports SuperSpeed Plus. Currently we don't provide the default gen1x2
>> and gen2x2 sublink speed capability descriptor for USB 3.2 roothub. The
>> USB stack depends on this to detect and match the correct speed.
>> In addition, if the xHCI host provides Protocol Speed ID (PSI)
>> capability, then make sure to convert Protocol Speed ID Mantissa and
>> Exponent (PSIM & PSIE) to lane speed for gen1x2 and gen2x2.
>>
>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>> ---
>>  drivers/usb/host/xhci-hub.c | 279 +++++++++++++++++++++++++++++++++++-
>>  1 file changed, 277 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
>> index 74c497fd3476..c095c30212e5 100644
>> --- a/drivers/usb/host/xhci-hub.c
>> +++ b/drivers/usb/host/xhci-hub.c
>> @@ -11,6 +11,7 @@
>>  
>>  #include <linux/slab.h>
>>  #include <asm/unaligned.h>
>> +#include <linux/bitfield.h>
>>  
>>  #include "xhci.ch"
>>  #include "xhci-trace.h"
>> @@ -52,7 +53,281 @@ static u8 usb_bos_descriptor [] = {
>>  	0xb5, 0x40, 0x0a, 0x00,		/* 10Gbps, SSP, symmetric, tx, ID = 5 */
>>  };
>>  
>> -static int xhci_create_usb3_bos_desc(struct xhci_hcd *xhci, char *buf,
>> +static int xhci_fill_default_ssp_attr(struct xhci_hcd *xhci,
>> +		struct usb_ssp_cap_descriptor *ssp_cap)
>> +{
>> +	u32 attr;
>> +	u8 ssac;
>> +	int i;
>> +
>> +	attr = le32_to_cpu(ssp_cap->bmAttributes);
>> +	ssac = FIELD_GET(USB_SSP_SUBLINK_SPEED_ATTRIBS, attr);
>> +
>> +	/* Currently we only support USB 3.1 and 3.2 */
>> +	if (ssac != 3 && ssac != 7)
>> +		return -EINVAL;
>> +
>> +	/*
>> +	 * Map default xHCI port speed ID to SSID:
>> +	 *  SSID 4 = Symmetric SSP Gen1x1
>> +	 *  SSID 5 = Symmetric SSP Gen2x1
>> +	 *  SSID 6 = Symmetric SSP Gen1x2
>> +	 *  SSID 7 = Symmetric SSP Gen2x2
>> +	 */
>> +	for (i = 0; i < ssac + 1; i++) {
>> +		u8 ssid;
>> +		u8 type;
>> +		u8 lp;
>> +		u16 mantissa;
>> +
>> +		ssid = (i >> 1) + 4;
>> +
>> +		if (ssid > 4)
>> +			lp = USB_SSP_SUBLINK_SPEED_LP_SSP;
>> +		else
>> +			lp = USB_SSP_SUBLINK_SPEED_LP_SS;
>> +
>> +		if (ssid == 5 || ssid == 7)
>> +			mantissa = 10;
>> +		else
>> +			mantissa = 5;
>> +
>> +		if (i % 2)
>> +			type = USB_SSP_SUBLINK_SPEED_ST_SYM_TX;
>> +		else
>> +			type = USB_SSP_SUBLINK_SPEED_ST_SYM_RX;
>> +
>> +		ssp_cap->bmSublinkSpeedAttr[i] =
>> +			cpu_to_le32(FIELD_PREP(USB_SSP_SUBLINK_SPEED_SSID,
>> +					       ssid) |
>> +				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSE,
>> +					       USB_SSP_SUBLINK_SPEED_LSE_GBPS) |
>> +				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_ST, type) |
>> +				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LP, lp) |
>> +				    FIELD_PREP(USB_SSP_SUBLINK_SPEED_LSM,
>> +					       mantissa));
>> +	}
>> +
>> +	return 0;
>> +}
> I don't think it makes sense to generates the default sublinkSpeedAttr[] entries like
> the function above does.
> The content is static and defined in xhci spec.
> For these I'd just make a static u32 array. (minding endianness)
>
> static u32 ssp_cap_default_ssa[] = {
> 	0xaaaaaaaa, /* USB 3.0 SS 5Gbps Id:4, rx */ 
> 	0xbbbbbbbb, /* USB 3.0 SS 5Gbps Id:4, tx */ 
> 	0xcccccccc, /* USB 3.1 SSP 10Gbps Id:5  rx */ 
> 	0xdddddddd, /* USB 3.1 SSP 10Gbps Id:5, tx */ 
> 	0xeeeeeeee, /* USB 3.2 Gen1x2 10Gbps Id:6, rx */ 
> 	...
> }
>
> Then when creating the ssp cap descriptor set the attribute count (ssac) based on
> bcdUSB as you do, and assign the default values if custom ones aren't given (!psi_count).
>
> for (i = 0; i < ssac + 1; i++)
> 	ssp_cap->mbSublinkSpeedAttr[i] = ssp_capdefault_ssa[i]; 

That looks good. I'll revise it.

>> +
>> +static int xhci_create_usb3x_bos_desc(struct xhci_hcd *xhci, char *buf,
>> +				      u16 wLength)
>> +{
>> +	struct usb_bos_descriptor	*bos;
>> +	struct usb_ss_cap_descriptor	*ss_cap;
>> +	struct usb_ssp_cap_descriptor	*ssp_cap;
>> +	struct xhci_port_cap		*port_cap = NULL;
>> +	u16				bcdUSB;
>> +	u32				reg;
>> +	u32				min_rate = 0;
>> +	u8				min_ssid;
>> +	u8				ssac;
>> +	u8				ssic;
>> +	int				offset;
>> +	int				i;
>> +
>> +	/* BOS descriptor */
>> +	bos = (struct usb_bos_descriptor *)buf;
>> +	bos->bLength = USB_DT_BOS_SIZE;
>> +	bos->bDescriptorType = USB_DT_BOS;
>> +	bos->wTotalLength = cpu_to_le16(USB_DT_BOS_SIZE +
>> +					USB_DT_USB_SS_CAP_SIZE);
>> +	bos->bNumDeviceCaps = 1;
> This is much better than the current way we fill the descriptor, much more readable.
>
>> +
>> +	/* Create the descriptor for port with the highest revision */
>> +	for (i = 0; i < xhci->num_port_caps; i++) {
>> +		u8 major = xhci->port_caps[i].maj_rev;
>> +		u8 minor = xhci->port_caps[i].min_rev;
>> +		u16 rev = (major << 8) | minor;
>> +
>> +		if (i == 0 || bcdUSB < rev) {
>> +			bcdUSB = rev;
>> +			port_cap = &xhci->port_caps[i];
>> +		}
>> +	}
>> +
>> +	if (bcdUSB >= 0x0310) {
>> +		/* Two SSA entries for each unique PSI ID, RX and TX */
>> +		if (port_cap->psi_count)
>> +			ssac = port_cap->psi_uid_count * 2 - 1;
>> +		else if (bcdUSB == 0x0320)
> maybe (bcdUSB >= 0x320), allows old driver to handle future hardware.

Ok.

>
>> +			ssac = 7;
>> +		else
>> +			ssac = 3;
>> +
>> +		if (port_cap->psi_count)
>> +			ssic = port_cap->psi_count - 1;
>> +		else
>> +			ssic = (ssac + 1) / 2 - 1;
>> +
>> +		bos->bNumDeviceCaps++;
>> +		bos->wTotalLength = cpu_to_le16(USB_DT_BOS_SIZE +
>> +						USB_DT_USB_SS_CAP_SIZE +
>> +						USB_DT_USB_SSP_CAP_SIZE(ssac));
>> +	}
>> +
>> +	if (wLength < USB_DT_BOS_SIZE + USB_DT_USB_SS_CAP_SIZE)
>> +		return wLength;
>> +
>> +	/* SuperSpeed USB Device Capability */
>> +	ss_cap = (struct usb_ss_cap_descriptor *)&buf[USB_DT_BOS_SIZE];
>> +	ss_cap->bLength = USB_DT_USB_SS_CAP_SIZE;
>> +	ss_cap->bDescriptorType = USB_DT_DEVICE_CAPABILITY;
>> +	ss_cap->bDevCapabilityType = USB_SS_CAP_TYPE;
>> +	ss_cap->bmAttributes = 0; /* set later */
>> +	ss_cap->wSpeedSupported = cpu_to_le16(USB_5GBPS_OPERATION);
>> +	ss_cap->bFunctionalitySupport = USB_LOW_SPEED_OPERATION;
>> +	ss_cap->bU1devExitLat = 0; /* set later */
>> +	ss_cap->bU2DevExitLat = 0; /* set later */
>> +
>> +	reg = readl(&xhci->cap_regs->hcc_params);
>> +	if (HCC_LTC(reg))
>> +		ss_cap->bmAttributes |= USB_LTM_SUPPORT;
>> +
>> +	if ((xhci->quirks & XHCI_LPM_SUPPORT)) {
>> +		reg = readl(&xhci->cap_regs->hcs_params3);
>> +		ss_cap->bU1devExitLat = HCS_U1_LATENCY(reg);
>> +		ss_cap->bU2DevExitLat = cpu_to_le16(HCS_U2_LATENCY(reg));
>> +	}
>> +
>> +	if (wLength < le16_to_cpu(bos->wTotalLength))
>> +		return wLength;
>> +
>> +	if (bcdUSB < 0x0310)
>> +		return le16_to_cpu(bos->wTotalLength);
>> +
>> +	ssp_cap = (struct usb_ssp_cap_descriptor *)&buf[USB_DT_BOS_SIZE +
>> +		USB_DT_USB_SS_CAP_SIZE];
>> +	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, ssac) |
>> +			    FIELD_PREP(USB_SSP_SUBLINK_SPEED_IDS, ssic));
>> +
>> +	if (!port_cap->psi_count) {
> for (i = 0; i < ssac + 1; i++)
> 	ssp_cap->mbSublinkSpeedAttr[i] = ssp_cap_default_ssa[i];  //mind endianness
>
>> +		int ret;
>> +
>> +		ret = xhci_fill_default_ssp_attr(xhci, ssp_cap);
>> +		if (ret)
>> +			return ret;
>> +
>> +		min_ssid = 4;
>> +		goto out;
>> +	}
>> +
>> +	offset = 0;
>> +	for (i = 0; i < port_cap->psi_count; i++) {
> I need to take a better look at this sublinkSpeedAttr[] creation from custom PSIC table a bit later.
>
> Thanks
> -Mathias

Thanks for the review!

Thinh

RE: [RFC PATCH 6/8] usb: xhci: Fix port minor revision

[David Laight]

From: Thinh Nguyen
> Sent: 03 February 2021 01:16
> 
> Sergei Shtylyov wrote:
> > Hello!
> >
> > On 02.02.2021 6:42, Thinh Nguyen wrote:
> >
> >> Some hosts incorrectly use sub-minor version for minor version (i.e.
> >> 0x02 instead of 0x20 for bcdUSB 0x320 and 0x01 for bcdUSB 0x310).
> >> Currently the xHCI driver works around this by just checking for minor
> >> revision > 0x01 for USB 3.1 everywhere. With the addition of USB 3.2,
> >> checking this gets a bit cumbersome. Since there is no USB release with
> >> bcdUSB 0x301 to 0x309, we can assume that sub-minor version 01 to 09 is
> >> incorrect. Let's try to fix this and use the minor revision that matches
> >> with the USB/xHCI spec to help with the version checking within the
> >> driver.
> >>
> >> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> >> ---
> >>   drivers/usb/host/xhci-mem.c | 9 +++++++++
> >>   1 file changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> >> index f2c4ee7c4786..34105b477c62 100644
> >> --- a/drivers/usb/host/xhci-mem.c
> >> +++ b/drivers/usb/host/xhci-mem.c
> >> @@ -2129,6 +2129,15 @@ static void xhci_add_in_port(struct xhci_hcd
> >> *xhci, unsigned int num_ports,
> >>         if (major_revision == 0x03) {
> >>           rhub = &xhci->usb3_rhub;
> >> +        /*
> >> +         * Some hosts incorrectly use sub-minor version for minor
> >> +         * version (i.e. 0x02 instead of 0x20 for bcdUSB 0x320 and 0x01
> >> +         * for bcdUSB 0x310). Since there is no USB release with sub
> >> +         * minor version 0x301 to 0x309, we can assume that they are
> >> +         * incorrect and fix it here.
> >> +         */
> >> +        if (!(minor_revision & 0xf0) && (minor_revision & 0x0f))
> >> +            minor_revision = minor_revision << 4;
> >
> >    Why not:
> >
> >             minor_revision <<= 4;
> >
> > [...]
> >
> > MBR, Sergei
> 
> Sure, we can do that.

Isn't it just:
		if (minor_revision < 0x10)
			minor_revision <<= 4;

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [RFC PATCH 6/8] usb: xhci: Fix port minor revision

[Thinh Nguyen]

David Laight wrote:
> From: Thinh Nguyen
>> Sent: 03 February 2021 01:16
>>
>> Sergei Shtylyov wrote:
>>> Hello!
>>>
>>> On 02.02.2021 6:42, Thinh Nguyen wrote:
>>>
>>>> Some hosts incorrectly use sub-minor version for minor version (i.e.
>>>> 0x02 instead of 0x20 for bcdUSB 0x320 and 0x01 for bcdUSB 0x310).
>>>> Currently the xHCI driver works around this by just checking for minor
>>>> revision > 0x01 for USB 3.1 everywhere. With the addition of USB 3.2,
>>>> checking this gets a bit cumbersome. Since there is no USB release with
>>>> bcdUSB 0x301 to 0x309, we can assume that sub-minor version 01 to 09 is
>>>> incorrect. Let's try to fix this and use the minor revision that matches
>>>> with the USB/xHCI spec to help with the version checking within the
>>>> driver.
>>>>
>>>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>>> ---
>>>>   drivers/usb/host/xhci-mem.c | 9 +++++++++
>>>>   1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>>>> index f2c4ee7c4786..34105b477c62 100644
>>>> --- a/drivers/usb/host/xhci-mem.c
>>>> +++ b/drivers/usb/host/xhci-mem.c
>>>> @@ -2129,6 +2129,15 @@ static void xhci_add_in_port(struct xhci_hcd
>>>> *xhci, unsigned int num_ports,
>>>>         if (major_revision == 0x03) {
>>>>           rhub = &xhci->usb3_rhub;
>>>> +        /*
>>>> +         * Some hosts incorrectly use sub-minor version for minor
>>>> +         * version (i.e. 0x02 instead of 0x20 for bcdUSB 0x320 and 0x01
>>>> +         * for bcdUSB 0x310). Since there is no USB release with sub
>>>> +         * minor version 0x301 to 0x309, we can assume that they are
>>>> +         * incorrect and fix it here.
>>>> +         */
>>>> +        if (!(minor_revision & 0xf0) && (minor_revision & 0x0f))
>>>> +            minor_revision = minor_revision << 4;
>>>    Why not:
>>>
>>>             minor_revision <<= 4;
>>>
>>> [...]
>>>
>>> MBR, Sergei
>> Sure, we can do that.
> Isn't it just:
> 		if (minor_revision < 0x10)
> 			minor_revision <<= 4;
>
> 	David
>
>

Ah.... Not sure what I was thinking when I made that roundabout check.
Thanks for pointing it out.

Thinh

Re: [RFC PATCH 6/8] usb: xhci: Fix port minor revision

[Thinh Nguyen]

Thinh Nguyen wrote:
> David Laight wrote:
>> From: Thinh Nguyen
>>> Sent: 03 February 2021 01:16
>>>
>>> Sergei Shtylyov wrote:
>>>> Hello!
>>>>
>>>> On 02.02.2021 6:42, Thinh Nguyen wrote:
>>>>
>>>>> Some hosts incorrectly use sub-minor version for minor version (i.e.
>>>>> 0x02 instead of 0x20 for bcdUSB 0x320 and 0x01 for bcdUSB 0x310).
>>>>> Currently the xHCI driver works around this by just checking for minor
>>>>> revision > 0x01 for USB 3.1 everywhere. With the addition of USB 3.2,
>>>>> checking this gets a bit cumbersome. Since there is no USB release with
>>>>> bcdUSB 0x301 to 0x309, we can assume that sub-minor version 01 to 09 is
>>>>> incorrect. Let's try to fix this and use the minor revision that matches
>>>>> with the USB/xHCI spec to help with the version checking within the
>>>>> driver.
>>>>>
>>>>> Signed-off-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>>>> ---
>>>>>   drivers/usb/host/xhci-mem.c | 9 +++++++++
>>>>>   1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>>>>> index f2c4ee7c4786..34105b477c62 100644
>>>>> --- a/drivers/usb/host/xhci-mem.c
>>>>> +++ b/drivers/usb/host/xhci-mem.c
>>>>> @@ -2129,6 +2129,15 @@ static void xhci_add_in_port(struct xhci_hcd
>>>>> *xhci, unsigned int num_ports,
>>>>>         if (major_revision == 0x03) {
>>>>>           rhub = &xhci->usb3_rhub;
>>>>> +        /*
>>>>> +         * Some hosts incorrectly use sub-minor version for minor
>>>>> +         * version (i.e. 0x02 instead of 0x20 for bcdUSB 0x320 and 0x01
>>>>> +         * for bcdUSB 0x310). Since there is no USB release with sub
>>>>> +         * minor version 0x301 to 0x309, we can assume that they are
>>>>> +         * incorrect and fix it here.
>>>>> +         */
>>>>> +        if (!(minor_revision & 0xf0) && (minor_revision & 0x0f))
>>>>> +            minor_revision = minor_revision << 4;
>>>>    Why not:
>>>>
>>>>             minor_revision <<= 4;
>>>>
>>>> [...]
>>>>
>>>> MBR, Sergei
>>> Sure, we can do that.
>> Isn't it just:
>> 		if (minor_revision < 0x10)
>> 			minor_revision <<= 4;
>>
>> 	David
>>
>>
> Ah.... Not sure what I was thinking when I made that roundabout check.
> Thanks for pointing it out.

Btw, the intention is this:
if (minor_revision >  0x00 && minor_revision < 0x10) ...

It's much clearer this way. I'll make the change after more feedbacks on
other patches.

BR,
Thinh