* [PATCH v2 1/5] arm: add msi.h to Kbuild
2015-07-31 10:15 [PATCH v2 0/5] Altera PCIe host controller driver with MSI support Ley Foon Tan
@ 2015-07-31 10:15 ` Ley Foon Tan
2015-07-31 10:15 ` [PATCH v2 2/5] pci:host: Add Altera PCIe host controller driver Ley Foon Tan
` (3 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Ley Foon Tan @ 2015-07-31 10:15 UTC (permalink / raw)
To: Bjorn Helgaas, Russell King, Arnd Bergmann, Dinh Nguyen
Cc: linux-pci, devicetree, linux-arm-kernel, linux-doc, linux-kernel,
Ley Foon Tan, lftan.linux, Rob Herring
Include asm-generic/msi.h to support CONFIG_GENERIC_MSI_IRQ_DOMAIN.
This to fix compilation error:
"include/linux/msi.h:123:21: fatal error: asm/msi.h:
No such file or directory"
Signed-off-by: Ley Foon Tan <lftan@altera.com>
---
arch/arm/include/asm/Kbuild | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arm/include/asm/Kbuild b/arch/arm/include/asm/Kbuild
index 83c5019..ddc2761 100644
--- a/arch/arm/include/asm/Kbuild
+++ b/arch/arm/include/asm/Kbuild
@@ -14,6 +14,7 @@ generic-y += local.h
generic-y += local64.h
generic-y += mcs_spinlock.h
generic-y += msgbuf.h
+generic-y += msi.h
generic-y += param.h
generic-y += parport.h
generic-y += poll.h
--
1.8.2.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 2/5] pci:host: Add Altera PCIe host controller driver
2015-07-31 10:15 [PATCH v2 0/5] Altera PCIe host controller driver with MSI support Ley Foon Tan
2015-07-31 10:15 ` [PATCH v2 1/5] arm: add msi.h to Kbuild Ley Foon Tan
@ 2015-07-31 10:15 ` Ley Foon Tan
2015-07-31 12:34 ` Marc Zyngier
2015-07-31 10:15 ` [PATCH v2 3/5] pci: altera: Add Altera PCIe MSI driver Ley Foon Tan
` (2 subsequent siblings)
4 siblings, 1 reply; 12+ messages in thread
From: Ley Foon Tan @ 2015-07-31 10:15 UTC (permalink / raw)
To: Bjorn Helgaas, Russell King, Arnd Bergmann, Dinh Nguyen
Cc: linux-pci, devicetree, linux-arm-kernel, linux-doc, linux-kernel,
Ley Foon Tan, lftan.linux, Rob Herring
This patch adds the Altera PCIe host controller driver.
Signed-off-by: Ley Foon Tan <lftan@altera.com>
---
drivers/pci/host/Kconfig | 8 +
drivers/pci/host/Makefile | 1 +
drivers/pci/host/pcie-altera.c | 526 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 535 insertions(+)
create mode 100644 drivers/pci/host/pcie-altera.c
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 675c2d1..108500a 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -145,4 +145,12 @@ config PCIE_IPROC_BCMA
Say Y here if you want to use the Broadcom iProc PCIe controller
through the BCMA bus interface
+config PCIE_ALTERA
+ tristate "Altera PCIe controller"
+ depends on ARCH_SOCFPGA
+ select PCI_MSI_IRQ_DOMAIN if PCI_MSI
+ help
+ Say Y here if you want to enable PCIe controller support for Altera
+ SoCFPGA family of SoCs.
+
endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 140d66f..6954f76 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -17,3 +17,4 @@ obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
obj-$(CONFIG_PCIE_IPROC_PLATFORM) += pcie-iproc-platform.o
obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
+obj-$(CONFIG_PCIE_ALTERA) += pcie-altera.o
diff --git a/drivers/pci/host/pcie-altera.c b/drivers/pci/host/pcie-altera.c
new file mode 100644
index 0000000..3ec51c7
--- /dev/null
+++ b/drivers/pci/host/pcie-altera.c
@@ -0,0 +1,526 @@
+/*
+ * Copyright Altera Corporation (C) 2013-2015. All rights reserved
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_pci.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define A2P_ADDR_MAP_LO0 0x1000
+#define A2P_ADDR_MAP_HI0 0x1004
+#define RP_TX_REG0 0x2000
+#define RP_TX_REG1 0x2004
+#define RP_TX_CNTRL 0x2008
+#define RP_TX_EOP 0x2
+#define RP_TX_SOP 0x1
+#define RP_RXCPL_STATUS 0x2010
+#define RP_RXCPL_EOP 0x2
+#define RP_RXCPL_SOP 0x1
+#define RP_RXCPL_REG0 0x2014
+#define RP_RXCPL_REG1 0x2018
+#define P2A_INT_STATUS 0x3060
+#define P2A_INT_STS_ALL 0xF
+#define P2A_INT_ENABLE 0x3070
+#define P2A_INT_ENA_ALL 0xF
+#define RP_LTSSM 0x3C64
+#define LTSSM_L0 0xF
+
+/* TLP configuration type 0 and 1 */
+#define TLP_FMTTYPE_CFGRD0 0x04 /* Configuration Read Type 0 */
+#define TLP_FMTTYPE_CFGWR0 0x44 /* Configuration Write Type 0 */
+#define TLP_FMTTYPE_CFGRD1 0x05 /* Configuration Read Type 1 */
+#define TLP_FMTTYPE_CFGWR1 0x45 /* Configuration Write Type 1 */
+#define TLP_PAYLOAD_SIZE 0x01
+#define TLP_READ_TAG 0x1D
+#define TLP_WRITE_TAG 0x10
+#define TLP_CFG_DW0(fmttype) (((fmttype) << 24) | TLP_PAYLOAD_SIZE)
+#define TLP_CFG_DW1(reqid, tag) (((reqid) << 16) | (tag << 8) | 0xF)
+#define TLP_CFG_DW2(bus, devfn, offset) \
+ (((bus) << 24) | ((devfn) << 16) | (offset))
+#define TLP_REQ_ID(bus, devfn) (((bus) << 8) | (devfn))
+#define TLP_COMPL_STATUS(hdr) (((hdr) & 0xE0) >> 13)
+#define TLP_HDR_SIZE 3
+#define TLP_LOOP 10
+
+#define INTX_NUM 4
+
+/* Address translation table entry size */
+#define ATT_ENTRY_SIZE 8
+
+#define DWORD_MASK 3
+
+struct altera_pcie {
+ struct platform_device *pdev;
+ struct resource *txs;
+ void __iomem *cra_base;
+ int hwirq;
+ u8 root_bus_nr;
+ struct irq_domain *irq_domain;
+ struct resource bus_range;
+ struct list_head resources;
+};
+
+struct tlp_rp_regpair_t {
+ u32 ctrl;
+ u32 reg0;
+ u32 reg1;
+};
+
+static void altera_pcie_retrain(struct pci_dev *dev)
+{
+ u16 linkcap, linkstat;
+
+ /*
+ * Set the retrain bit if the PCIe rootport support > 2.5GB/s, but
+ * current speed is 2.5 GB/s.
+ */
+ pcie_capability_read_word(dev, PCI_EXP_LNKCAP, &linkcap);
+
+ if ((linkcap & PCI_EXP_LNKCAP_SLS) <= PCI_EXP_LNKCAP_SLS_2_5GB)
+ return;
+
+ pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &linkstat);
+ if ((linkstat & PCI_EXP_LNKSTA_CLS) == PCI_EXP_LNKSTA_CLS_2_5GB)
+ pcie_capability_set_word(dev, PCI_EXP_LNKCTL,
+ PCI_EXP_LNKCTL_RL);
+
+}
+DECLARE_PCI_FIXUP_EARLY(PCI_ANY_ID, PCI_ANY_ID, altera_pcie_retrain);
+
+static inline void cra_writel(struct altera_pcie *pcie, u32 value, u32 reg)
+{
+ writel_relaxed(value, pcie->cra_base + reg);
+}
+
+static inline u32 cra_readl(struct altera_pcie *pcie, u32 reg)
+{
+ return readl_relaxed(pcie->cra_base + reg);
+}
+
+static void tlp_read_rx(struct altera_pcie *pcie,
+ struct tlp_rp_regpair_t *tlp_rp_regdata)
+{
+ tlp_rp_regdata->ctrl = cra_readl(pcie, RP_RXCPL_STATUS);
+ tlp_rp_regdata->reg0 = cra_readl(pcie, RP_RXCPL_REG0);
+ tlp_rp_regdata->reg1 = cra_readl(pcie, RP_RXCPL_REG1);
+}
+
+static void tlp_write_tx(struct altera_pcie *pcie,
+ struct tlp_rp_regpair_t *tlp_rp_regdata)
+{
+ cra_writel(pcie, tlp_rp_regdata->reg0, RP_TX_REG0);
+ cra_writel(pcie, tlp_rp_regdata->reg1, RP_TX_REG1);
+ cra_writel(pcie, tlp_rp_regdata->ctrl, RP_TX_CNTRL);
+}
+
+static bool altera_pcie_link_is_up(struct altera_pcie *pcie)
+{
+ return !!(cra_readl(pcie, RP_LTSSM) & LTSSM_L0);
+}
+
+static bool altera_pcie_valid_config(struct altera_pcie *pcie,
+ struct pci_bus *bus, int dev)
+{
+ /* If there is no link, then there is no device */
+ if (bus->number != pcie->root_bus_nr) {
+ if (!altera_pcie_link_is_up(pcie))
+ return false;
+ }
+
+ /* access only one slot on each root port */
+ if (bus->number == pcie->root_bus_nr && dev > 0)
+ return false;
+
+ /*
+ * Do not read more than one device on the bus directly attached
+ * to RC.
+ */
+ if (bus->primary == pcie->root_bus_nr && dev > 0)
+ return false;
+
+ return true;
+}
+
+static int tlp_read_packet(struct altera_pcie *pcie, u32 *value)
+{
+ u8 loop;
+ struct tlp_rp_regpair_t tlp_rp_regdata;
+
+ for (loop = TLP_LOOP; loop > 0; loop--) {
+ tlp_read_rx(pcie, &tlp_rp_regdata);
+ if (tlp_rp_regdata.ctrl & RP_RXCPL_EOP) {
+ if (value)
+ *value = tlp_rp_regdata.reg0;
+ return PCIBIOS_SUCCESSFUL;
+ }
+ }
+
+ return -ENOENT;
+}
+
+static void tlp_write_packet(struct altera_pcie *pcie, u32 *headers, u32 data)
+{
+ struct tlp_rp_regpair_t tlp_rp_regdata;
+
+ tlp_rp_regdata.reg0 = headers[0];
+ tlp_rp_regdata.reg1 = headers[1];
+ tlp_rp_regdata.ctrl = RP_TX_SOP;
+ tlp_write_tx(pcie, &tlp_rp_regdata);
+
+ tlp_rp_regdata.reg0 = headers[2];
+ tlp_rp_regdata.reg1 = data;
+ tlp_rp_regdata.ctrl = RP_TX_EOP;
+ tlp_write_tx(pcie, &tlp_rp_regdata);
+}
+
+static int tlp_cfg_dword_read(struct altera_pcie *pcie, u8 bus, u32 devfn,
+ int where, u32 *value)
+{
+ int ret;
+ u32 headers[TLP_HDR_SIZE];
+
+ if (bus == pcie->root_bus_nr)
+ headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGRD0);
+ else
+ headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGRD1);
+
+ headers[1] = TLP_CFG_DW1(TLP_REQ_ID(pcie->root_bus_nr, devfn),
+ TLP_READ_TAG);
+ headers[2] = TLP_CFG_DW2(bus, devfn, where);
+
+ tlp_write_packet(pcie, headers, 0);
+
+ ret = tlp_read_packet(pcie, value);
+ if (ret)
+ *value = ~0UL; /* return 0xFFFFFFFF if error */
+
+ return ret;
+}
+
+static int tlp_cfg_dword_write(struct altera_pcie *pcie, u8 bus, u32 devfn,
+ int where, u32 value)
+{
+ u32 headers[TLP_HDR_SIZE];
+
+ if (bus == pcie->root_bus_nr)
+ headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGWR0);
+ else
+ headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGWR1);
+
+ headers[1] = TLP_CFG_DW1(TLP_REQ_ID(pcie->root_bus_nr, devfn),
+ TLP_WRITE_TAG);
+ headers[2] = TLP_CFG_DW2(bus, devfn, where);
+
+ tlp_write_packet(pcie, headers, value);
+
+ tlp_read_packet(pcie, NULL);
+
+ /* Keep an eye out for changes to the root bus number */
+ if ((bus == pcie->root_bus_nr) && (where == PCI_PRIMARY_BUS))
+ pcie->root_bus_nr = (u8)(value);
+
+ return PCIBIOS_SUCCESSFUL;
+}
+
+static int altera_pcie_cfg_read(struct pci_bus *bus, unsigned int devfn,
+ int where, int size, u32 *value)
+{
+ struct altera_pcie *pcie = bus->sysdata;
+ int ret;
+
+ if (!altera_pcie_valid_config(pcie, bus, PCI_SLOT(devfn))) {
+ *value = ~0UL;
+ return PCIBIOS_DEVICE_NOT_FOUND;
+ }
+
+ ret = tlp_cfg_dword_read(pcie, bus->number, devfn,
+ (where & ~DWORD_MASK), value);
+ if (ret != PCIBIOS_SUCCESSFUL)
+ return ret;
+
+ if (size == 1)
+ *value = (*value >> (8 * (where & 0x3))) & 0xff;
+ else if (size == 2)
+ *value = (*value >> (8 * (where & 0x2))) & 0xffff;
+
+ return PCIBIOS_SUCCESSFUL;
+}
+
+static int altera_pcie_cfg_write(struct pci_bus *bus, unsigned int devfn,
+ int where, int size, u32 value)
+{
+ struct altera_pcie *pcie = bus->sysdata;
+ u32 data32 = value;
+ u32 shift = 8 * (where & 3);
+ int ret;
+
+ if (!altera_pcie_valid_config(pcie, bus, PCI_SLOT(devfn)))
+ return PCIBIOS_DEVICE_NOT_FOUND;
+
+ /* write partial */
+ if (size != sizeof(u32)) {
+ ret = tlp_cfg_dword_read(pcie, bus->number, devfn,
+ where & ~DWORD_MASK, &data32);
+ if (ret)
+ return ret;
+
+ if (size == 2)
+ data32 = (data32 & ~(0xffff << shift)) |
+ ((value & 0xffff) << shift);
+ else if (size == 1)
+ data32 = (data32 & ~(0xff << shift)) |
+ ((value & 0xff) << shift);
+ }
+
+ return tlp_cfg_dword_write(pcie, bus->number, devfn,
+ (where & ~DWORD_MASK), data32);
+}
+
+static struct pci_ops altera_pcie_ops = {
+ .read = altera_pcie_cfg_read,
+ .write = altera_pcie_cfg_write,
+};
+
+static int altera_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
+ irq_hw_number_t hwirq)
+{
+ irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
+ irq_set_chip_data(irq, domain->host_data);
+ set_irq_flags(irq, IRQF_VALID);
+
+ return 0;
+}
+
+static const struct irq_domain_ops intx_domain_ops = {
+ .map = altera_pcie_intx_map,
+};
+
+static irqreturn_t altera_pcie_isr(int irq, void *arg)
+{
+ struct altera_pcie *pcie = arg;
+ u32 status, i;
+
+ status = cra_readl(pcie, P2A_INT_STATUS) & P2A_INT_STS_ALL;
+
+ /* clear interrupts */
+ cra_writel(pcie, status, P2A_INT_STATUS);
+
+ for (i = 0; i < INTX_NUM; i++) {
+ if (status & (1 << i))
+ generic_handle_irq(irq_find_mapping(pcie->irq_domain,
+ i + 1));
+ }
+
+ return IRQ_HANDLED;
+}
+
+static void altera_pcie_release_of_pci_ranges(struct altera_pcie *pcie)
+{
+ pci_free_resource_list(&pcie->resources);
+}
+
+static int altera_pcie_parse_request_of_pci_ranges(struct altera_pcie *pcie)
+{
+ int err, res_valid = 0;
+ struct device *dev = &pcie->pdev->dev;
+ struct device_node *np = dev->of_node;
+ resource_size_t iobase;
+ struct resource_entry *win;
+ int offset = 0;
+
+ err = of_pci_get_host_bridge_resources(np, 0, 0xff, &pcie->resources,
+ &iobase);
+ if (err)
+ return err;
+
+ resource_list_for_each_entry(win, &pcie->resources) {
+ struct resource *parent, *res = win->res;
+
+ switch (resource_type(res)) {
+ case IORESOURCE_MEM:
+ parent = &iomem_resource;
+ res_valid |= !(res->flags & IORESOURCE_PREFETCH);
+ cra_writel(pcie, res->start,
+ A2P_ADDR_MAP_LO0 + offset);
+ cra_writel(pcie, 0,
+ A2P_ADDR_MAP_HI0 + offset);
+ offset += ATT_ENTRY_SIZE;
+ break;
+ default:
+ continue;
+ }
+
+ err = devm_request_resource(dev, parent, res);
+ if (err)
+ goto out_release_res;
+ }
+
+ if (!res_valid) {
+ dev_err(dev, "non-prefetchable memory resource required\n");
+ err = -EINVAL;
+ goto out_release_res;
+ }
+
+ return 0;
+
+out_release_res:
+ altera_pcie_release_of_pci_ranges(pcie);
+ return err;
+}
+
+static void altera_pcie_free_irq_domain(struct altera_pcie *pcie)
+{
+ int i;
+ u32 irq;
+
+ for (i = 0; i < INTX_NUM; i++) {
+ irq = irq_find_mapping(pcie->irq_domain, i);
+ if (irq > 0)
+ irq_dispose_mapping(irq);
+ }
+
+ irq_domain_remove(pcie->irq_domain);
+}
+
+static int altera_pcie_init_irq_domain(struct altera_pcie *pcie)
+{
+ struct device *dev = &pcie->pdev->dev;
+ struct device_node *node = dev->of_node;
+
+ /* Setup INTx */
+ pcie->irq_domain = irq_domain_add_linear(node, INTX_NUM,
+ &intx_domain_ops, pcie);
+ if (!pcie->irq_domain) {
+ dev_err(dev, "Failed to get a INTx IRQ domain\n");
+ return PTR_ERR(pcie->irq_domain);
+ }
+
+ return 0;
+}
+
+static int altera_pcie_parse_dt(struct altera_pcie *pcie)
+{
+ struct resource *cra;
+ int ret;
+ struct platform_device *pdev = pcie->pdev;
+
+ cra = platform_get_resource_byname(pdev, IORESOURCE_MEM, "Cra");
+ pcie->cra_base = devm_ioremap_resource(&pdev->dev, cra);
+ if (IS_ERR(pcie->cra_base)) {
+ dev_err(&pdev->dev, "get Cra resource failed\n");
+ return PTR_ERR(pcie->cra_base);
+ }
+
+ /* setup IRQ */
+ pcie->hwirq = platform_get_irq(pdev, 0);
+ if (pcie->hwirq <= 0) {
+ dev_err(&pdev->dev, "failed to get IRQ: %d\n", pcie->hwirq);
+ return -EINVAL;
+ }
+ ret = devm_request_irq(&pdev->dev, pcie->hwirq, altera_pcie_isr,
+ IRQF_SHARED, pdev->name, pcie);
+
+ if (ret) {
+ dev_err(&pdev->dev, "failed to request irq %d\n", pcie->hwirq);
+ return ret;
+ }
+
+ return 0;
+}
+
+static int altera_pcie_probe(struct platform_device *pdev)
+{
+ struct altera_pcie *pcie;
+ struct pci_bus *bus;
+ int ret;
+
+ pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
+ if (!pcie)
+ return -ENOMEM;
+
+ pcie->pdev = pdev;
+
+ ret = altera_pcie_parse_dt(pcie);
+ if (ret) {
+ dev_err(&pdev->dev, "Parsing DT failed\n");
+ return ret;
+ }
+
+ INIT_LIST_HEAD(&pcie->resources);
+
+ ret = altera_pcie_parse_request_of_pci_ranges(pcie);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed add resources\n");
+ return ret;
+ }
+
+ ret = altera_pcie_init_irq_domain(pcie);
+ if (ret) {
+ dev_err(&pdev->dev, "Failed creating IRQ Domain\n");
+ return ret;
+ }
+
+ pcie->root_bus_nr = 0;
+
+ /* clear all interrupts */
+ cra_writel(pcie, P2A_INT_STS_ALL, P2A_INT_STATUS);
+ /* enable all interrupts */
+ cra_writel(pcie, P2A_INT_ENA_ALL, P2A_INT_ENABLE);
+
+ bus = pci_scan_root_bus(&pdev->dev, pcie->root_bus_nr, &altera_pcie_ops,
+ pcie, &pcie->resources);
+ if (!bus)
+ return -ENOMEM;
+
+ pci_assign_unassigned_bus_resources(bus);
+ pci_bus_add_devices(bus);
+
+ platform_set_drvdata(pdev, pcie);
+ return ret;
+}
+
+static int __exit altera_pcie_remove(struct platform_device *pdev)
+{
+ struct altera_pcie *pcie = platform_get_drvdata(pdev);
+
+ altera_pcie_free_irq_domain(pcie);
+ platform_set_drvdata(pdev, NULL);
+ return 0;
+}
+
+static const struct of_device_id altera_pcie_of_match[] = {
+ { .compatible = "altr,pcie-root-port-1.0", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, altera_pcie_of_match);
+
+static struct platform_driver altera_pcie_driver = {
+ .probe = altera_pcie_probe,
+ .remove = altera_pcie_remove,
+ .driver = {
+ .name = "altera-pcie",
+ .of_match_table = altera_pcie_of_match,
+ },
+};
+
+module_platform_driver(altera_pcie_driver);
+
+MODULE_AUTHOR("Ley Foon Tan <lftan@altera.com>");
+MODULE_DESCRIPTION("Altera PCIe host controller driver");
+MODULE_LICENSE("GPL v2");
--
1.8.2.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 2/5] pci:host: Add Altera PCIe host controller driver
2015-07-31 10:15 ` [PATCH v2 2/5] pci:host: Add Altera PCIe host controller driver Ley Foon Tan
@ 2015-07-31 12:34 ` Marc Zyngier
2015-08-03 10:34 ` Ley Foon Tan
0 siblings, 1 reply; 12+ messages in thread
From: Marc Zyngier @ 2015-07-31 12:34 UTC (permalink / raw)
To: Ley Foon Tan, Bjorn Helgaas, Russell King, Arnd Bergmann,
Dinh Nguyen
Cc: devicetree, lftan.linux, linux-doc, linux-pci, linux-kernel,
Rob Herring, linux-arm-kernel
On 31/07/15 11:15, Ley Foon Tan wrote:
> This patch adds the Altera PCIe host controller driver.
>
> Signed-off-by: Ley Foon Tan <lftan@altera.com>
> ---
> drivers/pci/host/Kconfig | 8 +
> drivers/pci/host/Makefile | 1 +
> drivers/pci/host/pcie-altera.c | 526 +++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 535 insertions(+)
> create mode 100644 drivers/pci/host/pcie-altera.c
>
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index 675c2d1..108500a 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -145,4 +145,12 @@ config PCIE_IPROC_BCMA
> Say Y here if you want to use the Broadcom iProc PCIe controller
> through the BCMA bus interface
>
> +config PCIE_ALTERA
> + tristate "Altera PCIe controller"
> + depends on ARCH_SOCFPGA
> + select PCI_MSI_IRQ_DOMAIN if PCI_MSI
Why would you select this here? MSI support is in another driver, so I
don't see the point.
> + help
> + Say Y here if you want to enable PCIe controller support for Altera
> + SoCFPGA family of SoCs.
> +
> endmenu
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 140d66f..6954f76 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -17,3 +17,4 @@ obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
> obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
> obj-$(CONFIG_PCIE_IPROC_PLATFORM) += pcie-iproc-platform.o
> obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
> +obj-$(CONFIG_PCIE_ALTERA) += pcie-altera.o
> diff --git a/drivers/pci/host/pcie-altera.c b/drivers/pci/host/pcie-altera.c
> new file mode 100644
> index 0000000..3ec51c7
> --- /dev/null
> +++ b/drivers/pci/host/pcie-altera.c
> @@ -0,0 +1,526 @@
> +/*
> + * Copyright Altera Corporation (C) 2013-2015. All rights reserved
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_pci.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#define A2P_ADDR_MAP_LO0 0x1000
> +#define A2P_ADDR_MAP_HI0 0x1004
> +#define RP_TX_REG0 0x2000
> +#define RP_TX_REG1 0x2004
> +#define RP_TX_CNTRL 0x2008
> +#define RP_TX_EOP 0x2
> +#define RP_TX_SOP 0x1
> +#define RP_RXCPL_STATUS 0x2010
> +#define RP_RXCPL_EOP 0x2
> +#define RP_RXCPL_SOP 0x1
> +#define RP_RXCPL_REG0 0x2014
> +#define RP_RXCPL_REG1 0x2018
> +#define P2A_INT_STATUS 0x3060
> +#define P2A_INT_STS_ALL 0xF
> +#define P2A_INT_ENABLE 0x3070
> +#define P2A_INT_ENA_ALL 0xF
> +#define RP_LTSSM 0x3C64
> +#define LTSSM_L0 0xF
> +
> +/* TLP configuration type 0 and 1 */
> +#define TLP_FMTTYPE_CFGRD0 0x04 /* Configuration Read Type 0 */
> +#define TLP_FMTTYPE_CFGWR0 0x44 /* Configuration Write Type 0 */
> +#define TLP_FMTTYPE_CFGRD1 0x05 /* Configuration Read Type 1 */
> +#define TLP_FMTTYPE_CFGWR1 0x45 /* Configuration Write Type 1 */
> +#define TLP_PAYLOAD_SIZE 0x01
> +#define TLP_READ_TAG 0x1D
> +#define TLP_WRITE_TAG 0x10
> +#define TLP_CFG_DW0(fmttype) (((fmttype) << 24) | TLP_PAYLOAD_SIZE)
> +#define TLP_CFG_DW1(reqid, tag) (((reqid) << 16) | (tag << 8) | 0xF)
> +#define TLP_CFG_DW2(bus, devfn, offset) \
> + (((bus) << 24) | ((devfn) << 16) | (offset))
> +#define TLP_REQ_ID(bus, devfn) (((bus) << 8) | (devfn))
> +#define TLP_COMPL_STATUS(hdr) (((hdr) & 0xE0) >> 13)
> +#define TLP_HDR_SIZE 3
> +#define TLP_LOOP 10
> +
> +#define INTX_NUM 4
> +
> +/* Address translation table entry size */
> +#define ATT_ENTRY_SIZE 8
> +
> +#define DWORD_MASK 3
> +
> +struct altera_pcie {
> + struct platform_device *pdev;
> + struct resource *txs;
> + void __iomem *cra_base;
> + int hwirq;
> + u8 root_bus_nr;
> + struct irq_domain *irq_domain;
> + struct resource bus_range;
> + struct list_head resources;
> +};
> +
> +struct tlp_rp_regpair_t {
> + u32 ctrl;
> + u32 reg0;
> + u32 reg1;
> +};
> +
> +static void altera_pcie_retrain(struct pci_dev *dev)
> +{
> + u16 linkcap, linkstat;
> +
> + /*
> + * Set the retrain bit if the PCIe rootport support > 2.5GB/s, but
> + * current speed is 2.5 GB/s.
> + */
> + pcie_capability_read_word(dev, PCI_EXP_LNKCAP, &linkcap);
> +
> + if ((linkcap & PCI_EXP_LNKCAP_SLS) <= PCI_EXP_LNKCAP_SLS_2_5GB)
> + return;
> +
> + pcie_capability_read_word(dev, PCI_EXP_LNKSTA, &linkstat);
> + if ((linkstat & PCI_EXP_LNKSTA_CLS) == PCI_EXP_LNKSTA_CLS_2_5GB)
> + pcie_capability_set_word(dev, PCI_EXP_LNKCTL,
> + PCI_EXP_LNKCTL_RL);
> +
> +}
> +DECLARE_PCI_FIXUP_EARLY(PCI_ANY_ID, PCI_ANY_ID, altera_pcie_retrain);
> +
> +static inline void cra_writel(struct altera_pcie *pcie, u32 value, u32 reg)
> +{
> + writel_relaxed(value, pcie->cra_base + reg);
> +}
> +
> +static inline u32 cra_readl(struct altera_pcie *pcie, u32 reg)
> +{
> + return readl_relaxed(pcie->cra_base + reg);
> +}
> +
> +static void tlp_read_rx(struct altera_pcie *pcie,
> + struct tlp_rp_regpair_t *tlp_rp_regdata)
> +{
> + tlp_rp_regdata->ctrl = cra_readl(pcie, RP_RXCPL_STATUS);
> + tlp_rp_regdata->reg0 = cra_readl(pcie, RP_RXCPL_REG0);
> + tlp_rp_regdata->reg1 = cra_readl(pcie, RP_RXCPL_REG1);
> +}
> +
> +static void tlp_write_tx(struct altera_pcie *pcie,
> + struct tlp_rp_regpair_t *tlp_rp_regdata)
> +{
> + cra_writel(pcie, tlp_rp_regdata->reg0, RP_TX_REG0);
> + cra_writel(pcie, tlp_rp_regdata->reg1, RP_TX_REG1);
> + cra_writel(pcie, tlp_rp_regdata->ctrl, RP_TX_CNTRL);
> +}
> +
> +static bool altera_pcie_link_is_up(struct altera_pcie *pcie)
> +{
> + return !!(cra_readl(pcie, RP_LTSSM) & LTSSM_L0);
> +}
> +
> +static bool altera_pcie_valid_config(struct altera_pcie *pcie,
> + struct pci_bus *bus, int dev)
> +{
> + /* If there is no link, then there is no device */
> + if (bus->number != pcie->root_bus_nr) {
> + if (!altera_pcie_link_is_up(pcie))
> + return false;
> + }
> +
> + /* access only one slot on each root port */
> + if (bus->number == pcie->root_bus_nr && dev > 0)
> + return false;
> +
> + /*
> + * Do not read more than one device on the bus directly attached
> + * to RC.
> + */
> + if (bus->primary == pcie->root_bus_nr && dev > 0)
> + return false;
> +
> + return true;
> +}
> +
> +static int tlp_read_packet(struct altera_pcie *pcie, u32 *value)
> +{
> + u8 loop;
> + struct tlp_rp_regpair_t tlp_rp_regdata;
> +
> + for (loop = TLP_LOOP; loop > 0; loop--) {
> + tlp_read_rx(pcie, &tlp_rp_regdata);
> + if (tlp_rp_regdata.ctrl & RP_RXCPL_EOP) {
> + if (value)
> + *value = tlp_rp_regdata.reg0;
> + return PCIBIOS_SUCCESSFUL;
> + }
> + }
> +
> + return -ENOENT;
> +}
> +
> +static void tlp_write_packet(struct altera_pcie *pcie, u32 *headers, u32 data)
> +{
> + struct tlp_rp_regpair_t tlp_rp_regdata;
> +
> + tlp_rp_regdata.reg0 = headers[0];
> + tlp_rp_regdata.reg1 = headers[1];
> + tlp_rp_regdata.ctrl = RP_TX_SOP;
> + tlp_write_tx(pcie, &tlp_rp_regdata);
> +
> + tlp_rp_regdata.reg0 = headers[2];
> + tlp_rp_regdata.reg1 = data;
> + tlp_rp_regdata.ctrl = RP_TX_EOP;
> + tlp_write_tx(pcie, &tlp_rp_regdata);
> +}
> +
> +static int tlp_cfg_dword_read(struct altera_pcie *pcie, u8 bus, u32 devfn,
> + int where, u32 *value)
> +{
> + int ret;
> + u32 headers[TLP_HDR_SIZE];
> +
> + if (bus == pcie->root_bus_nr)
> + headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGRD0);
> + else
> + headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGRD1);
> +
> + headers[1] = TLP_CFG_DW1(TLP_REQ_ID(pcie->root_bus_nr, devfn),
> + TLP_READ_TAG);
> + headers[2] = TLP_CFG_DW2(bus, devfn, where);
> +
> + tlp_write_packet(pcie, headers, 0);
> +
> + ret = tlp_read_packet(pcie, value);
> + if (ret)
> + *value = ~0UL; /* return 0xFFFFFFFF if error */
> +
> + return ret;
> +}
> +
> +static int tlp_cfg_dword_write(struct altera_pcie *pcie, u8 bus, u32 devfn,
> + int where, u32 value)
> +{
> + u32 headers[TLP_HDR_SIZE];
> +
> + if (bus == pcie->root_bus_nr)
> + headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGWR0);
> + else
> + headers[0] = TLP_CFG_DW0(TLP_FMTTYPE_CFGWR1);
> +
> + headers[1] = TLP_CFG_DW1(TLP_REQ_ID(pcie->root_bus_nr, devfn),
> + TLP_WRITE_TAG);
> + headers[2] = TLP_CFG_DW2(bus, devfn, where);
> +
> + tlp_write_packet(pcie, headers, value);
> +
> + tlp_read_packet(pcie, NULL);
> +
> + /* Keep an eye out for changes to the root bus number */
> + if ((bus == pcie->root_bus_nr) && (where == PCI_PRIMARY_BUS))
> + pcie->root_bus_nr = (u8)(value);
> +
> + return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int altera_pcie_cfg_read(struct pci_bus *bus, unsigned int devfn,
> + int where, int size, u32 *value)
> +{
> + struct altera_pcie *pcie = bus->sysdata;
> + int ret;
> +
> + if (!altera_pcie_valid_config(pcie, bus, PCI_SLOT(devfn))) {
> + *value = ~0UL;
> + return PCIBIOS_DEVICE_NOT_FOUND;
> + }
> +
> + ret = tlp_cfg_dword_read(pcie, bus->number, devfn,
> + (where & ~DWORD_MASK), value);
> + if (ret != PCIBIOS_SUCCESSFUL)
> + return ret;
> +
> + if (size == 1)
> + *value = (*value >> (8 * (where & 0x3))) & 0xff;
> + else if (size == 2)
> + *value = (*value >> (8 * (where & 0x2))) & 0xffff;
> +
> + return PCIBIOS_SUCCESSFUL;
> +}
> +
> +static int altera_pcie_cfg_write(struct pci_bus *bus, unsigned int devfn,
> + int where, int size, u32 value)
> +{
> + struct altera_pcie *pcie = bus->sysdata;
> + u32 data32 = value;
> + u32 shift = 8 * (where & 3);
> + int ret;
> +
> + if (!altera_pcie_valid_config(pcie, bus, PCI_SLOT(devfn)))
> + return PCIBIOS_DEVICE_NOT_FOUND;
> +
> + /* write partial */
> + if (size != sizeof(u32)) {
> + ret = tlp_cfg_dword_read(pcie, bus->number, devfn,
> + where & ~DWORD_MASK, &data32);
> + if (ret)
> + return ret;
> +
> + if (size == 2)
> + data32 = (data32 & ~(0xffff << shift)) |
> + ((value & 0xffff) << shift);
> + else if (size == 1)
> + data32 = (data32 & ~(0xff << shift)) |
> + ((value & 0xff) << shift);
> + }
> +
> + return tlp_cfg_dword_write(pcie, bus->number, devfn,
> + (where & ~DWORD_MASK), data32);
> +}
> +
> +static struct pci_ops altera_pcie_ops = {
> + .read = altera_pcie_cfg_read,
> + .write = altera_pcie_cfg_write,
> +};
> +
> +static int altera_pcie_intx_map(struct irq_domain *domain, unsigned int irq,
> + irq_hw_number_t hwirq)
> +{
> + irq_set_chip_and_handler(irq, &dummy_irq_chip, handle_simple_irq);
> + irq_set_chip_data(irq, domain->host_data);
> + set_irq_flags(irq, IRQF_VALID);
> +
> + return 0;
> +}
> +
> +static const struct irq_domain_ops intx_domain_ops = {
> + .map = altera_pcie_intx_map,
> +};
> +
> +static irqreturn_t altera_pcie_isr(int irq, void *arg)
> +{
> + struct altera_pcie *pcie = arg;
> + u32 status, i;
> +
> + status = cra_readl(pcie, P2A_INT_STATUS) & P2A_INT_STS_ALL;
> +
> + /* clear interrupts */
> + cra_writel(pcie, status, P2A_INT_STATUS);
> +
> + for (i = 0; i < INTX_NUM; i++) {
> + if (status & (1 << i))
find_first/next_set_bit?
> + generic_handle_irq(irq_find_mapping(pcie->irq_domain,
> + i + 1));
Rule of thumb: if you're calling generic_handle_irq from an interrupt
handler, this is a chained IRQ controller. Please implement it as such.
> + }
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void altera_pcie_release_of_pci_ranges(struct altera_pcie *pcie)
> +{
> + pci_free_resource_list(&pcie->resources);
> +}
> +
> +static int altera_pcie_parse_request_of_pci_ranges(struct altera_pcie *pcie)
> +{
> + int err, res_valid = 0;
> + struct device *dev = &pcie->pdev->dev;
> + struct device_node *np = dev->of_node;
> + resource_size_t iobase;
> + struct resource_entry *win;
> + int offset = 0;
> +
> + err = of_pci_get_host_bridge_resources(np, 0, 0xff, &pcie->resources,
> + &iobase);
> + if (err)
> + return err;
> +
> + resource_list_for_each_entry(win, &pcie->resources) {
> + struct resource *parent, *res = win->res;
> +
> + switch (resource_type(res)) {
> + case IORESOURCE_MEM:
> + parent = &iomem_resource;
> + res_valid |= !(res->flags & IORESOURCE_PREFETCH);
> + cra_writel(pcie, res->start,
> + A2P_ADDR_MAP_LO0 + offset);
> + cra_writel(pcie, 0,
> + A2P_ADDR_MAP_HI0 + offset);
> + offset += ATT_ENTRY_SIZE;
> + break;
> + default:
> + continue;
> + }
> +
> + err = devm_request_resource(dev, parent, res);
> + if (err)
> + goto out_release_res;
> + }
> +
> + if (!res_valid) {
> + dev_err(dev, "non-prefetchable memory resource required\n");
> + err = -EINVAL;
> + goto out_release_res;
> + }
> +
> + return 0;
> +
> +out_release_res:
> + altera_pcie_release_of_pci_ranges(pcie);
> + return err;
> +}
> +
> +static void altera_pcie_free_irq_domain(struct altera_pcie *pcie)
> +{
> + int i;
> + u32 irq;
> +
> + for (i = 0; i < INTX_NUM; i++) {
> + irq = irq_find_mapping(pcie->irq_domain, i);
> + if (irq > 0)
> + irq_dispose_mapping(irq);
> + }
> +
> + irq_domain_remove(pcie->irq_domain);
> +}
> +
> +static int altera_pcie_init_irq_domain(struct altera_pcie *pcie)
> +{
> + struct device *dev = &pcie->pdev->dev;
> + struct device_node *node = dev->of_node;
> +
> + /* Setup INTx */
> + pcie->irq_domain = irq_domain_add_linear(node, INTX_NUM,
> + &intx_domain_ops, pcie);
> + if (!pcie->irq_domain) {
> + dev_err(dev, "Failed to get a INTx IRQ domain\n");
> + return PTR_ERR(pcie->irq_domain);
> + }
> +
> + return 0;
> +}
> +
> +static int altera_pcie_parse_dt(struct altera_pcie *pcie)
> +{
> + struct resource *cra;
> + int ret;
> + struct platform_device *pdev = pcie->pdev;
> +
> + cra = platform_get_resource_byname(pdev, IORESOURCE_MEM, "Cra");
> + pcie->cra_base = devm_ioremap_resource(&pdev->dev, cra);
> + if (IS_ERR(pcie->cra_base)) {
> + dev_err(&pdev->dev, "get Cra resource failed\n");
> + return PTR_ERR(pcie->cra_base);
> + }
> +
> + /* setup IRQ */
> + pcie->hwirq = platform_get_irq(pdev, 0);
> + if (pcie->hwirq <= 0) {
This is definitely not a hardware interrupt number. It is completely
virtual. I suggest you give it a better name.
> + dev_err(&pdev->dev, "failed to get IRQ: %d\n", pcie->hwirq);
> + return -EINVAL;
> + }
> + ret = devm_request_irq(&pdev->dev, pcie->hwirq, altera_pcie_isr,
> + IRQF_SHARED, pdev->name, pcie);
You're implementing a cascading interrupt controller again. Please fix
it just like you did for your MSI support.
> +
> + if (ret) {
> + dev_err(&pdev->dev, "failed to request irq %d\n", pcie->hwirq);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int altera_pcie_probe(struct platform_device *pdev)
> +{
> + struct altera_pcie *pcie;
> + struct pci_bus *bus;
> + int ret;
> +
> + pcie = devm_kzalloc(&pdev->dev, sizeof(*pcie), GFP_KERNEL);
> + if (!pcie)
> + return -ENOMEM;
> +
> + pcie->pdev = pdev;
> +
> + ret = altera_pcie_parse_dt(pcie);
> + if (ret) {
> + dev_err(&pdev->dev, "Parsing DT failed\n");
> + return ret;
> + }
> +
> + INIT_LIST_HEAD(&pcie->resources);
> +
> + ret = altera_pcie_parse_request_of_pci_ranges(pcie);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed add resources\n");
> + return ret;
> + }
> +
> + ret = altera_pcie_init_irq_domain(pcie);
> + if (ret) {
> + dev_err(&pdev->dev, "Failed creating IRQ Domain\n");
> + return ret;
> + }
> +
> + pcie->root_bus_nr = 0;
> +
> + /* clear all interrupts */
> + cra_writel(pcie, P2A_INT_STS_ALL, P2A_INT_STATUS);
> + /* enable all interrupts */
> + cra_writel(pcie, P2A_INT_ENA_ALL, P2A_INT_ENABLE);
> +
> + bus = pci_scan_root_bus(&pdev->dev, pcie->root_bus_nr, &altera_pcie_ops,
> + pcie, &pcie->resources);
> + if (!bus)
> + return -ENOMEM;
> +
> + pci_assign_unassigned_bus_resources(bus);
> + pci_bus_add_devices(bus);
> +
> + platform_set_drvdata(pdev, pcie);
> + return ret;
> +}
> +
> +static int __exit altera_pcie_remove(struct platform_device *pdev)
> +{
> + struct altera_pcie *pcie = platform_get_drvdata(pdev);
> +
> + altera_pcie_free_irq_domain(pcie);
> + platform_set_drvdata(pdev, NULL);
> + return 0;
> +}
> +
> +static const struct of_device_id altera_pcie_of_match[] = {
> + { .compatible = "altr,pcie-root-port-1.0", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, altera_pcie_of_match);
> +
> +static struct platform_driver altera_pcie_driver = {
> + .probe = altera_pcie_probe,
> + .remove = altera_pcie_remove,
> + .driver = {
> + .name = "altera-pcie",
> + .of_match_table = altera_pcie_of_match,
> + },
> +};
> +
> +module_platform_driver(altera_pcie_driver);
> +
> +MODULE_AUTHOR("Ley Foon Tan <lftan@altera.com>");
> +MODULE_DESCRIPTION("Altera PCIe host controller driver");
> +MODULE_LICENSE("GPL v2");
>
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 2/5] pci:host: Add Altera PCIe host controller driver
2015-07-31 12:34 ` Marc Zyngier
@ 2015-08-03 10:34 ` Ley Foon Tan
0 siblings, 0 replies; 12+ messages in thread
From: Ley Foon Tan @ 2015-08-03 10:34 UTC (permalink / raw)
To: Marc Zyngier
Cc: Bjorn Helgaas, Russell King, Arnd Bergmann, Dinh Nguyen,
devicetree, linux-doc@vger.kernel.org, linux-pci,
linux-kernel@vger.kernel.org, Rob Herring, linux-arm-kernel
On Fri, Jul 31, 2015 at 8:34 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 31/07/15 11:15, Ley Foon Tan wrote:
>> This patch adds the Altera PCIe host controller driver.
>>
>> Signed-off-by: Ley Foon Tan <lftan@altera.com>
>> ---
>> drivers/pci/host/Kconfig | 8 +
>> drivers/pci/host/Makefile | 1 +
>> drivers/pci/host/pcie-altera.c | 526 +++++++++++++++++++++++++++++++++++++++++
>> 3 files changed, 535 insertions(+)
>> create mode 100644 drivers/pci/host/pcie-altera.c
>>
>> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
>> index 675c2d1..108500a 100644
>> --- a/drivers/pci/host/Kconfig
>> +++ b/drivers/pci/host/Kconfig
>> @@ -145,4 +145,12 @@ config PCIE_IPROC_BCMA
>> Say Y here if you want to use the Broadcom iProc PCIe controller
>> through the BCMA bus interface
>>
>> +config PCIE_ALTERA
>> + tristate "Altera PCIe controller"
>> + depends on ARCH_SOCFPGA
>> + select PCI_MSI_IRQ_DOMAIN if PCI_MSI
>
> Why would you select this here? MSI support is in another driver, so I
> don't see the point.
Will move select PCI_MSI_IRQ_DOMAIN to config PCIE_ALTERA_MSI section.
>
>> + help
>> + Say Y here if you want to enable PCIe controller support for Altera
>> + SoCFPGA family of SoCs.
>> +
>> endmenu
>> +static const struct irq_domain_ops intx_domain_ops = {
>> + .map = altera_pcie_intx_map,
>> +};
>> +
>> +static irqreturn_t altera_pcie_isr(int irq, void *arg)
>> +{
>> + struct altera_pcie *pcie = arg;
>> + u32 status, i;
>> +
>> + status = cra_readl(pcie, P2A_INT_STATUS) & P2A_INT_STS_ALL;
>> +
>> + /* clear interrupts */
>> + cra_writel(pcie, status, P2A_INT_STATUS);
>> +
>> + for (i = 0; i < INTX_NUM; i++) {
>> + if (status & (1 << i))
>
> find_first/next_set_bit?
Yes, change this to find_first_bit().
>
>> + generic_handle_irq(irq_find_mapping(pcie->irq_domain,
>> + i + 1));
>
> Rule of thumb: if you're calling generic_handle_irq from an interrupt
> handler, this is a chained IRQ controller. Please implement it as such.
Okay, will change this to chained_irq_enter/chained_irq_exit implementation.
>
>> + }
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +static void altera_pcie_release_of_pci_ranges(struct altera_pcie *pcie)
>> +{
>> + pci_free_resource_list(&pcie->resources);
>> +}
>> +
>> +static int altera_pcie_parse_request_of_pci_ranges(struct altera_pcie *pcie)
>> +{
>> + int err, res_valid = 0;
>> + struct device *dev = &pcie->pdev->dev;
>> + struct device_node *np = dev->of_node;
>> + resource_size_t iobase;
>> + struct resource_entry *win;
>> + int offset = 0;
>> +
>> + err = of_pci_get_host_bridge_resources(np, 0, 0xff, &pcie->resources,
>> + &iobase);
>> + if (err)
>> + return err;
>> +
>> + resource_list_for_each_entry(win, &pcie->resources) {
>> + struct resource *parent, *res = win->res;
>> +
>> + switch (resource_type(res)) {
>> + case IORESOURCE_MEM:
>> + parent = &iomem_resource;
>> + res_valid |= !(res->flags & IORESOURCE_PREFETCH);
>> + cra_writel(pcie, res->start,
>> + A2P_ADDR_MAP_LO0 + offset);
>> + cra_writel(pcie, 0,
>> + A2P_ADDR_MAP_HI0 + offset);
>> + offset += ATT_ENTRY_SIZE;
>> + break;
>> + default:
>> + continue;
>> + }
>> +
>> + err = devm_request_resource(dev, parent, res);
>> + if (err)
>> + goto out_release_res;
>> + }
>> +
>> + if (!res_valid) {
>> + dev_err(dev, "non-prefetchable memory resource required\n");
>> + err = -EINVAL;
>> + goto out_release_res;
>> + }
>> +
>> + return 0;
>> +
>> +out_release_res:
>> + altera_pcie_release_of_pci_ranges(pcie);
>> + return err;
>> +}
>> +
>> +static void altera_pcie_free_irq_domain(struct altera_pcie *pcie)
>> +{
>> + int i;
>> + u32 irq;
>> +
>> + for (i = 0; i < INTX_NUM; i++) {
>> + irq = irq_find_mapping(pcie->irq_domain, i);
>> + if (irq > 0)
>> + irq_dispose_mapping(irq);
>> + }
>> +
>> + irq_domain_remove(pcie->irq_domain);
>> +}
>> +
>> +static int altera_pcie_init_irq_domain(struct altera_pcie *pcie)
>> +{
>> + struct device *dev = &pcie->pdev->dev;
>> + struct device_node *node = dev->of_node;
>> +
>> + /* Setup INTx */
>> + pcie->irq_domain = irq_domain_add_linear(node, INTX_NUM,
>> + &intx_domain_ops, pcie);
>> + if (!pcie->irq_domain) {
>> + dev_err(dev, "Failed to get a INTx IRQ domain\n");
>> + return PTR_ERR(pcie->irq_domain);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int altera_pcie_parse_dt(struct altera_pcie *pcie)
>> +{
>> + struct resource *cra;
>> + int ret;
>> + struct platform_device *pdev = pcie->pdev;
>> +
>> + cra = platform_get_resource_byname(pdev, IORESOURCE_MEM, "Cra");
>> + pcie->cra_base = devm_ioremap_resource(&pdev->dev, cra);
>> + if (IS_ERR(pcie->cra_base)) {
>> + dev_err(&pdev->dev, "get Cra resource failed\n");
>> + return PTR_ERR(pcie->cra_base);
>> + }
>> +
>> + /* setup IRQ */
>> + pcie->hwirq = platform_get_irq(pdev, 0);
>> + if (pcie->hwirq <= 0) {
>
> This is definitely not a hardware interrupt number. It is completely
> virtual. I suggest you give it a better name.
Noted.
>
>> + dev_err(&pdev->dev, "failed to get IRQ: %d\n", pcie->hwirq);
>> + return -EINVAL;
>> + }
>> + ret = devm_request_irq(&pdev->dev, pcie->hwirq, altera_pcie_isr,
>> + IRQF_SHARED, pdev->name, pcie);
>
> You're implementing a cascading interrupt controller again. Please fix
> it just like you did for your MSI support.
Sure, will change to irq_set_chained_handler_and_data here.
Thanks for reviewing.
Regards
Ley Foon
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 3/5] pci: altera: Add Altera PCIe MSI driver
2015-07-31 10:15 [PATCH v2 0/5] Altera PCIe host controller driver with MSI support Ley Foon Tan
2015-07-31 10:15 ` [PATCH v2 1/5] arm: add msi.h to Kbuild Ley Foon Tan
2015-07-31 10:15 ` [PATCH v2 2/5] pci:host: Add Altera PCIe host controller driver Ley Foon Tan
@ 2015-07-31 10:15 ` Ley Foon Tan
2015-07-31 12:12 ` Marc Zyngier
2015-07-31 10:15 ` [PATCH v2 4/5] Documentation: dt-bindings: pci: altera pcie device tree binding Ley Foon Tan
2015-07-31 10:15 ` [PATCH v2 5/5] MAINTAINERS: Add Altera PCIe and MSI drivers maintainer Ley Foon Tan
4 siblings, 1 reply; 12+ messages in thread
From: Ley Foon Tan @ 2015-07-31 10:15 UTC (permalink / raw)
To: Bjorn Helgaas, Russell King, Arnd Bergmann, Dinh Nguyen
Cc: linux-pci, devicetree, linux-arm-kernel, linux-doc, linux-kernel,
Ley Foon Tan, lftan.linux, Rob Herring
This patch adds Altera PCIe MSI driver. This soft IP supports configurable
number of vectors, which is a dts parameter.
Signed-off-by: Ley Foon Tan <lftan@altera.com>
---
drivers/pci/host/Kconfig | 7 +
drivers/pci/host/Makefile | 1 +
drivers/pci/host/pcie-altera-msi.c | 324 +++++++++++++++++++++++++++++++++++++
3 files changed, 332 insertions(+)
create mode 100644 drivers/pci/host/pcie-altera-msi.c
diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index 108500a..51d0468 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -153,4 +153,11 @@ config PCIE_ALTERA
Say Y here if you want to enable PCIe controller support for Altera
SoCFPGA family of SoCs.
+config PCIE_ALTERA_MSI
+ bool "Altera PCIe MSI feature"
+ depends on PCI_MSI
+ help
+ Say Y here if you want PCIe MSI support for the Altera SocFPGA SoC.
+ This MSI driver supports Altera MSI to GIC controller IP.
+
endmenu
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 6954f76..6c4913d 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -18,3 +18,4 @@ obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
obj-$(CONFIG_PCIE_IPROC_PLATFORM) += pcie-iproc-platform.o
obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
obj-$(CONFIG_PCIE_ALTERA) += pcie-altera.o
+obj-$(CONFIG_PCIE_ALTERA_MSI) += pcie-altera-msi.o
diff --git a/drivers/pci/host/pcie-altera-msi.c b/drivers/pci/host/pcie-altera-msi.c
new file mode 100644
index 0000000..6014719
--- /dev/null
+++ b/drivers/pci/host/pcie-altera-msi.c
@@ -0,0 +1,324 @@
+/*
+ * Copyright Altera Corporation (C) 2013-2015. All rights reserved
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+#include <linux/interrupt.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/module.h>
+#include <linux/msi.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_pci.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define MSI_STATUS 0x0
+#define MSI_ERROR 0x4
+#define MSI_INTMASK 0x8
+
+#define MAX_MSI_VECTORS 32
+struct altera_msi {
+ DECLARE_BITMAP(used, MAX_MSI_VECTORS);
+ struct mutex lock; /* proctect used variable */
+ struct platform_device *pdev;
+ struct irq_domain *msi_domain;
+ struct irq_domain *inner_domain;
+ void __iomem *csr_base;
+ void __iomem *vector_base;
+ phys_addr_t vector_phy;
+ u32 num_of_vectors;
+ int irq;
+};
+
+static inline void msi_writel(struct altera_msi *msi, u32 value, u32 reg)
+{
+ writel_relaxed(value, msi->csr_base + reg);
+}
+
+static inline u32 msi_readl(struct altera_msi *msi, u32 reg)
+{
+ return readl_relaxed(msi->csr_base + reg);
+}
+
+static void altera_msi_isr(unsigned int irq, struct irq_desc *desc)
+{
+ struct irq_chip *chip = irq_desc_get_chip(desc);
+ struct altera_msi *msi;
+ unsigned long status;
+ u32 num_of_vectors;
+ u32 bit;
+
+ chained_irq_enter(chip, desc);
+ msi = irq_desc_get_handler_data(desc);
+ num_of_vectors = msi->num_of_vectors;
+
+ do {
+ status = msi_readl(msi, MSI_STATUS);
+ if (!status)
+ break;
+
+ do {
+ bit = find_first_bit(&status, num_of_vectors);
+ /* Dummy read from vector to clear the interrupt */
+ readl_relaxed(msi->vector_base + (bit * sizeof(u32)));
+
+ irq = irq_find_mapping(msi->inner_domain, bit);
+ if (irq) {
+ if (test_bit(bit, msi->used))
+ generic_handle_irq(irq);
+ else
+ dev_info(&msi->pdev->dev, "unhandled MSI\n");
+ } else
+ dev_info(&msi->pdev->dev, "unexpected MSI\n");
+
+ /* Clear the bit from status and repeat without reading
+ * again status register. */
+ clear_bit(bit, &status);
+ } while (status);
+ } while (1);
+
+ chained_irq_exit(chip, desc);
+}
+
+static struct irq_chip altera_msi_irq_chip = {
+ .name = "Altera PCIe MSI",
+ .irq_enable = pci_msi_unmask_irq,
+ .irq_disable = pci_msi_mask_irq,
+ .irq_mask = pci_msi_mask_irq,
+ .irq_unmask = pci_msi_unmask_irq,
+};
+
+static struct msi_domain_info altera_msi_domain_info = {
+ .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
+ MSI_FLAG_PCI_MSIX),
+ .chip = &altera_msi_irq_chip,
+};
+
+static void altera_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
+{
+ u32 mask;
+ struct altera_msi *msi = irq_data_get_irq_chip_data(data);
+ u64 addr = msi->vector_phy + (data->hwirq * sizeof(u32));
+
+ msg->address_lo = lower_32_bits(addr);
+ msg->address_hi = upper_32_bits(addr);
+ msg->data = data->hwirq;
+
+ mask = msi_readl(msi, MSI_INTMASK);
+ mask |= 1 << data->hwirq;
+ msi_writel(msi, mask, MSI_INTMASK);
+ dev_dbg(&msi->pdev->dev, "msi#%d address_lo 0x%x\n", (int)data->hwirq,
+ msg->address_lo);
+}
+
+static int altera_msi_set_affinity(struct irq_data *irq_data,
+ const struct cpumask *mask, bool force)
+{
+ return -EINVAL;
+}
+
+static struct irq_chip altera_msi_bottom_irq_chip = {
+ .name = "Altera MSI",
+ .irq_compose_msi_msg = altera_compose_msi_msg,
+ .irq_set_affinity = altera_msi_set_affinity,
+};
+
+static int altera_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
+ unsigned int nr_irqs, void *args)
+{
+ struct altera_msi *msi = domain->host_data;
+ int bit;
+
+ mutex_lock(&msi->lock);
+
+ bit = find_first_zero_bit(msi->used, msi->num_of_vectors);
+ if (bit < msi->num_of_vectors)
+ set_bit(bit, msi->used);
+
+ mutex_unlock(&msi->lock);
+
+ if (bit < 0)
+ return bit;
+ else if (bit >= msi->num_of_vectors)
+ return -ENOSPC;
+
+ irq_domain_set_info(domain, virq, bit, &altera_msi_bottom_irq_chip,
+ domain->host_data, handle_simple_irq,
+ NULL, NULL);
+ set_irq_flags(virq, IRQF_VALID);
+
+ return 0;
+}
+
+static void altera_irq_domain_free(struct irq_domain *domain,
+ unsigned int virq, unsigned int nr_irqs)
+{
+ struct irq_data *d = irq_domain_get_irq_data(domain, virq);
+ struct altera_msi *msi = irq_data_get_irq_chip_data(d);
+ u32 mask;
+
+ mutex_lock(&msi->lock);
+
+ if (!test_bit(d->hwirq, msi->used))
+ dev_err(&msi->pdev->dev, "trying to free unused MSI#%lu\n",
+ d->hwirq);
+ else {
+ clear_bit(d->hwirq, msi->used);
+ mask = msi_readl(msi, MSI_INTMASK);
+ mask &= ~(1 << d->hwirq);
+ msi_writel(msi, mask, MSI_INTMASK);
+ }
+
+ mutex_unlock(&msi->lock);
+}
+
+static const struct irq_domain_ops msi_domain_ops = {
+ .alloc = altera_irq_domain_alloc,
+ .free = altera_irq_domain_free,
+};
+
+static int altera_allocate_domains(struct altera_msi *msi)
+{
+ msi->inner_domain = irq_domain_add_linear(NULL, msi->num_of_vectors,
+ &msi_domain_ops, msi);
+ if (!msi->inner_domain) {
+ dev_err(&msi->pdev->dev, "failed to create IRQ domain\n");
+ return -ENOMEM;
+ }
+
+ msi->msi_domain = pci_msi_create_irq_domain(
+ msi->pdev->dev.of_node,
+ &altera_msi_domain_info, msi->inner_domain);
+ if (!msi->msi_domain) {
+ dev_err(&msi->pdev->dev, "failed to create MSI domain\n");
+ irq_domain_remove(msi->inner_domain);
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static void altera_free_domains(struct altera_msi *msi)
+{
+ irq_domain_remove(msi->msi_domain);
+ irq_domain_remove(msi->inner_domain);
+}
+
+
+static int altera_msi_remove(struct platform_device *pdev)
+{
+ struct altera_msi *msi = platform_get_drvdata(pdev);
+
+ msi_writel(msi, 0, MSI_INTMASK);
+ irq_set_chained_handler(msi->irq, NULL);
+ irq_set_handler_data(msi->irq, NULL);
+
+ altera_free_domains(msi);
+
+ platform_set_drvdata(pdev, NULL);
+ return 0;
+}
+
+int altera_msi_probe(struct platform_device *pdev)
+{
+ struct altera_msi *msi;
+ struct device_node *np = pdev->dev.of_node;
+ struct resource *res;
+ int ret;
+
+ msi = devm_kzalloc(&pdev->dev, sizeof(struct altera_msi),
+ GFP_KERNEL);
+ if (!msi)
+ return -ENOMEM;
+
+ mutex_init(&msi->lock);
+ msi->pdev = pdev;
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "csr");
+ msi->csr_base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(msi->csr_base)) {
+ dev_err(&pdev->dev, "get csr resource failed\n");
+ return PTR_ERR(msi->csr_base);
+ }
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+ "vector_slave");
+ msi->vector_base = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(msi->vector_base)) {
+ dev_err(&pdev->dev, "get vector slave resource failed\n");
+ return PTR_ERR(msi->vector_base);
+ }
+
+ msi->vector_phy = res->start;
+
+ if (of_property_read_u32(np, "num-vectors", &msi->num_of_vectors)) {
+ dev_err(&pdev->dev, "failed to parse the number of vectors\n");
+ return -EINVAL;
+ }
+
+ ret = altera_allocate_domains(msi);
+ if (ret)
+ return ret;
+
+ msi->irq = platform_get_irq(pdev, 0);
+ if (msi->irq <= 0) {
+ dev_err(&pdev->dev, "failed to map IRQ: %d\n", msi->irq);
+ ret = -ENODEV;
+ goto err;
+ }
+
+ irq_set_chained_handler(msi->irq, altera_msi_isr);
+ ret = irq_set_handler_data(msi->irq, msi);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to register IRQ handler: %d\n",
+ ret);
+ goto err;
+ }
+
+ platform_set_drvdata(pdev, msi);
+
+ return 0;
+
+err:
+ altera_msi_remove(pdev);
+ return ret;
+}
+
+static const struct of_device_id altera_msi_of_match[] = {
+ { .compatible = "altr,msi-1.0", NULL },
+ { },
+};
+MODULE_DEVICE_TABLE(of, altera_msi_of_match);
+
+static struct platform_driver altera_msi_driver = {
+ .driver = {
+ .name = "altera-msi",
+ .owner = THIS_MODULE,
+ .of_match_table = altera_msi_of_match,
+ },
+ .probe = altera_msi_probe,
+ .remove = altera_msi_remove,
+};
+
+static int __init altera_msi_init(void)
+{
+ return platform_driver_register(&altera_msi_driver);
+}
+
+subsys_initcall(altera_msi_init);
+
+MODULE_AUTHOR("Ley Foon Tan <lftan@altera.com>");
+MODULE_DESCRIPTION("Altera PCIe MSI support");
+MODULE_LICENSE("GPL v2");
--
1.8.2.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* Re: [PATCH v2 3/5] pci: altera: Add Altera PCIe MSI driver
2015-07-31 10:15 ` [PATCH v2 3/5] pci: altera: Add Altera PCIe MSI driver Ley Foon Tan
@ 2015-07-31 12:12 ` Marc Zyngier
2015-08-03 10:37 ` Ley Foon Tan
0 siblings, 1 reply; 12+ messages in thread
From: Marc Zyngier @ 2015-07-31 12:12 UTC (permalink / raw)
To: Ley Foon Tan, Bjorn Helgaas, Russell King, Arnd Bergmann,
Dinh Nguyen
Cc: devicetree, lftan.linux, linux-doc, linux-pci, linux-kernel,
Rob Herring, linux-arm-kernel
On 31/07/15 11:15, Ley Foon Tan wrote:
> This patch adds Altera PCIe MSI driver. This soft IP supports configurable
> number of vectors, which is a dts parameter.
I've reviewed the initial drop of this code; basic courtesy would be to
keep me CCed on the follow-up series.
>
> Signed-off-by: Ley Foon Tan <lftan@altera.com>
> ---
> drivers/pci/host/Kconfig | 7 +
> drivers/pci/host/Makefile | 1 +
> drivers/pci/host/pcie-altera-msi.c | 324 +++++++++++++++++++++++++++++++++++++
> 3 files changed, 332 insertions(+)
> create mode 100644 drivers/pci/host/pcie-altera-msi.c
>
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index 108500a..51d0468 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -153,4 +153,11 @@ config PCIE_ALTERA
> Say Y here if you want to enable PCIe controller support for Altera
> SoCFPGA family of SoCs.
>
> +config PCIE_ALTERA_MSI
> + bool "Altera PCIe MSI feature"
> + depends on PCI_MSI
> + help
> + Say Y here if you want PCIe MSI support for the Altera SocFPGA SoC.
> + This MSI driver supports Altera MSI to GIC controller IP.
> +
> endmenu
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 6954f76..6c4913d 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -18,3 +18,4 @@ obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
> obj-$(CONFIG_PCIE_IPROC_PLATFORM) += pcie-iproc-platform.o
> obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
> obj-$(CONFIG_PCIE_ALTERA) += pcie-altera.o
> +obj-$(CONFIG_PCIE_ALTERA_MSI) += pcie-altera-msi.o
> diff --git a/drivers/pci/host/pcie-altera-msi.c b/drivers/pci/host/pcie-altera-msi.c
> new file mode 100644
> index 0000000..6014719
> --- /dev/null
> +++ b/drivers/pci/host/pcie-altera-msi.c
> @@ -0,0 +1,324 @@
> +/*
> + * Copyright Altera Corporation (C) 2013-2015. All rights reserved
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <linux/interrupt.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/module.h>
> +#include <linux/msi.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_pci.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#define MSI_STATUS 0x0
> +#define MSI_ERROR 0x4
> +#define MSI_INTMASK 0x8
> +
> +#define MAX_MSI_VECTORS 32
> +struct altera_msi {
> + DECLARE_BITMAP(used, MAX_MSI_VECTORS);
> + struct mutex lock; /* proctect used variable */
> + struct platform_device *pdev;
> + struct irq_domain *msi_domain;
> + struct irq_domain *inner_domain;
> + void __iomem *csr_base;
> + void __iomem *vector_base;
> + phys_addr_t vector_phy;
> + u32 num_of_vectors;
> + int irq;
> +};
> +
> +static inline void msi_writel(struct altera_msi *msi, u32 value, u32 reg)
> +{
> + writel_relaxed(value, msi->csr_base + reg);
> +}
> +
> +static inline u32 msi_readl(struct altera_msi *msi, u32 reg)
> +{
> + return readl_relaxed(msi->csr_base + reg);
> +}
> +
> +static void altera_msi_isr(unsigned int irq, struct irq_desc *desc)
> +{
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct altera_msi *msi;
> + unsigned long status;
> + u32 num_of_vectors;
> + u32 bit;
> +
> + chained_irq_enter(chip, desc);
> + msi = irq_desc_get_handler_data(desc);
> + num_of_vectors = msi->num_of_vectors;
> +
> + do {
> + status = msi_readl(msi, MSI_STATUS);
> + if (!status)
> + break;
> +
> + do {
> + bit = find_first_bit(&status, num_of_vectors);
> + /* Dummy read from vector to clear the interrupt */
> + readl_relaxed(msi->vector_base + (bit * sizeof(u32)));
> +
> + irq = irq_find_mapping(msi->inner_domain, bit);
Use of irq is a bit confusing, as this is a parameter describing the
chaining interrupt. Consider using a different name for this variable.
> + if (irq) {
> + if (test_bit(bit, msi->used))
> + generic_handle_irq(irq);
> + else
> + dev_info(&msi->pdev->dev, "unhandled MSI\n");
> + } else
> + dev_info(&msi->pdev->dev, "unexpected MSI\n");
The whole statement could be rewritten as:
if (irq && test_bit(bit, msi->used))
generic_handle_irq(irq);
else
dev_err(...);
In all cases, this is a grave error, so dev_info is not appropriate here.
> +
> + /* Clear the bit from status and repeat without reading
> + * again status register. */
> + clear_bit(bit, &status);
You don't need an atomic operation here, so __clear_bit() should be enough.
> + } while (status);
> + } while (1);
> +
> + chained_irq_exit(chip, desc);
> +}
> +
> +static struct irq_chip altera_msi_irq_chip = {
> + .name = "Altera PCIe MSI",
> + .irq_enable = pci_msi_unmask_irq,
> + .irq_disable = pci_msi_mask_irq,
> + .irq_mask = pci_msi_mask_irq,
> + .irq_unmask = pci_msi_unmask_irq,
> +};
> +
> +static struct msi_domain_info altera_msi_domain_info = {
> + .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> + MSI_FLAG_PCI_MSIX),
> + .chip = &altera_msi_irq_chip,
> +};
> +
> +static void altera_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> +{
> + u32 mask;
> + struct altera_msi *msi = irq_data_get_irq_chip_data(data);
> + u64 addr = msi->vector_phy + (data->hwirq * sizeof(u32));
Use phys_addr_t.
> +
> + msg->address_lo = lower_32_bits(addr);
> + msg->address_hi = upper_32_bits(addr);
> + msg->data = data->hwirq;
> +
> + mask = msi_readl(msi, MSI_INTMASK);
> + mask |= 1 << data->hwirq;
> + msi_writel(msi, mask, MSI_INTMASK);
It feels a bit weird to unmask the interrupt when you compose the
message. I'd expect this to be done in the mask/unmask methods.
> + dev_dbg(&msi->pdev->dev, "msi#%d address_lo 0x%x\n", (int)data->hwirq,
> + msg->address_lo);
You may want to print the whole address here.
> +}
> +
> +static int altera_msi_set_affinity(struct irq_data *irq_data,
> + const struct cpumask *mask, bool force)
> +{
> + return -EINVAL;
> +}
> +
> +static struct irq_chip altera_msi_bottom_irq_chip = {
> + .name = "Altera MSI",
> + .irq_compose_msi_msg = altera_compose_msi_msg,
> + .irq_set_affinity = altera_msi_set_affinity,
> +};
> +
> +static int altera_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> + unsigned int nr_irqs, void *args)
> +{
> + struct altera_msi *msi = domain->host_data;
> + int bit;
> +
> + mutex_lock(&msi->lock);
> +
> + bit = find_first_zero_bit(msi->used, msi->num_of_vectors);
> + if (bit < msi->num_of_vectors)
> + set_bit(bit, msi->used);
> +
> + mutex_unlock(&msi->lock);
> +
> + if (bit < 0)
> + return bit;
How can "bit" be negative here? find_first_zero_bit returns an unsigned
long... You probably want to change the type of "bit" to reflect that.
> + else if (bit >= msi->num_of_vectors)
Useless "else" anyway.
> + return -ENOSPC;
> +
> + irq_domain_set_info(domain, virq, bit, &altera_msi_bottom_irq_chip,
> + domain->host_data, handle_simple_irq,
> + NULL, NULL);
> + set_irq_flags(virq, IRQF_VALID);
> +
> + return 0;
> +}
> +
> +static void altera_irq_domain_free(struct irq_domain *domain,
> + unsigned int virq, unsigned int nr_irqs)
> +{
> + struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> + struct altera_msi *msi = irq_data_get_irq_chip_data(d);
> + u32 mask;
> +
> + mutex_lock(&msi->lock);
> +
> + if (!test_bit(d->hwirq, msi->used))
> + dev_err(&msi->pdev->dev, "trying to free unused MSI#%lu\n",
> + d->hwirq);
Put this statement in { } so that it matches the "else" construct.
> + else {
> + clear_bit(d->hwirq, msi->used);
No need for an atomic operation here, you own the mutex.
> + mask = msi_readl(msi, MSI_INTMASK);
> + mask &= ~(1 << d->hwirq);
> + msi_writel(msi, mask, MSI_INTMASK);
> + }
> +
> + mutex_unlock(&msi->lock);
> +}
> +
> +static const struct irq_domain_ops msi_domain_ops = {
> + .alloc = altera_irq_domain_alloc,
> + .free = altera_irq_domain_free,
> +};
> +
> +static int altera_allocate_domains(struct altera_msi *msi)
> +{
> + msi->inner_domain = irq_domain_add_linear(NULL, msi->num_of_vectors,
> + &msi_domain_ops, msi);
> + if (!msi->inner_domain) {
> + dev_err(&msi->pdev->dev, "failed to create IRQ domain\n");
> + return -ENOMEM;
> + }
> +
> + msi->msi_domain = pci_msi_create_irq_domain(
> + msi->pdev->dev.of_node,
Please put at least one argument to the function on the same line. This
is otherwise very hard to read.
> + &altera_msi_domain_info, msi->inner_domain);
> + if (!msi->msi_domain) {
> + dev_err(&msi->pdev->dev, "failed to create MSI domain\n");
> + irq_domain_remove(msi->inner_domain);
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +static void altera_free_domains(struct altera_msi *msi)
> +{
> + irq_domain_remove(msi->msi_domain);
> + irq_domain_remove(msi->inner_domain);
> +}
> +
> +
> +static int altera_msi_remove(struct platform_device *pdev)
> +{
> + struct altera_msi *msi = platform_get_drvdata(pdev);
> +
> + msi_writel(msi, 0, MSI_INTMASK);
> + irq_set_chained_handler(msi->irq, NULL);
> + irq_set_handler_data(msi->irq, NULL);
> +
> + altera_free_domains(msi);
> +
> + platform_set_drvdata(pdev, NULL);
> + return 0;
> +}
> +
> +int altera_msi_probe(struct platform_device *pdev)
Make this static.
> +{
> + struct altera_msi *msi;
> + struct device_node *np = pdev->dev.of_node;
> + struct resource *res;
> + int ret;
> +
> + msi = devm_kzalloc(&pdev->dev, sizeof(struct altera_msi),
> + GFP_KERNEL);
> + if (!msi)
> + return -ENOMEM;
> +
> + mutex_init(&msi->lock);
> + msi->pdev = pdev;
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "csr");
> + msi->csr_base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(msi->csr_base)) {
> + dev_err(&pdev->dev, "get csr resource failed\n");
> + return PTR_ERR(msi->csr_base);
> + }
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> + "vector_slave");
> + msi->vector_base = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(msi->vector_base)) {
> + dev_err(&pdev->dev, "get vector slave resource failed\n");
> + return PTR_ERR(msi->vector_base);
> + }
> +
> + msi->vector_phy = res->start;
> +
> + if (of_property_read_u32(np, "num-vectors", &msi->num_of_vectors)) {
> + dev_err(&pdev->dev, "failed to parse the number of vectors\n");
> + return -EINVAL;
> + }
> +
> + ret = altera_allocate_domains(msi);
> + if (ret)
> + return ret;
> +
> + msi->irq = platform_get_irq(pdev, 0);
> + if (msi->irq <= 0) {
> + dev_err(&pdev->dev, "failed to map IRQ: %d\n", msi->irq);
> + ret = -ENODEV;
> + goto err;
> + }
> +
> + irq_set_chained_handler(msi->irq, altera_msi_isr);
> + ret = irq_set_handler_data(msi->irq, msi);
Use irq_set_chained_handler_and_data.
> + if (ret) {
> + dev_err(&pdev->dev, "failed to register IRQ handler: %d\n",
> + ret);
> + goto err;
> + }
> +
> + platform_set_drvdata(pdev, msi);
> +
> + return 0;
> +
> +err:
> + altera_msi_remove(pdev);
> + return ret;
> +}
> +
> +static const struct of_device_id altera_msi_of_match[] = {
> + { .compatible = "altr,msi-1.0", NULL },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, altera_msi_of_match);
> +
> +static struct platform_driver altera_msi_driver = {
> + .driver = {
> + .name = "altera-msi",
> + .owner = THIS_MODULE,
Remove this .owner assignment, the core code deals with it.
> + .of_match_table = altera_msi_of_match,
> + },
> + .probe = altera_msi_probe,
> + .remove = altera_msi_remove,
> +};
> +
> +static int __init altera_msi_init(void)
> +{
> + return platform_driver_register(&altera_msi_driver);
> +}
> +
> +subsys_initcall(altera_msi_init);
Hmmm. if you're initializing this as part of the core kernel, it will
never be a module. What is the point of the MODULE_DEVICE_TABLE above?
> +
> +MODULE_AUTHOR("Ley Foon Tan <lftan@altera.com>");
> +MODULE_DESCRIPTION("Altera PCIe MSI support");
> +MODULE_LICENSE("GPL v2");
>
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 3/5] pci: altera: Add Altera PCIe MSI driver
2015-07-31 12:12 ` Marc Zyngier
@ 2015-08-03 10:37 ` Ley Foon Tan
2015-08-03 10:53 ` Marc Zyngier
0 siblings, 1 reply; 12+ messages in thread
From: Ley Foon Tan @ 2015-08-03 10:37 UTC (permalink / raw)
To: Marc Zyngier
Cc: Bjorn Helgaas, Russell King, Arnd Bergmann, Dinh Nguyen,
devicetree, linux-doc@vger.kernel.org, linux-pci,
linux-kernel@vger.kernel.org, Rob Herring, linux-arm-kernel
On Fri, Jul 31, 2015 at 8:12 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 31/07/15 11:15, Ley Foon Tan wrote:
>> This patch adds Altera PCIe MSI driver. This soft IP supports configurable
>> number of vectors, which is a dts parameter.
>
> I've reviewed the initial drop of this code; basic courtesy would be to
> keep me CCed on the follow-up series.
Will keep you in CC for the following revision. Sorry about this.
>>
>> Signed-off-by: Ley Foon Tan <lftan@altera.com>
>> ---
>> drivers/pci/host/Kconfig | 7 +
>> drivers/pci/host/Makefile | 1 +
>> drivers/pci/host/pcie-altera-msi.c | 324 +++++++++++++++++++++++++++++++++++++
>> 3 files changed, 332 insertions(+)
>> create mode 100644 drivers/pci/host/pcie-altera-msi.c
>>
>> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
>> index 108500a..51d0468 100644
>> --- a/drivers/pci/host/Kconfig
>> +++ b/drivers/pci/host/Kconfig
>> @@ -153,4 +153,11 @@ config PCIE_ALTERA
>> Say Y here if you want to enable PCIe controller support for Altera
>> SoCFPGA family of SoCs.
>>
>> +config PCIE_ALTERA_MSI
>> + bool "Altera PCIe MSI feature"
>> + depends on PCI_MSI
>> + help
>> + Say Y here if you want PCIe MSI support for the Altera SocFPGA SoC.
>> + This MSI driver supports Altera MSI to GIC controller IP.
>> +
>> endmenu
>> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
>> index 6954f76..6c4913d 100644
>> --- a/drivers/pci/host/Makefile
>> +++ b/drivers/pci/host/Makefile
>> @@ -18,3 +18,4 @@ obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
>> obj-$(CONFIG_PCIE_IPROC_PLATFORM) += pcie-iproc-platform.o
>> obj-$(CONFIG_PCIE_IPROC_BCMA) += pcie-iproc-bcma.o
>> obj-$(CONFIG_PCIE_ALTERA) += pcie-altera.o
>> +obj-$(CONFIG_PCIE_ALTERA_MSI) += pcie-altera-msi.o
>> diff --git a/drivers/pci/host/pcie-altera-msi.c b/drivers/pci/host/pcie-altera-msi.c
>> new file mode 100644
>> index 0000000..6014719
>> --- /dev/null
>> +++ b/drivers/pci/host/pcie-altera-msi.c
>> @@ -0,0 +1,324 @@
>> +/*
>> + * Copyright Altera Corporation (C) 2013-2015. All rights reserved
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +#include <linux/interrupt.h>
>> +#include <linux/irqchip/chained_irq.h>
>> +#include <linux/module.h>
>> +#include <linux/msi.h>
>> +#include <linux/of_address.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_pci.h>
>> +#include <linux/pci.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +
>> +#define MSI_STATUS 0x0
>> +#define MSI_ERROR 0x4
>> +#define MSI_INTMASK 0x8
>> +
>> +#define MAX_MSI_VECTORS 32
>> +struct altera_msi {
>> + DECLARE_BITMAP(used, MAX_MSI_VECTORS);
>> + struct mutex lock; /* proctect used variable */
>> + struct platform_device *pdev;
>> + struct irq_domain *msi_domain;
>> + struct irq_domain *inner_domain;
>> + void __iomem *csr_base;
>> + void __iomem *vector_base;
>> + phys_addr_t vector_phy;
>> + u32 num_of_vectors;
>> + int irq;
>> +};
>> +
>> +static inline void msi_writel(struct altera_msi *msi, u32 value, u32 reg)
>> +{
>> + writel_relaxed(value, msi->csr_base + reg);
>> +}
>> +
>> +static inline u32 msi_readl(struct altera_msi *msi, u32 reg)
>> +{
>> + return readl_relaxed(msi->csr_base + reg);
>> +}
>> +
>> +static void altera_msi_isr(unsigned int irq, struct irq_desc *desc)
>> +{
>> + struct irq_chip *chip = irq_desc_get_chip(desc);
>> + struct altera_msi *msi;
>> + unsigned long status;
>> + u32 num_of_vectors;
>> + u32 bit;
>> +
>> + chained_irq_enter(chip, desc);
>> + msi = irq_desc_get_handler_data(desc);
>> + num_of_vectors = msi->num_of_vectors;
>> +
>> + do {
>> + status = msi_readl(msi, MSI_STATUS);
>> + if (!status)
>> + break;
>> +
>> + do {
>> + bit = find_first_bit(&status, num_of_vectors);
>> + /* Dummy read from vector to clear the interrupt */
>> + readl_relaxed(msi->vector_base + (bit * sizeof(u32)));
>> +
>> + irq = irq_find_mapping(msi->inner_domain, bit);
>
> Use of irq is a bit confusing, as this is a parameter describing the
> chaining interrupt. Consider using a different name for this variable.
Okay, will change it to 'virq'.
>
>> + if (irq) {
>> + if (test_bit(bit, msi->used))
>> + generic_handle_irq(irq);
>> + else
>> + dev_info(&msi->pdev->dev, "unhandled MSI\n");
>> + } else
>> + dev_info(&msi->pdev->dev, "unexpected MSI\n");
>
> The whole statement could be rewritten as:
>
> if (irq && test_bit(bit, msi->used))
> generic_handle_irq(irq);
> else
> dev_err(...);
>
> In all cases, this is a grave error, so dev_info is not appropriate here.
Noted.
>
>> +
>> + /* Clear the bit from status and repeat without reading
>> + * again status register. */
>> + clear_bit(bit, &status);
>
> You don't need an atomic operation here, so __clear_bit() should be enough.
Noted.
>
>> + } while (status);
>> + } while (1);
>> +
>> + chained_irq_exit(chip, desc);
>> +}
>> +
>> +static struct irq_chip altera_msi_irq_chip = {
>> + .name = "Altera PCIe MSI",
>> + .irq_enable = pci_msi_unmask_irq,
>> + .irq_disable = pci_msi_mask_irq,
>> + .irq_mask = pci_msi_mask_irq,
>> + .irq_unmask = pci_msi_unmask_irq,
>> +};
>> +
>> +static struct msi_domain_info altera_msi_domain_info = {
>> + .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
>> + MSI_FLAG_PCI_MSIX),
>> + .chip = &altera_msi_irq_chip,
>> +};
>> +
>> +static void altera_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
>> +{
>> + u32 mask;
>> + struct altera_msi *msi = irq_data_get_irq_chip_data(data);
>> + u64 addr = msi->vector_phy + (data->hwirq * sizeof(u32));
>
> Use phys_addr_t.
Noted.
>
>> +
>> + msg->address_lo = lower_32_bits(addr);
>> + msg->address_hi = upper_32_bits(addr);
>> + msg->data = data->hwirq;
>> +
>> + mask = msi_readl(msi, MSI_INTMASK);
>> + mask |= 1 << data->hwirq;
>> + msi_writel(msi, mask, MSI_INTMASK);
>
> It feels a bit weird to unmask the interrupt when you compose the
> message. I'd expect this to be done in the mask/unmask methods.
Do you refer to these 2 callbacks?
.irq_mask = pci_msi_mask_irq,
.irq_unmask = pci_msi_unmask_irq,
How about we move this INTMASK code above to altera_irq_domain_alloc()?
We have unmask code in altera_irq_domain_free() now.
>
>> + dev_dbg(&msi->pdev->dev, "msi#%d address_lo 0x%x\n", (int)data->hwirq,
>> + msg->address_lo);
>
> You may want to print the whole address here.
Noted.
>
>> +}
>> +
>> +static int altera_msi_set_affinity(struct irq_data *irq_data,
>> + const struct cpumask *mask, bool force)
>> +{
>> + return -EINVAL;
>> +}
>> +
>> +static struct irq_chip altera_msi_bottom_irq_chip = {
>> + .name = "Altera MSI",
>> + .irq_compose_msi_msg = altera_compose_msi_msg,
>> + .irq_set_affinity = altera_msi_set_affinity,
>> +};
>> +
>> +static int altera_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>> + unsigned int nr_irqs, void *args)
>> +{
>> + struct altera_msi *msi = domain->host_data;
>> + int bit;
>> +
>> + mutex_lock(&msi->lock);
>> +
>> + bit = find_first_zero_bit(msi->used, msi->num_of_vectors);
>> + if (bit < msi->num_of_vectors)
>> + set_bit(bit, msi->used);
>> +
>> + mutex_unlock(&msi->lock);
>> +
>> + if (bit < 0)
>> + return bit;
>
> How can "bit" be negative here? find_first_zero_bit returns an unsigned
> long... You probably want to change the type of "bit" to reflect that.
Okay, will change to "unsigned long" type.
>
>> + else if (bit >= msi->num_of_vectors)
>
> Useless "else" anyway.
I think we still need to check for failing case, if we don't have
available unused bit.
This could be rewritten as below:
bit = find_first_zero_bit(msi->used, msi->num_of_vectors);
if (bit < msi->num_of_vectors)
set_bit(bit, msi->used);
else
return -ENOSPC;
>
>> + return -ENOSPC;
>> +
>> + irq_domain_set_info(domain, virq, bit, &altera_msi_bottom_irq_chip,
>> + domain->host_data, handle_simple_irq,
>> + NULL, NULL);
>> + set_irq_flags(virq, IRQF_VALID);
>> +
>> + return 0;
>> +}
>> +
>> +static void altera_irq_domain_free(struct irq_domain *domain,
>> + unsigned int virq, unsigned int nr_irqs)
>> +{
>> + struct irq_data *d = irq_domain_get_irq_data(domain, virq);
>> + struct altera_msi *msi = irq_data_get_irq_chip_data(d);
>> + u32 mask;
>> +
>> + mutex_lock(&msi->lock);
>> +
>> + if (!test_bit(d->hwirq, msi->used))
>> + dev_err(&msi->pdev->dev, "trying to free unused MSI#%lu\n",
>> + d->hwirq);
>
> Put this statement in { } so that it matches the "else" construct.
Noted.
>
>> + else {
>> + clear_bit(d->hwirq, msi->used);
>
> No need for an atomic operation here, you own the mutex.
Okay, change to __clear_bit.
>
>> + mask = msi_readl(msi, MSI_INTMASK);
>> + mask &= ~(1 << d->hwirq);
>> + msi_writel(msi, mask, MSI_INTMASK);
>> + }
>> +
>> + mutex_unlock(&msi->lock);
>> +}
>> +
>> +static const struct irq_domain_ops msi_domain_ops = {
>> + .alloc = altera_irq_domain_alloc,
>> + .free = altera_irq_domain_free,
>> +};
>> +
>> +static int altera_allocate_domains(struct altera_msi *msi)
>> +{
>> + msi->inner_domain = irq_domain_add_linear(NULL, msi->num_of_vectors,
>> + &msi_domain_ops, msi);
>> + if (!msi->inner_domain) {
>> + dev_err(&msi->pdev->dev, "failed to create IRQ domain\n");
>> + return -ENOMEM;
>> + }
>> +
>> + msi->msi_domain = pci_msi_create_irq_domain(
>> + msi->pdev->dev.of_node,
>
> Please put at least one argument to the function on the same line. This
> is otherwise very hard to read.
Okay.
>
>> + &altera_msi_domain_info, msi->inner_domain);
>> + if (!msi->msi_domain) {
>> + dev_err(&msi->pdev->dev, "failed to create MSI domain\n");
>> + irq_domain_remove(msi->inner_domain);
>> + return -ENOMEM;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void altera_free_domains(struct altera_msi *msi)
>> +{
>> + irq_domain_remove(msi->msi_domain);
>> + irq_domain_remove(msi->inner_domain);
>> +}
>> +
>> +
>> +static int altera_msi_remove(struct platform_device *pdev)
>> +{
>> + struct altera_msi *msi = platform_get_drvdata(pdev);
>> +
>> + msi_writel(msi, 0, MSI_INTMASK);
>> + irq_set_chained_handler(msi->irq, NULL);
>> + irq_set_handler_data(msi->irq, NULL);
>> +
>> + altera_free_domains(msi);
>> +
>> + platform_set_drvdata(pdev, NULL);
>> + return 0;
>> +}
>> +
>> +int altera_msi_probe(struct platform_device *pdev)
>
> Make this static.
Okay.
>
>> +{
>> + struct altera_msi *msi;
>> + struct device_node *np = pdev->dev.of_node;
>> + struct resource *res;
>> + int ret;
>> +
>> + msi = devm_kzalloc(&pdev->dev, sizeof(struct altera_msi),
>> + GFP_KERNEL);
>> + if (!msi)
>> + return -ENOMEM;
>> +
>> + mutex_init(&msi->lock);
>> + msi->pdev = pdev;
>> +
>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "csr");
>> + msi->csr_base = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(msi->csr_base)) {
>> + dev_err(&pdev->dev, "get csr resource failed\n");
>> + return PTR_ERR(msi->csr_base);
>> + }
>> +
>> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> + "vector_slave");
>> + msi->vector_base = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(msi->vector_base)) {
>> + dev_err(&pdev->dev, "get vector slave resource failed\n");
>> + return PTR_ERR(msi->vector_base);
>> + }
>> +
>> + msi->vector_phy = res->start;
>> +
>> + if (of_property_read_u32(np, "num-vectors", &msi->num_of_vectors)) {
>> + dev_err(&pdev->dev, "failed to parse the number of vectors\n");
>> + return -EINVAL;
>> + }
>> +
>> + ret = altera_allocate_domains(msi);
>> + if (ret)
>> + return ret;
>> +
>> + msi->irq = platform_get_irq(pdev, 0);
>> + if (msi->irq <= 0) {
>> + dev_err(&pdev->dev, "failed to map IRQ: %d\n", msi->irq);
>> + ret = -ENODEV;
>> + goto err;
>> + }
>> +
>> + irq_set_chained_handler(msi->irq, altera_msi_isr);
>> + ret = irq_set_handler_data(msi->irq, msi);
>
> Use irq_set_chained_handler_and_data.
Okay.
>
>> + if (ret) {
>> + dev_err(&pdev->dev, "failed to register IRQ handler: %d\n",
>> + ret);
>> + goto err;
>> + }
>> +
>> + platform_set_drvdata(pdev, msi);
>> +
>> + return 0;
>> +
>> +err:
>> + altera_msi_remove(pdev);
>> + return ret;
>> +}
>> +
>> +static const struct of_device_id altera_msi_of_match[] = {
>> + { .compatible = "altr,msi-1.0", NULL },
>> + { },
>> +};
>> +MODULE_DEVICE_TABLE(of, altera_msi_of_match);
>> +
>> +static struct platform_driver altera_msi_driver = {
>> + .driver = {
>> + .name = "altera-msi",
>> + .owner = THIS_MODULE,
>
> Remove this .owner assignment, the core code deals with it.
Okay.
>
>> + .of_match_table = altera_msi_of_match,
>> + },
>> + .probe = altera_msi_probe,
>> + .remove = altera_msi_remove,
>> +};
>> +
>> +static int __init altera_msi_init(void)
>> +{
>> + return platform_driver_register(&altera_msi_driver);
>> +}
>> +
>> +subsys_initcall(altera_msi_init);
>
> Hmmm. if you're initializing this as part of the core kernel, it will
> never be a module. What is the point of the MODULE_DEVICE_TABLE above?
Okay, will remove MODULE_DEVICE_TABLE.
Thanks for reviewing.
Regards
Ley Foon
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 3/5] pci: altera: Add Altera PCIe MSI driver
2015-08-03 10:37 ` Ley Foon Tan
@ 2015-08-03 10:53 ` Marc Zyngier
2015-08-03 11:00 ` Ley Foon Tan
0 siblings, 1 reply; 12+ messages in thread
From: Marc Zyngier @ 2015-08-03 10:53 UTC (permalink / raw)
To: Ley Foon Tan
Cc: Bjorn Helgaas, Russell King, Arnd Bergmann, Dinh Nguyen,
devicetree@vger.kernel.org, linux-doc@vger.kernel.org,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
Rob Herring, linux-arm-kernel@lists.infradead.org
On 03/08/15 11:37, Ley Foon Tan wrote:
> On Fri, Jul 31, 2015 at 8:12 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
>> On 31/07/15 11:15, Ley Foon Tan wrote:
>>> This patch adds Altera PCIe MSI driver. This soft IP supports configurable
>>> number of vectors, which is a dts parameter.
>>
>> I've reviewed the initial drop of this code; basic courtesy would be to
>> keep me CCed on the follow-up series.
> Will keep you in CC for the following revision. Sorry about this.
>
>>>
>>> Signed-off-by: Ley Foon Tan <lftan@altera.com>
>>> ---
>>> drivers/pci/host/Kconfig | 7 +
>>> drivers/pci/host/Makefile | 1 +
>>> drivers/pci/host/pcie-altera-msi.c | 324 +++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 332 insertions(+)
>>> create mode 100644 drivers/pci/host/pcie-altera-msi.c
>>>
[...]
>>> +
>>> + msg->address_lo = lower_32_bits(addr);
>>> + msg->address_hi = upper_32_bits(addr);
>>> + msg->data = data->hwirq;
>>> +
>>> + mask = msi_readl(msi, MSI_INTMASK);
>>> + mask |= 1 << data->hwirq;
>>> + msi_writel(msi, mask, MSI_INTMASK);
>>
>> It feels a bit weird to unmask the interrupt when you compose the
>> message. I'd expect this to be done in the mask/unmask methods.
> Do you refer to these 2 callbacks?
> .irq_mask = pci_msi_mask_irq,
> .irq_unmask = pci_msi_unmask_irq,
>
> How about we move this INTMASK code above to altera_irq_domain_alloc()?
> We have unmask code in altera_irq_domain_free() now.
Either you move the mask/unmask code to local irq_mask/irq_unmask
methods (and call pci_msi_(un)mask_irq from there), or you move it
entierely to alloc/free.
Some level of symmetry helps following what is going on in the code.
[...]
>>> +static int altera_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>>> + unsigned int nr_irqs, void *args)
>>> +{
>>> + struct altera_msi *msi = domain->host_data;
>>> + int bit;
>>> +
>>> + mutex_lock(&msi->lock);
>>> +
>>> + bit = find_first_zero_bit(msi->used, msi->num_of_vectors);
>>> + if (bit < msi->num_of_vectors)
>>> + set_bit(bit, msi->used);
>>> +
>>> + mutex_unlock(&msi->lock);
>>> +
>>> + if (bit < 0)
>>> + return bit;
>>
>> How can "bit" be negative here? find_first_zero_bit returns an unsigned
>> long... You probably want to change the type of "bit" to reflect that.
> Okay, will change to "unsigned long" type.
>>
>>> + else if (bit >= msi->num_of_vectors)
>>
>> Useless "else" anyway.
>
> I think we still need to check for failing case, if we don't have
> available unused bit.
> This could be rewritten as below:
>
> bit = find_first_zero_bit(msi->used, msi->num_of_vectors);
> if (bit < msi->num_of_vectors)
> set_bit(bit, msi->used);
> else
> return -ENOSPC;
The more logical to write this is:
if (bit >= msi->num_of_vectors)
return -ENOSPC;
set_bit(bit, msi->used);
which gets rid of the error case early and streamlines the normal case.
>
>>
>>> + return -ENOSPC;
>>> +
>>> + irq_domain_set_info(domain, virq, bit, &altera_msi_bottom_irq_chip,
>>> + domain->host_data, handle_simple_irq,
>>> + NULL, NULL);
>>> + set_irq_flags(virq, IRQF_VALID);
>>> +
>>> + return 0;
>>> +}
Thanks,
M.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH v2 3/5] pci: altera: Add Altera PCIe MSI driver
2015-08-03 10:53 ` Marc Zyngier
@ 2015-08-03 11:00 ` Ley Foon Tan
0 siblings, 0 replies; 12+ messages in thread
From: Ley Foon Tan @ 2015-08-03 11:00 UTC (permalink / raw)
To: Marc Zyngier
Cc: Bjorn Helgaas, Russell King, Arnd Bergmann, Dinh Nguyen,
devicetree@vger.kernel.org, linux-doc@vger.kernel.org,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
Rob Herring, linux-arm-kernel@lists.infradead.org
On Mon, Aug 3, 2015 at 6:53 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:
> On 03/08/15 11:37, Ley Foon Tan wrote:
>>>> + msg->address_lo = lower_32_bits(addr);
>>>> + msg->address_hi = upper_32_bits(addr);
>>>> + msg->data = data->hwirq;
>>>> +
>>>> + mask = msi_readl(msi, MSI_INTMASK);
>>>> + mask |= 1 << data->hwirq;
>>>> + msi_writel(msi, mask, MSI_INTMASK);
>>>
>>> It feels a bit weird to unmask the interrupt when you compose the
>>> message. I'd expect this to be done in the mask/unmask methods.
>> Do you refer to these 2 callbacks?
>> .irq_mask = pci_msi_mask_irq,
>> .irq_unmask = pci_msi_unmask_irq,
>>
>> How about we move this INTMASK code above to altera_irq_domain_alloc()?
>> We have unmask code in altera_irq_domain_free() now.
>
> Either you move the mask/unmask code to local irq_mask/irq_unmask
> methods (and call pci_msi_(un)mask_irq from there), or you move it
> entierely to alloc/free.
>
> Some level of symmetry helps following what is going on in the code.
Okay, will move it to alloc/free.
>
> [...]
>
>>>> +static int altera_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
>>>> + unsigned int nr_irqs, void *args)
>>>> +{
>>>> + struct altera_msi *msi = domain->host_data;
>>>> + int bit;
>>>> +
>>>> + mutex_lock(&msi->lock);
>>>> +
>>>> + bit = find_first_zero_bit(msi->used, msi->num_of_vectors);
>>>> + if (bit < msi->num_of_vectors)
>>>> + set_bit(bit, msi->used);
>>>> +
>>>> + mutex_unlock(&msi->lock);
>>>> +
>>>> + if (bit < 0)
>>>> + return bit;
>>>
>>> How can "bit" be negative here? find_first_zero_bit returns an unsigned
>>> long... You probably want to change the type of "bit" to reflect that.
>> Okay, will change to "unsigned long" type.
>>>
>>>> + else if (bit >= msi->num_of_vectors)
>>>
>>> Useless "else" anyway.
>>
>> I think we still need to check for failing case, if we don't have
>> available unused bit.
>> This could be rewritten as below:
>>
>> bit = find_first_zero_bit(msi->used, msi->num_of_vectors);
>> if (bit < msi->num_of_vectors)
>> set_bit(bit, msi->used);
>> else
>> return -ENOSPC;
>
> The more logical to write this is:
>
> if (bit >= msi->num_of_vectors)
> return -ENOSPC;
>
> set_bit(bit, msi->used);
>
> which gets rid of the error case early and streamlines the normal case.
Yes, will change to this way.
Thanks.
Regards
Ley Foon
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH v2 4/5] Documentation: dt-bindings: pci: altera pcie device tree binding
2015-07-31 10:15 [PATCH v2 0/5] Altera PCIe host controller driver with MSI support Ley Foon Tan
` (2 preceding siblings ...)
2015-07-31 10:15 ` [PATCH v2 3/5] pci: altera: Add Altera PCIe MSI driver Ley Foon Tan
@ 2015-07-31 10:15 ` Ley Foon Tan
2015-07-31 10:15 ` [PATCH v2 5/5] MAINTAINERS: Add Altera PCIe and MSI drivers maintainer Ley Foon Tan
4 siblings, 0 replies; 12+ messages in thread
From: Ley Foon Tan @ 2015-07-31 10:15 UTC (permalink / raw)
To: Bjorn Helgaas, Russell King, Arnd Bergmann, Dinh Nguyen
Cc: linux-pci, devicetree, linux-arm-kernel, linux-doc, linux-kernel,
Ley Foon Tan, lftan.linux, Rob Herring
This patch adds the bindings for Altera PCIe host controller driver and
Altera PCIe MSI driver.
Signed-off-by: Ley Foon Tan <lftan@altera.com>
---
.../devicetree/bindings/pci/altera-pcie-msi.txt | 27 ++++++++++++
.../devicetree/bindings/pci/altera-pcie.txt | 49 ++++++++++++++++++++++
2 files changed, 76 insertions(+)
create mode 100644 Documentation/devicetree/bindings/pci/altera-pcie-msi.txt
create mode 100644 Documentation/devicetree/bindings/pci/altera-pcie.txt
diff --git a/Documentation/devicetree/bindings/pci/altera-pcie-msi.txt b/Documentation/devicetree/bindings/pci/altera-pcie-msi.txt
new file mode 100644
index 0000000..7f330c9
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/altera-pcie-msi.txt
@@ -0,0 +1,27 @@
+* Altera PCIe MSI controller
+
+Required properties:
+- compatible: should contain "altr,msi-1.0"
+- reg: specifies the physical base address of the controller and
+ the length of the memory mapped region.
+- reg-names: Must include the following entries:
+ "csr": CSR registers
+ "vector_slave": vectors region
+-interrupts: specifies the interrupt source of the parent interrupt
+ controller. The format of the interrupt specifier depends on the
+ parent interrupt controller.
+- num-vectors: Number of vectors, range 1 to 32.
+- msi-controller: indicates that this is MSI controller node
+
+
+Example
+msi0: msi@0xFF200000 {
+ compatible = "altr,msi-1.0";
+ reg = <0xFF200000 0x00000010
+ 0xFF200010 0x00000080>;
+ reg-names = "csr", "vector_slave";
+ interrupt-parent = <&hps_0_arm_gic_0>;
+ interrupts = <0 42 4>;
+ msi-controller = <1>;
+ num-vectors = <32>;
+};
diff --git a/Documentation/devicetree/bindings/pci/altera-pcie.txt b/Documentation/devicetree/bindings/pci/altera-pcie.txt
new file mode 100644
index 0000000..54c45f5
--- /dev/null
+++ b/Documentation/devicetree/bindings/pci/altera-pcie.txt
@@ -0,0 +1,49 @@
+* Altera PCIe controller
+
+Required properties:
+- compatible : should contain "altr,pcie-root-port-1.0"
+- reg: A list of physical base address and length for TXS and CRA.
+- reg-names: Must include the following entries:
+ "Txs": TXS region
+ "Cra": Control register access region
+-interrupts: specifies the interrupt source of the parent interrupt controller.
+ The format of the interrupt specifier depends on the parent interrupt
+ controller.
+- device_type: must be "pci"
+- #address-cells: set to <3>
+- #size-cells: set to <2>
+- #interrupt-cells: set to <1>
+- ranges: Describes the translation of addresses for root ports and standard
+ PCI regions.
+- interrupt-map-mask and interrupt-map: standard PCI properties
+ to define the mapping of the PCIe interface to interrupt
+ numbers.
+
+Optional properties:
+- msi-parent: Link to the hardware entity that serves as the MSI controller for this PCIe
+ controller.
+- bus-range: PCI bus numbers covered
+
+Example
+ pcie_0: pcie@0xc00000000 {
+ compatible = "altr,pcie-root-port-1.0";
+ reg = <0xc0000000 0x20000000>,
+ <0xff220000 0x00004000>;
+ reg-names = "Txs", "Cra";
+ interrupt-parent = <&hps_0_arm_gic_0>;
+ interrupts = <0 40 4>;
+ interrupt-controller;
+ #interrupt-cells = <1>;
+ bus-range = <0x0 0xFF>;
+ device_type = "pci";
+ msi-parent = <&msi_to_gic_gen_0>;
+ #address-cells = <3>;
+ #size-cells = <2>;
+ interrupt-map-mask = <0 0 0 7>;
+ interrupt-map = <0 0 0 1 &pcie_0 1>,
+ <0 0 0 2 &pcie_0 2>,
+ <0 0 0 3 &pcie_0 3>,
+ <0 0 0 4 &pcie_0 4>;
+ ranges = <0x82000000 0x00000000 0xc0000000 0xc0000000 0x00000000 0x10000000
+ 0x82000000 0x00000000 0xd0000000 0xd0000000 0x00000000 0x10000000>;
+ };
--
1.8.2.1
^ permalink raw reply related [flat|nested] 12+ messages in thread* [PATCH v2 5/5] MAINTAINERS: Add Altera PCIe and MSI drivers maintainer
2015-07-31 10:15 [PATCH v2 0/5] Altera PCIe host controller driver with MSI support Ley Foon Tan
` (3 preceding siblings ...)
2015-07-31 10:15 ` [PATCH v2 4/5] Documentation: dt-bindings: pci: altera pcie device tree binding Ley Foon Tan
@ 2015-07-31 10:15 ` Ley Foon Tan
4 siblings, 0 replies; 12+ messages in thread
From: Ley Foon Tan @ 2015-07-31 10:15 UTC (permalink / raw)
To: Bjorn Helgaas, Russell King, Arnd Bergmann, Dinh Nguyen
Cc: linux-pci, devicetree, linux-arm-kernel, linux-doc, linux-kernel,
Ley Foon Tan, lftan.linux, Rob Herring
Signed-off-by: Ley Foon Tan <lftan@altera.com>
---
MAINTAINERS | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/MAINTAINERS b/MAINTAINERS
index fd60784..32f5287 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7759,6 +7759,14 @@ F: include/linux/pci*
F: arch/x86/pci/
F: arch/x86/kernel/quirks.c
+PCI DRIVER FOR ALTERA PCIE IP
+M: Ley Foon Tan <lftan@altera.com>
+L: rfi@lists.rocketboards.org (moderated for non-subscribers)
+L: linux-pci@vger.kernel.org
+S: Supported
+F: Documentation/devicetree/bindings/pci/altera-pcie.txt
+F: drivers/pci/host/pcie-altera.c
+
PCI DRIVER FOR ARM VERSATILE PLATFORM
M: Rob Herring <robh@kernel.org>
L: linux-pci@vger.kernel.org
@@ -7860,6 +7868,14 @@ L: linux-pci@vger.kernel.org
S: Maintained
F: drivers/pci/host/*spear*
+PCI MSI DRIVER FOR ALTERA MSI IP
+M: Ley Foon Tan <lftan@altera.com>
+L: rfi@lists.rocketboards.org (moderated for non-subscribers)
+L: linux-pci@vger.kernel.org
+S: Supported
+F: Documentation/devicetree/bindings/pci/altera-pcie-msi.txt
+F: drivers/pci/host/pcie-altera-msi.c
+
PCI MSI DRIVER FOR APPLIEDMICRO XGENE
M: Duc Dang <dhdang@apm.com>
L: linux-pci@vger.kernel.org
--
1.8.2.1
^ permalink raw reply related [flat|nested] 12+ messages in thread