[PATCH 0/7] mac_via: fixes and improvements

[PATCH 0/7] mac_via: fixes and improvements

[Mark Cave-Ayland]

This patchset consists of a number of mac_via fixes and improvements taken
from my attempts to boot MacOS on the q800 machine.

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


Mark Cave-Ayland (7):
  mac_via: switch rtc pram trace-events to use hex rather than decimal
    for addresses
  mac_via: fix up adb_via_receive() trace events
  mac_via: allow long accesses to VIA registers
  mac_via: don't re-inject ADB response when switching to IDLE state
  mac_via: rename VBL timer to 60Hz timer
  mac_via: fix 60Hz VIA1 timer interval
  mac_via: remove VIA1 timer optimisations

 hw/misc/mac_via.c         | 187 ++++++++++++++------------------------
 hw/misc/trace-events      |   4 +-
 include/hw/misc/mac_via.h |   8 +-
 3 files changed, 74 insertions(+), 125 deletions(-)

-- 
2.20.1

[PATCH 1/7] mac_via: switch rtc pram trace-events to use hex rather than decimal for addresses

[Mark Cave-Ayland]

Since all the documentation uses the hex offsets, this makes it much easier
to see what is going on.

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

diff --git a/hw/misc/trace-events b/hw/misc/trace-events
index d626b9d7a7..73191b5e9d 100644
--- a/hw/misc/trace-events
+++ b/hw/misc/trace-events
@@ -221,8 +221,8 @@ via1_rtc_cmd_test_write(int value) "value=0x%02x"
 via1_rtc_cmd_wprotect_write(int value) "value=0x%02x"
 via1_rtc_cmd_pram_read(int addr, int value) "addr=%u value=0x%02x"
 via1_rtc_cmd_pram_write(int addr, int value) "addr=%u value=0x%02x"
-via1_rtc_cmd_pram_sect_read(int sector, int offset, int addr, int value) "sector=%u offset=%u addr=%d value=0x%02x"
-via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "sector=%u offset=%u addr=%d value=0x%02x"
+via1_rtc_cmd_pram_sect_read(int sector, int offset, int addr, int value) "sector=%u offset=%u addr=0x%x value=0x%02x"
+via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "sector=%u offset=%u addr=0x%x value=0x%02x"
 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"
-- 
2.20.1

[PATCH 2/7] mac_via: fix up adb_via_receive() trace events

[Mark Cave-Ayland]

The use of the post-increment operator on adb_data_in_index meant that the
trace-event was accidentally displaying the next byte in the incoming ADB
data buffer rather than the current byte.

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

diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index 488d086a17..0f6586e102 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -816,33 +816,37 @@ static void adb_via_receive(MacVIAState *s, int state, uint8_t *data)
         switch (s->adb_data_in_index) {
         case 0:
             /* First EVEN byte: vADBInt indicates bus timeout */
-            trace_via1_adb_receive(state == ADB_STATE_EVEN ? "EVEN" : " ODD",
-                                   *data, (ms->b & VIA1B_vADBInt) ? "+" : "-",
-                                   adb_bus->status, s->adb_data_in_index,
-                                   s->adb_data_in_size);
-
-            *data = s->adb_data_in[s->adb_data_in_index++];
+            *data = s->adb_data_in[s->adb_data_in_index];
             if (adb_bus->status & ADB_STATUS_BUSTIMEOUT) {
                 ms->b &= ~VIA1B_vADBInt;
             } else {
                 ms->b |= VIA1B_vADBInt;
             }
-            break;
 
-        case 1:
-            /* First ODD byte: vADBInt indicates SRQ */
             trace_via1_adb_receive(state == ADB_STATE_EVEN ? "EVEN" : " ODD",
                                    *data, (ms->b & VIA1B_vADBInt) ? "+" : "-",
                                    adb_bus->status, s->adb_data_in_index,
                                    s->adb_data_in_size);
 
-            *data = s->adb_data_in[s->adb_data_in_index++];
+            s->adb_data_in_index++;
+            break;
+
+        case 1:
+            /* First ODD byte: vADBInt indicates SRQ */
+            *data = s->adb_data_in[s->adb_data_in_index];
             pending = adb_bus->pending & ~(1 << (s->adb_autopoll_cmd >> 4));
             if (pending) {
                 ms->b &= ~VIA1B_vADBInt;
             } else {
                 ms->b |= VIA1B_vADBInt;
             }
+
+            trace_via1_adb_receive(state == ADB_STATE_EVEN ? "EVEN" : " ODD",
+                                   *data, (ms->b & VIA1B_vADBInt) ? "+" : "-",
+                                   adb_bus->status, s->adb_data_in_index,
+                                   s->adb_data_in_size);
+
+            s->adb_data_in_index++;
             break;
 
         default:
@@ -852,14 +856,9 @@ static void adb_via_receive(MacVIAState *s, int state, uint8_t *data)
              * end of the poll reply, so provide these extra bytes below to
              * keep it happy
              */
-            trace_via1_adb_receive(state == ADB_STATE_EVEN ? "EVEN" : " ODD",
-                                   *data, (ms->b & VIA1B_vADBInt) ? "+" : "-",
-                                   adb_bus->status, s->adb_data_in_index,
-                                   s->adb_data_in_size);
-
             if (s->adb_data_in_index < s->adb_data_in_size) {
                 /* Next data byte */
-                *data = s->adb_data_in[s->adb_data_in_index++];
+                *data = s->adb_data_in[s->adb_data_in_index];
                 ms->b |= VIA1B_vADBInt;
             } else if (s->adb_data_in_index == s->adb_data_in_size) {
                 if (adb_bus->status & ADB_STATUS_BUSTIMEOUT) {
@@ -869,7 +868,6 @@ static void adb_via_receive(MacVIAState *s, int state, uint8_t *data)
                     /* Return 0x0 after reply */
                     *data = 0;
                 }
-                s->adb_data_in_index++;
                 ms->b &= ~VIA1B_vADBInt;
             } else {
                 /* Bus timeout (no more data) */
@@ -878,6 +876,15 @@ static void adb_via_receive(MacVIAState *s, int state, uint8_t *data)
                 adb_bus->status = 0;
                 adb_autopoll_unblock(adb_bus);
             }
+
+            trace_via1_adb_receive(state == ADB_STATE_EVEN ? "EVEN" : " ODD",
+                                   *data, (ms->b & VIA1B_vADBInt) ? "+" : "-",
+                                   adb_bus->status, s->adb_data_in_index,
+                                   s->adb_data_in_size);
+
+            if (s->adb_data_in_index <= s->adb_data_in_size) {
+                s->adb_data_in_index++;
+            }
             break;
         }
 
-- 
2.20.1

[PATCH 3/7] mac_via: allow long accesses to VIA registers

[Mark Cave-Ayland]

The MacOS SCSI driver uses a long access to read the VIA registers rather than
just a single byte during the message out phase.

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

diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index 0f6586e102..f38d6e2f6e 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -966,7 +966,7 @@ static const MemoryRegionOps mos6522_q800_via1_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
     .valid = {
         .min_access_size = 1,
-        .max_access_size = 1,
+        .max_access_size = 4,
     },
 };
 
@@ -995,7 +995,7 @@ static const MemoryRegionOps mos6522_q800_via2_ops = {
     .endianness = DEVICE_BIG_ENDIAN,
     .valid = {
         .min_access_size = 1,
-        .max_access_size = 1,
+        .max_access_size = 4,
     },
 };
 
-- 
2.20.1

[PATCH 4/7] mac_via: don't re-inject ADB response when switching to IDLE state

[Mark Cave-Ayland]

The current workaround for the Linux ADB state machine in kernels < 5.6 switching
the VIA back to IDLE state between send and receive modes is to re-inject the
first byte of the response in the IDLE state, and then force the state machine
into generating an autopoll reply.

In fact what is happening is much simpler: analysis of traces from a real Quadra
suggest that the existing data is returned as the first autopoll response rather
than generating an immediate response starting whilst still in IDLE state.

Update the ADB receive code to work in the same way, which allows the re-injection
code to be completely removed from adb_via_receive() and for adb_via_poll() to
be simplified accordingly.

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

diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index f38d6e2f6e..76f31b8cae 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -609,7 +609,6 @@ static void adb_via_poll(void *opaque)
     uint8_t obuf[9];
     uint8_t *data = &s->sr;
     int olen;
-    uint16_t pending;
 
     /*
      * Setting vADBInt below indicates that an autopoll reply has been
@@ -618,36 +617,36 @@ static void adb_via_poll(void *opaque)
      */
     adb_autopoll_block(adb_bus);
 
