[PATCH v6 0/6] [RFC] Support for creating generic host_bridge from device tree

[PATCH v6 0/6] [RFC] Support for creating generic host_bridge from device tree

[Liviu Dudau]

This is v6 of my attempt to add support for a generic pci_host_bridge controller created
from a description passed in the device tree.

Changes from v5:
  - Tested by Tanmay Inamdar, thanks Tanmay!
  - dropped v5 5/7 pci: Use parent domain number when allocating child busses.
  - Added weak implementation of pcibios_fixup_bridge_ranges() in drivers/pci/host-bridge.c
    so that architectures that enable CONFIG_OF and CONFIG_PCI don't suddenly get compilation
    errors. While at this, changed the signature of the function so that an error can be
    returned.
  - With the new error code in pcibios_fixup_bridge_ranges(), reworked the error handling
    in pci_host_bridge_of_get_ranges() and of_create_pci_host_bridge().
  - Add linux/slab.h to the #include list
  - Revisit the error path in pci_create_root_bus[_in_domain]() and fixed the case where
    failing to allocate the bus will not return an error.

Changes from v4:
  - Export pci_find_host_bridge() to be used by arch code. There is scope for
    making the arch/arm64 version of pci_domain_nr the default weak implementation
    but that would double the size of this series in order to handle all #define
    versions of the pci_domain_nr() function, so I suggest keeping that for a separate
    cleanup series.

Changes from v3:
  - Dynamically allocate bus_range resource in of_create_pci_host_bridge()
  - Fix the domain number used when creating child busses.
  - Changed domain number allocator to use atomic operations.
  - Use ERR_PTR() to propagate the error out of pci_create_root_bus_in_domain()
    and of_create_pci_host_bridge().

Changes from v2:
  - Use range->cpu_addr when calling pci_address_to_pio()
  - Introduce pci_register_io_range() helper function in order to register
    io ranges ahead of their conversion to PIO values. This is needed as no
    information is being stored yet regarding the range mapping, making
    pci_address_to_pio() fail. Default weak implementation does nothing,
    to cover the default weak implementation of pci_address_to_pio() that
    expects direct mapping of physical addresses into PIO values (x86 view).

Changes from v1:
  - Add patch to fix conversion of IO ranges into IO resources.
  - Added a domain_nr member to pci_host_bridge structure, and a new function
    to create a root bus in a given domain number. In order to facilitate that
    I propose changing the order of initialisation between pci_host_bridge and
    it's related bus in pci_create_root_bus() as sort of a rever of 7b5436635800.
    This is done in patch 1/4 and 2/4.
  - Added a simple allocator of domain numbers in drivers/pci/host-bridge.c. The
    code will first try to get a domain id from of_alias_get_id(..., "pci-domain")
    and if that fails assign the next unallocated domain id.
  - Changed the name of the function that creates the generic host bridge from
    pci_host_bridge_of_init to of_create_pci_host_bridge and exported as GPL symbol.

v5 thread here: https://lkml.org/lkml/2014/3/4/318
v4 thread here: https://lkml.org/lkml/2014/3/3/301
v3 thread here: https://lkml.org/lkml/2014/2/28/216
v2 thread here: https://lkml.org/lkml/2014/2/27/245
v1 thread here: https://lkml.org/lkml/2014/2/3/380

Best regards,
Liviu

Liviu Dudau (6):
  pci: Introduce pci_register_io_range() helper function.
  pci: OF: Fix the conversion of IO ranges into IO resources.
  pci: Create pci_host_bridge before its associated bus in pci_create_root_bus.
  pci: Introduce a domain number for pci_host_bridge.
  pci: Export find_pci_host_bridge() function.
  pci: Add support for creating a generic host_bridge from device tree

 drivers/of/address.c       |  39 +++++++++++
 drivers/pci/host-bridge.c  | 159 ++++++++++++++++++++++++++++++++++++++++++++-
 drivers/pci/probe.c        |  70 +++++++++++++-------
 include/linux/of_address.h |  14 +---
 include/linux/pci.h        |  17 +++++
 5 files changed, 264 insertions(+), 35 deletions(-)

-- 
1.9.0

[PATCH v6 0/6] [RFC] Support for creating generic host_bridge from device tree

[Liviu Dudau]

This is v6 of my attempt to add support for a generic pci_host_bridge controller created
from a description passed in the device tree.

Changes from v5:
  - Tested by Tanmay Inamdar, thanks Tanmay!
  - dropped v5 5/7 pci: Use parent domain number when allocating child busses.
  - Added weak implementation of pcibios_fixup_bridge_ranges() in drivers/pci/host-bridge.c
    so that architectures that enable CONFIG_OF and CONFIG_PCI don't suddenly get compilation
    errors. While at this, changed the signature of the function so that an error can be
    returned.
  - With the new error code in pcibios_fixup_bridge_ranges(), reworked the error handling
    in pci_host_bridge_of_get_ranges() and of_create_pci_host_bridge().
  - Add linux/slab.h to the #include list
  - Revisit the error path in pci_create_root_bus[_in_domain]() and fixed the case where
    failing to allocate the bus will not return an error.

Changes from v4:
  - Export pci_find_host_bridge() to be used by arch code. There is scope for
    making the arch/arm64 version of pci_domain_nr the default weak implementation
    but that would double the size of this series in order to handle all #define
    versions of the pci_domain_nr() function, so I suggest keeping that for a separate
    cleanup series.

Changes from v3:
  - Dynamically allocate bus_range resource in of_create_pci_host_bridge()
  - Fix the domain number used when creating child busses.
  - Changed domain number allocator to use atomic operations.
  - Use ERR_PTR() to propagate the error out of pci_create_root_bus_in_domain()
    and of_create_pci_host_bridge().

Changes from v2:
  - Use range->cpu_addr when calling pci_address_to_pio()
  - Introduce pci_register_io_range() helper function in order to register
    io ranges ahead of their conversion to PIO values. This is needed as no
    information is being stored yet regarding the range mapping, making
    pci_address_to_pio() fail. Default weak implementation does nothing,
    to cover the default weak implementation of pci_address_to_pio() that
    expects direct mapping of physical addresses into PIO values (x86 view).

Changes from v1:
  - Add patch to fix conversion of IO ranges into IO resources.
  - Added a domain_nr member to pci_host_bridge structure, and a new function
    to create a root bus in a given domain number. In order to facilitate that
    I propose changing the order of initialisation between pci_host_bridge and
    it's related bus in pci_create_root_bus() as sort of a rever of 7b5436635800.
    This is done in patch 1/4 and 2/4.
  - Added a simple allocator of domain numbers in drivers/pci/host-bridge.c. The
    code will first try to get a domain id from of_alias_get_id(..., "pci-domain")
    and if that fails assign the next unallocated domain id.
  - Changed the name of the function that creates the generic host bridge from
    pci_host_bridge_of_init to of_create_pci_host_bridge and exported as GPL symbol.

v5 thread here: https://lkml.org/lkml/2014/3/4/318
v4 thread here: https://lkml.org/lkml/2014/3/3/301
v3 thread here: https://lkml.org/lkml/2014/2/28/216
v2 thread here: https://lkml.org/lkml/2014/2/27/245
v1 thread here: https://lkml.org/lkml/2014/2/3/380

Best regards,
Liviu

Liviu Dudau (6):
  pci: Introduce pci_register_io_range() helper function.
  pci: OF: Fix the conversion of IO ranges into IO resources.
  pci: Create pci_host_bridge before its associated bus in pci_create_root_bus.
  pci: Introduce a domain number for pci_host_bridge.
  pci: Export find_pci_host_bridge() function.
  pci: Add support for creating a generic host_bridge from device tree

 drivers/of/address.c       |  39 +++++++++++
 drivers/pci/host-bridge.c  | 159 ++++++++++++++++++++++++++++++++++++++++++++-
 drivers/pci/probe.c        |  70 +++++++++++++-------
 include/linux/of_address.h |  14 +---
 include/linux/pci.h        |  17 +++++
 5 files changed, 264 insertions(+), 35 deletions(-)

-- 
1.9.0

[PATCH v6 1/6] pci: Introduce pci_register_io_range() helper function.

[Liviu Dudau]

Some architectures do not share x86 simple view of the I/O space and
instead use a range of addresses that map to external devices. For PCI,
these ranges can be expressed by OF bindings in a device tree file.

Introduce a pci_register_io_range() helper function that can be used
by the architecture code to keep track of the io ranges described by the
PCI bindings.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Tested-by: Tanmay Inamdar <tinamdar@apm.com>
---
 drivers/of/address.c       | 5 +++++
 include/linux/of_address.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 1a54f1f..d1bb30f 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -619,6 +619,11 @@ const __be32 *of_get_address(struct device_node *dev, int index, u64 *size,
 }
 EXPORT_SYMBOL(of_get_address);
 
+int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
+{
+	return 0;
+}
+
 unsigned long __weak pci_address_to_pio(phys_addr_t address)
 {
 	if (address > IO_SPACE_LIMIT)
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 5f6ed6b..40c418d 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -56,6 +56,7 @@ extern void __iomem *of_iomap(struct device_node *device, int index);
 extern const __be32 *of_get_address(struct device_node *dev, int index,
 			   u64 *size, unsigned int *flags);
 
+extern int pci_register_io_range(phys_addr_t addr, resource_size_t size);
 extern unsigned long pci_address_to_pio(phys_addr_t addr);
 
 extern int of_pci_range_parser_init(struct of_pci_range_parser *parser,
-- 
1.9.0

[PATCH v6 1/6] pci: Introduce pci_register_io_range() helper function.

[Liviu Dudau]

Some architectures do not share x86 simple view of the I/O space and
instead use a range of addresses that map to external devices. For PCI,
these ranges can be expressed by OF bindings in a device tree file.

Introduce a pci_register_io_range() helper function that can be used
by the architecture code to keep track of the io ranges described by the
PCI bindings.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Tested-by: Tanmay Inamdar <tinamdar@apm.com>
---
 drivers/of/address.c       | 5 +++++
 include/linux/of_address.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 1a54f1f..d1bb30f 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -619,6 +619,11 @@ const __be32 *of_get_address(struct device_node *dev, int index, u64 *size,
 }
 EXPORT_SYMBOL(of_get_address);
 
+int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
+{
+	return 0;
+}
+
 unsigned long __weak pci_address_to_pio(phys_addr_t address)
 {
 	if (address > IO_SPACE_LIMIT)
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 5f6ed6b..40c418d 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -56,6 +56,7 @@ extern void __iomem *of_iomap(struct device_node *device, int index);
 extern const __be32 *of_get_address(struct device_node *dev, int index,
 			   u64 *size, unsigned int *flags);
 
+extern int pci_register_io_range(phys_addr_t addr, resource_size_t size);
 extern unsigned long pci_address_to_pio(phys_addr_t addr);
 
 extern int of_pci_range_parser_init(struct of_pci_range_parser *parser,
-- 
1.9.0

[PATCH v6 1/6] pci: Introduce pci_register_io_range() helper function.

[Liviu Dudau]

Some architectures do not share x86 simple view of the I/O space and
instead use a range of addresses that map to external devices. For PCI,
these ranges can be expressed by OF bindings in a device tree file.

Introduce a pci_register_io_range() helper function that can be used
by the architecture code to keep track of the io ranges described by the
PCI bindings.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Tested-by: Tanmay Inamdar <tinamdar@apm.com>
---
 drivers/of/address.c       | 5 +++++
 include/linux/of_address.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 1a54f1f..d1bb30f 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -619,6 +619,11 @@ const __be32 *of_get_address(struct device_node *dev, int index, u64 *size,
 }
 EXPORT_SYMBOL(of_get_address);
 
+int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
+{
+	return 0;
+}
+
 unsigned long __weak pci_address_to_pio(phys_addr_t address)
 {
 	if (address > IO_SPACE_LIMIT)
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 5f6ed6b..40c418d 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -56,6 +56,7 @@ extern void __iomem *of_iomap(struct device_node *device, int index);
 extern const __be32 *of_get_address(struct device_node *dev, int index,
 			   u64 *size, unsigned int *flags);
 
+extern int pci_register_io_range(phys_addr_t addr, resource_size_t size);
 extern unsigned long pci_address_to_pio(phys_addr_t addr);
 
 extern int of_pci_range_parser_init(struct of_pci_range_parser *parser,
-- 
1.9.0

[PATCH v6 2/6] pci: OF: Fix the conversion of IO ranges into IO resources.

[Liviu Dudau]

The ranges property for a host bridge controller in DT describes
the mapping between the PCI bus address and the CPU physical address.
The resources framework however expects that the IO resources start
at a pseudo "port" address 0 (zero) and have a maximum size of IO_SPACE_LIMIT.
The conversion from pci ranges to resources failed to take that into account.

In the process move the function into drivers/of/address.c as it
now depends on pci_address_to_pio() code.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Tested-by: Tanmay Inamdar <tinamdar@apm.com>
---
 drivers/of/address.c       | 34 ++++++++++++++++++++++++++++++++++
 include/linux/of_address.h | 13 ++-----------
 2 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index d1bb30f..d595d98 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -724,3 +724,37 @@ void __iomem *of_iomap(struct device_node *np, int index)
 	return ioremap(res.start, resource_size(&res));
 }
 EXPORT_SYMBOL(of_iomap);
+
+/**
+ * of_pci_range_to_resource - Create a resource from an of_pci_range
+ * @range:	the PCI range that describes the resource
+ * @np:		device node where the range belongs to
+ * @res:	pointer to a valid resource that will be updated to
+ *              reflect the values contained in the range.
+ * Note that if the range is an IO range, the resource will be converted
+ * using pci_address_to_pio() which can fail if it is called too early or
+ * if the range cannot be matched to any host bridge IO space (our case here).
+ * To guard against that we try to register the IO range first.
+ * If that fails we know that pci_address_to_pio() will do too.
+ */
+void of_pci_range_to_resource(struct of_pci_range *range,
+	struct device_node *np, struct resource *res)
+{
+	res->flags = range->flags;
+	if (res->flags & IORESOURCE_IO) {
+		unsigned long port = -1;
+		if (!pci_register_io_range(range->cpu_addr, range->size))
+			port = pci_address_to_pio(range->cpu_addr);
+		if (port == (unsigned long)-1) {
+			res->start = (resource_size_t)OF_BAD_ADDR;
+			res->end = (resource_size_t)OF_BAD_ADDR;
+			return;
+		}
+		res->start = port;
+	} else {
+		res->start = range->cpu_addr;
+	}
+	res->end = res->start + range->size - 1;
+	res->parent = res->child = res->sibling = NULL;
+	res->name = np->full_name;
+}
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 40c418d..3fe500a 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -23,17 +23,8 @@ struct of_pci_range {
 #define for_each_of_pci_range(parser, range) \
 	for (; of_pci_range_parser_one(parser, range);)
 
-static inline void of_pci_range_to_resource(struct of_pci_range *range,
-					    struct device_node *np,
-					    struct resource *res)
-{
-	res->flags = range->flags;
-	res->start = range->cpu_addr;
-	res->end = range->cpu_addr + range->size - 1;
-	res->parent = res->child = res->sibling = NULL;
-	res->name = np->full_name;
-}
-
+extern void of_pci_range_to_resource(struct of_pci_range *range,
+		struct device_node *np, struct resource *res);
 /* Translate a DMA address from device space to CPU space */
 extern u64 of_translate_dma_address(struct device_node *dev,
 				    const __be32 *in_addr);
-- 
1.9.0

[PATCH v6 2/6] pci: OF: Fix the conversion of IO ranges into IO resources.

[Liviu Dudau]

The ranges property for a host bridge controller in DT describes
the mapping between the PCI bus address and the CPU physical address.
The resources framework however expects that the IO resources start
at a pseudo "port" address 0 (zero) and have a maximum size of IO_SPACE_LIMIT.
The conversion from pci ranges to resources failed to take that into account.

In the process move the function into drivers/of/address.c as it
now depends on pci_address_to_pio() code.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Tested-by: Tanmay Inamdar <tinamdar@apm.com>
---
 drivers/of/address.c       | 34 ++++++++++++++++++++++++++++++++++
 include/linux/of_address.h | 13 ++-----------
 2 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index d1bb30f..d595d98 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -724,3 +724,37 @@ void __iomem *of_iomap(struct device_node *np, int index)
 	return ioremap(res.start, resource_size(&res));
 }
 EXPORT_SYMBOL(of_iomap);
+
+/**
+ * of_pci_range_to_resource - Create a resource from an of_pci_range
+ * @range:	the PCI range that describes the resource
+ * @np:		device node where the range belongs to
+ * @res:	pointer to a valid resource that will be updated to
+ *              reflect the values contained in the range.
+ * Note that if the range is an IO range, the resource will be converted
+ * using pci_address_to_pio() which can fail if it is called too early or
+ * if the range cannot be matched to any host bridge IO space (our case here).
+ * To guard against that we try to register the IO range first.
+ * If that fails we know that pci_address_to_pio() will do too.
+ */
+void of_pci_range_to_resource(struct of_pci_range *range,
+	struct device_node *np, struct resource *res)
+{
+	res->flags = range->flags;
+	if (res->flags & IORESOURCE_IO) {
+		unsigned long port = -1;
+		if (!pci_register_io_range(range->cpu_addr, range->size))
+			port = pci_address_to_pio(range->cpu_addr);
+		if (port == (unsigned long)-1) {
+			res->start = (resource_size_t)OF_BAD_ADDR;
+			res->end = (resource_size_t)OF_BAD_ADDR;
+			return;
+		}
+		res->start = port;
+	} else {
+		res->start = range->cpu_addr;
+	}
+	res->end = res->start + range->size - 1;
+	res->parent = res->child = res->sibling = NULL;
+	res->name = np->full_name;
+}
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 40c418d..3fe500a 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -23,17 +23,8 @@ struct of_pci_range {
 #define for_each_of_pci_range(parser, range) \
 	for (; of_pci_range_parser_one(parser, range);)
 
-static inline void of_pci_range_to_resource(struct of_pci_range *range,
-					    struct device_node *np,
-					    struct resource *res)
-{
-	res->flags = range->flags;
-	res->start = range->cpu_addr;
-	res->end = range->cpu_addr + range->size - 1;
-	res->parent = res->child = res->sibling = NULL;
-	res->name = np->full_name;
-}
-
+extern void of_pci_range_to_resource(struct of_pci_range *range,
+		struct device_node *np, struct resource *res);
 /* Translate a DMA address from device space to CPU space */
 extern u64 of_translate_dma_address(struct device_node *dev,
 				    const __be32 *in_addr);
-- 
1.9.0

[PATCH v6 2/6] pci: OF: Fix the conversion of IO ranges into IO resources.

[Liviu Dudau]

The ranges property for a host bridge controller in DT describes
the mapping between the PCI bus address and the CPU physical address.
The resources framework however expects that the IO resources start
at a pseudo "port" address 0 (zero) and have a maximum size of IO_SPACE_LIMIT.
The conversion from pci ranges to resources failed to take that into account.

In the process move the function into drivers/of/address.c as it
now depends on pci_address_to_pio() code.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Tested-by: Tanmay Inamdar <tinamdar@apm.com>
---
 drivers/of/address.c       | 34 ++++++++++++++++++++++++++++++++++
 include/linux/of_address.h | 13 ++-----------
 2 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index d1bb30f..d595d98 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -724,3 +724,37 @@ void __iomem *of_iomap(struct device_node *np, int index)
 	return ioremap(res.start, resource_size(&res));
 }
 EXPORT_SYMBOL(of_iomap);
+
+/**
+ * of_pci_range_to_resource - Create a resource from an of_pci_range
+ * @range:	the PCI range that describes the resource
+ * @np:		device node where the range belongs to
+ * @res:	pointer to a valid resource that will be updated to
+ *              reflect the values contained in the range.
+ * Note that if the range is an IO range, the resource will be converted
+ * using pci_address_to_pio() which can fail if it is called too early or
+ * if the range cannot be matched to any host bridge IO space (our case here).
+ * To guard against that we try to register the IO range first.
+ * If that fails we know that pci_address_to_pio() will do too.
+ */
+void of_pci_range_to_resource(struct of_pci_range *range,
+	struct device_node *np, struct resource *res)
+{
+	res->flags = range->flags;
+	if (res->flags & IORESOURCE_IO) {
+		unsigned long port = -1;
+		if (!pci_register_io_range(range->cpu_addr, range->size))
+			port = pci_address_to_pio(range->cpu_addr);
+		if (port == (unsigned long)-1) {
+			res->start = (resource_size_t)OF_BAD_ADDR;
+			res->end = (resource_size_t)OF_BAD_ADDR;
+			return;
+		}
+		res->start = port;
+	} else {
+		res->start = range->cpu_addr;
+	}
+	res->end = res->start + range->size - 1;
+	res->parent = res->child = res->sibling = NULL;
+	res->name = np->full_name;
+}
diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 40c418d..3fe500a 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h
@@ -23,17 +23,8 @@ struct of_pci_range {
 #define for_each_of_pci_range(parser, range) \
 	for (; of_pci_range_parser_one(parser, range);)
 
-static inline void of_pci_range_to_resource(struct of_pci_range *range,
-					    struct device_node *np,
-					    struct resource *res)
-{
-	res->flags = range->flags;
-	res->start = range->cpu_addr;
-	res->end = range->cpu_addr + range->size - 1;
-	res->parent = res->child = res->sibling = NULL;
-	res->name = np->full_name;
-}
-
+extern void of_pci_range_to_resource(struct of_pci_range *range,
+		struct device_node *np, struct resource *res);
 /* Translate a DMA address from device space to CPU space */
 extern u64 of_translate_dma_address(struct device_node *dev,
 				    const __be32 *in_addr);
-- 
1.9.0

[PATCH v6 3/6] pci: Create pci_host_bridge before its associated bus in pci_create_root_bus.

[Liviu Dudau]

Before commit 7b5436635800 the pci_host_bridge was created before the root bus.
As that commit has added a needless dependency on the bus for pci_alloc_host_bridge()
the creation order has been changed for no good reason. Revert the order of
creation as we are going to depend on the pci_host_bridge structure to retrieve the
domain number of the root bus.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Tested-by: Tanmay Inamdar <tinamdar@apm.com>
---
 drivers/pci/probe.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6e34498..fd11c12 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -505,7 +505,7 @@ static void pci_release_host_bridge_dev(struct device *dev)
 	kfree(bridge);
 }
 
-static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
+static struct pci_host_bridge *pci_alloc_host_bridge(void)
 {
 	struct pci_host_bridge *bridge;
 
@@ -514,7 +514,6 @@ static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
 		return NULL;
 
 	INIT_LIST_HEAD(&bridge->windows);
-	bridge->bus = b;
 	return bridge;
 }
 
@@ -1727,9 +1726,19 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 	char bus_addr[64];
 	char *fmt;
 
+	bridge = pci_alloc_host_bridge();
+	if (!bridge)
+		return NULL;
+
+	bridge->dev.parent = parent;
+	bridge->dev.release = pci_release_host_bridge_dev;
+	error = pcibios_root_bridge_prepare(bridge);
+	if (error)
+		goto err_out;
+
 	b = pci_alloc_bus();
 	if (!b)
-		return NULL;
+		goto err_out;
 
 	b->sysdata = sysdata;
 	b->ops = ops;
@@ -1738,26 +1747,15 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 	if (b2) {
 		/* If we already got to this bus through a different bridge, ignore it */
 		dev_dbg(&b2->dev, "bus already known\n");
-		goto err_out;
+		goto err_bus_out;
 	}
 
-	bridge = pci_alloc_host_bridge(b);
-	if (!bridge)
-		goto err_out;
-
-	bridge->dev.parent = parent;
-	bridge->dev.release = pci_release_host_bridge_dev;
+	bridge->bus = b;
 	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
-	error = pcibios_root_bridge_prepare(bridge);
-	if (error) {
-		kfree(bridge);
-		goto err_out;
-	}
-
 	error = device_register(&bridge->dev);
 	if (error) {
 		put_device(&bridge->dev);
-		goto err_out;
+		goto err_bus_out;
 	}
 	b->bridge = get_device(&bridge->dev);
 	device_enable_async_suspend(b->bridge);
@@ -1814,8 +1812,10 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 class_dev_reg_err:
 	put_device(&bridge->dev);
 	device_unregister(&bridge->dev);
-err_out:
+err_bus_out:
 	kfree(b);
+err_out:
+	kfree(bridge);
 	return NULL;
 }
 
-- 
1.9.0

[PATCH v6 3/6] pci: Create pci_host_bridge before its associated bus in pci_create_root_bus.

[Liviu Dudau]

Before commit 7b5436635800 the pci_host_bridge was created before the root bus.
As that commit has added a needless dependency on the bus for pci_alloc_host_bridge()
the creation order has been changed for no good reason. Revert the order of
creation as we are going to depend on the pci_host_bridge structure to retrieve the
domain number of the root bus.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Tested-by: Tanmay Inamdar <tinamdar@apm.com>
---
 drivers/pci/probe.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 6e34498..fd11c12 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -505,7 +505,7 @@ static void pci_release_host_bridge_dev(struct device *dev)
 	kfree(bridge);
 }
 
-static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
+static struct pci_host_bridge *pci_alloc_host_bridge(void)
 {
 	struct pci_host_bridge *bridge;
 
@@ -514,7 +514,6 @@ static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
 		return NULL;
 
 	INIT_LIST_HEAD(&bridge->windows);
-	bridge->bus = b;
 	return bridge;
 }
 
@@ -1727,9 +1726,19 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 	char bus_addr[64];
 	char *fmt;
 
+	bridge = pci_alloc_host_bridge();
+	if (!bridge)
+		return NULL;
+
+	bridge->dev.parent = parent;
+	bridge->dev.release = pci_release_host_bridge_dev;
+	error = pcibios_root_bridge_prepare(bridge);
+	if (error)
+		goto err_out;
+
 	b = pci_alloc_bus();
 	if (!b)
-		return NULL;
+		goto err_out;
 
 	b->sysdata = sysdata;
 	b->ops = ops;
@@ -1738,26 +1747,15 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 	if (b2) {
 		/* If we already got to this bus through a different bridge, ignore it */
 		dev_dbg(&b2->dev, "bus already known\n");
-		goto err_out;
+		goto err_bus_out;
 	}
 
-	bridge = pci_alloc_host_bridge(b);
-	if (!bridge)
-		goto err_out;
-
-	bridge->dev.parent = parent;
-	bridge->dev.release = pci_release_host_bridge_dev;
+	bridge->bus = b;
 	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
-	error = pcibios_root_bridge_prepare(bridge);
-	if (error) {
-		kfree(bridge);
-		goto err_out;
-	}
-
 	error = device_register(&bridge->dev);
 	if (error) {
 		put_device(&bridge->dev);
-		goto err_out;
+		goto err_bus_out;
 	}
 	b->bridge = get_device(&bridge->dev);
 	device_enable_async_suspend(b->bridge);
@@ -1814,8 +1812,10 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 class_dev_reg_err:
 	put_device(&bridge->dev);
 	device_unregister(&bridge->dev);
-err_out:
+err_bus_out:
 	kfree(b);
+err_out:
+	kfree(bridge);
 	return NULL;
 }
 
-- 
1.9.0

[PATCH v6 4/6] pci: Introduce a domain number for pci_host_bridge.

[Liviu Dudau]

Make it easier to discover the domain number of a bus by storing
the number in pci_host_bridge for the root bus. Several architectures
have their own way of storing this information, so it makes sense
to try to unify the code. While at this, add a new function that
creates a root bus in a given domain and make pci_create_root_bus()
a wrapper around this function.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Tested-by: Tanmay Inamdar <tinamdar@apm.com>
---
 drivers/pci/probe.c | 41 +++++++++++++++++++++++++++++++++--------
 include/linux/pci.h |  4 ++++
 2 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index fd11c12..172c615 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1714,8 +1714,9 @@ void __weak pcibios_remove_bus(struct pci_bus *bus)
 {
 }
 
-struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
-		struct pci_ops *ops, void *sysdata, struct list_head *resources)
+struct pci_bus *pci_create_root_bus_in_domain(struct device *parent,
+		int domain, int bus, struct pci_ops *ops, void *sysdata,
+		struct list_head *resources)
 {
 	int error;
 	struct pci_host_bridge *bridge;
@@ -1728,30 +1729,34 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 
 	bridge = pci_alloc_host_bridge();
 	if (!bridge)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	bridge->dev.parent = parent;
 	bridge->dev.release = pci_release_host_bridge_dev;
+	bridge->domain_nr = domain;
 	error = pcibios_root_bridge_prepare(bridge);
 	if (error)
 		goto err_out;
 
 	b = pci_alloc_bus();
-	if (!b)
+	if (!b) {
+		error = -ENOMEM;
 		goto err_out;
+	}
 
 	b->sysdata = sysdata;
 	b->ops = ops;
 	b->number = b->busn_res.start = bus;
-	b2 = pci_find_bus(pci_domain_nr(b), bus);
+	b2 = pci_find_bus(bridge->domain_nr, bus);
 	if (b2) {
 		/* If we already got to this bus through a different bridge, ignore it */
 		dev_dbg(&b2->dev, "bus already known\n");
+		error = -EEXIST;
 		goto err_bus_out;
 	}
 
 	bridge->bus = b;
-	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
+	dev_set_name(&bridge->dev, "pci%04x:%02x", bridge->domain_nr, bus);
 	error = device_register(&bridge->dev);
 	if (error) {
 		put_device(&bridge->dev);
@@ -1766,7 +1771,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 
 	b->dev.class = &pcibus_class;
 	b->dev.parent = b->bridge;
-	dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus);
+	dev_set_name(&b->dev, "%04x:%02x", bridge->domain_nr, bus);
 	error = device_register(&b->dev);
 	if (error)
 		goto class_dev_reg_err;
@@ -1816,7 +1821,27 @@ err_bus_out:
 	kfree(b);
 err_out:
 	kfree(bridge);
-	return NULL;
+	return ERR_PTR(error);
+}
+
+struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
+		struct pci_ops *ops, void *sysdata, struct list_head *resources)
+{
+	int domain_nr;
+	struct pci_bus *b = pci_alloc_bus();
+	if (!b)
+		return NULL;
+
+	b->sysdata = sysdata;
+	domain_nr = pci_domain_nr(b);
+	kfree(b);
+
+	b = pci_create_root_bus_in_domain(parent, domain_nr, bus,
+				ops, sysdata, resources);
+	if (IS_ERR(b))
+		return NULL;
+
+	return b;
 }
 
 int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 33aa2ca..1eed009 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -394,6 +394,7 @@ struct pci_host_bridge_window {
 struct pci_host_bridge {
 	struct device dev;
 	struct pci_bus *bus;		/* root bus */
+	int domain_nr;
 	struct list_head windows;	/* pci_host_bridge_windows */
 	void (*release_fn)(struct pci_host_bridge *);
 	void *release_data;
@@ -747,6 +748,9 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata);
 struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 				    struct pci_ops *ops, void *sysdata,
 				    struct list_head *resources);
