| Message ID | 1410184472-17630-9-git-send-email-Liviu.Dudau@arm.com |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Sep 08, 2014 at 02:54:30PM +0100, Liviu Dudau wrote: > Provide a function to parse the PCI DT ranges that can be used to > create a pci_host_bridge structure together with its associated > bus. > > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Grant Likely <grant.likely@linaro.org> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > --- > drivers/of/of_pci.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++ > include/linux/of_pci.h | 11 ++++++ > 2 files changed, 113 insertions(+) > > diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c > index a107edb..36701da 100644 > --- a/drivers/of/of_pci.c > +++ b/drivers/of/of_pci.c > @@ -1,7 +1,9 @@ > #include <linux/kernel.h> > #include <linux/export.h> > #include <linux/of.h> > +#include <linux/of_address.h> > #include <linux/of_pci.h> > +#include <linux/slab.h> > > static inline int __of_pci_pci_compare(struct device_node *node, > unsigned int data) > @@ -123,6 +125,106 @@ int of_pci_get_domain_nr(struct device_node *node, bool allocate_if_missing) > } > EXPORT_SYMBOL_GPL(of_pci_get_domain_nr); > > +/** > + * of_pci_get_host_bridge_resources - Parse PCI host bridge resources from DT > + * @dev: device node of the host bridge having the range property > + * @busno: bus number associated with the bridge root bus > + * @bus_max: maximum number of busses for this bridge > + * @resources: list where the range of resources will be added after DT parsing > + * @io_base: pointer to a variable that will contain on return the physical > + * address for the start of the I/O range. > + * > + * It is the callers job to free the @resources list. > + * > + * 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. You should also define what it returns and when. > + * > + */ > +int of_pci_get_host_bridge_resources(struct device_node *dev, > + unsigned char busno, unsigned char bus_max, > + struct list_head *resources, resource_size_t *io_base) > +{ > + struct resource *res; > + struct resource *bus_range; > + struct of_pci_range range; > + struct of_pci_range_parser parser; > + char range_type[4]; > + int err; > + > + bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL); > + if (!bus_range) > + return -ENOMEM; > + > + pr_info("PCI host bridge %s ranges:\n", dev->full_name); > + > + err = of_pci_parse_bus_range(dev, bus_range); > + if (err) { > + bus_range->start = busno; > + bus_range->end = bus_max; > + bus_range->flags = IORESOURCE_BUS; > + pr_info(" No bus range found for %s, using %pR\n", > + dev->full_name, &bus_range); > + } else { > + if (bus_range->end > bus_range->start + bus_max) > + bus_range->end = bus_range->start + bus_max; > + } > + pci_add_resource(resources, bus_range); This means that eg in the PCI generic host controller I cannot "filter" the bus resource, unless I remove it, "filter" it, and add it again. I certainly can't filter a resource that has been already added without removing it first. Thoughts ? > + > + /* Check for ranges property */ > + err = of_pci_range_parser_init(&parser, dev); > + if (err) > + goto parse_failed; > + > + pr_debug("Parsing ranges property...\n"); > + for_each_of_pci_range(&parser, &range) { > + /* Read next ranges element */ > + if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_IO) > + snprintf(range_type, 4, " IO"); > + else if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_MEM) > + snprintf(range_type, 4, "MEM"); > + else > + snprintf(range_type, 4, "err"); > + pr_info(" %s %#010llx..%#010llx -> %#010llx\n", range_type, > + range.cpu_addr, range.cpu_addr + range.size - 1, > + range.pci_addr); > + > + /* > + * 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 parse_failed; > + } > + > + err = of_pci_range_to_resource(&range, dev, res); > + if (err) { > + kfree(res); You might want to add a label to free res to make things more uniform. > + goto parse_failed; > + } > + > + if (resource_type(res) == IORESOURCE_IO) { > + if (*io_base) You do not zero io_base in the first place so you should ask the API user to do that. Is 0 a valid value BTW ? If it is you've got to resort to something else to detect multiple IO resources. Lorenzo -- 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 Tue, Sep 09, 2014 at 02:35:46PM +0100, Lorenzo Pieralisi wrote: > On Mon, Sep 08, 2014 at 02:54:30PM +0100, Liviu Dudau wrote: > > Provide a function to parse the PCI DT ranges that can be used to > > create a pci_host_bridge structure together with its associated > > bus. > > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: Grant Likely <grant.likely@linaro.org> > > Cc: Rob Herring <robh+dt@kernel.org> > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > > --- > > drivers/of/of_pci.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++ > > include/linux/of_pci.h | 11 ++++++ > > 2 files changed, 113 insertions(+) > > > > diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c > > index a107edb..36701da 100644 > > --- a/drivers/of/of_pci.c > > +++ b/drivers/of/of_pci.c > > @@ -1,7 +1,9 @@ > > #include <linux/kernel.h> > > #include <linux/export.h> > > #include <linux/of.h> > > +#include <linux/of_address.h> > > #include <linux/of_pci.h> > > +#include <linux/slab.h> > > > > static inline int __of_pci_pci_compare(struct device_node *node, > > unsigned int data) > > @@ -123,6 +125,106 @@ int of_pci_get_domain_nr(struct device_node *node, bool allocate_if_missing) > > } > > EXPORT_SYMBOL_GPL(of_pci_get_domain_nr); > > > > +/** > > + * of_pci_get_host_bridge_resources - Parse PCI host bridge resources from DT > > + * @dev: device node of the host bridge having the range property > > + * @busno: bus number associated with the bridge root bus > > + * @bus_max: maximum number of busses for this bridge > > + * @resources: list where the range of resources will be added after DT parsing > > + * @io_base: pointer to a variable that will contain on return the physical > > + * address for the start of the I/O range. > > + * > > + * It is the callers job to free the @resources list. > > + * > > + * 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. > > You should also define what it returns and when. Thanks, will do. > > > + * > > + */ > > +int of_pci_get_host_bridge_resources(struct device_node *dev, > > + unsigned char busno, unsigned char bus_max, > > + struct list_head *resources, resource_size_t *io_base) > > +{ > > + struct resource *res; > > + struct resource *bus_range; > > + struct of_pci_range range; > > + struct of_pci_range_parser parser; > > + char range_type[4]; > > + int err; > > + > > + bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL); > > + if (!bus_range) > > + return -ENOMEM; > > + > > + pr_info("PCI host bridge %s ranges:\n", dev->full_name); > > + > > + err = of_pci_parse_bus_range(dev, bus_range); > > + if (err) { > > + bus_range->start = busno; > > + bus_range->end = bus_max; > > + bus_range->flags = IORESOURCE_BUS; > > + pr_info(" No bus range found for %s, using %pR\n", > > + dev->full_name, &bus_range); > > + } else { > > + if (bus_range->end > bus_range->start + bus_max) > > + bus_range->end = bus_range->start + bus_max; > > + } > > + pci_add_resource(resources, bus_range); > > This means that eg in the PCI generic host controller I cannot "filter" > the bus resource, unless I remove it, "filter" it, and add it again. I'm not sure what you mean. Why do you have to remove the bus resource and add it again? What you get back from of_pci_get_host_bridge_resources() is a list of resources as they have been parsed from DT. You now have the option of doing any filtering that you might have done pre-v10 in the pcibios_fixup_bridge_ranges() but that is only on the list that was returned from of_pci_get_host_bridge_resources(). At this moment no root bus or host bridge structure has been created so no resource was added to those. With the filtered list you can use it to call pci_scan_root_bus() and *then* it gets added to the pci_host_bridge structure. > > I certainly can't filter a resource that has been already added without > removing it first. > > Thoughts ? Hope I have explained what happens. Please let me know if you have any other comments. Best regards, Liviu > > > + > > + /* Check for ranges property */ > > + err = of_pci_range_parser_init(&parser, dev); > > + if (err) > > + goto parse_failed; > > + > > + pr_debug("Parsing ranges property...\n"); > > + for_each_of_pci_range(&parser, &range) { > > + /* Read next ranges element */ > > + if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_IO) > > + snprintf(range_type, 4, " IO"); > > + else if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_MEM) > > + snprintf(range_type, 4, "MEM"); > > + else > > + snprintf(range_type, 4, "err"); > > + pr_info(" %s %#010llx..%#010llx -> %#010llx\n", range_type, > > + range.cpu_addr, range.cpu_addr + range.size - 1, > > + range.pci_addr); > > + > > + /* > > + * 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 parse_failed; > > + } > > + > > + err = of_pci_range_to_resource(&range, dev, res); > > + if (err) { > > + kfree(res); > > You might want to add a label to free res to make things more uniform. > > > + goto parse_failed; > > + } > > + > > + if (resource_type(res) == IORESOURCE_IO) { > > + if (*io_base) > > You do not zero io_base in the first place so you should ask the API > user to do that. Is 0 a valid value BTW ? If it is you've got to resort > to something else to detect multiple IO resources. > > Lorenzo
On Wed, Sep 10, 2014 at 03:22:41PM +0100, Liviu Dudau wrote: > On Tue, Sep 09, 2014 at 02:35:46PM +0100, Lorenzo Pieralisi wrote: > > On Mon, Sep 08, 2014 at 02:54:30PM +0100, Liviu Dudau wrote: > > > Provide a function to parse the PCI DT ranges that can be used to > > > create a pci_host_bridge structure together with its associated > > > bus. > > > > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > > Cc: Arnd Bergmann <arnd@arndb.de> > > > Cc: Grant Likely <grant.likely@linaro.org> > > > Cc: Rob Herring <robh+dt@kernel.org> > > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > > > --- > > > drivers/of/of_pci.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++ > > > include/linux/of_pci.h | 11 ++++++ > > > 2 files changed, 113 insertions(+) > > > > > > diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c > > > index a107edb..36701da 100644 > > > --- a/drivers/of/of_pci.c > > > +++ b/drivers/of/of_pci.c > > > @@ -1,7 +1,9 @@ > > > #include <linux/kernel.h> > > > #include <linux/export.h> > > > #include <linux/of.h> > > > +#include <linux/of_address.h> > > > #include <linux/of_pci.h> > > > +#include <linux/slab.h> > > > > > > static inline int __of_pci_pci_compare(struct device_node *node, > > > unsigned int data) > > > @@ -123,6 +125,106 @@ int of_pci_get_domain_nr(struct device_node *node, bool allocate_if_missing) > > > } > > > EXPORT_SYMBOL_GPL(of_pci_get_domain_nr); > > > > > > +/** > > > + * of_pci_get_host_bridge_resources - Parse PCI host bridge resources from DT > > > + * @dev: device node of the host bridge having the range property > > > + * @busno: bus number associated with the bridge root bus > > > + * @bus_max: maximum number of busses for this bridge > > > + * @resources: list where the range of resources will be added after DT parsing > > > + * @io_base: pointer to a variable that will contain on return the physical > > > + * address for the start of the I/O range. > > > + * > > > + * It is the callers job to free the @resources list. > > > + * > > > + * 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. > > > > You should also define what it returns and when. > > Thanks, will do. > > > > > > + * > > > + */ > > > +int of_pci_get_host_bridge_resources(struct device_node *dev, > > > + unsigned char busno, unsigned char bus_max, > > > + struct list_head *resources, resource_size_t *io_base) > > > +{ > > > + struct resource *res; > > > + struct resource *bus_range; > > > + struct of_pci_range range; > > > + struct of_pci_range_parser parser; > > > + char range_type[4]; > > > + int err; > > > + > > > + bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL); > > > + if (!bus_range) > > > + return -ENOMEM; > > > + > > > + pr_info("PCI host bridge %s ranges:\n", dev->full_name); > > > + > > > + err = of_pci_parse_bus_range(dev, bus_range); > > > + if (err) { > > > + bus_range->start = busno; > > > + bus_range->end = bus_max; > > > + bus_range->flags = IORESOURCE_BUS; > > > + pr_info(" No bus range found for %s, using %pR\n", > > > + dev->full_name, &bus_range); > > > + } else { > > > + if (bus_range->end > bus_range->start + bus_max) > > > + bus_range->end = bus_range->start + bus_max; > > > + } > > > + pci_add_resource(resources, bus_range); > > > > This means that eg in the PCI generic host controller I cannot "filter" > > the bus resource, unless I remove it, "filter" it, and add it again. > > I'm not sure what you mean. Why do you have to remove the bus resource and > add it again? What you get back from of_pci_get_host_bridge_resources() is > a list of resources as they have been parsed from DT. You now have the option > of doing any filtering that you might have done pre-v10 in the > pcibios_fixup_bridge_ranges() but that is only on the list that was returned > from of_pci_get_host_bridge_resources(). At this moment no root bus or > host bridge structure has been created so no resource was added to those. > With the filtered list you can use it to call pci_scan_root_bus() and *then* > it gets added to the pci_host_bridge structure. You are right sorry. As discussed, I just have to remove the resources assignment in the respective host controller drivers (because you do that in the API) and grab the resource pointers to "filter" them. > > I certainly can't filter a resource that has been already added without > > removing it first. > > > > Thoughts ? > > Hope I have explained what happens. Please let me know if you have any other > comments. There were other comments below ;) Thanks, Lorenzo > > Best regards, > Liviu > > > > > > + > > > + /* Check for ranges property */ > > > + err = of_pci_range_parser_init(&parser, dev); > > > + if (err) > > > + goto parse_failed; > > > + > > > + pr_debug("Parsing ranges property...\n"); > > > + for_each_of_pci_range(&parser, &range) { > > > + /* Read next ranges element */ > > > + if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_IO) > > > + snprintf(range_type, 4, " IO"); > > > + else if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_MEM) > > > + snprintf(range_type, 4, "MEM"); > > > + else > > > + snprintf(range_type, 4, "err"); > > > + pr_info(" %s %#010llx..%#010llx -> %#010llx\n", range_type, > > > + range.cpu_addr, range.cpu_addr + range.size - 1, > > > + range.pci_addr); > > > + > > > + /* > > > + * 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 parse_failed; > > > + } > > > + > > > + err = of_pci_range_to_resource(&range, dev, res); > > > + if (err) { > > > + kfree(res); > > > > You might want to add a label to free res to make things more uniform. > > > > > + goto parse_failed; > > > + } > > > + > > > + if (resource_type(res) == IORESOURCE_IO) { > > > + if (*io_base) > > > > You do not zero io_base in the first place so you should ask the API > > user to do that. Is 0 a valid value BTW ? If it is you've got to resort > > to something else to detect multiple IO resources. > > > > Lorenzo > > -- > ==================== > | I would like to | > | fix the world, | > | but they're not | > | giving me the | > \ source code! / > --------------- > ?\_(?)_/? -- 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 Wed, Sep 10, 2014 at 04:10:26PM +0100, Lorenzo Pieralisi wrote: > On Wed, Sep 10, 2014 at 03:22:41PM +0100, Liviu Dudau wrote: > > On Tue, Sep 09, 2014 at 02:35:46PM +0100, Lorenzo Pieralisi wrote: > > > On Mon, Sep 08, 2014 at 02:54:30PM +0100, Liviu Dudau wrote: > > > > Provide a function to parse the PCI DT ranges that can be used to > > > > create a pci_host_bridge structure together with its associated > > > > bus. > > > > > > > > Cc: Bjorn Helgaas <bhelgaas@google.com> > > > > Cc: Arnd Bergmann <arnd@arndb.de> > > > > Cc: Grant Likely <grant.likely@linaro.org> > > > > Cc: Rob Herring <robh+dt@kernel.org> > > > > Cc: Catalin Marinas <catalin.marinas@arm.com> > > > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > > > > --- > > > > drivers/of/of_pci.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++ > > > > include/linux/of_pci.h | 11 ++++++ > > > > 2 files changed, 113 insertions(+) > > > > > > > > diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c > > > > index a107edb..36701da 100644 > > > > --- a/drivers/of/of_pci.c > > > > +++ b/drivers/of/of_pci.c > > > > @@ -1,7 +1,9 @@ > > > > #include <linux/kernel.h> > > > > #include <linux/export.h> > > > > #include <linux/of.h> > > > > +#include <linux/of_address.h> > > > > #include <linux/of_pci.h> > > > > +#include <linux/slab.h> > > > > > > > > static inline int __of_pci_pci_compare(struct device_node *node, > > > > unsigned int data) > > > > @@ -123,6 +125,106 @@ int of_pci_get_domain_nr(struct device_node *node, bool allocate_if_missing) > > > > } > > > > EXPORT_SYMBOL_GPL(of_pci_get_domain_nr); > > > > > > > > +/** > > > > + * of_pci_get_host_bridge_resources - Parse PCI host bridge resources from DT > > > > + * @dev: device node of the host bridge having the range property > > > > + * @busno: bus number associated with the bridge root bus > > > > + * @bus_max: maximum number of busses for this bridge > > > > + * @resources: list where the range of resources will be added after DT parsing > > > > + * @io_base: pointer to a variable that will contain on return the physical > > > > + * address for the start of the I/O range. > > > > + * > > > > + * It is the callers job to free the @resources list. > > > > + * > > > > + * 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. > > > > > > You should also define what it returns and when. > > > > Thanks, will do. > > > > > > > > > + * > > > > + */ > > > > +int of_pci_get_host_bridge_resources(struct device_node *dev, > > > > + unsigned char busno, unsigned char bus_max, > > > > + struct list_head *resources, resource_size_t *io_base) > > > > +{ > > > > + struct resource *res; > > > > + struct resource *bus_range; > > > > + struct of_pci_range range; > > > > + struct of_pci_range_parser parser; > > > > + char range_type[4]; > > > > + int err; > > > > + > > > > + bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL); > > > > + if (!bus_range) > > > > + return -ENOMEM; > > > > + > > > > + pr_info("PCI host bridge %s ranges:\n", dev->full_name); > > > > + > > > > + err = of_pci_parse_bus_range(dev, bus_range); > > > > + if (err) { > > > > + bus_range->start = busno; > > > > + bus_range->end = bus_max; > > > > + bus_range->flags = IORESOURCE_BUS; > > > > + pr_info(" No bus range found for %s, using %pR\n", > > > > + dev->full_name, &bus_range); > > > > + } else { > > > > + if (bus_range->end > bus_range->start + bus_max) > > > > + bus_range->end = bus_range->start + bus_max; > > > > + } > > > > + pci_add_resource(resources, bus_range); > > > > > > This means that eg in the PCI generic host controller I cannot "filter" > > > the bus resource, unless I remove it, "filter" it, and add it again. > > > > I'm not sure what you mean. Why do you have to remove the bus resource and > > add it again? What you get back from of_pci_get_host_bridge_resources() is > > a list of resources as they have been parsed from DT. You now have the option > > of doing any filtering that you might have done pre-v10 in the > > pcibios_fixup_bridge_ranges() but that is only on the list that was returned > > from of_pci_get_host_bridge_resources(). At this moment no root bus or > > host bridge structure has been created so no resource was added to those. > > With the filtered list you can use it to call pci_scan_root_bus() and *then* > > it gets added to the pci_host_bridge structure. > > You are right sorry. As discussed, I just have to remove the resources > assignment in the respective host controller drivers (because you do > that in the API) and grab the resource pointers to "filter" them. > > > > I certainly can't filter a resource that has been already added without > > > removing it first. > > > > > > Thoughts ? > > > > Hope I have explained what happens. Please let me know if you have any other > > comments. > > There were other comments below ;) My PgDn key is broken ;) > > Thanks, > Lorenzo > > > > > Best regards, > > Liviu > > > > > > > > > + > > > > + /* Check for ranges property */ > > > > + err = of_pci_range_parser_init(&parser, dev); > > > > + if (err) > > > > + goto parse_failed; > > > > + > > > > + pr_debug("Parsing ranges property...\n"); > > > > + for_each_of_pci_range(&parser, &range) { > > > > + /* Read next ranges element */ > > > > + if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_IO) > > > > + snprintf(range_type, 4, " IO"); > > > > + else if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_MEM) > > > > + snprintf(range_type, 4, "MEM"); > > > > + else > > > > + snprintf(range_type, 4, "err"); > > > > + pr_info(" %s %#010llx..%#010llx -> %#010llx\n", range_type, > > > > + range.cpu_addr, range.cpu_addr + range.size - 1, > > > > + range.pci_addr); > > > > + > > > > + /* > > > > + * 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 parse_failed; > > > > + } > > > > + > > > > + err = of_pci_range_to_resource(&range, dev, res); > > > > + if (err) { > > > > + kfree(res); > > > > > > You might want to add a label to free res to make things more uniform. Sorry, not following you. How would a label help here? > > > > > > > + goto parse_failed; > > > > + } > > > > + > > > > + if (resource_type(res) == IORESOURCE_IO) { > > > > + if (*io_base) > > > > > > You do not zero io_base in the first place so you should ask the API > > > user to do that. Is 0 a valid value BTW ? If it is you've got to resort > > > to something else to detect multiple IO resources. No, zero is not a valid value. It is the cpu_addr value from the IO range, I'm hopying that no one is crazy enough to map PCI address space at CPU address zero. Thanks for spotting the lack of initialisation though, I need to fix it. Best regards, Liviu > > > > > > Lorenzo
On Wed, Sep 10, 2014 at 04:32:33PM +0100, Liviu Dudau wrote: [...] > > > > > + /* > > > > > + * 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 parse_failed; > > > > > + } > > > > > + > > > > > + err = of_pci_range_to_resource(&range, dev, res); > > > > > + if (err) { > > > > > + kfree(res); > > > > > > > > You might want to add a label to free res to make things more uniform. > > Sorry, not following you. How would a label help here? It was just a suggestion so ignore it if you do not think it is cleaner. It is to make code more uniform and undo operations in one place instead of doing it piecemeal (you kfree the res here and jump to complete the clean-up, whereas you might want to add a different label and a different goto destination and carry out the kfree there). I do not mind either, it is just what I noticed. > > > > > + goto parse_failed; > > > > > + } > > > > > + > > > > > + if (resource_type(res) == IORESOURCE_IO) { > > > > > + if (*io_base) > > > > > > > > You do not zero io_base in the first place so you should ask the API > > > > user to do that. Is 0 a valid value BTW ? If it is you've got to resort > > > > to something else to detect multiple IO resources. > > No, zero is not a valid value. It is the cpu_addr value from the IO range, I'm > hopying that no one is crazy enough to map PCI address space at CPU address zero. > Thanks for spotting the lack of initialisation though, I need to fix it. Mmm...wasn't a trick question sorry :D PCI host bridge /pci ranges: IO 0x00000000..0x0000ffff -> 0x00000000 More than one I/O resource converted. CPU offset for old range lost! MEM 0x41000000..0x7fffffff -> 0x41000000 pci-host-generic 40000000.pci: PCI host bridge to bus 0000:00 pci_bus 0000:00: root bus resource [bu-01] pci_bus 0000:00: root bus resource [io 0x0000-0xffff] pci_bus 0000:00: root bus resource [mem 0x41000000-0x7fffffff] -- 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 Wed, Sep 10, 2014 at 05:37:47PM +0100, Lorenzo Pieralisi wrote: > On Wed, Sep 10, 2014 at 04:32:33PM +0100, Liviu Dudau wrote: > > [...] > > > > > > > + /* > > > > > > + * 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 parse_failed; > > > > > > + } > > > > > > + > > > > > > + err = of_pci_range_to_resource(&range, dev, res); > > > > > > + if (err) { > > > > > > + kfree(res); > > > > > > > > > > You might want to add a label to free res to make things more uniform. > > > > Sorry, not following you. How would a label help here? > > It was just a suggestion so ignore it if you do not think it is cleaner. > It is to make code more uniform and undo operations in one place instead of > doing it piecemeal (you kfree the res here and jump to complete the clean-up, > whereas you might want to add a different label and a different goto > destination and carry out the kfree there). But that kfree is the only done once, while the pci_free_resource_list() is done twice. > > I do not mind either, it is just what I noticed. > > > > > > > + goto parse_failed; > > > > > > + } > > > > > > + > > > > > > + if (resource_type(res) == IORESOURCE_IO) { > > > > > > + if (*io_base) > > > > > > > > > > You do not zero io_base in the first place so you should ask the API > > > > > user to do that. Is 0 a valid value BTW ? If it is you've got to resort > > > > > to something else to detect multiple IO resources. > > > > No, zero is not a valid value. It is the cpu_addr value from the IO range, I'm > > hopying that no one is crazy enough to map PCI address space at CPU address zero. > > Thanks for spotting the lack of initialisation though, I need to fix it. > > Mmm...wasn't a trick question sorry :D > > PCI host bridge /pci ranges: > IO 0x00000000..0x0000ffff -> 0x00000000 > More than one I/O resource converted. CPU offset for old range lost! > MEM 0x41000000..0x7fffffff -> 0x41000000 > pci-host-generic 40000000.pci: PCI host bridge to bus 0000:00 > pci_bus 0000:00: root bus resource [bu-01] > pci_bus 0000:00: root bus resource [io 0x0000-0xffff] > pci_bus 0000:00: root bus resource [mem 0x41000000-0x7fffffff] > Your DT puts the IO space at CPU address zero? Could you copy the relevant host bridge node from DT here? Best regards, Liviu
On Wed, Sep 10, 2014 at 05:53:47PM +0100, Liviu Dudau wrote: [...] > > > > > > > + if (resource_type(res) == IORESOURCE_IO) { > > > > > > > + if (*io_base) > > > > > > > > > > > > You do not zero io_base in the first place so you should ask the API > > > > > > user to do that. Is 0 a valid value BTW ? If it is you've got to resort > > > > > > to something else to detect multiple IO resources. > > > > > > No, zero is not a valid value. It is the cpu_addr value from the IO range, I'm > > > hopying that no one is crazy enough to map PCI address space at CPU address zero. > > > Thanks for spotting the lack of initialisation though, I need to fix it. > > > > Mmm...wasn't a trick question sorry :D > > > > PCI host bridge /pci ranges: > > IO 0x00000000..0x0000ffff -> 0x00000000 > > More than one I/O resource converted. CPU offset for old range lost! > > MEM 0x41000000..0x7fffffff -> 0x41000000 > > pci-host-generic 40000000.pci: PCI host bridge to bus 0000:00 > > pci_bus 0000:00: root bus resource [bu-01] > > pci_bus 0000:00: root bus resource [io 0x0000-0xffff] > > pci_bus 0000:00: root bus resource [mem 0x41000000-0x7fffffff] > > > > Your DT puts the IO space at CPU address zero? Could you copy the relevant > host bridge node from DT here? It is kvmtools generated dts, may not be representative of real HW but that's what it is. For the whole story: http://comments.gmane.org/gmane.linux.kernel.pci/29199 Lorenzo -- 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
diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c index a107edb..36701da 100644 --- a/drivers/of/of_pci.c +++ b/drivers/of/of_pci.c @@ -1,7 +1,9 @@ #include <linux/kernel.h> #include <linux/export.h> #include <linux/of.h> +#include <linux/of_address.h> #include <linux/of_pci.h> +#include <linux/slab.h> static inline int __of_pci_pci_compare(struct device_node *node, unsigned int data) @@ -123,6 +125,106 @@ int of_pci_get_domain_nr(struct device_node *node, bool allocate_if_missing) } EXPORT_SYMBOL_GPL(of_pci_get_domain_nr); +/** + * of_pci_get_host_bridge_resources - Parse PCI host bridge resources from DT + * @dev: device node of the host bridge having the range property + * @busno: bus number associated with the bridge root bus + * @bus_max: maximum number of busses for this bridge + * @resources: list where the range of resources will be added after DT parsing + * @io_base: pointer to a variable that will contain on return the physical + * address for the start of the I/O range. + * + * It is the callers job to free the @resources list. + * + * 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. + * + */ +int of_pci_get_host_bridge_resources(struct device_node *dev, + unsigned char busno, unsigned char bus_max, + struct list_head *resources, resource_size_t *io_base) +{ + struct resource *res; + struct resource *bus_range; + struct of_pci_range range; + struct of_pci_range_parser parser; + char range_type[4]; + int err; + + bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL); + if (!bus_range) + return -ENOMEM; + + pr_info("PCI host bridge %s ranges:\n", dev->full_name); + + err = of_pci_parse_bus_range(dev, bus_range); + if (err) { + bus_range->start = busno; + bus_range->end = bus_max; + bus_range->flags = IORESOURCE_BUS; + pr_info(" No bus range found for %s, using %pR\n", + dev->full_name, &bus_range); + } else { + if (bus_range->end > bus_range->start + bus_max) + bus_range->end = bus_range->start + bus_max; + } + pci_add_resource(resources, bus_range); + + /* Check for ranges property */ + err = of_pci_range_parser_init(&parser, dev); + if (err) + goto parse_failed; + + pr_debug("Parsing ranges property...\n"); + for_each_of_pci_range(&parser, &range) { + /* Read next ranges element */ + if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_IO) + snprintf(range_type, 4, " IO"); + else if ((range.flags & IORESOURCE_TYPE_BITS) == IORESOURCE_MEM) + snprintf(range_type, 4, "MEM"); + else + snprintf(range_type, 4, "err"); + pr_info(" %s %#010llx..%#010llx -> %#010llx\n", range_type, + range.cpu_addr, range.cpu_addr + range.size - 1, + range.pci_addr); + + /* + * 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 parse_failed; + } + + err = of_pci_range_to_resource(&range, dev, res); + if (err) { + kfree(res); + goto parse_failed; + } + + if (resource_type(res) == IORESOURCE_IO) { + if (*io_base) + pr_warn("More than one I/O resource converted. CPU offset for old range lost!\n"); + *io_base = range.cpu_addr; + } + + pci_add_resource_offset(resources, res, res->start - range.pci_addr); + } + + return 0; + +parse_failed: + pci_free_resource_list(resources); + return err; +} +EXPORT_SYMBOL_GPL(of_pci_get_host_bridge_resources); + #ifdef CONFIG_PCI_MSI static LIST_HEAD(of_pci_msi_chip_list); diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h index 3a3824c..a4b8a85 100644 --- a/include/linux/of_pci.h +++ b/include/linux/of_pci.h @@ -16,6 +16,10 @@ int of_pci_get_devfn(struct device_node *np); int of_irq_parse_and_map_pci(const struct pci_dev *dev, u8 slot, u8 pin); int of_pci_parse_bus_range(struct device_node *node, struct resource *res); int of_pci_get_domain_nr(struct device_node *node, bool allocate_if_missing); +int of_pci_get_host_bridge_resources(struct device_node *dev, + unsigned char busno, unsigned char bus_max, + struct list_head *resources, resource_size_t *io_base); + #else static inline int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq) { @@ -50,6 +54,13 @@ of_pci_get_domain_nr(struct device_node *node, bool allocate_if_missing) { return -1; } + +int of_pci_get_host_bridge_resources(struct device_node *dev, + unsigned char busno, unsigned char bus_max, + struct list_head *resources, resource_size_t *io_base); +{ + return -EINVAL; +} #endif #if defined(CONFIG_OF) && defined(CONFIG_PCI_MSI)
Provide a function to parse the PCI DT ranges that can be used to create a pci_host_bridge structure together with its associated bus. Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Grant Likely <grant.likely@linaro.org> Cc: Rob Herring <robh+dt@kernel.org> Cc: Catalin Marinas <catalin.marinas@arm.com> Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> --- drivers/of/of_pci.c | 102 +++++++++++++++++++++++++++++++++++++++++++++++++ include/linux/of_pci.h | 11 ++++++ 2 files changed, 113 insertions(+)