[PATCH v8 0/2] PCI: Configure Root Port MPS during host probing

[PATCH v8 0/2] PCI: Configure Root Port MPS during host probing

[Hans Zhang]

Current PCIe initialization exhibits a key optimization gap: Root Ports
may operate with non-optimal Maximum Payload Size (MPS) settings. While
downstream device configuration is handled during bus enumeration, Root
Port MPS values inherited from firmware or hardware defaults often fail
to utilize the full capabilities supported by controller hardware. This
results in suboptimal data transfer efficiency throughout the PCIe
hierarchy.

This patch series addresses this by:

1. Core PCI enhancement (Patch 1):
- Proactively configures Root Port MPS during host controller probing
- Sets initial MPS to hardware maximum (128 << dev->pcie_mpss)
- Conditional on PCIe bus tuning being enabled (PCIE_BUS_TUNE_OFF unset)
  and not in PCIE_BUS_PEER2PEER mode (which requires default 128 bytes)
- Maintains backward compatibility via PCIE_BUS_TUNE_OFF check
- Preserves standard MPS negotiation during downstream enumeration

2. Driver cleanup (Patch 2):
- Removes redundant MPS configuration from Meson PCIe controller driver
- Functionality is now centralized in PCI core
- Simplifies driver maintenance long-term

---
Changes in v8:
- Rebase to v7.3-rc1.

Changes in v7:
https://patchwork.kernel.org/project/linux-pci/cover/20251127170908.14850-1-18255117159@163.com/

- Exclude PCIE_BUS_PEER2PEER mode from Root Port MPS configuration
- Remove redundant check for upstream bridge (Root Ports don't have one)
- Improve commit message and code comments as per Bjorn.

Changes for v6:
https://patchwork.kernel.org/project/linux-pci/patch/20251104165125.174168-1-18255117159@163.com/

- Modify the commit message and comments. (Bjorn)
- Patch 1/2 code logic: Add !bridge check to configure MPS only for Root Ports
  without an upstream bridge (root bridges), avoiding incorrect handling of
  non-root-bridge Root Ports (Niklas).

Changes for v5:
https://patchwork.kernel.org/project/linux-pci/patch/20250620155507.1022099-1-18255117159@163.com/

- Use pcie_set_mps directly instead of pcie_write_mps.
- The patch 1 commit message were modified.

Changes for v4:
https://patchwork.kernel.org/project/linux-pci/patch/20250510155607.390687-1-18255117159@163.com/

- The patch [v4 1/2] add a comment to explain why it was done this way.
- The patch [v4 2/2] have not been modified.
- Drop patch [v3 3/3]. The Maintainer of the pci-aardvark.c file suggests
  that this patch cannot be submitted. In addition, Mani also suggests
  dropping this patch until this series of issues is resolved.

Changes for v3:
https://patchwork.kernel.org/project/linux-pci/patch/20250506173439.292460-1-18255117159@163.com/

- The new split is patch 2/3 and 3/3.
- Modify the patch 1/3 according to Niklas' suggestion.

Changes for v2:
https://patchwork.kernel.org/project/linux-pci/patch/20250425095708.32662-1-18255117159@163.com/

- According to the Maintainer's suggestion, limit the setting of MPS
  changes to platforms with controller drivers.
- Delete the MPS code set by the SOC manufacturer.
---

Hans Zhang (2):
  PCI: Configure Root Port MPS during host probing
  PCI: dwc: Remove redundant MPS configuration

 drivers/pci/controller/dwc/pci-meson.c | 17 -----------------
 drivers/pci/probe.c                    | 12 ++++++++++++
 2 files changed, 12 insertions(+), 17 deletions(-)


base-commit: bc35965f6940a9bf834d54187b6088b8eb09206d
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

[PATCH v8 1/2] PCI: Configure Root Port MPS during host probing

[Hans Zhang]

Current PCIe initialization logic may leave Root Ports operating with
non-optimal Maximum Payload Size (MPS) settings. The existing code in
pci_configure_mps() returns early for devices without an upstream bridge
which includes Root Ports, so their MPS values remain at firmware
defaults. This fails to utilize the controller's full capabilities,
leading to suboptimal data transfer efficiency across the PCIe hierarchy.

With this patch, during the host controller probing phase:
- When PCIe bus tuning is enabled (not PCIE_BUS_TUNE_OFF) and not
  PCIE_BUS_PEER2PEER (which requires the default 128 bytes for optimal
  peer-to-peer operation), and
- The device is a Root Port, the Root Port's MPS is set to its maximum
  supported value.

Note that this initial maximum MPS setting may be reduced later, during
downstream device enumeration, if any downstream device does not support
the Root Port's maximum MPS.

This change ensures Root Ports are initialized to their maximum MPS before
downstream devices negotiate MPS, while maintaining backward compatibility
via the PCIE_BUS_TUNE_OFF check and not interfering with the
PCIE_BUS_PEER2PEER strategy.

Suggested-by: Niklas Cassel <cassel@kernel.org>
Suggested-by: Manivannan Sadhasivam <mani@kernel.org>
Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Hans Zhang <18255117159@163.com>
Tested-by: Mahesh Vaidya <mahesh.vaidya@altera.com>
Tested-by: Ricardo Pardini <ricardo@pardini.net> # on Odroid-HC4 
Tested-by: Shawn Lin <shawn.lin@rock-chips.com>
---
 drivers/pci/probe.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 27008e2ea5af..dc4747858918 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2229,6 +2229,18 @@ static void pci_configure_mps(struct pci_dev *dev)
 		return;
 	}
 
