[PATCH 0/8] q800: GLUE updates for A/UX mode

[PATCH 0/8] q800: GLUE updates for A/UX mode

[Mark Cave-Ayland]

This patchset contains a set of GLUE updates to enable switching between
so-called A/UX mode and classic mode which is required to allow the q800
machine to boot both Linux/NetBSD and MacOS.

Patch 1 updates the comment for the VIA1B_vMystery bit indicating that
VIA1 port B bit 6 is used to switch between A/UX and classic mode.

Patch 2 moves the VIA1 IRQ from level 1 to level 6: this is because in
its current configuration the q800 machine is statically wired in A/UX mode
except for VIA1 which is currently still wired according to classic mode.

Patch 3 starts the process of abstracting the CPU IRQ levels from the GPIO
pins by using a set of numbered GPIO defines and using these to map to the
corresponding CPU IRQs accordingly.

Patches 4 and 5 add a GPIO to VIA1 so that updates to the VIA1B_vMystery bit
will set the auxmode variable in the GLUE device to facilitate dynamic CPU
IRQ routing.

Patches 6 and 7 adjust the CPU IRQ routing in GLUE_set_irq() so that the
incoming IRQs are mapped to the appropriate CPU IRQ levels dynamically based
upon the auxmode variable.

Finally patch 8 adds an NMI handler which can be used to invoke the Quadra's
programmer switch from the monitor for debugging.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


Mark Cave-Ayland (8):
  mac_via: update comment for VIA1B_vMystery bit
  q800: move VIA1 IRQ from level 1 to level 6
  q800: use GLUE IRQ numbers instead of IRQ level for GLUE IRQs
  mac_via: add GPIO for A/UX mode
  q800: wire up auxmode GPIO to GLUE
  q800: route SONIC on-board Ethernet IRQ via nubus IRQ 9 in classic
    mode
  q800: wire up remaining IRQs in classic mode
  q800: add NMI handler

 hw/m68k/q800.c            | 174 +++++++++++++++++++++++++++++++++++++-
 hw/misc/mac_via.c         |  22 +++++
 hw/misc/trace-events      |   1 +
 include/hw/misc/mac_via.h |   1 +
 4 files changed, 194 insertions(+), 4 deletions(-)

-- 
2.20.1

[PATCH 1/8] mac_via: update comment for VIA1B_vMystery bit

[Mark Cave-Ayland]

According to both Linux and NetBSD, port B bit 6 is used on the Quadra 800 to
configure the GLUE logic in A/UX mode. Whilst the name VIA1B_vMystery isn't
particularly descriptive, the patch leaves this to ensure that the constants
in mac_via.c remain in sync with Linux's mac_via.h.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/mac_via.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index 993bac017d..7a53a8b4c0 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -130,6 +130,10 @@
                                 * On SE/30, vertical sync interrupt enable.
                                 * 0=enabled. This vSync interrupt shows up
                                 * as a slot $E interrupt.
+                                * On Quadra 800 this bit toggles A/UX mode which
+                                * configures the glue logic to deliver some IRQs
+                                * at different levels compared to a classic
+                                * Mac.
                                 */
 #define VIA1B_vADBS2   0x20    /* ADB state input bit 1 (unused on IIfx) */
 #define VIA1B_vADBS1   0x10    /* ADB state input bit 0 (unused on IIfx) */
-- 
2.20.1

Re: [PATCH 1/8] mac_via: update comment for VIA1B_vMystery bit

[Laurent Vivier]

Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
> According to both Linux and NetBSD, port B bit 6 is used on the Quadra 800 to
> configure the GLUE logic in A/UX mode. Whilst the name VIA1B_vMystery isn't
> particularly descriptive, the patch leaves this to ensure that the constants
> in mac_via.c remain in sync with Linux's mac_via.h.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/misc/mac_via.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> index 993bac017d..7a53a8b4c0 100644
> --- a/hw/misc/mac_via.c
> +++ b/hw/misc/mac_via.c
> @@ -130,6 +130,10 @@
>                                  * On SE/30, vertical sync interrupt enable.
>                                  * 0=enabled. This vSync interrupt shows up
>                                  * as a slot $E interrupt.
> +                                * On Quadra 800 this bit toggles A/UX mode which
> +                                * configures the glue logic to deliver some IRQs
> +                                * at different levels compared to a classic
> +                                * Mac.
>                                  */
>  #define VIA1B_vADBS2   0x20    /* ADB state input bit 1 (unused on IIfx) */
>  #define VIA1B_vADBS1   0x10    /* ADB state input bit 0 (unused on IIfx) */
> 

I'm not sure we need to keep in sync a word that neither used by linux or qemu (for now).

Perhaps you can put a word from where it is (NetBSD)?

It seems it's supported by all Quadra, not only Quadra 800 (see NetBSD comment in mac68k/machdep.c

Anyway:

Reviewed-by: Laurent Vivier <laurent@vivier.Eu>

Re: [PATCH 1/8] mac_via: update comment for VIA1B_vMystery bit

[Mark Cave-Ayland]

On 15/10/2021 07:14, Laurent Vivier wrote:

> Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
>> According to both Linux and NetBSD, port B bit 6 is used on the Quadra 800 to
>> configure the GLUE logic in A/UX mode. Whilst the name VIA1B_vMystery isn't
>> particularly descriptive, the patch leaves this to ensure that the constants
>> in mac_via.c remain in sync with Linux's mac_via.h.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/misc/mac_via.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>> index 993bac017d..7a53a8b4c0 100644
>> --- a/hw/misc/mac_via.c
>> +++ b/hw/misc/mac_via.c
>> @@ -130,6 +130,10 @@
>>                                   * On SE/30, vertical sync interrupt enable.
>>                                   * 0=enabled. This vSync interrupt shows up
>>                                   * as a slot $E interrupt.
>> +                                * On Quadra 800 this bit toggles A/UX mode which
>> +                                * configures the glue logic to deliver some IRQs
>> +                                * at different levels compared to a classic
>> +                                * Mac.
>>                                   */
>>   #define VIA1B_vADBS2   0x20    /* ADB state input bit 1 (unused on IIfx) */
>>   #define VIA1B_vADBS1   0x10    /* ADB state input bit 0 (unused on IIfx) */
>>
> 
> I'm not sure we need to keep in sync a word that neither used by linux or qemu (for now).
> 
> Perhaps you can put a word from where it is (NetBSD)?
> 
> It seems it's supported by all Quadra, not only Quadra 800 (see NetBSD comment in mac68k/machdep.c
> 
> Anyway:
> 
> Reviewed-by: Laurent Vivier <laurent@vivier.Eu>

Yeah, it's a hard one: you can see the bit referenced in the via_alt_mapping logic in 
Linux but mac_via.h still shows it as VIA1B_vMystery (and indeed, it seems to have 
different uses on different machines). The thought behind adding the comment was to 
better explain the purpose of VIA1B_vMystery within the series.


ATB,

Mark.

[PATCH 2/8] q800: move VIA1 IRQ from level 1 to level 6

[Mark Cave-Ayland]

On a Quadra 800 machine Linux sets via_alt_mapping to 1 and clears port B bit 6 to
ensure that the VIA1 IRQ is delivered at level 6 rather than level 1. Even though
QEMU doesn't yet emulate this behaviour, Linux still installs the VIA1 level 1 IRQ
handler regardless of the value of via_alt_mapping which is why the kernel has been
able to boot until now.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/q800.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index fd4855047e..15f3067811 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -284,7 +284,7 @@ static void q800_init(MachineState *machine)
     sysbus = SYS_BUS_DEVICE(via1_dev);
     sysbus_realize_and_unref(sysbus, &error_fatal);
     sysbus_mmio_map(sysbus, 1, VIA_BASE);
-    sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(glue, 0));
+    sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(glue, 5));
 
     adb_bus = qdev_get_child_bus(via1_dev, "adb.0");
     dev = qdev_new(TYPE_ADB_KEYBOARD);
-- 
2.20.1

Re: [PATCH 2/8] q800: move VIA1 IRQ from level 1 to level 6

[Laurent Vivier]

Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
> On a Quadra 800 machine Linux sets via_alt_mapping to 1 and clears port B bit 6 to
> ensure that the VIA1 IRQ is delivered at level 6 rather than level 1. Even though
> QEMU doesn't yet emulate this behaviour, Linux still installs the VIA1 level 1 IRQ
> handler regardless of the value of via_alt_mapping which is why the kernel has been
> able to boot until now.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/q800.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index fd4855047e..15f3067811 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -284,7 +284,7 @@ static void q800_init(MachineState *machine)
>      sysbus = SYS_BUS_DEVICE(via1_dev);
>      sysbus_realize_and_unref(sysbus, &error_fatal);
>      sysbus_mmio_map(sysbus, 1, VIA_BASE);
> -    sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(glue, 0));
> +    sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(glue, 5));
>  
>      adb_bus = qdev_get_child_bus(via1_dev, "adb.0");
>      dev = qdev_new(TYPE_ADB_KEYBOARD);
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

[PATCH 3/8] q800: use GLUE IRQ numbers instead of IRQ level for GLUE IRQs

[Mark Cave-Ayland]

In order to allow dynamic routing of IRQs to different IRQ levels on the CPU
depending upon port B bit 6, use GLUE IRQ numbers and map them to the the
corresponding CPU IRQ level accordingly.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/q800.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 15f3067811..81c335bf16 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -102,11 +102,34 @@ struct GLUEState {
     uint8_t ipr;
 };
 
+#define GLUE_IRQ_IN_VIA1       0
+#define GLUE_IRQ_IN_VIA2       1
+#define GLUE_IRQ_IN_SONIC      2
+#define GLUE_IRQ_IN_ESCC       3
+
 static void GLUE_set_irq(void *opaque, int irq, int level)
 {
     GLUEState *s = opaque;
     int i;
 
+    switch (irq) {
+    case GLUE_IRQ_IN_VIA1:
+        irq = 5;
+        break;
+
+    case GLUE_IRQ_IN_VIA2:
+        irq = 1;
+        break;
+
+    case GLUE_IRQ_IN_SONIC:
+        irq = 2;
+        break;
+
+    case GLUE_IRQ_IN_ESCC:
+        irq = 3;
+        break;
+    }
+
     if (level) {
         s->ipr |= 1 << irq;
     } else {
@@ -284,7 +307,7 @@ static void q800_init(MachineState *machine)
     sysbus = SYS_BUS_DEVICE(via1_dev);
     sysbus_realize_and_unref(sysbus, &error_fatal);
     sysbus_mmio_map(sysbus, 1, VIA_BASE);
-    sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(glue, 5));
+    sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(glue, GLUE_IRQ_IN_VIA1));
 
     adb_bus = qdev_get_child_bus(via1_dev, "adb.0");
     dev = qdev_new(TYPE_ADB_KEYBOARD);
