Re: [PATCH v4 1/4] xhci: Add host support for eUSB2 double isochronous bandwidth devices

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Mathias, Michał,

On Mon, Aug 18, 2025 at 06:30:23PM +0300, Mathias Nyman wrote:
> On 18.8.2025 12.50, Michał Pecio wrote:
> > On Tue, 12 Aug 2025 16:24:42 +0300, Sakari Ailus wrote:
> > > From: "Rai, Amardeep" <amardeep.rai@intel.com>
> > > 
> > > Detect eUSB2 double isoc bw capable hosts and devices, and set the proper
> > > xhci endpoint context values such as 'Mult', 'Max Burst Size', and 'Max
> > > ESIT Payload' to enable the double isochronous bandwidth endpoints.
> > > 
> > > Intel xHC uses the endpoint context 'Mult' field for eUSB2 isoc
> > > endpoints even if hosts supporting Large ESIT Payload Capability should
> > > normally ignore the mult field.
> > > 
> > > Signed-off-by: Rai, Amardeep <amardeep.rai@intel.com>
> > > Co-developed-by: Kannappan R <r.kannappan@intel.com>
> > > Signed-off-by: Kannappan R <r.kannappan@intel.com>
> > > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Co-developed-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> > > Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
> > > Co-developed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >   drivers/usb/host/xhci-caps.h |  2 ++
> > >   drivers/usb/host/xhci-mem.c  | 60 ++++++++++++++++++++++++++++--------
> > >   drivers/usb/host/xhci-ring.c |  6 ++--
> > >   drivers/usb/host/xhci.c      | 16 +++++++++-
> > >   drivers/usb/host/xhci.h      | 19 ++++++++++++
> > >   5 files changed, 87 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/drivers/usb/host/xhci-caps.h b/drivers/usb/host/xhci-caps.h
> > > index 4b8ff4815644..89bc83e4f1eb 100644
> > > --- a/drivers/usb/host/xhci-caps.h
> > > +++ b/drivers/usb/host/xhci-caps.h
> > > @@ -89,3 +89,5 @@
> > >   #define HCC2_GSC(p)             ((p) & (1 << 8))
> > >   /* true: HC support Virtualization Based Trusted I/O Capability */
> > >   #define HCC2_VTC(p)             ((p) & (1 << 9))
> > > +/* true: HC support Double BW on a eUSB2 HS ISOC EP */
> > > +#define HCC2_EUSB2_DIC(p)       ((p) & (1 << 11))
> > 
> > I guess this bit is not defined in the original xHCI 1.2 spec which
> > predates BW doubling, any reference to where it is specified and what
> > it means exactly?
> 
> USB 2.0 Double Isochronous IN Bandwidth Capability (DIC) – RO.
> This bit when set to 1, indicates that the xHC supports the USB 2.0 Double Isochronous IN
> Bandwidth Capability on a eUSB2 HS Isochronous Endpoint.
> This feature is only applicable to a directly connected inbox native eUSB2 device.
> 
> The xHCI specification with this addition is on its way, we got permission from
> author(s) to start upstreaming the code already.
> 
> > 
> > > diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
> > > index 6680afa4f596..ea51434c80fa 100644
> > > --- a/drivers/usb/host/xhci-mem.c
> > > +++ b/drivers/usb/host/xhci-mem.c
> > > @@ -1328,18 +1328,36 @@ static unsigned int xhci_get_endpoint_interval(struct usb_device *udev,
> > >   	return interval;
> > >   }
> > > -/* The "Mult" field in the endpoint context is only set for SuperSpeed isoc eps.
> > > +/*
> > > + * xHCs without LEC use the "Mult" field in the endpoint context for SuperSpeed
> > > + * isoc eps, and High speed isoc eps that support bandwidth doubling. Standard
> > >    * High speed endpoint descriptors can define "the number of additional
> > >    * transaction opportunities per microframe", but that goes in the Max Burst
> > >    * endpoint context field.
> > >    */
> > > -static u32 xhci_get_endpoint_mult(struct usb_device *udev,
> > > -		struct usb_host_endpoint *ep)
> > > +static u32 xhci_get_endpoint_mult(struct xhci_hcd *xhci,
> > > +				  struct usb_device *udev,
> > > +				  struct usb_host_endpoint *ep)
> > >   {
> > > -	if (udev->speed < USB_SPEED_SUPER ||
> > > -			!usb_endpoint_xfer_isoc(&ep->desc))
> > > +	bool lec;
> > > +
> > > +	/* xHCI 1.1 with LEC set does not use mult field, except intel eUSB2 */
> > > +	lec = xhci->hci_version > 0x100 && HCC2_LEC(xhci->hcc_params2);
> > > +
> > > +	/* eUSB2 double isoc bw devices are the only USB2 devices using mult */
> > > +	if (xhci_eusb2_is_isoc_bw_double(udev, ep)) {
> > > +		if (!lec || xhci->quirks & XHCI_INTEL_HOST)
> > > +			return 1;
> > > +	}
> > > +
> > > +	/* Oherwise only isoc transfers on hosts without LEC uses mult field */
> > > +	if (!usb_endpoint_xfer_isoc(&ep->desc) || lec)
> > >   		return 0;
> > > -	return ep->ss_ep_comp.bmAttributes;
> > > +
> > > +	if (udev->speed >= USB_SPEED_SUPER)
> > > +		return ep->ss_ep_comp.bmAttributes;
> > > +
> > > +	return 0;
> > >   }
> > 
> > That's a complicated control flow. I think it could just be:
> > > +	/* SuperSpeed isoc transfers on hosts without LEC uses mult field */
> > > +	if (udev->speed >= USB_SPEED_SUPER && usb_endpoint_xfer_isoc(&ep->desc) && !lec)
> > > +		return ep->ss_ep_comp.bmAttributes;
> > > +	return 0;
> 
> Possibly, not sure there's much difference, especially if you break that line at
> 80 columns

