[PATCH v4 0/2] Configure root port MPS during host probing

[PATCH v4 0/2] Configure root port MPS during host probing

[Hans Zhang]

1. PCI: Configure root port MPS during host probing
2. PCI: dwc: Remove redundant MPS configuration

---
Changes for v4:
- 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:
- The new split is patch 2/3 and 3/3.
- Modify the patch 1/3 according to Niklas' suggestion.

Changes for v2:
- 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                    | 72 +++++++++++++++-----------
 2 files changed, 41 insertions(+), 48 deletions(-)


base-commit: 01f95500a162fca88cefab9ed64ceded5afabc12
-- 
2.25.1

[PATCH v4 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. While downstream device
configuration is handled during bus enumeration, root port MPS values
inherited from firmware or hardware defaults might not utilize the full
capabilities supported by the controller hardware. This can result is
uboptimal data transfer efficiency across the PCIe hierarchy.

During host controller probing phase, when PCIe bus tuning is enabled,
the implementation now configures root port MPS settings to their
hardware-supported maximum values. By iterating through bridge devices
under the root bus and identifying PCIe root ports, each port's MPS is
set to 128 << pcie_mpss to match the device's maximum supported payload
size. The Max Read Request Size (MRRS) is subsequently adjusted through
existing companion logic to maintain compatibility with PCIe
specifications.

Explicit initialization at host probing stage ensures consistent PCIe
topology configuration before downstream devices perform their own MPS
negotiations. This proactive approach addresses platform-specific
requirements where controller drivers depend on properly initialized
root port settings, while maintaining backward compatibility through
PCIE_BUS_TUNE_OFF conditional checks. Hardware capabilities are fully
utilized without altering existing device negotiation behaviors.

Suggested-by: Niklas Cassel <cassel@kernel.org>
Signed-off-by: Hans Zhang <18255117159@163.com>
---
 drivers/pci/probe.c | 72 ++++++++++++++++++++++++++-------------------
 1 file changed, 41 insertions(+), 31 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 364fa2a514f8..1f67c03d170a 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2149,6 +2149,37 @@ int pci_setup_device(struct pci_dev *dev)
 	return 0;
 }
 
+static void pcie_write_mps(struct pci_dev *dev, int mps)
+{
+	int rc;
+
+	if (pcie_bus_config == PCIE_BUS_PERFORMANCE) {
+		mps = 128 << dev->pcie_mpss;
+
+		if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT &&
+		    dev->bus->self)
+
+			/*
+			 * For "Performance", the assumption is made that
+			 * downstream communication will never be larger than
+			 * the MRRS.  So, the MPS only needs to be configured
+			 * for the upstream communication.  This being the case,
+			 * walk from the top down and set the MPS of the child
+			 * to that of the parent bus.
+			 *
+			 * Configure the device MPS with the smaller of the
+			 * device MPSS or the bridge MPS (which is assumed to be
+			 * properly configured at this point to the largest
+			 * allowable MPS based on its parent bus).
+			 */
+			mps = min(mps, pcie_get_mps(dev->bus->self));
+	}
+
+	rc = pcie_set_mps(dev, mps);
+	if (rc)
+		pci_err(dev, "Failed attempting to set the MPS\n");
+}
+
 static void pci_configure_mps(struct pci_dev *dev)
 {
 	struct pci_dev *bridge = pci_upstream_bridge(dev);
@@ -2178,6 +2209,16 @@ static void pci_configure_mps(struct pci_dev *dev)
 		return;
 	}
 
+	/*
+	 * Unless MPS strategy is PCIE_BUS_TUNE_OFF (don't touch MPS at all),
+	 * start off by setting root ports' MPS to MPSS. Depending on the MPS
+	 * strategy, and the MPSS of the devices below the root port, the MPS
+	 * of the root port might get overridden later.
+	 */
+	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
+	    pcie_bus_config != PCIE_BUS_TUNE_OFF)
+		pcie_write_mps(dev, 128 << dev->pcie_mpss);
+
 	if (!bridge || !pci_is_pcie(bridge))
 		return;
 
