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.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
