| Message ID | 1404240214-9804-7-git-send-email-Liviu.Dudau@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jul 01, 2014 at 07:43:31PM +0100, Liviu Dudau wrote: > Make it easier to discover the domain number of a bus by storing > the number in pci_host_bridge for the root bus. Several architectures > have their own way of storing this information, so it makes sense > to try to unify the code. While at this, add a new function that > creates a root bus in a given domain and make pci_create_root_bus() > a wrapper around this function. "While at this" is always a good clue that maybe something should be split into a separate patch :) This is a very good example, since it adds a new interface that deserves its own changelog. > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > Tested-by: Tanmay Inamdar <tinamdar@apm.com> > --- > drivers/pci/probe.c | 41 +++++++++++++++++++++++++++++++++-------- > include/linux/pci.h | 4 ++++ > 2 files changed, 37 insertions(+), 8 deletions(-) > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index 2c92662..abf5e82 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1748,8 +1748,9 @@ void __weak pcibios_remove_bus(struct pci_bus *bus) > { > } > > -struct pci_bus *pci_create_root_bus(struct device *parent, int bus, > - struct pci_ops *ops, void *sysdata, struct list_head *resources) > +struct pci_bus *pci_create_root_bus_in_domain(struct device *parent, > + int domain, int bus, struct pci_ops *ops, void *sysdata, > + struct list_head *resources) I don't think we should do it this way; this makes it possible to have a host bridge where "bridge->domain_nr != pci_domain_nr(bridge->bus)". I wonder if it would help to make a weak pci_domain_nr() function that returns "bridge->domain_nr". Then each arch could individually drop its pci_domain_nr() definition as it was converted, e.g., something like this: - Convert every arch pci_domain_nr() from a #define to a non-inline function - Add bridge.domain_nr, initialized from pci_domain_nr() - Add a weak generic pci_domain_nr() that returns bridge.domain_nr - Add a way to create a host bridge in a specified domain, so we can initialize bridge.domain_nr without using pci_domain_nr() - Convert each arch to use the new creation mechanism and drop its pci_domain_nr() implementation > { > int error; > struct pci_host_bridge *bridge; > @@ -1762,27 +1763,31 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, > > bridge = pci_alloc_host_bridge(); > if (!bridge) > - return NULL; > + return ERR_PTR(-ENOMEM); > > bridge->dev.parent = parent; > bridge->dev.release = pci_release_host_bridge_dev; > + bridge->domain_nr = domain; > > b = pci_alloc_bus(); > - if (!b) > + if (!b) { > + error = -ENOMEM; > goto err_out; > + } > > b->sysdata = sysdata; > b->ops = ops; > b->number = b->busn_res.start = bus; > - b2 = pci_find_bus(pci_domain_nr(b), bus); > + b2 = pci_find_bus(bridge->domain_nr, bus); > if (b2) { > /* If we already got to this bus through a different bridge, ignore it */ > dev_dbg(&b2->dev, "bus already known\n"); > + error = -EEXIST; > goto err_bus_out; > } > > bridge->bus = b; > - dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus); > + dev_set_name(&bridge->dev, "pci%04x:%02x", bridge->domain_nr, bus); > error = pcibios_root_bridge_prepare(bridge); > if (error) > goto err_out; > @@ -1801,7 +1806,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, > > b->dev.class = &pcibus_class; > b->dev.parent = b->bridge; > - dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus); > + dev_set_name(&b->dev, "%04x:%02x", bridge->domain_nr, bus); > error = device_register(&b->dev); > if (error) > goto class_dev_reg_err; > @@ -1851,7 +1856,27 @@ err_bus_out: > kfree(b); > err_out: > kfree(bridge); > - return NULL; > + return ERR_PTR(error); > +} > + > +struct pci_bus *pci_create_root_bus(struct device *parent, int bus, > + struct pci_ops *ops, void *sysdata, struct list_head *resources) > +{ > + int domain_nr; > + struct pci_bus *b = pci_alloc_bus(); > + if (!b) > + return NULL; > + > + b->sysdata = sysdata; > + domain_nr = pci_domain_nr(b); > + kfree(b); > + > + b = pci_create_root_bus_in_domain(parent, domain_nr, bus, > + ops, sysdata, resources); > + if (IS_ERR(b)) > + return NULL; > + > + return b; > } > > int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max) > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 466bcd1..7e7b939 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -401,6 +401,7 @@ struct pci_host_bridge_window { > struct pci_host_bridge { > struct device dev; > struct pci_bus *bus; /* root bus */ > + int domain_nr; > struct list_head windows; /* pci_host_bridge_windows */ > void (*release_fn)(struct pci_host_bridge *); > void *release_data; > @@ -769,6 +770,9 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata); > struct pci_bus *pci_create_root_bus(struct device *parent, int bus, > struct pci_ops *ops, void *sysdata, > struct list_head *resources); > +struct pci_bus *pci_create_root_bus_in_domain(struct device *parent, > + int domain, int bus, struct pci_ops *ops, > + void *sysdata, struct list_head *resources); > int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax); > int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax); > void pci_bus_release_busn_res(struct pci_bus *b); > -- > 2.0.0 >
On Tue, Jul 08, 2014 at 01:59:54AM +0100, Bjorn Helgaas wrote: > On Tue, Jul 01, 2014 at 07:43:31PM +0100, Liviu Dudau wrote: > > Make it easier to discover the domain number of a bus by storing > > the number in pci_host_bridge for the root bus. Several architectures > > have their own way of storing this information, so it makes sense > > to try to unify the code. While at this, add a new function that > > creates a root bus in a given domain and make pci_create_root_bus() > > a wrapper around this function. > > "While at this" is always a good clue that maybe something should be > split into a separate patch :) This is a very good example, since it > adds a new interface that deserves its own changelog. Yes, I'm coming to the same conclusion. :) > > > Signed-off-by: Liviu Dudau <Liviu.Dudau@arm.com> > > Tested-by: Tanmay Inamdar <tinamdar@apm.com> > > --- > > drivers/pci/probe.c | 41 +++++++++++++++++++++++++++++++++-------- > > include/linux/pci.h | 4 ++++ > > 2 files changed, 37 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > > index 2c92662..abf5e82 100644 > > --- a/drivers/pci/probe.c > > +++ b/drivers/pci/probe.c > > @@ -1748,8 +1748,9 @@ void __weak pcibios_remove_bus(struct pci_bus *bus) > > { > > } > > > > -struct pci_bus *pci_create_root_bus(struct device *parent, int bus, > > - struct pci_ops *ops, void *sysdata, struct list_head *resources) > > +struct pci_bus *pci_create_root_bus_in_domain(struct device *parent, > > + int domain, int bus, struct pci_ops *ops, void *sysdata, > > + struct list_head *resources) > > I don't think we should do it this way; this makes it possible to have a > host bridge where "bridge->domain_nr != pci_domain_nr(bridge->bus)". > > I wonder if it would help to make a weak pci_domain_nr() function that > returns "bridge->domain_nr". Then each arch could individually drop its > pci_domain_nr() definition as it was converted, e.g., something like this: > > - Convert every arch pci_domain_nr() from a #define to a non-inline > function > - Add bridge.domain_nr, initialized from pci_domain_nr() > - Add a weak generic pci_domain_nr() that returns bridge.domain_nr > - Add a way to create a host bridge in a specified domain, so we can > initialize bridge.domain_nr without using pci_domain_nr() > - Convert each arch to use the new creation mechanism and drop its > pci_domain_nr() implementation I will try to propose a patch implementing this. Best regards, Liviu > > > { > > int error; > > struct pci_host_bridge *bridge; > > @@ -1762,27 +1763,31 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, > > > > bridge = pci_alloc_host_bridge(); > > if (!bridge) > > - return NULL; > > + return ERR_PTR(-ENOMEM); > > > > bridge->dev.parent = parent; > > bridge->dev.release = pci_release_host_bridge_dev; > > + bridge->domain_nr = domain; > > > > b = pci_alloc_bus(); > > - if (!b) > > + if (!b) { > > + error = -ENOMEM; > > goto err_out; > > + } > > > > b->sysdata = sysdata; > > b->ops = ops; > > b->number = b->busn_res.start = bus; > > - b2 = pci_find_bus(pci_domain_nr(b), bus); > > + b2 = pci_find_bus(bridge->domain_nr, bus); > > if (b2) { > > /* If we already got to this bus through a different bridge, ignore it */ > > dev_dbg(&b2->dev, "bus already known\n"); > > + error = -EEXIST; > > goto err_bus_out; > > } > > > > bridge->bus = b; > > - dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus); > > + dev_set_name(&bridge->dev, "pci%04x:%02x", bridge->domain_nr, bus); > > error = pcibios_root_bridge_prepare(bridge); > > if (error) > > goto err_out; > > @@ -1801,7 +1806,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, > > > > b->dev.class = &pcibus_class; > > b->dev.parent = b->bridge; > > - dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus); > > + dev_set_name(&b->dev, "%04x:%02x", bridge->domain_nr, bus); > > error = device_register(&b->dev); > > if (error) > > goto class_dev_reg_err; > > @@ -1851,7 +1856,27 @@ err_bus_out: > > kfree(b); > > err_out: > > kfree(bridge); > > - return NULL; > > + return ERR_PTR(error); > > +} > > + > > +struct pci_bus *pci_create_root_bus(struct device *parent, int bus, > > + struct pci_ops *ops, void *sysdata, struct list_head *resources) > > +{ > > + int domain_nr; > > + struct pci_bus *b = pci_alloc_bus(); > > + if (!b) > > + return NULL; > > + > > + b->sysdata = sysdata; > > + domain_nr = pci_domain_nr(b); > > + kfree(b); > > + > > + b = pci_create_root_bus_in_domain(parent, domain_nr, bus, > > + ops, sysdata, resources); > > + if (IS_ERR(b)) > > + return NULL; > > + > > + return b; > > } > > > > int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max) > > diff --git a/include/linux/pci.h b/include/linux/pci.h > > index 466bcd1..7e7b939 100644 > > --- a/include/linux/pci.h > > +++ b/include/linux/pci.h > > @@ -401,6 +401,7 @@ struct pci_host_bridge_window { > > struct pci_host_bridge { > > struct device dev; > > struct pci_bus *bus; /* root bus */ > > + int domain_nr; > > struct list_head windows; /* pci_host_bridge_windows */ > > void (*release_fn)(struct pci_host_bridge *); > > void *release_data; > > @@ -769,6 +770,9 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata); > > struct pci_bus *pci_create_root_bus(struct device *parent, int bus, > > struct pci_ops *ops, void *sysdata, > > struct list_head *resources); > > +struct pci_bus *pci_create_root_bus_in_domain(struct device *parent, > > + int domain, int bus, struct pci_ops *ops, > > + void *sysdata, struct list_head *resources); > > int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax); > > int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax); > > void pci_bus_release_busn_res(struct pci_bus *b); > > -- > > 2.0.0 > > >
On Tue, Jul 8, 2014 at 4:46 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: > On Tue, Jul 08, 2014 at 01:59:54AM +0100, Bjorn Helgaas wrote: >> I wonder if it would help to make a weak pci_domain_nr() function that >> returns "bridge->domain_nr". Then each arch could individually drop its >> pci_domain_nr() definition as it was converted, e.g., something like this: >> >> - Convert every arch pci_domain_nr() from a #define to a non-inline >> function >> - Add bridge.domain_nr, initialized from pci_domain_nr() >> - Add a weak generic pci_domain_nr() that returns bridge.domain_nr >> - Add a way to create a host bridge in a specified domain, so we can >> initialize bridge.domain_nr without using pci_domain_nr() >> - Convert each arch to use the new creation mechanism and drop its >> pci_domain_nr() implementation > > I will try to propose a patch implementing this. I think this is more of an extra credit, cleanup sort of thing. I don't think it advances your primary goal of (I think) getting arm64 PCI support in. So my advice is to not worry about unifying domain handling until later. Bjorn
On Tue, Jul 08, 2014 at 07:41:50PM +0100, Bjorn Helgaas wrote: > On Tue, Jul 8, 2014 at 4:46 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: > > On Tue, Jul 08, 2014 at 01:59:54AM +0100, Bjorn Helgaas wrote: > > >> I wonder if it would help to make a weak pci_domain_nr() function that > >> returns "bridge->domain_nr". Then each arch could individually drop its > >> pci_domain_nr() definition as it was converted, e.g., something like this: > >> > >> - Convert every arch pci_domain_nr() from a #define to a non-inline > >> function > >> - Add bridge.domain_nr, initialized from pci_domain_nr() > >> - Add a weak generic pci_domain_nr() that returns bridge.domain_nr > >> - Add a way to create a host bridge in a specified domain, so we can > >> initialize bridge.domain_nr without using pci_domain_nr() > >> - Convert each arch to use the new creation mechanism and drop its > >> pci_domain_nr() implementation > > > > I will try to propose a patch implementing this. > > I think this is more of an extra credit, cleanup sort of thing. I > don't think it advances your primary goal of (I think) getting arm64 > PCI support in. So my advice is to not worry about unifying domain > handling until later. Getting arm64 supported *is* my main goal. But like you have stated in your review of v7, you wanted to see another architecture converted as a guarantee of "genericity" (for lack of a better word) for my patches. The one architecture I've set my eyes on is microblaze, and that one uses pci_scan_root_bus() rather than pci_create_root_bus() so I don't have any opportunity to pass the domain number or any additional info (like the sysdata pointer that we were talking about) to the pci_host_bridge structure unless I do this cleanup. Best regards, Liviu > > Bjorn >
On Tue, Jul 8, 2014 at 4:48 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: > On Tue, Jul 08, 2014 at 07:41:50PM +0100, Bjorn Helgaas wrote: >> On Tue, Jul 8, 2014 at 4:46 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: >> > On Tue, Jul 08, 2014 at 01:59:54AM +0100, Bjorn Helgaas wrote: >> >> >> I wonder if it would help to make a weak pci_domain_nr() function that >> >> returns "bridge->domain_nr". Then each arch could individually drop its >> >> pci_domain_nr() definition as it was converted, e.g., something like this: >> >> >> >> - Convert every arch pci_domain_nr() from a #define to a non-inline >> >> function >> >> - Add bridge.domain_nr, initialized from pci_domain_nr() >> >> - Add a weak generic pci_domain_nr() that returns bridge.domain_nr >> >> - Add a way to create a host bridge in a specified domain, so we can >> >> initialize bridge.domain_nr without using pci_domain_nr() >> >> - Convert each arch to use the new creation mechanism and drop its >> >> pci_domain_nr() implementation >> > >> > I will try to propose a patch implementing this. >> >> I think this is more of an extra credit, cleanup sort of thing. I >> don't think it advances your primary goal of (I think) getting arm64 >> PCI support in. So my advice is to not worry about unifying domain >> handling until later. > > Getting arm64 supported *is* my main goal. But like you have stated in your > review of v7, you wanted to see another architecture converted as a guarantee > of "genericity" (for lack of a better word) for my patches. The one architecture > I've set my eyes on is microblaze, and that one uses pci_scan_root_bus() > rather than pci_create_root_bus() so I don't have any opportunity to pass the > domain number or any additional info (like the sysdata pointer that we were > talking about) to the pci_host_bridge structure unless I do this cleanup. I think maybe I was too harsh about that, or maybe we had different ideas about what "conversion" involved. My comment was in response to "pci: Introduce pci_register_io_range() helper function", and I don't remember why I was concerned about that; it's not even in drivers/pci, and it doesn't have an obvious connection to putting the domain number in struct pci_host_bridge. The thing I'm more concerned about is adding new PCI interfaces, e.g., pci_create_root_bus_in_domain(), that are only used by one architecture. Then it's hard to be sure that it's going to be useful for other arches. If you can add arm64 using the existing PCI interfaces, I don't any problem with that. Bjorn
On Wed, Jul 09, 2014 at 04:10:04PM +0100, Bjorn Helgaas wrote: > On Tue, Jul 8, 2014 at 4:48 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: > > On Tue, Jul 08, 2014 at 07:41:50PM +0100, Bjorn Helgaas wrote: > >> On Tue, Jul 8, 2014 at 4:46 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: > >> > On Tue, Jul 08, 2014 at 01:59:54AM +0100, Bjorn Helgaas wrote: > >> > >> >> I wonder if it would help to make a weak pci_domain_nr() function that > >> >> returns "bridge->domain_nr". Then each arch could individually drop its > >> >> pci_domain_nr() definition as it was converted, e.g., something like this: > >> >> > >> >> - Convert every arch pci_domain_nr() from a #define to a non-inline > >> >> function > >> >> - Add bridge.domain_nr, initialized from pci_domain_nr() > >> >> - Add a weak generic pci_domain_nr() that returns bridge.domain_nr > >> >> - Add a way to create a host bridge in a specified domain, so we can > >> >> initialize bridge.domain_nr without using pci_domain_nr() > >> >> - Convert each arch to use the new creation mechanism and drop its > >> >> pci_domain_nr() implementation > >> > > >> > I will try to propose a patch implementing this. > >> > >> I think this is more of an extra credit, cleanup sort of thing. I > >> don't think it advances your primary goal of (I think) getting arm64 > >> PCI support in. So my advice is to not worry about unifying domain > >> handling until later. > > > > Getting arm64 supported *is* my main goal. But like you have stated in your > > review of v7, you wanted to see another architecture converted as a guarantee > > of "genericity" (for lack of a better word) for my patches. The one architecture > > I've set my eyes on is microblaze, and that one uses pci_scan_root_bus() > > rather than pci_create_root_bus() so I don't have any opportunity to pass the > > domain number or any additional info (like the sysdata pointer that we were > > talking about) to the pci_host_bridge structure unless I do this cleanup. > > I think maybe I was too harsh about that, or maybe we had different > ideas about what "conversion" involved. My comment was in response to > "pci: Introduce pci_register_io_range() helper function", and I don't > remember why I was concerned about that; it's not even in drivers/pci, > and it doesn't have an obvious connection to putting the domain number > in struct pci_host_bridge. Well, to be honest I did move some of the code (as mentioned in the Changelog) from drivers/pci into drivers/of. It makes more sense to be in OF, as it mostly concerns architectures that use it. > > The thing I'm more concerned about is adding new PCI interfaces, e.g., > pci_create_root_bus_in_domain(), that are only used by one > architecture. Then it's hard to be sure that it's going to be useful > for other arches. If you can add arm64 using the existing PCI > interfaces, I don't any problem with that. (No blame here or reproaches, I'm just restating the situation:) I (mostly) did try that in my v7 series but it also got NAK-ed by Arnd and Catalin as it had too much arm64 specific code in there. I don't see a way out of adding new PCI interfaces if we want to have support in the PCI framework for unifying existing architectures. Of course, there is the painful alternative of changing the existing APIs and fixing arches in one go, but like you've said is going to be messy. I don't think I (or the people and companies wanting PCIe on arm64) should cop out and pick a quick fix that adds sysdata structure into arm64 just to avoid new APIs, as this is not going to help anyone in long term. What I can do is to create a set of parallel APIs for pci_{scan,create}_root_bus() that take a pci_host_bridge pointer and start converting architectures one by one to that API while deprecating the existing one. That way we can add arm64 easily as it would be the first architecture to use new code without breaking things *and* we provide a migration path. Best regards, Liviu > > Bjorn >
On Thu, Jul 10, 2014 at 3:47 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: > I don't see a way out of adding new PCI interfaces if we want to have support in > the PCI framework for unifying existing architectures. Of course, there is the painful > alternative of changing the existing APIs and fixing arches in one go, but like you've > said is going to be messy. I don't think I (or the people and companies wanting PCIe > on arm64) should cop out and pick a quick fix that adds sysdata structure into arm64 > just to avoid new APIs, as this is not going to help anyone in long term. What I can > do is to create a set of parallel APIs for pci_{scan,create}_root_bus() that take > a pci_host_bridge pointer and start converting architectures one by one to that API > while deprecating the existing one. That way we can add arm64 easily as it would be > the first architecture to use new code without breaking things *and* we provide a > migration path. A lot of the v7 discussion was about pci_register_io_range(). I apologize, because I think I really derailed things there and it was unwarranted. Arnd was right that migrating other arches should be a separate effort. I *think* I was probably thinking about the proposal of adding pci_create_root_bus_in_domain(), and my reservations about that got transferred to the pci_register_io_range() discussion. In any case, I'm completely fine with pci_register_io_range() now. Most of the rest of the v7 discussion was about "Introduce a domain number for pci_host_bridge." I think we should add arm64 using the existing pci_scan_root_bus() and keep the domain number in the arm64 sysdata structure like every other arch does. Isn't that feasible? We can worry about domain unification later. I haven't followed closely enough to know what other objections people had. Bjorn
On Thu, Jul 10, 2014 at 11:36:10PM +0100, Bjorn Helgaas wrote: > On Thu, Jul 10, 2014 at 3:47 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote: > > > I don't see a way out of adding new PCI interfaces if we want to have support in > > the PCI framework for unifying existing architectures. Of course, there is the painful > > alternative of changing the existing APIs and fixing arches in one go, but like you've > > said is going to be messy. I don't think I (or the people and companies wanting PCIe > > on arm64) should cop out and pick a quick fix that adds sysdata structure into arm64 > > just to avoid new APIs, as this is not going to help anyone in long term. What I can > > do is to create a set of parallel APIs for pci_{scan,create}_root_bus() that take > > a pci_host_bridge pointer and start converting architectures one by one to that API > > while deprecating the existing one. That way we can add arm64 easily as it would be > > the first architecture to use new code without breaking things *and* we provide a > > migration path. > > A lot of the v7 discussion was about pci_register_io_range(). I > apologize, because I think I really derailed things there and it was > unwarranted. Arnd was right that migrating other arches should be a > separate effort. I *think* I was probably thinking about the proposal > of adding pci_create_root_bus_in_domain(), and my reservations about > that got transferred to the pci_register_io_range() discussion. In > any case, I'm completely fine with pci_register_io_range() now. > > Most of the rest of the v7 discussion was about "Introduce a domain > number for pci_host_bridge." I think we should add arm64 using the > existing pci_scan_root_bus() and keep the domain number in the arm64 > sysdata structure like every other arch does. Isn't that feasible? > We can worry about domain unification later. Thanks! I'm really not that keen to add sysdata support in the arch code as it requires initialisation code that I have tried to eliminate. What I'm going to suggest for my v9 is a parallel set of APIs that arm64 will be the first to use without changing the existing pci_{scan,create}_bus() functions and then the conversion process will migrate arches to the new API. Best regards, Liviu > > I haven't followed closely enough to know what other objections people had. > > Bjorn >
On Thu, Jul 10, 2014 at 11:36:10PM +0100, Bjorn Helgaas wrote: > Most of the rest of the v7 discussion was about "Introduce a domain > number for pci_host_bridge." I think we should add arm64 using the > existing pci_scan_root_bus() and keep the domain number in the arm64 > sysdata structure like every other arch does. Isn't that feasible? > We can worry about domain unification later. I think that's what we were trying to avoid, adding an arm64-specific pci_sys_data structure (and arm64-specific API). IIUC, avoiding this would allow the host controller drivers to use the sysdata pointer for their own private data structures. Also since you can specify the domain number via DT (and in Liviu's v8 patches read by of_create_pci_host_bridge), I think it would make sense to have it stored in some generic data structures (e.g. pci_host_bridge) rather than in an arm64 private sysdata. (Liviu is thinking of an alternative API but maybe he could briefly describe it here before posting a new series)
On Fri, Jul 11, 2014 at 03:11:16PM +0100, Catalin Marinas wrote: > On Thu, Jul 10, 2014 at 11:36:10PM +0100, Bjorn Helgaas wrote: > > Most of the rest of the v7 discussion was about "Introduce a domain > > number for pci_host_bridge." I think we should add arm64 using the > > existing pci_scan_root_bus() and keep the domain number in the arm64 > > sysdata structure like every other arch does. Isn't that feasible? > > We can worry about domain unification later. > > I think that's what we were trying to avoid, adding an arm64-specific > pci_sys_data structure (and arm64-specific API). IIUC, avoiding this > would allow the host controller drivers to use the sysdata pointer for > their own private data structures. > > Also since you can specify the domain number via DT (and in Liviu's > v8 patches read by of_create_pci_host_bridge), I think it would make > sense to have it stored in some generic data structures (e.g. > pci_host_bridge) rather than in an arm64 private sysdata. > > (Liviu is thinking of an alternative API but maybe he could briefly > describe it here before posting a new series) > > -- > Catalin My plan is to keep the domain number in the pci_host_bridge and split the creation of the pci_host_bridge out of the pci_create_root_bus(). The new function (tentatively called pci_create_new_root_bus()) will no longer call pci_alloc_host_bridge() but will accept it as a parameter, allowing one to be able to set the domain_nr ahead of the root bus creation. Best regards, Liviu
On Fri, Jul 11, 2014 at 04:08:23PM +0100, Liviu Dudau wrote: > On Fri, Jul 11, 2014 at 03:11:16PM +0100, Catalin Marinas wrote: > > On Thu, Jul 10, 2014 at 11:36:10PM +0100, Bjorn Helgaas wrote: > > > Most of the rest of the v7 discussion was about "Introduce a domain > > > number for pci_host_bridge." I think we should add arm64 using the > > > existing pci_scan_root_bus() and keep the domain number in the arm64 > > > sysdata structure like every other arch does. Isn't that feasible? > > > We can worry about domain unification later. > > > > I think that's what we were trying to avoid, adding an arm64-specific > > pci_sys_data structure (and arm64-specific API). IIUC, avoiding this > > would allow the host controller drivers to use the sysdata pointer for > > their own private data structures. > > > > Also since you can specify the domain number via DT (and in Liviu's > > v8 patches read by of_create_pci_host_bridge), I think it would make > > sense to have it stored in some generic data structures (e.g. > > pci_host_bridge) rather than in an arm64 private sysdata. > > > > (Liviu is thinking of an alternative API but maybe he could briefly > > describe it here before posting a new series) > > My plan is to keep the domain number in the pci_host_bridge and split > the creation of the pci_host_bridge out of the pci_create_root_bus(). Wouldn't it make more sense to add domain_nr to the pci_bus structure (well, only needed for the root bus)? It would simplify pci_domain_nr() as well which only takes a pci_bus parameter. > The new function (tentatively called pci_create_new_root_bus()) will > no longer call pci_alloc_host_bridge() but will accept it as a > parameter, allowing one to be able to set the domain_nr ahead of the > root bus creation. If we place domain_nr in pci_bus, this split wouldn't help but we still need your original pci_create_root_bus_in_domain(). Are there other uses of your proposal above? Yet another alternative is to ignore PCI domains altogether (domain 0 always).
On Fri, Jul 11, 2014 at 8:11 AM, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Thu, Jul 10, 2014 at 11:36:10PM +0100, Bjorn Helgaas wrote: >> Most of the rest of the v7 discussion was about "Introduce a domain >> number for pci_host_bridge." I think we should add arm64 using the >> existing pci_scan_root_bus() and keep the domain number in the arm64 >> sysdata structure like every other arch does. Isn't that feasible? >> We can worry about domain unification later. > > I think that's what we were trying to avoid, adding an arm64-specific > pci_sys_data structure (and arm64-specific API). IIUC, avoiding this > would allow the host controller drivers to use the sysdata pointer for > their own private data structures. > > Also since you can specify the domain number via DT (and in Liviu's > v8 patches read by of_create_pci_host_bridge), I think it would make > sense to have it stored in some generic data structures (e.g. > pci_host_bridge) rather than in an arm64 private sysdata. It would definitely be nice to keep the domain in a generic data structure rather than an arm64-specific one. But every other arch keeps it in an arch-specific structure today, and I think following that existing pattern is the quickest way forward. But you mentioned arm64-specific API, too. What do you have in mind there? I know there will be arm64 implementations of various pcibios_*() functions (just like for every other arch), but it sounds like there might be something else? Bjorn
On Fri, Jul 11, 2014 at 06:02:56PM +0100, Bjorn Helgaas wrote: > On Fri, Jul 11, 2014 at 8:11 AM, Catalin Marinas > <catalin.marinas@arm.com> wrote: > > On Thu, Jul 10, 2014 at 11:36:10PM +0100, Bjorn Helgaas wrote: > >> Most of the rest of the v7 discussion was about "Introduce a domain > >> number for pci_host_bridge." I think we should add arm64 using the > >> existing pci_scan_root_bus() and keep the domain number in the arm64 > >> sysdata structure like every other arch does. Isn't that feasible? > >> We can worry about domain unification later. > > > > I think that's what we were trying to avoid, adding an arm64-specific > > pci_sys_data structure (and arm64-specific API). IIUC, avoiding this > > would allow the host controller drivers to use the sysdata pointer for > > their own private data structures. > > > > Also since you can specify the domain number via DT (and in Liviu's > > v8 patches read by of_create_pci_host_bridge), I think it would make > > sense to have it stored in some generic data structures (e.g. > > pci_host_bridge) rather than in an arm64 private sysdata. > > It would definitely be nice to keep the domain in a generic data > structure rather than an arm64-specific one. But every other arch > keeps it in an arch-specific structure today, and I think following > that existing pattern is the quickest way forward. In this case we end up with an arm64-specific struct pci_sys_data and I assume some API that takes care of this data structure to populate the domain nr. In Liviu's implementation, of_create_pci_host_bridge() is called by the host controller driver directly and reads the domain_nr from the DT. It also gets a void *host_data which it stores as sysdata in the pci_bus structure (but that's specific to the host controller driver rather than arm64). Since sysdata is opaque to of_create_pci_host_bridge(), it cannot set the domain_nr. If we go for arm64 sysdata, the host controller driver would have to call some arm64 pci* function (maybe with its own private data as argument) which would have to parse the DT, assign the domain nr and eventually call pci_create_root_bus(). But it means that significant part of of_create_pci_host_bridge() would be moved under arch/arm64 (an alternative is for each host driver to implement its own of_create_pci_host_bridge()). In addition, host drivers would retrieve their private data from the arm64 specific sysdata structure and we were trying to make host drivers depend only on generic data structures and API rather than arch specific. > But you mentioned arm64-specific API, too. What do you have in mind > there? I know there will be arm64 implementations of various > pcibios_*() functions (just like for every other arch), but it sounds > like there might be something else? Not much left which is arm64 specific, just some callbacks: http://linux-arm.org/git?p=linux-ld.git;a=commitdiff;h=82ebbce34506676528b9a7ae8f8fbc84b6b6248e AFAICT, there isn't anything that host drivers need to call directly. Basically we want to decouple the PCI host driver model from the arch specific data structures/API.
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index 2c92662..abf5e82 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -1748,8 +1748,9 @@ void __weak pcibios_remove_bus(struct pci_bus *bus) { } -struct pci_bus *pci_create_root_bus(struct device *parent, int bus, - struct pci_ops *ops, void *sysdata, struct list_head *resources) +struct pci_bus *pci_create_root_bus_in_domain(struct device *parent, + int domain, int bus, struct pci_ops *ops, void *sysdata, + struct list_head *resources) { int error; struct pci_host_bridge *bridge; @@ -1762,27 +1763,31 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, bridge = pci_alloc_host_bridge(); if (!bridge) - return NULL; + return ERR_PTR(-ENOMEM); bridge->dev.parent = parent; bridge->dev.release = pci_release_host_bridge_dev; + bridge->domain_nr = domain; b = pci_alloc_bus(); - if (!b) + if (!b) { + error = -ENOMEM; goto err_out; + } b->sysdata = sysdata; b->ops = ops; b->number = b->busn_res.start = bus; - b2 = pci_find_bus(pci_domain_nr(b), bus); + b2 = pci_find_bus(bridge->domain_nr, bus); if (b2) { /* If we already got to this bus through a different bridge, ignore it */ dev_dbg(&b2->dev, "bus already known\n"); + error = -EEXIST; goto err_bus_out; } bridge->bus = b; - dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus); + dev_set_name(&bridge->dev, "pci%04x:%02x", bridge->domain_nr, bus); error = pcibios_root_bridge_prepare(bridge); if (error) goto err_out; @@ -1801,7 +1806,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus, b->dev.class = &pcibus_class; b->dev.parent = b->bridge; - dev_set_name(&b->dev, "%04x:%02x", pci_domain_nr(b), bus); + dev_set_name(&b->dev, "%04x:%02x", bridge->domain_nr, bus); error = device_register(&b->dev); if (error) goto class_dev_reg_err; @@ -1851,7 +1856,27 @@ err_bus_out: kfree(b); err_out: kfree(bridge); - return NULL; + return ERR_PTR(error); +} + +struct pci_bus *pci_create_root_bus(struct device *parent, int bus, + struct pci_ops *ops, void *sysdata, struct list_head *resources) +{ + int domain_nr; + struct pci_bus *b = pci_alloc_bus(); + if (!b) + return NULL; + + b->sysdata = sysdata; + domain_nr = pci_domain_nr(b); + kfree(b); + + b = pci_create_root_bus_in_domain(parent, domain_nr, bus, + ops, sysdata, resources); + if (IS_ERR(b)) + return NULL; + + return b; } int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max) diff --git a/include/linux/pci.h b/include/linux/pci.h index 466bcd1..7e7b939 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -401,6 +401,7 @@ struct pci_host_bridge_window { struct pci_host_bridge { struct device dev; struct pci_bus *bus; /* root bus */ + int domain_nr; struct list_head windows; /* pci_host_bridge_windows */ void (*release_fn)(struct pci_host_bridge *); void *release_data; @@ -769,6 +770,9 @@ struct pci_bus *pci_scan_bus(int bus, struct pci_ops *ops, void *sysdata); struct pci_bus *pci_create_root_bus(struct device *parent, int bus, struct pci_ops *ops, void *sysdata, struct list_head *resources); +struct pci_bus *pci_create_root_bus_in_domain(struct device *parent, + int domain, int bus, struct pci_ops *ops, + void *sysdata, struct list_head *resources); int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax); int pci_bus_update_busn_res_end(struct pci_bus *b, int busmax); void pci_bus_release_busn_res(struct pci_bus *b);