I prefer Michał's suggestion, line breaks are fine, too...

> 
> > > +/*
> > > + * USB 2.0 specification, chapter 5.6.4 Isochronous Transfer Bus Access
> > > + * Constraint. A high speed endpoint can move up to 3072 bytes per microframe
> > > + * (or 192Mb/s).
> > > + */
> > > +#define MAX_ISOC_XFER_SIZE_HS  3072
> > > +
> > > +static inline bool xhci_eusb2_is_isoc_bw_double(struct usb_device *udev,
> > > +						struct usb_host_endpoint *ep)
> > > +{
> > > +	return udev->speed == USB_SPEED_HIGH &&
> > > +		usb_endpoint_is_isoc_in(&ep->desc) &&
> > > +		le16_to_cpu(ep->desc.wMaxPacketSize) == 0 &&
> > > +		le32_to_cpu(ep->eusb2_isoc_ep_comp.dwBytesPerInterval) >
> > > +		MAX_ISOC_XFER_SIZE_HS;
> > > +}
> > 
> > It looks like eUSB2v2 is another spec which uses this descriptor for
> > larger isoc endpoints and this code might trigger on such devices once
> > USB core learns to parse them.
> > 
> 
> Could make sense to check that bcdUSB is 0x220 here to avoid that.

I'll add the check for v5.

> 
> We could also check if ep->eusb2_isoc_ep_comp.bDescriptorType exists
> like in PATCH 2/4, and could then remove the wMaxPacketSize == 0 check as
> usb core descriptor parsing will already do that check for us, before copying
> the descriptor.

Ack.

> 
> The USB_SPEED_HIGH check could also be moved to descriptor parsing in usb core.
> This way we could remove the speed check hare and from PATCH 2/3

Ack.

> 
> 
> > Would there be no issues with that? Or conversely, any chance that your
> > code could be trivially modified to support full 2v2, and "bw doubling"
> > removed from names and comments?
> 
> Proably not, eUSB2v2 may have some odd burst and mult fields while double
> bandwidth has fixed values.

-- 
Regards,

Sakari Ailus