[v8,8/9] pci: Add support for creating a generic host_bridge from device tree

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>
Tested-by: Tanmay Inamdar <tinamdar@apm.com>
---
 drivers/of/of_pci.c       | 135 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/host-bridge.c |  18 +++++++
 include/linux/of_pci.h    |  10 ++++
 include/linux/pci.h       |   8 +++
 4 files changed, 171 insertions(+)

It is clearly not my day today! This will fail compilation on ARCH=arm due to
missing header include for <linux/slab.h>

Will send an update tomorrow after more testing. Sorry!

Liviu

On Tue, Jul 01, 2014 at 09:50:50PM +0100, 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>
> Tested-by: Tanmay Inamdar <tinamdar@apm.com>
> ---
>  drivers/of/of_pci.c       | 136 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/host-bridge.c |  15 +++++
>  include/linux/of_pci.h    |  10 ++++
>  include/linux/pci.h       |   8 +++
>  4 files changed, 169 insertions(+)
> 
> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> index 8481996..e81402a 100644
> --- a/drivers/of/of_pci.c
> +++ b/drivers/of/of_pci.c
> @@ -1,6 +1,7 @@
>  #include <linux/kernel.h>
>  #include <linux/export.h>
>  #include <linux/of.h>
> +#include <linux/of_address.h>
>  #include <linux/of_pci.h>
>  
>  static inline int __of_pci_pci_compare(struct device_node *node,
> @@ -89,6 +90,141 @@ int of_pci_parse_bus_range(struct device_node *node, struct resource *res)
>  }
>  EXPORT_SYMBOL_GPL(of_pci_parse_bus_range);
>  
> +/**
> + * 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.
> + *
> + * It is the callers job to free the @resources list if an error is returned.
> + *
> + * 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 cpu_addr:0x%016llx size:0x%016llx\n",
> +			range.pci_space, range.pci_addr, 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)
> +			return -ENOMEM;
> +
> +		err = of_pci_range_to_resource(&range, dev, res);
> +		if (err)
> +			return err;
> +
> +		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 */
> +	return pcibios_fixup_bridge_ranges(resources);
> +}
> +
> +static atomic_t domain_nr = ATOMIC_INIT(-1);
> +
> +/**
> + * 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 = 0;
> +	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)
> +		goto err_create;
> +
> +	/* then create the root bus */
> +	root_bus = pci_create_root_bus_in_domain(parent, domain, busno,
> +						ops, host_data, &res);
> +	if (IS_ERR(root_bus)) {
> +		err = PTR_ERR(root_bus);
> +		goto err_create;
> +	}
> +
> +	bridge = to_pci_host_bridge(root_bus->bridge);
> +	bridge->io_base = io_base;
> +
> +	return bridge;
> +
> +err_create:
> +	pci_free_resource_list(&res);
> +	return ERR_PTR(err);
> +}
> +EXPORT_SYMBOL_GPL(of_create_pci_host_bridge);
> +
>  #ifdef CONFIG_PCI_MSI
>  
>  static LIST_HEAD(of_pci_msi_chip_list);
> diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
> index 36c669e..54ceafd 100644
> --- a/drivers/pci/host-bridge.c
> +++ b/drivers/pci/host-bridge.c
> @@ -83,3 +83,18 @@ void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
>  	res->end = region->end + offset;
>  }
>  EXPORT_SYMBOL(pcibios_bus_to_resource);
> +
> +/**
> + * Simple version of the platform specific code for filtering the list
> + * of resources obtained from the ranges declaration in DT.
> + *
> + * Platforms can override this function in order to impose stronger
> + * constraints onto the list of resources that a host bridge can use.
> + * The filtered list will then be used to create a root bus and associate
> + * it with the host bridge.
> + *
> + */
> +int __weak pcibios_fixup_bridge_ranges(struct list_head *resources)
> +{
> +	return 0;
> +}
> diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
> index dde3a4a..71e36d0 100644
> --- a/include/linux/of_pci.h
> +++ b/include/linux/of_pci.h
> @@ -15,6 +15,9 @@ struct device_node *of_pci_find_child_device(struct device_node *parent,
>  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);
> +struct pci_host_bridge *of_create_pci_host_bridge(struct device *parent,
> +					struct pci_ops *ops, void *host_data);
> +
>  #else
>  static inline int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
>  {
> @@ -43,6 +46,13 @@ of_pci_parse_bus_range(struct device_node *node, struct resource *res)
>  {
>  	return -EINVAL;
>  }
> +
> +static inline struct pci_host_bridge *
> +of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops,
> +			void *host_data)
> +{
> +	return NULL;
> +}
>  #endif
>  
>  #if defined(CONFIG_OF) && defined(CONFIG_PCI_MSI)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 7e7b939..556dc5f 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -402,6 +402,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;
> @@ -1809,8 +1810,15 @@ 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) { }
> +
>  #endif  /* CONFIG_OF */
>  
> +/* Used by architecture code to apply any quirks to the list of
> + * pci_host_bridge resource ranges before they are being used
> + * by of_create_pci_host_bridge()
> + */
> +extern int pcibios_fixup_bridge_ranges(struct list_head *resources);
> +
>  #ifdef CONFIG_EEH
>  static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
>  {
> -- 
> 2.0.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
>

Hi Liviu,

On Tue, Jul 01, 2014 at 07:43:33PM +0100, 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.

I just had a quick look at this to see how it differs from the parsing in
pci-host-generic.c and there a few small differences worth discussing.

> +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 cpu_addr:0x%016llx size:0x%016llx\n",
> +			range.pci_space, range.pci_addr, 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)
> +			return -ENOMEM;
> +
> +		err = of_pci_range_to_resource(&range, dev, res);
> +		if (err)
> +			return err;
> +
> +		if (resource_type(res) == IORESOURCE_IO)
> +			*io_base = range.cpu_addr;
> +
> +		pci_add_resource_offset(resources, res,
> +				res->start - range.pci_addr);

