[PATCH 0/2] Introduce ARM GICv2m MSI(-X) support

[PATCH 0/2] Introduce ARM GICv2m MSI(-X) support

[suravee.suthikulpanit]

From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

This patch set introduces support for MSI(-X) in GICv2m specification,
which is implemented in some variation of GIC400.

This depends on and has been tested with the V7 of "Add support for PCI in AArch64"
(https://lkml.org/lkml/2014/3/14/320).

Suravee Suthikulpanit (2):
  arm/gic: Add binding probe for GIC400
  arm/gic: Add supports for GICv2m MSI(-X)

 Documentation/devicetree/bindings/arm/gic.txt |  18 +-
 drivers/irqchip/Kconfig                       |   6 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/gic-msi-v2m.c                 | 249 ++++++++++++++++++++++++++
 drivers/irqchip/gic-msi-v2m.h                 |  20 +++
 drivers/irqchip/irq-gic.c                     |  23 ++-
 6 files changed, 313 insertions(+), 4 deletions(-)
 create mode 100644 drivers/irqchip/gic-msi-v2m.c
 create mode 100644 drivers/irqchip/gic-msi-v2m.h

-- 
1.9.0

[PATCH 1/2] arm/gic: Add binding probe for GIC400

[suravee.suthikulpanit]

From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

Add new Irqchip declaration for GIC400. This was mentioned in
gic binding documentation, but there is not code to support it.

Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 drivers/irqchip/irq-gic.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index 7e11c9d..adc86de 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -1071,6 +1071,8 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 	gic_cnt++;
 	return 0;
 }
+
+IRQCHIP_DECLARE(arm_gic_400, "arm,gic-400", gic_of_init);
 IRQCHIP_DECLARE(cortex_a15_gic, "arm,cortex-a15-gic", gic_of_init);
 IRQCHIP_DECLARE(cortex_a9_gic, "arm,cortex-a9-gic", gic_of_init);
 IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init);
-- 
1.9.0

Re: [PATCH 1/2] arm/gic: Add binding probe for GIC400

[Jason Cooper]

Suravee,

Please follow the convention for patch subject lines in this sub-system:

"irqchip: gic: ..."

thx,

Jason.

On Mon, Jun 23, 2014 at 07:32:59PM -0500, suravee.suthikulpanit@amd.com wrote:
> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> 
> Add new Irqchip declaration for GIC400. This was mentioned in
> gic binding documentation, but there is not code to support it.
> 
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>  drivers/irqchip/irq-gic.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index 7e11c9d..adc86de 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -1071,6 +1071,8 @@ gic_of_init(struct device_node *node, struct device_node *parent)
>  	gic_cnt++;
>  	return 0;
>  }
> +
> +IRQCHIP_DECLARE(arm_gic_400, "arm,gic-400", gic_of_init);
>  IRQCHIP_DECLARE(cortex_a15_gic, "arm,cortex-a15-gic", gic_of_init);
>  IRQCHIP_DECLARE(cortex_a9_gic, "arm,cortex-a9-gic", gic_of_init);
>  IRQCHIP_DECLARE(msm_8660_qgic, "qcom,msm-8660-qgic", gic_of_init);
> -- 
> 1.9.0
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)

[suravee.suthikulpanit]

From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>

GICv2m extends GICv2 to support MSI(-X) with a new set of register frames.

This patch introduces support for the non-secure GICv2m register frame.
This is optional. It uses the "msi-controller" keyword in ARM GIC
devicetree binding to indentify GIC driver that it should enable MSI(-X)
support, The region of GICv2m MSI register frame is specified using the register
frame index 4 in the device tree.

Each GIC maintains an "msi_chip" structure. To discover the msi_chip,
PCI host driver can do the following:

    struct device_node *gic_node = of_irq_find_parent(pdev->dev.of_node);
    pcie_bus->msi_chip = of_pci_find_msi_chip_by_node(gic_node);

Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
---
 Documentation/devicetree/bindings/arm/gic.txt |  18 +-
 drivers/irqchip/Kconfig                       |   6 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/gic-msi-v2m.c                 | 249 ++++++++++++++++++++++++++
 drivers/irqchip/gic-msi-v2m.h                 |  20 +++
 drivers/irqchip/irq-gic.c                     |  21 ++-
 6 files changed, 311 insertions(+), 4 deletions(-)
 create mode 100644 drivers/irqchip/gic-msi-v2m.c
 create mode 100644 drivers/irqchip/gic-msi-v2m.h

diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
index 5573c08..ee4bc02 100644
--- a/Documentation/devicetree/bindings/arm/gic.txt
+++ b/Documentation/devicetree/bindings/arm/gic.txt
@@ -17,6 +17,7 @@ Main node required properties:
 	"arm,cortex-a7-gic"
 	"arm,arm11mp-gic"
 - interrupt-controller : Identifies the node as an interrupt controller
+
 - #interrupt-cells : Specifies the number of cells needed to encode an
   interrupt source.  The type shall be a <u32> and the value shall be 3.
 
@@ -37,9 +38,16 @@ Main node required properties:
 	the 8 possible cpus attached to the GIC.  A bit set to '1' indicated
 	the interrupt is wired to that CPU.  Only valid for PPI interrupts.
 
-- reg : Specifies base physical address(s) and size of the GIC registers. The
-  first region is the GIC distributor register base and size. The 2nd region is
-  the GIC cpu interface register base and size.
+- reg : Specifies base physical address(s) and size of the GIC register frames.
+
+  Region | Description
+  Index  |
+  -------------------------------------------------------------------
+     0   | GIC distributor register base and size
+     1   | GIC cpu interface register base and size
+     2   | VGIC interface control register base and size (Optional)
+     3   | VGIC CPU interface register base and size (Optional)
+     4   | GICv2m MSI interface register base and size (Optional)
 
 Optional
 - interrupts	: Interrupt source of the parent interrupt controller on
@@ -55,6 +63,10 @@ Optional
 		  by a crossbar/multiplexer preceding the GIC. The GIC irq
 		  input line is assigned dynamically when the corresponding
 		  peripheral's crossbar line is mapped.
