Re: [RFC/RFT PATCH 18/18] ARM/ARM64: PCI: Drop pci_fixup_irqs() usage for DT based host controllers

[Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>]

On Fri, Apr 28, 2017 at 03:05:44PM +0200, Arnd Bergmann wrote:
> On Wed, Apr 26, 2017 at 1:18 PM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > DT based PCI host controllers are currently relying on
> > pci_fixup_irqs() to assign legacy PCI irqs to devices. This is
> > broken in that pci_fixup_irqs() assign IRQs for all PCI devices
> > present in a given system some of which may well be enabled by
> > the time pci_fixup_irqs() is called (ie a system with multiple
> > host controllers). With the introduction of
> > struct pci_host_bridge.map_irq pointer it is possible to assign IRQs
> > for all devices originating from a PCI host bridge at probe time;
> > this is implemented through pci_assign_irq() that relies on the
> > struct pci_host_bridge.map_irq pointer to map IRQ for a given device.
> >
> > The benefits this brings are twofold:
> >
> > - the IRQ for a device is assigned once at probe time
> > - the IRQ assignment works also for hotplugged devices
> >
> > Remove pci_fixup_irqs() call from all DT based PCI host controller
> > drivers. The map_irq() and swizzle_irq() struct pci_host_bridge callbacks
> > are either set-up in the respective PCI host controller driver or
> > delegated to ARM/ARM64 pcibios_root_bridge_prepare() implementations,
> > where, upon DT probe detection, the functions are set to DT defaults (ie
> > of_irq_parse_and_map_pci() and pci_common_swizzle() respectively.
> >
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> 
> Nice!
> 
> > +int pcibios_root_bridge_prepare(struct pci_host_bridge *bridge)
> > +{
> > +       /*
> > +        * Set-up IRQ mapping/swizzingly functions.
> > +        * If the either function pointer is already set,
> > +        * do not override any of them since it is a host
> > +        * controller specific mapping/swizzling function.
> > +        */
> > +       if (!bridge->map_irq && !bridge->swizzle_irq) {
> > +               struct device *parent = bridge->dev.parent;
> > +               /*
> > +                * If we have a parent pointer with a valid
> > +                * OF node this means we are probing a PCI host
> > +                * controller configured through DT firmware.
> > +                */
> > +               if (IS_ENABLED(CONFIG_OF) && parent && parent->of_node) {
> > +                       bridge->map_irq = of_irq_parse_and_map_pci;
> > +                       bridge->swizzle_irq = pci_common_swizzle;
> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
> 
> I think it would be better to reduce the number of global functions defined
> by common code to be called from PCI core code, and instead use
> additional callback pointers from the pci_host_bridge operations.

Yes but this means duplicating the whole map_irq/swizzle_irq
initialization thing in all DT PCI host controllers, it is getting
quite cumbersome to be frank, we should try to consolidate DT PCI
host controllers code, it is becoming a bit unwieldy to manage and
there is too much code duplication.

> In particular, there are only very few existing users of
> pcibios_root_bridge_prepare() at the moment, so we should
> be able to get rid of those quite easily now.

I could do but please see my comment above.

> > diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
> > index 0f39bd2..bc9e36a 100644
> > --- a/drivers/pci/host/pcie-iproc.c
> > +++ b/drivers/pci/host/pcie-iproc.c
> > @@ -1205,7 +1205,8 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
> >         struct device *dev;
> >         int ret;
> >         void *sysdata;
> > -       struct pci_bus *bus, *child;
> > +       struct pci_bus *child;
> > +       struct pci_host_bridge *host;
> >
> >         dev = pcie->dev;
> >
> > @@ -1252,15 +1253,30 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
> >         sysdata = pcie;
> >  #endif
> >
> > -       bus = pci_create_root_bus(dev, 0, &iproc_pcie_ops, sysdata, res);
> > -       if (!bus) {
> 
> Could this be a separate patch?

Yes, I can split it from the pci_fixup_irqs() removal.

Thanks,
Lorenzo