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

[Duc Dang]

On Thu, Dec 1, 2016 at 11:41 AM, Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
> On Thu, Dec 01, 2016 at 02:20:53PM -0500, Mark Salter wrote:
>> 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:
>
>> > > +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.
>> >
>> > >
>> > > +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]

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