| Message ID | 1447095459-21777-1-git-send-email-lorenzo.pieralisi@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday 09 November 2015 18:57:39 Lorenzo Pieralisi wrote: > Commit b3a72384fe29 ("ARM/PCI: Replace pci_sys_data->align_resource > with global function pointer") removed the struct pci_sys_data > dependency from the ARM pcibios functions that are part of the > common ARM PCI arch back-end (eg pcibios_align_resource()), so that > struct pci_sys_data has now become data that is only used internally > by the ARM bios32 layer (ie pci_common_init_dev()) and by host > controllers drivers callbacks (eg pci_sys_data.setup) that rely on the > ARM bios32 API to probe. > > PCI host controller drivers that do not rely on ARM bios32 calls to > probe do not need to have the pci_bus.sysdata pointer field pointing > at a struct pci_sys_data anymore, therefore it can be removed from the > respective drivers data structures. > > This patch removes the pci_sys_data structures from the host > controller drivers that do not rely on ARM bios32 interface to > scan the PCI bus, completing the pci_sys_data clean-up and removing > the related dependency on arch/arm specific data. > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Will Deacon <will.deacon@arm.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Rob Herring <robh@kernel.org> > Nice! Acked-by: Arnd Bergmann <arnd@arndb.de> On the downside, I just saw how b3a72384fe29 ("ARM/PCI: Replace pci_sys_data->align_resource with global function pointer") manages to get rid of the last user of this, and how this is not the way we had planned for it to be done. It doesn't matter that much, because there is only a single user of hw->align_resource, but this is now unportable code and cannot easily be separated from hw_pci. It also breaks if we ever get a machine with two different host controllers on ARM that don't use the same pointer here. I really would have hoped we could put that function pointer into 'struct pci_host_bridge' instead, and eventually killed off the architecture specific pcibios_align_resource function as well. Any idea why it was done with a global function pointer for ARM rather than a proper solution? Arnd
Hi Arnd [...] > > Nice! > > Acked-by: Arnd Bergmann <arnd@arndb.de> > > On the downside, I just saw how b3a72384fe29 ("ARM/PCI: Replace > pci_sys_data->align_resource with global function pointer") manages > to get rid of the last user of this, and how this is not the way > we had planned for it to be done. > > It doesn't matter that much, because there is only a single > user of hw->align_resource, but this is now unportable code and > cannot easily be separated from hw_pci. It also breaks if we ever > get a machine with two different host controllers on ARM that > don't use the same pointer here. > > I really would have hoped we could put that function pointer > into 'struct pci_host_bridge' instead, and eventually killed > off the architecture specific pcibios_align_resource function > as well. say we move that in pci_host_bridge struct, Are you suggesting to modify pcibios_init_hw to assign the align_resource function pointer at the end after pci_scan_root_bus_msi() has created the host bridge structure; then we can retrieve it in pcibios_align_resource using pci_find_host_bridge(dev->bus)....? > > Any idea why it was done with a global function pointer for > ARM rather than a proper solution? Well just because I didn't think about the solution above... If you think the above one can work I can send out another patch to fix this... Many Thanks Gab > > Arnd
On Tuesday 10 November 2015 06:19:17 Gabriele Paoloni wrote: > > Acked-by: Arnd Bergmann <arnd@arndb.de> > > > > On the downside, I just saw how b3a72384fe29 ("ARM/PCI: Replace > > pci_sys_data->align_resource with global function pointer") manages > > to get rid of the last user of this, and how this is not the way > > we had planned for it to be done. > > > > It doesn't matter that much, because there is only a single > > user of hw->align_resource, but this is now unportable code and > > cannot easily be separated from hw_pci. It also breaks if we ever > > get a machine with two different host controllers on ARM that > > don't use the same pointer here. > > > > I really would have hoped we could put that function pointer > > into 'struct pci_host_bridge' instead, and eventually killed > > off the architecture specific pcibios_align_resource function > > as well. > > say we move that in pci_host_bridge struct, > > Are you suggesting to modify pcibios_init_hw to assign the > align_resource function pointer at the end after > pci_scan_root_bus_msi() has created the host bridge structure; > then we can retrieve it in pcibios_align_resource using > pci_find_host_bridge(dev->bus)....? What I had in mind was more along the lines of removing the any mention of the align_resource callback from the ARM bios32.c code and to have the pci-mvebu driver set the pointer manually at an appropriate point (I did not look what would be that point). What you suggest above would work too, just doesn't go as far. In the long run, I think it would be good to migrate all drivers in drivers/pci/host/ away from calling pcibios_init_hw() at all, and just use architecture-independent interfaces and leave the hw_pci/pci_sys_data based implementation for the traditional PCI host implementations that we have in arch/arm/mach-* for platforms that do not use devicetree. This is not something I'm asking you to do at this point, though any help here is appreciated. > > Any idea why it was done with a global function pointer for > > ARM rather than a proper solution? > > Well just because I didn't think about the solution above... > > If you think the above one can work I can send out another > patch to fix this... That would be great, yes. As I said, it's currently harmless but it would be better to get it right. Arnd
On Mon, Nov 09, 2015 at 06:57:39PM +0000, Lorenzo Pieralisi wrote: > Commit b3a72384fe29 ("ARM/PCI: Replace pci_sys_data->align_resource > with global function pointer") removed the struct pci_sys_data > dependency from the ARM pcibios functions that are part of the > common ARM PCI arch back-end (eg pcibios_align_resource()), so that > struct pci_sys_data has now become data that is only used internally > by the ARM bios32 layer (ie pci_common_init_dev()) and by host > controllers drivers callbacks (eg pci_sys_data.setup) that rely on the > ARM bios32 API to probe. > > PCI host controller drivers that do not rely on ARM bios32 calls to > probe do not need to have the pci_bus.sysdata pointer field pointing > at a struct pci_sys_data anymore, therefore it can be removed from the > respective drivers data structures. > > This patch removes the pci_sys_data structures from the host > controller drivers that do not rely on ARM bios32 interface to > scan the PCI bus, completing the pci_sys_data clean-up and removing > the related dependency on arch/arm specific data. > > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: Will Deacon <will.deacon@arm.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > Cc: Rob Herring <robh@kernel.org> Applied with Arnd's ack to pci/host-generic for v4.5, thanks! > --- > Rob, > > I could not test it on versatile, only compile tested, so > please have an additional look. > > Tested the PCI host generic on 32-bit and 64-bit guests, > through kvmtool. > > Thanks, > Lorenzo > > drivers/pci/host/pci-host-generic.c | 9 --------- > drivers/pci/host/pci-versatile.c | 5 +---- > 2 files changed, 1 insertion(+), 13 deletions(-) > > diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c > index 5434c90..1652bc7 100644 > --- a/drivers/pci/host/pci-host-generic.c > +++ b/drivers/pci/host/pci-host-generic.c > @@ -38,16 +38,7 @@ struct gen_pci_cfg_windows { > struct gen_pci_cfg_bus_ops *ops; > }; > > -/* > - * ARM pcibios functions expect the ARM struct pci_sys_data as the PCI > - * sysdata. Add pci_sys_data as the first element in struct gen_pci so > - * that when we use a gen_pci pointer as sysdata, it is also a pointer to > - * a struct pci_sys_data. > - */ > 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; > diff --git a/drivers/pci/host/pci-versatile.c b/drivers/pci/host/pci-versatile.c > index 0863d9c..f843a72 100644 > --- a/drivers/pci/host/pci-versatile.c > +++ b/drivers/pci/host/pci-versatile.c > @@ -125,9 +125,6 @@ out_release_res: > return err; > } > > -/* Unused, temporary to satisfy ARM arch code */ > -struct pci_sys_data sys; > - > static int versatile_pci_probe(struct platform_device *pdev) > { > struct resource *res; > @@ -208,7 +205,7 @@ static int versatile_pci_probe(struct platform_device *pdev) > pci_add_flags(PCI_ENABLE_PROC_DOMAINS); > pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC); > > - bus = pci_scan_root_bus(&pdev->dev, 0, &pci_versatile_ops, &sys, &pci_res); > + bus = pci_scan_root_bus(&pdev->dev, 0, &pci_versatile_ops, NULL, &pci_res); > if (!bus) > return -ENOMEM; > > -- > 2.5.1 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pci" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/host/pci-host-generic.c b/drivers/pci/host/pci-host-generic.c index 5434c90..1652bc7 100644 --- a/drivers/pci/host/pci-host-generic.c +++ b/drivers/pci/host/pci-host-generic.c @@ -38,16 +38,7 @@ struct gen_pci_cfg_windows { struct gen_pci_cfg_bus_ops *ops; }; -/* - * ARM pcibios functions expect the ARM struct pci_sys_data as the PCI - * sysdata. Add pci_sys_data as the first element in struct gen_pci so - * that when we use a gen_pci pointer as sysdata, it is also a pointer to - * a struct pci_sys_data. - */ 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; diff --git a/drivers/pci/host/pci-versatile.c b/drivers/pci/host/pci-versatile.c index 0863d9c..f843a72 100644 --- a/drivers/pci/host/pci-versatile.c +++ b/drivers/pci/host/pci-versatile.c @@ -125,9 +125,6 @@ out_release_res: return err; } -/* Unused, temporary to satisfy ARM arch code */ -struct pci_sys_data sys; - static int versatile_pci_probe(struct platform_device *pdev) { struct resource *res; @@ -208,7 +205,7 @@ static int versatile_pci_probe(struct platform_device *pdev) pci_add_flags(PCI_ENABLE_PROC_DOMAINS); pci_add_flags(PCI_REASSIGN_ALL_BUS | PCI_REASSIGN_ALL_RSRC); - bus = pci_scan_root_bus(&pdev->dev, 0, &pci_versatile_ops, &sys, &pci_res); + bus = pci_scan_root_bus(&pdev->dev, 0, &pci_versatile_ops, NULL, &pci_res); if (!bus) return -ENOMEM;
Commit b3a72384fe29 ("ARM/PCI: Replace pci_sys_data->align_resource with global function pointer") removed the struct pci_sys_data dependency from the ARM pcibios functions that are part of the common ARM PCI arch back-end (eg pcibios_align_resource()), so that struct pci_sys_data has now become data that is only used internally by the ARM bios32 layer (ie pci_common_init_dev()) and by host controllers drivers callbacks (eg pci_sys_data.setup) that rely on the ARM bios32 API to probe. PCI host controller drivers that do not rely on ARM bios32 calls to probe do not need to have the pci_bus.sysdata pointer field pointing at a struct pci_sys_data anymore, therefore it can be removed from the respective drivers data structures. This patch removes the pci_sys_data structures from the host controller drivers that do not rely on ARM bios32 interface to scan the PCI bus, completing the pci_sys_data clean-up and removing the related dependency on arch/arm specific data. Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: Will Deacon <will.deacon@arm.com> Cc: Bjorn Helgaas <bhelgaas@google.com> Cc: Rob Herring <robh@kernel.org> --- Rob, I could not test it on versatile, only compile tested, so please have an additional look. Tested the PCI host generic on 32-bit and 64-bit guests, through kvmtool. Thanks, Lorenzo drivers/pci/host/pci-host-generic.c | 9 --------- drivers/pci/host/pci-versatile.c | 5 +---- 2 files changed, 1 insertion(+), 13 deletions(-)