linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Stefan Wahren <stefan.wahren@i2se.com>
To: Pavel Hofman <pavel.hofman@ivitera.com>,
	Minas Harutyunyan <hminas@synopsys.com>,
	Rob Herring <robh+dt@kernel.org>,
	Nicolas Saenz Julienne <nsaenz@kernel.org>,
	linux-usb@vger.kernel.org, devicetree@vger.kernel.org,
	linux-rpi-kernel@lists.infradead.org
Subject: Re: [PATCH] ARM: dts: bcm283x: increase dwc2's RX FIFO size
Date: Thu, 27 May 2021 15:47:39 +0200	[thread overview]
Message-ID: <f9c90203-a67e-0e33-09a8-f173af63e771@i2se.com> (raw)
In-Reply-To: <7c2e3e1f-db10-b376-9b87-ef93c8d8289c@ivitera.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.


  reply	other threads:[~2021-05-27 13:47 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-13  7:18 [PATCH] ARM: dts: bcm283x: increase dwc2's RX FIFO size Pavel Hofman
2020-11-20 16:44 ` Nicolas Saenz Julienne
2021-05-26 17:12 ` Stefan Wahren
2021-05-27 13:17   ` Pavel Hofman
2021-05-27 13:47     ` Stefan Wahren [this message]
2021-05-28  8:59       ` Pavel Hofman
2021-08-06 13:03         ` Pavel Hofman
2021-08-06 14:08           ` Stefan Wahren
2021-08-06 14:46             ` Pavel Hofman
2021-08-06 15:57               ` Stefan Wahren
  -- strict thread matches above, loose matches on Subject: below --
2020-02-07 21:12 Pavel Hofman
2020-02-12 18:57 ` Nicolas Saenz Julienne

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f9c90203-a67e-0e33-09a8-f173af63e771@i2se.com \
    --to=stefan.wahren@i2se.com \
    --cc=devicetree@vger.kernel.org \
    --cc=hminas@synopsys.com \
    --cc=linux-rpi-kernel@lists.infradead.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=nsaenz@kernel.org \
    --cc=pavel.hofman@ivitera.com \
    --cc=robh+dt@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).