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

[Nicolas Saenz Julienne <nsaenzjulienne@suse.de>]

[-- Attachment #1: Type: text/plain, Size: 4309 bytes --]

Hi Pavel,

On Fri, 2020-02-07 at 22:12 +0100, Pavel Hofman wrote:
> 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.
> 
> Below is a detailed calculation which arises from dwc2 documentation:
> 
> * RAM for SETUP Packets: 4 * n + 6 locations in Scatter/Gather DMA mode
> and 5 * n+8 locations in Slave and Buffer DMA mode must be reserved in
> the RxFIFO to receive up to n SETUP packets on control endpoints, where n
> is the number of control endpoints the device controller supports.
> 
> bcm283x: 5*n+8. The Broadcom core has 1 control EP (n=1), so 13 locations
> 
> * One location for Global OUT NAK
> 
> bcm283x: 1 location
> 
> * Status information is written to the FIFO along with each received
> packet. Therefore, a minimum space of (Largest Packet Size / 4) + 1 must
> be allotted to receive packets. If a high-bandwidth endpoint is enabled,
> or multiple isochronous endpoints are enabled, then at least two (Largest
> Packet Size / 4) + 1 spaces must be allotted to receive back-to-back
> packets. Typically, two (Largest Packet Size / 4) + 1 spaces are
> recommended so that when the previous packet is being transferred to AHB,
> the USB can receive the subsequent packet. If AHB latency is high, you
> must allocate enough space to receive multiple packets. This is critical
> to prevent dropping of any isochronous packets.
> 
> bcm283x: (1024/4) +1 = 257 locations. For MC >1: 2 * 257 = 514 locations
> 
> * Along with last packet of each endpoint, transfer complete status
> information is also pushed in to the FIFO.
> 
> bcm283x: The core should have 5 IN and 5 OUT EP's.
> 1 location for each EP - 10 locations.
> 
> It's for the case when all EPs are simultaneously completing transfers.
> 
> * An additional location for EPDisable status for each endpoint is
> also required.
> 
> bcm283x: 1 location for each EP - 10 EP's - 10 locations
> It's for case if EP simultaneously disabled.
> 
> * Typically, two locations for each OUT endpoint is recommended.
> 
> bcm283x: 5*2 = 10 locations
> 
> Total: 13 + 1 + 257 + 10 +10 + 10 = 301 locations
> 
> Safer is 301 + 257 (for MC>1) = 558 locations.
> 
> Signed-off-by: Phil Elwell <phil@raspberrypi.com>
> Signed-off-by: Pavel Hofman <pavel.hofman@ivitera.com>

It'd be nice if you summarized a little. Halfway between this and the first
revision of this patch. For example:

[...]

RX FIFO's size is calculated based on the following (in 4byte words):
 - 13 location for SETUP packets
 - 1 location for Global OUT NAK
 - 2 * 257 locations for status information and the received packet. Note that
   typically two spaces are recommended so that when the previous packet is
   being transferred to AHB, the USB can receive the subsequent packet.
 - etc...

Also, what is MC in your description? If in doubt just drop it a stick with the
explanation above.

Regards,
Nicolas

> ---
>  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>;
>  	g-np-tx-fifo-size = <32>;
>  	g-tx-fifo-size = <256 256 512 512 512 768 768>;
>  };


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]