On Fri, 2014-04-04 at 18:19 -0600, Bjorn Helgaas wrote:

...

Introduce a pci_register_io_range() helper function that can be used by the architecture code to keep track of the I/O ranges described by the PCI bindings. If the PCI_IOBASE macro is not defined that signals lack of support for PCI and we return an error.

I don't quite see how you intend to use this, because this series doesn't include any non-stub implementation of pci_register_io_range().

Is this anything like the ia64 strategy I mentioned above? If so, it would be really nice to unify some of this stuff.

We also use two different strategies on ppc32 and ppc64

- On ppc32, inb/outb turn into an MMIO access to _IO_BASE + port

That _IO_BASE is a variable which is initialized to the ioremapped address of the IO space MMIO aperture of the first bridge we discover. Then port numbers are "fixed up" on all other bridges so that the addition _IO_BASE + port fits the ioremapped address of the IO space on that bridge. A bit messy... and breaks whenever drivers copy port numbers into variables of the wrong type such as shorts.

- On ppc64, we have more virtual space, so instead we reserve a range of address space (fixed) for IO space, it's always the same. Bridges IO spaces are then mapped into that range, so we always have a positive offset from _IO_BASE which makes things a bit more robust and less "surprising" than ppc32. Additionally, the first 64k are reserved. They are only mapped if we see an ISA bridge (which some older machines have). Otherwise it's left unmapped, so crappy drivers trying to hard code x86 IO ports will blow up immediately which I deem better than silently whacking the wrong hardware. In addition, we have a mechanism we use on powernv to re-route accesses to that first 64k to the power8 built-in LPC bus which can have some legacy IOs on it such as a UART or a RTC.

Cheers, Ben.

On Mon, 2014-04-07 at 09:35 +0100, Liviu Dudau wrote:

Thanks for the summary, is really useful as I was recently looking into code in that area. One thing I was trying to understand is why ppc needs _IO_BASE at all rather than using the generic PCI_IOBASE?

Perhaps because our code predates it ? :-) I haven't looked much into the semantics of PCI_IOBASE yet...

Cheers, Ben.

On Mon, Apr 07, 2014 at 12:36:15PM +0100, Arnd Bergmann wrote:

On Monday 07 April 2014 09:31:20 Liviu Dudau wrote:

...

On Sat, Apr 05, 2014 at 01:19:53AM +0100, Bjorn Helgaas wrote:

...

Host bridges on x86 could have MMIO apertures that turn CPU memory accesses into PCI port accesses. We could implement any number of I/O port spaces this way, by making the kernel inb()/outb()/etc. interfaces smart enough to use the memory-mapped space instead of (or in addition to) the INB/OUTB/etc. instructions.

PowerPC actually has this already, as CONFIG_PPC_INDIRECT_PIO meaning that access to PIO registers is bus specific, and there is also CONFIG_PPC_INDIRECT_MMIO for the case where MMIO access is not native.

...

Right, sorry for my ignorance then: how does *currently* the device driver do the I/O transfer transparent of the implementation mechanism? Or they have intimate knowledge of wether the device is behind a host bridge and can do MMIO or is on an ISA or CF bus and then it needs INB/OUTB ? And if we make inb/outb smarter, does that mean that we need to change the drivers?

The idea of that would be to not change drivers.

My preference here would be to only have a generic function for those architectures that have the simple MMIO access all the time.

...

...

ia64 does this (see arch/ia64/include/asm/io.h for a little description) and I think maybe one or two other arches have something similar.

...

Introduce a pci_register_io_range() helper function that can be used by the architecture code to keep track of the I/O ranges described by the PCI bindings. If the PCI_IOBASE macro is not defined that signals lack of support for PCI and we return an error.

I don't quite see how you intend to use this, because this series doesn't include any non-stub implementation of pci_register_io_range().

Is this anything like the ia64 strategy I mentioned above? If so, it would be really nice to unify some of this stuff.

After discussions with Arnd and Catalin I know have a new series that moves some of the code from arm64 series into this one. I am putting it through testing right know as I am going to have to depend on another series that makes PCI_IOBASE defined only for architectures that do MMIO in order to choose the correct default implementation for these functions. My hope is that I will be able to send the series this week.

I think migrating other architectures to use the same code should be a separate effort from adding a generic implementation that can be used by arm64. It's probably a good idea to have patches to convert arm32 and/or microblaze.

Agree. My updated series only moves the arm64 code into framework to make the arm64 part a noop.

Liviu

Arnd

On Mon, Apr 07, 2014 at 06:58:24PM +0100, Bjorn Helgaas wrote:

On Mon, Apr 7, 2014 at 5:36 AM, Arnd Bergmann arnd@arndb.de wrote:

...

I think migrating other architectures to use the same code should be a separate effort from adding a generic implementation that can be used by arm64. It's probably a good idea to have patches to convert arm32 and/or microblaze.

Let me reiterate that I am 100% in favor of replacing arch-specific code with more generic implementations.

However, I am not 100% in favor of doing it as separate efforts (although maybe I could be convinced). The reasons I hesitate are that (1) if only one architecture uses a new "generic" implementation, we really don't know whether it is generic enough, (2) until I see the patches to convert other architectures, I have to assume that I'm the one who will write them, and (3) as soon as we add the code to drivers/pci, it becomes partly my headache to maintain it, even if only one arch benefits from it.

Please don't think I'm questioning anyone's intent or good will. It's just that I understand the business pressures, and I know how hard it can be to justify this sort of work to one's management, especially after the immediate problem has been solved.

I understand your concern. I guess the only way to prove my good intentions is to shut up and show the code.

Liviu

Bjorn

On Tue, Apr 8, 2014 at 4:22 AM, Arnd Bergmann arnd@arndb.de wrote:

On Tuesday 08 April 2014 10:50:39 Liviu Dudau wrote:

...

On Mon, Apr 07, 2014 at 06:58:24PM +0100, Bjorn Helgaas wrote:

...

On Mon, Apr 7, 2014 at 5:36 AM, Arnd Bergmann arnd@arndb.de wrote:

...

I think migrating other architectures to use the same code should be a separate effort from adding a generic implementation that can be used by arm64. It's probably a good idea to have patches to convert arm32 and/or microblaze.

Let me reiterate that I am 100% in favor of replacing arch-specific code with more generic implementations.

However, I am not 100% in favor of doing it as separate efforts (although maybe I could be convinced). The reasons I hesitate are that (1) if only one architecture uses a new "generic" implementation, we really don't know whether it is generic enough, (2) until I see the patches to convert other architectures, I have to assume that I'm the one who will write them, and (3) as soon as we add the code to drivers/pci, it becomes partly my headache to maintain it, even if only one arch benefits from it.

Fair enough.

My approach to the asm-generic infrastruction has mostly been to ensure that whoever adds a new architecture has to make things easier for the next person.

That's a good rule. But if we add a generic implementation used only by one architecture, the overall complexity has increased (we added new unshared code), so the next person has to look at N+1 existing implementations. If we even convert one existing arch, that seems like an improvement: we have N implementations with one being used by at least two arches.

Bjorn

On Thu, Jun 26, 2014 at 09:59:26AM +0100, Catalin Marinas wrote:

(sorry for replying to a months old thread)

On Mon, Apr 07, 2014 at 06:58:24PM +0100, Bjorn Helgaas wrote:

...

On Mon, Apr 7, 2014 at 5:36 AM, Arnd Bergmann arnd@arndb.de wrote:

...