@@ -2875,37 +2916,6 @@ static int pcie_find_smpss(struct pci_dev *dev, void *data)
 	return 0;
 }
 
-static void pcie_write_mps(struct pci_dev *dev, int mps)
-{
-	int rc;
-
-	if (pcie_bus_config == PCIE_BUS_PERFORMANCE) {
-		mps = 128 << dev->pcie_mpss;
-
-		if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT &&
-		    dev->bus->self)
-
-			/*
-			 * For "Performance", the assumption is made that
-			 * downstream communication will never be larger than
-			 * the MRRS.  So, the MPS only needs to be configured
-			 * for the upstream communication.  This being the case,
-			 * walk from the top down and set the MPS of the child
-			 * to that of the parent bus.
-			 *
-			 * Configure the device MPS with the smaller of the
-			 * device MPSS or the bridge MPS (which is assumed to be
-			 * properly configured at this point to the largest
-			 * allowable MPS based on its parent bus).
-			 */
-			mps = min(mps, pcie_get_mps(dev->bus->self));
-	}
-
-	rc = pcie_set_mps(dev, mps);
-	if (rc)
-		pci_err(dev, "Failed attempting to set the MPS\n");
-}
-
 static void pcie_write_mrrs(struct pci_dev *dev)
 {
 	int rc, mrrs;
-- 
2.25.1

[PATCH v4 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.

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

Re: [PATCH v4 1/2] PCI: Configure root port MPS during host probing

[Niklas Cassel]

On Sat, May 10, 2025 at 11:56:06PM +0800, Hans Zhang wrote:
> Current PCIe initialization logic may leave root ports operating 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 might not utilize the full
> capabilities supported by the controller hardware. This can result is
> uboptimal data transfer efficiency across the PCIe hierarchy.
> 
> During host controller probing phase, when PCIe bus tuning is enabled,
> the implementation now configures root port MPS settings to their
> hardware-supported maximum values. By iterating through bridge devices
> under the root bus and identifying PCIe root ports, each port's MPS is
> set to 128 << pcie_mpss to match the device's maximum supported payload
> size. The Max Read Request Size (MRRS) is subsequently adjusted through
> existing companion logic to maintain compatibility with PCIe
> specifications.
> 
> Explicit initialization at host probing stage ensures consistent PCIe
> topology configuration before downstream devices perform their own MPS
> negotiations. This proactive approach addresses platform-specific
> requirements where controller drivers depend on properly initialized
> root port settings, while maintaining backward compatibility through
> PCIE_BUS_TUNE_OFF conditional checks. Hardware capabilities are fully
> utilized without altering existing device negotiation behaviors.
> 
> Suggested-by: Niklas Cassel <cassel@kernel.org>
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---

Looks good to me, but since this I'm the one who suggested this specific
implementation, it would be good if someone else could review it as well.

Reviewed-by: Niklas Cassel <cassel@kernel.org>

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

[Niklas Cassel]

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

Looks good to me:
Reviewed-by: Niklas Cassel <cassel@kernel.org>

Re: [PATCH v4 1/2] PCI: Configure root port MPS during host probing

[Hans Zhang]

On 2025/5/13 16:04, Niklas Cassel wrote:
> On Sat, May 10, 2025 at 11:56:06PM +0800, Hans Zhang wrote:
>> Current PCIe initialization logic may leave root ports operating 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 might not utilize the full
>> capabilities supported by the controller hardware. This can result is
>> uboptimal data transfer efficiency across the PCIe hierarchy.
>>
>> During host controller probing phase, when PCIe bus tuning is enabled,
>> the implementation now configures root port MPS settings to their
>> hardware-supported maximum values. By iterating through bridge devices
>> under the root bus and identifying PCIe root ports, each port's MPS is
>> set to 128 << pcie_mpss to match the device's maximum supported payload
>> size. The Max Read Request Size (MRRS) is subsequently adjusted through
>> existing companion logic to maintain compatibility with PCIe
>> specifications.
>>
>> Explicit initialization at host probing stage ensures consistent PCIe
>> topology configuration before downstream devices perform their own MPS
>> negotiations. This proactive approach addresses platform-specific
>> requirements where controller drivers depend on properly initialized
>> root port settings, while maintaining backward compatibility through
>> PCIE_BUS_TUNE_OFF conditional checks. Hardware capabilities are fully
>> utilized without altering existing device negotiation behaviors.
>>
>> Suggested-by: Niklas Cassel <cassel@kernel.org>
>> Signed-off-by: Hans Zhang <18255117159@163.com>
>> ---
> 
> Looks good to me, but since this I'm the one who suggested this specific
> implementation, it would be good if someone else could review it as well.
> 
> Reviewed-by: Niklas Cassel <cassel@kernel.org>

Dear Niklas,

Thank you very much for your review and suggestions. Let's wait for 
others' opinions.

Best regards,
Hans

Re: [PATCH v4 1/2] PCI: Configure root port MPS during host probing

[Manivannan Sadhasivam]

On Sat, May 10, 2025 at 11:56:06PM +0800, Hans Zhang wrote:
> Current PCIe initialization logic may leave root ports operating 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 might not utilize the full
> capabilities supported by the controller hardware. This can result is
> uboptimal data transfer efficiency across the PCIe hierarchy.
> 
> During host controller probing phase, when PCIe bus tuning is enabled,
> the implementation now configures root port MPS settings to their
> hardware-supported maximum values. By iterating through bridge devices
> under the root bus and identifying PCIe root ports, each port's MPS is
> set to 128 << pcie_mpss to match the device's maximum supported payload
> size.

I don't think the above statement is accurate. This patch is not iterating
through the bridges and you cannot identify root ports using that. What this
patch does is, it checks whether the device is root port or not and if it is,
then it sets the MPS to MPSS (hw maximum) if PCIE_BUS_TUNE_OFF is not set.

> The Max Read Request Size (MRRS) is subsequently adjusted through
> existing companion logic to maintain compatibility with PCIe
> specifications.
> 
> Explicit initialization at host probing stage ensures consistent PCIe
> topology configuration before downstream devices perform their own MPS
> negotiations. This proactive approach addresses platform-specific
> requirements where controller drivers depend on properly initialized
> root port settings, while maintaining backward compatibility through
> PCIE_BUS_TUNE_OFF conditional checks. Hardware capabilities are fully
> utilized without altering existing device negotiation behaviors.
> 
> Suggested-by: Niklas Cassel <cassel@kernel.org>
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
>  drivers/pci/probe.c | 72 ++++++++++++++++++++++++++-------------------
>  1 file changed, 41 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 364fa2a514f8..1f67c03d170a 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2149,6 +2149,37 @@ int pci_setup_device(struct pci_dev *dev)
>  	return 0;
>  }
>  
> +static void pcie_write_mps(struct pci_dev *dev, int mps)
> +{
> +	int rc;
> +
> +	if (pcie_bus_config == PCIE_BUS_PERFORMANCE) {
> +		mps = 128 << dev->pcie_mpss;
> +
> +		if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT &&
> +		    dev->bus->self)
> +
> +			/*
> +			 * For "Performance", the assumption is made that
> +			 * downstream communication will never be larger than
> +			 * the MRRS.  So, the MPS only needs to be configured
> +			 * for the upstream communication.  This being the case,
> +			 * walk from the top down and set the MPS of the child
> +			 * to that of the parent bus.
> +			 *
> +			 * Configure the device MPS with the smaller of the
> +			 * device MPSS or the bridge MPS (which is assumed to be
> +			 * properly configured at this point to the largest
> +			 * allowable MPS based on its parent bus).
> +			 */
> +			mps = min(mps, pcie_get_mps(dev->bus->self));
> +	}
> +
> +	rc = pcie_set_mps(dev, mps);
> +	if (rc)
> +		pci_err(dev, "Failed attempting to set the MPS\n");
> +}
> +
>  static void pci_configure_mps(struct pci_dev *dev)
>  {
>  	struct pci_dev *bridge = pci_upstream_bridge(dev);
> @@ -2178,6 +2209,16 @@ static void pci_configure_mps(struct pci_dev *dev)
>  		return;
>  	}
>  
> +	/*
> +	 * Unless MPS strategy is PCIE_BUS_TUNE_OFF (don't touch MPS at all),
> +	 * start off by setting root ports' MPS to MPSS. Depending on the MPS
> +	 * strategy, and the MPSS of the devices below the root port, the MPS
> +	 * of the root port might get overridden later.
> +	 */
> +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
> +	    pcie_bus_config != PCIE_BUS_TUNE_OFF)
> +		pcie_write_mps(dev, 128 << dev->pcie_mpss);

