pci: Provide support for parsing PCI DT ranges property

PATCH Superseded Andrew Murray 3 participants 9 messages Dec 12, 2012 – Jan 14, 2013 2 messages not in the archive

Subsystems (2)

PCI Device tree

lore

Share & tools

lore

Andrew Murray

author

2012-12-12 16:37 UTC #0 lore raw

PATCH 1/1 pci: Provide support for parsing PCI DT ranges property

DT bindings for PCI host bridges often use the ranges property to describe memory and IO ranges - this binding tends to be the same across architectures yet several parsing implementations exist, e.g. arch/mips/pci/pci.c, arch/powerpc/kernel/pci-common.c, arch/sparc/kernel/pci.c and arch/microblaze/pci/pci-common.c (clone of PPC). Some of these duplicate functionality provided by drivers/of/address.c.

This patch provides a common iterator-based parser for the ranges property, it is hoped this will reduce DT representation differences between architectures and that architectures will migrate in part to this new parser.

It is also hoped (and the motativation for the patch) that this patch will reduce duplication of code when writing host bridge drivers that are supported by multiple architectures.

This patch provides struct resources from a device tree node, e.g.:

	u32 *last = NULL;
	struct resource res;
	while ((last = of_pci_process_ranges(np, res, last))) {
		//do something with res
	}

Platforms with quirks can then do what they like with the resource or migrate common quirk handling to the parser. In an ideal world drivers can just request the obtained resources and pass them on (e.g. pci_add_resource_offset).

Signed-off-by Andrew Murray <[email protected]>

Signed-off-by Liviu Dudau <[email protected]>

---
 drivers/of/address.c       |   53 +++++++++++++++++++++++++++++++++++++++++++-
 include/linux/of_address.h |    7 +++++
 2 files changed, 59 insertions(+), 1 deletions(-)

drivers/of/address.c +52 -1

index 7e262a6..03bfe61 100644
@@ -219,6 +219,57 @@ int of_pci_address_to_resource(struct device_node *dev, int bar,
 	return __of_address_to_resource(dev, addrp, size, flags, NULL, r);
 }
 EXPORT_SYMBOL_GPL(of_pci_address_to_resource);
