Message-ID: <20110405023230.GA26817@ponder.secretlab.ca>
Date:	Mon, 4 Apr 2011 20:32:30 -0600
From:	Grant Likely <grant.likely@...retlab.ca>
To:	Benjamin Herrenschmidt <benh@...nel.crashing.org>
Cc:	linux-arch@...r.kernel.org,
	linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	David Miller <davem@...emloft.net>,
	"linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>
Subject: Re: [RFC/PATCH] of: Match PCI devices to OF nodes generically

On Mon, Apr 04, 2011 at 05:37:44PM +1000, Benjamin Herrenschmidt wrote:
> 
> > Nice, looks like I forgot to add the new drivers/pci/of.c file :-)
> > Here's a new patch. Also added linux-pci to the CC list.
> 
> And this one removes a lot more cruft from the powermac code while at
> it, and moves the core matching logic to drivers/of/of_pci.c...
> 
> From 917ea61d6afcf443ca467d0a6ffa00d5c6e21bb3 Mon Sep 17 00:00:00 2001
> From: Benjamin Herrenschmidt <benh@...nel.crashing.org>
> Date: Mon, 4 Apr 2011 14:38:13 +1000
> Subject: [PATCH] of: Match PCI devices to OF nodes dynamically
> 
> powerpc has two different ways of matching PCI devices to their
> corresponding OF node (if any) for historical reasons. The ppc64 one
> does a scan looking for matching bus/dev/fn, while the ppc32 one does a
> scan looking only for matching dev/fn on each level in order to be
> agnostic to busses being renumbered (which Linux does on some
> platforms).
> 
> This removes both and instead moves the matching code to the PCI core
> itself. It's the most logical place to do it: when a pci_dev is created,
> we know the parent and thus can do a single level scan for the matching
> device_node (if any).
> 
> The benefit is that all archs now get the matching for free. There's one
> hook the arch might want to provide to match a PHB bus to its device
> node. A default weak implementation is provided that looks for the
> parent device device node, but it's not entirely reliable on powerpc for
> various reasons so powerpc provides its own.

Awesome.  I'm looking at doing pretty much exactly the same thing for
USB and platform_devices on SoCs.  I'm glad to see this for pci.

> 
> Signed-off-by: Benjamin Herrenschmidt <benh@...nel.crashing.org>
> ---
>  arch/powerpc/include/asm/pci-bridge.h |   35 +++------
>  arch/powerpc/include/asm/pci.h        |    3 +-
>  arch/powerpc/include/asm/prom.h       |   14 ---
>  arch/powerpc/kernel/pci-common.c      |   11 ++-
>  arch/powerpc/kernel/pci_32.c          |  148 +++------------------------------
>  arch/powerpc/kernel/pci_dn.c          |   47 -----------
>  arch/powerpc/kernel/pci_of_scan.c     |    9 +--
>  arch/powerpc/platforms/powermac/pci.c |    3 +-
>  arch/sparc/kernel/pci.c               |    2 +-
>  drivers/of/of_pci.c                   |   36 ++++++++
>  drivers/pci/Makefile                  |    2 +
>  drivers/pci/hotplug/rpadlpar_core.c   |    2 +-
>  drivers/pci/of.c                      |   60 +++++++++++++
>  drivers/pci/probe.c                   |    8 ++-
>  include/linux/of_pci.h                |    5 +
>  include/linux/pci.h                   |   17 ++++
>  16 files changed, 165 insertions(+), 237 deletions(-)
>  create mode 100644 drivers/pci/of.c
> 
> diff --git a/arch/powerpc/include/asm/pci-bridge.h b/arch/powerpc/include/asm/pci-bridge.h
> index 5e156e0..6912c45 100644
> --- a/arch/powerpc/include/asm/pci-bridge.h
> +++ b/arch/powerpc/include/asm/pci-bridge.h
> @@ -169,18 +169,22 @@ static inline struct pci_controller *pci_bus_to_host(const struct pci_bus *bus)
>  	return bus->sysdata;
>  }
>  
> -#ifndef CONFIG_PPC64
> +static inline struct device_node *pci_device_to_OF_node(struct pci_dev *dev)
> +{
> +	return dev->dev.of_node;
> +}
>  
>  static inline struct device_node *pci_bus_to_OF_node(struct pci_bus *bus)
>  {
> -	struct pci_controller *host;
> -
> -	if (bus->self)
> -		return pci_device_to_OF_node(bus->self);
> -	host = pci_bus_to_host(bus);
> -	return host ? host->dn : NULL;
> +	return bus->dev.of_node;
>  }