+struct pci_bus *pci_create_root_bus_in_domain(struct device *parent,
+			int domain, int bus, struct pci_ops *ops,
+			void *sysdata, struct list_head *resources);
 int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax);
 int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
 void pci_bus_release_busn_res(struct pci_bus *b);
-- 
1.9.0

[PATCH v6 4/6] pci: Introduce a domain number for pci_host_bridge.

[Liviu Dudau]

Make it easier to discover the domain number of a bus by storing
the number in pci_host_bridge for the root bus. Several architectures
have their own way of storing this information, so it makes sense
to try to unify the code. While at this, add a new function that
creates a root bus in a given domain and make pci_create_root_bus()
a wrapper around this function.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Tested-by: Tanmay Inamdar <tinamdar@apm.com>
---
 drivers/pci/probe.c | 41 +++++++++++++++++++++++++++++++++--------
 include/linux/pci.h |  4 ++++
 2 files changed, 37 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index fd11c12..172c615 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1714,8 +1714,9 @@ void __weak pcibios_remove_bus(struct pci_bus *bus)
 {
 }
 
-struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
-		struct pci_ops *ops, void *sysdata, struct list_head *resources)
+struct pci_bus *pci_create_root_bus_in_domain(struct device *parent,
+		int domain, int bus, struct pci_ops *ops, void *sysdata,
+		struct list_head *resources)
 {
 	int error;
 	struct pci_host_bridge *bridge;
@@ -1728,30 +1729,34 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 
 	bridge = pci_alloc_host_bridge();
 	if (!bridge)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	bridge->dev.parent = parent;
 	bridge->dev.release = pci_release_host_bridge_dev;
+	bridge->domain_nr = domain;
 	error = pcibios_root_bridge_prepare(bridge);
 	if (error)
 		goto err_out;
 
 	b = pci_alloc_bus();
-	if (!b)
+	if (!b) {
+		error = -ENOMEM;
 		goto err_out;
+	}
 
 	b->sysdata = sysdata;
 	b->ops = ops;
 	b->number = b->busn_res.start = bus;
-	b2 = pci_find_bus(pci_domain_nr(b), bus);
+	b2 = pci_find_bus(bridge->domain_nr, bus);
 	if (b2) {
 		/* If we already got to this bus through a different bridge, ignore it */
 		dev_dbg(&b2->dev, "bus already known\n");
+		error = -EEXIST;
 		goto err_bus_out;
 	}
 
 	bridge->bus = b;
-	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
+	dev_set_name(&bridge->dev, "pci%04x:%02x", bridge->domain_nr, bus);
 	error = device_register(&bridge->dev);
 	if (error) {
 		put_device(&bridge->dev);
@@ -1766,7 +1771,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 
 	b->dev.class = &pcibus_class;
 	b->dev.parent = b->bridge;
-	dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus);
+	dev_set_name(&b->dev, "%04x:%02x", bridge->domain_nr, bus);
 	error = device_register(&b->dev);
 	if (error)
 		goto class_dev_reg_err;
@@ -1816,7 +1821,27 @@ err_bus_out:
 	kfree(b);
 err_out:
 	kfree(bridge);
-	return NULL;
+	return ERR_PTR(error);
+}
+
+struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
+		struct pci_ops *ops, void *sysdata, struct list_head *resources)
+{
+	int domain_nr;
+	struct pci_bus *b = pci_alloc_bus();
+	if (!b)
+		return NULL;
+
+	b->sysdata = sysdata;
+	domain_nr = pci_domain_nr(b);
+	kfree(b);
+
+	b = pci_create_root_bus_in_domain(parent, domain_nr, bus,
+				ops, sysdata, resources);
+	if (IS_ERR(b))
+		return NULL;
+
+	return b;
 }
 
 int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 33aa2ca..1eed009 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -394,6 +394,7 @@ struct pci_host_bridge_window {
 struct pci_host_bridge {
 	struct device dev;
 	struct pci_bus *bus;		/* root bus */
+	int domain_nr;
 	struct list_head windows;	/* pci_host_bridge_windows */
 	void (*release_fn)(struct pci_host_bridge *);
 	void *release_data;
@@ -747,6 +748,9 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata);
 struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 				    struct pci_ops *ops, void *sysdata,
 				    struct list_head *resources);
+struct pci_bus *pci_create_root_bus_in_domain(struct device *parent,
+			int domain, int bus, struct pci_ops *ops,
+			void *sysdata, struct list_head *resources);
 int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax);
 int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
 void pci_bus_release_busn_res(struct pci_bus *b);
-- 
1.9.0

[PATCH v6 5/6] pci: Export find_pci_host_bridge() function.

[Liviu Dudau]

This is a useful function and we should make it visible outside the
generic PCI code. Export it as a GPL symbol.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Tested-by: Tanmay Inamdar <tinamdar@apm.com>
---
 drivers/pci/host-bridge.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
index 06ace62..8708b652 100644
--- a/drivers/pci/host-bridge.c
+++ b/drivers/pci/host-bridge.c
@@ -17,12 +17,13 @@ static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
 	return bus;
 }
 
-static struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus)
+struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus)
 {
 	struct pci_bus *root_bus = find_pci_root_bus(bus);
 
 	return to_pci_host_bridge(root_bus->bridge);
 }
+EXPORT_SYMBOL_GPL(find_pci_host_bridge);
 
 void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
 				 void (*release_fn)(struct pci_host_bridge *),
-- 
1.9.0

[PATCH v6 5/6] pci: Export find_pci_host_bridge() function.

[Liviu Dudau]

This is a useful function and we should make it visible outside the
generic PCI code. Export it as a GPL symbol.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Tested-by: Tanmay Inamdar <tinamdar@apm.com>
---
 drivers/pci/host-bridge.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
index 06ace62..8708b652 100644
--- a/drivers/pci/host-bridge.c
+++ b/drivers/pci/host-bridge.c
@@ -17,12 +17,13 @@ static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
 	return bus;
 }
 
-static struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus)
+struct pci_host_bridge *find_pci_host_bridge(struct pci_bus *bus)
 {
 	struct pci_bus *root_bus = find_pci_root_bus(bus);
 
 	return to_pci_host_bridge(root_bus->bridge);
 }
+EXPORT_SYMBOL_GPL(find_pci_host_bridge);
 
 void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
 				 void (*release_fn)(struct pci_host_bridge *),
-- 
1.9.0

[PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Liviu Dudau]

Several platforms use a rather generic version of parsing
the device tree to find the host bridge ranges. Move the common code
into the generic PCI code and use it to create a pci_host_bridge
structure that can be used by arch code.

Based on early attempts by Andrew Murray to unify the code.
Used powerpc and microblaze PCI code as starting point.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Tested-by: Tanmay Inamdar <tinamdar@apm.com>
---
 drivers/pci/host-bridge.c | 156 ++++++++++++++++++++++++++++++++++++
 include/linux/pci.h       |  13 +++
 2 files changed, 169 insertions(+)

diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
index 8708b652..db9f51a 100644
--- a/drivers/pci/host-bridge.c
+++ b/drivers/pci/host-bridge.c
@@ -6,9 +6,14 @@
 #include <linux/init.h>
 #include <linux/pci.h>
 #include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_pci.h>
+#include <linux/slab.h>
 
 #include "pci.h"
 
+static atomic_t domain_nr = ATOMIC_INIT(-1);
+
 static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
 {
 	while (bus->parent)
@@ -92,3 +97,154 @@ void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
 	res->end = region->end + offset;
 }
 EXPORT_SYMBOL(pcibios_bus_to_resource);
+
+#ifdef CONFIG_OF
+/**
+ * Simple version of the platform specific code for filtering the list
+ * of resources obtained from the ranges declaration in DT.
+ *
+ * Platforms can override this function in order to impose stronger
+ * constraints onto the list of resources that a host bridge can use.
+ * The filtered list will then be used to create a root bus and associate
+ * it with the host bridge.
+ *
+ */
+int __weak pcibios_fixup_bridge_ranges(struct list_head *resources)
+{
+	return 0;
+}
+
+/**
+ * pci_host_bridge_of_get_ranges - Parse PCI host bridge resources from DT
+ * @dev: device node of the host bridge having the range property
+ * @resources: list where the range of resources will be added after DT parsing
+ * @io_base: pointer to a variable that will contain the physical address for
+ * the start of the I/O range.
+ *
+ * It is the callers job to free the @resources list if an error is returned.
+ *
+ * This function will parse the "ranges" property of a PCI host bridge device
+ * node and setup the resource mapping based on its content. It is expected
+ * that the property conforms with the Power ePAPR document.
+ *
+ * Each architecture is then offered the chance of applying their own
+ * filtering of pci_host_bridge_windows based on their own restrictions by
+ * calling pcibios_fixup_bridge_ranges(). The filtered list of windows
+ * can then be used when creating a pci_host_bridge structure.
+ */
+static int pci_host_bridge_of_get_ranges(struct device_node *dev,
+		struct list_head *resources, resource_size_t *io_base)
+{
+	struct resource *res;
+	struct of_pci_range range;
+	struct of_pci_range_parser parser;
+	int err;
+
+	pr_info("PCI host bridge %s ranges:\n", dev->full_name);
+
+	/* Check for ranges property */
+	err = of_pci_range_parser_init(&parser, dev);
+	if (err)
+		return err;
+
+	pr_debug("Parsing ranges property...\n");
+	for_each_of_pci_range(&parser, &range) {
+		/* Read next ranges element */
+		pr_debug("pci_space: 0x%08x pci_addr:0x%016llx ",
+				range.pci_space, range.pci_addr);
+		pr_debug("cpu_addr:0x%016llx size:0x%016llx\n",
+					range.cpu_addr, range.size);
+
+		/*
+		 * If we failed translation or got a zero-sized region
+		 * then skip this range
+		 */
+		if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
+			continue;
+
+		res = kzalloc(sizeof(struct resource), GFP_KERNEL);
+		if (!res)
+			return -ENOMEM;
+
+		of_pci_range_to_resource(&range, dev, res);
+
+		if (resource_type(res) == IORESOURCE_IO)
+			*io_base = range.cpu_addr;
+
+		pci_add_resource_offset(resources, res,
+				res->start - range.pci_addr);
+	}
+
+	/* Apply architecture specific fixups for the ranges */
+	return pcibios_fixup_bridge_ranges(resources);
+}
+
+/**
+ * of_create_pci_host_bridge - Create a PCI host bridge structure using
+ * information passed in the DT.
+ * @parent: device owning this host bridge
+ * @ops: pci_ops associated with the host controller
+ * @host_data: opaque data structure used by the host controller.
+ *
+ * returns a pointer to the newly created pci_host_bridge structure, or
+ * NULL if the call failed.
+ *
+ * This function will try to obtain the host bridge domain number by
+ * using of_alias_get_id() call with "pci-domain" as a stem. If that
+ * fails, a local allocator will be used that will put each host bridge
+ * in a new domain.
+ */
+struct pci_host_bridge *
+of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops, void *host_data)
+{
+	int err, domain, busno;
+	struct resource *bus_range;
+	struct pci_bus *root_bus;
+	struct pci_host_bridge *bridge;
+	resource_size_t io_base;
+	LIST_HEAD(res);
+
+	bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
+	if (!bus_range)
+		return ERR_PTR(-ENOMEM);
+
+	domain = of_alias_get_id(parent->of_node, "pci-domain");
+	if (domain == -ENODEV)
+		domain = atomic_inc_return(&domain_nr);
+
+	err = of_pci_parse_bus_range(parent->of_node, bus_range);
+	if (err) {
+		dev_info(parent, "No bus range for %s, using default [0-255]\n",
+			parent->of_node->full_name);
+		bus_range->start = 0;
+		bus_range->end = 255;
+		bus_range->flags = IORESOURCE_BUS;
+	}
+	busno = bus_range->start;
+	pci_add_resource(&res, bus_range);
+
+	/* now parse the rest of host bridge bus ranges */
+	err = pci_host_bridge_of_get_ranges(parent->of_node, &res, &io_base);
+	if (err)
+		goto err_create;
+
+	/* then create the root bus */
+	root_bus = pci_create_root_bus_in_domain(parent, domain, busno,
+						ops, host_data, &res);
+	if (IS_ERR(root_bus)) {
+		err = PTR_ERR(root_bus);
+		goto err_create;
+	}
+
+	bridge = to_pci_host_bridge(root_bus->bridge);
+	bridge->io_base = io_base;
+
+	return bridge;
+
+err_create:
+	pci_free_resource_list(&res);
+	return ERR_PTR(err);
+}
+EXPORT_SYMBOL_GPL(of_create_pci_host_bridge);
+
+#endif /* CONFIG_OF */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 1eed009..40ddd3d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -395,6 +395,7 @@ struct pci_host_bridge {
 	struct device dev;
 	struct pci_bus *bus;		/* root bus */
 	int domain_nr;
+	resource_size_t io_base;	/* physical address for the start of I/O area */
 	struct list_head windows;	/* pci_host_bridge_windows */
 	void (*release_fn)(struct pci_host_bridge *);
 	void *release_data;
@@ -1786,11 +1787,23 @@ static inline struct device_node *pci_bus_to_OF_node(struct pci_bus *bus)
 	return bus ? bus->dev.of_node : NULL;
 }
 
+struct pci_host_bridge *
+of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops,
+			void *host_data);
+
+int pcibios_fixup_bridge_ranges(struct list_head *resources);
 #else /* CONFIG_OF */
 static inline void pci_set_of_node(struct pci_dev *dev) { }
 static inline void pci_release_of_node(struct pci_dev *dev) { }
 static inline void pci_set_bus_of_node(struct pci_bus *bus) { }
 static inline void pci_release_bus_of_node(struct pci_bus *bus) { }
+
+static inline struct pci_host_bridge *
+of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops,
+			void *host_data)
+{
+	return NULL;
+}
 #endif  /* CONFIG_OF */
 
 #ifdef CONFIG_EEH
-- 
1.9.0

[PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Liviu Dudau]

Several platforms use a rather generic version of parsing
the device tree to find the host bridge ranges. Move the common code
into the generic PCI code and use it to create a pci_host_bridge
structure that can be used by arch code.

Based on early attempts by Andrew Murray to unify the code.
Used powerpc and microblaze PCI code as starting point.

Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
Tested-by: Tanmay Inamdar <tinamdar@apm.com>
---
 drivers/pci/host-bridge.c | 156 ++++++++++++++++++++++++++++++++++++
 include/linux/pci.h       |  13 +++
 2 files changed, 169 insertions(+)

diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
index 8708b652..db9f51a 100644
--- a/drivers/pci/host-bridge.c
+++ b/drivers/pci/host-bridge.c
@@ -6,9 +6,14 @@
 #include <linux/init.h>
 #include <linux/pci.h>
 #include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_pci.h>
+#include <linux/slab.h>
 
 #include "pci.h"
 
+static atomic_t domain_nr = ATOMIC_INIT(-1);
+
 static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
 {
 	while (bus->parent)
@@ -92,3 +97,154 @@ void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
 	res->end = region->end + offset;
 }
 EXPORT_SYMBOL(pcibios_bus_to_resource);
+
+#ifdef CONFIG_OF
+/**
+ * Simple version of the platform specific code for filtering the list
+ * of resources obtained from the ranges declaration in DT.
+ *
+ * Platforms can override this function in order to impose stronger
+ * constraints onto the list of resources that a host bridge can use.
+ * The filtered list will then be used to create a root bus and associate
+ * it with the host bridge.
+ *
+ */
+int __weak pcibios_fixup_bridge_ranges(struct list_head *resources)
+{
+	return 0;
+}
+
+/**
+ * pci_host_bridge_of_get_ranges - Parse PCI host bridge resources from DT
+ * @dev: device node of the host bridge having the range property
+ * @resources: list where the range of resources will be added after DT parsing
+ * @io_base: pointer to a variable that will contain the physical address for
+ * the start of the I/O range.
+ *
+ * It is the callers job to free the @resources list if an error is returned.
+ *
+ * This function will parse the "ranges" property of a PCI host bridge device
+ * node and setup the resource mapping based on its content. It is expected
+ * that the property conforms with the Power ePAPR document.
+ *
+ * Each architecture is then offered the chance of applying their own
+ * filtering of pci_host_bridge_windows based on their own restrictions by
+ * calling pcibios_fixup_bridge_ranges(). The filtered list of windows
+ * can then be used when creating a pci_host_bridge structure.
+ */
+static int pci_host_bridge_of_get_ranges(struct device_node *dev,
+		struct list_head *resources, resource_size_t *io_base)
+{
+	struct resource *res;
+	struct of_pci_range range;
+	struct of_pci_range_parser parser;
+	int err;
+
+	pr_info("PCI host bridge %s ranges:\n", dev->full_name);
+
+	/* Check for ranges property */
+	err = of_pci_range_parser_init(&parser, dev);
+	if (err)
+		return err;
+
+	pr_debug("Parsing ranges property...\n");
+	for_each_of_pci_range(&parser, &range) {
+		/* Read next ranges element */
+		pr_debug("pci_space: 0x%08x pci_addr:0x%016llx ",
+				range.pci_space, range.pci_addr);
+		pr_debug("cpu_addr:0x%016llx size:0x%016llx\n",
+					range.cpu_addr, range.size);
+
+		/*
+		 * If we failed translation or got a zero-sized region
+		 * then skip this range
+		 */
+		if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
+			continue;
+
+		res = kzalloc(sizeof(struct resource), GFP_KERNEL);
+		if (!res)
+			return -ENOMEM;
+
+		of_pci_range_to_resource(&range, dev, res);
+
+		if (resource_type(res) == IORESOURCE_IO)
+			*io_base = range.cpu_addr;
+
+		pci_add_resource_offset(resources, res,
+				res->start - range.pci_addr);
+	}
+
+	/* Apply architecture specific fixups for the ranges */
+	return pcibios_fixup_bridge_ranges(resources);
+}
+
+/**
+ * of_create_pci_host_bridge - Create a PCI host bridge structure using
+ * information passed in the DT.
+ * @parent: device owning this host bridge
+ * @ops: pci_ops associated with the host controller
+ * @host_data: opaque data structure used by the host controller.
+ *
+ * returns a pointer to the newly created pci_host_bridge structure, or
+ * NULL if the call failed.
+ *
+ * This function will try to obtain the host bridge domain number by
+ * using of_alias_get_id() call with "pci-domain" as a stem. If that
+ * fails, a local allocator will be used that will put each host bridge
+ * in a new domain.
+ */
+struct pci_host_bridge *
+of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops, void *host_data)
+{
+	int err, domain, busno;
+	struct resource *bus_range;
+	struct pci_bus *root_bus;
+	struct pci_host_bridge *bridge;
+	resource_size_t io_base;
+	LIST_HEAD(res);
+
+	bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
+	if (!bus_range)
+		return ERR_PTR(-ENOMEM);
+
+	domain = of_alias_get_id(parent->of_node, "pci-domain");
+	if (domain == -ENODEV)
+		domain = atomic_inc_return(&domain_nr);
+
+	err = of_pci_parse_bus_range(parent->of_node, bus_range);
+	if (err) {
+		dev_info(parent, "No bus range for %s, using default [0-255]\n",
+			parent->of_node->full_name);
+		bus_range->start = 0;
+		bus_range->end = 255;
+		bus_range->flags = IORESOURCE_BUS;
+	}
+	busno = bus_range->start;
+	pci_add_resource(&res, bus_range);
+
+	/* now parse the rest of host bridge bus ranges */
+	err = pci_host_bridge_of_get_ranges(parent->of_node, &res, &io_base);
+	if (err)
+		goto err_create;
+
+	/* then create the root bus */
+	root_bus = pci_create_root_bus_in_domain(parent, domain, busno,
+						ops, host_data, &res);
+	if (IS_ERR(root_bus)) {
+		err = PTR_ERR(root_bus);
+		goto err_create;
+	}
+
+	bridge = to_pci_host_bridge(root_bus->bridge);
+	bridge->io_base = io_base;
+
+	return bridge;
+
+err_create:
+	pci_free_resource_list(&res);
+	return ERR_PTR(err);
+}
+EXPORT_SYMBOL_GPL(of_create_pci_host_bridge);
+
+#endif /* CONFIG_OF */
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 1eed009..40ddd3d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -395,6 +395,7 @@ struct pci_host_bridge {
 	struct device dev;
 	struct pci_bus *bus;		/* root bus */
 	int domain_nr;
+	resource_size_t io_base;	/* physical address for the start of I/O area */
 	struct list_head windows;	/* pci_host_bridge_windows */
 	void (*release_fn)(struct pci_host_bridge *);
 	void *release_data;
@@ -1786,11 +1787,23 @@ static inline struct device_node *pci_bus_to_OF_node(struct pci_bus *bus)
 	return bus ? bus->dev.of_node : NULL;
 }
 