+
+- msi-controller : Identifies the node as an MSI controller.  This requires
+  the register region index 4.
+
 Example:
 
 	intc: interrupt-controller@fff11000 {
diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index bbb746e..a36bf94 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -7,6 +7,12 @@ config ARM_GIC
 	select IRQ_DOMAIN
 	select MULTI_IRQ_HANDLER
 
+config ARM_GIC_MSI_V2M
+	bool
+	select IRQ_DOMAIN
+	select MULTI_IRQ_HANDLER
+	depends on PCI && PCI_MSI
+
 config GIC_NON_BANKED
 	bool
 
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 62a13e5..d43f904 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -16,6 +16,7 @@ obj-$(CONFIG_ARCH_SUNXI)		+= irq-sun4i.o
 obj-$(CONFIG_ARCH_SUNXI)		+= irq-sunxi-nmi.o
 obj-$(CONFIG_ARCH_SPEAR3XX)		+= spear-shirq.o
 obj-$(CONFIG_ARM_GIC)			+= irq-gic.o
+obj-$(CONFIG_ARM_GIC_MSI_V2M)		+= gic-msi-v2m.o
 obj-$(CONFIG_ARM_NVIC)			+= irq-nvic.o
 obj-$(CONFIG_ARM_VIC)			+= irq-vic.o
 obj-$(CONFIG_IMGPDC_IRQ)		+= irq-imgpdc.o
diff --git a/drivers/irqchip/gic-msi-v2m.c b/drivers/irqchip/gic-msi-v2m.c
new file mode 100644
index 0000000..e5c0f79
--- /dev/null
+++ b/drivers/irqchip/gic-msi-v2m.c
@@ -0,0 +1,249 @@
+/*
+ * ARM GIC v2m MSI support
+ * Support for Message Signalelled Interrupts for systems that
+ * implement ARM Generic Interrupt Controller: GICv2m.
+ *
+ * Copyright (C) 2014 Advanced Micro Devices, Inc.
+ * Authors: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
+ *          Harish Kasiviswanathan <harish.kasiviswanathan@amd.com>
+ *          Brandon Anderson <brandon.anderson@amd.com>
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 as published
+ * by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/pci.h>
+#include <linux/irq.h>
+#include <linux/spinlock.h>
+#include <linux/slab.h>
+#include <linux/of_address.h>
+#include <linux/bitmap.h>
+
+#include "gic-msi-v2m.h"
+
+/* GICv2m MSI Registers */
+#define MSI_TYPER			0x008
+#define MSI_SETSPI_NS			0x040
+#define GIC_V2M_MIN_SPI			32
+#define GIC_V2M_MAX_SPI			1024
+#define GIC_OF_MSIV2M_RANGE_INDEX	4
+
+extern struct irq_chip gic_arch_extn;
+
+/**
+ * alloc_msi_irq - Allocate MSIs from avaialbe MSI bitmap.
+ * @data: Pointer to gicv2m_msi_data
+ * @nvec: Number of interrupts to allocate
+ * @irq: Pointer to the allocated irq
+ *
+ * Allocates interrupts only if the contiguous range of MSIs
+ * with specified nvec are avaiable. Otherwise return the number
+ * of avaiable interrupts. If none is avaiable, then returns -ENOENT.
+ */
+static int alloc_msi_irq(struct gicv2m_msi_data *data, int nvec, int *irq)
+{
+	int size = sizeof(unsigned long) * data->bm_sz;
+	int next = size, ret, num;
+
+	spin_lock(&data->msi_cnt_lock);
+
+	for (num = nvec; num > 0; num--) {
+		next = bitmap_find_next_zero_area(data->bm,
+					size, 0, num, 0);
+		if (next < size)
+			break;
+	}
+
+	if (next < size) {
+		bitmap_set(data->bm, next, nvec);
+		*irq = data->spi_start + next;
+		ret = 0;
+	} else if (num != 0) {
+		ret = num;
+	} else {
+		ret = -ENOENT;
+	}
+
+
+	spin_unlock(&data->msi_cnt_lock);
+
+	return ret;
+}
+
+static void free_msi_irq(struct gicv2m_msi_data *data, unsigned int irq)
+{
+	int pos;
+
+	spin_lock(&data->msi_cnt_lock);
+
+	pos = irq - data->spi_start;
+	if (pos >= 0 && pos < data->max_spi_cnt)
+		bitmap_clear(data->bm, pos, 1);
+
+	spin_unlock(&data->msi_cnt_lock);
+}
+
+static inline struct gicv2m_msi_data *to_gicv2m_msi_data(struct msi_chip *chip)
+{
+	return container_of(chip, struct gicv2m_msi_data, chip);
+}
+
+static void gicv2m_teardown_msi_irq(struct msi_chip *chip, unsigned int irq)
+{
+	free_msi_irq(to_gicv2m_msi_data(chip), irq);
+}
+
+static int gicv2m_setup_msi_irq(struct msi_chip *chip, struct pci_dev *pdev,
+		      struct msi_desc *desc)
+{
+	int avail, irq = 0;
+	struct msi_msg msg;
+	phys_addr_t addr;
+	struct gicv2m_msi_data *data = to_gicv2m_msi_data(chip);
+
+	if (data == NULL) {
+		dev_err(&pdev->dev,
+			"GICv2m: MSI setup failed. Invalid platform data\n");
+		return -EINVAL;
+	}
+
+	if (!desc) {
+		dev_err(&pdev->dev,
+			"GICv2m: MSI setup failed. Invalid msi descriptor\n");
+		return -EINVAL;
+	}
+
+	avail = alloc_msi_irq(data, 1, &irq);
+	if (avail != 0) {
+		dev_err(&pdev->dev,
+			"GICv2m: MSI setup failed. Cannnot allocate IRQ\n");
+		return -ENOSPC;
+	}
+
+	irq_set_msi_desc(irq, desc);
+	irq_set_irq_type(irq, IRQ_TYPE_EDGE_RISING);
+
+	addr = (unsigned long)(data->paddr64 + MSI_SETSPI_NS);
+
+	msg.address_hi = (unsigned int)(addr >> 32);
+	msg.address_lo = (unsigned int)(addr);
+	msg.data = irq;
+#ifdef CONFIG_PCI_MSI
+	write_msi_msg(irq, &msg);
+#endif
+
+	return 0;
+}
+
+static void gicv2m_mask_msi(struct irq_data *d)
+{
+	if (d->msi_desc)
+		mask_msi_irq(d);
+}
+
+static void gicv2m_unmask_msi(struct irq_data *d)
+{
+	if (d->msi_desc)
+		unmask_msi_irq(d);
+}
+
+int __init gicv2m_msi_init(struct device_node *node,
+			   struct gicv2m_msi_data *msi)
+{
+	unsigned int val;
+	const __be32 *msi_be;
+
+	msi_be = of_get_address(node, GIC_OF_MSIV2M_RANGE_INDEX, NULL, NULL);
+	if (!msi_be) {
+		pr_err("GICv2m failed. Fail to locate MSI base.\n");
+		return -EINVAL;
+	}
+
+	msi->paddr64 = of_translate_address(node, msi_be);
+	msi->base = of_iomap(node, GIC_OF_MSIV2M_RANGE_INDEX);
+
+	/*
+	* MSI_TYPER:
+	*     [31:26] Reserved
+	*     [25:16] lowest SPI assigned to MSI
+	*     [15:10] Reserved
+	*     [9:0]   Numer of SPIs assigned to MSI
+	*/
+	val = __raw_readl(msi->base + MSI_TYPER);
+	if (!val) {
+		pr_warn("GICv2m: Failed to read MSI_TYPER register\n");
+		return -EINVAL;
+	}
+
+	msi->spi_start = (val >> 16) & 0x3ff;
+	msi->max_spi_cnt = val & 0x3ff;
+
+	pr_debug("GICv2m: SPI = %u, cnt = %u\n",
+		msi->spi_start, msi->max_spi_cnt);
+
+	if (msi->spi_start < GIC_V2M_MIN_SPI ||
+		msi->max_spi_cnt >= GIC_V2M_MAX_SPI) {
+			pr_err("GICv2m: Init failed\n");
+			return -EINVAL;
+	}
+
+	msi->bm_sz = BITS_TO_LONGS(msi->max_spi_cnt);
+	msi->bm = kzalloc(msi->bm_sz, GFP_KERNEL);
+	if (!msi->bm)
+		return -ENOMEM;
+
+	spin_lock_init(&msi->msi_cnt_lock);
+
+	msi->chip.owner = THIS_MODULE;
+	msi->chip.of_node = node;
+	msi->chip.setup_irq = gicv2m_setup_msi_irq;
+	msi->chip.teardown_irq = gicv2m_teardown_msi_irq;
+
+	pr_info("GICv2m: SPI range [%d:%d]\n",
+		msi->spi_start, (msi->spi_start + msi->max_spi_cnt));
+
+	gic_arch_extn.irq_mask = gicv2m_mask_msi;
+	gic_arch_extn.irq_unmask = gicv2m_unmask_msi;
+
+	return 0;
+}
+EXPORT_SYMBOL(gicv2m_msi_init);
+
+
+
+/**
+ * Override arch_setup_msi_irq in drivers/pci/msi.c
+ * since we don't want to change the chip_data
+ */
+int arch_setup_msi_irq(struct pci_dev *pdev, struct msi_desc *desc)
+{
+	struct msi_chip *chip = pdev->bus->msi;
+
+	if (!chip || !chip->setup_irq)
+		return -EINVAL;
+
+	return chip->setup_irq(chip, pdev, desc);
+}
+
+/**
+ * Override arch_teardown_msi_irq in drivers/pci/msi.c
+ */
+void arch_teardown_msi_irq(unsigned int irq)
+{
+	struct msi_desc *desc;
+	struct msi_chip *chip;
+
+	desc = irq_get_msi_desc(irq);
+	if (!desc)
+		return;
+
+	chip = desc->dev->bus->msi;
+	if (!chip)
+		return;
+
+	chip->teardown_irq(chip, irq);
+}
diff --git a/drivers/irqchip/gic-msi-v2m.h b/drivers/irqchip/gic-msi-v2m.h
new file mode 100644
index 0000000..164d929
--- /dev/null
+++ b/drivers/irqchip/gic-msi-v2m.h
@@ -0,0 +1,20 @@
+#ifndef GIC_MSI_V2M_H
+#define GIC_MSI_V2M_H
+
+#include <linux/msi.h>
+
+struct gicv2m_msi_data {
+	struct msi_chip chip;
+	spinlock_t msi_cnt_lock;
+	u64 paddr64;              /* GICv2m phys address */
+	void __iomem *base;       /* GICv2m virt address */
+	unsigned int spi_start;   /* The SPI number that MSIs start */
+	unsigned int max_spi_cnt; /* The number of SPIs for MSIs */
+	unsigned long *bm;        /* MSI vector bitmap */
+	unsigned long bm_sz;      /* MSI bitmap size */
+};
+
+extern int gicv2m_msi_init(struct device_node *node,
+			   struct gicv2m_msi_data *msi) __init;
+
+#endif /*GIC_MSI_V2M_H*/
diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
index adc86de..dcff99b 100644
--- a/drivers/irqchip/irq-gic.c
+++ b/drivers/irqchip/irq-gic.c
@@ -35,6 +35,7 @@
 #include <linux/of.h>
 #include <linux/of_address.h>
 #include <linux/of_irq.h>
+#include <linux/of_pci.h>
 #include <linux/irqdomain.h>
 #include <linux/interrupt.h>
 #include <linux/percpu.h>
@@ -48,6 +49,10 @@
 
 #include "irqchip.h"
 
+#ifdef CONFIG_ARM_GIC_MSI_V2M
+#include "gic-msi-v2m.h"
+#endif
+
 union gic_base {
 	void __iomem *common_base;
 	void __percpu * __iomem *percpu_base;
@@ -68,6 +73,9 @@ struct gic_chip_data {
 #ifdef CONFIG_GIC_NON_BANKED
 	void __iomem *(*get_base)(union gic_base *);
 #endif
+#ifdef CONFIG_ARM_GIC_MSI_V2M
+	struct gicv2m_msi_data msi_data;
+#endif
 };
 
 static DEFINE_RAW_SPINLOCK(irq_controller_lock);
@@ -246,7 +254,8 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
 			    bool force)
 {
 	void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3);
-	unsigned int cpu, shift = (gic_irq(d) % 4) * 8;
+	unsigned int shift = (gic_irq(d) % 4) * 8;
+	unsigned int cpu = cpumask_any_and(mask_val, cpu_online_mask);
 	u32 val, mask, bit;
 
 	if (!force)
@@ -1047,6 +1056,7 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 	void __iomem *dist_base;
 	u32 percpu_offset;
 	int irq;
+	struct gic_chip_data *gic;
 
 	if (WARN_ON(!node))
 		return -ENODEV;
@@ -1068,6 +1078,15 @@ gic_of_init(struct device_node *node, struct device_node *parent)
 		irq = irq_of_parse_and_map(node, 0);
 		gic_cascade_irq(gic_cnt, irq);
 	}
+
+#ifdef CONFIG_ARM_GIC_MSI_V2M
+	if (of_find_property(node, "msi-controller", NULL)) {
+		gic = &gic_data[gic_cnt];
+		if (!gicv2m_msi_init(node, &gic->msi_data))
+			of_pci_msi_chip_add(&gic->msi_data.chip);
+	}
+#endif
+
 	gic_cnt++;
 	return 0;
 }