Where do you request_resource before adding it?

> +	}
> +
> +	/* Apply architecture specific fixups for the ranges */
> +	return pcibios_fixup_bridge_ranges(resources);

I currently mandate at least one non-prefetchable resource in the
device-tree. Should I simply drop this restriction, or do I have to somehow
hook this into the pcibios callback?

> +}
> +
> +static atomic_t domain_nr = ATOMIC_INIT(-1);
> +
> +/**
> + * 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;

What about bus_range->name?

> +	}
> +	busno = bus_range->start;
> +	pci_add_resource(&res, bus_range);

I currently truncate the bus range to fit inside the Configuration Space
window I have (in the reg property). How can I continue to do that with this
patch?

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

On Wed, Jul 02, 2014 at 12:22:30PM +0100, Will Deacon wrote:
> Hi Liviu,
> 
> On Tue, Jul 01, 2014 at 07:43:33PM +0100, 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.
> 
> I just had a quick look at this to see how it differs from the parsing in
> pci-host-generic.c and there a few small differences worth discussing.
> 
> > +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 cpu_addr:0x%016llx size:0x%016llx\n",
> > +			range.pci_space, range.pci_addr, 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)
> > +			return -ENOMEM;
> > +
> > +		err = of_pci_range_to_resource(&range, dev, res);
> > +		if (err)
> > +			return err;
> > +
> > +		if (resource_type(res) == IORESOURCE_IO)
> > +			*io_base = range.cpu_addr;
> > +
> > +		pci_add_resource_offset(resources, res,
> > +				res->start - range.pci_addr);
> 
> Where do you request_resource before adding it?

I don't, because I'm expecting that arch code might filter the list. When 
the host bridge code calls pci_scan_root_bus() the resources will be
requested then.

> 
> > +	}
> > +
> > +	/* Apply architecture specific fixups for the ranges */
> > +	return pcibios_fixup_bridge_ranges(resources);
> 
> I currently mandate at least one non-prefetchable resource in the
> device-tree. Should I simply drop this restriction, or do I have to somehow
> hook this into the pcibios callback?

Don't think I understand why you need at least one non-prefetcheable resource
but if you want to mandate that then the pcibios_fixup_bridge_ranges() would
be the place to put that check.

> 
> > +}
> > +
> > +static atomic_t domain_nr = ATOMIC_INIT(-1);
> > +
> > +/**
> > + * 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;
> 
> What about bus_range->name?

Don't know! Is anyone using it?

> 
> > +	}
> > +	busno = bus_range->start;
> > +	pci_add_resource(&res, bus_range);
> 
> I currently truncate the bus range to fit inside the Configuration Space
> window I have (in the reg property). How can I continue to do that with this
> patch?

Not easily. Unless I add an argument to this function that will allow you to
pass in the max number for the bus range, then the code becomes:

+	err = of_pci_parse_bus_range(parent->of_node, bus_range);
+	if (err) {
+		dev_info(parent, "No bus range for %s, using default [0-%d]\n",
+			parent->of_node->full_name, max_range);
+		bus_range->start = 0;
+		bus_range->end = max_range;
+		bus_range->flags = IORESOURCE_BUS;
+	} else {
+		if (bus_range->end > bus_range->start + max_range) {
+			bus_range->end = bus_range->start + max_range;
+		}
+	}

Or something like that.

Best regards,
Liviu

> 
> Will
> --
> 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, Jul 02, 2014 at 06:23:55PM +0100, Liviu Dudau wrote:
> On Wed, Jul 02, 2014 at 12:22:30PM +0100, Will Deacon wrote:
> > On Tue, Jul 01, 2014 at 07:43:33PM +0100, 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.
> > 
> > I just had a quick look at this to see how it differs from the parsing in
> > pci-host-generic.c and there a few small differences worth discussing.

[...]

> > > +	}
> > > +
> > > +	/* Apply architecture specific fixups for the ranges */
> > > +	return pcibios_fixup_bridge_ranges(resources);
> > 
> > I currently mandate at least one non-prefetchable resource in the
> > device-tree. Should I simply drop this restriction, or do I have to somehow
> > hook this into the pcibios callback?
> 
> Don't think I understand why you need at least one non-prefetcheable resource
> but if you want to mandate that then the pcibios_fixup_bridge_ranges() would
> be the place to put that check.

