Message-ID: <20250509160025.s65aw5ix6s7533b5@pali>
Date: Fri, 9 May 2025 18:00:25 +0200
From: Pali Rohár <pali@...nel.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Cc: Hans Zhang <18255117159@....com>, lpieralisi@...nel.org, kw@...ux.com,
	bhelgaas@...gle.com, heiko@...ech.de, yue.wang@...ogic.com,
	neil.armstrong@...aro.org, robh@...nel.org, jingoohan1@...il.com,
	khilman@...libre.com, jbrunet@...libre.com,
	martin.blumenstingl@...glemail.com, linux-pci@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-amlogic@...ts.infradead.org,
	linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v3 3/3] PCI: aardvark: Remove redundant MPS configuration

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

Powered by blists - more mailing lists