Re: [Qemu-arm] [PATCH v4 5/8] bcm2836_control: add bcm2836 ARM control logic

[Peter Crosthwaite]

On Fri, Jan 15, 2016 at 3:58 PM, Andrew Baumann
<address@hidden> wrote:
> This module is specific to the bcm2836 (Pi2). It implements the top
> level interrupt controller, and mailboxes used for inter-processor
> synchronisation.
>
> Signed-off-by: Andrew Baumann <address@hidden>
> ---
>
> Notes:
>     v4:
>     * delete unused defs
>     * s/localirqs/timerirqs/
>     * factor out deliver_local() from bcm2836_control_update
>     * use deposit32 in place of bit manipulation in set_local_irq
>     * introduced register offset defs, and reduced comments in read/write 
> handlers
>     * delete commented code
>     * s/_/-/ rename GPIOs
>
>     v3:
>      * uint8 localirqs
>      * style tweaks
>      * add MR access size limits
>
>  hw/intc/Makefile.objs             |   2 +-
>  hw/intc/bcm2836_control.c         | 303 
> ++++++++++++++++++++++++++++++++++++++
>  include/hw/intc/bcm2836_control.h |  51 +++++++
>  3 files changed, 355 insertions(+), 1 deletion(-)
>  create mode 100644 hw/intc/bcm2836_control.c
>  create mode 100644 include/hw/intc/bcm2836_control.h
>
> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> index 2ad1204..6a13a39 100644
> --- a/hw/intc/Makefile.objs
> +++ b/hw/intc/Makefile.objs
> @@ -24,7 +24,7 @@ obj-$(CONFIG_GRLIB) += grlib_irqmp.o
>  obj-$(CONFIG_IOAPIC) += ioapic.o
>  obj-$(CONFIG_OMAP) += omap_intc.o
>  obj-$(CONFIG_OPENPIC_KVM) += openpic_kvm.o
> -obj-$(CONFIG_RASPI) += bcm2835_ic.o
> +obj-$(CONFIG_RASPI) += bcm2835_ic.o bcm2836_control.o
>  obj-$(CONFIG_SH4) += sh_intc.o
>  obj-$(CONFIG_XICS) += xics.o
>  obj-$(CONFIG_XICS_KVM) += xics_kvm.o
> diff --git a/hw/intc/bcm2836_control.c b/hw/intc/bcm2836_control.c
> new file mode 100644
> index 0000000..f0b7b0a
> --- /dev/null
> +++ b/hw/intc/bcm2836_control.c
> @@ -0,0 +1,303 @@
> +/*
> + * Rasperry Pi 2 emulation ARM control logic module.
> + * Copyright (c) 2015, Microsoft
> + * Written by Andrew Baumann
> + *
> + * Based on bcm2835_ic.c (Raspberry Pi emulation) (c) 2012 Gregory Estrade
> + * This code is licensed under the GNU GPLv2 and later.
> + *
> + * At present, only implements interrupt routing, and mailboxes (i.e.,
> + * not local timer, PMU interrupt, or AXI counters).
> + *
> + * Ref:
> + * 
> https://www.raspberrypi.org/documentation/hardware/raspberrypi/bcm2836/QA7_rev3.4.pdf
> + */
> +
> +#include "hw/intc/bcm2836_control.h"
> +
> +#define REG_GPU_ROUTE           0x0c
> +#define REG_TIMERCONTROL        0x40
> +#define REG_MBOXCONTROL         0x50
> +#define REG_IRQSRC              0x60
> +#define REG_FIQSRC              0x70
> +#define REG_MBOX0_WR            0x80
> +#define REG_MBOX0_RDCLR         0xc0
> +#define REG_LIMIT              0x100
> +
> +#define IRQ_BIT(cntrl, num) (((cntrl) & (1 << (num))) != 0)
> +#define FIQ_BIT(cntrl, num) (((cntrl) & (1 << ((num) + 4))) != 0)
> +
> +#define IRQ_CNTPSIRQ    0
> +#define IRQ_CNTPNSIRQ   1
> +#define IRQ_CNTHPIRQ    2
> +#define IRQ_CNTVIRQ     3
> +#define IRQ_MAILBOX0    4
> +#define IRQ_MAILBOX1    5
> +#define IRQ_MAILBOX2    6
> +#define IRQ_MAILBOX3    7
> +#define IRQ_GPU         8
> +#define IRQ_PMU         9
> +#define IRQ_AXI         10
> +#define IRQ_TIMER       11
> +#define IRQ_MAX         IRQ_TIMER
> +
> +static void deliver_local(BCM2836ControlState *s, uint8_t core, uint8_t irq,
> +                          uint32_t controlreg, uint8_t controlidx)
> +{
> +    if (FIQ_BIT(controlreg, controlidx)) {
> +        /* deliver a FIQ */
> +        s->fiqsrc[core] |= (uint32_t)1 << irq;
> +    } else if (IRQ_BIT(controlreg, controlidx)) {
> +        /* deliver an IRQ */
> +        s->irqsrc[core] |= (uint32_t)1 << irq;
> +    } else {
> +        /* the interrupt is masked */
> +    }
> +}
> +
> +/* Update interrupts.  */
> +static void bcm2836_control_update(BCM2836ControlState *s)
> +{
> +    int i, j;
> +
> +    /* reset pending IRQs/FIQs */
> +    for (i = 0; i < BCM2836_NCORES; i++) {
> +        s->irqsrc[i] = s->fiqsrc[i] = 0;
> +    }
> +
> +    /* apply routing logic, update status regs */
> +    if (s->gpu_irq) {
> +        assert(s->route_gpu_irq < BCM2836_NCORES);
> +        s->irqsrc[s->route_gpu_irq] |= (uint32_t)1 << IRQ_GPU;
> +    }
> +
> +    if (s->gpu_fiq) {
> +        assert(s->route_gpu_fiq < BCM2836_NCORES);
> +        s->fiqsrc[s->route_gpu_fiq] |= (uint32_t)1 << IRQ_GPU;
> +    }
> +
> +    for (i = 0; i < BCM2836_NCORES; i++) {
> +        /* handle local timer interrupts for this core */
> +        if (s->timerirqs[i]) {
> +            assert(s->timerirqs[i] < (1 << IRQ_MAILBOX0)); /* sanity check */
> +            for (j = 0; j < IRQ_MAILBOX0; j++) {

I think <= IRQ_CNTVIRQ is cleaner, as it keeps "MAILBOX" out of the timer code.

> +                if ((s->timerirqs[i] & (1 << j)) != 0) {
> +                    /* local interrupt j is set */
> +                    deliver_local(s, i, j, s->timercontrol[i], j);
> +                }
> +            }
> +        }
> +
> +        /* handle mailboxes for this core */
> +        for (j = 0; j < BCM2836_MBPERCORE; j++) {
> +            if (s->mailboxes[i * BCM2836_MBPERCORE + j] != 0) {
> +                /* mailbox j is set */
> +                deliver_local(s, i, j + IRQ_MAILBOX0, s->mailboxcontrol[i], 
> j);
> +            }
> +        }
> +    }
> +
> +    /* call set_irq appropriately for each output */
> +    for (i = 0; i < BCM2836_NCORES; i++) {
> +        qemu_set_irq(s->irq[i], s->irqsrc[i] != 0);
> +        qemu_set_irq(s->fiq[i], s->fiqsrc[i] != 0);
> +    }
> +}
> +
> +static void bcm2836_control_set_local_irq(void *opaque, int core, int 
> local_irq,
> +                                          int level)
> +{
> +    BCM2836ControlState *s = opaque;
> +
> +    assert(core >= 0 && core < BCM2836_NCORES);
> +    assert(local_irq >= 0 && local_irq <= IRQ_CNTVIRQ);

Continuing above, this is inconsistent with the above where you use <
IRQ_MAILBOX0 instead for the same check.

> +
> +    s->timerirqs[core] = deposit32(s->timerirqs[core], local_irq, 1, 
> !!level);
> +
> +    bcm2836_control_update(s);
> +}
> +
> +/* XXX: the following wrapper functions are a kludgy workaround,
> + * needed because I can't seem to pass useful information in the "irq"
> + * parameter when using named interrupts. Feel free to clean this up!
> + */
> +
> +static void bcm2836_control_set_local_irq0(void *opaque, int core, int level)
> +{
> +    bcm2836_control_set_local_irq(opaque, core, 0, level);
> +}
> +
> +static void bcm2836_control_set_local_irq1(void *opaque, int core, int level)
> +{
> +    bcm2836_control_set_local_irq(opaque, core, 1, level);
> +}
> +
> +static void bcm2836_control_set_local_irq2(void *opaque, int core, int level)
> +{
> +    bcm2836_control_set_local_irq(opaque, core, 2, level);
> +}
> +
> +static void bcm2836_control_set_local_irq3(void *opaque, int core, int level)
> +{
> +    bcm2836_control_set_local_irq(opaque, core, 3, level);
> +}
> +

