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

[Thierry Reding <thierry.reding@avionic-design.de>]

[-- Attachment #1: Type: text/plain, Size: 3320 bytes --]

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 <Andrew.Murray@arm.com>
> 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.

> diff --git a/drivers/of/address.c b/drivers/of/address.c
[...]
> @@ -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.

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

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

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]