I think it was Arnd's idea at the time:

  http://lists.infradead.org/pipermail/linux-arm-kernel/2014-February/232225.html

and it's probably worth keeping if possible (just to avoid changes to the
behaviour of the existing driver).

However, that means I already need a host-controller callback via
pcibios_fixup_bridge_ranges...

> > > +	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;
> > 
> > What about bus_range->name?
> 
> Don't know! Is anyone using it?

I guess /proc/iomem prints it out? I set it in my current driver, if you
want to take a look.

> > 
> > > +	}
> > > +	busno = bus_range->start;
> > > +	pci_add_resource(&res, bus_range);
> > 
> > I currently truncate the bus range to fit inside the Configuration Space
> > window I have (in the reg property). How can I continue to do that with this
> > patch?
> 
> Not easily. Unless I add an argument to this function that will allow you to
> pass in the max number for the bus range, then the code becomes:
> 
> +	err = of_pci_parse_bus_range(parent->of_node, bus_range);
> +	if (err) {
> +		dev_info(parent, "No bus range for %s, using default [0-%d]\n",
> +			parent->of_node->full_name, max_range);
> +		bus_range->start = 0;
> +		bus_range->end = max_range;
> +		bus_range->flags = IORESOURCE_BUS;
> +	} else {
> +		if (bus_range->end > bus_range->start + max_range) {
> +			bus_range->end = bus_range->start + max_range;
> +		}
> +	}
> 
> Or something like that.

Again, take a look at my driver (it's in mainline now) to see how I deal
with this.

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

On Wednesday 02 July 2014 18:31:13 Will Deacon wrote:
> > > > + 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;
> > > 
> > > What about bus_range->name?
> > 
> > Don't know! Is anyone using it?
> 
> I guess /proc/iomem prints it out? I set it in my current driver, if you
> want to take a look.

I don't think the bus resources show up anywhere in procfs. Anyway, it's
always a good idea to give resources a name, if only for debugging purposes.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

On Tue, Jul 01, 2014 at 07:43:33PM +0100, 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>
> Tested-by: Tanmay Inamdar <tinamdar@apm.com>
> ---
>  drivers/of/of_pci.c       | 135 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/host-bridge.c |  18 +++++++
>  include/linux/of_pci.h    |  10 ++++
>  include/linux/pci.h       |   8 +++
>  4 files changed, 171 insertions(+)
> 
> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> index 8481996..55d8320 100644
> --- a/drivers/of/of_pci.c
> +++ b/drivers/of/of_pci.c
> @@ -89,6 +89,141 @@ int of_pci_parse_bus_range(struct device_node *node, struct resource *res)
>  }
>  EXPORT_SYMBOL_GPL(of_pci_parse_bus_range);
>  
> +/**
> + * 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.
> + *
> + * It is the callers job to free the @resources list if an error is returned.
> + *
> + * 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 cpu_addr:0x%016llx size:0x%016llx\n",
> +			range.pci_space, range.pci_addr, range.cpu_addr, range.size);

If you're not trying to match other printk formats, you could try to match
the %pR format used elsewhere, e.g., "%#010llx-%#010llx" with
range.cpu_addr, range.cpu_addr + range.size - 1.

> +
> +		/*
> +		 * 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)
> +			return -ENOMEM;
> +
> +		err = of_pci_range_to_resource(&range, dev, res);
> +		if (err)
> +			return err;
> +
> +		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 */
> +	return pcibios_fixup_bridge_ranges(resources);
> +}
> +
> +static atomic_t domain_nr = ATOMIC_INIT(-1);
> +
> +/**
> + * 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;

If you put the dev_info() down here, you can print &bus_range with %pR.

> +	}
> +	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)
> +		goto err_create;
> +
> +	/* then create the root bus */
> +	root_bus = pci_create_root_bus_in_domain(parent, domain, busno,
> +						ops, host_data, &res);
> +	if (IS_ERR(root_bus)) {
> +		err = PTR_ERR(root_bus);
> +		goto err_create;
> +	}
> +
> +	bridge = to_pci_host_bridge(root_bus->bridge);
> +	bridge->io_base = io_base;
> +
> +	return bridge;
> +
> +err_create:
> +	pci_free_resource_list(&res);
> +	return ERR_PTR(err);
> +}
> +EXPORT_SYMBOL_GPL(of_create_pci_host_bridge);
> +
>  #ifdef CONFIG_PCI_MSI
>  
>  static LIST_HEAD(of_pci_msi_chip_list);
> diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
> index 36c669e..cfee5d1 100644
> --- a/drivers/pci/host-bridge.c
> +++ b/drivers/pci/host-bridge.c
> @@ -5,6 +5,9 @@
>  #include <linux/kernel.h>
>  #include <linux/pci.h>
>  #include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_pci.h>
> +#include <linux/slab.h>
>  
>  #include "pci.h"
>  
> @@ -83,3 +86,18 @@ void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
>  	res->end = region->end + offset;
>  }
>  EXPORT_SYMBOL(pcibios_bus_to_resource);
> +
> +/**
> + * Simple version of the platform specific code for filtering the list
> + * of resources obtained from the ranges declaration in DT.
> + *
> + * Platforms can override this function in order to impose stronger
> + * constraints onto the list of resources that a host bridge can use.
> + * The filtered list will then be used to create a root bus and associate
> + * it with the host bridge.
> + *
> + */
> +int __weak pcibios_fixup_bridge_ranges(struct list_head *resources)
> +{
> +	return 0;
> +}