-    m->adb_data_in_index = 0;
-    m->adb_data_out_index = 0;
-    olen = adb_poll(adb_bus, obuf, adb_bus->autopoll_mask);
-
-    if (olen > 0) {
-        /* Autopoll response */
-        *data = obuf[0];
-        olen--;
-        memcpy(m->adb_data_in, &obuf[1], olen);
-        m->adb_data_in_size = olen;
+    if (m->adb_data_in_size > 0 && m->adb_data_in_index == 0) {
+        /*
+         * For older Linux kernels that switch to IDLE mode after sending the
+         * ADB command, detect if there is an existing response and return that
+         * as a a "fake" autopoll reply or bus timeout accordingly
+         */
+        *data = m->adb_data_out[0];
+        olen = m->adb_data_in_size;
 
         s->b &= ~VIA1B_vADBInt;
         qemu_irq_raise(m->adb_data_ready);
-    } else if (olen < 0) {
-        /* Bus timeout (device does not exist) */
-        *data = 0xff;
-        s->b |= VIA1B_vADBInt;
-        adb_autopoll_unblock(adb_bus);
     } else {
-        pending = adb_bus->pending & ~(1 << (m->adb_autopoll_cmd >> 4));
+        /*
+         * Otherwise poll as normal
+         */
+        m->adb_data_in_index = 0;
+        m->adb_data_out_index = 0;
+        olen = adb_poll(adb_bus, obuf, adb_bus->autopoll_mask);
+
+        if (olen > 0) {
+            /* Autopoll response */
+            *data = obuf[0];
+            olen--;
+            memcpy(m->adb_data_in, &obuf[1], olen);
+            m->adb_data_in_size = olen;
 
-        if (pending) {
-            /*
-             * Bus timeout (device exists but another device has data). Block
-             * autopoll so the OS can read out the first EVEN and first ODD
-             * byte to determine bus timeout and SRQ status
-             */
-            *data = m->adb_autopoll_cmd;
             s->b &= ~VIA1B_vADBInt;
-
+            qemu_irq_raise(m->adb_data_ready);
+        } else {
+            *data = m->adb_autopoll_cmd;
             obuf[0] = 0xff;
             obuf[1] = 0xff;
             olen = 2;
@@ -655,12 +654,8 @@ static void adb_via_poll(void *opaque)
             memcpy(m->adb_data_in, obuf, olen);
             m->adb_data_in_size = olen;
 
+            s->b &= ~VIA1B_vADBInt;
             qemu_irq_raise(m->adb_data_ready);
-        } else {
-            /* Bus timeout (device exists but no other device has data) */
-            *data = 0;
-            s->b |= VIA1B_vADBInt;
-            adb_autopoll_unblock(adb_bus);
         }
     }
 
@@ -783,27 +778,8 @@ static void adb_via_receive(MacVIAState *s, int state, uint8_t *data)
         return;
 
     case ADB_STATE_IDLE:
-        /*
-         * Since adb_request() will have already consumed the data from the
-         * device, we must detect this extra state change and re-inject the
-         * reponse as either a "fake" autopoll reply or bus timeout
-         * accordingly
-         */
-        if (s->adb_data_in_index == 0) {
-            if (adb_bus->status & ADB_STATUS_BUSTIMEOUT) {
-                *data = 0xff;
-                ms->b |= VIA1B_vADBInt;
-                qemu_irq_raise(s->adb_data_ready);
-            } else if (s->adb_data_in_size > 0) {
-                adb_bus->status = ADB_STATUS_POLLREPLY;
-                *data = s->adb_autopoll_cmd;
-                ms->b &= ~VIA1B_vADBInt;
-                qemu_irq_raise(s->adb_data_ready);
-            }
-        } else {
-            ms->b |= VIA1B_vADBInt;
-            adb_autopoll_unblock(adb_bus);
-        }
+        ms->b |= VIA1B_vADBInt;
+        adb_autopoll_unblock(adb_bus);
 
         trace_via1_adb_receive("IDLE", *data,
                         (ms->b & VIA1B_vADBInt) ? "+" : "-", adb_bus->status,
-- 
2.20.1

[PATCH 5/7] mac_via: rename VBL timer to 60Hz timer

[Mark Cave-Ayland]

According to the "Guide To The Macintosh Family Hardware", the 60Hz VIA1 timer
on newer Macs such as the Quadra only exists for compatibility with old software
and is no longer synced to the VBL interval.

Rename the VBL timer to 60Hz timer to emphasise this and to prevent confusion
when the real VBL interrupt (now handled as a NuBus slot interrupt) is added in
future.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/misc/mac_via.c         | 41 ++++++++++++++++++++-------------------
 include/hw/misc/mac_via.h |  8 ++++----
 2 files changed, 25 insertions(+), 24 deletions(-)

diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index 76f31b8cae..f994fefa7c 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -297,18 +297,18 @@ enum {
     REG_EMPTY = 0xff,
 };
 
-static void via1_VBL_update(MOS6522Q800VIA1State *v1s)
+static void via1_sixty_hz_update(MOS6522Q800VIA1State *v1s)
 {
     MOS6522State *s = MOS6522(v1s);
 
     /* 60 Hz irq */
-    v1s->next_VBL = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630) /
-                    16630 * 16630;
+    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630) /
+                          16630 * 16630;
 
-    if (s->ier & VIA1_IRQ_VBLANK) {
-        timer_mod(v1s->VBL_timer, v1s->next_VBL);
+    if (s->ier & VIA1_IRQ_60HZ) {
+        timer_mod(v1s->sixty_hz_timer, v1s->next_sixty_hz);
     } else {
-        timer_del(v1s->VBL_timer);
+        timer_del(v1s->sixty_hz_timer);
     }
 }
 
@@ -325,16 +325,16 @@ static void via1_one_second_update(MOS6522Q800VIA1State *v1s)
     }
 }
 
-static void via1_VBL(void *opaque)
+static void via1_sixty_hz(void *opaque)
 {
     MOS6522Q800VIA1State *v1s = opaque;
     MOS6522State *s = MOS6522(v1s);
     MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
 
-    s->ifr |= VIA1_IRQ_VBLANK;
+    s->ifr |= VIA1_IRQ_60HZ;
     mdc->update_irq(s);
 
-    via1_VBL_update(v1s);
+    via1_sixty_hz_update(v1s);
 }
 
 static void via1_one_second(void *opaque)
@@ -897,12 +897,12 @@ static uint64_t mos6522_q800_via1_read(void *opaque, hwaddr addr, unsigned size)
 
     /*
      * If IRQs are disabled, timers are disabled, but we need to update
-     * VIA1_IRQ_VBLANK and VIA1_IRQ_ONE_SECOND bits in the IFR
+     * VIA1_IRQ_60HZ and VIA1_IRQ_ONE_SECOND bits in the IFR
      */
 
-    if (now >= s->next_VBL) {
-        ms->ifr |= VIA1_IRQ_VBLANK;
-        via1_VBL_update(s);
+    if (now >= s->next_sixty_hz) {
+        ms->ifr |= VIA1_IRQ_60HZ;
+        via1_sixty_hz_update(s);
     }
     if (now >= s->next_second) {
         ms->ifr |= VIA1_IRQ_ONE_SECOND;
@@ -933,7 +933,7 @@ static void mos6522_q800_via1_write(void *opaque, hwaddr addr, uint64_t val,
     }
 
     via1_one_second_update(v1s);
-    via1_VBL_update(v1s);
+    via1_sixty_hz_update(v1s);
 }
 
 static const MemoryRegionOps mos6522_q800_via1_ops = {
@@ -983,8 +983,8 @@ static void mac_via_reset(DeviceState *dev)
 
     adb_set_autopoll_enabled(adb_bus, true);
 
-    timer_del(v1s->VBL_timer);
-    v1s->next_VBL = 0;
+    timer_del(v1s->sixty_hz_timer);
+    v1s->next_sixty_hz = 0;
     timer_del(v1s->one_second_timer);
     v1s->next_second = 0;
 
@@ -1026,8 +1026,9 @@ static void mac_via_realize(DeviceState *dev, Error **errp)
     m->mos6522_via1.one_second_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
                                                      via1_one_second,
                                                      &m->mos6522_via1);
-    m->mos6522_via1.VBL_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, via1_VBL,
-                                              &m->mos6522_via1);
+    m->mos6522_via1.sixty_hz_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+                                                  via1_sixty_hz,
+                                                  &m->mos6522_via1);
 
     qemu_get_timedate(&tm, 0);
     m->tick_offset = (uint32_t)mktimegm(&tm) + RTC_OFFSET;