I think migrating other architectures to use the same code should be a separate effort from adding a generic implementation that can be used by arm64. It's probably a good idea to have patches to convert arm32 and/or microblaze.

Let me reiterate that I am 100% in favor of replacing arch-specific code with more generic implementations.

However, I am not 100% in favor of doing it as separate efforts (although maybe I could be convinced). The reasons I hesitate are that (1) if only one architecture uses a new "generic" implementation, we really don't know whether it is generic enough, (2) until I see the patches to convert other architectures, I have to assume that I'm the one who will write them, and (3) as soon as we add the code to drivers/pci, it becomes partly my headache to maintain it, even if only one arch benefits from it.

I agree and understand your point.

...

Please don't think I'm questioning anyone's intent or good will. It's just that I understand the business pressures, and I know how hard it can be to justify this sort of work to one's management, especially after the immediate problem has been solved.

But, unfortunately, that's something we failed to address in reasonable time (even though I was one of the proponents of the generic PCIe implementation). This work is very likely to slip further into the late part of this year and I am aware that several ARM partners are blocked on the (upstream) availability of PCIe support for the arm64 kernel.

Although a bit late, I'm raising this now and hopefully we'll come to a conclusion soon. Delaying arm64 PCIe support even further is not a real option, which leaves us with:

1. Someone else (with enough PCIe knowledge) volunteering to take over soon or
2. Dropping Liviu's work and going for an arm64-specific implementation (most likely based on the arm32 implementation, see below)

First option is ideal but there is work to do as laid out by Arnd here:

http://article.gmane.org/gmane.linux.kernel/1679304

The latest patches from Liviu are here (they only target arm64 and there are additional comments to be addressed from the above thread):

http://linux-arm.org/git?p=linux-ld.git%3Ba=shortlog%3Bh=refs/heads/for-upst...

The main reason for the second option is timing. We could temporarily move Liviu's code under arch/arm64 with the hope that we generalise it later. However, the risk is even higher that once the code is in mainline, the generic implementation won't happen. In which case, I don't see much point in departing from the arm32 PCI API, making bios32 clone the best second option.

For the alternative implementation above, we already have a heavily cut down version of the arm32 PCI support but only tested in a virtual environment so far:

https://git.kernel.org/cgit/linux/kernel/git/will/linux.git/log/?h=pci/bios3...

In conclusion, unless someone volunteers for the first option fairly soon, we'll post the alternative patches for review and take it from there.

That would be a huge step backwards IMO and a huge dissapointment. If you go with the alternative patches from Will you will basically reset every partner's implementation that has been built on top of my patches (when they did so with the understanding that my series will be the one ARM will support and publish) *and* make anyone's attempt to create a generic implementation harder, as they will have to undo this code to remove the arch-specific parts.

While I have part of a personal blame to carry as I have dragged some of the work for too long, there are things that I could not have done differently due to internal pressure inside the project I work on. It is my intent to resume work on it as soon as possible, but life is such at the moment that I have to dedicate my time to other things.

Best regards, Liviu

Thanks.

-- Catalin

On Thu, Jun 26, 2014 at 03:11:38PM +0100, Catalin Marinas wrote:

On Thu, Jun 26, 2014 at 10:30:29AM +0100, Liviu Dudau wrote:

...

On Thu, Jun 26, 2014 at 09:59:26AM +0100, Catalin Marinas wrote:

...

Although a bit late, I'm raising this now and hopefully we'll come to a conclusion soon. Delaying arm64 PCIe support even further is not a real option, which leaves us with:

1. Someone else (with enough PCIe knowledge) volunteering to take over soon or
2. Dropping Liviu's work and going for an arm64-specific implementation (most likely based on the arm32 implementation, see below)

[...]

...

...

In conclusion, unless someone volunteers for the first option fairly soon, we'll post the alternative patches for review and take it from there.

That would be a huge step backwards IMO and a huge dissapointment. If you go with the alternative patches from Will you will basically reset every partner's implementation that has been built on top of my patches (when they did so with the understanding that my series will be the one ARM will support and publish) *and* make anyone's attempt to create a generic implementation harder, as they will have to undo this code to remove the arch-specific parts.

I fully agree and the alternative patchset is definitely _not_ my preferred solution. You can read this email as a request for help to complete the work (whether it comes from ARM, Linaro or other interested parties). I don't mean taking over the whole patchset but potentially helping with other arch conversion (microblaze, arm multi-platform).

I feel it's also worth pointing out that I didn't write that code with the intention of getting it merged, nor as a competing solution to what Liviu was proposing at the time. It was merely a development tool to enable some of the SMMU and GICv3 work that Marc and I have been working on recently.

Will

On Thursday 26 June 2014 19:44:21 Rob Herring wrote:

...

Although a bit late, I'm raising this now and hopefully we'll come to a conclusion soon. Delaying arm64 PCIe support even further is not a real option, which leaves us with:

1. Someone else (with enough PCIe knowledge) volunteering to take over soon or

Well, I might have 2 months ago, but now I'm pretty booked up.

...

2. Dropping Liviu's work and going for an arm64-specific implementation (most likely based on the arm32 implementation, see below)

3. Keeping Liviu's patches leaving some of the architecture specific

bits. I know Arnd and I both commented on it still needing more common pieces, but compared to option 2 that would be way better.

Agreed.

920a685 pci: Add support for creating a generic host_bridge from device tree

This function could be moved to drivers/of/of_pci.c if having it in drivers/pci is too much maintenance burden. However, nearly the same code is already being duplicated in every DT enabled ARM PCI host driver and will continue as more PCI hosts are added. So this isn't really a question of converting other architectures to common PCI host infrastructure, but converting DT based PCI hosts to common infrastructure. ARM is the only arch moving host drivers to drivers/pci ATM. Until other architectures start doing that, converting them is pointless.

This is the most important part in my mind. Every implementation gets this code wrong at least initially, and we have to provide some generic helper to get the broken code out of host drivers. I don't care whether that helper is part of the PCI core or part of the OF core.

7cfde80 arm64: Add architecture support for PCI

What is here is really just a function of which option we pick.

...

First option is ideal but there is work to do as laid out by Arnd here:

http://article.gmane.org/gmane.linux.kernel/1679304

I don't agree arm32 is harder than microblaze. Yes, converting ALL of arm would be, but that is not necessary. With Liviu's latest branch the hacks I previously needed are gone (thanks!), and this is all I need to get Versatile PCI working (under QEMU):

I meant converting all of arm32 would be harder, but I agree we don't have to do it. It would be nice to convert all of the drivers/pci/host drivers though, iow all multiplatform-enabled ones, and leaving the current arm32 pci implementation for the platforms we don't want to convert to multiplatform anyway (footbridge, iop, ixp4xx, ks8695 (?), pxa, sa1100).

Arnd

On Friday 27 June 2014 13:49:49 Will Deacon wrote:

On Fri, Jun 27, 2014 at 12:03:34PM +0100, Arnd Bergmann wrote:

...

On Thursday 26 June 2014 19:44:21 Rob Herring wrote:

...

I don't agree arm32 is harder than microblaze. Yes, converting ALL of arm would be, but that is not necessary. With Liviu's latest branch the hacks I previously needed are gone (thanks!), and this is all I need to get Versatile PCI working (under QEMU):

I meant converting all of arm32 would be harder, but I agree we don't have to do it. It would be nice to convert all of the drivers/pci/host drivers though, iow all multiplatform-enabled ones, and leaving the current arm32 pci implementation for the platforms we don't want to convert to multiplatform anyway (footbridge, iop, ixp4xx, ks8695 (?), pxa, sa1100).

