2015-11-24 23:04:41

by Ray Jui

[permalink] [raw]
Subject: [PATCH v2 0/5] Add iProc PCIe PAXC and MSI support

This patch series adds support for the iProc PAXC interface and support for
event queue based MSI, integrated in the iProc PCIe core

This patch series is based on Linux v4.4-rc1 and is avaliable here:
https://github.com/Broadcom/cygnus-linux/tree/iproc-msi-v2

This patch series is tested on the following platforms:

PAXB:
- Broadcom Cygnus wireless audio board with Intel e1000e network card
- Broadcom NS2 SVK board with Intel e1000e network card

PAXC:
- Broadcom NS2 SVK board with integrated dual 10 Gig Ethernet ports

Changes from v1:
- Fixed incorrect 1-to-1 mapping between MSI vector and GIC interrupt. Now the
driver supports multiple MSI vectors per GIC interrupt
- Added MSI IRQ affinity support by distributing GIC interrupts across
available CPU cores and dynamically steer MSI vectors to the target CPU
- replace readl/writel with readl_relaxed/writel_relaxed since all register
accesses within the iProc MSI driver are to/from the same I/O block, i.e., the
iProc PCIe core
- Removed all redundant irq_chip callback assignments
- Changed to use uncached host memory for both MSI posted writes and event
queues
- Add functions to free resources in error/exit cases
- In pcie-iproc-platform.c, pass in interface type through OF device data
- Moved define for max number of interrupts from pcie-iproc.h to
pcie-iproc-msi.c
- Other misc. changes

Ray Jui (5):
PCI: iproc: Update iProc PCIe device tree binding
PCI: iproc: Add PAXC interface support
PCI: iproc: Add iProc PCIe MSI device tree binding
PCI: iproc: Add iProc PCIe MSI support
ARM: dts: Enable MSI support for Broadcom Cygnus

.../devicetree/bindings/pci/brcm,iproc-pcie.txt | 44 +-
arch/arm/boot/dts/bcm-cygnus.dtsi | 22 +
drivers/pci/host/Kconfig | 9 +
drivers/pci/host/Makefile | 1 +
drivers/pci/host/pcie-iproc-msi.c | 662 +++++++++++++++++++++
drivers/pci/host/pcie-iproc-platform.c | 24 +-
drivers/pci/host/pcie-iproc.c | 228 +++++--
drivers/pci/host/pcie-iproc.h | 40 +-
8 files changed, 987 insertions(+), 43 deletions(-)
create mode 100644 drivers/pci/host/pcie-iproc-msi.c

--
1.9.1


2015-11-24 23:04:46

by Ray Jui

[permalink] [raw]
Subject: [PATCH v2 1/5] PCI: iproc: Update iProc PCIe device tree binding

Add a new compatible string "brcm,iproc-pcie-paxc", for PAXC based iProc
PCIe root complex. A PAXC based PCIe root complex is connected to
emulated endpoint devices internal to the ASIC

Signed-off-by: Ray Jui <[email protected]>
Reviewed-by: Scott Branden <[email protected]>
---
Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
index 45c2a80..06eae0f 100644
--- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
@@ -1,7 +1,10 @@
* Broadcom iProc PCIe controller with the platform bus interface

Required properties:
-- compatible: Must be "brcm,iproc-pcie"
+- compatible: Must be "brcm,iproc-pcie" for PAXB, or "brcm,iproc-pcie-paxc"
+ for PAXC. PAXB based root complex is used for external endpoint devices.
+ PAXC based root complex is connected to emulated endpoint devices
+ internal to the ASIC
- reg: base address and length of the PCIe controller I/O register space
- #interrupt-cells: set to <1>
- interrupt-map-mask and interrupt-map, standard PCI properties to define the
--
1.9.1

2015-11-24 23:05:59

by Ray Jui

[permalink] [raw]
Subject: [PATCH v2 2/5] PCI: iproc: Add PAXC interface support

Traditionally, all iProc PCIe root complexes use PAXB based wrapper,
with an integrated on-chip Serdes to support external endpoint devices.
On newer iProc platforms, a PAXC based wrapper is introduced, for
connection with internally emulated PCIe endpoint devices in the ASIC

This patch adds support for PAXC based iProc PCIe root complex in the
iProc PCIe core driver. This change fators out common logic between
PAXB and PAXC, and use tables to store register offsets that are
different between PAXB and PAXC. This allows the driver to be scaled to
support subsequent PAXC revisions in the future

Signed-off-by: Ray Jui <[email protected]>
Reviewed-by: Scott Branden <[email protected]>
---
drivers/pci/host/pcie-iproc-platform.c | 24 +++-
drivers/pci/host/pcie-iproc.c | 202 +++++++++++++++++++++++++++------
drivers/pci/host/pcie-iproc.h | 19 ++++
3 files changed, 205 insertions(+), 40 deletions(-)

diff --git a/drivers/pci/host/pcie-iproc-platform.c b/drivers/pci/host/pcie-iproc-platform.c
index c9550dc..e8b32d8 100644
--- a/drivers/pci/host/pcie-iproc-platform.c
+++ b/drivers/pci/host/pcie-iproc-platform.c
@@ -26,8 +26,21 @@

#include "pcie-iproc.h"

