On Wednesday 02 July 2014 13:43:41 Tanmay Inamdar wrote:

On Wed, Jul 2, 2014 at 12:12 PM, Arnd Bergmann arnd@arndb.de wrote:

...

I think EXPORT_SYMBOL_GPL() is better here. The new symbols are unlikely to be used by a peripheral device driver, and PCI host controllers are already restricted by EXPORT_SYMBOL_GPL.

You are right as long as the functions are not used directly. But what if GPL functions are called indirectly. For example, 'pci_domain_nr' implementation in Liviu's V7 series calls 'find_pci_host_bridge'.

Good point. If pci_domain_nr() doesn't require access to an EXPORT_SYMBOL_GPL symbol, it should not start doing that after this patch.

For of_create_pci_host_bridge() however, I can't think of any reason to use a legacy EXPORT_SYMBOL.

Arnd

On Tuesday 01 July 2014 19:43:28 Liviu Dudau wrote:

+/*

• • Record the PCI IO range (expressed as CPU physical address + size).
• • Return a negative value if an error has occured, zero otherwise
• */

+int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) +{ +#ifdef PCI_IOBASE

•   struct io_range *res;
  

•   resource_size_t allocated_size = 0;
  

•   /* check if the range hasn't been previously recorded */
  

•   list_for_each_entry(res, &io_range_list, list) {
  

•           if (addr >= res->start && addr + size <= res->start + size)
  

•                   return 0;
  

•           allocated_size += res->size;
  

•   }
  

•   /* range not registed yet, check for available space */
  

•   if (allocated_size + size - 1 > IO_SPACE_LIMIT)
  

•           return -E2BIG;
  

•   /* add the range to the list */
  

•   res = kzalloc(sizeof(*res), GFP_KERNEL);
  

•   if (!res)
  

•           return -ENOMEM;
  

•   res->start = addr;
  

•   res->size = size;
  

•   list_add_tail(&res->list, &io_range_list);
  

•   return 0;
  

+#else

•   return -EINVAL;
  

+#endif +}

unsigned long __weak pci_address_to_pio(phys_addr_t address) { +#ifdef PCI_IOBASE

•   struct io_range *res;
  

•   resource_size_t offset = 0;
  

•   list_for_each_entry(res, &io_range_list, list) {
  

•           if (address >= res->start &&
  

•                   address < res->start + res->size) {
  

•                   return res->start - address + offset;
  

•           }
  

•           offset += res->size;
  

•   }
  

•   return (unsigned long)-1;
  

+#else if (address > IO_SPACE_LIMIT) return (unsigned long)-1; return (unsigned long) address; +#endif }

This still conflicts with the other allocator you have in patch 9 for pci_remap_iospace: nothing guarantees that the mapping is the same for both.

Also, this is a completely pointless exercise at this moment, because nobody cares about the result of pci_address_to_pio on architectures that don't already provide this function. If we ever get a proper Open Firmware implementation that wants to put hardcoded PCI devices into DT, we can add an implementation, but for now this seems overkill.

The allocator in pci_register_io_range seems reasonable, why not merge this function with pci_remap_iospace() as I have asked you multiple times before? Just make it return the io_offset so the caller can put that into the PCI host resources.

Arnd

On Tuesday 01 July 2014 21:45:09 Liviu Dudau wrote:

On Tue, Jul 01, 2014 at 09:36:10PM +0200, Arnd Bergmann wrote:

...

On Tuesday 01 July 2014 19:43:28 Liviu Dudau wrote:

This still conflicts with the other allocator you have in patch 9 for pci_remap_iospace: nothing guarantees that the mapping is the same for both.

Also, this is a completely pointless exercise at this moment, because nobody cares about the result of pci_address_to_pio on architectures that don't already provide this function. If we ever get a proper Open Firmware implementation that wants to put hardcoded PCI devices into DT, we can add an implementation, but for now this seems overkill.

The allocator in pci_register_io_range seems reasonable, why not merge this function with pci_remap_iospace() as I have asked you multiple times before? Just make it return the io_offset so the caller can put that into the PCI host resources.

Hi Arnd,

While I agree with you that at some moment the allocators were inconsistent wrt each other, for this version I would respectfully disagree on this. The allocator in pci_register_io_range() only makes sure that the ranges are not overlapping, it doesn't do any mapping whatsoever, while pci_remap_iospace() does only an ioremap_page_range(). The idea is that you get the offset out of pci_address_to_pio() and apply it to pci_remap_iospace().

Ok, got it now, I'm sorry I didn't read this properly at first.

Your solution looks correct to me, just using different tradeoffs to what I was expecting: You get a working pci_address_to_pio() function, which is probably never needed, but in turn you need to keep the state of each host bridge in a global list.

Arnd

On Wednesday 02 July 2014 15:23:03 Liviu Dudau wrote:

...

Your solution looks correct to me, just using different tradeoffs to what I was expecting: You get a working pci_address_to_pio() function, which is probably never needed, but in turn you need to keep the state of each host bridge in a global list.

Just a reminder that with my patchset I *do* start using pci_address_to_pio() in order to correctly parse the IO ranges from DT.

Yes, what I meant is that it would be easier not to do that. All existing drivers expect of_pci_range_to_resource() to return the CPU address for an I/O space register, not the Linux I/O port number that we want to pass to the PCI core. This is suboptimal because it's not obvious how it works, but it lets us get away without an extra registration step. Once all probe functions in PCI host drivers have been changed to the of_create_pci_host_bridge, that should not matter any more, because there is only one place left that calls it and we only have to get it right once.

Also, when you change that of_pci_range_to_resource, you also have to audit all callers of that function and ensure they can deal with the new behavior.

Arnd

On Wed, Jul 02, 2014 at 12:22:22PM +0100, Will Deacon wrote:

On Tue, Jul 01, 2014 at 07:43:28PM +0100, Liviu Dudau wrote:

...

Some architectures do not have a simple view of the PCI I/O space and instead use a range of CPU addresses that map to bus addresses. For some architectures these ranges will be expressed by OF bindings in a device tree file.

Introduce a pci_register_io_range() helper function with a generic implementation that can be used by such architectures 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.

[...]

...

+/*

• • Record the PCI IO range (expressed as CPU physical address + size).
• • Return a negative value if an error has occured, zero otherwise
• */

+int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) +{ +#ifdef PCI_IOBASE

• struct io_range *res;
• resource_size_t allocated_size = 0;
• /* check if the range hasn't been previously recorded */
• list_for_each_entry(res, &io_range_list, list) {

• if (addr >= res->start && addr + size <= res->start + size)
  

• 	return 0;
  

• allocated_size += res->size;
  

• }
• /* range not registed yet, check for available space */
• if (allocated_size + size - 1 > IO_SPACE_LIMIT)

• return -E2BIG;
  

• /* add the range to the list */
• res = kzalloc(sizeof(*res), GFP_KERNEL);
• if (!res)

• return -ENOMEM;
  

• res->start = addr;
• res->size = size;
• list_add_tail(&res->list, &io_range_list);
• return 0;

Hopefully a stupid question, but how is this serialised? I'm just surprised that adding to and searching a list are sufficient, unless there's a big lock somewhere.

Sorry, tripped into my own filters!

You are right, there is no serialisation here, will add one.

Best regards, Liviu

Will

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 Wed, Jul 02, 2014 at 01:38:04PM +0100, Arnd Bergmann wrote:

Some more detailed comments now

On Tuesday 01 July 2014 19:43:28 Liviu Dudau wrote:

...

+/*

• • Record the PCI IO range (expressed as CPU physical address + size).
• • Return a negative value if an error has occured, zero otherwise
• */

+int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size) +{ +#ifdef PCI_IOBASE

•   struct io_range *res;
  

I was confused by the variable naming here: A variable named 'res' is normally a 'struct resource'. Maybe better call this 'range'.

...

•   resource_size_t allocated_size = 0;
  

•   /* check if the range hasn't been previously recorded */
  

•   list_for_each_entry(res, &io_range_list, list) {
  

•           if (addr >= res->start && addr + size <= res->start + size)
  

•                   return 0;
  

•           allocated_size += res->size;
  

•   }
  

A spin_lock around the list lookup should be sufficient to get around the race that Will mentioned.

...

•   /* range not registed yet, check for available space */
  

•   if (allocated_size + size - 1 > IO_SPACE_LIMIT)
  

•           return -E2BIG;
  

It might be better to limit the size to 64K if it doesn't fit at first.

Thanks Arnd for review. Will update and post a new patch soon if I don't get any other comments.

Best regards, Liviu

Arnd

On Tuesday 08 July 2014, Bjorn Helgaas wrote:

On Tue, Jul 01, 2014 at 07:43:28PM +0100, Liviu Dudau wrote:

...

+static LIST_HEAD(io_range_list);

+/*

• • Record the PCI IO range (expressed as CPU physical address + size).
• • Return a negative value if an error has occured, zero otherwise
• */

+int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)

I don't understand the interface here. What's the mapping from CPU physical address to bus I/O port? For example, I have the following machine in mind:

HWP0002:00: PCI Root Bridge (domain 0000 [bus 00-1b]) HWP0002:00: memory-mapped IO port space [mem 0xf8010000000-0xf8010000fff] HWP0002:00: host bridge window [io 0x0000-0x0fff]

HWP0002:09: PCI Root Bridge (domain 0001 [bus 00-1b]) HWP0002:09: memory-mapped IO port space [mem 0xf8110000000-0xf8110000fff] HWP0002:09: host bridge window [io 0x1000000-0x1000fff] (PCI address [0x0-0xfff])

The CPU physical memory [mem 0xf8010000000-0xf8010000fff] is translated by the bridge to I/O ports 0x0000-0x0fff on PCI bus 0000:00. Drivers use, e.g., "inb(0)" to access it.

Similarly, [mem 0xf8110000000-0xf8110000fff] is translated by the second bridge to I/O ports 0x0000-0x0fff on PCI bus 0001:00. Drivers use "inb(0x1000000)" to access it.

I guess you are thinking of the IA64 model here where you keep the virtual I/O port numbers in a per-bus lookup table that gets accessed for each inb() call. I've thought about this some more, and I believe there are good reasons for sticking with the model used on arm32 and powerpc for the generic OF implementation.

The idea is that there is a single virtual memory range for all I/O port mappings and we use the MMU to do the translation rather than computing it manually in the inb() implemnetation. The main advantage is that all functions used in device drivers to (potentially) access I/O ports become trivial this way, which helps for code size and in some cases (e.g. SoC-internal registers with a low latency) it may even be performance relevant.

What this scheme gives you is a set of functions that literally do:

/* architecture specific virtual address */ #define PCI_IOBASE (void __iomem *)0xabcd00000000000

static inline u32 inl(unsigned long port) { return readl(port + PCI_IOBASE); }

static inline void __iomem *ioport_map(unsigned long port, unsigned int nr) { return port + PCI_IOBASE; }

static inline unsigned int ioread32(void __iomem *p) { return readl(p); }

Since we want this to work on 32-bit machines, the virtual I/O space has to be rather tightly packed, so Liviu's algorithm just picks the next available address for each new I/O space.

pci_register_io_range() seems sort of like it's intended to track the memory-mapped IO port spaces, e.g., [mem 0xf8010000000-0xf8010000fff]. But I would think you'd want to keep track of at least the base port number on the PCI bus, too. Or is that why it's weak?

The PCI bus start address only gets factored in when the window is registered with the PCI core in patch 8/9, where we go over all ranges doing

+ pci_add_resource_offset(resources, res, + res->start - range.pci_addr);

With Liviu's patch, this can be done in exactly the same way for both MMIO and PIO spaces.

Here's what these look like in /proc/iomem and /proc/ioports (note that there are two resource structs for each memory-mapped IO port space: one IORESOURCE_MEM for the memory-mapped area (used only by the host bridge driver), and one IORESOURCE_IO for the I/O port space (this becomes the parent of a region used by a regular device driver):

/proc/iomem: PCI Bus 0000:00 I/O Ports 00000000-00000fff PCI Bus 0001:00 I/O Ports 01000000-01000fff

/proc/ioports: 00000000-00000fff : PCI Bus 0000:00 01000000-01000fff : PCI Bus 0001:00

The only difference I'd expect here is that the last line would make it packed more tightly, so it's instead

/proc/ioports: 00000000-00000fff : PCI Bus 0000:00 00001000-00001fff : PCI Bus 0001:00

In practice we'd probably have 64KB per host controller, and each of them would be a separate domain. I think we normally don't register the IORESOURCE_MEM resource, but I agree it's a good idea and we should always do that.

Arnd

On Tue, Jul 08, 2014 at 10:29:51PM +0100, Bjorn Helgaas wrote:

On Tue, Jul 8, 2014 at 1:00 AM, Arnd Bergmann arnd@arndb.de wrote:

...

On Tuesday 08 July 2014, Bjorn Helgaas wrote:

...

On Tue, Jul 01, 2014 at 07:43:28PM +0100, Liviu Dudau wrote:

...

+static LIST_HEAD(io_range_list);

+/*

• • Record the PCI IO range (expressed as CPU physical address + size).
• • Return a negative value if an error has occured, zero otherwise
• */

+int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)

