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 <dhdang@xxxxxxx> wrote:
>
>> 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
