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
+
+#define TYPE_LOONGSON_LIOINTC "loongson.liointc"
+DECLARE_INSTANCE_CHECKER(struct loongson_liointc, LOONGSON_LIOINTC,
+                         TYPE_LOONGSON_LIOINTC)
+
+#endif /* LOONGSON_LIOINTC_H */
-- 
2.7.0

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).

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

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

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

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

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

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