@@ -1116,8 +1117,8 @@ static const VMStateDescription vmstate_mac_via = {
         VMSTATE_BUFFER(mos6522_via1.PRAM, MacVIAState),
         VMSTATE_TIMER_PTR(mos6522_via1.one_second_timer, MacVIAState),
         VMSTATE_INT64(mos6522_via1.next_second, MacVIAState),
-        VMSTATE_TIMER_PTR(mos6522_via1.VBL_timer, MacVIAState),
-        VMSTATE_INT64(mos6522_via1.next_VBL, MacVIAState),
+        VMSTATE_TIMER_PTR(mos6522_via1.sixty_hz_timer, MacVIAState),
+        VMSTATE_INT64(mos6522_via1.next_sixty_hz, MacVIAState),
         VMSTATE_STRUCT(mos6522_via2.parent_obj, MacVIAState, 0, vmstate_mos6522,
                        MOS6522State),
         /* RTC */
diff --git a/include/hw/misc/mac_via.h b/include/hw/misc/mac_via.h
index a59f0bd422..3058b30685 100644
--- a/include/hw/misc/mac_via.h
+++ b/include/hw/misc/mac_via.h
@@ -17,7 +17,7 @@
 
 /* VIA 1 */
 #define VIA1_IRQ_ONE_SECOND_BIT 0
-#define VIA1_IRQ_VBLANK_BIT     1
+#define VIA1_IRQ_60HZ_BIT       1
 #define VIA1_IRQ_ADB_READY_BIT  2
 #define VIA1_IRQ_ADB_DATA_BIT   3
 #define VIA1_IRQ_ADB_CLOCK_BIT  4
@@ -25,7 +25,7 @@
 #define VIA1_IRQ_NB             8
 
 #define VIA1_IRQ_ONE_SECOND (1 << VIA1_IRQ_ONE_SECOND_BIT)
-#define VIA1_IRQ_VBLANK     (1 << VIA1_IRQ_VBLANK_BIT)
+#define VIA1_IRQ_60HZ       (1 << VIA1_IRQ_60HZ_BIT)
 #define VIA1_IRQ_ADB_READY  (1 << VIA1_IRQ_ADB_READY_BIT)
 #define VIA1_IRQ_ADB_DATA   (1 << VIA1_IRQ_ADB_DATA_BIT)
 #define VIA1_IRQ_ADB_CLOCK  (1 << VIA1_IRQ_ADB_CLOCK_BIT)
@@ -45,8 +45,8 @@ struct MOS6522Q800VIA1State {
     /* external timers */
     QEMUTimer *one_second_timer;
     int64_t next_second;
-    QEMUTimer *VBL_timer;
-    int64_t next_VBL;
+    QEMUTimer *sixty_hz_timer;
+    int64_t next_sixty_hz;
 };
 
 
-- 
2.20.1

[PATCH 6/7] mac_via: fix 60Hz VIA1 timer interval

[Mark Cave-Ayland]

The 60Hz timer is initialised using timer_new_ns() meaning that the timer
interval should be measured in ns, and therefore its period is a thousand
times too short.

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

diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index f994fefa7c..c6e1552a59 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -302,8 +302,8 @@ static void via1_sixty_hz_update(MOS6522Q800VIA1State *v1s)
     MOS6522State *s = MOS6522(v1s);
 
     /* 60 Hz irq */
-    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630) /
-                          16630 * 16630;
+    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630000) /
+                          16630000 * 16630000;
 
     if (s->ier & VIA1_IRQ_60HZ) {
         timer_mod(v1s->sixty_hz_timer, v1s->next_sixty_hz);
-- 
2.20.1

[PATCH 7/7] mac_via: remove VIA1 timer optimisations

[Mark Cave-Ayland]

The original implementation of the Macintosh VIA devices in commit 6dca62a000
"hw/m68k: add VIA support" used timer optimisations to reduce high CPU usage on
the host when booting Linux. These optimisations worked by waiting until VIA1
port B was accessed before re-arming the timers.

The MacOS toolbox ROM constantly writes to VIA1 port B which calls
via1_one_second_update() and via1_sixty_hz_update() to calculate the new expiry
time, causing the timers to constantly reset and never fire. The effect of this
is that the Ticks (0x16a) global variable holding the number of 60Hz timer ticks
since reset is never incremented by the interrupt causing time to stand still.

Whilst the code was introduced as a performance optimisation, it is likely that
the high CPU usage was actually caused by the incorrect 60Hz timer interval
fixed in the previous patch. Remove the optimisation to keep everything simple
and enable the MacOS toolbox ROM to start keeping time.

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

diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
index c6e1552a59..23b11dd522 100644
--- a/hw/misc/mac_via.c
+++ b/hw/misc/mac_via.c
@@ -299,30 +299,17 @@ enum {
 
 static void via1_sixty_hz_update(MOS6522Q800VIA1State *v1s)
 {
-    MOS6522State *s = MOS6522(v1s);
-
     /* 60 Hz irq */
     v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630000) /
                           16630000 * 16630000;
-
-    if (s->ier & VIA1_IRQ_60HZ) {
-        timer_mod(v1s->sixty_hz_timer, v1s->next_sixty_hz);
-    } else {
-        timer_del(v1s->sixty_hz_timer);
-    }
+    timer_mod(v1s->sixty_hz_timer, v1s->next_sixty_hz);
 }
 
 static void via1_one_second_update(MOS6522Q800VIA1State *v1s)
 {
-    MOS6522State *s = MOS6522(v1s);
-
     v1s->next_second = (qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 1000) /
                        1000 * 1000;
-    if (s->ier & VIA1_IRQ_ONE_SECOND) {
-        timer_mod(v1s->one_second_timer, v1s->next_second);
-    } else {
-        timer_del(v1s->one_second_timer);
-    }
+    timer_mod(v1s->one_second_timer, v1s->next_second);
 }
 
 static void via1_sixty_hz(void *opaque)
@@ -893,21 +880,6 @@ static uint64_t mos6522_q800_via1_read(void *opaque, hwaddr addr, unsigned size)
 {
     MOS6522Q800VIA1State *s = MOS6522_Q800_VIA1(opaque);
     MOS6522State *ms = MOS6522(s);
-    int64_t now = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
-
-    /*
-     * If IRQs are disabled, timers are disabled, but we need to update
-     * VIA1_IRQ_60HZ and VIA1_IRQ_ONE_SECOND bits in the IFR
-     */
-
-    if (now >= s->next_sixty_hz) {
-        ms->ifr |= VIA1_IRQ_60HZ;
-        via1_sixty_hz_update(s);
-    }
-    if (now >= s->next_second) {
-        ms->ifr |= VIA1_IRQ_ONE_SECOND;
-        via1_one_second_update(s);
-    }
 
     addr = (addr >> 9) & 0xf;
     return mos6522_read(ms, addr, size);
@@ -931,9 +903,6 @@ static void mos6522_q800_via1_write(void *opaque, hwaddr addr, uint64_t val,
         v1s->last_b = ms->b;
         break;
     }
-
-    via1_one_second_update(v1s);
-    via1_sixty_hz_update(v1s);
 }
 
 static const MemoryRegionOps mos6522_q800_via1_ops = {
@@ -978,16 +947,10 @@ static const MemoryRegionOps mos6522_q800_via2_ops = {
 static void mac_via_reset(DeviceState *dev)
 {
     MacVIAState *m = MAC_VIA(dev);
-    MOS6522Q800VIA1State *v1s = &m->mos6522_via1;
     ADBBusState *adb_bus = &m->adb_bus;
 
     adb_set_autopoll_enabled(adb_bus, true);
 
-    timer_del(v1s->sixty_hz_timer);
-    v1s->next_sixty_hz = 0;
-    timer_del(v1s->one_second_timer);
-    v1s->next_second = 0;
-
     m->cmd = REG_EMPTY;
     m->alt = REG_EMPTY;
 }
@@ -1026,9 +989,11 @@ static void mac_via_realize(DeviceState *dev, Error **errp)
     m->mos6522_via1.one_second_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
                                                      via1_one_second,
                                                      &m->mos6522_via1);
+    via1_one_second_update(&m->mos6522_via1);
     m->mos6522_via1.sixty_hz_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
                                                   via1_sixty_hz,
                                                   &m->mos6522_via1);
+    via1_sixty_hz_update(&m->mos6522_via1);
 
     qemu_get_timedate(&tm, 0);
     m->tick_offset = (uint32_t)mktimegm(&tm) + RTC_OFFSET;
-- 
2.20.1

Re: [PATCH 1/7] mac_via: switch rtc pram trace-events to use hex rather than decimal for addresses

[Laurent Vivier]

Le 10/03/2021 à 09:09, Mark Cave-Ayland a écrit :
> Since all the documentation uses the hex offsets, this makes it much easier
> to see what is going on.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/misc/trace-events | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/misc/trace-events b/hw/misc/trace-events
> index d626b9d7a7..73191b5e9d 100644
> --- a/hw/misc/trace-events
> +++ b/hw/misc/trace-events
> @@ -221,8 +221,8 @@ via1_rtc_cmd_test_write(int value) "value=0x%02x"
>  via1_rtc_cmd_wprotect_write(int value) "value=0x%02x"
>  via1_rtc_cmd_pram_read(int addr, int value) "addr=%u value=0x%02x"
>  via1_rtc_cmd_pram_write(int addr, int value) "addr=%u value=0x%02x"
> -via1_rtc_cmd_pram_sect_read(int sector, int offset, int addr, int value) "sector=%u offset=%u addr=%d value=0x%02x"
> -via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "sector=%u offset=%u addr=%d value=0x%02x"
> +via1_rtc_cmd_pram_sect_read(int sector, int offset, int addr, int value) "sector=%u offset=%u addr=0x%x value=0x%02x"
> +via1_rtc_cmd_pram_sect_write(int sector, int offset, int addr, int value) "sector=%u offset=%u addr=0x%x value=0x%02x"
>  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"
> 

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

Re: [PATCH 5/7] mac_via: rename VBL timer to 60Hz timer

[Laurent Vivier]

Le 10/03/2021 à 09:09, Mark Cave-Ayland a écrit :
> According to the "Guide To The Macintosh Family Hardware", the 60Hz VIA1 timer
> on newer Macs such as the Quadra only exists for compatibility with old software
> and is no longer synced to the VBL interval.
> 
> Rename the VBL timer to 60Hz timer to emphasise this and to prevent confusion
> when the real VBL interrupt (now handled as a NuBus slot interrupt) is added in
> future.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/misc/mac_via.c         | 41 ++++++++++++++++++++-------------------
>  include/hw/misc/mac_via.h |  8 ++++----
>  2 files changed, 25 insertions(+), 24 deletions(-)
> 
> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> index 76f31b8cae..f994fefa7c 100644
> --- a/hw/misc/mac_via.c
> +++ b/hw/misc/mac_via.c
> @@ -297,18 +297,18 @@ enum {
>      REG_EMPTY = 0xff,
>  };
>  
> -static void via1_VBL_update(MOS6522Q800VIA1State *v1s)
> +static void via1_sixty_hz_update(MOS6522Q800VIA1State *v1s)
>  {
>      MOS6522State *s = MOS6522(v1s);
>  
>      /* 60 Hz irq */
> -    v1s->next_VBL = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630) /
> -                    16630 * 16630;
> +    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630) /
> +                          16630 * 16630;
>  
> -    if (s->ier & VIA1_IRQ_VBLANK) {
> -        timer_mod(v1s->VBL_timer, v1s->next_VBL);
> +    if (s->ier & VIA1_IRQ_60HZ) {
> +        timer_mod(v1s->sixty_hz_timer, v1s->next_sixty_hz);
>      } else {
> -        timer_del(v1s->VBL_timer);
> +        timer_del(v1s->sixty_hz_timer);
>      }
>  }
>  
> @@ -325,16 +325,16 @@ static void via1_one_second_update(MOS6522Q800VIA1State *v1s)
>      }
>  }
>  
> -static void via1_VBL(void *opaque)
> +static void via1_sixty_hz(void *opaque)
>  {
>      MOS6522Q800VIA1State *v1s = opaque;
>      MOS6522State *s = MOS6522(v1s);
>      MOS6522DeviceClass *mdc = MOS6522_GET_CLASS(s);
>  
> -    s->ifr |= VIA1_IRQ_VBLANK;
> +    s->ifr |= VIA1_IRQ_60HZ;
>      mdc->update_irq(s);
>  
> -    via1_VBL_update(v1s);
> +    via1_sixty_hz_update(v1s);
>  }
>  
>  static void via1_one_second(void *opaque)
> @@ -897,12 +897,12 @@ static uint64_t mos6522_q800_via1_read(void *opaque, hwaddr addr, unsigned size)
>  
>      /*
>       * If IRQs are disabled, timers are disabled, but we need to update
> -     * VIA1_IRQ_VBLANK and VIA1_IRQ_ONE_SECOND bits in the IFR
> +     * VIA1_IRQ_60HZ and VIA1_IRQ_ONE_SECOND bits in the IFR
>       */
>  
> -    if (now >= s->next_VBL) {
> -        ms->ifr |= VIA1_IRQ_VBLANK;
> -        via1_VBL_update(s);
> +    if (now >= s->next_sixty_hz) {
> +        ms->ifr |= VIA1_IRQ_60HZ;
> +        via1_sixty_hz_update(s);
>      }
>      if (now >= s->next_second) {
>          ms->ifr |= VIA1_IRQ_ONE_SECOND;
> @@ -933,7 +933,7 @@ static void mos6522_q800_via1_write(void *opaque, hwaddr addr, uint64_t val,
>      }
>  
>      via1_one_second_update(v1s);
> -    via1_VBL_update(v1s);
> +    via1_sixty_hz_update(v1s);
>  }
>  
>  static const MemoryRegionOps mos6522_q800_via1_ops = {
> @@ -983,8 +983,8 @@ static void mac_via_reset(DeviceState *dev)
>  
>      adb_set_autopoll_enabled(adb_bus, true);
>  
> -    timer_del(v1s->VBL_timer);
> -    v1s->next_VBL = 0;
> +    timer_del(v1s->sixty_hz_timer);
> +    v1s->next_sixty_hz = 0;
>      timer_del(v1s->one_second_timer);
>      v1s->next_second = 0;
>  
> @@ -1026,8 +1026,9 @@ static void mac_via_realize(DeviceState *dev, Error **errp)
>      m->mos6522_via1.one_second_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
>                                                       via1_one_second,
>                                                       &m->mos6522_via1);
> -    m->mos6522_via1.VBL_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, via1_VBL,
> -                                              &m->mos6522_via1);
> +    m->mos6522_via1.sixty_hz_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> +                                                  via1_sixty_hz,
> +                                                  &m->mos6522_via1);
>  
>      qemu_get_timedate(&tm, 0);
>      m->tick_offset = (uint32_t)mktimegm(&tm) + RTC_OFFSET;
> @@ -1116,8 +1117,8 @@ static const VMStateDescription vmstate_mac_via = {
>          VMSTATE_BUFFER(mos6522_via1.PRAM, MacVIAState),
>          VMSTATE_TIMER_PTR(mos6522_via1.one_second_timer, MacVIAState),
>          VMSTATE_INT64(mos6522_via1.next_second, MacVIAState),
> -        VMSTATE_TIMER_PTR(mos6522_via1.VBL_timer, MacVIAState),
> -        VMSTATE_INT64(mos6522_via1.next_VBL, MacVIAState),
> +        VMSTATE_TIMER_PTR(mos6522_via1.sixty_hz_timer, MacVIAState),
> +        VMSTATE_INT64(mos6522_via1.next_sixty_hz, MacVIAState),
>          VMSTATE_STRUCT(mos6522_via2.parent_obj, MacVIAState, 0, vmstate_mos6522,
>                         MOS6522State),
>          /* RTC */
> diff --git a/include/hw/misc/mac_via.h b/include/hw/misc/mac_via.h
> index a59f0bd422..3058b30685 100644
> --- a/include/hw/misc/mac_via.h
> +++ b/include/hw/misc/mac_via.h
> @@ -17,7 +17,7 @@
>  
>  /* VIA 1 */
>  #define VIA1_IRQ_ONE_SECOND_BIT 0
> -#define VIA1_IRQ_VBLANK_BIT     1
> +#define VIA1_IRQ_60HZ_BIT       1
>  #define VIA1_IRQ_ADB_READY_BIT  2
>  #define VIA1_IRQ_ADB_DATA_BIT   3
>  #define VIA1_IRQ_ADB_CLOCK_BIT  4
> @@ -25,7 +25,7 @@
>  #define VIA1_IRQ_NB             8
>  
>  #define VIA1_IRQ_ONE_SECOND (1 << VIA1_IRQ_ONE_SECOND_BIT)
> -#define VIA1_IRQ_VBLANK     (1 << VIA1_IRQ_VBLANK_BIT)
> +#define VIA1_IRQ_60HZ       (1 << VIA1_IRQ_60HZ_BIT)
>  #define VIA1_IRQ_ADB_READY  (1 << VIA1_IRQ_ADB_READY_BIT)
>  #define VIA1_IRQ_ADB_DATA   (1 << VIA1_IRQ_ADB_DATA_BIT)
>  #define VIA1_IRQ_ADB_CLOCK  (1 << VIA1_IRQ_ADB_CLOCK_BIT)
> @@ -45,8 +45,8 @@ struct MOS6522Q800VIA1State {
>      /* external timers */
>      QEMUTimer *one_second_timer;
>      int64_t next_second;
> -    QEMUTimer *VBL_timer;
> -    int64_t next_VBL;
> +    QEMUTimer *sixty_hz_timer;
> +    int64_t next_sixty_hz;
>  };
>  
>  
> 

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

Re: [PATCH 6/7] mac_via: fix 60Hz VIA1 timer interval

[Laurent Vivier]

Le 10/03/2021 à 09:09, Mark Cave-Ayland a écrit :
> The 60Hz timer is initialised using timer_new_ns() meaning that the timer
> interval should be measured in ns, and therefore its period is a thousand
> times too short.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/misc/mac_via.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> index f994fefa7c..c6e1552a59 100644
> --- a/hw/misc/mac_via.c
> +++ b/hw/misc/mac_via.c
> @@ -302,8 +302,8 @@ static void via1_sixty_hz_update(MOS6522Q800VIA1State *v1s)
>      MOS6522State *s = MOS6522(v1s);
>  
>      /* 60 Hz irq */
> -    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630) /
> -                          16630 * 16630;
> +    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630000) /
> +                          16630000 * 16630000;
>  
>      if (s->ier & VIA1_IRQ_60HZ) {
>          timer_mod(v1s->sixty_hz_timer, v1s->next_sixty_hz);
> 

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

Re: [PATCH 7/7] mac_via: remove VIA1 timer optimisations

[Laurent Vivier]

Le 10/03/2021 à 09:09, Mark Cave-Ayland a écrit :
> The original implementation of the Macintosh VIA devices in commit 6dca62a000
> "hw/m68k: add VIA support" used timer optimisations to reduce high CPU usage on
> the host when booting Linux. These optimisations worked by waiting until VIA1
> port B was accessed before re-arming the timers.
> 
> The MacOS toolbox ROM constantly writes to VIA1 port B which calls
> via1_one_second_update() and via1_sixty_hz_update() to calculate the new expiry
> time, causing the timers to constantly reset and never fire. The effect of this
> is that the Ticks (0x16a) global variable holding the number of 60Hz timer ticks
> since reset is never incremented by the interrupt causing time to stand still.
> 
> Whilst the code was introduced as a performance optimisation, it is likely that
> the high CPU usage was actually caused by the incorrect 60Hz timer interval
> fixed in the previous patch. Remove the optimisation to keep everything simple
> and enable the MacOS toolbox ROM to start keeping time.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/misc/mac_via.c | 43 ++++---------------------------------------
>  1 file changed, 4 insertions(+), 39 deletions(-)
> 
> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> index c6e1552a59..23b11dd522 100644
> --- a/hw/misc/mac_via.c
> +++ b/hw/misc/mac_via.c
> @@ -299,30 +299,17 @@ enum {
>  
>  static void via1_sixty_hz_update(MOS6522Q800VIA1State *v1s)
>  {
> -    MOS6522State *s = MOS6522(v1s);
> -
>      /* 60 Hz irq */
>      v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630000) /
>                            16630000 * 16630000;
> -
> -    if (s->ier & VIA1_IRQ_60HZ) {
> -        timer_mod(v1s->sixty_hz_timer, v1s->next_sixty_hz);
> -    } else {
> -        timer_del(v1s->sixty_hz_timer);
> -    }
> +    timer_mod(v1s->sixty_hz_timer, v1s->next_sixty_hz);
>  }
>  
>  static void via1_one_second_update(MOS6522Q800VIA1State *v1s)
>  {
> -    MOS6522State *s = MOS6522(v1s);
> -
>      v1s->next_second = (qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL) + 1000) /
>                         1000 * 1000;
> -    if (s->ier & VIA1_IRQ_ONE_SECOND) {
> -        timer_mod(v1s->one_second_timer, v1s->next_second);
> -    } else {
> -        timer_del(v1s->one_second_timer);
> -    }
> +    timer_mod(v1s->one_second_timer, v1s->next_second);
>  }
>  
>  static void via1_sixty_hz(void *opaque)
> @@ -893,21 +880,6 @@ static uint64_t mos6522_q800_via1_read(void *opaque, hwaddr addr, unsigned size)
>  {
>      MOS6522Q800VIA1State *s = MOS6522_Q800_VIA1(opaque);
>      MOS6522State *ms = MOS6522(s);
> -    int64_t now = qemu_clock_get_ms(QEMU_CLOCK_VIRTUAL);
> -
> -    /*
> -     * If IRQs are disabled, timers are disabled, but we need to update
> -     * VIA1_IRQ_60HZ and VIA1_IRQ_ONE_SECOND bits in the IFR
> -     */
> -
> -    if (now >= s->next_sixty_hz) {
> -        ms->ifr |= VIA1_IRQ_60HZ;
> -        via1_sixty_hz_update(s);
> -    }
> -    if (now >= s->next_second) {
> -        ms->ifr |= VIA1_IRQ_ONE_SECOND;
> -        via1_one_second_update(s);
> -    }
>  
>      addr = (addr >> 9) & 0xf;
>      return mos6522_read(ms, addr, size);
> @@ -931,9 +903,6 @@ static void mos6522_q800_via1_write(void *opaque, hwaddr addr, uint64_t val,
>          v1s->last_b = ms->b;
>          break;
>      }
> -
> -    via1_one_second_update(v1s);
> -    via1_sixty_hz_update(v1s);
>  }
>  
>  static const MemoryRegionOps mos6522_q800_via1_ops = {
> @@ -978,16 +947,10 @@ static const MemoryRegionOps mos6522_q800_via2_ops = {
>  static void mac_via_reset(DeviceState *dev)
>  {
>      MacVIAState *m = MAC_VIA(dev);
> -    MOS6522Q800VIA1State *v1s = &m->mos6522_via1;
>      ADBBusState *adb_bus = &m->adb_bus;
>  
>      adb_set_autopoll_enabled(adb_bus, true);
>  
> -    timer_del(v1s->sixty_hz_timer);
> -    v1s->next_sixty_hz = 0;
> -    timer_del(v1s->one_second_timer);
> -    v1s->next_second = 0;
> -
>      m->cmd = REG_EMPTY;
>      m->alt = REG_EMPTY;
>  }
> @@ -1026,9 +989,11 @@ static void mac_via_realize(DeviceState *dev, Error **errp)
>      m->mos6522_via1.one_second_timer = timer_new_ms(QEMU_CLOCK_VIRTUAL,
>                                                       via1_one_second,
>                                                       &m->mos6522_via1);
> +    via1_one_second_update(&m->mos6522_via1);
>      m->mos6522_via1.sixty_hz_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
>                                                    via1_sixty_hz,
>                                                    &m->mos6522_via1);
> +    via1_sixty_hz_update(&m->mos6522_via1);
>  
>      qemu_get_timedate(&tm, 0);
>      m->tick_offset = (uint32_t)mktimegm(&tm) + RTC_OFFSET;
> 

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

Re: [PATCH 2/7] mac_via: fix up adb_via_receive() trace events

[Laurent Vivier]

Le 10/03/2021 à 09:09, Mark Cave-Ayland a écrit :
> The use of the post-increment operator on adb_data_in_index meant that the
> trace-event was accidentally displaying the next byte in the incoming ADB
> data buffer rather than the current byte.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/misc/mac_via.c | 41 ++++++++++++++++++++++++-----------------
>  1 file changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> index 488d086a17..0f6586e102 100644
> --- a/hw/misc/mac_via.c
> +++ b/hw/misc/mac_via.c
> @@ -816,33 +816,37 @@ static void adb_via_receive(MacVIAState *s, int state, uint8_t *data)
>          switch (s->adb_data_in_index) {
>          case 0:
>              /* First EVEN byte: vADBInt indicates bus timeout */
> -            trace_via1_adb_receive(state == ADB_STATE_EVEN ? "EVEN" : " ODD",
> -                                   *data, (ms->b & VIA1B_vADBInt) ? "+" : "-",
> -                                   adb_bus->status, s->adb_data_in_index,
> -                                   s->adb_data_in_size);
> -
> -            *data = s->adb_data_in[s->adb_data_in_index++];
> +            *data = s->adb_data_in[s->adb_data_in_index];
>              if (adb_bus->status & ADB_STATUS_BUSTIMEOUT) {
>                  ms->b &= ~VIA1B_vADBInt;
>              } else {
>                  ms->b |= VIA1B_vADBInt;
>              }
> -            break;
>  
> -        case 1:
> -            /* First ODD byte: vADBInt indicates SRQ */
>              trace_via1_adb_receive(state == ADB_STATE_EVEN ? "EVEN" : " ODD",
>                                     *data, (ms->b & VIA1B_vADBInt) ? "+" : "-",
>                                     adb_bus->status, s->adb_data_in_index,
>                                     s->adb_data_in_size);
>  
> -            *data = s->adb_data_in[s->adb_data_in_index++];
> +            s->adb_data_in_index++;
> +            break;
> +
> +        case 1:
> +            /* First ODD byte: vADBInt indicates SRQ */
> +            *data = s->adb_data_in[s->adb_data_in_index];
>              pending = adb_bus->pending & ~(1 << (s->adb_autopoll_cmd >> 4));
>              if (pending) {
>                  ms->b &= ~VIA1B_vADBInt;
>              } else {
>                  ms->b |= VIA1B_vADBInt;
>              }
> +
> +            trace_via1_adb_receive(state == ADB_STATE_EVEN ? "EVEN" : " ODD",
> +                                   *data, (ms->b & VIA1B_vADBInt) ? "+" : "-",
> +                                   adb_bus->status, s->adb_data_in_index,
> +                                   s->adb_data_in_size);
> +
> +            s->adb_data_in_index++;
>              break;
>  
>          default:
> @@ -852,14 +856,9 @@ static void adb_via_receive(MacVIAState *s, int state, uint8_t *data)
>               * end of the poll reply, so provide these extra bytes below to
>               * keep it happy
>               */
> -            trace_via1_adb_receive(state == ADB_STATE_EVEN ? "EVEN" : " ODD",
> -                                   *data, (ms->b & VIA1B_vADBInt) ? "+" : "-",
> -                                   adb_bus->status, s->adb_data_in_index,
> -                                   s->adb_data_in_size);
> -
>              if (s->adb_data_in_index < s->adb_data_in_size) {
>                  /* Next data byte */
> -                *data = s->adb_data_in[s->adb_data_in_index++];
> +                *data = s->adb_data_in[s->adb_data_in_index];
>                  ms->b |= VIA1B_vADBInt;
>              } else if (s->adb_data_in_index == s->adb_data_in_size) {
>                  if (adb_bus->status & ADB_STATUS_BUSTIMEOUT) {
> @@ -869,7 +868,6 @@ static void adb_via_receive(MacVIAState *s, int state, uint8_t *data)
>                      /* Return 0x0 after reply */
>                      *data = 0;
>                  }
> -                s->adb_data_in_index++;
>                  ms->b &= ~VIA1B_vADBInt;
>              } else {
>                  /* Bus timeout (no more data) */
> @@ -878,6 +876,15 @@ static void adb_via_receive(MacVIAState *s, int state, uint8_t *data)
>                  adb_bus->status = 0;
>                  adb_autopoll_unblock(adb_bus);
>              }
> +
> +            trace_via1_adb_receive(state == ADB_STATE_EVEN ? "EVEN" : " ODD",
> +                                   *data, (ms->b & VIA1B_vADBInt) ? "+" : "-",
> +                                   adb_bus->status, s->adb_data_in_index,
> +                                   s->adb_data_in_size);
> +
> +            if (s->adb_data_in_index <= s->adb_data_in_size) {
> +                s->adb_data_in_index++;
> +            }
>              break;
>          }
>  
> 

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

Re: [PATCH 3/7] mac_via: allow long accesses to VIA registers

[Laurent Vivier]

Le 10/03/2021 à 09:09, Mark Cave-Ayland a écrit :
> The MacOS SCSI driver uses a long access to read the VIA registers rather than
> just a single byte during the message out phase.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/misc/mac_via.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> index 0f6586e102..f38d6e2f6e 100644
> --- a/hw/misc/mac_via.c
> +++ b/hw/misc/mac_via.c
> @@ -966,7 +966,7 @@ static const MemoryRegionOps mos6522_q800_via1_ops = {
>      .endianness = DEVICE_BIG_ENDIAN,
>      .valid = {
>          .min_access_size = 1,
> -        .max_access_size = 1,
> +        .max_access_size = 4,
>      },
>  };
>  
> @@ -995,7 +995,7 @@ static const MemoryRegionOps mos6522_q800_via2_ops = {
>      .endianness = DEVICE_BIG_ENDIAN,
>      .valid = {
>          .min_access_size = 1,
> -        .max_access_size = 1,
> +        .max_access_size = 4,
>      },
>  };
>  
> 

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

Re: [PATCH 6/7] mac_via: fix 60Hz VIA1 timer interval

[BALATON Zoltan]

On Wed, 10 Mar 2021, Mark Cave-Ayland wrote:
> The 60Hz timer is initialised using timer_new_ns() meaning that the timer
> interval should be measured in ns, and therefore its period is a thousand
> times too short.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
> hw/misc/mac_via.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
> index f994fefa7c..c6e1552a59 100644
> --- a/hw/misc/mac_via.c
> +++ b/hw/misc/mac_via.c
> @@ -302,8 +302,8 @@ static void via1_sixty_hz_update(MOS6522Q800VIA1State *v1s)
>     MOS6522State *s = MOS6522(v1s);
>
>     /* 60 Hz irq */
> -    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630) /
> -                          16630 * 16630;
> +    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630000) /
> +                          16630000 * 16630000;

