Re: [PATCH V17 2/6] hw/intc: Rework Loongson LIOINTC - Philippe Mathieu-Daudé

From: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
To: Huacai Chen <zltjiangshi@gmail.com>
Cc: Aleksandar Rikalo <aleksandar.rikalo@syrmia.com>,
	Huacai Chen <chenhuacai@gmail.com>,
	qemu-devel@nongnu.org,
	Aleksandar Markovic <aleksandar.qemu.devel@gmail.com>,
	Huacai Chen <chenhc@lemote.com>,
	Aurelien Jarno <aurelien@aurel32.net>
Subject: Re: [PATCH V17 2/6] hw/intc: Rework Loongson LIOINTC
Date: Mon, 23 Nov 2020 23:24:13 +0100	[thread overview]
Message-ID: <a60e002b-b102-a7b9-3de7-bb355319f8e1@amsat.org> (raw)
In-Reply-To: <a70dae49-2a47-12bc-f580-640f032b78fd@amsat.org>

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>

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