From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com ([217.140.101.70]:57716 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750977AbcCBKke (ORCPT ); Wed, 2 Mar 2016 05:40:34 -0500 Date: Wed, 2 Mar 2016 10:42:44 +0000 From: Lorenzo Pieralisi To: Jayachandran Chandrashekaran Nair Cc: Tomasz Nowicki , Bjorn Helgaas , Arnd Bergmann , Will Deacon , Catalin Marinas , "Rafael J. Wysocki" , Hanjun Guo , okaya@codeaurora.org, jiang.liu@linux.intel.com, Stefano Stabellini , robert.richter@caviumnetworks.com, Marcin Wojtas , Liviu.Dudau@arm.com, David Daney , wangyijing@huawei.com, Suravee.Suthikulpanit@amd.com, msalter@redhat.com, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-acpi@vger.kernel.org, linux-kernel@vger.kernel.org, linaro-acpi@lists.linaro.org, Jayachandran Chandrashekaran Nair , Jon Masters Subject: Re: [PATCH V4 17/23] acpi, mcfg: Add default PCI config accessors implementation and initial support for related quirks. Message-ID: <20160302104244.GA25547@red-moon> References: <1454606941-9523-1-git-send-email-tn@semihalf.com> <1454606941-9523-18-git-send-email-tn@semihalf.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii In-Reply-To: Sender: linux-pci-owner@vger.kernel.org List-ID: On Mon, Feb 29, 2016 at 01:33:41PM +0530, Jayachandran Chandrashekaran Nair wrote: > On Thu, Feb 4, 2016 at 10:58 PM, Tomasz Nowicki wrote: > > We use generic accessors from access.c by default. However, we already > > know platforms that need special handling while accessing to PCI config > > space. These platforms will need different accessors set matched against > > platform ID, domain, bus touple. Therefore we are going to add (in future) > > DECLARE_ACPI_MCFG_FIXUP which will register platform specific custom > > accessors. For now, we let pci_mcfg_get_ops to take acpi_pci_root structure > > as an arguments and left some space for quirk matching algorithm. > > > > Signed-off-by: Tomasz Nowicki > > Tested-by: Duc Dang > > Tested-by: Dongdong Liu > > Tested-by: Hanjun Guo > > Tested-by: Graeme Gregory > > Tested-by: Sinan Kaya > > --- > > drivers/acpi/mcfg.c | 30 ++++++++++++++++++++++++++++++ > > include/linux/pci-acpi.h | 8 ++++++++ > > 2 files changed, 38 insertions(+) > > > > diff --git a/drivers/acpi/mcfg.c b/drivers/acpi/mcfg.c > > index dca4c4e..dfc2d14 100644 > > --- a/drivers/acpi/mcfg.c > > +++ b/drivers/acpi/mcfg.c > > @@ -34,6 +34,36 @@ int __weak raw_pci_write(unsigned int domain, unsigned int bus, > > return PCIBIOS_DEVICE_NOT_FOUND; > > } > > > > +void __iomem * > > +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset) > > +{ > > + struct pci_mmcfg_region *cfg; > > + > > + cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number); > > In the existing code, calls to pci_mmconfig_lookup() is done inside an > rcu_read_lock/rcu_read_unlock pair. Any reason that is not required here? > > Also, you can avoid having to do the lookup every time by saving a cfg pointer > see http://lists.infradead.org/pipermail/linux-arm-kernel/2016-January/396921.html Yes, this is much better (and sysdata usage is self-contained, it does not trickle into other bits of the kernel - ie arch code), it should be integrated in the final version of the patchset. > > + if (cfg && cfg->virt) > > + return cfg->virt + > > + (PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) + > > + offset; > > PCI_MMCFG_OFFSET(bus->number, devfn) would be better Ditto. Thanks, Lorenzo > > > + return NULL; > > +} > > + > > +/* Default generic PCI config accessors */ > > +static struct pci_ops default_pci_mcfg_ops = { > > + .map_bus = pci_mcfg_dev_base, > > + .read = pci_generic_config_read, > > + .write = pci_generic_config_write, > > +}; > > + > > +struct pci_ops *pci_mcfg_get_ops(struct acpi_pci_root *root) > > +{ > > + /* > > + * TODO: Match against platform specific quirks and return > > + * corresponding PCI config space accessor set. > > + */ > > + > > + return &default_pci_mcfg_ops; > > +} > > Is it necessary to make these non-static, even in the next patch, the > functions are only used from this file. > > > + > > int __init acpi_parse_mcfg(struct acpi_table_header *header) > > { > > struct acpi_table_mcfg *mcfg; > > diff --git a/include/linux/pci-acpi.h b/include/linux/pci-acpi.h > > index 65b91f3..c974586 100644 > > --- a/include/linux/pci-acpi.h > > +++ b/include/linux/pci-acpi.h > > @@ -83,10 +83,18 @@ void acpi_pci_remove_bus(struct pci_bus *bus); > > #ifdef CONFIG_PCI_MMCONFIG > > int pci_mmcfg_setup_map(struct acpi_pci_root_info *ci); > > void pci_mmcfg_teardown_map(struct acpi_pci_root_info *ci); > > +struct pci_ops *pci_mcfg_get_ops(struct acpi_pci_root *root); > > +void __iomem * > > +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset); > > #else > > static inline int pci_mmcfg_setup_map(struct acpi_pci_root_info *ci) > > { return 0; } > > static inline void pci_mmcfg_teardown_map(struct acpi_pci_root_info *ci) { } > > +static inline struct pci_ops *pci_mcfg_get_ops(struct acpi_pci_root *root) > > +{ return NULL; } > > +static inline void __iomem * > > +pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset) > > +{ return NULL; } > > #endif > > #ifdef CONFIG_ACPI_PCI_SLOT > > JC. >