I'd wait to add this until there's a platform that needs to implement it.
Splitting it out will make this patch that much smaller and easier to
understand.

> diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
> index dde3a4a..71e36d0 100644
> --- a/include/linux/of_pci.h
> +++ b/include/linux/of_pci.h
> @@ -15,6 +15,9 @@ struct device_node *of_pci_find_child_device(struct device_node *parent,
>  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);
> +struct pci_host_bridge *of_create_pci_host_bridge(struct device *parent,
> +					struct pci_ops *ops, void *host_data);
> +
>  #else
>  static inline int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
>  {
> @@ -43,6 +46,13 @@ of_pci_parse_bus_range(struct device_node *node, struct resource *res)
>  {
>  	return -EINVAL;
>  }
> +
> +static inline struct pci_host_bridge *
> +of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops,
> +			void *host_data)
> +{
> +	return NULL;
> +}
>  #endif
>  
>  #if defined(CONFIG_OF) && defined(CONFIG_PCI_MSI)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 7e7b939..556dc5f 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -402,6 +402,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 */

I don't see where this is used yet.

As far as I know, there's nothing that prevents a host bridge from having
several I/O port apertures (or several memory-mapped I/O port spaces).

>  	struct list_head windows;	/* pci_host_bridge_windows */
>  	void (*release_fn)(struct pci_host_bridge *);
>  	void *release_data;
> @@ -1809,8 +1810,15 @@ 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) { }
> +
>  #endif  /* CONFIG_OF */
>  
> +/* Used by architecture code to apply any quirks to the list of
> + * pci_host_bridge resource ranges before they are being used
> + * by of_create_pci_host_bridge()
> + */
> +extern int pcibios_fixup_bridge_ranges(struct list_head *resources);
> +
>  #ifdef CONFIG_EEH
>  static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
>  {
> -- 
> 2.0.0
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

On Tue, Jul 08, 2014 at 02:01:04AM +0100, Bjorn Helgaas wrote:
> On Tue, Jul 01, 2014 at 07:43:33PM +0100, 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>
> > Tested-by: Tanmay Inamdar <tinamdar@apm.com>
> > ---
> >  drivers/of/of_pci.c       | 135 ++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/pci/host-bridge.c |  18 +++++++
> >  include/linux/of_pci.h    |  10 ++++
> >  include/linux/pci.h       |   8 +++
> >  4 files changed, 171 insertions(+)
> >
> > diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> > index 8481996..55d8320 100644
> > --- a/drivers/of/of_pci.c
> > +++ b/drivers/of/of_pci.c
> > @@ -89,6 +89,141 @@ int of_pci_parse_bus_range(struct device_node *node, struct resource *res)
> >  }
> >  EXPORT_SYMBOL_GPL(of_pci_parse_bus_range);
> >
> > +/**
> > + * 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.
> > + *
> > + * It is the callers job to free the @resources list if an error is returned.
> > + *
> > + * 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 cpu_addr:0x%016llx size:0x%016llx\n",
> > +                     range.pci_space, range.pci_addr, range.cpu_addr, range.size);
> 
> If you're not trying to match other printk formats, you could try to match
> the %pR format used elsewhere, e.g., "%#010llx-%#010llx" with
> range.cpu_addr, range.cpu_addr + range.size - 1.

Yes, not a big fan of the ugly output it generates, but the output matches closely the ranges
definition in the device tree file so it is easy to validate that you are parsing the right
entry. I am happy to change it to shorten the cpu range message.

> 
> > +
> > +             /*
> > +              * 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)
> > +                     return -ENOMEM;
> > +
> > +             err = of_pci_range_to_resource(&range, dev, res);
> > +             if (err)
> > +                     return err;
> > +
> > +             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 */
> > +     return pcibios_fixup_bridge_ranges(resources);
> > +}
> > +
> > +static atomic_t domain_nr = ATOMIC_INIT(-1);
> > +
> > +/**
> > + * 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;
> 
> If you put the dev_info() down here, you can print &bus_range with %pR.

Sure, will do.

> 
> > +     }
> > +     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)
> > +             goto err_create;
> > +
> > +     /* then create the root bus */
> > +     root_bus = pci_create_root_bus_in_domain(parent, domain, busno,
> > +                                             ops, host_data, &res);
> > +     if (IS_ERR(root_bus)) {
> > +             err = PTR_ERR(root_bus);
> > +             goto err_create;
> > +     }
> > +
> > +     bridge = to_pci_host_bridge(root_bus->bridge);
> > +     bridge->io_base = io_base;
> > +
> > +     return bridge;
> > +
> > +err_create:
> > +     pci_free_resource_list(&res);
> > +     return ERR_PTR(err);
> > +}
> > +EXPORT_SYMBOL_GPL(of_create_pci_host_bridge);
> > +
> >  #ifdef CONFIG_PCI_MSI
> >
> >  static LIST_HEAD(of_pci_msi_chip_list);
> > diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
> > index 36c669e..cfee5d1 100644
> > --- a/drivers/pci/host-bridge.c
> > +++ b/drivers/pci/host-bridge.c
> > @@ -5,6 +5,9 @@
> >  #include <linux/kernel.h>
> >  #include <linux/pci.h>
> >  #include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_pci.h>
> > +#include <linux/slab.h>
> >
> >  #include "pci.h"
> >
> > @@ -83,3 +86,18 @@ void pcibios_bus_to_resource(struct pci_bus *bus, struct resource *res,
> >       res->end = region->end + offset;
> >  }
> >  EXPORT_SYMBOL(pcibios_bus_to_resource);
> > +
> > +/**
> > + * Simple version of the platform specific code for filtering the list
> > + * of resources obtained from the ranges declaration in DT.
> > + *
> > + * Platforms can override this function in order to impose stronger
> > + * constraints onto the list of resources that a host bridge can use.
> > + * The filtered list will then be used to create a root bus and associate
> > + * it with the host bridge.
> > + *
> > + */
> > +int __weak pcibios_fixup_bridge_ranges(struct list_head *resources)
> > +{
> > +     return 0;
> > +}
> 
> I'd wait to add this until there's a platform that needs to implement it.
> Splitting it out will make this patch that much smaller and easier to
> understand.

