| Message ID | 1430307599-20536-1-git-send-email-jchandra@broadcom.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Jayachandran, On Wed, Apr 29, 2015 at 12:39:58PM +0100, Jayachandran C wrote: > The current code in pci-host-generic.c uses pci_common_init_dev() > from the arch/arm/ to do a part of the PCI initialization, and this > prevents it from being used on arm64. > > The initialization done by pci_common_init_dev() that is really > needed by pci-host-generic.c can be done in the same file without > using the hw_pci API of ARM. > > The ARM platform requires a pci_sys_data as sysdata for the PCI bus, > this is be handled by setting up 'struct gen_pci' to embed a > pci_sys_data variable as the first element on the ARM platform. > > Signed-off-by: Jayachandran C <jchandra@broadcom.com> > --- > > Notes: > - passing a zeroed out pci_sys_data for ARM looks ok, but I haven't > tested it on ARM. > - tested it on ARM64 fast model > - Any information on how this can be tested on arm is welcome. You should be able to test using a 32-bit guest under KVM. > - There is only one ifdef, and that can be removed when arm64 gets > a sysdata, or when arm loses its sysdata. Anyway, I believe Lorenzo (CC'd) has already been working on this so I'll let him comment here. Will > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c > index ba46e58..1631d34 100644 > --- a/drivers/pci/host/pci-host-generic.c > +++ b/drivers/pci/host/pci-host-generic.c > @@ -39,6 +39,9 @@ struct gen_pci_cfg_windows { > }; > > struct gen_pci { > +#ifdef CONFIG_ARM > + struct pci_sys_data sys; > +#endif > struct pci_host_bridge host; > struct gen_pci_cfg_windows cfg; > struct list_head resources; > @@ -48,8 +51,7 @@ 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->sysdata; > resource_size_t idx = bus->number - pci->cfg.bus_range->start; > > return pci->cfg.win[idx] + ((devfn << 8) | where); > @@ -64,8 +66,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->sysdata; > resource_size_t idx = bus->number - pci->cfg.bus_range->start; > > return pci->cfg.win[idx] + ((devfn << 12) | where); > @@ -198,11 +199,33 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) > return 0; > } > > -static int gen_pci_setup(int nr, struct pci_sys_data *sys) > +static int gen_pci_init(struct device *dev, struct gen_pci *pci) > { > - struct gen_pci *pci = sys->private_data; > - list_splice_init(&pci->resources, &sys->resources); > - return 1; > + struct pci_bus *bus; > + > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); > + > + bus = pci_scan_root_bus(dev, 0, &gen_pci_ops, pci, &pci->resources); > + if (!bus) { > + dev_err(dev, "Scanning rootbus failed"); > + return -ENODEV; > + } > + pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci); > + > + if (!pci_has_flag(PCI_PROBE_ONLY)) { > + pci_bus_size_bridges(bus); > + pci_bus_assign_resources(bus); > + } > + pci_bus_add_devices(bus); > + > + /* Configure PCI Express settings */ > + if (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; > } > > static int gen_pci_probe(struct platform_device *pdev) > @@ -214,13 +237,6 @@ 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; > @@ -258,8 +274,7 @@ static int gen_pci_probe(struct platform_device *pdev) > return err; > } > > - pci_common_init_dev(dev, &hw); > - return 0; > + return gen_pci_init(dev, pci); > } > > static struct platform_driver gen_pci_driver = { > -- > 1.9.1 >
On Wednesday 29 April 2015 17:09:58 Jayachandran C wrote: > The current code in pci-host-generic.c uses pci_common_init_dev() > from the arch/arm/ to do a part of the PCI initialization, and this > prevents it from being used on arm64. > > The initialization done by pci_common_init_dev() that is really > needed by pci-host-generic.c can be done in the same file without > using the hw_pci API of ARM. > > The ARM platform requires a pci_sys_data as sysdata for the PCI bus, > this is be handled by setting up 'struct gen_pci' to embed a > pci_sys_data variable as the first element on the ARM platform. > > Signed-off-by: Jayachandran C <jchandra@broadcom.com> This seems very useful > > -static int gen_pci_setup(int nr, struct pci_sys_data *sys) > +static int gen_pci_init(struct device *dev, struct gen_pci *pci) > { > - struct gen_pci *pci = sys->private_data; > - list_splice_init(&pci->resources, &sys->resources); > - return 1; > + struct pci_bus *bus; > + > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); Do we want to set that flag unconditionally? At least for servers, the resources should get assigned by firmware, and things might go wrong if Linux tries to reassign them. This is probably the case on kvmtool as well. > + bus = pci_scan_root_bus(dev, 0, &gen_pci_ops, pci, &pci->resources); > + if (!bus) { > + dev_err(dev, "Scanning rootbus failed"); > + return -ENODEV; > + } > + pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci); I thought this was done by default now. If it is not, can we make the of_irq swizzling the default? > + if (!pci_has_flag(PCI_PROBE_ONLY)) { > + pci_bus_size_bridges(bus); > + pci_bus_assign_resources(bus); > + } > + pci_bus_add_devices(bus); > + > + /* Configure PCI Express settings */ > + if (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; > } We should also try to come up wtih a way to make that pcie_bus_configure_settings() automatic if it's required here. > static int gen_pci_probe(struct platform_device *pdev) > @@ -214,13 +237,6 @@ 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; > @@ -258,8 +274,7 @@ static int gen_pci_probe(struct platform_device *pdev) > return err; > } > > - pci_common_init_dev(dev, &hw); > - return 0; > + return gen_pci_init(dev, pci); How about moving the new code right into the probe() function? Arnd
Hi Arnd, On Wed, Apr 29, 2015 at 02:34:20PM +0200, Arnd Bergmann wrote: > On Wednesday 29 April 2015 17:09:58 Jayachandran C wrote: > > The current code in pci-host-generic.c uses pci_common_init_dev() > > from the arch/arm/ to do a part of the PCI initialization, and this > > prevents it from being used on arm64. > > > > The initialization done by pci_common_init_dev() that is really > > needed by pci-host-generic.c can be done in the same file without > > using the hw_pci API of ARM. > > > > The ARM platform requires a pci_sys_data as sysdata for the PCI bus, > > this is be handled by setting up 'struct gen_pci' to embed a > > pci_sys_data variable as the first element on the ARM platform. > > > > Signed-off-by: Jayachandran C <jchandra@broadcom.com> > > This seems very useful I have tried to ensure that the new code is as much as possible functionally equivalent to the call to pci_common_init_dev on ARM. I should have added this as a note to the patch. Ideally, I would like this patch to be functionally equivalent to the existing code, and make further improvements with follow up patches, but I can make the changes you suggest if it is needed. > > > > > -static int gen_pci_setup(int nr, struct pci_sys_data *sys) > > +static int gen_pci_init(struct device *dev, struct gen_pci *pci) > > { > > - struct gen_pci *pci = sys->private_data; > > - list_splice_init(&pci->resources, &sys->resources); > > - return 1; > > + struct pci_bus *bus; > > + > > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); > > Do we want to set that flag unconditionally? At least for servers, > the resources should get assigned by firmware, and things might > go wrong if Linux tries to reassign them. This is probably the > case on kvmtool as well. > > > + bus = pci_scan_root_bus(dev, 0, &gen_pci_ops, pci, &pci->resources); > > + if (!bus) { > > + dev_err(dev, "Scanning rootbus failed"); > > + return -ENODEV; > > + } > > + pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci); > > I thought this was done by default now. If it is not, can > we make the of_irq swizzling the default? > > > + if (!pci_has_flag(PCI_PROBE_ONLY)) { > > + pci_bus_size_bridges(bus); > > + pci_bus_assign_resources(bus); > > + } > > + pci_bus_add_devices(bus); > > + > > + /* Configure PCI Express settings */ > > + if (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; > > } > > We should also try to come up wtih a way to make that > pcie_bus_configure_settings() automatic if it's required here. > > > static int gen_pci_probe(struct platform_device *pdev) > > @@ -214,13 +237,6 @@ 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; > > @@ -258,8 +274,7 @@ static int gen_pci_probe(struct platform_device *pdev) > > return err; > > } > > > > - pci_common_init_dev(dev, &hw); > > - return 0; > > + return gen_pci_init(dev, pci); > > How about moving the new code right into the probe() function? > > Arnd Thanks, JC.
On Wednesday 29 April 2015 19:55:55 Jayachandran C. wrote: > On Wed, Apr 29, 2015 at 02:34:20PM +0200, Arnd Bergmann wrote: > > On Wednesday 29 April 2015 17:09:58 Jayachandran C wrote: > > > The current code in pci-host-generic.c uses pci_common_init_dev() > > > from the arch/arm/ to do a part of the PCI initialization, and this > > > prevents it from being used on arm64. > > > > > > The initialization done by pci_common_init_dev() that is really > > > needed by pci-host-generic.c can be done in the same file without > > > using the hw_pci API of ARM. > > > > > > The ARM platform requires a pci_sys_data as sysdata for the PCI bus, > > > this is be handled by setting up 'struct gen_pci' to embed a > > > pci_sys_data variable as the first element on the ARM platform. > > > > > > Signed-off-by: Jayachandran C <jchandra@broadcom.com> > > > > This seems very useful > > I have tried to ensure that the new code is as much as possible > functionally equivalent to the call to pci_common_init_dev on ARM. > > I should have added this as a note to the patch. > > Ideally, I would like this patch to be functionally equivalent > to the existing code, and make further improvements with follow up > patches, but I can make the changes you suggest if it is needed. I believe the driver is only used in virtual machines at the moment, so it should be easy enough to test the relevant cases on arm32 and make it as simple as possible. For bisection purposes, it could however be better to start with your current patch and then follow it up with a second patch that removes the unnecessary parts again immediately. Arnd
On Wed, Apr 29, 2015 at 01:34:20PM +0100, Arnd Bergmann wrote: > On Wednesday 29 April 2015 17:09:58 Jayachandran C wrote: > > The current code in pci-host-generic.c uses pci_common_init_dev() > > from the arch/arm/ to do a part of the PCI initialization, and this > > prevents it from being used on arm64. > > > > The initialization done by pci_common_init_dev() that is really > > needed by pci-host-generic.c can be done in the same file without > > using the hw_pci API of ARM. > > > > The ARM platform requires a pci_sys_data as sysdata for the PCI bus, > > this is be handled by setting up 'struct gen_pci' to embed a > > pci_sys_data variable as the first element on the ARM platform. > > > > Signed-off-by: Jayachandran C <jchandra@broadcom.com> > > This seems very useful Yes, it is getting less awful, waiting for pci_sys_data to disappear. > > -static int gen_pci_setup(int nr, struct pci_sys_data *sys) > > +static int gen_pci_init(struct device *dev, struct gen_pci *pci) > > { > > - struct gen_pci *pci = sys->private_data; > > - list_splice_init(&pci->resources, &sys->resources); > > - return 1; > > + struct pci_bus *bus; > > + > > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); > > Do we want to set that flag unconditionally? At least for servers, > the resources should get assigned by firmware, and things might > go wrong if Linux tries to reassign them. This is probably the > case on kvmtool as well. I will give it a go on kvmtool, but at first glance concern is shared. > > + bus = pci_scan_root_bus(dev, 0, &gen_pci_ops, pci, &pci->resources); > > + if (!bus) { > > + dev_err(dev, "Scanning rootbus failed"); > > + return -ENODEV; > > + } > > + pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci); > > I thought this was done by default now. If it is not, can > we make the of_irq swizzling the default? Possibly yes. > > + if (!pci_has_flag(PCI_PROBE_ONLY)) { > > + pci_bus_size_bridges(bus); > > + pci_bus_assign_resources(bus); > > + } > > + pci_bus_add_devices(bus); > > + > > + /* Configure PCI Express settings */ > > + if (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; > > } > > We should also try to come up wtih a way to make that > pcie_bus_configure_settings() automatic if it's required here. > > > static int gen_pci_probe(struct platform_device *pdev) > > @@ -214,13 +237,6 @@ 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; > > @@ -258,8 +274,7 @@ static int gen_pci_probe(struct platform_device *pdev) > > return err; > > } > > > > - pci_common_init_dev(dev, &hw); > > - return 0; > > + return gen_pci_init(dev, pci); > > How about moving the new code right into the probe() function? +1. I suspect the PCI_PROBE_ONLY case was not tested on arm64, since we still need a patch to prevent resources enablement there (in the pcibios_enable_device call - actually the check for PCI_PROBE_ONLY should be moved to the default pcibios_enable_device function in drivers/pci/pci.c). The other bit missing is MSI handling, that should still be implemented. Lorenzo
On Wed, Apr 29, 2015 at 06:43:57PM +0100, Lorenzo Pieralisi wrote: > On Wed, Apr 29, 2015 at 01:34:20PM +0100, Arnd Bergmann wrote: > > On Wednesday 29 April 2015 17:09:58 Jayachandran C wrote: > > > The current code in pci-host-generic.c uses pci_common_init_dev() > > > from the arch/arm/ to do a part of the PCI initialization, and this > > > prevents it from being used on arm64. > > > > > > The initialization done by pci_common_init_dev() that is really > > > needed by pci-host-generic.c can be done in the same file without > > > using the hw_pci API of ARM. > > > > > > The ARM platform requires a pci_sys_data as sysdata for the PCI bus, > > > this is be handled by setting up 'struct gen_pci' to embed a > > > pci_sys_data variable as the first element on the ARM platform. > > > > > > Signed-off-by: Jayachandran C <jchandra@broadcom.com> > > > > This seems very useful > > Yes, it is getting less awful, waiting for pci_sys_data to disappear. > > > > -static int gen_pci_setup(int nr, struct pci_sys_data *sys) > > > +static int gen_pci_init(struct device *dev, struct gen_pci *pci) > > > { > > > - struct gen_pci *pci = sys->private_data; > > > - list_splice_init(&pci->resources, &sys->resources); > > > - return 1; > > > + struct pci_bus *bus; > > > + > > > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); > > > > Do we want to set that flag unconditionally? At least for servers, > > the resources should get assigned by firmware, and things might > > go wrong if Linux tries to reassign them. This is probably the > > case on kvmtool as well. I can skip setting this flag if PCI_PROBE_ONLY is set. Adding another device tree parameter for this may not be needed. > I will give it a go on kvmtool, but at first glance concern is > shared. > > > > + bus = pci_scan_root_bus(dev, 0, &gen_pci_ops, pci, &pci->resources); > > > + if (!bus) { > > > + dev_err(dev, "Scanning rootbus failed"); > > > + return -ENODEV; > > > + } > > > + pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci); > > > > I thought this was done by default now. If it is not, can > > we make the of_irq swizzling the default? > > Possibly yes. I am not sure I understand this, I thought pci_fixup_irqs needs to be called at this point. > > > + if (!pci_has_flag(PCI_PROBE_ONLY)) { > > > + pci_bus_size_bridges(bus); > > > + pci_bus_assign_resources(bus); > > > + } > > > + pci_bus_add_devices(bus); > > > + > > > + /* Configure PCI Express settings */ > > > + if (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; > > > } > > > > We should also try to come up wtih a way to make that > > pcie_bus_configure_settings() automatic if it's required here. > > > > > static int gen_pci_probe(struct platform_device *pdev) > > > @@ -214,13 +237,6 @@ 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; > > > @@ -258,8 +274,7 @@ static int gen_pci_probe(struct platform_device *pdev) > > > return err; > > > } > > > > > > - pci_common_init_dev(dev, &hw); > > > - return 0; > > > + return gen_pci_init(dev, pci); > > > > How about moving the new code right into the probe() function? > > +1. I will add this to patch v2. > I suspect the PCI_PROBE_ONLY case was not tested on arm64, since we > still need a patch to prevent resources enablement there (in the > pcibios_enable_device call - actually the check for PCI_PROBE_ONLY > should be moved to the default pcibios_enable_device function in > drivers/pci/pci.c). > > The other bit missing is MSI handling, that should still be implemented. I think we should parse msi-parent and set ->msi on the root bus in pci-host-generic.c. The implementation of pcibios_msi_controller() for arm/arm64 can go up the bus hierarchy and return the first msi controller it finds. This would be trivial to add to arm/arm64. I can post a follow up patch for this if you are interested. If there are no additional concerns, I can post v2 of the patch after testing on arm. Thanks, JC.
On Thu, Apr 30, 2015 at 10:59:50AM +0100, Jayachandran C. wrote: > On Wed, Apr 29, 2015 at 06:43:57PM +0100, Lorenzo Pieralisi wrote: > > On Wed, Apr 29, 2015 at 01:34:20PM +0100, Arnd Bergmann wrote: > > > On Wednesday 29 April 2015 17:09:58 Jayachandran C wrote: > > > > The current code in pci-host-generic.c uses pci_common_init_dev() > > > > from the arch/arm/ to do a part of the PCI initialization, and this > > > > prevents it from being used on arm64. > > > > > > > > The initialization done by pci_common_init_dev() that is really > > > > needed by pci-host-generic.c can be done in the same file without > > > > using the hw_pci API of ARM. > > > > > > > > The ARM platform requires a pci_sys_data as sysdata for the PCI bus, > > > > this is be handled by setting up 'struct gen_pci' to embed a > > > > pci_sys_data variable as the first element on the ARM platform. > > > > > > > > Signed-off-by: Jayachandran C <jchandra@broadcom.com> > > > > > > This seems very useful > > > > Yes, it is getting less awful, waiting for pci_sys_data to disappear. > > > > > > -static int gen_pci_setup(int nr, struct pci_sys_data *sys) > > > > +static int gen_pci_init(struct device *dev, struct gen_pci *pci) > > > > { > > > > - struct gen_pci *pci = sys->private_data; > > > > - list_splice_init(&pci->resources, &sys->resources); > > > > - return 1; > > > > + struct pci_bus *bus; > > > > + > > > > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); > > > > > > Do we want to set that flag unconditionally? At least for servers, > > > the resources should get assigned by firmware, and things might > > > go wrong if Linux tries to reassign them. This is probably the > > > case on kvmtool as well. > > I can skip setting this flag if PCI_PROBE_ONLY is set. Adding another > device tree parameter for this may not be needed. I think that's reasonable for the time being. > > I will give it a go on kvmtool, but at first glance concern is > > shared. > > > > > > + bus = pci_scan_root_bus(dev, 0, &gen_pci_ops, pci, &pci->resources); > > > > + if (!bus) { > > > > + dev_err(dev, "Scanning rootbus failed"); > > > > + return -ENODEV; > > > > + } > > > > + pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci); > > > > > > I thought this was done by default now. If it is not, can > > > we make the of_irq swizzling the default? > > > > Possibly yes. > > I am not sure I understand this, I thought pci_fixup_irqs needs to be > called at this point. What we meant is that passing of_irq_parse_and_map_pci as a mapping function is the most common option, but I do not think there is much you can do at the moment apart from adding a pci_of_fixup_irqs function to the generic PCI layer and calling that instead, I do not see where the improvement is, so I think you can leave it as it is. > > > > + if (!pci_has_flag(PCI_PROBE_ONLY)) { > > > > + pci_bus_size_bridges(bus); > > > > + pci_bus_assign_resources(bus); > > > > + } > > > > + pci_bus_add_devices(bus); > > > > + > > > > + /* Configure PCI Express settings */ > > > > + if (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; > > > > } > > > > > > We should also try to come up wtih a way to make that > > > pcie_bus_configure_settings() automatic if it's required here. > > > > > > > static int gen_pci_probe(struct platform_device *pdev) > > > > @@ -214,13 +237,6 @@ 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; > > > > @@ -258,8 +274,7 @@ static int gen_pci_probe(struct platform_device *pdev) > > > > return err; > > > > } > > > > > > > > - pci_common_init_dev(dev, &hw); > > > > - return 0; > > > > + return gen_pci_init(dev, pci); > > > > > > How about moving the new code right into the probe() function? > > > > +1. > > I will add this to patch v2. > > > I suspect the PCI_PROBE_ONLY case was not tested on arm64, since we > > still need a patch to prevent resources enablement there (in the > > pcibios_enable_device call - actually the check for PCI_PROBE_ONLY > > should be moved to the default pcibios_enable_device function in > > drivers/pci/pci.c). > > > > The other bit missing is MSI handling, that should still be implemented. > > I think we should parse msi-parent and set ->msi on the root bus in > pci-host-generic.c. The implementation of pcibios_msi_controller() for > arm/arm64 can go up the bus hierarchy and return the first msi > controller it finds. This would be trivial to add to arm/arm64. > I can post a follow up patch for this if you are interested. I do not want to see a pcibios_msi_controller implementation on arm64, or put it differently what you are proposing has nothing in it arm64 specific, we need to find a solution that goes into generic code, there is already, pending further discussion: http://lwn.net/Articles/628872/ Thanks, Lorenzo
On Fri, May 01, 2015 at 09:40:14AM +0100, Lorenzo Pieralisi wrote: > On Thu, Apr 30, 2015 at 10:59:50AM +0100, Jayachandran C. wrote: > > On Wed, Apr 29, 2015 at 06:43:57PM +0100, Lorenzo Pieralisi wrote: > > > On Wed, Apr 29, 2015 at 01:34:20PM +0100, Arnd Bergmann wrote: > > > > On Wednesday 29 April 2015 17:09:58 Jayachandran C wrote: > > > > > The current code in pci-host-generic.c uses pci_common_init_dev() > > > > > from the arch/arm/ to do a part of the PCI initialization, and this > > > > > prevents it from being used on arm64. > > > > > > > > > > The initialization done by pci_common_init_dev() that is really > > > > > needed by pci-host-generic.c can be done in the same file without > > > > > using the hw_pci API of ARM. > > > > > > > > > > The ARM platform requires a pci_sys_data as sysdata for the PCI bus, > > > > > this is be handled by setting up 'struct gen_pci' to embed a > > > > > pci_sys_data variable as the first element on the ARM platform. > > > > > > > > > > Signed-off-by: Jayachandran C <jchandra@broadcom.com> > > > > > > > > This seems very useful > > > > > > Yes, it is getting less awful, waiting for pci_sys_data to disappear. > > > > > > > > -static int gen_pci_setup(int nr, struct pci_sys_data *sys) > > > > > +static int gen_pci_init(struct device *dev, struct gen_pci *pci) > > > > > { > > > > > - struct gen_pci *pci = sys->private_data; > > > > > - list_splice_init(&pci->resources, &sys->resources); > > > > > - return 1; > > > > > + struct pci_bus *bus; > > > > > + > > > > > + pci_add_flags(PCI_REASSIGN_ALL_RSRC); > > > > > > > > Do we want to set that flag unconditionally? At least for servers, > > > > the resources should get assigned by firmware, and things might > > > > go wrong if Linux tries to reassign them. This is probably the > > > > case on kvmtool as well. > > > > I can skip setting this flag if PCI_PROBE_ONLY is set. Adding another > > device tree parameter for this may not be needed. > > I think that's reasonable for the time being. > > > > I will give it a go on kvmtool, but at first glance concern is > > > shared. > > > > > > > > + bus = pci_scan_root_bus(dev, 0, &gen_pci_ops, pci, &pci->resources); > > > > > + if (!bus) { > > > > > + dev_err(dev, "Scanning rootbus failed"); > > > > > + return -ENODEV; > > > > > + } > > > > > + pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci); > > > > > > > > I thought this was done by default now. If it is not, can > > > > we make the of_irq swizzling the default? > > > > > > Possibly yes. > > > > I am not sure I understand this, I thought pci_fixup_irqs needs to be > > called at this point. > > What we meant is that passing of_irq_parse_and_map_pci as a mapping > function is the most common option, but I do not think there is much > you can do at the moment apart from adding a pci_of_fixup_irqs function > to the generic PCI layer and calling that instead, I do not see where > the improvement is, so I think you can leave it as it is. > > > > > > + if (!pci_has_flag(PCI_PROBE_ONLY)) { > > > > > + pci_bus_size_bridges(bus); > > > > > + pci_bus_assign_resources(bus); > > > > > + } > > > > > + pci_bus_add_devices(bus); > > > > > + > > > > > + /* Configure PCI Express settings */ > > > > > + if (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; > > > > > } > > > > > > > > We should also try to come up wtih a way to make that > > > > pcie_bus_configure_settings() automatic if it's required here. > > > > > > > > > static int gen_pci_probe(struct platform_device *pdev) > > > > > @@ -214,13 +237,6 @@ 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; > > > > > @@ -258,8 +274,7 @@ static int gen_pci_probe(struct platform_device *pdev) > > > > > return err; > > > > > } > > > > > > > > > > - pci_common_init_dev(dev, &hw); > > > > > - return 0; > > > > > + return gen_pci_init(dev, pci); > > > > > > > > How about moving the new code right into the probe() function? > > > > > > +1. > > > > I will add this to patch v2. > > > > > I suspect the PCI_PROBE_ONLY case was not tested on arm64, since we > > > still need a patch to prevent resources enablement there (in the > > > pcibios_enable_device call - actually the check for PCI_PROBE_ONLY > > > should be moved to the default pcibios_enable_device function in > > > drivers/pci/pci.c). > > > > > > The other bit missing is MSI handling, that should still be implemented. > > > > I think we should parse msi-parent and set ->msi on the root bus in > > pci-host-generic.c. The implementation of pcibios_msi_controller() for > > arm/arm64 can go up the bus hierarchy and return the first msi > > controller it finds. This would be trivial to add to arm/arm64. > > I can post a follow up patch for this if you are interested. > > I do not want to see a pcibios_msi_controller implementation on arm64, > or put it differently what you are proposing has nothing in it arm64 > specific, we need to find a solution that goes into generic code, there > is already, pending further discussion: > > http://lwn.net/Articles/628872/ The default (weak) implementation of pcibios_msi_controller() could go up the parent buses to the root bus and return the first MSI controller, instead of retuning NULL always. This would also eliminate the need to keep the msi_ctrl in sysdata for ARM. Anyway, I will do v2 of this patchset and leave MSI for another patchset/RFC. Thanks, JC.
On 4/29/15 12:43, Lorenzo Pieralisi wrote: > On Wed, Apr 29, 2015 at 01:34:20PM +0100, Arnd Bergmann wrote: >> >On Wednesday 29 April 2015 17:09:58 Jayachandran C wrote: >>> > >The current code in pci-host-generic.c uses pci_common_init_dev() >>> > >from the arch/arm/ to do a part of the PCI initialization, and this >>> > >prevents it from being used on arm64. >>> > > >>> > >The initialization done by pci_common_init_dev() that is really >>> > >needed by pci-host-generic.c can be done in the same file without >>> > >using the hw_pci API of ARM. >>> > > >>> > >The ARM platform requires a pci_sys_data as sysdata for the PCI bus, >>> > >this is be handled by setting up 'struct gen_pci' to embed a >>> > >pci_sys_data variable as the first element on the ARM platform. >>> > > >>> > >Signed-off-by: Jayachandran C<jchandra@broadcom.com> >> > >> >This seems very useful > Yes, it is getting less awful, waiting for pci_sys_data to disappear. > Lorenzo, A while back, you mentioned here (https://lkml.org/lkml/2015/2/16/364) that the ARM32 pcibios_align_resource() implementation requires pci_sys_data, so we _still_ rely on pci_common_init_dev to create one for us. Is this still the case? I am looking at the arch/arm32/kernel/bios32.c: pcibios_init_hw() and see that it setup the pci_sys_data.align_resource to hw_pci.align_resource (see here http://lxr.free-electrons.com/source/arch/arm/kernel/bios32.c#L471). However it seems that the hw_pci.align_resource is never setup in the pci-host-generic.c. Am I missing something here? Thanks, Suravee
On Sun, May 03, 2015 at 04:06:15PM -0500, Suravee Suthikulpanit wrote: > > > On 4/29/15 12:43, Lorenzo Pieralisi wrote: > >On Wed, Apr 29, 2015 at 01:34:20PM +0100, Arnd Bergmann wrote: > >>>On Wednesday 29 April 2015 17:09:58 Jayachandran C wrote: > >>>> >The current code in pci-host-generic.c uses pci_common_init_dev() > >>>> >from the arch/arm/ to do a part of the PCI initialization, and this > >>>> >prevents it from being used on arm64. > >>>> > > >>>> >The initialization done by pci_common_init_dev() that is really > >>>> >needed by pci-host-generic.c can be done in the same file without > >>>> >using the hw_pci API of ARM. > >>>> > > >>>> >The ARM platform requires a pci_sys_data as sysdata for the PCI bus, > >>>> >this is be handled by setting up 'struct gen_pci' to embed a > >>>> >pci_sys_data variable as the first element on the ARM platform. > >>>> > > >>>> >Signed-off-by: Jayachandran C<jchandra@broadcom.com> > >>> > >>>This seems very useful > >Yes, it is getting less awful, waiting for pci_sys_data to disappear. > > > > Lorenzo, > > A while back, you mentioned here > (https://lkml.org/lkml/2015/2/16/364) that the ARM32 > pcibios_align_resource() implementation requires > pci_sys_data, so we _still_ rely on pci_common_init_dev to create one > for us. Is this still the case? > > I am looking at the arch/arm32/kernel/bios32.c: pcibios_init_hw() > and see that it setup the pci_sys_data.align_resource to > hw_pci.align_resource (see here > http://lxr.free-electrons.com/source/arch/arm/kernel/bios32.c#L471). > > However it seems that the hw_pci.align_resource is never setup in > the pci-host-generic.c. Am I missing something here? ARM32 needs a sysdata because functions like pcibios_align_resource() will dereference and access members of this structure. However having an allocated but zeroed sysdata is fine, since the function pointers in sysdata (as well as things like busnr, msi_ctrl) will be 0/NULL and pcibios_align_resource will do the default handling. If this patch needs changes to work on your platform please let me know. Thanks, JC.
diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c index ba46e58..1631d34 100644 --- a/drivers/pci/host/pci-host-generic.c +++ b/drivers/pci/host/pci-host-generic.c @@ -39,6 +39,9 @@ struct gen_pci_cfg_windows { }; struct gen_pci { +#ifdef CONFIG_ARM + struct pci_sys_data sys; +#endif struct pci_host_bridge host; struct gen_pci_cfg_windows cfg; struct list_head resources; @@ -48,8 +51,7 @@ 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->sysdata; resource_size_t idx = bus->number - pci->cfg.bus_range->start; return pci->cfg.win[idx] + ((devfn << 8) | where); @@ -64,8 +66,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->sysdata; resource_size_t idx = bus->number - pci->cfg.bus_range->start; return pci->cfg.win[idx] + ((devfn << 12) | where); @@ -198,11 +199,33 @@ static int gen_pci_parse_map_cfg_windows(struct gen_pci *pci) return 0; } -static int gen_pci_setup(int nr, struct pci_sys_data *sys) +static int gen_pci_init(struct device *dev, struct gen_pci *pci) { - struct gen_pci *pci = sys->private_data; - list_splice_init(&pci->resources, &sys->resources); - return 1; + struct pci_bus *bus; + + pci_add_flags(PCI_REASSIGN_ALL_RSRC); + + bus = pci_scan_root_bus(dev, 0, &gen_pci_ops, pci, &pci->resources); + if (!bus) { + dev_err(dev, "Scanning rootbus failed"); + return -ENODEV; + } + pci_fixup_irqs(pci_common_swizzle, of_irq_parse_and_map_pci); + + if (!pci_has_flag(PCI_PROBE_ONLY)) { + pci_bus_size_bridges(bus); + pci_bus_assign_resources(bus); + } + pci_bus_add_devices(bus); + + /* Configure PCI Express settings */ + if (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; } static int gen_pci_probe(struct platform_device *pdev) @@ -214,13 +237,6 @@ 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; @@ -258,8 +274,7 @@ static int gen_pci_probe(struct platform_device *pdev) return err; } - pci_common_init_dev(dev, &hw); - return 0; + return gen_pci_init(dev, pci); } static struct platform_driver gen_pci_driver = {
The current code in pci-host-generic.c uses pci_common_init_dev() from the arch/arm/ to do a part of the PCI initialization, and this prevents it from being used on arm64. The initialization done by pci_common_init_dev() that is really needed by pci-host-generic.c can be done in the same file without using the hw_pci API of ARM. The ARM platform requires a pci_sys_data as sysdata for the PCI bus, this is be handled by setting up 'struct gen_pci' to embed a pci_sys_data variable as the first element on the ARM platform. Signed-off-by: Jayachandran C <jchandra@broadcom.com> --- Notes: - passing a zeroed out pci_sys_data for ARM looks ok, but I haven't tested it on ARM. - tested it on ARM64 fast model - Any information on how this can be tested on arm is welcome. - There is only one ifdef, and that can be removed when arm64 gets a sysdata, or when arm loses its sysdata. drivers/pci/host/pci-host-generic.c | 49 ++++++++++++++++++++++++------------- 1 file changed, 32 insertions(+), 17 deletions(-)