| Message ID | 2148776.X8NPqiYA6S@wuerfel (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Wed, Oct 22, 2014 at 09:52:19PM +0100, Arnd Bergmann wrote: > On Wednesday 22 October 2014 16:59:14 Lorenzo Pieralisi wrote: > > On Wed, Oct 01, 2014 at 10:38:45AM +0100, Arnd Bergmann wrote: > > > > [...] > > > > > The arm32 implementations of pci_domain_nr/pci_proc_domain can probably be > > > removed if we change the arm32 pcibios_init_hw function to call the new > > > interfaces that set the domain number. > > > > I wished, but it is a bit more complicated than I thought unfortunately, > > mostly because some drivers, eg cns3xxx set the domain numbers > > statically in pci_sys_data and this sets a chain of dependency that is > > not easy to untangle. I think cns3xxx is the only legacy driver that "uses" > > the domain number (in pci_sys_data) in a way that clashes with the > > generic domain_nr implementation, I need to give it more thought. > > Just had a look at that driver, shouldn't be too hard to change, see below. I like this! One thing though ... > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > diff --git a/arch/arm/mach-cns3xxx/pcie.c b/arch/arm/mach-cns3xxx/pcie.c > index 45d6bd09e6ef..aa4b9d7c52fd 100644 > --- a/arch/arm/mach-cns3xxx/pcie.c > +++ b/arch/arm/mach-cns3xxx/pcie.c > @@ -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,10 @@ 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 +309,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 +329,9 @@ 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); > + hw_pci->domain = i; > + private_data = &cns3xxx_pcie[i]; Is this dance with pointers absolutely necessary? Does gcc though dishes at you for doing hw_pci->private_data = &cns3xxx_pcie[i] directly? Best regards, Liviu > + pci_common_init(&hw_pci); > } > > pci_assign_unassigned_resources(); > > > >
On Thu, Oct 23, 2014 at 10:13:09AM +0100, Liviu Dudau wrote: > On Wed, Oct 22, 2014 at 09:52:19PM +0100, Arnd Bergmann wrote: > > On Wednesday 22 October 2014 16:59:14 Lorenzo Pieralisi wrote: > > > On Wed, Oct 01, 2014 at 10:38:45AM +0100, Arnd Bergmann wrote: > > > > > > [...] > > > > > > > The arm32 implementations of pci_domain_nr/pci_proc_domain can probably be > > > > removed if we change the arm32 pcibios_init_hw function to call the new > > > > interfaces that set the domain number. > > > > > > I wished, but it is a bit more complicated than I thought unfortunately, > > > mostly because some drivers, eg cns3xxx set the domain numbers > > > statically in pci_sys_data and this sets a chain of dependency that is > > > not easy to untangle. I think cns3xxx is the only legacy driver that "uses" > > > the domain number (in pci_sys_data) in a way that clashes with the > > > generic domain_nr implementation, I need to give it more thought. > > > > Just had a look at that driver, shouldn't be too hard to change, see below. > > I like this! > > One thing though ... I like it too, it is one way of removing the artificial domain dependency from this driver. I think that by removing that, we could switch to CONFIG_PCI_DOMAINS_GENERIC on ARM32. I will remove the dependency in drivers/pci/host/pci-mvebu.c introduced by commit 2613ba48. pci_sys_data.domain is always 0 in that driver so its usefulness is doubtful, comments welcome, copied Jason in if he has comments. [...] > > @@ -323,6 +309,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 +329,9 @@ 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); > > + hw_pci->domain = i; + hw_pci.domain = i; I will remove this since if we move to generic domains it is useless to pass the value through hw_pci. > > + private_data = &cns3xxx_pcie[i]; > > Is this dance with pointers absolutely necessary? Does gcc though dishes at you > for doing hw_pci->private_data = &cns3xxx_pcie[i] directly? You can't, hw_pci.private_data is void **. 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
On Thu, Oct 23, 2014 at 12:27:31PM +0100, Lorenzo Pieralisi wrote: > I think that by removing that, we could switch to CONFIG_PCI_DOMAINS_GENERIC > on ARM32. I will remove the dependency in drivers/pci/host/pci-mvebu.c > introduced by commit 2613ba48. pci_sys_data.domain is always 0 in that > driver so its usefulness is doubtful, comments welcome, copied Jason in > if he has comments. pcie-mvebu is like all the other new drivers, each top level DT node that introduces the interface should have a unique domain number. It would be very strange (and currently unsupported by the driver) to ever have more than 1 mvebu top level node in any DT. Jason -- 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 Thu, Oct 23, 2014 at 05:52:06PM +0100, Jason Gunthorpe wrote: > On Thu, Oct 23, 2014 at 12:27:31PM +0100, Lorenzo Pieralisi wrote: > > > I think that by removing that, we could switch to CONFIG_PCI_DOMAINS_GENERIC > > on ARM32. I will remove the dependency in drivers/pci/host/pci-mvebu.c > > introduced by commit 2613ba48. pci_sys_data.domain is always 0 in that > > driver so its usefulness is doubtful, comments welcome, copied Jason in > > if he has comments. > > pcie-mvebu is like all the other new drivers, each top level DT node > that introduces the interface should have a unique domain number. It > would be very strange (and currently unsupported by the driver) to > ever have more than 1 mvebu top level node in any DT. Which as a matter of fact I should take as pci_sys_data.domain is useless on pci-mvebu.c, since that value will always be 0 (at least it is in the current driver): #ifdef CONFIG_PCI_DOMAINS domain = sys->domain; #endif Am I missing something ? Is that domain number meant to be used for anything else ? 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
On Wed, Oct 22, 2014 at 10:52:19PM +0200, Arnd Bergmann wrote: > On Wednesday 22 October 2014 16:59:14 Lorenzo Pieralisi wrote: > > On Wed, Oct 01, 2014 at 10:38:45AM +0100, Arnd Bergmann wrote: > > > > [...] > > > > > The arm32 implementations of pci_domain_nr/pci_proc_domain can probably be > > > removed if we change the arm32 pcibios_init_hw function to call the new > > > interfaces that set the domain number. > > > > I wished, but it is a bit more complicated than I thought unfortunately, > > mostly because some drivers, eg cns3xxx set the domain numbers > > statically in pci_sys_data and this sets a chain of dependency that is > > not easy to untangle. I think cns3xxx is the only legacy driver that "uses" > > the domain number (in pci_sys_data) in a way that clashes with the > > generic domain_nr implementation, I need to give it more thought. > > Just had a look at that driver, shouldn't be too hard to change, see below. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> This patch is in my patchwork, but it lacks a topic & changelog and I'm not sure of its state, so I'm going to drop it for now. Please post it again if you want me to do something with it. I guess it only touches arch/arm, so it would probably be merged via your tree anyway. Bjorn > diff --git a/arch/arm/mach-cns3xxx/pcie.c b/arch/arm/mach-cns3xxx/pcie.c > index 45d6bd09e6ef..aa4b9d7c52fd 100644 > --- a/arch/arm/mach-cns3xxx/pcie.c > +++ b/arch/arm/mach-cns3xxx/pcie.c > @@ -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,10 @@ 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 +309,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 +329,9 @@ 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); > + hw_pci->domain = i; > + private_data = &cns3xxx_pcie[i]; > + pci_common_init(&hw_pci); > } > > pci_assign_unassigned_resources(); > > -- 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 Wednesday 05 November 2014 16:39:21 Bjorn Helgaas wrote: > On Wed, Oct 22, 2014 at 10:52:19PM +0200, Arnd Bergmann wrote: > > On Wednesday 22 October 2014 16:59:14 Lorenzo Pieralisi wrote: > > > On Wed, Oct 01, 2014 at 10:38:45AM +0100, Arnd Bergmann wrote: > > > > > > [...] > > > > > > > The arm32 implementations of pci_domain_nr/pci_proc_domain can probably be > > > > removed if we change the arm32 pcibios_init_hw function to call the new > > > > interfaces that set the domain number. > > > > > > I wished, but it is a bit more complicated than I thought unfortunately, > > > mostly because some drivers, eg cns3xxx set the domain numbers > > > statically in pci_sys_data and this sets a chain of dependency that is > > > not easy to untangle. I think cns3xxx is the only legacy driver that "uses" > > > the domain number (in pci_sys_data) in a way that clashes with the > > > generic domain_nr implementation, I need to give it more thought. > > > > Just had a look at that driver, shouldn't be too hard to change, see below. > > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > This patch is in my patchwork, but it lacks a topic & changelog and I'm not > sure of its state, so I'm going to drop it for now. Please post it again > if you want me to do something with it. I guess it only touches arch/arm, > so it would probably be merged via your tree anyway. Lorenzo has posted an updated version as "arm: cns3xxx: pci: remove artificial dependency on pci_sys_data domain", and a second patch that depends on it. That is the version we should be merging, though I'm not sure through which tree. 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
On Thu, Nov 06, 2014 at 12:05:48AM +0000, Arnd Bergmann wrote: > On Wednesday 05 November 2014 16:39:21 Bjorn Helgaas wrote: > > On Wed, Oct 22, 2014 at 10:52:19PM +0200, Arnd Bergmann wrote: > > > On Wednesday 22 October 2014 16:59:14 Lorenzo Pieralisi wrote: > > > > On Wed, Oct 01, 2014 at 10:38:45AM +0100, Arnd Bergmann wrote: > > > > > > > > [...] > > > > > > > > > The arm32 implementations of pci_domain_nr/pci_proc_domain can probably be > > > > > removed if we change the arm32 pcibios_init_hw function to call the new > > > > > interfaces that set the domain number. > > > > > > > > I wished, but it is a bit more complicated than I thought unfortunately, > > > > mostly because some drivers, eg cns3xxx set the domain numbers > > > > statically in pci_sys_data and this sets a chain of dependency that is > > > > not easy to untangle. I think cns3xxx is the only legacy driver that "uses" > > > > the domain number (in pci_sys_data) in a way that clashes with the > > > > generic domain_nr implementation, I need to give it more thought. > > > > > > Just had a look at that driver, shouldn't be too hard to change, see below. > > > > > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > > > This patch is in my patchwork, but it lacks a topic & changelog and I'm not > > sure of its state, so I'm going to drop it for now. Please post it again > > if you want me to do something with it. I guess it only touches arch/arm, > > so it would probably be merged via your tree anyway. > > Lorenzo has posted an updated version as > > "arm: cns3xxx: pci: remove artificial dependency on pci_sys_data domain", > and a second patch that depends on it. That is the version we should be > merging, though I'm not sure through which tree. I am posting a v2 shortly, let's discuss the best way to merge it then. 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/mach-cns3xxx/pcie.c b/arch/arm/mach-cns3xxx/pcie.c index 45d6bd09e6ef..aa4b9d7c52fd 100644 --- a/arch/arm/mach-cns3xxx/pcie.c +++ b/arch/arm/mach-cns3xxx/pcie.c @@ -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,10 @@ 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 +309,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 +329,9 @@ 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); + hw_pci->domain = i; + private_data = &cns3xxx_pcie[i]; + pci_common_init(&hw_pci); } pci_assign_unassigned_resources();