| Message ID | 1393506402-11474-5-git-send-email-Liviu.Dudau@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thursday 27 February 2014 13:06:42 Liviu Dudau wrote: > 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> Please add Benjamin Herrenschmidt to Cc here, I think it would be helpful to get his input so we can make this work on powerpc as well. > diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c > index 06ace62..feb8436 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 int domain_nr; > + For correctness, I think you want an 'atomic_t' here and use atomic_inc_return() to get a new value. > static struct pci_bus *find_pci_root_bus(struct pci_bus *bus) > { > while (bus->parent) > @@ -91,3 +95,133 @@ 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); > + } This is not the correct resource for I/O space at all. Please talk to Will, I've been over this with him in detail and he probably understands it now. I assume you are both working in the same building. Since this is common PCI code, you could also decide to open-code the pci_add_resource_offset() function. If you don't do that, I think you have a memory leak for the resources that you can avoid by allocating the resource and pci_host_bridge_window structures together with a single kzalloc. > + /* 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); > + > + domain = of_alias_get_id(parent->of_node, "pci-domain"); > + if (domain == -ENODEV) > + domain = 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 */ > + if (pci_host_bridge_of_get_ranges(parent->of_node, &res, &io_base)) > + return NULL; > + > + /* 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; Do we have any code that checks for conflicting domain/bus numbers here? I guess pci_create_root_bus_in_domain() will fail if you have that. Since pci_create_root_bus_in_domain() is a new function that you just introduced, it would be helpful to change the calling conventions so it returns an error pointer instead of NULL upon failing. of_create_pci_host_bridge() can do the same, but pci_create_root_bus() should keep returning NULL so we don't have to change all the callers. > + 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; What is the io_base used for here? > @@ -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 >
On Thursday 27 February 2014 14:38:32 Arnd Bergmann wrote: > > + 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); > > + } > > This is not the correct resource for I/O space at all. Please talk > to Will, I've been over this with him in detail and he probably > understands it now. I assume you are both working in the same > building. > Sorry, I initially missed part of your changes in patch 1. I think it's actually correct now. Arnd
On Thu, 2014-02-27 at 14:38 +0100, Arnd Bergmann wrote: > On Thursday 27 February 2014 13:06:42 Liviu Dudau wrote: > > 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. So that is only going to work for archs that don't have any special twist. For example it won't work for powerpc which is why Andrew original approach didn't fly. The range walk helpers do help though, I need to review in more details and test Andrew powerpc patch here and will merge it. > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > > Please add Benjamin Herrenschmidt to Cc here, I think it would be helpful > to get his input so we can make this work on powerpc as well. Tricky... We would need hooks which would turn the whole thing into a pile of spaghetti. I think we should stick to using the range helpers (Andrew latest patch), which makes the powerpc code a lot smaller, and leave it at that. > > diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c > > index 06ace62..feb8436 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 int domain_nr; > > + > > For correctness, I think you want an 'atomic_t' here and use > atomic_inc_return() to get a new value. > > > static struct pci_bus *find_pci_root_bus(struct pci_bus *bus) > > { > > while (bus->parent) > > @@ -91,3 +95,133 @@ 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; Shouldn't that test move into the parsing helper ? > > + 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; You don't care about the size of the IO space ? > > + pci_add_resource_offset(resources, res, > > + res->start - range.pci_addr); > > + } > > This is not the correct resource for I/O space at all. Please talk > to Will, I've been over this with him in detail and he probably > understands it now. I assume you are both working in the same > building. Yes, the IO offsets work differently on powerpc as well > Since this is common PCI code, you could also decide to open-code > the pci_add_resource_offset() function. If you don't do that, I > think you have a memory leak for the resources that you can avoid > by allocating the resource and pci_host_bridge_window structures > together with a single kzalloc. > > > + /* 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); > > + > > + domain = of_alias_get_id(parent->of_node, "pci-domain"); > > + if (domain == -ENODEV) > > + domain = domain_nr++; > > We probably want some uniqueness testing here. > > + 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 */ > > + if (pci_host_bridge_of_get_ranges(parent->of_node, &res, &io_base)) > > + return NULL; > > + > > + /* 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; > > Do we have any code that checks for conflicting domain/bus numbers here? > I guess pci_create_root_bus_in_domain() will fail if you have that. > > Since pci_create_root_bus_in_domain() is a new function that you just > introduced, it would be helpful to change the calling conventions > so it returns an error pointer instead of NULL upon failing. > of_create_pci_host_bridge() can do the same, but pci_create_root_bus() > should keep returning NULL so we don't have to change all the > callers. > > > + 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; > > What is the io_base used for here? > > > @@ -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 > > > > -- > 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
On Friday 28 February 2014 10:32:04 Benjamin Herrenschmidt wrote: > On Thu, 2014-02-27 at 14:38 +0100, Arnd Bergmann wrote: > > On Thursday 27 February 2014 13:06:42 Liviu Dudau wrote: > > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > > > > Please add Benjamin Herrenschmidt to Cc here, I think it would be helpful > > to get his input so we can make this work on powerpc as well. > > Tricky... We would need hooks which would turn the whole thing into a > pile of spaghetti. I think we should stick to using the range helpers > (Andrew latest patch), which makes the powerpc code a lot smaller, > and leave it at that. We can certainly do both: small helpers that let you shrink the powerpc code, and a generic implementation that can be shared by some of the other architectures that you don't use. The PCI core already uses a number of 'weak' functions here, and we can expand on that. > > + 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; > > You don't care about the size of the IO space ? We probably should. The ARM code currently assumes that each I/O space is 64KB, but for a generic implementation we probably want to handle both smaller and larger windows. I suggested not supporting more than 1MB though, which is the maximum that I can see a reason for, i.e. the pci-mvebu fake host bridge that has to combine multiple per-port HW I/O spaces into one logical space. > > > + pci_add_resource_offset(resources, res, > > > + res->start - range.pci_addr); > > > + } > > > > This is not the correct resource for I/O space at all. Please talk > > to Will, I've been over this with him in detail and he probably > > understands it now. I assume you are both working in the same > > building. > > Yes, the IO offsets work differently on powerpc as well As I noticed later, the first patch in the series actually changes the range_to_resource parser to return the logical start here, which would make the offset correct even on powerpc, but unfortunately I think that cannot work. > > > +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); > > > + > > > + domain = of_alias_get_id(parent->of_node, "pci-domain"); > > > + if (domain == -ENODEV) > > > + domain = domain_nr++; > > > > We probably want some uniqueness testing here. I thought this at first too, but as Liviu mentioned, this does get caught later when trying to create the root bus with a conflicting number. What the above code cannot do is to put multiple host bridges into the same domain, using distinct bus ranges. This is an intentional simplification over what some architectures currently do, and we could not see a reason why you would still need to put multiple host bridges into one domain in 2014. x86 with ACPI does it, but they won't call of_create_pci_host_bridge. Arnd
On Thu, Feb 27, 2014 at 11:32:04PM +0000, Benjamin Herrenschmidt wrote: > On Thu, 2014-02-27 at 14:38 +0100, Arnd Bergmann wrote: > > On Thursday 27 February 2014 13:06:42 Liviu Dudau wrote: > > > 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. > > So that is only going to work for archs that don't have any special > twist. For example it won't work for powerpc which is why Andrew > original approach didn't fly. > > The range walk helpers do help though, I need to review in more details > and test Andrew powerpc patch here and will merge it. > > > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > > > > Please add Benjamin Herrenschmidt to Cc here, I think it would be helpful > > to get his input so we can make this work on powerpc as well. > > Tricky... We would need hooks which would turn the whole thing into a > pile of spaghetti. I think we should stick to using the range helpers > (Andrew latest patch), which makes the powerpc code a lot smaller, > and leave it at that. Benjamin, Thanks for looking at the patches. I did look at powerpc and microblaze. I've actually started modifying microblaze as it seems to have fewer corner cases and that's how I came up with the current design. Unfortunately my QEMU environment crashes when trying to feed it a custom dtb file that has a pci host bridge node. I know that in the range parsing powerpc does a lot more work and that is why I've split my code into the generic part and the arch specific callback that can do validation of the ranges and also a bit of housekeeping. The next phase will be to see how much of the pci_controller structure can move into the generic code and reduce its size. I do suffer from not having a PowerPC platform where I can test the changes, but I can produce some compiled code that someone can test. > > > > diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c > > > index 06ace62..feb8436 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 int domain_nr; > > > + > > > > For correctness, I think you want an 'atomic_t' here and use > > atomic_inc_return() to get a new value. > > > > > static struct pci_bus *find_pci_root_bus(struct pci_bus *bus) > > > { > > > while (bus->parent) > > > @@ -91,3 +95,133 @@ 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; > > Shouldn't that test move into the parsing helper ? Good question. Should we then automatically skip the bad translated ranges? > > > > + 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; > > You don't care about the size of the IO space ? Not here. I will leave it to the platform specific hook to do the validation. > > > > + pci_add_resource_offset(resources, res, > > > + res->start - range.pci_addr); > > > + } > > > > This is not the correct resource for I/O space at all. Please talk > > to Will, I've been over this with him in detail and he probably > > understands it now. I assume you are both working in the same > > building. > > Yes, the IO offsets work differently on powerpc as well Can you check for powerpc in light of the changes I've made in the of_pci_range_to_resource() function? (and please read range.cpu_addr instead of range.pci_addr in the pci_address_to_pio(). I'll post v3 shortly.) > > > Since this is common PCI code, you could also decide to open-code > > the pci_add_resource_offset() function. If you don't do that, I > > think you have a memory leak for the resources that you can avoid > > by allocating the resource and pci_host_bridge_window structures > > together with a single kzalloc. > > > > > + /* 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); > > > + > > > + domain = of_alias_get_id(parent->of_node, "pci-domain"); > > > + if (domain == -ENODEV) > > > + domain = domain_nr++; > > > > We probably want some uniqueness testing here. You mean you want one host bridge per domain? It's not entirely clear if there is general consensus here on this, but I'm in favour of enforcing that. Best regards, Liviu > > > > + 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 */ > > > + if (pci_host_bridge_of_get_ranges(parent->of_node, &res, &io_base)) > > > + return NULL; > > > + > > > + /* 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; > > > > Do we have any code that checks for conflicting domain/bus numbers here? > > I guess pci_create_root_bus_in_domain() will fail if you have that. > > > > Since pci_create_root_bus_in_domain() is a new function that you just > > introduced, it would be helpful to change the calling conventions > > so it returns an error pointer instead of NULL upon failing. > > of_create_pci_host_bridge() can do the same, but pci_create_root_bus() > > should keep returning NULL so we don't have to change all the > > callers. > > > > > + 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; > > > > What is the io_base used for here? > > > > > @@ -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 > > > > > > > -- > > 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 > > >
On Fri, 2014-02-28 at 09:55 +0000, Liviu Dudau wrote: > The next phase will be to see how much of the pci_controller structure can move > into the generic code and reduce its size. I do suffer from not having a PowerPC > platform where I can test the changes, but I can produce some compiled code that > someone can test. Ok, we can probably start from there. I think the right approach is to actually kill pci_controller by merging it with the new host bridge structure. Most of the resources are there now, we need to move the rest. We probably will need a way to attach platform specific bits to it though, and a few hooks to populate them but that isn't terribly hard. Cheers, Ben.
On Sun, 2014-03-02 at 12:23 +1100, Benjamin Herrenschmidt wrote: > On Fri, 2014-02-28 at 09:55 +0000, Liviu Dudau wrote: > > The next phase will be to see how much of the pci_controller structure can move > > into the generic code and reduce its size. I do suffer from not having a PowerPC > > platform where I can test the changes, but I can produce some compiled code that > > someone can test. > > Ok, we can probably start from there. I think the right approach is to actually > kill pci_controller by merging it with the new host bridge structure. > > Most of the resources are there now, we need to move the rest. We probably will > need a way to attach platform specific bits to it though, and a few hooks to > populate them but that isn't terribly hard. Oh and regarding testing... qemu should help. For qemu ppc64, try -M pseries, that's fairly well maintained since it's the basis for all our new KVM based products, and there's still some (hopefully) working 32-bit mac stuff in there. With pseries, I would expect that you can install fedora, debian or ubuntu (in fact you can also install SLES11/RHEL6 I believe). Cheers, Ben.
On Thu, 27 Feb 2014 14:38:32 +0100, Arnd Bergmann <arnd@arndb.de> wrote: > On Thursday 27 February 2014 13:06:42 Liviu Dudau wrote: > > 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> > > Please add Benjamin Herrenschmidt to Cc here, I think it would be helpful > to get his input so we can make this work on powerpc as well. s/helpful/required/ :-) g.
diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c index 06ace62..feb8436 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 int domain_nr; + static struct pci_bus *find_pci_root_bus(struct pci_bus *bus) { while (bus->parent) @@ -91,3 +95,133 @@ 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); + + domain = of_alias_get_id(parent->of_node, "pci-domain"); + if (domain == -ENODEV) + domain = 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 */ + if (pci_host_bridge_of_get_ranges(parent->of_node, &res, &io_base)) + return NULL; + + /* 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
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>