[PATCH v7 0/3] Add support for PCI in AArch64

[PATCH v7 0/3] Add support for PCI in AArch64

[Liviu Dudau]

Hi,

This patch adds support for PCI to AArch64. It is based on my v7 patch
that adds support for creating generic host bridge structure from
device tree. With that in place, I was able to boot a platform that
has PCIe host bridge support and use a PCIe network card.

I have dropped the RFC tag from the subject as I now have the ambitious goal
of trying to get it mainlined.

Changes from v6:
  - Guard the pci_domain_nr() inline implementation with #ifdef CONFIG_PCI as
    to avoid conflict with default empty version present in include/linux/pci.h.
    Thanks to Jingoo Han for catching this.

Changes from v5:
  - Removed pcibios_fixup_bridge_ranges() as the week default version is fine.
  - Removed the ALIGN() call in pcibios_align_resource()
  - Stopped exporting pcibios_align_resource()

Changes from v4:
  - Fixed the pci_domain_nr() implementation for arm64. Now we use
    find_pci_host_bride() to find the host bridge before we retrieve
    the domain number.

Changes from v3:
  - Added Acks accumulated so far ;)
  - Still carrying Catalin's patch for moving the PCI_IO_BASE until it
    lands in linux-next or mainline, in order to ease applying the series

Changes from v2:
  - Implement an arch specific version of pci_register_io_range() and
    pci_address_to_pio().
  - Return 1 from pci_proc_domain().

Changes from v1:
  - Added Catalin's patch for moving the PCI_IO_BASE location and extend
    its size to 16MB
  - Integrated Arnd's version of pci_ioremap_io that uses a bitmap for
    keeping track of assigned IO space and returns an io_offset. At the
    moment the code is added in arch/arm64 but it can be moved in drivers/pci.
  - Added a fix for the generic ioport_map() function when !CONFIG_GENERIC_IOMAP
    as suggested by Arnd.

v6 thread here: https://lkml.org/lkml/2014/3/5/41
v5 thread here: https://lkml.org/lkml/2014/3/4/307
v4 thread here: https://lkml.org/lkml/2014/3/3/298
v3 thread here: https://lkml.org/lkml/2014/2/28/211
v2 thread here: https://lkml.org/lkml/2014/2/27/255
v1 thread here: https://lkml.org/lkml/2014/2/3/389


The API used is different from the one used by ARM architecture. There is
no pci_common_init_dev() function and no hw_pci structure, as that is no
longer needed. Once the last signature is added to the legal agreement, I
will post the host bridge driver code that I am using. Meanwhile, here
is an example of what the probe function looks like, posted as an example:

static int myhostbridge_probe(struct platform_device *pdev)
{
	int err;
	struct device_node *dev;
	struct pci_host_bridge *bridge;
	struct myhostbridge_port *pp;
	resource_size_t lastbus;

	dev = pdev->dev.of_node;

	if (!of_device_is_available(dev)) {
		pr_warn("%s: disabled\n", dev->full_name);
		return -ENODEV;
	}

	pp = kzalloc(sizeof(struct myhostbridge_port), GFP_KERNEL);
	if (!pp)
		return -ENOMEM;

	bridge = of_create_pci_host_bridge(&pdev->dev, &myhostbridge_ops, pp);
	if (IS_ERR(bridge)) {
		err = PTR_ERR(bridge);
		goto bridge_create_fail;
	}

	err = myhostbridge_setup(bridge->bus);
	if (err)
		goto bridge_setup_fail;

	/* We always enable PCI domains and we keep domain 0 backward
	 * compatible in /proc for video cards
	 */
	pci_add_flags(PCI_ENABLE_PROC_DOMAINS);
	pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC);

	lastbus = pci_scan_child_bus(bridge->bus);
	pci_bus_update_busn_res_end(bridge->bus, lastbus);

	pci_assign_unassigned_bus_resources(bridge->bus);

	pci_bus_add_devices(bridge->bus);

	return 0;

bridge_setup_fail:
	put_device(&bridge->dev);
	device_unregister(&bridge->dev);
bridge_create_fail:
	kfree(pp);
	return err;
}

Best regards,
Liviu


Catalin Marinas (1):
  arm64: Extend the PCI I/O space to 16MB

Liviu Dudau (2):
  Fix ioport_map() for !CONFIG_GENERIC_IOMAP cases.
  arm64: Add architecture support for PCI

 Documentation/arm64/memory.txt |  16 +--
 arch/arm64/Kconfig             |  19 +++-
 arch/arm64/include/asm/Kbuild  |   1 +
 arch/arm64/include/asm/io.h    |   5 +-
 arch/arm64/include/asm/pci.h   |  51 +++++++++
 arch/arm64/kernel/Makefile     |   1 +
 arch/arm64/kernel/pci.c        | 173 +++++++++++++++++++++++++++++++
 include/asm-generic/io.h       |   2 +-
 8 files changed, 258 insertions(+), 10 deletions(-)
 create mode 100644 arch/arm64/include/asm/pci.h
 create mode 100644 arch/arm64/kernel/pci.c

-- 
1.9.0

[PATCH v7 1/3] Fix ioport_map() for !CONFIG_GENERIC_IOMAP cases.

[Liviu Dudau]

The inline version of ioport_map() that gets used when !CONFIG_GENERIC_IOMAP
is wrong. It returns a mapped (i.e. virtual) address that can start from
zero and completely ignores the PCI_IOBASE and IO_SPACE_LIMIT that most
architectures that use !CONFIG_GENERIC_MAP define.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Tested-by: Tanmay Inamdar <tinamdar@apm.com>
---
 include/asm-generic/io.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index d5afe96..df72051 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -331,7 +331,7 @@ static inline void iounmap(void __iomem *addr)
 #ifndef CONFIG_GENERIC_IOMAP
 static inline void __iomem *ioport_map(unsigned long port, unsigned int nr)
 {
-	return (void __iomem *) port;
+	return (void __iomem *)(PCI_IOBASE + (port & IO_SPACE_LIMIT));
 }
 
 static inline void ioport_unmap(void __iomem *p)
-- 
1.9.0

[PATCH v7 2/3] arm64: Extend the PCI I/O space to 16MB

[Liviu Dudau]

From: Catalin Marinas <catalin.marinas@arm.com>

The patch moves the PCI I/O space (currently at 64K) before the
earlyprintk mapping and extends it to 16MB.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
---
 Documentation/arm64/memory.txt | 16 ++++++++++------
 arch/arm64/include/asm/io.h    |  2 +-
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/Documentation/arm64/memory.txt b/Documentation/arm64/memory.txt
index 5e054bf..85e24c4 100644
--- a/Documentation/arm64/memory.txt
+++ b/Documentation/arm64/memory.txt
@@ -35,11 +35,13 @@ ffffffbc00000000	ffffffbdffffffff	   8GB		vmemmap
 
 ffffffbe00000000	ffffffbffbbfffff	  ~8GB		[guard, future vmmemap]
 
-ffffffbffbc00000	ffffffbffbdfffff	   2MB		earlyprintk device
+ffffffbffa000000	ffffffbffaffffff	  16MB		PCI I/O space
+
+ffffffbffb000000	ffffffbffbbfffff	  12MB		[guard]
 
-ffffffbffbe00000	ffffffbffbe0ffff	  64KB		PCI I/O space
+ffffffbffbc00000	ffffffbffbdfffff	   2MB		earlyprintk device
 
-ffffffbffbe10000	ffffffbcffffffff	  ~2MB		[guard]
+ffffffbffbe00000	ffffffbffbffffff	   2MB		[guard]
 
 ffffffbffc000000	ffffffbfffffffff	  64MB		modules
 
@@ -60,11 +62,13 @@ fffffdfc00000000	fffffdfdffffffff	   8GB		vmemmap
 
 fffffdfe00000000	fffffdfffbbfffff	  ~8GB		[guard, future vmmemap]
 
-fffffdfffbc00000	fffffdfffbdfffff	   2MB		earlyprintk device
+fffffdfffa000000	fffffdfffaffffff	  16MB		PCI I/O space
+
+fffffdfffb000000	fffffdfffbbfffff	  12MB		[guard]
 
-fffffdfffbe00000	fffffdfffbe0ffff	  64KB		PCI I/O space
+fffffdfffbc00000	fffffdfffbdfffff	   2MB		earlyprintk device
 
-fffffdfffbe10000	fffffdfffbffffff	  ~2MB		[guard]
+fffffdfffbe00000	fffffdfffbffffff	   2MB		[guard]
 
 fffffdfffc000000	fffffdffffffffff	  64MB		modules
 
diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
index 4cc813e..7846a6b 100644
--- a/arch/arm64/include/asm/io.h
+++ b/arch/arm64/include/asm/io.h
@@ -121,7 +121,7 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
  *  I/O port access primitives.
  */
 #define IO_SPACE_LIMIT		0xffff
-#define PCI_IOBASE		((void __iomem *)(MODULES_VADDR - SZ_2M))
+#define PCI_IOBASE		((void __iomem *)(MODULES_VADDR - SZ_32M))
 
 static inline u8 inb(unsigned long addr)
 {
-- 
1.9.0

[PATCH v7 3/3] arm64: Add architecture support for PCI

[Liviu Dudau]

Use the generic host bridge functions to provide support for
PCI Express on arm64. There is no support for ISA memory.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Tested-by: Tanmay Inamdar <tinamdar@apm.com>
---
 arch/arm64/Kconfig            |  19 +++-
 arch/arm64/include/asm/Kbuild |   1 +
 arch/arm64/include/asm/io.h   |   3 +-
 arch/arm64/include/asm/pci.h  |  51 ++++++++++
 arch/arm64/kernel/Makefile    |   1 +
 arch/arm64/kernel/pci.c       | 173 ++++++++++++++++++++++++++++++++
 6 files changed, 246 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm64/include/asm/pci.h
 create mode 100644 arch/arm64/kernel/pci.c

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 27bbcfc..d1c8568 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -62,7 +62,7 @@ config MMU
 	def_bool y
 
 config NO_IOPORT
-	def_bool y
+	def_bool y if !PCI
 
 config STACKTRACE_SUPPORT
 	def_bool y
@@ -134,6 +134,23 @@ menu "Bus support"
 config ARM_AMBA
 	bool
 
+config PCI
+	bool "PCI support"
+	help
+	  This feature enables support for PCIe bus system. If you say Y
+	  here, the kernel will include drivers and infrastructure code
+	  to support PCIe bus devices.
+
+config PCI_DOMAINS
+	def_bool PCI
+
+config PCI_SYSCALL
+	def_bool PCI
+
+source "drivers/pci/Kconfig"
+source "drivers/pci/pcie/Kconfig"
+source "drivers/pci/hotplug/Kconfig"
+
 endmenu
 
 menu "Kernel Features"
diff --git a/arch/arm64/include/asm/Kbuild b/arch/arm64/include/asm/Kbuild
index 71c53ec..46924bc 100644
--- a/arch/arm64/include/asm/Kbuild
+++ b/arch/arm64/include/asm/Kbuild
@@ -26,6 +26,7 @@ generic-y += mman.h
 generic-y += msgbuf.h
 generic-y += mutex.h
 generic-y += pci.h
+generic-y += pci-bridge.h
 generic-y += poll.h
 generic-y += posix_types.h
 generic-y += resource.h
diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
index 7846a6b..67463a5 100644
--- a/arch/arm64/include/asm/io.h
+++ b/arch/arm64/include/asm/io.h
@@ -120,7 +120,8 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
 /*
  *  I/O port access primitives.
  */
-#define IO_SPACE_LIMIT		0xffff
+#define arch_has_dev_port()	(1)
+#define IO_SPACE_LIMIT		0x1ffffff
 #define PCI_IOBASE		((void __iomem *)(MODULES_VADDR - SZ_32M))
 
 static inline u8 inb(unsigned long addr)
diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
new file mode 100644
index 0000000..d93576f
--- /dev/null
+++ b/arch/arm64/include/asm/pci.h
@@ -0,0 +1,51 @@
+#ifndef __ASM_PCI_H
+#define __ASM_PCI_H
+#ifdef __KERNEL__
+
+#include <linux/types.h>
+#include <linux/slab.h>
+#include <linux/dma-mapping.h>
+
+#include <asm/io.h>
+#include <asm-generic/pci-bridge.h>
+#include <asm-generic/pci-dma-compat.h>
+
+#define PCIBIOS_MIN_IO		0x1000
+#define PCIBIOS_MIN_MEM		0
+
+struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus);
+
+/*
+ * Set to 1 if the kernel should re-assign all PCI bus numbers
+ */
+#define pcibios_assign_all_busses() \
+	(pci_has_flag(PCI_REASSIGN_ALL_BUS))
+
+/*
+ * PCI address space differs from physical memory address space
+ */
+#define PCI_DMA_BUS_IS_PHYS	(0)
+
+extern int isa_dma_bridge_buggy;
+
+#ifdef CONFIG_PCI
+static inline int pci_domain_nr(struct pci_bus *bus)
+{
+	struct pci_host_bridge *bridge = find_pci_host_bridge(bus);
+
+	if (bridge)
+		return bridge->domain_nr;
+
+	return 0;
+}
+
+static inline int pci_proc_domain(struct pci_bus *bus)
+{
+	return 1;
+}
+#endif
+
+extern unsigned long pci_ioremap_io(const struct resource *res, phys_addr_t phys_addr);
+
+#endif  /* __KERNEL__ */
+#endif  /* __ASM_PCI_H */
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 2d4554b..64fc479 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -20,6 +20,7 @@ arm64-obj-$(CONFIG_HAVE_HW_BREAKPOINT)+= hw_breakpoint.o
 arm64-obj-$(CONFIG_EARLY_PRINTK)	+= early_printk.o
 arm64-obj-$(CONFIG_ARM64_CPU_SUSPEND)	+= sleep.o suspend.o
 arm64-obj-$(CONFIG_JUMP_LABEL)		+= jump_label.o
+arm64-obj-$(CONFIG_PCI)			+= pci.o
 
 obj-y					+= $(arm64-obj-y) vdso/
 obj-m					+= $(arm64-obj-m)
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
new file mode 100644
index 0000000..9f29c9a
--- /dev/null
+++ b/arch/arm64/kernel/pci.c
@@ -0,0 +1,173 @@
+/*
+ * Code borrowed from powerpc/kernel/pci-common.c
+ *
+ * Copyright (C) 2003 Anton Blanchard <anton@au.ibm.com>, IBM
+ * Copyright (C) 2014 ARM Ltd.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ */
+
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mm.h>
+#include <linux/of_pci.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+
+#include <asm/pci-bridge.h>
+
+struct ioresource {
+	struct list_head list;
+	phys_addr_t start;
+	resource_size_t size;
+};
+
+static LIST_HEAD(io_list);
+
+int pci_register_io_range(phys_addr_t address, resource_size_t size)
+{
+	struct ioresource *res;
+	resource_size_t allocated_size = 0;
+
+	/* find if the range has not been already allocated */
+	list_for_each_entry(res, &io_list, list) {
+		if (address >= res->start &&
+			address + size <= res->start + size)
+			return 0;
+		allocated_size += res->size;
+	}
+
+	/* range not already registered, check for space */
+	if (allocated_size + size > IO_SPACE_LIMIT)
+		return -E2BIG;
+
+	/* add the range in the list */
+	res = kzalloc(sizeof(*res), GFP_KERNEL);
+	if (!res)
+		return -ENOMEM;
+	res->start = address;
+	res->size = size;
+
+	list_add_tail(&res->list, &io_list);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(pci_register_io_range);
+
+unsigned long pci_address_to_pio(phys_addr_t address)
+{
+	struct ioresource *res;
+
+	list_for_each_entry(res, &io_list, list) {
+		if (address >= res->start &&
+			address < res->start + res->size) {
+			return res->start - address;
+		}
+	}
+
+	return (unsigned long)-1;
+}
+EXPORT_SYMBOL_GPL(pci_address_to_pio);
+
+/*
+ * Called after each bus is probed, but before its children are examined
+ */
+void pcibios_fixup_bus(struct pci_bus *bus)
+{
+	struct pci_dev *dev;
+	struct resource *res;
+	int i;
+
+	if (!pci_is_root_bus(bus)) {
+		pci_read_bridge_bases(bus);
+
+		pci_bus_for_each_resource(bus, res, i) {
+			if (!res || !res->flags || res->parent)
+				continue;
+
+			/*
+			 * If we are going to reassign everything, we can
+			 * shrink the P2P resource to have zero size to
+			 * save space
+			 */
+			if (pci_has_flag(PCI_REASSIGN_ALL_RSRC)) {
+				res->flags |= IORESOURCE_UNSET;
+				res->start = 0;
+				res->end = -1;
+				continue;
+			}
+		}
+	}
+
+	list_for_each_entry(dev, &bus->devices, bus_list) {
+		/* Ignore fully discovered devices */
+		if (dev->is_added)
+			continue;
+
+		set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
+
+		/* Read default IRQs and fixup if necessary */
+		dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
+	}
+}
+EXPORT_SYMBOL(pcibios_fixup_bus);
+
+/*
+ * We don't have to worry about legacy ISA devices, so nothing to do here
+ */
+resource_size_t pcibios_align_resource(void *data, const struct resource *res,
+				resource_size_t size, resource_size_t align)
+{
+	return res->start;
+}
+
+int pcibios_enable_device(struct pci_dev *dev, int mask)
+{
+	return pci_enable_resources(dev, mask);
+}
+
+#define IO_SPACE_PAGES	((IO_SPACE_LIMIT + 1) / PAGE_SIZE)
+static DECLARE_BITMAP(pci_iospace, IO_SPACE_PAGES);
+
+unsigned long pci_ioremap_io(const struct resource *res, phys_addr_t phys_addr)
+{
+	unsigned long start, len, virt_start;
+	int err;
+
+	if (res->end > IO_SPACE_LIMIT)
+		return -EINVAL;
+
+	/*
+	 * try finding free space for the whole size first,
+	 * fall back to 64K if not available
+	 */
+	len = resource_size(res);
+	start = bitmap_find_next_zero_area(pci_iospace, IO_SPACE_PAGES,
+				res->start / PAGE_SIZE, len / PAGE_SIZE, 0);
+	if (start == IO_SPACE_PAGES && len > SZ_64K) {
+		len = SZ_64K;
+		start = 0;
+		start = bitmap_find_next_zero_area(pci_iospace, IO_SPACE_PAGES,
+					start, len / PAGE_SIZE, 0);
+	}
+
+	/* no 64K area found */
+	if (start == IO_SPACE_PAGES)
+		return -ENOMEM;
+
+	/* ioremap physical aperture to virtual aperture */
+	virt_start = start * PAGE_SIZE + (unsigned long)PCI_IOBASE;
+	err = ioremap_page_range(virt_start, virt_start + len,
+				phys_addr, __pgprot(PROT_DEVICE_nGnRE));
+	if (err)
+		return err;
+
+	bitmap_set(pci_iospace, start, len / PAGE_SIZE);
+
+	/* return io_offset */
+	return start * PAGE_SIZE - res->start;
+}
-- 
1.9.0

Re: [PATCH v7 3/3] arm64: Add architecture support for PCI

[Catalin Marinas]

On Fri, Mar 14, 2014 at 03:34:18PM +0000, Liviu Dudau wrote:
> --- /dev/null
> +++ b/arch/arm64/kernel/pci.c
[...]
> +int pci_register_io_range(phys_addr_t address, resource_size_t size)
[...]
> +unsigned long pci_address_to_pio(phys_addr_t address)
[...]
> +void pcibios_fixup_bus(struct pci_bus *bus)
[ actually most of this file ]

Maybe it was raised before already but can we have __weak generic
definitions of these functions? They don't seem to be arm64 specific in
any way.

-- 
Catalin

Re: [PATCH v7 3/3] arm64: Add architecture support for PCI

[Arnd Bergmann]

On Friday 14 March 2014, Catalin Marinas wrote:
> On Fri, Mar 14, 2014 at 03:34:18PM +0000, Liviu Dudau wrote:
> > --- /dev/null
> > +++ b/arch/arm64/kernel/pci.c
> [...]
> > +int pci_register_io_range(phys_addr_t address, resource_size_t size)
> [...]
> > +unsigned long pci_address_to_pio(phys_addr_t address)
> [...]
> > +void pcibios_fixup_bus(struct pci_bus *bus)
> [ actually most of this file ]
> 
> Maybe it was raised before already but can we have __weak generic
> definitions of these functions? They don't seem to be arm64 specific in
> any way.
> 

I would definitely prefer that.

	Arnd

Re: [PATCH v7 3/3] arm64: Add architecture support for PCI

[Liviu Dudau]

On Fri, Mar 14, 2014 at 05:38:08PM +0000, Arnd Bergmann wrote:
> On Friday 14 March 2014, Catalin Marinas wrote:
> > On Fri, Mar 14, 2014 at 03:34:18PM +0000, Liviu Dudau wrote:
> > > --- /dev/null
> > > +++ b/arch/arm64/kernel/pci.c
> > [...]
> > > +int pci_register_io_range(phys_addr_t address, resource_size_t size)
> > [...]
> > > +unsigned long pci_address_to_pio(phys_addr_t address)
> > [...]
> > > +void pcibios_fixup_bus(struct pci_bus *bus)
> > [ actually most of this file ]
> > 
> > Maybe it was raised before already but can we have __weak generic
> > definitions of these functions? They don't seem to be arm64 specific in
> > any way.
> > 
> 
> I would definitely prefer that.
> 
> 	Arnd
> 

I haven't seen any reaction from Bjorn on this, so I threaded carefully on that
subject. I'm new to this so I don't know how to handle this.

To my mind, and looking at the way every architecture has been setup, the pcibios_*
function are intended to be provided by the architecture. No matter how much wishful
thinking we are going to put in here, it will not change the fact that the non-arm64
specific version of pcibios_fixup_bus() that I wrote is not shared by anyone else
and it will remain "for arm64 use only" regardless to where it is placed until the
next architecture comes into the kernel. And even then its adoption is questionable.
If we are looking for simple and common implementations of this function, maybe we
should look at why microblaze and powerpc versions, that are identical, are not being
made the default __weak implementation.

As for the other two functions, I've no special attachment to where they are present
and I'm happy to move them into drivers/pci on the condition that the patchset doesn't
double in size. The reason why I'm weary of touching other architectures in a significant
way is the current lack of engineering bandwidth and way of testing all the architectures.
My low friction approach has been to introduce them in arm64 and then slowly move them
into core (and yes, I know about good intentions and the road to hell.)

Catalin, if you are happy to ask for ACKs from all arch maintainers that might get
affected by our custom version of pci_address_to_pio() before you can pull PCI support
for arm64 then I can propose a new patchset.

Best regards,
Liviu 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v7 3/3] arm64: Add architecture support for PCI

