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@xxxxxxxxx>
> > >
> > > 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@xxxxxxxxx>
> > > Co-developed-by: Kannappan R <r.kannappan@xxxxxxxxx>
> > > Signed-off-by: Kannappan R <r.kannappan@xxxxxxxxx>
> > > Reviewed-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > Co-developed-by: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
> > > Co-developed-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> > > ---
> > > 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
