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

From: Murali Karicheri
Date: Tue Jul 22 2014 - 18:53:37 EST

Bjorn,

On 07/22/2014 06:35 PM, Bjorn Helgaas wrote:
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<m-karicheri2@xxxxxx>
Acked-by: Santosh Shilimkar<santosh.shilimkar@xxxxxx>
...

+++ b/Documentation/devicetree/bindings/pci/pci-keystone.txt
...

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

This was discussed initially and I had added following commit to get this working instead of a PCI quirk. To get some background please see the thread for commit below that you also had signed off as well.

commit 8b5742ad156d30ee38486652cdbd152e2d6ebbcc
Author: Murali Karicheri <m-karicheri2@xxxxxx>
Date: Wed May 28 13:14:53 2014 -0400

ARM/PCI: Call pcie_bus_configure_settings() to set MPS

Call pcie_bus_configure_settings() on ARM, like for other platforms.
pcie_bus_configure_settings() makes sure the MPS across the bus is uniform
and provides the ability to tune the MRSS and MPS to higher performance
values. This is particularly important for embedded where there is no
firmware to program these PCIe settings for the OS.

Signed-off-by: Murali Karicheri <m-karicheri2@xxxxxx>
Signed-off-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>
CC: Russell King <linux@xxxxxxxxxxxxxxxx>
CC: Arnd Bergmann <arnd@xxxxxxxx>
CC: Jason Gunthorpe <jgunthorpe@xxxxxxxxxxxxxxxxxxxx>
CC: Santosh Shilimkar <santosh.shilimkar@xxxxxx>

This was added as a preparatory patch to support keystone and
avoid a PCI quirk to do the same. Keystone has MRSS limitation
of 256 bytes. So adding a bootargs flag was suggested a better option than a PCI quirk.

I will look into the rest of the comments and possibly try to address them or discuss.

BTW, please apply patch 1-3 that has already got ack from maintainers
and is indepdent of this patch.

Thanks

Murali


diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 21df477..f8bc475 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -46,4 +46,9 @@ config PCI_HOST_GENERIC
Say Y here if you want to support a simple generic PCI host
controller, such as the one emulated by kvmtool.

+config PCI_KEYSTONE
+ bool "TI Keystone PCIe controller"
+ depends on ARCH_KEYSTONE
+ select PCIE_DW
+ select PCIEPORTBUS

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

+++ b/drivers/pci/host/pci-keystone-dw.c
...
+void ks_dw_pcie_handle_msi_irq(struct keystone_pcie *ks_pcie , int offset)
+{
+ struct pcie_port *pp =&ks_pcie->pp;
+ u32 pending, vector;
+ int src, virq;
+
+ pending = readl(ks_pcie->va_app_base + MSI0_IRQ_STATUS + (offset<< 4));

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);
+}

+++ b/drivers/pci/host/pci-keystone.c
...

+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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/