PCI: Configure max payload size on pci_host_probe

[PATCH v2 2/2] PCI: Remove redundant MPS configuration

Posted by Hans Zhang 9 months, 2 weeks ago

With the PCI core now centrally configuring root port MPS to
hardware-supported maximums (via 128 << pcie_mpss) during host probing,
platform-specific MPS adjustments are redundant. This patch removes the
custom the configuration of the max payload logic to align with the
standardized initialization flow.

By eliminating redundant code, this change prevents conflicts with global
PCIe hierarchy tuning policies and reduces maintenance overhead. The Meson
driver now fully relies on the core PCI framework for MPS configuration,
ensuring consistency across the PCIe topology while preserving
hardware-specific MRRS handling.

Signed-off-by: Hans Zhang <18255117159@163.com>
---
 drivers/pci/controller/dwc/pci-meson.c | 17 -----------------
 drivers/pci/controller/pci-aardvark.c  |  2 --
 2 files changed, 19 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;
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 v2 2/2] PCI: Remove redundant MPS configuration

Posted by Pali Rohár 9 months, 2 weeks ago

On Friday 25 April 2025 17:57:08 Hans Zhang wrote:
> 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.

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

Posted by Hans Zhang 9 months, 2 weeks ago


On 2025/4/26 02:13, Pali Rohár wrote:
> On Friday 25 April 2025 17:57:08 Hans Zhang wrote:
>> 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.

Hi Pali,

This series of patches is discussing the initialization of DevCtl.MPS by 
the Root Port. Please look at the first patch I submitted. If there is a 
reasonable method in the end, DevCtl.MPS will also be configured 
successfully. The PCIe maintainer will give the review opinions. Please 
rest assured that it will not affect the functions of the aardvark PCI 
controller.

Rockchip's RK3588 also has the same problem.


https://patchwork.kernel.org/project/linux-pci/patch/20250416151926.140202-1-18255117159@163.com/

Best regards,
Hans

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

Posted by Pali Rohár 9 months, 2 weeks ago

On Saturday 26 April 2025 23:02:08 Hans Zhang wrote:
> On 2025/4/26 02:13, Pali Rohár wrote:
> > On Friday 25 April 2025 17:57:08 Hans Zhang wrote:
> > > 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.
> 
> Hi Pali,
> 
> This series of patches is discussing the initialization of DevCtl.MPS by the
> Root Port. Please look at the first patch I submitted. If there is a
> reasonable method in the end, DevCtl.MPS will also be configured
> successfully.

This does not matter what would be configured in DevCtl.MPS at the end.

> The PCIe maintainer will give the review opinions. Please rest
> assured that it will not affect the functions of the aardvark PCI
> controller.

This patch is modifying initialization of the aardvark PCIe controller
and is removing the mandatory step of the controller configuration as
required and defined in the Armada 3700 Functional Specification.
It says exactly in which order and which values to which registers has
to be written.

> Rockchip's RK3588 also has the same problem.
> 
> 
> https://patchwork.kernel.org/project/linux-pci/patch/20250416151926.140202-1-18255117159@163.com/
> 
> Best regards,
> Hans
>

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

Posted by Hans Zhang 9 months, 2 weeks ago


On 2025/4/26 23:06, Pali Rohár wrote:
> On Saturday 26 April 2025 23:02:08 Hans Zhang wrote:
>> On 2025/4/26 02:13, Pali Rohár wrote:
>>> On Friday 25 April 2025 17:57:08 Hans Zhang wrote:
>>>> 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.
>>
>> Hi Pali,
>>
>> This series of patches is discussing the initialization of DevCtl.MPS by the
>> Root Port. Please look at the first patch I submitted. If there is a
>> reasonable method in the end, DevCtl.MPS will also be configured
>> successfully.
> 
> This does not matter what would be configured in DevCtl.MPS at the end.
> 
>> The PCIe maintainer will give the review opinions. Please rest
>> assured that it will not affect the functions of the aardvark PCI
>> controller.
> 
> This patch is modifying initialization of the aardvark PCIe controller
> and is removing the mandatory step of the controller configuration as
> required and defined in the Armada 3700 Functional Specification.
> It says exactly in which order and which values to which registers has
> to be written.

Hi Pali,

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.

Best regards,
Hans

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

Posted by neil.armstrong@linaro.org 9 months, 2 weeks ago