I need this as this is the default implementation (i.e. do nothing). Otherwise the
link phase will give errors. 

> 
> > diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
> > index dde3a4a..71e36d0 100644
> > --- a/include/linux/of_pci.h
> > +++ b/include/linux/of_pci.h
> > @@ -15,6 +15,9 @@ struct device_node *of_pci_find_child_device(struct device_node *parent,
> >  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);
> > +struct pci_host_bridge *of_create_pci_host_bridge(struct device *parent,
> > +                                     struct pci_ops *ops, void *host_data);
> > +
> >  #else
> >  static inline int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
> >  {
> > @@ -43,6 +46,13 @@ of_pci_parse_bus_range(struct device_node *node, struct resource *res)
> >  {
> >       return -EINVAL;
> >  }
> > +
> > +static inline struct pci_host_bridge *
> > +of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops,
> > +                     void *host_data)
> > +{
> > +     return NULL;
> > +}
> >  #endif
> >
> >  #if defined(CONFIG_OF) && defined(CONFIG_PCI_MSI)
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index 7e7b939..556dc5f 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -402,6 +402,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 */
> 
> I don't see where this is used yet.

It's used in pci_host_bridge_of_get_ranges() (earlier in this patch).

> 
> As far as I know, there's nothing that prevents a host bridge from having
> several I/O port apertures (or several memory-mapped I/O port spaces).

The pci_register_io_range() will give different offsets for each apperture.
I just need to make sure I don't overwrite io_base when parsing multiple IO
ranges.

Thanks for reviewing this,
Liviu