@@ -297,7 +320,7 @@ static void q800_init(MachineState *machine)
     sysbus = SYS_BUS_DEVICE(via2_dev);
     sysbus_realize_and_unref(sysbus, &error_fatal);
     sysbus_mmio_map(sysbus, 1, VIA_BASE + VIA_SIZE);
-    sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(glue, 1));
+    sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(glue, GLUE_IRQ_IN_VIA2));
 
     /* MACSONIC */
 
@@ -330,7 +353,7 @@ static void q800_init(MachineState *machine)
     sysbus = SYS_BUS_DEVICE(dev);
     sysbus_realize_and_unref(sysbus, &error_fatal);
     sysbus_mmio_map(sysbus, 0, SONIC_BASE);
-    sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(glue, 2));
+    sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(glue, GLUE_IRQ_IN_SONIC));
 
     memory_region_init_rom(dp8393x_prom, NULL, "dp8393x-q800.prom",
                            SONIC_PROM_SIZE, &error_fatal);
@@ -366,7 +389,8 @@ static void q800_init(MachineState *machine)
     qdev_realize_and_unref(escc_orgate, NULL, &error_fatal);
     sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(escc_orgate, 0));
     sysbus_connect_irq(sysbus, 1, qdev_get_gpio_in(escc_orgate, 1));
-    qdev_connect_gpio_out(DEVICE(escc_orgate), 0, qdev_get_gpio_in(glue, 3));
+    qdev_connect_gpio_out(DEVICE(escc_orgate), 0,
+                          qdev_get_gpio_in(glue, GLUE_IRQ_IN_ESCC));
     sysbus_mmio_map(sysbus, 0, SCC_BASE);
 
     /* SCSI */
-- 
2.20.1

Re: [PATCH 3/8] q800: use GLUE IRQ numbers instead of IRQ level for GLUE IRQs

