[PATCH 0/6] irqchip: introduce the Alpine MSIX driver

[PATCH 0/6] irqchip: introduce the Alpine MSIX driver

[Antoine Tenart]

Hi all,

This series introduce the Alpine MSIX driver, and enables it in both
the Alpine v1 and Alpine v2 device trees.

This series depends on "[PATCH 0/3] arm64: introduce the Alpine support":
https://lkml.org/lkml/2016/2/8/75

You can find the series at:
https://github.com/atenart/linux.git 4.5-rc1/alpinev2-msix

Antoine

Antoine Tenart (6):
  irqchip: add the Alpine MSIX interrupt controller
  Documentation: bindings: document the Alpine MSIX driver
  arm64: dts: alpine: add the MSIX node in the Alpine v2 dtsi
  ARM: dts: alpine: add the MSIX node
  arm64: alpine: select the Alpine MSI controller driver
  arm: alpine: select the Alpine MSI controller driver

 .../interrupt-controller/al,alpine-msix.txt        |  22 ++
 arch/arm/boot/dts/alpine.dtsi                      |  10 +
 arch/arm/mach-alpine/Kconfig                       |   1 +
 arch/arm64/Kconfig.platforms                       |   1 +
 arch/arm64/boot/dts/al/alpine-v2.dtsi              |  10 +
 drivers/irqchip/Kconfig                            |   6 +
 drivers/irqchip/Makefile                           |   1 +
 drivers/irqchip/irq-alpine-msi.c                   | 297 +++++++++++++++++++++
 8 files changed, 348 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/al,alpine-msix.txt
 create mode 100644 drivers/irqchip/irq-alpine-msi.c

-- 
2.7.0

[PATCH 1/6] irqchip: add the Alpine MSIX interrupt controller

[Antoine Tenart]

This patch adds the Alpine MSIX interrupt controller driver.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
Signed-off-by: Tsahee Zidenberg <tsahee@annapurnalabs.com>
---
 drivers/irqchip/Kconfig          |   6 +
 drivers/irqchip/Makefile         |   1 +
 drivers/irqchip/irq-alpine-msi.c | 297 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 304 insertions(+)
 create mode 100644 drivers/irqchip/irq-alpine-msi.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 715923d5236c..f20e5b28eb5f 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -60,6 +60,12 @@ config ARM_VIC_NR
 	  The maximum number of VICs available in the system, for
 	  power management.
 
+config ALPINE_MSI
+	bool
+	depends on PCI && PCI_MSI
+	select GENERIC_IRQ_CHIP
+	select PCI_MSI_IRQ_DOMAIN
+
 config ATMEL_AIC_IRQ
 	bool
 	select GENERIC_IRQ_CHIP
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index 18caacb60d58..57f68e0eea74 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -1,5 +1,6 @@
 obj-$(CONFIG_IRQCHIP)			+= irqchip.o
 
+obj-$(CONFIG_ALPINE_MSI)		+= irq-alpine-msi.o
 obj-$(CONFIG_ARCH_BCM2835)		+= irq-bcm2835.o
 obj-$(CONFIG_ARCH_BCM2835)		+= irq-bcm2836.o
 obj-$(CONFIG_ARCH_EXYNOS)		+= exynos-combiner.o
