RE: [PATCH 1/8] hw/intc: Add l2vic interrupt controller

[<ltaylorsimpson@gmail.com>]

> -----Original Message-----
> From: Brian Cain <brian.cain@oss.qualcomm.com>
> Sent: Saturday, March 1, 2025 11:21 AM
> To: qemu-devel@nongnu.org
> Cc: brian.cain@oss.qualcomm.com; richard.henderson@linaro.org;
> philmd@linaro.org; quic_mathbern@quicinc.com; ale@rev.ng; anjo@rev.ng;
> quic_mliebel@quicinc.com; ltaylorsimpson@gmail.com;
> alex.bennee@linaro.org; quic_mburton@quicinc.com;
> sidneym@quicinc.com; Damien Hedde <damien.hedde@dahe.fr>; Paolo
> Bonzini <pbonzini@redhat.com>
> Subject: [PATCH 1/8] hw/intc: Add l2vic interrupt controller
> 
> From: Sid Manning <sidneym@quicinc.com>
> 
> Co-authored-by: Matheus Tavares Bernardino
> <quic_mathbern@quicinc.com>
> Co-authored-by: Damien Hedde <damien.hedde@dahe.fr>
> Signed-off-by: Brian Cain <brian.cain@oss.qualcomm.com>
> ---
>  MAINTAINERS                    |   2 +
>  docs/devel/hexagon-l2vic.rst   |  59 +++++
>  docs/devel/index-internals.rst |   1 +
>  include/hw/intc/l2vic.h        |  37 +++
>  hw/intc/l2vic.c                | 417 +++++++++++++++++++++++++++++++++
>  hw/intc/Kconfig                |   3 +
>  hw/intc/meson.build            |   2 +
>  hw/intc/trace-events           |   4 +
>  8 files changed, 525 insertions(+)
>  create mode 100644 docs/devel/hexagon-l2vic.rst  create mode 100644
> include/hw/intc/l2vic.h  create mode 100644 hw/intc/l2vic.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 804c07bcd5..a842f7fe1b 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -232,6 +232,7 @@ Hexagon TCG CPUs
>  M: Brian Cain <brian.cain@oss.qualcomm.com>
>  S: Supported
>  F: target/hexagon/
> +F: hw/intc/l2vic.[ch]

Consider naming all the files outside target/hexagon as hex_* or hexagon_*
That will make it clear they belong to hexagon and you can use an easy wild card in the MAINTAINERS file.
Ditto for the docs files.