+	/*
+	 * Unless MPS strategy is PCIE_BUS_TUNE_OFF (don't touch MPS at all) or
+	 * PCIE_BUS_PEER2PEER (use minimum MPS for peer-to-peer), set Root Ports'
+	 * MPS to their maximum supported value. Depending on the MPS strategy
+	 * and MPSS of downstream devices, a Root Port's MPS may be reduced
+	 * later during device enumeration.
+	 */
+	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
+	    pcie_bus_config != PCIE_BUS_TUNE_OFF &&
+	    pcie_bus_config != PCIE_BUS_PEER2PEER)
+		pcie_set_mps(dev, 128 << dev->pcie_mpss);
+
 	if (!bridge || !pci_is_pcie(bridge))
 		return;
 
-- 
2.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

Re: [PATCH v8 1/2] PCI: Configure Root Port MPS during host probing

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Logic Error / PCIe MPS Mismatch. Unconditionally configuring the Root Port MPS to its maximum supported value breaks devices connected via PCIe switches under the default MPS strategy (PCIE_BUS_DEFAULT).
- [Medium] Missing Error Handling: The return value of `pcie_set_mps()` is silently ignored when configuring the Root Port MPS.
--

commit 13816df0ca2fe3070f644d4bbf35fd712be22285
Author: Hans Zhang <18255117159@163.com>

PCI: Configure Root Port MPS during host probing

Current PCIe initialization logic may leave Root Ports operating with
non-optimal Maximum Payload Size (MPS) settings. This change ensures
Root Ports are initialized to their maximum MPS before downstream devices
negotiate MPS, while not interfering with the PCIE_BUS_PEER2PEER strategy.

> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 27008e2ea5afc..dc47478589182 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2229,6 +2229,18 @@ static void pci_configure_mps(struct pci_dev *dev)
>  		return;
>  	}
>  
> +	/*
> +	 * Unless MPS strategy is PCIE_BUS_TUNE_OFF (don't touch MPS at all) or
> +	 * PCIE_BUS_PEER2PEER (use minimum MPS for peer-to-peer), set Root Ports'
> +	 * MPS to their maximum supported value. Depending on the MPS strategy
> +	 * and MPSS of downstream devices, a Root Port's MPS may be reduced
> +	 * later during device enumeration.
> +	 */
> +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
> +	    pcie_bus_config != PCIE_BUS_TUNE_OFF &&
> +	    pcie_bus_config != PCIE_BUS_PEER2PEER)
> +		pcie_set_mps(dev, 128 << dev->pcie_mpss);

[Severity: High]
Does unconditionally setting the Root Port MPS to maximum break devices
connected via PCIe switches under the default MPS strategy (PCIE_BUS_DEFAULT)?

If a system has a PCIe switch and the Root Port supports a larger MPS
(e.g., 256B) than the endpoint behind the switch (e.g., 128B), it appears this
would cause a mismatch.

