Hi Pavel,

Am 06.08.21 um 16:46 schrieb Pavel Hofman:

Hi Stefan,

Dne 06. 08. 21 v 16:08 Stefan Wahren napsal(a):

quoted

Hi Pavel,

Am 06.08.21 um 15:03 schrieb Pavel Hofman:

quoted

Dne 28. 05. 21 v 10:59 Pavel Hofman napsal(a):

quoted

Dne 27. 05. 21 v 15:47 Stefan Wahren napsal(a):

quoted

quoted

I think I see the problem.

IIUC the calculations and checks, all g-tx-fifo-size values +
g-rx-fifo-size + g-np-tx-fifo-size must not exceed
total_fifo_size. My
RPi4 reports the total_fifo_size as 4080 (in
/sys/kernel/debug/usb/fe980000.usb/hw_params).

Linux mainline
https://github.com/torvalds/linux/search?p=3&q=g-tx-fifo-size :

The increase in value of g-rx-fifo-size exceeds the limit for the
DTSI
files we patched:

Both bcm283x-rpi-usb-peripheral.dtsi and bcm283x-rpi-usb-otg.dtsi:
558 + 32 + 256 + 256 + 512 + 512 + 512 + 768 + 768 = 4174 > 4080

while the sum with the previous value of 256 reached just 3872 <
4080.


The raspberrypi repo
https://github.com/raspberrypi/linux/search?q=g-tx-fifo-size :

It has a different mix of the DTSI files
dwc2-overlay.dts
upstream-overlay.dts
upstream-pi4-overlay.dts

yes these overlay files are vendor specific and doesn't exist in
mainline. The upstream*dts were intended to "simulate" mainline
behavior, but unfortunately differ in this case.

quoted

all of which define
g-tx-fifo-size = <512 512 512 512 512 256 256>;

Here the calculation holds:
558 + 32 + 512 + 512 + 512 + 512 + 512 + 256 + 256 = 3662 < 4080

My RPi4 uses one of these DTSIs, because my
/sys/kernel/debug/usb/fe980000.usb/params says:

g_rx_fifo_size                : 558
g_np_tx_fifo_size             : 32
g_tx_fifo_size[0]             : 0
g_tx_fifo_size[1]             : 512
g_tx_fifo_size[2]             : 512
g_tx_fifo_size[3]             : 512
g_tx_fifo_size[4]             : 512
g_tx_fifo_size[5]             : 512
g_tx_fifo_size[6]             : 256
g_tx_fifo_size[7]             : 256


IIUC the tx_fifo values in bcm283x-rpi-usb-peripheral.dtsi and
bcm283x-rpi-usb-otg.dtsi files can be lowered to the values used and
tested (at least by me) in the RPi repo. But this is outside of my
knowledge, honestly I do not know what is the most appropriate
distribution of the remaining fifo space among the g_tx_fifo
buffers.
Please can the RPi developers (Phil?) suggest a fix?

As author of the mainline bcm283x-rpi-usb-otg.dtsi i was trying to
optimize the fifo sizes for EP 6 and 7. But i don't remember why.
So my
suggestion for a fix would be:

g-tx-fifo-size = <256 256 256 512 512 768 768>;

But i'm also unsure about the values.

IIUC this code
https://github.com/torvalds/linux/blob/master/drivers/usb/dwc2/gadget.c#L4091


optimizes the FIFO assignment to endpoints. From that I would conclude
that correct values are specific for each use-case configuration of
endpoints. Maybe a varied selection (256, 512, 768) is more convenient
than just 256 and 512. I really do not know what use cases need
what TX
fifo values.

My patch raising  g-rx-fifo-size = 558 has been reverted back to
g-rx-fifo-size = 256 in upstream. 256 is clearly a wrong value. 558 is
enough for 2 packets per microframe. How about raising the value in
the mainline DTS files to 301 instead which will correctly work with 1
packet per microframe (the most common scenario) and comply with the
4080 limit of the RX + all TXs sum of the TX configs in the mainline?

thank for your feedback. It has been reverted because the last patch
broke USB completely on Raspberry Pi Zero and the only explanation for
me is it has never been tested. The workflow is that the submitter is
responsibly for testing. In case this is not possible the patch must be
tagged with RFT or at least it must be mentioned in the commit message.

In case you want to have a different value, please feel free to send a
patch, but please test it against a mainline kernel before. In case you
have problems with it, feel free to ask.

Sorry, in case this sounds grumpy but it's very annoying to hunt down
especially avoidable regressions with every kernel release. This wastes
other developers time to get their patches upstream.

I understand your points. I really did not test the patch with
mainline combination of the TX values, sorry for that. I have no
problem with the revert at all, just pointing out that the value of
256 is incorrect. It took a number of hours with patient help of Minas
to find the culprit of the dwc2 gadget dropping audio samples with
packet sizes over 980 bytes.

believe me, i understand this absolutely as the author of the mainline
Raspberry Pi Zero DTS (back in 2017). In the old days there were a lot
of issues in the DWC2. It took until ~ 4.14 to get a proper working USB
host mode.

However, even if I did test and changed the TX values on my RPi4
accordingly, I would not have been able to test on RPi Zero and the
other RPi models. 

This doesn't matter. The USB IP is always the same. The mentioned issue
was also on the Raspberry Pi 4, but nobody notices this (using Raspberry
Pi 4 as USB gadget is very special). But for the RPI Zero this issue was
very critical.

The questions are:

* Why did your TX values, changed to comply with the 4080 limit, break
RPi Zero, what are the rules apart of the max sum of 4080?

Unfortunately i don't have access to the DWC2 reference manual and the
time to figure out all these constrains.

* Why does mainline config have different RX and TX sizes than the
RPi-vendor specific config (which I happen/ed to use)?

For my initial version of the DTS i took some working values, i don't
remember exactly. During time the values in the vendor tree changed.
Nobody upstreamed the later changes.

I'm fine with changing all to RPi-vendor specific settings, as long as
it works with OTG, Gadget mode, with and without USB hub, ...

I don't have a strong opinion for these magic numbers. Currently i'm
busy in my spare time with CM4 upstreaming, so not much time for this topic.

I hope this helps.

Best regards
Stefan

Maybe these questions should be resolved, allowing for safer
developing the gadget on the RPi hardware.

Best regards,

Pavel.