-- 
1.9.0

Re: [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)

[Marc Zyngier]

Hi Suravee,

On 24/06/14 01:33, suravee.suthikulpanit@amd.com wrote:
> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> 
> GICv2m extends GICv2 to support MSI(-X) with a new set of register frames.
> 
> This patch introduces support for the non-secure GICv2m register frame.
> This is optional. It uses the "msi-controller" keyword in ARM GIC
> devicetree binding to indentify GIC driver that it should enable MSI(-X)
> support, The region of GICv2m MSI register frame is specified using the register
> frame index 4 in the device tree.
> 
> Each GIC maintains an "msi_chip" structure. To discover the msi_chip,
> PCI host driver can do the following:
> 
>     struct device_node *gic_node = of_irq_find_parent(pdev->dev.of_node);
>     pcie_bus->msi_chip = of_pci_find_msi_chip_by_node(gic_node);
> 
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>  Documentation/devicetree/bindings/arm/gic.txt |  18 +-
>  drivers/irqchip/Kconfig                       |   6 +
>  drivers/irqchip/Makefile                      |   1 +
>  drivers/irqchip/gic-msi-v2m.c                 | 249 ++++++++++++++++++++++++++
>  drivers/irqchip/gic-msi-v2m.h                 |  20 +++
>  drivers/irqchip/irq-gic.c                     |  21 ++-
>  6 files changed, 311 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/irqchip/gic-msi-v2m.c
>  create mode 100644 drivers/irqchip/gic-msi-v2m.h
> 
> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
> index 5573c08..ee4bc02 100644
> --- a/Documentation/devicetree/bindings/arm/gic.txt
> +++ b/Documentation/devicetree/bindings/arm/gic.txt
> @@ -17,6 +17,7 @@ Main node required properties:
>         "arm,cortex-a7-gic"
>         "arm,arm11mp-gic"
>  - interrupt-controller : Identifies the node as an interrupt controller
> +
>  - #interrupt-cells : Specifies the number of cells needed to encode an
>    interrupt source.  The type shall be a <u32> and the value shall be 3.
> 
> @@ -37,9 +38,16 @@ Main node required properties:
>         the 8 possible cpus attached to the GIC.  A bit set to '1' indicated
>         the interrupt is wired to that CPU.  Only valid for PPI interrupts.
> 
> -- reg : Specifies base physical address(s) and size of the GIC registers. The
> -  first region is the GIC distributor register base and size. The 2nd region is
> -  the GIC cpu interface register base and size.
> +- reg : Specifies base physical address(s) and size of the GIC register frames.
> +
> +  Region | Description
> +  Index  |
> +  -------------------------------------------------------------------
> +     0   | GIC distributor register base and size
> +     1   | GIC cpu interface register base and size
> +     2   | VGIC interface control register base and size (Optional)
> +     3   | VGIC CPU interface register base and size (Optional)
> +     4   | GICv2m MSI interface register base and size (Optional)
> 
>  Optional
>  - interrupts   : Interrupt source of the parent interrupt controller on
> @@ -55,6 +63,10 @@ Optional
>                   by a crossbar/multiplexer preceding the GIC. The GIC irq
>                   input line is assigned dynamically when the corresponding
>                   peripheral's crossbar line is mapped.
> +
> +- msi-controller : Identifies the node as an MSI controller.  This requires
> +  the register region index 4.
> +
>  Example:
> 
>         intc: interrupt-controller@fff11000 {
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index bbb746e..a36bf94 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -7,6 +7,12 @@ config ARM_GIC
>         select IRQ_DOMAIN
>         select MULTI_IRQ_HANDLER
> 
> +config ARM_GIC_MSI_V2M
> +       bool
> +       select IRQ_DOMAIN
> +       select MULTI_IRQ_HANDLER
> +       depends on PCI && PCI_MSI
> +
>  config GIC_NON_BANKED
>         bool
> 
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 62a13e5..d43f904 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_ARCH_SUNXI)              += irq-sun4i.o
>  obj-$(CONFIG_ARCH_SUNXI)               += irq-sunxi-nmi.o
>  obj-$(CONFIG_ARCH_SPEAR3XX)            += spear-shirq.o
>  obj-$(CONFIG_ARM_GIC)                  += irq-gic.o
> +obj-$(CONFIG_ARM_GIC_MSI_V2M)          += gic-msi-v2m.o
>  obj-$(CONFIG_ARM_NVIC)                 += irq-nvic.o
>  obj-$(CONFIG_ARM_VIC)                  += irq-vic.o
>  obj-$(CONFIG_IMGPDC_IRQ)               += irq-imgpdc.o
> diff --git a/drivers/irqchip/gic-msi-v2m.c b/drivers/irqchip/gic-msi-v2m.c
> new file mode 100644
> index 0000000..e5c0f79
> --- /dev/null
> +++ b/drivers/irqchip/gic-msi-v2m.c
> @@ -0,0 +1,249 @@
> +/*
> + * ARM GIC v2m MSI support
> + * Support for Message Signalelled Interrupts for systems that
> + * implement ARM Generic Interrupt Controller: GICv2m.
> + *
> + * Copyright (C) 2014 Advanced Micro Devices, Inc.
> + * Authors: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> + *          Harish Kasiviswanathan <harish.kasiviswanathan@amd.com>
> + *          Brandon Anderson <brandon.anderson@amd.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pci.h>
> +#include <linux/irq.h>
> +#include <linux/spinlock.h>
> +#include <linux/slab.h>
> +#include <linux/of_address.h>
> +#include <linux/bitmap.h>
> +
> +#include "gic-msi-v2m.h"
> +
> +/* GICv2m MSI Registers */
> +#define MSI_TYPER                      0x008
> +#define MSI_SETSPI_NS                  0x040
> +#define GIC_V2M_MIN_SPI                        32
> +#define GIC_V2M_MAX_SPI                        1024
> +#define GIC_OF_MSIV2M_RANGE_INDEX      4
> +
> +extern struct irq_chip gic_arch_extn;
> +
> +/**
> + * alloc_msi_irq - Allocate MSIs from avaialbe MSI bitmap.
> + * @data: Pointer to gicv2m_msi_data
> + * @nvec: Number of interrupts to allocate
> + * @irq: Pointer to the allocated irq
> + *
> + * Allocates interrupts only if the contiguous range of MSIs
> + * with specified nvec are avaiable. Otherwise return the number
> + * of avaiable interrupts. If none is avaiable, then returns -ENOENT.
> + */
> +static int alloc_msi_irq(struct gicv2m_msi_data *data, int nvec, int *irq)
> +{
> +       int size = sizeof(unsigned long) * data->bm_sz;
> +       int next = size, ret, num;
> +
> +       spin_lock(&data->msi_cnt_lock);
> +
> +       for (num = nvec; num > 0; num--) {
> +               next = bitmap_find_next_zero_area(data->bm,
> +                                       size, 0, num, 0);
> +               if (next < size)
> +                       break;
> +       }
> +
> +       if (next < size) {
> +               bitmap_set(data->bm, next, nvec);
> +               *irq = data->spi_start + next;
> +               ret = 0;
> +       } else if (num != 0) {
> +               ret = num;
> +       } else {
> +               ret = -ENOENT;
> +       }
> +
> +
> +       spin_unlock(&data->msi_cnt_lock);
> +
> +       return ret;
> +}
> +
> +static void free_msi_irq(struct gicv2m_msi_data *data, unsigned int irq)
> +{
> +       int pos;
> +
> +       spin_lock(&data->msi_cnt_lock);
> +
> +       pos = irq - data->spi_start;
> +       if (pos >= 0 && pos < data->max_spi_cnt)
> +               bitmap_clear(data->bm, pos, 1);
> +
> +       spin_unlock(&data->msi_cnt_lock);
> +}
> +
> +static inline struct gicv2m_msi_data *to_gicv2m_msi_data(struct msi_chip *chip)
> +{
> +       return container_of(chip, struct gicv2m_msi_data, chip);
> +}
> +
> +static void gicv2m_teardown_msi_irq(struct msi_chip *chip, unsigned int irq)
> +{
> +       free_msi_irq(to_gicv2m_msi_data(chip), irq);
> +}
> +
> +static int gicv2m_setup_msi_irq(struct msi_chip *chip, struct pci_dev *pdev,
> +                     struct msi_desc *desc)
> +{
> +       int avail, irq = 0;
> +       struct msi_msg msg;
> +       phys_addr_t addr;
> +       struct gicv2m_msi_data *data = to_gicv2m_msi_data(chip);
> +
> +       if (data == NULL) {
> +               dev_err(&pdev->dev,
> +                       "GICv2m: MSI setup failed. Invalid platform data\n");
> +               return -EINVAL;
> +       }
> +
> +       if (!desc) {
> +               dev_err(&pdev->dev,
> +                       "GICv2m: MSI setup failed. Invalid msi descriptor\n");
> +               return -EINVAL;
> +       }
> +
> +       avail = alloc_msi_irq(data, 1, &irq);
> +       if (avail != 0) {
> +               dev_err(&pdev->dev,
> +                       "GICv2m: MSI setup failed. Cannnot allocate IRQ\n");
> +               return -ENOSPC;
> +       }
> +
> +       irq_set_msi_desc(irq, desc);
> +       irq_set_irq_type(irq, IRQ_TYPE_EDGE_RISING);
> +
> +       addr = (unsigned long)(data->paddr64 + MSI_SETSPI_NS);
> +
> +       msg.address_hi = (unsigned int)(addr >> 32);
> +       msg.address_lo = (unsigned int)(addr);
> +       msg.data = irq;
> +#ifdef CONFIG_PCI_MSI
> +       write_msi_msg(irq, &msg);
> +#endif
> +
> +       return 0;
> +}
> +
> +static void gicv2m_mask_msi(struct irq_data *d)
> +{
> +       if (d->msi_desc)
> +               mask_msi_irq(d);
> +}