[Laurent Vivier]

Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
> In order to allow dynamic routing of IRQs to different IRQ levels on the CPU
> depending upon port B bit 6, use GLUE IRQ numbers and map them to the the
> corresponding CPU IRQ level accordingly.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/q800.c | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index 15f3067811..81c335bf16 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -102,11 +102,34 @@ struct GLUEState {
>      uint8_t ipr;
>  };
>  
> +#define GLUE_IRQ_IN_VIA1       0
> +#define GLUE_IRQ_IN_VIA2       1
> +#define GLUE_IRQ_IN_SONIC      2
> +#define GLUE_IRQ_IN_ESCC       3
> +
>  static void GLUE_set_irq(void *opaque, int irq, int level)
>  {
>      GLUEState *s = opaque;
>      int i;
>  
> +    switch (irq) {
> +    case GLUE_IRQ_IN_VIA1:
> +        irq = 5;
> +        break;

Perhaps you can move this patch before patch 2 to help to understand why GLUE_IRQ_IN_VIA1 (0) is
mapped to irq 5 (before patch 2 it would be to 0).

> +
> +    case GLUE_IRQ_IN_VIA2:
> +        irq = 1;
> +        break;
> +
> +    case GLUE_IRQ_IN_SONIC:
> +        irq = 2;
> +        break;
> +
> +    case GLUE_IRQ_IN_ESCC:
> +        irq = 3;
> +        break;
> +    }
> +
>      if (level) {
>          s->ipr |= 1 << irq;

perhaps you can rename here "irq" to "shift"?

Thanks,
Laurent

Re: [PATCH 3/8] q800: use GLUE IRQ numbers instead of IRQ level for GLUE IRQs

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 1690 bytes --]

On Fri, 15 Oct 2021, Laurent Vivier wrote:
> Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
>> In order to allow dynamic routing of IRQs to different IRQ levels on the CPU
>> depending upon port B bit 6, use GLUE IRQ numbers and map them to the the
>> corresponding CPU IRQ level accordingly.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>  hw/m68k/q800.c | 32 ++++++++++++++++++++++++++++----
>>  1 file changed, 28 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
>> index 15f3067811..81c335bf16 100644
>> --- a/hw/m68k/q800.c
>> +++ b/hw/m68k/q800.c
>> @@ -102,11 +102,34 @@ struct GLUEState {
>>      uint8_t ipr;
>>  };
>>
>> +#define GLUE_IRQ_IN_VIA1       0
>> +#define GLUE_IRQ_IN_VIA2       1
>> +#define GLUE_IRQ_IN_SONIC      2
>> +#define GLUE_IRQ_IN_ESCC       3
>> +
>>  static void GLUE_set_irq(void *opaque, int irq, int level)
>>  {
>>      GLUEState *s = opaque;
>>      int i;
>>
>> +    switch (irq) {
>> +    case GLUE_IRQ_IN_VIA1:
>> +        irq = 5;
>> +        break;
>
> Perhaps you can move this patch before patch 2 to help to understand why GLUE_IRQ_IN_VIA1 (0) is
> mapped to irq 5 (before patch 2 it would be to 0).
>
>> +
>> +    case GLUE_IRQ_IN_VIA2:
>> +        irq = 1;
>> +        break;
>> +
>> +    case GLUE_IRQ_IN_SONIC:
>> +        irq = 2;
>> +        break;
>> +
>> +    case GLUE_IRQ_IN_ESCC:
>> +        irq = 3;
>> +        break;
>> +    }
>> +
>>      if (level) {
>>          s->ipr |= 1 << irq;
>
> perhaps you can rename here "irq" to "shift"?

I think if it's the irq number calling it irq is clearer than shift. Maybe 
use BIT(irq) instead?

Regards,
BALATON Zoltan

Re: [PATCH 3/8] q800: use GLUE IRQ numbers instead of IRQ level for GLUE IRQs

[Mark Cave-Ayland]

On 15/10/2021 07:31, Laurent Vivier wrote:

> Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
>> In order to allow dynamic routing of IRQs to different IRQ levels on the CPU
>> depending upon port B bit 6, use GLUE IRQ numbers and map them to the the
>> corresponding CPU IRQ level accordingly.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/m68k/q800.c | 32 ++++++++++++++++++++++++++++----
>>   1 file changed, 28 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
>> index 15f3067811..81c335bf16 100644
>> --- a/hw/m68k/q800.c
>> +++ b/hw/m68k/q800.c
>> @@ -102,11 +102,34 @@ struct GLUEState {
>>       uint8_t ipr;
>>   };
>>   
>> +#define GLUE_IRQ_IN_VIA1       0
>> +#define GLUE_IRQ_IN_VIA2       1
>> +#define GLUE_IRQ_IN_SONIC      2
>> +#define GLUE_IRQ_IN_ESCC       3
>> +
>>   static void GLUE_set_irq(void *opaque, int irq, int level)
>>   {
>>       GLUEState *s = opaque;
>>       int i;
>>   
>> +    switch (irq) {
>> +    case GLUE_IRQ_IN_VIA1:
>> +        irq = 5;
>> +        break;
> 
> Perhaps you can move this patch before patch 2 to help to understand why GLUE_IRQ_IN_VIA1 (0) is
> mapped to irq 5 (before patch 2 it would be to 0).

I think it should stay in the existing order because patch 2 is really a bug fix: all 
of the other IRQs are statically wired in A/UX mode except for VIA1. Once this is 
fixed, this patch then abstracts the *input* IRQs away from the CPU level so they can 
be swizzled independently depending upon whether A/UX mode is selected.

>> +
>> +    case GLUE_IRQ_IN_VIA2:
>> +        irq = 1;
>> +        break;
>> +
>> +    case GLUE_IRQ_IN_SONIC:
>> +        irq = 2;
>> +        break;
>> +
>> +    case GLUE_IRQ_IN_ESCC:
>> +        irq = 3;
>> +        break;
>> +    }
>> +
>>       if (level) {
>>           s->ipr |= 1 << irq;
> 
> perhaps you can rename here "irq" to "shift"?

At this point it is the CPU level IRQ so "irq" seems correct here. Are you thinking 
that using a different intermediate variable from the function parameter would help?


ATB,

Mark.

Re: [PATCH 3/8] q800: use GLUE IRQ numbers instead of IRQ level for GLUE IRQs

[Mark Cave-Ayland]

On 15/10/2021 07:31, Laurent Vivier wrote:

> Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
>> In order to allow dynamic routing of IRQs to different IRQ levels on the CPU
>> depending upon port B bit 6, use GLUE IRQ numbers and map them to the the
>> corresponding CPU IRQ level accordingly.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/m68k/q800.c | 32 ++++++++++++++++++++++++++++----
>>   1 file changed, 28 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
>> index 15f3067811..81c335bf16 100644
>> --- a/hw/m68k/q800.c
>> +++ b/hw/m68k/q800.c
>> @@ -102,11 +102,34 @@ struct GLUEState {
>>       uint8_t ipr;
>>   };
>>   
>> +#define GLUE_IRQ_IN_VIA1       0
>> +#define GLUE_IRQ_IN_VIA2       1
>> +#define GLUE_IRQ_IN_SONIC      2
>> +#define GLUE_IRQ_IN_ESCC       3
>> +
>>   static void GLUE_set_irq(void *opaque, int irq, int level)
>>   {
>>       GLUEState *s = opaque;
>>       int i;
>>   
>> +    switch (irq) {
>> +    case GLUE_IRQ_IN_VIA1:
>> +        irq = 5;
>> +        break;
> 
> Perhaps you can move this patch before patch 2 to help to understand why GLUE_IRQ_IN_VIA1 (0) is
> mapped to irq 5 (before patch 2 it would be to 0).
> 
>> +
>> +    case GLUE_IRQ_IN_VIA2:
>> +        irq = 1;
>> +        break;
>> +
>> +    case GLUE_IRQ_IN_SONIC:
>> +        irq = 2;
>> +        break;
>> +
>> +    case GLUE_IRQ_IN_ESCC:
>> +        irq = 3;
>> +        break;
>> +    }
>> +
>>       if (level) {
>>           s->ipr |= 1 << irq;
> 
> perhaps you can rename here "irq" to "shift"?

Were you happy to leave this as irq? Another alternative may be to use the BIT() 
macro as suggested by Zoltan.


ATB,

Mark.

Re: [PATCH 3/8] q800: use GLUE IRQ numbers instead of IRQ level for GLUE IRQs

[Laurent Vivier]

Le 17/10/2021 à 11:40, Mark Cave-Ayland a écrit :
> On 15/10/2021 07:31, Laurent Vivier wrote:
> 
>> Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
>>> In order to allow dynamic routing of IRQs to different IRQ levels on the CPU
>>> depending upon port B bit 6, use GLUE IRQ numbers and map them to the the
>>> corresponding CPU IRQ level accordingly.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>   hw/m68k/q800.c | 32 ++++++++++++++++++++++++++++----
>>>   1 file changed, 28 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
>>> index 15f3067811..81c335bf16 100644
>>> --- a/hw/m68k/q800.c
>>> +++ b/hw/m68k/q800.c
>>> @@ -102,11 +102,34 @@ struct GLUEState {
>>>       uint8_t ipr;
>>>   };
>>>   +#define GLUE_IRQ_IN_VIA1       0
>>> +#define GLUE_IRQ_IN_VIA2       1
>>> +#define GLUE_IRQ_IN_SONIC      2
>>> +#define GLUE_IRQ_IN_ESCC       3
>>> +
>>>   static void GLUE_set_irq(void *opaque, int irq, int level)
>>>   {
>>>       GLUEState *s = opaque;
>>>       int i;
>>>   +    switch (irq) {
>>> +    case GLUE_IRQ_IN_VIA1:
>>> +        irq = 5;
>>> +        break;
>>
>> Perhaps you can move this patch before patch 2 to help to understand why GLUE_IRQ_IN_VIA1 (0) is
>> mapped to irq 5 (before patch 2 it would be to 0).
>>
>>> +
>>> +    case GLUE_IRQ_IN_VIA2:
>>> +        irq = 1;
>>> +        break;
>>> +
>>> +    case GLUE_IRQ_IN_SONIC:
>>> +        irq = 2;
>>> +        break;
>>> +
>>> +    case GLUE_IRQ_IN_ESCC:
>>> +        irq = 3;
>>> +        break;
>>> +    }
>>> +
>>>       if (level) {
>>>           s->ipr |= 1 << irq;
>>
>> perhaps you can rename here "irq" to "shift"?
> 
> Were you happy to leave this as irq? Another alternative may be to use the BIT() macro as suggested
> by Zoltan.

I have no problem to keep this like that.

Thanks,
Laurent

[PATCH 4/8] mac_via: add GPIO for A/UX mode

[Mark Cave-Ayland]

Add a new auxmode GPIO that is updated when port B bit 6 is changed indicating
whether the hardware is configured for A/UX mode.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/mac_via.c         | 18 ++++++++++++++++++
 hw/misc/trace-events      |  1 +
 include/hw/misc/mac_via.h |  1 +
 3 files changed, 20 insertions(+)

diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index 7a53a8b4c0..a08ffbcd88 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -880,6 +880,20 @@ static void via1_adb_update(MOS6522Q800VIA1State *v1s)
     }
 }
 
+static void via1_auxmode_update(MOS6522Q800VIA1State *v1s)
+{
+    MOS6522State *s = MOS6522(v1s);
+    int oldirq, irq;
+
+    oldirq = (v1s->last_b & VIA1B_vMystery) ? 1 : 0;
+    irq = (s->b & VIA1B_vMystery) ? 1 : 0;
+
+    if (irq != oldirq) {
+        trace_via1_auxmode(irq);
+        qemu_set_irq(v1s->auxmode_irq, irq);
+    }
+}
+
 static uint64_t mos6522_q800_via1_read(void *opaque, hwaddr addr, unsigned size)
 {
     MOS6522Q800VIA1State *s = MOS6522_Q800_VIA1(opaque);
@@ -902,6 +916,7 @@ static void mos6522_q800_via1_write(void *opaque, hwaddr addr, uint64_t val,
     case VIA_REG_B:
         via1_rtc_update(v1s);
         via1_adb_update(v1s);
+        via1_auxmode_update(v1s);
 
         v1s->last_b = ms->b;
         break;
@@ -1046,6 +1061,9 @@ static void mos6522_q800_via1_init(Object *obj)
               TYPE_ADB_BUS, DEVICE(v1s), "adb.0");
 
     qdev_init_gpio_in(DEVICE(obj), via1_irq_request, VIA1_IRQ_NB);
+
+    /* A/UX mode */
+    qdev_init_gpio_out(DEVICE(obj), &v1s->auxmode_irq, 1);
 }
 
 static const VMStateDescription vmstate_q800_via1 = {
diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index ede413965b..2da96d167a 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -228,6 +228,7 @@ via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "secto
 via1_adb_send(const char *state, uint8_t data, const char *vadbint) "state %s data=0x%02x vADBInt=%s"
 via1_adb_receive(const char *state, uint8_t data, const char *vadbint, int status, int index, int size) "state %s data=0x%02x vADBInt=%s status=0x%x index=%d size=%d"
 via1_adb_poll(uint8_t data, const char *vadbint, int status, int index, int size) "data=0x%02x vADBInt=%s status=0x%x index=%d size=%d"
+via1_auxmode(int mode) "setting auxmode to %d"
 
 # grlib_ahb_apb_pnp.c
 grlib_ahb_pnp_read(uint64_t addr, uint32_t value) "AHB PnP read addr:0x%03"PRIx64" data:0x%08x"
diff --git a/include/hw/misc/mac_via.h b/include/hw/misc/mac_via.h
index 4506abe5d0..b445565866 100644
--- a/include/hw/misc/mac_via.h
+++ b/include/hw/misc/mac_via.h
@@ -43,6 +43,7 @@ struct MOS6522Q800VIA1State {
     MemoryRegion via_mem;
 
     qemu_irq irqs[VIA1_IRQ_NB];
+    qemu_irq auxmode_irq;
     uint8_t last_b;
 
     /* RTC */
-- 
2.20.1

Re: [PATCH 4/8] mac_via: add GPIO for A/UX mode

[Laurent Vivier]

Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
> Add a new auxmode GPIO that is updated when port B bit 6 is changed indicating
> whether the hardware is configured for A/UX mode.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/misc/mac_via.c         | 18 ++++++++++++++++++
>  hw/misc/trace-events      |  1 +
>  include/hw/misc/mac_via.h |  1 +
>  3 files changed, 20 insertions(+)
> 
> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> index 7a53a8b4c0..a08ffbcd88 100644
> --- a/hw/misc/mac_via.c
> +++ b/hw/misc/mac_via.c
> @@ -880,6 +880,20 @@ static void via1_adb_update(MOS6522Q800VIA1State *v1s)
>      }
>  }
>  
> +static void via1_auxmode_update(MOS6522Q800VIA1State *v1s)
> +{
> +    MOS6522State *s = MOS6522(v1s);
> +    int oldirq, irq;
> +

Please, add a comment to explain what happens here as "vMystery" is not self-explicit.

> +    oldirq = (v1s->last_b & VIA1B_vMystery) ? 1 : 0;
> +    irq = (s->b & VIA1B_vMystery) ? 1 : 0;

For me, it would be clearer with:

    oldirq = !!(v1s->last_b & VIA1B_vMystery);
    irq = !!(s->b & VIA1B_vMystery);

but it's a matter of taste.


> +
> +    if (irq != oldirq) {
> +        trace_via1_auxmode(irq);
> +        qemu_set_irq(v1s->auxmode_irq, irq);
> +    }
> +}
> +
>  static uint64_t mos6522_q800_via1_read(void *opaque, hwaddr addr, unsigned size)
>  {
>      MOS6522Q800VIA1State *s = MOS6522_Q800_VIA1(opaque);
> @@ -902,6 +916,7 @@ static void mos6522_q800_via1_write(void *opaque, hwaddr addr, uint64_t val,
>      case VIA_REG_B:
>          via1_rtc_update(v1s);
>          via1_adb_update(v1s);
> +        via1_auxmode_update(v1s);
>  
>          v1s->last_b = ms->b;
>          break;
> @@ -1046,6 +1061,9 @@ static void mos6522_q800_via1_init(Object *obj)
>                TYPE_ADB_BUS, DEVICE(v1s), "adb.0");
>  
>      qdev_init_gpio_in(DEVICE(obj), via1_irq_request, VIA1_IRQ_NB);
> +
> +    /* A/UX mode */
> +    qdev_init_gpio_out(DEVICE(obj), &v1s->auxmode_irq, 1);
>  }
>  
>  static const VMStateDescription vmstate_q800_via1 = {
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index ede413965b..2da96d167a 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -228,6 +228,7 @@ via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "secto
>  via1_adb_send(const char *state, uint8_t data, const char *vadbint) "state %s data=0x%02x vADBInt=%s"
>  via1_adb_receive(const char *state, uint8_t data, const char *vadbint, int status, int index, int size) "state %s data=0x%02x vADBInt=%s status=0x%x index=%d size=%d"
>  via1_adb_poll(uint8_t data, const char *vadbint, int status, int index, int size) "data=0x%02x vADBInt=%s status=0x%x index=%d size=%d"
> +via1_auxmode(int mode) "setting auxmode to %d"
>  
>  # grlib_ahb_apb_pnp.c
>  grlib_ahb_pnp_read(uint64_t addr, uint32_t value) "AHB PnP read addr:0x%03"PRIx64" data:0x%08x"
> diff --git a/include/hw/misc/mac_via.h b/include/hw/misc/mac_via.h
> index 4506abe5d0..b445565866 100644
> --- a/include/hw/misc/mac_via.h
> +++ b/include/hw/misc/mac_via.h
> @@ -43,6 +43,7 @@ struct MOS6522Q800VIA1State {
>      MemoryRegion via_mem;
>  
>      qemu_irq irqs[VIA1_IRQ_NB];
> +    qemu_irq auxmode_irq;
>      uint8_t last_b;
>  
>      /* RTC */
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 4/8] mac_via: add GPIO for A/UX mode

[Mark Cave-Ayland]

On 15/10/2021 07:58, Laurent Vivier wrote:

> Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
>> Add a new auxmode GPIO that is updated when port B bit 6 is changed indicating
>> whether the hardware is configured for A/UX mode.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/misc/mac_via.c         | 18 ++++++++++++++++++
>>   hw/misc/trace-events      |  1 +
>>   include/hw/misc/mac_via.h |  1 +
>>   3 files changed, 20 insertions(+)
>>
>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>> index 7a53a8b4c0..a08ffbcd88 100644
>> --- a/hw/misc/mac_via.c
>> +++ b/hw/misc/mac_via.c
>> @@ -880,6 +880,20 @@ static void via1_adb_update(MOS6522Q800VIA1State *v1s)
>>       }
>>   }
>>   
>> +static void via1_auxmode_update(MOS6522Q800VIA1State *v1s)
>> +{
>> +    MOS6522State *s = MOS6522(v1s);
>> +    int oldirq, irq;
>> +
> 
> Please, add a comment to explain what happens here as "vMystery" is not self-explicit.

Would something simple like:

/* Check to see if the A/UX mode bit has changed */

suffice here?

>> +    oldirq = (v1s->last_b & VIA1B_vMystery) ? 1 : 0;
>> +    irq = (s->b & VIA1B_vMystery) ? 1 : 0;
> 
> For me, it would be clearer with:
> 
>      oldirq = !!(v1s->last_b & VIA1B_vMystery);
>      irq = !!(s->b & VIA1B_vMystery);
> 
> but it's a matter of taste.

I had to think carefully about that one :)  If you're fine with the existing version 
I'd prefer to keep it as I find it easier to read.

>> +
>> +    if (irq != oldirq) {
>> +        trace_via1_auxmode(irq);
>> +        qemu_set_irq(v1s->auxmode_irq, irq);
>> +    }
>> +}
>> +
>>   static uint64_t mos6522_q800_via1_read(void *opaque, hwaddr addr, unsigned size)
>>   {
>>       MOS6522Q800VIA1State *s = MOS6522_Q800_VIA1(opaque);
>> @@ -902,6 +916,7 @@ static void mos6522_q800_via1_write(void *opaque, hwaddr addr, uint64_t val,
>>       case VIA_REG_B:
>>           via1_rtc_update(v1s);
>>           via1_adb_update(v1s);
>> +        via1_auxmode_update(v1s);
>>   
>>           v1s->last_b = ms->b;
>>           break;
>> @@ -1046,6 +1061,9 @@ static void mos6522_q800_via1_init(Object *obj)
>>                 TYPE_ADB_BUS, DEVICE(v1s), "adb.0");
>>   
>>       qdev_init_gpio_in(DEVICE(obj), via1_irq_request, VIA1_IRQ_NB);
>> +
>> +    /* A/UX mode */
>> +    qdev_init_gpio_out(DEVICE(obj), &v1s->auxmode_irq, 1);
>>   }
>>   
>>   static const VMStateDescription vmstate_q800_via1 = {
>> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
>> index ede413965b..2da96d167a 100644
>> --- a/hw/misc/trace-events
>> +++ b/hw/misc/trace-events
>> @@ -228,6 +228,7 @@ via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "secto
>>   via1_adb_send(const char *state, uint8_t data, const char *vadbint) "state %s data=0x%02x vADBInt=%s"
>>   via1_adb_receive(const char *state, uint8_t data, const char *vadbint, int status, int index, int size) "state %s data=0x%02x vADBInt=%s status=0x%x index=%d size=%d"
>>   via1_adb_poll(uint8_t data, const char *vadbint, int status, int index, int size) "data=0x%02x vADBInt=%s status=0x%x index=%d size=%d"
>> +via1_auxmode(int mode) "setting auxmode to %d"
>>   
>>   # grlib_ahb_apb_pnp.c
>>   grlib_ahb_pnp_read(uint64_t addr, uint32_t value) "AHB PnP read addr:0x%03"PRIx64" data:0x%08x"
>> diff --git a/include/hw/misc/mac_via.h b/include/hw/misc/mac_via.h
>> index 4506abe5d0..b445565866 100644
>> --- a/include/hw/misc/mac_via.h
>> +++ b/include/hw/misc/mac_via.h
>> @@ -43,6 +43,7 @@ struct MOS6522Q800VIA1State {
>>       MemoryRegion via_mem;
>>   
>>       qemu_irq irqs[VIA1_IRQ_NB];
>> +    qemu_irq auxmode_irq;
>>       uint8_t last_b;
>>   
>>       /* RTC */
>>
> 
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>


ATB,

Mark.

Re: [PATCH 4/8] mac_via: add GPIO for A/UX mode

[Laurent Vivier]

Le 15/10/2021 à 21:50, Mark Cave-Ayland a écrit :
> On 15/10/2021 07:58, Laurent Vivier wrote:
> 
>> Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
>>> Add a new auxmode GPIO that is updated when port B bit 6 is changed indicating
>>> whether the hardware is configured for A/UX mode.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>>   hw/misc/mac_via.c         | 18 ++++++++++++++++++
>>>   hw/misc/trace-events      |  1 +
>>>   include/hw/misc/mac_via.h |  1 +
>>>   3 files changed, 20 insertions(+)
>>>
>>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>>> index 7a53a8b4c0..a08ffbcd88 100644
>>> --- a/hw/misc/mac_via.c
>>> +++ b/hw/misc/mac_via.c
>>> @@ -880,6 +880,20 @@ static void via1_adb_update(MOS6522Q800VIA1State *v1s)
>>>       }
>>>   }
>>>   +static void via1_auxmode_update(MOS6522Q800VIA1State *v1s)
>>> +{
>>> +    MOS6522State *s = MOS6522(v1s);
>>> +    int oldirq, irq;
>>> +
>>
>> Please, add a comment to explain what happens here as "vMystery" is not self-explicit.
> 
> Would something simple like:
> 
> /* Check to see if the A/UX mode bit has changed */
> 
> suffice here?

Yes

> 
>>> +    oldirq = (v1s->last_b & VIA1B_vMystery) ? 1 : 0;
>>> +    irq = (s->b & VIA1B_vMystery) ? 1 : 0;
>>
>> For me, it would be clearer with:
>>
>>      oldirq = !!(v1s->last_b & VIA1B_vMystery);
>>      irq = !!(s->b & VIA1B_vMystery);
>>
>> but it's a matter of taste.
> 
> I had to think carefully about that one :)  If you're fine with the existing version I'd prefer to
> keep it as I find it easier to read.

Up to you, as I said, a matter of taste.

> 
>>> +
>>> +    if (irq != oldirq) {
>>> +        trace_via1_auxmode(irq);
>>> +        qemu_set_irq(v1s->auxmode_irq, irq);
>>> +    }
>>> +}
>>> +
>>>   static uint64_t mos6522_q800_via1_read(void *opaque, hwaddr addr, unsigned size)
>>>   {
>>>       MOS6522Q800VIA1State *s = MOS6522_Q800_VIA1(opaque);
>>> @@ -902,6 +916,7 @@ static void mos6522_q800_via1_write(void *opaque, hwaddr addr, uint64_t val,
>>>       case VIA_REG_B:
>>>           via1_rtc_update(v1s);
>>>           via1_adb_update(v1s);
>>> +        via1_auxmode_update(v1s);
>>>             v1s->last_b = ms->b;
>>>           break;
>>> @@ -1046,6 +1061,9 @@ static void mos6522_q800_via1_init(Object *obj)
>>>                 TYPE_ADB_BUS, DEVICE(v1s), "adb.0");
>>>         qdev_init_gpio_in(DEVICE(obj), via1_irq_request, VIA1_IRQ_NB);
>>> +
>>> +    /* A/UX mode */
>>> +    qdev_init_gpio_out(DEVICE(obj), &v1s->auxmode_irq, 1);
>>>   }
>>>     static const VMStateDescription vmstate_q800_via1 = {
>>> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
>>> index ede413965b..2da96d167a 100644
>>> --- a/hw/misc/trace-events
>>> +++ b/hw/misc/trace-events
>>> @@ -228,6 +228,7 @@ via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "secto
>>>   via1_adb_send(const char *state, uint8_t data, const char *vadbint) "state %s data=0x%02x
>>> vADBInt=%s"
>>>   via1_adb_receive(const char *state, uint8_t data, const char *vadbint, int status, int index,
>>> int size) "state %s data=0x%02x vADBInt=%s status=0x%x index=%d size=%d"
>>>   via1_adb_poll(uint8_t data, const char *vadbint, int status, int index, int size) "data=0x%02x
>>> vADBInt=%s status=0x%x index=%d size=%d"
>>> +via1_auxmode(int mode) "setting auxmode to %d"
>>>     # grlib_ahb_apb_pnp.c
>>>   grlib_ahb_pnp_read(uint64_t addr, uint32_t value) "AHB PnP read addr:0x%03"PRIx64" data:0x%08x"
>>> diff --git a/include/hw/misc/mac_via.h b/include/hw/misc/mac_via.h
>>> index 4506abe5d0..b445565866 100644
>>> --- a/include/hw/misc/mac_via.h
>>> +++ b/include/hw/misc/mac_via.h
>>> @@ -43,6 +43,7 @@ struct MOS6522Q800VIA1State {
>>>       MemoryRegion via_mem;
>>>         qemu_irq irqs[VIA1_IRQ_NB];
>>> +    qemu_irq auxmode_irq;
>>>       uint8_t last_b;
>>>         /* RTC */
>>>
>>
>> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
> 
> 
> ATB,
> 
> Mark.

Re: [PATCH 4/8] mac_via: add GPIO for A/UX mode

[Laurent Vivier]

Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
> Add a new auxmode GPIO that is updated when port B bit 6 is changed indicating
> whether the hardware is configured for A/UX mode.
> 

Stupid question: why do you use GPIO to pass the auxmode information between VIA and GLUE?

Can't we use object_property_set_link() to set a pointer to the GLUE object?

Thanks,
Laurent

Re: [PATCH 4/8] mac_via: add GPIO for A/UX mode

[Mark Cave-Ayland]

On 15/10/2021 08:17, Laurent Vivier wrote:

> Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
>> Add a new auxmode GPIO that is updated when port B bit 6 is changed indicating
>> whether the hardware is configured for A/UX mode.
> 
> Stupid question: why do you use GPIO to pass the auxmode information between VIA and GLUE?
> 
> Can't we use object_property_set_link() to set a pointer to the GLUE object?

For devices that are independent i.e. not contained within others I prefer to 
restrict the interface to properties that are visible within "info qom-tree" which 
are MRs and GPIOs. Otherwise GLUE requires knowledge of VIA internals which breaks 
the device abstraction.


ATB,

Mark.

Re: [PATCH 4/8] mac_via: add GPIO for A/UX mode

[Laurent Vivier]

Le 15/10/2021 à 21:59, Mark Cave-Ayland a écrit :
> On 15/10/2021 08:17, Laurent Vivier wrote:
> 
>> Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
>>> Add a new auxmode GPIO that is updated when port B bit 6 is changed indicating
>>> whether the hardware is configured for A/UX mode.
>>
>> Stupid question: why do you use GPIO to pass the auxmode information between VIA and GLUE?
>>
>> Can't we use object_property_set_link() to set a pointer to the GLUE object?
> 
> For devices that are independent i.e. not contained within others I prefer to restrict the interface
> to properties that are visible within "info qom-tree" which are MRs and GPIOs. Otherwise GLUE
> requires knowledge of VIA internals which breaks the device abstraction.
> 

OK, makes sense.

Thanks,
Laurent

[PATCH 5/8] q800: wire up auxmode GPIO to GLUE

[Mark Cave-Ayland]

This enables the GLUE logic to change its CPU level IRQ routing depending upon
whether the hardware has been configured for A/UX mode.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/q800.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 81c335bf16..0093872d89 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -100,6 +100,7 @@ struct GLUEState {
     SysBusDevice parent_obj;
     M68kCPU *cpu;
     uint8_t ipr;
+    uint8_t auxmode;
 };
 
 #define GLUE_IRQ_IN_VIA1       0
@@ -145,11 +146,19 @@ static void GLUE_set_irq(void *opaque, int irq, int level)
     m68k_set_irq_level(s->cpu, 0, 0);
 }
 
+static void glue_auxmode_set_irq(void *opaque, int irq, int level)
+{
+    GLUEState *s = GLUE(opaque);
+
+    s->auxmode = level;
+}
+
 static void glue_reset(DeviceState *dev)
 {
     GLUEState *s = GLUE(dev);
 
     s->ipr = 0;
+    s->auxmode = 0;
 }
 
 static const VMStateDescription vmstate_glue = {
@@ -158,6 +167,7 @@ static const VMStateDescription vmstate_glue = {
     .minimum_version_id = 0,
     .fields = (VMStateField[]) {
         VMSTATE_UINT8(ipr, GLUEState),
+        VMSTATE_UINT8(auxmode, GLUEState),
         VMSTATE_END_OF_LIST(),
     },
 };
@@ -178,6 +188,7 @@ static void glue_init(Object *obj)
     DeviceState *dev = DEVICE(obj);
 
     qdev_init_gpio_in(dev, GLUE_set_irq, 8);
+    qdev_init_gpio_in_named(dev, glue_auxmode_set_irq, "auxmode", 1);
 }
 
 static void glue_class_init(ObjectClass *klass, void *data)
@@ -308,6 +319,9 @@ static void q800_init(MachineState *machine)
     sysbus_realize_and_unref(sysbus, &error_fatal);
     sysbus_mmio_map(sysbus, 1, VIA_BASE);
     sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(glue, GLUE_IRQ_IN_VIA1));
+    /* A/UX mode */
+    qdev_connect_gpio_out(via1_dev, 0,
+                          qdev_get_gpio_in_named(glue, "auxmode", 0));
 
     adb_bus = qdev_get_child_bus(via1_dev, "adb.0");
     dev = qdev_new(TYPE_ADB_KEYBOARD);
-- 
2.20.1

Re: [PATCH 5/8] q800: wire up auxmode GPIO to GLUE

[Laurent Vivier]

Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
> This enables the GLUE logic to change its CPU level IRQ routing depending upon
> whether the hardware has been configured for A/UX mode.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/q800.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index 81c335bf16..0093872d89 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -100,6 +100,7 @@ struct GLUEState {
>      SysBusDevice parent_obj;
>      M68kCPU *cpu;
>      uint8_t ipr;
> +    uint8_t auxmode;
>  };
>  
>  #define GLUE_IRQ_IN_VIA1       0
> @@ -145,11 +146,19 @@ static void GLUE_set_irq(void *opaque, int irq, int level)
>      m68k_set_irq_level(s->cpu, 0, 0);
>  }
>  
> +static void glue_auxmode_set_irq(void *opaque, int irq, int level)
> +{
> +    GLUEState *s = GLUE(opaque);
> +
> +    s->auxmode = level;
> +}
> +
>  static void glue_reset(DeviceState *dev)
>  {
>      GLUEState *s = GLUE(dev);
>  
>      s->ipr = 0;
> +    s->auxmode = 0;
>  }
>  
>  static const VMStateDescription vmstate_glue = {
> @@ -158,6 +167,7 @@ static const VMStateDescription vmstate_glue = {
>      .minimum_version_id = 0,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT8(ipr, GLUEState),
> +        VMSTATE_UINT8(auxmode, GLUEState),
>          VMSTATE_END_OF_LIST(),
>      },
>  };
> @@ -178,6 +188,7 @@ static void glue_init(Object *obj)
>      DeviceState *dev = DEVICE(obj);
>  
>      qdev_init_gpio_in(dev, GLUE_set_irq, 8);
> +    qdev_init_gpio_in_named(dev, glue_auxmode_set_irq, "auxmode", 1);
>  }
>  
>  static void glue_class_init(ObjectClass *klass, void *data)
> @@ -308,6 +319,9 @@ static void q800_init(MachineState *machine)
>      sysbus_realize_and_unref(sysbus, &error_fatal);
>      sysbus_mmio_map(sysbus, 1, VIA_BASE);
>      sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(glue, GLUE_IRQ_IN_VIA1));
> +    /* A/UX mode */
> +    qdev_connect_gpio_out(via1_dev, 0,
> +                          qdev_get_gpio_in_named(glue, "auxmode", 0));
>  
>      adb_bus = qdev_get_child_bus(via1_dev, "adb.0");
>      dev = qdev_new(TYPE_ADB_KEYBOARD);
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 5/8] q800: wire up auxmode GPIO to GLUE

