Re: [Qemu-devel] [fixed-up][PATCH v5 3/5] hw/intc: add sunxi interrupt controller device

[Peter Crosthwaite]

On Wed, Nov 27, 2013 at 4:12 PM, liguang <address@hidden> wrote:
> Signed-off-by: liguang <address@hidden>
> ---
>  default-configs/arm-softmmu.mak |    1 +
>  hw/intc/Makefile.objs           |    1 +
>  hw/intc/sunxi-pic.c             |  247 
> +++++++++++++++++++++++++++++++++++++++
>  include/hw/intc/sunxi-pic.h     |   20 +++
>  4 files changed, 269 insertions(+), 0 deletions(-)
>  create mode 100644 hw/intc/sunxi-pic.c
>  create mode 100644 include/hw/intc/sunxi-pic.h
>
> diff --git a/default-configs/arm-softmmu.mak b/default-configs/arm-softmmu.mak
> index 7bf5ad0..bbe00e4 100644
> --- a/default-configs/arm-softmmu.mak
> +++ b/default-configs/arm-softmmu.mak
> @@ -83,3 +83,4 @@ CONFIG_SDHCI=y
>  CONFIG_INTEGRATOR_DEBUG=y
>
>  CONFIG_SUNXI_PIT=y
> +CONFIG_SUNXI_PIC=y
> diff --git a/hw/intc/Makefile.objs b/hw/intc/Makefile.objs
> index 47ac442..dad8c43 100644
> --- a/hw/intc/Makefile.objs
> +++ b/hw/intc/Makefile.objs
> @@ -12,6 +12,7 @@ common-obj-$(CONFIG_IOAPIC) += ioapic_common.o
>  common-obj-$(CONFIG_ARM_GIC) += arm_gic_common.o
>  common-obj-$(CONFIG_ARM_GIC) += arm_gic.o
>  common-obj-$(CONFIG_OPENPIC) += openpic.o
> +common-obj-$(CONFIG_SUNXI_PIC) += sunxi-pic.o
>
>  obj-$(CONFIG_APIC) += apic.o apic_common.o
>  obj-$(CONFIG_ARM_GIC_KVM) += arm_gic_kvm.o
> diff --git a/hw/intc/sunxi-pic.c b/hw/intc/sunxi-pic.c
> new file mode 100644
> index 0000000..a588c30
> --- /dev/null
> +++ b/hw/intc/sunxi-pic.c
> @@ -0,0 +1,247 @@
> +/*
> + * Allwinner sunxi interrupt controller device emulation
> + *
> + * Copyright (C) 2013 Li Guang
> + * Written by Li Guang <address@hidden>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but 
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +
> +#include "hw/sysbus.h"
> +#include "hw/devices.h"
> +#include "sysemu/sysemu.h"
> +#include "hw/intc/sunxi-pic.h"
> +
> +
> +typedef struct SunxiPICState {
> +    /*< private >*/
> +    SysBusDevice parent_obj;
> +    /*< public >*/
> +    MemoryRegion iomem;
> +    qemu_irq parent_fiq;
> +    qemu_irq parent_irq;
> +
> +    uint32_t vector;
> +    uint32_t base_addr;
> +    uint32_t protect;
> +    uint32_t nmi;
> +    uint32_t irq_pending[SUNXI_PIC_REG_IDX];

IDX is a wierd choice of suffix here. Is this really a "_NUM". I'm
happy as is though.

> +    uint32_t fiq_pending[SUNXI_PIC_REG_IDX];
> +    uint32_t select[SUNXI_PIC_REG_IDX];
> +    uint32_t enable[SUNXI_PIC_REG_IDX];
> +    uint32_t mask[SUNXI_PIC_REG_IDX];
> +    /*priority setting here*/
> +} SunxiPICState;
> +
> +static void sunxi_pic_update(SunxiPICState *s)
> +{
> +    uint8_t i, j;
> +    bool irq = false, fiq = false;
> +
> +    for (i = 0, j = 0; i < SUNXI_PIC_REG_IDX; i++) {
> +        if (s->irq_pending[i] == 0 && s->fiq_pending[i] == 0) {
> +            continue;
> +        }
> +        for (j = 0; j < 32; j++) {
> +            if (test_bit(j, (void *)&s->mask[i])) {
> +                continue;
> +            }
> +            if (test_bit(j, (void *)&s->irq_pending[i])) {
> +                irq = true;
> +            }
> +            if (test_bit(j, (void *)&s->fiq_pending[i]) &&
> +                test_bit(j, (void *)&s->select[i])) {
> +                fiq = true;
> +            }
> +            if (irq || fiq) {

This should be an && - the missed fiq problem i mentioned last time
still isnt solved. If an early iteration of this loop sets irq then no
latter iterations are given the chance to set fiq. You can only bail
out of the loop if both irq and fix are set. You could just ditch this
escape logic completely as I doubt its too much of a performance hit.

> +                goto out;
> +            }
> +        }
> +    }
> +
> +out:
> +    qemu_set_irq(s->parent_irq, irq);
> +    qemu_set_irq(s->parent_fiq, fiq);
> +}
> +
> +static void sunxi_pic_set_irq(void *opaque, int irq, int level)
> +{
> +    SunxiPICState *s = opaque;
> +
> +    if (level) {
> +        set_bit(irq%32, (void *)&s->irq_pending[irq/32]);
> +    }
> +    sunxi_pic_update(s);
> +}
> +
> +static uint64_t sunxi_pic_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    SunxiPICState *s = opaque;
> +    uint8_t index = (offset & 0xc)/4;
> +
> +    switch (offset) {
> +    case SUNXI_PIC_VECTOR:
> +        return s->vector;
> +        break;

Breaks after return. Fix globally.

Regards,
Peter