Under which circumstance can d->msi_desc be NULL? Why should we care?

> +static void gicv2m_unmask_msi(struct irq_data *d)
> +{
> +       if (d->msi_desc)
> +               unmask_msi_irq(d);
> +}
> +
> +int __init gicv2m_msi_init(struct device_node *node,
> +                          struct gicv2m_msi_data *msi)
> +{
> +       unsigned int val;
> +       const __be32 *msi_be;
> +
> +       msi_be = of_get_address(node, GIC_OF_MSIV2M_RANGE_INDEX, NULL, NULL);
> +       if (!msi_be) {
> +               pr_err("GICv2m failed. Fail to locate MSI base.\n");
> +               return -EINVAL;
> +       }
> +
> +       msi->paddr64 = of_translate_address(node, msi_be);
> +       msi->base = of_iomap(node, GIC_OF_MSIV2M_RANGE_INDEX);
> +
> +       /*
> +       * MSI_TYPER:
> +       *     [31:26] Reserved
> +       *     [25:16] lowest SPI assigned to MSI
> +       *     [15:10] Reserved
> +       *     [9:0]   Numer of SPIs assigned to MSI
> +       */
> +       val = __raw_readl(msi->base + MSI_TYPER);
> +       if (!val) {
> +               pr_warn("GICv2m: Failed to read MSI_TYPER register\n");
> +               return -EINVAL;
> +       }
> +
> +       msi->spi_start = (val >> 16) & 0x3ff;
> +       msi->max_spi_cnt = val & 0x3ff;
> +
> +       pr_debug("GICv2m: SPI = %u, cnt = %u\n",
> +               msi->spi_start, msi->max_spi_cnt);
> +
> +       if (msi->spi_start < GIC_V2M_MIN_SPI ||
> +               msi->max_spi_cnt >= GIC_V2M_MAX_SPI) {
> +                       pr_err("GICv2m: Init failed\n");
> +                       return -EINVAL;
> +       }
> +
> +       msi->bm_sz = BITS_TO_LONGS(msi->max_spi_cnt);
> +       msi->bm = kzalloc(msi->bm_sz, GFP_KERNEL);
> +       if (!msi->bm)
> +               return -ENOMEM;
> +
> +       spin_lock_init(&msi->msi_cnt_lock);
> +
> +       msi->chip.owner = THIS_MODULE;
> +       msi->chip.of_node = node;
> +       msi->chip.setup_irq = gicv2m_setup_msi_irq;
> +       msi->chip.teardown_irq = gicv2m_teardown_msi_irq;
> +
> +       pr_info("GICv2m: SPI range [%d:%d]\n",
> +               msi->spi_start, (msi->spi_start + msi->max_spi_cnt));
> +
> +       gic_arch_extn.irq_mask = gicv2m_mask_msi;
> +       gic_arch_extn.irq_unmask = gicv2m_unmask_msi;

Right, I now see why you need to test on the MSI descriptor. Don't do
that. The gic_arch_extn crap should *never* *be* *used*.

What you want to do is do assign a different irq_chip to your MSI
interrupts. This will require a different integration with the main GIC
code though. For the GICv3 ITS, I do it when the interrupt gets mapped.


> +       return 0;
> +}
> +EXPORT_SYMBOL(gicv2m_msi_init);
> +
> +
> +
> +/**
> + * Override arch_setup_msi_irq in drivers/pci/msi.c
> + * since we don't want to change the chip_data
> + */
> +int arch_setup_msi_irq(struct pci_dev *pdev, struct msi_desc *desc)
> +{
> +       struct msi_chip *chip = pdev->bus->msi;
> +
> +       if (!chip || !chip->setup_irq)
> +               return -EINVAL;
> +
> +       return chip->setup_irq(chip, pdev, desc);
> +}
> +
> +/**
> + * Override arch_teardown_msi_irq in drivers/pci/msi.c
> + */
> +void arch_teardown_msi_irq(unsigned int irq)
> +{
> +       struct msi_desc *desc;
> +       struct msi_chip *chip;
> +
> +       desc = irq_get_msi_desc(irq);
> +       if (!desc)
> +               return;
> +
> +       chip = desc->dev->bus->msi;
> +       if (!chip)
> +               return;
> +
> +       chip->teardown_irq(chip, irq);
> +}

Please don't overtide those. There shouldn't be any reason for a
*driver* to do so. Only architecture code should do it. And nothing in
your code requires it (at least once you've stopped playing with the
silly gic extension...).

> diff --git a/drivers/irqchip/gic-msi-v2m.h b/drivers/irqchip/gic-msi-v2m.h
> new file mode 100644
> index 0000000..164d929
> --- /dev/null
> +++ b/drivers/irqchip/gic-msi-v2m.h
> @@ -0,0 +1,20 @@
> +#ifndef GIC_MSI_V2M_H
> +#define GIC_MSI_V2M_H
> +
> +#include <linux/msi.h>
> +
> +struct gicv2m_msi_data {
> +       struct msi_chip chip;
> +       spinlock_t msi_cnt_lock;
> +       u64 paddr64;              /* GICv2m phys address */
> +       void __iomem *base;       /* GICv2m virt address */
> +       unsigned int spi_start;   /* The SPI number that MSIs start */
> +       unsigned int max_spi_cnt; /* The number of SPIs for MSIs */
> +       unsigned long *bm;        /* MSI vector bitmap */
> +       unsigned long bm_sz;      /* MSI bitmap size */
> +};
> +
> +extern int gicv2m_msi_init(struct device_node *node,
> +                          struct gicv2m_msi_data *msi) __init;
> +
> +#endif /*GIC_MSI_V2M_H*/
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index adc86de..dcff99b 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -35,6 +35,7 @@
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
> +#include <linux/of_pci.h>
>  #include <linux/irqdomain.h>
>  #include <linux/interrupt.h>
>  #include <linux/percpu.h>
> @@ -48,6 +49,10 @@
> 
>  #include "irqchip.h"
> 
> +#ifdef CONFIG_ARM_GIC_MSI_V2M
> +#include "gic-msi-v2m.h"
> +#endif
> +
>  union gic_base {
>         void __iomem *common_base;
>         void __percpu * __iomem *percpu_base;
> @@ -68,6 +73,9 @@ struct gic_chip_data {
>  #ifdef CONFIG_GIC_NON_BANKED
>         void __iomem *(*get_base)(union gic_base *);
>  #endif
> +#ifdef CONFIG_ARM_GIC_MSI_V2M
> +       struct gicv2m_msi_data msi_data;
> +#endif
>  };
> 
>  static DEFINE_RAW_SPINLOCK(irq_controller_lock);
> @@ -246,7 +254,8 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
>                             bool force)
>  {
>         void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3);
> -       unsigned int cpu, shift = (gic_irq(d) % 4) * 8;
> +       unsigned int shift = (gic_irq(d) % 4) * 8;
> +       unsigned int cpu = cpumask_any_and(mask_val, cpu_online_mask);
>         u32 val, mask, bit;
> 
>         if (!force)
> @@ -1047,6 +1056,7 @@ gic_of_init(struct device_node *node, struct device_node *parent)
>         void __iomem *dist_base;
>         u32 percpu_offset;
>         int irq;
> +       struct gic_chip_data *gic;
> 
>         if (WARN_ON(!node))
>                 return -ENODEV;
> @@ -1068,6 +1078,15 @@ gic_of_init(struct device_node *node, struct device_node *parent)
>                 irq = irq_of_parse_and_map(node, 0);
>                 gic_cascade_irq(gic_cnt, irq);
>         }
> +
> +#ifdef CONFIG_ARM_GIC_MSI_V2M
> +       if (of_find_property(node, "msi-controller", NULL)) {
> +               gic = &gic_data[gic_cnt];
> +               if (!gicv2m_msi_init(node, &gic->msi_data))
> +                       of_pci_msi_chip_add(&gic->msi_data.chip);
> +       }
> +#endif
> +
>         gic_cnt++;
>         return 0;
>  }
> --
> 1.9.0
> 
> 

Overall, this requires to be re-architected. If you want to have a look
at the way I did the GICv3 ITS support:

git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
gicv3/its

Thanks,

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

Re: [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)

[Suravee Suthikulanit]

On 6/24/2014 4:52 AM, Marc Zyngier wrote:
> Overall, this requires to be re-architected. If you want to have a look
> at the way I did the GICv3 ITS support:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git
> gicv3/its
>
> Thanks,

Thanks for the review comments. I'll take a look at the GICv3 and 
re-architect for the V2 patch set.

Suravee

Re: [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)

[Suravee Suthikulpanit]

Hi Marc,

After looking at the GICv3 implementation and trying to understand how 
you architect the driver, I have a couple questions below.

 > On 06/24/2014 04:52 AM, Marc Zyngier wrote:
> Hi Suravee,
>
> On 24/06/14 01:33, suravee.suthikulpanit@amd.com wrote:
>> +       pr_info("GICv2m: SPI range [%d:%d]\n",
>> +               msi->spi_start, (msi->spi_start + msi->max_spi_cnt));
>> +
>> +       gic_arch_extn.irq_mask = gicv2m_mask_msi;
>> +       gic_arch_extn.irq_unmask = gicv2m_unmask_msi;
>
> Right, I now see why you need to test on the MSI descriptor. Don't do
> that. The gic_arch_extn crap should *never* *be* *used*.

Hm, sounds like this should be removed all together then? In that case, 
this would require changes in the irq-gic.c to call these functions 
directly.

>
> What you want to do is do assign a different irq_chip to your MSI
> interrupts. This will require a different integration with the main GIC
> code though. For the GICv3 ITS, I do it when the interrupt gets mapped.

Ah, that's what I was trying to avoid. Why should we need a whole 
different irq_chip just to add the MSI register frame support on top of 
the GICv2 which is already supported by the current irq-gic.c?

>
>> +       return 0;
>> +}
>> +EXPORT_SYMBOL(gicv2m_msi_init);
>> +
>> +
>> +
>> +/**
>> + * Override arch_setup_msi_irq in drivers/pci/msi.c
>> + * since we don't want to change the chip_data
>> + */
>> +int arch_setup_msi_irq(struct pci_dev *pdev, struct msi_desc *desc)
>> +{
>> +       struct msi_chip *chip = pdev->bus->msi;
>> +
>> +       if (!chip || !chip->setup_irq)
>> +               return -EINVAL;
>> +
>> +       return chip->setup_irq(chip, pdev, desc);
>> +}
>> +
>> +/**
>> + * Override arch_teardown_msi_irq in drivers/pci/msi.c
>> + */
>> +void arch_teardown_msi_irq(unsigned int irq)
>> +{
>> +       struct msi_desc *desc;
>> +       struct msi_chip *chip;
>> +
>> +       desc = irq_get_msi_desc(irq);
>> +       if (!desc)
>> +               return;a
>> +
>> +       chip = desc->dev->bus->msi;
>> +       if (!chip)
>> +               return;
>> +
>> +       chip->teardown_irq(chip, irq);
>> +}
>
> Please don't overtide those. There shouldn't be any reason for a
> *driver* to do so. Only architecture code should do it. And nothing in
> your code requires it (at least once you've stopped playing with the
> silly gic extension...).

The reason I need these because the __weak version of arch_setup_msi_irq 
function in driver/pci/msi.c calls irq_set_chip_data and replace the 
chip_data with msi_chip (originally was pointing to the gic_chip_data 
structure). This ended up breaking the GIC.

Looking at the GICv3 ITS implementation, this seems to also have similar 
implementation. So, this was not an issue for you?

Thanks,

Suravee

Re: [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)

[Marc Zyngier]

Hi Suravee,

On 27/06/14 16:43, Suravee Suthikulpanit wrote:
> Hi Marc,
> 
> After looking at the GICv3 implementation and trying to understand how 
> you architect the driver, I have a couple questions below.
> 
>  > On 06/24/2014 04:52 AM, Marc Zyngier wrote:
>> Hi Suravee,
>>
>> On 24/06/14 01:33, suravee.suthikulpanit@amd.com wrote:
>>> +       pr_info("GICv2m: SPI range [%d:%d]\n",
>>> +               msi->spi_start, (msi->spi_start + msi->max_spi_cnt));
>>> +
>>> +       gic_arch_extn.irq_mask = gicv2m_mask_msi;
>>> +       gic_arch_extn.irq_unmask = gicv2m_unmask_msi;
>>
>> Right, I now see why you need to test on the MSI descriptor. Don't do
>> that. The gic_arch_extn crap should *never* *be* *used*.
> 
> Hm, sounds like this should be removed all together then? In that case, 
> this would require changes in the irq-gic.c to call these functions 
> directly.

No. The gic_arch_extn's sole purpose in life is to allow a parallel
interrupt controller that mimics what the GIC does, and that can be used
as a wake up source. Clearly, MSI support is completely out of the scope
of this ... thing.

>>
>> What you want to do is do assign a different irq_chip to your MSI
>> interrupts. This will require a different integration with the main GIC
>> code though. For the GICv3 ITS, I do it when the interrupt gets mapped.
> 
> Ah, that's what I was trying to avoid. Why should we need a whole 
> different irq_chip just to add the MSI register frame support on top of 
> the GICv2 which is already supported by the current irq-gic.c?

Because, as you've noticed, it has a different set of requirements
(accessing the MSI specific data, for a start). That's the very purpose
of the irq_chip structure. Don't work around it. Nothing prevents you
from reusing the existing code, by the way.

>>
>>> +       return 0;
>>> +}
>>> +EXPORT_SYMBOL(gicv2m_msi_init);
>>> +
>>> +
>>> +
>>> +/**
>>> + * Override arch_setup_msi_irq in drivers/pci/msi.c
>>> + * since we don't want to change the chip_data
>>> + */
>>> +int arch_setup_msi_irq(struct pci_dev *pdev, struct msi_desc *desc)
>>> +{
>>> +       struct msi_chip *chip = pdev->bus->msi;
>>> +
>>> +       if (!chip || !chip->setup_irq)
>>> +               return -EINVAL;
>>> +
>>> +       return chip->setup_irq(chip, pdev, desc);
>>> +}
>>> +
>>> +/**
>>> + * Override arch_teardown_msi_irq in drivers/pci/msi.c
>>> + */
>>> +void arch_teardown_msi_irq(unsigned int irq)
>>> +{
>>> +       struct msi_desc *desc;
>>> +       struct msi_chip *chip;
>>> +
>>> +       desc = irq_get_msi_desc(irq);
>>> +       if (!desc)
>>> +               return;a
>>> +
>>> +       chip = desc->dev->bus->msi;
>>> +       if (!chip)
>>> +               return;
>>> +
>>> +       chip->teardown_irq(chip, irq);
>>> +}
>>
>> Please don't overtide those. There shouldn't be any reason for a
>> *driver* to do so. Only architecture code should do it. And nothing in
>> your code requires it (at least once you've stopped playing with the
>> silly gic extension...).
> 
> The reason I need these because the __weak version of arch_setup_msi_irq 
> function in driver/pci/msi.c calls irq_set_chip_data and replace the 
> chip_data with msi_chip (originally was pointing to the gic_chip_data 
> structure). This ended up breaking the GIC.

This is exactly why I urged you to have a different irq_chip, pointing
to different methods. MSI and IRQs are treated a bit differently in the
kernel. Hijacking global symbols to work around this difference is
hardly acceptable. Here, you're mandating that all the MSI controllers,
past, present and future are going to fit the requirements you've now
dictated. Hmmm... ;-)

> Looking at the GICv3 ITS implementation, this seems to also have similar 
> implementation. So, this was not an issue for you?

No. This is actually a very useful feature (and I should definitely make
use of it). You can embed the msi_chip into the gic_chip_data, and use
"container_of()" to go from one to the other. But again, this mandates
you provide your own methods (not that it is a lot of code, really).

Thanks,

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

Re: [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)

[Mark Rutland]

