Re: [PATCH v30 02/30] xhci: sec-intr: add initial api to register a secondary interrupter entity

[Mathias Nyman]

On 21.11.2024 3.34, Wesley Cheng wrote:
> Hi Mathias,
>
> On 11/20/2024 6:36 AM, Mathias Nyman wrote:
> > On 6.11.2024 21.33, Wesley Cheng wrote:
> > > From: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
> > >
> > > Introduce XHCI sec intr, which manages the USB endpoints being requested by
> > > a client driver.  This is used for when client drivers are attempting to
> > > offload USB endpoints to another entity for handling USB transfers.  XHCI
> > > sec intr will allow for drivers to fetch the required information about the
> > > transfer ring, so the user can submit transfers independently.  Expose the
> > > required APIs for drivers to register and request for a USB endpoint and to
> > > manage XHCI secondary interrupters.
> > >
> > > Driver renaming, multiple ring segment page linking, proper endpoint clean
> > > up, and allowing module compilation added by Wesley Cheng to complete
> > > original concept code by Mathias Nyman.
> > >
> > > Signed-off-by: Mathias Nyman <mathias.nyman@xxxxxxxxxxxxxxx>
> > > Co-developed-by: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
> > > Signed-off-by: Wesley Cheng <quic_wcheng@xxxxxxxxxxx>
> > > ---
> > >    drivers/usb/host/Kconfig          |  11 +
> > >    drivers/usb/host/Makefile         |   2 +
> > >    drivers/usb/host/xhci-sec-intr.c  | 438 ++++++++++++++++++++++++++++++
> > >    drivers/usb/host/xhci.h           |   4 +
> > >    include/linux/usb/xhci-sec-intr.h |  70 +++++
> > >    5 files changed, 525 insertions(+)
> > >    create mode 100644 drivers/usb/host/xhci-sec-intr.c
> > >    create mode 100644 include/linux/usb/xhci-sec-intr.h
> > >
> > > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> > > index d011d6c753ed..a2d549e3e076 100644
> > > --- a/drivers/usb/host/Kconfig
> > > +++ b/drivers/usb/host/Kconfig
> > > @@ -104,6 +104,17 @@ config USB_XHCI_RZV2M
> > >          Say 'Y' to enable the support for the xHCI host controller
> > >          found in Renesas RZ/V2M SoC.
> > >    +config USB_XHCI_SEC_INTR
> > > +    tristate "xHCI support for secondary interrupter management"
> > > +    help
> > > +      Say 'Y' to enable the support for the xHCI secondary management.
> > > +      Provide a mechanism for a sideband datapath for payload associated
> > > +      with audio class endpoints. This allows for an audio DSP to use
> > > +      xHCI USB endpoints directly, allowing CPU to sleep while playing
> > > +      audio.  This is not the same feature as the audio sideband
> > > +      capability mentioned within the xHCI specification, and continues
> > > +      to utilize main system memory for data transfers.
> >
> > This same API should be used for the hardware xHCI sideband capability.
> > We should add a function that checks which types of xHC sideband capability xHC
> > hardware can support, and pick and pass a type to xhci xhci_sec_intr_register()
> > when registering a sideband/sec_intr
>
> Just to make sure we're on the same page, when you mention the term sideband capability, are you referring to section 7.9 xHCI Audio Sideband Capability in the xHCI spec?  If so, I'm not entirely sure if that capability relies much on secondary interrupters.  From reading the material, it just seems like its a way to map audio endpoints directly to another USB device connected to the controller? (I might be wrong, couldn't find much about potential use cases)

Yes, that is the one, 7.9 xHCI Audio Sideband Capability.

I had that in mind when I started writing the sideband API.
This is why registering a sideband and requesting a secondary interrupter
are done in separate functions.
The concept if still similar even if '7.9 Audio Sideband Capability' doesn't
need a secondary interrupter, we want to tell xhci driver/xHC hardware that
one connected usb device/endpoint handling is offloaded somewhere else.

I don't think we should write another API for that one just because more is
done by firmware than by xhci driver.

The only change for now would be to add some "sideband_type" parameter to
xhci_sec_intr_register(struct usb_device *udev, enum sideband_type), fail the
registration if isn't "software", and save the type in struct xhci_sec_intr

I'll add hardware sideband support (7.9 Audio Sideband) later, but it would be
nice to not change the API then.

The name change from sideband to sec-intr is a bit unfortunate with this in
mind. Was there some reason for it?

Thanks
Mathias