Re: [PATCH V17 2/6] hw/intc: Rework Loongson LIOINTC

[Huacai Chen]

Hi, Phillippe,

On Mon, Nov 30, 2020 at 6:08 PM Philippe Mathieu-Daudé <f4bug@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é <f4bug@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é <f4bug@amsat.org>
> >>> Signed-off-by: Huacai Chen <chenhc@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 <chenhc@lemote.com>
> >>>   * Copyright (c) 2020 Jiaxun Yang <jiaxun.yang@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 <chenhc@lemote.com>
> >>> + * Copyright (c) 2020 Jiaxun Yang <jiaxun.yang@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é <f4bug@amsat.org>
> >>
> >>> +
> >>> +#define TYPE_LOONGSON_LIOINTC "loongson.liointc"
> >>> +DECLARE_INSTANCE_CHECKER(struct loongson_liointc, LOONGSON_LIOINTC,
> >>> +                         TYPE_LOONGSON_LIOINTC)
> >>> +
> >>> +#endif /* LOONGSON_LIOINTC_H */
> >>>
> >