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

[Niklas Cassel <cassel@kernel.org>]

On Thu, Apr 17, 2025 at 03:08:34PM +0800, Shawn Lin wrote:
> 在 2025/04/17 星期四 15:04, Niklas Cassel 写道:
> > Hello Hans,
> > 
> > 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.
> > 
> > Patch looks good to me, but please always reference the TRM when you can.
> > 
> > Before this patch:
> > 		DevCap: MaxPayload 256 bytes
> > 		DevCtl: MaxPayload 128 bytes
> > 
> > 
> > As per rk3588 TRM, section "11.4.3.8 DSP_PCIE_CAP Detail Registers Description"
> > 
> > DevCap is per the register description of DSP_PCIE_CAP_DEVICE_CAPABILITIES_REG,
> > field PCIE_CAP_MAX_PAYLOAD_SIZE.
> > Which claims that the value after reset is 0x1 (256B).
> > 
> > DevCtl is per the register description of
> > DSP_PCIE_CAP_DEVICE_CONTROL_DEVICE_STATUS, field PCIE_CAP_MAX_PAYLOAD_SIZE_CS.
> > Which claims that the reset value is 0x0 (128B).
> > 
> > Both of these match the values above.
> > 
> > As per the description of PCIE_CAP_MAX_PAYLOAD_SIZE_CS:
> > "Permissible values that
> > can be programmed are indicated by the Max_Payload_Size
> > Supported field (PCIE_CAP_MAX_PAYLOAD_SIZE) in the Device
> > Capabilities (DEVICE_CAPABILITIES_REG) register (for more
> > details, see section 7.5.3.3 of PCI Express Base Specification)."
> > 
> > So your patch looks good.
> > 
> > I guess I'm mostly surprised that the e.g. pci_configure_mps() does not
> > already set DevCtl to the max(DevCap.MPS of the host, DevCap.MPS of the
> > endpoint).
> > 
> > Apparently pci_configure_mps() only decreases MPS from the reset values?
> > It never increases it?
> > 
> 
> Actually it does:
> 
> https://github.com/torvalds/linux/blob/master/Documentation/admin-guide/kernel-parameters.txt#L4757

If that is the case, then explain the before/after with Hans lspci output here:
https://lore.kernel.org/linux-pci/bb40385c-6839-484c-90b2-d6c7ecb95ba9@163.com/

His patch changes the default value of DevCtl.MPS (from 128B to 256B), but if
pci_configure_mps() can bump DevCtl.MPS to a higher value, his patch should not
be needed, since the EP (an NVMe SSD in his case) has DevCap.MPS 512B, and the
RC itself has DevCap.MPS 256B.

Seems like we are missing something here.


Kind regards,
Niklas