Re: [RFC 2/4] PCI: generic: Add support for ARM64 and MSI(x)

[Lorenzo Pieralisi]

On Tue, Sep 30, 2014 at 05:42:56PM +0100, Liviu Dudau wrote:
> On Tue, Sep 30, 2014 at 05:12:41PM +0100, Lorenzo Pieralisi wrote:
> > On Tue, Sep 30, 2014 at 01:31:44PM +0100, Arnd Bergmann wrote:
> > > On Tuesday 30 September 2014 13:03:44 Lorenzo Pieralisi wrote:
> > > > > >  static int gen_pci_probe(struct platform_device *pdev)
> > > > > >  {
> > > > > > @@ -326,6 +385,7 @@ static int gen_pci_probe(struct platform_device *pdev)
> > > > > >         struct device *dev = &pdev->dev;
> > > > > >         struct device_node *np = dev->of_node;
> > > > > >         struct gen_pci *pci = devm_kzalloc(dev, sizeof(*pci), GFP_KERNEL);
> > > > > > +#ifndef CONFIG_ARM64
> > > > > >         struct hw_pci hw = {
> > > > > >                 .nr_controllers = 1,
> > > > > >                 .private_data   = (void **)&pci,
> > > > > > @@ -333,6 +393,7 @@ static int gen_pci_probe(struct platform_device *pdev)
> > > > > >                 .map_irq        = of_irq_parse_and_map_pci,
> > > > > >                 .ops            = &gen_pci_ops,
> > > > > >         };
> > > > > > +#endif
> > > > > >  
> > > > > 
> > > > > Same here, I'd suggest marking this "#ifdef CONFIG_ARM" instead, as hw_pci
> > > > > is an arm32 specific data structure.
> > > > 
> > > > I do not think we need hw struct at all, see below, we can write code so
> > > > that we do not rely on ARM32 PCI bios, I will have a stab at that and
> > > > post the resulting code.
> > > 
> > > That would of course be best. I think it needs some rework of the
> > > arm32 PCI code though, or you'd still have to create pci_sys_data
> > > manually, and that is currently allocated by pcibios_init_hw.
> 
> I don't see why we need to involve the arm32 code here at all. A host bridge can
> be fully functional with the generic code without having to use any of the
> arm32 code (unless I'm missing something here).

Ok so I can remove the pci_common_init() call, use the common PCI API and
everything will work as expected, even if there is a list of functions (see
below) that *require* pci_sys_data to exist (and that's allocated in arm32
pcibios code in pcibios_init_hw(), called from pci_common_init()) ?

I like the idea but I think that's optimistic, or at least we did not
trigger the code paths that can cause issues.

> > Right, as far as I can see, creating a pci_sys_data struct
> > that's all we would need. "Problem" is that it does not exist on ARM64
> > so to avoid ifdeffery we have to declare a struct with the same
> > fields (ie only pci_sys_data.private_data is used by this driver -
> > apart from arm32 specific functions usage) that is passed to the PCI layer
> > and stored in the bus.sysdata, but that's extremely ugly (and we won't
> > need this when the arm32 conversion is completed).
> > 
> > > > > > +       if (!gen_scan_root_bus(&pdev->dev, pci->cfg.bus_range.start,
> > > > > > +                              &gen_pci_ops, pci, &pci->resources)) {
> > > > > > +               dev_err(&pdev->dev, "failed to enable PCIe ports\n");
> > > > > > +               return -ENODEV;
> > > > > > +       }
> > > > > > +#else
> > > > > >         pci_common_init_dev(dev, &hw);
> > > > > > +#endif /* CONFIG_ARM64 */
> > > > > > 
> > > > > 
> > > > > Again, just make the pci_common_init_dev() call #ifdef CONFIG_ARM, and move
> > > > > the generic case after it, outside of the #ifdef.
> > > > 
> > > > I went through the code quickly but I think we can (and should) remove
> > > > this quite ugly ifdeffery altogether. Most of the functionality in
> > > > pci_common_init_dev() can be implemented through the common PCI API (and this
> > > > would make this driver arch agnostic as it should be), I will go through ARM32
> > > > PCI bios code to check what is executed in detail in pci_common_init_dev() and
> > > > make sure that we follow those initialization steps in the resulting probe code
> > > > for this PCI generic host controller driver.
> > > 
> > > These are the functions I found that refer to pci_sys_data on arm32:
> > > 
> > > pcibios_add_bus
> > > pcibios_remove_bus
> > > pcibios_align_resource
> > > pci_mmap_page_range
> > > pci_domain_nr
> > > pci_proc_domain
> > > 
> > > This is not as bad as I had feared, but we still have to ensure that
> > > any caller of these functions will work with both the generic PCI support
> > > and the arm32 specific drivers that today use hw_pci.
> > > 
> > > My idea for dealing with this was to convert all host drivers in
> > > drivers/pci/host to the generic PCI code and never build the arm32
> > > bios32 code when CONFIG_ARCH_MULTIPLATFORM is set. Unfortunately that
> > > requires either doing them all at once or coming up with a migration
> > > strategy so we don't break things in the process.
> > 
> > That makes sense. Related to the migration strategy, thoughts
> > appreciated. Declaring a static pci_sys_data (with some ifdef around it)
> > seems a horrible hack to me. Calling pci_common_init() only if CONFIG_ARM
> > is rather horrible too, but we can probably live with that.
> > 
> > I do not see anything else as possible solution at the moment unless
> > we go the whole nine yards and do what you suggest above, might take a
> > little while though.
> > 
> > Probably leaving pci_common_init() call (and related hw_pci struct, and
> > related ifdeffery to differentiate between different sysdata layouts for ARM
> > and ARM64) is the fastest path but I still think it is not nice at all.
> 
> Rob Herring found the conversion of mach-integrator/pci_v3.c to the generic
> framework quite painless. We might have to go through a lot of testing, but I don't
> see the process to be too horrendous.

See my comments above, I have not said that the conversion is complicated,
what I am saying is that I am not sure we can get rid of pcibios code calls
yet, as Arnd pointed out.

Lorenzo

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/