[Laurent Vivier]

Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
> This enables the GLUE logic to change its CPU level IRQ routing depending upon
> whether the hardware has been configured for A/UX mode.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/q800.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index 81c335bf16..0093872d89 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -100,6 +100,7 @@ struct GLUEState {
>      SysBusDevice parent_obj;
>      M68kCPU *cpu;
>      uint8_t ipr;
> +    uint8_t auxmode;
>  };
>  
>  #define GLUE_IRQ_IN_VIA1       0
> @@ -145,11 +146,19 @@ static void GLUE_set_irq(void *opaque, int irq, int level)
>      m68k_set_irq_level(s->cpu, 0, 0);
>  }
>  
> +static void glue_auxmode_set_irq(void *opaque, int irq, int level)
> +{
> +    GLUEState *s = GLUE(opaque);
> +
> +    s->auxmode = level;
> +}
> +
>  static void glue_reset(DeviceState *dev)
>  {
>      GLUEState *s = GLUE(dev);
>  
>      s->ipr = 0;
> +    s->auxmode = 0;
>  }
>  
>  static const VMStateDescription vmstate_glue = {
> @@ -158,6 +167,7 @@ static const VMStateDescription vmstate_glue = {
>      .minimum_version_id = 0,
>      .fields = (VMStateField[]) {
>          VMSTATE_UINT8(ipr, GLUEState),
> +        VMSTATE_UINT8(auxmode, GLUEState),
>          VMSTATE_END_OF_LIST(),
>      },
>  };
> @@ -178,6 +188,7 @@ static void glue_init(Object *obj)
>      DeviceState *dev = DEVICE(obj);
>  
>      qdev_init_gpio_in(dev, GLUE_set_irq, 8);
> +    qdev_init_gpio_in_named(dev, glue_auxmode_set_irq, "auxmode", 1);
>  }
>  
>  static void glue_class_init(ObjectClass *klass, void *data)
> @@ -308,6 +319,9 @@ static void q800_init(MachineState *machine)
>      sysbus_realize_and_unref(sysbus, &error_fatal);
>      sysbus_mmio_map(sysbus, 1, VIA_BASE);
>      sysbus_connect_irq(sysbus, 0, qdev_get_gpio_in(glue, GLUE_IRQ_IN_VIA1));
> +    /* A/UX mode */
> +    qdev_connect_gpio_out(via1_dev, 0,
> +                          qdev_get_gpio_in_named(glue, "auxmode", 0));
>  
>      adb_bus = qdev_get_child_bus(via1_dev, "adb.0");
>      dev = qdev_new(TYPE_ADB_KEYBOARD);
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

