[PATCH v2 0/9] escc: fix reset and R_STATUS when SDLC mode is enabled

[PATCH v2 0/9] escc: fix reset and R_STATUS when SDLC mode is enabled

[Mark Cave-Ayland]

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

Patch 1 fixes up the formatting so that the remainder of the patchset keeps
checkpatch happy.

Patches 2-8 rework the reset handling so that the QEMU device reset is separate
from the ESCC in-built hardware and software reset as defined in the datasheet.
The aim here is two-fold: allow QEMU's device reset to place the ESCC device in
a known state (although we assume all registers are zeroed whilst their values are
undefined according to the datasheet) and ensure that the reset commands sent by
the MacOS OpenTransport extension on boot don't re-assert the active low
STATUS_SYNC bit in R_STATUS.

Finally patch 9 is the real fix: when entering SDLC mode using an "Enter hunt"
command the STATUS_SYNC bit in R_STATUS must remain high until the flag byte
is detected. Without this fix the active low STATUS_SYNC is continually asserted
causing the MacOS OpenTransport extension to hang on startup as it believes it is
constantly receiving LocalTalk traffic during its initial negotiation phase.

NOTE: this patchset currently fails CI because it exposed a bug that OpenBIOS
doesn't send ESCC channel reset commands before attempting to configure and use
the serial port. Those patches have just been posted to the OpenBIOS mailing list
here: https://mail.coreboot.org/hyperkitty/list/openbios@openbios.org/thread/PQCW5RDIDIEUYBVAHNIY3OMHCEVYYWPU/.

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


v2:
- Rebase onto master
- Rewrite cover letter to cover new reset changes
- Change reset to separate out QEMU device reset, soft reset and hard reset
  (ensuring register values are updated as specified in the datasheet)
- Add R-B tags from Peter


Mark Cave-Ayland (9):
  escc: checkpatch fixes
  escc: reset register values to zero in escc_reset()
  escc: introduce escc_soft_reset_chn() for software reset
  escc: introduce escc_hard_reset_chn() for hardware reset
  escc: implement soft reset as described in the datasheet
  escc: implement hard reset as described in the datasheet
  escc: remove register changes from escc_reset_chn()
  escc: re-use escc_reset_chn() for hard and soft reset
  escc: fix STATUS_SYNC bit in R_STATUS register

 hw/char/escc.c | 281 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 197 insertions(+), 84 deletions(-)

-- 
2.20.1

[PATCH v2 1/9] escc: checkpatch fixes

[Mark Cave-Ayland]

Also fix a couple of spelling mistakes in comments.

Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 hw/char/escc.c | 162 +++++++++++++++++++++++++++++--------------------
 1 file changed, 97 insertions(+), 65 deletions(-)

diff --git a/hw/char/escc.c b/hw/char/escc.c
index 52e7978287..c87ecd59d8 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
+                             STATUS_CTS | STATUS_TXUNDRN;
+    } 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 chaining: 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 specifies two extra motion bytes */
 
     put_queue(s, 0);
     put_queue(s, 0);
-- 
2.20.1

[PATCH v2 2/9] escc: reset register values to zero in escc_reset()

[Mark Cave-Ayland]

This is to ensure that a device reset always returns the ESCC to a known state.

Note that this is currently redundant with the same code in escc_reset_chn()
but that will change shortly.

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

diff --git a/hw/char/escc.c b/hw/char/escc.c
index c87ecd59d8..b0d3b92dc1 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -300,9 +300,24 @@ static void escc_reset_chn(ESCCChannelState *s)
 static void escc_reset(DeviceState *d)
 {
     ESCCState *s = ESCC(d);
+    int i, j;
 
-    escc_reset_chn(&s->chn[0]);
-    escc_reset_chn(&s->chn[1]);
+    for (i = 0; i < 2; i++) {
+        ESCCChannelState *cs = &s->chn[i];
+
+        /*
+         * According to the ESCC datasheet "Miscellaneous Questions" section
+         * on page 384, the values of the ESCC registers are not guaranteed on
+         * power-on until an explicit hardware or software reset has been
+         * issued. For now we zero the registers so that a device reset always
+         * returns the emulated device to a fixed state.
+         */
+        for (j = 0; j < ESCC_SERIAL_REGS; j++) {
+            cs->rregs[j] = 0;
+            cs->wregs[j] = 0;
+        }
+        escc_reset_chn(cs);
+    }
 }
 
 static inline void set_rxint(ESCCChannelState *s)
