Re: [PATCH] PCI: dw-rockchip: Configure max payload size on host init

Hans Zhang
2025-04-17T06:48:14Z

On 2025/4/17 14:01, Niklas Cassel wrote:
> On Thu, Apr 17, 2025 at 10:19:10AM +0800, Hans Zhang wrote:
>> On 2025/4/17 04:40, Bjorn Helgaas wrote:
>>> On Wed, Apr 16, 2025 at 11:19:26PM +0800, Hans Zhang wrote:
>>>> The RK3588's PCIe controller defaults to a 128-byte max payload size,
>>>> but its hardware capability actually supports 256 bytes. This results
>>>> in suboptimal performance with devices that support larger payloads.
>>>>
>>>> Signed-off-by: Hans Zhang <18255117159@163.com>
>>>> ---
>>>> drivers/pci/controller/dwc/pcie-dw-rockchip.c | 18 ++++++++++++++++++
>>>> 1 file changed, 18 insertions(+)
>>>>
>>>> diff --git a/drivers/pci/controller/dwc/pcie-dw-rockchip.c b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>>> index c624b7ebd118..5bbb536a2576 100644
>>>> --- a/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>>> +++ b/drivers/pci/controller/dwc/pcie-dw-rockchip.c
>>>> @@ -477,6 +477,22 @@ static irqreturn_t rockchip_pcie_ep_sys_irq_thread(int irq, void *arg)
>>>> return IRQ_HANDLED;
>>>> }
>>>> +static void rockchip_pcie_set_max_payload(struct rockchip_pcie *rockchip)
>>>> +{
>>>> + struct dw_pcie *pci = &rockchip->pci;
>>>> + u32 dev_cap, dev_ctrl;
>>>> + u16 offset;
>>>> +
>>>> + offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
>>>> + dev_cap = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCAP);
>>>> + dev_cap &= PCI_EXP_DEVCAP_PAYLOAD;
>>>> +
>>>> + dev_ctrl = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
>>>> + dev_ctrl &= ~PCI_EXP_DEVCTL_PAYLOAD;
>>>> + dev_ctrl |= dev_cap << 5;
>>>> + dw_pcie_writel_dbi(pci, offset + PCI_EXP_DEVCTL, dev_ctrl);
>>>> +}
>>>
>>> I can't really complain too much about this since meson does basically
>>> the same thing, but there are some things I don't like about this:
>>>
>>> - I don't think it's safe to set MPS higher in all cases. If we set
>>> the Root Port MPS=256, and an Endpoint only supports MPS=128, the
>>> Endpoint may do a 256-byte DMA read (assuming its MRRS>=256). In
>>> that case the RP may respond with a 256-byte payload the Endpoint
>>> can't handle. The generic code in pci_configure_mps() might be
>>> smart enough to avoid that situation, but I'm not confident about
>>> it. Maybe I could be convinced.
>>>
>>
>> Dear Bjorn,
>>
>> Thank you very much for your reply. If we set the Root Port MPS=256, and an
>> Endpoint only supports MPS=128. Finally, Root Port is also set to MPS=128 in
>> pci_configure_mps.
>
> In you example below, the Endpoint has:
> DevCap: MaxPayload 512 bytes
>
> So at least your example can't be used to prove this specific point.
> But perhaps you just wanted to show that your Max Payload Size increase
> actually works?
>

Dear Niklas,

Do you have an Endpoint with MPS=128? If so, you can also help verify
the logic of the pci_configure_mps function. I don't have an Endpoint
with MPS=128 here.

The processing logic of the pci_configure_mps function has been verified
on our own SOC platform. Please refer to the following log.
Our Root Port will set MPS=512.

0002:30:00.0 PCI bridge: Device 1f6c:0001 (prog-if 00 [Normal decode])
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop-
ParErr- Stepping- SERR- FastB2B- DisINTx+
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort-
<TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 0
Interrupt: pin A routed to IRQ 167
Bus: primary=30, secondary=31, subordinate=5f, sec-latency=0
I/O behind bridge: 300000-300fff [size=4K] [16-bit]
Memory behind bridge: 38300000-383fffff [size=1M] [32-bit]
Prefetchable memory behind bridge:
00000000fff00000-00000000000fffff [disabled] [64-bit]
Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort-
<TAbort- <MAbort- <SERR- <PERR-
Expansion ROM at 38200000 [virtual] [disabled] [size=1M]
BridgeCtl: Parity- SERR+ NoISA- VGA- VGA16- MAbort- >Reset-
FastB2B-
PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
Capabilities: [80] Power Management version 3
Flags: PMEClk- DSI- D1+ D2- AuxCurrent=0mA
PME(D0+,D1+,D2-,D3hot+,D3cold-)
Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
Capabilities: [90] MSI: Enable+ Count=1/32 Maskable+ 64bit+
Address: 000000000e060040 Data: 0000
Masking: fffffffe Pending: 00000000
Capabilities: [b0] MSI-X: Enable- Count=2 Masked-
Vector table: BAR=0 offset=00000040
PBA: BAR=0 offset=00000040
Capabilities: [c0] Express (v2) Root Port (Slot-), MSI 00
DevCap: MaxPayload 512 bytes, PhantFunc 0
ExtTag- RBE+
DevCtl: CorrErr+ NonFatalErr+ FatalErr+ UnsupReq+
RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop+
MaxPayload 256 bytes, MaxReadReq 1024 bytes