Should these two inlines move to include/linux/of_pci.h?  Microblaze
defines it differently, but I don't think it should be.  Sparc also
has a different variant in arch/sparc/kernel/pci.c, but not in
arch/sparc/kernel/pcic.c.  It looks to me like this should be the
common case unless a specific platform needs otherwise.

>  
> +#ifndef CONFIG_PPC64
> +
> +extern int pci_device_from_OF_node(struct device_node *node,
> +				   u8* bus, u8* devfn);
> +extern void pci_create_OF_bus_map(void);
> +
>  static inline int isa_vaddr_is_ioport(void __iomem *address)
>  {
>  	/* No specific ISA handling on ppc32 at this stage, it
> @@ -223,17 +227,8 @@ struct pci_dn {
>  /* Get the pointer to a device_node's pci_dn */
>  #define PCI_DN(dn)	((struct pci_dn *) (dn)->data)
>  
> -extern struct device_node *fetch_dev_dn(struct pci_dev *dev);
>  extern void * update_dn_pci_info(struct device_node *dn, void *data);
>  
> -/* Get a device_node from a pci_dev.  This code must be fast except
> - * in the case where the sysdata is incorrect and needs to be fixed
> - * up (this will only happen once). */
> -static inline struct device_node *pci_device_to_OF_node(struct pci_dev *dev)
> -{
> -	return dev->dev.of_node ? dev->dev.of_node : fetch_dev_dn(dev);
> -}
> -
>  static inline int pci_device_from_OF_node(struct device_node *np,
>  					  u8 *bus, u8 *devfn)
>  {
> @@ -244,14 +239,6 @@ static inline int pci_device_from_OF_node(struct device_node *np,
>  	return 0;
>  }
>  
> -static inline struct device_node *pci_bus_to_OF_node(struct pci_bus *bus)
> -{
> -	if (bus->self)
> -		return pci_device_to_OF_node(bus->self);
> -	else
> -		return bus->dev.of_node; /* Must be root bus (PHB) */
> -}
> -

Yay!

>  /** Find the bus corresponding to the indicated device node */
>  extern struct pci_bus *pcibios_find_pci_bus(struct device_node *dn);
>  
> diff --git a/arch/powerpc/include/asm/pci.h b/arch/powerpc/include/asm/pci.h
> index 7d77909..1f52268 100644
> --- a/arch/powerpc/include/asm/pci.h
> +++ b/arch/powerpc/include/asm/pci.h
> @@ -179,8 +179,7 @@ extern int remove_phb_dynamic(struct pci_controller *phb);
>  extern struct pci_dev *of_create_pci_dev(struct device_node *node,
>  					struct pci_bus *bus, int devfn);
>  
> -extern void of_scan_pci_bridge(struct device_node *node,
> -				struct pci_dev *dev);
> +extern void of_scan_pci_bridge(struct pci_dev *dev);
>  
>  extern void of_scan_bus(struct device_node *node, struct pci_bus *bus);
>  extern void of_rescan_bus(struct device_node *node, struct pci_bus *bus);
> diff --git a/arch/powerpc/include/asm/prom.h b/arch/powerpc/include/asm/prom.h
> index c189aa5..b823536 100644
> --- a/arch/powerpc/include/asm/prom.h
> +++ b/arch/powerpc/include/asm/prom.h
> @@ -22,20 +22,6 @@
>  
>  #define HAVE_ARCH_DEVTREE_FIXUPS
>  
> -#ifdef CONFIG_PPC32
> -/*
> - * PCI <-> OF matching functions
> - * (XXX should these be here?)
> - */
> -struct pci_bus;
> -struct pci_dev;
> -extern int pci_device_from_OF_node(struct device_node *node,
> -				   u8* bus, u8* devfn);
> -extern struct device_node* pci_busdev_to_OF_node(struct pci_bus *, int);
> -extern struct device_node* pci_device_to_OF_node(struct pci_dev *);
> -extern void pci_create_OF_bus_map(void);
> -#endif
> -
>  /*
>   * OF address retreival & translation
>   */
> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> index 893af2a..47c516b 100644
> --- a/arch/powerpc/kernel/pci-common.c
> +++ b/arch/powerpc/kernel/pci-common.c
> @@ -1097,9 +1097,6 @@ void __devinit pcibios_setup_bus_devices(struct pci_bus *bus)
>  		if (dev->is_added)
>  			continue;
>  
> -		/* Setup OF node pointer in the device */
> -		dev->dev.of_node = pci_device_to_OF_node(dev);
> -
>  		/* Fixup NUMA node as it may not be setup yet by the generic
>  		 * code and is needed by the DMA init
>  		 */
> @@ -1685,6 +1682,13 @@ int early_find_capability(struct pci_controller *hose, int bus, int devfn,
>  	return pci_bus_find_capability(fake_pci_bus(hose, bus), devfn, cap);
>  }
>  
> +struct device_node * pcibios_get_phb_of_node(struct pci_bus *bus)
> +{
> +	struct pci_controller *hose = bus->sysdata;
> +
> +	return of_node_get(hose->dn);
> +}
> +
>  /**
>   * pci_scan_phb - Given a pci_controller, setup and scan the PCI bus
>   * @hose: Pointer to the PCI host controller instance structure
> @@ -1705,7 +1709,6 @@ void __devinit pcibios_scan_phb(struct pci_controller *hose)
>  			hose->global_number);
>  		return;
>  	}
> -	bus->dev.of_node = of_node_get(node);
>  	bus->secondary = hose->first_busno;
>  	hose->bus = bus;
>  
> diff --git a/arch/powerpc/kernel/pci_32.c b/arch/powerpc/kernel/pci_32.c
> index bedb370..ca6bcb7 100644
> --- a/arch/powerpc/kernel/pci_32.c
> +++ b/arch/powerpc/kernel/pci_32.c
> @@ -167,150 +167,26 @@ pcibios_make_OF_bus_map(void)
>  #endif
>  }
>  
> -typedef int (*pci_OF_scan_iterator)(struct device_node* node, void* data);
> -
> -static struct device_node*
> -scan_OF_pci_childs(struct device_node *parent, pci_OF_scan_iterator filter, void* data)
> -{
> -	struct device_node *node;
> -	struct device_node* sub_node;
> -
> -	for_each_child_of_node(parent, node) {
> -		const unsigned int *class_code;
> -	
> -		if (filter(node, data)) {
> -			of_node_put(node);
> -			return node;
> -		}
> -
> -		/* For PCI<->PCI bridges or CardBus bridges, we go down
> -		 * Note: some OFs create a parent node "multifunc-device" as
> -		 * a fake root for all functions of a multi-function device,
> -		 * we go down them as well.
> -		 */
> -		class_code = of_get_property(node, "class-code", NULL);
> -		if ((!class_code || ((*class_code >> 8) != PCI_CLASS_BRIDGE_PCI &&
> -			(*class_code >> 8) != PCI_CLASS_BRIDGE_CARDBUS)) &&
> -			strcmp(node->name, "multifunc-device"))
> -			continue;
> -		sub_node = scan_OF_pci_childs(node, filter, data);
> -		if (sub_node) {
> -			of_node_put(node);
> -			return sub_node;
> -		}
> -	}
> -	return NULL;
> -}
> -
> -static struct device_node *scan_OF_for_pci_dev(struct device_node *parent,
> -					       unsigned int devfn)
> -{
> -	struct device_node *np, *cnp;
> -	const u32 *reg;
> -	unsigned int psize;
> -
> -	for_each_child_of_node(parent, np) {
> -		reg = of_get_property(np, "reg", &psize);
> -                if (reg && psize >= 4 && ((reg[0] >> 8) & 0xff) == devfn)
> -			return np;
> -
> -		/* Note: some OFs create a parent node "multifunc-device" as
> -		 * a fake root for all functions of a multi-function device,
> -		 * we go down them as well. */
> -                if (!strcmp(np->name, "multifunc-device")) {
> -                        cnp = scan_OF_for_pci_dev(np, devfn);
> -                        if (cnp)
> -                                return cnp;
> -                }
> -	}
> -	return NULL;
> -}
> -
> -
> -static struct device_node *scan_OF_for_pci_bus(struct pci_bus *bus)
> -{
> -	struct device_node *parent, *np;
> -
> -	/* Are we a root bus ? */
> -	if (bus->self == NULL || bus->parent == NULL) {
> -		struct pci_controller *hose = pci_bus_to_host(bus);
> -		if (hose == NULL)
> -			return NULL;
> -		return of_node_get(hose->dn);
> -	}
> -
> -	/* not a root bus, we need to get our parent */
> -	parent = scan_OF_for_pci_bus(bus->parent);
> -	if (parent == NULL)
> -		return NULL;
> -
> -	/* now iterate for children for a match */
> -	np = scan_OF_for_pci_dev(parent, bus->self->devfn);
> -	of_node_put(parent);
> -
> -	return np;
> -}
> -
> -/*
> - * Scans the OF tree for a device node matching a PCI device
> - */
> -struct device_node *
> -pci_busdev_to_OF_node(struct pci_bus *bus, int devfn)
> -{
> -	struct device_node *parent, *np;
> -
> -	pr_debug("pci_busdev_to_OF_node(%d,0x%x)\n", bus->number, devfn);
> -	parent = scan_OF_for_pci_bus(bus);
> -	if (parent == NULL)
> -		return NULL;
> -	pr_debug(" parent is %s\n", parent ? parent->full_name : "<NULL>");
> -	np = scan_OF_for_pci_dev(parent, devfn);
> -	of_node_put(parent);
> -	pr_debug(" result is %s\n", np ? np->full_name : "<NULL>");
> -
> -	/* XXX most callers don't release the returned node
> -	 * mostly because ppc64 doesn't increase the refcount,
> -	 * we need to fix that.
> -	 */
> -	return np;
> -}
> -EXPORT_SYMBOL(pci_busdev_to_OF_node);
> -
> -struct device_node*
> -pci_device_to_OF_node(struct pci_dev *dev)
> -{
> -	return pci_busdev_to_OF_node(dev->bus, dev->devfn);
> -}
> -EXPORT_SYMBOL(pci_device_to_OF_node);
> -
> -static int
> -find_OF_pci_device_filter(struct device_node* node, void* data)
> -{
> -	return ((void *)node == data);
> -}
>  
>  /*
>   * Returns the PCI device matching a given OF node
>   */
> -int
> -pci_device_from_OF_node(struct device_node* node, u8* bus, u8* devfn)
> +int pci_device_from_OF_node(struct device_node* node, u8* bus, u8* devfn)
>  {
> -	const unsigned int *reg;
> -	struct pci_controller* hose;
>  	struct pci_dev* dev = NULL;
> -	
> -	/* Make sure it's really a PCI device */
> -	hose = pci_find_hose_for_OF_device(node);
> -	if (!hose || !hose->dn)
> -		return -ENODEV;
> -	if (!scan_OF_pci_childs(hose->dn,
> -			find_OF_pci_device_filter, (void *)node))
> +	const __be32 *reg;
> +	int size;
> +
> +	/* Check if it might have a chance to be a PCI device */
> +	if (!pci_find_hose_for_OF_device(node))
>  		return -ENODEV;
> -	reg = of_get_property(node, "reg", NULL);
> -	if (!reg)
> +
> +	reg = of_get_property(node, "reg", &size);
> +	if (!reg || size < 5 * sizeof(u32))
>  		return -ENODEV;
> -	*bus = (reg[0] >> 16) & 0xff;
> -	*devfn = ((reg[0] >> 8) & 0xff);
> +	
> +	*bus = (be32_to_cpup(&reg[0]) >> 16) & 0xff;
> +	*devfn = (be32_to_cpup(&reg[0]) >> 8) & 0xff;
>  
>  	/* Ok, here we need some tweak. If we have already renumbered
>  	 * all busses, we can't rely on the OF bus number any more.
> diff --git a/arch/powerpc/kernel/pci_dn.c b/arch/powerpc/kernel/pci_dn.c
> index d225d99..8cb66a2 100644
> --- a/arch/powerpc/kernel/pci_dn.c
> +++ b/arch/powerpc/kernel/pci_dn.c
> @@ -143,53 +143,6 @@ void __devinit pci_devs_phb_init_dynamic(struct pci_controller *phb)
>  	traverse_pci_devices(dn, update_dn_pci_info, phb);
>  }
>  
> -/*
> - * Traversal func that looks for a <busno,devfcn> value.
> - * If found, the pci_dn is returned (thus terminating the traversal).
> - */
> -static void *is_devfn_node(struct device_node *dn, void *data)
> -{
> -	int busno = ((unsigned long)data >> 8) & 0xff;
> -	int devfn = ((unsigned long)data) & 0xff;
> -	struct pci_dn *pci = dn->data;
> -
> -	if (pci && (devfn == pci->devfn) && (busno == pci->busno))
> -		return dn;
> -	return NULL;
> -}
> -
> -/*
> - * This is the "slow" path for looking up a device_node from a
> - * pci_dev.  It will hunt for the device under its parent's
> - * phb and then update of_node pointer.
> - *
> - * It may also do fixups on the actual device since this happens
> - * on the first read/write.
> - *
> - * Note that it also must deal with devices that don't exist.
> - * In this case it may probe for real hardware ("just in case")
> - * and add a device_node to the device tree if necessary.
> - *
> - * Is this function necessary anymore now that dev->dev.of_node is
> - * used to store the node pointer?
> - *
> - */
> -struct device_node *fetch_dev_dn(struct pci_dev *dev)
> -{
> -	struct pci_controller *phb = dev->sysdata;
> -	struct device_node *dn;
> -	unsigned long searchval = (dev->bus->number << 8) | dev->devfn;
> -
> -	if (WARN_ON(!phb))
> -		return NULL;
> -
> -	dn = traverse_pci_devices(phb->dn, is_devfn_node, (void *)searchval);
> -	if (dn)
> -		dev->dev.of_node = dn;
> -	return dn;
> -}
> -EXPORT_SYMBOL(fetch_dev_dn);
> -
>  /** 
>   * pci_devs_phb_init - Initialize phbs and pci devs under them.
>   * 
> diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
> index 1e89a72..fe0a5ad 100644
> --- a/arch/powerpc/kernel/pci_of_scan.c
> +++ b/arch/powerpc/kernel/pci_of_scan.c
> @@ -202,9 +202,9 @@ EXPORT_SYMBOL(of_create_pci_dev);
>   * this routine in turn call of_scan_bus() recusively to scan for more child
>   * devices.
>   */
> -void __devinit of_scan_pci_bridge(struct device_node *node,
> -				  struct pci_dev *dev)
> +void __devinit of_scan_pci_bridge(struct pci_dev *dev)
>  {
> +	struct device_node *node = dev->dev.of_node;
>  	struct pci_bus *bus;
>  	const u32 *busrange, *ranges;
>  	int len, i, mode;
> @@ -238,7 +238,6 @@ void __devinit of_scan_pci_bridge(struct device_node *node,
>  	bus->primary = dev->bus->number;
>  	bus->subordinate = busrange[1];
>  	bus->bridge_ctl = 0;
> -	bus->dev.of_node = of_node_get(node);
>  
>  	/* parse ranges property */
>  	/* PCI #address-cells == 3 and #size-cells == 2 always */
> @@ -335,9 +334,7 @@ static void __devinit __of_scan_bus(struct device_node *node,
>  	list_for_each_entry(dev, &bus->devices, bus_list) {
>  		if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||
>  		    dev->hdr_type == PCI_HEADER_TYPE_CARDBUS) {
> -			struct device_node *child = pci_device_to_OF_node(dev);
> -			if (child)
> -				of_scan_pci_bridge(child, dev);
> +			of_scan_pci_bridge(dev);
>  		}
>  	}
>  }
> diff --git a/arch/powerpc/platforms/powermac/pci.c b/arch/powerpc/platforms/powermac/pci.c
> index ab68989..68a18da 100644
> --- a/arch/powerpc/platforms/powermac/pci.c
> +++ b/arch/powerpc/platforms/powermac/pci.c
> @@ -17,6 +17,7 @@
>  #include <linux/init.h>
>  #include <linux/bootmem.h>
>  #include <linux/irq.h>
> +#include <linux/of_pci.h>
>  
>  #include <asm/sections.h>
>  #include <asm/io.h>
> @@ -235,7 +236,7 @@ static int chaos_validate_dev(struct pci_bus *bus, int devfn, int offset)
>  
>  	if (offset >= 0x100)
>  		return  PCIBIOS_BAD_REGISTER_NUMBER;
> -	np = pci_busdev_to_OF_node(bus, devfn);
> +	np = of_pci_find_child_device(bus->dev.of_node, devfn);
>  	if (np == NULL)
>  		return PCIBIOS_DEVICE_NOT_FOUND;
>  
> diff --git a/arch/sparc/kernel/pci.c b/arch/sparc/kernel/pci.c
> index 713dc91..e539d23 100644
> --- a/arch/sparc/kernel/pci.c
> +++ b/arch/sparc/kernel/pci.c
> @@ -284,7 +284,7 @@ static struct pci_dev *of_create_pci_dev(struct pci_pbm_info *pbm,
>  	dev->sysdata = node;
>  	dev->dev.parent = bus->bridge;
>  	dev->dev.bus = &pci_bus_type;
> -	dev->dev.of_node = node;
> +	dev->dev.of_node = of_node_get(node);
>  	dev->devfn = devfn;
>  	dev->multifunction = 0;		/* maybe a lie? */
>  	set_pcie_port_type(dev);
> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> index ac1ec54..9d179c4 100644
> --- a/drivers/of/of_pci.c
> +++ b/drivers/of/of_pci.c
> @@ -90,3 +90,39 @@ int of_irq_map_pci(struct pci_dev *pdev, struct of_irq *out_irq)
>  	return of_irq_map_raw(ppnode, &lspec_be, 1, laddr, out_irq);
>  }
>  EXPORT_SYMBOL_GPL(of_irq_map_pci);
> +
> +
> +static inline int __of_pci_pci_compare(struct device_node *node, unsigned int devfn)
> +{
> +	unsigned int size;
> +	const __be32 *reg = of_get_property(node, "reg", &size);
> +
> +	if (!reg || size < 5 * sizeof(__be32))
> +		return 0;
> +	return ((be32_to_cpup(&reg[0]) >> 8) & 0xff) == devfn;
> +}
> +
> +struct device_node *of_pci_find_child_device(struct device_node *parent,
> +					     unsigned int devfn)
> +{
> +	struct device_node *node, *node2;
> +	
> +	for_each_child_of_node(parent, node) {
> +		if (__of_pci_pci_compare(node, devfn))
> +			return node;
> +		/*
> +		 * Some OFs create a parent node "multifunc-device" as
> +		 * a fake root for all functions of a multi-function
> +		 * device we go down them as well.
> +		 */
> +                if (!strcmp(node->name, "multifunc-device")) {
> +			for_each_child_of_node(node, node2) {
> +				if (__of_pci_pci_compare(node2, devfn)) {
> +					of_node_put(node);
> +					return node2;
> +				}
> +			}
> +                }
> +	}
> +	return NULL;
> +}
> diff --git a/drivers/pci/Makefile b/drivers/pci/Makefile
> index 98d61c8..d5c3cb9 100644
> --- a/drivers/pci/Makefile
> +++ b/drivers/pci/Makefile
> @@ -70,4 +70,6 @@ obj-$(CONFIG_PCI_STUB) += pci-stub.o
>  
>  obj-$(CONFIG_XEN_PCIDEV_FRONTEND) += xen-pcifront.o
>  
> +obj-$(CONFIG_OF) += of.o
> +
>  ccflags-$(CONFIG_PCI_DEBUG) := -DDEBUG
> diff --git a/drivers/pci/hotplug/rpadlpar_core.c b/drivers/pci/hotplug/rpadlpar_core.c
> index 0830347..1d002b1 100644
> --- a/drivers/pci/hotplug/rpadlpar_core.c
> +++ b/drivers/pci/hotplug/rpadlpar_core.c
> @@ -158,7 +158,7 @@ static void dlpar_pci_add_bus(struct device_node *dn)
>  	/* Scan below the new bridge */
>  	if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE ||
>  	    dev->hdr_type == PCI_HEADER_TYPE_CARDBUS)
> -		of_scan_pci_bridge(dn, dev);
> +		of_scan_pci_bridge(dev);
>  
>  	/* Map IO space for child bus, which may or may not succeed */
>  	pcibios_map_io_space(dev->subordinate);
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> new file mode 100644
> index 0000000..fff7270
> --- /dev/null
> +++ b/drivers/pci/of.c

Should this be consolidated with drivers/of/of_pci.c?  I don't have
strong opinions about where the result lives, but I don't think they
should be split up.

> @@ -0,0 +1,60 @@
> +/*
> + * PCI <-> OF mapping helpers
> + *
> + * Copyright 2011 IBM Corp.
> + * 
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/pci.h>
> +#include <linux/of_pci.h>
> +#include "pci.h"
> +
> +void pci_set_of_node(struct pci_dev *dev)
> +{
> +	if (!dev->bus->dev.of_node)
> +		return;
> +	dev->dev.of_node = of_pci_find_child_device(dev->bus->dev.of_node,
> +						    dev->devfn);
> +}
> +
> +void pci_release_of_node(struct pci_dev *dev)
> +{
> +	of_node_put(dev->dev.of_node);
> +	dev->dev.of_node = NULL;
> +}
> +
> +void pci_set_bus_of_node(struct pci_bus *bus)
> +{
> +	if (bus->self == NULL)
> +		bus->dev.of_node = pcibios_get_phb_of_node(bus);
> +	else
> +		bus->dev.of_node = of_node_get(bus->self->dev.of_node);
> +}
> +
> +void pci_release_bus_of_node(struct pci_bus *bus)
> +{
> +	of_node_put(bus->dev.of_node);
> +	bus->dev.of_node = NULL;
> +}
> +
> +struct device_node * __weak pcibios_get_phb_of_node(struct pci_bus *bus)
> +{
> +	/* This should only be called for PHBs */
> +	if (WARN_ON(bus->self || bus->parent))
> +		return NULL;
> +
> +	/* Look for a node pointer in either the intermediary device we
> +	 * create above the root bus or it's own parent. Normally only
> +	 * the later is populated.
> +	 */
> +	if (bus->bridge->of_node)
> +		return of_node_get(bus->bridge->of_node);
> +	if (bus->bridge->parent->of_node)
> +		return of_node_get(bus->bridge->parent->of_node);
> +	return NULL;
> +}
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 44cbbba..347349b 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -89,6 +89,7 @@ static void release_pcibus_dev(struct device *dev)
>  	if (pci_bus->bridge)
>  		put_device(pci_bus->bridge);
>  	pci_bus_remove_resources(pci_bus);
> +	pci_release_bus_of_node(pci_bus);
>  	kfree(pci_bus);
>  }
>  
> @@ -624,7 +625,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
>  
>  	child->self = bridge;
>  	child->bridge = get_device(&bridge->dev);
> -
> +	pci_set_bus_of_node(child);	

