From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1030199AbbCLNmY (ORCPT ); Thu, 12 Mar 2015 09:42:24 -0400 Received: from mail-la0-f49.google.com ([209.85.215.49]:34084 "EHLO mail-la0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754063AbbCLNmS (ORCPT ); Thu, 12 Mar 2015 09:42:18 -0400 Message-ID: <550197B1.8000405@linaro.org> Date: Thu, 12 Mar 2015 14:42:09 +0100 From: Tomasz Nowicki User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: Rob Herring CC: Bjorn Helgaas , Yijing Wang , Arnd Bergmann , Hanjun Guo , Liviu Dudau , Thomas Gleixner , Ingo Molnar , "H. Peter Anvin" , "Rafael J. Wysocki" , Al Stone , "linaro-acpi@lists.linaro.org" , "linux-pci@vger.kernel.org" , "x86@kernel.org" , "linux-kernel@vger.kernel.org" , "linux-acpi@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH v4 7/9] x86, pci, ecam: mmconfig_64.c becomes default implementation for ECAM driver. References: <1426083169-8698-1-git-send-email-tomasz.nowicki@linaro.org> <1426083169-8698-8-git-send-email-tomasz.nowicki@linaro.org> In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11.03.2015 16:37, Rob Herring wrote: > On Wed, Mar 11, 2015 at 9:12 AM, Tomasz Nowicki > wrote: >> Architectures which want to take advantage of ECAM generic goodness > > This is not necessarily an architecture decision. It is likely per host. Right, good point. > >> should select CONFIG_PCI_ECAM_GENERIC. Otherwise, like x86 32bits machines, >> are obligated to provide own low-level ECAM calls. >> >> Signed-off-by: Tomasz Nowicki >> --- > > [...] > >> diff --git a/drivers/pci/ecam.c b/drivers/pci/ecam.c >> index c588234..796b6e7 100644 >> --- a/drivers/pci/ecam.c >> +++ b/drivers/pci/ecam.c >> @@ -23,6 +23,119 @@ static DEFINE_MUTEX(pci_mmcfg_lock); >> >> LIST_HEAD(pci_mmcfg_list); >> >> +#ifdef CONFIG_GENERIC_PCI_ECAM >> +static char __iomem *pci_dev_base(unsigned int seg, unsigned int bus, >> + unsigned int devfn) >> +{ >> + struct pci_mmcfg_region *cfg = pci_mmconfig_lookup(seg, bus); >> + >> + if (cfg && cfg->virt) >> + return cfg->virt + (PCI_MMCFG_BUS_OFFSET(bus) | (devfn << 12)); >> + return NULL; >> +} >> + >> +int pci_mmcfg_read(unsigned int seg, unsigned int bus, >> + unsigned int devfn, int reg, int len, u32 *value) >> +{ >> + char __iomem *addr; >> + >> + /* Why do we have this when nobody checks it. How about a BUG()!? -AK */ >> + if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095))) { >> +err: *value = -1; >> + return -EINVAL; >> + } >> + >> + rcu_read_lock(); > > What is the purpose of the rcu lock other than the old implementation had it? Read/write calls consist on lookup RCU list (with MMCONFIG regions) and then corresponding operation. It is possible to hotplug another pci root bridge which leads to RCU list modification. > >> + addr = pci_dev_base(seg, bus, devfn); > > The .map_bus op provides the same function if you restructure to use > the generic accessors. As you noticed, pci_mmcfg_{read,write} and pci_generic_config_{read,write} prototypes are different. int pci_mmcfg_read(unsigned int seg, unsigned int bus, unsigned int devfn, int reg, int len, u32 *value); vs int pci_generic_config_read(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *val); This is because pci_mmcfg_{read,write} can be used before pci root bridge initialization (while we have no struct pci_bus *bus) inside of ACPICA code (osl.c --> acpi_os_read_pci_configuration()) For that reason, I decide to create ECAM related new accessors which do not depend on host bridge presence. In other words, pci_generic_config_{read,write} can be built on pci_mmcfg_{read,write} but not the other way around. In the light of above, I could not used .map_bus. I might not see a nicer way to solve that so any opinion/suggestion very appreciated :) > >> + if (!addr) { >> + rcu_read_unlock(); >> + goto err; >> + } >> + >> + *value = pci_mmio_read(len, addr + reg); >> + rcu_read_unlock(); >> + >> + return 0; >> +} >> + >> +int pci_mmcfg_write(unsigned int seg, unsigned int bus, >> + unsigned int devfn, int reg, int len, u32 value) >> +{ >> + char __iomem *addr; >> + >> + /* Why do we have this when nobody checks it. How about a BUG()!? -AK */ >> + if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095))) >> + return -EINVAL; >> + >> + rcu_read_lock(); >> + addr = pci_dev_base(seg, bus, devfn); >> + if (!addr) { >> + rcu_read_unlock(); >> + return -EINVAL; >> + } >> + >> + pci_mmio_write(len, addr + reg, value); >> + rcu_read_unlock(); >> + >> + return 0; >> +} >> + >> +static void __iomem *mcfg_ioremap(struct pci_mmcfg_region *cfg) >> +{ >> + void __iomem *addr; >> + u64 start, size; >> + int num_buses; >> + >> + start = cfg->address + PCI_MMCFG_BUS_OFFSET(cfg->start_bus); >> + num_buses = cfg->end_bus - cfg->start_bus + 1; >> + size = PCI_MMCFG_BUS_OFFSET(num_buses); >> + addr = ioremap_nocache(start, size); >> + if (addr) >> + addr -= PCI_MMCFG_BUS_OFFSET(cfg->start_bus); >> + return addr; >> +} >> + >> +int __init pci_mmcfg_arch_init(void) > > Where would this be called for the case of the generic host and using DT? > I focused on sharing the code in ACPI context and did not consider DT. I think we can improve that code as next steps. Tomasz