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@xxxxxxx> > > > > > > > --- > > > > > > > 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 -- மணிவண்ணன் சதாசிவம்