During top-down enumeration, this new code unconditionally sets the Root Port
MPS to max (256B), and the switch ports inherit 256B. When the endpoint is
later enumerated in pci_configure_mps(), it hits this pre-existing logic
further down in the function:

    mpss = 128 << dev->pcie_mpss;
    if (mpss < p_mps && pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT) {
        pcie_set_mps(bridge, mpss);

Because the upstream bridge is a Switch Downstream Port and not a Root Port,
the condition pci_pcie_type(bridge) == PCI_EXP_TYPE_ROOT_PORT evaluates to
false.

Since the MPS reduction strictly limits itself to Root Ports, the MPS of the
switch is not reduced, leaving it at 256B. The endpoint then fails to set its
MPS to 256B, returning -EINVAL and permanently leaving a mismatch which could
lead to dropped packets as Malformed TLPs.

[Severity: Medium]
Are we missing an error check here?

If the configuration write fails due to hardware quirks or an invalid value,
the return value of pcie_set_mps() is silently ignored, unlike similar calls
in pci_configure_mps() which log a warning or handle the failure.

> +
>  	if (!bridge || !pci_is_pcie(bridge))
>  		return;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911132145.211380-1-18255117159@163.com?part=1

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Hans Zhang]

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.

Reviewed-by: Manivannan Sadhasivam <mani@kernel.org>
Signed-off-by: Hans Zhang <18255117159@163.com>
---
 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 8559d132dcde..8c176f78953e 100644
--- a/drivers/pci/controller/dwc/pci-meson.c
+++ b/drivers/pci/controller/dwc/pci-meson.c
@@ -273,22 +273,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;
@@ -363,7 +347,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.34.1


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [Low] Unused macros PCIE_CAP_MAX_PAYLOAD_SIZE and MAX_PAYLOAD_SIZE left behind after removing the MPS configuration function.
- [Low] Misleading function name and warning message for MRRS calculation after removing MPS configuration.

Pre-existing issues:
- [High] 32-bit read-modify-write of PCI_EXP_DEVCTL inadvertently clears W1C error bits in PCI_EXP_DEVSTA.
--

commit 3ff79063d97cb0f265efaab65bab69ff2c512a3d
Author: Hans Zhang <18255117159@163.com>

PCI: dwc: Remove redundant MPS configuration

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.

> diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
> index 8559d132dcde8..8c176f78953e9 100644
> --- a/drivers/pci/controller/dwc/pci-meson.c
> +++ b/drivers/pci/controller/dwc/pci-meson.c
> @@ -273,22 +273,6 @@ static int meson_size_to_payload(struct meson_pcie *mp, int size)
>  	return fls(size) - 8;
>  }

[Severity: Low]
With the removal of the payload configuration function, this helper is now
exclusively called by meson_set_max_rd_req_size() to calculate the Max Read
Request Size (MRRS).

Does this function name and its internal log message:

	dev_warn(dev, "payload size %d, set to default 256\n", size);

need to be updated to reflect that it is calculating MRRS? Leaving it as-is
might cause confusion for future maintainers.

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

[Severity: Low]
Should the definition of PCIE_CAP_MAX_PAYLOAD_SIZE be removed as well? Since
it is no longer used after removing this function, it appears to be dead
code now.

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

[Severity: High]
This is a pre-existing issue, but does meson_set_max_rd_req_size()
inadvertently clear W1C error bits in PCI_EXP_DEVSTA?

This function performs a 32-bit read-modify-write on PCI_EXP_DEVCTL:

	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
	val &= ~PCI_EXP_DEVCTL_READRQ;
	dw_pcie_writel_dbi(pci, offset + PCI_EXP_DEVCTL, val);

Because this 32-bit access covers both the 16-bit PCI_EXP_DEVCTL and the
adjacent 16-bit PCI_EXP_DEVSTA, it fetches both. PCI_EXP_DEVSTA contains
multiple Write-1-to-Clear (W1C) error flags. Writing the entire 32-bit value
back means any W1C bit in DEVSTA read as '1' is written as '1', inadvertently
clearing the error status and masking hardware faults.

Should this be updated to use dw_pcie_readw_dbi() and dw_pcie_writew_dbi()
instead?

