Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1761577AbcLAWrg convert rfc822-to-8bit (ORCPT ); Thu, 1 Dec 2016 17:47:36 -0500 Received: from mx6-phx2.redhat.com ([209.132.183.39]:42934 "EHLO mx6-phx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1761208AbcLAWre (ORCPT ); Thu, 1 Dec 2016 17:47:34 -0500 Content-Type: text/plain; charset=us-ascii Content-Transfer-Encoding: 8BIT From: Jon Masters MIME-Version: 1.0 Subject: Re: [PATCH v3] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller Message-Id: Date: Thu, 1 Dec 2016 17:31:21 -0500 (EST) References: <1480549373-2123-1-git-send-email-dhdang@apm.com> <20161201183350.GF30746@bhelgaas-glaptop.roam.corp.google.com> <1480620053.4751.30.camel@redhat.com> <20161201194114.GA8263@bhelgaas-glaptop.roam.corp.google.com> To: Duc Dang In-Reply-To: Cc: Bjorn Helgaas , Mark Salter , Rafael Wysocki , Lorenzo Pieralisi , Arnd Bergmann , linux-pci@vger.kernel.org, linux-arm , Linux Kernel Mailing List , Tomasz Nowicki , patches X-Mailer: Zimbra 8.0.6_GA_5922 (MobileSync - Apple-iPhone9C2/1402.100) Thread-Topic: PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller Thread-Index: BaOr/1TmeHVM9/0BSkxoL4XIC3hIWw== Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6625 Lines: 146 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 wrote: > >> On Thu, Dec 1, 2016 at 11:41 AM, Bjorn Helgaas 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.