On Mon, 1 Nov 2021 at 20:31, Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
The recent ESCC reset changes cause a regression when attemping to use a real
SS-5 Sun PROM instead of OpenBIOS. The Sun PROM doesn't send an explicit reset
command to the ESCC but gets stuck in a loop probing the keyboard waiting for
STATUS_TXEMPTY to be set in R_STATUS followed by SPEC_ALLSENT in R_SPEC.
Reading through the ESCC datasheet again indicates that SPEC_ALLSENT is updated
when a write to W_TXCTRL1 selects async mode, or remains high if sync mode is
selected. Whilst there is no explicit mention of STATUS_TXEMPTY, the ESCC device
emulation always updates these two register bits together when transmitting
data.
My reading of the spec is that this isn't the right behaviour. I think
what we ought to have is:
* in both async and sync mode, TXEMPTY tracks the tx fifo status
(which I think means "set if there is any space in it", contrary to
what the name of the bit implies)
Add extra logic to update both transmit status bits accordingly when writing to
W_TXCTRL1 which enables the Sun PROM to initialise and boot again under QEMU.
Signed-off-by: Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk>
---
hw/char/escc.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/hw/char/escc.c b/hw/char/escc.c
index 0fce4f6324..b33cdc229f 100644
--- a/hw/char/escc.c
+++ b/hw/char/escc.c
@@ -575,6 +575,18 @@ static void escc_mem_write(void *opaque, hwaddr addr,
s->wregs[s->reg] = val;
break;
case W_TXCTRL1:
+ s->wregs[s->reg] = val;
+ if (val & TXCTRL1_STPMSK) {
+ ESCCSERIOQueue *q = &s->queue;
+ if (s->type == escc_serial || q->count == 0) {
+ s->rregs[R_STATUS] |= STATUS_TXEMPTY;
+ s->rregs[R_SPEC] |= SPEC_ALLSENT;
+ }
...should there be an 'else' clause here which clears these
bits if we are now in async mode and the tx queue is not empty ?
+ } else {
+ s->rregs[R_STATUS] &= ~STATUS_TXEMPTY;
+ s->rregs[R_SPEC] |= SPEC_ALLSENT;
+ }
Something is a bit odd with how we currently handle both these bits
For SPEC_ALLSENT:
* it is zero on power-on
* soft-reset leaves it unchanged
* we set it on a write to SERIAL_DATA
* this new code adds two places where we set it
* but there is nowhere where we clear it
For STATUS_TXEMPTY:
* it is set on power-on and soft-reset
* it is set on write to SERIAL_DATA
* it is never cleared
Shouldn't we be clearing these bits somewhere ?
+ /* fallthrough */
case W_TXCTRL2:
s->wregs[s->reg] = val;
escc_update_parameters(s);
At this point all the "fallthrough" is doing is (1) repeat the
setting of s->wregs[s->reg] and (2) call escc_update_parameters().
So I think it would be clearer to instead make the two cases
completely separate, and have
escc_update_parameters(s);
break;
instead of the /* fallthrough */.