[PATCH] USB: core: Add eUSB2 descriptor and parsing in USB core

[PATCH] USB: core: Add eUSB2 descriptor and parsing in USB core

[Mathias Nyman]

From: Kannappan R <r.kannappan@intel.com>

Add support for the 'eUSB2 Isochronous Endpoint Companion Descriptor'
introduced in the recent USB 2.0 specification 'USB 2.0 Double Isochronous
IN Bandwidth' ECN.

It allows embedded USB2 (eUSB2) devices to report and use higher bandwidths
for isochronous IN transfers in order to support higher camera resolutions
on the lid of laptops and tablets with minimal change to the USB2 protocol.

The motivation for expanding USB 2.0 is further clarified in an additional
Embedded USB2 version 2.0 (eUSB2v2) supplement to the USB 2.0
specification. It points out this is optimized for performance, power and
cost by using the USB 2.0 low-voltage, power efficient PHY and half-duplex
link for the asymmetric camera bandwidth needs, avoiding the costly and
complex full-duplex USB 3.x symmetric link and gigabit receivers.

eUSB2 devices that support the higher isochronous IN bandwidth and the new
descriptor can be identified by their device descriptor bcdUSB value of
0x0220

Co-developed-by: Amardeep Rai <amardeep.rai@intel.com>
Signed-off-by: Amardeep Rai <amardeep.rai@intel.com>
Signed-off-by: Kannappan R <r.kannappan@intel.com>
Co-developed-by: Mathias Nyman <mathias.nyman@linux.intel.com>
Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
---
 drivers/usb/core/config.c    | 51 ++++++++++++++++++++++++++++++++----
 include/linux/usb.h          |  8 +++---
 include/uapi/linux/usb/ch9.h | 15 +++++++++++
 3 files changed, 66 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
index f7bf8d1de3ad..13bd4ec4ea5f 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -64,6 +64,37 @@ static void usb_parse_ssp_isoc_endpoint_companion(struct device *ddev,
 	memcpy(&ep->ssp_isoc_ep_comp, desc, USB_DT_SSP_ISOC_EP_COMP_SIZE);
 }
 
+static void usb_parse_eusb2_isoc_endpoint_companion(struct device *ddev,
+		int cfgno, int inum, int asnum, struct usb_host_endpoint *ep,
+		unsigned char *buffer, int size)
+{
+	struct usb_eusb2_isoc_ep_comp_descriptor *desc;
+	struct usb_descriptor_header *h;
+
+	/*
+	 * eUSB2 isochronous endpoint companion descriptor for this endpoint
+	 * shall be declared before the next endpoint or interface descriptor
+	 */
+	while (size >= USB_DT_EUSB2_ISOC_EP_COMP_SIZE) {
+		h = (struct usb_descriptor_header *)buffer;
+
+		if (h->bDescriptorType == USB_DT_EUSB2_ISOC_ENDPOINT_COMP) {
+			desc = (struct usb_eusb2_isoc_ep_comp_descriptor *)buffer;
+			ep->eusb2_isoc_ep_comp = *desc;
+			return;
+		}
+		if (h->bDescriptorType == USB_DT_ENDPOINT ||
+		    h->bDescriptorType == USB_DT_INTERFACE)
+			break;
+
+		buffer += h->bLength;
+		size -= h->bLength;
+	}
+
+	dev_notice(ddev, "No eUSB2 isoc ep %d companion for config %d interface %d altsetting %d\n",
+		   ep->desc.bEndpointAddress, cfgno, inum, asnum);
+}
+
 static void usb_parse_ss_endpoint_companion(struct device *ddev, int cfgno,
 		int inum, int asnum, struct usb_host_endpoint *ep,
 		unsigned char *buffer, int size)
@@ -258,8 +289,10 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
 	int n, i, j, retval;
 	unsigned int maxp;
 	const unsigned short *maxpacket_maxes;
+	u16 bcdUSB;
 
 	d = (struct usb_endpoint_descriptor *) buffer;
+	bcdUSB = le16_to_cpu(udev->descriptor.bcdUSB);
 	buffer += d->bLength;
 	size -= d->bLength;
 
@@ -409,15 +442,17 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
 
 	/*
 	 * Validate the wMaxPacketSize field.
-	 * Some devices have isochronous endpoints in altsetting 0;
-	 * the USB-2 spec requires such endpoints to have wMaxPacketSize = 0
-	 * (see the end of section 5.6.3), so don't warn about them.
+	 * eUSB2 devices (see USB 2.0 Double Isochronous IN ECN 9.6.6 Endpoint)
+	 * and devices with isochronous endpoints in altsetting 0 (see USB 2.0
+	 * end of section 5.6.3) have wMaxPacketSize = 0.
+	 * So don't warn about those.
 	 */
 	maxp = le16_to_cpu(endpoint->desc.wMaxPacketSize);
-	if (maxp == 0 && !(usb_endpoint_xfer_isoc(d) && asnum == 0)) {
+
+	if (maxp == 0 && bcdUSB != 0x0220 &&
+	    !(usb_endpoint_xfer_isoc(d) && asnum == 0))
 		dev_notice(ddev, "config %d interface %d altsetting %d endpoint 0x%X has invalid wMaxPacketSize 0\n",
 		    cfgno, inum, asnum, d->bEndpointAddress);
-	}
 
 	/* Find the highest legal maxpacket size for this endpoint */
 	i = 0;		/* additional transactions per microframe */
@@ -465,6 +500,12 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
 				maxp);
 	}
 
