From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757319AbcKBUzS (ORCPT ); Wed, 2 Nov 2016 16:55:18 -0400 Received: from mail-vk0-f47.google.com ([209.85.213.47]:36470 "EHLO mail-vk0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756691AbcKBUzQ (ORCPT ); Wed, 2 Nov 2016 16:55:16 -0400 MIME-Version: 1.0 In-Reply-To: <20161102165329.GB10528@bhelgaas-glaptop.roam.corp.google.com> References: <20160921212258.GE20006@localhost> <20161102165329.GB10528@bhelgaas-glaptop.roam.corp.google.com> From: Duc Dang Date: Wed, 2 Nov 2016 13:54:44 -0700 Message-ID: Subject: Re: [RFC PATCH v2 1/1] PCI/ACPI: xgene: Add ECAM quirk for X-Gene PCIe controller To: Bjorn Helgaas Cc: Rafael Wysocki , Lorenzo Pieralisi , Arnd Bergmann , Mark Salter , linux-pci@vger.kernel.org, linux-arm , Linux Kernel Mailing List , Jon Masters , Tomasz Nowicki , patches Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Nov 2, 2016 at 9:53 AM, Bjorn Helgaas wrote: > > Hi Duc, > > On Tue, Oct 25, 2016 at 06:24:32PM -0700, Duc Dang wrote: > > PCIe controllers in X-Gene SoCs is not ECAM compliant: software > > needs to configure additional controller's register to address > > device at bus:dev:function. > > > > This patch depends on "ECAM quirks handling for ARM64 platforms" > > series (http://www.spinics.net/lists/arm-kernel/msg530692.html, > > the series was also modified by Bjorn) to address the limitation > > above for X-Gene PCIe controller. > > > > The quirk will only be applied for X-Gene PCIe MCFG table with > > OEM revison 1, 2, 3 or 4 (PCIe controller v1 and v2 on X-Gene SoCs). > > The quirks here contain some hard-coded address space consumed by > ECAM. The ECAM quirk itself is not a generic description of that > address space in the sense of a PCI BAR or an ACPI _CRS method, i.e., > the quirk description is not enough to keep other parts of the kernel > from treating the address space as "available". Your concern here is the controller register region that I declared as hard-coded array of resource (xgene_vx_csr_res) is not owned by the root bridge? So unlike DT case where kernel discovers and knows exactly who owns this resource (and we can also use platform_get_resource to get the resource); in ACPI case with ECAM quirk, kernel knows who requests this resource but does not know who owns it? Do I understand correctly? > > Can you add a note here in the changelog about how you are describing > this space generically? The standard solution is a PNP0C02 device > with _CRS that describes it. > > It would be ideal if you could open a bugzilla at bugzilla.kernel.org > and attach there a dmesg log, /proc/iomem contents, and DSDT. This > would show both the generic PNP0C02 piece and the ECAM quirk piece. We don't have PNP0C02 device in our ACPI Table. Do you want me add a PNP0C02 device into our firmware and check/document the difference in /proc/iomem output? > > BTW, I did refresh and re-push the pci/ecam-v6 branch where I'm > collecting this stuff, so if you want to rebase your patch on top of > that and test it, that would be great. Thanks, I will definitely do that. > > > Signed-off-by: Duc Dang > > --- > > v2 changes: > > 1. Get rid of pci-xgene-ecam.c file and fold quirk code into pci-xgene.c > > 2. Redefine fixup array for X-Gene > > 3. Use devm_ioremap_resource to map csr_base > > > > drivers/acpi/pci_mcfg.c | 30 ++++++++ > > drivers/pci/host/pci-xgene.c | 165 ++++++++++++++++++++++++++++++++++++++++++- > > include/linux/pci-ecam.h | 5 ++ > > 3 files changed, 197 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c > > index bb2c508..9dfc937 100644 > > --- a/drivers/acpi/pci_mcfg.c > > +++ b/drivers/acpi/pci_mcfg.c > > @@ -96,6 +96,36 @@ struct mcfg_fixup { > > THUNDER_ECAM_MCFG(2, 12), > > THUNDER_ECAM_MCFG(2, 13), > > #endif > > +#ifdef CONFIG_PCI_XGENE > > +#define XGENE_V1_ECAM_MCFG(rev, seg) \ > > + {"APM ", "XGENE ", rev, seg, MCFG_BUS_ANY, \ > > + &xgene_v1_pcie_ecam_ops } > > +#define XGENE_V2_1_ECAM_MCFG(rev, seg) \ > > + {"APM ", "XGENE ", rev, seg, MCFG_BUS_ANY, \ > > + &xgene_v2_1_pcie_ecam_ops } > > +#define XGENE_V2_2_ECAM_MCFG(rev, seg) \ > > + {"APM ", "XGENE ", rev, seg, MCFG_BUS_ANY, \ > > + &xgene_v2_2_pcie_ecam_ops } > > + > > + /* X-Gene SoC with v1 PCIe controller */ > > + XGENE_V1_ECAM_MCFG(1, 0), > > + XGENE_V1_ECAM_MCFG(1, 1), > > + XGENE_V1_ECAM_MCFG(1, 2), > > + XGENE_V1_ECAM_MCFG(1, 3), > > + XGENE_V1_ECAM_MCFG(1, 4), > > + XGENE_V1_ECAM_MCFG(2, 0), > > + XGENE_V1_ECAM_MCFG(2, 1), > > + XGENE_V1_ECAM_MCFG(2, 2), > > + XGENE_V1_ECAM_MCFG(2, 3), > > + XGENE_V1_ECAM_MCFG(2, 4), > > + /* X-Gene SoC with v2.1 PCIe controller */ > > + XGENE_V2_1_ECAM_MCFG(3, 0), > > + XGENE_V2_1_ECAM_MCFG(3, 1), > > + /* X-Gene SoC with v2.2 PCIe controller */ > > + XGENE_V2_2_ECAM_MCFG(4, 0), > > + XGENE_V2_2_ECAM_MCFG(4, 1), > > + XGENE_V2_2_ECAM_MCFG(4, 2), > > +#endif > > }; > > > > static char mcfg_oem_id[ACPI_OEM_ID_SIZE]; > > diff --git a/drivers/pci/host/pci-xgene.c b/drivers/pci/host/pci-xgene.c > > index 1de23d7..d6aa642 100644 > > --- a/drivers/pci/host/pci-xgene.c > > +++ b/drivers/pci/host/pci-xgene.c > > @@ -27,6 +27,8 @@ > > #include > > #include > > #include > > +#include > > +#include > > #include > > #include > > > > @@ -64,6 +66,7 @@ > > /* PCIe IP version */ > > #define XGENE_PCIE_IP_VER_UNKN 0 > > #define XGENE_PCIE_IP_VER_1 1 > > +#define XGENE_PCIE_IP_VER_2 2 > > > > struct xgene_pcie_port { > > struct device_node *node; > > @@ -97,7 +100,15 @@ static inline u32 pcie_bar_low_val(u32 addr, u32 flags) > > */ > > static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus) > > { > > - struct xgene_pcie_port *port = bus->sysdata; > > + struct pci_config_window *cfg; > > + struct xgene_pcie_port *port; > > + > > + if (acpi_disabled) > > + port = bus->sysdata; > > + else { > > + cfg = bus->sysdata; > > + port = cfg->priv; > > + } > > > > if (bus->number >= (bus->primary + 1)) > > return port->cfg_base + AXI_EP_CFG_ACCESS; > > @@ -111,10 +122,18 @@ static void __iomem *xgene_pcie_get_cfg_base(struct pci_bus *bus) > > */ > > static void xgene_pcie_set_rtdid_reg(struct pci_bus *bus, uint devfn) > > { > > - struct xgene_pcie_port *port = bus->sysdata; > > + struct pci_config_window *cfg; > > + struct xgene_pcie_port *port; > > unsigned int b, d, f; > > u32 rtdid_val = 0; > > > > + if (acpi_disabled) > > + port = bus->sysdata; > > + else { > > + cfg = bus->sysdata; > > + port = cfg->priv; > > + } > > + > > b = bus->number; > > d = PCI_SLOT(devfn); > > f = PCI_FUNC(devfn); > > @@ -158,7 +177,15 @@ static void __iomem *xgene_pcie_map_bus(struct pci_bus *bus, unsigned int devfn, > > static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn, > > int where, int size, u32 *val) > > { > > - struct xgene_pcie_port *port = bus->sysdata; > > + struct pci_config_window *cfg; > > + struct xgene_pcie_port *port; > > + > > + if (acpi_disabled) > > + port = bus->sysdata; > > + else { > > + cfg = bus->sysdata; > > + port = cfg->priv; > > + } > > > > if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) != > > PCIBIOS_SUCCESSFUL) > > @@ -189,6 +216,138 @@ static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn, > > .write = pci_generic_config_write32, > > }; > > > > +#ifdef CONFIG_ACPI > > +static struct resource xgene_v1_csr_res[] = { > > + [0] = DEFINE_RES_MEM(0x1f2b0000UL, SZ_64K), > > + [1] = DEFINE_RES_MEM(0x1f2c0000UL, SZ_64K), > > + [2] = DEFINE_RES_MEM(0x1f2d0000UL, SZ_64K), > > + [3] = DEFINE_RES_MEM(0x1f500000UL, SZ_64K), > > + [4] = DEFINE_RES_MEM(0x1f510000UL, SZ_64K), > > +}; > > + > > +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]; > > + port->csr_base = devm_ioremap_resource(dev, csr); > > + if (IS_ERR(port->csr_base)) { > > + kfree(port); > > + return -ENOMEM; > > + } > > + > > + port->cfg_base = cfg->win; > > + port->version = XGENE_PCIE_IP_VER_1; > > + > > + cfg->priv = port; > > + > > + return 0; > > +} > > + > > +struct pci_ecam_ops xgene_v1_pcie_ecam_ops = { > > + .bus_shift = 16, > > + .init = xgene_v1_pcie_ecam_init, > > + .pci_ops = { > > + .map_bus = xgene_pcie_map_bus, > > + .read = xgene_pcie_config_read32, > > + .write = pci_generic_config_write, > > + } > > +}; > > + > > +static struct resource xgene_v2_1_csr_res[] = { > > + [0] = DEFINE_RES_MEM(0x1f2b0000UL, SZ_64K), > > + [1] = DEFINE_RES_MEM(0x1f2c0000UL, SZ_64K), > > +}; > > + > > +static int xgene_v2_1_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_v2_1_csr_res[root->segment]; > > + port->csr_base = devm_ioremap_resource(dev, csr); > > + if (IS_ERR(port->csr_base)) { > > + kfree(port); > > + return -ENOMEM; > > + } > > + > > + port->cfg_base = cfg->win; > > + port->version = XGENE_PCIE_IP_VER_2; > > + > > + cfg->priv = port; > > + > > + return 0; > > +} > > + > > +struct pci_ecam_ops xgene_v2_1_pcie_ecam_ops = { > > + .bus_shift = 16, > > + .init = xgene_v2_1_pcie_ecam_init, > > + .pci_ops = { > > + .map_bus = xgene_pcie_map_bus, > > + .read = xgene_pcie_config_read32, > > + .write = pci_generic_config_write, > > + } > > +}; > > + > > +static struct resource xgene_v2_2_csr_res[] = { > > + [0] = DEFINE_RES_MEM(0x1f2b0000UL, SZ_64K), > > + [1] = DEFINE_RES_MEM(0x1f500000UL, SZ_64K), > > + [2] = DEFINE_RES_MEM(0x1f2d0000UL, SZ_64K), > > +}; > > + > > +static int xgene_v2_2_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_v2_2_csr_res[root->segment]; > > + port->csr_base = devm_ioremap_resource(dev, csr); > > + if (IS_ERR(port->csr_base)) { > > + kfree(port); > > + return -ENOMEM; > > + } > > + > > + port->cfg_base = cfg->win; > > + port->version = XGENE_PCIE_IP_VER_2; > > + > > + cfg->priv = port; > > + > > + return 0; > > +} > > + > > +struct pci_ecam_ops xgene_v2_2_pcie_ecam_ops = { > > + .bus_shift = 16, > > + .init = xgene_v2_2_pcie_ecam_init, > > + .pci_ops = { > > + .map_bus = xgene_pcie_map_bus, > > + .read = xgene_pcie_config_read32, > > + .write = pci_generic_config_write, > > + } > > +}; > > +#endif > > + > > static u64 xgene_pcie_set_ib_mask(struct xgene_pcie_port *port, u32 addr, > > u32 flags, u64 size) > > { > > diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h > > index 35f0e81..40da3e7 100644 > > --- a/include/linux/pci-ecam.h > > +++ b/include/linux/pci-ecam.h > > @@ -65,6 +65,11 @@ void __iomem *pci_ecam_map_bus(struct pci_bus *bus, unsigned int devfn, > > #ifdef CONFIG_PCI_HOST_THUNDER_ECAM > > extern struct pci_ecam_ops pci_thunder_ecam_ops; > > #endif > > +#ifdef CONFIG_PCI_XGENE > > +extern struct pci_ecam_ops xgene_v1_pcie_ecam_ops; > > +extern struct pci_ecam_ops xgene_v2_1_pcie_ecam_ops; > > +extern struct pci_ecam_ops xgene_v2_2_pcie_ecam_ops; > > +#endif > > > > #ifdef CONFIG_PCI_HOST_GENERIC > > /* for DT-based PCI controllers that support ECAM */ > > -- > > 1.9.1 > > Regards, Duc Dang.