I'm more than happy to convert the generic host controller we merged recently, but I'd probably want the core changes merged first so that I know I'm not wasting my time!

That is definitely fine with me, but it's Bjorn's decision.

Arnd

On Fri, Jun 27, 2014 at 01:44:21AM +0100, Rob Herring wrote:

On Thu, Jun 26, 2014 at 3:59 AM, Catalin Marinas catalin.marinas@arm.com wrote:

...

Although a bit late, I'm raising this now and hopefully we'll come to a conclusion soon. Delaying arm64 PCIe support even further is not a real option, which leaves us with:

1. Someone else (with enough PCIe knowledge) volunteering to take over soon or
2. Dropping Liviu's work and going for an arm64-specific implementation (most likely based on the arm32 implementation, see below)

3. Keeping Liviu's patches leaving some of the architecture specific

bits. I know Arnd and I both commented on it still needing more common pieces, but compared to option 2 that would be way better.

Let's look at the patches in question:

3e71867 pci: Introduce pci_register_io_range() helper function. 6681dff pci: OF: Fix the conversion of IO ranges into IO resources.

Both OF patches. I'll happily merge them.

We just need to make sure they don't break other users of of_pci_range_to_resource() that the second patch introduces.

2d5dd85 pci: Create pci_host_bridge before its associated bus in pci_create_root_bus. f6f2854 pci: Introduce a domain number for pci_host_bridge. 524a9f5 pci: Export find_pci_host_bridge() function.

These don't seem to be too controversial.

I think here there were discussions around introducing domain_nr to pci_host_bridge, particularly to the pci_create_root_bus_in_domain() API change. I don't think we reached any conclusion.

fb75718 pci: of: Parse and map the IRQ when adding the PCI device.

6 LOC. Hardly controversial.

I agree.

920a685 pci: Add support for creating a generic host_bridge from device tree

This function could be moved to drivers/of/of_pci.c if having it in drivers/pci is too much maintenance burden.

I think it makes sense. Currently drivers/pci/host-bridge.c doesn't have anything OF related, so of_pci.c looks more appropriate.

However, nearly the same code is already being duplicated in every DT enabled ARM PCI host driver and will continue as more PCI hosts are added. So this isn't really a question of converting other architectures to common PCI host infrastructure, but converting DT based PCI hosts to common infrastructure. ARM is the only arch moving host drivers to drivers/pci ATM. Until other architectures start doing that, converting them is pointless.

I agree. It's probably more important to convert some of the drivers/pci/host implementations to using the common parsing rather than a new architecture (this way we avoid even more code duplication).

bcf5c10 Fix ioport_map() for !CONFIG_GENERIC_IOMAP cases.

Seems like an independent fix that should be applied regardless.

Indeed. But it got stuck at the top of this series and hasn't been pushed upstream.

7cfde80 arm64: Add architecture support for PCI

What is here is really just a function of which option we pick.

With Liviu's latest version (not posted) and with of_create_pci_host_bridge() function moved to of_pci.c, I don't think there is much new functionality added to drivers/pci/. What I think we need is clarifying the domain_nr patch (and API change) and more users of the new generic code. As you said, it doesn't need to be a separate architecture but rather existing pci host drivers under drivers/pci. Of course, other arch conversion should follow shortly as well but even without an immediate conversion, I don't see too much additional maintenance burden for the core PCIe code (and code sharing between new PCIe host drivers is even more beneficial).

On Fri, Jun 27, 2014 at 03:55:04PM +0100, Bjorn Helgaas wrote:

On Fri, Jun 27, 2014 at 8:14 AM, Catalin Marinas catalin.marinas@arm.com wrote:

...

... With Liviu's latest version (not posted) and with of_create_pci_host_bridge() function moved to of_pci.c, I don't think there is much new functionality added to drivers/pci/. What I think we need is clarifying the domain_nr patch (and API change) and more users of the new generic code. As you said, it doesn't need to be a separate architecture but rather existing pci host drivers under drivers/pci. Of course, other arch conversion should follow shortly as well but even without an immediate conversion, I don't see too much additional maintenance burden for the core PCIe code (and code sharing between new PCIe host drivers is even more beneficial).

Sorry, I haven't had time to follow this. It sounds like there are several pieces we could get out of the way easily. How about posting the actual patches again? Maybe re-order them so the easy pieces are first so they can get applied even if there are issues with later ones?

OK, I will post a new series on Monday.

Thanks, Liviu

Bjorn

On Fri, Mar 14, 2014 at 06:46:23PM +0000, Arnd Bergmann wrote:

On Friday 14 March 2014, Liviu Dudau wrote:

...

You are right, that was lazy of me. What about this version?

Yes, that seems better. Thanks for fixing it up.

But back to the more important question that I realized we have not resolved yet:

You now have two completely independent allocation functions for logical I/O space numbers, and they will return different numbers for any nontrivial scenario.

...

+int of_pci_range_to_resource(struct of_pci_range *range,

•   struct device_node *np, struct resource *res)
  

