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

[Mark Cave-Ayland]

On 25/05/2023 09:12, Philippe Mathieu-Daudé wrote:

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

That's a fair comment. In fact it seems that even outside of this series q800.c could 
do with some better QOM parenting.

It's reasonably trivial to fix up the QOM tree within this series, although of course 
it will make it a bit larger. Let me see if I can fix this for v2.

> >       MemoryRegion macio;
> >       MemoryRegion macio_alias;
> >   };


ATB,

Mark.