+struct pci_host_bridge *
+of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops,
+			void *host_data);
+
+int pcibios_fixup_bridge_ranges(struct list_head *resources);
 #else /* CONFIG_OF */
 static inline void pci_set_of_node(struct pci_dev *dev) { }
 static inline void pci_release_of_node(struct pci_dev *dev) { }
 static inline void pci_set_bus_of_node(struct pci_bus *bus) { }
 static inline void pci_release_bus_of_node(struct pci_bus *bus) { }
+
+static inline struct pci_host_bridge *
+of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops,
+			void *host_data)
+{
+	return NULL;
+}
 #endif  /* CONFIG_OF */
 
 #ifdef CONFIG_EEH
-- 
1.9.0

Re: [PATCH v6 1/6] pci: Introduce pci_register_io_range() helper function.

[Grant Likely]

On Wed,  5 Mar 2014 11:48:52 +0000, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> Some architectures do not share x86 simple view of the I/O space and
> instead use a range of addresses that map to external devices. For PCI,
> these ranges can be expressed by OF bindings in a device tree file.
> 
> Introduce a pci_register_io_range() helper function that can be used
> by the architecture code to keep track of the io ranges described by the
> PCI bindings.
> 
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Tested-by: Tanmay Inamdar <tinamdar@apm.com>

Acked-by: Grant Likely <grant.likely@linaro.org>

> ---
>  drivers/of/address.c       | 5 +++++
>  include/linux/of_address.h | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 1a54f1f..d1bb30f 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -619,6 +619,11 @@ const __be32 *of_get_address(struct device_node *dev, int index, u64 *size,
>  }
>  EXPORT_SYMBOL(of_get_address);
>  
> +int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
> +{
> +	return 0;
> +}
> +
>  unsigned long __weak pci_address_to_pio(phys_addr_t address)
>  {
>  	if (address > IO_SPACE_LIMIT)
> diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> index 5f6ed6b..40c418d 100644
> --- a/include/linux/of_address.h
> +++ b/include/linux/of_address.h
> @@ -56,6 +56,7 @@ extern void __iomem *of_iomap(struct device_node *device, int index);
>  extern const __be32 *of_get_address(struct device_node *dev, int index,
>  			   u64 *size, unsigned int *flags);
>  
> +extern int pci_register_io_range(phys_addr_t addr, resource_size_t size);
>  extern unsigned long pci_address_to_pio(phys_addr_t addr);
>  
>  extern int of_pci_range_parser_init(struct of_pci_range_parser *parser,
> -- 
> 1.9.0
> 
> 
> _______________________________________________
> linaro-kernel mailing list
> linaro-kernel@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-kernel

Re: [PATCH v6 1/6] pci: Introduce pci_register_io_range() helper function.

[Grant Likely]

On Wed,  5 Mar 2014 11:48:52 +0000, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> Some architectures do not share x86 simple view of the I/O space and
> instead use a range of addresses that map to external devices. For PCI,
> these ranges can be expressed by OF bindings in a device tree file.
> 
> Introduce a pci_register_io_range() helper function that can be used
> by the architecture code to keep track of the io ranges described by the
> PCI bindings.
> 
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Tested-by: Tanmay Inamdar <tinamdar@apm.com>

Acked-by: Grant Likely <grant.likely@linaro.org>

> ---
>  drivers/of/address.c       | 5 +++++
>  include/linux/of_address.h | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 1a54f1f..d1bb30f 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -619,6 +619,11 @@ const __be32 *of_get_address(struct device_node *dev, int index, u64 *size,
>  }
>  EXPORT_SYMBOL(of_get_address);
>  
> +int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
> +{
> +	return 0;
> +}
> +
>  unsigned long __weak pci_address_to_pio(phys_addr_t address)
>  {
>  	if (address > IO_SPACE_LIMIT)
> diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> index 5f6ed6b..40c418d 100644
> --- a/include/linux/of_address.h
> +++ b/include/linux/of_address.h
> @@ -56,6 +56,7 @@ extern void __iomem *of_iomap(struct device_node *device, int index);
>  extern const __be32 *of_get_address(struct device_node *dev, int index,
>  			   u64 *size, unsigned int *flags);
>  
> +extern int pci_register_io_range(phys_addr_t addr, resource_size_t size);
>  extern unsigned long pci_address_to_pio(phys_addr_t addr);
>  
>  extern int of_pci_range_parser_init(struct of_pci_range_parser *parser,
> -- 
> 1.9.0
> 
> 
> _______________________________________________
> linaro-kernel mailing list
> linaro-kernel@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-kernel

[PATCH v6 1/6] pci: Introduce pci_register_io_range() helper function.

[Grant Likely]

On Wed,  5 Mar 2014 11:48:52 +0000, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> Some architectures do not share x86 simple view of the I/O space and
> instead use a range of addresses that map to external devices. For PCI,
> these ranges can be expressed by OF bindings in a device tree file.
> 
> Introduce a pci_register_io_range() helper function that can be used
> by the architecture code to keep track of the io ranges described by the
> PCI bindings.
> 
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Tested-by: Tanmay Inamdar <tinamdar@apm.com>

Acked-by: Grant Likely <grant.likely@linaro.org>

> ---
>  drivers/of/address.c       | 5 +++++
>  include/linux/of_address.h | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 1a54f1f..d1bb30f 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -619,6 +619,11 @@ const __be32 *of_get_address(struct device_node *dev, int index, u64 *size,
>  }
>  EXPORT_SYMBOL(of_get_address);
>  
> +int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
> +{
> +	return 0;
> +}
> +
>  unsigned long __weak pci_address_to_pio(phys_addr_t address)
>  {
>  	if (address > IO_SPACE_LIMIT)
> diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> index 5f6ed6b..40c418d 100644
> --- a/include/linux/of_address.h
> +++ b/include/linux/of_address.h
> @@ -56,6 +56,7 @@ extern void __iomem *of_iomap(struct device_node *device, int index);
>  extern const __be32 *of_get_address(struct device_node *dev, int index,
>  			   u64 *size, unsigned int *flags);
>  
> +extern int pci_register_io_range(phys_addr_t addr, resource_size_t size);
>  extern unsigned long pci_address_to_pio(phys_addr_t addr);
>  
>  extern int of_pci_range_parser_init(struct of_pci_range_parser *parser,
> -- 
> 1.9.0
> 
> 
> _______________________________________________
> linaro-kernel mailing list
> linaro-kernel at lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-kernel

Re: [PATCH v6 2/6] pci: OF: Fix the conversion of IO ranges into IO resources.

[Grant Likely]

On Wed,  5 Mar 2014 11:48:53 +0000, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> The ranges property for a host bridge controller in DT describes
> the mapping between the PCI bus address and the CPU physical address.
> The resources framework however expects that the IO resources start
> at a pseudo "port" address 0 (zero) and have a maximum size of IO_SPACE_LIMIT.
> The conversion from pci ranges to resources failed to take that into account.
> 
> In the process move the function into drivers/of/address.c as it
> now depends on pci_address_to_pio() code.
> 
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Tested-by: Tanmay Inamdar <tinamdar@apm.com>

I think this is okay, but I'll need an ack from Arnd or Ben.

g.

> ---
>  drivers/of/address.c       | 34 ++++++++++++++++++++++++++++++++++
>  include/linux/of_address.h | 13 ++-----------
>  2 files changed, 36 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index d1bb30f..d595d98 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -724,3 +724,37 @@ void __iomem *of_iomap(struct device_node *np, int index)
>  	return ioremap(res.start, resource_size(&res));
>  }
>  EXPORT_SYMBOL(of_iomap);
> +
> +/**
> + * of_pci_range_to_resource - Create a resource from an of_pci_range
> + * @range:	the PCI range that describes the resource
> + * @np:		device node where the range belongs to
> + * @res:	pointer to a valid resource that will be updated to
> + *              reflect the values contained in the range.
> + * Note that if the range is an IO range, the resource will be converted
> + * using pci_address_to_pio() which can fail if it is called too early or
> + * if the range cannot be matched to any host bridge IO space (our case here).
> + * To guard against that we try to register the IO range first.
> + * If that fails we know that pci_address_to_pio() will do too.
> + */
> +void of_pci_range_to_resource(struct of_pci_range *range,
> +	struct device_node *np, struct resource *res)
> +{
> +	res->flags = range->flags;
> +	if (res->flags & IORESOURCE_IO) {
> +		unsigned long port = -1;
> +		if (!pci_register_io_range(range->cpu_addr, range->size))
> +			port = pci_address_to_pio(range->cpu_addr);
> +		if (port == (unsigned long)-1) {
> +			res->start = (resource_size_t)OF_BAD_ADDR;
> +			res->end = (resource_size_t)OF_BAD_ADDR;
> +			return;
> +		}
> +		res->start = port;
> +	} else {
> +		res->start = range->cpu_addr;
> +	}
> +	res->end = res->start + range->size - 1;
> +	res->parent = res->child = res->sibling = NULL;
> +	res->name = np->full_name;
> +}
> diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> index 40c418d..3fe500a 100644
> --- a/include/linux/of_address.h
> +++ b/include/linux/of_address.h
> @@ -23,17 +23,8 @@ struct of_pci_range {
>  #define for_each_of_pci_range(parser, range) \
>  	for (; of_pci_range_parser_one(parser, range);)
>  
> -static inline void of_pci_range_to_resource(struct of_pci_range *range,
> -					    struct device_node *np,
> -					    struct resource *res)
> -{
> -	res->flags = range->flags;
> -	res->start = range->cpu_addr;
> -	res->end = range->cpu_addr + range->size - 1;
> -	res->parent = res->child = res->sibling = NULL;
> -	res->name = np->full_name;
> -}
> -
> +extern void of_pci_range_to_resource(struct of_pci_range *range,
> +		struct device_node *np, struct resource *res);
>  /* Translate a DMA address from device space to CPU space */
>  extern u64 of_translate_dma_address(struct device_node *dev,
>  				    const __be32 *in_addr);
> -- 
> 1.9.0
> 
> 
> _______________________________________________
> linaro-kernel mailing list
> linaro-kernel@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-kernel

[PATCH v6 2/6] pci: OF: Fix the conversion of IO ranges into IO resources.

[Grant Likely]

On Wed,  5 Mar 2014 11:48:53 +0000, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> The ranges property for a host bridge controller in DT describes
> the mapping between the PCI bus address and the CPU physical address.
> The resources framework however expects that the IO resources start
> at a pseudo "port" address 0 (zero) and have a maximum size of IO_SPACE_LIMIT.
> The conversion from pci ranges to resources failed to take that into account.
> 
> In the process move the function into drivers/of/address.c as it
> now depends on pci_address_to_pio() code.
> 
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Tested-by: Tanmay Inamdar <tinamdar@apm.com>

I think this is okay, but I'll need an ack from Arnd or Ben.

g.

> ---
>  drivers/of/address.c       | 34 ++++++++++++++++++++++++++++++++++
>  include/linux/of_address.h | 13 ++-----------
>  2 files changed, 36 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index d1bb30f..d595d98 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -724,3 +724,37 @@ void __iomem *of_iomap(struct device_node *np, int index)
>  	return ioremap(res.start, resource_size(&res));
>  }
>  EXPORT_SYMBOL(of_iomap);
> +
> +/**
> + * of_pci_range_to_resource - Create a resource from an of_pci_range
> + * @range:	the PCI range that describes the resource
> + * @np:		device node where the range belongs to
> + * @res:	pointer to a valid resource that will be updated to
> + *              reflect the values contained in the range.
> + * Note that if the range is an IO range, the resource will be converted
> + * using pci_address_to_pio() which can fail if it is called too early or
> + * if the range cannot be matched to any host bridge IO space (our case here).
> + * To guard against that we try to register the IO range first.
> + * If that fails we know that pci_address_to_pio() will do too.
> + */
> +void of_pci_range_to_resource(struct of_pci_range *range,
> +	struct device_node *np, struct resource *res)
> +{
> +	res->flags = range->flags;
> +	if (res->flags & IORESOURCE_IO) {
> +		unsigned long port = -1;
> +		if (!pci_register_io_range(range->cpu_addr, range->size))
> +			port = pci_address_to_pio(range->cpu_addr);
> +		if (port == (unsigned long)-1) {
> +			res->start = (resource_size_t)OF_BAD_ADDR;
> +			res->end = (resource_size_t)OF_BAD_ADDR;
> +			return;
> +		}
> +		res->start = port;
> +	} else {
> +		res->start = range->cpu_addr;
> +	}
> +	res->end = res->start + range->size - 1;
> +	res->parent = res->child = res->sibling = NULL;
> +	res->name = np->full_name;
> +}
> diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> index 40c418d..3fe500a 100644
> --- a/include/linux/of_address.h
> +++ b/include/linux/of_address.h
> @@ -23,17 +23,8 @@ struct of_pci_range {
>  #define for_each_of_pci_range(parser, range) \
>  	for (; of_pci_range_parser_one(parser, range);)
>  
> -static inline void of_pci_range_to_resource(struct of_pci_range *range,
> -					    struct device_node *np,
> -					    struct resource *res)
> -{
> -	res->flags = range->flags;
> -	res->start = range->cpu_addr;
> -	res->end = range->cpu_addr + range->size - 1;
> -	res->parent = res->child = res->sibling = NULL;
> -	res->name = np->full_name;
> -}
> -
> +extern void of_pci_range_to_resource(struct of_pci_range *range,
> +		struct device_node *np, struct resource *res);
>  /* Translate a DMA address from device space to CPU space */
>  extern u64 of_translate_dma_address(struct device_node *dev,
>  				    const __be32 *in_addr);
> -- 
> 1.9.0
> 
> 
> _______________________________________________
> linaro-kernel mailing list
> linaro-kernel at lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-kernel

Re: [PATCH v6 3/6] pci: Create pci_host_bridge before its associated bus in pci_create_root_bus.

[Grant Likely]

On Wed,  5 Mar 2014 11:48:54 +0000, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> Before commit 7b5436635800 the pci_host_bridge was created before the root bus.
> As that commit has added a needless dependency on the bus for pci_alloc_host_bridge()
> the creation order has been changed for no good reason. Revert the order of
> creation as we are going to depend on the pci_host_bridge structure to retrieve the
> domain number of the root bus.
> 
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Tested-by: Tanmay Inamdar <tinamdar@apm.com>

Acked-by: Grant Likely <grant.likely@linaro.org>

> ---
>  drivers/pci/probe.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 6e34498..fd11c12 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -505,7 +505,7 @@ static void pci_release_host_bridge_dev(struct device *dev)
>  	kfree(bridge);
>  }
>  
> -static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
> +static struct pci_host_bridge *pci_alloc_host_bridge(void)
>  {
>  	struct pci_host_bridge *bridge;
>  
> @@ -514,7 +514,6 @@ static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
>  		return NULL;
>  
>  	INIT_LIST_HEAD(&bridge->windows);
> -	bridge->bus = b;
>  	return bridge;
>  }
>  
> @@ -1727,9 +1726,19 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  	char bus_addr[64];
>  	char *fmt;
>  
> +	bridge = pci_alloc_host_bridge();
> +	if (!bridge)
> +		return NULL;
> +
> +	bridge->dev.parent = parent;
> +	bridge->dev.release = pci_release_host_bridge_dev;
> +	error = pcibios_root_bridge_prepare(bridge);
> +	if (error)
> +		goto err_out;
> +
>  	b = pci_alloc_bus();
>  	if (!b)
> -		return NULL;
> +		goto err_out;
>  
>  	b->sysdata = sysdata;
>  	b->ops = ops;
> @@ -1738,26 +1747,15 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  	if (b2) {
>  		/* If we already got to this bus through a different bridge, ignore it */
>  		dev_dbg(&b2->dev, "bus already known\n");
> -		goto err_out;
> +		goto err_bus_out;
>  	}
>  
> -	bridge = pci_alloc_host_bridge(b);
> -	if (!bridge)
> -		goto err_out;
> -
> -	bridge->dev.parent = parent;
> -	bridge->dev.release = pci_release_host_bridge_dev;
> +	bridge->bus = b;
>  	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
> -	error = pcibios_root_bridge_prepare(bridge);
> -	if (error) {
> -		kfree(bridge);
> -		goto err_out;
> -	}
> -
>  	error = device_register(&bridge->dev);
>  	if (error) {
>  		put_device(&bridge->dev);
> -		goto err_out;
> +		goto err_bus_out;
>  	}
>  	b->bridge = get_device(&bridge->dev);
>  	device_enable_async_suspend(b->bridge);
> @@ -1814,8 +1812,10 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  class_dev_reg_err:
>  	put_device(&bridge->dev);
>  	device_unregister(&bridge->dev);
> -err_out:
> +err_bus_out:
>  	kfree(b);
> +err_out:
> +	kfree(bridge);
>  	return NULL;
>  }
>  
> -- 
> 1.9.0
> 
> 
> _______________________________________________
> linaro-kernel mailing list
> linaro-kernel@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-kernel

[PATCH v6 3/6] pci: Create pci_host_bridge before its associated bus in pci_create_root_bus.

[Grant Likely]

On Wed,  5 Mar 2014 11:48:54 +0000, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> Before commit 7b5436635800 the pci_host_bridge was created before the root bus.
> As that commit has added a needless dependency on the bus for pci_alloc_host_bridge()
> the creation order has been changed for no good reason. Revert the order of
> creation as we are going to depend on the pci_host_bridge structure to retrieve the
> domain number of the root bus.
> 
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Tested-by: Tanmay Inamdar <tinamdar@apm.com>

Acked-by: Grant Likely <grant.likely@linaro.org>

> ---
>  drivers/pci/probe.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 6e34498..fd11c12 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -505,7 +505,7 @@ static void pci_release_host_bridge_dev(struct device *dev)
>  	kfree(bridge);
>  }
>  
> -static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
> +static struct pci_host_bridge *pci_alloc_host_bridge(void)
>  {
>  	struct pci_host_bridge *bridge;
>  
> @@ -514,7 +514,6 @@ static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
>  		return NULL;
>  
>  	INIT_LIST_HEAD(&bridge->windows);
> -	bridge->bus = b;
>  	return bridge;
>  }
>  
> @@ -1727,9 +1726,19 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  	char bus_addr[64];
>  	char *fmt;
>  
> +	bridge = pci_alloc_host_bridge();
> +	if (!bridge)
> +		return NULL;
> +
> +	bridge->dev.parent = parent;
> +	bridge->dev.release = pci_release_host_bridge_dev;
> +	error = pcibios_root_bridge_prepare(bridge);
> +	if (error)
> +		goto err_out;
> +
>  	b = pci_alloc_bus();
>  	if (!b)
> -		return NULL;
> +		goto err_out;
>  
>  	b->sysdata = sysdata;
>  	b->ops = ops;
> @@ -1738,26 +1747,15 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  	if (b2) {
>  		/* If we already got to this bus through a different bridge, ignore it */
>  		dev_dbg(&b2->dev, "bus already known\n");
> -		goto err_out;
> +		goto err_bus_out;
>  	}
>  
> -	bridge = pci_alloc_host_bridge(b);
> -	if (!bridge)
> -		goto err_out;
> -
> -	bridge->dev.parent = parent;
> -	bridge->dev.release = pci_release_host_bridge_dev;
> +	bridge->bus = b;
>  	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
> -	error = pcibios_root_bridge_prepare(bridge);
> -	if (error) {
> -		kfree(bridge);
> -		goto err_out;
> -	}
> -
>  	error = device_register(&bridge->dev);
>  	if (error) {
>  		put_device(&bridge->dev);
> -		goto err_out;
> +		goto err_bus_out;
>  	}
>  	b->bridge = get_device(&bridge->dev);
>  	device_enable_async_suspend(b->bridge);
> @@ -1814,8 +1812,10 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  class_dev_reg_err:
>  	put_device(&bridge->dev);
>  	device_unregister(&bridge->dev);
> -err_out:
> +err_bus_out:
>  	kfree(b);
> +err_out:
> +	kfree(bridge);
>  	return NULL;
>  }
>  
> -- 
> 1.9.0
> 
> 
> _______________________________________________
> linaro-kernel mailing list
> linaro-kernel at lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-kernel

Re: [PATCH v6 4/6] pci: Introduce a domain number for pci_host_bridge.

[Grant Likely]

On Wed,  5 Mar 2014 11:48:55 +0000, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> Make it easier to discover the domain number of a bus by storing
> the number in pci_host_bridge for the root bus. Several architectures
> have their own way of storing this information, so it makes sense
> to try to unify the code. While at this, add a new function that
> creates a root bus in a given domain and make pci_create_root_bus()
> a wrapper around this function.
> 
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Tested-by: Tanmay Inamdar <tinamdar@apm.com>

Looks okay to me structure wise, again I'd like an ack from Ben or Arnd.

g.

> ---
>  drivers/pci/probe.c | 41 +++++++++++++++++++++++++++++++++--------
>  include/linux/pci.h |  4 ++++
>  2 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index fd11c12..172c615 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1714,8 +1714,9 @@ void __weak pcibios_remove_bus(struct pci_bus *bus)
>  {
>  }
>  
> -struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> -		struct pci_ops *ops, void *sysdata, struct list_head *resources)
> +struct pci_bus *pci_create_root_bus_in_domain(struct device *parent,
> +		int domain, int bus, struct pci_ops *ops, void *sysdata,
> +		struct list_head *resources)
>  {
>  	int error;
>  	struct pci_host_bridge *bridge;
> @@ -1728,30 +1729,34 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  
>  	bridge = pci_alloc_host_bridge();
>  	if (!bridge)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	bridge->dev.parent = parent;
>  	bridge->dev.release = pci_release_host_bridge_dev;
> +	bridge->domain_nr = domain;
>  	error = pcibios_root_bridge_prepare(bridge);
>  	if (error)
>  		goto err_out;
>  
>  	b = pci_alloc_bus();
> -	if (!b)
> +	if (!b) {
> +		error = -ENOMEM;
>  		goto err_out;
> +	}
>  
>  	b->sysdata = sysdata;
>  	b->ops = ops;
>  	b->number = b->busn_res.start = bus;
> -	b2 = pci_find_bus(pci_domain_nr(b), bus);
> +	b2 = pci_find_bus(bridge->domain_nr, bus);
>  	if (b2) {
>  		/* If we already got to this bus through a different bridge, ignore it */
>  		dev_dbg(&b2->dev, "bus already known\n");
> +		error = -EEXIST;
>  		goto err_bus_out;
>  	}
>  
>  	bridge->bus = b;
> -	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
> +	dev_set_name(&bridge->dev, "pci%04x:%02x", bridge->domain_nr, bus);
>  	error = device_register(&bridge->dev);
>  	if (error) {
>  		put_device(&bridge->dev);
> @@ -1766,7 +1771,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  
>  	b->dev.class = &pcibus_class;
>  	b->dev.parent = b->bridge;
> -	dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus);
> +	dev_set_name(&b->dev, "%04x:%02x", bridge->domain_nr, bus);
>  	error = device_register(&b->dev);
>  	if (error)
>  		goto class_dev_reg_err;
> @@ -1816,7 +1821,27 @@ err_bus_out:
>  	kfree(b);
>  err_out:
>  	kfree(bridge);
> -	return NULL;
> +	return ERR_PTR(error);
> +}
> +
> +struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> +		struct pci_ops *ops, void *sysdata, struct list_head *resources)
> +{
> +	int domain_nr;
> +	struct pci_bus *b = pci_alloc_bus();
> +	if (!b)
> +		return NULL;
> +
> +	b->sysdata = sysdata;
> +	domain_nr = pci_domain_nr(b);
> +	kfree(b);
> +
> +	b = pci_create_root_bus_in_domain(parent, domain_nr, bus,
> +				ops, sysdata, resources);
> +	if (IS_ERR(b))
> +		return NULL;
> +
> +	return b;
>  }
>  
>  int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 33aa2ca..1eed009 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -394,6 +394,7 @@ struct pci_host_bridge_window {
>  struct pci_host_bridge {
>  	struct device dev;
>  	struct pci_bus *bus;		/* root bus */
> +	int domain_nr;
>  	struct list_head windows;	/* pci_host_bridge_windows */
>  	void (*release_fn)(struct pci_host_bridge *);
>  	void *release_data;
> @@ -747,6 +748,9 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata);
>  struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  				    struct pci_ops *ops, void *sysdata,
>  				    struct list_head *resources);
> +struct pci_bus *pci_create_root_bus_in_domain(struct device *parent,
> +			int domain, int bus, struct pci_ops *ops,
> +			void *sysdata, struct list_head *resources);
>  int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax);
>  int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
>  void pci_bus_release_busn_res(struct pci_bus *b);
> -- 
> 1.9.0
> 
> 
> _______________________________________________
> linaro-kernel mailing list
> linaro-kernel@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-kernel

