Re: [PATCH] irqchip: irq-mvebu-odmi: new driver

[Marc Zyngier]

Hi Thomas,

On 15/02/16 07:42, Thomas Petazzoni wrote:
> This commits adds a new irqchip driver that handles the ODMI
> controller found on Marvell 7K/8K processors. The ODMI controller
> provide MSI interrupt functionality to on-board peripherals, much like
> the GIC-v2m.

May I suggest a title that says a bit more than just "new driver"?
Something a bit more descriptive like "Platform MSI support"?

> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@xxxxxxxxxxxxxxxxxx>
> ---
>  .../marvell,odmi-controller.txt                    |  36 +++
>  drivers/irqchip/Kconfig                            |   4 +
>  drivers/irqchip/Makefile                           |   1 +
>  drivers/irqchip/irq-mvebu-odmi.c                   | 270 +++++++++++++++++++++
>  4 files changed, 311 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/marvell,odmi-controller.txt
>  create mode 100644 drivers/irqchip/irq-mvebu-odmi.c
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/marvell,odmi-controller.txt b/Documentation/devicetree/bindings/interrupt-controller/marvell,odmi-controller.txt
> new file mode 100644
> index 0000000..a2470af
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/marvell,odmi-controller.txt
> @@ -0,0 +1,36 @@
> +
> +* Marvell ODMI for MSI support
> +
> +Some Marvell SoCs have an On-Die Message Interrupt (ODMI) controller
> +which can be used by on-board peripheral for MSI interrupts.
> +
> +Required properties:
> +
> +- compatible	       : The value here should contain "marvell,odmi-controller".
> +
> +- interrupt,controller : Identifies the node as an interrupt controller.
> +
> +- msi-controller       : Identifies the node as an MSI controller.
> +
> +- marvell,odmi-frames  : Number of ODMI frames available. Each frame
> +                         provides a number of events.
> +
> +- reg		       : List of register definitions, one for each
> +                         ODMI frame.
> +
> +- marvell,spi-base     : List of GIC base SPI interrupts, one for each
> +                         ODMI frame.

Please add a pointer to the GIC documentation, so that it is explicit
what SPIs are. Also, is the SPI number 0 based? or 32 based? Looking at
the code, it looks to be 32-based.

Please document the need for an "interrupt-parent" property, which may
be implicit in a DT, but fundamental to the understanding of the system.

> +
> +Example:
> +
> +	odmi: odmi@300000 {
> +		compatible = "marvell,odmi-controller";
> +		interrupt-controller;
> +		msi-controller;
> +		marvell,odmi-frames = <4>;
> +		reg = <0x300000 0x4000>,
> +		      <0x304000 0x4000>,
> +		      <0x308000 0x4000>,
> +		      <0x30C000 0x4000>;
> +		marvell,spi-base = <128>, <136>, <144>, <152>;
> +	};
> diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
> index 715923d..18bed3c 100644
> --- a/drivers/irqchip/Kconfig
> +++ b/drivers/irqchip/Kconfig
> @@ -217,3 +217,7 @@ config IRQ_MXS
>  	def_bool y if MACH_ASM9260 || ARCH_MXS
>  	select IRQ_DOMAIN
>  	select STMP_DEVICE
> +
> +config MVEBU_ODMI
> +	bool
> +	select GENERIC_MSI_IRQ_DOMAIN
> diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
> index 18caacb..29c388f 100644
> --- a/drivers/irqchip/Makefile
> +++ b/drivers/irqchip/Makefile
> @@ -59,3 +59,4 @@ obj-$(CONFIG_ARCH_SA1100)		+= irq-sa11x0.o
>  obj-$(CONFIG_INGENIC_IRQ)		+= irq-ingenic.o
>  obj-$(CONFIG_IMX_GPCV2)			+= irq-imx-gpcv2.o
>  obj-$(CONFIG_PIC32_EVIC)		+= irq-pic32-evic.o
> +obj-$(CONFIG_MVEBU_ODMI)		+= irq-mvebu-odmi.o
> diff --git a/drivers/irqchip/irq-mvebu-odmi.c b/drivers/irqchip/irq-mvebu-odmi.c
> new file mode 100644
> index 0000000..c3e00b9
> --- /dev/null
> +++ b/drivers/irqchip/irq-mvebu-odmi.c
> @@ -0,0 +1,270 @@
> +/*
> + * Copyright (C) 2016 Marvell
> + *
> + * Thomas Petazzoni <thomas.petazzoni@xxxxxxxxxxxxxxxxxx>
> + *
> + * 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) "GIC-ODMI: " fmt
> +
> +#include <linux/irq.h>
> +#include <linux/irqchip.h>
> +#include <linux/irqdomain.h>
> +#include <linux/kernel.h>
> +#include <linux/msi.h>
> +#include <linux/of_address.h>
> +#include <linux/slab.h>
> +#include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +#define GICP_ODMIN_SET			0x40
> +#define   GICP_ODMI_INT_NUM_SHIFT	12
> +#define GICP_ODMIN_GM_EP_R0		0x110
> +#define GICP_ODMIN_GM_EP_R1		0x114
> +#define GICP_ODMIN_GM_EA_R0		0x108
> +#define GICP_ODMIN_GM_EA_R1		0x118
> +
> +/*
> + * We don't support the group events, so we simply have 8 interrupts
> + * per frame.
> + */
> +#define NODMIS_PER_FRAME 8
> +
> +struct odmi_data {
> +	struct resource res;
> +	void __iomem *base;
> +	unsigned int spi_base;
> +	unsigned long bm;

So you keep a bitmap per frame? That seems a bit odd - see below.

> +};
> +
> +static struct odmi_data *odmis;
> +static unsigned int odmis_count;
> +
> +/*
> + * Lock protecting the allocation bitmap embedded in the odmi_data
> + * structure. Only one spinlock is used, since allocation/freeing of
> + * MSI interrupts is infrequent.
> + */
> +static DEFINE_SPINLOCK(odmis_lock);
> +
> +static int odmi_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);
> +	if (ret == IRQ_SET_MASK_OK)
> +		ret = IRQ_SET_MASK_OK_DONE;

