This is v4 of my attempt to add support for a generic pci_host_bridge controller created from a description passed in the device tree.
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.
v1 thread here: https://lkml.org/lkml/2014/2/3/380
The following is an edit of the original blurb:
Following the discussion started here [1], I now have a proposal for tackling generic support for host bridges described via device tree. It is an initial stab at it, to try to get feedback and suggestions, but it is functional enough that I have PCI Express for arm64 working on an FPGA using the patch that I am also publishing that adds support for PCI for that platform.
Looking at the existing architectures that fit the requirements (use of device tree and PCI) yields the powerpc and microblaze as generic enough to make them candidates for conversion. I have a tentative patch for microblaze that I can only compile test it, unfortunately using qemu-microblaze leads to an early crash in the kernel.
As Bjorn has mentioned in the previous discussion, the idea is to add to struct pci_host_bridge enough data to be able to reduce the size or remove the architecture specific pci_controller structure. arm64 support actually manages to get rid of all the architecture static data and has no pci_controller structure defined. For host bridge drivers that means a change of API unless architectures decide to provide a compatibility layer (comments here please).
In order to initialise a host bridge with the new API, the following example code is sufficient for a _probe() function:
static int myhostbridge_probe(struct platform_device *pdev) { int err; struct device_node *dev; struct pci_host_bridge *bridge; struct myhostbridge_port *pp; resource_size_t lastbus;
dev = pdev->dev.of_node;
if (!of_device_is_available(dev)) { pr_warn("%s: disabled\n", dev->full_name); return -ENODEV; }
pp = kzalloc(sizeof(struct myhostbridge_port), GFP_KERNEL); if (!pp) return -ENOMEM;
bridge = of_create_pci_host_bridge(&pdev->dev, &myhostbridge_ops, pp); if (IS_ERR(bridge)) { err = PTR_ERR(bridge); goto bridge_create_fail; }
err = myhostbridge_setup(bridge->bus); if (err) goto bridge_setup_fail;
/* We always enable PCI domains and we keep domain 0 backward * compatible in /proc for video cards */ pci_add_flags(PCI_ENABLE_PROC_DOMAINS); pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC);
lastbus = pci_scan_child_bus(bridge->bus); pci_bus_update_busn_res_end(bridge->bus, lastbus);
pci_assign_unassigned_bus_resources(bridge->bus);
pci_bus_add_devices(bridge->bus);
return 0;
bridge_setup_fail: put_device(&bridge->dev); device_unregister(&bridge->dev); bridge_create_fail: kfree(pp); return err; }
[1] http://thread.gmane.org/gmane.linux.kernel.pci/25946
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: Use parent domain number when allocating child busses. pci: Add support for creating a generic host_bridge from device tree
drivers/of/address.c | 39 +++++++++++++ drivers/pci/host-bridge.c | 139 +++++++++++++++++++++++++++++++++++++++++++++ drivers/pci/probe.c | 72 +++++++++++++++-------- include/linux/of_address.h | 14 +---- include/linux/pci.h | 17 ++++++ 5 files changed, 246 insertions(+), 35 deletions(-)
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
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,
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
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);
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
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 6e34498..78ccba0 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,21 @@ 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) { + kfree(bridge); + return NULL; + } + b = pci_alloc_bus(); if (!b) - return NULL; + goto err_out;
b->sysdata = sysdata; b->ops = ops; @@ -1738,26 +1749,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 +1814,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; }
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
--- Arnd, I know you have Acked the previous version of this patch, but now I have added ERR_PTR() calls to it. Do I get to keep the Ack?
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 78ccba0..26237a0 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,14 +1729,15 @@ 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) { kfree(bridge); - return NULL; + return ERR_PTR(error); }
b = pci_alloc_bus(); @@ -1745,7 +1747,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, 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"); @@ -1753,7 +1755,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, }
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); @@ -1768,7 +1770,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; @@ -1818,7 +1820,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);
pci_alloc_child_bus() uses the newly allocated child bus to figure out the domain number that is going to use for setting the device name. A better option is to use the parent bus domain number.
Signed-off-by: Liviu Dudau Liviu.Dudau@arm.com
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 26237a0..a12cda5 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -677,7 +677,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, * now as the parent is not properly set up yet. */ child->dev.class = &pcibus_class; - dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr); + dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(parent), busnr);
/* * Set up the primary, secondary and subordinate
Hello Liviu,
Thanks for fixing up domain_nr. Now I have moved on further to a new domain_nr related warning dump :-)
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- xgene-pcie 1f500000.pcie: (rc) x8 gen-3 link up pci_bus 0001:00: scanning bus pci_setup_device:1101 domain_nr = 1 pci 0001:00:00.0: [e008:e004] type 01 class 0x060400 pci 0001:00:00.0: reg 0x10: [mem 0x4000000000-0x7fffffffff 64bit] pci 0001:00:00.0: calling xgene_pcie_fixup_bridge+0x0/0x80 pci 0001:00:00.0: Hiding X-Gene pci host bridge resources 0001:00:00.0 pci 0001:00:00.0: supports D1 D2 pci_bus 0001:00: fixups for bus pci 0001:00:00.0: scanning [bus 03-03] behind bridge, pass 0 pci 0001:00:00.0: bridge configuration invalid ([bus 03-03]), reconfiguring pci 0001:00:00.0: scanning [bus 00-00] behind bridge, pass 1 ** pci_scan_bridge:855 pci_domain_nr(bus) = 1 ** pci_alloc_child_bus:681 pci_domain_nr(bus) = 1 pci_bus 0001:01: scanning bus pci_setup_device:1101 domain_nr = 0 pci 0000:01:00.0: [15b3:1003] type 00 class 0x020000 pci 0000:01:00.0: reg 0x10: [mem 0x00100000-0x001fffff 64bit] pci 0000:01:00.0: reg 0x18: [mem 0x00800000-0x00ffffff 64bit pref] pci 0000:01:00.0: reg 0x30: [mem 0x01000000-0x010fffff pref] ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at /home/tinamdar/work/open-source/linux/fs/sysfs/dir.c:52 sysfs_warn_dup+0x80/0xc0() sysfs: cannot create duplicate filename '/bus/pci/devices/0000:01:00.0' Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.0-rc5+ #40 Call trace: [<ffffffc000088180>] dump_backtrace+0x0/0x140 [<ffffffc0000882d0>] show_stack+0x10/0x20 [<ffffffc0004f65ac>] dump_stack+0x74/0xc4 [<ffffffc000096e04>] warn_slowpath_common+0x84/0xc0 [<ffffffc000096e8c>] warn_slowpath_fmt+0x4c/0x60 [<ffffffc0001b83dc>] sysfs_warn_dup+0x7c/0xc0 [<ffffffc0001b8894>] sysfs_do_create_link_sd.isra.2+0xf4/0x100 [<ffffffc0001b88bc>] sysfs_create_link+0x1c/0x40 [<ffffffc0003250b0>] bus_add_device+0x110/0x1c0 [<ffffffc000322f1c>] device_add+0x31c/0x520 [<ffffffc0002c444c>] pci_device_add+0xec/0x140 [<ffffffc0004f1758>] pci_scan_single_device+0x98/0xe0 [<ffffffc0002c44e8>] pci_scan_slot+0x48/0x120 [<ffffffc0002c5368>] pci_scan_child_bus+0x48/0x140 [<ffffffc0002c522c>] pci_scan_bridge+0x4ec/0x5e0 [<ffffffc0002c53c8>] pci_scan_child_bus+0xa8/0x140 [<ffffffc0004f1b30>] pci_rescan_bus+0x10/0x40 [<ffffffc0006a12ac>] xgene_pcie_probe_bridge+0x660/0x72c [<ffffffc000327d20>] platform_drv_probe+0x20/0x60 [<ffffffc000325e30>] really_probe+0xf0/0x220 [<ffffffc000326080>] __driver_attach+0xa0/0xc0 [<ffffffc000323ed4>] bus_for_each_dev+0x54/0xa0 [<ffffffc0003258bc>] driver_attach+0x1c/0x40 [<ffffffc00032548c>] bus_add_driver+0x14c/0x220 [<ffffffc00032683c>] driver_register+0x5c/0x120 [<ffffffc000327cdc>] __platform_driver_register+0x5c/0x80 [<ffffffc0006a0c40>] xgene_pcie_driver_init+0x14/0x20 [<ffffffc0000814c0>] do_one_initcall+0xe0/0x160 [<ffffffc00068c934>] kernel_init_freeable+0x134/0x1d8 [<ffffffc0004f07ac>] kernel_init+0xc/0xe0 ---[ end trace 3ee052d463aab7f3 ]--- ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at /home/tinamdar/work/open-source/linux/drivers/pci/probe.c:1380 pci_device_add+0x128/0x140() -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
I have made a small fix above your patch. After the fix is applied, dumps are gone and the enumeration finishes up smoothly for all the ports. Since the change is small, just pasting it here. Please review and apply if it's clean.
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index a12cda5..aac8366 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -693,7 +693,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, }
child->self = bridge; - child->bridge = get_device(&bridge->dev); + child->bridge = get_device(parent->bridge); child->dev.parent = child->bridge; pci_set_bus_of_node(child); pci_set_bus_speed(child);
Thanks, Tanmay
On Mon, Mar 3, 2014 at 9:33 AM, Liviu Dudau Liviu.Dudau@arm.com wrote:
pci_alloc_child_bus() uses the newly allocated child bus to figure out the domain number that is going to use for setting the device name. A better option is to use the parent bus domain number.
Signed-off-by: Liviu Dudau Liviu.Dudau@arm.com
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 26237a0..a12cda5 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -677,7 +677,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, * now as the parent is not properly set up yet. */ child->dev.class = &pcibus_class;
dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);
dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(parent), busnr); /* * Set up the primary, secondary and subordinate-- 1.9.0
On Mon, Mar 03, 2014 at 03:14:47PM -0800, Tanmay Inamdar wrote:
Hello Liviu,
Thanks for fixing up domain_nr. Now I have moved on further to a new domain_nr related warning dump :-)
xgene-pcie 1f500000.pcie: (rc) x8 gen-3 link up pci_bus 0001:00: scanning bus pci_setup_device:1101 domain_nr = 1 pci 0001:00:00.0: [e008:e004] type 01 class 0x060400 pci 0001:00:00.0: reg 0x10: [mem 0x4000000000-0x7fffffffff 64bit] pci 0001:00:00.0: calling xgene_pcie_fixup_bridge+0x0/0x80 pci 0001:00:00.0: Hiding X-Gene pci host bridge resources 0001:00:00.0 pci 0001:00:00.0: supports D1 D2 pci_bus 0001:00: fixups for bus pci 0001:00:00.0: scanning [bus 03-03] behind bridge, pass 0 pci 0001:00:00.0: bridge configuration invalid ([bus 03-03]), reconfiguring pci 0001:00:00.0: scanning [bus 00-00] behind bridge, pass 1 ** pci_scan_bridge:855 pci_domain_nr(bus) = 1 ** pci_alloc_child_bus:681 pci_domain_nr(bus) = 1 pci_bus 0001:01: scanning bus pci_setup_device:1101 domain_nr = 0
Why does the domain_nr change here?
pci 0000:01:00.0: [15b3:1003] type 00 class 0x020000 pci 0000:01:00.0: reg 0x10: [mem 0x00100000-0x001fffff 64bit] pci 0000:01:00.0: reg 0x18: [mem 0x00800000-0x00ffffff 64bit pref] pci 0000:01:00.0: reg 0x30: [mem 0x01000000-0x010fffff pref] ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at /home/tinamdar/work/open-source/linux/fs/sysfs/dir.c:52 sysfs_warn_dup+0x80/0xc0() sysfs: cannot create duplicate filename '/bus/pci/devices/0000:01:00.0' Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.0-rc5+ #40 Call trace: [<ffffffc000088180>] dump_backtrace+0x0/0x140 [<ffffffc0000882d0>] show_stack+0x10/0x20 [<ffffffc0004f65ac>] dump_stack+0x74/0xc4 [<ffffffc000096e04>] warn_slowpath_common+0x84/0xc0 [<ffffffc000096e8c>] warn_slowpath_fmt+0x4c/0x60 [<ffffffc0001b83dc>] sysfs_warn_dup+0x7c/0xc0 [<ffffffc0001b8894>] sysfs_do_create_link_sd.isra.2+0xf4/0x100 [<ffffffc0001b88bc>] sysfs_create_link+0x1c/0x40 [<ffffffc0003250b0>] bus_add_device+0x110/0x1c0 [<ffffffc000322f1c>] device_add+0x31c/0x520 [<ffffffc0002c444c>] pci_device_add+0xec/0x140 [<ffffffc0004f1758>] pci_scan_single_device+0x98/0xe0 [<ffffffc0002c44e8>] pci_scan_slot+0x48/0x120 [<ffffffc0002c5368>] pci_scan_child_bus+0x48/0x140 [<ffffffc0002c522c>] pci_scan_bridge+0x4ec/0x5e0 [<ffffffc0002c53c8>] pci_scan_child_bus+0xa8/0x140 [<ffffffc0004f1b30>] pci_rescan_bus+0x10/0x40 [<ffffffc0006a12ac>] xgene_pcie_probe_bridge+0x660/0x72c [<ffffffc000327d20>] platform_drv_probe+0x20/0x60 [<ffffffc000325e30>] really_probe+0xf0/0x220 [<ffffffc000326080>] __driver_attach+0xa0/0xc0 [<ffffffc000323ed4>] bus_for_each_dev+0x54/0xa0 [<ffffffc0003258bc>] driver_attach+0x1c/0x40 [<ffffffc00032548c>] bus_add_driver+0x14c/0x220 [<ffffffc00032683c>] driver_register+0x5c/0x120 [<ffffffc000327cdc>] __platform_driver_register+0x5c/0x80 [<ffffffc0006a0c40>] xgene_pcie_driver_init+0x14/0x20 [<ffffffc0000814c0>] do_one_initcall+0xe0/0x160 [<ffffffc00068c934>] kernel_init_freeable+0x134/0x1d8 [<ffffffc0004f07ac>] kernel_init+0xc/0xe0 ---[ end trace 3ee052d463aab7f3 ]--- ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at /home/tinamdar/work/open-source/linux/drivers/pci/probe.c:1380 pci_device_add+0x128/0x140()
I have made a small fix above your patch. After the fix is applied, dumps are gone and the enumeration finishes up smoothly for all the ports. Since the change is small, just pasting it here. Please review and apply if it's clean.
Honestly, I have no idea. I kept staring at the code for a better part of an hour trying to decipher what the intent of the code was, without too much progress. I still don't understand why the code in pci_alloc_child_bus() takes a shortcut when the bridge argument is NULL when in my opinion it should use parent->bridge instead and continue as normal.
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index a12cda5..aac8366 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -693,7 +693,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, }
child->self = bridge;
child->bridge = get_device(&bridge->dev);
child->bridge = get_device(parent->bridge); child->dev.parent = child->bridge;
Hmm, not sure why this is needed. What does get_device(&bridge->dev) return for you? The next line sets child->dev.parent to child->bridge, but with your change I'm not sure we end up using the correct parent.
Can you try to revert your change and modify the implementation of pci_domain_nr() in arm64 to look like this:
static inline int pci_domain_nr(struct pci_bus *bus) { struct pci_host_bridge *bridge;
while (bus->parent) bus = bus->parent;
bridge = to_pci_host_bridge(bus->bridge); if (bridge) return bridge->domain_nr;
return 0; }
Please let me know what results you get.
Best regards, Liviu
pci_set_bus_of_node(child); pci_set_bus_speed(child);Thanks, Tanmay
On Mon, Mar 3, 2014 at 9:33 AM, Liviu Dudau Liviu.Dudau@arm.com wrote:
pci_alloc_child_bus() uses the newly allocated child bus to figure out the domain number that is going to use for setting the device name. A better option is to use the parent bus domain number.
Signed-off-by: Liviu Dudau Liviu.Dudau@arm.com
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 26237a0..a12cda5 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -677,7 +677,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, * now as the parent is not properly set up yet. */ child->dev.class = &pcibus_class;
dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);
dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(parent), busnr); /* * Set up the primary, secondary and subordinate-- 1.9.0
-- 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
Hello,
Please see inline.
On Mon, Mar 3, 2014 at 3:51 PM, Liviu Dudau liviu@dudau.co.uk wrote:
On Mon, Mar 03, 2014 at 03:14:47PM -0800, Tanmay Inamdar wrote:
Hello Liviu,
Thanks for fixing up domain_nr. Now I have moved on further to a new domain_nr related warning dump :-)
xgene-pcie 1f500000.pcie: (rc) x8 gen-3 link up pci_bus 0001:00: scanning bus pci_setup_device:1101 domain_nr = 1 pci 0001:00:00.0: [e008:e004] type 01 class 0x060400 pci 0001:00:00.0: reg 0x10: [mem 0x4000000000-0x7fffffffff 64bit] pci 0001:00:00.0: calling xgene_pcie_fixup_bridge+0x0/0x80 pci 0001:00:00.0: Hiding X-Gene pci host bridge resources 0001:00:00.0 pci 0001:00:00.0: supports D1 D2 pci_bus 0001:00: fixups for bus pci 0001:00:00.0: scanning [bus 03-03] behind bridge, pass 0 pci 0001:00:00.0: bridge configuration invalid ([bus 03-03]), reconfiguring pci 0001:00:00.0: scanning [bus 00-00] behind bridge, pass 1 ** pci_scan_bridge:855 pci_domain_nr(bus) = 1 ** pci_alloc_child_bus:681 pci_domain_nr(bus) = 1 pci_bus 0001:01: scanning bus pci_setup_device:1101 domain_nr = 0
Why does the domain_nr change here?
The bridge device pointer for parent and child should be same right? I think this is not the case here. Please look at the log at the bottom that I captured after trying your suggestions.
pci 0000:01:00.0: [15b3:1003] type 00 class 0x020000 pci 0000:01:00.0: reg 0x10: [mem 0x00100000-0x001fffff 64bit] pci 0000:01:00.0: reg 0x18: [mem 0x00800000-0x00ffffff 64bit pref] pci 0000:01:00.0: reg 0x30: [mem 0x01000000-0x010fffff pref] ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at /home/tinamdar/work/open-source/linux/fs/sysfs/dir.c:52 sysfs_warn_dup+0x80/0xc0() sysfs: cannot create duplicate filename '/bus/pci/devices/0000:01:00.0' Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.0-rc5+ #40 Call trace: [<ffffffc000088180>] dump_backtrace+0x0/0x140 [<ffffffc0000882d0>] show_stack+0x10/0x20 [<ffffffc0004f65ac>] dump_stack+0x74/0xc4 [<ffffffc000096e04>] warn_slowpath_common+0x84/0xc0 [<ffffffc000096e8c>] warn_slowpath_fmt+0x4c/0x60 [<ffffffc0001b83dc>] sysfs_warn_dup+0x7c/0xc0 [<ffffffc0001b8894>] sysfs_do_create_link_sd.isra.2+0xf4/0x100 [<ffffffc0001b88bc>] sysfs_create_link+0x1c/0x40 [<ffffffc0003250b0>] bus_add_device+0x110/0x1c0 [<ffffffc000322f1c>] device_add+0x31c/0x520 [<ffffffc0002c444c>] pci_device_add+0xec/0x140 [<ffffffc0004f1758>] pci_scan_single_device+0x98/0xe0 [<ffffffc0002c44e8>] pci_scan_slot+0x48/0x120 [<ffffffc0002c5368>] pci_scan_child_bus+0x48/0x140 [<ffffffc0002c522c>] pci_scan_bridge+0x4ec/0x5e0 [<ffffffc0002c53c8>] pci_scan_child_bus+0xa8/0x140 [<ffffffc0004f1b30>] pci_rescan_bus+0x10/0x40 [<ffffffc0006a12ac>] xgene_pcie_probe_bridge+0x660/0x72c [<ffffffc000327d20>] platform_drv_probe+0x20/0x60 [<ffffffc000325e30>] really_probe+0xf0/0x220 [<ffffffc000326080>] __driver_attach+0xa0/0xc0 [<ffffffc000323ed4>] bus_for_each_dev+0x54/0xa0 [<ffffffc0003258bc>] driver_attach+0x1c/0x40 [<ffffffc00032548c>] bus_add_driver+0x14c/0x220 [<ffffffc00032683c>] driver_register+0x5c/0x120 [<ffffffc000327cdc>] __platform_driver_register+0x5c/0x80 [<ffffffc0006a0c40>] xgene_pcie_driver_init+0x14/0x20 [<ffffffc0000814c0>] do_one_initcall+0xe0/0x160 [<ffffffc00068c934>] kernel_init_freeable+0x134/0x1d8 [<ffffffc0004f07ac>] kernel_init+0xc/0xe0 ---[ end trace 3ee052d463aab7f3 ]--- ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at /home/tinamdar/work/open-source/linux/drivers/pci/probe.c:1380 pci_device_add+0x128/0x140()
I have made a small fix above your patch. After the fix is applied, dumps are gone and the enumeration finishes up smoothly for all the ports. Since the change is small, just pasting it here. Please review and apply if it's clean.
Honestly, I have no idea. I kept staring at the code for a better part of an hour trying to decipher what the intent of the code was, without too much progress. I still don't understand why the code in pci_alloc_child_bus() takes a shortcut when the bridge argument is NULL when in my opinion it should use parent->bridge instead and continue as normal.
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index a12cda5..aac8366 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -693,7 +693,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, }
child->self = bridge;
child->bridge = get_device(&bridge->dev);
child->bridge = get_device(parent->bridge); child->dev.parent = child->bridge;Hmm, not sure why this is needed. What does get_device(&bridge->dev) return for you? The next line sets child->dev.parent to child->bridge, but with your change I'm not sure we end up using the correct parent.
Can you try to revert your change and modify the implementation of pci_domain_nr() in arm64 to look like this:
static inline int pci_domain_nr(struct pci_bus *bus) { struct pci_host_bridge *bridge;
while (bus->parent) bus = bus->parent; bridge = to_pci_host_bridge(bus->bridge); if (bridge) return bridge->domain_nr; return 0;}
This did not work for me.
Please let me know what results you get.
I am printing following values
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index a12cda5..c89f86a 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -695,6 +695,8 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, child->self = bridge; child->bridge = get_device(&bridge->dev); child->dev.parent = child->bridge; + printk("%s:%d bus = %p , bridge_dev = %p, domain = %d\n", + __func__, __LINE__, child, child->bridge, pci_domain_nr(parent)); pci_set_bus_of_node(child); pci_set_bus_speed(child);
@@ -1095,6 +1097,8 @@ int pci_setup_device(struct pci_dev *dev) dev_set_name(&dev->dev, "%04x:%02x:%02x.%d", pci_domain_nr(dev->bus), dev->bus->number, PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn)); + printk("%s:%d bus = %p , bridge_dev = %p, domain = %d\n", + __func__, __LINE__, dev->bus, dev->bus->bridge, pci_domain_nr(dev->bus));
pci_read_config_dword(dev, PCI_CLASS_REVISION, &class); dev->revision = class & 0xff;
Following looks suspicious to me.
bridge_dev = ffffffc7e03ffc00 for bus 0 in domain 1 while bridge_dev = ffffffc7e03f7098 for bus 1 in domain 1.
Log --> ----------------------------------------------------------------------------------------------------------------------------- xgene-pcie 1f500000.pcie: (rc) x8 gen-3 link up pci_bus 0001:00: scanning bus pci_setup_device:1101 bus = ffffffc7e0060400 , bridge_dev = ffffffc7e03ffc00, domain = 1 pci 0001:00:00.0: [e008:e004] type 01 class 0x060400 pci 0001:00:00.0: reg 0x10: [mem 0x4000000000-0x7fffffffff 64bit] pci 0001:00:00.0: calling xgene_pcie_fixup_bridge+0x0/0x80 pci 0001:00:00.0: Hiding X-Gene pci host bridge resources 0001:00:00.0 pci 0001:00:00.0: supports D1 D2 pci_bus 0001:00: fixups for bus pci 0001:00:00.0: scanning [bus 03-03] behind bridge, pass 0 pci 0001:00:00.0: bridge configuration invalid ([bus 03-03]), reconfiguring pci 0001:00:00.0: scanning [bus 00-00] behind bridge, pass 1 pci_alloc_child_bus:699 bus = ffffffc7e0063000 , bridge_dev = ffffffc7e03f7098, domain = 1 pci_bus 0001:01: scanning bus pci_setup_device:1101 bus = ffffffc7e0063000 , bridge_dev = ffffffc7e03f7098, domain = 0 pci 0000:01:00.0: [15b3:1003] type 00 class 0x020000 pci 0000:01:00.0: reg 0x10: [mem 0x00100000-0x001fffff 64bit] pci 0000:01:00.0: reg 0x18: [mem 0x00800000-0x00ffffff 64bit pref] pci 0000:01:00.0: reg 0x30: [mem 0x01000000-0x010fffff pref] ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at /home/tinamdar/work/open-source/linux/fs/sysfs/dir.c:52 sysfs_warn_dup+0x80/0xc0() sysfs: cannot create duplicate filename '/bus/pci/devices/0000:01:00.0' Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.0-rc5+ #50 -----------------------------------------------------------------------------------------------------------------------------
Best regards, Liviu
pci_set_bus_of_node(child); pci_set_bus_speed(child);Thanks, Tanmay
On Mon, Mar 3, 2014 at 9:33 AM, Liviu Dudau Liviu.Dudau@arm.com wrote:
pci_alloc_child_bus() uses the newly allocated child bus to figure out the domain number that is going to use for setting the device name. A better option is to use the parent bus domain number.
Signed-off-by: Liviu Dudau Liviu.Dudau@arm.com
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 26237a0..a12cda5 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -677,7 +677,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, * now as the parent is not properly set up yet. */ child->dev.class = &pcibus_class;
dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);
dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(parent), busnr); /* * Set up the primary, secondary and subordinate-- 1.9.0
-- 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
--
.oooO ( ) \ ( Oooo. _) ( ) ) / (_/
One small step for me ...
On Mon, Mar 03, 2014 at 04:42:11PM -0800, Tanmay Inamdar wrote:
Hello,
Please see inline.
On Mon, Mar 3, 2014 at 3:51 PM, Liviu Dudau liviu@dudau.co.uk wrote:
On Mon, Mar 03, 2014 at 03:14:47PM -0800, Tanmay Inamdar wrote:
Hello Liviu,
Thanks for fixing up domain_nr. Now I have moved on further to a new domain_nr related warning dump :-)
xgene-pcie 1f500000.pcie: (rc) x8 gen-3 link up pci_bus 0001:00: scanning bus pci_setup_device:1101 domain_nr = 1 pci 0001:00:00.0: [e008:e004] type 01 class 0x060400 pci 0001:00:00.0: reg 0x10: [mem 0x4000000000-0x7fffffffff 64bit] pci 0001:00:00.0: calling xgene_pcie_fixup_bridge+0x0/0x80 pci 0001:00:00.0: Hiding X-Gene pci host bridge resources 0001:00:00.0 pci 0001:00:00.0: supports D1 D2 pci_bus 0001:00: fixups for bus pci 0001:00:00.0: scanning [bus 03-03] behind bridge, pass 0 pci 0001:00:00.0: bridge configuration invalid ([bus 03-03]), reconfiguring pci 0001:00:00.0: scanning [bus 00-00] behind bridge, pass 1 ** pci_scan_bridge:855 pci_domain_nr(bus) = 1 ** pci_alloc_child_bus:681 pci_domain_nr(bus) = 1 pci_bus 0001:01: scanning bus pci_setup_device:1101 domain_nr = 0
Why does the domain_nr change here?
The bridge device pointer for parent and child should be same right? I think this is not the case here. Please look at the log at the bottom that I captured after trying your suggestions.
pci 0000:01:00.0: [15b3:1003] type 00 class 0x020000 pci 0000:01:00.0: reg 0x10: [mem 0x00100000-0x001fffff 64bit] pci 0000:01:00.0: reg 0x18: [mem 0x00800000-0x00ffffff 64bit pref] pci 0000:01:00.0: reg 0x30: [mem 0x01000000-0x010fffff pref] ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at /home/tinamdar/work/open-source/linux/fs/sysfs/dir.c:52 sysfs_warn_dup+0x80/0xc0() sysfs: cannot create duplicate filename '/bus/pci/devices/0000:01:00.0' Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.0-rc5+ #40 Call trace: [<ffffffc000088180>] dump_backtrace+0x0/0x140 [<ffffffc0000882d0>] show_stack+0x10/0x20 [<ffffffc0004f65ac>] dump_stack+0x74/0xc4 [<ffffffc000096e04>] warn_slowpath_common+0x84/0xc0 [<ffffffc000096e8c>] warn_slowpath_fmt+0x4c/0x60 [<ffffffc0001b83dc>] sysfs_warn_dup+0x7c/0xc0 [<ffffffc0001b8894>] sysfs_do_create_link_sd.isra.2+0xf4/0x100 [<ffffffc0001b88bc>] sysfs_create_link+0x1c/0x40 [<ffffffc0003250b0>] bus_add_device+0x110/0x1c0 [<ffffffc000322f1c>] device_add+0x31c/0x520 [<ffffffc0002c444c>] pci_device_add+0xec/0x140 [<ffffffc0004f1758>] pci_scan_single_device+0x98/0xe0 [<ffffffc0002c44e8>] pci_scan_slot+0x48/0x120 [<ffffffc0002c5368>] pci_scan_child_bus+0x48/0x140 [<ffffffc0002c522c>] pci_scan_bridge+0x4ec/0x5e0 [<ffffffc0002c53c8>] pci_scan_child_bus+0xa8/0x140 [<ffffffc0004f1b30>] pci_rescan_bus+0x10/0x40 [<ffffffc0006a12ac>] xgene_pcie_probe_bridge+0x660/0x72c [<ffffffc000327d20>] platform_drv_probe+0x20/0x60 [<ffffffc000325e30>] really_probe+0xf0/0x220 [<ffffffc000326080>] __driver_attach+0xa0/0xc0 [<ffffffc000323ed4>] bus_for_each_dev+0x54/0xa0 [<ffffffc0003258bc>] driver_attach+0x1c/0x40 [<ffffffc00032548c>] bus_add_driver+0x14c/0x220 [<ffffffc00032683c>] driver_register+0x5c/0x120 [<ffffffc000327cdc>] __platform_driver_register+0x5c/0x80 [<ffffffc0006a0c40>] xgene_pcie_driver_init+0x14/0x20 [<ffffffc0000814c0>] do_one_initcall+0xe0/0x160 [<ffffffc00068c934>] kernel_init_freeable+0x134/0x1d8 [<ffffffc0004f07ac>] kernel_init+0xc/0xe0 ---[ end trace 3ee052d463aab7f3 ]--- ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at /home/tinamdar/work/open-source/linux/drivers/pci/probe.c:1380 pci_device_add+0x128/0x140()
I have made a small fix above your patch. After the fix is applied, dumps are gone and the enumeration finishes up smoothly for all the ports. Since the change is small, just pasting it here. Please review and apply if it's clean.
Honestly, I have no idea. I kept staring at the code for a better part of an hour trying to decipher what the intent of the code was, without too much progress. I still don't understand why the code in pci_alloc_child_bus() takes a shortcut when the bridge argument is NULL when in my opinion it should use parent->bridge instead and continue as normal.
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index a12cda5..aac8366 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -693,7 +693,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, }
child->self = bridge;
child->bridge = get_device(&bridge->dev);
child->bridge = get_device(parent->bridge); child->dev.parent = child->bridge;Hmm, not sure why this is needed. What does get_device(&bridge->dev) return for you? The next line sets child->dev.parent to child->bridge, but with your change I'm not sure we end up using the correct parent.
Can you try to revert your change and modify the implementation of pci_domain_nr() in arm64 to look like this:
static inline int pci_domain_nr(struct pci_bus *bus) { struct pci_host_bridge *bridge;
while (bus->parent) bus = bus->parent; bridge = to_pci_host_bridge(bus->bridge); if (bridge) return bridge->domain_nr; return 0;}
This did not work for me.
Please let me know what results you get.
I am printing following values
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index a12cda5..c89f86a 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -695,6 +695,8 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, child->self = bridge; child->bridge = get_device(&bridge->dev); child->dev.parent = child->bridge;
printk("%s:%d bus = %p , bridge_dev = %p, domain = %d\n",__func__, __LINE__, child, child->bridge,pci_domain_nr(parent));
I see that you are using parent here.
pci_set_bus_of_node(child); pci_set_bus_speed(child);@@ -1095,6 +1097,8 @@ int pci_setup_device(struct pci_dev *dev) dev_set_name(&dev->dev, "%04x:%02x:%02x.%d", pci_domain_nr(dev->bus), dev->bus->number, PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
printk("%s:%d bus = %p , bridge_dev = %p, domain = %d\n",__func__, __LINE__, dev->bus, dev->bus->bridge,pci_domain_nr(dev->bus));
pci_read_config_dword(dev, PCI_CLASS_REVISION, &class); dev->revision = class & 0xff;Following looks suspicious to me.
bridge_dev = ffffffc7e03ffc00 for bus 0 in domain 1 while bridge_dev = ffffffc7e03f7098 for bus 1 in domain 1.
Log -->
xgene-pcie 1f500000.pcie: (rc) x8 gen-3 link up pci_bus 0001:00: scanning bus pci_setup_device:1101 bus = ffffffc7e0060400 , bridge_dev = ffffffc7e03ffc00, domain = 1
This is domain = 1, bus = 0, right?
pci 0001:00:00.0: [e008:e004] type 01 class 0x060400 pci 0001:00:00.0: reg 0x10: [mem 0x4000000000-0x7fffffffff 64bit] pci 0001:00:00.0: calling xgene_pcie_fixup_bridge+0x0/0x80 pci 0001:00:00.0: Hiding X-Gene pci host bridge resources 0001:00:00.0 pci 0001:00:00.0: supports D1 D2 pci_bus 0001:00: fixups for bus pci 0001:00:00.0: scanning [bus 03-03] behind bridge, pass 0 pci 0001:00:00.0: bridge configuration invalid ([bus 03-03]), reconfiguring pci 0001:00:00.0: scanning [bus 00-00] behind bridge, pass 1 pci_alloc_child_bus:699 bus = ffffffc7e0063000 , bridge_dev = ffffffc7e03f7098, domain = 1
This is domain = 1, bus = 1, correct?
pci_bus 0001:01: scanning bus pci_setup_device:1101 bus = ffffffc7e0063000 , bridge_dev = ffffffc7e03f7098, domain = 0
Then this is ... ? bus pointer matches 0001:01, same for bridge_dev, but suddenly domain is 0 ? That means that when the child was created, the bridge->dev was not setup? (in which case your patch would make sense).
It would be really helpful if you could add more tracing messages so that we can figure out why the domain ends up being zero here.
Many thanks, Liviu
pci 0000:01:00.0: [15b3:1003] type 00 class 0x020000 pci 0000:01:00.0: reg 0x10: [mem 0x00100000-0x001fffff 64bit] pci 0000:01:00.0: reg 0x18: [mem 0x00800000-0x00ffffff 64bit pref] pci 0000:01:00.0: reg 0x30: [mem 0x01000000-0x010fffff pref] ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at /home/tinamdar/work/open-source/linux/fs/sysfs/dir.c:52 sysfs_warn_dup+0x80/0xc0() sysfs: cannot create duplicate filename '/bus/pci/devices/0000:01:00.0' Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.0-rc5+ #50
Best regards, Liviu
pci_set_bus_of_node(child); pci_set_bus_speed(child);Thanks, Tanmay
On Mon, Mar 3, 2014 at 9:33 AM, Liviu Dudau Liviu.Dudau@arm.com wrote:
pci_alloc_child_bus() uses the newly allocated child bus to figure out the domain number that is going to use for setting the device name. A better option is to use the parent bus domain number.
Signed-off-by: Liviu Dudau Liviu.Dudau@arm.com
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 26237a0..a12cda5 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -677,7 +677,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, * now as the parent is not properly set up yet. */ child->dev.class = &pcibus_class;
dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);
dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(parent), busnr); /* * Set up the primary, secondary and subordinate-- 1.9.0
-- 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
--
.oooO ( ) \ ( Oooo. _) ( ) ) / (_/
One small step for me ...
-- 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
Hi Liviu,
Actually your suggested implementation of pci_domain_nr is working perfectly. My bad for earlier email. I did not implement it correctly.
Thanks, Tanmay
On Mon, Mar 3, 2014 at 4:42 PM, Tanmay Inamdar tinamdar@apm.com wrote:
Hello,
Please see inline.
On Mon, Mar 3, 2014 at 3:51 PM, Liviu Dudau liviu@dudau.co.uk wrote:
On Mon, Mar 03, 2014 at 03:14:47PM -0800, Tanmay Inamdar wrote:
Hello Liviu,
Thanks for fixing up domain_nr. Now I have moved on further to a new domain_nr related warning dump :-)
xgene-pcie 1f500000.pcie: (rc) x8 gen-3 link up pci_bus 0001:00: scanning bus pci_setup_device:1101 domain_nr = 1 pci 0001:00:00.0: [e008:e004] type 01 class 0x060400 pci 0001:00:00.0: reg 0x10: [mem 0x4000000000-0x7fffffffff 64bit] pci 0001:00:00.0: calling xgene_pcie_fixup_bridge+0x0/0x80 pci 0001:00:00.0: Hiding X-Gene pci host bridge resources 0001:00:00.0 pci 0001:00:00.0: supports D1 D2 pci_bus 0001:00: fixups for bus pci 0001:00:00.0: scanning [bus 03-03] behind bridge, pass 0 pci 0001:00:00.0: bridge configuration invalid ([bus 03-03]), reconfiguring pci 0001:00:00.0: scanning [bus 00-00] behind bridge, pass 1 ** pci_scan_bridge:855 pci_domain_nr(bus) = 1 ** pci_alloc_child_bus:681 pci_domain_nr(bus) = 1 pci_bus 0001:01: scanning bus pci_setup_device:1101 domain_nr = 0
Why does the domain_nr change here?
The bridge device pointer for parent and child should be same right? I think this is not the case here. Please look at the log at the bottom that I captured after trying your suggestions.
pci 0000:01:00.0: [15b3:1003] type 00 class 0x020000 pci 0000:01:00.0: reg 0x10: [mem 0x00100000-0x001fffff 64bit] pci 0000:01:00.0: reg 0x18: [mem 0x00800000-0x00ffffff 64bit pref] pci 0000:01:00.0: reg 0x30: [mem 0x01000000-0x010fffff pref] ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at /home/tinamdar/work/open-source/linux/fs/sysfs/dir.c:52 sysfs_warn_dup+0x80/0xc0() sysfs: cannot create duplicate filename '/bus/pci/devices/0000:01:00.0' Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.0-rc5+ #40 Call trace: [<ffffffc000088180>] dump_backtrace+0x0/0x140 [<ffffffc0000882d0>] show_stack+0x10/0x20 [<ffffffc0004f65ac>] dump_stack+0x74/0xc4 [<ffffffc000096e04>] warn_slowpath_common+0x84/0xc0 [<ffffffc000096e8c>] warn_slowpath_fmt+0x4c/0x60 [<ffffffc0001b83dc>] sysfs_warn_dup+0x7c/0xc0 [<ffffffc0001b8894>] sysfs_do_create_link_sd.isra.2+0xf4/0x100 [<ffffffc0001b88bc>] sysfs_create_link+0x1c/0x40 [<ffffffc0003250b0>] bus_add_device+0x110/0x1c0 [<ffffffc000322f1c>] device_add+0x31c/0x520 [<ffffffc0002c444c>] pci_device_add+0xec/0x140 [<ffffffc0004f1758>] pci_scan_single_device+0x98/0xe0 [<ffffffc0002c44e8>] pci_scan_slot+0x48/0x120 [<ffffffc0002c5368>] pci_scan_child_bus+0x48/0x140 [<ffffffc0002c522c>] pci_scan_bridge+0x4ec/0x5e0 [<ffffffc0002c53c8>] pci_scan_child_bus+0xa8/0x140 [<ffffffc0004f1b30>] pci_rescan_bus+0x10/0x40 [<ffffffc0006a12ac>] xgene_pcie_probe_bridge+0x660/0x72c [<ffffffc000327d20>] platform_drv_probe+0x20/0x60 [<ffffffc000325e30>] really_probe+0xf0/0x220 [<ffffffc000326080>] __driver_attach+0xa0/0xc0 [<ffffffc000323ed4>] bus_for_each_dev+0x54/0xa0 [<ffffffc0003258bc>] driver_attach+0x1c/0x40 [<ffffffc00032548c>] bus_add_driver+0x14c/0x220 [<ffffffc00032683c>] driver_register+0x5c/0x120 [<ffffffc000327cdc>] __platform_driver_register+0x5c/0x80 [<ffffffc0006a0c40>] xgene_pcie_driver_init+0x14/0x20 [<ffffffc0000814c0>] do_one_initcall+0xe0/0x160 [<ffffffc00068c934>] kernel_init_freeable+0x134/0x1d8 [<ffffffc0004f07ac>] kernel_init+0xc/0xe0 ---[ end trace 3ee052d463aab7f3 ]--- ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at /home/tinamdar/work/open-source/linux/drivers/pci/probe.c:1380 pci_device_add+0x128/0x140()
I have made a small fix above your patch. After the fix is applied, dumps are gone and the enumeration finishes up smoothly for all the ports. Since the change is small, just pasting it here. Please review and apply if it's clean.
Honestly, I have no idea. I kept staring at the code for a better part of an hour trying to decipher what the intent of the code was, without too much progress. I still don't understand why the code in pci_alloc_child_bus() takes a shortcut when the bridge argument is NULL when in my opinion it should use parent->bridge instead and continue as normal.
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index a12cda5..aac8366 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -693,7 +693,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, }
child->self = bridge;
child->bridge = get_device(&bridge->dev);
child->bridge = get_device(parent->bridge); child->dev.parent = child->bridge;Hmm, not sure why this is needed. What does get_device(&bridge->dev) return for you? The next line sets child->dev.parent to child->bridge, but with your change I'm not sure we end up using the correct parent.
Can you try to revert your change and modify the implementation of pci_domain_nr() in arm64 to look like this:
static inline int pci_domain_nr(struct pci_bus *bus) { struct pci_host_bridge *bridge;
while (bus->parent) bus = bus->parent; bridge = to_pci_host_bridge(bus->bridge); if (bridge) return bridge->domain_nr; return 0;}
This did not work for me.
Please let me know what results you get.
I am printing following values
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index a12cda5..c89f86a 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -695,6 +695,8 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, child->self = bridge; child->bridge = get_device(&bridge->dev); child->dev.parent = child->bridge;
printk("%s:%d bus = %p , bridge_dev = %p, domain = %d\n",__func__, __LINE__, child, child->bridge,pci_domain_nr(parent)); pci_set_bus_of_node(child); pci_set_bus_speed(child);
@@ -1095,6 +1097,8 @@ int pci_setup_device(struct pci_dev *dev) dev_set_name(&dev->dev, "%04x:%02x:%02x.%d", pci_domain_nr(dev->bus), dev->bus->number, PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
printk("%s:%d bus = %p , bridge_dev = %p, domain = %d\n",__func__, __LINE__, dev->bus, dev->bus->bridge,pci_domain_nr(dev->bus));
pci_read_config_dword(dev, PCI_CLASS_REVISION, &class); dev->revision = class & 0xff;Following looks suspicious to me.
bridge_dev = ffffffc7e03ffc00 for bus 0 in domain 1 while bridge_dev = ffffffc7e03f7098 for bus 1 in domain 1.
Log -->
xgene-pcie 1f500000.pcie: (rc) x8 gen-3 link up pci_bus 0001:00: scanning bus pci_setup_device:1101 bus = ffffffc7e0060400 , bridge_dev = ffffffc7e03ffc00, domain = 1 pci 0001:00:00.0: [e008:e004] type 01 class 0x060400 pci 0001:00:00.0: reg 0x10: [mem 0x4000000000-0x7fffffffff 64bit] pci 0001:00:00.0: calling xgene_pcie_fixup_bridge+0x0/0x80 pci 0001:00:00.0: Hiding X-Gene pci host bridge resources 0001:00:00.0 pci 0001:00:00.0: supports D1 D2 pci_bus 0001:00: fixups for bus pci 0001:00:00.0: scanning [bus 03-03] behind bridge, pass 0 pci 0001:00:00.0: bridge configuration invalid ([bus 03-03]), reconfiguring pci 0001:00:00.0: scanning [bus 00-00] behind bridge, pass 1 pci_alloc_child_bus:699 bus = ffffffc7e0063000 , bridge_dev = ffffffc7e03f7098, domain = 1 pci_bus 0001:01: scanning bus pci_setup_device:1101 bus = ffffffc7e0063000 , bridge_dev = ffffffc7e03f7098, domain = 0 pci 0000:01:00.0: [15b3:1003] type 00 class 0x020000 pci 0000:01:00.0: reg 0x10: [mem 0x00100000-0x001fffff 64bit] pci 0000:01:00.0: reg 0x18: [mem 0x00800000-0x00ffffff 64bit pref] pci 0000:01:00.0: reg 0x30: [mem 0x01000000-0x010fffff pref] ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at /home/tinamdar/work/open-source/linux/fs/sysfs/dir.c:52 sysfs_warn_dup+0x80/0xc0() sysfs: cannot create duplicate filename '/bus/pci/devices/0000:01:00.0' Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.0-rc5+ #50
Best regards, Liviu
pci_set_bus_of_node(child); pci_set_bus_speed(child);Thanks, Tanmay
On Mon, Mar 3, 2014 at 9:33 AM, Liviu Dudau Liviu.Dudau@arm.com wrote:
pci_alloc_child_bus() uses the newly allocated child bus to figure out the domain number that is going to use for setting the device name. A better option is to use the parent bus domain number.
Signed-off-by: Liviu Dudau Liviu.Dudau@arm.com
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 26237a0..a12cda5 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -677,7 +677,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, * now as the parent is not properly set up yet. */ child->dev.class = &pcibus_class;
dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);
dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(parent), busnr); /* * Set up the primary, secondary and subordinate-- 1.9.0
-- 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
--
.oooO ( ) \ ( Oooo. _) ( ) ) / (_/
One small step for me ...
On Mon, Mar 03, 2014 at 05:42:04PM -0800, Tanmay Inamdar wrote:
Hi Liviu,
Actually your suggested implementation of pci_domain_nr is working perfectly. My bad for earlier email. I did not implement it correctly.
Great. I will send an updated series for arm64 tomorrow.
Best regards, Liviu
Thanks, Tanmay
On Mon, Mar 3, 2014 at 4:42 PM, Tanmay Inamdar tinamdar@apm.com wrote:
Hello,
Please see inline.
On Mon, Mar 3, 2014 at 3:51 PM, Liviu Dudau liviu@dudau.co.uk wrote:
On Mon, Mar 03, 2014 at 03:14:47PM -0800, Tanmay Inamdar wrote:
Hello Liviu,
Thanks for fixing up domain_nr. Now I have moved on further to a new domain_nr related warning dump :-)
xgene-pcie 1f500000.pcie: (rc) x8 gen-3 link up pci_bus 0001:00: scanning bus pci_setup_device:1101 domain_nr = 1 pci 0001:00:00.0: [e008:e004] type 01 class 0x060400 pci 0001:00:00.0: reg 0x10: [mem 0x4000000000-0x7fffffffff 64bit] pci 0001:00:00.0: calling xgene_pcie_fixup_bridge+0x0/0x80 pci 0001:00:00.0: Hiding X-Gene pci host bridge resources 0001:00:00.0 pci 0001:00:00.0: supports D1 D2 pci_bus 0001:00: fixups for bus pci 0001:00:00.0: scanning [bus 03-03] behind bridge, pass 0 pci 0001:00:00.0: bridge configuration invalid ([bus 03-03]), reconfiguring pci 0001:00:00.0: scanning [bus 00-00] behind bridge, pass 1 ** pci_scan_bridge:855 pci_domain_nr(bus) = 1 ** pci_alloc_child_bus:681 pci_domain_nr(bus) = 1 pci_bus 0001:01: scanning bus pci_setup_device:1101 domain_nr = 0
Why does the domain_nr change here?
The bridge device pointer for parent and child should be same right? I think this is not the case here. Please look at the log at the bottom that I captured after trying your suggestions.
pci 0000:01:00.0: [15b3:1003] type 00 class 0x020000 pci 0000:01:00.0: reg 0x10: [mem 0x00100000-0x001fffff 64bit] pci 0000:01:00.0: reg 0x18: [mem 0x00800000-0x00ffffff 64bit pref] pci 0000:01:00.0: reg 0x30: [mem 0x01000000-0x010fffff pref] ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at /home/tinamdar/work/open-source/linux/fs/sysfs/dir.c:52 sysfs_warn_dup+0x80/0xc0() sysfs: cannot create duplicate filename '/bus/pci/devices/0000:01:00.0' Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.0-rc5+ #40 Call trace: [<ffffffc000088180>] dump_backtrace+0x0/0x140 [<ffffffc0000882d0>] show_stack+0x10/0x20 [<ffffffc0004f65ac>] dump_stack+0x74/0xc4 [<ffffffc000096e04>] warn_slowpath_common+0x84/0xc0 [<ffffffc000096e8c>] warn_slowpath_fmt+0x4c/0x60 [<ffffffc0001b83dc>] sysfs_warn_dup+0x7c/0xc0 [<ffffffc0001b8894>] sysfs_do_create_link_sd.isra.2+0xf4/0x100 [<ffffffc0001b88bc>] sysfs_create_link+0x1c/0x40 [<ffffffc0003250b0>] bus_add_device+0x110/0x1c0 [<ffffffc000322f1c>] device_add+0x31c/0x520 [<ffffffc0002c444c>] pci_device_add+0xec/0x140 [<ffffffc0004f1758>] pci_scan_single_device+0x98/0xe0 [<ffffffc0002c44e8>] pci_scan_slot+0x48/0x120 [<ffffffc0002c5368>] pci_scan_child_bus+0x48/0x140 [<ffffffc0002c522c>] pci_scan_bridge+0x4ec/0x5e0 [<ffffffc0002c53c8>] pci_scan_child_bus+0xa8/0x140 [<ffffffc0004f1b30>] pci_rescan_bus+0x10/0x40 [<ffffffc0006a12ac>] xgene_pcie_probe_bridge+0x660/0x72c [<ffffffc000327d20>] platform_drv_probe+0x20/0x60 [<ffffffc000325e30>] really_probe+0xf0/0x220 [<ffffffc000326080>] __driver_attach+0xa0/0xc0 [<ffffffc000323ed4>] bus_for_each_dev+0x54/0xa0 [<ffffffc0003258bc>] driver_attach+0x1c/0x40 [<ffffffc00032548c>] bus_add_driver+0x14c/0x220 [<ffffffc00032683c>] driver_register+0x5c/0x120 [<ffffffc000327cdc>] __platform_driver_register+0x5c/0x80 [<ffffffc0006a0c40>] xgene_pcie_driver_init+0x14/0x20 [<ffffffc0000814c0>] do_one_initcall+0xe0/0x160 [<ffffffc00068c934>] kernel_init_freeable+0x134/0x1d8 [<ffffffc0004f07ac>] kernel_init+0xc/0xe0 ---[ end trace 3ee052d463aab7f3 ]--- ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at /home/tinamdar/work/open-source/linux/drivers/pci/probe.c:1380 pci_device_add+0x128/0x140()
I have made a small fix above your patch. After the fix is applied, dumps are gone and the enumeration finishes up smoothly for all the ports. Since the change is small, just pasting it here. Please review and apply if it's clean.
Honestly, I have no idea. I kept staring at the code for a better part of an hour trying to decipher what the intent of the code was, without too much progress. I still don't understand why the code in pci_alloc_child_bus() takes a shortcut when the bridge argument is NULL when in my opinion it should use parent->bridge instead and continue as normal.
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index a12cda5..aac8366 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -693,7 +693,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, }
child->self = bridge;
child->bridge = get_device(&bridge->dev);
child->bridge = get_device(parent->bridge); child->dev.parent = child->bridge;Hmm, not sure why this is needed. What does get_device(&bridge->dev) return for you? The next line sets child->dev.parent to child->bridge, but with your change I'm not sure we end up using the correct parent.
Can you try to revert your change and modify the implementation of pci_domain_nr() in arm64 to look like this:
static inline int pci_domain_nr(struct pci_bus *bus) { struct pci_host_bridge *bridge;
while (bus->parent) bus = bus->parent; bridge = to_pci_host_bridge(bus->bridge); if (bridge) return bridge->domain_nr; return 0;}
This did not work for me.
Please let me know what results you get.
I am printing following values
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index a12cda5..c89f86a 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -695,6 +695,8 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, child->self = bridge; child->bridge = get_device(&bridge->dev); child->dev.parent = child->bridge;
printk("%s:%d bus = %p , bridge_dev = %p, domain = %d\n",__func__, __LINE__, child, child->bridge,pci_domain_nr(parent)); pci_set_bus_of_node(child); pci_set_bus_speed(child);
@@ -1095,6 +1097,8 @@ int pci_setup_device(struct pci_dev *dev) dev_set_name(&dev->dev, "%04x:%02x:%02x.%d", pci_domain_nr(dev->bus), dev->bus->number, PCI_SLOT(dev->devfn), PCI_FUNC(dev->devfn));
printk("%s:%d bus = %p , bridge_dev = %p, domain = %d\n",__func__, __LINE__, dev->bus, dev->bus->bridge,pci_domain_nr(dev->bus));
pci_read_config_dword(dev, PCI_CLASS_REVISION, &class); dev->revision = class & 0xff;Following looks suspicious to me.
bridge_dev = ffffffc7e03ffc00 for bus 0 in domain 1 while bridge_dev = ffffffc7e03f7098 for bus 1 in domain 1.
Log -->
xgene-pcie 1f500000.pcie: (rc) x8 gen-3 link up pci_bus 0001:00: scanning bus pci_setup_device:1101 bus = ffffffc7e0060400 , bridge_dev = ffffffc7e03ffc00, domain = 1 pci 0001:00:00.0: [e008:e004] type 01 class 0x060400 pci 0001:00:00.0: reg 0x10: [mem 0x4000000000-0x7fffffffff 64bit] pci 0001:00:00.0: calling xgene_pcie_fixup_bridge+0x0/0x80 pci 0001:00:00.0: Hiding X-Gene pci host bridge resources 0001:00:00.0 pci 0001:00:00.0: supports D1 D2 pci_bus 0001:00: fixups for bus pci 0001:00:00.0: scanning [bus 03-03] behind bridge, pass 0 pci 0001:00:00.0: bridge configuration invalid ([bus 03-03]), reconfiguring pci 0001:00:00.0: scanning [bus 00-00] behind bridge, pass 1 pci_alloc_child_bus:699 bus = ffffffc7e0063000 , bridge_dev = ffffffc7e03f7098, domain = 1 pci_bus 0001:01: scanning bus pci_setup_device:1101 bus = ffffffc7e0063000 , bridge_dev = ffffffc7e03f7098, domain = 0 pci 0000:01:00.0: [15b3:1003] type 00 class 0x020000 pci 0000:01:00.0: reg 0x10: [mem 0x00100000-0x001fffff 64bit] pci 0000:01:00.0: reg 0x18: [mem 0x00800000-0x00ffffff 64bit pref] pci 0000:01:00.0: reg 0x30: [mem 0x01000000-0x010fffff pref] ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at /home/tinamdar/work/open-source/linux/fs/sysfs/dir.c:52 sysfs_warn_dup+0x80/0xc0() sysfs: cannot create duplicate filename '/bus/pci/devices/0000:01:00.0' Modules linked in: CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.14.0-rc5+ #50
Best regards, Liviu
pci_set_bus_of_node(child); pci_set_bus_speed(child);Thanks, Tanmay
On Mon, Mar 3, 2014 at 9:33 AM, Liviu Dudau Liviu.Dudau@arm.com wrote:
pci_alloc_child_bus() uses the newly allocated child bus to figure out the domain number that is going to use for setting the device name. A better option is to use the parent bus domain number.
Signed-off-by: Liviu Dudau Liviu.Dudau@arm.com
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 26237a0..a12cda5 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -677,7 +677,7 @@ static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent, * now as the parent is not properly set up yet. */ child->dev.class = &pcibus_class;
dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(child), busnr);
dev_set_name(&child->dev, "%04x:%02x", pci_domain_nr(parent), busnr); /* * Set up the primary, secondary and subordinate-- 1.9.0
-- 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
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
diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c index 06ace62..3c6cbd3 100644 --- a/drivers/pci/host-bridge.c +++ b/drivers/pci/host-bridge.c @@ -6,9 +6,13 @@ #include <linux/init.h> #include <linux/pci.h> #include <linux/module.h> +#include <linux/of_address.h> +#include <linux/of_pci.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) @@ -91,3 +95,138 @@ void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res, res->end = region->end + offset; } EXPORT_SYMBOL(pcibios_bus_to_resource); + +/** + * 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. + * + * If this function returns an error then the @resources list will be freed. + * + * 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) { + err = -ENOMEM; + goto bridge_ranges_nomem; + } + + 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 */ + pcibios_fixup_bridge_ranges(resources); + + return 0; + +bridge_ranges_nomem: + pci_free_resource_list(resources); + return err; +} + +/** + * 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) + return ERR_PTR(err); + + /* then create the root bus */ + root_bus = pci_create_root_bus_in_domain(parent, domain, busno, + ops, host_data, &res); + if (!root_bus) + return NULL; + + bridge = to_pci_host_bridge(root_bus->bridge); + bridge->io_base = io_base; + + return bridge; +} +EXPORT_SYMBOL_GPL(of_create_pci_host_bridge); diff --git a/include/linux/pci.h b/include/linux/pci.h index 1eed009..0c5e269 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); + +void 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 * +pci_host_bridge_of_init(struct device *parent, struct pci_ops *ops, + void *host_data) +{ + return NULL; +} #endif /* CONFIG_OF */
#ifdef CONFIG_EEH
linaro-kernel@lists.linaro.org
-
Liviu Dudau -
Liviu Dudau -
Tanmay Inamdar