Re: [PATCH 09/30] q800: add djMEMC memory controller

[Philippe Mathieu-Daudé]

On 24/5/23 23:10, Mark Cave-Ayland wrote:

The djMEMC controller is used to store information related to the physical 
memory
configuration.

Co-developed-by: Laurent Vivier <laur...@vivier.eu>
Signed-off-by: Mark Cave-Ayland <mark.cave-ayl...@ilande.co.uk>
---
  MAINTAINERS              |   2 +
  hw/m68k/Kconfig          |   1 +
  hw/m68k/q800.c           |   9 +++
  hw/misc/Kconfig          |   3 +
  hw/misc/djmemc.c         | 154 +++++++++++++++++++++++++++++++++++++++
  hw/misc/meson.build      |   1 +
  hw/misc/trace-events     |   4 +
  include/hw/m68k/q800.h   |   2 +
  include/hw/misc/djmemc.h |  46 ++++++++++++
  9 files changed, 222 insertions(+)
  create mode 100644 hw/misc/djmemc.c
  create mode 100644 include/hw/misc/djmemc.h

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index f15f1eaff9..456407898e 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -40,6 +40,7 @@
  #include "bootinfo.h"
  #include "hw/m68k/q800.h"
  #include "hw/misc/mac_via.h"
+#include "hw/misc/djmemc.h"
  #include "hw/input/adb.h"
  #include "hw/nubus/mac-nubus-bridge.h"
  #include "hw/display/macfb.h"
@@ -66,6 +67,7 @@
  #define SONIC_PROM_BASE       (IO_BASE + 0x08000)
  #define SONIC_BASE            (IO_BASE + 0x0a000)
  #define SCC_BASE              (IO_BASE + 0x0c020)
+#define DJMEMC_BASE           (IO_BASE + 0x0e000)
  #define ESP_BASE              (IO_BASE + 0x10000)
  #define ESP_PDMA              (IO_BASE + 0x10100)
  #define ASC_BASE              (IO_BASE + 0x14000)
@@ -492,6 +494,13 @@ static void q800_machine_init(MachineState *machine)
                               &error_abort);
      sysbus_realize_and_unref(SYS_BUS_DEVICE(m->glue), &error_fatal);

+ /* djMEMC memory controller */

+    m->djmemc = qdev_new(TYPE_DJMEMC);
+    sysbus = SYS_BUS_DEVICE(m->djmemc);
+    sysbus_realize_and_unref(sysbus, &error_fatal);
+    memory_region_add_subregion(&m->macio, DJMEMC_BASE - IO_BASE,
+                                sysbus_mmio_get_region(sysbus, 0));

diff --git a/include/hw/m68k/q800.h b/include/hw/m68k/q800.h
index 8d788a7072..d0e37cc665 100644
--- a/include/hw/m68k/q800.h
+++ b/include/hw/m68k/q800.h
@@ -33,6 +33,8 @@ struct Q800MachineState {
      M68kCPU *cpu;
      MemoryRegion rom;
      DeviceState *glue;
+    DeviceState *djmemc;

While I like the simplicity of using pointer to common QOM parent
type, isn't the consensus to have QOM objects embed their children
state? Maybe we never agreed on that explicitly :) So here I'd rather:

        DJMEMCState djmemc;

      MemoryRegion macio;
      MemoryRegion macio_alias;
  };