+	/* Parse a possible eUSB2 periodic endpoint companion descriptor */
+	if (bcdUSB == 0x0220 && d->wMaxPacketSize == 0 &&
+	    (usb_endpoint_xfer_isoc(d) || usb_endpoint_xfer_int(d)))
+		usb_parse_eusb2_isoc_endpoint_companion(ddev, cfgno, inum, asnum,
+							endpoint, buffer, size);
+
 	/* Parse a possible SuperSpeed endpoint companion descriptor */
 	if (udev->speed >= USB_SPEED_SUPER)
 		usb_parse_ss_endpoint_companion(ddev, cfgno,
diff --git a/include/linux/usb.h b/include/linux/usb.h
index cfa8005e24f9..b46738701f8d 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -51,6 +51,7 @@ struct ep_device;
  * @desc: descriptor for this endpoint, wMaxPacketSize in native byteorder
  * @ss_ep_comp: SuperSpeed companion descriptor for this endpoint
  * @ssp_isoc_ep_comp: SuperSpeedPlus isoc companion descriptor for this endpoint
+ * @eusb2_isoc_ep_comp: eUSB2 isoc companion descriptor for this endpoint
  * @urb_list: urbs queued to this endpoint; maintained by usbcore
  * @hcpriv: for use by HCD; typically holds hardware dma queue head (QH)
  *	with one or more transfer descriptors (TDs) per urb
@@ -64,9 +65,10 @@ struct ep_device;
  * descriptor within an active interface in a given USB configuration.
  */
 struct usb_host_endpoint {
-	struct usb_endpoint_descriptor		desc;
-	struct usb_ss_ep_comp_descriptor	ss_ep_comp;
-	struct usb_ssp_isoc_ep_comp_descriptor	ssp_isoc_ep_comp;
+	struct usb_endpoint_descriptor			desc;
+	struct usb_ss_ep_comp_descriptor		ss_ep_comp;
+	struct usb_ssp_isoc_ep_comp_descriptor		ssp_isoc_ep_comp;
+	struct usb_eusb2_isoc_ep_comp_descriptor	eusb2_isoc_ep_comp;
 	struct list_head		urb_list;
 	void				*hcpriv;
 	struct ep_device		*ep_dev;	/* For sysfs info */
diff --git a/include/uapi/linux/usb/ch9.h b/include/uapi/linux/usb/ch9.h
index 91f0f7e214a5..475af9358173 100644
--- a/include/uapi/linux/usb/ch9.h
+++ b/include/uapi/linux/usb/ch9.h
@@ -253,6 +253,9 @@ struct usb_ctrlrequest {
 #define USB_DT_BOS			0x0f
 #define USB_DT_DEVICE_CAPABILITY	0x10
 #define USB_DT_WIRELESS_ENDPOINT_COMP	0x11
+/* From the eUSB2 spec */
+#define USB_DT_EUSB2_ISOC_ENDPOINT_COMP	0x12
+/* From Wireless USB spec */
 #define USB_DT_WIRE_ADAPTER		0x21
 /* From USB Device Firmware Upgrade Specification, Revision 1.1 */
 #define USB_DT_DFU_FUNCTIONAL		0x21
@@ -675,6 +678,18 @@ static inline int usb_endpoint_interrupt_type(
 
 /*-------------------------------------------------------------------------*/
 
+/* USB_DT_EUSB2_ISOC_ENDPOINT_COMP: eUSB2 Isoch Endpoint Companion descriptor */
+struct usb_eusb2_isoc_ep_comp_descriptor {
+	__u8	bLength;
+	__u8	bDescriptorType;
+	__le16	wMaxPacketSize;
+	__le32	dwBytesPerInterval;
+} __attribute__ ((packed));
+
+#define USB_DT_EUSB2_ISOC_EP_COMP_SIZE	8
+
+/*-------------------------------------------------------------------------*/
+
 /* USB_DT_SSP_ISOC_ENDPOINT_COMP: SuperSpeedPlus Isochronous Endpoint Companion
  * descriptor
  */
-- 
2.43.0

Re: [PATCH] USB: core: Add eUSB2 descriptor and parsing in USB core

[Alan Stern]

On Thu, Feb 20, 2025 at 04:13:39PM +0200, Mathias Nyman wrote:
> From: Kannappan R <r.kannappan@intel.com>
> 
> Add support for the 'eUSB2 Isochronous Endpoint Companion Descriptor'
> introduced in the recent USB 2.0 specification 'USB 2.0 Double Isochronous
> IN Bandwidth' ECN.
> 
> It allows embedded USB2 (eUSB2) devices to report and use higher bandwidths
> for isochronous IN transfers in order to support higher camera resolutions
> on the lid of laptops and tablets with minimal change to the USB2 protocol.
> 
> The motivation for expanding USB 2.0 is further clarified in an additional
> Embedded USB2 version 2.0 (eUSB2v2) supplement to the USB 2.0
> specification. It points out this is optimized for performance, power and
> cost by using the USB 2.0 low-voltage, power efficient PHY and half-duplex
> link for the asymmetric camera bandwidth needs, avoiding the costly and
> complex full-duplex USB 3.x symmetric link and gigabit receivers.
> 
> eUSB2 devices that support the higher isochronous IN bandwidth and the new
> descriptor can be identified by their device descriptor bcdUSB value of
> 0x0220
> 
> Co-developed-by: Amardeep Rai <amardeep.rai@intel.com>
> Signed-off-by: Amardeep Rai <amardeep.rai@intel.com>
> Signed-off-by: Kannappan R <r.kannappan@intel.com>
> Co-developed-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Re: [PATCH] USB: core: Add eUSB2 descriptor and parsing in USB core

[Greg KH]

On Thu, Feb 20, 2025 at 04:13:39PM +0200, Mathias Nyman wrote:
> From: Kannappan R <r.kannappan@intel.com>
> 
> Add support for the 'eUSB2 Isochronous Endpoint Companion Descriptor'
> introduced in the recent USB 2.0 specification 'USB 2.0 Double Isochronous
> IN Bandwidth' ECN.
> 
> It allows embedded USB2 (eUSB2) devices to report and use higher bandwidths
> for isochronous IN transfers in order to support higher camera resolutions
> on the lid of laptops and tablets with minimal change to the USB2 protocol.
> 
> The motivation for expanding USB 2.0 is further clarified in an additional
> Embedded USB2 version 2.0 (eUSB2v2) supplement to the USB 2.0
> specification. It points out this is optimized for performance, power and
> cost by using the USB 2.0 low-voltage, power efficient PHY and half-duplex
> link for the asymmetric camera bandwidth needs, avoiding the costly and
> complex full-duplex USB 3.x symmetric link and gigabit receivers.
> 
> eUSB2 devices that support the higher isochronous IN bandwidth and the new
> descriptor can be identified by their device descriptor bcdUSB value of
> 0x0220
> 
> Co-developed-by: Amardeep Rai <amardeep.rai@intel.com>
> Signed-off-by: Amardeep Rai <amardeep.rai@intel.com>
> Signed-off-by: Kannappan R <r.kannappan@intel.com>
> Co-developed-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/core/config.c    | 51 ++++++++++++++++++++++++++++++++----
>  include/linux/usb.h          |  8 +++---
>  include/uapi/linux/usb/ch9.h | 15 +++++++++++
>  3 files changed, 66 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> index f7bf8d1de3ad..13bd4ec4ea5f 100644
> --- a/drivers/usb/core/config.c
> +++ b/drivers/usb/core/config.c
> @@ -64,6 +64,37 @@ static void usb_parse_ssp_isoc_endpoint_companion(struct device *ddev,
>  	memcpy(&ep->ssp_isoc_ep_comp, desc, USB_DT_SSP_ISOC_EP_COMP_SIZE);
>  }
>  
> +static void usb_parse_eusb2_isoc_endpoint_companion(struct device *ddev,
> +		int cfgno, int inum, int asnum, struct usb_host_endpoint *ep,
> +		unsigned char *buffer, int size)
> +{
> +	struct usb_eusb2_isoc_ep_comp_descriptor *desc;
> +	struct usb_descriptor_header *h;
> +
> +	/*
> +	 * eUSB2 isochronous endpoint companion descriptor for this endpoint
> +	 * shall be declared before the next endpoint or interface descriptor
> +	 */
> +	while (size >= USB_DT_EUSB2_ISOC_EP_COMP_SIZE) {
> +		h = (struct usb_descriptor_header *)buffer;
> +
> +		if (h->bDescriptorType == USB_DT_EUSB2_ISOC_ENDPOINT_COMP) {
> +			desc = (struct usb_eusb2_isoc_ep_comp_descriptor *)buffer;
> +			ep->eusb2_isoc_ep_comp = *desc;
> +			return;
> +		}
> +		if (h->bDescriptorType == USB_DT_ENDPOINT ||
> +		    h->bDescriptorType == USB_DT_INTERFACE)
> +			break;
> +
> +		buffer += h->bLength;
> +		size -= h->bLength;
> +	}
> +
> +	dev_notice(ddev, "No eUSB2 isoc ep %d companion for config %d interface %d altsetting %d\n",
> +		   ep->desc.bEndpointAddress, cfgno, inum, asnum);
> +}
> +
>  static void usb_parse_ss_endpoint_companion(struct device *ddev, int cfgno,
>  		int inum, int asnum, struct usb_host_endpoint *ep,
>  		unsigned char *buffer, int size)
> @@ -258,8 +289,10 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
>  	int n, i, j, retval;
>  	unsigned int maxp;
>  	const unsigned short *maxpacket_maxes;
> +	u16 bcdUSB;
>  
>  	d = (struct usb_endpoint_descriptor *) buffer;
> +	bcdUSB = le16_to_cpu(udev->descriptor.bcdUSB);
>  	buffer += d->bLength;
>  	size -= d->bLength;
>  
> @@ -409,15 +442,17 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
>  
>  	/*
>  	 * Validate the wMaxPacketSize field.
> -	 * Some devices have isochronous endpoints in altsetting 0;
> -	 * the USB-2 spec requires such endpoints to have wMaxPacketSize = 0
> -	 * (see the end of section 5.6.3), so don't warn about them.
> +	 * eUSB2 devices (see USB 2.0 Double Isochronous IN ECN 9.6.6 Endpoint)
> +	 * and devices with isochronous endpoints in altsetting 0 (see USB 2.0
> +	 * end of section 5.6.3) have wMaxPacketSize = 0.
> +	 * So don't warn about those.
>  	 */
>  	maxp = le16_to_cpu(endpoint->desc.wMaxPacketSize);
> -	if (maxp == 0 && !(usb_endpoint_xfer_isoc(d) && asnum == 0)) {
> +
> +	if (maxp == 0 && bcdUSB != 0x0220 &&

You use "0x0220" at least twice here, should it be defined to show what
it really means?

> +	    !(usb_endpoint_xfer_isoc(d) && asnum == 0))
>  		dev_notice(ddev, "config %d interface %d altsetting %d endpoint 0x%X has invalid wMaxPacketSize 0\n",
>  		    cfgno, inum, asnum, d->bEndpointAddress);
> -	}
>  
>  	/* Find the highest legal maxpacket size for this endpoint */
>  	i = 0;		/* additional transactions per microframe */
> @@ -465,6 +500,12 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
>  				maxp);
>  	}
>  
> +	/* Parse a possible eUSB2 periodic endpoint companion descriptor */
> +	if (bcdUSB == 0x0220 && d->wMaxPacketSize == 0 &&
> +	    (usb_endpoint_xfer_isoc(d) || usb_endpoint_xfer_int(d)))
> +		usb_parse_eusb2_isoc_endpoint_companion(ddev, cfgno, inum, asnum,
> +							endpoint, buffer, size);
> +
>  	/* Parse a possible SuperSpeed endpoint companion descriptor */
>  	if (udev->speed >= USB_SPEED_SUPER)
>  		usb_parse_ss_endpoint_companion(ddev, cfgno,
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index cfa8005e24f9..b46738701f8d 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -51,6 +51,7 @@ struct ep_device;
>   * @desc: descriptor for this endpoint, wMaxPacketSize in native byteorder
>   * @ss_ep_comp: SuperSpeed companion descriptor for this endpoint
>   * @ssp_isoc_ep_comp: SuperSpeedPlus isoc companion descriptor for this endpoint
> + * @eusb2_isoc_ep_comp: eUSB2 isoc companion descriptor for this endpoint
>   * @urb_list: urbs queued to this endpoint; maintained by usbcore
>   * @hcpriv: for use by HCD; typically holds hardware dma queue head (QH)
>   *	with one or more transfer descriptors (TDs) per urb
> @@ -64,9 +65,10 @@ struct ep_device;
>   * descriptor within an active interface in a given USB configuration.
>   */
>  struct usb_host_endpoint {
> -	struct usb_endpoint_descriptor		desc;
> -	struct usb_ss_ep_comp_descriptor	ss_ep_comp;
> -	struct usb_ssp_isoc_ep_comp_descriptor	ssp_isoc_ep_comp;
> +	struct usb_endpoint_descriptor			desc;
> +	struct usb_ss_ep_comp_descriptor		ss_ep_comp;
> +	struct usb_ssp_isoc_ep_comp_descriptor		ssp_isoc_ep_comp;
> +	struct usb_eusb2_isoc_ep_comp_descriptor	eusb2_isoc_ep_comp;

No real need to indent any of these, but oh well :)

>  	struct list_head		urb_list;
>  	void				*hcpriv;
>  	struct ep_device		*ep_dev;	/* For sysfs info */
> diff --git a/include/uapi/linux/usb/ch9.h b/include/uapi/linux/usb/ch9.h
> index 91f0f7e214a5..475af9358173 100644
> --- a/include/uapi/linux/usb/ch9.h
> +++ b/include/uapi/linux/usb/ch9.h
> @@ -253,6 +253,9 @@ struct usb_ctrlrequest {
>  #define USB_DT_BOS			0x0f
>  #define USB_DT_DEVICE_CAPABILITY	0x10
>  #define USB_DT_WIRELESS_ENDPOINT_COMP	0x11
> +/* From the eUSB2 spec */
> +#define USB_DT_EUSB2_ISOC_ENDPOINT_COMP	0x12
> +/* From Wireless USB spec */
>  #define USB_DT_WIRE_ADAPTER		0x21
>  /* From USB Device Firmware Upgrade Specification, Revision 1.1 */
>  #define USB_DT_DFU_FUNCTIONAL		0x21
> @@ -675,6 +678,18 @@ static inline int usb_endpoint_interrupt_type(
>  
>  /*-------------------------------------------------------------------------*/
>  
> +/* USB_DT_EUSB2_ISOC_ENDPOINT_COMP: eUSB2 Isoch Endpoint Companion descriptor */
> +struct usb_eusb2_isoc_ep_comp_descriptor {
> +	__u8	bLength;
> +	__u8	bDescriptorType;
> +	__le16	wMaxPacketSize;
> +	__le32	dwBytesPerInterval;
> +} __attribute__ ((packed));
> +
> +#define USB_DT_EUSB2_ISOC_EP_COMP_SIZE	8

Can't we use a sizeof() for this as well?  I guess we don't do it for
other structures, so maybe not.

Anyway, this looks fine, if you want to just send an update for the
0x0220 later on if you think it's needed, please do.

thanks,

greg k-h

Re: [PATCH] USB: core: Add eUSB2 descriptor and parsing in USB core

[Thinh Nguyen]

Hi,

On Thu, Feb 20, 2025, Mathias Nyman wrote:
> From: Kannappan R <r.kannappan@intel.com>
> 
> Add support for the 'eUSB2 Isochronous Endpoint Companion Descriptor'
> introduced in the recent USB 2.0 specification 'USB 2.0 Double Isochronous
> IN Bandwidth' ECN.
> 
> It allows embedded USB2 (eUSB2) devices to report and use higher bandwidths
> for isochronous IN transfers in order to support higher camera resolutions
> on the lid of laptops and tablets with minimal change to the USB2 protocol.
> 
> The motivation for expanding USB 2.0 is further clarified in an additional
> Embedded USB2 version 2.0 (eUSB2v2) supplement to the USB 2.0
> specification. It points out this is optimized for performance, power and
> cost by using the USB 2.0 low-voltage, power efficient PHY and half-duplex
> link for the asymmetric camera bandwidth needs, avoiding the costly and
> complex full-duplex USB 3.x symmetric link and gigabit receivers.
> 
> eUSB2 devices that support the higher isochronous IN bandwidth and the new
> descriptor can be identified by their device descriptor bcdUSB value of
> 0x0220

Isn't eUSB2v2 has bcdUSB value of 0x0230?

> 
> Co-developed-by: Amardeep Rai <amardeep.rai@intel.com>
> Signed-off-by: Amardeep Rai <amardeep.rai@intel.com>
> Signed-off-by: Kannappan R <r.kannappan@intel.com>
> Co-developed-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> ---
>  drivers/usb/core/config.c    | 51 ++++++++++++++++++++++++++++++++----
>  include/linux/usb.h          |  8 +++---
>  include/uapi/linux/usb/ch9.h | 15 +++++++++++
>  3 files changed, 66 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> index f7bf8d1de3ad..13bd4ec4ea5f 100644
> --- a/drivers/usb/core/config.c
> +++ b/drivers/usb/core/config.c
> @@ -64,6 +64,37 @@ static void usb_parse_ssp_isoc_endpoint_companion(struct device *ddev,
>  	memcpy(&ep->ssp_isoc_ep_comp, desc, USB_DT_SSP_ISOC_EP_COMP_SIZE);
>  }
>  
> +static void usb_parse_eusb2_isoc_endpoint_companion(struct device *ddev,
> +		int cfgno, int inum, int asnum, struct usb_host_endpoint *ep,
> +		unsigned char *buffer, int size)
> +{
> +	struct usb_eusb2_isoc_ep_comp_descriptor *desc;
> +	struct usb_descriptor_header *h;
> +
> +	/*
> +	 * eUSB2 isochronous endpoint companion descriptor for this endpoint
> +	 * shall be declared before the next endpoint or interface descriptor
> +	 */
> +	while (size >= USB_DT_EUSB2_ISOC_EP_COMP_SIZE) {
> +		h = (struct usb_descriptor_header *)buffer;
> +
> +		if (h->bDescriptorType == USB_DT_EUSB2_ISOC_ENDPOINT_COMP) {
> +			desc = (struct usb_eusb2_isoc_ep_comp_descriptor *)buffer;
> +			ep->eusb2_isoc_ep_comp = *desc;
> +			return;
> +		}
> +		if (h->bDescriptorType == USB_DT_ENDPOINT ||
> +		    h->bDescriptorType == USB_DT_INTERFACE)
> +			break;
> +
> +		buffer += h->bLength;
> +		size -= h->bLength;
> +	}
> +
> +	dev_notice(ddev, "No eUSB2 isoc ep %d companion for config %d interface %d altsetting %d\n",
> +		   ep->desc.bEndpointAddress, cfgno, inum, asnum);

Since eUSB2v2 devices should also include at least an alternate
interface with isoc endpoint descriptors using legacy settings, does the
spec require those legacy alternate interfaces to also have this isoc
companion descriptor?

> +}
> +
>  static void usb_parse_ss_endpoint_companion(struct device *ddev, int cfgno,
>  		int inum, int asnum, struct usb_host_endpoint *ep,
>  		unsigned char *buffer, int size)
> @@ -258,8 +289,10 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
>  	int n, i, j, retval;
>  	unsigned int maxp;
>  	const unsigned short *maxpacket_maxes;
> +	u16 bcdUSB;
>  
>  	d = (struct usb_endpoint_descriptor *) buffer;
> +	bcdUSB = le16_to_cpu(udev->descriptor.bcdUSB);
>  	buffer += d->bLength;
>  	size -= d->bLength;
>  
> @@ -409,15 +442,17 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
>  
>  	/*
>  	 * Validate the wMaxPacketSize field.
> -	 * Some devices have isochronous endpoints in altsetting 0;
> -	 * the USB-2 spec requires such endpoints to have wMaxPacketSize = 0
> -	 * (see the end of section 5.6.3), so don't warn about them.
> +	 * eUSB2 devices (see USB 2.0 Double Isochronous IN ECN 9.6.6 Endpoint)
> +	 * and devices with isochronous endpoints in altsetting 0 (see USB 2.0
> +	 * end of section 5.6.3) have wMaxPacketSize = 0.
> +	 * So don't warn about those.
>  	 */
>  	maxp = le16_to_cpu(endpoint->desc.wMaxPacketSize);
> -	if (maxp == 0 && !(usb_endpoint_xfer_isoc(d) && asnum == 0)) {
> +
> +	if (maxp == 0 && bcdUSB != 0x0220 &&
> +	    !(usb_endpoint_xfer_isoc(d) && asnum == 0))
>  		dev_notice(ddev, "config %d interface %d altsetting %d endpoint 0x%X has invalid wMaxPacketSize 0\n",
>  		    cfgno, inum, asnum, d->bEndpointAddress);
> -	}
>  
>  	/* Find the highest legal maxpacket size for this endpoint */
>  	i = 0;		/* additional transactions per microframe */
> @@ -465,6 +500,12 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
>  				maxp);
>  	}
>  
> +	/* Parse a possible eUSB2 periodic endpoint companion descriptor */
> +	if (bcdUSB == 0x0220 && d->wMaxPacketSize == 0 &&
> +	    (usb_endpoint_xfer_isoc(d) || usb_endpoint_xfer_int(d)))

Why are we checking for interrupt endpoint to parse for isoc?

> +		usb_parse_eusb2_isoc_endpoint_companion(ddev, cfgno, inum, asnum,
> +							endpoint, buffer, size);
> +
>  	/* Parse a possible SuperSpeed endpoint companion descriptor */
>  	if (udev->speed >= USB_SPEED_SUPER)
>  		usb_parse_ss_endpoint_companion(ddev, cfgno,
> diff --git a/include/linux/usb.h b/include/linux/usb.h
> index cfa8005e24f9..b46738701f8d 100644
> --- a/include/linux/usb.h
> +++ b/include/linux/usb.h
> @@ -51,6 +51,7 @@ struct ep_device;
>   * @desc: descriptor for this endpoint, wMaxPacketSize in native byteorder
>   * @ss_ep_comp: SuperSpeed companion descriptor for this endpoint
>   * @ssp_isoc_ep_comp: SuperSpeedPlus isoc companion descriptor for this endpoint
> + * @eusb2_isoc_ep_comp: eUSB2 isoc companion descriptor for this endpoint
>   * @urb_list: urbs queued to this endpoint; maintained by usbcore
>   * @hcpriv: for use by HCD; typically holds hardware dma queue head (QH)
>   *	with one or more transfer descriptors (TDs) per urb
> @@ -64,9 +65,10 @@ struct ep_device;
>   * descriptor within an active interface in a given USB configuration.
>   */
>  struct usb_host_endpoint {
> -	struct usb_endpoint_descriptor		desc;
> -	struct usb_ss_ep_comp_descriptor	ss_ep_comp;
> -	struct usb_ssp_isoc_ep_comp_descriptor	ssp_isoc_ep_comp;
> +	struct usb_endpoint_descriptor			desc;
> +	struct usb_ss_ep_comp_descriptor		ss_ep_comp;
> +	struct usb_ssp_isoc_ep_comp_descriptor		ssp_isoc_ep_comp;
> +	struct usb_eusb2_isoc_ep_comp_descriptor	eusb2_isoc_ep_comp;
>  	struct list_head		urb_list;
>  	void				*hcpriv;
>  	struct ep_device		*ep_dev;	/* For sysfs info */
> diff --git a/include/uapi/linux/usb/ch9.h b/include/uapi/linux/usb/ch9.h
> index 91f0f7e214a5..475af9358173 100644
> --- a/include/uapi/linux/usb/ch9.h
> +++ b/include/uapi/linux/usb/ch9.h
> @@ -253,6 +253,9 @@ struct usb_ctrlrequest {
>  #define USB_DT_BOS			0x0f
>  #define USB_DT_DEVICE_CAPABILITY	0x10
>  #define USB_DT_WIRELESS_ENDPOINT_COMP	0x11
> +/* From the eUSB2 spec */
> +#define USB_DT_EUSB2_ISOC_ENDPOINT_COMP	0x12
> +/* From Wireless USB spec */
>  #define USB_DT_WIRE_ADAPTER		0x21
>  /* From USB Device Firmware Upgrade Specification, Revision 1.1 */
>  #define USB_DT_DFU_FUNCTIONAL		0x21
> @@ -675,6 +678,18 @@ static inline int usb_endpoint_interrupt_type(
>  
>  /*-------------------------------------------------------------------------*/
>  
> +/* USB_DT_EUSB2_ISOC_ENDPOINT_COMP: eUSB2 Isoch Endpoint Companion descriptor */

Should we name this usb_hsx_isoc_ep_comp_descriptor for consistency?

Just bringing up the discussion here as I don't have a hard stance on
this. The naming of speeds and usb versions are getting more
inconsistent, and naming of these are getting more challenging. Not sure
if there will be something new in the future.

> +struct usb_eusb2_isoc_ep_comp_descriptor {
> +	__u8	bLength;
> +	__u8	bDescriptorType;
> +	__le16	wMaxPacketSize;
> +	__le32	dwBytesPerInterval;
> +} __attribute__ ((packed));
> +
> +#define USB_DT_EUSB2_ISOC_EP_COMP_SIZE	8
> +
> +/*-------------------------------------------------------------------------*/
> +
>  /* USB_DT_SSP_ISOC_ENDPOINT_COMP: SuperSpeedPlus Isochronous Endpoint Companion
>   * descriptor
>   */
> -- 
> 2.43.0
> 

BR,
Thinh

Re: [PATCH] USB: core: Add eUSB2 descriptor and parsing in USB core

[Mathias Nyman]

On 20.2.2025 18.35, Greg KH wrote:
> On Thu, Feb 20, 2025 at 04:13:39PM +0200, Mathias Nyman wrote:
>> From: Kannappan R <r.kannappan@intel.com>
>> --->> @@ -64,9 +65,10 @@ struct ep_device;
>>    * descriptor within an active interface in a given USB configuration.
>>    */
>>   struct usb_host_endpoint {
>> -	struct usb_endpoint_descriptor		desc;
>> -	struct usb_ss_ep_comp_descriptor	ss_ep_comp;
>> -	struct usb_ssp_isoc_ep_comp_descriptor	ssp_isoc_ep_comp;
>> +	struct usb_endpoint_descriptor			desc;
>> +	struct usb_ss_ep_comp_descriptor		ss_ep_comp;
>> +	struct usb_ssp_isoc_ep_comp_descriptor		ssp_isoc_ep_comp;
>> +	struct usb_eusb2_isoc_ep_comp_descriptor	eusb2_isoc_ep_comp;
> 
> No real need to indent any of these, but oh well :)

It looked odd when adding one new variable off by a space compared to all the
other neatly tab-aligned variables. So I shifted them all right.

>> +/* USB_DT_EUSB2_ISOC_ENDPOINT_COMP: eUSB2 Isoch Endpoint Companion descriptor */
>> +struct usb_eusb2_isoc_ep_comp_descriptor {
>> +	__u8	bLength;
>> +	__u8	bDescriptorType;
>> +	__le16	wMaxPacketSize;
>> +	__le32	dwBytesPerInterval;
>> +} __attribute__ ((packed));
>> +
>> +#define USB_DT_EUSB2_ISOC_EP_COMP_SIZE	8
> 
> Can't we use a sizeof() for this as well?  I guess we don't do it for
> other structures, so maybe not.
> 
> Anyway, this looks fine, if you want to just send an update for the
> 0x0220 later on if you think it's needed, please do.

Thanks for looking at this.

We did consider defining 0x0220, but checked that usb core uses magic numbers
for bcdUSB in other places:

hcd.c:  if (le16_to_cpu(usb_dev->descriptor.bcdUSB) >= 0x0201) {
hub.c:                  (le16_to_cpu(udev->descriptor.bcdUSB) < 0x0300)) {
hub.c:  if (le16_to_cpu(udev->descriptor.bcdUSB) >= 0x0201) {
hub.c:          if (le16_to_cpu(udev->descriptor.bcdUSB) >= 0x0200
hub.h:          le16_to_cpu(hdev->descriptor.bcdUSB) >= 0x0310 &&

Makes sense to add a separate patch later on that define all these.

Thanks
Mathias

Re: [PATCH] USB: core: Add eUSB2 descriptor and parsing in USB core

[Mathias Nyman]

On 20.2.2025 23.56, Thinh Nguyen wrote:
> Hi,
> 
> On Thu, Feb 20, 2025, Mathias Nyman wrote:
>> From: Kannappan R <r.kannappan@intel.com>
>>
>> Add support for the 'eUSB2 Isochronous Endpoint Companion Descriptor'
>> introduced in the recent USB 2.0 specification 'USB 2.0 Double Isochronous
>> IN Bandwidth' ECN.
>>
>> It allows embedded USB2 (eUSB2) devices to report and use higher bandwidths
>> for isochronous IN transfers in order to support higher camera resolutions
>> on the lid of laptops and tablets with minimal change to the USB2 protocol.
>>
>> The motivation for expanding USB 2.0 is further clarified in an additional
>> Embedded USB2 version 2.0 (eUSB2v2) supplement to the USB 2.0
>> specification. It points out this is optimized for performance, power and
>> cost by using the USB 2.0 low-voltage, power efficient PHY and half-duplex
>> link for the asymmetric camera bandwidth needs, avoiding the costly and
>> complex full-duplex USB 3.x symmetric link and gigabit receivers.
>>
>> eUSB2 devices that support the higher isochronous IN bandwidth and the new
>> descriptor can be identified by their device descriptor bcdUSB value of
>> 0x0220
> 
> Isn't eUSB2v2 has bcdUSB value of 0x0230?

My understanding is that this descriptor is introduced for bcdUSB 0x0220
eUSB2 devices that support Double Isochronous IN Bandwidth, 3072 to
6144 bytes per microframe. See "USB 2.0 Double Isochronous IN Bandwidth"
engineering change notice (USB 2.0 Double Isochronous IN ECN.pdf)

eUSB2v2 devices with bcdUSB 0x0230 use the same descriptor, but have additional
features such as assymmectric speed and bitrate up to 4.8Gbps.
These types of devices are defined in the
"Embedded USB2 Version 2.0 Supplement to the USB 2.0 Specification" document

This patch focuses on initial support for 0x0220 devices

> 
>>
>> Co-developed-by: Amardeep Rai <amardeep.rai@intel.com>
>> Signed-off-by: Amardeep Rai <amardeep.rai@intel.com>
>> Signed-off-by: Kannappan R <r.kannappan@intel.com>
>> Co-developed-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>> ---
>>   drivers/usb/core/config.c    | 51 ++++++++++++++++++++++++++++++++----
>>   include/linux/usb.h          |  8 +++---
>>   include/uapi/linux/usb/ch9.h | 15 +++++++++++
>>   3 files changed, 66 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
>> index f7bf8d1de3ad..13bd4ec4ea5f 100644
>> --- a/drivers/usb/core/config.c
>> +++ b/drivers/usb/core/config.c
>> @@ -64,6 +64,37 @@ static void usb_parse_ssp_isoc_endpoint_companion(struct device *ddev,
>>   	memcpy(&ep->ssp_isoc_ep_comp, desc, USB_DT_SSP_ISOC_EP_COMP_SIZE);
>>   }
>>   
>> +static void usb_parse_eusb2_isoc_endpoint_companion(struct device *ddev,
>> +		int cfgno, int inum, int asnum, struct usb_host_endpoint *ep,
>> +		unsigned char *buffer, int size)
>> +{
>> +	struct usb_eusb2_isoc_ep_comp_descriptor *desc;
>> +	struct usb_descriptor_header *h;
>> +
>> +	/*
>> +	 * eUSB2 isochronous endpoint companion descriptor for this endpoint
>> +	 * shall be declared before the next endpoint or interface descriptor
>> +	 */
>> +	while (size >= USB_DT_EUSB2_ISOC_EP_COMP_SIZE) {
>> +		h = (struct usb_descriptor_header *)buffer;
>> +
>> +		if (h->bDescriptorType == USB_DT_EUSB2_ISOC_ENDPOINT_COMP) {
>> +			desc = (struct usb_eusb2_isoc_ep_comp_descriptor *)buffer;
>> +			ep->eusb2_isoc_ep_comp = *desc;
>> +			return;
>> +		}
>> +		if (h->bDescriptorType == USB_DT_ENDPOINT ||
>> +		    h->bDescriptorType == USB_DT_INTERFACE)
>> +			break;
>> +
>> +		buffer += h->bLength;
>> +		size -= h->bLength;
>> +	}
>> +
>> +	dev_notice(ddev, "No eUSB2 isoc ep %d companion for config %d interface %d altsetting %d\n",
>> +		   ep->desc.bEndpointAddress, cfgno, inum, asnum);
> 
> Since eUSB2v2 devices should also include at least an alternate
> interface with isoc endpoint descriptors using legacy settings, does the
> spec require those legacy alternate interfaces to also have this isoc
> companion descriptor?

I think those alternate interfaces with 'legacy' settings will have normal nonzero
wMaxPacketSize, and no eusb2 isoc companion descriptor.

we only look for this descriptor if wMaxpacketSize == 0

This is based on the text the ECN adds to USB2 section 9.6.6 "Endpoint"

"For high bandwidth eUSB2 Isochronous IN Endpoint that require bandwidths above
3KB/microframe, the standard Endpoint descriptor shall declare a zero bandwidth setting,
i.e., the wMaxPacketSize field shall be set to 0, and the actual maximum packet size and
bandwidth requirements shall be defined in the eUSB2 Iso Endpoint Companion descriptor
wMaxPacketSize and dwBytesPerInterval fields, respectively. Note that Devices shall also
implement one or more non-zero bandwidth alternate settings with a non-zero wMaxPacketSize
(up to 3KB/microframe) in the standard Endpoint descriptor."

> 
>> +}>> +
>>   static void usb_parse_ss_endpoint_companion(struct device *ddev, int cfgno,
>>   		int inum, int asnum, struct usb_host_endpoint *ep,
>>   		unsigned char *buffer, int size)
>> @@ -258,8 +289,10 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
>>   	int n, i, j, retval;
>>   	unsigned int maxp;
>>   	const unsigned short *maxpacket_maxes;
>> +	u16 bcdUSB;
>>   
>>   	d = (struct usb_endpoint_descriptor *) buffer;
>> +	bcdUSB = le16_to_cpu(udev->descriptor.bcdUSB);
>>   	buffer += d->bLength;
>>   	size -= d->bLength;
>>   
>> @@ -409,15 +442,17 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
>>   
>>   	/*
>>   	 * Validate the wMaxPacketSize field.
>> -	 * Some devices have isochronous endpoints in altsetting 0;
>> -	 * the USB-2 spec requires such endpoints to have wMaxPacketSize = 0
>> -	 * (see the end of section 5.6.3), so don't warn about them.
>> +	 * eUSB2 devices (see USB 2.0 Double Isochronous IN ECN 9.6.6 Endpoint)
>> +	 * and devices with isochronous endpoints in altsetting 0 (see USB 2.0
>> +	 * end of section 5.6.3) have wMaxPacketSize = 0.
>> +	 * So don't warn about those.
>>   	 */
>>   	maxp = le16_to_cpu(endpoint->desc.wMaxPacketSize);
>> -	if (maxp == 0 && !(usb_endpoint_xfer_isoc(d) && asnum == 0)) {
>> +
>> +	if (maxp == 0 && bcdUSB != 0x0220 &&
>> +	    !(usb_endpoint_xfer_isoc(d) && asnum == 0))
>>   		dev_notice(ddev, "config %d interface %d altsetting %d endpoint 0x%X has invalid wMaxPacketSize 0\n",
>>   		    cfgno, inum, asnum, d->bEndpointAddress);
>> -	}
>>   
>>   	/* Find the highest legal maxpacket size for this endpoint */
>>   	i = 0;		/* additional transactions per microframe */
>> @@ -465,6 +500,12 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
>>   				maxp);
>>   	}
>>   
>> +	/* Parse a possible eUSB2 periodic endpoint companion descriptor */
>> +	if (bcdUSB == 0x0220 && d->wMaxPacketSize == 0 &&
>> +	    (usb_endpoint_xfer_isoc(d) || usb_endpoint_xfer_int(d)))
> 
> Why are we checking for interrupt endpoint to parse for isoc?
> 

Good point, can't recall why it ended up here. I'll fix that

>> +		usb_parse_eusb2_isoc_endpoint_companion(ddev, cfgno, inum, asnum,
>> +							endpoint, buffer, size);
>> +
>>   	/* Parse a possible SuperSpeed endpoint companion descriptor */
>>   	if (udev->speed >= USB_SPEED_SUPER)
>>   		usb_parse_ss_endpoint_companion(ddev, cfgno,
>> diff --git a/include/linux/usb.h b/include/linux/usb.h
>> index cfa8005e24f9..b46738701f8d 100644
>> --- a/include/linux/usb.h
>> +++ b/include/linux/usb.h
>> @@ -51,6 +51,7 @@ struct ep_device;
>>    * @desc: descriptor for this endpoint, wMaxPacketSize in native byteorder
>>    * @ss_ep_comp: SuperSpeed companion descriptor for this endpoint
>>    * @ssp_isoc_ep_comp: SuperSpeedPlus isoc companion descriptor for this endpoint
>> + * @eusb2_isoc_ep_comp: eUSB2 isoc companion descriptor for this endpoint
>>    * @urb_list: urbs queued to this endpoint; maintained by usbcore
>>    * @hcpriv: for use by HCD; typically holds hardware dma queue head (QH)
>>    *	with one or more transfer descriptors (TDs) per urb
>> @@ -64,9 +65,10 @@ struct ep_device;
>>    * descriptor within an active interface in a given USB configuration.
>>    */
>>   struct usb_host_endpoint {
>> -	struct usb_endpoint_descriptor		desc;
>> -	struct usb_ss_ep_comp_descriptor	ss_ep_comp;
>> -	struct usb_ssp_isoc_ep_comp_descriptor	ssp_isoc_ep_comp;
>> +	struct usb_endpoint_descriptor			desc;
>> +	struct usb_ss_ep_comp_descriptor		ss_ep_comp;
>> +	struct usb_ssp_isoc_ep_comp_descriptor		ssp_isoc_ep_comp;
>> +	struct usb_eusb2_isoc_ep_comp_descriptor	eusb2_isoc_ep_comp;
>>   	struct list_head		urb_list;
>>   	void				*hcpriv;
>>   	struct ep_device		*ep_dev;	/* For sysfs info */
>> diff --git a/include/uapi/linux/usb/ch9.h b/include/uapi/linux/usb/ch9.h
>> index 91f0f7e214a5..475af9358173 100644
>> --- a/include/uapi/linux/usb/ch9.h
>> +++ b/include/uapi/linux/usb/ch9.h
>> @@ -253,6 +253,9 @@ struct usb_ctrlrequest {
>>   #define USB_DT_BOS			0x0f
>>   #define USB_DT_DEVICE_CAPABILITY	0x10
>>   #define USB_DT_WIRELESS_ENDPOINT_COMP	0x11
>> +/* From the eUSB2 spec */
>> +#define USB_DT_EUSB2_ISOC_ENDPOINT_COMP	0x12
>> +/* From Wireless USB spec */
>>   #define USB_DT_WIRE_ADAPTER		0x21
>>   /* From USB Device Firmware Upgrade Specification, Revision 1.1 */
>>   #define USB_DT_DFU_FUNCTIONAL		0x21
>> @@ -675,6 +678,18 @@ static inline int usb_endpoint_interrupt_type(
>>   
>>   /*-------------------------------------------------------------------------*/
>>   
>> +/* USB_DT_EUSB2_ISOC_ENDPOINT_COMP: eUSB2 Isoch Endpoint Companion descriptor */
> 
> Should we name this usb_hsx_isoc_ep_comp_descriptor for consistency?
> 
> Just bringing up the discussion here as I don't have a hard stance on
> this. The naming of speeds and usb versions are getting more
> inconsistent, and naming of these are getting more challenging. Not sure
> if there will be something new in the future.

USB 2.0 Double Isochronous IN Bandwidth ECN documentation uses the names:
"eUSB2 Isochronous Endpoint Companion Descriptor"
"eUSB2 Iso Endpoint Companion Descriptor" and
EUSB2_ISO_ENDPOINT_COMPANION as "desriptor type"

hsx is not mentioned in this ECN, it seems to be introduced in the
eUSB2v2 Supplement.

But I don't have a strong opinion regarding this, but have grown used
to eusb2

Thanks for looking at this

-Mathias

Re: [PATCH] USB: core: Add eUSB2 descriptor and parsing in USB core

[Alan Stern]

On Fri, Feb 21, 2025 at 10:53:08AM +0200, Mathias Nyman wrote:
> We did consider defining 0x0220, but checked that usb core uses magic numbers
> for bcdUSB in other places:
> 
> hcd.c:  if (le16_to_cpu(usb_dev->descriptor.bcdUSB) >= 0x0201) {
> hub.c:                  (le16_to_cpu(udev->descriptor.bcdUSB) < 0x0300)) {
> hub.c:  if (le16_to_cpu(udev->descriptor.bcdUSB) >= 0x0201) {
> hub.c:          if (le16_to_cpu(udev->descriptor.bcdUSB) >= 0x0200
> hub.h:          le16_to_cpu(hdev->descriptor.bcdUSB) >= 0x0310 &&
> 
> Makes sense to add a separate patch later on that define all these.

It's hard to imagine how introducing #define's for these numbers could 
improve the readability of the code.  I mean, is it really better to 
say

	if (le16_to_cpu(udev->descriptor.bcdUSB) >= USB_v3_10

than

	if (le16_to_cpu(udev->descriptor.bcdUSB) >= 0x0310

?

Alan Stern

Re: [PATCH] USB: core: Add eUSB2 descriptor and parsing in USB core

[Thinh Nguyen]

On Fri, Feb 21, 2025, Mathias Nyman wrote:
> On 20.2.2025 23.56, Thinh Nguyen wrote:
> > Hi,
> > 
> > On Thu, Feb 20, 2025, Mathias Nyman wrote:
> > > From: Kannappan R <r.kannappan@intel.com>
> > > 
> > > Add support for the 'eUSB2 Isochronous Endpoint Companion Descriptor'
> > > introduced in the recent USB 2.0 specification 'USB 2.0 Double Isochronous
> > > IN Bandwidth' ECN.
> > > 
> > > It allows embedded USB2 (eUSB2) devices to report and use higher bandwidths
> > > for isochronous IN transfers in order to support higher camera resolutions
> > > on the lid of laptops and tablets with minimal change to the USB2 protocol.
> > > 
> > > The motivation for expanding USB 2.0 is further clarified in an additional
> > > Embedded USB2 version 2.0 (eUSB2v2) supplement to the USB 2.0
> > > specification. It points out this is optimized for performance, power and
> > > cost by using the USB 2.0 low-voltage, power efficient PHY and half-duplex
> > > link for the asymmetric camera bandwidth needs, avoiding the costly and
> > > complex full-duplex USB 3.x symmetric link and gigabit receivers.
> > > 
> > > eUSB2 devices that support the higher isochronous IN bandwidth and the new
> > > descriptor can be identified by their device descriptor bcdUSB value of
> > > 0x0220
> > 
> > Isn't eUSB2v2 has bcdUSB value of 0x0230?
> 
> My understanding is that this descriptor is introduced for bcdUSB 0x0220
> eUSB2 devices that support Double Isochronous IN Bandwidth, 3072 to
> 6144 bytes per microframe. See "USB 2.0 Double Isochronous IN Bandwidth"
> engineering change notice (USB 2.0 Double Isochronous IN ECN.pdf)
> 
> eUSB2v2 devices with bcdUSB 0x0230 use the same descriptor, but have additional
> features such as assymmectric speed and bitrate up to 4.8Gbps.
> These types of devices are defined in the
> "Embedded USB2 Version 2.0 Supplement to the USB 2.0 Specification" document

Ok. The change log also mentioned eUSB2v2. That confused me.

> 
> This patch focuses on initial support for 0x0220 devices

Regarding the new isoc companion descriptor for both eUSB2v1 and
eUSB2v2, if you don't see any additional change needed and if it's not
too troublesome, can you also add the additional support for 0x0230
check?

> 
> > 
> > > 
> > > Co-developed-by: Amardeep Rai <amardeep.rai@intel.com>
> > > Signed-off-by: Amardeep Rai <amardeep.rai@intel.com>
> > > Signed-off-by: Kannappan R <r.kannappan@intel.com>
> > > Co-developed-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> > > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> > > ---
> > >   drivers/usb/core/config.c    | 51 ++++++++++++++++++++++++++++++++----
> > >   include/linux/usb.h          |  8 +++---
> > >   include/uapi/linux/usb/ch9.h | 15 +++++++++++
> > >   3 files changed, 66 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> > > index f7bf8d1de3ad..13bd4ec4ea5f 100644
> > > --- a/drivers/usb/core/config.c
> > > +++ b/drivers/usb/core/config.c
> > > @@ -64,6 +64,37 @@ static void usb_parse_ssp_isoc_endpoint_companion(struct device *ddev,
> > >   	memcpy(&ep->ssp_isoc_ep_comp, desc, USB_DT_SSP_ISOC_EP_COMP_SIZE);
> > >   }
> > > +static void usb_parse_eusb2_isoc_endpoint_companion(struct device *ddev,
> > > +		int cfgno, int inum, int asnum, struct usb_host_endpoint *ep,
> > > +		unsigned char *buffer, int size)
> > > +{
> > > +	struct usb_eusb2_isoc_ep_comp_descriptor *desc;
> > > +	struct usb_descriptor_header *h;
> > > +
> > > +	/*
> > > +	 * eUSB2 isochronous endpoint companion descriptor for this endpoint
> > > +	 * shall be declared before the next endpoint or interface descriptor
> > > +	 */
> > > +	while (size >= USB_DT_EUSB2_ISOC_EP_COMP_SIZE) {
> > > +		h = (struct usb_descriptor_header *)buffer;
> > > +
> > > +		if (h->bDescriptorType == USB_DT_EUSB2_ISOC_ENDPOINT_COMP) {
> > > +			desc = (struct usb_eusb2_isoc_ep_comp_descriptor *)buffer;
> > > +			ep->eusb2_isoc_ep_comp = *desc;
> > > +			return;
> > > +		}
> > > +		if (h->bDescriptorType == USB_DT_ENDPOINT ||
> > > +		    h->bDescriptorType == USB_DT_INTERFACE)
> > > +			break;
> > > +
> > > +		buffer += h->bLength;
> > > +		size -= h->bLength;
> > > +	}
> > > +
> > > +	dev_notice(ddev, "No eUSB2 isoc ep %d companion for config %d interface %d altsetting %d\n",
> > > +		   ep->desc.bEndpointAddress, cfgno, inum, asnum);
> > 
> > Since eUSB2v2 devices should also include at least an alternate
> > interface with isoc endpoint descriptors using legacy settings, does the
> > spec require those legacy alternate interfaces to also have this isoc
> > companion descriptor?
> 
> I think those alternate interfaces with 'legacy' settings will have normal nonzero
> wMaxPacketSize, and no eusb2 isoc companion descriptor.
> 
> we only look for this descriptor if wMaxpacketSize == 0

Thanks for clarifications.

> 
> This is based on the text the ECN adds to USB2 section 9.6.6 "Endpoint"
> 
> "For high bandwidth eUSB2 Isochronous IN Endpoint that require bandwidths above
> 3KB/microframe, the standard Endpoint descriptor shall declare a zero bandwidth setting,
> i.e., the wMaxPacketSize field shall be set to 0, and the actual maximum packet size and
> bandwidth requirements shall be defined in the eUSB2 Iso Endpoint Companion descriptor
> wMaxPacketSize and dwBytesPerInterval fields, respectively. Note that Devices shall also
> implement one or more non-zero bandwidth alternate settings with a non-zero wMaxPacketSize
> (up to 3KB/microframe) in the standard Endpoint descriptor."
> 
> > 
> > > +}>> +
> > >   static void usb_parse_ss_endpoint_companion(struct device *ddev, int cfgno,
> > >   		int inum, int asnum, struct usb_host_endpoint *ep,
> > >   		unsigned char *buffer, int size)
> > > @@ -258,8 +289,10 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
> > >   	int n, i, j, retval;
> > >   	unsigned int maxp;
> > >   	const unsigned short *maxpacket_maxes;
> > > +	u16 bcdUSB;
> > >   	d = (struct usb_endpoint_descriptor *) buffer;
> > > +	bcdUSB = le16_to_cpu(udev->descriptor.bcdUSB);
> > >   	buffer += d->bLength;
> > >   	size -= d->bLength;
> > > @@ -409,15 +442,17 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
> > >   	/*
> > >   	 * Validate the wMaxPacketSize field.
> > > -	 * Some devices have isochronous endpoints in altsetting 0;
> > > -	 * the USB-2 spec requires such endpoints to have wMaxPacketSize = 0
> > > -	 * (see the end of section 5.6.3), so don't warn about them.
> > > +	 * eUSB2 devices (see USB 2.0 Double Isochronous IN ECN 9.6.6 Endpoint)
> > > +	 * and devices with isochronous endpoints in altsetting 0 (see USB 2.0
> > > +	 * end of section 5.6.3) have wMaxPacketSize = 0.
> > > +	 * So don't warn about those.
> > >   	 */
> > >   	maxp = le16_to_cpu(endpoint->desc.wMaxPacketSize);
> > > -	if (maxp == 0 && !(usb_endpoint_xfer_isoc(d) && asnum == 0)) {
> > > +
> > > +	if (maxp == 0 && bcdUSB != 0x0220 &&
> > > +	    !(usb_endpoint_xfer_isoc(d) && asnum == 0))
> > >   		dev_notice(ddev, "config %d interface %d altsetting %d endpoint 0x%X has invalid wMaxPacketSize 0\n",
> > >   		    cfgno, inum, asnum, d->bEndpointAddress);
> > > -	}
> > >   	/* Find the highest legal maxpacket size for this endpoint */
> > >   	i = 0;		/* additional transactions per microframe */
> > > @@ -465,6 +500,12 @@ static int usb_parse_endpoint(struct device *ddev, int cfgno,
> > >   				maxp);
> > >   	}
> > > +	/* Parse a possible eUSB2 periodic endpoint companion descriptor */
> > > +	if (bcdUSB == 0x0220 && d->wMaxPacketSize == 0 &&
> > > +	    (usb_endpoint_xfer_isoc(d) || usb_endpoint_xfer_int(d)))
> > 
> > Why are we checking for interrupt endpoint to parse for isoc?
> > 
> 
> Good point, can't recall why it ended up here. I'll fix that
> 
> > > +		usb_parse_eusb2_isoc_endpoint_companion(ddev, cfgno, inum, asnum,
> > > +							endpoint, buffer, size);
> > > +
> > >   	/* Parse a possible SuperSpeed endpoint companion descriptor */
> > >   	if (udev->speed >= USB_SPEED_SUPER)
> > >   		usb_parse_ss_endpoint_companion(ddev, cfgno,
> > > diff --git a/include/linux/usb.h b/include/linux/usb.h
> > > index cfa8005e24f9..b46738701f8d 100644
> > > --- a/include/linux/usb.h
> > > +++ b/include/linux/usb.h
> > > @@ -51,6 +51,7 @@ struct ep_device;
> > >    * @desc: descriptor for this endpoint, wMaxPacketSize in native byteorder
> > >    * @ss_ep_comp: SuperSpeed companion descriptor for this endpoint
> > >    * @ssp_isoc_ep_comp: SuperSpeedPlus isoc companion descriptor for this endpoint
> > > + * @eusb2_isoc_ep_comp: eUSB2 isoc companion descriptor for this endpoint
> > >    * @urb_list: urbs queued to this endpoint; maintained by usbcore
> > >    * @hcpriv: for use by HCD; typically holds hardware dma queue head (QH)
> > >    *	with one or more transfer descriptors (TDs) per urb
> > > @@ -64,9 +65,10 @@ struct ep_device;
> > >    * descriptor within an active interface in a given USB configuration.
> > >    */
> > >   struct usb_host_endpoint {
> > > -	struct usb_endpoint_descriptor		desc;
> > > -	struct usb_ss_ep_comp_descriptor	ss_ep_comp;
> > > -	struct usb_ssp_isoc_ep_comp_descriptor	ssp_isoc_ep_comp;
> > > +	struct usb_endpoint_descriptor			desc;
> > > +	struct usb_ss_ep_comp_descriptor		ss_ep_comp;
> > > +	struct usb_ssp_isoc_ep_comp_descriptor		ssp_isoc_ep_comp;
> > > +	struct usb_eusb2_isoc_ep_comp_descriptor	eusb2_isoc_ep_comp;
> > >   	struct list_head		urb_list;
> > >   	void				*hcpriv;
> > >   	struct ep_device		*ep_dev;	/* For sysfs info */
> > > diff --git a/include/uapi/linux/usb/ch9.h b/include/uapi/linux/usb/ch9.h
> > > index 91f0f7e214a5..475af9358173 100644
> > > --- a/include/uapi/linux/usb/ch9.h
> > > +++ b/include/uapi/linux/usb/ch9.h
> > > @@ -253,6 +253,9 @@ struct usb_ctrlrequest {
> > >   #define USB_DT_BOS			0x0f
> > >   #define USB_DT_DEVICE_CAPABILITY	0x10
> > >   #define USB_DT_WIRELESS_ENDPOINT_COMP	0x11
> > > +/* From the eUSB2 spec */
> > > +#define USB_DT_EUSB2_ISOC_ENDPOINT_COMP	0x12
> > > +/* From Wireless USB spec */
> > >   #define USB_DT_WIRE_ADAPTER		0x21
> > >   /* From USB Device Firmware Upgrade Specification, Revision 1.1 */
> > >   #define USB_DT_DFU_FUNCTIONAL		0x21
> > > @@ -675,6 +678,18 @@ static inline int usb_endpoint_interrupt_type(
> > >   /*-------------------------------------------------------------------------*/
> > > +/* USB_DT_EUSB2_ISOC_ENDPOINT_COMP: eUSB2 Isoch Endpoint Companion descriptor */
> > 
> > Should we name this usb_hsx_isoc_ep_comp_descriptor for consistency?
> > 
> > Just bringing up the discussion here as I don't have a hard stance on
> > this. The naming of speeds and usb versions are getting more
> > inconsistent, and naming of these are getting more challenging. Not sure
> > if there will be something new in the future.
> 
> USB 2.0 Double Isochronous IN Bandwidth ECN documentation uses the names:
> "eUSB2 Isochronous Endpoint Companion Descriptor"
> "eUSB2 Iso Endpoint Companion Descriptor" and
> EUSB2_ISO_ENDPOINT_COMPANION as "desriptor type"
> 

Good point.

> hsx is not mentioned in this ECN, it seems to be introduced in the
> eUSB2v2 Supplement.
> 
> But I don't have a strong opinion regarding this, but have grown used
> to eusb2
> 
> Thanks for looking at this
> 

Thanks for the patch!

BR,
Thinh