[PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller

Thanks Duc! I will test quickly if you have it today :)

-- 
Computer Architect | Sent from my 64-bit #ARM Powered phone

On Dec 1, 2016, at 17:10, Duc Dang [off-list ref] wrote:

quoted

On Thu, Dec 1, 2016 at 11:41 AM, Bjorn Helgaas [off-list ref] wrote:

quoted

On Thu, Dec 01, 2016 at 02:20:53PM -0500, Mark Salter wrote:

quoted

On Thu, 2016-12-01 at 12:33 -0600, Bjorn Helgaas wrote:
On Wed, Nov 30, 2016 at 03:42:53PM -0800, Duc Dang wrote:

quoted

quoted

quoted

+static struct resource xgene_v1_csr_res[] = {
+ [0] = DEFINE_RES_MEM_NAMED(0x1f2b0000UL, SZ_64K, "PCIe CSR"),
+ [1] = DEFINE_RES_MEM_NAMED(0x1f2c0000UL, SZ_64K, "PCIe CSR"),
+ [2] = DEFINE_RES_MEM_NAMED(0x1f2d0000UL, SZ_64K, "PCIe CSR"),
+ [3] = DEFINE_RES_MEM_NAMED(0x1f500000UL, SZ_64K, "PCIe CSR"),
+ [4] = DEFINE_RES_MEM_NAMED(0x1f510000UL, SZ_64K, "PCIe CSR"),

I assume these ranges are not the actual ECAM space, right?
If they *were* ECAM, I assume you would have included them in the
quirk itself in the mcfg_quirks[] table.

These are base addresses for some RC mmio registers.

quoted

quoted

+static int xgene_v1_pcie_ecam_init(struct pci_config_window *cfg)
+{
+ struct acpi_device *adev = to_acpi_device(cfg->parent);
+ struct acpi_pci_root *root = acpi_driver_data(adev);
+ struct device *dev = cfg->parent;
+ struct xgene_pcie_port *port;
+ struct resource *csr;
+
+ port = devm_kzalloc(dev, sizeof(*port), GFP_KERNEL);
+ if (!port)
+         return -ENOMEM;
+
+ csr = &xgene_v1_csr_res[root->segment];

This makes me nervous because root->segment comes from the ACPI _SEG,
and if firmware gives us junk in _SEG, we will reference something in
the weeds.

The SoC provide some number of RC bridges, each with a different base
for some mmio registers. Even if segment is legitimate in MCFG, there
is still a problem if a platform doesn't use the segment ordering
implied by the code. But the PNP0A03 _CRS does have this base address
as the first memory resource, so we could get it from there and not
have hard-coded addresses and implied ording in the quirk code.

I'm confused.  Doesn't the current code treat every item in PNP0A03
_CRS as a window?  Do you mean the first resource is handled
differently somehow?  The Consumer/Producer bit could allow us to do
this by marking the RC MMIO space as "Consumer", but I didn't think
that strategy was quite working yet.

The first resource is defined like below. It was introduced long time
ago to use with older version of X-Gene ECAM quirks.
Memory32Fixed(ReadWrite, 0x1F2B0000, 0x10000, )

The resource discovered during booting will be like following:
[    0.728117] ACPI: MCFG table detected, 1 entries
[    0.735330] ACPI: Power Resource [SCVR] (on)
[    0.767478] ACPI: PCI Root Bridge [PCI0] (domain 0000 [bus 00-ff])
[    0.774013] acpi PNP0A08:00: _OSC: OS supports [ExtendedConfig ASPM
ClockPM Segments MSI]
[    0.782864] acpi PNP0A08:00: _OSC: OS now controls [PCIeHotplug PME
AER PCIeCapability]
[    0.791331] acpi PNP0A08:00: MCFG quirk: ECAM at [mem
0xe0d0000000-0xe0dfffffff] for [bus 00-ff] with xgene_v1_pcie_ecam_ops
[    0.803207] acpi PNP0A08:00: ECAM at [mem
0xe0d0000000-0xe0dfffffff] for [bus 00-ff]
[    0.811399] Remapped I/O 0x000000e010000000 to [io  0x0000-0xffff window]
[    0.818678] PCI host bridge to bus 0000:00
[    0.822990] pci_bus 0000:00: root bus resource [mem 0x1f2b0000-0x1f2bffff]
[    0.830257] pci_bus 0000:00: root bus resource [io  0x0000-0xffff
window] (bus address [0x10000000-0x1000ffff])
[    0.840917] pci_bus 0000:00: root bus resource [mem
0xe040000000-0xe07fffffff window] (bus address
[0x40000000-0x7fffffff])
[    0.852675] pci_bus 0000:00: root bus resource [mem
0xf000000000-0xffffffffff window]
[    0.860950] pci_bus 0000:00: root bus resource [bus 00-ff]
[    0.866761] pci 0000:00:00.0: [10e8:e004] type 01 class 0x060400
[    0.873175] pci 0000:00:00.0: supports D1 D2
[    0.877980] pci 0000:01:00.0: [15b3:1003] type 00 class 0x020000
[    0.884597] pci 0000:01:00.0: reg 0x10: [mem 0xe040000000-0xe0400fffff 64bit]
[    0.892337] pci 0000:01:00.0: reg 0x18: [mem
0xe042000000-0xe043ffffff 64bit pref]
[    0.900694] pci 0000:01:00.0: reg 0x30: [mem 0xfff00000-0xffffffff pref]
[    0.923853] pci_bus 0000:00: on NUMA node 0
[    0.928269] pci 0000:00:00.0: BAR 15: assigned [mem
0xf000000000-0xf001ffffff 64bit pref]
[    0.936908] pci 0000:00:00.0: BAR 14: assigned [mem
0xe040000000-0xe0401fffff]
[    0.944539] pci 0000:01:00.0: BAR 2: assigned [mem
0xf000000000-0xf001ffffff 64bit pref]
[    0.953210] pci 0000:01:00.0: BAR 0: assigned [mem
0xe040000000-0xe0400fffff 64bit]
[    0.961430] pci 0000:01:00.0: BAR 6: assigned [mem
0xe040100000-0xe0401fffff pref]
[    0.969438] pci 0000:00:00.0: PCI bridge to [bus 01]
[    0.974690] pci 0000:00:00.0:   bridge window [mem 0xe040000000-0xe0401fffff]
[    0.982231] pci 0000:00:00.0:   bridge window [mem
0xf000000000-0xf001ffffff 64bit pref]

quoted

quoted

I have tested a modified version of these quirks using this to
get the CSR base and it works on the 3 different platforms I have
access to.

static int xgene_pcie_get_csr(struct device *dev, struct resource *r)
{
     struct acpi_device *adev = to_acpi_device(dev);
     unsigned long flags;
     struct list_head list;
     struct resource_entry *entry;
     int ret;

     INIT_LIST_HEAD(&list);
     flags = IORESOURCE_MEM;
     ret = acpi_dev_get_resources(adev, &list,
                                  acpi_dev_filter_resource_type_cb,
                                  (void *)flags);
     if (ret < 0) {
             dev_err(dev, "failed to parse _CRS, error: %d\n", ret);
             return ret;
     } else if (ret == 0) {
             dev_err(dev, "no memory resources present in _CRS\n");
             return -EINVAL;
     }

     entry = list_first_entry(&list, struct resource_entry, node);
     *r = *entry->res;
     acpi_dev_free_resource_list(&list);
     return 0;
}

The code above is identical to acpi_get_rc_addr(), which is used in
the acpi_get_rc_resources() path by the other quirks.  Can you use
that path, too, instead of reimplementing it here?

I will post a new version using acpi_get_rc_resources and includes
other changes that you suggested.

Regards,
Duc Dang.