-- 
2.20.1

[PATCH v2 3/9] escc: introduce escc_soft_reset_chn() for software reset

[Mark Cave-Ayland]

This new software reset function is to be called when the appropriate channel
software reset bit is written to register WR9. Its initial implementation is
the same as the existing escc_reset_chn() function used for device reset.

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

diff --git a/hw/char/escc.c b/hw/char/escc.c
index b0d3b92dc1..935ec1aef6 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -297,6 +297,40 @@ static void escc_reset_chn(ESCCChannelState *s)
     clear_queue(s);
 }
 
+static void escc_soft_reset_chn(ESCCChannelState *s)
+{
+    int i;
+
+    s->reg = 0;
+    for (i = 0; i < ESCC_SERIAL_REGS; i++) {
+        s->rregs[i] = 0;
+        s->wregs[i] = 0;
+    }
+    /* 1X divisor, 1 stop bit, no parity */
+    s->wregs[W_TXCTRL1] = TXCTRL1_1STOP;
+    s->wregs[W_MINTR] = MINTR_RST_ALL;
+    /* 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;
+    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_SPEC] = SPEC_BITS8 | SPEC_ALLSENT;
+
+    s->rx = s->tx = 0;
+    s->rxint = s->txint = 0;
+    s->rxint_under_svc = s->txint_under_svc = 0;
+    s->e0_mode = s->led_mode = s->caps_lock_mode = s->num_lock_mode = 0;
+    clear_queue(s);
+}
+
 static void escc_reset(DeviceState *d)
 {
     ESCCState *s = ESCC(d);
@@ -547,10 +581,10 @@ static void escc_mem_write(void *opaque, hwaddr addr,
             default:
                 break;
             case MINTR_RST_B:
-                escc_reset_chn(&serial->chn[0]);
+                escc_soft_reset_chn(&serial->chn[0]);
                 return;
             case MINTR_RST_A:
-                escc_reset_chn(&serial->chn[1]);
+                escc_soft_reset_chn(&serial->chn[1]);
                 return;
             case MINTR_RST_ALL:
                 escc_reset(DEVICE(serial));
-- 
2.20.1

[PATCH v2 4/9] escc: introduce escc_hard_reset_chn() for hardware reset

[Mark Cave-Ayland]

This new hardware reset function is to be called for both channels when the
hardware reset bit is written to register WR9. Its initial implementation is
the same as the existing escc_reset_chn() function used for device reset.

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

diff --git a/hw/char/escc.c b/hw/char/escc.c
index 935ec1aef6..691086d97d 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -331,6 +331,40 @@ static void escc_soft_reset_chn(ESCCChannelState *s)
     clear_queue(s);
 }
 
+static void escc_hard_reset_chn(ESCCChannelState *s)
+{
+    int i;
+
+    s->reg = 0;
+    for (i = 0; i < ESCC_SERIAL_REGS; i++) {
+        s->rregs[i] = 0;
+        s->wregs[i] = 0;
+    }
+    /* 1X divisor, 1 stop bit, no parity */
+    s->wregs[W_TXCTRL1] = TXCTRL1_1STOP;
+    s->wregs[W_MINTR] = MINTR_RST_ALL;
+    /* 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;
+    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_SPEC] = SPEC_BITS8 | SPEC_ALLSENT;
+
+    s->rx = s->tx = 0;
+    s->rxint = s->txint = 0;
+    s->rxint_under_svc = s->txint_under_svc = 0;
+    s->e0_mode = s->led_mode = s->caps_lock_mode = s->num_lock_mode = 0;
+    clear_queue(s);
+}
+
 static void escc_reset(DeviceState *d)
 {
     ESCCState *s = ESCC(d);
@@ -587,7 +621,8 @@ static void escc_mem_write(void *opaque, hwaddr addr,
                 escc_soft_reset_chn(&serial->chn[1]);
                 return;
             case MINTR_RST_ALL:
-                escc_reset(DEVICE(serial));
+                escc_hard_reset_chn(&serial->chn[0]);
+                escc_hard_reset_chn(&serial->chn[1]);
                 return;
             }
             break;
-- 
2.20.1

[PATCH v2 5/9] escc: implement soft reset as described in the datasheet

[Mark Cave-Ayland]

The software reset differs from a device reset in that it only changes the contents
of specific registers. Remove the code that resets all the registers to zero during
soft reset and implement the default values listed in the table in the "Z85C30 Reset"
section.

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

diff --git a/hw/char/escc.c b/hw/char/escc.c
index 691086d97d..9a4e5ae20d 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -89,6 +89,8 @@
 #define INTR_RXMODEMSK 0x18
 #define INTR_RXINT1ST  0x08
 #define INTR_RXINTALL  0x10
+#define INTR_WTDMA_EN  0x40
+#define INTR_WTDMA_RQ  0x80
 #define W_IVEC    2
 #define W_RXCTRL  3
 #define RXCTRL_RXEN    0x01
@@ -105,27 +107,38 @@
 #define TXCTRL1_CLK64X 0xc0
 #define TXCTRL1_CLKMSK 0xc0
 #define W_TXCTRL2 5
+#define TXCTRL2_RTS    0x02
+#define TXCTRL2_CRCPLY 0x04
 #define TXCTRL2_TXEN   0x08
 #define TXCTRL2_BITMSK 0x60
 #define TXCTRL2_5BITS  0x00
+#define TXCTRL2_SNDBRK 0x10
 #define TXCTRL2_7BITS  0x20
 #define TXCTRL2_6BITS  0x40
 #define TXCTRL2_8BITS  0x60
+#define TXCTRL2_DTR    0x80
 #define W_SYNC1   6
 #define W_SYNC2   7
 #define W_TXBUF   8
 #define W_MINTR   9
 #define MINTR_STATUSHI 0x10
+#define MINTR_SOFTIACK 0x20
 #define MINTR_RST_MASK 0xc0
 #define MINTR_RST_B    0x40
 #define MINTR_RST_A    0x80
 #define MINTR_RST_ALL  0xc0
 #define W_MISC1  10
+#define MISC1_ENC_MASK 0x60
 #define W_CLOCK  11
 #define CLOCK_TRXC     0x08
 #define W_BRGLO  12
 #define W_BRGHI  13
 #define W_MISC2  14
+#define MISC2_DTRREQ   0x04
+#define MISC2_AUTOECHO 0x08
+#define MISC2_LCL_LOOP 0x10
+#define MISC2_PLLCMD0  0x20
+#define MISC2_PLLCMD1  0x40
 #define MISC2_PLLDIS   0x30
 #define W_EXTINT 15
 #define EXTINT_DCD     0x08
@@ -170,6 +183,7 @@
 #define R_RXBUF   8
 #define R_RXCTRL  9
 #define R_MISC   10
+#define MISC_2CLKMISS  0x40
 #define R_MISC1  11
 #define R_BRGLO  12
 #define R_BRGHI  13
@@ -299,30 +313,33 @@ static void escc_reset_chn(ESCCChannelState *s)
 
 static void escc_soft_reset_chn(ESCCChannelState *s)
 {
-    int i;
-
     s->reg = 0;
-    for (i = 0; i < ESCC_SERIAL_REGS; i++) {
-        s->rregs[i] = 0;
-        s->wregs[i] = 0;
-    }
+    s->wregs[W_CMD] = 0;
+    s->wregs[W_INTR] &= ~(INTR_INTALL | INTR_TXINT | INTR_RXINT1ST |
+                          INTR_RXINTALL | INTR_WTDMA_EN | INTR_WTDMA_RQ);
+    s->wregs[W_RXCTRL] &= ~RXCTRL_RXEN;
     /* 1X divisor, 1 stop bit, no parity */
     s->wregs[W_TXCTRL1] = TXCTRL1_1STOP;
-    s->wregs[W_MINTR] = MINTR_RST_ALL;
-    /* Synch mode tx clock = TRxC */
-    s->wregs[W_CLOCK] = CLOCK_TRXC;
+    s->wregs[W_TXCTRL2] &= ~(TXCTRL2_RTS | TXCTRL2_CRCPLY | TXCTRL2_TXEN |
+                             TXCTRL2_SNDBRK | TXCTRL2_DTR);
+    s->wregs[W_MINTR] &= ~MINTR_SOFTIACK;
+    s->wregs[W_MISC1] &= ~MISC1_ENC_MASK;
     /* PLL disabled */
-    s->wregs[W_MISC2] = MISC2_PLLDIS;
+    s->wregs[W_MISC2] &= ~(MISC2_DTRREQ | MISC2_AUTOECHO | MISC2_PLLCMD0);
+    s->wregs[W_MISC2] |= MISC2_PLLCMD1;
     /* Enable most interrupts */
     s->wregs[W_EXTINT] = EXTINT_DCD | EXTINT_SYNCINT | EXTINT_CTSINT |
                          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_SYNC | STATUS_CTS;
     }
