[PATCH] PCI: dw-rockchip: Configure max payload size on host init
Hans Zhang
18255117159 at 163.com
Wed Apr 16 19:19:10 PDT 2025
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 at 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.
lspci information before the patch was submitted:
root at firefly:~# lspci -vvv
00:00.0 PCI bridge: Fuzhou Rockchip Electronics Co., Ltd Device 3588
(rev 01) (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 73
Bus: primary=00, secondary=01, subordinate=ff, sec-latency=0
I/O behind bridge: 0000f000-00000fff [disabled]
Memory behind bridge: f0200000-f02fffff [size=1M]
Prefetchable memory behind bridge:
00000000fff00000-00000000000fffff [disabled]
Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort-
<TAbort- <MAbort- <SERR- <PERR-
Expansion ROM at f0300000 [virtual] [disabled] [size=64K]
BridgeCtl: Parity- SERR+ NoISA- VGA- VGA16- MAbort- >Reset-
FastB2B-
PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
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=16/32 Maskable+ 64bit+
Address: 00000000fe670040 Data: 0000
Masking: fffffeff Pending: 00000000
Capabilities: [70] Express (v2) Root Port (Slot-), MSI 08
DevCap: MaxPayload 256 bytes, PhantFunc 0
ExtTag+ RBE+
DevCtl: CorrErr- NonFatalErr- FatalErr- UnsupReq-
RlxdOrd+ ExtTag+ PhantFunc- AuxPwr- NoSnoop-
MaxPayload 128 bytes, MaxReadReq 512 bytes
01:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd
Device a80c (prog-if 02 [NVM Express])
Subsystem: Samsung Electronics Co Ltd Device a801
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 72
Region 0: Memory at f0200000 (64-bit, non-prefetchable) [size=16K]
Capabilities: [40] 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: [50] MSI: Enable- Count=1/32 Maskable- 64bit+
Address: 0000000000000000 Data: 0000
Capabilities: [70] Express (v2) Endpoint, MSI 00
DevCap: MaxPayload 512 bytes, PhantFunc 0, Latency L0s
unlimited, L1 unlimited
ExtTag+ AttnBtn- AttnInd- PwrInd- RBE+ FLReset+
SlotPowerLimit 0.000W
DevCtl: CorrErr- NonFatalErr- FatalErr- UnsupReq-
RlxdOrd+ ExtTag+ PhantFunc- AuxPwr- NoSnoop+
FLReset-
MaxPayload 128 bytes, MaxReadReq 512 bytes
lspci information after the patch was submitted:
root at firefly:~# lspci -vvv
00:00.0 PCI bridge: Fuzhou Rockchip Electronics Co., Ltd Device 3588
(rev 01) (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 73
Bus: primary=00, secondary=01, subordinate=ff, sec-latency=0
I/O behind bridge: 0000f000-00000fff [disabled]
Memory behind bridge: f0200000-f02fffff [size=1M]
Prefetchable memory behind bridge:
00000000fff00000-00000000000fffff [disabled]
Secondary status: 66MHz- FastB2B- ParErr- DEVSEL=fast >TAbort-
<TAbort- <MAbort- <SERR- <PERR-
Expansion ROM at f0300000 [virtual] [disabled] [size=64K]
BridgeCtl: Parity- SERR+ NoISA- VGA- VGA16- MAbort- >Reset-
FastB2B-
PriDiscTmr- SecDiscTmr- DiscTmrStat- DiscTmrSERREn-
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=16/32 Maskable+ 64bit+
Address: 00000000fe670040 Data: 0000
Masking: fffffeff Pending: 00000000
Capabilities: [70] Express (v2) Root Port (Slot-), MSI 08
DevCap: MaxPayload 256 bytes, PhantFunc 0
ExtTag+ RBE+
DevCtl: CorrErr- NonFatalErr- FatalErr- UnsupReq-
RlxdOrd+ ExtTag+ PhantFunc- AuxPwr- NoSnoop-
MaxPayload 256 bytes, MaxReadReq 512 bytes
01:00.0 Non-Volatile memory controller: Samsung Electronics Co Ltd
Device a80c (prog-if 02 [NVM Express])
Subsystem: Samsung Electronics Co Ltd Device a801
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 72
Region 0: Memory at f0200000 (64-bit, non-prefetchable) [size=16K]
Capabilities: [40] 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: [50] MSI: Enable- Count=1/32 Maskable- 64bit+
Address: 0000000000000000 Data: 0000
Capabilities: [70] Express (v2) Endpoint, MSI 00
DevCap: MaxPayload 512 bytes, PhantFunc 0, Latency L0s
unlimited, L1 unlimited
ExtTag+ AttnBtn- AttnInd- PwrInd- RBE+ FLReset+
SlotPowerLimit 0.000W
DevCtl: CorrErr- NonFatalErr- FatalErr- UnsupReq-
RlxdOrd+ ExtTag+ PhantFunc- AuxPwr- NoSnoop+
FLReset-
MaxPayload 256 bytes, MaxReadReq 512 bytes
Here are my tests and the NVMe SSD worked fine.
root at firefly:~# df -h
Filesystem Size Used Avail Use% Mounted on
......
/dev/nvme0n1 916G 28K 870G 1% /root/nvme
......
root at firefly:~#
root at firefly:~# ls -l nvme/
total 16
drwx------ 2 root root 16384 Dec 24 06:34 lost+found
root at firefly:~#
root at firefly:~# cd nvme/
root at firefly:~/nvme# time dd if=/dev/zero of=test bs=1M count=1024
1024+0 records in
1024+0 records out
1073741824 bytes (1.1 GB, 1.0 GiB) copied, 0.875072 s, 1.2 GB/s
real 0m0.881s
user 0m0.001s
sys 0m0.874s
root at firefly:~/nvme# ls -l
total 1048596
drwx------ 2 root root 16384 Dec 24 06:34 lost+found
-rw-r--r-- 1 root root 1073741824 Apr 17 02:11 test
root at firefly:~/nvme# time cp -rf test test1
real 0m0.901s
user 0m0.005s
sys 0m0.889s
root at firefly:~/nvme# ls -lh
total 2.1G
drwx------ 2 root root 16K Dec 24 06:34 lost+found
-rw-r--r-- 1 root root 1.0G Apr 17 02:11 test
-rw-r--r-- 1 root root 1.0G Apr 17 02:12 test1
> - There's nothing rockchip-specific about this.
>
> - It's very similar to meson_set_max_payload(), so it'd be nice to
> share that code somehow.
The next version will be added to DWC.
>
> - The commit log is specific about Max_Payload_Size Supported being
> 256 bytes, but the patch actually reads the value from Device
> Capabilities.
The commit log will be modified.
>
> - I'd like to see FIELD_PREP()/FIELD_GET() used when possible.
> PCIE_LTSSM_STATUS_MASK is probably the only other place.
>
Will change.
> These would be material for a separate patch:
>
Thank you very much for your reminding and advice. I will submit another
patch separately for modification.
> - The #defines for register offsets and bits are kind of a mess,
> e.g., PCIE_SMLH_LINKUP, PCIE_RDLH_LINKUP, PCIE_LINKUP,
> PCIE_L0S_ENTRY, and PCIE_LTSSM_STATUS_MASK are in
> PCIE_CLIENT_LTSSM_STATUS, but you couldn't tell that from the
> names, and they're not even defined together.
>
> - Same for PCIE_RDLH_LINK_UP_CHGED, PCIE_LINK_REQ_RST_NOT_INT,
> PCIE_RDLH_LINK_UP_CHGED, which are in
> PCIE_CLIENT_INTR_STATUS_MISC.
>
> - PCIE_LTSSM_ENABLE_ENHANCE is apparently in
> PCIE_CLIENT_HOT_RESET_CTRL? Sure wouldn't guess that from the
> names or the order of #defines.
>
> - PCIE_CLIENT_GENERAL_DEBUG isn't used at all.
Will delete.
Best regard,
Hans
>
>> static int rockchip_pcie_configure_rc(struct platform_device *pdev,
>> struct rockchip_pcie *rockchip)
>> {
>> @@ -511,6 +527,8 @@ static int rockchip_pcie_configure_rc(struct platform_device *pdev,
>> pp->ops = &rockchip_pcie_host_ops;
>> pp->use_linkup_irq = true;
>>
>> + rockchip_pcie_set_max_payload(rockchip);
>> +
>> ret = dw_pcie_host_init(pp);
>> if (ret) {
>> dev_err(dev, "failed to initialize host\n");
>>
>> base-commit: a24588245776dafc227243a01bfbeb8a59bafba9
>> --
>> 2.25.1
>>
More information about the linux-arm-kernel
mailing list