On Tue, Jun 24, 2014 at 01:33:00AM +0100, suravee.suthikulpanit@amd.com wrote:
> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> 
> GICv2m extends GICv2 to support MSI(-X) with a new set of register frames.
> 
> This patch introduces support for the non-secure GICv2m register frame.
> This is optional. It uses the "msi-controller" keyword in ARM GIC
> devicetree binding to indentify GIC driver that it should enable MSI(-X)
> support, The region of GICv2m MSI register frame is specified using the register
> frame index 4 in the device tree.
> 
> Each GIC maintains an "msi_chip" structure. To discover the msi_chip,
> PCI host driver can do the following:
> 
>     struct device_node *gic_node = of_irq_find_parent(pdev->dev.of_node);
>     pcie_bus->msi_chip = of_pci_find_msi_chip_by_node(gic_node);
> 
> Signed-off-by: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> ---
>  Documentation/devicetree/bindings/arm/gic.txt |  18 +-
>  drivers/irqchip/Kconfig                       |   6 +
>  drivers/irqchip/Makefile                      |   1 +
>  drivers/irqchip/gic-msi-v2m.c                 | 249 ++++++++++++++++++++++++++
>  drivers/irqchip/gic-msi-v2m.h                 |  20 +++
>  drivers/irqchip/irq-gic.c                     |  21 ++-
>  6 files changed, 311 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/irqchip/gic-msi-v2m.c
>  create mode 100644 drivers/irqchip/gic-msi-v2m.h
> 
> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
> index 5573c08..ee4bc02 100644
> --- a/Documentation/devicetree/bindings/arm/gic.txt
> +++ b/Documentation/devicetree/bindings/arm/gic.txt
> @@ -17,6 +17,7 @@ Main node required properties:
>         "arm,cortex-a7-gic"
>         "arm,arm11mp-gic"
>  - interrupt-controller : Identifies the node as an interrupt controller
> +
>  - #interrupt-cells : Specifies the number of cells needed to encode an
>    interrupt source.  The type shall be a <u32> and the value shall be 3.
> 
> @@ -37,9 +38,16 @@ Main node required properties:
>         the 8 possible cpus attached to the GIC.  A bit set to '1' indicated
>         the interrupt is wired to that CPU.  Only valid for PPI interrupts.
> 
> -- reg : Specifies base physical address(s) and size of the GIC registers. The
> -  first region is the GIC distributor register base and size. The 2nd region is
> -  the GIC cpu interface register base and size.
> +- reg : Specifies base physical address(s) and size of the GIC register frames.
> +
> +  Region | Description
> +  Index  |
> +  -------------------------------------------------------------------
> +     0   | GIC distributor register base and size
> +     1   | GIC cpu interface register base and size
> +     2   | VGIC interface control register base and size (Optional)
> +     3   | VGIC CPU interface register base and size (Optional)
> +     4   | GICv2m MSI interface register base and size (Optional)
> 
>  Optional
>  - interrupts   : Interrupt source of the parent interrupt controller on
> @@ -55,6 +63,10 @@ Optional
>                   by a crossbar/multiplexer preceding the GIC. The GIC irq
>                   input line is assigned dynamically when the corresponding
>                   peripheral's crossbar line is mapped.
> +
> +- msi-controller : Identifies the node as an MSI controller.  This requires
> +  the register region index 4.
> +
>  Example:
> 
>         intc: interrupt-controller@fff11000 {
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index bbb746e..a36bf94 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -7,6 +7,12 @@ config ARM_GIC
>         select IRQ_DOMAIN
>         select MULTI_IRQ_HANDLER
> 
> +config ARM_GIC_MSI_V2M
> +       bool
> +       select IRQ_DOMAIN
> +       select MULTI_IRQ_HANDLER
> +       depends on PCI && PCI_MSI
> +
>  config GIC_NON_BANKED
>         bool
> 
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 62a13e5..d43f904 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -16,6 +16,7 @@ obj-$(CONFIG_ARCH_SUNXI)              += irq-sun4i.o
>  obj-$(CONFIG_ARCH_SUNXI)               += irq-sunxi-nmi.o
>  obj-$(CONFIG_ARCH_SPEAR3XX)            += spear-shirq.o
>  obj-$(CONFIG_ARM_GIC)                  += irq-gic.o
> +obj-$(CONFIG_ARM_GIC_MSI_V2M)          += gic-msi-v2m.o
>  obj-$(CONFIG_ARM_NVIC)                 += irq-nvic.o
>  obj-$(CONFIG_ARM_VIC)                  += irq-vic.o
>  obj-$(CONFIG_IMGPDC_IRQ)               += irq-imgpdc.o
> diff --git a/drivers/irqchip/gic-msi-v2m.c b/drivers/irqchip/gic-msi-v2m.c
> new file mode 100644
> index 0000000..e5c0f79
> --- /dev/null
> +++ b/drivers/irqchip/gic-msi-v2m.c
> @@ -0,0 +1,249 @@
> +/*
> + * ARM GIC v2m MSI support
> + * Support for Message Signalelled Interrupts for systems that
> + * implement ARM Generic Interrupt Controller: GICv2m.
> + *
> + * Copyright (C) 2014 Advanced Micro Devices, Inc.
> + * Authors: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> + *          Harish Kasiviswanathan <harish.kasiviswanathan@amd.com>
> + *          Brandon Anderson <brandon.anderson@amd.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published
> + * by the Free Software Foundation.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/pci.h>
> +#include <linux/irq.h>
> +#include <linux/spinlock.h>
> +#include <linux/slab.h>
> +#include <linux/of_address.h>
> +#include <linux/bitmap.h>
> +
> +#include "gic-msi-v2m.h"
> +
> +/* GICv2m MSI Registers */
> +#define MSI_TYPER                      0x008
> +#define MSI_SETSPI_NS                  0x040
> +#define GIC_V2M_MIN_SPI                        32
> +#define GIC_V2M_MAX_SPI                        1024
> +#define GIC_OF_MSIV2M_RANGE_INDEX      4
> +
> +extern struct irq_chip gic_arch_extn;

Shouldn't this have come from a header somewhere?

> +
> +/**
> + * alloc_msi_irq - Allocate MSIs from avaialbe MSI bitmap.
> + * @data: Pointer to gicv2m_msi_data
> + * @nvec: Number of interrupts to allocate
> + * @irq: Pointer to the allocated irq
> + *
> + * Allocates interrupts only if the contiguous range of MSIs
> + * with specified nvec are avaiable. Otherwise return the number
> + * of avaiable interrupts. If none is avaiable, then returns -ENOENT.

%s/avaiable/available/

s/none is/none are/

> + */
> +static int alloc_msi_irq(struct gicv2m_msi_data *data, int nvec, int *irq)
> +{
> +       int size = sizeof(unsigned long) * data->bm_sz;

You already have the precise number you need: data->max_spi_cnt.

> +       int next = size, ret, num;
> +
> +       spin_lock(&data->msi_cnt_lock);
> +
> +       for (num = nvec; num > 0; num--) {

s/num/i/

> +               next = bitmap_find_next_zero_area(data->bm,
> +                                       size, 0, num, 0);
> +               if (next < size)
> +                       break;
> +       }

If max_spi_cnt is not a multiple of BITS_PER_LONG, this can allocate a
number greater or equal to max_spi_cnt, but below size. We should never
allocate max_spi_cnt or above.

> +
> +       if (next < size) {
> +               bitmap_set(data->bm, next, nvec);
> +               *irq = data->spi_start + next;
> +               ret = 0;
> +       } else if (num != 0) {
> +               ret = num;
> +       } else {
> +               ret = -ENOENT;
> +       }
> +
> +

Nit: extraneous whitespace.

> +       spin_unlock(&data->msi_cnt_lock);
> +
> +       return ret;
> +}
> +
> +static void free_msi_irq(struct gicv2m_msi_data *data, unsigned int irq)
> +{
> +       int pos;
> +
> +       spin_lock(&data->msi_cnt_lock);
> +
> +       pos = irq - data->spi_start;
> +       if (pos >= 0 && pos < data->max_spi_cnt)

Should either of these cases ever happen?

> +               bitmap_clear(data->bm, pos, 1);
> +
> +       spin_unlock(&data->msi_cnt_lock);
> +}
> +
> +static inline struct gicv2m_msi_data *to_gicv2m_msi_data(struct msi_chip *chip)
> +{
> +       return container_of(chip, struct gicv2m_msi_data, chip);
> +}
> +
> +static void gicv2m_teardown_msi_irq(struct msi_chip *chip, unsigned int irq)
> +{
> +       free_msi_irq(to_gicv2m_msi_data(chip), irq);
> +}
> +
> +static int gicv2m_setup_msi_irq(struct msi_chip *chip, struct pci_dev *pdev,
> +                     struct msi_desc *desc)
> +{
> +       int avail, irq = 0;
> +       struct msi_msg msg;
> +       phys_addr_t addr;
> +       struct gicv2m_msi_data *data = to_gicv2m_msi_data(chip);
> +
> +       if (data == NULL) {

If container_of returns NULL, you have bigger problems.

> +               dev_err(&pdev->dev,
> +                       "GICv2m: MSI setup failed. Invalid platform data\n");
> +               return -EINVAL;
> +       }
> +
> +       if (!desc) {
> +               dev_err(&pdev->dev,
> +                       "GICv2m: MSI setup failed. Invalid msi descriptor\n");
> +               return -EINVAL;
> +       }
> +
> +       avail = alloc_msi_irq(data, 1, &irq);
> +       if (avail != 0) {
> +               dev_err(&pdev->dev,
> +                       "GICv2m: MSI setup failed. Cannnot allocate IRQ\n");
> +               return -ENOSPC;
> +       }
> +
> +       irq_set_msi_desc(irq, desc);
> +       irq_set_irq_type(irq, IRQ_TYPE_EDGE_RISING);
> +
> +       addr = (unsigned long)(data->paddr64 + MSI_SETSPI_NS);

Why the cast?

> +       msg.address_hi = (unsigned int)(addr >> 32);
> +       msg.address_lo = (unsigned int)(addr);

Use u32, it makes this clearer (and matches the types in the definition
of struct msi_msg).

> +       msg.data = irq;
> +#ifdef CONFIG_PCI_MSI
> +       write_msi_msg(irq, &msg);
> +#endif
> +
> +       return 0;
> +}
> +
> +static void gicv2m_mask_msi(struct irq_data *d)
> +{
> +       if (d->msi_desc)
> +               mask_msi_irq(d);
> +}
> +
> +static void gicv2m_unmask_msi(struct irq_data *d)
> +{
> +       if (d->msi_desc)
> +               unmask_msi_irq(d);
> +}
> +
> +int __init gicv2m_msi_init(struct device_node *node,
> +                          struct gicv2m_msi_data *msi)
> +{
> +       unsigned int val;
> +       const __be32 *msi_be;

Every time I see a __be32* in a DT probe function I weep. There is no
need to deal with the internal details of the DTB.

> +
> +       msi_be = of_get_address(node, GIC_OF_MSIV2M_RANGE_INDEX, NULL, NULL);
> +       if (!msi_be) {
> +               pr_err("GICv2m failed. Fail to locate MSI base.\n");
> +               return -EINVAL;
> +       }
> +
> +       msi->paddr64 = of_translate_address(node, msi_be);
> +       msi->base = of_iomap(node, GIC_OF_MSIV2M_RANGE_INDEX);

You can instead use of_address_to_resource to query the address from the
DTB once, without having to muck about with endianness conversion
manually. Take a look at what of_iomap does.

I'm surprised we don't have an ioremap_resource given we have a devm_
variant.

> +
> +       /*
> +       * MSI_TYPER:
> +       *     [31:26] Reserved
> +       *     [25:16] lowest SPI assigned to MSI
> +       *     [15:10] Reserved
> +       *     [9:0]   Numer of SPIs assigned to MSI
> +       */
> +       val = __raw_readl(msi->base + MSI_TYPER);

Are you sure you want to use __raw_readl?

> +       if (!val) {
> +               pr_warn("GICv2m: Failed to read MSI_TYPER register\n");
> +               return -EINVAL;
> +       }
> +
> +       msi->spi_start = (val >> 16) & 0x3ff;
> +       msi->max_spi_cnt = val & 0x3ff;
> +
> +       pr_debug("GICv2m: SPI = %u, cnt = %u\n",
> +               msi->spi_start, msi->max_spi_cnt);
> +
> +       if (msi->spi_start < GIC_V2M_MIN_SPI ||
> +               msi->max_spi_cnt >= GIC_V2M_MAX_SPI) {
> +                       pr_err("GICv2m: Init failed\n");
> +                       return -EINVAL;
> +       }
> +
> +       msi->bm_sz = BITS_TO_LONGS(msi->max_spi_cnt);

So msi->bm_sz is msi->max_spi_cnt in _longs_ (rounded up)...

> +       msi->bm = kzalloc(msi->bm_sz, GFP_KERNEL);

...yet we allocate that many _bytes_?

> +       if (!msi->bm)
> +               return -ENOMEM;
> +
> +       spin_lock_init(&msi->msi_cnt_lock);
> +
> +       msi->chip.owner = THIS_MODULE;
> +       msi->chip.of_node = node;
> +       msi->chip.setup_irq = gicv2m_setup_msi_irq;
> +       msi->chip.teardown_irq = gicv2m_teardown_msi_irq;
> +
> +       pr_info("GICv2m: SPI range [%d:%d]\n",
> +               msi->spi_start, (msi->spi_start + msi->max_spi_cnt));

You have a (redundant) pr_debug for this above.

> +
> +       gic_arch_extn.irq_mask = gicv2m_mask_msi;
> +       gic_arch_extn.irq_unmask = gicv2m_unmask_msi;

I'll leave others to comment on the validity of this...

> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(gicv2m_msi_init);

Why?

> +
> +
> +
> +/**
> + * Override arch_setup_msi_irq in drivers/pci/msi.c
> + * since we don't want to change the chip_data
> + */
> +int arch_setup_msi_irq(struct pci_dev *pdev, struct msi_desc *desc)
> +{
> +       struct msi_chip *chip = pdev->bus->msi;
> +
> +       if (!chip || !chip->setup_irq)
> +               return -EINVAL;
> +
> +       return chip->setup_irq(chip, pdev, desc);
> +}
> +
> +/**
> + * Override arch_teardown_msi_irq in drivers/pci/msi.c
> + */
> +void arch_teardown_msi_irq(unsigned int irq)
> +{
> +       struct msi_desc *desc;
> +       struct msi_chip *chip;
> +
> +       desc = irq_get_msi_desc(irq);
> +       if (!desc)
> +               return;
> +
> +       chip = desc->dev->bus->msi;
> +       if (!chip)
> +               return;
> +
> +       chip->teardown_irq(chip, irq);
> +}
> diff --git a/drivers/irqchip/gic-msi-v2m.h b/drivers/irqchip/gic-msi-v2m.h
> new file mode 100644
> index 0000000..164d929
> --- /dev/null
> +++ b/drivers/irqchip/gic-msi-v2m.h
> @@ -0,0 +1,20 @@
> +#ifndef GIC_MSI_V2M_H
> +#define GIC_MSI_V2M_H
> +
> +#include <linux/msi.h>
> +
> +struct gicv2m_msi_data {
> +       struct msi_chip chip;
> +       spinlock_t msi_cnt_lock;
> +       u64 paddr64;              /* GICv2m phys address */

phys_addr_t?

> +       void __iomem *base;       /* GICv2m virt address */
> +       unsigned int spi_start;   /* The SPI number that MSIs start */
> +       unsigned int max_spi_cnt; /* The number of SPIs for MSIs */
> +       unsigned long *bm;        /* MSI vector bitmap */
> +       unsigned long bm_sz;      /* MSI bitmap size */

It's a bit odd in my mind that this is in longs. Why not just use
max_spi_cnt, which you can trivially use to determine bytes or longs?

Also, s/max_spi_cnt/nr_spis/. 

> +};
> +
> +extern int gicv2m_msi_init(struct device_node *node,
> +                          struct gicv2m_msi_data *msi) __init;
> +
> +#endif /*GIC_MSI_V2M_H*/
> diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> index adc86de..dcff99b 100644
> --- a/drivers/irqchip/irq-gic.c
> +++ b/drivers/irqchip/irq-gic.c
> @@ -35,6 +35,7 @@
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
> +#include <linux/of_pci.h>
>  #include <linux/irqdomain.h>
>  #include <linux/interrupt.h>
>  #include <linux/percpu.h>
> @@ -48,6 +49,10 @@
> 
>  #include "irqchip.h"
> 
> +#ifdef CONFIG_ARM_GIC_MSI_V2M
> +#include "gic-msi-v2m.h"
> +#endif
> +
>  union gic_base {
>         void __iomem *common_base;
>         void __percpu * __iomem *percpu_base;
> @@ -68,6 +73,9 @@ struct gic_chip_data {
>  #ifdef CONFIG_GIC_NON_BANKED
>         void __iomem *(*get_base)(union gic_base *);
>  #endif
> +#ifdef CONFIG_ARM_GIC_MSI_V2M
> +       struct gicv2m_msi_data msi_data;
> +#endif
>  };
> 
>  static DEFINE_RAW_SPINLOCK(irq_controller_lock);
> @@ -246,7 +254,8 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
>                             bool force)
>  {
>         void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3);
> -       unsigned int cpu, shift = (gic_irq(d) % 4) * 8;
> +       unsigned int shift = (gic_irq(d) % 4) * 8;
> +       unsigned int cpu = cpumask_any_and(mask_val, cpu_online_mask);

Unrelated change?

>         u32 val, mask, bit;
> 
>         if (!force)
> @@ -1047,6 +1056,7 @@ gic_of_init(struct device_node *node, struct device_node *parent)
>         void __iomem *dist_base;
>         u32 percpu_offset;
>         int irq;
> +       struct gic_chip_data *gic;
> 
>         if (WARN_ON(!node))
>                 return -ENODEV;
> @@ -1068,6 +1078,15 @@ gic_of_init(struct device_node *node, struct device_node *parent)
>                 irq = irq_of_parse_and_map(node, 0);
>                 gic_cascade_irq(gic_cnt, irq);
>         }
> +
> +#ifdef CONFIG_ARM_GIC_MSI_V2M
> +       if (of_find_property(node, "msi-controller", NULL)) {

Use of_property_read_bool for booleans.

Thanks,
Mark.

Re: [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)

[Suravee Suthikulanit]

Mark,

Thank you for all your comments. Please see my reply below. I have 
omitted the minor ones.

On 6/24/2014 5:11 AM, Mark Rutland wrote:
> On Tue, Jun 24, 2014 at 01:33:00AM +0100, suravee.suthikulpanit@amd.com wrote:
>> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>> +static int alloc_msi_irq(struct gicv2m_msi_data *data, int nvec, int *irq)
>> +{
>> +       int size = sizeof(unsigned long) * data->bm_sz;
>> +       int next = size, ret, num;
>> +
>> +       spin_lock(&data->msi_cnt_lock);
>> +
>> +       for (num = nvec; num > 0; num--) {
>> +               next = bitmap_find_next_zero_area(data->bm,
>> +                                       size, 0, num, 0);
>> +               if (next < size)
>> +                       break;
>> +       }
>
> If max_spi_cnt is not a multiple of BITS_PER_LONG, this can allocate a
> number greater or equal to max_spi_cnt, but below size. We should never
> allocate max_spi_cnt or above.
>
Thanks for the catch. I also need to clean up this logic for V2.

>> +       spin_unlock(&data->msi_cnt_lock);
>> +
>> +       return ret;
>> +}
>> +
>> +static void free_msi_irq(struct gicv2m_msi_data *data, unsigned int irq)
>> +{
>> +       int pos;
>> +
>> +       spin_lock(&data->msi_cnt_lock);
>> +
>> +       pos = irq - data->spi_start;
>> +       if (pos >= 0 && pos < data->max_spi_cnt)
>
> Should either of these cases ever happen?

This is to check if the irq provided is within the MSI range.

>> +static int gicv2m_setup_msi_irq(struct msi_chip *chip, struct pci_dev *pdev,
>> +                     struct msi_desc *desc)
>> +{
>> +       int avail, irq = 0;
>> +       struct msi_msg msg;
>> +       phys_addr_t addr;
>> +       struct gicv2m_msi_data *data = to_gicv2m_msi_data(chip);
>> +
>> +       if (data == NULL) {
>
> If container_of returns NULL, you have bigger problems.

It was just sanity check. But, if you think this is obvious, I'll remove 
this check then.

>> +int __init gicv2m_msi_init(struct device_node *node,
>> +                          struct gicv2m_msi_data *msi)
>> +{
>> +       unsigned int val;
>> +       const __be32 *msi_be;
>
> Every time I see a __be32* in a DT probe function I weep. There is no
> need to deal with the internal details of the DTB.
>
>> +
>> +       msi_be = of_get_address(node, GIC_OF_MSIV2M_RANGE_INDEX, NULL, NULL);
>> +       if (!msi_be) {
>> +               pr_err("GICv2m failed. Fail to locate MSI base.\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       msi->paddr64 = of_translate_address(node, msi_be);
>> +       msi->base = of_iomap(node, GIC_OF_MSIV2M_RANGE_INDEX);
>
> You can instead use of_address_to_resource to query the address from the
> DTB once, without having to muck about with endianness conversion
> manually. Take a look at what of_iomap does.

Thanks for this suggestion. I was not quite familiar with the "of_" 
interface. I am cleaning up this whole section now.

> I'm surprised we don't have an ioremap_resource given we have a devm_
> variant.
>

>> +
>> +       /*
>> +       * MSI_TYPER:
>> +       *     [31:26] Reserved
>> +       *     [25:16] lowest SPI assigned to MSI
>> +       *     [15:10] Reserved
>> +       *     [9:0]   Numer of SPIs assigned to MSI
>> +       */
>> +       val = __raw_readl(msi->base + MSI_TYPER);
>
> Are you sure you want to use __raw_readl?
>
Probably not.  I am replacing this with ioread32(msi->base + MSI_TYPER).

>> +       if (!val) {
>> +               pr_warn("GICv2m: Failed to read MSI_TYPER register\n");
>> +               return -EINVAL;
>> +       }
>> +
>> +       msi->spi_start = (val >> 16) & 0x3ff;
>> +       msi->max_spi_cnt = val & 0x3ff;
>> +
>> +       pr_debug("GICv2m: SPI = %u, cnt = %u\n",
>> +               msi->spi_start, msi->max_spi_cnt);
>> +
>> +       if (msi->spi_start < GIC_V2M_MIN_SPI ||
>> +               msi->max_spi_cnt >= GIC_V2M_MAX_SPI) {
>> +                       pr_err("GICv2m: Init failed\n");
>> +                       return -EINVAL;
>> +       }
>> +
>> +       msi->bm_sz = BITS_TO_LONGS(msi->max_spi_cnt);
>
> So msi->bm_sz is msi->max_spi_cnt in _longs_ (rounded up)...
>
>> +       msi->bm = kzalloc(msi->bm_sz, GFP_KERNEL);
>
> ...yet we allocate that many _bytes_?
>
Sorry, I got a bit confused here. I'll fix this.

>> +
>> +       gic_arch_extn.irq_mask = gicv2m_mask_msi;
>> +       gic_arch_extn.irq_unmask = gicv2m_unmask_msi;
>
> I'll leave others to comment on the validity of this...
>
Marc has commented this part in the other patch.

>> +       void __iomem *base;       /* GICv2m virt address */
>> +       unsigned int spi_start;   /* The SPI number that MSIs start */
>> +       unsigned int max_spi_cnt; /* The number of SPIs for MSIs */
>> +       unsigned long *bm;        /* MSI vector bitmap */
>> +       unsigned long bm_sz;      /* MSI bitmap size */
>
> It's a bit odd in my mind that this is in longs. Why not just use
> max_spi_cnt, which you can trivially use to determine bytes or longs?

That's true. I'm cleaning this up.

>> @@ -246,7 +254,8 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
>>                              bool force)
>>   {
>>          void __iomem *reg = gic_dist_base(d) + GIC_DIST_TARGET + (gic_irq(d) & ~3);
>> -       unsigned int cpu, shift = (gic_irq(d) % 4) * 8;
>> +       unsigned int shift = (gic_irq(d) % 4) * 8;
>> +       unsigned int cpu = cpumask_any_and(mask_val, cpu_online_mask);
>
> Unrelated change?

Sorry, this was not supposed to be part of this patch.

Thanks,

Suravee

Re: [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)

[Mark Rutland]

On Wed, Jun 25, 2014 at 03:55:54AM +0100, Suravee Suthikulanit wrote:
> Mark,
> 
> Thank you for all your comments. Please see my reply below. I have 
> omitted the minor ones.

Likewise.

> >> +static void free_msi_irq(struct gicv2m_msi_data *data, unsigned int irq)
> >> +{
> >> +       int pos;
> >> +
> >> +       spin_lock(&data->msi_cnt_lock);
> >> +
> >> +       pos = irq - data->spi_start;
> >> +       if (pos >= 0 && pos < data->max_spi_cnt)
> >
> > Should either of these cases ever happen?
> 
> This is to check if the irq provided is within the MSI range.

Sure, but surely this should only be called for the correct set of MSI
IRQs? Allowing this to be called for arbitrary interrupts without
reporting an error sounds wrong.

[...]

> >> +static int gicv2m_setup_msi_irq(struct msi_chip *chip, struct pci_dev *pdev,
> >> +                     struct msi_desc *desc)
> >> +{
> >> +       int avail, irq = 0;
> >> +       struct msi_msg msg;
> >> +       phys_addr_t addr;
> >> +       struct gicv2m_msi_data *data = to_gicv2m_msi_data(chip);
> >> +
> >> +       if (data == NULL) {
> >
> > If container_of returns NULL, you have bigger problems.
> 
> It was just sanity check. But, if you think this is obvious, I'll remove 
> this check then.

The issue is you check for NULL _after_ you've performed some pointer
arithmetic. Because the msi_chip is the first element of
gicv2m_msi_data, to_gicv2m_msi_data is currently an identity function
with some type casting, but we should rely on that here. What if the
msi_chip were moved to later in gicv2m_msi_data?

If you want to check for NULL, check chip before
to_gicv2m_msi_data(chip).

[...]

> >> +       /*
> >> +       * MSI_TYPER:
> >> +       *     [31:26] Reserved
> >> +       *     [25:16] lowest SPI assigned to MSI
> >> +       *     [15:10] Reserved
> >> +       *     [9:0]   Numer of SPIs assigned to MSI
> >> +       */
> >> +       val = __raw_readl(msi->base + MSI_TYPER);
> >
> > Are you sure you want to use __raw_readl?
> >
> Probably not.  I am replacing this with ioread32(msi->base + MSI_TYPER).

It's probably better to use readl() or a readl_relaxed() here for
consistency with the rest of the ARM code, but otherwise that sounds
sane.

Thanks,
Mark.

Re: [PATCH 0/2] Introduce ARM GICv2m MSI(-X) support

[Jason Cooper]

On Mon, Jun 23, 2014 at 07:32:58PM -0500, suravee.suthikulpanit@amd.com wrote:
> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
> 
> This patch set introduces support for MSI(-X) in GICv2m specification,
> which is implemented in some variation of GIC400.
> 
> This depends on and has been tested with the V7 of "Add support for PCI in AArch64"
> (https://lkml.org/lkml/2014/3/14/320).
> 
> Suravee Suthikulpanit (2):
>   arm/gic: Add binding probe for GIC400
>   arm/gic: Add supports for GICv2m MSI(-X)
> 
>  Documentation/devicetree/bindings/arm/gic.txt |  18 +-
>  drivers/irqchip/Kconfig                       |   6 +
>  drivers/irqchip/Makefile                      |   1 +
>  drivers/irqchip/gic-msi-v2m.c                 | 249 ++++++++++++++++++++++++++
>  drivers/irqchip/gic-msi-v2m.h                 |  20 +++
>  drivers/irqchip/irq-gic.c                     |  23 ++-
>  6 files changed, 313 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/irqchip/gic-msi-v2m.c
>  create mode 100644 drivers/irqchip/gic-msi-v2m.h

Please don't forget to run get_maintainers.pl before you send.  I almost
missed this series. :)

thx,

Jason.

Re: [PATCH 0/2] Introduce ARM GICv2m MSI(-X) support

[Jason Cooper]

On Mon, Jun 23, 2014 at 07:32:58PM -0500, suravee.suthikulpanit@amd.com wrote:
> This patch set introduces support for MSI(-X) in GICv2m specification,
> which is implemented in some variation of GIC400.
> 
> This depends on and has been tested with the V7 of "Add support for PCI in AArch64"
> (https://lkml.org/lkml/2014/3/14/320).

Is this a build, boot, or runtime dependency?

thx,

Jason.

Re: [PATCH 0/2] Introduce ARM GICv2m MSI(-X) support

[Suravee Suthikulanit]

On 6/24/2014 7:26 AM, Jason Cooper wrote:
> On Mon, Jun 23, 2014 at 07:32:58PM -0500, suravee.suthikulpanit@amd.com wrote:
>> This patch set introduces support for MSI(-X) in GICv2m specification,
>> which is implemented in some variation of GIC400.
>>
>> This depends on and has been tested with the V7 of "Add support for PCI in AArch64"
>> (https://lkml.org/lkml/2014/3/14/320).
>
> Is this a build, boot, or runtime dependency?
>
> thx,
>
> Jason.
>

For all of the above.

Suravee

Re: [PATCH 0/2] Introduce ARM GICv2m MSI(-X) support

[Jason Cooper]

On Tue, Jun 24, 2014 at 07:19:26PM -0500, Suravee Suthikulanit wrote:
> On 6/24/2014 7:26 AM, Jason Cooper wrote:
> >On Mon, Jun 23, 2014 at 07:32:58PM -0500, suravee.suthikulpanit@amd.com wrote:
> >>This patch set introduces support for MSI(-X) in GICv2m specification,
> >>which is implemented in some variation of GIC400.
> >>
> >>This depends on and has been tested with the V7 of "Add support for PCI in AArch64"
> >>(https://lkml.org/lkml/2014/3/14/320).
> >
> >Is this a build, boot, or runtime dependency?
> >
> 
> For all of the above.

Ok, the gic change in #1 should be a trivial conflict to resolve, so
it'll be easiest for you guys to merge this with the other code you have
going into arm-soc.

Just keep me in the Cc and I'll Ack it when it's ready.

thx,

Jason.

Re: [PATCH 0/2] Introduce ARM GICv2m MSI(-X) support

[Joel Schopp]

I've been running and doing development on top of these patches.  I 
found a problem in an earlier version that i can confirm is now fixed in 
this current version.

Reviewed-by: Joel Schopp <joel.schopp@amd.com>

On 06/23/2014 07:32 PM, suravee.suthikulpanit@amd.com wrote:
> From: Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com>
>
> This patch set introduces support for MSI(-X) in GICv2m specification,
> which is implemented in some variation of GIC400.
>
> This depends on and has been tested with the V7 of "Add support for PCI in AArch64"
> (https://lkml.org/lkml/2014/3/14/320).
>
> Suravee Suthikulpanit (2):
>    arm/gic: Add binding probe for GIC400
>    arm/gic: Add supports for GICv2m MSI(-X)
>
>   Documentation/devicetree/bindings/arm/gic.txt |  18 +-
>   drivers/irqchip/Kconfig                       |   6 +
>   drivers/irqchip/Makefile                      |   1 +
>   drivers/irqchip/gic-msi-v2m.c                 | 249 ++++++++++++++++++++++++++
>   drivers/irqchip/gic-msi-v2m.h                 |  20 +++
>   drivers/irqchip/irq-gic.c                     |  23 ++-
>   6 files changed, 313 insertions(+), 4 deletions(-)
>   create mode 100644 drivers/irqchip/gic-msi-v2m.c
>   create mode 100644 drivers/irqchip/gic-msi-v2m.h
>