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

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

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 <laurent@vivier.eu>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@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;
>   };