[1/2] hw/intc: Add Loongson Inter Processor Interrupt controller

Loongson IPI controller is a MMIO based simple level triggered
interrupt controller. It will trigger IRQ to it's upstream
processor when set register is written.

It also has 8 32bit mailboxes to pass boot information to
secondary processor.

Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
---
 hw/intc/Kconfig                |   3 +
 hw/intc/loongson_ipi.c         | 146 +++++++++++++++++++++++++++++++++
 hw/intc/meson.build            |   1 +
 include/hw/intc/loongson_ipi.h |  20 +++++
 4 files changed, 170 insertions(+)
 create mode 100644 hw/intc/loongson_ipi.c
 create mode 100644 include/hw/intc/loongson_ipi.h

Hi Jiaxun,

On 1/12/21 4:32 AM, Jiaxun Yang wrote:
> Loongson IPI controller is a MMIO based simple level triggered
> interrupt controller. It will trigger IRQ to it's upstream
> processor when set register is written.
> 
> It also has 8 32bit mailboxes to pass boot information to
> secondary processor.

Hmm the datasheet describe them as 4 64bit mailboxes (also
accessible in 32bit).

I have no problem modelling this device as 8x32 to simplify,
but the documentation should be accurate. Along with a comment
describing NUM_MBOX value.

> Signed-off-by: Jiaxun Yang <jiaxun.yang@flygoat.com>
> ---
>  hw/intc/Kconfig                |   3 +
>  hw/intc/loongson_ipi.c         | 146 +++++++++++++++++++++++++++++++++
>  hw/intc/meson.build            |   1 +
>  include/hw/intc/loongson_ipi.h |  20 +++++
>  4 files changed, 170 insertions(+)
>  create mode 100644 hw/intc/loongson_ipi.c
>  create mode 100644 include/hw/intc/loongson_ipi.h
> 
> diff --git a/hw/intc/Kconfig b/hw/intc/Kconfig
> index c18d11142a..0e15102662 100644
> --- a/hw/intc/Kconfig
> +++ b/hw/intc/Kconfig
> @@ -59,6 +59,9 @@ config RX_ICU
>  config LOONGSON_LIOINTC
>      bool
>  
> +config LOONGSON_IPI
> +    bool
> +
>  config SIFIVE_CLINT
>      bool
>  
> diff --git a/hw/intc/loongson_ipi.c b/hw/intc/loongson_ipi.c
> new file mode 100644
> index 0000000000..7246f05f9e
> --- /dev/null
> +++ b/hw/intc/loongson_ipi.c
> @@ -0,0 +1,146 @@
> +/*
> + * QEMU Loongson Inter Processor Interrupt Controller
> + *
> + * Copyright (c) 2020-2021 Jiaxun Yang <jiaxun.yang@flygoat.com>
> + *
> + * 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.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <https://www.gnu.org/licenses/>.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/sysbus.h"
> +#include "qemu/module.h"
> +#include "qemu/log.h"
> +#include "hw/irq.h"
> +#include "hw/qdev-properties.h"
> +#include "hw/intc/loongson_ipi.h"
> +
> +#define R_ISR       0
> +#define R_IEN       1
> +#define R_SET       2
> +#define R_CLR       3
> +/* No register between 0x10~0x20 */
> +#define R_MBOX0     8
> +#define NUM_MBOX    8
> +#define R_END       16

Can you use an enum please?