-    s->rregs[R_SPEC] = SPEC_BITS8 | SPEC_ALLSENT;
+    s->rregs[R_SPEC] &= SPEC_ALLSENT;
+    s->rregs[R_SPEC] |= SPEC_BITS8;
+    s->rregs[R_INTR] = 0;
+    s->rregs[R_MISC] &= ~MISC_2CLKMISS;
 
     s->rx = s->tx = 0;
     s->rxint = s->txint = 0;
-- 
2.20.1

[PATCH v2 6/9] escc: implement hard reset as described in the datasheet

[Mark Cave-Ayland]

The hardware reset differs from a device reset in that it only changes the contents
of specific registers. Remove the code that resets all the registers to zero during
hardware reset and implement the default values listed in the table in the "Z85C30
Reset" section.

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

diff --git a/hw/char/escc.c b/hw/char/escc.c
index 9a4e5ae20d..1d90e77db7 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -121,6 +121,8 @@
 #define W_SYNC2   7
 #define W_TXBUF   8
 #define W_MINTR   9
+#define MINTR_VIS      0x01
+#define MINTR_NV       0x02
 #define MINTR_STATUSHI 0x10
 #define MINTR_SOFTIACK 0x20
 #define MINTR_RST_MASK 0xc0
@@ -139,6 +141,7 @@
 #define MISC2_LCL_LOOP 0x10
 #define MISC2_PLLCMD0  0x20
 #define MISC2_PLLCMD1  0x40