Can you put this magic number in a #define maybe also rewriting it in a 
way that shows it corresponds to a 60 Hz interval. (There's 
NANOSECONDS_PER_SECOND defined in include/qemu/timer.h that could be used 
for that, there's also SCALE_MS that might replace 1000 * 1000 elsewhere 
in this file). Also NANOSECONDS_PER_SECOND / 60 is 16666666, should that 
value be used here instead?

Regards,
BALATON Zoltan

>
>     if (s->ier & VIA1_IRQ_60HZ) {
>         timer_mod(v1s->sixty_hz_timer, v1s->next_sixty_hz);
>

Re: [PATCH 6/7] mac_via: fix 60Hz VIA1 timer interval

[Laurent Vivier]

Le 10/03/2021 à 13:32, BALATON Zoltan a écrit :
> On Wed, 10 Mar 2021, Mark Cave-Ayland wrote:
>> The 60Hz timer is initialised using timer_new_ns() meaning that the timer
>> interval should be measured in ns, and therefore its period is a thousand
>> times too short.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>> hw/misc/mac_via.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>> index f994fefa7c..c6e1552a59 100644
>> --- a/hw/misc/mac_via.c
>> +++ b/hw/misc/mac_via.c
>> @@ -302,8 +302,8 @@ static void via1_sixty_hz_update(MOS6522Q800VIA1State *v1s)
>>     MOS6522State *s = MOS6522(v1s);
>>
>>     /* 60 Hz irq */
>> -    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630) /
>> -                          16630 * 16630;
>> +    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630000) /
>> +                          16630000 * 16630000;
> 
> Can you put this magic number in a #define maybe also rewriting it in a way that shows it
> corresponds to a 60 Hz interval. (There's NANOSECONDS_PER_SECOND defined in include/qemu/timer.h
> that could be used for that, there's also SCALE_MS that might replace 1000 * 1000 elsewhere in this
> file). Also NANOSECONDS_PER_SECOND / 60 is 16666666, should that value be used here instead?

