diff mbox

[v4,7/9] x86, pci, ecam: mmconfig_64.c becomes default implementation for ECAM driver.

Message ID 1426083169-8698-8-git-send-email-tomasz.nowicki@linaro.org (mailing list archive)
State New, archived
Headers show

Commit Message

Tomasz Nowicki March 11, 2015, 2:12 p.m. UTC 
Architectures which want to take advantage of ECAM generic goodness
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 <tomasz.nowicki@linaro.org>
---
 arch/x86/Kconfig           |   1 +
 arch/x86/pci/Makefile      |   5 +-
 arch/x86/pci/mmconfig_64.c | 127 ---------------------------------------------
 drivers/pci/Kconfig        |   3 ++
 drivers/pci/ecam.c         | 113 ++++++++++++++++++++++++++++++++++++++++
 5 files changed, 121 insertions(+), 128 deletions(-)
 delete mode 100644 arch/x86/pci/mmconfig_64.c

Comments

Rob Herring March 11, 2015, 3:37 p.m. UTC | #1 
On Wed, Mar 11, 2015 at 9:12 AM, Tomasz Nowicki
<tomasz.nowicki@linaro.org> wrote:
> Architectures which want to take advantage of ECAM generic goodness

This is not necessarily an architecture decision. It is likely per host.

> 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 <tomasz.nowicki@linaro.org>
> ---

[...]

> 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?

> +       addr = pci_dev_base(seg, bus, devfn);

The .map_bus op provides the same function if you restructure to use
the generic accessors.

> +       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?

Rob
Brian Gerst March 11, 2015, 4:30 p.m. UTC | #2 
On Wed, Mar 11, 2015 at 10:12 AM, Tomasz Nowicki
<tomasz.nowicki@linaro.org> wrote:

> diff --git a/arch/x86/pci/Makefile b/arch/x86/pci/Makefile
> index 5c6fc35..35c765b 100644
> --- a/arch/x86/pci/Makefile
> +++ b/arch/x86/pci/Makefile
> @@ -1,7 +1,10 @@
>  obj-y                          := i386.o init.o
>
>  obj-$(CONFIG_PCI_BIOS)         += pcbios.o
> -obj-$(CONFIG_PCI_MMCONFIG)     += mmconfig_$(BITS).o direct.o mmconfig-shared.o
> +obj-$(CONFIG_PCI_MMCONFIG)     += direct.o mmconfig-shared.o
> +ifeq ($(BITS),32)
> +obj-$(CONFIG_PCI_MMCONFIG)     += mmconfig_32.o
> +endif
>  obj-$(CONFIG_PCI_DIRECT)       += direct.o
>  obj-$(CONFIG_PCI_OLPC)         += olpc.o
>  obj-$(CONFIG_PCI_XEN)          += xen.o

This would be better written as:

mmconfig-y := direct.o mmconfig-shared.o
mmconfig-$(CONFIG_X86_32) += mmconfig_32.o
obj-$(CONFIG_PCI_MMCONFIG) += $(mmconfig-y)

--
Brian Gerst
Tomasz Nowicki March 12, 2015, 1:42 p.m. UTC | #3 
On 11.03.2015 16:37, Rob Herring wrote:
> On Wed, Mar 11, 2015 at 9:12 AM, Tomasz Nowicki
> <tomasz.nowicki@linaro.org> 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 <tomasz.nowicki@linaro.org>
>> ---
>
> [...]
>
>> 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
Tomasz Nowicki March 12, 2015, 1:42 p.m. UTC | #4 
On 11.03.2015 17:30, Brian Gerst wrote:
> On Wed, Mar 11, 2015 at 10:12 AM, Tomasz Nowicki
> <tomasz.nowicki@linaro.org> wrote:
>
>> diff --git a/arch/x86/pci/Makefile b/arch/x86/pci/Makefile
>> index 5c6fc35..35c765b 100644
>> --- a/arch/x86/pci/Makefile
>> +++ b/arch/x86/pci/Makefile
>> @@ -1,7 +1,10 @@
>>   obj-y                          := i386.o init.o
>>
>>   obj-$(CONFIG_PCI_BIOS)         += pcbios.o
>> -obj-$(CONFIG_PCI_MMCONFIG)     += mmconfig_$(BITS).o direct.o mmconfig-shared.o
>> +obj-$(CONFIG_PCI_MMCONFIG)     += direct.o mmconfig-shared.o
>> +ifeq ($(BITS),32)
>> +obj-$(CONFIG_PCI_MMCONFIG)     += mmconfig_32.o
>> +endif
>>   obj-$(CONFIG_PCI_DIRECT)       += direct.o
>>   obj-$(CONFIG_PCI_OLPC)         += olpc.o
>>   obj-$(CONFIG_PCI_XEN)          += xen.o
>
> This would be better written as:
>
> mmconfig-y := direct.o mmconfig-shared.o
> mmconfig-$(CONFIG_X86_32) += mmconfig_32.o
> obj-$(CONFIG_PCI_MMCONFIG) += $(mmconfig-y)

Nice! Will update my patch.

Tomasz
diff mbox

Patch

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index aa451bf..3bf1aa5 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -142,6 +142,7 @@  config X86
 	select X86_FEATURE_NAMES if PROC_FS
 	select SRCU
 	select HAVE_PCI_ECAM