> +    OBJECT_CHECK(BCM2836ControlState, (obj), TYPE_BCM2836_CONTROL)
> +
> +typedef struct BCM2836ControlState {
> +    /*< private >*/
> +    SysBusDevice busdev;
> +    /*< public >*/
> +    MemoryRegion iomem;
> +

> +    /* interrupt status registers (not directly visible to user) */
> +    bool gpu_irq, gpu_fiq;
> +    uint8_t timerirqs[BCM2836_NCORES];
> +

This ...

> +    /* mailboxes */
> +    uint32_t mailboxes[BCM2836_NCORES * BCM2836_MBPERCORE];
> +
> +    /* interrupt routing/control registers */
> +    uint8_t route_gpu_irq, route_gpu_fiq;
> +    uint32_t timercontrol[BCM2836_NCORES];
> +    uint32_t mailboxcontrol[BCM2836_NCORES];
> +

> +    /* interrupt source registers, post-routing (visible) */
> +    uint32_t irqsrc[BCM2836_NCORES];
> +    uint32_t fiqsrc[BCM2836_NCORES];
> +

And this are absent from the VMSD, but after some thought they don't
need to be as they are pure functions of the input pin state that is
always refreshable from other state no? I would these together with a
brief comment as to the above, and keep the migratable state (genuine
device state) all together.

Otherwise,

Reviewed-by: Peter Crosthwaite <address@hidden>

Regards,
Peter

> +    /* outputs to CPU cores */
> +    qemu_irq irq[BCM2836_NCORES];
> +    qemu_irq fiq[BCM2836_NCORES];
> +} BCM2836ControlState;
> +
> +#endif
> --
> 2.5.3
>