| Message ID | 1416219710-26088-8-git-send-email-wangyijing@huawei.com |
|---|---|
| State | New |
| Headers | show |
On Monday 17 November 2014 18:21:41 Yijing Wang wrote: > There are some common PCI infos like domain, msi_controller, these > infos are saved in arch PCI sysdata, and lots arch specific functions > like pci_domain_nr() and pcibios_msi_controller() required. > We could separate pci_host_bridge creation out of pci_create_root_bus(), > then we could put the common infos in, then we could eliminate > the arch specifc functions. > > Signed-off-by: Yijing Wang <wangyijing@huawei.com> > --- > drivers/pci/host-bridge.c | 99 +++++++++++++++++++++++++++++++++ > drivers/pci/probe.c | 134 ++++++++++++++++---------------------------- > include/linux/pci.h | 11 +++- > 3 files changed, 158 insertions(+), 86 deletions(-) > > diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c > index 0e5f3c9..e31604f 100644 > --- a/drivers/pci/host-bridge.c > +++ b/drivers/pci/host-bridge.c > @@ -8,6 +8,105 @@ > > #include "pci.h" > > +LIST_HEAD(pci_host_bridge_list); > +DECLARE_RWSEM(pci_host_bridge_sem); Unless the pci_host_bridge_sem is accessed thousands of times per second, it's normally better to use a simple mutex instead. > +static struct resource busn_resource = { > + .name = "PCI busn", > + .start = 0, > + .end = 255, > + .flags = IORESOURCE_BUS, > +}; I think it would be better to require callers to pass the bus resource down to the function. > +struct pci_host_bridge *pci_create_host_bridge( > + struct device *parent, u32 db, > + struct pci_ops *ops, void *sysdata, > + struct list_head *resources) > +{ Do we still need to pass the 'sysdata' in here? If we are guaranteed to have a device pointer, we should always be able to get the driver private data from dev_get_drvdata(host->dev->parent). > + host = kzalloc(sizeof(*host), GFP_KERNEL); > + if (!host) > + return NULL; devm_kzalloc maybe? > + if (!resources) { > + /* Use default IO/MEM/BUS resources*/ > + pci_add_resource(&host->windows, &ioport_resource); > + pci_add_resource(&host->windows, &iomem_resource); > + pci_add_resource(&host->windows, &busn_resource); > + } else { > + list_for_each_entry_safe(window, n, resources, list) > + list_move_tail(&window->list, &host->windows); > + } I think we should assume that the correct resources are passed. You could add a wrapper around this function to convert old platforms though. > +EXPORT_SYMBOL(pci_create_host_bridge); EXPORT_SYMBOL_GPL() maybe? > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 8b11b38..daa7f40 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -402,7 +402,12 @@ struct pci_host_bridge_window { > struct pci_host_bridge { > struct device dev; > struct pci_bus *bus; /* root bus */ > + struct list_head list; > struct list_head windows; /* pci_host_bridge_windows */ > + int busnum; The busnum should already be implied through the bus resource. 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/
>> +LIST_HEAD(pci_host_bridge_list); >> +DECLARE_RWSEM(pci_host_bridge_sem); > > Unless the pci_host_bridge_sem is accessed thousands of times per second, > it's normally better to use a simple mutex instead. OK, I will use simple mutex instead. > >> +static struct resource busn_resource = { >> + .name = "PCI busn", >> + .start = 0, >> + .end = 255, >> + .flags = IORESOURCE_BUS, >> +}; > > I think it would be better to require callers to pass the bus resource > down to the function. Hmm, I think most of caller will provide the bus resource, but some others will not give any bus resource, extremely, no any resources :(. But we still need properly configure their resources for compatibility. > >> +struct pci_host_bridge *pci_create_host_bridge( >> + struct device *parent, u32 db, >> + struct pci_ops *ops, void *sysdata, >> + struct list_head *resources) >> +{ > > Do we still need to pass the 'sysdata' in here? If we are guaranteed to > have a device pointer, we should always be able to get the driver > private data from dev_get_drvdata(host->dev->parent). We need, some platforms pass NULL pointer as host bridge parent. > >> + host = kzalloc(sizeof(*host), GFP_KERNEL); >> + if (!host) >> + return NULL; > > devm_kzalloc maybe? I don't know much detail about devm_kzalloc(), but we have no pci host driver here, and I found no devm_kzalloc() uses in core PCI code before. > >> + if (!resources) { >> + /* Use default IO/MEM/BUS resources*/ >> + pci_add_resource(&host->windows, &ioport_resource); >> + pci_add_resource(&host->windows, &iomem_resource); >> + pci_add_resource(&host->windows, &busn_resource); >> + } else { >> + list_for_each_entry_safe(window, n, resources, list) >> + list_move_tail(&window->list, &host->windows); >> + } > > I think we should assume that the correct resources are passed. You > could add a wrapper around this function to convert old platforms > though. OK, I will move these code out of pci_create_host_bridge, and add a wrapper to setup the default resources. > >> +EXPORT_SYMBOL(pci_create_host_bridge); > > EXPORT_SYMBOL_GPL() maybe? OK, will update it. > >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index 8b11b38..daa7f40 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -402,7 +402,12 @@ struct pci_host_bridge_window { >> struct pci_host_bridge { >> struct device dev; >> struct pci_bus *bus; /* root bus */ >> + struct list_head list; >> struct list_head windows; /* pci_host_bridge_windows */ >> + int busnum; > > The busnum should already be implied through the bus resource. Yes, I will consider remove it and introduce a helper function to get the root bus number, thanks! Thanks! Yijing. > > Arnd > > . >
On Tuesday 18 November 2014 16:32:26 Yijing Wang wrote: > >> +static struct resource busn_resource = { > >> + .name = "PCI busn", > >> + .start = 0, > >> + .end = 255, > >> + .flags = IORESOURCE_BUS, > >> +}; > > > > I think it would be better to require callers to pass the bus resource > > down to the function. > > Hmm, I think most of caller will provide the bus resource, but some others > will not give any bus resource, extremely, no any resources :(. But we still > need properly configure their resources for compatibility. I think that is what the conversion to pci_scan_bus_parented() is about: The idea is that we add the correct bus resource to callers of pci_scan_bus_parented or pci_scan_bus and then change them to call pci_scan_root_bus instead. > >> +struct pci_host_bridge *pci_create_host_bridge( > >> + struct device *parent, u32 db, > >> + struct pci_ops *ops, void *sysdata, > >> + struct list_head *resources) > >> +{ > > > > Do we still need to pass the 'sysdata' in here? If we are guaranteed to > > have a device pointer, we should always be able to get the driver > > private data from dev_get_drvdata(host->dev->parent). > > We need, some platforms pass NULL pointer as host bridge parent. But those don't have to use the new pci_create_host_bridge() function, right? > >> + host = kzalloc(sizeof(*host), GFP_KERNEL); > >> + if (!host) > >> + return NULL; > > > > devm_kzalloc maybe? > > I don't know much detail about devm_kzalloc(), but we have no pci host driver > here, and I found no devm_kzalloc() uses in core PCI code before. It also depends on having a valid device pointer. The idea is that the memory is automatically freed if the probe() function returns with an error, or the device driver gets unloaded. For the classic PCI hosts that are not connected to a device, that wouldn't work of course. 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 2014/11/18 17:30, Arnd Bergmann wrote: > On Tuesday 18 November 2014 16:32:26 Yijing Wang wrote: > >>>> +static struct resource busn_resource = { >>>> + .name = "PCI busn", >>>> + .start = 0, >>>> + .end = 255, >>>> + .flags = IORESOURCE_BUS, >>>> +}; >>> >>> I think it would be better to require callers to pass the bus resource >>> down to the function. >> >> Hmm, I think most of caller will provide the bus resource, but some others >> will not give any bus resource, extremely, no any resources :(. But we still >> need properly configure their resources for compatibility. > > I think that is what the conversion to pci_scan_bus_parented() is about: > The idea is that we add the correct bus resource to callers of > pci_scan_bus_parented or pci_scan_bus and then change them to call > pci_scan_root_bus instead. It looks good to me, but for simplification, or I will try to use a wrapper to process the drivers don't pass the busnr resources, and make sure the generic pci_create_host_bridge() always get the valid resources. > >>>> +struct pci_host_bridge *pci_create_host_bridge( >>>> + struct device *parent, u32 db, >>>> + struct pci_ops *ops, void *sysdata, >>>> + struct list_head *resources) >>>> +{ >>> >>> Do we still need to pass the 'sysdata' in here? If we are guaranteed to >>> have a device pointer, we should always be able to get the driver >>> private data from dev_get_drvdata(host->dev->parent). >> >> We need, some platforms pass NULL pointer as host bridge parent. > > But those don't have to use the new pci_create_host_bridge() function, > right? As I mentioned in another reply, I hope all pci host drivers could use pci_create_host_bridge(), keep different PCI scan interfaces in PCI core make things become complex. > >>>> + host = kzalloc(sizeof(*host), GFP_KERNEL); >>>> + if (!host) >>>> + return NULL; >>> >>> devm_kzalloc maybe? >> >> I don't know much detail about devm_kzalloc(), but we have no pci host driver >> here, and I found no devm_kzalloc() uses in core PCI code before. > > It also depends on having a valid device pointer. The idea is that the memory > is automatically freed if the probe() function returns with an error, or > the device driver gets unloaded. For the classic PCI hosts that are not > connected to a device, that wouldn't work of course. > > Arnd > > . >
On Tuesday 18 November 2014 19:44:36 Yijing Wang wrote: > On 2014/11/18 17:30, Arnd Bergmann wrote: > > On Tuesday 18 November 2014 16:32:26 Yijing Wang wrote: > > > >>>> +static struct resource busn_resource = { > >>>> + .name = "PCI busn", > >>>> + .start = 0, > >>>> + .end = 255, > >>>> + .flags = IORESOURCE_BUS, > >>>> +}; > >>> > >>> I think it would be better to require callers to pass the bus resource > >>> down to the function. > >> > >> Hmm, I think most of caller will provide the bus resource, but some others > >> will not give any bus resource, extremely, no any resources :(. But we still > >> need properly configure their resources for compatibility. > > > > I think that is what the conversion to pci_scan_bus_parented() is about: > > The idea is that we add the correct bus resource to callers of > > pci_scan_bus_parented or pci_scan_bus and then change them to call > > pci_scan_root_bus instead. > > It looks good to me, but for simplification, or I will try to use a wrapper to > process the drivers don't pass the busnr resources, and make sure the generic > pci_create_host_bridge() always get the valid resources. Ok. > >>>> +struct pci_host_bridge *pci_create_host_bridge( > >>>> + struct device *parent, u32 db, > >>>> + struct pci_ops *ops, void *sysdata, > >>>> + struct list_head *resources) > >>>> +{ > >>> > >>> Do we still need to pass the 'sysdata' in here? If we are guaranteed to > >>> have a device pointer, we should always be able to get the driver > >>> private data from dev_get_drvdata(host->dev->parent). > >> > >> We need, some platforms pass NULL pointer as host bridge parent. > > > > But those don't have to use the new pci_create_host_bridge() function, > > right? > > As I mentioned in another reply, I hope all pci host drivers could use > pci_create_host_bridge(), keep different PCI scan interfaces in PCI core > make things become complex. Doing this for all platforms that have PCI support would be a lot of work though, I think it's better to focus on having a the best interface for the majority of users. 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/
>>>> We need, some platforms pass NULL pointer as host bridge parent. >>> >>> But those don't have to use the new pci_create_host_bridge() function, >>> right? >> >> As I mentioned in another reply, I hope all pci host drivers could use >> pci_create_host_bridge(), keep different PCI scan interfaces in PCI core >> make things become complex. > > Doing this for all platforms that have PCI support would be a lot of > work though, I think it's better to focus on having a the best interface > for the majority of users. Yes. > > Arnd > > . >
On Tue, Nov 18, 2014 at 08:32:26AM +0000, Yijing Wang wrote: > > >> +LIST_HEAD(pci_host_bridge_list); > >> +DECLARE_RWSEM(pci_host_bridge_sem); > > > > Unless the pci_host_bridge_sem is accessed thousands of times per second, > > it's normally better to use a simple mutex instead. > > OK, I will use simple mutex instead. > > > > >> +static struct resource busn_resource = { > >> + .name = "PCI busn", > >> + .start = 0, > >> + .end = 255, > >> + .flags = IORESOURCE_BUS, > >> +}; > > > > I think it would be better to require callers to pass the bus resource > > down to the function. > > Hmm, I think most of caller will provide the bus resource, but some others > will not give any bus resource, extremely, no any resources :(. But we still > need properly configure their resources for compatibility. > > > > >> +struct pci_host_bridge *pci_create_host_bridge( > >> + struct device *parent, u32 db, > >> + struct pci_ops *ops, void *sysdata, > >> + struct list_head *resources) > >> +{ > > > > Do we still need to pass the 'sysdata' in here? If we are guaranteed to > > have a device pointer, we should always be able to get the driver > > private data from dev_get_drvdata(host->dev->parent). > > We need, some platforms pass NULL pointer as host bridge parent. Yijing, May I suggest a different approach here? Rather than having to pass an opaque pointer that gets converted by the host bridge driver back to the private structure, what about promoting a new style of usage, that is similar to the way device drivers work? Lets try to promote the embedding of the generic pci_host_bridge structure in the host bridge specific structure! Then we can access the private data doing container_of(). Something like this: struct pci_controller { struct pci_host_bridge bridge; /* private host bridge data here */ ..... }; #define PCI_CONTROLLER(bus) ({ struct pci_host_bridge *hb = to_pci_host_bridge(bus->bridge); \ container_of(hb, struct pci_controller, bridge); }) Then we can retrieve the host bridge structure from everywhere we have a device. Best regards, Liviu > > > > >> + host = kzalloc(sizeof(*host), GFP_KERNEL); > >> + if (!host) > >> + return NULL; > > > > devm_kzalloc maybe? > > I don't know much detail about devm_kzalloc(), but we have no pci host driver > here, and I found no devm_kzalloc() uses in core PCI code before. > > > > >> + if (!resources) { > >> + /* Use default IO/MEM/BUS resources*/ > >> + pci_add_resource(&host->windows, &ioport_resource); > >> + pci_add_resource(&host->windows, &iomem_resource); > >> + pci_add_resource(&host->windows, &busn_resource); > >> + } else { > >> + list_for_each_entry_safe(window, n, resources, list) > >> + list_move_tail(&window->list, &host->windows); > >> + } > > > > I think we should assume that the correct resources are passed. You > > could add a wrapper around this function to convert old platforms > > though. > > OK, I will move these code out of pci_create_host_bridge, and add a wrapper > to setup the default resources. > > > > >> +EXPORT_SYMBOL(pci_create_host_bridge); > > > > EXPORT_SYMBOL_GPL() maybe? > > OK, will update it. > > > > >> diff --git a/include/linux/pci.h b/include/linux/pci.h > >> index 8b11b38..daa7f40 100644 > >> --- a/include/linux/pci.h > >> +++ b/include/linux/pci.h > >> @@ -402,7 +402,12 @@ struct pci_host_bridge_window { > >> struct pci_host_bridge { > >> struct device dev; > >> struct pci_bus *bus; /* root bus */ > >> + struct list_head list; > >> struct list_head windows; /* pci_host_bridge_windows */ > >> + int busnum; > > > > The busnum should already be implied through the bus resource. > > Yes, I will consider remove it and introduce a helper function to get the root bus number, thanks! > > Thanks! > Yijing. > > > > > Arnd > > > > . > > > > > -- > Thanks! > Yijing > > -- > 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 Mon, Nov 17, 2014 at 10:21:41AM +0000, Yijing Wang wrote: > There are some common PCI infos like domain, msi_controller, these > infos are saved in arch PCI sysdata, and lots arch specific functions > like pci_domain_nr() and pcibios_msi_controller() required. > We could separate pci_host_bridge creation out of pci_create_root_bus(), > then we could put the common infos in, then we could eliminate > the arch specifc functions. > Please Cc: Yinghai Lu and Jiang Liu on future versions. More comments on the conversion of pci_create_root_bus(): > Signed-off-by: Yijing Wang <wangyijing@huawei.com> > --- > drivers/pci/host-bridge.c | 99 +++++++++++++++++++++++++++++++++ > drivers/pci/probe.c | 134 ++++++++++++++++---------------------------- > include/linux/pci.h | 11 +++- > 3 files changed, 158 insertions(+), 86 deletions(-) > > diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c > index 0e5f3c9..e31604f 100644 > --- a/drivers/pci/host-bridge.c > +++ b/drivers/pci/host-bridge.c > @@ -8,6 +8,105 @@ > > #include "pci.h" > > +LIST_HEAD(pci_host_bridge_list); > +DECLARE_RWSEM(pci_host_bridge_sem); > + > +static struct resource busn_resource = { > + .name = "PCI busn", > + .start = 0, > + .end = 255, > + .flags = IORESOURCE_BUS, > +}; > + > +static void pci_release_host_bridge_dev(struct device *dev) > +{ > + struct pci_host_bridge *bridge = to_pci_host_bridge(dev); > + > + if (bridge->release_fn) > + bridge->release_fn(bridge); > + pci_free_resource_list(&bridge->windows); > + kfree(bridge); > +} > + > +struct pci_host_bridge *pci_create_host_bridge( > + struct device *parent, u32 db, > + struct pci_ops *ops, void *sysdata, I don't thinks it is worth moving the buses' pci_ops into pci_host_bridge. It might be more useful to have pci_host_bridge specific ops here. > + struct list_head *resources) > +{ > + int error; > + struct pci_bus *b; > + struct pci_host_bridge *host, *h; > + struct pci_host_bridge_window *window, *n; > + > + down_read(&pci_host_bridge_sem); > + list_for_each_entry(h, &pci_host_bridge_list, list) { > + if (h->domain == PCI_DOMAIN(db) && > + h->busnum == PCI_BUSNUM(db)) { > + dev_dbg(&h->dev, "pci host bridge exist\n"); > + up_read(&pci_host_bridge_sem); > + return NULL; > + } > + } > + up_read(&pci_host_bridge_sem); > + > + host = kzalloc(sizeof(*host), GFP_KERNEL); > + if (!host) > + return NULL; > + > + host->sysdata = sysdata; > + host->busnum = PCI_BUSNUM(db); > + host->domain = PCI_DOMAIN(db); > + host->ops = ops; > + host->dev.parent = parent; > + INIT_LIST_HEAD(&host->windows); > + host->dev.release = pci_release_host_bridge_dev; > + > + /* this is hack, just for build, will be removed later*/ > + b = kzalloc(sizeof(*b), GFP_KERNEL); > + b->sysdata = sysdata; > + pci_bus_assign_domain_nr(b, parent); > + host->domain = pci_domain_nr(b); > + > + if (!resources) { > + /* Use default IO/MEM/BUS resources*/ > + pci_add_resource(&host->windows, &ioport_resource); > + pci_add_resource(&host->windows, &iomem_resource); > + pci_add_resource(&host->windows, &busn_resource); > + } else { > + list_for_each_entry_safe(window, n, resources, list) > + list_move_tail(&window->list, &host->windows); > + } > + > + dev_set_name(&host->dev, "pci%04x:%02x", host->domain, > + host->busnum); > + error = pcibios_root_bridge_prepare(host); > + if(error) { > + kfree(host); > + return NULL; > + } > + > + error = device_register(&host->dev); > + if (error) { > + put_device(&host->dev); > + return NULL; > + } > + > + down_write(&pci_host_bridge_sem); > + list_add_tail(&host->list, &pci_host_bridge_list); > + up_write(&pci_host_bridge_sem); > + return host; > +} > +EXPORT_SYMBOL(pci_create_host_bridge); > + > +void pci_free_host_bridge(struct pci_host_bridge *host) > +{ > + down_write(&pci_host_bridge_sem); > + list_del(&host->list); > + up_write(&pci_host_bridge_sem); > + > + device_unregister(&host->dev); > +} > + > static struct pci_bus *find_pci_root_bus(struct pci_bus *bus) > { > while (bus->parent) > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index adf4d09..d472da4 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -17,13 +17,6 @@ > #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */ > #define CARDBUS_RESERVE_BUSNR 3 > > -static struct resource busn_resource = { > - .name = "PCI busn", > - .start = 0, > - .end = 255, > - .flags = IORESOURCE_BUS, > -}; > - > /* Ugh. Need to stop exporting this to modules. */ > LIST_HEAD(pci_root_buses); > EXPORT_SYMBOL(pci_root_buses); > @@ -508,31 +501,6 @@ static struct pci_bus *pci_alloc_bus(struct pci_bus *parent) > return b; > } > > -static void pci_release_host_bridge_dev(struct device *dev) > -{ > - struct pci_host_bridge *bridge = to_pci_host_bridge(dev); > - > - if (bridge->release_fn) > - bridge->release_fn(bridge); > - > - pci_free_resource_list(&bridge->windows); > - > - kfree(bridge); > -} > - > -static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b) > -{ > - struct pci_host_bridge *bridge; > - > - bridge = kzalloc(sizeof(*bridge), GFP_KERNEL); > - if (!bridge) > - return NULL; > - > - INIT_LIST_HEAD(&bridge->windows); > - bridge->bus = b; > - return bridge; > -} > - > static const unsigned char pcix_bus_speed[] = { > PCI_SPEED_UNKNOWN, /* 0 */ > PCI_SPEED_66MHz_PCIX, /* 1 */ > @@ -1895,52 +1863,33 @@ void __weak pcibios_remove_bus(struct pci_bus *bus) > { > } > > -struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, > - struct pci_ops *ops, void *sysdata, struct list_head *resources) > +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *bridge) > { > int error; > - struct pci_host_bridge *bridge; > struct pci_bus *b, *b2; > - struct pci_host_bridge_window *window, *n; > + struct pci_host_bridge_window *window; > struct resource *res; > resource_size_t offset; > char bus_addr[64]; > char *fmt; > - u8 bus = PCI_BUSNUM(db); > + struct device *parent = bridge->dev.parent; > > b = pci_alloc_bus(NULL); > if (!b) > return NULL; > > - b->sysdata = sysdata; > - b->ops = ops; > - b->number = b->busn_res.start = bus; > + b->sysdata = bridge->sysdata; I think bridge should be the b->sysdata here. > + b->ops = bridge->ops; See comment above why I don't think this is necessary. > + b->number = b->busn_res.start = bridge->busnum; > pci_bus_assign_domain_nr(b, parent); > - b2 = pci_find_bus(pci_domain_nr(b), bus); > + bridge->domain = pci_domain_nr(b); Do you really want to overwrite the bridge's domain with the one from a bus that could possibly be rejected a couple of lines further down? As an asside: if we are doing the split of pci_host_bridge from root bus creation it is worth in my opinion to move the domain setup in pci_create_host_bridge() and stop fiddling with it here. Otherwise it looks to me like you are heading in the right direction. Best regards, Liviu > + b2 = pci_find_bus(pci_domain_nr(b), bridge->busnum); > 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); > - if (!bridge) > - goto err_out; > - > - 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); > @@ -1950,7 +1899,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, > > 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", pci_domain_nr(b), b->number); > error = device_register(&b->dev); > if (error) > goto class_dev_reg_err; > @@ -1966,12 +1915,11 @@ struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, > printk(KERN_INFO "PCI host bridge to bus %s\n", dev_name(&b->dev)); > > /* Add initial resources to the bus */ > - list_for_each_entry_safe(window, n, resources, list) { > - list_move_tail(&window->list, &bridge->windows); > + list_for_each_entry(window, &bridge->windows, list) { > res = window->res; > offset = window->offset; > if (res->flags & IORESOURCE_BUS) > - pci_bus_insert_busn_res(b, bus, res->end); > + pci_bus_insert_busn_res(b, b->number, res->end); > else > pci_bus_add_resource(b, res, 0); > if (offset) { > @@ -2001,6 +1949,25 @@ err_out: > return NULL; > } > > +struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus, > + struct pci_ops *ops, void *sysdata, struct list_head *resources) > +{ > + struct pci_host_bridge *host; > + > + host = pci_create_host_bridge(parent, bus, ops, > + sysdata ,resources); > + if (!host) > + return NULL; > + > + host->bus = __pci_create_root_bus(host); > + if (!host->bus) { > + pci_free_host_bridge(host); > + return NULL; > + } > + > + return host->bus; > +} > + > int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max) > { > struct resource *res = &b->busn_res; > @@ -2069,40 +2036,37 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, u32 db, > { > struct pci_host_bridge_window *window; > bool found = false; > - struct pci_bus *b; > - LIST_HEAD(default_res); > + struct pci_host_bridge *host; > int max; > > - if (!resources) { > - pci_add_resource(&default_res, &ioport_resource); > - pci_add_resource(&default_res, &iomem_resource); > - pci_add_resource(&default_res, &busn_resource); > - } else { > - list_for_each_entry(window, resources, list) > - if (window->res->flags & IORESOURCE_BUS) { > - found = true; > - break; > - } > - } > + host = pci_create_host_bridge(parent, db, ops, sysdata, resources); > + if (!host) > + return NULL; > > - b = pci_create_root_bus(parent, db, ops, sysdata, > - resources ? resources : &default_res); > - if (!b) > + list_for_each_entry(window, &host->windows, list) > + if (window->res->flags & IORESOURCE_BUS) { > + found = true; > + break; > + } > + > + host->bus = __pci_create_root_bus(host); > + if (!host->bus) { > + pci_free_host_bridge(host); > return NULL; > + } > > if (!found) { > - dev_info(&b->dev, > + dev_info(&host->bus->dev, > "No busn resource found for root bus, will use [bus %02x-ff]\n", > PCI_BUSNUM(db)); > - pci_bus_insert_busn_res(b, PCI_BUSNUM(db), 255); > + pci_bus_insert_busn_res(host->bus, PCI_BUSNUM(db), 255); > } > > - max = pci_scan_child_bus(b); > - > + max = pci_scan_child_bus(host->bus); > if (!found) > - pci_bus_update_busn_res_end(b, max); > + pci_bus_update_busn_res_end(host->bus, max); > > - return b; > + return host->bus; > } > EXPORT_SYMBOL(pci_scan_root_bus); > > diff --git a/include/linux/pci.h b/include/linux/pci.h > index 8b11b38..daa7f40 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -402,7 +402,12 @@ struct pci_host_bridge_window { > struct pci_host_bridge { > struct device dev; > struct pci_bus *bus; /* root bus */ > + struct list_head list; > struct list_head windows; /* pci_host_bridge_windows */ > + int busnum; > + int domain; > + void *sysdata; > + struct pci_ops *ops; > void (*release_fn)(struct pci_host_bridge *); > void *release_data; > }; > @@ -413,7 +418,9 @@ void pci_set_host_bridge_release(struct pci_host_bridge *bridge, > void *release_data); > > int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge); > - > +struct pci_host_bridge *pci_create_host_bridge( > + struct device *parent, u32 db, struct pci_ops *ops, > + void *sys, struct list_head *resources); > /* > * The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that correspond > * to P2P or CardBus bridge windows) go in a table. Additional ones (for > @@ -770,6 +777,8 @@ void pci_bus_add_devices(const struct pci_bus *bus); > struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus, > struct pci_ops *ops, void *sysdata, > struct list_head *resources); > +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *host); > +void pci_free_host_bridge(struct pci_host_bridge *host); > 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); > -- > 1.7.1 > > -- > 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 2014/11/18 23:30, Liviu Dudau wrote: > On Mon, Nov 17, 2014 at 10:21:41AM +0000, Yijing Wang wrote: >> There are some common PCI infos like domain, msi_controller, these >> infos are saved in arch PCI sysdata, and lots arch specific functions >> like pci_domain_nr() and pcibios_msi_controller() required. >> We could separate pci_host_bridge creation out of pci_create_root_bus(), >> then we could put the common infos in, then we could eliminate >> the arch specifc functions. >> > > Please Cc: Yinghai Lu and Jiang Liu on future versions. > > More comments on the conversion of pci_create_root_bus(): That's my mistake, will add CC Yinghai and Jiang in next version. > >> Signed-off-by: Yijing Wang <wangyijing@huawei.com> >> --- ... >> + >> +static void pci_release_host_bridge_dev(struct device *dev) >> +{ >> + struct pci_host_bridge *bridge = to_pci_host_bridge(dev); >> + >> + if (bridge->release_fn) >> + bridge->release_fn(bridge); >> + pci_free_resource_list(&bridge->windows); >> + kfree(bridge); >> +} >> + >> +struct pci_host_bridge *pci_create_host_bridge( >> + struct device *parent, u32 db, >> + struct pci_ops *ops, void *sysdata, > > I don't thinks it is worth moving the buses' pci_ops into pci_host_bridge. It > might be more useful to have pci_host_bridge specific ops here. Because we want to create pci_host_bridge before pci root bus creation, so when we scan the root bus and child buses, we use pci_host_bridge as the only argument, and another pci_host_info will be addes in later patch, which one support carry the pci_host_bridge ops. > >> + struct list_head *resources) >> +{ >> + int error; >> + struct pci_bus *b; >> + struct pci_host_bridge *host, *h; ... >> -struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, >> - struct pci_ops *ops, void *sysdata, struct list_head *resources) >> +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *bridge) >> { >> int error; >> - struct pci_host_bridge *bridge; >> struct pci_bus *b, *b2; >> - struct pci_host_bridge_window *window, *n; >> + struct pci_host_bridge_window *window; >> struct resource *res; >> resource_size_t offset; >> char bus_addr[64]; >> char *fmt; >> - u8 bus = PCI_BUSNUM(db); >> + struct device *parent = bridge->dev.parent; >> >> b = pci_alloc_bus(NULL); >> if (!b) >> return NULL; >> >> - b->sysdata = sysdata; >> - b->ops = ops; >> - b->number = b->busn_res.start = bus; >> + b->sysdata = bridge->sysdata; > > I think bridge should be the b->sysdata here. ? what's the meaning? > >> + b->ops = bridge->ops; > > See comment above why I don't think this is necessary. > >> + b->number = b->busn_res.start = bridge->busnum; >> pci_bus_assign_domain_nr(b, parent); >> - b2 = pci_find_bus(pci_domain_nr(b), bus); >> + bridge->domain = pci_domain_nr(b); > > Do you really want to overwrite the bridge's domain with the one from a bus that > could possibly be rejected a couple of lines further down? > > As an asside: if we are doing the split of pci_host_bridge from root bus creation > it is worth in my opinion to move the domain setup in pci_create_host_bridge() > and stop fiddling with it here. Hi Liviu, these lines just temporary, I will remove it after all host drivers save its domain in pci_host_bridge. > > Otherwise it looks to me like you are heading in the right direction. Thanks! Yijing. > > Best regards, > Liviu > >> + b2 = pci_find_bus(pci_domain_nr(b), bridge->busnum); >> 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); >> - if (!bridge) >> - goto err_out; >> - >> - 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); >> @@ -1950,7 +1899,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, >> >> 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", pci_domain_nr(b), b->number); >> error = device_register(&b->dev); >> if (error) >> goto class_dev_reg_err; >> @@ -1966,12 +1915,11 @@ struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, >> printk(KERN_INFO "PCI host bridge to bus %s\n", dev_name(&b->dev)); >> >> /* Add initial resources to the bus */ >> - list_for_each_entry_safe(window, n, resources, list) { >> - list_move_tail(&window->list, &bridge->windows); >> + list_for_each_entry(window, &bridge->windows, list) { >> res = window->res; >> offset = window->offset; >> if (res->flags & IORESOURCE_BUS) >> - pci_bus_insert_busn_res(b, bus, res->end); >> + pci_bus_insert_busn_res(b, b->number, res->end); >> else >> pci_bus_add_resource(b, res, 0); >> if (offset) { >> @@ -2001,6 +1949,25 @@ err_out: >> return NULL; >> } >> >> +struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus, >> + struct pci_ops *ops, void *sysdata, struct list_head *resources) >> +{ >> + struct pci_host_bridge *host; >> + >> + host = pci_create_host_bridge(parent, bus, ops, >> + sysdata ,resources); >> + if (!host) >> + return NULL; >> + >> + host->bus = __pci_create_root_bus(host); >> + if (!host->bus) { >> + pci_free_host_bridge(host); >> + return NULL; >> + } >> + >> + return host->bus; >> +} >> + >> int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max) >> { >> struct resource *res = &b->busn_res; >> @@ -2069,40 +2036,37 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, u32 db, >> { >> struct pci_host_bridge_window *window; >> bool found = false; >> - struct pci_bus *b; >> - LIST_HEAD(default_res); >> + struct pci_host_bridge *host; >> int max; >> >> - if (!resources) { >> - pci_add_resource(&default_res, &ioport_resource); >> - pci_add_resource(&default_res, &iomem_resource); >> - pci_add_resource(&default_res, &busn_resource); >> - } else { >> - list_for_each_entry(window, resources, list) >> - if (window->res->flags & IORESOURCE_BUS) { >> - found = true; >> - break; >> - } >> - } >> + host = pci_create_host_bridge(parent, db, ops, sysdata, resources); >> + if (!host) >> + return NULL; >> >> - b = pci_create_root_bus(parent, db, ops, sysdata, >> - resources ? resources : &default_res); >> - if (!b) >> + list_for_each_entry(window, &host->windows, list) >> + if (window->res->flags & IORESOURCE_BUS) { >> + found = true; >> + break; >> + } >> + >> + host->bus = __pci_create_root_bus(host); >> + if (!host->bus) { >> + pci_free_host_bridge(host); >> return NULL; >> + } >> >> if (!found) { >> - dev_info(&b->dev, >> + dev_info(&host->bus->dev, >> "No busn resource found for root bus, will use [bus %02x-ff]\n", >> PCI_BUSNUM(db)); >> - pci_bus_insert_busn_res(b, PCI_BUSNUM(db), 255); >> + pci_bus_insert_busn_res(host->bus, PCI_BUSNUM(db), 255); >> } >> >> - max = pci_scan_child_bus(b); >> - >> + max = pci_scan_child_bus(host->bus); >> if (!found) >> - pci_bus_update_busn_res_end(b, max); >> + pci_bus_update_busn_res_end(host->bus, max); >> >> - return b; >> + return host->bus; >> } >> EXPORT_SYMBOL(pci_scan_root_bus); >> >> diff --git a/include/linux/pci.h b/include/linux/pci.h >> index 8b11b38..daa7f40 100644 >> --- a/include/linux/pci.h >> +++ b/include/linux/pci.h >> @@ -402,7 +402,12 @@ struct pci_host_bridge_window { >> struct pci_host_bridge { >> struct device dev; >> struct pci_bus *bus; /* root bus */ >> + struct list_head list; >> struct list_head windows; /* pci_host_bridge_windows */ >> + int busnum; >> + int domain; >> + void *sysdata; >> + struct pci_ops *ops; >> void (*release_fn)(struct pci_host_bridge *); >> void *release_data; >> }; >> @@ -413,7 +418,9 @@ void pci_set_host_bridge_release(struct pci_host_bridge *bridge, >> void *release_data); >> >> int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge); >> - >> +struct pci_host_bridge *pci_create_host_bridge( >> + struct device *parent, u32 db, struct pci_ops *ops, >> + void *sys, struct list_head *resources); >> /* >> * The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that correspond >> * to P2P or CardBus bridge windows) go in a table. Additional ones (for >> @@ -770,6 +777,8 @@ void pci_bus_add_devices(const struct pci_bus *bus); >> struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus, >> struct pci_ops *ops, void *sysdata, >> struct list_head *resources); >> +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *host); >> +void pci_free_host_bridge(struct pci_host_bridge *host); >> 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); >> -- >> 1.7.1 >> >> -- >> 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 >> >
>> We need, some platforms pass NULL pointer as host bridge parent. > > Yijing, > > May I suggest a different approach here? Rather than having to pass an opaque > pointer that gets converted by the host bridge driver back to the private > structure, what about promoting a new style of usage, that is similar to the > way device drivers work? Lets try to promote the embedding of the generic > pci_host_bridge structure in the host bridge specific structure! Then we can > access the private data doing container_of(). > > Something like this: > > struct pci_controller { > struct pci_host_bridge bridge; > /* private host bridge data here */ > ..... > }; > > #define PCI_CONTROLLER(bus) ({ > struct pci_host_bridge *hb = to_pci_host_bridge(bus->bridge); \ > container_of(hb, struct pci_controller, bridge); }) > > > Then we can retrieve the host bridge structure from everywhere we have a device. Hi Liviu, it looks good to me, because this change will involve lots platforms, I would think more about it. Thanks for your suggestion very much! :) Thanks! Yijing. > > Best regards, > Liviu > >> >>> >>>> + host = kzalloc(sizeof(*host), GFP_KERNEL); >>>> + if (!host) >>>> + return NULL; >>> >>> devm_kzalloc maybe? >> >> I don't know much detail about devm_kzalloc(), but we have no pci host driver >> here, and I found no devm_kzalloc() uses in core PCI code before. >> >>> >>>> + if (!resources) { >>>> + /* Use default IO/MEM/BUS resources*/ >>>> + pci_add_resource(&host->windows, &ioport_resource); >>>> + pci_add_resource(&host->windows, &iomem_resource); >>>> + pci_add_resource(&host->windows, &busn_resource); >>>> + } else { >>>> + list_for_each_entry_safe(window, n, resources, list) >>>> + list_move_tail(&window->list, &host->windows); >>>> + } >>> >>> I think we should assume that the correct resources are passed. You >>> could add a wrapper around this function to convert old platforms >>> though. >> >> OK, I will move these code out of pci_create_host_bridge, and add a wrapper >> to setup the default resources. >> >>> >>>> +EXPORT_SYMBOL(pci_create_host_bridge); >>> >>> EXPORT_SYMBOL_GPL() maybe? >> >> OK, will update it. >> >>> >>>> diff --git a/include/linux/pci.h b/include/linux/pci.h >>>> index 8b11b38..daa7f40 100644 >>>> --- a/include/linux/pci.h >>>> +++ b/include/linux/pci.h >>>> @@ -402,7 +402,12 @@ struct pci_host_bridge_window { >>>> struct pci_host_bridge { >>>> struct device dev; >>>> struct pci_bus *bus; /* root bus */ >>>> + struct list_head list; >>>> struct list_head windows; /* pci_host_bridge_windows */ >>>> + int busnum; >>> >>> The busnum should already be implied through the bus resource. >> >> Yes, I will consider remove it and introduce a helper function to get the root bus number, thanks! >> >> Thanks! >> Yijing. >> >>> >>> Arnd >>> >>> . >>> >> >> >> -- >> Thanks! >> Yijing >> >> -- >> 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, Nov 19, 2014 at 10:24:52AM +0800, Yijing Wang wrote: > >> We need, some platforms pass NULL pointer as host bridge parent. > > > > Yijing, > > > > May I suggest a different approach here? Rather than having to pass an opaque > > pointer that gets converted by the host bridge driver back to the private > > structure, what about promoting a new style of usage, that is similar to the > > way device drivers work? Lets try to promote the embedding of the generic > > pci_host_bridge structure in the host bridge specific structure! Then we can > > access the private data doing container_of(). > > > > Something like this: > > > > struct pci_controller { > > struct pci_host_bridge bridge; > > /* private host bridge data here */ > > ..... > > }; > > > > #define PCI_CONTROLLER(bus) ({ > > struct pci_host_bridge *hb = to_pci_host_bridge(bus->bridge); \ > > container_of(hb, struct pci_controller, bridge); }) > > > > > > Then we can retrieve the host bridge structure from everywhere we have a device. > > Hi Liviu, it looks good to me, because this change will involve lots platforms, > I would think more about it. Thanks for your suggestion very much! :) Given that I also look at this area maybe we should join forces and divide the problem? Best regards, Liviu > > > Thanks! > Yijing. > > > > > Best regards, > > Liviu > > > >> > >>> > >>>> + host = kzalloc(sizeof(*host), GFP_KERNEL); > >>>> + if (!host) > >>>> + return NULL; > >>> > >>> devm_kzalloc maybe? > >> > >> I don't know much detail about devm_kzalloc(), but we have no pci host driver > >> here, and I found no devm_kzalloc() uses in core PCI code before. > >> > >>> > >>>> + if (!resources) { > >>>> + /* Use default IO/MEM/BUS resources*/ > >>>> + pci_add_resource(&host->windows, &ioport_resource); > >>>> + pci_add_resource(&host->windows, &iomem_resource); > >>>> + pci_add_resource(&host->windows, &busn_resource); > >>>> + } else { > >>>> + list_for_each_entry_safe(window, n, resources, list) > >>>> + list_move_tail(&window->list, &host->windows); > >>>> + } > >>> > >>> I think we should assume that the correct resources are passed. You > >>> could add a wrapper around this function to convert old platforms > >>> though. > >> > >> OK, I will move these code out of pci_create_host_bridge, and add a wrapper > >> to setup the default resources. > >> > >>> > >>>> +EXPORT_SYMBOL(pci_create_host_bridge); > >>> > >>> EXPORT_SYMBOL_GPL() maybe? > >> > >> OK, will update it. > >> > >>> > >>>> diff --git a/include/linux/pci.h b/include/linux/pci.h > >>>> index 8b11b38..daa7f40 100644 > >>>> --- a/include/linux/pci.h > >>>> +++ b/include/linux/pci.h > >>>> @@ -402,7 +402,12 @@ struct pci_host_bridge_window { > >>>> struct pci_host_bridge { > >>>> struct device dev; > >>>> struct pci_bus *bus; /* root bus */ > >>>> + struct list_head list; > >>>> struct list_head windows; /* pci_host_bridge_windows */ > >>>> + int busnum; > >>> > >>> The busnum should already be implied through the bus resource. > >> > >> Yes, I will consider remove it and introduce a helper function to get the root bus number, thanks! > >> > >> Thanks! > >> Yijing. > >> > >>> > >>> Arnd > >>> > >>> . > >>> > >> > >> > >> -- > >> Thanks! > >> Yijing > >> > >> -- > >> 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 > >> > > > > > -- > Thanks! > Yijing > > -- > 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, Nov 19, 2014 at 01:42:52AM +0000, Yijing Wang wrote: > On 2014/11/18 23:30, Liviu Dudau wrote: > > On Mon, Nov 17, 2014 at 10:21:41AM +0000, Yijing Wang wrote: > >> There are some common PCI infos like domain, msi_controller, these > >> infos are saved in arch PCI sysdata, and lots arch specific functions > >> like pci_domain_nr() and pcibios_msi_controller() required. > >> We could separate pci_host_bridge creation out of pci_create_root_bus(), > >> then we could put the common infos in, then we could eliminate > >> the arch specifc functions. > >> > > > > Please Cc: Yinghai Lu and Jiang Liu on future versions. > > > > More comments on the conversion of pci_create_root_bus(): > > That's my mistake, will add CC Yinghai and Jiang in next version. > > > > >> Signed-off-by: Yijing Wang <wangyijing@huawei.com> > >> --- > ... > >> + > >> +static void pci_release_host_bridge_dev(struct device *dev) > >> +{ > >> + struct pci_host_bridge *bridge = to_pci_host_bridge(dev); > >> + > >> + if (bridge->release_fn) > >> + bridge->release_fn(bridge); > >> + pci_free_resource_list(&bridge->windows); > >> + kfree(bridge); > >> +} > >> + > >> +struct pci_host_bridge *pci_create_host_bridge( > >> + struct device *parent, u32 db, > >> + struct pci_ops *ops, void *sysdata, > > > > I don't thinks it is worth moving the buses' pci_ops into pci_host_bridge. It > > might be more useful to have pci_host_bridge specific ops here. > > Because we want to create pci_host_bridge before pci root bus creation, > so when we scan the root bus and child buses, we use pci_host_bridge as > the only argument, and another pci_host_info will be addes in later patch, > which one support carry the pci_host_bridge ops. But pci_create_root_bus() already has a pci_ops argument, I don't see the reason to drop that. pci_create_host_bridge() can get pci_host_bridge ops while pci_create_root_bus() gets the bus ops. For find out the MSI controller, the domain number and any other HB specific stuff, you use the HB ops. For config R/W acceses you use bus ops. > > > > >> + struct list_head *resources) > >> +{ > >> + int error; > >> + struct pci_bus *b; > >> + struct pci_host_bridge *host, *h; > ... > >> -struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, > >> - struct pci_ops *ops, void *sysdata, struct list_head *resources) > >> +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *bridge) > >> { > >> int error; > >> - struct pci_host_bridge *bridge; > >> struct pci_bus *b, *b2; > >> - struct pci_host_bridge_window *window, *n; > >> + struct pci_host_bridge_window *window; > >> struct resource *res; > >> resource_size_t offset; > >> char bus_addr[64]; > >> char *fmt; > >> - u8 bus = PCI_BUSNUM(db); > >> + struct device *parent = bridge->dev.parent; > >> > >> b = pci_alloc_bus(NULL); > >> if (!b) > >> return NULL; > >> > >> - b->sysdata = sysdata; > >> - b->ops = ops; > >> - b->number = b->busn_res.start = bus; > >> + b->sysdata = bridge->sysdata; > > > > I think bridge should be the b->sysdata here. > > ? what's the meaning? Currently, bus->sysdata holds a pointer to the arch/driver host bridge structure, as passed in pci_create_root_bus(). If you agree with my idea of wrapping the host bridge driver structure around the pci_host_bridge, then we will always have a way of retrieving that information; but for backwards compatibility we could set bus->sysdata to be the bridge. Then existing macros that convert sysdata to pci_controller can be reused after being updated. Best regards, Liviu > > > > >> + b->ops = bridge->ops; > > > > See comment above why I don't think this is necessary. > > > >> + b->number = b->busn_res.start = bridge->busnum; > >> pci_bus_assign_domain_nr(b, parent); > >> - b2 = pci_find_bus(pci_domain_nr(b), bus); > >> + bridge->domain = pci_domain_nr(b); > > > > Do you really want to overwrite the bridge's domain with the one from a bus that > > could possibly be rejected a couple of lines further down? > > > > As an asside: if we are doing the split of pci_host_bridge from root bus creation > > it is worth in my opinion to move the domain setup in pci_create_host_bridge() > > and stop fiddling with it here. > > > Hi Liviu, these lines just temporary, I will remove it after all host drivers > save its domain in pci_host_bridge. > > > > > > Otherwise it looks to me like you are heading in the right direction. > > Thanks! > Yijing. > > > > > Best regards, > > Liviu > > > >> + b2 = pci_find_bus(pci_domain_nr(b), bridge->busnum); > >> 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); > >> - if (!bridge) > >> - goto err_out; > >> - > >> - 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); > >> @@ -1950,7 +1899,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, > >> > >> 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", pci_domain_nr(b), b->number); > >> error = device_register(&b->dev); > >> if (error) > >> goto class_dev_reg_err; > >> @@ -1966,12 +1915,11 @@ struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, > >> printk(KERN_INFO "PCI host bridge to bus %s\n", dev_name(&b->dev)); > >> > >> /* Add initial resources to the bus */ > >> - list_for_each_entry_safe(window, n, resources, list) { > >> - list_move_tail(&window->list, &bridge->windows); > >> + list_for_each_entry(window, &bridge->windows, list) { > >> res = window->res; > >> offset = window->offset; > >> if (res->flags & IORESOURCE_BUS) > >> - pci_bus_insert_busn_res(b, bus, res->end); > >> + pci_bus_insert_busn_res(b, b->number, res->end); > >> else > >> pci_bus_add_resource(b, res, 0); > >> if (offset) { > >> @@ -2001,6 +1949,25 @@ err_out: > >> return NULL; > >> } > >> > >> +struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus, > >> + struct pci_ops *ops, void *sysdata, struct list_head *resources) > >> +{ > >> + struct pci_host_bridge *host; > >> + > >> + host = pci_create_host_bridge(parent, bus, ops, > >> + sysdata ,resources); > >> + if (!host) > >> + return NULL; > >> + > >> + host->bus = __pci_create_root_bus(host); > >> + if (!host->bus) { > >> + pci_free_host_bridge(host); > >> + return NULL; > >> + } > >> + > >> + return host->bus; > >> +} > >> + > >> int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max) > >> { > >> struct resource *res = &b->busn_res; > >> @@ -2069,40 +2036,37 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, u32 db, > >> { > >> struct pci_host_bridge_window *window; > >> bool found = false; > >> - struct pci_bus *b; > >> - LIST_HEAD(default_res); > >> + struct pci_host_bridge *host; > >> int max; > >> > >> - if (!resources) { > >> - pci_add_resource(&default_res, &ioport_resource); > >> - pci_add_resource(&default_res, &iomem_resource); > >> - pci_add_resource(&default_res, &busn_resource); > >> - } else { > >> - list_for_each_entry(window, resources, list) > >> - if (window->res->flags & IORESOURCE_BUS) { > >> - found = true; > >> - break; > >> - } > >> - } > >> + host = pci_create_host_bridge(parent, db, ops, sysdata, resources); > >> + if (!host) > >> + return NULL; > >> > >> - b = pci_create_root_bus(parent, db, ops, sysdata, > >> - resources ? resources : &default_res); > >> - if (!b) > >> + list_for_each_entry(window, &host->windows, list) > >> + if (window->res->flags & IORESOURCE_BUS) { > >> + found = true; > >> + break; > >> + } > >> + > >> + host->bus = __pci_create_root_bus(host); > >> + if (!host->bus) { > >> + pci_free_host_bridge(host); > >> return NULL; > >> + } > >> > >> if (!found) { > >> - dev_info(&b->dev, > >> + dev_info(&host->bus->dev, > >> "No busn resource found for root bus, will use [bus %02x-ff]\n", > >> PCI_BUSNUM(db)); > >> - pci_bus_insert_busn_res(b, PCI_BUSNUM(db), 255); > >> + pci_bus_insert_busn_res(host->bus, PCI_BUSNUM(db), 255); > >> } > >> > >> - max = pci_scan_child_bus(b); > >> - > >> + max = pci_scan_child_bus(host->bus); > >> if (!found) > >> - pci_bus_update_busn_res_end(b, max); > >> + pci_bus_update_busn_res_end(host->bus, max); > >> > >> - return b; > >> + return host->bus; > >> } > >> EXPORT_SYMBOL(pci_scan_root_bus); > >> > >> diff --git a/include/linux/pci.h b/include/linux/pci.h > >> index 8b11b38..daa7f40 100644 > >> --- a/include/linux/pci.h > >> +++ b/include/linux/pci.h > >> @@ -402,7 +402,12 @@ struct pci_host_bridge_window { > >> struct pci_host_bridge { > >> struct device dev; > >> struct pci_bus *bus; /* root bus */ > >> + struct list_head list; > >> struct list_head windows; /* pci_host_bridge_windows */ > >> + int busnum; > >> + int domain; > >> + void *sysdata; > >> + struct pci_ops *ops; > >> void (*release_fn)(struct pci_host_bridge *); > >> void *release_data; > >> }; > >> @@ -413,7 +418,9 @@ void pci_set_host_bridge_release(struct pci_host_bridge *bridge, > >> void *release_data); > >> > >> int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge); > >> - > >> +struct pci_host_bridge *pci_create_host_bridge( > >> + struct device *parent, u32 db, struct pci_ops *ops, > >> + void *sys, struct list_head *resources); > >> /* > >> * The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that correspond > >> * to P2P or CardBus bridge windows) go in a table. Additional ones (for > >> @@ -770,6 +777,8 @@ void pci_bus_add_devices(const struct pci_bus *bus); > >> struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus, > >> struct pci_ops *ops, void *sysdata, > >> struct list_head *resources); > >> +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *host); > >> +void pci_free_host_bridge(struct pci_host_bridge *host); > >> 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); > >> -- > >> 1.7.1 > >> > >> -- > >> 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 > >> > > > > > -- > Thanks! > Yijing > > -- > 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 >
>>> Something like this: >>> >>> struct pci_controller { >>> struct pci_host_bridge bridge; >>> /* private host bridge data here */ >>> ..... >>> }; >>> >>> #define PCI_CONTROLLER(bus) ({ >>> struct pci_host_bridge *hb = to_pci_host_bridge(bus->bridge); \ >>> container_of(hb, struct pci_controller, bridge); }) >>> >>> >>> Then we can retrieve the host bridge structure from everywhere we have a device. >> >> Hi Liviu, it looks good to me, because this change will involve lots platforms, >> I would think more about it. Thanks for your suggestion very much! :) > > Given that I also look at this area maybe we should join forces and divide the problem? That's better, we could cooperate to do this. :) > > Best regards, > Liviu > >> >> >> Thanks! >> Yijing. >> >>> >>> Best regards, >>> Liviu >>> >>>> >>>>> >>>>>> + host = kzalloc(sizeof(*host), GFP_KERNEL); >>>>>> + if (!host) >>>>>> + return NULL; >>>>> >>>>> devm_kzalloc maybe? >>>> >>>> I don't know much detail about devm_kzalloc(), but we have no pci host driver >>>> here, and I found no devm_kzalloc() uses in core PCI code before. >>>> >>>>> >>>>>> + if (!resources) { >>>>>> + /* Use default IO/MEM/BUS resources*/ >>>>>> + pci_add_resource(&host->windows, &ioport_resource); >>>>>> + pci_add_resource(&host->windows, &iomem_resource); >>>>>> + pci_add_resource(&host->windows, &busn_resource); >>>>>> + } else { >>>>>> + list_for_each_entry_safe(window, n, resources, list) >>>>>> + list_move_tail(&window->list, &host->windows); >>>>>> + } >>>>> >>>>> I think we should assume that the correct resources are passed. You >>>>> could add a wrapper around this function to convert old platforms >>>>> though. >>>> >>>> OK, I will move these code out of pci_create_host_bridge, and add a wrapper >>>> to setup the default resources. >>>> >>>>> >>>>>> +EXPORT_SYMBOL(pci_create_host_bridge); >>>>> >>>>> EXPORT_SYMBOL_GPL() maybe? >>>> >>>> OK, will update it. >>>> >>>>> >>>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h >>>>>> index 8b11b38..daa7f40 100644 >>>>>> --- a/include/linux/pci.h >>>>>> +++ b/include/linux/pci.h >>>>>> @@ -402,7 +402,12 @@ struct pci_host_bridge_window { >>>>>> struct pci_host_bridge { >>>>>> struct device dev; >>>>>> struct pci_bus *bus; /* root bus */ >>>>>> + struct list_head list; >>>>>> struct list_head windows; /* pci_host_bridge_windows */ >>>>>> + int busnum; >>>>> >>>>> The busnum should already be implied through the bus resource. >>>> >>>> Yes, I will consider remove it and introduce a helper function to get the root bus number, thanks! >>>> >>>> Thanks! >>>> Yijing. >>>> >>>>> >>>>> Arnd >>>>> >>>>> . >>>>> >>>> >>>> >>>> -- >>>> Thanks! >>>> Yijing >>>> >>>> -- >>>> 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 >>>> >>> >> >> >> -- >> Thanks! >> Yijing >> >> -- >> 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/ >> >
>>>> +static void pci_release_host_bridge_dev(struct device *dev) >>>> +{ >>>> + struct pci_host_bridge *bridge = to_pci_host_bridge(dev); >>>> + >>>> + if (bridge->release_fn) >>>> + bridge->release_fn(bridge); >>>> + pci_free_resource_list(&bridge->windows); >>>> + kfree(bridge); >>>> +} >>>> + >>>> +struct pci_host_bridge *pci_create_host_bridge( >>>> + struct device *parent, u32 db, >>>> + struct pci_ops *ops, void *sysdata, >>> >>> I don't thinks it is worth moving the buses' pci_ops into pci_host_bridge. It >>> might be more useful to have pci_host_bridge specific ops here. >> >> Because we want to create pci_host_bridge before pci root bus creation, >> so when we scan the root bus and child buses, we use pci_host_bridge as >> the only argument, and another pci_host_info will be addes in later patch, >> which one support carry the pci_host_bridge ops. > > But pci_create_root_bus() already has a pci_ops argument, I don't see the reason > to drop that. > > pci_create_host_bridge() can get pci_host_bridge ops while pci_create_root_bus() gets > the bus ops. For find out the MSI controller, the domain number and any other HB > specific stuff, you use the HB ops. For config R/W acceses you use bus ops. > I want to unexport pci_create_root_bus() if we have pci_create_host_bridge(). >>>> -struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, >>>> - struct pci_ops *ops, void *sysdata, struct list_head *resources) >>>> +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *bridge) >>>> { >>>> int error; >>>> - struct pci_host_bridge *bridge; >>>> struct pci_bus *b, *b2; >>>> - struct pci_host_bridge_window *window, *n; >>>> + struct pci_host_bridge_window *window; >>>> struct resource *res; >>>> resource_size_t offset; >>>> char bus_addr[64]; >>>> char *fmt; >>>> - u8 bus = PCI_BUSNUM(db); >>>> + struct device *parent = bridge->dev.parent; >>>> >>>> b = pci_alloc_bus(NULL); >>>> if (!b) >>>> return NULL; >>>> >>>> - b->sysdata = sysdata; >>>> - b->ops = ops; >>>> - b->number = b->busn_res.start = bus; >>>> + b->sysdata = bridge->sysdata; >>> >>> I think bridge should be the b->sysdata here. >> >> ? what's the meaning? > > Currently, bus->sysdata holds a pointer to the arch/driver host bridge structure, as passed > in pci_create_root_bus(). If you agree with my idea of wrapping the host bridge driver > structure around the pci_host_bridge, then we will always have a way of retrieving that > information; but for backwards compatibility we could set bus->sysdata to be the bridge. > Then existing macros that convert sysdata to pci_controller can be reused after being > updated. I think wrapping the host bridge driver structure around the pci_host_bridge could help us to find the private pci_controller, but in this case, pci_host_bridge is not a pointer so we put all pci_host_bridge init stuff in host drivers ? And if we still use arch spec macros convert ssydata to pci_controller, so what's the greatest advantage ? If we move domain and msi_chip out of sysdata, what's other things in sysdata still need us to convert in PCI core ? > > Best regards, > Liviu > >> >>> >>>> + b->ops = bridge->ops; >>> >>> See comment above why I don't think this is necessary. >>> >>>> + b->number = b->busn_res.start = bridge->busnum; >>>> pci_bus_assign_domain_nr(b, parent); >>>> - b2 = pci_find_bus(pci_domain_nr(b), bus); >>>> + bridge->domain = pci_domain_nr(b); >>> >>> Do you really want to overwrite the bridge's domain with the one from a bus that >>> could possibly be rejected a couple of lines further down? >>> >>> As an asside: if we are doing the split of pci_host_bridge from root bus creation >>> it is worth in my opinion to move the domain setup in pci_create_host_bridge() >>> and stop fiddling with it here. >> >> >> Hi Liviu, these lines just temporary, I will remove it after all host drivers >> save its domain in pci_host_bridge. >> >> >>> >>> Otherwise it looks to me like you are heading in the right direction. >> >> Thanks! >> Yijing. >> >>> >>> Best regards, >>> Liviu >>> >>>> + b2 = pci_find_bus(pci_domain_nr(b), bridge->busnum); >>>> 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); >>>> - if (!bridge) >>>> - goto err_out; >>>> - >>>> - 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); >>>> @@ -1950,7 +1899,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, >>>> >>>> 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", pci_domain_nr(b), b->number); >>>> error = device_register(&b->dev); >>>> if (error) >>>> goto class_dev_reg_err; >>>> @@ -1966,12 +1915,11 @@ struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, >>>> printk(KERN_INFO "PCI host bridge to bus %s\n", dev_name(&b->dev)); >>>> >>>> /* Add initial resources to the bus */ >>>> - list_for_each_entry_safe(window, n, resources, list) { >>>> - list_move_tail(&window->list, &bridge->windows); >>>> + list_for_each_entry(window, &bridge->windows, list) { >>>> res = window->res; >>>> offset = window->offset; >>>> if (res->flags & IORESOURCE_BUS) >>>> - pci_bus_insert_busn_res(b, bus, res->end); >>>> + pci_bus_insert_busn_res(b, b->number, res->end); >>>> else >>>> pci_bus_add_resource(b, res, 0); >>>> if (offset) { >>>> @@ -2001,6 +1949,25 @@ err_out: >>>> return NULL; >>>> } >>>> >>>> +struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus, >>>> + struct pci_ops *ops, void *sysdata, struct list_head *resources) >>>> +{ >>>> + struct pci_host_bridge *host; >>>> + >>>> + host = pci_create_host_bridge(parent, bus, ops, >>>> + sysdata ,resources); >>>> + if (!host) >>>> + return NULL; >>>> + >>>> + host->bus = __pci_create_root_bus(host); >>>> + if (!host->bus) { >>>> + pci_free_host_bridge(host); >>>> + return NULL; >>>> + } >>>> + >>>> + return host->bus; >>>> +} >>>> + >>>> int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max) >>>> { >>>> struct resource *res = &b->busn_res; >>>> @@ -2069,40 +2036,37 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, u32 db, >>>> { >>>> struct pci_host_bridge_window *window; >>>> bool found = false; >>>> - struct pci_bus *b; >>>> - LIST_HEAD(default_res); >>>> + struct pci_host_bridge *host; >>>> int max; >>>> >>>> - if (!resources) { >>>> - pci_add_resource(&default_res, &ioport_resource); >>>> - pci_add_resource(&default_res, &iomem_resource); >>>> - pci_add_resource(&default_res, &busn_resource); >>>> - } else { >>>> - list_for_each_entry(window, resources, list) >>>> - if (window->res->flags & IORESOURCE_BUS) { >>>> - found = true; >>>> - break; >>>> - } >>>> - } >>>> + host = pci_create_host_bridge(parent, db, ops, sysdata, resources); >>>> + if (!host) >>>> + return NULL; >>>> >>>> - b = pci_create_root_bus(parent, db, ops, sysdata, >>>> - resources ? resources : &default_res); >>>> - if (!b) >>>> + list_for_each_entry(window, &host->windows, list) >>>> + if (window->res->flags & IORESOURCE_BUS) { >>>> + found = true; >>>> + break; >>>> + } >>>> + >>>> + host->bus = __pci_create_root_bus(host); >>>> + if (!host->bus) { >>>> + pci_free_host_bridge(host); >>>> return NULL; >>>> + } >>>> >>>> if (!found) { >>>> - dev_info(&b->dev, >>>> + dev_info(&host->bus->dev, >>>> "No busn resource found for root bus, will use [bus %02x-ff]\n", >>>> PCI_BUSNUM(db)); >>>> - pci_bus_insert_busn_res(b, PCI_BUSNUM(db), 255); >>>> + pci_bus_insert_busn_res(host->bus, PCI_BUSNUM(db), 255); >>>> } >>>> >>>> - max = pci_scan_child_bus(b); >>>> - >>>> + max = pci_scan_child_bus(host->bus); >>>> if (!found) >>>> - pci_bus_update_busn_res_end(b, max); >>>> + pci_bus_update_busn_res_end(host->bus, max); >>>> >>>> - return b; >>>> + return host->bus; >>>> } >>>> EXPORT_SYMBOL(pci_scan_root_bus); >>>> >>>> diff --git a/include/linux/pci.h b/include/linux/pci.h >>>> index 8b11b38..daa7f40 100644 >>>> --- a/include/linux/pci.h >>>> +++ b/include/linux/pci.h >>>> @@ -402,7 +402,12 @@ struct pci_host_bridge_window { >>>> struct pci_host_bridge { >>>> struct device dev; >>>> struct pci_bus *bus; /* root bus */ >>>> + struct list_head list; >>>> struct list_head windows; /* pci_host_bridge_windows */ >>>> + int busnum; >>>> + int domain; >>>> + void *sysdata; >>>> + struct pci_ops *ops; >>>> void (*release_fn)(struct pci_host_bridge *); >>>> void *release_data; >>>> }; >>>> @@ -413,7 +418,9 @@ void pci_set_host_bridge_release(struct pci_host_bridge *bridge, >>>> void *release_data); >>>> >>>> int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge); >>>> - >>>> +struct pci_host_bridge *pci_create_host_bridge( >>>> + struct device *parent, u32 db, struct pci_ops *ops, >>>> + void *sys, struct list_head *resources); >>>> /* >>>> * The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that correspond >>>> * to P2P or CardBus bridge windows) go in a table. Additional ones (for >>>> @@ -770,6 +777,8 @@ void pci_bus_add_devices(const struct pci_bus *bus); >>>> struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus, >>>> struct pci_ops *ops, void *sysdata, >>>> struct list_head *resources); >>>> +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *host); >>>> +void pci_free_host_bridge(struct pci_host_bridge *host); >>>> 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); >>>> -- >>>> 1.7.1 >>>> >>>> -- >>>> 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 >>>> >>> >> >> >> -- >> Thanks! >> Yijing >> >> -- >> 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 Thu, Nov 20, 2014 at 02:47:35AM +0000, Yijing Wang wrote: > >>>> +static void pci_release_host_bridge_dev(struct device *dev) > >>>> +{ > >>>> + struct pci_host_bridge *bridge = to_pci_host_bridge(dev); > >>>> + > >>>> + if (bridge->release_fn) > >>>> + bridge->release_fn(bridge); > >>>> + pci_free_resource_list(&bridge->windows); > >>>> + kfree(bridge); > >>>> +} > >>>> + > >>>> +struct pci_host_bridge *pci_create_host_bridge( > >>>> + struct device *parent, u32 db, > >>>> + struct pci_ops *ops, void *sysdata, > >>> > >>> I don't thinks it is worth moving the buses' pci_ops into pci_host_bridge. It > >>> might be more useful to have pci_host_bridge specific ops here. > >> > >> Because we want to create pci_host_bridge before pci root bus creation, > >> so when we scan the root bus and child buses, we use pci_host_bridge as > >> the only argument, and another pci_host_info will be addes in later patch, > >> which one support carry the pci_host_bridge ops. > > > > But pci_create_root_bus() already has a pci_ops argument, I don't see the reason > > to drop that. > > > > pci_create_host_bridge() can get pci_host_bridge ops while pci_create_root_bus() gets > > the bus ops. For find out the MSI controller, the domain number and any other HB > > specific stuff, you use the HB ops. For config R/W acceses you use bus ops. > > > > I want to unexport pci_create_root_bus() if we have pci_create_host_bridge(). That's fine, but the function signature doesn't change, right? And I think we should keep pci_scan_root_bus() with the current signature as well (except change sysdata into a struct pci_host_bridge *). We create the host bridge in one step, stuff it with all the data that we need for scanning the root and associated busses, and then call pci_scan_root_bus(). pci_host_bridge has the ops from pci_create_host_bridge() and the root bus from pci_scan_root_bus(). Hope this makes sense. > > > >>>> -struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, > >>>> - struct pci_ops *ops, void *sysdata, struct list_head *resources) > >>>> +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *bridge) > >>>> { > >>>> int error; > >>>> - struct pci_host_bridge *bridge; > >>>> struct pci_bus *b, *b2; > >>>> - struct pci_host_bridge_window *window, *n; > >>>> + struct pci_host_bridge_window *window; > >>>> struct resource *res; > >>>> resource_size_t offset; > >>>> char bus_addr[64]; > >>>> char *fmt; > >>>> - u8 bus = PCI_BUSNUM(db); > >>>> + struct device *parent = bridge->dev.parent; > >>>> > >>>> b = pci_alloc_bus(NULL); > >>>> if (!b) > >>>> return NULL; > >>>> > >>>> - b->sysdata = sysdata; > >>>> - b->ops = ops; > >>>> - b->number = b->busn_res.start = bus; > >>>> + b->sysdata = bridge->sysdata; > >>> > >>> I think bridge should be the b->sysdata here. > >> > >> ? what's the meaning? > > > > Currently, bus->sysdata holds a pointer to the arch/driver host bridge structure, as passed > > in pci_create_root_bus(). If you agree with my idea of wrapping the host bridge driver > > structure around the pci_host_bridge, then we will always have a way of retrieving that > > information; but for backwards compatibility we could set bus->sysdata to be the bridge. > > Then existing macros that convert sysdata to pci_controller can be reused after being > > updated. > > I think wrapping the host bridge driver structure around the pci_host_bridge could help > us to find the private pci_controller, but in this case, pci_host_bridge is not a pointer > so we put all pci_host_bridge init stuff in host drivers ? Of course we put pci_host_bridge init *calls* in the host drivers because we need to do host driver specific initialisation there anyway. > And if we still use arch spec > macros convert ssydata to pci_controller, so what's the greatest advantage ? Agree, the macro is not useful unless we have a common pci_controller structure. Sorry about the noise. > > If we move domain and msi_chip out of sysdata, what's other things in sysdata still need us > to convert in PCI core ? I've never said we need to keep sysdata. From my discussion with Bjorn a year ago the idea was to put the sysdata members into pci_host_bridge structure and remove sysdata member from pci_bus. pci_scan_root_bus() can still get a parameter called sysdata if we want, but it will be a struct pci_host_bridge * type. Best regards, Liviu > > > > > > > Best regards, > > Liviu > > > >> > >>> > >>>> + b->ops = bridge->ops; > >>> > >>> See comment above why I don't think this is necessary. > >>> > >>>> + b->number = b->busn_res.start = bridge->busnum; > >>>> pci_bus_assign_domain_nr(b, parent); > >>>> - b2 = pci_find_bus(pci_domain_nr(b), bus); > >>>> + bridge->domain = pci_domain_nr(b); > >>> > >>> Do you really want to overwrite the bridge's domain with the one from a bus that > >>> could possibly be rejected a couple of lines further down? > >>> > >>> As an asside: if we are doing the split of pci_host_bridge from root bus creation > >>> it is worth in my opinion to move the domain setup in pci_create_host_bridge() > >>> and stop fiddling with it here. > >> > >> > >> Hi Liviu, these lines just temporary, I will remove it after all host drivers > >> save its domain in pci_host_bridge. > >> > >> > >>> > >>> Otherwise it looks to me like you are heading in the right direction. > >> > >> Thanks! > >> Yijing. > >> > >>> > >>> Best regards, > >>> Liviu > >>> > >>>> + b2 = pci_find_bus(pci_domain_nr(b), bridge->busnum); > >>>> 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); > >>>> - if (!bridge) > >>>> - goto err_out; > >>>> - > >>>> - 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); > >>>> @@ -1950,7 +1899,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, > >>>> > >>>> 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", pci_domain_nr(b), b->number); > >>>> error = device_register(&b->dev); > >>>> if (error) > >>>> goto class_dev_reg_err; > >>>> @@ -1966,12 +1915,11 @@ struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, > >>>> printk(KERN_INFO "PCI host bridge to bus %s\n", dev_name(&b->dev)); > >>>> > >>>> /* Add initial resources to the bus */ > >>>> - list_for_each_entry_safe(window, n, resources, list) { > >>>> - list_move_tail(&window->list, &bridge->windows); > >>>> + list_for_each_entry(window, &bridge->windows, list) { > >>>> res = window->res; > >>>> offset = window->offset; > >>>> if (res->flags & IORESOURCE_BUS) > >>>> - pci_bus_insert_busn_res(b, bus, res->end); > >>>> + pci_bus_insert_busn_res(b, b->number, res->end); > >>>> else > >>>> pci_bus_add_resource(b, res, 0); > >>>> if (offset) { > >>>> @@ -2001,6 +1949,25 @@ err_out: > >>>> return NULL; > >>>> } > >>>> > >>>> +struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus, > >>>> + struct pci_ops *ops, void *sysdata, struct list_head *resources) > >>>> +{ > >>>> + struct pci_host_bridge *host; > >>>> + > >>>> + host = pci_create_host_bridge(parent, bus, ops, > >>>> + sysdata ,resources); > >>>> + if (!host) > >>>> + return NULL; > >>>> + > >>>> + host->bus = __pci_create_root_bus(host); > >>>> + if (!host->bus) { > >>>> + pci_free_host_bridge(host); > >>>> + return NULL; > >>>> + } > >>>> + > >>>> + return host->bus; > >>>> +} > >>>> + > >>>> int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max) > >>>> { > >>>> struct resource *res = &b->busn_res; > >>>> @@ -2069,40 +2036,37 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, u32 db, > >>>> { > >>>> struct pci_host_bridge_window *window; > >>>> bool found = false; > >>>> - struct pci_bus *b; > >>>> - LIST_HEAD(default_res); > >>>> + struct pci_host_bridge *host; > >>>> int max; > >>>> > >>>> - if (!resources) { > >>>> - pci_add_resource(&default_res, &ioport_resource); > >>>> - pci_add_resource(&default_res, &iomem_resource); > >>>> - pci_add_resource(&default_res, &busn_resource); > >>>> - } else { > >>>> - list_for_each_entry(window, resources, list) > >>>> - if (window->res->flags & IORESOURCE_BUS) { > >>>> - found = true; > >>>> - break; > >>>> - } > >>>> - } > >>>> + host = pci_create_host_bridge(parent, db, ops, sysdata, resources); > >>>> + if (!host) > >>>> + return NULL; > >>>> > >>>> - b = pci_create_root_bus(parent, db, ops, sysdata, > >>>> - resources ? resources : &default_res); > >>>> - if (!b) > >>>> + list_for_each_entry(window, &host->windows, list) > >>>> + if (window->res->flags & IORESOURCE_BUS) { > >>>> + found = true; > >>>> + break; > >>>> + } > >>>> + > >>>> + host->bus = __pci_create_root_bus(host); > >>>> + if (!host->bus) { > >>>> + pci_free_host_bridge(host); > >>>> return NULL; > >>>> + } > >>>> > >>>> if (!found) { > >>>> - dev_info(&b->dev, > >>>> + dev_info(&host->bus->dev, > >>>> "No busn resource found for root bus, will use [bus %02x-ff]\n", > >>>> PCI_BUSNUM(db)); > >>>> - pci_bus_insert_busn_res(b, PCI_BUSNUM(db), 255); > >>>> + pci_bus_insert_busn_res(host->bus, PCI_BUSNUM(db), 255); > >>>> } > >>>> > >>>> - max = pci_scan_child_bus(b); > >>>> - > >>>> + max = pci_scan_child_bus(host->bus); > >>>> if (!found) > >>>> - pci_bus_update_busn_res_end(b, max); > >>>> + pci_bus_update_busn_res_end(host->bus, max); > >>>> > >>>> - return b; > >>>> + return host->bus; > >>>> } > >>>> EXPORT_SYMBOL(pci_scan_root_bus); > >>>> > >>>> diff --git a/include/linux/pci.h b/include/linux/pci.h > >>>> index 8b11b38..daa7f40 100644 > >>>> --- a/include/linux/pci.h > >>>> +++ b/include/linux/pci.h > >>>> @@ -402,7 +402,12 @@ struct pci_host_bridge_window { > >>>> struct pci_host_bridge { > >>>> struct device dev; > >>>> struct pci_bus *bus; /* root bus */ > >>>> + struct list_head list; > >>>> struct list_head windows; /* pci_host_bridge_windows */ > >>>> + int busnum; > >>>> + int domain; > >>>> + void *sysdata; > >>>> + struct pci_ops *ops; > >>>> void (*release_fn)(struct pci_host_bridge *); > >>>> void *release_data; > >>>> }; > >>>> @@ -413,7 +418,9 @@ void pci_set_host_bridge_release(struct pci_host_bridge *bridge, > >>>> void *release_data); > >>>> > >>>> int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge); > >>>> - > >>>> +struct pci_host_bridge *pci_create_host_bridge( > >>>> + struct device *parent, u32 db, struct pci_ops *ops, > >>>> + void *sys, struct list_head *resources); > >>>> /* > >>>> * The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that correspond > >>>> * to P2P or CardBus bridge windows) go in a table. Additional ones (for > >>>> @@ -770,6 +777,8 @@ void pci_bus_add_devices(const struct pci_bus *bus); > >>>> struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus, > >>>> struct pci_ops *ops, void *sysdata, > >>>> struct list_head *resources); > >>>> +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *host); > >>>> +void pci_free_host_bridge(struct pci_host_bridge *host); > >>>> 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); > >>>> -- > >>>> 1.7.1 > >>>> > >>>> -- > >>>> 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 > >>>> > >>> > >> > >> > >> -- > >> Thanks! > >> Yijing > >> > >> -- > >> 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 > >> > > > > > -- > Thanks! > Yijing > >
>>> pci_create_host_bridge() can get pci_host_bridge ops while pci_create_root_bus() gets >>> the bus ops. For find out the MSI controller, the domain number and any other HB >>> specific stuff, you use the HB ops. For config R/W acceses you use bus ops. >>> >> >> I want to unexport pci_create_root_bus() if we have pci_create_host_bridge(). > > That's fine, but the function signature doesn't change, right? And I think we should keep > pci_scan_root_bus() with the current signature as well (except change sysdata into a > struct pci_host_bridge *). We create the host bridge in one step, stuff it with all the > data that we need for scanning the root and associated busses, and then call pci_scan_root_bus(). > pci_host_bridge has the ops from pci_create_host_bridge() and the root bus from pci_scan_root_bus(). Liviu, could you have some draft patches about replace sysdata with pci_host_bridge, so we could discuss the details base it. :) > > Hope this makes sense. > >> >> >>>>>> -struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, >>>>>> - struct pci_ops *ops, void *sysdata, struct list_head *resources) >>>>>> +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *bridge) >>>>>> { >>>>>> int error; >>>>>> - struct pci_host_bridge *bridge; >>>>>> struct pci_bus *b, *b2; >>>>>> - struct pci_host_bridge_window *window, *n; >>>>>> + struct pci_host_bridge_window *window; >>>>>> struct resource *res; >>>>>> resource_size_t offset; >>>>>> char bus_addr[64]; >>>>>> char *fmt; >>>>>> - u8 bus = PCI_BUSNUM(db); >>>>>> + struct device *parent = bridge->dev.parent; >>>>>> >>>>>> b = pci_alloc_bus(NULL); >>>>>> if (!b) >>>>>> return NULL; >>>>>> >>>>>> - b->sysdata = sysdata; >>>>>> - b->ops = ops; >>>>>> - b->number = b->busn_res.start = bus; >>>>>> + b->sysdata = bridge->sysdata; >>>>> >>>>> I think bridge should be the b->sysdata here. >>>> >>>> ? what's the meaning? >>> >>> Currently, bus->sysdata holds a pointer to the arch/driver host bridge structure, as passed >>> in pci_create_root_bus(). If you agree with my idea of wrapping the host bridge driver >>> structure around the pci_host_bridge, then we will always have a way of retrieving that >>> information; but for backwards compatibility we could set bus->sysdata to be the bridge. >>> Then existing macros that convert sysdata to pci_controller can be reused after being >>> updated. >> >> I think wrapping the host bridge driver structure around the pci_host_bridge could help >> us to find the private pci_controller, but in this case, pci_host_bridge is not a pointer >> so we put all pci_host_bridge init stuff in host drivers ? > > Of course we put pci_host_bridge init *calls* in the host drivers because we need to do host > driver specific initialisation there anyway. > >> And if we still use arch spec >> macros convert ssydata to pci_controller, so what's the greatest advantage ? > > Agree, the macro is not useful unless we have a common pci_controller structure. Sorry about > the noise. > >> >> If we move domain and msi_chip out of sysdata, what's other things in sysdata still need us >> to convert in PCI core ? > > I've never said we need to keep sysdata. From my discussion with Bjorn a year ago the idea was > to put the sysdata members into pci_host_bridge structure and remove sysdata member from pci_bus. > pci_scan_root_bus() can still get a parameter called sysdata if we want, but it will be a > struct pci_host_bridge * type. Fine. > > Best regards, > Liviu > >> >> >> >>> >>> Best regards, >>> Liviu >>> >>>> >>>>> >>>>>> + b->ops = bridge->ops; >>>>> >>>>> See comment above why I don't think this is necessary. >>>>> >>>>>> + b->number = b->busn_res.start = bridge->busnum; >>>>>> pci_bus_assign_domain_nr(b, parent); >>>>>> - b2 = pci_find_bus(pci_domain_nr(b), bus); >>>>>> + bridge->domain = pci_domain_nr(b); >>>>> >>>>> Do you really want to overwrite the bridge's domain with the one from a bus that >>>>> could possibly be rejected a couple of lines further down? >>>>> >>>>> As an asside: if we are doing the split of pci_host_bridge from root bus creation >>>>> it is worth in my opinion to move the domain setup in pci_create_host_bridge() >>>>> and stop fiddling with it here. >>>> >>>> >>>> Hi Liviu, these lines just temporary, I will remove it after all host drivers >>>> save its domain in pci_host_bridge. >>>> >>>> >>>>> >>>>> Otherwise it looks to me like you are heading in the right direction. >>>> >>>> Thanks! >>>> Yijing. >>>> >>>>> >>>>> Best regards, >>>>> Liviu >>>>> >>>>>> + b2 = pci_find_bus(pci_domain_nr(b), bridge->busnum); >>>>>> 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); >>>>>> - if (!bridge) >>>>>> - goto err_out; >>>>>> - >>>>>> - 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); >>>>>> @@ -1950,7 +1899,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, >>>>>> >>>>>> 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", pci_domain_nr(b), b->number); >>>>>> error = device_register(&b->dev); >>>>>> if (error) >>>>>> goto class_dev_reg_err; >>>>>> @@ -1966,12 +1915,11 @@ struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, >>>>>> printk(KERN_INFO "PCI host bridge to bus %s\n", dev_name(&b->dev)); >>>>>> >>>>>> /* Add initial resources to the bus */ >>>>>> - list_for_each_entry_safe(window, n, resources, list) { >>>>>> - list_move_tail(&window->list, &bridge->windows); >>>>>> + list_for_each_entry(window, &bridge->windows, list) { >>>>>> res = window->res; >>>>>> offset = window->offset; >>>>>> if (res->flags & IORESOURCE_BUS) >>>>>> - pci_bus_insert_busn_res(b, bus, res->end); >>>>>> + pci_bus_insert_busn_res(b, b->number, res->end); >>>>>> else >>>>>> pci_bus_add_resource(b, res, 0); >>>>>> if (offset) { >>>>>> @@ -2001,6 +1949,25 @@ err_out: >>>>>> return NULL; >>>>>> } >>>>>> >>>>>> +struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus, >>>>>> + struct pci_ops *ops, void *sysdata, struct list_head *resources) >>>>>> +{ >>>>>> + struct pci_host_bridge *host; >>>>>> + >>>>>> + host = pci_create_host_bridge(parent, bus, ops, >>>>>> + sysdata ,resources); >>>>>> + if (!host) >>>>>> + return NULL; >>>>>> + >>>>>> + host->bus = __pci_create_root_bus(host); >>>>>> + if (!host->bus) { >>>>>> + pci_free_host_bridge(host); >>>>>> + return NULL; >>>>>> + } >>>>>> + >>>>>> + return host->bus; >>>>>> +} >>>>>> + >>>>>> int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max) >>>>>> { >>>>>> struct resource *res = &b->busn_res; >>>>>> @@ -2069,40 +2036,37 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, u32 db, >>>>>> { >>>>>> struct pci_host_bridge_window *window; >>>>>> bool found = false; >>>>>> - struct pci_bus *b; >>>>>> - LIST_HEAD(default_res); >>>>>> + struct pci_host_bridge *host; >>>>>> int max; >>>>>> >>>>>> - if (!resources) { >>>>>> - pci_add_resource(&default_res, &ioport_resource); >>>>>> - pci_add_resource(&default_res, &iomem_resource); >>>>>> - pci_add_resource(&default_res, &busn_resource); >>>>>> - } else { >>>>>> - list_for_each_entry(window, resources, list) >>>>>> - if (window->res->flags & IORESOURCE_BUS) { >>>>>> - found = true; >>>>>> - break; >>>>>> - } >>>>>> - } >>>>>> + host = pci_create_host_bridge(parent, db, ops, sysdata, resources); >>>>>> + if (!host) >>>>>> + return NULL; >>>>>> >>>>>> - b = pci_create_root_bus(parent, db, ops, sysdata, >>>>>> - resources ? resources : &default_res); >>>>>> - if (!b) >>>>>> + list_for_each_entry(window, &host->windows, list) >>>>>> + if (window->res->flags & IORESOURCE_BUS) { >>>>>> + found = true; >>>>>> + break; >>>>>> + } >>>>>> + >>>>>> + host->bus = __pci_create_root_bus(host); >>>>>> + if (!host->bus) { >>>>>> + pci_free_host_bridge(host); >>>>>> return NULL; >>>>>> + } >>>>>> >>>>>> if (!found) { >>>>>> - dev_info(&b->dev, >>>>>> + dev_info(&host->bus->dev, >>>>>> "No busn resource found for root bus, will use [bus %02x-ff]\n", >>>>>> PCI_BUSNUM(db)); >>>>>> - pci_bus_insert_busn_res(b, PCI_BUSNUM(db), 255); >>>>>> + pci_bus_insert_busn_res(host->bus, PCI_BUSNUM(db), 255); >>>>>> } >>>>>> >>>>>> - max = pci_scan_child_bus(b); >>>>>> - >>>>>> + max = pci_scan_child_bus(host->bus); >>>>>> if (!found) >>>>>> - pci_bus_update_busn_res_end(b, max); >>>>>> + pci_bus_update_busn_res_end(host->bus, max); >>>>>> >>>>>> - return b; >>>>>> + return host->bus; >>>>>> } >>>>>> EXPORT_SYMBOL(pci_scan_root_bus); >>>>>> >>>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h >>>>>> index 8b11b38..daa7f40 100644 >>>>>> --- a/include/linux/pci.h >>>>>> +++ b/include/linux/pci.h >>>>>> @@ -402,7 +402,12 @@ struct pci_host_bridge_window { >>>>>> struct pci_host_bridge { >>>>>> struct device dev; >>>>>> struct pci_bus *bus; /* root bus */ >>>>>> + struct list_head list; >>>>>> struct list_head windows; /* pci_host_bridge_windows */ >>>>>> + int busnum; >>>>>> + int domain; >>>>>> + void *sysdata; >>>>>> + struct pci_ops *ops; >>>>>> void (*release_fn)(struct pci_host_bridge *); >>>>>> void *release_data; >>>>>> }; >>>>>> @@ -413,7 +418,9 @@ void pci_set_host_bridge_release(struct pci_host_bridge *bridge, >>>>>> void *release_data); >>>>>> >>>>>> int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge); >>>>>> - >>>>>> +struct pci_host_bridge *pci_create_host_bridge( >>>>>> + struct device *parent, u32 db, struct pci_ops *ops, >>>>>> + void *sys, struct list_head *resources); >>>>>> /* >>>>>> * The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that correspond >>>>>> * to P2P or CardBus bridge windows) go in a table. Additional ones (for >>>>>> @@ -770,6 +777,8 @@ void pci_bus_add_devices(const struct pci_bus *bus); >>>>>> struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus, >>>>>> struct pci_ops *ops, void *sysdata, >>>>>> struct list_head *resources); >>>>>> +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *host); >>>>>> +void pci_free_host_bridge(struct pci_host_bridge *host); >>>>>> 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); >>>>>> -- >>>>>> 1.7.1 >>>>>> >>>>>> -- >>>>>> 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 >>>>>> >>>>> >>>> >>>> >>>> -- >>>> Thanks! >>>> Yijing >>>> >>>> -- >>>> 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 >>>> >>> >> >> >> -- >> Thanks! >> Yijing >> >> >
On Fri, Nov 21, 2014 at 02:53:01AM +0000, Yijing Wang wrote: > >>> pci_create_host_bridge() can get pci_host_bridge ops while pci_create_root_bus() gets > >>> the bus ops. For find out the MSI controller, the domain number and any other HB > >>> specific stuff, you use the HB ops. For config R/W acceses you use bus ops. > >>> > >> > >> I want to unexport pci_create_root_bus() if we have pci_create_host_bridge(). > > > > That's fine, but the function signature doesn't change, right? And I think we should keep > > pci_scan_root_bus() with the current signature as well (except change sysdata into a > > struct pci_host_bridge *). We create the host bridge in one step, stuff it with all the > > data that we need for scanning the root and associated busses, and then call pci_scan_root_bus(). > > pci_host_bridge has the ops from pci_create_host_bridge() and the root bus from pci_scan_root_bus(). > > Liviu, could you have some draft patches about replace sysdata with pci_host_bridge, so we could > discuss the details base it. :) Sure, I'll try to send something soon. Best regards, Liviu > > > > > Hope this makes sense. > > > >> > >> > >>>>>> -struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, > >>>>>> - struct pci_ops *ops, void *sysdata, struct list_head *resources) > >>>>>> +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *bridge) > >>>>>> { > >>>>>> int error; > >>>>>> - struct pci_host_bridge *bridge; > >>>>>> struct pci_bus *b, *b2; > >>>>>> - struct pci_host_bridge_window *window, *n; > >>>>>> + struct pci_host_bridge_window *window; > >>>>>> struct resource *res; > >>>>>> resource_size_t offset; > >>>>>> char bus_addr[64]; > >>>>>> char *fmt; > >>>>>> - u8 bus = PCI_BUSNUM(db); > >>>>>> + struct device *parent = bridge->dev.parent; > >>>>>> > >>>>>> b = pci_alloc_bus(NULL); > >>>>>> if (!b) > >>>>>> return NULL; > >>>>>> > >>>>>> - b->sysdata = sysdata; > >>>>>> - b->ops = ops; > >>>>>> - b->number = b->busn_res.start = bus; > >>>>>> + b->sysdata = bridge->sysdata; > >>>>> > >>>>> I think bridge should be the b->sysdata here. > >>>> > >>>> ? what's the meaning? > >>> > >>> Currently, bus->sysdata holds a pointer to the arch/driver host bridge structure, as passed > >>> in pci_create_root_bus(). If you agree with my idea of wrapping the host bridge driver > >>> structure around the pci_host_bridge, then we will always have a way of retrieving that > >>> information; but for backwards compatibility we could set bus->sysdata to be the bridge. > >>> Then existing macros that convert sysdata to pci_controller can be reused after being > >>> updated. > >> > >> I think wrapping the host bridge driver structure around the pci_host_bridge could help > >> us to find the private pci_controller, but in this case, pci_host_bridge is not a pointer > >> so we put all pci_host_bridge init stuff in host drivers ? > > > > Of course we put pci_host_bridge init *calls* in the host drivers because we need to do host > > driver specific initialisation there anyway. > > > >> And if we still use arch spec > >> macros convert ssydata to pci_controller, so what's the greatest advantage ? > > > > Agree, the macro is not useful unless we have a common pci_controller structure. Sorry about > > the noise. > > > >> > >> If we move domain and msi_chip out of sysdata, what's other things in sysdata still need us > >> to convert in PCI core ? > > > > I've never said we need to keep sysdata. From my discussion with Bjorn a year ago the idea was > > to put the sysdata members into pci_host_bridge structure and remove sysdata member from pci_bus. > > pci_scan_root_bus() can still get a parameter called sysdata if we want, but it will be a > > struct pci_host_bridge * type. > > Fine. > > > > > Best regards, > > Liviu > > > >> > >> > >> > >>> > >>> Best regards, > >>> Liviu > >>> > >>>> > >>>>> > >>>>>> + b->ops = bridge->ops; > >>>>> > >>>>> See comment above why I don't think this is necessary. > >>>>> > >>>>>> + b->number = b->busn_res.start = bridge->busnum; > >>>>>> pci_bus_assign_domain_nr(b, parent); > >>>>>> - b2 = pci_find_bus(pci_domain_nr(b), bus); > >>>>>> + bridge->domain = pci_domain_nr(b); > >>>>> > >>>>> Do you really want to overwrite the bridge's domain with the one from a bus that > >>>>> could possibly be rejected a couple of lines further down? > >>>>> > >>>>> As an asside: if we are doing the split of pci_host_bridge from root bus creation > >>>>> it is worth in my opinion to move the domain setup in pci_create_host_bridge() > >>>>> and stop fiddling with it here. > >>>> > >>>> > >>>> Hi Liviu, these lines just temporary, I will remove it after all host drivers > >>>> save its domain in pci_host_bridge. > >>>> > >>>> > >>>>> > >>>>> Otherwise it looks to me like you are heading in the right direction. > >>>> > >>>> Thanks! > >>>> Yijing. > >>>> > >>>>> > >>>>> Best regards, > >>>>> Liviu > >>>>> > >>>>>> + b2 = pci_find_bus(pci_domain_nr(b), bridge->busnum); > >>>>>> 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); > >>>>>> - if (!bridge) > >>>>>> - goto err_out; > >>>>>> - > >>>>>> - 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); > >>>>>> @@ -1950,7 +1899,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, > >>>>>> > >>>>>> 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", pci_domain_nr(b), b->number); > >>>>>> error = device_register(&b->dev); > >>>>>> if (error) > >>>>>> goto class_dev_reg_err; > >>>>>> @@ -1966,12 +1915,11 @@ struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, > >>>>>> printk(KERN_INFO "PCI host bridge to bus %s\n", dev_name(&b->dev)); > >>>>>> > >>>>>> /* Add initial resources to the bus */ > >>>>>> - list_for_each_entry_safe(window, n, resources, list) { > >>>>>> - list_move_tail(&window->list, &bridge->windows); > >>>>>> + list_for_each_entry(window, &bridge->windows, list) { > >>>>>> res = window->res; > >>>>>> offset = window->offset; > >>>>>> if (res->flags & IORESOURCE_BUS) > >>>>>> - pci_bus_insert_busn_res(b, bus, res->end); > >>>>>> + pci_bus_insert_busn_res(b, b->number, res->end); > >>>>>> else > >>>>>> pci_bus_add_resource(b, res, 0); > >>>>>> if (offset) { > >>>>>> @@ -2001,6 +1949,25 @@ err_out: > >>>>>> return NULL; > >>>>>> } > >>>>>> > >>>>>> +struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus, > >>>>>> + struct pci_ops *ops, void *sysdata, struct list_head *resources) > >>>>>> +{ > >>>>>> + struct pci_host_bridge *host; > >>>>>> + > >>>>>> + host = pci_create_host_bridge(parent, bus, ops, > >>>>>> + sysdata ,resources); > >>>>>> + if (!host) > >>>>>> + return NULL; > >>>>>> + > >>>>>> + host->bus = __pci_create_root_bus(host); > >>>>>> + if (!host->bus) { > >>>>>> + pci_free_host_bridge(host); > >>>>>> + return NULL; > >>>>>> + } > >>>>>> + > >>>>>> + return host->bus; > >>>>>> +} > >>>>>> + > >>>>>> int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max) > >>>>>> { > >>>>>> struct resource *res = &b->busn_res; > >>>>>> @@ -2069,40 +2036,37 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, u32 db, > >>>>>> { > >>>>>> struct pci_host_bridge_window *window; > >>>>>> bool found = false; > >>>>>> - struct pci_bus *b; > >>>>>> - LIST_HEAD(default_res); > >>>>>> + struct pci_host_bridge *host; > >>>>>> int max; > >>>>>> > >>>>>> - if (!resources) { > >>>>>> - pci_add_resource(&default_res, &ioport_resource); > >>>>>> - pci_add_resource(&default_res, &iomem_resource); > >>>>>> - pci_add_resource(&default_res, &busn_resource); > >>>>>> - } else { > >>>>>> - list_for_each_entry(window, resources, list) > >>>>>> - if (window->res->flags & IORESOURCE_BUS) { > >>>>>> - found = true; > >>>>>> - break; > >>>>>> - } > >>>>>> - } > >>>>>> + host = pci_create_host_bridge(parent, db, ops, sysdata, resources); > >>>>>> + if (!host) > >>>>>> + return NULL; > >>>>>> > >>>>>> - b = pci_create_root_bus(parent, db, ops, sysdata, > >>>>>> - resources ? resources : &default_res); > >>>>>> - if (!b) > >>>>>> + list_for_each_entry(window, &host->windows, list) > >>>>>> + if (window->res->flags & IORESOURCE_BUS) { > >>>>>> + found = true; > >>>>>> + break; > >>>>>> + } > >>>>>> + > >>>>>> + host->bus = __pci_create_root_bus(host); > >>>>>> + if (!host->bus) { > >>>>>> + pci_free_host_bridge(host); > >>>>>> return NULL; > >>>>>> + } > >>>>>> > >>>>>> if (!found) { > >>>>>> - dev_info(&b->dev, > >>>>>> + dev_info(&host->bus->dev, > >>>>>> "No busn resource found for root bus, will use [bus %02x-ff]\n", > >>>>>> PCI_BUSNUM(db)); > >>>>>> - pci_bus_insert_busn_res(b, PCI_BUSNUM(db), 255); > >>>>>> + pci_bus_insert_busn_res(host->bus, PCI_BUSNUM(db), 255); > >>>>>> } > >>>>>> > >>>>>> - max = pci_scan_child_bus(b); > >>>>>> - > >>>>>> + max = pci_scan_child_bus(host->bus); > >>>>>> if (!found) > >>>>>> - pci_bus_update_busn_res_end(b, max); > >>>>>> + pci_bus_update_busn_res_end(host->bus, max); > >>>>>> > >>>>>> - return b; > >>>>>> + return host->bus; > >>>>>> } > >>>>>> EXPORT_SYMBOL(pci_scan_root_bus); > >>>>>> > >>>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h > >>>>>> index 8b11b38..daa7f40 100644 > >>>>>> --- a/include/linux/pci.h > >>>>>> +++ b/include/linux/pci.h > >>>>>> @@ -402,7 +402,12 @@ struct pci_host_bridge_window { > >>>>>> struct pci_host_bridge { > >>>>>> struct device dev; > >>>>>> struct pci_bus *bus; /* root bus */ > >>>>>> + struct list_head list; > >>>>>> struct list_head windows; /* pci_host_bridge_windows */ > >>>>>> + int busnum; > >>>>>> + int domain; > >>>>>> + void *sysdata; > >>>>>> + struct pci_ops *ops; > >>>>>> void (*release_fn)(struct pci_host_bridge *); > >>>>>> void *release_data; > >>>>>> }; > >>>>>> @@ -413,7 +418,9 @@ void pci_set_host_bridge_release(struct pci_host_bridge *bridge, > >>>>>> void *release_data); > >>>>>> > >>>>>> int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge); > >>>>>> - > >>>>>> +struct pci_host_bridge *pci_create_host_bridge( > >>>>>> + struct device *parent, u32 db, struct pci_ops *ops, > >>>>>> + void *sys, struct list_head *resources); > >>>>>> /* > >>>>>> * The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that correspond > >>>>>> * to P2P or CardBus bridge windows) go in a table. Additional ones (for > >>>>>> @@ -770,6 +777,8 @@ void pci_bus_add_devices(const struct pci_bus *bus); > >>>>>> struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus, > >>>>>> struct pci_ops *ops, void *sysdata, > >>>>>> struct list_head *resources); > >>>>>> +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *host); > >>>>>> +void pci_free_host_bridge(struct pci_host_bridge *host); > >>>>>> 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); > >>>>>> -- > >>>>>> 1.7.1 > >>>>>> > >>>>>> -- > >>>>>> 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 > >>>>>> > >>>>> > >>>> > >>>> > >>>> -- > >>>> Thanks! > >>>> Yijing > >>>> > >>>> -- > >>>> 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 > >>>> > >>> > >> > >> > >> -- > >> Thanks! > >> Yijing > >> > >> > > > > > -- > Thanks! > Yijing > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c index 0e5f3c9..e31604f 100644 --- a/drivers/pci/host-bridge.c +++ b/drivers/pci/host-bridge.c @@ -8,6 +8,105 @@ #include "pci.h" +LIST_HEAD(pci_host_bridge_list); +DECLARE_RWSEM(pci_host_bridge_sem); + +static struct resource busn_resource = { + .name = "PCI busn", + .start = 0, + .end = 255, + .flags = IORESOURCE_BUS, +}; + +static void pci_release_host_bridge_dev(struct device *dev) +{ + struct pci_host_bridge *bridge = to_pci_host_bridge(dev); + + if (bridge->release_fn) + bridge->release_fn(bridge); + pci_free_resource_list(&bridge->windows); + kfree(bridge); +} + +struct pci_host_bridge *pci_create_host_bridge( + struct device *parent, u32 db, + struct pci_ops *ops, void *sysdata, + struct list_head *resources) +{ + int error; + struct pci_bus *b; + struct pci_host_bridge *host, *h; + struct pci_host_bridge_window *window, *n; + + down_read(&pci_host_bridge_sem); + list_for_each_entry(h, &pci_host_bridge_list, list) { + if (h->domain == PCI_DOMAIN(db) && + h->busnum == PCI_BUSNUM(db)) { + dev_dbg(&h->dev, "pci host bridge exist\n"); + up_read(&pci_host_bridge_sem); + return NULL; + } + } + up_read(&pci_host_bridge_sem); + + host = kzalloc(sizeof(*host), GFP_KERNEL); + if (!host) + return NULL; + + host->sysdata = sysdata; + host->busnum = PCI_BUSNUM(db); + host->domain = PCI_DOMAIN(db); + host->ops = ops; + host->dev.parent = parent; + INIT_LIST_HEAD(&host->windows); + host->dev.release = pci_release_host_bridge_dev; + + /* this is hack, just for build, will be removed later*/ + b = kzalloc(sizeof(*b), GFP_KERNEL); + b->sysdata = sysdata; + pci_bus_assign_domain_nr(b, parent); + host->domain = pci_domain_nr(b); + + if (!resources) { + /* Use default IO/MEM/BUS resources*/ + pci_add_resource(&host->windows, &ioport_resource); + pci_add_resource(&host->windows, &iomem_resource); + pci_add_resource(&host->windows, &busn_resource); + } else { + list_for_each_entry_safe(window, n, resources, list) + list_move_tail(&window->list, &host->windows); + } + + dev_set_name(&host->dev, "pci%04x:%02x", host->domain, + host->busnum); + error = pcibios_root_bridge_prepare(host); + if(error) { + kfree(host); + return NULL; + } + + error = device_register(&host->dev); + if (error) { + put_device(&host->dev); + return NULL; + } + + down_write(&pci_host_bridge_sem); + list_add_tail(&host->list, &pci_host_bridge_list); + up_write(&pci_host_bridge_sem); + return host; +} +EXPORT_SYMBOL(pci_create_host_bridge); + +void pci_free_host_bridge(struct pci_host_bridge *host) +{ + down_write(&pci_host_bridge_sem); + list_del(&host->list); + up_write(&pci_host_bridge_sem); + + device_unregister(&host->dev); +} + static struct pci_bus *find_pci_root_bus(struct pci_bus *bus) { while (bus->parent) diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index adf4d09..d472da4 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -17,13 +17,6 @@ #define CARDBUS_LATENCY_TIMER 176 /* secondary latency timer */ #define CARDBUS_RESERVE_BUSNR 3 -static struct resource busn_resource = { - .name = "PCI busn", - .start = 0, - .end = 255, - .flags = IORESOURCE_BUS, -}; - /* Ugh. Need to stop exporting this to modules. */ LIST_HEAD(pci_root_buses); EXPORT_SYMBOL(pci_root_buses); @@ -508,31 +501,6 @@ static struct pci_bus *pci_alloc_bus(struct pci_bus *parent) return b; } -static void pci_release_host_bridge_dev(struct device *dev) -{ - struct pci_host_bridge *bridge = to_pci_host_bridge(dev); - - if (bridge->release_fn) - bridge->release_fn(bridge); - - pci_free_resource_list(&bridge->windows); - - kfree(bridge); -} - -static struct pci_host_bridge *pci_alloc_host_bridge(struct pci_bus *b) -{ - struct pci_host_bridge *bridge; - - bridge = kzalloc(sizeof(*bridge), GFP_KERNEL); - if (!bridge) - return NULL; - - INIT_LIST_HEAD(&bridge->windows); - bridge->bus = b; - return bridge; -} - static const unsigned char pcix_bus_speed[] = { PCI_SPEED_UNKNOWN, /* 0 */ PCI_SPEED_66MHz_PCIX, /* 1 */ @@ -1895,52 +1863,33 @@ void __weak pcibios_remove_bus(struct pci_bus *bus) { } -struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, - struct pci_ops *ops, void *sysdata, struct list_head *resources) +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *bridge) { int error; - struct pci_host_bridge *bridge; struct pci_bus *b, *b2; - struct pci_host_bridge_window *window, *n; + struct pci_host_bridge_window *window; struct resource *res; resource_size_t offset; char bus_addr[64]; char *fmt; - u8 bus = PCI_BUSNUM(db); + struct device *parent = bridge->dev.parent; b = pci_alloc_bus(NULL); if (!b) return NULL; - b->sysdata = sysdata; - b->ops = ops; - b->number = b->busn_res.start = bus; + b->sysdata = bridge->sysdata; + b->ops = bridge->ops; + b->number = b->busn_res.start = bridge->busnum; pci_bus_assign_domain_nr(b, parent); - b2 = pci_find_bus(pci_domain_nr(b), bus); + bridge->domain = pci_domain_nr(b); + b2 = pci_find_bus(pci_domain_nr(b), bridge->busnum); 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); - if (!bridge) - goto err_out; - - 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); @@ -1950,7 +1899,7 @@ struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, 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", pci_domain_nr(b), b->number); error = device_register(&b->dev); if (error) goto class_dev_reg_err; @@ -1966,12 +1915,11 @@ struct pci_bus *pci_create_root_bus(struct device *parent, u32 db, printk(KERN_INFO "PCI host bridge to bus %s\n", dev_name(&b->dev)); /* Add initial resources to the bus */ - list_for_each_entry_safe(window, n, resources, list) { - list_move_tail(&window->list, &bridge->windows); + list_for_each_entry(window, &bridge->windows, list) { res = window->res; offset = window->offset; if (res->flags & IORESOURCE_BUS) - pci_bus_insert_busn_res(b, bus, res->end); + pci_bus_insert_busn_res(b, b->number, res->end); else pci_bus_add_resource(b, res, 0); if (offset) { @@ -2001,6 +1949,25 @@ err_out: return NULL; } +struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus, + struct pci_ops *ops, void *sysdata, struct list_head *resources) +{ + struct pci_host_bridge *host; + + host = pci_create_host_bridge(parent, bus, ops, + sysdata ,resources); + if (!host) + return NULL; + + host->bus = __pci_create_root_bus(host); + if (!host->bus) { + pci_free_host_bridge(host); + return NULL; + } + + return host->bus; +} + int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int bus_max) { struct resource *res = &b->busn_res; @@ -2069,40 +2036,37 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, u32 db, { struct pci_host_bridge_window *window; bool found = false; - struct pci_bus *b; - LIST_HEAD(default_res); + struct pci_host_bridge *host; int max; - if (!resources) { - pci_add_resource(&default_res, &ioport_resource); - pci_add_resource(&default_res, &iomem_resource); - pci_add_resource(&default_res, &busn_resource); - } else { - list_for_each_entry(window, resources, list) - if (window->res->flags & IORESOURCE_BUS) { - found = true; - break; - } - } + host = pci_create_host_bridge(parent, db, ops, sysdata, resources); + if (!host) + return NULL; - b = pci_create_root_bus(parent, db, ops, sysdata, - resources ? resources : &default_res); - if (!b) + list_for_each_entry(window, &host->windows, list) + if (window->res->flags & IORESOURCE_BUS) { + found = true; + break; + } + + host->bus = __pci_create_root_bus(host); + if (!host->bus) { + pci_free_host_bridge(host); return NULL; + } if (!found) { - dev_info(&b->dev, + dev_info(&host->bus->dev, "No busn resource found for root bus, will use [bus %02x-ff]\n", PCI_BUSNUM(db)); - pci_bus_insert_busn_res(b, PCI_BUSNUM(db), 255); + pci_bus_insert_busn_res(host->bus, PCI_BUSNUM(db), 255); } - max = pci_scan_child_bus(b); - + max = pci_scan_child_bus(host->bus); if (!found) - pci_bus_update_busn_res_end(b, max); + pci_bus_update_busn_res_end(host->bus, max); - return b; + return host->bus; } EXPORT_SYMBOL(pci_scan_root_bus); diff --git a/include/linux/pci.h b/include/linux/pci.h index 8b11b38..daa7f40 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -402,7 +402,12 @@ struct pci_host_bridge_window { struct pci_host_bridge { struct device dev; struct pci_bus *bus; /* root bus */ + struct list_head list; struct list_head windows; /* pci_host_bridge_windows */ + int busnum; + int domain; + void *sysdata; + struct pci_ops *ops; void (*release_fn)(struct pci_host_bridge *); void *release_data; }; @@ -413,7 +418,9 @@ void pci_set_host_bridge_release(struct pci_host_bridge *bridge, void *release_data); int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge); - +struct pci_host_bridge *pci_create_host_bridge( + struct device *parent, u32 db, struct pci_ops *ops, + void *sys, struct list_head *resources); /* * The first PCI_BRIDGE_RESOURCE_NUM PCI bus resources (those that correspond * to P2P or CardBus bridge windows) go in a table. Additional ones (for @@ -770,6 +777,8 @@ void pci_bus_add_devices(const struct pci_bus *bus); struct pci_bus *pci_create_root_bus(struct device *parent, u32 bus, struct pci_ops *ops, void *sysdata, struct list_head *resources); +struct pci_bus *__pci_create_root_bus(struct pci_host_bridge *host); +void pci_free_host_bridge(struct pci_host_bridge *host); 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);
There are some common PCI infos like domain, msi_controller, these infos are saved in arch PCI sysdata, and lots arch specific functions like pci_domain_nr() and pcibios_msi_controller() required. We could separate pci_host_bridge creation out of pci_create_root_bus(), then we could put the common infos in, then we could eliminate the arch specifc functions. Signed-off-by: Yijing Wang <wangyijing@huawei.com> --- drivers/pci/host-bridge.c | 99 +++++++++++++++++++++++++++++++++ drivers/pci/probe.c | 134 ++++++++++++++++---------------------------- include/linux/pci.h | 11 +++- 3 files changed, 158 insertions(+), 86 deletions(-)