Hi, Phillippe, On Mon, Nov 30, 2020 at 6:08 PM Philippe Mathieu-Daudé <f4...@amsat.org> wrote: > > On 11/28/20 7:19 AM, Huacai Chen wrote: > > On Tue, Nov 24, 2020 at 4:52 AM Philippe Mathieu-Daudé <f4...@amsat.org> > > 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 > >>> + > >>> +#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). > > If put them in .c, then .h is to small.., > > Not a problem: > > include/hw/ppc/openpic_kvm.h > include/hw/display/ramfb.h > include/hw/input/lasips2.h > include/hw/usb/chipidea.h > include/hw/s390x/ap-bridge.h > include/hw/char/lm32_juart.h > include/hw/isa/vt82c686.h > include/hw/net/lan9118.h > include/hw/intc/imx_gpcv2.h > include/hw/usb/xhci.h OK, I will put all these macros in .c file.
Huacai > > > but TYPE_LOONGSON_LIOINTC > > should be defined in .h since it will be used in other place. > > > > Huacai > >> > >> 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 */ > >>> > >