On Sat, Aug 05, 2017 at 12:52:43PM +0800, Ryder Lee wrote:

Hi Honghui, Bjorn,

On Fri, 2017-08-04 at 08:18 -0500, Bjorn Helgaas wrote:

quoted

On Fri, Aug 04, 2017 at 04:39:36PM +0800, Honghui Zhang wrote:

quoted

On Thu, 2017-08-03 at 17:42 -0500, Bjorn Helgaas wrote:

quoted

quoted

+
+static struct mtk_pcie_port *mtk_pcie_find_port(struct mtk_pcie *pcie,
+						struct pci_bus *bus, int devfn)
+{
+	struct pci_dev *dev;
+	struct pci_bus *pbus;
+	struct mtk_pcie_port *port, *tmp;
+
+	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
+		if (bus->number == 0 && port->index == PCI_SLOT(devfn)) {
+			return port;
+		} else if (bus->number != 0) {
+			pbus = bus;
+			do {
+				dev = pbus->self;
+				if (port->index == PCI_SLOT(dev->devfn))
+					return port;
+				pbus = dev->bus;
+			} while (dev->bus->number != 0);
+		}
+	}
+
+	return NULL;

You should be able to use sysdata to avoid searching the list.
See drivers/pci/host/pci-aardvark.c, for example.

I could put the mtk_pcie * in sysdata, but still need to searching the
list to get the mtk_pcie_port *, how about:

	list_for_each_entry_safe(port, tmp, &pcie->ports, list) {
		if (port->index == PCI_SLOT(devfn))
			return port;
	}

No.  Other drivers don't need to search the list.  Please take a look
at them and see how they solve this problem.  I don't think your
hardware is fundamentally different in a way that means you need to
search when the others don't.

I'm not directly involved in this generation, but I guess the main
reason why Honghui need to do that is just because this hardware
access configuration space via per-port registers, not just for the
guard.  Currently, We had a host bridge with two ports (two subnodes
in binding text), thus he tried to tells them apart so that he can
get the correct registers.

Some platforms don't need to do that since they just have a single
port (no more subnodes), the others might have specific/shared
registers to access configuration space. (e.g. Tegra, MTK legacy IP
block).  Or, he can split them into two independent nodes, but it
will break common probing flow by doing so. (I'd prefer to use
subnodes.)

The PCI core interface (pci_scan_root_bus_bridge()) starts with
sysdata in the struct pci_host_bridge, so every PCI bus under that
host bridge has the same sysdata.  

You have multiple root ports (sounds like two ports in this case)
under that host bridge.  Each port has independent interrupt mappings,
but the MMIO address space routed to the ports is described in the
upper-level host bridge (the "pcie" node in DT).  I assume the I/O and
MMIO routing through the root ports works as described in the
PCI-to-PCI bridge spec, using PCI_MEMORY_BASE, PCI_PREF_MEMORY_BASE,
etc.

If that's the case, I think your current DT "pcie" node is
appropriate, and both ports should have the same sysdata (as they do
in your current patch), and you do need some additional way to get
from that sysdata (the struct mtk_pcie) to the per-port data (the
struct mtk_pcie_port).

Apparently the two root ports of this MT7622/MT2712 controller are
hardwired at device (PCI_SLOT) 0 and 1.  Using a list to look that up
seems like a little overkill, since you could index an array by
PCI_SLOT(), but I guess you could do it either way.

I would probably rename "port->index" to "port->slot" or similar to
make it more obvious that it's not merely the Nth port we found; it's
the one that is hardwired at PCI_SLOT N.

Also, s/mtk_pcie_parse_ports/mtk_pcie_parse_port/, since it parses one
port each time you call it.

And factor out the lookup as Ryder suggested so it's not duplicated in
the mtk_pcie_config_read()/mtk_pcie_config_write() paths.

Bjorn
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html