| Message ID | 1411937610-22125-3-git-send-email-suravee.suthikulpanit@amd.com |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
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 devicetree" 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 devicetree" 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 devicetree" 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 devicetree" 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 devicetree" 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 devicetree" 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 devicetree" 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 09:01:14PM +0100, Arnd Bergmann wrote: > On Tuesday 30 September 2014 20:54:41 Arnd Bergmann wrote: > > 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. > > FWIW, the last time we discussed this, I think I had suggested that the > functions that are currently architecture specific and have a generic > __weak fallback could become function pointers in a per-host structure > passed to pci_scan_root_bus, either a new structure or an extended > struct pci_ops. Something along these lines: Agree to the general idea. But have a look why host drivers need the add_bus ops: to add MSI information into the bus!! If we take care of the MSI in the generic code there is less of a need for this function at all. Best regards, Liviu > > diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h > index 7fc42784becb..3da32fc631d0 100644 > --- a/arch/arm/include/asm/mach/pci.h > +++ b/arch/arm/include/asm/mach/pci.h > @@ -36,7 +36,6 @@ struct hw_pci { > resource_size_t start, > resource_size_t size, > resource_size_t align); > - void (*add_bus)(struct pci_bus *bus); > void (*remove_bus)(struct pci_bus *bus); > }; > > @@ -65,7 +64,6 @@ struct pci_sys_data { > resource_size_t start, > resource_size_t size, > resource_size_t align); > - void (*add_bus)(struct pci_bus *bus); > void (*remove_bus)(struct pci_bus *bus); > void *private_data; /* platform controller private data */ > }; > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c > index 17a26c17f7f5..3cbcf8dc41e4 100644 > --- a/arch/arm/kernel/bios32.c > +++ b/arch/arm/kernel/bios32.c > @@ -360,13 +360,6 @@ void pcibios_fixup_bus(struct pci_bus *bus) > } > EXPORT_SYMBOL(pcibios_fixup_bus); > > -void pcibios_add_bus(struct pci_bus *bus) > -{ > - struct pci_sys_data *sys = bus->sysdata; > - if (sys->add_bus) > - sys->add_bus(bus); > -} > - > void pcibios_remove_bus(struct pci_bus *bus) > { > struct pci_sys_data *sys = bus->sysdata; > @@ -475,7 +468,6 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, > sys->swizzle = hw->swizzle; > sys->map_irq = hw->map_irq; > sys->align_resource = hw->align_resource; > - sys->add_bus = hw->add_bus; > sys->remove_bus = hw->remove_bus; > INIT_LIST_HEAD(&sys->resources); > > diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c > index b1315e197ffb..c9a0ee0429e8 100644 > --- a/drivers/pci/host/pci-mvebu.c > +++ b/drivers/pci/host/pci-mvebu.c > @@ -716,6 +716,7 @@ static int mvebu_pcie_rd_conf(struct pci_bus *bus, u32 devfn, int where, > static struct pci_ops mvebu_pcie_ops = { > .read = mvebu_pcie_rd_conf, > .write = mvebu_pcie_wr_conf, > + .add_bus = mvebu_pcie_add_bus, > }; > > static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys) > @@ -823,7 +824,6 @@ static void mvebu_pcie_enable(struct mvebu_pcie *pcie) > hw.map_irq = of_irq_parse_and_map_pci; > hw.ops = &mvebu_pcie_ops; > hw.align_resource = mvebu_pcie_align_resource; > - hw.add_bus = mvebu_pcie_add_bus; > > pci_common_init(&hw); > } > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index a63a47a70846..be6d56358320 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -1885,6 +1885,8 @@ int __weak pcibios_root_bridge_prepare(struct pci_host_bridge *bridge) > > void __weak pcibios_add_bus(struct pci_bus *bus) > { > + if (bus->ops && bus->ops->add_bus) > + bus->ops->add_bus(bus); > } > > void __weak pcibios_remove_bus(struct pci_bus *bus) > > Arnd > >
On Wed, Oct 01, 2014 at 10:38:45AM +0100, Arnd Bergmann wrote: [...] > pci_mmap_page_range could either get generalized some more in an attempt > to have a __weak default implementation that works on ARM, or it could > be changed to lose the dependency on pci_sys_data instead. In either > case, the change would involve using the generic pci_host_bridge_window > list. On ARM pci_mmap_page_range requires pci_sys_data to retrieve its mem_offset parameter. I had a look, and I do not understand *why* it is required in that function, so I am asking. That function is basically used to map PCI resources to userspace, IIUC, through /proc or /sysfs file mappings. As far as I understand those mappings expect VMA pgoff to be the CPU address when files representing resources are mmapped from /proc and 0 when mmapped from /sys (I mean from userspace, then VMA pgoff should be updated by the kernel to map the resource). Question is: why pci_mmap_page_range() should apply an additional shift to the VMA pgoff based on pci_sys_data.mem_offset, which represents the offset from cpu->bus offset. I do not understand that. PowerPC does not seem to apply that fix-up (in PowerPC __pci_mmap_make_offset there is commented out code which prevents the pci_mem_offset shift to be applied). I think it all boils down to what the userspace interface is expecting when the memory areas are mmapped, if anyone has comments on this that is appreciated. Thanks, Lorenzo -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 07 October 2014 13:06:59 Lorenzo Pieralisi wrote: > On Wed, Oct 01, 2014 at 10:38:45AM +0100, Arnd Bergmann wrote: > > [...] > > > pci_mmap_page_range could either get generalized some more in an attempt > > to have a __weak default implementation that works on ARM, or it could > > be changed to lose the dependency on pci_sys_data instead. In either > > case, the change would involve using the generic pci_host_bridge_window > > list. > > On ARM pci_mmap_page_range requires pci_sys_data to retrieve its > mem_offset parameter. I had a look, and I do not understand *why* > it is required in that function, so I am asking. That function > is basically used to map PCI resources to userspace, IIUC, through > /proc or /sysfs file mappings. As far as I understand those mappings > expect VMA pgoff to be the CPU address when files representing resources > are mmapped from /proc and 0 when mmapped from /sys (I mean from > userspace, then VMA pgoff should be updated by the kernel to map the > resource). Applying the mem_offset is certainly the more intuitive way, since that lets you read the PCI BAR values from a device and access the device with the appropriate offsets. > Question is: why pci_mmap_page_range() should apply an additional > shift to the VMA pgoff based on pci_sys_data.mem_offset, which represents > the offset from cpu->bus offset. I do not understand that. PowerPC > does not seem to apply that fix-up (in PowerPC __pci_mmap_make_offset there > is commented out code which prevents the pci_mem_offset shift to be > applied). I think it all boils down to what the userspace interface is > expecting when the memory areas are mmapped, if anyone has comments on > this that is appreciated. The important part is certainly that whatever transformation is done by pci_resource_to_user() gets undone by __pci_mmap_make_offset(). In case of PowerPC and Microblaze, the mem_offset handling is commented out in both, to work around X11 trying to use the same values on /dev/mem. However, they do have the respective fixup for io_offset. sparc applies the offset in both places for both io_offset and mem_offset. xtensa applies only io_offset in __pci_mmap_make_offset but neither in pci_resource_to_user. This probably works because the mem_offset is always zero there. mips applies a different fixup (for 36-bit addressing), but not the mem_offset. Every other architecture applies no offset here, neither in __pci_mmap_make_offset/pci_mmap_page_range nor in pci_resource_to_user The only hint I could find for how the ARM version came to be is from the historic kernel tree git log for linux-2.5.42, which added the current code as 2002/10/13 11:05:47+01:00 rmk [ARM] Update pcibios_enable_device, supply pci_mmap_page_range() Update pcibios_enable_device to only enable requested resources, mainly for IDE. Supply a pci_mmap_page_range() function to allow user space to mmap PCI regions. At that point, only two platforms had a nonzero mem_offset: footbridge/dc21285 and integrator/pci_v3. Both were using VGA, and presumably used this to make X work. (rmk might remember details). The code at the time matched what powerpc and sparc did, but then both implemented pci_resource_to_user() in order for libpciaccess to work correctly (bcea1db16b for sparc, 463ce0e103f for powerpc), and later powerpc changed it again to not apply the offset in pci_resource_to_user or pci_mmap_page_range in 396a1a5832ae. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 07, 2014 at 02:52:27PM +0100, Arnd Bergmann wrote: > On Tuesday 07 October 2014 13:06:59 Lorenzo Pieralisi wrote: > > On Wed, Oct 01, 2014 at 10:38:45AM +0100, Arnd Bergmann wrote: > > > > [...] > > > > > pci_mmap_page_range could either get generalized some more in an attempt > > > to have a __weak default implementation that works on ARM, or it could > > > be changed to lose the dependency on pci_sys_data instead. In either > > > case, the change would involve using the generic pci_host_bridge_window > > > list. > > > > On ARM pci_mmap_page_range requires pci_sys_data to retrieve its > > mem_offset parameter. I had a look, and I do not understand *why* > > it is required in that function, so I am asking. That function > > is basically used to map PCI resources to userspace, IIUC, through > > /proc or /sysfs file mappings. As far as I understand those mappings > > expect VMA pgoff to be the CPU address when files representing resources > > are mmapped from /proc and 0 when mmapped from /sys (I mean from > > userspace, then VMA pgoff should be updated by the kernel to map the > > resource). > > Applying the mem_offset is certainly the more intuitive way, since > that lets you read the PCI BAR values from a device and access the > device with the appropriate offsets. Ok, but I am referring to this snippet (drivers/pci/pci-sysfs.c): /* pci_mmap_page_range() expects the same kind of entry as coming * from /proc/bus/pci/ which is a "user visible" value. If this is * different from the resource itself, arch will do necessary fixup. */ pci_resource_to_user(pdev, i, res, &start, &end); --> Here start represents a CPU physical address, if pci_resource_to_user() does not fix it up, correct ? vma->vm_pgoff += start >> PAGE_SHIFT; [...] return pci_mmap_page_range(...); pci_mmap_page_range() applies (mem_offset >> PAGE_SHIFT) to pgoff in the ARM implemention. Is not there a mismatch here on platforms where mem_offset != 0 ? > > Question is: why pci_mmap_page_range() should apply an additional > > shift to the VMA pgoff based on pci_sys_data.mem_offset, which represents > > the offset from cpu->bus offset. I do not understand that. PowerPC > > does not seem to apply that fix-up (in PowerPC __pci_mmap_make_offset there > > is commented out code which prevents the pci_mem_offset shift to be > > applied). I think it all boils down to what the userspace interface is > > expecting when the memory areas are mmapped, if anyone has comments on > > this that is appreciated. > > The important part is certainly that whatever transformation is done > by pci_resource_to_user() gets undone by __pci_mmap_make_offset(). Exactly, it does not seem to be the case above, that's why I asked. > In case of PowerPC and Microblaze, the mem_offset handling is commented > out in both, to work around X11 trying to use the same values on > /dev/mem. However, they do have the respective fixup for io_offset. > > sparc applies the offset in both places for both io_offset and mem_offset. > xtensa applies only io_offset in __pci_mmap_make_offset but neither > in pci_resource_to_user. This probably works because the mem_offset is > always zero there. > mips applies a different fixup (for 36-bit addressing), but not the > mem_offset. > > Every other architecture applies no offset here, neither in __pci_mmap_make_offset/pci_mmap_page_range nor in pci_resource_to_user > > The only hint I could find for how the ARM version came to be is > from the historic kernel tree git log for linux-2.5.42, which added > the current code as > > 2002/10/13 11:05:47+01:00 rmk > [ARM] Update pcibios_enable_device, supply pci_mmap_page_range() > Update pcibios_enable_device to only enable requested resources, > mainly for IDE. Supply a pci_mmap_page_range() function to allow > user space to mmap PCI regions. > > At that point, only two platforms had a nonzero mem_offset: > footbridge/dc21285 and integrator/pci_v3. Both were using VGA, > and presumably used this to make X work. (rmk might remember > details). I think that, as I mentioned, it boils down to what the userspace interface (proc/sys and they seem to differ) is supposed to be passed from userspace processes upon mmap. > The code at the time matched what powerpc and sparc did, but then > both implemented pci_resource_to_user() in order for libpciaccess > to work correctly (bcea1db16b for sparc, 463ce0e103f for powerpc), > and later powerpc changed it again to not apply the offset in > pci_resource_to_user or pci_mmap_page_range in 396a1a5832ae. I will keep investigating, thank you for your help, any further comments appreciated. Lorenzo -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 07 October 2014 15:47:50 Lorenzo Pieralisi wrote: > On Tue, Oct 07, 2014 at 02:52:27PM +0100, Arnd Bergmann wrote: > > On Tuesday 07 October 2014 13:06:59 Lorenzo Pieralisi wrote: > > > On Wed, Oct 01, 2014 at 10:38:45AM +0100, Arnd Bergmann wrote: > > > > > > [...] > > > > > > > pci_mmap_page_range could either get generalized some more in an attempt > > > > to have a __weak default implementation that works on ARM, or it could > > > > be changed to lose the dependency on pci_sys_data instead. In either > > > > case, the change would involve using the generic pci_host_bridge_window > > > > list. > > > > > > On ARM pci_mmap_page_range requires pci_sys_data to retrieve its > > > mem_offset parameter. I had a look, and I do not understand *why* > > > it is required in that function, so I am asking. That function > > > is basically used to map PCI resources to userspace, IIUC, through > > > /proc or /sysfs file mappings. As far as I understand those mappings > > > expect VMA pgoff to be the CPU address when files representing resources > > > are mmapped from /proc and 0 when mmapped from /sys (I mean from > > > userspace, then VMA pgoff should be updated by the kernel to map the > > > resource). > > > > Applying the mem_offset is certainly the more intuitive way, since > > that lets you read the PCI BAR values from a device and access the > > device with the appropriate offsets. > > Ok, but I am referring to this snippet (drivers/pci/pci-sysfs.c): > > /* pci_mmap_page_range() expects the same kind of entry as coming > * from /proc/bus/pci/ which is a "user visible" value. If this is > * different from the resource itself, arch will do necessary fixup. > */ > pci_resource_to_user(pdev, i, res, &start, &end); > > --> Here start represents a CPU physical address, if pci_resource_to_user() > does not fix it up, correct ? > > vma->vm_pgoff += start >> PAGE_SHIFT; > > [...] > > return pci_mmap_page_range(...); > > pci_mmap_page_range() applies (mem_offset >> PAGE_SHIFT) to pgoff in the > ARM implemention. > > Is not there a mismatch here on platforms where mem_offset != 0 ? Yes, I think that's right: ARM never gained its own pci_resource_to_user() implementation, presumably because nobody ran into this problem and debugged it all the way. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Oct 07, 2014 at 10:39:47PM +0100, Arnd Bergmann wrote: > On Tuesday 07 October 2014 15:47:50 Lorenzo Pieralisi wrote: > > On Tue, Oct 07, 2014 at 02:52:27PM +0100, Arnd Bergmann wrote: > > > On Tuesday 07 October 2014 13:06:59 Lorenzo Pieralisi wrote: > > > > On Wed, Oct 01, 2014 at 10:38:45AM +0100, Arnd Bergmann wrote: > > > > > > > > [...] > > > > > > > > > pci_mmap_page_range could either get generalized some more in an attempt > > > > > to have a __weak default implementation that works on ARM, or it could > > > > > be changed to lose the dependency on pci_sys_data instead. In either > > > > > case, the change would involve using the generic pci_host_bridge_window > > > > > list. > > > > > > > > On ARM pci_mmap_page_range requires pci_sys_data to retrieve its > > > > mem_offset parameter. I had a look, and I do not understand *why* > > > > it is required in that function, so I am asking. That function > > > > is basically used to map PCI resources to userspace, IIUC, through > > > > /proc or /sysfs file mappings. As far as I understand those mappings > > > > expect VMA pgoff to be the CPU address when files representing resources > > > > are mmapped from /proc and 0 when mmapped from /sys (I mean from > > > > userspace, then VMA pgoff should be updated by the kernel to map the > > > > resource). > > > > > > Applying the mem_offset is certainly the more intuitive way, since > > > that lets you read the PCI BAR values from a device and access the > > > device with the appropriate offsets. > > > > Ok, but I am referring to this snippet (drivers/pci/pci-sysfs.c): > > > > /* pci_mmap_page_range() expects the same kind of entry as coming > > * from /proc/bus/pci/ which is a "user visible" value. If this is > > * different from the resource itself, arch will do necessary fixup. > > */ > > pci_resource_to_user(pdev, i, res, &start, &end); > > > > --> Here start represents a CPU physical address, if pci_resource_to_user() > > does not fix it up, correct ? > > > > vma->vm_pgoff += start >> PAGE_SHIFT; > > > > [...] > > > > return pci_mmap_page_range(...); > > > > pci_mmap_page_range() applies (mem_offset >> PAGE_SHIFT) to pgoff in the > > ARM implemention. > > > > Is not there a mismatch here on platforms where mem_offset != 0 ? > > Yes, I think that's right: ARM never gained its own pci_resource_to_user() > implementation, presumably because nobody ran into this problem and > debugged it all the way. Ok. So, unless I am missing something, on platform with mem_offset != 0 /proc and /sys interfaces for remapping PCI resources can't work (IIUC the proc interface expects the user to pass in the resource address as seen from /proc/bus/pci/devices - which are not BAR values. Even if the user passed the BAR value to mmap, pci_mmap_fits() in proc_bus_pci_mmap() would fail since it compares the pgoff to resource values, which are not BAR values). As things stand I think we can safely remove the mem_offset (and pci_sys_data dependency) from pci_mmap_page_range(). I do not think we can break userspace in any way, basically because it can't work at the moment, again, happy to be corrected if I am wrong, please shout. Or we can add mem_offset to the host bridge (after all architectures like PowerPC and Microblaze have a pci_mem_offset variable in their host controllers), but still, this removes pci_sys_data dependency but does not solve the pci_mmap_page_range() issue. Lorenzo -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 08 October 2014 11:19:43 Lorenzo Pieralisi wrote: > > Ok. So, unless I am missing something, on platform with mem_offset != 0 > /proc and /sys interfaces for remapping PCI resources can't work (IIUC > the proc interface expects the user to pass in the resource address as > seen from /proc/bus/pci/devices - which are not BAR values. Even if the > user passed the BAR value to mmap, pci_mmap_fits() in proc_bus_pci_mmap() > would fail since it compares the pgoff to resource values, which are not > BAR values). I think you are right for the sysfs interface, that one can't possibly work because of the incorrect address computation. For the /procfs interface, I think it can work as long as the offsets used there are coming from the config space dump in /proc/bus/pci/* rather than from the /sys/bus/pci/devices/*/resource file. > As things stand I think we can safely remove the mem_offset (and > pci_sys_data dependency) from pci_mmap_page_range(). I do not think > we can break userspace in any way, basically because it can't work at > the moment, again, happy to be corrected if I am wrong, please shout. Please look at the procfs interface again. That one can be defined in two ways (either like sparc and arm, or like powerpc and microblaze) but either one should be able to work with user space that expects that interface and break with user space that expects the other one. > Or we can add mem_offset to the host bridge (after all architectures like > PowerPC and Microblaze have a pci_mem_offset variable in their host > controllers), but still, this removes pci_sys_data dependency but does > not solve the pci_mmap_page_range() issue. The host bridge already stores the mem_offset in terms of the resource list, so we could readily use that, except that it might break the powerpc hack if that is still in use. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 08, 2014 at 03:47:43PM +0100, Arnd Bergmann wrote: > On Wednesday 08 October 2014 11:19:43 Lorenzo Pieralisi wrote: > > > > Ok. So, unless I am missing something, on platform with mem_offset != 0 > > /proc and /sys interfaces for remapping PCI resources can't work (IIUC > > the proc interface expects the user to pass in the resource address as > > seen from /proc/bus/pci/devices - which are not BAR values. Even if the > > user passed the BAR value to mmap, pci_mmap_fits() in proc_bus_pci_mmap() > > would fail since it compares the pgoff to resource values, which are not > > BAR values). > > I think you are right for the sysfs interface, that one can't possibly > work because of the incorrect address computation. > > For the /procfs interface, I think it can work as long as the offsets > used there are coming from the config space dump in /proc/bus/pci/* > rather than from the /sys/bus/pci/devices/*/resource file. > > > As things stand I think we can safely remove the mem_offset (and > > pci_sys_data dependency) from pci_mmap_page_range(). I do not think > > we can break userspace in any way, basically because it can't work at > > the moment, again, happy to be corrected if I am wrong, please shout. > > Please look at the procfs interface again. That one can be defined > in two ways (either like sparc and arm, or like powerpc and microblaze) > but either one should be able to work with user space that expects > that interface and break with user space that expects the other one. I agree as long as pci_mmap_page_range() is concerned, but I am referring to the pci_mmap_fits() implementation here: start = vma->vm_pgoff; size = ((pci_resource_len(pdev, resno) - 1) >> PAGE_SHIFT) + 1; pci_start = (mmap_api == PCI_MMAP_PROCFS) ? pci_resource_start(pdev, resno) >> PAGE_SHIFT : 0; if (start >= pci_start && start < pci_start + size && start + nr <= pci_start + size) return 1; return 0; pci_mmap_fits(), when mapping from procfs always check the offset against resources, which are fixed-up addresses. If we passed the values dumped from the device config space (as pci_mmap_page_range() expects on arm) IMHO the check above would fail (always referring to platforms where mem_offset != 0). Last changes where introduced by commit 8c05cd08a, whose commit log adds to my confusion: "[...] I think what we want here is for pci_start to be 0 when mmap_api == PCI_MMAP_PROCFS.[...]" But that's not what the code does. I will try to grab an integrator board to give it a go. > > Or we can add mem_offset to the host bridge (after all architectures like > > PowerPC and Microblaze have a pci_mem_offset variable in their host > > controllers), but still, this removes pci_sys_data dependency but does > > not solve the pci_mmap_page_range() issue. > > The host bridge already stores the mem_offset in terms of the resource > list, so we could readily use that, except that it might break the > powerpc hack if that is still in use. Well, yes, I am not saying it can't be done by using the resources list, I am just trying to understand if that's really useful. Thank you ! Lorenzo -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 09 October 2014 10:04:20 Lorenzo Pieralisi wrote: > On Wed, Oct 08, 2014 at 03:47:43PM +0100, Arnd Bergmann wrote: > > On Wednesday 08 October 2014 11:19:43 Lorenzo Pieralisi wrote: > > Please look at the procfs interface again. That one can be defined > > in two ways (either like sparc and arm, or like powerpc and microblaze) > > but either one should be able to work with user space that expects > > that interface and break with user space that expects the other one. > > I agree as long as pci_mmap_page_range() is concerned, but I am > referring to the pci_mmap_fits() implementation here: > > start = vma->vm_pgoff; > size = ((pci_resource_len(pdev, resno) - 1) >> PAGE_SHIFT) + 1; > pci_start = (mmap_api == PCI_MMAP_PROCFS) ? > pci_resource_start(pdev, resno) >> PAGE_SHIFT : 0; > if (start >= pci_start && start < pci_start + size && > start + nr <= pci_start + size) > return 1; > return 0; > > pci_mmap_fits(), when mapping from procfs always check the offset against > resources, which are fixed-up addresses. If we passed the values dumped > from the device config space (as pci_mmap_page_range() expects on arm) IMHO > the check above would fail (always referring to platforms where > mem_offset != 0). Ah, I see it now too. > Last changes where introduced by commit 8c05cd08a, whose commit log adds > to my confusion: > > "[...] I think what we want here is for pci_start to be 0 when mmap_api == > PCI_MMAP_PROCFS.[...]" > > But that's not what the code does. My best guess is that this is a typo and that Darrick meant PCI_MMAP_SYSFS in the changelog, which is the same thing that the code does. It's also the sensible thing to do. This probably means that the procfs interface is now also broken on sparc. > I will try to grab an integrator board to give it a go. Ok, good idea. > > > Or we can add mem_offset to the host bridge (after all architectures like > > > PowerPC and Microblaze have a pci_mem_offset variable in their host > > > controllers), but still, this removes pci_sys_data dependency but does > > > not solve the pci_mmap_page_range() issue. > > > > The host bridge already stores the mem_offset in terms of the resource > > list, so we could readily use that, except that it might break the > > powerpc hack if that is still in use. > > Well, yes, I am not saying it can't be done by using the resources list, > I am just trying to understand if that's really useful. The PCI core tries to be ready for PCI host bridges that have multiple discontiguous memory spaces with different offsets, although I don't know of anybody has that. However if we decide to implement a generic pci_mmap_page_range that tries to take the offset into account, we should use the resource list in the host bridge because it can tell us the correct offsets. However, given what you found, the procfs interface being broken since 2010 on both architectures (arm32 and sparc) that try to honor the offset, we should probably go back to your previous suggestion of removing the offset handling, which would make it possible to use the procfs interface and the sysfs interface on all architectures. Would you be able to prepare a patch that does this and circulate that with the sparc, powerpc and microblaze maintainers as well as Darrick and Martin who were involved with the pci_mmap_fits change? Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 09, 2014 at 11:51:43AM +0100, Arnd Bergmann wrote: [...] > > Last changes where introduced by commit 8c05cd08a, whose commit log adds > > to my confusion: > > > > "[...] I think what we want here is for pci_start to be 0 when mmap_api == > > PCI_MMAP_PROCFS.[...]" > > > > But that's not what the code does. > > My best guess is that this is a typo and that Darrick meant PCI_MMAP_SYSFS > in the changelog, which is the same thing that the code does. It's also > the sensible thing to do. > > This probably means that the procfs interface is now also broken on > sparc. > > > I will try to grab an integrator board to give it a go. > > Ok, good idea. Grabbed, tested it, my theory was correct, I can't map PCI resources to userspace. Actually, if I pass resource offset as a fixed-up address, mmap succeeds through proc, but it does not mmap the resource, it maps the resource + mem_offset that happens to be RAM :D for the PCI slot I am using. I am testing on an oldish (3.16) kernel since I am having trouble with mainline PCI and my network adapter on integrator, but I do not see why this is a problem, this bug has been there forever. By removing mem_offset from pci_mmap_page_range() everything works fine, both proc and sys mappings are ok. > > > > Or we can add mem_offset to the host bridge (after all architectures like > > > > PowerPC and Microblaze have a pci_mem_offset variable in their host > > > > controllers), but still, this removes pci_sys_data dependency but does > > > > not solve the pci_mmap_page_range() issue. > > > > > > The host bridge already stores the mem_offset in terms of the resource > > > list, so we could readily use that, except that it might break the > > > powerpc hack if that is still in use. > > > > Well, yes, I am not saying it can't be done by using the resources list, > > I am just trying to understand if that's really useful. > > The PCI core tries to be ready for PCI host bridges that have multiple > discontiguous memory spaces with different offsets, although I don't know > of anybody has that. However if we decide to implement a generic > pci_mmap_page_range that tries to take the offset into account, we should > use the resource list in the host bridge because it can tell us the correct > offsets. > > However, given what you found, the procfs interface being broken since > 2010 on both architectures (arm32 and sparc) that try to honor the offset, > we should probably go back to your previous suggestion of removing > the offset handling, which would make it possible to use the procfs > interface and the sysfs interface on all architectures. > > Would you be able to prepare a patch that does this and circulate that > with the sparc, powerpc and microblaze maintainers as well as Darrick > and Martin who were involved with the pci_mmap_fits change? Yes, but let's step back a second. I think that the proc interface should expect an offset as passed to the user in /proc/bus/pci/devices, and there the resource is exposed through pci_resource_to_user(). Hence, the pci_mmap_fits() should check the offset against the resource filtered through pci_resource_to_user(), job done, patch is trivial, and does what pci_resource_to_user() was meant for IMHO. Then we have to decide what to do with arm32 code: 1) we remove mem_offset from pci_mmap_page_range() (and rely on default pci_resource_to_user()) or 2) we provide pci_resource_to_user() for arm32 which does the CPU->bus conversion for us (and leave mem_offset as-is in pci_mmap_range()) Thoughts ? Thanks, Lorenzo -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 10 October 2014 14:58:04 Lorenzo Pieralisi wrote: > On Thu, Oct 09, 2014 at 11:51:43AM +0100, Arnd Bergmann wrote: > > > > Last changes where introduced by commit 8c05cd08a, whose commit log adds > > > to my confusion: > > > > > > "[...] I think what we want here is for pci_start to be 0 when mmap_api == > > > PCI_MMAP_PROCFS.[...]" > > > > > > But that's not what the code does. > > > > My best guess is that this is a typo and that Darrick meant PCI_MMAP_SYSFS > > in the changelog, which is the same thing that the code does. It's also > > the sensible thing to do. > > > > This probably means that the procfs interface is now also broken on > > sparc. > > > > > I will try to grab an integrator board to give it a go. > > > > Ok, good idea. > > Grabbed, tested it, my theory was correct, I can't map PCI resources > to userspace. Actually, if I pass resource offset as a fixed-up address, mmap > succeeds through proc, but it does not mmap the resource, it maps > the resource + mem_offset that happens to be RAM :D for the PCI slot I am > using. > > I am testing on an oldish (3.16) kernel since I am having trouble with > mainline PCI and my network adapter on integrator, but I do not see why this > is a problem, this bug has been there forever. I would guess that almost the only users of the sysfs and procfs interfaces are Xorg drivers, you certainly don't need it to get a network adapter working. > By removing mem_offset from pci_mmap_page_range() everything works fine, > both proc and sys mappings are ok. Ok, thanks for confirming! > > However, given what you found, the procfs interface being broken since > > 2010 on both architectures (arm32 and sparc) that try to honor the offset, > > we should probably go back to your previous suggestion of removing > > the offset handling, which would make it possible to use the procfs > > interface and the sysfs interface on all architectures. > > > > Would you be able to prepare a patch that does this and circulate that > > with the sparc, powerpc and microblaze maintainers as well as Darrick > > and Martin who were involved with the pci_mmap_fits change? > > Yes, but let's step back a second. I think that the proc interface > should expect an offset as passed to the user in /proc/bus/pci/devices, > and there the resource is exposed through pci_resource_to_user(). > > Hence, the pci_mmap_fits() should check the offset against the > resource filtered through pci_resource_to_user(), job done, patch > is trivial, and does what pci_resource_to_user() was meant for IMHO. My point was that there is no reason why sparc and powerpc should do this differently. At the moment they do and sparc is broken as you proved. We can either fix sparc to restore the old behavior by adding pci_resource_to_user to pci_mmap_fits, or by making it do what powerpc does, essentially removing the memory space handling from pci_resource_to_user. Whatever we do for sparc is probably what we need to do on ARM as well, except that ARM has been broken for a longer time than sparc. > Then we have to decide what to do with arm32 code: > > 1) we remove mem_offset from pci_mmap_page_range() (and rely on default > pci_resource_to_user()) > > or > > 2) we provide pci_resource_to_user() for arm32 which does the CPU->bus > conversion for us (and leave mem_offset as-is in pci_mmap_range()) I'd vote for 1) to get it in line with the only working architectures that currently use a nonzero offset, but Russell needs to have the final word on this, and I still think we have to involve the sparc and powerpc maintainers as well, hoping to find a common solution for everybody. Making a user space interface behave differently based on the CPU architecture is a bad idea. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Oct 10, 2014 at 07:31:26PM +0100, Arnd Bergmann wrote: > On Friday 10 October 2014 14:58:04 Lorenzo Pieralisi wrote: > > On Thu, Oct 09, 2014 at 11:51:43AM +0100, Arnd Bergmann wrote: > > > > > > Last changes where introduced by commit 8c05cd08a, whose commit log adds > > > > to my confusion: > > > > > > > > "[...] I think what we want here is for pci_start to be 0 when mmap_api == > > > > PCI_MMAP_PROCFS.[...]" > > > > > > > > But that's not what the code does. > > > > > > My best guess is that this is a typo and that Darrick meant PCI_MMAP_SYSFS > > > in the changelog, which is the same thing that the code does. It's also > > > the sensible thing to do. > > > > > > This probably means that the procfs interface is now also broken on > > > sparc. > > > > > > > I will try to grab an integrator board to give it a go. > > > > > > Ok, good idea. > > > > Grabbed, tested it, my theory was correct, I can't map PCI resources > > to userspace. Actually, if I pass resource offset as a fixed-up address, mmap > > succeeds through proc, but it does not mmap the resource, it maps > > the resource + mem_offset that happens to be RAM :D for the PCI slot I am > > using. > > > > I am testing on an oldish (3.16) kernel since I am having trouble with > > mainline PCI and my network adapter on integrator, but I do not see why this > > is a problem, this bug has been there forever. > > I would guess that almost the only users of the sysfs and procfs > interfaces are Xorg drivers, you certainly don't need it to get > a network adapter working. The issue I am facing is not related to the PCI mmap implementation, that's certainly broken but does not stop me from using the board. [...] > > By removing mem_offset from pci_mmap_page_range() everything works fine, > > both proc and sys mappings are ok. > > Ok, thanks for confirming! > > > > However, given what you found, the procfs interface being broken since > > > 2010 on both architectures (arm32 and sparc) that try to honor the offset, > > > we should probably go back to your previous suggestion of removing > > > the offset handling, which would make it possible to use the procfs > > > interface and the sysfs interface on all architectures. > > > > > > Would you be able to prepare a patch that does this and circulate that > > > with the sparc, powerpc and microblaze maintainers as well as Darrick > > > and Martin who were involved with the pci_mmap_fits change? > > > > Yes, but let's step back a second. I think that the proc interface > > should expect an offset as passed to the user in /proc/bus/pci/devices, > > and there the resource is exposed through pci_resource_to_user(). > > > > Hence, the pci_mmap_fits() should check the offset against the > > resource filtered through pci_resource_to_user(), job done, patch > > is trivial, and does what pci_resource_to_user() was meant for IMHO. > > My point was that there is no reason why sparc and powerpc should > do this differently. At the moment they do and sparc is broken > as you proved. We can either fix sparc to restore the old behavior > by adding pci_resource_to_user to pci_mmap_fits, or by making it > do what powerpc does, essentially removing the memory space handling > from pci_resource_to_user. > > Whatever we do for sparc is probably what we need to do on ARM as well, > except that ARM has been broken for a longer time than sparc. > > > Then we have to decide what to do with arm32 code: > > > > 1) we remove mem_offset from pci_mmap_page_range() (and rely on default > > pci_resource_to_user()) > > > > or > > > > 2) we provide pci_resource_to_user() for arm32 which does the CPU->bus > > conversion for us (and leave mem_offset as-is in pci_mmap_range()) > > I'd vote for 1) to get it in line with the only working architectures > that currently use a nonzero offset, but Russell needs to have the final > word on this, and I still think we have to involve the sparc and powerpc > maintainers as well, hoping to find a common solution for everybody. > > Making a user space interface behave differently based on the CPU > architecture is a bad idea. I agree with you, I will put together a patchset and copy all people who should be involved. Lorenzo -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 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. > pci_mmap_page_range could either get generalized some more in an attempt > to have a __weak default implementation that works on ARM, or it could > be changed to lose the dependency on pci_sys_data instead. In either > case, the change would involve using the generic pci_host_bridge_window > list. I need to repost my series, but I *think* we can consider the dependency on pci_sys_data gone in pci_mmap_page_range(). > pcibios_align_resource should probably be per host, and we could move > that into a pointer in pci_host_bridge, something like this: Yes, and that's likely to be true for add_bus too. I wonder what's the best course of action. Putting together all the bits and pieces required to remove PCI bios dependency from this patch can take a while, I wonder whether we should aim for merging this driver (rebased on top of my port to the new parse ranges API) with the ARM/ARM64 ifdeffery and clean it up later or aim for the whole thing at once, I am just worried it can take us a while. Lorenzo > > diff --git a/drivers/pci/setup-res.c b/drivers/pci/setup-res.c > index b7c3a5ea1fca..d9cb6c916d54 100644 > --- a/drivers/pci/setup-res.c > +++ b/drivers/pci/setup-res.c > @@ -200,11 +200,15 @@ static int pci_revert_fw_address(struct resource *res, struct pci_dev *dev, > static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev, > int resno, resource_size_t size, resource_size_t align) > { > + struct pci_host_bridge *host = find_pci_host_bridge(bus); > + resource_size_t (*alignf)(void *, const struct resource *, > + resource_size_t, resource_size_t), > struct resource *res = dev->resource + resno; > resource_size_t min; > int ret; > > min = (res->flags & IORESOURCE_IO) ? PCIBIOS_MIN_IO : PCIBIOS_MIN_MEM; > + alignf = host->align_resource ?: pcibios_align_resource; > > /* > * First, try exact prefetching match. Even if a 64-bit > @@ -215,7 +219,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev, > */ > ret = pci_bus_alloc_resource(bus, res, size, align, min, > IORESOURCE_PREFETCH | IORESOURCE_MEM_64, > - pcibios_align_resource, dev); > + alignf, dev); > if (ret == 0) > return 0; > > @@ -227,7 +231,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev, > (IORESOURCE_PREFETCH | IORESOURCE_MEM_64)) { > ret = pci_bus_alloc_resource(bus, res, size, align, min, > IORESOURCE_PREFETCH, > - pcibios_align_resource, dev); > + alignf, dev); > if (ret == 0) > return 0; > } > @@ -240,7 +244,7 @@ static int __pci_assign_resource(struct pci_bus *bus, struct pci_dev *dev, > */ > if (res->flags & (IORESOURCE_PREFETCH | IORESOURCE_MEM_64)) > ret = pci_bus_alloc_resource(bus, res, size, align, min, 0, > - pcibios_align_resource, dev); > + alignf, dev); > > return ret; > } > > > If we decide constantly calling find_pci_host_bridge() is too expensive, we can > be more clever about it. > > Arnd > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 9:59 AM, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote: > ... I wonder what's the > best course of action. Putting together all the bits and pieces required > to remove PCI bios dependency from this patch can take a while, I wonder > whether we should aim for merging this driver (rebased on top of my port to the > new parse ranges API) with the ARM/ARM64 ifdeffery and clean it up later > or aim for the whole thing at once, I am just worried it can take us a while. I haven't looked at your patches, but "the whole thing at once" is never *my* goal. A gradual cleanup is just fine with me. Bjorn -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 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 devicetree" 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 devicetree" 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 02:33:16PM +0100, Arnd Bergmann wrote: > On Thursday 23 October 2014 10:13:09 Liviu Dudau wrote: > > > @@ -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? > > hw_pci->private_data is an array of pointers to private_data for each > host controller instance within the domain. There is only one entry > here, but you still the the correct type, so that would be > > hw_pci->private_data = (void **)&&cns3xxx_pcie[i]; > > which is even more confusing and ugly than what I wrote. If you have > a better idea, I'm all for it. Maybe it's clearer to write like this > (taken from rcar driver)? I was not questioning the implementation (and I do like it) I was just wondering if it is to work around some quirk of gcc. As I did not had the time to test it I was hoping that if there is a gcc requirement to phrase the code that way you might provide me with an answer. Best regards, Liviu > > void *hw_private[1]; > hw_pci.private_data = hw_private; > > for each host { > ... > hw_private[0] = &cns3xxx_pcie[i]; > pci_common_init_dev(&hw_pci); > } > > Note that all 'modern' controllers always use nr_controllers=1, so we > only need a single private_data pointer per domain, and the entire > hw_pci interface is a bit pointless. > > The platforms that currently require it are iop13xx, dove, mv78xx0 > and orion5x. We have plans to remove the last three platforms in > the next merge window or two, once all users are able to migrate to > mach-mvebu. Once that happens, we could probably move the entire > hw_pci logic that deals with multiple hosts per domain into the > iop13xx pci driver if we want to. A less intrusive simplification > would be to convert all 'multiplatform'-aware host controllers to > use pci_common_init_dev() and then take hw_pci out of that. > > See below for a sample patch I just did. It duplicates the code from > pci_common_init_dev/pci_common_init because we know that all users > of pci_common_init_dev are modern and only pass a single host bridge. > The new pci_common_init_dev is simpler than the old one but should > do the exact same thing for all current users, with the addition > of propagating the return value. > > pci_init_single() is the new internal helper and we should be able to > convert all existing users of pci_common_init_dev() to use that directly > and no longer define hw_pci at all. > > I've converted two drivers to give an example, but the conversions > should be done in follow-up patches really, and the pci_common_init_dev > function removed after all users are moved over. > > The new pci_init_single() is also rather simple, and it should just > converge with what we do for arm64 over time. > > Arnd > > --- > arch/arm/include/asm/mach/pci.h | 20 ++++--- > arch/arm/kernel/bios32.c | 103 ++++++++++++++++++++++++++++++++++-- > drivers/pci/host/pci-host-generic.c | 53 ++++++++----------- > drivers/pci/host/pci-mvebu.c | 44 +++++++-------- > 4 files changed, 157 insertions(+), 63 deletions(-) > > diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h > index 7fc42784becb..fe7e13759ec0 100644 > --- a/arch/arm/include/asm/mach/pci.h > +++ b/arch/arm/include/asm/mach/pci.h > @@ -73,16 +73,22 @@ struct pci_sys_data { > /* > * Call this with your hw_pci struct to initialise the PCI system. > */ > -void pci_common_init_dev(struct device *, struct hw_pci *); > +void pci_common_init(struct hw_pci *); > > /* > - * Compatibility wrapper for older platforms that do not care about > - * passing the parent device. > + * Used by modern platforms, only one host allowed. > */ > -static inline void pci_common_init(struct hw_pci *hw) > -{ > - pci_common_init_dev(NULL, hw); > -} > +int pci_common_init_dev(struct device *, struct hw_pci *); > + > +/* > + * Replaces pci_common_init_dev for drivers that want to do the > + * initialization simpler and avoid defining hw_pci > + */ > +int pci_init_single(struct device *parent, > + struct pci_sys_data *sys, > + struct pci_bus *(*scan)(int nr, struct pci_sys_data *), > + struct pci_ops *ops); > + > > /* > * Setup early fixed I/O mapping. > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c > index 17a26c17f7f5..bccc8703e575 100644 > --- a/arch/arm/kernel/bios32.c > +++ b/arch/arm/kernel/bios32.c > @@ -456,8 +456,7 @@ static int pcibios_init_resources(int busnr, struct pci_sys_data *sys) > return 0; > } > > -static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, > - struct list_head *head) > +static void pcibios_init_hw(struct hw_pci *hw, struct list_head *head) > { > struct pci_sys_data *sys = NULL; > int ret; > @@ -494,7 +493,7 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, > if (hw->scan) > sys->bus = hw->scan(nr, sys); > else > - sys->bus = pci_scan_root_bus(parent, sys->busnr, > + sys->bus = pci_scan_root_bus(NULL, sys->busnr, > hw->ops, sys, &sys->resources); > > if (!sys->bus) > @@ -511,7 +510,7 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, > } > } > > -void pci_common_init_dev(struct device *parent, struct hw_pci *hw) > +void pci_common_init(struct hw_pci *hw) > { > struct pci_sys_data *sys; > LIST_HEAD(head); > @@ -519,7 +518,7 @@ void pci_common_init_dev(struct device *parent, struct hw_pci *hw) > pci_add_flags(PCI_REASSIGN_ALL_RSRC); > if (hw->preinit) > hw->preinit(); > - pcibios_init_hw(parent, hw, &head); > + pcibios_init_hw(hw, &head); > if (hw->postinit) > hw->postinit(); > > @@ -559,6 +558,100 @@ void pci_common_init_dev(struct device *parent, struct hw_pci *hw) > } > } > > +int pci_init_single(struct device *parent, > + struct pci_sys_data *sys, > + struct pci_bus *(*scan)(int nr, struct pci_sys_data *), > + struct pci_ops *ops) > +{ > + int ret; > + struct pci_bus *bus; > + > + ret = pcibios_init_resources(0, sys); > + if (ret) > + return ret; > + > + if (scan) > + bus = scan(0, sys); > + else > + bus = pci_scan_root_bus(parent, 0, ops, sys, &sys->resources); > + > + if (!bus) { > + dev_err(parent, "PCI: unable to scan bus!"); > + return -ENXIO; > + } > + sys->bus = bus; > + > + pci_fixup_irqs(pcibios_swizzle, pcibios_map_irq); > + > + if (!pci_has_flag(PCI_PROBE_ONLY)) { > + /* > + * Size the bridge windows. > + */ > + pci_bus_size_bridges(bus); > + > + /* > + * Assign resources. > + */ > + pci_bus_assign_resources(bus); > + } > + > + /* > + * Tell drivers about devices found. > + */ > + pci_bus_add_devices(bus); > + > + /* Configure PCI Express settings */ > + if (bus && !pci_has_flag(PCI_PROBE_ONLY)) { > + struct pci_bus *child; > + > + list_for_each_entry(child, &bus->children, node) > + pcie_bus_configure_settings(child); > + } > + > + return 0; > +} > + > +int pci_common_init_dev(struct device *parent, struct hw_pci *hw) > +{ > + struct pci_sys_data *sys; > + int ret; > + > + if (hw->nr_controllers != 1 || > + hw->preinit || hw->postinit) > + return -EINVAL; > + > + sys = kzalloc(sizeof(struct pci_sys_data), GFP_KERNEL); > + if (!sys) > + return -ENOMEM; > + > +#ifdef CONFIG_PCI_DOMAINS > + sys->domain = hw->domain; > +#endif > + sys->swizzle = hw->swizzle; > + sys->map_irq = hw->map_irq; > + sys->align_resource = hw->align_resource; > + sys->add_bus = hw->add_bus; > + sys->remove_bus = hw->remove_bus; > + INIT_LIST_HEAD(&sys->resources); > + > + if (hw->private_data) > + sys->private_data = hw->private_data[0]; > + > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); > + ret = hw->setup(0, sys); > + if (ret == 0) > + ret = -ENXIO; > + if (ret < 0) > + return ret; > + > + ret = pcibios_init_sysdata(parent, sys, hw->scan, hw->ops); > + if (ret) > + /* FIXME: undo ->setup */ > + kfree(sys); > + > + return ret; > +} > + > #ifndef CONFIG_PCI_HOST_ITE8152 > void pcibios_set_master(struct pci_dev *dev) > { > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c > index 3d2076f59911..3542a7b740e5 100644 > --- a/drivers/pci/host/pci-host-generic.c > +++ b/drivers/pci/host/pci-host-generic.c > @@ -40,16 +40,20 @@ struct gen_pci_cfg_windows { > > struct gen_pci { > struct pci_host_bridge host; > + struct pci_sys_data sys; > struct gen_pci_cfg_windows cfg; > - struct list_head resources; > }; > > +static inline struct gen_pci *gen_pci_from_sys(struct pci_sys_data *sys) > +{ > + return container_of(sys, struct gen_pci, sys); > +} > + > 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 = gen_pci_from_sys(bus->sysdata); > resource_size_t idx = bus->number - pci->cfg.bus_range.start; > > return pci->cfg.win[idx] + ((devfn << 8) | where); > @@ -64,8 +68,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 = gen_pci_from_sys(bus->sysdata); > resource_size_t idx = bus->number - pci->cfg.bus_range.start; > > return pci->cfg.win[idx] + ((devfn << 12) | where); > @@ -80,8 +83,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 = gen_pci_from_sys(bus->sysdata); > > addr = pci->cfg.ops->map_bus(bus, devfn, where); > > @@ -103,8 +105,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 = gen_pci_from_sys(bus->sysdata); > > addr = pci->cfg.ops->map_bus(bus, devfn, where); > > @@ -181,10 +182,10 @@ static void gen_pci_release_of_pci_ranges(struct gen_pci *pci) > { > struct pci_host_bridge_window *win; > > - list_for_each_entry(win, &pci->resources, list) > + list_for_each_entry(win, &pci->sys.resources, list) > release_resource(win->res); > > - pci_free_resource_list(&pci->resources); > + pci_free_resource_list(&pci->sys.resources); > } > > static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci) > @@ -237,7 +238,7 @@ static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci) > if (err) > goto out_release_res; > > - pci_add_resource_offset(&pci->resources, res, offset); > + pci_add_resource_offset(&pci->sys.resources, res, offset); > } > > if (!res_valid) { > @@ -306,17 +307,10 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) > } > > /* Register bus resource */ > - pci_add_resource(&pci->resources, bus_range); > + pci_add_resource(&pci->sys.resources, bus_range); > return 0; > } > > -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; > -} > - > static int gen_pci_probe(struct platform_device *pdev) > { > int err; > @@ -326,17 +320,12 @@ 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); > - struct hw_pci hw = { > - .nr_controllers = 1, > - .private_data = (void **)&pci, > - .setup = gen_pci_setup, > - .map_irq = of_irq_parse_and_map_pci, > - .ops = &gen_pci_ops, > - }; > > if (!pci) > return -ENOMEM; > > + pci->sys.map_irq = of_irq_parse_and_map_pci, > + > type = of_get_property(np, "device_type", NULL); > if (!type || strcmp(type, "pci")) { > dev_err(dev, "invalid \"device_type\" %s\n", type); > @@ -355,7 +344,7 @@ static int gen_pci_probe(struct platform_device *pdev) > pci->cfg.ops = of_id->data; > pci->host.dev.parent = dev; > INIT_LIST_HEAD(&pci->host.windows); > - INIT_LIST_HEAD(&pci->resources); > + INIT_LIST_HEAD(&pci->sys.resources); > > /* Parse our PCI ranges and request their resources */ > err = gen_pci_parse_request_of_pci_ranges(pci); > @@ -369,8 +358,12 @@ static int gen_pci_probe(struct platform_device *pdev) > return err; > } > > - pci_common_init_dev(dev, &hw); > - return 0; > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); > + err = pci_init_single(dev, &pci->sys, NULL, &gen_pci_ops); > + if (err) > + gen_pci_release_of_pci_ranges(pci); > + > + return err; > } > > static struct platform_driver gen_pci_driver = { > diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c > index b1315e197ffb..e1381c0699be 100644 > --- a/drivers/pci/host/pci-mvebu.c > +++ b/drivers/pci/host/pci-mvebu.c > @@ -99,6 +99,7 @@ struct mvebu_pcie_port; > struct mvebu_pcie { > struct platform_device *pdev; > struct mvebu_pcie_port *ports; > + struct pci_sys_data sysdata; > struct msi_chip *msi; > struct resource io; > char io_name[30]; > @@ -611,7 +612,7 @@ static int mvebu_sw_pci_bridge_write(struct mvebu_pcie_port *port, > > static inline struct mvebu_pcie *sys_to_pcie(struct pci_sys_data *sys) > { > - return sys->private_data; > + return container_of(sys, struct mvebu_pcie, sysdata); > } > > static struct mvebu_pcie_port *mvebu_pcie_find_port(struct mvebu_pcie *pcie, > @@ -718,11 +719,26 @@ static struct pci_ops mvebu_pcie_ops = { > .write = mvebu_pcie_wr_conf, > }; > > -static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys) > +/* FIXME: move the code around to avoid these */ > +static struct pci_bus *mvebu_pcie_scan_bus(int nr, struct pci_sys_data *sys); > +static void mvebu_pcie_add_bus(struct pci_bus *bus); > +static resource_size_t mvebu_pcie_align_resource(struct pci_dev *dev, > + const struct resource *res, > + resource_size_t start, > + resource_size_t size, > + resource_size_t align); > + > +static int mvebu_pcie_enable(struct mvebu_pcie *pcie) > { > - struct mvebu_pcie *pcie = sys_to_pcie(sys); > int i; > int domain = 0; > + struct pci_sys_data *sys = &pcie->sysdata; > + > + pcie->sysdata = (struct pci_sys_data) { > + .map_irq = of_irq_parse_and_map_pci, > + .align_resource = mvebu_pcie_align_resource, > + .add_bus = mvebu_pcie_add_bus, > + }; > > #ifdef CONFIG_PCI_DOMAINS > domain = sys->domain; > @@ -738,11 +754,13 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys) > if (request_resource(&iomem_resource, &pcie->mem)) > return 0; > > + INIT_LIST_HEAD(&sys->resources); > if (resource_size(&pcie->realio) != 0) { > if (request_resource(&ioport_resource, &pcie->realio)) { > release_resource(&pcie->mem); > return 0; > } > + > pci_add_resource_offset(&sys->resources, &pcie->realio, > sys->io_offset); > } > @@ -756,7 +774,9 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys) > mvebu_pcie_setup_hw(port); > } > > - return 1; > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); > + return pci_init_single(&pcie->pdev->dev, &pcie->sysdata, > + mvebu_pcie_scan_bus, &mvebu_pcie_ops); > } > > static struct pci_bus *mvebu_pcie_scan_bus(int nr, struct pci_sys_data *sys) > @@ -810,24 +830,6 @@ static resource_size_t mvebu_pcie_align_resource(struct pci_dev *dev, > return start; > } > > -static void mvebu_pcie_enable(struct mvebu_pcie *pcie) > -{ > - struct hw_pci hw; > - > - memset(&hw, 0, sizeof(hw)); > - > - hw.nr_controllers = 1; > - hw.private_data = (void **)&pcie; > - hw.setup = mvebu_pcie_setup; > - hw.scan = mvebu_pcie_scan_bus; > - hw.map_irq = of_irq_parse_and_map_pci; > - hw.ops = &mvebu_pcie_ops; > - hw.align_resource = mvebu_pcie_align_resource; > - hw.add_bus = mvebu_pcie_add_bus; > - > - pci_common_init(&hw); > -} > - > /* > * Looks up the list of register addresses encoded into the reg = > * <...> property for one that matches the given port/lane. Once > @@ -1066,9 +1068,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev) > pci_ioremap_io(i, pcie->io.start + i); > > mvebu_pcie_msi_enable(pcie); > - mvebu_pcie_enable(pcie); > - > - return 0; > + return mvebu_pcie_enable(pcie); > } > > static const struct of_device_id mvebu_pcie_of_match_table[] = { > > >
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 devicetree" 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 devicetree" 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 03:33:16PM +0200, Arnd Bergmann wrote: > On Thursday 23 October 2014 10:13:09 Liviu Dudau wrote: > > > @@ -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? > > hw_pci->private_data is an array of pointers to private_data for each > host controller instance within the domain. There is only one entry > here, but you still the the correct type, so that would be > > hw_pci->private_data = (void **)&&cns3xxx_pcie[i]; > > which is even more confusing and ugly than what I wrote. If you have > a better idea, I'm all for it. Maybe it's clearer to write like this > (taken from rcar driver)? > > void *hw_private[1]; > hw_pci.private_data = hw_private; > > for each host { > ... > hw_private[0] = &cns3xxx_pcie[i]; > pci_common_init_dev(&hw_pci); > } > > Note that all 'modern' controllers always use nr_controllers=1, so we > only need a single private_data pointer per domain, and the entire > hw_pci interface is a bit pointless. > > The platforms that currently require it are iop13xx, dove, mv78xx0 > and orion5x. We have plans to remove the last three platforms in > the next merge window or two, once all users are able to migrate to > mach-mvebu. Once that happens, we could probably move the entire > hw_pci logic that deals with multiple hosts per domain into the > iop13xx pci driver if we want to. A less intrusive simplification > would be to convert all 'multiplatform'-aware host controllers to > use pci_common_init_dev() and then take hw_pci out of that. > > See below for a sample patch I just did. It duplicates the code from > pci_common_init_dev/pci_common_init because we know that all users > of pci_common_init_dev are modern and only pass a single host bridge. > The new pci_common_init_dev is simpler than the old one but should > do the exact same thing for all current users, with the addition > of propagating the return value. Same with this one; I'm ignoring for now on the theory that this was just a sample to show the idea, and a more formal patch might come later. Bjorn > pci_init_single() is the new internal helper and we should be able to > convert all existing users of pci_common_init_dev() to use that directly > and no longer define hw_pci at all. > > I've converted two drivers to give an example, but the conversions > should be done in follow-up patches really, and the pci_common_init_dev > function removed after all users are moved over. > > The new pci_init_single() is also rather simple, and it should just > converge with what we do for arm64 over time. > > Arnd > > --- > arch/arm/include/asm/mach/pci.h | 20 ++++--- > arch/arm/kernel/bios32.c | 103 ++++++++++++++++++++++++++++++++++-- > drivers/pci/host/pci-host-generic.c | 53 ++++++++----------- > drivers/pci/host/pci-mvebu.c | 44 +++++++-------- > 4 files changed, 157 insertions(+), 63 deletions(-) > > diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h > index 7fc42784becb..fe7e13759ec0 100644 > --- a/arch/arm/include/asm/mach/pci.h > +++ b/arch/arm/include/asm/mach/pci.h > @@ -73,16 +73,22 @@ struct pci_sys_data { > /* > * Call this with your hw_pci struct to initialise the PCI system. > */ > -void pci_common_init_dev(struct device *, struct hw_pci *); > +void pci_common_init(struct hw_pci *); > > /* > - * Compatibility wrapper for older platforms that do not care about > - * passing the parent device. > + * Used by modern platforms, only one host allowed. > */ > -static inline void pci_common_init(struct hw_pci *hw) > -{ > - pci_common_init_dev(NULL, hw); > -} > +int pci_common_init_dev(struct device *, struct hw_pci *); > + > +/* > + * Replaces pci_common_init_dev for drivers that want to do the > + * initialization simpler and avoid defining hw_pci > + */ > +int pci_init_single(struct device *parent, > + struct pci_sys_data *sys, > + struct pci_bus *(*scan)(int nr, struct pci_sys_data *), > + struct pci_ops *ops); > + > > /* > * Setup early fixed I/O mapping. > diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c > index 17a26c17f7f5..bccc8703e575 100644 > --- a/arch/arm/kernel/bios32.c > +++ b/arch/arm/kernel/bios32.c > @@ -456,8 +456,7 @@ static int pcibios_init_resources(int busnr, struct pci_sys_data *sys) > return 0; > } > > -static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, > - struct list_head *head) > +static void pcibios_init_hw(struct hw_pci *hw, struct list_head *head) > { > struct pci_sys_data *sys = NULL; > int ret; > @@ -494,7 +493,7 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, > if (hw->scan) > sys->bus = hw->scan(nr, sys); > else > - sys->bus = pci_scan_root_bus(parent, sys->busnr, > + sys->bus = pci_scan_root_bus(NULL, sys->busnr, > hw->ops, sys, &sys->resources); > > if (!sys->bus) > @@ -511,7 +510,7 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw, > } > } > > -void pci_common_init_dev(struct device *parent, struct hw_pci *hw) > +void pci_common_init(struct hw_pci *hw) > { > struct pci_sys_data *sys; > LIST_HEAD(head); > @@ -519,7 +518,7 @@ void pci_common_init_dev(struct device *parent, struct hw_pci *hw) > pci_add_flags(PCI_REASSIGN_ALL_RSRC); > if (hw->preinit) > hw->preinit(); > - pcibios_init_hw(parent, hw, &head); > + pcibios_init_hw(hw, &head); > if (hw->postinit) > hw->postinit(); > > @@ -559,6 +558,100 @@ void pci_common_init_dev(struct device *parent, struct hw_pci *hw) > } > } > > +int pci_init_single(struct device *parent, > + struct pci_sys_data *sys, > + struct pci_bus *(*scan)(int nr, struct pci_sys_data *), > + struct pci_ops *ops) > +{ > + int ret; > + struct pci_bus *bus; > + > + ret = pcibios_init_resources(0, sys); > + if (ret) > + return ret; > + > + if (scan) > + bus = scan(0, sys); > + else > + bus = pci_scan_root_bus(parent, 0, ops, sys, &sys->resources); > + > + if (!bus) { > + dev_err(parent, "PCI: unable to scan bus!"); > + return -ENXIO; > + } > + sys->bus = bus; > + > + pci_fixup_irqs(pcibios_swizzle, pcibios_map_irq); > + > + if (!pci_has_flag(PCI_PROBE_ONLY)) { > + /* > + * Size the bridge windows. > + */ > + pci_bus_size_bridges(bus); > + > + /* > + * Assign resources. > + */ > + pci_bus_assign_resources(bus); > + } > + > + /* > + * Tell drivers about devices found. > + */ > + pci_bus_add_devices(bus); > + > + /* Configure PCI Express settings */ > + if (bus && !pci_has_flag(PCI_PROBE_ONLY)) { > + struct pci_bus *child; > + > + list_for_each_entry(child, &bus->children, node) > + pcie_bus_configure_settings(child); > + } > + > + return 0; > +} > + > +int pci_common_init_dev(struct device *parent, struct hw_pci *hw) > +{ > + struct pci_sys_data *sys; > + int ret; > + > + if (hw->nr_controllers != 1 || > + hw->preinit || hw->postinit) > + return -EINVAL; > + > + sys = kzalloc(sizeof(struct pci_sys_data), GFP_KERNEL); > + if (!sys) > + return -ENOMEM; > + > +#ifdef CONFIG_PCI_DOMAINS > + sys->domain = hw->domain; > +#endif > + sys->swizzle = hw->swizzle; > + sys->map_irq = hw->map_irq; > + sys->align_resource = hw->align_resource; > + sys->add_bus = hw->add_bus; > + sys->remove_bus = hw->remove_bus; > + INIT_LIST_HEAD(&sys->resources); > + > + if (hw->private_data) > + sys->private_data = hw->private_data[0]; > + > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); > + ret = hw->setup(0, sys); > + if (ret == 0) > + ret = -ENXIO; > + if (ret < 0) > + return ret; > + > + ret = pcibios_init_sysdata(parent, sys, hw->scan, hw->ops); > + if (ret) > + /* FIXME: undo ->setup */ > + kfree(sys); > + > + return ret; > +} > + > #ifndef CONFIG_PCI_HOST_ITE8152 > void pcibios_set_master(struct pci_dev *dev) > { > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c > index 3d2076f59911..3542a7b740e5 100644 > --- a/drivers/pci/host/pci-host-generic.c > +++ b/drivers/pci/host/pci-host-generic.c > @@ -40,16 +40,20 @@ struct gen_pci_cfg_windows { > > struct gen_pci { > struct pci_host_bridge host; > + struct pci_sys_data sys; > struct gen_pci_cfg_windows cfg; > - struct list_head resources; > }; > > +static inline struct gen_pci *gen_pci_from_sys(struct pci_sys_data *sys) > +{ > + return container_of(sys, struct gen_pci, sys); > +} > + > 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 = gen_pci_from_sys(bus->sysdata); > resource_size_t idx = bus->number - pci->cfg.bus_range.start; > > return pci->cfg.win[idx] + ((devfn << 8) | where); > @@ -64,8 +68,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 = gen_pci_from_sys(bus->sysdata); > resource_size_t idx = bus->number - pci->cfg.bus_range.start; > > return pci->cfg.win[idx] + ((devfn << 12) | where); > @@ -80,8 +83,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 = gen_pci_from_sys(bus->sysdata); > > addr = pci->cfg.ops->map_bus(bus, devfn, where); > > @@ -103,8 +105,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 = gen_pci_from_sys(bus->sysdata); > > addr = pci->cfg.ops->map_bus(bus, devfn, where); > > @@ -181,10 +182,10 @@ static void gen_pci_release_of_pci_ranges(struct gen_pci *pci) > { > struct pci_host_bridge_window *win; > > - list_for_each_entry(win, &pci->resources, list) > + list_for_each_entry(win, &pci->sys.resources, list) > release_resource(win->res); > > - pci_free_resource_list(&pci->resources); > + pci_free_resource_list(&pci->sys.resources); > } > > static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci) > @@ -237,7 +238,7 @@ static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci) > if (err) > goto out_release_res; > > - pci_add_resource_offset(&pci->resources, res, offset); > + pci_add_resource_offset(&pci->sys.resources, res, offset); > } > > if (!res_valid) { > @@ -306,17 +307,10 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) > } > > /* Register bus resource */ > - pci_add_resource(&pci->resources, bus_range); > + pci_add_resource(&pci->sys.resources, bus_range); > return 0; > } > > -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; > -} > - > static int gen_pci_probe(struct platform_device *pdev) > { > int err; > @@ -326,17 +320,12 @@ 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); > - struct hw_pci hw = { > - .nr_controllers = 1, > - .private_data = (void **)&pci, > - .setup = gen_pci_setup, > - .map_irq = of_irq_parse_and_map_pci, > - .ops = &gen_pci_ops, > - }; > > if (!pci) > return -ENOMEM; > > + pci->sys.map_irq = of_irq_parse_and_map_pci, > + > type = of_get_property(np, "device_type", NULL); > if (!type || strcmp(type, "pci")) { > dev_err(dev, "invalid \"device_type\" %s\n", type); > @@ -355,7 +344,7 @@ static int gen_pci_probe(struct platform_device *pdev) > pci->cfg.ops = of_id->data; > pci->host.dev.parent = dev; > INIT_LIST_HEAD(&pci->host.windows); > - INIT_LIST_HEAD(&pci->resources); > + INIT_LIST_HEAD(&pci->sys.resources); > > /* Parse our PCI ranges and request their resources */ > err = gen_pci_parse_request_of_pci_ranges(pci); > @@ -369,8 +358,12 @@ static int gen_pci_probe(struct platform_device *pdev) > return err; > } > > - pci_common_init_dev(dev, &hw); > - return 0; > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); > + err = pci_init_single(dev, &pci->sys, NULL, &gen_pci_ops); > + if (err) > + gen_pci_release_of_pci_ranges(pci); > + > + return err; > } > > static struct platform_driver gen_pci_driver = { > diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c > index b1315e197ffb..e1381c0699be 100644 > --- a/drivers/pci/host/pci-mvebu.c > +++ b/drivers/pci/host/pci-mvebu.c > @@ -99,6 +99,7 @@ struct mvebu_pcie_port; > struct mvebu_pcie { > struct platform_device *pdev; > struct mvebu_pcie_port *ports; > + struct pci_sys_data sysdata; > struct msi_chip *msi; > struct resource io; > char io_name[30]; > @@ -611,7 +612,7 @@ static int mvebu_sw_pci_bridge_write(struct mvebu_pcie_port *port, > > static inline struct mvebu_pcie *sys_to_pcie(struct pci_sys_data *sys) > { > - return sys->private_data; > + return container_of(sys, struct mvebu_pcie, sysdata); > } > > static struct mvebu_pcie_port *mvebu_pcie_find_port(struct mvebu_pcie *pcie, > @@ -718,11 +719,26 @@ static struct pci_ops mvebu_pcie_ops = { > .write = mvebu_pcie_wr_conf, > }; > > -static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys) > +/* FIXME: move the code around to avoid these */ > +static struct pci_bus *mvebu_pcie_scan_bus(int nr, struct pci_sys_data *sys); > +static void mvebu_pcie_add_bus(struct pci_bus *bus); > +static resource_size_t mvebu_pcie_align_resource(struct pci_dev *dev, > + const struct resource *res, > + resource_size_t start, > + resource_size_t size, > + resource_size_t align); > + > +static int mvebu_pcie_enable(struct mvebu_pcie *pcie) > { > - struct mvebu_pcie *pcie = sys_to_pcie(sys); > int i; > int domain = 0; > + struct pci_sys_data *sys = &pcie->sysdata; > + > + pcie->sysdata = (struct pci_sys_data) { > + .map_irq = of_irq_parse_and_map_pci, > + .align_resource = mvebu_pcie_align_resource, > + .add_bus = mvebu_pcie_add_bus, > + }; > > #ifdef CONFIG_PCI_DOMAINS > domain = sys->domain; > @@ -738,11 +754,13 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys) > if (request_resource(&iomem_resource, &pcie->mem)) > return 0; > > + INIT_LIST_HEAD(&sys->resources); > if (resource_size(&pcie->realio) != 0) { > if (request_resource(&ioport_resource, &pcie->realio)) { > release_resource(&pcie->mem); > return 0; > } > + > pci_add_resource_offset(&sys->resources, &pcie->realio, > sys->io_offset); > } > @@ -756,7 +774,9 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys) > mvebu_pcie_setup_hw(port); > } > > - return 1; > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); > + return pci_init_single(&pcie->pdev->dev, &pcie->sysdata, > + mvebu_pcie_scan_bus, &mvebu_pcie_ops); > } > > static struct pci_bus *mvebu_pcie_scan_bus(int nr, struct pci_sys_data *sys) > @@ -810,24 +830,6 @@ static resource_size_t mvebu_pcie_align_resource(struct pci_dev *dev, > return start; > } > > -static void mvebu_pcie_enable(struct mvebu_pcie *pcie) > -{ > - struct hw_pci hw; > - > - memset(&hw, 0, sizeof(hw)); > - > - hw.nr_controllers = 1; > - hw.private_data = (void **)&pcie; > - hw.setup = mvebu_pcie_setup; > - hw.scan = mvebu_pcie_scan_bus; > - hw.map_irq = of_irq_parse_and_map_pci; > - hw.ops = &mvebu_pcie_ops; > - hw.align_resource = mvebu_pcie_align_resource; > - hw.add_bus = mvebu_pcie_add_bus; > - > - pci_common_init(&hw); > -} > - > /* > * Looks up the list of register addresses encoded into the reg = > * <...> property for one that matches the given port/lane. Once > @@ -1066,9 +1068,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev) > pci_ioremap_io(i, pcie->io.start + i); > > mvebu_pcie_msi_enable(pcie); > - mvebu_pcie_enable(pcie); > - > - return 0; > + return mvebu_pcie_enable(pcie); > } > > static const struct of_device_id mvebu_pcie_of_match_table[] = { > -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 devicetree" 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:40:58 Bjorn Helgaas wrote: > > > > See below for a sample patch I just did. It duplicates the code from > > pci_common_init_dev/pci_common_init because we know that all users > > of pci_common_init_dev are modern and only pass a single host bridge. > > The new pci_common_init_dev is simpler than the old one but should > > do the exact same thing for all current users, with the addition > > of propagating the return value. > > Same with this one; I'm ignoring for now on the theory that this was just a > sample to show the idea, and a more formal patch might come later. > Correct. Or we might not do it at all, if some of the other patches are done before we need this. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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 devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, I am not sure if this thread is still alive. I'm trying to see what I can do to help clean up/convert to make the PCI GHC also works for arm64 w/ zero or minimal ifdefs. Please let me know if someone is already working on this. I noticed that Lorenzo's patches has already been in 3.19-rc1, and in Bjorn's pci/domain branch. Otherwise, I'll try to continue the work based on the sample patch from Arnd here. On 10/23/14 08:33, Arnd Bergmann wrote: > [...] > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c > index 3d2076f59911..3542a7b740e5 100644 > --- a/drivers/pci/host/pci-host-generic.c > +++ b/drivers/pci/host/pci-host-generic.c > @@ -40,16 +40,20 @@ struct gen_pci_cfg_windows { > > struct gen_pci { > struct pci_host_bridge host; > + struct pci_sys_data sys; > struct gen_pci_cfg_windows cfg; > - struct list_head resources; > }; Arnd, based on the patch here, if we are trying to use the pci-host-generic driver on arm64, this means that we are going to have to introduce struct pci_sys_data for the arm64 as well (e.g move the struct from include/asm/mach/pci.h to include/linux/pci.h). Is this also your intention? Thanks, Suravee > > +static inline struct gen_pci *gen_pci_from_sys(struct pci_sys_data *sys) > +{ > + return container_of(sys, struct gen_pci, sys); > +} > + > 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 = gen_pci_from_sys(bus->sysdata); > resource_size_t idx = bus->number - pci->cfg.bus_range.start; > > return pci->cfg.win[idx] + ((devfn << 8) | where); > @@ -64,8 +68,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 = gen_pci_from_sys(bus->sysdata); > resource_size_t idx = bus->number - pci->cfg.bus_range.start; > > return pci->cfg.win[idx] + ((devfn << 12) | where); > @@ -80,8 +83,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 = gen_pci_from_sys(bus->sysdata); > > addr = pci->cfg.ops->map_bus(bus, devfn, where); > > @@ -103,8 +105,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 = gen_pci_from_sys(bus->sysdata); > > addr = pci->cfg.ops->map_bus(bus, devfn, where); > > @@ -181,10 +182,10 @@ static void gen_pci_release_of_pci_ranges(struct gen_pci *pci) > { > struct pci_host_bridge_window *win; > > - list_for_each_entry(win, &pci->resources, list) > + list_for_each_entry(win, &pci->sys.resources, list) > release_resource(win->res); > > - pci_free_resource_list(&pci->resources); > + pci_free_resource_list(&pci->sys.resources); > } > > static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci) > @@ -237,7 +238,7 @@ static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci) > if (err) > goto out_release_res; > > - pci_add_resource_offset(&pci->resources, res, offset); > + pci_add_resource_offset(&pci->sys.resources, res, offset); > } > > if (!res_valid) { > @@ -306,17 +307,10 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) > } > > /* Register bus resource */ > - pci_add_resource(&pci->resources, bus_range); > + pci_add_resource(&pci->sys.resources, bus_range); > return 0; > } > > -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; > -} > - > static int gen_pci_probe(struct platform_device *pdev) > { > int err; > @@ -326,17 +320,12 @@ 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); > - struct hw_pci hw = { > - .nr_controllers = 1, > - .private_data = (void **)&pci, > - .setup = gen_pci_setup, > - .map_irq = of_irq_parse_and_map_pci, > - .ops = &gen_pci_ops, > - }; > > if (!pci) > return -ENOMEM; > > + pci->sys.map_irq = of_irq_parse_and_map_pci, > + > type = of_get_property(np, "device_type", NULL); > if (!type || strcmp(type, "pci")) { > dev_err(dev, "invalid \"device_type\" %s\n", type); > @@ -355,7 +344,7 @@ static int gen_pci_probe(struct platform_device *pdev) > pci->cfg.ops = of_id->data; > pci->host.dev.parent = dev; > INIT_LIST_HEAD(&pci->host.windows); > - INIT_LIST_HEAD(&pci->resources); > + INIT_LIST_HEAD(&pci->sys.resources); > > /* Parse our PCI ranges and request their resources */ > err = gen_pci_parse_request_of_pci_ranges(pci); > @@ -369,8 +358,12 @@ static int gen_pci_probe(struct platform_device *pdev) > return err; > } > > - pci_common_init_dev(dev, &hw); > - return 0; > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); > + err = pci_init_single(dev, &pci->sys, NULL, &gen_pci_ops); > + if (err) > + gen_pci_release_of_pci_ranges(pci); > + > + return err; > } > > static struct platform_driver gen_pci_driver = { > diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c > index b1315e197ffb..e1381c0699be 100644 > --- a/drivers/pci/host/pci-mvebu.c > +++ b/drivers/pci/host/pci-mvebu.c > @@ -99,6 +99,7 @@ struct mvebu_pcie_port; > struct mvebu_pcie { > struct platform_device *pdev; > struct mvebu_pcie_port *ports; > + struct pci_sys_data sysdata; > struct msi_chip *msi; > struct resource io; > char io_name[30]; > @@ -611,7 +612,7 @@ static int mvebu_sw_pci_bridge_write(struct mvebu_pcie_port *port, > > static inline struct mvebu_pcie *sys_to_pcie(struct pci_sys_data *sys) > { > - return sys->private_data; > + return container_of(sys, struct mvebu_pcie, sysdata); > } > > static struct mvebu_pcie_port *mvebu_pcie_find_port(struct mvebu_pcie *pcie, > @@ -718,11 +719,26 @@ static struct pci_ops mvebu_pcie_ops = { > .write = mvebu_pcie_wr_conf, > }; > > -static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys) > +/* FIXME: move the code around to avoid these */ > +static struct pci_bus *mvebu_pcie_scan_bus(int nr, struct pci_sys_data *sys); > +static void mvebu_pcie_add_bus(struct pci_bus *bus); > +static resource_size_t mvebu_pcie_align_resource(struct pci_dev *dev, > + const struct resource *res, > + resource_size_t start, > + resource_size_t size, > + resource_size_t align); > + > +static int mvebu_pcie_enable(struct mvebu_pcie *pcie) > { > - struct mvebu_pcie *pcie = sys_to_pcie(sys); > int i; > int domain = 0; > + struct pci_sys_data *sys = &pcie->sysdata; > + > + pcie->sysdata = (struct pci_sys_data) { > + .map_irq = of_irq_parse_and_map_pci, > + .align_resource = mvebu_pcie_align_resource, > + .add_bus = mvebu_pcie_add_bus, > + }; > > #ifdef CONFIG_PCI_DOMAINS > domain = sys->domain; > @@ -738,11 +754,13 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys) > if (request_resource(&iomem_resource, &pcie->mem)) > return 0; > > + INIT_LIST_HEAD(&sys->resources); > if (resource_size(&pcie->realio) != 0) { > if (request_resource(&ioport_resource, &pcie->realio)) { > release_resource(&pcie->mem); > return 0; > } > + > pci_add_resource_offset(&sys->resources, &pcie->realio, > sys->io_offset); > } > @@ -756,7 +774,9 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys) > mvebu_pcie_setup_hw(port); > } > > - return 1; > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); > + return pci_init_single(&pcie->pdev->dev, &pcie->sysdata, > + mvebu_pcie_scan_bus, &mvebu_pcie_ops); > } > > static struct pci_bus *mvebu_pcie_scan_bus(int nr, struct pci_sys_data *sys) > @@ -810,24 +830,6 @@ static resource_size_t mvebu_pcie_align_resource(struct pci_dev *dev, > return start; > } > > -static void mvebu_pcie_enable(struct mvebu_pcie *pcie) > -{ > - struct hw_pci hw; > - > - memset(&hw, 0, sizeof(hw)); > - > - hw.nr_controllers = 1; > - hw.private_data = (void **)&pcie; > - hw.setup = mvebu_pcie_setup; > - hw.scan = mvebu_pcie_scan_bus; > - hw.map_irq = of_irq_parse_and_map_pci; > - hw.ops = &mvebu_pcie_ops; > - hw.align_resource = mvebu_pcie_align_resource; > - hw.add_bus = mvebu_pcie_add_bus; > - > - pci_common_init(&hw); > -} > - > /* > * Looks up the list of register addresses encoded into the reg = > * <...> property for one that matches the given port/lane. Once > @@ -1066,9 +1068,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev) > pci_ioremap_io(i, pcie->io.start + i); > > mvebu_pcie_msi_enable(pcie); > - mvebu_pcie_enable(pcie); > - > - return 0; > + return mvebu_pcie_enable(pcie); > } > > static const struct of_device_id mvebu_pcie_of_match_table[] = { > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Suravee, On Mon, Dec 29, 2014 at 07:32:44PM +0000, Suravee Suthikulpanit wrote: > Hi, > > I am not sure if this thread is still alive. I'm trying to see what I > can do to help clean up/convert to make the PCI GHC also works for arm64 > w/ zero or minimal ifdefs. > > Please let me know if someone is already working on this. I noticed that > Lorenzo's patches has already been in 3.19-rc1, and in Bjorn's > pci/domain branch. Otherwise, I'll try to continue the work based on the > sample patch from Arnd here. If I am not mistaken, the only bit missing to remove pci_sys_data (and so having a generic host controller driver that works on ARM32/64) is generic MSI management. I know for certain Marc is working on it, and the solution is WIP, I think we should prevent adding more churn to pci_sys_data, since I managed to remove most of the dependencies (domain, mem_offset). So to sum it up, to have a generic host controller driver for ARM32/64 we just need to work out how to handle the MSI data, patches will be on the lists shortly to handle that, please review. Thanks, Lorenzo > On 10/23/14 08:33, Arnd Bergmann wrote: > > [...] > > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c > > index 3d2076f59911..3542a7b740e5 100644 > > --- a/drivers/pci/host/pci-host-generic.c > > +++ b/drivers/pci/host/pci-host-generic.c > > @@ -40,16 +40,20 @@ struct gen_pci_cfg_windows { > > > > struct gen_pci { > > struct pci_host_bridge host; > > + struct pci_sys_data sys; > > struct gen_pci_cfg_windows cfg; > > - struct list_head resources; > > }; > > Arnd, based on the patch here, if we are trying to use the > pci-host-generic driver on arm64, this means that we are going to have > to introduce struct pci_sys_data for the arm64 as well (e.g move the > struct from include/asm/mach/pci.h to include/linux/pci.h). Is this also > your intention? > > Thanks, > > Suravee > > > > > +static inline struct gen_pci *gen_pci_from_sys(struct pci_sys_data *sys) > > +{ > > + return container_of(sys, struct gen_pci, sys); > > +} > > + > > 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 = gen_pci_from_sys(bus->sysdata); > > resource_size_t idx = bus->number - pci->cfg.bus_range.start; > > > > return pci->cfg.win[idx] + ((devfn << 8) | where); > > @@ -64,8 +68,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 = gen_pci_from_sys(bus->sysdata); > > resource_size_t idx = bus->number - pci->cfg.bus_range.start; > > > > return pci->cfg.win[idx] + ((devfn << 12) | where); > > @@ -80,8 +83,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 = gen_pci_from_sys(bus->sysdata); > > > > addr = pci->cfg.ops->map_bus(bus, devfn, where); > > > > @@ -103,8 +105,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 = gen_pci_from_sys(bus->sysdata); > > > > addr = pci->cfg.ops->map_bus(bus, devfn, where); > > > > @@ -181,10 +182,10 @@ static void gen_pci_release_of_pci_ranges(struct gen_pci *pci) > > { > > struct pci_host_bridge_window *win; > > > > - list_for_each_entry(win, &pci->resources, list) > > + list_for_each_entry(win, &pci->sys.resources, list) > > release_resource(win->res); > > > > - pci_free_resource_list(&pci->resources); > > + pci_free_resource_list(&pci->sys.resources); > > } > > > > static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci) > > @@ -237,7 +238,7 @@ static int gen_pci_parse_request_of_pci_ranges(struct gen_pci *pci) > > if (err) > > goto out_release_res; > > > > - pci_add_resource_offset(&pci->resources, res, offset); > > + pci_add_resource_offset(&pci->sys.resources, res, offset); > > } > > > > if (!res_valid) { > > @@ -306,17 +307,10 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) > > } > > > > /* Register bus resource */ > > - pci_add_resource(&pci->resources, bus_range); > > + pci_add_resource(&pci->sys.resources, bus_range); > > return 0; > > } > > > > -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; > > -} > > - > > static int gen_pci_probe(struct platform_device *pdev) > > { > > int err; > > @@ -326,17 +320,12 @@ 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); > > - struct hw_pci hw = { > > - .nr_controllers = 1, > > - .private_data = (void **)&pci, > > - .setup = gen_pci_setup, > > - .map_irq = of_irq_parse_and_map_pci, > > - .ops = &gen_pci_ops, > > - }; > > > > if (!pci) > > return -ENOMEM; > > > > + pci->sys.map_irq = of_irq_parse_and_map_pci, > > + > > type = of_get_property(np, "device_type", NULL); > > if (!type || strcmp(type, "pci")) { > > dev_err(dev, "invalid \"device_type\" %s\n", type); > > @@ -355,7 +344,7 @@ static int gen_pci_probe(struct platform_device *pdev) > > pci->cfg.ops = of_id->data; > > pci->host.dev.parent = dev; > > INIT_LIST_HEAD(&pci->host.windows); > > - INIT_LIST_HEAD(&pci->resources); > > + INIT_LIST_HEAD(&pci->sys.resources); > > > > /* Parse our PCI ranges and request their resources */ > > err = gen_pci_parse_request_of_pci_ranges(pci); > > @@ -369,8 +358,12 @@ static int gen_pci_probe(struct platform_device *pdev) > > return err; > > } > > > > - pci_common_init_dev(dev, &hw); > > - return 0; > > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); > > + err = pci_init_single(dev, &pci->sys, NULL, &gen_pci_ops); > > + if (err) > > + gen_pci_release_of_pci_ranges(pci); > > + > > + return err; > > } > > > > static struct platform_driver gen_pci_driver = { > > diff --git a/drivers/pci/host/pci-mvebu.c b/drivers/pci/host/pci-mvebu.c > > index b1315e197ffb..e1381c0699be 100644 > > --- a/drivers/pci/host/pci-mvebu.c > > +++ b/drivers/pci/host/pci-mvebu.c > > @@ -99,6 +99,7 @@ struct mvebu_pcie_port; > > struct mvebu_pcie { > > struct platform_device *pdev; > > struct mvebu_pcie_port *ports; > > + struct pci_sys_data sysdata; > > struct msi_chip *msi; > > struct resource io; > > char io_name[30]; > > @@ -611,7 +612,7 @@ static int mvebu_sw_pci_bridge_write(struct mvebu_pcie_port *port, > > > > static inline struct mvebu_pcie *sys_to_pcie(struct pci_sys_data *sys) > > { > > - return sys->private_data; > > + return container_of(sys, struct mvebu_pcie, sysdata); > > } > > > > static struct mvebu_pcie_port *mvebu_pcie_find_port(struct mvebu_pcie *pcie, > > @@ -718,11 +719,26 @@ static struct pci_ops mvebu_pcie_ops = { > > .write = mvebu_pcie_wr_conf, > > }; > > > > -static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys) > > +/* FIXME: move the code around to avoid these */ > > +static struct pci_bus *mvebu_pcie_scan_bus(int nr, struct pci_sys_data *sys); > > +static void mvebu_pcie_add_bus(struct pci_bus *bus); > > +static resource_size_t mvebu_pcie_align_resource(struct pci_dev *dev, > > + const struct resource *res, > > + resource_size_t start, > > + resource_size_t size, > > + resource_size_t align); > > + > > +static int mvebu_pcie_enable(struct mvebu_pcie *pcie) > > { > > - struct mvebu_pcie *pcie = sys_to_pcie(sys); > > int i; > > int domain = 0; > > + struct pci_sys_data *sys = &pcie->sysdata; > > + > > + pcie->sysdata = (struct pci_sys_data) { > > + .map_irq = of_irq_parse_and_map_pci, > > + .align_resource = mvebu_pcie_align_resource, > > + .add_bus = mvebu_pcie_add_bus, > > + }; > > > > #ifdef CONFIG_PCI_DOMAINS > > domain = sys->domain; > > @@ -738,11 +754,13 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys) > > if (request_resource(&iomem_resource, &pcie->mem)) > > return 0; > > > > + INIT_LIST_HEAD(&sys->resources); > > if (resource_size(&pcie->realio) != 0) { > > if (request_resource(&ioport_resource, &pcie->realio)) { > > release_resource(&pcie->mem); > > return 0; > > } > > + > > pci_add_resource_offset(&sys->resources, &pcie->realio, > > sys->io_offset); > > } > > @@ -756,7 +774,9 @@ static int mvebu_pcie_setup(int nr, struct pci_sys_data *sys) > > mvebu_pcie_setup_hw(port); > > } > > > > - return 1; > > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); > > + return pci_init_single(&pcie->pdev->dev, &pcie->sysdata, > > + mvebu_pcie_scan_bus, &mvebu_pcie_ops); > > } > > > > static struct pci_bus *mvebu_pcie_scan_bus(int nr, struct pci_sys_data *sys) > > @@ -810,24 +830,6 @@ static resource_size_t mvebu_pcie_align_resource(struct pci_dev *dev, > > return start; > > } > > > > -static void mvebu_pcie_enable(struct mvebu_pcie *pcie) > > -{ > > - struct hw_pci hw; > > - > > - memset(&hw, 0, sizeof(hw)); > > - > > - hw.nr_controllers = 1; > > - hw.private_data = (void **)&pcie; > > - hw.setup = mvebu_pcie_setup; > > - hw.scan = mvebu_pcie_scan_bus; > > - hw.map_irq = of_irq_parse_and_map_pci; > > - hw.ops = &mvebu_pcie_ops; > > - hw.align_resource = mvebu_pcie_align_resource; > > - hw.add_bus = mvebu_pcie_add_bus; > > - > > - pci_common_init(&hw); > > -} > > - > > /* > > * Looks up the list of register addresses encoded into the reg = > > * <...> property for one that matches the given port/lane. Once > > @@ -1066,9 +1068,7 @@ static int mvebu_pcie_probe(struct platform_device *pdev) > > pci_ioremap_io(i, pcie->io.start + i); > > > > mvebu_pcie_msi_enable(pcie); > > - mvebu_pcie_enable(pcie); > > - > > - return 0; > > + return mvebu_pcie_enable(pcie); > > } > > > > static const struct of_device_id mvebu_pcie_of_match_table[] = { > > > -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 1/2/2015 5:55 AM, Lorenzo Pieralisi wrote: > Hi Suravee, > > On Mon, Dec 29, 2014 at 07:32:44PM +0000, Suravee Suthikulpanit wrote: >> >Hi, >> > >> >I am not sure if this thread is still alive. I'm trying to see what I >> >can do to help clean up/convert to make the PCI GHC also works for arm64 >> >w/ zero or minimal ifdefs. >> > >> >Please let me know if someone is already working on this. I noticed that >> >Lorenzo's patches has already been in 3.19-rc1, and in Bjorn's >> >pci/domain branch. Otherwise, I'll try to continue the work based on the >> >sample patch from Arnd here. > If I am not mistaken, the only bit missing to remove pci_sys_data (and so > having a generic host controller driver that works on ARM32/64) is generic > MSI management. Lorenzo, Do you mean to remove pci_sys_data from pci-host-generic.c or removing it completely? I assume the former case. So, looking at the current code in the pci-host-generic.c, my understanding is that the: *gen_pci = pci_bus->sysdata->private_data will be changed to: *gen_pci = pci_bus->sysdata Then, we can simply just call pci_scan_root_bus() directly since we no longer need to declare hw_pci for calling pci_common_init_dev(). > I know for certain Marc is working on it, and the solution is WIP, > I think we should prevent adding more churn to pci_sys_data, since > I managed to remove most of the dependencies (domain, mem_offset). Thanks for cleaning up the domain and mem_offset. I saw Marc's irq/msi_domain patch series (http://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/msi_domain). My understanding is that deals with associating the newly introduced msi_domain to each device, which replaces the need for pci_bus->msi and hw_pci->msi_ctrl when configure with CONFIG_PCI_MSI_IRQ_DOMAIN (not sure if this would be the plan for all arm32). For ARM32, if not define CONFIG_PCI_MSI_IRQ_DOMAIN, it would still fall back to using the [pci_sys_data|hw_pci]->msi_ctrl. However, I noticed that the hw_pci->msi_controller is not even used for the pci-host-generic. So, this should not be blocking the work to free pci-host-generic from pci_sys_data and hw_pci, as the MSI stuff can go in separately. Am I missing something? > So to sum it up, to have a generic host controller driver for ARM32/64 > we just need to work out how to handle the MSI data, patches will be > on the lists shortly to handle that, please review. > > Thanks, > Lorenzo > Thanks for the update and the summary. I'll help review/test the MSI patch once posted. Thanks, Suravee -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Friday 02 January 2015 12:18:06 Suravee Suthikulanit wrote: > On 1/2/2015 5:55 AM, Lorenzo Pieralisi wrote: > > Hi Suravee, > > > > On Mon, Dec 29, 2014 at 07:32:44PM +0000, Suravee Suthikulpanit wrote: > >> >Hi, > >> > > >> >I am not sure if this thread is still alive. I'm trying to see what I > >> >can do to help clean up/convert to make the PCI GHC also works for arm64 > >> >w/ zero or minimal ifdefs. > >> > > >> >Please let me know if someone is already working on this. I noticed that > >> >Lorenzo's patches has already been in 3.19-rc1, and in Bjorn's > >> >pci/domain branch. Otherwise, I'll try to continue the work based on the > >> >sample patch from Arnd here. > > If I am not mistaken, the only bit missing to remove pci_sys_data (and so > > having a generic host controller driver that works on ARM32/64) is generic > > MSI management. > > Lorenzo, > > Do you mean to remove pci_sys_data from pci-host-generic.c or removing > it completely? I assume the former case. Something inbetween: We should be able to remove pci_sys_data and pci_common_init_dev from all drivers in drivers/pci/host/, but keep them for all drivers in arch/arm/*/pci.c > So, looking at the current code in the pci-host-generic.c, my > understanding is that the: > *gen_pci = pci_bus->sysdata->private_data > will be changed to: > *gen_pci = pci_bus->sysdata > > Then, we can simply just call pci_scan_root_bus() directly since we no > longer need to declare hw_pci for calling pci_common_init_dev(). Right. > > I know for certain Marc is working on it, and the solution is WIP, > > I think we should prevent adding more churn to pci_sys_data, since > > I managed to remove most of the dependencies (domain, mem_offset). > > Thanks for cleaning up the domain and mem_offset. > > I saw Marc's irq/msi_domain patch series > (http://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/msi_domain). > > > My understanding is that deals with associating the newly introduced > msi_domain to each device, which replaces the need for pci_bus->msi and > hw_pci->msi_ctrl when configure with CONFIG_PCI_MSI_IRQ_DOMAIN (not sure > if this would be the plan for all arm32). For ARM32, if not define > CONFIG_PCI_MSI_IRQ_DOMAIN, it would still fall back to using the > [pci_sys_data|hw_pci]->msi_ctrl. For all I can tell, we have two cases on ARM regarding MSI: - arch/arm/mach-iop13xx/pci.c uses its own arch_setup_msi_irq/arch_teardown_msi_irq implementation and does not use pci_bus->msi. - everything else that supports MSI has a modern driver with multiplatform support and uses msi_controller. If any platform wants to support GICv2m, we have to use CONFIG_PCI_MSI_IRQ_DOMAIN for all of them, and that seems like the best way forward. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jan 02, 2015 at 09:09:33PM +0000, Arnd Bergmann wrote: > On Friday 02 January 2015 12:18:06 Suravee Suthikulanit wrote: > > On 1/2/2015 5:55 AM, Lorenzo Pieralisi wrote: > > > Hi Suravee, > > > > > > On Mon, Dec 29, 2014 at 07:32:44PM +0000, Suravee Suthikulpanit wrote: > > >> >Hi, > > >> > > > >> >I am not sure if this thread is still alive. I'm trying to see what I > > >> >can do to help clean up/convert to make the PCI GHC also works for arm64 > > >> >w/ zero or minimal ifdefs. > > >> > > > >> >Please let me know if someone is already working on this. I noticed that > > >> >Lorenzo's patches has already been in 3.19-rc1, and in Bjorn's > > >> >pci/domain branch. Otherwise, I'll try to continue the work based on the > > >> >sample patch from Arnd here. > > > If I am not mistaken, the only bit missing to remove pci_sys_data (and so > > > having a generic host controller driver that works on ARM32/64) is generic > > > MSI management. > > > > Lorenzo, > > > > Do you mean to remove pci_sys_data from pci-host-generic.c or removing > > it completely? I assume the former case. > > Something inbetween: We should be able to remove pci_sys_data and > pci_common_init_dev from all drivers in drivers/pci/host/, but keep them > for all drivers in arch/arm/*/pci.c > > > So, looking at the current code in the pci-host-generic.c, my > > understanding is that the: > > *gen_pci = pci_bus->sysdata->private_data > > will be changed to: > > *gen_pci = pci_bus->sysdata > > > > Then, we can simply just call pci_scan_root_bus() directly since we no > > longer need to declare hw_pci for calling pci_common_init_dev(). > > Right. > > > > I know for certain Marc is working on it, and the solution is WIP, > > > I think we should prevent adding more churn to pci_sys_data, since > > > I managed to remove most of the dependencies (domain, mem_offset). > > > > Thanks for cleaning up the domain and mem_offset. > > > > I saw Marc's irq/msi_domain patch series > > (http://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/log/?h=irq/msi_domain). > > > > > > My understanding is that deals with associating the newly introduced > > msi_domain to each device, which replaces the need for pci_bus->msi and > > hw_pci->msi_ctrl when configure with CONFIG_PCI_MSI_IRQ_DOMAIN (not sure > > if this would be the plan for all arm32). For ARM32, if not define > > CONFIG_PCI_MSI_IRQ_DOMAIN, it would still fall back to using the > > [pci_sys_data|hw_pci]->msi_ctrl. > > For all I can tell, we have two cases on ARM regarding MSI: > > - arch/arm/mach-iop13xx/pci.c uses its own > arch_setup_msi_irq/arch_teardown_msi_irq implementation and does not > use pci_bus->msi. > > - everything else that supports MSI has a modern driver with multiplatform > support and uses msi_controller. If any platform wants to support GICv2m, > we have to use CONFIG_PCI_MSI_IRQ_DOMAIN for all of them, and that > seems like the best way forward. Yes, I think that's the current situation. With a hook (added by Marc's code) in PCI core to set the msi domain (DT), all we need to do is convert the generic host controller code to pci_scan_root_bus() and remove the pcibios init calls, it is a mechanical change when all patches mentioned above are merged. Thanks, Lorenzo -- To unsubscribe from this list: send the line "unsubscribe devicetree" 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; }