I believe you can just use "pcie_set_mps(dev, 128 << dev->pcie_mpss)" directly
and avoid moving pcie_write_mps() around.

- Mani

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

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

[Manivannan Sadhasivam]

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
> 

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

Re: [PATCH v4 1/2] PCI: Configure root port MPS during host probing

[Niklas Cassel]

On Fri, Jun 13, 2025 at 12:08:31PM +0530, Manivannan Sadhasivam wrote:
> On Sat, May 10, 2025 at 11:56:06PM +0800, Hans Zhang wrote:
> > Current PCIe initialization logic may leave root ports operating 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 might not utilize the full
> > capabilities supported by the controller hardware. This can result is
> > uboptimal data transfer efficiency across the PCIe hierarchy.
> > 
> > During host controller probing phase, when PCIe bus tuning is enabled,
> > the implementation now configures root port MPS settings to their
> > hardware-supported maximum values. By iterating through bridge devices
> > under the root bus and identifying PCIe root ports, each port's MPS is
> > set to 128 << pcie_mpss to match the device's maximum supported payload
> > size.
> 
> I don't think the above statement is accurate. This patch is not iterating
> through the bridges and you cannot identify root ports using that. What this
> patch does is, it checks whether the device is root port or not and if it is,
> then it sets the MPS to MPSS (hw maximum) if PCIE_BUS_TUNE_OFF is not set.

