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.petazz...@free-electrons.com>
> ---
>  .../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.petazz...@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) "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...

Reply via email to