[PATCH 6/8] q800: route SONIC on-board Ethernet IRQ via nubus IRQ 9 in classic mode

[Mark Cave-Ayland]

When the hardware is operating in classic mode the SONIC on-board Ethernet IRQ is
routed to nubus IRQ 9 instead of directly to the CPU at level 3. This does not
affect the framebuffer which although it exists in slot 9, has its own
dedicated IRQ on the Quadra 800 hardware.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/q800.c | 57 ++++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 46 insertions(+), 11 deletions(-)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index 0093872d89..d55e6a7541 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -101,6 +101,7 @@ struct GLUEState {
     M68kCPU *cpu;
     uint8_t ipr;
     uint8_t auxmode;
+    qemu_irq irqs[1];
 };
 
 #define GLUE_IRQ_IN_VIA1       0
@@ -108,27 +109,50 @@ struct GLUEState {
 #define GLUE_IRQ_IN_SONIC      2
 #define GLUE_IRQ_IN_ESCC       3
 
+#define GLUE_IRQ_NUBUS_9       0
+
 static void GLUE_set_irq(void *opaque, int irq, int level)
 {
     GLUEState *s = opaque;
     int i;
 
-    switch (irq) {
-    case GLUE_IRQ_IN_VIA1:
-        irq = 5;
-        break;
+    switch (s->auxmode) {
+    case 0:
+        /* A/UX mode */
+        switch (irq) {
+        case GLUE_IRQ_IN_VIA1:
+            irq = 5;
+            break;
 
-    case GLUE_IRQ_IN_VIA2:
-        irq = 1;
-        break;
+        case GLUE_IRQ_IN_VIA2:
+            irq = 1;
+            break;
+
+        case GLUE_IRQ_IN_SONIC:
+            irq = 2;
+            break;
 
-    case GLUE_IRQ_IN_SONIC:
-        irq = 2;
+        case GLUE_IRQ_IN_ESCC:
+            irq = 3;
+            break;
+
+        default:
+            g_assert_not_reached();
+        }
         break;
 
-    case GLUE_IRQ_IN_ESCC:
-        irq = 3;
+    case 1:
+        /* Classic mode */
+        switch (irq) {
+        case GLUE_IRQ_IN_SONIC:
+            /* Route to VIA2 instead */
+            qemu_set_irq(s->irqs[GLUE_IRQ_NUBUS_9], level);
+            return;
+        }
         break;
+
+    default:
+        g_assert_not_reached();
     }
 
     if (level) {
@@ -186,9 +210,12 @@ static Property glue_properties[] = {
 static void glue_init(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
+    GLUEState *s = GLUE(dev);
 
     qdev_init_gpio_in(dev, GLUE_set_irq, 8);
     qdev_init_gpio_in_named(dev, glue_auxmode_set_irq, "auxmode", 1);
+
+    qdev_init_gpio_out(dev, s->irqs, 1);
 }
 
 static void glue_class_init(ObjectClass *klass, void *data)
@@ -454,6 +481,14 @@ static void q800_init(MachineState *machine)
                                                      VIA2_NUBUS_IRQ_9 + i));
     }
 
+    /*
+     * Since the framebuffer in slot 0x9 uses a separate IRQ, wire the unused
+     * IRQ via GLUE for use by SONIC Ethernet in A/UX mode
+     */
+    qdev_connect_gpio_out(glue, GLUE_IRQ_NUBUS_9,
+                          qdev_get_gpio_in_named(via2_dev, "nubus-irq",
+                                                 VIA2_NUBUS_IRQ_9));
+
     nubus = &NUBUS_BRIDGE(dev)->bus;
 
     /* framebuffer in nubus slot #9 */
-- 
2.20.1

Re: [PATCH 6/8] q800: route SONIC on-board Ethernet IRQ via nubus IRQ 9 in classic mode

[Laurent Vivier]

Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
> When the hardware is operating in classic mode the SONIC on-board Ethernet IRQ is
> routed to nubus IRQ 9 instead of directly to the CPU at level 3. This does not
> affect the framebuffer which although it exists in slot 9, has its own
> dedicated IRQ on the Quadra 800 hardware.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/q800.c | 57 ++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 46 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index 0093872d89..d55e6a7541 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -101,6 +101,7 @@ struct GLUEState {
>      M68kCPU *cpu;
>      uint8_t ipr;
>      uint8_t auxmode;
> +    qemu_irq irqs[1];
>  };
>  
>  #define GLUE_IRQ_IN_VIA1       0
> @@ -108,27 +109,50 @@ struct GLUEState {
>  #define GLUE_IRQ_IN_SONIC      2
>  #define GLUE_IRQ_IN_ESCC       3
>  
> +#define GLUE_IRQ_NUBUS_9       0
> +
>  static void GLUE_set_irq(void *opaque, int irq, int level)
>  {
>      GLUEState *s = opaque;
>      int i;
>  
> -    switch (irq) {
> -    case GLUE_IRQ_IN_VIA1:
> -        irq = 5;
> -        break;
> +    switch (s->auxmode) {
> +    case 0:

why don't you use "if () {} else {}" rather than "switch() { case 0: ; case 1: }" ?
(I don't think we need to manage a "default:" case.)

Thanks,
Laurent

Re: [PATCH 6/8] q800: route SONIC on-board Ethernet IRQ via nubus IRQ 9 in classic mode

[Mark Cave-Ayland]

On 16/10/2021 19:08, Laurent Vivier wrote:

> Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
>> When the hardware is operating in classic mode the SONIC on-board Ethernet IRQ is
>> routed to nubus IRQ 9 instead of directly to the CPU at level 3. This does not
>> affect the framebuffer which although it exists in slot 9, has its own
>> dedicated IRQ on the Quadra 800 hardware.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/m68k/q800.c | 57 ++++++++++++++++++++++++++++++++++++++++----------
>>   1 file changed, 46 insertions(+), 11 deletions(-)
>>
>> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
>> index 0093872d89..d55e6a7541 100644
>> --- a/hw/m68k/q800.c
>> +++ b/hw/m68k/q800.c
>> @@ -101,6 +101,7 @@ struct GLUEState {
>>       M68kCPU *cpu;
>>       uint8_t ipr;
>>       uint8_t auxmode;
>> +    qemu_irq irqs[1];
>>   };
>>   
>>   #define GLUE_IRQ_IN_VIA1       0
>> @@ -108,27 +109,50 @@ struct GLUEState {
>>   #define GLUE_IRQ_IN_SONIC      2
>>   #define GLUE_IRQ_IN_ESCC       3
>>   
>> +#define GLUE_IRQ_NUBUS_9       0
>> +
>>   static void GLUE_set_irq(void *opaque, int irq, int level)
>>   {
>>       GLUEState *s = opaque;
>>       int i;
>>   
>> -    switch (irq) {
>> -    case GLUE_IRQ_IN_VIA1:
>> -        irq = 5;
>> -        break;
>> +    switch (s->auxmode) {
>> +    case 0:
> 
> why don't you use "if () {} else {}" rather than "switch() { case 0: ; case 1: }" ?
> (I don't think we need to manage a "default:" case.)

Sure, I can do that: I used the switch()/default originally to make sure the irq 
wasn't being set to an unexpected value.

I've also just spotted that the comment in this patch is incorrect:

     /*
      * Since the framebuffer in slot 0x9 uses a separate IRQ, wire the unused
      * IRQ via GLUE for use by SONIC Ethernet in A/UX mode
      */

This should be classic mode, of course.


ATB,

Mark.

[PATCH 7/8] q800: wire up remaining IRQs in classic mode

[Mark Cave-Ayland]

Explicitly wire up the remaining IRQs in classic mode to enable the use of
g_assert_not_reached() in the default case to detect any unexpected IRQs.

Add a comment explaining the IRQ routing differences in A/UX mode based
upon the comments in NetBSD (also noting that at least A/UX 3.0.1 still
uses classic mode).

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/q800.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index d55e6a7541..fa851e2ec9 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -111,6 +111,37 @@ struct GLUEState {
 
 #define GLUE_IRQ_NUBUS_9       0
 
+/*
+ * The GLUE logic on the Quadra 800 supports 2 different IRQ routing modes
+ * controlled from the VIA1 auxmode GPIO (port B bit 6) which are documented
+ * in NetBSD as follows:
+ *
+ * A/UX mode (Linux, NetBSD, auxmode GPIO low)
+ *
+ *   Level 0:        Spurious: ignored
+ *   Level 1:        Software
+ *   Level 2:        VIA2 (except ethernet, sound)
+ *   Level 3:        Ethernet
+ *   Level 4:        Serial (SCC)
+ *   Level 5:        Sound
+ *   Level 6:        VIA1
+ *   Level 7:        NMIs: parity errors, RESET button, YANCC error
+ *
+ * Classic mode (default: used by MacOS, A/UX 3.0.1, auxmode GPIO high)
+ *
+ *   Level 0:        Spurious: ignored
+ *   Level 1:        VIA1 (clock, ADB)
+ *   Level 2:        VIA2 (NuBus, SCSI)
+ *   Level 3:
+ *   Level 4:        Serial (SCC)
+ *   Level 5:
+ *   Level 6:
+ *   Level 7:        Non-maskable: parity errors, RESET button
+ *
+ * Note that despite references to A/UX mode in Linux and NetBSD, at least
+ * A/UX 3.0.1 still uses Classic mode.
+ */
+
 static void GLUE_set_irq(void *opaque, int irq, int level)
 {
     GLUEState *s = opaque;
@@ -144,10 +175,25 @@ static void GLUE_set_irq(void *opaque, int irq, int level)
     case 1:
         /* Classic mode */
         switch (irq) {
+        case GLUE_IRQ_IN_VIA1:
+            irq = 0;
+            break;
+
+        case GLUE_IRQ_IN_VIA2:
+            irq = 1;
+            break;
+
         case GLUE_IRQ_IN_SONIC:
             /* Route to VIA2 instead */
             qemu_set_irq(s->irqs[GLUE_IRQ_NUBUS_9], level);
             return;
+
+        case GLUE_IRQ_IN_ESCC:
+            irq = 3;
+            break;
+
+        default:
+            g_assert_not_reached();
         }
         break;
 
-- 
2.20.1

Re: [PATCH 7/8] q800: wire up remaining IRQs in classic mode

[Laurent Vivier]

Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
> Explicitly wire up the remaining IRQs in classic mode to enable the use of
> g_assert_not_reached() in the default case to detect any unexpected IRQs.
> 
> Add a comment explaining the IRQ routing differences in A/UX mode based
> upon the comments in NetBSD (also noting that at least A/UX 3.0.1 still
> uses classic mode).
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/m68k/q800.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index d55e6a7541..fa851e2ec9 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -111,6 +111,37 @@ struct GLUEState {
>  
>  #define GLUE_IRQ_NUBUS_9       0
>  
> +/*
> + * The GLUE logic on the Quadra 800 supports 2 different IRQ routing modes
> + * controlled from the VIA1 auxmode GPIO (port B bit 6) which are documented
> + * in NetBSD as follows:
> + *
> + * A/UX mode (Linux, NetBSD, auxmode GPIO low)
> + *
> + *   Level 0:        Spurious: ignored
> + *   Level 1:        Software
> + *   Level 2:        VIA2 (except ethernet, sound)
> + *   Level 3:        Ethernet
> + *   Level 4:        Serial (SCC)
> + *   Level 5:        Sound
> + *   Level 6:        VIA1
> + *   Level 7:        NMIs: parity errors, RESET button, YANCC error
> + *
> + * Classic mode (default: used by MacOS, A/UX 3.0.1, auxmode GPIO high)
> + *
> + *   Level 0:        Spurious: ignored
> + *   Level 1:        VIA1 (clock, ADB)
> + *   Level 2:        VIA2 (NuBus, SCSI)
> + *   Level 3:
> + *   Level 4:        Serial (SCC)
> + *   Level 5:
> + *   Level 6:
> + *   Level 7:        Non-maskable: parity errors, RESET button
> + *
> + * Note that despite references to A/UX mode in Linux and NetBSD, at least
> + * A/UX 3.0.1 still uses Classic mode.
> + */
> +
>  static void GLUE_set_irq(void *opaque, int irq, int level)
>  {
>      GLUEState *s = opaque;
> @@ -144,10 +175,25 @@ static void GLUE_set_irq(void *opaque, int irq, int level)
>      case 1:
>          /* Classic mode */
>          switch (irq) {
> +        case GLUE_IRQ_IN_VIA1:
> +            irq = 0;
> +            break;
> +
> +        case GLUE_IRQ_IN_VIA2:
> +            irq = 1;
> +            break;
> +
>          case GLUE_IRQ_IN_SONIC:
>              /* Route to VIA2 instead */
>              qemu_set_irq(s->irqs[GLUE_IRQ_NUBUS_9], level);
>              return;
> +
> +        case GLUE_IRQ_IN_ESCC:
> +            irq = 3;
> +            break;
> +
> +        default:
> +            g_assert_not_reached();
>          }
>          break;
>  
> 

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

[PATCH 8/8] q800: add NMI handler

[Mark Cave-Ayland]

This allows the programmer's switch to be triggered via the monitor for debugging
purposes. Since the CPU level 7 interrupt is level-triggered, use a timer to hold
the NMI active for 100ms before releasing it again.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/m68k/q800.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index fa851e2ec9..fca2127ddd 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -28,6 +28,7 @@
 #include "cpu.h"
 #include "hw/boards.h"
 #include "hw/or-irq.h"
+#include "hw/nmi.h"
 #include "elf.h"
 #include "hw/loader.h"
 #include "ui/console.h"
@@ -102,12 +103,14 @@ struct GLUEState {
     uint8_t ipr;
     uint8_t auxmode;
     qemu_irq irqs[1];
+    QEMUTimer *nmi_release;
 };
 
 #define GLUE_IRQ_IN_VIA1       0
 #define GLUE_IRQ_IN_VIA2       1
 #define GLUE_IRQ_IN_SONIC      2
 #define GLUE_IRQ_IN_ESCC       3
+#define GLUE_IRQ_IN_NMI        4
 
 #define GLUE_IRQ_NUBUS_9       0
 
@@ -167,6 +170,10 @@ static void GLUE_set_irq(void *opaque, int irq, int level)
             irq = 3;
             break;
 
+        case GLUE_IRQ_IN_NMI:
+            irq = 6;
+            break;
+
         default:
             g_assert_not_reached();
         }
@@ -192,6 +199,10 @@ static void GLUE_set_irq(void *opaque, int irq, int level)
             irq = 3;
             break;
 
+        case GLUE_IRQ_IN_NMI:
+            irq = 6;
+            break;
+
         default:
             g_assert_not_reached();
         }
@@ -223,12 +234,30 @@ static void glue_auxmode_set_irq(void *opaque, int irq, int level)
     s->auxmode = level;
 }
 
+static void glue_nmi(NMIState *n, int cpu_index, Error **errp)
+{
+    GLUEState *s = GLUE(n);
+
+    /* Hold NMI active for 100ms */
+    GLUE_set_irq(s, GLUE_IRQ_IN_NMI, 1);
+    timer_mod(s->nmi_release, qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 100);
+}
+
+static void glue_nmi_release(void *opaque)
+{
+    GLUEState *s = GLUE(opaque);
+
+    GLUE_set_irq(s, GLUE_IRQ_IN_NMI, 0);
+}
+
 static void glue_reset(DeviceState *dev)
 {
     GLUEState *s = GLUE(dev);
 
     s->ipr = 0;
     s->auxmode = 0;
+
+    timer_del(s->nmi_release);
 }
 
 static const VMStateDescription vmstate_glue = {
@@ -238,6 +267,7 @@ static const VMStateDescription vmstate_glue = {
     .fields = (VMStateField[]) {
         VMSTATE_UINT8(ipr, GLUEState),
         VMSTATE_UINT8(auxmode, GLUEState),
+        VMSTATE_TIMER_PTR(nmi_release, GLUEState),
         VMSTATE_END_OF_LIST(),
     },
 };
@@ -253,6 +283,13 @@ static Property glue_properties[] = {
     DEFINE_PROP_END_OF_LIST(),
 };
 
+static void glue_finalize(Object *obj)
+{
+    GLUEState *s = GLUE(obj);
+
+    timer_free(s->nmi_release);
+}
+
 static void glue_init(Object *obj)
 {
     DeviceState *dev = DEVICE(obj);
@@ -262,15 +299,20 @@ static void glue_init(Object *obj)
     qdev_init_gpio_in_named(dev, glue_auxmode_set_irq, "auxmode", 1);
 
     qdev_init_gpio_out(dev, s->irqs, 1);
+
+    /* NMI release timer */
+    s->nmi_release = timer_new_ms(QEMU_CLOCK_VIRTUAL, glue_nmi_release, s);
 }
 
 static void glue_class_init(ObjectClass *klass, void *data)
 {
     DeviceClass *dc = DEVICE_CLASS(klass);
+    NMIClass *nc = NMI_CLASS(klass);
 
     dc->vmsd = &vmstate_glue;
     dc->reset = glue_reset;
     device_class_set_props(dc, glue_properties);
+    nc->nmi_monitor_handler = glue_nmi;
 }
 
 static const TypeInfo glue_info = {
@@ -278,7 +320,12 @@ static const TypeInfo glue_info = {
     .parent = TYPE_SYS_BUS_DEVICE,
     .instance_size = sizeof(GLUEState),
     .instance_init = glue_init,
+    .instance_finalize = glue_finalize,
     .class_init = glue_class_init,
+    .interfaces = (InterfaceInfo[]) {
+         { TYPE_NMI },
+         { }
+    },
 };
 
 static void main_cpu_reset(void *opaque)
-- 
2.20.1

Re: [PATCH 8/8] q800: add NMI handler

[Laurent Vivier]

Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
> This allows the programmer's switch to be triggered via the monitor for debugging
> purposes. Since the CPU level 7 interrupt is level-triggered, use a timer to hold
> the NMI active for 100ms before releasing it again.
> 

I'm wondering if Qemu provides another way to have a level-triggered interrupt in this case.

I' tried to see if keeping the button pressed on a mac kept the IRQ up (as QMP NMI does), but a real
mac is too slow and has to many things to display it was not really conclusive...

Thanks,
Laurent

Re: [PATCH 8/8] q800: add NMI handler

[Mark Cave-Ayland]

On 15/10/2021 09:40, Laurent Vivier wrote:

> Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
>> This allows the programmer's switch to be triggered via the monitor for debugging
>> purposes. Since the CPU level 7 interrupt is level-triggered, use a timer to hold
>> the NMI active for 100ms before releasing it again.
> 
> I'm wondering if Qemu provides another way to have a level-triggered interrupt in this case.
> 
> I' tried to see if keeping the button pressed on a mac kept the IRQ up (as QMP NMI does), but a real
> mac is too slow and has to many things to display it was not really conclusive...

When writing the patch I rebased the outstanding MacOS patches onto the branch, 
installed Macsbug into MacOS and used "info nmi" to break into it.

Testing glue_nmi() with:

   GLUE_set_irq(s, GLUE_IRQ_IN_NMI, 1);
   GLUE_set_irq(s, GLUE_IRQ_IN_NMI, 0);

i.e. a simple pulse didn't launch MacsBug at all. Keeping the NMI high launches 
MacsBug (which is usable) but then as soon as you exit MacsBug with ES, MacsBug 
breaks immediately again making it impossible to return to the Finder. Adding the 
timer allows launching MacsBug and then exiting/re-entering MacsBug again on demand 
as expected.


ATB,

Mark.

Re: [PATCH 8/8] q800: add NMI handler

[Laurent Vivier]

Le 15/10/2021 à 22:12, Mark Cave-Ayland a écrit :
> On 15/10/2021 09:40, Laurent Vivier wrote:
> 
>> Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
>>> This allows the programmer's switch to be triggered via the monitor for debugging
>>> purposes. Since the CPU level 7 interrupt is level-triggered, use a timer to hold
>>> the NMI active for 100ms before releasing it again.
>>
>> I'm wondering if Qemu provides another way to have a level-triggered interrupt in this case.
>>
>> I' tried to see if keeping the button pressed on a mac kept the IRQ up (as QMP NMI does), but a real
>> mac is too slow and has to many things to display it was not really conclusive...
> 
> When writing the patch I rebased the outstanding MacOS patches onto the branch, installed Macsbug
> into MacOS and used "info nmi" to break into it.
> 
> Testing glue_nmi() with:
> 
>   GLUE_set_irq(s, GLUE_IRQ_IN_NMI, 1);
>   GLUE_set_irq(s, GLUE_IRQ_IN_NMI, 0);
> 
> i.e. a simple pulse didn't launch MacsBug at all. Keeping the NMI high launches MacsBug (which is
> usable) but then as soon as you exit MacsBug with ES, MacsBug breaks immediately again making it
> impossible to return to the Finder. Adding the timer allows launching MacsBug and then
> exiting/re-entering MacsBug again on demand as expected.
> 

I think we have to mimic the finger pressing the button..

By the way, NMI should also dumps the CPU registers under linux.

Reviewied-by: Laurent Vivier <laurent@vivier.eu>

Re: [PATCH 8/8] q800: add NMI handler

[Mark Cave-Ayland]

On 16/10/2021 18:09, Laurent Vivier wrote:

> Le 15/10/2021 à 22:12, Mark Cave-Ayland a écrit :
>> On 15/10/2021 09:40, Laurent Vivier wrote:
>>
>>> Le 13/10/2021 à 23:21, Mark Cave-Ayland a écrit :
>>>> This allows the programmer's switch to be triggered via the monitor for debugging
>>>> purposes. Since the CPU level 7 interrupt is level-triggered, use a timer to hold
>>>> the NMI active for 100ms before releasing it again.
>>>
>>> I'm wondering if Qemu provides another way to have a level-triggered interrupt in this case.
>>>
>>> I' tried to see if keeping the button pressed on a mac kept the IRQ up (as QMP NMI does), but a real
>>> mac is too slow and has to many things to display it was not really conclusive...
>>
>> When writing the patch I rebased the outstanding MacOS patches onto the branch, installed Macsbug
>> into MacOS and used "info nmi" to break into it.
>>
>> Testing glue_nmi() with:
>>
>>    GLUE_set_irq(s, GLUE_IRQ_IN_NMI, 1);
>>    GLUE_set_irq(s, GLUE_IRQ_IN_NMI, 0);
>>
>> i.e. a simple pulse didn't launch MacsBug at all. Keeping the NMI high launches MacsBug (which is
>> usable) but then as soon as you exit MacsBug with ES, MacsBug breaks immediately again making it
>> impossible to return to the Finder. Adding the timer allows launching MacsBug and then
>> exiting/re-entering MacsBug again on demand as expected.
>>
> 
> I think we have to mimic the finger pressing the button..
> 
> By the way, NMI should also dumps the CPU registers under linux.
> 
> Reviewied-by: Laurent Vivier <laurent@vivier.eu>

I've just tried this on Linux, and it seems to work okay although I get several 
copies of the register dump on the console for a single invocation of "info nmi" e.g.

[    4.610000] Non-Maskable Interrupt
[    4.610000] Modules linked in: mac_esp(+) esp_scsi macsonic
[    4.610000] PC: [<00002dc0>] arch_cpu_idle+0x4/0x6
[    4.610000] SR: 2000  SP: (ptrval)  a2: 00395314
[    4.610000] d0: 00000000    d1: 00000002    d2: 0004b492    d3: 00392000
[    4.610000] d4: 00000000    d5: 00000000    a0: 00392000    a1: 00395314
[    4.610000] Process swapper (pid: 0, task=(ptrval))
[    4.610000] Frame format=0
[    4.610000] Stack from 00393f9c:
[    4.610000]         0004b540 00000431 0040ee22 0004b4c4 0002bf6a 004005f0 00393ff8 
0004b714
[    4.610000]         0003e23a 0029c518 000000c2 0040eb54 3dc1e5d0 00000000 0004ff74 
0003efe2
[    4.610000]         003f4e92 00000000 00000040 00000000 00000000 00000000 0040ee22 
00000000
[    4.610000]         003f3872
[    4.610000] Call Trace: [<0004b540>] do_idle+0x7c/0xd2
[    4.610000]  [<0004b4c4>] do_idle+0x0/0xd2
[    4.610000]  [<0002bf6a>] kernel_thread+0x0/0x26
[    4.610000]  [<004005f0>] __alloc_bootmem+0x0/0x38
[    4.610000]  [<0004b714>] cpu_startup_entry+0xe/0x12
[    4.610000]  [<0003e23a>] find_task_by_pid_ns+0x0/0x22
[    4.610000]  [<0029c518>] rest_init+0x78/0x82
[    4.610000]  [<0004ff74>] printk+0x0/0x18
[    4.610000]  [<0003efe2>] parse_args+0x0/0x2c2
[    4.610000]  [<003f4e92>] start_kernel+0x43e/0x448
[    4.610000]  [<003f3872>] _sinittext+0x872/0x11f8
[    4.610000] Code: bc00 0060 f210 f0ff 7001 60cc 4e72 2000 <4e75> 2079 003b fc6c 
4a88 6702 4e90 60fe 2079 003b fc68 4a88 6702 4e90 60fe 2f0c

(repeated 7 or 8 times)

MacsBug seems to handle this better: as soon as the NMI is triggered, MacsBug opens 
and displays "NMI" both if the programmer switch GPIO is held active, or pulsed as 
implemented in the patch. When using the above patch, subsequent NMI triggers do not 
display "NMI" or change MacsBug in any visible way which suggests that once the NMI 
is triggered, the NMI is ignored until the programmers switch GPIO is inactive once 
again.

Do you see the repeated register output in Linux for a single press of the 
programmers switch on a real Quadra 800? It may be that Linux could be improved by 
having similar logic.


ATB,

Mark.

Re: [PATCH 8/8] q800: add NMI handler

[Laurent Vivier]

Le 17/10/2021 à 12:00, Mark Cave-Ayland a écrit :
...
> I've just tried this on Linux, and it seems to work okay although I get several copies of the
> register dump on the console for a single invocation of "info nmi" e.g.
> 
> [    4.610000] Non-Maskable Interrupt
> [    4.610000] Modules linked in: mac_esp(+) esp_scsi macsonic
> [    4.610000] PC: [<00002dc0>] arch_cpu_idle+0x4/0x6
> [    4.610000] SR: 2000  SP: (ptrval)  a2: 00395314
> [    4.610000] d0: 00000000    d1: 00000002    d2: 0004b492    d3: 00392000
> [    4.610000] d4: 00000000    d5: 00000000    a0: 00392000    a1: 00395314
> [    4.610000] Process swapper (pid: 0, task=(ptrval))
> [    4.610000] Frame format=0
> [    4.610000] Stack from 00393f9c:
> [    4.610000]         0004b540 00000431 0040ee22 0004b4c4 0002bf6a 004005f0 00393ff8 0004b714
> [    4.610000]         0003e23a 0029c518 000000c2 0040eb54 3dc1e5d0 00000000 0004ff74 0003efe2
> [    4.610000]         003f4e92 00000000 00000040 00000000 00000000 00000000 0040ee22 00000000
> [    4.610000]         003f3872
> [    4.610000] Call Trace: [<0004b540>] do_idle+0x7c/0xd2
> [    4.610000]  [<0004b4c4>] do_idle+0x0/0xd2
> [    4.610000]  [<0002bf6a>] kernel_thread+0x0/0x26
> [    4.610000]  [<004005f0>] __alloc_bootmem+0x0/0x38
> [    4.610000]  [<0004b714>] cpu_startup_entry+0xe/0x12
> [    4.610000]  [<0003e23a>] find_task_by_pid_ns+0x0/0x22
> [    4.610000]  [<0029c518>] rest_init+0x78/0x82
> [    4.610000]  [<0004ff74>] printk+0x0/0x18
> [    4.610000]  [<0003efe2>] parse_args+0x0/0x2c2
> [    4.610000]  [<003f4e92>] start_kernel+0x43e/0x448
> [    4.610000]  [<003f3872>] _sinittext+0x872/0x11f8
> [    4.610000] Code: bc00 0060 f210 f0ff 7001 60cc 4e72 2000 <4e75> 2079 003b fc6c 4a88 6702 4e90
> 60fe 2079 003b fc68 4a88 6702 4e90 60fe 2f0c
> 
> (repeated 7 or 8 times)
> 
> MacsBug seems to handle this better: as soon as the NMI is triggered, MacsBug opens and displays
> "NMI" both if the programmer switch GPIO is held active, or pulsed as implemented in the patch. When
> using the above patch, subsequent NMI triggers do not display "NMI" or change MacsBug in any visible
> way which suggests that once the NMI is triggered, the NMI is ignored until the programmers switch
> GPIO is inactive once again.
> 
> Do you see the repeated register output in Linux for a single press of the programmers switch on a
> real Quadra 800? It may be that Linux could be improved by having similar logic.

In fact, register output is repeated while I keep the button pressed (5.14 kernel provided by Finn)

If I press and release one (or several) time(s) before the end of the first output I have only one
output.

Thanks,
Laurent

Re: [PATCH 8/8] q800: add NMI handler

[Mark Cave-Ayland]

On 17/10/2021 17:56, Laurent Vivier wrote:

> Le 17/10/2021 à 12:00, Mark Cave-Ayland a écrit :
> ...
>> I've just tried this on Linux, and it seems to work okay although I get several copies of the
>> register dump on the console for a single invocation of "info nmi" e.g.
>>
>> [    4.610000] Non-Maskable Interrupt
>> [    4.610000] Modules linked in: mac_esp(+) esp_scsi macsonic
>> [    4.610000] PC: [<00002dc0>] arch_cpu_idle+0x4/0x6
>> [    4.610000] SR: 2000  SP: (ptrval)  a2: 00395314
>> [    4.610000] d0: 00000000    d1: 00000002    d2: 0004b492    d3: 00392000
>> [    4.610000] d4: 00000000    d5: 00000000    a0: 00392000    a1: 00395314
>> [    4.610000] Process swapper (pid: 0, task=(ptrval))
>> [    4.610000] Frame format=0
>> [    4.610000] Stack from 00393f9c:
>> [    4.610000]         0004b540 00000431 0040ee22 0004b4c4 0002bf6a 004005f0 00393ff8 0004b714
>> [    4.610000]         0003e23a 0029c518 000000c2 0040eb54 3dc1e5d0 00000000 0004ff74 0003efe2
>> [    4.610000]         003f4e92 00000000 00000040 00000000 00000000 00000000 0040ee22 00000000
>> [    4.610000]         003f3872
>> [    4.610000] Call Trace: [<0004b540>] do_idle+0x7c/0xd2
>> [    4.610000]  [<0004b4c4>] do_idle+0x0/0xd2
>> [    4.610000]  [<0002bf6a>] kernel_thread+0x0/0x26
>> [    4.610000]  [<004005f0>] __alloc_bootmem+0x0/0x38
>> [    4.610000]  [<0004b714>] cpu_startup_entry+0xe/0x12
>> [    4.610000]  [<0003e23a>] find_task_by_pid_ns+0x0/0x22
>> [    4.610000]  [<0029c518>] rest_init+0x78/0x82
>> [    4.610000]  [<0004ff74>] printk+0x0/0x18
>> [    4.610000]  [<0003efe2>] parse_args+0x0/0x2c2
>> [    4.610000]  [<003f4e92>] start_kernel+0x43e/0x448
>> [    4.610000]  [<003f3872>] _sinittext+0x872/0x11f8
>> [    4.610000] Code: bc00 0060 f210 f0ff 7001 60cc 4e72 2000 <4e75> 2079 003b fc6c 4a88 6702 4e90
>> 60fe 2079 003b fc68 4a88 6702 4e90 60fe 2f0c
>>
>> (repeated 7 or 8 times)
>>
>> MacsBug seems to handle this better: as soon as the NMI is triggered, MacsBug opens and displays
>> "NMI" both if the programmer switch GPIO is held active, or pulsed as implemented in the patch. When
>> using the above patch, subsequent NMI triggers do not display "NMI" or change MacsBug in any visible
>> way which suggests that once the NMI is triggered, the NMI is ignored until the programmers switch
>> GPIO is inactive once again.
>>
>> Do you see the repeated register output in Linux for a single press of the programmers switch on a
>> real Quadra 800? It may be that Linux could be improved by having similar logic.
> 
> In fact, register output is repeated while I keep the button pressed (5.14 kernel provided by Finn)
> 
> If I press and release one (or several) time(s) before the end of the first output I have only one
> output.

Thanks, that makes sense - my guess is that the repeated output in QEMU is just a 
symptom of the QEMU machine being much faster than the real one.


ATB,

Mark.