On 31/01/2017 00:10, Bjorn Helgaas wrote:

On Tue, Jan 24, 2017 at 03:05:22PM +0800, zhichang.yuan wrote:

quoted

After indirect-IO is introduced, system must can assigned indirect-IO devices
with logical I/O ranges which are different from those for PCI I/O devices.
Otherwise, I/O accessors can't identify whether the I/O port is for memory
mapped I/O or indirect-IO.

Maybe:

  We must assign logical I/O port space for indirect I/O such that the
  I/O accessors can tell whether a logical I/O port refers to memory-
  mapped I/O space or indirect I/O space.

It's better

quoted

As current helper, pci_register_io_range(), is used for PCI I/O ranges
registration and translation, indirect-IO devices should also apply these
helpers to manage the I/O ranges. It will be easy to ensure the assigned
logical I/O ranges unique.
But for indirect-IO devices, there is no cpu address. The current
pci_register_io_range() can not work for this case.

This patch makes some changes on the pci_register_io_range() to support the
I/O range registration with device's fwnode also. After this, the indirect-IO
devices can register the device-local I/O range to system logical I/O and
easily perform the translation between device-local I/O range and sytem
logical I/O range.

quoted

-int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
+int __weak pci_register_io_range(struct fwnode_handle *node, phys_addr_t addr,
+				 resource_size_t size, unsigned long *port)

Why is this __weak?  It looks like it's been __weak since its
introduction by 41f8bba7f555 ("of/pci: Add pci_register_io_range() and
pci_pio_to_address()"), but I don't see any other implementations of
it.

Can you add a patch that does nothing but make this non-weak?

OK

quoted

+#else
+	/*
+	 * powerpc and microblaze have their own registration,
+	 * just look up the value here

Can you include a pointer to the powerpc and microblaze registration
code here?  It's conceivable that somebody could generalize this
enough to support powerpc and microblaze as well.

It should be no problem

quoted

--- a/include/linux/pci.h
+++ b/include/linux/pci.h

@@ -34,6 +34,9 @@

 #include <linux/pci_ids.h>

+/* the macro below flags an invalid cpu address
+ * and is used by IO special hosts              */

s/cpu/CPU/

OK

Use conventional multi-line comment style:

/*
 * IO_RANGE_IOEXT flags an invalid CPU address ...
 */

quoted

+#define IO_RANGE_IOEXT (resource_size_t)(-1ull)

And put this close to related things, e.g., pci_register_io_range(),
instead of just dropping it in at the top of the file.

OK

quoted

 /*
  * The PCI interface treats multi-function devices as independent
  * devices.  The slot/function address of each device is encoded

@@ -1197,8 +1200,8 @@ int __must_check pci_bus_alloc_resource(struct pci_bus *bus,
 						  resource_size_t),
 			void *alignf_data);

-
-int pci_register_io_range(phys_addr_t addr, resource_size_t size);
+int pci_register_io_range(struct fwnode_handle *node, phys_addr_t addr,
+			  resource_size_t size, unsigned long *port);
 unsigned long pci_address_to_pio(phys_addr_t addr);
 phys_addr_t pci_pio_to_address(unsigned long pio);
 int pci_remap_iospace(const struct resource *res, phys_addr_t phys_addr);
--

1.9.1

Thanks,
John

.