I'm planning to fix GICv2 so that we don't need that anymore (much like
Antoine did got GICv3 in his Alpine series).

> +
> +	return ret;
> +}
> +
> +static void odmi_compose_msi_msg(struct irq_data *data, struct msi_msg *msg)
> +{
> +	struct odmi_data *odmi = NULL;
> +	phys_addr_t addr;
> +	unsigned int odmi_offset, i;
> +
> +	/* Search the ODMI frame handling this interrupt */
> +	for (i = 0; i < odmis_count; i++) {
> +		if (data->hwirq >= odmis[i].spi_base &&
> +		    data->hwirq < (odmis[i].spi_base + NODMIS_PER_FRAME)) {
> +			odmi = &odmis[i];
> +			break;
> +		}
> +	}

So assume that instead of a bitmap per frame, you have a global one, and
hwirq is just the bit number in the bitmap. You have 8 interrupts per
frame, so you can compute things like this:

odmi = &odmi[data->hwirq >> 3];
odmi_offset = odmi->spi_base + (data->hwirq & 3);

Job done.

> +
> +	if (WARN_ON(!odmi))
> +		return;
> +
> +	odmi_offset = (data->hwirq - odmi->spi_base);
> +
> +	addr = odmi->res.start + GICP_ODMIN_SET;
> +
> +	msg->address_hi = upper_32_bits(addr);
> +	msg->address_lo = lower_32_bits(addr);
> +	msg->data = odmi_offset << GICP_ODMI_INT_NUM_SHIFT;
> +}
> +
> +static struct irq_chip odmi_irq_chip = {
> +	.name			= "ODMI",
> +	.irq_mask		= irq_chip_mask_parent,
> +	.irq_unmask		= irq_chip_unmask_parent,
> +	.irq_eoi		= irq_chip_eoi_parent,
> +	.irq_set_affinity	= odmi_set_affinity,
> +	.irq_compose_msi_msg	= odmi_compose_msi_msg,
> +};
> +
> +static int odmi_irq_domain_alloc(struct irq_domain *domain, unsigned int virq,
> +				 unsigned int nr_irqs, void *args)
> +{
> +	struct odmi_data *odmi = NULL;
> +	struct irq_fwspec fwspec;
> +	struct irq_data *d;
> +	unsigned int offset, hwirq;
> +	int i, ret;
> +
> +	spin_lock(&odmis_lock);
> +	for (i = 0; i < odmis_count; i++) {
> +		offset = find_first_zero_bit(&odmis[i].bm, NODMIS_PER_FRAME);
> +		if (offset < NODMIS_PER_FRAME) {
> +			__set_bit(offset, &odmis[i].bm);
> +			odmi = &odmis[i];
> +			break;
> +		}
> +	}
> +	spin_unlock(&odmis_lock);

And this becomes much simpler.

> +
> +	if (!odmi)
> +		return -ENOSPC;
> +
> +	hwirq = odmi->spi_base + offset;
> +
> +	fwspec.fwnode = domain->parent->fwnode;
> +	fwspec.param_count = 3;
> +	fwspec.param[0] = GIC_SPI;
> +	fwspec.param[1] = hwirq - 32;
> +	fwspec.param[2] = IRQ_TYPE_EDGE_RISING;
> +
> +	ret = irq_domain_alloc_irqs_parent(domain, virq, 1, &fwspec);
> +	if (ret) {
> +		pr_err("Cannot allocate parent IRQ\n");
> +		spin_lock(&odmis_lock);
> +		__clear_bit(offset, &odmi->bm);
> +		spin_unlock(&odmis_lock);
> +		return ret;
> +	}
> +
> +	/* Configure the interrupt line to be edge */
> +	d = irq_domain_get_irq_data(domain->parent, virq);
> +	d->chip->irq_set_type(d, IRQ_TYPE_EDGE_RISING);
> +
> +	irq_domain_set_hwirq_and_chip(domain, virq, hwirq,
> +				      &odmi_irq_chip, NULL);
> +
> +	return 0;
> +}
> +
> +static void odmi_irq_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 odmi_data *odmi = NULL;
> +	unsigned int offset;
> +	int i;
> +
> +	/*
> +	 * Check if the hwirq number is valid, and if so, to which
> +	 * ODMI frame it belongs
> +	 */
> +	spin_lock(&odmis_lock);
> +	for (i = 0; i < odmis_count; i++) {
> +		if (d->hwirq >= odmis[i].spi_base &&
> +		    d->hwirq < (odmis[i].spi_base + NODMIS_PER_FRAME)) {
> +			odmi = &odmis[i];
> +			offset = d->hwirq - odmis[i].spi_base;
> +			break;
> +		}
> +	}
> +	spin_unlock(&odmis_lock);