[Arnd Bergmann]

On Friday 14 March 2014, Liviu Dudau wrote:
> > 
> 
> I haven't seen any reaction from Bjorn on this, so I threaded carefully on that
> subject. I'm new to this so I don't know how to handle this.
> 
> To my mind, and looking at the way every architecture has been setup, the pcibios_*
> function are intended to be provided by the architecture.

That is definitely correct.

> No matter how much wishful
> thinking we are going to put in here, it will not change the fact that the non-arm64
> specific version of pcibios_fixup_bus() that I wrote is not shared by anyone else
> and it will remain "for arm64 use only" regardless to where it is placed until the
> next architecture comes into the kernel. And even then its adoption is questionable.

Agreed as well.

> If we are looking for simple and common implementations of this function, maybe we
> should look at why microblaze and powerpc versions, that are identical, are not being
> made the default __weak implementation.

Microblaze could most likely just be moved over to your version. The only
reason it is shared with the powerpc implementation is that they were
anticipating the code to become shared again and that it was known to
be working with flattened device tree.

> As for the other two functions, I've no special attachment to where they are present
> and I'm happy to move them into drivers/pci on the condition that the patchset doesn't
> double in size. The reason why I'm weary of touching other architectures in a significant
> way is the current lack of engineering bandwidth and way of testing all the architectures.
> My low friction approach has been to introduce them in arm64 and then slowly move them
> into core (and yes, I know about good intentions and the road to hell.)

I think everyone working on PCI is fed up with having arch-specific implementations
of all these, and Bjorn has been very supportive of generic infrastructure in the
past. Even just adding a generic infrastructure in a common place that is used
only by one architecture in my mind would be a significant improvement.

	Arnd

Re: [PATCH v7 3/3] arm64: Add architecture support for PCI

[Benjamin Herrenschmidt]

On Fri, 2014-03-14 at 20:10 +0100, Arnd Bergmann wrote:
> > As for the other two functions, I've no special attachment to where they are present
> > and I'm happy to move them into drivers/pci on the condition that the patchset doesn't
> > double in size. The reason why I'm weary of touching other architectures in a significant
> > way is the current lack of engineering bandwidth and way of testing all the architectures.
> > My low friction approach has been to introduce them in arm64 and then slowly move them
> > into core (and yes, I know about good intentions and the road to hell.)
> 
> I think everyone working on PCI is fed up with having arch-specific implementations
> of all these, and Bjorn has been very supportive of generic infrastructure in the
> past. Even just adding a generic infrastructure in a common place that is used
> only by one architecture in my mind would be a significant improvement.

I agree, it's a reasonable approach and microblaze which is simple and just "copied"
powerpc originally would be a good one to move over as well.

powerpc itself has many historical quirks and while I'm interested in a common
implementation, it will take me a bit of spare time to get through things and
figure out what can be done there and what "hooks" might still be necessary.

At this point, it's mostly a matter of:

 - I'm the one who knows the most about the powerpc PCI code as I wrote large
chunks of it

 - I'm very very very busy with some other things at the moment

So don't take my silence on these matters as a lack of interest, I think it's
definitely all going in the right direction, I just don't have much bandwidth
to consider the move of powerpc over just yet.

Cheers,
Ben.

Re: [PATCH v7 3/3] arm64: Add architecture support for PCI

[Rob Herring]

On Fri, Mar 14, 2014 at 10:34 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> Use the generic host bridge functions to provide support for
> PCI Express on arm64. There is no support for ISA memory.
>
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Tested-by: Tanmay Inamdar <tinamdar@apm.com>
> ---
>  arch/arm64/Kconfig            |  19 +++-
>  arch/arm64/include/asm/Kbuild |   1 +
>  arch/arm64/include/asm/io.h   |   3 +-
>  arch/arm64/include/asm/pci.h  |  51 ++++++++++
>  arch/arm64/kernel/Makefile    |   1 +
>  arch/arm64/kernel/pci.c       | 173 ++++++++++++++++++++++++++++++++
>  6 files changed, 246 insertions(+), 2 deletions(-)
>  create mode 100644 arch/arm64/include/asm/pci.h
>  create mode 100644 arch/arm64/kernel/pci.c

[snip]

> +#endif
> +
> +extern unsigned long pci_ioremap_io(const struct resource *res, phys_addr_t phys_addr);

Can we at least align the function definition across architectures if
not the implementation.


> +int pci_register_io_range(phys_addr_t address, resource_size_t size)
> +{
> +       struct ioresource *res;
> +       resource_size_t allocated_size = 0;
> +
> +       /* find if the range has not been already allocated */
> +       list_for_each_entry(res, &io_list, list) {
> +               if (address >= res->start &&
> +                       address + size <= res->start + size)
> +                       return 0;
> +               allocated_size += res->size;
> +       }
> +
> +       /* range not already registered, check for space */
> +       if (allocated_size + size > IO_SPACE_LIMIT)

I believe this needs to be "allocated_size + size - 1".

> +               return -E2BIG;
> +

Re: [PATCH v7 3/3] arm64: Add architecture support for PCI

[Liviu Dudau]

On Mon, Mar 17, 2014 at 04:05:38PM +0000, Rob Herring wrote:
> On Fri, Mar 14, 2014 at 10:34 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > Use the generic host bridge functions to provide support for
> > PCI Express on arm64. There is no support for ISA memory.
> >
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > Tested-by: Tanmay Inamdar <tinamdar@apm.com>
> > ---
> >  arch/arm64/Kconfig            |  19 +++-
> >  arch/arm64/include/asm/Kbuild |   1 +
> >  arch/arm64/include/asm/io.h   |   3 +-
> >  arch/arm64/include/asm/pci.h  |  51 ++++++++++
> >  arch/arm64/kernel/Makefile    |   1 +
> >  arch/arm64/kernel/pci.c       | 173 ++++++++++++++++++++++++++++++++
> >  6 files changed, 246 insertions(+), 2 deletions(-)
> >  create mode 100644 arch/arm64/include/asm/pci.h
> >  create mode 100644 arch/arm64/kernel/pci.c
> 
> [snip]
> 
> > +#endif
> > +
> > +extern unsigned long pci_ioremap_io(const struct resource *res, phys_addr_t phys_addr);
> 
> Can we at least align the function definition across architectures if
> not the implementation.

The choice of names is unfortunate. We are trying to follow the spirit of the
arch/arm function, not the implementation, and for RFC that served the purpose.

I'll come up with a better name as I don't intend to share the implementation
with arch/arm here.

Best regards,
Liviu

> 
> 
> > +int pci_register_io_range(phys_addr_t address, resource_size_t size)
> > +{
> > +       struct ioresource *res;
> > +       resource_size_t allocated_size = 0;
> > +
> > +       /* find if the range has not been already allocated */
> > +       list_for_each_entry(res, &io_list, list) {
> > +               if (address >= res->start &&
> > +                       address + size <= res->start + size)
> > +                       return 0;
> > +               allocated_size += res->size;
> > +       }
> > +
> > +       /* range not already registered, check for space */
> > +       if (allocated_size + size > IO_SPACE_LIMIT)
> 
> I believe this needs to be "allocated_size + size - 1".
> 
> > +               return -E2BIG;
> > +
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v7 3/3] arm64: Add architecture support for PCI

[Catalin Marinas]

On Fri, Mar 14, 2014 at 06:05:27PM +0000, Liviu Dudau wrote:
> On Fri, Mar 14, 2014 at 05:38:08PM +0000, Arnd Bergmann wrote:
> > On Friday 14 March 2014, Catalin Marinas wrote:
> > > On Fri, Mar 14, 2014 at 03:34:18PM +0000, Liviu Dudau wrote:
> > > > --- /dev/null
> > > > +++ b/arch/arm64/kernel/pci.c
> > > [...]
> > > > +int pci_register_io_range(phys_addr_t address, resource_size_t size)
> > > [...]
> > > > +unsigned long pci_address_to_pio(phys_addr_t address)
> > > [...]
> > > > +void pcibios_fixup_bus(struct pci_bus *bus)
> > > [ actually most of this file ]
> > > 
> > > Maybe it was raised before already but can we have __weak generic
> > > definitions of these functions? They don't seem to be arm64 specific in
> > > any way.
[...]
> Catalin, if you are happy to ask for ACKs from all arch maintainers that might get
> affected by our custom version of pci_address_to_pio() before you can pull PCI support
> for arm64 then I can propose a new patchset.

You don't need to change the other architectures, that's the point of a
__weak definition, it will be automatically overridden. If you want, you
can even place a GENERIC_PCI or whatever config option that is only
selected by arm64 for the time being.

-- 
Catalin

Re: [PATCH v7 3/3] arm64: Add architecture support for PCI

[Liviu Dudau]

On Mon, Mar 17, 2014 at 05:38:21PM +0000, Catalin Marinas wrote:
> On Fri, Mar 14, 2014 at 06:05:27PM +0000, Liviu Dudau wrote:
> > On Fri, Mar 14, 2014 at 05:38:08PM +0000, Arnd Bergmann wrote:
> > > On Friday 14 March 2014, Catalin Marinas wrote:
> > > > On Fri, Mar 14, 2014 at 03:34:18PM +0000, Liviu Dudau wrote:
> > > > > --- /dev/null
> > > > > +++ b/arch/arm64/kernel/pci.c
> > > > [...]
> > > > > +int pci_register_io_range(phys_addr_t address, resource_size_t size)
> > > > [...]
> > > > > +unsigned long pci_address_to_pio(phys_addr_t address)
> > > > [...]
> > > > > +void pcibios_fixup_bus(struct pci_bus *bus)
> > > > [ actually most of this file ]
> > > > 
> > > > Maybe it was raised before already but can we have __weak generic
> > > > definitions of these functions? They don't seem to be arm64 specific in
> > > > any way.
> [...]
> > Catalin, if you are happy to ask for ACKs from all arch maintainers that might get
> > affected by our custom version of pci_address_to_pio() before you can pull PCI support
> > for arm64 then I can propose a new patchset.
> 
> You don't need to change the other architectures, that's the point of a
> __weak definition, it will be automatically overridden. If you want, you
> can even place a GENERIC_PCI or whatever config option that is only
> selected by arm64 for the time being.

pci_address_to_pio() is alread __weak. My patch was adding the arm64 version of it. Adding
an #ifdef GENERIC_PCI to the __weak implementation is not just a temporary solution.
We would still have to keep some sort of #ifdef around in the function as x86
will never pickup our version. Regardless, the list of people that would need to ACK that
change will increase, right?

Best regards,
Liviu

> 
> -- 
> Catalin
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v7 3/3] arm64: Add architecture support for PCI

[Catalin Marinas]

On Mon, Mar 17, 2014 at 06:05:25PM +0000, Liviu Dudau wrote:
> On Mon, Mar 17, 2014 at 05:38:21PM +0000, Catalin Marinas wrote:
> > On Fri, Mar 14, 2014 at 06:05:27PM +0000, Liviu Dudau wrote:
> > > On Fri, Mar 14, 2014 at 05:38:08PM +0000, Arnd Bergmann wrote:
> > > > On Friday 14 March 2014, Catalin Marinas wrote:
> > > > > On Fri, Mar 14, 2014 at 03:34:18PM +0000, Liviu Dudau wrote:
> > > > > > --- /dev/null
> > > > > > +++ b/arch/arm64/kernel/pci.c
> > > > > [...]
> > > > > > +int pci_register_io_range(phys_addr_t address, resource_size_t size)
> > > > > [...]
> > > > > > +unsigned long pci_address_to_pio(phys_addr_t address)
> > > > > [...]
> > > > > > +void pcibios_fixup_bus(struct pci_bus *bus)
> > > > > [ actually most of this file ]
> > > > > 
> > > > > Maybe it was raised before already but can we have __weak generic
> > > > > definitions of these functions? They don't seem to be arm64 specific in
> > > > > any way.
> > [...]
> > > Catalin, if you are happy to ask for ACKs from all arch maintainers that might get
> > > affected by our custom version of pci_address_to_pio() before you can pull PCI support
> > > for arm64 then I can propose a new patchset.
> > 
> > You don't need to change the other architectures, that's the point of a
> > __weak definition, it will be automatically overridden. If you want, you
> > can even place a GENERIC_PCI or whatever config option that is only
> > selected by arm64 for the time being.
> 
> pci_address_to_pio() is alread __weak. My patch was adding the arm64 version of it. Adding
> an #ifdef GENERIC_PCI to the __weak implementation is not just a temporary solution.

Ah, I start to understand what you mean, pci_address_to_pio() is already
defined as __weak in drivers/of/address.c. So the reason we redefine it
on arm64 is that it uses the io_list resources which are populated by
pci_register_io_range(). Do you see any other architecture using a
similar logic (that could be shared)?

Any other functions in this file that could be shared (and are not
__weak already)?

-- 
Catalin

Re: [PATCH v7 3/3] arm64: Add architecture support for PCI

[Liviu Dudau]

On Wed, Mar 19, 2014 at 01:56:19PM +0000, Catalin Marinas wrote:
> On Mon, Mar 17, 2014 at 06:05:25PM +0000, Liviu Dudau wrote:
> > On Mon, Mar 17, 2014 at 05:38:21PM +0000, Catalin Marinas wrote:
> > > On Fri, Mar 14, 2014 at 06:05:27PM +0000, Liviu Dudau wrote:
> > > > On Fri, Mar 14, 2014 at 05:38:08PM +0000, Arnd Bergmann wrote:
> > > > > On Friday 14 March 2014, Catalin Marinas wrote:
> > > > > > On Fri, Mar 14, 2014 at 03:34:18PM +0000, Liviu Dudau wrote:
> > > > > > > --- /dev/null
> > > > > > > +++ b/arch/arm64/kernel/pci.c
> > > > > > [...]
> > > > > > > +int pci_register_io_range(phys_addr_t address, resource_size_t size)
> > > > > > [...]
> > > > > > > +unsigned long pci_address_to_pio(phys_addr_t address)
> > > > > > [...]
> > > > > > > +void pcibios_fixup_bus(struct pci_bus *bus)
> > > > > > [ actually most of this file ]
> > > > > > 
> > > > > > Maybe it was raised before already but can we have __weak generic
> > > > > > definitions of these functions? They don't seem to be arm64 specific in
> > > > > > any way.
> > > [...]
> > > > Catalin, if you are happy to ask for ACKs from all arch maintainers that might get
> > > > affected by our custom version of pci_address_to_pio() before you can pull PCI support
> > > > for arm64 then I can propose a new patchset.
> > > 
> > > You don't need to change the other architectures, that's the point of a
> > > __weak definition, it will be automatically overridden. If you want, you
> > > can even place a GENERIC_PCI or whatever config option that is only
> > > selected by arm64 for the time being.
> > 
> > pci_address_to_pio() is alread __weak. My patch was adding the arm64 version of it. Adding
> > an #ifdef GENERIC_PCI to the __weak implementation is not just a temporary solution.
> 
> Ah, I start to understand what you mean, pci_address_to_pio() is already
> defined as __weak in drivers/of/address.c. So the reason we redefine it
> on arm64 is that it uses the io_list resources which are populated by
> pci_register_io_range(). Do you see any other architecture using a
> similar logic (that could be shared)?

All architectures that memory map the PCI IO range should be supported by my version of
pci_address_to_pio(). But that still leaves the x86 and those architectures that have
separate IO space or map it 1:1 into CPU address space to carry a different version (which
the current "generic" weak version catters for).

> 
> Any other functions in this file that could be shared (and are not
> __weak already)?

A version of the pci_register_io_range() that uses part of pci_ioremap_io() (the calculation
of io_offset part).

My ultimate point is that no matter how long we argue about the shape of the functions that
I've added into arch/arm64/kernel/pci.c I don't think we can get away without having that
file, or at least not in the first phase if we want speedy integration into mainline.