[PATCH v6 4/6] pci: Introduce a domain number for pci_host_bridge.

[Grant Likely]

On Wed,  5 Mar 2014 11:48:55 +0000, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> Make it easier to discover the domain number of a bus by storing
> the number in pci_host_bridge for the root bus. Several architectures
> have their own way of storing this information, so it makes sense
> to try to unify the code. While at this, add a new function that
> creates a root bus in a given domain and make pci_create_root_bus()
> a wrapper around this function.
> 
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Tested-by: Tanmay Inamdar <tinamdar@apm.com>

Looks okay to me structure wise, again I'd like an ack from Ben or Arnd.

g.

> ---
>  drivers/pci/probe.c | 41 +++++++++++++++++++++++++++++++++--------
>  include/linux/pci.h |  4 ++++
>  2 files changed, 37 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index fd11c12..172c615 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1714,8 +1714,9 @@ void __weak pcibios_remove_bus(struct pci_bus *bus)
>  {
>  }
>  
> -struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> -		struct pci_ops *ops, void *sysdata, struct list_head *resources)
> +struct pci_bus *pci_create_root_bus_in_domain(struct device *parent,
> +		int domain, int bus, struct pci_ops *ops, void *sysdata,
> +		struct list_head *resources)
>  {
>  	int error;
>  	struct pci_host_bridge *bridge;
> @@ -1728,30 +1729,34 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  
>  	bridge = pci_alloc_host_bridge();
>  	if (!bridge)
> -		return NULL;
> +		return ERR_PTR(-ENOMEM);
>  
>  	bridge->dev.parent = parent;
>  	bridge->dev.release = pci_release_host_bridge_dev;
> +	bridge->domain_nr = domain;
>  	error = pcibios_root_bridge_prepare(bridge);
>  	if (error)
>  		goto err_out;
>  
>  	b = pci_alloc_bus();
> -	if (!b)
> +	if (!b) {
> +		error = -ENOMEM;
>  		goto err_out;
> +	}
>  
>  	b->sysdata = sysdata;
>  	b->ops = ops;
>  	b->number = b->busn_res.start = bus;
> -	b2 = pci_find_bus(pci_domain_nr(b), bus);
> +	b2 = pci_find_bus(bridge->domain_nr, bus);
>  	if (b2) {
>  		/* If we already got to this bus through a different bridge, ignore it */
>  		dev_dbg(&b2->dev, "bus already known\n");
> +		error = -EEXIST;
>  		goto err_bus_out;
>  	}
>  
>  	bridge->bus = b;
> -	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
> +	dev_set_name(&bridge->dev, "pci%04x:%02x", bridge->domain_nr, bus);
>  	error = device_register(&bridge->dev);
>  	if (error) {
>  		put_device(&bridge->dev);
> @@ -1766,7 +1771,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  
>  	b->dev.class = &pcibus_class;
>  	b->dev.parent = b->bridge;
> -	dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus);
> +	dev_set_name(&b->dev, "%04x:%02x", bridge->domain_nr, bus);
>  	error = device_register(&b->dev);
>  	if (error)
>  		goto class_dev_reg_err;
> @@ -1816,7 +1821,27 @@ err_bus_out:
>  	kfree(b);
>  err_out:
>  	kfree(bridge);
> -	return NULL;
> +	return ERR_PTR(error);
> +}
> +
> +struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
> +		struct pci_ops *ops, void *sysdata, struct list_head *resources)
> +{
> +	int domain_nr;
> +	struct pci_bus *b = pci_alloc_bus();
> +	if (!b)
> +		return NULL;
> +
> +	b->sysdata = sysdata;
> +	domain_nr = pci_domain_nr(b);
> +	kfree(b);
> +
> +	b = pci_create_root_bus_in_domain(parent, domain_nr, bus,
> +				ops, sysdata, resources);
> +	if (IS_ERR(b))
> +		return NULL;
> +
> +	return b;
>  }
>  
>  int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 33aa2ca..1eed009 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -394,6 +394,7 @@ struct pci_host_bridge_window {
>  struct pci_host_bridge {
>  	struct device dev;
>  	struct pci_bus *bus;		/* root bus */
> +	int domain_nr;
>  	struct list_head windows;	/* pci_host_bridge_windows */
>  	void (*release_fn)(struct pci_host_bridge *);
>  	void *release_data;
> @@ -747,6 +748,9 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata);
>  struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  				    struct pci_ops *ops, void *sysdata,
>  				    struct list_head *resources);
> +struct pci_bus *pci_create_root_bus_in_domain(struct device *parent,
> +			int domain, int bus, struct pci_ops *ops,
> +			void *sysdata, struct list_head *resources);
>  int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax);
>  int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax);
>  void pci_bus_release_busn_res(struct pci_bus *b);
> -- 
> 1.9.0
> 
> 
> _______________________________________________
> linaro-kernel mailing list
> linaro-kernel at lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-kernel

Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Grant Likely]

On Wed,  5 Mar 2014 11:48:57 +0000, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> Several platforms use a rather generic version of parsing
> the device tree to find the host bridge ranges. Move the common code
> into the generic PCI code and use it to create a pci_host_bridge
> structure that can be used by arch code.
> 
> Based on early attempts by Andrew Murray to unify the code.
> Used powerpc and microblaze PCI code as starting point.
> 
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Tested-by: Tanmay Inamdar <tinamdar@apm.com>

Tentative ack for the whole series conditional on Arnd or Ben

g.

> ---
>  drivers/pci/host-bridge.c | 156 ++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h       |  13 +++
>  2 files changed, 169 insertions(+)
> 
> diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
> index 8708b652..db9f51a 100644
> --- a/drivers/pci/host-bridge.c
> +++ b/drivers/pci/host-bridge.c
> @@ -6,9 +6,14 @@
>  #include <linux/init.h>
>  #include <linux/pci.h>
>  #include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_pci.h>
> +#include <linux/slab.h>
>  
>  #include "pci.h"
>  
> +static atomic_t domain_nr = ATOMIC_INIT(-1);
> +
>  static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
>  {
>  	while (bus->parent)
> @@ -92,3 +97,154 @@ void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
>  	res->end = region->end + offset;
>  }
>  EXPORT_SYMBOL(pcibios_bus_to_resource);
> +
> +#ifdef CONFIG_OF
> +/**
> + * Simple version of the platform specific code for filtering the list
> + * of resources obtained from the ranges declaration in DT.
> + *
> + * Platforms can override this function in order to impose stronger
> + * constraints onto the list of resources that a host bridge can use.
> + * The filtered list will then be used to create a root bus and associate
> + * it with the host bridge.
> + *
> + */
> +int __weak pcibios_fixup_bridge_ranges(struct list_head *resources)
> +{
> +	return 0;
> +}
> +
> +/**
> + * pci_host_bridge_of_get_ranges - Parse PCI host bridge resources from DT
> + * @dev: device node of the host bridge having the range property
> + * @resources: list where the range of resources will be added after DT parsing
> + * @io_base: pointer to a variable that will contain the physical address for
> + * the start of the I/O range.
> + *
> + * It is the callers job to free the @resources list if an error is returned.
> + *
> + * This function will parse the "ranges" property of a PCI host bridge device
> + * node and setup the resource mapping based on its content. It is expected
> + * that the property conforms with the Power ePAPR document.
> + *
> + * Each architecture is then offered the chance of applying their own
> + * filtering of pci_host_bridge_windows based on their own restrictions by
> + * calling pcibios_fixup_bridge_ranges(). The filtered list of windows
> + * can then be used when creating a pci_host_bridge structure.
> + */
> +static int pci_host_bridge_of_get_ranges(struct device_node *dev,
> +		struct list_head *resources, resource_size_t *io_base)
> +{
> +	struct resource *res;
> +	struct of_pci_range range;
> +	struct of_pci_range_parser parser;
> +	int err;
> +
> +	pr_info("PCI host bridge %s ranges:\n", dev->full_name);
> +
> +	/* Check for ranges property */
> +	err = of_pci_range_parser_init(&parser, dev);
> +	if (err)
> +		return err;
> +
> +	pr_debug("Parsing ranges property...\n");
> +	for_each_of_pci_range(&parser, &range) {
> +		/* Read next ranges element */
> +		pr_debug("pci_space: 0x%08x pci_addr:0x%016llx ",
> +				range.pci_space, range.pci_addr);
> +		pr_debug("cpu_addr:0x%016llx size:0x%016llx\n",
> +					range.cpu_addr, range.size);
> +
> +		/*
> +		 * If we failed translation or got a zero-sized region
> +		 * then skip this range
> +		 */
> +		if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
> +			continue;
> +
> +		res = kzalloc(sizeof(struct resource), GFP_KERNEL);
> +		if (!res)
> +			return -ENOMEM;
> +
> +		of_pci_range_to_resource(&range, dev, res);
> +
> +		if (resource_type(res) == IORESOURCE_IO)
> +			*io_base = range.cpu_addr;
> +
> +		pci_add_resource_offset(resources, res,
> +				res->start - range.pci_addr);
> +	}
> +
> +	/* Apply architecture specific fixups for the ranges */
> +	return pcibios_fixup_bridge_ranges(resources);
> +}
> +
> +/**
> + * of_create_pci_host_bridge - Create a PCI host bridge structure using
> + * information passed in the DT.
> + * @parent: device owning this host bridge
> + * @ops: pci_ops associated with the host controller
> + * @host_data: opaque data structure used by the host controller.
> + *
> + * returns a pointer to the newly created pci_host_bridge structure, or
> + * NULL if the call failed.
> + *
> + * This function will try to obtain the host bridge domain number by
> + * using of_alias_get_id() call with "pci-domain" as a stem. If that
> + * fails, a local allocator will be used that will put each host bridge
> + * in a new domain.
> + */
> +struct pci_host_bridge *
> +of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops, void *host_data)
> +{
> +	int err, domain, busno;
> +	struct resource *bus_range;
> +	struct pci_bus *root_bus;
> +	struct pci_host_bridge *bridge;
> +	resource_size_t io_base;
> +	LIST_HEAD(res);
> +
> +	bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
> +	if (!bus_range)
> +		return ERR_PTR(-ENOMEM);
> +
> +	domain = of_alias_get_id(parent->of_node, "pci-domain");
> +	if (domain == -ENODEV)
> +		domain = atomic_inc_return(&domain_nr);
> +
> +	err = of_pci_parse_bus_range(parent->of_node, bus_range);
> +	if (err) {
> +		dev_info(parent, "No bus range for %s, using default [0-255]\n",
> +			parent->of_node->full_name);
> +		bus_range->start = 0;
> +		bus_range->end = 255;
> +		bus_range->flags = IORESOURCE_BUS;
> +	}
> +	busno = bus_range->start;
> +	pci_add_resource(&res, bus_range);
> +
> +	/* now parse the rest of host bridge bus ranges */
> +	err = pci_host_bridge_of_get_ranges(parent->of_node, &res, &io_base);
> +	if (err)
> +		goto err_create;
> +
> +	/* then create the root bus */
> +	root_bus = pci_create_root_bus_in_domain(parent, domain, busno,
> +						ops, host_data, &res);
> +	if (IS_ERR(root_bus)) {
> +		err = PTR_ERR(root_bus);
> +		goto err_create;
> +	}
> +
> +	bridge = to_pci_host_bridge(root_bus->bridge);
> +	bridge->io_base = io_base;
> +
> +	return bridge;
> +
> +err_create:
> +	pci_free_resource_list(&res);
> +	return ERR_PTR(err);
> +}
> +EXPORT_SYMBOL_GPL(of_create_pci_host_bridge);
> +
> +#endif /* CONFIG_OF */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 1eed009..40ddd3d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -395,6 +395,7 @@ struct pci_host_bridge {
>  	struct device dev;
>  	struct pci_bus *bus;		/* root bus */
>  	int domain_nr;
> +	resource_size_t io_base;	/* physical address for the start of I/O area */
>  	struct list_head windows;	/* pci_host_bridge_windows */
>  	void (*release_fn)(struct pci_host_bridge *);
>  	void *release_data;
> @@ -1786,11 +1787,23 @@ static inline struct device_node *pci_bus_to_OF_node(struct pci_bus *bus)
>  	return bus ? bus->dev.of_node : NULL;
>  }
>  
> +struct pci_host_bridge *
> +of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops,
> +			void *host_data);
> +
> +int pcibios_fixup_bridge_ranges(struct list_head *resources);
>  #else /* CONFIG_OF */
>  static inline void pci_set_of_node(struct pci_dev *dev) { }
>  static inline void pci_release_of_node(struct pci_dev *dev) { }
>  static inline void pci_set_bus_of_node(struct pci_bus *bus) { }
>  static inline void pci_release_bus_of_node(struct pci_bus *bus) { }
> +
> +static inline struct pci_host_bridge *
> +of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops,
> +			void *host_data)
> +{
> +	return NULL;
> +}
>  #endif  /* CONFIG_OF */
>  
>  #ifdef CONFIG_EEH
> -- 
> 1.9.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

[PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Grant Likely]

On Wed,  5 Mar 2014 11:48:57 +0000, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> Several platforms use a rather generic version of parsing
> the device tree to find the host bridge ranges. Move the common code
> into the generic PCI code and use it to create a pci_host_bridge
> structure that can be used by arch code.
> 
> Based on early attempts by Andrew Murray to unify the code.
> Used powerpc and microblaze PCI code as starting point.
> 
> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> Tested-by: Tanmay Inamdar <tinamdar@apm.com>

Tentative ack for the whole series conditional on Arnd or Ben

g.

> ---
>  drivers/pci/host-bridge.c | 156 ++++++++++++++++++++++++++++++++++++
>  include/linux/pci.h       |  13 +++
>  2 files changed, 169 insertions(+)
> 
> diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
> index 8708b652..db9f51a 100644
> --- a/drivers/pci/host-bridge.c
> +++ b/drivers/pci/host-bridge.c
> @@ -6,9 +6,14 @@
>  #include <linux/init.h>
>  #include <linux/pci.h>
>  #include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_pci.h>
> +#include <linux/slab.h>
>  
>  #include "pci.h"
>  
> +static atomic_t domain_nr = ATOMIC_INIT(-1);
> +
>  static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
>  {
>  	while (bus->parent)
> @@ -92,3 +97,154 @@ void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
>  	res->end = region->end + offset;
>  }
>  EXPORT_SYMBOL(pcibios_bus_to_resource);
> +
> +#ifdef CONFIG_OF
> +/**
> + * Simple version of the platform specific code for filtering the list
> + * of resources obtained from the ranges declaration in DT.
> + *
> + * Platforms can override this function in order to impose stronger
> + * constraints onto the list of resources that a host bridge can use.
> + * The filtered list will then be used to create a root bus and associate
> + * it with the host bridge.
> + *
> + */
> +int __weak pcibios_fixup_bridge_ranges(struct list_head *resources)
> +{
> +	return 0;
> +}
> +
> +/**
> + * pci_host_bridge_of_get_ranges - Parse PCI host bridge resources from DT
> + * @dev: device node of the host bridge having the range property
> + * @resources: list where the range of resources will be added after DT parsing
> + * @io_base: pointer to a variable that will contain the physical address for
> + * the start of the I/O range.
> + *
> + * It is the callers job to free the @resources list if an error is returned.
> + *
> + * This function will parse the "ranges" property of a PCI host bridge device
> + * node and setup the resource mapping based on its content. It is expected
> + * that the property conforms with the Power ePAPR document.
> + *
> + * Each architecture is then offered the chance of applying their own
> + * filtering of pci_host_bridge_windows based on their own restrictions by
> + * calling pcibios_fixup_bridge_ranges(). The filtered list of windows
> + * can then be used when creating a pci_host_bridge structure.
> + */
> +static int pci_host_bridge_of_get_ranges(struct device_node *dev,
> +		struct list_head *resources, resource_size_t *io_base)
> +{
> +	struct resource *res;
> +	struct of_pci_range range;
> +	struct of_pci_range_parser parser;
> +	int err;
> +
> +	pr_info("PCI host bridge %s ranges:\n", dev->full_name);
> +
> +	/* Check for ranges property */
> +	err = of_pci_range_parser_init(&parser, dev);
> +	if (err)
> +		return err;
> +
> +	pr_debug("Parsing ranges property...\n");
> +	for_each_of_pci_range(&parser, &range) {
> +		/* Read next ranges element */
> +		pr_debug("pci_space: 0x%08x pci_addr:0x%016llx ",
> +				range.pci_space, range.pci_addr);
> +		pr_debug("cpu_addr:0x%016llx size:0x%016llx\n",
> +					range.cpu_addr, range.size);
> +
> +		/*
> +		 * If we failed translation or got a zero-sized region
> +		 * then skip this range
> +		 */
> +		if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
> +			continue;
> +
> +		res = kzalloc(sizeof(struct resource), GFP_KERNEL);
> +		if (!res)
> +			return -ENOMEM;
> +
> +		of_pci_range_to_resource(&range, dev, res);
> +
> +		if (resource_type(res) == IORESOURCE_IO)
> +			*io_base = range.cpu_addr;
> +
> +		pci_add_resource_offset(resources, res,
> +				res->start - range.pci_addr);
> +	}
> +
> +	/* Apply architecture specific fixups for the ranges */
> +	return pcibios_fixup_bridge_ranges(resources);
> +}
> +
> +/**
> + * of_create_pci_host_bridge - Create a PCI host bridge structure using
> + * information passed in the DT.
> + * @parent: device owning this host bridge
> + * @ops: pci_ops associated with the host controller
> + * @host_data: opaque data structure used by the host controller.
> + *
> + * returns a pointer to the newly created pci_host_bridge structure, or
> + * NULL if the call failed.
> + *
> + * This function will try to obtain the host bridge domain number by
> + * using of_alias_get_id() call with "pci-domain" as a stem. If that
> + * fails, a local allocator will be used that will put each host bridge
> + * in a new domain.
> + */
> +struct pci_host_bridge *
> +of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops, void *host_data)
> +{
> +	int err, domain, busno;
> +	struct resource *bus_range;
> +	struct pci_bus *root_bus;
> +	struct pci_host_bridge *bridge;
> +	resource_size_t io_base;
> +	LIST_HEAD(res);
> +
> +	bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
> +	if (!bus_range)
> +		return ERR_PTR(-ENOMEM);
> +
> +	domain = of_alias_get_id(parent->of_node, "pci-domain");
> +	if (domain == -ENODEV)
> +		domain = atomic_inc_return(&domain_nr);
> +
> +	err = of_pci_parse_bus_range(parent->of_node, bus_range);
> +	if (err) {
> +		dev_info(parent, "No bus range for %s, using default [0-255]\n",
> +			parent->of_node->full_name);
> +		bus_range->start = 0;
> +		bus_range->end = 255;
> +		bus_range->flags = IORESOURCE_BUS;
> +	}
> +	busno = bus_range->start;
> +	pci_add_resource(&res, bus_range);
> +
> +	/* now parse the rest of host bridge bus ranges */
> +	err = pci_host_bridge_of_get_ranges(parent->of_node, &res, &io_base);
> +	if (err)
> +		goto err_create;
> +
> +	/* then create the root bus */
> +	root_bus = pci_create_root_bus_in_domain(parent, domain, busno,
> +						ops, host_data, &res);
> +	if (IS_ERR(root_bus)) {
> +		err = PTR_ERR(root_bus);
> +		goto err_create;
> +	}
> +
> +	bridge = to_pci_host_bridge(root_bus->bridge);
> +	bridge->io_base = io_base;
> +
> +	return bridge;
> +
> +err_create:
> +	pci_free_resource_list(&res);
> +	return ERR_PTR(err);
> +}
> +EXPORT_SYMBOL_GPL(of_create_pci_host_bridge);
> +
> +#endif /* CONFIG_OF */
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 1eed009..40ddd3d 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -395,6 +395,7 @@ struct pci_host_bridge {
>  	struct device dev;
>  	struct pci_bus *bus;		/* root bus */
>  	int domain_nr;
> +	resource_size_t io_base;	/* physical address for the start of I/O area */
>  	struct list_head windows;	/* pci_host_bridge_windows */
>  	void (*release_fn)(struct pci_host_bridge *);
>  	void *release_data;
> @@ -1786,11 +1787,23 @@ static inline struct device_node *pci_bus_to_OF_node(struct pci_bus *bus)
>  	return bus ? bus->dev.of_node : NULL;
>  }
>  
> +struct pci_host_bridge *
> +of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops,
> +			void *host_data);
> +
> +int pcibios_fixup_bridge_ranges(struct list_head *resources);
>  #else /* CONFIG_OF */
>  static inline void pci_set_of_node(struct pci_dev *dev) { }
>  static inline void pci_release_of_node(struct pci_dev *dev) { }
>  static inline void pci_set_bus_of_node(struct pci_bus *bus) { }
>  static inline void pci_release_bus_of_node(struct pci_bus *bus) { }
> +
> +static inline struct pci_host_bridge *
> +of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops,
> +			void *host_data)
> +{
> +	return NULL;
> +}
>  #endif  /* CONFIG_OF */
>  
>  #ifdef CONFIG_EEH
> -- 
> 1.9.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Liviu Dudau]

On Fri, Mar 07, 2014 at 09:14:27PM +0000, Grant Likely wrote:
> On Wed,  5 Mar 2014 11:48:57 +0000, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > Several platforms use a rather generic version of parsing
> > the device tree to find the host bridge ranges. Move the common code
> > into the generic PCI code and use it to create a pci_host_bridge
> > structure that can be used by arch code.
> > 
> > Based on early attempts by Andrew Murray to unify the code.
> > Used powerpc and microblaze PCI code as starting point.
> > 
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > Tested-by: Tanmay Inamdar <tinamdar@apm.com>
> 
> Tentative ack for the whole series conditional on Arnd or Ben