+	select GENERIC_PCI_ECAM if X86_64
 
 config INSTRUCTION_DECODER
 	def_bool y
diff --git a/arch/x86/pci/Makefile b/arch/x86/pci/Makefile
index 5c6fc35..35c765b 100644
--- a/arch/x86/pci/Makefile
+++ b/arch/x86/pci/Makefile
@@ -1,7 +1,10 @@ 
 obj-y				:= i386.o init.o
 
 obj-$(CONFIG_PCI_BIOS)		+= pcbios.o
-obj-$(CONFIG_PCI_MMCONFIG)	+= mmconfig_$(BITS).o direct.o mmconfig-shared.o
+obj-$(CONFIG_PCI_MMCONFIG)	+= direct.o mmconfig-shared.o
+ifeq ($(BITS),32)
+obj-$(CONFIG_PCI_MMCONFIG)	+= mmconfig_32.o
+endif
 obj-$(CONFIG_PCI_DIRECT)	+= direct.o
 obj-$(CONFIG_PCI_OLPC)		+= olpc.o
 obj-$(CONFIG_PCI_XEN)		+= xen.o
diff --git a/arch/x86/pci/mmconfig_64.c b/arch/x86/pci/mmconfig_64.c
deleted file mode 100644
index fd857ea..0000000
--- a/arch/x86/pci/mmconfig_64.c
+++ /dev/null
@@ -1,127 +0,0 @@ 
-/*
- * mmconfig.c - Low-level direct PCI config space access via MMCONFIG
- *
- * This is an 64bit optimized version that always keeps the full mmconfig
- * space mapped. This allows lockless config space operation.
- */
-
-#include <linux/pci.h>
-#include <linux/init.h>
-#include <linux/acpi.h>
-#include <linux/bitmap.h>
-#include <linux/rcupdate.h>
-#include <linux/ecam.h>
-#include <asm/e820.h>
-#include <asm/pci_x86.h>
-
-#define PREFIX "PCI: "
-
-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();
-	addr = pci_dev_base(seg, bus, devfn);
-	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)
-{
-	struct pci_mmcfg_region *cfg;
-
-	list_for_each_entry(cfg, &pci_mmcfg_list, list)
-		if (pci_mmcfg_arch_map(cfg)) {
-			pci_mmcfg_arch_free();
-			return 0;
-		}
-
-	return 1;
-}
-
-void __init pci_mmcfg_arch_free(void)
-{
-	struct pci_mmcfg_region *cfg;
-
-	list_for_each_entry(cfg, &pci_mmcfg_list, list)
-		pci_mmcfg_arch_unmap(cfg);
-}
-
-int pci_mmcfg_arch_map(struct pci_mmcfg_region *cfg)
-{
-	cfg->virt = mcfg_ioremap(cfg);
-	if (!cfg->virt) {
-		pr_err(PREFIX "can't map MMCONFIG at %pR\n", &cfg->res);
-		return -ENOMEM;
-	}
-
-	return 0;
-}
-
-void pci_mmcfg_arch_unmap(struct pci_mmcfg_region *cfg)
-{
-	if (cfg && cfg->virt) {
-		iounmap(cfg->virt + PCI_MMCFG_BUS_OFFSET(cfg->start_bus));
-		cfg->virt = NULL;
-	}
-}
diff --git a/drivers/pci/Kconfig b/drivers/pci/Kconfig
index 90a5fb9..fae4aa7 100644
--- a/drivers/pci/Kconfig
+++ b/drivers/pci/Kconfig
@@ -29,6 +29,9 @@  config PCI_ECAM
 config HAVE_PCI_ECAM
 	bool
 
+config GENERIC_PCI_ECAM
+	bool
+
 config PCI_DEBUG
 	bool "PCI Debugging"
 	depends on PCI && DEBUG_KERNEL
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();
+	addr = pci_dev_base(seg, bus, devfn);
+	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)
+{
+	struct pci_mmcfg_region *cfg;
+
+	list_for_each_entry(cfg, &pci_mmcfg_list, list)
+		if (pci_mmcfg_arch_map(cfg)) {
+			pci_mmcfg_arch_free();
+			return 0;
+		}
+
+	return 1;
+}
+
+void __init pci_mmcfg_arch_free(void)
+{
+	struct pci_mmcfg_region *cfg;
+
+	list_for_each_entry(cfg, &pci_mmcfg_list, list)
+		pci_mmcfg_arch_unmap(cfg);
+}
+
+int pci_mmcfg_arch_map(struct pci_mmcfg_region *cfg)
+{
+	cfg->virt = mcfg_ioremap(cfg);
+	if (!cfg->virt) {
+		pr_err(PREFIX "can't map MMCONFIG at %pR\n", &cfg->res);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+void pci_mmcfg_arch_unmap(struct pci_mmcfg_region *cfg)
+{
+	if (cfg && cfg->virt) {
+		iounmap(cfg->virt + PCI_MMCFG_BUS_OFFSET(cfg->start_bus));
+		cfg->virt = NULL;
+	}
+}
+#endif
+
 static u32
 pci_mmconfig_generic_read(int len, void __iomem *addr)
 {