On 25/04/2025 11:57, Hans Zhang wrote:
> With the PCI core now centrally configuring root port MPS to
> hardware-supported maximums (via 128 << pcie_mpss) during host probing,
> platform-specific MPS adjustments are redundant. This patch removes the
> custom the configuration of the max payload logic to align with the
> standardized initialization flow.
> 
> By eliminating redundant code, this change prevents conflicts with global
> PCIe hierarchy tuning policies and reduces maintenance overhead. The Meson
> driver now fully relies on the core PCI framework for MPS configuration,
> ensuring consistency across the PCIe topology while preserving
> hardware-specific MRRS handling.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
>   drivers/pci/controller/dwc/pci-meson.c | 17 -----------------
>   drivers/pci/controller/pci-aardvark.c  |  2 --
>   2 files changed, 19 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);

Seems you can also remove meson_set_max_rd_req_size() since it's
done by pcie_write_mrrs()

Neil

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

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

Posted by Hans Zhang 9 months, 2 weeks ago


On 2025/4/25 19:59, neil.armstrong@linaro.org wrote:
> On 25/04/2025 11:57, Hans Zhang wrote:
>> With the PCI core now centrally configuring root port MPS to
>> hardware-supported maximums (via 128 << pcie_mpss) during host probing,
>> platform-specific MPS adjustments are redundant. This patch removes the
>> custom the configuration of the max payload logic to align with the
>> standardized initialization flow.
>>
>> By eliminating redundant code, this change prevents conflicts with global
>> PCIe hierarchy tuning policies and reduces maintenance overhead. The 
>> Meson
>> driver now fully relies on the core PCI framework for MPS configuration,
>> ensuring consistency across the PCIe topology while preserving
>> hardware-specific MRRS handling.
>>
>> Signed-off-by: Hans Zhang <18255117159@163.com>
>> ---
>>   drivers/pci/controller/dwc/pci-meson.c | 17 -----------------
>>   drivers/pci/controller/pci-aardvark.c  |  2 --
>>   2 files changed, 19 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);
> 
> Seems you can also remove meson_set_max_rd_req_size() since it's
> done by pcie_write_mrrs()


Dear neil,

Thank you very much for your reply and reminder.

I want to wait for the result of the first patch discussion, and then 
see if we need to remove meson_set_max_rd_req_size().

Best regards,
Hans

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

Posted by Niklas Cassel 9 months, 2 weeks ago

On Fri, Apr 25, 2025 at 05:57:08PM +0800, Hans Zhang wrote:
> With the PCI core now centrally configuring root port MPS to
> hardware-supported maximums (via 128 << pcie_mpss) during host probing,
> platform-specific MPS adjustments are redundant. This patch removes the
> custom the configuration of the max payload logic to align with the
> standardized initialization flow.
> 
> By eliminating redundant code, this change prevents conflicts with global
> PCIe hierarchy tuning policies and reduces maintenance overhead. The Meson
> driver now fully relies on the core PCI framework for MPS configuration,
> ensuring consistency across the PCIe topology while preserving
> hardware-specific MRRS handling.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
>  drivers/pci/controller/dwc/pci-meson.c | 17 -----------------
>  drivers/pci/controller/pci-aardvark.c  |  2 --

Since you are touching two drivers (and the changes are not exactly identical),
I suggest that you do one patch per driver.


Kind regards,
Niklas

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

Posted by Hans Zhang 9 months, 2 weeks ago


On 2025/4/25 18:17, Niklas Cassel wrote:
> On Fri, Apr 25, 2025 at 05:57:08PM +0800, Hans Zhang wrote:
>> With the PCI core now centrally configuring root port MPS to
>> hardware-supported maximums (via 128 << pcie_mpss) during host probing,
>> platform-specific MPS adjustments are redundant. This patch removes the
>> custom the configuration of the max payload logic to align with the
>> standardized initialization flow.
>>
>> By eliminating redundant code, this change prevents conflicts with global
>> PCIe hierarchy tuning policies and reduces maintenance overhead. The Meson
>> driver now fully relies on the core PCI framework for MPS configuration,
>> ensuring consistency across the PCIe topology while preserving
>> hardware-specific MRRS handling.
>>
>> Signed-off-by: Hans Zhang <18255117159@163.com>
>> ---
>>   drivers/pci/controller/dwc/pci-meson.c | 17 -----------------
>>   drivers/pci/controller/pci-aardvark.c  |  2 --
> 
> Since you are touching two drivers (and the changes are not exactly identical),
> I suggest that you do one patch per driver.

Dear Niklas,

Thank you very much for your reply. In the next version, I will split 
two patches.

Best regards,
Hans