[PATCH v3 0/3] Configure root port MPS during host probing

[PATCH v3 0/3] 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
3. PCI: aardvark: Remove redundant MPS configuration

---
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 (3):
  PCI: Configure root port MPS during host probing
  PCI: dwc: Remove redundant MPS configuration
  PCI: aardvark: Remove redundant MPS configuration

 drivers/pci/controller/dwc/pci-meson.c | 17 -------
 drivers/pci/controller/pci-aardvark.c  |  2 -
 drivers/pci/probe.c                    | 66 ++++++++++++++------------
 3 files changed, 35 insertions(+), 50 deletions(-)


base-commit: 01f95500a162fca88cefab9ed64ceded5afabc12
-- 
2.25.1

[PATCH v3 1/3] 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.

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 | 66 ++++++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 31 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 364fa2a514f8..365d9a7dd37f 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,10 @@ static void pci_configure_mps(struct pci_dev *dev)
 		return;
 	}
 
+	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 +2910,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

Re: [PATCH v3 1/3] PCI: Configure root port MPS during host probing

[Niklas Cassel]

On Wed, May 07, 2025 at 01:34:37AM +0800, Hans Zhang wrote:

(snip)

>  static void pci_configure_mps(struct pci_dev *dev)
>  {
>  	struct pci_dev *bridge = pci_upstream_bridge(dev);
> @@ -2178,6 +2209,10 @@ static void pci_configure_mps(struct pci_dev *dev)
>  		return;
>  	}

We should probably add a comment explaining why we are doing this here.

Perhaps something like:

/*
 * 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 overriden 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;
>  


Kind regards,
Niklas

Re: [PATCH v3 1/3] PCI: Configure root port MPS during host probing

[Hans Zhang]

On 2025/5/7 15:38, Niklas Cassel wrote:
> On Wed, May 07, 2025 at 01:34:37AM +0800, Hans Zhang wrote:
> 
> (snip)
> 
>>   static void pci_configure_mps(struct pci_dev *dev)
>>   {
>>   	struct pci_dev *bridge = pci_upstream_bridge(dev);
>> @@ -2178,6 +2209,10 @@ static void pci_configure_mps(struct pci_dev *dev)
>>   		return;
>>   	}
> 
> We should probably add a comment explaining why we are doing this here.
> 
> Perhaps something like:
> 
> /*
>   * 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 overriden later.
>   */
> 
> 

Dear Niklas,

Thank you very much for your reply and suggestions.
It will be added in the next version.

Best regards,
Hans

>> +	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;
>>   
> 
> 
> Kind regards,
> Niklas

[PATCH v3 2/3] 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

[PATCH v3 3/3] PCI: aardvark: Remove redundant MPS configuration

[Hans Zhang]

The Aardvark PCIe controller enforces a fixed 512B payload size via
PCI_EXP_DEVCTL_PAYLOAD_512B, overriding hardware capabilities and PCIe
core negotiations.

Remove explicit MPS overrides (PCI_EXP_DEVCTL_PAYLOAD and
PCI_EXP_DEVCTL_PAYLOAD_512B). MPS is now determined by the PCI core
during device initialization, leveraging root port configurations and
device-specific capabilities.

Aligning Aardvark with the unified MPS framework ensures consistency,
avoids artificial constraints, and allows the hardware to operate at
its maximum supported payload size while adhering to PCIe specifications.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
 drivers/pci/controller/pci-aardvark.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index a29796cce420..d8852892994a 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -549,9 +549,7 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	reg = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
 	reg &= ~PCI_EXP_DEVCTL_RELAX_EN;
 	reg &= ~PCI_EXP_DEVCTL_NOSNOOP_EN;
-	reg &= ~PCI_EXP_DEVCTL_PAYLOAD;
 	reg &= ~PCI_EXP_DEVCTL_READRQ;
