LS1043A : "synchronous abort" at boot due to PCI config read

Commit Message

Gilles Buloz April 27, 2018, 12:29 p.m. UTC

Le 27/04/2018 10:43, Ard Biesheuvel a écrit :
> (add Bjorn and linux-pci)
>
> On 13 April 2018 at 19:32, Gilles Buloz <[e-mail]> wrote:
>> Dear developers,
>>
>> I currently have two functional workarounds for this issue but would like to know which one you would recommend, if any :-)
>> I'm using a LS1043A CPU (NXP QorIQ Layerscape) and get a "synchronous external abort" when booting because of a PCI config read
>> during PCI scan.
>>
>> I'm using a custom hardware (based on LS1043ARDB) having a PEX8112 PCIe-to-PCI bridge connected to the LS1043A to have a PCI slot
>> for legacy devices. This bridge only supports PCI-Compatible config accesses (offset 0x00-0xFF).
>> On this PCI slot I connect a PCI module made of a PCI-to-PCIe bridge plus PCIe devices behind.
>> The problem occurs when the kernel probes the PCIe devices : as they are PCIe devices, the kernel does a PCI config read access at
>> offset 0x100 to check if "PCIe extended capability registers" are accessible (see drivers/pci/probe.c, function
>> pci_cfg_space_size_ext()). Unfortunately the PEX8112 PCIe-to-PCI bridge that is in the path reports an error to the CPU for this
>> access, and it seems there's no way to disable that on this bridge.
>>
>> The first workaround I found was to patch drivers/pci/host/pci-layerscape.c to have PCIE_ABSERR_SETTING set to 0x9400 instead of
>> 0x9401 (for PCIE_ABSERR register) to disable error reporting. This only impacts an NXP part of the Linux kernel code, but I'm not
>> sure this is a good idea (however it seems to be like that on Intel platforms where even MEM accesses to a no-device address return
>> FF without any error).
>>
>> I've also tried another workaround that works : patch drivers/pci/probe.c to use bus_flags to remember if a bus is behind a bridge
>> without extended address capability, to avoid PCi config read accesses at offset 0x100 in
>> pci_cfg_space_size() / pci_cfg_space_size_ext(). But this patch impacts the generic PCI probe method of Linux.
>>
>> Any Idea to properly handle that issue ?
>>
> This seems like a rather unusual configuration, but I guess that if
> the first bridge/switch advertises its inability to support extended
> config space accesses, we should not be performing them on any of its
> subordinate buses. How does the PEX8112 advertise this limitation?
>
> That said, I wonder if it is reasonable in the first place to expect
> that a PCIe device works as expected passing through a legacy PCI
> layer like that.
>
> .
The PEX8112 PCIe-to-PCI bridge has capability PCI_CAP_ID_EXP, but has no PCI_CAP_ID_PCIX capability.
As I understand the lack of PCI_CAP_ID_PCIX is advertising this limitation on the PCI side (no support for PCI config offset >=0x100).
Also I guess in the case of a bridge having PCI_CAP_ID_PCIX, this limitation would be advertised by the lack of PCI_X_STATUS_266MHZ 
and PCI_X_STATUS_533MHZ (as done in drivers/pci/probe.c at pci_cfg_space_size())

I'm currently using the attached patch (for kernel 4.1.35-rt41 from NXP Yocto BSP). It uses bus_flags to remember if a bus is behind 
a bridge without extended address capability to avoid PCi config accesses at offset >= 0x100. Thanks to this patch I now have a 
functional system with functional PCI/PCIe devices.

Patch

--- include/linux/pci.h.orig	2018-03-26 16:51:18.050000000 +0000
+++ include/linux/pci.h	2018-03-26 16:51:27.660000000 +0000
@@ -193,6 +193,7 @@ 
 enum pci_bus_flags {
 	PCI_BUS_FLAGS_NO_MSI   = (__force pci_bus_flags_t) 1,
 	PCI_BUS_FLAGS_NO_MMRBC = (__force pci_bus_flags_t) 2,
+	PCI_BUS_FLAGS_COMPAT_CFG_SPACE = (__force pci_bus_flags_t) 4,
 };
 
 /* These values come from the PCI Express Spec */
--- drivers/pci/probe.c.orig	2018-01-22 09:29:52.000000000 +0000
+++ drivers/pci/probe.c	2018-03-26 16:54:30.830000000 +0000
@@ -827,6 +827,28 @@ 
 			child->primary = primary;
 			pci_bus_insert_busn_res(child, secondary, subordinate);
 			child->bridge_ctl = bctl;
+
+			{
+				int pos;
+				u32 status;
+				bool pci_compat_cfg_space = false;
+
+				if (!pci_is_pcie(dev) || (pci_pcie_type(dev) == PCI_EXP_TYPE_PCIE_BRIDGE) || (pci_pcie_type(dev) == PCI_EXP_TYPE_PCI_BRIDGE)) {
+					/* for PCI/PCI bridges, or PCIe/PCI bridge in forward or reverse mode, we have to check for PCI-X capabilities */
+					pos = pci_find_capability(dev, PCI_CAP_ID_PCIX);
+					if (pos) {
+						pci_read_config_dword(dev, pos + PCI_X_STATUS, &status);
+						if (!(status & (PCI_X_STATUS_266MHZ | PCI_X_STATUS_533MHZ)))
+							pci_compat_cfg_space = true;
+					} else {
+						pci_compat_cfg_space = true;
+					}
+					if (pci_compat_cfg_space) {
+						dev_info(&dev->dev, "[%04x:%04x] Child bus limited to PCI-Compatible config space\n", dev->vendor, dev->device);
+						child->bus_flags |= PCI_BUS_FLAGS_COMPAT_CFG_SPACE;
+					}
+				}
+			}
 		}
 
 		cmax = pci_scan_child_bus(child);
@@ -1098,6 +1120,11 @@ 
 			goto fail;
 	}
 
+	if (dev->bus->bus_flags & PCI_BUS_FLAGS_COMPAT_CFG_SPACE) {
+		dev_info(&dev->dev, "[%04x:%04x] PCI-Compatible config space only due to parent bus(es)\n", dev->vendor, dev->device);
+		return PCI_CFG_SPACE_SIZE;
+	}
+
 	return pci_cfg_space_size_ext(dev);
 
  fail: