Re: [PATCH v3 12/20] nubus: move nubus to its own 32-bit address space

[BALATON Zoltan]

On Thu, 16 Sep 2021, Mark Cave-Ayland wrote:
> On 16/09/2021 13:48, BALATON Zoltan wrote:
>
> > On Thu, 16 Sep 2021, Mark Cave-Ayland wrote:
> > > According to "Designing Cards and Drivers for the Macintosh Family" the 
> > > Nubus
> > > has its own 32-bit address space based upon physical slot addressing.
> > >
> > > Move Nubus to its own 32-bit address space and then use memory region 
> > > aliases
> > > to map available slot and super slot ranges into the q800 system address
> > > space via the Macintosh Nubus bridge.
> > >
> > > Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> > > Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > > ---
> > > hw/m68k/q800.c                      |  8 +++-----
> > > hw/nubus/mac-nubus-bridge.c         | 15 +++++++++++++--
> > > hw/nubus/nubus-bus.c                | 18 ++++++++++++++++++
> > > hw/nubus/nubus-device.c             |  2 +-
> > > include/hw/nubus/mac-nubus-bridge.h |  2 ++
> > > include/hw/nubus/nubus.h            | 10 +++++++---
> > > 6 files changed, 44 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> > > index 5ba87f789c..0a0051a296 100644
> > > --- a/hw/m68k/q800.c
> > > +++ b/hw/m68k/q800.c
> > > @@ -67,9 +67,6 @@
> > > #define ASC_BASE              (IO_BASE + 0x14000)
> > > #define SWIM_BASE             (IO_BASE + 0x1E000)
> > >
> > > -#define NUBUS_SUPER_SLOT_BASE 0x60000000
> > > -#define NUBUS_SLOT_BASE       0xf0000000
> > > -
> > > #define SONIC_PROM_SIZE       0x1000
> > >
> > > /*
> > > @@ -396,8 +393,9 @@ static void q800_init(MachineState *machine)
> > >
> > >     dev = qdev_new(TYPE_MAC_NUBUS_BRIDGE);
> > >     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
> > > -    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, NUBUS_SUPER_SLOT_BASE);
> > > -    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, NUBUS_SLOT_BASE);
> > > +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, 9 * NUBUS_SUPER_SLOT_SIZE);
> > > +    sysbus_mmio_map(SYS_BUS_DEVICE(dev), 1, NUBUS_SLOT_BASE +
> > > +                                            9 * NUBUS_SLOT_SIZE);
> > >
> > >     nubus = MAC_NUBUS_BRIDGE(dev)->bus;
> > >
> > > diff --git a/hw/nubus/mac-nubus-bridge.c b/hw/nubus/mac-nubus-bridge.c
> > > index c1d77e2bc7..574bc7831e 100644
> > > --- a/hw/nubus/mac-nubus-bridge.c
> > > +++ b/hw/nubus/mac-nubus-bridge.c
> > > @@ -21,8 +21,19 @@ static void mac_nubus_bridge_init(Object *obj)
> > >     /* Macintosh only has slots 0x9 to 0xe available */
> > >     s->bus->slot_available_mask = MAKE_64BIT_MASK(9, 6);
> > >
> > > -    sysbus_init_mmio(sbd, &s->bus->super_slot_io);
> > > -    sysbus_init_mmio(sbd, &s->bus->slot_io);
> > > +    /* Aliases for slots 0x9 to 0xe */
> > > +    memory_region_init_alias(&s->super_slot_alias, obj, 
> > > "super-slot-alias",
> > > +                             &s->bus->nubus_mr,
> > > +                             9 * NUBUS_SUPER_SLOT_SIZE,
> > > +                             6 * NUBUS_SUPER_SLOT_SIZE);
> >
> > Sorry for not spotting it yesterday in v2 but I only had time to have a 
> > closer look now. Do these 9 and 6 worth a #define? Are these something like 
> > MAC_FIST_SLOT and MAC_NUM_SLOTS? As they maybe always appear together with 
> > NUBUS_SUPER_SLOT_SIZE (I haven't checked all but most look like that) so 
> > those products could have a #define just to make it shorter in these calls. 
> > (Are those the #defines that you've removed above?) Maybe
> >
> > #define MAC_FIRST_SLOT 9
> > #define MAC_NUM_SLOTS  6
> >
> > then use these to
> >
> > #define MAC_SLOTS_MASK  MAKE_64BIT_MASK(MAC_FIRST_SLOT, MAC_NUM_SLOTS)
> >
> > and similarly the memory address and size as
> >
> > #define MAC_SLOT_BASE  9 * NUBUS_SUPER_SLOT_SIZE #define MAC_SLOT_SIZE  6 * 
> > NUBUS_SUPER_SLOT_SIZE
> >
> > or so and then use these latter three where they appear now open coded 
> > could be shorter and clearer but I don't mind either way so if you want to 
> > keep the current version that's OK with me as well. (I may have got the 
> > names for these wrong but hopefully you get the idea, I haven't tried to 
> > understand in detail what these really are.)
> >
> > Regards,
> > BALATON Zoltan
>
> I'm not amazingly sold on this since it introduces another layer of 
> indirection: in its current form you can immediately correlate all of mask, 
> offset, and size with the comment without having to look up another set of 
> constants first.

As I said I'm OK with it as it as too.

Regards,
BALATON Zoltan