Best regards,
Liviu

> 
> -- 
> Catalin

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v7 3/3] arm64: Add architecture support for PCI

[Rob Herring]

On Wed, Mar 19, 2014 at 12:21 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> On Wed, Mar 19, 2014 at 01:56:19PM +0000, Catalin Marinas wrote:
>> On Mon, Mar 17, 2014 at 06:05:25PM +0000, Liviu Dudau wrote:
>> > On Mon, Mar 17, 2014 at 05:38:21PM +0000, Catalin Marinas wrote:
>> > > On Fri, Mar 14, 2014 at 06:05:27PM +0000, Liviu Dudau wrote:
>> > > > On Fri, Mar 14, 2014 at 05:38:08PM +0000, Arnd Bergmann wrote:
>> > > > > On Friday 14 March 2014, Catalin Marinas wrote:
>> > > > > > On Fri, Mar 14, 2014 at 03:34:18PM +0000, Liviu Dudau wrote:
>> > > > > > > --- /dev/null
>> > > > > > > +++ b/arch/arm64/kernel/pci.c
>> > > > > > [...]
>> > > > > > > +int pci_register_io_range(phys_addr_t address, resource_size_t size)
>> > > > > > [...]
>> > > > > > > +unsigned long pci_address_to_pio(phys_addr_t address)
>> > > > > > [...]
>> > > > > > > +void pcibios_fixup_bus(struct pci_bus *bus)
>> > > > > > [ actually most of this file ]
>> > > > > >
>> > > > > > Maybe it was raised before already but can we have __weak generic
>> > > > > > definitions of these functions? They don't seem to be arm64 specific in
>> > > > > > any way.
>> > > [...]
>> > > > Catalin, if you are happy to ask for ACKs from all arch maintainers that might get
>> > > > affected by our custom version of pci_address_to_pio() before you can pull PCI support
>> > > > for arm64 then I can propose a new patchset.
>> > >
>> > > You don't need to change the other architectures, that's the point of a
>> > > __weak definition, it will be automatically overridden. If you want, you
>> > > can even place a GENERIC_PCI or whatever config option that is only
>> > > selected by arm64 for the time being.
>> >
>> > pci_address_to_pio() is alread __weak. My patch was adding the arm64 version of it. Adding
>> > an #ifdef GENERIC_PCI to the __weak implementation is not just a temporary solution.
>>
>> Ah, I start to understand what you mean, pci_address_to_pio() is already
>> defined as __weak in drivers/of/address.c. So the reason we redefine it
>> on arm64 is that it uses the io_list resources which are populated by
>> pci_register_io_range(). Do you see any other architecture using a
>> similar logic (that could be shared)?
>
> All architectures that memory map the PCI IO range should be supported by my version of
> pci_address_to_pio(). But that still leaves the x86 and those architectures that have
> separate IO space or map it 1:1 into CPU address space to carry a different version (which
> the current "generic" weak version catters for).
>
>>
>> Any other functions in this file that could be shared (and are not
>> __weak already)?
>
> A version of the pci_register_io_range() that uses part of pci_ioremap_io() (the calculation
> of io_offset part).
>
> My ultimate point is that no matter how long we argue about the shape of the functions that
> I've added into arch/arm64/kernel/pci.c I don't think we can get away without having that
> file, or at least not in the first phase if we want speedy integration into mainline.

"Speedy integration into mainline" is another way to say "leave it for
others to deal with and clean up later."

I've successfully used the preceeding series on ARM converting the
Versatile PCI support to use it. The main part of the process was just
copying this arm64 code to arm which tells me the code is in the wrong
place. There are still some differences in pcibios_fixup_bus, and it
is not clear to me whether the arm version should be doing all the
things the arm64 version does and vice-versa. Certainly that should be
sorted out. The other issues I see is pci_ioremap_io still has to be
called by the driver and is not aligned at least for the prototype as
I pointed out. Seems all these issues could be fixed with a simple
CONFIG_ARCH_HAS_MMIO_IOSPACE kconfig option.

Rob

Re: [PATCH v7 3/3] arm64: Add architecture support for PCI

[Arnd Bergmann]

On Wednesday 19 March 2014 12:53:11 Rob Herring wrote:
> I've successfully used the preceeding series on ARM converting the
> Versatile PCI support to use it. The main part of the process was just
> copying this arm64 code to arm which tells me the code is in the wrong
> place. There are still some differences in pcibios_fixup_bus, and it
> is not clear to me whether the arm version should be doing all the
> things the arm64 version does and vice-versa. Certainly that should be
> sorted out. The other issues I see is pci_ioremap_io still has to be
> called by the driver and is not aligned at least for the prototype as
> I pointed out. Seems all these issues could be fixed with a simple
> CONFIG_ARCH_HAS_MMIO_IOSPACE kconfig option.

I don't even think we need a CONFIG option: Any architecture that wants
to provide pci_ioremap_io (or whatever we end up calling the new version)
needs to pass the virtual base address to the common code, and the
easiest way to do that is using a macro  (e.g. PCI_IO_VIRT_BASE).

What I think we should end up with is a generic implementation like

/* 
 * architectures with special needs may provide their own version,
 * but most should be able to use this one.
 */
unsigned long __weak pci_ioremap_iospace(struct resource *bus, unsigned long cpu)
{
#ifdef PCI_IO_VIRT_BASE
	/* find free space, ioremap the bus address, return the offset */
	...
#endif

	/* this architecture doesn't have memory mapped I/O space,
	   so this function should never be called */
	WARN_ON(1);
	return -ENODEV;
}

	Arnd

Re: [PATCH v7 3/3] arm64: Add architecture support for PCI

[Arnd Bergmann]

On Wednesday 19 March 2014 17:21:41 Liviu Dudau wrote:
> 
> My ultimate point is that no matter how long we argue about the shape of the functions that
> I've added into arch/arm64/kernel/pci.c I don't think we can get away without having that
> file, or at least not in the first phase if we want speedy integration into mainline.

Let me simplify the discussion here:

NAK to adding yet another architecture specific implementation.

	Arnd

Re: [PATCH v7 3/3] arm64: Add architecture support for PCI

[Liviu Dudau]

On Wed, Mar 19, 2014 at 06:37:51PM +0000, Arnd Bergmann wrote:
> On Wednesday 19 March 2014 17:21:41 Liviu Dudau wrote:
> > 
> > My ultimate point is that no matter how long we argue about the shape of the functions that
> > I've added into arch/arm64/kernel/pci.c I don't think we can get away without having that
> > file, or at least not in the first phase if we want speedy integration into mainline.
> 
> Let me simplify the discussion here:
> 
> NAK to adding yet another architecture specific implementation.

So what would be your approach for handling pci_address_to_pio() in a non-arch specific way?

unsigned long __weak pci_address_to_pio(phys_addr_t address)
{
#ifdef ARCH_HAS_IOSPACE
	if (address > IO_SPACE_LIMIT)
		return (unsigned long)-1;

	return (unsigned long) address;
#else
	struct ioresource *res;

	list_for_each_entry(res, &io_list, list) {
		if (address >= res->start &&
			address < res->start + res->size) {
			return res->start - address;
		}
	}

	return (unsigned long)-1;
#endif
}


Either that, or you have more magic rabbits than me.

Best regards,
Liviu

> 
> 	Arnd
> 
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v7 3/3] arm64: Add architecture support for PCI

[Arnd Bergmann]

On Thursday 20 March 2014, Liviu Dudau wrote:
> On Wed, Mar 19, 2014 at 06:37:51PM +0000, Arnd Bergmann wrote:
> > On Wednesday 19 March 2014 17:21:41 Liviu Dudau wrote:
> > > 
> > > My ultimate point is that no matter how long we argue about the shape of the functions that
> > > I've added into arch/arm64/kernel/pci.c I don't think we can get away without having that
> > > file, or at least not in the first phase if we want speedy integration into mainline.
> > 
> > Let me simplify the discussion here:
> > 
> > NAK to adding yet another architecture specific implementation.
> 
> So what would be your approach for handling pci_address_to_pio() in a non-arch specific way?
> 
> unsigned long __weak pci_address_to_pio(phys_addr_t address)
> {
> #ifdef ARCH_HAS_IOSPACE
>         if (address > IO_SPACE_LIMIT)
>                 return (unsigned long)-1;
> 
>         return (unsigned long) address;
> #else
>         struct ioresource *res;
> 
>         list_for_each_entry(res, &io_list, list) {
>                 if (address >= res->start &&
>                         address < res->start + res->size) {
>                         return res->start - address;
>                 }
>         }
> 
>         return (unsigned long)-1;
> #endif
> }
> 
> 
> Either that, or you have more magic rabbits than me.

I don't even understand why you want to create a generic pci_address_to_pio
implementation, when we don't need that for arm64 at all. Unless I'm
missing something important, that function is only called in case of
PCI_PROBE_DEVTREE with pci_of_scan on PowerPC. I don't expect any
architecture to do the same thing, and the only other architecture that
needs something like it (sparc) has a different implementation.

The regular (non-DEVTREE) PCI bus scan should just be able to translate
the BUS I/O addresses into Linux I/O port numbers using io_offset,
without going through an intermediate step of translating into a CPU
physical address first, so the entire lookup method won't get used.

The reason why PowerPC needs it is that they traditionally don't
use PCI config space access to assign or look at resources, but
instead rely on the firmware to have set it up in advance and then
put matching information into DT and the BARs. For Solaris and AIX,
it's probably easier to use the information from DT, but in Linux,
we already need to implement the manual bus scan, e.g. to do
PCI device hotplugging if nothing else.

	Arnd

Re: [PATCH v7 3/3] arm64: Add architecture support for PCI

[Liviu Dudau]

On Thu, Mar 20, 2014 at 11:17:22AM +0000, Arnd Bergmann wrote:
> On Thursday 20 March 2014, Liviu Dudau wrote:
> > On Wed, Mar 19, 2014 at 06:37:51PM +0000, Arnd Bergmann wrote:
> > > On Wednesday 19 March 2014 17:21:41 Liviu Dudau wrote:
> > > > 
> > > > My ultimate point is that no matter how long we argue about the shape of the functions that
> > > > I've added into arch/arm64/kernel/pci.c I don't think we can get away without having that
> > > > file, or at least not in the first phase if we want speedy integration into mainline.
> > > 
> > > Let me simplify the discussion here:
> > > 
> > > NAK to adding yet another architecture specific implementation.
> > 
> > So what would be your approach for handling pci_address_to_pio() in a non-arch specific way?
> > 
> > unsigned long __weak pci_address_to_pio(phys_addr_t address)
> > {
> > #ifdef ARCH_HAS_IOSPACE
> >         if (address > IO_SPACE_LIMIT)
> >                 return (unsigned long)-1;
> > 
> >         return (unsigned long) address;
> > #else
> >         struct ioresource *res;
> > 
> >         list_for_each_entry(res, &io_list, list) {
> >                 if (address >= res->start &&
> >                         address < res->start + res->size) {
> >                         return res->start - address;
> >                 }
> >         }
> > 
> >         return (unsigned long)-1;
> > #endif
> > }
> > 
> > 
> > Either that, or you have more magic rabbits than me.
> 
> I don't even understand why you want to create a generic pci_address_to_pio
> implementation, when we don't need that for arm64 at all. Unless I'm
> missing something important, that function is only called in case of
> PCI_PROBE_DEVTREE with pci_of_scan on PowerPC. I don't expect any
> architecture to do the same thing, and the only other architecture that
> needs something like it (sparc) has a different implementation.

Because in my [v7 2/6]* patch for the generic host bridge support I start
using pci_address_to_pio to fix the conversion of PCI ranges to resources.
That requires an arm64 (or more correctly, an arch with memory mapped IO
specific) version of pci_address_to_pio().

Best regards,
Liviu

> 
> The regular (non-DEVTREE) PCI bus scan should just be able to translate
> the BUS I/O addresses into Linux I/O port numbers using io_offset,
> without going through an intermediate step of translating into a CPU
> physical address first, so the entire lookup method won't get used.
> 
> The reason why PowerPC needs it is that they traditionally don't
> use PCI config space access to assign or look at resources, but
> instead rely on the firmware to have set it up in advance and then
> put matching information into DT and the BARs. For Solaris and AIX,
> it's probably easier to use the information from DT, but in Linux,
> we already need to implement the manual bus scan, e.g. to do
> PCI device hotplugging if nothing else.
> 
> 	Arnd
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v7 3/3] arm64: Add architecture support for PCI

[Arnd Bergmann]

On Thursday 20 March 2014, Liviu Dudau wrote:
> > I don't even understand why you want to create a generic pci_address_to_pio
> > implementation, when we don't need that for arm64 at all. Unless I'm
> > missing something important, that function is only called in case of
> > PCI_PROBE_DEVTREE with pci_of_scan on PowerPC. I don't expect any
> > architecture to do the same thing, and the only other architecture that
> > needs something like it (sparc) has a different implementation.
> 
> Because in my [v7 2/6]* patch for the generic host bridge support I start
> using pci_address_to_pio to fix the conversion of PCI ranges to resources.
> That requires an arm64 (or more correctly, an arch with memory mapped IO
> specific) version of pci_address_to_pio().

Yes, but why do you use it there?

	Arnd

Re: [PATCH v7 3/3] arm64: Add architecture support for PCI

[Liviu Dudau]

On Thu, Mar 20, 2014 at 12:26:02PM +0000, Arnd Bergmann wrote:
> On Thursday 20 March 2014, Liviu Dudau wrote:
> > > I don't even understand why you want to create a generic pci_address_to_pio
> > > implementation, when we don't need that for arm64 at all. Unless I'm
> > > missing something important, that function is only called in case of
> > > PCI_PROBE_DEVTREE with pci_of_scan on PowerPC. I don't expect any
> > > architecture to do the same thing, and the only other architecture that
> > > needs something like it (sparc) has a different implementation.
> > 
> > Because in my [v7 2/6]* patch for the generic host bridge support I start
> > using pci_address_to_pio to fix the conversion of PCI ranges to resources.
> > That requires an arm64 (or more correctly, an arch with memory mapped IO
> > specific) version of pci_address_to_pio().
> 
> Yes, but why do you use it there?

I'm parsing the device tree and have reached the PCI IO range for the host bridge.
There is no host bridge created yet for that and hence no mapping. I need to
take the DT range declaration that gives me   CPU start .. SIZE --> BUS start and
convert it into   Logical IO port start .. Logical IO port end. While I could've
come up with (yet) another function that does the conversion from CPU address to
port IO address, I thought it would be nice to use the existing pci_address_to_pio()
function and adapt it to my needs. It has a __weak implementation anyway so I
could overwrite it in the architecture code.

To me it looked like a good plan and we had some discussions around my initial
goofy mistake of using pci_addr as physical address for translation. I never
thought it was considered a bad idea.

Best regards,
Liviu

> 
> 	Arnd
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v7 3/3] arm64: Add architecture support for PCI

[Bjorn Helgaas]

