Message-ID: <c30b77dc-0a34-4ea7-a4c0-37a5d3065ee7@quicinc.com>
Date: Thu, 21 Nov 2024 12:24:54 -0800
From: Wesley Cheng <quic_wcheng@...cinc.com>
To: Mathias Nyman <mathias.nyman@...ux.intel.com>,
        <srinivas.kandagatla@...aro.org>, <mathias.nyman@...el.com>,
        <perex@...ex.cz>, <conor+dt@...nel.org>, <dmitry.torokhov@...il.com>,
        <corbet@....net>, <broonie@...nel.org>, <lgirdwood@...il.com>,
        <krzk+dt@...nel.org>, <pierre-louis.bossart@...ux.intel.com>,
        <Thinh.Nguyen@...opsys.com>, <tiwai@...e.com>, <robh@...nel.org>,
        <gregkh@...uxfoundation.org>
CC: <linux-kernel@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-sound@...r.kernel.org>, <linux-usb@...r.kernel.org>,
        <linux-input@...r.kernel.org>, <linux-arm-msm@...r.kernel.org>,
        <linux-doc@...r.kernel.org>
Subject: Re: [PATCH v30 02/30] xhci: sec-intr: add initial api to register a
 secondary interrupter entity

Hi Mathias,

On 11/21/2024 11:15 AM, Mathias Nyman wrote:
> 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@...ux.intel.com>
>>>>
>>>> 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@...ux.intel.com>
>>>> Co-developed-by: Wesley Cheng <quic_wcheng@...cinc.com>
>>>> Signed-off-by: Wesley Cheng <quic_wcheng@...cinc.com>
>>>> ---
>>>>    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.
>

Ah, ok...now I understand a bit more on what you were trying to do.  When you do eventually introduce the audio sideband capability, you'd need to modify the endpoint APIs to also issue the 'Set Resource Assignment' command with the proper device/endpoint being offloaded.  Initially, when Thinh brought up this section in the xHCI spec, I couldn't find any use cases that utilized that capability, so it was a bit unclear to me what it was meant for.  Now you've explained it a bit more, I think I can get the gist of it.


> 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?
>

This was because I wasn't too sure how the audio sideband and this driver was related, but I get what you are trying to do now.  I can change it back to the original xhci-sideband, as the name change was something that happened on this revision.  I will fix the corner case I mentioned WRT the xhci_discover_or_reset_device() scenario, add the basic sideband type handling and rename this back to xhci-sideband in the next rev.

Thanks

Wesley Cheng

Powered by blists - more mailing lists