Thanks Grant!

Liviu

> 
> g.
> 
> > ---
> >  drivers/pci/host-bridge.c | 156 ++++++++++++++++++++++++++++++++++++
> >  include/linux/pci.h       |  13 +++
> >  2 files changed, 169 insertions(+)
> > 
> > diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
> > index 8708b652..db9f51a 100644
> > --- a/drivers/pci/host-bridge.c
> > +++ b/drivers/pci/host-bridge.c
> > @@ -6,9 +6,14 @@
> >  #include <linux/init.h>
> >  #include <linux/pci.h>
> >  #include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_pci.h>
> > +#include <linux/slab.h>
> >  
> >  #include "pci.h"
> >  
> > +static atomic_t domain_nr = ATOMIC_INIT(-1);
> > +
> >  static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
> >  {
> >  	while (bus->parent)
> > @@ -92,3 +97,154 @@ void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
> >  	res->end = region->end + offset;
> >  }
> >  EXPORT_SYMBOL(pcibios_bus_to_resource);
> > +
> > +#ifdef CONFIG_OF
> > +/**
> > + * Simple version of the platform specific code for filtering the list
> > + * of resources obtained from the ranges declaration in DT.
> > + *
> > + * Platforms can override this function in order to impose stronger
> > + * constraints onto the list of resources that a host bridge can use.
> > + * The filtered list will then be used to create a root bus and associate
> > + * it with the host bridge.
> > + *
> > + */
> > +int __weak pcibios_fixup_bridge_ranges(struct list_head *resources)
> > +{
> > +	return 0;
> > +}
> > +
> > +/**
> > + * pci_host_bridge_of_get_ranges - Parse PCI host bridge resources from DT
> > + * @dev: device node of the host bridge having the range property
> > + * @resources: list where the range of resources will be added after DT parsing
> > + * @io_base: pointer to a variable that will contain the physical address for
> > + * the start of the I/O range.
> > + *
> > + * It is the callers job to free the @resources list if an error is returned.
> > + *
> > + * This function will parse the "ranges" property of a PCI host bridge device
> > + * node and setup the resource mapping based on its content. It is expected
> > + * that the property conforms with the Power ePAPR document.
> > + *
> > + * Each architecture is then offered the chance of applying their own
> > + * filtering of pci_host_bridge_windows based on their own restrictions by
> > + * calling pcibios_fixup_bridge_ranges(). The filtered list of windows
> > + * can then be used when creating a pci_host_bridge structure.
> > + */
> > +static int pci_host_bridge_of_get_ranges(struct device_node *dev,
> > +		struct list_head *resources, resource_size_t *io_base)
> > +{
> > +	struct resource *res;
> > +	struct of_pci_range range;
> > +	struct of_pci_range_parser parser;
> > +	int err;
> > +
> > +	pr_info("PCI host bridge %s ranges:\n", dev->full_name);
> > +
> > +	/* Check for ranges property */
> > +	err = of_pci_range_parser_init(&parser, dev);
> > +	if (err)
> > +		return err;
> > +
> > +	pr_debug("Parsing ranges property...\n");
> > +	for_each_of_pci_range(&parser, &range) {
> > +		/* Read next ranges element */
> > +		pr_debug("pci_space: 0x%08x pci_addr:0x%016llx ",
> > +				range.pci_space, range.pci_addr);
> > +		pr_debug("cpu_addr:0x%016llx size:0x%016llx\n",
> > +					range.cpu_addr, range.size);
> > +
> > +		/*
> > +		 * If we failed translation or got a zero-sized region
> > +		 * then skip this range
> > +		 */
> > +		if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
> > +			continue;
> > +
> > +		res = kzalloc(sizeof(struct resource), GFP_KERNEL);
> > +		if (!res)
> > +			return -ENOMEM;
> > +
> > +		of_pci_range_to_resource(&range, dev, res);
> > +
> > +		if (resource_type(res) == IORESOURCE_IO)
> > +			*io_base = range.cpu_addr;
> > +
> > +		pci_add_resource_offset(resources, res,
> > +				res->start - range.pci_addr);
> > +	}
> > +
> > +	/* Apply architecture specific fixups for the ranges */
> > +	return pcibios_fixup_bridge_ranges(resources);
> > +}
> > +
> > +/**
> > + * of_create_pci_host_bridge - Create a PCI host bridge structure using
> > + * information passed in the DT.
> > + * @parent: device owning this host bridge
> > + * @ops: pci_ops associated with the host controller
> > + * @host_data: opaque data structure used by the host controller.
> > + *
> > + * returns a pointer to the newly created pci_host_bridge structure, or
> > + * NULL if the call failed.
> > + *
> > + * This function will try to obtain the host bridge domain number by
> > + * using of_alias_get_id() call with "pci-domain" as a stem. If that
> > + * fails, a local allocator will be used that will put each host bridge
> > + * in a new domain.
> > + */
> > +struct pci_host_bridge *
> > +of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops, void *host_data)
> > +{
> > +	int err, domain, busno;
> > +	struct resource *bus_range;
> > +	struct pci_bus *root_bus;
> > +	struct pci_host_bridge *bridge;
> > +	resource_size_t io_base;
> > +	LIST_HEAD(res);
> > +
> > +	bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
> > +	if (!bus_range)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	domain = of_alias_get_id(parent->of_node, "pci-domain");
> > +	if (domain == -ENODEV)
> > +		domain = atomic_inc_return(&domain_nr);
> > +
> > +	err = of_pci_parse_bus_range(parent->of_node, bus_range);
> > +	if (err) {
> > +		dev_info(parent, "No bus range for %s, using default [0-255]\n",
> > +			parent->of_node->full_name);
> > +		bus_range->start = 0;
> > +		bus_range->end = 255;
> > +		bus_range->flags = IORESOURCE_BUS;
> > +	}
> > +	busno = bus_range->start;
> > +	pci_add_resource(&res, bus_range);
> > +
> > +	/* now parse the rest of host bridge bus ranges */
> > +	err = pci_host_bridge_of_get_ranges(parent->of_node, &res, &io_base);
> > +	if (err)
> > +		goto err_create;
> > +
> > +	/* then create the root bus */
> > +	root_bus = pci_create_root_bus_in_domain(parent, domain, busno,
> > +						ops, host_data, &res);
> > +	if (IS_ERR(root_bus)) {
> > +		err = PTR_ERR(root_bus);
> > +		goto err_create;
> > +	}
> > +
> > +	bridge = to_pci_host_bridge(root_bus->bridge);
> > +	bridge->io_base = io_base;
> > +
> > +	return bridge;
> > +
> > +err_create:
> > +	pci_free_resource_list(&res);
> > +	return ERR_PTR(err);
> > +}
> > +EXPORT_SYMBOL_GPL(of_create_pci_host_bridge);
> > +
> > +#endif /* CONFIG_OF */
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 1eed009..40ddd3d 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -395,6 +395,7 @@ struct pci_host_bridge {
> >  	struct device dev;
> >  	struct pci_bus *bus;		/* root bus */
> >  	int domain_nr;
> > +	resource_size_t io_base;	/* physical address for the start of I/O area */
> >  	struct list_head windows;	/* pci_host_bridge_windows */
> >  	void (*release_fn)(struct pci_host_bridge *);
> >  	void *release_data;
> > @@ -1786,11 +1787,23 @@ static inline struct device_node *pci_bus_to_OF_node(struct pci_bus *bus)
> >  	return bus ? bus->dev.of_node : NULL;
> >  }
> >  
> > +struct pci_host_bridge *
> > +of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops,
> > +			void *host_data);
> > +
> > +int pcibios_fixup_bridge_ranges(struct list_head *resources);
> >  #else /* CONFIG_OF */
> >  static inline void pci_set_of_node(struct pci_dev *dev) { }
> >  static inline void pci_release_of_node(struct pci_dev *dev) { }
> >  static inline void pci_set_bus_of_node(struct pci_bus *bus) { }
> >  static inline void pci_release_bus_of_node(struct pci_bus *bus) { }
> > +
> > +static inline struct pci_host_bridge *
> > +of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops,
> > +			void *host_data)
> > +{
> > +	return NULL;
> > +}
> >  #endif  /* CONFIG_OF */
> >  
> >  #ifdef CONFIG_EEH
> > -- 
> > 1.9.0
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 
> --
> 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 v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Liviu Dudau]

On Fri, Mar 07, 2014 at 09:14:27PM +0000, Grant Likely wrote:
> On Wed,  5 Mar 2014 11:48:57 +0000, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > Several platforms use a rather generic version of parsing
> > the device tree to find the host bridge ranges. Move the common code
> > into the generic PCI code and use it to create a pci_host_bridge
> > structure that can be used by arch code.
> > 
> > Based on early attempts by Andrew Murray to unify the code.
> > Used powerpc and microblaze PCI code as starting point.
> > 
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > Tested-by: Tanmay Inamdar <tinamdar@apm.com>
> 
> Tentative ack for the whole series conditional on Arnd or Ben

Thanks Grant!

Liviu

> 
> g.
> 
> > ---
> >  drivers/pci/host-bridge.c | 156 ++++++++++++++++++++++++++++++++++++
> >  include/linux/pci.h       |  13 +++
> >  2 files changed, 169 insertions(+)
> > 
> > diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
> > index 8708b652..db9f51a 100644
> > --- a/drivers/pci/host-bridge.c
> > +++ b/drivers/pci/host-bridge.c
> > @@ -6,9 +6,14 @@
> >  #include <linux/init.h>
> >  #include <linux/pci.h>
> >  #include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_pci.h>
> > +#include <linux/slab.h>
> >  
> >  #include "pci.h"
> >  
> > +static atomic_t domain_nr = ATOMIC_INIT(-1);
> > +
> >  static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
> >  {
> >  	while (bus->parent)
> > @@ -92,3 +97,154 @@ void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
> >  	res->end = region->end + offset;
> >  }
> >  EXPORT_SYMBOL(pcibios_bus_to_resource);
> > +
> > +#ifdef CONFIG_OF
> > +/**
> > + * Simple version of the platform specific code for filtering the list
> > + * of resources obtained from the ranges declaration in DT.
> > + *
> > + * Platforms can override this function in order to impose stronger
> > + * constraints onto the list of resources that a host bridge can use.
> > + * The filtered list will then be used to create a root bus and associate
> > + * it with the host bridge.
> > + *
> > + */
> > +int __weak pcibios_fixup_bridge_ranges(struct list_head *resources)
> > +{
> > +	return 0;
> > +}
> > +
> > +/**
> > + * pci_host_bridge_of_get_ranges - Parse PCI host bridge resources from DT
> > + * @dev: device node of the host bridge having the range property
> > + * @resources: list where the range of resources will be added after DT parsing
> > + * @io_base: pointer to a variable that will contain the physical address for
> > + * the start of the I/O range.
> > + *
> > + * It is the callers job to free the @resources list if an error is returned.
> > + *
> > + * This function will parse the "ranges" property of a PCI host bridge device
> > + * node and setup the resource mapping based on its content. It is expected
> > + * that the property conforms with the Power ePAPR document.
> > + *
> > + * Each architecture is then offered the chance of applying their own
> > + * filtering of pci_host_bridge_windows based on their own restrictions by
> > + * calling pcibios_fixup_bridge_ranges(). The filtered list of windows
> > + * can then be used when creating a pci_host_bridge structure.
> > + */
> > +static int pci_host_bridge_of_get_ranges(struct device_node *dev,
> > +		struct list_head *resources, resource_size_t *io_base)
> > +{
> > +	struct resource *res;
> > +	struct of_pci_range range;
> > +	struct of_pci_range_parser parser;
> > +	int err;
> > +
> > +	pr_info("PCI host bridge %s ranges:\n", dev->full_name);
> > +
> > +	/* Check for ranges property */
> > +	err = of_pci_range_parser_init(&parser, dev);
> > +	if (err)
> > +		return err;
> > +
> > +	pr_debug("Parsing ranges property...\n");
> > +	for_each_of_pci_range(&parser, &range) {
> > +		/* Read next ranges element */
> > +		pr_debug("pci_space: 0x%08x pci_addr:0x%016llx ",
> > +				range.pci_space, range.pci_addr);
> > +		pr_debug("cpu_addr:0x%016llx size:0x%016llx\n",
> > +					range.cpu_addr, range.size);
> > +
> > +		/*
> > +		 * If we failed translation or got a zero-sized region
> > +		 * then skip this range
> > +		 */
> > +		if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
> > +			continue;
> > +
> > +		res = kzalloc(sizeof(struct resource), GFP_KERNEL);
> > +		if (!res)
> > +			return -ENOMEM;
> > +
> > +		of_pci_range_to_resource(&range, dev, res);
> > +
> > +		if (resource_type(res) == IORESOURCE_IO)
> > +			*io_base = range.cpu_addr;
> > +
> > +		pci_add_resource_offset(resources, res,
> > +				res->start - range.pci_addr);
> > +	}
> > +
> > +	/* Apply architecture specific fixups for the ranges */
> > +	return pcibios_fixup_bridge_ranges(resources);
> > +}
> > +
> > +/**
> > + * of_create_pci_host_bridge - Create a PCI host bridge structure using
> > + * information passed in the DT.
> > + * @parent: device owning this host bridge
> > + * @ops: pci_ops associated with the host controller
> > + * @host_data: opaque data structure used by the host controller.
> > + *
> > + * returns a pointer to the newly created pci_host_bridge structure, or
> > + * NULL if the call failed.
> > + *
> > + * This function will try to obtain the host bridge domain number by
> > + * using of_alias_get_id() call with "pci-domain" as a stem. If that
> > + * fails, a local allocator will be used that will put each host bridge
> > + * in a new domain.
> > + */
> > +struct pci_host_bridge *
> > +of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops, void *host_data)
> > +{
> > +	int err, domain, busno;
> > +	struct resource *bus_range;
> > +	struct pci_bus *root_bus;
> > +	struct pci_host_bridge *bridge;
> > +	resource_size_t io_base;
> > +	LIST_HEAD(res);
> > +
> > +	bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
> > +	if (!bus_range)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	domain = of_alias_get_id(parent->of_node, "pci-domain");
> > +	if (domain == -ENODEV)
> > +		domain = atomic_inc_return(&domain_nr);
> > +
> > +	err = of_pci_parse_bus_range(parent->of_node, bus_range);
> > +	if (err) {
> > +		dev_info(parent, "No bus range for %s, using default [0-255]\n",
> > +			parent->of_node->full_name);
> > +		bus_range->start = 0;
> > +		bus_range->end = 255;
> > +		bus_range->flags = IORESOURCE_BUS;
> > +	}
> > +	busno = bus_range->start;
> > +	pci_add_resource(&res, bus_range);
> > +
> > +	/* now parse the rest of host bridge bus ranges */
> > +	err = pci_host_bridge_of_get_ranges(parent->of_node, &res, &io_base);
> > +	if (err)
> > +		goto err_create;
> > +
> > +	/* then create the root bus */
> > +	root_bus = pci_create_root_bus_in_domain(parent, domain, busno,
> > +						ops, host_data, &res);
> > +	if (IS_ERR(root_bus)) {
> > +		err = PTR_ERR(root_bus);
> > +		goto err_create;
> > +	}
> > +
> > +	bridge = to_pci_host_bridge(root_bus->bridge);
> > +	bridge->io_base = io_base;
> > +
> > +	return bridge;
> > +
> > +err_create:
> > +	pci_free_resource_list(&res);
> > +	return ERR_PTR(err);
> > +}
> > +EXPORT_SYMBOL_GPL(of_create_pci_host_bridge);
> > +
> > +#endif /* CONFIG_OF */
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 1eed009..40ddd3d 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -395,6 +395,7 @@ struct pci_host_bridge {
> >  	struct device dev;
> >  	struct pci_bus *bus;		/* root bus */
> >  	int domain_nr;
> > +	resource_size_t io_base;	/* physical address for the start of I/O area */
> >  	struct list_head windows;	/* pci_host_bridge_windows */
> >  	void (*release_fn)(struct pci_host_bridge *);
> >  	void *release_data;
> > @@ -1786,11 +1787,23 @@ static inline struct device_node *pci_bus_to_OF_node(struct pci_bus *bus)
> >  	return bus ? bus->dev.of_node : NULL;
> >  }
> >  
> > +struct pci_host_bridge *
> > +of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops,
> > +			void *host_data);
> > +
> > +int pcibios_fixup_bridge_ranges(struct list_head *resources);
> >  #else /* CONFIG_OF */
> >  static inline void pci_set_of_node(struct pci_dev *dev) { }
> >  static inline void pci_release_of_node(struct pci_dev *dev) { }
> >  static inline void pci_set_bus_of_node(struct pci_bus *bus) { }
> >  static inline void pci_release_bus_of_node(struct pci_bus *bus) { }
> > +
> > +static inline struct pci_host_bridge *
> > +of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops,
> > +			void *host_data)
> > +{
> > +	return NULL;
> > +}
> >  #endif  /* CONFIG_OF */
> >  
> >  #ifdef CONFIG_EEH
> > -- 
> > 1.9.0
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 
> --
> 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
> 

[PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Liviu Dudau]

On Fri, Mar 07, 2014 at 09:14:27PM +0000, Grant Likely wrote:
> On Wed,  5 Mar 2014 11:48:57 +0000, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > Several platforms use a rather generic version of parsing
> > the device tree to find the host bridge ranges. Move the common code
> > into the generic PCI code and use it to create a pci_host_bridge
> > structure that can be used by arch code.
> > 
> > Based on early attempts by Andrew Murray to unify the code.
> > Used powerpc and microblaze PCI code as starting point.
> > 
> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
> > Tested-by: Tanmay Inamdar <tinamdar@apm.com>
> 
> Tentative ack for the whole series conditional on Arnd or Ben

Thanks Grant!

Liviu

> 
> g.
> 
> > ---
> >  drivers/pci/host-bridge.c | 156 ++++++++++++++++++++++++++++++++++++
> >  include/linux/pci.h       |  13 +++
> >  2 files changed, 169 insertions(+)
> > 
> > diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
> > index 8708b652..db9f51a 100644
> > --- a/drivers/pci/host-bridge.c
> > +++ b/drivers/pci/host-bridge.c
> > @@ -6,9 +6,14 @@
> >  #include <linux/init.h>
> >  #include <linux/pci.h>
> >  #include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_pci.h>
> > +#include <linux/slab.h>
> >  
> >  #include "pci.h"
> >  
> > +static atomic_t domain_nr = ATOMIC_INIT(-1);
> > +
> >  static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
> >  {
> >  	while (bus->parent)
> > @@ -92,3 +97,154 @@ void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
> >  	res->end = region->end + offset;
> >  }
> >  EXPORT_SYMBOL(pcibios_bus_to_resource);
> > +
> > +#ifdef CONFIG_OF
> > +/**
> > + * Simple version of the platform specific code for filtering the list
> > + * of resources obtained from the ranges declaration in DT.
> > + *
> > + * Platforms can override this function in order to impose stronger
> > + * constraints onto the list of resources that a host bridge can use.
> > + * The filtered list will then be used to create a root bus and associate
> > + * it with the host bridge.
> > + *
> > + */
> > +int __weak pcibios_fixup_bridge_ranges(struct list_head *resources)
> > +{
> > +	return 0;
> > +}
> > +
> > +/**
> > + * pci_host_bridge_of_get_ranges - Parse PCI host bridge resources from DT
> > + * @dev: device node of the host bridge having the range property
> > + * @resources: list where the range of resources will be added after DT parsing
> > + * @io_base: pointer to a variable that will contain the physical address for
> > + * the start of the I/O range.
> > + *
> > + * It is the callers job to free the @resources list if an error is returned.
> > + *
> > + * This function will parse the "ranges" property of a PCI host bridge device
> > + * node and setup the resource mapping based on its content. It is expected
> > + * that the property conforms with the Power ePAPR document.
> > + *
> > + * Each architecture is then offered the chance of applying their own
> > + * filtering of pci_host_bridge_windows based on their own restrictions by
> > + * calling pcibios_fixup_bridge_ranges(). The filtered list of windows
> > + * can then be used when creating a pci_host_bridge structure.
> > + */
> > +static int pci_host_bridge_of_get_ranges(struct device_node *dev,
> > +		struct list_head *resources, resource_size_t *io_base)
> > +{
> > +	struct resource *res;
> > +	struct of_pci_range range;
> > +	struct of_pci_range_parser parser;
> > +	int err;
> > +
> > +	pr_info("PCI host bridge %s ranges:\n", dev->full_name);
> > +
> > +	/* Check for ranges property */
> > +	err = of_pci_range_parser_init(&parser, dev);
> > +	if (err)
> > +		return err;
> > +
> > +	pr_debug("Parsing ranges property...\n");
> > +	for_each_of_pci_range(&parser, &range) {
> > +		/* Read next ranges element */
> > +		pr_debug("pci_space: 0x%08x pci_addr:0x%016llx ",
> > +				range.pci_space, range.pci_addr);
> > +		pr_debug("cpu_addr:0x%016llx size:0x%016llx\n",
> > +					range.cpu_addr, range.size);
> > +
> > +		/*
> > +		 * If we failed translation or got a zero-sized region
> > +		 * then skip this range
> > +		 */
> > +		if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
> > +			continue;
> > +
> > +		res = kzalloc(sizeof(struct resource), GFP_KERNEL);
> > +		if (!res)
> > +			return -ENOMEM;
> > +
> > +		of_pci_range_to_resource(&range, dev, res);
> > +
> > +		if (resource_type(res) == IORESOURCE_IO)
> > +			*io_base = range.cpu_addr;
> > +
> > +		pci_add_resource_offset(resources, res,
> > +				res->start - range.pci_addr);
> > +	}
> > +
> > +	/* Apply architecture specific fixups for the ranges */
> > +	return pcibios_fixup_bridge_ranges(resources);
> > +}
> > +
> > +/**
> > + * of_create_pci_host_bridge - Create a PCI host bridge structure using
> > + * information passed in the DT.
> > + * @parent: device owning this host bridge
> > + * @ops: pci_ops associated with the host controller
> > + * @host_data: opaque data structure used by the host controller.
> > + *
> > + * returns a pointer to the newly created pci_host_bridge structure, or
> > + * NULL if the call failed.
> > + *
> > + * This function will try to obtain the host bridge domain number by
> > + * using of_alias_get_id() call with "pci-domain" as a stem. If that
> > + * fails, a local allocator will be used that will put each host bridge
> > + * in a new domain.
> > + */
> > +struct pci_host_bridge *
> > +of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops, void *host_data)
> > +{
> > +	int err, domain, busno;
> > +	struct resource *bus_range;
> > +	struct pci_bus *root_bus;
> > +	struct pci_host_bridge *bridge;
> > +	resource_size_t io_base;
> > +	LIST_HEAD(res);
> > +
> > +	bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
> > +	if (!bus_range)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	domain = of_alias_get_id(parent->of_node, "pci-domain");
> > +	if (domain == -ENODEV)
> > +		domain = atomic_inc_return(&domain_nr);
> > +
> > +	err = of_pci_parse_bus_range(parent->of_node, bus_range);
> > +	if (err) {
> > +		dev_info(parent, "No bus range for %s, using default [0-255]\n",
> > +			parent->of_node->full_name);
> > +		bus_range->start = 0;
> > +		bus_range->end = 255;
> > +		bus_range->flags = IORESOURCE_BUS;
> > +	}
> > +	busno = bus_range->start;
> > +	pci_add_resource(&res, bus_range);
> > +
> > +	/* now parse the rest of host bridge bus ranges */
> > +	err = pci_host_bridge_of_get_ranges(parent->of_node, &res, &io_base);
> > +	if (err)
> > +		goto err_create;
> > +
> > +	/* then create the root bus */
> > +	root_bus = pci_create_root_bus_in_domain(parent, domain, busno,
> > +						ops, host_data, &res);
> > +	if (IS_ERR(root_bus)) {
> > +		err = PTR_ERR(root_bus);
> > +		goto err_create;
> > +	}
> > +
> > +	bridge = to_pci_host_bridge(root_bus->bridge);
> > +	bridge->io_base = io_base;
> > +
> > +	return bridge;
> > +
> > +err_create:
> > +	pci_free_resource_list(&res);
> > +	return ERR_PTR(err);
> > +}
> > +EXPORT_SYMBOL_GPL(of_create_pci_host_bridge);
> > +
> > +#endif /* CONFIG_OF */
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 1eed009..40ddd3d 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -395,6 +395,7 @@ struct pci_host_bridge {
> >  	struct device dev;
> >  	struct pci_bus *bus;		/* root bus */
> >  	int domain_nr;
> > +	resource_size_t io_base;	/* physical address for the start of I/O area */
> >  	struct list_head windows;	/* pci_host_bridge_windows */
> >  	void (*release_fn)(struct pci_host_bridge *);
> >  	void *release_data;
> > @@ -1786,11 +1787,23 @@ static inline struct device_node *pci_bus_to_OF_node(struct pci_bus *bus)
> >  	return bus ? bus->dev.of_node : NULL;
> >  }
> >  
> > +struct pci_host_bridge *
> > +of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops,
> > +			void *host_data);
> > +
> > +int pcibios_fixup_bridge_ranges(struct list_head *resources);
> >  #else /* CONFIG_OF */
> >  static inline void pci_set_of_node(struct pci_dev *dev) { }
> >  static inline void pci_release_of_node(struct pci_dev *dev) { }
> >  static inline void pci_set_bus_of_node(struct pci_bus *bus) { }
> >  static inline void pci_release_bus_of_node(struct pci_bus *bus) { }
> > +
> > +static inline struct pci_host_bridge *
> > +of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops,
> > +			void *host_data)
> > +{
> > +	return NULL;
> > +}
> >  #endif  /* CONFIG_OF */
> >  
> >  #ifdef CONFIG_EEH
> > -- 
> > 1.9.0
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo at vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Arnd Bergmann]