On Fri, Mar 14, 2014 at 03:34:18PM +0000, Liviu Dudau wrote:
> Use the generic host bridge functions to provide support for
> PCI Express on arm64. There is no support for ISA memory.
> 
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Tested-by: Tanmay Inamdar <tinamdar@apm.com>
> ---
>  arch/arm64/Kconfig            |  19 +++-
>  arch/arm64/include/asm/Kbuild |   1 +
>  arch/arm64/include/asm/io.h   |   3 +-
>  arch/arm64/include/asm/pci.h  |  51 ++++++++++
>  arch/arm64/kernel/Makefile    |   1 +
>  arch/arm64/kernel/pci.c       | 173 ++++++++++++++++++++++++++++++++
>  6 files changed, 246 insertions(+), 2 deletions(-)
>  create mode 100644 arch/arm64/include/asm/pci.h
>  create mode 100644 arch/arm64/kernel/pci.c
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 27bbcfc..d1c8568 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -62,7 +62,7 @@ config MMU
>  	def_bool y
>  
>  config NO_IOPORT
> -	def_bool y
> +	def_bool y if !PCI
>  
>  config STACKTRACE_SUPPORT
>  	def_bool y
> @@ -134,6 +134,23 @@ menu "Bus support"
>  config ARM_AMBA
>  	bool
>  
> +config PCI
> +	bool "PCI support"
> +	help
> +	  This feature enables support for PCIe bus system. If you say Y
> +	  here, the kernel will include drivers and infrastructure code
> +	  to support PCIe bus devices.
> +
> +config PCI_DOMAINS
> +	def_bool PCI
> +
> +config PCI_SYSCALL
> +	def_bool PCI
> +
> +source "drivers/pci/Kconfig"
> +source "drivers/pci/pcie/Kconfig"
> +source "drivers/pci/hotplug/Kconfig"
> +
>  endmenu
>  
>  menu "Kernel Features"
> diff --git a/arch/arm64/include/asm/Kbuild b/arch/arm64/include/asm/Kbuild
> index 71c53ec..46924bc 100644
> --- a/arch/arm64/include/asm/Kbuild
> +++ b/arch/arm64/include/asm/Kbuild
> @@ -26,6 +26,7 @@ generic-y += mman.h
>  generic-y += msgbuf.h
>  generic-y += mutex.h
>  generic-y += pci.h
> +generic-y += pci-bridge.h
>  generic-y += poll.h
>  generic-y += posix_types.h
>  generic-y += resource.h
> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
> index 7846a6b..67463a5 100644
> --- a/arch/arm64/include/asm/io.h
> +++ b/arch/arm64/include/asm/io.h
> @@ -120,7 +120,8 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
>  /*
>   *  I/O port access primitives.
>   */
> -#define IO_SPACE_LIMIT		0xffff
> +#define arch_has_dev_port()	(1)
> +#define IO_SPACE_LIMIT		0x1ffffff
>  #define PCI_IOBASE		((void __iomem *)(MODULES_VADDR - SZ_32M))
>  
>  static inline u8 inb(unsigned long addr)
> diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
> new file mode 100644
> index 0000000..d93576f
> --- /dev/null
> +++ b/arch/arm64/include/asm/pci.h
> @@ -0,0 +1,51 @@
> +#ifndef __ASM_PCI_H
> +#define __ASM_PCI_H
> +#ifdef __KERNEL__
> +
> +#include <linux/types.h>
> +#include <linux/slab.h>
> +#include <linux/dma-mapping.h>
> +
> +#include <asm/io.h>
> +#include <asm-generic/pci-bridge.h>
> +#include <asm-generic/pci-dma-compat.h>
> +
> +#define PCIBIOS_MIN_IO		0x1000
> +#define PCIBIOS_MIN_MEM		0
> +
> +struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus);
> +
> +/*
> + * Set to 1 if the kernel should re-assign all PCI bus numbers
> + */
> +#define pcibios_assign_all_busses() \
> +	(pci_has_flag(PCI_REASSIGN_ALL_BUS))
> +
> +/*
> + * PCI address space differs from physical memory address space
> + */
> +#define PCI_DMA_BUS_IS_PHYS	(0)
> +
> +extern int isa_dma_bridge_buggy;
> +
> +#ifdef CONFIG_PCI
> +static inline int pci_domain_nr(struct pci_bus *bus)
> +{
> +	struct pci_host_bridge *bridge = find_pci_host_bridge(bus);
> +
> +	if (bridge)
> +		return bridge->domain_nr;
> +
> +	return 0;
> +}
> +
> +static inline int pci_proc_domain(struct pci_bus *bus)
> +{
> +	return 1;
> +}
> +#endif
> +
> +extern unsigned long pci_ioremap_io(const struct resource *res, phys_addr_t phys_addr);
> +
> +#endif  /* __KERNEL__ */
> +#endif  /* __ASM_PCI_H */
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index 2d4554b..64fc479 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -20,6 +20,7 @@ arm64-obj-$(CONFIG_HAVE_HW_BREAKPOINT)+= hw_breakpoint.o
>  arm64-obj-$(CONFIG_EARLY_PRINTK)	+= early_printk.o
>  arm64-obj-$(CONFIG_ARM64_CPU_SUSPEND)	+= sleep.o suspend.o
>  arm64-obj-$(CONFIG_JUMP_LABEL)		+= jump_label.o
> +arm64-obj-$(CONFIG_PCI)			+= pci.o
>  
>  obj-y					+= $(arm64-obj-y) vdso/
>  obj-m					+= $(arm64-obj-m)
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> new file mode 100644
> index 0000000..9f29c9a
> --- /dev/null
> +++ b/arch/arm64/kernel/pci.c
> @@ -0,0 +1,173 @@
> +/*
> + * Code borrowed from powerpc/kernel/pci-common.c
> + *
> + * Copyright (C) 2003 Anton Blanchard <anton@au.ibm.com>, IBM
> + * Copyright (C) 2014 ARM Ltd.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + */
> +
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/mm.h>
> +#include <linux/of_pci.h>
> +#include <linux/of_platform.h>
> +#include <linux/slab.h>
> +
> +#include <asm/pci-bridge.h>
> +
> +struct ioresource {
> +	struct list_head list;
> +	phys_addr_t start;
> +	resource_size_t size;
> +};
> +
> +static LIST_HEAD(io_list);
> +
> +int pci_register_io_range(phys_addr_t address, resource_size_t size)
> +{
> +	struct ioresource *res;
> +	resource_size_t allocated_size = 0;
> +
> +	/* find if the range has not been already allocated */
> +	list_for_each_entry(res, &io_list, list) {
> +		if (address >= res->start &&
> +			address + size <= res->start + size)
> +			return 0;
> +		allocated_size += res->size;
> +	}
> +
> +	/* range not already registered, check for space */
> +	if (allocated_size + size > IO_SPACE_LIMIT)
> +		return -E2BIG;
> +
> +	/* add the range in the list */
> +	res = kzalloc(sizeof(*res), GFP_KERNEL);
> +	if (!res)
> +		return -ENOMEM;
> +	res->start = address;
> +	res->size = size;
> +
> +	list_add_tail(&res->list, &io_list);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(pci_register_io_range);
> +
> +unsigned long pci_address_to_pio(phys_addr_t address)
> +{
> +	struct ioresource *res;
> +
> +	list_for_each_entry(res, &io_list, list) {
> +		if (address >= res->start &&
> +			address < res->start + res->size) {
> +			return res->start - address;
> +		}
> +	}
> +
> +	return (unsigned long)-1;
> +}
> +EXPORT_SYMBOL_GPL(pci_address_to_pio);
> +
> +/*
> + * Called after each bus is probed, but before its children are examined
> + */
> +void pcibios_fixup_bus(struct pci_bus *bus)
> +{
> +	struct pci_dev *dev;
> +	struct resource *res;
> +	int i;
> +
> +	if (!pci_is_root_bus(bus)) {
> +		pci_read_bridge_bases(bus);
> +
> +		pci_bus_for_each_resource(bus, res, i) {
> +			if (!res || !res->flags || res->parent)
> +				continue;
> +
> +			/*
> +			 * If we are going to reassign everything, we can
> +			 * shrink the P2P resource to have zero size to
> +			 * save space
> +			 */
> +			if (pci_has_flag(PCI_REASSIGN_ALL_RSRC)) {
> +				res->flags |= IORESOURCE_UNSET;
> +				res->start = 0;
> +				res->end = -1;
> +				continue;
> +			}
> +		}
> +	}

Is there any other place we can put this?  I'm trying to nuke
pcibios_fixup_bus() because things like hotplug work better if we can do
them in a per-device way rather than a per-bus way.

> +
> +	list_for_each_entry(dev, &bus->devices, bus_list) {
> +		/* Ignore fully discovered devices */
> +		if (dev->is_added)
> +			continue;
> +
> +		set_dev_node(&dev->dev, pcibus_to_node(dev->bus));

Do you really need this?  pci_device_add() already contains the same line.

> +
> +		/* Read default IRQs and fixup if necessary */
> +		dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);

Could this be done in pcibios_add_device() or someplace similar?

> +	}
> +}
> +EXPORT_SYMBOL(pcibios_fixup_bus);
> +
> +/*
> + * We don't have to worry about legacy ISA devices, so nothing to do here
> + */
> +resource_size_t pcibios_align_resource(void *data, const struct resource *res,
> +				resource_size_t size, resource_size_t align)
> +{
> +	return res->start;
> +}
> +
> +int pcibios_enable_device(struct pci_dev *dev, int mask)
> +{
> +	return pci_enable_resources(dev, mask);
> +}

There is already a weak default implementation of pcibios_enable_device()
that does this, so you should be able to just drop this.

> +#define IO_SPACE_PAGES	((IO_SPACE_LIMIT + 1) / PAGE_SIZE)
> +static DECLARE_BITMAP(pci_iospace, IO_SPACE_PAGES);
> +
> +unsigned long pci_ioremap_io(const struct resource *res, phys_addr_t phys_addr)
> +{
> +	unsigned long start, len, virt_start;
> +	int err;
> +
> +	if (res->end > IO_SPACE_LIMIT)
> +		return -EINVAL;
> +
> +	/*
> +	 * try finding free space for the whole size first,
> +	 * fall back to 64K if not available
> +	 */
> +	len = resource_size(res);
> +	start = bitmap_find_next_zero_area(pci_iospace, IO_SPACE_PAGES,
> +				res->start / PAGE_SIZE, len / PAGE_SIZE, 0);
> +	if (start == IO_SPACE_PAGES && len > SZ_64K) {
> +		len = SZ_64K;
> +		start = 0;
> +		start = bitmap_find_next_zero_area(pci_iospace, IO_SPACE_PAGES,
> +					start, len / PAGE_SIZE, 0);
> +	}
> +
> +	/* no 64K area found */
> +	if (start == IO_SPACE_PAGES)
> +		return -ENOMEM;
> +
> +	/* ioremap physical aperture to virtual aperture */
> +	virt_start = start * PAGE_SIZE + (unsigned long)PCI_IOBASE;
> +	err = ioremap_page_range(virt_start, virt_start + len,
> +				phys_addr, __pgprot(PROT_DEVICE_nGnRE));
> +	if (err)
> +		return err;
> +
> +	bitmap_set(pci_iospace, start, len / PAGE_SIZE);
> +
> +	/* return io_offset */
> +	return start * PAGE_SIZE - res->start;
> +}
> -- 
> 1.9.0
> 

Re: [PATCH v7 3/3] arm64: Add architecture support for PCI

[Liviu Dudau]

On Tue, Apr 08, 2014 at 12:58:30AM +0100, Bjorn Helgaas wrote:
> On Fri, Mar 14, 2014 at 03:34:18PM +0000, Liviu Dudau wrote:
> > Use the generic host bridge functions to provide support for
> > PCI Express on arm64. There is no support for ISA memory.
> >
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > Tested-by: Tanmay Inamdar <tinamdar@apm.com>
> > ---
> >  arch/arm64/Kconfig            |  19 +++-
> >  arch/arm64/include/asm/Kbuild |   1 +
> >  arch/arm64/include/asm/io.h   |   3 +-
> >  arch/arm64/include/asm/pci.h  |  51 ++++++++++
> >  arch/arm64/kernel/Makefile    |   1 +
> >  arch/arm64/kernel/pci.c       | 173 ++++++++++++++++++++++++++++++++
> >  6 files changed, 246 insertions(+), 2 deletions(-)
> >  create mode 100644 arch/arm64/include/asm/pci.h
> >  create mode 100644 arch/arm64/kernel/pci.c
> >
> > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> > index 27bbcfc..d1c8568 100644
> > --- a/arch/arm64/Kconfig
> > +++ b/arch/arm64/Kconfig
> > @@ -62,7 +62,7 @@ config MMU
> >       def_bool y
> >
> >  config NO_IOPORT
> > -     def_bool y
> > +     def_bool y if !PCI
> >
> >  config STACKTRACE_SUPPORT
> >       def_bool y
> > @@ -134,6 +134,23 @@ menu "Bus support"
> >  config ARM_AMBA
> >       bool
> >
> > +config PCI
> > +     bool "PCI support"
> > +     help
> > +       This feature enables support for PCIe bus system. If you say Y
> > +       here, the kernel will include drivers and infrastructure code
> > +       to support PCIe bus devices.
> > +
> > +config PCI_DOMAINS
> > +     def_bool PCI
> > +
> > +config PCI_SYSCALL
> > +     def_bool PCI
> > +
> > +source "drivers/pci/Kconfig"
> > +source "drivers/pci/pcie/Kconfig"
> > +source "drivers/pci/hotplug/Kconfig"
> > +
> >  endmenu
> >
> >  menu "Kernel Features"
> > diff --git a/arch/arm64/include/asm/Kbuild b/arch/arm64/include/asm/Kbuild
> > index 71c53ec..46924bc 100644
> > --- a/arch/arm64/include/asm/Kbuild
> > +++ b/arch/arm64/include/asm/Kbuild
> > @@ -26,6 +26,7 @@ generic-y += mman.h
> >  generic-y += msgbuf.h
> >  generic-y += mutex.h
> >  generic-y += pci.h
> > +generic-y += pci-bridge.h
> >  generic-y += poll.h
> >  generic-y += posix_types.h
> >  generic-y += resource.h
> > diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
> > index 7846a6b..67463a5 100644
> > --- a/arch/arm64/include/asm/io.h
> > +++ b/arch/arm64/include/asm/io.h
> > @@ -120,7 +120,8 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
> >  /*
> >   *  I/O port access primitives.
> >   */
> > -#define IO_SPACE_LIMIT               0xffff
> > +#define arch_has_dev_port()  (1)
> > +#define IO_SPACE_LIMIT               0x1ffffff
> >  #define PCI_IOBASE           ((void __iomem *)(MODULES_VADDR - SZ_32M))
> >
> >  static inline u8 inb(unsigned long addr)
> > diff --git a/arch/arm64/include/asm/pci.h b/arch/arm64/include/asm/pci.h
> > new file mode 100644
> > index 0000000..d93576f
> > --- /dev/null
> > +++ b/arch/arm64/include/asm/pci.h
> > @@ -0,0 +1,51 @@
> > +#ifndef __ASM_PCI_H
> > +#define __ASM_PCI_H
> > +#ifdef __KERNEL__
> > +
> > +#include <linux/types.h>
> > +#include <linux/slab.h>
> > +#include <linux/dma-mapping.h>
> > +
> > +#include <asm/io.h>
> > +#include <asm-generic/pci-bridge.h>
> > +#include <asm-generic/pci-dma-compat.h>
> > +
> > +#define PCIBIOS_MIN_IO               0x1000
> > +#define PCIBIOS_MIN_MEM              0
> > +
> > +struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus);
> > +
> > +/*
> > + * Set to 1 if the kernel should re-assign all PCI bus numbers
> > + */
> > +#define pcibios_assign_all_busses() \
> > +     (pci_has_flag(PCI_REASSIGN_ALL_BUS))
> > +
> > +/*
> > + * PCI address space differs from physical memory address space
> > + */
> > +#define PCI_DMA_BUS_IS_PHYS  (0)
> > +
> > +extern int isa_dma_bridge_buggy;
> > +
> > +#ifdef CONFIG_PCI
> > +static inline int pci_domain_nr(struct pci_bus *bus)
> > +{
> > +     struct pci_host_bridge *bridge = find_pci_host_bridge(bus);
> > +
> > +     if (bridge)
> > +             return bridge->domain_nr;
> > +
> > +     return 0;
> > +}
> > +
> > +static inline int pci_proc_domain(struct pci_bus *bus)
> > +{
> > +     return 1;
> > +}
> > +#endif
> > +
> > +extern unsigned long pci_ioremap_io(const struct resource *res, phys_addr_t phys_addr);
> > +
> > +#endif  /* __KERNEL__ */
> > +#endif  /* __ASM_PCI_H */
> > diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> > index 2d4554b..64fc479 100644
> > --- a/arch/arm64/kernel/Makefile
> > +++ b/arch/arm64/kernel/Makefile
> > @@ -20,6 +20,7 @@ arm64-obj-$(CONFIG_HAVE_HW_BREAKPOINT)+= hw_breakpoint.o
> >  arm64-obj-$(CONFIG_EARLY_PRINTK)     += early_printk.o
> >  arm64-obj-$(CONFIG_ARM64_CPU_SUSPEND)        += sleep.o suspend.o
> >  arm64-obj-$(CONFIG_JUMP_LABEL)               += jump_label.o
> > +arm64-obj-$(CONFIG_PCI)                      += pci.o
> >
> >  obj-y                                        += $(arm64-obj-y) vdso/
> >  obj-m                                        += $(arm64-obj-m)
> > diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> > new file mode 100644
> > index 0000000..9f29c9a
> > --- /dev/null
> > +++ b/arch/arm64/kernel/pci.c
> > @@ -0,0 +1,173 @@
> > +/*
> > + * Code borrowed from powerpc/kernel/pci-common.c
> > + *
> > + * Copyright (C) 2003 Anton Blanchard <anton@au.ibm.com>, IBM
> > + * Copyright (C) 2014 ARM Ltd.
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License
> > + * version 2 as published by the Free Software Foundation.
> > + *
> > + */
> > +
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/kernel.h>
> > +#include <linux/mm.h>
> > +#include <linux/of_pci.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/slab.h>
> > +
> > +#include <asm/pci-bridge.h>
> > +
> > +struct ioresource {
> > +     struct list_head list;
> > +     phys_addr_t start;
> > +     resource_size_t size;
> > +};
> > +
> > +static LIST_HEAD(io_list);
> > +
> > +int pci_register_io_range(phys_addr_t address, resource_size_t size)
> > +{
> > +     struct ioresource *res;
> > +     resource_size_t allocated_size = 0;
> > +
> > +     /* find if the range has not been already allocated */
> > +     list_for_each_entry(res, &io_list, list) {
> > +             if (address >= res->start &&
> > +                     address + size <= res->start + size)
> > +                     return 0;
> > +             allocated_size += res->size;
> > +     }
> > +
> > +     /* range not already registered, check for space */
> > +     if (allocated_size + size > IO_SPACE_LIMIT)
> > +             return -E2BIG;
> > +
> > +     /* add the range in the list */
> > +     res = kzalloc(sizeof(*res), GFP_KERNEL);
> > +     if (!res)
> > +             return -ENOMEM;
> > +     res->start = address;
> > +     res->size = size;
> > +
> > +     list_add_tail(&res->list, &io_list);
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_register_io_range);
> > +
> > +unsigned long pci_address_to_pio(phys_addr_t address)
> > +{
> > +     struct ioresource *res;
> > +
> > +     list_for_each_entry(res, &io_list, list) {
> > +             if (address >= res->start &&
> > +                     address < res->start + res->size) {
> > +                     return res->start - address;
> > +             }
> > +     }
> > +
> > +     return (unsigned long)-1;
> > +}
> > +EXPORT_SYMBOL_GPL(pci_address_to_pio);
> > +
> > +/*
> > + * Called after each bus is probed, but before its children are examined
> > + */
> > +void pcibios_fixup_bus(struct pci_bus *bus)
> > +{
> > +     struct pci_dev *dev;
> > +     struct resource *res;
> > +     int i;
> > +
> > +     if (!pci_is_root_bus(bus)) {
> > +             pci_read_bridge_bases(bus);
> > +
> > +             pci_bus_for_each_resource(bus, res, i) {
> > +                     if (!res || !res->flags || res->parent)
> > +                             continue;
> > +
> > +                     /*
> > +                      * If we are going to reassign everything, we can
> > +                      * shrink the P2P resource to have zero size to
> > +                      * save space
> > +                      */
> > +                     if (pci_has_flag(PCI_REASSIGN_ALL_RSRC)) {
> > +                             res->flags |= IORESOURCE_UNSET;
> > +                             res->start = 0;
> > +                             res->end = -1;
> > +                             continue;
> > +                     }
> > +             }
> > +     }
> 
> Is there any other place we can put this?  I'm trying to nuke
> pcibios_fixup_bus() because things like hotplug work better if we can do
> them in a per-device way rather than a per-bus way.

That's good. I'm all for removing this one, will give it a go.

> 
> > +
> > +     list_for_each_entry(dev, &bus->devices, bus_list) {
> > +             /* Ignore fully discovered devices */
> > +             if (dev->is_added)
> > +                     continue;
> > +
> > +             set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
> 
> Do you really need this?  pci_device_add() already contains the same line.

OK.

> 
> > +
> > +             /* Read default IRQs and fixup if necessary */
> > +             dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
> 
> Could this be done in pcibios_add_device() or someplace similar?

Will do.

> 
> > +     }
> > +}
> > +EXPORT_SYMBOL(pcibios_fixup_bus);
> > +
> > +/*
> > + * We don't have to worry about legacy ISA devices, so nothing to do here
> > + */
> > +resource_size_t pcibios_align_resource(void *data, const struct resource *res,
> > +                             resource_size_t size, resource_size_t align)
> > +{
> > +     return res->start;
> > +}
> > +
> > +int pcibios_enable_device(struct pci_dev *dev, int mask)
> > +{
> > +     return pci_enable_resources(dev, mask);
> > +}
> 
> There is already a weak default implementation of pcibios_enable_device()
> that does this, so you should be able to just drop this.

OK.

Thanks for reviewing the series!

Liviu

> 
> > +#define IO_SPACE_PAGES       ((IO_SPACE_LIMIT + 1) / PAGE_SIZE)
> > +static DECLARE_BITMAP(pci_iospace, IO_SPACE_PAGES);
> > +
> > +unsigned long pci_ioremap_io(const struct resource *res, phys_addr_t phys_addr)
> > +{
> > +     unsigned long start, len, virt_start;
> > +     int err;
> > +
> > +     if (res->end > IO_SPACE_LIMIT)
> > +             return -EINVAL;
> > +
> > +     /*
> > +      * try finding free space for the whole size first,
> > +      * fall back to 64K if not available
> > +      */
> > +     len = resource_size(res);
> > +     start = bitmap_find_next_zero_area(pci_iospace, IO_SPACE_PAGES,
> > +                             res->start / PAGE_SIZE, len / PAGE_SIZE, 0);
> > +     if (start == IO_SPACE_PAGES && len > SZ_64K) {
> > +             len = SZ_64K;
> > +             start = 0;
> > +             start = bitmap_find_next_zero_area(pci_iospace, IO_SPACE_PAGES,
> > +                                     start, len / PAGE_SIZE, 0);
> > +     }
> > +
> > +     /* no 64K area found */
> > +     if (start == IO_SPACE_PAGES)
> > +             return -ENOMEM;
> > +
> > +     /* ioremap physical aperture to virtual aperture */
> > +     virt_start = start * PAGE_SIZE + (unsigned long)PCI_IOBASE;
> > +     err = ioremap_page_range(virt_start, virt_start + len,
> > +                             phys_addr, __pgprot(PROT_DEVICE_nGnRE));
> > +     if (err)
> > +             return err;
> > +
> > +     bitmap_set(pci_iospace, start, len / PAGE_SIZE);
> > +
> > +     /* return io_offset */
> > +     return start * PAGE_SIZE - res->start;
> > +}
> > --
> > 1.9.0
> >
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v7 0/3] Add support for PCI in AArch64