Correct.
Later, when the bus is walked, if any downstream device does not support
the MPS value currently configured in the root port, pci_configure_mps()
will reduce the MPS in the root port to the max supported by the downstream
device.

So even we start off by setting MPS in the root port to the max supported
by the root port, it might get reduced later on.


> 
> > The Max Read Request Size (MRRS) is subsequently adjusted through
> > existing companion logic to maintain compatibility with PCIe
> > specifications.
> > 
> > Explicit initialization at host probing stage ensures consistent PCIe
> > topology configuration before downstream devices perform their own MPS
> > negotiations. This proactive approach addresses platform-specific
> > requirements where controller drivers depend on properly initialized
> > root port settings, while maintaining backward compatibility through
> > PCIE_BUS_TUNE_OFF conditional checks. Hardware capabilities are fully
> > utilized without altering existing device negotiation behaviors.
> > 
> > Suggested-by: Niklas Cassel <cassel@kernel.org>
> > Signed-off-by: Hans Zhang <18255117159@163.com>
> > ---
> >  drivers/pci/probe.c | 72 ++++++++++++++++++++++++++-------------------
> >  1 file changed, 41 insertions(+), 31 deletions(-)
> > 
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 364fa2a514f8..1f67c03d170a 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -2149,6 +2149,37 @@ int pci_setup_device(struct pci_dev *dev)
> >  	return 0;
> >  }
> >  
> > +static void pcie_write_mps(struct pci_dev *dev, int mps)
> > +{
> > +	int rc;
> > +
> > +	if (pcie_bus_config == PCIE_BUS_PERFORMANCE) {
> > +		mps = 128 << dev->pcie_mpss;
> > +
> > +		if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT &&
> > +		    dev->bus->self)
> > +
> > +			/*
> > +			 * For "Performance", the assumption is made that
> > +			 * downstream communication will never be larger than
> > +			 * the MRRS.  So, the MPS only needs to be configured
> > +			 * for the upstream communication.  This being the case,
> > +			 * walk from the top down and set the MPS of the child
> > +			 * to that of the parent bus.
> > +			 *
> > +			 * Configure the device MPS with the smaller of the
> > +			 * device MPSS or the bridge MPS (which is assumed to be
> > +			 * properly configured at this point to the largest
> > +			 * allowable MPS based on its parent bus).
> > +			 */
> > +			mps = min(mps, pcie_get_mps(dev->bus->self));
> > +	}
> > +
> > +	rc = pcie_set_mps(dev, mps);
> > +	if (rc)
> > +		pci_err(dev, "Failed attempting to set the MPS\n");
> > +}
> > +
> >  static void pci_configure_mps(struct pci_dev *dev)
> >  {
> >  	struct pci_dev *bridge = pci_upstream_bridge(dev);
> > @@ -2178,6 +2209,16 @@ static void pci_configure_mps(struct pci_dev *dev)
> >  		return;
> >  	}
> >  
> > +	/*
> > +	 * Unless MPS strategy is PCIE_BUS_TUNE_OFF (don't touch MPS at all),
> > +	 * start off by setting root ports' MPS to MPSS. Depending on the MPS
> > +	 * strategy, and the MPSS of the devices below the root port, the MPS
> > +	 * of the root port might get overridden later.
> > +	 */
> > +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
> > +	    pcie_bus_config != PCIE_BUS_TUNE_OFF)
> > +		pcie_write_mps(dev, 128 << dev->pcie_mpss);
> 
> I believe you can just use "pcie_set_mps(dev, 128 << dev->pcie_mpss)" directly
> and avoid moving pcie_write_mps() around.