And that too.

> +
> +	if (!odmi) {
> +		pr_err("Failed to teardown msi. Invalid hwirq %lu\n", d->hwirq);
> +		return;
> +	}
> +
> +	irq_domain_free_irqs_parent(domain, virq, nr_irqs);
> +
> +	/* Actually free the MSI */
> +	spin_lock(&odmis_lock);
> +	__clear_bit(offset, &odmi->bm);
> +	spin_unlock(&odmis_lock);
> +}
> +
> +static const struct irq_domain_ops odmi_domain_ops = {
> +	.alloc	= odmi_irq_domain_alloc,
> +	.free	= odmi_irq_domain_free,
> +};
> +
> +static struct irq_chip odmi_msi_irq_chip = {
> +	.name	= "ODMI",
> +};
> +
> +static struct msi_domain_ops odmi_msi_ops = {
> +};
> +
> +static struct msi_domain_info odmi_msi_domain_info = {
> +	.flags	= (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS),
> +	.ops	= &odmi_msi_ops,
> +	.chip	= &odmi_msi_irq_chip,
> +};
> +
> +static int __init mvebu_odmi_init(struct device_node *node,
> +				  struct device_node *parent)
> +{
> +	struct irq_domain *inner_domain, *plat_domain;
> +	int ret, i;
> +
> +	if (of_property_read_u32(node, "marvell,odmi-frames", &odmis_count))
> +		return -EINVAL;
> +
> +	odmis = kcalloc(odmis_count, sizeof(struct odmi_data), GFP_KERNEL);
> +	if (!odmis)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < odmis_count; i++) {
> +		struct odmi_data *odmi = &odmis[i];
> +
> +		ret = of_address_to_resource(node, i, &odmi->res);
> +		if (ret)
> +			goto err_unmap;
> +
> +		odmi->base = of_io_request_and_map(node, i, "odmi");
> +		if (IS_ERR(odmi->base)) {
> +			ret = PTR_ERR(odmi->base);
> +			goto err_unmap;
> +		}
> +
> +		if (of_property_read_u32_index(node, "marvell,spi-base",
> +					       i, &odmi->spi_base)) {
> +			ret = -EINVAL;
> +			goto err_unmap;
> +		}
> +	}
> +
> +	inner_domain = irq_domain_create_linear(of_node_to_fwnode(node),
> +						odmis_count * NODMIS_PER_FRAME,
> +						&odmi_domain_ops, NULL);
> +	if (!inner_domain) {
> +		ret = -ENOMEM;
> +		goto err_unmap;
> +	}
> +
> +	inner_domain->parent = irq_find_host(parent);
> +
> +	plat_domain = platform_msi_create_irq_domain(of_node_to_fwnode(node),
> +						     &odmi_msi_domain_info,
> +						     inner_domain);
> +	if (!plat_domain) {
> +		ret = -ENOMEM;
> +		goto err_remove_inner;
> +	}
> +
> +	return 0;
> +
> +err_remove_inner:
> +	irq_domain_remove(inner_domain);
> +err_unmap:
> +	for (i = 0; i < odmis_count; i++) {
> +		struct odmi_data *odmi = &odmis[i];
> +
> +		if (odmi->base && !IS_ERR(odmi->base))
> +			iounmap(odmis[i].base);
> +	}
> +	kfree(odmis);
> +	return ret;
> +}
> +
> +IRQCHIP_DECLARE(mvebu_odmi, "marvell,odmi-controller", mvebu_odmi_init);
> 

It otherwise looks pretty good to me.

Thanks,

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