[Sandeepa Prabhu]

On 14 March 2014 21:04, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> Hi,
>
> This patch adds support for PCI to AArch64. It is based on my v7 patch
> that adds support for creating generic host bridge structure from
> device tree. With that in place, I was able to boot a platform that
> has PCIe host bridge support and use a PCIe network card.
Hi Liviu,

Are these patches (including your other patchset for device tree host
bridge support ) available on a public git repo I can checkout from?

Thanks,
~Sandeepa

>
> I have dropped the RFC tag from the subject as I now have the ambitious goal
> of trying to get it mainlined.
>
> Changes from v6:
>   - Guard the pci_domain_nr() inline implementation with #ifdef CONFIG_PCI as
>     to avoid conflict with default empty version present in include/linux/pci.h.
>     Thanks to Jingoo Han for catching this.
>
> Changes from v5:
>   - Removed pcibios_fixup_bridge_ranges() as the week default version is fine.
>   - Removed the ALIGN() call in pcibios_align_resource()
>   - Stopped exporting pcibios_align_resource()
>
> Changes from v4:
>   - Fixed the pci_domain_nr() implementation for arm64. Now we use
>     find_pci_host_bride() to find the host bridge before we retrieve
>     the domain number.
>
> Changes from v3:
>   - Added Acks accumulated so far ;)
>   - Still carrying Catalin's patch for moving the PCI_IO_BASE until it
>     lands in linux-next or mainline, in order to ease applying the series
>
> Changes from v2:
>   - Implement an arch specific version of pci_register_io_range() and
>     pci_address_to_pio().
>   - Return 1 from pci_proc_domain().
>
> Changes from v1:
>   - Added Catalin's patch for moving the PCI_IO_BASE location and extend
>     its size to 16MB
>   - Integrated Arnd's version of pci_ioremap_io that uses a bitmap for
>     keeping track of assigned IO space and returns an io_offset. At the
>     moment the code is added in arch/arm64 but it can be moved in drivers/pci.
>   - Added a fix for the generic ioport_map() function when !CONFIG_GENERIC_IOMAP
>     as suggested by Arnd.
>
> v6 thread here: https://lkml.org/lkml/2014/3/5/41
> v5 thread here: https://lkml.org/lkml/2014/3/4/307
> v4 thread here: https://lkml.org/lkml/2014/3/3/298
> v3 thread here: https://lkml.org/lkml/2014/2/28/211
> v2 thread here: https://lkml.org/lkml/2014/2/27/255
> v1 thread here: https://lkml.org/lkml/2014/2/3/389
>
>
> The API used is different from the one used by ARM architecture. There is
> no pci_common_init_dev() function and no hw_pci structure, as that is no
> longer needed. Once the last signature is added to the legal agreement, I
> will post the host bridge driver code that I am using. Meanwhile, here
> is an example of what the probe function looks like, posted as an example:
>
> static int myhostbridge_probe(struct platform_device *pdev)
> {
>         int err;
>         struct device_node *dev;
>         struct pci_host_bridge *bridge;
>         struct myhostbridge_port *pp;
>         resource_size_t lastbus;
>
>         dev = pdev->dev.of_node;
>
>         if (!of_device_is_available(dev)) {
>                 pr_warn("%s: disabled\n", dev->full_name);
>                 return -ENODEV;
>         }
>
>         pp = kzalloc(sizeof(struct myhostbridge_port), GFP_KERNEL);
>         if (!pp)
>                 return -ENOMEM;
>
>         bridge = of_create_pci_host_bridge(&pdev->dev, &myhostbridge_ops, pp);
>         if (IS_ERR(bridge)) {
>                 err = PTR_ERR(bridge);
>                 goto bridge_create_fail;
>         }
>
>         err = myhostbridge_setup(bridge->bus);
>         if (err)
>                 goto bridge_setup_fail;
>
>         /* We always enable PCI domains and we keep domain 0 backward
>          * compatible in /proc for video cards
>          */
>         pci_add_flags(PCI_ENABLE_PROC_DOMAINS);
>         pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC);
>
>         lastbus = pci_scan_child_bus(bridge->bus);
>         pci_bus_update_busn_res_end(bridge->bus, lastbus);
>
>         pci_assign_unassigned_bus_resources(bridge->bus);
>
>         pci_bus_add_devices(bridge->bus);
>
>         return 0;
>
> bridge_setup_fail:
>         put_device(&bridge->dev);
>         device_unregister(&bridge->dev);
> bridge_create_fail:
>         kfree(pp);
>         return err;
> }
>
> Best regards,
> Liviu
>
>
> Catalin Marinas (1):
>   arm64: Extend the PCI I/O space to 16MB
>
> Liviu Dudau (2):
>   Fix ioport_map() for !CONFIG_GENERIC_IOMAP cases.
>   arm64: Add architecture support for PCI
>
>  Documentation/arm64/memory.txt |  16 +--
>  arch/arm64/Kconfig             |  19 +++-
>  arch/arm64/include/asm/Kbuild  |   1 +
>  arch/arm64/include/asm/io.h    |   5 +-
>  arch/arm64/include/asm/pci.h   |  51 +++++++++
>  arch/arm64/kernel/Makefile     |   1 +
>  arch/arm64/kernel/pci.c        | 173 +++++++++++++++++++++++++++++++
>  include/asm-generic/io.h       |   2 +-
>  8 files changed, 258 insertions(+), 10 deletions(-)
>  create mode 100644 arch/arm64/include/asm/pci.h
>  create mode 100644 arch/arm64/kernel/pci.c
>
> --
> 1.9.0
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v7 0/3] Add support for PCI in AArch64

[Liviu Dudau]

On Tue, Apr 22, 2014 at 09:58:28AM +0100, Sandeepa Prabhu wrote:
> On 14 March 2014 21:04, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > Hi,
> >
> > This patch adds support for PCI to AArch64. It is based on my v7 patch
> > that adds support for creating generic host bridge structure from
> > device tree. With that in place, I was able to boot a platform that
> > has PCIe host bridge support and use a PCIe network card.
> Hi Liviu,
> 
> Are these patches (including your other patchset for device tree host
> bridge support ) available on a public git repo I can checkout from?

Yes, I have pushed them into git://linux-arm.org/linux-ld.git

Best regards,
Liviu

> 
> Thanks,
> ~Sandeepa
> 
> >
> > I have dropped the RFC tag from the subject as I now have the ambitious goal
> > of trying to get it mainlined.
> >
> > Changes from v6:
> >   - Guard the pci_domain_nr() inline implementation with #ifdef CONFIG_PCI as
> >     to avoid conflict with default empty version present in include/linux/pci.h.
> >     Thanks to Jingoo Han for catching this.
> >
> > Changes from v5:
> >   - Removed pcibios_fixup_bridge_ranges() as the week default version is fine.
> >   - Removed the ALIGN() call in pcibios_align_resource()
> >   - Stopped exporting pcibios_align_resource()
> >
> > Changes from v4:
> >   - Fixed the pci_domain_nr() implementation for arm64. Now we use
> >     find_pci_host_bride() to find the host bridge before we retrieve
> >     the domain number.
> >
> > Changes from v3:
> >   - Added Acks accumulated so far ;)
> >   - Still carrying Catalin's patch for moving the PCI_IO_BASE until it
> >     lands in linux-next or mainline, in order to ease applying the series
> >
> > Changes from v2:
> >   - Implement an arch specific version of pci_register_io_range() and
> >     pci_address_to_pio().
> >   - Return 1 from pci_proc_domain().
> >
> > Changes from v1:
> >   - Added Catalin's patch for moving the PCI_IO_BASE location and extend
> >     its size to 16MB
> >   - Integrated Arnd's version of pci_ioremap_io that uses a bitmap for
> >     keeping track of assigned IO space and returns an io_offset. At the
> >     moment the code is added in arch/arm64 but it can be moved in drivers/pci.
> >   - Added a fix for the generic ioport_map() function when !CONFIG_GENERIC_IOMAP
> >     as suggested by Arnd.
> >
> > v6 thread here: https://lkml.org/lkml/2014/3/5/41
> > v5 thread here: https://lkml.org/lkml/2014/3/4/307
> > v4 thread here: https://lkml.org/lkml/2014/3/3/298
> > v3 thread here: https://lkml.org/lkml/2014/2/28/211
> > v2 thread here: https://lkml.org/lkml/2014/2/27/255
> > v1 thread here: https://lkml.org/lkml/2014/2/3/389
> >
> >
> > The API used is different from the one used by ARM architecture. There is
> > no pci_common_init_dev() function and no hw_pci structure, as that is no
> > longer needed. Once the last signature is added to the legal agreement, I
> > will post the host bridge driver code that I am using. Meanwhile, here
> > is an example of what the probe function looks like, posted as an example:
> >
> > static int myhostbridge_probe(struct platform_device *pdev)
> > {
> >         int err;
> >         struct device_node *dev;
> >         struct pci_host_bridge *bridge;
> >         struct myhostbridge_port *pp;
> >         resource_size_t lastbus;
> >
> >         dev = pdev->dev.of_node;
> >
> >         if (!of_device_is_available(dev)) {
> >                 pr_warn("%s: disabled\n", dev->full_name);
> >                 return -ENODEV;
> >         }
> >
> >         pp = kzalloc(sizeof(struct myhostbridge_port), GFP_KERNEL);
> >         if (!pp)
> >                 return -ENOMEM;
> >
> >         bridge = of_create_pci_host_bridge(&pdev->dev, &myhostbridge_ops, pp);
> >         if (IS_ERR(bridge)) {
> >                 err = PTR_ERR(bridge);
> >                 goto bridge_create_fail;
> >         }
> >
> >         err = myhostbridge_setup(bridge->bus);
> >         if (err)
> >                 goto bridge_setup_fail;
> >
> >         /* We always enable PCI domains and we keep domain 0 backward
> >          * compatible in /proc for video cards
> >          */
> >         pci_add_flags(PCI_ENABLE_PROC_DOMAINS);
> >         pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC);
> >
> >         lastbus = pci_scan_child_bus(bridge->bus);
> >         pci_bus_update_busn_res_end(bridge->bus, lastbus);
> >
> >         pci_assign_unassigned_bus_resources(bridge->bus);
> >
> >         pci_bus_add_devices(bridge->bus);
> >
> >         return 0;
> >
> > bridge_setup_fail:
> >         put_device(&bridge->dev);
> >         device_unregister(&bridge->dev);
> > bridge_create_fail:
> >         kfree(pp);
> >         return err;
> > }
> >
> > Best regards,
> > Liviu
> >
> >
> > Catalin Marinas (1):
> >   arm64: Extend the PCI I/O space to 16MB
> >
> > Liviu Dudau (2):
> >   Fix ioport_map() for !CONFIG_GENERIC_IOMAP cases.
> >   arm64: Add architecture support for PCI
> >
> >  Documentation/arm64/memory.txt |  16 +--
> >  arch/arm64/Kconfig             |  19 +++-
> >  arch/arm64/include/asm/Kbuild  |   1 +
> >  arch/arm64/include/asm/io.h    |   5 +-
> >  arch/arm64/include/asm/pci.h   |  51 +++++++++
> >  arch/arm64/kernel/Makefile     |   1 +
> >  arch/arm64/kernel/pci.c        | 173 +++++++++++++++++++++++++++++++
> >  include/asm-generic/io.h       |   2 +-
> >  8 files changed, 258 insertions(+), 10 deletions(-)
> >  create mode 100644 arch/arm64/include/asm/pci.h
> >  create mode 100644 arch/arm64/kernel/pci.c
> >
> > --
> > 1.9.0
> >
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v7 0/3] Add support for PCI in AArch64

[Sandeepa Prabhu]

On 22 April 2014 15:41, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> On Tue, Apr 22, 2014 at 09:58:28AM +0100, Sandeepa Prabhu wrote:
>> On 14 March 2014 21:04, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
>> > Hi,
>> >
>> > This patch adds support for PCI to AArch64. It is based on my v7 patch
>> > that adds support for creating generic host bridge structure from
>> > device tree. With that in place, I was able to boot a platform that
>> > has PCIe host bridge support and use a PCIe network card.
>> Hi Liviu,
>>
>> Are these patches (including your other patchset for device tree host
>> bridge support ) available on a public git repo I can checkout from?
>
> Yes, I have pushed them into git://linux-arm.org/linux-ld.git
Thanks Liviu,
Just to understand, is the X-gene PCIe
(https://lwn.net/Articles/589733/) verified using the same patchset?

~Sandeepa
>
> Best regards,
> Liviu
>
>>
>> Thanks,
>> ~Sandeepa
>>
>> >
>> > I have dropped the RFC tag from the subject as I now have the ambitious goal
>> > of trying to get it mainlined.
>> >
>> > Changes from v6:
>> >   - Guard the pci_domain_nr() inline implementation with #ifdef CONFIG_PCI as
>> >     to avoid conflict with default empty version present in include/linux/pci.h.
>> >     Thanks to Jingoo Han for catching this.
>> >
>> > Changes from v5:
>> >   - Removed pcibios_fixup_bridge_ranges() as the week default version is fine.
>> >   - Removed the ALIGN() call in pcibios_align_resource()
>> >   - Stopped exporting pcibios_align_resource()
>> >
>> > Changes from v4:
>> >   - Fixed the pci_domain_nr() implementation for arm64. Now we use
>> >     find_pci_host_bride() to find the host bridge before we retrieve
>> >     the domain number.
>> >
>> > Changes from v3:
>> >   - Added Acks accumulated so far ;)
>> >   - Still carrying Catalin's patch for moving the PCI_IO_BASE until it
>> >     lands in linux-next or mainline, in order to ease applying the series
>> >
>> > Changes from v2:
>> >   - Implement an arch specific version of pci_register_io_range() and
>> >     pci_address_to_pio().
>> >   - Return 1 from pci_proc_domain().
>> >
>> > Changes from v1:
>> >   - Added Catalin's patch for moving the PCI_IO_BASE location and extend
>> >     its size to 16MB
>> >   - Integrated Arnd's version of pci_ioremap_io that uses a bitmap for
>> >     keeping track of assigned IO space and returns an io_offset. At the
>> >     moment the code is added in arch/arm64 but it can be moved in drivers/pci.
>> >   - Added a fix for the generic ioport_map() function when !CONFIG_GENERIC_IOMAP
>> >     as suggested by Arnd.
>> >
>> > v6 thread here: https://lkml.org/lkml/2014/3/5/41
>> > v5 thread here: https://lkml.org/lkml/2014/3/4/307
>> > v4 thread here: https://lkml.org/lkml/2014/3/3/298
>> > v3 thread here: https://lkml.org/lkml/2014/2/28/211
>> > v2 thread here: https://lkml.org/lkml/2014/2/27/255
>> > v1 thread here: https://lkml.org/lkml/2014/2/3/389
>> >
>> >
>> > The API used is different from the one used by ARM architecture. There is
>> > no pci_common_init_dev() function and no hw_pci structure, as that is no
>> > longer needed. Once the last signature is added to the legal agreement, I
>> > will post the host bridge driver code that I am using. Meanwhile, here
>> > is an example of what the probe function looks like, posted as an example:
>> >
>> > static int myhostbridge_probe(struct platform_device *pdev)
>> > {
>> >         int err;
>> >         struct device_node *dev;
>> >         struct pci_host_bridge *bridge;
>> >         struct myhostbridge_port *pp;
>> >         resource_size_t lastbus;
>> >
>> >         dev = pdev->dev.of_node;
>> >
>> >         if (!of_device_is_available(dev)) {
>> >                 pr_warn("%s: disabled\n", dev->full_name);
>> >                 return -ENODEV;
>> >         }
>> >
>> >         pp = kzalloc(sizeof(struct myhostbridge_port), GFP_KERNEL);
>> >         if (!pp)
>> >                 return -ENOMEM;
>> >
>> >         bridge = of_create_pci_host_bridge(&pdev->dev, &myhostbridge_ops, pp);
>> >         if (IS_ERR(bridge)) {
>> >                 err = PTR_ERR(bridge);
>> >                 goto bridge_create_fail;
>> >         }
>> >
>> >         err = myhostbridge_setup(bridge->bus);
>> >         if (err)
>> >                 goto bridge_setup_fail;
>> >
>> >         /* We always enable PCI domains and we keep domain 0 backward
>> >          * compatible in /proc for video cards
>> >          */
>> >         pci_add_flags(PCI_ENABLE_PROC_DOMAINS);
>> >         pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC);
>> >
>> >         lastbus = pci_scan_child_bus(bridge->bus);
>> >         pci_bus_update_busn_res_end(bridge->bus, lastbus);
>> >
>> >         pci_assign_unassigned_bus_resources(bridge->bus);
>> >
>> >         pci_bus_add_devices(bridge->bus);
>> >
>> >         return 0;
>> >
>> > bridge_setup_fail:
>> >         put_device(&bridge->dev);
>> >         device_unregister(&bridge->dev);
>> > bridge_create_fail:
>> >         kfree(pp);
>> >         return err;
>> > }
>> >
>> > Best regards,
>> > Liviu
>> >
>> >
>> > Catalin Marinas (1):
>> >   arm64: Extend the PCI I/O space to 16MB
>> >
>> > Liviu Dudau (2):
>> >   Fix ioport_map() for !CONFIG_GENERIC_IOMAP cases.
>> >   arm64: Add architecture support for PCI
>> >
>> >  Documentation/arm64/memory.txt |  16 +--
>> >  arch/arm64/Kconfig             |  19 +++-
>> >  arch/arm64/include/asm/Kbuild  |   1 +
>> >  arch/arm64/include/asm/io.h    |   5 +-
>> >  arch/arm64/include/asm/pci.h   |  51 +++++++++
>> >  arch/arm64/kernel/Makefile     |   1 +
>> >  arch/arm64/kernel/pci.c        | 173 +++++++++++++++++++++++++++++++
>> >  include/asm-generic/io.h       |   2 +-
>> >  8 files changed, 258 insertions(+), 10 deletions(-)
>> >  create mode 100644 arch/arm64/include/asm/pci.h
>> >  create mode 100644 arch/arm64/kernel/pci.c
>> >
>> > --
>> > 1.9.0
>> >
>> >
>> > _______________________________________________
>> > linux-arm-kernel mailing list
>> > linux-arm-kernel@lists.infradead.org
>> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>
>
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯
>

Re: [PATCH v7 0/3] Add support for PCI in AArch64

[Liviu Dudau]

