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

[Hans Zhang <18255117159@163.com>]

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