On Wednesday 05 March 2014, Liviu Dudau wrote:
> +
> +	pr_debug("Parsing ranges property...\n");
> +	for_each_of_pci_range(&parser, &range) {
> +		/* Read next ranges element */
> +		pr_debug("pci_space: 0x%08x pci_addr:0x%016llx ",
> +				range.pci_space, range.pci_addr);
> +		pr_debug("cpu_addr:0x%016llx size:0x%016llx\n",
> +					range.cpu_addr, range.size);
> +
> +		/*
> +		 * If we failed translation or got a zero-sized region
> +		 * then skip this range
> +		 */
> +		if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
> +			continue;
> +
> +		res = kzalloc(sizeof(struct resource), GFP_KERNEL);
> +		if (!res)
> +			return -ENOMEM;
> +
> +		of_pci_range_to_resource(&range, dev, res);
> +
> +		if (resource_type(res) == IORESOURCE_IO)
> +			*io_base = range.cpu_addr;
> +
> +		pci_add_resource_offset(resources, res,
> +				res->start - range.pci_addr);
> +	}

As mentioned regarding the pci_register_io_range() helper, x86
would not enter the 'resource_type(res) == IORESOURCE_IO' code path,
which on the one hand is fine so we can return an error from
pci_register_io_range() there, but I think it will lead to
io_base getting an uninitialized content.

There could also be other reasons why pci_register_io_range() fails,
e.g. because the space is exhausted, and I think we should try to
catch that here and skip the pci_add_resource_offset() and io_base
assignment then.

	Arnd

[PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Arnd Bergmann]

On Wednesday 05 March 2014, Liviu Dudau wrote:
> +
> +	pr_debug("Parsing ranges property...\n");
> +	for_each_of_pci_range(&parser, &range) {
> +		/* Read next ranges element */
> +		pr_debug("pci_space: 0x%08x pci_addr:0x%016llx ",
> +				range.pci_space, range.pci_addr);
> +		pr_debug("cpu_addr:0x%016llx size:0x%016llx\n",
> +					range.cpu_addr, range.size);
> +
> +		/*
> +		 * If we failed translation or got a zero-sized region
> +		 * then skip this range
> +		 */
> +		if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
> +			continue;
> +
> +		res = kzalloc(sizeof(struct resource), GFP_KERNEL);
> +		if (!res)
> +			return -ENOMEM;
> +
> +		of_pci_range_to_resource(&range, dev, res);
> +
> +		if (resource_type(res) == IORESOURCE_IO)
> +			*io_base = range.cpu_addr;
> +
> +		pci_add_resource_offset(resources, res,
> +				res->start - range.pci_addr);
> +	}

As mentioned regarding the pci_register_io_range() helper, x86
would not enter the 'resource_type(res) == IORESOURCE_IO' code path,
which on the one hand is fine so we can return an error from
pci_register_io_range() there, but I think it will lead to
io_base getting an uninitialized content.

There could also be other reasons why pci_register_io_range() fails,
e.g. because the space is exhausted, and I think we should try to
catch that here and skip the pci_add_resource_offset() and io_base
assignment then.

	Arnd

Re: [PATCH v6 2/6] pci: OF: Fix the conversion of IO ranges into IO resources.

[Arnd Bergmann]

On Wednesday 05 March 2014, Liviu Dudau wrote:
> +               unsigned long port = -1;
> +               if (!pci_register_io_range(range->cpu_addr, range->size))
> +                       port = pci_address_to_pio(range->cpu_addr);

I don't remember the outcome of my suggestion to just return 'port' from
pci_register_io_range(). In my mind that would be much simpler than
adding the range in a data structure just so we can look it up in the very
next line.

	Arnd

[PATCH v6 2/6] pci: OF: Fix the conversion of IO ranges into IO resources.

[Arnd Bergmann]

On Wednesday 05 March 2014, Liviu Dudau wrote:
> +               unsigned long port = -1;
> +               if (!pci_register_io_range(range->cpu_addr, range->size))
> +                       port = pci_address_to_pio(range->cpu_addr);

I don't remember the outcome of my suggestion to just return 'port' from
pci_register_io_range(). In my mind that would be much simpler than
adding the range in a data structure just so we can look it up in the very
next line.

	Arnd

Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Liviu Dudau]

On Sat, Mar 08, 2014 at 05:15:08PM +0000, Arnd Bergmann wrote:
> On Wednesday 05 March 2014, Liviu Dudau wrote:
> > +
> > +	pr_debug("Parsing ranges property...\n");
> > +	for_each_of_pci_range(&parser, &range) {
> > +		/* Read next ranges element */
> > +		pr_debug("pci_space: 0x%08x pci_addr:0x%016llx ",
> > +				range.pci_space, range.pci_addr);
> > +		pr_debug("cpu_addr:0x%016llx size:0x%016llx\n",
> > +					range.cpu_addr, range.size);
> > +
> > +		/*
> > +		 * If we failed translation or got a zero-sized region
> > +		 * then skip this range
> > +		 */
> > +		if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
> > +			continue;
> > +
> > +		res = kzalloc(sizeof(struct resource), GFP_KERNEL);
> > +		if (!res)
> > +			return -ENOMEM;
> > +
> > +		of_pci_range_to_resource(&range, dev, res);
> > +
> > +		if (resource_type(res) == IORESOURCE_IO)
> > +			*io_base = range.cpu_addr;
> > +
> > +		pci_add_resource_offset(resources, res,
> > +				res->start - range.pci_addr);
> > +	}
> 
> As mentioned regarding the pci_register_io_range() helper, x86
> would not enter the 'resource_type(res) == IORESOURCE_IO' code path,
> which on the one hand is fine so we can return an error from
> pci_register_io_range() there, but I think it will lead to
> io_base getting an uninitialized content.
> 
> There could also be other reasons why pci_register_io_range() fails,
> e.g. because the space is exhausted, and I think we should try to
> catch that here and skip the pci_add_resource_offset() and io_base
> assignment then.

Hi Arnd,

I will try to improve the error handling in the next patchset version.
However I am still confused about the earlier discussion on
pci_register_io_range(). Your suggestion initially was to return an
error in the default weak implementation, but in your last email you
are talking about returning 'port'. My idea when I've introduced the
helper function was that it would return an error if it fails to
register the IO range and zero otherwise. I agree that we can treat
the default 'do nothing with the IO range' case as an error, with
the caveat that will force architectures that use this code to
provide their own implementation of pci_register_io_range() in order
to avoid failure, even for the cases where the architecture has a 1:1
mapping between IO and CPU addresses.

I've just noticed that my home server has silently dropped my reply
to you from the 7th of March, so I'm going to resend it using ARM's
setup.

Best regards,
Liviu


> 
> 	Arnd
> 

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

[PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Liviu Dudau]

On Sat, Mar 08, 2014 at 05:15:08PM +0000, Arnd Bergmann wrote:
> On Wednesday 05 March 2014, Liviu Dudau wrote:
> > +
> > +	pr_debug("Parsing ranges property...\n");
> > +	for_each_of_pci_range(&parser, &range) {
> > +		/* Read next ranges element */
> > +		pr_debug("pci_space: 0x%08x pci_addr:0x%016llx ",
> > +				range.pci_space, range.pci_addr);
> > +		pr_debug("cpu_addr:0x%016llx size:0x%016llx\n",
> > +					range.cpu_addr, range.size);
> > +
> > +		/*
> > +		 * If we failed translation or got a zero-sized region
> > +		 * then skip this range
> > +		 */
> > +		if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
> > +			continue;
> > +
> > +		res = kzalloc(sizeof(struct resource), GFP_KERNEL);
> > +		if (!res)
> > +			return -ENOMEM;
> > +
> > +		of_pci_range_to_resource(&range, dev, res);
> > +
> > +		if (resource_type(res) == IORESOURCE_IO)
> > +			*io_base = range.cpu_addr;
> > +
> > +		pci_add_resource_offset(resources, res,
> > +				res->start - range.pci_addr);
> > +	}
> 
> As mentioned regarding the pci_register_io_range() helper, x86
> would not enter the 'resource_type(res) == IORESOURCE_IO' code path,
> which on the one hand is fine so we can return an error from
> pci_register_io_range() there, but I think it will lead to
> io_base getting an uninitialized content.
> 
> There could also be other reasons why pci_register_io_range() fails,
> e.g. because the space is exhausted, and I think we should try to
> catch that here and skip the pci_add_resource_offset() and io_base
> assignment then.

Hi Arnd,

I will try to improve the error handling in the next patchset version.
However I am still confused about the earlier discussion on
pci_register_io_range(). Your suggestion initially was to return an
error in the default weak implementation, but in your last email you
are talking about returning 'port'. My idea when I've introduced the
helper function was that it would return an error if it fails to
register the IO range and zero otherwise. I agree that we can treat
the default 'do nothing with the IO range' case as an error, with
the caveat that will force architectures that use this code to
provide their own implementation of pci_register_io_range() in order
to avoid failure, even for the cases where the architecture has a 1:1
mapping between IO and CPU addresses.

I've just noticed that my home server has silently dropped my reply
to you from the 7th of March, so I'm going to resend it using ARM's
setup.

Best regards,
Liviu


> 
> 	Arnd
> 

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

Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Arnd Bergmann]

On Monday 10 March 2014 14:44:14 Liviu Dudau wrote:
> I will try to improve the error handling in the next patchset version.
> However I am still confused about the earlier discussion on
> pci_register_io_range(). Your suggestion initially was to return an
> error in the default weak implementation, but in your last email you
> are talking about returning 'port'.

You can do either one: 'port' should be positive or zero, while the
error would always be negative. We do the same thing in many interfaces
in the kernel.

> My idea when I've introduced the
> helper function was that it would return an error if it fails to
> register the IO range and zero otherwise. I agree that we can treat
> the default 'do nothing with the IO range' case as an error, with
> the caveat that will force architectures that use this code to
> provide their own implementation of pci_register_io_range() in order
> to avoid failure, even for the cases where the architecture has a 1:1
> mapping between IO and CPU addresses.

Which architectures are you thinking of? The only one I know that
does this is ia64, and we won't ever have to support this helper
on that architecture.

I did not ask to treat 'do nothing with the IO range' as an error,
what I meant is that we should treat 'architecture cannot translate
from I/O space to memory space but DT lists a translation anyway'
as an error. On x86, you should never see an entry for the I/O space
in "ranges", so we will not call this function unless there is a
bug in DT.

	Arnd

[PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Arnd Bergmann]

On Monday 10 March 2014 14:44:14 Liviu Dudau wrote:
> I will try to improve the error handling in the next patchset version.
> However I am still confused about the earlier discussion on
> pci_register_io_range(). Your suggestion initially was to return an
> error in the default weak implementation, but in your last email you
> are talking about returning 'port'.

You can do either one: 'port' should be positive or zero, while the
error would always be negative. We do the same thing in many interfaces
in the kernel.

> My idea when I've introduced the
> helper function was that it would return an error if it fails to
> register the IO range and zero otherwise. I agree that we can treat
> the default 'do nothing with the IO range' case as an error, with
> the caveat that will force architectures that use this code to
> provide their own implementation of pci_register_io_range() in order
> to avoid failure, even for the cases where the architecture has a 1:1
> mapping between IO and CPU addresses.

Which architectures are you thinking of? The only one I know that
does this is ia64, and we won't ever have to support this helper
on that architecture.

I did not ask to treat 'do nothing with the IO range' as an error,
what I meant is that we should treat 'architecture cannot translate
from I/O space to memory space but DT lists a translation anyway'
as an error. On x86, you should never see an entry for the I/O space
in "ranges", so we will not call this function unless there is a
bug in DT.

	Arnd

Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Liviu Dudau]

On Mon, Mar 10, 2014 at 03:21:01PM +0000, Arnd Bergmann wrote:
> On Monday 10 March 2014 14:44:14 Liviu Dudau wrote:
> > I will try to improve the error handling in the next patchset version.
> > However I am still confused about the earlier discussion on
> > pci_register_io_range(). Your suggestion initially was to return an
> > error in the default weak implementation, but in your last email you
> > are talking about returning 'port'.
> 
> You can do either one: 'port' should be positive or zero, while the
> error would always be negative. We do the same thing in many interfaces
> in the kernel.
> 
> > My idea when I've introduced the
> > helper function was that it would return an error if it fails to
> > register the IO range and zero otherwise. I agree that we can treat
> > the default 'do nothing with the IO range' case as an error, with
> > the caveat that will force architectures that use this code to
> > provide their own implementation of pci_register_io_range() in order
> > to avoid failure, even for the cases where the architecture has a 1:1
> > mapping between IO and CPU addresses.
> 
> Which architectures are you thinking of? The only one I know that
> does this is ia64, and we won't ever have to support this helper
> on that architecture.

I was thinking about architectures that have IO_SPACE_LIMIT >= 0xffffffff.
While not an absolute indicator, with the default pci_address_to_pio()
that means that they can use the CPU MMIO address as IO address directly.

$ git grep IO_SPACE_LIMIT | grep -i ffffffff
arch/arm/include/asm/io.h:#define IO_SPACE_LIMIT ((resource_size_t)0xffffffff)
arch/arm/mach-at91/include/mach/io.h:#define IO_SPACE_LIMIT		0xFFFFFFFF
arch/arm/mach-omap1/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
arch/arm/mach-pxa/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
arch/arm/mach-s3c24xx/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
arch/avr32/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff
arch/frv/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff
arch/ia64/include/asm/io.h:#define IO_SPACE_LIMIT		0xffffffffffffffffUL
arch/m32r/include/asm/io.h:#define IO_SPACE_LIMIT  0xFFFFFFFF
arch/m68k/include/asm/io_no.h:#define IO_SPACE_LIMIT 0xffffffff
arch/microblaze/include/asm/io.h:#define IO_SPACE_LIMIT (0xFFFFFFFF)
arch/mn10300/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff
arch/sh/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff
arch/sparc/include/asm/io_32.h:#define IO_SPACE_LIMIT 0xffffffff
arch/sparc/include/asm/io_64.h:#define IO_SPACE_LIMIT 0xffffffffffffffffUL
arch/tile/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff


> 
> I did not ask to treat 'do nothing with the IO range' as an error,
> what I meant is that we should treat 'architecture cannot translate
> from I/O space to memory space but DT lists a translation anyway'
> as an error. On x86, you should never see an entry for the I/O space
> in "ranges", so we will not call this function unless there is a
> bug in DT.

Ok, it's just that there is no "architecture cannot translate from I/O
space to memory space" indicator anywhere and I don't want to make x86
a special case.

So, my proposal is this: default weak implementation of pci_register_io_range()
returns an error (meaning I have no idea how to translate IO addresses
to memory space) and anyone that wants this function to return success will
have to provide their implementation.

I will send an updated series.

Best regards,
Liviu

> 
> 	Arnd
> 
> 

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

Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Liviu Dudau]

On Mon, Mar 10, 2014 at 03:21:01PM +0000, Arnd Bergmann wrote:
> On Monday 10 March 2014 14:44:14 Liviu Dudau wrote:
> > I will try to improve the error handling in the next patchset version.
> > However I am still confused about the earlier discussion on
> > pci_register_io_range(). Your suggestion initially was to return an
> > error in the default weak implementation, but in your last email you
> > are talking about returning 'port'.
> 
> You can do either one: 'port' should be positive or zero, while the
> error would always be negative. We do the same thing in many interfaces
> in the kernel.
> 
> > My idea when I've introduced the
> > helper function was that it would return an error if it fails to
> > register the IO range and zero otherwise. I agree that we can treat
> > the default 'do nothing with the IO range' case as an error, with
> > the caveat that will force architectures that use this code to
> > provide their own implementation of pci_register_io_range() in order
> > to avoid failure, even for the cases where the architecture has a 1:1
> > mapping between IO and CPU addresses.
> 
> Which architectures are you thinking of? The only one I know that
> does this is ia64, and we won't ever have to support this helper
> on that architecture.

I was thinking about architectures that have IO_SPACE_LIMIT >= 0xffffffff.
While not an absolute indicator, with the default pci_address_to_pio()
that means that they can use the CPU MMIO address as IO address directly.

$ git grep IO_SPACE_LIMIT | grep -i ffffffff
arch/arm/include/asm/io.h:#define IO_SPACE_LIMIT ((resource_size_t)0xffffffff)
arch/arm/mach-at91/include/mach/io.h:#define IO_SPACE_LIMIT		0xFFFFFFFF
arch/arm/mach-omap1/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
arch/arm/mach-pxa/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
arch/arm/mach-s3c24xx/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
arch/avr32/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff
arch/frv/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff
arch/ia64/include/asm/io.h:#define IO_SPACE_LIMIT		0xffffffffffffffffUL
arch/m32r/include/asm/io.h:#define IO_SPACE_LIMIT  0xFFFFFFFF
arch/m68k/include/asm/io_no.h:#define IO_SPACE_LIMIT 0xffffffff
arch/microblaze/include/asm/io.h:#define IO_SPACE_LIMIT (0xFFFFFFFF)
arch/mn10300/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff
arch/sh/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff
arch/sparc/include/asm/io_32.h:#define IO_SPACE_LIMIT 0xffffffff
arch/sparc/include/asm/io_64.h:#define IO_SPACE_LIMIT 0xffffffffffffffffUL
arch/tile/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff


> 
> I did not ask to treat 'do nothing with the IO range' as an error,
> what I meant is that we should treat 'architecture cannot translate
> from I/O space to memory space but DT lists a translation anyway'
> as an error. On x86, you should never see an entry for the I/O space
> in "ranges", so we will not call this function unless there is a
> bug in DT.

Ok, it's just that there is no "architecture cannot translate from I/O
space to memory space" indicator anywhere and I don't want to make x86
a special case.

So, my proposal is this: default weak implementation of pci_register_io_range()
returns an error (meaning I have no idea how to translate IO addresses
to memory space) and anyone that wants this function to return success will
have to provide their implementation.

I will send an updated series.

Best regards,
Liviu

> 
> 	Arnd
> 
> 

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

[PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Liviu Dudau]

On Mon, Mar 10, 2014 at 03:21:01PM +0000, Arnd Bergmann wrote:
> On Monday 10 March 2014 14:44:14 Liviu Dudau wrote:
> > I will try to improve the error handling in the next patchset version.
> > However I am still confused about the earlier discussion on
> > pci_register_io_range(). Your suggestion initially was to return an
> > error in the default weak implementation, but in your last email you
> > are talking about returning 'port'.
> 
> You can do either one: 'port' should be positive or zero, while the
> error would always be negative. We do the same thing in many interfaces
> in the kernel.
> 
> > My idea when I've introduced the
> > helper function was that it would return an error if it fails to
> > register the IO range and zero otherwise. I agree that we can treat
> > the default 'do nothing with the IO range' case as an error, with
> > the caveat that will force architectures that use this code to
> > provide their own implementation of pci_register_io_range() in order
> > to avoid failure, even for the cases where the architecture has a 1:1
> > mapping between IO and CPU addresses.
> 
> Which architectures are you thinking of? The only one I know that
> does this is ia64, and we won't ever have to support this helper
> on that architecture.

I was thinking about architectures that have IO_SPACE_LIMIT >= 0xffffffff.
While not an absolute indicator, with the default pci_address_to_pio()
that means that they can use the CPU MMIO address as IO address directly.

$ git grep IO_SPACE_LIMIT | grep -i ffffffff
arch/arm/include/asm/io.h:#define IO_SPACE_LIMIT ((resource_size_t)0xffffffff)
arch/arm/mach-at91/include/mach/io.h:#define IO_SPACE_LIMIT		0xFFFFFFFF
arch/arm/mach-omap1/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
arch/arm/mach-pxa/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
arch/arm/mach-s3c24xx/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
arch/avr32/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff
arch/frv/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff
arch/ia64/include/asm/io.h:#define IO_SPACE_LIMIT		0xffffffffffffffffUL
arch/m32r/include/asm/io.h:#define IO_SPACE_LIMIT  0xFFFFFFFF
arch/m68k/include/asm/io_no.h:#define IO_SPACE_LIMIT 0xffffffff
arch/microblaze/include/asm/io.h:#define IO_SPACE_LIMIT (0xFFFFFFFF)
arch/mn10300/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff
arch/sh/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff
arch/sparc/include/asm/io_32.h:#define IO_SPACE_LIMIT 0xffffffff
arch/sparc/include/asm/io_64.h:#define IO_SPACE_LIMIT 0xffffffffffffffffUL
arch/tile/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff


> 
> I did not ask to treat 'do nothing with the IO range' as an error,
> what I meant is that we should treat 'architecture cannot translate
> from I/O space to memory space but DT lists a translation anyway'
> as an error. On x86, you should never see an entry for the I/O space
> in "ranges", so we will not call this function unless there is a
> bug in DT.

Ok, it's just that there is no "architecture cannot translate from I/O
space to memory space" indicator anywhere and I don't want to make x86
a special case.

So, my proposal is this: default weak implementation of pci_register_io_range()
returns an error (meaning I have no idea how to translate IO addresses
to memory space) and anyone that wants this function to return success will
have to provide their implementation.

I will send an updated series.

Best regards,
Liviu

> 
> 	Arnd
> 
> 

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

Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Arnd Bergmann]

On Monday 10 March 2014 16:33:44 Liviu Dudau wrote:
> On Mon, Mar 10, 2014 at 03:21:01PM +0000, Arnd Bergmann wrote:
> > On Monday 10 March 2014 14:44:14 Liviu Dudau wrote:
> > > I will try to improve the error handling in the next patchset version.
> > > However I am still confused about the earlier discussion on
> > > pci_register_io_range(). Your suggestion initially was to return an
> > > error in the default weak implementation, but in your last email you
> > > are talking about returning 'port'.
> > 
> > You can do either one: 'port' should be positive or zero, while the
> > error would always be negative. We do the same thing in many interfaces
> > in the kernel.
> > 
> > > My idea when I've introduced the
> > > helper function was that it would return an error if it fails to
> > > register the IO range and zero otherwise. I agree that we can treat
> > > the default 'do nothing with the IO range' case as an error, with
> > > the caveat that will force architectures that use this code to
> > > provide their own implementation of pci_register_io_range() in order
> > > to avoid failure, even for the cases where the architecture has a 1:1
> > > mapping between IO and CPU addresses.
> > 
> > Which architectures are you thinking of? The only one I know that
> > does this is ia64, and we won't ever have to support this helper
> > on that architecture.
> 
> I was thinking about architectures that have IO_SPACE_LIMIT >= 0xffffffff.
> While not an absolute indicator, with the default pci_address_to_pio()
> that means that they can use the CPU MMIO address as IO address directly.

Not really, that would only work if they also have instructions to do
raw accesses to physical memory addresses rather than virtual memory
pointers that most architectures do.

> $ git grep IO_SPACE_LIMIT | grep -i ffffffff
> arch/arm/include/asm/io.h:#define IO_SPACE_LIMIT ((resource_size_t)0xffffffff)
> arch/arm/mach-at91/include/mach/io.h:#define IO_SPACE_LIMIT		0xFFFFFFFF
> arch/arm/mach-omap1/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
> arch/arm/mach-pxa/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
> arch/arm/mach-s3c24xx/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff

