arm: pcibios: remove pci_sys_data domain
Subsystems (4)
v2 cover arm: pcibios: remove pci_sys_data domain
This patchset is a v2 of a previous posting:
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-October/298537.html
v1 => v2 changelog
- Removed stale hw_pci domain member
- Reworked pci-mvebu domain handling according to review
- Rebased against 3.18-rc3 and updated the logs
- Dropped RFC status
Original cover letter:
----
This patchset is a first RFC stab at removing the dependency on pci_sys_data domain field on ARM platforms and by replacing it with generic code that stashes the domain value in the pci_bus control structure, introduced in
commit 41e5c0f81d3e676d671d96a0a1fafb27abfbd9
("of/pci: Add pci_get_new_domain_nr() and of_get_pci_domain_nr()")
commit 670ba0c8883b576d0aec28bd7a838358a4be1
("PCI: Add generic domain handling")
All the drivers converted (apart from PCIe designware, tested on iMX6SL) were only compile tested for lack of HW, so along some comments, testing and verifying that patchset does not break any existing platform are very appreciated.
Code in drivers/pci/pci-mvebu.c has been changed to add a domain number to PCI resources by using the nr value coming from the setup pcibios32 callback, which may not be correct and should be considered a temporary solution waiting for review comments.
The patchset removes entirely the pci_sys_data.domain field, since its usage is removed at the same time in the respective host controllers and ARM is made to select the configuration option CONFIG_PCI_DOMAINS_GENERIC by default, which compiles a domain_nr in the pci_bus structure so that the pci_domain_nr() look-up can rely on it.
Lorenzo Pieralisi (2):
arm: cns3xxx: pci: remove artificial dependency on pci_sys_data domain
arm: pcibios: move to generic PCI domains
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 ++++++++++++++++++++++--- arch/arm/mach-cns3xxx/pcie.c | 40 ++++++++++++++++---------------------- drivers/pci/host/pci-mvebu.c | 15 ++------------ drivers/pci/host/pcie-designware.c | 3 --- drivers/pci/host/pcie-rcar.c | 3 --- 8 files changed, 45 insertions(+), 58 deletions(-)
Signature · 1 line
2.1.2
PATCH v2 1/2 arm: cns3xxx: pci: remove artificial dependency on pci_sys_data domain
On cns3xxx platforms the PCI controller probing code relies on an artificial dependency on the domain number to look-up the internal data structures.
This patch reworks the host controller control data structure and adds a domain equivalent field named port in it so that the dependency on pci_sys_data domain field can be eventually removed.
--- arch/arm/mach-cns3xxx/pcie.c | 40 +++++++++++++++++----------------------- 1 file changed, 17 insertions(+), 23 deletions(-)
arch/arm/mach-cns3xxx/pcie.c +17 -23
@@ -30,18 +30,15 @@ struct cns3xxx_pcie {unsigned int irqs[2 ];struct resource res_io;struct resource res_mem; -struct hw_pci hw_pci; - +int port;bool linked; }; -static struct cns3xxx_pcie cns3xxx_pcie[];/* forward decl. */ -static struct cns3xxx_pcie *sysdata_to_cnspci (void *sysdata) {struct pci_sys_data *root = sysdata; -return &cns3xxx_pcie[root->domain]; +return root->private_data; }static struct cns3xxx_pcie *pdev_to_cnspci (const struct pci_dev *dev)
@@ -192,13 +189,7 @@ static struct cns3xxx_pcie cns3xxx_pcie[] = { .flags = IORESOURCE_MEM, }, .irqs = { IRQ_CNS3XXX_PCIE0_RC, IRQ_CNS3XXX_PCIE0_DEVICE, }, - .hw_pci = { - .domain =0 , - .nr_controllers = 1, - .ops = &cns3xxx_pcie_ops, - .setup = cns3xxx_pci_setup, - .map_irq = cns3xxx_pcie_map_irq, - }, + .port = 0, }, [1] = { .host_regs = (void __iomem *)CNS3XXX_PCIE1_HOST_BASE_VIRT,
@@ -217,19 +208,13 @@ static struct cns3xxx_pcie cns3xxx_pcie[] = { .flags = IORESOURCE_MEM, }, .irqs = { IRQ_CNS3XXX_PCIE1_RC, IRQ_CNS3XXX_PCIE1_DEVICE, }, - .hw_pci = { - .domain =1 , - .nr_controllers = 1, - .ops = &cns3xxx_pcie_ops, - .setup = cns3xxx_pci_setup, - .map_irq = cns3xxx_pcie_map_irq, - }, + .port = 1, }, };static void __init cns3xxx_pcie_check_link (struct cns3xxx_pcie *cnspci) { -int port = cnspci->hw_pci.domain; +int port = cnspci->port;u32 reg;unsigned long time;
@@ -260,9 +245,9 @@ static void __init cns3xxx_pcie_check_link(struct cns3xxx_pcie *cnspci)static void __init cns3xxx_pcie_hw_init (struct cns3xxx_pcie *cnspci) { -int port = cnspci->hw_pci.domain; +int port = cnspci->port;struct pci_sys_data sd = { - .domain = port, + .private_data = cnspci, };struct pci_bus bus = { .number =0 ,
@@ -323,6 +308,14 @@ static int cns3xxx_pcie_abort_handler(unsigned long addr, unsigned int fsr,void __init cns3xxx_pcie_init_late (void ) {int i; +void *private_data; +struct hw_pci hw_pci = { + .nr_controllers =1 , + .ops = &cns3xxx_pcie_ops, + .setup = cns3xxx_pci_setup, + .map_irq = cns3xxx_pcie_map_irq, + .private_data = &private_data, + }; pcibios_min_io =0 ; pcibios_min_mem =0 ;
@@ -335,7 +328,8 @@ void __init cns3xxx_pcie_init_late(void)cns3xxx_pwr_soft_rst (0x1 <<PM_SOFT_RST_REG_OFFST_PCIE (i));cns3xxx_pcie_check_link (&cns3xxx_pcie[i]);cns3xxx_pcie_hw_init (&cns3xxx_pcie[i]); -pci_common_init (&cns3xxx_pcie[i].hw_pci); + private_data = &cns3xxx_pcie[i]; +pci_common_init (&hw_pci); }pci_assign_unassigned_resources ();
Signature · 1 line
2.1.2
PATCH v2 1/2 arm: cns3xxx: pci: remove artificial dependency on pci_sys_data domain
Quoted 7 lines · Lorenzo Pieralisi
On cns3xxx platforms the PCI controller probing code relies on an artificial dependency on the domain number to look-up the internal data structures.
This patch reworks the host controller control data structure and adds a domain equivalent field named port in it so that the dependency on
pci_sys_datadomain field can be eventually removed.
Looks sane.
Signature · 4 lines
Krzysztof Halasa Research Institute for Automation and Measurements PIAP Al. Jerozolimskie 202, 02-486 Warsaw, Poland
PATCH v2 2/2 arm: pcibios: move to generic PCI domains
In reply to Lorenzo PieralisiMost 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.
--- 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(-)
arch/arm/Kconfig +3 -0
@@ -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
arch/arm/include/asm/mach/pci.h +0 -6
@@ -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 */
arch/arm/include/asm/pci.h +0 -7
@@ -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);
arch/arm/kernel/bios32.c +23 -3
@@ -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;
drivers/pci/host/pci-mvebu.c +2 -13
@@ -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 ;
drivers/pci/host/pcie-designware.c +0 -3
@@ -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 ; }
drivers/pci/host/pcie-rcar.c +0 -3
@@ -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)
Signature · 1 line
2.1.2
PATCH v2 2/2 arm: pcibios: move to generic PCI domains
Quoted 14 lines · Lorenzo Pieralisi
Most if not all ARM PCI host controller device drivers either ignore the domain field in the
pci_sys_datastructure or just increment it every time a host controller is probed, using it as a domain counter.Therefore, instead of relying on
pci_sys_datato 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: Addpci_get_new_domain_nr()andof_get_pci_domain_nr()")commit
670ba0c8883b576d0aec28bd7a838358a4be1
("PCI: Add generic domain handling")
Quoted 8 lines
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.
[...]
arch/arm/kernel/bios32.c static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, · line 514
Quoted 2 lines
arch/arm/kernel/bios32.c +2
-0+#ifdef CONFIG_PCI_DOMAINS_GENERIC +static bool dt_domain_found;
This can be moved into the function.
arch/arm/kernel/bios32.c static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, · line 525
Quoted 11 lines
arch/arm/kernel/bios32.c +11
-0+ +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?
arch/arm/kernel/bios32.c static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, · line 531
Quoted 6 lines
arch/arm/kernel/bios32.c +6
-0+ }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
PATCH v2 2/2 arm: pcibios: move to generic PCI domains
Quoted 21 lines · Rob Herring
Quoted 19 lines · Lorenzo Pieralisi
Most if not all ARM PCI host controller device drivers either ignore the domain field in the
pci_sys_datastructure or just increment it every time a host controller is probed, using it as a domain counter.Therefore, instead of relying on
pci_sys_datato 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: Addpci_get_new_domain_nr()andof_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(...)
Quoted 7 lines
Quoted 3 lines
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.
Quoted 7 lines
[...]
arch/arm/kernel/bios32.c static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, · line 514
Quoted 2 lines
arch/arm/kernel/bios32.c +2
-0+#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.
Quoted 6 lines
Quoted 6 lines
arch/arm/kernel/bios32.c +6
-0+ +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 ) {
Quoted 8 lines
arch/arm/kernel/bios32.c static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, · line 525
Quoted 5 lines
arch/arm/kernel/bios32.c +5
-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_nrto an illegal value or something?
See above.
Quoted 10 lines
arch/arm/kernel/bios32.c static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, · line 531
Quoted 6 lines
arch/arm/kernel/bios32.c +6
-0+ }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
};
Quoted 9 lines
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 {
Quoted 4 lines
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