On Tue, Apr 22, 2014 at 12:50:40PM +0100, Sandeepa Prabhu wrote:
> On 22 April 2014 15:41, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > On Tue, Apr 22, 2014 at 09:58:28AM +0100, Sandeepa Prabhu wrote:
> >> On 14 March 2014 21:04, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> >> > Hi,
> >> >
> >> > This patch adds support for PCI to AArch64. It is based on my v7 patch
> >> > that adds support for creating generic host bridge structure from
> >> > device tree. With that in place, I was able to boot a platform that
> >> > has PCIe host bridge support and use a PCIe network card.
> >> Hi Liviu,
> >>
> >> Are these patches (including your other patchset for device tree host
> >> bridge support ) available on a public git repo I can checkout from?
> >
> > Yes, I have pushed them into git://linux-arm.org/linux-ld.git
> Thanks Liviu,
> Just to understand, is the X-gene PCIe
> (https://lwn.net/Articles/589733/) verified using the same patchset?

Tanmay can confirm, but I belive so.

Best regards,
Liviu

> 
> ~Sandeepa
> >
> > Best regards,
> > Liviu
> >
> >>
> >> Thanks,
> >> ~Sandeepa
> >>
> >> >
> >> > I have dropped the RFC tag from the subject as I now have the ambitious goal
> >> > of trying to get it mainlined.
> >> >
> >> > Changes from v6:
> >> >   - Guard the pci_domain_nr() inline implementation with #ifdef CONFIG_PCI as
> >> >     to avoid conflict with default empty version present in include/linux/pci.h.
> >> >     Thanks to Jingoo Han for catching this.
> >> >
> >> > Changes from v5:
> >> >   - Removed pcibios_fixup_bridge_ranges() as the week default version is fine.
> >> >   - Removed the ALIGN() call in pcibios_align_resource()
> >> >   - Stopped exporting pcibios_align_resource()
> >> >
> >> > Changes from v4:
> >> >   - Fixed the pci_domain_nr() implementation for arm64. Now we use
> >> >     find_pci_host_bride() to find the host bridge before we retrieve
> >> >     the domain number.
> >> >
> >> > Changes from v3:
> >> >   - Added Acks accumulated so far ;)
> >> >   - Still carrying Catalin's patch for moving the PCI_IO_BASE until it
> >> >     lands in linux-next or mainline, in order to ease applying the series
> >> >
> >> > Changes from v2:
> >> >   - Implement an arch specific version of pci_register_io_range() and
> >> >     pci_address_to_pio().
> >> >   - Return 1 from pci_proc_domain().
> >> >
> >> > Changes from v1:
> >> >   - Added Catalin's patch for moving the PCI_IO_BASE location and extend
> >> >     its size to 16MB
> >> >   - Integrated Arnd's version of pci_ioremap_io that uses a bitmap for
> >> >     keeping track of assigned IO space and returns an io_offset. At the
> >> >     moment the code is added in arch/arm64 but it can be moved in drivers/pci.
> >> >   - Added a fix for the generic ioport_map() function when !CONFIG_GENERIC_IOMAP
> >> >     as suggested by Arnd.
> >> >
> >> > v6 thread here: https://lkml.org/lkml/2014/3/5/41
> >> > v5 thread here: https://lkml.org/lkml/2014/3/4/307
> >> > v4 thread here: https://lkml.org/lkml/2014/3/3/298
> >> > v3 thread here: https://lkml.org/lkml/2014/2/28/211
> >> > v2 thread here: https://lkml.org/lkml/2014/2/27/255
> >> > v1 thread here: https://lkml.org/lkml/2014/2/3/389
> >> >
> >> >
> >> > The API used is different from the one used by ARM architecture. There is
> >> > no pci_common_init_dev() function and no hw_pci structure, as that is no
> >> > longer needed. Once the last signature is added to the legal agreement, I
> >> > will post the host bridge driver code that I am using. Meanwhile, here
> >> > is an example of what the probe function looks like, posted as an example:
> >> >
> >> > static int myhostbridge_probe(struct platform_device *pdev)
> >> > {
> >> >         int err;
> >> >         struct device_node *dev;
> >> >         struct pci_host_bridge *bridge;
> >> >         struct myhostbridge_port *pp;
> >> >         resource_size_t lastbus;
> >> >
> >> >         dev = pdev->dev.of_node;
> >> >
> >> >         if (!of_device_is_available(dev)) {
> >> >                 pr_warn("%s: disabled\n", dev->full_name);
> >> >                 return -ENODEV;
> >> >         }
> >> >
> >> >         pp = kzalloc(sizeof(struct myhostbridge_port), GFP_KERNEL);
> >> >         if (!pp)
> >> >                 return -ENOMEM;
> >> >
> >> >         bridge = of_create_pci_host_bridge(&pdev->dev, &myhostbridge_ops, pp);
> >> >         if (IS_ERR(bridge)) {
> >> >                 err = PTR_ERR(bridge);
> >> >                 goto bridge_create_fail;
> >> >         }
> >> >
> >> >         err = myhostbridge_setup(bridge->bus);
> >> >         if (err)
> >> >                 goto bridge_setup_fail;
> >> >
> >> >         /* We always enable PCI domains and we keep domain 0 backward
> >> >          * compatible in /proc for video cards
> >> >          */
> >> >         pci_add_flags(PCI_ENABLE_PROC_DOMAINS);
> >> >         pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC);
> >> >
> >> >         lastbus = pci_scan_child_bus(bridge->bus);
> >> >         pci_bus_update_busn_res_end(bridge->bus, lastbus);
> >> >
> >> >         pci_assign_unassigned_bus_resources(bridge->bus);
> >> >
> >> >         pci_bus_add_devices(bridge->bus);
> >> >
> >> >         return 0;
> >> >
> >> > bridge_setup_fail:
> >> >         put_device(&bridge->dev);
> >> >         device_unregister(&bridge->dev);
> >> > bridge_create_fail:
> >> >         kfree(pp);
> >> >         return err;
> >> > }
> >> >
> >> > Best regards,
> >> > Liviu
> >> >
> >> >
> >> > Catalin Marinas (1):
> >> >   arm64: Extend the PCI I/O space to 16MB
> >> >
> >> > Liviu Dudau (2):
> >> >   Fix ioport_map() for !CONFIG_GENERIC_IOMAP cases.
> >> >   arm64: Add architecture support for PCI
> >> >
> >> >  Documentation/arm64/memory.txt |  16 +--
> >> >  arch/arm64/Kconfig             |  19 +++-
> >> >  arch/arm64/include/asm/Kbuild  |   1 +
> >> >  arch/arm64/include/asm/io.h    |   5 +-
> >> >  arch/arm64/include/asm/pci.h   |  51 +++++++++
> >> >  arch/arm64/kernel/Makefile     |   1 +
> >> >  arch/arm64/kernel/pci.c        | 173 +++++++++++++++++++++++++++++++
> >> >  include/asm-generic/io.h       |   2 +-
> >> >  8 files changed, 258 insertions(+), 10 deletions(-)
> >> >  create mode 100644 arch/arm64/include/asm/pci.h
> >> >  create mode 100644 arch/arm64/kernel/pci.c
> >> >
> >> > --
> >> > 1.9.0
> >> >
> >> >
> >> > _______________________________________________
> >> > linux-arm-kernel mailing list
> >> > linux-arm-kernel@lists.infradead.org
> >> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >>
> >
> > --
> > ====================
> > | I would like to |
> > | fix the world,  |
> > | but they're not |
> > | giving me the   |
> >  \ source code!  /
> >   ---------------
> >     ¯\_(ツ)_/¯
> >
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v7 0/3] Add support for PCI in AArch64

[Tanmay Inamdar]

Hello Sandeepa,

On Tue, Apr 22, 2014 at 4:50 AM, Sandeepa Prabhu
<sandeepa.prabhu@linaro.org> wrote:
> On 22 April 2014 15:41, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
>> On Tue, Apr 22, 2014 at 09:58:28AM +0100, Sandeepa Prabhu wrote:
>>> On 14 March 2014 21:04, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
>>> > Hi,
>>> >
>>> > This patch adds support for PCI to AArch64. It is based on my v7 patch
>>> > that adds support for creating generic host bridge structure from
>>> > device tree. With that in place, I was able to boot a platform that
>>> > has PCIe host bridge support and use a PCIe network card.
>>> Hi Liviu,
>>>
>>> Are these patches (including your other patchset for device tree host
>>> bridge support ) available on a public git repo I can checkout from?
>>
>> Yes, I have pushed them into git://linux-arm.org/linux-ld.git
> Thanks Liviu,
> Just to understand, is the X-gene PCIe
> (https://lwn.net/Articles/589733/) verified using the same patchset?

Yes. V5 version of X-Gene PCIe patches have verified Liviu's patchset
on X-Gene platform.

>
> ~Sandeepa
>>
>> Best regards,
>> Liviu
>>
>>>
>>> Thanks,
>>> ~Sandeepa
>>>
>>> >
>>> > I have dropped the RFC tag from the subject as I now have the ambitious goal
>>> > of trying to get it mainlined.
>>> >
>>> > Changes from v6:
>>> >   - Guard the pci_domain_nr() inline implementation with #ifdef CONFIG_PCI as
>>> >     to avoid conflict with default empty version present in include/linux/pci.h.
>>> >     Thanks to Jingoo Han for catching this.
>>> >
>>> > Changes from v5:
>>> >   - Removed pcibios_fixup_bridge_ranges() as the week default version is fine.
>>> >   - Removed the ALIGN() call in pcibios_align_resource()
>>> >   - Stopped exporting pcibios_align_resource()
>>> >
>>> > Changes from v4:
>>> >   - Fixed the pci_domain_nr() implementation for arm64. Now we use
>>> >     find_pci_host_bride() to find the host bridge before we retrieve
>>> >     the domain number.
>>> >
>>> > Changes from v3:
>>> >   - Added Acks accumulated so far ;)
>>> >   - Still carrying Catalin's patch for moving the PCI_IO_BASE until it
>>> >     lands in linux-next or mainline, in order to ease applying the series
>>> >
>>> > Changes from v2:
>>> >   - Implement an arch specific version of pci_register_io_range() and
>>> >     pci_address_to_pio().
>>> >   - Return 1 from pci_proc_domain().
>>> >
>>> > Changes from v1:
>>> >   - Added Catalin's patch for moving the PCI_IO_BASE location and extend
>>> >     its size to 16MB
>>> >   - Integrated Arnd's version of pci_ioremap_io that uses a bitmap for
>>> >     keeping track of assigned IO space and returns an io_offset. At the
>>> >     moment the code is added in arch/arm64 but it can be moved in drivers/pci.
>>> >   - Added a fix for the generic ioport_map() function when !CONFIG_GENERIC_IOMAP
>>> >     as suggested by Arnd.
>>> >
>>> > v6 thread here: https://lkml.org/lkml/2014/3/5/41
>>> > v5 thread here: https://lkml.org/lkml/2014/3/4/307
>>> > v4 thread here: https://lkml.org/lkml/2014/3/3/298
>>> > v3 thread here: https://lkml.org/lkml/2014/2/28/211
>>> > v2 thread here: https://lkml.org/lkml/2014/2/27/255
>>> > v1 thread here: https://lkml.org/lkml/2014/2/3/389
>>> >
>>> >
>>> > The API used is different from the one used by ARM architecture. There is
>>> > no pci_common_init_dev() function and no hw_pci structure, as that is no
>>> > longer needed. Once the last signature is added to the legal agreement, I
>>> > will post the host bridge driver code that I am using. Meanwhile, here
>>> > is an example of what the probe function looks like, posted as an example:
>>> >
>>> > static int myhostbridge_probe(struct platform_device *pdev)
>>> > {
>>> >         int err;
>>> >         struct device_node *dev;
>>> >         struct pci_host_bridge *bridge;
>>> >         struct myhostbridge_port *pp;
>>> >         resource_size_t lastbus;
>>> >
>>> >         dev = pdev->dev.of_node;
>>> >
>>> >         if (!of_device_is_available(dev)) {
>>> >                 pr_warn("%s: disabled\n", dev->full_name);
>>> >                 return -ENODEV;
>>> >         }
>>> >
>>> >         pp = kzalloc(sizeof(struct myhostbridge_port), GFP_KERNEL);
>>> >         if (!pp)
>>> >                 return -ENOMEM;
>>> >
>>> >         bridge = of_create_pci_host_bridge(&pdev->dev, &myhostbridge_ops, pp);
>>> >         if (IS_ERR(bridge)) {
>>> >                 err = PTR_ERR(bridge);
>>> >                 goto bridge_create_fail;
>>> >         }
>>> >
>>> >         err = myhostbridge_setup(bridge->bus);
>>> >         if (err)
>>> >                 goto bridge_setup_fail;
>>> >
>>> >         /* We always enable PCI domains and we keep domain 0 backward
>>> >          * compatible in /proc for video cards
>>> >          */
>>> >         pci_add_flags(PCI_ENABLE_PROC_DOMAINS);
>>> >         pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC);
>>> >
>>> >         lastbus = pci_scan_child_bus(bridge->bus);
>>> >         pci_bus_update_busn_res_end(bridge->bus, lastbus);
>>> >
>>> >         pci_assign_unassigned_bus_resources(bridge->bus);
>>> >
>>> >         pci_bus_add_devices(bridge->bus);
>>> >
>>> >         return 0;
>>> >
>>> > bridge_setup_fail:
>>> >         put_device(&bridge->dev);
>>> >         device_unregister(&bridge->dev);
>>> > bridge_create_fail:
>>> >         kfree(pp);
>>> >         return err;
>>> > }
>>> >
>>> > Best regards,
>>> > Liviu
>>> >
>>> >
>>> > Catalin Marinas (1):
>>> >   arm64: Extend the PCI I/O space to 16MB
>>> >
>>> > Liviu Dudau (2):
>>> >   Fix ioport_map() for !CONFIG_GENERIC_IOMAP cases.
>>> >   arm64: Add architecture support for PCI
>>> >
>>> >  Documentation/arm64/memory.txt |  16 +--
>>> >  arch/arm64/Kconfig             |  19 +++-
>>> >  arch/arm64/include/asm/Kbuild  |   1 +
>>> >  arch/arm64/include/asm/io.h    |   5 +-
>>> >  arch/arm64/include/asm/pci.h   |  51 +++++++++
>>> >  arch/arm64/kernel/Makefile     |   1 +
>>> >  arch/arm64/kernel/pci.c        | 173 +++++++++++++++++++++++++++++++
>>> >  include/asm-generic/io.h       |   2 +-
>>> >  8 files changed, 258 insertions(+), 10 deletions(-)
>>> >  create mode 100644 arch/arm64/include/asm/pci.h
>>> >  create mode 100644 arch/arm64/kernel/pci.c
>>> >
>>> > --
>>> > 1.9.0
>>> >
>>> >
>>> > _______________________________________________
>>> > linux-arm-kernel mailing list
>>> > linux-arm-kernel@lists.infradead.org
>>> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>
>>
>> --
>> ====================
>> | I would like to |
>> | fix the world,  |
>> | but they're not |
>> | giving me the   |
>>  \ source code!  /
>>   ---------------
>>     ¯\_(ツ)_/¯
>>

Re: [PATCH v7 0/3] Add support for PCI in AArch64

[Sandeepa Prabhu]

On 24 April 2014 02:02, Tanmay Inamdar <tinamdar@apm.com> wrote:
> Hello Sandeepa,
>
> On Tue, Apr 22, 2014 at 4:50 AM, Sandeepa Prabhu
> <sandeepa.prabhu@linaro.org> wrote:
>> On 22 April 2014 15:41, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
>>> On Tue, Apr 22, 2014 at 09:58:28AM +0100, Sandeepa Prabhu wrote:
>>>> On 14 March 2014 21:04, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
>>>> > Hi,
>>>> >
>>>> > This patch adds support for PCI to AArch64. It is based on my v7 patch
>>>> > that adds support for creating generic host bridge structure from
>>>> > device tree. With that in place, I was able to boot a platform that
>>>> > has PCIe host bridge support and use a PCIe network card.
>>>> Hi Liviu,
>>>>
>>>> Are these patches (including your other patchset for device tree host
>>>> bridge support ) available on a public git repo I can checkout from?
>>>
>>> Yes, I have pushed them into git://linux-arm.org/linux-ld.git
>> Thanks Liviu,
>> Just to understand, is the X-gene PCIe
>> (https://lwn.net/Articles/589733/) verified using the same patchset?
>
> Yes. V5 version of X-Gene PCIe patches have verified Liviu's patchset
> on X-Gene platform.
Thanks Tanmay!
>
>>
>> ~Sandeepa
>>>
>>> Best regards,
>>> Liviu
>>>
>>>>
>>>> Thanks,
>>>> ~Sandeepa
>>>>
>>>> >
>>>> > I have dropped the RFC tag from the subject as I now have the ambitious goal
>>>> > of trying to get it mainlined.
>>>> >
>>>> > Changes from v6:
>>>> >   - Guard the pci_domain_nr() inline implementation with #ifdef CONFIG_PCI as
>>>> >     to avoid conflict with default empty version present in include/linux/pci.h.
>>>> >     Thanks to Jingoo Han for catching this.
>>>> >
>>>> > Changes from v5:
>>>> >   - Removed pcibios_fixup_bridge_ranges() as the week default version is fine.
>>>> >   - Removed the ALIGN() call in pcibios_align_resource()
>>>> >   - Stopped exporting pcibios_align_resource()
>>>> >
>>>> > Changes from v4:
>>>> >   - Fixed the pci_domain_nr() implementation for arm64. Now we use
>>>> >     find_pci_host_bride() to find the host bridge before we retrieve
>>>> >     the domain number.
>>>> >
>>>> > Changes from v3:
>>>> >   - Added Acks accumulated so far ;)
>>>> >   - Still carrying Catalin's patch for moving the PCI_IO_BASE until it
>>>> >     lands in linux-next or mainline, in order to ease applying the series
>>>> >
>>>> > Changes from v2:
>>>> >   - Implement an arch specific version of pci_register_io_range() and
>>>> >     pci_address_to_pio().
>>>> >   - Return 1 from pci_proc_domain().
>>>> >
>>>> > Changes from v1:
>>>> >   - Added Catalin's patch for moving the PCI_IO_BASE location and extend
>>>> >     its size to 16MB
>>>> >   - Integrated Arnd's version of pci_ioremap_io that uses a bitmap for
>>>> >     keeping track of assigned IO space and returns an io_offset. At the
>>>> >     moment the code is added in arch/arm64 but it can be moved in drivers/pci.
>>>> >   - Added a fix for the generic ioport_map() function when !CONFIG_GENERIC_IOMAP
>>>> >     as suggested by Arnd.
>>>> >
>>>> > v6 thread here: https://lkml.org/lkml/2014/3/5/41
>>>> > v5 thread here: https://lkml.org/lkml/2014/3/4/307
>>>> > v4 thread here: https://lkml.org/lkml/2014/3/3/298
>>>> > v3 thread here: https://lkml.org/lkml/2014/2/28/211
>>>> > v2 thread here: https://lkml.org/lkml/2014/2/27/255
>>>> > v1 thread here: https://lkml.org/lkml/2014/2/3/389
>>>> >
>>>> >
>>>> > The API used is different from the one used by ARM architecture. There is
>>>> > no pci_common_init_dev() function and no hw_pci structure, as that is no
>>>> > longer needed. Once the last signature is added to the legal agreement, I
>>>> > will post the host bridge driver code that I am using. Meanwhile, here
>>>> > is an example of what the probe function looks like, posted as an example:
>>>> >
>>>> > static int myhostbridge_probe(struct platform_device *pdev)
>>>> > {
>>>> >         int err;
>>>> >         struct device_node *dev;
>>>> >         struct pci_host_bridge *bridge;
>>>> >         struct myhostbridge_port *pp;
>>>> >         resource_size_t lastbus;
>>>> >
>>>> >         dev = pdev->dev.of_node;
>>>> >
>>>> >         if (!of_device_is_available(dev)) {
>>>> >                 pr_warn("%s: disabled\n", dev->full_name);
>>>> >                 return -ENODEV;
>>>> >         }
>>>> >
>>>> >         pp = kzalloc(sizeof(struct myhostbridge_port), GFP_KERNEL);
>>>> >         if (!pp)
>>>> >                 return -ENOMEM;
>>>> >
>>>> >         bridge = of_create_pci_host_bridge(&pdev->dev, &myhostbridge_ops, pp);
>>>> >         if (IS_ERR(bridge)) {
>>>> >                 err = PTR_ERR(bridge);
>>>> >                 goto bridge_create_fail;
>>>> >         }
>>>> >
>>>> >         err = myhostbridge_setup(bridge->bus);
>>>> >         if (err)
>>>> >                 goto bridge_setup_fail;
>>>> >
>>>> >         /* We always enable PCI domains and we keep domain 0 backward
>>>> >          * compatible in /proc for video cards
>>>> >          */
>>>> >         pci_add_flags(PCI_ENABLE_PROC_DOMAINS);
>>>> >         pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC);
>>>> >
>>>> >         lastbus = pci_scan_child_bus(bridge->bus);
>>>> >         pci_bus_update_busn_res_end(bridge->bus, lastbus);
>>>> >
>>>> >         pci_assign_unassigned_bus_resources(bridge->bus);
>>>> >
>>>> >         pci_bus_add_devices(bridge->bus);
>>>> >
>>>> >         return 0;
>>>> >
>>>> > bridge_setup_fail:
>>>> >         put_device(&bridge->dev);
>>>> >         device_unregister(&bridge->dev);
>>>> > bridge_create_fail:
>>>> >         kfree(pp);
>>>> >         return err;
>>>> > }
>>>> >
>>>> > Best regards,
>>>> > Liviu
>>>> >
>>>> >
>>>> > Catalin Marinas (1):
>>>> >   arm64: Extend the PCI I/O space to 16MB
>>>> >
>>>> > Liviu Dudau (2):
>>>> >   Fix ioport_map() for !CONFIG_GENERIC_IOMAP cases.
>>>> >   arm64: Add architecture support for PCI
>>>> >
>>>> >  Documentation/arm64/memory.txt |  16 +--
>>>> >  arch/arm64/Kconfig             |  19 +++-
>>>> >  arch/arm64/include/asm/Kbuild  |   1 +
>>>> >  arch/arm64/include/asm/io.h    |   5 +-
>>>> >  arch/arm64/include/asm/pci.h   |  51 +++++++++
>>>> >  arch/arm64/kernel/Makefile     |   1 +
>>>> >  arch/arm64/kernel/pci.c        | 173 +++++++++++++++++++++++++++++++
>>>> >  include/asm-generic/io.h       |   2 +-
>>>> >  8 files changed, 258 insertions(+), 10 deletions(-)
>>>> >  create mode 100644 arch/arm64/include/asm/pci.h
>>>> >  create mode 100644 arch/arm64/kernel/pci.c
>>>> >
>>>> > --
>>>> > 1.9.0
>>>> >
>>>> >
>>>> > _______________________________________________
>>>> > linux-arm-kernel mailing list
>>>> > linux-arm-kernel@lists.infradead.org
>>>> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>>>>
>>>
>>> --
>>> ====================
>>> | I would like to |
>>> | fix the world,  |
>>> | but they're not |
>>> | giving me the   |
>>>  \ source code!  /
>>>   ---------------
>>>     ¯\_(ツ)_/¯
>>>