These use a special trick where an __iomem pointer is the same as
the port number. This works most of the time, but breaks anything
that assumes that port numbers are low, such as /dev/port or
broken devices. Moreover, it means your code won't work because
it depends on passing the virtual start address of the PIO mapping
window as io_offset.

> arch/avr32/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff
> arch/frv/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff

They have no MMU, and the code relies on the port number to match
both the virtual and the physical address. You could be right about
these, but I would guess that the code also needs some other
changes if we want to make it work on nommu kernels. It also depends
on whether the I/O bus address is the same as the CPU address, or
if it starts at bus address 0.

> arch/ia64/include/asm/io.h:#define IO_SPACE_LIMIT		0xffffffffffffffffUL

Here, the definition is special, the token is just used to encode
a space number and an offset within the I/O space.

> arch/m32r/include/asm/io.h:#define IO_SPACE_LIMIT  0xFFFFFFFF

no PCI here.

> arch/m68k/include/asm/io_no.h:#define IO_SPACE_LIMIT 0xffffffff

This looks like a mistake, it should be smaller

> arch/microblaze/include/asm/io.h:#define IO_SPACE_LIMIT (0xFFFFFFFF)

I suspect it doesn't actually work. microblaze copied large parts
of this from PowerPC, but the parts that differ apparently get
it wrong for the I/O space. 

> arch/mn10300/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff

Same category as frv. We should ask David Howells whether he
thinks I/O space actually works on these.

> arch/sh/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff

I think this should just be 0xffff.

> arch/sparc/include/asm/io_32.h:#define IO_SPACE_LIMIT 0xffffffff
> arch/sparc/include/asm/io_64.h:#define IO_SPACE_LIMIT 0xffffffffffffffffUL

Sparc actually accesses the physical addresses, so in theory
it could always work. In the 64-bit case it would however have
to check that the port number is smaller than 0xffffffff, otherwise
you couldn't set the BAR. This means you still need a custom
function.

> arch/tile/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff

tile seems to support only ioport_map() but not inb/outb, if I'm
reading this right.

> > I did not ask to treat 'do nothing with the IO range' as an error,
> > what I meant is that we should treat 'architecture cannot translate
> > from I/O space to memory space but DT lists a translation anyway'
> > as an error. On x86, you should never see an entry for the I/O space
> > in "ranges", so we will not call this function unless there is a
> > bug in DT.
> 
> Ok, it's just that there is no "architecture cannot translate from I/O
> space to memory space" indicator anywhere and I don't want to make x86
> a special case.

Right.

> So, my proposal is this: default weak implementation of pci_register_io_range()
> returns an error (meaning I have no idea how to translate IO addresses
> to memory space) and anyone that wants this function to return success will
> have to provide their implementation.

Another idea: make this conditional on the definition of PCI_IOBASE: If this
is defined, we can use the arm64 version that uses this number. Otherwise
we fall back to returning an error, which means that either on the
architecture we shouldn't be calling that function, or we need a custom
implementation.

	Arnd

Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Arnd Bergmann]

On Monday 10 March 2014 16:33:44 Liviu Dudau wrote:
> On Mon, Mar 10, 2014 at 03:21:01PM +0000, Arnd Bergmann wrote:
> > On Monday 10 March 2014 14:44:14 Liviu Dudau wrote:
> > > I will try to improve the error handling in the next patchset version.
> > > However I am still confused about the earlier discussion on
> > > pci_register_io_range(). Your suggestion initially was to return an
> > > error in the default weak implementation, but in your last email you
> > > are talking about returning 'port'.
> > 
> > You can do either one: 'port' should be positive or zero, while the
> > error would always be negative. We do the same thing in many interfaces
> > in the kernel.
> > 
> > > My idea when I've introduced the
> > > helper function was that it would return an error if it fails to
> > > register the IO range and zero otherwise. I agree that we can treat
> > > the default 'do nothing with the IO range' case as an error, with
> > > the caveat that will force architectures that use this code to
> > > provide their own implementation of pci_register_io_range() in order
> > > to avoid failure, even for the cases where the architecture has a 1:1
> > > mapping between IO and CPU addresses.
> > 
> > Which architectures are you thinking of? The only one I know that
> > does this is ia64, and we won't ever have to support this helper
> > on that architecture.
> 
> I was thinking about architectures that have IO_SPACE_LIMIT >= 0xffffffff.
> While not an absolute indicator, with the default pci_address_to_pio()
> that means that they can use the CPU MMIO address as IO address directly.

Not really, that would only work if they also have instructions to do
raw accesses to physical memory addresses rather than virtual memory
pointers that most architectures do.

> $ git grep IO_SPACE_LIMIT | grep -i ffffffff
> arch/arm/include/asm/io.h:#define IO_SPACE_LIMIT ((resource_size_t)0xffffffff)
> arch/arm/mach-at91/include/mach/io.h:#define IO_SPACE_LIMIT		0xFFFFFFFF
> arch/arm/mach-omap1/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
> arch/arm/mach-pxa/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
> arch/arm/mach-s3c24xx/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff

These use a special trick where an __iomem pointer is the same as
the port number. This works most of the time, but breaks anything
that assumes that port numbers are low, such as /dev/port or
broken devices. Moreover, it means your code won't work because
it depends on passing the virtual start address of the PIO mapping
window as io_offset.

> arch/avr32/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff
> arch/frv/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff

They have no MMU, and the code relies on the port number to match
both the virtual and the physical address. You could be right about
these, but I would guess that the code also needs some other
changes if we want to make it work on nommu kernels. It also depends
on whether the I/O bus address is the same as the CPU address, or
if it starts at bus address 0.

> arch/ia64/include/asm/io.h:#define IO_SPACE_LIMIT		0xffffffffffffffffUL

Here, the definition is special, the token is just used to encode
a space number and an offset within the I/O space.

> arch/m32r/include/asm/io.h:#define IO_SPACE_LIMIT  0xFFFFFFFF

no PCI here.

> arch/m68k/include/asm/io_no.h:#define IO_SPACE_LIMIT 0xffffffff

This looks like a mistake, it should be smaller

> arch/microblaze/include/asm/io.h:#define IO_SPACE_LIMIT (0xFFFFFFFF)

I suspect it doesn't actually work. microblaze copied large parts
of this from PowerPC, but the parts that differ apparently get
it wrong for the I/O space. 

> arch/mn10300/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff

Same category as frv. We should ask David Howells whether he
thinks I/O space actually works on these.

> arch/sh/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff

I think this should just be 0xffff.

> arch/sparc/include/asm/io_32.h:#define IO_SPACE_LIMIT 0xffffffff
> arch/sparc/include/asm/io_64.h:#define IO_SPACE_LIMIT 0xffffffffffffffffUL

Sparc actually accesses the physical addresses, so in theory
it could always work. In the 64-bit case it would however have
to check that the port number is smaller than 0xffffffff, otherwise
you couldn't set the BAR. This means you still need a custom
function.

> arch/tile/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff

tile seems to support only ioport_map() but not inb/outb, if I'm
reading this right.

> > I did not ask to treat 'do nothing with the IO range' as an error,
> > what I meant is that we should treat 'architecture cannot translate
> > from I/O space to memory space but DT lists a translation anyway'
> > as an error. On x86, you should never see an entry for the I/O space
> > in "ranges", so we will not call this function unless there is a
> > bug in DT.
> 
> Ok, it's just that there is no "architecture cannot translate from I/O
> space to memory space" indicator anywhere and I don't want to make x86
> a special case.

Right.

> So, my proposal is this: default weak implementation of pci_register_io_range()
> returns an error (meaning I have no idea how to translate IO addresses
> to memory space) and anyone that wants this function to return success will
> have to provide their implementation.

Another idea: make this conditional on the definition of PCI_IOBASE: If this
is defined, we can use the arm64 version that uses this number. Otherwise
we fall back to returning an error, which means that either on the
architecture we shouldn't be calling that function, or we need a custom
implementation.

	Arnd

[PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Arnd Bergmann]

On Monday 10 March 2014 16:33:44 Liviu Dudau wrote:
> On Mon, Mar 10, 2014 at 03:21:01PM +0000, Arnd Bergmann wrote:
> > On Monday 10 March 2014 14:44:14 Liviu Dudau wrote:
> > > I will try to improve the error handling in the next patchset version.
> > > However I am still confused about the earlier discussion on
> > > pci_register_io_range(). Your suggestion initially was to return an
> > > error in the default weak implementation, but in your last email you
> > > are talking about returning 'port'.
> > 
> > You can do either one: 'port' should be positive or zero, while the
> > error would always be negative. We do the same thing in many interfaces
> > in the kernel.
> > 
> > > My idea when I've introduced the
> > > helper function was that it would return an error if it fails to
> > > register the IO range and zero otherwise. I agree that we can treat
> > > the default 'do nothing with the IO range' case as an error, with
> > > the caveat that will force architectures that use this code to
> > > provide their own implementation of pci_register_io_range() in order
> > > to avoid failure, even for the cases where the architecture has a 1:1
> > > mapping between IO and CPU addresses.
> > 
> > Which architectures are you thinking of? The only one I know that
> > does this is ia64, and we won't ever have to support this helper
> > on that architecture.
> 
> I was thinking about architectures that have IO_SPACE_LIMIT >= 0xffffffff.
> While not an absolute indicator, with the default pci_address_to_pio()
> that means that they can use the CPU MMIO address as IO address directly.

Not really, that would only work if they also have instructions to do
raw accesses to physical memory addresses rather than virtual memory
pointers that most architectures do.

> $ git grep IO_SPACE_LIMIT | grep -i ffffffff
> arch/arm/include/asm/io.h:#define IO_SPACE_LIMIT ((resource_size_t)0xffffffff)
> arch/arm/mach-at91/include/mach/io.h:#define IO_SPACE_LIMIT		0xFFFFFFFF
> arch/arm/mach-omap1/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
> arch/arm/mach-pxa/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
> arch/arm/mach-s3c24xx/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff

These use a special trick where an __iomem pointer is the same as
the port number. This works most of the time, but breaks anything
that assumes that port numbers are low, such as /dev/port or
broken devices. Moreover, it means your code won't work because
it depends on passing the virtual start address of the PIO mapping
window as io_offset.

> arch/avr32/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff
> arch/frv/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff

They have no MMU, and the code relies on the port number to match
both the virtual and the physical address. You could be right about
these, but I would guess that the code also needs some other
changes if we want to make it work on nommu kernels. It also depends
on whether the I/O bus address is the same as the CPU address, or
if it starts at bus address 0.

> arch/ia64/include/asm/io.h:#define IO_SPACE_LIMIT		0xffffffffffffffffUL

Here, the definition is special, the token is just used to encode
a space number and an offset within the I/O space.

> arch/m32r/include/asm/io.h:#define IO_SPACE_LIMIT  0xFFFFFFFF

no PCI here.

> arch/m68k/include/asm/io_no.h:#define IO_SPACE_LIMIT 0xffffffff

This looks like a mistake, it should be smaller

> arch/microblaze/include/asm/io.h:#define IO_SPACE_LIMIT (0xFFFFFFFF)

I suspect it doesn't actually work. microblaze copied large parts
of this from PowerPC, but the parts that differ apparently get
it wrong for the I/O space. 

> arch/mn10300/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff

Same category as frv. We should ask David Howells whether he
thinks I/O space actually works on these.

> arch/sh/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff

I think this should just be 0xffff.

> arch/sparc/include/asm/io_32.h:#define IO_SPACE_LIMIT 0xffffffff
> arch/sparc/include/asm/io_64.h:#define IO_SPACE_LIMIT 0xffffffffffffffffUL

Sparc actually accesses the physical addresses, so in theory
it could always work. In the 64-bit case it would however have
to check that the port number is smaller than 0xffffffff, otherwise
you couldn't set the BAR. This means you still need a custom
function.

> arch/tile/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff

tile seems to support only ioport_map() but not inb/outb, if I'm
reading this right.

> > I did not ask to treat 'do nothing with the IO range' as an error,
> > what I meant is that we should treat 'architecture cannot translate
> > from I/O space to memory space but DT lists a translation anyway'
> > as an error. On x86, you should never see an entry for the I/O space
> > in "ranges", so we will not call this function unless there is a
> > bug in DT.
> 
> Ok, it's just that there is no "architecture cannot translate from I/O
> space to memory space" indicator anywhere and I don't want to make x86
> a special case.

Right.

> So, my proposal is this: default weak implementation of pci_register_io_range()
> returns an error (meaning I have no idea how to translate IO addresses
> to memory space) and anyone that wants this function to return success will
> have to provide their implementation.

Another idea: make this conditional on the definition of PCI_IOBASE: If this
is defined, we can use the arm64 version that uses this number. Otherwise
we fall back to returning an error, which means that either on the
architecture we shouldn't be calling that function, or we need a custom
implementation.

	Arnd

Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Geert Uytterhoeven]

On Mon, Mar 10, 2014 at 7:59 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> arch/avr32/include/asm/io.h:#define IO_SPACE_LIMIT    0xffffffff
>> arch/frv/include/asm/io.h:#define IO_SPACE_LIMIT      0xffffffff
>
> They have no MMU, and the code relies on the port number to match
> both the virtual and the physical address. You could be right about
> these, but I would guess that the code also needs some other
> changes if we want to make it work on nommu kernels. It also depends
> on whether the I/O bus address is the same as the CPU address, or
> if it starts at bus address 0.

>> arch/m68k/include/asm/io_no.h:#define IO_SPACE_LIMIT 0xffffffff
>
> This looks like a mistake, it should be smaller

io_no.h is for nommu.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Geert Uytterhoeven]

On Mon, Mar 10, 2014 at 7:59 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> arch/avr32/include/asm/io.h:#define IO_SPACE_LIMIT    0xffffffff
>> arch/frv/include/asm/io.h:#define IO_SPACE_LIMIT      0xffffffff
>
> They have no MMU, and the code relies on the port number to match
> both the virtual and the physical address. You could be right about
> these, but I would guess that the code also needs some other
> changes if we want to make it work on nommu kernels. It also depends
> on whether the I/O bus address is the same as the CPU address, or
> if it starts at bus address 0.

>> arch/m68k/include/asm/io_no.h:#define IO_SPACE_LIMIT 0xffffffff
>
> This looks like a mistake, it should be smaller

io_no.h is for nommu.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Geert Uytterhoeven]

On Mon, Mar 10, 2014 at 7:59 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> arch/avr32/include/asm/io.h:#define IO_SPACE_LIMIT    0xffffffff
>> arch/frv/include/asm/io.h:#define IO_SPACE_LIMIT      0xffffffff
>
> They have no MMU, and the code relies on the port number to match
> both the virtual and the physical address. You could be right about
> these, but I would guess that the code also needs some other
> changes if we want to make it work on nommu kernels. It also depends
> on whether the I/O bus address is the same as the CPU address, or
> if it starts at bus address 0.

>> arch/m68k/include/asm/io_no.h:#define IO_SPACE_LIMIT 0xffffffff
>
> This looks like a mistake, it should be smaller

io_no.h is for nommu.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Arnd Bergmann]

On Monday 10 March 2014 20:16:25 Geert Uytterhoeven wrote:
> On Mon, Mar 10, 2014 at 7:59 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> arch/avr32/include/asm/io.h:#define IO_SPACE_LIMIT    0xffffffff
> >> arch/frv/include/asm/io.h:#define IO_SPACE_LIMIT      0xffffffff
> >
> > They have no MMU, and the code relies on the port number to match
> > both the virtual and the physical address. You could be right about
> > these, but I would guess that the code also needs some other
> > changes if we want to make it work on nommu kernels. It also depends
> > on whether the I/O bus address is the same as the CPU address, or
> > if it starts at bus address 0.
> 
> >> arch/m68k/include/asm/io_no.h:#define IO_SPACE_LIMIT 0xffffffff
> >
> > This looks like a mistake, it should be smaller
> 
> io_no.h is for nommu.

Ah, I missed that. In that case I assume it doesn't matter because
the only m68k with PCI is M54xx and that always has an MMU.

	Arnd

Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Arnd Bergmann]

On Monday 10 March 2014 20:16:25 Geert Uytterhoeven wrote:
> On Mon, Mar 10, 2014 at 7:59 PM, Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org> wrote:
> >> arch/avr32/include/asm/io.h:#define IO_SPACE_LIMIT    0xffffffff
> >> arch/frv/include/asm/io.h:#define IO_SPACE_LIMIT      0xffffffff
> >
> > They have no MMU, and the code relies on the port number to match
> > both the virtual and the physical address. You could be right about
> > these, but I would guess that the code also needs some other
> > changes if we want to make it work on nommu kernels. It also depends
> > on whether the I/O bus address is the same as the CPU address, or
> > if it starts at bus address 0.
> 
> >> arch/m68k/include/asm/io_no.h:#define IO_SPACE_LIMIT 0xffffffff
> >
> > This looks like a mistake, it should be smaller
> 
> io_no.h is for nommu.

Ah, I missed that. In that case I assume it doesn't matter because
the only m68k with PCI is M54xx and that always has an MMU.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Arnd Bergmann]

On Monday 10 March 2014 20:16:25 Geert Uytterhoeven wrote:
> On Mon, Mar 10, 2014 at 7:59 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> arch/avr32/include/asm/io.h:#define IO_SPACE_LIMIT    0xffffffff
> >> arch/frv/include/asm/io.h:#define IO_SPACE_LIMIT      0xffffffff
> >
> > They have no MMU, and the code relies on the port number to match
> > both the virtual and the physical address. You could be right about
> > these, but I would guess that the code also needs some other
> > changes if we want to make it work on nommu kernels. It also depends
> > on whether the I/O bus address is the same as the CPU address, or
> > if it starts at bus address 0.
> 
> >> arch/m68k/include/asm/io_no.h:#define IO_SPACE_LIMIT 0xffffffff
> >
> > This looks like a mistake, it should be smaller
> 
> io_no.h is for nommu.

Ah, I missed that. In that case I assume it doesn't matter because
the only m68k with PCI is M54xx and that always has an MMU.

	Arnd

Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Liviu Dudau]

On Mon, Mar 10, 2014 at 07:59:59PM +0100, Arnd Bergmann wrote:
> On Monday 10 March 2014 16:33:44 Liviu Dudau wrote:
> > On Mon, Mar 10, 2014 at 03:21:01PM +0000, Arnd Bergmann wrote:
> > > On Monday 10 March 2014 14:44:14 Liviu Dudau wrote:
> > > > I will try to improve the error handling in the next patchset version.
> > > > However I am still confused about the earlier discussion on
> > > > pci_register_io_range(). Your suggestion initially was to return an
> > > > error in the default weak implementation, but in your last email you
> > > > are talking about returning 'port'.
> > > 
> > > You can do either one: 'port' should be positive or zero, while the
> > > error would always be negative. We do the same thing in many interfaces
> > > in the kernel.
> > > 
> > > > My idea when I've introduced the
> > > > helper function was that it would return an error if it fails to
> > > > register the IO range and zero otherwise. I agree that we can treat
> > > > the default 'do nothing with the IO range' case as an error, with
> > > > the caveat that will force architectures that use this code to
> > > > provide their own implementation of pci_register_io_range() in order
> > > > to avoid failure, even for the cases where the architecture has a 1:1
> > > > mapping between IO and CPU addresses.
> > > 
> > > Which architectures are you thinking of? The only one I know that
> > > does this is ia64, and we won't ever have to support this helper
> > > on that architecture.
> > 
> > I was thinking about architectures that have IO_SPACE_LIMIT >= 0xffffffff.
> > While not an absolute indicator, with the default pci_address_to_pio()
> > that means that they can use the CPU MMIO address as IO address directly.
> 
> Not really, that would only work if they also have instructions to do
> raw accesses to physical memory addresses rather than virtual memory
> pointers that most architectures do.
> 
> > $ git grep IO_SPACE_LIMIT | grep -i ffffffff
> > arch/arm/include/asm/io.h:#define IO_SPACE_LIMIT ((resource_size_t)0xffffffff)
> > arch/arm/mach-at91/include/mach/io.h:#define IO_SPACE_LIMIT		0xFFFFFFFF
> > arch/arm/mach-omap1/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
> > arch/arm/mach-pxa/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
> > arch/arm/mach-s3c24xx/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
> 
> These use a special trick where an __iomem pointer is the same as
> the port number. This works most of the time, but breaks anything
> that assumes that port numbers are low, such as /dev/port or
> broken devices. Moreover, it means your code won't work because
> it depends on passing the virtual start address of the PIO mapping
> window as io_offset.
> 
> > arch/avr32/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff
> > arch/frv/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff
> 
> They have no MMU, and the code relies on the port number to match
> both the virtual and the physical address. You could be right about
> these, but I would guess that the code also needs some other
> changes if we want to make it work on nommu kernels. It also depends
> on whether the I/O bus address is the same as the CPU address, or
> if it starts at bus address 0.
> 
> > arch/ia64/include/asm/io.h:#define IO_SPACE_LIMIT		0xffffffffffffffffUL
> 
> Here, the definition is special, the token is just used to encode
> a space number and an offset within the I/O space.
> 
> > arch/m32r/include/asm/io.h:#define IO_SPACE_LIMIT  0xFFFFFFFF
> 
> no PCI here.
> 
> > arch/m68k/include/asm/io_no.h:#define IO_SPACE_LIMIT 0xffffffff
> 
> This looks like a mistake, it should be smaller
> 
> > arch/microblaze/include/asm/io.h:#define IO_SPACE_LIMIT (0xFFFFFFFF)
> 
> I suspect it doesn't actually work. microblaze copied large parts
> of this from PowerPC, but the parts that differ apparently get
> it wrong for the I/O space. 
> 
> > arch/mn10300/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff
> 
> Same category as frv. We should ask David Howells whether he
> thinks I/O space actually works on these.
> 
> > arch/sh/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff
> 
> I think this should just be 0xffff.
> 
> > arch/sparc/include/asm/io_32.h:#define IO_SPACE_LIMIT 0xffffffff
> > arch/sparc/include/asm/io_64.h:#define IO_SPACE_LIMIT 0xffffffffffffffffUL
> 
> Sparc actually accesses the physical addresses, so in theory
> it could always work. In the 64-bit case it would however have
> to check that the port number is smaller than 0xffffffff, otherwise
> you couldn't set the BAR. This means you still need a custom
> function.
> 
> > arch/tile/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff
> 
> tile seems to support only ioport_map() but not inb/outb, if I'm
> reading this right.
> 
> > > I did not ask to treat 'do nothing with the IO range' as an error,
> > > what I meant is that we should treat 'architecture cannot translate
> > > from I/O space to memory space but DT lists a translation anyway'
> > > as an error. On x86, you should never see an entry for the I/O space
> > > in "ranges", so we will not call this function unless there is a
> > > bug in DT.
> > 
> > Ok, it's just that there is no "architecture cannot translate from I/O
> > space to memory space" indicator anywhere and I don't want to make x86
> > a special case.
> 
> Right.
> 
> > So, my proposal is this: default weak implementation of pci_register_io_range()
> > returns an error (meaning I have no idea how to translate IO addresses
> > to memory space) and anyone that wants this function to return success will
> > have to provide their implementation.
> 
> Another idea: make this conditional on the definition of PCI_IOBASE: If this
> is defined, we can use the arm64 version that uses this number. Otherwise
> we fall back to returning an error, which means that either on the
> architecture we shouldn't be calling that function, or we need a custom
> implementation.

PCI_IOBASE is always defined. See the discussion with Russell on this subject.

include/asm-generic/io.h has at line 118:

#ifndef PCI_IOBASE
#define PCI_IOBASE ((void __iomem *) 0)
#endif

I will go with my idea tomorrow. arm64 overwrite the implementation anyway, I
find it cleaner rather than having to do #ifdefs and/or ifs.

Best regards,
Liviu

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

Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Liviu Dudau]

