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

Bjorn Helgaas
2025-04-16T20:40:53Z

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.

- 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 commit log is specific about Max_Payload_Size Supported being
256 bytes, but the patch actually reads the value from Device
Capabilities.

- I'd like to see FIELD_PREP()/FIELD_GET() used when possible.
PCIE_LTSSM_STATUS_MASK is probably the only other place.

These would be material for a separate patch:

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

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