From: Yijing Wang <hidden> Date: 2014-11-17 09:41:05
This series is based Linux 3.18-rc1 and Lorenzo Pieralisi's
arm PCI domain cleanup patches, link:
https://patchwork.ozlabs.org/patch/407585/
Current pci scan interfaces like pci_scan_root_bus() and directly
call pci_create_root_bus()/pci_scan_child_bus() lack flexiblity.
Some platform infos like PCI domain and msi_chip have to be
associated to PCI bus by some arch specific function.
We want to make a generic pci_host_bridge, and make it hold
the platform infos or hook. Then we could eliminate the lots
of arch pci_domain_nr, also we could associate some platform
ops something like pci_get_msi_chip(struct pci_dev *dev)
with pci_host_bridge to avoid introduce arch weak functions.
This RFC version not for all platforms, just applied the new
scan interface in x86/arm/powerpc/ia64, I will refresh other
platforms after the core pci scan interfaces are ok.
Only test in x86.
Thanks!
Yijing.
Yijing Wang (16):
PCI: Enhance pci_scan_root_bus() to support default IO/MEM resources
PCI: Use pci_scan_root_bus() instead of pci_scan_bus()
PCI: Clean up pci_scan_bus()
PCI: Rip out pci_bus_add_devices() from pci_scan_root_bus()
PCI: Use pci_scan_root_bus() instead of pci_scan_bus_parented()
PCI: Use u32 type to combine PCI domain and bus number
PCI: Separate pci_host_bridge creation out of pci_create_root_bus()
PCI: Introduce pci_scan_host_bridge() and pci_host_info
PCI: Associate .get_msi_ctrl() with pci_host_bridge
PCI: Add of_scan_bus() to pci_host_info
x86/PCI: Use pci_scan_host_bridge() instead of pci_create_root_bus()
ia64/PCI: Remove the redundant bus variable
ia64/PCI: Use pci_scan_host_bridge() to refactor pci_acpi_scan_root()
arm/PCI: Introduce pci_get_domain_nr()
arm/PCI: Use pci_scan_host_bridge() instead of pci_scan_root_bus()
powerpc/PCI: Use pci_scan_host_bridge() to scan PCI bus
arch/alpha/include/asm/pci.h | 2 +-
arch/alpha/kernel/pci.c | 1 +
arch/alpha/kernel/sys_nautilus.c | 3 +-
arch/arm/include/asm/mach/pci.h | 10 ++
arch/arm/kernel/bios32.c | 60 +++++++----
arch/cris/include/asm/pci.h | 2 +-
arch/frv/mb93090-mb00/pci-vdk.c | 8 +-
arch/ia64/include/asm/pci.h | 2 +-
arch/ia64/pci/pci.c | 85 +++++++--------
arch/ia64/sn/kernel/io_init.c | 2 +
arch/m68k/coldfire/pci.c | 3 +-
arch/microblaze/pci/pci-common.c | 2 +-
arch/mips/include/asm/pci.h | 2 +-
arch/mips/pci/pci.c | 1 +
arch/mn10300/include/asm/pci.h | 2 +-
arch/mn10300/unit-asb2305/pci.c | 7 +-
arch/powerpc/kernel/pci-common.c | 81 ++++++++------
arch/s390/pci/pci.c | 2 +-
arch/sh/drivers/pci/pci.c | 1 +
arch/sh/include/asm/pci.h | 2 +-
arch/sparc/include/asm/pci_32.h | 2 +-
arch/sparc/include/asm/pci_64.h | 2 +-
arch/sparc/kernel/leon_pci.c | 1 +
arch/sparc/kernel/pcic.c | 5 +-
arch/tile/kernel/pci.c | 3 +-
arch/tile/kernel/pci_gx.c | 3 +-
arch/unicore32/kernel/pci.c | 4 +-
arch/x86/include/asm/pci.h | 2 +-
arch/x86/pci/acpi.c | 65 +++++++-----
arch/x86/pci/amd_bus.c | 2 +-
arch/x86/pci/common.c | 27 ++++--
arch/xtensa/include/asm/pci.h | 2 +-
arch/xtensa/kernel/pci.c | 1 +
drivers/parisc/dino.c | 4 +-
drivers/pci/host-bridge.c | 104 ++++++++++++++++++
drivers/pci/host/pci-xgene.c | 2 +-
drivers/pci/hotplug/ibmphp_core.c | 6 +-
drivers/pci/probe.c | 214 +++++++++++++++++++------------------
drivers/pci/xen-pcifront.c | 6 +-
include/linux/pci.h | 41 ++++++-
include/uapi/linux/pci.h | 3 +
41 files changed, 497 insertions(+), 280 deletions(-)
@@ -416,10 +417,12 @@ struct pci_host_bridge {structpci_host_info{u8res_type;void*arg;+structdevice_node*node;structlist_head*resources;/*just for build, will clean up later */structmsi_controller*(*get_msi_ctrl)(structpci_dev*pdev);int(*init_res)(structpci_host_bridge*host,structpci_host_info*info);+void(*of_scan_bus)(structpci_host_bridge*host);};staticinlinevoidinit_pci_host_info(structpci_host_info*info)
@@ -58,6 +58,7 @@ struct pci_host_bridge *pci_create_host_bridge(host->dev.parent=parent;INIT_LIST_HEAD(&host->windows);host->dev.release=pci_release_host_bridge_dev;+host->get_msi_ctrl=info->get_msi_ctrl;/* this is hack, just for build, will be removed later*/b=kzalloc(sizeof(*b),GFP_KERNEL);
@@ -416,6 +417,7 @@ struct pci_host_info {u8res_type;void*arg;structlist_head*resources;/*just for build, will clean up later */+structmsi_controller*(*get_msi_ctrl)(structpci_dev*pdev);int(*init_res)(structpci_host_bridge*host,structpci_host_info*info);};
From: Yijing Wang <hidden> Date: 2014-11-17 09:41:03
Now we can remove the deprecated pci_scan_bus_parented(),
and use pci_scan_root_bus() instead.
Signed-off-by: Yijing Wang <redacted>
---
drivers/pci/probe.c | 19 -------------------
drivers/pci/xen-pcifront.c | 6 +++---
include/linux/pci.h | 2 --
3 files changed, 3 insertions(+), 24 deletions(-)
@@ -488,7 +488,7 @@ static int pcifront_scan_root(struct pcifront_device *pdev,list_add(&bus_entry->list,&pdev->root_buses);-/* pci_scan_bus_parented skips devices which do not have a have+/* pci_scan_root_bus skips devices which do not have a have*devfn==0.Thepcifront_scan_busenumeratesalldevfn.*/err=pcifront_scan_bus(pdev,domain,bus,b);
@@ -115,23 +115,10 @@ struct pci_ops pci_root_ops = {.write=pci_write,};-/* Called by ACPI when it finds a new root bus. */--staticstructpci_controller*alloc_pci_controller(intseg)-{-structpci_controller*controller;--controller=kzalloc(sizeof(*controller),GFP_KERNEL);-if(!controller)-returnNULL;--controller->segment=seg;-returncontroller;-}-structpci_root_info{structacpi_device*bridge;-structpci_controller*controller;+structpci_controllercontroller;+structacpi_pci_root*root;structlist_headresources;structresource*res;resource_size_t*res_offset;
@@ -1578,54 +1578,69 @@ struct device_node *pcibios_get_phb_of_node(struct pci_bus *bus)returnof_node_get(hose->dn);}-/**-*pci_scan_phb-Givenapci_controller,setupandscanthePCIbus-*@hose:PointertothePCIhostcontrollerinstancestructure-*/-voidpcibios_scan_phb(structpci_controller*hose)+staticintpcibios_host_init_res(structpci_host_bridge*host,+structpci_host_info*info){-LIST_HEAD(resources);-structpci_bus*bus;-structdevice_node*node=hose->dn;-intmode;--pr_debug("PCI: Scanning PHB %s\n",of_node_full_name(node));+structpci_controller*hose=info->arg;/* Get some IO space for the new PHB */pcibios_setup_phb_io_space(hose);-/* Wire up PHB bus resources */-pcibios_setup_phb_resources(hose,&resources);+pcibios_setup_phb_resources(hose,&host->windows);hose->busn.start=hose->first_busno;hose->busn.end=hose->last_busno;hose->busn.flags=IORESOURCE_BUS;-pci_add_resource(&resources,&hose->busn);+pci_add_resource(&host->windows,&hose->busn);+return0;+}-/* Create an empty bus for the toplevel */-bus=pci_create_root_bus(hose->parent,hose->first_busno,-hose->ops,hose,&resources);-if(bus==NULL){-pr_err("Failed to create bus for PCI domain %04x\n",-hose->global_number);-pci_free_resource_list(&resources);-return;-}-hose->bus=bus;+staticvoidpcibios_of_scan_bus(structpci_host_bridge*host)+{+intmode;+intmax;/* Get probe mode and perform scan */mode=PCI_PROBE_NORMAL;-if(node&&ppc_md.pci_probe_mode)-mode=ppc_md.pci_probe_mode(bus);+if(host->node&&ppc_md.pci_probe_mode)+mode=ppc_md.pci_probe_mode(host->bus);pr_debug(" probe mode: %d\n",mode);if(mode==PCI_PROBE_DEVTREE)-of_scan_bus(node,bus);-+of_scan_bus(host->node,host->bus);+if(mode==PCI_PROBE_NORMAL){-pci_bus_update_busn_res_end(bus,255);-hose->last_busno=pci_scan_child_bus(bus);-pci_bus_update_busn_res_end(bus,hose->last_busno);+pci_bus_update_busn_res_end(host->bus,255);+max=pci_scan_child_bus(host->bus);+pci_bus_update_busn_res_end(host->bus,max);+}+}+/**+*pci_scan_phb-Givenapci_controller,setupandscanthePCIbus+*@hose:PointertothePCIhostcontrollerinstancestructure+*/+voidpcibios_scan_phb(structpci_controller*hose)+{+structpci_host_bridge*host;+structpci_host_infoinfo;++pr_debug("PCI: Scanning PHB %s\n",of_node_full_name(hose->dn));++init_pci_host_info(&info);+info.arg=hose;+info.node=hose->dn;+info.init_res=pcibios_host_init_res;+info.of_scan_bus=pcibios_of_scan_bus;+/* Create an empty bus for the toplevel */+host=pci_scan_host_bridge(hose->parent,+PCI_DOMBUS(hose->global_number,hose->first_busno),+hose->ops,&info);+if(host==NULL){+pr_err("Failed to create bus for PCI domain %04x\n",+hose->global_number);+return;}+hose->bus=host->bus;+hose->last_busno=host->bus->busn_res.end;/* Platform gets a chance to do some global fixups before*weproceedtoresourceallocation
@@ -326,7 +326,8 @@ int __init pcibios_init(void)/* Configure the max_read_size and max_payload_size values. */fixup_read_and_payload_sizes();-+if(bus)+pci_bus_add_devices(bus);/* Record the I/O resources in the PCI controller structure. */for(i=0;i<TILE_NUM_PCIE;i++){/*
@@ -903,7 +903,8 @@ int __init pcibios_init(void)*associatedwiththedevicesreadinabove.*/pci_assign_unassigned_resources();-+if(bus)+pci_bus_add_devices(bus);/* Record the I/O resources in the PCI controller structure. */for(i=0;i<num_rc_controllers;i++){structpci_controller*controller=&pci_controllers[i];
From: Yijing Wang <hidden> Date: 2014-11-17 09:43:41
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 <redacted>
---
drivers/pci/host-bridge.c | 99 +++++++++++++++++++++++++++++++++
drivers/pci/probe.c | 134 ++++++++++++++++----------------------------
include/linux/pci.h | 11 +++-
3 files changed, 158 insertions(+), 86 deletions(-)
@@ -8,6 +8,105 @@#include"pci.h"+LIST_HEAD(pci_host_bridge_list);+DECLARE_RWSEM(pci_host_bridge_sem);++staticstructresourcebusn_resource={+.name="PCI busn",+.start=0,+.end=255,+.flags=IORESOURCE_BUS,+};++staticvoidpci_release_host_bridge_dev(structdevice*dev)+{+structpci_host_bridge*bridge=to_pci_host_bridge(dev);++if(bridge->release_fn)+bridge->release_fn(bridge);+pci_free_resource_list(&bridge->windows);+kfree(bridge);+}++structpci_host_bridge*pci_create_host_bridge(+structdevice*parent,u32db,+structpci_ops*ops,void*sysdata,+structlist_head*resources)+{+interror;+structpci_bus*b;+structpci_host_bridge*host,*h;+structpci_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);+returnNULL;+}+}+up_read(&pci_host_bridge_sem);++host=kzalloc(sizeof(*host),GFP_KERNEL);+if(!host)+returnNULL;++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);+returnNULL;+}++error=device_register(&host->dev);+if(error){+put_device(&host->dev);+returnNULL;+}++down_write(&pci_host_bridge_sem);+list_add_tail(&host->list,&pci_host_bridge_list);+up_write(&pci_host_bridge_sem);+returnhost;+}+EXPORT_SYMBOL(pci_create_host_bridge);++voidpci_free_host_bridge(structpci_host_bridge*host)+{+down_write(&pci_host_bridge_sem);+list_del(&host->list);+up_write(&pci_host_bridge_sem);++device_unregister(&host->dev);+}+staticstructpci_bus*find_pci_root_bus(structpci_bus*bus){while(bus->parent)
@@ -1895,52 +1863,33 @@ void __weak pcibios_remove_bus(struct pci_bus *bus){}-structpci_bus*pci_create_root_bus(structdevice*parent,u32db,-structpci_ops*ops,void*sysdata,structlist_head*resources)+structpci_bus*__pci_create_root_bus(structpci_host_bridge*bridge){interror;-structpci_host_bridge*bridge;structpci_bus*b,*b2;-structpci_host_bridge_window*window,*n;+structpci_host_bridge_window*window;structresource*res;resource_size_toffset;charbus_addr[64];char*fmt;-u8bus=PCI_BUSNUM(db);+structdevice*parent=bridge->dev.parent;b=pci_alloc_bus(NULL);if(!b)returnNULL;-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");gotoerr_out;}-bridge=pci_alloc_host_bridge(b);-if(!bridge)-gotoerr_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);-gotoerr_out;-}--error=device_register(&bridge->dev);-if(error){-put_device(&bridge->dev);-gotoerr_out;-}b->bridge=get_device(&bridge->dev);device_enable_async_suspend(b->bridge);pci_set_bus_of_node(b);
@@ -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);elsepci_bus_add_resource(b,res,0);if(offset){
From: Yijing Wang <hidden> Date: 2014-11-17 09:43:43
Now we could use pci_scan_host_bridge() to refactor
pci_acpi_scan_root().
Signed-off-by: Yijing Wang <redacted>
---
arch/x86/pci/acpi.c | 65 ++++++++++++++++++++++++++++++-------------------
arch/x86/pci/common.c | 23 ++++++++++++-----
2 files changed, 56 insertions(+), 32 deletions(-)
@@ -467,16 +468,40 @@ static void probe_pci_root_info(struct pci_root_info *info,info);}+staticintpci_acpi_init_res(structpci_host_bridge*host,+structpci_host_info*arg)+{+structpci_sysdata*sd=arg->arg;+structpci_root_info*info=container_of(sd,+structpci_root_info,sd);++/* insert busn res at first */+pci_add_resource(&host->windows,&info->root->secondary);+/*+*_CRSwithnoaperturesisnormal,soonlyfallbackto+*defaultsornativebridgeinfoifwe'reignoring_CRS.+*/+if(pci_use_crs)+add_resources(info,&host->windows);+else{+free_pci_root_info_res(info);+x86_pci_root_bus_resources(host->busnum,&host->windows);+}++return0;+}+structpci_bus*pci_acpi_scan_root(structacpi_pci_root*root){structacpi_device*device=root->device;structpci_root_info*info;intdomain=root->segment;intbusnum=root->secondary.start;-LIST_HEAD(resources);structpci_bus*bus;structpci_sysdata*sd;intnode;+structpci_host_infoarg;+structpci_host_bridge*host;if(pci_ignore_seg)domain=0;
@@ -522,33 +551,19 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)}else{probe_pci_root_info(info,device,busnum,domain);-/* insert busn res at first */-pci_add_resource(&resources,&root->secondary);-/*-*_CRSwithnoaperturesisnormal,soonlyfallbackto-*defaultsornativebridgeinfoifwe'reignoring_CRS.-*/-if(pci_use_crs)-add_resources(info,&resources);-else{-free_pci_root_info_res(info);-x86_pci_root_bus_resources(busnum,&resources);+if(!setup_mcfg_map(info,domain,(u8)root->secondary.start,+(u8)root->secondary.end,root->mcfg_addr)){+host=pci_scan_host_bridge(NULL,PCI_DOMBUS(domain,busnum),+&pci_root_ops,&arg);+bus=host->bus;}-if(!setup_mcfg_map(info,domain,(u8)root->secondary.start,-(u8)root->secondary.end,root->mcfg_addr))-bus=pci_create_root_bus(NULL,busnum,&pci_root_ops,-sd,&resources);--if(bus){-pci_scan_child_bus(bus);-pci_set_host_bridge_release(-to_pci_host_bridge(bus->bridge),+if(bus)+pci_set_host_bridge_release(host,release_pci_root_info,info);-}else{-pci_free_resource_list(&resources);+else__release_pci_root_info(info);-}+}/* After the PCI-E bus has been walked and all devices discovered,
From: Yijing Wang <hidden> Date: 2014-11-17 09:43:45
From: Yijing Wang <redacted>
Now pci_host_bridge has been ripped out from pci root
bus creation. Currently pci_scan_root_bus() lacks
scalability, so platform host drivers have no proper
way to configure pci_host_bridge. E.g we should assign
msi_controller to pci_host_bridge, add argument for
pci_scan_root_bus() is not a good idea, it has already
five, so introudce struct pci_host_info to make
pci scan interfaces more scalable. Because almost
all host drivers need to configure host resources,
so we put .init_res() in it first, and add other
hooks when need.
Signed-off-by: Yijing Wang <redacted>
---
drivers/pci/host-bridge.c | 27 +++++++++-------
drivers/pci/probe.c | 73 ++++++++++++++++++++++++++++++++++++++++++--
include/linux/pci.h | 20 ++++++++++++-
3 files changed, 103 insertions(+), 17 deletions(-)
@@ -63,18 +61,23 @@ struct pci_host_bridge *pci_create_host_bridge(/* this is hack, just for build, will be removed later*/b=kzalloc(sizeof(*b),GFP_KERNEL);-b->sysdata=sysdata;+b->sysdata=host->sysdata;pci_bus_assign_domain_nr(b,parent);host->domain=pci_domain_nr(b);+kfree(b);-if(!resources){+if(info->res_type==PCI_HOST_RES_DEFAULT){/* 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);+if(!info->init_res||info->init_res(host,info)){+pr_err("pci host %04x:%02x init resources fail\n",+host->domain,host->busnum);+kfree(host);+returnNULL;+}}dev_set_name(&host->dev,"pci%04x:%02x",host->domain,
From: Yijing Wang <hidden> Date: 2014-11-17 09:43:46
From: Yijing Wang <redacted>
Pci_scan_root_bus(), pci_scan_bus() and pci_scan_bus_parented()
are very similar. But the latter two use the default
io/mem resources. Enhance pci_scan_root_bus() to support
default io/mem resources, then we could use
pci_scan_root_bus() instead of them, and clean them up.
Signed-off-by: Yijing Wang <redacted>
---
drivers/pci/probe.c | 20 ++++++++++++++------
1 files changed, 14 insertions(+), 6 deletions(-)
@@ -49,7 +49,7 @@ struct pci_controller {void*sysdata;};-/* Override the logic in pci_scan_bus for skipping already-configured+/* Override the logic in pci_scan_root_bus for skipping already-configuredbusnumbers.*/#define pcibios_assign_all_busses() 1
@@ -5,7 +5,7 @@#ifdef __KERNEL__#include<linux/mm.h>/* for struct page */-/* Can be used to override the logic in pci_scan_bus for skipping+/* Can be used to override the logic in pci_scan_root_bus for skippingalready-configuredbusnumbers-tobeusedforbuggyBIOSesorarchitectureswithincompletePCIsetupbytheloader*/
@@ -60,7 +60,7 @@ extern void register_pci_controller(struct pci_controller *hose);externintpcibios_map_irq(conststructpci_dev*dev,u8slot,u8pin);-/* Can be used to override the logic in pci_scan_bus for skipping+/* Can be used to override the logic in pci_scan_root_bus for skippingalready-configuredbusnumbers-tobeusedforbuggyBIOSesorarchitectureswithincompletePCIsetupbytheloader*/
@@ -33,7 +33,7 @@ do { \#define __pcidebug(FMT, BUS, DEVFN, WHERE, ...) do {} while (0)#endif-/* Can be used to override the logic in pci_scan_bus for skipping+/* Can be used to override the logic in pci_scan_root_bus for skipping*already-configuredbusnumbers-tobeusedforbuggyBIOSesor*architectureswithincompletePCIsetupbytheloader*/
@@ -3,7 +3,7 @@#ifdef __KERNEL__-/* Can be used to override the logic in pci_scan_bus for skipping+/* Can be used to override the logic in pci_scan_root_bus for skippingalready-configuredbusnumbers-tobeusedforbuggyBIOSesorarchitectureswithincompletePCIsetupbytheloader*/
@@ -5,7 +5,7 @@#include<linux/dma-mapping.h>-/* Can be used to override the logic in pci_scan_bus for skipping+/* Can be used to override the logic in pci_scan_root_bus for skipping*already-configuredbusnumbers-tobeusedforbuggyBIOSes*orarchitectureswithincompletePCIsetupbytheloader.*/
@@ -5,7 +5,7 @@#include<linux/dma-mapping.h>-/* Can be used to override the logic in pci_scan_bus for skipping+/* Can be used to override the logic in pci_scan_root_bus for skipping*already-configuredbusnumbers-tobeusedforbuggyBIOSes*orarchitectureswithincompletePCIsetupbytheloader.*/
@@ -41,7 +41,7 @@ static inline int pci_proc_domain(struct pci_bus *bus)}#endif-/* Can be used to override the logic in pci_scan_bus for skipping+/* Can be used to override the logic in pci_scan_root_bus for skippingalready-configuredbusnumbers-tobeusedforbuggyBIOSesorarchitectureswithincompletePCIsetupbytheloader*/
@@ -13,7 +13,7 @@#ifdef __KERNEL__-/* Can be used to override the logic in pci_scan_bus for skipping+/* Can be used to override the logic in pci_scan_root_bus for skipping*already-configuredbusnumbers-tobeusedforbuggyBIOSes*orarchitectureswithincompletePCIsetupbytheloader*/
@@ -787,7 +787,7 @@ static int __init dino_common_init(struct parisc_device *dev,pcibios_register_hba(&dino_dev->hba);-pci_bios=&dino_bios_ops;/* used by pci_scan_bus() */+pci_bios=&dino_bios_ops;pci_port=&dino_port_ops;/*
From: Yijing Wang <hidden> Date: 2014-11-17 09:43:50
Currently, we use int type for bus number in
pci_create_root_bus() and pci_scan_root_bus().
Because bus number is <= 255, so we could
change the bus number argument type to u32,
and combine PCI domain and bus number, we will
save the domain number in pci_host_bridge in
later patch, and eliminate the arch specific
pci_domain_nr().
Signed-off-by: Yijing Wang <redacted>
---
drivers/pci/probe.c | 11 ++++++-----
include/linux/pci.h | 4 ++--
include/uapi/linux/pci.h | 3 +++
3 files changed, 11 insertions(+), 7 deletions(-)
@@ -2092,8 +2093,8 @@ struct pci_bus *pci_scan_root_bus(struct device *parent, int bus,if(!found){dev_info(&b->dev,"No busn resource found for root bus, will use [bus %02x-ff]\n",-bus);-pci_bus_insert_busn_res(b,bus,255);+PCI_BUSNUM(db));+pci_bus_insert_busn_res(b,PCI_BUSNUM(db),255);}max=pci_scan_child_bus(b);
On Monday 17 November 2014 18:21:35 Yijing Wang wrote:
quoted hunk
- list_for_each_entry(window, resources, list)- if (window->res->flags & IORESOURCE_BUS) {- found = true;- break;- }+ if (!resources) {+ pci_add_resource(&default_res, &ioport_resource);+ pci_add_resource(&default_res, &iomem_resource);+ pci_add_resource(&default_res, &busn_resource);+ } else {
Isn't it almost always wrong to do this? You are adding all of the
I/O ports and memory to the host bridge, which will prevent you from
adding another host bridge, and the iomem_resource normally
includes a lot of addresses that are not accessible by the PCI host.
Arnd
On Monday 17 November 2014 18:21:41 Yijing Wang wrote:
quoted hunk
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 <redacted>
---
drivers/pci/host-bridge.c | 99 +++++++++++++++++++++++++++++++++
drivers/pci/probe.c | 134 ++++++++++++++++----------------------------
include/linux/pci.h | 11 +++-
3 files changed, 158 insertions(+), 86 deletions(-)
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).
quoted hunk
+ host = kzalloc(sizeof(*host), GFP_KERNEL);+ if (!host)+ return NULL;
devm_kzalloc maybe?
quoted hunk
+ 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.
On Monday 17 November 2014 18:21:34 Yijing Wang wrote:
This series is based Linux 3.18-rc1 and Lorenzo Pieralisi's
arm PCI domain cleanup patches, link:
https://patchwork.ozlabs.org/patch/407585/
Current pci scan interfaces like pci_scan_root_bus() and directly
call pci_create_root_bus()/pci_scan_child_bus() lack flexiblity.
Some platform infos like PCI domain and msi_chip have to be
associated to PCI bus by some arch specific function.
We want to make a generic pci_host_bridge, and make it hold
the platform infos or hook. Then we could eliminate the lots
of arch pci_domain_nr, also we could associate some platform
ops something like pci_get_msi_chip(struct pci_dev *dev)
with pci_host_bridge to avoid introduce arch weak functions.
This RFC version not for all platforms, just applied the new
scan interface in x86/arm/powerpc/ia64, I will refresh other
platforms after the core pci scan interfaces are ok.
I think overall this is a good direction to take, in particular
moving more things into struct pci_host_bridge so we can
slim down the architecture specific code.
I don't particularly like the way you use the 'pci_host_info'
to pass callback pointers and some of the generic information.
This duplicates some of the issues we are currently trying
to untangle in the arm32 code to make drivers easier to share
between architectures.
As a general approach, I'd rather see generic helper functions
being exported by the PCI core that a driver may or may not
call.
The way you split the interface between things that happen
before scanning the buses (pci_create_host_bridge) and
the actual scanning (__pci_create_root_bus, pci_scan_child_bus)
seems very helpful and I think we can expand that concept further:
- The normal pci_create_host_bridge() function can contain
all of the DT scanning functions (finding bus/mem/io resources,
finding the msi-parent), while drivers that don't depend on DT
for this information can call the same function and fill the
same things after they have the pci_host_bridge pointer.
- If a driver needs to set up mapping windows, it can do that after
calling pci_create_host_bridge(). E.g. all the dw_pcie glue drivers
can call a dw_pcie_setup_windows() function that takes the resources
out of the pci_host_bridge pointer before the bus is scanned.
- The ACPI code can have a completely different way of creating
a struct pci_host_bridge, which is also passed into the same
bus scanning functions, but doesn't have to come from
pci_create_host_bridge.
- The PowerPC of_scan_bus function can take the same pci_host_bridge
pointer that comes from pci_create_host_bridge(), but we'd call
either pci_create_root_bus or of_scan_bus instead of calling
of_scan_bus through an indirect pointer from pci_create_root_bus.
Arnd
@@ -416,6 +417,7 @@ struct pci_host_info {=09u8res_type;=09void*arg;=09structlist_head*resources;/*just for build, will clean up later */+=09structmsi_controller*(*get_msi_ctrl)(structpci_dev*pdev);=09int(*init_res)(structpci_host_bridge*host,=20=09=09=09structpci_host_info*info);};
Where would you use the get_msi_ctrl pointer then ? Wasn't it better
to wait for this patchset to take shape before adding more churn to
the ARM (and other archs) pci_sys_data structure and consequently add
another pcibios call to achieve what the get_msi_ctrl pointer is there to
achieve (ie making msi_controller retrieval arch independent ?)
I just do not see what the pci_sys_data intermediate step buys you if
we consider the approach taken in this patch as the proper solution.
Thanks,
Lorenzo
From: Yijing Wang <hidden> Date: 2014-11-18 07:45:12
On 2014/11/17 18:08, Arnd Bergmann wrote:
On Monday 17 November 2014 18:21:35 Yijing Wang wrote:
quoted
- list_for_each_entry(window, resources, list)- if (window->res->flags & IORESOURCE_BUS) {- found = true;- break;- }+ if (!resources) {+ pci_add_resource(&default_res, &ioport_resource);+ pci_add_resource(&default_res, &iomem_resource);+ pci_add_resource(&default_res, &busn_resource);+ } else {
Isn't it almost always wrong to do this? You are adding all of the
I/O ports and memory to the host bridge, which will prevent you from
adding another host bridge, and the iomem_resource normally
includes a lot of addresses that are not accessible by the PCI host.
Hi Arnd, pci host bridge windows are the ranges allow child devices to setup
from. Add all of IO/MEM here just a limit to child devices, no request for these
resources, so it won't hurt another host bridge. Some platforms have no dts or ACPI
report host bridge resources, in this case, we directly assign ioport/iomem_resources
as the root resources of PCI devices.
Thanks!
Yijing.
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.
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.
quoted
+ 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.
quoted
+ 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.
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.
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?
quoted
quoted
+ 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 15:44:23 Yijing Wang wrote:
On 2014/11/17 18:08, Arnd Bergmann wrote:
quoted
On Monday 17 November 2014 18:21:35 Yijing Wang wrote:
quoted
- list_for_each_entry(window, resources, list)- if (window->res->flags & IORESOURCE_BUS) {- found = true;- break;- }+ if (!resources) {+ pci_add_resource(&default_res, &ioport_resource);+ pci_add_resource(&default_res, &iomem_resource);+ pci_add_resource(&default_res, &busn_resource);+ } else {
Isn't it almost always wrong to do this? You are adding all of the
I/O ports and memory to the host bridge, which will prevent you from
adding another host bridge, and the iomem_resource normally
includes a lot of addresses that are not accessible by the PCI host.
Hi Arnd, pci host bridge windows are the ranges allow child devices to setup
from. Add all of IO/MEM here just a limit to child devices, no request for these
resources, so it won't hurt another host bridge. Some platforms have no dts or ACPI
report host bridge resources, in this case, we directly assign ioport/iomem_resources
as the root resources of PCI devices.
But it would be wrong to allow hosts to allocate a device BAR that is not
visible through the host bridge. I think we need to keep these separate
from the general case: if you call any of the modern interfaces you have
to provide the resources and a device. I notice that there is only one
caller of pci_scan_bus_parented(), we should probably change that over to
pci_scan_root_bus() or your new interface and remove the old one, but
keep pci_scan_bus() as the only entry point for all of the legacy users
that do not know about the resources.
Arnd
From: Yijing Wang <hidden> Date: 2014-11-18 11:18:11
On 2014/11/17 22:13, Arnd Bergmann wrote:
On Monday 17 November 2014 18:21:34 Yijing Wang wrote:
quoted
This series is based Linux 3.18-rc1 and Lorenzo Pieralisi's
arm PCI domain cleanup patches, link:
https://patchwork.ozlabs.org/patch/407585/
Current pci scan interfaces like pci_scan_root_bus() and directly
call pci_create_root_bus()/pci_scan_child_bus() lack flexiblity.
Some platform infos like PCI domain and msi_chip have to be
associated to PCI bus by some arch specific function.
We want to make a generic pci_host_bridge, and make it hold
the platform infos or hook. Then we could eliminate the lots
of arch pci_domain_nr, also we could associate some platform
ops something like pci_get_msi_chip(struct pci_dev *dev)
with pci_host_bridge to avoid introduce arch weak functions.
This RFC version not for all platforms, just applied the new
scan interface in x86/arm/powerpc/ia64, I will refresh other
platforms after the core pci scan interfaces are ok.
I think overall this is a good direction to take, in particular
moving more things into struct pci_host_bridge so we can
slim down the architecture specific code.
Hi Arnd, thanks very much for your review and comments!
I don't particularly like the way you use the 'pci_host_info'
to pass callback pointers and some of the generic information.
This duplicates some of the issues we are currently trying
to untangle in the arm32 code to make drivers easier to share
between architectures.
What arm32 code you are trying to untangle for example ?
Introduce pci_host_info here because I want to make the PCI scan interfaces
simple to host drviers, host drivers only need to call one scan
interface(pci_scan_host_bridge), but from your comments,
The combination pci_create_host_bridge() + pci_scan_xx()
seems to be more popular.
As a general approach, I'd rather see generic helper functions
being exported by the PCI core that a driver may or may not
call.
The way you split the interface between things that happen
before scanning the buses (pci_create_host_bridge) and
the actual scanning (__pci_create_root_bus, pci_scan_child_bus)
seems very helpful and I think we can expand that concept further:
- The normal pci_create_host_bridge() function can contain
all of the DT scanning functions (finding bus/mem/io resources,
finding the msi-parent), while drivers that don't depend on DT
for this information can call the same function and fill the
same things after they have the pci_host_bridge pointer.
- If a driver needs to set up mapping windows, it can do that after
calling pci_create_host_bridge(). E.g. all the dw_pcie glue drivers
can call a dw_pcie_setup_windows() function that takes the resources
out of the pci_host_bridge pointer before the bus is scanned.
- The ACPI code can have a completely different way of creating
a struct pci_host_bridge, which is also passed into the same
bus scanning functions, but doesn't have to come from
pci_create_host_bridge.
I hope platforms with ACPI or DT could both use pci_create_host_bridge().
Why we need to use two different ways to process it ?
- The PowerPC of_scan_bus function can take the same pci_host_bridge
pointer that comes from pci_create_host_bridge(), but we'd call
either pci_create_root_bus or of_scan_bus instead of calling
of_scan_bus through an indirect pointer from pci_create_root_bus.
Arnd
.
On Tuesday 18 November 2014 19:17:32 Yijing Wang wrote:
On 2014/11/17 22:13, Arnd Bergmann wrote:
quoted
On Monday 17 November 2014 18:21:34 Yijing Wang wrote:
quoted
This series is based Linux 3.18-rc1 and Lorenzo Pieralisi's
arm PCI domain cleanup patches, link:
https://patchwork.ozlabs.org/patch/407585/
Current pci scan interfaces like pci_scan_root_bus() and directly
call pci_create_root_bus()/pci_scan_child_bus() lack flexiblity.
Some platform infos like PCI domain and msi_chip have to be
associated to PCI bus by some arch specific function.
We want to make a generic pci_host_bridge, and make it hold
the platform infos or hook. Then we could eliminate the lots
of arch pci_domain_nr, also we could associate some platform
ops something like pci_get_msi_chip(struct pci_dev *dev)
with pci_host_bridge to avoid introduce arch weak functions.
This RFC version not for all platforms, just applied the new
scan interface in x86/arm/powerpc/ia64, I will refresh other
platforms after the core pci scan interfaces are ok.
I think overall this is a good direction to take, in particular
moving more things into struct pci_host_bridge so we can
slim down the architecture specific code.
Hi Arnd, thanks very much for your review and comments!
quoted
I don't particularly like the way you use the 'pci_host_info'
to pass callback pointers and some of the generic information.
This duplicates some of the issues we are currently trying
to untangle in the arm32 code to make drivers easier to share
between architectures.
What arm32 code you are trying to untangle for example ?
We have a few problems that currently prevent us from using shared
drivers across arm32 and arm64:
- arm32 has an architecture-defined pci_sys_data structure, but
we really want to have one that is defined by the host bridge driver
and that is architecture independent. Some core functions depend
on this structure at the moment, which Lorenzo is trying to
undo
- The pci_common_init interface on arm32 doesn't work well on
loadable drivers, it does not return an error, and it is built
around the assumption that you probe all pci host bridges at
the same time, while the standard Linux driver model assumes
that you probe one at a time.
- The way we pass a temporary structure (hw_pci) with function pointers
into the architecture code makes it relatively hard to follow
how the initialization sequence works.
Introduce pci_host_info here because I want to make the PCI scan interfaces
simple to host drviers, host drivers only need to call one scan
interface(pci_scan_host_bridge), but from your comments,
The combination pci_create_host_bridge() + pci_scan_xx()
seems to be more popular.
Yes, I think a simpler interface structure would be better than trying
to minimize the amount of code needed in drivers at the expense of
interface complexity.
quoted
As a general approach, I'd rather see generic helper functions
being exported by the PCI core that a driver may or may not
call.
The way you split the interface between things that happen
before scanning the buses (pci_create_host_bridge) and
the actual scanning (__pci_create_root_bus, pci_scan_child_bus)
seems very helpful and I think we can expand that concept further:
- The normal pci_create_host_bridge() function can contain
all of the DT scanning functions (finding bus/mem/io resources,
finding the msi-parent), while drivers that don't depend on DT
for this information can call the same function and fill the
same things after they have the pci_host_bridge pointer.
- If a driver needs to set up mapping windows, it can do that after
calling pci_create_host_bridge(). E.g. all the dw_pcie glue drivers
can call a dw_pcie_setup_windows() function that takes the resources
out of the pci_host_bridge pointer before the bus is scanned.
- The ACPI code can have a completely different way of creating
a struct pci_host_bridge, which is also passed into the same
bus scanning functions, but doesn't have to come from
pci_create_host_bridge.
I hope platforms with ACPI or DT could both use pci_create_host_bridge().
Why we need to use two different ways to process it ?
These are completely different use cases:
a) For DT, we want loadable device drivers that start by probing a host
bridge device which was added through the DT platform code. The
driver is self-contained, and eventually we want to be able to unload
it. We have lots of different per-soc drivers that require different
quirks
b) For ACPI, the interface is defined in the ACPI spec across architectures
and SoCs, we don't have host bridge drivers and the code that initializes
the PCI is required early during boot and called from architecture
code. There is no parent device, as ACPI sees PCI as a fundamental building
block by itself, and there are no drivers because the firmware does
the initial hardware setup, so we only have to access the config space.
Arnd
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.
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.
quoted
quoted
quoted
+ 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
.
From: Lorenzo Pieralisi <hidden> Date: 2014-11-18 11:45:45
On Tue, Nov 18, 2014 at 11:30:11AM +0000, Arnd Bergmann wrote:
On Tuesday 18 November 2014 19:17:32 Yijing Wang wrote:
quoted
On 2014/11/17 22:13, Arnd Bergmann wrote:
quoted
On Monday 17 November 2014 18:21:34 Yijing Wang wrote:
quoted
This series is based Linux 3.18-rc1 and Lorenzo Pieralisi's
arm PCI domain cleanup patches, link:
https://patchwork.ozlabs.org/patch/407585/
Current pci scan interfaces like pci_scan_root_bus() and directly
call pci_create_root_bus()/pci_scan_child_bus() lack flexiblity.
Some platform infos like PCI domain and msi_chip have to be
associated to PCI bus by some arch specific function.
We want to make a generic pci_host_bridge, and make it hold
the platform infos or hook. Then we could eliminate the lots
of arch pci_domain_nr, also we could associate some platform
ops something like pci_get_msi_chip(struct pci_dev *dev)
with pci_host_bridge to avoid introduce arch weak functions.
This RFC version not for all platforms, just applied the new
scan interface in x86/arm/powerpc/ia64, I will refresh other
platforms after the core pci scan interfaces are ok.
I think overall this is a good direction to take, in particular
moving more things into struct pci_host_bridge so we can
slim down the architecture specific code.
Hi Arnd, thanks very much for your review and comments!
quoted
I don't particularly like the way you use the 'pci_host_info'
to pass callback pointers and some of the generic information.
This duplicates some of the issues we are currently trying
to untangle in the arm32 code to make drivers easier to share
between architectures.
What arm32 code you are trying to untangle for example ?
We have a few problems that currently prevent us from using shared
drivers across arm32 and arm64:
- arm32 has an architecture-defined pci_sys_data structure, but
we really want to have one that is defined by the host bridge driver
and that is architecture independent. Some core functions depend
on this structure at the moment, which Lorenzo is trying to
undo
Yes, and on this specific point I would like to understand why we
are adding yet more pci_sys_data data in the last series that is
already in -next:
https://lkml.org/lkml/2014/10/27/85
What does this buy us ? The cover letter says already that there *is*
a better solution, why do not we work on that instead of adding more churn
to arch specific code ?
Thanks,
Lorenzo
From: Yijing Wang <hidden> Date: 2014-11-18 11:46:36
On 2014/11/18 17:36, Arnd Bergmann wrote:
On Tuesday 18 November 2014 15:44:23 Yijing Wang wrote:
quoted
On 2014/11/17 18:08, Arnd Bergmann wrote:
quoted
On Monday 17 November 2014 18:21:35 Yijing Wang wrote:
quoted
- list_for_each_entry(window, resources, list)- if (window->res->flags & IORESOURCE_BUS) {- found = true;- break;- }+ if (!resources) {+ pci_add_resource(&default_res, &ioport_resource);+ pci_add_resource(&default_res, &iomem_resource);+ pci_add_resource(&default_res, &busn_resource);+ } else {
Isn't it almost always wrong to do this? You are adding all of the
I/O ports and memory to the host bridge, which will prevent you from
adding another host bridge, and the iomem_resource normally
includes a lot of addresses that are not accessible by the PCI host.
Hi Arnd, pci host bridge windows are the ranges allow child devices to setup
from. Add all of IO/MEM here just a limit to child devices, no request for these
resources, so it won't hurt another host bridge. Some platforms have no dts or ACPI
report host bridge resources, in this case, we directly assign ioport/iomem_resources
as the root resources of PCI devices.
But it would be wrong to allow hosts to allocate a device BAR that is not
visible through the host bridge. I think we need to keep these separate
from the general case: if you call any of the modern interfaces you have
to provide the resources and a device. I notice that there is only one
caller of pci_scan_bus_parented(), we should probably change that over to
pci_scan_root_bus() or your new interface and remove the old one, but
keep pci_scan_bus() as the only entry point for all of the legacy users
that do not know about the resources.
Ok, I will move this out of the generic interface.
Thanks!
Yijing.
From: Yijing Wang <hidden> Date: 2014-11-18 12:14:57
On 2014/11/18 19:45, Lorenzo Pieralisi wrote:
On Tue, Nov 18, 2014 at 11:30:11AM +0000, Arnd Bergmann wrote:
quoted
On Tuesday 18 November 2014 19:17:32 Yijing Wang wrote:
quoted
On 2014/11/17 22:13, Arnd Bergmann wrote:
quoted
On Monday 17 November 2014 18:21:34 Yijing Wang wrote:
quoted
This series is based Linux 3.18-rc1 and Lorenzo Pieralisi's
arm PCI domain cleanup patches, link:
https://patchwork.ozlabs.org/patch/407585/
Current pci scan interfaces like pci_scan_root_bus() and directly
call pci_create_root_bus()/pci_scan_child_bus() lack flexiblity.
Some platform infos like PCI domain and msi_chip have to be
associated to PCI bus by some arch specific function.
We want to make a generic pci_host_bridge, and make it hold
the platform infos or hook. Then we could eliminate the lots
of arch pci_domain_nr, also we could associate some platform
ops something like pci_get_msi_chip(struct pci_dev *dev)
with pci_host_bridge to avoid introduce arch weak functions.
This RFC version not for all platforms, just applied the new
scan interface in x86/arm/powerpc/ia64, I will refresh other
platforms after the core pci scan interfaces are ok.
I think overall this is a good direction to take, in particular
moving more things into struct pci_host_bridge so we can
slim down the architecture specific code.
Hi Arnd, thanks very much for your review and comments!
quoted
I don't particularly like the way you use the 'pci_host_info'
to pass callback pointers and some of the generic information.
This duplicates some of the issues we are currently trying
to untangle in the arm32 code to make drivers easier to share
between architectures.
What arm32 code you are trying to untangle for example ?
We have a few problems that currently prevent us from using shared
drivers across arm32 and arm64:
- arm32 has an architecture-defined pci_sys_data structure, but
we really want to have one that is defined by the host bridge driver
and that is architecture independent. Some core functions depend
on this structure at the moment, which Lorenzo is trying to
undo
Yes, and on this specific point I would like to understand why we
are adding yet more pci_sys_data data in the last series that is
already in -next:
https://lkml.org/lkml/2014/10/27/85
What does this buy us ? The cover letter says already that there *is*
a better solution, why do not we work on that instead of adding more churn
to arch specific code ?
In my plan, first save msi_chip in pci_sys_data, so we could remove the lots duplicate
pcibios_add_bus(), second, make a generic pci_host_bridge, and move the msi_chip in that,
so we could eliminate all MSI arch weak functions. And in arm I think it's no need to
associate msi_chip with PCI bus, because all pci devices under the same pci host bridge
share the same msi_chip.
From: Yijing Wang <hidden> Date: 2014-11-18 12:18:31
On 2014/11/18 19:30, Arnd Bergmann wrote:
On Tuesday 18 November 2014 19:17:32 Yijing Wang wrote:
quoted
On 2014/11/17 22:13, Arnd Bergmann wrote:
quoted
On Monday 17 November 2014 18:21:34 Yijing Wang wrote:
quoted
This series is based Linux 3.18-rc1 and Lorenzo Pieralisi's
arm PCI domain cleanup patches, link:
https://patchwork.ozlabs.org/patch/407585/
Current pci scan interfaces like pci_scan_root_bus() and directly
call pci_create_root_bus()/pci_scan_child_bus() lack flexiblity.
Some platform infos like PCI domain and msi_chip have to be
associated to PCI bus by some arch specific function.
We want to make a generic pci_host_bridge, and make it hold
the platform infos or hook. Then we could eliminate the lots
of arch pci_domain_nr, also we could associate some platform
ops something like pci_get_msi_chip(struct pci_dev *dev)
with pci_host_bridge to avoid introduce arch weak functions.
This RFC version not for all platforms, just applied the new
scan interface in x86/arm/powerpc/ia64, I will refresh other
platforms after the core pci scan interfaces are ok.
I think overall this is a good direction to take, in particular
moving more things into struct pci_host_bridge so we can
slim down the architecture specific code.
Hi Arnd, thanks very much for your review and comments!
quoted
I don't particularly like the way you use the 'pci_host_info'
to pass callback pointers and some of the generic information.
This duplicates some of the issues we are currently trying
to untangle in the arm32 code to make drivers easier to share
between architectures.
What arm32 code you are trying to untangle for example ?
We have a few problems that currently prevent us from using shared
drivers across arm32 and arm64:
- arm32 has an architecture-defined pci_sys_data structure, but
we really want to have one that is defined by the host bridge driver
and that is architecture independent. Some core functions depend
on this structure at the moment, which Lorenzo is trying to
undo
- The pci_common_init interface on arm32 doesn't work well on
loadable drivers, it does not return an error, and it is built
around the assumption that you probe all pci host bridges at
the same time, while the standard Linux driver model assumes
that you probe one at a time.
- The way we pass a temporary structure (hw_pci) with function pointers
into the architecture code makes it relatively hard to follow
how the initialization sequence works.
quoted
Introduce pci_host_info here because I want to make the PCI scan interfaces
simple to host drviers, host drivers only need to call one scan
interface(pci_scan_host_bridge), but from your comments,
The combination pci_create_host_bridge() + pci_scan_xx()
seems to be more popular.
Yes, I think a simpler interface structure would be better than trying
to minimize the amount of code needed in drivers at the expense of
interface complexity.
quoted
quoted
As a general approach, I'd rather see generic helper functions
being exported by the PCI core that a driver may or may not
call.
The way you split the interface between things that happen
before scanning the buses (pci_create_host_bridge) and
the actual scanning (__pci_create_root_bus, pci_scan_child_bus)
seems very helpful and I think we can expand that concept further:
- The normal pci_create_host_bridge() function can contain
all of the DT scanning functions (finding bus/mem/io resources,
finding the msi-parent), while drivers that don't depend on DT
for this information can call the same function and fill the
same things after they have the pci_host_bridge pointer.
- If a driver needs to set up mapping windows, it can do that after
calling pci_create_host_bridge(). E.g. all the dw_pcie glue drivers
can call a dw_pcie_setup_windows() function that takes the resources
out of the pci_host_bridge pointer before the bus is scanned.
- The ACPI code can have a completely different way of creating
a struct pci_host_bridge, which is also passed into the same
bus scanning functions, but doesn't have to come from
pci_create_host_bridge.
Thanks for your explanation, I will consider these problems when I refactor the
core generic interfaces.
quoted
I hope platforms with ACPI or DT could both use pci_create_host_bridge().
Why we need to use two different ways to process it ?
These are completely different use cases:
a) For DT, we want loadable device drivers that start by probing a host
bridge device which was added through the DT platform code. The
driver is self-contained, and eventually we want to be able to unload
it. We have lots of different per-soc drivers that require different
quirks
b) For ACPI, the interface is defined in the ACPI spec across architectures
and SoCs, we don't have host bridge drivers and the code that initializes
the PCI is required early during boot and called from architecture
code. There is no parent device, as ACPI sees PCI as a fundamental building
block by itself, and there are no drivers because the firmware does
the initial hardware setup, so we only have to access the config space.
Hmmm, I'm a little confused, so why you think ACPI host driver should not use
pci_create_host_bridge(), because ACPI PCI driver has no parent device ?
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.
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
On Tuesday 18 November 2014 20:17:57 Yijing Wang wrote:
quoted
quoted
I hope platforms with ACPI or DT could both use pci_create_host_bridge().
Why we need to use two different ways to process it ?
These are completely different use cases:
a) For DT, we want loadable device drivers that start by probing a host
bridge device which was added through the DT platform code. The
driver is self-contained, and eventually we want to be able to unload
it. We have lots of different per-soc drivers that require different
quirks
b) For ACPI, the interface is defined in the ACPI spec across architectures
and SoCs, we don't have host bridge drivers and the code that initializes
the PCI is required early during boot and called from architecture
code. There is no parent device, as ACPI sees PCI as a fundamental building
block by itself, and there are no drivers because the firmware does
the initial hardware setup, so we only have to access the config space.
Hmmm, I'm a little confused, so why you think ACPI host driver should not use
pci_create_host_bridge(), because ACPI PCI driver has no parent device ?
It's one of the difference. Having a parent device can certainly make your
life simpler, since you have devm_kzalloc(), dev_info(), etc. Coming from
the other end, I think ACPI needs PCI to be available during early boot,
at a time where we might not want pci_create_host_bridge() to do the
right thing.
Arnd
From: Yijing Wang <hidden> Date: 2014-11-18 12:42:03
quoted
quoted
quoted
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.
On Tue, Nov 18, 2014 at 11:46:06AM +0000, Yijing Wang wrote:
On 2014/11/18 17:36, Arnd Bergmann wrote:
quoted
On Tuesday 18 November 2014 15:44:23 Yijing Wang wrote:
quoted
On 2014/11/17 18:08, Arnd Bergmann wrote:
quoted
On Monday 17 November 2014 18:21:35 Yijing Wang wrote:
quoted
- list_for_each_entry(window, resources, list)- if (window->res->flags & IORESOURCE_BUS) {- found =3D true;- break;- }+ if (!resources) {+ pci_add_resource(&default_res, &ioport_resource);+ pci_add_resource(&default_res, &iomem_resource);+ pci_add_resource(&default_res, &busn_resource);+ } else {
Isn't it almost always wrong to do this? You are adding all of the
I/O ports and memory to the host bridge, which will prevent you from
adding another host bridge, and the iomem_resource normally
includes a lot of addresses that are not accessible by the PCI host.
Hi Arnd, pci host bridge windows are the ranges allow child devices to=
setup
quoted
quoted
from. Add all of IO/MEM here just a limit to child devices, no request=
for these
quoted
quoted
resources, so it won't hurt another host bridge. Some platforms have n=
o dts or ACPI
quoted
quoted
report host bridge resources, in this case, we directly assign ioport/=
iomem_resources
quoted
quoted
as the root resources of PCI devices.
=20
But it would be wrong to allow hosts to allocate a device BAR that is n=
ot
quoted
visible through the host bridge. I think we need to keep these separate
from the general case: if you call any of the modern interfaces you hav=
e
quoted
to provide the resources and a device. I notice that there is only one
caller of pci_scan_bus_parented(), we should probably change that over =
to
quoted
pci_scan_root_bus() or your new interface and remove the old one, but
keep pci_scan_bus() as the only entry point for all of the legacy users
that do not know about the resources.
=20
Ok, I will move this out of the generic interface.
My suggestion would actually be to trigger a warning/error if you detect th=
at the resources
are missing. That way we can force the drivers to clean up.
Best regards,
Liviu
=20
Thanks!
Yijing.
=20
quoted
=20
=09Arnd
=20
.
=20
=20
=20
--=20
Thanks!
Yijing
=20
--
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
=20
--=20
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
=C2=AF\_(=E3=83=84)_/=C2=AF
On Mon, Nov 17, 2014 at 10:21:36AM +0000, Yijing Wang wrote:
From: Yijing Wang <redacted>
=20
Now we could use pci_scan_root_bus() instead of
pci_scan_bus(), pass NULL resources means use the default
io/mem.
Actually, I think this patch goes in the wrong direction. You are adding
two additional parameters that are NULL for every call you replace.
pci_scan_bus() was actually intended as a "shortcut with defaults" kind
of call so that you don't have to pass a value for parameters you don't
care about.
My suggestion would be to keep the function as is and replace the call
to pci_create_root_bus() inside with the appropriate sequence.
Best regards,
Liviu
@@ -5,7 +5,7 @@#ifdef __KERNEL__#include<linux/mm.h>=09=09/* for struct page */
=20
-/* Can be used to override the logic in pci_scan_bus for skipping
+/* Can be used to override the logic in pci_scan_root_bus for skipping
already-configured bus numbers - to be used for buggy BIOSes
or architectures with incomplete PCI setup by the loader */
=20
extern int pcibios_map_irq(const struct pci_dev *dev, u8 slot, u8 pin);
=20
=20
-/* Can be used to override the logic in pci_scan_bus for skipping
+/* Can be used to override the logic in pci_scan_root_bus for skipping
already-configured bus numbers - to be used for buggy BIOSes
or architectures with incomplete PCI setup by the loader */
=20
@@ -33,7 +33,7 @@ do {=09=09=09=09=09=09=09\#define __pcidebug(FMT, BUS, DEVFN, WHERE, ...)=09do {} while (0)#endif
=20
-/* Can be used to override the logic in pci_scan_bus for skipping
+/* Can be used to override the logic in pci_scan_root_bus for skipping
* already-configured bus numbers - to be used for buggy BIOSes or
* architectures with incomplete PCI setup by the loader */
=20
=20
#ifdef __KERNEL__
=20
-/* Can be used to override the logic in pci_scan_bus for skipping
+/* Can be used to override the logic in pci_scan_root_bus for skipping
already-configured bus numbers - to be used for buggy BIOSes
or architectures with incomplete PCI setup by the loader */
=20
=20
#include <linux/dma-mapping.h>
=20
-/* Can be used to override the logic in pci_scan_bus for skipping
+/* Can be used to override the logic in pci_scan_root_bus for skipping
* already-configured bus numbers - to be used for buggy BIOSes
* or architectures with incomplete PCI setup by the loader.
*/
=20
#include <linux/dma-mapping.h>
=20
-/* Can be used to override the logic in pci_scan_bus for skipping
+/* Can be used to override the logic in pci_scan_root_bus for skipping
* already-configured bus numbers - to be used for buggy BIOSes
* or architectures with incomplete PCI setup by the loader.
*/
@@ -41,7 +41,7 @@ static inline int pci_proc_domain(struct pci_bus *bus)}#endif
=20
-/* Can be used to override the logic in pci_scan_bus for skipping
+/* Can be used to override the logic in pci_scan_root_bus for skipping
already-configured bus numbers - to be used for buggy BIOSes
or architectures with incomplete PCI setup by the loader */
=20
=20
/**
* early_root_info_init()
- * called before pcibios_scan_root and pci_scan_bus
+ * called before pcibios_scan_root and pci_scan_root_bus
* fills the mp_bus_to_cpumask array based according
* to the LDT Bus Number Registers found in the northbridge.
*/
=20
#ifdef __KERNEL__
=20
-/* Can be used to override the logic in pci_scan_bus for skipping
+/* Can be used to override the logic in pci_scan_root_bus for skipping
* already-configured bus numbers - to be used for buggy BIOSes
* or architectures with incomplete PCI setup by the loader
*/
=09=09=09break;
=09=09}
=09}
--=20
1.7.1
=20
--
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
=20
--=20
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
=C2=AF\_(=E3=83=84)_/=C2=AF
On Mon, Nov 17, 2014 at 10:21:38AM +0000, Yijing Wang wrote:
Rip out pci_bus_add_devices() from pci_scan_root_bus()
for following reasons.
1. pci_scan_root_bus() means we only do the scan, we should
not add pci busses.
2. A lots of drviers which use pci_scan_root_bus() call
pci_bus_size_bridges(), pci_bus_assign_resources() after
pci_scan_root_bus(). But strictly speaking pci_bus_add_devices()
should be called after the resources assignment.
This looks like a nice cleanup! Can we split this patch out
of the series and start the ball running on getting ACKs
for it? You are touching a lot of drivers here, you probably
don't want the whole series to wait on approvals for this.
Best regards,
Liviu
=20
/* Configure the max_read_size and max_payload_size values. */
fixup_read_and_payload_sizes();
-
+ if (bus)
+ pci_bus_add_devices(bus);
/* Record the I/O resources in the PCI controller structure. */
for (i =3D 0; i < TILE_NUM_PCIE; i++) {
/*
@@ -903,7 +903,8 @@ int __init pcibios_init(void)*associatedwiththedevicesreadinabove.*/pci_assign_unassigned_resources();-+if(bus)+pci_bus_add_devices(bus);/* Record the I/O resources in the PCI controller structure. */for(i=3D0;i<num_rc_controllers;i++){structpci_controller*controller=3D&pci_controllers[i]=
if (!found)
pci_bus_update_busn_res_end(b, max);
=20
- pci_bus_add_devices(b);
return b;
}
EXPORT_SYMBOL(pci_scan_root_bus);
--
1.7.1
=20
--
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
=20
--=20
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
=C2=AF\_(=E3=83=84)_/=C2=AF
=20
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).
=20
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 opaq=
ue
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 th=
e
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 ca=
n
access the private data doing container_of().
Something like this:
struct pci_controller {
=09struct pci_host_bridge bridge;
=09/* private host bridge data here */
=09.....
};
#define PCI_CONTROLLER(bus)=09({
=09struct pci_host_bridge *hb =3D to_pci_host_bridge(bus->bridge); \
=09container_of(hb, struct pci_controller, bridge); })
Then we can retrieve the host bridge structure from everywhere we have a de=
vice.
Best regards,
Liviu
=20
The busnum should already be implied through the bus resource.
=20
Yes, I will consider remove it and introduce a helper function to get the=
root bus number, thanks!
=20
Thanks!
Yijing.
=20
quoted
=20
=09Arnd
=20
.
=20
=20
=20
--=20
Thanks!
Yijing
=20
--
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
=20
--=20
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
=C2=AF\_(=E3=83=84)_/=C2=AF
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.
=20
Please Cc: Yinghai Lu and Jiang Liu on future versions.
More comments on the conversion of pci_create_root_bus():
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 cr=
eation
it is worth in my opinion to move the domain setup in pci_create_host_bridg=
e()
and stop fiddling with it here.
Otherwise it looks to me like you are heading in the right direction.
Best regards,
Liviu
quoted hunk
+ b2 =3D pci_find_bus(pci_domain_nr(b), bridge->busnum); if (b2) { /* If we already got to this bus through a different brid=
void *release_data);
=20
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 corre=
spond
* to P2P or CardBus bridge windows) go in a table. Additional ones (fo=
r
quoted hunk
@@ -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
=20
--
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
=20
--=20
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
=C2=AF\_(=E3=83=84)_/=C2=AF
On Mon, Nov 17, 2014 at 10:21:42AM +0000, Yijing Wang wrote:
From: Yijing Wang <redacted>
=20
Now pci_host_bridge has been ripped out from pci root
bus creation. Currently pci_scan_root_bus() lacks
scalability, so platform host drivers have no proper
way to configure pci_host_bridge. E.g we should assign
msi_controller to pci_host_bridge, add argument for
pci_scan_root_bus() is not a good idea, it has already
five, so introudce struct pci_host_info to make
pci scan interfaces more scalable. Because almost
all host drivers need to configure host resources,
so we put .init_res() in it first, and add other
hooks when need.
=20
Signed-off-by: Yijing Wang <redacted>
---
drivers/pci/host-bridge.c | 27 +++++++++-------
drivers/pci/probe.c | 73 +++++++++++++++++++++++++++++++++++++++=
I have mixed feelings about this patch. While it is heading in the right di=
rection
of moving pci_host_bridge relevant information towards the right user, I do=
n't think
you picked up the right set to move. The resource list is going to be copie=
d into
internal pci_host_bridge list anyway, keeping another copy is not helpful *=
and*
you have increased the code size.
I think for now we should aim to get the *missing* data into pci_host_bridg=
e: MSI
controllers and PCI domain/segment. Then we can do more cleanup.
=09=09=09=09int busnr);
void pcie_update_link_speed(struct pci_bus *bus, u16 link_status);
--=20
1.7.1
=20
--
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
=20
--=20
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
=C2=AF\_(=E3=83=84)_/=C2=AF
From: Yijing Wang <hidden> Date: 2014-11-19 01:16:15
On 2014/11/18 22:23, Liviu Dudau wrote:
On Tue, Nov 18, 2014 at 11:46:06AM +0000, Yijing Wang wrote:
quoted
On 2014/11/18 17:36, Arnd Bergmann wrote:
quoted
On Tuesday 18 November 2014 15:44:23 Yijing Wang wrote:
quoted
On 2014/11/17 18:08, Arnd Bergmann wrote:
quoted
On Monday 17 November 2014 18:21:35 Yijing Wang wrote:
quoted
- list_for_each_entry(window, resources, list)- if (window->res->flags & IORESOURCE_BUS) {- found = true;- break;- }+ if (!resources) {+ pci_add_resource(&default_res, &ioport_resource);+ pci_add_resource(&default_res, &iomem_resource);+ pci_add_resource(&default_res, &busn_resource);+ } else {
Isn't it almost always wrong to do this? You are adding all of the
I/O ports and memory to the host bridge, which will prevent you from
adding another host bridge, and the iomem_resource normally
includes a lot of addresses that are not accessible by the PCI host.
Hi Arnd, pci host bridge windows are the ranges allow child devices to setup
from. Add all of IO/MEM here just a limit to child devices, no request for these
resources, so it won't hurt another host bridge. Some platforms have no dts or ACPI
report host bridge resources, in this case, we directly assign ioport/iomem_resources
as the root resources of PCI devices.
But it would be wrong to allow hosts to allocate a device BAR that is not
visible through the host bridge. I think we need to keep these separate
from the general case: if you call any of the modern interfaces you have
to provide the resources and a device. I notice that there is only one
caller of pci_scan_bus_parented(), we should probably change that over to
pci_scan_root_bus() or your new interface and remove the old one, but
keep pci_scan_bus() as the only entry point for all of the legacy users
that do not know about the resources.
Ok, I will move this out of the generic interface.
My suggestion would actually be to trigger a warning/error if you detect that the resources
are missing. That way we can force the drivers to clean up.
It make sense to me, thanks.
Best regards,
Liviu
quoted
Thanks!
Yijing.
quoted
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
From: Yijing Wang <hidden> Date: 2014-11-19 01:19:44
On 2014/11/18 22:28, Liviu Dudau wrote:
On Mon, Nov 17, 2014 at 10:21:36AM +0000, Yijing Wang wrote:
quoted
From: Yijing Wang <redacted>
Now we could use pci_scan_root_bus() instead of
pci_scan_bus(), pass NULL resources means use the default
io/mem.
Actually, I think this patch goes in the wrong direction. You are adding
two additional parameters that are NULL for every call you replace.
pci_scan_bus() was actually intended as a "shortcut with defaults" kind
of call so that you don't have to pass a value for parameters you don't
care about.
My suggestion would be to keep the function as is and replace the call
to pci_create_root_bus() inside with the appropriate sequence.
Hmmm, maybe you are right, provide a simple pci scan interfaces
(which don't care parent and resources) is better than use a complex
one with several NULL arguments. :)
@@ -49,7 +49,7 @@ struct pci_controller {void*sysdata;};-/* Override the logic in pci_scan_bus for skipping already-configured+/* Override the logic in pci_scan_root_bus for skipping already-configuredbusnumbers.*/#define pcibios_assign_all_busses() 1
@@ -5,7 +5,7 @@#ifdef __KERNEL__#include<linux/mm.h>/* for struct page */-/* Can be used to override the logic in pci_scan_bus for skipping+/* Can be used to override the logic in pci_scan_root_bus for skippingalready-configuredbusnumbers-tobeusedforbuggyBIOSesorarchitectureswithincompletePCIsetupbytheloader*/
@@ -60,7 +60,7 @@ extern void register_pci_controller(struct pci_controller *hose);externintpcibios_map_irq(conststructpci_dev*dev,u8slot,u8pin);-/* Can be used to override the logic in pci_scan_bus for skipping+/* Can be used to override the logic in pci_scan_root_bus for skippingalready-configuredbusnumbers-tobeusedforbuggyBIOSesorarchitectureswithincompletePCIsetupbytheloader*/
@@ -33,7 +33,7 @@ do { \#define __pcidebug(FMT, BUS, DEVFN, WHERE, ...) do {} while (0)#endif-/* Can be used to override the logic in pci_scan_bus for skipping+/* Can be used to override the logic in pci_scan_root_bus for skipping*already-configuredbusnumbers-tobeusedforbuggyBIOSesor*architectureswithincompletePCIsetupbytheloader*/
@@ -3,7 +3,7 @@#ifdef __KERNEL__-/* Can be used to override the logic in pci_scan_bus for skipping+/* Can be used to override the logic in pci_scan_root_bus for skippingalready-configuredbusnumbers-tobeusedforbuggyBIOSesorarchitectureswithincompletePCIsetupbytheloader*/
@@ -5,7 +5,7 @@#include<linux/dma-mapping.h>-/* Can be used to override the logic in pci_scan_bus for skipping+/* Can be used to override the logic in pci_scan_root_bus for skipping*already-configuredbusnumbers-tobeusedforbuggyBIOSes*orarchitectureswithincompletePCIsetupbytheloader.*/
@@ -5,7 +5,7 @@#include<linux/dma-mapping.h>-/* Can be used to override the logic in pci_scan_bus for skipping+/* Can be used to override the logic in pci_scan_root_bus for skipping*already-configuredbusnumbers-tobeusedforbuggyBIOSes*orarchitectureswithincompletePCIsetupbytheloader.*/
@@ -41,7 +41,7 @@ static inline int pci_proc_domain(struct pci_bus *bus)}#endif-/* Can be used to override the logic in pci_scan_bus for skipping+/* Can be used to override the logic in pci_scan_root_bus for skippingalready-configuredbusnumbers-tobeusedforbuggyBIOSesorarchitectureswithincompletePCIsetupbytheloader*/
@@ -13,7 +13,7 @@#ifdef __KERNEL__-/* Can be used to override the logic in pci_scan_bus for skipping+/* Can be used to override the logic in pci_scan_root_bus for skipping*already-configuredbusnumbers-tobeusedforbuggyBIOSes*orarchitectureswithincompletePCIsetupbytheloader*/
@@ -787,7 +787,7 @@ static int __init dino_common_init(struct parisc_device *dev,pcibios_register_hba(&dino_dev->hba);-pci_bios=&dino_bios_ops;/* used by pci_scan_bus() */+pci_bios=&dino_bios_ops;pci_port=&dino_port_ops;/*
--
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
From: Yijing Wang <hidden> Date: 2014-11-19 01:21:49
On 2014/11/18 22:34, Liviu Dudau wrote:
On Mon, Nov 17, 2014 at 10:21:38AM +0000, Yijing Wang wrote:
quoted
Rip out pci_bus_add_devices() from pci_scan_root_bus()
for following reasons.
1. pci_scan_root_bus() means we only do the scan, we should
not add pci busses.
2. A lots of drviers which use pci_scan_root_bus() call
pci_bus_size_bridges(), pci_bus_assign_resources() after
pci_scan_root_bus(). But strictly speaking pci_bus_add_devices()
should be called after the resources assignment.
This looks like a nice cleanup! Can we split this patch out
of the series and start the ball running on getting ACKs
for it? You are touching a lot of drivers here, you probably
don't want the whole series to wait on approvals for this.
OK, I will separately this one out of this series.
@@ -326,7 +326,8 @@ int __init pcibios_init(void)/* Configure the max_read_size and max_payload_size values. */fixup_read_and_payload_sizes();-+if(bus)+pci_bus_add_devices(bus);/* Record the I/O resources in the PCI controller structure. */for(i=0;i<TILE_NUM_PCIE;i++){/*
@@ -903,7 +903,8 @@ int __init pcibios_init(void)*associatedwiththedevicesreadinabove.*/pci_assign_unassigned_resources();-+if(bus)+pci_bus_add_devices(bus);/* Record the I/O resources in the PCI controller structure. */for(i=0;i<num_rc_controllers;i++){structpci_controller*controller=&pci_controllers[i];
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
From: Yijing Wang <hidden> Date: 2014-11-19 01:45:18
On 2014/11/18 23:30, Liviu Dudau wrote:
On Mon, Nov 17, 2014 at 10:21:41AM +0000, Yijing Wang wrote:
quoted
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.
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.
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
quoted
+ 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);
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
+static int pci_default_init_res(struct pci_host_bridge *host,+ struct pci_host_info *info)+{+ struct pci_host_bridge_window *window, *n;++ if (info->res_type != PCI_HOST_RES_DEFAULT) + list_for_each_entry_safe(window, n, info->resources,+ list)+ list_move_tail(&window->list, &host->windows);+ else+ info->res_type = PCI_HOST_RES_DEFAULT;
I'm confused about this assignment. Isn't this a nop as the else part
means info->res_type *is* PCI_HOST_RES_DEFAULT?
No, in this patch, host drivers pass a pci host bridge resources init hook
in pci_host_info *info, and we call this info->init_res() in pci_create_host_bridge().
+struct pci_host_info {
+ u8 res_type;
+ void *arg;
+ struct list_head *resources; /*just for build, will clean up later */
+ int (*init_res)(struct pci_host_bridge *host,
+ struct pci_host_info *info);
+};
+
I have mixed feelings about this patch. While it is heading in the right direction
of moving pci_host_bridge relevant information towards the right user, I don't think
you picked up the right set to move. The resource list is going to be copied into
internal pci_host_bridge list anyway, keeping another copy is not helpful *and*
you have increased the code size.
I think for now we should aim to get the *missing* data into pci_host_bridge: MSI
controllers and PCI domain/segment. Then we can do more cleanup.
Hi Liviu, I agree with you here, the changes to resources stuff seems not a perfect
solution. In my patch 6, we could pass pci domain nr by u32 PCI_DOMBUS(domain, bus) argument,
and store it in pci_host_bridge. For msi controller, we couldn't save the msi_controller
in pci_host_bridge. Before we assume one pci host bridge only had one msi_controller,
but now something changes, Jiang introduce hierarchy irq domain in x86, and now
one pci host bridge may has more than one msi_controller. So I prefer to add a
function to pci_host_bridge something like
struct msi_controller *pci_get_msi_controller(struct pci_dev *dev)
@@ -412,6 +412,21 @@ struct pci_host_bridge {void*release_data;};+structpci_host_info{+u8res_type;+void*arg;+structlist_head*resources;/*just for build, will clean up later */+int(*init_res)(structpci_host_bridge*host,+structpci_host_info*info);+};++staticinlinevoidinit_pci_host_info(structpci_host_info*info)+{+memset(info,0,sizeof(*info));+}
Where is this used?
Host driver uses it to init pci_host_info.
quoted
++#define PCI_HOST_RES_DEFAULT 0x2+
Magic number?
Hmmm, I will rework pci host bridge resources stuff.
--
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
From: Yijing Wang <hidden> Date: 2014-11-19 02:25:22
quoted
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
quoted
quoted
quoted
+ 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.
quoted
quoted
+ 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.
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.
quoted
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:
quoted
quoted
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.
quoted
Best regards,
Liviu
quoted
quoted
quoted
+ 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.
quoted
quoted
+ 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.
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.
quoted
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 01:42:52AM +0000, Yijing Wang wrote:
On 2014/11/18 23:30, Liviu Dudau wrote:
quoted
On Mon, Nov 17, 2014 at 10:21:41AM +0000, Yijing Wang wrote:
quoted
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(=
),
quoted
quoted
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():
=20
That's my mistake, will add CC Yinghai and Jiang in next version.
=20
I don't thinks it is worth moving the buses' pci_ops into pci_host_brid=
ge. It
quoted
might be more useful to have pci_host_bridge specific ops here.
=20
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 r=
eason
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 oth=
er HB
specific stuff, you use the HB ops. For config R/W acceses you use bus ops.
Currently, bus->sysdata holds a pointer to the arch/driver host bridge stru=
cture, as passed
in pci_create_root_bus(). If you agree with my idea of wrapping the host br=
idge driver
structure around the pci_host_bridge, then we will always have a way of ret=
rieving that
information; but for backwards compatibility we could set bus->sysdata to b=
e the bridge.
Then existing macros that convert sysdata to pci_controller can be reused a=
fter being
updated.
Best regards,
Liviu
=20
quoted
quoted
+ b->ops =3D bridge->ops;
See comment above why I don't think this is necessary.
{ struct pci_host_bridge_window *window; bool found =3D 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 =3D true;- break;- }- }+ host =3D pci_create_host_bridge(parent, db, ops, sysdata, reso=
urces);
quoted
quoted
+ if (!host)+ return NULL;- b =3D 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 =3D true;+ break;+ }++ host->bus =3D __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 %=
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" i=
=20
=20
--
Thanks!
Yijing
=20
--
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
=20
--=20
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
=C2=AF\_(=E3=83=84)_/=C2=AF
+static int pci_default_init_res(struct pci_host_bridge *host,+=09=09struct pci_host_info *info)+{+=09struct pci_host_bridge_window *window, *n;++=09if (info->res_type !=3D PCI_HOST_RES_DEFAULT)=20+=09=09list_for_each_entry_safe(window, n, info->resources,+=09=09=09=09list)+=09=09=09list_move_tail(&window->list, &host->windows);+=09else+=09=09info->res_type =3D PCI_HOST_RES_DEFAULT;
=20
I'm confused about this assignment. Isn't this a nop as the else part
means info->res_type *is* PCI_HOST_RES_DEFAULT?
=20
No, in this patch, host drivers pass a pci host bridge resources init hoo=
k
in pci_host_info *info, and we call this info->init_res() in pci_create_h=
ost_bridge().
=20
+struct pci_host_info {
+=09u8 res_type;
+=09void *arg;
+=09struct list_head *resources; /*just for build, will clean up later */
+=09int (*init_res)(struct pci_host_bridge *host,
+=09=09=09struct pci_host_info *info);
+};
+
That's not what I've asked! Your code does:
=09if (info->res_type !=3D PCI_HOST_RES_DEFAULT)
=09=09....
=09else /* info->res_type =3D=3D PCI_HOST_RES_DEFAULT)
=09=09info->res_type =3D PCI_HOST_RES_DEFAULT;
info->res_type is already =3D=3D PCI_HOST_RES_DEFAULT in the else side, ass=
ignment is a NOP?
+=09=09=09host->busnum);+=09=09pci_bus_insert_busn_res(host->bus, host->busnum, 255);+=09}++=09max =3D pci_scan_child_bus(host->bus);+=09if (!found)+=09=09pci_bus_update_busn_res_end(host->bus, max);++=09return host;++}+EXPORT_SYMBOL(pci_scan_host_bridge);+ /** * pci_rescan_bus_bridge_resize - scan a PCI bus for devices. * @bridge: PCI bridge for the bus to scan
=09=09=09=09int busnr);
void pcie_update_link_speed(struct pci_bus *bus, u16 link_status);
--=20
1.7.1
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" i=
=20
=20
--=20
Thanks!
Yijing
=20
--
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
=20
--=20
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
=C2=AF\_(=E3=83=84)_/=C2=AF
From: Yijing Wang <hidden> Date: 2014-11-20 02:01:23
quoted
quoted
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
quoted
Thanks!
Yijing.
quoted
Best regards,
Liviu
quoted
quoted
quoted
+ 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.
quoted
quoted
+ 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.
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.
quoted
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
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().
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
quoted
quoted
quoted
+ b->ops = bridge->ops;
See comment above why I don't think this is necessary.
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.
quoted
Otherwise it looks to me like you are heading in the right direction.
Thanks!
Yijing.
quoted
Best regards,
Liviu
quoted
+ 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);
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
From: Yijing Wang <hidden> Date: 2014-11-20 02:54:34
quoted
No, in this patch, host drivers pass a pci host bridge resources init hook
in pci_host_info *info, and we call this info->init_res() in pci_create_host_bridge().
+struct pci_host_info {
+ u8 res_type;
+ void *arg;
+ struct list_head *resources; /*just for build, will clean up later */
+ int (*init_res)(struct pci_host_bridge *host,
+ struct pci_host_info *info);
+};
+
That's not what I've asked! Your code does:
if (info->res_type != PCI_HOST_RES_DEFAULT)
....
else /* info->res_type == PCI_HOST_RES_DEFAULT)
info->res_type = PCI_HOST_RES_DEFAULT;
info->res_type is already == PCI_HOST_RES_DEFAULT in the else side, assignment is a NOP?
Hmmm, I wanted pci_create_host_bridge() to process the default res later(add default res),
It's ugly code, I will rework it.
I have mixed feelings about this patch. While it is heading in the right direction
of moving pci_host_bridge relevant information towards the right user, I don't think
you picked up the right set to move. The resource list is going to be copied into
internal pci_host_bridge list anyway, keeping another copy is not helpful *and*
you have increased the code size.
I think for now we should aim to get the *missing* data into pci_host_bridge: MSI
controllers and PCI domain/segment. Then we can do more cleanup.
Hi Liviu, I agree with you here, the changes to resources stuff seems not a perfect
solution. In my patch 6, we could pass pci domain nr by u32 PCI_DOMBUS(domain, bus) argument,
and store it in pci_host_bridge. For msi controller, we couldn't save the msi_controller
in pci_host_bridge. Before we assume one pci host bridge only had one msi_controller,
but now something changes, Jiang introduce hierarchy irq domain in x86, and now
one pci host bridge may has more than one msi_controller. So I prefer to add a
function to pci_host_bridge something like
struct msi_controller *pci_get_msi_controller(struct pci_dev *dev)
@@ -412,6 +412,21 @@ struct pci_host_bridge {void*release_data;};+structpci_host_info{+u8res_type;+void*arg;+structlist_head*resources;/*just for build, will clean up later */+int(*init_res)(structpci_host_bridge*host,+structpci_host_info*info);+};++staticinlinevoidinit_pci_host_info(structpci_host_info*info)+{+memset(info,0,sizeof(*info));+}
Where is this used?
Host driver uses it to init pci_host_info.
Might be worth adding it that patch rather than here.
OK
Best regards,
Liviu
quoted
quoted
quoted
++#define PCI_HOST_RES_DEFAULT 0x2+
Magic number?
Hmmm, I will rework pci host bridge resources stuff.
--
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
I don't thinks it is worth moving the buses' pci_ops into pci_host_br=
idge. It
quoted
quoted
quoted
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=
,
quoted
quoted
so when we scan the root bus and child buses, we use pci_host_bridge a=
s
quoted
quoted
the only argument, and another pci_host_info will be addes in later pa=
tch,
quoted
quoted
which one support carry the pci_host_bridge ops.
But pci_create_root_bus() already has a pci_ops argument, I don't see t=
he reason
quoted
to drop that.
pci_create_host_bridge() can get pci_host_bridge ops while pci_create_r=
oot_bus() gets
quoted
the bus ops. For find out the MSI controller, the domain number and any=
other HB
quoted
specific stuff, you use the HB ops. For config R/W acceses you use bus =
ops.
quoted
=20
I want to unexport pci_create_root_bus() if we have pci_create_host_bridg=
e().
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 sysda=
ta 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 cal=
l 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.
Currently, bus->sysdata holds a pointer to the arch/driver host bridge =
structure, as passed
quoted
in pci_create_root_bus(). If you agree with my idea of wrapping the hos=
t bridge driver
quoted
structure around the pci_host_bridge, then we will always have a way of=
retrieving that
quoted
information; but for backwards compatibility we could set bus->sysdata =
to be the bridge.
quoted
Then existing macros that convert sysdata to pci_controller can be reus=
ed after being
quoted
updated.
=20
I think wrapping the host bridge driver structure around the pci_host_bri=
dge 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 ?=20
Of course we put pci_host_bridge init *calls* in the host drivers because w=
e need to do host
driver specific initialisation there anyway.=20
And if we still use arch spec
macros convert ssydata to pci_controller, so what's the greatest advantag=
e ?
Agree, the macro is not useful unless we have a common pci_controller struc=
ture. Sorry about
the noise.
=20
If we move domain and msi_chip out of sysdata, what's other things in sys=
data still need us
to convert in PCI core ?
I've never said we need to keep sysdata. From my discussion with Bjorn a ye=
ar ago the idea was
to put the sysdata members into pci_host_bridge structure and remove sysdat=
a member from pci_bus.
pci_scan_root_bus() can still get a parameter called sysdata if we want, bu=
t it will be a
struct pci_host_bridge * type.
Best regards,
Liviu
=20
=20
=20
quoted
Best regards,
Liviu
quoted
quoted
quoted
+ b->ops =3D bridge->ops;
See comment above why I don't think this is necessary.
{ struct pci_host_bridge_window *window; bool found =3D 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 =3D true;- break;- }- }+ host =3D pci_create_host_bridge(parent, db, ops, sysdata, re=
sources);
quoted
quoted
quoted
quoted
+ if (!host)+ return NULL;- b =3D 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 =3D true;+ break;+ }++ host->bus =3D __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=
+void pci_free_host_bridge(struct pci_host_bridge *host);
int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busmax)=
;
quoted
quoted
quoted
quoted
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"=
--=20
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
=C2=AF\_(=E3=83=84)_/=C2=AF
From: Tomasz Nowicki <hidden> Date: 2014-11-20 11:54:58
On 17.11.2014 15:13, Arnd Bergmann wrote:
On Monday 17 November 2014 18:21:34 Yijing Wang wrote:
quoted
This series is based Linux 3.18-rc1 and Lorenzo Pieralisi's
arm PCI domain cleanup patches, link:
https://patchwork.ozlabs.org/patch/407585/
Current pci scan interfaces like pci_scan_root_bus() and directly
call pci_create_root_bus()/pci_scan_child_bus() lack flexiblity.
Some platform infos like PCI domain and msi_chip have to be
associated to PCI bus by some arch specific function.
We want to make a generic pci_host_bridge, and make it hold
the platform infos or hook. Then we could eliminate the lots
of arch pci_domain_nr, also we could associate some platform
ops something like pci_get_msi_chip(struct pci_dev *dev)
with pci_host_bridge to avoid introduce arch weak functions.
This RFC version not for all platforms, just applied the new
scan interface in x86/arm/powerpc/ia64, I will refresh other
platforms after the core pci scan interfaces are ok.
I think overall this is a good direction to take, in particular
moving more things into struct pci_host_bridge so we can
slim down the architecture specific code.
I don't particularly like the way you use the 'pci_host_info'
to pass callback pointers and some of the generic information.
This duplicates some of the issues we are currently trying
to untangle in the arm32 code to make drivers easier to share
between architectures.
As a general approach, I'd rather see generic helper functions
being exported by the PCI core that a driver may or may not
call.
The way you split the interface between things that happen
before scanning the buses (pci_create_host_bridge) and
the actual scanning (__pci_create_root_bus, pci_scan_child_bus)
seems very helpful and I think we can expand that concept further:
- The normal pci_create_host_bridge() function can contain
all of the DT scanning functions (finding bus/mem/io resources,
finding the msi-parent), while drivers that don't depend on DT
for this information can call the same function and fill the
same things after they have the pci_host_bridge pointer.
How about finding PCI domain number (in the DT way) within pci_create_host_bridge() too ?
Tomasz
From: Tomasz Nowicki <hidden> Date: 2014-11-20 12:01:15
On 18.11.2014 13:27, Arnd Bergmann wrote:
On Tuesday 18 November 2014 20:17:57 Yijing Wang wrote:
quoted
quoted
quoted
I hope platforms with ACPI or DT could both use pci_create_host_bridge().
Why we need to use two different ways to process it ?
These are completely different use cases:
a) For DT, we want loadable device drivers that start by probing a host
bridge device which was added through the DT platform code. The
driver is self-contained, and eventually we want to be able to unload
it. We have lots of different per-soc drivers that require different
quirks
b) For ACPI, the interface is defined in the ACPI spec across architectures
and SoCs, we don't have host bridge drivers and the code that initializes
the PCI is required early during boot and called from architecture
code. There is no parent device, as ACPI sees PCI as a fundamental building
block by itself, and there are no drivers because the firmware does
the initial hardware setup, so we only have to access the config space.
Hmmm, I'm a little confused, so why you think ACPI host driver should not use
pci_create_host_bridge(), because ACPI PCI driver has no parent device ?
It's one of the difference. Having a parent device can certainly make your
life simpler, since you have devm_kzalloc(), dev_info(), etc. Coming from
the other end, I think ACPI needs PCI to be available during early boot,
at a time where we might not want pci_create_host_bridge() to do the
right thing.
Device pointer is not required for ACPI, struct acpi_device is all we need to get all that info. If pci_create_host_bridge() would be DT specific, it would be nice to have sth similar for ACPI but that is out of this patch set scope.
Tomasz
On Thu, Nov 20, 2014 at 12:54:48PM +0100, Tomasz Nowicki wrote:
On 17.11.2014 15:13, Arnd Bergmann wrote:
quoted
On Monday 17 November 2014 18:21:34 Yijing Wang wrote:
quoted
This series is based Linux 3.18-rc1 and Lorenzo Pieralisi's
arm PCI domain cleanup patches, link:
https://patchwork.ozlabs.org/patch/407585/
Current pci scan interfaces like pci_scan_root_bus() and directly
call pci_create_root_bus()/pci_scan_child_bus() lack flexiblity.
Some platform infos like PCI domain and msi_chip have to be
associated to PCI bus by some arch specific function.
We want to make a generic pci_host_bridge, and make it hold
the platform infos or hook. Then we could eliminate the lots
of arch pci_domain_nr, also we could associate some platform
ops something like pci_get_msi_chip(struct pci_dev *dev)
with pci_host_bridge to avoid introduce arch weak functions.
This RFC version not for all platforms, just applied the new
scan interface in x86/arm/powerpc/ia64, I will refresh other
platforms after the core pci scan interfaces are ok.
I think overall this is a good direction to take, in particular
moving more things into struct pci_host_bridge so we can
slim down the architecture specific code.
I don't particularly like the way you use the 'pci_host_info'
to pass callback pointers and some of the generic information.
This duplicates some of the issues we are currently trying
to untangle in the arm32 code to make drivers easier to share
between architectures.
As a general approach, I'd rather see generic helper functions
being exported by the PCI core that a driver may or may not
call.
The way you split the interface between things that happen
before scanning the buses (pci_create_host_bridge) and
the actual scanning (__pci_create_root_bus, pci_scan_child_bus)
seems very helpful and I think we can expand that concept further:
- The normal pci_create_host_bridge() function can contain
all of the DT scanning functions (finding bus/mem/io resources,
finding the msi-parent), while drivers that don't depend on DT
for this information can call the same function and fill the
same things after they have the pci_host_bridge pointer.
How about finding PCI domain number (in the DT way) within
pci_create_host_bridge() too ?
It is an idea worth pursuing for the 99% of the cases. I would like
to understand the 1% of the time when we want a domain number to be
shared between two host bridges or the time when we want more than
one domain per bridge.
Is that possible? Is it useful? Is it already in practice?
Best regards,
Liviu
From: Tomasz Nowicki <hidden> Date: 2014-11-20 12:53:55
On 20.11.2014 13:08, Liviu Dudau wrote:
On Thu, Nov 20, 2014 at 12:54:48PM +0100, Tomasz Nowicki wrote:
quoted
On 17.11.2014 15:13, Arnd Bergmann wrote:
quoted
On Monday 17 November 2014 18:21:34 Yijing Wang wrote:
quoted
This series is based Linux 3.18-rc1 and Lorenzo Pieralisi's
arm PCI domain cleanup patches, link:
https://patchwork.ozlabs.org/patch/407585/
Current pci scan interfaces like pci_scan_root_bus() and directly
call pci_create_root_bus()/pci_scan_child_bus() lack flexiblity.
Some platform infos like PCI domain and msi_chip have to be
associated to PCI bus by some arch specific function.
We want to make a generic pci_host_bridge, and make it hold
the platform infos or hook. Then we could eliminate the lots
of arch pci_domain_nr, also we could associate some platform
ops something like pci_get_msi_chip(struct pci_dev *dev)
with pci_host_bridge to avoid introduce arch weak functions.
This RFC version not for all platforms, just applied the new
scan interface in x86/arm/powerpc/ia64, I will refresh other
platforms after the core pci scan interfaces are ok.
I think overall this is a good direction to take, in particular
moving more things into struct pci_host_bridge so we can
slim down the architecture specific code.
I don't particularly like the way you use the 'pci_host_info'
to pass callback pointers and some of the generic information.
This duplicates some of the issues we are currently trying
to untangle in the arm32 code to make drivers easier to share
between architectures.
As a general approach, I'd rather see generic helper functions
being exported by the PCI core that a driver may or may not
call.
The way you split the interface between things that happen
before scanning the buses (pci_create_host_bridge) and
the actual scanning (__pci_create_root_bus, pci_scan_child_bus)
seems very helpful and I think we can expand that concept further:
- The normal pci_create_host_bridge() function can contain
all of the DT scanning functions (finding bus/mem/io resources,
finding the msi-parent), while drivers that don't depend on DT
for this information can call the same function and fill the
same things after they have the pci_host_bridge pointer.
How about finding PCI domain number (in the DT way) within
pci_create_host_bridge() too ?
It is an idea worth pursuing for the 99% of the cases. I would like
to understand the 1% of the time when we want a domain number to be
shared between two host bridges or the time when we want more than
one domain per bridge.
Even though we have shared domain, this should be resolved via DT calls, do I miss something ?
Is that possible? Is it useful? Is it already in practice?
This is good question... IMO:
1. Two host bridges can shared domain number if they are children of the same parent host bridge.
2. But I can not find good explanation for more than one domain per bridge.
Tomasz
On Thursday 20 November 2014 13:01:08 Tomasz Nowicki wrote:
On 18.11.2014 13:27, Arnd Bergmann wrote:
quoted
On Tuesday 18 November 2014 20:17:57 Yijing Wang wrote:
quoted
quoted
quoted
I hope platforms with ACPI or DT could both use pci_create_host_bridge().
Why we need to use two different ways to process it ?
These are completely different use cases:
a) For DT, we want loadable device drivers that start by probing a host
bridge device which was added through the DT platform code. The
driver is self-contained, and eventually we want to be able to unload
it. We have lots of different per-soc drivers that require different
quirks
b) For ACPI, the interface is defined in the ACPI spec across architectures
and SoCs, we don't have host bridge drivers and the code that initializes
the PCI is required early during boot and called from architecture
code. There is no parent device, as ACPI sees PCI as a fundamental building
block by itself, and there are no drivers because the firmware does
the initial hardware setup, so we only have to access the config space.
Hmmm, I'm a little confused, so why you think ACPI host driver should not use
pci_create_host_bridge(), because ACPI PCI driver has no parent device ?
It's one of the difference. Having a parent device can certainly make your
life simpler, since you have devm_kzalloc(), dev_info(), etc. Coming from
the other end, I think ACPI needs PCI to be available during early boot,
at a time where we might not want pci_create_host_bridge() to do the
right thing.
Device pointer is not required for ACPI, struct acpi_device is all we
need to get all that info. If pci_create_host_bridge() would be DT
specific, it would be nice to have sth similar for ACPI but that is out
of this patch set scope.
My point was more that we don't need to have something like it for ACPI,
since we don't get random drivers that need to be probed that way,
just one common implementation that calls into the PCI core. We should
of course share the common bits with pci_create_host_bridge() in some
form, but that can be done by moving the x86 pci_acpi_scan_root
function and/or acpi_pci_root_add() to a common place in drivers/pci
and then refactoring the internals.
Arnd
On Thu, Nov 20, 2014 at 01:53:48PM +0100, Tomasz Nowicki wrote:
On 20.11.2014 13:08, Liviu Dudau wrote:
quoted
On Thu, Nov 20, 2014 at 12:54:48PM +0100, Tomasz Nowicki wrote:
quoted
On 17.11.2014 15:13, Arnd Bergmann wrote:
quoted
On Monday 17 November 2014 18:21:34 Yijing Wang wrote:
quoted
This series is based Linux 3.18-rc1 and Lorenzo Pieralisi's
arm PCI domain cleanup patches, link:
https://patchwork.ozlabs.org/patch/407585/
Current pci scan interfaces like pci_scan_root_bus() and directly
call pci_create_root_bus()/pci_scan_child_bus() lack flexiblity.
Some platform infos like PCI domain and msi_chip have to be
associated to PCI bus by some arch specific function.
We want to make a generic pci_host_bridge, and make it hold
the platform infos or hook. Then we could eliminate the lots
of arch pci_domain_nr, also we could associate some platform
ops something like pci_get_msi_chip(struct pci_dev *dev)
with pci_host_bridge to avoid introduce arch weak functions.
This RFC version not for all platforms, just applied the new
scan interface in x86/arm/powerpc/ia64, I will refresh other
platforms after the core pci scan interfaces are ok.
I think overall this is a good direction to take, in particular
moving more things into struct pci_host_bridge so we can
slim down the architecture specific code.
I don't particularly like the way you use the 'pci_host_info'
to pass callback pointers and some of the generic information.
This duplicates some of the issues we are currently trying
to untangle in the arm32 code to make drivers easier to share
between architectures.
As a general approach, I'd rather see generic helper functions
being exported by the PCI core that a driver may or may not
call.
The way you split the interface between things that happen
before scanning the buses (pci_create_host_bridge) and
the actual scanning (__pci_create_root_bus, pci_scan_child_bus)
seems very helpful and I think we can expand that concept further:
- The normal pci_create_host_bridge() function can contain
all of the DT scanning functions (finding bus/mem/io resources,
finding the msi-parent), while drivers that don't depend on DT
for this information can call the same function and fill the
same things after they have the pci_host_bridge pointer.
How about finding PCI domain number (in the DT way) within
pci_create_host_bridge() too ?
It is an idea worth pursuing for the 99% of the cases. I would like
to understand the 1% of the time when we want a domain number to be
shared between two host bridges or the time when we want more than
one domain per bridge.
Even though we have shared domain, this should be resolved via DT calls, do
I miss something ?
If we only going to hold one domain number per host bridge, then no, you're
not missing anything.
quoted
Is that possible? Is it useful? Is it already in practice?
This is good question... IMO:
1. Two host bridges can shared domain number if they are children of the
same parent host bridge.
2. But I can not find good explanation for more than one domain per bridge.
Splitting a root bus into two or more "segments" ?
Best regards,
Liviu
Tomasz
--
-------------------
.oooO
( )
\ ( Oooo.
\_) ( )
) /
(_/
One small step
for me ...
From: Yijing Wang <hidden> Date: 2014-11-21 02:55:56
quoted
quoted
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. :)
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.
quoted
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.
quoted
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
quoted
quoted
Best regards,
Liviu
quoted
quoted
quoted
+ b->ops = bridge->ops;
See comment above why I don't think this is necessary.
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.
quoted
Otherwise it looks to me like you are heading in the right direction.
Thanks!
Yijing.
quoted
Best regards,
Liviu
quoted
+ 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);
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
From: Yijing Wang <hidden> Date: 2014-11-21 02:59:06
On 2014/11/21 0:39, Liviu Dudau wrote:
On Thu, Nov 20, 2014 at 01:53:48PM +0100, Tomasz Nowicki wrote:
quoted
On 20.11.2014 13:08, Liviu Dudau wrote:
quoted
On Thu, Nov 20, 2014 at 12:54:48PM +0100, Tomasz Nowicki wrote:
quoted
On 17.11.2014 15:13, Arnd Bergmann wrote:
quoted
On Monday 17 November 2014 18:21:34 Yijing Wang wrote:
quoted
This series is based Linux 3.18-rc1 and Lorenzo Pieralisi's
arm PCI domain cleanup patches, link:
https://patchwork.ozlabs.org/patch/407585/
Current pci scan interfaces like pci_scan_root_bus() and directly
call pci_create_root_bus()/pci_scan_child_bus() lack flexiblity.
Some platform infos like PCI domain and msi_chip have to be
associated to PCI bus by some arch specific function.
We want to make a generic pci_host_bridge, and make it hold
the platform infos or hook. Then we could eliminate the lots
of arch pci_domain_nr, also we could associate some platform
ops something like pci_get_msi_chip(struct pci_dev *dev)
with pci_host_bridge to avoid introduce arch weak functions.
This RFC version not for all platforms, just applied the new
scan interface in x86/arm/powerpc/ia64, I will refresh other
platforms after the core pci scan interfaces are ok.
I think overall this is a good direction to take, in particular
moving more things into struct pci_host_bridge so we can
slim down the architecture specific code.
I don't particularly like the way you use the 'pci_host_info'
to pass callback pointers and some of the generic information.
This duplicates some of the issues we are currently trying
to untangle in the arm32 code to make drivers easier to share
between architectures.
As a general approach, I'd rather see generic helper functions
being exported by the PCI core that a driver may or may not
call.
The way you split the interface between things that happen
before scanning the buses (pci_create_host_bridge) and
the actual scanning (__pci_create_root_bus, pci_scan_child_bus)
seems very helpful and I think we can expand that concept further:
- The normal pci_create_host_bridge() function can contain
all of the DT scanning functions (finding bus/mem/io resources,
finding the msi-parent), while drivers that don't depend on DT
for this information can call the same function and fill the
same things after they have the pci_host_bridge pointer.
How about finding PCI domain number (in the DT way) within
pci_create_host_bridge() too ?
It is an idea worth pursuing for the 99% of the cases. I would like
to understand the 1% of the time when we want a domain number to be
shared between two host bridges or the time when we want more than
one domain per bridge.
Even though we have shared domain, this should be resolved via DT calls, do
I miss something ?
If we only going to hold one domain number per host bridge, then no, you're
not missing anything.
quoted
quoted
Is that possible? Is it useful? Is it already in practice?
This is good question... IMO:
1. Two host bridges can shared domain number if they are children of the
same parent host bridge.
2. But I can not find good explanation for more than one domain per bridge.
Splitting a root bus into two or more "segments" ?
{ struct pci_host_bridge_window *window; bool found =3D 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 =3D true;- break;- }- }+ host =3D pci_create_host_bridge(parent, db, ops, sysdata, =
resources);
quoted
quoted
quoted
quoted
quoted
quoted
+ if (!host)+ return NULL;- b =3D 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 =3D true;+ break;+ }++ host->bus =3D __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 [b=
+void pci_free_host_bridge(struct pci_host_bridge *host);
int pci_bus_insert_busn_res(struct pci_bus *b, int bus, int busma=
x);
quoted
quoted
quoted
quoted
quoted
quoted
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-pc=
=20
=20
--
Thanks!
Yijing
=20
--
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
=20
--=20
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
| I would like to |
| fix the world, |
| but they're not |
| giving me the |
\ source code! /
---------------
=C2=AF\_(=E3=83=84)_/=C2=AF