> +
> +struct loongson_ipi {
> +    SysBusDevice parent_obj;
> +
> +    MemoryRegion mmio;
> +    qemu_irq parent_irq;
> +
> +    uint32_t isr;
> +    uint32_t ien;
> +    uint32_t mbox[NUM_MBOX];
> +};
> +
> +static uint64_t
> +ipi_read(void *opaque, hwaddr addr, unsigned int size)
> +{
> +    struct loongson_ipi *p = opaque;
> +    uint64_t r = 0;
> +
> +    addr >>= 2;
> +    switch (addr) {
> +    case R_ISR:
> +        r = p->isr;
> +        break;
> +    case R_IEN:
> +        r = p->ien;
> +        break;
> +    case R_MBOX0 ... (R_END - 1):
> +        r = p->mbox[addr - R_MBOX0];
> +        break;

Logging here write-only registers (R_SET, R_CLR) as LOG_GUEST_ERROR
might result useful.

> +    default:
> +        break;
> +    }
> +
> +    qemu_log_mask(CPU_LOG_INT,
> +                  "%s: size=%d, addr=%"HWADDR_PRIx", val=%"PRIx64"\n",
> +                  __func__, size, addr, r);

CPU_LOG_INT is used for the architectural part (within GS464V).
For hardware we use trace events. Do you mind changing?

> +
> +    return r;
> +}
> +
> +static void
> +ipi_write(void *opaque, hwaddr addr,
> +          uint64_t val64, unsigned int size)
> +{
> +    struct loongson_ipi *p = opaque;
> +    uint32_t value = val64;
> +
> +    addr >>= 2;
> +    switch (addr) {
> +    case R_ISR:
> +        /* Do nothing */
> +        break;
> +    case R_IEN:
> +        p->ien = value;
> +        break;
> +    case R_SET:
> +        p->isr |= value;
> +        break;
> +    case R_CLR:
> +        p->isr &= ~value;
> +        break;
> +    case R_MBOX0 ... (R_END - 1):
> +        p->mbox[addr - R_MBOX0] = value;
> +        break;
> +    default:
> +        break;
> +    }
> +    p->isr &= p->ien;
> +
> +    qemu_log_mask(CPU_LOG_INT,
> +                  "%s: size=%d, addr=%"HWADDR_PRIx", val=%"PRIx32"\n",
> +                  __func__, size, addr, value);

Ditto.

> +
> +    qemu_set_irq(p->parent_irq, !!p->isr);
> +}
> +
> +static const MemoryRegionOps pic_ops = {
> +    .read = ipi_read,
> +    .write = ipi_write,
> +    .endianness = DEVICE_NATIVE_ENDIAN,
> +    .valid = {
> +        .min_access_size = 4,
> +        .max_access_size = 4
> +    }

64-bit access are valid. You probably want:

       .impl = {
           .min_access_size = 4,
           .max_access_size = 4
       },
       .valid = {
           .min_access_size = 4,
           .max_access_size = 8
       },

> +};
> +
> +static void loongson_ipi_init(Object *obj)
> +{
> +    struct loongson_ipi *p = LOONGSON_IPI(obj);
> +
> +    sysbus_init_irq(SYS_BUS_DEVICE(obj), &p->parent_irq);
> +
> +    memory_region_init_io(&p->mmio, obj, &pic_ops, p, "loongson.ipi",
> +                          R_END * 4);
> +    sysbus_init_mmio(SYS_BUS_DEVICE(obj), &p->mmio);
> +}
> +

You forgot the reset() method which clear the registers.

> +static const TypeInfo loongson_ipi_info = {
> +    .name          = TYPE_LOONGSON_IPI,
> +    .parent        = TYPE_SYS_BUS_DEVICE,
> +    .instance_size = sizeof(struct loongson_ipi),
> +    .instance_init = loongson_ipi_init,
> +};
> +
> +static void loongson_ipi_register_types(void)
> +{
> +    type_register_static(&loongson_ipi_info);
> +}
> +
> +type_init(loongson_ipi_register_types)
> diff --git a/hw/intc/meson.build b/hw/intc/meson.build
> index 53cba11569..5257c5fb94 100644
> --- a/hw/intc/meson.build
> +++ b/hw/intc/meson.build
> @@ -36,6 +36,7 @@ specific_ss.add(when: 'CONFIG_GRLIB', if_true: files('grlib_irqmp.c'))
>  specific_ss.add(when: 'CONFIG_IBEX', if_true: files('ibex_plic.c'))
>  specific_ss.add(when: 'CONFIG_IOAPIC', if_true: files('ioapic.c'))
>  specific_ss.add(when: 'CONFIG_LOONGSON_LIOINTC', if_true: files('loongson_liointc.c'))
> +specific_ss.add(when: 'CONFIG_LOONGSON_IPI', if_true: files('loongson_ipi.c'))
>  specific_ss.add(when: 'CONFIG_MIPS_CPS', if_true: files('mips_gic.c'))
>  specific_ss.add(when: 'CONFIG_OMAP', if_true: files('omap_intc.c'))
>  specific_ss.add(when: 'CONFIG_OMPIC', if_true: files('ompic.c'))
> diff --git a/include/hw/intc/loongson_ipi.h b/include/hw/intc/loongson_ipi.h
> new file mode 100644
> index 0000000000..a535c467bf
> --- /dev/null
> +++ b/include/hw/intc/loongson_ipi.h

If possible use the scripts/git.orderfile to ease review.

Minor comments, but the patch is good!

Thanks,

Phil.