>  X: target/hexagon/idef-parser/
>  X: target/hexagon/gen_idef_parser_funcs.py
>  F: linux-user/hexagon/
> diff --git a/include/hw/intc/l2vic.h b/include/hw/intc/l2vic.h new file mode
> 100644 index 0000000000..ed8ccf33b1
> --- /dev/null
> +++ b/include/hw/intc/l2vic.h
> @@ -0,0 +1,37 @@
> +/*
> + * QEMU L2VIC Interrupt Controller
> + *
> + * Copyright(c) 2020-2025 Qualcomm Innovation Center, Inc. All Rights
> Reserved.
> + * SPDX-License-Identifier: GPL-2.0-or-later  */
> +
> +#define L2VIC_VID_GRP_0 0x0 /* Read */
> +#define L2VIC_VID_GRP_1 0x4 /* Read */
> +#define L2VIC_VID_GRP_2 0x8 /* Read */
> +#define L2VIC_VID_GRP_3 0xC /* Read */
> +#define L2VIC_VID_0 0x10 /* Read SOFTWARE DEFINED */ #define
> +L2VIC_VID_1 0x14 /* Read SOFTWARE DEFINED NOT YET USED */ #define
> +L2VIC_INT_ENABLEn 0x100 /* Read/Write */ #define
> +L2VIC_INT_ENABLE_CLEARn 0x180 /* Write */ #define
> L2VIC_INT_ENABLE_SETn
> +0x200 /* Write */ #define L2VIC_INT_TYPEn 0x280 /* Read/Write */
> +#define L2VIC_INT_STATUSn 0x380 /* Read */ #define L2VIC_INT_CLEARn
> +0x400 /* Write */ #define L2VIC_SOFT_INTn 0x480 /* Write */ #define
> +L2VIC_INT_PENDINGn 0x500 /* Read */ #define L2VIC_INT_GRPn_0 0x600
> /*
> +Read/Write */ #define L2VIC_INT_GRPn_1 0x680 /* Read/Write */ #define
> +L2VIC_INT_GRPn_2 0x700 /* Read/Write */ #define L2VIC_INT_GRPn_3
> 0x780
> +/* Read/Write */
> +
> +#define L2VIC_INTERRUPT_MAX 1024
> +#define L2VIC_CIAD_INSTRUCTION -1
> +/*
> + * Note about l2vic groups:
> + * Each interrupt to L2VIC can be configured to associate with one of
> + * four groups.
> + * Group 0 interrupts go to IRQ2 via VID 0 (SSR: 0xC2, the default)
> + * Group 1 interrupts go to IRQ3 via VID 1 (SSR: 0xC3)
> + * Group 2 interrupts go to IRQ4 via VID 2 (SSR: 0xC4)
> + * Group 3 interrupts go to IRQ5 via VID 3 (SSR: 0xC5)  */
> diff --git a/hw/intc/l2vic.c b/hw/intc/l2vic.c new file mode 100644 index
> 0000000000..9df6575214
> --- /dev/null
> +++ b/hw/intc/l2vic.c
> @@ -0,0 +1,417 @@
> +/*
> + * QEMU L2VIC Interrupt Controller
> + *
> + * Arm PrimeCell PL190 Vector Interrupt Controller was used as a reference.
> + * Copyright(c) 2020-2025 Qualcomm Innovation Center, Inc. All Rights
> Reserved.
> + * SPDX-License-Identifier: GPL-2.0-or-later  */
> +
> +#include "qemu/osdep.h"
> +#include "hw/irq.h"
> +#include "hw/sysbus.h"
> +#include "migration/vmstate.h"
> +#include "qemu/log.h"
> +#include "qemu/module.h"
> +#include "hw/intc/l2vic.h"
> +#include "trace.h"
> +
> +#define L2VICA(s, n) (s[(n) >> 2])
> +
> +#define TYPE_L2VIC "l2vic"
> +#define L2VIC(obj) OBJECT_CHECK(L2VICState, (obj), TYPE_L2VIC)
> +
> +#define SLICE_MAX (L2VIC_INTERRUPT_MAX / 32)
> +
> +typedef struct L2VICState {
> +    SysBusDevice parent_obj;
> +
> +    QemuMutex active;
> +    MemoryRegion iomem;
> +    MemoryRegion fast_iomem;
> +    uint32_t level;
> +    /*
> +     * offset 0:vid group 0 etc, 10 bits in each group
> +     * are used:
> +     */
> +    uint32_t vid_group[4];
> +    uint32_t vid0;
> +    /* Clear Status of Active Edge interrupt, not used: */
> +    uint32_t int_clear[SLICE_MAX] QEMU_ALIGNED(16);
> +    /* Enable interrupt source */
> +    uint32_t int_enable[SLICE_MAX] QEMU_ALIGNED(16);
> +    /* Clear (set to 0) corresponding bit in int_enable */
> +    uint32_t int_enable_clear;
> +    /* Set (to 1) corresponding bit in int_enable */
> +    uint32_t int_enable_set;
> +    /* Present for debugging, not used */
> +    uint32_t int_pending[SLICE_MAX] QEMU_ALIGNED(16);

Consider using DECLARE_BITMAP32 since you use test_bit/set_bit/clear_bit.
Are these alignments needed?

> +    /* Generate an interrupt */
> +    uint32_t int_soft;
> +    /* Which enabled interrupt is active */
> +    uint32_t int_status[SLICE_MAX] QEMU_ALIGNED(16);
> +    /* Edge or Level interrupt */
> +    uint32_t int_type[SLICE_MAX] QEMU_ALIGNED(16);
> +    /* L2 interrupt group 0-3 0x600-0x7FF */
> +    uint32_t int_group_n0[SLICE_MAX] QEMU_ALIGNED(16);
> +    uint32_t int_group_n1[SLICE_MAX] QEMU_ALIGNED(16);
> +    uint32_t int_group_n2[SLICE_MAX] QEMU_ALIGNED(16);
> +    uint32_t int_group_n3[SLICE_MAX] QEMU_ALIGNED(16);
> +    qemu_irq irq[8];
> +} L2VICState;
> +



> +
> +static void l2vic_set_irq(void *opaque, int irq, int level) {
> +    L2VICState *s = (L2VICState *)opaque;
> +    if (level) {
> +        qemu_mutex_lock(&s->active);
> +        set_bit(irq, (unsigned long *)s->int_pending);

Here's an example of the set_bit mentioned above.

> +        qemu_mutex_unlock(&s->active);
> +    }
> +    l2vic_update(s, irq);
> +}
> +
> +static void l2vic_reset_hold(Object *obj, G_GNUC_UNUSED ResetType
> +res_type) {
> +    L2VICState *s = L2VIC(obj);
> +    memset(s->int_clear, 0, sizeof(s->int_clear));
> +    memset(s->int_enable, 0, sizeof(s->int_enable));
> +    memset(s->int_pending, 0, sizeof(s->int_pending));
> +    memset(s->int_status, 0, sizeof(s->int_status));
> +    memset(s->int_type, 0, sizeof(s->int_type));
> +    memset(s->int_group_n0, 0, sizeof(s->int_group_n0));
> +    memset(s->int_group_n1, 0, sizeof(s->int_group_n1));
> +    memset(s->int_group_n2, 0, sizeof(s->int_group_n2));
> +    memset(s->int_group_n3, 0, sizeof(s->int_group_n3));
> +    s->int_soft = 0;
> +    s->vid0 = 0;

How about a single memset(s, 0, sizeof(L2VICState))?

> +
> +    l2vic_update_all(s);
> +}