0002:31:00.0 Ethernet controller: Realtek Semiconductor Co., Ltd.
RTL8125 2.5GbE Controller (rev 05)
Subsystem: Realtek Semiconductor Co., Ltd. RTL8125 2.5GbE
Controller
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop-
ParErr- Stepping- SERR- FastB2B- DisINTx+
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort-
<TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 0, Cache Line Size: 64 bytes
Interrupt: pin A routed to IRQ 166
Region 0: I/O ports at 300000 [size=256]
Region 2: Memory at 38300000 (64-bit, non-prefetchable) [size=64K]
Region 4: Memory at 38310000 (64-bit, non-prefetchable) [size=16K]
Capabilities: [40] Power Management version 3
Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=375mA
PME(D0+,D1+,D2+,D3hot+,D3cold+)
Status: D0 NoSoftRst+ PME-Enable- DSel=0 DScale=0 PME-
Capabilities: [50] MSI: Enable- Count=1/1 Maskable+ 64bit+
Address: 0000000000000000 Data: 0000
Masking: 00000000 Pending: 00000000
Capabilities: [70] Express (v2) Endpoint, MSI 01
DevCap: MaxPayload 256 bytes, PhantFunc 0, Latency L0s
<512ns, L1 <64us
ExtTag- AttnBtn- AttnInd- PwrInd- RBE+ FLReset-
SlotPowerLimit 0W
DevCtl: CorrErr+ NonFatalErr+ FatalErr+ UnsupReq+
RlxdOrd+ ExtTag- PhantFunc- AuxPwr- NoSnoop-
MaxPayload 256 bytes, MaxReadReq 2048 bytes

hans@hans:~$ iperf3 -s
-----------------------------------------------------------
Server listening on 5201
-----------------------------------------------------------
Accepted connection from ethernet_ip, port 47114
[ 5] local ubuntu_host_ip port 5201 connected to ethernet_ip port 47122
[ ID] Interval Transfer Bitrate
[ 5] 0.00-1.00 sec 108 MBytes 902 Mbits/sec
[ 5] 1.00-2.00 sec 112 MBytes 941 Mbits/sec
[ 5] 2.00-3.00 sec 112 MBytes 941 Mbits/sec
[ 5] 3.00-4.00 sec 112 MBytes 941 Mbits/sec
[ 5] 4.00-5.00 sec 112 MBytes 941 Mbits/sec
[ 5] 5.00-6.00 sec 112 MBytes 941 Mbits/sec
[ 5] 6.00-7.00 sec 112 MBytes 941 Mbits/sec
[ 5] 7.00-8.00 sec 112 MBytes 941 Mbits/sec
[ 5] 8.00-9.00 sec 112 MBytes 941 Mbits/sec
[ 5] 9.00-10.00 sec 112 MBytes 941 Mbits/sec
[ 5] 10.00-10.04 sec 4.92 MBytes 941 Mbits/sec
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval Transfer Bitrate
[ 5] 0.00-10.04 sec 1.10 GBytes 938 Mbits/sec
receiver
-----------------------------------------------------------

root@cix-localhost:~# iperf3 -c ubuntu_host_ip
Connecting to host ubuntu_host_ip, port 5201
[ 5] local ethernet_ip port 47122 connected to ubuntu_host_ip port 5201
[ ID] Interval Transfer Bitrate Retr Cwnd
[ 5] 0.00-1.00 sec 114 MBytes 958 Mbits/sec 0 484 KBytes
[ 5] 1.00-2.00 sec 113 MBytes 946 Mbits/sec 0 535 KBytes
[ 5] 2.00-3.00 sec 112 MBytes 936 Mbits/sec 0 559 KBytes
[ 5] 3.00-4.00 sec 113 MBytes 946 Mbits/sec 0 587 KBytes
[ 5] 4.00-5.00 sec 112 MBytes 939 Mbits/sec 0 587 KBytes
[ 5] 5.00-6.00 sec 113 MBytes 948 Mbits/sec 0 587 KBytes
[ 5] 6.00-7.00 sec 112 MBytes 936 Mbits/sec 0 587 KBytes
[ 5] 7.00-8.00 sec 112 MBytes 939 Mbits/sec 0 587 KBytes
[ 5] 8.00-9.00 sec 112 MBytes 942 Mbits/sec 0 619 KBytes
[ 5] 9.00-10.00 sec 113 MBytes 945 Mbits/sec 0 677 KBytes
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval Transfer Bitrate Retr
[ 5] 0.00-10.00 sec 1.10 GBytes 944 Mbits/sec 0 sender
[ 5] 0.00-10.04 sec 1.10 GBytes 938 Mbits/sec
receiver

Best regards,
Hans