Re: [PATCH v8 6/9] pci: Introduce a domain number for pci_host_bridge.

[Liviu Dudau <Liviu.Dudau@arm.com>]

On Wed, Jul 09, 2014 at 04:10:04PM +0100, Bjorn Helgaas wrote:
> On Tue, Jul 8, 2014 at 4:48 PM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> > On Tue, Jul 08, 2014 at 07:41:50PM +0100, Bjorn Helgaas wrote:
> >> On Tue, Jul 8, 2014 at 4:46 AM, Liviu Dudau <Liviu.Dudau@arm.com> wrote:
> >> > On Tue, Jul 08, 2014 at 01:59:54AM +0100, Bjorn Helgaas wrote:
> >>
> >> >> I wonder if it would help to make a weak pci_domain_nr() function that
> >> >> returns "bridge->domain_nr".  Then each arch could individually drop its
> >> >> pci_domain_nr() definition as it was converted, e.g., something like this:
> >> >>
> >> >>   - Convert every arch pci_domain_nr() from a #define to a non-inline
> >> >>     function
> >> >>   - Add bridge.domain_nr, initialized from pci_domain_nr()
> >> >>   - Add a weak generic pci_domain_nr() that returns bridge.domain_nr
> >> >>   - Add a way to create a host bridge in a specified domain, so we can
> >> >>     initialize bridge.domain_nr without using pci_domain_nr()
> >> >>   - Convert each arch to use the new creation mechanism and drop its
> >> >>     pci_domain_nr() implementation
> >> >
> >> > I will try to propose a patch implementing this.
> >>
> >> I think this is more of an extra credit, cleanup sort of thing.  I
> >> don't think it advances your primary goal of (I think) getting arm64
> >> PCI support in.  So my advice is to not worry about unifying domain
> >> handling until later.
> >
> > Getting arm64 supported *is* my main goal. But like you have stated in your
> > review of v7, you wanted to see another architecture converted as a guarantee
> > of "genericity" (for lack of a better word) for my patches. The one architecture
> > I've set my eyes on is microblaze, and that one uses pci_scan_root_bus()
> > rather than pci_create_root_bus() so I don't have any opportunity to pass the
> > domain number or any additional info (like the sysdata pointer that we were
> > talking about) to the pci_host_bridge structure unless I do this cleanup.
> 
> I think maybe I was too harsh about that, or maybe we had different
> ideas about what "conversion" involved.  My comment was in response to
> "pci: Introduce pci_register_io_range() helper function", and I don't
> remember why I was concerned about that; it's not even in drivers/pci,
> and it doesn't have an obvious connection to putting the domain number
> in struct pci_host_bridge.

Well, to be honest I did move some of the code (as mentioned in the Changelog) from
drivers/pci into drivers/of. It makes more sense to be in OF, as it mostly concerns
architectures that use it.

> 
> The thing I'm more concerned about is adding new PCI interfaces, e.g.,
> pci_create_root_bus_in_domain(), that are only used by one
> architecture.  Then it's hard to be sure that it's going to be useful
> for other arches.  If you can add arm64 using the existing PCI
> interfaces, I don't any problem with that.

(No blame here or reproaches, I'm just restating the situation:) I (mostly) did try
that in my v7 series but it also got NAK-ed by Arnd and Catalin as it had too much
arm64 specific code in there.

I don't see a way out of adding new PCI interfaces if we want to have support in
the PCI framework for unifying existing architectures. Of course, there is the painful
alternative of changing the existing APIs and fixing arches in one go, but like you've
said is going to be messy. I don't think I (or the people and companies wanting PCIe
on arm64) should cop out and pick a quick fix that adds sysdata structure into arm64
just to avoid new APIs, as this is not going to help anyone in long term. What I can
do is to create a set of parallel APIs for pci_{scan,create}_root_bus() that take
a pci_host_bridge pointer and start converting architectures one by one to that API
while deprecating the existing one. That way we can add arm64 easily as it would be
the first architecture to use new code without breaking things *and* we provide a
migration path.

Best regards,
Liviu

> 
> Bjorn
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