| Message ID | 1415287936-31203-3-git-send-email-lorenzo.pieralisi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Thu, Nov 6, 2014 at 9:32 AM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > Most if not all ARM PCI host controller device drivers either ignore the > domain field in the pci_sys_data structure or just increment it every > time a host controller is probed, using it as a domain counter. > > Therefore, instead of relying on pci_sys_data to stash the domain number > in a standard location, ARM pcibios code can be moved to the newly > introduced generic PCI domains code, implemented in commits: > > commit 41e5c0f81d3e676d671d96a0a1fafb27abfbd9 > ("of/pci: Add pci_get_new_domain_nr() and of_get_pci_domain_nr()") > > commit 670ba0c8883b576d0aec28bd7a838358a4be1 > ("PCI: Add generic domain handling") > > In order to assign a domain number dynamically, the ARM pcibios defines > the function, called by core PCI code: > > void pci_bus_assign_domain_nr(...) > > that relies on a DT property to define the domain number or falls back to > a counter; its usage replaces the current domain assignment code in PCI > host controllers present in the kernel. I realize you are just copying the arm64 version, but as we've agreed the DT domain handling should be common and what the error cases are, I've got some comments on the current implementation. [...] > +#ifdef CONFIG_PCI_DOMAINS_GENERIC > +static bool dt_domain_found; This can be moved into the function. > + > +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) > +{ > + int domain = of_get_pci_domain_nr(parent->of_node); > + > + if (domain >= 0) { > + dt_domain_found = true; > + } else if (dt_domain_found == true) { > + dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n", > + parent->of_node->full_name); > + return; No error other than a printk? Do we want to set domain_nr to an illegal value or something? > + } else { > + domain = pci_get_new_domain_nr(); > + } > + > + bus->domain_nr = domain; > +} This doesn't handle the case where the 1st node(s) probed does not have a domain prop and a later one does. I think something like this should work: static int use_dt_domains = -1; int domain = of_get_pci_domain_nr(parent->of_node); if (domain >= 0 && use_dt_domains) { use_dt_domains = 1; } else if (domain < 0 && use_dt_domain != 1) { use_dt_domains = 0; domain = pci_get_new_domain_nr(); } else { dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" property in DT\n", parent->of_node->full_name); domain = -1; // Not sure if -1 would be illegal or not???? } bus->domain_nr = domain; Rob -- 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 Fri, Nov 07, 2014 at 04:07:30PM +0000, Rob Herring wrote: > On Thu, Nov 6, 2014 at 9:32 AM, Lorenzo Pieralisi > <lorenzo.pieralisi@arm.com> wrote: > > Most if not all ARM PCI host controller device drivers either ignore the > > domain field in the pci_sys_data structure or just increment it every > > time a host controller is probed, using it as a domain counter. > > > > Therefore, instead of relying on pci_sys_data to stash the domain number > > in a standard location, ARM pcibios code can be moved to the newly > > introduced generic PCI domains code, implemented in commits: > > > > commit 41e5c0f81d3e676d671d96a0a1fafb27abfbd9 > > ("of/pci: Add pci_get_new_domain_nr() and of_get_pci_domain_nr()") > > > > commit 670ba0c8883b576d0aec28bd7a838358a4be1 > > ("PCI: Add generic domain handling") > > > > In order to assign a domain number dynamically, the ARM pcibios defines > > the function, called by core PCI code: > > > > void pci_bus_assign_domain_nr(...) > > > > that relies on a DT property to define the domain number or falls back to > > a counter; its usage replaces the current domain assignment code in PCI > > host controllers present in the kernel. > > I realize you are just copying the arm64 version, but as we've agreed > the DT domain handling should be common and what the error cases are, > I've got some comments on the current implementation. So this means that either I patch arm64 code with your implementation below and I duplicate the code for arm too, or I have to factor out an implementation based on your code below and add it somewhere in common kernel code (drivers/pci/pci.c ? drivers/pci/of.c ?), where, it has to be seen. > > [...] > > > +#ifdef CONFIG_PCI_DOMAINS_GENERIC > > +static bool dt_domain_found; > > This can be moved into the function. Yes, I noticed while putting the patch together, I was more concerned about the removal of pci_sys_data.domain than the code parsing the domain value, which I considered agreed upon. > > + > > +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) > > +{ > > + int domain = of_get_pci_domain_nr(parent->of_node); > > + > > + if (domain >= 0) { > > + dt_domain_found = true; > > + } else if (dt_domain_found == true) { > > + dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n", > > + parent->of_node->full_name); > > + return; > > No error other than a printk? Do we want to set domain_nr to an > illegal value or something? See above. > > + } else { > > + domain = pci_get_new_domain_nr(); > > + } > > + > > + bus->domain_nr = domain; > > +} > > This doesn't handle the case where the 1st node(s) probed does not > have a domain prop and a later one does. I think something like this > should work: Well, a boolean won't do, your code below looks ok, I can add a trivial enum to make use_dt_domains usage clearer (but I think a comment to your code will do): enum { PCI_DOMAIN_INVALID, PCI_DOMAIN_DT, PCI_DOMAIN_DEFAULT }; > static int use_dt_domains = -1; > int domain = of_get_pci_domain_nr(parent->of_node); > > if (domain >= 0 && use_dt_domains) { > use_dt_domains = 1; > } else if (domain < 0 && use_dt_domain != 1) { > use_dt_domains = 0; > domain = pci_get_new_domain_nr(); > } else { > dev_err(parent, "Node %s has inconsistent \"linux,pci-domain\" > property in DT\n", > parent->of_node->full_name); > domain = -1; // Not sure if -1 would be illegal or not???? I have to check, that's a valid point, we can't certainly leave it as it is. Thanks, Lorenzo -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig index 89c4b5c..29544f0 100644 --- a/arch/arm/Kconfig +++ b/arch/arm/Kconfig @@ -1292,6 +1292,9 @@ config PCI_DOMAINS bool depends on PCI +config PCI_DOMAINS_GENERIC + def_bool PCI_DOMAINS + config PCI_NANOENGINE bool "BSE nanoEngine PCI support" depends on SA1100_NANOENGINE diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h index 7fc4278..112b770 100644 --- a/arch/arm/include/asm/mach/pci.h +++ b/arch/arm/include/asm/mach/pci.h @@ -19,9 +19,6 @@ struct pci_bus; struct device; struct hw_pci { -#ifdef CONFIG_PCI_DOMAINS - int domain; -#endif struct pci_ops *ops; int nr_controllers; void **private_data; @@ -44,9 +41,6 @@ struct hw_pci { * Per-controller structure */ struct pci_sys_data { -#ifdef CONFIG_PCI_DOMAINS - int domain; -#endif struct list_head node; int busnr; /* primary bus number */ u64 mem_offset; /* bus->cpu memory mapping offset */ diff --git a/arch/arm/include/asm/pci.h b/arch/arm/include/asm/pci.h index 7e95d85..585dc33 100644 --- a/arch/arm/include/asm/pci.h +++ b/arch/arm/include/asm/pci.h @@ -18,13 +18,6 @@ static inline int pcibios_assign_all_busses(void) } #ifdef CONFIG_PCI_DOMAINS -static inline int pci_domain_nr(struct pci_bus *bus) -{ - struct pci_sys_data *root = bus->sysdata; - - return root->domain; -} - static inline int pci_proc_domain(struct pci_bus *bus) { return pci_domain_nr(bus); diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c index 17a26c1..d8c2b4e 100644 --- a/arch/arm/kernel/bios32.c +++ b/arch/arm/kernel/bios32.c @@ -11,6 +11,8 @@ #include <linux/slab.h> #include <linux/init.h> #include <linux/io.h> +#include <linux/of.h> +#include <linux/of_pci.h> #include <asm/mach-types.h> #include <asm/mach/map.h> @@ -468,9 +470,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, if (!sys) panic("PCI: unable to allocate sys data!"); -#ifdef CONFIG_PCI_DOMAINS - sys->domain = hw->domain; -#endif sys->busnr = busnr; sys->swizzle = hw->swizzle; sys->map_irq = hw->map_irq; @@ -511,6 +510,27 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, } } +#ifdef CONFIG_PCI_DOMAINS_GENERIC +static bool dt_domain_found; + +void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) +{ + int domain = of_get_pci_domain_nr(parent->of_node); + + if (domain >= 0) { + dt_domain_found = true; + } else if (dt_domain_found == true) { + dev_err(parent, "Node %s is missing \"linux,pci-domain\" property in DT\n", + parent->of_node->full_name); + return; + } else { + domain = pci_get_new_domain_nr(); + } + + bus->domain_nr = domain; +} +#endif + void pci_common_init_dev(struct device *parent, struct hw_pci *hw) { struct pci_sys_data *sys; diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c index b1315e1..dc2ed4d 100644 --- a/drivers/pci/host/pci-mvebu.c +++ b/drivers/pci/host/pci-mvebu.c @@ -101,9 +101,7 @@ 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; @@ -722,18 +720,9 @@ 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; + pcie->mem.name = "PCI MEM"; + pcie->realio.name = "PCI I/O"; if (request_resource(&iomem_resource, &pcie->mem)) return 0; diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c index dfed00a..6790b87 100644 --- a/drivers/pci/host/pcie-designware.c +++ b/drivers/pci/host/pcie-designware.c @@ -502,9 +502,6 @@ int __init dw_pcie_host_init(struct pcie_port *pp) dw_pci.private_data = (void **)&pp; pci_common_init_dev(pp->dev, &dw_pci); -#ifdef CONFIG_PCI_DOMAINS - dw_pci.domain++; -#endif return 0; } diff --git a/drivers/pci/host/pcie-rcar.c b/drivers/pci/host/pcie-rcar.c index 61158e0..b6b859e 100644 --- a/drivers/pci/host/pcie-rcar.c +++ b/drivers/pci/host/pcie-rcar.c @@ -404,9 +404,6 @@ static void rcar_pcie_enable(struct rcar_pcie *pcie) rcar_pci.private_data = (void **)&pcie; pci_common_init_dev(&pdev->dev, &rcar_pci); -#ifdef CONFIG_PCI_DOMAINS - rcar_pci.domain++; -#endif } static int phy_wait_for_ack(struct rcar_pcie *pcie)
Most if not all ARM PCI host controller device drivers either ignore the domain field in the pci_sys_data structure or just increment it every time a host controller is probed, using it as a domain counter. Therefore, instead of relying on pci_sys_data to stash the domain number in a standard location, ARM pcibios code can be moved to the newly introduced generic PCI domains code, implemented in commits: commit 41e5c0f81d3e676d671d96a0a1fafb27abfbd9 ("of/pci: Add pci_get_new_domain_nr() and of_get_pci_domain_nr()") commit 670ba0c8883b576d0aec28bd7a838358a4be1 ("PCI: Add generic domain handling") In order to assign a domain number dynamically, the ARM pcibios defines the function, called by core PCI code: void pci_bus_assign_domain_nr(...) that relies on a DT property to define the domain number or falls back to a counter; its usage replaces the current domain assignment code in PCI host controllers present in the kernel. Cc: Arnd Bergmann <arnd@arndb.de> Cc: Phil Edworthy <phil.edworthy@renesas.com> Cc: Jason Gunthorpe <jgunthorpe@obsidianresearch.com> Cc: Jingoo Han <jg1.han@samsung.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Russell King <linux@arm.linux.org.uk> Cc: Mohit Kumar <mohit.kumar@st.com> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Tested-by: Lucas Stach <l.stach@pengutronix.de> Acked-by: Lucas Stach <l.stach@pengutronix.de> Acked-by: Jingoo Han <jg1.han@samsung.com> Acked-by: Phil Edworthy <phil.edworthy@renesas.com> Acked-by: Arnd Bergmann <arnd@arndb.de> Reviewed-by: Yijing Wang <wangyijing@huawei.com> --- arch/arm/Kconfig | 3 +++ arch/arm/include/asm/mach/pci.h | 6 ------ arch/arm/include/asm/pci.h | 7 ------- arch/arm/kernel/bios32.c | 26 +++++++++++++++++++++++--- drivers/pci/host/pci-mvebu.c | 15 ++------------- drivers/pci/host/pcie-designware.c | 3 --- drivers/pci/host/pcie-rcar.c | 3 --- 7 files changed, 28 insertions(+), 35 deletions(-)