+
+const __be32 *of_pci_process_ranges(struct device_node *node,
+				    struct resource *res, const __be32 *from)
+{
+	const __be32 *start, *end;
+	int na, ns, np, pna;
+	int rlen;
+	struct of_bus *bus;
+	WARN_ON(!res);
+
+	bus = of_match_bus(node);
+	bus->count_cells(node, &na, &ns);
+	if (!OF_CHECK_COUNTS(na, ns)) {
+		pr_err("Bad cell count for %s\n", node->full_name);
+		return NULL;
+	}
+
+	pna = of_n_addr_cells(node);
+	np = pna + na + ns;
+
+	start = of_get_property(node, "ranges", &rlen);
+	if (start == NULL)
+		return NULL;
+
+	end = start + rlen;
+
+	if (!from)
+		from = start;
+
+	while (from + np <= end) {
+		u64 cpu_addr, size;
+
+		cpu_addr = of_translate_address(node, from + na);
+		size = of_read_number(from + na + pna, ns);
+		res->flags = bus->get_flags(from);
+		from += np;
+
+		if (cpu_addr == OF_BAD_ADDR || size == 0)
+			continue;
+
+		res->name = node->full_name;
+		res->start = cpu_addr;
+		res->end = res->start + size - 1;
+		res->parent = res->child = res->sibling = NULL;
+		return from;
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(of_pci_process_ranges);
+
 #endif /* CONFIG_PCI */
 
 /*

@@ -421,7 +472,7 @@ u64 __of_translate_address(struct device_node *dev, const __be32 *in_addr,
 		goto bail;
 	bus = of_match_bus(parent);
 
-	/* Cound address cells & copy address locally */
+	/* Count address cells & copy address locally */
 	bus->count_cells(dev, &na, &ns);
 	if (!OF_CHECK_COUNTS(na, ns)) {
 		printk(KERN_ERR "prom_parse: Bad cell count for %s\n",

include/linux/of_address.h +7 -0

index 01b925a..4582b20 100644
@@ -26,6 +26,8 @@ static inline unsigned long pci_address_to_pio(phys_addr_t addr) { return -1; }
 #define pci_address_to_pio pci_address_to_pio
 #endif
 
+const __be32 *of_pci_process_ranges(struct device_node *node,
+				    struct resource *res, const __be32 *from);
 #else /* CONFIG_OF_ADDRESS */
 static inline int of_address_to_resource(struct device_node *dev, int index,
 					 struct resource *r)

@@ -48,6 +50,11 @@ static inline const u32 *of_get_address(struct device_node *dev, int index,
 {
 	return NULL;
 }
+const __be32 *of_pci_process_ranges(struct device_node *node,
+				    struct resource *res, const __be32 *from)
+{
+	return NULL;
+}
 #endif /* CONFIG_OF_ADDRESS */

Signature · 1 line

1.7.0.4

A message that is not in the archive (sent off-list or not yet received): [email protected]

Thierry Reding

2012-12-13 09:13 UTC #2 lore raw

Quoted 27 lines · Andrew Murray

DT bindings for PCI host bridges often use the ranges property to describe memory and IO ranges - this binding tends to be the same across architectures yet several parsing implementations exist, e.g. arch/mips/pci/pci.c, arch/powerpc/kernel/pci-common.c, arch/sparc/kernel/pci.c and arch/microblaze/pci/pci-common.c (clone of PPC). Some of these duplicate functionality provided by drivers/of/address.c.
This patch provides a common iterator-based parser for the ranges property, it is hoped this will reduce DT representation differences between architectures and that architectures will migrate in part to this new parser.
It is also hoped (and the motativation for the patch) that this patch will reduce duplication of code when writing host bridge drivers that are supported by multiple architectures.
This patch provides struct resources from a device tree node, e.g.:
	u32 *last = NULL;
	struct resource res;
	while ((last = of_pci_process_ranges(np, res, last))) {
		//do something with res
	}
Platforms with quirks can then do what they like with the resource or migrate common quirk handling to the parser. In an ideal world drivers can just request the obtained resources and pass them on (e.g. pci_add_resource_offset).

Quoted 6 lines

Signed-off-by Andrew Murray <[email protected]>
Signed-off-by Liviu Dudau <[email protected]>
---
 drivers/of/address.c       |   53 +++++++++++++++++++++++++++++++++++++++++++-
 include/linux/of_address.h |    7 +++++
 2 files changed, 59 insertions(+), 1 deletions(-)

Hi Andrew,

I don't like iterator interfaces too much, but I can live with that. Other than that the patch looks good to me and I'll try to work it into my Tegra PCIe patch series.

Just two minor comments below.

Quoted 1 line

drivers/of/address.c +0 -0

[...]

drivers/of/address.c u64 __of_translate_address(struct device_node *dev, const __be32 *in_addr, · line 478

Quoted 9 lines

drivers/of/address.c +1 -1

@@ -421,7 +472,7 @@ u64 __of_translate_address(struct device_node *dev, const __be32 *in_addr,
 		goto bail;
 	bus = of_match_bus(parent);
 
-	/* Cound address cells & copy address locally */
+	/* Count address cells & copy address locally */
 	bus->count_cells(dev, &na, &ns);
 	if (!OF_CHECK_COUNTS(na, ns)) {
 		printk(KERN_ERR "prom_parse: Bad cell count for %s\n",

This is really minor, but it should still go into a separate patch.

Quoted 14 lines

include/linux/of_address.h +2 -0

index 01b925a..4582b20 100644
@@ -26,6 +26,8 @@ static inline unsigned long pci_address_to_pio(phys_addr_t addr) { return -1; }
 #define pci_address_to_pio pci_address_to_pio
 #endif
 
+const __be32 *of_pci_process_ranges(struct device_node *node,
+				    struct resource *res, const __be32 *from);
 #else /* CONFIG_OF_ADDRESS */
 static inline int of_address_to_resource(struct device_node *dev, int index,
 					 struct resource *r)

include/linux/of_address.h static inline const u32 *of_get_address(struct device_node *dev, int index, · line 53

Quoted 4 lines

include/linux/of_address.h +1 -0

@@ -48,6 +50,11 @@ static inline const u32 *of_get_address(struct device_node *dev, int index,
 {
 	return NULL;
 }
+const __be32 *of_pci_process_ranges(struct device_node *node,

There should be a blank line to separate the above two lines.

Thierry

_______________________________________________
devicetree-discuss mailing list devicetree-discuss-uLR06cmDAlY/[email protected]
https://lists.ozlabs.org/listinfo/devicetree-discuss

Andrew Murray

author

2012-12-13 09:45 UTC #3 lore raw

Quoted 30 lines · Thierry Reding

Hi Andrew,
I don't like iterator interfaces too much, but I can live with that. Other than that the patch looks good to me and I'll try to work it into my Tegra PCIe patch series.
Just two minor comments below.
Quoted 1 line
drivers/of/address.c +0 -0
[...]
drivers/of/address.c u64 __of_translate_address(struct device_node *dev, const __be32 *in_addr, · line 478
Quoted 9 lines
drivers/of/address.c +1 -1
@@ -421,7 +472,7 @@ u64 __of_translate_address(struct device_node *dev, const __be32 *in_addr,
 		goto bail;
 	bus = of_match_bus(parent);
 
-	/* Cound address cells & copy address locally */
+	/* Count address cells & copy address locally */
 	bus->count_cells(dev, &na, &ns);
 	if (!OF_CHECK_COUNTS(na, ns)) {
 		printk(KERN_ERR "prom_parse: Bad cell count for %s\n",
This is really minor, but it should still go into a separate patch.
Quoted 8 lines
include/linux/of_address.h +0 -0
index 01b925a..4582b20 100644
@@ -26,6 +26,8 @@ static inline unsigned long pci_address_to_pio(phys_addr_t addr) { return -1; }
 #define pci_address_to_pio pci_address_to_pio
 #endif
 

Quoted 13 lines

include/linux/of_address.h static inline const u32 *of_get_address(struct device_node *dev, int index, · line 53

Quoted 10 lines

include/linux/of_address.h +3 -0

+const __be32 *of_pci_process_ranges(struct device_node *node,
+				    struct resource *res, const __be32 *from);
 #else /* CONFIG_OF_ADDRESS */
 static inline int of_address_to_resource(struct device_node *dev, int index,
 					 struct resource *r)

@@ -48,6 +50,11 @@ static inline const u32 *of_get_address(struct device_node *dev, int index,
 {
 	return NULL;
 }
+const __be32 *of_pci_process_ranges(struct device_node *node,

There should be a blank line to separate the above two lines.

Thanks for the feedback.

I will send another patch for the typo and leave this patch with you for working into your existing series.

Andrew Murray

A message that is not in the archive (sent off-list or not yet received): [email protected]

Thierry Reding

2012-12-13 10:03 UTC #5 lore raw

Quoted 45 lines · Andrew Murray

Quoted 30 lines · Thierry Reding
Hi Andrew,
I don't like iterator interfaces too much, but I can live with that. Other than that the patch looks good to me and I'll try to work it into my Tegra PCIe patch series.
Just two minor comments below.
Quoted 1 line
drivers/of/address.c +0 -0
[...]
drivers/of/address.c u64 __of_translate_address(struct device_node *dev, const __be32 *in_addr, · line 478
Quoted 9 lines
drivers/of/address.c +1 -1
@@ -421,7 +472,7 @@ u64 __of_translate_address(struct device_node *dev, const __be32 *in_addr,
 		goto bail;
 	bus = of_match_bus(parent);
 
-	/* Cound address cells & copy address locally */
+	/* Count address cells & copy address locally */
 	bus->count_cells(dev, &na, &ns);
 	if (!OF_CHECK_COUNTS(na, ns)) {
 		printk(KERN_ERR "prom_parse: Bad cell count for %s\n",
This is really minor, but it should still go into a separate patch.
Quoted 8 lines
include/linux/of_address.h +0 -0
index 01b925a..4582b20 100644
@@ -26,6 +26,8 @@ static inline unsigned long pci_address_to_pio(phys_addr_t addr) { return -1; }
 #define pci_address_to_pio pci_address_to_pio
 #endif
 
Quoted 13 lines
include/linux/of_address.h static inline const u32 *of_get_address(struct device_node *dev, int index, · line 53
Quoted 10 lines
include/linux/of_address.h +3 -0
+const __be32 *of_pci_process_ranges(struct device_node *node,
+				    struct resource *res, const __be32 *from);
 #else /* CONFIG_OF_ADDRESS */
 static inline int of_address_to_resource(struct device_node *dev, int index,
 					 struct resource *r)
@@ -48,6 +50,11 @@ static inline const u32 *of_get_address(struct device_node *dev, int index,
 {
 	return NULL;
 }
+const __be32 *of_pci_process_ranges(struct device_node *node,
There should be a blank line to separate the above two lines.

Quoted 4 lines

Thanks for the feedback.
I will send another patch for the typo and leave this patch with you for working into your existing series.

I suppose you have your own series that uses this patch?

Thierry

_______________________________________________
devicetree-discuss mailing list devicetree-discuss-uLR06cmDAlY/[email protected]
https://lists.ozlabs.org/listinfo/devicetree-discuss

Andrew Murray

author

2012-12-13 10:34 UTC #6 lore raw

Quoted 46 lines · Thierry Reding

Quoted 45 lines · Andrew Murray
Quoted 43 lines · Thierry Reding
Hi Andrew,
I don't like iterator interfaces too much, but I can live with that. Other than that the patch looks good to me and I'll try to work it into my Tegra PCIe patch series.
Just two minor comments below.
Quoted 1 line
drivers/of/address.c +0 -0
[...]
drivers/of/address.c u64 __of_translate_address(struct device_node *dev, const __be32 *in_addr, · line 478
Quoted 9 lines
drivers/of/address.c +1 -1
@@ -421,7 +472,7 @@ u64 __of_translate_address(struct device_node *dev, const __be32 *in_addr,
 		goto bail;
 	bus = of_match_bus(parent);
 
-	/* Cound address cells & copy address locally */
+	/* Count address cells & copy address locally */
 	bus->count_cells(dev, &na, &ns);
 	if (!OF_CHECK_COUNTS(na, ns)) {
 		printk(KERN_ERR "prom_parse: Bad cell count for %s\n",
This is really minor, but it should still go into a separate patch.
Quoted 14 lines
include/linux/of_address.h +2 -0
index 01b925a..4582b20 100644
@@ -26,6 +26,8 @@ static inline unsigned long pci_address_to_pio(phys_addr_t addr) { return -1; }
 #define pci_address_to_pio pci_address_to_pio
 #endif
 
+const __be32 *of_pci_process_ranges(struct device_node *node,
+				    struct resource *res, const __be32 *from);
 #else /* CONFIG_OF_ADDRESS */
 static inline int of_address_to_resource(struct device_node *dev, int index,
 					 struct resource *r)
include/linux/of_address.h static inline const u32 *of_get_address(struct device_node *dev, int index, · line 53
Quoted 4 lines
include/linux/of_address.h +1 -0
@@ -48,6 +50,11 @@ static inline const u32 *of_get_address(struct device_node *dev, int index,
 {
 	return NULL;
 }
+const __be32 *of_pci_process_ranges(struct device_node *node,
There should be a blank line to separate the above two lines.

Quoted 6 lines

Quoted 4 lines
Thanks for the feedback.
I will send another patch for the typo and leave this patch with you for working into your existing series.
I suppose you have your own series that uses this patch?

Not yet, it may be some time before I submit my PCI host bridge driver. Though I am making changes else where (e.g. this patch) which I'm hoping to submit as early as possible. I can rebase my work for these upstream dependencies.

I can re-spin this patch with your suggested changes if you prefer?

Andrew Murray

Thierry Reding

2012-12-20 08:25 UTC #7 lore raw

In reply to a message not in the archive

On Wed, Dec 12, 2012 at 04:37:50PM +0000, Andrew Murray wrote:
[...]

Quoted 1 line

drivers/of/address.c +0 -0

[...]

drivers/of/address.c int of_pci_address_to_resource(struct device_node *dev, int bar, · line 246

Quoted 5 lines

drivers/of/address.c +5 -0

+	start = of_get_property(node, "ranges", &rlen);
+	if (start == NULL)
+		return NULL;
+
+	end = start + rlen;

I'm currently rewriting large parts of the Tegra PCIe controller driver and I'm trying to use this new API. This seems to work fine, except that I think this line needs to be:

	end = start + rlen / sizeof(__be32);

Otherwise we'll try to process 4 times as many ranges as there are.

Thierry

_______________________________________________
devicetree-discuss mailing list devicetree-discuss-uLR06cmDAlY/[email protected]
https://lists.ozlabs.org/listinfo/devicetree-discuss

Andrew Murray

author

2013-01-14 09:15 UTC #8 lore raw

Quoted 14 lines · Thierry Reding

On Wed, Dec 12, 2012 at 04:37:50PM +0000, Andrew Murray wrote:
[...]
Quoted 1 line
drivers/of/address.c +0 -0
[...]
drivers/of/address.c int of_pci_address_to_resource(struct device_node *dev, int bar, · line 246
Quoted 5 lines
drivers/of/address.c +5 -0
+	start = of_get_property(node, "ranges", &rlen);
+	if (start == NULL)
+		return NULL;
+
+	end = start + rlen;
I'm currently rewriting large parts of the Tegra PCIe controller driver and I'm trying to use this new API. This seems to work fine, except that I think this line needs to be:

Quoted 5 lines

	end = start + rlen / sizeof(__be32);
Otherwise we'll try to process 4 times as many ranges as there are.
Thierry

Good catch. Thanks for taking this on.

Andrew Murray

Grant Likely

2012-12-15 01:06 UTC #9 lore raw

In reply to Andrew Murray

Quoted 6 lines · Andrew Murray

DT bindings for PCI host bridges often use the ranges property to describe memory and IO ranges - this binding tends to be the same across architectures yet several parsing implementations exist, e.g. arch/mips/pci/pci.c, arch/powerpc/kernel/pci-common.c, arch/sparc/kernel/pci.c and arch/microblaze/pci/pci-common.c (clone of PPC). Some of these duplicate functionality provided by drivers/of/address.c.

Hi Andrew,

Thanks for looking into this. This definitely needs to be done.

However, I cannot merge this patch as-is because it actually makes things worse by adding yet another implementation of the parsing code. Plus it doesn't actually have any users. :-)

Instead, move the existing code that you need out of arch/powerpc/kernel/pci-common.c into a shared place and add in the features you need. Bonus points if you fixup microblaze or others at the same time.

Andrew Murray

author

2013-01-14 09:24 UTC #10 lore raw

Quoted 8 lines · Grant Likely

Quoted 6 lines · Andrew Murray
DT bindings for PCI host bridges often use the ranges property to describe memory and IO ranges - this binding tends to be the same across architectures yet several parsing implementations exist, e.g. arch/mips/pci/pci.c, arch/powerpc/kernel/pci-common.c, arch/sparc/kernel/pci.c and arch/microblaze/pci/pci-common.c (clone of PPC). Some of these duplicate functionality provided by drivers/of/address.c.

Quoted 7 lines

Hi Andrew,
Thanks for looking into this. This definitely needs to be done.
However, I cannot merge this patch as-is because it actually makes things worse by adding yet another implementation of the parsing code. Plus it doesn't actually have any users. :-)

I understand. Though I see Thierry has included this in his patch set - I guess that means there is potentially one user now :).

Quoted 5 lines

Instead, move the existing code that you need out of arch/powerpc/kernel/pci-common.c into a shared place and add in the features you need. Bonus points if you fixup microblaze or others at the same time.

In most part the patch I submitted was the common code from powerpc but without quirks and tie-ins to powerpc structures. I'd like to convert powerpc to using my patch and others but won't get time to do this at present :(

Quoted 3 lines

g.

Andrew Murray