I don't understand the interface here. What's the mapping from CPU physical address to bus I/O port? For example, I have the following machine in mind:

HWP0002:00: PCI Root Bridge (domain 0000 [bus 00-1b]) HWP0002:00: memory-mapped IO port space [mem 0xf8010000000-0xf8010000fff] HWP0002:00: host bridge window [io 0x0000-0x0fff]

HWP0002:09: PCI Root Bridge (domain 0001 [bus 00-1b]) HWP0002:09: memory-mapped IO port space [mem 0xf8110000000-0xf8110000fff] HWP0002:09: host bridge window [io 0x1000000-0x1000fff] (PCI address [0x0-0xfff])

The CPU physical memory [mem 0xf8010000000-0xf8010000fff] is translated by the bridge to I/O ports 0x0000-0x0fff on PCI bus 0000:00. Drivers use, e.g., "inb(0)" to access it.

Similarly, [mem 0xf8110000000-0xf8110000fff] is translated by the second bridge to I/O ports 0x0000-0x0fff on PCI bus 0001:00. Drivers use "inb(0x1000000)" to access it.

I guess you are thinking of the IA64 model here where you keep the virtual I/O port numbers in a per-bus lookup table that gets accessed for each inb() call. I've thought about this some more, and I believe there are good reasons for sticking with the model used on arm32 and powerpc for the generic OF implementation.