In fact, the Mac Frequency is not exactly 60 Hz, in docs we can find 60.147 Hz, in kernel 60.15 Hz.
I Think there are several ways to compute it...

Thanks,
Laurent

Re: [PATCH 6/7] mac_via: fix 60Hz VIA1 timer interval

[Laurent Vivier]

Le 10/03/2021 à 13:56, Laurent Vivier a écrit :
> Le 10/03/2021 à 13:32, BALATON Zoltan a écrit :
>> On Wed, 10 Mar 2021, Mark Cave-Ayland wrote:
>>> The 60Hz timer is initialised using timer_new_ns() meaning that the timer
>>> interval should be measured in ns, and therefore its period is a thousand
>>> times too short.
>>>
>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>> ---
>>> hw/misc/mac_via.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>>> index f994fefa7c..c6e1552a59 100644
>>> --- a/hw/misc/mac_via.c
>>> +++ b/hw/misc/mac_via.c
>>> @@ -302,8 +302,8 @@ static void via1_sixty_hz_update(MOS6522Q800VIA1State *v1s)
>>>     MOS6522State *s = MOS6522(v1s);
>>>
>>>     /* 60 Hz irq */
>>> -    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630) /
>>> -                          16630 * 16630;
>>> +    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630000) /
>>> +                          16630000 * 16630000;
>>
>> Can you put this magic number in a #define maybe also rewriting it in a way that shows it
>> corresponds to a 60 Hz interval. (There's NANOSECONDS_PER_SECOND defined in include/qemu/timer.h
>> that could be used for that, there's also SCALE_MS that might replace 1000 * 1000 elsewhere in this
>> file). Also NANOSECONDS_PER_SECOND / 60 is 16666666, should that value be used here instead?
> 
> In fact, the Mac Frequency is not exactly 60 Hz, in docs we can find 60.147 Hz, in kernel 60.15 Hz.
> I Think there are several ways to compute it...
> 

In fact, we can read:

"the vertical retrace frequency is approximately 60.15 Hz, resulting in a period of approximately
16.63 milliseconds"

https://developer.apple.com/library/archive/documentation/mac/pdf/Processes/Vertical_Retrace_Mgr.pdf

Thanks,
Laurent

Re: [PATCH 6/7] mac_via: fix 60Hz VIA1 timer interval

[Laurent Vivier]

Le 10/03/2021 à 14:10, Laurent Vivier a écrit :
> Le 10/03/2021 à 13:56, Laurent Vivier a écrit :
>> Le 10/03/2021 à 13:32, BALATON Zoltan a écrit :
>>> On Wed, 10 Mar 2021, Mark Cave-Ayland wrote:
>>>> The 60Hz timer is initialised using timer_new_ns() meaning that the timer
>>>> interval should be measured in ns, and therefore its period is a thousand
>>>> times too short.
>>>>
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>> hw/misc/mac_via.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>>>> index f994fefa7c..c6e1552a59 100644
>>>> --- a/hw/misc/mac_via.c
>>>> +++ b/hw/misc/mac_via.c
>>>> @@ -302,8 +302,8 @@ static void via1_sixty_hz_update(MOS6522Q800VIA1State *v1s)
>>>>     MOS6522State *s = MOS6522(v1s);
>>>>
>>>>     /* 60 Hz irq */
>>>> -    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630) /
>>>> -                          16630 * 16630;
>>>> +    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630000) /
>>>> +                          16630000 * 16630000;
>>>
>>> Can you put this magic number in a #define maybe also rewriting it in a way that shows it
>>> corresponds to a 60 Hz interval. (There's NANOSECONDS_PER_SECOND defined in include/qemu/timer.h
>>> that could be used for that, there's also SCALE_MS that might replace 1000 * 1000 elsewhere in this
>>> file). Also NANOSECONDS_PER_SECOND / 60 is 16666666, should that value be used here instead?
>>
>> In fact, the Mac Frequency is not exactly 60 Hz, in docs we can find 60.147 Hz, in kernel 60.15 Hz.
>> I Think there are several ways to compute it...
>>
> 
> In fact, we can read:
> 
> "the vertical retrace frequency is approximately 60.15 Hz, resulting in a period of approximately
> 16.63 milliseconds"
> 
> https://developer.apple.com/library/archive/documentation/mac/pdf/Processes/Vertical_Retrace_Mgr.pdf

The exact value is 16625800 ns

"Macintosh Family Hardware Reference" ISBN 0-201-19255-1
"The video interface"
p. 13-3

"[...] This means the full frame is redisplayed every 370 scan lines, or once every 166625.8 µs."

Thanks,
Laurent

Re: [PATCH 6/7] mac_via: fix 60Hz VIA1 timer interval

[Philippe Mathieu-Daudé]

On 3/10/21 2:10 PM, Laurent Vivier wrote:
> Le 10/03/2021 à 13:56, Laurent Vivier a écrit :
>> Le 10/03/2021 à 13:32, BALATON Zoltan a écrit :
>>> On Wed, 10 Mar 2021, Mark Cave-Ayland wrote:
>>>> The 60Hz timer is initialised using timer_new_ns() meaning that the timer
>>>> interval should be measured in ns, and therefore its period is a thousand
>>>> times too short.
>>>>
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>> ---
>>>> hw/misc/mac_via.c | 4 ++--
>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>>>> index f994fefa7c..c6e1552a59 100644
>>>> --- a/hw/misc/mac_via.c
>>>> +++ b/hw/misc/mac_via.c
>>>> @@ -302,8 +302,8 @@ static void via1_sixty_hz_update(MOS6522Q800VIA1State *v1s)
>>>>     MOS6522State *s = MOS6522(v1s);
>>>>
>>>>     /* 60 Hz irq */
>>>> -    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630) /
>>>> -                          16630 * 16630;
>>>> +    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630000) /
>>>> +                          16630000 * 16630000;
>>>
>>> Can you put this magic number in a #define maybe also rewriting it in a way that shows it
>>> corresponds to a 60 Hz interval. (There's NANOSECONDS_PER_SECOND defined in include/qemu/timer.h
>>> that could be used for that, there's also SCALE_MS that might replace 1000 * 1000 elsewhere in this
>>> file). Also NANOSECONDS_PER_SECOND / 60 is 16666666, should that value be used here instead?
>>
>> In fact, the Mac Frequency is not exactly 60 Hz, in docs we can find 60.147 Hz, in kernel 60.15 Hz.
>> I Think there are several ways to compute it...