[ ... ]

> @@ -363,7 +347,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);

[Severity: Low]
Should the MAX_PAYLOAD_SIZE macro be removed as well? Since its usage is
removed here, leaving the macro behind leaves dead code.

>  	meson_set_max_rd_req_size(mp, MAX_READ_REQ_SIZE);
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911132145.211380-1-18255117159@163.com?part=2

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

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

[Hans Zhang]

On 9/11/26 21:34, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> New issues:
> - [Low] Unused macros PCIE_CAP_MAX_PAYLOAD_SIZE and MAX_PAYLOAD_SIZE left behind after removing the MPS configuration function.
> - [Low] Misleading function name and warning message for MRRS calculation after removing MPS configuration.
> 
> Pre-existing issues:
> - [High] 32-bit read-modify-write of PCI_EXP_DEVCTL inadvertently clears W1C error bits in PCI_EXP_DEVSTA.
> --
> 
> commit 3ff79063d97cb0f265efaab65bab69ff2c512a3d
> Author: Hans Zhang <18255117159@163.com>
> 
> PCI: dwc: Remove redundant MPS configuration
> 
> 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.
> 
>> diff --git a/drivers/pci/controller/dwc/pci-meson.c b/drivers/pci/controller/dwc/pci-meson.c
>> index 8559d132dcde8..8c176f78953e9 100644
>> --- a/drivers/pci/controller/dwc/pci-meson.c
>> +++ b/drivers/pci/controller/dwc/pci-meson.c
>> @@ -273,22 +273,6 @@ static int meson_size_to_payload(struct meson_pcie *mp, int size)
>>   	return fls(size) - 8;
>>   }
> 
> [Severity: Low]
> With the removal of the payload configuration function, this helper is now
> exclusively called by meson_set_max_rd_req_size() to calculate the Max Read
> Request Size (MRRS).

Hi,

Will add 0003 patch to rename function name.
s/meson_size_to_payload/meson_size_to_mrrs/

> 
> Does this function name and its internal log message:
> 
> 	dev_warn(dev, "payload size %d, set to default 256\n", size);
> 
> need to be updated to reflect that it is calculating MRRS? Leaving it as-is
> might cause confusion for future maintainers.
> 
>>   
>> -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);
> 
> [Severity: Low]
> Should the definition of PCIE_CAP_MAX_PAYLOAD_SIZE be removed as well? Since
> it is no longer used after removing this function, it appears to be dead
> code now.

The next version will remove:

#define PCIE_CAP_MAX_PAYLOAD_SIZE(x)	((x) << 5)
#define MAX_PAYLOAD_SIZE		256


Best regards,
Hans

> 
>> -	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;
> 
> [Severity: High]
> This is a pre-existing issue, but does meson_set_max_rd_req_size()
> inadvertently clear W1C error bits in PCI_EXP_DEVSTA?
> 
> This function performs a 32-bit read-modify-write on PCI_EXP_DEVCTL:
> 
> 	val = dw_pcie_readl_dbi(pci, offset + PCI_EXP_DEVCTL);
> 	val &= ~PCI_EXP_DEVCTL_READRQ;
> 	dw_pcie_writel_dbi(pci, offset + PCI_EXP_DEVCTL, val);
> 
> Because this 32-bit access covers both the 16-bit PCI_EXP_DEVCTL and the
> adjacent 16-bit PCI_EXP_DEVSTA, it fetches both. PCI_EXP_DEVSTA contains
> multiple Write-1-to-Clear (W1C) error flags. Writing the entire 32-bit value
> back means any W1C bit in DEVSTA read as '1' is written as '1', inadvertently
> clearing the error status and masking hardware faults.
> 
> Should this be updated to use dw_pcie_readw_dbi() and dw_pcie_writew_dbi()
> instead?
> 
> [ ... ]
> 
>> @@ -363,7 +347,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);
> 
> [Severity: Low]
> Should the MAX_PAYLOAD_SIZE macro be removed as well? Since its usage is
> removed here, leaving the macro behind leaves dead code.
> 
>>   	meson_set_max_rd_req_size(mp, MAX_READ_REQ_SIZE);
>>   
>>   	return 0;
> 


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic