On Tue, Sep 16, 2014 at 03:33:41PM -0700, Tanmay Inamdar wrote:

This patch adds the AppliedMicro X-Gene SOC PCIe host controller driver.
X-Gene PCIe controller supports maximum up to 8 lanes and GEN3 speed.
X-Gene SOC supports maximum 5 PCIe ports.

Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com>
Signed-off-by: Tanmay Inamdar <redacted>
---
 drivers/pci/host/Kconfig     |  10 +
 drivers/pci/host/Makefile    |   1 +
 drivers/pci/host/pci-xgene.c | 646 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 657 insertions(+)
 create mode 100644 drivers/pci/host/pci-xgene.c
...

+static inline void
+xgene_pcie_cfg_in16(void __iomem *addr, int offset, u32 *val)

Whitespace - can fit on one line.  Also others below.

+{
+	*val = readl(addr + (offset & ~0x3));
+
+	switch (offset & 0x3) {
+	case 2:
+		*val >>= 16;
+		break;
+	}
+
+	*val &= 0xFFFF;
+}
+
+static inline void
+xgene_pcie_cfg_in8(void __iomem *addr, int offset, u32 *val)
+{
+	*val = readl(addr + (offset & ~0x3));
+
+	switch (offset & 0x3) {
+	case 3:
+		*val = *val >> 24;
+		break;
+	case 2:
+		*val = *val >> 16;
+		break;
+	case 1:
+		*val = *val >> 8;
+		break;
+	}
+	*val &= 0xFF;
+}
+
+/* When the address bit [17:16] is 2'b01, the Configuration access will be
+ * treated as Type 1 and it will be forwarded to external PCIe device.
+ */

Follow usual block comment style:

    /*
     * text
     */

...
+static void xgene_pcie_fixup_bridge(struct pci_dev *dev)
+{
+	int i;
+
+	/* Hide the PCI host BARs from the kernel as their content doesn't
+	 * fit well in the resource management
+	 */

This needs a better explanation than "doesn't fit well."

I *think* you're probably talking about something similar to the MVEBU
devices mentioned here:
http://lkml.kernel.org/r/CAErSpo56jB1Bf2JtYCGKXZBZqRF1jXFxGmeewPX_e6vSXueGyA at mail.gmail.com

where the device can be configured as either an endpoint or a root port,
and the endpoint BARs are still visible when configured as a root port.

In any event, I'd like a description of exactly what these BARs are and wha
the problem is.  Presumably the BARs exist and were sized by the PCI core
in __pci_read_base().  That will generate some log messages and possibly
some warnings, depending on how the host bridge windows are set up.

We might eventually need a way to skip BARs like that altogether so we
don't even try to size them.

+	for (i = 0; i < DEVICE_COUNT_RESOURCE; i++) {
+		dev->resource[i].start = dev->resource[i].end = 0;
+		dev->resource[i].flags = 0;
+	}
+	dev_info(&dev->dev, "Hiding X-Gene pci host bridge resources %s\n",
+		 pci_name(dev));
+}
+DECLARE_PCI_FIXUP_HEADER(XGENE_PCIE_VENDORID, XGENE_PCIE_DEVICEID,
+			 xgene_pcie_fixup_bridge);
+
...

+static void xgene_pcie_setup_ob_reg(struct xgene_pcie_port *port,
+				    struct resource *res, u32 offset,
+				    u64 cpu_addr, u64 pci_addr)
+{
+	void __iomem *base = port->csr_base + offset;
+	resource_size_t size = resource_size(res);
+	u64 restype = resource_type(res);
+	u64 mask = 0;
+	u32 min_size;
+	u32 flag = EN_REG;
+
+	if (restype == IORESOURCE_MEM) {
+		min_size = SZ_128M;
+	} else {
+		min_size = 128;
+		flag |= OB_LO_IO;
+	}
+
+	if (size >= min_size)
+		mask = ~(size - 1) | flag;
+	else
+		dev_warn(port->dev, "res size 0x%llx less than minimum 0x%x\n",
+			 (u64)size, min_size);

I'd include a %pR here to help identify the offending resource.

+static int xgene_pcie_map_ranges(struct xgene_pcie_port *port,
+				 struct list_head *res,
+				 resource_size_t io_base)
+{
+	struct pci_host_bridge_window *window;
+	struct device *dev = port->dev;
+	int ret;
+
+	list_for_each_entry(window, res, list) {
+		struct resource *res = window->res;
+		u64 restype = resource_type(res);
+
+		dev_dbg(port->dev, "0x%08lx 0x%016llx...0x%016llx\n",
+			res->flags, res->start, res->end);

Use %pR here.

+
+		switch (restype) {
+		case IORESOURCE_IO:
+			xgene_pcie_setup_ob_reg(port, res, OMR3BARL, io_base,
+						res->start - window->offset);
+			ret = pci_remap_iospace(res, io_base);
+			if (ret < 0)
+				return ret;
+			break;
+		case IORESOURCE_MEM:
+			xgene_pcie_setup_ob_reg(port, res, OMR1BARL, res->start,
+						res->start - window->offset);
+			break;
+		case IORESOURCE_BUS:
+			break;
+		default:
+			dev_err(dev, "invalid io resource!");

If you're going to print something here, you might as well include the type
that seems invalid.  If you use %pR, I think it will do that automatically.

+			return -EINVAL;
+		}
+	}
+	xgene_pcie_setup_cfg_reg(port->csr_base, port->cfg_addr);
+
+	return 0;
+}

Bjorn