-	reg |= PCI_EXP_DEVCTL_PAYLOAD_512B;
 	reg |= PCI_EXP_DEVCTL_READRQ_512B;
 	advk_writel(pcie, reg, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
 
-- 
2.25.1

Re: [PATCH v3 3/3] PCI: aardvark: Remove redundant MPS configuration

[Pali Rohár]

On Wednesday 07 May 2025 01:34:39 Hans Zhang wrote:
> The Aardvark PCIe controller enforces a fixed 512B payload size via
> PCI_EXP_DEVCTL_PAYLOAD_512B, overriding hardware capabilities and PCIe
> core negotiations.
> 
> Remove explicit MPS overrides (PCI_EXP_DEVCTL_PAYLOAD and
> PCI_EXP_DEVCTL_PAYLOAD_512B). MPS is now determined by the PCI core
> during device initialization, leveraging root port configurations and
> device-specific capabilities.
> 
> Aligning Aardvark with the unified MPS framework ensures consistency,
> avoids artificial constraints, and allows the hardware to operate at
> its maximum supported payload size while adhering to PCIe specifications.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
>  drivers/pci/controller/pci-aardvark.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index a29796cce420..d8852892994a 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -549,9 +549,7 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
>  	reg = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
>  	reg &= ~PCI_EXP_DEVCTL_RELAX_EN;
>  	reg &= ~PCI_EXP_DEVCTL_NOSNOOP_EN;
> -	reg &= ~PCI_EXP_DEVCTL_PAYLOAD;
>  	reg &= ~PCI_EXP_DEVCTL_READRQ;
> -	reg |= PCI_EXP_DEVCTL_PAYLOAD_512B;
>  	reg |= PCI_EXP_DEVCTL_READRQ_512B;
>  	advk_writel(pcie, reg, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
>  
> -- 
> 2.25.1
> 

Please do not remove this code. It is required part of the
initialization of the aardvark PCI controller at the specific phase,
as defined in the Armada 3700 Functional Specification.

There were reported more issues with those Armada PCIe controllers for
which were already sent patches to mailing list in last 5 years. But
unfortunately not all fixes were taken / applied yet.

Re: [PATCH v3 3/3] PCI: aardvark: Remove redundant MPS configuration

[Hans Zhang]

On 2025/5/7 01:41, Pali Rohár wrote:
> On Wednesday 07 May 2025 01:34:39 Hans Zhang wrote:
>> The Aardvark PCIe controller enforces a fixed 512B payload size via
>> PCI_EXP_DEVCTL_PAYLOAD_512B, overriding hardware capabilities and PCIe
>> core negotiations.
>>
>> Remove explicit MPS overrides (PCI_EXP_DEVCTL_PAYLOAD and
>> PCI_EXP_DEVCTL_PAYLOAD_512B). MPS is now determined by the PCI core
>> during device initialization, leveraging root port configurations and
>> device-specific capabilities.
>>
>> Aligning Aardvark with the unified MPS framework ensures consistency,
>> avoids artificial constraints, and allows the hardware to operate at
>> its maximum supported payload size while adhering to PCIe specifications.
>>
>> Signed-off-by: Hans Zhang <18255117159@163.com>
>> ---
>>   drivers/pci/controller/pci-aardvark.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
>> index a29796cce420..d8852892994a 100644
>> --- a/drivers/pci/controller/pci-aardvark.c
>> +++ b/drivers/pci/controller/pci-aardvark.c
>> @@ -549,9 +549,7 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
>>   	reg = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
>>   	reg &= ~PCI_EXP_DEVCTL_RELAX_EN;
>>   	reg &= ~PCI_EXP_DEVCTL_NOSNOOP_EN;
>> -	reg &= ~PCI_EXP_DEVCTL_PAYLOAD;
>>   	reg &= ~PCI_EXP_DEVCTL_READRQ;
>> -	reg |= PCI_EXP_DEVCTL_PAYLOAD_512B;
>>   	reg |= PCI_EXP_DEVCTL_READRQ_512B;
>>   	advk_writel(pcie, reg, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
>>   
>> -- 
>> 2.25.1
>>
> 
> Please do not remove this code. It is required part of the
> initialization of the aardvark PCI controller at the specific phase,
> as defined in the Armada 3700 Functional Specification.
> 
> There were reported more issues with those Armada PCIe controllers for
> which were already sent patches to mailing list in last 5 years. But
> unfortunately not all fixes were taken / applied yet.

Hi Pali,

I replied to you in version v2.

Is the maximum MPS supported by Armada 3700 512 bytes? What are the 
default values of DevCap.MPS and DevCtl.MPS?

Because the default value of DevCtl.MPS is not 512 bytes, it needs to be 
configured here, right?

If it's my guess, RK3588 also has the same requirements as you, just 
like the first patch I submitted.

Please take a look at the communication history:
https://patchwork.kernel.org/project/linux-pci/patch/20250416151926.140202-1-18255117159@163.com/

Please test it using patch 1/3 of this series. If there are any 
problems, please let me know.


Best regards,
Hans

Re: [PATCH v3 3/3] PCI: aardvark: Remove redundant MPS configuration

[Hans Zhang]

On 2025/5/7 23:03, Hans Zhang wrote:
> 
> 
> On 2025/5/7 01:41, Pali Rohár wrote:
>> On Wednesday 07 May 2025 01:34:39 Hans Zhang wrote:
>>> The Aardvark PCIe controller enforces a fixed 512B payload size via
>>> PCI_EXP_DEVCTL_PAYLOAD_512B, overriding hardware capabilities and PCIe
>>> core negotiations.
>>>
>>> Remove explicit MPS overrides (PCI_EXP_DEVCTL_PAYLOAD and
>>> PCI_EXP_DEVCTL_PAYLOAD_512B). MPS is now determined by the PCI core
>>> during device initialization, leveraging root port configurations and
>>> device-specific capabilities.
>>>
>>> Aligning Aardvark with the unified MPS framework ensures consistency,
>>> avoids artificial constraints, and allows the hardware to operate at
>>> its maximum supported payload size while adhering to PCIe 
>>> specifications.
>>>
>>> Signed-off-by: Hans Zhang <18255117159@163.com>
>>> ---
>>>   drivers/pci/controller/pci-aardvark.c | 2 --
>>>   1 file changed, 2 deletions(-)
>>>
>>> diff --git a/drivers/pci/controller/pci-aardvark.c 
>>> b/drivers/pci/controller/pci-aardvark.c
>>> index a29796cce420..d8852892994a 100644
>>> --- a/drivers/pci/controller/pci-aardvark.c
>>> +++ b/drivers/pci/controller/pci-aardvark.c
>>> @@ -549,9 +549,7 @@ static void advk_pcie_setup_hw(struct advk_pcie 
>>> *pcie)
>>>       reg = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
>>>       reg &= ~PCI_EXP_DEVCTL_RELAX_EN;
>>>       reg &= ~PCI_EXP_DEVCTL_NOSNOOP_EN;
>>> -    reg &= ~PCI_EXP_DEVCTL_PAYLOAD;
>>>       reg &= ~PCI_EXP_DEVCTL_READRQ;
>>> -    reg |= PCI_EXP_DEVCTL_PAYLOAD_512B;
>>>       reg |= PCI_EXP_DEVCTL_READRQ_512B;
>>>       advk_writel(pcie, reg, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
>>> -- 
>>> 2.25.1
>>>
>>
>> Please do not remove this code. It is required part of the
>> initialization of the aardvark PCI controller at the specific phase,
>> as defined in the Armada 3700 Functional Specification.
>>
>> There were reported more issues with those Armada PCIe controllers for
>> which were already sent patches to mailing list in last 5 years. But
>> unfortunately not all fixes were taken / applied yet.
> 
> Hi Pali,
> 
> I replied to you in version v2.
> 
> Is the maximum MPS supported by Armada 3700 512 bytes? What are the 
> default values of DevCap.MPS and DevCtl.MPS?
> 
> Because the default value of DevCtl.MPS is not 512 bytes, it needs to be 
> configured here, right?
> 
> If it's my guess, RK3588 also has the same requirements as you, just 
> like the first patch I submitted.
> 
> Please take a look at the communication history:
> https://patchwork.kernel.org/project/linux-pci/patch/20250416151926.140202-1-18255117159@163.com/
And this:
https://patchwork.kernel.org/project/linux-pci/patch/20250425095708.32662-2-18255117159@163.com/

> 
> Please test it using patch 1/3 of this series. If there are any 
> problems, please let me know.
> 
> 
> Best regards,
> Hans

Re: [PATCH v3 3/3] PCI: aardvark: Remove redundant MPS configuration

[Pali Rohár]

On Wednesday 07 May 2025 23:06:51 Hans Zhang wrote:
> On 2025/5/7 23:03, Hans Zhang wrote:
> > On 2025/5/7 01:41, Pali Rohár wrote:
> > > On Wednesday 07 May 2025 01:34:39 Hans Zhang wrote:
> > > > The Aardvark PCIe controller enforces a fixed 512B payload size via
> > > > PCI_EXP_DEVCTL_PAYLOAD_512B, overriding hardware capabilities and PCIe
> > > > core negotiations.
> > > > 
> > > > Remove explicit MPS overrides (PCI_EXP_DEVCTL_PAYLOAD and
> > > > PCI_EXP_DEVCTL_PAYLOAD_512B). MPS is now determined by the PCI core
> > > > during device initialization, leveraging root port configurations and
> > > > device-specific capabilities.
> > > > 
> > > > Aligning Aardvark with the unified MPS framework ensures consistency,
> > > > avoids artificial constraints, and allows the hardware to operate at
> > > > its maximum supported payload size while adhering to PCIe
> > > > specifications.
> > > > 
> > > > Signed-off-by: Hans Zhang <18255117159@163.com>
> > > > ---
> > > >   drivers/pci/controller/pci-aardvark.c | 2 --
> > > >   1 file changed, 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/controller/pci-aardvark.c
> > > > b/drivers/pci/controller/pci-aardvark.c
> > > > index a29796cce420..d8852892994a 100644
> > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > @@ -549,9 +549,7 @@ static void advk_pcie_setup_hw(struct
> > > > advk_pcie *pcie)
> > > >       reg = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
> > > >       reg &= ~PCI_EXP_DEVCTL_RELAX_EN;
> > > >       reg &= ~PCI_EXP_DEVCTL_NOSNOOP_EN;
> > > > -    reg &= ~PCI_EXP_DEVCTL_PAYLOAD;
> > > >       reg &= ~PCI_EXP_DEVCTL_READRQ;
> > > > -    reg |= PCI_EXP_DEVCTL_PAYLOAD_512B;
> > > >       reg |= PCI_EXP_DEVCTL_READRQ_512B;
> > > >       advk_writel(pcie, reg, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
> > > > -- 
> > > > 2.25.1
> > > > 
> > > 
> > > Please do not remove this code. It is required part of the
> > > initialization of the aardvark PCI controller at the specific phase,
> > > as defined in the Armada 3700 Functional Specification.
> > > 
> > > There were reported more issues with those Armada PCIe controllers for
> > > which were already sent patches to mailing list in last 5 years. But
> > > unfortunately not all fixes were taken / applied yet.
> > 
> > Hi Pali,
> > 
> > I replied to you in version v2.
> > 
> > Is the maximum MPS supported by Armada 3700 512 bytes?

IIRC yes, 512-byte mode is supported. And I think in past I was testing
some PCIe endpoint card which required 512-byte long payload and did not
worked in 256-byte long mode (not sure if the card was not able to split
transaction or something other was broken, it is quite longer time).

> > What are the default values of DevCap.MPS and DevCtl.MPS?

Do you mean values in the PCI-to-PCI bridge device of PCIe Root Port
type?

Aardvark controller does not have the real HW PCI-to-PCI bridge device.
There is only in-kernel emulation drivers/pci/pci-bridge-emul.c which
create fake kernel PCI device in the hierarchy to make kernel and
userspace happy. Yes, this is deviation from the PCIe standard but well,
buggy HW is also HW.

And same applies for the pci-mvebu.c driver for older Marvell PCIe HW.

> > Because the default value of DevCtl.MPS is not 512 bytes, it needs to be
> > configured here, right?
> > 
> > If it's my guess, RK3588 also has the same requirements as you, just
> > like the first patch I submitted.
> > 
> > Please take a look at the communication history:
> > https://patchwork.kernel.org/project/linux-pci/patch/20250416151926.140202-1-18255117159@163.com/
> And this:
> https://patchwork.kernel.org/project/linux-pci/patch/20250425095708.32662-2-18255117159@163.com/

These changes are referring the to root ports PCI devices, which are not
applicable for aardvark PCIe controller.

> > 
> > Please test it using patch 1/3 of this series. If there are any
> > problems, please let me know.
> > 
> > 
> > Best regards,
> > Hans
> 

Sorry, but I stopped doing any testing of the aardvark driver with the
mainline kernel after PCI maintainers stopped taking fixes for the
driver and stopped responding.

I'm not going to debug same issues again, which I have analyzed,
prepared fixes, sent patches and see no progress there.

Seems that there is a status quo, and I'm not going to change it.

Re: [PATCH v3 3/3] PCI: aardvark: Remove redundant MPS configuration

[Hans Zhang]

On 2025/5/8 00:36, Pali Rohár wrote:
> On Wednesday 07 May 2025 23:06:51 Hans Zhang wrote:
>> On 2025/5/7 23:03, Hans Zhang wrote:
>>> On 2025/5/7 01:41, Pali Rohár wrote:
>>>> On Wednesday 07 May 2025 01:34:39 Hans Zhang wrote:
>>>>> The Aardvark PCIe controller enforces a fixed 512B payload size via
>>>>> PCI_EXP_DEVCTL_PAYLOAD_512B, overriding hardware capabilities and PCIe
>>>>> core negotiations.
>>>>>
>>>>> Remove explicit MPS overrides (PCI_EXP_DEVCTL_PAYLOAD and
>>>>> PCI_EXP_DEVCTL_PAYLOAD_512B). MPS is now determined by the PCI core
>>>>> during device initialization, leveraging root port configurations and
>>>>> device-specific capabilities.
>>>>>
>>>>> Aligning Aardvark with the unified MPS framework ensures consistency,
>>>>> avoids artificial constraints, and allows the hardware to operate at
>>>>> its maximum supported payload size while adhering to PCIe
>>>>> specifications.
>>>>>
>>>>> Signed-off-by: Hans Zhang <18255117159@163.com>
>>>>> ---
>>>>>    drivers/pci/controller/pci-aardvark.c | 2 --
>>>>>    1 file changed, 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pci/controller/pci-aardvark.c
>>>>> b/drivers/pci/controller/pci-aardvark.c
>>>>> index a29796cce420..d8852892994a 100644
>>>>> --- a/drivers/pci/controller/pci-aardvark.c
>>>>> +++ b/drivers/pci/controller/pci-aardvark.c
>>>>> @@ -549,9 +549,7 @@ static void advk_pcie_setup_hw(struct
>>>>> advk_pcie *pcie)
>>>>>        reg = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
>>>>>        reg &= ~PCI_EXP_DEVCTL_RELAX_EN;
>>>>>        reg &= ~PCI_EXP_DEVCTL_NOSNOOP_EN;
>>>>> -    reg &= ~PCI_EXP_DEVCTL_PAYLOAD;
>>>>>        reg &= ~PCI_EXP_DEVCTL_READRQ;
>>>>> -    reg |= PCI_EXP_DEVCTL_PAYLOAD_512B;
>>>>>        reg |= PCI_EXP_DEVCTL_READRQ_512B;
>>>>>        advk_writel(pcie, reg, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
>>>>> -- 
>>>>> 2.25.1
>>>>>
>>>>
>>>> Please do not remove this code. It is required part of the
>>>> initialization of the aardvark PCI controller at the specific phase,
>>>> as defined in the Armada 3700 Functional Specification.
>>>>
>>>> There were reported more issues with those Armada PCIe controllers for
>>>> which were already sent patches to mailing list in last 5 years. But
>>>> unfortunately not all fixes were taken / applied yet.
>>>
>>> Hi Pali,
>>>
>>> I replied to you in version v2.
>>>
>>> Is the maximum MPS supported by Armada 3700 512 bytes?
> 
> IIRC yes, 512-byte mode is supported. And I think in past I was testing
> some PCIe endpoint card which required 512-byte long payload and did not
> worked in 256-byte long mode (not sure if the card was not able to split
> transaction or something other was broken, it is quite longer time).
> 
>>> What are the default values of DevCap.MPS and DevCtl.MPS?
> 
> Do you mean values in the PCI-to-PCI bridge device of PCIe Root Port
> type?
> 
> Aardvark controller does not have the real HW PCI-to-PCI bridge device.
> There is only in-kernel emulation drivers/pci/pci-bridge-emul.c which
> create fake kernel PCI device in the hierarchy to make kernel and
> userspace happy. Yes, this is deviation from the PCIe standard but well,
> buggy HW is also HW.
> 
> And same applies for the pci-mvebu.c driver for older Marvell PCIe HW.
> 
>>> Because the default value of DevCtl.MPS is not 512 bytes, it needs to be
>>> configured here, right?
>>>
>>> If it's my guess, RK3588 also has the same requirements as you, just
>>> like the first patch I submitted.
>>>
>>> Please take a look at the communication history:
>>> https://patchwork.kernel.org/project/linux-pci/patch/20250416151926.140202-1-18255117159@163.com/
>> And this:
>> https://patchwork.kernel.org/project/linux-pci/patch/20250425095708.32662-2-18255117159@163.com/
> 
> These changes are referring the to root ports PCI devices, which are not
> applicable for aardvark PCIe controller.
> 
>>>
>>> Please test it using patch 1/3 of this series. If there are any
>>> problems, please let me know.
>>>
>>>
>>> Best regards,
>>> Hans
>>
> 
> Sorry, but I stopped doing any testing of the aardvark driver with the
> mainline kernel after PCI maintainers stopped taking fixes for the
> driver and stopped responding.
> 
> I'm not going to debug same issues again, which I have analyzed,
> prepared fixes, sent patches and see no progress there.
> 
> Seems that there is a status quo, and I'm not going to change it.

Dear Niklas,

Do you have any opinion on Pali's reply? Should patch 3/3 be discarded?

Beat regards,
Hans

Re: [PATCH v3 3/3] PCI: aardvark: Remove redundant MPS configuration

[Niklas Cassel]

Hello Hans,

On Thu, May 08, 2025 at 12:47:12AM +0800, Hans Zhang wrote:
> On 2025/5/8 00:36, Pali Rohár wrote:
> > 
> > Sorry, but I stopped doing any testing of the aardvark driver with the
> > mainline kernel after PCI maintainers stopped taking fixes for the
> > driver and stopped responding.
> > 
> > I'm not going to debug same issues again, which I have analyzed,
> > prepared fixes, sent patches and see no progress there.
> > 
> > Seems that there is a status quo, and I'm not going to change it.
> 
> Dear Niklas,
> 
> Do you have any opinion on Pali's reply? Should patch 3/3 be discarded?

While I do have an opinion, I'm not going to share it on a public mailing
list :)

With regards to your patch 3/3, I think that your patch looks fine, but if
the driver maintainer does not want the cleanup for >reasons*, that is totally
fine with me. However, I'm not a PCI maintainer, so my opinion does not really
matter. It's the PCI maintainers that decide.


Kind regards,
Niklas

Re: [PATCH v3 3/3] PCI: aardvark: Remove redundant MPS configuration

[Hans Zhang]

On 2025/5/8 19:53, Niklas Cassel wrote:
> Hello Hans,
> 
> On Thu, May 08, 2025 at 12:47:12AM +0800, Hans Zhang wrote:
>> On 2025/5/8 00:36, Pali Rohár wrote:
>>>
>>> Sorry, but I stopped doing any testing of the aardvark driver with the
>>> mainline kernel after PCI maintainers stopped taking fixes for the
>>> driver and stopped responding.
>>>
>>> I'm not going to debug same issues again, which I have analyzed,
>>> prepared fixes, sent patches and see no progress there.
>>>
>>> Seems that there is a status quo, and I'm not going to change it.
>>
>> Dear Niklas,
>>
>> Do you have any opinion on Pali's reply? Should patch 3/3 be discarded?
> 
> While I do have an opinion, I'm not going to share it on a public mailing
> list :)
> 
> With regards to your patch 3/3, I think that your patch looks fine, but if
> the driver maintainer does not want the cleanup for >reasons*, that is totally
> fine with me. However, I'm not a PCI maintainer, so my opinion does not really
> matter. It's the PCI maintainers that decide.
> 

Dear Niklas,

Thank you very much for your reply. Let's wait for the decision of the 
PCI maintainer on this series of patches.

Best regards,
Hans

Re: [PATCH v3 3/3] PCI: aardvark: Remove redundant MPS configuration

[Pali Rohár]

On Friday 09 May 2025 00:12:24 Hans Zhang wrote:
> On 2025/5/8 19:53, Niklas Cassel wrote:
> > Hello Hans,
> > 
> > On Thu, May 08, 2025 at 12:47:12AM +0800, Hans Zhang wrote:
> > > On 2025/5/8 00:36, Pali Rohár wrote:
> > > > 
> > > > Sorry, but I stopped doing any testing of the aardvark driver with the
> > > > mainline kernel after PCI maintainers stopped taking fixes for the
> > > > driver and stopped responding.
> > > > 
> > > > I'm not going to debug same issues again, which I have analyzed,
> > > > prepared fixes, sent patches and see no progress there.
> > > > 
> > > > Seems that there is a status quo, and I'm not going to change it.
> > > 
> > > Dear Niklas,
> > > 
> > > Do you have any opinion on Pali's reply? Should patch 3/3 be discarded?
> > 
> > While I do have an opinion, I'm not going to share it on a public mailing
> > list :)
> > 
> > With regards to your patch 3/3, I think that your patch looks fine, but if
> > the driver maintainer does not want the cleanup for >reasons*, that is totally
> > fine with me. However, I'm not a PCI maintainer, so my opinion does not really
> > matter. It's the PCI maintainers that decide.
> > 
> 
> Dear Niklas,
> 
> Thank you very much for your reply. Let's wait for the decision of the PCI
> maintainer on this series of patches.
> 
> Best regards,
> Hans
> 

I do not see any cleanup in 3/3. There is just a removal of the step
needed for configuring the controller.

Re: [PATCH v3 3/3] PCI: aardvark: Remove redundant MPS configuration

[Manivannan Sadhasivam]

On Wed, May 07, 2025 at 06:36:20PM +0200, Pali Rohár wrote:
> On Wednesday 07 May 2025 23:06:51 Hans Zhang wrote:
> > On 2025/5/7 23:03, Hans Zhang wrote:
> > > On 2025/5/7 01:41, Pali Rohár wrote:
> > > > On Wednesday 07 May 2025 01:34:39 Hans Zhang wrote:
> > > > > The Aardvark PCIe controller enforces a fixed 512B payload size via
> > > > > PCI_EXP_DEVCTL_PAYLOAD_512B, overriding hardware capabilities and PCIe
> > > > > core negotiations.
> > > > > 
> > > > > Remove explicit MPS overrides (PCI_EXP_DEVCTL_PAYLOAD and
> > > > > PCI_EXP_DEVCTL_PAYLOAD_512B). MPS is now determined by the PCI core
> > > > > during device initialization, leveraging root port configurations and
> > > > > device-specific capabilities.
> > > > > 
> > > > > Aligning Aardvark with the unified MPS framework ensures consistency,
> > > > > avoids artificial constraints, and allows the hardware to operate at
> > > > > its maximum supported payload size while adhering to PCIe
> > > > > specifications.
> > > > > 
> > > > > Signed-off-by: Hans Zhang <18255117159@163.com>
> > > > > ---
> > > > >   drivers/pci/controller/pci-aardvark.c | 2 --
> > > > >   1 file changed, 2 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/pci/controller/pci-aardvark.c
> > > > > b/drivers/pci/controller/pci-aardvark.c
> > > > > index a29796cce420..d8852892994a 100644
> > > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > > @@ -549,9 +549,7 @@ static void advk_pcie_setup_hw(struct
> > > > > advk_pcie *pcie)
> > > > >       reg = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
> > > > >       reg &= ~PCI_EXP_DEVCTL_RELAX_EN;
> > > > >       reg &= ~PCI_EXP_DEVCTL_NOSNOOP_EN;
> > > > > -    reg &= ~PCI_EXP_DEVCTL_PAYLOAD;
> > > > >       reg &= ~PCI_EXP_DEVCTL_READRQ;
> > > > > -    reg |= PCI_EXP_DEVCTL_PAYLOAD_512B;
> > > > >       reg |= PCI_EXP_DEVCTL_READRQ_512B;
> > > > >       advk_writel(pcie, reg, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
> > > > > -- 
> > > > > 2.25.1
> > > > > 
> > > > 
> > > > Please do not remove this code. It is required part of the
> > > > initialization of the aardvark PCI controller at the specific phase,
> > > > as defined in the Armada 3700 Functional Specification.
> > > > 
> > > > There were reported more issues with those Armada PCIe controllers for
> > > > which were already sent patches to mailing list in last 5 years. But
> > > > unfortunately not all fixes were taken / applied yet.
> > > 
> > > Hi Pali,
> > > 
> > > I replied to you in version v2.
> > > 
> > > Is the maximum MPS supported by Armada 3700 512 bytes?
> 
> IIRC yes, 512-byte mode is supported. And I think in past I was testing
> some PCIe endpoint card which required 512-byte long payload and did not
> worked in 256-byte long mode (not sure if the card was not able to split
> transaction or something other was broken, it is quite longer time).
> 
> > > What are the default values of DevCap.MPS and DevCtl.MPS?
> 
> Do you mean values in the PCI-to-PCI bridge device of PCIe Root Port
> type?
> 
> Aardvark controller does not have the real HW PCI-to-PCI bridge device.
> There is only in-kernel emulation drivers/pci/pci-bridge-emul.c which
> create fake kernel PCI device in the hierarchy to make kernel and
> userspace happy. Yes, this is deviation from the PCIe standard but well,
> buggy HW is also HW.
> 
> And same applies for the pci-mvebu.c driver for older Marvell PCIe HW.
> 

Oh. Then this patch is not going to change the MPS setting of the root bus. But
that also means that there is a deviation in what the PCI core expects for a
root port and what is actually programmed in the hw.

Even in this MPS case, if the PCI core decides to scale down the MPS value of
the root port, then it won't be changed in the hw and the hw will continue to
work with 512B? Shouldn't the controller driver change the hw values based on
the values programmed by PCI core of the emul bridge?

But until that is fixed, this patch should be dropped.

- Mani

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

Re: [PATCH v3 3/3] PCI: aardvark: Remove redundant MPS configuration

[Hans Zhang]

On 2025/5/9 15:08, Manivannan Sadhasivam wrote:
> On Wed, May 07, 2025 at 06:36:20PM +0200, Pali Rohár wrote:
>> On Wednesday 07 May 2025 23:06:51 Hans Zhang wrote:
>>> On 2025/5/7 23:03, Hans Zhang wrote:
>>>> On 2025/5/7 01:41, Pali Rohár wrote:
>>>>> On Wednesday 07 May 2025 01:34:39 Hans Zhang wrote:
>>>>>> The Aardvark PCIe controller enforces a fixed 512B payload size via
>>>>>> PCI_EXP_DEVCTL_PAYLOAD_512B, overriding hardware capabilities and PCIe
>>>>>> core negotiations.
>>>>>>
>>>>>> Remove explicit MPS overrides (PCI_EXP_DEVCTL_PAYLOAD and
>>>>>> PCI_EXP_DEVCTL_PAYLOAD_512B). MPS is now determined by the PCI core
>>>>>> during device initialization, leveraging root port configurations and
>>>>>> device-specific capabilities.
>>>>>>
>>>>>> Aligning Aardvark with the unified MPS framework ensures consistency,
>>>>>> avoids artificial constraints, and allows the hardware to operate at
>>>>>> its maximum supported payload size while adhering to PCIe
>>>>>> specifications.
>>>>>>
>>>>>> Signed-off-by: Hans Zhang <18255117159@163.com>
>>>>>> ---
>>>>>>    drivers/pci/controller/pci-aardvark.c | 2 --
>>>>>>    1 file changed, 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/pci/controller/pci-aardvark.c
>>>>>> b/drivers/pci/controller/pci-aardvark.c
>>>>>> index a29796cce420..d8852892994a 100644
>>>>>> --- a/drivers/pci/controller/pci-aardvark.c
>>>>>> +++ b/drivers/pci/controller/pci-aardvark.c
>>>>>> @@ -549,9 +549,7 @@ static void advk_pcie_setup_hw(struct
>>>>>> advk_pcie *pcie)
>>>>>>        reg = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
>>>>>>        reg &= ~PCI_EXP_DEVCTL_RELAX_EN;
>>>>>>        reg &= ~PCI_EXP_DEVCTL_NOSNOOP_EN;
>>>>>> -    reg &= ~PCI_EXP_DEVCTL_PAYLOAD;
>>>>>>        reg &= ~PCI_EXP_DEVCTL_READRQ;
>>>>>> -    reg |= PCI_EXP_DEVCTL_PAYLOAD_512B;
>>>>>>        reg |= PCI_EXP_DEVCTL_READRQ_512B;
>>>>>>        advk_writel(pcie, reg, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
>>>>>> -- 
>>>>>> 2.25.1
>>>>>>
>>>>>
>>>>> Please do not remove this code. It is required part of the
>>>>> initialization of the aardvark PCI controller at the specific phase,
>>>>> as defined in the Armada 3700 Functional Specification.
>>>>>
>>>>> There were reported more issues with those Armada PCIe controllers for
>>>>> which were already sent patches to mailing list in last 5 years. But
>>>>> unfortunately not all fixes were taken / applied yet.
>>>>
>>>> Hi Pali,
>>>>
>>>> I replied to you in version v2.
>>>>
>>>> Is the maximum MPS supported by Armada 3700 512 bytes?
>>
>> IIRC yes, 512-byte mode is supported. And I think in past I was testing
>> some PCIe endpoint card which required 512-byte long payload and did not
>> worked in 256-byte long mode (not sure if the card was not able to split
>> transaction or something other was broken, it is quite longer time).
>>
>>>> What are the default values of DevCap.MPS and DevCtl.MPS?
>>
>> Do you mean values in the PCI-to-PCI bridge device of PCIe Root Port
>> type?
>>
>> Aardvark controller does not have the real HW PCI-to-PCI bridge device.
>> There is only in-kernel emulation drivers/pci/pci-bridge-emul.c which
>> create fake kernel PCI device in the hierarchy to make kernel and
>> userspace happy. Yes, this is deviation from the PCIe standard but well,
>> buggy HW is also HW.
>>
>> And same applies for the pci-mvebu.c driver for older Marvell PCIe HW.
>>
> 
> Oh. Then this patch is not going to change the MPS setting of the root bus. But
> that also means that there is a deviation in what the PCI core expects for a
> root port and what is actually programmed in the hw.
> 
> Even in this MPS case, if the PCI core decides to scale down the MPS value of
> the root port, then it won't be changed in the hw and the hw will continue to
> work with 512B? Shouldn't the controller driver change the hw values based on
> the values programmed by PCI core of the emul bridge?
> 
> But until that is fixed, this patch should be dropped.
> 

Dear Mani,

I will drop this patch in the next version.

Best regards,
Hans

Re: [PATCH v3 3/3] PCI: aardvark: Remove redundant MPS configuration

[Pali Rohár]

On Friday 09 May 2025 12:38:48 Manivannan Sadhasivam wrote:
> On Wed, May 07, 2025 at 06:36:20PM +0200, Pali Rohár wrote:
> > On Wednesday 07 May 2025 23:06:51 Hans Zhang wrote:
> > > On 2025/5/7 23:03, Hans Zhang wrote:
> > > > On 2025/5/7 01:41, Pali Rohár wrote:
> > > > > On Wednesday 07 May 2025 01:34:39 Hans Zhang wrote:
> > > > > > The Aardvark PCIe controller enforces a fixed 512B payload size via
> > > > > > PCI_EXP_DEVCTL_PAYLOAD_512B, overriding hardware capabilities and PCIe
> > > > > > core negotiations.
> > > > > > 
> > > > > > Remove explicit MPS overrides (PCI_EXP_DEVCTL_PAYLOAD and
> > > > > > PCI_EXP_DEVCTL_PAYLOAD_512B). MPS is now determined by the PCI core
> > > > > > during device initialization, leveraging root port configurations and
> > > > > > device-specific capabilities.
> > > > > > 
> > > > > > Aligning Aardvark with the unified MPS framework ensures consistency,
> > > > > > avoids artificial constraints, and allows the hardware to operate at
> > > > > > its maximum supported payload size while adhering to PCIe
> > > > > > specifications.
> > > > > > 
> > > > > > Signed-off-by: Hans Zhang <18255117159@163.com>
> > > > > > ---
> > > > > >   drivers/pci/controller/pci-aardvark.c | 2 --
> > > > > >   1 file changed, 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/pci/controller/pci-aardvark.c
> > > > > > b/drivers/pci/controller/pci-aardvark.c
> > > > > > index a29796cce420..d8852892994a 100644
> > > > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > > > @@ -549,9 +549,7 @@ static void advk_pcie_setup_hw(struct
> > > > > > advk_pcie *pcie)
> > > > > >       reg = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
> > > > > >       reg &= ~PCI_EXP_DEVCTL_RELAX_EN;
> > > > > >       reg &= ~PCI_EXP_DEVCTL_NOSNOOP_EN;
> > > > > > -    reg &= ~PCI_EXP_DEVCTL_PAYLOAD;
> > > > > >       reg &= ~PCI_EXP_DEVCTL_READRQ;
> > > > > > -    reg |= PCI_EXP_DEVCTL_PAYLOAD_512B;
> > > > > >       reg |= PCI_EXP_DEVCTL_READRQ_512B;
> > > > > >       advk_writel(pcie, reg, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
> > > > > > -- 
> > > > > > 2.25.1
> > > > > > 
> > > > > 
> > > > > Please do not remove this code. It is required part of the
> > > > > initialization of the aardvark PCI controller at the specific phase,
> > > > > as defined in the Armada 3700 Functional Specification.
> > > > > 
> > > > > There were reported more issues with those Armada PCIe controllers for
> > > > > which were already sent patches to mailing list in last 5 years. But
> > > > > unfortunately not all fixes were taken / applied yet.
> > > > 
> > > > Hi Pali,
> > > > 
> > > > I replied to you in version v2.
> > > > 
> > > > Is the maximum MPS supported by Armada 3700 512 bytes?
> > 
> > IIRC yes, 512-byte mode is supported. And I think in past I was testing
> > some PCIe endpoint card which required 512-byte long payload and did not
> > worked in 256-byte long mode (not sure if the card was not able to split
> > transaction or something other was broken, it is quite longer time).
> > 
> > > > What are the default values of DevCap.MPS and DevCtl.MPS?
> > 
> > Do you mean values in the PCI-to-PCI bridge device of PCIe Root Port
> > type?
> > 
> > Aardvark controller does not have the real HW PCI-to-PCI bridge device.
> > There is only in-kernel emulation drivers/pci/pci-bridge-emul.c which
> > create fake kernel PCI device in the hierarchy to make kernel and
> > userspace happy. Yes, this is deviation from the PCIe standard but well,
> > buggy HW is also HW.
> > 
> > And same applies for the pci-mvebu.c driver for older Marvell PCIe HW.
> > 
> 
> Oh. Then this patch is not going to change the MPS setting of the root bus. But
> that also means that there is a deviation in what the PCI core expects for a
> root port and what is actually programmed in the hw.

Yes, exactly this aardvark PCIe controller deviates from the PCIe spec
in lot of things. That is why it is needed to be really careful about
such changes.

Same applies for pci-mvebu.c. Both are PCIe controllers on Marvell
hardware, but it questionable from who both these IPs and hence source
of the issues.

Also these PCIe controllers have lot of HW bugs and documented and
undocumented erratas (for things which should work, but does not).

So it is not just as "enable or disable this bit and it would work". It
is needed to properly check if such functionality is provided by HW and
whether there is not some documented/undocumented errata for this
feature which could say "its broken, do not try to set this bit".

> Even in this MPS case, if the PCI core decides to scale down the MPS value of
> the root port, then it won't be changed in the hw and the hw will continue to
> work with 512B? Shouldn't the controller driver change the hw values based on
> the values programmed by PCI core of the emul bridge?

Marvell PCIe controllers has their own ways how to configure different
things of PCIe HW via custom platform registers. This is something which
needs to be properly understood and implemented as 1:1 mapping to kernel
root port emulator. Drivers should do it but it is unfinished. And as I
already said I stopped any development in this area years ago when PCIe
maintainers stopped taking my fixes for these drivers. As I said I'm not
going to spend my free time to investigate again issues there, prepare
fixes for them and just let them dropped into trash as nobody is
interested in them. I have other more useful things to do in my free
time.

> But until that is fixed, this patch should be dropped.

Re: [PATCH v3 3/3] PCI: aardvark: Remove redundant MPS configuration

[Manivannan Sadhasivam]

On Fri, May 09, 2025 at 06:00:25PM +0200, Pali Rohár wrote:
> On Friday 09 May 2025 12:38:48 Manivannan Sadhasivam wrote:
> > On Wed, May 07, 2025 at 06:36:20PM +0200, Pali Rohár wrote:
> > > On Wednesday 07 May 2025 23:06:51 Hans Zhang wrote:
> > > > On 2025/5/7 23:03, Hans Zhang wrote:
> > > > > On 2025/5/7 01:41, Pali Rohár wrote:
> > > > > > On Wednesday 07 May 2025 01:34:39 Hans Zhang wrote:
> > > > > > > The Aardvark PCIe controller enforces a fixed 512B payload size via
> > > > > > > PCI_EXP_DEVCTL_PAYLOAD_512B, overriding hardware capabilities and PCIe
> > > > > > > core negotiations.
> > > > > > > 
> > > > > > > Remove explicit MPS overrides (PCI_EXP_DEVCTL_PAYLOAD and
> > > > > > > PCI_EXP_DEVCTL_PAYLOAD_512B). MPS is now determined by the PCI core
> > > > > > > during device initialization, leveraging root port configurations and
> > > > > > > device-specific capabilities.
> > > > > > > 
> > > > > > > Aligning Aardvark with the unified MPS framework ensures consistency,
> > > > > > > avoids artificial constraints, and allows the hardware to operate at
> > > > > > > its maximum supported payload size while adhering to PCIe
> > > > > > > specifications.
> > > > > > > 
> > > > > > > Signed-off-by: Hans Zhang <18255117159@163.com>
> > > > > > > ---
> > > > > > >   drivers/pci/controller/pci-aardvark.c | 2 --
> > > > > > >   1 file changed, 2 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/pci/controller/pci-aardvark.c
> > > > > > > b/drivers/pci/controller/pci-aardvark.c
> > > > > > > index a29796cce420..d8852892994a 100644
> > > > > > > --- a/drivers/pci/controller/pci-aardvark.c
> > > > > > > +++ b/drivers/pci/controller/pci-aardvark.c
> > > > > > > @@ -549,9 +549,7 @@ static void advk_pcie_setup_hw(struct
> > > > > > > advk_pcie *pcie)
> > > > > > >       reg = advk_readl(pcie, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
> > > > > > >       reg &= ~PCI_EXP_DEVCTL_RELAX_EN;
> > > > > > >       reg &= ~PCI_EXP_DEVCTL_NOSNOOP_EN;
> > > > > > > -    reg &= ~PCI_EXP_DEVCTL_PAYLOAD;
> > > > > > >       reg &= ~PCI_EXP_DEVCTL_READRQ;
> > > > > > > -    reg |= PCI_EXP_DEVCTL_PAYLOAD_512B;
> > > > > > >       reg |= PCI_EXP_DEVCTL_READRQ_512B;
> > > > > > >       advk_writel(pcie, reg, PCIE_CORE_PCIEXP_CAP + PCI_EXP_DEVCTL);
> > > > > > > -- 
> > > > > > > 2.25.1
> > > > > > > 
> > > > > > 
> > > > > > Please do not remove this code. It is required part of the
> > > > > > initialization of the aardvark PCI controller at the specific phase,
> > > > > > as defined in the Armada 3700 Functional Specification.
> > > > > > 
> > > > > > There were reported more issues with those Armada PCIe controllers for
> > > > > > which were already sent patches to mailing list in last 5 years. But
> > > > > > unfortunately not all fixes were taken / applied yet.
> > > > > 
> > > > > Hi Pali,
> > > > > 
> > > > > I replied to you in version v2.
> > > > > 
> > > > > Is the maximum MPS supported by Armada 3700 512 bytes?
> > > 
> > > IIRC yes, 512-byte mode is supported. And I think in past I was testing
> > > some PCIe endpoint card which required 512-byte long payload and did not
> > > worked in 256-byte long mode (not sure if the card was not able to split
> > > transaction or something other was broken, it is quite longer time).
> > > 
> > > > > What are the default values of DevCap.MPS and DevCtl.MPS?
> > > 
> > > Do you mean values in the PCI-to-PCI bridge device of PCIe Root Port
> > > type?
> > > 
> > > Aardvark controller does not have the real HW PCI-to-PCI bridge device.
> > > There is only in-kernel emulation drivers/pci/pci-bridge-emul.c which
> > > create fake kernel PCI device in the hierarchy to make kernel and
> > > userspace happy. Yes, this is deviation from the PCIe standard but well,
> > > buggy HW is also HW.
> > > 
> > > And same applies for the pci-mvebu.c driver for older Marvell PCIe HW.
> > > 
> > 
> > Oh. Then this patch is not going to change the MPS setting of the root bus. But
> > that also means that there is a deviation in what the PCI core expects for a
> > root port and what is actually programmed in the hw.
> 
> Yes, exactly this aardvark PCIe controller deviates from the PCIe spec
> in lot of things. That is why it is needed to be really careful about
> such changes.
> 
> Same applies for pci-mvebu.c. Both are PCIe controllers on Marvell
> hardware, but it questionable from who both these IPs and hence source
> of the issues.
> 
> Also these PCIe controllers have lot of HW bugs and documented and
> undocumented erratas (for things which should work, but does not).
> 
> So it is not just as "enable or disable this bit and it would work". It
> is needed to properly check if such functionality is provided by HW and
> whether there is not some documented/undocumented errata for this
> feature which could say "its broken, do not try to set this bit".
> 
> > Even in this MPS case, if the PCI core decides to scale down the MPS value of
> > the root port, then it won't be changed in the hw and the hw will continue to
> > work with 512B? Shouldn't the controller driver change the hw values based on
> > the values programmed by PCI core of the emul bridge?
> 
> Marvell PCIe controllers has their own ways how to configure different
> things of PCIe HW via custom platform registers. This is something which
> needs to be properly understood and implemented as 1:1 mapping to kernel
> root port emulator. Drivers should do it but it is unfinished. And as I
> already said I stopped any development in this area years ago when PCIe
> maintainers stopped taking my fixes for these drivers. As I said I'm not
> going to spend my free time to investigate again issues there, prepare
> fixes for them and just let them dropped into trash as nobody is
> interested in them. I have other more useful things to do in my free
> time.
> 

If the patches are not related to unloading the driver which acts as a msi
controller, I don't see issues with them ;) But I have no visibility on the past
conversations.

- Mani

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