Re: [EXTERNAL] Re: [PATCH] PCI: cadence: Set the AFS bit in Device Capabilities 2 Register

From: Verma, Achal
Date: Fri Aug 04 2023 - 03:53:24 EST

On 8/2/2023 9:49 PM, Bjorn Helgaas wrote:
In subject, "Advertise ARI Forwarding Supported".

It's not obvious that "AFS" refers to ARI Forwarding Supported, and
the bit name is enough; we don't need to know that it's in Dev Cap 2.
"Advertise" shows that we're just *advertising* the functionality, not
*enabling* it.

On Wed, Aug 02, 2023 at 04:00:59PM +0530, Achal Verma wrote:
J7 PCIe Root Complex has ARI Forwarding Support, means supporting
forwarding of TLPs addressed to functions with function number greater than
7 but some PCIe instances on J7 have this bit cleared which results in
failure of forwarding of TLPs destined for function number > 7.
Setting the AFS bit in Device Capabilities 2 Register explicitly, resolves
the issue and leads to successful access to function number > 7.

s/AFS/ARI Forwarding Supported/

Some observations:
1. J7200-EVB has single PCIe instance(PCIe1) for which ARIFwd bit is not
set. Enumeration gracefully fails for funciton number greater than 7 but
later read/write access to these funcitons results in a crash.

By "ARIFwd bit" here, I assume you mean PCI_EXP_DEVCAP2_ARI in the Root
Port? Maybe you can use the #define to make this more greppable.

will replace with PCI_EXP_DEVCAP2_ARI
s/funciton/function/ (twice)

If we don't enumerate function numbers greater than 7, we shouldn't
have pci_dev structs for them, so why are there later read/write
config accesses to them?

If the Root Port doesn't advertise ARI Forwarding Supported,
bridge->ari_enabled will not be set, and we shouldn't even try to
enumerate functions greater than 7. So it's not that enumeration
*fails*; it just doesn't happen at all.

2. On J721E-EVB, PCIe1 instance has ARIFwd bit set while it is cleared for
PCIe0 instance. This issue combined with errata i2086
(Unsupported Request (UR) Response Results in External Abort) results in
SERROR while scanning multi-function endpoint device.

Is the SERROR when scanning under PCIe0 or under PCIe1?

I'm not clear on what's happening here:

1) Root Port advertises PCI_EXP_DEVCAP2_ARI, we set
bridge->ari_enabled and scan functions > 7, we do a config read
to function 8, get a UR response (as expected during enumeration)
and that results in SERROR?

2) Root Port *doesn't* advertise PCI_EXP_DEVCAP2_ARI, we don't set
bridge->ari_enabled, so we don't try config read to function 8,
and something blows up later?

3) Something else?

Hello Bjorn,

There are multiple issues which are leading to different behavior on different platforms.

1. PCI_EXP_DEVCAP2_ARI is not set.

Consider scenario:
J7200 (RC) --- J721E (EP with 1 PF and 4 VFs)

PF enumerates successfully on J7200 but bringing up 4 associated VFs (echo 4 > /sys/bus/pci/devices/<device>/sriov_numvfs) doesn't workout. First VF gets devfn = 6 and then +1 for next one on wards. 6 and 7 are setup fine but 8 and 9 fails and UR is received while accessing them. Accessing VFs > 7 doesn't go through the ARI support check and directly calls pci_setup_device(). So, since PCI_EXP_DEVCAP2_ARI is not set, unable to access VFs > 7.


Same issue happens when J7200 RC is replaced by J721E PCIe0 in above setup but because of errata i2086 in this case, UR response results in SERROR too.

2. PCI_ARI_CAP_NFN field in PCI_ARI_CAP for last function is not set to 0 which marks current function as last but instead set to current_fn+1, which leads to reading of vendor ID for current_fn+1 which doesn't exists, giving UR response.

Consider scenario:
J721E(PCIe1 as RC) ----- J721E (EP with 1PF)
PCIe1 has PCI_EXP_DEVCAP2_ARI set , so it reads the PCI_ARI_CAP_NFN field of EP to get the next function number, now since at last function (PF=0) PCI_ARI_CAP_NFN field is set to 1 and not to 0, RC tries to access devfn=1 , which results in UR along with SERROR because of errata i2086.
For this I had pushed the workaround


@@ -507,6 +507,7 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
struct cdns_pcie *pcie;
struct resource *res;
int ret;
+ u32 pcie_cap2;
bridge = pci_host_bridge_from_priv(rc);
if (!bridge)
@@ -536,6 +537,12 @@ int cdns_pcie_host_setup(struct cdns_pcie_rc *rc)
if (rc->quirk_detect_quiet_flag)
+ if (rc->set_afs_bit) {
+ pcie_cap2 = cdns_pcie_rp_readl(pcie, CDNS_PCIE_RP_CAP_OFFSET + PCI_EXP_DEVCAP2);
+ pcie_cap2 |= PCI_EXP_DEVCAP2_ARI;
+ cdns_pcie_rp_writel(pcie, CDNS_PCIE_RP_CAP_OFFSET + PCI_EXP_DEVCAP2, pcie_cap2);
+ }

This seems like a j721e defect; why does the workaround need to be in
the generic cadence code?

This register setting has to go between devm_platform_ioremap_resource_byname(pdev, "reg") and cdns_pcie_start_link() which is in pcie-cadence-host.c

And this will be done only if private data field set_afs_bit is true, other quirks are also done in similar way.

Achal Verma