+static const struct of_device_id iproc_pcie_of_match_table[] = {
+ {
+ .compatible = "brcm,iproc-pcie",
+ .data = (int *)IPROC_PCIE_PAXB,
+ }, {
+ .compatible = "brcm,iproc-pcie-paxc",
+ .data = (int *)IPROC_PCIE_PAXC,
+ },
+ { /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, iproc_pcie_of_match_table);
+
static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
{
+ const struct of_device_id *of_id;
struct iproc_pcie *pcie;
struct device_node *np = pdev->dev.of_node;
struct resource reg;
@@ -35,11 +48,16 @@ static int iproc_pcie_pltfm_probe(struct platform_device *pdev)
LIST_HEAD(res);
int ret;

+ of_id = of_match_device(iproc_pcie_of_match_table, &pdev->dev);
+ if (!of_id)
+ return -EINVAL;
+
pcie = devm_kzalloc(&pdev->dev, sizeof(struct iproc_pcie), GFP_KERNEL);
if (!pcie)
return -ENOMEM;

pcie->dev = &pdev->dev;
+ pcie->type = (enum iproc_pcie_type)of_id->data;
platform_set_drvdata(pdev, pcie);

ret = of_address_to_resource(np, 0, &reg);
@@ -114,12 +132,6 @@ static int iproc_pcie_pltfm_remove(struct platform_device *pdev)
return iproc_pcie_remove(pcie);
}

-static const struct of_device_id iproc_pcie_of_match_table[] = {
- { .compatible = "brcm,iproc-pcie", },
- { /* sentinel */ }
-};
-MODULE_DEVICE_TABLE(of, iproc_pcie_of_match_table);
-
static struct platform_driver iproc_pcie_pltfm_driver = {
.driver = {
.name = "iproc-pcie",
diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index eac719a..24d5b62 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -30,20 +30,16 @@

#include "pcie-iproc.h"

-#define CLK_CONTROL_OFFSET 0x000
#define EP_PERST_SOURCE_SELECT_SHIFT 2
#define EP_PERST_SOURCE_SELECT BIT(EP_PERST_SOURCE_SELECT_SHIFT)
#define EP_MODE_SURVIVE_PERST_SHIFT 1
#define EP_MODE_SURVIVE_PERST BIT(EP_MODE_SURVIVE_PERST_SHIFT)
#define RC_PCIE_RST_OUTPUT_SHIFT 0
#define RC_PCIE_RST_OUTPUT BIT(RC_PCIE_RST_OUTPUT_SHIFT)
+#define PAXC_RESET_MASK 0x7f

-#define CFG_IND_ADDR_OFFSET 0x120
#define CFG_IND_ADDR_MASK 0x00001ffc

-#define CFG_IND_DATA_OFFSET 0x124
-
-#define CFG_ADDR_OFFSET 0x1f8
#define CFG_ADDR_BUS_NUM_SHIFT 20
#define CFG_ADDR_BUS_NUM_MASK 0x0ff00000
#define CFG_ADDR_DEV_NUM_SHIFT 15
@@ -55,12 +51,8 @@
#define CFG_ADDR_CFG_TYPE_SHIFT 0
#define CFG_ADDR_CFG_TYPE_MASK 0x00000003

-#define CFG_DATA_OFFSET 0x1fc
-
-#define SYS_RC_INTX_EN 0x330
#define SYS_RC_INTX_MASK 0xf

-#define PCIE_LINK_STATUS_OFFSET 0xf0c
#define PCIE_PHYLINKUP_SHIFT 3
#define PCIE_PHYLINKUP BIT(PCIE_PHYLINKUP_SHIFT)
#define PCIE_DL_ACTIVE_SHIFT 2
@@ -71,12 +63,54 @@
#define OARR_SIZE_CFG_SHIFT 1
#define OARR_SIZE_CFG BIT(OARR_SIZE_CFG_SHIFT)

-#define OARR_LO(window) (0xd20 + (window) * 8)
-#define OARR_HI(window) (0xd24 + (window) * 8)
-#define OMAP_LO(window) (0xd40 + (window) * 8)
-#define OMAP_HI(window) (0xd44 + (window) * 8)
-
#define MAX_NUM_OB_WINDOWS 2
+#define MAX_NUM_PAXC_PF 4
+
+#define IPROC_PCIE_REG_INVALID 0xffff
+
+enum iproc_pcie_reg {
+ IPROC_PCIE_CLK_CTRL = 0,
+ IPROC_PCIE_CFG_IND_ADDR,
+ IPROC_PCIE_CFG_IND_DATA,
+ IPROC_PCIE_CFG_ADDR,
+ IPROC_PCIE_CFG_DATA,
+ IPROC_PCIE_INTX_EN,
+ IPROC_PCIE_OARR_LO,
+ IPROC_PCIE_OARR_HI,
+ IPROC_PCIE_OMAP_LO,
+ IPROC_PCIE_OMAP_HI,
+ IPROC_PCIE_LINK_STATUS,
+};
+
+/* iProc PCIe PAXB registers */
+static const u16 iproc_pcie_reg_paxb[] = {
+ [IPROC_PCIE_CLK_CTRL] = 0x000,
+ [IPROC_PCIE_CFG_IND_ADDR] = 0x120,
+ [IPROC_PCIE_CFG_IND_DATA] = 0x124,
+ [IPROC_PCIE_CFG_ADDR] = 0x1f8,
+ [IPROC_PCIE_CFG_DATA] = 0x1fc,
+ [IPROC_PCIE_INTX_EN] = 0x330,
+ [IPROC_PCIE_OARR_LO] = 0xd20,
+ [IPROC_PCIE_OARR_HI] = 0xd24,
+ [IPROC_PCIE_OMAP_LO] = 0xd40,
+ [IPROC_PCIE_OMAP_HI] = 0xd44,
+ [IPROC_PCIE_LINK_STATUS] = 0xf0c,
+};
+
+/* iProc PCIe PAXC v1 registers */
+static const u16 iproc_pcie_reg_paxc[] = {
+ [IPROC_PCIE_CLK_CTRL] = 0x000,
+ [IPROC_PCIE_CFG_IND_ADDR] = 0x1f0,
+ [IPROC_PCIE_CFG_IND_DATA] = 0x1f4,
+ [IPROC_PCIE_CFG_ADDR] = 0x1f8,
+ [IPROC_PCIE_CFG_DATA] = 0x1fc,
+ [IPROC_PCIE_INTX_EN] = IPROC_PCIE_REG_INVALID,
+ [IPROC_PCIE_OARR_LO] = IPROC_PCIE_REG_INVALID,
+ [IPROC_PCIE_OARR_HI] = IPROC_PCIE_REG_INVALID,
+ [IPROC_PCIE_OMAP_LO] = IPROC_PCIE_REG_INVALID,
+ [IPROC_PCIE_OMAP_HI] = IPROC_PCIE_REG_INVALID,
+ [IPROC_PCIE_LINK_STATUS] = IPROC_PCIE_REG_INVALID,
+};

static inline struct iproc_pcie *iproc_data(struct pci_bus *bus)
{
@@ -91,6 +125,65 @@ static inline struct iproc_pcie *iproc_data(struct pci_bus *bus)
return pcie;
}

+static inline bool iproc_pcie_reg_is_invalid(u16 reg_offset)
+{
+ return !!(reg_offset == IPROC_PCIE_REG_INVALID);
+}
+
+static inline u16 iproc_pcie_reg_offset(struct iproc_pcie *pcie,
+ enum iproc_pcie_reg reg)
+{
+ return pcie->reg_offsets[reg];
+}
+
+static inline u32 iproc_pcie_read_reg(struct iproc_pcie *pcie,
+ enum iproc_pcie_reg reg)
+{
+ u16 offset = iproc_pcie_reg_offset(pcie, reg);
+
+ if (iproc_pcie_reg_is_invalid(offset))
+ return 0;
+
+ return readl(pcie->base + offset);
+}
+
+static inline void iproc_pcie_write_reg(struct iproc_pcie *pcie,
+ enum iproc_pcie_reg reg, u32 val)
+{
+ u16 offset = iproc_pcie_reg_offset(pcie, reg);
+
+ if (iproc_pcie_reg_is_invalid(offset))
+ return;
+
+ writel(val, pcie->base + offset);
+}
+
+static inline void iproc_pcie_ob_write(struct iproc_pcie *pcie,
+ enum iproc_pcie_reg reg,
+ unsigned window, u32 val)
+{
+ u16 offset = iproc_pcie_reg_offset(pcie, reg);
+
+ if (iproc_pcie_reg_is_invalid(offset))
+ return;
+
+ writel(val, pcie->base + offset + (window * 8));
+}
+
+static inline bool iproc_pcie_device_is_valid(struct iproc_pcie *pcie,
+ unsigned int slot,
+ unsigned int fn)
+{
+ if (slot > 0)
+ return false;
+
+ /* PAXC can only support limited number of functions */
+ if (pcie->type == IPROC_PCIE_PAXC && fn >= MAX_NUM_PAXC_PF)
+ return false;
+
+ return true;
+}
+
/**
* Note access to the configuration registers are protected at the higher layer
* by 'pci_lock' in drivers/pci/access.c
@@ -104,28 +197,34 @@ static void __iomem *iproc_pcie_map_cfg_bus(struct pci_bus *bus,
unsigned fn = PCI_FUNC(devfn);
unsigned busno = bus->number;
u32 val;
+ u16 offset;
+
+ if (!iproc_pcie_device_is_valid(pcie, slot, fn))
+ return NULL;

/* root complex access */
if (busno == 0) {
- if (slot >= 1)
+ iproc_pcie_write_reg(pcie, IPROC_PCIE_CFG_IND_ADDR,
+ where & CFG_IND_ADDR_MASK);
+ offset = iproc_pcie_reg_offset(pcie, IPROC_PCIE_CFG_IND_DATA);
+ if (iproc_pcie_reg_is_invalid(offset))
return NULL;
- writel(where & CFG_IND_ADDR_MASK,
- pcie->base + CFG_IND_ADDR_OFFSET);
- return (pcie->base + CFG_IND_DATA_OFFSET);
+ else
+ return (pcie->base + offset);
}

- if (fn > 1)
- return NULL;
-
/* EP device access */
val = (busno << CFG_ADDR_BUS_NUM_SHIFT) |
(slot << CFG_ADDR_DEV_NUM_SHIFT) |
(fn << CFG_ADDR_FUNC_NUM_SHIFT) |
(where & CFG_ADDR_REG_NUM_MASK) |
(1 & CFG_ADDR_CFG_TYPE_MASK);
- writel(val, pcie->base + CFG_ADDR_OFFSET);
-
- return (pcie->base + CFG_DATA_OFFSET);
+ iproc_pcie_write_reg(pcie, IPROC_PCIE_CFG_ADDR, val);
+ offset = iproc_pcie_reg_offset(pcie, IPROC_PCIE_CFG_DATA);
+ if (iproc_pcie_reg_is_invalid(offset))
+ return NULL;
+ else
+ return (pcie->base + offset);
}

static struct pci_ops iproc_pcie_ops = {
@@ -138,18 +237,29 @@ static void iproc_pcie_reset(struct iproc_pcie *pcie)
{
u32 val;

+ if (pcie->type == IPROC_PCIE_PAXC) {
+ val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
+ val &= ~PAXC_RESET_MASK;
+ iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
+ udelay(100);
+ val |= PAXC_RESET_MASK;
+ iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
+ udelay(100);
+ return;
+ }
+
/*
* Select perst_b signal as reset source. Put the device into reset,
* and then bring it out of reset
*/
- val = readl(pcie->base + CLK_CONTROL_OFFSET);
+ val = iproc_pcie_read_reg(pcie, IPROC_PCIE_CLK_CTRL);
val &= ~EP_PERST_SOURCE_SELECT & ~EP_MODE_SURVIVE_PERST &
~RC_PCIE_RST_OUTPUT;
- writel(val, pcie->base + CLK_CONTROL_OFFSET);
+ iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
udelay(250);

val |= RC_PCIE_RST_OUTPUT;
- writel(val, pcie->base + CLK_CONTROL_OFFSET);
+ iproc_pcie_write_reg(pcie, IPROC_PCIE_CLK_CTRL, val);
msleep(100);
}

@@ -160,7 +270,14 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie, struct pci_bus *bus)
u16 pos, link_status;
bool link_is_active = false;

- val = readl(pcie->base + PCIE_LINK_STATUS_OFFSET);
+ /*
+ * PAXC connects to emulated endpoint devices directly and does not
+ * have a Serdes. Therefore skip the link detection logic here
+ */
+ if (pcie->type == IPROC_PCIE_PAXC)
+ return 0;
+
+ val = iproc_pcie_read_reg(pcie, IPROC_PCIE_LINK_STATUS);
if (!(val & PCIE_PHYLINKUP) || !(val & PCIE_DL_ACTIVE)) {
dev_err(pcie->dev, "PHY or data link is INACTIVE!\n");
return -ENODEV;
@@ -221,7 +338,7 @@ static int iproc_pcie_check_link(struct iproc_pcie *pcie, struct pci_bus *bus)

static void iproc_pcie_enable(struct iproc_pcie *pcie)
{
- writel(SYS_RC_INTX_MASK, pcie->base + SYS_RC_INTX_EN);
+ iproc_pcie_write_reg(pcie, IPROC_PCIE_INTX_EN, SYS_RC_INTX_MASK);
}

/**
@@ -272,11 +389,15 @@ static int iproc_pcie_setup_ob(struct iproc_pcie *pcie, u64 axi_addr,
axi_addr -= ob->axi_offset;

for (i = 0; i < MAX_NUM_OB_WINDOWS; i++) {
- writel(lower_32_bits(axi_addr) | OARR_VALID |
- (ob->set_oarr_size ? 1 : 0), pcie->base + OARR_LO(i));
- writel(upper_32_bits(axi_addr), pcie->base + OARR_HI(i));
- writel(lower_32_bits(pci_addr), pcie->base + OMAP_LO(i));
- writel(upper_32_bits(pci_addr), pcie->base + OMAP_HI(i));
+ iproc_pcie_ob_write(pcie, IPROC_PCIE_OARR_LO, i,
+ lower_32_bits(axi_addr) | OARR_VALID |
+ (ob->set_oarr_size ? 1 : 0));
+ iproc_pcie_ob_write(pcie, IPROC_PCIE_OARR_HI, i,
+ upper_32_bits(axi_addr));
+ iproc_pcie_ob_write(pcie, IPROC_PCIE_OMAP_LO, i,
+ lower_32_bits(pci_addr));
+ iproc_pcie_ob_write(pcie, IPROC_PCIE_OMAP_HI, i,
+ upper_32_bits(pci_addr));

size -= ob->window_size;
if (size == 0)
@@ -340,6 +461,19 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
goto err_exit_phy;
}

+ switch (pcie->type) {
+ case IPROC_PCIE_PAXB:
+ pcie->reg_offsets = iproc_pcie_reg_paxb;
+ break;
+ case IPROC_PCIE_PAXC:
+ pcie->reg_offsets = iproc_pcie_reg_paxc;
+ break;
+ default:
+ dev_err(pcie->dev, "incompatible iProc PCIe interface\n");
+ ret = -EINVAL;
+ goto err_power_off_phy;
+ }
+
iproc_pcie_reset(pcie);

if (pcie->need_ob_cfg) {
diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
index d3dc940..051b651 100644
--- a/drivers/pci/host/pcie-iproc.h
+++ b/drivers/pci/host/pcie-iproc.h
@@ -15,6 +15,20 @@
#define _PCIE_IPROC_H

/**
+ * iProc PCIe interface type
+ *
+ * PAXB is the wrapper used in root complex that can be connected to an
+ * external endpoint device
+ *
+ * PAXC is the wrapper used in root complex dedicated for internal emulated
+ * endpoint devices
+ */
+enum iproc_pcie_type {
+ IPROC_PCIE_PAXB = 0,
+ IPROC_PCIE_PAXC,
+};
+
+/**
* iProc PCIe outbound mapping
* @set_oarr_size: indicates the OARR size bit needs to be set
* @axi_offset: offset from the AXI address to the internal address used by
@@ -29,7 +43,10 @@ struct iproc_pcie_ob {

/**
* iProc PCIe device
+ *
* @dev: pointer to device data structure
+ * @type: iProc PCIe interface type
+ * @reg_offsets: register offsets
* @base: PCIe host controller I/O register base
* @sysdata: Per PCI controller data (ARM-specific)
* @root_bus: pointer to root bus
@@ -41,6 +58,8 @@ struct iproc_pcie_ob {
*/
struct iproc_pcie {
struct device *dev;
+ enum iproc_pcie_type type;
+ const u16 *reg_offsets;
void __iomem *base;
#ifdef CONFIG_ARM
struct pci_sys_data sysdata;
--
1.9.1

2015-11-24 23:05:39

by Ray Jui

[permalink] [raw]
Subject: [PATCH v2 3/5] PCI: iproc: Add iProc PCIe MSI device tree binding

This patch updates the iProc PCIe device tree bindings with added
binding information for MSI

Signed-off-by: Ray Jui <[email protected]>
Reviewed-by: Anup Patel <[email protected]>
Reviewed-by: Vikram Prakash <[email protected]>
Reviewed-by: Scott Branden <[email protected]>
---
.../devicetree/bindings/pci/brcm,iproc-pcie.txt | 39 ++++++++++++++++++++++
1 file changed, 39 insertions(+)

diff --git a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
index 06eae0f..6ee1616 100644
--- a/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
+++ b/Documentation/devicetree/bindings/pci/brcm,iproc-pcie.txt
@@ -35,6 +35,32 @@ Optional:
- brcm,pcie-ob-oarr-size: Some iProc SoCs need the OARR size bit to be set to
increase the outbound window size

+MSI support (optional):
+
+For older platforms without MSI integrated in the GIC, iProc PCIe core provides
+an event queue based MSI support. The iProc MSI uses host memories to store
+MSI posted writes and event queues
+
+- msi-parent: Link to the device node of the MSI controller. On newer iProc
+platforms, the MSI controller may be gicv2m or gicv3-its. On older iProc
+platforms without MSI support in its interrupt controller, one may use the
+event queue based MSI support integrated within the iProc PCIe core
+
+When the iProc event queue based MSI is used, one needs to define the
+following properties in the MSI device node:
+- compatible: Must be "brcm,iproc-msi"
+- msi-controller: claims itself as an MSI controller
+- interrupt-parent: Link to its parent interrupt device
+- interrupts: List of interrupt IDs from its parent interrupt device
+
+Optional properties:
+- brcm,num-eq-region: Required number of 4K aligned memory regions for MSI
+event queue. If not specified, it's defaulted to 1
+- brcm,num-msi-msg-region: Required number of 4K aligned memory regions for MSI
+posted writes. If not specified, it's defaulted to 1
+- brcm,pcie-msi-inten: Needs to be present for some older iProc platforms that
+require the interrupt enable registers to be set explicitly to enable MSI
+
Example:
pcie0: pcie@18012000 {
compatible = "brcm,iproc-pcie";
@@ -61,6 +87,19 @@ Example:
brcm,pcie-ob-oarr-size;
brcm,pcie-ob-axi-offset = <0x00000000>;
brcm,pcie-ob-window-size = <256>;
+
+ msi-parent = <&msi0>;
+
+ /* iProc event queue based MSI */
+ msi0: msi@18012000 {
+ compatible = "brcm,iproc-msi";
+ msi-controller;
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_SPI 96 IRQ_TYPE_NONE>,
+ <GIC_SPI 97 IRQ_TYPE_NONE>,
+ <GIC_SPI 98 IRQ_TYPE_NONE>,
+ <GIC_SPI 99 IRQ_TYPE_NONE>,
+ };
};

pcie1: pcie@18013000 {
--
1.9.1

2015-11-24 23:04:57

by Ray Jui

[permalink] [raw]
Subject: [PATCH v2 4/5] PCI: iproc: Add iProc PCIe MSI support

This patch adds PCIe MSI support for both PAXB and PAXC interfaces on
all iProc based platforms. The patch follows the latest trend in the
kernel to use MSI domain based implementation

This iProc event queue based MSI support should not be used with newer
platforms with integrated MSI support in the GIC (e.g., giv2m or
gicv3-its)

Signed-off-by: Ray Jui <[email protected]>
Reviewed-by: Anup Patel <[email protected]>
Reviewed-by: Vikram Prakash <[email protected]>
Reviewed-by: Scott Branden <[email protected]>
---
drivers/pci/host/Kconfig | 9 +
drivers/pci/host/Makefile | 1 +
drivers/pci/host/pcie-iproc-msi.c | 662 ++++++++++++++++++++++++++++++++++++++
drivers/pci/host/pcie-iproc.c | 26 ++
drivers/pci/host/pcie-iproc.h | 21 +-
5 files changed, 717 insertions(+), 2 deletions(-)
create mode 100644 drivers/pci/host/pcie-iproc-msi.c

diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
index f131ba9..972e906 100644
--- a/drivers/pci/host/Kconfig
+++ b/drivers/pci/host/Kconfig
@@ -126,6 +126,15 @@ config PCIE_IPROC
iProc family of SoCs. An appropriate bus interface driver also needs
to be enabled

+config PCIE_IPROC_MSI
+ bool "Broadcom iProc PCIe MSI support"
+ depends on ARCH_BCM_IPROC && PCI_MSI
+ select PCI_MSI_IRQ_DOMAIN
+ default ARCH_BCM_IPROC
+ help
+ Say Y here if you want to enable MSI support for Broadcom's iProc
+ PCIe controller
+
config PCIE_IPROC_PLATFORM
tristate "Broadcom iProc PCIe platform bus driver"
depends on ARCH_BCM_IPROC || (ARM && COMPILE_TEST)
diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
index 9d4d3c6..0e4e95e 100644
--- a/drivers/pci/host/Makefile
+++ b/drivers/pci/host/Makefile
@@ -15,6 +15,7 @@ obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
+obj-$(CONFIG_PCIE_IPROC_MSI) += pcie-iproc-msi.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-iproc-msi.c b/drivers/pci/host/pcie-iproc-msi.c
new file mode 100644
index 0000000..afc54c2
--- /dev/null
+++ b/drivers/pci/host/pcie-iproc-msi.c
@@ -0,0 +1,662 @@
+/*
+ * Copyright (C) 2015 Broadcom Corporation
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/msi.h>
+#include <linux/of_irq.h>
+#include <linux/of_pci.h>
+#include <linux/pci.h>
+
+#include "pcie-iproc.h"
+
+#define IPROC_MSI_INTR_EN_SHIFT 11
+#define IPROC_MSI_INTR_EN BIT(IPROC_MSI_INTR_EN_SHIFT)
+#define IPROC_MSI_INT_N_EVENT_SHIFT 1
+#define IPROC_MSI_INT_N_EVENT BIT(IPROC_MSI_INT_N_EVENT_SHIFT)
+#define IPROC_MSI_EQ_EN_SHIFT 0
+#define IPROC_MSI_EQ_EN BIT(IPROC_MSI_EQ_EN_SHIFT)
+
+#define IPROC_MSI_EQ_MASK 0x3f
+
+/* max number of GIC interrupts */
+#define NR_HW_IRQS 6
+
+/* number of entries in each event queue */
+#define EQ_LEN 64
+
+/* size of each event queue memory region */
+#define EQ_MEM_REGION_SIZE SZ_4K
+
+/* size of each MSI message memory region */
+#define MSI_MEM_REGION_SIZE SZ_4K
+
+enum iproc_msi_reg {
+ IPROC_MSI_EQ_PAGE = 0,
+ IPROC_MSI_EQ_PAGE_UPPER,
+ IPROC_MSI_PAGE,
+ IPROC_MSI_PAGE_UPPER,
+ IPROC_MSI_CTRL,
+ IPROC_MSI_EQ_HEAD,
+ IPROC_MSI_EQ_TAIL,
+ IPROC_MSI_INTS_EN,
+ IPROC_MSI_REG_SIZE,
+};
+
+struct iproc_msi;
+
+/**
+ * iProc MSI group
+ *
+ * One MSI group is allocated per GIC interrupt, serviced by one iProc MSI
+ * event queue
+ *
+ * @msi: pointer to iProc MSI data
+ * @gic_irq: GIC interrupt
+ * @eq: Event queue number
+ */
+struct iproc_msi_grp {
+ struct iproc_msi *msi;
+ int gic_irq;
+ unsigned int eq;
+};
+
+/**
+ * iProc event queue based MSI
+ *
+ * Only meant to be used on platforms without MSI support integrated into the
+ * GIC
+ *
+ * @pcie: pointer to iProc PCIe data
+ * @reg_offsets: MSI register offsets
+ * @grps: MSI groups
+ * @nr_irqs: number of total interrupts connected to GIC
+ * @nr_cpus: number of toal CPUs
+ * @has_inten_reg: indicates the MSI interrupt enable register needs to be
+ * set explicitly (required for some legacy platforms)
+ * @bitmap: MSI vector bitmap
+ * @bitmap_lock: lock to protect access to the MSI bitmap
+ * @nr_msi_vecs: total number of MSI vectors
+ * @inner_domain: inner IRQ domain
+ * @msi_domain: MSI IRQ domain
+ * @nr_eq_region: required number of 4K aligned memory region for MSI event
+ * queues
+ * @nr_msi_region: required number of 4K aligned memory region for MSI posted
+ * writes
+ * @eq_cpu: pointer to allocated memory region for MSI event queues
+ * @eq_dma: DMA address of MSI event queues
+ * @msi_cpu: pointer to allocated memory region for MSI posted writes
+ * @msi_dma: DMA address of MSI posted writes
+ */
+struct iproc_msi {
+ struct iproc_pcie *pcie;
+ const u16 (*reg_offsets)[IPROC_MSI_REG_SIZE];
+ struct iproc_msi_grp *grps;
+ int nr_irqs;
+ int nr_cpus;
+ bool has_inten_reg;
+ unsigned long *bitmap;
+ struct mutex bitmap_lock;
+ unsigned int nr_msi_vecs;
+ struct irq_domain *inner_domain;
+ struct irq_domain *msi_domain;
+ unsigned int nr_eq_region;
+ unsigned int nr_msi_region;
+ void *eq_cpu;
+ dma_addr_t eq_dma;
+ void *msi_cpu;
+ dma_addr_t msi_dma;
+};
+
+static const u16 iproc_msi_reg_paxb[NR_HW_IRQS][IPROC_MSI_REG_SIZE] = {
+ { 0x200, 0x2c0, 0x204, 0x2c4, 0x210, 0x250, 0x254, 0x208 },
+ { 0x200, 0x2c0, 0x204, 0x2c4, 0x214, 0x258, 0x25c, 0x208 },
+ { 0x200, 0x2c0, 0x204, 0x2c4, 0x218, 0x260, 0x264, 0x208 },
+ { 0x200, 0x2c0, 0x204, 0x2c4, 0x21c, 0x268, 0x26c, 0x208 },
+ { 0x200, 0x2c0, 0x204, 0x2c4, 0x220, 0x270, 0x274, 0x208 },
+ { 0x200, 0x2c0, 0x204, 0x2c4, 0x224, 0x278, 0x27c, 0x208 },
+};
+
+static const u16 iproc_msi_reg_paxc[NR_HW_IRQS][IPROC_MSI_REG_SIZE] = {
+ { 0xc00, 0xc04, 0xc08, 0xc0c, 0xc40, 0xc50, 0xc60 },
+ { 0xc10, 0xc14, 0xc18, 0xc1c, 0xc44, 0xc54, 0xc64 },
+ { 0xc20, 0xc24, 0xc28, 0xc2c, 0xc48, 0xc58, 0xc68 },
+ { 0xc30, 0xc34, 0xc38, 0xc3c, 0xc4c, 0xc5c, 0xc6c },
+};
+
+static inline u32 iproc_msi_read_reg(struct iproc_msi *msi,
+ enum iproc_msi_reg reg,
+ unsigned int eq)
+{
+ struct iproc_pcie *pcie = msi->pcie;
+
+ return readl_relaxed(pcie->base + msi->reg_offsets[eq][reg]);
+}
+
+static inline void iproc_msi_write_reg(struct iproc_msi *msi,
+ enum iproc_msi_reg reg,
+ int eq, u32 val)
+{
+ struct iproc_pcie *pcie = msi->pcie;
+
+ writel_relaxed(val, pcie->base + msi->reg_offsets[eq][reg]);
+}
+
+static inline bool iproc_msi_has_mult_regions(struct iproc_msi *msi)
+{
+ return ((msi->nr_msi_region > 1) ? true : false);
+}
+
+static inline bool iproc_eq_has_mult_regions(struct iproc_msi *msi)
+{
+ return ((msi->nr_eq_region > 1) ? true : false);
+}
+
+static struct irq_chip iproc_msi_irq_chip = {
+ .name = "iProc-MSI",
+};
+
+static struct msi_domain_info iproc_msi_domain_info = {
+ .flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
+ MSI_FLAG_PCI_MSIX,
+ .chip = &iproc_msi_irq_chip,
+};
+
+/*
+ * In iProc PCIe core, each MSI group is serviced by a GIC interrupt and a
+ * dedicated event queue. Each MSI group can support up to 64 MSI vectors
+ *
+ * The number of MSI groups varies between different iProc SoCs. The total
+ * number of CPU cores also varies. To support MSI IRQ affinity, we
+ * distribute GIC interrupts across all available CPUs. MSI vector is moved
+ * from one GIC interrupt to another to steer to the target CPU
+ *
+ * Assuming:
+ * - the number of MSI groups is M
+ * - the number of CPU cores is N
+ * - M is always a multiple of N
+ *
+ * Total number of raw MSI vectors = M * 64
+ * Total number of supported MSI vectors = (M * 64) / N
+ */
+static inline int hwirq_to_cpu(struct iproc_msi *msi, unsigned long hwirq)
+{
+ return (hwirq % msi->nr_cpus);
+}
+
+static inline unsigned long hwirq_to_canonical_hwirq(struct iproc_msi *msi,
+ unsigned long hwirq)
+{
+ return (hwirq - hwirq_to_cpu(msi, hwirq));
+}
+
+static int iproc_msi_irq_set_affinity(struct irq_data *data,
+ const struct cpumask *mask, bool force)
+{
+ struct iproc_msi *msi = irq_data_get_irq_chip_data(data);
+ int target_cpu = cpumask_first(mask);
+ int curr_cpu;
+
+ curr_cpu = hwirq_to_cpu(msi, data->hwirq);
+ if (curr_cpu == target_cpu)
+ return IRQ_SET_MASK_OK_DONE;
+
+ /* steer MSI to the target CPU */
+ data->hwirq = hwirq_to_canonical_hwirq(msi, data->hwirq) + target_cpu;
+
+ return IRQ_SET_MASK_OK;
+}
+
+static inline u32 hwirq_to_group(struct iproc_msi *msi, unsigned long hwirq)
+{
+ return (hwirq % msi->nr_irqs);
+}
+
+static void iproc_msi_irq_compose_msi_msg(struct irq_data *data,
+ struct msi_msg *msg)
+{
+ struct iproc_msi *msi = irq_data_get_irq_chip_data(data);
+ dma_addr_t addr;
+ unsigned int mul;
+
+ if (iproc_msi_has_mult_regions(msi))
+ mul = MSI_MEM_REGION_SIZE;
+ else
+ mul = sizeof(u32);
+
+ addr = msi->msi_dma + hwirq_to_group(msi, data->hwirq) * mul;
+ msg->address_lo = lower_32_bits(addr);
+ msg->address_hi = upper_32_bits(addr);
+ msg->data = data->hwirq;
+}
+
+static struct irq_chip iproc_msi_bottom_irq_chip = {
+ .name = "MSI",
+ .irq_set_affinity = iproc_msi_irq_set_affinity,
+ .irq_compose_msi_msg = iproc_msi_irq_compose_msi_msg,
+};
+
+static int iproc_msi_irq_domain_alloc(struct irq_domain *domain,
+ unsigned int virq, unsigned int nr_irqs,
+ void *args)
+{
+ struct iproc_msi *msi = domain->host_data;
+ int hwirq;
+
+ mutex_lock(&msi->bitmap_lock);
+
+ /* allocate 'nr_cpus' number of MSI vectors each time */
+ hwirq = bitmap_find_next_zero_area(msi->bitmap, msi->nr_msi_vecs, 0,
+ msi->nr_cpus, 0);
+ if (hwirq < msi->nr_msi_vecs)
+ bitmap_set(msi->bitmap, hwirq, msi->nr_cpus);
+ else
+ return -ENOSPC;
+
+ mutex_unlock(&msi->bitmap_lock);
+
+ irq_domain_set_info(domain, virq, hwirq, &iproc_msi_bottom_irq_chip,
+ domain->host_data, handle_simple_irq, NULL, NULL);
+
+ return 0;
+}
+
+static void iproc_msi_irq_domain_free(struct irq_domain *domain,
+ unsigned int virq, unsigned int nr_irqs)
+{
+ struct irq_data *data = irq_domain_get_irq_data(domain, virq);
+ struct iproc_msi *msi = irq_data_get_irq_chip_data(data);
+ unsigned int hwirq;
+
+ mutex_lock(&msi->bitmap_lock);
+
+ hwirq = hwirq_to_canonical_hwirq(msi, data->hwirq);
+ bitmap_clear(msi->bitmap, hwirq, msi->nr_cpus);
+
+ mutex_unlock(&msi->bitmap_lock);
+
+ irq_domain_free_irqs_parent(domain, virq, nr_irqs);
+}
+
+static const struct irq_domain_ops msi_domain_ops = {
+ .alloc = iproc_msi_irq_domain_alloc,
+ .free = iproc_msi_irq_domain_free,
+};
+
+static inline u32 decode_msi_hwirq(struct iproc_msi *msi, u32 eq, u32 head)
+{
+ u32 *msg, hwirq;
+ unsigned int offs;
+
+ if (iproc_eq_has_mult_regions(msi))
+ offs = eq * EQ_MEM_REGION_SIZE;
+ else
+ offs = eq * EQ_LEN * sizeof(u32);
+
+ offs += head * sizeof(u32);
+ msg = (u32 *)(msi->eq_cpu + offs);
+ hwirq = *msg & IPROC_MSI_EQ_MASK;
+
+ /*
+ * Since we have multiple hwirq mapped to a single MSI vector,
+ * now we need to derive the hwirq at CPU0. It can then be used to
+ * mapped back to virq
+ */
+ return hwirq_to_canonical_hwirq(msi, hwirq);
+}
+
+static void iproc_msi_handler(struct irq_desc *desc)
+{
+ struct irq_chip *chip = irq_desc_get_chip(desc);
+ struct iproc_msi_grp *grp;
+ struct iproc_msi *msi;
+ struct iproc_pcie *pcie;
+ u32 eq, head, tail, nr_events;
+ unsigned long hwirq;
+ int virq;
+
+ chained_irq_enter(chip, desc);
+
+ grp = irq_desc_get_handler_data(desc);
+ msi = grp->msi;
+ pcie = msi->pcie;
+ eq = grp->eq;
+
+ /*
+ * iProc MSI event queue is tracked by head and tail pointers. Head
+ * pointer indicates the next entry to be processed by SW in the
+ * queue. Entries between head and tail pointers contain valid MSI
+ * data to be processed
+ */
+ head = iproc_msi_read_reg(msi, IPROC_MSI_EQ_HEAD,
+ eq) & IPROC_MSI_EQ_MASK;
+ do {
+ tail = iproc_msi_read_reg(msi, IPROC_MSI_EQ_TAIL,
+ eq) & IPROC_MSI_EQ_MASK;
+
+ /*
+ * Figure out total number of events (MSI data) to be
+ * processed
+ */
+ nr_events = (tail < head) ?
+ (EQ_LEN - (head - tail)) : (tail - head);
+ if (!nr_events)
+ break;
+
+ /* process all outstanding events */
+ while (nr_events--) {
+ hwirq = decode_msi_hwirq(msi, eq, head);
+ virq = irq_find_mapping(msi->inner_domain, hwirq);
+ generic_handle_irq(virq);
+
+ head++;
+ head %= EQ_LEN;
+ iproc_msi_write_reg(msi, IPROC_MSI_EQ_HEAD, eq, head);
+ }
+
+ /*
+ * Now go read the tail pointer again to see if there are new
+ * oustanding events that came in during the above window
+ */
+ } while (true);
+
+ chained_irq_exit(chip, desc);
+}
+
+static void iproc_msi_enable(struct iproc_msi *msi)
+{
+ int i, eq;
+ u32 val;
+
+ /* program memory region for each event queue */
+ for (i = 0; i < msi->nr_eq_region; i++) {
+ dma_addr_t addr = msi->eq_dma + (i * EQ_MEM_REGION_SIZE);
+
+ iproc_msi_write_reg(msi, IPROC_MSI_EQ_PAGE, i,
+ lower_32_bits(addr));
+ iproc_msi_write_reg(msi, IPROC_MSI_EQ_PAGE_UPPER, i,
+ upper_32_bits(addr));
+ }
+
+ /* program memory region for MSI posted writes */
+ for (i = 0; i < msi->nr_msi_region; i++) {
+ dma_addr_t addr = msi->msi_dma + (i * MSI_MEM_REGION_SIZE);
+
+ iproc_msi_write_reg(msi, IPROC_MSI_PAGE, i,
+ lower_32_bits(addr));
+ iproc_msi_write_reg(msi, IPROC_MSI_PAGE_UPPER, i,
+ upper_32_bits(addr));
+ }
+
+ for (eq = 0; eq < msi->nr_irqs; eq++) {
+ /* enable MSI event queue */
+ val = IPROC_MSI_INTR_EN | IPROC_MSI_INT_N_EVENT |
+ IPROC_MSI_EQ_EN;
+ iproc_msi_write_reg(msi, IPROC_MSI_CTRL, eq, val);
+
+ /*
+ * Some legacy platforms require the MSI interrupt enable
+ * register to be set explicitly
+ */
+ if (msi->has_inten_reg) {
+ val = iproc_msi_read_reg(msi, IPROC_MSI_INTS_EN, eq);
+ val |= BIT(eq);
+ iproc_msi_write_reg(msi, IPROC_MSI_INTS_EN, eq, val);
+ }
+ }
+}
+
+static void iproc_msi_disable(struct iproc_msi *msi)
+{
+ u32 eq, val;
+
+ for (eq = 0; eq < msi->nr_irqs; eq++) {
+ if (msi->has_inten_reg) {
+ val = iproc_msi_read_reg(msi, IPROC_MSI_INTS_EN, eq);
+ val &= ~BIT(eq);
+ iproc_msi_write_reg(msi, IPROC_MSI_INTS_EN, eq, val);
+ }
+
+ val = iproc_msi_read_reg(msi, IPROC_MSI_CTRL, eq);
+ val &= ~(IPROC_MSI_INTR_EN | IPROC_MSI_INT_N_EVENT |
+ IPROC_MSI_EQ_EN);
+ iproc_msi_write_reg(msi, IPROC_MSI_CTRL, eq, val);
+ }
+}
+
+static int iproc_msi_alloc_domains(struct device_node *node,
+ struct iproc_msi *msi)
+{
+ msi->inner_domain = irq_domain_add_linear(NULL, msi->nr_msi_vecs,
+ &msi_domain_ops, msi);
+ if (!msi->inner_domain)
+ return -ENOMEM;
+
+ msi->msi_domain = pci_msi_create_irq_domain(of_node_to_fwnode(node),
+ &iproc_msi_domain_info,
+ msi->inner_domain);
+ if (!msi->msi_domain) {
+ irq_domain_remove(msi->inner_domain);
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
+static void iproc_msi_free_domains(struct iproc_msi *msi)
+{
+ if (msi->msi_domain)
+ irq_domain_remove(msi->msi_domain);
+
+ if (msi->inner_domain)
+ irq_domain_remove(msi->inner_domain);
+}
+
+static int iproc_msi_irq_setup(struct iproc_msi *msi, unsigned int cpu)
+{
+ int i, ret;
+ cpumask_var_t mask;
+ struct iproc_pcie *pcie = msi->pcie;
+
+ for (i = cpu; i < msi->nr_irqs; i += msi->nr_cpus) {
+ irq_set_chained_handler_and_data(msi->grps[i].gic_irq,
+ iproc_msi_handler,
+ &msi->grps[i]);
+ /* dedicate GIC interrupt to each CPU core */
+ if (alloc_cpumask_var(&mask, GFP_KERNEL)) {
+ cpumask_clear(mask);
+ cpumask_set_cpu(cpu, mask);
+ ret = irq_set_affinity(msi->grps[i].gic_irq, mask);
+ if (ret)
+ dev_err(pcie->dev,
+ "failed to set affinity for IRQ%d\n",
+ msi->grps[i].gic_irq);
+ free_cpumask_var(mask);
+ } else {
+ dev_err(pcie->dev, "failed to alloc CPU mask\n");
+ ret = -EINVAL;
+ }
+
+ if (ret) {
+ irq_set_chained_handler_and_data(msi->grps[i].gic_irq,
+ NULL, NULL);
+ return ret;
+ }
+ }
+
+ return 0;
+}
+
+static void iproc_msi_irq_free(struct iproc_msi *msi, unsigned int cpu)
+{
+ int i;
+
+ for (i = cpu; i < msi->nr_irqs; i += msi->nr_cpus) {
+ irq_set_chained_handler_and_data(msi->grps[i].gic_irq,
+ NULL, NULL);
+ }
+}
+
+int iproc_msi_init(struct iproc_pcie *pcie, struct device_node *node)
+{
+ struct iproc_msi *msi;
+ int i, ret;
+ unsigned int cpu;
+
+ if (!of_device_is_compatible(node, "brcm,iproc-msi"))
+ return -ENODEV;
+
+ if (!of_find_property(node, "msi-controller", NULL))
+ return -ENODEV;
+
+ if (pcie->msi)
+ return -EBUSY;
+
+ msi = devm_kzalloc(pcie->dev, sizeof(*msi), GFP_KERNEL);
+ if (!msi)
+ return -ENOMEM;
+
+ msi->pcie = pcie;
+ pcie->msi = msi;
+ mutex_init(&msi->bitmap_lock);
+ msi->nr_cpus = num_possible_cpus();
+
+ switch (pcie->type) {
+ case IPROC_PCIE_PAXB:
+ msi->reg_offsets = iproc_msi_reg_paxb;
+ break;
+ case IPROC_PCIE_PAXC:
+ msi->reg_offsets = iproc_msi_reg_paxc;
+ break;
+ default:
+ dev_err(pcie->dev, "incompatible iProc PCIe interface\n");
+ return -EINVAL;
+ }
+
+ if (of_find_property(node, "brcm,pcie-msi-inten", NULL))
+ msi->has_inten_reg = true;
+
+ ret = of_property_read_u32(node, "brcm,num-eq-region",
+ &msi->nr_eq_region);
+ if (ret || !msi->nr_eq_region)
+ msi->nr_eq_region = 1;
+
+ ret = of_property_read_u32(node, "brcm,num-msi-msg-region",
+ &msi->nr_msi_region);
+ if (ret || !msi->nr_msi_region)
+ msi->nr_msi_region = 1;
+
+ msi->nr_irqs = of_irq_count(node);
+ if (!msi->nr_irqs) {
+ dev_err(pcie->dev, "found no MSI GIC interrupt\n");
+ return -ENODEV;
+ }
+ if (msi->nr_irqs > NR_HW_IRQS) {
+ dev_warn(pcie->dev, "too many MSI GIC interrupts defined %d\n",
+ msi->nr_irqs);
+ msi->nr_irqs = NR_HW_IRQS;
+ }
+
+ msi->nr_msi_vecs = msi->nr_irqs * EQ_LEN;
+ msi->bitmap = devm_kcalloc(pcie->dev, BITS_TO_LONGS(msi->nr_msi_vecs),
+ sizeof(*msi->bitmap), GFP_KERNEL);
+ if (!msi->bitmap)
+ return -ENOMEM;
+
+ msi->grps = devm_kcalloc(pcie->dev, msi->nr_irqs, sizeof(*msi->grps),
+ GFP_KERNEL);
+ if (!msi->grps)
+ return -ENOMEM;
+
+ for (i = 0; i < msi->nr_irqs; i++) {
+ unsigned int irq = irq_of_parse_and_map(node, i);
+
+ if (!irq) {
+ dev_err(pcie->dev, "unable to parse/map interrupt\n");
+ return -ENODEV;
+ }
+ msi->grps[i].gic_irq = irq;
+ msi->grps[i].msi = msi;
+ msi->grps[i].eq = i;
+ }
+
+ /* reserve memory for MSI event queue */
+ msi->eq_cpu = dma_alloc_coherent(pcie->dev,
+ msi->nr_eq_region * EQ_MEM_REGION_SIZE,
+ &msi->eq_dma, GFP_KERNEL);
+ if (!msi->eq_cpu)
+ return -ENOMEM;
+
+ /* reserve memory for MSI posted writes */
+ msi->msi_cpu = dma_alloc_coherent(pcie->dev,
+ msi->nr_msi_region * MSI_MEM_REGION_SIZE,
+ &msi->msi_dma, GFP_KERNEL);
+ if (!msi->msi_cpu) {
+ ret = -ENOMEM;
+ goto free_eq_dma;
+ }
+
+ ret = iproc_msi_alloc_domains(node, msi);
+ if (ret) {
+ dev_err(pcie->dev, "failed to create MSI domains\n");
+ goto free_msi_dma;
+ }
+
+ for_each_online_cpu(cpu) {
+ ret = iproc_msi_irq_setup(msi, cpu);
+ if (ret)
+ goto free_msi_irq;
+ }
+
+ iproc_msi_enable(msi);
+
+ return 0;
+
+free_msi_irq:
+ for_each_online_cpu(cpu)
+ iproc_msi_irq_free(msi, cpu);
+ iproc_msi_free_domains(msi);
+
+free_msi_dma:
+ dma_free_coherent(pcie->dev, msi->nr_msi_region * MSI_MEM_REGION_SIZE,
+ msi->msi_cpu, msi->msi_dma);
+free_eq_dma:
+ dma_free_coherent(pcie->dev, msi->nr_eq_region * EQ_MEM_REGION_SIZE,
+ msi->eq_cpu, msi->eq_dma);
+ pcie->msi = NULL;
+ return ret;
+}
+EXPORT_SYMBOL(iproc_msi_init);
+
+void iproc_msi_exit(struct iproc_pcie *pcie)
+{
+ struct iproc_msi *msi = pcie->msi;
+ unsigned int cpu;
+
+ if (!msi)
+ return;
+
+ iproc_msi_disable(msi);
+
+ for_each_online_cpu(cpu)
+ iproc_msi_irq_free(msi, cpu);
+
+ iproc_msi_free_domains(msi);
+
+ dma_free_coherent(pcie->dev, msi->nr_msi_region * MSI_MEM_REGION_SIZE,
+ msi->msi_cpu, msi->msi_dma);
+ dma_free_coherent(pcie->dev, msi->nr_eq_region * EQ_MEM_REGION_SIZE,
+ msi->eq_cpu, msi->eq_dma);
+}
+EXPORT_SYMBOL(iproc_msi_exit);
diff --git a/drivers/pci/host/pcie-iproc.c b/drivers/pci/host/pcie-iproc.c
index 24d5b62..b7fda356 100644
--- a/drivers/pci/host/pcie-iproc.c
+++ b/drivers/pci/host/pcie-iproc.c
@@ -440,6 +440,26 @@ static int iproc_pcie_map_ranges(struct iproc_pcie *pcie,
return 0;
}

+static int iproc_pcie_msi_enable(struct iproc_pcie *pcie)
+{
+ struct device_node *msi_node;
+
+ msi_node = of_parse_phandle(pcie->dev->of_node, "msi-parent", 0);
+ if (!msi_node)
+ return -ENODEV;
+
+ /*
+ * If another MSI controller is being used, the call below should fail
+ * but that is okay
+ */
+ return iproc_msi_init(pcie, msi_node);
+}
+
+static void iproc_pcie_msi_disable(struct iproc_pcie *pcie)
+{
+ iproc_msi_exit(pcie);
+}
+
int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)
{
int ret;
@@ -507,6 +527,10 @@ int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res)

iproc_pcie_enable(pcie);

+ if (IS_ENABLED(CONFIG_PCI_MSI))
+ if (iproc_pcie_msi_enable(pcie))
+ dev_info(pcie->dev, "not using iProc MSI\n");
+
pci_scan_child_bus(bus);
pci_assign_unassigned_bus_resources(bus);
pci_fixup_irqs(pci_common_swizzle, pcie->map_irq);
@@ -531,6 +555,8 @@ int iproc_pcie_remove(struct iproc_pcie *pcie)
pci_stop_root_bus(pcie->root_bus);
pci_remove_root_bus(pcie->root_bus);

+ iproc_pcie_msi_disable(pcie);
+
phy_power_off(pcie->phy);
phy_exit(pcie->phy);

diff --git a/drivers/pci/host/pcie-iproc.h b/drivers/pci/host/pcie-iproc.h
index 051b651..8b9bb86 100644
--- a/drivers/pci/host/pcie-iproc.h
+++ b/drivers/pci/host/pcie-iproc.h
@@ -41,6 +41,8 @@ struct iproc_pcie_ob {
resource_size_t window_size;
};

+struct iproc_msi;
+
/**
* iProc PCIe device
*
@@ -51,10 +53,10 @@ struct iproc_pcie_ob {
* @sysdata: Per PCI controller data (ARM-specific)
* @root_bus: pointer to root bus
* @phy: optional PHY device that controls the Serdes
- * @irqs: interrupt IDs
* @map_irq: function callback to map interrupts
- * @need_ob_cfg: indidates SW needs to configure the outbound mapping window
+ * @need_ob_cfg: indicates SW needs to configure the outbound mapping window
* @ob: outbound mapping parameters
+ * @msi: MSI data
*/
struct iproc_pcie {
struct device *dev;
@@ -69,9 +71,24 @@ struct iproc_pcie {
int (*map_irq)(const struct pci_dev *, u8, u8);
bool need_ob_cfg;
struct iproc_pcie_ob ob;
+ struct iproc_msi *msi;
};

int iproc_pcie_setup(struct iproc_pcie *pcie, struct list_head *res);
int iproc_pcie_remove(struct iproc_pcie *pcie);

+#ifdef CONFIG_PCI_MSI
+int iproc_msi_init(struct iproc_pcie *pcie, struct device_node *node);
+void iproc_msi_exit(struct iproc_pcie *pcie);
+#else
+static inline int iproc_msi_init(struct iproc_pcie *pcie,
+ struct device_node *node)
+{
+ return -ENODEV;
+}
+static void iproc_msi_exit(struct iproc_pcie *pcie)
+{
+}
+#endif
+
#endif /* _PCIE_IPROC_H */
--
1.9.1

2015-11-24 23:04:53

by Ray Jui

[permalink] [raw]
Subject: [PATCH v2 5/5] ARM: dts: Enable MSI support for Broadcom Cygnus

Enable MSI support for Broadcom Cygnus platforms

Signed-off-by: Ray Jui <[email protected]>
Reviewed-by: Anup Patel <[email protected]>
Reviewed-by: Pramod KUMAR <[email protected]>
Reviewed-by: Vikram Prakash <[email protected]>
Reviewed-by: Scott Branden <[email protected]>
---
arch/arm/boot/dts/bcm-cygnus.dtsi | 22 ++++++++++++++++++++++
1 file changed, 22 insertions(+)

diff --git a/arch/arm/boot/dts/bcm-cygnus.dtsi b/arch/arm/boot/dts/bcm-cygnus.dtsi
index 2778533..789b519 100644
--- a/arch/arm/boot/dts/bcm-cygnus.dtsi
+++ b/arch/arm/boot/dts/bcm-cygnus.dtsi
@@ -162,6 +162,17 @@
0x82000000 0 0x20000000 0x20000000 0 0x04000000>;

status = "disabled";
+
+ msi-parent = <&msi0>;
+ msi0: msi@18012000 {
+ compatible = "brcm,iproc-msi";
+ msi-controller;
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_SPI 96 IRQ_TYPE_NONE>,
+ <GIC_SPI 97 IRQ_TYPE_NONE>,
+ <GIC_SPI 98 IRQ_TYPE_NONE>,
+ <GIC_SPI 99 IRQ_TYPE_NONE>;
+ };
};

pcie1: pcie@18013000 {
@@ -183,6 +194,17 @@
0x82000000 0 0x40000000 0x40000000 0 0x04000000>;

status = "disabled";
+
+ msi-parent = <&msi1>;
+ msi1: msi@18013000 {
+ compatible = "brcm,iproc-msi";
+ msi-controller;
+ interrupt-parent = <&gic>;
+ interrupts = <GIC_SPI 102 IRQ_TYPE_NONE>,
+ <GIC_SPI 103 IRQ_TYPE_NONE>,
+ <GIC_SPI 104 IRQ_TYPE_NONE>,
+ <GIC_SPI 105 IRQ_TYPE_NONE>;
+ };
};

uart0: serial@18020000 {
--
1.9.1

2015-11-25 17:36:53

by Marc Zyngier

[permalink] [raw]
Subject: Re: [PATCH v2 4/5] PCI: iproc: Add iProc PCIe MSI support

On Tue, 24 Nov 2015 15:04:53 -0800
Ray Jui <[email protected]> wrote:

> This patch adds PCIe MSI support for both PAXB and PAXC interfaces on
> all iProc based platforms. The patch follows the latest trend in the
> kernel to use MSI domain based implementation
>

That's a pretty useless comment. The general trend in the kernel is to
use the most appropriate infrastructure.

> This iProc event queue based MSI support should not be used with newer
> platforms with integrated MSI support in the GIC (e.g., giv2m or
> gicv3-its)
>

I'd be more interested in some documentation explaining how the HW
works, how the various data structures are updated, and when.

> Signed-off-by: Ray Jui <[email protected]>
> Reviewed-by: Anup Patel <[email protected]>
> Reviewed-by: Vikram Prakash <[email protected]>
> Reviewed-by: Scott Branden <[email protected]>
> ---
> drivers/pci/host/Kconfig | 9 +
> drivers/pci/host/Makefile | 1 +
> drivers/pci/host/pcie-iproc-msi.c | 662 ++++++++++++++++++++++++++++++++++++++
> drivers/pci/host/pcie-iproc.c | 26 ++
> drivers/pci/host/pcie-iproc.h | 21 +-
> 5 files changed, 717 insertions(+), 2 deletions(-)
> create mode 100644 drivers/pci/host/pcie-iproc-msi.c
>
> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
> index f131ba9..972e906 100644
> --- a/drivers/pci/host/Kconfig
> +++ b/drivers/pci/host/Kconfig
> @@ -126,6 +126,15 @@ config PCIE_IPROC
> iProc family of SoCs. An appropriate bus interface driver also needs
> to be enabled
>
> +config PCIE_IPROC_MSI
> + bool "Broadcom iProc PCIe MSI support"
> + depends on ARCH_BCM_IPROC && PCI_MSI
> + select PCI_MSI_IRQ_DOMAIN
> + default ARCH_BCM_IPROC
> + help
> + Say Y here if you want to enable MSI support for Broadcom's iProc
> + PCIe controller
> +
> config PCIE_IPROC_PLATFORM
> tristate "Broadcom iProc PCIe platform bus driver"
> depends on ARCH_BCM_IPROC || (ARM && COMPILE_TEST)
> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
> index 9d4d3c6..0e4e95e 100644
> --- a/drivers/pci/host/Makefile
> +++ b/drivers/pci/host/Makefile
> @@ -15,6 +15,7 @@ obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
> obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
> obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
> obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
> +obj-$(CONFIG_PCIE_IPROC_MSI) += pcie-iproc-msi.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-iproc-msi.c b/drivers/pci/host/pcie-iproc-msi.c
> new file mode 100644
> index 0000000..afc54c2
> --- /dev/null
> +++ b/drivers/pci/host/pcie-iproc-msi.c
> @@ -0,0 +1,662 @@
> +/*
> + * Copyright (C) 2015 Broadcom Corporation
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/irqchip/chained_irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/msi.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_pci.h>
> +#include <linux/pci.h>
> +
> +#include "pcie-iproc.h"
> +
> +#define IPROC_MSI_INTR_EN_SHIFT 11
> +#define IPROC_MSI_INTR_EN BIT(IPROC_MSI_INTR_EN_SHIFT)
> +#define IPROC_MSI_INT_N_EVENT_SHIFT 1
> +#define IPROC_MSI_INT_N_EVENT BIT(IPROC_MSI_INT_N_EVENT_SHIFT)
> +#define IPROC_MSI_EQ_EN_SHIFT 0
> +#define IPROC_MSI_EQ_EN BIT(IPROC_MSI_EQ_EN_SHIFT)
> +
> +#define IPROC_MSI_EQ_MASK 0x3f
> +
> +/* max number of GIC interrupts */
> +#define NR_HW_IRQS 6
> +
> +/* number of entries in each event queue */
> +#define EQ_LEN 64
> +
> +/* size of each event queue memory region */
> +#define EQ_MEM_REGION_SIZE SZ_4K
> +
> +/* size of each MSI message memory region */
> +#define MSI_MEM_REGION_SIZE SZ_4K
> +
> +enum iproc_msi_reg {
> + IPROC_MSI_EQ_PAGE = 0,
> + IPROC_MSI_EQ_PAGE_UPPER,
> + IPROC_MSI_PAGE,
> + IPROC_MSI_PAGE_UPPER,
> + IPROC_MSI_CTRL,
> + IPROC_MSI_EQ_HEAD,
> + IPROC_MSI_EQ_TAIL,
> + IPROC_MSI_INTS_EN,
> + IPROC_MSI_REG_SIZE,
> +};
> +
> +struct iproc_msi;
> +
> +/**
> + * iProc MSI group
> + *
> + * One MSI group is allocated per GIC interrupt, serviced by one iProc MSI
> + * event queue
> + *
> + * @msi: pointer to iProc MSI data
> + * @gic_irq: GIC interrupt
> + * @eq: Event queue number
> + */
> +struct iproc_msi_grp {
> + struct iproc_msi *msi;
> + int gic_irq;
> + unsigned int eq;
> +};
> +
> +/**
> + * iProc event queue based MSI
> + *
> + * Only meant to be used on platforms without MSI support integrated into the
> + * GIC
> + *
> + * @pcie: pointer to iProc PCIe data
> + * @reg_offsets: MSI register offsets
> + * @grps: MSI groups
> + * @nr_irqs: number of total interrupts connected to GIC
> + * @nr_cpus: number of toal CPUs
> + * @has_inten_reg: indicates the MSI interrupt enable register needs to be
> + * set explicitly (required for some legacy platforms)
> + * @bitmap: MSI vector bitmap
> + * @bitmap_lock: lock to protect access to the MSI bitmap
> + * @nr_msi_vecs: total number of MSI vectors
> + * @inner_domain: inner IRQ domain
> + * @msi_domain: MSI IRQ domain
> + * @nr_eq_region: required number of 4K aligned memory region for MSI event
> + * queues
> + * @nr_msi_region: required number of 4K aligned memory region for MSI posted
> + * writes
> + * @eq_cpu: pointer to allocated memory region for MSI event queues
> + * @eq_dma: DMA address of MSI event queues
> + * @msi_cpu: pointer to allocated memory region for MSI posted writes
> + * @msi_dma: DMA address of MSI posted writes
> + */
> +struct iproc_msi {
> + struct iproc_pcie *pcie;
> + const u16 (*reg_offsets)[IPROC_MSI_REG_SIZE];
> + struct iproc_msi_grp *grps;
> + int nr_irqs;
> + int nr_cpus;
> + bool has_inten_reg;
> + unsigned long *bitmap;
> + struct mutex bitmap_lock;
> + unsigned int nr_msi_vecs;
> + struct irq_domain *inner_domain;
> + struct irq_domain *msi_domain;
> + unsigned int nr_eq_region;
> + unsigned int nr_msi_region;
> + void *eq_cpu;
> + dma_addr_t eq_dma;
> + void *msi_cpu;
> + dma_addr_t msi_dma;
> +};
> +
> +static const u16 iproc_msi_reg_paxb[NR_HW_IRQS][IPROC_MSI_REG_SIZE] = {
> + { 0x200, 0x2c0, 0x204, 0x2c4, 0x210, 0x250, 0x254, 0x208 },
> + { 0x200, 0x2c0, 0x204, 0x2c4, 0x214, 0x258, 0x25c, 0x208 },
> + { 0x200, 0x2c0, 0x204, 0x2c4, 0x218, 0x260, 0x264, 0x208 },
> + { 0x200, 0x2c0, 0x204, 0x2c4, 0x21c, 0x268, 0x26c, 0x208 },
> + { 0x200, 0x2c0, 0x204, 0x2c4, 0x220, 0x270, 0x274, 0x208 },
> + { 0x200, 0x2c0, 0x204, 0x2c4, 0x224, 0x278, 0x27c, 0x208 },
> +};
> +
> +static const u16 iproc_msi_reg_paxc[NR_HW_IRQS][IPROC_MSI_REG_SIZE] = {
> + { 0xc00, 0xc04, 0xc08, 0xc0c, 0xc40, 0xc50, 0xc60 },
> + { 0xc10, 0xc14, 0xc18, 0xc1c, 0xc44, 0xc54, 0xc64 },
> + { 0xc20, 0xc24, 0xc28, 0xc2c, 0xc48, 0xc58, 0xc68 },
> + { 0xc30, 0xc34, 0xc38, 0xc3c, 0xc4c, 0xc5c, 0xc6c },
> +};
> +
> +static inline u32 iproc_msi_read_reg(struct iproc_msi *msi,
> + enum iproc_msi_reg reg,
> + unsigned int eq)
> +{
> + struct iproc_pcie *pcie = msi->pcie;
> +
> + return readl_relaxed(pcie->base + msi->reg_offsets[eq][reg]);
> +}
> +
> +static inline void iproc_msi_write_reg(struct iproc_msi *msi,
> + enum iproc_msi_reg reg,
> + int eq, u32 val)
> +{
> + struct iproc_pcie *pcie = msi->pcie;
> +
> + writel_relaxed(val, pcie->base + msi->reg_offsets[eq][reg]);
> +}
> +
> +static inline bool iproc_msi_has_mult_regions(struct iproc_msi *msi)
> +{
> + return ((msi->nr_msi_region > 1) ? true : false);

return msi->nr_msi_region > 1;

> +}
> +
> +static inline bool iproc_eq_has_mult_regions(struct iproc_msi *msi)
> +{
> + return ((msi->nr_eq_region > 1) ? true : false);

return msi->nr_eq_region > 1;

> +}
> +
> +static struct irq_chip iproc_msi_irq_chip = {
> + .name = "iProc-MSI",
> +};
> +
> +static struct msi_domain_info iproc_msi_domain_info = {
> + .flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> + MSI_FLAG_PCI_MSIX,
> + .chip = &iproc_msi_irq_chip,
> +};
> +
> +/*
> + * In iProc PCIe core, each MSI group is serviced by a GIC interrupt and a
> + * dedicated event queue. Each MSI group can support up to 64 MSI vectors
> + *
> + * The number of MSI groups varies between different iProc SoCs. The total
> + * number of CPU cores also varies. To support MSI IRQ affinity, we
> + * distribute GIC interrupts across all available CPUs. MSI vector is moved
> + * from one GIC interrupt to another to steer to the target CPU
> + *
> + * Assuming:
> + * - the number of MSI groups is M
> + * - the number of CPU cores is N
> + * - M is always a multiple of N

How do you enforce that last condition?

> + *
> + * Total number of raw MSI vectors = M * 64
> + * Total number of supported MSI vectors = (M * 64) / N
> + */
> +static inline int hwirq_to_cpu(struct iproc_msi *msi, unsigned long hwirq)
> +{
> + return (hwirq % msi->nr_cpus);
> +}
> +
> +static inline unsigned long hwirq_to_canonical_hwirq(struct iproc_msi *msi,
> + unsigned long hwirq)
> +{
> + return (hwirq - hwirq_to_cpu(msi, hwirq));
> +}
> +
> +static int iproc_msi_irq_set_affinity(struct irq_data *data,
> + const struct cpumask *mask, bool force)
> +{
> + struct iproc_msi *msi = irq_data_get_irq_chip_data(data);
> + int target_cpu = cpumask_first(mask);
> + int curr_cpu;
> +
> + curr_cpu = hwirq_to_cpu(msi, data->hwirq);
> + if (curr_cpu == target_cpu)
> + return IRQ_SET_MASK_OK_DONE;
> +
> + /* steer MSI to the target CPU */
> + data->hwirq = hwirq_to_canonical_hwirq(msi, data->hwirq) + target_cpu;
> +
> + return IRQ_SET_MASK_OK;
> +}
> +
> +static inline u32 hwirq_to_group(struct iproc_msi *msi, unsigned long hwirq)
> +{
> + return (hwirq % msi->nr_irqs);
> +}
> +
> +static void iproc_msi_irq_compose_msi_msg(struct irq_data *data,
> + struct msi_msg *msg)
> +{
> + struct iproc_msi *msi = irq_data_get_irq_chip_data(data);
> + dma_addr_t addr;
> + unsigned int mul;
> +
> + if (iproc_msi_has_mult_regions(msi))
> + mul = MSI_MEM_REGION_SIZE;
> + else
> + mul = sizeof(u32);

Since this is the only use of that function, why don't you have it to
directly return the right multiplier?

> +
> + addr = msi->msi_dma + hwirq_to_group(msi, data->hwirq) * mul;
> + msg->address_lo = lower_32_bits(addr);
> + msg->address_hi = upper_32_bits(addr);
> + msg->data = data->hwirq;
> +}
> +
> +static struct irq_chip iproc_msi_bottom_irq_chip = {
> + .name = "MSI",
> + .irq_set_affinity = iproc_msi_irq_set_affinity,
> + .irq_compose_msi_msg = iproc_msi_irq_compose_msi_msg,
> +};
> +
> +static int iproc_msi_irq_domain_alloc(struct irq_domain *domain,
> + unsigned int virq, unsigned int nr_irqs,
> + void *args)
> +{
> + struct iproc_msi *msi = domain->host_data;
> + int hwirq;
> +
> + mutex_lock(&msi->bitmap_lock);
> +
> + /* allocate 'nr_cpus' number of MSI vectors each time */
> + hwirq = bitmap_find_next_zero_area(msi->bitmap, msi->nr_msi_vecs, 0,
> + msi->nr_cpus, 0);
> + if (hwirq < msi->nr_msi_vecs)
> + bitmap_set(msi->bitmap, hwirq, msi->nr_cpus);
> + else
> + return -ENOSPC;

Deadlock, here we come...

> +
> + mutex_unlock(&msi->bitmap_lock);
> +
> + irq_domain_set_info(domain, virq, hwirq, &iproc_msi_bottom_irq_chip,
> + domain->host_data, handle_simple_irq, NULL, NULL);
> +
> + return 0;
> +}
> +
> +static void iproc_msi_irq_domain_free(struct irq_domain *domain,
> + unsigned int virq, unsigned int nr_irqs)
> +{
> + struct irq_data *data = irq_domain_get_irq_data(domain, virq);
> + struct iproc_msi *msi = irq_data_get_irq_chip_data(data);
> + unsigned int hwirq;
> +
> + mutex_lock(&msi->bitmap_lock);
> +
> + hwirq = hwirq_to_canonical_hwirq(msi, data->hwirq);
> + bitmap_clear(msi->bitmap, hwirq, msi->nr_cpus);
> +
> + mutex_unlock(&msi->bitmap_lock);
> +
> + irq_domain_free_irqs_parent(domain, virq, nr_irqs);
> +}
> +
> +static const struct irq_domain_ops msi_domain_ops = {
> + .alloc = iproc_msi_irq_domain_alloc,
> + .free = iproc_msi_irq_domain_free,
> +};
> +
> +static inline u32 decode_msi_hwirq(struct iproc_msi *msi, u32 eq, u32 head)
> +{
> + u32 *msg, hwirq;
> + unsigned int offs;
> +
> + if (iproc_eq_has_mult_regions(msi))
> + offs = eq * EQ_MEM_REGION_SIZE;
> + else
> + offs = eq * EQ_LEN * sizeof(u32);

Same here.

> +
> + offs += head * sizeof(u32);
> + msg = (u32 *)(msi->eq_cpu + offs);

If that's the only place where you dereference msi->eq_cpu, why doesn't
it have the right type?

> + hwirq = *msg & IPROC_MSI_EQ_MASK;
> +
> + /*
> + * Since we have multiple hwirq mapped to a single MSI vector,
> + * now we need to derive the hwirq at CPU0. It can then be used to
> + * mapped back to virq
> + */
> + return hwirq_to_canonical_hwirq(msi, hwirq);
> +}
> +
> +static void iproc_msi_handler(struct irq_desc *desc)
> +{
> + struct irq_chip *chip = irq_desc_get_chip(desc);
> + struct iproc_msi_grp *grp;
> + struct iproc_msi *msi;
> + struct iproc_pcie *pcie;
> + u32 eq, head, tail, nr_events;
> + unsigned long hwirq;
> + int virq;
> +
> + chained_irq_enter(chip, desc);
> +
> + grp = irq_desc_get_handler_data(desc);
> + msi = grp->msi;
> + pcie = msi->pcie;
> + eq = grp->eq;
> +
> + /*
> + * iProc MSI event queue is tracked by head and tail pointers. Head
> + * pointer indicates the next entry to be processed by SW in the
> + * queue. Entries between head and tail pointers contain valid MSI
> + * data to be processed
> + */
> + head = iproc_msi_read_reg(msi, IPROC_MSI_EQ_HEAD,
> + eq) & IPROC_MSI_EQ_MASK;
> + do {
> + tail = iproc_msi_read_reg(msi, IPROC_MSI_EQ_TAIL,
> + eq) & IPROC_MSI_EQ_MASK;
> +
> + /*
> + * Figure out total number of events (MSI data) to be
> + * processed
> + */
> + nr_events = (tail < head) ?
> + (EQ_LEN - (head - tail)) : (tail - head);
> + if (!nr_events)
> + break;
> +
> + /* process all outstanding events */
> + while (nr_events--) {
> + hwirq = decode_msi_hwirq(msi, eq, head);
> + virq = irq_find_mapping(msi->inner_domain, hwirq);
> + generic_handle_irq(virq);
> +
> + head++;
> + head %= EQ_LEN;
> + iproc_msi_write_reg(msi, IPROC_MSI_EQ_HEAD, eq, head);
> + }

Wouldn't it be better to process all nr_events and only update head
once?

> +
> + /*
> + * Now go read the tail pointer again to see if there are new
> + * oustanding events that came in during the above window
> + */
> + } while (true);
> +
> + chained_irq_exit(chip, desc);
> +}
> +
> +static void iproc_msi_enable(struct iproc_msi *msi)
> +{
> + int i, eq;
> + u32 val;
> +
> + /* program memory region for each event queue */
> + for (i = 0; i < msi->nr_eq_region; i++) {
> + dma_addr_t addr = msi->eq_dma + (i * EQ_MEM_REGION_SIZE);
> +
> + iproc_msi_write_reg(msi, IPROC_MSI_EQ_PAGE, i,
> + lower_32_bits(addr));
> + iproc_msi_write_reg(msi, IPROC_MSI_EQ_PAGE_UPPER, i,
> + upper_32_bits(addr));
> + }
> +
> + /* program memory region for MSI posted writes */
> + for (i = 0; i < msi->nr_msi_region; i++) {
> + dma_addr_t addr = msi->msi_dma + (i * MSI_MEM_REGION_SIZE);
> +
> + iproc_msi_write_reg(msi, IPROC_MSI_PAGE, i,
> + lower_32_bits(addr));
> + iproc_msi_write_reg(msi, IPROC_MSI_PAGE_UPPER, i,
> + upper_32_bits(addr));
> + }
> +
> + for (eq = 0; eq < msi->nr_irqs; eq++) {
> + /* enable MSI event queue */
> + val = IPROC_MSI_INTR_EN | IPROC_MSI_INT_N_EVENT |
> + IPROC_MSI_EQ_EN;
> + iproc_msi_write_reg(msi, IPROC_MSI_CTRL, eq, val);
> +
> + /*
> + * Some legacy platforms require the MSI interrupt enable
> + * register to be set explicitly
> + */
> + if (msi->has_inten_reg) {
> + val = iproc_msi_read_reg(msi, IPROC_MSI_INTS_EN, eq);
> + val |= BIT(eq);
> + iproc_msi_write_reg(msi, IPROC_MSI_INTS_EN, eq, val);
> + }
> + }
> +}
> +
> +static void iproc_msi_disable(struct iproc_msi *msi)
> +{
> + u32 eq, val;
> +
> + for (eq = 0; eq < msi->nr_irqs; eq++) {
> + if (msi->has_inten_reg) {
> + val = iproc_msi_read_reg(msi, IPROC_MSI_INTS_EN, eq);
> + val &= ~BIT(eq);
> + iproc_msi_write_reg(msi, IPROC_MSI_INTS_EN, eq, val);
> + }
> +
> + val = iproc_msi_read_reg(msi, IPROC_MSI_CTRL, eq);
> + val &= ~(IPROC_MSI_INTR_EN | IPROC_MSI_INT_N_EVENT |
> + IPROC_MSI_EQ_EN);
> + iproc_msi_write_reg(msi, IPROC_MSI_CTRL, eq, val);
> + }
> +}
> +
> +static int iproc_msi_alloc_domains(struct device_node *node,
> + struct iproc_msi *msi)
> +{
> + msi->inner_domain = irq_domain_add_linear(NULL, msi->nr_msi_vecs,
> + &msi_domain_ops, msi);
> + if (!msi->inner_domain)
> + return -ENOMEM;
> +
> + msi->msi_domain = pci_msi_create_irq_domain(of_node_to_fwnode(node),
> + &iproc_msi_domain_info,
> + msi->inner_domain);
> + if (!msi->msi_domain) {
> + irq_domain_remove(msi->inner_domain);
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> +static void iproc_msi_free_domains(struct iproc_msi *msi)
> +{
> + if (msi->msi_domain)
> + irq_domain_remove(msi->msi_domain);
> +
> + if (msi->inner_domain)
> + irq_domain_remove(msi->inner_domain);
> +}
> +
> +static int iproc_msi_irq_setup(struct iproc_msi *msi, unsigned int cpu)
> +{
> + int i, ret;
> + cpumask_var_t mask;
> + struct iproc_pcie *pcie = msi->pcie;
> +
> + for (i = cpu; i < msi->nr_irqs; i += msi->nr_cpus) {
> + irq_set_chained_handler_and_data(msi->grps[i].gic_irq,
> + iproc_msi_handler,
> + &msi->grps[i]);
> + /* dedicate GIC interrupt to each CPU core */
> + if (alloc_cpumask_var(&mask, GFP_KERNEL)) {
> + cpumask_clear(mask);
> + cpumask_set_cpu(cpu, mask);
> + ret = irq_set_affinity(msi->grps[i].gic_irq, mask);
> + if (ret)
> + dev_err(pcie->dev,
> + "failed to set affinity for IRQ%d\n",
> + msi->grps[i].gic_irq);
> + free_cpumask_var(mask);
> + } else {
> + dev_err(pcie->dev, "failed to alloc CPU mask\n");
> + ret = -EINVAL;
> + }
> +
> + if (ret) {
> + irq_set_chained_handler_and_data(msi->grps[i].gic_irq,
> + NULL, NULL);
> + return ret;

What happens to interrupts you've already configured? I'd expect a full
rollback.

> + }
> + }
> +
> + return 0;
> +}
> +
> +static void iproc_msi_irq_free(struct iproc_msi *msi, unsigned int cpu)
> +{
> + int i;
> +
> + for (i = cpu; i < msi->nr_irqs; i += msi->nr_cpus) {
> + irq_set_chained_handler_and_data(msi->grps[i].gic_irq,
> + NULL, NULL);
> + }
> +}
> +
> +int iproc_msi_init(struct iproc_pcie *pcie, struct device_node *node)
> +{
> + struct iproc_msi *msi;
> + int i, ret;
> + unsigned int cpu;
> +
> + if (!of_device_is_compatible(node, "brcm,iproc-msi"))
> + return -ENODEV;
> +
> + if (!of_find_property(node, "msi-controller", NULL))
> + return -ENODEV;
> +
> + if (pcie->msi)
> + return -EBUSY;
> +
> + msi = devm_kzalloc(pcie->dev, sizeof(*msi), GFP_KERNEL);
> + if (!msi)
> + return -ENOMEM;
> +
> + msi->pcie = pcie;
> + pcie->msi = msi;
> + mutex_init(&msi->bitmap_lock);
> + msi->nr_cpus = num_possible_cpus();
> +
> + switch (pcie->type) {
> + case IPROC_PCIE_PAXB:
> + msi->reg_offsets = iproc_msi_reg_paxb;
> + break;
> + case IPROC_PCIE_PAXC:
> + msi->reg_offsets = iproc_msi_reg_paxc;
> + break;
> + default:
> + dev_err(pcie->dev, "incompatible iProc PCIe interface\n");
> + return -EINVAL;
> + }
> +
> + if (of_find_property(node, "brcm,pcie-msi-inten", NULL))
> + msi->has_inten_reg = true;
> +
> + ret = of_property_read_u32(node, "brcm,num-eq-region",
> + &msi->nr_eq_region);
> + if (ret || !msi->nr_eq_region)
> + msi->nr_eq_region = 1;
> +
> + ret = of_property_read_u32(node, "brcm,num-msi-msg-region",
> + &msi->nr_msi_region);
> + if (ret || !msi->nr_msi_region)
> + msi->nr_msi_region = 1;
> +
> + msi->nr_irqs = of_irq_count(node);
> + if (!msi->nr_irqs) {
> + dev_err(pcie->dev, "found no MSI GIC interrupt\n");
> + return -ENODEV;
> + }
> + if (msi->nr_irqs > NR_HW_IRQS) {
> + dev_warn(pcie->dev, "too many MSI GIC interrupts defined %d\n",
> + msi->nr_irqs);
> + msi->nr_irqs = NR_HW_IRQS;
> + }
> +
> + msi->nr_msi_vecs = msi->nr_irqs * EQ_LEN;
> + msi->bitmap = devm_kcalloc(pcie->dev, BITS_TO_LONGS(msi->nr_msi_vecs),
> + sizeof(*msi->bitmap), GFP_KERNEL);
> + if (!msi->bitmap)
> + return -ENOMEM;
> +
> + msi->grps = devm_kcalloc(pcie->dev, msi->nr_irqs, sizeof(*msi->grps),
> + GFP_KERNEL);
> + if (!msi->grps)
> + return -ENOMEM;
> +
> + for (i = 0; i < msi->nr_irqs; i++) {
> + unsigned int irq = irq_of_parse_and_map(node, i);
> +
> + if (!irq) {
> + dev_err(pcie->dev, "unable to parse/map interrupt\n");
> + return -ENODEV;
> + }
> + msi->grps[i].gic_irq = irq;
> + msi->grps[i].msi = msi;
> + msi->grps[i].eq = i;
> + }
> +
> + /* reserve memory for MSI event queue */
> + msi->eq_cpu = dma_alloc_coherent(pcie->dev,
> + msi->nr_eq_region * EQ_MEM_REGION_SIZE,
> + &msi->eq_dma, GFP_KERNEL);

Do you need to zero that memory? Or is the HW happy with whatever will
be there?

> + if (!msi->eq_cpu)
> + return -ENOMEM;
> +
> + /* reserve memory for MSI posted writes */
> + msi->msi_cpu = dma_alloc_coherent(pcie->dev,
> + msi->nr_msi_region * MSI_MEM_REGION_SIZE,
> + &msi->msi_dma, GFP_KERNEL);

Same here. Also, what is the exact purpose of that memory? You have a
coherent mapping with the CPU, but you never read from it. So what's
the point?

> + if (!msi->msi_cpu) {
> + ret = -ENOMEM;
> + goto free_eq_dma;
> + }
> +
> + ret = iproc_msi_alloc_domains(node, msi);
> + if (ret) {
> + dev_err(pcie->dev, "failed to create MSI domains\n");
> + goto free_msi_dma;
> + }
> +
> + for_each_online_cpu(cpu) {
> + ret = iproc_msi_irq_setup(msi, cpu);
> + if (ret)
> + goto free_msi_irq;
> + }
> +
> + iproc_msi_enable(msi);
> +
> + return 0;
> +
> +free_msi_irq:
> + for_each_online_cpu(cpu)
> + iproc_msi_irq_free(msi, cpu);
> + iproc_msi_free_domains(msi);
> +
> +free_msi_dma:
> + dma_free_coherent(pcie->dev, msi->nr_msi_region * MSI_MEM_REGION_SIZE,
> + msi->msi_cpu, msi->msi_dma);
> +free_eq_dma:
> + dma_free_coherent(pcie->dev, msi->nr_eq_region * EQ_MEM_REGION_SIZE,
> + msi->eq_cpu, msi->eq_dma);
> + pcie->msi = NULL;
> + return ret;
> +}
> +EXPORT_SYMBOL(iproc_msi_init);

[...]

Thanks,

M.
--
Jazz is not dead. It just smells funny.

2015-11-26 01:52:45

by Ray Jui

[permalink] [raw]
Subject: Re: [PATCH v2 4/5] PCI: iproc: Add iProc PCIe MSI support

Hi Marc,

On 11/25/2015 9:36 AM, Marc Zyngier wrote:
> On Tue, 24 Nov 2015 15:04:53 -0800
> Ray Jui <[email protected]> wrote:
>
>> This patch adds PCIe MSI support for both PAXB and PAXC interfaces on
>> all iProc based platforms. The patch follows the latest trend in the
>> kernel to use MSI domain based implementation
>>
>
> That's a pretty useless comment. The general trend in the kernel is to
> use the most appropriate infrastructure.
>
>> This iProc event queue based MSI support should not be used with newer
>> platforms with integrated MSI support in the GIC (e.g., giv2m or
>> gicv3-its)
>>
>
> I'd be more interested in some documentation explaining how the HW
> works, how the various data structures are updated, and when.
>

Okay will try my best to describe my understanding of iProc MSI in the
commit message of my next iteration of patches.

>> Signed-off-by: Ray Jui <[email protected]>
>> Reviewed-by: Anup Patel <[email protected]>
>> Reviewed-by: Vikram Prakash <[email protected]>
>> Reviewed-by: Scott Branden <[email protected]>
>> ---
>> drivers/pci/host/Kconfig | 9 +
>> drivers/pci/host/Makefile | 1 +
>> drivers/pci/host/pcie-iproc-msi.c | 662 ++++++++++++++++++++++++++++++++++++++
>> drivers/pci/host/pcie-iproc.c | 26 ++
>> drivers/pci/host/pcie-iproc.h | 21 +-
>> 5 files changed, 717 insertions(+), 2 deletions(-)
>> create mode 100644 drivers/pci/host/pcie-iproc-msi.c
>>
>> diff --git a/drivers/pci/host/Kconfig b/drivers/pci/host/Kconfig
>> index f131ba9..972e906 100644
>> --- a/drivers/pci/host/Kconfig
>> +++ b/drivers/pci/host/Kconfig
>> @@ -126,6 +126,15 @@ config PCIE_IPROC
>> iProc family of SoCs. An appropriate bus interface driver also needs
>> to be enabled
>>
>> +config PCIE_IPROC_MSI
>> + bool "Broadcom iProc PCIe MSI support"
>> + depends on ARCH_BCM_IPROC && PCI_MSI
>> + select PCI_MSI_IRQ_DOMAIN
>> + default ARCH_BCM_IPROC
>> + help
>> + Say Y here if you want to enable MSI support for Broadcom's iProc
>> + PCIe controller
>> +
>> config PCIE_IPROC_PLATFORM
>> tristate "Broadcom iProc PCIe platform bus driver"
>> depends on ARCH_BCM_IPROC || (ARM && COMPILE_TEST)
>> diff --git a/drivers/pci/host/Makefile b/drivers/pci/host/Makefile
>> index 9d4d3c6..0e4e95e 100644
>> --- a/drivers/pci/host/Makefile
>> +++ b/drivers/pci/host/Makefile
>> @@ -15,6 +15,7 @@ obj-$(CONFIG_PCI_XGENE_MSI) += pci-xgene-msi.o
>> obj-$(CONFIG_PCI_LAYERSCAPE) += pci-layerscape.o
>> obj-$(CONFIG_PCI_VERSATILE) += pci-versatile.o
>> obj-$(CONFIG_PCIE_IPROC) += pcie-iproc.o
>> +obj-$(CONFIG_PCIE_IPROC_MSI) += pcie-iproc-msi.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-iproc-msi.c b/drivers/pci/host/pcie-iproc-msi.c
>> new file mode 100644
>> index 0000000..afc54c2
>> --- /dev/null
>> +++ b/drivers/pci/host/pcie-iproc-msi.c
>> @@ -0,0 +1,662 @@
>> +/*
>> + * Copyright (C) 2015 Broadcom Corporation
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation version 2.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether express or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/irqchip/chained_irq.h>
>> +#include <linux/irqdomain.h>
>> +#include <linux/msi.h>
>> +#include <linux/of_irq.h>
>> +#include <linux/of_pci.h>
>> +#include <linux/pci.h>
>> +
>> +#include "pcie-iproc.h"
>> +
>> +#define IPROC_MSI_INTR_EN_SHIFT 11
>> +#define IPROC_MSI_INTR_EN BIT(IPROC_MSI_INTR_EN_SHIFT)
>> +#define IPROC_MSI_INT_N_EVENT_SHIFT 1
>> +#define IPROC_MSI_INT_N_EVENT BIT(IPROC_MSI_INT_N_EVENT_SHIFT)
>> +#define IPROC_MSI_EQ_EN_SHIFT 0
>> +#define IPROC_MSI_EQ_EN BIT(IPROC_MSI_EQ_EN_SHIFT)
>> +
>> +#define IPROC_MSI_EQ_MASK 0x3f
>> +
>> +/* max number of GIC interrupts */
>> +#define NR_HW_IRQS 6
>> +
>> +/* number of entries in each event queue */
>> +#define EQ_LEN 64
>> +
>> +/* size of each event queue memory region */
>> +#define EQ_MEM_REGION_SIZE SZ_4K
>> +
>> +/* size of each MSI message memory region */
>> +#define MSI_MEM_REGION_SIZE SZ_4K
>> +
>> +enum iproc_msi_reg {
>> + IPROC_MSI_EQ_PAGE = 0,
>> + IPROC_MSI_EQ_PAGE_UPPER,
>> + IPROC_MSI_PAGE,
>> + IPROC_MSI_PAGE_UPPER,
>> + IPROC_MSI_CTRL,
>> + IPROC_MSI_EQ_HEAD,
>> + IPROC_MSI_EQ_TAIL,
>> + IPROC_MSI_INTS_EN,
>> + IPROC_MSI_REG_SIZE,
>> +};
>> +
>> +struct iproc_msi;
>> +
>> +/**
>> + * iProc MSI group
>> + *
>> + * One MSI group is allocated per GIC interrupt, serviced by one iProc MSI
>> + * event queue
>> + *
>> + * @msi: pointer to iProc MSI data
>> + * @gic_irq: GIC interrupt
>> + * @eq: Event queue number
>> + */
>> +struct iproc_msi_grp {
>> + struct iproc_msi *msi;
>> + int gic_irq;
>> + unsigned int eq;
>> +};
>> +
>> +/**
>> + * iProc event queue based MSI
>> + *
>> + * Only meant to be used on platforms without MSI support integrated into the
>> + * GIC
>> + *
>> + * @pcie: pointer to iProc PCIe data
>> + * @reg_offsets: MSI register offsets
>> + * @grps: MSI groups
>> + * @nr_irqs: number of total interrupts connected to GIC
>> + * @nr_cpus: number of toal CPUs
>> + * @has_inten_reg: indicates the MSI interrupt enable register needs to be
>> + * set explicitly (required for some legacy platforms)
>> + * @bitmap: MSI vector bitmap
>> + * @bitmap_lock: lock to protect access to the MSI bitmap
>> + * @nr_msi_vecs: total number of MSI vectors
>> + * @inner_domain: inner IRQ domain
>> + * @msi_domain: MSI IRQ domain
>> + * @nr_eq_region: required number of 4K aligned memory region for MSI event
>> + * queues
>> + * @nr_msi_region: required number of 4K aligned memory region for MSI posted
>> + * writes
>> + * @eq_cpu: pointer to allocated memory region for MSI event queues
>> + * @eq_dma: DMA address of MSI event queues
>> + * @msi_cpu: pointer to allocated memory region for MSI posted writes
>> + * @msi_dma: DMA address of MSI posted writes
>> + */
>> +struct iproc_msi {
>> + struct iproc_pcie *pcie;
>> + const u16 (*reg_offsets)[IPROC_MSI_REG_SIZE];
>> + struct iproc_msi_grp *grps;
>> + int nr_irqs;
>> + int nr_cpus;
>> + bool has_inten_reg;
>> + unsigned long *bitmap;
>> + struct mutex bitmap_lock;
>> + unsigned int nr_msi_vecs;
>> + struct irq_domain *inner_domain;
>> + struct irq_domain *msi_domain;
>> + unsigned int nr_eq_region;
>> + unsigned int nr_msi_region;
>> + void *eq_cpu;
>> + dma_addr_t eq_dma;
>> + void *msi_cpu;
>> + dma_addr_t msi_dma;
>> +};
>> +
>> +static const u16 iproc_msi_reg_paxb[NR_HW_IRQS][IPROC_MSI_REG_SIZE] = {
>> + { 0x200, 0x2c0, 0x204, 0x2c4, 0x210, 0x250, 0x254, 0x208 },
>> + { 0x200, 0x2c0, 0x204, 0x2c4, 0x214, 0x258, 0x25c, 0x208 },
>> + { 0x200, 0x2c0, 0x204, 0x2c4, 0x218, 0x260, 0x264, 0x208 },
>> + { 0x200, 0x2c0, 0x204, 0x2c4, 0x21c, 0x268, 0x26c, 0x208 },
>> + { 0x200, 0x2c0, 0x204, 0x2c4, 0x220, 0x270, 0x274, 0x208 },
>> + { 0x200, 0x2c0, 0x204, 0x2c4, 0x224, 0x278, 0x27c, 0x208 },
>> +};
>> +
>> +static const u16 iproc_msi_reg_paxc[NR_HW_IRQS][IPROC_MSI_REG_SIZE] = {
>> + { 0xc00, 0xc04, 0xc08, 0xc0c, 0xc40, 0xc50, 0xc60 },
>> + { 0xc10, 0xc14, 0xc18, 0xc1c, 0xc44, 0xc54, 0xc64 },
>> + { 0xc20, 0xc24, 0xc28, 0xc2c, 0xc48, 0xc58, 0xc68 },
>> + { 0xc30, 0xc34, 0xc38, 0xc3c, 0xc4c, 0xc5c, 0xc6c },
>> +};
>> +
>> +static inline u32 iproc_msi_read_reg(struct iproc_msi *msi,
>> + enum iproc_msi_reg reg,
>> + unsigned int eq)
>> +{
>> + struct iproc_pcie *pcie = msi->pcie;
>> +
>> + return readl_relaxed(pcie->base + msi->reg_offsets[eq][reg]);
>> +}
>> +
>> +static inline void iproc_msi_write_reg(struct iproc_msi *msi,
>> + enum iproc_msi_reg reg,
>> + int eq, u32 val)
>> +{
>> + struct iproc_pcie *pcie = msi->pcie;
>> +
>> + writel_relaxed(val, pcie->base + msi->reg_offsets[eq][reg]);
>> +}
>> +
>> +static inline bool iproc_msi_has_mult_regions(struct iproc_msi *msi)
>> +{
>> + return ((msi->nr_msi_region > 1) ? true : false);
>
> return msi->nr_msi_region > 1;
>

Will change this function to return the multiplier according to your
comment below.

>> +}
>> +
>> +static inline bool iproc_eq_has_mult_regions(struct iproc_msi *msi)
>> +{
>> + return ((msi->nr_eq_region > 1) ? true : false);
>
> return msi->nr_eq_region > 1;
>

Will change this function to return the offsets directly.

>> +}
>> +
>> +static struct irq_chip iproc_msi_irq_chip = {
>> + .name = "iProc-MSI",
>> +};
>> +
>> +static struct msi_domain_info iproc_msi_domain_info = {
>> + .flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
>> + MSI_FLAG_PCI_MSIX,
>> + .chip = &iproc_msi_irq_chip,
>> +};
>> +
>> +/*
>> + * In iProc PCIe core, each MSI group is serviced by a GIC interrupt and a
>> + * dedicated event queue. Each MSI group can support up to 64 MSI vectors
>> + *
>> + * The number of MSI groups varies between different iProc SoCs. The total
>> + * number of CPU cores also varies. To support MSI IRQ affinity, we
>> + * distribute GIC interrupts across all available CPUs. MSI vector is moved
>> + * from one GIC interrupt to another to steer to the target CPU
>> + *
>> + * Assuming:
>> + * - the number of MSI groups is M
>> + * - the number of CPU cores is N
>> + * - M is always a multiple of N
>
> How do you enforce that last condition?
>

Okay I need to enforce this as soon as the number of GIC interrupts is
determined from DT in the init routine. I will add code to enforce this
condition.

>> + *
>> + * Total number of raw MSI vectors = M * 64
>> + * Total number of supported MSI vectors = (M * 64) / N
>> + */
>> +static inline int hwirq_to_cpu(struct iproc_msi *msi, unsigned long hwirq)
>> +{
>> + return (hwirq % msi->nr_cpus);
>> +}
>> +
>> +static inline unsigned long hwirq_to_canonical_hwirq(struct iproc_msi *msi,
>> + unsigned long hwirq)
>> +{
>> + return (hwirq - hwirq_to_cpu(msi, hwirq));
>> +}
>> +
>> +static int iproc_msi_irq_set_affinity(struct irq_data *data,
>> + const struct cpumask *mask, bool force)
>> +{
>> + struct iproc_msi *msi = irq_data_get_irq_chip_data(data);
>> + int target_cpu = cpumask_first(mask);
>> + int curr_cpu;
>> +
>> + curr_cpu = hwirq_to_cpu(msi, data->hwirq);
>> + if (curr_cpu == target_cpu)
>> + return IRQ_SET_MASK_OK_DONE;
>> +
>> + /* steer MSI to the target CPU */
>> + data->hwirq = hwirq_to_canonical_hwirq(msi, data->hwirq) + target_cpu;
>> +
>> + return IRQ_SET_MASK_OK;
>> +}
>> +
>> +static inline u32 hwirq_to_group(struct iproc_msi *msi, unsigned long hwirq)
>> +{
>> + return (hwirq % msi->nr_irqs);
>> +}
>> +
>> +static void iproc_msi_irq_compose_msi_msg(struct irq_data *data,
>> + struct msi_msg *msg)
>> +{
>> + struct iproc_msi *msi = irq_data_get_irq_chip_data(data);
>> + dma_addr_t addr;
>> + unsigned int mul;
>> +
>> + if (iproc_msi_has_mult_regions(msi))
>> + mul = MSI_MEM_REGION_SIZE;
>> + else
>> + mul = sizeof(u32);
>
> Since this is the only use of that function, why don't you have it to
> directly return the right multiplier?
>

True, that will make the above code simpler just one line. Will do!

>> +
>> + addr = msi->msi_dma + hwirq_to_group(msi, data->hwirq) * mul;
>> + msg->address_lo = lower_32_bits(addr);
>> + msg->address_hi = upper_32_bits(addr);
>> + msg->data = data->hwirq;
>> +}
>> +
>> +static struct irq_chip iproc_msi_bottom_irq_chip = {
>> + .name = "MSI",
>> + .irq_set_affinity = iproc_msi_irq_set_affinity,
>> + .irq_compose_msi_msg = iproc_msi_irq_compose_msi_msg,
>> +};
>> +
>> +static int iproc_msi_irq_domain_alloc(struct irq_domain *domain,
>> + unsigned int virq, unsigned int nr_irqs,
>> + void *args)
>> +{
>> + struct iproc_msi *msi = domain->host_data;
>> + int hwirq;
>> +
>> + mutex_lock(&msi->bitmap_lock);
>> +
>> + /* allocate 'nr_cpus' number of MSI vectors each time */
>> + hwirq = bitmap_find_next_zero_area(msi->bitmap, msi->nr_msi_vecs, 0,
>> + msi->nr_cpus, 0);
>> + if (hwirq < msi->nr_msi_vecs)
>> + bitmap_set(msi->bitmap, hwirq, msi->nr_cpus);
>> + else
>> + return -ENOSPC;
>
> Deadlock, here we come...
>

Damn it. My bad. Will fix.

>> +
>> + mutex_unlock(&msi->bitmap_lock);
>> +
>> + irq_domain_set_info(domain, virq, hwirq, &iproc_msi_bottom_irq_chip,
>> + domain->host_data, handle_simple_irq, NULL, NULL);
>> +
>> + return 0;
>> +}
>> +
>> +static void iproc_msi_irq_domain_free(struct irq_domain *domain,
>> + unsigned int virq, unsigned int nr_irqs)
>> +{
>> + struct irq_data *data = irq_domain_get_irq_data(domain, virq);
>> + struct iproc_msi *msi = irq_data_get_irq_chip_data(data);
>> + unsigned int hwirq;
>> +
>> + mutex_lock(&msi->bitmap_lock);
>> +
>> + hwirq = hwirq_to_canonical_hwirq(msi, data->hwirq);
>> + bitmap_clear(msi->bitmap, hwirq, msi->nr_cpus);
>> +
>> + mutex_unlock(&msi->bitmap_lock);
>> +
>> + irq_domain_free_irqs_parent(domain, virq, nr_irqs);
>> +}
>> +
>> +static const struct irq_domain_ops msi_domain_ops = {
>> + .alloc = iproc_msi_irq_domain_alloc,
>> + .free = iproc_msi_irq_domain_free,
>> +};
>> +
>> +static inline u32 decode_msi_hwirq(struct iproc_msi *msi, u32 eq, u32 head)
>> +{
>> + u32 *msg, hwirq;
>> + unsigned int offs;
>> +
>> + if (iproc_eq_has_mult_regions(msi))
>> + offs = eq * EQ_MEM_REGION_SIZE;
>> + else
>> + offs = eq * EQ_LEN * sizeof(u32);
>
> Same here.
>

Okay. Will change the code. Thanks.

>> +
>> + offs += head * sizeof(u32);
>> + msg = (u32 *)(msi->eq_cpu + offs);
>
> If that's the only place where you dereference msi->eq_cpu, why doesn't
> it have the right type?
>

Okay this really caught me, :) I actually changed msi->eq_cpu to type
'u32 *' and found things all of a sudden stopped working. Then I
realized it's a lot easier to let msi->eq_cpu stay as 'void *', so we
can keep the arithmetic to calcualte 'offs' as it is. If I change it to
'u32 *', then I would need to divide 'offs' by sizeof(u32) to get right
address of the pointer.

>> + hwirq = *msg & IPROC_MSI_EQ_MASK;
>> +
>> + /*
>> + * Since we have multiple hwirq mapped to a single MSI vector,
>> + * now we need to derive the hwirq at CPU0. It can then be used to
>> + * mapped back to virq
>> + */
>> + return hwirq_to_canonical_hwirq(msi, hwirq);
>> +}
>> +
>> +static void iproc_msi_handler(struct irq_desc *desc)
>> +{
>> + struct irq_chip *chip = irq_desc_get_chip(desc);
>> + struct iproc_msi_grp *grp;
>> + struct iproc_msi *msi;
>> + struct iproc_pcie *pcie;
>> + u32 eq, head, tail, nr_events;
>> + unsigned long hwirq;
>> + int virq;
>> +
>> + chained_irq_enter(chip, desc);
>> +
>> + grp = irq_desc_get_handler_data(desc);
>> + msi = grp->msi;
>> + pcie = msi->pcie;
>> + eq = grp->eq;
>> +
>> + /*
>> + * iProc MSI event queue is tracked by head and tail pointers. Head
>> + * pointer indicates the next entry to be processed by SW in the
>> + * queue. Entries between head and tail pointers contain valid MSI
>> + * data to be processed
>> + */
>> + head = iproc_msi_read_reg(msi, IPROC_MSI_EQ_HEAD,
>> + eq) & IPROC_MSI_EQ_MASK;
>> + do {
>> + tail = iproc_msi_read_reg(msi, IPROC_MSI_EQ_TAIL,
>> + eq) & IPROC_MSI_EQ_MASK;
>> +
>> + /*
>> + * Figure out total number of events (MSI data) to be
>> + * processed
>> + */
>> + nr_events = (tail < head) ?
>> + (EQ_LEN - (head - tail)) : (tail - head);
>> + if (!nr_events)
>> + break;
>> +
>> + /* process all outstanding events */
>> + while (nr_events--) {
>> + hwirq = decode_msi_hwirq(msi, eq, head);
>> + virq = irq_find_mapping(msi->inner_domain, hwirq);
>> + generic_handle_irq(virq);
>> +
>> + head++;
>> + head %= EQ_LEN;
>> + iproc_msi_write_reg(msi, IPROC_MSI_EQ_HEAD, eq, head);
>> + }
>
> Wouldn't it be better to process all nr_events and only update head
> once?
>

You are right. That will help to get rid of the overhead of repeatedly
writing to the event queue head register. Will make the change.

>> +
>> + /*
>> + * Now go read the tail pointer again to see if there are new
>> + * oustanding events that came in during the above window
>> + */
>> + } while (true);
>> +
>> + chained_irq_exit(chip, desc);
>> +}
>> +
>> +static void iproc_msi_enable(struct iproc_msi *msi)
>> +{
>> + int i, eq;
>> + u32 val;
>> +
>> + /* program memory region for each event queue */
>> + for (i = 0; i < msi->nr_eq_region; i++) {
>> + dma_addr_t addr = msi->eq_dma + (i * EQ_MEM_REGION_SIZE);
>> +
>> + iproc_msi_write_reg(msi, IPROC_MSI_EQ_PAGE, i,
>> + lower_32_bits(addr));
>> + iproc_msi_write_reg(msi, IPROC_MSI_EQ_PAGE_UPPER, i,
>> + upper_32_bits(addr));
>> + }
>> +
>> + /* program memory region for MSI posted writes */
>> + for (i = 0; i < msi->nr_msi_region; i++) {
>> + dma_addr_t addr = msi->msi_dma + (i * MSI_MEM_REGION_SIZE);
>> +
>> + iproc_msi_write_reg(msi, IPROC_MSI_PAGE, i,
>> + lower_32_bits(addr));
>> + iproc_msi_write_reg(msi, IPROC_MSI_PAGE_UPPER, i,
>> + upper_32_bits(addr));
>> + }
>> +
>> + for (eq = 0; eq < msi->nr_irqs; eq++) {
>> + /* enable MSI event queue */
>> + val = IPROC_MSI_INTR_EN | IPROC_MSI_INT_N_EVENT |
>> + IPROC_MSI_EQ_EN;
>> + iproc_msi_write_reg(msi, IPROC_MSI_CTRL, eq, val);
>> +
>> + /*
>> + * Some legacy platforms require the MSI interrupt enable
>> + * register to be set explicitly
>> + */
>> + if (msi->has_inten_reg) {
>> + val = iproc_msi_read_reg(msi, IPROC_MSI_INTS_EN, eq);
>> + val |= BIT(eq);
>> + iproc_msi_write_reg(msi, IPROC_MSI_INTS_EN, eq, val);
>> + }
>> + }
>> +}
>> +
>> +static void iproc_msi_disable(struct iproc_msi *msi)
>> +{
>> + u32 eq, val;
>> +
>> + for (eq = 0; eq < msi->nr_irqs; eq++) {
>> + if (msi->has_inten_reg) {
>> + val = iproc_msi_read_reg(msi, IPROC_MSI_INTS_EN, eq);
>> + val &= ~BIT(eq);
>> + iproc_msi_write_reg(msi, IPROC_MSI_INTS_EN, eq, val);
>> + }
>> +
>> + val = iproc_msi_read_reg(msi, IPROC_MSI_CTRL, eq);
>> + val &= ~(IPROC_MSI_INTR_EN | IPROC_MSI_INT_N_EVENT |
>> + IPROC_MSI_EQ_EN);
>> + iproc_msi_write_reg(msi, IPROC_MSI_CTRL, eq, val);
>> + }
>> +}
>> +
>> +static int iproc_msi_alloc_domains(struct device_node *node,
>> + struct iproc_msi *msi)
>> +{
>> + msi->inner_domain = irq_domain_add_linear(NULL, msi->nr_msi_vecs,
>> + &msi_domain_ops, msi);
>> + if (!msi->inner_domain)
>> + return -ENOMEM;
>> +
>> + msi->msi_domain = pci_msi_create_irq_domain(of_node_to_fwnode(node),
>> + &iproc_msi_domain_info,
>> + msi->inner_domain);
>> + if (!msi->msi_domain) {
>> + irq_domain_remove(msi->inner_domain);
>> + return -ENOMEM;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void iproc_msi_free_domains(struct iproc_msi *msi)
>> +{
>> + if (msi->msi_domain)
>> + irq_domain_remove(msi->msi_domain);
>> +
>> + if (msi->inner_domain)
>> + irq_domain_remove(msi->inner_domain);
>> +}
>> +
>> +static int iproc_msi_irq_setup(struct iproc_msi *msi, unsigned int cpu)
>> +{
>> + int i, ret;
>> + cpumask_var_t mask;
>> + struct iproc_pcie *pcie = msi->pcie;
>> +
>> + for (i = cpu; i < msi->nr_irqs; i += msi->nr_cpus) {
>> + irq_set_chained_handler_and_data(msi->grps[i].gic_irq,
>> + iproc_msi_handler,
>> + &msi->grps[i]);
>> + /* dedicate GIC interrupt to each CPU core */
>> + if (alloc_cpumask_var(&mask, GFP_KERNEL)) {
>> + cpumask_clear(mask);
>> + cpumask_set_cpu(cpu, mask);
>> + ret = irq_set_affinity(msi->grps[i].gic_irq, mask);
>> + if (ret)
>> + dev_err(pcie->dev,
>> + "failed to set affinity for IRQ%d\n",
>> + msi->grps[i].gic_irq);
>> + free_cpumask_var(mask);
>> + } else {
>> + dev_err(pcie->dev, "failed to alloc CPU mask\n");
>> + ret = -EINVAL;
>> + }
>> +
>> + if (ret) {
>> + irq_set_chained_handler_and_data(msi->grps[i].gic_irq,
>> + NULL, NULL);
>> + return ret;
>
> What happens to interrupts you've already configured? I'd expect a full
> rollback.
>

Yeah in the error case, need to roll back all previously configured irq
by the time we exit this function. Will fix!

>> + }
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static void iproc_msi_irq_free(struct iproc_msi *msi, unsigned int cpu)
>> +{
>> + int i;
>> +
>> + for (i = cpu; i < msi->nr_irqs; i += msi->nr_cpus) {
>> + irq_set_chained_handler_and_data(msi->grps[i].gic_irq,
>> + NULL, NULL);
>> + }
>> +}
>> +
>> +int iproc_msi_init(struct iproc_pcie *pcie, struct device_node *node)
>> +{
>> + struct iproc_msi *msi;
>> + int i, ret;
>> + unsigned int cpu;
>> +
>> + if (!of_device_is_compatible(node, "brcm,iproc-msi"))
>> + return -ENODEV;
>> +
>> + if (!of_find_property(node, "msi-controller", NULL))
>> + return -ENODEV;
>> +
>> + if (pcie->msi)
>> + return -EBUSY;
>> +
>> + msi = devm_kzalloc(pcie->dev, sizeof(*msi), GFP_KERNEL);
>> + if (!msi)
>> + return -ENOMEM;
>> +
>> + msi->pcie = pcie;
>> + pcie->msi = msi;
>> + mutex_init(&msi->bitmap_lock);
>> + msi->nr_cpus = num_possible_cpus();
>> +
>> + switch (pcie->type) {
>> + case IPROC_PCIE_PAXB:
>> + msi->reg_offsets = iproc_msi_reg_paxb;
>> + break;
>> + case IPROC_PCIE_PAXC:
>> + msi->reg_offsets = iproc_msi_reg_paxc;
>> + break;
>> + default:
>> + dev_err(pcie->dev, "incompatible iProc PCIe interface\n");
>> + return -EINVAL;
>> + }
>> +
>> + if (of_find_property(node, "brcm,pcie-msi-inten", NULL))
>> + msi->has_inten_reg = true;
>> +
>> + ret = of_property_read_u32(node, "brcm,num-eq-region",
>> + &msi->nr_eq_region);
>> + if (ret || !msi->nr_eq_region)
>> + msi->nr_eq_region = 1;
>> +
>> + ret = of_property_read_u32(node, "brcm,num-msi-msg-region",
>> + &msi->nr_msi_region);
>> + if (ret || !msi->nr_msi_region)
>> + msi->nr_msi_region = 1;
>> +
>> + msi->nr_irqs = of_irq_count(node);
>> + if (!msi->nr_irqs) {
>> + dev_err(pcie->dev, "found no MSI GIC interrupt\n");
>> + return -ENODEV;
>> + }
>> + if (msi->nr_irqs > NR_HW_IRQS) {
>> + dev_warn(pcie->dev, "too many MSI GIC interrupts defined %d\n",
>> + msi->nr_irqs);
>> + msi->nr_irqs = NR_HW_IRQS;
>> + }
>> +
>> + msi->nr_msi_vecs = msi->nr_irqs * EQ_LEN;
>> + msi->bitmap = devm_kcalloc(pcie->dev, BITS_TO_LONGS(msi->nr_msi_vecs),
>> + sizeof(*msi->bitmap), GFP_KERNEL);
>> + if (!msi->bitmap)
>> + return -ENOMEM;
>> +
>> + msi->grps = devm_kcalloc(pcie->dev, msi->nr_irqs, sizeof(*msi->grps),
>> + GFP_KERNEL);
>> + if (!msi->grps)
>> + return -ENOMEM;
>> +
>> + for (i = 0; i < msi->nr_irqs; i++) {
>> + unsigned int irq = irq_of_parse_and_map(node, i);
>> +
>> + if (!irq) {
>> + dev_err(pcie->dev, "unable to parse/map interrupt\n");
>> + return -ENODEV;
>> + }
>> + msi->grps[i].gic_irq = irq;
>> + msi->grps[i].msi = msi;
>> + msi->grps[i].eq = i;
>> + }
>> +
>> + /* reserve memory for MSI event queue */
>> + msi->eq_cpu = dma_alloc_coherent(pcie->dev,
>> + msi->nr_eq_region * EQ_MEM_REGION_SIZE,
>> + &msi->eq_dma, GFP_KERNEL);
>
> Do you need to zero that memory? Or is the HW happy with whatever will
> be there?
>

It seems to work fine. But you are right, it's much safer to zero the
memory since the PCIe controller could be using the upper 16-bit of each
word-sized entry in the event queue.

>> + if (!msi->eq_cpu)
>> + return -ENOMEM;
>> +
>> + /* reserve memory for MSI posted writes */
>> + msi->msi_cpu = dma_alloc_coherent(pcie->dev,
>> + msi->nr_msi_region * MSI_MEM_REGION_SIZE,
>> + &msi->msi_dma, GFP_KERNEL);
>
> Same here. Also, what is the exact purpose of that memory? You have a
> coherent mapping with the CPU, but you never read from it. So what's
> the point?
>

Yeah I guess I can change this back to kmalloc since coherent memory is
a scarce resource, and the CPU does not need to access the memory, so
there's no cache issue.

I know I have not answered the first part of your question. Let me do
some experiments first and I'll get back to you on that, :)

>> + if (!msi->msi_cpu) {
>> + ret = -ENOMEM;
>> + goto free_eq_dma;
>> + }
>> +
>> + ret = iproc_msi_alloc_domains(node, msi);
>> + if (ret) {
>> + dev_err(pcie->dev, "failed to create MSI domains\n");
>> + goto free_msi_dma;
>> + }
>> +
>> + for_each_online_cpu(cpu) {
>> + ret = iproc_msi_irq_setup(msi, cpu);
>> + if (ret)
>> + goto free_msi_irq;
>> + }
>> +
>> + iproc_msi_enable(msi);
>> +
>> + return 0;
>> +
>> +free_msi_irq:
>> + for_each_online_cpu(cpu)
>> + iproc_msi_irq_free(msi, cpu);
>> + iproc_msi_free_domains(msi);
>> +
>> +free_msi_dma:
>> + dma_free_coherent(pcie->dev, msi->nr_msi_region * MSI_MEM_REGION_SIZE,
>> + msi->msi_cpu, msi->msi_dma);
>> +free_eq_dma:
>> + dma_free_coherent(pcie->dev, msi->nr_eq_region * EQ_MEM_REGION_SIZE,
>> + msi->eq_cpu, msi->eq_dma);
>> + pcie->msi = NULL;
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(iproc_msi_init);
>
> [...]
>
> Thanks,
>
> M.
>

Thanks, Marc!

Ray

2015-11-26 05:14:40

by Ray Jui

[permalink] [raw]
Subject: Re: [PATCH v2 4/5] PCI: iproc: Add iProc PCIe MSI support

Hi Marc,

On 11/25/2015 5:52 PM, Ray Jui wrote:
> Hi Marc,
>
[...]
>>> + /* reserve memory for MSI posted writes */
>>> + msi->msi_cpu = dma_alloc_coherent(pcie->dev,
>>> + msi->nr_msi_region * MSI_MEM_REGION_SIZE,
>>> + &msi->msi_dma, GFP_KERNEL);
>>
>> Same here. Also, what is the exact purpose of that memory? You have a
>> coherent mapping with the CPU, but you never read from it. So what's
>> the point?
>>
>
> Yeah I guess I can change this back to kmalloc since coherent memory is
> a scarce resource, and the CPU does not need to access the memory, so
> there's no cache issue.
>
> I know I have not answered the first part of your question. Let me do
> some experiments first and I'll get back to you on that, :)
>

I did some experiment with the msi_dma here. It looks like it can be any
address as long as it's 4K aligned (i.e., can be from the device address
range instead of the RAM address range). The MSI message data actually
goes to the memory allocated for the event queue (makes sense...), and
never made it to the MSI page memory allocated here. Our arch doc is
just confusing....:(

I saw your comment on the other email thread with Xilinx MSI. I'll set
the address to the base address of the iProc PCIe controller (which is
always 4K aligned).

[...]
>>
>> Thanks,
>>
>> M.
>>
>
> Thanks, Marc!
>
> Ray

Thanks again!

Ray