[PATCH v3 1/4] PCI: Add new method for registering PCI hosts

[PATCH v3 1/4] PCI: Add new method for registering PCI hosts

[Thierry Reding]

From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

This patch makes the existing pci_host_bridge structure a proper device
that is usable by PCI host drivers in a more standard way. In addition
to the existing pci_scan_bus, pci_scan_root_bus, pci_scan_root_bus_msi,
and pci_create_root_bus interfaces, this unfortunately means having to
add yet another interface doing basically the same thing, and add some
extra code in the initial step.

However, this time it's more likely to be extensible enough that we
won't have to do another one again in the future, and we should be
able to reduce code much more as a result.

The main idea is to pull the allocation of 'struct pci_host_bridge' out
of the registration, and let individual host drivers and architecture
code fill the members before calling the registration function.

There are a number of things we can do based on this:

* Use a single memory allocation for the driver-specific structure
  and the generic PCI host bridge
* consolidate the contents of driver specific structures by moving
  them into pci_host_bridge
* Add a consistent interface for removing a PCI host bridge again
  when unloading a host driver module
* Replace the architecture specific __weak pcibios_* functions with
  callbacks in a pci_host_bridge device
* Move common boilerplate code from host drivers into the generic
  function, based on contents of the structure
* Extend pci_host_bridge with additional members when needed without
  having to add arguments to pci_scan_*.
* Move members of struct pci_bus into pci_host_bridge to avoid
  having lots of identical copies.

As mentioned in a previous email, one open question is whether we want
to export a function for allocating a pci_host_bridge device in
combination with the per-device structure or let the driver itself
call kzalloc.

Changes in v3 (Thierry Reding):
- swap out pci_host_bridge_init() for pci_alloc_host_bridge() with an
  extra parameter specifying the size of the driver's private data
- rename pci_host_bridge_register() to pci_register_host_bridge() for
  more consistency with existing functions
- split patches into smaller chunks to make diff more readable

Changes in v2 (Thierry Reding):
- add a pci_host_bridge_init() helper that drivers can use to perform
  all the necessary steps to initialize the bridge
- rename pci_register_host() to pci_host_bridge_register() to reflect
  the naming used by other functions
- plug memory leak on registration failure

Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
I tried to keep the diff readable as requested, but had to place the new
pci_register_host_bridge() in a somewhat artificial location in order to
keep the diff algorithm from trying to make it appear as the replacement
of pci_create_root_bus() that it really is.

It's not all that bad in the end, because the file doesn't seem to have
a very strict ordering in the first place.

Thierry

 drivers/pci/probe.c | 238 +++++++++++++++++++++++++++++++---------------------
 include/linux/pci.h |   4 +
 2 files changed, 145 insertions(+), 97 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 93f280df3428..93583b389058 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -521,7 +521,7 @@ static void pci_release_host_bridge_dev(struct device *dev)
 	kfree(bridge);
 }
 
-static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
+static struct pci_host_bridge *pci_alloc_host_bridge(void)
 {
 	struct pci_host_bridge *bridge;
 
@@ -530,7 +530,7 @@ static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b)
 		return NULL;
 
 	INIT_LIST_HEAD(&bridge->windows);
-	bridge->bus = b;
+
 	return bridge;
 }
 
@@ -717,6 +717,122 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus)
 	dev_set_msi_domain(&bus->dev, d);
 }
 
+static int pci_register_host_bridge(struct pci_host_bridge *bridge)
+{
+	struct device *parent = bridge->dev.parent;
+	struct resource_entry *window, *n;
+	struct pci_bus *bus, *b;
+	resource_size_t offset;
+	LIST_HEAD(resources);
+	struct resource *res;
+	char addr[64], *fmt;
+	const char *name;
+	int err;
+
+	bus = pci_alloc_bus(NULL);
+	if (!bus)
+		return -ENOMEM;
+
+	bridge->bus = bus;
+
+	/* temporarily move resources off the list */
+	list_splice_init(&bridge->windows, &resources);
+	bus->sysdata = bridge->sysdata;
+	bus->msi = bridge->msi;
+	bus->ops = bridge->ops;
+	bus->number = bus->busn_res.start = bridge->busnr;
+#ifdef CONFIG_PCI_DOMAINS_GENERIC
+	bus->domain_nr = pci_bus_find_domain_nr(bus, parent);
+#endif
+
+	b = pci_find_bus(pci_domain_nr(bus), bridge->busnr);
+	if (b) {
+		/* If we already got to this bus through a different bridge, ignore it */
+		dev_dbg(&b->dev, "bus already known\n");
+		err = -EEXIST;
+		goto free;
+	}
+
+	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(bus),
+		     bridge->busnr);
+
+	err = pcibios_root_bridge_prepare(bridge);
+	if (err)
+		goto free;
+
+	err = device_register(&bridge->dev);
+	if (err)
+		put_device(&bridge->dev);
+
+	bus->bridge = get_device(&bridge->dev);
+	device_enable_async_suspend(bus->bridge);
+	pci_set_bus_of_node(bus);
+	pci_set_bus_msi_domain(bus);
+
+	if (!parent)
+		set_dev_node(bus->bridge, pcibus_to_node(bus));
+
+	bus->dev.class = &pcibus_class;
+	bus->dev.parent = bus->bridge;
+
+	dev_set_name(&bus->dev, "%04x:%02x", pci_domain_nr(bus), bus->number);
+	name = dev_name(&bus->dev);
+
+	err = device_register(&bus->dev);
+	if (err)
+		goto unregister;
+
+	pcibios_add_bus(bus);
+
+	/* Create legacy_io and legacy_mem files for this bus */
+	pci_create_legacy_files(bus);
+
+	if (parent)
+		dev_info(parent, "PCI host bridge to bus %s\n", name);
+	else
+		pr_info("PCI host bridge to bus %s\n", name);
+
+	/* Add initial resources to the bus */
+	resource_list_for_each_entry_safe(window, n, &resources) {
+		list_move_tail(&window->node, &bridge->windows);
+		offset = window->offset;
+		res = window->res;
+
+		if (res->flags & IORESOURCE_BUS)
+			pci_bus_insert_busn_res(bus, bus->number, res->end);
+		else
+			pci_bus_add_resource(bus, res, 0);
+
+		if (offset) {
+			if (resource_type(res) == IORESOURCE_IO)
+				fmt = " (bus address [%#06llx-%#06llx])";
+			else
+				fmt = " (bus address [%#010llx-%#010llx])";
+
+			snprintf(addr, sizeof(addr), fmt,
+				 (unsigned long long)(res->start - offset),
+				 (unsigned long long)(res->end - offset));
+		} else
+			addr[0] = '\0';
+
+		dev_info(&bus->dev, "root bus resource %pR%s\n", res, addr);
+	}
+
+	down_write(&pci_bus_sem);
+	list_add_tail(&bus->node, &pci_root_buses);
+	up_write(&pci_bus_sem);
+
+	return 0;
+
+unregister:
+	put_device(&bridge->dev);
+	device_unregister(&bridge->dev);
+
+free:
+	kfree(bus);
+	return err;
+}
+
 static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 					   struct pci_dev *bridge, int busnr)
 {
@@ -2126,113 +2242,43 @@ void __weak pcibios_remove_bus(struct pci_bus *bus)
 {
 }
 
-struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
-		struct pci_ops *ops, void *sysdata, struct list_head *resources)
+static struct pci_bus *pci_create_root_bus_msi(struct device *parent,
+		int bus, struct pci_ops *ops, void *sysdata,
+		struct list_head *resources, struct msi_controller *msi)
 {
 	int error;
 	struct pci_host_bridge *bridge;
-	struct pci_bus *b, *b2;
-	struct resource_entry *window, *n;
-	struct resource *res;
-	resource_size_t offset;
-	char bus_addr[64];
-	char *fmt;
-
-	b = pci_alloc_bus(NULL);
-	if (!b)
-		return NULL;
 
-	b->sysdata = sysdata;
-	b->ops = ops;
-	b->number = b->busn_res.start = bus;
-#ifdef CONFIG_PCI_DOMAINS_GENERIC
-	b->domain_nr = pci_bus_find_domain_nr(b, parent);
-#endif
-	b2 = pci_find_bus(pci_domain_nr(b), bus);
-	if (b2) {
-		/* If we already got to this bus through a different bridge, ignore it */
-		dev_dbg(&b2->dev, "bus already known\n");
-		goto err_out;
-	}
-
-	bridge = pci_alloc_host_bridge(b);
+	bridge = pci_alloc_host_bridge();
 	if (!bridge)
-		goto err_out;
+		return NULL;
 
 	bridge->dev.parent = parent;
 	bridge->dev.release = pci_release_host_bridge_dev;
-	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
-	error = pcibios_root_bridge_prepare(bridge);
-	if (error) {
-		kfree(bridge);
-		goto err_out;
-	}
-
-	error = device_register(&bridge->dev);
-	if (error) {
-		put_device(&bridge->dev);
-		goto err_out;
-	}
-	b->bridge = get_device(&bridge->dev);
-	device_enable_async_suspend(b->bridge);
-	pci_set_bus_of_node(b);
-	pci_set_bus_msi_domain(b);
 
-	if (!parent)
-		set_dev_node(b->bridge, pcibus_to_node(b));
-
-	b->dev.class = &pcibus_class;
-	b->dev.parent = b->bridge;
-	dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus);
-	error = device_register(&b->dev);
-	if (error)
-		goto class_dev_reg_err;
+	list_splice_init(resources, &bridge->windows);
+	bridge->sysdata = sysdata;
+	bridge->busnr = bus;
+	bridge->ops = ops;
+	bridge->msi = msi;
 