> 
> >       struct list_head windows;       /* pci_host_bridge_windows */
> >       void (*release_fn)(struct pci_host_bridge *);
> >       void *release_data;
> > @@ -1809,8 +1810,15 @@ 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) { }
> > +
> >  #endif  /* CONFIG_OF */
> >
> > +/* Used by architecture code to apply any quirks to the list of
> > + * pci_host_bridge resource ranges before they are being used
> > + * by of_create_pci_host_bridge()
> > + */
> > +extern int pcibios_fixup_bridge_ranges(struct list_head *resources);
> > +
> >  #ifdef CONFIG_EEH
> >  static inline struct eeh_dev *pci_dev_to_eeh_dev(struct pci_dev *pdev)
> >  {
> > --
> > 2.0.0
> >
>

On Tue, Jul 08, 2014 at 11:29:40AM +0100, Liviu Dudau wrote:
> On Tue, Jul 08, 2014 at 02:01:04AM +0100, Bjorn Helgaas wrote:
> > On Tue, Jul 01, 2014 at 07:43:33PM +0100, Liviu Dudau wrote:
> > > ...
> > > +     for_each_of_pci_range(&parser, &range) {
> > > +             /* Read next ranges element */
> > > +             pr_debug("pci_space: 0x%08x pci_addr:0x%016llx cpu_addr:0x%016llx size:0x%016llx\n",
> > > +                     range.pci_space, range.pci_addr, range.cpu_addr, range.size);
> > 
> > If you're not trying to match other printk formats, you could try to match
> > the %pR format used elsewhere, e.g., "%#010llx-%#010llx" with
> > range.cpu_addr, range.cpu_addr + range.size - 1.
> 
> Yes, not a big fan of the ugly output it generates, but the output matches closely the ranges
> definition in the device tree file so it is easy to validate that you are parsing the right
> entry. I am happy to change it to shorten the cpu range message.

If it already matches other device tree stuff, that's perfect.  I'm not
familiar with that.

> > > +int __weak pcibios_fixup_bridge_ranges(struct list_head *resources)
> > > +{
> > > +     return 0;
> > > +}
> > 
> > I'd wait to add this until there's a platform that needs to implement it.
> > Splitting it out will make this patch that much smaller and easier to
> > understand.
> 
> I need this as this is the default implementation (i.e. do nothing). Otherwise the
> link phase will give errors. 

I meant that you could remove the default implementation *and* the call of
it, since it currently does nothing.

> > > diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
> > > index dde3a4a..71e36d0 100644
> > > --- a/include/linux/of_pci.h
> > > +++ b/include/linux/of_pci.h
> > > @@ -15,6 +15,9 @@ struct device_node *of_pci_find_child_device(struct device_node *parent,
> > >  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);
> > > +struct pci_host_bridge *of_create_pci_host_bridge(struct device *parent,
> > > +                                     struct pci_ops *ops, void *host_data);
> > > +
> > >  #else
> > >  static inline int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
> > >  {
> > > @@ -43,6 +46,13 @@ of_pci_parse_bus_range(struct device_node *node, struct resource *res)
> > >  {
> > >       return -EINVAL;
> > >  }
> > > +
> > > +static inline struct pci_host_bridge *
> > > +of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops,
> > > +                     void *host_data)
> > > +{
> > > +     return NULL;
> > > +}
> > >  #endif
> > >
> > >  #if defined(CONFIG_OF) && defined(CONFIG_PCI_MSI)
> > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > index 7e7b939..556dc5f 100644
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -402,6 +402,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 */
> > 
> > I don't see where this is used yet.
> 
> It's used in pci_host_bridge_of_get_ranges() (earlier in this patch).

of_create_pci_host_bridge() fills in bridge->io_base, but I don't see
anything that ever *reads* bridge->io_base.

> > As far as I know, there's nothing that prevents a host bridge from having
> > several I/O port apertures (or several memory-mapped I/O port spaces).
> 
> The pci_register_io_range() will give different offsets for each apperture.
> I just need to make sure I don't overwrite io_base when parsing multiple IO
> ranges.

Let's continue this in the other thread where I'm trying to understand the
I/O apertures.  Obviously I'm still missing something if you can indeed
have multiple I/O apertures per bridge (because then only one of them can
start at I/O address 0 on PCI).

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Jul 08, 2014 at 10:33:05PM +0100, Bjorn Helgaas wrote:
> On Tue, Jul 08, 2014 at 11:29:40AM +0100, Liviu Dudau wrote:
> > On Tue, Jul 08, 2014 at 02:01:04AM +0100, Bjorn Helgaas wrote:
> > > On Tue, Jul 01, 2014 at 07:43:33PM +0100, Liviu Dudau wrote:
> > > > ...
> > > > +     for_each_of_pci_range(&parser, &range) {
> > > > +             /* Read next ranges element */
> > > > +             pr_debug("pci_space: 0x%08x pci_addr:0x%016llx cpu_addr:0x%016llx size:0x%016llx\n",
> > > > +                     range.pci_space, range.pci_addr, range.cpu_addr, range.size);
> > > 
> > > If you're not trying to match other printk formats, you could try to match
> > > the %pR format used elsewhere, e.g., "%#010llx-%#010llx" with
> > > range.cpu_addr, range.cpu_addr + range.size - 1.
> > 
> > Yes, not a big fan of the ugly output it generates, but the output matches closely the ranges
> > definition in the device tree file so it is easy to validate that you are parsing the right
> > entry. I am happy to change it to shorten the cpu range message.
> 
> If it already matches other device tree stuff, that's perfect.  I'm not
> familiar with that.
> 
> > > > +int __weak pcibios_fixup_bridge_ranges(struct list_head *resources)
> > > > +{
> > > > +     return 0;
> > > > +}
> > > 
> > > I'd wait to add this until there's a platform that needs to implement it.
> > > Splitting it out will make this patch that much smaller and easier to
> > > understand.
> > 
> > I need this as this is the default implementation (i.e. do nothing). Otherwise the
> > link phase will give errors. 
> 
> I meant that you could remove the default implementation *and* the call of
> it, since it currently does nothing.

True. But it looks like converting Will's pci-host-generic.c driver will make use of this.

> 
> > > > diff --git a/include/linux/of_pci.h b/include/linux/of_pci.h
> > > > index dde3a4a..71e36d0 100644
> > > > --- a/include/linux/of_pci.h
> > > > +++ b/include/linux/of_pci.h
> > > > @@ -15,6 +15,9 @@ struct device_node *of_pci_find_child_device(struct device_node *parent,
> > > >  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);
> > > > +struct pci_host_bridge *of_create_pci_host_bridge(struct device *parent,
> > > > +                                     struct pci_ops *ops, void *host_data);
> > > > +
> > > >  #else
> > > >  static inline int of_irq_parse_pci(const struct pci_dev *pdev, struct of_phandle_args *out_irq)
> > > >  {
> > > > @@ -43,6 +46,13 @@ of_pci_parse_bus_range(struct device_node *node, struct resource *res)
> > > >  {
> > > >       return -EINVAL;
> > > >  }
> > > > +
> > > > +static inline struct pci_host_bridge *
> > > > +of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops,
> > > > +                     void *host_data)
> > > > +{
> > > > +     return NULL;
> > > > +}
> > > >  #endif
> > > >
> > > >  #if defined(CONFIG_OF) && defined(CONFIG_PCI_MSI)
> > > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > > index 7e7b939..556dc5f 100644
> > > > --- a/include/linux/pci.h
> > > > +++ b/include/linux/pci.h
> > > > @@ -402,6 +402,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 */
> > > 
> > > I don't see where this is used yet.
> > 
> > It's used in pci_host_bridge_of_get_ranges() (earlier in this patch).
> 
> of_create_pci_host_bridge() fills in bridge->io_base, but I don't see
> anything that ever *reads* bridge->io_base.

Ah, understood. It is used by the host bridge drivers to set their ATR registers to the
correct CPU address values.

> 
> > > As far as I know, there's nothing that prevents a host bridge from having
> > > several I/O port apertures (or several memory-mapped I/O port spaces).
> > 
> > The pci_register_io_range() will give different offsets for each apperture.
> > I just need to make sure I don't overwrite io_base when parsing multiple IO
> > ranges.
> 
> Let's continue this in the other thread where I'm trying to understand the
> I/O apertures.  Obviously I'm still missing something if you can indeed
> have multiple I/O apertures per bridge (because then only one of them can
> start at I/O address 0 on PCI).

Sure.

Thanks,
Liviu

> 
> Bjorn
>

On Tue, Jul 08, 2014 at 11:27:38PM +0100, Liviu Dudau wrote:
> On Tue, Jul 08, 2014 at 10:33:05PM +0100, Bjorn Helgaas wrote:
> > On Tue, Jul 08, 2014 at 11:29:40AM +0100, Liviu Dudau wrote:
> > > On Tue, Jul 08, 2014 at 02:01:04AM +0100, Bjorn Helgaas wrote:
> > > > On Tue, Jul 01, 2014 at 07:43:33PM +0100, Liviu Dudau wrote:
> > > > > ...
> > > > > +int __weak pcibios_fixup_bridge_ranges(struct list_head *resources)
> > > > > +{
> > > > > +     return 0;
> > > > > +}
> > > > 
> > > > I'd wait to add this until there's a platform that needs to implement it.
> > > > Splitting it out will make this patch that much smaller and easier to
> > > > understand.
> > > 
> > > I need this as this is the default implementation (i.e. do nothing). Otherwise the
> > > link phase will give errors. 
> > 
> > I meant that you could remove the default implementation *and* the call of
> > it, since it currently does nothing.
> 
> True. But it looks like converting Will's pci-host-generic.c driver will make use of this.

I think we should move this part of the patch to the conversion of
that driver.  That will keep related changes grouped together, which 
makes them easier to review and easier to backport, e.g., to distro
kernels.

> > > > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > > > index 7e7b939..556dc5f 100644
> > > > > --- a/include/linux/pci.h
> > > > > +++ b/include/linux/pci.h
> > > > > @@ -402,6 +402,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 */
> > > > 
> > > > I don't see where this is used yet.
> > > 
> > > It's used in pci_host_bridge_of_get_ranges() (earlier in this patch).
> > 
> > of_create_pci_host_bridge() fills in bridge->io_base, but I don't see
> > anything that ever *reads* bridge->io_base.
> 
> Ah, understood. It is used by the host bridge drivers to set their ATR registers to the
> correct CPU address values.

Same applies here.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, Jul 08, 2014 at 11:37:37PM +0100, Bjorn Helgaas wrote:
> On Tue, Jul 08, 2014 at 11:27:38PM +0100, Liviu Dudau wrote:
> > On Tue, Jul 08, 2014 at 10:33:05PM +0100, Bjorn Helgaas wrote:
> > > On Tue, Jul 08, 2014 at 11:29:40AM +0100, Liviu Dudau wrote:
> > > > On Tue, Jul 08, 2014 at 02:01:04AM +0100, Bjorn Helgaas wrote:
> > > > > On Tue, Jul 01, 2014 at 07:43:33PM +0100, Liviu Dudau wrote:
> > > > > > ...
> > > > > > +int __weak pcibios_fixup_bridge_ranges(struct list_head *resources)
> > > > > > +{
> > > > > > +     return 0;
> > > > > > +}
> > > > > 
> > > > > I'd wait to add this until there's a platform that needs to implement it.
> > > > > Splitting it out will make this patch that much smaller and easier to
> > > > > understand.
> > > > 
> > > > I need this as this is the default implementation (i.e. do nothing). Otherwise the
> > > > link phase will give errors. 
> > > 
> > > I meant that you could remove the default implementation *and* the call of
> > > it, since it currently does nothing.
> > 
> > True. But it looks like converting Will's pci-host-generic.c driver will make use of this.
> 
> I think we should move this part of the patch to the conversion of
> that driver.  That will keep related changes grouped together, which 
> makes them easier to review and easier to backport, e.g., to distro
> kernels.
> 
> > > > > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > > > > index 7e7b939..556dc5f 100644
> > > > > > --- a/include/linux/pci.h
> > > > > > +++ b/include/linux/pci.h
> > > > > > @@ -402,6 +402,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 */
> > > > > 
> > > > > I don't see where this is used yet.
> > > > 
> > > > It's used in pci_host_bridge_of_get_ranges() (earlier in this patch).
> > > 
> > > of_create_pci_host_bridge() fills in bridge->io_base, but I don't see
> > > anything that ever *reads* bridge->io_base.
> > 
> > Ah, understood. It is used by the host bridge drivers to set their ATR registers to the
> > correct CPU address values.
> 
> Same applies here.

Except I have no idea which host bridge driver is going to be submitted first. There are
at least a couple of public series for HB drivers based on my series plus the one I have yet
to submit for my platform.

Maybe an associated documentation patch that explains the intended use for io_base could
persuade you on the value of keeping the io_base patch here?

Best regards,
Liviu

> 
> Bjorn
>

On Wednesday 09 July 2014, Liviu Dudau wrote:
> On Tue, Jul 08, 2014 at 11:37:37PM +0100, Bjorn Helgaas wrote:
> > On Tue, Jul 08, 2014 at 11:27:38PM +0100, Liviu Dudau wrote:
> > > > > > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > > > > > index 7e7b939..556dc5f 100644
> > > > > > > --- a/include/linux/pci.h
> > > > > > > +++ b/include/linux/pci.h
> > > > > > > @@ -402,6 +402,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 */
> > > > > > 
> > > > > > I don't see where this is used yet.
> > > > > 
> > > > > It's used in pci_host_bridge_of_get_ranges() (earlier in this patch).
> > > > 
> > > > of_create_pci_host_bridge() fills in bridge->io_base, but I don't see
> > > > anything that ever reads bridge->io_base.
> > > 
> > > Ah, understood. It is used by the host bridge drivers to set their ATR registers to the
> > > correct CPU address values.

Actually, as we just discovered with one of the pci_dw drivers, it may
be the wrong number: what you program in the ATR registers is not
necessarily the same as the address visible to the CPU. What you need
instead is the address at the bus immediately above the PCI host bridge.

I think for now we can leave out this part from common code and add
the infrastructure later. Host drivers can have their own loop
around the ranges if they need to set up these registers. Ideally
at least on arm64, they should be set up by the firmware already.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

On Wednesday, July 02, 2014 3:44 AM, 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>
> Tested-by: Tanmay Inamdar <tinamdar@apm.com>
> ---
>  drivers/of/of_pci.c       | 135 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/pci/host-bridge.c |  18 +++++++
>  include/linux/of_pci.h    |  10 ++++
>  include/linux/pci.h       |   8 +++
>  4 files changed, 171 insertions(+)
> 
> diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> index 8481996..55d8320 100644
> --- a/drivers/of/of_pci.c
> +++ b/drivers/of/of_pci.c

[.....]

> +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)
> +		goto err_create;
> +
> +	/* then create the root bus */
> +	root_bus = pci_create_root_bus_in_domain(parent, domain, busno,
> +						ops, host_data, &res);
> +	if (IS_ERR(root_bus)) {
> +		err = PTR_ERR(root_bus);
> +		goto err_create;
> +	}
> +
> +	bridge = to_pci_host_bridge(root_bus->bridge);
> +	bridge->io_base = io_base;

Hi Liviu Dudau,

Would you fix the following warning?

drivers/of/of_pci.c: In function 'of_create_pci_host_bridge'
drivers/of/of_pci.c:218:18: warning: 'of_base' may be used uninitialized in this function [-Wmaybe-uninitialized]
  bridge->io_base = io_base;

Best regards,
Jingoo Han

[.....]

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

On Fri, Jul 11, 2014 at 08:43:21AM +0100, Jingoo Han wrote:
> On Wednesday, July 02, 2014 3:44 AM, 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>
> > Tested-by: Tanmay Inamdar <tinamdar@apm.com>
> > ---
> >  drivers/of/of_pci.c       | 135 ++++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/pci/host-bridge.c |  18 +++++++
> >  include/linux/of_pci.h    |  10 ++++
> >  include/linux/pci.h       |   8 +++
> >  4 files changed, 171 insertions(+)
> > 
> > diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c
> > index 8481996..55d8320 100644
> > --- a/drivers/of/of_pci.c
> > +++ b/drivers/of/of_pci.c
> 
> [.....]
> 
> > +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)
> > +		goto err_create;
> > +
> > +	/* then create the root bus */
> > +	root_bus = pci_create_root_bus_in_domain(parent, domain, busno,
> > +						ops, host_data, &res);
> > +	if (IS_ERR(root_bus)) {
> > +		err = PTR_ERR(root_bus);
> > +		goto err_create;
> > +	}
> > +
> > +	bridge = to_pci_host_bridge(root_bus->bridge);
> > +	bridge->io_base = io_base;
> 
> Hi Liviu Dudau,
> 
> Would you fix the following warning?
> 
> drivers/of/of_pci.c: In function 'of_create_pci_host_bridge'
> drivers/of/of_pci.c:218:18: warning: 'of_base' may be used uninitialized in this function [-Wmaybe-uninitialized]
>   bridge->io_base = io_base;

Yes, I have a simple fix which is to set the initial value to zero when declaring the variable.

Best regards,
Liviu

> 
> Best regards,
> Jingoo Han
> 
> [.....]
> 
>