+1


Kind regards,
Niklas

Re: [PATCH v4 1/2] PCI: Configure root port MPS during host probing

[Hans Zhang]

On 2025/6/13 19:52, Niklas Cassel wrote:
> On Fri, Jun 13, 2025 at 12:08:31PM +0530, Manivannan Sadhasivam wrote:
>> On Sat, May 10, 2025 at 11:56:06PM +0800, Hans Zhang wrote:
>>> Current PCIe initialization logic may leave root ports operating 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 might not utilize the full
>>> capabilities supported by the controller hardware. This can result is
>>> uboptimal data transfer efficiency across the PCIe hierarchy.
>>>
>>> During host controller probing phase, when PCIe bus tuning is enabled,
>>> the implementation now configures root port MPS settings to their
>>> hardware-supported maximum values. By iterating through bridge devices
>>> under the root bus and identifying PCIe root ports, each port's MPS is
>>> set to 128 << pcie_mpss to match the device's maximum supported payload
>>> size.
>>
>> I don't think the above statement is accurate. This patch is not iterating
>> through the bridges and you cannot identify root ports using that. What this
>> patch does is, it checks whether the device is root port or not and if it is,
>> then it sets the MPS to MPSS (hw maximum) if PCIE_BUS_TUNE_OFF is not set.
> 
> Correct.
> Later, when the bus is walked, if any downstream device does not support
> the MPS value currently configured in the root port, pci_configure_mps()
> will reduce the MPS in the root port to the max supported by the downstream
> device.
> 
> So even we start off by setting MPS in the root port to the max supported
> by the root port, it might get reduced later on.
> 
> 

Dear Mani and Niklas,

Is it okay to modify the commit message as follows? The last paragraph 
remains unchanged.



Current PCIe initialization logic may leave root ports operating 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 might not utilize the full
capabilities supported by the controller hardware. This can result in
suboptimal data transfer efficiency across the PCIe hierarchy.