-	pcibios_add_bus(b);
-
-	/* Create legacy_io and legacy_mem files for this bus */
-	pci_create_legacy_files(b);
-
-	if (parent)
-		dev_info(parent, "PCI host bridge to bus %s\n", dev_name(&b->dev));
-	else
-		printk(KERN_INFO "PCI host bridge to bus %s\n", dev_name(&b->dev));
-
-	/* Add initial resources to the bus */
-	resource_list_for_each_entry_safe(window, n, resources) {
-		list_move_tail(&window->node, &bridge->windows);
-		res = window->res;
-		offset = window->offset;
-		if (res->flags & IORESOURCE_BUS)
-			pci_bus_insert_busn_res(b, bus, res->end);
-		else
-			pci_bus_add_resource(b, res, 0);
-		if (offset) {
-			if (resource_type(res) == IORESOURCE_IO)
-				fmt = " (bus address [%#06llx-%#06llx])";
-			else
-				fmt = " (bus address [%#010llx-%#010llx])";
-			snprintf(bus_addr, sizeof(bus_addr), fmt,
-				 (unsigned long long) (res->start - offset),
-				 (unsigned long long) (res->end - offset));
-		} else
-			bus_addr[0] = '\0';
-		dev_info(&b->dev, "root bus resource %pR%s\n", res, bus_addr);
-	}
+	error = pci_register_host_bridge(bridge);
+	if (error < 0)
+		goto err_out;
 
-	down_write(&pci_bus_sem);
-	list_add_tail(&b->node, &pci_root_buses);
-	up_write(&pci_bus_sem);
+	return bridge->bus;
 
-	return b;
-
-class_dev_reg_err:
-	put_device(&bridge->dev);
-	device_unregister(&bridge->dev);
 err_out:
-	kfree(b);
+	kfree(bridge);
 	return NULL;
 }
+
+struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
+		struct pci_ops *ops, void *sysdata, struct list_head *resources)
+{
+	return pci_create_root_bus_msi(parent, bus, ops, sysdata, resources,
+				       NULL);
+}
 EXPORT_SYMBOL_GPL(pci_create_root_bus);
 
 int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max)
@@ -2313,12 +2359,10 @@ struct pci_bus *pci_scan_root_bus_msi(struct device *parent, int bus,
 			break;
 		}
 
-	b = pci_create_root_bus(parent, bus, ops, sysdata, resources);
+	b = pci_create_root_bus_msi(parent, bus, ops, sysdata, resources, msi);
 	if (!b)
 		return NULL;
 
