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

[Huacai Chen]

Hi, Philippe,

On Tue, Nov 24, 2020 at 6:24 AM Philippe Mathieu-Daudé <f4bug@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é <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
>
> 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é <f4bug@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é <f4bug@amsat.org>
> >
> >> +
> >> +#define TYPE_LOONGSON_LIOINTC "loongson.liointc"
> >> +DECLARE_INSTANCE_CHECKER(struct loongson_liointc, LOONGSON_LIOINTC,
> >> +                         TYPE_LOONGSON_LIOINTC)
> >> +
> >> +#endif /* LOONGSON_LIOINTC_H */
> >>
> >