During host controller probing phase, when PCIe bus tuning is enabled,
the implementation now configures root port MPS settings to their
hardware-supported maximum values. Specifically, when configuring the MPS
for a PCIe device, if the device is a root port and the bus tuning is not
disabled (PCIE_BUS_TUNE_OFF), the MPS is set to 128 << dev->pcie_mpss to
match the device's maximum supported payload size. The Max Read Request
Size (MRRS) is subsequently adjusted through existing companion logic to
maintain compatibility with PCIe specifications.

Note that this initial setting of the root port MPS to the maximum might
be reduced later during the enumeration of downstream devices if any of
those devices do not support the maximum MPS of the root port.

>>
>>> The Max Read Request Size (MRRS) is subsequently adjusted through
>>> existing companion logic to maintain compatibility with PCIe
>>> specifications.
>>>
>>> Explicit initialization at host probing stage ensures consistent PCIe
>>> topology configuration before downstream devices perform their own MPS
>>> negotiations. This proactive approach addresses platform-specific
>>> requirements where controller drivers depend on properly initialized
>>> root port settings, while maintaining backward compatibility through
>>> PCIE_BUS_TUNE_OFF conditional checks. Hardware capabilities are fully
>>> utilized without altering existing device negotiation behaviors.
>>>
>>> Suggested-by: Niklas Cassel <cassel@kernel.org>
>>> Signed-off-by: Hans Zhang <18255117159@163.com>
>>> ---
>>>   drivers/pci/probe.c | 72 ++++++++++++++++++++++++++-------------------
>>>   1 file changed, 41 insertions(+), 31 deletions(-)
>>>
>>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>>> index 364fa2a514f8..1f67c03d170a 100644
>>> --- a/drivers/pci/probe.c
>>> +++ b/drivers/pci/probe.c
>>> @@ -2149,6 +2149,37 @@ int pci_setup_device(struct pci_dev *dev)
>>>   	return 0;
>>>   }
>>>   
>>> +static void pcie_write_mps(struct pci_dev *dev, int mps)
>>> +{
>>> +	int rc;
>>> +
>>> +	if (pcie_bus_config == PCIE_BUS_PERFORMANCE) {
>>> +		mps = 128 << dev->pcie_mpss;
>>> +
>>> +		if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT &&
>>> +		    dev->bus->self)
>>> +
>>> +			/*
>>> +			 * For "Performance", the assumption is made that
>>> +			 * downstream communication will never be larger than
>>> +			 * the MRRS.  So, the MPS only needs to be configured
>>> +			 * for the upstream communication.  This being the case,
>>> +			 * walk from the top down and set the MPS of the child
>>> +			 * to that of the parent bus.
>>> +			 *
>>> +			 * Configure the device MPS with the smaller of the
>>> +			 * device MPSS or the bridge MPS (which is assumed to be
>>> +			 * properly configured at this point to the largest
>>> +			 * allowable MPS based on its parent bus).
>>> +			 */
>>> +			mps = min(mps, pcie_get_mps(dev->bus->self));
>>> +	}
>>> +
>>> +	rc = pcie_set_mps(dev, mps);
>>> +	if (rc)
>>> +		pci_err(dev, "Failed attempting to set the MPS\n");
>>> +}
>>> +
>>>   static void pci_configure_mps(struct pci_dev *dev)
>>>   {
>>>   	struct pci_dev *bridge = pci_upstream_bridge(dev);
>>> @@ -2178,6 +2209,16 @@ static void pci_configure_mps(struct pci_dev *dev)
>>>   		return;
>>>   	}
>>>   
>>> +	/*
>>> +	 * Unless MPS strategy is PCIE_BUS_TUNE_OFF (don't touch MPS at all),
>>> +	 * start off by setting root ports' MPS to MPSS. Depending on the MPS
>>> +	 * strategy, and the MPSS of the devices below the root port, the MPS
>>> +	 * of the root port might get overridden later.
>>> +	 */
>>> +	if (pci_pcie_type(dev) == PCI_EXP_TYPE_ROOT_PORT &&
>>> +	    pcie_bus_config != PCIE_BUS_TUNE_OFF)
>>> +		pcie_write_mps(dev, 128 << dev->pcie_mpss);
>>
>> I believe you can just use "pcie_set_mps(dev, 128 << dev->pcie_mpss)" directly
>> and avoid moving pcie_write_mps() around.
> 
> +1
> 

