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

[Huacai Chen]

Hi, Philippe,

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.., 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 */
> >