Re: [PATCH v7 0/3] Add support for PCI in AArch64

[Sunil Kovvuri]

Hi Liviu,

I am using your ARM64 PCIe patches to write a PCIe host controller
driver for our SOC. I am facing an issue with SR-IOV capable device.

Consider an PCI Express endpoint connected to a PCI Express
Root Port.  The PCI Express endpoint provides PCI-SIG SR-IOV
capabilities with a single physical function and a large number
of virtual functions.  The Root Port contains a pci-pci bridge in
between it and the SR-IOV device.

When the SR-IOV capable device's driver tries to enable sriov
(pci_enable_sriov()) it fails to create/add PCI device for each
virtual function reporting  "not enough MMIO resources for SR-IOV".

In sriov_enable() (drivers/pci/iov.c)

 296        for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
 297                 bars |= (1 << (i + PCI_IOV_RESOURCES));
 298                 res = dev->resource + PCI_IOV_RESOURCES + i;
 299                 if (res->parent)
 300                         nres++;
 301         }
 302         if (nres != iov->nres) {
 303                 dev_err(&dev->dev, "not enough MMIO resources for
 SR-IOV\n");
 304                 return -ENOMEM;
 305         }

Here its checking if physical function's IOV resource has a parent or not.
Which is pci-pci bridge in this case. Otherwise it doesn't consider
that resource.

Added below api to your patch.
This will try to claim a resource while creating a PCI device which
inturn sets 'res->parent'.

Let me know if this is okay.

diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index 9f29c9a..fbfb48f 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -125,6 +125,21 @@ resource_size_t pcibios_align_resource(void
*data, const struct resource *res,
        return res->start;
 }

+int pcibios_add_device(struct pci_dev *pdev)
+{
+        unsigned int i, type_mask = IORESOURCE_IO | IORESOURCE_MEM;
+        struct resource *res;
+
+        for (i = 0; i < PCI_NUM_RESOURCES; i++) {
+                res = &pdev->resource[i];
+                if (res->parent || !(res->flags & type_mask))
+                        continue;
+                pci_claim_resource(pdev, i);
+        }
+


On Fri, Mar 14, 2014 at 9:04 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> Hi,
>
> This patch adds support for PCI to AArch64. It is based on my v7 patch
> that adds support for creating generic host bridge structure from
> device tree. With that in place, I was able to boot a platform that
> has PCIe host bridge support and use a PCIe network card.
>
> I have dropped the RFC tag from the subject as I now have the ambitious goal
> of trying to get it mainlined.
>
> Changes from v6:
>   - Guard the pci_domain_nr() inline implementation with #ifdef CONFIG_PCI as
>     to avoid conflict with default empty version present in include/linux/pci.h.
>     Thanks to Jingoo Han for catching this.
>
> Changes from v5:
>   - Removed pcibios_fixup_bridge_ranges() as the week default version is fine.
>   - Removed the ALIGN() call in pcibios_align_resource()
>   - Stopped exporting pcibios_align_resource()
>
> Changes from v4:
>   - Fixed the pci_domain_nr() implementation for arm64. Now we use
>     find_pci_host_bride() to find the host bridge before we retrieve
>     the domain number.
>
> Changes from v3:
>   - Added Acks accumulated so far ;)
>   - Still carrying Catalin's patch for moving the PCI_IO_BASE until it
>     lands in linux-next or mainline, in order to ease applying the series
>
> Changes from v2:
>   - Implement an arch specific version of pci_register_io_range() and
>     pci_address_to_pio().
>   - Return 1 from pci_proc_domain().
>
> Changes from v1:
>   - Added Catalin's patch for moving the PCI_IO_BASE location and extend
>     its size to 16MB
>   - Integrated Arnd's version of pci_ioremap_io that uses a bitmap for
>     keeping track of assigned IO space and returns an io_offset. At the
>     moment the code is added in arch/arm64 but it can be moved in drivers/pci.
>   - Added a fix for the generic ioport_map() function when !CONFIG_GENERIC_IOMAP
>     as suggested by Arnd.
>
> v6 thread here: https://lkml.org/lkml/2014/3/5/41
> v5 thread here: https://lkml.org/lkml/2014/3/4/307
> v4 thread here: https://lkml.org/lkml/2014/3/3/298
> v3 thread here: https://lkml.org/lkml/2014/2/28/211
> v2 thread here: https://lkml.org/lkml/2014/2/27/255
> v1 thread here: https://lkml.org/lkml/2014/2/3/389
>
>
> The API used is different from the one used by ARM architecture. There is
> no pci_common_init_dev() function and no hw_pci structure, as that is no
> longer needed. Once the last signature is added to the legal agreement, I
> will post the host bridge driver code that I am using. Meanwhile, here
> is an example of what the probe function looks like, posted as an example:
>
> static int myhostbridge_probe(struct platform_device *pdev)
> {
>         int err;
>         struct device_node *dev;
>         struct pci_host_bridge *bridge;
>         struct myhostbridge_port *pp;
>         resource_size_t lastbus;
>
>         dev = pdev->dev.of_node;
>
>         if (!of_device_is_available(dev)) {
>                 pr_warn("%s: disabled\n", dev->full_name);
>                 return -ENODEV;
>         }
>
>         pp = kzalloc(sizeof(struct myhostbridge_port), GFP_KERNEL);
>         if (!pp)
>                 return -ENOMEM;
>
>         bridge = of_create_pci_host_bridge(&pdev->dev, &myhostbridge_ops, pp);
>         if (IS_ERR(bridge)) {
>                 err = PTR_ERR(bridge);
>                 goto bridge_create_fail;
>         }
>
>         err = myhostbridge_setup(bridge->bus);
>         if (err)
>                 goto bridge_setup_fail;
>
>         /* We always enable PCI domains and we keep domain 0 backward
>          * compatible in /proc for video cards
>          */
>         pci_add_flags(PCI_ENABLE_PROC_DOMAINS);
>         pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC);
>
>         lastbus = pci_scan_child_bus(bridge->bus);
>         pci_bus_update_busn_res_end(bridge->bus, lastbus);
>
>         pci_assign_unassigned_bus_resources(bridge->bus);
>
>         pci_bus_add_devices(bridge->bus);
>
>         return 0;
>
> bridge_setup_fail:
>         put_device(&bridge->dev);
>         device_unregister(&bridge->dev);
> bridge_create_fail:
>         kfree(pp);
>         return err;
> }
>
> Best regards,
> Liviu
>
>
> Catalin Marinas (1):
>   arm64: Extend the PCI I/O space to 16MB
>
> Liviu Dudau (2):
>   Fix ioport_map() for !CONFIG_GENERIC_IOMAP cases.
>   arm64: Add architecture support for PCI
>
>  Documentation/arm64/memory.txt |  16 +--
>  arch/arm64/Kconfig             |  19 +++-
>  arch/arm64/include/asm/Kbuild  |   1 +
>  arch/arm64/include/asm/io.h    |   5 +-
>  arch/arm64/include/asm/pci.h   |  51 +++++++++
>  arch/arm64/kernel/Makefile     |   1 +
>  arch/arm64/kernel/pci.c        | 173 +++++++++++++++++++++++++++++++
>  include/asm-generic/io.h       |   2 +-
>  8 files changed, 258 insertions(+), 10 deletions(-)
>  create mode 100644 arch/arm64/include/asm/pci.h
>  create mode 100644 arch/arm64/kernel/pci.c
>
> --
> 1.9.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 0/3] Add support for PCI in AArch64

[Liviu Dudau]

Hi Sunil,

On Fri, May 16, 2014 at 11:33:04AM +0100, Sunil Kovvuri wrote:
> Hi Liviu,
> 
> I am using your ARM64 PCIe patches to write a PCIe host controller
> driver for our SOC. I am facing an issue with SR-IOV capable device.
> 
> Consider an PCI Express endpoint connected to a PCI Express
> Root Port.  The PCI Express endpoint provides PCI-SIG SR-IOV
> capabilities with a single physical function and a large number
> of virtual functions.  The Root Port contains a pci-pci bridge in
> between it and the SR-IOV device.
> 
> When the SR-IOV capable device's driver tries to enable sriov
> (pci_enable_sriov()) it fails to create/add PCI device for each
> virtual function reporting  "not enough MMIO resources for SR-IOV".
> 
> In sriov_enable() (drivers/pci/iov.c)
> 
>  296        for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
>  297                 bars |= (1 << (i + PCI_IOV_RESOURCES));
>  298                 res = dev->resource + PCI_IOV_RESOURCES + i;
>  299                 if (res->parent)
>  300                         nres++;
>  301         }
>  302         if (nres != iov->nres) {
>  303                 dev_err(&dev->dev, "not enough MMIO resources for
>  SR-IOV\n");
>  304                 return -ENOMEM;
>  305         }
> 
> Here its checking if physical function's IOV resource has a parent or not.
> Which is pci-pci bridge in this case. Otherwise it doesn't consider
> that resource.
> 
> Added below api to your patch.
> This will try to claim a resource while creating a PCI device which
> inturn sets 'res->parent'.
> 
> Let me know if this is okay.
> 
> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> index 9f29c9a..fbfb48f 100644
> --- a/arch/arm64/kernel/pci.c
> +++ b/arch/arm64/kernel/pci.c
> @@ -125,6 +125,21 @@ resource_size_t pcibios_align_resource(void
> *data, const struct resource *res,
>         return res->start;
>  }
> 
> +int pcibios_add_device(struct pci_dev *pdev)
> +{
> +        unsigned int i, type_mask = IORESOURCE_IO | IORESOURCE_MEM;
> +        struct resource *res;
> +
> +        for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> +                res = &pdev->resource[i];
> +                if (res->parent || !(res->flags & type_mask))
> +                        continue;
> +                pci_claim_resource(pdev, i);
> +        }
> +
> 

I would like not to have to add your patch in this file as I am trying to
remove it entirely. I don't have an SR-IOV capable device in my setup so
I am not able to debug your problem, but could you have a go and try to
figure out why the SR-IOV resources do not get a parent associated with
when they get created?

Many thanks,
Liviu


-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v7 0/3] Add support for PCI in AArch64

[Sunil Kovvuri]

Hi Liviu,

Issue may not be only with SR-IOV resources.

I am not an expert with Linux PCI core. But i am trying to understand
how even for a non SR-IOV capable device's resource, gets a parent
associated with it.

When a PCI device driver calls pci_enable_device() which inturn calls
'arch/arm64/kernel/pci.c' pcibios_enable_device() which uses Linux PCI core
API 'pci_enable_resources'. This API checks 'res->parent' for all
valid resources
before enabling them, otherwise it fails.
                if (!r->parent) {
                        dev_err(&dev->dev, "device not available "
                                "(can't reserve %pR)\n", r);
                        return -EINVAL;
                }

Can you please let me know where this hierarchy is being set for
'pcibios_enable_resources' to work.

I could only find request_resource() API which sets the parent and
pci_claim_resource() uses this.

Thanks,
Sunil.