-	b->msi = msi;
-
 	if (!found) {
 		dev_info(&b->dev,
 		 "No busn resource found for root bus, will use [bus %02x-ff]\n",
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 2599a980340f..ccf298fad9e7 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -407,9 +407,13 @@ static inline int pci_channel_offline(struct pci_dev *pdev)
 struct pci_host_bridge {
 	struct device dev;
 	struct pci_bus *bus;		/* root bus */
+	struct pci_ops *ops;
+	void *sysdata;
+	int busnr;
 	struct list_head windows;	/* resource_entry */
 	void (*release_fn)(struct pci_host_bridge *);
 	void *release_data;
+	struct msi_controller *msi;
 	unsigned int ignore_reset_delay:1;	/* for entire hierarchy */
 	/* Resource alignment requirements */
 	resource_size_t (*align_resource)(struct pci_dev *dev,
-- 
2.9.0

[PATCH v3 2/4] PCI: Allow driver-specific data in host bridge

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Provide a way to allocate driver-specific data along with a PCI host
bridge structure. The bridge's ->private field points to this data.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/pci/probe.c | 9 ++++++---
 include/linux/pci.h | 1 +
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 93583b389058..ecf543014da3 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -521,16 +521,19 @@ static void pci_release_host_bridge_dev(struct device *dev)
 	kfree(bridge);
 }
 
-static struct pci_host_bridge *pci_alloc_host_bridge(void)
+static struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
 {
 	struct pci_host_bridge *bridge;
 
-	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
+	bridge = kzalloc(sizeof(*bridge) + priv, GFP_KERNEL);
 	if (!bridge)
 		return NULL;
 
 	INIT_LIST_HEAD(&bridge->windows);
 
+	if (priv)
+		bridge->private = &bridge[1];
+
 	return bridge;
 }
 
@@ -2249,7 +2252,7 @@ static struct pci_bus *pci_create_root_bus_msi(struct device *parent,
 	int error;
 	struct pci_host_bridge *bridge;
 
-	bridge = pci_alloc_host_bridge();
+	bridge = pci_alloc_host_bridge(0);
 	if (!bridge)
 		return NULL;
 
diff --git a/include/linux/pci.h b/include/linux/pci.h
index ccf298fad9e7..3aa7240800c8 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -421,6 +421,7 @@ struct pci_host_bridge {
 			resource_size_t start,
 			resource_size_t size,
 			resource_size_t align);
+	void *private;
 };
 
 #define	to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev)
-- 
2.9.0

Re: [PATCH v3 2/4] PCI: Allow driver-specific data in host bridge

[Lorenzo Pieralisi]

On Mon, Aug 15, 2016 at 05:36:45PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> Provide a way to allocate driver-specific data along with a PCI host
> bridge structure. The bridge's ->private field points to this data.
> 
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/pci/probe.c | 9 ++++++---
>  include/linux/pci.h | 1 +
>  2 files changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 93583b389058..ecf543014da3 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -521,16 +521,19 @@ static void pci_release_host_bridge_dev(struct device *dev)
>  	kfree(bridge);
>  }
>  
> -static struct pci_host_bridge *pci_alloc_host_bridge(void)
> +static struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
>  {
>  	struct pci_host_bridge *bridge;
>  
> -	bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> +	bridge = kzalloc(sizeof(*bridge) + priv, GFP_KERNEL);
>  	if (!bridge)
>  		return NULL;
>  
>  	INIT_LIST_HEAD(&bridge->windows);
>  
> +	if (priv)
> +		bridge->private = &bridge[1];

How about making private a zero length array ?

Lorenzo

> +
>  	return bridge;
>  }
>  
> @@ -2249,7 +2252,7 @@ static struct pci_bus *pci_create_root_bus_msi(struct device *parent,
>  	int error;
>  	struct pci_host_bridge *bridge;
>  
> -	bridge = pci_alloc_host_bridge();
> +	bridge = pci_alloc_host_bridge(0);
>  	if (!bridge)
>  		return NULL;
>  
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index ccf298fad9e7..3aa7240800c8 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -421,6 +421,7 @@ struct pci_host_bridge {
>  			resource_size_t start,
>  			resource_size_t size,
>  			resource_size_t align);
> +	void *private;
>  };
>  
>  #define	to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev)
> -- 
> 2.9.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 2/4] PCI: Allow driver-specific data in host bridge

[Arnd Bergmann]

On Friday, August 19, 2016 5:55:06 PM CEST Lorenzo Pieralisi wrote:
> > 
> > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > index 93583b389058..ecf543014da3 100644
> > --- a/drivers/pci/probe.c
> > +++ b/drivers/pci/probe.c
> > @@ -521,16 +521,19 @@ static void pci_release_host_bridge_dev(struct device *dev)
> >       kfree(bridge);
> >  }
> >  
> > -static struct pci_host_bridge *pci_alloc_host_bridge(void)
> > +static struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
> >  {
> >       struct pci_host_bridge *bridge;
> >  
> > -     bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> > +     bridge = kzalloc(sizeof(*bridge) + priv, GFP_KERNEL);
> >       if (!bridge)
> >               return NULL;
> >  
> >       INIT_LIST_HEAD(&bridge->windows);
> >  
> > +     if (priv)
> > +             bridge->private = &bridge[1];
> 
> How about making private a zero length array ?

Right, the member can actually be removed here if we want to, this
was just meant as a shorthand so we can avoid the cast or function
call in device drivers. I think either way makes sense.

Also, someone commented on a related issue in the previous version, and I
recommended to align it to ARCH_SLAB_MINALIGN. This might be done more
easily with the zero-length array using

struct pci_host_bridge {
	...

	unsigned long private[0] __attribute__((aligned(ARCH_SLAB_MINALIGN)));
};

Without this, we need a bit more computation, like
	
	bridge = kzalloc(ALIGN(sizeof(*bridge), ARCH_SLAB_MINALIGN) + priv, GFP_KERNEL);
	bridge->private = (void *)bridge + ALIGN(sizeof(*bridge);

or

static inline void *pci_bridge_private(struct pci_host_bridge *bridge)
{
	return (void *)bridge + ALIGN(sizeof(*bridge);
}
static inline struct pci_host_bridge *pci_bridge_from_private(void *priv)
{
	return priv - ALIGN(sizeof(*bridge);
}

The alignment is needed if the private structure contains any data that
we may want to transfer using DMA.

	Arnd

Re: [PATCH v3 2/4] PCI: Allow driver-specific data in host bridge

[Lorenzo Pieralisi]

On Mon, Aug 22, 2016 at 04:00:42PM +0200, Arnd Bergmann wrote:
> On Friday, August 19, 2016 5:55:06 PM CEST Lorenzo Pieralisi wrote:
> > > 
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index 93583b389058..ecf543014da3 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -521,16 +521,19 @@ static void pci_release_host_bridge_dev(struct device *dev)
> > >       kfree(bridge);
> > >  }
> > >  
> > > -static struct pci_host_bridge *pci_alloc_host_bridge(void)
> > > +static struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
> > >  {
> > >       struct pci_host_bridge *bridge;
> > >  
> > > -     bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> > > +     bridge = kzalloc(sizeof(*bridge) + priv, GFP_KERNEL);
> > >       if (!bridge)
> > >               return NULL;
> > >  
> > >       INIT_LIST_HEAD(&bridge->windows);
> > >  
> > > +     if (priv)
> > > +             bridge->private = &bridge[1];
> > 
> > How about making private a zero length array ?
> 
> Right, the member can actually be removed here if we want to, this
> was just meant as a shorthand so we can avoid the cast or function
> call in device drivers. I think either way makes sense.
> 
> Also, someone commented on a related issue in the previous version, and I
> recommended to align it to ARCH_SLAB_MINALIGN. This might be done more
> easily with the zero-length array using
> 
> struct pci_host_bridge {
> 	...
> 
> 	unsigned long private[0] __attribute__((aligned(ARCH_SLAB_MINALIGN)));
> };

Yes, it looks simpler and I personally prefer it but it is more style
than anything else, I thought it was worth mentioning it though.

Thanks,
Lorenzo

> Without this, we need a bit more computation, like
> 	
> 	bridge = kzalloc(ALIGN(sizeof(*bridge), ARCH_SLAB_MINALIGN) + priv, GFP_KERNEL);
> 	bridge->private = (void *)bridge + ALIGN(sizeof(*bridge);
> 
> or
> 
> static inline void *pci_bridge_private(struct pci_host_bridge *bridge)
> {
> 	return (void *)bridge + ALIGN(sizeof(*bridge);
> }
> static inline struct pci_host_bridge *pci_bridge_from_private(void *priv)
> {
> 	return priv - ALIGN(sizeof(*bridge);
> }
> 
> The alignment is needed if the private structure contains any data that
> we may want to transfer using DMA.
> 
> 	Arnd
> 

Re: [PATCH v3 2/4] PCI: Allow driver-specific data in host bridge

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2840 bytes --]

On Mon, Aug 22, 2016 at 04:00:42PM +0200, Arnd Bergmann wrote:
> On Friday, August 19, 2016 5:55:06 PM CEST Lorenzo Pieralisi wrote:
> > > 
> > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > index 93583b389058..ecf543014da3 100644
> > > --- a/drivers/pci/probe.c
> > > +++ b/drivers/pci/probe.c
> > > @@ -521,16 +521,19 @@ static void pci_release_host_bridge_dev(struct device *dev)
> > >       kfree(bridge);
> > >  }
> > >  
> > > -static struct pci_host_bridge *pci_alloc_host_bridge(void)
> > > +static struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
> > >  {
> > >       struct pci_host_bridge *bridge;
> > >  
> > > -     bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> > > +     bridge = kzalloc(sizeof(*bridge) + priv, GFP_KERNEL);
> > >       if (!bridge)
> > >               return NULL;
> > >  
> > >       INIT_LIST_HEAD(&bridge->windows);
> > >  
> > > +     if (priv)
> > > +             bridge->private = &bridge[1];
> > 
> > How about making private a zero length array ?
> 
> Right, the member can actually be removed here if we want to, this
> was just meant as a shorthand so we can avoid the cast or function
> call in device drivers. I think either way makes sense.
> 
> Also, someone commented on a related issue in the previous version, and I
> recommended to align it to ARCH_SLAB_MINALIGN. This might be done more
> easily with the zero-length array using
> 
> struct pci_host_bridge {
> 	...
> 
> 	unsigned long private[0] __attribute__((aligned(ARCH_SLAB_MINALIGN)));
> };
> 
> Without this, we need a bit more computation, like
> 	
> 	bridge = kzalloc(ALIGN(sizeof(*bridge), ARCH_SLAB_MINALIGN) + priv, GFP_KERNEL);
> 	bridge->private = (void *)bridge + ALIGN(sizeof(*bridge);
> 
> or
> 
> static inline void *pci_bridge_private(struct pci_host_bridge *bridge)
> {
> 	return (void *)bridge + ALIGN(sizeof(*bridge);
> }
> static inline struct pci_host_bridge *pci_bridge_from_private(void *priv)
> {
> 	return priv - ALIGN(sizeof(*bridge);
> }
> 
> The alignment is needed if the private structure contains any data that
> we may want to transfer using DMA.

Looks like I never replied to this, though I seem to remember that I
did...

My question is: why would we want the private structure to be aligned?
Surely if somebody has data in the private structure that needs to be
transferred using DMA then they need to make sure that that field will
be properly aligned. Aligning the private structure itself won't ensure
that, right?

Anyway, it seems like there's very little contentious about this other
than the minor issue of how to get at the private data. I'd like to make
progress on this because we're now three generations into 64-bit with
Tegra and still missing PCI support on all of them.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]

Re: [PATCH v3 2/4] PCI: Allow driver-specific data in host bridge

[Arnd Bergmann]

On Friday, November 25, 2016 8:26:29 AM CET Thierry Reding wrote:
> On Mon, Aug 22, 2016 at 04:00:42PM +0200, Arnd Bergmann wrote:
> > On Friday, August 19, 2016 5:55:06 PM CEST Lorenzo Pieralisi wrote:
> > > > 
> > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> > > > index 93583b389058..ecf543014da3 100644
> > > > --- a/drivers/pci/probe.c
> > > > +++ b/drivers/pci/probe.c
> > > > @@ -521,16 +521,19 @@ static void pci_release_host_bridge_dev(struct device *dev)
> > > >       kfree(bridge);
> > > >  }
> > > >  
> > > > -static struct pci_host_bridge *pci_alloc_host_bridge(void)
> > > > +static struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
> > > >  {
> > > >       struct pci_host_bridge *bridge;
> > > >  
> > > > -     bridge = kzalloc(sizeof(*bridge), GFP_KERNEL);
> > > > +     bridge = kzalloc(sizeof(*bridge) + priv, GFP_KERNEL);
> > > >       if (!bridge)
> > > >               return NULL;
> > > >  
> > > >       INIT_LIST_HEAD(&bridge->windows);
> > > >  
> > > > +     if (priv)
> > > > +             bridge->private = &bridge[1];
> > > 
> > > How about making private a zero length array ?
> > 
> > Right, the member can actually be removed here if we want to, this
> > was just meant as a shorthand so we can avoid the cast or function
> > call in device drivers. I think either way makes sense.
> > 
> > Also, someone commented on a related issue in the previous version, and I
> > recommended to align it to ARCH_SLAB_MINALIGN. This might be done more
> > easily with the zero-length array using
> > 
> > struct pci_host_bridge {
> > 	...
> > 
> > 	unsigned long private[0] __attribute__((aligned(ARCH_SLAB_MINALIGN)));
> > };
> > 
> > Without this, we need a bit more computation, like
> > 	
> > 	bridge = kzalloc(ALIGN(sizeof(*bridge), ARCH_SLAB_MINALIGN) + priv, GFP_KERNEL);
> > 	bridge->private = (void *)bridge + ALIGN(sizeof(*bridge);
> > 
> > or
> > 
> > static inline void *pci_bridge_private(struct pci_host_bridge *bridge)
> > {
> > 	return (void *)bridge + ALIGN(sizeof(*bridge);
> > }
> > static inline struct pci_host_bridge *pci_bridge_from_private(void *priv)
> > {
> > 	return priv - ALIGN(sizeof(*bridge);
> > }
> > 
> > The alignment is needed if the private structure contains any data that
> > we may want to transfer using DMA.
> 
> Looks like I never replied to this, though I seem to remember that I
> did...
> 
> My question is: why would we want the private structure to be aligned?
> Surely if somebody has data in the private structure that needs to be
> transferred using DMA then they need to make sure that that field will
> be properly aligned. Aligning the private structure itself won't ensure
> that, right?

You need both of them. Having the field aligned in the structure when
the structure itself is misaligned will break in surprising ways.

It's possible that we don't ever need this for PCI hosts the way we
do need it for ethernet drivers.
 
> Anyway, it seems like there's very little contentious about this other
> than the minor issue of how to get at the private data. I'd like to make
> progress on this because we're now three generations into 64-bit with
> Tegra and still missing PCI support on all of them.


Yes, that would be good.

	Arnd

[PATCH v3 3/4] PCI: Make host bridge interface publicly available

[Thierry Reding]

From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Allow PCI host bridge drivers to use the new host bridge interfaces to
register their host bridge.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/pci/probe.c | 6 ++++--
 include/linux/pci.h | 2 ++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index ecf543014da3..4e05e703d1de 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -521,7 +521,7 @@ static void pci_release_host_bridge_dev(struct device *dev)
 	kfree(bridge);
 }
 
-static struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
+struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
 {
 	struct pci_host_bridge *bridge;
 
@@ -536,6 +536,7 @@ static struct pci_host_bridge *pci_alloc_host_bridge(size_t priv)
 
 	return bridge;
 }
+EXPORT_SYMBOL(pci_alloc_host_bridge);
 
 static const unsigned char pcix_bus_speed[] = {
 	PCI_SPEED_UNKNOWN,		/* 0 */
@@ -720,7 +721,7 @@ static void pci_set_bus_msi_domain(struct pci_bus *bus)
 	dev_set_msi_domain(&bus->dev, d);
 }
 
-static int pci_register_host_bridge(struct pci_host_bridge *bridge)
+int pci_register_host_bridge(struct pci_host_bridge *bridge)
 {
 	struct device *parent = bridge->dev.parent;
 	struct resource_entry *window, *n;
@@ -835,6 +836,7 @@ free:
 	kfree(bus);
 	return err;
 }
+EXPORT_SYMBOL(pci_register_host_bridge);
 
 static struct pci_bus *pci_alloc_child_bus(struct pci_bus *parent,
 					   struct pci_dev *bridge, int busnr)
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 3aa7240800c8..2e22ee99c841 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -426,6 +426,8 @@ struct pci_host_bridge {
 
 #define	to_pci_host_bridge(n) container_of(n, struct pci_host_bridge, dev)
 
+struct pci_host_bridge *pci_alloc_host_bridge(size_t priv);
+int pci_register_host_bridge(struct pci_host_bridge *bridge);
 struct pci_host_bridge *pci_find_host_bridge(struct pci_bus *bus);
 
 void pci_set_host_bridge_release(struct pci_host_bridge *bridge,
-- 
2.9.0

[PATCH v3 4/4] PCI: tegra: Use new pci_register_host_bridge() interface

[Thierry Reding]

From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>

Tegra is one of the remaining platforms that still use the traditional
pci_common_init_dev() interface for probing PCI host bridges.

This demonstrates how to convert it to the pci_register_host interface
I just added in a previous patch. This leads to a more linear probe
sequence that can handle errors better because we avoid callbacks into
the driver, and it makes the driver architecture independent.

As a side note, I should mention that I noticed this driver does not
register any IORESOURCE_IO resource with the bus, but instead registers
the I/O port window as a memory resource, which is surely a bug.

Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/pci/host/pci-tegra.c | 99 +++++++++++++++++++++++---------------------
 1 file changed, 52 insertions(+), 47 deletions(-)

diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
index 2d520755b1d7..6737d1be9798 100644
--- a/drivers/pci/host/pci-tegra.c
+++ b/drivers/pci/host/pci-tegra.c
@@ -260,6 +260,7 @@ static inline struct tegra_msi *to_tegra_msi(struct msi_controller *chip)
 }
 
 struct tegra_pcie {
+	struct pci_host_bridge *bridge;
 	struct device *dev;
 
 	void __iomem *pads;
@@ -322,11 +323,6 @@ struct tegra_pcie_bus {
 	unsigned int nr;
 };
 
-static inline struct tegra_pcie *sys_to_pcie(struct pci_sys_data *sys)
-{
-	return sys->private_data;
-}
-
 static inline void afi_writel(struct tegra_pcie *pcie, u32 value,
 			      unsigned long offset)
 {
@@ -430,7 +426,8 @@ free:
 
 static int tegra_pcie_add_bus(struct pci_bus *bus)
 {
-	struct tegra_pcie *pcie = sys_to_pcie(bus->sysdata);
+	struct pci_host_bridge *host = pci_find_host_bridge(bus);
+	struct tegra_pcie *pcie = host->private;
 	struct tegra_pcie_bus *b;
 
 	b = tegra_pcie_bus_alloc(pcie, bus->number);
@@ -444,7 +441,8 @@ static int tegra_pcie_add_bus(struct pci_bus *bus)
 
 static void tegra_pcie_remove_bus(struct pci_bus *child)
 {
-	struct tegra_pcie *pcie = sys_to_pcie(child->sysdata);
+	struct pci_host_bridge *host = pci_find_host_bridge(child);
+	struct tegra_pcie *pcie = host->private;
 	struct tegra_pcie_bus *bus, *tmp;
 
 	list_for_each_entry_safe(bus, tmp, &pcie->buses, list) {
@@ -461,7 +459,8 @@ static void __iomem *tegra_pcie_map_bus(struct pci_bus *bus,
 					unsigned int devfn,
 					int where)
 {
-	struct tegra_pcie *pcie = sys_to_pcie(bus->sysdata);
+	struct pci_host_bridge *host = pci_find_host_bridge(bus);
+	struct tegra_pcie *pcie = host->private;
 	void __iomem *addr = NULL;
 
 	if (bus->number == 0) {
@@ -609,35 +608,29 @@ static void tegra_pcie_relax_enable(struct pci_dev *dev)
 }
 DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, tegra_pcie_relax_enable);
 
-static int tegra_pcie_setup(int nr, struct pci_sys_data *sys)
+static int tegra_pcie_request_resources(struct tegra_pcie *pcie)
 {
-	struct tegra_pcie *pcie = sys_to_pcie(sys);
+	struct list_head *windows = &pcie->bridge->windows;
 	int err;
 
-	sys->mem_offset = pcie->offset.mem;
-	sys->io_offset = pcie->offset.io;
-
-	err = devm_request_resource(pcie->dev, &iomem_resource, &pcie->io);
-	if (err < 0)
-		return err;
-
-	pci_add_resource_offset(&sys->resources, &pcie->pio, sys->io_offset);
-	pci_add_resource_offset(&sys->resources, &pcie->mem, sys->mem_offset);
-	pci_add_resource_offset(&sys->resources, &pcie->prefetch,
-				sys->mem_offset);
-	pci_add_resource(&sys->resources, &pcie->busn);
+	pci_add_resource_offset(windows, &pcie->pio, pcie->offset.io);
+	pci_add_resource_offset(windows, &pcie->mem, pcie->offset.mem);
+	pci_add_resource_offset(windows, &pcie->prefetch, pcie->offset.mem);
+	pci_add_resource(windows, &pcie->busn);
 
-	err = devm_request_pci_bus_resources(pcie->dev, &sys->resources);
+	err = devm_request_pci_bus_resources(pcie->dev, windows);
 	if (err < 0)
 		return err;
 
 	pci_remap_iospace(&pcie->pio, pcie->io.start);
-	return 1;
+
+	return 0;
 }
 
 static int tegra_pcie_map_irq(const struct pci_dev *pdev, u8 slot, u8 pin)
 {
-	struct tegra_pcie *pcie = sys_to_pcie(pdev->bus->sysdata);
+	struct pci_host_bridge *host = pci_find_host_bridge(pdev->bus);
+	struct tegra_pcie *pcie = host->private;
 	int irq;
 
 	tegra_cpuidle_pcie_irqs_in_use();
@@ -1544,6 +1537,8 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
 	reg |= AFI_INTR_MASK_MSI_MASK;
 	afi_writel(pcie, reg, AFI_INTR_MASK);
 
+	pcie->bridge->msi = &msi->chip;
+
 	return 0;
 
 err:
@@ -2006,10 +2001,9 @@ retry:
 	return false;
 }
 
-static int tegra_pcie_enable(struct tegra_pcie *pcie)
+static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
 {
 	struct tegra_pcie_port *port, *tmp;
-	struct hw_pci hw;
 
 	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
 		dev_info(pcie->dev, "probing port %u, using %u lanes\n",
@@ -2025,22 +2019,6 @@ static int tegra_pcie_enable(struct tegra_pcie *pcie)
 		tegra_pcie_port_disable(port);
 		tegra_pcie_port_free(port);
 	}
-
-	memset(&hw, 0, sizeof(hw));
-
-#ifdef CONFIG_PCI_MSI
-	hw.msi_ctrl = &pcie->msi.chip;
-#endif
-
-	hw.nr_controllers = 1;
-	hw.private_data = (void **)&pcie;
-	hw.setup = tegra_pcie_setup;
-	hw.map_irq = tegra_pcie_map_irq;
-	hw.ops = &tegra_pcie_ops;
-
-	pci_common_init_dev(pcie->dev, &hw);
-
-	return 0;
 }
 
 static const struct tegra_pcie_soc tegra20_pcie = {
@@ -2201,13 +2179,18 @@ remove:
 
 static int tegra_pcie_probe(struct platform_device *pdev)
 {
+	struct pci_host_bridge *bridge;
 	struct tegra_pcie *pcie;
+	struct pci_bus *child;
 	int err;
 
-	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
-	if (!pcie)
+	bridge = pci_alloc_host_bridge(sizeof(*pcie));
+	if (!bridge)
 		return -ENOMEM;
 
+	pcie = bridge->private;
+	pcie->bridge = bridge;
+
 	pcie->soc = of_device_get_match_data(&pdev->dev);
 	INIT_LIST_HEAD(&pcie->buses);
 	INIT_LIST_HEAD(&pcie->ports);
@@ -2227,6 +2210,10 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 	if (err)
 		goto put_resources;
 
+	err = tegra_pcie_request_resources(pcie);
+	if (err)
+		goto put_resources;
+
 	/* setup the AFI address translations */
 	tegra_pcie_setup_translations(pcie);
 
@@ -2240,12 +2227,30 @@ static int tegra_pcie_probe(struct platform_device *pdev)
 		}
 	}
 
-	err = tegra_pcie_enable(pcie);
+	tegra_pcie_enable_ports(pcie);
+
+	pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS);
+	bridge->busnr = pcie->busn.start;
+	bridge->dev.parent = &pdev->dev;
+	bridge->ops = &tegra_pcie_ops;
+
+	err = pci_register_host_bridge(bridge);
 	if (err < 0) {
-		dev_err(&pdev->dev, "failed to enable PCIe ports: %d\n", err);
+		dev_err(&pdev->dev, "failed to register host: %d\n", err);
 		goto disable_msi;
 	}
 
+	pci_scan_child_bus(bridge->bus);
+
+	pci_fixup_irqs(pci_common_swizzle, tegra_pcie_map_irq);
+	pci_bus_size_bridges(bridge->bus);
+	pci_bus_assign_resources(bridge->bus);
+
+	list_for_each_entry(child, &bridge->bus->children, node)
+		pcie_bus_configure_settings(child);
+
+	pci_bus_add_devices(bridge->bus);
+
 	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
 		err = tegra_pcie_debugfs_init(pcie);
 		if (err < 0)
-- 
2.9.0

Re: [PATCH v3 4/4] PCI: tegra: Use new pci_register_host_bridge() interface

[Lorenzo Pieralisi]

On Mon, Aug 15, 2016 at 05:36:47PM +0200, Thierry Reding wrote:
> From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> 
> Tegra is one of the remaining platforms that still use the traditional
> pci_common_init_dev() interface for probing PCI host bridges.
> 
> This demonstrates how to convert it to the pci_register_host interface
> I just added in a previous patch. This leads to a more linear probe
> sequence that can handle errors better because we avoid callbacks into
> the driver, and it makes the driver architecture independent.
> 
> As a side note, I should mention that I noticed this driver does not
> register any IORESOURCE_IO resource with the bus, but instead registers
> the I/O port window as a memory resource, which is surely a bug.

I do not think that's true (and these comments do not belong in
a commit log anyway). It registers both; granted the way
the resources are named is a bit misleading, but by looking
at the code it seems correct to me (struct tegra_pcie.{io/pio}).

> Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/pci/host/pci-tegra.c | 99 +++++++++++++++++++++++---------------------
>  1 file changed, 52 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> index 2d520755b1d7..6737d1be9798 100644
> --- a/drivers/pci/host/pci-tegra.c
> +++ b/drivers/pci/host/pci-tegra.c
> @@ -260,6 +260,7 @@ static inline struct tegra_msi *to_tegra_msi(struct msi_controller *chip)
>  }
>  
>  struct tegra_pcie {
> +	struct pci_host_bridge *bridge;

If we go for the zero length array approach we would remove this
pointer too, since it would be superfluos, a container_of would
just do, right ?

Lorenzo

>  	struct device *dev;
>  
>  	void __iomem *pads;
> @@ -322,11 +323,6 @@ struct tegra_pcie_bus {
>  	unsigned int nr;
>  };
>  
> -static inline struct tegra_pcie *sys_to_pcie(struct pci_sys_data *sys)
> -{
> -	return sys->private_data;
> -}
> -
>  static inline void afi_writel(struct tegra_pcie *pcie, u32 value,
>  			      unsigned long offset)
>  {
> @@ -430,7 +426,8 @@ free:
>  
>  static int tegra_pcie_add_bus(struct pci_bus *bus)
>  {
> -	struct tegra_pcie *pcie = sys_to_pcie(bus->sysdata);
> +	struct pci_host_bridge *host = pci_find_host_bridge(bus);
> +	struct tegra_pcie *pcie = host->private;
>  	struct tegra_pcie_bus *b;
>  
>  	b = tegra_pcie_bus_alloc(pcie, bus->number);
> @@ -444,7 +441,8 @@ static int tegra_pcie_add_bus(struct pci_bus *bus)
>  
>  static void tegra_pcie_remove_bus(struct pci_bus *child)
>  {
> -	struct tegra_pcie *pcie = sys_to_pcie(child->sysdata);
> +	struct pci_host_bridge *host = pci_find_host_bridge(child);
> +	struct tegra_pcie *pcie = host->private;
>  	struct tegra_pcie_bus *bus, *tmp;
>  
>  	list_for_each_entry_safe(bus, tmp, &pcie->buses, list) {
> @@ -461,7 +459,8 @@ static void __iomem *tegra_pcie_map_bus(struct pci_bus *bus,
>  					unsigned int devfn,
>  					int where)
>  {
> -	struct tegra_pcie *pcie = sys_to_pcie(bus->sysdata);
> +	struct pci_host_bridge *host = pci_find_host_bridge(bus);
> +	struct tegra_pcie *pcie = host->private;
>  	void __iomem *addr = NULL;
>  
>  	if (bus->number == 0) {
> @@ -609,35 +608,29 @@ static void tegra_pcie_relax_enable(struct pci_dev *dev)
>  }
>  DECLARE_PCI_FIXUP_FINAL(PCI_ANY_ID, PCI_ANY_ID, tegra_pcie_relax_enable);
>  
> -static int tegra_pcie_setup(int nr, struct pci_sys_data *sys)
> +static int tegra_pcie_request_resources(struct tegra_pcie *pcie)
>  {
> -	struct tegra_pcie *pcie = sys_to_pcie(sys);
> +	struct list_head *windows = &pcie->bridge->windows;
>  	int err;
>  
> -	sys->mem_offset = pcie->offset.mem;
> -	sys->io_offset = pcie->offset.io;
> -
> -	err = devm_request_resource(pcie->dev, &iomem_resource, &pcie->io);
> -	if (err < 0)
> -		return err;
> -
> -	pci_add_resource_offset(&sys->resources, &pcie->pio, sys->io_offset);
> -	pci_add_resource_offset(&sys->resources, &pcie->mem, sys->mem_offset);
> -	pci_add_resource_offset(&sys->resources, &pcie->prefetch,
> -				sys->mem_offset);
> -	pci_add_resource(&sys->resources, &pcie->busn);
> +	pci_add_resource_offset(windows, &pcie->pio, pcie->offset.io);
> +	pci_add_resource_offset(windows, &pcie->mem, pcie->offset.mem);
> +	pci_add_resource_offset(windows, &pcie->prefetch, pcie->offset.mem);
> +	pci_add_resource(windows, &pcie->busn);
>  
> -	err = devm_request_pci_bus_resources(pcie->dev, &sys->resources);
> +	err = devm_request_pci_bus_resources(pcie->dev, windows);
>  	if (err < 0)
>  		return err;
>  
>  	pci_remap_iospace(&pcie->pio, pcie->io.start);
> -	return 1;
> +
> +	return 0;
>  }
>  
>  static int tegra_pcie_map_irq(const struct pci_dev *pdev, u8 slot, u8 pin)
>  {
> -	struct tegra_pcie *pcie = sys_to_pcie(pdev->bus->sysdata);
> +	struct pci_host_bridge *host = pci_find_host_bridge(pdev->bus);
> +	struct tegra_pcie *pcie = host->private;
>  	int irq;
>  
>  	tegra_cpuidle_pcie_irqs_in_use();
> @@ -1544,6 +1537,8 @@ static int tegra_pcie_enable_msi(struct tegra_pcie *pcie)
>  	reg |= AFI_INTR_MASK_MSI_MASK;
>  	afi_writel(pcie, reg, AFI_INTR_MASK);
>  
> +	pcie->bridge->msi = &msi->chip;
> +
>  	return 0;
>  
>  err:
> @@ -2006,10 +2001,9 @@ retry:
>  	return false;
>  }
>  
> -static int tegra_pcie_enable(struct tegra_pcie *pcie)
> +static void tegra_pcie_enable_ports(struct tegra_pcie *pcie)
>  {
>  	struct tegra_pcie_port *port, *tmp;
> -	struct hw_pci hw;
>  
>  	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
>  		dev_info(pcie->dev, "probing port %u, using %u lanes\n",
> @@ -2025,22 +2019,6 @@ static int tegra_pcie_enable(struct tegra_pcie *pcie)
>  		tegra_pcie_port_disable(port);
>  		tegra_pcie_port_free(port);
>  	}
> -
> -	memset(&hw, 0, sizeof(hw));
> -
> -#ifdef CONFIG_PCI_MSI
> -	hw.msi_ctrl = &pcie->msi.chip;
> -#endif
> -
> -	hw.nr_controllers = 1;
> -	hw.private_data = (void **)&pcie;
> -	hw.setup = tegra_pcie_setup;
> -	hw.map_irq = tegra_pcie_map_irq;
> -	hw.ops = &tegra_pcie_ops;
> -
> -	pci_common_init_dev(pcie->dev, &hw);
> -
> -	return 0;
>  }
>  
>  static const struct tegra_pcie_soc tegra20_pcie = {
> @@ -2201,13 +2179,18 @@ remove:
>  
>  static int tegra_pcie_probe(struct platform_device *pdev)
>  {
> +	struct pci_host_bridge *bridge;
>  	struct tegra_pcie *pcie;
> +	struct pci_bus *child;
>  	int err;
>  
> -	pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
> -	if (!pcie)
> +	bridge = pci_alloc_host_bridge(sizeof(*pcie));
> +	if (!bridge)
>  		return -ENOMEM;
>  
> +	pcie = bridge->private;
> +	pcie->bridge = bridge;
> +
>  	pcie->soc = of_device_get_match_data(&pdev->dev);
>  	INIT_LIST_HEAD(&pcie->buses);
>  	INIT_LIST_HEAD(&pcie->ports);
> @@ -2227,6 +2210,10 @@ static int tegra_pcie_probe(struct platform_device *pdev)
>  	if (err)
>  		goto put_resources;
>  
> +	err = tegra_pcie_request_resources(pcie);
> +	if (err)
> +		goto put_resources;
> +
>  	/* setup the AFI address translations */
>  	tegra_pcie_setup_translations(pcie);
>  
> @@ -2240,12 +2227,30 @@ static int tegra_pcie_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> -	err = tegra_pcie_enable(pcie);
> +	tegra_pcie_enable_ports(pcie);
> +
> +	pci_add_flags(PCI_REASSIGN_ALL_RSRC | PCI_REASSIGN_ALL_BUS);
> +	bridge->busnr = pcie->busn.start;
> +	bridge->dev.parent = &pdev->dev;
> +	bridge->ops = &tegra_pcie_ops;
> +
> +	err = pci_register_host_bridge(bridge);
>  	if (err < 0) {
> -		dev_err(&pdev->dev, "failed to enable PCIe ports: %d\n", err);
> +		dev_err(&pdev->dev, "failed to register host: %d\n", err);
>  		goto disable_msi;
>  	}
>  
> +	pci_scan_child_bus(bridge->bus);
> +
> +	pci_fixup_irqs(pci_common_swizzle, tegra_pcie_map_irq);
> +	pci_bus_size_bridges(bridge->bus);
> +	pci_bus_assign_resources(bridge->bus);
> +
> +	list_for_each_entry(child, &bridge->bus->children, node)
> +		pcie_bus_configure_settings(child);
> +
> +	pci_bus_add_devices(bridge->bus);
> +
>  	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
>  		err = tegra_pcie_debugfs_init(pcie);
>  		if (err < 0)
> -- 
> 2.9.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v3 4/4] PCI: tegra: Use new pci_register_host_bridge() interface

[Arnd Bergmann]

On Friday, August 19, 2016 6:13:00 PM CEST Lorenzo Pieralisi wrote:
> On Mon, Aug 15, 2016 at 05:36:47PM +0200, Thierry Reding wrote:
> > From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> > 
> > Tegra is one of the remaining platforms that still use the traditional
> > pci_common_init_dev() interface for probing PCI host bridges.
> > 
> > This demonstrates how to convert it to the pci_register_host interface
> > I just added in a previous patch. This leads to a more linear probe
> > sequence that can handle errors better because we avoid callbacks into
> > the driver, and it makes the driver architecture independent.
> > 
> > As a side note, I should mention that I noticed this driver does not
> > register any IORESOURCE_IO resource with the bus, but instead registers
> > the I/O port window as a memory resource, which is surely a bug.
> 
> I do not think that's true (and these comments do not belong in
> a commit log anyway). It registers both; granted the way
> the resources are named is a bit misleading, but by looking
> at the code it seems correct to me (struct tegra_pcie.{io/pio}).

Hmm, I don't remember when or why I wrote this comment, but it seems
you are right. This was apparently fixed by 5106787a9e08 ("PCI: tegra:
Use physical range for I/O mapping"), which I reviewed and which
has been applied a long time ago, surely before I sent the first
version of this patch.

> > Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > ---
> >  drivers/pci/host/pci-tegra.c | 99 +++++++++++++++++++++++---------------------
> >  1 file changed, 52 insertions(+), 47 deletions(-)
> > 
> > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> > index 2d520755b1d7..6737d1be9798 100644
> > --- a/drivers/pci/host/pci-tegra.c
> > +++ b/drivers/pci/host/pci-tegra.c
> > @@ -260,6 +260,7 @@ static inline struct tegra_msi *to_tegra_msi(struct msi_controller *chip)
> >  }
> >  
> >  struct tegra_pcie {
> > +     struct pci_host_bridge *bridge;
> 
> If we go for the zero length array approach we would remove this
> pointer too, since it would be superfluos, a container_of would
> just do, right ?

Regardless of whether we have a zero-length array or the current
code, we can provide a helper function that computes the pointer
to the pci_host_bridge from the tegra_pcie pointer.

	Arnd

Re: [PATCH v3 4/4] PCI: tegra: Use new pci_register_host_bridge() interface

[Lorenzo Pieralisi]

On Mon, Aug 22, 2016 at 03:50:54PM +0200, Arnd Bergmann wrote:
> On Friday, August 19, 2016 6:13:00 PM CEST Lorenzo Pieralisi wrote:
> > On Mon, Aug 15, 2016 at 05:36:47PM +0200, Thierry Reding wrote:
> > > From: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> > > 
> > > Tegra is one of the remaining platforms that still use the traditional
> > > pci_common_init_dev() interface for probing PCI host bridges.
> > > 
> > > This demonstrates how to convert it to the pci_register_host interface
> > > I just added in a previous patch. This leads to a more linear probe
> > > sequence that can handle errors better because we avoid callbacks into
> > > the driver, and it makes the driver architecture independent.
> > > 
> > > As a side note, I should mention that I noticed this driver does not
> > > register any IORESOURCE_IO resource with the bus, but instead registers
> > > the I/O port window as a memory resource, which is surely a bug.
> > 
> > I do not think that's true (and these comments do not belong in
> > a commit log anyway). It registers both; granted the way
> > the resources are named is a bit misleading, but by looking
> > at the code it seems correct to me (struct tegra_pcie.{io/pio}).
> 
> Hmm, I don't remember when or why I wrote this comment, but it seems
> you are right. This was apparently fixed by 5106787a9e08 ("PCI: tegra:
> Use physical range for I/O mapping"), which I reviewed and which
> has been applied a long time ago, surely before I sent the first
> version of this patch.
> 
> > > Signed-off-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> > > Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > > ---
> > >  drivers/pci/host/pci-tegra.c | 99 +++++++++++++++++++++++---------------------
> > >  1 file changed, 52 insertions(+), 47 deletions(-)
> > > 
> > > diff --git a/drivers/pci/host/pci-tegra.c b/drivers/pci/host/pci-tegra.c
> > > index 2d520755b1d7..6737d1be9798 100644
> > > --- a/drivers/pci/host/pci-tegra.c
> > > +++ b/drivers/pci/host/pci-tegra.c
> > > @@ -260,6 +260,7 @@ static inline struct tegra_msi *to_tegra_msi(struct msi_controller *chip)
> > >  }
> > >  
> > >  struct tegra_pcie {
> > > +     struct pci_host_bridge *bridge;
> > 
> > If we go for the zero length array approach we would remove this
> > pointer too, since it would be superfluos, a container_of would
> > just do, right ?
> 
> Regardless of whether we have a zero-length array or the current
> code, we can provide a helper function that computes the pointer
> to the pci_host_bridge from the tegra_pcie pointer.

Yes, that's true as I said it is just a matter of choosing the
best way to implement it but the mechanism is pretty much identical.

Lorenzo