The idea is that there is a single virtual memory range for all I/O port mappings and we use the MMU to do the translation rather than computing it manually in the inb() implemnetation. The main advantage is that all functions used in device drivers to (potentially) access I/O ports become trivial this way, which helps for code size and in some cases (e.g. SoC-internal registers with a low latency) it may even be performance relevant.

My example is from ia64, but I'm not advocating for the lookup table. The point is that the hardware works similarly (at least for dense ia64 I/O port spaces) in terms of mapping CPU physical addresses to PCI I/O space.

I think my confusion is because your pci_register_io_range() and pci_addess_to_pci() implementations assume that every io_range starts at I/O port 0 on PCI (correct me if I'm wrong). I suspect that's why you don't save the I/O port number in struct io_range.

Maybe that assumption is guaranteed by OF, but it doesn't hold for ACPI; ACPI can describe several I/O port apertures for a single bridge, each associated with a different CPU physical memory region.

That is actually a good catch, I've completely missed the fact that io_range->pci_addr could be non-zero.

If my speculation here is correct, a comment to the effect that each io_range corresponds to a PCI I/O space range that starts at 0 might be enough.

If you did add a PCI I/O port number argument to pci_register_io_range(), we might be able to make an ACPI-based implementation of it. But I guess that could be done if/when anybody ever wants to do that.

No, I think you are right, the PCI I/O port number needs to be recorded. I need to add that to pci_register_io_range().

...

...

Here's what these look like in /proc/iomem and /proc/ioports (note that there are two resource structs for each memory-mapped IO port space: one IORESOURCE_MEM for the memory-mapped area (used only by the host bridge driver), and one IORESOURCE_IO for the I/O port space (this becomes the parent of a region used by a regular device driver):

/proc/iomem: PCI Bus 0000:00 I/O Ports 00000000-00000fff PCI Bus 0001:00 I/O Ports 01000000-01000fff

Oops, I forgot the actual physical memory addresses here, but you got the idea anyway. It should have been something like this:

/proc/iomem: f8010000000-f8010000fff PCI Bus 0000:00 I/O Ports 00000000-00000fff f8110000000-f8110000fff PCI Bus 0001:00 I/O Ports 01000000-01000fff

Bjorn

Thanks for being thorough with your review.

Best regards, Liviu

On Wed, Jul 09, 2014 at 07:32:37AM +0100, Arnd Bergmann wrote:

On Wednesday 09 July 2014, Liviu Dudau wrote:

...

...

Maybe that assumption is guaranteed by OF, but it doesn't hold for ACPI; ACPI can describe several I/O port apertures for a single bridge, each associated with a different CPU physical memory region.

That is actually a good catch, I've completely missed the fact that io_range->pci_addr could be non-zero.

Hmm, that's what I thought in my initial review, but you convinced me that it's actually correct later on, and I still believe it is. Maybe now you got confused by your own code?

Man, it has been too long. Yes, I am now confused by my own code, which is not a good sign.

Please have another look, I think your code in pci_host_bridge_of_get_ranges sufficiently handles the registration to the PCI code with the correct io_offset. The only thing that we might want to add is to record the PCI address along with the bridge->io_base: For the host driver to set up the mapping window correctly, you either need both of them, or you assume they are already set up.

Hmm, having another look at pci_host_bridge_of_get_range() I'm not convinced that we need another storage for pci_addr. The resource gets added to the list of resources used by the bridge offsetted by range.pci_addr, so when re-creating the PCI bus address the value should come in play.

I will double check but I think the code is correct as it is. Sorry for the early confusion.

Best regards, Liviu

Arnd

On Wed, Jul 09, 2014 at 07:20:49AM +0100, Arnd Bergmann wrote:

On Tuesday 08 July 2014, Bjorn Helgaas wrote:

...

On Tue, Jul 8, 2014 at 1:00 AM, Arnd Bergmann arnd@arndb.de wrote:

...

On Tuesday 08 July 2014, Bjorn Helgaas wrote:

...

On Tue, Jul 01, 2014 at 07:43:28PM +0100, Liviu Dudau wrote:

...

+static LIST_HEAD(io_range_list);

+/*

• • Record the PCI IO range (expressed as CPU physical address + size).
• • Return a negative value if an error has occured, zero otherwise
• */

+int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)

I don't understand the interface here. What's the mapping from CPU physical address to bus I/O port? For example, I have the following machine in mind:

HWP0002:00: PCI Root Bridge (domain 0000 [bus 00-1b]) HWP0002:00: memory-mapped IO port space [mem 0xf8010000000-0xf8010000fff] HWP0002:00: host bridge window [io 0x0000-0x0fff]

HWP0002:09: PCI Root Bridge (domain 0001 [bus 00-1b]) HWP0002:09: memory-mapped IO port space [mem 0xf8110000000-0xf8110000fff] HWP0002:09: host bridge window [io 0x1000000-0x1000fff] (PCI address [0x0-0xfff])

The CPU physical memory [mem 0xf8010000000-0xf8010000fff] is translated by the bridge to I/O ports 0x0000-0x0fff on PCI bus 0000:00. Drivers use, e.g., "inb(0)" to access it.

Similarly, [mem 0xf8110000000-0xf8110000fff] is translated by the second bridge to I/O ports 0x0000-0x0fff on PCI bus 0001:00. Drivers use "inb(0x1000000)" to access it.

I guess you are thinking of the IA64 model here where you keep the virtual I/O port numbers in a per-bus lookup table that gets accessed for each inb() call. I've thought about this some more, and I believe there are good reasons for sticking with the model used on arm32 and powerpc for the generic OF implementation.

