From: Suravee Suthikulpanit <[email protected]>
ARM GICv2m specification extends GICv2 to support MSI(-X) with
a new set of register frames. This patch introduces support for
the non-secure GICv2m register frame.
The driver currently matchs "arm,gic-400-plus" in device tree binding,
which implements GICv2m.
The "msi-controller" keyword in ARM GIC devicetree binding is used 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.
MSI support is optional.
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);
Cc: Mark Rutland <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Signed-off-by: Suravee Suthikulpanit <[email protected]>
---
Documentation/devicetree/bindings/arm/gic.txt | 19 +-
drivers/irqchip/Kconfig | 6 +
drivers/irqchip/Makefile | 1 +
drivers/irqchip/irq-gic-v2m.c | 248 ++++++++++++++++++++++++++
drivers/irqchip/irq-gic-v2m.h | 13 ++
5 files changed, 284 insertions(+), 3 deletions(-)
create mode 100644 drivers/irqchip/irq-gic-v2m.c
create mode 100644 drivers/irqchip/irq-gic-v2m.h
diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
index 5573c08..9e46f7f 100644
--- a/Documentation/devicetree/bindings/arm/gic.txt
+++ b/Documentation/devicetree/bindings/arm/gic.txt
@@ -12,11 +12,13 @@ Main node required properties:
- compatible : should be one of:
"arm,gic-400"
+ "arm,gic-400-plus"
"arm,cortex-a15-gic"
"arm,cortex-a9-gic"
"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 +39,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 +64,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..795d704 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_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..c8a9ca9 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_V2M) += irq-gic-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/irq-gic-v2m.c b/drivers/irqchip/irq-gic-v2m.c
new file mode 100644
index 0000000..232a220
--- /dev/null
+++ b/drivers/irqchip/irq-gic-v2m.c
@@ -0,0 +1,248 @@
+/*
+ * 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 <[email protected]>
+ * Harish Kasiviswanathan <[email protected]>
+ * Brandon Anderson <[email protected]>
+ *
+ * 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/of_pci.h>
+#include <linux/bitmap.h>
+
+#include "irqchip.h"
+#include "irq-gic.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
+
+/**
+ * alloc_msi_irq - Allocate MSIs from avaialbe MSI bitmap.
+ * @data: Pointer to v2m_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 available. Otherwise return the number
+ * of available interrupts. If none are available, then returns -ENOENT.
+ */
+static int alloc_msi_irq(struct v2m_data *data, int nvec, int *irq)
+{
+ int size = data->nr_spis;
+ int next = size, i = nvec, ret;
+
+ /* We should never allocate more than available nr_spis */
+ if (i >= size)
+ i = size - 1;
+
+ spin_lock(&data->msi_cnt_lock);
+
+ for (; i > 0; i--) {
+ next = bitmap_find_next_zero_area(data->bm,
+ size, 0, i, 0);
+ if (next < size)
+ break;
+ }
+
+ if (next >= size || i != nvec) {
+ ret = i ? : -ENOENT;
+ } else {
+ bitmap_set(data->bm, next, nvec);
+ *irq = data->spi_start + next;
+ ret = 0;
+ }
+
+ spin_unlock(&data->msi_cnt_lock);
+
+ return ret;
+}
+
+static struct v2m_data *to_v2m_data(struct msi_chip *chip)
+{
+ struct gic_chip_data *gic = container_of(chip, struct gic_chip_data,
+ msi_chip);
+ return &gic->v2m_data;
+}
+
+static void gicv2m_teardown_msi_irq(struct msi_chip *chip, unsigned int irq)
+{
+ int pos;
+ struct v2m_data *data = to_v2m_data(chip);
+
+ spin_lock(&data->msi_cnt_lock);
+
+ pos = irq - data->spi_start;
+ if (pos >= 0 && pos < data->nr_spis)
+ bitmap_clear(data->bm, pos, 1);
+
+ spin_unlock(&data->msi_cnt_lock);
+}
+
+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 v2m_data *data = to_v2m_data(chip);
+
+ 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_chip_data(irq, chip);
+ irq_set_msi_desc(irq, desc);
+ irq_set_irq_type(irq, IRQ_TYPE_EDGE_RISING);
+
+ addr = data->res.start + MSI_SETSPI_NS;
+
+ msg.address_hi = (u32)(addr >> 32);
+ msg.address_lo = (u32)(addr);
+ msg.data = irq;
+#ifdef CONFIG_PCI_MSI
+ write_msi_msg(irq, &msg);
+#endif
+
+ return 0;
+}
+
+static int __init
+gicv2m_msi_init(struct device_node *node, struct v2m_data *v2m)
+{
+ unsigned int val;
+
+ if (of_address_to_resource(node, GIC_OF_MSIV2M_RANGE_INDEX,
+ &v2m->res)) {
+ pr_err("GICv2m: Failed locate GICv2m MSI register frame\n");
+ return -EINVAL;
+ }
+
+ v2m->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 = readl_relaxed(v2m->base + MSI_TYPER);
+ if (!val) {
+ pr_warn("GICv2m: Failed to read MSI_TYPER register\n");
+ return -EINVAL;
+ }
+
+ v2m->spi_start = (val >> 16) & 0x3ff;
+ v2m->nr_spis = val & 0x3ff;
+
+ if (v2m->spi_start < GIC_V2M_MIN_SPI ||
+ v2m->nr_spis >= GIC_V2M_MAX_SPI) {
+ pr_err("GICv2m: Init failed\n");
+ return -EINVAL;
+ }
+
+ v2m->bm = kzalloc(sizeof(long) * BITS_TO_LONGS(v2m->nr_spis),
+ GFP_KERNEL);
+ if (!v2m->bm)
+ return -ENOMEM;
+
+ spin_lock_init(&v2m->msi_cnt_lock);
+
+ pr_info("GICv2m: SPI range [%d:%d]\n",
+ v2m->spi_start, (v2m->spi_start + v2m->nr_spis));
+
+ return 0;
+}
+
+static void gicv2m_mask_irq(struct irq_data *d)
+{
+ gic_mask_irq(d);
+ if (d->msi_desc)
+ mask_msi_irq(d);
+}
+
+static void gicv2m_unmask_irq(struct irq_data *d)
+{
+ gic_unmask_irq(d);
+ if (d->msi_desc)
+ unmask_msi_irq(d);
+}
+
+static struct irq_chip gicv2m_chip = {
+ .name = "GICv2m",
+ .irq_mask = gicv2m_mask_irq,
+ .irq_unmask = gicv2m_unmask_irq,
+ .irq_eoi = gic_eoi_irq,
+ .irq_set_type = gic_set_type,
+ .irq_retrigger = gic_retrigger,
+#ifdef CONFIG_SMP
+ .irq_set_affinity = gic_set_affinity,
+#endif
+#ifdef CONFIG_PM
+ .irq_set_wake = gic_set_wake,
+#endif
+};
+
+#ifdef CONFIG_OF
+static int __init
+gicv2m_of_init(struct device_node *node, struct device_node *parent)
+{
+ struct gic_chip_data *gic;
+ int ret;
+
+ ret = _gic_of_init(node, parent, &gicv2m_chip, &gic);
+ if (ret) {
+ pr_err("GICv2m: Failed to initialize GIC\n");
+ return ret;
+ }
+
+#ifdef CONFIG_PCI_MSI
+ gic->msi_chip.owner = THIS_MODULE;
+ gic->msi_chip.of_node = node;
+ gic->msi_chip.setup_irq = gicv2m_setup_msi_irq;
+ gic->msi_chip.teardown_irq = gicv2m_teardown_msi_irq;
+ ret = of_pci_msi_chip_add(&gic->msi_chip);
+ if (ret) {
+ /* MSI is optional and not supported here */
+ pr_warn("GICv2m: MSI is not supported.\n");
+ return 0;
+ }
+
+ ret = gicv2m_msi_init(node, &gic->v2m_data);
+ if (ret)
+ return ret;
+#endif
+ return ret;
+}
+
+IRQCHIP_DECLARE(arm_gic_400_plus, "arm,gic-400-plus", gicv2m_of_init);
+
+#endif /* CONFIG_OF */
diff --git a/drivers/irqchip/irq-gic-v2m.h b/drivers/irqchip/irq-gic-v2m.h
new file mode 100644
index 0000000..2d93a87
--- /dev/null
+++ b/drivers/irqchip/irq-gic-v2m.h
@@ -0,0 +1,13 @@
+#ifndef _IRQ_GIC_V2M_H_
+#define _IRQ_GIC_V2M_H_
+
+struct v2m_data {
+ spinlock_t msi_cnt_lock;
+ struct resource res; /* GICv2m resource */
+ void __iomem *base; /* GICv2m virt address */
+ unsigned int spi_start; /* The SPI number that MSIs start */
+ unsigned int nr_spis; /* The number of SPIs for MSIs */
+ unsigned long *bm; /* MSI vector bitmap */
+};
+
+#endif /*_IRQ_GIC_V2M_H_*/
--
1.9.0
On Wed, Jul 02, 2014 at 04:22:23PM +0100, [email protected] wrote:
> From: Suravee Suthikulpanit <[email protected]>
>
> ARM GICv2m specification extends GICv2 to support MSI(-X) with
> a new set of register frames. This patch introduces support for
> the non-secure GICv2m register frame.
>
> The driver currently matchs "arm,gic-400-plus" in device tree binding,
> which implements GICv2m.
As far as I am aware, "GIC 400 plus" is not a product name.
> The "msi-controller" keyword in ARM GIC devicetree binding is used 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.
> MSI support is optional.
>
> 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);
>
> Cc: Mark Rutland <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Cc: Jason Cooper <[email protected]>
> Cc: Catalin Marinas <[email protected]>
> Cc: Will Deacon <[email protected]>
>
> Signed-off-by: Suravee Suthikulpanit <[email protected]>
> ---
> Documentation/devicetree/bindings/arm/gic.txt | 19 +-
> drivers/irqchip/Kconfig | 6 +
> drivers/irqchip/Makefile | 1 +
> drivers/irqchip/irq-gic-v2m.c | 248 ++++++++++++++++++++++++++
> drivers/irqchip/irq-gic-v2m.h | 13 ++
> 5 files changed, 284 insertions(+), 3 deletions(-)
> create mode 100644 drivers/irqchip/irq-gic-v2m.c
> create mode 100644 drivers/irqchip/irq-gic-v2m.h
>
> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
> index 5573c08..9e46f7f 100644
> --- a/Documentation/devicetree/bindings/arm/gic.txt
> +++ b/Documentation/devicetree/bindings/arm/gic.txt
> @@ -12,11 +12,13 @@ Main node required properties:
>
> - compatible : should be one of:
> "arm,gic-400"
> + "arm,gic-400-plus"
I am not keen on this name.
> "arm,cortex-a15-gic"
> "arm,cortex-a9-gic"
> "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.
Random (inconsistent) whitespace change?
> @@ -37,9 +39,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)
As far as I am aware, the MSI interface is completely orthogonal to
having a GICH and GICV.
We should figure out how we expect to handle that (zero-sized reg
entries? reg-names?).
>
> Optional
> - interrupts : Interrupt source of the parent interrupt controller on
> @@ -55,6 +64,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.
That last clarifying comment is more confusing than helpful.
[...]
> +#define GIC_V2M_MIN_SPI 32
> +#define GIC_V2M_MAX_SPI 1024
GIC interrupt IDs 1020 and above are reserved, no?
Surely the max ID an SPI can take is 1019?
> +#define GIC_OF_MSIV2M_RANGE_INDEX 4
> +
> +/**
> + * alloc_msi_irq - Allocate MSIs from avaialbe MSI bitmap.
> + * @data: Pointer to v2m_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 available. Otherwise return the number
> + * of available interrupts. If none are available, then returns -ENOENT.
> + */
This function is overly complicated, and pointlessly so.
It doesn't achieve anything useful as it returns the size of the _last_
contiguous block rather than the _largest_ contiguous block, and the
only caller (gicv2m_setup_msi_irq) doesn't even care.
Simplify this to just return an error code if allocation is impossible.
> +static int alloc_msi_irq(struct v2m_data *data, int nvec, int *irq)
> +{
> + int size = data->nr_spis;
> + int next = size, i = nvec, ret;
Initialise ret to -ENOENT here...
> +
> + /* We should never allocate more than available nr_spis */
> + if (i >= size)
> + i = size - 1;
...return -ENOENT here...
> +
> + spin_lock(&data->msi_cnt_lock);
> +
> + for (; i > 0; i--) {
> + next = bitmap_find_next_zero_area(data->bm,
> + size, 0, i, 0);
> + if (next < size)
> + break;
> + }
> +
> + if (next >= size || i != nvec) {
> + ret = i ? : -ENOENT;
> + } else {
> + bitmap_set(data->bm, next, nvec);
> + *irq = data->spi_start + next;
> + ret = 0;
> + }
...and change this if-else block to:
if (next < size) {
bitmap_set(data->bm, next, nvec);
*irq = data->spi_start + next;
ret = 0;
}
> +
> + spin_unlock(&data->msi_cnt_lock);
> +
> + return ret;
> +}
[...]
> +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 v2m_data *data = to_v2m_data(chip);
> +
> + 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;
> + }
As mentioned above, in all failure cases for alloc_msi_irq we simply
return -ENOSPC here, so there's no need for alloc_msi_irq to be so
complicated.
We can move the alloc_msi_irq() call into the test, and get rid of the
avail variable.
> + irq_set_chip_data(irq, chip);
> + irq_set_msi_desc(irq, desc);
> + irq_set_irq_type(irq, IRQ_TYPE_EDGE_RISING);
> +
> + addr = data->res.start + MSI_SETSPI_NS;
> +
> + msg.address_hi = (u32)(addr >> 32);
> + msg.address_lo = (u32)(addr);
> + msg.data = irq;
> +#ifdef CONFIG_PCI_MSI
> + write_msi_msg(irq, &msg);
> +#endif
Is it worth doing any of the above if !CONFIG_PCI_MSI?
> + return 0;
> +}
> +
> +static int __init
> +gicv2m_msi_init(struct device_node *node, struct v2m_data *v2m)
> +{
> + unsigned int val;
> +
> + if (of_address_to_resource(node, GIC_OF_MSIV2M_RANGE_INDEX,
> + &v2m->res)) {
> + pr_err("GICv2m: Failed locate GICv2m MSI register frame\n");
> + return -EINVAL;
> + }
> +
> + v2m->base = of_iomap(node, GIC_OF_MSIV2M_RANGE_INDEX);
of_iomap can return NULL...
> + /*
> + * MSI_TYPER:
> + * [31:26] Reserved
> + * [25:16] lowest SPI assigned to MSI
> + * [15:10] Reserved
> + * [9:0] Numer of SPIs assigned to MSI
> + */
> + val = readl_relaxed(v2m->base + MSI_TYPER);
> + if (!val) {
> + pr_warn("GICv2m: Failed to read MSI_TYPER register\n");
> + return -EINVAL;
> + }
> +
> + v2m->spi_start = (val >> 16) & 0x3ff;
> + v2m->nr_spis = val & 0x3ff;
Rather than have the comment above the readl_relaxed, Why not define
some macros for these magic numbers and keep the comment with them?
> +
> + if (v2m->spi_start < GIC_V2M_MIN_SPI ||
> + v2m->nr_spis >= GIC_V2M_MAX_SPI) {
> + pr_err("GICv2m: Init failed\n");
> + return -EINVAL;
> + }
It would be nice to point out why we failed here: the hardware reported
bad values.
> + v2m->bm = kzalloc(sizeof(long) * BITS_TO_LONGS(v2m->nr_spis),
> + GFP_KERNEL);
This looks better than last time :)
[...]
> + ret = of_pci_msi_chip_add(&gic->msi_chip);
> + if (ret) {
> + /* MSI is optional and not supported here */
> + pr_warn("GICv2m: MSI is not supported.\n");
> + return 0;
> + }
Why not pr_info?
Thanks,
Mark.
Thanks again for the review. Please see my comments below.
On 7/2/2014 11:39 AM, Mark Rutland wrote:
> On Wed, Jul 02, 2014 at 04:22:23PM +0100, [email protected] wrote:
>> From: Suravee Suthikulpanit <[email protected]>
>> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
>> index 5573c08..9e46f7f 100644
>> --- a/Documentation/devicetree/bindings/arm/gic.txt
>> +++ b/Documentation/devicetree/bindings/arm/gic.txt
>> @@ -12,11 +12,13 @@ Main node required properties:
>>
>> - compatible : should be one of:
>> "arm,gic-400"
>> + "arm,gic-400-plus"
>
> I am not keen on this name.
Ok, I'll change it. Any suggestion on name? I'm not sure what is the
"official" product name. I've seen this in some slides from ARM. What
about "gic-400-v2m"??.
>> "arm,cortex-a15-gic"
>> "arm,cortex-a9-gic"
>> "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.
>
> Random (inconsistent) whitespace change?
It looks to me like there should have been a space here to keep the
consistent look, and make it easy to read
>> @@ -37,9 +39,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)
>
> As far as I am aware, the MSI interface is completely orthogonal to
> having a GICH and GICV.
Agree. I'm not doing anything with it. I'm just listing them here since
they are also mentioned in the gic.txt
>
> We should figure out how we expect to handle that (zero-sized reg
> entries? reg-names?).
I'm not sure how VGIC stuff handles reg/size = 0.
>
>>
>> Optional
>> - interrupts : Interrupt source of the parent interrupt controller on
>> @@ -55,6 +64,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.
>
> That last clarifying comment is more confusing than helpful.
If you are referring to the table, I added that since it was easier to
see than scanning the text.
> [...]
>
>> +#define GIC_V2M_MIN_SPI 32
>> +#define GIC_V2M_MAX_SPI 1024
>
> GIC interrupt IDs 1020 and above are reserved, no?
>
> Surely the max ID an SPI can take is 1019?
Right, thanks for catching. But the spec says that the SPI ID value must
be in the range of 32 to 1020. I guess it was a bit unclear, but
definitely not 1024 :)
>> +#define GIC_OF_MSIV2M_RANGE_INDEX 4
>> +
>> +/**
>> + * alloc_msi_irq - Allocate MSIs from avaialbe MSI bitmap.
>> + * @data: Pointer to v2m_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 available. Otherwise return the number
>> + * of available interrupts. If none are available, then returns -ENOENT.
>> + */
>
> This function is overly complicated, and pointlessly so.
>
> It doesn't achieve anything useful as it returns the size of the _last_
> contiguous block rather than the _largest_ contiguous block, and the
> only caller (gicv2m_setup_msi_irq) doesn't even care.
>
> Simplify this to just return an error code if allocation is impossible.
Actually, I made another mistake in the gicv2m_setup_msi_irq when
returning from the alloc_msi_irq().
My understanding is the arch_setup_msi_irqs() is supposed to return the
number of available vectors if the requested amount could not be
allocated. I notice that the current "drivers/pci/msi.c:
arch_setup_msi_irqs()" does not do so, which is okay.
However, We are also working on adding support for multi-MSI support
since some of the devices we have are using it, which means we will need
to provide a different version of the "arch_setup_msi_irqs()" as the
current one does not allow (PCI_CAP_ID_MSI && nvec > 1).
Therefore, I implemented the "alloc_msi_irq" this way to account for
future changes.
Thanks,
Suravee
On Wed, Jul 02, 2014 at 09:04:09PM +0100, Suravee Suthikulanit wrote:
> Thanks again for the review. Please see my comments below.
>
> On 7/2/2014 11:39 AM, Mark Rutland wrote:
> > On Wed, Jul 02, 2014 at 04:22:23PM +0100, [email protected] wrote:
> >> From: Suravee Suthikulpanit <[email protected]>
> >> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
> >> index 5573c08..9e46f7f 100644
> >> --- a/Documentation/devicetree/bindings/arm/gic.txt
> >> +++ b/Documentation/devicetree/bindings/arm/gic.txt
> >> @@ -12,11 +12,13 @@ Main node required properties:
> >>
> >> - compatible : should be one of:
> >> "arm,gic-400"
> >> + "arm,gic-400-plus"
> >
> > I am not keen on this name.
>
> Ok, I'll change it. Any suggestion on name? I'm not sure what is the
> "official" product name. I've seen this in some slides from ARM. What
> about "gic-400-v2m"??.
I'll query this internally.
> >> "arm,cortex-a15-gic"
> >> "arm,cortex-a9-gic"
> >> "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.
> >
> > Random (inconsistent) whitespace change?
> It looks to me like there should have been a space here to keep the
> consistent look, and make it easy to read
As far as I can see it's inconsistent because you didn't add a newline
before the "interrupt-controller" line immediately before.
I'm not against adding spacing between the properties, so long as it is
consistent.
> >> @@ -37,9 +39,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)
> >
> > As far as I am aware, the MSI interface is completely orthogonal to
> > having a GICH and GICV.
>
> Agree. I'm not doing anything with it. I'm just listing them here since
> they are also mentioned in the gic.txt
>
> >
> > We should figure out how we expect to handle that (zero-sized reg
> > entries? reg-names?).
>
> I'm not sure how VGIC stuff handles reg/size = 0.
Neither am I.
However, what I said was that we should figure out how we _expect_ to
handle that case. If we have to make changes to handle it, that's fine
given we already have to make changes to support GICv2m.
> >
> >>
> >> Optional
> >> - interrupts : Interrupt source of the parent interrupt controller on
> >> @@ -55,6 +64,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.
> >
> > That last clarifying comment is more confusing than helpful.
>
> If you are referring to the table, I added that since it was easier to
> see than scanning the text.
I was referring to "This requires the register region index 4".
How about:
- msi-controller : Identifies the node as an MSI controller. Requried
for GICv2m.
>
> > [...]
> >
> >> +#define GIC_V2M_MIN_SPI 32
> >> +#define GIC_V2M_MAX_SPI 1024
> >
> > GIC interrupt IDs 1020 and above are reserved, no?
> >
> > Surely the max ID an SPI can take is 1019?
>
> Right, thanks for catching. But the spec says that the SPI ID value must
> be in the range of 32 to 1020. I guess it was a bit unclear, but
> definitely not 1024 :)
Which spec?
In the GICv2 spec I see:
* "Interrupt numbers ID32-ID1019 are used for SPIs." in
2.2.1 Interrupt IDs.
* "The GIC architecture reserves interrupt ID numbers 1020-1023 for
special purposes." in 3.2.5 Special interrupt numbers.
>
> >> +#define GIC_OF_MSIV2M_RANGE_INDEX 4
> >> +
> >> +/**
> >> + * alloc_msi_irq - Allocate MSIs from avaialbe MSI bitmap.
> >> + * @data: Pointer to v2m_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 available. Otherwise return the number
> >> + * of available interrupts. If none are available, then returns -ENOENT.
> >> + */
> >
> > This function is overly complicated, and pointlessly so.
> >
> > It doesn't achieve anything useful as it returns the size of the _last_
> > contiguous block rather than the _largest_ contiguous block, and the
> > only caller (gicv2m_setup_msi_irq) doesn't even care.
> >
> > Simplify this to just return an error code if allocation is impossible.
>
> Actually, I made another mistake in the gicv2m_setup_msi_irq when
> returning from the alloc_msi_irq().
Ok.
> My understanding is the arch_setup_msi_irqs() is supposed to return the
> number of available vectors if the requested amount could not be
> allocated. I notice that the current "drivers/pci/msi.c:
> arch_setup_msi_irqs()" does not do so, which is okay.
>
> However, We are also working on adding support for multi-MSI support
> since some of the devices we have are using it, which means we will need
> to provide a different version of the "arch_setup_msi_irqs()" as the
> current one does not allow (PCI_CAP_ID_MSI && nvec > 1).
>
> Therefore, I implemented the "alloc_msi_irq" this way to account for
> future changes.
Per my comments, I still think the function is broken given it returns
the size of the last contiguous block of available interrupts.
If we don't yet have support for multi-MSI, just return an error code
for now. The code isn't helpful and that path isn't tested.
We can fix up the driver when we add multi-MSI support.
Thanks,
Mark.
On 7/3/2014 3:46 AM, Mark Rutland wrote:
> On Wed, Jul 02, 2014 at 09:04:09PM +0100, Suravee Suthikulanit wrote:
>> Thanks again for the review. Please see my comments below.
>>
>> On 7/2/2014 11:39 AM, Mark Rutland wrote:
>>> On Wed, Jul 02, 2014 at 04:22:23PM +0100, [email protected] wrote:
>>>> From: Suravee Suthikulpanit <[email protected]>
>>>> diff --git a/Documentation/devicetree/bindings/arm/gic.txt b/Documentation/devicetree/bindings/arm/gic.txt
>>>> index 5573c08..9e46f7f 100644
>>>> --- a/Documentation/devicetree/bindings/arm/gic.txt
>>>> +++ b/Documentation/devicetree/bindings/arm/gic.txt
>>>> @@ -12,11 +12,13 @@ Main node required properties:
>>>>
>>>> - compatible : should be one of:
>>>> "arm,gic-400"
>>>> + "arm,gic-400-plus"
>>>
>>> I am not keen on this name.
>>
>> Ok, I'll change it. Any suggestion on name? I'm not sure what is the
>> "official" product name. I've seen this in some slides from ARM. What
>> about "gic-400-v2m"??.
>
> I'll query this internally.
Thanks.
>>>> @@ -37,9 +39,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)
>>>
>>> As far as I am aware, the MSI interface is completely orthogonal to
>>> having a GICH and GICV.
>>
>> Agree. I'm not doing anything with it. I'm just listing them here since
>> they are also mentioned in the gic.txt
>>
>>>
>>> We should figure out how we expect to handle that (zero-sized reg
>>> entries? reg-names?).
>>
>> I'm not sure how VGIC stuff handles reg/size = 0.
>
> Neither am I.
>
> However, what I said was that we should figure out how we _expect_ to
> handle that case. If we have to make changes to handle it, that's fine
> given we already have to make changes to support GICv2m.
Looking through the code in virt/kvm/arm/vgic.c, I believe this would
ended up returning -EINVAL or -ENXIO from kvm_vgic_hyp_init().
>>>
>>>>
>>>> Optional
>>>> - interrupts : Interrupt source of the parent interrupt controller on
>>>> @@ -55,6 +64,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.
>>>
>>> That last clarifying comment is more confusing than helpful.
>>
>> If you are referring to the table, I added that since it was easier to
>> see than scanning the text.
>
> I was referring to "This requires the register region index 4".
>
> How about:
>
> - msi-controller : Identifies the node as an MSI controller. Requried
> for GICv2m.
OK
>>
>>> [...]
>>>
>>>> +#define GIC_V2M_MIN_SPI 32
>>>> +#define GIC_V2M_MAX_SPI 1024
>>>
>>> GIC interrupt IDs 1020 and above are reserved, no?
>>>
>>> Surely the max ID an SPI can take is 1019?
>>
>> Right, thanks for catching. But the spec says that the SPI ID value must
>> be in the range of 32 to 1020. I guess it was a bit unclear, but
>> definitely not 1024 :)
>
> Which spec?
This was in the "Server Base System Architecture v1.0".
>
> In the GICv2 spec I see:
>
> * "Interrupt numbers ID32-ID1019 are used for SPIs." in
> 2.2.1 Interrupt IDs.
>
> * "The GIC architecture reserves interrupt ID numbers 1020-1023 for
> special purposes." in 3.2.5 Special interrupt numbers.
>
>>
>>>> +#define GIC_OF_MSIV2M_RANGE_INDEX 4
>>>> +
>>>> +/**
>>>> + * alloc_msi_irq - Allocate MSIs from avaialbe MSI bitmap.
>>>> + * @data: Pointer to v2m_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 available. Otherwise return the number
>>>> + * of available interrupts. If none are available, then returns -ENOENT.
>>>> + */
>>>
>>> This function is overly complicated, and pointlessly so.
>>>
>>> It doesn't achieve anything useful as it returns the size of the _last_
>>> contiguous block rather than the _largest_ contiguous block, and the
>>> only caller (gicv2m_setup_msi_irq) doesn't even care.
>>>
>>> Simplify this to just return an error code if allocation is impossible.
>>
>> Actually, I made another mistake in the gicv2m_setup_msi_irq when
>> returning from the alloc_msi_irq().
>
> Ok.
>
>> My understanding is the arch_setup_msi_irqs() is supposed to return the
>> number of available vectors if the requested amount could not be
>> allocated. I notice that the current "drivers/pci/msi.c:
>> arch_setup_msi_irqs()" does not do so, which is okay.
>>
>> However, We are also working on adding support for multi-MSI support
>> since some of the devices we have are using it, which means we will need
>> to provide a different version of the "arch_setup_msi_irqs()" as the
>> current one does not allow (PCI_CAP_ID_MSI && nvec > 1).
>>
>> Therefore, I implemented the "alloc_msi_irq" this way to account for
>> future changes.
>
> Per my comments, I still think the function is broken given it returns
> the size of the last contiguous block of available interrupts.
Actually, the current logic for alloc_msi_irq() should do the following:
- Return the next available contiguous block of nvec.
- In case the requested nvec is not available, return the size of
of the largest available range, since the for loop always call the
"bitmap_find_next_zero_area()" from the first index of the bitmap.
Actually, let me just add the code for multiple MSIs, which should be
making use of this function also in the V3 patch set. That way, it
would be clear.
Thanks,
Suravee
Suravee