+{

•   res->flags = range->flags;
  

•   res->parent = res->child = res->sibling = NULL;
  

•   res->name = np->full_name;
  

•   if (res->flags & IORESOURCE_IO) {
  

•           unsigned long port = -1;
  

•           int err = pci_register_io_range(range->cpu_addr, range->size);
  

•           if (err)
  

•                   goto invalid_range;
  

•           port = pci_address_to_pio(range->cpu_addr);
  

•           if (port == (unsigned long)-1) {
  

•                   err = -EINVAL;
  

•                   goto invalid_range;
  

•           }
  

•           res->start = port;
  

•   } else {
  

This one concatenates the I/O spaces and assumes that each space starts at bus address zero, and takes little precaution to avoid filling up IO_SPACE_LIMIT if the sizes are too big.

Actually, you are attaching too much meaning to this one. pci_register_io_range() only tries to remember the ranges, nothing else. And it *does* check that the total sum of registered ranges does not exceed IO_SPACE_LIMIT. This is used before we have any resource mapped for that range (actually it is used to *create* the resource for the range) so there is no other helping hand.

It doesn't assume that space starts at bus address zero, it ignores the bus address. It only handles CPU addresses for the range, to help with generating logical IO ports. If you have overlapping CPU addresses with different bus addresses it will not work, but then I guess you will have different problems then.

...

+unsigned long pci_ioremap_io(const struct resource *res, phys_addr_t phys_addr) +{

•   unsigned long start, len, virt_start;
  

•   int err;
  

•   if (res->end > IO_SPACE_LIMIT)
  

•           return -EINVAL;
  

•   /*
  

•    * try finding free space for the whole size first,
  

•    * fall back to 64K if not available
  

•    */
  

•   len = resource_size(res);
  

•   start = bitmap_find_next_zero_area(pci_iospace, IO_SPACE_PAGES,
  

•                           res->start / PAGE_SIZE, len / PAGE_SIZE, 0);
  

•   if (start == IO_SPACE_PAGES && len > SZ_64K) {
  

•           len = SZ_64K;
  

•           start = 0;
  

•           start = bitmap_find_next_zero_area(pci_iospace, IO_SPACE_PAGES,
  

•                                   start, len / PAGE_SIZE, 0);
  

•   }
  

•   /* no 64K area found */
  

•   if (start == IO_SPACE_PAGES)
  

•           return -ENOMEM;
  

•   /* ioremap physical aperture to virtual aperture */
  

•   virt_start = start * PAGE_SIZE + (unsigned long)PCI_IOBASE;
  

•   err = ioremap_page_range(virt_start, virt_start + len,
  

•                           phys_addr, __pgprot(PROT_DEVICE_nGnRE));
  

•   if (err)
  

•           return err;
  

•   bitmap_set(pci_iospace, start, len / PAGE_SIZE);
  

•   /* return io_offset */
  

•   return start * PAGE_SIZE - res->start;
  

+}

While this one will try to fall back to smaller sizes, and will honor nonzero bus addresses.

Yes, because this one does the actual allocation. And it needs to know how the mapping from CPU to bus works.

I think we shouldn't even try to do the same thing twice, but instead just use a single allocator. I'd prefer the one I came up with, but I realize that I am biased here ;-)

I agree, but I think the two functions serve two different and distinct roles.

Best regards, Liviu

Arnd

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

On Fri, Mar 14, 2014 at 07:16:10PM +0000, Arnd Bergmann wrote:

On Friday 14 March 2014, Liviu Dudau wrote:

...

On Fri, Mar 14, 2014 at 06:46:23PM +0000, Arnd Bergmann wrote:

...

On Friday 14 March 2014, Liviu Dudau wrote:

...

+int of_pci_range_to_resource(struct of_pci_range *range,

•   struct device_node *np, struct resource *res)
  

+{

•   res->flags = range->flags;
  

•   res->parent = res->child = res->sibling = NULL;
  

•   res->name = np->full_name;
  

•   if (res->flags & IORESOURCE_IO) {
  

•           unsigned long port = -1;
  

•           int err = pci_register_io_range(range->cpu_addr, range->size);
  

•           if (err)
  

•                   goto invalid_range;
  

•           port = pci_address_to_pio(range->cpu_addr);
  

•           if (port == (unsigned long)-1) {
  

•                   err = -EINVAL;
  

•                   goto invalid_range;
  

•           }
  

•           res->start = port;
  

•   } else {
  

This one concatenates the I/O spaces and assumes that each space starts at bus address zero, and takes little precaution to avoid filling up IO_SPACE_LIMIT if the sizes are too big.

Actually, you are attaching too much meaning to this one. pci_register_io_range() only tries to remember the ranges, nothing else. And it *does* check that the total sum of registered ranges does not exceed IO_SPACE_LIMIT. This is used before we have any resource mapped for that range (actually it is used to *create* the resource for the range) so there is no other helping hand.

It doesn't assume that space starts at bus address zero, it ignores the bus address. It only handles CPU addresses for the range, to help with generating logical IO ports. If you have overlapping CPU addresses with different bus addresses it will not work, but then I guess you will have different problems then.

The problem is that it tries to set up a mapping so that pci_address_to_pio returns the actual port number, but the port that you assign to res->start above is assumed to match the 'start' variable below. If the value ends up different, the BARs that get set by the PCI bus scan are not in the same place that got ioremapped into the virtual I/O aperture.

Yes, after writting a reply trying to justify why it would actually work I've realised where the fault of my logic stands (short version, two host controllers will get different io_offsets but the ports numbers will start from zero leading to confusion about which host controller the resource belongs to).

I will try to split your function into two parts, one that calculates the io_offset and another that does the ioremap_page_range() side and replace my cooked function.

Best regards, Liviu

...

...

...

+unsigned long pci_ioremap_io(const struct resource *res, phys_addr_t phys_addr) +{

•   unsigned long start, len, virt_start;
  

•   int err;
  

•   if (res->end > IO_SPACE_LIMIT)
  

•           return -EINVAL;
  

•   /*
  

•    * try finding free space for the whole size first,
  

•    * fall back to 64K if not available
  

•    */
  

•   len = resource_size(res);
  

•   start = bitmap_find_next_zero_area(pci_iospace, IO_SPACE_PAGES,
  

•                           res->start / PAGE_SIZE, len / PAGE_SIZE, 0);
  

•   if (start == IO_SPACE_PAGES && len > SZ_64K) {
  

•           len = SZ_64K;
  

•           start = 0;
  

•           start = bitmap_find_next_zero_area(pci_iospace, IO_SPACE_PAGES,
  

•                                   start, len / PAGE_SIZE, 0);
  

•   }
  

•   /* no 64K area found */
  

•   if (start == IO_SPACE_PAGES)
  

•           return -ENOMEM;
  

•   /* ioremap physical aperture to virtual aperture */
  

•   virt_start = start * PAGE_SIZE + (unsigned long)PCI_IOBASE;
  

•   err = ioremap_page_range(virt_start, virt_start + len,
  

•                           phys_addr, __pgprot(PROT_DEVICE_nGnRE));
  

•   if (err)
  

•           return err;
  

•   bitmap_set(pci_iospace, start, len / PAGE_SIZE);
  

•   /* return io_offset */
  

•   return start * PAGE_SIZE - res->start;
  

+}

Arnd

On Mon, Apr 07, 2014 at 10:14:18AM +0100, Benjamin Herrenschmidt wrote:

On Mon, 2014-04-07 at 09:46 +0100, Liviu Dudau wrote:

...

*My* strategy is to get rid of pci_domain_nr(). I don't see why we need to have arch specific way of providing the number, specially after looking at the existing implementations that return a value from a variable that is never touched or incremented. My guess is that pci_domain_nr() was created to work around the fact that there was no domain_nr maintainance in the generic code.

Well, there was no generic host bridge structure. There is one now, it should go there.

Exactly! Hence my patch. After it gets accepted I will go through architectures and remove their version of pci_domain_nr().

Best regards, Liviu

Cheers, Ben.

On Mon, Apr 07, 2014 at 11:44:51PM +0100, Bjorn Helgaas wrote:

On Mon, Apr 7, 2014 at 4:07 AM, Liviu Dudau Liviu.Dudau@arm.com wrote:

...

On Mon, Apr 07, 2014 at 10:14:18AM +0100, Benjamin Herrenschmidt wrote:

...

On Mon, 2014-04-07 at 09:46 +0100, Liviu Dudau wrote:

...

*My* strategy is to get rid of pci_domain_nr(). I don't see why we need to have arch specific way of providing the number, specially after looking at the existing implementations that return a value from a variable that is never touched or incremented. My guess is that pci_domain_nr() was created to work around the fact that there was no domain_nr maintainance in the generic code.

Well, there was no generic host bridge structure. There is one now, it should go there.

Exactly! Hence my patch. After it gets accepted I will go through architectures and remove their version of pci_domain_nr().

Currently the arch has to supply pci_domain_nr() because that's the only way for the generic code to learn the domain. After you add pci_create_root_bus_in_domain(), the arch can supply the domain that way, and we won't need the arch-specific pci_domain_nr(). Right? That makes more sense to me; thanks for the explanation.

Right.

Let me try to explain my concern about the pci_create_root_bus_in_domain() interface. We currently have these interfaces:

pci_scan_root_bus() pci_scan_bus() pci_scan_bus_parented() pci_create_root_bus()

pci_scan_root_bus() is a higher-level interface than pci_create_root_bus(), so I'm trying to migrate toward it because it lets us remove a little code from the arch, e.g., pci_scan_child_bus() and pci_bus_add_devices().

I think we can only remove the arch-specific pci_domain_nr() if that arch uses pci_create_root_bus_in_domain(). When we convert an arch from using scan_bus interfaces to using pci_create_root_bus_in_domain(), we will have to move the rest of the scan_bus code (pci_scan_child_bus(), pci_bus_add_devices()) back into the arch code.

One alternative is to add an _in_domain() variant of each of these interfaces, but that doesn't seem very convenient either. My idea of passing in a structure would also require adding variants, so there's not really an advantage there, but I am thinking of the next unification effort, e.g., for NUMA node info. I don't really want to have to change all the _in_domain() interfaces to also take yet another parameter for the node number.

OK, what about this: all the functions that you have mentioned take a void *sysdata parameter. Should we convert this opaque pointer into a specific structure that holds the domain_nr and (in future) the NUMA node info?

Converting all the architectures is going to be a long winded job as everyone loved to add their own stuff in the structure pointed by sysdata, breaking that will lead to frustrations, but maybe the framework should take back ownership of it.

Best regards, Liviu

Bjorn

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

On Tue, Apr 08, 2014 at 05:28:39PM +0100, Bjorn Helgaas wrote:

On Tue, Apr 8, 2014 at 4:20 AM, Liviu Dudau Liviu.Dudau@arm.com wrote:

...

On Mon, Apr 07, 2014 at 11:44:51PM +0100, Bjorn Helgaas wrote:

...

...

Let me try to explain my concern about the pci_create_root_bus_in_domain() interface. We currently have these interfaces:

pci_scan_root_bus() pci_scan_bus() pci_scan_bus_parented() pci_create_root_bus() ...

...

...

One alternative is to add an _in_domain() variant of each of these interfaces, but that doesn't seem very convenient either. My idea of passing in a structure would also require adding variants, so there's not really an advantage there, but I am thinking of the next unification effort, e.g., for NUMA node info. I don't really want to have to change all the _in_domain() interfaces to also take yet another parameter for the node number.

OK, what about this: all the functions that you have mentioned take a void *sysdata parameter. Should we convert this opaque pointer into a specific structure that holds the domain_nr and (in future) the NUMA node info?

I doubt if we can make sysdata itself generic because I suspect we need a way to have *some* arch-specific data. But maybe the arch could supply a structure containing a struct device *, domain, struct pci_ops *, list of resources, aperture info, etc. I wonder if struct pci_host_bridge would be a reasonable place to put this stuff, e.g., something like this:

struct pci_host_bridge { int domain; int node; struct device *dev; struct pci_ops *ops; struct list_head resources; void *sysdata; struct pci_bus *bus; /* filled in by core, not by arch */ ... /* other existing contents managed by core */ };

struct pci_bus *pci_scan_host_bridge(struct pci_host_bridge *bridge);

I'm really reluctant to give the arches more rope to hang themselves. I really dislike the use of xxxx_initcall() to do PCI initialisation ordering that is currently in widespread use through the arch code. As I hope to have proven with my arm64 code, you can have PCI support for an architecture without having to provide any arch specific code. We have enough correct code in the PCI framework, what would the architectures provide to the generic code that we cannot get by following the standard?

Of course, there are always arch specific corners and they need their data structures to make sense of those, but rather than having architectures fill in a structure *before* we can setup host bridges I think we need to reverse the order. Using your example structure, I don't think is the arch's job to provide the list of resources or the domain number before we can scan the host bridge. We should be able to get those from somewhere else (like adding by default the ioport_resource and iomem_resource and managing domain numbers inside the core framework).

Best regards, Liviu

-- 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

On Wednesday 09 April 2014 08:02:41 Bjorn Helgaas wrote:

It's possible we could manage domain numbers in the core. On ACPI systems, we currently we use the ACPI _SEG value as the domain. In some cases, e.g., on ia64, config space access is done via firmware interfaces, and those interfaces expect the _SEG values. We could conceivably maintain a mapping between _SEG and domain, but I'm not sure there's an advantage there.

I think it's a safe assumption that we will never have more than one firmware trying to enumerate the domains, so it would be safe to let ACPI keep doing its own thing for domain numbers, have the DT code pick domain number using some method we come up with, and for everything else let the architecture code deal with it. There are probably very few systems that have multiple domains but use neither ACPI nor DT.

Arnd

On Wed, Apr 09, 2014 at 08:02:41AM -0600, Bjorn Helgaas wrote:

On Wed, Apr 9, 2014 at 6:07 AM, Liviu Dudau Liviu.Dudau@arm.com wrote:

...

On Tue, Apr 08, 2014 at 05:28:39PM +0100, Bjorn Helgaas wrote:

...

On Tue, Apr 8, 2014 at 4:20 AM, Liviu Dudau Liviu.Dudau@arm.com wrote:

...

On Mon, Apr 07, 2014 at 11:44:51PM +0100, Bjorn Helgaas wrote:

...

...

Let me try to explain my concern about the pci_create_root_bus_in_domain() interface. We currently have these interfaces:

pci_scan_root_bus() pci_scan_bus() pci_scan_bus_parented() pci_create_root_bus() ...

...

...

One alternative is to add an _in_domain() variant of each of these interfaces, but that doesn't seem very convenient either. My idea of passing in a structure would also require adding variants, so there's not really an advantage there, but I am thinking of the next unification effort, e.g., for NUMA node info. I don't really want to have to change all the _in_domain() interfaces to also take yet another parameter for the node number.

OK, what about this: all the functions that you have mentioned take a void *sysdata parameter. Should we convert this opaque pointer into a specific structure that holds the domain_nr and (in future) the NUMA node info?

I doubt if we can make sysdata itself generic because I suspect we need a way to have *some* arch-specific data. But maybe the arch could supply a structure containing a struct device *, domain, struct pci_ops *, list of resources, aperture info, etc. I wonder if struct pci_host_bridge would be a reasonable place to put this stuff, e.g., something like this:

struct pci_host_bridge { int domain; int node; struct device *dev; struct pci_ops *ops; struct list_head resources; void *sysdata; struct pci_bus *bus; /* filled in by core, not by arch */ ... /* other existing contents managed by core */ };

struct pci_bus *pci_scan_host_bridge(struct pci_host_bridge *bridge);

I'm really reluctant to give the arches more rope to hang themselves.

If you mean the sysdata pointer is rope to hang themselves, I think it would be great it we didn't need sysdata at all. But I think it would be a huge amount of work to get rid of it completely, and keeping it would let us work at that incrementally.

Agree. But then your suggestion was to wrap sysdata inside another structure, which to me constitutes additional rope.

...

I really dislike the use of xxxx_initcall() to do PCI initialisation ordering that is currently in widespread use through the arch code.

I certainly agree that initcall ordering is fragile and to be avoided when possible.

...

As I hope to have proven with my arm64 code, you can have PCI support for an architecture without having to provide any arch specific code. We have enough correct code in the PCI framework, what would the architectures provide to the generic code that we cannot get by following the standard?

PCI host bridges are not architected, i.e., the PCI/PCIe specs do not say anything about how to discover them or how to program them. So the arch or a driver for another bus (ACPI, OF, etc.) must enumerate them and discover the bridge apertures (those are in the resource list). And obviously the arch has to provide the root bus number and PCI config accessors.

The "what" for PCI host bridges is defined in the spec. The "how" is implementation defined. What I'm trying to get with the cleanup is the ordering of pci_host_bridge creation: core creates the structure first ("what"), arch then has the chance of adding specific data to it (ops, resources, etc) ("how").

At the moment arm and powerpc do some horrible dances in trying to create their local idea of a host bridge before passing it to the core code.

As for the root bus number, maybe we can offer some sensible default strategy for numbering them and the arches that don't care too much can use that.

...

Of course, there are always arch specific corners and they need their data structures to make sense of those, but rather than having architectures fill in a structure *before* we can setup host bridges I think we need to reverse the order. Using your example structure, I don't think is the arch's job to provide the list of resources or the domain number before we can scan the host bridge. We should be able to get those from somewhere else (like adding by default the ioport_resource and iomem_resource and managing domain numbers inside the core framework).

It's possible we could manage domain numbers in the core. On ACPI systems, we currently we use the ACPI _SEG value as the domain. In some cases, e.g., on ia64, config space access is done via firmware interfaces, and those interfaces expect the _SEG values. We could conceivably maintain a mapping between _SEG and domain, but I'm not sure there's an advantage there.

I probably don't understand what you intend by reversing the order. Are you suggesting something like new pcibios_*() interfaces the arch can use to get the host bridge apertures and domain number?

Yes. Lets stop having the architectures do early inits so that they can prepare their host bridge structures to be ready for when the PCI framework calls them. Lets create the host bridge in the PCI core and use pcibios_*() to add to it where necessary without having to race for position.

Best regards, Liviu

Bjorn

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

On Wednesday 09 April 2014 21:48:14 Bjorn Helgaas wrote:

On Wed, Apr 9, 2014 at 7:27 PM, Liviu Dudau liviu@dudau.co.uk wrote:

...

On Wed, Apr 09, 2014 at 08:02:41AM -0600, Bjorn Helgaas wrote:

...

...

...

struct pci_host_bridge { int domain; int node; struct device *dev; struct pci_ops *ops; struct list_head resources; void *sysdata; struct pci_bus *bus; /* filled in by core, not by arch */ ... /* other existing contents managed by core */ };

struct pci_bus *pci_scan_host_bridge(struct pci_host_bridge *bridge);

I'm really reluctant to give the arches more rope to hang themselves.

If you mean the sysdata pointer is rope to hang themselves, I think it would be great it we didn't need sysdata at all. But I think it would be a huge amount of work to get rid of it completely, and keeping it would let us work at that incrementally.

Agree. But then your suggestion was to wrap sysdata inside another structure, which to me constitutes additional rope.

I'll ponder this more, but I don't see your point here yet. The arch already supplies a sysdata pointer to pci_scan_root_bus(), and we stash it in every struct pci_bus already. My idea was just to pass it in differently, as a structure member rather than a separate argument. (And I'm not completely attached to my proposal; it was only to illustrate my concern about the explosion of interfaces if we have to add *_domain(), *_node(), etc.)

As a minor variation of your suggestion, how about passing in a pointer to struct pci_host_bridge, and embed that within its own private structure? I think this is closer to how a lot of other subsystems do the abstraction.

...

The "what" for PCI host bridges is defined in the spec. The "how" is implementation defined. What I'm trying to get with the cleanup is the ordering of pci_host_bridge creation: core creates the structure first ("what"), arch then has the chance of adding specific data to it (ops, resources, etc) ("how").

At the moment arm and powerpc do some horrible dances in trying to create their local idea of a host bridge before passing it to the core code.

As for the root bus number, maybe we can offer some sensible default strategy for numbering them and the arches that don't care too much can use that.

I think we're at the limit of what can be accomplished with the abstractness of English.

My opinion is that it's reasonable for the arch to discover the host bridge properties first and pass them to the core, and it doesn't require unreasonable things of the arch. I know the arm PCI setup is complicated, partly because it deals with a huge number of machines that don't have a consistent firmware interface. The x86/ACPI setup is relatively simple because it deals with a simple firmware interface. But that's just my opinion, and maybe your code will show otherwise.

Makes sense. One of the areas where the PCI code shows its age is the method how the various parts link together: there are function calls going back and forth between architecture specific files and generic files, rather a hierarchy of files with generic code being code by more specific code.

To do the probing properly, I think it's totally fine to have the core code expect stuff like the resources and domain number to be filled out already by whoever calls it, but then have wrappers around it that get this information from a firmware interface, or from hardwired architecture specific code where necessary.

Arnd

On Thursday 10 April 2014 07:50:52 Bjorn Helgaas wrote:

On Thu, Apr 10, 2014 at 2:00 AM, Arnd Bergmann arnd@arndb.de wrote:

...

On Wednesday 09 April 2014 21:48:14 Bjorn Helgaas wrote:

...

On Wed, Apr 9, 2014 at 7:27 PM, Liviu Dudau liviu@dudau.co.uk wrote:

...

On Wed, Apr 09, 2014 at 08:02:41AM -0600, Bjorn Helgaas wrote:

...

...

> struct pci_host_bridge { > int domain; > int node; > struct device *dev; > struct pci_ops *ops; > struct list_head resources; > void *sysdata; > struct pci_bus *bus; /* filled in by core, not by arch */ > ... /* other existing contents managed by core */ > }; > > struct pci_bus *pci_scan_host_bridge(struct pci_host_bridge *bridge);

I'm not sure I'm following you; you mean the arch-specific sysdata structure would contain a pointer to struct pci_host_bridge?

I have to admit that I'm not up on how other subsystems handle this sort of abstraction. Do you have any pointers to good examples that I can study?

What I mean is like this:

/* generic structure */ struct pci_host_bridge { int domain; int node; struct device *dev; struct pci_ops *ops; struct list_head resources; struct pci_bus *bus; /* filled in by core, not by arch */ ... /* other existing contents managed by core */ };

/* arm specific structure */ struct pci_sys_data { char io_res_name[12]; /* Bridge swizzling */ u8 (*swizzle)(struct pci_dev *, u8 *); /* IRQ mapping */ int (*map_irq)(const struct pci_dev *, u8, u8); /* Resource alignement requirements */ void (*add_bus)(struct pci_bus *bus); void (*remove_bus)(struct pci_bus *bus); void *private_data; /* platform controller private data */

/* not a pointer: */ struct pci_host_bridge bridge; }; static inline struct pci_sys_data *to_pci_sys_data(struct pci_host_bridge *bridge) { return container_of(bridge, struct pci_sys_data, bridge); }

/* arm specific, driver specific structure */ struct tegra_pcie { void __iomem *pads; void __iomem *afi;

struct clk *pex_clk; struct clk *afi_clk; struct clk *pll_e; struct clk *cml_clk;

struct tegra_msi msi;

struct list_head ports; unsigned int num_ports;

struct pci_sys_data sysdata; }; static inline struct tegra_pcie *to_tegra_pcie(struct pci_sys_data *sysdata) { return container_of(sysdata, struct tegra_pcie, sysdata); }

This mirrors how we treat devices: a pci_device has an embedded device, and so on, in other subsystems we can have multiple layers.

In this example, the tegra pcie driver then allocates its own tegra_pcie structure, fills out the fields it needs, and registers it with the ARM architecture code, passing just the pci_sys_data pointer. That function in turn passes a pointer to the embedded pci_host_bridge down to the generic code. Ideally we should try to eliminate the architecture specific portion here, but that is a later step.

Arnd

On Thursday 10 April 2014 15:53:04 Liviu Dudau wrote:

On Thu, Apr 10, 2014 at 03:07:44PM +0100, Arnd Bergmann wrote:

...

This mirrors how we treat devices: a pci_device has an embedded device, and so on, in other subsystems we can have multiple layers.

In this example, the tegra pcie driver then allocates its own tegra_pcie structure, fills out the fields it needs, and registers it with the ARM architecture code, passing just the pci_sys_data pointer. That function in turn passes a pointer to the embedded pci_host_bridge down to the generic code. Ideally we should try to eliminate the architecture specific portion here, but that is a later step.

So Arnd seems to agree with me: we should try to get out of architecture specific pci_sys_data and link the host bridge driver straight into the PCI core. The core then can call into arch code via pcibios_*() functions.

Arnd, am I reading correctly into what you are saying?

Half of it ;-)

I think it would be better to not have an architecture specific data structure, just like it would be better not to have architecture specific pcibios_* functions that get called by the PCI core. Note that the architecture specific functions are the ones that rely on the architecture specific data structures as well. If they only use the common fields, it should also be possible to share the code.

I also don't realistically think we can get there on a lot of architectures any time soon. Note that most architectures only have one PCI host implementation, so the architecture structure is the same as the host driver structure anyway.

For architectures like powerpc and arm that have people actively working on them, we have a chance to clean up that code in the way we want it (if we can agree on the direction), but it's still not trivial to do.

Speaking of arm32 in particular, I think we will end up with a split approach: modern platforms (multiplatform, possibly all DT based) using PCI core infrastructure directly and no architecture specific PCI code on the one side, and a variation of today's code for the legacy platforms on the other.

Arnd

On Friday 11 April 2014 15:01:09 Benjamin Herrenschmidt wrote:

On Thu, 2014-04-10 at 22:46 +0200, Arnd Bergmann wrote:

...

Half of it ;-)

I think it would be better to not have an architecture specific data structure, just like it would be better not to have architecture specific pcibios_* functions that get called by the PCI core. Note that the architecture specific functions are the ones that rely on the architecture specific data structures as well. If they only use the common fields, it should also be possible to share the code.

I don't understand... we'll never get rid of architecture specific hooks in one form or another.

We'll always need to some things in an architecture or host-bridge specific way. Now if you don't want to call them arch hooks, then call them host bridge ops, but they are needed and thus they will need some kind of architecture specific extension to the base host bridge structure.

Absolutely right. The thing I'd like to get rid of in the long run is global functions defined in the architecture code that are called by core code for host bridge specific functionality. In a lot of cases, they should not be needed if we can express the same things in a generic way. In other cases, we can use function pointers that are set at the time that the host bridge is registered.

EEH is one big nasty example on powerpc.

Another random one that happens to be hot in my brain right now because we just finished debugging it: On powernv, we are just fixing a series of bugs caused by the generic code trying to do hot resets on PCIe "by hand" by directly toggling the secondary reset register in bridges.

Well, on our root complexes, this triggers a link loss which triggers a fatal EEH "ER_all" interrupt which we escalate into a fence and all hell breaks loose.

We need to mask some error traps in the hardware before doing something that can cause an intentional link loss... and unmask them when done. (Among other things, there are other issues on P7 with hot reset).

So hot reset must be an architecture hook.

This sounds to me very much host bridge specific, not architecture specific. If you have the same host bridge in an ARM system, you'd want the same things to happen, and if you have another host bridge on PowerPC, you probably don't want that code to be called.

PERST (fundamental reset) can *only* be a hook. The way to generate a PERST is not specified. In fact, on our machines, we have special GPIOs we can use to generate PERST on individual slots below a PLX bridge and a different methods for slots directly on a PHB.

Eventually most of those hooks land into firmware, and as such it's akin to ACPI which also keeps a separate state structure and a pile of hooks.

On PowerPC, there are currently a bunch of platform specific callbacks in the ppc_md: pcibios_after_init, pci_exclude_device, pcibios_fixup_resources, pcibios_fixup_bus, pcibios_enable_device_hook, pcibios_fixup_phb, pcibios_window_alignment, and possibly some more. There is some architecture specific code that gets called by the PCI core, with the main purpose of calling into these.

On ARM32, we have a similar set of callbacks in the architecture private pci_sys_data: swizzle, map_irq, align_resource, add_bus, remove_bus, and some more callbacks for setup in the hw_pci structure that is used at initialization time: setup, scan, preinit, postinit. Again, these are called from architecture specific code that gets called by the PCI core.

I'm sure some of the other architectures have similar things, most of them probably less verbose because there is fewer variability between subarchitectures.

I think a nice way to deal with these in the long run would be to have a generic 'struct pci_host_bridge_ops' that can be defined by the architecture or the platform, or a particular host bridge driver. We'd have to define exactly which function pointers would go in there, but a good start would be the set of functions that are today provided by each architecture. The reset method you describe above would also fit well into this.

A host bridge driver can fill out the pointers with its own functions, or put platform or architecture specific function pointers in there, that get called by the PCI core. There are multiple ways to deal with default implementations here, one way would be that the core just falls back to a generic implementation (which is currently the __weak function) if it sees a NULL pointer. Another way would be to require each driver to either fill out all pointers or none of them, in which case we would use a default pci_host_bridge_ops struct that contains the pointers to the global pcibios_*() functions.

Arnd

On Thu, Apr 10, 2014 at 09:46:36PM +0100, Arnd Bergmann wrote:

On Thursday 10 April 2014 15:53:04 Liviu Dudau wrote:

...

On Thu, Apr 10, 2014 at 03:07:44PM +0100, Arnd Bergmann wrote:

...

...

This mirrors how we treat devices: a pci_device has an embedded device, and so on, in other subsystems we can have multiple layers.

In this example, the tegra pcie driver then allocates its own tegra_pcie structure, fills out the fields it needs, and registers it with the ARM architecture code, passing just the pci_sys_data pointer. That function in turn passes a pointer to the embedded pci_host_bridge down to the generic code. Ideally we should try to eliminate the architecture specific portion here, but that is a later step.

So Arnd seems to agree with me: we should try to get out of architecture specific pci_sys_data and link the host bridge driver straight into the PCI core. The core then can call into arch code via pcibios_*() functions.

Arnd, am I reading correctly into what you are saying?

Half of it ;-)

I think it would be better to not have an architecture specific data structure, just like it would be better not to have architecture specific pcibios_* functions that get called by the PCI core. Note that the architecture specific functions are the ones that rely on the architecture specific data structures as well. If they only use the common fields, it should also be possible to share the code.

While I've come to like the pcibios_*() interface (and yes, it could be formalised and abstracted into a pci_xxxx_ops structure) I don't like the fact that those functions use architectural data in order to function. I know it might sound strange, as they *are* supposed to be implemented by the arches, but in my mind the link between generic code and arch code for PCI should be done by the host bridge driver. That's how PCI spec describes it, and I see no reason why we should not be able to adopt the same view.

To be more precise, what I would like to happen in the case of some functions would be for the PCI core code to call a pci_host_bridge_ops method which in turn will call the arch specific code if it needs to. Why I think that would be better? Because otherwise you put in the architectural side code to cope with a certain host bridge, then another host bridge comes in and you add more architectural code, but then when you port host bridge X to arch B you discover that you need to add code there as well for X. And it all ends up in the mess we currently have where the drivers in drivers/pci/host are not capable of being ported to a different architecture because they rely on infrastructure only present in arm32 that is not properly documented.

I also don't realistically think we can get there on a lot of architectures any time soon. Note that most architectures only have one PCI host implementation, so the architecture structure is the same as the host driver structure anyway.

For architectures like powerpc and arm that have people actively working on them, we have a chance to clean up that code in the way we want it (if we can agree on the direction), but it's still not trivial to do.

Speaking of arm32 in particular, I think we will end up with a split approach: modern platforms (multiplatform, possibly all DT based) using PCI core infrastructure directly and no architecture specific PCI code on the one side, and a variation of today's code for the legacy platforms on the other.

Actually, if we could come up with a compromise for the pci_fixup_*() functions (are they still used by functional hardware?) then I think we could convert most of the arm32 arch code to re-direct the calls to the infrastructure code. But yes, there might be a lot of resistance to change due to lack of resources when changing old platforms.

Best regards, Liviu

Arnd

On Mon, Apr 07, 2014 at 11:44:51PM +0100, Bjorn Helgaas wrote:

On Mon, Apr 7, 2014 at 4:07 AM, Liviu Dudau Liviu.Dudau@arm.com wrote:

...

On Mon, Apr 07, 2014 at 10:14:18AM +0100, Benjamin Herrenschmidt wrote:

...

On Mon, 2014-04-07 at 09:46 +0100, Liviu Dudau wrote:

...

*My* strategy is to get rid of pci_domain_nr(). I don't see why we need to have arch specific way of providing the number, specially after looking at the existing implementations that return a value from a variable that is never touched or incremented. My guess is that pci_domain_nr() was created to work around the fact that there was no domain_nr maintainance in the generic code.

Well, there was no generic host bridge structure. There is one now, it should go there.

Exactly! Hence my patch. After it gets accepted I will go through architectures and remove their version of pci_domain_nr().

Currently the arch has to supply pci_domain_nr() because that's the only way for the generic code to learn the domain. After you add pci_create_root_bus_in_domain(), the arch can supply the domain that way, and we won't need the arch-specific pci_domain_nr(). Right? That makes more sense to me; thanks for the explanation.

Let me try to explain my concern about the pci_create_root_bus_in_domain() interface. We currently have these interfaces:

pci_scan_root_bus() pci_scan_bus() pci_scan_bus_parented() pci_create_root_bus()

pci_scan_root_bus() is a higher-level interface than pci_create_root_bus(), so I'm trying to migrate toward it because it lets us remove a little code from the arch, e.g., pci_scan_child_bus() and pci_bus_add_devices().

I think we can only remove the arch-specific pci_domain_nr() if that arch uses pci_create_root_bus_in_domain(). When we convert an arch from using scan_bus interfaces to using pci_create_root_bus_in_domain(), we will have to move the rest of the scan_bus code (pci_scan_child_bus(), pci_bus_add_devices()) back into the arch code.

One alternative is to add an _in_domain() variant of each of these interfaces, but that doesn't seem very convenient either. My idea of passing in a structure would also require adding variants, so there's not really an advantage there, but I am thinking of the next unification effort, e.g., for NUMA node info. I don't really want to have to change all the _in_domain() interfaces to also take yet another parameter for the node number.

Resurecting this thread as I'm about to send an updated patch:

TL;DR: Bjorn is concerned that my introduction of an _in_domain() version of pci_create_root_bus() as a way to pass a domain number from the arch code down (or up?) into the generic PCI code is incomplete, as other APIs that he listed make use of the non-domain aware version of pci_create_root_bus() and as he plans to remove the use of the function and use higher level APIs like pci_scan_root_bus() we will have to introduce an _in_domain() version for those higher level functions.

After a bit of thinking I think the change I'm proposing is fine exactly because it is a low level API. My intention is to automate the management of the PCI domain numbers and any architecture that wants to go against that should probably use the lower abstraction API to better control the flow. So, in my updated v8 version of the patch I'm going to keep the suggestion *as is* and hope we can have a(nother) discussion and come up with a conclusion.

Best regards, Liviu

Bjorn

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

On Fri, Jul 4, 2014 at 8:57 AM, Liviu Dudau Liviu.Dudau@arm.com wrote:

On Mon, Apr 07, 2014 at 11:44:51PM +0100, Bjorn Helgaas wrote:

...

On Mon, Apr 7, 2014 at 4:07 AM, Liviu Dudau Liviu.Dudau@arm.com wrote:

...

On Mon, Apr 07, 2014 at 10:14:18AM +0100, Benjamin Herrenschmidt wrote:

...

On Mon, 2014-04-07 at 09:46 +0100, Liviu Dudau wrote:

...

*My* strategy is to get rid of pci_domain_nr(). I don't see why we need to have arch specific way of providing the number, specially after looking at the existing implementations that return a value from a variable that is never touched or incremented. My guess is that pci_domain_nr() was created to work around the fact that there was no domain_nr maintainance in the generic code.

Well, there was no generic host bridge structure. There is one now, it should go there.

Exactly! Hence my patch. After it gets accepted I will go through architectures and remove their version of pci_domain_nr().

Currently the arch has to supply pci_domain_nr() because that's the only way for the generic code to learn the domain. After you add pci_create_root_bus_in_domain(), the arch can supply the domain that way, and we won't need the arch-specific pci_domain_nr(). Right? That makes more sense to me; thanks for the explanation.

Let me try to explain my concern about the pci_create_root_bus_in_domain() interface. We currently have these interfaces:

pci_scan_root_bus() pci_scan_bus() pci_scan_bus_parented() pci_create_root_bus()

pci_scan_root_bus() is a higher-level interface than pci_create_root_bus(), so I'm trying to migrate toward it because it lets us remove a little code from the arch, e.g., pci_scan_child_bus() and pci_bus_add_devices().

I think we can only remove the arch-specific pci_domain_nr() if that arch uses pci_create_root_bus_in_domain(). When we convert an arch from using scan_bus interfaces to using pci_create_root_bus_in_domain(), we will have to move the rest of the scan_bus code (pci_scan_child_bus(), pci_bus_add_devices()) back into the arch code.

One alternative is to add an _in_domain() variant of each of these interfaces, but that doesn't seem very convenient either. My idea of passing in a structure would also require adding variants, so there's not really an advantage there, but I am thinking of the next unification effort, e.g., for NUMA node info. I don't really want to have to change all the _in_domain() interfaces to also take yet another parameter for the node number.

... My understanding is that when pci_host_bridge structure was introduced you were trying to keep the APIs unchanged and hence the creation of a bridge was hidden inside the pci_create_root_bus() function.

You mean pci_alloc_host_bridge()? Right; ideally I would have used pci_scan_root_bus() everywhere and gotten rid of pci_create_root_bus(). The outline of pci_scan_root_bus() is:

pci_create_root_bus() pci_scan_child_bus() pci_bus_add_devices()

The problem was that several arches do interesting things scattered among that core. The ACPI host bridge driver used on x86 and ia64 does resource allocation before pci_bus_add_devices(), as does parisc. Probably all arches should do this, but they don't.

And powerpc and sparc use of_scan_bus() or something similar instead of pci_scan_child_bus(). They probably *could* provide config space accessors that talk to OF and would allow pci_scan_child_bus() to work. But that seemed like too much work at the time.

If we want to store the domain_nr information in the host bridge structure, together with a pointer to sysdata, then we need to break up the creation of the pci_host_bridge from the creation of a root bus. At that moment, pci_scan_root_bus() will need to be changed to accept a pci_host_bridge pointer, while pci_scan_bus() and pci_scan_bus_parented() will create the host bridge in the body of their function.

It's hard to change an existing interface like pci_scan_root_bus() because it's called from so many places and you have to change them all at once. Then if something goes wrong, the revert makes a mess for everybody. But I think it makes sense to add a new interface that does what you want.

Bjorn