[PATCH v4 00/23] More SH4 clean ups

[PATCH v4 00/23] 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 got big enough
for now so I'll wait until these are merged (hopefully before the
freeze) before going on so I don't have to carry them in my tree. Only
plan to submit another version if needed from review otherwise that's
it for now.

v4: Drop changes from fprintf before abort() as Philippe said, only
change sh_serial now which can use hw_error instead; missed two more
debug printfs in sh_timer that I've also added now; some more clean ups

v3: Correct mistakes found in review, drop size change of sh_intc
iomem as that was wrong so only rename it, more clean ups

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 (23):
  hw/sh4: Fix typos in a comment
  hw/char/sh_serial: Use hw_error instead of fprintf and abort
  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: Rename iomem region
  hw/intc/sh_intc: Drop another useless macro
  hw/intc/sh_intc: Move sh_intc_register() closer to its only user
  hw/intc/sh_intc: Remove excessive parenthesis
  hw/intc/sh_intc: Use array index instead of pointer arithmetics
  hw/intc/sh_intc: Inline and drop sh_intc_source() function
  hw/intc/sh_intc: Replace abort() with g_assert_not_reached()
  hw/intc/sh_intc: Avoid using continue in loops
  hw/intc/sh_intc: Simplify allocating sources array
  hw/intc/sh_intc: Remove unneeded local variable initialisers
  hw/timer/sh_timer: Rename sh_timer_state to SHTimerState
  hw/timer/sh_timer: Fix format strings and remove casts
  hw/timer/sh_timer: Do not wrap lines that are not too long
  hw/timer/sh_timer: Fix timer memory region size

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

-- 
2.21.4

[PATCH v4 04/23] 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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.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

[PATCH v4 22/23] hw/timer/sh_timer: Do not wrap lines that are not too long

[BALATON Zoltan]

It's more readable to keep things on one line if it fits the length limit.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/timer/sh_timer.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/hw/timer/sh_timer.c b/hw/timer/sh_timer.c
index fca27cb247..f4cc481a90 100644
--- a/hw/timer/sh_timer.c
+++ b/hw/timer/sh_timer.c
@@ -82,8 +82,7 @@ static uint32_t sh_timer_read(void *opaque, hwaddr offset)
     }
 }
 
