From: Yijing Wang <wangyijing@huawei.com>
To: Liviu Dudau <Liviu.Dudau@arm.com>
Cc: Liviu Dudau <liviu@dudau.co.uk>, Tony Luck <tony.luck@intel.com>,
Russell King <linux@arm.linux.org.uk>,
"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
"x86@kernel.org" <x86@kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"huxinwei@huawei.com" <huxinwei@huawei.com>,
Thierry Reding <thierry.reding@gmail.com>,
"suravee.suthikulpanit@amd.com" <suravee.suthikulpanit@amd.com>,
Bjorn Helgaas <bhelgaas@google.com>,
"linux-ia64@vger.kernel.org" <linux-ia64@vger.kernel.org>,
Thomas Gleixner <tglx@linutronix.de>, Wuyun <wuyun.wu@huawei.com>,
"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>
Subject: Re: [RFC PATCH 07/16] PCI: Separate pci_host_bridge creation out of pci_create_root_bus()
Date: Wed, 19 Nov 2014 09:42:52 +0800 [thread overview]
Message-ID: <546BF59C.8060203@huawei.com> (raw)
In-Reply-To: <20141118153020.GL12037@e106497-lin.cambridge.arm.com>
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
>>
>
--
Thanks!
Yijing
next prev parent reply other threads:[~2014-11-19 1:45 UTC|newest]
Thread overview: 65+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-11-17 10:21 [RFC PATCH 00/16] Refine PCI host bridge scan interfaces Yijing Wang
2014-11-17 10:21 ` [RFC PATCH 01/16] PCI: Enhance pci_scan_root_bus() to support default IO/MEM resources Yijing Wang
2014-11-17 10:08 ` Arnd Bergmann
2014-11-18 7:44 ` Yijing Wang
2014-11-18 9:36 ` Arnd Bergmann
2014-11-18 11:46 ` Yijing Wang
2014-11-18 14:23 ` Liviu Dudau
2014-11-19 1:15 ` Yijing Wang
2014-11-17 10:21 ` [RFC PATCH 02/16] PCI: Use pci_scan_root_bus() instead of pci_scan_bus() Yijing Wang
2014-11-18 14:28 ` Liviu Dudau
2014-11-19 1:19 ` Yijing Wang
2014-11-17 10:21 ` [RFC PATCH 03/16] PCI: Clean up pci_scan_bus() Yijing Wang
2014-11-17 10:21 ` [RFC PATCH 04/16] PCI: Rip out pci_bus_add_devices() from pci_scan_root_bus() Yijing Wang
2014-11-18 14:34 ` Liviu Dudau
2014-11-19 1:21 ` Yijing Wang
2014-11-17 10:21 ` [RFC PATCH 05/16] PCI: Use pci_scan_root_bus() instead of pci_scan_bus_parented() Yijing Wang
2014-11-17 10:21 ` [RFC PATCH 06/16] PCI: Use u32 type to combine PCI domain and bus number Yijing Wang
2014-11-17 10:21 ` [RFC PATCH 07/16] PCI: Separate pci_host_bridge creation out of pci_create_root_bus() Yijing Wang
2014-11-17 10:56 ` Arnd Bergmann
2014-11-18 8:32 ` Yijing Wang
2014-11-18 9:30 ` Arnd Bergmann
2014-11-18 11:44 ` Yijing Wang
2014-11-18 12:25 ` Arnd Bergmann
2014-11-18 12:41 ` Yijing Wang
2014-11-18 14:48 ` Liviu Dudau
2014-11-19 2:24 ` Yijing Wang
2014-11-19 16:29 ` Liviu Dudau
2014-11-20 2:00 ` Yijing Wang
2014-11-18 15:30 ` Liviu Dudau
2014-11-19 1:42 ` Yijing Wang [this message]
2014-11-19 16:37 ` Liviu Dudau
2014-11-20 2:47 ` Yijing Wang
2014-11-20 9:47 ` Liviu Dudau
2014-11-21 2:53 ` Yijing Wang
2014-11-21 9:53 ` Liviu Dudau
2014-11-17 10:21 ` [RFC PATCH 08/16] PCI: Introduce pci_scan_host_bridge() and pci_host_info Yijing Wang
2014-11-18 15:42 ` Liviu Dudau
2014-11-19 2:09 ` Yijing Wang
2014-11-19 16:41 ` Liviu Dudau
2014-11-20 2:54 ` Yijing Wang
2014-11-17 10:21 ` [RFC PATCH 09/16] PCI: Associate .get_msi_ctrl() with pci_host_bridge Yijing Wang
2014-11-17 15:03 ` Lorenzo Pieralisi
2014-11-17 10:21 ` [RFC PATCH 10/16] PCI: Add of_scan_bus() to pci_host_info Yijing Wang
2014-11-17 10:21 ` [RFC PATCH 11/16] x86/PCI: Use pci_scan_host_bridge() instead of pci_create_root_bus() Yijing Wang
2014-11-17 10:21 ` [RFC PATCH 12/16] ia64/PCI: Remove the redundant bus variable Yijing Wang
2014-11-17 10:21 ` [RFC PATCH 13/16] ia64/PCI: Use pci_scan_host_bridge() to refactor pci_acpi_scan_root() Yijing Wang
2014-11-17 10:21 ` [RFC PATCH 14/16] arm/PCI: Introduce pci_get_domain_nr() Yijing Wang
2014-11-17 12:08 ` Lorenzo Pieralisi
2014-11-18 0:55 ` Yijing Wang
2014-11-17 10:21 ` [RFC PATCH 15/16] arm/PCI: Use pci_scan_host_bridge() instead of pci_scan_root_bus() Yijing Wang
2014-11-17 10:21 ` [RFC PATCH 16/16] powerpc/PCI: Use pci_scan_host_bridge() to scan PCI bus Yijing Wang
2014-11-17 14:13 ` [RFC PATCH 00/16] Refine PCI host bridge scan interfaces Arnd Bergmann
2014-11-18 11:17 ` Yijing Wang
2014-11-18 11:30 ` Arnd Bergmann
2014-11-18 11:45 ` Lorenzo Pieralisi
2014-11-18 12:14 ` Yijing Wang
2014-11-18 12:17 ` Yijing Wang
2014-11-18 12:27 ` Arnd Bergmann
2014-11-20 12:01 ` Tomasz Nowicki
2014-11-20 13:15 ` Arnd Bergmann
2014-11-20 11:54 ` Tomasz Nowicki
2014-11-20 12:08 ` Liviu Dudau
2014-11-20 12:53 ` Tomasz Nowicki
2014-11-20 16:39 ` Liviu Dudau
2014-11-21 2:58 ` Yijing Wang
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=546BF59C.8060203@huawei.com \
--to=wangyijing@huawei.com \
--cc=Liviu.Dudau@arm.com \
--cc=bhelgaas@google.com \
--cc=huxinwei@huawei.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-ia64@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=linux@arm.linux.org.uk \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=liviu@dudau.co.uk \
--cc=suravee.suthikulpanit@amd.com \
--cc=tglx@linutronix.de \
--cc=thierry.reding@gmail.com \
--cc=tony.luck@intel.com \
--cc=wuyun.wu@huawei.com \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).