Message-ID: <56B86391.1030609@arm.com>
Date:	Mon, 8 Feb 2016 09:44:49 +0000
From:	Marc Zyngier <marc.zyngier@....com>
To:	Antoine Tenart <antoine.tenart@...e-electrons.com>,
	tglx@...utronix.de, jason@...edaemon.net, tsahee@...apurnalabs.com
Cc:	rshitrit@...apurnalabs.com, thomas.petazzoni@...e-electrons.com,
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/6] irqchip: add the Alpine MSIX interrupt controller

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@...e-electrons.com>
> Signed-off-by: Tsahee Zidenberg <tsahee@...apurnalabs.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@...e-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...

Powered by blists - more mailing lists