The idea is that there is a single virtual memory range for all I/O port mappings and we use the MMU to do the translation rather than computing it manually in the inb() implemnetation. The main advantage is that all functions used in device drivers to (potentially) access I/O ports become trivial this way, which helps for code size and in some cases (e.g. SoC-internal registers with a low latency) it may even be performance relevant.

My example is from ia64, but I'm not advocating for the lookup table. The point is that the hardware works similarly (at least for dense ia64 I/O port spaces) in terms of mapping CPU physical addresses to PCI I/O space.

I think my confusion is because your pci_register_io_range() and pci_addess_to_pci() implementations assume that every io_range starts at I/O port 0 on PCI (correct me if I'm wrong). I suspect that's why you don't save the I/O port number in struct io_range.

I think you are just misreading the code, but I agree it's hard to understand and I made the same mistake in my initial reply to the first version.

I am willing to make the code more easy to understand and validate. Proof that things are not that easy to check is that I've also got confused last night without having all the code in front of me. Any suggestions?

Best regards, Liviu

pci_register_io_range and pci_address_to_pci only worry about the mapping between CPU physical and Linux I/O address, they do not care which PCI port numbers are behind that. The mapping between PCI port numbers and Linux port numbers is done correctly in patch 8/9 in the pci_host_bridge_of_get_ranges() function.

...

Maybe that assumption is guaranteed by OF, but it doesn't hold for ACPI; ACPI can describe several I/O port apertures for a single bridge, each associated with a different CPU physical memory region.

DT can have the same, although the common case is that each PCI host bridge has 64KB of I/O ports starting at address 0. Most driver writers get it wrong for the case where it starts at a different address, so I really want to have a generic implementation that gets it right.

...

If my speculation here is correct, a comment to the effect that each io_range corresponds to a PCI I/O space range that starts at 0 might be enough.

If you did add a PCI I/O port number argument to pci_register_io_range(), we might be able to make an ACPI-based implementation of it. But I guess that could be done if/when anybody ever wants to do that.

I think we shoulnd't worry about it before we actually need it. As far as I understand, the only user of that code (unless someone wants to convert ia64) would be ARM64 with ACPI, but that uses the SBSA hardware model that recommends having no I/O space at all.

Arnd

On Tue, Jul 08, 2014 at 03:14:17PM +0100, Arnd Bergmann wrote:

On Tuesday 08 July 2014, Liviu Dudau wrote:

...

...

Here's what these look like in /proc/iomem and /proc/ioports (note that there are two resource structs for each memory-mapped IO port space: one IORESOURCE_MEM for the memory-mapped area (used only by the host bridge driver), and one IORESOURCE_IO for the I/O port space (this becomes the parent of a region used by a regular device driver):

/proc/iomem: PCI Bus 0000:00 I/O Ports 00000000-00000fff PCI Bus 0001:00 I/O Ports 01000000-01000fff

/proc/ioports: 00000000-00000fff : PCI Bus 0000:00 01000000-01000fff : PCI Bus 0001:00

OK, I have a question that might be ovbious to you but I have missed the answer so far: how does the IORESOURCE_MEM area gets created? Is it the host bridge driver's job to do it? Is it something that the framework should do when it notices that the IORESOURCE_IO is memory mapped?

The host bridge driver should either register the IORESOURCE_MEM resource itself from its probe or setup function, or it should get registered behind the covers in drivers using of_create_pci_host_bridge().

Your new pci_host_bridge_of_get_ranges already loops over all the resources, so it would be a good place to put that.

OK, so it is not something that I've missed, just something that x86-64 does and my version doesn't yet.

Thanks for confirming that.

Liviu

Arnd

On Sat, Jul 05, 2014 at 09:46:09PM +0100, Arnd Bergmann wrote:

On Saturday 05 July 2014 14:25:52 Rob Herring wrote:

...

On Tue, Jul 1, 2014 at 1:43 PM, Liviu Dudau Liviu.Dudau@arm.com wrote:

...

The ranges property for a host bridge controller in DT describes the mapping between the PCI bus address and the CPU physical address. The resources framework however expects that the IO resources start at a pseudo "port" address 0 (zero) and have a maximum size of IO_SPACE_LIMIT. The conversion from pci ranges to resources failed to take that into account.

I don't think this change is right. There are 2 resources: the PCI bus addresses and cpu addresses. This function deals with the cpu addresses. Returning pci addresses for i/o and cpu addresses for memory is going to be error prone. We probably need both cpu and pci resources exposed to host controllers.

Making the new function only deal with i/o bus resources and naming it of_pci_range_to_io_resource would be better.

I think you are correct that this change by itself is will break existing drivers that rely on the current behavior of of_pci_range_to_resource, but there is also something wrong with the existing implementation:

Either I'm very confused or I've managed to confuse everyone else. The I/O resources described using CPU addresses *are* using "pseudo" port based addresses (or at least that is my understanding and my reading of the code). Can you point me to a function that is expecting the IO resource to have the start address at the physical address of the mapped space?

I was trying to fix exactly this issue, that you cannot use the resource structure returned by this function in any call that is expecting an IO resource.

Rob, you can try this function with two host bridges. Patch [3/9] changes pci_address_to_pio() to calculate the offset of the range based on already registed ranges, so the first host bridge will have it's IO resources starting from zero, but the second host bridge will have .start offseted by the size of the IO space of the first bridge. That is not a PCI bus address AFAICT.

Best regards, Liviu

of_pci_range_to_resource() at the moment returns a the address in cpu address space (i.e. IORESOURCE_MEM) but sets the res->flags value to IORESOURCE_IO, which means it doesn't fit into the resource tree. Liviu's version gets that part right, and it would be nice to fix that eventually, however we do it here.

Arnd

On Mon, Jul 07, 2014 at 10:22:00PM +0100, Arnd Bergmann wrote:

On Monday 07 July 2014, Liviu Dudau wrote:

...

On Sat, Jul 05, 2014 at 09:46:09PM +0100, Arnd Bergmann wrote:

...

On Saturday 05 July 2014 14:25:52 Rob Herring wrote:

...

On Tue, Jul 1, 2014 at 1:43 PM, Liviu Dudau Liviu.Dudau@arm.com wrote:

...

The ranges property for a host bridge controller in DT describes the mapping between the PCI bus address and the CPU physical address. The resources framework however expects that the IO resources start at a pseudo "port" address 0 (zero) and have a maximum size of IO_SPACE_LIMIT. The conversion from pci ranges to resources failed to take that into account.

I don't think this change is right. There are 2 resources: the PCI bus addresses and cpu addresses. This function deals with the cpu addresses. Returning pci addresses for i/o and cpu addresses for memory is going to be error prone. We probably need both cpu and pci resources exposed to host controllers.

Making the new function only deal with i/o bus resources and naming it of_pci_range_to_io_resource would be better.

I think you are correct that this change by itself is will break existing drivers that rely on the current behavior of of_pci_range_to_resource, but there is also something wrong with the existing implementation:

Either I'm very confused or I've managed to confuse everyone else. The I/O resources described using CPU addresses *are* using "pseudo" port based addresses (or at least that is my understanding and my reading of the code). Can you point me to a function that is expecting the IO resource to have the start address at the physical address of the mapped space?

pci_v3_preinit() in arch/arm/mach-integrator/pci_v3.c for instance takes the resource returned by of_pci_range_to_resource and programs the start and size into hardware registers that expect a physical address as far as I can tell.

...

I was trying to fix exactly this issue, that you cannot use the resource structure returned by this function in any call that is expecting an IO resource.

I looked at the other drivers briefly, and I think you indeed fix the Tegra driver with this but break the integrator driver as mentioned above. The other callers of of_pci_range_to_resource() are apparently not impacted as they recalculate the values they get.

I would argue that integrator version is having broken assumptions. If it would try to allocate that IO range or request the resource as returned currently by of_pci_range_to_resource (without my patch) it would fail. I know because I did the same thing in my host bridge driver and it failed miserably. That's why I tried to patch it.

I will lay out my argument here and people can tell me if I am wrong:

PCI IO resources (even if they are memory mapped on certain architectures) need to emulate the x86 world "port" concept. Why do I think this? Because of this structure at the beginning of kernel/resource.c:

struct resource ioport_resource = { .name = "PCI IO", .start = 0, .end = IO_SPACE_LIMIT, .flags = IORESOURCE_IO, }; EXPORT_SYMBOL(ioport_resource);

The other resource that people seem to confuse it with is the next one in that file:

struct resource iomem_resource = { .name = "PCI mem", .start = 0, .end = -1, .flags = IORESOURCE_MEM, }; EXPORT_SYMBOL(iomem_resource);

Now, there are architecture that override the .start and .end values, but arm is not one of those, and mach-integrator doesn't change it either. So one can play with the ioport_resource values to move the "port" window wherever he/she wants, but it doesn't change the "port access" way of addressing it.

If the IO space is memory mapped, then we use the port number, the io_offset and the PCI_IOBASE to get to the virtual address that, when accessed, will generate the correct addresses on the bus, based on what the host bridge has been configured.

This is the current level of my understanding of PCI IO.

Now, I believe Rob has switched entirely to using my series in some test that he has run and he hasn't encountered any issues, as long as one remembers in the host bridge driver to add the io_base offset to the .start resource. If not then I need to patch pci_v3.c.

Best regards, Liviu

Arnd

On Wed, Jul 09, 2014 at 09:31:50AM +0100, Arnd Bergmann wrote:

On Tuesday 08 July 2014, Liviu Dudau wrote:

...

On Mon, Jul 07, 2014 at 10:22:00PM +0100, Arnd Bergmann wrote:

...

I looked at the other drivers briefly, and I think you indeed fix the Tegra driver with this but break the integrator driver as mentioned above. The other callers of of_pci_range_to_resource() are apparently not impacted as they recalculate the values they get.

I would argue that integrator version is having broken assumptions. If it would try to allocate that IO range or request the resource as returned currently by of_pci_range_to_resource (without my patch) it would fail. I know because I did the same thing in my host bridge driver and it failed miserably. That's why I tried to patch it.

The integrator code was just introduced and the reason for how it does things is the way that of_pci_range_to_resource() works today. We tried to cope with it and not change the existing behavior in order to not break any other drivers.

It's certainly not fair to call the integrator version broken, it just works around the common code having a quirky interface. We should probably have done of_pci_range_to_resource better than it is today (I would have argued for it to return an IORESOURCE_MEM with the CPU address), but it took long enough to get that merged and I was sick of arguing about it.

Understood. That is why I have carefully worded my email as not to diss anyone. I didn't say the code is broken, I've said it has broken assumptions.

...

If the IO space is memory mapped, then we use the port number, the io_offset and the PCI_IOBASE to get to the virtual address that, when accessed, will generate the correct addresses on the bus, based on what the host bridge has been configured.

This is the current level of my understanding of PCI IO.

Your understanding is absolutely correct, and that's great because very few people get that right. What I think we're really arguing about is what the of_pci_range_to_resource is supposed to return. As you and Bjorn both pointed out earlier, there are in fact two resources associated with the I/O window and the flaw in the current implementation is that of_pci_range_to_resource returns the numeric values for the IORESOURCE_MEM resource, but sets the type to IORESOURCE_IO, which is offset from that by PCI_IOBASE.

You try to fix that by making it return the correct IORESOURCE_IO resource, which is a reasonable approach but you must not break drivers that rely on the broken resource while doing that.

Or I need to fix the existing drivers that rely on the old behaviour.

The approach that I would have picked is to return the IORESOURCE_MEM resource associated with the I/O window and pick a (basically random) IORESOURCE_IO resource struct based on what hasn't been used and then compute the appropriate io_offset from that. This approach of course would also have required fixing up all drivers relying on the current behavior.

To be clear, I'm fine with you (and Bjorn if he cares) picking the approach you like here, either one of these works fine as long as the host drivers use the interface in the way it is defined.

OK. Thanks for that. It does look like either way some existing code needs fixing, so I will have a look at that. Unless Bjorn votes for making a new version of pci_range_to_resource().

...

Now, I believe Rob has switched entirely to using my series in some test that he has run and he hasn't encountered any issues, as long as one remembers in the host bridge driver to add the io_base offset to the .start resource. If not then I need to patch pci_v3.c.

The crazy part of all these discussions is that basically nobody ever uses I/O port access, so it's very hard to test and we don't even notice when we get it wrong, but we end up spending most of the time for PCI host controller reviews trying to get these right.

I'm very thankful that you are doing this work to get it moved into common code so hopefully this is the last time we ever have to worry about it because all future drivers will be able to use the common implemnetation.

Ahh, we humans! We always hope for the best! :)

