[PATCH v2 00/11] More SH4 clean ups

[PATCH v2 00/11] More SH4 clean ups

[BALATON Zoltan]

Based-on: <cover.1635036053.git.balaton@eik.bme.hu>
^ (hw/sh4: Codeing style fixes)

Continuing the clean up stared in previous series this now removes
printfs and QOM-ify sh_serial.

v2: separate sh_serial trace events, split QOM-ify patch for easier
review and some more patches to clean up sh_intc a bit

Regards,
BALATON Zoltan

BALATON Zoltan (11):
  hw/sh4: Fix a typo in a comment
  hw//sh4: Use qemu_log instead of fprintf to stderr
  hw/sh4: Change debug printfs to traces
  hw/sh4/r2d: Use error_report instead of fprintf to stderr
  hw/char/sh_serial: Rename type sh_serial_state to SHSerialState
  hw/char/sh_serial: QOM-ify
  hw/char/sh_serial: Add device id to trace output
  hw/intc/sh_intc: Use existing macro instead of local one
  hw/intc/sh_intc: Turn some defines into an enum
  hw/intc/sh_intc: Clean up iomem region
  hw/intc/sh_intc: Drop another useless macro

 hw/char/sh_serial.c   | 149 +++++++++++++++++++++++------------------
 hw/char/trace-events  |   4 ++
 hw/intc/sh_intc.c     | 151 +++++++++++++-----------------------------
 hw/intc/trace-events  |   8 +++
 hw/sh4/r2d.c          |   5 +-
 hw/sh4/sh7750.c       |  83 +++++++++++++++--------
 hw/sh4/trace-events   |   3 +
 hw/sh4/trace.h        |   1 +
 hw/timer/sh_timer.c   |  14 +---
 hw/timer/trace-events |   3 +
 include/hw/sh4/sh.h   |   9 +--
 meson.build           |   1 +
 12 files changed, 214 insertions(+), 217 deletions(-)
 create mode 100644 hw/sh4/trace-events
 create mode 100644 hw/sh4/trace.h

-- 
2.21.4

[PATCH v2 02/11] hw//sh4: Use qemu_log instead of fprintf to stderr

[BALATON Zoltan]

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/char/sh_serial.c |  7 ++++---
 hw/sh4/sh7750.c     | 13 ++++++-------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/hw/char/sh_serial.c b/hw/char/sh_serial.c
index 1b1e6a6a04..c4231975c7 100644
--- a/hw/char/sh_serial.c
+++ b/hw/char/sh_serial.c
@@ -30,6 +30,7 @@
 #include "hw/sh4/sh.h"
 #include "chardev/char-fe.h"
 #include "qapi/error.h"
+#include "qemu/log.h"
 #include "qemu/timer.h"
 
 //#define DEBUG_SERIAL
@@ -200,8 +201,8 @@ static void sh_serial_write(void *opaque, hwaddr offs,
         }
     }
 
-    fprintf(stderr, "sh_serial: unsupported write to 0x%02"
-            HWADDR_PRIx "\n", offs);
+    qemu_log_mask(LOG_UNIMP, "sh_serial: unsupported write to 0x%02"
+                  HWADDR_PRIx "\n", offs);
     abort();
 }
 
