* [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)
@ 2014-06-24 0:33 ` suravee.suthikulpanit at amd.com
0 siblings, 0 replies; 32+ messages in thread
From: suravee.suthikulpanit at amd.com @ 2014-06-24 0:33 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)
2014-06-24 0:33 ` suravee.suthikulpanit at amd.com
@ 2014-06-24 9:52 ` Marc Zyngier
-1 siblings, 0 replies; 32+ messages in thread
From: Marc Zyngier @ 2014-06-24 9:52 UTC (permalink / raw)
To: suravee.suthikulpanit@amd.com
Cc: Catalin Marinas, Will Deacon,
linux-arm-kernel@lists.infradead.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org
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...
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)
@ 2014-06-24 9:52 ` Marc Zyngier
0 siblings, 0 replies; 32+ messages in thread
From: Marc Zyngier @ 2014-06-24 9:52 UTC (permalink / raw)
To: linux-arm-kernel
Hi Suravee,
On 24/06/14 01:33, suravee.suthikulpanit at 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 at 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...
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)
2014-06-24 9:52 ` Marc Zyngier
@ 2014-06-25 3:04 ` Suravee Suthikulanit
-1 siblings, 0 replies; 32+ messages in thread
From: Suravee Suthikulanit @ 2014-06-25 3:04 UTC (permalink / raw)
To: Marc Zyngier
Cc: Catalin Marinas, Will Deacon,
linux-arm-kernel@lists.infradead.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)
@ 2014-06-25 3:04 ` Suravee Suthikulanit
0 siblings, 0 replies; 32+ messages in thread
From: Suravee Suthikulanit @ 2014-06-25 3:04 UTC (permalink / raw)
To: linux-arm-kernel
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
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)
2014-06-24 9:52 ` Marc Zyngier
@ 2014-06-27 15:43 ` Suravee Suthikulpanit
-1 siblings, 0 replies; 32+ messages in thread
From: Suravee Suthikulpanit @ 2014-06-27 15:43 UTC (permalink / raw)
To: Marc Zyngier
Cc: Catalin Marinas, Will Deacon,
linux-arm-kernel@lists.infradead.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)
@ 2014-06-27 15:43 ` Suravee Suthikulpanit
0 siblings, 0 replies; 32+ messages in thread
From: Suravee Suthikulpanit @ 2014-06-27 15:43 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)
2014-06-27 15:43 ` Suravee Suthikulpanit
@ 2014-06-27 17:35 ` Marc Zyngier
-1 siblings, 0 replies; 32+ messages in thread
From: Marc Zyngier @ 2014-06-27 17:35 UTC (permalink / raw)
To: Suravee Suthikulpanit
Cc: Catalin Marinas, Will Deacon,
linux-arm-kernel@lists.infradead.org, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org
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...
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)
@ 2014-06-27 17:35 ` Marc Zyngier
0 siblings, 0 replies; 32+ messages in thread
From: Marc Zyngier @ 2014-06-27 17:35 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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...
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)
2014-06-24 0:33 ` suravee.suthikulpanit at amd.com
@ 2014-06-24 10:11 ` Mark Rutland
-1 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2014-06-24 10:11 UTC (permalink / raw)
To: suravee.suthikulpanit@amd.com
Cc: Marc Zyngier, Catalin Marinas, Will Deacon,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
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.
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)
@ 2014-06-24 10:11 ` Mark Rutland
0 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2014-06-24 10:11 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Jun 24, 2014 at 01:33:00AM +0100, suravee.suthikulpanit at 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 at 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.
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)
2014-06-24 10:11 ` Mark Rutland
@ 2014-06-25 2:55 ` Suravee Suthikulanit
-1 siblings, 0 replies; 32+ messages in thread
From: Suravee Suthikulanit @ 2014-06-25 2:55 UTC (permalink / raw)
To: Mark Rutland
Cc: Marc Zyngier, Catalin Marinas, Will Deacon,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
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
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)
@ 2014-06-25 2:55 ` Suravee Suthikulanit
0 siblings, 0 replies; 32+ messages in thread
From: Suravee Suthikulanit @ 2014-06-25 2:55 UTC (permalink / raw)
To: linux-arm-kernel
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 at 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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)
2014-06-25 2:55 ` Suravee Suthikulanit
@ 2014-06-25 8:57 ` Mark Rutland
-1 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2014-06-25 8:57 UTC (permalink / raw)
To: Suravee Suthikulanit
Cc: Marc Zyngier, Catalin Marinas, Will Deacon,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
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.
^ permalink raw reply [flat|nested] 32+ messages in thread* [PATCH 2/2] arm/gic: Add supports for GICv2m MSI(-X)
@ 2014-06-25 8:57 ` Mark Rutland
0 siblings, 0 replies; 32+ messages in thread
From: Mark Rutland @ 2014-06-25 8:57 UTC (permalink / raw)
To: linux-arm-kernel
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.
^ permalink raw reply [flat|nested] 32+ messages in thread