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

[Philippe Mathieu-Daudé]

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

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