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

[Stefan Wahren]

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.

Best regards
Stefan

>
> Thanks for considering.
>
> Best regards,
>
> Pavel.
>
>
>