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

[Bjorn Helgaas <helgaas@kernel.org>]

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
>