Re: [PATCH] ARM: dts: bcm283x: increase dwc2's RX FIFO size

[Stefan Wahren <stefan.wahren@i2se.com>]

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):
>> Hi Pavel,
>>
>> Am 06.08.21 um 15:03 schrieb Pavel Hofman:
>>> Dne 28. 05. 21 v 10:59 Pavel Hofman napsal(a):
>>>> Dne 27. 05. 21 v 15:47 Stefan Wahren napsal(a):
>>>>
>>>>>> 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.
>>>>>>
>>>>>> 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.