+#define MISC2_PLLCMD2  0x80
 #define MISC2_PLLDIS   0x30
 #define W_EXTINT 15
 #define EXTINT_DCD     0x08
@@ -350,30 +353,35 @@ static void escc_soft_reset_chn(ESCCChannelState *s)
 
 static void escc_hard_reset_chn(ESCCChannelState *s)
 {
-    int i;
-
     s->reg = 0;
-    for (i = 0; i < ESCC_SERIAL_REGS; i++) {
-        s->rregs[i] = 0;
-        s->wregs[i] = 0;
-    }
+    s->wregs[W_CMD] = 0;
+    s->wregs[W_INTR] &= ~(INTR_INTALL | INTR_TXINT | INTR_RXINT1ST |
+                          INTR_RXINTALL | INTR_WTDMA_EN | INTR_WTDMA_RQ);
+    s->wregs[W_RXCTRL] &= ~RXCTRL_RXEN;
     /* 1X divisor, 1 stop bit, no parity */
     s->wregs[W_TXCTRL1] = TXCTRL1_1STOP;
-    s->wregs[W_MINTR] = MINTR_RST_ALL;
-    /* Synch mode tx clock = TRxC */
+    s->wregs[W_TXCTRL2] &= ~(TXCTRL2_RTS | TXCTRL2_CRCPLY | TXCTRL2_TXEN |
+                             TXCTRL2_SNDBRK | TXCTRL2_DTR);
+    s->wregs[W_MINTR] &= MINTR_VIS | MINTR_NV;
+    s->wregs[W_MINTR] |= MINTR_RST_B | MINTR_RST_A;
+    s->wregs[W_MISC1] = 0;
     s->wregs[W_CLOCK] = CLOCK_TRXC;
     /* PLL disabled */
-    s->wregs[W_MISC2] = MISC2_PLLDIS;
+    s->wregs[W_MISC2] &= MISC2_PLLCMD1 | MISC2_PLLCMD2;
+    s->wregs[W_MISC2] |= MISC2_LCL_LOOP | MISC2_PLLCMD0;
     /* Enable most interrupts */
     s->wregs[W_EXTINT] = EXTINT_DCD | EXTINT_SYNCINT | EXTINT_CTSINT |
                          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_SYNC | STATUS_CTS;
     }
-    s->rregs[R_SPEC] = SPEC_BITS8 | SPEC_ALLSENT;
+    s->rregs[R_SPEC] &= SPEC_ALLSENT;
+    s->rregs[R_SPEC] |= SPEC_BITS8;
+    s->rregs[R_INTR] = 0;
+    s->rregs[R_MISC] &= ~MISC_2CLKMISS;
 
     s->rx = s->tx = 0;
     s->rxint = s->txint = 0;