If I'm reading this correctly, the only time an of_node can be
associated with a pci_bus is here and pci_alloc_bus.  In both cases
that makes it safe for release_pcibus_dev() to remove it.  Am I
correct?

oh, and trailing whitespace.  :-)

>  	pci_set_bus_speed(child);
>  
>  	/* Set up default resource pointers and names.. */
> @@ -1074,6 +1075,7 @@ static void pci_release_dev(struct device *dev)
>  
>  	pci_dev = to_pci_dev(dev);
>  	pci_release_capabilities(pci_dev);
> +	pci_release_of_node(pci_dev);
>  	kfree(pci_dev);
>  }
>  
> @@ -1150,6 +1152,7 @@ struct pci_dev *alloc_pci_dev(void)
>  }
>  EXPORT_SYMBOL(alloc_pci_dev);
>  
> +
>  /*
>   * Read the config data for a PCI device, sanity-check it
>   * and fill in the dev structure...

Unrelated whitespace change

> @@ -1193,6 +1196,8 @@ static struct pci_dev *pci_scan_device(struct pci_bus *bus, int devfn)
>  	dev->vendor = l & 0xffff;
>  	dev->device = (l >> 16) & 0xffff;
>  
> +	pci_set_of_node(dev);
> +
>  	if (pci_setup_device(dev)) {
>  		kfree(dev);
>  		return NULL;
> @@ -1445,6 +1450,7 @@ struct pci_bus * pci_create_bus(struct device *parent,
>  		goto dev_reg_err;
>  	b->bridge = get_device(dev);
>  	device_enable_async_suspend(b->bridge);
> +	pci_set_bus_of_node(b);

Similarly here; so device node linkage to pci devices will always use
the same mechanism except for in of_create_pci_dev() for sparc, right?

>  
>  	if (!parent)
>  		set_dev_node(b->bridge, pcibus_to_node(b));
> diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
> index 85a27b6..f93e217 100644
> --- a/include/linux/of_pci.h
> +++ b/include/linux/of_pci.h
> @@ -6,4 +6,9 @@
>  struct pci_dev;
>  struct of_irq;
>  int of_irq_map_pci(struct pci_dev *pdev, struct of_irq *out_irq);
> +
> +struct device_node;
> +struct device_node *of_pci_find_child_device(struct device_node *parent,
> +					     unsigned int devfn);
> +
>  #endif
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 96f70d7..e5111da 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1543,5 +1543,22 @@ int pci_vpd_find_tag(const u8 *buf, unsigned int off, unsigned int len, u8 rdt);
>  int pci_vpd_find_info_keyword(const u8 *buf, unsigned int off,
>  			      unsigned int len, const char *kw);
>  
> +/* PCI <-> OF binding helpers */
> +#ifdef  CONFIG_OF
> +#include <linux/of.h>

linux/of.h is safe to include when CONFIG_OF is not selected; it can
live at the top of the file with the rest of the includes.

> +extern void pci_set_of_node(struct pci_dev *dev);
> +extern void pci_release_of_node(struct pci_dev *dev);
> +extern void pci_set_bus_of_node(struct pci_bus *bus);
> +extern void pci_release_bus_of_node(struct pci_bus *bus);

Looks to me like these functions still get called when
!defined(CONFIG_OF).

> +
> +/* Arch may override this (weak) */
> +extern struct device_node * __weak pcibios_get_phb_of_node(struct pci_bus *bus);
> +
> +
> +#else /* CONFIG_OF */
> +static void pci_locate_of_node(struct pci_dev *dev) { }
> +static void pci_release_of_node(struct pci_dev *dev) { }
> +#endif  /* CONFIG_OF */
> +
>  #endif /* __KERNEL__ */
>  #endif /* LINUX_PCI_H */
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@...r.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-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists