[PATCH 0/3] escc: fix R_STATUS when SDLC mode is enabled

[PATCH 0/3] escc: fix R_STATUS when SDLC mode is enabled

[Mark Cave-Ayland]

Here is another small set of ESCC fixes from my attempts to boot MacOS on the q800
machine.

When MacOS loads the OpenTransport extension on boot it attempts to enable
SDLC mode on the ESCC. QEMU's emulation doesn't support SDLC mode, but without
these fixes the active low STATUS_SYNC bit in R_STATUS is continually
asserted causing the extension to hang on startup as it believe it is constantly
receiving LocalTalk responses during its initial negotiation phase.

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


Mark Cave-Ayland (3):
  escc: checkpatch fixes
  escc: fix R_STATUS channel reset value
  escc: fix STATUS_SYNC bit in R_STATUS register

 hw/char/escc.c | 174 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 106 insertions(+), 68 deletions(-)

-- 
2.20.1

[PATCH 1/3] escc: checkpatch fixes

[Mark Cave-Ayland]

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/char/escc.c | 160 +++++++++++++++++++++++++++++--------------------
 1 file changed, 96 insertions(+), 64 deletions(-)

diff --git a/hw/char/escc.c b/hw/char/escc.c
index 52e7978287..63e0f15dfa 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -230,20 +230,23 @@ static uint32_t get_queue(void *opaque)
         q->count--;
     }
     trace_escc_get_queue(CHN_C(s), val);
-    if (q->count > 0)
+    if (q->count > 0) {
         serial_receive_byte(s, 0);
+    }
     return val;
 }
 
 static int escc_update_irq_chn(ESCCChannelState *s)
 {
     if ((((s->wregs[W_INTR] & INTR_TXINT) && (s->txint == 1)) ||
-         // tx ints enabled, pending
-         ((((s->wregs[W_INTR] & INTR_RXMODEMSK) == INTR_RXINT1ST) ||
-           ((s->wregs[W_INTR] & INTR_RXMODEMSK) == INTR_RXINTALL)) &&
-          s->rxint == 1) || // rx ints enabled, pending
-         ((s->wregs[W_EXTINT] & EXTINT_BRKINT) &&
-          (s->rregs[R_STATUS] & STATUS_BRK)))) { // break int e&p
+        /* tx ints enabled, pending */
+        ((((s->wregs[W_INTR] & INTR_RXMODEMSK) == INTR_RXINT1ST) ||
+        ((s->wregs[W_INTR] & INTR_RXMODEMSK) == INTR_RXINTALL)) &&
+            s->rxint == 1) ||
+        /* rx ints enabled, pending */
+        ((s->wregs[W_EXTINT] & EXTINT_BRKINT) &&
+            (s->rregs[R_STATUS] & STATUS_BRK)))) {
+        /* break int e&p */
         return 1;
     }
     return 0;
@@ -269,17 +272,22 @@ static void escc_reset_chn(ESCCChannelState *s)
         s->rregs[i] = 0;
         s->wregs[i] = 0;
     }
-    s->wregs[W_TXCTRL1] = TXCTRL1_1STOP; // 1X divisor, 1 stop bit, no parity
+    /* 1X divisor, 1 stop bit, no parity */
+    s->wregs[W_TXCTRL1] = TXCTRL1_1STOP;
     s->wregs[W_MINTR] = MINTR_RST_ALL;
-    s->wregs[W_CLOCK] = CLOCK_TRXC; // Synch mode tx clock = TRxC
-    s->wregs[W_MISC2] = MISC2_PLLDIS; // PLL disabled
+    /* Synch mode tx clock = TRxC */
+    s->wregs[W_CLOCK] = CLOCK_TRXC;
+    /* PLL disabled */
+    s->wregs[W_MISC2] = MISC2_PLLDIS;
+    /* Enable most interrupts */
     s->wregs[W_EXTINT] = EXTINT_DCD | EXTINT_SYNCINT | EXTINT_CTSINT |
-        EXTINT_TXUNDRN | EXTINT_BRKINT; // Enable most interrupts
-    if (s->disabled)
+        EXTINT_TXUNDRN | EXTINT_BRKINT;
+    if (s->disabled) {
         s->rregs[R_STATUS] = STATUS_TXEMPTY | STATUS_DCD | STATUS_SYNC |
             STATUS_CTS | STATUS_TXUNDRN;
-    else
+    } else {
         s->rregs[R_STATUS] = STATUS_TXEMPTY | STATUS_TXUNDRN;
+    }
     s->rregs[R_SPEC] = SPEC_BITS8 | SPEC_ALLSENT;
 
     s->rx = s->tx = 0;
