Re: [PATCH v4 2/2] PCI: dwc: Remove redundant MPS configuration

Manivannan Sadhasivam
2025-06-13T06:54:53Z

On Sat, May 10, 2025 at 11:56:07PM +0800, Hans Zhang wrote:
> The Meson PCIe controller driver manually configures maximum payload
> size (MPS) through meson_set_max_payload, duplicating functionality now
> centralized in the PCI core. Deprecating redundant code simplifies the
> driver and aligns it with the consolidated MPS management strategy,
> improving long-term maintainability.
>
> Signed-off-by: Hans Zhang <18255117159@163.com>

I believe that the root port MPS set by PCI core in patch 1 should be enough to
remove the logic in the driver. But given that we already saw that is not the
case with Armada controllers, it would be good if one of the Meson maintainers
could verify if this series works as intented. Since the driver is not using the
DEVCAP value, but using the hardcoded value, I'm slightly worried that setting
MPS value other than 256 would have any downside.

But anyway, the root port MPS should be the same with and without this series.
This can be verified by:

sudo lspci -vvv | grep MaxPayload

Also, performing any benchmark and making sure that the device performance
didn't get affected would be great.

- Mani

> ---
> drivers/pci/controller/dwc/pci-meson.c | 17 -----------------
> 1 file changed, 17 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
> index db9482a113e9..126f38ed453d 100644
> --- a/drivers/pci/controller/dwc/pci-meson.c
> +++ b/drivers/pci/controller/dwc/pci-meson.c
> @@ -261,22 +261,6 @@ static int meson_size_to_payload(struct meson_pcie *mp, int size)
> return fls(size) - 8;
> }
>
> -static void meson_set_max_payload(struct meson_pcie *mp, int size)
> -{
> - struct dw_pcie *pci = &mp->pci;
> - u32 val;
> - u16 offset = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> - int max_payload_size = meson_size_to_payload(mp, size);
> -
> - val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
> - val &= ~PCI_EXP_DEVCTL_PAYLOAD;
> - dw_pcie_writel_dbi(pci, offset + PCI_EXP_DEVCTL, val);
> -
> - val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
> - val |= PCIE_CAP_MAX_PAYLOAD_SIZE(max_payload_size);
> - dw_pcie_writel_dbi(pci, offset + PCI_EXP_DEVCTL, val);
> -}
> -
> static void meson_set_max_rd_req_size(struct meson_pcie *mp, int size)
> {
> struct dw_pcie *pci = &mp->pci;
> @@ -381,7 +365,6 @@ static int meson_pcie_host_init(struct dw_pcie_rp *pp)
>
> pp->bridge->ops = &meson_pci_ops;
>
> - meson_set_max_payload(mp, MAX_PAYLOAD_SIZE);
> meson_set_max_rd_req_size(mp, MAX_READ_REQ_SIZE);
>
> return 0;
> --
> 2.25.1
>

--
மணிவண்ணன் சதாசிவம்