Re: [PATCH] pci: Provide support for parsing PCI DT ranges property

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

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 <redacted>
Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com>
---
 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.

[...]

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

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

Thierry

On Thu, Dec 13, 2012 at 09:13:33AM +0000, Thierry Reding wrote:

Hi Andrew,
=20
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.
=20
Just two minor comments below.
=20

quoted

diff --git a/drivers/of/address.c b/drivers/of/address.c

[...]

quoted

@@ -421,7 +472,7 @@ u64 __of_translate_address(struct device_node *dev,=

 const __be32 *in_addr,

quoted

 =09=09goto bail;
 =09bus =3D of_match_bus(parent);
=20
-=09/* Cound address cells & copy address locally */
+=09/* Count address cells & copy address locally */
 =09bus->count_cells(dev, &na, &ns);
 =09if (!OF_CHECK_COUNTS(na, ns)) {
 =09=09printk(KERN_ERR "prom_parse: Bad cell count for %s\n",

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

quoted

diff --git a/include/linux/of_address.h b/include/linux/of_address.h
index 01b925a..4582b20 100644
--- a/include/linux/of_address.h
+++ b/include/linux/of_address.h

@@ -26,6 +26,8 @@ static inline unsigned long pci_address_to_pio(phys_a=

ddr_t addr) { return -1; }

quoted

 #define pci_address_to_pio pci_address_to_pio
 #endif
=20
+const __be32 *of_pci_process_ranges(struct device_node *node,
+=09=09=09=09    struct resource *res, const __be32 *from);
 #else /* CONFIG_OF_ADDRESS */
 static inline int of_address_to_resource(struct device_node *dev, int =

index,

quoted

 =09=09=09=09=09 struct resource *r)

@@ -48,6 +50,11 @@ static inline const u32 *of_get_address(struct devic=

e_node *dev, int index,

quoted

 {
 =09return NULL;
 }
+const __be32 *of_pci_process_ranges(struct device_node *node,

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

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