Good candidate to switch to the Clock API, see docs/devel/clocks.rst
once this gets merged:
https://www.mail-archive.com/qemu-devel@nongnu.org/msg788864.html

> In fact, we can read:
> 
> "the vertical retrace frequency is approximately 60.15 Hz, resulting in a period of approximately
> 16.63 milliseconds"
> 
> https://developer.apple.com/library/archive/documentation/mac/pdf/Processes/Vertical_Retrace_Mgr.pdf
> 
> Thanks,
> Laurent
> 
> 

Re: [PATCH 6/7] mac_via: fix 60Hz VIA1 timer interval

[Mark Cave-Ayland]

On 10/03/2021 13:24, Laurent Vivier wrote:

> Le 10/03/2021 à 14:10, Laurent Vivier a écrit :
>> Le 10/03/2021 à 13:56, Laurent Vivier a écrit :
>>> Le 10/03/2021 à 13:32, BALATON Zoltan a écrit :
>>>> On Wed, 10 Mar 2021, Mark Cave-Ayland wrote:
>>>>> The 60Hz timer is initialised using timer_new_ns() meaning that the timer
>>>>> interval should be measured in ns, and therefore its period is a thousand
>>>>> times too short.
>>>>>
>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>> ---
>>>>> hw/misc/mac_via.c | 4 ++--
>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>>>>> index f994fefa7c..c6e1552a59 100644
>>>>> --- a/hw/misc/mac_via.c
>>>>> +++ b/hw/misc/mac_via.c
>>>>> @@ -302,8 +302,8 @@ static void via1_sixty_hz_update(MOS6522Q800VIA1State *v1s)
>>>>>      MOS6522State *s = MOS6522(v1s);
>>>>>
>>>>>      /* 60 Hz irq */
>>>>> -    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630) /
>>>>> -                          16630 * 16630;
>>>>> +    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630000) /
>>>>> +                          16630000 * 16630000;
>>>>
>>>> Can you put this magic number in a #define maybe also rewriting it in a way that shows it
>>>> corresponds to a 60 Hz interval. (There's NANOSECONDS_PER_SECOND defined in include/qemu/timer.h
>>>> that could be used for that, there's also SCALE_MS that might replace 1000 * 1000 elsewhere in this
>>>> file). Also NANOSECONDS_PER_SECOND / 60 is 16666666, should that value be used here instead?
>>>
>>> In fact, the Mac Frequency is not exactly 60 Hz, in docs we can find 60.147 Hz, in kernel 60.15 Hz.
>>> I Think there are several ways to compute it...
>>>
>>
>> In fact, we can read:
>>
>> "the vertical retrace frequency is approximately 60.15 Hz, resulting in a period of approximately
>> 16.63 milliseconds"
>>
>> https://developer.apple.com/library/archive/documentation/mac/pdf/Processes/Vertical_Retrace_Mgr.pdf
> 
> The exact value is 16625800 ns
> 
> "Macintosh Family Hardware Reference" ISBN 0-201-19255-1
> "The video interface"
> p. 13-3
> 
> "[...] This means the full frame is redisplayed every 370 scan lines, or once every 166625.8 µs."

Thanks Laurent! Given that the exact precision is 6 digits I don't think it's 
possible to make use of conversion macros without either making it harder to read or 
reducing the precision.

I think the best solution here would be to #define VIA1_60HZ_TIMER_PERIOD_NS with a 
comment containing the above reference, and use that in the period calculation. Would 
that be sufficient?


ATB,

Mark.

Re: [PATCH 6/7] mac_via: fix 60Hz VIA1 timer interval

[BALATON Zoltan]

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

On Wed, 10 Mar 2021, Mark Cave-Ayland wrote:
> On 10/03/2021 13:24, Laurent Vivier wrote:
>> Le 10/03/2021 à 14:10, Laurent Vivier a écrit :
>>> Le 10/03/2021 à 13:56, Laurent Vivier a écrit :
>>>> Le 10/03/2021 à 13:32, BALATON Zoltan a écrit :
>>>>> On Wed, 10 Mar 2021, Mark Cave-Ayland wrote:
>>>>>> The 60Hz timer is initialised using timer_new_ns() meaning that the 
>>>>>> timer
>>>>>> interval should be measured in ns, and therefore its period is a 
>>>>>> thousand
>>>>>> times too short.
>>>>>> 
>>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>>> ---
>>>>>> hw/misc/mac_via.c | 4 ++--
>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>> 
>>>>>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>>>>>> index f994fefa7c..c6e1552a59 100644
>>>>>> --- a/hw/misc/mac_via.c
>>>>>> +++ b/hw/misc/mac_via.c
>>>>>> @@ -302,8 +302,8 @@ static void 
>>>>>> via1_sixty_hz_update(MOS6522Q800VIA1State *v1s)
>>>>>>      MOS6522State *s = MOS6522(v1s);
>>>>>>
>>>>>>      /* 60 Hz irq */
>>>>>> -    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 
>>>>>> 16630) /
>>>>>> -                          16630 * 16630;
>>>>>> +    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 
>>>>>> 16630000) /
>>>>>> +                          16630000 * 16630000;
>>>>> 
>>>>> Can you put this magic number in a #define maybe also rewriting it in a 
>>>>> way that shows it
>>>>> corresponds to a 60 Hz interval. (There's NANOSECONDS_PER_SECOND defined 
>>>>> in include/qemu/timer.h
>>>>> that could be used for that, there's also SCALE_MS that might replace 
>>>>> 1000 * 1000 elsewhere in this
>>>>> file). Also NANOSECONDS_PER_SECOND / 60 is 16666666, should that value 
>>>>> be used here instead?
>>>> 
>>>> In fact, the Mac Frequency is not exactly 60 Hz, in docs we can find 
>>>> 60.147 Hz, in kernel 60.15 Hz.
>>>> I Think there are several ways to compute it...
>>>> 
>>> 
>>> In fact, we can read:
>>> 
>>> "the vertical retrace frequency is approximately 60.15 Hz, resulting in a 
>>> period of approximately
>>> 16.63 milliseconds"
>>> 
>>> https://developer.apple.com/library/archive/documentation/mac/pdf/Processes/Vertical_Retrace_Mgr.pdf
>> 
>> The exact value is 16625800 ns
>> 
>> "Macintosh Family Hardware Reference" ISBN 0-201-19255-1
>> "The video interface"
>> p. 13-3
>> 
>> "[...] This means the full frame is redisplayed every 370 scan lines, or 
>> once every 166625.8 µs."
>
> Thanks Laurent! Given that the exact precision is 6 digits I don't think it's 
> possible to make use of conversion macros without either making it harder to 
> read or reducing the precision.
>
> I think the best solution here would be to #define VIA1_60HZ_TIMER_PERIOD_NS 
> with a comment containing the above reference, and use that in the period 
> calculation. Would that be sufficient?

Yes, I think that would make this a lot clearer than having this number 
without explanation so that would be sufficient.

Is this referred to as 60Hz timer in the hardware docs? Because that name 
is a bit misleading when it's actually not exactly 60Hz. But in the 
previous patch VBlank which this was called before was also found 
misleading so I can't think of a better name. Not sure if calling it 
compat_vblank would be too verbose or any better. Maybe you can just add a 
comment explaining what this is somewhere where it's defined or near the 
update function and then it does not matter what you call it, the comment 
should explain why it's not quite sixty_hz. I'm also OK with calling it 
sixty_hz just though if there's a way to give it a less confusing name you 
could consider that but I don't have a better name either so feel free to 
ignore this.

Regards,
BALATON Zoltan

Re: [PATCH 6/7] mac_via: fix 60Hz VIA1 timer interval

[Mark Cave-Ayland]

On 11/03/2021 00:15, BALATON Zoltan wrote:

