Hi, Philippe, On Tue, Nov 24, 2020 at 6:24 AM Philippe Mathieu-Daudé <f4...@amsat.org> wrote: > > On 11/23/20 9:52 PM, Philippe Mathieu-Daudé wrote: > > On 11/6/20 5:21 AM, Huacai Chen wrote: > >> As suggested by Philippe Mathieu-Daudé, rework Loongson's liointc: > >> 1, Move macro definitions to loongson_liointc.h; > >> 2, Remove magic values and use macros instead; > >> 3, Replace dead D() code by trace events. > >> > >> Suggested-by: Philippe Mathieu-Daudé <f4...@amsat.org> > >> Signed-off-by: Huacai Chen <che...@lemote.com> > >> --- > >> hw/intc/loongson_liointc.c | 49 > >> +++++++++++--------------------------- > >> include/hw/intc/loongson_liointc.h | 39 ++++++++++++++++++++++++++++++ > >> 2 files changed, 53 insertions(+), 35 deletions(-) > >> create mode 100644 include/hw/intc/loongson_liointc.h > >> > >> diff --git a/hw/intc/loongson_liointc.c b/hw/intc/loongson_liointc.c > >> index fbbfb57..be29e2f 100644 > >> --- a/hw/intc/loongson_liointc.c > >> +++ b/hw/intc/loongson_liointc.c > >> @@ -1,6 +1,7 @@ > >> /* > >> * QEMU Loongson Local I/O interrupt controler. > >> * > >> + * Copyright (c) 2020 Huacai Chen <che...@lemote.com> > >> * Copyright (c) 2020 Jiaxun Yang <jiaxun.y...@flygoat.com> > >> * > >> * This program is free software: you can redistribute it and/or modify > >> @@ -19,33 +20,11 @@ > >> */ > >> > >> #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 "qom/object.h" > >> - > >> -#define D(x) > >> - > >> -#define NUM_IRQS 32 > >> - > >> -#define NUM_CORES 4 > >> -#define NUM_IPS 4 > >> -#define NUM_PARENTS (NUM_CORES * NUM_IPS) > >> -#define PARENT_COREx_IPy(x, y) (NUM_IPS * x + y) > >> - > >> -#define R_MAPPER_START 0x0 > >> -#define R_MAPPER_END 0x20 > >> -#define R_ISR R_MAPPER_END > >> -#define R_IEN 0x24 > >> -#define R_IEN_SET 0x28 > >> -#define R_IEN_CLR 0x2c > >> -#define R_PERCORE_ISR(x) (0x40 + 0x8 * x) > >> -#define R_END 0x64 > >> - > >> -#define TYPE_LOONGSON_LIOINTC "loongson.liointc" > >> -DECLARE_INSTANCE_CHECKER(struct loongson_liointc, LOONGSON_LIOINTC, > >> - TYPE_LOONGSON_LIOINTC) > >> +#include "hw/intc/loongson_liointc.h" > >> > >> struct loongson_liointc { > >> SysBusDevice parent_obj; > >> @@ -123,14 +102,13 @@ liointc_read(void *opaque, hwaddr addr, unsigned int > >> size) > >> goto out; > >> } > >> > >> - /* Rest is 4 byte */ > >> + /* Rest are 4 bytes */ > >> if (size != 4 || (addr % 4)) { > >> goto out; > >> } > >> > >> - if (addr >= R_PERCORE_ISR(0) && > >> - addr < R_PERCORE_ISR(NUM_CORES)) { > >> - int core = (addr - R_PERCORE_ISR(0)) / 8; > >> + if (addr >= R_START && addr < R_END) { > >> + int core = (addr - R_START) / R_ISR_SIZE; > >> r = p->per_core_isr[core]; > >> goto out; > >> } > >> @@ -147,7 +125,8 @@ liointc_read(void *opaque, hwaddr addr, unsigned int > >> size) > >> } > >> > >> out: > >> - D(qemu_log("%s: size=%d addr=%lx val=%x\n", __func__, size, addr, r)); > >> + qemu_log_mask(CPU_LOG_INT, "%s: size=%d addr=%lx val=%x\n", > >> + __func__, size, addr, r); > >> return r; > >> } > >> > >> @@ -158,7 +137,8 @@ liointc_write(void *opaque, hwaddr addr, > >> struct loongson_liointc *p = opaque; > >> uint32_t value = val64; > >> > >> - D(qemu_log("%s: size=%d, addr=%lx val=%x\n", __func__, size, addr, > >> value)); > >> + qemu_log_mask(CPU_LOG_INT, "%s: size=%d, addr=%lx val=%x\n", > >> + __func__, size, addr, value); > >> > >> /* Mapper is 1 byte */ > >> if (size == 1 && addr < R_MAPPER_END) { > >> @@ -166,14 +146,13 @@ liointc_write(void *opaque, hwaddr addr, > >> goto out; > >> } > >> > >> - /* Rest is 4 byte */ > >> + /* Rest are 4 bytes */ > >> if (size != 4 || (addr % 4)) { > >> goto out; > >> } > >> > >> - if (addr >= R_PERCORE_ISR(0) && > >> - addr < R_PERCORE_ISR(NUM_CORES)) { > >> - int core = (addr - R_PERCORE_ISR(0)) / 8; > >> + if (addr >= R_START && addr < R_END) { > >> + int core = (addr - R_START) / R_ISR_SIZE; > >> p->per_core_isr[core] = value; > >> goto out; > >> } > >> @@ -224,7 +203,7 @@ static void loongson_liointc_init(Object *obj) > >> } > >> > >> memory_region_init_io(&p->mmio, obj, &pic_ops, p, > >> - "loongson.liointc", R_END); > >> + TYPE_LOONGSON_LIOINTC, R_END); > >> sysbus_init_mmio(SYS_BUS_DEVICE(obj), &p->mmio); > >> } > >> > >> diff --git a/include/hw/intc/loongson_liointc.h > >> b/include/hw/intc/loongson_liointc.h > >> new file mode 100644 > >> index 0000000..e11f482 > >> --- /dev/null > >> +++ b/include/hw/intc/loongson_liointc.h > >> @@ -0,0 +1,39 @@ > >> +/* > >> + * This file is subject to the terms and conditions of the GNU General > >> Public > >> + * License. See the file "COPYING" in the main directory of this archive > >> + * for more details. > >> + * > >> + * Copyright (c) 2020 Huacai Chen <che...@lemote.com> > >> + * Copyright (c) 2020 Jiaxun Yang <jiaxun.y...@flygoat.com> > >> + * > >> + */ > >> + > >> +#ifndef LOONSGON_LIOINTC_H > >> +#define LOONGSON_LIOINTC_H > > Clang is smart: > > hw/intc/loongson_liointc.h:11:9: error: 'LOONSGON_LIOINTC_H' is used as > a header guard here, followed by #define of a different macro > [-Werror,-Wheader-guard] > #ifndef LOONSGON_LIOINTC_H > ^~~~~~~~~~~~~~~~~~ > include/hw/intc/loongson_liointc.h:12:9: note: 'LOONGSON_LIOINTC_H' is > defined here; did you mean 'LOONSGON_LIOINTC_H'? > #define LOONGSON_LIOINTC_H > ^~~~~~~~~~~~~~~~~~ > LOONSGON_LIOINTC_H > 1 error generated. > > Once fixed: > Tested-by: Philippe Mathieu-Daudé <f4...@amsat.org> OK, will be fixed in the next version.
> > >> + > >> +#include "qemu/units.h" > >> +#include "hw/sysbus.h" > >> +#include "qom/object.h" > >> + > >> +#define NUM_IRQS 32 > >> + > >> +#define NUM_CORES 4 > >> +#define NUM_IPS 4 > >> +#define NUM_PARENTS (NUM_CORES * NUM_IPS) > >> +#define PARENT_COREx_IPy(x, y) (NUM_IPS * x + y) > >> + > >> +#define R_MAPPER_START 0x0 > >> +#define R_MAPPER_END 0x20 > >> +#define R_ISR R_MAPPER_END > >> +#define R_IEN 0x24 > >> +#define R_IEN_SET 0x28 > >> +#define R_IEN_CLR 0x2c > >> +#define R_ISR_SIZE 0x8 > >> +#define R_START 0x40 > >> +#define R_END 0x64 > > > > Can we keep the R_* definitions local in the .c? > > (if you agree I can amend that when applying). > > > > Thanks for cleaning! > > > > Reviewed-by: Philippe Mathieu-Daudé <f4...@amsat.org> > > > >> + > >> +#define TYPE_LOONGSON_LIOINTC "loongson.liointc" > >> +DECLARE_INSTANCE_CHECKER(struct loongson_liointc, LOONGSON_LIOINTC, > >> + TYPE_LOONGSON_LIOINTC) > >> + > >> +#endif /* LOONGSON_LIOINTC_H */ > >> > >