On Mon, Mar 10, 2014 at 07:59:59PM +0100, Arnd Bergmann wrote:
> On Monday 10 March 2014 16:33:44 Liviu Dudau wrote:
> > On Mon, Mar 10, 2014 at 03:21:01PM +0000, Arnd Bergmann wrote:
> > > On Monday 10 March 2014 14:44:14 Liviu Dudau wrote:
> > > > I will try to improve the error handling in the next patchset version.
> > > > However I am still confused about the earlier discussion on
> > > > pci_register_io_range(). Your suggestion initially was to return an
> > > > error in the default weak implementation, but in your last email you
> > > > are talking about returning 'port'.
> > > 
> > > You can do either one: 'port' should be positive or zero, while the
> > > error would always be negative. We do the same thing in many interfaces
> > > in the kernel.
> > > 
> > > > My idea when I've introduced the
> > > > helper function was that it would return an error if it fails to
> > > > register the IO range and zero otherwise. I agree that we can treat
> > > > the default 'do nothing with the IO range' case as an error, with
> > > > the caveat that will force architectures that use this code to
> > > > provide their own implementation of pci_register_io_range() in order
> > > > to avoid failure, even for the cases where the architecture has a 1:1
> > > > mapping between IO and CPU addresses.
> > > 
> > > Which architectures are you thinking of? The only one I know that
> > > does this is ia64, and we won't ever have to support this helper
> > > on that architecture.
> > 
> > I was thinking about architectures that have IO_SPACE_LIMIT >= 0xffffffff.
> > While not an absolute indicator, with the default pci_address_to_pio()
> > that means that they can use the CPU MMIO address as IO address directly.
> 
> Not really, that would only work if they also have instructions to do
> raw accesses to physical memory addresses rather than virtual memory
> pointers that most architectures do.
> 
> > $ git grep IO_SPACE_LIMIT | grep -i ffffffff
> > arch/arm/include/asm/io.h:#define IO_SPACE_LIMIT ((resource_size_t)0xffffffff)
> > arch/arm/mach-at91/include/mach/io.h:#define IO_SPACE_LIMIT		0xFFFFFFFF
> > arch/arm/mach-omap1/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
> > arch/arm/mach-pxa/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
> > arch/arm/mach-s3c24xx/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
> 
> These use a special trick where an __iomem pointer is the same as
> the port number. This works most of the time, but breaks anything
> that assumes that port numbers are low, such as /dev/port or
> broken devices. Moreover, it means your code won't work because
> it depends on passing the virtual start address of the PIO mapping
> window as io_offset.
> 
> > arch/avr32/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff
> > arch/frv/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff
> 
> They have no MMU, and the code relies on the port number to match
> both the virtual and the physical address. You could be right about
> these, but I would guess that the code also needs some other
> changes if we want to make it work on nommu kernels. It also depends
> on whether the I/O bus address is the same as the CPU address, or
> if it starts at bus address 0.
> 
> > arch/ia64/include/asm/io.h:#define IO_SPACE_LIMIT		0xffffffffffffffffUL
> 
> Here, the definition is special, the token is just used to encode
> a space number and an offset within the I/O space.
> 
> > arch/m32r/include/asm/io.h:#define IO_SPACE_LIMIT  0xFFFFFFFF
> 
> no PCI here.
> 
> > arch/m68k/include/asm/io_no.h:#define IO_SPACE_LIMIT 0xffffffff
> 
> This looks like a mistake, it should be smaller
> 
> > arch/microblaze/include/asm/io.h:#define IO_SPACE_LIMIT (0xFFFFFFFF)
> 
> I suspect it doesn't actually work. microblaze copied large parts
> of this from PowerPC, but the parts that differ apparently get
> it wrong for the I/O space. 
> 
> > arch/mn10300/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff
> 
> Same category as frv. We should ask David Howells whether he
> thinks I/O space actually works on these.
> 
> > arch/sh/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff
> 
> I think this should just be 0xffff.
> 
> > arch/sparc/include/asm/io_32.h:#define IO_SPACE_LIMIT 0xffffffff
> > arch/sparc/include/asm/io_64.h:#define IO_SPACE_LIMIT 0xffffffffffffffffUL
> 
> Sparc actually accesses the physical addresses, so in theory
> it could always work. In the 64-bit case it would however have
> to check that the port number is smaller than 0xffffffff, otherwise
> you couldn't set the BAR. This means you still need a custom
> function.
> 
> > arch/tile/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff
> 
> tile seems to support only ioport_map() but not inb/outb, if I'm
> reading this right.
> 
> > > I did not ask to treat 'do nothing with the IO range' as an error,
> > > what I meant is that we should treat 'architecture cannot translate
> > > from I/O space to memory space but DT lists a translation anyway'
> > > as an error. On x86, you should never see an entry for the I/O space
> > > in "ranges", so we will not call this function unless there is a
> > > bug in DT.
> > 
> > Ok, it's just that there is no "architecture cannot translate from I/O
> > space to memory space" indicator anywhere and I don't want to make x86
> > a special case.
> 
> Right.
> 
> > So, my proposal is this: default weak implementation of pci_register_io_range()
> > returns an error (meaning I have no idea how to translate IO addresses
> > to memory space) and anyone that wants this function to return success will
> > have to provide their implementation.
> 
> Another idea: make this conditional on the definition of PCI_IOBASE: If this
> is defined, we can use the arm64 version that uses this number. Otherwise
> we fall back to returning an error, which means that either on the
> architecture we shouldn't be calling that function, or we need a custom
> implementation.

PCI_IOBASE is always defined. See the discussion with Russell on this subject.

include/asm-generic/io.h has at line 118:

#ifndef PCI_IOBASE
#define PCI_IOBASE ((void __iomem *) 0)
#endif

I will go with my idea tomorrow. arm64 overwrite the implementation anyway, I
find it cleaner rather than having to do #ifdefs and/or ifs.

Best regards,
Liviu

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

Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Liviu Dudau]

On Mon, Mar 10, 2014 at 07:59:59PM +0100, Arnd Bergmann wrote:
> On Monday 10 March 2014 16:33:44 Liviu Dudau wrote:
> > On Mon, Mar 10, 2014 at 03:21:01PM +0000, Arnd Bergmann wrote:
> > > On Monday 10 March 2014 14:44:14 Liviu Dudau wrote:
> > > > I will try to improve the error handling in the next patchset version.
> > > > However I am still confused about the earlier discussion on
> > > > pci_register_io_range(). Your suggestion initially was to return an
> > > > error in the default weak implementation, but in your last email you
> > > > are talking about returning 'port'.
> > > 
> > > You can do either one: 'port' should be positive or zero, while the
> > > error would always be negative. We do the same thing in many interfaces
> > > in the kernel.
> > > 
> > > > My idea when I've introduced the
> > > > helper function was that it would return an error if it fails to
> > > > register the IO range and zero otherwise. I agree that we can treat
> > > > the default 'do nothing with the IO range' case as an error, with
> > > > the caveat that will force architectures that use this code to
> > > > provide their own implementation of pci_register_io_range() in order
> > > > to avoid failure, even for the cases where the architecture has a 1:1
> > > > mapping between IO and CPU addresses.
> > > 
> > > Which architectures are you thinking of? The only one I know that
> > > does this is ia64, and we won't ever have to support this helper
> > > on that architecture.
> > 
> > I was thinking about architectures that have IO_SPACE_LIMIT >= 0xffffffff.
> > While not an absolute indicator, with the default pci_address_to_pio()
> > that means that they can use the CPU MMIO address as IO address directly.
> 
> Not really, that would only work if they also have instructions to do
> raw accesses to physical memory addresses rather than virtual memory
> pointers that most architectures do.
> 
> > $ git grep IO_SPACE_LIMIT | grep -i ffffffff
> > arch/arm/include/asm/io.h:#define IO_SPACE_LIMIT ((resource_size_t)0xffffffff)
> > arch/arm/mach-at91/include/mach/io.h:#define IO_SPACE_LIMIT		0xFFFFFFFF
> > arch/arm/mach-omap1/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
> > arch/arm/mach-pxa/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
> > arch/arm/mach-s3c24xx/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
> 
> These use a special trick where an __iomem pointer is the same as
> the port number. This works most of the time, but breaks anything
> that assumes that port numbers are low, such as /dev/port or
> broken devices. Moreover, it means your code won't work because
> it depends on passing the virtual start address of the PIO mapping
> window as io_offset.
> 
> > arch/avr32/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff
> > arch/frv/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff
> 
> They have no MMU, and the code relies on the port number to match
> both the virtual and the physical address. You could be right about
> these, but I would guess that the code also needs some other
> changes if we want to make it work on nommu kernels. It also depends
> on whether the I/O bus address is the same as the CPU address, or
> if it starts at bus address 0.
> 
> > arch/ia64/include/asm/io.h:#define IO_SPACE_LIMIT		0xffffffffffffffffUL
> 
> Here, the definition is special, the token is just used to encode
> a space number and an offset within the I/O space.
> 
> > arch/m32r/include/asm/io.h:#define IO_SPACE_LIMIT  0xFFFFFFFF
> 
> no PCI here.
> 
> > arch/m68k/include/asm/io_no.h:#define IO_SPACE_LIMIT 0xffffffff
> 
> This looks like a mistake, it should be smaller
> 
> > arch/microblaze/include/asm/io.h:#define IO_SPACE_LIMIT (0xFFFFFFFF)
> 
> I suspect it doesn't actually work. microblaze copied large parts
> of this from PowerPC, but the parts that differ apparently get
> it wrong for the I/O space. 
> 
> > arch/mn10300/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff
> 
> Same category as frv. We should ask David Howells whether he
> thinks I/O space actually works on these.
> 
> > arch/sh/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff
> 
> I think this should just be 0xffff.
> 
> > arch/sparc/include/asm/io_32.h:#define IO_SPACE_LIMIT 0xffffffff
> > arch/sparc/include/asm/io_64.h:#define IO_SPACE_LIMIT 0xffffffffffffffffUL
> 
> Sparc actually accesses the physical addresses, so in theory
> it could always work. In the 64-bit case it would however have
> to check that the port number is smaller than 0xffffffff, otherwise
> you couldn't set the BAR. This means you still need a custom
> function.
> 
> > arch/tile/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff
> 
> tile seems to support only ioport_map() but not inb/outb, if I'm
> reading this right.
> 
> > > I did not ask to treat 'do nothing with the IO range' as an error,
> > > what I meant is that we should treat 'architecture cannot translate
> > > from I/O space to memory space but DT lists a translation anyway'
> > > as an error. On x86, you should never see an entry for the I/O space
> > > in "ranges", so we will not call this function unless there is a
> > > bug in DT.
> > 
> > Ok, it's just that there is no "architecture cannot translate from I/O
> > space to memory space" indicator anywhere and I don't want to make x86
> > a special case.
> 
> Right.
> 
> > So, my proposal is this: default weak implementation of pci_register_io_range()
> > returns an error (meaning I have no idea how to translate IO addresses
> > to memory space) and anyone that wants this function to return success will
> > have to provide their implementation.
> 
> Another idea: make this conditional on the definition of PCI_IOBASE: If this
> is defined, we can use the arm64 version that uses this number. Otherwise
> we fall back to returning an error, which means that either on the
> architecture we shouldn't be calling that function, or we need a custom
> implementation.

PCI_IOBASE is always defined. See the discussion with Russell on this subject.

include/asm-generic/io.h has at line 118:

#ifndef PCI_IOBASE
#define PCI_IOBASE ((void __iomem *) 0)
#endif

I will go with my idea tomorrow. arm64 overwrite the implementation anyway, I
find it cleaner rather than having to do #ifdefs and/or ifs.

Best regards,
Liviu

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

[PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Liviu Dudau]

On Mon, Mar 10, 2014 at 07:59:59PM +0100, Arnd Bergmann wrote:
> On Monday 10 March 2014 16:33:44 Liviu Dudau wrote:
> > On Mon, Mar 10, 2014 at 03:21:01PM +0000, Arnd Bergmann wrote:
> > > On Monday 10 March 2014 14:44:14 Liviu Dudau wrote:
> > > > I will try to improve the error handling in the next patchset version.
> > > > However I am still confused about the earlier discussion on
> > > > pci_register_io_range(). Your suggestion initially was to return an
> > > > error in the default weak implementation, but in your last email you
> > > > are talking about returning 'port'.
> > > 
> > > You can do either one: 'port' should be positive or zero, while the
> > > error would always be negative. We do the same thing in many interfaces
> > > in the kernel.
> > > 
> > > > My idea when I've introduced the
> > > > helper function was that it would return an error if it fails to
> > > > register the IO range and zero otherwise. I agree that we can treat
> > > > the default 'do nothing with the IO range' case as an error, with
> > > > the caveat that will force architectures that use this code to
> > > > provide their own implementation of pci_register_io_range() in order
> > > > to avoid failure, even for the cases where the architecture has a 1:1
> > > > mapping between IO and CPU addresses.
> > > 
> > > Which architectures are you thinking of? The only one I know that
> > > does this is ia64, and we won't ever have to support this helper
> > > on that architecture.
> > 
> > I was thinking about architectures that have IO_SPACE_LIMIT >= 0xffffffff.
> > While not an absolute indicator, with the default pci_address_to_pio()
> > that means that they can use the CPU MMIO address as IO address directly.
> 
> Not really, that would only work if they also have instructions to do
> raw accesses to physical memory addresses rather than virtual memory
> pointers that most architectures do.
> 
> > $ git grep IO_SPACE_LIMIT | grep -i ffffffff
> > arch/arm/include/asm/io.h:#define IO_SPACE_LIMIT ((resource_size_t)0xffffffff)
> > arch/arm/mach-at91/include/mach/io.h:#define IO_SPACE_LIMIT		0xFFFFFFFF
> > arch/arm/mach-omap1/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
> > arch/arm/mach-pxa/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
> > arch/arm/mach-s3c24xx/include/mach/io.h:#define IO_SPACE_LIMIT 0xffffffff
> 
> These use a special trick where an __iomem pointer is the same as
> the port number. This works most of the time, but breaks anything
> that assumes that port numbers are low, such as /dev/port or
> broken devices. Moreover, it means your code won't work because
> it depends on passing the virtual start address of the PIO mapping
> window as io_offset.
> 
> > arch/avr32/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff
> > arch/frv/include/asm/io.h:#define IO_SPACE_LIMIT	0xffffffff
> 
> They have no MMU, and the code relies on the port number to match
> both the virtual and the physical address. You could be right about
> these, but I would guess that the code also needs some other
> changes if we want to make it work on nommu kernels. It also depends
> on whether the I/O bus address is the same as the CPU address, or
> if it starts at bus address 0.
> 
> > arch/ia64/include/asm/io.h:#define IO_SPACE_LIMIT		0xffffffffffffffffUL
> 
> Here, the definition is special, the token is just used to encode
> a space number and an offset within the I/O space.
> 
> > arch/m32r/include/asm/io.h:#define IO_SPACE_LIMIT  0xFFFFFFFF
> 
> no PCI here.
> 
> > arch/m68k/include/asm/io_no.h:#define IO_SPACE_LIMIT 0xffffffff
> 
> This looks like a mistake, it should be smaller
> 
> > arch/microblaze/include/asm/io.h:#define IO_SPACE_LIMIT (0xFFFFFFFF)
> 
> I suspect it doesn't actually work. microblaze copied large parts
> of this from PowerPC, but the parts that differ apparently get
> it wrong for the I/O space. 
> 
> > arch/mn10300/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff
> 
> Same category as frv. We should ask David Howells whether he
> thinks I/O space actually works on these.
> 
> > arch/sh/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff
> 
> I think this should just be 0xffff.
> 
> > arch/sparc/include/asm/io_32.h:#define IO_SPACE_LIMIT 0xffffffff
> > arch/sparc/include/asm/io_64.h:#define IO_SPACE_LIMIT 0xffffffffffffffffUL
> 
> Sparc actually accesses the physical addresses, so in theory
> it could always work. In the 64-bit case it would however have
> to check that the port number is smaller than 0xffffffff, otherwise
> you couldn't set the BAR. This means you still need a custom
> function.
> 
> > arch/tile/include/asm/io.h:#define IO_SPACE_LIMIT 0xffffffff
> 
> tile seems to support only ioport_map() but not inb/outb, if I'm
> reading this right.
> 
> > > I did not ask to treat 'do nothing with the IO range' as an error,
> > > what I meant is that we should treat 'architecture cannot translate
> > > from I/O space to memory space but DT lists a translation anyway'
> > > as an error. On x86, you should never see an entry for the I/O space
> > > in "ranges", so we will not call this function unless there is a
> > > bug in DT.
> > 
> > Ok, it's just that there is no "architecture cannot translate from I/O
> > space to memory space" indicator anywhere and I don't want to make x86
> > a special case.
> 
> Right.
> 
> > So, my proposal is this: default weak implementation of pci_register_io_range()
> > returns an error (meaning I have no idea how to translate IO addresses
> > to memory space) and anyone that wants this function to return success will
> > have to provide their implementation.
> 
> Another idea: make this conditional on the definition of PCI_IOBASE: If this
> is defined, we can use the arm64 version that uses this number. Otherwise
> we fall back to returning an error, which means that either on the
> architecture we shouldn't be calling that function, or we need a custom
> implementation.

PCI_IOBASE is always defined. See the discussion with Russell on this subject.

include/asm-generic/io.h has at line 118:

#ifndef PCI_IOBASE
#define PCI_IOBASE ((void __iomem *) 0)
#endif

I will go with my idea tomorrow. arm64 overwrite the implementation anyway, I
find it cleaner rather than having to do #ifdefs and/or ifs.

Best regards,
Liviu

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

Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Arnd Bergmann]

On Monday 10 March 2014 21:56:00 Liviu Dudau wrote:
> 
> PCI_IOBASE is always defined. See the discussion with Russell on this subject.
> 
> include/asm-generic/io.h has at line 118:
> 
> #ifndef PCI_IOBASE
> #define PCI_IOBASE ((void __iomem *) 0)
> #endif

That is only defined for those that use asm-generic/pci.h, which most architectures
don't.
 
> I will go with my idea tomorrow. arm64 overwrite the implementation anyway, I
> find it cleaner rather than having to do #ifdefs and/or ifs.

I'd really hope we can get to a point where arm64 doesn't need any architecture
specific code for this. It doesn't do anything special.

	Arnd

Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Arnd Bergmann]

On Monday 10 March 2014 21:56:00 Liviu Dudau wrote:
> 
> PCI_IOBASE is always defined. See the discussion with Russell on this subject.
> 
> include/asm-generic/io.h has at line 118:
> 
> #ifndef PCI_IOBASE
> #define PCI_IOBASE ((void __iomem *) 0)
> #endif

That is only defined for those that use asm-generic/pci.h, which most architectures
don't.
 
> I will go with my idea tomorrow. arm64 overwrite the implementation anyway, I
> find it cleaner rather than having to do #ifdefs and/or ifs.

I'd really hope we can get to a point where arm64 doesn't need any architecture
specific code for this. It doesn't do anything special.

	Arnd

[PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Arnd Bergmann]

On Monday 10 March 2014 21:56:00 Liviu Dudau wrote:
> 
> PCI_IOBASE is always defined. See the discussion with Russell on this subject.
> 
> include/asm-generic/io.h has at line 118:
> 
> #ifndef PCI_IOBASE
> #define PCI_IOBASE ((void __iomem *) 0)
> #endif

That is only defined for those that use asm-generic/pci.h, which most architectures
don't.
 
> I will go with my idea tomorrow. arm64 overwrite the implementation anyway, I
> find it cleaner rather than having to do #ifdefs and/or ifs.

I'd really hope we can get to a point where arm64 doesn't need any architecture
specific code for this. It doesn't do anything special.

	Arnd

Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Liviu Dudau]

On Tue, Mar 11, 2014 at 06:50:24AM +0000, Arnd Bergmann wrote:
> On Monday 10 March 2014 21:56:00 Liviu Dudau wrote:
> > 
> > PCI_IOBASE is always defined. See the discussion with Russell on this subject.
> > 
> > include/asm-generic/io.h has at line 118:
> > 
> > #ifndef PCI_IOBASE
> > #define PCI_IOBASE ((void __iomem *) 0)
> > #endif
> 
> That is only defined for those that use asm-generic/pci.h, which most architectures
> don't.

I think it is defined for anyone that #includes <asm-generic/io.h>. There is no other
#ifdef around that.

>  
> > I will go with my idea tomorrow. arm64 overwrite the implementation anyway, I
> > find it cleaner rather than having to do #ifdefs and/or ifs.
> 
> I'd really hope we can get to a point where arm64 doesn't need any architecture
> specific code for this. It doesn't do anything special.

I agree.

Best regards,
Liviu

> 
> 	Arnd
> 
> 

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

Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Liviu Dudau]

On Tue, Mar 11, 2014 at 06:50:24AM +0000, Arnd Bergmann wrote:
> On Monday 10 March 2014 21:56:00 Liviu Dudau wrote:
> > 
> > PCI_IOBASE is always defined. See the discussion with Russell on this subject.
> > 
> > include/asm-generic/io.h has at line 118:
> > 
> > #ifndef PCI_IOBASE
> > #define PCI_IOBASE ((void __iomem *) 0)
> > #endif
> 
> That is only defined for those that use asm-generic/pci.h, which most architectures
> don't.

I think it is defined for anyone that #includes <asm-generic/io.h>. There is no other
#ifdef around that.

>  
> > I will go with my idea tomorrow. arm64 overwrite the implementation anyway, I
> > find it cleaner rather than having to do #ifdefs and/or ifs.
> 
> I'd really hope we can get to a point where arm64 doesn't need any architecture
> specific code for this. It doesn't do anything special.

I agree.

Best regards,
Liviu

> 
> 	Arnd
> 
> 

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

[PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Liviu Dudau]

On Tue, Mar 11, 2014 at 06:50:24AM +0000, Arnd Bergmann wrote:
> On Monday 10 March 2014 21:56:00 Liviu Dudau wrote:
> > 
> > PCI_IOBASE is always defined. See the discussion with Russell on this subject.
> > 
> > include/asm-generic/io.h has at line 118:
> > 
> > #ifndef PCI_IOBASE
> > #define PCI_IOBASE ((void __iomem *) 0)
> > #endif
> 
> That is only defined for those that use asm-generic/pci.h, which most architectures
> don't.

I think it is defined for anyone that #includes <asm-generic/io.h>. There is no other
#ifdef around that.

>  
> > I will go with my idea tomorrow. arm64 overwrite the implementation anyway, I
> > find it cleaner rather than having to do #ifdefs and/or ifs.
> 
> I'd really hope we can get to a point where arm64 doesn't need any architecture
> specific code for this. It doesn't do anything special.

I agree.

Best regards,
Liviu

> 
> 	Arnd
> 
> 

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

Re: [PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Arnd Bergmann]

On Tuesday 11 March 2014 09:46:41 Liviu Dudau wrote:
> On Tue, Mar 11, 2014 at 06:50:24AM +0000, Arnd Bergmann wrote:
> > On Monday 10 March 2014 21:56:00 Liviu Dudau wrote:
> > > 
> > > PCI_IOBASE is always defined. See the discussion with Russell on this subject.
> > > 
> > > include/asm-generic/io.h has at line 118:
> > > 
> > > #ifndef PCI_IOBASE
> > > #define PCI_IOBASE ((void __iomem *) 0)
> > > #endif
> > 
> > That is only defined for those that use asm-generic/pci.h, which most architectures
> > don't.
> 
> I think it is defined for anyone that #includes <asm-generic/io.h>. There is no other
> #ifdef around that.
> 

My mistake, I meant to write asm-generic/io.h.

On a related note, I would actually prefer to get rid of this PCI_IOBASE
default and move it into the architectures that really want it like this.
The default when PCI_IOBASE is not set IMHO should be to also not provide
inb/outb and ioport_map() helpers, but we need a little more infrastructure
to actually make the kernel build in all valid configuration when we remove them.

	Arnd

[PATCH v6 6/6] pci: Add support for creating a generic host_bridge from device tree

[Arnd Bergmann]

On Tuesday 11 March 2014 09:46:41 Liviu Dudau wrote:
> On Tue, Mar 11, 2014 at 06:50:24AM +0000, Arnd Bergmann wrote:
> > On Monday 10 March 2014 21:56:00 Liviu Dudau wrote:
> > > 
> > > PCI_IOBASE is always defined. See the discussion with Russell on this subject.
> > > 
> > > include/asm-generic/io.h has at line 118:
> > > 
> > > #ifndef PCI_IOBASE
> > > #define PCI_IOBASE ((void __iomem *) 0)
> > > #endif
> > 
> > That is only defined for those that use asm-generic/pci.h, which most architectures
> > don't.
> 
> I think it is defined for anyone that #includes <asm-generic/io.h>. There is no other
> #ifdef around that.
> 

My mistake, I meant to write asm-generic/io.h.

On a related note, I would actually prefer to get rid of this PCI_IOBASE
default and move it into the architectures that really want it like this.
The default when PCI_IOBASE is not set IMHO should be to also not provide
inb/outb and ioport_map() helpers, but we need a little more infrastructure
to actually make the kernel build in all valid configuration when we remove them.

	Arnd