-- 
2.20.1

[PATCH v2 7/9] escc: remove register changes from escc_reset_chn()

[Mark Cave-Ayland]

Now that register values at reset are handled elsewhere for all of device reset,
soft reset and hard reset, escc_reset_chn() only needs to handle initialisation
of internal device state.

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

diff --git a/hw/char/escc.c b/hw/char/escc.c
index 1d90e77db7..c6b477ef78 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -142,7 +142,6 @@
 #define MISC2_PLLCMD0  0x20
 #define MISC2_PLLCMD1  0x40
 #define MISC2_PLLCMD2  0x80
-#define MISC2_PLLDIS   0x30
 #define W_EXTINT 15
 #define EXTINT_DCD     0x08
 #define EXTINT_SYNCINT 0x10
@@ -282,31 +281,7 @@ static void escc_update_irq(ESCCChannelState *s)
 
 static void escc_reset_chn(ESCCChannelState *s)
 {
-    int i;
-
     s->reg = 0;
-    for (i = 0; i < ESCC_SERIAL_REGS; i++) {
-        s->rregs[i] = 0;
-        s->wregs[i] = 0;
-    }
-    /* 1X divisor, 1 stop bit, no parity */
-    s->wregs[W_TXCTRL1] = TXCTRL1_1STOP;
-    s->wregs[W_MINTR] = MINTR_RST_ALL;
-    /* 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;
-    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_SPEC] = SPEC_BITS8 | SPEC_ALLSENT;
-
     s->rx = s->tx = 0;
     s->rxint = s->txint = 0;
     s->rxint_under_svc = s->txint_under_svc = 0;
-- 
2.20.1

[PATCH v2 8/9] escc: re-use escc_reset_chn() for hard and soft reset

[Mark Cave-Ayland]

This removes duplication of the internal device state initialisation from all
of device reset, soft reset and hard reset.

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

diff --git a/hw/char/escc.c b/hw/char/escc.c
index c6b477ef78..29fcdcb625 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -291,7 +291,8 @@ static void escc_reset_chn(ESCCChannelState *s)
 
 static void escc_soft_reset_chn(ESCCChannelState *s)
 {
-    s->reg = 0;
+    escc_reset_chn(s);
+
     s->wregs[W_CMD] = 0;
     s->wregs[W_INTR] &= ~(INTR_INTALL | INTR_TXINT | INTR_RXINT1ST |
                           INTR_RXINTALL | INTR_WTDMA_EN | INTR_WTDMA_RQ);
@@ -318,17 +319,12 @@ static void escc_soft_reset_chn(ESCCChannelState *s)
     s->rregs[R_SPEC] |= SPEC_BITS8;
     s->rregs[R_INTR] = 0;
     s->rregs[R_MISC] &= ~MISC_2CLKMISS;
-
-    s->rx = s->tx = 0;
-    s->rxint = s->txint = 0;
-    s->rxint_under_svc = s->txint_under_svc = 0;
-    s->e0_mode = s->led_mode = s->caps_lock_mode = s->num_lock_mode = 0;
-    clear_queue(s);
 }
 
 static void escc_hard_reset_chn(ESCCChannelState *s)
 {
-    s->reg = 0;
+    escc_reset_chn(s);
+
     s->wregs[W_CMD] = 0;
     s->wregs[W_INTR] &= ~(INTR_INTALL | INTR_TXINT | INTR_RXINT1ST |
                           INTR_RXINTALL | INTR_WTDMA_EN | INTR_WTDMA_RQ);
@@ -357,12 +353,6 @@ static void escc_hard_reset_chn(ESCCChannelState *s)
     s->rregs[R_SPEC] |= SPEC_BITS8;
     s->rregs[R_INTR] = 0;
     s->rregs[R_MISC] &= ~MISC_2CLKMISS;
-
-    s->rx = s->tx = 0;
-    s->rxint = s->txint = 0;
-    s->rxint_under_svc = s->txint_under_svc = 0;
-    s->e0_mode = s->led_mode = s->caps_lock_mode = s->num_lock_mode = 0;
-    clear_queue(s);
 }
 
 static void escc_reset(DeviceState *d)
-- 
2.20.1

[PATCH v2 9/9] 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>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
---
 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 29fcdcb625..b566758d62 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -94,6 +94,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
@@ -582,7 +583,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 v2 8/9] escc: re-use escc_reset_chn() for hard and soft reset

[Philippe Mathieu-Daudé]

On 9/2/21 12:22 PM, Mark Cave-Ayland wrote:
> This removes duplication of the internal device state initialisation from all
> of device reset, soft reset and hard reset.
> 
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> ---
>  hw/char/escc.c | 18 ++++--------------
>  1 file changed, 4 insertions(+), 14 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH v2 4/9] escc: introduce escc_hard_reset_chn() for hardware reset

[Peter Maydell]

On Thu, 2 Sept 2021 at 11:33, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> This new hardware reset function is to be called for both channels when the
> hardware reset bit is written to register WR9. Its initial implementation is
> the same as the existing escc_reset_chn() function used for device reset.
>
> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>


The datasheet says the only differences between hard and soft
reset are for registers W9, W10, W11 and W14. I wasn't expecting
the functions to be completely separated out like this.

-- PMM

Re: [PATCH v2 4/9] escc: introduce escc_hard_reset_chn() for hardware reset

[Mark Cave-Ayland]

On 02/09/2021 16:42, Peter Maydell wrote:

> On Thu, 2 Sept 2021 at 11:33, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> This new hardware reset function is to be called for both channels when the
>> hardware reset bit is written to register WR9. Its initial implementation is
>> the same as the existing escc_reset_chn() function used for device reset.
>>
>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> 
> 
> The datasheet says the only differences between hard and soft
> reset are for registers W9, W10, W11 and W14. I wasn't expecting
> the functions to be completely separated out like this.

I did consider doing it that way, but felt having the 2 separate functions was the 
easiest to read against the tables in the datasheet. What do you think would be the 
best way to organise the reset functions?


ATB,

Mark.

Re: [PATCH v2 4/9] escc: introduce escc_hard_reset_chn() for hardware reset

[Peter Maydell]

On Thu, 2 Sept 2021 at 18:46, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 02/09/2021 16:42, Peter Maydell wrote:
>
> > On Thu, 2 Sept 2021 at 11:33, Mark Cave-Ayland
> > <mark.cave-ayland@ilande.co.uk> wrote:
> >>
> >> This new hardware reset function is to be called for both channels when the
> >> hardware reset bit is written to register WR9. Its initial implementation is
> >> the same as the existing escc_reset_chn() function used for device reset.
> >>
> >> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
> >
> >
> > The datasheet says the only differences between hard and soft
> > reset are for registers W9, W10, W11 and W14. I wasn't expecting
> > the functions to be completely separated out like this.
>
> I did consider doing it that way, but felt having the 2 separate functions was the
> easiest to read against the tables in the datasheet. What do you think would be the
> best way to organise the reset functions?

I think having the hard-reset be "call the soft-reset and then
clear/set the handful of bits that hard-reset touches and
soft-reset doesn't" is probably clearer overall.

-- PMM

Re: [PATCH v2 4/9] escc: introduce escc_hard_reset_chn() for hardware reset

[Mark Cave-Ayland]

On 02/09/2021 20:31, Peter Maydell wrote:

> On Thu, 2 Sept 2021 at 18:46, Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> On 02/09/2021 16:42, Peter Maydell wrote:
>>
>>> On Thu, 2 Sept 2021 at 11:33, Mark Cave-Ayland
>>> <mark.cave-ayland@ilande.co.uk> wrote:
>>>>
>>>> This new hardware reset function is to be called for both channels when the
>>>> hardware reset bit is written to register WR9. Its initial implementation is
>>>> the same as the existing escc_reset_chn() function used for device reset.
>>>>
>>>> Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
>>>
>>>
>>> The datasheet says the only differences between hard and soft
>>> reset are for registers W9, W10, W11 and W14. I wasn't expecting
>>> the functions to be completely separated out like this.
>>
>> I did consider doing it that way, but felt having the 2 separate functions was the
>> easiest to read against the tables in the datasheet. What do you think would be the
>> best way to organise the reset functions?
> 
> I think having the hard-reset be "call the soft-reset and then
> clear/set the handful of bits that hard-reset touches and
> soft-reset doesn't" is probably clearer overall.

Okay thanks, I'll give that a go (probably tomorrow now...)


ATB,

Mark.