Re: [Linux-stm32] [PATCH] ARM: dts: stm32mp15x: adjust USB OTG gadget tx fifo sizes

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[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


[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux