Received-SPF: pass (google.com: domain of linux-kernel+bounces-201779-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <20240601120640.73556-1-joswang1221@gmail.com> <1662c9c9-7330-4794-843a-940f05802021@linux.intel.com>
In-Reply-To: <1662c9c9-7330-4794-843a-940f05802021@linux.intel.com>
From: joswang <joswang1221@gmail.com>
Date: Wed, 5 Jun 2024 13:37:39 +0800
Message-ID: <CAMtoTm1u+8ynBRaWgCMVgaR+dBoZfNGhzGqmvooBSDxZm5Qx+g@mail.gmail.com>
Subject: Re: [RFC 1/1] usb: host: xhci-plat: add enable XHCI-AVOID-BEI quirk
 by dts
To: Mathias Nyman <mathias.nyman@linux.intel.com>
Cc: mathias.nyman@intel.com, gregkh@linuxfoundation.org, 
	linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, 
	joswang <joswang@lenovo.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Jun 3, 2024 at 8:21=E2=80=AFPM Mathias Nyman
<mathias.nyman@linux.intel.com> wrote:
>
> On 1.6.2024 15.06, joswang wrote:
> > From: joswang <joswang@lenovo.com>
> >
> > For Synopsys DWC31 2.00a and earlier versions, every isochronous
> > interval the BEI(Block Event Interrupt) flag is set for all except
> > the last Isoch TRB in a URB, host controller consumes the event
> > TRBs in the event ring, once the event ring is full, it will not
> > generate an interrupt and will stop all data transmission and command
> > execution.
> >
> > To avoid the problem of event ring full, the XHCI-AVOID-BEI quirk is
> > introduced. Currently, the XHCI-AVOID-BEI quirk has been applied to all
> > Intel xHCI controllers, see commit '227a4fd801c8 ("USB: xhci: apply
> > XHCI-AVOID-BEI quirk to all Intel xHCI controllers")'.
> >
> > For Linux system, each event ring consists of one or more event ring
> > segments and each segment is 4 KB that contains 256 TRBs. It seems that
> > the TRBs on the event ring are sufficient and the event ring will not b=
e
> > full. In real application, if it does happen, event ring is full, host
> > controller no interrupt causes the driver to timeout.
> >
> > However, applying XHCI-AVOID-BEI quirk will also bring power consumptio=
n
> > issues. We can consider the application scenarios of the product to dec=
ide
> > whether to enable it. Therefore, we add the enable XHCI-AVOID-BEI quirk
> > through dts configuration to make it more flexible.
>
> Took a look at XHCI_AVOID_BEI quirk and it seems that it evolved from
> solving a hardware issue into a interrupt trigger optimization.
Thanks for reviewing the code.
Yes, you optimized the interrupt triggering frequency.
>
> How about making current XHCI_AVOID_BEI the default behavior, i.e. force
> an interrupt every 32nd isoc trb, and reduce it in case event ring
> has more than half a segments of events per interrupt.
Yes=EF=BC=8Cenabling XHCI_AVOID_BEI quirk is to solve the problem of event =
ring fullness
>
> The actual XHCI_AVOID_BEI quirk would only be used for hardware that that
> can't handle BEI flag properly
>
> something like:
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 266fcbc4bb93..dd161ebf15a3 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -3991,16 +3991,17 @@ static int xhci_get_isoc_frame_id(struct xhci_hcd=
 *xhci,
>   static bool trb_block_event_intr(struct xhci_hcd *xhci, int num_tds, in=
t i,
>                                   struct xhci_interrupter *ir)
>   {
> -       if (xhci->hci_version < 0x100)
> +       if (xhci->hci_version < 0x100 || xhci->quirks & XHCI_AVOID_BEI)
>                  return false;
> +
>          /* always generate an event interrupt for the last TD */
>          if (i =3D=3D num_tds - 1)
>                  return false;
>          /*
> -        * If AVOID_BEI is set the host handles full event rings poorly,
> -        * generate an event at least every 8th TD to clear the event rin=
g
> +        * host handles full event rings poorly, force an interrupt at le=
ast
> +        * every 32 isoc TRB to clear the event ring.
>           */
> -       if (i && ir->isoc_bei_interval && xhci->quirks & XHCI_AVOID_BEI)
> +       if (i && ir->isoc_bei_interval)
>
For Synopsys DWC31 2.00a IP and earlier versions, the corresponding
driver is in drivers/usb/dwc3/core.c.
If XHCI_AVOID_BEI quirk is not enabled, in other words, an interrupt
is triggered every 32nd isoc trb, then
the event ring may be full. After the event ring is full, the
controller cannot trigger an interrupt, causing the driver
to timeout.
My initial solution:
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index a171b27a7845..1e33e58c7281 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -126,7 +126,7 @@ static int dwc3_host_get_irq(struct dwc3 *dwc)

 int dwc3_host_init(struct dwc3 *dwc)
 {
-       struct property_entry   props[5];
+       struct property_entry   props[6];
        struct platform_device  *xhci;
        int                     ret, irq;
        int                     prop_idx =3D 0;
@@ -180,6 +180,9 @@ int dwc3_host_init(struct dwc3 *dwc)
        if (DWC3_VER_IS_WITHIN(DWC3, ANY, 300A))
                props[prop_idx++] =3D
PROPERTY_ENTRY_BOOL("quirk-broken-port-ped");

+       if (DWC3_VER_IS_WITHIN(DWC31, ANY, 200A))
+               props[prop_idx++] =3D PROPERTY_ENTRY_BOOL("quirk-avoid-bei"=
);
+
        if (prop_idx) {
                ret =3D device_create_managed_software_node(&xhci->dev,
props, NULL);
                if (ret) {
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 3d071b875308..e1071827d4b3 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -253,6 +253,9 @@ int xhci_plat_probe(struct platform_device *pdev,
struct device *sysdev, const s
                if (device_property_read_bool(tmpdev, "quirk-broken-port-pe=
d"))
                        xhci->quirks |=3D XHCI_BROKEN_PORT_PED;

+               if (device_property_read_bool(tmpdev, "quirk-avoid-bei"))
+                       xhci->quirks |=3D XHCI_AVOID_BEI;
+
                if (device_property_read_bool(tmpdev,
"xhci-sg-trb-cache-size-quirk"))
                        xhci->quirks |=3D XHCI_SG_TRB_CACHE_SIZE_QUIRK;

I consider that enabling XHCI_AVOID_BEI quirk will increase the number
of isoc transmission
interrupts, and some specific applications of products may not have
full event rings.
For Synopsys DWC31 2.00a IP and earlier versions, XHCI_AVOID_BEI quirk
is forced to be enabled,
which is not the best solution. Therefore, the second solution is
generated, which is to remove the
modification of drivers/usb/dwc3/host.c file, only keep
drivers/usb/host/xhci-plat.c, enable XHCI_AVOID_BEI
quirk by adding dts configuration. Let users decide whether to enable
XHCI_AVOID_BEI quirk based on
the specific application of the product.
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 3d071b875308..e1071827d4b3 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -253,6 +253,9 @@ int xhci_plat_probe(struct platform_device *pdev,
struct device *sysdev, const s
                if (device_property_read_bool(tmpdev, "quirk-broken-port-pe=
d"))
                        xhci->quirks |=3D XHCI_BROKEN_PORT_PED;

+               if (device_property_read_bool(tmpdev, "quirk-avoid-bei"))
+                       xhci->quirks |=3D XHCI_AVOID_BEI;
+
                if (device_property_read_bool(tmpdev,
"xhci-sg-trb-cache-size-quirk"))
                        xhci->quirks |=3D XHCI_SG_TRB_CACHE_SIZE_QUIRK;
Please help evaluate, thank you.
>
> Thanks
> Mathias
>