My only chance of succeeding is if I make it a no brainer for people to use the code. At the moment the interface for host bridge drivers is not too bad, but it looks like the internals are still hard to comprehend.

Best regards, Liviu

Arnd

On Wed, Jul 16, 2014 at 03:35:37PM +0100, Rob Herring wrote:

On Wed, Jul 9, 2014 at 3:31 AM, Arnd Bergmann arnd@arndb.de wrote:

...

On Tuesday 08 July 2014, Liviu Dudau wrote:

...

On Mon, Jul 07, 2014 at 10:22:00PM +0100, Arnd Bergmann wrote:

...

I looked at the other drivers briefly, and I think you indeed fix the Tegra driver with this but break the integrator driver as mentioned above. The other callers of of_pci_range_to_resource() are apparently not impacted as they recalculate the values they get.

I would argue that integrator version is having broken assumptions. If it would try to allocate that IO range or request the resource as returned currently by of_pci_range_to_resource (without my patch) it would fail. I know because I did the same thing in my host bridge driver and it failed miserably. That's why I tried to patch it.

The integrator code was just introduced and the reason for how it does things is the way that of_pci_range_to_resource() works today. We tried to cope with it and not change the existing behavior in order to not break any other drivers.

It's certainly not fair to call the integrator version broken, it just works around the common code having a quirky interface. We should probably have done of_pci_range_to_resource better than it is today (I would have argued for it to return an IORESOURCE_MEM with the CPU address), but it took long enough to get that merged and I was sick of arguing about it.