On Fri, May 16, 2014 at 6:54 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> Hi Sunil,
>
> On Fri, May 16, 2014 at 11:33:04AM +0100, Sunil Kovvuri wrote:
>> Hi Liviu,
>>
>> I am using your ARM64 PCIe patches to write a PCIe host controller
>> driver for our SOC. I am facing an issue with SR-IOV capable device.
>>
>> Consider an PCI Express endpoint connected to a PCI Express
>> Root Port.  The PCI Express endpoint provides PCI-SIG SR-IOV
>> capabilities with a single physical function and a large number
>> of virtual functions.  The Root Port contains a pci-pci bridge in
>> between it and the SR-IOV device.
>>
>> When the SR-IOV capable device's driver tries to enable sriov
>> (pci_enable_sriov()) it fails to create/add PCI device for each
>> virtual function reporting  "not enough MMIO resources for SR-IOV".
>>
>> In sriov_enable() (drivers/pci/iov.c)
>>
>>  296        for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
>>  297                 bars |= (1 << (i + PCI_IOV_RESOURCES));
>>  298                 res = dev->resource + PCI_IOV_RESOURCES + i;
>>  299                 if (res->parent)
>>  300                         nres++;
>>  301         }
>>  302         if (nres != iov->nres) {
>>  303                 dev_err(&dev->dev, "not enough MMIO resources for
>>  SR-IOV\n");
>>  304                 return -ENOMEM;
>>  305         }
>>
>> Here its checking if physical function's IOV resource has a parent or not.
>> Which is pci-pci bridge in this case. Otherwise it doesn't consider
>> that resource.
>>
>> Added below api to your patch.
>> This will try to claim a resource while creating a PCI device which
>> inturn sets 'res->parent'.
>>
>> Let me know if this is okay.
>>
>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>> index 9f29c9a..fbfb48f 100644
>> --- a/arch/arm64/kernel/pci.c
>> +++ b/arch/arm64/kernel/pci.c
>> @@ -125,6 +125,21 @@ resource_size_t pcibios_align_resource(void
>> *data, const struct resource *res,
>>         return res->start;
>>  }
>>
>> +int pcibios_add_device(struct pci_dev *pdev)
>> +{
>> +        unsigned int i, type_mask = IORESOURCE_IO | IORESOURCE_MEM;
>> +        struct resource *res;
>> +
>> +        for (i = 0; i < PCI_NUM_RESOURCES; i++) {
>> +                res = &pdev->resource[i];
>> +                if (res->parent || !(res->flags & type_mask))
>> +                        continue;
>> +                pci_claim_resource(pdev, i);
>> +        }
>> +
>>
>
> I would like not to have to add your patch in this file as I am trying to
> remove it entirely. I don't have an SR-IOV capable device in my setup so
> I am not able to debug your problem, but could you have a go and try to
> figure out why the SR-IOV resources do not get a parent associated with
> when they get created?
>
> Many thanks,
> Liviu
>
>
> --
> ====================
> | I would like to |
> | fix the world,  |
> | but they're not |
> | giving me the   |
>  \ source code!  /
>   ---------------
>     ¯\_(ツ)_/¯
>

Re: [PATCH v7 0/3] Add support for PCI in AArch64

[Arnd Bergmann]

On Friday 16 May 2014 16:03:04 Sunil Kovvuri wrote:
> When the SR-IOV capable device's driver tries to enable sriov
> (pci_enable_sriov()) it fails to create/add PCI device for each
> virtual function reporting  "not enough MMIO resources for SR-IOV".

I assume you have checked that there is indeed enough MMIO space
available, right?

> In sriov_enable() (drivers/pci/iov.c)
> 
>  296        for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
>  297                 bars |= (1 << (i + PCI_IOV_RESOURCES));
>  298                 res = dev->resource + PCI_IOV_RESOURCES + i;
>  299                 if (res->parent)
>  300                         nres++;
>  301         }
>  302         if (nres != iov->nres) {
>  303                 dev_err(&dev->dev, "not enough MMIO resources for
>  SR-IOV\n");
>  304                 return -ENOMEM;
>  305         }
> 
> Here its checking if physical function's IOV resource has a parent or not.
> Which is pci-pci bridge in this case. Otherwise it doesn't consider
> that resource.
> 
> Added below api to your patch.
> This will try to claim a resource while creating a PCI device which
> inturn sets 'res->parent'.

This looks like the wrong approach. The PCI host controller should
really have been registered with the root 'iomem_resource' during
the probe of the host controller.

	Arnd

Re: [PATCH v7 0/3] Add support for PCI in AArch64

[Sunil Kovvuri]

On Mon, May 19, 2014 at 6:31 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 16 May 2014 16:03:04 Sunil Kovvuri wrote:
>> When the SR-IOV capable device's driver tries to enable sriov
>> (pci_enable_sriov()) it fails to create/add PCI device for each
>> virtual function reporting  "not enough MMIO resources for SR-IOV".
>
> I assume you have checked that there is indeed enough MMIO space
> available, right?
>
Yes, i have checked for enough MMIO space multiple times.

>> In sriov_enable() (drivers/pci/iov.c)
>>
>>  296        for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
>>  297                 bars |= (1 << (i + PCI_IOV_RESOURCES));
>>  298                 res = dev->resource + PCI_IOV_RESOURCES + i;
>>  299                 if (res->parent)
>>  300                         nres++;
>>  301         }
>>  302         if (nres != iov->nres) {
>>  303                 dev_err(&dev->dev, "not enough MMIO resources for
>>  SR-IOV\n");
>>  304                 return -ENOMEM;
>>  305         }
>>
>> Here its checking if physical function's IOV resource has a parent or not.
>> Which is pci-pci bridge in this case. Otherwise it doesn't consider
>> that resource.
>>
>> Added below api to your patch.
>> This will try to claim a resource while creating a PCI device which
>> inturn sets 'res->parent'.
>
> This looks like the wrong approach. The PCI host controller should
> really have been registered with the root 'iomem_resource' during
> the probe of the host controller.
>
>         Arnd
I didn't get this, if a SR-IOV device is connected to a PCI-PCI bridge
and inturn bridge connected to root port. Then the parent bus is not root,
but the bridge.  The issue is either hierarchy should not be checked for
SR-IOV resources or someone should set the hierarchy (i.e parent resources).

Regards,
Sunil.

Re: [PATCH v7 0/3] Add support for PCI in AArch64

[Arnd Bergmann]

On Tuesday 20 May 2014 09:52:33 Sunil Kovvuri wrote:
> >> In sriov_enable() (drivers/pci/iov.c)
> >>
> >>  296        for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> >>  297                 bars |= (1 << (i + PCI_IOV_RESOURCES));
> >>  298                 res = dev->resource + PCI_IOV_RESOURCES + i;
> >>  299                 if (res->parent)
> >>  300                         nres++;
> >>  301         }
> >>  302         if (nres != iov->nres) {
> >>  303                 dev_err(&dev->dev, "not enough MMIO resources for
> >>  SR-IOV\n");
> >>  304                 return -ENOMEM;
> >>  305         }
> >>
> >> Here its checking if physical function's IOV resource has a parent or not.
> >> Which is pci-pci bridge in this case. Otherwise it doesn't consider
> >> that resource.
> >>
> >> Added below api to your patch.
> >> This will try to claim a resource while creating a PCI device which
> >> inturn sets 'res->parent'.
> >
> > This looks like the wrong approach. The PCI host controller should
> > really have been registered with the root 'iomem_resource' during
> > the probe of the host controller.
> >
> I didn't get this, if a SR-IOV device is connected to a PCI-PCI bridge
> and inturn bridge connected to root port. Then the parent bus is not root,
> but the bridge.  The issue is either hierarchy should not be checked for
> SR-IOV resources or someone should set the hierarchy (i.e parent resources).

Ah, I misunderstood the problem, I thought the PCI core was complaining
about lack of space in the resources, not about a lack of BARs.

It seems there is code like yours in a couple of architectures, but they
only claim the resources of bridges, not the actual devices as you
seem to be doing. Can you check if the x86 version of
pcibios_allocate_bus_resources() does the trick for you? Maybe we can
turn that into a generic helper.

	Arnd

Re: [PATCH v7 0/3] Add support for PCI in AArch64

[Sunil Kovvuri]

On Tue, May 20, 2014 at 2:14 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Tuesday 20 May 2014 09:52:33 Sunil Kovvuri wrote:
>> >> In sriov_enable() (drivers/pci/iov.c)
>> >>
>> >>  296        for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
>> >>  297                 bars |= (1 << (i + PCI_IOV_RESOURCES));
>> >>  298                 res = dev->resource + PCI_IOV_RESOURCES + i;
>> >>  299                 if (res->parent)
>> >>  300                         nres++;
>> >>  301         }
>> >>  302         if (nres != iov->nres) {
>> >>  303                 dev_err(&dev->dev, "not enough MMIO resources for
>> >>  SR-IOV\n");
>> >>  304                 return -ENOMEM;
>> >>  305         }
>> >>
>> >> Here its checking if physical function's IOV resource has a parent or not.
>> >> Which is pci-pci bridge in this case. Otherwise it doesn't consider
>> >> that resource.
>> >>
>> >> Added below api to your patch.
>> >> This will try to claim a resource while creating a PCI device which
>> >> inturn sets 'res->parent'.
>> >
>> > This looks like the wrong approach. The PCI host controller should
>> > really have been registered with the root 'iomem_resource' during
>> > the probe of the host controller.
>> >
>> I didn't get this, if a SR-IOV device is connected to a PCI-PCI bridge
>> and inturn bridge connected to root port. Then the parent bus is not root,
>> but the bridge.  The issue is either hierarchy should not be checked for
>> SR-IOV resources or someone should set the hierarchy (i.e parent resources).
>
> Ah, I misunderstood the problem, I thought the PCI core was complaining
> about lack of space in the resources, not about a lack of BARs.
>
> It seems there is code like yours in a couple of architectures, but they
> only claim the resources of bridges, not the actual devices as you
> seem to be doing. Can you check if the x86 version of
> pcibios_allocate_bus_resources() does the trick for you? Maybe we can
> turn that into a generic helper.
>
Thanks for the suggestion. Will try that once.
FYI, IA64 architecture does claim resources for devices as well.
arch/ia64/pci/pci.c  'pcibios_fixup_device_resources()'

Thanks,
Sunil.

Re: [PATCH v7 0/3] Add support for PCI in AArch64

[Sunil Kovvuri]

Hi Liviu,

Sorry for the trouble.
I got why 'res->parent' is not set in my case.
Basically my SR-IOV device has fixed resources, so resources will not
be allocated/assigned and hence parent resource is not set.
I will move the resource claiming to host controller driver as a fixup
so that parent resource hierarchy is set.

Thanks for the support.

Regards,
Sunil.


On Fri, May 16, 2014 at 11:12 PM, Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote:
> Hi Liviu,
>
> Issue may not be only with SR-IOV resources.
>
> I am not an expert with Linux PCI core. But i am trying to understand
> how even for a non SR-IOV capable device's resource, gets a parent
> associated with it.
>
> When a PCI device driver calls pci_enable_device() which inturn calls
> 'arch/arm64/kernel/pci.c' pcibios_enable_device() which uses Linux PCI core
> API 'pci_enable_resources'. This API checks 'res->parent' for all
> valid resources
> before enabling them, otherwise it fails.
>                 if (!r->parent) {
>                         dev_err(&dev->dev, "device not available "
>                                 "(can't reserve %pR)\n", r);
>                         return -EINVAL;
>                 }
>
> Can you please let me know where this hierarchy is being set for
> 'pcibios_enable_resources' to work.
>
> I could only find request_resource() API which sets the parent and
> pci_claim_resource() uses this.
>
> Thanks,
> Sunil.
>
> On Fri, May 16, 2014 at 6:54 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
>> Hi Sunil,
>>
>> On Fri, May 16, 2014 at 11:33:04AM +0100, Sunil Kovvuri wrote:
>>> Hi Liviu,
>>>
>>> I am using your ARM64 PCIe patches to write a PCIe host controller
>>> driver for our SOC. I am facing an issue with SR-IOV capable device.
>>>
>>> Consider an PCI Express endpoint connected to a PCI Express
>>> Root Port.  The PCI Express endpoint provides PCI-SIG SR-IOV
>>> capabilities with a single physical function and a large number
>>> of virtual functions.  The Root Port contains a pci-pci bridge in
>>> between it and the SR-IOV device.
>>>
>>> When the SR-IOV capable device's driver tries to enable sriov
>>> (pci_enable_sriov()) it fails to create/add PCI device for each
>>> virtual function reporting  "not enough MMIO resources for SR-IOV".
>>>
>>> In sriov_enable() (drivers/pci/iov.c)
>>>
>>>  296        for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
>>>  297                 bars |= (1 << (i + PCI_IOV_RESOURCES));
>>>  298                 res = dev->resource + PCI_IOV_RESOURCES + i;
>>>  299                 if (res->parent)
>>>  300                         nres++;
>>>  301         }
>>>  302         if (nres != iov->nres) {
>>>  303                 dev_err(&dev->dev, "not enough MMIO resources for
>>>  SR-IOV\n");
>>>  304                 return -ENOMEM;
>>>  305         }
>>>
>>> Here its checking if physical function's IOV resource has a parent or not.
>>> Which is pci-pci bridge in this case. Otherwise it doesn't consider
>>> that resource.
>>>
>>> Added below api to your patch.
>>> This will try to claim a resource while creating a PCI device which
>>> inturn sets 'res->parent'.
>>>
>>> Let me know if this is okay.
>>>
>>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
>>> index 9f29c9a..fbfb48f 100644
>>> --- a/arch/arm64/kernel/pci.c
>>> +++ b/arch/arm64/kernel/pci.c
>>> @@ -125,6 +125,21 @@ resource_size_t pcibios_align_resource(void
>>> *data, const struct resource *res,
>>>         return res->start;
>>>  }
>>>
>>> +int pcibios_add_device(struct pci_dev *pdev)
>>> +{
>>> +        unsigned int i, type_mask = IORESOURCE_IO | IORESOURCE_MEM;
>>> +        struct resource *res;
>>> +
>>> +        for (i = 0; i < PCI_NUM_RESOURCES; i++) {
>>> +                res = &pdev->resource[i];
>>> +                if (res->parent || !(res->flags & type_mask))
>>> +                        continue;
>>> +                pci_claim_resource(pdev, i);
>>> +        }
>>> +
>>>
>>
>> I would like not to have to add your patch in this file as I am trying to
>> remove it entirely. I don't have an SR-IOV capable device in my setup so
>> I am not able to debug your problem, but could you have a go and try to
>> figure out why the SR-IOV resources do not get a parent associated with
>> when they get created?
>>
>> Many thanks,
>> Liviu
>>
>>
>> --
>> ====================
>> | I would like to |
>> | fix the world,  |
>> | but they're not |
>> | giving me the   |
>>  \ source code!  /
>>   ---------------
>>     ¯\_(ツ)_/¯
>>

Re: [PATCH v7 0/3] Add support for PCI in AArch64

[Liviu Dudau]

On Wed, May 21, 2014 at 04:45:29PM +0530, Sunil Kovvuri wrote:
> Hi Liviu,
> 
> Sorry for the trouble.
> I got why 'res->parent' is not set in my case.
> Basically my SR-IOV device has fixed resources, so resources will not
> be allocated/assigned and hence parent resource is not set.
> I will move the resource claiming to host controller driver as a fixup
> so that parent resource hierarchy is set.
> 
> Thanks for the support.

Glad you worked out the cause for the problem. I will still at to my list of
ToDo things to investigate resource parenting with my patchset.

Best regards,
Liviu

> 
> Regards,
> Sunil.
> 
> 
> On Fri, May 16, 2014 at 11:12 PM, Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote:
> > Hi Liviu,
> >
> > Issue may not be only with SR-IOV resources.
> >
> > I am not an expert with Linux PCI core. But i am trying to understand
> > how even for a non SR-IOV capable device's resource, gets a parent
> > associated with it.
> >
> > When a PCI device driver calls pci_enable_device() which inturn calls
> > 'arch/arm64/kernel/pci.c' pcibios_enable_device() which uses Linux PCI core
> > API 'pci_enable_resources'. This API checks 'res->parent' for all
> > valid resources
> > before enabling them, otherwise it fails.
> >                 if (!r->parent) {
> >                         dev_err(&dev->dev, "device not available "
> >                                 "(can't reserve %pR)\n", r);
> >                         return -EINVAL;
> >                 }
> >
> > Can you please let me know where this hierarchy is being set for
> > 'pcibios_enable_resources' to work.
> >
> > I could only find request_resource() API which sets the parent and
> > pci_claim_resource() uses this.
> >
> > Thanks,
> > Sunil.
> >
> > On Fri, May 16, 2014 at 6:54 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> >> Hi Sunil,
> >>
> >> On Fri, May 16, 2014 at 11:33:04AM +0100, Sunil Kovvuri wrote:
> >>> Hi Liviu,
> >>>
> >>> I am using your ARM64 PCIe patches to write a PCIe host controller
> >>> driver for our SOC. I am facing an issue with SR-IOV capable device.
> >>>
> >>> Consider an PCI Express endpoint connected to a PCI Express
> >>> Root Port.  The PCI Express endpoint provides PCI-SIG SR-IOV
> >>> capabilities with a single physical function and a large number
> >>> of virtual functions.  The Root Port contains a pci-pci bridge in
> >>> between it and the SR-IOV device.
> >>>
> >>> When the SR-IOV capable device's driver tries to enable sriov
> >>> (pci_enable_sriov()) it fails to create/add PCI device for each
> >>> virtual function reporting  "not enough MMIO resources for SR-IOV".
> >>>
> >>> In sriov_enable() (drivers/pci/iov.c)
> >>>
> >>>  296        for (i = 0; i < PCI_SRIOV_NUM_BARS; i++) {
> >>>  297                 bars |= (1 << (i + PCI_IOV_RESOURCES));
> >>>  298                 res = dev->resource + PCI_IOV_RESOURCES + i;
> >>>  299                 if (res->parent)
> >>>  300                         nres++;
> >>>  301         }
> >>>  302         if (nres != iov->nres) {
> >>>  303                 dev_err(&dev->dev, "not enough MMIO resources for
> >>>  SR-IOV\n");
> >>>  304                 return -ENOMEM;
> >>>  305         }
> >>>
> >>> Here its checking if physical function's IOV resource has a parent or not.
> >>> Which is pci-pci bridge in this case. Otherwise it doesn't consider
> >>> that resource.
> >>>
> >>> Added below api to your patch.
> >>> This will try to claim a resource while creating a PCI device which
> >>> inturn sets 'res->parent'.
> >>>
> >>> Let me know if this is okay.
> >>>
> >>> diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
> >>> index 9f29c9a..fbfb48f 100644
> >>> --- a/arch/arm64/kernel/pci.c
> >>> +++ b/arch/arm64/kernel/pci.c
> >>> @@ -125,6 +125,21 @@ resource_size_t pcibios_align_resource(void
> >>> *data, const struct resource *res,
> >>>         return res->start;
> >>>  }
> >>>
> >>> +int pcibios_add_device(struct pci_dev *pdev)
> >>> +{
> >>> +        unsigned int i, type_mask = IORESOURCE_IO | IORESOURCE_MEM;
> >>> +        struct resource *res;
> >>> +
> >>> +        for (i = 0; i < PCI_NUM_RESOURCES; i++) {
> >>> +                res = &pdev->resource[i];
> >>> +                if (res->parent || !(res->flags & type_mask))
> >>> +                        continue;
> >>> +                pci_claim_resource(pdev, i);
> >>> +        }
> >>> +
> >>>
> >>
> >> I would like not to have to add your patch in this file as I am trying to
> >> remove it entirely. I don't have an SR-IOV capable device in my setup so
> >> I am not able to debug your problem, but could you have a go and try to
> >> figure out why the SR-IOV resources do not get a parent associated with
> >> when they get created?
> >>
> >> Many thanks,
> >> Liviu
> >>
> >>
> >> --
> >> ====================
> >> | I would like to |
> >> | fix the world,  |
> >> | but they're not |
> >> | giving me the   |
> >>  \ source code!  /
> >>   ---------------
> >>     ¯\_(ツ)_/¯
> >>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
-------------------
   .oooO
   (   )
    \ (  Oooo.
     \_) (   )
          ) /
         (_/

 One small step
   for me ...

Re: [PATCH v7 0/3] Add support for PCI in AArch64

[Jason Gunthorpe]

On Wed, May 21, 2014 at 12:34:21PM +0100, Liviu Dudau wrote:
> On Wed, May 21, 2014 at 04:45:29PM +0530, Sunil Kovvuri wrote:
> > Hi Liviu,
> > 
> > Sorry for the trouble.
> > I got why 'res->parent' is not set in my case.
> > Basically my SR-IOV device has fixed resources, so resources will not
> > be allocated/assigned and hence parent resource is not set.
> > I will move the resource claiming to host controller driver as a fixup
> > so that parent resource hierarchy is set.
> > 
> > Thanks for the support.
> 
> Glad you worked out the cause for the problem. I will still at to my list of
> ToDo things to investigate resource parenting with my patchset.

We recently fixed some things in this area on mvebu. It is important
to ensure that the aperature in the host driver has a proper resource
associated with it, or the PCI core won't create sub resources.

commit 2613ba480fb7b40c67eea36d03c9946977828623
Author: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Date:   Wed Feb 12 15:57:08 2014 -0700

    PCI: mvebu: Call request_resource() on the apertures
    
    It is typical for host drivers to request a resource for the aperture; once
    this is done the PCI core will properly populate resources for all BARs in
    the system.
    
    With this patch cat /proc/iomem will now show:
    
      e0000000-efffffff : PCI MEM 0000
        e0000000-e00fffff : PCI Bus 0000:01
          e0000000-e001ffff : 0000:01:00.0
    
    Tested on Kirkwood.
    
    Signed-off-by: Arnd Bergmann <arnd@arndb.de>
    Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
    Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
    Acked-by: Jason Cooper <jason@lakedaemon.net>

diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c
index 05e352889868..d3d1cfd51e09 100644
--- a/drivers/pci/host/pci-mvebu.c
+++ b/drivers/pci/host/pci-mvebu.c
@@ -101,7 +101,9 @@ struct mvebu_pcie {
        struct mvebu_pcie_port *ports;
        struct msi_chip *msi;
        struct resource io;
+       char io_name[30];
        struct resource realio;
+       char mem_name[30];
        struct resource mem;
        struct resource busn;
        int nports;
@@ -672,10 +674,30 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys)
 {
        struct mvebu_pcie *pcie = sys_to_pcie(sys);
        int i;
+       int domain = 0;
 
-       if (resource_size(&pcie->realio) != 0)
+#ifdef CONFIG_PCI_DOMAINS
+       domain = sys->domain;
+#endif
+
+       snprintf(pcie->mem_name, sizeof(pcie->mem_name), "PCI MEM %04x",
+                domain);
+       pcie->mem.name = pcie->mem_name;
+
+       snprintf(pcie->io_name, sizeof(pcie->io_name), "PCI I/O %04x", domain);
+       pcie->realio.name = pcie->io_name;
+
+       if (request_resource(&iomem_resource, &pcie->mem))
+               return 0;
+
+       if (resource_size(&pcie->realio) != 0) {
+               if (request_resource(&ioport_resource, &pcie->realio)) {
+                       release_resource(&pcie->mem);
+                       return 0;
+               }
                pci_add_resource_offset(&sys->resources, &pcie->realio,
                                        sys->io_offset);
+       }
        pci_add_resource_offset(&sys->resources, &pcie->mem, sys->mem_offset);
        pci_add_resource(&sys->resources, &pcie->busn);