diff --git a/drivers/irqchip/irq-alpine-msi.c b/drivers/irqchip/irq-alpine-msi.c
new file mode 100644
index 000000000000..435dd4ab3626
--- /dev/null
+++ b/drivers/irqchip/irq-alpine-msi.c
@@ -0,0 +1,297 @@
+/*
+ * Annapurna Labs MSIX support services
+ *
+ * Copyright (C) 2016, Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ *
+ * Antoine Tenart <antoine.tenart@free-electrons.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/irqchip.h>
+#include <linux/irqchip/arm-gic.h>
+#include <linux/msi.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_pci.h>
+#include <linux/pci.h>
+#include <linux/slab.h>
+
+#include <asm/irq.h>
+#include <asm-generic/msi.h>
+
+/* MSIX message address format: local GIC target */
+#define ALPINE_MSIX_SPI_TARGET_CLUSTER0		BIT(16)
+
+struct alpine_msix_data {
+	spinlock_t msi_map_lock;
+	u32 addr_high;
+	u32 addr_low;
+	u32 spi_first;		/* The SGI number that MSIs start */
+	u32 num_spis;		/* The number of SGIs for MSIs */
+	unsigned long *msi_map;
+};
+
+static void alpine_msix_mask_msi_irq(struct irq_data *d)
+{
+	pci_msi_mask_irq(d);
+	irq_chip_mask_parent(d);
+}
+
+static void alpine_msix_unmask_msi_irq(struct irq_data *d)
+{
+	pci_msi_unmask_irq(d);
+	irq_chip_unmask_parent(d);
+}
+
+static int alpine_msix_set_affinity(struct irq_data *irq_data,
+				    const struct cpumask *mask, bool force)
+{
+	int ret;
+
+	ret = irq_chip_set_affinity_parent(irq_data, mask, force);
+	return ret == IRQ_SET_MASK_OK ? IRQ_SET_MASK_OK_DONE : ret;
+}
+
+static struct irq_chip alpine_msix_irq_chip = {
+	.name			= "MSIx",
+	.irq_mask		= alpine_msix_mask_msi_irq,
+	.irq_unmask		= alpine_msix_unmask_msi_irq,
+	.irq_eoi		= irq_chip_eoi_parent,
+	.irq_set_affinity	= alpine_msix_set_affinity,
+};
+
+static int alpine_msix_allocate_sgi(struct alpine_msix_data *priv, int num_req)
+{
+	int first, i;
+
+	spin_lock(&priv->msi_map_lock);
+
+	first = bitmap_find_next_zero_area(priv->msi_map, priv->num_spis, 0,
+					   num_req, 0);
+	if (first >= priv->num_spis) {
+		spin_unlock(&priv->msi_map_lock);
+		return -ENOSPC;
+	}
+
+	for (i = 0; i < num_req; i++)
+		set_bit(first + i, priv->msi_map);
+
+	spin_unlock(&priv->msi_map_lock);
+
+	return priv->spi_first + first;
+}
+
+static void alpine_msix_free_sgi(struct alpine_msix_data *priv, unsigned sgi,
+				 int num_req)
+{
+	int i, first;
+
+	first = sgi - priv->spi_first;
+
+	spin_lock(&priv->msi_map_lock);
+
+	for (i = 0; i < num_req; i++)
+		clear_bit(first + i, priv->msi_map);
+
+	spin_unlock(&priv->msi_map_lock);
+}
+
+static void alpine_msix_compose_msi_msg(struct irq_data *data,
+					struct msi_msg *msg)
+{
+	struct alpine_msix_data *priv = irq_data_get_irq_chip_data(data);
+
+	msg->address_hi = priv->addr_high;
+	msg->address_lo = priv->addr_low + (data->hwirq << 3);
+	msg->data = 0;
+}
+
+static struct msi_domain_info alpine_msix_domain_info = {
+	.flags	= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
+		  MSI_FLAG_PCI_MSIX,
+	.chip	= &alpine_msix_irq_chip,
+};
+
+static struct irq_chip middle_irq_chip = {
+	.name			= "alpine_msix_middle",
+	.irq_mask		= irq_chip_mask_parent,
+	.irq_unmask		= irq_chip_unmask_parent,
+	.irq_eoi		= irq_chip_eoi_parent,
+	.irq_set_affinity	= irq_chip_set_affinity_parent,
+	.irq_compose_msi_msg	= alpine_msix_compose_msi_msg,
+};
+
+static int alpine_msix_gic_domain_alloc(struct irq_domain *domain,
+					unsigned int virq, int sgi)
+{
+	struct irq_fwspec fwspec;
+	struct irq_data *d;
+	int ret;
+
+	if (!is_of_node(domain->parent->fwnode))
+		return -EINVAL;
+
+	fwspec.fwnode = domain->parent->fwnode;
+	fwspec.param_count = 3;
+	fwspec.param[0] = 0;
+	fwspec.param[1] = sgi;
+	fwspec.param[2] = IRQ_TYPE_EDGE_RISING;
+
+	ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
+	if (ret)
+		return ret;
+
+	d = irq_domain_get_irq_data(domain->parent, virq);
+	d->chip->irq_set_type(d, IRQ_TYPE_EDGE_RISING);
+
+	return 0;
+}
+
+static int alpine_msix_middle_domain_alloc(struct irq_domain *domain,
+					   unsigned int virq,
+					   unsigned int nr_irqs, void *args)
+{
+	struct alpine_msix_data *priv = domain->host_data;
+	int sgi, err, i;
+
+	sgi = alpine_msix_allocate_sgi(priv, nr_irqs);
+	if (sgi < 0)
+		return sgi;
+
+	for (i = 0; i < nr_irqs; i++) {
+		err = alpine_msix_gic_domain_alloc(domain, virq + i, sgi + i);
+		if (err)
+			goto err_sgi;
+
+		irq_domain_set_hwirq_and_chip(domain, virq + i, sgi + i,
+					      &middle_irq_chip, priv);
+	}
+
+	return 0;
+
+err_sgi:
+	while (--i >= 0)
+		irq_domain_free_irqs_parent(domain, virq, i);
+	alpine_msix_free_sgi(priv, sgi, nr_irqs);
+	return err;
+}
+
+static void alpine_msix_middle_domain_free(struct irq_domain *domain,
+					   unsigned int virq,
+					   unsigned int nr_irqs)
+{
+	struct irq_data *d = irq_domain_get_irq_data(domain, virq);
+	struct alpine_msix_data *priv = irq_data_get_irq_chip_data(d);
+
+	irq_domain_free_irqs_parent(domain, virq, nr_irqs);
+	alpine_msix_free_sgi(priv, d->hwirq, nr_irqs);
+}
+
+static const struct irq_domain_ops alpine_msix_middle_domain_ops = {
+	.alloc	= alpine_msix_middle_domain_alloc,
+	.free	= alpine_msix_middle_domain_free,
+};
+
+static int alpine_msix_init_domains(struct alpine_msix_data *priv,
+				    struct device_node *node)
+{
+	struct irq_domain *middle_domain, *msi_domain, *gic_domain;
+	struct device_node *gic_node;
+
+	gic_node = of_irq_find_parent(node);
+	if (!gic_node) {
+		pr_err("Failed to find the GIC node\n");
+		return -ENODEV;
+	}
+
+	gic_domain = irq_find_host(gic_node);
+	if (!gic_domain) {
+		pr_err("Failed to find the GIC domain\n");
+		return -ENXIO;
+	}
+
+	middle_domain = irq_domain_add_tree(NULL,
+					    &alpine_msix_middle_domain_ops,
+					    priv);
+	if (!middle_domain) {
+		pr_err("Failed to create the MSIX middle domain\n");
+		return -ENOMEM;
+	}
+
+	middle_domain->parent = gic_domain;
+
+	msi_domain = pci_msi_create_irq_domain(of_node_to_fwnode(node),
+					       &alpine_msix_domain_info,
+					       middle_domain);
+	if (!msi_domain) {
+		pr_err("Failed to create MSI domain\n");
+		irq_domain_remove(msi_domain);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int alpine_msix_init(struct device_node *node,
+			    struct device_node *parent)
+{
+	struct alpine_msix_data *priv;
+	struct resource res;
+	int ret;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	spin_lock_init(&priv->msi_map_lock);
+
+	ret = of_address_to_resource(node, 0, &res);
+	if (ret) {
+		pr_err("Failed to allocate resource\n");
+		goto err_priv;
+	}
+
+	priv->addr_high = upper_32_bits((u64)res.start);
+	priv->addr_low = lower_32_bits(res.start) + ALPINE_MSIX_SPI_TARGET_CLUSTER0;
+
+	if (of_property_read_u32(node, "al,msi-base-spi", &priv->spi_first)) {
+		pr_err("Unable to parse MSI base\n");
+		ret = -EINVAL;
+		goto err_priv;
+	}
+
+	if (of_property_read_u32(node, "al,msi-num-spis", &priv->num_spis)) {
+		pr_err("Unable to parse MSI numbers\n");
+		ret = -EINVAL;
+		goto err_priv;
+	}
+
+	priv->msi_map = kzalloc(sizeof(*priv->msi_map) * BITS_TO_LONGS(priv->num_spis),
+				GFP_KERNEL);
+	if (!priv->msi_map) {
+		ret = -ENOMEM;
+		goto err_priv;
+	}
+
+	pr_debug("Registering %d msixs, starting at %d\n",
+		 priv->num_spis, priv->spi_first);
+
+	ret = alpine_msix_init_domains(priv, node);
+	if (ret)
+		goto err_map;
+
+	return 0;
+
+err_map:
+	kfree(priv->msi_map);
+err_priv:
+	kfree(priv);
+	return ret;
+}
+IRQCHIP_DECLARE(alpine_msix, "al,alpine-msix", alpine_msix_init);
-- 
2.7.0

[PATCH 1/6] irqchip: add the Alpine MSIX interrupt controller

[Marc Zyngier]

Hi Antoine,

On 08/02/16 09:16, Antoine Tenart wrote:
> This patch adds the Alpine MSIX interrupt controller driver.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> Signed-off-by: Tsahee Zidenberg <tsahee@annapurnalabs.com>
> ---
>  drivers/irqchip/Kconfig          |   6 +
>  drivers/irqchip/Makefile         |   1 +
>  drivers/irqchip/irq-alpine-msi.c | 297 +++++++++++++++++++++++++++++++++++++++
>  3 files changed, 304 insertions(+)
>  create mode 100644 drivers/irqchip/irq-alpine-msi.c
> 
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 715923d5236c..f20e5b28eb5f 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -60,6 +60,12 @@ config ARM_VIC_NR
>  	  The maximum number of VICs available in the system, for
>  	  power management.
>  
> +config ALPINE_MSI
> +	bool
> +	depends on PCI && PCI_MSI
> +	select GENERIC_IRQ_CHIP
> +	select PCI_MSI_IRQ_DOMAIN
> +
>  config ATMEL_AIC_IRQ
>  	bool
>  	select GENERIC_IRQ_CHIP
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 18caacb60d58..57f68e0eea74 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -1,5 +1,6 @@
>  obj-$(CONFIG_IRQCHIP)			+= irqchip.o
>  
> +obj-$(CONFIG_ALPINE_MSI)		+= irq-alpine-msi.o
>  obj-$(CONFIG_ARCH_BCM2835)		+= irq-bcm2835.o
>  obj-$(CONFIG_ARCH_BCM2835)		+= irq-bcm2836.o
>  obj-$(CONFIG_ARCH_EXYNOS)		+= exynos-combiner.o
> diff --git a/drivers/irqchip/irq-alpine-msi.c b/drivers/irqchip/irq-alpine-msi.c
> new file mode 100644
> index 000000000000..435dd4ab3626
> --- /dev/null
> +++ b/drivers/irqchip/irq-alpine-msi.c
> @@ -0,0 +1,297 @@
> +/*
> + * Annapurna Labs MSIX support services
> + *
> + * Copyright (C) 2016, Amazon.com, Inc. or its affiliates. All Rights Reserved.
> + *
> + * Antoine Tenart <antoine.tenart@free-electrons.com>
> + *
> + * This file is licensed under the terms of the GNU General Public
> + * License version 2. This program is licensed "as is" without any
> + * warranty of any kind, whether express or implied.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/irqchip.h>
> +#include <linux/irqchip/arm-gic.h>
> +#include <linux/msi.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_pci.h>
> +#include <linux/pci.h>
> +#include <linux/slab.h>
> +
> +#include <asm/irq.h>
> +#include <asm-generic/msi.h>
> +
> +/* MSIX message address format: local GIC target */
> +#define ALPINE_MSIX_SPI_TARGET_CLUSTER0		BIT(16)
> +
> +struct alpine_msix_data {
> +	spinlock_t msi_map_lock;
> +	u32 addr_high;
> +	u32 addr_low;

As this looks to be a physical address, please consider using phys_addr_t.

> +	u32 spi_first;		/* The SGI number that MSIs start */
> +	u32 num_spis;		/* The number of SGIs for MSIs */
> +	unsigned long *msi_map;
> +};
> +
> +static void alpine_msix_mask_msi_irq(struct irq_data *d)
> +{
> +	pci_msi_mask_irq(d);
> +	irq_chip_mask_parent(d);
> +}
> +
> +static void alpine_msix_unmask_msi_irq(struct irq_data *d)
> +{
> +	pci_msi_unmask_irq(d);
> +	irq_chip_unmask_parent(d);
> +}
> +
> +static int alpine_msix_set_affinity(struct irq_data *irq_data,
> +				    const struct cpumask *mask, bool force)
> +{
> +	int ret;
> +
> +	ret = irq_chip_set_affinity_parent(irq_data, mask, force);
> +	return ret == IRQ_SET_MASK_OK ? IRQ_SET_MASK_OK_DONE : ret;
> +}
> +
> +static struct irq_chip alpine_msix_irq_chip = {
> +	.name			= "MSIx",
> +	.irq_mask		= alpine_msix_mask_msi_irq,
> +	.irq_unmask		= alpine_msix_unmask_msi_irq,
> +	.irq_eoi		= irq_chip_eoi_parent,
> +	.irq_set_affinity	= alpine_msix_set_affinity,
> +};
> +
> +static int alpine_msix_allocate_sgi(struct alpine_msix_data *priv, int num_req)
> +{
> +	int first, i;
> +
> +	spin_lock(&priv->msi_map_lock);
> +
> +	first = bitmap_find_next_zero_area(priv->msi_map, priv->num_spis, 0,
> +					   num_req, 0);
> +	if (first >= priv->num_spis) {
> +		spin_unlock(&priv->msi_map_lock);
> +		return -ENOSPC;
> +	}
> +
> +	for (i = 0; i < num_req; i++)
> +		set_bit(first + i, priv->msi_map);
> +
> +	spin_unlock(&priv->msi_map_lock);
> +
> +	return priv->spi_first + first;
> +}
> +
> +static void alpine_msix_free_sgi(struct alpine_msix_data *priv, unsigned sgi,
> +				 int num_req)
> +{
> +	int i, first;
> +
> +	first = sgi - priv->spi_first;
> +
> +	spin_lock(&priv->msi_map_lock);
> +
> +	for (i = 0; i < num_req; i++)
> +		clear_bit(first + i, priv->msi_map);
> +
> +	spin_unlock(&priv->msi_map_lock);
> +}
> +
> +static void alpine_msix_compose_msi_msg(struct irq_data *data,
> +					struct msi_msg *msg)
> +{
> +	struct alpine_msix_data *priv = irq_data_get_irq_chip_data(data);
> +
> +	msg->address_hi = priv->addr_high;
> +	msg->address_lo = priv->addr_low + (data->hwirq << 3);
> +	msg->data = 0;
> +}
> +
> +static struct msi_domain_info alpine_msix_domain_info = {
> +	.flags	= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> +		  MSI_FLAG_PCI_MSIX,

You can probably add MSI_FLAG_PCI_MSI, it should work as well (MULTI_MSI
obviously won't).

> +	.chip	= &alpine_msix_irq_chip,
> +};
> +
> +static struct irq_chip middle_irq_chip = {
> +	.name			= "alpine_msix_middle",
> +	.irq_mask		= irq_chip_mask_parent,
> +	.irq_unmask		= irq_chip_unmask_parent,
> +	.irq_eoi		= irq_chip_eoi_parent,
> +	.irq_set_affinity	= irq_chip_set_affinity_parent,
> +	.irq_compose_msi_msg	= alpine_msix_compose_msi_msg,
> +};
> +
> +static int alpine_msix_gic_domain_alloc(struct irq_domain *domain,
> +					unsigned int virq, int sgi)
> +{
> +	struct irq_fwspec fwspec;
> +	struct irq_data *d;
> +	int ret;
> +
> +	if (!is_of_node(domain->parent->fwnode))
> +		return -EINVAL;
> +
> +	fwspec.fwnode = domain->parent->fwnode;
> +	fwspec.param_count = 3;
> +	fwspec.param[0] = 0;
> +	fwspec.param[1] = sgi;
> +	fwspec.param[2] = IRQ_TYPE_EDGE_RISING;
> +
> +	ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
> +	if (ret)
> +		return ret;
> +
> +	d = irq_domain_get_irq_data(domain->parent, virq);
> +	d->chip->irq_set_type(d, IRQ_TYPE_EDGE_RISING);
> +
> +	return 0;
> +}
> +
> +static int alpine_msix_middle_domain_alloc(struct irq_domain *domain,
> +					   unsigned int virq,
> +					   unsigned int nr_irqs, void *args)
> +{
> +	struct alpine_msix_data *priv = domain->host_data;
> +	int sgi, err, i;
> +
> +	sgi = alpine_msix_allocate_sgi(priv, nr_irqs);
> +	if (sgi < 0)
> +		return sgi;
> +
> +	for (i = 0; i < nr_irqs; i++) {
> +		err = alpine_msix_gic_domain_alloc(domain, virq + i, sgi + i);
> +		if (err)
> +			goto err_sgi;
> +
> +		irq_domain_set_hwirq_and_chip(domain, virq + i, sgi + i,
> +					      &middle_irq_chip, priv);
> +	}
> +
> +	return 0;
> +
> +err_sgi:
> +	while (--i >= 0)
> +		irq_domain_free_irqs_parent(domain, virq, i);
> +	alpine_msix_free_sgi(priv, sgi, nr_irqs);
> +	return err;
> +}
> +
> +static void alpine_msix_middle_domain_free(struct irq_domain *domain,
> +					   unsigned int virq,
> +					   unsigned int nr_irqs)
> +{
> +	struct irq_data *d = irq_domain_get_irq_data(domain, virq);
> +	struct alpine_msix_data *priv = irq_data_get_irq_chip_data(d);
> +
> +	irq_domain_free_irqs_parent(domain, virq, nr_irqs);
> +	alpine_msix_free_sgi(priv, d->hwirq, nr_irqs);
> +}
> +
> +static const struct irq_domain_ops alpine_msix_middle_domain_ops = {
> +	.alloc	= alpine_msix_middle_domain_alloc,
> +	.free	= alpine_msix_middle_domain_free,
> +};
> +
> +static int alpine_msix_init_domains(struct alpine_msix_data *priv,
> +				    struct device_node *node)
> +{
> +	struct irq_domain *middle_domain, *msi_domain, *gic_domain;
> +	struct device_node *gic_node;
> +
> +	gic_node = of_irq_find_parent(node);
> +	if (!gic_node) {
> +		pr_err("Failed to find the GIC node\n");
> +		return -ENODEV;
> +	}
> +
> +	gic_domain = irq_find_host(gic_node);
> +	if (!gic_domain) {
> +		pr_err("Failed to find the GIC domain\n");
> +		return -ENXIO;
> +	}
> +
> +	middle_domain = irq_domain_add_tree(NULL,
> +					    &alpine_msix_middle_domain_ops,
> +					    priv);
> +	if (!middle_domain) {
> +		pr_err("Failed to create the MSIX middle domain\n");
> +		return -ENOMEM;
> +	}
> +
> +	middle_domain->parent = gic_domain;
> +
> +	msi_domain = pci_msi_create_irq_domain(of_node_to_fwnode(node),
> +					       &alpine_msix_domain_info,
> +					       middle_domain);
> +	if (!msi_domain) {
> +		pr_err("Failed to create MSI domain\n");
> +		irq_domain_remove(msi_domain);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
> +static int alpine_msix_init(struct device_node *node,
> +			    struct device_node *parent)
> +{
> +	struct alpine_msix_data *priv;
> +	struct resource res;
> +	int ret;
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	spin_lock_init(&priv->msi_map_lock);
> +
> +	ret = of_address_to_resource(node, 0, &res);
> +	if (ret) {
> +		pr_err("Failed to allocate resource\n");
> +		goto err_priv;
> +	}
> +
> +	priv->addr_high = upper_32_bits((u64)res.start);
> +	priv->addr_low = lower_32_bits(res.start) + ALPINE_MSIX_SPI_TARGET_CLUSTER0;

This is a bit odd. If you always set bit 16, why isn't that reflected in
the base address coming from the DT?

> +
> +	if (of_property_read_u32(node, "al,msi-base-spi", &priv->spi_first)) {
> +		pr_err("Unable to parse MSI base\n");
> +		ret = -EINVAL;
> +		goto err_priv;
> +	}
> +
> +	if (of_property_read_u32(node, "al,msi-num-spis", &priv->num_spis)) {
> +		pr_err("Unable to parse MSI numbers\n");
> +		ret = -EINVAL;
> +		goto err_priv;
> +	}
> +
> +	priv->msi_map = kzalloc(sizeof(*priv->msi_map) * BITS_TO_LONGS(priv->num_spis),
> +				GFP_KERNEL);
> +	if (!priv->msi_map) {
> +		ret = -ENOMEM;
> +		goto err_priv;
> +	}
> +
> +	pr_debug("Registering %d msixs, starting at %d\n",
> +		 priv->num_spis, priv->spi_first);
> +
> +	ret = alpine_msix_init_domains(priv, node);
> +	if (ret)
> +		goto err_map;
> +
> +	return 0;
> +
> +err_map:
> +	kfree(priv->msi_map);
> +err_priv:
> +	kfree(priv);
> +	return ret;
> +}
> +IRQCHIP_DECLARE(alpine_msix, "al,alpine-msix", alpine_msix_init);
> 

Otherwise, looks pretty good.

Thanks,

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

[PATCH 1/6] irqchip: add the Alpine MSIX interrupt controller

[Thomas Petazzoni]

Hello Marc,

On Mon, 8 Feb 2016 09:44:49 +0000, Marc Zyngier wrote:

> > +static struct msi_domain_info alpine_msix_domain_info = {
> > +	.flags	= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> > +		  MSI_FLAG_PCI_MSIX,
> 
> You can probably add MSI_FLAG_PCI_MSI, it should work as well (MULTI_MSI
> obviously won't).

Why wouldn't MULTI_MSI work? The code is using
bitmap_find_next_zero_area() in alpine_msix_allocate_sgi() precisely to
find num_req consecutive bits set to 0, in order to allocate multiple
MSIs at once. Am I missing something?

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 1/6] irqchip: add the Alpine MSIX interrupt controller

[Marc Zyngier]

Hi Thomas,

On 08/02/16 09:53, Thomas Petazzoni wrote:
> Hello Marc,
> 
> On Mon, 8 Feb 2016 09:44:49 +0000, Marc Zyngier wrote:
> 
>>> +static struct msi_domain_info alpine_msix_domain_info = {
>>> +	.flags	= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
>>> +		  MSI_FLAG_PCI_MSIX,
>>
>> You can probably add MSI_FLAG_PCI_MSI, it should work as well (MULTI_MSI
>> obviously won't).
> 
> Why wouldn't MULTI_MSI work? The code is using
> bitmap_find_next_zero_area() in alpine_msix_allocate_sgi() precisely to
> find num_req consecutive bits set to 0, in order to allocate multiple
> MSIs at once. Am I missing something?

The clue is in the patch:

+	msg->address_lo = priv->addr_low + (data->hwirq << 3);

Multi-MSI imposes a single doorbell address, and consecutive message
identifiers. So while the allocator deals perfectly with the consecutive
IDs part, the fact that you have to encode the message in the address
(instead of putting it in the data field) makes it completely
incompatible with Multi-MSI.

It is a bit silly that brand new HW comes out with such limitations (but
Multi-MSI is such a pain anyway...).

Thanks,

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

[PATCH 1/6] irqchip: add the Alpine MSIX interrupt controller

[Antoine Tenart]

Hi Marc,

On Mon, Feb 08, 2016 at 09:44:49AM +0000, Marc Zyngier wrote:
> On 08/02/16 09:16, Antoine Tenart wrote:
> > +
> > +/* MSIX message address format: local GIC target */
> > +#define ALPINE_MSIX_SPI_TARGET_CLUSTER0		BIT(16)
> > +
> > +struct alpine_msix_data {
> > +	spinlock_t msi_map_lock;
> > +	u32 addr_high;
> > +	u32 addr_low;
> 
> As this looks to be a physical address, please consider using phys_addr_t.

Sure.

[?]

> > +static int alpine_msix_init(struct device_node *node,
> > +			    struct device_node *parent)
> > +{
> > +	struct alpine_msix_data *priv;
> > +	struct resource res;
> > +	int ret;
> > +
> > +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	spin_lock_init(&priv->msi_map_lock);
> > +
> > +	ret = of_address_to_resource(node, 0, &res);
> > +	if (ret) {
> > +		pr_err("Failed to allocate resource\n");
> > +		goto err_priv;
> > +	}
> > +
> > +	priv->addr_high = upper_32_bits((u64)res.start);
> > +	priv->addr_low = lower_32_bits(res.start) + ALPINE_MSIX_SPI_TARGET_CLUSTER0;
> 
> This is a bit odd. If you always set bit 16, why isn't that reflected in
> the base address coming from the DT?

The 20 least significant bits of addr_low provide direct information
regarding the interrupt destination, so I thought it would be clearer
to have this explicitly in the driver so that we know what those bits
mean.

What do you think?


Thanks for the review!

Antoine

-- 
Antoine T?nart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160208/bd26f3fe/attachment-0001.sig>

[PATCH 1/6] irqchip: add the Alpine MSIX interrupt controller

[Marc Zyngier]

On 08/02/16 10:26, Antoine Tenart wrote:
>>> +static int alpine_msix_init(struct device_node *node,
>>> +			    struct device_node *parent)
>>> +{
>>> +	struct alpine_msix_data *priv;
>>> +	struct resource res;
>>> +	int ret;
>>> +
>>> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>>> +	if (!priv)
>>> +		return -ENOMEM;
>>> +
>>> +	spin_lock_init(&priv->msi_map_lock);
>>> +
>>> +	ret = of_address_to_resource(node, 0, &res);
>>> +	if (ret) {
>>> +		pr_err("Failed to allocate resource\n");
>>> +		goto err_priv;
>>> +	}
>>> +
>>> +	priv->addr_high = upper_32_bits((u64)res.start);
>>> +	priv->addr_low = lower_32_bits(res.start) + ALPINE_MSIX_SPI_TARGET_CLUSTER0;
>>
>> This is a bit odd. If you always set bit 16, why isn't that reflected in
>> the base address coming from the DT?
> 
> The 20 least significant bits of addr_low provide direct information
> regarding the interrupt destination, so I thought it would be clearer
> to have this explicitly in the driver so that we know what those bits
> mean.

So what is this information? TARGET_CLUSTER0 is not very expressive, and
doesn't show what the alternatives are. Could you please elaborate a bit
on that front?

Thanks,

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

[PATCH 1/6] irqchip: add the Alpine MSIX interrupt controller

[Antoine Tenart]

On Mon, Feb 08, 2016 at 10:32:08AM +0000, Marc Zyngier wrote:
> On 08/02/16 10:26, Antoine Tenart wrote:
> >>> +static int alpine_msix_init(struct device_node *node,
> >>> +			    struct device_node *parent)
> >>> +{
> >>> +	struct alpine_msix_data *priv;
> >>> +	struct resource res;
> >>> +	int ret;
> >>> +
> >>> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> >>> +	if (!priv)
> >>> +		return -ENOMEM;
> >>> +
> >>> +	spin_lock_init(&priv->msi_map_lock);
> >>> +
> >>> +	ret = of_address_to_resource(node, 0, &res);
> >>> +	if (ret) {
> >>> +		pr_err("Failed to allocate resource\n");
> >>> +		goto err_priv;
> >>> +	}
> >>> +
> >>> +	priv->addr_high = upper_32_bits((u64)res.start);
> >>> +	priv->addr_low = lower_32_bits(res.start) + ALPINE_MSIX_SPI_TARGET_CLUSTER0;
> >>
> >> This is a bit odd. If you always set bit 16, why isn't that reflected in
> >> the base address coming from the DT?
> > 
> > The 20 least significant bits of addr_low provide direct information
> > regarding the interrupt destination, so I thought it would be clearer
> > to have this explicitly in the driver so that we know what those bits
> > mean.
> 
> So what is this information? TARGET_CLUSTER0 is not very expressive, and
> doesn't show what the alternatives are. Could you please elaborate a bit
> on that front?

For now lots of bits are reserved, so there aren't many alternatives.
Bits [18:17] are used to set the GIC to which to route the MSI and bit
16 must be set when this target GIC is the primary GIC (bits [18:17] set
to 0x0). There aren't other options available for now (that I'm aware
of) for the target GIC configuration.

Antoine

-- 
Antoine T?nart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160208/845247f2/attachment.sig>

[PATCH 1/6] irqchip: add the Alpine MSIX interrupt controller

[Marc Zyngier]

On 08/02/16 10:44, Antoine Tenart wrote:
> On Mon, Feb 08, 2016 at 10:32:08AM +0000, Marc Zyngier wrote:
>> On 08/02/16 10:26, Antoine Tenart wrote:
>>>>> +static int alpine_msix_init(struct device_node *node,
>>>>> +			    struct device_node *parent)
>>>>> +{
>>>>> +	struct alpine_msix_data *priv;
>>>>> +	struct resource res;
>>>>> +	int ret;
>>>>> +
>>>>> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
>>>>> +	if (!priv)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	spin_lock_init(&priv->msi_map_lock);
>>>>> +
>>>>> +	ret = of_address_to_resource(node, 0, &res);
>>>>> +	if (ret) {
>>>>> +		pr_err("Failed to allocate resource\n");
>>>>> +		goto err_priv;
>>>>> +	}
>>>>> +
>>>>> +	priv->addr_high = upper_32_bits((u64)res.start);
>>>>> +	priv->addr_low = lower_32_bits(res.start) + ALPINE_MSIX_SPI_TARGET_CLUSTER0;
>>>>
>>>> This is a bit odd. If you always set bit 16, why isn't that reflected in
>>>> the base address coming from the DT?
>>>
>>> The 20 least significant bits of addr_low provide direct information
>>> regarding the interrupt destination, so I thought it would be clearer
>>> to have this explicitly in the driver so that we know what those bits
>>> mean.
>>
>> So what is this information? TARGET_CLUSTER0 is not very expressive, and
>> doesn't show what the alternatives are. Could you please elaborate a bit
>> on that front?
> 
> For now lots of bits are reserved, so there aren't many alternatives.
> Bits [18:17] are used to set the GIC to which to route the MSI and bit
> 16 must be set when this target GIC is the primary GIC (bits [18:17] set
> to 0x0). There aren't other options available for now (that I'm aware
> of) for the target GIC configuration.

OK. So maybe add that as a comment, so that people know what is
happening there. And if the code gets updated to include new
functionalities, it will be easier to track the changes.

Thanks,

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

[PATCH 1/6] irqchip: add the Alpine MSIX interrupt controller

[Antoine Tenart]

On Mon, Feb 08, 2016 at 10:56:16AM +0000, Marc Zyngier wrote:
> On 08/02/16 10:44, Antoine Tenart wrote:
> > On Mon, Feb 08, 2016 at 10:32:08AM +0000, Marc Zyngier wrote:
> >> On 08/02/16 10:26, Antoine Tenart wrote:
> >>>>> +static int alpine_msix_init(struct device_node *node,
> >>>>> +			    struct device_node *parent)
> >>>>> +{
> >>>>> +	struct alpine_msix_data *priv;
> >>>>> +	struct resource res;
> >>>>> +	int ret;
> >>>>> +
> >>>>> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> >>>>> +	if (!priv)
> >>>>> +		return -ENOMEM;
> >>>>> +
> >>>>> +	spin_lock_init(&priv->msi_map_lock);
> >>>>> +
> >>>>> +	ret = of_address_to_resource(node, 0, &res);
> >>>>> +	if (ret) {
> >>>>> +		pr_err("Failed to allocate resource\n");
> >>>>> +		goto err_priv;
> >>>>> +	}
> >>>>> +
> >>>>> +	priv->addr_high = upper_32_bits((u64)res.start);
> >>>>> +	priv->addr_low = lower_32_bits(res.start) + ALPINE_MSIX_SPI_TARGET_CLUSTER0;
> >>>>
> >>>> This is a bit odd. If you always set bit 16, why isn't that reflected in
> >>>> the base address coming from the DT?
> >>>
> >>> The 20 least significant bits of addr_low provide direct information
> >>> regarding the interrupt destination, so I thought it would be clearer
> >>> to have this explicitly in the driver so that we know what those bits
> >>> mean.
> >>
> >> So what is this information? TARGET_CLUSTER0 is not very expressive, and
> >> doesn't show what the alternatives are. Could you please elaborate a bit
> >> on that front?
> > 
> > For now lots of bits are reserved, so there aren't many alternatives.
> > Bits [18:17] are used to set the GIC to which to route the MSI and bit
> > 16 must be set when this target GIC is the primary GIC (bits [18:17] set
> > to 0x0). There aren't other options available for now (that I'm aware
> > of) for the target GIC configuration.
> 
> OK. So maybe add that as a comment, so that people know what is
> happening there. And if the code gets updated to include new
> functionalities, it will be easier to track the changes.

OK, I'll update.

Antoine

-- 
Antoine T?nart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160208/6d946471/attachment-0001.sig>

[PATCH 1/6] irqchip: add the Alpine MSIX interrupt controller

[Antoine Tenart]

Hi Marc,

On Mon, Feb 08, 2016 at 09:44:49AM +0000, Marc Zyngier wrote:
> On 08/02/16 09:16, Antoine Tenart wrote:
> > +
> > +static struct msi_domain_info alpine_msix_domain_info = {
> > +	.flags	= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
> > +		  MSI_FLAG_PCI_MSIX,
> 
> You can probably add MSI_FLAG_PCI_MSI

Are you sure such a flag is available? (Or am I missing something
obvious?).

Antoine

-- 
Antoine T?nart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160208/18f7073a/attachment.sig>

[PATCH 1/6] irqchip: add the Alpine MSIX interrupt controller

[Marc Zyngier]

On 08/02/16 14:04, Antoine Tenart wrote:
> Hi Marc,
> 
> On Mon, Feb 08, 2016 at 09:44:49AM +0000, Marc Zyngier wrote:
>> On 08/02/16 09:16, Antoine Tenart wrote:
>>> +
>>> +static struct msi_domain_info alpine_msix_domain_info = {
>>> +	.flags	= MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
>>> +		  MSI_FLAG_PCI_MSIX,
>>
>> You can probably add MSI_FLAG_PCI_MSI
> 
> Are you sure such a flag is available? (Or am I missing something
> obvious?).

No, I'm just confused (first patch in the morning, what did you
expect?). It looks like we consider single MSI as a given, please ignore
my rambling...

Thanks,

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

[PATCH 1/6] irqchip: add the Alpine MSIX interrupt controller

[Thomas Gleixner]

Antoine,

On Mon, 8 Feb 2016, Antoine Tenart wrote:
> +static int alpine_msix_set_affinity(struct irq_data *irq_data,
> +				    const struct cpumask *mask, bool force)
> +{
> +	int ret;
> +
> +	ret = irq_chip_set_affinity_parent(irq_data, mask, force);
> +	return ret == IRQ_SET_MASK_OK ? IRQ_SET_MASK_OK_DONE : ret;

What's the point of this exercise? Why can't you just set the affinity
callback to irq_chip_set_affinity_parent() ?

> +static struct irq_chip alpine_msix_irq_chip = {
> +	.name			= "MSIx",
> +	.irq_mask		= alpine_msix_mask_msi_irq,
> +	.irq_unmask		= alpine_msix_unmask_msi_irq,
> +	.irq_eoi		= irq_chip_eoi_parent,
> +	.irq_set_affinity	= alpine_msix_set_affinity,
> +};
> +
> +static int alpine_msix_allocate_sgi(struct alpine_msix_data *priv, int num_req)
> +{
> +	int first, i;
> +
> +	spin_lock(&priv->msi_map_lock);
> +
> +	first = bitmap_find_next_zero_area(priv->msi_map, priv->num_spis, 0,
> +					   num_req, 0);
> +	if (first >= priv->num_spis) {
> +		spin_unlock(&priv->msi_map_lock);
> +		return -ENOSPC;
> +	}
> +
> +	for (i = 0; i < num_req; i++)
> +		set_bit(first + i, priv->msi_map);

  bitmap_set() ??

> +
> +	spin_unlock(&priv->msi_map_lock);
> +
> +	return priv->spi_first + first;
> +}
> +
> +static void alpine_msix_free_sgi(struct alpine_msix_data *priv, unsigned sgi,
> +				 int num_req)
> +{
> +	int i, first;
> +
> +	first = sgi - priv->spi_first;
> +
> +	spin_lock(&priv->msi_map_lock);
> +
> +	for (i = 0; i < num_req; i++)
> +		clear_bit(first + i, priv->msi_map);

  bitmap_clear() ??

> +	spin_unlock(&priv->msi_map_lock);
> +}

Thanks,

	tglx

[PATCH 1/6] irqchip: add the Alpine MSIX interrupt controller

[Antoine Tenart]

Thomas,

On Mon, Feb 08, 2016 at 11:31:47AM +0100, Thomas Gleixner wrote:
> On Mon, 8 Feb 2016, Antoine Tenart wrote:
> > +static int alpine_msix_set_affinity(struct irq_data *irq_data,
> > +				    const struct cpumask *mask, bool force)
> > +{
> > +	int ret;
> > +
> > +	ret = irq_chip_set_affinity_parent(irq_data, mask, force);
> > +	return ret == IRQ_SET_MASK_OK ? IRQ_SET_MASK_OK_DONE : ret;
> 
> What's the point of this exercise? Why can't you just set the affinity
> callback to irq_chip_set_affinity_parent() ?

That's what done in irq-gic-v2m.c. Besides that, I see no point. I'll
update for v2.

> > +static struct irq_chip alpine_msix_irq_chip = {
> > +	.name			= "MSIx",
> > +	.irq_mask		= alpine_msix_mask_msi_irq,
> > +	.irq_unmask		= alpine_msix_unmask_msi_irq,
> > +	.irq_eoi		= irq_chip_eoi_parent,
> > +	.irq_set_affinity	= alpine_msix_set_affinity,
> > +};
> > +
> > +static int alpine_msix_allocate_sgi(struct alpine_msix_data *priv, int num_req)
> > +{
> > +	int first, i;
> > +
> > +	spin_lock(&priv->msi_map_lock);
> > +
> > +	first = bitmap_find_next_zero_area(priv->msi_map, priv->num_spis, 0,
> > +					   num_req, 0);
> > +	if (first >= priv->num_spis) {
> > +		spin_unlock(&priv->msi_map_lock);
> > +		return -ENOSPC;
> > +	}
> > +
> > +	for (i = 0; i < num_req; i++)
> > +		set_bit(first + i, priv->msi_map);
> 
>   bitmap_set() ??

Indeed, that's better :)

> > +	spin_unlock(&priv->msi_map_lock);
> > +
> > +	return priv->spi_first + first;
> > +}
> > +
> > +static void alpine_msix_free_sgi(struct alpine_msix_data *priv, unsigned sgi,
> > +				 int num_req)
> > +{
> > +	int i, first;
> > +
> > +	first = sgi - priv->spi_first;
> > +
> > +	spin_lock(&priv->msi_map_lock);
> > +
> > +	for (i = 0; i < num_req; i++)
> > +		clear_bit(first + i, priv->msi_map);
> 
>   bitmap_clear() ??

Ditto.

Antoine

-- 
Antoine T?nart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160208/9197c055/attachment.sig>

[PATCH 1/6] irqchip: add the Alpine MSIX interrupt controller

[Marc Zyngier]

On 08/02/16 14:17, Antoine Tenart wrote:
> Thomas,
> 
> On Mon, Feb 08, 2016 at 11:31:47AM +0100, Thomas Gleixner wrote:
>> On Mon, 8 Feb 2016, Antoine Tenart wrote:
>>> +static int alpine_msix_set_affinity(struct irq_data *irq_data,
>>> +				    const struct cpumask *mask, bool force)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = irq_chip_set_affinity_parent(irq_data, mask, force);
>>> +	return ret == IRQ_SET_MASK_OK ? IRQ_SET_MASK_OK_DONE : ret;
>>
>> What's the point of this exercise? Why can't you just set the affinity
>> callback to irq_chip_set_affinity_parent() ?
> 
> That's what done in irq-gic-v2m.c. Besides that, I see no point. I'll
> update for v2.

That's because there is no need to do another compose_msi_msg/write_msg
in msi_domain_set_affinity() once the affinity has been updated at the
GIC level. Alternatively, updating the GIC driver to always return
IRQ_SET_MASK_OK_DONE would be perfectly acceptable.

Thanks,

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

[PATCH 1/6] irqchip: add the Alpine MSIX interrupt controller

[Antoine Tenart]

On Mon, Feb 08, 2016 at 02:29:12PM +0000, Marc Zyngier wrote:
> On 08/02/16 14:17, Antoine Tenart wrote:
> > Thomas,
> > 
> > On Mon, Feb 08, 2016 at 11:31:47AM +0100, Thomas Gleixner wrote:
> >> On Mon, 8 Feb 2016, Antoine Tenart wrote:
> >>> +static int alpine_msix_set_affinity(struct irq_data *irq_data,
> >>> +				    const struct cpumask *mask, bool force)
> >>> +{
> >>> +	int ret;
> >>> +
> >>> +	ret = irq_chip_set_affinity_parent(irq_data, mask, force);
> >>> +	return ret == IRQ_SET_MASK_OK ? IRQ_SET_MASK_OK_DONE : ret;
> >>
> >> What's the point of this exercise? Why can't you just set the affinity
> >> callback to irq_chip_set_affinity_parent() ?
> > 
> > That's what done in irq-gic-v2m.c. Besides that, I see no point. I'll
> > update for v2.
> 
> That's because there is no need to do another compose_msi_msg/write_msg
> in msi_domain_set_affinity() once the affinity has been updated at the
> GIC level. Alternatively, updating the GIC driver to always return
> IRQ_SET_MASK_OK_DONE would be perfectly acceptable.

I'm using drivers/irqchip/irq-gic-v3.c which is indeed always returning
IRQ_SET_MASK_OK. I'll make a new patch in the v2 of this series to
return IRQ_SET_MASK_OK_DONE instead in the GIC driver (and then patch
irq-gic-v2m.c).

Thanks,

Antoine

-- 
Antoine T?nart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160208/eaae11f1/attachment.sig>

[PATCH 1/6] irqchip: add the Alpine MSIX interrupt controller

[Marc Zyngier]

On 08/02/16 14:48, Antoine Tenart wrote:
> On Mon, Feb 08, 2016 at 02:29:12PM +0000, Marc Zyngier wrote:
>> On 08/02/16 14:17, Antoine Tenart wrote:
>>> Thomas,
>>>
>>> On Mon, Feb 08, 2016 at 11:31:47AM +0100, Thomas Gleixner wrote:
>>>> On Mon, 8 Feb 2016, Antoine Tenart wrote:
>>>>> +static int alpine_msix_set_affinity(struct irq_data *irq_data,
>>>>> +				    const struct cpumask *mask, bool force)
>>>>> +{
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = irq_chip_set_affinity_parent(irq_data, mask, force);
>>>>> +	return ret == IRQ_SET_MASK_OK ? IRQ_SET_MASK_OK_DONE : ret;
>>>>
>>>> What's the point of this exercise? Why can't you just set the affinity
>>>> callback to irq_chip_set_affinity_parent() ?
>>>
>>> That's what done in irq-gic-v2m.c. Besides that, I see no point. I'll
>>> update for v2.
>>
>> That's because there is no need to do another compose_msi_msg/write_msg
>> in msi_domain_set_affinity() once the affinity has been updated at the
>> GIC level. Alternatively, updating the GIC driver to always return
>> IRQ_SET_MASK_OK_DONE would be perfectly acceptable.
> 
> I'm using drivers/irqchip/irq-gic-v3.c which is indeed always returning
> IRQ_SET_MASK_OK. I'll make a new patch in the v2 of this series to
> return IRQ_SET_MASK_OK_DONE instead in the GIC driver (and then patch
> irq-gic-v2m.c).

/me puzzled. GICv3, but no ITS??? WTF???

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

[PATCH 2/6] Documentation: bindings: document the Alpine MSIX driver

[Antoine Tenart]

Following the addition of the Alpine MSIX driver, this patch adds the
corresponding bindings documentation.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
Signed-off-by: Tsahee Zidenberg <tsahee@annapurnalabs.com>
---
 .../interrupt-controller/al,alpine-msix.txt        | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/al,alpine-msix.txt

diff --git a/Documentation/devicetree/bindings/interrupt-controller/al,alpine-msix.txt b/Documentation/devicetree/bindings/interrupt-controller/al,alpine-msix.txt
new file mode 100644
index 000000000000..a77d5a76e8d5
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/al,alpine-msix.txt
@@ -0,0 +1,22 @@
+Alpine MSIX controller
+
+Required properties:
+
+- compatible: should be "al,alpine-msix"
+- reg: physical base address and size of the registers
+- interrupt-controller: identifies the node as an interrupt controller
+- msi-controller: identifies the node as an PCI Message Signaled Interrupt
+		  controller
+- al,msi-base-spi: SPI base of the MSI frame
+- al,msi-num-spis: number of SPIs assigned to the MSI frame
+
+Example:
+
+msix: msix {
+	compatible = "al,alpine-msix";
+	reg = <0x0 0xfbe00000 0x0 0x100000>;
+	interrupt-controller;
+	msi-controller;
+	al,msi-base-spi = <160>;
+	al,msi-num-spis = <160>;
+};
-- 
2.7.0

[PATCH 2/6] Documentation: bindings: document the Alpine MSIX driver

[Marc Zyngier]

On 08/02/16 09:16, Antoine Tenart wrote:
> Following the addition of the Alpine MSIX driver, this patch adds the
> corresponding bindings documentation.
> 
> Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> Signed-off-by: Tsahee Zidenberg <tsahee@annapurnalabs.com>
> ---
>  .../interrupt-controller/al,alpine-msix.txt        | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/al,alpine-msix.txt
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/al,alpine-msix.txt b/Documentation/devicetree/bindings/interrupt-controller/al,alpine-msix.txt
> new file mode 100644
> index 000000000000..a77d5a76e8d5
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/al,alpine-msix.txt
> @@ -0,0 +1,22 @@
> +Alpine MSIX controller
> +
> +Required properties:
> +
> +- compatible: should be "al,alpine-msix"
> +- reg: physical base address and size of the registers
> +- interrupt-controller: identifies the node as an interrupt controller
> +- msi-controller: identifies the node as an PCI Message Signaled Interrupt
> +		  controller
> +- al,msi-base-spi: SPI base of the MSI frame
> +- al,msi-num-spis: number of SPIs assigned to the MSI frame
> +
> +Example:
> +
> +msix: msix {
> +	compatible = "al,alpine-msix";
> +	reg = <0x0 0xfbe00000 0x0 0x100000>;
> +	interrupt-controller;
> +	msi-controller;
> +	al,msi-base-spi = <160>;
> +	al,msi-num-spis = <160>;
> +};
> 

This example seems to rely on an implicit interrupt-parent. You probably
want to update the example to clarify it.

Thanks,

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

[PATCH 2/6] Documentation: bindings: document the Alpine MSIX driver

[Antoine Tenart]

On Mon, Feb 08, 2016 at 02:55:57PM +0000, Marc Zyngier wrote:
> On 08/02/16 09:16, Antoine Tenart wrote:
> > +
> > +Example:
> > +
> > +msix: msix {
> > +	compatible = "al,alpine-msix";
> > +	reg = <0x0 0xfbe00000 0x0 0x100000>;
> > +	interrupt-controller;
> > +	msi-controller;
> > +	al,msi-base-spi = <160>;
> > +	al,msi-num-spis = <160>;
> > +};
> > 
> 
> This example seems to rely on an implicit interrupt-parent. You probably
> want to update the example to clarify it.

Right. I'll add the interrupt-parent property in the required properties
and in the example msix node.

Antoine

-- 
Antoine T?nart, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160208/ff723db7/attachment.sig>

[PATCH 3/6] arm64: dts: alpine: add the MSIX node in the Alpine v2 dtsi

[Antoine Tenart]

Following the addition of the Alpine MSIX controller driver, add the
corresponding node in the Alpine v2 device tree.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
Signed-off-by: Tsahee Zidenberg <tsahee@annapurnalabs.com>
---
 arch/arm64/boot/dts/al/alpine-v2.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm64/boot/dts/al/alpine-v2.dtsi b/arch/arm64/boot/dts/al/alpine-v2.dtsi
index eb7a03b71456..413062be1674 100644
--- a/arch/arm64/boot/dts/al/alpine-v2.dtsi
+++ b/arch/arm64/boot/dts/al/alpine-v2.dtsi
@@ -141,6 +141,16 @@
 			/* 32 bit non prefetchable memory space */
 			ranges = <0x2000000 0x0 0xfe000000 0x0 0xfe000000 0x0 0x1000000>;
 			bus-range = <0x00 0x00>;
+			msi-parent = <&msix>;
+		};
+
+		msix: msix at fbe00000 {
+			compatible = "al,alpine-msix";
+			reg = <0x0 0xfbe00000 0x0 0x100000>;
+			interrupt-controller;
+			msi-controller;
+			al,msi-base-spi = <160>;
+			al,msi-num-spis = <160>;
 		};
 
 		uart0: uart at fd883000 {
-- 
2.7.0

[PATCH 4/6] ARM: dts: alpine: add the MSIX node

[Antoine Tenart]

With the newly available MSIX driver for Alpine, add the corresponding
node in the Alpine device tree.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
---
 arch/arm/boot/dts/alpine.dtsi | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/alpine.dtsi b/arch/arm/boot/dts/alpine.dtsi
index 9af2d60e9a7f..db8752fc480e 100644
--- a/arch/arm/boot/dts/alpine.dtsi
+++ b/arch/arm/boot/dts/alpine.dtsi
@@ -155,6 +155,16 @@
 			ranges = <0x02000000 0x0 0xfe000000 0x0 0xfe000000 0x0 0x1000000>;
 
 			bus-range = <0x00 0x00>;
+			msi-parent = <&msix>;
+		};
+
+		msix: msix at fbe00000 {
+			compatible = "al,alpine-msix";
+			reg = <0x0 0xfbe00000 0x0 0x100000>;
+			interrupt-controller;
+			msi-controller;
+			al,msi-base-spi = <96>;
+			al,msi-num-spis = <64>;
 		};
 	};
 };
-- 
2.7.0

[PATCH 5/6] arm64: alpine: select the Alpine MSI controller driver

[Antoine Tenart]

Select the Alpine MSI controller driver when using an Alpine platform.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
Signed-off-by: Tsahee Zidenberg <tsahee@annapurnalabs.com>
---
 arch/arm64/Kconfig.platforms | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/Kconfig.platforms b/arch/arm64/Kconfig.platforms
index bfbefa3e0dbe..6761e12e5e13 100644
--- a/arch/arm64/Kconfig.platforms
+++ b/arch/arm64/Kconfig.platforms
@@ -2,6 +2,7 @@ menu "Platform selection"
 
 config ARCH_ALPINE
 	bool "Annapurna Labs Alpine platform"
+	select ALPINE_MSI
 	help
 	  This enables support for the Annapurna Labs Alpine
 	  Soc family.
-- 
2.7.0

[PATCH 6/6] arm: alpine: select the Alpine MSI controller driver

[Antoine Tenart]

Select the Alpine MSI controller driver when using an Alpine platform.

Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
Signed-off-by: Tsahee Zidenberg <tsahee@annapurnalabs.com>
---
 arch/arm/mach-alpine/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/mach-alpine/Kconfig b/arch/arm/mach-alpine/Kconfig
index 5c2d54f59f53..b41838a58ae4 100644
--- a/arch/arm/mach-alpine/Kconfig
+++ b/arch/arm/mach-alpine/Kconfig
@@ -1,6 +1,7 @@
 config ARCH_ALPINE
 	bool "Annapurna Labs Alpine platform"
 	depends on ARCH_MULTI_V7
+	select ALPINE_MSI
 	select ARM_AMBA
 	select ARM_GIC
 	select GENERIC_IRQ_CHIP
-- 
2.7.0