Mani and Niklas,

Thank you very much for your review and suggestions. Will change.

Best regards,
Hans


> 
> Kind regards,
> Niklas

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

[Hans Zhang]

On 2025/6/13 14:54, Manivannan Sadhasivam wrote:
> 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.
> 

Dear Mani,

Thank you for your reminder. I found two friends of Amlogic from the 
submission records and copied the email to them. And ask them to help 
test these two patches.

But I don't know if they are still employed, or do you know anyone to 
help with the test?





Dear yue.wang and hanjie.lin,

Could you help test these two patches? I don't know if it affects the 
normal function of your Root Port. If it works properly, please let me 
know. Thank you very much.

Best regards,
Hans



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

Re: [PATCH v4 1/2] PCI: Configure root port MPS during host probing

[Manivannan Sadhasivam]

On Fri, Jun 13, 2025 at 11:31:01PM +0800, Hans Zhang wrote:
> 
> 
> On 2025/6/13 19:52, Niklas Cassel wrote:
> > On Fri, Jun 13, 2025 at 12:08:31PM +0530, Manivannan Sadhasivam wrote:
> > > On Sat, May 10, 2025 at 11:56:06PM +0800, Hans Zhang wrote:
> > > > Current PCIe initialization logic may leave root ports operating 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 might not utilize the full
> > > > capabilities supported by the controller hardware. This can result is
> > > > uboptimal data transfer efficiency across the PCIe hierarchy.
> > > > 
> > > > During host controller probing phase, when PCIe bus tuning is enabled,
> > > > the implementation now configures root port MPS settings to their
> > > > hardware-supported maximum values. By iterating through bridge devices
> > > > under the root bus and identifying PCIe root ports, each port's MPS is
> > > > set to 128 << pcie_mpss to match the device's maximum supported payload
> > > > size.
> > > 
> > > I don't think the above statement is accurate. This patch is not iterating
> > > through the bridges and you cannot identify root ports using that. What this
> > > patch does is, it checks whether the device is root port or not and if it is,
> > > then it sets the MPS to MPSS (hw maximum) if PCIE_BUS_TUNE_OFF is not set.
> > 
> > Correct.
> > Later, when the bus is walked, if any downstream device does not support
> > the MPS value currently configured in the root port, pci_configure_mps()
> > will reduce the MPS in the root port to the max supported by the downstream
> > device.
> > 
> > So even we start off by setting MPS in the root port to the max supported
> > by the root port, it might get reduced later on.
> > 
> > 
> 
> Dear Mani and Niklas,
> 
> Is it okay to modify the commit message as follows? The last paragraph
> remains unchanged.
> 
> 
> 
> Current PCIe initialization logic may leave root ports operating 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 might not utilize the full
> capabilities supported by the controller hardware. This can result in
> suboptimal data transfer efficiency across the PCIe hierarchy.
> 
> During host controller probing phase, when PCIe bus tuning is enabled,
> the implementation now configures root port MPS settings to their
> hardware-supported maximum values. Specifically, when configuring the MPS
> for a PCIe device, if the device is a root port and the bus tuning is not
> disabled (PCIE_BUS_TUNE_OFF), the MPS is set to 128 << dev->pcie_mpss to
> match the device's maximum supported payload size. The Max Read Request

s/device/Root Port

> Size (MRRS) is subsequently adjusted through existing companion logic to
> maintain compatibility with PCIe specifications.
> 
> Note that this initial setting of the root port MPS to the maximum might
> be reduced later during the enumeration of downstream devices if any of
> those devices do not support the maximum MPS of the root port.
> 

Rest LGTM!

- Mani

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