Re: USB 3.2 Gen 2x2 "Superspeed+20GBps" support for ASM3242

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

youling 257 wrote:
> please you subject a patch to linux-usb.
> 
I'll take a look.

> 2021-12-31 9:39 GMT+08:00, Thinh Nguyen <Thinh.Nguyen@synopsys.com>:
>> The ASmedia host controller incorrectly reports the speed ID in the
>> port-status mismatching with its PSI capability for SSP devices. As
>> a result, the host/hub driver will report the wrong speed.
>>
>> To resolve/workaround this, the xHCI driver can capture the device speed
>> from sublink speed notification of a SSP device. All SSP devices must
>> send sublink speed device notification, so this method should resolve
>> your issue.
>>
>> You can apply the change below. This should resolve your issue.
>>
>> BR,
>> Thinh
>>
>>
>> diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
>> index 9ddcc0ab4db7..6de15f004684 100644
>> --- a/drivers/usb/host/xhci-mem.c
>> +++ b/drivers/usb/host/xhci-mem.c
>> @@ -2602,7 +2602,7 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
>>  	 */
>>  	temp = readl(&xhci->op_regs->dev_notification);
>>  	temp &= ~DEV_NOTE_MASK;
>> -	temp |= DEV_NOTE_FWAKE;
>> +	temp |= DEV_NOTE_FWAKE | DEV_NOTE_SUBLINK_SPEED;
>>  	writel(temp, &xhci->op_regs->dev_notification);
>>
>>  	return 0;
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index 99d9d9c88988..80081b3fd52a 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -1860,6 +1860,8 @@ static void handle_device_notification(struct xhci_hcd
>> *xhci,
>>  {
>>  	u32 slot_id;
>>  	struct usb_device *udev;
>> +	u32 type;
>> +	u32 dn;
>>
>>  	slot_id = TRB_TO_SLOT_ID(le32_to_cpu(event->generic.field[3]));
>>  	if (!xhci->devs[slot_id]) {
>> @@ -1868,11 +1870,45 @@ static void handle_device_notification(struct
>> xhci_hcd *xhci,
>>  		return;
>>  	}
>>
>> -	xhci_dbg(xhci, "Device Wake Notification event for slot ID %u\n",
>> -			slot_id);
>>  	udev = xhci->devs[slot_id]->udev;
>> -	if (udev && udev->parent)
>> -		usb_wakeup_notification(udev->parent, udev->portnum);
>> +	type = TRB_DN_TYPE(le32_to_cpu(event->generic.field[0]));
>> +
>> +	switch (type) {
>> +	case TRB_DN_TYPE_FUNC_WAKE:
>> +		xhci_info(xhci, "Device Wake Notification event for slot ID %u\n",
>> +			  slot_id);
>> +		if (udev && udev->parent)
>> +			usb_wakeup_notification(udev->parent, udev->portnum);
>> +		break;
>> +	case TRB_DN_TYPE_SUBLINK_SPEED:
>> +		if (!udev)
>> +			break;
>> +
>> +		dn = le32_to_cpu(event->generic.field[1]);
>> +		udev->ssp_rate = USB_SSP_GEN_UNKNOWN;
>> +
>> +		if (TRB_DN_SUBLINK_SPEED_LP(dn) ==
>> +		    TRB_DN_SUBLINK_SPEED_LP_SSP) {
>> +			udev->speed = USB_SPEED_SUPER_PLUS;
>> +
>> +			if (TRB_DN_SUBLINK_SPEED_LSE(dn) !=
>> +			    TRB_DN_SUBLINK_SPEED_LSE_GBPS)
>> +				break;
>> +
>> +			if (TRB_DN_SUBLINK_SPEED_LSM(dn) == 10) {
>> +				if (TRB_DN_SUBLINK_SPEED_LANES(dn))
>> +					udev->ssp_rate = USB_SSP_GEN_2x2;
>> +				else
>> +					udev->ssp_rate = USB_SSP_GEN_2x1;
>> +			} else if (TRB_DN_SUBLINK_SPEED_LSM(dn) == 5) {
>> +				if (TRB_DN_SUBLINK_SPEED_LANES(dn))
>> +					udev->ssp_rate = USB_SSP_GEN_1x2;
>> +			}
>> +		} else {
>> +			udev->speed = USB_SPEED_SUPER;
>> +		}
>> +		break;
>> +	}
>>  }
>>
>>  /*
>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> index 9192465fd5f3..ce2ca67c115f 100644
>> --- a/drivers/usb/host/xhci.h
>> +++ b/drivers/usb/host/xhci.h
>> @@ -267,6 +267,7 @@ struct xhci_op_regs {
>>   * SW does need to pay attention to function wake notifications.
>>   */
>>  #define	DEV_NOTE_FWAKE		ENABLE_DEV_NOTE(1)
>> +#define	DEV_NOTE_SUBLINK_SPEED	ENABLE_DEV_NOTE(5)
>>
>>  /* CRCR - Command Ring Control Register - cmd_ring bitmasks */
>>  /* bit 0 is the command ring cycle state */
>> @@ -1434,6 +1435,30 @@ union xhci_trb {
>>  /* Get NEC firmware revision. */
>>  #define	TRB_NEC_GET_FW		49
>>
>> +/* Get Device Notification type */
>> +#define TRB_DN_TYPE(p)			(((p) >> 4) & 0xf)
>> +
>> +#define TRB_DN_TYPE_FUNC_WAKE		1
>> +#define TRB_DN_TYPE_SUBLINK_SPEED	5
>> +
>> +/* Get sublink speed attributes */
>> +#define TRB_DN_SUBLINK_SPEED_LSE(p)	(((p) >> 4) & 0x3)
>> +#define TRB_DN_SUBLINK_SPEED_LSE_BPS	0
>> +#define TRB_DN_SUBLINK_SPEED_LSE_KBPS	1
>> +#define TRB_DN_SUBLINK_SPEED_LSE_MBPS	2
>> +#define TRB_DN_SUBLINK_SPEED_LSE_GBPS	3
>> +#define TRB_DN_SUBLINK_SPEED_ST(p)	(((p) >> 6) & 0x3)
>> +#define TRB_DN_SUBLINK_SPEED_LANES(p)	(((p) >> 10) & 0xf)
>> +#define TRB_DN_SUBLINK_SPEED_LP(p)	(((p) >> 14) & 0x3)
>> +#define TRB_DN_SUBLINK_SPEED_LP_SS	0
>> +#define TRB_DN_SUBLINK_SPEED_LP_SSP	1
>> +#define TRB_DN_SUBLINK_SPEED_LSM(p)	(((p) >> 16) & 0xffff)
>> +
>> +#define TRB_DN_SUBLINK_SPEED_IS_SYMMETRIC(p) \
>> +	(!(TRB_DN_SUBLINK_SPEED_ST(p) & BIT(0)))
>> +#define TRB_DN_SUBLINK_SPEED_IS_TX(p) \
>> +	(!!(TRB_DN_SUBLINK_SPEED_ST(p) & BIT(1)))
>> +
>>  static inline const char *xhci_trb_type_string(u8 type)
>>  {
>>  	switch (type) {
>>
>>
>>
>>
>>


Hi Mathias/all,

Some ASmedia hosts have issue with reporting correct port speed ID for
devices operating in SSP. We can workaround this by getting the device
speed from the device notification sublink speed instead.

The question here is whether we should check speed from device
notification (SSP only) or do we want to selectively set quirk for
ASmedia hosts only. Should we trust a device response more or the host more?

The change above that Youling tested overrides speed detected from port
status with device notification sublink speed.

Thanks,
Thinh