[Cc += linux-usb, so I keep a bit more context than I'd normally do]
Hello Fabrice,
I stumbled about this old thread while cleaning up my mailbox. I think
this topic is still open, at least stm32mp151.dtsi still uses the
settings I changed with my patch.
On Fri, Jan 20, 2023 at 10:18:32AM +0100, Fabrice Gasnier wrote:
> On 1/12/23 12:20, Uwe Kleine-König wrote:
> > There are in sum 952 dwords available for g-rx-fifo-size,
> > g-np-tx-fifo-size and the eight entries of g-tx-fifo-size. For high
> > speed endpoints the maximal packet size is 512 (for full speed it's 64)
> > bytes. So a tx-fifo-size of more than 128 (dwords) isn't sensible.
>
> The current FIFO tuning in the device tree allows to maximize throughput
> regarding single function device.
Are you sure here? I don't claim to be an expert for usb and/or
stm32mp1, but in my understanding an allocation of 256 dwords (= 1024
bytes) will only be used in the first half when a driver requests 512
bytes. If so, it would complicate to implement the idea to dynamically
allocate the fifo chunks (sketched below).
> Having twice the packet size for the endpoint allows the controller to
> simultaneously transfer data to the USB, while gathering next data
> from the system memory.
>
> > So instead of one (too) big and several small fifos, use two big fifos
> > and to better use the remaining available space increase one of the
> > small fifos.
>
> So, I wouldn't mention "too" big here. That's a performance tuning for
> single function device use case.
>
> Drawback is this doesn't allow multi-function device, as you're trying
> to achieve (with 2 x 512 endpoints).
> Hence, the "No suitable fifo found" message you've noticed.
>
> So just on the wording, I'd rather talk about allowing multi function
> (composite) device with 512 bytes endpoints. Doing this has an impact on
> the performance for all current users in terms of performance.
>
> This change is probably fine, as it enables one additional use case, and
> it is in the SOC dtsi file.
> I'm just wondering a bit if we could/should keep current tuning in some
> board dts files ? (Or let each board vendor do their own tuning ?)
>
> Perhaps you could CC people that pushed various boards here ?
Maybe it would make sense to overwrite the setting for the existing
boards such that the boards are not affected by the change. I will
consider this if and when I prepare a v2.
> > This allows to work with CONFIG_USB_CDC_COMPOSITE (i.e. Ethernet and
> > ACM) which requires 4 endpoints with fifo sizes 512, 512, 16 and 10
> > respectively.
>
> Just a note, this one looks like a legacy driver. I think the preferred
> method is to use configfs gadget to compose a device.
The nice thing about CONFIG_USB_CDC_COMPOSITE is that it works without
userspace, e.g. for nfsroot.
> > with CONFIG_USB_CDC_COMPOSITE enabled on the old device tree, the driver
> > dies in a rather bad way:
> >
> > [ 2.472914] dwc2 49000000.usb-otg: dwc2_hsotg_ep_enable: No suitable fifo found
> > [ 2.478767] ------------[ cut here ]------------
> > [ 2.483369] WARNING: CPU: 0 PID: 0 at kernel/dma/mapping.c:532 dma_free_attrs+0xc8/0xcc
> > [ 2.491363] Modules linked in:
> > [ 2.494407] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 5.15.0-20221026-1 #1
> > [ 2.501267] Hardware name: STM32 (Device Tree Support)
> > [ 2.506409] [<c01110e8>] (unwind_backtrace) from [<c010c9c0>] (show_stack+0x18/0x1c)
> > [ 2.514129] [<c010c9c0>] (show_stack) from [<c0a83648>] (dump_stack_lvl+0x40/0x4c)
> > [ 2.521689] [<c0a83648>] (dump_stack_lvl) from [<c0136228>] (__warn+0xf4/0x150)
> > [ 2.528986] [<c0136228>] (__warn) from [<c0a7fe2c>] (warn_slowpath_fmt+0x6c/0xd0)
> > [ 2.536458] [<c0a7fe2c>] (warn_slowpath_fmt) from [<c01ad534>] (dma_free_attrs+0xc8/0xcc)
> > [ 2.544623] [<c01ad534>] (dma_free_attrs) from [<c01adbc0>] (dmam_free_coherent+0x40/0x9c)
> > [ 2.552876] [<c01adbc0>] (dmam_free_coherent) from [<c0754570>] (dwc2_hsotg_ep_enable+0x63c/0x6b0)
> > [ 2.561827] [<c0754570>] (dwc2_hsotg_ep_enable) from [<c0791a44>] (usb_ep_enable+0x40/0xf0)
> > [ 2.570167] [<c0791a44>] (usb_ep_enable) from [<c0798364>] (gether_connect+0x2c/0x1c0)
> > [ 2.578073] [<c0798364>] (gether_connect) from [<c0799f70>] (ecm_set_alt+0xcc/0x1f8)
> > [ 2.585805] [<c0799f70>] (ecm_set_alt) from [<c078d0ec>] (composite_setup+0x5bc/0x1d40)
> > [ 2.593799] [<c078d0ec>] (composite_setup) from [<c07568f0>] (dwc2_hsotg_complete_setup+0x16c/0x68c)
> > [ 2.602921] [<c07568f0>] (dwc2_hsotg_complete_setup) from [<c0755474>] (dwc2_hsotg_complete_request+0x9c/0x210)
> > [ 2.612999] [<c0755474>] (dwc2_hsotg_complete_request) from [<c0757d68>] (dwc2_hsotg_epint+0xe0c/0x1248)
> > [ 2.622470] [<c0757d68>] (dwc2_hsotg_epint) from [<c075a1a4>] (dwc2_hsotg_irq+0x9c4/0x10a4)
> > [ 2.630812] [<c075a1a4>] (dwc2_hsotg_irq) from [<c0194238>] (__handle_irq_event_percpu+0x64/0x234)
> > [ 2.639762] [<c0194238>] (__handle_irq_event_percpu) from [<c01944f0>] (handle_irq_event+0x64/0xc8)
> > [ 2.648798] [<c01944f0>] (handle_irq_event) from [<c0199028>] (handle_fasteoi_irq+0xbc/0x214)
> > [ 2.657312] [<c0199028>] (handle_fasteoi_irq) from [<c0193a9c>] (handle_domain_irq+0x84/0xb8)
> > [ 2.665827] [<c0193a9c>] (handle_domain_irq) from [<c05628b0>] (gic_handle_irq+0x84/0x98)
> > [ 2.673995] [<c05628b0>] (gic_handle_irq) from [<c0100afc>] (__irq_svc+0x5c/0x90)
> > [ 2.681466] Exception stack(0xc1001ef8 to 0xc1001f40)
> > [ 2.686509] 1ee0: 00000484 c0d6f994
> > [ 2.694680] 1f00: 00000000 c011afe0 c10f5ae0 00000000 ffffe000 c1004f54 00000000 00000000
> > [ 2.702848] 1f20: c1000000 c0e11af0 c1000000 c1001f48 c01091ec c01091f0 60000013 ffffffff
> > [ 2.711008] [<c0100afc>] (__irq_svc) from [<c01091f0>] (arch_cpu_idle+0x40/0x44)
> > [ 2.718393] [<c01091f0>] (arch_cpu_idle) from [<c0a91f40>] (default_idle_call+0x4c/0x168)
> > [ 2.726561] [<c0a91f40>] (default_idle_call) from [<c016f054>] (do_idle+0x23c/0x290)
> > [ 2.734294] [<c016f054>] (do_idle) from [<c016f3fc>] (cpu_startup_entry+0x20/0x24)
> > [ 2.741852] [<c016f3fc>] (cpu_startup_entry) from [<c0f01040>] (start_kernel+0x5e8/0x634)
> > [ 2.750020] [<c0f01040>] (start_kernel) from [<00000000>] (0x0)
> > [ 2.755929] ---[ end trace febb0e7bfc3d83c0 ]---
> >
> > so there might be another change required to fail in a nicer way.
> > (That's the WARN_ON(irqs_disabled()) in dma_free_attrs() that triggers
> > here.)
>
> Indeed, probably all dwc2 users could be affected by this (not only
> stm32). IMHO, This could be reported to the USB mailing list.
Indeed. I hope someone picks up this topic.
> > Another thought I had while tuning the tx fifo sizes was: Why is the
> > size allocation not (more) done dynamically? At least only setting a
> > fixed amount of dwords aside for g-tx-fifo-size and allocate from that
> > shouldn't be too hard, should it?
>
> Same, better place could be to suggest this directly on the USB ML (with
> Minas in CC).
Minas was already on Cc for the initial mail. I wonder if other host
cores also need this explicit fifo size allocation. Then maybe there is
some place to copy a dynamic allocation from?
> > Note I know very little about USB, so it might well be possible that I
> > missed a use case, but with this change my USB gadget works as expected.
>
> See my earlier comment on the use case. It's probably a good idea to
> update the gadget FIFO size to enable composite device with two
> functions (w/512 bytes EP).
> Could you update the commit message with these updates ?
>
> Not sure thought, if perf penalty should be handled (and how) for single
> function device use case, possibly affecting all current users.
>
> > arch/arm/boot/dts/stm32mp151.dtsi | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/arch/arm/boot/dts/stm32mp151.dtsi b/arch/arm/boot/dts/stm32mp151.dtsi
> > index 5491b6c4dec2..af70ca1f9b57 100644
> > --- a/arch/arm/boot/dts/stm32mp151.dtsi
> > +++ b/arch/arm/boot/dts/stm32mp151.dtsi
> > @@ -1137,7 +1137,7 @@ usbotg_hs: usb-otg@49000000 {
> > interrupts = <GIC_SPI 98 IRQ_TYPE_LEVEL_HIGH>;
> > g-rx-fifo-size = <512>;
> > g-np-tx-fifo-size = <32>;
> > - g-tx-fifo-size = <256 16 16 16 16 16 16 16>;
> > + g-tx-fifo-size = <128 128 64 16 16 16 16 16>;
> > dr_mode = "otg";
> > otg-rev = <0x200>;
> > usb33d-supply = <&usb33>;
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
Attachment:
signature.asc
Description: PGP signature
