[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/2] hw/intc: Add Loongson Inter Processor Interrupt controll
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [PATCH 1/2] hw/intc: Add Loongson Inter Processor Interrupt controller |
Date: |
Thu, 14 Jan 2021 00:07:15 +0100 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.6.0 |
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.