> On Wed, 10 Mar 2021, Mark Cave-Ayland wrote:
>> On 10/03/2021 13:24, Laurent Vivier wrote:
>>> Le 10/03/2021 à 14:10, Laurent Vivier a écrit :
>>>> Le 10/03/2021 à 13:56, Laurent Vivier a écrit :
>>>>> Le 10/03/2021 à 13:32, BALATON Zoltan a écrit :
>>>>>> On Wed, 10 Mar 2021, Mark Cave-Ayland wrote:
>>>>>>> The 60Hz timer is initialised using timer_new_ns() meaning that the timer
>>>>>>> interval should be measured in ns, and therefore its period is a thousand
>>>>>>> times too short.
>>>>>>>
>>>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>>>> ---
>>>>>>> hw/misc/mac_via.c | 4 ++--
>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>>>>>>> index f994fefa7c..c6e1552a59 100644
>>>>>>> --- a/hw/misc/mac_via.c
>>>>>>> +++ b/hw/misc/mac_via.c
>>>>>>> @@ -302,8 +302,8 @@ static void via1_sixty_hz_update(MOS6522Q800VIA1State *v1s)
>>>>>>>      MOS6522State *s = MOS6522(v1s);
>>>>>>>
>>>>>>>      /* 60 Hz irq */
>>>>>>> -    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630) /
>>>>>>> -                          16630 * 16630;
>>>>>>> +    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630000) /
>>>>>>> +                          16630000 * 16630000;
>>>>>>
>>>>>> Can you put this magic number in a #define maybe also rewriting it in a way 
>>>>>> that shows it
>>>>>> corresponds to a 60 Hz interval. (There's NANOSECONDS_PER_SECOND defined in 
>>>>>> include/qemu/timer.h
>>>>>> that could be used for that, there's also SCALE_MS that might replace 1000 * 
>>>>>> 1000 elsewhere in this
>>>>>> file). Also NANOSECONDS_PER_SECOND / 60 is 16666666, should that value be used 
>>>>>> here instead?
>>>>>
>>>>> In fact, the Mac Frequency is not exactly 60 Hz, in docs we can find 60.147 Hz, 
>>>>> in kernel 60.15 Hz.
>>>>> I Think there are several ways to compute it...
>>>>>
>>>>
>>>> In fact, we can read:
>>>>
>>>> "the vertical retrace frequency is approximately 60.15 Hz, resulting in a period 
>>>> of approximately
>>>> 16.63 milliseconds"
>>>>
>>>> https://developer.apple.com/library/archive/documentation/mac/pdf/Processes/Vertical_Retrace_Mgr.pdf 
>>>>
>>>
>>> The exact value is 16625800 ns
>>>
>>> "Macintosh Family Hardware Reference" ISBN 0-201-19255-1
>>> "The video interface"
>>> p. 13-3
>>>
>>> "[...] This means the full frame is redisplayed every 370 scan lines, or once 
>>> every 166625.8 µs."
>>
>> Thanks Laurent! Given that the exact precision is 6 digits I don't think it's 
>> possible to make use of conversion macros without either making it harder to read 
>> or reducing the precision.
>>
>> I think the best solution here would be to #define VIA1_60HZ_TIMER_PERIOD_NS with a 
>> comment containing the above reference, and use that in the period calculation. 
>> Would that be sufficient?
> 
> Yes, I think that would make this a lot clearer than having this number without 
> explanation so that would be sufficient.
> 
> Is this referred to as 60Hz timer in the hardware docs? Because that name is a bit 
> misleading when it's actually not exactly 60Hz. But in the previous patch VBlank 
> which this was called before was also found misleading so I can't think of a better 
> name. Not sure if calling it compat_vblank would be too verbose or any better. Maybe 
> you can just add a comment explaining what this is somewhere where it's defined or 
> near the update function and then it does not matter what you call it, the comment 
> should explain why it's not quite sixty_hz. I'm also OK with calling it sixty_hz just 
> though if there's a way to give it a less confusing name you could consider that but 
> I don't have a better name either so feel free to ignore this.

Yes indeed, depending upon the documentation it is referred to as either the 60Hz or 
the 60.15Hz timer. Certainly that's enough information for anyone familiar with Mac 
internals to understand exactly what you are referring to. There are also plenty of 
examples of this elsewhere e.g. for graphics cards the high level claim will be that 
it supports over 16 million colours whereas the engineers know specifically that the 
exact number is 16777216.

I'll write up a suitable comment with a #define and send through a v2, perhaps 
altering the comment on the timer itself to read 60.15Hz as that's what the reference 
cited by Laurent also uses.


ATB,

Mark.

Re: [PATCH 6/7] mac_via: fix 60Hz VIA1 timer interval

[Laurent Vivier]

Le 11/03/2021 à 10:04, Mark Cave-Ayland a écrit :
> On 11/03/2021 00:15, BALATON Zoltan wrote:
> 
>> On Wed, 10 Mar 2021, Mark Cave-Ayland wrote:
>>> On 10/03/2021 13:24, Laurent Vivier wrote:
>>>> Le 10/03/2021 à 14:10, Laurent Vivier a écrit :
>>>>> Le 10/03/2021 à 13:56, Laurent Vivier a écrit :
>>>>>> Le 10/03/2021 à 13:32, BALATON Zoltan a écrit :
>>>>>>> On Wed, 10 Mar 2021, Mark Cave-Ayland wrote:
>>>>>>>> The 60Hz timer is initialised using timer_new_ns() meaning that the timer
>>>>>>>> interval should be measured in ns, and therefore its period is a thousand
>>>>>>>> times too short.
>>>>>>>>
>>>>>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>>>>>> ---
>>>>>>>> hw/misc/mac_via.c | 4 ++--
>>>>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/hw/misc/mac_via.c b/hw/misc/mac_via.c
>>>>>>>> index f994fefa7c..c6e1552a59 100644
>>>>>>>> --- a/hw/misc/mac_via.c
>>>>>>>> +++ b/hw/misc/mac_via.c
>>>>>>>> @@ -302,8 +302,8 @@ static void via1_sixty_hz_update(MOS6522Q800VIA1State *v1s)
>>>>>>>>      MOS6522State *s = MOS6522(v1s);
>>>>>>>>
>>>>>>>>      /* 60 Hz irq */
>>>>>>>> -    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630) /
>>>>>>>> -                          16630 * 16630;
>>>>>>>> +    v1s->next_sixty_hz = (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + 16630000) /
>>>>>>>> +                          16630000 * 16630000;
>>>>>>>
>>>>>>> Can you put this magic number in a #define maybe also rewriting it in a way that shows it
>>>>>>> corresponds to a 60 Hz interval. (There's NANOSECONDS_PER_SECOND defined in include/qemu/timer.h
>>>>>>> that could be used for that, there's also SCALE_MS that might replace 1000 * 1000 elsewhere
>>>>>>> in this
>>>>>>> file). Also NANOSECONDS_PER_SECOND / 60 is 16666666, should that value be used here instead?
>>>>>>
>>>>>> In fact, the Mac Frequency is not exactly 60 Hz, in docs we can find 60.147 Hz, in kernel
>>>>>> 60.15 Hz.
>>>>>> I Think there are several ways to compute it...
>>>>>>
>>>>>
>>>>> In fact, we can read:
>>>>>
>>>>> "the vertical retrace frequency is approximately 60.15 Hz, resulting in a period of approximately
>>>>> 16.63 milliseconds"
>>>>>
>>>>> https://developer.apple.com/library/archive/documentation/mac/pdf/Processes/Vertical_Retrace_Mgr.pdf
>>>>>
>>>>
>>>> The exact value is 16625800 ns
>>>>
>>>> "Macintosh Family Hardware Reference" ISBN 0-201-19255-1
>>>> "The video interface"
>>>> p. 13-3
>>>>
>>>> "[...] This means the full frame is redisplayed every 370 scan lines, or once every 166625.8 µs."
>>>
>>> Thanks Laurent! Given that the exact precision is 6 digits I don't think it's possible to make
>>> use of conversion macros without either making it harder to read or reducing the precision.
>>>
>>> I think the best solution here would be to #define VIA1_60HZ_TIMER_PERIOD_NS with a comment
>>> containing the above reference, and use that in the period calculation. Would that be sufficient?
>>
>> Yes, I think that would make this a lot clearer than having this number without explanation so
>> that would be sufficient.
>>
>> Is this referred to as 60Hz timer in the hardware docs? Because that name is a bit misleading when
>> it's actually not exactly 60Hz. But in the previous patch VBlank which this was called before was
>> also found misleading so I can't think of a better name. Not sure if calling it compat_vblank
>> would be too verbose or any better. Maybe you can just add a comment explaining what this is
>> somewhere where it's defined or near the update function and then it does not matter what you call
>> it, the comment should explain why it's not quite sixty_hz. I'm also OK with calling it sixty_hz
>> just though if there's a way to give it a less confusing name you could consider that but I don't
>> have a better name either so feel free to ignore this.
> 
> Yes indeed, depending upon the documentation it is referred to as either the 60Hz or the 60.15Hz
> timer. Certainly that's enough information for anyone familiar with Mac internals to understand
> exactly what you are referring to. There are also plenty of examples of this elsewhere e.g. for
> graphics cards the high level claim will be that it supports over 16 million colours whereas the
> engineers know specifically that the exact number is 16777216.
> 
> I'll write up a suitable comment with a #define and send through a v2, perhaps altering the comment
> on the timer itself to read 60.15Hz as that's what the reference cited by Laurent also uses.

If you want more details, this is also in "Apple Guide to the Macintosh Family Hardware", 2nd
edition, Chapter 12, "Displays", p. 401 (p.440 of the following PDF):

https://archive.org/details/apple-guide-macintosh-family-hardware

Thanks,
Laurent

Re: [PATCH 6/7] mac_via: fix 60Hz VIA1 timer interval

[Mark Cave-Ayland]

On 11/03/2021 09:44, Laurent Vivier wrote:

>> Yes indeed, depending upon the documentation it is referred to as either the 60Hz or the 60.15Hz
>> timer. Certainly that's enough information for anyone familiar with Mac internals to understand
>> exactly what you are referring to. There are also plenty of examples of this elsewhere e.g. for
>> graphics cards the high level claim will be that it supports over 16 million colours whereas the
>> engineers know specifically that the exact number is 16777216.
>>
>> I'll write up a suitable comment with a #define and send through a v2, perhaps altering the comment
>> on the timer itself to read 60.15Hz as that's what the reference cited by Laurent also uses.
> 
> If you want more details, this is also in "Apple Guide to the Macintosh Family Hardware", 2nd
> edition, Chapter 12, "Displays", p. 401 (p.440 of the following PDF):
> 
> https://archive.org/details/apple-guide-macintosh-family-hardware

Ah yes, that's a better reference as I don't see online versions of "Macintosh Family 
Hardware Reference" anywhere. I'll post the v2 shortly.


ATB,

Mark.