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

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

Hi Pavel,

fix Nicolas address. Sorry about this.

Am 27.05.21 um 15:17 schrieb Pavel Hofman:
> Hi Stefan,
>
> Dne 26. 05. 21 v 19:12 Stefan Wahren napsal(a):
>> Am 13.11.20 um 08:18 schrieb Pavel Hofman:
>>> The previous version of the dwc2 overlay set the RX FIFO size to
>>> 256 4-byte words. This is not enough for 1024 bytes of the largest
>>> isochronous high speed packet allowed, because it doesn't take into
>>> account extra space needed by dwc2.
>>>
>>> RX FIFO's size is calculated based on the following (in 4byte words):
>>> - 13 locations for SETUP packets
>>>   5*n + 8 for Slave and Buffer DMA mode where n is number of control
>>>   endpoints which is 1 on the bcm283x core
>>>
>>> - 1 location for Global OUT NAK
>>>
>>> - 2 * 257 locations for status information and the received packet.
>>>   Typically two spaces are recommended so that when the previous packet
>>>   is being transferred to AHB, the USB can receive the subsequent
>>>   packet.
>>>
>>> - 10 * 1 location for transfer complete status for last packet of each
>>>   endpoint. The bcm283x core has 5 IN and 5 OUT EPs
>>>
>>> - 10 * 1 additional location for EPDisable status for each endpoint
>>>
>>> - 5 * 2 additional locations are recommended for each OUT endpoint
>>>
>>> Total is 558 locations.
>>>
>>> Signed-off-by: Phil Elwell <phil@raspberrypi.com>
>>> Signed-off-by: Pavel Hofman <pavel.hofman@ivitera.com>
>>> ---
>>>  arch/arm/boot/dts/bcm283x-rpi-usb-otg.dtsi        | 2 +-
>>>  arch/arm/boot/dts/bcm283x-rpi-usb-peripheral.dtsi | 2 +-
>>>  2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/arm/boot/dts/bcm283x-rpi-usb-otg.dtsi
>>> b/arch/arm/boot/dts/bcm283x-rpi-usb-otg.dtsi
>>> index e2fd961..20322de 100644
>>> --- a/arch/arm/boot/dts/bcm283x-rpi-usb-otg.dtsi
>>> +++ b/arch/arm/boot/dts/bcm283x-rpi-usb-otg.dtsi
>>> @@ -1,7 +1,7 @@
>>>  // SPDX-License-Identifier: GPL-2.0
>>>  &usb {
>>>  	dr_mode = "otg";
>>> -	g-rx-fifo-size = <256>;
>>> +	g-rx-fifo-size = <558>;
>>>  	g-np-tx-fifo-size = <32>;
>>>  	/*
>>>  	 * According to dwc2 the sum of all device EP
>>> diff --git a/arch/arm/boot/dts/bcm283x-rpi-usb-peripheral.dtsi
>>> b/arch/arm/boot/dts/bcm283x-rpi-usb-peripheral.dtsi
>>> index 0ff0e9e..1409d1b 100644
>>> --- a/arch/arm/boot/dts/bcm283x-rpi-usb-peripheral.dtsi
>>> +++ b/arch/arm/boot/dts/bcm283x-rpi-usb-peripheral.dtsi
>>> @@ -1,7 +1,7 @@
>>>  // SPDX-License-Identifier: GPL-2.0
>>>  &usb {
>>>  	dr_mode = "peripheral";
>>> -	g-rx-fifo-size = <256>;
>>> +	g-rx-fifo-size = <558>;
>> sorry for being late at the party, but this change introduce a
>> regression on Raspberry Pi 4 B:
>>
>> dwc2 fe980000.usb: dwc2_check_param_tx_fifo_sizes: Invalid parameter
>> g-tx-fifo-size, setting to default average
>>
>> I known you didn't change the tx fifo size, but there are complex
>> constrains regarding the total fifo size.
>>
>> Are you able to test this with a mainline kernel (not Raspberry Pi
>> kernel) and send a fix for this?
>>
>>>  	g-np-tx-fifo-size = <32>;
>>>  	g-tx-fifo-size = <256 256 512 512 512 768 768>;
>>>  };
> 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.

>
> Thanks a lot to Stefan and to everyone involved.
>
> Pavel.