...

If the IO space is memory mapped, then we use the port number, the io_offset and the PCI_IOBASE to get to the virtual address that, when accessed, will generate the correct addresses on the bus, based on what the host bridge has been configured.

This is the current level of my understanding of PCI IO.

What is io_offset supposed to be and be based on?

io_offset is the offset that gets applied for each host bridge to the port number to get the offset from PCI_IOBASE. Basically, the second host bridge will have port numbers starting from zero like the first one in the system, but the io_offset will be >= largest port number in the first host bridge.

...

Your understanding is absolutely correct, and that's great because very few people get that right. What I think we're really arguing about is what the of_pci_range_to_resource is supposed to return. As you and Bjorn both pointed out earlier, there are in fact two resources associated with the I/O window and the flaw in the current implementation is that of_pci_range_to_resource returns the numeric values for the IORESOURCE_MEM resource, but sets the type to IORESOURCE_IO, which is offset from that by PCI_IOBASE.

You try to fix that by making it return the correct IORESOURCE_IO resource, which is a reasonable approach but you must not break drivers that rely on the broken resource while doing that.

The approach that I would have picked is to return the IORESOURCE_MEM resource associated with the I/O window and pick a (basically random) IORESOURCE_IO resource struct based on what hasn't been used and then compute the appropriate io_offset from that. This approach of course would also have required fixing up all drivers relying on the current behavior.

To be clear, I'm fine with you (and Bjorn if he cares) picking the approach you like here, either one of these works fine as long as the host drivers use the interface in the way it is defined.

...

Now, I believe Rob has switched entirely to using my series in some test that he has run and he hasn't encountered any issues, as long as one remembers in the host bridge driver to add the io_base offset to the .start resource. If not then I need to patch pci_v3.c.

The crazy part of all these discussions is that basically nobody ever uses I/O port access, so it's very hard to test and we don't even notice when we get it wrong, but we end up spending most of the time for PCI host controller reviews trying to get these right.

FWIW, I test i/o accesses with Versatile QEMU. The LSI53xxxx device in the model has a kconfig option to use i/o accesses. However, I have seen in the past this is an area where 2 wrongs can make a right.

:)

Best regards, Liviu

Rob

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.

Bjorn

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.

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.

Bjorn

On Thu, Jul 10, 2014 at 3:47 AM, Liviu Dudau Liviu.Dudau@arm.com wrote:

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.

A lot of the v7 discussion was about pci_register_io_range(). I apologize, because I think I really derailed things there and it was unwarranted. Arnd was right that migrating other arches should be a separate effort. I *think* I was probably thinking about the proposal of adding pci_create_root_bus_in_domain(), and my reservations about that got transferred to the pci_register_io_range() discussion. In any case, I'm completely fine with pci_register_io_range() now.

Most of the rest of the v7 discussion was about "Introduce a domain number for pci_host_bridge." I think we should add arm64 using the existing pci_scan_root_bus() and keep the domain number in the arm64 sysdata structure like every other arch does. Isn't that feasible? We can worry about domain unification later.

I haven't followed closely enough to know what other objections people had.

Bjorn

On Thu, Jul 10, 2014 at 11:36:10PM +0100, Bjorn Helgaas wrote:

Most of the rest of the v7 discussion was about "Introduce a domain number for pci_host_bridge." I think we should add arm64 using the existing pci_scan_root_bus() and keep the domain number in the arm64 sysdata structure like every other arch does. Isn't that feasible? We can worry about domain unification later.

I think that's what we were trying to avoid, adding an arm64-specific pci_sys_data structure (and arm64-specific API). IIUC, avoiding this would allow the host controller drivers to use the sysdata pointer for their own private data structures.

