linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Pavel Hofman <pavel.hofman@ivitera.com>
To: Stefan Wahren <stefan.wahren@i2se.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,
	Phil Elwell <phil@raspberrypi.com>
Subject: Re: [PATCH] ARM: dts: bcm283x: increase dwc2's RX FIFO size
Date: Fri, 6 Aug 2021 16:46:25 +0200	[thread overview]
Message-ID: <791c432f-fd65-870f-3167-6b2056d6d5bd@ivitera.com> (raw)
In-Reply-To: <76d98dbe-61b3-148e-e9d6-ceba1db600d6@i2se.com>

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.

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. 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?

* Why does mainline config have different RX and TX sizes than the 
RPi-vendor specific config (which I happen/ed to use)?

Maybe these questions should be resolved, allowing for safer developing 
the gadget on the RPi hardware.

Best regards,

Pavel.

  reply	other threads:[~2021-08-06 14:46 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
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 [this message]
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=791c432f-fd65-870f-3167-6b2056d6d5bd@ivitera.com \
    --to=pavel.hofman@ivitera.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=phil@raspberrypi.com \
    --cc=robh+dt@kernel.org \
    --cc=stefan.wahren@i2se.com \
    /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).