[PATCH v7 4/5] PCI: add PCI controller for keystone PCIe h/w

On Mon, Jul 21, 2014 at 12:58:44PM -0400, Murali Karicheri wrote:

keystone PCIe controller is based on v3.65 version of the
designware h/w. Main differences are
	1. No ATU support
	2. Legacy and MSI irq functions are implemented in
	   application register space
	3. MSI interrupts are multiplexed over 8 IRQ lines to the Host
	   side.
All of the Application register space handing code are organized into
pci-keystone-dw.c and the functions are called from pci-keystone.c
to implement PCI controller driver. Also add necessary DT documentation
for the driver.

Signed-off-by: Murali Karicheri <redacted>
Acked-by: Santosh Shilimkar <redacted>
...

+Note for PCI driver usage
+=========================
+Driver requires pci=pcie_bus_perf in the bootargs for proper functioning.

Whoa, why is this?  Special boot args should not be required.

It'd be nice to have some help text here.  I know, not everybody else does.

Blank line here (before the block comment).

+	/*
+	 * MSI0, Status bit 0-3 shows vectors 0, 8, 16, 24, MSI1 status bit
+	 * shows 1, 9, 17, 25 and so forth
+	 */
+	for (src = 0; src < 4; src++) {
+		if (BIT(src) & pending) {
+			vector = offset + (src << 3);
+			virq = irq_linear_revmap(pp->irq_domain, vector);
+			dev_dbg(pp->dev,
+				"irq: bit %d, vector %d, virq %d\n",
+				 src, vector, virq);
+			generic_handle_irq(virq);
+		}
+	}
+}
+
...

+static void __iomem *ks_pcie_cfg_setup(struct keystone_pcie *ks_pcie, u8 bus,
+					unsigned int devfn)
+{
+	u8 device = PCI_SLOT(devfn), function = PCI_FUNC(devfn);
+	struct pcie_port *pp = &ks_pcie->pp;
+	u32 regval;
+
+	if (bus == 0)
+		return pp->dbi_base;
+
+	regval = (bus << 16) | (device << 8) | function;
+	/*
+	 * Since Bus#1 will be a virtual bus, we need to have TYPE0
+	 * access only.
+	 * TYPE 1
+	 */
+	if (bus != 1)
+		regval |= BIT(24);
+
+	writel(regval, ks_pcie->va_app_base + CFG_SETUP);
+	return pp->va_cfg0_base;
+}
+
+int ks_dw_pcie_rd_other_conf(struct pcie_port *pp, struct pci_bus *bus,
+		unsigned int devfn, int where, int size, u32 *val)
+{
+	struct keystone_pcie *ks_pcie = to_keystone_pcie(pp);
+	u8 bus_num = bus->number;
+	void __iomem *addr;
+	int ret;
+
+	addr = ks_pcie_cfg_setup(ks_pcie, bus_num, devfn);
+	ret = dw_pcie_cfg_read(addr + (where & ~0x3), where, size, val);

This *looks* like it needs a lock to protect against concurrent
ks_pcie_cfg_setup() users, since it writes a register.

+
+	return ret;

Please use the same style as in ks_dw_pcie_wr_other_conf(), i.e., get rid
of "ret".

+}
+
+int ks_dw_pcie_wr_other_conf(struct pcie_port *pp,
+		struct pci_bus *bus, unsigned int devfn, int where,
+		int size, u32 val)
+{
+	struct keystone_pcie *ks_pcie = to_keystone_pcie(pp);
+	u8 bus_num = bus->number;
+	void __iomem *addr;
+
+	addr = ks_pcie_cfg_setup(ks_pcie, bus_num, devfn);
+
+	return dw_pcie_cfg_write(addr + (where & ~0x3), where, size, val);
+}

+static struct platform_driver ks_pcie_driver __refdata = {

Why does this need to be __refdata?  There are no other occurrences in
drivers/pci.

+	.probe  = ks_pcie_probe,
+	.remove = __exit_p(ks_pcie_remove),
+	.driver = {
+		.name	= "keystone-pcie",
+		.owner	= THIS_MODULE,
+		.of_match_table = of_match_ptr(ks_pcie_of_match),
+	},
+};

Bjorn