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

On Tue, Jul 8, 2014 at 1:00 AM, Arnd Bergmann [off-list ref] wrote:

quoted

On Tuesday 08 July 2014, Bjorn Helgaas wrote:

quoted

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

quoted

+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().

quoted

quoted

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

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