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

[Brian Cain <brian.cain@oss.qualcomm.com>]

On 3/3/2025 6:26 AM, Philippe Mathieu-Daudé wrote:
> Hi Brian and Sid,
>
> On 1/3/25 18:20, Brian Cain wrote:
>> 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/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)
>
> Why not use OBJECT_DECLARE_SIMPLE_TYPE()?
>

Great suggestion, ty.  Will do.


>> +
>> +#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);
>> +    /* 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;
>
> OBJECT_DECLARE_SIMPLE_TYPE(L2VICState, L2VIC)
>
>
>> +static inline bool vid_active(L2VICState *s)
>> +
>> +{
>> +    /* scan all 1024 bits in int_status arrary */
>> +    const int size = sizeof(s->int_status) * CHAR_BIT;
>> +    const int active_irq = find_first_bit((unsigned long 
>> *)s->int_status, size);
>
> Maybe this file could leverage the 32-bit bitops.h API:
>
> $ git grep bit32\( include/qemu/bitops.h
> include/qemu/bitops.h:38: * - Bits stored in an array of 'uint32_t': 
> set_bit32(), clear_bit32(), etc
> include/qemu/bitops.h:270:static inline void set_bit32(long nr, 
> uint32_t *addr)
> include/qemu/bitops.h:296:static inline void clear_bit32(long nr, 
> uint32_t *addr)
> include/qemu/bitops.h:322:static inline void change_bit32(long nr, 
> uint32_t *addr)
> include/qemu/bitops.h:335:static inline int test_and_set_bit32(long 
> nr, uint32_t *addr)
> include/qemu/bitops.h:350:static inline int test_and_clear_bit32(long 
> nr, uint32_t *addr)
> include/qemu/bitops.h:365:static inline int test_and_change_bit32(long 
> nr, uint32_t *addr)
> include/qemu/bitops.h:380:static inline int test_bit32(long nr, const 
> uint32_t *addr)
>

Okay - I will make this change.


There's several places in the code today where we do this L2VICA() macro 
to assign all of the bits to/from a single uint32_t word.  And if we 
take your and Taylor's suggestion to use the DECLARE_BITMAP32, we can 
probably change these to use bitmap_copy_with_{src,dst}_offset() 
instead, I think?


>> +    return ((active_irq != size)) ? true : false;
>> +}
>> +
>> +static bool l2vic_update(L2VICState *s, int irq)
>> +{
>> +    if (vid_active(s)) {
>> +        return true;
>> +    }
>> +
>> +    bool pending = test_bit(irq, (unsigned long *)s->int_pending);
>> +    bool enable = test_bit(irq, (unsigned long *)s->int_enable);
>> +    if (pending && enable) {
>> +        int vid = get_vid(s, irq);
>> +        set_bit(irq, (unsigned long *)s->int_status);
>> +        clear_bit(irq, (unsigned long *)s->int_pending);
>> +        clear_bit(irq, (unsigned long *)s->int_enable);
>> +        /* ensure the irq line goes low after going high */
>> +        s->vid0 = irq;
>> +        s->vid_group[get_vid(s, irq)] = irq;
>> +
>> +        /* already low: now call pulse */
>> +        /*     pulse: calls qemu_upper() and then qemu_lower()) */
>> +        qemu_irq_pulse(s->irq[vid + 2]);
>> +        trace_l2vic_delivered(irq, vid);
>> +        return true;
>> +    }
>> +    return false;
>> +}
>