| Message ID | 1411937610-22125-3-git-send-email-suravee.suthikulpanit@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Sunday 28 September 2014 15:53:28 suravee.suthikulpanit@amd.com wrote: > + > +#ifdef CONFIG_ARM64 > +struct pci_bus *gen_scan_root_bus(struct device *parent, int bus, > + struct pci_ops *ops, void *sysdata, > + struct list_head *resources) > +{ I don't see anything ARM64 specific in this, the function only uses the newly added generic helpers. > static int gen_pci_probe(struct platform_device *pdev) > { > @@ -326,6 +385,7 @@ static int gen_pci_probe(struct platform_device *pdev) > struct device *dev = &pdev->dev; > struct device_node *np = dev->of_node; > struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); > +#ifndef CONFIG_ARM64 > struct hw_pci hw = { > .nr_controllers = 1, > .private_data = (void **)&pci, > @@ -333,6 +393,7 @@ static int gen_pci_probe(struct platform_device *pdev) > .map_irq = of_irq_parse_and_map_pci, > .ops = &gen_pci_ops, > }; > +#endif > Same here, I'd suggest marking this "#ifdef CONFIG_ARM" instead, as hw_pci is an arm32 specific data structure. > if (!pci) > return -ENOMEM; > @@ -368,8 +429,24 @@ static int gen_pci_probe(struct platform_device *pdev) > gen_pci_release_of_pci_ranges(pci); > return err; > } > +#ifdef CONFIG_ARM64 > > +#ifdef CONFIG_PCI_MSI > + pci->msi_parent = of_parse_phandle(np, "msi-parent", 0); > + if (!pci->msi_parent) { > + dev_err(&pdev->dev, "Failed to allocate msi-parent.\n"); > + return -EINVAL; > + } > +#endif We probably want to enable MSI unconditionally on ARM64, so the #ifdef is not necessary here. However, I don't think that a missing msi-parent should be a fatal error here: we can still continue without MSI support if this case. > + if (!gen_scan_root_bus(&pdev->dev, pci->cfg.bus_range.start, > + &gen_pci_ops, pci, &pci->resources)) { > + dev_err(&pdev->dev, "failed to enable PCIe ports\n"); > + return -ENODEV; > + } > +#else > pci_common_init_dev(dev, &hw); > +#endif /* CONFIG_ARM64 */ > Again, just make the pci_common_init_dev() call #ifdef CONFIG_ARM, and move the generic case after it, outside of the #ifdef. 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
How does the res->parent for devices of non-"PCI_PROBE_ONLY" controller gets set with this patch ? On Mon, Sep 29, 2014 at 2:23 AM, <suravee.suthikulpanit@amd.com> wrote: > From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > > This patch adds ARM64 support to the generic PCI host driver. > > For MSI support, it adds new device tree binding "msi-parent", > which should point to corresponded msi-controller. > > Cc: Will Deacon <will.deacon@arm.com> > Cc: Liviu Dudau <Liviu.Dudau@arm.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> > --- > .../devicetree/bindings/pci/host-generic-pci.txt | 3 + > drivers/pci/host/Kconfig | 2 +- > drivers/pci/host/pci-host-generic.c | 95 ++++++++++++++++++++-- > 3 files changed, 90 insertions(+), 10 deletions(-) > > diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.txt b/Documentation/devicetree/bindings/pci/host-generic-pci.txt > index f0b0436..327e5b1 100644 > --- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt > +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt > @@ -69,6 +69,9 @@ Practice: Interrupt Mapping' and requires the following properties: > > - interrupt-map-mask : <see aforementioned specification> > > +Optinal Properties: > + > +- msi-parent : Specify the msi-controller phandle. > > Example: > > diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig > index 90f5cca..44bf523 100644 > --- a/drivers/pci/host/Kconfig > +++ b/drivers/pci/host/Kconfig > @@ -50,7 +50,7 @@ config PCI_RCAR_GEN2_PCIE > > config PCI_HOST_GENERIC > bool "Generic PCI host controller" > - depends on ARM && OF > + depends on (ARM || ARM64) && OF > help > Say Y here if you want to support a simple generic PCI host > controller, such as the one emulated by kvmtool. > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c > index 3d2076f..f33c547 100644 > --- a/drivers/pci/host/pci-host-generic.c > +++ b/drivers/pci/host/pci-host-generic.c > @@ -42,14 +42,24 @@ struct gen_pci { > struct pci_host_bridge host; > struct gen_pci_cfg_windows cfg; > struct list_head resources; > + struct device_node *msi_parent; > }; > > +#ifdef CONFIG_ARM64 > +#define bus_to_gen_pci(b) \ > + ((struct gen_pci *)b->sysdata) > +#else > +#define bus_to_gen_pci(b) \ > + ((struct gen_pci *) \ > + (((struct pci_sys_data *) \ > + (bus->sysdata))->private_data)) > +#endif > + > static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus, > unsigned int devfn, > int where) > { > - struct pci_sys_data *sys = bus->sysdata; > - struct gen_pci *pci = sys->private_data; > + struct gen_pci *pci = bus_to_gen_pci(bus); > resource_size_t idx = bus->number - pci->cfg.bus_range.start; > > return pci->cfg.win[idx] + ((devfn << 8) | where); > @@ -64,8 +74,7 @@ static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus, > unsigned int devfn, > int where) > { > - struct pci_sys_data *sys = bus->sysdata; > - struct gen_pci *pci = sys->private_data; > + struct gen_pci *pci = bus_to_gen_pci(bus); > resource_size_t idx = bus->number - pci->cfg.bus_range.start; > > return pci->cfg.win[idx] + ((devfn << 12) | where); > @@ -80,8 +89,7 @@ static int gen_pci_config_read(struct pci_bus *bus, unsigned int devfn, > int where, int size, u32 *val) > { > void __iomem *addr; > - struct pci_sys_data *sys = bus->sysdata; > - struct gen_pci *pci = sys->private_data; > + struct gen_pci *pci = bus_to_gen_pci(bus); > > addr = pci->cfg.ops->map_bus(bus, devfn, where); > > @@ -103,8 +111,7 @@ static int gen_pci_config_write(struct pci_bus *bus, unsigned int devfn, > int where, int size, u32 val) > { > void __iomem *addr; > - struct pci_sys_data *sys = bus->sysdata; > - struct gen_pci *pci = sys->private_data; > + struct gen_pci *pci = bus_to_gen_pci(bus); > > addr = pci->cfg.ops->map_bus(bus, devfn, where); > > @@ -144,8 +151,11 @@ static int gen_pci_calc_io_offset(struct device *dev, > resource_size_t *offset) > { > static atomic_t wins = ATOMIC_INIT(0); > - int err, idx, max_win; > + int idx, max_win; > unsigned int window; > +#ifndef CONFIG_ARM64 > + int err; > +#endif > > if (!PAGE_ALIGNED(range->cpu_addr)) > return -EINVAL; > @@ -156,9 +166,12 @@ static int gen_pci_calc_io_offset(struct device *dev, > return -ENOSPC; > > window = (idx - 1) * SZ_64K; > + > +#ifndef CONFIG_ARM64 > err = pci_ioremap_io(window, range->cpu_addr); > if (err) > return err; > +#endif > > of_pci_range_to_resource(range, dev->of_node, res); > res->start = window; > @@ -310,12 +323,58 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) > return 0; > } > > +#ifndef CONFIG_ARM64 > static int gen_pci_setup(int nr, struct pci_sys_data *sys) > { > struct gen_pci *pci = sys->private_data; > list_splice_init(&pci->resources, &sys->resources); > return 1; > } > +#endif > + > +#ifdef CONFIG_ARM64 > +struct pci_bus *gen_scan_root_bus(struct device *parent, int bus, > + struct pci_ops *ops, void *sysdata, > + struct list_head *resources) > +{ > + struct pci_host_bridge_window *window; > + bool found = false; > + struct pci_bus *b; > + int max; > + struct gen_pci *pci = sysdata; > + > + list_for_each_entry(window, resources, list) > + if (window->res->flags & IORESOURCE_BUS) { > + found = true; > + break; > + } > + > + b = pci_create_root_bus(parent, bus, ops, sysdata, resources); > + if (!b) > + return NULL; > + > + /* TODO: > + * This is probably should be done in the core pci driver somewhere > + */ > + if (pci->msi_parent) > + b->msi = of_pci_find_msi_chip_by_node(pci->msi_parent); > + > + 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); > + } > + > + max = pci_scan_child_bus(b); > + > + if (!found) > + pci_bus_update_busn_res_end(b, max); > + > + pci_bus_add_devices(b); > + return b; > +} > +#endif > > static int gen_pci_probe(struct platform_device *pdev) > { > @@ -326,6 +385,7 @@ static int gen_pci_probe(struct platform_device *pdev) > struct device *dev = &pdev->dev; > struct device_node *np = dev->of_node; > struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); > +#ifndef CONFIG_ARM64 > struct hw_pci hw = { > .nr_controllers = 1, > .private_data = (void **)&pci, > @@ -333,6 +393,7 @@ static int gen_pci_probe(struct platform_device *pdev) > .map_irq = of_irq_parse_and_map_pci, > .ops = &gen_pci_ops, > }; > +#endif > > if (!pci) > return -ENOMEM; > @@ -368,8 +429,24 @@ static int gen_pci_probe(struct platform_device *pdev) > gen_pci_release_of_pci_ranges(pci); > return err; > } > +#ifdef CONFIG_ARM64 > > +#ifdef CONFIG_PCI_MSI > + pci->msi_parent = of_parse_phandle(np, "msi-parent", 0); > + if (!pci->msi_parent) { > + dev_err(&pdev->dev, "Failed to allocate msi-parent.\n"); > + return -EINVAL; > + } > +#endif > + > + if (!gen_scan_root_bus(&pdev->dev, pci->cfg.bus_range.start, > + &gen_pci_ops, pci, &pci->resources)) { > + dev_err(&pdev->dev, "failed to enable PCIe ports\n"); > + return -ENODEV; > + } > +#else > pci_common_init_dev(dev, &hw); > +#endif /* CONFIG_ARM64 */ > return 0; > } > > -- > 1.9.3 > > -- > 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 -- 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 Mon, Sep 29, 2014 at 03:36:30PM +0100, Arnd Bergmann wrote: > On Sunday 28 September 2014 15:53:28 suravee.suthikulpanit@amd.com wrote: > > + > > +#ifdef CONFIG_ARM64 > > +struct pci_bus *gen_scan_root_bus(struct device *parent, int bus, > > + struct pci_ops *ops, void *sysdata, > > + struct list_head *resources) > > +{ > > I don't see anything ARM64 specific in this, the function only uses > the newly added generic helpers. I do not see it either, it is just a copy'n'paste of common PCI code, plus msi management. > > static int gen_pci_probe(struct platform_device *pdev) > > { > > @@ -326,6 +385,7 @@ static int gen_pci_probe(struct platform_device *pdev) > > struct device *dev = &pdev->dev; > > struct device_node *np = dev->of_node; > > struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); > > +#ifndef CONFIG_ARM64 > > struct hw_pci hw = { > > .nr_controllers = 1, > > .private_data = (void **)&pci, > > @@ -333,6 +393,7 @@ static int gen_pci_probe(struct platform_device *pdev) > > .map_irq = of_irq_parse_and_map_pci, > > .ops = &gen_pci_ops, > > }; > > +#endif > > > > Same here, I'd suggest marking this "#ifdef CONFIG_ARM" instead, as hw_pci > is an arm32 specific data structure. I do not think we need hw struct at all, see below, we can write code so that we do not rely on ARM32 PCI bios, I will have a stab at that and post the resulting code. > > if (!pci) > > return -ENOMEM; > > @@ -368,8 +429,24 @@ static int gen_pci_probe(struct platform_device *pdev) > > gen_pci_release_of_pci_ranges(pci); > > return err; > > } > > +#ifdef CONFIG_ARM64 > > > > +#ifdef CONFIG_PCI_MSI > > + pci->msi_parent = of_parse_phandle(np, "msi-parent", 0); > > + if (!pci->msi_parent) { > > + dev_err(&pdev->dev, "Failed to allocate msi-parent.\n"); > > + return -EINVAL; > > + } > > +#endif > > We probably want to enable MSI unconditionally on ARM64, so the #ifdef is > not necessary here. However, I don't think that a missing msi-parent should > be a fatal error here: we can still continue without MSI support if this > case. > > > + if (!gen_scan_root_bus(&pdev->dev, pci->cfg.bus_range.start, > > + &gen_pci_ops, pci, &pci->resources)) { > > + dev_err(&pdev->dev, "failed to enable PCIe ports\n"); > > + return -ENODEV; > > + } > > +#else > > pci_common_init_dev(dev, &hw); > > +#endif /* CONFIG_ARM64 */ > > > > Again, just make the pci_common_init_dev() call #ifdef CONFIG_ARM, and move > the generic case after it, outside of the #ifdef. I went through the code quickly but I think we can (and should) remove this quite ugly ifdeffery altogether. Most of the functionality in pci_common_init_dev() can be implemented through the common PCI API (and this would make this driver arch agnostic as it should be), I will go through ARM32 PCI bios code to check what is executed in detail in pci_common_init_dev() and make sure that we follow those initialization steps in the resulting probe code for this PCI generic host controller driver. 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 Tuesday 30 September 2014 13:03:44 Lorenzo Pieralisi wrote: > > > static int gen_pci_probe(struct platform_device *pdev) > > > { > > > @@ -326,6 +385,7 @@ static int gen_pci_probe(struct platform_device *pdev) > > > struct device *dev = &pdev->dev; > > > struct device_node *np = dev->of_node; > > > struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); > > > +#ifndef CONFIG_ARM64 > > > struct hw_pci hw = { > > > .nr_controllers = 1, > > > .private_data = (void **)&pci, > > > @@ -333,6 +393,7 @@ static int gen_pci_probe(struct platform_device *pdev) > > > .map_irq = of_irq_parse_and_map_pci, > > > .ops = &gen_pci_ops, > > > }; > > > +#endif > > > > > > > Same here, I'd suggest marking this "#ifdef CONFIG_ARM" instead, as hw_pci > > is an arm32 specific data structure. > > I do not think we need hw struct at all, see below, we can write code so > that we do not rely on ARM32 PCI bios, I will have a stab at that and > post the resulting code. That would of course be best. I think it needs some rework of the arm32 PCI code though, or you'd still have to create pci_sys_data manually, and that is currently allocated by pcibios_init_hw. > > > + if (!gen_scan_root_bus(&pdev->dev, pci->cfg.bus_range.start, > > > + &gen_pci_ops, pci, &pci->resources)) { > > > + dev_err(&pdev->dev, "failed to enable PCIe ports\n"); > > > + return -ENODEV; > > > + } > > > +#else > > > pci_common_init_dev(dev, &hw); > > > +#endif /* CONFIG_ARM64 */ > > > > > > > Again, just make the pci_common_init_dev() call #ifdef CONFIG_ARM, and move > > the generic case after it, outside of the #ifdef. > > I went through the code quickly but I think we can (and should) remove > this quite ugly ifdeffery altogether. Most of the functionality in > pci_common_init_dev() can be implemented through the common PCI API (and this > would make this driver arch agnostic as it should be), I will go through ARM32 > PCI bios code to check what is executed in detail in pci_common_init_dev() and > make sure that we follow those initialization steps in the resulting probe code > for this PCI generic host controller driver. These are the functions I found that refer to pci_sys_data on arm32: pcibios_add_bus pcibios_remove_bus pcibios_align_resource pci_mmap_page_range pci_domain_nr pci_proc_domain This is not as bad as I had feared, but we still have to ensure that any caller of these functions will work with both the generic PCI support and the arm32 specific drivers that today use hw_pci. My idea for dealing with this was to convert all host drivers in drivers/pci/host to the generic PCI code and never build the arm32 bios32 code when CONFIG_ARCH_MULTIPLATFORM is set. Unfortunately that requires either doing them all at once or coming up with a migration strategy so we don't break things in the process. Note that arch/arm/mach-cns3xxx/pcie.c also belongs in the drivers/pci/host category and should probably be moved there. The integrator and versatile PCI drivers are currently not used with MULTIPLATFORM but will at some point and we can convert them as we get there. 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 Tue, Sep 30, 2014 at 01:31:44PM +0100, Arnd Bergmann wrote: > On Tuesday 30 September 2014 13:03:44 Lorenzo Pieralisi wrote: > > > > static int gen_pci_probe(struct platform_device *pdev) > > > > { > > > > @@ -326,6 +385,7 @@ static int gen_pci_probe(struct platform_device *pdev) > > > > struct device *dev = &pdev->dev; > > > > struct device_node *np = dev->of_node; > > > > struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); > > > > +#ifndef CONFIG_ARM64 > > > > struct hw_pci hw = { > > > > .nr_controllers = 1, > > > > .private_data = (void **)&pci, > > > > @@ -333,6 +393,7 @@ static int gen_pci_probe(struct platform_device *pdev) > > > > .map_irq = of_irq_parse_and_map_pci, > > > > .ops = &gen_pci_ops, > > > > }; > > > > +#endif > > > > > > > > > > Same here, I'd suggest marking this "#ifdef CONFIG_ARM" instead, as hw_pci > > > is an arm32 specific data structure. > > > > I do not think we need hw struct at all, see below, we can write code so > > that we do not rely on ARM32 PCI bios, I will have a stab at that and > > post the resulting code. > > That would of course be best. I think it needs some rework of the > arm32 PCI code though, or you'd still have to create pci_sys_data > manually, and that is currently allocated by pcibios_init_hw. Right, as far as I can see, creating a pci_sys_data struct that's all we would need. "Problem" is that it does not exist on ARM64 so to avoid ifdeffery we have to declare a struct with the same fields (ie only pci_sys_data.private_data is used by this driver - apart from arm32 specific functions usage) that is passed to the PCI layer and stored in the bus.sysdata, but that's extremely ugly (and we won't need this when the arm32 conversion is completed). > > > > + if (!gen_scan_root_bus(&pdev->dev, pci->cfg.bus_range.start, > > > > + &gen_pci_ops, pci, &pci->resources)) { > > > > + dev_err(&pdev->dev, "failed to enable PCIe ports\n"); > > > > + return -ENODEV; > > > > + } > > > > +#else > > > > pci_common_init_dev(dev, &hw); > > > > +#endif /* CONFIG_ARM64 */ > > > > > > > > > > Again, just make the pci_common_init_dev() call #ifdef CONFIG_ARM, and move > > > the generic case after it, outside of the #ifdef. > > > > I went through the code quickly but I think we can (and should) remove > > this quite ugly ifdeffery altogether. Most of the functionality in > > pci_common_init_dev() can be implemented through the common PCI API (and this > > would make this driver arch agnostic as it should be), I will go through ARM32 > > PCI bios code to check what is executed in detail in pci_common_init_dev() and > > make sure that we follow those initialization steps in the resulting probe code > > for this PCI generic host controller driver. > > These are the functions I found that refer to pci_sys_data on arm32: > > pcibios_add_bus > pcibios_remove_bus > pcibios_align_resource > pci_mmap_page_range > pci_domain_nr > pci_proc_domain > > This is not as bad as I had feared, but we still have to ensure that > any caller of these functions will work with both the generic PCI support > and the arm32 specific drivers that today use hw_pci. > > My idea for dealing with this was to convert all host drivers in > drivers/pci/host to the generic PCI code and never build the arm32 > bios32 code when CONFIG_ARCH_MULTIPLATFORM is set. Unfortunately that > requires either doing them all at once or coming up with a migration > strategy so we don't break things in the process. That makes sense. Related to the migration strategy, thoughts appreciated. Declaring a static pci_sys_data (with some ifdef around it) seems a horrible hack to me. Calling pci_common_init() only if CONFIG_ARM is rather horrible too, but we can probably live with that. I do not see anything else as possible solution at the moment unless we go the whole nine yards and do what you suggest above, might take a little while though. Probably leaving pci_common_init() call (and related hw_pci struct, and related ifdeffery to differentiate between different sysdata layouts for ARM and ARM64) is the fastest path but I still think it is not nice at all. 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 Tue, Sep 30, 2014 at 05:12:41PM +0100, Lorenzo Pieralisi wrote: > On Tue, Sep 30, 2014 at 01:31:44PM +0100, Arnd Bergmann wrote: > > On Tuesday 30 September 2014 13:03:44 Lorenzo Pieralisi wrote: > > > > > static int gen_pci_probe(struct platform_device *pdev) > > > > > { > > > > > @@ -326,6 +385,7 @@ static int gen_pci_probe(struct platform_device *pdev) > > > > > struct device *dev = &pdev->dev; > > > > > struct device_node *np = dev->of_node; > > > > > struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); > > > > > +#ifndef CONFIG_ARM64 > > > > > struct hw_pci hw = { > > > > > .nr_controllers = 1, > > > > > .private_data = (void **)&pci, > > > > > @@ -333,6 +393,7 @@ static int gen_pci_probe(struct platform_device *pdev) > > > > > .map_irq = of_irq_parse_and_map_pci, > > > > > .ops = &gen_pci_ops, > > > > > }; > > > > > +#endif > > > > > > > > > > > > > Same here, I'd suggest marking this "#ifdef CONFIG_ARM" instead, as hw_pci > > > > is an arm32 specific data structure. > > > > > > I do not think we need hw struct at all, see below, we can write code so > > > that we do not rely on ARM32 PCI bios, I will have a stab at that and > > > post the resulting code. > > > > That would of course be best. I think it needs some rework of the > > arm32 PCI code though, or you'd still have to create pci_sys_data > > manually, and that is currently allocated by pcibios_init_hw. I don't see why we need to involve the arm32 code here at all. A host bridge can be fully functional with the generic code without having to use any of the arm32 code (unless I'm missing something here). > > Right, as far as I can see, creating a pci_sys_data struct > that's all we would need. "Problem" is that it does not exist on ARM64 > so to avoid ifdeffery we have to declare a struct with the same > fields (ie only pci_sys_data.private_data is used by this driver - > apart from arm32 specific functions usage) that is passed to the PCI layer > and stored in the bus.sysdata, but that's extremely ugly (and we won't > need this when the arm32 conversion is completed). > > > > > > + if (!gen_scan_root_bus(&pdev->dev, pci->cfg.bus_range.start, > > > > > + &gen_pci_ops, pci, &pci->resources)) { > > > > > + dev_err(&pdev->dev, "failed to enable PCIe ports\n"); > > > > > + return -ENODEV; > > > > > + } > > > > > +#else > > > > > pci_common_init_dev(dev, &hw); > > > > > +#endif /* CONFIG_ARM64 */ > > > > > > > > > > > > > Again, just make the pci_common_init_dev() call #ifdef CONFIG_ARM, and move > > > > the generic case after it, outside of the #ifdef. > > > > > > I went through the code quickly but I think we can (and should) remove > > > this quite ugly ifdeffery altogether. Most of the functionality in > > > pci_common_init_dev() can be implemented through the common PCI API (and this > > > would make this driver arch agnostic as it should be), I will go through ARM32 > > > PCI bios code to check what is executed in detail in pci_common_init_dev() and > > > make sure that we follow those initialization steps in the resulting probe code > > > for this PCI generic host controller driver. > > > > These are the functions I found that refer to pci_sys_data on arm32: > > > > pcibios_add_bus > > pcibios_remove_bus > > pcibios_align_resource > > pci_mmap_page_range > > pci_domain_nr > > pci_proc_domain > > > > This is not as bad as I had feared, but we still have to ensure that > > any caller of these functions will work with both the generic PCI support > > and the arm32 specific drivers that today use hw_pci. > > > > My idea for dealing with this was to convert all host drivers in > > drivers/pci/host to the generic PCI code and never build the arm32 > > bios32 code when CONFIG_ARCH_MULTIPLATFORM is set. Unfortunately that > > requires either doing them all at once or coming up with a migration > > strategy so we don't break things in the process. > > That makes sense. Related to the migration strategy, thoughts > appreciated. Declaring a static pci_sys_data (with some ifdef around it) > seems a horrible hack to me. Calling pci_common_init() only if CONFIG_ARM > is rather horrible too, but we can probably live with that. > > I do not see anything else as possible solution at the moment unless > we go the whole nine yards and do what you suggest above, might take a > little while though. > > Probably leaving pci_common_init() call (and related hw_pci struct, and > related ifdeffery to differentiate between different sysdata layouts for ARM > and ARM64) is the fastest path but I still think it is not nice at all. Rob Herring found the conversion of mach-integrator/pci_v3.c to the generic framework quite painless. We might have to go through a lot of testing, but I don't see the process to be too horrendous. That being said, I think we first need to make sure we have all the features needed by the host bridge drivers in place before we start the conversion. MSI support is amongst them. Best regards, Liviu > > Thanks, > Lorenzo
On Tue, Sep 30, 2014 at 05:42:56PM +0100, Liviu Dudau wrote: > On Tue, Sep 30, 2014 at 05:12:41PM +0100, Lorenzo Pieralisi wrote: > > On Tue, Sep 30, 2014 at 01:31:44PM +0100, Arnd Bergmann wrote: > > > On Tuesday 30 September 2014 13:03:44 Lorenzo Pieralisi wrote: > > > > > > static int gen_pci_probe(struct platform_device *pdev) > > > > > > { > > > > > > @@ -326,6 +385,7 @@ static int gen_pci_probe(struct platform_device *pdev) > > > > > > struct device *dev = &pdev->dev; > > > > > > struct device_node *np = dev->of_node; > > > > > > struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); > > > > > > +#ifndef CONFIG_ARM64 > > > > > > struct hw_pci hw = { > > > > > > .nr_controllers = 1, > > > > > > .private_data = (void **)&pci, > > > > > > @@ -333,6 +393,7 @@ static int gen_pci_probe(struct platform_device *pdev) > > > > > > .map_irq = of_irq_parse_and_map_pci, > > > > > > .ops = &gen_pci_ops, > > > > > > }; > > > > > > +#endif > > > > > > > > > > > > > > > > Same here, I'd suggest marking this "#ifdef CONFIG_ARM" instead, as hw_pci > > > > > is an arm32 specific data structure. > > > > > > > > I do not think we need hw struct at all, see below, we can write code so > > > > that we do not rely on ARM32 PCI bios, I will have a stab at that and > > > > post the resulting code. > > > > > > That would of course be best. I think it needs some rework of the > > > arm32 PCI code though, or you'd still have to create pci_sys_data > > > manually, and that is currently allocated by pcibios_init_hw. > > I don't see why we need to involve the arm32 code here at all. A host bridge can > be fully functional with the generic code without having to use any of the > arm32 code (unless I'm missing something here). Ok so I can remove the pci_common_init() call, use the common PCI API and everything will work as expected, even if there is a list of functions (see below) that *require* pci_sys_data to exist (and that's allocated in arm32 pcibios code in pcibios_init_hw(), called from pci_common_init()) ? I like the idea but I think that's optimistic, or at least we did not trigger the code paths that can cause issues. > > Right, as far as I can see, creating a pci_sys_data struct > > that's all we would need. "Problem" is that it does not exist on ARM64 > > so to avoid ifdeffery we have to declare a struct with the same > > fields (ie only pci_sys_data.private_data is used by this driver - > > apart from arm32 specific functions usage) that is passed to the PCI layer > > and stored in the bus.sysdata, but that's extremely ugly (and we won't > > need this when the arm32 conversion is completed). > > > > > > > > + if (!gen_scan_root_bus(&pdev->dev, pci->cfg.bus_range.start, > > > > > > + &gen_pci_ops, pci, &pci->resources)) { > > > > > > + dev_err(&pdev->dev, "failed to enable PCIe ports\n"); > > > > > > + return -ENODEV; > > > > > > + } > > > > > > +#else > > > > > > pci_common_init_dev(dev, &hw); > > > > > > +#endif /* CONFIG_ARM64 */ > > > > > > > > > > > > > > > > Again, just make the pci_common_init_dev() call #ifdef CONFIG_ARM, and move > > > > > the generic case after it, outside of the #ifdef. > > > > > > > > I went through the code quickly but I think we can (and should) remove > > > > this quite ugly ifdeffery altogether. Most of the functionality in > > > > pci_common_init_dev() can be implemented through the common PCI API (and this > > > > would make this driver arch agnostic as it should be), I will go through ARM32 > > > > PCI bios code to check what is executed in detail in pci_common_init_dev() and > > > > make sure that we follow those initialization steps in the resulting probe code > > > > for this PCI generic host controller driver. > > > > > > These are the functions I found that refer to pci_sys_data on arm32: > > > > > > pcibios_add_bus > > > pcibios_remove_bus > > > pcibios_align_resource > > > pci_mmap_page_range > > > pci_domain_nr > > > pci_proc_domain > > > > > > This is not as bad as I had feared, but we still have to ensure that > > > any caller of these functions will work with both the generic PCI support > > > and the arm32 specific drivers that today use hw_pci. > > > > > > My idea for dealing with this was to convert all host drivers in > > > drivers/pci/host to the generic PCI code and never build the arm32 > > > bios32 code when CONFIG_ARCH_MULTIPLATFORM is set. Unfortunately that > > > requires either doing them all at once or coming up with a migration > > > strategy so we don't break things in the process. > > > > That makes sense. Related to the migration strategy, thoughts > > appreciated. Declaring a static pci_sys_data (with some ifdef around it) > > seems a horrible hack to me. Calling pci_common_init() only if CONFIG_ARM > > is rather horrible too, but we can probably live with that. > > > > I do not see anything else as possible solution at the moment unless > > we go the whole nine yards and do what you suggest above, might take a > > little while though. > > > > Probably leaving pci_common_init() call (and related hw_pci struct, and > > related ifdeffery to differentiate between different sysdata layouts for ARM > > and ARM64) is the fastest path but I still think it is not nice at all. > > Rob Herring found the conversion of mach-integrator/pci_v3.c to the generic > framework quite painless. We might have to go through a lot of testing, but I don't > see the process to be too horrendous. See my comments above, I have not said that the conversion is complicated, what I am saying is that I am not sure we can get rid of pcibios code calls yet, as Arnd pointed out. 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 Tue, Sep 30, 2014 at 06:35:40PM +0100, Lorenzo Pieralisi wrote: > On Tue, Sep 30, 2014 at 05:42:56PM +0100, Liviu Dudau wrote: > > On Tue, Sep 30, 2014 at 05:12:41PM +0100, Lorenzo Pieralisi wrote: > > > On Tue, Sep 30, 2014 at 01:31:44PM +0100, Arnd Bergmann wrote: > > > > On Tuesday 30 September 2014 13:03:44 Lorenzo Pieralisi wrote: > > > > > > > static int gen_pci_probe(struct platform_device *pdev) > > > > > > > { > > > > > > > @@ -326,6 +385,7 @@ static int gen_pci_probe(struct platform_device *pdev) > > > > > > > struct device *dev = &pdev->dev; > > > > > > > struct device_node *np = dev->of_node; > > > > > > > struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); > > > > > > > +#ifndef CONFIG_ARM64 > > > > > > > struct hw_pci hw = { > > > > > > > .nr_controllers = 1, > > > > > > > .private_data = (void **)&pci, > > > > > > > @@ -333,6 +393,7 @@ static int gen_pci_probe(struct platform_device *pdev) > > > > > > > .map_irq = of_irq_parse_and_map_pci, > > > > > > > .ops = &gen_pci_ops, > > > > > > > }; > > > > > > > +#endif > > > > > > > > > > > > > > > > > > > Same here, I'd suggest marking this "#ifdef CONFIG_ARM" instead, as hw_pci > > > > > > is an arm32 specific data structure. > > > > > > > > > > I do not think we need hw struct at all, see below, we can write code so > > > > > that we do not rely on ARM32 PCI bios, I will have a stab at that and > > > > > post the resulting code. > > > > > > > > That would of course be best. I think it needs some rework of the > > > > arm32 PCI code though, or you'd still have to create pci_sys_data > > > > manually, and that is currently allocated by pcibios_init_hw. > > > > I don't see why we need to involve the arm32 code here at all. A host bridge can > > be fully functional with the generic code without having to use any of the > > arm32 code (unless I'm missing something here). > > Ok so I can remove the pci_common_init() call, use the common PCI API and > everything will work as expected, even if there is a list of functions (see > below) that *require* pci_sys_data to exist (and that's allocated in arm32 > pcibios code in pcibios_init_hw(), called from pci_common_init()) ? See bellow my comments on those functions. > > I like the idea but I think that's optimistic, or at least we did not > trigger the code paths that can cause issues. > > > > Right, as far as I can see, creating a pci_sys_data struct > > > that's all we would need. "Problem" is that it does not exist on ARM64 > > > so to avoid ifdeffery we have to declare a struct with the same > > > fields (ie only pci_sys_data.private_data is used by this driver - > > > apart from arm32 specific functions usage) that is passed to the PCI layer > > > and stored in the bus.sysdata, but that's extremely ugly (and we won't > > > need this when the arm32 conversion is completed). > > > > > > > > > > + if (!gen_scan_root_bus(&pdev->dev, pci->cfg.bus_range.start, > > > > > > > + &gen_pci_ops, pci, &pci->resources)) { > > > > > > > + dev_err(&pdev->dev, "failed to enable PCIe ports\n"); > > > > > > > + return -ENODEV; > > > > > > > + } > > > > > > > +#else > > > > > > > pci_common_init_dev(dev, &hw); > > > > > > > +#endif /* CONFIG_ARM64 */ > > > > > > > > > > > > > > > > > > > Again, just make the pci_common_init_dev() call #ifdef CONFIG_ARM, and move > > > > > > the generic case after it, outside of the #ifdef. > > > > > > > > > > I went through the code quickly but I think we can (and should) remove > > > > > this quite ugly ifdeffery altogether. Most of the functionality in > > > > > pci_common_init_dev() can be implemented through the common PCI API (and this > > > > > would make this driver arch agnostic as it should be), I will go through ARM32 > > > > > PCI bios code to check what is executed in detail in pci_common_init_dev() and > > > > > make sure that we follow those initialization steps in the resulting probe code > > > > > for this PCI generic host controller driver. > > > > > > > > These are the functions I found that refer to pci_sys_data on arm32: > > > > > > > > pcibios_add_bus > > > > pcibios_remove_bus These are only needed if you want to do per HB processing of the bus > > > > pcibios_align_resource mvebu is the only user of this function. > > > > pci_mmap_page_range This is only needed when mapping a PCI resource to userspace. Is that your case here? > > > > pci_domain_nr > > > > pci_proc_domain We have equivalent functionality in the generic patches for those. Best regards, Liviu > > > > > > > > This is not as bad as I had feared, but we still have to ensure that > > > > any caller of these functions will work with both the generic PCI support > > > > and the arm32 specific drivers that today use hw_pci. > > > > > > > > My idea for dealing with this was to convert all host drivers in > > > > drivers/pci/host to the generic PCI code and never build the arm32 > > > > bios32 code when CONFIG_ARCH_MULTIPLATFORM is set. Unfortunately that > > > > requires either doing them all at once or coming up with a migration > > > > strategy so we don't break things in the process. > > > > > > That makes sense. Related to the migration strategy, thoughts > > > appreciated. Declaring a static pci_sys_data (with some ifdef around it) > > > seems a horrible hack to me. Calling pci_common_init() only if CONFIG_ARM > > > is rather horrible too, but we can probably live with that. > > > > > > I do not see anything else as possible solution at the moment unless > > > we go the whole nine yards and do what you suggest above, might take a > > > little while though. > > > > > > Probably leaving pci_common_init() call (and related hw_pci struct, and > > > related ifdeffery to differentiate between different sysdata layouts for ARM > > > and ARM64) is the fastest path but I still think it is not nice at all. > > > > Rob Herring found the conversion of mach-integrator/pci_v3.c to the generic > > framework quite painless. We might have to go through a lot of testing, but I don't > > see the process to be too horrendous. > > See my comments above, I have not said that the conversion is complicated, > what I am saying is that I am not sure we can get rid of pcibios code calls > yet, as Arnd pointed out. > > Lorenzo
On Tuesday 30 September 2014 18:48:21 Liviu Dudau wrote: > > > > > These are the functions I found that refer to pci_sys_data on arm32: > > > > > > > > > > pcibios_add_bus > > > > > pcibios_remove_bus > > These are only needed if you want to do per HB processing of the bus > > > > > > pcibios_align_resource > > mvebu is the only user of this function. > > > > > > pci_mmap_page_range > > This is only needed when mapping a PCI resource to userspace. Is that your case here? > > > > > > pci_domain_nr > > > > > pci_proc_domain > > We have equivalent functionality in the generic patches for those. > We clearly don't need those functions for the new drivers, but that's not the point. The problem is that when you build a kernel that has both a traditional host bridge driver and a new one in it, you always get those functions and they get called from the PCI core, with incorrect arguments. 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
diff --git a/Documentation/devicetree/bindings/pci/host-generic-pci.txt b/Documentation/devicetree/bindings/pci/host-generic-pci.txt index f0b0436..327e5b1 100644 --- a/Documentation/devicetree/bindings/pci/host-generic-pci.txt +++ b/Documentation/devicetree/bindings/pci/host-generic-pci.txt @@ -69,6 +69,9 @@ Practice: Interrupt Mapping' and requires the following properties: - interrupt-map-mask : <see aforementioned specification> +Optinal Properties: + +- msi-parent : Specify the msi-controller phandle. Example: diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig index 90f5cca..44bf523 100644 --- a/drivers/pci/host/Kconfig +++ b/drivers/pci/host/Kconfig @@ -50,7 +50,7 @@ config PCI_RCAR_GEN2_PCIE config PCI_HOST_GENERIC bool "Generic PCI host controller" - depends on ARM && OF + depends on (ARM || ARM64) && OF help Say Y here if you want to support a simple generic PCI host controller, such as the one emulated by kvmtool. diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c index 3d2076f..f33c547 100644 --- a/drivers/pci/host/pci-host-generic.c +++ b/drivers/pci/host/pci-host-generic.c @@ -42,14 +42,24 @@ struct gen_pci { struct pci_host_bridge host; struct gen_pci_cfg_windows cfg; struct list_head resources; + struct device_node *msi_parent; }; +#ifdef CONFIG_ARM64 +#define bus_to_gen_pci(b) \ + ((struct gen_pci *)b->sysdata) +#else +#define bus_to_gen_pci(b) \ + ((struct gen_pci *) \ + (((struct pci_sys_data *) \ + (bus->sysdata))->private_data)) +#endif + static void __iomem *gen_pci_map_cfg_bus_cam(struct pci_bus *bus, unsigned int devfn, int where) { - struct pci_sys_data *sys = bus->sysdata; - struct gen_pci *pci = sys->private_data; + struct gen_pci *pci = bus_to_gen_pci(bus); resource_size_t idx = bus->number - pci->cfg.bus_range.start; return pci->cfg.win[idx] + ((devfn << 8) | where); @@ -64,8 +74,7 @@ static void __iomem *gen_pci_map_cfg_bus_ecam(struct pci_bus *bus, unsigned int devfn, int where) { - struct pci_sys_data *sys = bus->sysdata; - struct gen_pci *pci = sys->private_data; + struct gen_pci *pci = bus_to_gen_pci(bus); resource_size_t idx = bus->number - pci->cfg.bus_range.start; return pci->cfg.win[idx] + ((devfn << 12) | where); @@ -80,8 +89,7 @@ static int gen_pci_config_read(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 *val) { void __iomem *addr; - struct pci_sys_data *sys = bus->sysdata; - struct gen_pci *pci = sys->private_data; + struct gen_pci *pci = bus_to_gen_pci(bus); addr = pci->cfg.ops->map_bus(bus, devfn, where); @@ -103,8 +111,7 @@ static int gen_pci_config_write(struct pci_bus *bus, unsigned int devfn, int where, int size, u32 val) { void __iomem *addr; - struct pci_sys_data *sys = bus->sysdata; - struct gen_pci *pci = sys->private_data; + struct gen_pci *pci = bus_to_gen_pci(bus); addr = pci->cfg.ops->map_bus(bus, devfn, where); @@ -144,8 +151,11 @@ static int gen_pci_calc_io_offset(struct device *dev, resource_size_t *offset) { static atomic_t wins = ATOMIC_INIT(0); - int err, idx, max_win; + int idx, max_win; unsigned int window; +#ifndef CONFIG_ARM64 + int err; +#endif if (!PAGE_ALIGNED(range->cpu_addr)) return -EINVAL; @@ -156,9 +166,12 @@ static int gen_pci_calc_io_offset(struct device *dev, return -ENOSPC; window = (idx - 1) * SZ_64K; + +#ifndef CONFIG_ARM64 err = pci_ioremap_io(window, range->cpu_addr); if (err) return err; +#endif of_pci_range_to_resource(range, dev->of_node, res); res->start = window; @@ -310,12 +323,58 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) return 0; } +#ifndef CONFIG_ARM64 static int gen_pci_setup(int nr, struct pci_sys_data *sys) { struct gen_pci *pci = sys->private_data; list_splice_init(&pci->resources, &sys->resources); return 1; } +#endif + +#ifdef CONFIG_ARM64 +struct pci_bus *gen_scan_root_bus(struct device *parent, int bus, + struct pci_ops *ops, void *sysdata, + struct list_head *resources) +{ + struct pci_host_bridge_window *window; + bool found = false; + struct pci_bus *b; + int max; + struct gen_pci *pci = sysdata; + + list_for_each_entry(window, resources, list) + if (window->res->flags & IORESOURCE_BUS) { + found = true; + break; + } + + b = pci_create_root_bus(parent, bus, ops, sysdata, resources); + if (!b) + return NULL; + + /* TODO: + * This is probably should be done in the core pci driver somewhere + */ + if (pci->msi_parent) + b->msi = of_pci_find_msi_chip_by_node(pci->msi_parent); + + 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); + } + + max = pci_scan_child_bus(b); + + if (!found) + pci_bus_update_busn_res_end(b, max); + + pci_bus_add_devices(b); + return b; +} +#endif static int gen_pci_probe(struct platform_device *pdev) { @@ -326,6 +385,7 @@ static int gen_pci_probe(struct platform_device *pdev) struct device *dev = &pdev->dev; struct device_node *np = dev->of_node; struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL); +#ifndef CONFIG_ARM64 struct hw_pci hw = { .nr_controllers = 1, .private_data = (void **)&pci, @@ -333,6 +393,7 @@ static int gen_pci_probe(struct platform_device *pdev) .map_irq = of_irq_parse_and_map_pci, .ops = &gen_pci_ops, }; +#endif if (!pci) return -ENOMEM; @@ -368,8 +429,24 @@ static int gen_pci_probe(struct platform_device *pdev) gen_pci_release_of_pci_ranges(pci); return err; } +#ifdef CONFIG_ARM64 +#ifdef CONFIG_PCI_MSI + pci->msi_parent = of_parse_phandle(np, "msi-parent", 0); + if (!pci->msi_parent) { + dev_err(&pdev->dev, "Failed to allocate msi-parent.\n"); + return -EINVAL; + } +#endif + + if (!gen_scan_root_bus(&pdev->dev, pci->cfg.bus_range.start, + &gen_pci_ops, pci, &pci->resources)) { + dev_err(&pdev->dev, "failed to enable PCIe ports\n"); + return -ENODEV; + } +#else pci_common_init_dev(dev, &hw); +#endif /* CONFIG_ARM64 */ return 0; }