-static void sh_timer_write(void *opaque, hwaddr offset,
-                            uint32_t value)
+static void sh_timer_write(void *opaque, hwaddr offset, uint32_t value)
 {
     SHTimerState *s = opaque;
     int freq;
@@ -256,8 +255,7 @@ typedef struct {
     int feat;
 } tmu012_state;
 
-static uint64_t tmu012_read(void *opaque, hwaddr offset,
-                            unsigned size)
+static uint64_t tmu012_read(void *opaque, hwaddr offset, unsigned size)
 {
     tmu012_state *s = opaque;
 
@@ -338,8 +336,7 @@ static const MemoryRegionOps tmu012_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-void tmu012_init(MemoryRegion *sysmem, hwaddr base,
-                 int feat, uint32_t freq,
+void tmu012_init(MemoryRegion *sysmem, hwaddr base, int feat, uint32_t freq,
                  qemu_irq ch0_irq, qemu_irq ch1_irq,
                  qemu_irq ch2_irq0, qemu_irq ch2_irq1)
 {
-- 
2.21.4

Re: [PATCH v4 22/23] hw/timer/sh_timer: Do not wrap lines that are not too long

[Philippe Mathieu-Daudé]

On 10/28/21 21:27, BALATON Zoltan wrote:
> It's more readable to keep things on one line if it fits the length limit.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/timer/sh_timer.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)

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

[PATCH v4 13/23] hw/intc/sh_intc: Remove excessive parenthesis

[BALATON Zoltan]

Drop unneded parenthesis and split up one complex expression to write
it with less brackets so it's easier to follow.

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

diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
index 54803bc2ca..537fc503d4 100644
--- a/hw/intc/sh_intc.c
+++ b/hw/intc/sh_intc.c
@@ -23,7 +23,7 @@ void sh_intc_toggle_source(struct intc_source *source,
     int pending_changed = 0;
     int old_pending;
 
-    if ((source->enable_count == source->enable_max) && (enable_adj == -1)) {
+    if (source->enable_count == source->enable_max && enable_adj == -1) {
         enable_changed = -1;
     }
     source->enable_count += enable_adj;
@@ -68,7 +68,7 @@ void sh_intc_toggle_source(struct intc_source *source,
 static void sh_intc_set_irq(void *opaque, int n, int level)
 {
   struct intc_desc *desc = opaque;
-  struct intc_source *source = &(desc->sources[n]);
+  struct intc_source *source = &desc->sources[n];
 
   if (level && !source->asserted) {
     sh_intc_toggle_source(source, 0, 1);
@@ -164,7 +164,7 @@ static void sh_intc_locate(struct intc_desc *desc,
             *modep = mode | INTC_MODE_IS_PRIO;
             *datap = &pr->value;
             *enums = pr->enum_ids;
-            *first = (pr->reg_width / pr->field_width) - 1;
+            *first = pr->reg_width / pr->field_width - 1;
             *width = pr->field_width;
             return;
         }
@@ -245,7 +245,8 @@ static void sh_intc_write(void *opaque, hwaddr offset,
     }
 
     for (k = 0; k <= first; k++) {
-        mask = ((1 << width) - 1) << ((first - k) * width);
+        mask = (1 << width) - 1;
+        mask <<= (first - k) * width;
 
         if ((*valuep & mask) == (value & mask)) {
             continue;
-- 
2.21.4

Re: [PATCH v4 13/23] hw/intc/sh_intc: Remove excessive parenthesis

[Philippe Mathieu-Daudé]

On 10/28/21 21:27, BALATON Zoltan wrote:
> Drop unneded parenthesis and split up one complex expression to write
> it with less brackets so it's easier to follow.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  hw/intc/sh_intc.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)

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

[PATCH v4 20/23] hw/timer/sh_timer: Rename sh_timer_state to SHTimerState

[BALATON Zoltan]

According to coding style types should be camel case, also remove
unneded casts from void *.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
The tmu123_state is left for now, that's the real exported object with
SHTimerState being an internal object for a single timer. I'll come
back to this when QOM-ifying so only handled SHTimerState in this patch.

 hw/timer/sh_timer.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/hw/timer/sh_timer.c b/hw/timer/sh_timer.c
index e1b6145df8..2038adfb0a 100644
--- a/hw/timer/sh_timer.c
+++ b/hw/timer/sh_timer.c
@@ -45,11 +45,11 @@ typedef struct {
     int feat;
     int enabled;
     qemu_irq irq;
-} sh_timer_state;
+} SHTimerState;
 
 /* Check all active timers, and schedule the next timer interrupt. */
 
-static void sh_timer_update(sh_timer_state *s)
+static void sh_timer_update(SHTimerState *s)
 {
     int new_level = s->int_level && (s->tcr & TIMER_TCR_UNIE);
 
@@ -62,7 +62,7 @@ static void sh_timer_update(sh_timer_state *s)
 
 static uint32_t sh_timer_read(void *opaque, hwaddr offset)
 {
-    sh_timer_state *s = (sh_timer_state *)opaque;
+    SHTimerState *s = opaque;
 
     switch (offset >> 2) {
     case OFFSET_TCOR:
@@ -85,7 +85,7 @@ static uint32_t sh_timer_read(void *opaque, hwaddr offset)
 static void sh_timer_write(void *opaque, hwaddr offset,
                             uint32_t value)
 {
-    sh_timer_state *s = (sh_timer_state *)opaque;
+    SHTimerState *s = opaque;
     int freq;
 
     switch (offset >> 2) {
@@ -200,7 +200,7 @@ static void sh_timer_write(void *opaque, hwaddr offset,
 
 static void sh_timer_start_stop(void *opaque, int enable)
 {
-    sh_timer_state *s = (sh_timer_state *)opaque;
+    SHTimerState *s = opaque;
 
     trace_sh_timer_start_stop(enable, s->enabled);
     ptimer_transaction_begin(s->timer);
@@ -216,14 +216,14 @@ static void sh_timer_start_stop(void *opaque, int enable)
 
 static void sh_timer_tick(void *opaque)
 {
-    sh_timer_state *s = (sh_timer_state *)opaque;
+    SHTimerState *s = opaque;
     s->int_level = s->enabled;
     sh_timer_update(s);
 }
 
 static void *sh_timer_init(uint32_t freq, int feat, qemu_irq irq)
 {
-    sh_timer_state *s;
+    SHTimerState *s;
 
     s = g_malloc0(sizeof(*s));
     s->freq = freq;
@@ -259,7 +259,7 @@ typedef struct {
 static uint64_t tmu012_read(void *opaque, hwaddr offset,
                             unsigned size)
 {
-    tmu012_state *s = (tmu012_state *)opaque;
+    tmu012_state *s = opaque;
 
     trace_sh_timer_read(offset);
     if (offset >= 0x20) {
@@ -289,7 +289,7 @@ static uint64_t tmu012_read(void *opaque, hwaddr offset,
 static void tmu012_write(void *opaque, hwaddr offset,
                         uint64_t value, unsigned size)
 {
-    tmu012_state *s = (tmu012_state *)opaque;
+    tmu012_state *s = opaque;
 
     trace_sh_timer_write(offset, value);
     if (offset >= 0x20) {
-- 
2.21.4

Re: [PATCH v4 20/23] hw/timer/sh_timer: Rename sh_timer_state to SHTimerState

[Philippe Mathieu-Daudé]

On 10/28/21 21:27, BALATON Zoltan wrote:
> According to coding style types should be camel case, also remove
> unneded casts from void *.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> The tmu123_state is left for now, that's the real exported object with
> SHTimerState being an internal object for a single timer. I'll come
> back to this when QOM-ifying so only handled SHTimerState in this patch.
> 
>  hw/timer/sh_timer.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)

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

[PATCH v4 21/23] hw/timer/sh_timer: Fix format strings and remove casts

[BALATON Zoltan]

The type casts are not needed when using the right format strings.

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

diff --git a/hw/timer/sh_timer.c b/hw/timer/sh_timer.c
index 2038adfb0a..fca27cb247 100644
--- a/hw/timer/sh_timer.c
+++ b/hw/timer/sh_timer.c
@@ -77,7 +77,7 @@ static uint32_t sh_timer_read(void *opaque, hwaddr offset)
         }
         /* fall through */
     default:
-        hw_error("sh_timer_read: Bad offset %x\n", (int)offset);
+        hw_error("sh_timer_read: Bad offset 0x%"HWADDR_PRIx"\n", offset);
         return 0;
     }
 }
@@ -193,7 +193,7 @@ static void sh_timer_write(void *opaque, hwaddr offset,
         }
         /* fallthrough */
     default:
-        hw_error("sh_timer_write: Bad offset %x\n", (int)offset);
+        hw_error("sh_timer_write: Bad offset 0x%"HWADDR_PRIx"\n", offset);
     }
     sh_timer_update(s);
 }
@@ -264,7 +264,8 @@ static uint64_t tmu012_read(void *opaque, hwaddr offset,
     trace_sh_timer_read(offset);
     if (offset >= 0x20) {
         if (!(s->feat & TMU012_FEAT_3CHAN)) {
-            hw_error("tmu012_write: Bad channel offset %x\n", (int)offset);
+            hw_error("tmu012_write: Bad channel offset 0x%"HWADDR_PRIx"\n",
+                     offset);
         }
         return sh_timer_read(s->timer[2], offset - 0x20);
     }
@@ -282,7 +283,7 @@ static uint64_t tmu012_read(void *opaque, hwaddr offset,
         return s->tocr;
     }
 
-    hw_error("tmu012_write: Bad offset %x\n", (int)offset);
+    hw_error("tmu012_write: Bad offset 0x%"HWADDR_PRIx"\n", offset);
     return 0;
 }
 
@@ -294,7 +295,8 @@ static void tmu012_write(void *opaque, hwaddr offset,
     trace_sh_timer_write(offset, value);
     if (offset >= 0x20) {
         if (!(s->feat & TMU012_FEAT_3CHAN)) {
-            hw_error("tmu012_write: Bad channel offset %x\n", (int)offset);
+            hw_error("tmu012_write: Bad channel offset 0x%"HWADDR_PRIx"\n",
+                     offset);
         }
         sh_timer_write(s->timer[2], offset - 0x20, value);
         return;
-- 
2.21.4

[PATCH v4 18/23] hw/intc/sh_intc: Simplify allocating sources array

[BALATON Zoltan]

Use g_new0 instead of g_malloc0 and avoid some unneeded temporary
variable assignments.

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

diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
index eb58707e83..ed0a5f87cc 100644
--- a/hw/intc/sh_intc.c
+++ b/hw/intc/sh_intc.c
@@ -400,21 +400,14 @@ int sh_intc_init(MemoryRegion *sysmem,
     /* Allocate 4 MemoryRegions per register (2 actions * 2 aliases) */
     desc->iomem_aliases = g_new0(MemoryRegion,
                                  (nr_mask_regs + nr_prio_regs) * 4);
-
-    j = 0;
-    i = sizeof(struct intc_source) * nr_sources;
-    desc->sources = g_malloc0(i);
-
+    desc->sources = g_new0(struct intc_source, nr_sources);
     for (i = 0; i < desc->nr_sources; i++) {
-        struct intc_source *source = &desc->sources[i];
-
-        source->parent = desc;
+        desc->sources[i].parent = desc;
     }
-
     desc->irqs = qemu_allocate_irqs(sh_intc_set_irq, desc, nr_sources);
     memory_region_init_io(&desc->iomem, NULL, &sh_intc_ops, desc, "intc",
                           0x100000000ULL);
-
+    j = 0;
     if (desc->mask_regs) {
         for (i = 0; i < desc->nr_mask_regs; i++) {
             struct intc_mask_reg *mr = &desc->mask_regs[i];
-- 
2.21.4

Re: [PATCH v4 18/23] hw/intc/sh_intc: Simplify allocating sources array

[Philippe Mathieu-Daudé]

On 10/28/21 21:27, BALATON Zoltan wrote:
> Use g_new0 instead of g_malloc0 and avoid some unneeded temporary
> variable assignments.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/intc/sh_intc.c | 13 +++----------
>  1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
> index eb58707e83..ed0a5f87cc 100644
> --- a/hw/intc/sh_intc.c
> +++ b/hw/intc/sh_intc.c
> @@ -400,21 +400,14 @@ int sh_intc_init(MemoryRegion *sysmem,
>      /* Allocate 4 MemoryRegions per register (2 actions * 2 aliases) */
>      desc->iomem_aliases = g_new0(MemoryRegion,
>                                   (nr_mask_regs + nr_prio_regs) * 4);
> -
> -    j = 0;
> -    i = sizeof(struct intc_source) * nr_sources;
> -    desc->sources = g_malloc0(i);
> -
> +    desc->sources = g_new0(struct intc_source, nr_sources);

g_new() is enough, since all get initialized in the next line.

>      for (i = 0; i < desc->nr_sources; i++) {

Even clearer as:

       for (i = 0; i < nr_sources; i++) {

> -        struct intc_source *source = &desc->sources[i];
> -
> -        source->parent = desc;
> +        desc->sources[i].parent = desc;
>      }
> -
>      desc->irqs = qemu_allocate_irqs(sh_intc_set_irq, desc, nr_sources);
>      memory_region_init_io(&desc->iomem, NULL, &sh_intc_ops, desc, "intc",
>                            0x100000000ULL);
> -
> +    j = 0;
>      if (desc->mask_regs) {
>          for (i = 0; i < desc->nr_mask_regs; i++) {
>              struct intc_mask_reg *mr = &desc->mask_regs[i];
> 

Re: [PATCH v4 18/23] hw/intc/sh_intc: Simplify allocating sources array

[BALATON Zoltan]

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

On Fri, 29 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/28/21 21:27, BALATON Zoltan wrote:
>> Use g_new0 instead of g_malloc0 and avoid some unneeded temporary
>> variable assignments.
>>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/intc/sh_intc.c | 13 +++----------
>>  1 file changed, 3 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
>> index eb58707e83..ed0a5f87cc 100644
>> --- a/hw/intc/sh_intc.c
>> +++ b/hw/intc/sh_intc.c
>> @@ -400,21 +400,14 @@ int sh_intc_init(MemoryRegion *sysmem,
>>      /* Allocate 4 MemoryRegions per register (2 actions * 2 aliases) */
>>      desc->iomem_aliases = g_new0(MemoryRegion,
>>                                   (nr_mask_regs + nr_prio_regs) * 4);
>> -
>> -    j = 0;
>> -    i = sizeof(struct intc_source) * nr_sources;
>> -    desc->sources = g_malloc0(i);
>> -
>> +    desc->sources = g_new0(struct intc_source, nr_sources);
>
> g_new() is enough, since all get initialized in the next line.

Only their parent fields get init not the whole struct so I think g_new0 
is still needed.

>>      for (i = 0; i < desc->nr_sources; i++) {
>
> Even clearer as:
>
>       for (i = 0; i < nr_sources; i++) {

This may be a small improvement but not too much, desc->sources is 
assigned a few lines before. I consider this change but not sure about the 
g_new0.

Regards,
BALATON Zoltan

>> -        struct intc_source *source = &desc->sources[i];
>> -
>> -        source->parent = desc;
>> +        desc->sources[i].parent = desc;
>>      }
>> -
>>      desc->irqs = qemu_allocate_irqs(sh_intc_set_irq, desc, nr_sources);
>>      memory_region_init_io(&desc->iomem, NULL, &sh_intc_ops, desc, "intc",
>>                            0x100000000ULL);
>> -
>> +    j = 0;
>>      if (desc->mask_regs) {
>>          for (i = 0; i < desc->nr_mask_regs; i++) {
>>              struct intc_mask_reg *mr = &desc->mask_regs[i];
>>
>
>

Re: [PATCH v4 18/23] hw/intc/sh_intc: Simplify allocating sources array

[Philippe Mathieu-Daudé]

On 10/29/21 13:59, BALATON Zoltan wrote:
> On Fri, 29 Oct 2021, Philippe Mathieu-Daudé wrote:
>> On 10/28/21 21:27, BALATON Zoltan wrote:
>>> Use g_new0 instead of g_malloc0 and avoid some unneeded temporary
>>> variable assignments.
>>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>>  hw/intc/sh_intc.c | 13 +++----------
>>>  1 file changed, 3 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
>>> index eb58707e83..ed0a5f87cc 100644
>>> --- a/hw/intc/sh_intc.c
>>> +++ b/hw/intc/sh_intc.c
>>> @@ -400,21 +400,14 @@ int sh_intc_init(MemoryRegion *sysmem,
>>>      /* Allocate 4 MemoryRegions per register (2 actions * 2 aliases) */
>>>      desc->iomem_aliases = g_new0(MemoryRegion,
>>>                                   (nr_mask_regs + nr_prio_regs) * 4);
>>> -
>>> -    j = 0;
>>> -    i = sizeof(struct intc_source) * nr_sources;
>>> -    desc->sources = g_malloc0(i);
>>> -
>>> +    desc->sources = g_new0(struct intc_source, nr_sources);
>>
>> g_new() is enough, since all get initialized in the next line.
> 
> Only their parent fields get init not the whole struct so I think g_new0
> is still needed.

Oh you are right, I missed that.

> 
>>>      for (i = 0; i < desc->nr_sources; i++) {
>>
>> Even clearer as:
>>
>>       for (i = 0; i < nr_sources; i++) {
> 
> This may be a small improvement but not too much, desc->sources is
> assigned a few lines before. I consider this change but not sure about
> the g_new0.
> 
> Regards,
> BALATON Zoltan

[PATCH v4 15/23] hw/intc/sh_intc: Inline and drop sh_intc_source() function

[BALATON Zoltan]

This function is very simple and provides no advantage. Call sites
become simpler without it so just write it in line and drop the
separate function.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/intc/sh_intc.c | 54 +++++++++++++++++++----------------------------
 hw/sh4/sh7750.c   |  4 ++--
 2 files changed, 24 insertions(+), 34 deletions(-)

diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
index b1056f769e..57c341c030 100644
--- a/hw/intc/sh_intc.c
+++ b/hw/intc/sh_intc.c
@@ -263,33 +263,22 @@ static const MemoryRegionOps sh_intc_ops = {
     .endianness = DEVICE_NATIVE_ENDIAN,
 };
 
-struct intc_source *sh_intc_source(struct intc_desc *desc, intc_enum id)
-{
-    if (id) {
-        return &desc->sources[id];
-    }
-    return NULL;
-}
-
 static void sh_intc_register_source(struct intc_desc *desc,
                                     intc_enum source,
                                     struct intc_group *groups,
                                     int nr_groups)
 {
     unsigned int i, k;
-    struct intc_source *s;
+    intc_enum id;
 
     if (desc->mask_regs) {
         for (i = 0; i < desc->nr_mask_regs; i++) {
             struct intc_mask_reg *mr = &desc->mask_regs[i];
 
             for (k = 0; k < ARRAY_SIZE(mr->enum_ids); k++) {
-                if (mr->enum_ids[k] != source) {
-                    continue;
-                }
-                s = sh_intc_source(desc, mr->enum_ids[k]);
-                if (s) {
-                    s->enable_max++;
+                id = mr->enum_ids[k];
+                if (id && id == source) {
+                    desc->sources[id].enable_max++;
                 }
             }
         }
@@ -300,12 +289,9 @@ static void sh_intc_register_source(struct intc_desc *desc,
             struct intc_prio_reg *pr = &desc->prio_regs[i];
 
             for (k = 0; k < ARRAY_SIZE(pr->enum_ids); k++) {
-                if (pr->enum_ids[k] != source) {
-                    continue;
-                }
-                s = sh_intc_source(desc, pr->enum_ids[k]);
-                if (s) {
-                    s->enable_max++;
+                id = pr->enum_ids[k];
+                if (id && id == source) {
+                    desc->sources[id].enable_max++;
                 }
             }
         }
@@ -316,12 +302,9 @@ static void sh_intc_register_source(struct intc_desc *desc,
             struct intc_group *gr = &groups[i];
 
             for (k = 0; k < ARRAY_SIZE(gr->enum_ids); k++) {
-                if (gr->enum_ids[k] != source) {
-                    continue;
-                }
-                s = sh_intc_source(desc, gr->enum_ids[k]);
-                if (s) {
-                    s->enable_max++;
+                id = gr->enum_ids[k];
+                if (id && id == source) {
+                    desc->sources[id].enable_max++;
                 }
             }
         }
@@ -336,14 +319,16 @@ void sh_intc_register_sources(struct intc_desc *desc,
                               int nr_groups)
 {
     unsigned int i, k;
+    intc_enum id;
     struct intc_source *s;
 
     for (i = 0; i < nr_vectors; i++) {
         struct intc_vect *vect = &vectors[i];
 
         sh_intc_register_source(desc, vect->enum_id, groups, nr_groups);
-        s = sh_intc_source(desc, vect->enum_id);
-        if (s) {
+        id = vect->enum_id;
+        if (id) {
+            s = &desc->sources[id];
             s->vect = vect->vect;
             trace_sh_intc_register("source", vect->enum_id, s->vect,
                                    s->enable_count, s->enable_max);
@@ -354,14 +339,16 @@ void sh_intc_register_sources(struct intc_desc *desc,
         for (i = 0; i < nr_groups; i++) {
             struct intc_group *gr = &groups[i];
 
-            s = sh_intc_source(desc, gr->enum_id);
+            id = gr->enum_id;
+            s = &desc->sources[id];
             s->next_enum_id = gr->enum_ids[0];
 
             for (k = 1; k < ARRAY_SIZE(gr->enum_ids); k++) {
                 if (!gr->enum_ids[k]) {
                     continue;
                 }
-                s = sh_intc_source(desc, gr->enum_ids[k - 1]);
+                id = gr->enum_ids[k - 1];
+                s = &desc->sources[id];
                 s->next_enum_id = gr->enum_ids[k];
             }
             trace_sh_intc_register("group", gr->enum_id, 0xffff,
@@ -463,7 +450,10 @@ void sh_intc_set_irl(void *opaque, int n, int level)
 {
     struct intc_source *s = opaque;
     int i, irl = level ^ 15;
-    for (i = 0; (s = sh_intc_source(s->parent, s->next_enum_id)); i++) {
+    intc_enum id = s->next_enum_id;
+
+    for (i = 0; id; id = s->next_enum_id, i++) {
+        s = &s->parent->sources[id];
         if (i == irl) {
             sh_intc_toggle_source(s, s->enable_count ? 0 : 1,
                                   s->asserted ? 0 : 1);
diff --git a/hw/sh4/sh7750.c b/hw/sh4/sh7750.c
index 22016de664..3c10fc863d 100644
--- a/hw/sh4/sh7750.c
+++ b/hw/sh4/sh7750.c
@@ -905,6 +905,6 @@ SH7750State *sh7750_init(SuperHCPU *cpu, MemoryRegion *sysmem)
 
 qemu_irq sh7750_irl(SH7750State *s)
 {
-    sh_intc_toggle_source(sh_intc_source(&s->intc, IRL), 1, 0); /* enable */
-    return qemu_allocate_irq(sh_intc_set_irl, sh_intc_source(&s->intc, IRL), 0);
+    sh_intc_toggle_source(&s->intc.sources[IRL], 1, 0); /* enable */
+    return qemu_allocate_irq(sh_intc_set_irl, &s->intc.sources[IRL], 0);
 }
-- 
2.21.4

[PATCH v4 17/23] hw/intc/sh_intc: Avoid using continue in loops

[BALATON Zoltan]

Instead of if !expr continue else do something it is more straight
forward to say if expr then do something, especially if the action is
just a few lines. Remove such uses of continue to make the code easier
to follow.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/intc/sh_intc.c | 44 ++++++++++++++++++++------------------------
 1 file changed, 20 insertions(+), 24 deletions(-)

diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
index 56a288e093..eb58707e83 100644
--- a/hw/intc/sh_intc.c
+++ b/hw/intc/sh_intc.c
@@ -140,15 +140,14 @@ static void sh_intc_locate(struct intc_desc *desc,
             struct intc_mask_reg *mr = &desc->mask_regs[i];
 
             mode = sh_intc_mode(address, mr->set_reg, mr->clr_reg);
-            if (mode == INTC_MODE_NONE) {
-                continue;
+            if (mode != INTC_MODE_NONE) {
+                *modep = mode;
+                *datap = &mr->value;
+                *enums = mr->enum_ids;
+                *first = mr->reg_width - 1;
+                *width = 1;
+                return;
             }
-            *modep = mode;
-            *datap = &mr->value;
-            *enums = mr->enum_ids;
-            *first = mr->reg_width - 1;
-            *width = 1;
-            return;
         }
     }
 
@@ -157,15 +156,14 @@ static void sh_intc_locate(struct intc_desc *desc,
             struct intc_prio_reg *pr = &desc->prio_regs[i];
 
             mode = sh_intc_mode(address, pr->set_reg, pr->clr_reg);
-            if (mode == INTC_MODE_NONE) {
-                continue;
+            if (mode != INTC_MODE_NONE) {
+                *modep = mode | INTC_MODE_IS_PRIO;
+                *datap = &pr->value;
+                *enums = pr->enum_ids;
+                *first = pr->reg_width / pr->field_width - 1;
+                *width = pr->field_width;
+                return;
             }
-            *modep = mode | INTC_MODE_IS_PRIO;
-            *datap = &pr->value;
-            *enums = pr->enum_ids;
-            *first = pr->reg_width / pr->field_width - 1;
-            *width = pr->field_width;
-            return;
         }
     }
     g_assert_not_reached();
@@ -246,10 +244,9 @@ static void sh_intc_write(void *opaque, hwaddr offset,
         mask = (1 << width) - 1;
         mask <<= (first - k) * width;
 
-        if ((*valuep & mask) == (value & mask)) {
-            continue;
+        if ((*valuep & mask) != (value & mask)) {
+            sh_intc_toggle_mask(desc, enum_ids[k], value & mask, 0);
         }
-        sh_intc_toggle_mask(desc, enum_ids[k], value & mask, 0);
     }
 
     *valuep = value;
@@ -342,12 +339,11 @@ void sh_intc_register_sources(struct intc_desc *desc,
             s->next_enum_id = gr->enum_ids[0];
 
             for (k = 1; k < ARRAY_SIZE(gr->enum_ids); k++) {
-                if (!gr->enum_ids[k]) {
-                    continue;
+                if (gr->enum_ids[k]) {
+                    id = gr->enum_ids[k - 1];
+                    s = &desc->sources[id];
+                    s->next_enum_id = gr->enum_ids[k];
                 }
-                id = gr->enum_ids[k - 1];
-                s = &desc->sources[id];
-                s->next_enum_id = gr->enum_ids[k];
             }
             trace_sh_intc_register("group", gr->enum_id, 0xffff,
                                    s->enable_count, s->enable_max);
-- 
2.21.4

[PATCH v4 03/23] hw/sh4: Change debug printfs to traces

[BALATON Zoltan]

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 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   | 22 +++---------
 hw/timer/trace-events |  5 +++
 meson.build           |  1 +
 10 files changed, 52 insertions(+), 92 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 dbefb51d71..6ffab376d8 100644
--- a/hw/char/sh_serial.c
+++ b/hw/char/sh_serial.c
@@ -32,8 +32,7 @@
 #include "chardev/char-fe.h"
 #include "qapi/error.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);
@@ -300,10 +296,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)) {
         hw_error("sh_serial: unsupported read from 0x%02"HWADDR_PRIx"\n", offs);
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 ca7e261aba..6c702d627c 100644
--- a/hw/sh4/sh7750.c
+++ b/hw/sh4/sh7750.c
@@ -32,6 +32,7 @@
 #include "hw/sh4/sh_intc.h"
 #include "hw/timer/tmu012.h"
 #include "exec/exec-all.h"
+#include "trace.h"
 
 #define NB_DEVICES 4
 
@@ -147,15 +148,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++) {
@@ -182,6 +179,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 cc7c1897a8..e1b6145df8 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)
@@ -269,10 +261,7 @@ static uint64_t tmu012_read(void *opaque, hwaddr offset,
 {
     tmu012_state *s = (tmu012_state *)opaque;
 
-#ifdef DEBUG_TIMER
-    printf("tmu012_read 0x%lx\n", (unsigned long) offset);
-#endif
-
+    trace_sh_timer_read(offset);
     if (offset >= 0x20) {
         if (!(s->feat & TMU012_FEAT_3CHAN)) {
             hw_error("tmu012_write: Bad channel offset %x\n", (int)offset);
@@ -302,10 +291,7 @@ static void tmu012_write(void *opaque, hwaddr offset,
 {
     tmu012_state *s = (tmu012_state *)opaque;
 
-#ifdef DEBUG_TIMER
-    printf("tmu012_write 0x%lx 0x%08x\n", (unsigned long) offset, value);
-#endif
-
+    trace_sh_timer_write(offset, value);
     if (offset >= 0x20) {
         if (!(s->feat & TMU012_FEAT_3CHAN)) {
             hw_error("tmu012_write: Bad channel offset %x\n", (int)offset);
diff --git a/hw/timer/trace-events b/hw/timer/trace-events
index d0edcd2a80..3eccef8385 100644
--- a/hw/timer/trace-events
+++ b/hw/timer/trace-events
@@ -94,3 +94,8 @@ 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)"
+sh_timer_read(uint64_t offset) "tmu012_read 0x%" PRIx64
+sh_timer_write(uint64_t offset, uint64_t value) "tmu012_write 0x%" PRIx64 " 0x%08" PRIx64
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 v4 19/23] hw/intc/sh_intc: Remove unneeded local variable initialisers

[BALATON Zoltan]

The sh_intc_locate function will either init these or not return so no
need to initialise them.

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

diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
index ed0a5f87cc..1f4e1b9370 100644
--- a/hw/intc/sh_intc.c
+++ b/hw/intc/sh_intc.c
@@ -196,14 +196,13 @@ static void sh_intc_toggle_mask(struct intc_desc *desc, intc_enum id,
     }
 }
 
-static uint64_t sh_intc_read(void *opaque, hwaddr offset,
-                             unsigned size)
+static uint64_t sh_intc_read(void *opaque, hwaddr offset, unsigned size)
 {
     struct intc_desc *desc = opaque;
-    intc_enum *enum_ids = NULL;
-    unsigned int first = 0;
-    unsigned int width = 0;
-    unsigned int mode = 0;
+    intc_enum *enum_ids;
+    unsigned int first;
+    unsigned int width;
+    unsigned int mode;
     unsigned long *valuep;
 
     sh_intc_locate(desc, (unsigned long)offset, &valuep,
@@ -216,12 +215,12 @@ static void sh_intc_write(void *opaque, hwaddr offset,
                           uint64_t value, unsigned size)
 {
     struct intc_desc *desc = opaque;
-    intc_enum *enum_ids = NULL;
-    unsigned int first = 0;
-    unsigned int width = 0;
-    unsigned int mode = 0;
-    unsigned int k;
+    intc_enum *enum_ids;
+    unsigned int first;
+    unsigned int width;
+    unsigned int mode;
     unsigned long *valuep;
+    unsigned int k;
     unsigned long mask;
 
     trace_sh_intc_write(size, offset, value);
-- 
2.21.4

[PATCH v4 23/23] hw/timer/sh_timer: Fix timer memory region size

[BALATON Zoltan]

The timer memory region is only accessed via aliases that are 0x1000
bytes long, no need to have the timer region larger than that.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
 hw/timer/sh_timer.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/timer/sh_timer.c b/hw/timer/sh_timer.c
index f4cc481a90..ee3986edd0 100644
--- a/hw/timer/sh_timer.c
+++ b/hw/timer/sh_timer.c
@@ -352,8 +352,7 @@ void tmu012_init(MemoryRegion *sysmem, hwaddr base, int feat, uint32_t freq,
                                     ch2_irq0); /* ch2_irq1 not supported */
     }
 
-    memory_region_init_io(&s->iomem, NULL, &tmu012_ops, s,
-                          "timer", 0x100000000ULL);
+    memory_region_init_io(&s->iomem, NULL, &tmu012_ops, s, "timer", 0x1000);
 
     memory_region_init_alias(&s->iomem_p4, NULL, "timer-p4",
                              &s->iomem, 0, 0x1000);
-- 
2.21.4

[PATCH v4 11/23] 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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 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 e386372b6f..763ebbfec2 100644
--- a/hw/intc/sh_intc.c
+++ b/hw/intc/sh_intc.c
@@ -433,16 +433,12 @@ int sh_intc_init(MemoryRegion *sysmem,
     memory_region_init_io(&desc->iomem, NULL, &sh_intc_ops, desc, "intc",
                           0x100000000ULL);
 
-#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);
         }
     }
 
@@ -450,13 +446,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

[PATCH v4 14/23] hw/intc/sh_intc: Use array index instead of pointer arithmetics

[BALATON Zoltan]

Address of element i is one word thus clearer than array + i.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/intc/sh_intc.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
index 537fc503d4..b1056f769e 100644
--- a/hw/intc/sh_intc.c
+++ b/hw/intc/sh_intc.c
@@ -89,7 +89,7 @@ int sh_intc_get_pending_vector(struct intc_desc *desc, int imask)
     }
 
     for (i = 0; i < desc->nr_sources; i++) {
-        struct intc_source *source = desc->sources + i;
+        struct intc_source *source = &desc->sources[i];
 
         if (source->pending) {
             trace_sh_intc_pending(desc->pending, source->vect);
@@ -138,7 +138,7 @@ static void sh_intc_locate(struct intc_desc *desc,
 
     if (desc->mask_regs) {
         for (i = 0; i < desc->nr_mask_regs; i++) {
-            struct intc_mask_reg *mr = desc->mask_regs + i;
+            struct intc_mask_reg *mr = &desc->mask_regs[i];
 
             mode = sh_intc_mode(address, mr->set_reg, mr->clr_reg);
             if (mode == INTC_MODE_NONE) {
@@ -155,7 +155,7 @@ static void sh_intc_locate(struct intc_desc *desc,
 
     if (desc->prio_regs) {
         for (i = 0; i < desc->nr_prio_regs; i++) {
-            struct intc_prio_reg *pr = desc->prio_regs + i;
+            struct intc_prio_reg *pr = &desc->prio_regs[i];
 
             mode = sh_intc_mode(address, pr->set_reg, pr->clr_reg);
             if (mode == INTC_MODE_NONE) {
@@ -176,7 +176,7 @@ static void sh_intc_locate(struct intc_desc *desc,
 static void sh_intc_toggle_mask(struct intc_desc *desc, intc_enum id,
                                 int enable, int is_group)
 {
-    struct intc_source *source = desc->sources + id;
+    struct intc_source *source = &desc->sources[id];
 
     if (!id) {
         return;
@@ -266,7 +266,7 @@ static const MemoryRegionOps sh_intc_ops = {
 struct intc_source *sh_intc_source(struct intc_desc *desc, intc_enum id)
 {
     if (id) {
-        return desc->sources + id;
+        return &desc->sources[id];
     }
     return NULL;
 }
@@ -281,7 +281,7 @@ static void sh_intc_register_source(struct intc_desc *desc,
 
     if (desc->mask_regs) {
         for (i = 0; i < desc->nr_mask_regs; i++) {
-            struct intc_mask_reg *mr = desc->mask_regs + i;
+            struct intc_mask_reg *mr = &desc->mask_regs[i];
 
             for (k = 0; k < ARRAY_SIZE(mr->enum_ids); k++) {
                 if (mr->enum_ids[k] != source) {
@@ -297,7 +297,7 @@ static void sh_intc_register_source(struct intc_desc *desc,
 
     if (desc->prio_regs) {
         for (i = 0; i < desc->nr_prio_regs; i++) {
-            struct intc_prio_reg *pr = desc->prio_regs + i;
+            struct intc_prio_reg *pr = &desc->prio_regs[i];
 
             for (k = 0; k < ARRAY_SIZE(pr->enum_ids); k++) {
                 if (pr->enum_ids[k] != source) {
@@ -313,7 +313,7 @@ static void sh_intc_register_source(struct intc_desc *desc,
 
     if (groups) {
         for (i = 0; i < nr_groups; i++) {
-            struct intc_group *gr = groups + i;
+            struct intc_group *gr = &groups[i];
 
             for (k = 0; k < ARRAY_SIZE(gr->enum_ids); k++) {
                 if (gr->enum_ids[k] != source) {
@@ -339,7 +339,7 @@ void sh_intc_register_sources(struct intc_desc *desc,
     struct intc_source *s;
 
     for (i = 0; i < nr_vectors; i++) {
-        struct intc_vect *vect = vectors + i;
+        struct intc_vect *vect = &vectors[i];
 
         sh_intc_register_source(desc, vect->enum_id, groups, nr_groups);
         s = sh_intc_source(desc, vect->enum_id);
@@ -352,7 +352,7 @@ void sh_intc_register_sources(struct intc_desc *desc,
 
     if (groups) {
         for (i = 0; i < nr_groups; i++) {
-            struct intc_group *gr = groups + i;
+            struct intc_group *gr = &groups[i];
 
             s = sh_intc_source(desc, gr->enum_id);
             s->next_enum_id = gr->enum_ids[0];
@@ -385,7 +385,7 @@ static unsigned int sh_intc_register(MemoryRegion *sysmem,
     }
 
     iomem = &desc->iomem;
-    iomem_p4 = desc->iomem_aliases + index;
+    iomem_p4 = &desc->iomem_aliases[index];
     iomem_a7 = iomem_p4 + 1;
 
     snprintf(name, sizeof(name), "intc-%s-%s-%s", type, action, "p4");
@@ -425,7 +425,7 @@ int sh_intc_init(MemoryRegion *sysmem,
     desc->sources = g_malloc0(i);
 
     for (i = 0; i < desc->nr_sources; i++) {
-        struct intc_source *source = desc->sources + i;
+        struct intc_source *source = &desc->sources[i];
 
         source->parent = desc;
     }
@@ -436,7 +436,7 @@ int sh_intc_init(MemoryRegion *sysmem,
 
     if (desc->mask_regs) {
         for (i = 0; i < desc->nr_mask_regs; i++) {
-            struct intc_mask_reg *mr = desc->mask_regs + i;
+            struct intc_mask_reg *mr = &desc->mask_regs[i];
 
             j += sh_intc_register(sysmem, desc, mr->set_reg, "mask", "set", j);
             j += sh_intc_register(sysmem, desc, mr->clr_reg, "mask", "clr", j);
@@ -445,7 +445,7 @@ int sh_intc_init(MemoryRegion *sysmem,
 
     if (desc->prio_regs) {
         for (i = 0; i < desc->nr_prio_regs; i++) {
-            struct intc_prio_reg *pr = desc->prio_regs + i;
+            struct intc_prio_reg *pr = &desc->prio_regs[i];
 
             j += sh_intc_register(sysmem, desc, pr->set_reg, "prio", "set", j);
             j += sh_intc_register(sysmem, desc, pr->clr_reg, "prio", "clr", j);
-- 
2.21.4

Re: [PATCH v4 14/23] hw/intc/sh_intc: Use array index instead of pointer arithmetics

[Philippe Mathieu-Daudé]

On 10/28/21 21:27, BALATON Zoltan wrote:
> Address of element i is one word thus clearer than array + i.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  hw/intc/sh_intc.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)

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

[PATCH v4 01/23] hw/sh4: Fix typos in a comment

[BALATON Zoltan]

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.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..cc7c1897a8 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 messiness
              */
             ptimer_stop(s->timer);
         }
-- 
2.21.4

[PATCH v4 16/23] hw/intc/sh_intc: Replace abort() with g_assert_not_reached()

[BALATON Zoltan]

All the places that call abort should not happen which is better
marked by g_assert_not_reached.

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

diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
index 57c341c030..56a288e093 100644
--- a/hw/intc/sh_intc.c
+++ b/hw/intc/sh_intc.c
@@ -96,8 +96,7 @@ int sh_intc_get_pending_vector(struct intc_desc *desc, int imask)
             return source->vect;
         }
     }
-
-    abort();
+    g_assert_not_reached();
 }
 
 #define INTC_MODE_IS_PRIO 0x80
@@ -169,8 +168,7 @@ static void sh_intc_locate(struct intc_desc *desc,
             return;
         }
     }
-
-    abort();
+    g_assert_not_reached();
 }
 
 static void sh_intc_toggle_mask(struct intc_desc *desc, intc_enum id,
@@ -241,7 +239,7 @@ static void sh_intc_write(void *opaque, hwaddr offset,
         value = *valuep & ~value;
         break;
     default:
-        abort();
+        g_assert_not_reached();
     }
 
     for (k = 0; k <= first; k++) {
-- 
2.21.4

Re: [PATCH v4 16/23] hw/intc/sh_intc: Replace abort() with g_assert_not_reached()

[Philippe Mathieu-Daudé]

On 10/28/21 21:27, BALATON Zoltan wrote:
> All the places that call abort should not happen which is better
> marked by g_assert_not_reached.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  hw/intc/sh_intc.c | 8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)

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

[PATCH v4 08/23] 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 definition.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 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

[PATCH v4 05/23] 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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 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 6ffab376d8..b93b403555 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);
@@ -203,7 +203,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
@@ -305,12 +305,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);
@@ -319,13 +319,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) {
@@ -335,7 +335,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;
@@ -365,7 +365,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);
     }
@@ -386,9 +386,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

[PATCH v4 12/23] hw/intc/sh_intc: Move sh_intc_register() closer to its only user

[BALATON Zoltan]

The sh_intc_register() function is only used at one place. Move them
together so it's easier to see what's going on.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/intc/sh_intc.c | 60 +++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
index 763ebbfec2..54803bc2ca 100644
--- a/hw/intc/sh_intc.c
+++ b/hw/intc/sh_intc.c
@@ -270,36 +270,6 @@ struct intc_source *sh_intc_source(struct intc_desc *desc, intc_enum id)
     return NULL;
 }
 
-static unsigned int sh_intc_register(MemoryRegion *sysmem,
-                                     struct intc_desc *desc,
-                                     const unsigned long address,
-                                     const char *type,
-                                     const char *action,
-                                     const unsigned int index)
-{
-    char name[60];
-    MemoryRegion *iomem, *iomem_p4, *iomem_a7;
-
-    if (!address) {
-        return 0;
-    }
-
-    iomem = &desc->iomem;
-    iomem_p4 = desc->iomem_aliases + index;
-    iomem_a7 = iomem_p4 + 1;
-
-    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), "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);
-
-    /* used to increment aliases index */
-    return 2;
-}
-
 static void sh_intc_register_source(struct intc_desc *desc,
                                     intc_enum source,
                                     struct intc_group *groups,
@@ -399,6 +369,36 @@ void sh_intc_register_sources(struct intc_desc *desc,
     }
 }
 
+static unsigned int sh_intc_register(MemoryRegion *sysmem,
+                                     struct intc_desc *desc,
+                                     const unsigned long address,
+                                     const char *type,
+                                     const char *action,
+                                     const unsigned int index)
+{
+    char name[60];
+    MemoryRegion *iomem, *iomem_p4, *iomem_a7;
+
+    if (!address) {
+        return 0;
+    }
+
+    iomem = &desc->iomem;
+    iomem_p4 = desc->iomem_aliases + index;
+    iomem_a7 = iomem_p4 + 1;
+
+    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), "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);
+
+    /* used to increment aliases index */
+    return 2;
+}
+
 int sh_intc_init(MemoryRegion *sysmem,
                  struct intc_desc *desc,
                  int nr_sources,
-- 
2.21.4

Re: [PATCH v4 12/23] hw/intc/sh_intc: Move sh_intc_register() closer to its only user

[Philippe Mathieu-Daudé]

On 10/28/21 21:27, BALATON Zoltan wrote:
> The sh_intc_register() function is only used at one place. Move them
> together so it's easier to see what's going on.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  hw/intc/sh_intc.c | 60 +++++++++++++++++++++++------------------------
>  1 file changed, 30 insertions(+), 30 deletions(-)

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

[PATCH v4 07/23] 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>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 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 0af5d7a228..3f0a1cec7c 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);
@@ -211,6 +212,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
@@ -303,7 +305,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)) {
         hw_error("sh_serial: unsupported read from 0x%02"HWADDR_PRIx"\n", offs);
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

[PATCH v4 10/23] hw/intc/sh_intc: Rename iomem region

[BALATON Zoltan]

Rename the iomem region 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 hw/intc/sh_intc.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/hw/intc/sh_intc.c b/hw/intc/sh_intc.c
index 18461ff554..e386372b6f 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,8 @@ 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",
+                          0x100000000ULL);
 
 #define INT_REG_PARAMS(reg_struct, type, action, j) \
         reg_struct->action##_reg, #type, #action, j
-- 
2.21.4

Re: [PATCH v4 10/23] hw/intc/sh_intc: Rename iomem region

[Philippe Mathieu-Daudé]

On 10/28/21 21:27, BALATON Zoltan wrote:
> Rename the iomem region 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>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  hw/intc/sh_intc.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)

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

[PATCH v4 09/23] hw/intc/sh_intc: Turn some defines into an enum

[BALATON Zoltan]

Turn the INTC_MODE defines into an enum and clean up the function
returning these to make it clearer by removing nested ifs and
superfluous parenthesis. The one remaining #define is a flag which is
moved further apart by changing its value from 8 to 0x80 to leave some
spare bits as this is or-ed with the enum value at some places.

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

[PATCH v4 02/23] hw/char/sh_serial: Use hw_error instead of fprintf and abort

[BALATON Zoltan]

It does the same with dumping some more state but avoids calling abort
directly and printing to stderr from the device model.

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

diff --git a/hw/char/sh_serial.c b/hw/char/sh_serial.c
index 1b1e6a6a04..dbefb51d71 100644
--- a/hw/char/sh_serial.c
+++ b/hw/char/sh_serial.c
@@ -26,6 +26,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "hw/hw.h"
 #include "hw/irq.h"
 #include "hw/sh4/sh.h"
 #include "chardev/char-fe.h"
@@ -200,9 +201,7 @@ static void sh_serial_write(void *opaque, hwaddr offs,
         }
     }
 
-    fprintf(stderr, "sh_serial: unsupported write to 0x%02"
-            HWADDR_PRIx "\n", offs);
-    abort();
+    hw_error("sh_serial: unsupported write to 0x%02"HWADDR_PRIx"\n", offs);
 }
 
 static uint64_t sh_serial_read(void *opaque, hwaddr offs,
@@ -307,9 +306,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"
-                HWADDR_PRIx "\n", offs);
-        abort();
+        hw_error("sh_serial: unsupported read from 0x%02"HWADDR_PRIx"\n", offs);
     }
 
     return ret;
-- 
2.21.4

Re: [PATCH v4 02/23] hw/char/sh_serial: Use hw_error instead of fprintf and abort

[Philippe Mathieu-Daudé]

On 10/28/21 21:27, BALATON Zoltan wrote:
> It does the same with dumping some more state but avoids calling abort
> directly and printing to stderr from the device model.

hw_error() is unfortunately misnamed, it is meant for CPU code,
and we want to get ride of it. What you probably want here is
error_report() which also reports to the monitor.

> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
>  hw/char/sh_serial.c | 9 +++------
>  1 file changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/char/sh_serial.c b/hw/char/sh_serial.c
> index 1b1e6a6a04..dbefb51d71 100644
> --- a/hw/char/sh_serial.c
> +++ b/hw/char/sh_serial.c
> @@ -26,6 +26,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "hw/hw.h"
>  #include "hw/irq.h"
>  #include "hw/sh4/sh.h"
>  #include "chardev/char-fe.h"
> @@ -200,9 +201,7 @@ static void sh_serial_write(void *opaque, hwaddr offs,
>          }
>      }
>  
> -    fprintf(stderr, "sh_serial: unsupported write to 0x%02"
> -            HWADDR_PRIx "\n", offs);
> -    abort();
> +    hw_error("sh_serial: unsupported write to 0x%02"HWADDR_PRIx"\n", offs);
>  }
>  
>  static uint64_t sh_serial_read(void *opaque, hwaddr offs,
> @@ -307,9 +306,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"
> -                HWADDR_PRIx "\n", offs);
> -        abort();
> +        hw_error("sh_serial: unsupported read from 0x%02"HWADDR_PRIx"\n", offs);
>      }
>  
>      return ret;
> 

Re: [PATCH v4 02/23] hw/char/sh_serial: Use hw_error instead of fprintf and abort

[Thomas Huth]

On 29/10/2021 07.38, Philippe Mathieu-Daudé wrote:
> On 10/28/21 21:27, BALATON Zoltan wrote:
>> It does the same with dumping some more state but avoids calling abort
>> directly and printing to stderr from the device model.
> 
> hw_error() is unfortunately misnamed, it is meant for CPU code,
> and we want to get ride of it. What you probably want here is
> error_report() which also reports to the monitor.

Looking at the text of the messages, maybe it would be even better to use 
qemu_log_mask(LOG_UNIMP, ...) or qemu_log_mask(LOG_GUEST_ERROR, ...) ?

  Thomas


>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>   hw/char/sh_serial.c | 9 +++------
>>   1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/char/sh_serial.c b/hw/char/sh_serial.c
>> index 1b1e6a6a04..dbefb51d71 100644
>> --- a/hw/char/sh_serial.c
>> +++ b/hw/char/sh_serial.c
>> @@ -26,6 +26,7 @@
>>    */
>>   
>>   #include "qemu/osdep.h"
>> +#include "hw/hw.h"
>>   #include "hw/irq.h"
>>   #include "hw/sh4/sh.h"
>>   #include "chardev/char-fe.h"
>> @@ -200,9 +201,7 @@ static void sh_serial_write(void *opaque, hwaddr offs,
>>           }
>>       }
>>   
>> -    fprintf(stderr, "sh_serial: unsupported write to 0x%02"
>> -            HWADDR_PRIx "\n", offs);
>> -    abort();
>> +    hw_error("sh_serial: unsupported write to 0x%02"HWADDR_PRIx"\n", offs);
>>   }
>>   
>>   static uint64_t sh_serial_read(void *opaque, hwaddr offs,
>> @@ -307,9 +306,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"
>> -                HWADDR_PRIx "\n", offs);
>> -        abort();
>> +        hw_error("sh_serial: unsupported read from 0x%02"HWADDR_PRIx"\n", offs);
>>       }
>>   
>>       return ret;
>>
> 

Re: [PATCH v4 02/23] hw/char/sh_serial: Use hw_error instead of fprintf and abort

[BALATON Zoltan]

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

On Fri, 29 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/28/21 21:27, BALATON Zoltan wrote:
>> It does the same with dumping some more state but avoids calling abort
>> directly and printing to stderr from the device model.
>
> hw_error() is unfortunately misnamed, it is meant for CPU code,
> and we want to get ride of it. What you probably want here is
> error_report() which also reports to the monitor.

OK, I'll drop the abort and make these qemu_log_mask UNIMP or GUEST_ERROR, 
have to check docs if these are valid or not otherwise.

Regards,
BALATON Zoltan

>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>>  hw/char/sh_serial.c | 9 +++------
>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/char/sh_serial.c b/hw/char/sh_serial.c
>> index 1b1e6a6a04..dbefb51d71 100644
>> --- a/hw/char/sh_serial.c
>> +++ b/hw/char/sh_serial.c
>> @@ -26,6 +26,7 @@
>>   */
>>
>>  #include "qemu/osdep.h"
>> +#include "hw/hw.h"
>>  #include "hw/irq.h"
>>  #include "hw/sh4/sh.h"
>>  #include "chardev/char-fe.h"
>> @@ -200,9 +201,7 @@ static void sh_serial_write(void *opaque, hwaddr offs,
>>          }
>>      }
>>
>> -    fprintf(stderr, "sh_serial: unsupported write to 0x%02"
>> -            HWADDR_PRIx "\n", offs);
>> -    abort();
>> +    hw_error("sh_serial: unsupported write to 0x%02"HWADDR_PRIx"\n", offs);
>>  }
>>
>>  static uint64_t sh_serial_read(void *opaque, hwaddr offs,
>> @@ -307,9 +306,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"
>> -                HWADDR_PRIx "\n", offs);
>> -        abort();
>> +        hw_error("sh_serial: unsupported read from 0x%02"HWADDR_PRIx"\n", offs);
>>      }
>>
>>      return ret;
>>
>
>

[PATCH v4 06/23] 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 b93b403555..0af5d7a228 100644
--- a/hw/char/sh_serial.c
+++ b/hw/char/sh_serial.c
@@ -27,7 +27,11 @@
 
 #include "qemu/osdep.h"
 #include "hw/hw.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)
 {
@@ -349,11 +356,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);
                 }
             }
@@ -377,18 +384,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;
 
@@ -397,38 +396,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 6c702d627c..22016de664 100644
--- a/hw/sh4/sh7750.c
+++ b/hw/sh4/sh7750.c
@@ -24,9 +24,13 @@
  */
 
 #include "qemu/osdep.h"
+#include "qapi/error.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"
@@ -762,6 +766,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;
@@ -807,21 +814,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

Re: [PATCH v4 06/23] hw/char/sh_serial: QOM-ify

[Philippe Mathieu-Daudé]

On 10/28/21 21:27, BALATON Zoltan wrote:
> 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(-)

> +OBJECT_DECLARE_SIMPLE_TYPE(SHSerialState, SH_SERIAL)
> +
> +struct SHSerialState {
> +    SysBusDevice parent;
[...]
> -} SHSerialState;
> +};
> +
> +typedef struct {} SHSerialStateClass;

OBJECT_DECLARE_TYPE()?

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

Can you extract sh_serial_reset() in a previous patch?

>  {
> -    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;
>  
> @@ -397,38 +396,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);

Keep that, ...

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

... and these lines become one single sysbus_init_mmio() ...

> -
> -    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;
> +}

> @@ -762,6 +766,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;
> @@ -807,21 +814,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);

... then you can replace the aliases by 2 sysbus_mmio_map() calls.

> +    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]);
>  

Re: [PATCH v4 06/23] hw/char/sh_serial: QOM-ify

[BALATON Zoltan]

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

On Fri, 29 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/28/21 21:27, BALATON Zoltan wrote:
>> 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(-)
>
>> +OBJECT_DECLARE_SIMPLE_TYPE(SHSerialState, SH_SERIAL)
>> +
>> +struct SHSerialState {
>> +    SysBusDevice parent;
> [...]
>> -} SHSerialState;
>> +};
>> +
>> +typedef struct {} SHSerialStateClass;
>
> OBJECT_DECLARE_TYPE()?

From include/qom/object.h:
  * OBJECT_DECLARE_SIMPLE_TYPE:
[...]
  * This does the same as OBJECT_DECLARE_TYPE(), but with no class struct
  * declared.
  *
  * This macro should be used unless the class struct needs to have
  * virtual methods declared.

I think we're rather missing OBJECT_DEFINE_SIMPLE_TYPE. A lot of current 
object definitions are open coded because of that and could be replaced if 
we had that simple variant but we don't, so this is the shortest way for 
now.

>> -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)
>
> Can you extract sh_serial_reset() in a previous patch?

I could.

>>  {
>> -    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;
>>
>> @@ -397,38 +396,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);
>
> Keep that, ...
>
>> -    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);
>
> ... and these lines become one single sysbus_init_mmio() ...

Not sure about that. The device doesn't really have two io regions, they 
just appear twice due to how the CPU maps them. So I'd keep a single MMIO 
region here but could map one directly and use only one alias for the 
other instead. (That would get rid of either serial-a7 or serial-p4 with 
the other just called serial or actually sci/scif after this series).

Regards,
BALATON Zoltan

>> -
>> -    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;
>> +}
>
>> @@ -762,6 +766,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;
>> @@ -807,21 +814,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);
>
> ... then you can replace the aliases by 2 sysbus_mmio_map() calls.
>
>> +    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]);
>>
>
>

Re: [PATCH v4 06/23] hw/char/sh_serial: QOM-ify

[Philippe Mathieu-Daudé]

On 10/29/21 14:15, BALATON Zoltan wrote:
> On Fri, 29 Oct 2021, Philippe Mathieu-Daudé wrote:
>> On 10/28/21 21:27, BALATON Zoltan wrote:
>>> 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(-)
>>
>>> +OBJECT_DECLARE_SIMPLE_TYPE(SHSerialState, SH_SERIAL)
>>> +
>>> +struct SHSerialState {
>>> +    SysBusDevice parent;
>> [...]
>>> -} SHSerialState;
>>> +};
>>> +
>>> +typedef struct {} SHSerialStateClass;
>>
>> OBJECT_DECLARE_TYPE()?
> 
> From include/qom/object.h:
>  * OBJECT_DECLARE_SIMPLE_TYPE:
> [...]
>  * This does the same as OBJECT_DECLARE_TYPE(), but with no class struct
>  * declared.
>  *
>  * This macro should be used unless the class struct needs to have
>  * virtual methods declared.
> 
> I think we're rather missing OBJECT_DEFINE_SIMPLE_TYPE. A lot of current
> object definitions are open coded because of that and could be replaced
> if we had that simple variant but we don't, so this is the shortest way
> for now.
> 
>>> -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)
>>
>> Can you extract sh_serial_reset() in a previous patch?
> 
> I could.
> 
>>>  {
>>> -    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;
>>>
>>> @@ -397,38 +396,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);
>>
>> Keep that, ...
>>
>>> -    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);
>>
>> ... and these lines become one single sysbus_init_mmio() ...
> 
> Not sure about that. The device doesn't really have two io regions, they
> just appear twice due to how the CPU maps them. So I'd keep a single
> MMIO region here but could map one directly and use only one alias for
> the other instead. (That would get rid of either serial-a7 or serial-p4
> with the other just called serial or actually sci/scif after this series).

Looking at the current mapping:

memory-region: system
  0000000000000000-ffffffffffffffff (prio 0, i/o): system
    0000000000000000-0000000000ffffff (prio 0, romd): r2d.flash
    0000000004000000-000000000400003f (prio 0, i/o): r2d-fpga
    000000000c000000-000000000fffffff (prio 0, ram): r2d.sdram
    0000000010000000-00000000107fffff (prio 0, ram): sm501.local
    0000000013e00000-0000000013ffffff (prio 0, i/o): sm501.mmio
      0000000013e00000-0000000013e0006b (prio 0, i/o): sm501-system-config
      0000000013e10040-0000000013e10053 (prio 0, i/o): sm501-i2c
      0000000013e30000-0000000013e3001f (prio 0, i/o): serial
      0000000013e40000-0000000013e400ff (prio 0, i/o): ohci
      0000000013e80000-0000000013e80fff (prio 0, i/o): sm501-disp-ctrl
      0000000013f00000-0000000013f00053 (prio 0, i/o): sm501-2d-engine
    000000001400080c-000000001400080f (prio 0, i/o): ide-mmio.2
    0000000014001000-000000001400101f (prio 0, i/o): ide-mmio.1
    000000001e080000-000000001e080003 (prio 0, i/o): alias
interrupt-controller-prio-set-a7 @interrupt-controller
000000001e080000-000000001e080003
    000000001e080040-000000001e080043 (prio 0, i/o): alias
interrupt-controller-mask-set-a7 @interrupt-controller
000000001e080040-000000001e080043
    000000001e080060-000000001e080063 (prio 0, i/o): alias
interrupt-controller-mask-clr-a7 @interrupt-controller
000000001e080060-000000001e080063
    000000001e100000-000000001e100fff (prio 0, i/o): alias timer-a7
@timer 0000000000000000-0000000000000fff
    000000001e200000-000000001e200223 (prio 0, i/o): alias sh_pci.2
@sh_pci 0000000000000000-0000000000000223
    000000001f000000-000000001f000fff (prio 0, i/o): alias memory-1f0
@memory 000000001f000000-000000001f000fff
    000000001f800000-000000001f800fff (prio 0, i/o): alias memory-1f8
@memory 000000001f800000-000000001f800fff
    000000001fc00000-000000001fc00fff (prio 0, i/o): alias memory-1fc
@memory 000000001fc00000-000000001fc00fff
    000000001fd00004-000000001fd00007 (prio 0, i/o): alias
interrupt-controller-prio-set-a7 @interrupt-controller
000000001fd00004-000000001fd00007
    000000001fd00008-000000001fd0000b (prio 0, i/o): alias
interrupt-controller-prio-set-a7 @interrupt-controller
000000001fd00008-000000001fd0000b
    000000001fd0000c-000000001fd0000f (prio 0, i/o): alias
interrupt-controller-prio-set-a7 @interrupt-controller
000000001fd0000c-000000001fd0000f
    000000001fd00010-000000001fd00013 (prio 0, i/o): alias
interrupt-controller-prio-set-a7 @interrupt-controller
000000001fd00010-000000001fd00013
    000000001fd80000-000000001fd80fff (prio 0, i/o): alias timer-a7
@timer 0000000000000000-0000000000000fff
    000000001fe00000-000000001fe00027 (prio 0, i/o): alias serial-a7
@serial 0000000000000000-0000000000000027
    000000001fe80000-000000001fe80027 (prio 0, i/o): alias serial-a7
@serial 0000000000000000-0000000000000027
    00000000f0000000-00000000f7ffffff (prio 0, i/o): cache-and-tlb
    00000000fe080000-00000000fe080003 (prio 0, i/o): alias
interrupt-controller-prio-set-p4 @interrupt-controller
000000001e080000-000000001e080003
    00000000fe080040-00000000fe080043 (prio 0, i/o): alias
interrupt-controller-mask-set-p4 @interrupt-controller
000000001e080040-000000001e080043
    00000000fe080060-00000000fe080063 (prio 0, i/o): alias
interrupt-controller-mask-clr-p4 @interrupt-controller
000000001e080060-000000001e080063
    00000000fe100000-00000000fe100fff (prio 0, i/o): alias timer-p4
@timer 0000000000000000-0000000000000fff
    00000000fe200000-00000000fe200223 (prio 0, i/o): sh_pci
    00000000fe240000-00000000fe27ffff (prio 0, i/o): alias sh_pci.isa
@io 0000000000000000-000000000003ffff
    00000000ff000000-00000000ff000fff (prio 0, i/o): alias memory-ff0
@memory 000000001f000000-000000001f000fff
    00000000ff800000-00000000ff800fff (prio 0, i/o): alias memory-ff8
@memory 000000001f800000-000000001f800fff
    00000000ffc00000-00000000ffc00fff (prio 0, i/o): alias memory-ffc
@memory 000000001fc00000-000000001fc00fff
    00000000ffd00004-00000000ffd00007 (prio 0, i/o): alias
interrupt-controller-prio-set-p4 @interrupt-controller
000000001fd00004-000000001fd00007
    00000000ffd00008-00000000ffd0000b (prio 0, i/o): alias
interrupt-controller-prio-set-p4 @interrupt-controller
000000001fd00008-000000001fd0000b
    00000000ffd0000c-00000000ffd0000f (prio 0, i/o): alias
interrupt-controller-prio-set-p4 @interrupt-controller
000000001fd0000c-000000001fd0000f
    00000000ffd00010-00000000ffd00013 (prio 0, i/o): alias
interrupt-controller-prio-set-p4 @interrupt-controller
000000001fd00010-000000001fd00013
    00000000ffd80000-00000000ffd80fff (prio 0, i/o): alias timer-p4
@timer 0000000000000000-0000000000000fff
    00000000ffe00000-00000000ffe00027 (prio 0, i/o): alias serial-p4
@serial 0000000000000000-0000000000000027
    00000000ffe80000-00000000ffe80027 (prio 0, i/o): alias serial-p4
@serial 0000000000000000-0000000000000027

It seems the 32MiB container region in 0x1e000000-0x1fffffff is
aliased to 0xfe000000-0xffffffff. But I haven't looked at the
datasheet (and don't have time until next week).

Re: [PATCH v4 06/23] hw/char/sh_serial: QOM-ify

[BALATON Zoltan]

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

On Fri, 29 Oct 2021, Philippe Mathieu-Daudé wrote:
> On 10/29/21 14:15, BALATON Zoltan wrote:
>> On Fri, 29 Oct 2021, Philippe Mathieu-Daudé wrote:
>>> On 10/28/21 21:27, BALATON Zoltan wrote:
>>>> 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(-)
>>>
>>>> +OBJECT_DECLARE_SIMPLE_TYPE(SHSerialState, SH_SERIAL)
>>>> +
>>>> +struct SHSerialState {
>>>> +    SysBusDevice parent;
>>> [...]
>>>> -} SHSerialState;
>>>> +};
>>>> +
>>>> +typedef struct {} SHSerialStateClass;
>>>
>>> OBJECT_DECLARE_TYPE()?
>>
>> From include/qom/object.h:
>>  * OBJECT_DECLARE_SIMPLE_TYPE:
>> [...]
>>  * This does the same as OBJECT_DECLARE_TYPE(), but with no class struct
>>  * declared.
>>  *
>>  * This macro should be used unless the class struct needs to have
>>  * virtual methods declared.
>>
>> I think we're rather missing OBJECT_DEFINE_SIMPLE_TYPE. A lot of current
>> object definitions are open coded because of that and could be replaced
>> if we had that simple variant but we don't, so this is the shortest way
>> for now.
>>
>>>> -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)
>>>
>>> Can you extract sh_serial_reset() in a previous patch?
>>
>> I could.
>>
>>>>  {
>>>> -    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;
>>>>
>>>> @@ -397,38 +396,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);
>>>
>>> Keep that, ...
>>>
>>>> -    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);
>>>
>>> ... and these lines become one single sysbus_init_mmio() ...
>>
>> Not sure about that. The device doesn't really have two io regions, they
>> just appear twice due to how the CPU maps them. So I'd keep a single
>> MMIO region here but could map one directly and use only one alias for
>> the other instead. (That would get rid of either serial-a7 or serial-p4
>> with the other just called serial or actually sci/scif after this series).
>
> Looking at the current mapping:
>
> memory-region: system
>  0000000000000000-ffffffffffffffff (prio 0, i/o): system
>    0000000000000000-0000000000ffffff (prio 0, romd): r2d.flash
>    0000000004000000-000000000400003f (prio 0, i/o): r2d-fpga
>    000000000c000000-000000000fffffff (prio 0, ram): r2d.sdram
>    0000000010000000-00000000107fffff (prio 0, ram): sm501.local
>    0000000013e00000-0000000013ffffff (prio 0, i/o): sm501.mmio
>      0000000013e00000-0000000013e0006b (prio 0, i/o): sm501-system-config
>      0000000013e10040-0000000013e10053 (prio 0, i/o): sm501-i2c
>      0000000013e30000-0000000013e3001f (prio 0, i/o): serial
>      0000000013e40000-0000000013e400ff (prio 0, i/o): ohci
>      0000000013e80000-0000000013e80fff (prio 0, i/o): sm501-disp-ctrl
>      0000000013f00000-0000000013f00053 (prio 0, i/o): sm501-2d-engine
>    000000001400080c-000000001400080f (prio 0, i/o): ide-mmio.2
>    0000000014001000-000000001400101f (prio 0, i/o): ide-mmio.1
>    000000001e080000-000000001e080003 (prio 0, i/o): alias
> interrupt-controller-prio-set-a7 @interrupt-controller
> 000000001e080000-000000001e080003
>    000000001e080040-000000001e080043 (prio 0, i/o): alias
> interrupt-controller-mask-set-a7 @interrupt-controller
> 000000001e080040-000000001e080043
>    000000001e080060-000000001e080063 (prio 0, i/o): alias
> interrupt-controller-mask-clr-a7 @interrupt-controller
> 000000001e080060-000000001e080063
>    000000001e100000-000000001e100fff (prio 0, i/o): alias timer-a7
> @timer 0000000000000000-0000000000000fff
>    000000001e200000-000000001e200223 (prio 0, i/o): alias sh_pci.2
> @sh_pci 0000000000000000-0000000000000223
>    000000001f000000-000000001f000fff (prio 0, i/o): alias memory-1f0
> @memory 000000001f000000-000000001f000fff
>    000000001f800000-000000001f800fff (prio 0, i/o): alias memory-1f8
> @memory 000000001f800000-000000001f800fff
>    000000001fc00000-000000001fc00fff (prio 0, i/o): alias memory-1fc
> @memory 000000001fc00000-000000001fc00fff
>    000000001fd00004-000000001fd00007 (prio 0, i/o): alias
> interrupt-controller-prio-set-a7 @interrupt-controller
> 000000001fd00004-000000001fd00007
>    000000001fd00008-000000001fd0000b (prio 0, i/o): alias
> interrupt-controller-prio-set-a7 @interrupt-controller
> 000000001fd00008-000000001fd0000b
>    000000001fd0000c-000000001fd0000f (prio 0, i/o): alias
> interrupt-controller-prio-set-a7 @interrupt-controller
> 000000001fd0000c-000000001fd0000f
>    000000001fd00010-000000001fd00013 (prio 0, i/o): alias
> interrupt-controller-prio-set-a7 @interrupt-controller
> 000000001fd00010-000000001fd00013
>    000000001fd80000-000000001fd80fff (prio 0, i/o): alias timer-a7
> @timer 0000000000000000-0000000000000fff
>    000000001fe00000-000000001fe00027 (prio 0, i/o): alias serial-a7
> @serial 0000000000000000-0000000000000027
>    000000001fe80000-000000001fe80027 (prio 0, i/o): alias serial-a7
> @serial 0000000000000000-0000000000000027
>    00000000f0000000-00000000f7ffffff (prio 0, i/o): cache-and-tlb
>    00000000fe080000-00000000fe080003 (prio 0, i/o): alias
> interrupt-controller-prio-set-p4 @interrupt-controller
> 000000001e080000-000000001e080003
>    00000000fe080040-00000000fe080043 (prio 0, i/o): alias
> interrupt-controller-mask-set-p4 @interrupt-controller
> 000000001e080040-000000001e080043
>    00000000fe080060-00000000fe080063 (prio 0, i/o): alias
> interrupt-controller-mask-clr-p4 @interrupt-controller
> 000000001e080060-000000001e080063
>    00000000fe100000-00000000fe100fff (prio 0, i/o): alias timer-p4
> @timer 0000000000000000-0000000000000fff
>    00000000fe200000-00000000fe200223 (prio 0, i/o): sh_pci
>    00000000fe240000-00000000fe27ffff (prio 0, i/o): alias sh_pci.isa
> @io 0000000000000000-000000000003ffff
>    00000000ff000000-00000000ff000fff (prio 0, i/o): alias memory-ff0
> @memory 000000001f000000-000000001f000fff
>    00000000ff800000-00000000ff800fff (prio 0, i/o): alias memory-ff8
> @memory 000000001f800000-000000001f800fff
>    00000000ffc00000-00000000ffc00fff (prio 0, i/o): alias memory-ffc
> @memory 000000001fc00000-000000001fc00fff
>    00000000ffd00004-00000000ffd00007 (prio 0, i/o): alias
> interrupt-controller-prio-set-p4 @interrupt-controller
> 000000001fd00004-000000001fd00007
>    00000000ffd00008-00000000ffd0000b (prio 0, i/o): alias
> interrupt-controller-prio-set-p4 @interrupt-controller
> 000000001fd00008-000000001fd0000b
>    00000000ffd0000c-00000000ffd0000f (prio 0, i/o): alias
> interrupt-controller-prio-set-p4 @interrupt-controller
> 000000001fd0000c-000000001fd0000f
>    00000000ffd00010-00000000ffd00013 (prio 0, i/o): alias
> interrupt-controller-prio-set-p4 @interrupt-controller
> 000000001fd00010-000000001fd00013
>    00000000ffd80000-00000000ffd80fff (prio 0, i/o): alias timer-p4
> @timer 0000000000000000-0000000000000fff
>    00000000ffe00000-00000000ffe00027 (prio 0, i/o): alias serial-p4
> @serial 0000000000000000-0000000000000027
>    00000000ffe80000-00000000ffe80027 (prio 0, i/o): alias serial-p4
> @serial 0000000000000000-0000000000000027
>
> It seems the 32MiB container region in 0x1e000000-0x1fffffff is
> aliased to 0xfe000000-0xffffffff. But I haven't looked at the
> datasheet (and don't have time until next week).

All regs are available at two addresses. The P regions are similar to 
MIPS' ksegs, the A7 is only accessible via MMU while P4 is I think only in 
protected mode but I also don't know all the details so I'd just leave it 
as it is for now. This could be cleaned up later in separete patches, I 
think there will be more clean ups needed as I've also found missing 
functionality that I plan to look at later but I'd like the clean ups 
merged before the freeze at least so I have less patches to roll in my 
tree.

Regards,
BALATON Zoltan