Also since you can specify the domain number via DT (and in Liviu's v8 patches read by of_create_pci_host_bridge), I think it would make sense to have it stored in some generic data structures (e.g. pci_host_bridge) rather than in an arm64 private sysdata.

(Liviu is thinking of an alternative API but maybe he could briefly describe it here before posting a new series)

On Fri, Jul 11, 2014 at 04:08:23PM +0100, Liviu Dudau wrote:

On Fri, Jul 11, 2014 at 03:11:16PM +0100, Catalin Marinas wrote:

...

On Thu, Jul 10, 2014 at 11:36:10PM +0100, Bjorn Helgaas wrote:

...

Most of the rest of the v7 discussion was about "Introduce a domain number for pci_host_bridge." I think we should add arm64 using the existing pci_scan_root_bus() and keep the domain number in the arm64 sysdata structure like every other arch does. Isn't that feasible? We can worry about domain unification later.

I think that's what we were trying to avoid, adding an arm64-specific pci_sys_data structure (and arm64-specific API). IIUC, avoiding this would allow the host controller drivers to use the sysdata pointer for their own private data structures.

Also since you can specify the domain number via DT (and in Liviu's v8 patches read by of_create_pci_host_bridge), I think it would make sense to have it stored in some generic data structures (e.g. pci_host_bridge) rather than in an arm64 private sysdata.

(Liviu is thinking of an alternative API but maybe he could briefly describe it here before posting a new series)

My plan is to keep the domain number in the pci_host_bridge and split the creation of the pci_host_bridge out of the pci_create_root_bus().

Wouldn't it make more sense to add domain_nr to the pci_bus structure (well, only needed for the root bus)? It would simplify pci_domain_nr() as well which only takes a pci_bus parameter.

The new function (tentatively called pci_create_new_root_bus()) will no longer call pci_alloc_host_bridge() but will accept it as a parameter, allowing one to be able to set the domain_nr ahead of the root bus creation.

If we place domain_nr in pci_bus, this split wouldn't help but we still need your original pci_create_root_bus_in_domain(). Are there other uses of your proposal above?

Yet another alternative is to ignore PCI domains altogether (domain 0 always).

On Fri, Jul 11, 2014 at 06:02:56PM +0100, Bjorn Helgaas wrote:

On Fri, Jul 11, 2014 at 8:11 AM, Catalin Marinas catalin.marinas@arm.com wrote:

...

On Thu, Jul 10, 2014 at 11:36:10PM +0100, Bjorn Helgaas wrote:

...

Most of the rest of the v7 discussion was about "Introduce a domain number for pci_host_bridge." I think we should add arm64 using the existing pci_scan_root_bus() and keep the domain number in the arm64 sysdata structure like every other arch does. Isn't that feasible? We can worry about domain unification later.

I think that's what we were trying to avoid, adding an arm64-specific pci_sys_data structure (and arm64-specific API). IIUC, avoiding this would allow the host controller drivers to use the sysdata pointer for their own private data structures.

Also since you can specify the domain number via DT (and in Liviu's v8 patches read by of_create_pci_host_bridge), I think it would make sense to have it stored in some generic data structures (e.g. pci_host_bridge) rather than in an arm64 private sysdata.

It would definitely be nice to keep the domain in a generic data structure rather than an arm64-specific one. But every other arch keeps it in an arch-specific structure today, and I think following that existing pattern is the quickest way forward.

In this case we end up with an arm64-specific struct pci_sys_data and I assume some API that takes care of this data structure to populate the domain nr.

In Liviu's implementation, of_create_pci_host_bridge() is called by the host controller driver directly and reads the domain_nr from the DT. It also gets a void *host_data which it stores as sysdata in the pci_bus structure (but that's specific to the host controller driver rather than arm64). Since sysdata is opaque to of_create_pci_host_bridge(), it cannot set the domain_nr.

If we go for arm64 sysdata, the host controller driver would have to call some arm64 pci* function (maybe with its own private data as argument) which would have to parse the DT, assign the domain nr and eventually call pci_create_root_bus(). But it means that significant part of of_create_pci_host_bridge() would be moved under arch/arm64 (an alternative is for each host driver to implement its own of_create_pci_host_bridge()).

In addition, host drivers would retrieve their private data from the arm64 specific sysdata structure and we were trying to make host drivers depend only on generic data structures and API rather than arch specific.

But you mentioned arm64-specific API, too. What do you have in mind there? I know there will be arm64 implementations of various pcibios_*() functions (just like for every other arch), but it sounds like there might be something else?

Not much left which is arm64 specific, just some callbacks:

http://linux-arm.org/git?p=linux-ld.git%3Ba=commitdiff%3Bh=82ebbce3450667652...

AFAICT, there isn't anything that host drivers need to call directly. Basically we want to decouple the PCI host driver model from the arch specific data structures/API.

Hi Liviu,

On Tue, Jul 01, 2014 at 07:43:33PM +0100, Liviu Dudau wrote:

Several platforms use a rather generic version of parsing the device tree to find the host bridge ranges. Move the common code into the generic PCI code and use it to create a pci_host_bridge structure that can be used by arch code.

Based on early attempts by Andrew Murray to unify the code. Used powerpc and microblaze PCI code as starting point.

I just had a quick look at this to see how it differs from the parsing in pci-host-generic.c and there a few small differences worth discussing.

+static int pci_host_bridge_of_get_ranges(struct device_node *dev,

• struct list_head *resources, resource_size_t *io_base)
  

+{

• struct resource *res;
• struct of_pci_range range;
• struct of_pci_range_parser parser;
• int err;
• pr_info("PCI host bridge %s ranges:\n", dev->full_name);
• /* Check for ranges property */
• err = of_pci_range_parser_init(&parser, dev);
• if (err)

• return err;
  

• pr_debug("Parsing ranges property...\n");
• for_each_of_pci_range(&parser, &range) {

• /* Read next ranges element */
  

• pr_debug("pci_space: 0x%08x pci_addr:0x%016llx cpu_addr:0x%016llx size:0x%016llx\n",
  

• 	range.pci_space, range.pci_addr, range.cpu_addr, range.size);
  

• /*
  

•  * If we failed translation or got a zero-sized region
  

•  * then skip this range
  

•  */
  

• if (range.cpu_addr == OF_BAD_ADDR || range.size == 0)
  

• 	continue;
  

• res = kzalloc(sizeof(struct resource), GFP_KERNEL);
  

• if (!res)
  

• 	return -ENOMEM;
  

• err = of_pci_range_to_resource(&range, dev, res);
  

• if (err)
  

• 	return err;
  

• if (resource_type(res) == IORESOURCE_IO)
  

• 	*io_base = range.cpu_addr;
  

• pci_add_resource_offset(resources, res,
  

• 		res->start - range.pci_addr);
  

Where do you request_resource before adding it?

• }
• /* Apply architecture specific fixups for the ranges */
• return pcibios_fixup_bridge_ranges(resources);

I currently mandate at least one non-prefetchable resource in the device-tree. Should I simply drop this restriction, or do I have to somehow hook this into the pcibios callback?

+}

+static atomic_t domain_nr = ATOMIC_INIT(-1);

+/**

• • of_create_pci_host_bridge - Create a PCI host bridge structure using
• • information passed in the DT.
• • @parent: device owning this host bridge
• • @ops: pci_ops associated with the host controller
• • @host_data: opaque data structure used by the host controller.
• • returns a pointer to the newly created pci_host_bridge structure, or
• • NULL if the call failed.
• • This function will try to obtain the host bridge domain number by
• • using of_alias_get_id() call with "pci-domain" as a stem. If that
• • fails, a local allocator will be used that will put each host bridge
• • in a new domain.
• */

+struct pci_host_bridge * +of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops, void *host_data) +{

• int err, domain, busno;
• struct resource *bus_range;
• struct pci_bus *root_bus;
• struct pci_host_bridge *bridge;
• resource_size_t io_base;
• LIST_HEAD(res);
• bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
• if (!bus_range)

• return ERR_PTR(-ENOMEM);
  

• domain = of_alias_get_id(parent->of_node, "pci-domain");
• if (domain == -ENODEV)

• domain = atomic_inc_return(&domain_nr);
  

• err = of_pci_parse_bus_range(parent->of_node, bus_range);
• if (err) {

• dev_info(parent, "No bus range for %s, using default [0-255]\n",
  

• 	parent->of_node->full_name);
  

• bus_range->start = 0;
  

• bus_range->end = 255;
  

• bus_range->flags = IORESOURCE_BUS;
  

What about bus_range->name?

• }
• busno = bus_range->start;
• pci_add_resource(&res, bus_range);

I currently truncate the bus range to fit inside the Configuration Space window I have (in the reg property). How can I continue to do that with this patch?

Will

On Wed, Jul 02, 2014 at 06:23:55PM +0100, Liviu Dudau wrote:

On Wed, Jul 02, 2014 at 12:22:30PM +0100, Will Deacon wrote:

...

On Tue, Jul 01, 2014 at 07:43:33PM +0100, Liviu Dudau wrote:

...

Several platforms use a rather generic version of parsing the device tree to find the host bridge ranges. Move the common code into the generic PCI code and use it to create a pci_host_bridge structure that can be used by arch code.

Based on early attempts by Andrew Murray to unify the code. Used powerpc and microblaze PCI code as starting point.

I just had a quick look at this to see how it differs from the parsing in pci-host-generic.c and there a few small differences worth discussing.

[...]

...

...

• }
• /* Apply architecture specific fixups for the ranges */
• return pcibios_fixup_bridge_ranges(resources);

I currently mandate at least one non-prefetchable resource in the device-tree. Should I simply drop this restriction, or do I have to somehow hook this into the pcibios callback?

Don't think I understand why you need at least one non-prefetcheable resource but if you want to mandate that then the pcibios_fixup_bridge_ranges() would be the place to put that check.

I think it was Arnd's idea at the time:

http://lists.infradead.org/pipermail/linux-arm-kernel/2014-February/232225.h...

and it's probably worth keeping if possible (just to avoid changes to the behaviour of the existing driver).

However, that means I already need a host-controller callback via pcibios_fixup_bridge_ranges...

...

...

• err = of_pci_parse_bus_range(parent->of_node, bus_range);
• if (err) {

• dev_info(parent, "No bus range for %s, using default [0-255]\n",
  

• 	parent->of_node->full_name);
  

• bus_range->start = 0;
  

• bus_range->end = 255;
  

• bus_range->flags = IORESOURCE_BUS;
  

What about bus_range->name?

Don't know! Is anyone using it?

I guess /proc/iomem prints it out? I set it in my current driver, if you want to take a look.

...

...

• }
• busno = bus_range->start;
• pci_add_resource(&res, bus_range);

I currently truncate the bus range to fit inside the Configuration Space window I have (in the reg property). How can I continue to do that with this patch?

Not easily. Unless I add an argument to this function that will allow you to pass in the max number for the bus range, then the code becomes:

• err = of_pci_parse_bus_range(parent->of_node, bus_range);
• if (err) {

• dev_info(parent, "No bus range for %s, using default [0-%d]\n",
  

• 	parent->of_node->full_name, max_range);
  

• bus_range->start = 0;
  

• bus_range->end = max_range;
  

• bus_range->flags = IORESOURCE_BUS;
  

• } else {

• if (bus_range->end > bus_range->start + max_range) {
  

• 	bus_range->end = bus_range->start + max_range;
  

• }
  

• }

Or something like that.

Again, take a look at my driver (it's in mainline now) to see how I deal with this.

Will

On Fri, Jul 11, 2014 at 08:43:21AM +0100, Jingoo Han wrote:

On Wednesday, July 02, 2014 3:44 AM, Liviu Dudau wrote:

...

Several platforms use a rather generic version of parsing the device tree to find the host bridge ranges. Move the common code into the generic PCI code and use it to create a pci_host_bridge structure that can be used by arch code.

Based on early attempts by Andrew Murray to unify the code. Used powerpc and microblaze PCI code as starting point.

Signed-off-by: Liviu Dudau Liviu.Dudau@arm.com Tested-by: Tanmay Inamdar tinamdar@apm.com

---

drivers/of/of_pci.c | 135 ++++++++++++++++++++++++++++++++++++++++++++++ drivers/pci/host-bridge.c | 18 +++++++ include/linux/of_pci.h | 10 ++++ include/linux/pci.h | 8 +++ 4 files changed, 171 insertions(+)

diff --git a/drivers/of/of_pci.c b/drivers/of/of_pci.c index 8481996..55d8320 100644 --- a/drivers/of/of_pci.c +++ b/drivers/of/of_pci.c

[.....]

...

+struct pci_host_bridge * +of_create_pci_host_bridge(struct device *parent, struct pci_ops *ops, void *host_data) +{

• int err, domain, busno;
• struct resource *bus_range;
• struct pci_bus *root_bus;
• struct pci_host_bridge *bridge;
• resource_size_t io_base;
• LIST_HEAD(res);
• bus_range = kzalloc(sizeof(*bus_range), GFP_KERNEL);
• if (!bus_range)

• return ERR_PTR(-ENOMEM);
  

• domain = of_alias_get_id(parent->of_node, "pci-domain");
• if (domain == -ENODEV)

• domain = atomic_inc_return(&domain_nr);
  

• err = of_pci_parse_bus_range(parent->of_node, bus_range);
• if (err) {

• dev_info(parent, "No bus range for %s, using default [0-255]\n",
  

• 	parent->of_node->full_name);
  

• bus_range->start = 0;
  

• bus_range->end = 255;
  

• bus_range->flags = IORESOURCE_BUS;
  

• }
• busno = bus_range->start;
• pci_add_resource(&res, bus_range);
• /* now parse the rest of host bridge bus ranges */
• err = pci_host_bridge_of_get_ranges(parent->of_node, &res, &io_base);
• if (err)

• goto err_create;
  

• /* then create the root bus */
• root_bus = pci_create_root_bus_in_domain(parent, domain, busno,

• 				ops, host_data, &res);
  

• if (IS_ERR(root_bus)) {

• err = PTR_ERR(root_bus);
  

• goto err_create;
  

• }
• bridge = to_pci_host_bridge(root_bus->bridge);
• bridge->io_base = io_base;

Hi Liviu Dudau,

Would you fix the following warning?

drivers/of/of_pci.c: In function 'of_create_pci_host_bridge' drivers/of/of_pci.c:218:18: warning: 'of_base' may be used uninitialized in this function [-Wmaybe-uninitialized] bridge->io_base = io_base;

Yes, I have a simple fix which is to set the initial value to zero when declaring the variable.

Best regards, Liviu

Best regards, Jingoo Han

[.....]

On Tue, Jul 01, 2014 at 07:43:34PM +0100, Liviu Dudau wrote:

Introduce a default implementation for remapping PCI bus I/O resources onto the CPU address space. Architectures with special needs may provide their own version, but most should be able to use this one.

[...]

+/**

• • pci_remap_iospace - Remap the memory mapped I/O space
• • @res: Resource describing the I/O space
• • @phys_addr: physical address where the range will be mapped.
• • Remap the memory mapped I/O space described by the @res
• • into the CPU physical address space. Only architectures
• • that have memory mapped IO defined (and hence PCI_IOBASE)
• • should call this function.
• */

+int __weak pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr) +{

• int err = -ENODEV;

+#ifdef PCI_IOBASE

• if (!(res->flags & IORESOURCE_IO))

• return -EINVAL;
  

• if (res->end > IO_SPACE_LIMIT)

• return -EINVAL;
  

• err = ioremap_page_range(res->start + (unsigned long)PCI_IOBASE,

• 		res->end + 1 + (unsigned long)PCI_IOBASE,
  

• 		phys_addr, __pgprot(PROT_DEVICE_nGnRE));