@@ -300,21 +308,25 @@ static void escc_reset(DeviceState *d)
 static inline void set_rxint(ESCCChannelState *s)
 {
     s->rxint = 1;
-    /* XXX: missing daisy chainnig: escc_chn_b rx should have a lower priority
-       than chn_a rx/tx/special_condition service*/
+    /*
+     * XXX: missing daisy chainnig: escc_chn_b rx should have a lower priority
+     * than chn_a rx/tx/special_condition service
+     */
     s->rxint_under_svc = 1;
     if (s->chn == escc_chn_a) {
         s->rregs[R_INTR] |= INTR_RXINTA;
-        if (s->wregs[W_MINTR] & MINTR_STATUSHI)
+        if (s->wregs[W_MINTR] & MINTR_STATUSHI) {
             s->otherchn->rregs[R_IVEC] = IVEC_HIRXINTA;
-        else
+        } else {
             s->otherchn->rregs[R_IVEC] = IVEC_LORXINTA;
+        }
     } else {
         s->otherchn->rregs[R_INTR] |= INTR_RXINTB;
-        if (s->wregs[W_MINTR] & MINTR_STATUSHI)
+        if (s->wregs[W_MINTR] & MINTR_STATUSHI) {
             s->rregs[R_IVEC] = IVEC_HIRXINTB;
-        else
+        } else {
             s->rregs[R_IVEC] = IVEC_LORXINTB;
+        }
     }
     escc_update_irq(s);
 }
@@ -328,17 +340,18 @@ static inline void set_txint(ESCCChannelState *s)
             if (s->wregs[W_INTR] & INTR_TXINT) {
                 s->rregs[R_INTR] |= INTR_TXINTA;
             }
-            if (s->wregs[W_MINTR] & MINTR_STATUSHI)
+            if (s->wregs[W_MINTR] & MINTR_STATUSHI) {
                 s->otherchn->rregs[R_IVEC] = IVEC_HITXINTA;
-            else
+            } else {
                 s->otherchn->rregs[R_IVEC] = IVEC_LOTXINTA;
+            }
         } else {
             s->rregs[R_IVEC] = IVEC_TXINTB;
             if (s->wregs[W_INTR] & INTR_TXINT) {
                 s->otherchn->rregs[R_INTR] |= INTR_TXINTB;
             }
         }
-    escc_update_irq(s);
+        escc_update_irq(s);
     }
 }
 
@@ -347,20 +360,23 @@ static inline void clr_rxint(ESCCChannelState *s)
     s->rxint = 0;
     s->rxint_under_svc = 0;
     if (s->chn == escc_chn_a) {
-        if (s->wregs[W_MINTR] & MINTR_STATUSHI)
+        if (s->wregs[W_MINTR] & MINTR_STATUSHI) {
             s->otherchn->rregs[R_IVEC] = IVEC_HINOINT;
-        else
+        } else {
             s->otherchn->rregs[R_IVEC] = IVEC_LONOINT;
+        }
         s->rregs[R_INTR] &= ~INTR_RXINTA;
     } else {
-        if (s->wregs[W_MINTR] & MINTR_STATUSHI)
+        if (s->wregs[W_MINTR] & MINTR_STATUSHI) {
             s->rregs[R_IVEC] = IVEC_HINOINT;
-        else
+        } else {
             s->rregs[R_IVEC] = IVEC_LONOINT;
+        }
         s->otherchn->rregs[R_INTR] &= ~INTR_RXINTB;
     }
-    if (s->txint)
+    if (s->txint) {
         set_txint(s);
+    }
     escc_update_irq(s);
 }
 
@@ -369,21 +385,24 @@ static inline void clr_txint(ESCCChannelState *s)
     s->txint = 0;
     s->txint_under_svc = 0;
     if (s->chn == escc_chn_a) {
-        if (s->wregs[W_MINTR] & MINTR_STATUSHI)
+        if (s->wregs[W_MINTR] & MINTR_STATUSHI) {
             s->otherchn->rregs[R_IVEC] = IVEC_HINOINT;
-        else
+        } else {
             s->otherchn->rregs[R_IVEC] = IVEC_LONOINT;
+        }
         s->rregs[R_INTR] &= ~INTR_TXINTA;
     } else {
         s->otherchn->rregs[R_INTR] &= ~INTR_TXINTB;
-        if (s->wregs[W_MINTR] & MINTR_STATUSHI)
+        if (s->wregs[W_MINTR] & MINTR_STATUSHI) {
             s->rregs[R_IVEC] = IVEC_HINOINT;
-        else
+        } else {
             s->rregs[R_IVEC] = IVEC_LONOINT;
+        }
         s->otherchn->rregs[R_INTR] &= ~INTR_TXINTB;
     }
-    if (s->rxint)
+    if (s->rxint) {
         set_rxint(s);
+    }
     escc_update_irq(s);
 }
 
@@ -392,21 +411,24 @@ static void escc_update_parameters(ESCCChannelState *s)
     int speed, parity, data_bits, stop_bits;
     QEMUSerialSetParams ssp;
 
-    if (!qemu_chr_fe_backend_connected(&s->chr) || s->type != escc_serial)
+    if (!qemu_chr_fe_backend_connected(&s->chr) || s->type != escc_serial) {
         return;
+    }
 
     if (s->wregs[W_TXCTRL1] & TXCTRL1_PAREN) {
-        if (s->wregs[W_TXCTRL1] & TXCTRL1_PAREV)
+        if (s->wregs[W_TXCTRL1] & TXCTRL1_PAREV) {
             parity = 'E';
-        else
+        } else {
             parity = 'O';
+        }
     } else {
         parity = 'N';
     }
-    if ((s->wregs[W_TXCTRL1] & TXCTRL1_STPMSK) == TXCTRL1_2STOP)
+    if ((s->wregs[W_TXCTRL1] & TXCTRL1_STPMSK) == TXCTRL1_2STOP) {
         stop_bits = 2;
-    else
+    } else {
         stop_bits = 1;
+    }
     switch (s->wregs[W_TXCTRL2] & TXCTRL2_BITMSK) {
     case TXCTRL2_5BITS:
         data_bits = 5;
@@ -523,10 +545,11 @@ static void escc_mem_write(void *opaque, hwaddr addr,
         default:
             break;
         }
-        if (s->reg == 0)
+        if (s->reg == 0) {
             s->reg = newreg;
-        else
+        } else {
             s->reg = 0;
+        }
         break;
     case SERIAL_DATA:
         trace_escc_mem_writeb_data(CHN_C(s), val);
@@ -538,17 +561,19 @@ static void escc_mem_write(void *opaque, hwaddr addr,
         s->txint = 0;
         escc_update_irq(s);
         s->tx = val;
-        if (s->wregs[W_TXCTRL2] & TXCTRL2_TXEN) { // tx enabled
+        if (s->wregs[W_TXCTRL2] & TXCTRL2_TXEN) { /* tx enabled */
             if (qemu_chr_fe_backend_connected(&s->chr)) {
-                /* XXX this blocks entire thread. Rewrite to use
-                 * qemu_chr_fe_write and background I/O callbacks */
+                /*
+                 * XXX this blocks entire thread. Rewrite to use
+                 * qemu_chr_fe_write and background I/O callbacks
+                 */
                 qemu_chr_fe_write_all(&s->chr, &s->tx, 1);
             } else if (s->type == escc_kbd && !s->disabled) {
                 handle_kbd_command(s, val);
             }
         }
-        s->rregs[R_STATUS] |= STATUS_TXEMPTY; // Tx buffer empty
-        s->rregs[R_SPEC] |= SPEC_ALLSENT; // All sent
+        s->rregs[R_STATUS] |= STATUS_TXEMPTY; /* Tx buffer empty */
+        s->rregs[R_SPEC] |= SPEC_ALLSENT; /* All sent */
         set_txint(s);
         break;
     default:
@@ -606,12 +631,13 @@ static int serial_can_receive(void *opaque)
     ESCCChannelState *s = opaque;
     int ret;
 
-    if (((s->wregs[W_RXCTRL] & RXCTRL_RXEN) == 0) // Rx not enabled
-        || ((s->rregs[R_STATUS] & STATUS_RXAV) == STATUS_RXAV))
-        // char already available
+    if (((s->wregs[W_RXCTRL] & RXCTRL_RXEN) == 0) /* Rx not enabled */
+        || ((s->rregs[R_STATUS] & STATUS_RXAV) == STATUS_RXAV)) {
+        /* char already available */
         ret = 0;
-    else
+    } else {
         ret = 1;
+    }
     return ret;
 }
 
@@ -638,12 +664,13 @@ static void serial_receive1(void *opaque, const uint8_t *buf, int size)
 static void serial_event(void *opaque, QEMUChrEvent event)
 {
     ESCCChannelState *s = opaque;
-    if (event == CHR_EVENT_BREAK)
+    if (event == CHR_EVENT_BREAK) {
         serial_receive_break(s);
+    }
 }
 
 static const VMStateDescription vmstate_escc_chn = {
-    .name ="escc_chn",
+    .name = "escc_chn",
     .version_id = 2,
     .minimum_version_id = 1,
     .fields = (VMStateField[]) {
@@ -662,7 +689,7 @@ static const VMStateDescription vmstate_escc_chn = {
 };
 
 static const VMStateDescription vmstate_escc = {
-    .name ="escc",
+    .name = "escc",
     .version_id = 2,
     .minimum_version_id = 1,
     .fields = (VMStateField[]) {
@@ -734,21 +761,21 @@ static QemuInputHandler sunkbd_handler = {
 static void handle_kbd_command(ESCCChannelState *s, int val)
 {
     trace_escc_kbd_command(val);
-    if (s->led_mode) { // Ignore led byte
+    if (s->led_mode) { /* Ignore led byte */
         s->led_mode = 0;
         return;
     }
     switch (val) {
-    case 1: // Reset, return type code
+    case 1: /* Reset, return type code */
         clear_queue(s);
         put_queue(s, 0xff);
-        put_queue(s, 4); // Type 4
+        put_queue(s, 4); /* Type 4 */
         put_queue(s, 0x7f);
         break;
-    case 0xe: // Set leds
+    case 0xe: /* Set leds */
         s->led_mode = 1;
         break;
-    case 7: // Query layout
+    case 7: /* Query layout */
     case 0xf:
         clear_queue(s);
         put_queue(s, 0xfe);
@@ -768,34 +795,39 @@ static void sunmouse_event(void *opaque,
     trace_escc_sunmouse_event(dx, dy, buttons_state);
     ch = 0x80 | 0x7; /* protocol start byte, no buttons pressed */
 
-    if (buttons_state & MOUSE_EVENT_LBUTTON)
+    if (buttons_state & MOUSE_EVENT_LBUTTON) {
         ch ^= 0x4;
-    if (buttons_state & MOUSE_EVENT_MBUTTON)
+    }
+    if (buttons_state & MOUSE_EVENT_MBUTTON) {
         ch ^= 0x2;
-    if (buttons_state & MOUSE_EVENT_RBUTTON)
+    }
+    if (buttons_state & MOUSE_EVENT_RBUTTON) {
         ch ^= 0x1;
+    }
 
     put_queue(s, ch);
 
     ch = dx;
 
-    if (ch > 127)
+    if (ch > 127) {
         ch = 127;
-    else if (ch < -127)
+    } else if (ch < -127) {
         ch = -127;
+    }
 
     put_queue(s, ch & 0xff);
 
     ch = -dy;
 
-    if (ch > 127)
+    if (ch > 127) {
         ch = 127;
-    else if (ch < -127)
+    } else if (ch < -127) {
         ch = -127;
+    }
 
     put_queue(s, ch & 0xff);
 
-    // MSC protocol specify two extra motion bytes
+    /* MSC protocol specify two extra motion bytes */
 
     put_queue(s, 0);
     put_queue(s, 0);
-- 
2.20.1

[PATCH 2/3] escc: fix R_STATUS channel reset value

[Mark Cave-Ayland]

According to the "Z80X30 Register Reset Values" table in the ESCC datasheet
bits 2 and 6 are set whilst bits 0 and 1 are cleared during channel reset.
All other bits should be left unaltered.

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

diff --git a/hw/char/escc.c b/hw/char/escc.c
index 63e0f15dfa..0f6957ba8b 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -281,12 +281,11 @@ static void escc_reset_chn(ESCCChannelState *s)
     s->wregs[W_MISC2] = MISC2_PLLDIS;
     /* Enable most interrupts */
     s->wregs[W_EXTINT] = EXTINT_DCD | EXTINT_SYNCINT | EXTINT_CTSINT |
-        EXTINT_TXUNDRN | EXTINT_BRKINT;
+                         EXTINT_TXUNDRN | EXTINT_BRKINT;
+    s->rregs[R_STATUS] &= ~(STATUS_RXAV | STATUS_ZERO);
+    s->rregs[R_STATUS] |= STATUS_TXEMPTY | STATUS_TXUNDRN;
     if (s->disabled) {
-        s->rregs[R_STATUS] = STATUS_TXEMPTY | STATUS_DCD | STATUS_SYNC |
-            STATUS_CTS | STATUS_TXUNDRN;
-    } else {
-        s->rregs[R_STATUS] = STATUS_TXEMPTY | STATUS_TXUNDRN;
+        s->rregs[R_STATUS] |= STATUS_DCD | STATUS_CTS;
     }
     s->rregs[R_SPEC] = SPEC_BITS8 | SPEC_ALLSENT;
 
-- 
2.20.1

[PATCH 3/3] escc: fix STATUS_SYNC bit in R_STATUS register

[Mark Cave-Ayland]

After an SDLC "Enter hunt" command has been sent the STATUS_SYNC bit should remain
high until the flag byte has been detected. Whilst the ESCC device doesn't yet
implement SDLC mode, without this change the active low STATUS_SYNC is constantly
asserted causing the MacOS OpenTransport extension to hang on startup as it thinks
it is constantly receiving LocalTalk responses during its initial negotiation
phase.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
 hw/char/escc.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/hw/char/escc.c b/hw/char/escc.c
index 0f6957ba8b..eba6b45baa 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -92,6 +92,7 @@
 #define W_IVEC    2
 #define W_RXCTRL  3
 #define RXCTRL_RXEN    0x01
+#define RXCTRL_HUNT    0x10
 #define W_TXCTRL1 4
 #define TXCTRL1_PAREN  0x01
 #define TXCTRL1_PAREV  0x02
@@ -508,7 +509,13 @@ static void escc_mem_write(void *opaque, hwaddr addr,
                 break;
             }
             break;
-        case W_INTR ... W_RXCTRL:
+        case W_RXCTRL:
+            s->wregs[s->reg] = val;
+            if (val & RXCTRL_HUNT) {
+                s->rregs[R_STATUS] |= STATUS_SYNC;
+            }
+            break;
+        case W_INTR ... W_IVEC:
         case W_SYNC1 ... W_TXBUF:
         case W_MISC1 ... W_CLOCK:
         case W_MISC2 ... W_EXTINT:
-- 
2.20.1

Re: [PATCH 1/3] escc: checkpatch fixes

[Peter Maydell]

On Sun, 29 Aug 2021 at 11:04, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/char/escc.c | 160 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 96 insertions(+), 64 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

You might want to fix this existing typo:

> @@ -300,21 +308,25 @@ static void escc_reset(DeviceState *d)
>  static inline void set_rxint(ESCCChannelState *s)
>  {
>      s->rxint = 1;
> -    /* XXX: missing daisy chainnig: escc_chn_b rx should have a lower priority
> -       than chn_a rx/tx/special_condition service*/
> +    /*
> +     * XXX: missing daisy chainnig: escc_chn_b rx should have a lower priority
> +     * than chn_a rx/tx/special_condition service
> +     */

"daisy chaining"


> -    // MSC protocol specify two extra motion bytes
> +    /* MSC protocol specify two extra motion bytes */

and this should be "specifies".

-- PMM

Re: [PATCH 3/3] escc: fix STATUS_SYNC bit in R_STATUS register

[Peter Maydell]

On Sun, 29 Aug 2021 at 11:04, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> After an SDLC "Enter hunt" command has been sent the STATUS_SYNC bit should remain
> high until the flag byte has been detected. Whilst the ESCC device doesn't yet
> implement SDLC mode, without this change the active low STATUS_SYNC is constantly
> asserted causing the MacOS OpenTransport extension to hang on startup as it thinks
> it is constantly receiving LocalTalk responses during its initial negotiation
> phase.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/char/escc.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/hw/char/escc.c b/hw/char/escc.c
> index 0f6957ba8b..eba6b45baa 100644
> --- a/hw/char/escc.c
> +++ b/hw/char/escc.c
> @@ -92,6 +92,7 @@
>  #define W_IVEC    2
>  #define W_RXCTRL  3
>  #define RXCTRL_RXEN    0x01
> +#define RXCTRL_HUNT    0x10
>  #define W_TXCTRL1 4
>  #define TXCTRL1_PAREN  0x01
>  #define TXCTRL1_PAREV  0x02
> @@ -508,7 +509,13 @@ static void escc_mem_write(void *opaque, hwaddr addr,
>                  break;
>              }
>              break;
> -        case W_INTR ... W_RXCTRL:
> +        case W_RXCTRL:
> +            s->wregs[s->reg] = val;
> +            if (val & RXCTRL_HUNT) {
> +                s->rregs[R_STATUS] |= STATUS_SYNC;
> +            }
> +            break;
> +        case W_INTR ... W_IVEC:
>          case W_SYNC1 ... W_TXBUF:
>          case W_MISC1 ... W_CLOCK:
>          case W_MISC2 ... W_EXTINT:

If I read the manual correctly I think strictly speaking if this
is a 0->1 transition for the SYNC bit it is supposed to generate
an interrupt. But I guess since we don't implement any of this
stuff we can ignore that...

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [PATCH 2/3] escc: fix R_STATUS channel reset value

[Peter Maydell]

On Sun, 29 Aug 2021 at 11:07, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> According to the "Z80X30 Register Reset Values" table in the ESCC datasheet
> bits 2 and 6 are set whilst bits 0 and 1 are cleared during channel reset.
> All other bits should be left unaltered.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/char/escc.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/hw/char/escc.c b/hw/char/escc.c
> index 63e0f15dfa..0f6957ba8b 100644
> --- a/hw/char/escc.c
> +++ b/hw/char/escc.c
> @@ -281,12 +281,11 @@ static void escc_reset_chn(ESCCChannelState *s)
>      s->wregs[W_MISC2] = MISC2_PLLDIS;
>      /* Enable most interrupts */
>      s->wregs[W_EXTINT] = EXTINT_DCD | EXTINT_SYNCINT | EXTINT_CTSINT |
> -        EXTINT_TXUNDRN | EXTINT_BRKINT;
> +                         EXTINT_TXUNDRN | EXTINT_BRKINT;

This indentation fix should probably have been in the
coding-style-fixes patch.

> +    s->rregs[R_STATUS] &= ~(STATUS_RXAV | STATUS_ZERO);
> +    s->rregs[R_STATUS] |= STATUS_TXEMPTY | STATUS_TXUNDRN;
>      if (s->disabled) {
> -        s->rregs[R_STATUS] = STATUS_TXEMPTY | STATUS_DCD | STATUS_SYNC |
> -            STATUS_CTS | STATUS_TXUNDRN;
> -    } else {
> -        s->rregs[R_STATUS] = STATUS_TXEMPTY | STATUS_TXUNDRN;
> +        s->rregs[R_STATUS] |= STATUS_DCD | STATUS_CTS;
>      }
>      s->rregs[R_SPEC] = SPEC_BITS8 | SPEC_ALLSENT;

We seem to use this function for both 'channel reset' and
'hardware reset' -- escc_reset just calls escc_reset_chn for
each channel, and is used as the DeviceState::reset as well
as for the 'hardware reset' you get by writing to WR9 with
both D6 and D7 set to 1.

Because we want this software-triggered 'hardware reset' to not
reset all registers to fixed values, I think we need to disentangle
the power-on reset DeviceState::reset so that power-on reset always
brings the device back to exactly the state that it has when QEMU
first starts.

thanks
-- PMM

Re: [PATCH 0/3] escc: fix R_STATUS when SDLC mode is enabled

[Peter Maydell]

On Sun, 29 Aug 2021 at 11:05, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> Here is another small set of ESCC fixes from my attempts to boot MacOS on the q800
> machine.
>
> When MacOS loads the OpenTransport extension on boot it attempts to enable
> SDLC mode on the ESCC. QEMU's emulation doesn't support SDLC mode, but without
> these fixes the active low STATUS_SYNC bit in R_STATUS is continually
> asserted causing the extension to hang on startup as it believe it is constantly
> receiving LocalTalk responses during its initial negotiation phase.

The ESCC data sheet is the first one I've ever encountered that includes
a multiple-choice test on features of the device ("ESCC Questions and Answers"
on pages 385/386: http://www.zilog.com/docs/serial/scc_escc_um.pdf ) :-)

-- PMM

Re: [PATCH 2/3] escc: fix R_STATUS channel reset value

[Mark Cave-Ayland]

On 29/08/2021 14:17, Peter Maydell wrote:

> On Sun, 29 Aug 2021 at 11:07, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> According to the "Z80X30 Register Reset Values" table in the ESCC datasheet
>> bits 2 and 6 are set whilst bits 0 and 1 are cleared during channel reset.
>> All other bits should be left unaltered.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>> ---
>>   hw/char/escc.c | 9 ++++-----
>>   1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/hw/char/escc.c b/hw/char/escc.c
>> index 63e0f15dfa..0f6957ba8b 100644
>> --- a/hw/char/escc.c
>> +++ b/hw/char/escc.c
>> @@ -281,12 +281,11 @@ static void escc_reset_chn(ESCCChannelState *s)
>>       s->wregs[W_MISC2] = MISC2_PLLDIS;
>>       /* Enable most interrupts */
>>       s->wregs[W_EXTINT] = EXTINT_DCD | EXTINT_SYNCINT | EXTINT_CTSINT |
>> -        EXTINT_TXUNDRN | EXTINT_BRKINT;
>> +                         EXTINT_TXUNDRN | EXTINT_BRKINT;
> 
> This indentation fix should probably have been in the
> coding-style-fixes patch.

Okay I can do that.

>> +    s->rregs[R_STATUS] &= ~(STATUS_RXAV | STATUS_ZERO);
>> +    s->rregs[R_STATUS] |= STATUS_TXEMPTY | STATUS_TXUNDRN;
>>       if (s->disabled) {
>> -        s->rregs[R_STATUS] = STATUS_TXEMPTY | STATUS_DCD | STATUS_SYNC |
>> -            STATUS_CTS | STATUS_TXUNDRN;
>> -    } else {
>> -        s->rregs[R_STATUS] = STATUS_TXEMPTY | STATUS_TXUNDRN;
>> +        s->rregs[R_STATUS] |= STATUS_DCD | STATUS_CTS;
>>       }
>>       s->rregs[R_SPEC] = SPEC_BITS8 | SPEC_ALLSENT;
> 
> We seem to use this function for both 'channel reset' and
> 'hardware reset' -- escc_reset just calls escc_reset_chn for
> each channel, and is used as the DeviceState::reset as well
> as for the 'hardware reset' you get by writing to WR9 with
> both D6 and D7 set to 1.
> 
> Because we want this software-triggered 'hardware reset' to not
> reset all registers to fixed values, I think we need to disentangle
> the power-on reset DeviceState::reset so that power-on reset always
> brings the device back to exactly the state that it has when QEMU
> first starts.

I'm currently struggling to understand what the initial power-on values are for the 
ESCC registers. According to the reset table both 'hardware reset' and 'software 
reset' don't change the values of all bits which implies the power-on state is 
different, but I can't seem to find a description of it anywhere? Could we assume 
that all ESCC registers are initialised to zero, and the guest will perform a 
suitable reset during driver initialisation?


ATB,

Mark.

Re: [PATCH 0/3] escc: fix R_STATUS when SDLC mode is enabled

[Mark Cave-Ayland]

On 29/08/2021 14:19, Peter Maydell wrote:

> On Sun, 29 Aug 2021 at 11:05, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> Here is another small set of ESCC fixes from my attempts to boot MacOS on the q800
>> machine.
>>
>> When MacOS loads the OpenTransport extension on boot it attempts to enable
>> SDLC mode on the ESCC. QEMU's emulation doesn't support SDLC mode, but without
>> these fixes the active low STATUS_SYNC bit in R_STATUS is continually
>> asserted causing the extension to hang on startup as it believe it is constantly
>> receiving LocalTalk responses during its initial negotiation phase.
> 
> The ESCC data sheet is the first one I've ever encountered that includes
> a multiple-choice test on features of the device ("ESCC Questions and Answers"
> on pages 385/386: http://www.zilog.com/docs/serial/scc_escc_um.pdf ) :-)

I wonder why the idea never took hold? :D


ATB,

Mark.