| Message ID | 20140212194313.GA17248@obsidianresearch.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wednesday 12 February 2014 12:43:13 Jason Gunthorpe wrote: > On Tue, Feb 11, 2014 at 11:42:52AM +0100, Arnd Bergmann wrote: > > > Still missing release_region.. > > Thoughts on upstreamining these bits? Upstreaming them would be great. Patches look correct by inspection as well. > > Since the mvebu_pcie_setup() function seems very generic at this, > > we should probably try to factor out that code into a common > > helper, at least for arm64, but ideally shared with arm32 > > as well. > > Yah, especially since people are not getting it completely right.. > > But some of the trouble here is a lack of a generic pci host driver > structure, eg I have to pull the domain number out of the ARM32 > specific structure .. I'm sure that Bjorn also has some plans of his own, but I think we should have a generic host driver structure and gradually move stuff into it from the architecture specific structs. Now there is a 'struct pci_host_bridge' that is used on some architectures already but not on ARM. It also contains a list of resource windows plus offsets, and there is a set of functions associated with it: pci_create_root_bus() pcibios_root_bridge_prepare() etc. Maybe all that's needed is to actually start using those on arm32? Arnd
On Wed, Feb 12, 2014 at 1:07 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Wednesday 12 February 2014 12:43:13 Jason Gunthorpe wrote: >> On Tue, Feb 11, 2014 at 11:42:52AM +0100, Arnd Bergmann wrote: >> >> >> Still missing release_region.. >> >> Thoughts on upstreamining these bits? > > Upstreaming them would be great. Patches look correct by inspection as well. > >> > Since the mvebu_pcie_setup() function seems very generic at this, >> > we should probably try to factor out that code into a common >> > helper, at least for arm64, but ideally shared with arm32 >> > as well. >> >> Yah, especially since people are not getting it completely right.. >> >> But some of the trouble here is a lack of a generic pci host driver >> structure, eg I have to pull the domain number out of the ARM32 >> specific structure .. > > I'm sure that Bjorn also has some plans of his own, but I think > we should have a generic host driver structure and gradually move > stuff into it from the architecture specific structs. I don't have plans in this area right now, but I would definitely like to migrate things like the domain, NUMA node, etc., into a generic structure. > Now there is a 'struct pci_host_bridge' that is used on some > architectures already but not on ARM. It also contains a list > of resource windows plus offsets, and there is a set of functions > associated with it: > > pci_create_root_bus() > pcibios_root_bridge_prepare() > > etc. > > Maybe all that's needed is to actually start using those on arm32? > > Arnd > -- > 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
Dear Jason Gunthorpe, On Wed, 12 Feb 2014 12:43:13 -0700, Jason Gunthorpe wrote: > diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c > index 2394e97..7fd54e9 100644 > --- a/drivers/bus/mvebu-mbus.c > +++ b/drivers/bus/mvebu-mbus.c > @@ -876,14 +876,14 @@ static void __init mvebu_mbus_get_pcie_resources(struct device_node *np, > ret = of_property_read_u32_array(np, "pcie-mem-aperture", reg, ARRAY_SIZE(reg)); > if (!ret) { > mem->start = reg[0]; > - mem->end = mem->start + reg[1]; > + mem->end = mem->start + reg[1] - 1; > mem->flags = IORESOURCE_MEM; > } > > ret = of_property_read_u32_array(np, "pcie-io-aperture", reg, ARRAY_SIZE(reg)); > if (!ret) { > io->start = reg[0]; > - io->end = io->start + reg[1]; > + io->end = io->start + reg[1] - 1; > io->flags = IORESOURCE_IO; > } > } This certainly looks good. > Fixes the wrong length (e0000000-f0000000 should be e0000000-efffffff) > > And this fixes the <BAD>: > > diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c > index ef8691a..fbb89cb 100644 > --- a/drivers/pci/host/pci-mvebu.c > +++ b/drivers/pci/host/pci-mvebu.c > @@ -109,7 +109,9 @@ struct mvebu_pcie { > struct mvebu_pcie_port *ports; > struct msi_chip *msi; > struct resource io; > + char io_name[30]; > struct resource realio; > + char mem_name[30]; > struct resource mem; > struct resource busn; > int nports; > @@ -681,10 +683,29 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys) > { > struct mvebu_pcie *pcie = sys_to_pcie(sys); > int i; > + int domain = 0; > > +#ifdef CONFIG_PCI_DOMAINS > + domain = sys->domain; > +#endif > + > + snprintf(pcie->mem_name, sizeof(pcie->mem_name), "PCI MEM %04x", domain); > + pcie->mem.name = pcie->mem_name; > + > + snprintf(pcie->io_name, sizeof(pcie->io_name), "PCI I/O %04x", domain); > + pcie->realio.name = pcie->io_name; I honestly don't know what PCI domains are, but this change looks fairly harmless to me. I would however use kasprintf() instead maybe. I can submit patches for those two changes if you want. > Still missing release_region.. To match which request_region? > > Since the mvebu_pcie_setup() function seems very generic at this, > > we should probably try to factor out that code into a common > > helper, at least for arm64, but ideally shared with arm32 > > as well. > > Yah, especially since people are not getting it completely right.. > > But some of the trouble here is a lack of a generic pci host driver > structure, eg I have to pull the domain number out of the ARM32 > specific structure .. There is indeed a good deal of duplication of PCI related code in the different architectures. The PCI_DOMAINS Kconfig option is for example replicated in multiple architectures. Best regards, Thomas
diff --git a/drivers/bus/mvebu-mbus.c b/drivers/bus/mvebu-mbus.c index 2394e97..7fd54e9 100644 --- a/drivers/bus/mvebu-mbus.c +++ b/drivers/bus/mvebu-mbus.c @@ -876,14 +876,14 @@ static void __init mvebu_mbus_get_pcie_resources(struct device_node *np, ret = of_property_read_u32_array(np, "pcie-mem-aperture", reg, ARRAY_SIZE(reg)); if (!ret) { mem->start = reg[0]; - mem->end = mem->start + reg[1]; + mem->end = mem->start + reg[1] - 1; mem->flags = IORESOURCE_MEM; } ret = of_property_read_u32_array(np, "pcie-io-aperture", reg, ARRAY_SIZE(reg)); if (!ret) { io->start = reg[0]; - io->end = io->start + reg[1]; + io->end = io->start + reg[1] - 1; io->flags = IORESOURCE_IO; } } Fixes the wrong length (e0000000-f0000000 should be e0000000-efffffff) And this fixes the <BAD>: diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c index ef8691a..fbb89cb 100644 --- a/drivers/pci/host/pci-mvebu.c +++ b/drivers/pci/host/pci-mvebu.c @@ -109,7 +109,9 @@ struct mvebu_pcie { struct mvebu_pcie_port *ports; struct msi_chip *msi; struct resource io; + char io_name[30]; struct resource realio; + char mem_name[30]; struct resource mem; struct resource busn; int nports; @@ -681,10 +683,29 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys) { struct mvebu_pcie *pcie = sys_to_pcie(sys); int i; + int domain = 0; +#ifdef CONFIG_PCI_DOMAINS + domain = sys->domain; +#endif + + snprintf(pcie->mem_name, sizeof(pcie->mem_name), "PCI MEM %04x", domain); + pcie->mem.name = pcie->mem_name; + + snprintf(pcie->io_name, sizeof(pcie->io_name), "PCI I/O %04x", domain); + pcie->realio.name = pcie->io_name; Still missing release_region.. Thoughts on upstreamining these bits? > Since the mvebu_pcie_setup() function seems very generic at this, > we should probably try to factor out that code into a common > helper, at least for arm64, but ideally shared with arm32 > as well. Yah, especially since people are not getting it completely right.. But some of the trouble here is a lack of a generic pci host driver structure, eg I have to pull the domain number out of the ARM32