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

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