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