@@ -307,7 +308,7 @@ static uint64_t sh_serial_read(void *opaque, hwaddr offs,
 #endif
 
     if (ret & ~((1 << 16) - 1)) {
-        fprintf(stderr, "sh_serial: unsupported read from 0x%02"
+        qemu_log_mask(LOG_UNIMP, "sh_serial: unsupported read from 0x%02"
                 HWADDR_PRIx "\n", offs);
         abort();
     }
diff --git a/hw/sh4/sh7750.c b/hw/sh4/sh7750.c
index ca7e261aba..f2f251f165 100644
--- a/hw/sh4/sh7750.c
+++ b/hw/sh4/sh7750.c
@@ -24,6 +24,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/log.h"
 #include "hw/irq.h"
 #include "hw/sh4/sh.h"
 #include "sysemu/sysemu.h"
@@ -205,13 +206,13 @@ static void portb_changed(SH7750State *s, uint16_t prev)
 
 static void error_access(const char *kind, hwaddr addr)
 {
-    fprintf(stderr, "%s to %s (0x" TARGET_FMT_plx ") not supported\n",
-            kind, regname(addr), addr);
+    qemu_log_mask(LOG_GUEST_ERROR, "%s to %s (0x" TARGET_FMT_plx
+                  ") not supported\n", kind, regname(addr), addr);
 }
 
 static void ignore_access(const char *kind, hwaddr addr)
 {
-    fprintf(stderr, "%s to %s (0x" TARGET_FMT_plx ") ignored\n",
+    qemu_log_mask(LOG_UNIMP, "%s to %s (0x" TARGET_FMT_plx ") ignored\n",
             kind, regname(addr), addr);
 }
 
@@ -241,8 +242,7 @@ static uint32_t sh7750_mem_readw(void *opaque, hwaddr addr)
     case SH7750_PCR_A7:
         return s->pcr;
     case SH7750_RFCR_A7:
-        fprintf(stderr,
-                "Read access to refresh count register, incrementing\n");
+        /* Read access to refresh count register, incrementing */
         return s->rfcr++;
     case SH7750_PDTRA_A7:
         return porta_lines(s);
@@ -363,13 +363,12 @@ static void sh7750_mem_writew(void *opaque, hwaddr addr,
         portb_changed(s, temp);
         return;
     case SH7750_RFCR_A7:
-        fprintf(stderr, "Write access to refresh count register\n");
         s->rfcr = mem_value;
         return;
     case SH7750_GPIOIC_A7:
         s->gpioic = mem_value;
         if (mem_value != 0) {
-            fprintf(stderr, "I/O interrupts not implemented\n");
+            qemu_log_mask(LOG_UNIMP, "I/O interrupts not implemented\n");
             abort();
         }
         return;
-- 
2.21.4

Re: [PATCH v2 02/11] hw//sh4: Use qemu_log instead of fprintf to stderr

[Philippe Mathieu-Daudé]

On 10/27/21 15:46, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  hw/char/sh_serial.c |  7 ++++---
>  hw/sh4/sh7750.c     | 13 ++++++-------
>  2 files changed, 10 insertions(+), 10 deletions(-)

>      case SH7750_GPIOIC_A7:
>          s->gpioic = mem_value;
>          if (mem_value != 0) {
> -            fprintf(stderr, "I/O interrupts not implemented\n");
> +            qemu_log_mask(LOG_UNIMP, "I/O interrupts not implemented\n");
>              abort();

This change is annoying. Before you'd get an error message and the abort
signal. Now if you don't use "-d unimp" you get an abort without
explanation. Can we use error_report() instead?

>          }
>          return;
> 

Re: [PATCH v2 02/11] hw//sh4: Use qemu_log instead of fprintf to stderr

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1134 bytes --]

On Wed, 27 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/27/21 15:46, BALATON Zoltan wrote:
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  hw/char/sh_serial.c |  7 ++++---
>>  hw/sh4/sh7750.c     | 13 ++++++-------
>>  2 files changed, 10 insertions(+), 10 deletions(-)
>
>>      case SH7750_GPIOIC_A7:
>>          s->gpioic = mem_value;
>>          if (mem_value != 0) {
>> -            fprintf(stderr, "I/O interrupts not implemented\n");
>> +            qemu_log_mask(LOG_UNIMP, "I/O interrupts not implemented\n");
>>              abort();
>
> This change is annoying. Before you'd get an error message and the abort
> signal. Now if you don't use "-d unimp" you get an abort without
> explanation. Can we use error_report() instead?

I plan to remove the aborts later. What's annoying is that the guest can 
crash QEMU by doing something invalid. Other devices don't do this just 
report the error with LOG_UNIMP and continue even if it will not work so I 
follow that convention but did not yet cleaned up the aborts.

Regards,
BALATON Zoltan

[PATCH v2 03/11] hw/sh4: Change debug printfs to traces

[BALATON Zoltan]

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/char/sh_serial.c   | 13 ++-----
 hw/char/trace-events  |  4 +++
 hw/intc/sh_intc.c     | 79 +++++++++++--------------------------------
 hw/intc/trace-events  |  8 +++++
 hw/sh4/sh7750.c       |  8 ++---
 hw/sh4/trace-events   |  3 ++
 hw/sh4/trace.h        |  1 +
 hw/timer/sh_timer.c   | 12 ++-----
 hw/timer/trace-events |  3 ++
 meson.build           |  1 +
 10 files changed, 48 insertions(+), 84 deletions(-)
 create mode 100644 hw/sh4/trace-events
 create mode 100644 hw/sh4/trace.h

diff --git a/hw/char/sh_serial.c b/hw/char/sh_serial.c
index c4231975c7..bbf7586892 100644
--- a/hw/char/sh_serial.c
+++ b/hw/char/sh_serial.c
@@ -32,8 +32,7 @@
 #include "qapi/error.h"
 #include "qemu/log.h"
 #include "qemu/timer.h"
-
-//#define DEBUG_SERIAL
+#include "trace.h"
 
 #define SH_SERIAL_FLAG_TEND (1 << 0)
 #define SH_SERIAL_FLAG_TDE  (1 << 1)
@@ -90,10 +89,7 @@ static void sh_serial_write(void *opaque, hwaddr offs,
     sh_serial_state *s = opaque;
     unsigned char ch;
 
-#ifdef DEBUG_SERIAL
-    printf("sh_serial: write offs=0x%02x val=0x%02x\n",
-           offs, val);
-#endif
+    trace_sh_serial_write(size, offs, val);
     switch (offs) {
     case 0x00: /* SMR */
         s->smr = val & ((s->feat & SH_SERIAL_FEAT_SCIF) ? 0x7b : 0xff);
@@ -302,10 +298,7 @@ static uint64_t sh_serial_read(void *opaque, hwaddr offs,
             break;
         }
     }
-#ifdef DEBUG_SERIAL
-    printf("sh_serial: read offs=0x%02x val=0x%x\n",
-           offs, ret);
-#endif
+    trace_sh_serial_read(size, offs, ret);
 
     if (ret & ~((1 << 16) - 1)) {
         qemu_log_mask(LOG_UNIMP, "sh_serial: unsupported read from 0x%02"
diff --git a/hw/char/trace-events b/hw/char/trace-events
index b774832af4..4a92e7674a 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -101,3 +101,7 @@ exynos_uart_rx_timeout(uint32_t channel, uint32_t stat, uint32_t intsp) "UART%d:
 
 # cadence_uart.c
 cadence_uart_baudrate(unsigned baudrate) "baudrate %u"
+
+# sh_serial.c
+sh_serial_read(unsigned size, uint64_t offs, uint64_t val) " size %d offs 0x%02" PRIx64 " -> 0x%02" PRIx64
+sh_serial_write(unsigned size, uint64_t offs, uint64_t val) "size %d offs 0x%02" PRIx64 " <- 0x%02" PRIx64
diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
index e7c9964dba..c1058d97c0 100644
--- a/hw/intc/sh_intc.c
+++ b/hw/intc/sh_intc.c
@@ -9,13 +9,12 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/log.h"
 #include "cpu.h"
 #include "hw/sh4/sh_intc.h"
 #include "hw/irq.h"
 #include "hw/sh4/sh.h"
-
-//#define DEBUG_INTC
-//#define DEBUG_INTC_SOURCES
+#include "trace.h"
 
 #define INTC_A7(x) ((x) & 0x1fffffff)
 
@@ -57,20 +56,14 @@ void sh_intc_toggle_source(struct intc_source *source,
         }
     }
 
-  if (enable_changed || assert_adj || pending_changed) {
-#ifdef DEBUG_INTC_SOURCES
-            printf("sh_intc: (%d/%d/%d/%d) interrupt source 0x%x %s%s%s\n",
-                   source->parent->pending,
-                   source->asserted,
-                   source->enable_count,
-                   source->enable_max,
-                   source->vect,
-                   source->asserted ? "asserted " :
-                   assert_adj ? "deasserted" : "",
-                   enable_changed == 1 ? "enabled " :
-                   enable_changed == -1 ? "disabled " : "",
-                   source->pending ? "pending" : "");
-#endif
+    if (enable_changed || assert_adj || pending_changed) {
+        trace_sh_intc_sources(source->parent->pending, source->asserted,
+                              source->enable_count, source->enable_max,
+                              source->vect, source->asserted ? "asserted " :
+                              assert_adj ? "deasserted" : "",
+                              enable_changed == 1 ? "enabled " :
+                              enable_changed == -1 ? "disabled " : "",
+                              source->pending ? "pending" : "");
   }
 }
 
@@ -101,10 +94,7 @@ int sh_intc_get_pending_vector(struct intc_desc *desc, int imask)
         struct intc_source *source = desc->sources + i;
 
         if (source->pending) {
-#ifdef DEBUG_INTC_SOURCES
-            printf("sh_intc: (%d) returning interrupt source 0x%x\n",
-                   desc->pending, source->vect);
-#endif
+            trace_sh_intc_pending(desc->pending, source->vect);
             return source->vect;
         }
     }
@@ -199,30 +189,22 @@ static void sh_intc_toggle_mask(struct intc_desc *desc, intc_enum id,
         return;
     }
     if (!source->next_enum_id && (!source->enable_max || !source->vect)) {
-#ifdef DEBUG_INTC_SOURCES
-        printf("sh_intc: reserved interrupt source %d modified\n", id);
-#endif
+        qemu_log_mask(LOG_UNIMP,
+                      "sh_intc: reserved interrupt source %d modified\n", id);
         return;
     }
 
     if (source->vect) {
         sh_intc_toggle_source(source, enable ? 1 : -1, 0);
     }
-#ifdef DEBUG_INTC
-    else {
-        printf("setting interrupt group %d to %d\n", id, !!enable);
-    }
-#endif
 
     if ((is_group || !source->vect) && source->next_enum_id) {
         sh_intc_toggle_mask(desc, source->next_enum_id, enable, 1);
     }
 
-#ifdef DEBUG_INTC
     if (!source->vect) {
-        printf("setting interrupt group %d to %d - done\n", id, !!enable);
+        trace_sh_intc_set(id, !!enable);
     }
-#endif
 }
 
 static uint64_t sh_intc_read(void *opaque, hwaddr offset,
@@ -235,12 +217,9 @@ static uint64_t sh_intc_read(void *opaque, hwaddr offset,
     unsigned int mode = 0;
     unsigned long *valuep;
 
-#ifdef DEBUG_INTC
-    printf("sh_intc_read 0x%lx\n", (unsigned long) offset);
-#endif
-
     sh_intc_locate(desc, (unsigned long)offset, &valuep,
                    &enum_ids, &first, &width, &mode);
+    trace_sh_intc_read(size, offset, *valuep);
     return *valuep;
 }
 
@@ -256,13 +235,9 @@ static void sh_intc_write(void *opaque, hwaddr offset,
     unsigned long *valuep;
     unsigned long mask;
 
-#ifdef DEBUG_INTC
-    printf("sh_intc_write 0x%lx 0x%08x\n", (unsigned long) offset, value);
-#endif
-
+    trace_sh_intc_write(size, offset, value);
     sh_intc_locate(desc, (unsigned long)offset, &valuep,
                    &enum_ids, &first, &width, &mode);
-
     switch (mode) {
     case INTC_MODE_ENABLE_REG | INTC_MODE_IS_PRIO:
         break;
@@ -282,18 +257,10 @@ static void sh_intc_write(void *opaque, hwaddr offset,
         if ((*valuep & mask) == (value & mask)) {
             continue;
         }
-#if 0
-        printf("k = %d, first = %d, enum = %d, mask = 0x%08x\n",
-               k, first, enum_ids[k], (unsigned int)mask);
-#endif
         sh_intc_toggle_mask(desc, enum_ids[k], value & mask, 0);
     }
 
     *valuep = value;
-
-#ifdef DEBUG_INTC
-    printf("sh_intc_write 0x%lx -> 0x%08x\n", (unsigned long) offset, value);
-#endif
 }
 
 static const MemoryRegionOps sh_intc_ops = {
@@ -416,11 +383,8 @@ void sh_intc_register_sources(struct intc_desc *desc,
         s = sh_intc_source(desc, vect->enum_id);
         if (s) {
             s->vect = vect->vect;
-
-#ifdef DEBUG_INTC_SOURCES
-            printf("sh_intc: registered source %d -> 0x%04x (%d/%d)\n",
-                   vect->enum_id, s->vect, s->enable_count, s->enable_max);
-#endif
+            trace_sh_intc_register("source", vect->enum_id, s->vect,
+                                   s->enable_count, s->enable_max);
         }
     }
 
@@ -438,11 +402,8 @@ void sh_intc_register_sources(struct intc_desc *desc,
                 s = sh_intc_source(desc, gr->enum_ids[k - 1]);
                 s->next_enum_id = gr->enum_ids[k];
             }
-
-#ifdef DEBUG_INTC_SOURCES
-            printf("sh_intc: registered group %d (%d/%d)\n",
-                   gr->enum_id, s->enable_count, s->enable_max);
-#endif
+            trace_sh_intc_register("group", gr->enum_id, 0xffff,
+                                   s->enable_count, s->enable_max);
         }
     }
 }
diff --git a/hw/intc/trace-events b/hw/intc/trace-events
index 6a17d38998..9c7e41f41c 100644
--- a/hw/intc/trace-events
+++ b/hw/intc/trace-events
@@ -238,3 +238,11 @@ goldfish_pic_write(void *dev, int idx, unsigned int addr, unsigned int size, uin
 goldfish_pic_reset(void *dev, int idx) "pic: %p goldfish-irq.%d"
 goldfish_pic_realize(void *dev, int idx) "pic: %p goldfish-irq.%d"
 goldfish_pic_instance_init(void *dev) "pic: %p goldfish-irq"
+
+# sh_intc.c
+sh_intc_sources(int p, int a, int c, int m, unsigned short v, const char *s1, const char *s2, const char *s3) "(%d/%d/%d/%d) interrupt source 0x%x %s%s%s"
+sh_intc_pending(int p, unsigned short v) "(%d) returning interrupt source 0x%x"
+sh_intc_register(const char *s, int id, unsigned short v, int c, int m) "%s %d -> 0x%04x (%d/%d)"
+sh_intc_read(unsigned size, uint64_t offset, unsigned long val) "size %d 0x%" PRIx64 " -> 0x%" PRIx64
+sh_intc_write(unsigned size, uint64_t offset, unsigned long val) "size %d 0x%" PRIx64 " <- 0x%" PRIx64
+sh_intc_set(int id, int enable) "setting interrupt group %d to %d"
diff --git a/hw/sh4/sh7750.c b/hw/sh4/sh7750.c
index f2f251f165..c3c3caf952 100644
--- a/hw/sh4/sh7750.c
+++ b/hw/sh4/sh7750.c
@@ -33,6 +33,7 @@
 #include "hw/sh4/sh_intc.h"
 #include "hw/timer/tmu012.h"
 #include "exec/exec-all.h"
+#include "trace.h"
 
 #define NB_DEVICES 4
 
@@ -148,15 +149,11 @@ static void porta_changed(SH7750State *s, uint16_t prev)
     uint16_t currenta, changes;
     int i, r = 0;
 
-#if 0
-    fprintf(stderr, "porta changed from 0x%04x to 0x%04x\n",
-            prev, porta_lines(s));
-    fprintf(stderr, "pdtra=0x%04x, pctra=0x%08x\n", s->pdtra, s->pctra);
-#endif
     currenta = porta_lines(s);
     if (currenta == prev) {
         return;
     }
+    trace_sh7750_porta(prev, currenta, s->pdtra, s->pctra);
     changes = currenta ^ prev;
 
     for (i = 0; i < NB_DEVICES; i++) {
@@ -183,6 +180,7 @@ static void portb_changed(SH7750State *s, uint16_t prev)
     if (currentb == prev) {
         return;
     }
+    trace_sh7750_portb(prev, currentb, s->pdtrb, s->pctrb);
     changes = currentb ^ prev;
 
     for (i = 0; i < NB_DEVICES; i++) {
diff --git a/hw/sh4/trace-events b/hw/sh4/trace-events
new file mode 100644
index 0000000000..4b61cd56c8
--- /dev/null
+++ b/hw/sh4/trace-events
@@ -0,0 +1,3 @@
+# sh7750.c
+sh7750_porta(uint16_t prev, uint16_t cur, uint16_t pdtr, uint16_t pctr) "porta changed from 0x%04x to 0x%04x\npdtra=0x%04x, pctra=0x%08x"
+sh7750_portb(uint16_t prev, uint16_t cur, uint16_t pdtr, uint16_t pctr) "portb changed from 0x%04x to 0x%04x\npdtrb=0x%04x, pctrb=0x%08x"
diff --git a/hw/sh4/trace.h b/hw/sh4/trace.h
new file mode 100644
index 0000000000..e2c13323b7
--- /dev/null
+++ b/hw/sh4/trace.h
@@ -0,0 +1 @@
+#include "trace/trace-hw_sh4.h"
diff --git a/hw/timer/sh_timer.c b/hw/timer/sh_timer.c
index 0a18ac8276..1f29f4a650 100644
--- a/hw/timer/sh_timer.c
+++ b/hw/timer/sh_timer.c
@@ -15,8 +15,7 @@
 #include "hw/sh4/sh.h"
 #include "hw/timer/tmu012.h"
 #include "hw/ptimer.h"
-
-//#define DEBUG_TIMER
+#include "trace.h"
 
 #define TIMER_TCR_TPSC          (7 << 0)
 #define TIMER_TCR_CKEG          (3 << 3)
@@ -203,10 +202,7 @@ static void sh_timer_start_stop(void *opaque, int enable)
 {
     sh_timer_state *s = (sh_timer_state *)opaque;
 
-#ifdef DEBUG_TIMER
-    printf("sh_timer_start_stop %d (%d)\n", enable, s->enabled);
-#endif
-
+    trace_sh_timer_start_stop(enable, s->enabled);
     ptimer_transaction_begin(s->timer);
     if (s->enabled && !enable) {
         ptimer_stop(s->timer);
@@ -216,10 +212,6 @@ static void sh_timer_start_stop(void *opaque, int enable)
     }
     ptimer_transaction_commit(s->timer);
     s->enabled = !!enable;
-
-#ifdef DEBUG_TIMER
-    printf("sh_timer_start_stop done %d\n", s->enabled);
-#endif
 }
 
 static void sh_timer_tick(void *opaque)
diff --git a/hw/timer/trace-events b/hw/timer/trace-events
index d0edcd2a80..653025817b 100644
--- a/hw/timer/trace-events
+++ b/hw/timer/trace-events
@@ -94,3 +94,6 @@ sifive_pwm_set_alarm(uint64_t alarm, uint64_t now) "Setting alarm to: 0x%" PRIx6
 sifive_pwm_interrupt(int num) "Interrupt %d"
 sifive_pwm_read(uint64_t offset) "Read at address: 0x%" PRIx64
 sifive_pwm_write(uint64_t data, uint64_t offset) "Write 0x%" PRIx64 " at address: 0x%" PRIx64
+
+# sh_timer.c
+sh_timer_start_stop(int enable, int current) "%d (%d)"
diff --git a/meson.build b/meson.build
index 2c5b53cbe2..b092728397 100644
--- a/meson.build
+++ b/meson.build
@@ -2459,6 +2459,7 @@ if have_system
     'hw/s390x',
     'hw/scsi',
     'hw/sd',
+    'hw/sh4',
     'hw/sparc',
     'hw/sparc64',
     'hw/ssi',
-- 
2.21.4

[PATCH v2 07/11] hw/char/sh_serial: Add device id to trace output

[BALATON Zoltan]

Normally there are at least two sh_serial instances. Add device id to
trace messages to make it clear which instance they belong to
otherwise its not possible to tell which serial device is accessed.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/char/sh_serial.c  | 6 ++++--
 hw/char/trace-events | 4 ++--
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/hw/char/sh_serial.c b/hw/char/sh_serial.c
index ad576b693b..3c400b2dd1 100644
--- a/hw/char/sh_serial.c
+++ b/hw/char/sh_serial.c
@@ -94,9 +94,10 @@ static void sh_serial_write(void *opaque, hwaddr offs,
                             uint64_t val, unsigned size)
 {
     SHSerialState *s = opaque;
+    DeviceState *d = DEVICE(s);
     unsigned char ch;
 
-    trace_sh_serial_write(size, offs, val);
+    trace_sh_serial_write(d->id, size, offs, val);
     switch (offs) {
     case 0x00: /* SMR */
         s->smr = val & ((s->feat & SH_SERIAL_FEAT_SCIF) ? 0x7b : 0xff);
@@ -213,6 +214,7 @@ static uint64_t sh_serial_read(void *opaque, hwaddr offs,
                                unsigned size)
 {
     SHSerialState *s = opaque;
+    DeviceState *d = DEVICE(s);
     uint32_t ret = ~0;
 
 #if 0
@@ -305,7 +307,7 @@ static uint64_t sh_serial_read(void *opaque, hwaddr offs,
             break;
         }
     }
-    trace_sh_serial_read(size, offs, ret);
+    trace_sh_serial_read(d->id, size, offs, ret);
 
     if (ret & ~((1 << 16) - 1)) {
         qemu_log_mask(LOG_UNIMP, "sh_serial: unsupported read from 0x%02"
diff --git a/hw/char/trace-events b/hw/char/trace-events
index 4a92e7674a..2ecb36232e 100644
--- a/hw/char/trace-events
+++ b/hw/char/trace-events
@@ -103,5 +103,5 @@ exynos_uart_rx_timeout(uint32_t channel, uint32_t stat, uint32_t intsp) "UART%d:
 cadence_uart_baudrate(unsigned baudrate) "baudrate %u"
 
 # sh_serial.c
-sh_serial_read(unsigned size, uint64_t offs, uint64_t val) " size %d offs 0x%02" PRIx64 " -> 0x%02" PRIx64
-sh_serial_write(unsigned size, uint64_t offs, uint64_t val) "size %d offs 0x%02" PRIx64 " <- 0x%02" PRIx64
+sh_serial_read(char *id, unsigned size, uint64_t offs, uint64_t val) " %s size %d offs 0x%02" PRIx64 " -> 0x%02" PRIx64
+sh_serial_write(char *id, unsigned size, uint64_t offs, uint64_t val) "%s size %d offs 0x%02" PRIx64 " <- 0x%02" PRIx64
-- 
2.21.4

Re: [PATCH v2 07/11] hw/char/sh_serial: Add device id to trace output

[Philippe Mathieu-Daudé]

On 10/27/21 15:46, BALATON Zoltan wrote:
> Normally there are at least two sh_serial instances. Add device id to
> trace messages to make it clear which instance they belong to
> otherwise its not possible to tell which serial device is accessed.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/char/sh_serial.c  | 6 ++++--
>  hw/char/trace-events | 4 ++--
>  2 files changed, 6 insertions(+), 4 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH v2 10/11] hw/intc/sh_intc: Clean up iomem region

[BALATON Zoltan]

Fix the size of the iomem region and rename it to "intc" from
"interrupt-controller" which makes the info mtree output less wide as
it is already too wide because of all the aliases. Also drop the
format macro which was only used twice in close proximity so we can
just use the literal string instead without a macro definition.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/intc/sh_intc.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
index 18461ff554..fc1905f299 100644
--- a/hw/intc/sh_intc.c
+++ b/hw/intc/sh_intc.c
@@ -288,15 +288,13 @@ static unsigned int sh_intc_register(MemoryRegion *sysmem,
     iomem_p4 = desc->iomem_aliases + index;
     iomem_a7 = iomem_p4 + 1;
 
-#define SH_INTC_IOMEM_FORMAT "interrupt-controller-%s-%s-%s"
-    snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "p4");
+    snprintf(name, sizeof(name), "intc-%s-%s-%s", type, action, "p4");
     memory_region_init_alias(iomem_p4, NULL, name, iomem, A7ADDR(address), 4);
     memory_region_add_subregion(sysmem, P4ADDR(address), iomem_p4);
 
-    snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "a7");
+    snprintf(name, sizeof(name), "intc-%s-%s-%s", type, action, "a7");
     memory_region_init_alias(iomem_a7, NULL, name, iomem, A7ADDR(address), 4);
     memory_region_add_subregion(sysmem, A7ADDR(address), iomem_a7);
-#undef SH_INTC_IOMEM_FORMAT
 
     /* used to increment aliases index */
     return 2;
@@ -432,9 +430,7 @@ int sh_intc_init(MemoryRegion *sysmem,
     }
 
     desc->irqs = qemu_allocate_irqs(sh_intc_set_irq, desc, nr_sources);
-
-    memory_region_init_io(&desc->iomem, NULL, &sh_intc_ops, desc,
-                          "interrupt-controller", 0x100000000ULL);
+    memory_region_init_io(&desc->iomem, NULL, &sh_intc_ops, desc, "intc", 4);
 
 #define INT_REG_PARAMS(reg_struct, type, action, j) \
         reg_struct->action##_reg, #type, #action, j
-- 
2.21.4

Re: [PATCH v2 10/11] hw/intc/sh_intc: Clean up iomem region

[Philippe Mathieu-Daudé]

On 10/27/21 15:46, BALATON Zoltan wrote:
> Fix the size of the iomem region and rename it to "intc" from
> "interrupt-controller" which makes the info mtree output less wide as
> it is already too wide because of all the aliases. Also drop the
> format macro which was only used twice in close proximity so we can
> just use the literal string instead without a macro definition.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/intc/sh_intc.c | 10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)

> -    memory_region_init_io(&desc->iomem, NULL, &sh_intc_ops, desc,
> -                          "interrupt-controller", 0x100000000ULL);
> +    memory_region_init_io(&desc->iomem, NULL, &sh_intc_ops, desc, "intc", 4);

Why the region size change from 4GB -> 4B? Did you mean '4 * GiB'?

Re: [PATCH v2 10/11] hw/intc/sh_intc: Clean up iomem region

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1455 bytes --]

On Wed, 27 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/27/21 15:46, BALATON Zoltan wrote:
>> Fix the size of the iomem region and rename it to "intc" from
>> "interrupt-controller" which makes the info mtree output less wide as
>> it is already too wide because of all the aliases. Also drop the
>> format macro which was only used twice in close proximity so we can
>> just use the literal string instead without a macro definition.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/intc/sh_intc.c | 10 +++-------
>>  1 file changed, 3 insertions(+), 7 deletions(-)
>
>> -    memory_region_init_io(&desc->iomem, NULL, &sh_intc_ops, desc,
>> -                          "interrupt-controller", 0x100000000ULL);
>> +    memory_region_init_io(&desc->iomem, NULL, &sh_intc_ops, desc, "intc", 4);
>
> Why the region size change from 4GB -> 4B? Did you mean '4 * GiB'?

No, it's really just 4 bytes, like the sh_serial region is 0x28 bytes but 
previously these were unnecessarily allocated as 4GB and then mapped in 
sysmem via the small 4 byte (or 0x28 byte for sh_serial) alias regions 
only. So we don't actually need these to be 4GB as there's nothing beyond 
the actual length so just declare them the necessary size. (I'm thinking 
maybe later we can drop one of the P4 or A7 alias and map the actual iomem 
at one of these directly and use an alias for the other but that's a 
separate clean up later.)

Regards,
BALATON Zoltan

[PATCH v2 09/11] hw/intc/sh_intc: Turn some defines into an enum

[BALATON Zoltan]

Turn the INTC_MODE defines into an enum (except the one which is a
flag) and clean up the function returning these to make it clearer by
removing nested ifs and superfluous parenthesis.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/intc/sh_intc.c | 43 +++++++++++++++++++------------------------
 1 file changed, 19 insertions(+), 24 deletions(-)

diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
index 0bd27aaf4f..18461ff554 100644
--- a/hw/intc/sh_intc.c
+++ b/hw/intc/sh_intc.c
@@ -100,33 +100,27 @@ int sh_intc_get_pending_vector(struct intc_desc *desc, int imask)
     abort();
 }
 
-#define INTC_MODE_NONE       0
-#define INTC_MODE_DUAL_SET   1
-#define INTC_MODE_DUAL_CLR   2
-#define INTC_MODE_ENABLE_REG 3
-#define INTC_MODE_MASK_REG   4
-#define INTC_MODE_IS_PRIO    8
-
-static unsigned int sh_intc_mode(unsigned long address,
-                                 unsigned long set_reg, unsigned long clr_reg)
+#define INTC_MODE_IS_PRIO 0x80
+typedef enum {
+    INTC_MODE_NONE,
+    INTC_MODE_DUAL_SET,
+    INTC_MODE_DUAL_CLR,
+    INTC_MODE_ENABLE_REG,
+    INTC_MODE_MASK_REG,
+} SHIntCMode;
+
+
+static SHIntCMode sh_intc_mode(unsigned long address, unsigned long set_reg,
+                               unsigned long clr_reg)
 {
-    if ((address != A7ADDR(set_reg)) &&
-        (address != A7ADDR(clr_reg)))
+    if (address != A7ADDR(set_reg) && address != A7ADDR(clr_reg)) {
         return INTC_MODE_NONE;
-
-    if (set_reg && clr_reg) {
-        if (address == A7ADDR(set_reg)) {
-            return INTC_MODE_DUAL_SET;
-        } else {
-            return INTC_MODE_DUAL_CLR;
-        }
     }
-
-    if (set_reg) {
-        return INTC_MODE_ENABLE_REG;
-    } else {
-        return INTC_MODE_MASK_REG;
+    if (set_reg && clr_reg) {
+        return address == A7ADDR(set_reg) ?
+               INTC_MODE_DUAL_SET : INTC_MODE_DUAL_CLR;
     }
+    return set_reg ? INTC_MODE_ENABLE_REG : INTC_MODE_MASK_REG;
 }
 
 static void sh_intc_locate(struct intc_desc *desc,
@@ -137,7 +131,8 @@ static void sh_intc_locate(struct intc_desc *desc,
                            unsigned int *width,
                            unsigned int *modep)
 {
-    unsigned int i, mode;
+    SHIntCMode mode;
+    unsigned int i;
 
     /* this is slow but works for now */
 
-- 
2.21.4

Re: [PATCH v2 09/11] hw/intc/sh_intc: Turn some defines into an enum

[Philippe Mathieu-Daudé]

On 10/27/21 15:46, BALATON Zoltan wrote:
> Turn the INTC_MODE defines into an enum (except the one which is a
> flag) and clean up the function returning these to make it clearer by
> removing nested ifs and superfluous parenthesis.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/intc/sh_intc.c | 43 +++++++++++++++++++------------------------
>  1 file changed, 19 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
> index 0bd27aaf4f..18461ff554 100644
> --- a/hw/intc/sh_intc.c
> +++ b/hw/intc/sh_intc.c
> @@ -100,33 +100,27 @@ int sh_intc_get_pending_vector(struct intc_desc *desc, int imask)
>      abort();
>  }
>  
> -#define INTC_MODE_NONE       0
> -#define INTC_MODE_DUAL_SET   1
> -#define INTC_MODE_DUAL_CLR   2
> -#define INTC_MODE_ENABLE_REG 3
> -#define INTC_MODE_MASK_REG   4
> -#define INTC_MODE_IS_PRIO    8
> -
> -static unsigned int sh_intc_mode(unsigned long address,
> -                                 unsigned long set_reg, unsigned long clr_reg)
> +#define INTC_MODE_IS_PRIO 0x80

Why change 8 -> 0x80 without updating the definition usage?

> +typedef enum {
> +    INTC_MODE_NONE,
> +    INTC_MODE_DUAL_SET,
> +    INTC_MODE_DUAL_CLR,
> +    INTC_MODE_ENABLE_REG,
> +    INTC_MODE_MASK_REG,

If this is defined by the spec, better explicit the enum value,
otherwise if someone add a misplaced field this would change the
definitions.

> +} SHIntCMode;
> +
> +
> +static SHIntCMode sh_intc_mode(unsigned long address, unsigned long set_reg,
> +                               unsigned long clr_reg)
>  {
> -    if ((address != A7ADDR(set_reg)) &&
> -        (address != A7ADDR(clr_reg)))
> +    if (address != A7ADDR(set_reg) && address != A7ADDR(clr_reg)) {
>          return INTC_MODE_NONE;
> -
> -    if (set_reg && clr_reg) {
> -        if (address == A7ADDR(set_reg)) {
> -            return INTC_MODE_DUAL_SET;
> -        } else {
> -            return INTC_MODE_DUAL_CLR;
> -        }
>      }
> -
> -    if (set_reg) {
> -        return INTC_MODE_ENABLE_REG;
> -    } else {
> -        return INTC_MODE_MASK_REG;
> +    if (set_reg && clr_reg) {
> +        return address == A7ADDR(set_reg) ?
> +               INTC_MODE_DUAL_SET : INTC_MODE_DUAL_CLR;
>      }
> +    return set_reg ? INTC_MODE_ENABLE_REG : INTC_MODE_MASK_REG;
>  }
>  
>  static void sh_intc_locate(struct intc_desc *desc,
> @@ -137,7 +131,8 @@ static void sh_intc_locate(struct intc_desc *desc,
>                             unsigned int *width,
>                             unsigned int *modep)
>  {
> -    unsigned int i, mode;
> +    SHIntCMode mode;
> +    unsigned int i;
>  
>      /* this is slow but works for now */
>  
> 

Re: [PATCH v2 09/11] hw/intc/sh_intc: Turn some defines into an enum

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3219 bytes --]

On Wed, 27 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/27/21 15:46, BALATON Zoltan wrote:
>> Turn the INTC_MODE defines into an enum (except the one which is a
>> flag) and clean up the function returning these to make it clearer by
>> removing nested ifs and superfluous parenthesis.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/intc/sh_intc.c | 43 +++++++++++++++++++------------------------
>>  1 file changed, 19 insertions(+), 24 deletions(-)
>>
>> diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
>> index 0bd27aaf4f..18461ff554 100644
>> --- a/hw/intc/sh_intc.c
>> +++ b/hw/intc/sh_intc.c
>> @@ -100,33 +100,27 @@ int sh_intc_get_pending_vector(struct intc_desc *desc, int imask)
>>      abort();
>>  }
>>
>> -#define INTC_MODE_NONE       0
>> -#define INTC_MODE_DUAL_SET   1
>> -#define INTC_MODE_DUAL_CLR   2
>> -#define INTC_MODE_ENABLE_REG 3
>> -#define INTC_MODE_MASK_REG   4
>> -#define INTC_MODE_IS_PRIO    8
>> -
>> -static unsigned int sh_intc_mode(unsigned long address,
>> -                                 unsigned long set_reg, unsigned long clr_reg)
>> +#define INTC_MODE_IS_PRIO 0x80
>
> Why change 8 -> 0x80 without updating the definition usage?

To better separate it from the enum values as these are OR-ed together 
later so just leave some spare bits inbetween. Should this be a separate 
one line patch or mention it in the commit message?

>> +typedef enum {
>> +    INTC_MODE_NONE,
>> +    INTC_MODE_DUAL_SET,
>> +    INTC_MODE_DUAL_CLR,
>> +    INTC_MODE_ENABLE_REG,
>> +    INTC_MODE_MASK_REG,
>
> If this is defined by the spec, better explicit the enum value,
> otherwise if someone add a misplaced field this would change the
> definitions.

I don't know. It didn't occur to me it could be part of the arch, looked 
more like an implementation detail but have to check the docs if anything 
similar is mentioned.

Regards,
BALATON Zoltan

>> +} SHIntCMode;
>> +
>> +
>> +static SHIntCMode sh_intc_mode(unsigned long address, unsigned long set_reg,
>> +                               unsigned long clr_reg)
>>  {
>> -    if ((address != A7ADDR(set_reg)) &&
>> -        (address != A7ADDR(clr_reg)))
>> +    if (address != A7ADDR(set_reg) && address != A7ADDR(clr_reg)) {
>>          return INTC_MODE_NONE;
>> -
>> -    if (set_reg && clr_reg) {
>> -        if (address == A7ADDR(set_reg)) {
>> -            return INTC_MODE_DUAL_SET;
>> -        } else {
>> -            return INTC_MODE_DUAL_CLR;
>> -        }
>>      }
>> -
>> -    if (set_reg) {
>> -        return INTC_MODE_ENABLE_REG;
>> -    } else {
>> -        return INTC_MODE_MASK_REG;
>> +    if (set_reg && clr_reg) {
>> +        return address == A7ADDR(set_reg) ?
>> +               INTC_MODE_DUAL_SET : INTC_MODE_DUAL_CLR;
>>      }
>> +    return set_reg ? INTC_MODE_ENABLE_REG : INTC_MODE_MASK_REG;
>>  }
>>
>>  static void sh_intc_locate(struct intc_desc *desc,
>> @@ -137,7 +131,8 @@ static void sh_intc_locate(struct intc_desc *desc,
>>                             unsigned int *width,
>>                             unsigned int *modep)
>>  {
>> -    unsigned int i, mode;
>> +    SHIntCMode mode;
>> +    unsigned int i;
>>
>>      /* this is slow but works for now */
>>
>>
>
>

[PATCH v2 04/11] hw/sh4/r2d: Use error_report instead of fprintf to stderr

[BALATON Zoltan]

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/sh4/r2d.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/sh4/r2d.c b/hw/sh4/r2d.c
index 57ccae7249..72759413f3 100644
--- a/hw/sh4/r2d.c
+++ b/hw/sh4/r2d.c
@@ -26,6 +26,7 @@
 #include "qemu/osdep.h"
 #include "qemu/units.h"
 #include "qapi/error.h"
+#include "qemu/error-report.h"
 #include "cpu.h"
 #include "hw/sysbus.h"
 #include "hw/sh4/sh.h"
@@ -324,7 +325,7 @@ static void r2d_init(MachineState *machine)
                                           SDRAM_BASE + LINUX_LOAD_OFFSET,
                                           INITRD_LOAD_OFFSET - LINUX_LOAD_OFFSET);
         if (kernel_size < 0) {
-            fprintf(stderr, "qemu: could not load kernel '%s'\n", kernel_filename);
+            error_report("qemu: could not load kernel '%s'", kernel_filename);
             exit(1);
         }
 
@@ -345,7 +346,7 @@ static void r2d_init(MachineState *machine)
                                           SDRAM_SIZE - INITRD_LOAD_OFFSET);
 
         if (initrd_size < 0) {
-            fprintf(stderr, "qemu: could not load initrd '%s'\n", initrd_filename);
+            error_report("qemu: could not load initrd '%s'", initrd_filename);
             exit(1);
         }
 
-- 
2.21.4

Re: [PATCH v2 04/11] hw/sh4/r2d: Use error_report instead of fprintf to stderr

[Philippe Mathieu-Daudé]

On 10/27/21 15:46, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  hw/sh4/r2d.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH v2 01/11] hw/sh4: Fix a typo in a comment

[BALATON Zoltan]

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/timer/sh_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/timer/sh_timer.c b/hw/timer/sh_timer.c
index 02eb865908..0a18ac8276 100644
--- a/hw/timer/sh_timer.c
+++ b/hw/timer/sh_timer.c
@@ -107,7 +107,7 @@ static void sh_timer_write(void *opaque, hwaddr offset,
         if (s->enabled) {
             /*
              * Pause the timer if it is running. This may cause some inaccuracy
-             * dure to rounding, but avoids a whole lot of other messyness
+             * due to rounding, but avoids a whole lot of other messyness
              */
             ptimer_stop(s->timer);
         }
-- 
2.21.4

Re: [PATCH v2 01/11] hw/sh4: Fix a typo in a comment

[Philippe Mathieu-Daudé]

On 10/27/21 15:46, BALATON Zoltan wrote:
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  hw/timer/sh_timer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/timer/sh_timer.c b/hw/timer/sh_timer.c
> index 02eb865908..0a18ac8276 100644
> --- a/hw/timer/sh_timer.c
> +++ b/hw/timer/sh_timer.c
> @@ -107,7 +107,7 @@ static void sh_timer_write(void *opaque, hwaddr offset,
>          if (s->enabled) {
>              /*
>               * Pause the timer if it is running. This may cause some inaccuracy
> -             * dure to rounding, but avoids a whole lot of other messyness
> +             * due to rounding, but avoids a whole lot of other messyness

Also "messiness", otherwise:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

>               */
>              ptimer_stop(s->timer);
>          }
> 

Re: [PATCH v2 01/11] hw/sh4: Fix a typo in a comment

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1109 bytes --]

On Wed, 27 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/27/21 15:46, BALATON Zoltan wrote:
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>  hw/timer/sh_timer.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/timer/sh_timer.c b/hw/timer/sh_timer.c
>> index 02eb865908..0a18ac8276 100644
>> --- a/hw/timer/sh_timer.c
>> +++ b/hw/timer/sh_timer.c
>> @@ -107,7 +107,7 @@ static void sh_timer_write(void *opaque, hwaddr offset,
>>          if (s->enabled) {
>>              /*
>>               * Pause the timer if it is running. This may cause some inaccuracy
>> -             * dure to rounding, but avoids a whole lot of other messyness
>> +             * due to rounding, but avoids a whole lot of other messyness
>
> Also "messiness", otherwise:

I wasn't sure that's a word but appears in a dictionary so I'll change 
that too.

Regards,
BALATON Zoltan

> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
>>               */
>>              ptimer_stop(s->timer);
>>          }
>>
>
>

[PATCH v2 06/11] hw/char/sh_serial: QOM-ify

[BALATON Zoltan]

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/char/sh_serial.c | 107 +++++++++++++++++++++++++++-----------------
 hw/sh4/sh7750.c     |  62 ++++++++++++++++++-------
 include/hw/sh4/sh.h |   9 +---
 3 files changed, 114 insertions(+), 64 deletions(-)

diff --git a/hw/char/sh_serial.c b/hw/char/sh_serial.c
index 6d02e0ad11..ad576b693b 100644
--- a/hw/char/sh_serial.c
+++ b/hw/char/sh_serial.c
@@ -26,7 +26,11 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/sysbus.h"
 #include "hw/irq.h"
+#include "hw/qdev-core.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
 #include "hw/sh4/sh.h"
 #include "chardev/char-fe.h"
 #include "qapi/error.h"
@@ -42,10 +46,10 @@
 
 #define SH_RX_FIFO_LENGTH (16)
 
-typedef struct {
-    MemoryRegion iomem;
-    MemoryRegion iomem_p4;
-    MemoryRegion iomem_a7;
+OBJECT_DECLARE_SIMPLE_TYPE(SHSerialState, SH_SERIAL)
+
+struct SHSerialState {
+    SysBusDevice parent;
     uint8_t smr;
     uint8_t brr;
     uint8_t scr;
@@ -59,13 +63,12 @@ typedef struct {
     uint8_t rx_tail;
     uint8_t rx_head;
 
-    int freq;
-    int feat;
+    uint8_t feat;
     int flags;
     int rtrg;
 
     CharBackend chr;
-    QEMUTimer *fifo_timeout_timer;
+    QEMUTimer fifo_timeout_timer;
     uint64_t etu; /* Elementary Time Unit (ns) */
 
     qemu_irq eri;
@@ -73,7 +76,11 @@ typedef struct {
     qemu_irq txi;
     qemu_irq tei;
     qemu_irq bri;
-} SHSerialState;
+};
+
+typedef struct {} SHSerialStateClass;
+
+OBJECT_DEFINE_TYPE(SHSerialState, sh_serial, SH_SERIAL, SYS_BUS_DEVICE)
 
 static void sh_serial_clear_fifo(SHSerialState *s)
 {
@@ -353,11 +360,11 @@ static void sh_serial_receive1(void *opaque, const uint8_t *buf, int size)
                 if (s->rx_cnt >= s->rtrg) {
                     s->flags |= SH_SERIAL_FLAG_RDF;
                     if (s->scr & (1 << 6) && s->rxi) {
-                        timer_del(s->fifo_timeout_timer);
+                        timer_del(&s->fifo_timeout_timer);
                         qemu_set_irq(s->rxi, 1);
                     }
                 } else {
-                    timer_mod(s->fifo_timeout_timer,
+                    timer_mod(&s->fifo_timeout_timer,
                         qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 15 * s->etu);
                 }
             }
@@ -381,18 +388,10 @@ static const MemoryRegionOps sh_serial_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-void sh_serial_init(MemoryRegion *sysmem,
-                    hwaddr base, int feat,
-                    uint32_t freq, Chardev *chr,
-                    qemu_irq eri_source,
-                    qemu_irq rxi_source,
-                    qemu_irq txi_source,
-                    qemu_irq tei_source,
-                    qemu_irq bri_source)
+static void sh_serial_reset(DeviceState *dev)
 {
-    SHSerialState *s = g_malloc0(sizeof(*s));
+    SHSerialState *s = SH_SERIAL(dev);
 
-    s->feat = feat;
     s->flags = SH_SERIAL_FLAG_TEND | SH_SERIAL_FLAG_TDE;
     s->rtrg = 1;
 
@@ -401,38 +400,64 @@ void sh_serial_init(MemoryRegion *sysmem,
     s->scr = 1 << 5; /* pretend that TX is enabled so early printk works */
     s->sptr = 0;
 
-    if (feat & SH_SERIAL_FEAT_SCIF) {
+    if (s->feat & SH_SERIAL_FEAT_SCIF) {
         s->fcr = 0;
     } else {
         s->dr = 0xff;
     }
 
     sh_serial_clear_fifo(s);
+}
 
-    memory_region_init_io(&s->iomem, NULL, &sh_serial_ops, s,
-                          "serial", 0x100000000ULL);
-
-    memory_region_init_alias(&s->iomem_p4, NULL, "serial-p4", &s->iomem,
-                             0, 0x28);
-    memory_region_add_subregion(sysmem, P4ADDR(base), &s->iomem_p4);
-
-    memory_region_init_alias(&s->iomem_a7, NULL, "serial-a7", &s->iomem,
-                             0, 0x28);
-    memory_region_add_subregion(sysmem, A7ADDR(base), &s->iomem_a7);
-
-    if (chr) {
-        qemu_chr_fe_init(&s->chr, chr, &error_abort);
+static void sh_serial_realize(DeviceState *d, Error **errp)
+{
+    SHSerialState *s = SH_SERIAL(d);
+    MemoryRegion *iomem = g_malloc(sizeof(*iomem));
+
+    assert(d->id);
+    memory_region_init_io(iomem, OBJECT(d), &sh_serial_ops, s, d->id, 0x28);
+    sysbus_init_mmio(SYS_BUS_DEVICE(d), iomem);
+    qdev_init_gpio_out_named(d, &s->eri, "eri", 1);
+    qdev_init_gpio_out_named(d, &s->rxi, "rxi", 1);
+    qdev_init_gpio_out_named(d, &s->txi, "txi", 1);
+    qdev_init_gpio_out_named(d, &s->tei, "tei", 1);
+    qdev_init_gpio_out_named(d, &s->bri, "bri", 1);
+
+    if (qemu_chr_fe_backend_connected(&s->chr)) {
         qemu_chr_fe_set_handlers(&s->chr, sh_serial_can_receive1,
                                  sh_serial_receive1,
                                  sh_serial_event, NULL, s, NULL, true);
     }
 
-    s->fifo_timeout_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
-                                         sh_serial_timeout_int, s);
+    timer_init_ns(&s->fifo_timeout_timer, QEMU_CLOCK_VIRTUAL,
+                  sh_serial_timeout_int, s);
     s->etu = NANOSECONDS_PER_SECOND / 9600;
-    s->eri = eri_source;
-    s->rxi = rxi_source;
-    s->txi = txi_source;
-    s->tei = tei_source;
-    s->bri = bri_source;
+}
+
+static void sh_serial_finalize(Object *obj)
+{
+    SHSerialState *s = SH_SERIAL(obj);
+
+    timer_del(&s->fifo_timeout_timer);
+}
+
+static void sh_serial_init(Object *obj)
+{
+}
+
+static Property sh_serial_properties[] = {
+    DEFINE_PROP_CHR("chardev", SHSerialState, chr),
+    DEFINE_PROP_UINT8("features", SHSerialState, feat, 0),
+    DEFINE_PROP_END_OF_LIST()
+};
+
+static void sh_serial_class_init(ObjectClass *oc, void *data)
+{
+    DeviceClass *dc = DEVICE_CLASS(oc);
+
+    device_class_set_props(dc, sh_serial_properties);
+    dc->realize = sh_serial_realize;
+    dc->reset = sh_serial_reset;
+    /* Reason: part of SuperH CPU/SoC, needs to be wired up */
+    dc->user_creatable = false;
 }
diff --git a/hw/sh4/sh7750.c b/hw/sh4/sh7750.c
index c3c3caf952..dba40a6fb4 100644
--- a/hw/sh4/sh7750.c
+++ b/hw/sh4/sh7750.c
@@ -24,10 +24,14 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.h"
 #include "qemu/log.h"
+#include "hw/sysbus.h"
 #include "hw/irq.h"
 #include "hw/sh4/sh.h"
 #include "sysemu/sysemu.h"
+#include "hw/qdev-properties.h"
+#include "hw/qdev-properties-system.h"
 #include "sh7750_regs.h"
 #include "sh7750_regnames.h"
 #include "hw/sh4/sh_intc.h"
@@ -761,6 +765,9 @@ static const MemoryRegionOps sh7750_mmct_ops = {
 SH7750State *sh7750_init(SuperHCPU *cpu, MemoryRegion *sysmem)
 {
     SH7750State *s;
+    DeviceState *dev;
+    SysBusDevice *sb;
+    MemoryRegion *mr, *alias;
 
     s = g_malloc0(sizeof(SH7750State));
     s->cpu = cpu;
@@ -806,21 +813,46 @@ SH7750State *sh7750_init(SuperHCPU *cpu, MemoryRegion *sysmem)
 
     cpu->env.intc_handle = &s->intc;
 
-    sh_serial_init(sysmem, 0x1fe00000,
-                   0, s->periph_freq, serial_hd(0),
-                   s->intc.irqs[SCI1_ERI],
-                   s->intc.irqs[SCI1_RXI],
-                   s->intc.irqs[SCI1_TXI],
-                   s->intc.irqs[SCI1_TEI],
-                   NULL);
-    sh_serial_init(sysmem, 0x1fe80000,
-                   SH_SERIAL_FEAT_SCIF,
-                   s->periph_freq, serial_hd(1),
-                   s->intc.irqs[SCIF_ERI],
-                   s->intc.irqs[SCIF_RXI],
-                   s->intc.irqs[SCIF_TXI],
-                   NULL,
-                   s->intc.irqs[SCIF_BRI]);
+    /* SCI */
+    dev = qdev_new(TYPE_SH_SERIAL);
+    dev->id = (char *)"sci";
+    qdev_prop_set_chr(dev, "chardev", serial_hd(0));
+    sb = SYS_BUS_DEVICE(dev);
+    sysbus_realize_and_unref(sb, &error_fatal);
+    mr = sysbus_mmio_get_region(sb, 0);
+    alias = g_malloc(sizeof(*alias));
+    memory_region_init_alias(alias, OBJECT(dev), "sci-p4", mr,
+                             0, memory_region_size(mr));
+    memory_region_add_subregion(sysmem, P4ADDR(0x1fe00000), alias);
+    alias = g_malloc(sizeof(*alias));
+    memory_region_init_alias(alias, OBJECT(dev), "sci-a7", mr,
+                             0, memory_region_size(mr));
+    memory_region_add_subregion(sysmem, A7ADDR(0x1fe00000), alias);
+    qdev_connect_gpio_out_named(dev, "eri", 0, s->intc.irqs[SCI1_ERI]);
+    qdev_connect_gpio_out_named(dev, "rxi", 0, s->intc.irqs[SCI1_RXI]);
+    qdev_connect_gpio_out_named(dev, "txi", 0, s->intc.irqs[SCI1_TXI]);
+    qdev_connect_gpio_out_named(dev, "tei", 0, s->intc.irqs[SCI1_TEI]);
+
+    /* SCIF */
+    dev = qdev_new(TYPE_SH_SERIAL);
+    dev->id =  (char *)"scif";
+    qdev_prop_set_chr(dev, "chardev", serial_hd(1));
+    qdev_prop_set_uint8(dev, "features", SH_SERIAL_FEAT_SCIF);
+    sb = SYS_BUS_DEVICE(dev);
+    sysbus_realize_and_unref(sb, &error_fatal);
+    mr = sysbus_mmio_get_region(sb, 0);
+    alias = g_malloc(sizeof(*alias));
+    memory_region_init_alias(alias, OBJECT(dev), "scif-p4", mr,
+                             0, memory_region_size(mr));
+    memory_region_add_subregion(sysmem, P4ADDR(0x1fe80000), alias);
+    alias = g_malloc(sizeof(*alias));
+    memory_region_init_alias(alias, OBJECT(dev), "scif-a7", mr,
+                             0, memory_region_size(mr));
+    memory_region_add_subregion(sysmem, A7ADDR(0x1fe80000), alias);
+    qdev_connect_gpio_out_named(dev, "eri", 0, s->intc.irqs[SCIF_ERI]);
+    qdev_connect_gpio_out_named(dev, "rxi", 0, s->intc.irqs[SCIF_RXI]);
+    qdev_connect_gpio_out_named(dev, "txi", 0, s->intc.irqs[SCIF_TXI]);
+    qdev_connect_gpio_out_named(dev, "bri", 0, s->intc.irqs[SCIF_BRI]);
 
     tmu012_init(sysmem, 0x1fd80000,
                 TMU012_FEAT_TOCR | TMU012_FEAT_3CHAN | TMU012_FEAT_EXTCLK,
diff --git a/include/hw/sh4/sh.h b/include/hw/sh4/sh.h
index 366cedcda0..ec716cdd45 100644
--- a/include/hw/sh4/sh.h
+++ b/include/hw/sh4/sh.h
@@ -54,15 +54,8 @@ int sh7750_register_io_device(struct SH7750State *s,
                               sh7750_io_device *device);
 
 /* sh_serial.c */
+#define TYPE_SH_SERIAL "sh-serial"
 #define SH_SERIAL_FEAT_SCIF (1 << 0)
-void sh_serial_init(MemoryRegion *sysmem,
-                    hwaddr base, int feat,
-                    uint32_t freq, Chardev *chr,
-                    qemu_irq eri_source,
-                    qemu_irq rxi_source,
-                    qemu_irq txi_source,
-                    qemu_irq tei_source,
-                    qemu_irq bri_source);
 
 /* sh7750.c */
 qemu_irq sh7750_irl(struct SH7750State *s);
-- 
2.21.4

[PATCH v2 08/11] hw/intc/sh_intc: Use existing macro instead of local one

[BALATON Zoltan]

The INTC_A7 local macro does the same as the A7ADDR from
include/sh/sh.h so use the latter and drop the local macro definiion.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/intc/sh_intc.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
index c1058d97c0..0bd27aaf4f 100644
--- a/hw/intc/sh_intc.c
+++ b/hw/intc/sh_intc.c
@@ -16,8 +16,6 @@
 #include "hw/sh4/sh.h"
 #include "trace.h"
 
-#define INTC_A7(x) ((x) & 0x1fffffff)
-
 void sh_intc_toggle_source(struct intc_source *source,
                            int enable_adj, int assert_adj)
 {
@@ -112,12 +110,12 @@ int sh_intc_get_pending_vector(struct intc_desc *desc, int imask)
 static unsigned int sh_intc_mode(unsigned long address,
                                  unsigned long set_reg, unsigned long clr_reg)
 {
-    if ((address != INTC_A7(set_reg)) &&
-        (address != INTC_A7(clr_reg)))
+    if ((address != A7ADDR(set_reg)) &&
+        (address != A7ADDR(clr_reg)))
         return INTC_MODE_NONE;
 
     if (set_reg && clr_reg) {
-        if (address == INTC_A7(set_reg)) {
+        if (address == A7ADDR(set_reg)) {
             return INTC_MODE_DUAL_SET;
         } else {
             return INTC_MODE_DUAL_CLR;
@@ -297,11 +295,11 @@ static unsigned int sh_intc_register(MemoryRegion *sysmem,
 
 #define SH_INTC_IOMEM_FORMAT "interrupt-controller-%s-%s-%s"
     snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "p4");
-    memory_region_init_alias(iomem_p4, NULL, name, iomem, INTC_A7(address), 4);
+    memory_region_init_alias(iomem_p4, NULL, name, iomem, A7ADDR(address), 4);
     memory_region_add_subregion(sysmem, P4ADDR(address), iomem_p4);
 
     snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "a7");
-    memory_region_init_alias(iomem_a7, NULL, name, iomem, INTC_A7(address), 4);
+    memory_region_init_alias(iomem_a7, NULL, name, iomem, A7ADDR(address), 4);
     memory_region_add_subregion(sysmem, A7ADDR(address), iomem_a7);
 #undef SH_INTC_IOMEM_FORMAT
 
-- 
2.21.4

Re: [PATCH v2 08/11] hw/intc/sh_intc: Use existing macro instead of local one

[Philippe Mathieu-Daudé]

On 10/27/21 15:46, BALATON Zoltan wrote:
> The INTC_A7 local macro does the same as the A7ADDR from
> include/sh/sh.h so use the latter and drop the local macro definiion.

Typo "definition".

> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/intc/sh_intc.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
> index c1058d97c0..0bd27aaf4f 100644
> --- a/hw/intc/sh_intc.c
> +++ b/hw/intc/sh_intc.c
> @@ -16,8 +16,6 @@
>  #include "hw/sh4/sh.h"
>  #include "trace.h"
>  
> -#define INTC_A7(x) ((x) & 0x1fffffff)
> -
>  void sh_intc_toggle_source(struct intc_source *source,
>                             int enable_adj, int assert_adj)
>  {
> @@ -112,12 +110,12 @@ int sh_intc_get_pending_vector(struct intc_desc *desc, int imask)
>  static unsigned int sh_intc_mode(unsigned long address,
>                                   unsigned long set_reg, unsigned long clr_reg)
>  {
> -    if ((address != INTC_A7(set_reg)) &&
> -        (address != INTC_A7(clr_reg)))
> +    if ((address != A7ADDR(set_reg)) &&
> +        (address != A7ADDR(clr_reg)))
>          return INTC_MODE_NONE;
>  
>      if (set_reg && clr_reg) {
> -        if (address == INTC_A7(set_reg)) {
> +        if (address == A7ADDR(set_reg)) {
>              return INTC_MODE_DUAL_SET;
>          } else {
>              return INTC_MODE_DUAL_CLR;
> @@ -297,11 +295,11 @@ static unsigned int sh_intc_register(MemoryRegion *sysmem,
>  
>  #define SH_INTC_IOMEM_FORMAT "interrupt-controller-%s-%s-%s"
>      snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "p4");
> -    memory_region_init_alias(iomem_p4, NULL, name, iomem, INTC_A7(address), 4);
> +    memory_region_init_alias(iomem_p4, NULL, name, iomem, A7ADDR(address), 4);
>      memory_region_add_subregion(sysmem, P4ADDR(address), iomem_p4);
>  
>      snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "a7");
> -    memory_region_init_alias(iomem_a7, NULL, name, iomem, INTC_A7(address), 4);
> +    memory_region_init_alias(iomem_a7, NULL, name, iomem, A7ADDR(address), 4);

I wonder why the address is masked out... It looks there is a mismatch
in the memory region mapping. Anyway this predates this cleanup, so:

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2 08/11] hw/intc/sh_intc: Use existing macro instead of local one

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2675 bytes --]

On Wed, 27 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/27/21 15:46, BALATON Zoltan wrote:
>> The INTC_A7 local macro does the same as the A7ADDR from
>> include/sh/sh.h so use the latter and drop the local macro definiion.
>
> Typo "definition".
>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/intc/sh_intc.c | 12 +++++-------
>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
>> index c1058d97c0..0bd27aaf4f 100644
>> --- a/hw/intc/sh_intc.c
>> +++ b/hw/intc/sh_intc.c
>> @@ -16,8 +16,6 @@
>>  #include "hw/sh4/sh.h"
>>  #include "trace.h"
>>
>> -#define INTC_A7(x) ((x) & 0x1fffffff)
>> -
>>  void sh_intc_toggle_source(struct intc_source *source,
>>                             int enable_adj, int assert_adj)
>>  {
>> @@ -112,12 +110,12 @@ int sh_intc_get_pending_vector(struct intc_desc *desc, int imask)
>>  static unsigned int sh_intc_mode(unsigned long address,
>>                                   unsigned long set_reg, unsigned long clr_reg)
>>  {
>> -    if ((address != INTC_A7(set_reg)) &&
>> -        (address != INTC_A7(clr_reg)))
>> +    if ((address != A7ADDR(set_reg)) &&
>> +        (address != A7ADDR(clr_reg)))
>>          return INTC_MODE_NONE;
>>
>>      if (set_reg && clr_reg) {
>> -        if (address == INTC_A7(set_reg)) {
>> +        if (address == A7ADDR(set_reg)) {
>>              return INTC_MODE_DUAL_SET;
>>          } else {
>>              return INTC_MODE_DUAL_CLR;
>> @@ -297,11 +295,11 @@ static unsigned int sh_intc_register(MemoryRegion *sysmem,
>>
>>  #define SH_INTC_IOMEM_FORMAT "interrupt-controller-%s-%s-%s"
>>      snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "p4");
>> -    memory_region_init_alias(iomem_p4, NULL, name, iomem, INTC_A7(address), 4);
>> +    memory_region_init_alias(iomem_p4, NULL, name, iomem, A7ADDR(address), 4);
>>      memory_region_add_subregion(sysmem, P4ADDR(address), iomem_p4);
>>
>>      snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, "a7");
>> -    memory_region_init_alias(iomem_a7, NULL, name, iomem, INTC_A7(address), 4);
>> +    memory_region_init_alias(iomem_a7, NULL, name, iomem, A7ADDR(address), 4);
>
> I wonder why the address is masked out... It looks there is a mismatch
> in the memory region mapping. Anyway this predates this cleanup, so:

This seems to be a peculiarity of the SH architecture. Like MIPS, it has 
some strange memory mapping conventions where same registers appear in 
different areas at predefined addresses. These macros just calculate that 
address.

Regards,
BALATON Zoltan

> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
>

Re: [PATCH v2 08/11] hw/intc/sh_intc: Use existing macro instead of local one

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2942 bytes --]

On Wed, 27 Oct 2021, BALATON Zoltan wrote:
> On Wed, 27 Oct 2021, Philippe Mathieu-Daudé wrote:
>> On 10/27/21 15:46, BALATON Zoltan wrote:
>>> The INTC_A7 local macro does the same as the A7ADDR from
>>> include/sh/sh.h so use the latter and drop the local macro definiion.
>> 
>> Typo "definition".
>> 
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>  hw/intc/sh_intc.c | 12 +++++-------
>>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>> 
>>> diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
>>> index c1058d97c0..0bd27aaf4f 100644
>>> --- a/hw/intc/sh_intc.c
>>> +++ b/hw/intc/sh_intc.c
>>> @@ -16,8 +16,6 @@
>>>  #include "hw/sh4/sh.h"
>>>  #include "trace.h"
>>> 
>>> -#define INTC_A7(x) ((x) & 0x1fffffff)
>>> -
>>>  void sh_intc_toggle_source(struct intc_source *source,
>>>                             int enable_adj, int assert_adj)
>>>  {
>>> @@ -112,12 +110,12 @@ int sh_intc_get_pending_vector(struct intc_desc 
>>> *desc, int imask)
>>>  static unsigned int sh_intc_mode(unsigned long address,
>>>                                   unsigned long set_reg, unsigned long 
>>> clr_reg)
>>>  {
>>> -    if ((address != INTC_A7(set_reg)) &&
>>> -        (address != INTC_A7(clr_reg)))
>>> +    if ((address != A7ADDR(set_reg)) &&
>>> +        (address != A7ADDR(clr_reg)))
>>>          return INTC_MODE_NONE;
>>>
>>>      if (set_reg && clr_reg) {
>>> -        if (address == INTC_A7(set_reg)) {
>>> +        if (address == A7ADDR(set_reg)) {
>>>              return INTC_MODE_DUAL_SET;
>>>          } else {
>>>              return INTC_MODE_DUAL_CLR;
>>> @@ -297,11 +295,11 @@ static unsigned int sh_intc_register(MemoryRegion 
>>> *sysmem,
>>>
>>>  #define SH_INTC_IOMEM_FORMAT "interrupt-controller-%s-%s-%s"
>>>      snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, 
>>> "p4");
>>> -    memory_region_init_alias(iomem_p4, NULL, name, iomem, 
>>> INTC_A7(address), 4);
>>> +    memory_region_init_alias(iomem_p4, NULL, name, iomem, 
>>> A7ADDR(address), 4);
>>>      memory_region_add_subregion(sysmem, P4ADDR(address), iomem_p4);
>>>
>>>      snprintf(name, sizeof(name), SH_INTC_IOMEM_FORMAT, type, action, 
>>> "a7");
>>> -    memory_region_init_alias(iomem_a7, NULL, name, iomem, 
>>> INTC_A7(address), 4);
>>> +    memory_region_init_alias(iomem_a7, NULL, name, iomem, 
>>> A7ADDR(address), 4);
>> 
>> I wonder why the address is masked out... It looks there is a mismatch
>> in the memory region mapping. Anyway this predates this cleanup, so:
>
> This seems to be a peculiarity of the SH architecture. Like MIPS, it has some 
> strange memory mapping conventions where same registers appear in different 
> areas at predefined addresses. These macros just calculate that address.

Hmm, on second look it creates alias at A7ADDR into the original region so 
maybe reducing the size from 4GB could break that. I'll have to check 
again what this is doing.

Regards,
BALATON Zoltan

[PATCH v2 11/11] hw/intc/sh_intc: Drop another useless macro

[BALATON Zoltan]

The INT_REG_PARAMS macro was only used a few times within one function
on adjacent lines and is actually more complex than writing out the
parameters so simplify it by expanding the macro at call sites and
dropping the #define.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/intc/sh_intc.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
index fc1905f299..d3616b0078 100644
--- a/hw/intc/sh_intc.c
+++ b/hw/intc/sh_intc.c
@@ -432,16 +432,12 @@ int sh_intc_init(MemoryRegion *sysmem,
     desc->irqs = qemu_allocate_irqs(sh_intc_set_irq, desc, nr_sources);
     memory_region_init_io(&desc->iomem, NULL, &sh_intc_ops, desc, "intc", 4);
 
-#define INT_REG_PARAMS(reg_struct, type, action, j) \
-        reg_struct->action##_reg, #type, #action, j
     if (desc->mask_regs) {
         for (i = 0; i < desc->nr_mask_regs; i++) {
             struct intc_mask_reg *mr = desc->mask_regs + i;
 
-            j += sh_intc_register(sysmem, desc,
-                                  INT_REG_PARAMS(mr, mask, set, j));
-            j += sh_intc_register(sysmem, desc,
-                                  INT_REG_PARAMS(mr, mask, clr, j));
+            j += sh_intc_register(sysmem, desc, mr->set_reg, "mask", "set", j);
+            j += sh_intc_register(sysmem, desc, mr->clr_reg, "mask", "clr", j);
         }
     }
 
@@ -449,13 +445,10 @@ int sh_intc_init(MemoryRegion *sysmem,
         for (i = 0; i < desc->nr_prio_regs; i++) {
             struct intc_prio_reg *pr = desc->prio_regs + i;
 
-            j += sh_intc_register(sysmem, desc,
-                                  INT_REG_PARAMS(pr, prio, set, j));
-            j += sh_intc_register(sysmem, desc,
-                                  INT_REG_PARAMS(pr, prio, clr, j));
+            j += sh_intc_register(sysmem, desc, pr->set_reg, "prio", "set", j);
+            j += sh_intc_register(sysmem, desc, pr->clr_reg, "prio", "clr", j);
         }
     }
-#undef INT_REG_PARAMS
 
     return 0;
 }
-- 
2.21.4

Re: [PATCH v2 11/11] hw/intc/sh_intc: Drop another useless macro

[Philippe Mathieu-Daudé]

On 10/27/21 15:46, BALATON Zoltan wrote:
> The INT_REG_PARAMS macro was only used a few times within one function
> on adjacent lines and is actually more complex than writing out the
> parameters so simplify it by expanding the macro at call sites and
> dropping the #define.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/intc/sh_intc.c | 15 ++++-----------
>  1 file changed, 4 insertions(+), 11 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

[PATCH v2 05/11] hw/char/sh_serial: Rename type sh_serial_state to SHSerialState

[BALATON Zoltan]

Coding style says types should be camel case.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/char/sh_serial.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/hw/char/sh_serial.c b/hw/char/sh_serial.c
index bbf7586892..6d02e0ad11 100644
--- a/hw/char/sh_serial.c
+++ b/hw/char/sh_serial.c
@@ -73,9 +73,9 @@ typedef struct {
     qemu_irq txi;
     qemu_irq tei;
     qemu_irq bri;
-} sh_serial_state;
+} SHSerialState;
 
-static void sh_serial_clear_fifo(sh_serial_state *s)
+static void sh_serial_clear_fifo(SHSerialState *s)
 {
     memset(s->rx_fifo, 0, SH_RX_FIFO_LENGTH);
     s->rx_cnt = 0;
@@ -86,7 +86,7 @@ static void sh_serial_clear_fifo(sh_serial_state *s)
 static void sh_serial_write(void *opaque, hwaddr offs,
                             uint64_t val, unsigned size)
 {
-    sh_serial_state *s = opaque;
+    SHSerialState *s = opaque;
     unsigned char ch;
 
     trace_sh_serial_write(size, offs, val);
@@ -205,7 +205,7 @@ static void sh_serial_write(void *opaque, hwaddr offs,
 static uint64_t sh_serial_read(void *opaque, hwaddr offs,
                                unsigned size)
 {
-    sh_serial_state *s = opaque;
+    SHSerialState *s = opaque;
     uint32_t ret = ~0;
 
 #if 0
@@ -309,12 +309,12 @@ static uint64_t sh_serial_read(void *opaque, hwaddr offs,
     return ret;
 }
 
-static int sh_serial_can_receive(sh_serial_state *s)
+static int sh_serial_can_receive(SHSerialState *s)
 {
     return s->scr & (1 << 4);
 }
 
-static void sh_serial_receive_break(sh_serial_state *s)
+static void sh_serial_receive_break(SHSerialState *s)
 {
     if (s->feat & SH_SERIAL_FEAT_SCIF) {
         s->sr |= (1 << 4);
@@ -323,13 +323,13 @@ static void sh_serial_receive_break(sh_serial_state *s)
 
 static int sh_serial_can_receive1(void *opaque)
 {
-    sh_serial_state *s = opaque;
+    SHSerialState *s = opaque;
     return sh_serial_can_receive(s);
 }
 
 static void sh_serial_timeout_int(void *opaque)
 {
-    sh_serial_state *s = opaque;
+    SHSerialState *s = opaque;
 
     s->flags |= SH_SERIAL_FLAG_RDF;
     if (s->scr & (1 << 6) && s->rxi) {
@@ -339,7 +339,7 @@ static void sh_serial_timeout_int(void *opaque)
 
 static void sh_serial_receive1(void *opaque, const uint8_t *buf, int size)
 {
-    sh_serial_state *s = opaque;
+    SHSerialState *s = opaque;
 
     if (s->feat & SH_SERIAL_FEAT_SCIF) {
         int i;
@@ -369,7 +369,7 @@ static void sh_serial_receive1(void *opaque, const uint8_t *buf, int size)
 
 static void sh_serial_event(void *opaque, QEMUChrEvent event)
 {
-    sh_serial_state *s = opaque;
+    SHSerialState *s = opaque;
     if (event == CHR_EVENT_BREAK) {
         sh_serial_receive_break(s);
     }
@@ -390,9 +390,7 @@ void sh_serial_init(MemoryRegion *sysmem,
                     qemu_irq tei_source,
                     qemu_irq bri_source)
 {
-    sh_serial_state *s;
-
-    s = g_malloc0(sizeof(sh_serial_state));
+    SHSerialState *s = g_malloc0(sizeof(*s));
 
     s->feat = feat;
     s->flags = SH_SERIAL_FLAG_TEND | SH_SERIAL_FLAG_TDE;
-- 
2.21.4

Re: [PATCH v2 05/11] hw/char/sh_serial: Rename type sh_serial_state to SHSerialState

[Philippe Mathieu-Daudé]

On 10/27/21 15:46, BALATON Zoltan wrote:
> Coding style says types should be camel case.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/char/sh_serial.c | 24 +++++++++++-------------
>  1 file changed, 11 insertions(+), 13 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>